Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork9.7k
[Security][Ldap] Fixed issue with password attribute containing an array of values.#18881
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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Uh oh!
There was an error while loading.Please reload this page.
Conversation
…values and made password attribute configurable
| privatefunctionloadUser($username,Entry$entry) | ||
| { | ||
| returnnewUser($username,$entry->getAttribute('userpassword'),$this->defaultRoles); | ||
| $password =$this->getPassword($entry); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Could be inlined
fabpot commentedMay 26, 2016
Thank you@csarrazi. |
…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 PRfixes#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
| privatefunctiongetPassword(Entry$entry) | ||
| { | ||
| if (null ===$this->passwordAttribute) { | ||
| return; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
under what circumstance does it make sense to returnnull 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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
It makes sense when you use LDAP Bind based authentication mechanisms, which do not rely on the password being provided as an LDAP attribute.
This PRfixes#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.