fix(admin): route admin-panel bans through ModerationService (W1-4)

requireBanAuthority (BAN_MEMBERS + role hierarchy) was wired only into
ModerationService.BanUser/UnbanUser — which had zero production callers.
The live path, handlePatchUser, ran a raw UPDATE with no hierarchy check,
so any admin-panel actor could ban an equal- or higher-ranked user,
including the owner. The ban/unban branch now calls the service (dead code
becomes THE code — ban path 1 of 3 consolidated), which also audits as
user_ban/user_unban, keeping the historical audit vocabulary.

Authorization now runs in permission → existence → hierarchy order: an
actor without ban authority sees Forbidden, never NotFound, so the ban
path cannot enumerate user ids. The role+ban transaction is gone — the
ban leg lives in the service, runs first, and a refusal returns before
the role change executes, so a rejected ban never half-applies a PATCH.
MemStore gains honest BanUser/UnbanUser so the matrix is testable.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>

Server/admin/admin.go

@@ -9,6 +9,7 @@ import (
 
 	"github.com/go-chi/chi/v5"
 	"github.com/owncord/server/db"
+	"github.com/owncord/server/service"
 	"github.com/owncord/server/updater"
 )
 
@@ -22,11 +23,11 @@ var staticFiles embed.FS
 //
 //	/api/*  — admin REST API (all require ADMINISTRATOR permission)
 //	/*      — embedded static files (SPA; index.html for unknown paths)
