Don't delete empty groups from database #758
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
It was broken in multiple ways: * It deletes the user even if it already existed in the database before this update. * It's not protected by a mutex, so a concurrent update could have successfully added the user, which is then incorrectly deleted here. In addition to that, now that we don't automatically delete empty groups, it still leaves the groups (including the user private group) which were added. Fixing all that would require effort which I'm not sure would be justified. I don't see any issues with users/groups from an authenticated but erroneous login attempt still being in the database, except that it might be a bit surprising to users. Therefore, with this commit we simply leave the updated user and groups as they are.
When a user logs in, we fetch the groups from the provider and update our database accordingly. When this causes a user to be removed from an authd group and that group doesn't have any other users in it, it's currently removed from our database. That's an issue for two reasons: 1. The next time a user who is a member of that group logs in, a new random GID is generated for that group, which means that any existing files owned by the group won't be accessible to members of the group anymore. 2. Whenever a another group is added, the random GID generated for that group can by chance be the same as the GID of the deleted group, allowing members access to existing files owned by the deleted group. With this commit, we don't delete empty groups but just keep them in the database, so that its GID is still reserved and reused the next time a user who is a member of that group logs in.
adombeck
force-pushed
the
757-do-not-delete-empty-groups
branch
from
54e1f1a to
a71ad8b
Compare
didrocks
requested changes
Jan 28, 2025
There was a problem hiding this comment.
That will be a good et necessary change. Thanks for spotting and raising this!
In addition to my comments, I think there is one test missing (once "Delete existing user" is modified). We should have a dedicated tests for "Deleting last user from a group keeps the group record" or something along that line.
That way, we have the 2 separated tests and logic articulated:
- ensuring that when we remove a user, we clean up the group and let other members intacts (which is different from "Remove group from user").
- ensure that removing last user from a group keeps the group record for reusing.
Does it make sense?
internal/users/cache/testdata/golden/TestDeleteUser/Delete_existing_user
Outdated
Show resolved
Hide resolved
... as the name suggests. This allows us to test that removing user1 removes it from that group but doesn't remove other members.
To make it clear that it tests keeping other group members intact.
To make it clear that it tests keeping the group record when the last user of a group is deleted.
didrocks
approved these changes
Jan 28, 2025
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
When a user logs in, we fetch the groups from the provider and update
our database accordingly. When this causes a user to be removed from an
authd group and that group doesn't have any other users in it, it's
currently removed from our database. That's an issue for two reasons:
The next time a user who is a member of that group logs in, a new
random GID is generated for that group, which means that any existing
files owned by the group won't be accessible to members of the group
anymore.
Whenever a another group is added, the random GID generated for that
group can by chance be the same as the GID of the deleted group,
allowing members access to existing files owned by the deleted group.
With this commit, we don't delete an empty group but just keep it in the
database, so that its GID is still reserved and reused the next time a
user who is a member of that group logs in.
UDENG-5873