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

Fix unexpected handling of the 'attributes' setting #397

Closed
wants to merge 1 commit into from

Conversation

@tvdijen
Copy link
Member

@tvdijen tvdijen commented May 31, 2016

In authsources.php, taken from the ldap-example:
// Which attributes should be retrieved from the LDAP server.
// This can be an array of attribute names, or NULL, in which case
// all attributes are fetched.
'attributes' => NULL,

This suggests that an empty array would return no attributes, but in fact it returns them all, just like a NULL value.
This change fixes that to make this setting act as you would expect and also takes care of a couple of attributes that were being reused within the same function.

In authsources.php, taken from the ldap-example:
        // Which attributes should be retrieved from the LDAP server.
        // This can be an array of attribute names, or NULL, in which case
        // all attributes are fetched.
        'attributes' => NULL,

This suggests that an empty array would return no attributes, but in fact it returns them all, just like a NULL value.
This change fixes that to make this setting act as you would expect and also takes care of a couple of attributes that were being reused within the same function.
@tvdijen
Copy link
Member Author

@tvdijen tvdijen commented May 31, 2016

Or, on second thought, would it be better to take care of this case at the end of the validate-function?

@coveralls
Copy link

@coveralls coveralls commented May 31, 2016

Coverage Status

Coverage decreased (-0.008%) to 16.395% when pulling 4293c45 on tvdijen:patch-3 into 76506e1 on simplesamlphp:master.

@jaimeperez
Copy link
Member

@jaimeperez jaimeperez commented Jun 3, 2016

Hi @tvdijen! Thanks so much for this!

You are right, the documentation is misleading and we should fix it (the issue, not the documentation, as I totally believe we should have a way to tell that we don't want any attributes). On the other hand, I'm a bit concerned about backwards-compatibility, as this change could break existing configurations (people having an empty array there because it works to get all the attributes, and all of a sudden getting nothing back from the LDAP after upgrading.

I'm a bit tempted to put this on hold until we have released 1.15 and we can start breaking things properly in 2.0. However, the LDAP class is so incredibly crappy that it is high up in the list for a refactoring (and even substituting it by a new class), and that would break your PR.

What do you think? Is it urgent for you to have this solved?

@jaimeperez jaimeperez added the bug label Jun 3, 2016
@tvdijen
Copy link
Member Author

@tvdijen tvdijen commented Jun 3, 2016

No, it's not urgent at all.. It's just something I ran into and it only affects my admin logins.

Another thing I noticed, but don't have a solution for, is that you can brute-force someone's password without the account getting locked out.. This kind of contradicts to our (most?) company's security policy.

@jaimeperez
Copy link
Member

@jaimeperez jaimeperez commented Jun 3, 2016

Thanks for the feedback!

Regarding the brute force attack, yes, I'm aware of that. I myself wrote a script some time ago to brute-force the admin login form 😅

@tvdijen
Copy link
Member Author

@tvdijen tvdijen commented May 16, 2019

@tvdijen tvdijen closed this May 16, 2019
@tvdijen tvdijen deleted the tvdijen:patch-3 branch Aug 31, 2019
@tvdijen tvdijen restored the tvdijen:patch-3 branch Aug 31, 2019
@tvdijen tvdijen deleted the tvdijen:patch-3 branch Aug 31, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

3 participants
You can’t perform that action at this time.