fix(usergroups): clear cached user groups on pre hooks

[iLinaza]

• Resolves: [Bug]: Shared folders don’t show up for new users created by SAML #45034

Summary

Groups Manager stores the cache of which groups the users belong to. This can be inconsistent at certain times as it is cleared late (in post hooks).

When a user is added to a group the following steps are taken (in OC\Group\Group, addUser method):

1. The preAddUser hook is called.
2. The backend is called to actually add the user to the group.
3. The UserAddedEvent event is dispatched.
4. The postAddUser hook is called.

The cache is cleared in the postAddUser hook, in the Manager. When someone performs operations on the UserAddedEvent event and is checking which groups the user belongs to, the Manager may be returning invalid data since the cache may be in an inconsistent state. The user has already been added to the group but the cache is cleared later, in the postAddUser hook, and the manager returns outdated group list.

Before the cache is cleared, the file_sharing app is doing the sharing of folders to the newly added user (in OCA\Files_Sharing\Listener\UserAddedToGroupListener), in the UserAddedEvent event. But, at this point, when the user is created by SAML, the cache is in a incosistent state because it has not been cleared yet and is being retrieved from cache. It will be cleared later, in the postAddUser hook.

Checklist

• Code is properly formatted
• Sign-off message is added to all commits
• Tests (unit, integration, api and/or acceptance) are included
• Screenshots before/after for front-end changes
• Documentation (manuals or wiki) has been updated or is not required
• Backports requested where applicable (ex: critical bugfixes)

[blizzz]

I am not sure which side effects this would have. Scary change ;) Would it not be better to emit additional Events in user_saml?

[iLinaza]

In my case, am clearing the cache in the pre event, using reflectivity from a custom App, and it is working fine, but I don't know if it can have consequences in other Apps that we don't use.

Clearing the cache of groups to which users belong must be the task immediately before or after the user is added to a group. I understand that, if we could ensure that, doing it in the pre-hook is the last thing to be executed it should be working fine, but I don't think priority can be added to hook subscriptions, as it is possible with Events.

Otherwise, as you say, a new event or Hook could be added, but in the long run the same problem (execution order) could occur if more applications use that event? Alternatively, we could use the already existing event, UserAddedEvent, and add the subscription to clear the cache with the highest possible priority value, instead of using the post- hooks.

[hrenard]

@blizzz, would something like hrenard@0aefd68 be safer ?

[blizzz]

For the record, the underlying code has changed slightly in master.

@blizzz, would something like hrenard@0aefd68 be safer ?

I think effectively it is the same.

ccing @come-nc for another opinion on this matter.

[come-nc]

Hello,

I think the change makes sense.

But it should get migrated to events anyway, so might as well use the priority solution instead and migrate to events right away. It also avoids clearing the cache if the deletion fails.

[A1bi]

This PR fixes a bug in Nextcloud that I was fighting with for years. I am using the Social Login app for OAuth authentication. It is able to create Nextcloud users if they do not yet exist and sync group memberships. The newly created users are correctly added to the Nextcloud groups returned by the OAuth provider. The problem is: They never get access to the group's shares which they obviously should. Simply no entries are created in oc_share. I always have to manually fix this for each user by removing them from the groups and then adding them again in the user management UI. Only then the entries in oc_share are correctly created and the shares suddenly appear to the user.

For a long time I thought this was a bug in the Social Login app, but after debugging this issue for hours, I found this to be caused by the incorrect caching in OC\Group\Manager just as @iLinaza described. Let me explain it based on my case:

1. User logs in to Nextcloud through Social Login (e.g. OAuth) for the first time.
2. Social Login creates the user in Nextcloud.
3. Social Login fetches the groups of the user in order to remove existing group memberships (this way it ensures the groups are always in sync). The user is not a member of any groups yet as the user was just created, but this empty result is already cached by OC\Group\Manager.
4. Social Login adds the user to groups returned by the OAuth provider, calling OC\Group\Group->addUser and entries are created in oc_group_user.
5. OCA\Files_Sharing\Listener\UserAddedToGroupListener is triggered which is responsible for creating the oc_share entries based on group memberships. The group memberships are already established (step 4), but the empty group memberships array from step 3 is still cached, so OC\Group\Manager->getUserIdGroupIds will return no groups, so the user is given no access to any shares of the group, no entries in oc_share are created.
6. The postAddUser event is emitted which OC\Group\Manager acts upon and clears the cache, but too late.

The cache needs to be cleared before the user is added to a group so that UserAddedToGroupListener is not missing the added group that it was just triggered for.

Why isn't this issue happening when adding a user to a group through the user management UI? Simply because step 3 doesn't happen in that case and therefore no empty group membership is cached before the user is added to the group, the cache is still empty. So the issue is then only hidden, but the order of caching and clearing the cache is still incorrect.

This PR contains a simple but important change. To me it's also not a very scary one. We're just changing when the cache is cleared, worst case the cache might to be cleared too eagerly, at least we're not acting upon an outdated cache. I'd be very happy if this gets merged.

I was about to create a PR with a fix myself before I found this one. I am happy to help getting this PR over the finish line if @iLinaza is currently not available to resolve the conflicts.

[come-nc]

Alright, let’s merge this as a quickfix, and these hooks can be moved to events later.
I solved conflicts with master.