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

Fixed #13336 - Save unhashed password if no password provided #13343

Merged
merged 5 commits into from
Sep 14, 2023

Conversation

snipe
Copy link
Owner

@snipe snipe commented Jul 20, 2023

This addresses issue #13336, where if LDAP sync is enabled, and then disabled, the LDAP password remains for users that logged in while LDAP sync was enabled.

By setting passwords to a static string that isn't hashed, the user will never be able to login, since we the process of logging in compares only hashes, and the string *** NO PASSWORD *** will never ever match a submitted hashed password.

We have to be careful here to make sure we do not ever hash that NO PASSWORD string, of course.

This has the added benefit of making importing a large number of LDAP users go faster, since bcrypt does slow things down a bit.

@what-the-diff
Copy link

what-the-diff bot commented Jul 20, 2023

PR Summary

  • Introduction of a centralised password configuration
    A new function called noPassword() has been introduced in the User model. This function serves to centralize the way we manage password handling across different files.

  • Refactoring of code for managing passwords
    The previously scattered code for generating random passwords in LdapSync.php, UsersController.php, LoginController.php, and Ldap.php has been replaced with the newly created centralized noPassword() function located in the User model. This leads to cleaner and easier to maintain code.

  • Minor corrections and adjustments
    Small tweaks were made to further improve code accuracy and performance. This included fixing a typo in the Ldap.php model and refining the password attribute handling in SCIMUser.php.

  • Code Documentation
    An explanation of the noPassword() function was added in the User model. This enhances the readability and understanding of the codebase, fostering efficient collaboration among developers.

@snipe snipe requested review from uberbrady and marcusmoore July 24, 2023 10:29
@snipe
Copy link
Owner Author

snipe commented Jul 24, 2023

We probably also have to add a bit in the settings where we batch update any user with ldap_import (or whatever the field name is) to set it this way as well.

Copy link
Collaborator

@marcusmoore marcusmoore left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Some minor changes but overall 👍🏾

app/Http/Controllers/Api/UsersController.php Outdated Show resolved Hide resolved
app/Models/Ldap.php Outdated Show resolved Hide resolved
app/Models/SCIMUser.php Outdated Show resolved Hide resolved
Copy link
Collaborator

@uberbrady uberbrady left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks good to me - if you take Marcus's comment into consideration and fix that one part.

But I had another idea that might be good -

What if we change the ->noPassword() method to look like this:

function noPassword()
{
     $this->password = "*** NO PASSWORD ***";
}

Then you could make all the places where you're calling this method a little bit simpler -

Instead of $this->password = $this->noPassword() you could just say $this->noPassword().

This is not a hill I'm going to die on, however. Just an idea.

@snipe snipe merged commit 2166d66 into develop Sep 14, 2023
@snipe snipe deleted the fixes/re-scramble_password_if_ldap_pw_sync_not_enabled branch September 14, 2023 13:16
@snipe
Copy link
Owner Author

snipe commented Sep 14, 2023

@uberbrady I think that's definitely worth looking at for a refactor. I'd like to push this out as-is right now though - thank you!

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.

3 participants