user_ldap: Filter groups after nested groups by Cybso · Pull Request #8227 · nextcloud/server

Cybso · 2018-02-07T13:18:13Z

Currently groupsMatchFilter is called before nested groups are resolved.
This basicly breaks this feature since it is not possible to inherit
membership in a group from another group.

Minimal example:

 Group filter: (&(objectClass=group),(cn=nextcloud))
Nested groups: enabled
cn=nextcloud,ou=Nextcloud,ou=groups,dn=company,dn=local
objectClass: group
cn=IT,ou=groups,dn=company,dn=local
objectClass: group
memberOf: cn=nextcloud,ou=Nextcloud,ou=groups,dn=company,dn=local
cn=John Doe,ou=users,dn=company,dn=local
objectClass: person
memberOf: cn=IT,ou=groups,dn=company,dn=local

Since 'cn=IT,ou=groups,dn=company,dn=local' doesn't match the group
filter, John wouldn't be a member of group 'nextcloud'.

This patch fixes this by filtering the groups after all nested groups
have been collected. If nested groups is disabled the result will be the
same as without this patch.

Signed-off-by: Roland Tapken roland@bitarbeiter.net

Currently groupsMatchFilter is called before nested groups are resolved. This basicly breaks this feature since it is not possible to inherit membership in a group from another group. Minimal example: Group filter: (&(objectClass=group),(cn=nextcloud)) Nested groups: enabled cn=nextcloud,ou=Nextcloud,ou=groups,dn=company,dn=local objectClass: group cn=IT,ou=groups,dn=company,dn=local objectClass: group memberOf: cn=nextcloud,ou=Nextcloud,ou=groups,dn=company,dn=local cn=John Doe,ou=users,dn=company,dn=local objectClass: person memberOf: cn=IT,ou=groups,dn=company,dn=local Since 'cn=IT,ou=groups,dn=company,dn=local' doesn't match the group filter, John wouldn't be a member of group 'nextcloud'. This patch fixes this by filtering the groups after all nested groups have been collected. If nested groups is disabled the result will be the same as without this patch. Signed-off-by: Roland Tapken <roland@bitarbeiter.net>

Cybso · 2018-02-07T14:23:17Z

Sorry, this patch is incomplete. It only works for one level on indirection and fails if there is a second level:

 Group filter: (&(objectClass=group),(cn=nextcloud))
Nested groups: enabled
cn=nextcloud,ou=Nextcloud,ou=groups,dn=company,dn=local
objectClass: group
cn=IT,ou=groups,dn=company,dn=local
objectClass: group
memberOf: cn=nextcloud,ou=Nextcloud,ou=groups,dn=company,dn=local
cn=Administrators,ou=groups,dn=company,dn=local
objectClass: group
memberOf: cn=IT,ou=groups,dn=company,dn=local
cn=John Doe,ou=users,dn=company,dn=local
objectClass: person
memberOf: cn=Administrators,ou=groups,dn=company,dn=local

I'll see if I can rewrite the patch and push it.

The previous patch fixed the problem only for one level of indirection because groupsMatchFilter() had been applied on each recursive call (and thus there would be no second level if the first level fails the check). This new implementation replaces the recursive call with a stack that iterates all nested groups before filtering with groupsMatchFilter(). Signed-off-by: Roland Tapken <roland@bitarbeiter.net>

Cybso · 2018-02-07T14:56:40Z

The updated pull request is a more complex change, but it ensures that nested groups are resolved until their root.

Nested groups are now cached in a CappedMemoryCache object to reduce queries to the LDAP backend. Signed-off-by: Roland Tapken <roland@bitarbeiter.net>

codecov · 2018-02-07T17:55:45Z

Codecov Report

Merging #8227 into master will decrease coverage by 1.06%.
The diff coverage is 75%.

@@ Coverage Diff @@## master #8227 +/- ##
============================================
- Coverage 52.75% 51.69% -1.07% - Complexity 24062 25392 +1330 
============================================
Files 1506 1599 +93 Lines 90305 95122 +4817 Branches 1376 1376 ============================================
+ Hits 47643 49175 +1532 - Misses 42662 45947 +3285

