feat(system-admin): implement revocation of system admin privileges with safeguards

- Added RevokeSystemAdmin functionality to the user service and repository, ensuring atomic checks for self-revoke and last admin scenarios. - Updated the system handler to utilize the new revocation method, improving error handling for various edge cases. - Enhanced the bootstrap process to prevent unintended promotions when system admins already exist. - Refactored related comments and documentation for clarity on the new behavior and safeguards in place.
2026-06-04 13:30:32 +08:00 · 2026-05-24 21:53:15 +08:00
parent 47a183aa65
commit d074dc067a
8 changed files with 301 additions and 195 deletions
--- a/cmd/server/bootstrap.go
+++ b/cmd/server/bootstrap.go
@@ -18,17 +18,18 @@ import (
 )

 // bootstrapEnvVar is the env var that names the email of the user who
-// should be promoted to system administrator on every startup.
+// may be promoted to system administrator when the deployment has no
+// existing system administrators.
 //
 // Why an env var (vs a CLI subcommand)?
 //   - Zero-friction in docker-compose / k8s deploys: set it once in the
 //     manifest and the very first user account that signs up with that
 //     email is auto-promoted, with no extra ops step.
 //   - Idempotent: if the user is already a system admin, bootstrapping is
-//     a no-op (no DB write, no log noise beyond a debug line).
-//   - Safe to leave set: the operation only ever PROMOTES; it never
-//     demotes. So leaving the var in place across restarts won't undo
-//     manual revokes from the UI.
+//     a no-op.
+//   - Safe to leave set: once at least one system admin exists, the env
+//     var stops granting privileges. That prevents a UI revoke from being
+//     silently undone on the next restart.
 const bootstrapEnvVar = "WEKNORA_BOOTSTRAP_SYSTEM_ADMIN_EMAIL"

 // runStartupBootstrap consults the env and applies any one-shot
@@ -52,8 +53,9 @@ func runStartupBootstrap(c *dig.Container) {
 }

 // bootstrapSystemAdmin promotes the user identified by `email` to system
-// administrator if they exist and are not already one. The function is
-// idempotent and non-fatal — it warns and returns on every error path.
+// administrator only when the deployment currently has no system admins.
+// The function is idempotent and non-fatal — it warns and returns on
+// every error path.
 //
 // The bootstrap intentionally does NOT create a user when the email is
 // not yet registered: account creation is a workflow with side effects
@@ -83,6 +85,19 @@ func bootstrapSystemAdmin(ctx context.Context, userSvc interfaces.UserService, e
 			bootstrapEnvVar, email, user.ID)
 		return
 	}
