Skip to content

Nested ldap groups#30223

Merged
come-nc merged 22 commits into
masterfrom
nested_ldap_groups
Oct 20, 2022
Merged

Nested ldap groups#30223
come-nc merged 22 commits into
masterfrom
nested_ldap_groups

Conversation

@CarlSchwan

@CarlSchwan CarlSchwan commented Dec 12, 2021 •
edited
Loading

Copy link
Copy Markdown
Member

This PR adds support for nested groups and also increases the amount of caching. This can be tested with nextcloud/nextcloud-docker-dev#51

Please look at the different commit messages for more explanation of every change

@CarlSchwan CarlSchwan requested review from a team, blizzz, come-nc and icewind1991 and removed request for a team December 12, 2021 13:30
@CarlSchwan CarlSchwan force-pushed the nested_ldap_groups branch 4 times, most recently from 2673c54 to 94cf6f1 Compare December 12, 2021 14:18
@CarlSchwan CarlSchwan mentioned this pull request Dec 13, 2021
Closed
come-nc
come-nc reviewed Dec 13, 2021
View reviewed changes

@come-nc come-nc left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am still against this solution of caching differently intermediate result instead of fixing them.
Fixing them might actually improve performances as they would be cached accross requests like other groups.

@come-nc

come-nc commented Dec 13, 2021

Copy link
Copy Markdown
Contributor

I am still against this solution of caching differently intermediate result instead of fixing them. Fixing them might actually improve performances as they would be cached accross requests like other groups.

@CarlSchwan c21c7f2 What do you think of this for instance?
I have 2 failing tests but they are failing before my changes as well.

@CarlSchwan

CarlSchwan commented Dec 13, 2021

Copy link
Copy Markdown
Member Author

I am still against this solution of caching differently intermediate result instead of fixing them. Fixing them might actually improve performances as they would be cached accross requests like other groups.

@CarlSchwan c21c7f2 What do you think of this for instance? I have 2 failing tests but they are failing before my changes as well.

The patch looks good and also easier to read and understand

@szaimen szaimen added 3. to review Waiting for reviews enhancement labels Jan 31, 2022
@szaimen szaimen added this to the Nextcloud 24 milestone Jan 31, 2022
@come-nc come-nc self-assigned this Mar 7, 2022
@skjnldsv skjnldsv mentioned this pull request Mar 24, 2022
Merged
@blizzz blizzz mentioned this pull request Mar 31, 2022
Merged
This was referenced Apr 7, 2022
Merged
Merged
@blizzz blizzz modified the milestones: Nextcloud 24, Nextcloud 25 Apr 21, 2022
@come-nc come-nc force-pushed the nested_ldap_groups branch from 55e472b to b65ce12 Compare May 3, 2022 15:51
@come-nc

come-nc commented May 3, 2022

Copy link
Copy Markdown
Contributor

Rebased on master and solved conflicts

@CarlSchwan

CarlSchwan commented Jul 18, 2022 •
edited
Loading

Copy link
Copy Markdown
Member Author