Impacted Files	Coverage Δ	Complexity Δ
apps/user_ldap/lib/Group_LDAP.php	`62.61% <75%> (-0.32%)`	`179 <9> (+3)`
apps/dav/lib/Files/FileSearchBackend.php	`42.61% <0%> (-29.27%)`	`71% <0%> (+22%)`
lib/private/Server.php	`83.18% <0%> (-0.1%)`	`282% <0%> (ø)`
settings/templates/settings/admin/tipstricks.php	`0% <0%> (ø)`	`0% <0%> (ø)`	⬇️
.../dav/lib/Connector/Sabre/Exception/InvalidPath.php	`81.81% <0%> (ø)`	`3% <0%> (ø)`	⬇️
...pps/files_external/lib/Service/StoragesService.php	`92.67% <0%> (ø)`	`59% <0%> (?)`
apps/files_external/lib/Lib/Auth/OAuth2/OAuth2.php	`0% <0%> (ø)`	`1% <0%> (?)`
...ernal/lib/Lib/Auth/Password/SessionCredentials.php	`42.85% <0%> (ø)`	`4% <0%> (?)`
apps/files_external/lib/AppInfo/Application.php	`0% <0%> (ø)`	`4% <0%> (?)`
apps/files_external/lib/Lib/Auth/OAuth1/OAuth1.php	`0% <0%> (ø)`	`1% <0%> (?)`
... and 92 more

…anymore. Signed-off-by: Roland Tapken <roland@bitarbeiter.net>

MorrisJobke · 2018-03-06T21:13:22Z

@nextcloud/ldap Could you please have a look at this? It's already lying around for too long.

blizzz · 2018-03-07T10:05:38Z

Currently groupsMatchFilter is called before nested groups are resolved.
This basicly breaks this feature since it is not possible to inherit
membership in a group from another group.

Yes, when the filter is as strict as that. With this changes, the filter does not apply to the subgroups so their members are being read despite that they are not whitelisted.

Cybso · 2018-03-07T10:20:24Z

@blizzz Thanks for your feedback. No, the filter is still applied to the subgroups, but only at the end after all sub groups have been resolved. This is the only way to allow the usage of "transparent" distributions groups (like "IT" and "Administrator" in the example) that are common in larger Active Directory configurations.

blizzz · 2018-03-07T10:31:32Z

@Cybso that's my point. It allows members of those groups to be retrieved while the groups themselves are not white listed.

I am sure it makes sense from one point of view, but rather not from another and constitutes a behavioural change.

Further more, the better approach is perhaps to solely rely on LDAP_MATCHING_RULE_IN_CHAIN so that the LDAP server resolves the the subgroups. I never tested it with OpenLDAP, however it would also be the better performing approach.

Cybso · 2018-03-07T10:39:57Z

@blizzz This is not for members of groups but for memberships of a user, and even if the intermediate groups are not listed the user is a member of the groups at the leaves.

LDAP_MATCHING_RULE_IN_CHAIN resolves the members of a group recursively, but in this case the opposite is required (get all groups where a person is member of, direct or indirect) and as far as I know there is no search syntax that provides this feature besides to recursively climb down the tree.

In other words: the user will be listed when querying for the group's member with LDAP_MATCHING_RULE_IN_CHAIN, so _getGroupDNsFromMemberOf($DN) should return the group, too.

blizzz · 2018-03-07T11:48:03Z

@blizzz This is not for members of groups but for memberships of a user, and even if the intermediate groups are not listed the user is a member of the groups at the leaves.

Ah, excuse me, I was mentally in another place of the code 🙊

LDAP_MATCHING_RULE_IN_CHAIN resolves the members of a group recursively, but in this case the opposite is required (get all groups where a person is member of, direct or indirect) and as far as I know there is no search syntax that provides this feature besides to recursively climb down the tree.

True.

Can I ask also here for unit tests? TestCase offers invokePrivate() for calling restricted methods directly.

MorrisJobke · 2019-01-28T17:03:18Z

@blizzz Should we take it as it is or wait longer?

Cybso · 2019-01-28T18:07:32Z

I am very sorry, but at the moment I am extremely busy both professionally and privately, so I did not have the time to do that and probably will not find an opportunity to get into the test framework in the foreseeable future.

