fix(ldap): store last known user groups

[blizzz]

• Resolves: Share recipients loose groupfolder-based shares when the sharer is an LDAP user and was removed from the directory

Summary

The LDAP groups of an LDAP user are fetched live, unless they are cached in Redis or similar. When an LDAP user is being deleted on the LDAP directory, the resulting groups are empty, thus the user may use access to certain files.

When LDAP users are removed on the directy, we still perform normal operations, e.g. shared files are still accessible to the recipients. This works with regular local shares, but the above scenario requires to keep track of the groups.

This approach here is a minimal fix, and not ideal in some aspects, but rather simple and easy to backport.

It is a bit cumbersome to do it connection/configuration-based, but the Group_Proxy can be kept clean and lean this way. Still tainting the Group backend with User bits cannot be prevented.

If I would create the whole logic again, from scratch you could say, we still have the group management abstraction method entry point in the group proxy. There, I would for regular operations directly return the known groups (apart of initially), and not query LDAP. he goal would be to provide fast results to the user. Then, the user sync background job would be extended to also pull and store group information. The information would not be very up to date, the refresh happens twice a day. It should be accompanied with a way to signal Nextcloud to update a user (we have this need anyway). The Provisioning API could get an endpoint and emit an event RequestUpdateExternalUserDataEvent or something like this. Basically analogue to occ ldap:check-user --update (which might need to be extended on the group memberships as well).
But being a behavorial change, a solution like this is only possible for a major release. It could be an opportunity to adjust any other user- and group method to work like that.

@come-nc what do you think? Is this approach acceptable for you, also to backporting?

TODO

• discuss

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)

[come-nc]

I do not get what this is about. Can you write a step by step scenario of what happens with current code which would happen differently with your idea?

We now have a store of known group<>user association in oc_ldap_group_membership since #39446 , and have a check-group --update similar to check-user --update.

There is still the discrepancy of using live membership from LDAP while announced memberships (through events) are done by the background job.
I try to improve on that in #40367 which checks at login if all groups of the user have been announced (which does not add LDAP requests because group membership is fetched at login anyway).

[blizzz]

Can you write a step by step scenario of what happens with current code which would happen differently with your idea?

1. Have a group folder with content.
2. Have no memcache enabled (to not have to wait for TTL running out)
3. Have LDAP users Alice with access to the group folder, and Bob without access to the group folder
4. As Alice, share a folder from the group folder with Bob
5. As Bob, confirm you have received that share and can access it
6. Delete Alice from LDAP
7. Run `occ ldap:check-user? against Alice
8. As Bob, try to access the share now

Expected: you still can access the share

Actual: The folder is visible, cannot be entered (no content shown, error instead).

[come-nc]

Can you write a step by step scenario of what happens with current code which would happen differently with your idea?

1. Have a group folder with content.
2. Have no memcache enabled (to not have to wait for TTL running out)
3. Have LDAP users Alice with access to the group folder, and Bob without access to the group folder
4. As Alice, share a folder from the group folder with Bob
5. As Bob, confirm you have received that share and can access it
6. Delete Alice from LDAP
7. Run `occ ldap:check-user? against Alice
8. As Bob, try to access the share now

Expected: you still can access the share

Actual: The folder is visible, cannot be entered (no content shown, error instead).

And which part is misbehaving here, is it a listener on the user delete which is suppose to update the share somehow?

If I re-read your first message, the problem is that once the user is deleted we do not know in which groups they were before deletion? If so, we can fetch this information from OCA\User_LDAP\Db\GroupMembershipMapper now I think.

[blizzz]

Can you write a step by step scenario of what happens with current code which would happen differently with your idea?

1. Have a group folder with content.
2. Have no memcache enabled (to not have to wait for TTL running out)
3. Have LDAP users Alice with access to the group folder, and Bob without access to the group folder
4. As Alice, share a folder from the group folder with Bob
5. As Bob, confirm you have received that share and can access it
6. Delete Alice from LDAP
7. Run `occ ldap:check-user? against Alice
8. As Bob, try to access the share now

Expected: you still can access the share
Actual: The folder is visible, cannot be entered (no content shown, error instead).

And which part is misbehaving here, is it a listener on the user delete which is suppose to update the share somehow?

getUserGroups() returns an empty array.

If I re-read your first message, the problem is that once the user is deleted we do not know in which groups they were before deletion?

Correct

If so, we can fetch this information from OCA\User_LDAP\Db\GroupMembershipMapper now I think.

Affected instance is on 23 🙄

[blizzz]

Addressed the remarks. Evaluating utilizing the Mapper would be good for a followup? So we can have this working and backported.

[blizzz]

Rebased and added a test.

Alas, I think I found a race condition while smoke testing :-/

[blizzz]

Alas, I think I found a race condition while smoke testing :-/

Should be a bit obvious. If the user is not detected yet as "offline", it still goes through as normal user and there is no userExists check.

Rebased and added a test.

Alas, I think I found a race condition while smoke testing :-/

Have a potential fix for this, relies on #40839, but would contact LDAP each time once it passed the first cache check, when the user object is not OfflineUser. Not so cool. Maybe I get another idea.

[blizzz]

Completed now and the race condition resolved

[blizzz]

/backport to stable27

[blizzz]

/backport to stable26

[backportbot-nextcloud]

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

# Switch to the target branch and update it
git checkout stable27
git pull origin stable27

# Create the new backport branch
git checkout -b fix/foo-stable27

# Cherry pick the change from the commit sha1 of the change against the default branch
# This might cause conflicts. Resolve them.
git cherry-pick abc123

# Push the cherry pick commit to the remote repository and open a pull request
git push origin fix/foo-stable27

More info at https://docs.nextcloud.com/server/latest/developer_manual/getting_started/development_process.html#manual-backport

[backportbot-nextcloud]

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

# Switch to the target branch and update it
git checkout stable26
git pull origin stable26

# Create the new backport branch
git checkout -b fix/foo-stable26

# Cherry pick the change from the commit sha1 of the change against the default branch
# This might cause conflicts. Resolve them.
git cherry-pick abc123

# Push the cherry pick commit to the remote repository and open a pull request
git push origin fix/foo-stable26

More info at https://docs.nextcloud.com/server/latest/developer_manual/getting_started/development_process.html#manual-backport