LDAP: fix inGroup for memberUid type of group memberships

[blizzz]

fixes#24252

basically, the code that resolves the rdns to full DNs returned the whole records, as @Hexasoft observed, but the following code requires a flat list of DNs.

@Hexasoft@steffen-moser can you confirm it fixes the issue for you?

@tofuSCHNITZEL you are informed because it is relevant for zimbramailforwardingaddress types, I would be surprised if it breaks anything though.

It set the label to "developing", because i want to check one, two things tomorrow.

[steffen-moser]

@blizzz - I think you also need to consider

server/apps/user_ldap/lib/Group_LDAP.php

Line 112 in 57bfe0d

returnin_array($userDN, $this->cachedGroupMembers[$gid]);

as the list there is also not flat according to my debug log.

I've not checked whether

server/apps/user_ldap/lib/Group_LDAP.php

Line 119 in 57bfe0d

$isInGroup = in_array($userDN, $members, true);

needs fixing because I did not run into this if clause on any of our Nextcloud servers.

[tofuSCHNITZEL]

I just ran a few tests and you change makes total sense... now $members really contains dns so the in_array check works.
But there are problems if this is tripped

server/apps/user_ldap/lib/Group_LDAP.php

Line 160 in 57bfe0d

if ($bytes >= 9000000) {

because then $dns is already filled but ins this form:

Array
(
[0] => Array
(
[entryuuid] => Array
(
[0] => c2a1b704-4463-1038-9b2b-b7afed259854
)
[dn] => Array
(
[0] => uid=testaccount,ou=people,dc=email,dc=at
)
[uid] => Array
(
[0] => testaccount
)
[displayname] => Array
(
[0] => testaccount
)
)
[1] => Array
(
[entryuuid] => Array
(
[0] => 77dd1308-5857-1030-8796-392f61712f0b
)
[dn] => Array
(
[0] => uid=user.name,ou=people,dc=email,dc=at
)
[uid] => Array
(
[0] => user.name
)
[zimbramailalias] => Array
(
[0] => user@email.at
)
[displayname] => Array
(
[0] => user
)
)
)

and now the array_reduce part might or might not be run (depending if there are filterParts left)
if its run then using $dns as initial with the array_reduce does not produces the desired output.
if its not run then we have nonsense in $members like before this fix

TLDR:
after over an hour of playing around with different scenarios I think I found the solution that works with all cases.

I committed my changes to this branch... I hope this is okay? (and the proper way?)

I'm also thinking, that a foreach for cleaning up the array would be better to understand than the array_reduce.

we could replace:

 // now we cleanup the users array to get only dns
$dns = array_reduce($users, function (array $carry, array $record) {
if (!in_array($carry, $record['dn'][0])) {
$carry[$record['dn'][0]] = 1;
}
return $carry;
}, []);
$members = array_keys($dns);

with a simple:

 // now create a list of dns
$members = [];
foreach ($users as $user){
$dn = $user['dn'][0];
if (!in_array($dn, $members))
$members[] = $dn;
}

also should we could (should?) move:

server/apps/user_ldap/lib/Group_LDAP.php

Lines 139 to 141 in 57bfe0d

	if (!is_array($members) || count($members) === 0) {
	$this->access->connection->writeToCache($cacheKey, false);
	returnfalse;

after the switch statement

[blizzz]

as the list there is also not flat according to my debug log.

is it possible that this was old information, perhaps pulled it from caching? Since it really only is assigned the value of $members. Also not that cachedGroupMembers is not a plain array, but an instance of CappedMemoryCache that behaves like one.

[blizzz]

@tofuSCHNITZEL good catch with the loop! Also thanks for the commit, that's appreciated. I'd build upon it and turn the last array_reduce to an unsexy, but less hungry and more performant foreach (like you suggest). I'd still work with having the DN as index, as it saves in_arrays and prevents potential duplicates.

Moving the count check down makes sense as well. And dropping the is_array check with it since it is guaranteed by the return type.

[tofuSCHNITZEL]

great ;) should / will this fix be backported to 19 or even earlier? I guess sind its not a huge change we should/could do it. the structure of the file is a bit different pre v20 because of my changes to it for the zimbra support so I could port the changes to the v19 GroupLDAP file.

[blizzz]

…and added unit tests.

great ;) should / will this fix be backported to 19 or even earlier? I guess sind its not a huge change we should/could do it. the structure of the file is a bit different pre v20 because of my changes to it for the zimbra support so I could port the changes to the v19 GroupLDAP file.

