fix(auth/users): persist email change in admin update path (closes #892)#893
Conversation
UpdateUserAPI accepted req.Email on the wire (APIUpdateUserRequest) but dropped it during conversion to the internal UpdateUserRequest, which had no Email field. The handler returned 200, the success toast fired, but users.email was never UPDATEd. An admin attempting to edit another user's address saw the UI confirm success while the user remained locked into the old address (sign-in lockout risk). Wire the field through: - types.go: add Email *string to UpdateUserRequest (pointer so callers can distinguish "not sending email" from "explicitly setting email", matching the existing Role pointer pattern). - service_api.go: copy req.Email into authReq.Email when non-empty. - service_user.go: in UpdateUser, route email changes through the existing s.updateUserEmail helper so admin edits get the same format/TLD validation and uniqueness check as the self-edit profile path. No new code path; reuses #868's helper. Regression test (TestService_UpdateUserAPI): - "successful email update persists to store" asserts the new email value reaches store.UpdateUser, not just that HTTP 200 returns. Fails on pre-fix code. - "email update rejects duplicate address" asserts the uniqueness check surfaces "email already in use" via mapAuthError -> 409.
|
Warning Review limit reached
More reviews will be available in 15 minutes and 40 seconds. Learn how PR review limits work. Your organization has run out of usage credits. Purchase more in the billing tab. ⌛ How to resolve this issue?After more reviews become available, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans include higher PR review limits than trial, open-source, and free plans. In all cases, reviews become available again over time. During sustained high-volume PR review activity, CodeRabbit may temporarily slow when the next review becomes available. Please see our Fair Usage Limits Policy for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (4)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
Summary
Fixes #892. Editing another user's email via the admin Users page reported success but never persisted the change — the new email was silently dropped on the service-layer boundary, so the user remained signed in (or locked out) with the old address.
Root cause
internal/auth/service_api.go:222-239.Service.UpdateUserAPIbuilt an internalUpdateUserRequestfrom the API payload but only copiedRoleandGroups.req.Emailwas read off the JSON (it's a real field onAPIUpdateUserRequest), then thrown away. DownstreamService.UpdateUserfetched the existing user and calledstore.UpdateUserwith the unchangeduser.Emailvalue, so the SQLUPDATE users SET email = $2 ...wrote the OLD email back. The handler returned 200, the frontend's success toast fired, the database row was unchanged.Why the toast lied
The frontend's
updateUser(frontend/src/api/users.ts:48) keys success off HTTP 200. The backend returned 200 with the (unchanged)APIUserJSON. Without a per-field comparison against the request, the frontend has no way to detect that the server silently ignored a field — and there's no good reason it should need to. The contract has to hold server-side.Why existing tests didn't catch it
TestService_UpdateUserAPI/successful user update(service_api_test.go:183) exercised onlyRoleandGroups. No prior test passedEmail:in anAPIUpdateUserRequest, so the silent drop was never observed.Fix shape
Minimal: wire the field through the layer that was dropping it. Reuses the existing
s.updateUserEmailhelper (already used by the self-edit profile path atservice_user.go:376), which performs format validation (TLD constraint per #868) and uniqueness check. No duplication, no new validation logic.internal/auth/types.go: addEmail *stringtoUpdateUserRequest(pointer matches theRole *stringpattern so callers can distinguish "not sending" from "sending empty").internal/auth/service_api.go: copyreq.EmailintoauthReq.Emailwhen non-empty.internal/auth/service_user.go: calls.updateUserEmailinUpdateUserwhenreq.Email != nil.mapAuthErroralready mapsErrInvalidEmail→ 400 andErrEmailInUse→ 409, so error surfacing is correct for free.Regression test
TestService_UpdateUserAPIgains two subtests:successful email update persists to store— assertion on theUsercaptured bystore.UpdateUser(must carry the NEW email), not on the HTTP status. Without the fix, this subtest fails because the capturedUserstill carries the old email.email update rejects duplicate address— asserts the uniqueness check surfacesErrEmailInUse(handler maps to 409).Stash-verified: both subtests fail against pre-fix code, pass with fix applied. The pre-existing role-only subtest is unchanged and still passes.
Test plan
gofmt -l internal/auth/(clean)go vet ./internal/auth/...(clean)go build ./...(clean)go test ./internal/auth/... ./internal/api/... -count=1(1854 pass)Scope discipline
Only the admin email-edit path is touched.
UpdateUserProfile(self-edit) is unchanged — already worked. No drive-by refactors. Other update paths (Active,Role) keep their existing semantics.closes #892