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.

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

fix: remove caching in fetchListOfGroups #47513

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

kesselb
Copy link
Contributor

@kesselb kesselb commented Aug 27, 2024

Summary

When using nested groups without a memberof overlay, then fetchListOfGroups is called from getGroupsByMember without applying the group filter.

In some setups, the "unfiltered" result is then written back to the group mapping table. That might cause random "An administrator removed you from group" activities.

I was unable to replicate it locally, but we got the feedback that the random activities stopped with the patch applied.

Ref: #42195

TODO

  • CI & Tests
  • Review

Checklist

@kesselb kesselb added bug 2. developing Work in progress labels Aug 27, 2024
@kesselb kesselb added this to the Nextcloud 31 milestone Aug 27, 2024
@kesselb kesselb self-assigned this Aug 27, 2024
@kesselb kesselb force-pushed the bug/noid/weird-ldap-caching branch from b911880 to a81b6be Compare September 10, 2024 13:58
@kesselb kesselb added 3. to review Waiting for reviews and removed 2. developing Work in progress labels Sep 13, 2024
@kesselb kesselb requested review from blizzz and come-nc September 13, 2024 10:19
@come-nc
Copy link
Contributor

come-nc commented Sep 13, 2024

Is that fixing the same issue as #45364?

@kesselb
Copy link
Contributor Author

kesselb commented Sep 16, 2024

Is that fixing the same issue as #45364?

Looks related.

$groupRecords = $this->searchGroups($filter, $attr, $limit, $offset);
$listOfDNs = array_reduce($groupRecords, function ($listOfDNs, $entry) {
$listOfDNs[] = $entry['dn'][0];
return $listOfDNs;
}, []);
$idsByDn = $this->getGroupMapper()->getListOfIdsByDn($listOfDNs);
array_walk($groupRecords, function (array $record) use ($idsByDn) {
$newlyMapped = false;
$gid = $idsByDn[$record['dn'][0]] ?? null;
if ($gid === null) {
$gid = $this->dn2ocname($record['dn'][0], null, false, $newlyMapped, $record);
}
if (!$newlyMapped && is_string($gid)) {
$this->cacheGroupExists($gid);
}
});

Our working theory was that the above block maps LDAP groups that do not match the group filter. That can happen with nested groups, where we cannot use the group filter for the query right away.

I think it should work to combine this pr with #46364: Keep the caching logic, but turn auto-mapping off.

If @blizzz and you think we should keep it, then let's do it. But please keep in mind that the amount of caching logic makes the code really difficult to understand and debug, and therefore removing the cache block improves the maintainability a bit.

@come-nc
Copy link
Contributor

come-nc commented Sep 17, 2024

Quite frankly I think we can remove this logic it looks like it tries to be too smart.
@kesselb Could you test how many extra LDAP queries are fired, if any, with this change when listing the groups of a user or the members of a group?

@kesselb kesselb force-pushed the bug/noid/weird-ldap-caching branch from a81b6be to afe91fb Compare September 20, 2024 15:18
When using nested groups without a memberof overlay, then fetchListOfGroups is called from getGroupsByMember without applying the group filter.

In some setups, the "unfiltered" result is then written back to the group mapping table. That might cause random "An administrator removed you from group" activities.

I was unable to replicate it locally, but we got the feedback that the random activities stopped with the patch applied.

Ref: #42195

Signed-off-by: Daniel Kesselberg <mail@danielkesselberg.de>
@kesselb kesselb force-pushed the bug/noid/weird-ldap-caching branch from afe91fb to a50ce1a Compare September 23, 2024 16:38
@kesselb
Copy link
Contributor Author

kesselb commented Sep 23, 2024

I tried gathering you some reliable data, but's the caching layers, having the mapping in the db already yes/no, .... makes it horrible to get reproducible results.

Login with LDAP user:

ldapCacheTTL: 0

login master, 39 ldap calls, WU1eZOvwNdoB5eGtXzmg
login branch, 39 ldap calls, xH5v3gZjD1DC978P19s8


ldapCacheTTL: 600

(extra login was done before to warmup the cache)

login master, 6 ldap calls, RCeguhBIPMUV5367Y3EP
login branch, 15 ldap calls, aZrSU6ja9xgU3k85USB2

Calls RCeguhBIPMUV5367Y3EP (master)

"ldap_explode_dn"
"ldap_connect"
"ldap_set_option"
"ldap_set_option"
"ldap_set_option"
"ldap_bind"

Calls aZrSU6ja9xgU3k85USB2 (this branch)

"ldap_explode_dn"
"ldap_connect"
"ldap_set_option"
"ldap_set_option"
"ldap_set_option"
"ldap_bind"
"ldap_search"
"ldap_errno"
"ldap_get_entries"
"ldap_parse_result"
"ldap_connect"
"ldap_set_option"
"ldap_set_option"
"ldap_set_option"
"ldap_bind"

@jgrocha
Copy link

jgrocha commented Oct 9, 2024

I may be missing something, but I did not solve the issue on my side with this fix. I've commented out the code on my side, but the problem persists. I'm still getting those emails "An administrator removed you from group ..."

How should I test this? Should I clear something from the BD? Reconfigure LDAP?

@come-nc
Copy link
Contributor

come-nc commented Oct 14, 2024

I may be missing something, but I did not solve the issue on my side with this fix. I've commented out the code on my side, but the problem persists. I'm still getting those emails "An administrator removed you from group ..."

How should I test this? Should I clear something from the BD? Reconfigure LDAP?

You should double-check your LDAP configuration in Nextcloud for group-member association.

@jgrocha
Copy link

jgrocha commented Oct 14, 2024

I may be missing something, but I did not solve the issue on my side with this fix. I've commented out the code on my side, but the problem persists. I'm still getting those emails "An administrator removed you from group ..."
How should I test this? Should I clear something from the BD? Reconfigure LDAP?

You should double-check your LDAP configuration in Nextcloud for group-member association.

Upgrading to the recent 29.0.8 version fixed the problem. This same code is still there (the code commented out in this PR on apps/user_ldap/lib/Access.php) and works as it used to work.

@jgrocha
Copy link

jgrocha commented Oct 22, 2024

I may be missing something, but I did not solve the issue on my side with this fix. I've commented out the code on my side, but the problem persists. I'm still getting those emails "An administrator removed you from group ..."
How should I test this? Should I clear something from the BD? Reconfigure LDAP?

You should double-check your LDAP configuration in Nextcloud for group-member association.

Upgrading to the recent 29.0.8 version fixed the problem. This same code is still there (the code commented out in this PR on apps/user_ldap/lib/Access.php) and works as it used to work.

Upgrading to 30.0.1 restores this issue!

A lot of new email messages with An administrator removed you from group xxxx

@joshtrichards
Copy link
Member

Issue #29832 may or may not also contain some related reports this covers.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
3. to review Waiting for reviews bug
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Bug]: v28 - Receiving erroneous emails about removal from LDAP groups
4 participants