From a3459e5f8010412218bf430ecf47b112fe329422 Mon Sep 17 00:00:00 2001 From: J3vb <192430104+J3vb@users.noreply.github.com> Date: Sun, 19 Jul 2026 08:41:12 +0200 Subject: [PATCH] fix(admin): route admin-panel bans through ModerationService (W1-4) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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 --- Server/admin/admin.go | 5 +- Server/admin/api.go | 5 +- Server/admin/handlers_users.go | 110 ++++++++++++++------------------- Server/api/router.go | 2 +- Server/service/moderation.go | 49 +++++++++------ Server/store/memstore.go | 22 +++++-- 6 files changed, 101 insertions(+), 92 deletions(-) diff --git a/Server/admin/admin.go b/Server/admin/admin.go index cf7c0879..b9ecad2b 100644 --- a/Server/admin/admin.go +++ b/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/…", diff --git a/Server/admin/api.go b/Server/admin/api.go index 0e2210ea..276bbb7e 100644 --- a/Server/admin/api.go +++ b/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)) diff --git a/Server/admin/handlers_users.go b/Server/admin/handlers_users.go index 7b61da73..c3956bd9 100644 --- a/Server/admin/handlers_users.go +++ b/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 { diff --git a/Server/api/router.go b/Server/api/router.go index e6b50dfe..5cbd877c 100644 --- a/Server/api/router.go +++ b/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) diff --git a/Server/service/moderation.go b/Server/service/moderation.go index be1b39cd..12742bbf 100644 --- a/Server/service/moderation.go +++ b/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) } diff --git a/Server/store/memstore.go b/Server/store/memstore.go index 5a56a9d8..8837b2e9 100644 --- a/Server/store/memstore.go +++ b/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 {