Rebased this PR and found some issues :(

With two groups that are circular: A (with 3 members) and B (with 2 members)

  1. when loading the list of the member of A, we see 5 members, this is correct
  2. when loading the list of the member of B, we only see 2 members, this is not correct
  3. Clearing the redis cache
  4. when loading the list of the member of B, we only see 5 members, this is correct
  5. when loading the list of the member of A, we see 3 members, this is not correct

This was fixed with 682a6ac and 4e165b1 in the stable20 fork but applying these patches in this branch doesn't work :(

@CarlSchwan CarlSchwan force-pushed the nested_ldap_groups branch from b65ce12 to 9853ee1 Compare July 18, 2022 08:03
@CarlSchwan

CarlSchwan commented Jul 18, 2022

Copy link
Copy Markdown
Member Author

Found the issue! This

$shouldCacheResult = count($seen) === 0;

got removed and when we ended up caching the intermediate steps, something that works fine for nested groups but not with circular groups. Now locally this patch seems to work.

I added it back and also added back the runtime cache optimization

github-advanced-security[bot]
github-advanced-security AI found potential problems Jul 18, 2022
View reviewed changes
Comment thread apps/user_ldap/lib/Group_LDAP.php Fixed
@CarlSchwan CarlSchwan force-pushed the nested_ldap_groups branch from b55b68b to 7de3156 Compare July 18, 2022 10:16
CarlSchwan and others added 13 commits October 20, 2022 12:09
@CarlSchwan
Unify a bit the types of the fetcher
49aa352 
Now it will only accept a string as parameter instead of either a string
(DN) or a array (complete record).

Signed-off-by: Carl Schwan <[email protected]>
@CarlSchwan
Fix merging list with null
6522f8a 
This fixes some cases observed with the debugger where we end up merging
a non empty list with null. The result is then null and the looping over
the items would then end.

Signed-off-by: Carl Schwan <[email protected]>
@come-nc @CarlSchwan
Refactor _groupMembers to correctly use cache on intermediate results
d07f43d 
Signed-off-by: Côme Chilliet <[email protected]>
@come-nc @CarlSchwan
Small optimisation of _groupMembers
8b19cfc 
This will not change the result as users are check to be existing
 afterwards but avoids this check when we know it’s a group.

Signed-off-by: Côme Chilliet <[email protected]>
@come-nc @CarlSchwan
Add tests for nested groups
02ccce1 
Signed-off-by: Côme Chilliet <[email protected]>
@come-nc @CarlSchwan
Refactor group membership listing for nested groups
6ed0d0b 
Signed-off-by: Côme Chilliet <[email protected]>
@come-nc @CarlSchwan
Add testing of nested group membership
7437673 
Signed-off-by: Côme Chilliet <[email protected]>
@come-nc @CarlSchwan
Fix types in docblocks
150e6ad 
Signed-off-by: Côme Chilliet <[email protected]>
@come-nc @CarlSchwan
Removed unused use declaration
69f9e9f 
Signed-off-by: Côme Chilliet <[email protected]>
@come-nc @CarlSchwan
Add missing copyright author in Group_LDAP
604b5ac 
Signed-off-by: Côme Chilliet <[email protected]>
@CarlSchwan
Only cache base inGroup search
33be3f7 
And not intermediate search for nested groups, this is causing issues
othewise with nested groups

Signed-off-by: Carl Schwan <[email protected]>
@CarlSchwan
Add back runtime cache for intermediate ldap read results
e0fbd39 
This is a small optimization that save a few LDAP queries

Signed-off-by: Carl Schwan <[email protected]>
@CarlSchwan
Fix user_ldap tests
1b12a08 
Signed-off-by: Carl Schwan <[email protected]>
come-nc
come-nc approved these changes Oct 20, 2022
View reviewed changes

@come-nc come-nc left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍 Tested and validated with Carl

@blizzz blizzz added 4. to release Ready to be released and/or waiting for tests to finish and removed 3. to review Waiting for reviews labels Oct 20, 2022
CarlSchwan and others added 5 commits October 20, 2022 13:14
@CarlSchwan
Revert Carl changes on apps/user_ldap/lib/Group_LDAP.php
be5338e 
Signed-off-by: Carl Schwan <[email protected]>
@come-nc @CarlSchwan
Fix LDAP recursive nested group support
746a5fb 
Signed-off-by: Côme Chilliet <[email protected]>
@come-nc @CarlSchwan
Bring back small fixes by Carl
1a6a6c9 
Signed-off-by: Côme Chilliet <[email protected]>
@CarlSchwan
Check if cache is present with isset
60ec5e6 
Otherwise we get false for empty array

Signed-off-by: Carl Schwan <[email protected]>
@CarlSchwan
Fix psalm
99a7529 
Signed-off-by: Carl Schwan <[email protected]>
@CarlSchwan CarlSchwan enabled auto-merge October 20, 2022 11:15
@come-nc

come-nc commented Oct 20, 2022

Copy link
Copy Markdown
Contributor

CI failure unrelated

@come-nc come-nc disabled auto-merge October 20, 2022 13:03
@come-nc come-nc merged commit 00c4c3d into master Oct 20, 2022
@come-nc come-nc deleted the nested_ldap_groups branch October 20, 2022 13:03
@blizzz blizzz mentioned this pull request Oct 21, 2022
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@blizzz blizzz blizzz approved these changes

@come-nc come-nc come-nc approved these changes

@icewind1991 icewind1991 Awaiting requested review from icewind1991

Assignees

@come-nc come-nc

Labels

4. to release Ready to be released and/or waiting for tests to finish enhancement

Projects

None yet

Milestone

Nextcloud 26

Development

Successfully merging this pull request may close these issues.

5 participants

@CarlSchwan @come-nc @blizzz @github-advanced-security @szaimen