Uh oh!
There was an error while loading. Please reload this page.
Fix an existing user being renamed when the admin form rebuilds its changeset - #671
Merged
Merged
Conversation
…hangeset Opening a user in the admin form and saving — toggling the password field is enough — renamed them: maria became maria_1, then maria_2. Two causes, both here. maybe_generate_username_from_email/1 runs on every registration changeset and generates a username whenever the params carry none, which is exactly what the admin form sends when it rebuilds the changeset for an already-saved user. And ensure_unique_username/2 asked the database whether the generated name was taken without excluding the user it was generating for, so maria's own row made maria look unavailable to maria. Generation is now skipped for a saved user that already has a username, and the uniqueness walk ignores the user's own row. An explicitly typed username still wins, a new user still gets one generated from their email, and a genuine collision with someone else's name still gets its suffix. Also removes seven Logger.info calls left in the admin user form from chasing this: they dumped the username at every step of validate/save/reload. Verified against the running application on real data: rebuilding the edit changeset with no username param now produces no username change (it produced fotkin_1 before). Regression tests added to user_org_changeset_test.exs — they are :integration and were not run here, this environment has no PostgreSQL for the core suite.
ddon pushed a commit
that referenced
this pull request
Jul 29, 2026
Impersonation (#672) documented "every attempt written to the activity feed before the session changes" and delivered neither half: the row was written only on success, only after the token had been minted, and alongside a second `session.account_added` row indistinguishable from a user adding an account of their own. Refusals — an Admin reaching for the Owner's account, an Admin reaching sideways, a non-staff user hitting the endpoint — left no trace at all, which is the entry a security review actually goes looking for. Refusals now write `session.impersonation_refused` with the deciding rule, carrying no target_uuid so they stay a feed entry rather than a message to the account they named; successes write one row, and it says impersonated. The controller resolved the uuid before checking authority, so its deliberately precise operator copy told every signed-in user which uuids were live accounts. `may_impersonate?/1` settles authority first and shares `staff?/1` with `authorize_impersonation/2` so the rules cannot drift. Both session actions reach the target's inbox via target_uuid but were claimed by no notification type and had no Render clause, so the customer's bell read "Session impersonated" and could not be switched off. Claimed by `security`, given recipient-facing copy. `already_intercepted` (#670) was independent of `skip_queue`, so a worker that set one and forgot the other handed its own dequeued job back to the queue. It can only mean "a worker is re-sending", so it now implies skip_queue. A generated username (#671) is put after `validate_username/2` has already run, so it reached the database with no uniqueness guard; two registrations racing on the same local part raised `Ecto.ConstraintError` out of `Repo.insert/1`. The constraint is re-attached to the generation branch. Reviews in dev_docs/pull_requests/2026/{670,671,672}-*/CLAUDE_REVIEW.md. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
This file contains hidden or 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
Sign up for freeto join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
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.
The bug
Open a user in the admin form, click "change password", save — and the user is renamed.
mariabecomesmaria_1; do it again and she becomesmaria_2.Why
Two things combine, both in
User:maybe_generate_username_from_email/1runs on everyregistration_changeset/3, and generates a username whenever the params carry none. That is right for a new user, but the admin form rebuilds this changeset for an already-saved user several times while editing —reload_changeset_with_password/2does it every time the password field is toggled — and those rebuilds send nousername.ensure_unique_username/2then asked the database whether the generated name was taken, without excluding the user it was generating for. Maria's own row mademarialook unavailable to Maria, so the walk moved on tomaria_1.The fix
An explicitly typed username still wins; a new user still gets one generated from their email; two different people whose emails share a local part still get
mariaandmaria_1.Also removes seven
Logger.infocalls left behind inuser_form.exfrom chasing this — they dumped the username at every step of validate / save / reload.Verification
Checked against a running host application on real data, building the same changesets the admin form builds (nothing saved):
usernameparamusernamechange tofotkin_1usernamechange, field staysfotkinusernameRegression tests added to
test/phoenix_kit/users/user_org_changeset_test.exs(saved user keeps its name; explicit name wins; new user still generated; a real collision still gets a suffix). They are:integration— the environment this was written in has no PostgreSQL for the core suite, so they were not executed here; please run them with the suite.