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

Fire group membership events from LDAP at login #40367

Merged
merged 7 commits into from
Oct 16, 2023

Conversation

come-nc
Copy link
Contributor

@come-nc come-nc commented Sep 11, 2023

Summary

Rework of #30512 to use new group membership mappings from #39446

TODO

  • Fix it for the share accepting usecase

Checklist

@come-nc come-nc self-assigned this Sep 11, 2023
@come-nc come-nc added bug 2. developing Work in progress labels Sep 11, 2023
@come-nc come-nc added this to the Nextcloud 28 milestone Sep 11, 2023
apps/user_ldap/lib/AppInfo/Application.php Outdated Show resolved Hide resolved
apps/user_ldap/lib/LoginListener.php Outdated Show resolved Hide resolved
apps/user_ldap/lib/AppInfo/Application.php Fixed Show fixed Hide fixed
apps/user_ldap/lib/AppInfo/Application.php Fixed Show fixed Hide fixed
apps/user_ldap/lib/AppInfo/Application.php Fixed Show fixed Hide fixed
}

private function registerBackendDependents(IAppContainer $appContainer, IEventDispatcher $dispatcher) {
private function registerBackendDependents(IAppContainer $appContainer, IEventDispatcher $dispatcher): void {

Check notice

Code scanning / Psalm

DeprecatedInterface Note

Interface OCP\AppFramework\IAppContainer is marked as deprecated
}

public function handle(Event $event): void {
if ($event instanceof PostLoginEvent) {

Check notice

Code scanning / Psalm

RedundantConditionGivenDocblockType Note

Docblock-defined type OCP\User\Events\PostLoginEvent for $event is always OCP\User\Events\PostLoginEvent
@come-nc
Copy link
Contributor Author

come-nc commented Sep 12, 2023

Here is the current state:

It crashes with:

Type : OC\Share20\Exception\ProviderException
Code : 0
Message : Recipient not in receiving group
Fichier : /var/www/html/lib/private/Share20/DefaultShareProvider.php
Ligne : 355

The problem is that the user<->group relation is cached in several places, and in this case there is a specific cache for inGroup.

So the cache should be emptied, but which one and when?
On top of redis, Group_LDAP uses some CappedMemoryCache instances to cache group members as well.

Also, clearing the whole cache each time a user logs in with a new group membership may be too often on big instances with tons of users and groups.

Another approach would be to add a method to add this specific user<>group relation ship to caches. I am gonna try this first.

@come-nc
Copy link
Contributor Author

come-nc commented Sep 12, 2023

Another approach would be to add a method to add this specific user<>group relation ship to caches. I am gonna try this first.

This sounds complicated, there are these caches:

		/** @var CappedMemoryCache<string[]> $cachedGroupMembers array of users with gid as key */
		protected CappedMemoryCache $cachedGroupMembers;
		/** @var CappedMemoryCache<array[]> $cachedGroupsByMember array of groups with uid as key */
		protected CappedMemoryCache $cachedGroupsByMember;
		/** @var CappedMemoryCache<string[]> $cachedNestedGroups array of groups with gid (DN) as key */
		protected CappedMemoryCache $cachedNestedGroups;
		$cacheKey = 'inGroup' . $uid . ':' . $gid;
		$cacheKeyMembers = 'inGroup-members:' . $gid;
		$cacheKey = '_groupMembers' . $dnGroup;
		$cacheKey = 'getUserGroups' . $uid;
		$cacheKey = 'usersInGroup-' . $gid . '-' . $search . '-' . $limit . '-' . $offset;
		$cacheKey = 'usersInGroup-' . $gid . '-' . $search;
		$cacheKey = 'countUsersInGroup-' . $gid . '-' . $search;

The most problematic are usersInGroup search caches because we cannot easily know how many of them there are, and clearing by prefix is not working for Memcached, so not sure if it is ok to rely on it?
Maybe we can add the relationship to the other ones and ignore usersInGroup searches as it should not cause much damage to have outdated data in there.

@come-nc come-nc force-pushed the fix/user_ldap-update-groups-on-login branch from 0ab3781 to e1ba954 Compare September 12, 2023 10:29
@come-nc come-nc changed the title Fix/user ldap update groups on login Fire group membership events from LDAP at login Sep 12, 2023
@come-nc
Copy link
Contributor Author

come-nc commented Sep 12, 2023

The most problematic are usersInGroup search caches because we cannot easily know how many of them there are, and clearing by prefix is not working for Memcached, so not sure if it is ok to rely on it? Maybe we can add the relationship to the other ones and ignore usersInGroup searches as it should not cause much damage to have outdated data in there.

I implemented this solution and it works.

@come-nc come-nc force-pushed the fix/user_ldap-update-groups-on-login branch from f0345e4 to cf811b9 Compare September 12, 2023 14:22
@come-nc come-nc marked this pull request as ready for review September 19, 2023 08:26
@come-nc come-nc added 3. to review Waiting for reviews and removed 2. developing Work in progress labels Sep 19, 2023
@come-nc come-nc requested review from blizzz, a team, icewind1991 and nfebe and removed request for a team September 19, 2023 08:26
Copy link
Member

@blizzz blizzz left a comment

Choose a reason for hiding this comment

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

Not sure the logic really has to run with each login

apps/user_ldap/lib/LoginListener.php Outdated Show resolved Hide resolved
Signed-off-by: Côme Chilliet <come.chilliet@nextcloud.com>
Signed-off-by: Côme Chilliet <come.chilliet@nextcloud.com>
Signed-off-by: Côme Chilliet <come.chilliet@nextcloud.com>
Signed-off-by: Côme Chilliet <come.chilliet@nextcloud.com>
Signed-off-by: Côme Chilliet <come.chilliet@nextcloud.com>
…fore firing the event

Signed-off-by: Côme Chilliet <come.chilliet@nextcloud.com>
It seems now psalm correctly supports this.

Signed-off-by: Côme Chilliet <come.chilliet@nextcloud.com>
@come-nc come-nc force-pushed the fix/user_ldap-update-groups-on-login branch from cf811b9 to 500374a Compare October 12, 2023 08:13
@come-nc
Copy link
Contributor Author

come-nc commented Oct 12, 2023

Not sure the logic really has to run with each login

If my analysis is correct, the groups are fetched to cache on each login anyway, so this does not add much overhead, it only compares with DB.

@come-nc come-nc requested a review from Altahrim October 12, 2023 08:17
@come-nc come-nc merged commit 8212fee into master Oct 16, 2023
36 of 39 checks passed
@come-nc come-nc deleted the fix/user_ldap-update-groups-on-login branch October 16, 2023 08:01
@nickvergessen
Copy link
Member

/backport 500374a to stable27

@nickvergessen
Copy link
Member

/backport 500374a to stable26

@nickvergessen
Copy link
Member

/backport 500374a to stable25

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.

Sharing still broken with ldap groups
4 participants