From d95f90850caced80e22e4f1db87a41c4ca076fa4 Mon Sep 17 00:00:00 2001 From: xarmian Date: Thu, 15 Oct 2026 08:23:56 -0400 Subject: [PATCH] fix(store): a role change or disable of a vanished user is 404, and the unguarded DisableUser is gone (BUG-3826) (#2196) * fix(store): a role change or disable of a vanished user is 404, and the unguarded DisableUser is gone (BUG-3826) Follow-ups to BUG-3811's last-active-admin guard, one unit per the lead: 1. Store.DisableUser wrote disabled_at with no last-admin guard and no lock, the one admin-removing writer outside the invariant. Removed. Its test callers need the pre-BUG-3349 state (disabled, credentials not revoked), so they move to DisableUserLeavingCredentialsForTesting. 2. A user deleted between the handler's read and the write: a promotion answered ErrLastAdmin (a false 409 last_active_admin), a demotion a plain error (500), and a disable succeeded on nothing (200). All three now return store.ErrUserNotFound, which both handlers answer 404. The disable takes the users row FOR NO KEY UPDATE after the guard, so a concurrent deletion cannot slip between the check and the UPDATE. 3. The race test only held demote or disable open as the first door. Account deletion is now held at its user-row lock too, so moving its guard after that lock goes red. 4. docs/api.md documents 409 last_active_admin on delete-account, the admin role PATCH and disable, and the new 404. Claude-Session: https://claude.ai/code/session_01Rj4CMyK2EqxerMihbGs3Kr * docs(api): the vanished-user 404 is promised for a role change only (BUG-3826) Codex r1 (Low): the plan fields of the same PATCH do not map ErrUserNotFound. Claude-Session: https://claude.ai/code/session_01Rj4CMyK2EqxerMihbGs3Kr * docs,test: the delete-account 409 is checked before the Stripe cancel and re-checked after; the race test comment names its contrast (BUG-3826) Blind hunter r2 (Lows): the docs said the refusal comes before any Stripe cancel, which is true of the pre-check only; the handler answers 500 partial_delete when the locked re-check refuses after a cancel ran. The race test's comment read as if it described the other test. Claude-Session: https://claude.ai/code/session_01Rj4CMyK2EqxerMihbGs3Kr --- docs/api.md | 3 + internal/server/bug3349_disable_test.go | 18 +-- internal/server/bug3826_vanished_user_test.go | 49 ++++++++ internal/server/cloud_admin_gate_test.go | 2 +- internal/server/handlers_admin_users.go | 21 ++++ .../server/handlers_watch_vis_cache_test.go | 4 +- .../server/task3352_invitation_proof_test.go | 2 +- internal/store/bug3349_active_user_test.go | 6 +- .../bug3826_last_admin_followups_test.go | 117 ++++++++++++++++++ internal/store/testing_helpers.go | 17 +++ internal/store/users.go | 35 ++++-- internal/store/users_test.go | 2 +- 12 files changed, 247 insertions(+), 29 deletions(-) create mode 100644 internal/server/bug3826_vanished_user_test.go create mode 100644 internal/store/bug3826_last_admin_followups_test.go diff --git a/docs/api.md b/docs/api.md index 68ca51c78..d8bda843d 100644 --- a/docs/api.md +++ b/docs/api.md @@ -112,6 +112,7 @@ One browser tab's content writes to an item are ordered by the server, also **pe - `POST /api/v1/auth/logout` — destroy session - `GET/PATCH /api/v1/auth/me` — current user profile (GET) and update name/password (PATCH) - `GET /api/v1/auth/export` — the caller's account export (GDPR portability), streamed as `pad-account-export.json` (not `pad-export.json`, which is the workspace bundle's file): `{format: "pad-account-export", version: 1, user, workspaces: [...], complete: true}`, where owned workspaces carry their collections and items. `complete: true` is written last and only after every workspace (BUG-3626). The status line goes out before the body, so a timeout (60s) or a store error mid-export cannot become an error status. The server cuts the connection instead, and the body never reaches the trailer. A reader must refuse a file without `complete: true`; the web client does. 403 `forbidden` when the caller is collection-restricted in any owned workspace (BUG-1945) +- `POST /api/v1/auth/delete-account` — delete the caller's account: `{password}`, or `{confirm: true}` for a cloud account with no password, plus `totp_code` when 2FA is on. The instance's last ACTIVE human admin is refused `409 last_active_admin` (BUG-3811), by a check that runs before any Stripe cancel; the deletion re-checks under its lock, and an admin set that changed in between refuses it there: `409 last_active_admin` again, or `500 partial_delete` when a Stripe cancel already ran. A disabled admin is not the admin who remains - `POST /api/v1/auth/forgot-password` — request password reset email - `POST /api/v1/auth/reset-password` — reset password with token - `POST /api/v1/auth/cli/sessions`, `GET /api/v1/auth/cli/sessions/{code}`, `POST /api/v1/auth/cli/sessions/{code}/approve`, `POST /api/v1/auth/cli/sessions/{code}/deny` — browser sign-in for `pad auth login`: the CLI creates a session and polls it, and a signed-in user approves it at `/auth/cli/{code}`, which mints a 30-day session the poll hands to the CLI. Since TASK-2253 the session records `requester_ip` (the server's view of the creating client) and `requester_user_agent` (what the creator sent, capped at 200 characters, so requester-controlled text), and a PENDING poll returns them with `created_at` (additive, omitempty) for the approval page, which renders the agent as text labelled "reported by the requester". `deny` checks no account, because the code is the authority (it is already a bearer secret: whoever holds it can poll and collect an approval's token); it moves pending → denied, answers 200 again on a denied session, and 409 `already_approved` / 410 `expired` otherwise. A denied session POLLS as **410 `cli_auth_denied`**, not as a 200 status: a CLI from before Deny has no case for an unknown status and would poll it for 20 minutes, whereas a non-2xx counts as a poll error there and it stops after five in a row, printing the message; a current CLI stops on the first. Approving a denied session answers 409 `cli_auth_denied`, and an approval that loses to a concurrent deny ends the session it minted @@ -120,6 +121,8 @@ One browser tab's content writes to an item are ordered by the server, also **pe - `GET/PATCH /api/v1/admin/settings` — platform settings (admin-only) - `GET/PUT /api/v1/admin/mcp` — MCP over HTTP (PLAN-2310, admin-only). GET returns `{enabled, source: setting|environment|cloud, locked, readiness}`, where readiness is the resolved origin, MCP URL and issuer, `https`, `auth_methods` (`["oauth","pat"]` on https, `["pat"]` on http, `[]` with no usable origin), `state` (`on` / `off` / `blocked` with the reason) and `resume` counts (OAuth connections and PATs that work again when it is turned on: turning it off revokes nothing). PUT `{enabled}` stores `platform_settings.mcp_enabled` (read per request, no restart); `409 set_by_environment` when `PAD_MCP_ENABLED` forces it, `403 managed_by_operator` on Pad Cloud. MCP is available when cloud, or the setting is on with a usable origin (`PAD_URL` / toml `url` / `--url`, else `PUBLIC_URL`; no `http://host:port` fallback); the MCP URL is `PAD_MCP_PUBLIC_URL` else `/mcp`, the issuer `PAD_AUTH_SERVER_URL` else the origin, all startup-time. Operator docs: `docs/deployment.md` "MCP for agents (self-hosted)" - `GET/PUT /api/v1/admin/decision-provider` — decision provider setting (admin-only, TASK-3121): enable toggle, model pin, and a WRITE-ONLY API key stored encrypted (the response carries `api_key_set`, never the key or a mask of it). Resolved config file < this setting < environment, per field; a PUT to a field the environment sets is refused `409 set_by_environment`. A change rebuilds the provider in place, with no restart. On Pad Cloud the environment is the only source and PUT is `403 managed_by_operator`. `pad server info` resolves only the config file and environment on the CLI host, and says so +- `PATCH /api/v1/admin/users/{userID}` — change a user's `role` (`admin`/`member`), `plan`, `plan_expires_at` or `plan_overrides` (admin-only). Demoting the instance's last ACTIVE human admin answers `409 last_active_admin` (BUG-3811; it used to answer `400 bad_request` "Cannot demote the last admin"); a disabled admin does not count. Changing your own role is `400 bad_request`. A role change for a user deleted while the request runs answers `404 not_found` (BUG-3826); the plan fields do not make that promise +- `POST /api/v1/admin/users/{userID}/disable` — disable an account and revoke its sessions, API tokens and connected apps in one transaction (admin-only, BUG-3349). Disabling the last ACTIVE human admin answers `409 last_active_admin` (BUG-3811); disabling yourself is `400 bad_request`; a user deleted while the request runs answers `404 not_found` (BUG-3826) - `POST /api/v1/admin/test-email` — send test email (admin-only) - `GET /api/v1/admin/mcp-audit?limit=&offset=` — every user's MCP calls, newest first (admin-only; limit default 50, capped at 200), plus `dropped` (audit rows lost to a full writer queue). Each item carries the raw `user_id` and `connection_id` (the OAuth grant's request id, or the PAT id), and since TASK-2255 (C110) the display names `user_name` (the name, else the email) and `connection_name` (the OAuth client's name, else the label the user gave the connection; for a PAT, its name). Both are omitted when the id no longer resolves (a deleted user, a revoked grant), and the page then shows the id. Resolved in one IN-list query per kind per page, not per row - `POST /api/v1/invitations/{code}/accept` — accept workspace invitation diff --git a/internal/server/bug3349_disable_test.go b/internal/server/bug3349_disable_test.go index 861771f39..f915bef28 100644 --- a/internal/server/bug3349_disable_test.go +++ b/internal/server/bug3349_disable_test.go @@ -118,7 +118,7 @@ func TestBUG3349_DisabledAccountIsRefusedBeforeTheAuthExemption(t *testing.T) { t.Fatal(err) } // Disabled, but the credential not revoked: the survivor case. - if err := srv.store.DisableUser(user.ID); err != nil { + if err := srv.store.DisableUserLeavingCredentialsForTesting(user.ID); err != nil { t.Fatal(err) } @@ -151,7 +151,7 @@ func TestBUG3349_DisabledAccountsOAuthGrantIsRefusedAtMCP(t *testing.T) { if err != nil || user == nil { t.Fatal(err) } - if err := srv.store.DisableUser(user.ID); err != nil { + if err := srv.store.DisableUserLeavingCredentialsForTesting(user.ID); err != nil { t.Fatal(err) } if rr := postMCP(srv, "/mcp", tok); rr.Code != http.StatusUnauthorized { @@ -169,7 +169,7 @@ func TestBUG3349_TokenEndpointRefusesADisabledSubject(t *testing.T) { } refresh, client := lastRefresh, lastClientID user, _ := srv.store.GetUserByEmail("oauth-test@example.com") - if err := srv.store.DisableUser(user.ID); err != nil { + if err := srv.store.DisableUserLeavingCredentialsForTesting(user.ID); err != nil { t.Fatal(err) } rr := postOAuthForm(srv, "/oauth/token", url.Values{ @@ -189,7 +189,7 @@ func TestBUG3349_ConsentRefusesADisabledSession(t *testing.T) { srv := twoResourceOAuthServer(t) sess := newOAuthSession(t, srv) user, _ := srv.store.GetUserByEmail("oauth-test@example.com") - if err := srv.store.DisableUser(user.ID); err != nil { + if err := srv.store.DisableUserLeavingCredentialsForTesting(user.ID); err != nil { t.Fatal(err) } if _, code := mintWithResource(t, srv, sess, testCanonicalAudience); code == http.StatusOK { @@ -215,7 +215,7 @@ func TestBUG3349_TOTPChallengeMintsNothingForADisabledAccount(t *testing.T) { if b.Error.Details.ChallengeToken == "" { t.Fatalf("no challenge minted (%d): %s", rr.Code, rr.Body.String()) } - if err := srv.store.DisableUser(userID); err != nil { + if err := srv.store.DisableUserLeavingCredentialsForTesting(userID); err != nil { t.Fatal(err) } before := countRows(t, srv, `SELECT COUNT(*) FROM sessions WHERE user_id = ?`, userID) @@ -239,7 +239,7 @@ func TestBUG3349_NoSessionForADisabledAccount(t *testing.T) { if err != nil { t.Fatal(err) } - if err := srv.store.DisableUser(u.ID); err != nil { + if err := srv.store.DisableUserLeavingCredentialsForTesting(u.ID); err != nil { t.Fatal(err) } if _, err := srv.store.CreateSession(u.ID, "cli-browser-auth", "192.0.2.1", "", time.Hour); !errors.Is(err, store.ErrUserDisabled) { @@ -265,7 +265,7 @@ func TestBUG3349_ShareLinksAreSuspendedWhileTheCreatorIsDisabled(t *testing.T) { if rr := doRequest(srv, "GET", "/api/v1/s/"+link.Token, nil); rr.Code != http.StatusOK { t.Fatalf("fixture: the link does not serve (%d)", rr.Code) } - if err := srv.store.DisableUser(creator.ID); err != nil { + if err := srv.store.DisableUserLeavingCredentialsForTesting(creator.ID); err != nil { t.Fatal(err) } suspended := doRequest(srv, "GET", "/api/v1/s/"+link.Token, nil) @@ -293,7 +293,7 @@ func TestBUG3349_EmailVerificationRefusesADisabledAccount(t *testing.T) { if err != nil { t.Fatal(err) } - if err := srv.store.DisableUser(u.ID); err != nil { + if err := srv.store.DisableUserLeavingCredentialsForTesting(u.ID); err != nil { t.Fatal(err) } rr := doRequest(srv, "POST", "/api/v1/auth/verify-email", map[string]any{"token": tok}) @@ -319,7 +319,7 @@ func TestBUG3349_AuthorizeTreatsADisabledSessionAsSignedOut(t *testing.T) { if rr := doRequestWithCookie(srv, "GET", "/oauth/authorize?"+q.Encode(), nil, sess.sessionToken); rr.Code != http.StatusOK { t.Fatalf("fixture: the consent page does not render for an enabled session (%d)", rr.Code) } - if err := srv.store.DisableUser(user.ID); err != nil { + if err := srv.store.DisableUserLeavingCredentialsForTesting(user.ID); err != nil { t.Fatal(err) } rr := doRequestWithCookie(srv, "GET", "/oauth/authorize?"+q.Encode(), nil, sess.sessionToken) diff --git a/internal/server/bug3826_vanished_user_test.go b/internal/server/bug3826_vanished_user_test.go new file mode 100644 index 000000000..f330fc117 --- /dev/null +++ b/internal/server/bug3826_vanished_user_test.go @@ -0,0 +1,49 @@ +package server + +import ( + "fmt" + "net/http" + "testing" + + "github.com/PerpetualSoftware/pad/internal/models" +) + +// BUG-3826: a user deleted between the admin handler's read and its write +// answers 404. A promotion used to answer 409 last_active_admin (a false +// message), a demotion 500, and a disable 200 for a disable of nothing. +func TestBUG3826_VanishedUserAnswers404(t *testing.T) { + for _, door := range []struct { + name, method, path string + body any + }{ + {"promote", "PATCH", "/api/v1/admin/users/%s", map[string]any{"role": "admin"}}, + {"demote", "PATCH", "/api/v1/admin/users/%s", map[string]any{"role": "member"}}, + {"disable", "POST", "/api/v1/admin/users/%s/disable", nil}, + } { + t.Run(door.name, func(t *testing.T) { + srv := testServer(t) + token := bootstrapFirstUser(t, srv, "admin-3826@test.com", "Admin") + u, err := srv.store.CreateUser(models.UserCreate{Email: "gone-3826@test.com", Name: "Gone", Password: "pw-test-12345"}) + if err != nil { + t.Fatal(err) + } + deleted := false + adminUserAfterReadHook = func(id string) { + adminUserAfterReadHook = nil + if err := srv.store.DeleteAccountAtomic(id); err != nil { + t.Errorf("delete %s mid-request: %v", id, err) + } + deleted = true + } + t.Cleanup(func() { adminUserAfterReadHook = nil }) + + rr := doRequestWithCookie(srv, door.method, fmt.Sprintf(door.path, u.ID), door.body, token) + if !deleted { + t.Fatalf("the hook never ran: %d %s", rr.Code, rr.Body.String()) + } + if rr.Code != http.StatusNotFound || errorCode(t, rr) != "not_found" { + t.Errorf("%s of a user deleted mid-request: %d %s, want 404 not_found", door.name, rr.Code, rr.Body.String()) + } + }) + } +} diff --git a/internal/server/cloud_admin_gate_test.go b/internal/server/cloud_admin_gate_test.go index dd16ed4b3..d21d461b3 100644 --- a/internal/server/cloud_admin_gate_test.go +++ b/internal/server/cloud_admin_gate_test.go @@ -1008,7 +1008,7 @@ func TestCloudAdminGate_DisabledAdminSession_GarbageSecret_RejectedByRequireAuth } beforePlan := target.Plan - if err := srv.store.DisableUser(admin.ID); err != nil { + if err := srv.store.DisableUserLeavingCredentialsForTesting(admin.ID); err != nil { t.Fatalf("disable admin: %v", err) } diff --git a/internal/server/handlers_admin_users.go b/internal/server/handlers_admin_users.go index b533aa500..fbd504a28 100644 --- a/internal/server/handlers_admin_users.go +++ b/internal/server/handlers_admin_users.go @@ -390,6 +390,11 @@ func (s *Server) handleAdminGetUserWorkspaces(w http.ResponseWriter, r *http.Req }) } +// adminUserAfterReadHook, when set by a test, runs in the role-change and +// disable handlers after they read the user and before they write, so a test +// can delete the user in between (BUG-3826). +var adminUserAfterReadHook func(userID string) + // handleAdminUpdateUser updates a user's plan, overrides, or role. // PATCH /api/v1/admin/users/{userID} func (s *Server) handleAdminUpdateUser(w http.ResponseWriter, r *http.Request) { @@ -407,6 +412,9 @@ func (s *Server) handleAdminUpdateUser(w http.ResponseWriter, r *http.Request) { writeError(w, http.StatusNotFound, "not_found", "User not found") return } + if adminUserAfterReadHook != nil { + adminUserAfterReadHook(userID) + } var input struct { Role *string `json:"role"` @@ -439,6 +447,11 @@ func (s *Server) handleAdminUpdateUser(w http.ResponseWriter, r *http.Request) { writeLastActiveAdmin(w) return } + // Deleted since the GetUser above (BUG-3826). + if errors.Is(err, store.ErrUserNotFound) { + writeError(w, http.StatusNotFound, "not_found", "User not found") + return + } writeInternalError(w, err) return } @@ -663,6 +676,9 @@ func (s *Server) handleAdminDisableUser(w http.ResponseWriter, r *http.Request) writeError(w, http.StatusNotFound, "not_found", "User not found") return } + if adminUserAfterReadHook != nil { + adminUserAfterReadHook(userID) + } // Every credential, not only sessions, in one transaction (BUG-3349): a // PAT or an MCP grant kept working after a disable. Idempotent, so a // retry after a failure redoes all of it. @@ -671,6 +687,11 @@ func (s *Server) handleAdminDisableUser(w http.ResponseWriter, r *http.Request) writeLastActiveAdmin(w) return } + // Deleted since the GetUser above (BUG-3826). + if errors.Is(err, store.ErrUserNotFound) { + writeError(w, http.StatusNotFound, "not_found", "User not found") + return + } writeInternalError(w, err) return } diff --git a/internal/server/handlers_watch_vis_cache_test.go b/internal/server/handlers_watch_vis_cache_test.go index a76f721c9..464d492b0 100644 --- a/internal/server/handlers_watch_vis_cache_test.go +++ b/internal/server/handlers_watch_vis_cache_test.go @@ -100,7 +100,7 @@ func TestWatchVisCache_DeniesAfterUserDisabled(t *testing.T) { t.Fatalf("expected the active owner-member to have full access before disable, got %+v", before) } - if err := srv.store.DisableUser(user.ID); err != nil { + if err := srv.store.DisableUserLeavingCredentialsForTesting(user.ID); err != nil { t.Fatalf("disable user: %v", err) } @@ -175,7 +175,7 @@ func TestWatchEventsStream_StopsDeliveringAfterUserDisabled(t *testing.T) { waitForWatchEvent(t, ch, 3*time.Second) // the pre-disable notification // Disable the connected user and wait past a reval tick. - if err := srv.store.DisableUser(user.ID); err != nil { + if err := srv.store.DisableUserLeavingCredentialsForTesting(user.ID); err != nil { t.Fatalf("DisableUser: %v", err) } time.Sleep(150 * time.Millisecond) // several reval ticks diff --git a/internal/server/task3352_invitation_proof_test.go b/internal/server/task3352_invitation_proof_test.go index 74d62c5dc..5f70c3fe9 100644 --- a/internal/server/task3352_invitation_proof_test.go +++ b/internal/server/task3352_invitation_proof_test.go @@ -251,7 +251,7 @@ func TestTASK3352_ConsumeInvitationProofRules(t *testing.T) { t.Errorf("an invitation with an empty expiry verified: %v %v", ok, err) } - if err := srv.store.DisableUser(b.ID); err != nil { + if err := srv.store.DisableUserLeavingCredentialsForTesting(b.ID); err != nil { t.Fatal(err) } if ok, err := srv.store.ConsumeInvitationProof(invB.ID, b.ID, invB.Proof); err != nil || ok { diff --git a/internal/store/bug3349_active_user_test.go b/internal/store/bug3349_active_user_test.go index 5bf00f072..5b7ac7c9f 100644 --- a/internal/store/bug3349_active_user_test.go +++ b/internal/store/bug3349_active_user_test.go @@ -29,7 +29,7 @@ func activeUserFixture(t *testing.T) (*Store, *models.User, *models.OAuthClient) func TestBUG3349_NoCredentialIsMintedForADisabledAccount(t *testing.T) { s, u, client := activeUserFixture(t) - if err := s.DisableUser(u.ID); err != nil { + if err := s.DisableUserLeavingCredentialsForTesting(u.ID); err != nil { t.Fatal(err) } req := func(sig string) models.OAuthRequest { @@ -238,7 +238,7 @@ func TestBUG3349_DisabledAccountMintsNoInvitationOrShareLink(t *testing.T) { if err != nil { t.Fatal(err) } - if err := s.DisableUser(u.ID); err != nil { + if err := s.DisableUserLeavingCredentialsForTesting(u.ID); err != nil { t.Fatal(err) } if _, err := s.CreateInvitation(ws.ID, "invitee@example.com", "owner", u.ID); !errors.Is(err, ErrUserDisabled) { @@ -258,7 +258,7 @@ func TestBUG3349_EmailVerificationConsumesNothingForADisabledAccount(t *testing. if err != nil { t.Fatal(err) } - if err := s.DisableUser(u.ID); err != nil { + if err := s.DisableUserLeavingCredentialsForTesting(u.ID); err != nil { t.Fatal(err) } if _, err := s.ConsumeEmailVerification(tok); !errors.Is(err, ErrUserDisabled) { diff --git a/internal/store/bug3826_last_admin_followups_test.go b/internal/store/bug3826_last_admin_followups_test.go new file mode 100644 index 000000000..62ed69e49 --- /dev/null +++ b/internal/store/bug3826_last_admin_followups_test.go @@ -0,0 +1,117 @@ +package store + +import ( + "database/sql" + "errors" + "testing" + "time" +) + +// BUG-3826: follow-ups to BUG-3811's last-active-admin guard. + +// advisoryHasWaiter reports whether a session waits on the advisory lock +// keyed hashtext(key); bug3811GuardHasWaiter's query for any key. +func advisoryHasWaiter(t *testing.T, db *sql.DB, key string) bool { + t.Helper() + var n int + if err := db.QueryRow(`SELECT COUNT(*) FROM pg_locks WHERE locktype = 'advisory' AND NOT granted + AND database = (SELECT oid FROM pg_database WHERE datname = current_database()) + AND objsubid = 1 + AND classid = (((hashtext($1)::bigint) >> 32) & 4294967295)::oid + AND objid = ((hashtext($1)::bigint) & 4294967295)::oid`, key).Scan(&n); err != nil { + t.Fatal(err) + } + return n > 0 +} + +// Unlike TestBUG3811_ConcurrentRemovalsKeepAnActiveAdmin, which holds demote +// or disable open as the FIRST door, this holds account deletion open, at the point it +// already holds its user-row lock, so a regression that moved the deletion's +// guard after that lock (inverting account-deletion -> active-admins -> row) +// lets the second door run straight through, and goes red. +func TestBUG3826_DeleteSelfHeldFirstKeepsAnActiveAdmin(t *testing.T) { + second := map[string]struct { + run func(s *Store, id string) error + waitsFor string + }{ + "demote": {func(s *Store, id string) error { return s.SetUserRole(id, "member") }, "pad:active-admins"}, + "disable": {func(s *Store, id string) error { return s.DisableUserAndRevokeAccess(id) }, "pad:active-admins"}, + "delete-self": {func(s *Store, id string) error { return s.DeleteAccountAtomic(id) }, "pad:account-deletion"}, + } + for sn, b2 := range second { + t.Run("delete-self/"+sn, func(t *testing.T) { + s := testStore(t) + if s.dialect.Driver() != DriverPostgres { + t.Skip("Postgres only: SQLite's single writer serializes the two removals") + } + a, b := bug3811Admins(t, s) + + done := make(chan error, 1) + deleteAccountAfterUserLockHook = func() { + deleteAccountAfterUserLockHook = nil + go func() { done <- b2.run(s, b) }() + // Let A commit only once B is seen WAITING, so the pair is + // concurrent by observation, not by a timer. + deadline := time.Now().Add(10 * time.Second) + for !advisoryHasWaiter(t, s.db, b2.waitsFor) { + if time.Now().After(deadline) { + t.Errorf("B's %s never waited on %s while A's deletion held its user row", sn, b2.waitsFor) + return + } + select { + case err := <-done: + t.Errorf("B's %s returned before A's deletion committed (err %v): it did not wait", sn, err) + done <- err + return + case <-time.After(20 * time.Millisecond): + } + } + } + t.Cleanup(func() { deleteAccountAfterUserLockHook = nil }) + + if err := s.DeleteAccountAtomic(a); err != nil { + t.Fatalf("A's deletion: %v", err) + } + var bErr error + select { + case bErr = <-done: + case <-time.After(10 * time.Second): + t.Fatalf("B's %s never returned after A's deletion committed", sn) + } + if n := bug3811ActiveAdmins(t, s); n == 0 { + t.Errorf("zero active admins: B's %s returned %v", sn, bErr) + } + if !errors.Is(bErr, ErrLastAdmin) { + t.Errorf("B's %s err = %v, want ErrLastAdmin", sn, bErr) + } + }) + } +} + +// A role change or disable whose user is gone answers ErrUserNotFound: a +// promotion used to answer ErrLastAdmin (a false 409 last_active_admin), a +// demotion a plain error (a 500), and a disable succeeded on nothing. +func TestBUG3826_VanishedUserIsNotFound(t *testing.T) { + doors := map[string]func(s *Store, id string) error{ + "promote": func(s *Store, id string) error { return s.SetUserRole(id, "admin") }, + "demote": func(s *Store, id string) error { return s.SetUserRole(id, "member") }, + "disable": func(s *Store, id string) error { return s.DisableUserAndRevokeAccess(id) }, + } + for name, door := range doors { + t.Run(name, func(t *testing.T) { + s := testStore(t) + bug3811Admins(t, s) + u := createTestUser(t, s, "gone@test.com", "Gone", "password123") + if err := s.DeleteAccountAtomic(u.ID); err != nil { + t.Fatal(err) + } + err := door(s, u.ID) + if !errors.Is(err, ErrUserNotFound) { + t.Errorf("%s of a deleted user err = %v, want ErrUserNotFound", name, err) + } + if errors.Is(err, ErrLastAdmin) { + t.Errorf("%s of a deleted user answered ErrLastAdmin", name) + } + }) + } +} diff --git a/internal/store/testing_helpers.go b/internal/store/testing_helpers.go index 796edfd77..dbc751684 100644 --- a/internal/store/testing_helpers.go +++ b/internal/store/testing_helpers.go @@ -306,3 +306,20 @@ func (s *Store) SetBeforeItemVersionsReadHookForTesting(hook func(itemID string) s.beforeItemVersionsRead = hook return func() { s.beforeItemVersionsRead = prev } } + +// DisableUserLeavingCredentialsForTesting sets disabled_at without revoking +// anything and without the last-admin guard: the state a disable made before +// BUG-3349 left behind, which the credential gates must still refuse. The +// only production door is DisableUserAndRevokeAccess; Store.DisableUser, +// which did this unguarded, was removed (BUG-3826). +// +// Production code MUST NOT call this. The "ForTesting" suffix is the grep +// signal. +func (s *Store) DisableUserLeavingCredentialsForTesting(userID string) error { + _, err := s.db.Exec(s.q(`UPDATE users SET disabled_at = ?, updated_at = ?, credential_epoch = credential_epoch + 1 WHERE id = ?`), + now(), now(), userID) + if err != nil { + return fmt.Errorf("disable user: %w", err) + } + return nil +} diff --git a/internal/store/users.go b/internal/store/users.go index 7d06ba41a..2e86fdb09 100644 --- a/internal/store/users.go +++ b/internal/store/users.go @@ -828,6 +828,11 @@ func (s *Store) RemoveOAuthProvider(userID, provider string) error { // does not count: they cannot sign in to recover anything. var ErrLastAdmin = fmt.Errorf("the instance must keep at least one active admin") +// ErrUserNotFound is a role change or disable whose user is gone, typically +// deleted between the handler's read and the write (BUG-3826). The handlers +// answer it 404, not 409 last_active_admin or 500. +var ErrUserNotFound = errors.New("user not found") + // guardActiveAdminLossTx is called inside every transaction that can take // userID out of the set of active human admins: a demotion, a disable, an // account deletion. It refuses with ErrLastAdmin when userID is an active @@ -1002,16 +1007,6 @@ func throttleTime(ts string) string { return t.Add(-5 * time.Minute).Format(time.RFC3339) } -// DisableUser soft-disables a user account by setting disabled_at. -func (s *Store) DisableUser(userID string) error { - _, err := s.db.Exec(s.q(`UPDATE users SET disabled_at = ?, updated_at = ?, credential_epoch = credential_epoch + 1 WHERE id = ?`), - now(), now(), userID) - if err != nil { - return fmt.Errorf("disable user: %w", err) - } - return nil -} - // requireActiveUserTx is the one gate every credential-minting insert passes // (BUG-3349): it reads the user's row inside the insert's own transaction, // FOR SHARE on Postgres, and refuses with ErrUserDisabled when the account is @@ -1093,6 +1088,20 @@ func (s *Store) disableGuardedTx(tx *sql.Tx, userID string) error { if err := s.guardActiveAdminLossTx(tx, userID); err != nil { return err } + // The row lock the disable's UPDATE takes anyway, taken first so a user + // deleted since the handler read it is ErrUserNotFound, not a disable of + // nothing answered 200 (BUG-3826). After the guard, per its lock order. + q := `SELECT 1 FROM users WHERE id = ?` + if s.dialect.Driver() == DriverPostgres { + q += ` FOR NO KEY UPDATE` + } + var one int + if err := tx.QueryRow(s.q(q), userID).Scan(&one); err != nil { + if errors.Is(err, sql.ErrNoRows) { + return ErrUserNotFound + } + return fmt.Errorf("disable user: read user: %w", err) + } return s.disableUserAndRevokeAccessTx(tx, userID) } @@ -1180,9 +1189,11 @@ func (s *Store) SetUserRole(userID, role string) error { return fmt.Errorf("set user role: %w", err) } + // A promotion only grows the admin set, so zero rows means the user is + // gone, never the last-admin refusal it used to answer (BUG-3826). n, _ := result.RowsAffected() if n == 0 { - return ErrLastAdmin + return ErrUserNotFound } return nil } @@ -1212,7 +1223,7 @@ func (s *Store) demoteGuardedTx(tx *sql.Tx, userID string) error { return fmt.Errorf("set user role: %w", err) } if n, _ := result.RowsAffected(); n == 0 { - return fmt.Errorf("set user role: user not found") + return ErrUserNotFound } return nil } diff --git a/internal/store/users_test.go b/internal/store/users_test.go index f3ec9e7a3..27f81b7fe 100644 --- a/internal/store/users_test.go +++ b/internal/store/users_test.go @@ -306,7 +306,7 @@ func TestSearchUsersAggregations(t *testing.T) { alice := createTestUser(t, s, "alice@example.com", "Alice", "password123") bob := createTestUser(t, s, "bob@example.com", "Bob", "password123") carol := createTestUser(t, s, "carol@example.com", "Carol", "password123") - if err := s.DisableUser(carol.ID); err != nil { + if err := s.DisableUserLeavingCredentialsForTesting(carol.ID); err != nil { t.Fatalf("disable carol: %v", err) }