Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork9.7k
[Ldap] cast to string when checking empty passwords#26589
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
[Ldap] cast to string when checking empty passwords#26589
Uh oh!
There was an error while loading.Please reload this page.
Conversation
ismail1432 commentedMar 18, 2018
| Q | A |
|---|---|
| Branch? | master for features / 2.7 up to 4.0 for bug fixes |
| Bug fix? | yes |
| New feature? | no |
| BC breaks? | no |
| Deprecations? | no |
| Tests pass? | yes |
| Fixed tickets | #26525 |
| License | MIT |
| Doc PR | symfony/symfony-docs#... |
| $password =$token->getCredentials(); | ||
| if ('' ===$password) { | ||
| if (empty($password)) { |
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.
We try to avoid usingempty in Symfony. Here, a password like0 would returntrue with empty, which is not what we want here.
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.
Thanks for feedback, What about use theNotBlankValidator conditionif(empty($value) && '0' != $value)
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.
I'd suggest to write this asif ('' === (string) $password) {
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.
Thanks@nicolas-grekas I applied your code in the PR, Hope it's good
nicolas-grekas left a comment• edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
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.
(to be merged on 2.8)
| $password =$token->getCredentials(); | ||
| if ('' ===$password) { | ||
| if (empty($password)) { |
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.
I'd suggest to write this asif ('' === (string) $password) {
I updated the source code following your advices and applied Nicolas code
nicolas-grekas left a comment
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.
(for 2.8)
nicolas-grekas commentedMar 19, 2018
@ismail1432 would you mind adding a test case please? |
ismail1432 commentedMar 21, 2018
@nicolas-grekas sorry for answer now, thistest is not enough ? To be honest I don't have enough experience in Security and Tests to go more far... Sorry. |
nicolas-grekas commentedMar 21, 2018
I mean a test case, something inhttps://github.com/symfony/symfony/blob/master/src/Symfony/Component/Security/Core/Tests/Authentication/Provider/LdapBindAuthenticationProviderTest.php |
ismail1432 commentedMar 21, 2018
thanks for feedback, I add test with |
nicolas-grekas commentedMar 22, 2018
Thank you@ismail1432. |
…l1432)This PR was submitted for the master branch but it was squashed and merged into the 2.8 branch instead (closes#26589).Discussion----------[Ldap] cast to string when checking empty passwords| Q | A| ------------- | ---| Branch? | master for features / 2.7 up to 4.0 for bug fixes <!-- see below -->| Bug fix? | yes| New feature? | no <!-- don't forget to update src/**/CHANGELOG.md files -->| BC breaks? | no <!-- seehttps://symfony.com/bc -->| Deprecations? | no <!-- don't forget to update UPGRADE-*.md files -->| Tests pass? | yes <!-- please add some, will be required by reviewers -->| Fixed tickets |#26525 <!-- #-prefixed issue number(s), if any -->| License | MIT| Doc PR | symfony/symfony-docs#... <!-- required for new features --><!--Quick fix condition that solved the issue.-->Commits-------f276989 [Ldap] cast to string when checking empty passwords