Better cleanup of sharing roles on user purge #19096
Open
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
Now that role name doesn't have to be unique, we don't want to pass args like "private role" or "shared role" on role creation.
Because we no longer can match role name to user email
Reason: decouple user email from private role naming: emails can be
changed or redacted; user id in user-role-association + role type is
sufficient to tie a user to a private role.
The description (i.e., "this is a private role for a user" is inferrable
from the role name ("private role"), which is assigned by default.
We must use a union because when we retrieve roles with a query, we check against: 1) role name 2) email of associated user for private roles We factor out this select into a helper method, which we then test extensively.
Co-authored-by: David López <[email protected]>
Co-authored-by: David López <[email protected]>
1. Update sharing role name removing purged user email 2. If sharing role has only one user association, delete it. 3. Factor out cleanup, add test. Note: this will not work if user email has been udpated after the sharing role was created.
jdavcs
added
kind/bug
kind/enhancement
area/admin
area/database
To accommodate package unit testing
bgruening
reviewed
Nov 1, 2024
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.
Builds on top of #18966 (depends on a relocated fixture from that PR, nothing more)
Currently, a user's email address is used in a sharing role's name. It's not practical to remove it because a sharing role must be identified somehow in the UI; it is possible to pull the current email for each associated user for each sharing role, but that seems like overkill, especially when multiple sharing roles need to be displayed, each potentially associated with multiple users. Using an email in the role name is for role description only: we don't retrieve roles based on email comparison anymore (fixed in #18942).
However, when a user is purged, we don't remove their email from associated sharing roles' names. This PR fixed this: we replace the user's email in the sharing role name with the string "[USER PURGED]", so a sharing role for john and alice, after john is purged, will appear as "sharing role for [USER PURGED], alice". If this sharing role has only one user association remaining with the user being purged, the role will be deleted. This also ensures there'll be no orphan sharing roles in the db (a sharing role without an associated user).
Test included.
To do in a follow-up: handle user email updates (in
api/users.py) by updating email in sharing role name.How to test the changes?
(Select all options that apply)
License