fix(auth/users): persist email change in admin update path (closes #892)

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

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)

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)
 	}

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"`