Problem with dynamic ldap group processing in 9.0.0

[alexweirig]

@blizzz @PVince81
Hello,

after upgrading to 9.0.0 the dynamic ldap group processing is no longer correctly.

I've debugged the group_ldap.php and getUserGroups method and found the following 2 issues:

1. when doing the access->readAttribute for the dynamic group filter, the $uid variable is used. I think the $userDN must be used in order to have LDAP find anything
2. when the user is member of a dynamic group, the group name conversion can be simplified:
   $groupName = $this->access->dn2groupname($dynamicGroup['dn'][0]);
   if(is_string($groupName)) {
   // be sure to never return false if the dn could not be
   // resolved to a name, for whatever reason.
   $groups[] = $groupName;
   }
3. after all the processing has been done, at the end of the method, some cache tests are performed:
   if(isset($this->cachedGroupsByMember[$uid])) {
   (1) $groups = $this->cachedGroupsByMember[$uid];
   } else {
   (2) $groups = array_values($this->getGroupsByMember($uid));
   (3) $groups = $this->access->ownCloudGroupNames($groups);
   $this->cachedGroupsByMember[$uid] = $groups;
   }
   if I leave this tests in the code, the dynamic groups are not working. After this cache test, the group list for the user is empty even though the dynamic groups had been correctly retrieved previously.
   If I comment these lines out, everything seems to work fine.

Why is the cache test only performed at the end of method?
Why would the dynamic groups not be in the cache?
Line (1) overwrites the content (groups found previously) of the groups array, isn't it?
Is line (3) not overwriting the result of line (2)?

I'm not sure if removing the cache processing can cause other problems but I don't understand why the cache is only retrieved at the end or at least why the results from the cache are not simply added to the existing groups array rather than overwriting the contents of the array.

I've attached my version of the getUserGroups method showing my debug code and my comments.

If this sounds OK for you I can try to do a pull request again.
getUserGroups.txt

Cheers,

Alex

[alexweirig]

All this processing makes displaying the user list extremely slow. I've created a screen capture showing how the user list is build and showing the logging that is going on during the process.
I've also attached the owncloud.log file being produced during this process.

The screencast can be downloaded here:
https://owncloud.technolink.lu/index.php/s/KyXYucDljOb2rnR

owncloud.log_save.zip

[PVince81]

@blizzz please have a look

[blizzz]

The screencast can be downloaded here:
https://owncloud.technolink.lu/index.php/s/KyXYucDljOb2rnR

Just tells me "File not found"

[alexweirig]

@blizzz
Sorry, the file was "unshared".
I've shared it again with this link

[alexweirig]

https://owncloud.technolink.lu/index.php/s/sVq1mNqMY856sZ6

[blizzz]

1. when doing the access->readAttribute for the dynamic group filter, the $uid variable is used. I think the $userDN must be used in order to have LDAP find anything

I agree with you

2. when the user is member of a dynamic group, the group name conversion can be simplified

Looks good as well and would improve readability.

3. after all the processing has been done, at the end of the method, some cache tests are performed:

Doh. This makes reading the dynamic groups totally useless, worsening performance since it is done any time. And if the cache is not set, the groups are being re-fetched in the else part, ignoring dynamic members. How did this slip our eyes at all?

However, this does not make sense here anyway, just as you say it yourself

Why is the cache test only performed at the end of method?

There are more checks. On is done directly at the beginning:

        $cacheKey = 'getUserGroups'.$uid;
        if($this->access->connection->isCached($cacheKey)) {
            return $this->access->connection->getFromCache($cacheKey);
        }

This one here in particular is an extra check on groups by member. But I am not entirely sure why we have the extra check in place here, it seems unnecessary. Also looking at the rest of the code in this class. Bit rot, possibly.

I'm not sure if removing the cache processing can cause other problems but I don't understand why the cache is only retrieved at the end or at least why the results from the cache are not simply added to the existing groups array rather than overwriting the contents of the array.

I think we can remove this cache check and the $this->cachedGroupsByMember array in general. However, calling and processing the results from $this->getGroupsByMember($uid) is necessary. Would you like to open a PR, so we can test it? D

All this processing makes displaying the user list extremely slow. I've created a screen capture showing how the user list is build and showing the logging that is going on during the process.

Is this before or after the changes?

[alexweirig]

@blizzz
Thanks for the feedback.

Yes, I'll try to do a PR ...

Is this before or after the changes?
This is (unfortunately) after the changes ...

[blizzz]

Fix in #23344

[PVince81]

Backport PR is here, needs review and testing: #24950

[lock]

This thread has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.