haeaeae/silo-server
1
0
Fork 0
Code Issues Pull Requests Actions Packages Projects Releases Wiki Activity

fix(auth): prevent disabling or deleting the last enabled administrator

Browse Source 
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
This commit is contained in:
Claude Code
2026-06-11 01:23:34 +02:00
parent 43beb4693a
commit 20ed9ecd05
4 changed files with 141 additions and 33 deletions
Download Patch File Download Diff File Expand all files Collapse all files
internal/api/handlers/admin.go
+4
View File
@@ -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
}
internal/api/handlers/admin_test.go
+59
View File
@@ -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
internal/auth/group_repository.go
+49 -30
View File
@@ -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
}
}
internal/auth/repository.go
+29 -3
View File
@@ -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
}