Yes, it is a bug fix, and should be brought back down to 18. If we need to touch a bit there, so be it :)

[steffen-moser]

as the list there is also not flat according to my debug log.

is it possible that this was old information, perhaps pulled it from caching? Since it really only is assigned the value of $members. Also not that cachedGroupMembers is not a plain array, but an instance of CappedMemoryCache that behaves like one.

Of course, It is very well possible that this old information came from caching.

We encountered the problem on two servers, but only one of them is actually running using a cache. And only on this server, the if clause, where line 112 resides, was entered, and only there I needed the fix. I haven't done further testing, but maybe emptying the cache would have also helped (after fixing it at the right place where the cache gets filled).

[Hexasoft]

Sorry, I was too busy at work to test the last proposal.
Thank you for your job. For the last change, not sure if it can occurs, but is it certain that $dn = $user['dn'][0]; is always defined? I mean, does adding if (isset($user['dn'][0])) {} is of any interest?
@steffen-moser: before testing, I invalidated all caches (by changing TTL in configuration). Before doing this I also get strange behaviors.

[tofuSCHNITZEL]

Thank you for your job. For the last change, not sure if it can occurs, but is it certain that $dn = $user['dn'][0]; is always defined? I mean, does adding if (isset($user['dn'][0])) {} is of any interest?

I think, since dn is such an integral attribute to ldap, it will always be a part of a ldap query response.

[blizzz]

Thank you for your job. For the last change, not sure if it can occurs, but is it certain that $dn = $user['dn'][0]; is always defined? I mean, does adding if (isset($user['dn'][0])) {} is of any interest?

I think, since dn is such an integral attribute to ldap, it will always be a part of a ldap query response.

Yes, it always returns the dn as it is the de-facto (current!) identifier, even if you ask for a single different attribute:

dapsearch -LLL -a find -H ldap://localhost:7770 -D cn=root,dc=nc,dc=zara -W -b "ou=small,dc=nc,dc=zara" "uid=*" uid Enter LDAP Password: dn: uid=folk_0,ou=users,ou=small,dc=nc,dc=zara
uid: folk_0
dn: uid=folk_1,ou=users,ou=small,dc=nc,dc=zara
uid: folk_1
dn: uid=folk_2,ou=users,ou=small,dc=nc,dc=zara
uid: folk_2
…

[faily-bot]

🤖 beep boop beep 🤖

Here are the logs for the failed build:

Status of 36131: failure

mariadb10.1-php7.3

Show full log

There were 2 warnings:
1) Test\Files\ViewTest::testRenameFailDeleteTargetKeepSource
Trying to configure method "writeStream" which cannot be configured because it does not exist, has not been specified, is final, or is static
2) Test\Files\ViewTest::testCopyFailDeleteTargetKeepSource
Trying to configure method "writeStream" which cannot be configured because it does not exist, has not been specified, is final, or is static
--
There was 1 failure:
1) Test\Files\ObjectStore\ObjectStoreStorageTest::testRenameOverWriteDirectory
Failed asserting that false matches expected 'foo'.
/drone/src/tests/lib/Files/ObjectStore/ObjectStoreStorageTest.php:148

[ChristophWurst]

👍 🐘

[tofuSCHNITZEL]

lgtm

[blizzz]

/backport to stable20

[blizzz]

/backport to stable19

[backportbot-nextcloud]

The backport to stable19 failed. Please do this backport manually.

[tofuSCHNITZEL]

@blizzz how will we do the stable19 backport? will we create a new branch and do the changes there manually? on what commit will this branch be based?

[blizzz]

@blizzz how will we do the stable19 backport? will we create a new branch and do the changes there manually? on what commit will this branch be based?

Yes, we should do it. It's somewhere deep in my list, but if you want to beat me, that would be really appreciated :) A branch called backport/24402/stable19 shall be based against stable19. 'git cherry-pick' the commits in chronological order, and git mergetool to resolve conflicts.

[tofuSCHNITZEL]

@blizzz how will we do the stable19 backport? will we create a new branch and do the changes there manually? on what commit will this branch be based?

Yes, we should do it. It's somewhere deep in my list, but if you want to beat me, that would be really appreciated :) A branch called backport/24402/stable19 shall be based against stable19. 'git cherry-pick' the commits in chronological order, and git mergetool to resolve conflicts.

okay tried my best ;) hopefully I did everything how its supposed to be done ;)
it was interesting that the auto merging at the first cherry-pick replaced the wrong $dns = array_merge($dns, $users); (the one inside the over 9000 if) I guess it just matched the first "similar" line occurrence since the line numbers were different