fix(auth): do not update email in UserDetailsEndpoint#119253
Conversation
github-actions
Bot
added
the
Scope: Backend
| raise serializers.ValidationError("That username is already in use.") | ||
| return value | ||
|
|
||
| def validate(self, attrs: dict[str, Any]) -> dict[str, Any]: |
There was a problem hiding this comment.
It looks like BaseUserSerializer is inherited by a bunch of other serializers, so removing this might affect how the other serializers work.
There was a problem hiding this comment.
I think this is actually a good consequence - the sync removal now applies uniformly to regular, superuser, and staff edits. I don't think changing username should ever change email, but let me take a look at how BaseUserSerializer is being used.
Looks like BaseUserSerializer is only being used for the 3 other UserSerializers in this file, which are only used here and in 1 other place: write_relocation_import on the User model. (note there is another unrelated UserSerializer in src/sentry/users/api/serializers/user.py)
The validate() function I removed was mutating validated_data so the changes it caused would only persist if serializer.save() was used.
This is not the case for the uses in src/sentry/users/models/user.py, which use self.save(), so the removed code was already not being used in write_relocation_import
| ) | ||
|
|
||
| serializer_cls = BaseUserSerializer | ||
| serializer_cls: type[BaseUserSerializer] |
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
| attrs.setdefault("email", attrs["username"]) | ||
| # Reject a value already taken as a username OR another user's email | ||
| # because login resolves username before falling back to email. | ||
| if others.filter(Q(username__iexact=value) | Q(email__iexact=value)).exists(): |
There was a problem hiding this comment.
Hmm... I think relocation currently allows for potentially duplicate emails. Would this change the logic for relocation?
There was a problem hiding this comment.
Good call - relocation uses a "resolve-and-insert" method m rather than failing and rejecting on username collision.
I extended this behavior by handling relocated user's username == existing user's email the same way username collisions were handled.
Relocated users are created in recovery state, when they successfully log in they are prompted to change their username so they can change their username if they don't like it. (added the username == email check to this form validation as well)
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 1 potential issue.
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit 3b06650. Configure here.
f9fde73 to
57dc341
Compare
| """ | ||
| username_or_primary_taken = ( | ||
| cls.objects.exclude(id=exclude_user_id) | ||
| .filter(Q(username__iexact=username) | Q(email__iexact=username)) |
There was a problem hiding this comment.
Thoughts on defensively adding a email_unique check too? The user's email doesn't always match the email_unique 🙃
![sentry[bot]](https://avatars.githubusercontent.com/in/12637?s=60&v=4){kind=small}
| verified_email_taken = ( | ||
| UserEmail.objects.filter(email__iexact=username, is_verified=True) | ||
| .exclude(user_id=exclude_user_id) | ||
| .exists() | ||
| ) |
There was a problem hiding this comment.
If username_or_primary_taken is true, you could return early saving a query.
3 participants
Removes the verified email enforcement and the
validateoverride fromUserDetailsEndpoint.put()that incorrectly synced email when username changed. Changing username should never require a verified email nor implicitly update the user's primary email.UserEmailsEndpointusernameshould never changeemailRemoving this check revealed a new issue: A username equal to another user's primary email shadows that account at login:
find_users()resolves a username before falling back to email, so the original user logging in with their email matches the other account and fails. Username uniqueness was only enforced against other usernames, not emails, leaving this possible via the account-settings endpoint, the relocation claim form, and relocation import.Add
User.is_username_available(checks both username and primary email) and route all three surfaces through it:UserDetailsEndpointand the relocation claim form reject a conflicting username.