Skip to content
New issue

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

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

Already on GitHub? # to your account

[3.0] Password improvements #8464

Conversation

Sesquipedalian
Copy link
Member

  • As per internal discussion, we no longer prefix the username to the password in SMF\Security::hashPassword().
  • Moves various functions for generating and validating passwords and authentication codes into SMF\Security.
  • Modernizes our password strength checks by using zxcvbn instead of discredited old rules that require passwords to contain certain characters.

Signed-off-by: Jon Stovell <jonstovell@gmail.com>
Signed-off-by: Jon Stovell <jonstovell@gmail.com>
Signed-off-by: Jon Stovell <jonstovell@gmail.com>
@jdarwood007
Copy link
Member

I tried to change my own password in my profile and get this error:

 Fatal error: Uncaught Error: Class "Security" not found in /Themes/default/Profile.template.php:2755 Stack trace: #0 /Themes/default/Profile.template.php(34): template_error_message() #1 [internal function]: template_profile_above() #2 /Sources/Theme.php(520): call_user_func_array() #3 /Sources/Theme.php(1438): SMF\Theme::loadSubTemplate() #4 /Sources/Utils.php(2397): SMF\Theme::template_header() #5 /Sources/Forum.php(472): SMF\Utils::obExit() #6 /index.php(149): SMF\Forum->execute() #7 {main} thrown in /Themes/default/Profile.template.php on line 2755

@Sesquipedalian
Copy link
Member Author

Derp. I will push a fix for that soon.

Signed-off-by: Jon Stovell <jonstovell@gmail.com>
@Sesquipedalian Sesquipedalian force-pushed the 3.0/password_improvements branch from b5dd454 to 04483c5 Compare February 12, 2025 04:26
@Sesquipedalian
Copy link
Member Author

Fixed

@dragomano
Copy link
Contributor

One more reason to use composer #8240

@jdarwood007
Copy link
Member

So, a scenario I have here is that I logged into my admin account and loaded up this PR. I refreshed the page, and nothing changed, and was still logged in. I couldn't go into the ACP because I failed to authenticate into that.

Shouldn't this trigger a failure due to the hashed password in the cookie no longer being valid?

@Sesquipedalian
Copy link
Member Author

Sesquipedalian commented Feb 12, 2025

Do you have caching enabled? If so, the cached value for your user profile data would still include the old password hash, which would in turn mean that the password hash in the cookie data would still match when checked by User::verifyPassword() during page load.

I don't believe this will be an issue except during the period of time when we are testing this PR. It is only happening now because there is no way for the cache to be aware that you've changed which Git branch you happen to have checked out. Once the PR has been merged, the possibility for a mismatch between the cached password hash and the real password hash will disappear.

In the meantime, just disable caching while testing this PR. Once the PR has been merged, you can turn caching back on.

@jdarwood007
Copy link
Member

I do not.

If thats not a concern, then this is all ready go to.

@Sesquipedalian Sesquipedalian merged commit dda4503 into SimpleMachines:release-3.0 Feb 13, 2025
6 checks passed
@Sesquipedalian Sesquipedalian deleted the 3.0/password_improvements branch February 13, 2025 03:45
# for free to join this conversation on GitHub. Already have an account? # to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants