Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[4.0] Removed required state for Secret Key field #17713

Merged
merged 1 commit into from
Aug 25, 2017

Conversation

eXsiLe95
Copy link
Contributor

@eXsiLe95 eXsiLe95 commented Aug 25, 2017

System Information

  • Running on xampp v3.2.2
  • Windows 10 (1703: 15063:540)
  • Chrome 60.0.3112.101 (64bit)
  • PHP 7.1.7
  • Joomla! 4.0-dev ([email protected])

Steps to reproduce the issue

With Two Factor Authentication (afterwards called TFA) enabled, try to log in to <yourinstallpath>/administrator with a user which has no TFA enabled.
For more information, see __ > Testing instructions__.

Summary of Changes

I removed the required class from the input field. There is a JS running which checks for this class and then adds the required aria-required="true" which isn't needed for the Secret Key field.

Testing instructions

  1. Fresh installation of Joomla! 4.0-dev
  2. Enable TFA in administrator
    1. Go to Extensions > Plugins
      1. Enable Two Factor Authentication - Google Authenticator
      2. Enable Two Factor Authentication - YubiKey
  3. Create users
    1. Go to Users > Manage
    2. Create a new user
    3. Edit the new user and enable Two Factor Authentication
      1. Go to Two Factor Authentication Tab
      2. Select Google Authenticator as Authentication Method
      3. Follow the on screen instructions to set up Google Authenticator
    4. Create a new user
    5. Edit the new user and enable Two Factor Authentication
      1. Got to Two Factor Authentication Tab
      2. Select YubiKey as Authentication Method
      3. Follow the on screen instructions to set up YubiKey Authenticator
  4. Go to backend/administrator <yourinstallpath/administrator
    1. Test without TFA
      1. Try to log in with superuser with wrong password
      2. Try to log in with superuser with wrong password and additional secret key (is always wrong)
      3. Try to log in with superuser with correct password and additional secret key (is always wrong)
      4. Login with superuser without TFA
      5. Log out
    2. Test with Google TFA
      1. Try to log in with Google TFA user with wrong password but no secret key
      2. Try to log in with Google TFA user with wrong password and wrong secret key
      3. Try to log in with Google TFA user with wrong password but correct secret key
      4. Try to log in with Google TFA user with correct password but no secret key
      5. Try to log in with Google TFA user with correct password but incorrect secret key
      6. Login with the user with Google TFA with the login box
      7. Log out
    3. Test with YubiKey TFA
      1. Try to log in with YubiKey TFA user with wrong password but no secret key
      2. Try to log in with YubiKey TFA user with wrong password and wrong secret key
      3. Try to log in with YubiKey TFA user with wrong password but correct secret key
      4. Try to log in with YubiKey TFA user with correct password but no secret key
      5. Try to log in with YubiKey TFA user with correct password but incorrect secret key
      6. Login with the user with YubiKey TFA with the login box
      7. Log out

If you have any other ideas to test this, please think outside the box!

Expected result

Checking if a user has TFA enabled in PHP is laborious (would be more of a JS thing). Therefore, it is okay to display the Secret Key field, but ignore it for users with no TFA enabled.

For users with not TFA enabled for them, it should look like this and login needs to be possible:

image

Actual result

At the moment, login with non-TFA-users in administrator is not possible. The Secret Key field is always required.

image

Summary of Changes

I removed the required class from the input field. There is a JS running which checks for this class and then adds the required aria-required="true" which isn't needed for the Secret Key field.

Additional comments

This is a bugfix according to the bug I found in #17687
This fix is compatible with the changes made in #17687

Documentation Changes Required

The template file (default.php) isn't really documented so there are no changes needed.

Developed @icampus

@joomla-cms-bot joomla-cms-bot changed the title Removed required state for Secret Key field [4.0] Removed required state for Secret Key field Aug 25, 2017
@brianteeman
Copy link
Contributor

I have tested this item ✅ successfully on e874dc4


This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/17713.

@wilsonge
Copy link
Contributor

Awesome work. Thanks for your first Joomla PR!!

@wilsonge wilsonge merged commit 824023c into joomla:4.0-dev Aug 25, 2017
@wilsonge wilsonge added this to the Joomla 4.0 milestone Aug 25, 2017
roland-d added a commit to roland-d/joomla-cms that referenced this pull request Aug 31, 2017
…olandd-4.0-dev

* '4.0-dev' of https://github.com/joomla/joomla-cms: (35 commits)
  Delete redis handler in favor of fw handler (joomla#17798)
  Remove deprecated JArrayHelper (joomla#17795)
  [4.0] codestyle (joomla#17779)
  Update error renderers for PHP 7 code structure, update Exception/Throwable references to only reference Throwable (joomla#17750)
  Improve article association links
  Fix parsing routes with language filter enabled
  Fix JString use
  [4.0] Fix content margin if no "top" modules are assigned (joomla#17699)
  Removed required state for Secret Key field (joomla#17713)
  [4.0] [installation]  set proper default for lastResetTime (joomla#16847)
  [4.0] Remove FOF From Joomla Core (joomla#17687)
  [4.0] Add Controller suffix to extension controllers (joomla#17624)
  Fix menu association form field not loading
  remove html imports (joomla#17691)
  [4.0] Update Bootstrap to beta-1 (joomla#17496)
  Move files
  [4.0] Cleanup classmap and include it properly for stubs generation (joomla#17667)
  Add back class that got deleted somewhere
  Fix Sql field class name (joomla#17666)
  [4.0] Fix namespaced form fields Part 2 (joomla#17664)
  ...
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants