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

EZP-29703: Legacy LDAP users cause error message on login #1394

Merged
merged 2 commits into from
Apr 29, 2019

Conversation

glye
Copy link
Member

@glye glye commented Oct 5, 2018

https://jira.ez.no/browse/EZP-29703
Ready for review

Users created by the LDAP login handler have password_hash_type = 0 and password_hash is empty string. On login the standard login handler runs first. It currently doesn't recognise these users and logs an error: Password hash type ID '0' is not recognized. Defaulting to eZUser::DEFAULT_PASSWORD_HASH. We should accept this case without errors.

Resolved by adding new type eZUser::PASSWORD_HASH_EMPTY = 0 and by not running createHash() for this hash type. Log error if it still happens.

@glye glye requested a review from andrerom October 5, 2018 13:40
@andrerom
Copy link
Contributor

andrerom commented Oct 5, 2018

As I think we did the password hash things on 2017.xx release, you may rebase this for 2017.12 branch

@glye glye changed the base branch from master to 2017.12 October 5, 2018 13:50
@glye glye changed the base branch from 2017.12 to master October 5, 2018 13:51
@andrerom andrerom changed the base branch from master to 2017.12 October 5, 2018 13:55
@andrerom
Copy link
Contributor

andrerom commented Oct 5, 2018

@glye change base branch here first, then force push. So Travis gets right base branch to merge into and test for when it is triggered.

PS: remember to pull in changes on 2017.12 before rebasing btw, added commit there to enable Travis there 20-30min ago.

@glye glye force-pushed the ezp29703_legacy_ldap_users_cause_error branch from 2f5a4c7 to aeaca6a Compare October 5, 2018 14:01
@andrerom
Copy link
Contributor

andrerom commented Oct 5, 2018

1) eZTextFileUserTest::testLoginWrongPassword
Failed asserting that true matches expected false.

@glye glye force-pushed the ezp29703_legacy_ldap_users_cause_error branch from aeaca6a to eb74391 Compare October 8, 2018 15:11
@glye
Copy link
Member Author

glye commented Oct 8, 2018

Update to fix textfile handler test error.

@glye
Copy link
Member Author

glye commented Oct 11, 2018

@andrerom Ok now that tests are passing?

@andrerom
Copy link
Contributor

Kind of, but I'm also a bit unsure if the code in createHash is correct or not, does it really enter that code path when ldap is correctly configured? if not, maybe we should rather fail here as it like @gggeek points out feel wrong that we silently accept.

@glye glye force-pushed the ezp29703_legacy_ldap_users_cause_error branch from eb74391 to 40e3449 Compare October 31, 2018 15:44
@glye
Copy link
Member Author

glye commented Oct 31, 2018

@andrerom @gggeek Rewrite after the release of security advisory http://share.ez.no/community-project/security-advisories/ezsa-2018-005-passwordless-login-for-ldap-users

  • Let authenticateHash() fail immediately on empty login, empty password, or hash type PASSWORD_HASH_EMPTY
  • Ensure that createHash() is not run for hash type PASSWORD_HASH_EMPTY
  • Log error if createHash() is somehow still run for hash type PASSWORD_HASH_EMPTY or for unknown hash type.

Copy link
Contributor

@andrerom andrerom left a comment

Choose a reason for hiding this comment

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

There are some cases in ldap which could be changed to use PASSWORD_HASH_EMPTY now.

@glye
Copy link
Member Author

glye commented Nov 1, 2018

  • Reshuffled createHash else-if for readability
  • Used self::PASSWORD_HASH_EMPTY for 0 in eZLDAPUser (found just one case)

@glye
Copy link
Member Author

glye commented Nov 8, 2018

@gggeek I guess you're busy these days, but if you can take a look, the code is more like what you wanted now.

@glye glye requested a review from alongosz November 8, 2018 13:10
@glye
Copy link
Member Author

glye commented Nov 23, 2018

Sent to QA.

@andrerom andrerom added this to the 2019.03 milestone Mar 21, 2019
@andrerom andrerom requested a review from emodric April 2, 2019 14:39
@andrerom andrerom merged commit 01930a9 into 2017.12 Apr 29, 2019
@andrerom andrerom deleted the ezp29703_legacy_ldap_users_cause_error branch April 29, 2019 18:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
3 participants