Change 702976 had a related patch set uploaded (by Robert Vogel; author: Robert Vogel):

[mediawiki/extensions/LDAPAuthorization@REL1_31] Check all values of multi-value attributes

https://gerrit.wikimedia.org/r/702976

Hi! Thanks for reporting! I have provided a patch. @MarkAHershberger , could you please to the review? If you merge the patch, please cherry-pick the change to REL1_35 and master if possible. If there is a merge conflict, please ping me. I will then do the cherry-picking manually. Thanks.

@MarkAHershberger Thanks for reviewing: Regarding your comment on https://gerrit.wikimedia.org/r/c/mediawiki/extensions/LDAPAuthorization/+/702976/3/tests/phpunit/Requirement/MatchAttributesTest.php#13

I have implemented this change but it will break other changes. As I write in the review: This is a design decision.

I have implemented a change: https://gerrit.wikimedia.org/r/c/mediawiki/extensions/LDAPAuthorization/+/702976/3..4/tests/phpunit/Requirement/MatchAttributesTest.php
But now other tests hat to be changed. @MarkAHershberger What do you think. Does it match your expectation this way?

In T280875#7197180, @Osnard wrote:

I have implemented a change: https://gerrit.wikimedia.org/r/c/mediawiki/extensions/LDAPAuthorization/+/702976/3..4/tests/phpunit/Requirement/MatchAttributesTest.php
But now other tests hat to be changed. @MarkAHershberger What do you think. Does it match your expectation this way?

I think this change deviates from the LDAP standard. For T280875-multivalue-attributes, I'd still expect true because the query is (SOMEMULTIVALUEATTRIBUTE=value2) and the attribute values are value1 and value2 - by the LDAP standard, this should match.

For T280875-multivalue-attributes-9, I interpreted the query as &(SOMEMULTIVALUEATTRIBUTE=value2)(SOMEMULTIVALUEATTRIBUTE=value1) which would return true because both operands of the AND are true, but the test checks for false. Analogously, for T280875-multivalue-attributes-3, I'd expect false.

Change 702976 merged by jenkins-bot:

[mediawiki/extensions/LDAPAuthorization@REL1_31] Check all values of multi-value attributes

https://gerrit.wikimedia.org/r/702976

In T280875#7197482, @DanielsThomas wrote:

I think this change deviates from the LDAP standard.

You appear to be more familiar with the standard than I am. Could you point me to the RFC and section of the standard that you are referring to?

Just to clarify: I would really like to have some standard to refer to here. If you know about something I don't, please share.

In T280875#7197180, @Osnard wrote:

@MarkAHershberger What do you think. Does it match your expectation this way?

I think it works and, as I commented on the code, I like using the array_diff, array_intersect, etc functions here since it seems like these are set queries.

But, obviously, I could be wrong, and someone who knows more about LDAP than me could point me to something that clearly shows I'm missing something that the LDAP folx have already thought about.

Change 703223 had a related patch set uploaded (by MarkAHershberger; author: MarkAHershberger):

[mediawiki/extensions/LDAPAuthorization@REL1_31] Multi-value checks that fail

https://gerrit.wikimedia.org/r/703223

Quoting https://ldap.com/ldap-filters/ :

An equality filter is used to determine whether an entry contains a specified attribute value. If an entry includes the specified value, regardless of whether it has any other values for the target attribute, then that entry will match an equality filter for that value.

Additionally, attributes may also be matched incorrectly. The equality check is now done with array_diff, but when an LDAP server processes a filter, it's a more complicated process. Search filters get mapped on a matching rule based on the type of the attribute (https://datatracker.ietf.org/doc/html/rfc4511#section-4.5.1.7 and https://datatracker.ietf.org/doc/html/rfc4512#section-4.1.2 ). Matching rules are opaque (their implementation is internal to the server) and identified based on their OID (https://datatracker.ietf.org/doc/html/rfc4512#section-4.1.3 ). Several basic matching rules are defined at https://datatracker.ietf.org/doc/html/rfc4517#section-4 but a server can define custom matching rules under its own OIDs. It is therefore not possible to cover all cases.

However, it would propose to deal with this in a way that prevents these kind of issues in the future (and the additional maintenance). Instead of doing the filtering ourselves, it would make more sense to let the LDAP server do this (after all, filtering is a core LDAP feature). This would also make certain searches possible that aren't just equalities without any additional work.

The predecessor of the LDAP Stack, https://www.mediawiki.org/wiki/Extension:LDAP_Authentication, also asked for a filter string in its configuration which got passed to the LDAP server. It also allows for more freedom in your query, such as less/greater than or substrings, which are currently not possible with LDAPAuthorization.

In general, it does not seem like a good idea to provide an interface to the user that looks like it's an LDAP search filter, but isn't. This will probably cause confusion and additional bug reports in the future.

