diff --git a/internal/auth/service_api.go b/internal/auth/service_api.go index a726d9bf..b61a1ad3 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 c81a0360..b066ddcc 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 156e3dcc..660f4ab4 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 5519cd76..49ddb2cc 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"`