[Security][Ldap] Fixed issue with password attribute containing an array of values. #18881
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
…values and made password attribute configurable
| private function loadUser($username, Entry $entry) | ||
| { | ||
| return new User($username, $entry->getAttribute('userpassword'), $this->defaultRoles); | ||
| $password = $this->getPassword($entry); |
|
Thank you @csarrazi. |
fabpot
added a commit
that referenced
this pull request
May 26, 2016
…ining an array of values. (csarrazi) This PR was merged into the 3.1 branch. Discussion ---------- [Security][Ldap] Fixed issue with password attribute containing an array of values. | Q | A | ------------- | --- | Branch? | 3.1 | Bug fix? | yes | New feature? | no | BC breaks? | no | Deprecations? | no | Tests pass? | yes | Fixed tickets | #18401 | License | MIT | Doc PR | This PR fixes #18401, as well as other possible issues: * First, the user provider no longer requires a password attribute by default. While this is not mandatory, it is more explicit to not set a password when using the `form_login_ldap` or `http_basic_ldap`, as these two providers don't use a password comparison mechanism, but `ldap_bind()` instead. * Second, the attribute is now configurable. Some implementations actually use different properties to store the user's password attribute. This will enable some users to correctly work with specific configurations. * Third, the user provider normalises the attribute array into a single string. Also, if the attribute has more than one value (which should not be possible), or if is not set, an exception will be thrown, with a clear error message. Commits ------- dbf45e4 [Ldap] Fixed issue with Entry password attribute containing array of values and made password attribute configurable
Merged
lsmith77
reviewed
Dec 13, 2016
| private function getPassword(Entry $entry) | ||
| { | ||
| if (null === $this->passwordAttribute) { | ||
| return; |
There was a problem hiding this comment.
under what circumstance does it make sense to return null here? from my testing this then eventually leads to an error with password hashing not allowing a null parameter. so my question is, should we not consider this an exception?
There was a problem hiding this comment.
It makes sense when you use LDAP Bind based authentication mechanisms, which do not rely on the password being provided as an LDAP attribute.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This PR fixes #18401, as well as other possible issues:
form_login_ldaporhttp_basic_ldap, as these two providers don't use a password comparison mechanism, butldap_bind()instead.