From 949c0c9e09f7c58ae6a0fb92064593dee03a93a1 Mon Sep 17 00:00:00 2001 From: Cristian Magherusan-Stanciu Date: Sat, 30 May 2026 23:46:05 +0200 Subject: [PATCH] fix(auth/users): persist email change in admin update path (closes #892) 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. --- internal/auth/service_api.go | 8 ++++ internal/auth/service_api_test.go | 74 +++++++++++++++++++++++++++++++ internal/auth/service_user.go | 10 +++++ internal/auth/types.go | 9 +++- 4 files changed, 100 insertions(+), 1 deletion(-) diff --git a/internal/auth/service_api.go b/internal/auth/service_api.go index a726d9bf4..b61a1ad30 100644 --- a/internal/auth/service_api.go +++ b/internal/auth/service_api.go @@ -231,6 +231,14 @@ func (s *Service) UpdateUserAPI(ctx context.Context, userID string, reqInterface if req.Role != "" { authReq.Role = &req.Role } + // Wire email through so admins can edit other users' email addresses. + // Before #892 this field was silently dropped: the API accepted email + // in the JSON, the handler returned 200, and the user's email column + // was never touched, producing a false-positive success toast and + // (worst case) a sign-in lockout once the user lost the old address. + if req.Email != "" { + authReq.Email = &req.Email + } user, err := s.UpdateUser(ctx, userID, authReq) if err != nil { return nil, err diff --git a/internal/auth/service_api_test.go b/internal/auth/service_api_test.go index c81a03608..b066ddcca 100644 --- a/internal/auth/service_api_test.go +++ b/internal/auth/service_api_test.go @@ -210,6 +210,80 @@ func TestService_UpdateUserAPI(t *testing.T) { mockStore.AssertExpectations(t) }) + // Regression for issue #892: before the fix, UpdateUserAPI dropped + // req.Email on the floor; the API would return 200, the success + // toast would fire, but the users.email column was never touched. We + // assert that (a) the email-uniqueness lookup happens (proves the + // admin path runs the same validation as self-edit), and (b) the + // user persisted via store.UpdateUser carries the new email value, + // NOT just that the call succeeded. + t.Run("successful email update persists to store", func(t *testing.T) { + mockStore := new(MockStore) + mockEmail := new(MockEmailSender) + service := createTestService(mockStore, mockEmail) + + existingUser := &User{ + ID: "user-123", + Email: "old@example.com", + Role: RoleAdmin, + } + + mockStore.On("GetUserByID", ctx, "user-123").Return(existingUser, nil).Once() + // Uniqueness check: new email not yet in use. + mockStore.On("GetUserByEmail", ctx, "new@example.com").Return(nil, nil).Once() + // Capture the User passed to UpdateUser and assert its email is + // the NEW one. Without the fix, this fails because the User would + // still carry old@example.com. + mockStore.On("UpdateUser", ctx, mock.MatchedBy(func(u *User) bool { + return u != nil && u.ID == "user-123" && u.Email == "new@example.com" + })).Return(nil).Once() + + req := APIUpdateUserRequest{ + Email: "new@example.com", + } + + result, err := service.UpdateUserAPI(ctx, "user-123", req) + require.NoError(t, err) + require.NotNil(t, result) + + apiUser, ok := result.(*APIUser) + require.True(t, ok) + assert.Equal(t, "new@example.com", apiUser.Email, + "returned APIUser must reflect the new email; the frontend's success toast keys off this response") + + mockStore.AssertExpectations(t) + }) + + t.Run("email update rejects duplicate address", func(t *testing.T) { + mockStore := new(MockStore) + mockEmail := new(MockEmailSender) + service := createTestService(mockStore, mockEmail) + + existingUser := &User{ + ID: "user-123", + Email: "old@example.com", + Role: RoleAdmin, + } + conflictingUser := &User{ + ID: "user-456", + Email: "taken@example.com", + } + + mockStore.On("GetUserByID", ctx, "user-123").Return(existingUser, nil).Once() + mockStore.On("GetUserByEmail", ctx, "taken@example.com").Return(conflictingUser, nil).Once() + + req := APIUpdateUserRequest{ + Email: "taken@example.com", + } + + result, err := service.UpdateUserAPI(ctx, "user-123", req) + assert.Error(t, err) + assert.Nil(t, result) + assert.Contains(t, err.Error(), "email already in use") + + mockStore.AssertExpectations(t) + }) + t.Run("invalid request type", func(t *testing.T) { mockStore := new(MockStore) mockEmail := new(MockEmailSender) diff --git a/internal/auth/service_user.go b/internal/auth/service_user.go index 156e3dcc2..660f4ab4e 100644 --- a/internal/auth/service_user.go +++ b/internal/auth/service_user.go @@ -290,6 +290,16 @@ func (s *Service) UpdateUser(ctx context.Context, userID string, req UpdateUserR return nil, err } + // Email is mutated through updateUserEmail rather than applyUpdateUserRequest + // because it requires a DB lookup (uniqueness check) and format validation + // (same rules as the self-edit profile path; see updateUserEmail and #868 + // for the TLD constraint). Issue #892. + if req.Email != nil { + if err := s.updateUserEmail(ctx, user, *req.Email); err != nil { + return nil, err + } + } + if err := s.store.UpdateUser(ctx, user); err != nil { return nil, fmt.Errorf("failed to update user: %w", err) } diff --git a/internal/auth/types.go b/internal/auth/types.go index 5519cd763..49ddb2cc5 100644 --- a/internal/auth/types.go +++ b/internal/auth/types.go @@ -235,8 +235,15 @@ type CreateUserRequest struct { GroupIDs []string `json:"group_ids,omitempty"` } -// UpdateUserRequest for updating user details +// UpdateUserRequest for updating user details. +// +// Email is a pointer so callers can distinguish "not sending email" (nil) +// from "explicitly setting email to a new value". This matters because the +// service layer applies email changes via updateUserEmail, which performs +// format validation and uniqueness checks that must NOT run on no-op +// updates that only touch role/groups/active. type UpdateUserRequest struct { + Email *string `json:"email,omitempty"` Role *string `json:"role,omitempty"` GroupIDs []string `json:"group_ids,omitempty"` Active *bool `json:"active,omitempty"`