From a136e2d2564903164e962202a148eff7680e9bd2 Mon Sep 17 00:00:00 2001 From: Remylus Losius Date: Thu, 24 Sep 2026 16:51:08 -0400 Subject: [PATCH 1/2] fix(users): revoke interactive credentials when an account is re-enabled Enable cleared disabled_at and revoked nothing, so a credential that existed while the account was disabled authenticated again the moment an administrator re-enabled it. The integrated closure run on a22c8b29 measured it: a session cookie, an access token and a body refresh token written during the disabled window all answered 200 after Enable. This implements I10, approved 2026-09-23. Enable now takes the per-user lock, reads the account state under it, and on a real disabled-to-enabled transition clears disabled_at and performs the user-wide interactive revocation in the same transaction. Enable on an account that is not disabled changes no row and revokes nothing. Service-account tokens are untouched: enable neither revokes one nor restores a revoked one. The AC-70 fixture writes its stray credentials directly, bypassing every handler. It proves what Enable does with state an older release or a leaked issuance path could leave. It does not show that a current handler issues credentials while an account is disabled. Specs: system-auth-identity 1.8.0 (C-34, C-36 amended; AC-70 to AC-72), api-users 1.4.0 (C-07 amended; AC-17 amended so it can fail without this change). Refs: bugs/OW-069, bugs/doing/OW-073, bugs/doing/OW-062 section 14.7 --- internal/server/api_users_admin_test.go | 16 ++ internal/server/enable_revocation_test.go | 326 ++++++++++++++++++++++ internal/users/users.go | 50 +++- specs/api/users.spec.yaml | 10 +- specs/system/auth-identity.spec.yaml | 112 +++++++- 5 files changed, 502 insertions(+), 12 deletions(-) create mode 100644 internal/server/enable_revocation_test.go diff --git a/internal/server/api_users_admin_test.go b/internal/server/api_users_admin_test.go index 68ca6561..06e8ff32 100644 --- a/internal/server/api_users_admin_test.go +++ b/internal/server/api_users_admin_test.go @@ -174,12 +174,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 { diff --git a/internal/server/enable_revocation_test.go b/internal/server/enable_revocation_test.go new file mode 100644 index 00000000..49362280 --- /dev/null +++ b/internal/server/enable_revocation_test.go @@ -0,0 +1,326 @@ +// @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) + } + + if err := svc.Enable(ctx, li.u.ID); err != nil { + t.Fatalf("enable: %v", err) + } + 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") + err := svc.Enable(ctx, li.u.ID) + restore() + 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() { done <- svc.Enable(ctx, li.u.ID) }() + 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++ { + if err := svc.Enable(ctx, li.u.ID); err != nil { + t.Fatalf("enable call %d: %v", i+1, err) + } + } + 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/users/users.go b/internal/users/users.go index ddf5afce..45cc9d97 100644 --- a/internal/users/users.go +++ b/internal/users/users.go @@ -513,21 +513,55 @@ 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). +// Spec api-users C-07; system-auth-identity C-34, C-36. 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) + tx, err := s.pool.Begin(ctx) if err != nil { - return fmt.Errorf("users: enable: %w", err) + return fmt.Errorf("users: begin: %w", err) } - if tag.RowsAffected() == 0 { + defer func() { _ = tx.Rollback(ctx) }() + + if err := identity.LockUser(ctx, tx, id); err != nil { + if errors.Is(err, pgx.ErrNoRows) { + return ErrUserNotFound + } + return fmt.Errorf("users: lock: %w", err) + } + // 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 fmt.Errorf("users: enable: read state: %w", err) + } + if deleted { return ErrUserNotFound } + if !disabled { + return nil + } + if _, err := tx.Exec(ctx, + `UPDATE users SET disabled_at = NULL, updated_at = now() WHERE id = $1`, id); err != nil { + return 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 fmt.Errorf("users: revoke credentials on enable: %w", err) + } + if err := tx.Commit(ctx); err != nil { + return fmt.Errorf("users: commit: %w", err) + } return nil } diff --git a/specs/api/users.spec.yaml b/specs/api/users.spec.yaml index 9027949a..9881b216 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 @@ -155,7 +159,7 @@ 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] - id: AC-18 diff --git a/specs/system/auth-identity.spec.yaml b/specs/system/auth-identity.spec.yaml index ca541460..358e6535 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,104 @@ 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: + 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: + 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 + 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: 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: ErrUserNotFound + 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: [] + per_token: + Otherwise valid: + revoked_by_enable: false + Permanently revoked: + authenticates_after_enable: false From 1e1bab6d9a3efc447d51c5667d1cfe60b536c58b Mon Sep 17 00:00:00 2001 From: Remylus Losius Date: Thu, 24 Sep 2026 20:11:59 -0400 Subject: [PATCH 2/2] fix(users): record whether an enable changed the account The enable handler emitted admin.user.enabled with target_user_id only, identically for a real disabled-to-enabled transition and for a call on an account that was already enabled. I10 requires the two to be distinguishable in the audit record. Enable now returns the transition from the locked transaction that made it, and the handler records detail.transition and detail.revocation_scope (interactive or none). The value is never inferred from a later read, which a concurrent enable or disable could already have changed. Nothing is claimed about service-account tokens. api-users 1.4.0: C-08 amended, AC-17 given explicit inputs and expected output, AC-20 added. AC-20 reads the audit row the request itself produced, by correlation id, for both outcomes. The admin.user.enabled declaration now describes both outcomes; the old description said "re-enabled a previously disabled user account", which was false for a no-op. Refs: bugs/OW-069, bugs/doing/OW-062 sections 9.1, 9.2 and 11.2 --- audit/events.yaml | 10 ++- internal/audit/events.gen.go | 6 +- internal/server/api_users_admin_test.go | 99 +++++++++++++++++++++++ internal/server/enable_revocation_test.go | 28 +++++-- internal/server/users_admin_handlers.go | 17 +++- internal/users/users.go | 29 ++++--- specs/api/users.spec.yaml | 43 +++++++++- specs/system/auth-identity.spec.yaml | 11 ++- 8 files changed, 215 insertions(+), 28 deletions(-) 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 06e8ff32..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" @@ -246,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 index 49362280..402657bf 100644 --- a/internal/server/enable_revocation_test.go +++ b/internal/server/enable_revocation_test.go @@ -108,9 +108,13 @@ func TestEnable_TransitionRevokesInteractiveCredentials(t *testing.T) { t.Fatalf("precondition: stray live credentials = %d/%d, want 1/2", s, r) } - if err := svc.Enable(ctx, li.u.ID); err != nil { + 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") } @@ -151,8 +155,11 @@ func TestEnable_TransitionRevokesInteractiveCredentials(t *testing.T) { } writeStrayCredentials(t, pool, li.u.ID) restore := failOnWrite(t, pool, "refresh_tokens", "UPDATE") - err := svc.Enable(ctx, li.u.ID) + 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") } @@ -178,7 +185,10 @@ func TestEnable_TransitionRevokesInteractiveCredentials(t *testing.T) { t.Fatalf("hold lock: %v", err) } done := make(chan error, 1) - go func() { done <- svc.Enable(ctx, li.u.ID) }() + 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") } @@ -221,9 +231,13 @@ func TestEnable_NoOpWhenNotDisabled(t *testing.T) { } s0, r0 := liveCounts(t, pool, li.u.ID) for i := 0; i < calls; i++ { - if err := svc.Enable(ctx, li.u.ID); err != nil { + 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 { @@ -248,7 +262,7 @@ func TestEnable_NoOpWhenNotDisabled(t *testing.T) { } t.Run("unknown or soft-deleted", func(t *testing.T) { - if err := svc.Enable(ctx, uuid.New()); !errors.Is(err, users.ErrUserNotFound) { + 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) @@ -258,7 +272,7 @@ func TestEnable_NoOpWhenNotDisabled(t *testing.T) { 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) { + 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) { @@ -306,7 +320,7 @@ func TestEnable_LeavesServiceTokensAlone(t *testing.T) { return s } before := snapshot() - if err := svc.Enable(ctx, u.ID); err != nil { + if _, err := svc.Enable(ctx, u.ID); err != nil { t.Fatalf("enable: %v", err) } if after := snapshot(); after != before { 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 45cc9d97..b8f10ba1 100644 --- a/internal/users/users.go +++ b/internal/users/users.go @@ -521,19 +521,24 @@ func (s *Service) Disable(ctx context.Context, id uuid.UUID) error { // nobody out. Service-account tokens are never touched. ErrUserNotFound for // unknown or soft-deleted users. // -// Spec api-users C-07; system-auth-identity C-34, C-36. -func (s *Service) Enable(ctx context.Context, id uuid.UUID) error { +// 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: begin: %w", err) + return false, fmt.Errorf("users: begin: %w", err) } defer func() { _ = tx.Rollback(ctx) }() if err := identity.LockUser(ctx, tx, id); err != nil { if errors.Is(err, pgx.ErrNoRows) { - return ErrUserNotFound + return false, ErrUserNotFound } - return fmt.Errorf("users: lock: %w", err) + return false, fmt.Errorf("users: lock: %w", err) } // Read under the lock. The state decides whether this call is a // transition, and "already enabled" must stay distinguishable from @@ -542,27 +547,27 @@ func (s *Service) Enable(ctx context.Context, id uuid.UUID) error { 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 fmt.Errorf("users: enable: read state: %w", err) + return false, fmt.Errorf("users: enable: read state: %w", err) } if deleted { - return ErrUserNotFound + return false, ErrUserNotFound } if !disabled { - return nil + return false, nil } if _, err := tx.Exec(ctx, `UPDATE users SET disabled_at = NULL, updated_at = now() WHERE id = $1`, id); err != nil { - return fmt.Errorf("users: enable: %w", err) + 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 fmt.Errorf("users: revoke credentials on enable: %w", err) + return false, fmt.Errorf("users: revoke credentials on enable: %w", err) } if err := tx.Commit(ctx); err != nil { - return fmt.Errorf("users: commit: %w", err) + return false, fmt.Errorf("users: commit: %w", err) } - return nil + 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 9881b216..368bce88 100644 --- a/specs/api/users.spec.yaml +++ b/specs/api/users.spec.yaml @@ -86,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 @@ -162,6 +162,23 @@ spec: 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 @@ -170,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 358e6535..ae9d4bf9 100644 --- a/specs/system/auth-identity.spec.yaml +++ b/specs/system/auth-identity.spec.yaml @@ -1598,12 +1598,14 @@ spec: 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: @@ -1612,6 +1614,9 @@ spec: 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" @@ -1632,14 +1637,15 @@ spec: expected_output: per_variant: Already enabled: - returns: nil + 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: ErrUserNotFound + 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" @@ -1661,6 +1667,7 @@ spec: 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