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) }