See also #8230

blizzz

I have some nitpicks but it is essentially done. I'd look into it when i am back. It's to the most degree also a bug fix, so should not be affected by the freeze for 16 imho.

blizzz · 2019-02-16T00:27:42Z

+ $seen[$group] = 1;
+
+ // Resolve nested groups
+ if (isset($cachedNestedGroups[$group])) {


this should be $this->cachedNestedGroups[$group]

blizzz · 2019-02-16T00:28:01Z

+
+ // Resolve nested groups
+ if (isset($cachedNestedGroups[$group])) {
+ $nestedGroups = $cachedNestedGroups[$group];


here as well

blizzz · 2019-02-16T00:28:09Z

+ if (!is_array($nestedGroups)) {
+ $nestedGroups = [];
+ }
+ $cachedNestedGroups[$group] = $nestedGroups;


blizzz · 2019-02-16T00:30:23Z

+ }
+ $cachedNestedGroups[$group] = $nestedGroups;
+ }
+ foreach ($nestedGroups as $nestedGroup) {


can be simplified with an array_merge if i am not mistaken

Sign in to view

+ }
 }
+ // Get unique group DN's from those we have visited in the loop
+ $groups = array_keys($seen);


blizzz · 2019-03-01T23:40:23Z

I took over, (extended) follow up PR is #14464

rullzer requested a review from blizzz February 7, 2018 14:03

rullzer added enhancement 3. to review Waiting for reviews feature: ldap labels Feb 7, 2018

Cybso changed the title ~~user_ldap: Filter groups after nexted groups~~user_ldap: Filter groups after nested groupsFeb 7, 2018

Reduce queries to LDAP by caching nested groups
ba241d7
Nested groups are now cached in a CappedMemoryCache object to reduce queries to the LDAP backend. Signed-off-by: Roland Tapken <roland@bitarbeiter.net>

Fixed unit test: groupsMatchFilter will not be called multiple times …
16441a4
…anymore. Signed-off-by: Roland Tapken <roland@bitarbeiter.net>

MorrisJobke added the stale Ticket or PR with no recent activity label Jun 19, 2018

MorrisJobke added 2. developing Work in progress and removed 3. to review Waiting for reviews labels Jul 9, 2018

nextcloud-bot removed the stale Ticket or PR with no recent activity label Jul 9, 2018

blizzz reviewed Feb 16, 2019
View reviewed changes

blizzz mentioned this pull request Mar 1, 2019
resolve user and groups in nested groups first before filtering the results #14464
Merged
2 tasks

blizzz closed this Mar 1, 2019

hottwister mentioned this pull request Mar 15, 2019
LDAP Nested Groups doesn't work with Active Directory #14711
Closed

Uh oh!

Uh oh!

Conversation

Cybso commented Feb 7, 2018

Uh oh!

Cybso commented Feb 7, 2018

Uh oh!

Cybso commented Feb 7, 2018

Uh oh!

codecovBot commented Feb 7, 2018• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

MorrisJobke commented Mar 6, 2018

Uh oh!

blizzz commented Mar 7, 2018

Uh oh!

Cybso commented Mar 7, 2018• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

blizzz commented Mar 7, 2018

Uh oh!

Cybso commented Mar 7, 2018• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

blizzz commented Mar 7, 2018

Uh oh!

MorrisJobke commented Jan 28, 2019

Uh oh!

Cybso commented Jan 28, 2019

Uh oh!

blizzz left a comment

Choose a reason for hiding this comment

Uh oh!

blizzzFeb 16, 2019

Choose a reason for hiding this comment

Uh oh!

blizzzFeb 16, 2019

Choose a reason for hiding this comment

Uh oh!

blizzzFeb 16, 2019

Choose a reason for hiding this comment

Uh oh!

blizzzFeb 16, 2019

Choose a reason for hiding this comment

Uh oh!

This comment was marked as off-topic.

Uh oh!

blizzz commented Mar 1, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

codecovBot commented Feb 7, 2018•
edited
Loading

Cybso commented Mar 7, 2018•
edited
Loading

Cybso commented Mar 7, 2018•
edited
Loading