diff --git a/internal/api/handlers/admin.go b/internal/api/handlers/admin.go index 909b28f2..8f5dd997 100644 --- a/internal/api/handlers/admin.go +++ b/internal/api/handlers/admin.go @@ -495,6 +495,10 @@ func (h *AdminHandler) HandleDeleteUser(w http.ResponseWriter, r *http.Request) err = h.userRepo.Delete(r.Context(), id) if err != nil { + if errors.Is(err, auth.ErrLastAdministrator) { + writeError(w, http.StatusConflict, "last_administrator", "Cannot remove the last enabled administrator") + return + } writeError(w, http.StatusInternalServerError, "internal_error", "Failed to delete user") return } diff --git a/internal/api/handlers/admin_test.go b/internal/api/handlers/admin_test.go index adc7e285..1e63b1fa 100644 --- a/internal/api/handlers/admin_test.go +++ b/internal/api/handlers/admin_test.go @@ -1,11 +1,70 @@ package handlers import ( + "context" + "net/http" "testing" + "github.com/go-chi/chi/v5" + + "github.com/Silo-Server/silo-server/internal/auth" "github.com/Silo-Server/silo-server/internal/models" ) +// fakeAdminUserRepo is a configurable in-memory UserRepository for handler +// tests. +type fakeAdminUserRepo struct { + user *models.User + updateErr error + deleteErr error +} + +func (f *fakeAdminUserRepo) List(context.Context) ([]*models.User, error) { return nil, nil } + +func (f *fakeAdminUserRepo) Create(context.Context, models.CreateUserInput) (*models.User, error) { + return f.user, nil +} + +func (f *fakeAdminUserRepo) Update(context.Context, int, models.UpdateUserInput) error { + return f.updateErr +} + +func (f *fakeAdminUserRepo) Delete(context.Context, int) error { return f.deleteErr } + +func (f *fakeAdminUserRepo) GetByID(context.Context, int) (*models.User, error) { + if f.user == nil { + return nil, auth.ErrNotFound + } + return f.user, nil +} + +// newAdminUsersTestRouter mounts the user-management routes the same way the +// API router does so tests exercise real chi URL params. +func newAdminUsersTestRouter(repo UserRepository) chi.Router { + h := NewAdminHandler(repo, nil, nil) + r := chi.NewRouter() + r.Put("/admin/users/{id}", h.HandleUpdateUser) + r.Delete("/admin/users/{id}", h.HandleDeleteUser) + return r +} + +func TestAdminUpdateUserDisableLastAdministrator(t *testing.T) { + repo := &fakeAdminUserRepo{ + user: &models.User{ID: 2, Enabled: true}, + updateErr: auth.ErrLastAdministrator, + } + rec := doGroupsRequest(t, newAdminUsersTestRouter(repo), http.MethodPut, + "/admin/users/2", map[string]any{"enabled": false}) + assertGroupsError(t, rec, http.StatusConflict, "last_administrator") +} + +func TestAdminDeleteUserLastAdministrator(t *testing.T) { + repo := &fakeAdminUserRepo{deleteErr: auth.ErrLastAdministrator} + rec := doGroupsRequest(t, newAdminUsersTestRouter(repo), http.MethodDelete, + "/admin/users/2", nil) + assertGroupsError(t, rec, http.StatusConflict, "last_administrator") +} + func TestUpdateRequiresSessionRevocation(t *testing.T) { enabled := true disabled := false diff --git a/internal/auth/group_repository.go b/internal/auth/group_repository.go index b3f24bf4..bb9e0b5f 100644 --- a/internal/auth/group_repository.go +++ b/internal/auth/group_repository.go @@ -476,6 +476,51 @@ func (r *GroupRepository) AddMember(ctx context.Context, groupID, userID int) er return tx.Commit(ctx) } +// lastEnabledAdministratorGuard returns ErrLastAdministrator when userID is an +// enabled member of the administrators group and no other enabled member +// exists, i.e. when removing, disabling, or deleting userID would leave the +// server without an enabled administrator. It locks the administrators group +// row (FOR UPDATE) so concurrent membership/disable/delete mutations +// serialize; re-locking the same row later in the same transaction is a no-op. +func lastEnabledAdministratorGuard(ctx context.Context, tx pgx.Tx, excludingUserID int) error { + var adminGroupID int + if err := tx.QueryRow(ctx, ` + SELECT id FROM groups WHERE slug = $1 FOR UPDATE`, + models.GroupSlugAdministrators).Scan(&adminGroupID); err != nil { + return fmt.Errorf("locking administrators group: %w", err) + } + + // Only an enabled administrator can be the last enabled administrator; + // mutating a non-member or disabled member never breaks the invariant. + var enabledMember bool + if err := tx.QueryRow(ctx, ` + SELECT EXISTS ( + SELECT 1 + FROM user_groups ug + JOIN users u ON u.id = ug.user_id + WHERE ug.group_id = $1 AND ug.user_id = $2 AND u.enabled)`, + adminGroupID, excludingUserID).Scan(&enabledMember); err != nil { + return fmt.Errorf("checking administrators membership: %w", err) + } + if !enabledMember { + return nil + } + + var remaining int + if err := tx.QueryRow(ctx, ` + SELECT COUNT(*) + FROM user_groups ug + JOIN users u ON u.id = ug.user_id + WHERE ug.group_id = $1 AND u.enabled AND ug.user_id <> $2`, + adminGroupID, excludingUserID).Scan(&remaining); err != nil { + return fmt.Errorf("counting administrators: %w", err) + } + if remaining == 0 { + return ErrLastAdministrator + } + return nil +} + // RemoveMember removes a user from a group. Removing the last enabled member // of the administrators group is rejected. func (r *GroupRepository) RemoveMember(ctx context.Context, groupID, userID int) error { @@ -491,17 +536,8 @@ func (r *GroupRepository) RemoveMember(ctx context.Context, groupID, userID int) return err } if group.Slug == models.GroupSlugAdministrators { - var remaining int - if err := tx.QueryRow(ctx, ` - SELECT COUNT(*) - FROM user_groups ug - JOIN users u ON u.id = ug.user_id - WHERE ug.group_id = $1 AND u.enabled AND ug.user_id <> $2`, - groupID, userID).Scan(&remaining); err != nil { - return fmt.Errorf("counting administrators: %w", err) - } - if remaining == 0 { - return ErrLastAdministrator + if err := lastEnabledAdministratorGuard(ctx, tx, userID); err != nil { + return err } } tag, err := tx.Exec(ctx, @@ -553,25 +589,8 @@ func (r *GroupRepository) replaceUserGroupsTx(ctx context.Context, tx pgx.Tx, us } } if !keepingAdmin { - var wasAdmin bool - if err := tx.QueryRow(ctx, ` - SELECT EXISTS (SELECT 1 FROM user_groups WHERE user_id = $1 AND group_id = $2)`, - userID, adminGroupID).Scan(&wasAdmin); err != nil { - return fmt.Errorf("checking administrators membership: %w", err) - } - if wasAdmin { - var remaining int - if err := tx.QueryRow(ctx, ` - SELECT COUNT(*) - FROM user_groups ug - JOIN users u ON u.id = ug.user_id - WHERE ug.group_id = $1 AND u.enabled AND ug.user_id <> $2`, - adminGroupID, userID).Scan(&remaining); err != nil { - return fmt.Errorf("counting administrators: %w", err) - } - if remaining == 0 { - return ErrLastAdministrator - } + if err := lastEnabledAdministratorGuard(ctx, tx, userID); err != nil { + return err } } diff --git a/internal/auth/repository.go b/internal/auth/repository.go index 0c509953..4b976b8d 100644 --- a/internal/auth/repository.go +++ b/internal/auth/repository.go @@ -247,6 +247,17 @@ func (r *UserRepository) Update(ctx context.Context, id int, input models.Update } defer func() { _ = tx.Rollback(ctx) }() + // Disabling the last enabled administrator would brick admin access just + // like removing their membership, so the same transactional guard applies. + // It runs before the identity UPDATE so the administrators group row is + // always locked before the user row (replaceUserGroupsTx re-locks the same + // group row later in this transaction, which is a no-op). + if input.Enabled != nil && !*input.Enabled { + if err := lastEnabledAdministratorGuard(ctx, tx, id); err != nil { + return err + } + } + if len(setClauses) == 0 { // Nothing to update on the users row; still verify the user exists. var exists int @@ -296,17 +307,32 @@ func (r *UserRepository) Update(ctx context.Context, id int, input models.Update return nil } -// Delete removes a user by their ID. +// Delete removes a user by their ID. Deleting the last enabled administrator +// is rejected with ErrLastAdministrator; the guard and the delete share one +// transaction so concurrent admin mutations serialize on the administrators +// group row. func (r *UserRepository) Delete(ctx context.Context, id int) error { - tag, err := r.pool.Exec(ctx, "DELETE FROM users WHERE id = $1", id) + tx, err := r.pool.Begin(ctx) + if err != nil { + return fmt.Errorf("beginning user delete: %w", err) + } + defer func() { _ = tx.Rollback(ctx) }() + + if err := lastEnabledAdministratorGuard(ctx, tx, id); err != nil { + return err + } + + tag, err := tx.Exec(ctx, "DELETE FROM users WHERE id = $1", id) if err != nil { return fmt.Errorf("deleting user: %w", err) } - if tag.RowsAffected() == 0 { return ErrNotFound } + if err := tx.Commit(ctx); err != nil { + return fmt.Errorf("committing user delete: %w", err) + } return nil }