-func NewHandler(database *db.DB, version string, hub HubBroadcaster, u *updater.Updater, logBuf *RingBuffer, allowedOrigins []string, permInvalidator PermissionInvalidator) http.Handler {
+func NewHandler(database *db.DB, version string, hub HubBroadcaster, u *updater.Updater, logBuf *RingBuffer, allowedOrigins []string, permInvalidator PermissionInvalidator, mod *service.ModerationService) http.Handler {
 	r := chi.NewRouter()
 
 	// Admin REST API mounted at /api
-	r.Mount("/api", NewAdminAPI(database, version, hub, u, logBuf, allowedOrigins, permInvalidator))
+	r.Mount("/api", NewAdminAPI(database, version, hub, u, logBuf, allowedOrigins, permInvalidator, mod))
 
 	// Static files — serve from the "static" sub-tree of the embedded FS.
 	// The //go:embed static directive in this package embeds as "static/…",

Server/admin/api.go

@@ -6,6 +6,7 @@ import (
 	"github.com/go-chi/chi/v5"
 	"github.com/owncord/server/auth"
 	"github.com/owncord/server/db"
+	"github.com/owncord/server/service"
 	"github.com/owncord/server/updater"
 )
 
@@ -14,7 +15,7 @@ import (
 // NewAdminAPI returns a chi router with all /admin/api/* routes. All routes
 // are protected by adminAuthMiddleware which requires the ADMINISTRATOR bit,
 // except for the setup endpoints which are unauthenticated.
-func NewAdminAPI(database *db.DB, version string, hub HubBroadcaster, u *updater.Updater, logBuf *RingBuffer, allowedOrigins []string, permInvalidator PermissionInvalidator) http.Handler {
+func NewAdminAPI(database *db.DB, version string, hub HubBroadcaster, u *updater.Updater, logBuf *RingBuffer, allowedOrigins []string, permInvalidator PermissionInvalidator, mod *service.ModerationService) http.Handler {
 	r := chi.NewRouter()
 
 	// Setup endpoints — unauthenticated, only functional when no users exist.
@@ -39,7 +40,7 @@ func NewAdminAPI(database *db.DB, version string, hub HubBroadcaster, u *updater
 
 		r.Get("/stats", handleGetStats(database, hub))
 		r.Get("/users", handleListUsers(database))
-		r.Patch("/users/{id}", handlePatchUser(database, hub, permInvalidator))
+		r.Patch("/users/{id}", handlePatchUser(database, hub, permInvalidator, mod))
 		r.Delete("/users/{id}/sessions", handleForceLogout(database))
 		r.Get("/channels", handleListChannels(database))
 		r.Post("/channels", handleCreateChannel(database, hub))

Server/admin/handlers_users.go

@@ -2,11 +2,13 @@ package admin
 
 import (
 	"encoding/json"
+	"errors"
 	"fmt"
 	"log/slog"
 	"net/http"
 
 	"github.com/owncord/server/db"
+	"github.com/owncord/server/service"
 )
 
 // ─── User Handlers ───────────────────────────────────────────────────────────
@@ -51,7 +53,21 @@ type patchUserRequest struct {
 	BanReason *string `json:"ban_reason"`
 }
 
-func handlePatchUser(database *db.DB, hub HubBroadcaster, permInvalidator PermissionInvalidator) http.HandlerFunc {
+// writeModerationErr maps ModerationService errors onto admin API responses.
+func writeModerationErr(w http.ResponseWriter, err error) {
+	switch {
+	case errors.Is(err, service.ErrForbidden):
+		writeErr(w, http.StatusForbidden, "FORBIDDEN", err.Error())
+	case errors.Is(err, service.ErrNotFound):
+		writeErr(w, http.StatusNotFound, "NOT_FOUND", "user not found")
+	case errors.Is(err, service.ErrBadRequest):
+		writeErr(w, http.StatusBadRequest, "BAD_REQUEST", err.Error())
+	default:
+		writeErr(w, http.StatusInternalServerError, "INTERNAL_ERROR", "moderation action failed")
+	}
+}
+
+func handlePatchUser(database *db.DB, hub HubBroadcaster, permInvalidator PermissionInvalidator, mod *service.ModerationService) http.HandlerFunc {
 	return func(w http.ResponseWriter, r *http.Request) {
 		id, err := pathInt64(r, "id")
 		if err != nil {
@@ -84,21 +100,38 @@ func handlePatchUser(database *db.DB, hub HubBroadcaster, permInvalidator Permis
 			return
 		}
 
-		// Wrap role + ban updates in a transaction so both succeed or fail atomically.
-		tx, txErr := database.Begin()
-		if txErr != nil {
-			writeErr(w, http.StatusInternalServerError, "INTERNAL_ERROR", "failed to begin transaction")
-			return
-		}
-		committed := false
-		defer func() {
-			if !committed {
-				_ = tx.Rollback()
+		// Ban/unban first: it routes through ModerationService, which enforces
+		// BAN_MEMBERS + role hierarchy (the admin-auth perimeter alone does
+		// not — any admin-panel actor could previously ban the owner). The
+		// service also audits and refuses before the role change runs, so a
+		// rejected ban never leaves a half-applied PATCH behind.
+		if req.Banned != nil {
+			if mod == nil {
+				// Fail closed rather than fall back to an unchecked UPDATE.
+				writeErr(w, http.StatusInternalServerError, "INTERNAL_ERROR", "moderation service unavailable")
+				return
 			}
-		}()
+			banReason := ""
+			if req.BanReason != nil {
+				banReason = *req.BanReason
+			}
+			var actionErr error
+			if *req.Banned {
+				actionErr = mod.BanUser(actor, id, banReason, nil)
+			} else {
+				actionErr = mod.UnbanUser(actor, id)
+			}
+			if actionErr != nil {
+				writeModerationErr(w, actionErr)
+				return
+			}
+			if *req.Banned && hub != nil {
+				hub.BroadcastMemberBan(id)
+			}
+		}
 
 		if req.RoleID != nil {
-			if _, err := tx.Exec(`UPDATE users SET role_id = ? WHERE id = ?`, *req.RoleID, id); err != nil {
+			if _, err := database.Exec(`UPDATE users SET role_id = ? WHERE id = ?`, *req.RoleID, id); err != nil {
 				writeErr(w, http.StatusInternalServerError, "INTERNAL_ERROR", "failed to update role")
 				return
 			}
@@ -106,45 +139,6 @@ func handlePatchUser(database *db.DB, hub HubBroadcaster, permInvalidator Permis
 			if permInvalidator != nil {
 				permInvalidator.InvalidateUser(id)
 			}
-		}
-
-		banReason := ""
-		if req.Banned != nil {
-			if req.BanReason != nil {
-				banReason = *req.BanReason
-			}
-			if *req.Banned {
-				var expiresStr *string
-				if _, err := tx.Exec(
-					`UPDATE users SET banned = 1, ban_reason = ?, ban_expires = ? WHERE id = ?`,
-					banReason, expiresStr, id,
-				); err != nil {
-					writeErr(w, http.StatusInternalServerError, "INTERNAL_ERROR", "failed to ban user")
-					return
-				}
-				slog.Warn("user banned", "actor_id", actor, "target_user", user.Username, "reason", banReason)
-			} else {
-				if _, err := tx.Exec(
-					`UPDATE users SET banned = 0, ban_reason = NULL, ban_expires = NULL WHERE id = ?`,
-					id,
-				); err != nil {
-					writeErr(w, http.StatusInternalServerError, "INTERNAL_ERROR", "failed to unban user")
-					return
-				}
-				slog.Info("user unbanned", "actor_id", actor, "target_user", user.Username)
-			}
-		}
-
-		if err := tx.Commit(); err != nil {
-			writeErr(w, http.StatusInternalServerError, "INTERNAL_ERROR", "failed to commit user update")
-			return
-		}
-		committed = true
-
-		// Post-commit side effects: audit logging and broadcasts.
-		// These run outside the transaction to avoid SQLite write-lock
-		// contention (LogAudit uses the main *sql.DB, not the tx).
-		if req.RoleID != nil {
 			_ = database.LogAudit(actor, "role_change", "user", id,
 				fmt.Sprintf("changed %s role to %d", user.Username, *req.RoleID))
 			if role, err := database.GetRoleByID(*req.RoleID); err == nil && role != nil {
@@ -153,18 +147,6 @@ func handlePatchUser(database *db.DB, hub HubBroadcaster, permInvalidator Permis
 				}
 			}
 		}
-		if req.Banned != nil {
-			if *req.Banned {
-				_ = database.LogAudit(actor, "user_ban", "user", id,
-					fmt.Sprintf("banned %s: %s", user.Username, banReason))
-				if hub != nil {
-					hub.BroadcastMemberBan(id)
-				}
-			} else {
-				_ = database.LogAudit(actor, "user_unban", "user", id,
-					fmt.Sprintf("unbanned %s", user.Username))
-			}
-		}
 
 		updated, err := database.GetUserByID(id)
 		if err != nil {

Server/api/router.go

@@ -233,7 +233,7 @@ func NewRouter(cfg *config.Config, database *db.DB, ver string, logBuf *admin.Ri
 	// Admin panel: static files + REST API (Phase 6).
 	// Restrict /admin to configured CIDRs (default: private networks only).
 	u := updater.NewUpdater(ver, cfg.GitHub.Token, cfg.GitHub.Owner, cfg.GitHub.Repo)
-	adminHandler := admin.NewHandler(database, ver, hub, u, logBuf, cfg.Server.AllowedOrigins, svc.Permissions)
+	adminHandler := admin.NewHandler(database, ver, hub, u, logBuf, cfg.Server.AllowedOrigins, svc.Permissions, svc.Moderation)
 	r.Group(func(r chi.Router) {
 		r.Use(AdminIPRestrict(cfg.Server.AdminAllowedCIDRs, cfg.Server.TrustedProxies))
 		r.Mount("/admin", adminHandler)

Server/service/moderation.go

@@ -22,29 +22,36 @@ func NewModerationService(st store.Store, perms *PermissionService) *ModerationS
 	return &ModerationService{st: st, perms: perms}
 }
 
-// requireBanAuthority verifies the actor is allowed to ban/unban the target.
-// The actor must hold BAN_MEMBERS (or Administrator, which bypasses permission
-// checks) and must outrank the target in the role hierarchy — mirroring the
-// position-based hierarchy used elsewhere (see admin/middleware.go and
-// permissions.OwnerRolePosition). Returns ErrForbidden when either check fails.
-func (s *ModerationService) requireBanAuthority(actorID, targetID int64) error {
+// requireBanPermission verifies the actor holds BAN_MEMBERS (or the
+// Administrator bypass). It deliberately takes no target: it runs before any
+// target lookup so an actor without ban authority always sees Forbidden and
+// never NotFound — the ban path cannot be used to enumerate user ids.
+func (s *ModerationService) requireBanPermission(actorID int64) error {
 	if s.perms == nil {
 		// No permission service wired — fail closed rather than allow unchecked bans.
 		return fmt.Errorf("%w: permission service unavailable", ErrForbidden)
 	}
-
 	actorRole, err := s.perms.GetRoleForUser(actorID)
 	if err != nil || actorRole == nil {
 		return fmt.Errorf("%w: failed to load actor role", ErrForbidden)
 	}
-
 	if !permissions.HasAdmin(actorRole.Permissions) &&
 		!permissions.HasPerm(actorRole.Permissions, permissions.BanMembers) {
 		return fmt.Errorf("%w: missing BAN_MEMBERS permission", ErrForbidden)
 	}
+	return nil
+}
 
-	// Role hierarchy: the actor must strictly outrank the target so a user
-	// cannot ban a peer or a higher-ranked user (e.g. the owner).
+// requireOutranks enforces the role hierarchy: the actor must strictly
+// outrank the target so a user cannot ban a peer or a higher-ranked user
+// (e.g. the owner) — mirroring the position-based hierarchy used elsewhere.
+// Runs after requireBanPermission and the existence check, so only callers
+// that already hold ban authority reach it.
+func (s *ModerationService) requireOutranks(actorID, targetID int64) error {
+	actorRole, err := s.perms.GetRoleForUser(actorID)
+	if err != nil || actorRole == nil {
+		return fmt.Errorf("%w: failed to load actor role", ErrForbidden)
+	}
 	targetRole, err := s.perms.GetRoleForUser(targetID)
 	if err != nil || targetRole == nil {
 		return fmt.Errorf("%w: failed to load target role", ErrForbidden)
@@ -52,7 +59,6 @@ func (s *ModerationService) requireBanAuthority(actorID, targetID int64) error {
 	if actorRole.Position <= targetRole.Position {
 		return fmt.Errorf("%w: cannot moderate a user of equal or higher rank", ErrForbidden)
 	}
-
 	return nil
 }
 
@@ -77,13 +83,16 @@ func (s *ModerationService) BanUser(actorID, targetID int64, reason string, expi
 		return fmt.Errorf("%w: cannot ban yourself", ErrBadRequest)
 	}
 
+	// Authorization before existence: an actor without ban authority learns
+	// nothing about which user ids exist.
+	if err := s.requireBanPermission(actorID); err != nil {
+		return err
+	}
 	target, err := s.st.GetUserByID(targetID)
 	if err != nil || target == nil {
 		return fmt.Errorf("%w: user not found", ErrNotFound)
 	}
-
-	// Authorization: actor must hold BAN_MEMBERS and outrank the target.
-	if err := s.requireBanAuthority(actorID, targetID); err != nil {
+	if err := s.requireOutranks(actorID, targetID); err != nil {
 		return err
 	}
 
@@ -91,7 +100,7 @@ func (s *ModerationService) BanUser(actorID, targetID int64, reason string, expi
 		return fmt.Errorf("%w: failed to ban user", ErrInternal)
 	}
 
-	if err := s.st.LogAudit(actorID, "ban", "user", targetID, reason); err != nil {
+	if err := s.st.LogAudit(actorID, "user_ban", "user", targetID, reason); err != nil {
 		slog.Error("failed to log audit entry", "error", err)
 	}
 
@@ -105,13 +114,15 @@ func (s *ModerationService) UnbanUser(actorID, targetID int64) error {
 		return fmt.Errorf("%w: user_id must be positive", ErrBadRequest)
 	}
 
+	// Authorization before existence — see BanUser.
+	if err := s.requireBanPermission(actorID); err != nil {
+		return err
+	}
 	target, err := s.st.GetUserByID(targetID)
 	if err != nil || target == nil {
 		return fmt.Errorf("%w: user not found", ErrNotFound)
 	}
-
-	// Authorization: actor must hold BAN_MEMBERS and outrank the target.
-	if err := s.requireBanAuthority(actorID, targetID); err != nil {
+	if err := s.requireOutranks(actorID, targetID); err != nil {
 		return err
 	}
 
@@ -119,7 +130,7 @@ func (s *ModerationService) UnbanUser(actorID, targetID int64) error {
 		return fmt.Errorf("%w: failed to unban user", ErrInternal)
 	}
 
-	if err := s.st.LogAudit(actorID, "unban", "user", targetID, ""); err != nil {
+	if err := s.st.LogAudit(actorID, "user_unban", "user", targetID, ""); err != nil {
 		slog.Error("failed to log audit entry", "error", err)
 	}
 

Server/store/memstore.go

@@ -743,12 +743,26 @@ func (m *MemStore) ListAllUsers(_ int, _ int) ([]db.UserWithRole, error) {
 	panic("memstore: not implemented: ListAllUsers")
 }
 
-func (m *MemStore) BanUser(_ int64, _ string, _ *time.Time) error {
-	panic("memstore: not implemented: BanUser")
+func (m *MemStore) BanUser(userID int64, reason string, _ *time.Time) error {
+	m.mu.Lock()
+	defer m.mu.Unlock()
+	if u, ok := m.users[userID]; ok {
+		u.Banned = true
+		r := reason
+		u.BanReason = &r
+	}
+	return nil
 }
 
-func (m *MemStore) UnbanUser(_ int64) error {
-	panic("memstore: not implemented: UnbanUser")
+func (m *MemStore) UnbanUser(userID int64) error {
+	m.mu.Lock()
+	defer m.mu.Unlock()
+	if u, ok := m.users[userID]; ok {
+		u.Banned = false
+		u.BanReason = nil
+		u.BanExpires = nil
+	}
+	return nil
 }
 
 func (m *MemStore) LogAudit(_ int64, _, _ string, _ int64, _ string) error {