Skip to content

fix detecting cyclic group memberships - #25757

Merged
blizzz merged 1 commit into
masterfrom
fix/noid/ldap-cyclic-group-memberships
Mar 1, 2021
Merged

fix detecting cyclic group memberships#25757
blizzz merged 1 commit into
masterfrom
fix/noid/ldap-cyclic-group-memberships

Conversation

@blizzz

Copy link
Copy Markdown
Member

When there is a cyclic group membership of groups (AD, Samba4), this was not being detected as not all information was taken into account, resulting in an infinite loop.

To reproduce:

  1. In AD/Samba4 have three groups Group A, Group B and Group C.
  2. Group B is member of Group A, Group C is member of Group B and Group A is member of Group C.
  3. LDAP settings Nested groups are enabled and hasMemberOfFilterSupport is disabled.
  4. php occ group:list

@blizzzblizzz added this to the Nextcloud 22 milestone Feb 22, 2021
@blizzzblizzz added the 2. developing Work in progress label Feb 23, 2021
Signed-off-by: Arthur Schiwon <blizzz@arthur-schiwon.de>
@blizzz
blizzzforce-pushed the fix/noid/ldap-cyclic-group-memberships branch from 70943b7 to 4d33449CompareFebruary 23, 2021 21:50
@blizzzblizzz added 3. to review Waiting for reviews and removed 2. developing Work in progress labels Feb 23, 2021
@blizzz

Copy link
Copy Markdown
MemberAuthor

/backport to stable21

@blizzz

Copy link
Copy Markdown
MemberAuthor

/backport to stable20

@blizzz

Copy link
Copy Markdown
MemberAuthor

/backport to stable19

@ChristophWurstChristophWurst left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I know some of these words

@awaw9

awaw9 commented Feb 26, 2021 via email

Copy link
Copy Markdown

@tofuSCHNITZEL

tofuSCHNITZEL commented Feb 27, 2021

Copy link
Copy Markdown
Contributor

LGTM.
while checking the code and testing it with my local setup I found a rather small and simple addition to make the nested groups work with non DN group member attribute groups. you can check it out here:
tofuSCHNITZEL@5bf4fea
if it makes sense we could include it also here since its a rather small fix. (I tested it with my zimbra ldap - but I guess it should be tested with another ldap that uses eg uid as a member attribute as well.)

@blizzz

Copy link
Copy Markdown
MemberAuthor

LGTM.
while checking the code and testing it with my local setup I found a rather small and simple addition to make the nested groups work with non DN group member attribute groups. you can check it out here:
tofuSCHNITZEL@5bf4fea
if it makes sense we could include it also here since its a rather small fix. (I tested it with my zimbra ldap - but I guess it should be tested with another ldap that uses eg uid as a member attribute as well.)

Nice one. I am happy to take it, but prefer it in a follow up PR for issue separation.

Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

3. to reviewWaiting for reviewsbugfeature: ldap

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants

@blizzz@awaw9@tofuSCHNITZEL@ChristophWurst