diff --git a/audit/events.yaml b/audit/events.yaml index c99a232b..d861039a 100644 --- a/audit/events.yaml +++ b/audit/events.yaml @@ -1063,11 +1063,19 @@ events: - code: admin.user.enabled severity: warning - description: An administrator re-enabled a previously disabled user account. + description: >- + An administrator called enable on a user account. transition is true + when the call changed the account from disabled to enabled, and false + when the account was already enabled. revocation_scope is interactive + when the transition revoked the user's sessions and refresh tokens, so + the user signs in again, and none when nothing was revoked. Service + account tokens are not revoked or restored by enable. detail_schema: type: object properties: target_user_id: {type: string} + transition: {type: boolean} + revocation_scope: {type: string, enum: [interactive, none]} - code: admin.role.changed severity: warning diff --git a/internal/audit/events.gen.go b/internal/audit/events.gen.go index 3cf5c20a..251cdde5 100644 --- a/internal/audit/events.gen.go +++ b/internal/audit/events.gen.go @@ -303,7 +303,7 @@ const ( AdminUserPasswordReset Code = "admin.user.password_reset" // An administrator disabled a user account (cannot authenticate). AdminUserDisabled Code = "admin.user.disabled" - // An administrator re-enabled a previously disabled user account. + // An administrator called enable on a user account. transition is true when the call changed the account from disabled to enabled, and false when the account was already enabled. revocation_scope is interactive when the transition revoked the user's sessions and refresh tokens, so the user signs in again, and none when nothing was revoked. Service account tokens are not revoked or restored by enable. AdminUserEnabled Code = "admin.user.enabled" // AdminRoleChanged Code = "admin.role.changed" @@ -1462,9 +1462,9 @@ var Metadata = map[Code]EventMeta{ Code: AdminUserEnabled, Category: "admin", Severity: SeverityWarning, - Description: `An administrator re-enabled a previously disabled user account.`, + Description: `An administrator called enable on a user account. transition is true when the call changed the account from disabled to enabled, and false when the account was already enabled. revocation_scope is interactive when the transition revoked the user's sessions and refresh tokens, so the user signs in again, and none when nothing was revoked. Service account tokens are not revoked or restored by enable.`, ActorTypes: nil, - DetailKeys: []string{"target_user_id"}, + DetailKeys: []string{"revocation_scope", "target_user_id", "transition"}, }, AdminRoleChanged: { Code: AdminRoleChanged, diff --git a/internal/server/api_users_admin_test.go b/internal/server/api_users_admin_test.go index 68ca6561..9c35eb71 100644 --- a/internal/server/api_users_admin_test.go +++ b/internal/server/api_users_admin_test.go @@ -10,14 +10,19 @@ // AC-17 TestAPI_AdminDisableEnable (enable half) // AC-18 TestAPI_AdminDisableEnable (self-disable guard) // AC-19 TestAPI_AdminUserMgmt_NotFoundAndRBAC +// AC-20 TestAPI_AdminEnable_AuditRecordsTheTransition package server import ( "context" + "encoding/json" "net/http" + "strings" "testing" + "time" "github.com/google/uuid" + "github.com/jackc/pgx/v5/pgxpool" "github.com/Hanalyx/openwatch/internal/auth" "github.com/Hanalyx/openwatch/internal/identity" @@ -174,12 +179,28 @@ func TestAPI_AdminDisableEnable(t *testing.T) { }) t.Run("api-users/AC-17", func(t *testing.T) { + // Credentials that exist while the account is disabled and that no + // revocation reached. Without these the criterion passes whether or + // not enable revokes anything, because disable already revoked the + // login's own session. + stray := writeStrayCredentials(t, pool, target.ID) + // enable -> clears disabled_at; the user can authenticate again er := doReq(t, asRole(t, "POST", url+"/api/v1/users/"+target.ID.String()+":enable", auth.RoleAdmin, nil)) er.Body.Close() if er.StatusCode != http.StatusOK { t.Fatalf("enable = %d, want 200", er.StatusCode) } + // ... but only through a fresh sign-in (C-07, 1.4.0). + if code := authMe(t, url, stray.sessionCookie); code != http.StatusUnauthorized { + t.Errorf("pre-enable session cookie after enable = %d, want 401", code) + } + if code := authMeBearer(t, url, stray.accessToken); code != http.StatusUnauthorized { + t.Errorf("pre-enable access token after enable = %d, want 401", code) + } + if code, _ := refreshBody(t, url, stray.bodyRefresh); code == http.StatusOK { + t.Error("pre-enable refresh token rotated after enable") + } reLogin := login(t, url, map[string]string{"username": target.Username, "password": target.Password}) reLogin.Body.Close() if reLogin.StatusCode != http.StatusOK { @@ -230,3 +251,97 @@ func TestAPI_AdminUserMgmt_NotFoundAndRBAC(t *testing.T) { } }) } + +// auditDetailFor returns the detail of the ONE row with this action that +// the request carrying this correlation id produced. It waits, bounded, +// because the audit writer batches, and fails rather than fall back to +// another request's row. +func auditDetailFor(t *testing.T, pool *pgxpool.Pool, action, correlationID string) map[string]any { + t.Helper() + deadline := time.Now().Add(10 * time.Second) + for { + rows, err := pool.Query(context.Background(), + `SELECT COALESCE(detail::text, '{}') FROM audit_events WHERE action = $1 AND correlation_id = $2`, + action, correlationID) + if err != nil { + t.Fatalf("read audit: %v", err) + } + var details []string + for rows.Next() { + var d string + if err := rows.Scan(&d); err != nil { + t.Fatalf("scan audit: %v", err) + } + details = append(details, d) + } + rows.Close() + if len(details) > 1 { + t.Fatalf("request %s recorded %d %s rows, want 1", correlationID, len(details), action) + } + if len(details) == 1 { + var m map[string]any + if err := json.Unmarshal([]byte(details[0]), &m); err != nil { + t.Fatalf("decode detail: %v", err) + } + return m + } + if time.Now().After(deadline) { + t.Fatalf("request %s recorded no %s within 10s", correlationID, action) + } + time.Sleep(50 * time.Millisecond) + } +} + +// @ac AC-20 +// AC-20: admin.user.enabled records whether the request changed the +// account, read from the request's own audit row. +func TestAPI_AdminEnable_AuditRecordsTheTransition(t *testing.T) { + t.Run("api-users/AC-20", func(t *testing.T) { + url, pool := freshAPIServer(t) + ctx := context.Background() + svc := users.NewService(pool, nil) + for _, tc := range []struct { + name string + disable bool + transition bool + scope string + }{ + {"transition", true, true, "interactive"}, + {"already enabled", false, false, "none"}, + } { + tc := tc + t.Run(tc.name, func(t *testing.T) { + li := loginFresh(t, url, pool, "ac20"+strings.ReplaceAll(tc.name, " ", "")) + if tc.disable { + if err := svc.Disable(ctx, li.u.ID); err != nil { + t.Fatalf("disable: %v", err) + } + } + req := asRole(t, "POST", url+"/api/v1/users/"+li.u.ID.String()+":enable", auth.RoleAdmin, nil) + cid := "ac20-" + strings.ReplaceAll(uuid.NewString(), "-", "") + req.Header.Set("X-Correlation-Id", cid) + resp := doReq(t, req) + resp.Body.Close() + if resp.StatusCode != http.StatusOK { + t.Fatalf("enable = %d, want 200", resp.StatusCode) + } + for _, c := range resp.Cookies() { + if c.Name == identity.SessionCookieName || c.Name == identity.RefreshCookieName { + t.Errorf("enable set a credential cookie %q", c.Name) + } + } + d := auditDetailFor(t, pool, "admin.user.enabled", cid) + if d["transition"] != tc.transition || d["revocation_scope"] != tc.scope { + t.Errorf("audit detail = transition %v, revocation_scope %v; want %v, %q", + d["transition"], d["revocation_scope"], tc.transition, tc.scope) + } + if d["target_user_id"] != li.u.ID.String() { + t.Errorf("audit target_user_id = %v, want %s", d["target_user_id"], li.u.ID) + } + if !tc.disable && authMe(t, url, li.sessionCookie) != http.StatusOK { + t.Error("a no-op enable signed the user out") + } + }) + } + }) +} diff --git a/internal/server/enable_revocation_test.go b/internal/server/enable_revocation_test.go new file mode 100644 index 00000000..402657bf --- /dev/null +++ b/internal/server/enable_revocation_test.go @@ -0,0 +1,340 @@ +// @spec system-auth-identity +// +// Re-enabling an account (I10). A disabled-to-enabled transition revokes +// every interactive credential the user holds, and a call on an account +// that is not disabled changes nothing. +// +// The stray credentials in AC-70 are written directly for the disabled +// user with the identity package's issue functions, bypassing every +// handler and the lock. That is deliberate: it measures what Enable does +// with state an older release or a leaked issuance path could leave, which +// no amount of issuance prevention reaches. It does not show that a +// current handler issues credentials while an account is disabled. + +package server + +import ( + "context" + "errors" + "net/http" + "testing" + "time" + + "github.com/Hanalyx/openwatch/internal/apitoken" + "github.com/Hanalyx/openwatch/internal/auth" + "github.com/Hanalyx/openwatch/internal/identity" + "github.com/Hanalyx/openwatch/internal/users" + "github.com/google/uuid" + "github.com/jackc/pgx/v5/pgxpool" +) + +// strayCredentials are interactive credentials that exist for a user +// without ever having passed through a handler. +type strayCredentials struct { + sessionID uuid.UUID + sessionCookie *http.Cookie + accessToken string + bodyRefresh string + cookieRefresh *http.Cookie +} + +func writeStrayCredentials(t *testing.T, pool *pgxpool.Pool, uid uuid.UUID) strayCredentials { + t.Helper() + ctx := context.Background() + tok, sess, err := identity.IssueSession(ctx, pool, uid, "127.0.0.1", "stray") + if err != nil { + t.Fatalf("write stray session: %v", err) + } + access, _, err := identity.IssueJWTForSession(uid, "viewer", sess.ID) + if err != nil { + t.Fatalf("mint stray access token: %v", err) + } + body, err := identity.IssueRefreshTokenForSession(ctx, pool, uid, sess.ID, sess.AbsoluteExpiresAt) + if err != nil { + t.Fatalf("write stray body refresh token: %v", err) + } + cookie, err := identity.IssueRefreshTokenForSession(ctx, pool, uid, sess.ID, sess.AbsoluteExpiresAt) + if err != nil { + t.Fatalf("write stray cookie refresh token: %v", err) + } + return strayCredentials{ + sessionID: sess.ID, + sessionCookie: &http.Cookie{Name: identity.SessionCookieName, Value: tok}, + accessToken: access, + bodyRefresh: body, + cookieRefresh: &http.Cookie{Name: identity.RefreshCookieName, Value: cookie}, + } +} + +func isDisabled(t *testing.T, pool *pgxpool.Pool, uid uuid.UUID) bool { + t.Helper() + var disabled bool + if err := pool.QueryRow(context.Background(), + `SELECT disabled_at IS NOT NULL FROM users WHERE id = $1`, uid).Scan(&disabled); err != nil { + t.Fatalf("read disabled_at: %v", err) + } + return disabled +} + +// freshLoginWorks signs in through the real login handler and proves the +// new session authenticates. +func freshLoginWorks(t *testing.T, url string, u authTestUser) bool { + t.Helper() + resp := login(t, url, map[string]string{"username": u.Username, "password": u.Password}) + resp.Body.Close() + if resp.StatusCode != http.StatusOK { + t.Logf("fresh login = %d", resp.StatusCode) + return false + } + return getMeWithCookie(t, url, sessionCookie(resp)) == http.StatusOK +} + +// @ac AC-70 +// AC-70: a real enable transition revokes every interactive credential, +// including ones no revocation ever reached, atomically and under the lock. +func TestEnable_TransitionRevokesInteractiveCredentials(t *testing.T) { + t.Run("system-auth-identity/AC-70", func(t *testing.T) { + url, pool := freshAPIServer(t) + ctx := context.Background() + svc := users.NewService(pool, nil) + + t.Run("transition", func(t *testing.T) { + li := loginFresh(t, url, pool, "enabletransition") + if err := svc.Disable(ctx, li.u.ID); err != nil { + t.Fatalf("disable: %v", err) + } + stray := writeStrayCredentials(t, pool, li.u.ID) + if s, r := liveCounts(t, pool, li.u.ID); s != 1 || r != 2 { + t.Fatalf("precondition: stray live credentials = %d/%d, want 1/2", s, r) + } + + transitioned, err := svc.Enable(ctx, li.u.ID) + if err != nil { + t.Fatalf("enable: %v", err) + } + if !transitioned { + t.Error("enable of a disabled account reported no transition") + } + if isDisabled(t, pool, li.u.ID) { + t.Fatal("enable did not clear disabled_at") + } + // Attributed positively: the stray session row itself is revoked. + var sessionRevoked bool + if err := pool.QueryRow(ctx, + `SELECT revoked_at IS NOT NULL FROM sessions WHERE id = $1`, stray.sessionID).Scan(&sessionRevoked); err != nil { + t.Fatalf("read stray session: %v", err) + } + if !sessionRevoked { + t.Error("the stray session is still live after enable") + } + if s, r := liveCounts(t, pool, li.u.ID); s != 0 || r != 0 { + t.Errorf("live credentials after enable = %d/%d, want 0/0", s, r) + } + + if code := authMe(t, url, stray.sessionCookie); code != http.StatusUnauthorized { + t.Errorf("stray session cookie after enable = %d, want 401", code) + } + if code := authMeBearer(t, url, stray.accessToken); code != http.StatusUnauthorized { + t.Errorf("stray access token after enable = %d, want 401", code) + } + if code, _ := refreshBody(t, url, stray.bodyRefresh); code == http.StatusOK { + t.Error("stray body refresh token rotated after enable") + } + if code, sess := refreshCookie(t, url, stray.cookieRefresh); code == http.StatusOK || sess != nil { + t.Errorf("stray refresh cookie after enable = %d (session minted: %v)", code, sess != nil) + } + if !freshLoginWorks(t, url, li.u) { + t.Error("a fresh login after enable did not authenticate") + } + }) + + t.Run("controlled failure", func(t *testing.T) { + li := loginFresh(t, url, pool, "enablefailure") + if err := svc.Disable(ctx, li.u.ID); err != nil { + t.Fatalf("disable: %v", err) + } + writeStrayCredentials(t, pool, li.u.ID) + restore := failOnWrite(t, pool, "refresh_tokens", "UPDATE") + transitioned, err := svc.Enable(ctx, li.u.ID) + restore() + if transitioned { + t.Error("a failed enable reported a transition") + } + if err == nil { + t.Fatal("enable succeeded although its revocation failed") + } + if !isDisabled(t, pool, li.u.ID) { + t.Error("disabled_at was cleared without the revocation") + } + if s, r := liveCounts(t, pool, li.u.ID); s != 1 || r != 2 { + t.Errorf("live credentials after a failed enable = %d/%d, want 1/2 untouched", s, r) + } + }) + + t.Run("serialized", func(t *testing.T) { + li := loginFresh(t, url, pool, "enableserialized") + if err := svc.Disable(ctx, li.u.ID); err != nil { + t.Fatalf("disable: %v", err) + } + holder, err := pool.Begin(ctx) + if err != nil { + t.Fatalf("begin holder: %v", err) + } + defer func() { _ = holder.Rollback(ctx) }() + if err := identity.LockUser(ctx, holder, li.u.ID); err != nil { + t.Fatalf("hold lock: %v", err) + } + done := make(chan error, 1) + go func() { + _, err := svc.Enable(ctx, li.u.ID) + done <- err + }() + if !waitForUserLockWaiter(t, pool) { + t.Fatal("enable did not wait on the per-user lock") + } + if !isDisabled(t, pool, li.u.ID) { + t.Error("enable changed the account while another transaction held the lock") + } + if err := holder.Commit(ctx); err != nil { + t.Fatalf("release: %v", err) + } + select { + case err := <-done: + if err != nil { + t.Fatalf("enable after release: %v", err) + } + case <-time.After(10 * time.Second): + t.Fatal("enable did not complete after the lock was released") + } + if isDisabled(t, pool, li.u.ID) { + t.Error("enable did not clear disabled_at after the lock was released") + } + }) + }) +} + +// @ac AC-71 +// AC-71: enable on an account that is not disabled is a no-op. +func TestEnable_NoOpWhenNotDisabled(t *testing.T) { + t.Run("system-auth-identity/AC-71", func(t *testing.T) { + url, pool := freshAPIServer(t) + ctx := context.Background() + svc := users.NewService(pool, nil) + + for _, calls := range []int{1, 2} { + name := map[int]string{1: "already enabled", 2: "repeated"}[calls] + t.Run(name, func(t *testing.T) { + li := loginFresh(t, url, pool, "enablenoop"+map[int]string{1: "once", 2: "twice"}[calls]) + var before time.Time + if err := pool.QueryRow(ctx, `SELECT updated_at FROM users WHERE id = $1`, li.u.ID).Scan(&before); err != nil { + t.Fatalf("read updated_at: %v", err) + } + s0, r0 := liveCounts(t, pool, li.u.ID) + for i := 0; i < calls; i++ { + transitioned, err := svc.Enable(ctx, li.u.ID) + if err != nil { + t.Fatalf("enable call %d: %v", i+1, err) + } + if transitioned { + t.Errorf("enable call %d on an enabled account reported a transition", i+1) + } + } + var after time.Time + if err := pool.QueryRow(ctx, `SELECT updated_at FROM users WHERE id = $1`, li.u.ID).Scan(&after); err != nil { + t.Fatalf("read updated_at: %v", err) + } + if !after.Equal(before) { + t.Errorf("updated_at changed from %v to %v on a no-op", before, after) + } + if s1, r1 := liveCounts(t, pool, li.u.ID); s1 != s0 || r1 != r0 { + t.Errorf("live credentials %d/%d -> %d/%d on a no-op", s0, r0, s1, r1) + } + if code := authMe(t, url, li.sessionCookie); code != http.StatusOK { + t.Errorf("session cookie after a no-op enable = %d, want 200", code) + } + if code := authMeBearer(t, url, li.accessToken); code != http.StatusOK { + t.Errorf("access token after a no-op enable = %d, want 200", code) + } + if code, _ := refreshBody(t, url, li.bodyRefresh); code != http.StatusOK { + t.Errorf("body refresh after a no-op enable = %d, want 200", code) + } + }) + } + + t.Run("unknown or soft-deleted", func(t *testing.T) { + if _, err := svc.Enable(ctx, uuid.New()); !errors.Is(err, users.ErrUserNotFound) { + t.Errorf("unknown user: err = %v, want ErrUserNotFound", err) + } + u := seedAuthUser(t, svc, "enabledeleted", false) + if err := svc.Disable(ctx, u.ID); err != nil { + t.Fatalf("disable: %v", err) + } + if err := svc.SoftDelete(ctx, u.ID); err != nil { + t.Fatalf("soft delete: %v", err) + } + if _, err := svc.Enable(ctx, u.ID); !errors.Is(err, users.ErrUserNotFound) { + t.Errorf("soft-deleted user: err = %v, want ErrUserNotFound", err) + } + if !isDisabled(t, pool, u.ID) { + t.Error("enable cleared disabled_at on a soft-deleted user") + } + }) + }) +} + +// @ac AC-72 +// AC-72: enable neither revokes a service-account token nor restores a +// revoked one. +func TestEnable_LeavesServiceTokensAlone(t *testing.T) { + t.Run("system-auth-identity/AC-72", func(t *testing.T) { + _, pool := freshAPIServer(t) + ctx := context.Background() + svc := users.NewService(pool, nil) + tokens := apitoken.NewService(pool) + + u := seedAuthUser(t, svc, "enabletokens", false) + _ = svc.AssignRole(ctx, u.ID, "viewer", nil) + _, valid, err := tokens.Create(ctx, apitoken.CreateParams{Name: "valid", RoleID: auth.RoleID("viewer"), CreatedBy: &u.ID}) + if err != nil { + t.Fatalf("create valid token: %v", err) + } + revokedRaw, revoked, err := tokens.Create(ctx, apitoken.CreateParams{Name: "revoked", RoleID: auth.RoleID("viewer"), CreatedBy: &u.ID}) + if err != nil { + t.Fatalf("create revoked token: %v", err) + } + if err := tokens.Revoke(ctx, revoked.ID); err != nil { + t.Fatalf("revoke token: %v", err) + } + if err := svc.Disable(ctx, u.ID); err != nil { + t.Fatalf("disable: %v", err) + } + + snapshot := func() string { + var s string + if err := pool.QueryRow(ctx, ` + SELECT string_agg(id::text || ':' || COALESCE(revoked_at::text, 'live') || ':' || + COALESCE(expires_at::text, 'none'), ',' ORDER BY id) + FROM api_tokens WHERE created_by = $1`, u.ID).Scan(&s); err != nil { + t.Fatalf("snapshot api_tokens: %v", err) + } + return s + } + before := snapshot() + if _, err := svc.Enable(ctx, u.ID); err != nil { + t.Fatalf("enable: %v", err) + } + if after := snapshot(); after != before { + t.Errorf("enable changed api_tokens:\nbefore %s\nafter %s", before, after) + } + var validRevoked bool + if err := pool.QueryRow(ctx, `SELECT revoked_at IS NOT NULL FROM api_tokens WHERE id = $1`, valid.ID).Scan(&validRevoked); err != nil { + t.Fatalf("read valid token: %v", err) + } + if validRevoked { + t.Error("enable revoked a service-account token") + } + if _, err := tokens.AuthenticateToken(ctx, revokedRaw); err == nil { + t.Error("a permanently revoked token authenticates after enable") + } + }) +} diff --git a/internal/server/users_admin_handlers.go b/internal/server/users_admin_handlers.go index fccd6377..edd4ddcc 100644 --- a/internal/server/users_admin_handlers.go +++ b/internal/server/users_admin_handlers.go @@ -91,11 +91,24 @@ func (h *handlers) PostUserEnable(w http.ResponseWriter, r *http.Request, id ope if denied := auth.EnforcePermission(w, r, auth.AdminUserManage); denied { return } - if err := h.users.Enable(r.Context(), uuid.UUID(id)); mapUserAdminErr(w, err) { + transitioned, err := h.users.Enable(r.Context(), uuid.UUID(id)) + if mapUserAdminErr(w, err) { return } + // The transition comes from the locked transaction itself, not from a + // later read. A real transition revoked the user's interactive + // credentials; a call on an account that was not disabled revoked + // nothing. Service-account tokens are outside both. Spec api-users C-08. + scope := "none" + if transitioned { + scope = "interactive" + } caller := auth.FromContext(r.Context()).ID - emitAudit(r, audit.AdminUserEnabled, caller, map[string]any{"target_user_id": id.String()}) + emitAudit(r, audit.AdminUserEnabled, caller, map[string]any{ + "target_user_id": id.String(), + "transition": transitioned, + "revocation_scope": scope, + }) h.writeUser(w, r, uuid.UUID(id)) } diff --git a/internal/users/users.go b/internal/users/users.go index ddf5afce..b8f10ba1 100644 --- a/internal/users/users.go +++ b/internal/users/users.go @@ -513,22 +513,61 @@ func (s *Service) Disable(ctx context.Context, id uuid.UUID) error { return s.mutateAccountState(ctx, id, stmt) } -// Enable clears the disabled flag. The user can authenticate again with a -// fresh login; sessions revoked while disabled stay dead. ErrUserNotFound for +// Enable clears the disabled flag. A real disabled-to-enabled transition +// revokes every interactive credential the user holds, so the user signs in +// again: nothing that existed while the account was disabled, including a +// credential no revocation ever reached, authenticates afterwards. Enable on +// an account that is not disabled is a no-op that revokes nothing and signs +// nobody out. Service-account tokens are never touched. ErrUserNotFound for // unknown or soft-deleted users. // -// Spec api-users (disable/enable). -func (s *Service) Enable(ctx context.Context, id uuid.UUID) error { - const stmt = `UPDATE users SET disabled_at = NULL, updated_at = now() - WHERE id = $1 AND deleted_at IS NULL` - tag, err := s.pool.Exec(ctx, stmt, id) +// transitioned reports what the locked transaction did: true only when it +// changed disabled_at from set to null and committed the revocation. The +// caller records it on the audit event; it must not infer it from a later +// read, which another enable or disable could already have changed. +// +// Spec api-users C-07, C-08; system-auth-identity C-34, C-36. +func (s *Service) Enable(ctx context.Context, id uuid.UUID) (transitioned bool, err error) { + tx, err := s.pool.Begin(ctx) if err != nil { - return fmt.Errorf("users: enable: %w", err) + return false, fmt.Errorf("users: begin: %w", err) } - if tag.RowsAffected() == 0 { - return ErrUserNotFound + defer func() { _ = tx.Rollback(ctx) }() + + if err := identity.LockUser(ctx, tx, id); err != nil { + if errors.Is(err, pgx.ErrNoRows) { + return false, ErrUserNotFound + } + return false, fmt.Errorf("users: lock: %w", err) } - return nil + // Read under the lock. The state decides whether this call is a + // transition, and "already enabled" must stay distinguishable from + // "no such user", which a conditional UPDATE alone cannot do. + var disabled, deleted bool + if err := tx.QueryRow(ctx, + `SELECT disabled_at IS NOT NULL, deleted_at IS NOT NULL FROM users WHERE id = $1`, + id).Scan(&disabled, &deleted); err != nil { + return false, fmt.Errorf("users: enable: read state: %w", err) + } + if deleted { + return false, ErrUserNotFound + } + if !disabled { + return false, nil + } + if _, err := tx.Exec(ctx, + `UPDATE users SET disabled_at = NULL, updated_at = now() WHERE id = $1`, id); err != nil { + return false, fmt.Errorf("users: enable: %w", err) + } + // The transition and the revocation commit together, so the account + // is never enabled with a stale credential still live. + if err := identity.RevokeUserCredentials(ctx, tx, id); err != nil { + return false, fmt.Errorf("users: revoke credentials on enable: %w", err) + } + if err := tx.Commit(ctx); err != nil { + return false, fmt.Errorf("users: commit: %w", err) + } + return true, nil } // AssignRole inserts a user_roles row. Role must exist; FK enforcement diff --git a/specs/api/users.spec.yaml b/specs/api/users.spec.yaml index 9027949a..368bce88 100644 --- a/specs/api/users.spec.yaml +++ b/specs/api/users.spec.yaml @@ -1,7 +1,7 @@ spec: id: api-users title: User CRUD + custom-role administration - version: "1.3.0" + version: "1.4.0" status: approved tier: 2 @@ -73,7 +73,11 @@ spec: the per-user lock. Revoking sessions alone is not sufficient, because the refresh family survives and can mint a working session for an account the administrator believes is locked out. SoftDelete MUST - perform the same revocation. Service-account tokens are NOT + perform the same revocation. Enable MUST perform it too, when and + only when it changes disabled_at from set to null, in the same + transaction under the same lock, so the user signs in again; enable + on an account that is not disabled revokes nothing. Service-account + tokens are NOT interactive credentials and MUST NOT be revoked by these operations, so no text describing them may claim they end all of an account's credentials. A disabled user MUST be unable to authenticate on any @@ -82,7 +86,7 @@ spec: type: security enforcement: error - id: C-08 - description: 'Each admin user-management mutation MUST emit its audit code (admin.user.password_reset | admin.user.disabled | admin.user.enabled) carrying detail.target_user_id; an unknown or soft-deleted target MUST return 404 users.not_found.' + description: 'Each admin user-management mutation MUST emit its audit code (admin.user.password_reset | admin.user.disabled | admin.user.enabled) carrying detail.target_user_id; an unknown or soft-deleted target MUST return 404 users.not_found. admin.user.enabled MUST also carry detail.transition (true only when the call changed the account from disabled to enabled) and detail.revocation_scope (interactive for a transition, none otherwise), both taken from the locked transaction that made the change, never inferred from a later read.' type: technical enforcement: error @@ -155,9 +159,26 @@ spec: priority: critical references_constraints: [C-07, C-08] - id: AC-17 - description: 'POST /users/{id}:enable clears disabled_at (200, disabled_at null) and the user can authenticate again; emits admin.user.enabled.' + description: 'POST /users/{id}:enable clears disabled_at (200, disabled_at null) and the user can authenticate again through a FRESH sign-in; emits admin.user.enabled. Every interactive credential that existed while the account was disabled, including one no revocation reached, no longer authenticates afterwards.' priority: high references_constraints: [C-07] + inputs: + initial_state: > + A user disabled by POST /users/{id}:disable, then holding a session + cookie, an access token and a refresh token written directly while + disabled, which no revocation reached + operation: "POST /users/{id}:enable as an administrator" + expected_output: + response: {status: 200, disabled_at: null} + cookies_set: none + audit: "admin.user.enabled (detail asserted by AC-20)" + transaction_effects: ["disabled_at cleared", "every session and refresh token for the user revoked"] + subsequent: + - "The pre-enable session cookie answers 401" + - "The pre-enable access token answers 401" + - "The pre-enable refresh token does not rotate" + - "A fresh sign-in succeeds" + - id: AC-18 description: 'An admin calling POST /users/{id}:disable with their OWN id is denied 409 users.cannot_disable_self and the account is unchanged (still able to authenticate).' priority: critical @@ -166,3 +187,27 @@ spec: description: 'reset-password, :disable and :enable each return 404 users.not_found for an unknown/soft-deleted user, and each requires admin:user_manage (a caller without it is 403 before any state change).' priority: high references_constraints: [C-06, C-07, C-08] + - id: AC-20 + description: 'The admin.user.enabled event records what the request did. A real transition records transition true and revocation_scope interactive; a call on an already-enabled account records transition false and revocation_scope none. Each assertion reads the audit row the request itself produced.' + priority: high + references_constraints: [C-07, C-08] + inputs: + variants: + - name: "Transition" + setup: "A disabled user holding a live session; POST /users/{id}:enable" + - name: "Already enabled" + setup: "An enabled user holding a live session; POST /users/{id}:enable" + method: "Each request carries its own X-Correlation-Id, and only the admin.user.enabled row with that id is read" + expected_output: + per_variant: + Transition: + response: {status: 200, disabled_at: null} + audit_detail: {transition: true, revocation_scope: interactive} + transaction_effects: ["disabled_at cleared", "the user's sessions and refresh tokens revoked"] + cookies_set: none + Already enabled: + response: {status: 200, disabled_at: null} + audit_detail: {transition: false, revocation_scope: none} + transaction_effects: [] + subsequent: ["The user's existing session still authenticates"] + cookies_set: none diff --git a/specs/system/auth-identity.spec.yaml b/specs/system/auth-identity.spec.yaml index ca541460..ae9d4bf9 100644 --- a/specs/system/auth-identity.spec.yaml +++ b/specs/system/auth-identity.spec.yaml @@ -1,7 +1,7 @@ spec: id: system-auth-identity title: Auth identity primitives (password, session, JWT, MFA) - version: "1.7.0" + version: "1.8.0" status: approved tier: 1 @@ -201,6 +201,9 @@ spec: a transaction and MUST NOT own, commit, or roll back one, so a caller cannot be left with a partially committed issuance. Nothing may be reported to the client as issued before the transaction commits. + The account-state mutations are on the same protocol, Enable + included: it reads the account state under the lock to decide + whether the call is a transition at all. type: technical enforcement: error - id: C-35 @@ -220,6 +223,12 @@ spec: refresh family for one user and is what Disable, AdminResetPassword and SoftDelete MUST perform; revoking only sessions leaves the refresh family live and a disabled account can mint a working session from it. + Enable MUST perform the same revocation when, and only when, it + changes disabled_at from set to null, in the same transaction as that + change: re-enabling requires a fresh sign-in, so no credential that + existed while the account was disabled authenticates afterwards, + including one no earlier revocation reached. Enable on an account + that is not disabled changes nothing and revokes nothing. Service-account tokens are NOT interactive credentials and MUST NOT be touched by interactive revocation. No text describing these operations may claim they end all of an account's credentials. @@ -1556,3 +1565,111 @@ spec: - "A response or log asserting nothing was revoked" - "retryable true" - "A second revocation attempt" + - id: AC-70 + description: > + Re-enabling a disabled account revokes every interactive credential the + user holds, atomically with the transition, and a fresh login + afterwards succeeds. This includes credentials that exist at enable + time without ever having been revoked, which is the state an older + release or a leaked issuance path can leave behind. The transition is + serialized on the per-user lock. + priority: critical + references_constraints: [C-34, C-36] + inputs: + initial_state: > + A disabled user holding, at enable time, a live session row and its + cookie, a live refresh token, and an access token bound to that + session. The credentials are written directly for the disabled user, + bypassing the handlers, so the criterion measures what Enable does + with stray state rather than whether issuance is prevented. + operation: "Enable(user)" + variants: + - name: "Transition" + setup: "The enable commits" + - name: "Controlled failure" + setup: "A trigger fails the refresh-token revocation, so the enable transaction fails before commit" + - name: "Serialized" + setup: "A second connection holds the per-user lock when Enable starts" + method: > + Each variant calls the real Enable, then presents each credential to + the real cookie binder, the real Bearer binder, and both refresh + endpoints. A fresh login runs afterwards through the real login + handler. No elapsed-time dependence. + expected_output: + per_variant: + Transition: + returns: {transitioned: true, error: nil} + durable_changes: + - "disabled_at cleared" + - "Every interactive session and refresh token for the user revoked" + interactive_credentials_authenticating_afterward: 0 + subsequent: ["A fresh login succeeds and its session authenticates"] + Controlled failure: + returns: {transitioned: false} + enable_returns_error: true + durable_changes: [] + prohibited_changes: + - "disabled_at cleared without the revocation" + - "Credentials revoked without the account being enabled" + Serialized: + enable_waits_for_the_lock: true + completes_after_release: true + response_cookies_audit: > + Enable is a service call and sets no cookie. The HTTP response and + the admin.user.enabled audit detail are api-users AC-17 and AC-20. + prohibited_changes: + - "Any pre-enable session cookie, refresh token or session-bound access token authenticating" + + - id: AC-71 + description: > + Enable on an account that is not disabled is a no-op. It changes no + row, revokes nothing, and signs nobody out. An unknown or soft-deleted + user is reported as not found. + priority: critical + references_constraints: [C-34, C-36] + inputs: + initial_state: "An enabled user with a live session, a live refresh token and a working access token" + operation: "Enable(user)" + variants: + - "Already enabled" + - "Repeated: Enable twice in succession" + - "Unknown or soft-deleted user" + expected_output: + per_variant: + Already enabled: + returns: {transitioned: false, error: nil} + durable_changes: [] + credentials_revoked: 0 + subsequent: ["The pre-existing session, access token and refresh token still work"] + Repeated: + identical_to: "Already enabled" + Unknown or soft-deleted user: + returns: {transitioned: false, error: ErrUserNotFound} + response_cookies_audit: "A service call; the HTTP response and audit detail are api-users AC-19 and AC-20" + prohibited_changes: + - "Any credential revoked by an enable that changed no account state" + - "updated_at bumped by a no-op" + + - id: AC-72 + description: > + Service-account tokens are outside the enable policy. Enable never + revokes one and never restores one that was revoked. + priority: high + references_constraints: [C-36] + inputs: + initial_state: > + A disabled user owning two owk_ tokens created before the disable: + one otherwise valid, one permanently revoked + operation: "Enable(user)" + scope: > + Asserts the separation only. Whether an owk_ token authenticates + while its owner is disabled is a separate, service-token policy + and is not asserted here. + expected_output: + durable_changes_to_api_tokens: [] + response_cookies_audit: "A service call; nothing is set or emitted about service-account tokens" + per_token: + Otherwise valid: + revoked_by_enable: false + Permanently revoked: + authenticates_after_enable: false