+	_, total, err := userSvc.ListSystemAdmins(ctx, 0, 1)
+	if err != nil {
+		logger.Warnf(ctx,
+			"[bootstrap] %s=%s: cannot verify existing system admins, skipping promotion: %v",
+			bootstrapEnvVar, email, err)
+		return
+	}
+	if total > 0 {
+		logger.Infof(ctx,
+			"[bootstrap] %s=%s: %d system admin(s) already exist; not promoting user %s",
+			bootstrapEnvVar, email, total, user.ID)
+		return
+	}
 	user.IsSystemAdmin = true
 	if err := userSvc.UpdateUser(ctx, user); err != nil {
 		logger.Warnf(ctx,
--- a/internal/application/repository/user.go
+++ b/internal/application/repository/user.go
@@ -7,12 +7,16 @@ import (
 	"github.com/Tencent/WeKnora/internal/types"
 	"github.com/Tencent/WeKnora/internal/types/interfaces"
 	"gorm.io/gorm"
+	"gorm.io/gorm/clause"
 )

 var (
-	ErrUserNotFound      = errors.New("user not found")
-	ErrUserAlreadyExists = errors.New("user already exists")
-	ErrTokenNotFound     = errors.New("token not found")
+	ErrUserNotFound       = errors.New("user not found")
+	ErrUserAlreadyExists  = errors.New("user already exists")
+	ErrTokenNotFound      = errors.New("token not found")
+	ErrCannotRevokeSelf   = errors.New("cannot revoke your own system admin privileges")
+	ErrLastSystemAdmin    = errors.New("cannot revoke the last remaining system administrator")
+	ErrUserNotSystemAdmin = errors.New("user is not a system administrator")
 )

 // userRepository implements user repository interface
@@ -160,6 +164,64 @@ func (r *userRepository) ListSystemAdmins(ctx context.Context, offset, limit int
 	return users, total, nil
 }

+// RevokeSystemAdmin revokes system-admin privileges inside a transaction.
+// It locks the current admin rows before counting so concurrent revokes
+// cannot both observe "two admins" and leave the platform with zero.
+func (r *userRepository) RevokeSystemAdmin(ctx context.Context, userID, actorID string) (*types.User, error) {
+	if userID == actorID {
+		return nil, ErrCannotRevokeSelf
+	}
+
+	var revoked *types.User
+	err := r.db.WithContext(ctx).Transaction(func(tx *gorm.DB) error {
+		locking := func(db *gorm.DB) *gorm.DB {
+			switch tx.Dialector.Name() {
+			case "postgres", "mysql":
+				return db.Clauses(clause.Locking{Strength: "UPDATE"})
+			default:
+				return db
+			}
+		}
+		var user types.User
+		if err := locking(tx).
+			Where("id = ?", userID).
+			First(&user).Error; err != nil {
+			if errors.Is(err, gorm.ErrRecordNotFound) {
+				return ErrUserNotFound
+			}
+			return err
+		}
+		if !user.IsSystemAdmin {
+			revoked = &user
+			return ErrUserNotSystemAdmin
+		}
+
+		var admins []types.User
+		if err := locking(tx).
+			Where("is_system_admin = ?", true).
+			Find(&admins).Error; err != nil {
+			return err
+		}
+		if len(admins) <= 1 {
+			return ErrLastSystemAdmin
+		}
+
+		user.IsSystemAdmin = false
+		if err := tx.Save(&user).Error; err != nil {
+			return err
+		}
+		revoked = &user
+		return nil
+	})
+	if errors.Is(err, ErrUserNotSystemAdmin) {
+		return revoked, nil
+	}
+	if err != nil {
+		return nil, err
+	}
+	return revoked, nil
+}
+
 // SearchUsers searches users by username or email
 func (r *userRepository) SearchUsers(ctx context.Context, query string, limit int) ([]*types.User, error) {
 	var users []*types.User
--- a/internal/application/service/system_setting.go
+++ b/internal/application/service/system_setting.go
@@ -1,11 +1,13 @@
 package service

 import (
+	"bytes"
 	"context"
 	"encoding/json"
 	"errors"
 	"fmt"
 	"os"
+	"sort"
 	"strconv"
 	"strings"
 	"sync"
@@ -15,6 +17,7 @@ import (
 	"github.com/google/uuid"
 	"github.com/redis/go-redis/v9"

+	"github.com/Tencent/WeKnora/internal/config"
 	"github.com/Tencent/WeKnora/internal/logger"
 	"github.com/Tencent/WeKnora/internal/types"
 	"github.com/Tencent/WeKnora/internal/types/interfaces"
@@ -50,11 +53,11 @@ type changeMessage struct {
 // The registry serves as the **only** authority on which keys are legal
 // + what type they hold + what their ENV-fallback name is + what the
 // built-in default is. Adding a new tunable is a matter of:
-//   1. Adding an entry here.
-//   2. (Optional) adding a SQL seed row in a new migration so the UI
-//      shows the row even before any operator hits Update.
-//   3. Replacing existing os.Getenv() reads with calls into the
-//      service.
+//  1. Adding an entry here.
+//  2. (Optional) adding a SQL seed row in a new migration so the UI
+//     shows the row even before any operator hits Update.
+//  3. Replacing existing os.Getenv() reads with calls into the
+//     service.
 //
 // Update rejects any key not in this registry — so the UI cannot inject
 // arbitrary keys into the DB, even with an attacker-controlled body.
@@ -98,18 +101,18 @@ var registry = map[string]settingSpec{
 		Default: int64(50),
 	},
 	"ssrf.whitelist": {
-		Type:    "string_list",
-		EnvName: "SSRF_WHITELIST",
-		Default: []string{},
+		Type:     "string_list",
+		EnvName:  "SSRF_WHITELIST",
+		Default:  []string{},
 		Category: "security",
 		Description: "SSRF 防护白名单。可填入 example.com / *.foo.com / 10.0.0.0/8 / 2001:db8::1。" +
 			"修改后立即生效。SSRF_WHITELIST_EXTRA 环境变量仍由部署方维护，不在此处覆盖。",
 	},
 	"auth.registration_mode": {
-		Type:    "string",
-		EnvName: "", // No env fallback — handler passes cfg.Auth.RegistrationMode as default
-		Default: "self_serve",
-		Enum:    []string{"self_serve", "invite_only"},
+		Type:     "string",
+		EnvName:  "", // No env fallback — handler passes cfg.Auth.RegistrationMode as default
+		Default:  "self_serve",
+		Enum:     []string{"self_serve", "invite_only"},
 		Category: "auth",
 		Description: "自助注册模式。self_serve = 任何人可注册账号；invite_only = 关闭公网注册，" +
 			"仅 Owner/Admin 可邀请。修改后立即生效，但谨慎对待 self_serve（公网会接受 spam）。",
@@ -140,6 +143,7 @@ type systemSettingService struct {
 	repo  interfaces.SystemSettingRepository
 	audit interfaces.AuditLogService
 	rdb   *redis.Client // may be nil in lite mode
+	cfg   *config.Config

 	// instanceID disambiguates this replica from its peers in the
 	// pubsub stream. Generated once at construction; never changes.
@@ -166,17 +170,19 @@ type systemSettingService struct {
 // NewSystemSettingService is the dig provider. audit may be nil
 // (matches the tenantMemberService convention — tests that don't care
 // about audit can pass nil and emitAudit no-ops). rdb may also be nil
-// when REDIS_ADDR is unset — the service degrades gracefully to the
-// P1 "no cache, every read hits DB" path.
+// when REDIS_ADDR is unset — the service still uses its local cache,
+// but skips cross-replica pubsub invalidation.
 func NewSystemSettingService(
 	repo interfaces.SystemSettingRepository,
 	audit interfaces.AuditLogService,
 	rdb *redis.Client,
+	cfg *config.Config,
 ) interfaces.SystemSettingService {
 	s := &systemSettingService{
 		repo:       repo,
 		audit:      audit,
 		rdb:        rdb,
+		cfg:        cfg,
 		instanceID: uuid.NewString(),
 		cache:      make(map[string]*types.SystemSetting),
 	}
@@ -205,14 +211,6 @@ func (s *systemSettingService) preload(ctx context.Context) {
 	}
 	s.mu.Unlock()

-	// Backfill: any registry key that doesn't yet have a DB row gets
-	// inserted now with its built-in default. This makes the in-code
-	// `registry` map the single source of truth — adding a new tunable
-	// is a code change, no migration required, and the management UI
-	// surfaces it the next time the server boots. Idempotent: existing
-	// rows are never touched (Upsert would, but we skip when present).
-	s.seedMissingFromRegistry(ctx)
-
 	s.loaded.Store(true)
 	s.mu.RLock()
 	loadedCount := len(s.cache)
@@ -226,65 +224,6 @@ func (s *systemSettingService) preload(ctx context.Context) {
 	s.applySSRFWhitelist(ctx)
 }

-// seedMissingFromRegistry inserts a default row for every registry key
-// that doesn't already exist in the DB. Called from preload after the
-// initial List, so that:
-//
-//   - New deployments (empty table) get every key seeded automatically.
-//   - Existing deployments where a new key was added in code (without a
-//     migration) automatically pick it up on the next server start.
-//   - Hand-deleted rows are restored on next start (mild self-healing).
-//
-// Critically, this DOES NOT touch existing rows — operator edits via UI
-// are preserved. Errors per-key are logged but never block other keys
-// or fail the boot. The s.cache mutation runs under the write lock so a
-// reader landing in the middle of seeding still sees a consistent view.
-func (s *systemSettingService) seedMissingFromRegistry(ctx context.Context) {
-	for key, spec := range registry {
-		s.mu.RLock()
-		_, exists := s.cache[key]
-		s.mu.RUnlock()
-		if exists {
-			continue
-		}
-		encoded, err := encodeDefault(spec)
-		if err != nil {
-			logger.Warnf(ctx, "[system_settings] cannot encode default for %q: %v", key, err)
-			continue
-		}
-		category := spec.Category
-		if category == "" {
-			category = "general"
-		}
-		row := &types.SystemSetting{
-			Key:             key,
-			Value:           encoded,
-			ValueType:       spec.Type,
-			Category:        category,
-			Description:     spec.Description,
-			IsSecret:        false, // P3+ may flip via spec; today every seed row is non-secret
-			RequiresRestart: false,
-			LastModifiedBy:  "", // empty = "seeded by system"
-		}
-		if err := s.repo.Upsert(ctx, row); err != nil {
-			logger.Warnf(ctx, "[system_settings] seed %q failed: %v", key, err)
-			continue
-		}
-		// Read back so we see DB-assigned id / timestamps (and so the
-		// cache entry round-trips through the same JSON shape as a
-		// hand-edited row would).
-		persisted, err := s.repo.Get(ctx, key)
-		if err != nil || persisted == nil {
-			persisted = row
-		}
-		s.mu.Lock()
-		s.cache[key] = persisted
-		s.mu.Unlock()
-		logger.Infof(ctx, "[system_settings] seeded missing key %q (type=%s, category=%s)",
-			key, spec.Type, category)
-	}
-}
-
 // encodeDefault produces the JSONB encoding for a spec's built-in
 // default. Mirrors encodeForType but operates on already-typed Go
 // values from registry so we never have to round-trip through `any`
@@ -450,11 +389,15 @@ func (s *systemSettingService) publishChange(ctx context.Context, key string) {
 // instead of a 500. This is the deliberate degradation policy spelled
 // out in the interface comment.
 func (s *systemSettingService) resolveRaw(ctx context.Context, key string) (raw types.JSON, fromDB bool) {
+	spec, known := registry[key]
 	if s.loaded.Load() {
 		s.mu.RLock()
 		row, ok := s.cache[key]
 		s.mu.RUnlock()
 		if ok && row != nil {
+			if known && isBootstrapDefaultRow(row, spec) {
+				return nil, false
+			}
 			return row.Value, true
 		}
 		// Cache populated and key not present → authoritative miss.
@@ -470,6 +413,9 @@ func (s *systemSettingService) resolveRaw(ctx context.Context, key string) (raw
 	if row == nil {
 		return nil, false
 	}
+	if known && isBootstrapDefaultRow(row, spec) {
+		return nil, false
+	}
 	return row.Value, true
 }

@@ -579,21 +525,53 @@ func (s *systemSettingService) GetStringList(ctx context.Context, key string, en
 	return def
 }

-// List returns all rows for the management UI. Pass-through to repo,
-// then enriched with the in-code registry's `Enum` so the UI can render
-// a select. Rows whose key isn't in the registry (out-of-band hand-edits)
-// pass through untouched — UI will fall back to a free-form input.
+// List returns all known settings for the management UI. Persisted rows
+// are enriched with registry metadata. Registry keys without a saved DB
+// override are returned as virtual rows using the effective fallback
+// value (ENV/config/default), so merely migrating the schema never
+// changes runtime behaviour.
 func (s *systemSettingService) List(ctx context.Context) ([]*types.SystemSetting, error) {
 	rows, err := s.repo.List(ctx)
 	if err != nil {
 		return nil, err
 	}
-	for _, r := range rows {
-		if spec, ok := registry[r.Key]; ok {
-			r.Enum = spec.Enum
-		}
+	byKey := make(map[string]*types.SystemSetting, len(rows))
+	out := make([]*types.SystemSetting, 0, len(rows)+len(registry))
+	for _, row := range rows {
+		byKey[row.Key] = row
 	}
-	return rows, nil
+
+	keys := make([]string, 0, len(registry))
+	for key := range registry {
+		keys = append(keys, key)
+	}
+	sort.Strings(keys)
+
+	for _, key := range keys {
+		spec := registry[key]
+		if row := byKey[key]; row != nil {
+			row.Enum = spec.Enum
+			if isBootstrapDefaultRow(row, spec) {
+				row.Value = s.fallbackJSONForSpec(key, spec)
+			}
+			out = append(out, row)
+			delete(byKey, key)
+			continue
+		}
+		out = append(out, s.virtualSetting(key, spec))
+	}
+
+	// Preserve out-of-band rows so operators can still see unexpected
+	// data instead of having it disappear from the UI.
+	extraKeys := make([]string, 0, len(byKey))
+	for key := range byKey {
+		extraKeys = append(extraKeys, key)
+	}
+	sort.Strings(extraKeys)
+	for _, key := range extraKeys {
+		out = append(out, byKey[key])
+	}
+	return out, nil
 }

 // Get returns one row by key. Used by the management UI's "load before
@@ -613,8 +591,109 @@ func (s *systemSettingService) Get(ctx context.Context, key string) (*types.Syst
 	}
 	if row != nil {
 		row.Enum = spec.Enum
+		if isBootstrapDefaultRow(row, spec) {
+			row.Value = s.fallbackJSONForSpec(key, spec)
+		}
+		return row, nil
 	}
-	return row, nil
+	return s.virtualSetting(key, spec), nil
+}
+
+func (s *systemSettingService) virtualSetting(key string, spec settingSpec) *types.SystemSetting {
+	category := spec.Category
+	if category == "" {
+		category = "general"
+	}
+	return &types.SystemSetting{
+		Key:             key,
+		Value:           s.fallbackJSONForSpec(key, spec),
+		ValueType:       spec.Type,
+		Category:        category,
+		Description:     spec.Description,
+		IsSecret:        false,
+		RequiresRestart: false,
+		LastModifiedBy:  "",
+		Enum:            spec.Enum,
+	}
+}
+
+func (s *systemSettingService) fallbackJSONForSpec(key string, spec settingSpec) types.JSON {
+	if spec.EnvName != "" {
+		if raw := strings.TrimSpace(os.Getenv(spec.EnvName)); raw != "" {
+			switch spec.Type {
+			case "int":
+				if n, err := strconv.ParseInt(raw, 10, 64); err == nil {
+					if encoded, err := encodeForType(spec.Type, n); err == nil {
+						return encoded
+					}
+				}
+			case "string":
+				if encoded, err := encodeForType(spec.Type, raw); err == nil {
+					return encoded
+				}
+			case "bool":
+				if b, err := strconv.ParseBool(raw); err == nil {
+					if encoded, err := encodeForType(spec.Type, b); err == nil {
+						return encoded
+					}
+				}
+			case "string_list":
+				entries := make([]string, 0, 4)
+				for _, entry := range strings.Split(raw, ",") {
+					entry = strings.TrimSpace(entry)
+					if entry != "" {
+						entries = append(entries, entry)
+					}
+				}
+				if encoded, err := encodeForType(spec.Type, entries); err == nil {
+					return encoded
+				}
+			}
+		}
+	}
+	if key == "auth.registration_mode" {
+		mode := config.AuthRegistrationModeSelfServe
+		if s.cfg != nil && s.cfg.Auth != nil {
+			if configured := strings.TrimSpace(s.cfg.Auth.RegistrationMode); configured != "" {
+				mode = configured
+			}
+		}
+		if encoded, err := encodeForType(spec.Type, mode); err == nil {
+			return encoded
+		}
+	}
+	encoded, err := encodeDefault(spec)
+	if err != nil {
+		return types.JSON(`null`)
+	}
+	return encoded
+}
+
+// isBootstrapDefaultRow treats old migration/service seeded defaults as
+// placeholders rather than operator-owned overrides. Those rows used an
+// empty last_modified_by and the registry default value, so deployments
+// that already ran the unsafe seed regain the intended ENV/config
+// fallback behaviour until a SystemAdmin explicitly saves a value.
+func isBootstrapDefaultRow(row *types.SystemSetting, spec settingSpec) bool {
+	if row == nil || strings.TrimSpace(row.LastModifiedBy) != "" {
+		return false
+	}
+	def, err := encodeDefault(spec)
+	if err != nil {
+		return false
+	}
+	return jsonEqual(row.Value, def)
+}
+
+func jsonEqual(a, b types.JSON) bool {
+	var ca, cb bytes.Buffer
+	if err := json.Compact(&ca, a); err != nil {
+		return false
+	}
+	if err := json.Compact(&cb, b); err != nil {
+		return false
+	}
+	return bytes.Equal(ca.Bytes(), cb.Bytes())
 }

 // Update validates and persists a new value. Steps:
--- a/internal/application/service/user.go
+++ b/internal/application/service/user.go
@@ -510,6 +510,13 @@ func (s *userService) ListSystemAdmins(
 	return s.userRepo.ListSystemAdmins(ctx, offset, limit)
 }

+// RevokeSystemAdmin removes system-admin privileges through the
+// repository's transactional guard so concurrent revokes cannot remove
+// the final administrator.
+func (s *userService) RevokeSystemAdmin(ctx context.Context, userID, actorID string) (*types.User, error) {
+	return s.userRepo.RevokeSystemAdmin(ctx, userID, actorID)
+}
+
 // UpdateUserPreferences applies a partial update over the user's
 // preferences blob. PATCH semantics: only keys present in `patch`
 // (non-nil pointer fields) replace the existing value; everything else
--- a/internal/handler/system.go
+++ b/internal/handler/system.go
@@ -2,6 +2,7 @@ package handler

 import (
 	"context"
+	"errors"
 	"fmt"
 	"net"
 	"net/http"
@@ -10,6 +11,7 @@ import (
 	"strconv"
 	"strings"

+	"github.com/Tencent/WeKnora/internal/application/repository"
 	"github.com/Tencent/WeKnora/internal/application/service"
 	"github.com/Tencent/WeKnora/internal/application/service/file"
 	"github.com/Tencent/WeKnora/internal/config"
@@ -1099,54 +1101,28 @@ func (h *SystemHandler) RevokeSystemAdmin(c *gin.Context) {
 		return
 	}

-	// Self-revoke guard. Without it, a single careless click could leave a
-	// deployment with zero system admins and no UI path to recover —
-	// operators would have to set the env-var bootstrap or hand-edit the DB.
-	if callerID, _ := types.UserIDFromContext(ctx); callerID == req.UserID {
-		c.JSON(http.StatusBadRequest, gin.H{
-			"error": "Cannot revoke your own system admin privileges",
-		})
-		return
-	}
-
-	user, err := h.userSvc.GetUserByID(ctx, req.UserID)
+	callerID, _ := types.UserIDFromContext(ctx)
+	user, err := h.userSvc.RevokeSystemAdmin(ctx, req.UserID, callerID)
 	if err != nil {
-		logger.Errorf(ctx, "Error fetching user %s: %v", req.UserID, err)
-		c.JSON(http.StatusNotFound, gin.H{"error": "User not found"})
+		switch {
+		case errors.Is(err, repository.ErrCannotRevokeSelf):
+			c.JSON(http.StatusBadRequest, gin.H{
+				"error": "Cannot revoke your own system admin privileges",
+			})
+		case errors.Is(err, repository.ErrLastSystemAdmin):
+			c.JSON(http.StatusBadRequest, gin.H{
+				"error": "Cannot revoke the last remaining system administrator",
+			})
+		case errors.Is(err, repository.ErrUserNotFound):
+			c.JSON(http.StatusNotFound, gin.H{"error": "User not found"})
+		default:
+			logger.Errorf(ctx, "Error revoking system admin from user %s: %v", req.UserID, err)
+			c.JSON(http.StatusInternalServerError, gin.H{"error": "Failed to revoke system admin privileges"})
+		}
 		return
 	}
 	if user == nil {
-		c.JSON(http.StatusNotFound, gin.H{"error": "User not found"})
-		return
-	}
-
-	if !user.IsSystemAdmin {
-		// Idempotent: revoking from a non-admin is a no-op success.
-		c.JSON(http.StatusOK, user.ToUserInfo())
-		return
-	}
-
-	// Last-admin guard. ListSystemAdmins is bounded to a single row here
-	// because we only need the total count; this stays O(1) on the
-	// is_system_admin index. Combined with the self-revoke guard above,
-	// these two checks make it impossible for a SystemAdmin to lock
-	// themselves out of system-level administration via this endpoint.
-	_, total, err := h.userSvc.ListSystemAdmins(ctx, 0, 1)
-	if err != nil {
-		logger.Errorf(ctx, "Error counting system admins for last-admin check: %v", err)
-		c.JSON(http.StatusInternalServerError, gin.H{"error": "Failed to verify admin count"})
-		return
-	}
-	if total <= 1 {
-		c.JSON(http.StatusBadRequest, gin.H{
-			"error": "Cannot revoke the last remaining system administrator",
-		})
-		return
-	}
-
-	user.IsSystemAdmin = false
-	if err := h.userSvc.UpdateUser(ctx, user); err != nil {
-		logger.Errorf(ctx, "Error revoking system admin from user %s: %v", req.UserID, err)
+		logger.Errorf(ctx, "RevokeSystemAdmin returned nil user for %s", req.UserID)
 		c.JSON(http.StatusInternalServerError, gin.H{"error": "Failed to revoke system admin privileges"})
 		return
 	}
--- a/internal/middleware/rbac.go
+++ b/internal/middleware/rbac.go
@@ -113,13 +113,10 @@ func RequireRole(min types.TenantRole, cfg *config.Config) gin.HandlerFunc {
 // editing global settings, cross-tenant operations) where the per-tenant
 // Owner/Admin/Contributor/Viewer ladder does not apply.
 //
-// When cfg.Tenant.EnableRBAC is false, the middleware logs the would-be
-// rejection but lets the request through — preserving backward
-// compatibility during rollout. Once operators flip the flag to true,
-// the same code paths start rejecting unauthorised callers. SystemAdmin
-// rides on the same RBAC kill-switch deliberately: ops should be able to
-// disable BOTH per-tenant RBAC and system-admin gating during an
-// emergency without juggling two independent flags.
+// Unlike tenant-role guards, this check is always enforced. The
+// tenant RBAC rollout switch only controls per-tenant Owner/Admin/etc.
+// checks; it must not turn platform-wide administration endpoints into
+// "any authenticated user can call this" endpoints.
 func RequireSystemAdmin(cfg *config.Config) gin.HandlerFunc {
 	warnOnNilConfig(cfg)
 	return func(c *gin.Context) {
@@ -129,13 +126,6 @@ func RequireSystemAdmin(cfg *config.Config) gin.HandlerFunc {
 			return
 		}
 		uid, _ := types.UserIDFromContext(ctx)
-		if !rbacEnforcementEnabled(cfg) {
-			logger.Warnf(ctx,
-				"[rbac] system admin required (logged but not enforced): user=%s path=%s",
-				uid, c.Request.URL.Path)
-			c.Next()
-			return
-		}
 		logger.Warnf(ctx,
 			"[rbac] system admin required: user=%s path=%s",
 			uid, c.Request.URL.Path)
--- a/internal/types/interfaces/user.go
+++ b/internal/types/interfaces/user.go
@@ -70,6 +70,9 @@ type UserService interface {
 	// callers pass offset/limit to page through results. Used by the
 	// /api/v1/system/admin/list endpoint, gated to SystemAdmin callers.
 	ListSystemAdmins(ctx context.Context, offset, limit int) ([]*types.User, int64, error)
+	// RevokeSystemAdmin removes system-admin privileges with the
+	// last-admin/self-revoke checks performed atomically.
+	RevokeSystemAdmin(ctx context.Context, userID, actorID string) (*types.User, error)
 	// UpdateUserPreferences partially updates the calling user's
 	// preferences blob (PATCH semantics: only keys present in `patch`
 	// overwrite existing values). Returns the updated, persisted prefs.
@@ -102,6 +105,9 @@ type UserRepository interface {
 	// the slice plus the total count for pagination metadata. Used by
 	// the system-admin management endpoint.
 	ListSystemAdmins(ctx context.Context, offset, limit int) ([]*types.User, int64, error)
+	// RevokeSystemAdmin removes system-admin privileges with the
+	// last-admin/self-revoke checks performed atomically.
+	RevokeSystemAdmin(ctx context.Context, userID, actorID string) (*types.User, error)
 	// SearchUsers searches users by username or email
 	SearchUsers(ctx context.Context, query string, limit int) ([]*types.User, error)
 }
--- a/migrations/versioned/000053_system_settings.up.sql
+++ b/migrations/versioned/000053_system_settings.up.sql
@@ -1,15 +1,13 @@
 -- Migration: 000053_system_settings
 -- Adds a system-scoped (NOT tenant-scoped) settings table for platform-wide
-- runtime tunables, gated by SystemAdmin in P1.
+-- runtime tunables, gated by SystemAdmin.
 --
-- Scope:
--   - P1 ships the schema, the 3-tier resolver (DB > ENV > built-in default),
--     and a single seeded key (file.max_size_mb) as a worked example. Adding
--     more keys is purely a service-layer registry change — no further
--     migrations needed.
--   - is_secret / requires_restart columns exist now but are wired to
--     `false` for every P1 row. P3 turns them into real semantics
--     (mask + reveal flow / "needs restart" UI badge).
+-- Deliberately do not seed values here. For migrated deployments, a DB row
+-- has higher precedence than ENV, so inserting built-in defaults would
+-- silently override existing operator configuration such as
+-- DISABLE_REGISTRATION, SSRF_WHITELIST, and MAX_FILE_SIZE_MB. The service
+-- exposes registry-backed virtual rows to the management UI until an admin
+-- explicitly saves a value.
 --
 -- Why JSONB for `value`?
 --   We want to support int / string / bool / arrays / objects under one
@@ -40,31 +38,4 @@ CREATE TABLE IF NOT EXISTS system_settings (
 CREATE INDEX IF NOT EXISTS idx_system_settings_category
    ON system_settings (category);

-- Seed the P1 / P3 worked examples. ON CONFLICT DO NOTHING so re-running
-- the migration on an instance where the operator already tweaked the
-- value via UI doesn't reset it.
--
-- Categories drive the management UI grouping:
--   limits   = quota / size knobs
--   security = SSRF whitelist, future ACLs
--   auth     = registration mode etc.
-INSERT INTO system_settings (key, value, value_type, category, description)
-VALUES
-    ('file.max_size_mb',
-     '50',
-     'int',
-     'limits',
-     '上传文件大小上限（MB）。修改后立即对下次上传生效，无需重启服务。'),
-    ('ssrf.whitelist',
-     '[]',
-     'string_list',
-     'security',
-     'SSRF 防护白名单。可填入 example.com / *.foo.com / 10.0.0.0/8 / 2001:db8::1。修改后立即生效。SSRF_WHITELIST_EXTRA 环境变量仍由部署方维护，不在此处覆盖。'),
-    ('auth.registration_mode',
-     '"self_serve"',
-     'string',
-     'auth',
-     '自助注册模式。self_serve = 任何人可注册账号；invite_only = 关闭公网注册，仅 Owner/Admin 可邀请。修改后立即生效。')
-ON CONFLICT (key) DO NOTHING;
-
 DO $$ BEGIN RAISE NOTICE '[Migration 000053] system_settings table ready'; END $$;