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

[5.2] User: Allow MFA before password reset #44521

Merged
merged 8 commits into from
Dec 18, 2024
Merged

Conversation

Hackwar
Copy link
Member

@Hackwar Hackwar commented Nov 24, 2024

Pull Request for Issue #43311, #39895, #38788, #29576.

Summary of Changes

When having MFA enabled for a user, you can't log out, can't force a password reset and can't setup MFA after first login.

Testing Instructions

  1. Create a user and setup MFA for that user. Save the user.
  2. For this new user, set the "Require password reset" flag.
  3. Try to login from the frontend.
  4. Setup a second user and have the user configuration enforce MFA for that usergroup.
  5. Force the user to reset their password
  6. Try to login from the frontend.
  7. Setup a third user without MFA (remember to disable enforcing MFA in the user configuration again)
  8. Force the user to reset their password
  9. Login with the user and try to logout again

Actual result BEFORE applying this Pull Request

The user is stuck in a redirect loop or can't logout.

Expected result AFTER applying this Pull Request

1-3 The user gets shown the MFA captive view and can type in the required code. Afterwards the user is redirected to a page to update their password.
4-6 The user is redirected to setup MFA and then to reset their password.
7-9 The user is able to logout

Link to documentations

Please select:

  • Documentation link for docs.joomla.org:

  • No documentation changes for docs.joomla.org needed

  • Pull Request link for manual.joomla.org:

  • No documentation changes for manual.joomla.org needed

@bembelimen
Copy link
Contributor

This needs to be moved to a new method and the old method needs a deprecation to be b/c

@Hackwar Hackwar removed the b/c break This item changes the behavior in an incompatible why. HEADS UP label Dec 3, 2024
@Hackwar
Copy link
Member Author

Hackwar commented Dec 3, 2024

This should be b/c now. Please test (again).

@Stevec4
Copy link
Contributor

Stevec4 commented Dec 6, 2024

@Hackwar
I successfully tested this on Joomla 5.2.2.
The user profile is presented, and the password was changed successfully - no loops or errors.
I still have some concerns with the implementation. When the user enters their current password, they are redirected to the index of the site and the profile is displayed requiring the PW change. But I am still able to view and read items on my index.
I feel that it should not allow viewing of anything but the profile page.

Thank you for taking this issue on it has prevented me from rolling out an updated J4/5 site since none of my users could reset their passwords without manual intervention form the admin.

steve

@fgsw
Copy link

fgsw commented Dec 7, 2024

@Stevec4 can you open https://issues.joomla.org/tracker/joomla-cms/44521 and

  • login with your github-account
  • click button "Test this"
  • mark "Tested successfully"

Now the test count as successfull.

@Stevec4
Copy link
Contributor

Stevec4 commented Dec 7, 2024

I have tested this item ✅ successfully on f7778c3


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

@Stevec4
Copy link
Contributor

Stevec4 commented Dec 7, 2024

@fgsw Sorry forgot to submit the test.


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

@dautrich
Copy link

I have tested this item ✅ successfully on 9c16296


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

@pe7er pe7er self-assigned this Dec 12, 2024
@Hackwar
Copy link
Member Author

Hackwar commented Dec 12, 2024

rtc


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

@joomla-cms-bot joomla-cms-bot added the RTC This Pull Request is Ready To Commit label Dec 12, 2024
@pe7er pe7er merged commit c1d35dc into joomla:5.2-dev Dec 18, 2024
4 checks passed
@joomla-cms-bot joomla-cms-bot removed the RTC This Pull Request is Ready To Commit label Dec 18, 2024
@pe7er
Copy link
Contributor

pe7er commented Dec 18, 2024

Thanks @Hackwar !

@krileon
Copy link

krileon commented Jan 14, 2025

This breaks any extension using password_reset_password_tasks to add additional allowed URLs since the new checkUserRequiresReset is now checking only password_reset_password_urls.

Shouldn't checkUserRequiresReset also be checking password_reset_password_tasks? Otherwise you've introduce a backwards compatibility break in a point release. We've just had to do an emergency fix and release because of this.. please stop doing this in minor/point releases.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants