diff --git a/Server/api/auth_deps.go b/Server/api/auth_deps.go new file mode 100644 index 00000000..c3ff3f94 --- /dev/null +++ b/Server/api/auth_deps.go @@ -0,0 +1,53 @@ +package api + +import ( + "context" + + "github.com/J3vb/OwnCord/Server/service" +) + +// AuthService is the consumer-owned interface behind the auth routes: every +// call auth_handler.go and totp_handler.go make below the transport layer, and +// nothing more (layout-refactor supplement, "interface beside the consumer"). +// service.AuthService implements it. A handler decodes and validates the +// request, calls one method, and encodes either the result or the returned +// service.Err* value; every lockout, password compare, sentinel mapping, +// audit write and broadcast lives behind these nine methods. +// +// Nine methods stand in for the ten *db.DB methods, two db functions and two +// db sentinels the two handlers called directly at 71d867cb +// (docs/architecture/server-boundaries.md, "Auth slice"). +type AuthService interface { + // RegistrationPolicy reports whether registration is permitted right now. + // It is the one gate that runs before the body is read: two + // characterization rows pin a closed server's 403 ahead of any + // credential, malformed body included. + RegistrationPolicy(ctx context.Context) error + // Register consumes the invite, creates the account and issues a session. + // in is already validated (see service.RegisterInput). + Register(ctx context.Context, in service.RegisterInput) (*service.AuthResult, error) + // Login runs the lockout gates and the constant-time password check, then + // issues a session or, for an enrolled account, starts a two-factor + // challenge. + Login(ctx context.Context, in service.LoginInput) (*service.AuthResult, error) + // VerifyTOTP completes a challenge Login started and issues the session, + // bound to the login request's device and IP rather than this one's. + VerifyTOTP(ctx context.Context, partialToken, code string) (*service.AuthResult, error) + // Logout revokes p.Session server-side and clears the custom status. + Logout(ctx context.Context, p service.Principal) error + // DeleteAccount confirms the password, anonymises and bans the account and + // broadcasts member_ban. ip is only logged and audited. + DeleteAccount(ctx context.Context, p service.Principal, password, ip string) error + // EnableTOTP confirms the password and stages a pending secret; qrURI is + // the enrolment payload for the authenticator app. + EnableTOTP(ctx context.Context, p service.Principal, password string) (qrURI string, err error) + // ConfirmTOTP verifies code against the pending secret, persists it and + // revokes the caller's other sessions. + ConfirmTOTP(ctx context.Context, p service.Principal, password, code string) (*service.TOTPChangeResult, error) + // DisableTOTP confirms the password, refuses while the server requires + // 2FA, clears the secret and revokes the caller's other sessions. + DisableTOTP(ctx context.Context, p service.Principal, password string) (*service.TOTPChangeResult, error) +} + +// The production implementation satisfies the interface it was extracted for. +var _ AuthService = (*service.AuthService)(nil) diff --git a/Server/api/auth_handler.go b/Server/api/auth_handler.go index c5d33254..a2b81ee2 100644 --- a/Server/api/auth_handler.go +++ b/Server/api/auth_handler.go @@ -4,7 +4,6 @@ import ( "context" "encoding/json" "errors" - "fmt" "log/slog" "net/http" "strings" @@ -12,8 +11,6 @@ import ( "unicode/utf8" "github.com/J3vb/OwnCord/Server/auth" - "github.com/J3vb/OwnCord/Server/db" - "github.com/J3vb/OwnCord/Server/permissions" "github.com/J3vb/OwnCord/Server/service" "github.com/go-chi/chi/v5" ) @@ -21,16 +18,9 @@ import ( // maxLoginUsernameLen bounds the username accepted by handleLogin, mirroring // auth.ValidateUsername's 32-rune cap on registered usernames. Enforced // before the value is ever used to build a RateLimiter map key — see the -// check in handleLogin for why. +// check in loginReadRequest for why. const maxLoginUsernameLen = 32 -// genericAuthError is returned for all login/register failures to avoid -// revealing whether a username exists. -var genericAuthError = errorResponse{ - Error: "INVALID_CREDENTIALS", - Message: "invalid invite or credentials", -} - // registerRequest is the JSON body for POST /api/v1/auth/register. type registerRequest struct { Username string `json:"username"` @@ -44,25 +34,6 @@ type loginRequest struct { Password string `json:"password"` } -// userResponse is the user shape included in auth responses. -type userResponse struct { - ID int64 `json:"id"` - Username string `json:"username"` - Avatar string `json:"avatar,omitempty"` - // DisplayName and About are always present (null = unset) so the settings - // form can tell "cleared" from "the server does not know this field". - DisplayName *string `json:"display_name"` - About *string `json:"about"` - // CustomStatus is the user's own free-text status line. - CustomStatus *string `json:"custom_status"` - // Status is the user's own true status, invisible included. This response - // only ever describes the caller, so there is nothing to hide from them. - Status string `json:"status"` - RoleID int64 `json:"role_id"` - TOTPEnabled bool `json:"totp_enabled"` - CreatedAt string `json:"created_at"` -} - // authSuccessResponse is returned on successful login/register. type authSuccessResponse struct { Token string `json:"token,omitempty"` @@ -71,77 +42,56 @@ type authSuccessResponse struct { User *userResponse `json:"user,omitempty"` } -// AuthBroadcaster is the interface handleDeleteAccount uses to notify -// connected WebSocket clients that an account is gone. Satisfied by *ws.Hub -// (which already implements BroadcastMemberBan for the admin ban path this -// mirrors). -type AuthBroadcaster interface { - BroadcastMemberBan(userID int64) -} - -// MountAuthRoutes registers all auth endpoints on the given router. -// Rate limiters are applied per-endpoint as specified. trustedProxies is the -// list of CIDRs whose X-Forwarded-For / X-Real-IP headers are honoured for -// rate-limiting IP resolution. totpKey is the AES-256 key used to encrypt -// TOTP secrets at rest (M1 security hardening). -// -// broadcaster is variadic and optional: MountAuthRoutes is called before the -// hub exists (router.go mounts auth routes first, and the hub needs the -// router to register its own webhook route), so a caller that cannot supply -// one yet may omit it entirely and self-deletion simply sends no event, -// exactly like today. A caller mounted after hub creation should pass it so -// DELETE /api/v1/auth/account can broadcast the same member_ban event the -// admin ban path already sends for the identical anonymise-and-ban DB state. -func MountAuthRoutes(r chi.Router, database *db.DB, limiter *auth.RateLimiter, trustedProxies []string, totpKey []byte, broadcaster ...AuthBroadcaster) { - var ab AuthBroadcaster - if len(broadcaster) > 0 { - ab = broadcaster[0] - } - registerLimiter := limiter - loginLimiter := limiter - partialStore := auth.NewPartialAuthStore(partialAuthStoreTTL) - pendingTOTPStore := auth.NewPendingTOTPStore(pendingTOTPStoreTTL) - usedTOTPCodes := auth.NewUsedTOTPCodeStore() - +// MountAuthRoutes registers all auth endpoints on the given router. svc owns +// every decision below the transport (service.AuthService in production, see +// AuthService); requireAuth is the AuthMiddleware the authenticated routes +// mount, built by the caller because it needs the database handle this file +// no longer sees. Rate limiters are applied per-endpoint as specified; +// trustedProxies is the list of CIDRs whose X-Forwarded-For / X-Real-IP +// headers are honoured for rate-limiting IP resolution. +func MountAuthRoutes(r chi.Router, svc AuthService, requireAuth func(http.Handler) http.Handler, limiter *auth.RateLimiter, trustedProxies []string) { r.Route("/api/v1/auth", func(r chi.Router) { - r.With(RateLimitMiddleware(registerLimiter, "register:", scaledAuthLimit(registerRateLimitPerMinute), time.Minute, trustedProxies)). - Post("/register", handleRegister(database, trustedProxies)) + r.With(RateLimitMiddleware(limiter, "register:", scaledAuthLimit(registerRateLimitPerMinute), time.Minute, trustedProxies)). + Post("/register", handleRegister(svc, trustedProxies)) - r.With(RateLimitMiddleware(loginLimiter, "login:", scaledAuthLimit(loginRateLimitPerMinute), time.Minute, trustedProxies)). - Post("/login", handleLogin(database, limiter, partialStore, trustedProxies)) + r.With(RateLimitMiddleware(limiter, "login:", scaledAuthLimit(loginRateLimitPerMinute), time.Minute, trustedProxies)). + Post("/login", handleLogin(svc, trustedProxies)) r.With(RateLimitMiddleware(limiter, "totp_verify:", scaledAuthLimit(verifyTOTPRateLimitPerMinute), time.Minute, trustedProxies)). - Post("/verify-totp", handleVerifyTOTP(database, partialStore, limiter, usedTOTPCodes, totpKey)) + Post("/verify-totp", handleVerifyTOTP(svc)) - r.With(AuthMiddleware(database)). - Post("/logout", handleLogout(database)) + r.With(requireAuth). + Post("/logout", handleLogout(svc)) - r.With(AuthMiddleware(database)). + r.With(requireAuth). Get("/me", handleMe()) - r.With(AuthMiddleware(database), + r.With(requireAuth, RateLimitMiddleware(limiter, "del_account:", scaledAuthLimit(sensitiveEndpointRateLimitPerMinute), time.Minute, trustedProxies)). - Delete("/account", handleDeleteAccount(database, limiter, ab)) + Delete("/account", handleDeleteAccount(svc)) }) - r.With(AuthMiddleware(database), + r.With(requireAuth, RateLimitMiddleware(limiter, "totp:", scaledAuthLimit(sensitiveEndpointRateLimitPerMinute), time.Minute, trustedProxies)). - Post("/api/v1/users/me/totp/enable", handleEnableTOTP(pendingTOTPStore, limiter)) + Post("/api/v1/users/me/totp/enable", handleEnableTOTP(svc)) - r.With(AuthMiddleware(database), + r.With(requireAuth, RateLimitMiddleware(limiter, "totp:", scaledAuthLimit(sensitiveEndpointRateLimitPerMinute), time.Minute, trustedProxies)). - Post("/api/v1/users/me/totp/confirm", handleConfirmTOTP(database, pendingTOTPStore, usedTOTPCodes, limiter, totpKey)) + Post("/api/v1/users/me/totp/confirm", handleConfirmTOTP(svc)) - r.With(AuthMiddleware(database), + r.With(requireAuth, RateLimitMiddleware(limiter, "totp:", scaledAuthLimit(sensitiveEndpointRateLimitPerMinute), time.Minute, trustedProxies)). - Delete("/api/v1/users/me/totp", handleDisableTOTP(database, pendingTOTPStore, limiter)) + Delete("/api/v1/users/me/totp", handleDisableTOTP(svc)) } // handleRegister processes POST /api/v1/auth/register. -func handleRegister(database *db.DB, trustedProxies []string) http.HandlerFunc { +func handleRegister(svc AuthService, trustedProxies []string) http.HandlerFunc { proxyNets := parseCIDRList(trustedProxies) // W3-3a: parse once at construction return func(w http.ResponseWriter, r *http.Request) { - if !registerPolicyGate(w, r, database) { + // The policy gate runs before any credential is read: a closed + // server refuses even a malformed body with the policy's 403. + if err := svc.RegistrationPolicy(r.Context()); err != nil { + writeAuthError(r.Context(), w, err) return } @@ -150,116 +100,21 @@ func handleRegister(database *db.DB, trustedProxies []string) http.HandlerFunc { return } - // Hash password before consuming the invite so that a hashing failure - // does not burn a valid invite code. - hash, err := auth.HashPassword(req.Password) + res, err := svc.Register(r.Context(), service.RegisterInput{ + Username: req.Username, + Password: req.Password, + InviteCode: req.InviteCode, + Device: truncateDevice(r.Header.Get("User-Agent")), + IP: clientIPWithProxies(r, proxyNets), + }) if err != nil { - writeJSON(w, http.StatusInternalServerError, errorResponse{ - Error: "INTERNAL_ERROR", - Message: "failed to process registration", - }) + writeAuthError(r.Context(), w, err) return } - - // Atomically consume the invite and create the user so failed - // registrations do not burn a valid invite code. - uid, err := database.CreateUserWithInvite(r.Context(), req.Username, hash, int(permissions.MemberRoleID), req.InviteCode) - if err != nil { - // UNIQUE constraint violation → duplicate username → 400. - // Any other DB error → 500. - switch { - case db.IsUniqueConstraintError(err): - writeJSON(w, http.StatusBadRequest, genericAuthError) - case errors.Is(err, db.ErrNotFound): - writeJSON(w, http.StatusBadRequest, genericAuthError) - default: - slog.Error("CreateUserWithInvite failed", "err", err, "username", req.Username) - writeJSON(w, http.StatusInternalServerError, errorResponse{ - Error: "INTERNAL_ERROR", - Message: "registration failed — please try again", - }) - } - return - } - - ip := clientIPWithProxies(r, proxyNets) - slog.Info("user registered", "username", req.Username, "user_id", uid, "ip", ip) - db.WriteAudit(context.WithoutCancel(r.Context()), database, uid, "user_register", "user", uid, - "new account created via invite") - - // Issue session. - token, err := auth.GenerateToken() - if err != nil { - writeJSON(w, http.StatusInternalServerError, errorResponse{ - Error: "INTERNAL_ERROR", - Message: "failed to create session", - }) - return - } - - device := truncateDevice(r.Header.Get("User-Agent")) - if _, err := database.CreateSession(r.Context(), uid, auth.HashToken(token), device, ip); err != nil { - writeJSON(w, http.StatusInternalServerError, errorResponse{ - Error: "INTERNAL_ERROR", - Message: "failed to create session", - }) - return - } - - user, err := database.GetUserByID(r.Context(), uid) - if err != nil || user == nil { - slog.Error("failed to fetch user after registration", "user_id", uid, "error", err) - writeJSON(w, http.StatusInternalServerError, errorResponse{ - Error: "INTERNAL_ERROR", - Message: "registration succeeded but user fetch failed", - }) - return - } - writeJSON(w, http.StatusCreated, authSuccessResponse{ - Token: token, - Requires2FA: false, - User: toUserResponse(user), - }) + writeJSON(w, http.StatusCreated, authResponse(res)) } } -// registerPolicyGate reports whether registration is currently permitted, -// writing the refusal response itself when it is not. -func registerPolicyGate(w http.ResponseWriter, r *http.Request, database *db.DB) bool { - registrationOpen, err := isRegistrationOpen(r.Context(), database) - if err != nil { - writeJSON(w, http.StatusInternalServerError, errorResponse{ - Error: "INTERNAL_ERROR", - Message: "failed to load registration policy", - }) - return false - } - if !registrationOpen { - writeJSON(w, http.StatusForbidden, errorResponse{ - Error: "FORBIDDEN", - Message: "registration is currently closed", - }) - return false - } - - require2FA, err := isRequire2FAEnabled(r.Context(), database) - if err != nil { - writeJSON(w, http.StatusInternalServerError, errorResponse{ - Error: "INTERNAL_ERROR", - Message: "failed to load registration policy", - }) - return false - } - if require2FA { - writeJSON(w, http.StatusForbidden, errorResponse{ - Error: "FORBIDDEN", - Message: "registration is unavailable while two-factor authentication is required", - }) - return false - } - return true -} - // registerReadRequest decodes and validates the registration body, writing the // rejection response itself when the input cannot be used. func registerReadRequest(w http.ResponseWriter, r *http.Request) (registerRequest, bool) { @@ -327,7 +182,7 @@ func registerReadRequest(w http.ResponseWriter, r *http.Request) (registerReques } // handleLogin processes POST /api/v1/auth/login. -func handleLogin(database *db.DB, limiter *auth.RateLimiter, partialStore *auth.PartialAuthStore, trustedProxies []string) http.HandlerFunc { +func handleLogin(svc AuthService, trustedProxies []string) http.HandlerFunc { proxyNets := parseCIDRList(trustedProxies) // W3-3a: parse once at construction return func(w http.ResponseWriter, r *http.Request) { req, ok := loginReadRequest(w, r) @@ -335,77 +190,17 @@ func handleLogin(database *db.DB, limiter *auth.RateLimiter, partialStore *auth. return } - ip := clientIPWithProxies(r, proxyNets) - - user, ok := loginAuthenticate(w, r, database, limiter, req, ip) - if !ok { - return - } - - if auth.IsEffectivelyBanned(user) { - slog.Warn("banned user login attempt", "username", user.Username, "user_id", user.ID, "ip", ip) - db.WriteAudit(context.WithoutCancel(r.Context()), database, user.ID, "login_blocked_banned", "user", user.ID, - "banned user attempted login from "+ip) - writeJSON(w, http.StatusForbidden, errorResponse{ - Error: "FORBIDDEN", - Message: "your account has been suspended", - }) - return - } - - require2FA, err := isRequire2FAEnabled(r.Context(), database) - if err != nil { - writeJSON(w, http.StatusInternalServerError, errorResponse{ - Error: "INTERNAL_ERROR", - Message: "failed to load authentication policy", - }) - return - } - if user.TOTPSecret != nil { - partialToken, err := partialStore.Issue(user.ID, truncateDevice(r.Header.Get("User-Agent")), ip) - if err != nil { - writeJSON(w, http.StatusInternalServerError, errorResponse{ - Error: "INTERNAL_ERROR", - Message: "failed to start two-factor challenge", - }) - return - } - writeJSON(w, http.StatusOK, authSuccessResponse{ - PartialToken: partialToken, - Requires2FA: true, - }) - return - } - if require2FA { - writeJSON(w, http.StatusForbidden, errorResponse{ - Error: "FORBIDDEN", - Message: "two-factor authentication must be enabled on this account before login", - }) - return - } - - // Issue session. - token, err := issueSession(r.Context(), database, user.ID, truncateDevice(r.Header.Get("User-Agent")), ip) - if err != nil { - writeJSON(w, http.StatusInternalServerError, errorResponse{ - Error: "INTERNAL_ERROR", - Message: "failed to create session", - }) - return - } - - // Don't set status to "online" here — the WebSocket connection in - // serve.go does that when the user actually connects. Setting it here - // would leave the user permanently "online" if they never open a WS - // connection or if the client crashes before connecting. - slog.Info("user logged in", "username", user.Username, "user_id", user.ID, "ip", ip) - db.WriteAudit(context.WithoutCancel(r.Context()), database, user.ID, "user_login", "user", user.ID, - "logged in from "+ip) - writeJSON(w, http.StatusOK, authSuccessResponse{ - Token: token, - Requires2FA: false, - User: toUserResponse(user), + res, err := svc.Login(r.Context(), service.LoginInput{ + Username: req.Username, + Password: req.Password, + Device: truncateDevice(r.Header.Get("User-Agent")), + IP: clientIPWithProxies(r, proxyNets), }) + if err != nil { + writeAuthError(r.Context(), w, err) + return + } + writeJSON(w, http.StatusOK, authResponse(res)) } } @@ -434,12 +229,13 @@ func loginReadRequest(w http.ResponseWriter, r *http.Request) (loginRequest, boo } // F: reject an over-long username before it is ever used to build a - // RateLimiter map key below (unameKey, failKey, userFailKey, lockout - // keys). Unlike registration, login has no account to validate - // against yet, so nothing else bounds this value — an unauthenticated - // caller could otherwise pin an arbitrarily large, body-sized string - // as a retained key (Cleanup only evicts it after hours). Mirrors the - // same 32-rune cap auth.ValidateUsername enforces at registration. + // RateLimiter map key (unameKey, failKey, userFailKey, lockout keys in + // service.AuthService). Unlike registration, login has no account to + // validate against yet, so nothing else bounds this value — an + // unauthenticated caller could otherwise pin an arbitrarily large, + // body-sized string as a retained key (Cleanup only evicts it after + // hours). Mirrors the same 32-rune cap auth.ValidateUsername enforces at + // registration. if utf8.RuneCountInString(req.Username) > maxLoginUsernameLen { writeJSON(w, http.StatusBadRequest, errorResponse{ Error: "INVALID_INPUT", @@ -450,144 +246,18 @@ func loginReadRequest(w http.ResponseWriter, r *http.Request) (loginRequest, boo return req, true } -// loginAuthenticate runs the lockout gates, the constant-time password compare -// and the failure accounting for one login attempt. It returns the -// authenticated user, or false after writing the rejection response itself. -func loginAuthenticate(w http.ResponseWriter, r *http.Request, database *db.DB, limiter *auth.RateLimiter, req loginRequest, ip string) (*db.User, bool) { - // Check per-IP lockout first. - lockKey := "login_lock:" + ip - if limiter.IsLockedOut(lockKey) { - writeJSON(w, http.StatusTooManyRequests, errorResponse{ - Error: "RATE_LIMITED", - Message: "account temporarily locked due to too many failed attempts", - }) - return nil, false - } - - // BUG-110: Also check per-username lockout to prevent distributed brute force. - // F1: canonicalize the username the same way GetUserByUsername does (COLLATE - // NOCASE) before keying the lockout, so case variants of one account - // (admin/Admin/ADMIN) share a single bucket instead of each getting its own. - unameKey := strings.ToLower(req.Username) - userLockKey := "login_user_lock:" + unameKey - if limiter.IsLockedOut(userLockKey) { - writeJSON(w, http.StatusTooManyRequests, errorResponse{ - Error: "RATE_LIMITED", - Message: "account temporarily locked due to too many failed attempts", - }) - return nil, false - } - - // Constant-time lookup: always attempt bcrypt compare even when user - // does not exist to prevent timing-based username enumeration. - user, err := database.GetUserByUsername(r.Context(), req.Username) - - // Distinguish DB errors from authentication failures. DB errors - // should NOT increment the rate limiter — otherwise a transient - // DB outage would lock out legitimate users. - if err != nil && user == nil { - // Could be a real DB error or simply "user not found". - // GetUserByUsername returns (nil, nil) for not-found, so a - // non-nil error here is a genuine DB failure. - slog.Error("login: GetUserByUsername failed", "err", err, "ip", ip) - writeJSON(w, http.StatusInternalServerError, errorResponse{ - Error: "INTERNAL_ERROR", - Message: "login temporarily unavailable", - }) - return nil, false - } - - failKey := "login_fail:" + ip - userFailKey := "login_user_fail:" + unameKey - // F3: atomically reserve this attempt BEFORE the bcrypt compare. The - // read-only IsLockedOut gates above are check-then-act: N concurrent - // requests all pass them before any failure is recorded below, so the - // per-username cap — the only cross-IP brute-force defence — bound - // only sequential attackers. Allow records the attempt under the - // limiter's lock, capping a concurrent burst at the same budget a - // sequential attacker gets. Sized at threshold+1 so the sequential - // accepted-input set is unchanged: failures 1–10 still land, the 10th - // still trips the lockout (via the Check below), and a correct - // password on attempt 10 still succeeds — successful logins reset - // both counters. The reservation sits after the DB-error return above - // so a transient DB outage still does not consume attempts. - if !limiter.Allow(failKey, scaledAuthLimit(loginFailureThreshold)+1, loginFailureWindow) || - !limiter.Allow(userFailKey, loginUserFailureThreshold+1, loginUserFailureWindow) { - writeJSON(w, http.StatusTooManyRequests, errorResponse{ - Error: "RATE_LIMITED", - Message: "account temporarily locked due to too many failed attempts", - }) - return nil, false - } - // Always run the password check — with an empty hash when the user does - // not exist. auth.CheckPassword performs a dummy bcrypt comparison for an - // empty hash, so bcrypt executes on every path and response time stays - // constant, preventing timing-based username enumeration. (A `user == nil - // || CheckPassword(...)` short-circuit would skip bcrypt entirely for - // unknown usernames, reintroducing the timing side-channel.) - storedHash := "" - if user != nil { - storedHash = user.PasswordHash - } - if !auth.CheckPassword(storedHash, req.Password) { - // The attempt was already recorded atomically up-front (F3); here - // only decide the lockouts, at the same boundary as before: the - // 10th in-window failure locks the key. Check is read-only, so - // the reservation is not double-counted. - if !limiter.Check(failKey, scaledAuthLimit(loginFailureThreshold)+1, loginFailureWindow) { - limiter.Lockout(r.Context(), lockKey, loginLockoutDuration) - } - // BUG-110: per-username lockout on threshold. - if !limiter.Check(userFailKey, loginUserFailureThreshold+1, loginUserFailureWindow) { - limiter.Lockout(r.Context(), userLockKey, loginUserLockoutDuration) - } - slog.Info("login failed", "ip", ip, "username_len", len(req.Username)) - writeJSON(w, http.StatusUnauthorized, errorResponse{ - Error: "UNAUTHORIZED", - Message: "invalid credentials", - }) - return nil, false - } - - // Reset failure counters on success. - limiter.Reset(r.Context(), failKey) - limiter.Reset(r.Context(), userFailKey) - return user, true -} - // handleLogout processes POST /api/v1/auth/logout. -func handleLogout(database *db.DB) http.HandlerFunc { +func handleLogout(svc AuthService) http.HandlerFunc { return func(w http.ResponseWriter, r *http.Request) { - sess, ok := r.Context().Value(SessionKey).(*db.Session) - if !ok || sess == nil { - writeJSON(w, http.StatusUnauthorized, errorResponse{ - Error: "UNAUTHORIZED", - Message: "not authenticated", - }) + p, ok := principal(r) + if !ok || p.Session == nil { + writeNotAuthenticated(w) return } - - // The client clears its token optimistically — once logout reaches the - // server, the revocation must not die with a dropped connection. - if err := database.DeleteSession(context.WithoutCancel(r.Context()), sess.TokenHash); err != nil { - writeJSON(w, http.StatusInternalServerError, errorResponse{ - Error: "INTERNAL_ERROR", - Message: "failed to logout", - }) + if err := svc.Logout(r.Context(), p); err != nil { + writeAuthError(r.Context(), w, err) return } - - // A custom status is a "what I am doing right now" note. Leaving it - // standing after the user signed out states something about them that - // is no longer true, so logout clears it — unlike the chosen presence - // status, which is a preference and deliberately survives. - if err := database.UpdateUserCustomStatus(context.WithoutCancel(r.Context()), sess.UserID, nil); err != nil { - slog.Warn("failed to clear custom status on logout", "user_id", sess.UserID, "err", err) - } - - slog.Info("user logged out", "user_id", sess.UserID) - db.WriteAudit(context.WithoutCancel(r.Context()), database, sess.UserID, "user_logout", "user", sess.UserID, "") - w.WriteHeader(http.StatusNoContent) } } @@ -595,15 +265,12 @@ func handleLogout(database *db.DB) http.HandlerFunc { // handleMe processes GET /api/v1/auth/me. func handleMe() http.HandlerFunc { return func(w http.ResponseWriter, r *http.Request) { - user, ok := r.Context().Value(UserKey).(*db.User) - if !ok || user == nil { - writeJSON(w, http.StatusUnauthorized, errorResponse{ - Error: "UNAUTHORIZED", - Message: "not authenticated", - }) + p, ok := principal(r) + if !ok { + writeNotAuthenticated(w) return } - writeJSON(w, http.StatusOK, toUserResponse(user)) + writeJSON(w, http.StatusOK, toUserResponse(p.User)) } } @@ -612,30 +279,14 @@ type deleteAccountRequest struct { Password string `json:"password"` } -// handleDeleteAccount processes DELETE /api/v1/auth/account. -// The caller must supply their current password for confirmation. -// Progressive lockout mirrors the login handler: 3 failures → 15-min lock. -// broadcaster may be nil, in which case no event is sent and other connected -// clients converge on their next reconnect instead (same fallback every -// other broadcaster-optional handler in this package uses). -func handleDeleteAccount(database *db.DB, limiter *auth.RateLimiter, broadcaster AuthBroadcaster) http.HandlerFunc { +// handleDeleteAccount processes DELETE /api/v1/auth/account. The caller must +// supply their current password for confirmation; the lockout, the compare +// and the member_ban broadcast are the service's. +func handleDeleteAccount(svc AuthService) http.HandlerFunc { return func(w http.ResponseWriter, r *http.Request) { - user, ok := r.Context().Value(UserKey).(*db.User) - if !ok || user == nil { - writeJSON(w, http.StatusUnauthorized, errorResponse{ - Error: "UNAUTHORIZED", - Message: "not authenticated", - }) - return - } - - // Per-user lockout to prevent password brute-force on this destructive endpoint. - lockKey := auth.Key("delete_lock", user.ID) - if limiter.IsLockedOut(lockKey) { - writeJSON(w, http.StatusTooManyRequests, errorResponse{ - Error: "RATE_LIMITED", - Message: "too many failed attempts, try again later", - }) + p, ok := principal(r) + if !ok { + writeNotAuthenticated(w) return } @@ -648,80 +299,70 @@ func handleDeleteAccount(database *db.DB, limiter *auth.RateLimiter, broadcaster return } - if req.Password == "" { - writeJSON(w, http.StatusBadRequest, errorResponse{ - Error: "INVALID_INPUT", - Message: "password is required", - }) + if err := svc.DeleteAccount(r.Context(), p, req.Password, clientIP(r)); err != nil { + writeAuthError(r.Context(), w, err) return } - - // Verify the supplied password matches the stored hash. - failKey := auth.Key("delete_fail", user.ID) - if !auth.CheckPassword(user.PasswordHash, req.Password) { - if !limiter.Allow(failKey, deleteAccountFailureThreshold, deleteAccountFailureWindow) { - limiter.Lockout(r.Context(), lockKey, deleteAccountLockoutDuration) - } - writeJSON(w, http.StatusBadRequest, errorResponse{ - Error: "INVALID_INPUT", - Message: "incorrect password", - }) - return - } - limiter.Reset(r.Context(), failKey) - - if err := database.DeleteAccount(r.Context(), user.ID); err != nil { - if errors.Is(err, db.ErrLastAdmin) { - writeJSON(w, http.StatusForbidden, errorResponse{ - Error: "FORBIDDEN", - Message: "cannot delete the last admin account", - }) - return - } - slog.Error("DeleteAccount failed", "err", err, "user_id", user.ID) - writeJSON(w, http.StatusInternalServerError, errorResponse{ - Error: "INTERNAL_ERROR", - Message: "failed to delete account", - }) - return - } - - ip := clientIP(r) - slog.Info("account deleted", "username", user.Username, "user_id", user.ID, "ip", ip) - db.WriteAudit(context.WithoutCancel(r.Context()), database, user.ID, "account_deleted", "user", user.ID, - "account self-deleted from "+ip) - - // DeleteAccount left the row in exactly the state an admin ban does - // (anonymised, banned, sessions revoked) — broadcast the same event so - // every other connected client drops the deleted user immediately - // instead of keeping their pre-deletion username until it reconnects. - if broadcaster != nil { - broadcaster.BroadcastMemberBan(user.ID) - } - w.WriteHeader(http.StatusNoContent) } } -// toUserResponse converts a db.User to the API response shape. -func toUserResponse(u *db.User) *userResponse { - avatar := "" - if u.Avatar != nil { - avatar = *u.Avatar +// authResponse encodes a service result: a session, or the two-factor +// challenge Login started instead of one. +func authResponse(res *service.AuthResult) authSuccessResponse { + if res.Requires2FA { + return authSuccessResponse{ + PartialToken: res.PartialToken, + Requires2FA: true, + } } - resp := &userResponse{ - ID: u.ID, - Username: u.Username, - Avatar: avatar, - DisplayName: u.DisplayName, - About: u.About, - CustomStatus: u.CustomStatus, - Status: u.Status, - RoleID: u.RoleID, - TOTPEnabled: u.TOTPSecret != nil, - CreatedAt: u.CreatedAt, + return authSuccessResponse{ + Token: res.Token, + Requires2FA: false, + User: toUserResponse(res.User), } - return resp +} + +// writeNotAuthenticated is the refusal for a route mounted behind +// AuthMiddleware that still finds no usable principal on the request. +func writeNotAuthenticated(w http.ResponseWriter) { + writeJSON(w, http.StatusUnauthorized, errorResponse{ + Error: "UNAUTHORIZED", + Message: "not authenticated", + }) +} + +// writeAuthError encodes a service.Err* refusal from the auth slice. Each +// named value's Error() is the public message; its category picks the +// status and code, and two values carry a code of their own. Anything that +// is not an auth refusal is a contract bug in the service, logged and +// answered as a generic 500 so no cause leaks to the client. +func writeAuthError(ctx context.Context, w http.ResponseWriter, err error) { + var status int + var code string + switch { + case errors.Is(err, service.ErrRegistrationRejected): + status, code = http.StatusBadRequest, "INVALID_CREDENTIALS" + case errors.Is(err, service.ErrTOTPAlreadyEnabled): + status, code = http.StatusConflict, "TOTP_ALREADY_ENABLED" + case errors.Is(err, service.ErrRateLimited): + status, code = http.StatusTooManyRequests, "RATE_LIMITED" + case errors.Is(err, service.ErrUnauthorized): + status, code = http.StatusUnauthorized, "UNAUTHORIZED" + case errors.Is(err, service.ErrForbidden): + status, code = http.StatusForbidden, "FORBIDDEN" + case errors.Is(err, service.ErrInvalidInput): + status, code = http.StatusBadRequest, "INVALID_INPUT" + case errors.Is(err, service.ErrBadRequest): + status, code = http.StatusBadRequest, "BAD_REQUEST" + case errors.Is(err, service.ErrInternal): + status, code = http.StatusInternalServerError, "INTERNAL_ERROR" + default: + slog.ErrorContext(ctx, "auth service returned a non-refusal error", "error", err) + writeJSON(w, http.StatusInternalServerError, errorResponse{Error: "INTERNAL_ERROR", Message: "internal error"}) + return + } + writeJSON(w, status, errorResponse{Error: code, Message: err.Error()}) } // truncateDevice truncates the User-Agent to prevent oversized session records. @@ -733,54 +374,3 @@ func truncateDevice(ua string) string { } return ua } - -func issueSession(ctx context.Context, database *db.DB, userID int64, device, ip string) (string, error) { - token, err := auth.GenerateToken() - if err != nil { - return "", err - } - if _, err := database.CreateSession(ctx, userID, auth.HashToken(token), device, ip); err != nil { - return "", err - } - return token, nil -} - -func isRequire2FAEnabled(ctx context.Context, database *db.DB) (bool, error) { - return getBooleanSetting(ctx, database, "require_2fa", false) -} - -func isRegistrationOpen(ctx context.Context, database *db.DB) (bool, error) { - return getBooleanSetting(ctx, database, "registration_open", true) -} - -func getBooleanSetting(ctx context.Context, database *db.DB, key string, defaultValue bool) (bool, error) { - value, err := database.GetSetting(ctx, key) - if err != nil { - if errors.Is(err, db.ErrNotFound) { - return defaultValue, nil - } - return false, err - } - return parseBooleanSettingValue(value) -} - -func parseBooleanSettingValue(value string) (bool, error) { - switch strings.ToLower(strings.TrimSpace(value)) { - case "1", "true": - return true, nil - case "0", "false": - return false, nil - default: - return false, fmt.Errorf("invalid boolean setting value %q", value) - } -} - -func requirePasswordConfirmation(user *db.User, password string) error { - if password == "" { - return fmt.Errorf("password is required") - } - if !auth.CheckPassword(user.PasswordHash, password) { - return fmt.Errorf("password confirmation failed") - } - return nil -} diff --git a/Server/api/auth_handler_delete_broadcast_test.go b/Server/api/auth_handler_delete_broadcast_test.go index 8d43c6c9..e418fcc1 100644 --- a/Server/api/auth_handler_delete_broadcast_test.go +++ b/Server/api/auth_handler_delete_broadcast_test.go @@ -7,6 +7,7 @@ import ( "github.com/J3vb/OwnCord/Server/api" "github.com/J3vb/OwnCord/Server/auth" + "github.com/J3vb/OwnCord/Server/service" "github.com/go-chi/chi/v5" ) @@ -32,7 +33,7 @@ func TestDeleteAccount_BroadcastsMemberBan(t *testing.T) { broadcaster := &recordingAuthBroadcaster{} r := chi.NewRouter() - api.MountAuthRoutes(r, database, limiter, nil, testTOTPKey, broadcaster) + api.MountAuthRoutes(r, service.NewAuthService(database, limiter, testTOTPKey, broadcaster), api.AuthMiddleware(database), limiter, nil) hash, _ := auth.HashPassword("correctPass1") uid, _ := database.CreateUser(context.Background(), "deletebroadcast", hash, 4) @@ -51,14 +52,14 @@ func TestDeleteAccount_BroadcastsMemberBan(t *testing.T) { } } -// Omitting the broadcaster (the shape every existing MountAuthRoutes call -// site uses today) must keep working exactly as before: no event, no panic. +// A nil broadcaster (the shape every test mount uses) must keep working +// exactly as before: no event, no panic. func TestDeleteAccount_NoBroadcasterOmitted(t *testing.T) { database := newAuthTestDB(t) limiter := auth.NewRateLimiter() r := chi.NewRouter() - api.MountAuthRoutes(r, database, limiter, nil, testTOTPKey) + api.MountAuthRoutes(r, service.NewAuthService(database, limiter, testTOTPKey, nil), api.AuthMiddleware(database), limiter, nil) hash, _ := auth.HashPassword("correctPass1") uid, _ := database.CreateUser(context.Background(), "deletenobroadcast", hash, 4) diff --git a/Server/api/auth_handler_test.go b/Server/api/auth_handler_test.go index dba48a33..24d90d3f 100644 --- a/Server/api/auth_handler_test.go +++ b/Server/api/auth_handler_test.go @@ -17,6 +17,7 @@ import ( "github.com/J3vb/OwnCord/Server/api" "github.com/J3vb/OwnCord/Server/auth" "github.com/J3vb/OwnCord/Server/db" + "github.com/J3vb/OwnCord/Server/service" "github.com/go-chi/chi/v5" ) @@ -45,7 +46,7 @@ func buildAuthRouter(database *db.DB, limiter *auth.RateLimiter) http.Handler { func buildAuthRouterWithProxies(database *db.DB, limiter *auth.RateLimiter, trustedProxies []string) http.Handler { r := chi.NewRouter() - api.MountAuthRoutes(r, database, limiter, trustedProxies, testTOTPKey) + api.MountAuthRoutes(r, service.NewAuthService(database, limiter, testTOTPKey, nil), api.AuthMiddleware(database), limiter, trustedProxies) return r } diff --git a/Server/api/constants.go b/Server/api/constants.go index 4c2fa052..528d3e26 100644 --- a/Server/api/constants.go +++ b/Server/api/constants.go @@ -1,10 +1,9 @@ package api import ( - "math" - "sync/atomic" "time" + "github.com/J3vb/OwnCord/Server/auth" "github.com/J3vb/OwnCord/Server/config" ) @@ -13,36 +12,18 @@ import ( // Each constant defines either a request cap or a sliding-window duration used // by the per-endpoint rate limiters. -// authRateScaleBits holds the security.auth_rate_limit_multiplier as float -// bits. It scales the per-IP auth request caps and failure thresholds for -// deployments where many users share one IP (office/school NAT) — the -// compiled-in constants below assume roughly one person per address. Atomic -// because tests construct multiple routers concurrently. Set via -// setAuthRateScale in NewRouter; reads happen at mount time and on the login -// failure-count path. -var authRateScaleBits atomic.Uint64 - -func init() { authRateScaleBits.Store(math.Float64bits(1.0)) } +// The auth rate multiplier (security.auth_rate_limit_multiplier) lives in +// auth (auth/ratescale.go) since B3-2, so the route mounts here and the login +// failure accounting in service.AuthService read one value. These wrappers +// keep the mount sites and api/constants_test.go unchanged. // setAuthRateScale clamps and installs the auth rate multiplier. Zero or // negative (unset config) means 1.0. -func setAuthRateScale(m float64) { - if m <= 0 { - m = 1.0 - } - m = math.Min(math.Max(m, 0.1), 100) - authRateScaleBits.Store(math.Float64bits(m)) -} +func setAuthRateScale(m float64) { auth.SetRateScale(m) } // scaledAuthLimit applies the auth rate multiplier to a compiled-in limit, // never returning less than 1. -func scaledAuthLimit(n int) int { - scaled := int(math.Round(float64(n) * math.Float64frombits(authRateScaleBits.Load()))) - if scaled < 1 { - return 1 - } - return scaled -} +func scaledAuthLimit(n int) int { return auth.ScaledLimit(n) } const ( // registerRateLimitPerMinute is the maximum registration attempts per IP per minute. @@ -72,40 +53,6 @@ const ( // stays under this; it exists to bound abuse of the operator's Klipy quota. gifRateLimitPerMinute = 30 - // loginFailureThreshold is the number of failed login attempts (within - // loginFailureWindow) before the IP is locked out. - loginFailureThreshold = 9 - - // loginFailureWindow is the sliding window for counting login failures. - loginFailureWindow = 15 * time.Minute - - // loginLockoutDuration is how long an IP is locked out after exceeding - // loginFailureThreshold. - loginLockoutDuration = 15 * time.Minute - - // deleteAccountFailureThreshold is the number of wrong-password attempts - // before the per-user lockout kicks in. - deleteAccountFailureThreshold = 3 - - // deleteAccountFailureWindow is the sliding window for counting - // delete-account password failures. - deleteAccountFailureWindow = 15 * time.Minute - - // deleteAccountLockoutDuration is how long the account-deletion endpoint - // is locked after exceeding deleteAccountFailureThreshold. - deleteAccountLockoutDuration = 15 * time.Minute - - // totpFailureRateLimit is the maximum TOTP verification failures per user - // within totpFailureWindow before the user is rate-limited. - totpFailureRateLimit = 10 - - // totpFailureWindow is the sliding window for counting per-user TOTP failures. - totpFailureWindow = 15 * time.Minute - - // partialAuthMaxFailures is the number of failed TOTP attempts on a single - // partial-auth challenge before it is revoked. - partialAuthMaxFailures = 5 - // profilePasswordRateLimitPerMinute is the maximum password change attempts // per IP per minute. profilePasswordRateLimitPerMinute = 5 @@ -114,29 +61,6 @@ const ( // per user per minute. profileUpdateRateLimitPerMinute = 10 - // loginUserFailureThreshold is the number of failed login attempts for a - // specific username (regardless of source IP) before the account is locked. - loginUserFailureThreshold = 9 - - // loginUserFailureWindow is the sliding window for per-username login failures. - loginUserFailureWindow = 15 * time.Minute - - // loginUserLockoutDuration is how long a username is locked after exceeding - // loginUserFailureThreshold. - loginUserLockoutDuration = 15 * time.Minute - - // pwConfirmFailureThreshold is the number of wrong-password attempts on - // password-confirmation endpoints before per-user lockout kicks in. - pwConfirmFailureThreshold = 3 - - // pwConfirmFailureWindow is the sliding window for per-user password - // confirmation failures. - pwConfirmFailureWindow = 15 * time.Minute - - // pwConfirmLockoutDuration is how long password-confirmation endpoints are - // locked after exceeding pwConfirmFailureThreshold. - pwConfirmLockoutDuration = 15 * time.Minute - // uploadRateLimitPerMinute is the maximum file uploads per user per minute. uploadRateLimitPerMinute = 10 @@ -149,12 +73,6 @@ const ( // ─── Timeouts & TTLs ──────────────────────────────────────────────────────── const ( - // partialAuthStoreTTL is the lifetime of a partial-auth (2FA) challenge token. - partialAuthStoreTTL = 10 * time.Minute - - // pendingTOTPStoreTTL is the lifetime of a pending TOTP enrollment secret. - pendingTOTPStoreTTL = 10 * time.Minute - // rateLimiterCleanupInterval is how often stale rate-limiter entries are reaped. rateLimiterCleanupInterval = 5 * time.Minute diff --git a/Server/api/constants_test.go b/Server/api/constants_test.go index 44f98e73..01ac3cfc 100644 --- a/Server/api/constants_test.go +++ b/Server/api/constants_test.go @@ -27,8 +27,9 @@ func TestRateLimiterCleanupHorizon_CoversMaxSlowMode(t *testing.T) { } // setAuthRateScale/scaledAuthLimit gate every per-IP auth limit -// (auth_handler.go:107-136) and the per-IP login failure threshold that arms -// the lockout (auth_handler.go:514,537). The multiplier is operator-supplied +// (auth_handler.go MountAuthRoutes) and, through auth.ScaledLimit, the per-IP +// login failure threshold that arms the lockout (service/auth.go +// authenticate). The multiplier is operator-supplied // via security.auth_rate_limit_multiplier and config validates nothing, so // this clamp is all that stands between a typo and brute-force protection // disappearing. @@ -49,7 +50,7 @@ func TestSetAuthRateScale_ClampsMultiplier(t *testing.T) { {"below the floor clamps to 0.1x", 1e-9, verifyTOTPRateLimitPerMinute, 1}, {"at the floor is 0.1x", 0.1, verifyTOTPRateLimitPerMinute, 1}, {"in range scales and rounds", 0.5, loginRateLimitPerMinute, 3}, - {"in range scales the failure threshold", 2, loginFailureThreshold, 18}, + {"in range scales the failure threshold", 2, 9, 18}, // 9 = service/auth.go loginFailureThreshold } for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { @@ -63,7 +64,7 @@ func TestSetAuthRateScale_ClampsMultiplier(t *testing.T) { } // A limit of 0 lets nothing through: on the login failure threshold -// (auth_handler.go:514) that locks every IP out on its first attempt. The +// (service/auth.go authenticate) that locks every IP out on its first attempt. The // smallest allowed multiplier must still leave every scaled limit usable. func TestScaledAuthLimit_NeverBelowOne(t *testing.T) { t.Cleanup(func() { setAuthRateScale(1.0) }) @@ -75,7 +76,7 @@ func TestScaledAuthLimit_NeverBelowOne(t *testing.T) { loginRateLimitPerMinute, verifyTOTPRateLimitPerMinute, sensitiveEndpointRateLimitPerMinute, - loginFailureThreshold, + 9, // service/auth.go loginFailureThreshold } { if got := scaledAuthLimit(n); got < 1 { t.Errorf("scaledAuthLimit(%d) = %d at the 0.1x floor, want >= 1", n, got) @@ -85,20 +86,19 @@ func TestScaledAuthLimit_NeverBelowOne(t *testing.T) { // The multiplier exists for shared-NAT *per-IP* limits. The per-user caps are // the only cross-IP brute-force defence, so scaling them would hand a -// distributed attacker up to 100x the guesses (totp_handler.go:76-80). Those -// caps are only observable through a limiter key inside the handler, so this -// pins the call site instead. +// distributed attacker up to 100x the guesses (service/auth.go VerifyTOTP). +// Those caps are only observable through a limiter key inside the service, so +// this pins the call site instead. func TestPerUserFailureCapsStayUnscaled(t *testing.T) { for file, constants := range map[string][]string{ - "totp_handler.go": {"totpFailureRateLimit"}, - "auth_handler.go": {"loginUserFailureThreshold"}, + "../service/auth.go": {"totpFailureRateLimit", "loginUserFailureThreshold"}, } { src, err := os.ReadFile(file) if err != nil { t.Fatalf("read %s: %v", file, err) } for _, c := range constants { - if strings.Contains(string(src), "scaledAuthLimit("+c) { + if strings.Contains(string(src), "ScaledLimit("+c) || strings.Contains(string(src), "scaledAuthLimit("+c) { t.Errorf("%s scales %s with the per-IP auth multiplier; per-user caps must stay unscaled", file, c) } diff --git a/Server/api/coverage_push_test.go b/Server/api/coverage_push_test.go index 546fb6b2..85cd90e5 100644 --- a/Server/api/coverage_push_test.go +++ b/Server/api/coverage_push_test.go @@ -750,7 +750,7 @@ func buildCombinedRouter(t *testing.T) (http.Handler, *auth.RateLimiter, string) r := chi.NewRouter() svc := service.New(database, limiter) - api.MountAuthRoutes(r, database, limiter, nil, testTOTPKey) + api.MountAuthRoutes(r, service.NewAuthService(database, limiter, testTOTPKey, nil), api.AuthMiddleware(database), limiter, nil) api.MountProfileRoutes(r, database, svc, nil, limiter, nil, nil) api.MountInviteRoutes(r, database, svc) diff --git a/Server/api/invite_handler_test.go b/Server/api/invite_handler_test.go index 031986f7..e530c3ec 100644 --- a/Server/api/invite_handler_test.go +++ b/Server/api/invite_handler_test.go @@ -19,7 +19,7 @@ import ( func buildInviteRouter(database *db.DB, limiter *auth.RateLimiter) http.Handler { r := chi.NewRouter() svc := service.New(database, limiter) - api.MountAuthRoutes(r, database, limiter, nil, testTOTPKey) + api.MountAuthRoutes(r, service.NewAuthService(database, limiter, testTOTPKey, nil), api.AuthMiddleware(database), limiter, nil) api.MountInviteRoutes(r, database, svc) return r } diff --git a/Server/api/middleware.go b/Server/api/middleware.go index dfbb0f3a..bedeabd3 100644 --- a/Server/api/middleware.go +++ b/Server/api/middleware.go @@ -14,6 +14,7 @@ import ( "github.com/J3vb/OwnCord/Server/auth" "github.com/J3vb/OwnCord/Server/db" "github.com/J3vb/OwnCord/Server/permissions" + "github.com/J3vb/OwnCord/Server/service" ) // contextKey is an unexported type for context keys in this package. @@ -67,6 +68,18 @@ func (t *touchThrottle) shouldTouch(hash string, now time.Time) bool { return true } +// principal returns the caller AuthMiddleware resolved for r as the shape +// the service layer takes. ok is false when the request carries no +// authenticated user; Session is nil for an API-token principal. +func principal(r *http.Request) (service.Principal, bool) { + user, ok := r.Context().Value(UserKey).(*db.User) + if !ok || user == nil { + return service.Principal{}, false + } + sess, _ := r.Context().Value(SessionKey).(*db.Session) + return service.Principal{User: user, Session: sess}, true +} + // AuthMiddleware reads the "Authorization: Bearer " header, validates // the session, and injects the user and session into the request context. // Returns 401 if the token is missing, invalid, or the session is expired. diff --git a/Server/api/profile_handler.go b/Server/api/profile_handler.go index 14f7c483..a9853bd8 100644 --- a/Server/api/profile_handler.go +++ b/Server/api/profile_handler.go @@ -23,6 +23,48 @@ import ( // ─── Request / Response types ──────────────────────────────────────────────── +// userResponse is the caller's own user record, the shape auth responses and +// PATCH /users/me return. It lives beside the profile handler because this is +// the file that still sees db.User; the auth handlers get it through +// toUserResponse without naming db (B3-2). +type userResponse struct { + ID int64 `json:"id"` + Username string `json:"username"` + Avatar string `json:"avatar,omitempty"` + // DisplayName and About are always present (null = unset) so the settings + // form can tell "cleared" from "the server does not know this field". + DisplayName *string `json:"display_name"` + About *string `json:"about"` + // CustomStatus is the user's own free-text status line. + CustomStatus *string `json:"custom_status"` + // Status is the user's own true status, invisible included. This response + // only ever describes the caller, so there is nothing to hide from them. + Status string `json:"status"` + RoleID int64 `json:"role_id"` + TOTPEnabled bool `json:"totp_enabled"` + CreatedAt string `json:"created_at"` +} + +// toUserResponse converts a db.User to the API response shape. +func toUserResponse(u *db.User) *userResponse { + avatar := "" + if u.Avatar != nil { + avatar = *u.Avatar + } + return &userResponse{ + ID: u.ID, + Username: u.Username, + Avatar: avatar, + DisplayName: u.DisplayName, + About: u.About, + CustomStatus: u.CustomStatus, + Status: u.Status, + RoleID: u.RoleID, + TOTPEnabled: u.TOTPSecret != nil, + CreatedAt: u.CreatedAt, + } +} + // updateProfileRequest is the JSON body for PATCH /api/v1/users/me. // identity_public_key, when present, publishes the client's long-term E2EE // identity public key (F3 voice E2EE TOFU); omitted = leave unchanged. @@ -392,8 +434,8 @@ func handleChangePassword(svc *service.Services, limiter *auth.RateLimiter) http // Verify old password using constant-time bcrypt comparison. failKey := auth.Key("pw_confirm_fail", user.ID) if !auth.CheckPassword(user.PasswordHash, req.OldPassword) { - if !limiter.Allow(failKey, pwConfirmFailureThreshold, pwConfirmFailureWindow) { - limiter.Lockout(r.Context(), lockKey, pwConfirmLockoutDuration) + if !limiter.Allow(failKey, service.PwConfirmFailureThreshold, service.PwConfirmFailureWindow) { + limiter.Lockout(r.Context(), lockKey, service.PwConfirmLockoutDuration) } writeJSON(w, http.StatusForbidden, errorResponse{ Error: "FORBIDDEN", Message: "incorrect password", diff --git a/Server/api/router.go b/Server/api/router.go index a39b5da9..dd877001 100644 --- a/Server/api/router.go +++ b/Server/api/router.go @@ -109,12 +109,11 @@ func NewRouter(cfg *config.Config, database *db.DB, ver string, logBuf *admin.Ri getOnlineUsers = func() int { return hub.ClientCount() } hubAlive = func() bool { return hub.DispatchAlive() } - // Auth routes: register, login, logout, me. Mounted with the hub as the - // AuthBroadcaster so DELETE /api/v1/auth/account (self-service account - // deletion) fans out member_ban and force-disconnects the deleted user's - // own socket, exactly like the admin ban path does for the same - // anonymise-and-ban DB state. - MountAuthRoutes(r, database, limiter, cfg.Server.TrustedProxies, totpKey, hub) + // Auth routes. The service is built after the hub, with the hub as its + // AuthBroadcaster, so DELETE /api/v1/auth/account fans out member_ban and + // force-disconnects the deleted user's own socket exactly like the admin + // ban path does for the same DB state. B3-3 moves this to internal/app. + MountAuthRoutes(r, service.NewAuthService(database, limiter, totpKey, hub), AuthMiddleware(database), limiter, cfg.Server.TrustedProxies) routerPluginWiring(hub, pluginRegistry) diff --git a/Server/api/router_delete_account_broadcast_test.go b/Server/api/router_delete_account_broadcast_test.go index 39e5e9fa..a831bd52 100644 --- a/Server/api/router_delete_account_broadcast_test.go +++ b/Server/api/router_delete_account_broadcast_test.go @@ -1,11 +1,11 @@ package api_test // router_delete_account_broadcast_test.go pins the production wiring for -// OC-0048: NewRouter (router.go) must pass the WS hub to MountAuthRoutes as -// its optional AuthBroadcaster so self-service account deletion fans out -// member_ban exactly like the admin ban path does. MountAuthRoutes is called -// before the hub exists in router.go, so the only production call site used -// to omit the broadcaster entirely — handleDeleteAccount's +// OC-0048: NewRouter (router.go) must hand the WS hub to +// service.NewAuthService as its AuthBroadcaster so self-service account +// deletion fans out member_ban exactly like the admin ban path does. Auth +// routes were once mounted before the hub existed in router.go, so the only +// production call site used to omit the broadcaster entirely — handleDeleteAccount's // `if broadcaster != nil` guard was never taken outside tests that construct // their own fake broadcaster (see auth_handler_delete_broadcast_test.go, // which only proves the handler itself works when a broadcaster IS passed). @@ -169,7 +169,7 @@ func TestNewRouter_DeleteAccount_BroadcastsMemberBanOverWS(t *testing.T) { if !sawMemberBan { t.Fatal("no member_ban WS broadcast for the deleted user observed on a second client — " + - "router.go's MountAuthRoutes call must pass the hub as the optional " + - "AuthBroadcaster (mount it after ws.NewHub, not before)") + "router.go must build service.NewAuthService with the hub as its " + + "AuthBroadcaster (after ws.NewHub, not before)") } } diff --git a/Server/api/totp_handler.go b/Server/api/totp_handler.go index 9c0fd1d4..8cbe17d2 100644 --- a/Server/api/totp_handler.go +++ b/Server/api/totp_handler.go @@ -1,17 +1,13 @@ package api import ( - "context" "encoding/json" "errors" "io" - "log/slog" "net/http" - "strings" - "time" "github.com/J3vb/OwnCord/Server/auth" - "github.com/J3vb/OwnCord/Server/db" + "github.com/J3vb/OwnCord/Server/service" ) // ─── TOTP request/response types ───────────────────────────────────────────── @@ -36,7 +32,9 @@ type totpEnableResponse struct { // ─── Handlers ──────────────────────────────────────────────────────────────── -func handleVerifyTOTP(database *db.DB, partialStore *auth.PartialAuthStore, limiter *auth.RateLimiter, usedTOTPCodes *auth.UsedTOTPCodeStore, totpKey []byte) http.HandlerFunc { +// handleVerifyTOTP processes POST /api/v1/auth/verify-totp: the bearer token +// is the partial-login challenge Login issued. +func handleVerifyTOTP(svc AuthService) http.HandlerFunc { return func(w http.ResponseWriter, r *http.Request) { partialToken, ok := auth.ExtractBearerToken(r) if !ok { @@ -47,15 +45,6 @@ func handleVerifyTOTP(database *db.DB, partialStore *auth.PartialAuthStore, limi return } - challenge, ok := partialStore.Lookup(partialToken) - if !ok { - writeJSON(w, http.StatusUnauthorized, errorResponse{ - Error: "UNAUTHORIZED", - Message: "invalid or expired two-factor challenge", - }) - return - } - var req verifyTotpRequest if err := json.NewDecoder(r.Body).Decode(&req); err != nil { writeJSON(w, http.StatusBadRequest, errorResponse{ @@ -65,137 +54,21 @@ func handleVerifyTOTP(database *db.DB, partialStore *auth.PartialAuthStore, limi return } - totpRateLimitKey := auth.Key("totp_fail", challenge.UserID) - // Atomically record this attempt and reject once the per-user failure cap - // is reached. Recording up-front — rather than a read-only Check now and - // Allow only on failure — closes a TOCTOU where many concurrent requests - // reusing one valid partial token all pass the read-only check before any - // failure is recorded, defeating the per-user brute-force cap (the only - // cross-IP defence). A successful verification resets the counter below, - // so legitimate retries are not penalised. - // Deliberately NOT scaledAuthLimit: this cap is keyed per USER, and it - // is the only cross-IP brute-force defence on TOTP codes. The - // multiplier exists for shared-NAT per-IP limits; scaling a per-user - // threshold with it would hand a distributed attacker more guesses. - // Mirrors loginUserFailureThreshold staying unscaled in auth_handler. - if !limiter.Allow(totpRateLimitKey, totpFailureRateLimit, totpFailureWindow) { - writeJSON(w, http.StatusTooManyRequests, errorResponse{ - Error: "RATE_LIMITED", - Message: "too many failed attempts, try again later", - }) - return - } - - user, secret, ok := totpChallengeSecret(w, r, database, totpKey, challenge.UserID) - if !ok { - return - } - - if !auth.VerifyTOTPCodeOnce(secret, strings.TrimSpace(req.Code), time.Now().UTC(), user.ID, usedTOTPCodes) { - // The attempt was already recorded atomically up-front via - // limiter.Allow; only the per-partial-token counter is advanced here. - partialStore.RegisterFailure(partialToken, partialAuthMaxFailures) - writeJSON(w, http.StatusUnauthorized, errorResponse{ - Error: "UNAUTHORIZED", - Message: "invalid two-factor code", - }) - return - } - - limiter.Reset(r.Context(), totpRateLimitKey) - - if _, ok := partialStore.Consume(partialToken); !ok { - writeJSON(w, http.StatusUnauthorized, errorResponse{ - Error: "UNAUTHORIZED", - Message: "invalid or expired two-factor challenge", - }) - return - } - - token, err := issueSession(r.Context(), database, user.ID, challenge.Device, challenge.IP) + res, err := svc.VerifyTOTP(r.Context(), partialToken, req.Code) if err != nil { - writeJSON(w, http.StatusInternalServerError, errorResponse{ - Error: "INTERNAL_ERROR", - Message: "failed to create session", - }) + writeAuthError(r.Context(), w, err) return } - - slog.Info("totp verified", "user_id", user.ID, "ip", challenge.IP) - db.WriteAudit(context.WithoutCancel(r.Context()), database, user.ID, "totp_verified", "user", user.ID, - "two-factor verification completed from "+challenge.IP) - - writeJSON(w, http.StatusOK, authSuccessResponse{ - Token: token, - Requires2FA: false, - User: toUserResponse(user), - }) + writeJSON(w, http.StatusOK, authResponse(res)) } } -// totpChallengeSecret resolves the user behind a partial-auth challenge and -// returns their decrypted TOTP secret. It writes its own refusal, so a false -// third result means the response is already complete. -func totpChallengeSecret(w http.ResponseWriter, r *http.Request, database *db.DB, totpKey []byte, challengeUserID int64) (*db.User, string, bool) { - user, err := database.GetUserByID(r.Context(), challengeUserID) - if err != nil || user == nil || user.TOTPSecret == nil { - writeJSON(w, http.StatusUnauthorized, errorResponse{ - Error: "UNAUTHORIZED", - Message: "invalid or expired two-factor challenge", - }) - return nil, "", false - } - - // A ban can land inside the partial-token window; the login path - // refuses banned users right after the password compare, so the - // second factor must refuse them too. - if auth.IsEffectivelyBanned(user) { - writeJSON(w, http.StatusForbidden, errorResponse{ - Error: "FORBIDDEN", - Message: "your account has been suspended", - }) - return nil, "", false - } - - secret, decErr := auth.DecryptTOTPSecret(totpKey, *user.TOTPSecret) - if decErr != nil { - slog.Error("failed to decrypt TOTP secret", "user_id", user.ID, "error", decErr) - writeJSON(w, http.StatusInternalServerError, errorResponse{ - Error: "INTERNAL_ERROR", - Message: "failed to verify two-factor code", - }) - return nil, "", false - } - - return user, secret, true -} - -func handleEnableTOTP(pendingStore *auth.PendingTOTPStore, limiter *auth.RateLimiter) http.HandlerFunc { +// handleEnableTOTP processes POST /api/v1/users/me/totp/enable. +func handleEnableTOTP(svc AuthService) http.HandlerFunc { return func(w http.ResponseWriter, r *http.Request) { - user, ok := r.Context().Value(UserKey).(*db.User) - if !ok || user == nil { - writeJSON(w, http.StatusUnauthorized, errorResponse{ - Error: "UNAUTHORIZED", - Message: "not authenticated", - }) - return - } - - // BUG-111: Per-user lockout for password confirmation. - lockKey := auth.Key("pw_confirm_lock", user.ID) - if limiter.IsLockedOut(lockKey) { - writeJSON(w, http.StatusTooManyRequests, errorResponse{ - Error: "RATE_LIMITED", - Message: "too many failed attempts, try again later", - }) - return - } - - if user.TOTPSecret != nil && *user.TOTPSecret != "" { - writeJSON(w, http.StatusConflict, errorResponse{ - Error: "TOTP_ALREADY_ENABLED", - Message: "disable 2FA before re-enabling", - }) + p, ok := principal(r) + if !ok { + writeNotAuthenticated(w) return } @@ -207,78 +80,25 @@ func handleEnableTOTP(pendingStore *auth.PendingTOTPStore, limiter *auth.RateLim }) return } - failKey := auth.Key("pw_confirm_fail", user.ID) - if err := requirePasswordConfirmation(user, req.Password); err != nil { - if !limiter.Allow(failKey, pwConfirmFailureThreshold, pwConfirmFailureWindow) { - limiter.Lockout(r.Context(), lockKey, pwConfirmLockoutDuration) - } - writeJSON(w, http.StatusBadRequest, errorResponse{ - Error: "INVALID_INPUT", - Message: err.Error(), - }) - return - } - limiter.Reset(r.Context(), failKey) - secret, err := auth.GenerateTOTPSecret() + qrURI, err := svc.EnableTOTP(r.Context(), p, req.Password) if err != nil { - writeJSON(w, http.StatusInternalServerError, errorResponse{ - Error: "INTERNAL_ERROR", - Message: "failed to generate two-factor secret", - }) + writeAuthError(r.Context(), w, err) return } - - pendingStore.Put(user.ID, secret) writeJSON(w, http.StatusOK, totpEnableResponse{ - QRURI: auth.BuildTOTPURI(user.Username, secret, "OwnCord"), + QRURI: qrURI, BackupCodes: []string{}, }) } } -// revokeOtherSessionsAfterAuthChange revokes every session for userID except -// keepSessionID as the security tail of a committed 2FA state change. It -// mirrors UserService.ChangePassword (service/user.go:262-274): a failure is -// logged and retried once (bounded compensating retry for transient write -// contention); if the retry also fails, revoked reports what did succeed and -// failed is true so the caller can report a partial success instead of -// silently claiming the other sessions were revoked when they were not. -func revokeOtherSessionsAfterAuthChange(ctx context.Context, database *db.DB, userID, keepSessionID int64, action string) (revoked int64, failed bool) { - revoked, err := database.DeleteOtherSessions(ctx, userID, keepSessionID) - if err != nil { - slog.Error("DeleteOtherSessions after "+action, "err", err, "user_id", userID) - revokedRetry, retryErr := database.DeleteOtherSessions(ctx, userID, keepSessionID) - if retryErr != nil { - slog.Error("DeleteOtherSessions retry after "+action, "err", retryErr, "user_id", userID) - return revoked, true - } - revoked += revokedRetry - } - if revoked > 0 { - slog.Info("revoked other sessions after "+action, "user_id", userID, "revoked", revoked) - } - return revoked, false -} - -func handleConfirmTOTP(database *db.DB, pendingStore *auth.PendingTOTPStore, usedTOTPCodes *auth.UsedTOTPCodeStore, limiter *auth.RateLimiter, totpKey []byte) http.HandlerFunc { +// handleConfirmTOTP processes POST /api/v1/users/me/totp/confirm. +func handleConfirmTOTP(svc AuthService) http.HandlerFunc { return func(w http.ResponseWriter, r *http.Request) { - user, ok := r.Context().Value(UserKey).(*db.User) - if !ok || user == nil { - writeJSON(w, http.StatusUnauthorized, errorResponse{ - Error: "UNAUTHORIZED", - Message: "not authenticated", - }) - return - } - - // BUG-111: Per-user lockout for password confirmation. - lockKey := auth.Key("pw_confirm_lock", user.ID) - if limiter.IsLockedOut(lockKey) { - writeJSON(w, http.StatusTooManyRequests, errorResponse{ - Error: "RATE_LIMITED", - Message: "too many failed attempts, try again later", - }) + p, ok := principal(r) + if !ok { + writeNotAuthenticated(w) return } @@ -290,107 +110,24 @@ func handleConfirmTOTP(database *db.DB, pendingStore *auth.PendingTOTPStore, use }) return } - failKey := auth.Key("pw_confirm_fail", user.ID) - if err := requirePasswordConfirmation(user, req.Password); err != nil { - if !limiter.Allow(failKey, pwConfirmFailureThreshold, pwConfirmFailureWindow) { - limiter.Lockout(r.Context(), lockKey, pwConfirmLockoutDuration) - } - writeJSON(w, http.StatusBadRequest, errorResponse{ - Error: "INVALID_INPUT", - Message: err.Error(), - }) + + res, err := svc.ConfirmTOTP(r.Context(), p, req.Password, req.Code) + if err != nil { + writeAuthError(r.Context(), w, err) return } - limiter.Reset(r.Context(), failKey) - - secret, ok := pendingStore.Lookup(user.ID) - if !ok { - writeJSON(w, http.StatusBadRequest, errorResponse{ - Error: "BAD_REQUEST", - Message: "no pending two-factor enrollment found", - }) - return - } - - if !auth.VerifyTOTPCodeOnce(secret, strings.TrimSpace(req.Code), time.Now().UTC(), user.ID, usedTOTPCodes) { - writeJSON(w, http.StatusUnauthorized, errorResponse{ - Error: "UNAUTHORIZED", - Message: "invalid two-factor code", - }) - return - } - - encryptedSecret, encErr := auth.EncryptTOTPSecret(totpKey, secret) - if encErr != nil { - slog.Error("failed to encrypt TOTP secret", "user_id", user.ID, "error", encErr) - writeJSON(w, http.StatusInternalServerError, errorResponse{ - Error: "INTERNAL_ERROR", - Message: "failed to enable two-factor authentication", - }) - return - } - - if err := database.UpdateUserTOTPSecret(r.Context(), user.ID, &encryptedSecret); err != nil { - writeJSON(w, http.StatusInternalServerError, errorResponse{ - Error: "INTERNAL_ERROR", - Message: "failed to enable two-factor authentication", - }) - return - } - pendingStore.Delete(user.ID) - - // BUG-108: Revoke all other sessions after 2FA state change. An - // API-token principal has a nil session; keep=0 matches no row, so - // every login session is revoked — same semantics as change-password. - sess, _ := r.Context().Value(SessionKey).(*db.Session) - keepSessionID := int64(0) - if sess != nil { - keepSessionID = sess.ID - } - // Security tail of the 2FA change: once the secret update committed, - // revoking the other sessions must not be aborted by a dead request. - tailCtx := context.WithoutCancel(r.Context()) - revoked, revokeFailed := revokeOtherSessionsAfterAuthChange(tailCtx, database, user.ID, keepSessionID, "totp enable") - - slog.Info("totp enabled", "user_id", user.ID) - db.WriteAudit(tailCtx, database, user.ID, "totp_enabled", "user", user.ID, - "two-factor authentication enrolled") - - if revokeFailed { - // Partial success: 2FA IS enabled; only revoking the other - // sessions failed. A 5xx here would be a lie — the state change - // already committed — so mirror the ChangePassword contract - // (api/profile_handler.go) and report 200 with an explicit warning - // instead of a silent, unqualified 204. - writeJSON(w, http.StatusOK, map[string]any{ - "warning": "two-factor authentication enabled, but other sessions could not be revoked; revoke them from the sessions list", - "sessions_revoked": revoked, - }) - return - } - - w.WriteHeader(http.StatusNoContent) + writeTOTPChange(w, res) } } -func handleDisableTOTP(database *db.DB, pendingStore *auth.PendingTOTPStore, limiter *auth.RateLimiter) http.HandlerFunc { +// handleDisableTOTP processes DELETE /api/v1/users/me/totp. An empty body is +// accepted (and then refused by the service as a missing password); only a +// body that is present and not JSON is malformed. +func handleDisableTOTP(svc AuthService) http.HandlerFunc { return func(w http.ResponseWriter, r *http.Request) { - user, ok := r.Context().Value(UserKey).(*db.User) - if !ok || user == nil { - writeJSON(w, http.StatusUnauthorized, errorResponse{ - Error: "UNAUTHORIZED", - Message: "not authenticated", - }) - return - } - - // BUG-111: Per-user lockout for password confirmation. - lockKey := auth.Key("pw_confirm_lock", user.ID) - if limiter.IsLockedOut(lockKey) { - writeJSON(w, http.StatusTooManyRequests, errorResponse{ - Error: "RATE_LIMITED", - Message: "too many failed attempts, try again later", - }) + p, ok := principal(r) + if !ok { + writeNotAuthenticated(w) return } @@ -402,74 +139,27 @@ func handleDisableTOTP(database *db.DB, pendingStore *auth.PendingTOTPStore, lim }) return } - failKey := auth.Key("pw_confirm_fail", user.ID) - if err := requirePasswordConfirmation(user, req.Password); err != nil { - if !limiter.Allow(failKey, pwConfirmFailureThreshold, pwConfirmFailureWindow) { - limiter.Lockout(r.Context(), lockKey, pwConfirmLockoutDuration) - } - writeJSON(w, http.StatusBadRequest, errorResponse{ - Error: "INVALID_INPUT", - Message: err.Error(), - }) - return - } - limiter.Reset(r.Context(), failKey) - require2FA, err := isRequire2FAEnabled(r.Context(), database) + res, err := svc.DisableTOTP(r.Context(), p, req.Password) if err != nil { - writeJSON(w, http.StatusInternalServerError, errorResponse{ - Error: "INTERNAL_ERROR", - Message: "failed to load authentication policy", - }) + writeAuthError(r.Context(), w, err) return } - if require2FA { - writeJSON(w, http.StatusForbidden, errorResponse{ - Error: "FORBIDDEN", - Message: "two-factor authentication is required for this server", - }) - return - } - - pendingStore.Delete(user.ID) - if err := database.UpdateUserTOTPSecret(r.Context(), user.ID, nil); err != nil { - writeJSON(w, http.StatusInternalServerError, errorResponse{ - Error: "INTERNAL_ERROR", - Message: "failed to disable two-factor authentication", - }) - return - } - - // BUG-108: Revoke all other sessions after 2FA state change. An - // API-token principal has a nil session; keep=0 matches no row, so - // every login session is revoked — same semantics as change-password. - sess, _ := r.Context().Value(SessionKey).(*db.Session) - keepSessionID := int64(0) - if sess != nil { - keepSessionID = sess.ID - } - // Security tail of the 2FA change: once the secret update committed, - // revoking the other sessions must not be aborted by a dead request. - tailCtx := context.WithoutCancel(r.Context()) - revoked, revokeFailed := revokeOtherSessionsAfterAuthChange(tailCtx, database, user.ID, keepSessionID, "totp disable") - - slog.Info("totp disabled", "user_id", user.ID) - db.WriteAudit(tailCtx, database, user.ID, "totp_disabled", "user", user.ID, - "two-factor authentication disabled") - - if revokeFailed { - // Partial success: 2FA IS disabled; only revoking the other - // sessions failed. A 5xx here would be a lie — the state change - // already committed — so mirror the ChangePassword contract - // (api/profile_handler.go) and report 200 with an explicit warning - // instead of a silent, unqualified 204. - writeJSON(w, http.StatusOK, map[string]any{ - "warning": "two-factor authentication disabled, but other sessions could not be revoked; revoke them from the sessions list", - "sessions_revoked": revoked, - }) - return - } - - w.WriteHeader(http.StatusNoContent) + writeTOTPChange(w, res) } } + +// writeTOTPChange answers a committed 2FA change: 204, or 200 with the +// warning when the caller's other sessions could not be revoked — a partial +// success the service reports instead of a 5xx, because the change is +// already durable. +func writeTOTPChange(w http.ResponseWriter, res *service.TOTPChangeResult) { + if res.Warning != "" { + writeJSON(w, http.StatusOK, map[string]any{ + "warning": res.Warning, + "sessions_revoked": res.SessionsRevoked, + }) + return + } + w.WriteHeader(http.StatusNoContent) +} diff --git a/Server/auth/ratescale.go b/Server/auth/ratescale.go new file mode 100644 index 00000000..d1283761 --- /dev/null +++ b/Server/auth/ratescale.go @@ -0,0 +1,39 @@ +package auth + +import ( + "math" + "sync/atomic" +) + +// rateScaleBits holds security.auth_rate_limit_multiplier as float bits. It +// scales the per-IP auth request caps and failure thresholds for deployments +// where many users share one IP (office/school NAT) — the compiled-in limits +// assume roughly one person per address. Atomic because tests construct +// multiple routers concurrently. Installed by api.NewRouter via SetRateScale; +// read at route-mount time and on the login failure-count path +// (service.AuthService). +var rateScaleBits atomic.Uint64 + +func init() { rateScaleBits.Store(math.Float64bits(1.0)) } + +// SetRateScale clamps and installs the auth rate multiplier. Zero or +// negative (unset config) means 1.0. +func SetRateScale(m float64) { + if m <= 0 { + m = 1.0 + } + m = math.Min(math.Max(m, 0.1), 100) + rateScaleBits.Store(math.Float64bits(m)) +} + +// ScaledLimit applies the auth rate multiplier to a compiled-in limit, never +// returning less than 1. Per-user caps must not go through it: they are the +// only cross-IP brute-force defence, and the multiplier exists for shared-NAT +// per-IP limits. +func ScaledLimit(n int) int { + scaled := int(math.Round(float64(n) * math.Float64frombits(rateScaleBits.Load()))) + if scaled < 1 { + return 1 + } + return scaled +} diff --git a/Server/invariants/db_import_boundary.go b/Server/invariants/db_import_boundary.go index d9650b4c..60c38a50 100644 --- a/Server/invariants/db_import_boundary.go +++ b/Server/invariants/db_import_boundary.go @@ -56,7 +56,6 @@ var DBImportAllow = map[string]DBImportEntry{ "admin/setup_wizard.go": {"move", "auth", "BeginTx for the wizard; setup sub-family"}, "admin/types.go": {"adapter", "", "response DTOs; the one GetRoleByID moves with handlers_users"}, // ── api ─────────────────────────────────────────────────────────────── - "api/auth_handler.go": {"move", "auth", "B3-2 slice: register/login/logout/delete own the DB"}, "api/channel_handler.go": {"adapter", "", "response types only; service owns the calls"}, "api/dm_handler.go": {"adapter", "", "DM response types + pure status helpers"}, "api/emoji_handler.go": {"adapter", "", "Emoji/User types only"}, @@ -66,7 +65,6 @@ var DBImportAllow = map[string]DBImportEntry{ "api/plugins_handler.go": {"adapter", "", "db.Auditor is the seam; WriteAudit only"}, "api/profile_handler.go": {"move", "upload", "avatar upload creates the attachment row"}, "api/router.go": {"boundary", "", "health probe (PingRead, SQLDb); hub construction leaves in B3-3"}, - "api/totp_handler.go": {"move", "auth", "B3-2 slice: TOTP enrol/verify write the user row"}, "api/upload_handler.go": {"move", "upload", "attachment access + a raw QueryRowContext"}, // ── auth ────────────────────────────────────────────────────────────── "auth/helpers.go": {"adapter", "", "db.User type in a helper signature"}, diff --git a/Server/invariants/db_import_boundary_test.go b/Server/invariants/db_import_boundary_test.go index 71a1f837..f887b1d3 100644 --- a/Server/invariants/db_import_boundary_test.go +++ b/Server/invariants/db_import_boundary_test.go @@ -30,7 +30,7 @@ func TestDBImportBoundary(t *testing.T) { }, { name: "listed file is allowed", - path: "api/auth_handler.go", + path: "api/middleware.go", src: "package api\n" + importDB + "\nvar _ *db.DB\n", want: 0, }, diff --git a/Server/service/auth.go b/Server/service/auth.go new file mode 100644 index 00000000..2da0c745 --- /dev/null +++ b/Server/service/auth.go @@ -0,0 +1,854 @@ +package service + +import ( + "context" + "errors" + "fmt" + "log/slog" + "strings" + "time" + + "github.com/J3vb/OwnCord/Server/auth" + "github.com/J3vb/OwnCord/Server/db" + "github.com/J3vb/OwnCord/Server/permissions" +) + +// ─── Collaborators and shapes ──────────────────────────────────────────────── + +// AuthBroadcaster is how the auth slice tells connected WebSocket clients that +// an account is gone. Satisfied by *ws.Hub, which already implements +// BroadcastMemberBan for the admin ban path that self-deletion mirrors. A nil +// broadcaster sends nothing and other clients converge on their next +// reconnect. +type AuthBroadcaster interface { + BroadcastMemberBan(userID int64) +} + +// Principal is the authenticated caller api.AuthMiddleware resolved for a +// request. Session is nil for an API-token principal. +type Principal struct { + User *db.User + Session *db.Session +} + +// RegisterInput is a validated registration: the transport has already +// trimmed, sanitized and format-checked Username, checked Password strength +// and trimmed InviteCode. Device and IP describe the request that will own +// the issued session. +type RegisterInput struct { + Username string + Password string + InviteCode string + Device string + IP string +} + +// LoginInput is one login attempt. Username is trimmed; Password is not — +// passwords may carry leading or trailing whitespace on purpose. +type LoginInput struct { + Username string + Password string + Device string + IP string +} + +// AuthResult is a successful register, login or second-factor step. Either a +// session was issued (Token and User) or a two-factor challenge was started +// (PartialToken with Requires2FA) — never both. +type AuthResult struct { + Token string + PartialToken string + Requires2FA bool + User *db.User +} + +// TOTPChangeResult reports a committed 2FA enable or disable. Warning is +// non-empty when the state change committed but revoking the caller's other +// sessions failed: a partial success the transport must answer with 200 and +// the warning, never a 5xx, because the change is already durable. +type TOTPChangeResult struct { + SessionsRevoked int64 + Warning string +} + +// ─── Limits ────────────────────────────────────────────────────────────────── +// +// Moved from api/constants.go with the orchestration that reads them (B3-2). + +const ( + // loginFailureThreshold is the number of failed login attempts (within + // loginFailureWindow) before the IP is locked out. + loginFailureThreshold = 9 + + // loginFailureWindow is the sliding window for counting login failures. + loginFailureWindow = 15 * time.Minute + + // loginLockoutDuration is how long an IP is locked out after exceeding + // loginFailureThreshold. + loginLockoutDuration = 15 * time.Minute + + // loginUserFailureThreshold is the number of failed login attempts for a + // specific username (regardless of source IP) before the account is locked. + loginUserFailureThreshold = 9 + + // loginUserFailureWindow is the sliding window for per-username login failures. + loginUserFailureWindow = 15 * time.Minute + + // loginUserLockoutDuration is how long a username is locked after exceeding + // loginUserFailureThreshold. + loginUserLockoutDuration = 15 * time.Minute + + // deleteAccountFailureThreshold is the number of wrong-password attempts + // before the per-user lockout kicks in. + deleteAccountFailureThreshold = 3 + + // deleteAccountFailureWindow is the sliding window for counting + // delete-account password failures. + deleteAccountFailureWindow = 15 * time.Minute + + // deleteAccountLockoutDuration is how long the account-deletion endpoint + // is locked after exceeding deleteAccountFailureThreshold. + deleteAccountLockoutDuration = 15 * time.Minute + + // totpFailureRateLimit is the maximum TOTP verification failures per user + // within totpFailureWindow before the user is rate-limited. + totpFailureRateLimit = 10 + + // totpFailureWindow is the sliding window for counting per-user TOTP failures. + totpFailureWindow = 15 * time.Minute + + // partialAuthMaxFailures is the number of failed TOTP attempts on a single + // partial-auth challenge before it is revoked. + partialAuthMaxFailures = 5 + + // partialAuthStoreTTL is the lifetime of a partial-auth (2FA) challenge token. + partialAuthStoreTTL = 10 * time.Minute + + // pendingTOTPStoreTTL is the lifetime of a pending TOTP enrollment secret. + pendingTOTPStoreTTL = 10 * time.Minute +) + +// The password-confirmation lockout is shared with the change-password route +// (api/profile_handler.go, the user family in B3-8), so one key space +// ("pw_confirm_fail", "pw_confirm_lock") and one budget cover every route +// that asks for the current password. +const ( + // PwConfirmFailureThreshold is the number of wrong-password attempts on + // password-confirmation endpoints before per-user lockout kicks in. + PwConfirmFailureThreshold = 3 + + // PwConfirmFailureWindow is the sliding window for per-user password + // confirmation failures. + PwConfirmFailureWindow = 15 * time.Minute + + // PwConfirmLockoutDuration is how long password-confirmation endpoints are + // locked after exceeding PwConfirmFailureThreshold. + PwConfirmLockoutDuration = 15 * time.Minute +) + +// ─── Errors ────────────────────────────────────────────────────────────────── + +// Category sentinels the transport maps to a status and code. ErrRateLimited, +// ErrForbidden, ErrBadRequest, ErrConflict and ErrInternal are the shared set +// in message.go; these two are what auth adds. +var ( + // ErrUnauthorized is a credential or challenge that does not authenticate + // (401). Deliberately generic: the enumeration guard depends on every + // failed login looking the same. + ErrUnauthorized = errors.New("unauthorized") + // ErrInvalidInput is a request the auth routes refuse as INVALID_INPUT + // (400) — the password-confirmation refusals. + ErrInvalidInput = errors.New("invalid input") +) + +// authError is one refusal the auth slice can return. Error() is the exact +// public message the pre-B3-2 handler wrote — B3-1's characterization rows +// pin it byte for byte — and Is reports the category sentinel the transport +// maps to a status and code, so errors.Is matches both the named value and +// its category. +type authError struct { + kind error + msg string +} + +func (e *authError) Error() string { return e.msg } +func (e *authError) Is(target error) bool { return target == e.kind } + +// Every refusal below is returned bare, never wrapped around the cause: the +// transport echoes Error() to the client, and the cause (a database error, a +// decrypt failure) is logged here instead. +var ( + // Registration. + ErrRegistrationPolicyUnavailable = &authError{ErrInternal, "failed to load registration policy"} + ErrRegistrationClosed = &authError{ErrForbidden, "registration is currently closed"} + ErrRegistrationRequires2FA = &authError{ErrForbidden, "registration is unavailable while two-factor authentication is required"} + ErrPasswordHash = &authError{ErrInternal, "failed to process registration"} + // ErrRegistrationRejected is the generic register refusal: an unknown, + // used-up or expired invite and a taken username share it so the response + // reveals neither. The transport writes it as 400 INVALID_CREDENTIALS. + ErrRegistrationRejected = &authError{ErrBadRequest, "invalid invite or credentials"} + ErrRegistrationFailed = &authError{ErrInternal, "registration failed — please try again"} + ErrSessionIssue = &authError{ErrInternal, "failed to create session"} + ErrRegisteredUserFetch = &authError{ErrInternal, "registration succeeded but user fetch failed"} + + // Login. + ErrLockedOut = &authError{ErrRateLimited, "account temporarily locked due to too many failed attempts"} + ErrLoginUnavailable = &authError{ErrInternal, "login temporarily unavailable"} + ErrInvalidCredentials = &authError{ErrUnauthorized, "invalid credentials"} + ErrBanned = &authError{ErrForbidden, "your account has been suspended"} + ErrAuthPolicyUnavailable = &authError{ErrInternal, "failed to load authentication policy"} + ErrTOTPChallengeStart = &authError{ErrInternal, "failed to start two-factor challenge"} + ErrRequire2FA = &authError{ErrForbidden, "two-factor authentication must be enabled on this account before login"} + + // Logout. + ErrLogoutFailed = &authError{ErrInternal, "failed to logout"} + + // Password confirmation (account deletion and TOTP management). + ErrTooManyAttempts = &authError{ErrRateLimited, "too many failed attempts, try again later"} + ErrPasswordRequired = &authError{ErrInvalidInput, "password is required"} + ErrIncorrectPassword = &authError{ErrInvalidInput, "incorrect password"} + ErrPasswordConfirmationFailed = &authError{ErrInvalidInput, "password confirmation failed"} + + // Account deletion. + ErrLastAdmin = &authError{ErrForbidden, "cannot delete the last admin account"} + ErrDeleteAccountFailed = &authError{ErrInternal, "failed to delete account"} + + // Second factor. + ErrTOTPChallengeInvalid = &authError{ErrUnauthorized, "invalid or expired two-factor challenge"} + ErrTOTPSecretUnreadable = &authError{ErrInternal, "failed to verify two-factor code"} + ErrTOTPCodeInvalid = &authError{ErrUnauthorized, "invalid two-factor code"} + // ErrTOTPAlreadyEnabled is written by the transport as 409 + // TOTP_ALREADY_ENABLED, not the generic CONFLICT code. + ErrTOTPAlreadyEnabled = &authError{ErrConflict, "disable 2FA before re-enabling"} + ErrTOTPSecretGenerate = &authError{ErrInternal, "failed to generate two-factor secret"} + ErrNoPendingTOTP = &authError{ErrBadRequest, "no pending two-factor enrollment found"} + ErrTOTPEnableFailed = &authError{ErrInternal, "failed to enable two-factor authentication"} + ErrTOTPRequiredByServer = &authError{ErrForbidden, "two-factor authentication is required for this server"} + ErrTOTPDisableFailed = &authError{ErrInternal, "failed to disable two-factor authentication"} +) + +// ─── Service ───────────────────────────────────────────────────────────────── + +// AuthService owns the auth slice's orchestration: the lockout and +// enumeration guards, the password and second-factor checks, session issue +// and revoke, the audit writes and the member_ban broadcast on self-deletion. +// Persistence stays in db behind Store. B3-2 moved every line here verbatim +// from api/auth_handler.go and api/totp_handler.go at 71d867cb; B3-1's +// characterization rows pin the behaviour. +type AuthService struct { + st Store + limiter *auth.RateLimiter + partial *auth.PartialAuthStore + pending *auth.PendingTOTPStore + usedCodes *auth.UsedTOTPCodeStore + totpKey []byte + broadcaster AuthBroadcaster +} + +// NewAuthService wires the auth slice. limiter is the shared auth rate +// limiter (lockouts are keyed inside it); totpKey is the AES-256 key that +// encrypts TOTP secrets at rest; broadcaster may be nil. The three in-memory +// stores — partial-login challenges, pending TOTP enrolments, used TOTP +// codes — are created here with the fixed TTLs the route mount used to own. +func NewAuthService(st Store, limiter *auth.RateLimiter, totpKey []byte, broadcaster AuthBroadcaster) *AuthService { + return &AuthService{ + st: st, + limiter: limiter, + partial: auth.NewPartialAuthStore(partialAuthStoreTTL), + pending: auth.NewPendingTOTPStore(pendingTOTPStoreTTL), + usedCodes: auth.NewUsedTOTPCodeStore(), + totpKey: totpKey, + broadcaster: broadcaster, + } +} + +// RegistrationPolicy reports whether registration is currently permitted. It +// runs before the transport reads any credential: a closed server refuses +// even a malformed body with the policy's 403. +func (s *AuthService) RegistrationPolicy(ctx context.Context) error { + registrationOpen, err := s.registrationOpen(ctx) + if err != nil { + return ErrRegistrationPolicyUnavailable + } + if !registrationOpen { + return ErrRegistrationClosed + } + + require2FA, err := s.require2FAEnabled(ctx) + if err != nil { + return ErrRegistrationPolicyUnavailable + } + if require2FA { + return ErrRegistrationRequires2FA + } + return nil +} + +// Register consumes the invite, creates the account and issues a session. +func (s *AuthService) Register(ctx context.Context, in RegisterInput) (*AuthResult, error) { + // Hash password before consuming the invite so that a hashing failure + // does not burn a valid invite code. + hash, err := auth.HashPassword(in.Password) + if err != nil { + return nil, ErrPasswordHash + } + + // Atomically consume the invite and create the user so failed + // registrations do not burn a valid invite code. + uid, err := s.st.CreateUserWithInvite(ctx, in.Username, hash, int(permissions.MemberRoleID), in.InviteCode) + if err != nil { + // UNIQUE constraint violation → duplicate username → 400. + // Any other DB error → 500. + switch { + case db.IsUniqueConstraintError(err): + return nil, ErrRegistrationRejected + case errors.Is(err, db.ErrNotFound): + return nil, ErrRegistrationRejected + default: + slog.Error("CreateUserWithInvite failed", "err", err, "username", in.Username) + return nil, ErrRegistrationFailed + } + } + + slog.Info("user registered", "username", in.Username, "user_id", uid, "ip", in.IP) + db.WriteAudit(context.WithoutCancel(ctx), s.st, uid, "user_register", "user", uid, + "new account created via invite") + + // Issue session. + token, err := issueSession(ctx, s.st, uid, in.Device, in.IP) + if err != nil { + return nil, ErrSessionIssue + } + + user, err := s.st.GetUserByID(ctx, uid) + if err != nil || user == nil { + slog.Error("failed to fetch user after registration", "user_id", uid, "error", err) + return nil, ErrRegisteredUserFetch + } + return &AuthResult{Token: token, User: user}, nil +} + +// Login runs the lockout gates and the constant-time password check, then +// issues a session or, for an enrolled account, starts a two-factor +// challenge. +func (s *AuthService) Login(ctx context.Context, in LoginInput) (*AuthResult, error) { + user, err := s.authenticate(ctx, in) + if err != nil { + return nil, err + } + + if auth.IsEffectivelyBanned(user) { + slog.Warn("banned user login attempt", "username", user.Username, "user_id", user.ID, "ip", in.IP) + db.WriteAudit(context.WithoutCancel(ctx), s.st, user.ID, "login_blocked_banned", "user", user.ID, + "banned user attempted login from "+in.IP) + return nil, ErrBanned + } + + require2FA, err := s.require2FAEnabled(ctx) + if err != nil { + return nil, ErrAuthPolicyUnavailable + } + if user.TOTPSecret != nil { + partialToken, err := s.partial.Issue(user.ID, in.Device, in.IP) + if err != nil { + return nil, ErrTOTPChallengeStart + } + return &AuthResult{PartialToken: partialToken, Requires2FA: true}, nil + } + if require2FA { + return nil, ErrRequire2FA + } + + // Issue session. + token, err := issueSession(ctx, s.st, user.ID, in.Device, in.IP) + if err != nil { + return nil, ErrSessionIssue + } + + // Don't set status to "online" here — the WebSocket connection in + // serve.go does that when the user actually connects. Setting it here + // would leave the user permanently "online" if they never open a WS + // connection or if the client crashes before connecting. + slog.Info("user logged in", "username", user.Username, "user_id", user.ID, "ip", in.IP) + db.WriteAudit(context.WithoutCancel(ctx), s.st, user.ID, "user_login", "user", user.ID, + "logged in from "+in.IP) + return &AuthResult{Token: token, User: user}, nil +} + +// authenticate runs the lockout gates, the constant-time password compare and +// the failure accounting for one login attempt, returning the authenticated +// user or the refusal. +func (s *AuthService) authenticate(ctx context.Context, in LoginInput) (*db.User, error) { + // Check per-IP lockout first. + lockKey := "login_lock:" + in.IP + if s.limiter.IsLockedOut(lockKey) { + return nil, ErrLockedOut + } + + // BUG-110: Also check per-username lockout to prevent distributed brute force. + // F1: canonicalize the username the same way GetUserByUsername does (COLLATE + // NOCASE) before keying the lockout, so case variants of one account + // (admin/Admin/ADMIN) share a single bucket instead of each getting its own. + unameKey := strings.ToLower(in.Username) + userLockKey := "login_user_lock:" + unameKey + if s.limiter.IsLockedOut(userLockKey) { + return nil, ErrLockedOut + } + + // Constant-time lookup: always attempt bcrypt compare even when user + // does not exist to prevent timing-based username enumeration. + user, err := s.st.GetUserByUsername(ctx, in.Username) + + // Distinguish DB errors from authentication failures. DB errors + // should NOT increment the rate limiter — otherwise a transient + // DB outage would lock out legitimate users. + if err != nil && user == nil { + // Could be a real DB error or simply "user not found". + // GetUserByUsername returns (nil, nil) for not-found, so a + // non-nil error here is a genuine DB failure. + slog.Error("login: GetUserByUsername failed", "err", err, "ip", in.IP) + return nil, ErrLoginUnavailable + } + + failKey := "login_fail:" + in.IP + userFailKey := "login_user_fail:" + unameKey + // F3: atomically reserve this attempt BEFORE the bcrypt compare. The + // read-only IsLockedOut gates above are check-then-act: N concurrent + // requests all pass them before any failure is recorded below, so the + // per-username cap — the only cross-IP brute-force defence — bound + // only sequential attackers. Allow records the attempt under the + // limiter's lock, capping a concurrent burst at the same budget a + // sequential attacker gets. Sized at threshold+1 so the sequential + // accepted-input set is unchanged: failures 1–10 still land, the 10th + // still trips the lockout (via the Check below), and a correct + // password on attempt 10 still succeeds — successful logins reset + // both counters. The reservation sits after the DB-error return above + // so a transient DB outage still does not consume attempts. + // Deliberately NOT auth.ScaledLimit for the per-username budget: that cap + // is keyed per USER and is the only cross-IP brute-force defence, so + // scaling it with the shared-NAT multiplier would hand a distributed + // attacker more guesses (api/constants_test.go pins this call site). + if !s.limiter.Allow(failKey, auth.ScaledLimit(loginFailureThreshold)+1, loginFailureWindow) || + !s.limiter.Allow(userFailKey, loginUserFailureThreshold+1, loginUserFailureWindow) { + return nil, ErrLockedOut + } + // Always run the password check — with an empty hash when the user does + // not exist. auth.CheckPassword performs a dummy bcrypt comparison for an + // empty hash, so bcrypt executes on every path and response time stays + // constant, preventing timing-based username enumeration. (A `user == nil + // || CheckPassword(...)` short-circuit would skip bcrypt entirely for + // unknown usernames, reintroducing the timing side-channel.) + storedHash := "" + if user != nil { + storedHash = user.PasswordHash + } + if !auth.CheckPassword(storedHash, in.Password) { + // The attempt was already recorded atomically up-front (F3); here + // only decide the lockouts, at the same boundary as before: the + // 10th in-window failure locks the key. Check is read-only, so + // the reservation is not double-counted. + if !s.limiter.Check(failKey, auth.ScaledLimit(loginFailureThreshold)+1, loginFailureWindow) { + s.limiter.Lockout(ctx, lockKey, loginLockoutDuration) + } + // BUG-110: per-username lockout on threshold. + if !s.limiter.Check(userFailKey, loginUserFailureThreshold+1, loginUserFailureWindow) { + s.limiter.Lockout(ctx, userLockKey, loginUserLockoutDuration) + } + slog.Info("login failed", "ip", in.IP, "username_len", len(in.Username)) + return nil, ErrInvalidCredentials + } + + // Reset failure counters on success. + s.limiter.Reset(ctx, failKey) + s.limiter.Reset(ctx, userFailKey) + return user, nil +} + +// VerifyTOTP completes a challenge Login started and issues the session, +// bound to the login request's device and IP rather than this one's. +func (s *AuthService) VerifyTOTP(ctx context.Context, partialToken, code string) (*AuthResult, error) { + challenge, ok := s.partial.Lookup(partialToken) + if !ok { + return nil, ErrTOTPChallengeInvalid + } + + totpRateLimitKey := auth.Key("totp_fail", challenge.UserID) + // Atomically record this attempt and reject once the per-user failure cap + // is reached. Recording up-front — rather than a read-only Check now and + // Allow only on failure — closes a TOCTOU where many concurrent requests + // reusing one valid partial token all pass the read-only check before any + // failure is recorded, defeating the per-user brute-force cap (the only + // cross-IP defence). A successful verification resets the counter below, + // so legitimate retries are not penalised. + // Deliberately NOT auth.ScaledLimit: this cap is keyed per USER, and it + // is the only cross-IP brute-force defence on TOTP codes. The + // multiplier exists for shared-NAT per-IP limits; scaling a per-user + // threshold with it would hand a distributed attacker more guesses. + // Mirrors loginUserFailureThreshold staying unscaled in authenticate. + if !s.limiter.Allow(totpRateLimitKey, totpFailureRateLimit, totpFailureWindow) { + return nil, ErrTooManyAttempts + } + + user, secret, err := s.challengeSecret(ctx, challenge.UserID) + if err != nil { + return nil, err + } + + if !auth.VerifyTOTPCodeOnce(secret, strings.TrimSpace(code), time.Now().UTC(), user.ID, s.usedCodes) { + // The attempt was already recorded atomically up-front via + // limiter.Allow; only the per-partial-token counter is advanced here. + s.partial.RegisterFailure(partialToken, partialAuthMaxFailures) + return nil, ErrTOTPCodeInvalid + } + + s.limiter.Reset(ctx, totpRateLimitKey) + + if _, ok := s.partial.Consume(partialToken); !ok { + return nil, ErrTOTPChallengeInvalid + } + + token, err := issueSession(ctx, s.st, user.ID, challenge.Device, challenge.IP) + if err != nil { + return nil, ErrSessionIssue + } + + slog.Info("totp verified", "user_id", user.ID, "ip", challenge.IP) + db.WriteAudit(context.WithoutCancel(ctx), s.st, user.ID, "totp_verified", "user", user.ID, + "two-factor verification completed from "+challenge.IP) + return &AuthResult{Token: token, User: user}, nil +} + +// challengeSecret resolves the user behind a partial-auth challenge and +// returns their decrypted TOTP secret. +func (s *AuthService) challengeSecret(ctx context.Context, challengeUserID int64) (*db.User, string, error) { + user, err := s.st.GetUserByID(ctx, challengeUserID) + if err != nil || user == nil || user.TOTPSecret == nil { + return nil, "", ErrTOTPChallengeInvalid + } + + // A ban can land inside the partial-token window; the login path + // refuses banned users right after the password compare, so the + // second factor must refuse them too. + if auth.IsEffectivelyBanned(user) { + return nil, "", ErrBanned + } + + secret, decErr := auth.DecryptTOTPSecret(s.totpKey, *user.TOTPSecret) + if decErr != nil { + slog.Error("failed to decrypt TOTP secret", "user_id", user.ID, "error", decErr) + return nil, "", ErrTOTPSecretUnreadable + } + + return user, secret, nil +} + +// Logout revokes p.Session server-side and clears the custom status. p.Session +// must be non-nil: the transport answers an API-token principal with 401 +// before calling. +func (s *AuthService) Logout(ctx context.Context, p Principal) error { + sess := p.Session + if sess == nil { + return errors.New("logout: principal has no session") + } + + // The client clears its token optimistically — once logout reaches the + // server, the revocation must not die with a dropped connection. + if err := s.st.DeleteSession(context.WithoutCancel(ctx), sess.TokenHash); err != nil { + return ErrLogoutFailed + } + + // A custom status is a "what I am doing right now" note. Leaving it + // standing after the user signed out states something about them that + // is no longer true, so logout clears it — unlike the chosen presence + // status, which is a preference and deliberately survives. + if err := s.st.UpdateUserCustomStatus(context.WithoutCancel(ctx), sess.UserID, nil); err != nil { + slog.Warn("failed to clear custom status on logout", "user_id", sess.UserID, "err", err) + } + + slog.Info("user logged out", "user_id", sess.UserID) + db.WriteAudit(context.WithoutCancel(ctx), s.st, sess.UserID, "user_logout", "user", sess.UserID, "") + return nil +} + +// DeleteAccount confirms the password, anonymises and bans the account and +// broadcasts member_ban. Progressive lockout mirrors login: 3 failures → +// 15-min lock. ip is only logged and audited. +func (s *AuthService) DeleteAccount(ctx context.Context, p Principal, password, ip string) error { + user := p.User + + // Per-user lockout to prevent password brute-force on this destructive endpoint. + lockKey := auth.Key("delete_lock", user.ID) + if s.limiter.IsLockedOut(lockKey) { + return ErrTooManyAttempts + } + + if password == "" { + return ErrPasswordRequired + } + + // Verify the supplied password matches the stored hash. + failKey := auth.Key("delete_fail", user.ID) + if !auth.CheckPassword(user.PasswordHash, password) { + if !s.limiter.Allow(failKey, deleteAccountFailureThreshold, deleteAccountFailureWindow) { + s.limiter.Lockout(ctx, lockKey, deleteAccountLockoutDuration) + } + return ErrIncorrectPassword + } + s.limiter.Reset(ctx, failKey) + + if err := s.st.DeleteAccount(ctx, user.ID); err != nil { + if errors.Is(err, db.ErrLastAdmin) { + return ErrLastAdmin + } + slog.Error("DeleteAccount failed", "err", err, "user_id", user.ID) + return ErrDeleteAccountFailed + } + + slog.Info("account deleted", "username", user.Username, "user_id", user.ID, "ip", ip) + db.WriteAudit(context.WithoutCancel(ctx), s.st, user.ID, "account_deleted", "user", user.ID, + "account self-deleted from "+ip) + + // DeleteAccount left the row in exactly the state an admin ban does + // (anonymised, banned, sessions revoked) — broadcast the same event so + // every other connected client drops the deleted user immediately + // instead of keeping their pre-deletion username until it reconnects. + if s.broadcaster != nil { + s.broadcaster.BroadcastMemberBan(user.ID) + } + return nil +} + +// EnableTOTP confirms the password and stages a pending secret; the returned +// URI is the enrolment payload for the authenticator app. +func (s *AuthService) EnableTOTP(ctx context.Context, p Principal, password string) (string, error) { + user := p.User + + // BUG-111: Per-user lockout for password confirmation. + lockKey := auth.Key("pw_confirm_lock", user.ID) + if s.limiter.IsLockedOut(lockKey) { + return "", ErrTooManyAttempts + } + + if user.TOTPSecret != nil && *user.TOTPSecret != "" { + return "", ErrTOTPAlreadyEnabled + } + + if err := s.confirmPassword(ctx, user, password, lockKey); err != nil { + return "", err + } + + secret, err := auth.GenerateTOTPSecret() + if err != nil { + return "", ErrTOTPSecretGenerate + } + + s.pending.Put(user.ID, secret) + return auth.BuildTOTPURI(user.Username, secret, "OwnCord"), nil +} + +// ConfirmTOTP verifies code against the pending secret, persists it and +// revokes the caller's other sessions. +func (s *AuthService) ConfirmTOTP(ctx context.Context, p Principal, password, code string) (*TOTPChangeResult, error) { + user := p.User + + // BUG-111: Per-user lockout for password confirmation. + lockKey := auth.Key("pw_confirm_lock", user.ID) + if s.limiter.IsLockedOut(lockKey) { + return nil, ErrTooManyAttempts + } + + if err := s.confirmPassword(ctx, user, password, lockKey); err != nil { + return nil, err + } + + secret, ok := s.pending.Lookup(user.ID) + if !ok { + return nil, ErrNoPendingTOTP + } + + if !auth.VerifyTOTPCodeOnce(secret, strings.TrimSpace(code), time.Now().UTC(), user.ID, s.usedCodes) { + return nil, ErrTOTPCodeInvalid + } + + encryptedSecret, encErr := auth.EncryptTOTPSecret(s.totpKey, secret) + if encErr != nil { + slog.Error("failed to encrypt TOTP secret", "user_id", user.ID, "error", encErr) + return nil, ErrTOTPEnableFailed + } + + if err := s.st.UpdateUserTOTPSecret(ctx, user.ID, &encryptedSecret); err != nil { + return nil, ErrTOTPEnableFailed + } + s.pending.Delete(user.ID) + + // Security tail of the 2FA change: once the secret update committed, + // revoking the other sessions must not be aborted by a dead request. + tailCtx := context.WithoutCancel(ctx) + revoked, revokeFailed := s.revokeOtherSessionsAfterAuthChange(tailCtx, user.ID, keepSessionID(p), "totp enable") + + slog.Info("totp enabled", "user_id", user.ID) + db.WriteAudit(tailCtx, s.st, user.ID, "totp_enabled", "user", user.ID, + "two-factor authentication enrolled") + + res := &TOTPChangeResult{SessionsRevoked: revoked} + if revokeFailed { + // Partial success: 2FA IS enabled; only revoking the other + // sessions failed. A 5xx here would be a lie — the state change + // already committed — so mirror the ChangePassword contract + // (api/profile_handler.go) and report 200 with an explicit warning + // instead of a silent, unqualified 204. + res.Warning = "two-factor authentication enabled, but other sessions could not be revoked; revoke them from the sessions list" + } + return res, nil +} + +// DisableTOTP confirms the password, refuses while the server requires 2FA, +// clears the secret and revokes the caller's other sessions. +func (s *AuthService) DisableTOTP(ctx context.Context, p Principal, password string) (*TOTPChangeResult, error) { + user := p.User + + // BUG-111: Per-user lockout for password confirmation. + lockKey := auth.Key("pw_confirm_lock", user.ID) + if s.limiter.IsLockedOut(lockKey) { + return nil, ErrTooManyAttempts + } + + if err := s.confirmPassword(ctx, user, password, lockKey); err != nil { + return nil, err + } + + require2FA, err := s.require2FAEnabled(ctx) + if err != nil { + return nil, ErrAuthPolicyUnavailable + } + if require2FA { + return nil, ErrTOTPRequiredByServer + } + + s.pending.Delete(user.ID) + if err := s.st.UpdateUserTOTPSecret(ctx, user.ID, nil); err != nil { + return nil, ErrTOTPDisableFailed + } + + // Security tail of the 2FA change: once the secret update committed, + // revoking the other sessions must not be aborted by a dead request. + tailCtx := context.WithoutCancel(ctx) + revoked, revokeFailed := s.revokeOtherSessionsAfterAuthChange(tailCtx, user.ID, keepSessionID(p), "totp disable") + + slog.Info("totp disabled", "user_id", user.ID) + db.WriteAudit(tailCtx, s.st, user.ID, "totp_disabled", "user", user.ID, + "two-factor authentication disabled") + + res := &TOTPChangeResult{SessionsRevoked: revoked} + if revokeFailed { + // Partial success: 2FA IS disabled; only revoking the other + // sessions failed. A 5xx here would be a lie — the state change + // already committed — so mirror the ChangePassword contract + // (api/profile_handler.go) and report 200 with an explicit warning + // instead of a silent, unqualified 204. + res.Warning = "two-factor authentication disabled, but other sessions could not be revoked; revoke them from the sessions list" + } + return res, nil +} + +// confirmPassword is the password-confirmation step the TOTP routes share: a +// missing or wrong password counts against the per-user pw_confirm budget +// and trips the lockout on the threshold; a correct one resets the counter. +func (s *AuthService) confirmPassword(ctx context.Context, user *db.User, password, lockKey string) error { + failKey := auth.Key("pw_confirm_fail", user.ID) + if err := requirePasswordConfirmation(user, password); err != nil { + if !s.limiter.Allow(failKey, PwConfirmFailureThreshold, PwConfirmFailureWindow) { + s.limiter.Lockout(ctx, lockKey, PwConfirmLockoutDuration) + } + return err + } + s.limiter.Reset(ctx, failKey) + return nil +} + +// keepSessionID is the session a 2FA state change keeps alive. BUG-108: an +// API-token principal has a nil session; keep=0 matches no row, so every +// login session is revoked — same semantics as change-password. +func keepSessionID(p Principal) int64 { + if p.Session != nil { + return p.Session.ID + } + return 0 +} + +// revokeOtherSessionsAfterAuthChange revokes every session for userID except +// keepSessionID as the security tail of a committed 2FA state change. It +// mirrors UserService.ChangePassword (service/user.go:262-274): a failure is +// logged and retried once (bounded compensating retry for transient write +// contention); if the retry also fails, revoked reports what did succeed and +// failed is true so the caller can report a partial success instead of +// silently claiming the other sessions were revoked when they were not. +func (s *AuthService) revokeOtherSessionsAfterAuthChange(ctx context.Context, userID, keepSessionID int64, action string) (revoked int64, failed bool) { + revoked, err := s.st.DeleteOtherSessions(ctx, userID, keepSessionID) + if err != nil { + slog.Error("DeleteOtherSessions after "+action, "err", err, "user_id", userID) + revokedRetry, retryErr := s.st.DeleteOtherSessions(ctx, userID, keepSessionID) + if retryErr != nil { + slog.Error("DeleteOtherSessions retry after "+action, "err", retryErr, "user_id", userID) + return revoked, true + } + revoked += revokedRetry + } + if revoked > 0 { + slog.Info("revoked other sessions after "+action, "user_id", userID, "revoked", revoked) + } + return revoked, false +} + +// ─── Helpers moved from api/auth_handler.go ────────────────────────────────── + +func issueSession(ctx context.Context, st Store, userID int64, device, ip string) (string, error) { + token, err := auth.GenerateToken() + if err != nil { + return "", err + } + if _, err := st.CreateSession(ctx, userID, auth.HashToken(token), device, ip); err != nil { + return "", err + } + return token, nil +} + +func (s *AuthService) require2FAEnabled(ctx context.Context) (bool, error) { + return getBooleanSetting(ctx, s.st, "require_2fa", false) +} + +func (s *AuthService) registrationOpen(ctx context.Context) (bool, error) { + return getBooleanSetting(ctx, s.st, "registration_open", true) +} + +func getBooleanSetting(ctx context.Context, st Store, key string, defaultValue bool) (bool, error) { + value, err := st.GetSetting(ctx, key) + if err != nil { + if errors.Is(err, db.ErrNotFound) { + return defaultValue, nil + } + return false, err + } + return parseBooleanSettingValue(value) +} + +func parseBooleanSettingValue(value string) (bool, error) { + switch strings.ToLower(strings.TrimSpace(value)) { + case "1", "true": + return true, nil + case "0", "false": + return false, nil + default: + return false, fmt.Errorf("invalid boolean setting value %q", value) + } +} + +func requirePasswordConfirmation(user *db.User, password string) error { + if password == "" { + return ErrPasswordRequired + } + if !auth.CheckPassword(user.PasswordHash, password) { + return ErrPasswordConfirmationFailed + } + return nil +} diff --git a/docs/architecture/server-boundaries.md b/docs/architecture/server-boundaries.md index 56f1a743..ecb9ef4f 100644 --- a/docs/architecture/server-boundaries.md +++ b/docs/architecture/server-boundaries.md @@ -1,6 +1,9 @@ # Server boundaries — database-call and lifecycle inventory **Written:** 2026-08-29 (B3-0), measured at `dev` `ad4defc2`. +**Re-measured:** 2026-08-30 (B3-2) — the first table and the auth slice's +after-state at `fe1d11b8` (pre-squash; the squash SHA is in the plan's B3-2 +evidence block). **Owner:** the B3 plan, [plans/b3-server-architecture-guardrails-2026-08-29.md](../plans/b3-server-architecture-guardrails-2026-08-29.md). **Regenerate the first table:** `cd Server && go run ./cmd/dbinventory` and @@ -18,7 +21,7 @@ happens to that use — one of four dispositions from the | Disposition | Meaning | Rows | | ----------- | --------------------------------------------------------------------------------------------------------------------- | ---: | -| `move` | persistence or a domain decision that belongs behind a service; **Family** names the service B3-8 (or B3-2) builds | 28 | +| `move` | persistence or a domain decision that belongs behind a service; **Family** names the service B3-8 (or B3-2) builds | 26 | | `adapter` | a transport adapter that uses `db` types or pure helpers only — response shapes, status helpers — no persistence call | 17 | | `boundary` | an explicit composition or transaction boundary that legitimately owns a handle (process entry, CLIs, health probe) | 6 | | `remove` | the import is unnecessary and goes | 0 | @@ -26,8 +29,8 @@ happens to that use — one of four dispositions from the The rows live in code, not only here: `Server/invariants/db_import_boundary.go` holds them as `DBImportAllow`, the `db-import-boundary` rule fails any new importer that has no row, and `TestDBImportAllowIsLive` fails any row whose -file stopped importing `db`. The list only shrinks — B3-2 deletes the two auth -handler rows, B3-8 deletes a family's rows as it moves. +file stopped importing `db`. The list only shrinks — B3-2 deleted the two auth +handler rows (28 → 26 `move`), B3-8 deletes a family's rows as it moves. ## How the measurement works @@ -55,62 +58,60 @@ which is a row worth reading, and none exists today. -| File | `db.*` types | `db.*` funcs and sentinels | `*db.DB` method calls | Shape | Disposition | Family | Why | -| --------------------------------- | ---------------------------------------------------------------------------------------------------------------------------- | --------------------------------------------------------------------------- | -------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------- | --------- | ----------- | ------------ | --------------------------------------------------------------- | -| `admin/admin.go` | `DB` | — | — | type-only | boundary | — | holds the handle for the admin mux; no calls | -| `admin/api.go` | `DB` | — | — | type-only | boundary | — | passes the handle to handlers; no calls | -| `admin/backup_maintenance.go` | `DB×3` | `CheckBackupIntegrity()` `ErrNotFound×2` `WriteAudit()×2` | `BackupToSafe` `GetSetting×2` | calls | move | settings-ops | BackupToSafe, integrity check, settings reads | -| `admin/handlers_backup.go` | `DB×5` | `CheckBackupIntegrity()×2` `WriteAudit()×2` | `BackupToSafe×2` `Close` `LogAudit` `SQLDb` | calls | move | settings-ops | backup trigger and download; raw SQLDb for VACUUM INTO | -| `admin/handlers_channel_perms.go` | `Channel` `ChannelRoleOverride×2` `ChannelUserOverride×2` `DB×8` `Role×2` `User×2` | `WriteAudit()×4` | `DeleteChannelOverride` `DeleteChannelUserOverride` `GetChannel` `GetChannelPermissions×2` `GetRoleByID×3` `GetUserByID` `GetUserChannelPermissions×2` `ListChannelRoleOverrides` `ListChannelUserOverrides` `ListUserIDsByRole×2` `UpsertChannelOverride` `UpsertChannelUserOverride` | calls | move | channel | override CRUD decides permission policy in the handler | -| `admin/handlers_channels.go` | `Channel×2` `ChannelUpdate×2` `DB×6` | `WriteAudit()×3` | `AdminCreateChannel` `AdminDeleteChannel` `AdminUpdateChannel×2` `GetAuditLog` `GetChannel×4` `ListChannels` | calls | move | channel | channel CRUD + audit | -| `admin/handlers_roles.go` | `DB×4` | — | `GetRoleByID` `ListRoles` | calls | move | role | two reads; service/role.go already owns the writes | -| `admin/handlers_settings.go` | `DB×4` | `ErrNotFound` `WriteAudit()` | `BeginTx` `CountUsersWithoutTOTP` `GetAllSettings×2` `GetSetting` | calls | move | settings-ops | BeginTx in a handler; TOTP census | -| `admin/handlers_tokens.go` | `DB×3` `User` | `WriteAudit()×2` | `CreateAPIToken` `GetOwnerUser` `GetUserByUsername` `ListAPITokens` `RevokeAPIToken` | calls | move | auth | API-token CRUD duplicated in token_cli.go | -| `admin/handlers_users.go` | `DB×4` `Role` `User` | — | `GetServerStats` `GetUserByID×2` `ListAllUsers` | calls | move | user | user list, stats, lookups | -| `admin/helpers.go` | `Role×2` `User` | — | — | type-only | adapter | — | Role/User types in response helpers | -| `admin/logstream.go` | `DB×3` | — | — | type-only | boundary | — | handle threaded to the SSE stream's auth check; no calls | -| `admin/middleware.go` | `DB×3` `Role` `User` | — | `GetRoleByID` | calls | move | auth | owner gate re-reads the role — OC-0345 | -| `admin/setup_handler.go` | `DB×4` | `ErrConflict` `WriteAudit()×2` | `CreateChannel×2` `CreateInvite` `CreateOwnerIfEmpty` `CreateSession` `GetSetting×3` `UserCount` | calls | move | auth | first-run owner creation (setup sub-family) | -| `admin/setup_wizard.go` | `DB` | — | `BeginTx` | calls | move | auth | BeginTx for the wizard; setup sub-family | -| `admin/types.go` | `Channel×3` `DB` `Role` `User` `UserWithRole` | — | `GetRoleByID` | calls | adapter | — | response DTOs; the one GetRoleByID moves with handlers_users | -| `api/auth_handler.go` | `DB×11` `Session` `User×5` | `ErrLastAdmin` `ErrNotFound×2` `IsUniqueConstraintError()` `WriteAudit()×5` | `CreateSession×2` `CreateUserWithInvite` `DeleteAccount` `DeleteSession` `GetSetting` `GetUserByID` `GetUserByUsername` `UpdateUserCustomStatus` | calls | move | auth | B3-2 slice: register/login/logout/delete own the DB | -| `api/channel_handler.go` | `DB` `MessageAPIResponse×2` `MessageSearchResult×2` `ReactionUser` `User×8` | — | — | type-only | adapter | — | response types only; service owns the calls | -| `api/dm_handler.go` | `DB` `DMChannelInfo` `DMUser×2` `User×8` | `NewDMChannelInfo()` `StatusForViewer()` | — | calls | adapter | — | DM response types + pure status helpers | -| `api/emoji_handler.go` | `DB` `Emoji×3` `User×2` | — | — | type-only | adapter | — | Emoji/User types only | -| `api/gif_handler.go` | `DB` | — | — | type-only | adapter | — | handle in the signature, unused for calls | -| `api/invite_handler.go` | `DB` `Invite` `User×2` | — | — | type-only | adapter | — | Invite/User types only | -| `api/middleware.go` | `DB` `Role` | — | `DeleteSession` `TouchAPIToken` `TouchSession` | calls | move | auth | session/API-token touch and revoke | -| `api/plugins_handler.go` | `Auditor×2` | `WriteAudit()` | — | calls | adapter | — | db.Auditor is the seam; WriteAudit only | -| `api/profile_handler.go` | `DB×2` `Session×2` `User×6` | — | `CreateAttachment` | calls | move | upload | avatar upload creates the attachment row | -| `api/router.go` | `DB×4` | — | `PingRead` `SQLDb` | calls | boundary | — | health probe (PingRead, SQLDb); hub construction leaves in B3-3 | -| `api/totp_handler.go` | `DB×5` `Session×2` `User×4` | `WriteAudit()×3` | `DeleteOtherSessions×2` `GetUserByID` `UpdateUserTOTPSecret×2` | calls | move | auth | B3-2 slice: TOTP enrol/verify write the user row | -| `api/upload_handler.go` | `AttachmentAccess×2` `DB×5` `Role` `User×3` | — | `CreateAttachment` `GetAttachmentWithChannel` `IsAvatarFileURL` `IsDMParticipant` `QueryRowContext` | calls | move | upload | attachment access + a raw QueryRowContext | -| `auth/helpers.go` | `User` | — | — | type-only | adapter | — | db.User type in a helper signature | -| `auth/resolve.go` | `APIToken` `Role×2` `Session×2` `User×2` | — | — | type-only | adapter | — | Session/APIToken/Role/User types; resolution is injected | -| `cmd/seed/main.go` | `DB×6` | `Migrate()` `Open()` | `Close` `CreateChannel` `CreateMessage×2` `CreateUser` `GetOrCreateDMChannel` `GetUserByUsername` `ListChannels` `QueryRowContext` | calls | boundary | — | developer seeding tool owns its handle | -| `main.go` | `AuditWriter×2` `DB×10` | `ErrNotFound` `Migrate()` `NewAuditWriter()` `OpenWithMaxReaders()` | `ClearAllVoiceStates` `Close` `DeleteExpiredSessions` `DeleteOrphanedAttachments` `GetMaxEventSeq` `GetSetting` `ResetAllUserStatuses` `SetAuditWriter` `SetSetting` | calls | boundary | — | process composition root; B3-3 moves it to internal/app | -| `plugin/pluginstore.go` | `PluginRow×3` | — | — | type-only | adapter | — | PluginRow type only; the store is injected | -| `token_cli.go` | `DB×3` `User` | `Migrate()` `OpenShared()` `WriteAudit()×3` | `Close` `CreateAPIToken` `GetOwnerUser` `GetUserByUsername` `ListAPITokens` `RevokeAPIToken` `RevokeAPITokenByLabel` | calls | move | auth | API-token CLI duplicates admin/handlers_tokens.go | -| `ws/client.go` | `User×2` | — | — | type-only | adapter | — | db.User type on the connection | -| `ws/deps.go` | `Channel` `DB×5` | — | `GetRoleForUser×3` `IsDMParticipant` | calls | move | channel | role and DM-membership reads behind the hub's deps | -| `ws/event.go` | — | `BroadcastStatus()` | — | calls | adapter | — | pure BroadcastStatus helper | -| `ws/event_persister.go` | `PersistedEvent×2` | — | — | type-only | adapter | — | PersistedEvent type; store is an interface | -| `ws/eventstore.go` | `PersistedEvent×3` | — | — | type-only | adapter | — | PersistedEvent type; store is an interface | -| `ws/handlers.go` | `User` | — | `GetChannel` `GetRoleForUser` `GetSessionWithBanStatus` `IsDMParticipant` | calls | move | channel | channel, role, session-ban and DM reads in command handlers | -| `ws/handlers_chat.go` | — | `NewDMChannelInfo()` | — | calls | adapter | — | pure NewDMChannelInfo helper | -| `ws/hub.go` | `DB×2` | — | `GetSetting×2` | calls | move | settings-ops | GetSetting at construction | -| `ws/hub_broadcast.go` | `Channel×4` `Emoji` `Role` | `BroadcastStatus()` | `GetChannel×2` `GetDMParticipantIDs` `GetRoleForUser` `GetUserByID×2` `ListChannels` | calls | move | channel | visibility refresh reads channels, roles, users | -| `ws/hub_sweep.go` | `User` | — | `GetAllVoiceStates` `GetChannel` `GetChannelVoiceStates` `GetSessionsWithBanStatusBatch` `LeaveVoiceChannelIfMatch×2` | calls | move | voice | stale-voice sweep reads and leaves | -| `ws/messages.go` | `Channel×4` `DMChannelInfo×2` `DMUser×2` `Emoji` `Role×3` `User×2` `VoiceState` | `BroadcastStatus()` `StatusForViewer()` | — | calls | adapter | — | wire types + pure status helpers | -| `ws/serve.go` | `ChannelOverride` `DB×9` `PersistedEvent` `User` `VoiceState` | `ConnectStatus()` `StatusOffline` `WriteAudit()` | `GetChannelOverridesFor` `GetRoleByID×4` `GetUserByID×2` `GetUserDMChannelIDs` `GetVoiceState` `LeaveVoiceChannelIfMatch` `ListChannels` `MarkUserDisconnected` `UpdateUserStatus` | calls | move | connection | connect/disconnect lifecycle; B3-5 splits it by family first | -| `ws/serve_auth.go` | `DB` `User` | — | `GetSessionByTokenHash` `GetUserByID` | calls | move | auth | session lookup on the WebSocket handshake | -| `ws/serve_pumps.go` | — | `StatusOffline` | `MarkUserDisconnected` | calls | move | user | MarkUserDisconnected on pump exit | -| `ws/serve_ready.go` | `Channel×10` `ChannelOverride×6` `ChannelUnread×2` `DB×5` `DMChannelInfo×4` `MemberSummary×3` `Role×4` `User` `VoiceState×4` | `StatusOffline×3` | `GetAllVoiceStates` `GetChannelOverridesFor` `GetChannelUnreadCounts` `GetUserDMChannels` `ListChannels` `ListMembers` `ListRoles` | calls | move | channel | ready snapshot: channels, overrides, unreads, DMs, members | -| `ws/voice_join.go` | `Channel×3` `ChannelOverride` `VoiceState×5` | `ErrChannelFull` | `GetChannel×2` `GetChannelOverridesFor` `GetChannelVoiceStates` `GetRoleForUser` `GetVoiceState×6` `JoinVoiceChannel` `JoinVoiceChannelIfCapacity` `LeaveVoiceChannelIfMatch` `SetVoiceServerDeafen` `SetVoiceServerMute` | calls | move | voice | voice state reads and writes | -| `ws/voice_moderation.go` | `Role` `VoiceState×3` | `WriteAudit()` | `CountChannelVoiceUsers` `GetChannel×2` `GetRoleForUser` `GetVoiceState×2` `SetVoiceServerDeafen×2` `SetVoiceServerMute×2` | calls | move | voice | mute/deafen/move persist voice state | +| File | `db.*` types | `db.*` funcs and sentinels | `*db.DB` method calls | Shape | Disposition | Family | Why | +| --------------------------------- | ---------------------------------------------------------------------------------------------------------------------------- | ------------------------------------------------------------------- | -------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------- | --------- | ----------- | ------------ | --------------------------------------------------------------- | +| `admin/admin.go` | `DB` | — | — | type-only | boundary | — | holds the handle for the admin mux; no calls | +| `admin/api.go` | `DB` | — | — | type-only | boundary | — | passes the handle to handlers; no calls | +| `admin/backup_maintenance.go` | `DB×3` | `CheckBackupIntegrity()` `ErrNotFound×2` `WriteAudit()×2` | `BackupToSafe` `GetSetting×2` | calls | move | settings-ops | BackupToSafe, integrity check, settings reads | +| `admin/handlers_backup.go` | `DB×5` | `CheckBackupIntegrity()×2` `WriteAudit()×2` | `BackupToSafe×2` `Close` `LogAudit` `SQLDb` | calls | move | settings-ops | backup trigger and download; raw SQLDb for VACUUM INTO | +| `admin/handlers_channel_perms.go` | `Channel` `ChannelRoleOverride×2` `ChannelUserOverride×2` `DB×8` `Role×2` `User×2` | `WriteAudit()×4` | `DeleteChannelOverride` `DeleteChannelUserOverride` `GetChannel` `GetChannelPermissions×2` `GetRoleByID×3` `GetUserByID` `GetUserChannelPermissions×2` `ListChannelRoleOverrides` `ListChannelUserOverrides` `ListUserIDsByRole×2` `UpsertChannelOverride` `UpsertChannelUserOverride` | calls | move | channel | override CRUD decides permission policy in the handler | +| `admin/handlers_channels.go` | `Channel×2` `ChannelUpdate×2` `DB×6` | `WriteAudit()×3` | `AdminCreateChannel` `AdminDeleteChannel` `AdminUpdateChannel×2` `GetAuditLog` `GetChannel×4` `ListChannels` | calls | move | channel | channel CRUD + audit | +| `admin/handlers_roles.go` | `DB×4` | — | `GetRoleByID` `ListRoles` | calls | move | role | two reads; service/role.go already owns the writes | +| `admin/handlers_settings.go` | `DB×4` | `ErrNotFound` `WriteAudit()` | `BeginTx` `CountUsersWithoutTOTP` `GetAllSettings×2` `GetSetting` | calls | move | settings-ops | BeginTx in a handler; TOTP census | +| `admin/handlers_tokens.go` | `DB×3` `User` | `WriteAudit()×2` | `CreateAPIToken` `GetOwnerUser` `GetUserByUsername` `ListAPITokens` `RevokeAPIToken` | calls | move | auth | API-token CRUD duplicated in token_cli.go | +| `admin/handlers_users.go` | `DB×4` `Role` `User` | — | `GetServerStats` `GetUserByID×2` `ListAllUsers` | calls | move | user | user list, stats, lookups | +| `admin/helpers.go` | `Role×2` `User` | — | — | type-only | adapter | — | Role/User types in response helpers | +| `admin/logstream.go` | `DB×3` | — | — | type-only | boundary | — | handle threaded to the SSE stream's auth check; no calls | +| `admin/middleware.go` | `DB×3` `Role` `User` | — | `GetRoleByID` | calls | move | auth | owner gate re-reads the role — OC-0345 | +| `admin/setup_handler.go` | `DB×4` | `ErrConflict` `WriteAudit()×2` | `CreateChannel×2` `CreateInvite` `CreateOwnerIfEmpty` `CreateSession` `GetSetting×3` `UserCount` | calls | move | auth | first-run owner creation (setup sub-family) | +| `admin/setup_wizard.go` | `DB` | — | `BeginTx` | calls | move | auth | BeginTx for the wizard; setup sub-family | +| `admin/types.go` | `Channel×3` `DB` `Role` `User` `UserWithRole` | — | `GetRoleByID` | calls | adapter | — | response DTOs; the one GetRoleByID moves with handlers_users | +| `api/channel_handler.go` | `DB` `MessageAPIResponse×2` `MessageSearchResult×2` `ReactionUser` `User×8` | — | — | type-only | adapter | — | response types only; service owns the calls | +| `api/dm_handler.go` | `DB` `DMChannelInfo` `DMUser×2` `User×8` | `NewDMChannelInfo()` `StatusForViewer()` | — | calls | adapter | — | DM response types + pure status helpers | +| `api/emoji_handler.go` | `DB` `Emoji×3` `User×2` | — | — | type-only | adapter | — | Emoji/User types only | +| `api/gif_handler.go` | `DB` | — | — | type-only | adapter | — | handle in the signature, unused for calls | +| `api/invite_handler.go` | `DB` `Invite` `User×2` | — | — | type-only | adapter | — | Invite/User types only | +| `api/middleware.go` | `DB` `Role` `Session` `User` | — | `DeleteSession` `TouchAPIToken` `TouchSession` | calls | move | auth | session/API-token touch and revoke | +| `api/plugins_handler.go` | `Auditor×2` | `WriteAudit()` | — | calls | adapter | — | db.Auditor is the seam; WriteAudit only | +| `api/profile_handler.go` | `DB×2` `Session×2` `User×7` | — | `CreateAttachment` | calls | move | upload | avatar upload creates the attachment row | +| `api/router.go` | `DB×4` | — | `PingRead` `SQLDb` | calls | boundary | — | health probe (PingRead, SQLDb); hub construction leaves in B3-3 | +| `api/upload_handler.go` | `AttachmentAccess×2` `DB×5` `Role` `User×3` | — | `CreateAttachment` `GetAttachmentWithChannel` `IsAvatarFileURL` `IsDMParticipant` `QueryRowContext` | calls | move | upload | attachment access + a raw QueryRowContext | +| `auth/helpers.go` | `User` | — | — | type-only | adapter | — | db.User type in a helper signature | +| `auth/resolve.go` | `APIToken` `Role×2` `Session×2` `User×2` | — | — | type-only | adapter | — | Session/APIToken/Role/User types; resolution is injected | +| `cmd/seed/main.go` | `DB×6` | `Migrate()` `Open()` | `Close` `CreateChannel` `CreateMessage×2` `CreateUser` `GetOrCreateDMChannel` `GetUserByUsername` `ListChannels` `QueryRowContext` | calls | boundary | — | developer seeding tool owns its handle | +| `main.go` | `AuditWriter×2` `DB×10` | `ErrNotFound` `Migrate()` `NewAuditWriter()` `OpenWithMaxReaders()` | `ClearAllVoiceStates` `Close` `DeleteExpiredSessions` `DeleteOrphanedAttachments` `GetMaxEventSeq` `GetSetting` `ResetAllUserStatuses` `SetAuditWriter` `SetSetting` | calls | boundary | — | process composition root; B3-3 moves it to internal/app | +| `plugin/pluginstore.go` | `PluginRow×3` | — | — | type-only | adapter | — | PluginRow type only; the store is injected | +| `token_cli.go` | `DB×3` `User` | `Migrate()` `OpenShared()` `WriteAudit()×3` | `Close` `CreateAPIToken` `GetOwnerUser` `GetUserByUsername` `ListAPITokens` `RevokeAPIToken` `RevokeAPITokenByLabel` | calls | move | auth | API-token CLI duplicates admin/handlers_tokens.go | +| `ws/client.go` | `User×2` | — | — | type-only | adapter | — | db.User type on the connection | +| `ws/deps.go` | `Channel` `DB×5` | — | `GetRoleForUser×3` `IsDMParticipant` | calls | move | channel | role and DM-membership reads behind the hub's deps | +| `ws/event.go` | — | `BroadcastStatus()` | — | calls | adapter | — | pure BroadcastStatus helper | +| `ws/event_persister.go` | `PersistedEvent×2` | — | — | type-only | adapter | — | PersistedEvent type; store is an interface | +| `ws/eventstore.go` | `PersistedEvent×3` | — | — | type-only | adapter | — | PersistedEvent type; store is an interface | +| `ws/handlers.go` | `User` | — | `GetChannel` `GetRoleForUser` `GetSessionWithBanStatus` `IsDMParticipant` | calls | move | channel | channel, role, session-ban and DM reads in command handlers | +| `ws/handlers_chat.go` | — | `NewDMChannelInfo()` | — | calls | adapter | — | pure NewDMChannelInfo helper | +| `ws/hub.go` | `DB×2` | — | `GetSetting×2` | calls | move | settings-ops | GetSetting at construction | +| `ws/hub_broadcast.go` | `Channel×4` `Emoji` `Role` | `BroadcastStatus()` | `GetChannel×2` `GetDMParticipantIDs` `GetRoleForUser` `GetUserByID×2` `ListChannels` | calls | move | channel | visibility refresh reads channels, roles, users | +| `ws/hub_sweep.go` | `User` | — | `GetAllVoiceStates` `GetChannel` `GetChannelVoiceStates` `GetSessionsWithBanStatusBatch` `LeaveVoiceChannelIfMatch×2` | calls | move | voice | stale-voice sweep reads and leaves | +| `ws/messages.go` | `Channel×4` `DMChannelInfo×2` `DMUser×2` `Emoji` `Role×3` `User×2` `VoiceState` | `BroadcastStatus()` `StatusForViewer()` | — | calls | adapter | — | wire types + pure status helpers | +| `ws/serve.go` | `ChannelOverride` `DB×9` `PersistedEvent` `User` `VoiceState` | `ConnectStatus()` `StatusOffline` `WriteAudit()` | `GetChannelOverridesFor` `GetRoleByID×4` `GetUserByID×2` `GetUserDMChannelIDs` `GetVoiceState` `LeaveVoiceChannelIfMatch` `ListChannels` `MarkUserDisconnected` `UpdateUserStatus` | calls | move | connection | connect/disconnect lifecycle; B3-5 splits it by family first | +| `ws/serve_auth.go` | `DB` `User` | — | `GetSessionByTokenHash` `GetUserByID` | calls | move | auth | session lookup on the WebSocket handshake | +| `ws/serve_pumps.go` | — | `StatusOffline` | `MarkUserDisconnected` | calls | move | user | MarkUserDisconnected on pump exit | +| `ws/serve_ready.go` | `Channel×10` `ChannelOverride×6` `ChannelUnread×2` `DB×5` `DMChannelInfo×4` `MemberSummary×3` `Role×4` `User` `VoiceState×4` | `StatusOffline×3` | `GetAllVoiceStates` `GetChannelOverridesFor` `GetChannelUnreadCounts` `GetUserDMChannels` `ListChannels` `ListMembers` `ListRoles` | calls | move | channel | ready snapshot: channels, overrides, unreads, DMs, members | +| `ws/voice_join.go` | `Channel×3` `ChannelOverride` `VoiceState×5` | `ErrChannelFull` | `GetChannel×2` `GetChannelOverridesFor` `GetChannelVoiceStates` `GetRoleForUser` `GetVoiceState×6` `JoinVoiceChannel` `JoinVoiceChannelIfCapacity` `LeaveVoiceChannelIfMatch` `SetVoiceServerDeafen` `SetVoiceServerMute` | calls | move | voice | voice state reads and writes | +| `ws/voice_moderation.go` | `Role` `VoiceState×3` | `WriteAudit()` | `CountChannelVoiceUsers` `GetChannel×2` `GetRoleForUser` `GetVoiceState×2` `SetVoiceServerDeafen×2` `SetVoiceServerMute×2` | calls | move | voice | mute/deafen/move persist voice state | -51 files import `db` outside `db/` and `service/` (. 2, admin 16, api 12, auth 2, cmd/seed 1, plugin 1, ws 17); 14 are type-only; 0 unlisted. -Dispositions: adapter 17, boundary 6, move 28. Move targets: auth 9, channel 6, connection 1, role 1, settings-ops 4, upload 2, user 2, voice 3. +49 files import `db` outside `db/` and `service/` (. 2, admin 16, api 10, auth 2, cmd/seed 1, plugin 1, ws 17); 14 are type-only; 0 unlisted. +Dispositions: adapter 17, boundary 6, move 26. Move targets: auth 7, channel 6, connection 1, role 1, settings-ops 4, upload 2, user 2, voice 3. @@ -216,7 +217,7 @@ close function, which is exactly what B3-3's failure-injection test pins. ## Auth slice — before-state dependency graph The three files B3-2 moves, and what they depend on at `ad4defc2`. The -after-state table is appended by B3-2. +after-state table follows it. | File | Imports (module-internal) | `db` symbols used | | ---------------------- | ---------------------------------------------------------------------- | ------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------ | @@ -224,9 +225,32 @@ after-state table is appended by B3-2. | `api/totp_handler.go` | `auth`, `db` | types `DB`, `Session`, `User`; func `WriteAudit`; methods `DeleteOtherSessions`, `GetUserByID`, `UpdateUserTOTPSecret` | | `auth/*.go` (10 files) | `db` (types only, in `helpers.go`, `resolve.go`), `config`, `syncutil` | types `User`, `Session`, `APIToken`, `Role` — no method calls; `auth` is a leaf that computes and does not persist | -Eleven distinct `*db.DB` methods across the two handlers. That is the upper -bound of the interface `api/auth_deps.go` declares in B3-2; the after-state -row must show the handlers importing neither `db` nor `service` directly. +Eleven distinct `*db.DB` methods across the two handlers (ten after +de-duplicating `GetUserByID`). That was the upper bound set for the interface +`api/auth_deps.go` declares in B3-2. + +### Auth slice — after-state dependency graph + +Measured at `fe1d11b8` (B3-2, pre-squash). The handlers import `db` nowhere; +`api` goes from 12 `db` importers to 10. + +| File | Imports (module-internal) | `db` symbols used | +| ------------------------ | -------------------------------------------- | -------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------- | +| `api/auth_deps.go` | `service` | none — nine-method `AuthService` interface naming `service.Principal`, `RegisterInput`, `LoginInput`, `AuthResult`, `TOTPChangeResult` | +| `api/auth_handler.go` | `auth`, `service` | none — `auth` for `ValidateUsername`/`ValidatePasswordStrength`; `service` for the interface's types, the `Err*` categories `writeAuthError` switches on, and `SanitizeText` (imported before B3-2 too) | +| `api/totp_handler.go` | `auth`, `service` | none — `auth` for `ExtractBearerToken`; `service` for `TOTPChangeResult` and the `Err*` categories | +| `api/middleware.go` | `auth`, `db`, `permissions`, `service` (new) | unchanged row (`move`, auth): gained `principal(r)`, which reads the `*db.User`/`*db.Session` it already stores on the context and hands them to the handlers as `service.Principal` | +| `api/profile_handler.go` | unchanged | unchanged row (`move`, upload): now hosts `userResponse`/`toUserResponse`, the one converter that names `db.User`, which `/me` and the auth responses share with `PATCH /users/me` | +| `service/auth.go` | `auth`, `db`, `permissions` | funcs `WriteAudit`, `IsUniqueConstraintError`; sentinels `ErrLastAdmin`, `ErrNotFound`; the ten methods, through `service.Store`: `CreateSession`, `CreateUserWithInvite`, `DeleteAccount`, `DeleteOtherSessions`, `DeleteSession`, `GetSetting`, `GetUserByID`, `GetUserByUsername`, `UpdateUserCustomStatus`, `UpdateUserTOTPSecret` | +| `auth/ratescale.go` | — | none — the auth rate multiplier, moved from `api/constants.go` so the route mounts and the service's login accounting read one value | + +Honest reading of the plan's target ("handlers importing neither `db` nor +`service` directly"): met for `db`, not for `service`. Both handlers import +`service` because the consumer-owned interface is expressed in the service's +input and result types and its `Err*` values — the alternative, an `api`-side +copy of every type, would have been a second definition of the same shapes. +The dependency direction is still `api → service → db`; what the handlers no +longer see is the database. ## Client baselines are not here diff --git a/docs/architecture/server.md b/docs/architecture/server.md index e501a12b..755a3f10 100644 --- a/docs/architecture/server.md +++ b/docs/architecture/server.md @@ -1,6 +1,7 @@ # Server Architecture -**Verified against:** commit `5630aa1`, 2026-08-04 +**Verified against:** commit `5630aa1`, 2026-08-04; §D4 and the auth rows of +§D3 against `fe1d11b8`, 2026-08-30 (B3-2) Single Go binary (`github.com/J3vb/OwnCord/Server`, Go 1.26). Pure-Go SQLite (`modernc.org/sqlite`, no CGO), chi router, `github.com/coder/websocket`, @@ -121,7 +122,7 @@ sequenceDiagram DB-->>C: JSON response (errorResponse envelope on failure) rect rgba(200,120,120,0.15) - Note over H,DB: Deviation — auth routes: MountAuthRoutes(r, database, …)
bypasses the service layer and queries *db.DB directly.
Admin REST (Server/admin) does the same behind
AdminIPRestrict + RequireAdminAuth. + Note over H,DB: Deviation — Admin REST (Server/admin) queries *db.DB
directly behind AdminIPRestrict + RequireAdminAuth.
The auth routes did the same until B3-2 (see D4). end ``` @@ -136,9 +137,83 @@ channel-less routes on server-wide role permissions via per-channel allow must never open a server-wide gate), while anything channel-scoped is checked in the service layer through `svc.Permissions` / `permissions.Checker`, which resolves overrides and fails closed if they cannot -be fetched. The shaded region marks the two documented bypass paths of the -domain layer. +be fetched. The shaded region marks the remaining documented bypass of the +domain layer; the auth routes left it in B3-2 (`MountAuthRoutes(r, svc, +requireAuth, …)` → `service.AuthService` → `Store`). **Source of truth:** `Server/api/router.go`, `Server/api/middleware.go`, `Server/api/auth_handler.go`, `Server/admin/middleware.go`, `Server/permissions/`, `Server/service/`. + +## D4 — The vertical-slice pattern (B3-2; the rule for B3-8) + +The auth slice was the first domain family moved from "handler owns the +database" to `api → service → db` behind a consumer-owned interface, with a +frozen characterization set proving nothing changed. B3-8 repeats this per +family. The steps, in commit order, each its own commit so the reviewer can +diff the move separately from the rewrite: + +1. **Characterize first, in its own PR.** Table tests over the mounted + router pin today's behaviour per route: status, code and message of every + refusal, session shape, rate-limit accounting, DB-fault paths. A row that + finds a defect is pinned as-is with a `// known:` comment and a ledger + entry — the slice moves behaviour, it does not fix it. Record per-file + statement coverage of the handler files. +2. **List the gates, then grep the rows.** Before writing the interface, + list every handler statement that runs _before_ the body decode (policy + reads, per-user lockouts, "already enrolled", challenge lookups) and grep + the characterization file for malformed-body rows per route (`{not json`). + A route with such a row keeps its gate as a separate interface method, + ahead of the decode; a route without one decodes first, the service runs + the gate in the original order, and the corner case (garbage input behind + a gate now gets 400 instead of the gate's status) goes in the evidence + block. This was the awkward step on the auth slice: `/register` had the + rows (→ `RegistrationPolicy`), the password-confirmation routes did not. +3. **Interface beside the consumer** — `api/_deps.go`, exported, + one method per route call, named in the service's input/result types + (`service.Principal`, `service.Input`, `service.Result`), never in + `db` types, so the file adds no `db` importer. Method count ≤ the distinct + `*db.DB` methods the handlers called; the after-state table records both + numbers. `var _ Service = (*service.Service)(nil)` pins the + production implementation. +4. **Service owns every decision, verbatim.** `service/.go` takes + `Store` and the collaborators the handlers used (the shared + `auth.RateLimiter`, keys, broadcasters). Lockout keys, enumeration + guards, audit writes, best-effort side effects, partial-success contracts + and the limits that size them move line for line, comments included. Each + refusal is a named `Err*` value whose `Error()` is the public message and + whose category (`ErrRateLimited`, `ErrForbidden`, `ErrBadRequest`, + `ErrConflict`, `ErrInternal`, `ErrUnauthorized`, `ErrInvalidInput`) the + transport maps to a status; return them bare and log the cause in the + service, so nothing leaks through `Error()`. Expect one value per distinct + public message — the auth slice had thirty-one. +5. **Handler = decode, one call, encode.** Validation of the body shape and + format (`ValidateUsername`, size bounds, `SanitizeText`) stays in the + handler; everything that reads state moves. One `writeError` + switch on the categories, with the two or three values that carry their + own code listed first. The principal comes from `principal(r)` in + `middleware.go`, never from a `db` type assertion in the handler file. + `MountRoutes` takes the interface and the `AuthMiddleware` the + caller built; `*db.DB` leaves every signature in the file. +6. **The row leaves with the import.** Delete the family's + `invariants.DBImportAllow` rows in the same commit that removes the + import; `TestDBImportAllowIsLive` fails if a row outlives it, and + `db-import-boundary` fails if an import outlives its row. Constants move + with the code that reads them; a converter that still names a `db` type + (`toUserResponse`) moves to a file that legitimately imports `db`. +7. **Composition root builds the service after its collaborators.** The + auth service needs the hub (broadcast) that `service.New` runs before, so + `router.go` constructs it separately, after `ws.NewHub`. B3-3 moves that + into `internal/app`; until then it is one line in `NewRouter`, which sits + at the `funlen` limit — fold, do not add. +8. **Evidence block:** pre-squash SHAs with the characterization run against + each tree, before/after inventory rows, the full gate, and coverage of + the handler files plus the service (blocks merged per file, each counted + once) — the slice must not drop below its before-figure even when the + per-file handler numbers dip, because the unreachable branches are now a + larger share of a smaller file. + +What the pattern does not promise: the handlers import `service` (types and +categories); the crypto-failure paths remain uninjectable inside the service; +and a family whose gates all precede the decode will spend interface methods +on them. diff --git a/docs/plans/README.md b/docs/plans/README.md index 70bdf896..8d99f33d 100644 --- a/docs/plans/README.md +++ b/docs/plans/README.md @@ -22,7 +22,8 @@ authority**. | [hp-1-scorecard-2026-08-27](hp-1-scorecard-2026-08-27.md) | **HP-1 accepted 2026-08-27.** Structural-diff proofs for the flatten and module rename, plus the B1 exit gate. | | [b2-protocol-trust-compat-2026-08-28](b2-protocol-trust-compat-2026-08-28.md) | **B2 complete — HP-2 accepted 2026-08-29.** B2-0, B2-1, B2-8 done 2026-08-28; B2-2 (B2-3/B2-4 folded in), B2-5, B2-6, B2-7, B2-9 done 2026-08-29. Scorecard below. | | [hp-2-scorecard-2026-08-29](hp-2-scorecard-2026-08-29.md) | **HP-2 accepted 2026-08-29.** Seven questions answered with commands; B2 exit gate, nine conditions met (1 at the slim epoch scope, 4 with one E2EE gap disclaimed). Owner follow-ups that do not gate B3: BPR-051 reader line, SEC-01/SEC-04 advisory IDs. | -| [b3-server-architecture-guardrails-2026-08-29](b3-server-architecture-guardrails-2026-08-29.md) | **B3 in progress from 2026-08-29.** Execution plan: B3-0 inventory → B3-1/B3-2 auth slice → HP-3 → lifecycle, hub options, `ws` split, families; guardrails and the alpha dataset beside the slice. B3-0 (inventory + `db-import-boundary` rule) in review 2026-08-29. | +| [b3-server-architecture-guardrails-2026-08-29](b3-server-architecture-guardrails-2026-08-29.md) | **B3 in progress from 2026-08-29.** Execution plan: B3-0 inventory → B3-1/B3-2 auth slice → HP-3 → lifecycle, hub options, `ws` split, families; guardrails and the alpha dataset beside the slice. B3-0 (inventory + `db-import-boundary` rule) and B3-1 (auth characterization) merged 2026-08-29; B3-2 (auth vertical slice) in review 2026-08-30. | +| [hp-3-scorecard-2026-08-29](hp-3-scorecard-2026-08-29.md) | **HP-3 draft 2026-08-30, awaiting the owner's signature.** Five questions on the auth vertical slice answered with commands: frozen set green at every pre-squash SHA, `api` db importers 12 → 10, B2 contracts unchanged, the pattern written as D4 in server.md, guardrails as they exist. | | [audit-2026-08-19-remediation](audit-2026-08-19-remediation.md) | Phases 1–6 done 2026-08-20; **phase 7 pending**. Its header still reads "in progress 2026-08-19" — stale; the phase table is correct. | ## Partially implemented diff --git a/docs/plans/b3-server-architecture-guardrails-2026-08-29.md b/docs/plans/b3-server-architecture-guardrails-2026-08-29.md index d57a5a4b..f90af996 100644 --- a/docs/plans/b3-server-architecture-guardrails-2026-08-29.md +++ b/docs/plans/b3-server-architecture-guardrails-2026-08-29.md @@ -6,7 +6,7 @@ verified at `bf7b886d` **Status:** in progress — plan merged 2026-08-29 (PR #1447 = `ad4defc2`); B3-0 merged 2026-08-29 (PR #1448 = `d383d8c7`; closes entry-gate item 3); -B3-1 in progress 2026-08-29. +B3-1 merged 2026-08-29 (PR #1449 = `71d867cb`); B3-2 in progress 2026-08-30. Update this line, not only the step table, when a step lands. Primary inputs: @@ -34,7 +34,7 @@ surface to it. | Step | What | Size | Parallel with | | -------- | ---------------------------------------------------------------------------------------------------------------------------------- | -------- | ---------------------------------------------------------------- | | **B3-0** | Boundary inventory: every upper-layer `db` import with a disposition; hub lifecycle; before-graph — **DONE 2026-08-29 (PR #1448)** | 1–2 days | B3-6, B3-7 | -| **B3-1** | Auth characterization tests — enumeration, sentinels, sessions, TOTP, rate limits, failure paths | 1 day | B3-6, B3-7 | +| **B3-1** | Auth characterization tests — enumeration, sentinels, sessions, TOTP, rate limits, failure paths — **DONE 2026-08-29 (PR #1449)** | 1 day | B3-6, B3-7 | | **B3-2** | The auth vertical slice (S-10): route → `service.AuthService` → `db`, behaviour-neutral | 2–3 days | B3-6, B3-7 | | **HP-3** | First vertical-slice review — scorecard | — | — | | **B3-3** | Lifecycle extraction: `main.go` → `internal/app/` with one composite close contract | 1–2 days | B3-4 | @@ -233,7 +233,7 @@ Exit: the characterization file is green on HEAD; its row count and the two files' coverage are in the evidence block. One PR. **Evidence, 2026-08-29** — branch `feat/b3-1-auth-characterization` from `dev` -`d383d8c7`; PR to `dev` recorded below. +`d383d8c7`; PR #1449 to `dev`, squash-merged 2026-08-29 as `71d867cb`. - **Inventory.** The three existing files hold **85** tests (`auth_handler_test.go` 61, `totp_handler_test.go` 22, `auth_handler_delete_broadcast_test.go` 2 — @@ -316,8 +316,8 @@ TO x_gone` for read faults, `RAISE(FAIL)` triggers for write faults), so no 83.3% (`GenerateToken` failure). - **Pre-squash SHAs:** `659c8cbd` (B3-0 SHA recorded), `0905a942` (inventory - table), `b7317d03` (characterization file + ledger + claim updates), plus the - coverage commit that carries this bullet. + table), `b7317d03` (characterization file + ledger + claim updates), `a0356ee1` + (this coverage bullet); Codex rounds `7c38c2bd`, `bf49453c`, `8614603b` (head). - Gates before every commit: `check:docs`, `check:hygiene`; from `Server/` the four build-tag variants, `go vet`, `go test -race ./...`, `go test -tags deadlock ./ws/`, `golangci-lint run` (first run tripped `prealloc` and @@ -359,6 +359,119 @@ rewrite, remove old path — with B3-1's tests green after every commit. Exit: HP-3. +**Evidence, 2026-08-30** — branch `feat/b3-2-auth-slice` from `dev` +`71d867cb`; PR #1450 to `dev` (squash SHA recorded at merge). + +- **Pre-squash SHAs**, one per numbered item. For each, the characterization + file was run against that exact tree in a detached worktree + (`go test -count=1 -run TestAuthCharacterization ./api/`) and the frozen + files (`auth_characterization_test.go`, `totp_handler_test.go`) diffed + against `71d867cb`: + + | Commit | Item | Characterization | Frozen files vs `71d867cb` | + | ----------- | --------------------------------------------------------------------------------------------------------------------------------------------- | ---------------- | -------------------------- | + | `825b406f` | B3-1's merge SHA recorded in this plan | ok 1.860 s | identical | + | `448c50f6` | 1 — `api/auth_deps.go`: the consumer-owned interface, and the input/result types it names in `service/auth.go` | ok 1.852 s | identical | + | `24ed138d` | 2 — `service/auth.go`: orchestration moved verbatim, the `Err*` set; `auth/ratescale.go` | ok 1.854 s | identical | + | `fe1d11b8` | 3 — thin handlers; `MountAuthRoutes(r, svc, requireAuth, limiter, proxies)`; `router.go` builds the service; two `DBImportAllow` rows deleted | ok 1.849 s | identical (see wiring) | + | `3f0d24ec` | 4 — after-state inventory and graph rows in `server-boundaries.md` | ok 1.832 s | identical | + | this commit | 5 — this block | docs only | — | + +- **The interface.** Nine methods — `RegistrationPolicy`, `Register`, + `Login`, `VerifyTOTP`, `Logout`, `DeleteAccount`, `EnableTOTP`, + `ConfirmTOTP`, `DisableTOTP` — against the ten distinct `*db.DB` methods, + two `db` functions and two `db` sentinels the handlers called at + `71d867cb`. `RegistrationPolicy` is the ninth: two characterization rows + pin `/register`'s 403 "before any credential is read" (a malformed body + still receives the policy's 403), so that gate stays ahead of the body + decode as its own call. `service.AuthService` is constructed with the + `Store` (`*db.DB`), the shared `auth.RateLimiter`, the TOTP key and the + `AuthBroadcaster`; it creates the partial-login, pending-TOTP and used-code + stores itself with the fixed TTLs the route mount used to own. +- **Error semantics** (item 4): every refusal is a named `service.Err*` value + whose `Error()` is the exact public message the handler wrote and whose + category (`ErrRateLimited`, `ErrForbidden`, `ErrBadRequest`, `ErrConflict`, + `ErrInternal`, plus the new `ErrUnauthorized` and `ErrInvalidInput`) the one + `writeAuthError` switch maps to a status and code; two values carry their + own code (`INVALID_CREDENTIALS`, `TOTP_ALREADY_ENABLED`). Thirty-one values, + because the pre-slice handlers had thirty-one distinct refusal triples. No + sentinel-mapping row changed. +- **Behaviour notes** — no row changed; listed because HP-3 Q1 asks: + 1. _Decode before gate on five routes._ At `71d867cb`, `DELETE /account`, + the three TOTP-management routes and `/verify-totp` ran a gate (per-user + lockout; already-enrolled; challenge lookup) before decoding the body. A + thin handler decodes first and the service runs the same gates in the + same order. Observable only for a _malformed_ body behind a gate: + locked-out + malformed → 400 (was 429); enrolled + malformed enable → + 400 (was 409); unknown partial token + malformed verify body → 400 (was + 401). No frozen row exercises those; every well-formed request is + byte-identical. `/register` kept its gate first (above). + 2. _One `AuthMiddleware` for the six authenticated auth routes._ The caller + builds it once and passes it in; before, each `r.With(AuthMiddleware(database))` + had its own `last_used` touch throttle. Fewer writes, same semantics. + 3. `confirmPassword` folds the three identical password-confirmation blocks + of the TOTP routes into one method (verbatim otherwise; the accounting + — a missing or wrong password counts, a correct one resets — is + unchanged). + 4. The auth-slice limits left `api/constants.go` with the code that reads + them; the shared `pw_confirm` budget is exported as `service.PwConfirm*` + and `profile_handler.go` reads it there. The auth rate multiplier moved + to `auth/ratescale.go` (`SetRateScale`, `ScaledLimit`) so the route + mounts and the service's login accounting read one value; `api` keeps + `setAuthRateScale`/`scaledAuthLimit` as wrappers, and + `TestPerUserFailureCapsStayUnscaled` now pins `../service/auth.go`. + 5. `userResponse`/`toUserResponse` moved to `profile_handler.go` (the other + consumer, which still imports `db`); `middleware.go` gained + `principal(r)`, which hands the handlers the context's `*db.User` and + `*db.Session` as `service.Principal`. +- **Test files changed** — wiring only (`git diff -U0 71d867cb fe1d11b8 -- '*_test.go'`): + `auth_handler_test.go` (+1 import, the one mount line in + `buildAuthRouterWithProxies`), `auth_handler_delete_broadcast_test.go` (+1 + import, two mount lines, one comment), `coverage_push_test.go` and + `invite_handler_test.go` (one mount line each), `constants_test.go` (the + scale tests follow the constant), `router_delete_account_broadcast_test.go` + (comment and failure message), `invariants/db_import_boundary_test.go` + (fixture path). `auth_characterization_test.go` and `totp_handler_test.go`: + untouched. No assertion, row or expected value moved. +- **Graph tables:** [server-boundaries.md](../architecture/server-boundaries.md) + §"Auth slice" — before at `ad4defc2`, after at `fe1d11b8`. `api` `db` + importers **12 → 10**, `move` rows 28 → 26, 51 → 49 files. The handlers + import `service` (the interface's types, the `Err*` categories, + `SanitizeText`), so the plan's "neither `db` nor `service`" is met for + `db` only — stated there, not bent. +- **Gates**, before every commit per `ci-check`: from `Server/` the four + build-tag variants, `go vet ./...`, `go test -race ./...` (18 packages + ok), `go test -tags deadlock -count=1 ./ws/` (ok, 59.9 s), + `golangci-lint run` (0 issues — one `funlen` on `NewRouter` at 101 lines + during item 3, fixed by folding the constructor into the mount call), + `sqlc generate` and `genprotocol` drift clean, `go test ./invariants/` ok; + from the root `check:docs` and `check:hygiene`. + `go test -count=1 -race ./api/ ./service/ ./auth/` at `3f0d24ec`: + `ok api 51.447s`, `ok service 84.403s`, `ok auth 1.802s`. +- **Coverage** (statements; `go test -coverprofile` blocks merged per file — + before is `-coverprofile ./api/` at `71d867cb`, after is the same run for + the handler files plus `-coverpkg=./api/,./service/ ./api/ ./service/` for + the slice, each profile block counted once): + + | File | Before (`71d867cb`) | After (`fe1d11b8`) | + | ------------------------------ | ------------------- | ------------------- | + | `api/auth_handler.go` | 229/254 = 90.2% | 98/114 = 86.0% | + | `api/totp_handler.go` | 163/179 = 91.1% | 54/60 = 90.0% | + | `service/auth.go` | — | 240/253 = 94.9% | + | **slice (handlers + service)** | **392/433 = 90.5%** | **392/427 = 91.8%** | + + The same 392 statements are exercised after the move; the total shrank by + six (the folded confirmation blocks). The handler files' percentages dip + because their unreachable branches are now a larger share of a smaller + file: `writeNotAuthenticated` (the no-principal branch behind + `AuthMiddleware` — B3-1's `handleMe` note), `writeAuthError`'s default (a + service contract bug, unreachable by construction), and + `registerReadRequest`/`loginReadRequest`, unchanged at 78.9%/83.3% (B3-1's + note: those branches are `auth/` tests). The service's gaps are B3-1's + not-injectable set: `Register` 85.0% (hash failure, post-commit + `GetUserByID`), `issueSession` 83.3% (`GenerateToken`), `Logout` 90.0% (the + nil-session guard the handler answers first). + ## HP-3 — First vertical-slice review `docs/plans/hp-3-scorecard-.md`, in the HP-2 shape. Questions: diff --git a/docs/plans/hp-3-scorecard-2026-08-29.md b/docs/plans/hp-3-scorecard-2026-08-29.md new file mode 100644 index 00000000..9d221287 --- /dev/null +++ b/docs/plans/hp-3-scorecard-2026-08-29.md @@ -0,0 +1,231 @@ +# HP-3 — First vertical-slice review scorecard + +**Hold point:** HP-3, defined in +[b3-server-architecture-guardrails-2026-08-29.md](b3-server-architecture-guardrails-2026-08-29.md) +§HP-3 (roadmap +[repo-health-roadmap-2026-08-23.md](repo-health-roadmap-2026-08-23.md), B3) +**Commits reviewed:** the pre-squash commits of #1450 (B3-2; table below), on top of B3-0 (#1448 = `d383d8c7`) and B3-1 (#1449 = `71d867cb`) +**Measured at:** `fe1d11b8` (the handler-move commit) and `3f0d24ec` (the +after-state inventory), branch `feat/b3-2-auth-slice` off `dev` `71d867cb` +**Measured:** 2026-08-30 +**Evidence base:** the plan's B3-0, B3-1 and B3-2 evidence blocks; +[server-boundaries.md](../architecture/server-boundaries.md) §"Auth slice"; +[server.md](../architecture/server.md) §D4 + +**Decision: DRAFT — awaiting the owner's signature (line at the end).** + +HP-3 asks five questions. Each is answered below with the command that +produces the evidence and what it printed on the measured tree, not with an +assertion. It follows the shape of +[hp-2-scorecard-2026-08-29.md](hp-2-scorecard-2026-08-29.md). Acceptance +authorises B3-3 onward and B3-8's per-family repeats of the pattern; it +claims nothing about beta readiness. + +## The commits under review are not on `dev` + +`dev` is squash-merge only. The structure HP-3 reviews — one commit per +numbered item of §B3-2, characterization green after each — survives only on +the pull-request ref: + +```bash +git fetch origin 'refs/pull/1450/head:pr-1450' +``` + +| Commit | §B3-2 item | +| ----------- | -------------------------------------------------------------------------------------------------------------------------- | +| `825b406f` | — B3-1's merge SHA `71d867cb` recorded in the plan | +| `448c50f6` | 1 — `api/auth_deps.go`, the consumer-owned `AuthService` interface; its input/result types in `service/auth.go` | +| `24ed138d` | 2 — `service/auth.go`, the orchestration moved verbatim and the `Err*` set; `auth/ratescale.go` | +| `fe1d11b8` | 3 — thin handlers, `MountAuthRoutes` takes the interface, `router.go` builds the service, two `DBImportAllow` rows deleted | +| `3f0d24ec` | 4 — after-state inventory and graph rows in `server-boundaries.md` | +| `1077a992` | 5 — the evidence block | +| this commit | 6 — this scorecard and the pattern section in `server.md` | + +## Question 1 — did the slice move behaviour without changing it? + +**The proof is the frozen set.** `Server/api/auth_characterization_test.go` +(12 tests, 44 rows) and the 85 tests in `auth_handler_test.go`, +`totp_handler_test.go` and `auth_handler_delete_broadcast_test.go` were +written before a line of the slice moved (B3-1) and did not change here. + +```bash +# each pre-squash SHA, in a detached worktree of that exact tree +cd Server && go test -count=1 -run TestAuthCharacterization ./api/ +git diff --stat 71d867cb -- Server/api/auth_characterization_test.go Server/api/totp_handler_test.go +``` + +| SHA | Characterization | Frozen files vs `71d867cb` | +| ---------- | ---------------- | -------------------------- | +| `825b406f` | ok 1.860 s | identical | +| `448c50f6` | ok 1.852 s | identical | +| `24ed138d` | ok 1.854 s | identical | +| `fe1d11b8` | ok 1.849 s | identical | +| `3f0d24ec` | ok 1.832 s | identical | + +The 85 tests: `go test -count=1 -race ./api/` at `3f0d24ec` — `ok 51.447s`, +the whole package. Of the frozen files, `auth_handler_test.go` and +`auth_handler_delete_broadcast_test.go` changed only where they mount the +routes (one helper line, two direct mounts, one import each, one comment — +`git diff -U0 71d867cb fe1d11b8 -- Server/api/auth_handler_test.go Server/api/auth_handler_delete_broadcast_test.go`); +no assertion or expected value moved. + +**The sentinel-mapping table is byte-identical.** Every refusal the handlers +wrote at `71d867cb` — thirty-one distinct (status, code, message) triples — +is now a named `service.Err*` value whose `Error()` is that message and whose +category the one `writeAuthError` switch maps back to that status and code. +The rows that pin them (`RegisterRejectionsAreIndistinguishable`, +`LoginRejectionsAreIndistinguishable`, `LoginFailurePaths`, +`DeleteAccountFailurePaths`, `VerifyTOTPFailurePaths`, +`TOTPManagementFailurePaths`, `RouteRateLimits`) assert status, code and +message with `wantErr` and are green above. The three `// known:` rows +(OC-0376, OC-0377, OC-0378) are still pinned as defects — the slice moved +them, it did not fix them; B3-9 flips each row in the commit that fixes it. + +**What did change, recorded rather than hidden** (the plan's B3-2 evidence +block, "Behaviour notes"): five routes that ran a gate before decoding the +body now decode first and let the service run the same gates in the same +order. The only observable difference is a _malformed_ body arriving behind +a gate (locked-out, already-enrolled, unknown challenge): it is refused as +400 where it was refused as the gate's 429/409/401. No row exercises that +input; every well-formed request is byte-identical. `/register` kept its +gate first because two rows pin "403 before any credential is read" — that +is why the interface has a ninth method, `RegistrationPolicy`. The six +authenticated auth routes share one `AuthMiddleware` instance (one +`last_used` throttle instead of six). + +**Verdict: PASS** — the frozen behaviour set is unchanged and green at every +commit; the one ordering change is confined to malformed input behind a gate +and is written down. + +## Question 2 — did it reduce coupling? + +```bash +cd Server && go run ./cmd/dbinventory | tail -2 +# before (ad4defc2, B3-0): 51 files import db ... api 12 ...; Dispositions: adapter 17, boundary 6, move 28 +# after (fe1d11b8): 49 files import db ... api 10 ...; Dispositions: adapter 17, boundary 6, move 26 +go test -count=1 ./invariants/ # ok — db-import-boundary and TestDBImportAllowIsLive at the new list +``` + +| Measure | Before (`ad4defc2`) | After (`fe1d11b8`) | +| ------------------------------------------- | ------------------------------------------------------------------------------- | ----------------------------------------------------------------------------------- | +| `db` importers in `api` | 12 | **10** | +| `DBImportAllow` `move` rows | 28 | 26 | +| `db` symbols the two handlers touched | 3 types, 2 funcs, 2 sentinels, **10 distinct `*db.DB` methods** (11 call sites) | **none** — `auth_handler.go` and `totp_handler.go` import `auth` and `service` only | +| Interface the handlers call | — | `api.AuthService`, **9 methods** (`api/auth_deps.go`) | +| Where the ten `db` methods live | the handlers | `service/auth.go`, through `service.Store` | +| Handler size (statements, `go test -cover`) | `auth_handler.go` 254, `totp_handler.go` 179 | 114 and 60; `service/auth.go` 253 | +| Direction | `api → db` (handlers) beside `api → service → db` (the rest) | `api → service → db` for the whole slice | + +The full before/after tables are `server-boundaries.md` §"Auth slice — +before-state dependency graph" and §"after-state dependency graph". The +after-state also records what is _not_ met: the plan hoped the handlers would +import neither `db` nor `service`; they import `service` for the interface's +input/result types, the `Err*` categories and `SanitizeText`. The +alternative — an `api`-side copy of every type — would be a second definition +of the same shapes, so the dependency is kept and named. + +**Verdict: PASS** — nine methods replace ten database methods plus four +symbols; two importers gone; the rule that proves it (`db-import-boundary`) +is green with the rows deleted in the same commit as the imports. + +## Question 3 — did it weaken a B2 contract? + +Nothing under `ws/`, `protocol/`, `permissions/` or the fixtures changed on +the branch (`git diff --stat 71d867cb 3f0d24ec -- Server/ws Server/permissions protocol` is empty). The +tests HP-2 accepted, run at `3f0d24ec`: + +```bash +cd Server && go test -count=1 -run 'TestEpoch1Fixtures|TestAuth_ProtocolEpoch|TestAbsenceContract_|Parity' -v ./ws/ ./api/ ./service/ +``` + +| Contract | Test | Result | +| ------------------------------------------ | ----------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------- | -------- | +| Epoch-1 fixtures replay, `auth-failure` | `TestEpoch1Fixtures/auth-failure` | PASS | +| Epoch-1 fixtures replay, `fresh-connect` | `TestEpoch1Fixtures/fresh-connect` (and the other nine: ping, chat-send-fanout, chat-edit-delete, reaction-add-remove, typing, mark-read, dm-send, resume-replay, voice-join-e2ee-leave) | PASS ×11 | +| Epoch negotiation matrix | `TestAuth_ProtocolEpoch` | PASS | +| Absence (federation / directory / listing) | `TestAbsenceContract_NoFederationDirectoryOrListingRoutes`, `…WireTypes`, `…ConfigKeys` | PASS ×3 | +| Predicate parity | `TestSendPolicyParity`, `TestViewPolicyParity` (service); `TestRefreshChannelVisibilityCanSend_Parity`, `TestApplySetChannelID_Parity`, `TestChannelReadAudience_Parity`, `TestChannelSubject_Parity`, `TestVoiceJoinPrecheck_Parity`, `TestVoiceStillAllowed_Parity`, `TestRefreshChannelVisibility_Parity` (ws) | PASS ×9 | + +`ok ws 7.658s`, `ok api 1.423s`, `ok service 0.717s`. The auth rate +multiplier moved packages (`auth.SetRateScale`/`ScaledLimit`), and +`api/constants_test.go`'s clamp and floor tests still run through `api`'s +wrappers — `TestSetAuthRateScale_ClampsMultiplier`, +`TestScaledAuthLimit_NeverBelowOne`, `TestPerUserFailureCapsStayUnscaled` +green in the `-race` run above. + +**Verdict: PASS** — every B2 contract test is unchanged and green. + +## Question 4 — is the pattern repeatable? + +Written down as the rule for B3-8 in +[server.md](../architecture/server.md) §"D4 — The vertical-slice pattern": +interface beside the consumer → service owns every decision → handler is +decode, one call, encode → the allowlist row leaves with the import → the +limits and converters move with the code that reads them → the composition +root builds the service after the collaborators it needs. + +**The one thing that was awkward, named honestly:** _gate-before-decode._ +Six of the nine routes checked something before reading the body — a policy, +a per-user lockout, "already enrolled", a challenge lookup. A thin handler +cannot keep that order without a second interface method per route, and the +interface had a method-count cap. The characterization file settled it +route by route: `/register` had two rows pinning the gate ahead of the body +(a malformed body still gets the policy's 403), so its gate became the ninth +method; the other five had none, decode first, and the corner cases are in +the evidence block. The general rule (D4, step 2): _before writing the +interface, list every statement that precedes the body decode and grep the +characterization file for malformed-body rows per route._ B3-8's families +will meet the same shape (every "confirm your password" route has it). + +A smaller cost worth knowing before the next family: thirty-one named error +values, because the pre-slice handlers had thirty-one distinct public +messages and the rows pin them byte for byte. That is the price of +"verbatim"; it is also a list a later phase can consolidate on purpose. + +**Verdict: PASS** — the shape is written as steps with the awkward step +named, and every step was exercised once on this slice. + +## Question 5 — are the B3-6 guardrails green on the slice's SHA? + +B3-6 did not land in parallel: no guardrail PR merged to `dev` between +`71d867cb` and this branch. What exists today, run at `fe1d11b8`: + +| Guardrail | State | +| ------------------------------------------------- | ------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------- | +| `db-import-boundary` (B3-0, `Server/invariants/`) | **green** — `go test -count=1 ./invariants/` ok with the two rows deleted; `go run ./cmd/dbinventory` exits 0, "0 unlisted" | +| Coverage floor (S-06, B3-6 item 1) | **not yet a gate** — measured by hand instead: slice 392/433 = 90.5% → 392/427 = 91.8% (B3-2 evidence block); the same 392 statements exercised. B3-6 turns the measurement into a check | +| Hub simulation / fault transport / fuzz seeds | **not landed** — nothing to run; the slice touches no `ws/` file, so no new surface for them either | +| The full server gate | **green** before every commit: four build-tag variants, vet, `-race` (18 packages), `-tags deadlock ./ws/`, `golangci-lint` 0 issues, sqlc/genprotocol drift clean, `check:docs`, `check:hygiene` | + +**Verdict: PASS for the guardrail that exists; the rest is B3-6's own exit, +not this slice's.** Recorded so the exit gate's condition 5 can point at +this table and at B3-6's later run on the same SHA. + +## Open items carried past HP-3 + +Recorded, not fixed. None blocks B3-3. + +| Item | State | +| -------------------------------------------------------------- | ---------------------------------------------------------------------------------------------------------------------------------------------------------------- | +| OC-0376, OC-0377, OC-0378 (B3-1's `// known:` rows) | **B3-9, now unblocked** — the orchestration they live in is `service.AuthService`; each fix flips its row in the same commit | +| Decode-before-gate corner cases (malformed body behind a gate) | **recorded** in the B3-2 evidence block; not a defect, a shape. If a later phase wants the gate's status back for garbage input, it is one more interface method | +| Handlers import `service` | **accepted** — types and error categories, not persistence; `server-boundaries.md` says so | +| B3-1's not-injectable set (crypto failures, post-commit fetch) | **still not injectable** — they sit inside the service now; a `Store` double would reach the post-commit `GetUserByID`, `auth.GenerateToken` still has no seam | +| Thirty-one auth `Err*` values | **by design** (verbatim); consolidation would be a behaviour change and a later phase's decision | +| Coverage floor as a gate | **B3-6** — the per-file block-merge measurement in the evidence block is the method to encode | + +## What acceptance does and does not authorise + +Accepting HP-3 authorises B3-3 (lifecycle extraction), B3-4, B3-5 and the +per-family repeats in B3-8 to proceed with the D4 pattern. It does **not** +claim: + +- that the auth slice is bug-free — three known defects are pinned and + scheduled (B3-9); +- that every handler is thin — ten `api` files still import `db`, each with + a disposition in `server-boundaries.md`; +- that the coverage floor is enforced — it is measured, B3-6 makes it a + gate; +- that the error set is final — it is verbatim. + +**Signed:** \_\_\_\_ (repository owner), 2026-\_\_-\_\_ —