fix potential unwarranted memberships in nested groups from LDAP

[blizzz]

Fix

• the issue was present only when using PHP based resolving of nested group members. Normally nested members are common in AD (and Samba4) and are resolved per LDAP_MATCHING_RULE_IN_CHAIN by default
• resolving nested members is recursive
• when the cache entry was created it happend for intermediate groups, too, containing members from the parent group
• the check was added to only cache the root group with its members
• a runtime cache stores intermediate ldap read results

Background

I found it when running the occ group:list command over all groups and afterwards (with a hot cache) checking the results for a single group in a similar way.

# change config to clear the cache
php occ ldap:set-config ${CONFIGID} ldapCacheTTL  $(( $(php occ ldap:show-config ${CONFIGID} | grep ldapCacheTTL | tr -d ' ' | cut -d '|' -f3) + 1 ))
# fetch members of all (<=500) groups
php occ group:list --verbose -l500 | wc -l
# check number of members of a specific group
php occ group:list --verbose -l1 -o24 | grep '    - ' | wc -l

The result should match with following LDAP query (number of group members have to be subtracted)

ldapsearch -LLL -a find -H ${LDAP_HOST_URI} -D ${LDAP_BIND_DN} -w ${LDAP_BIND_PWD} -b "${LDAP_BASE}" "memberof:1.2.840.113556.1.4.1941:=${GROUPDN}" dn | egrep '^dn:'

(obviously only with AD or Samba4)

In my test setup I have about 200 LDAP groups with nested members (including groups). About 80 groups are enabled in Nextcloud, the remaining groups may be intermediaries.

[blizzz]

/backport to stable22

[blizzz]

/backport to stable21

[blizzz]

/backport to stable20

[come-nc]

The code is fine, but I do not understand the description of the problem.
Why was caching everything a problem, the nested members ended up being duplicated in the member list?

[blizzz]

The code is fine, but I do not understand the description of the problem. Why was caching everything a problem, the nested members ended up being duplicated in the member list?

Eventually the result are the validated entries in the $seen array. Let me give you a simple example:

1. _groupMembers(GroupA, null)
   1. fetches UserA, UserB, GroupB
   2. _groupMembers(GroupB, [userA, userB, GroupA])
      1. fetches UserC, UserD
      2. caches users as member of GroupB: userA, userB, userC, userD – UserA and UserB because they are already in $seen
2. caches users as member of GroupA: userA, userB, userC, userD – which is correct

[come-nc]

The code is fine, but I do not understand the description of the problem. Why was caching everything a problem, the nested members ended up being duplicated in the member list?

Eventually the result are the validated entries in the $seen array. Let me give you a simple example:

1. _groupMembers(GroupA, null)

   1. fetches UserA, UserB, GroupB

   2. _groupMembers(GroupB, [userA, userB, GroupA])

      1. fetches UserC, UserD
      2. caches users as member of GroupB: userA, userB, userC, userD – UserA and UserB because they are already in $seen

2. caches users as member of GroupA: userA, userB, userC, userD – which is correct

Thank you for your explanation, I understand now.

I read the code for _groupMembers, walkNestedGroups and _getGroupDNsFromMemberOf.
For me the problem is walkNestedGroups is mixing $seen and $list, it should not return items which were in $seen but not in $list.
Also, the recursive pattern is hard to follow because _groupMembers calls walkNestedGroups which calls _groupMembers recursively on all members.
In _getGroupDNsFromMemberOf the fetcher does not add a recursive calls and it’s way easier to follow.

I’m not sure what is the best way to fix this but adding a layer of local cache feels like making it worse.

[blizzz]

For me the problem is walkNestedGroups is mixing $seen and $list, it should not return items which were in $seen but not in $list.

$list is just a working queue, it's empty eventually.

I’m not sure what is the best way to fix this but adding a layer of local cache feels like making it worse.

The runtime one? That's mostly to catch some of the performance that would get lost from fixing the erroneous behaviour.

[blizzz]

After talking with @come-nc i will take another look and see whether it is not possible by indeed caching the correct members by having a seperate/second array next to $seen to track them.

[blizzz]

After talking with @come-nc i will take another look and see whether it is not possible by indeed caching the correct members by having a seperate/second array next to $seen to track them.

@come-nc for the recursion i did not come across a mechanics that would allows the desired effect. My preference would be to get this in as is, also because the changes are simple easy to backport. A more sophisticated solution / restructuring would be better for a next major. I am also not having the time for this at the moment. Ok?

[blizzz]

(rebased)

[come-nc]

@blizzz Is it possible to have a test for this (which would fail before the PR and pass after)?
It would help with future refactoring.

[blizzz]

@blizzz Is it possible to have a test for this (which would fail before the PR and pass after)? It would help with future refactoring.

I have a separate item on my backlog to look again into integration tests with Samba4.

[blizzz]

/backport to stable23