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.

Sign up for GitHub

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

Jump to bottom

Nested ldap groups #30223

Merged
merged 22 commits into from
Oct 20, 2022
Merged

Nested ldap groups #30223

merged 22 commits into from
Oct 20, 2022

Conversation

CarlSchwan
Copy link
Member

@CarlSchwan CarlSchwan commented Dec 12, 2021 •
edited
Loading

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

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

@CarlSchwan CarlSchwan requested review from blizzz, come-nc, a team 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
Copy link
Contributor

@come-nc come-nc left a comment

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
Copy link
Contributor

come-nc commented Dec 13, 2021

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
Copy link
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
Copy link
Contributor

come-nc commented May 3, 2022

Rebased on master and solved conflicts

@CarlSchwan
Copy link
Member Author

CarlSchwan commented Jul 18, 2022 •
edited
Loading

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
Copy link
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

@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 <carl@carlschwan.eu>
@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 <carl@carlschwan.eu>
@come-nc @CarlSchwan
Refactor _groupMembers to correctly use cache on intermediate results
d07f43d 
Signed-off-by: Côme Chilliet <come.chilliet@nextcloud.com>
@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 <come.chilliet@nextcloud.com>
@come-nc @CarlSchwan
Add tests for nested groups
02ccce1 
Signed-off-by: Côme Chilliet <come.chilliet@nextcloud.com>
@come-nc @CarlSchwan
Refactor group membership listing for nested groups
6ed0d0b 
Signed-off-by: Côme Chilliet <come.chilliet@nextcloud.com>
@come-nc @CarlSchwan
Add testing of nested group membership
7437673 
Signed-off-by: Côme Chilliet <come.chilliet@nextcloud.com>
@come-nc @CarlSchwan
Fix types in docblocks
150e6ad 
Signed-off-by: Côme Chilliet <come.chilliet@nextcloud.com>
@come-nc @CarlSchwan
Removed unused use declaration
69f9e9f 
Signed-off-by: Côme Chilliet <come.chilliet@nextcloud.com>
@come-nc @CarlSchwan
Add missing copyright author in Group_LDAP
604b5ac 
Signed-off-by: Côme Chilliet <come.chilliet@nextcloud.com>
@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 <carl@carlschwan.eu>
@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 <carl@carlschwan.eu>
@CarlSchwan
Fix user_ldap tests
1b12a08 
Signed-off-by: Carl Schwan <carl@carlschwan.eu>
come-nc
come-nc approved these changes Oct 20, 2022
View reviewed changes
Copy link
Contributor

@come-nc come-nc left a comment

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 <carl@carlschwan.eu>
@come-nc @CarlSchwan
Fix LDAP recursive nested group support
746a5fb 
Signed-off-by: Côme Chilliet <come.chilliet@nextcloud.com>
@come-nc @CarlSchwan
Bring back small fixes by Carl
1a6a6c9 
Signed-off-by: Côme Chilliet <come.chilliet@nextcloud.com>
@CarlSchwan
Check if cache is present with isset
60ec5e6 
Otherwise we get false for empty array

Signed-off-by: Carl Schwan <carl@carlschwan.eu>
@CarlSchwan
Fix psalm
99a7529 
Signed-off-by: Carl Schwan <carl@carlschwan.eu>
@CarlSchwan CarlSchwan enabled auto-merge October 20, 2022 11:15
@come-nc
Copy link
Contributor

come-nc commented Oct 20, 2022

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

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

@blizzz blizzz blizzz 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.
4 participants
@CarlSchwan @come-nc @blizzz @szaimen