Could you please point me to the configuration of the old "Extension:LdapAuthentication" you are referring to?

Sure, the following pages outline how to configure that extension:

https://www.mediawiki.org/wiki/Extension:LDAP_Authentication/Configuration_Options
https://www.mediawiki.org/wiki/Extension:LDAP_Authentication/Examples

This is our configuration for that extension:

# LDAP Auth
require_once( "$IP/extensions/LdapAuthentication/LdapAuthentication.php" );
$wgAuth = new LdapAuthenticationPlugin();

$wgLDAPDomainNames = array('LOKO');
$wgLDAPServerNames = array('LOKO' => 'localhost');
$wgLDAPEncryptionType = array('LOKO' => 'none');
$wgLDAPSearchStrings = array('LOKO' => 'cn=USER-NAME,ou=people,ou=accounts,dc=loko,dc=be');
$wgLDAPAuthAttribute = array('LOKO' => '&(orgLOKOGeactiveerd=TRUE)(orgLOKORechtAlias=cn=Kennisdatabank,ou=rights,ou=accounts,dc=loko,dc=be)');

$wgGroupPermissions['*']['autocreateaccount'] = true; # Necessary to enable new users to log in

Hi, it's been a while since we last talked about this, so I wondered what your thoughts are on the approach of using an LDAP search filter string. Our organisation can look into helping with its implementation, if it sounds good to you.

Can you confirm that https://gerrit.wikimedia.org/r/c/mediawiki/extensions/LDAPAuthorization/+/702976/ didn't address you use case properly?

In general having an "authentication rule" that just checks for the return of a certain LDAP search filter string sounds very good to me. I believe best would be to just implement a new IRequirement class [1]. It should be straight forward to do so. Just get the filter string from the config and run a search against the LDAP resource. You will need to add it to the RequirementsChecker also [2].

Patches are always welcome!

[1] https://github.com/wikimedia/mediawiki-extensions-LDAPAuthorization/blob/master/src/IRequirement.php
[2] https://github.com/wikimedia/mediawiki-extensions-LDAPAuthorization/blob/master/src/RequirementsChecker.php#L46-L51

Change 722867 had a related patch set uploaded (by Thomas Daniels; author: Thomas Daniels):

[mediawiki/extensions/LDAPAuthorization@master] Add requirement to check if user matches LDAP query

https://gerrit.wikimedia.org/r/722867

Can you confirm that https://gerrit.wikimedia.org/r/c/mediawiki/extensions/LDAPAuthorization/+/702976/ didn't address you use case properly?

Yes, I can confirm that.

I have submitted a patch for review here: https://gerrit.wikimedia.org/r/c/mediawiki/extensions/LDAPAuthorization/+/722867 - it adds a new config value 'query' to 'rules'

Change 722886 had a related patch set uploaded (by Robert Vogel; author: Thomas Daniels):

[mediawiki/extensions/LDAPAuthorization@REL1_35] Add requirement to check if user matches LDAP query

https://gerrit.wikimedia.org/r/722886

Change 722887 had a related patch set uploaded (by Robert Vogel; author: Thomas Daniels):

[mediawiki/extensions/LDAPAuthorization@REL1_31] Add requirement to check if user matches LDAP query

https://gerrit.wikimedia.org/r/722887

Thank you very much for you contribution! I will review and merge it as soon as possible.

Change 722867 merged by jenkins-bot:

[mediawiki/extensions/LDAPAuthorization@master] Add requirement to check if user matches LDAP query

https://gerrit.wikimedia.org/r/722867

Change 722887 merged by jenkins-bot:

[mediawiki/extensions/LDAPAuthorization@REL1_31] Add requirement to check if user matches LDAP query

https://gerrit.wikimedia.org/r/722887

Change 722886 merged by jenkins-bot:

[mediawiki/extensions/LDAPAuthorization@REL1_35] Add requirement to check if user matches LDAP query

https://gerrit.wikimedia.org/r/722886

Your changes have been merged into branches REL1_31, REL1_35 and master. Could you please check if I have updated the documentation properly? https://www.mediawiki.org/w/index.php?diff=4820113&oldid=4801930&title=Extension:LDAPAuthorization&type=revision&diffmode=visual

Looks right, thank you!

Change 703223 abandoned by Umherirrender:

[mediawiki/extensions/LDAPAuthorization@REL1_31] Multi-value checks that fail

Reason:

This release is EOL

https://gerrit.wikimedia.org/r/703223

Change 903547 had a related patch set uploaded (by Robert Vogel; author: MarkAHershberger):

[mediawiki/extensions/LDAPAuthorization@master] Multi-value checks that fail

https://gerrit.wikimedia.org/r/903547

Change 903547 abandoned by Robert Vogel:

[mediawiki/extensions/LDAPAuthorization@master] Multi-value checks that fail

Reason:

Outdated

https://gerrit.wikimedia.org/r/903547