From 7396ec42bbdfd5902ada9b202a983ff8f4059209 Mon Sep 17 00:00:00 2001 From: xarmian Date: Sun, 4 Oct 2026 19:22:21 -0400 Subject: [PATCH] feat(oauth): delegated sign-in for installed apps through the issuance barrier, shipped closed (TASK-3399, SPEC-6 U5b-1) (#1779) * feat(oauth): delegated install grants through the issuance barrier: code, PKCE, access and refresh for a live human member, the consented access bound and capped by the manifest (TASK-3399, U5b-1) * feat(oauth): delegated app sign-in: a fixed-workspace consent page, code+PKCE for install clients, the person's access choice in the grant, and an ownership check before fosite on code and refresh (TASK-3399, U5b-1) * feat(oauth): app grants are not MCP connections: the readers skip install chains, the console lists and revokes them through the binding, the sweep clears orphan bindings, and revoking a person reaches their delegated codes (TASK-3399, U5b-1) * test(server): the console lists an app grant on its own, refuses every connection mutation on it, and revokes it through the binding (TASK-3399, U5b-1) * test(server): the console revoke ends an app grant through its binding (TASK-3399, U5b-1) * feat(web): Connected apps lists installed apps acting as you, read-only plus revoke (TASK-3399, U5b-1) * test(oauth): pin the binding's person and a resource-less sign-in; name the fosite layers behind the consent gate's PKCE and audience checks (TASK-3399, U5b-1) * fix(oauth): U5b-1 codex r1: a delegated binding remembers the person's credential epoch and membership and is tombstoned on revoke; issuance share-locks the person on Postgres; app sign-in and grant management work with MCP off (TASK-3399, U5b-1) * fix(oauth): U5b-1 codex r2: only a delegated grant's code writes its binding; member removal, disable and claim end a person's delegated grants through their installs; users before installs before tokens in every path (TASK-3399, U5b-1) * fix(oauth): U5b-1 codex r3: delegated revocation takes no install lock (the person's row orders it against issuance) and writes child rows in teardown's order, closing two deadlocks with owner-account deletion (TASK-3399, U5b-1) * fix(oauth): U5b-1 codex r4: install teardown and a person's delegated revocation lock their bindings in request_id order before any child row (TASK-3399, U5b-1) * feat(store): bounded retry on a Postgres deadlock for delegated-grant revocation, member removal, account disable and claim, and the OAuth sweep, with a per-site metric (TASK-3399, U5b-1, lead ruling (a)) * fix(oauth): U5b-1 codex r6: the console does not list an app grant the install's epoch has moved past (TASK-3399, U5b-1) * fix(oauth): U5b-1 codex r7: a consent carries the person's credential epoch and the barrier refuses it after a disable/re-enable; the console lists a grant not yet exchanged (TASK-3399, U5b-1) * feat(oauth): delegated app sign-in stays closed (access_denied, not yet available) behind one named guard until U5b-2 opens it with the app API (TASK-3399, U5b-1, lead ruling) * fix(oauth): U5b-1 codex r8: a consent carries the install epoch and membership its gate read, so a disable/re-enable or removal/re-add after the gate refuses the code (TASK-3399, U5b-1) * fix(oauth): U5b-1 codex r9: a delegated grant needs a live workspace, at the consent gate and the barrier (TASK-3399, U5b-1) * docs(oauth): name the accepted one-second residual of the consent's membership check (TASK-3399, U5b-1) * test: census the delegated consent's form readers, the app-grant revoke's user agent, and four binding columns that carry no caller text (TASK-3399, U5b-1) --- internal/metrics/metrics.go | 12 + internal/oauth/server.go | 40 ++ internal/server/app_delegated_consent.go | 240 +++++++ internal/server/app_tokens.go | 9 +- internal/server/body_reader_census_test.go | 31 +- internal/server/decode_json_nul_test.go | 7 +- internal/server/handlers_admin_mcp.go | 20 +- internal/server/handlers_connected_apps.go | 82 ++- internal/server/handlers_oauth.go | 54 +- internal/server/server.go | 18 +- .../server/task3394_install_tokens_test.go | 13 +- .../server/task3399_delegated_flow_test.go | 435 ++++++++++++ internal/store/account_claim.go | 30 +- internal/store/app_grants.go | 226 ++++++ internal/store/connected_apps.go | 4 +- internal/store/deadlock_retry.go | 71 ++ internal/store/install_clients.go | 355 +++++++++- internal/store/mcp_resume.go | 4 +- .../121_app_token_binding_access.sql | 36 + internal/store/nul_unprotected_baseline.txt | 4 + internal/store/oauth_connections_backfill.go | 4 +- internal/store/oauth_sweep.go | 54 +- .../095_app_token_binding_access.sql | 8 + internal/store/store.go | 5 + .../store/task3394_install_clients_test.go | 4 +- .../store/task3399_deadlock_retry_test.go | 107 +++ .../store/task3399_delegated_barrier_test.go | 586 ++++++++++++++++ internal/store/task3399_reader_census_test.go | 133 ++++ internal/store/users.go | 42 +- internal/store/workspace_members.go | 31 + web/src/lib/api/client.ts | 3 +- web/src/lib/types/index.ts | 12 + .../console/connected-apps/+page.svelte | 644 ++++++++++-------- .../connected-apps/appGrants.svelte.test.ts | 96 +++ 34 files changed, 3059 insertions(+), 361 deletions(-) create mode 100644 internal/server/app_delegated_consent.go create mode 100644 internal/server/task3399_delegated_flow_test.go create mode 100644 internal/store/app_grants.go create mode 100644 internal/store/deadlock_retry.go create mode 100644 internal/store/migrations/121_app_token_binding_access.sql create mode 100644 internal/store/pgmigrations/095_app_token_binding_access.sql create mode 100644 internal/store/task3399_deadlock_retry_test.go create mode 100644 internal/store/task3399_delegated_barrier_test.go create mode 100644 internal/store/task3399_reader_census_test.go create mode 100644 web/src/routes/console/connected-apps/appGrants.svelte.test.ts diff --git a/internal/metrics/metrics.go b/internal/metrics/metrics.go index 3b87ca93a..a761abd1e 100644 --- a/internal/metrics/metrics.go +++ b/internal/metrics/metrics.go @@ -422,6 +422,11 @@ type Metrics struct { // detail the log line carries. ContentWritesSupersededTotal prometheus.Counter + // DBDeadlockRetriesTotal counts store transactions run again after a + // Postgres deadlock (40P01), by site (TASK-3399). A site that climbs is a + // pair of writers whose lock order needs fixing, not just retrying. + DBDeadlockRetriesTotal *prometheus.CounterVec + // OAuth flow metrics (PLAN-943 TASK-961). Wired from // internal/server/handlers_oauth.go (per-handler seams) and // internal/oauth/storage.go (revocation TTL observation). @@ -538,6 +543,11 @@ func New() *Metrics { Help: "Number of currently-open MCP Streamable HTTP sessions.", }) + dbDeadlockRetriesTotal := prometheus.NewCounterVec(prometheus.CounterOpts{ + Name: "pad_db_deadlock_retries_total", + Help: "Store transactions run again after a Postgres deadlock (SQLSTATE 40P01), by site (TASK-3399).", + }, []string{"site"}) + contentWritesSupersededTotal := prometheus.NewCounter(prometheus.CounterOpts{ Name: "pad_content_writes_superseded_total", Help: "Item content writes refused because the same browser tab had already applied a newer one (BUG-3080).", @@ -727,6 +737,7 @@ func New() *Metrics { mcpPreAuthDeniedTotal, mcpActiveSessions, contentWritesSupersededTotal, + dbDeadlockRetriesTotal, oauthFlowsTotal, oauthResourceMissingTotal, planWritesTotal, @@ -770,6 +781,7 @@ func New() *Metrics { MCPPreAuthDeniedTotal: mcpPreAuthDeniedTotal, MCPActiveSessions: mcpActiveSessions, ContentWritesSupersededTotal: contentWritesSupersededTotal, + DBDeadlockRetriesTotal: dbDeadlockRetriesTotal, OAuthFlowsTotal: oauthFlowsTotal, OAuthResourceMissingTotal: oauthResourceMissingTotal, PlanWritesTotal: planWritesTotal, diff --git a/internal/oauth/server.go b/internal/oauth/server.go index 45ee82cc6..0dc6d6d81 100644 --- a/internal/oauth/server.go +++ b/internal/oauth/server.go @@ -10,6 +10,7 @@ import ( "github.com/ory/fosite/compose" "github.com/ory/fosite/handler/oauth2" + "github.com/PerpetualSoftware/pad/internal/models" "github.com/PerpetualSoftware/pad/internal/store" ) @@ -111,6 +112,7 @@ type Server struct { provider fosite.OAuth2Provider cfg Config storage *Storage + strategy *oauth2.HMACSHAStrategy } // NewServer constructs an OAuth 2.1 authorization server backed by @@ -261,9 +263,47 @@ func NewServer(cfg Config) (*Server, error) { provider: provider, cfg: cfg, storage: storage, + strategy: strategy, }, nil } +// GrantOwner names the client a presented authorization code or refresh +// token was issued to, active or not, or "" when it names nothing. It reads +// the row by the token's signature and writes nothing. +// +// fosite's code and refresh flows treat a used code or a rotated refresh +// token as a replay and revoke that grant's whole family BEFORE checking the +// client that presented it. An installed app's client is refused any grant +// it does not own before fosite runs (TASK-3399, keeping U5a's guarantee), +// so it can never revoke another client's family. +func (s *Server) GrantOwner(ctx context.Context, grantType, token string) (string, error) { + var sig string + switch grantType { + case "authorization_code": + sig = s.strategy.AuthorizeCodeSignature(ctx, token) + case "refresh_token": + sig = s.strategy.RefreshTokenSignature(ctx, token) + } + // A malformed token has no signature, and so names nothing. + if sig == "" { + return "", nil + } + var req *models.OAuthRequest + var err error + if grantType == "authorization_code" { + req, err = s.cfg.Store.GetAuthorizationCode(sig) + } else { + req, err = s.cfg.Store.GetRefreshToken(sig) + } + if req == nil { + if err != nil && !errors.Is(err, store.ErrOAuthNotFound) { + return "", err + } + return "", nil + } + return req.ClientID, nil +} + // Provider returns the fosite.OAuth2Provider that sub-PR C's HTTP // handlers will call (NewAuthorizeRequest, NewAccessRequest, // WriteAuthorizeResponse, etc.). Exposed as a method rather than a diff --git a/internal/server/app_delegated_consent.go b/internal/server/app_delegated_consent.go new file mode 100644 index 000000000..6173f7e4b --- /dev/null +++ b/internal/server/app_delegated_consent.go @@ -0,0 +1,240 @@ +package server + +import ( + "fmt" + "html/template" + "net/http" + "net/url" + + "github.com/ory/fosite" + + "github.com/PerpetualSoftware/pad/internal/models" + "github.com/PerpetualSoftware/pad/internal/oauth" + "github.com/PerpetualSoftware/pad/internal/store" +) + +// Delegated app sign-in (SPEC-6 U5b-1, TASK-3399; DOC-3371 §4 "Delegated +// tokens"): authorization code + PKCE for an install's client, so a Pad user +// can act through the app. The consent page's workspace is FIXED to the +// install: no workspace choice, no wildcard, no creation flag. The person +// chooses the access, read-only by default, and write only when the manifest +// offers it (lead ruling Q2). No oauth_connections row is written (lead +// ruling Q1): the install fixes the workspace, and the binding the issuance +// barrier writes carries the kind, epoch and access. + +// installClientOf returns the install client an authorize request names, or +// nil for any other client. +func (s *Server) installClientOf(ar fosite.AuthorizeRequester) (*models.OAuthClient, error) { + c, err := s.store.GetOAuthClient(ar.GetClient().GetID()) + if err != nil || c == nil || !c.IsInstallClient() { + return nil, err + } + return c, nil +} + +// appConsentRefusal is why a delegated sign-in cannot proceed: answered as +// an authorize error to the app (a registered redirect), except when apps +// are off, which answers the same 404 every app route gives. +type appConsentRefusal struct { + notFound bool + err *fosite.RFC6749Error +} + +// appConsentGate is every check the consent render AND the decision make, +// read fresh each time: apps on, the install active and offering delegated +// access, PKCE with S256, the app API audience, and the person a human +// member of the install's workspace. +func (s *Server) appConsentGate(ar fosite.AuthorizeRequester, user *models.User, client *models.OAuthClient) (*appConsentFacts, *appConsentRefusal) { + if !s.appsAvailable() { + return nil, &appConsentRefusal{notFound: true} + } + // Closed until U5b-2 (TASK-3399): the app API does not accept delegated + // tokens yet, so no grant is issued that it could not honour. + if !s.delegatedSignInOpen { + return nil, &appConsentRefusal{err: fosite.ErrAccessDenied.WithHint("Signing in to installed apps is not yet available.")} + } + st, err := s.store.GetInstallConsentState(client.AppInstallID) + if err != nil { + return nil, &appConsentRefusal{err: fosite.ErrServerError.WithWrap(err)} + } + if st == nil || st.State != "active" { + return nil, &appConsentRefusal{err: fosite.ErrAccessDenied.WithHint("This app is not available.")} + } + if st.DelegatedAccess != "read" && st.DelegatedAccess != "write" { + return nil, &appConsentRefusal{err: fosite.ErrAccessDenied.WithHint("This app does not offer sign-in.")} + } + // Second layer: fosite already refuses a missing or plain PKCE for every + // client (oauth/server.go EnforcePKCE, S256 only), so this check stays + // for the case a future config loosens that (U5b-1 mutant C2 survives). + form := ar.GetRequestForm() + if form.Get("code_challenge") == "" || form.Get("code_challenge_method") != "S256" { + return nil, &appConsentRefusal{err: fosite.ErrInvalidRequest.WithHint("An installed app signs people in with PKCE (S256).")} + } + // Second layer: an install client's fosite audiences are its own + // AllowedAudiences, the app API only (oauth/storage.go), so fosite + // already refuses any other resource (U5b-1 mutant C3 survives). + appAud := s.store.AppAPIAudience() + audOK := false + for _, a := range ar.GetRequestedAudience() { + if appAud != "" && oauth.NormalizeAudience(a) == oauth.NormalizeAudience(appAud) { + audOK = true + } else { + audOK = false + break + } + } + if !audOK { + return nil, &appConsentRefusal{err: fosite.ErrInvalidRequest.WithHint("An installed app's client holds the app API resource only.")} + } + since, err := s.store.WorkspaceMemberSince(st.WorkspaceID, user.ID) + if err != nil { + return nil, &appConsentRefusal{err: fosite.ErrServerError.WithWrap(err)} + } + if since == "" || user.IsApp() { + return nil, &appConsentRefusal{err: fosite.ErrAccessDenied.WithHint("You are not a member of the workspace this app is installed in.")} + } + return &appConsentFacts{InstallConsentState: st, MemberSince: since}, nil +} + +// appConsentFacts is what the consent gate read and the decision carries +// into the grant: the install state (with its epoch, read before the checks) +// and the person's membership as of the gate (codex U5b-1 r8). +type appConsentFacts struct { + *store.InstallConsentState + MemberSince string +} + +func (s *Server) writeAppConsentRefusal(w http.ResponseWriter, r *http.Request, ar fosite.AuthorizeRequester, ref *appConsentRefusal) { + s.recordOAuthFlow("failed") + if ref.notFound { + http.NotFound(w, r) + return + } + s.oauthServer.Provider().WriteAuthorizeError(r.Context(), s.authorizeResponseWriter(w), ar, ref.err) +} + +var appConsentTmpl = template.Must(template.New("app-consent").Parse(` + +Sign in to {{.AppName}} + +
+

Sign in to {{.AppName}}

+

{{.Origin}}

+

{{.AppName}} will act as you, {{.Username}}, in the workspace {{.WorkspaceName}} only, inside the collections the app was installed with.

+
+ + {{range $k, $vs := .HiddenFields}}{{range $vs}}{{end}}{{end}} +
What may it do as you? + + {{if .OfferWrite}}{{end}} +
+

You can revoke this at any time from Connected apps.

+
+ + +
+
+
`)) + +type appConsentData struct { + AppName, Origin, Username, WorkspaceName, CSRF string + OfferWrite bool + HiddenFields url.Values +} + +// renderAppConsent renders the delegated consent page. +func (s *Server) renderAppConsent(w http.ResponseWriter, r *http.Request, ar fosite.AuthorizeRequester, user *models.User, client *models.OAuthClient) { + st, ref := s.appConsentGate(ar, user, client) + if ref != nil { + s.writeAppConsentRefusal(w, r, ar, ref) + return + } + ws, err := s.store.GetWorkspaceByID(st.WorkspaceID) + if err != nil || ws == nil { + s.writeAppConsentRefusal(w, r, ar, &appConsentRefusal{err: fosite.ErrAccessDenied.WithHint("This app is not available.")}) + return + } + s.recordOAuthFlow("started") + csrf := readOrSetConsentCSRF(w, r, s.secureCookies) + w.Header().Set("Content-Security-Policy", + "default-src 'none'; style-src 'unsafe-inline'; form-action 'self'; frame-ancestors 'none'; base-uri 'none'") + w.Header().Set("Content-Type", "text/html; charset=utf-8") + w.Header().Set("Cache-Control", "no-store") + if err := appConsentTmpl.Execute(w, appConsentData{ + AppName: st.AppName, Origin: st.Origin, Username: user.Name, WorkspaceName: ws.Name, CSRF: csrf, + OfferWrite: st.DelegatedAccess == "write", + HiddenFields: allowlistedAuthorizeParams(r.URL.Query()), + }); err != nil { + writeInternalError(w, fmt.Errorf("oauth: render app consent: %w", err)) + } +} + +// decideAppConsent completes an approved delegated sign-in: the gate again, +// the person's access choice, and the authorization code, whose session +// carries the install's epoch (read now, as the code is minted; the store's +// barrier re-checks it at every persistence after this one), the kind and +// the access. Deny was answered by the caller. +func (s *Server) decideAppConsent(w http.ResponseWriter, r *http.Request, ar fosite.AuthorizeRequester, user *models.User, client *models.OAuthClient) { + st, ref := s.appConsentGate(ar, user, client) + if ref != nil { + s.writeAppConsentRefusal(w, r, ar, ref) + return + } + access := r.PostForm.Get("app_access") + if access == "" { + access = "read" // least privilege (lead ruling Q2) + } + if access != "read" && !(access == "write" && st.DelegatedAccess == "write") { + s.recordOAuthFlow("failed") + writeError(w, http.StatusBadRequest, "invalid_request", "app_access must be read, or write when the app offers it") + return + } + // The epoch and membership the gate read, never re-read here: a disable + // and re-enable, or a removal and re-add, after the gate refuses the code + // at the barrier (codex U5b-1 r8). + epoch := st.AuthEpoch + ar.GrantScope(store.InstallClientScope) + ar.GrantAudience(s.store.AppAPIAudience()) + session := oauth.NewSession(user.ID) + session.DefaultSession.Extra[store.InstallEpochSessionKey] = epoch + session.DefaultSession.Extra[store.InstallAuthKindSessionKey] = "delegated" + session.DefaultSession.Extra[store.InstallAccessSessionKey] = access + // The credentials this request's session resolved the person under: the + // barrier refuses the code if they changed since (a disable destroyed + // this session, then a re-enable), codex U5b-1 r7. + session.DefaultSession.Extra[store.InstallPersonEpochSessionKey] = user.CredentialEpoch + session.DefaultSession.Extra[store.InstallMemberSinceSessionKey] = st.MemberSince + resp, err := s.oauthServer.Provider().NewAuthorizeResponse(r.Context(), ar, session) + if err != nil { + s.recordOAuthFlow("failed") + s.oauthServer.Provider().WriteAuthorizeError(r.Context(), s.authorizeResponseWriter(w), ar, err) + return + } + s.recordOAuthFlow("completed") + s.oauthServer.Provider().WriteAuthorizeResponse(r.Context(), s.authorizeResponseWriter(w), ar, resp) +} + +// appSignInAvailable reports whether a request at the authorize or decide +// door names an installed app's client while apps are available: those doors +// serve that sign-in with MCP off too. It reads client_id from the query or +// the form; the full rules run later, in appConsentGate. +func (s *Server) appSignInAvailable(r *http.Request) bool { + if !s.appsAvailable() || s.oauthServer == nil { + return false + } + if err := r.ParseForm(); err != nil { + return false + } + c, err := s.store.GetOAuthClient(r.Form.Get("client_id")) + return err == nil && c != nil && c.IsInstallClient() +} diff --git a/internal/server/app_tokens.go b/internal/server/app_tokens.go index 126cb62e1..1fc25496f 100644 --- a/internal/server/app_tokens.go +++ b/internal/server/app_tokens.go @@ -61,13 +61,16 @@ func (s *Server) writeTokenError(ctx context.Context, w http.ResponseWriter, err // prepareInstallTokenRequest applies the install-client rules to a token // request BEFORE fosite reads it, and reports false after answering when one // refuses: -// - only client_credentials (delegated grants arrive with TASK-3399); +// - client_credentials (a service token), or authorization_code and +// refresh_token (a delegated grant, TASK-3399); // - `resource` is REQUIRED, with no MCP default (install clients are new, // so they never had the log-only phase TASK-3363 gave humans); // - an `audience` that names something else is refused, not reconciled. func (s *Server) prepareInstallTokenRequest(ctx context.Context, w http.ResponseWriter, r *http.Request) bool { - if r.PostForm.Get("grant_type") != "client_credentials" { - s.writeTokenError(ctx, w, fosite.ErrUnsupportedGrantType.WithHint("An installed app's client uses client_credentials.")) + switch r.PostForm.Get("grant_type") { + case "client_credentials", "authorization_code", "refresh_token": + default: + s.writeTokenError(ctx, w, fosite.ErrUnsupportedGrantType.WithHint("An installed app's client uses client_credentials, authorization_code or refresh_token.")) return false } resource := strings.TrimSpace(r.PostForm.Get("resource")) diff --git a/internal/server/body_reader_census_test.go b/internal/server/body_reader_census_test.go index 036c181b0..76a8bc0f3 100644 --- a/internal/server/body_reader_census_test.go +++ b/internal/server/body_reader_census_test.go @@ -276,6 +276,7 @@ func TestEveryRequestBodyReaderIsAccountedFor(t *testing.T) { "middleware_mcp_audit.go": "audit capture — parses the body ITSELF and binds the decoded method / params.name to mcp_audit_log.tool_name, so it is a second READER, not a pass-through. That the MCP dispatcher decodes the body again is true and says nothing about what this middleware persists — the earlier rationale here made exactly that mistake and certified it safe (codex round 20). parseMCPRequestBody now runs both caller-derived returns through sanitiseStoredText", "handlers_tokens.go": "guards on r.Body != nil && r.ContentLength != 0, then decodes THROUGH decodeJSON — so the body is read by the chokepoint, which applies the cap and the NUL rule. The earlier reason here said it never reads the body, which was simply false (codex round 29): a wrong reason in this list is the same defect as a missing entry, since both let a reader pass as reviewed", "app_tokens.go": "the install-client token rules (TASK-3394) read the same FORM-encoded OAuth body handleOAuthToken/handleOAuthIntrospect read, behind ValidateFormBody (BUG-2811), to classify the client exactly as fosite will", + "app_delegated_consent.go": "the delegated app sign-in (TASK-3399) is the OAuth authorize/decide doors' form: appSignInAvailable parses it for client_id and decideAppConsent reads app_access, both on routes wrapped in ValidateFormBody (BUG-2811) like the rest of handlers_oauth.go; a form-encoded body, never JSON", "handlers_oauth.go": "the OAuth handlers read FORM-encoded bodies (r.Form/FormValue). Every POST route they serve is wrapped in ValidateFormBody (BUG-2811), which applies bindableText to the body before these reads. The transport rules cover the query half of r.Form; ValidateFormBody covers the body half", "middleware_form_body.go": "the form-body chokepoint itself: ValidateFormBody reads a form-encoded body up to net/http's own ParseForm cap, checks it with validQueryText, and hands the same bytes (or the same read error) back to the handler (BUG-2811)", "handlers_watches.go": "guards on r.Body != nil && r.ContentLength != 0, then decodes THROUGH decodeJSON — the closing-round-4 fix for the chunked-body drop; the one reader expression is the nil check itself, and the body bytes flow through the chokepoint", @@ -306,19 +307,23 @@ func TestEveryRequestBodyReaderIsAccountedFor(t *testing.T) { // TASK-3394: parsed as fosite parses it (multipart included), so the // client classified for the install-client rules is fosite's client. "handlers_oauth.go::Server.handleOAuthToken::ParseMultipartForm": 1, - "handlers_oauth.go::Server.handleOAuthToken::PostForm": 1, - "handlers_oauth.go::Server.handleOAuthIntrospect::ParseForm": 1, - "app_tokens.go::Server.prepareInstallTokenRequest::PostForm": 4, - "app_tokens.go::requestClientID::PostForm": 1, - "handlers_oauth.go::Server.handleOAuthRevoke::ParseForm": 1, - "handlers_oauth.go::Server.handleOAuthRevoke::PostForm": 1, - "handlers_oauth.go::Server.validateConsentCSRFToken::FormValue": 1, - "handlers_tokens.go::Server.handleRotateUserToken::Body": 1, - "handlers_watches.go::Server.handleCreateWatch::Body": 1, - "import_read_deadline.go::Server.withImportReadDeadline::Body": 2, - "middleware_mcp_audit.go::Server.MCPAuditLog::Body": 5, - "middleware_mcp_audit.go::Server.emitMCPAuditDenied::Body": 2, - "middleware_request_text.go::readBodyForDecode::Body": 4, + "app_delegated_consent.go::Server.appSignInAvailable::ParseForm": 1, + "app_delegated_consent.go::Server.decideAppConsent::PostForm": 1, + // TASK-3399 adds four: the install client's grant type and the code + // or refresh token its ownership is checked by before fosite runs. + "handlers_oauth.go::Server.handleOAuthToken::PostForm": 5, + "handlers_oauth.go::Server.handleOAuthIntrospect::ParseForm": 1, + "app_tokens.go::Server.prepareInstallTokenRequest::PostForm": 4, + "app_tokens.go::requestClientID::PostForm": 1, + "handlers_oauth.go::Server.handleOAuthRevoke::ParseForm": 1, + "handlers_oauth.go::Server.handleOAuthRevoke::PostForm": 1, + "handlers_oauth.go::Server.validateConsentCSRFToken::FormValue": 1, + "handlers_tokens.go::Server.handleRotateUserToken::Body": 1, + "handlers_watches.go::Server.handleCreateWatch::Body": 1, + "import_read_deadline.go::Server.withImportReadDeadline::Body": 2, + "middleware_mcp_audit.go::Server.MCPAuditLog::Body": 5, + "middleware_mcp_audit.go::Server.emitMCPAuditDenied::Body": 2, + "middleware_request_text.go::readBodyForDecode::Body": 4, } sites := collectRequestBodyReads(t, ".") diff --git a/internal/server/decode_json_nul_test.go b/internal/server/decode_json_nul_test.go index 9dacceb70..57b68d86a 100644 --- a/internal/server/decode_json_nul_test.go +++ b/internal/server/decode_json_nul_test.go @@ -902,8 +902,11 @@ func TestTextSafeHelpersAreUsedAtEveryCallSite(t *testing.T) { // which only substring-matches the mobile shells' marker to choose // plan-limit copy and binds nothing to a column. It goes through the // helper because this scan admits no other raw reader. - if safeUAUses != 6 { - t.Errorf("requestUserAgent occurrences = %d, want 6 (1 declaration + 5 call sites); "+ + // The sixth is the app-grant revoke's audit row (TASK-3399), the same + // audit shape as the MCP connection revoke beside it: the sanitised + // header into activities.user_agent. + if safeUAUses != 7 { + t.Errorf("requestUserAgent occurrences = %d, want 7 (1 declaration + 6 call sites); "+ "a site was added or removed — re-justify and re-pin", safeUAUses) } } diff --git a/internal/server/handlers_admin_mcp.go b/internal/server/handlers_admin_mcp.go index fd1236d11..c39b77514 100644 --- a/internal/server/handlers_admin_mcp.go +++ b/internal/server/handlers_admin_mcp.go @@ -158,7 +158,11 @@ func (s *Server) requireMCPAvailable(next http.Handler) http.Handler { // install client unless MCP is on. func (s *Server) requireOAuthRoutesAvailable(next http.Handler) http.Handler { return http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { - if s.oauthAvailable() || (r.URL.Path == "/oauth/token" && s.appsAvailable()) { + // With OAuth off, installed apps still need the token door (U5a) and + // the sign-in doors (TASK-3399); those handlers then admit only an + // installed app's client (appSignInAvailable, installClientFor). + appDoor := r.URL.Path == "/oauth/token" || r.URL.Path == "/oauth/authorize" || r.URL.Path == "/oauth/authorize/decide" + if s.oauthAvailable() || (appDoor && s.appsAvailable()) { next.ServeHTTP(w, r) return } @@ -176,6 +180,20 @@ func (s *Server) requireOAuthAvailable(next http.Handler) http.Handler { }) } +// requireOAuthOrAppsAvailable gates the Connected Apps list and revoke: they +// manage MCP connections when OAuth is available, and the grants people gave +// installed apps when apps are (TASK-3399, codex U5b-1 r1): turning MCP off +// must not strand an app grant its person cannot see or revoke. +func (s *Server) requireOAuthOrAppsAvailable(next http.Handler) http.Handler { + return http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + if !s.oauthAvailable() && !s.appsAvailable() { + writeError(w, http.StatusNotFound, "not_found", "Not found") + return + } + next.ServeHTTP(w, r) + }) +} + type mcpReadiness struct { Origin string `json:"origin"` OriginVar string `json:"origin_var,omitempty"` diff --git a/internal/server/handlers_connected_apps.go b/internal/server/handlers_connected_apps.go index ec756f183..b31bf2395 100644 --- a/internal/server/handlers_connected_apps.go +++ b/internal/server/handlers_connected_apps.go @@ -115,10 +115,16 @@ func (s *Server) handleListConnectedApps(w http.ResponseWriter, r *http.Request) return } - conns, err := s.store.ListUserOAuthConnections(user.ID) - if err != nil { - writeInternalError(w, err) - return + // MCP connections are listed only while OAuth is available; app grants + // are listed whenever this route is (requireOAuthOrAppsAvailable). + var conns []models.OAuthConnection + if s.oauthAvailable() { + var err error + conns, err = s.store.ListUserOAuthConnections(user.ID) + if err != nil { + writeInternalError(w, err) + return + } } // Enrich with audit-log aggregates. One bulk query — see @@ -143,11 +149,44 @@ func (s *Server) handleListConnectedApps(w http.ResponseWriter, r *http.Request) out = append(out, connectionToDTO(c, statPtr)) } + // Delegated grants given to installed apps (TASK-3399): listed on their + // own, read-only plus revoke, never as MCP connections (no + // oauth_connections row, lead ruling Q1). Additive member. + grants, err := s.store.ListUserAppGrants(user.ID) + if err != nil { + writeInternalError(w, err) + return + } + appGrants := make([]appGrantDTO, 0, len(grants)) + for _, g := range grants { + appGrants = append(appGrants, appGrantDTO{ + ID: g.RequestID, AppName: g.AppName, Origin: g.Origin, Access: g.Access, GrantedAt: g.GrantedAt, + Workspace: appGrantWorkspaceDTO{Slug: g.WorkspaceSlug, Name: g.WorkspaceName}, + }) + } + writeJSON(w, http.StatusOK, map[string]any{ - "items": out, + "items": out, + "app_grants": appGrants, }) } +// appGrantDTO is one delegated grant to an installed app on the Connected +// Apps page: who, where, what access, since when. It can only be revoked. +type appGrantDTO struct { + ID string `json:"id"` + AppName string `json:"app_name"` + Origin string `json:"origin"` + Workspace appGrantWorkspaceDTO `json:"workspace"` + Access string `json:"access"` + GrantedAt time.Time `json:"granted_at"` +} + +type appGrantWorkspaceDTO struct { + Slug string `json:"slug"` + Name string `json:"name"` +} + // handleRevokeConnectedApp invalidates every token in the OAuth // grant chain identified by the URL id parameter, after verifying // ownership. @@ -180,6 +219,39 @@ func (s *Server) handleRevokeConnectedApp(w http.ResponseWriter, r *http.Request return } + // A delegated grant to an installed app (TASK-3399) is revoked through + // its binding: the tokens deactivated, an unused code and PKCE row + // deleted, and the binding deleted, which introspection reads. + if isGrant, err := s.store.IsUserAppGrant(user.ID, id); err != nil { + writeInternalError(w, err) + return + } else if isGrant { + installID, err := s.store.RevokeUserAppGrant(user.ID, id) + if errors.Is(err, store.ErrAppGrantNotFound) { + writeError(w, http.StatusNotFound, "not_found", "Connection not found.") + return + } + if err != nil { + writeInternalError(w, err) + return + } + metaJSON, _ := json.Marshal(map[string]string{"connection_id": id, "app_install_id": installID}) + if _, err := s.store.CreateActivity(models.Activity{ + Action: "oauth_connection_revoked", Actor: "user", Source: "web", UserID: user.ID, + Metadata: string(metaJSON), IPAddress: clientIP(r), UserAgent: requestUserAgent(r), + }); err != nil { + slog.Warn("connected-apps: audit log write failed", "error", err, "user_id", user.ID, "connection_id", id) + } + w.WriteHeader(http.StatusNoContent) + return + } + + // MCP connections are managed only while OAuth is available. + if !s.oauthAvailable() { + writeError(w, http.StatusNotFound, "not_found", "Connection not found.") + return + } + // Pre-revoke lookup so the audit trail can carry the client_id. // Cheap (we'd need to hit the table anyway via Revoke). Failure // here doesn't block the revoke — we'll just record an audit row diff --git a/internal/server/handlers_oauth.go b/internal/server/handlers_oauth.go index 0d3f8ecbc..49c896e8f 100644 --- a/internal/server/handlers_oauth.go +++ b/internal/server/handlers_oauth.go @@ -632,7 +632,9 @@ func (s *Server) handleOAuthAuthorize(w http.ResponseWriter, r *http.Request) { start := time.Now() defer s.observeOAuthFlowDuration("authorize", start) - if !s.oauthAvailable() { + // An installed app's sign-in is served while apps are available, even + // with MCP off (TASK-3399, codex U5b-1 r1); everything else needs OAuth. + if !s.oauthAvailable() && !s.appSignInAvailable(r) { http.NotFound(w, r) return } @@ -734,6 +736,16 @@ func (s *Server) handleOAuthAuthorize(w http.ResponseWriter, r *http.Request) { return } + // An installed app's client signs a person in to that app only, on its + // own fixed-workspace consent page (TASK-3399). + if client, err := s.installClientOf(ar); err != nil { + writeInternalError(w, fmt.Errorf("oauth: read client: %w", err)) + return + } else if client != nil { + s.renderAppConsent(w, r, ar, user, client) + return + } + // TASK-961: consent render is the canonical "flow started" signal — // the user has identified themselves AND fosite has accepted the // authorize request. From here the only outcomes are @@ -791,7 +803,7 @@ func (s *Server) handleOAuthAuthorizeDecide(w http.ResponseWriter, r *http.Reque start := time.Now() defer s.observeOAuthFlowDuration("decide", start) - if !s.oauthAvailable() { + if !s.oauthAvailable() && !s.appSignInAvailable(r) { http.NotFound(w, r) return } @@ -873,6 +885,17 @@ func (s *Server) handleOAuthAuthorizeDecide(w http.ResponseWriter, r *http.Reque return } + // An installed app's delegated sign-in (TASK-3399): its own decision, + // with no workspace choice and no oauth_connections row. + if client, err := s.installClientOf(ar); err != nil { + s.recordOAuthFlow("failed") + writeInternalError(w, fmt.Errorf("oauth: read client: %w", err)) + return + } else if client != nil { + s.decideAppConsent(w, r, ar, user, client) + return + } + // Parse + validate the consent payload. decision, vErr := s.parseConsentPayload(r, ar, user) if vErr != nil { @@ -1217,10 +1240,30 @@ func (s *Server) handleOAuthToken(w http.ResponseWriter, r *http.Request) { http.NotFound(w, r) return } - // Resource required, no MCP default, client_credentials only. + // Resource required, no MCP default; client_credentials, or a + // delegated grant's code or refresh (TASK-3399). if !s.prepareInstallTokenRequest(ctx, w, r) { return } + // A code or refresh token presented by an installed app's client + // must be its own, checked BEFORE fosite: fosite revokes a used + // code's or a rotated refresh token's whole family as a replay + // before it checks the presenting client (U5a's guarantee, kept). + if gt := r.PostForm.Get("grant_type"); gt == "authorization_code" || gt == "refresh_token" { + tok := r.PostForm.Get("code") + if gt == "refresh_token" { + tok = r.PostForm.Get("refresh_token") + } + owner, err := s.oauthServer.GrantOwner(ctx, gt, tok) + if err != nil { + s.writeTokenError(ctx, w, fosite.ErrServerError.WithWrap(err)) + return + } + if owner != installClient.ID { + s.writeTokenError(ctx, w, fosite.ErrInvalidGrant.WithHint("The grant was not issued to this app.")) + return + } + } case !mcpOn: http.NotFound(w, r) return @@ -1247,7 +1290,10 @@ func (s *Server) handleOAuthToken(w http.ResponseWriter, r *http.Request) { } // An installed app's service token: the app scope and audience, and its // install's bot as the subject (fosite's client_credentials grants none). - if installClient != nil { + // A code or refresh exchange (a delegated grant, TASK-3399) carries what + // its consent decision stored: the person, the epoch, the kind and the + // access; the barrier re-checks them as it persists. + if installClient != nil && r.PostForm.Get("grant_type") == "client_credentials" { if err := s.grantInstallServiceToken(ar, installClient, preEpoch); err != nil { s.oauthServer.Provider().WriteAccessError(ctx, w, ar, err) return diff --git a/internal/server/server.go b/internal/server/server.go index 40f940962..6b13522bd 100644 --- a/internal/server/server.go +++ b/internal/server/server.go @@ -102,6 +102,12 @@ type Server struct { // appBeforeHandler is a TEST-ONLY seam, nil in production: it runs after // the app middleware admitted a request and before its handler. appBeforeHandler func() + // delegatedSignInOpen is THE guard on delegated app sign-in (TASK-3399, + // lead ruling): false, so the consent page answers access_denied, until + // U5b-2 deletes this field in the same PR that makes the app API accept + // delegated tokens, so a grant and its use go live together. Tests open + // it on their own server. + delegatedSignInOpen bool // appBeforeFirstByte is a TEST-ONLY seam, nil in production: it runs in // an app download after the blob is open and before the re-admission // gate that precedes the first byte. @@ -1249,6 +1255,9 @@ func (s *Server) countMidStreamResync(activity bool) { // no-ops until both prerequisites are present. func (s *Server) SetMetrics(m *metrics.Metrics) { s.metrics = m + if m != nil && m.DBDeadlockRetriesTotal != nil { + s.store.SetDeadlockRetryObserver(func(site string) { m.DBDeadlockRetriesTotal.WithLabelValues(site).Inc() }) + } s.wireOAuthMetricsObserver() s.wireStreamGauge() } @@ -1843,10 +1852,17 @@ func (s *Server) setupRouter() { // OAuth there are no grants to manage. The audit route // above is deliberately outside the gate: it is history // of a connection the caller owns. + // The list and the revoke also serve the grants people gave + // installed apps, so they are open when either OAuth or apps + // are (TASK-3399); with OAuth off they show and revoke app + // grants only. r.Group(func(r chi.Router) { - r.Use(s.requireOAuthAvailable) + r.Use(s.requireOAuthOrAppsAvailable) r.Get("/connected-apps", s.handleListConnectedApps) r.Delete("/connected-apps/{id}", s.handleRevokeConnectedApp) + }) + r.Group(func(r chi.Router) { + r.Use(s.requireOAuthAvailable) // PLAN-1519 / TASK-1524 / IDEA-1517 §3: mutation // endpoints for the connections-page UI. Per-field // patches rather than a general PATCH for cleaner diff --git a/internal/server/task3394_install_tokens_test.go b/internal/server/task3394_install_tokens_test.go index e270e0fdb..5436a3296 100644 --- a/internal/server/task3394_install_tokens_test.go +++ b/internal/server/task3394_install_tokens_test.go @@ -154,7 +154,11 @@ func TestTask3394_InstallTokenRequestRules(t *testing.T) { f.Set("audience", testCanonicalAudience) return f }()}, - {"a delegated grant", "uses client_credentials", url.Values{"grant_type": {"refresh_token"}, "refresh_token": {"x"}, "resource": {testAppAPIAudience}}}, + // TASK-3399: a delegated grant's code and refresh are admitted, but + // only for a grant this client owns, checked before fosite. + {"a refresh token it does not own", "not issued to this app", url.Values{"grant_type": {"refresh_token"}, "refresh_token": {"x"}, "resource": {testAppAPIAudience}}}, + {"a code it does not own", "not issued to this app", url.Values{"grant_type": {"authorization_code"}, "code": {"x"}, "redirect_uri": {"https://portal.example/cb"}, "resource": {testAppAPIAudience}}}, + {"another grant type", "uses client_credentials, authorization_code or refresh_token", url.Values{"grant_type": {"password"}, "username": {"a"}, "password": {"b"}, "resource": {testAppAPIAudience}}}, } for _, tc := range cases { rr := postTokenBasic(srv, tc.form, in.clientID, in.secret) @@ -362,8 +366,9 @@ func TestTask3394_PublicIntrospectionCallerEncodings(t *testing.T) { // codex r2 P2: the token endpoint classifies the client from the body fosite // will parse, multipart included. A multipart request from an install client -// must meet the same rules (client_credentials only), never reach fosite's -// refresh-reuse handling, and so never revoke another client's family. +// must meet the same rules, never reach fosite's refresh-reuse handling with +// a grant it does not own, and so never revoke another client's family +// (TASK-3399 admits its OWN refresh tokens, checked before fosite). func TestTask3394_MultipartTokenRequestFromAnInstallClient(t *testing.T) { srv := appOAuthServer(t, true) in := newTestInstall(t, srv, "inst-multi") @@ -395,7 +400,7 @@ func TestTask3394_MultipartTokenRequestFromAnInstallClient(t *testing.T) { req.RemoteAddr = "192.0.2.1:1234" mrr := httptest.NewRecorder() srv.ServeHTTP(mrr, req) - if !strings.Contains(mrr.Body.String(), "uses client_credentials") { + if !strings.Contains(mrr.Body.String(), "not issued to this app") { t.Errorf("a multipart install-client refresh: %d %s, want the install-client refusal", mrr.Code, mrr.Body.String()) } // The victim's current family survives. diff --git a/internal/server/task3399_delegated_flow_test.go b/internal/server/task3399_delegated_flow_test.go new file mode 100644 index 000000000..b57e8feb0 --- /dev/null +++ b/internal/server/task3399_delegated_flow_test.go @@ -0,0 +1,435 @@ +package server + +import ( + "context" + "net/http" + "net/http/httptest" + "net/url" + "strings" + "testing" + + "github.com/PerpetualSoftware/pad/internal/models" +) + +// TASK-3399 (SPEC-6 U5b-1): a person signs in to an installed app with +// authorization code + PKCE, on a consent page fixed to the install. + +type delegatedFix struct { + selfHost bool + srv *Server + in testInstall + person *models.User + sessionToken string + csrf string +} + +func delegatedFixture(t *testing.T, installID, offered string) delegatedFix { + t.Helper() + return delegatedFixtureOn(t, installID, offered, true) +} + +// delegatedFixtureOn builds the fixture on Pad Cloud, or on a self-host with +// apps enabled and MCP left off (cloud=false). +func delegatedFixtureOn(t *testing.T, installID, offered string, cloud bool) delegatedFix { + t.Helper() + srv := appOAuthServer(t, cloud) + srv.delegatedSignInOpen = true // the guard U5b-2 deletes; see TestTask3399_DelegatedSignInIsClosedUntilU5b2 + if !cloud { + if err := srv.store.SetPlatformSetting(settingAppsEnabled, "true"); err != nil { + t.Fatal(err) + } + } + in := newTestInstall(t, srv, installID) + if offered != "" { + if _, err := srv.store.DB().Exec(`UPDATE app_installs SET delegated_access = ? WHERE id = ?`, offered, installID); err != nil { + t.Fatal(err) + } + } + person, sessionToken := loginTestUserAs(t, srv, "person-"+installID+"@example.com", "Dana", "password123") + if err := srv.store.AddWorkspaceMember(in.wsID, person.ID, "editor"); err != nil { + t.Fatal(err) + } + f := delegatedFix{srv: srv, in: in, person: person, sessionToken: sessionToken, selfHost: !cloud} + f.csrf = f.csrfFromAppConsent(t) + return f +} + +// serve sends req as the browser would: on a self-host, to the configured +// origin's host (requireConfiguredHost refuses any other). +func (f delegatedFix) serve(req *http.Request) *httptest.ResponseRecorder { + if f.selfHost { + req.Host = "app.test.example" + } + req.Header.Set("User-Agent", testSessionUA) + req.AddCookie(&http.Cookie{Name: sessionCookieName(f.srv.secureCookies), Value: f.sessionToken}) + req.RemoteAddr = "192.0.2.1:1234" + rr := httptest.NewRecorder() + f.srv.ServeHTTP(rr, req) + return rr +} + +func (f delegatedFix) get(path string) *httptest.ResponseRecorder { + return f.serve(httptest.NewRequest("GET", path, nil)) +} + +// api sends an authenticated API request with the double-submit CSRF pair. +func (f delegatedFix) api(method, path string) *httptest.ResponseRecorder { + const csrf = "0123456789abcdef0123456789abcdef0123456789abcdef0123456789abcdef" + req := httptest.NewRequest(method, path, nil) + req.AddCookie(&http.Cookie{Name: csrfCookieName(f.srv.secureCookies), Value: csrf}) + req.Header.Set("X-CSRF-Token", csrf) + return f.serve(req) +} + +// csrfFromAppConsent reads the consent CSRF cookie from the app's own consent +// page, which serves with MCP off too (readCSRFFromCookie registers an MCP +// client, which needs MCP on). +func (f delegatedFix) csrfFromAppConsent(t *testing.T) string { + t.Helper() + q := f.authorizeParams() + rr := f.get("/oauth/authorize?" + q.Encode()) + for _, c := range rr.Result().Cookies() { + if c.Name == csrfCookieName(f.srv.secureCookies) && c.Value != "" { + return c.Value + } + } + // No membership or no delegated access: the page refused, and the tests + // that build such a fixture never post a decision needing the cookie. + return "no-csrf-" + f.in.id +} + +const delegatedVerifier = "verifier-3399-abcdefghijklmnopqrstuvwxyz0123456789ABCDEF" + +func (f delegatedFix) authorizeParams() url.Values { + return url.Values{ + "client_id": {f.in.clientID}, + "response_type": {"code"}, + "redirect_uri": {"https://portal.example/cb"}, + "code_challenge": {s256Challenge(delegatedVerifier)}, + "code_challenge_method": {"S256"}, + "state": {"state-3399-0001"}, + "resource": {testAppAPIAudience}, + } +} + +func (f delegatedFix) decide(t *testing.T, params url.Values, decision, access string) *httptest.ResponseRecorder { + t.Helper() + form := url.Values{} + for k, v := range params { + form[k] = v + } + form.Set("decision", decision) + form.Set("csrf_token", f.csrf) + if access != "" { + form.Set("app_access", access) + } + req := httptest.NewRequest("POST", "/oauth/authorize/decide", strings.NewReader(form.Encode())) + req.Header.Set("Content-Type", "application/x-www-form-urlencoded") + req.AddCookie(&http.Cookie{Name: csrfCookieName(f.srv.secureCookies), Value: f.csrf}) + return f.serve(req) +} + +// codeFrom returns the code a decide redirect carries, or "" with the +// redirect's error. +func codeFrom(rr *httptest.ResponseRecorder) (code, oauthErr string) { + cb, _ := url.Parse(rr.Header().Get("Location")) + if cb == nil { + return "", "" + } + return cb.Query().Get("code"), cb.Query().Get("error") +} + +func (f delegatedFix) exchange(code string) *httptest.ResponseRecorder { + return postTokenBasic(f.srv, url.Values{ + "grant_type": {"authorization_code"}, "code": {code}, "redirect_uri": {"https://portal.example/cb"}, + "code_verifier": {delegatedVerifier}, "resource": {testAppAPIAudience}, + }, f.in.clientID, f.in.secret) +} + +func TestTask3399_TheConsentPageIsFixedToTheInstall(t *testing.T) { + f := delegatedFixture(t, "inst-page", "write") + q := f.authorizeParams() + rr := f.get("/oauth/authorize?" + q.Encode()) + if rr.Code != http.StatusOK { + t.Fatalf("consent page: %d %s", rr.Code, rr.Body.String()) + } + body := rr.Body.String() + for _, want := range []string{"WS inst-page", `value="read" checked`, `value="write"`, "Portal"} { + if !strings.Contains(body, want) { + t.Errorf("consent page lacks %q", want) + } + } + for _, never := range []string{"allowed_workspaces", "may_create_workspaces", "capability_tier", "workspace_access"} { + if strings.Contains(body, never) { + t.Errorf("consent page offers %q: the workspace is fixed to the install", never) + } + } + // A manifest that offers read only renders no write choice. + g := delegatedFixture(t, "inst-page-ro", "read") + q = g.authorizeParams() + if rr := doAuthedRequest(g.srv, "GET", "/oauth/authorize?"+q.Encode(), nil, g.sessionToken); strings.Contains(rr.Body.String(), `value="write"`) { + t.Error("a read-only manifest offered write") + } +} + +func TestTask3399_ADelegatedSignInIssuesAPersonsToken(t *testing.T) { + f := delegatedFixture(t, "inst-flow", "write") + connsBefore := count3399(t, f.srv, `SELECT COUNT(*) FROM oauth_connections`) + rr := f.decide(t, f.authorizeParams(), "approve", "") // no choice: read-only + code, oerr := codeFrom(rr) + if code == "" { + t.Fatalf("decide: %d, error %q", rr.Code, oerr) + } + trr := f.exchange(code) + if trr.Code != http.StatusOK { + t.Fatalf("exchange: %d %s", trr.Code, trr.Body.String()) + } + var resp map[string]any + parseJSON(t, trr, &resp) + tok, _ := resp["access_token"].(string) + g, err := f.srv.introspectAppToken(context.Background(), tok) + if err != nil { + t.Fatalf("introspect: %v", err) + } + if g.AuthKind != "delegated" || g.Subject != f.person.ID || g.InstallID != f.in.id || g.WorkspaceID != f.in.wsID { + t.Errorf("grant = %+v, want a delegated grant for the person", g) + } + var access string + if err := f.srv.store.DB().QueryRow(`SELECT delegated_access FROM app_token_bindings WHERE request_id = ?`, g.RequestID).Scan(&access); err != nil { + t.Fatal(err) + } + if access != "read" { + t.Errorf("consented access = %q, want the read-only default", access) + } + if n := count3399(t, f.srv, `SELECT COUNT(*) FROM oauth_connections`); n != connsBefore { + t.Errorf("a delegated sign-in wrote %d oauth_connections rows (lead ruling Q1: none)", n-connsBefore) + } + // The app refreshes its own grant. + refresh, _ := resp["refresh_token"].(string) + if rr := postTokenBasic(f.srv, url.Values{"grant_type": {"refresh_token"}, "refresh_token": {refresh}, "resource": {testAppAPIAudience}}, + f.in.clientID, f.in.secret); rr.Code != http.StatusOK { + t.Errorf("refresh: %d %s", rr.Code, rr.Body.String()) + } +} + +func TestTask3399_WriteIsChosenOnlyWhenOffered(t *testing.T) { + f := delegatedFixture(t, "inst-w", "write") + code, _ := codeFrom(f.decide(t, f.authorizeParams(), "approve", "write")) + if code == "" || f.exchange(code).Code != http.StatusOK { + t.Fatal("a write choice on a write manifest was refused") + } + g := delegatedFixture(t, "inst-ro", "read") + if rr := g.decide(t, g.authorizeParams(), "approve", "write"); rr.Code != http.StatusBadRequest { + t.Errorf("write on a read-only manifest: %d, want 400", rr.Code) + } +} + +// The lead's required race: a disable or rotate between the consent +// decision (the code is minted) and the token exchange refuses the exchange. +func TestTask3399_ADisableBetweenAuthorizeAndExchangeRefusesTheToken(t *testing.T) { + for name, q := range map[string]string{ + "disable": `UPDATE app_installs SET state = 'disabling', auth_epoch = auth_epoch + 1 WHERE id = ?`, + "rotate": `UPDATE app_installs SET auth_epoch = auth_epoch + 1 WHERE id = ?`, + } { + t.Run(name, func(t *testing.T) { + f := delegatedFixture(t, "inst-race-"+name, "write") + code, _ := codeFrom(f.decide(t, f.authorizeParams(), "approve", "read")) + if code == "" { + t.Fatal("no code") + } + if _, err := f.srv.store.DB().Exec(q, f.in.id); err != nil { + t.Fatal(err) + } + rr := f.exchange(code) + if rr.Code == http.StatusOK || strings.Contains(rr.Body.String(), "access_token") { + t.Errorf("exchange after a %s: %d %s, want a refusal", name, rr.Code, rr.Body.String()) + } + }) + } +} + +func TestTask3399_SignInRefusals(t *testing.T) { + // A non-member is refused, with no code. + f := delegatedFixture(t, "inst-nonmember", "write") + if _, err := f.srv.store.DB().Exec(`DELETE FROM workspace_members WHERE user_id = ? AND workspace_id = ?`, f.person.ID, f.in.wsID); err != nil { + t.Fatal(err) + } + if code, oerr := codeFrom(f.decide(t, f.authorizeParams(), "approve", "read")); code != "" || oerr != "access_denied" { + t.Errorf("a non-member: code %q, error %q, want access_denied", code, oerr) + } + // An install offering no delegated access. + g := delegatedFixture(t, "inst-nodeleg", "") + if code, oerr := codeFrom(g.decide(t, g.authorizeParams(), "approve", "read")); code != "" || oerr != "access_denied" { + t.Errorf("no delegated access offered: code %q, error %q, want access_denied", code, oerr) + } + // No PKCE. + h := delegatedFixture(t, "inst-nopkce", "write") + p := h.authorizeParams() + p.Del("code_challenge") + p.Del("code_challenge_method") + if code, _ := codeFrom(h.decide(t, p, "approve", "read")); code != "" { + t.Error("a sign-in without PKCE minted a code") + } + // The MCP resource, not the app API's. + k := delegatedFixture(t, "inst-mcpres", "write") + p = k.authorizeParams() + p.Set("resource", testCanonicalAudience) + if code, _ := codeFrom(k.decide(t, p, "approve", "read")); code != "" { + t.Error("a sign-in for the MCP resource minted a code") + } + // No resource at all: the app API's must be named. + n := delegatedFixture(t, "inst-nores", "write") + p = n.authorizeParams() + p.Del("resource") + if code, _ := codeFrom(n.decide(t, p, "approve", "read")); code != "" { + t.Error("a sign-in naming no resource minted a code") + } + // A disabled install. + m := delegatedFixture(t, "inst-inactive", "write") + if _, err := m.srv.store.DB().Exec(`UPDATE app_installs SET state = 'inactive' WHERE id = ?`, m.in.id); err != nil { + t.Fatal(err) + } + if code, oerr := codeFrom(m.decide(t, m.authorizeParams(), "approve", "read")); code != "" || oerr != "access_denied" { + t.Errorf("an inactive install: code %q, error %q", code, oerr) + } +} + +func count3399(t *testing.T, srv *Server, q string, args ...any) int { + t.Helper() + var n int + if err := srv.store.DB().QueryRow(q, args...).Scan(&n); err != nil { + t.Fatal(err) + } + return n +} + +// signIn runs a whole delegated sign-in and returns the access token and the +// grant's request id. +func (f delegatedFix) signIn(t *testing.T, access string) (string, string) { + t.Helper() + code, oerr := codeFrom(f.decide(t, f.authorizeParams(), "approve", access)) + if code == "" { + t.Fatalf("sign-in: no code (%s)", oerr) + } + rr := f.exchange(code) + if rr.Code != http.StatusOK { + t.Fatalf("exchange: %d %s", rr.Code, rr.Body.String()) + } + var resp map[string]any + parseJSON(t, rr, &resp) + tok, _ := resp["access_token"].(string) + g, err := f.srv.introspectAppToken(context.Background(), tok) + if err != nil { + t.Fatal(err) + } + return tok, g.RequestID +} + +// Lead ruling Q1: the console lists an app grant on its own, read-only plus +// revoke; every connection mutation refuses it and changes nothing. +func TestTask3399_TheConsoleShowsAndRevokesAppGrants(t *testing.T) { + f := delegatedFixture(t, "inst-console", "write") + tok, reqID := f.signIn(t, "write") + + rr := doAuthedJSON(f.srv, "GET", "/api/v1/connected-apps", nil, f.sessionToken) + if rr.Code != http.StatusOK { + t.Fatalf("list: %d %s", rr.Code, rr.Body.String()) + } + var list struct { + Items []map[string]any `json:"items"` + AppGrants []map[string]any `json:"app_grants"` + } + parseJSON(t, rr, &list) + for _, it := range list.Items { + if it["id"] == reqID || it["request_id"] == reqID { + t.Errorf("the app grant is listed as an MCP connection: %v", it) + } + } + if len(list.AppGrants) != 1 || list.AppGrants[0]["id"] != reqID || list.AppGrants[0]["access"] != "write" || + list.AppGrants[0]["app_name"] != "Portal" { + t.Fatalf("app_grants = %v", list.AppGrants) + } + + // Read-only: every connection mutation refuses the app grant. + for _, m := range []struct { + method, path string + body any + }{ + {"PATCH", "/api/v1/connected-apps/" + reqID + "/name", map[string]any{"name": "x"}}, + {"PATCH", "/api/v1/connected-apps/" + reqID + "/flags", map[string]any{"may_create_workspaces": true}}, + {"POST", "/api/v1/connected-apps/" + reqID + "/limit-to-current", nil}, + {"POST", "/api/v1/connected-apps/" + reqID + "/workspaces", map[string]any{"slug": "anything"}}, + {"DELETE", "/api/v1/connected-apps/" + reqID + "/workspaces/anything", nil}, + } { + if rr := doAuthedJSON(f.srv, m.method, m.path, m.body, f.sessionToken); rr.Code < 400 { + t.Errorf("%s %s on an app grant: %d, want a refusal", m.method, m.path, rr.Code) + } + } + if n := count3399(t, f.srv, `SELECT COUNT(*) FROM oauth_connections WHERE request_id = ?`, reqID); n != 0 { + t.Error("a mutation wrote a connection row for the app grant") + } + if _, err := f.srv.introspectAppToken(context.Background(), tok); err != nil { + t.Fatalf("control: the grant died before the revoke: %v", err) + } + + // Revoke: the token stops at introspection. + if rr := doAuthedJSON(f.srv, "DELETE", "/api/v1/connected-apps/"+reqID, nil, f.sessionToken); rr.Code != http.StatusNoContent { + t.Fatalf("revoke: %d %s", rr.Code, rr.Body.String()) + } + if _, err := f.srv.introspectAppToken(context.Background(), tok); err == nil { + t.Error("a revoked app grant's token still introspects") + } + // Through the binding (lead ruling Q1), not only the tokens. + if n := count3399(t, f.srv, `SELECT COUNT(*) FROM app_token_bindings WHERE request_id = ? AND revoked_at IS NOT NULL`, reqID); n != 1 { + t.Error("the console revoke left no tombstone on the grant's binding") + } + // Someone else cannot revoke it, and the answer is the plain 404. + g := delegatedFixture(t, "inst-console2", "write") + _, other := g.signIn(t, "read") + _, stranger := loginTestUserAs(t, g.srv, "stranger@example.com", "Stranger", "password123") + if rr := doAuthedJSON(g.srv, "DELETE", "/api/v1/connected-apps/"+other, nil, stranger); rr.Code != http.StatusNotFound { + t.Errorf("a stranger's revoke: %d, want 404", rr.Code) + } +} + +// Codex U5b-1 r1 P2: on a self-host with apps on and MCP off, a person can +// still sign in to an installed app, and still see and revoke the grant. +func TestTask3399_AppGrantsWorkWithMCPOff(t *testing.T) { + f := delegatedFixtureOn(t, "inst-nomcp", "write", false) + if f.srv.oauthAvailable() { + t.Fatal("control: MCP is on in this fixture") + } + _, reqID := f.signIn(t, "read") + rr := f.api("GET", "/api/v1/connected-apps") + if rr.Code != http.StatusOK || !strings.Contains(rr.Body.String(), reqID) { + t.Fatalf("list with MCP off: %d %s", rr.Code, rr.Body.String()) + } + if rr := f.api("DELETE", "/api/v1/connected-apps/"+reqID); rr.Code != http.StatusNoContent { + t.Errorf("revoke with MCP off: %d %s", rr.Code, rr.Body.String()) + } + // Any other client's authorize stays closed with MCP off. + q := url.Values{"client_id": {"some-mcp-client"}, "response_type": {"code"}, "redirect_uri": {"https://app.test/cb"}} + if rr := f.get("/oauth/authorize?" + q.Encode()); rr.Code != http.StatusNotFound { + t.Errorf("a non-app authorize with MCP off: %d, want 404", rr.Code) + } +} + +// Lead ruling (U5b-1): delegated sign-in stays closed, with a clear +// access_denied, until U5b-2 opens it together with the app API accepting +// delegated tokens. U5b-2 deletes the guard and this test. +func TestTask3399_DelegatedSignInIsClosedUntilU5b2(t *testing.T) { + f := delegatedFixture(t, "inst-closed", "write") + f.srv.delegatedSignInOpen = false + rr := f.decide(t, f.authorizeParams(), "approve", "read") + code, oerr := codeFrom(rr) + if code != "" || oerr != "access_denied" { + t.Fatalf("decide with sign-in closed: code %q, error %q, want access_denied", code, oerr) + } + cb, _ := url.Parse(rr.Header().Get("Location")) + if !strings.Contains(cb.Query().Get("error_description")+cb.Query().Get("error_hint"), "not yet available") { + t.Errorf("the refusal does not say sign-in is not yet available: %s", rr.Header().Get("Location")) + } + if rr := f.get("/oauth/authorize?" + f.authorizeParams().Encode()); rr.Code == http.StatusOK && strings.Contains(rr.Body.String(), "app_access") { + t.Error("the consent page rendered with sign-in closed") + } +} diff --git a/internal/store/account_claim.go b/internal/store/account_claim.go index a5be128e7..c44ec864a 100644 --- a/internal/store/account_claim.go +++ b/internal/store/account_claim.go @@ -60,7 +60,9 @@ type AccountClaim struct { // The caller then gives the claimant a way to set a password and kicks the // account's live connections. func (s *Store) ClaimAccountByVerification(token string) (*AccountClaim, error) { - return claimWithRetry(func() (*AccountClaim, error) { return s.claimAccountByVerificationOnce(token) }) + return claimWithRetry(func() (*AccountClaim, error) { + return claimRetryingDeadlocks(s, func() (*AccountClaim, error) { return s.claimAccountByVerificationOnce(token) }) + }) } func (s *Store) claimAccountByVerificationOnce(token string) (*AccountClaim, error) { @@ -131,7 +133,9 @@ func (s *Store) claimAccountByVerificationOnce(token string) (*AccountClaim, err // account's name and username; empty falls back to the address. func (s *Store) ClaimAccountByProvider(userID, provider, subject, name string) (*AccountClaim, error) { return claimWithRetry(func() (*AccountClaim, error) { - return s.claimAccountByProviderOnce(userID, provider, subject, name) + return claimRetryingDeadlocks(s, func() (*AccountClaim, error) { + return s.claimAccountByProviderOnce(userID, provider, subject, name) + }) }) } @@ -243,6 +247,19 @@ var claimAfterUsernameHook func(username string) // another account between choosing and writing it. The attempt's // transaction rolled back whole (a link claim's token included), so a // retry starts clean. +// claimRetryingDeadlocks runs one claim attempt under retryOnDeadlock: each +// attempt is one transaction with nothing outside it before its commit +// (TASK-3399). +func claimRetryingDeadlocks(s *Store, once func() (*AccountClaim, error)) (*AccountClaim, error) { + var claim *AccountClaim + err := s.retryOnDeadlock("account_claim", func() error { + var err error + claim, err = once() + return err + }) + return claim, err +} + func claimWithRetry(attempt func() (*AccountClaim, error)) (*AccountClaim, error) { var err error for i := 0; i < 3; i++ { @@ -381,10 +398,17 @@ func (s *Store) claimAccountTx(tx *sql.Tx, userID string, unusable []byte, ts, c {"delete tabs of owned workspaces", `DELETE FROM user_workspace_tabs WHERE workspace_id IN (SELECT id FROM workspaces WHERE owner_id = ? AND deleted_at IS NULL)`, []any{userID}}, {"soft-delete owned workspaces", `UPDATE workspaces SET deleted_at = ?, updated_at = ? WHERE owner_id = ? AND deleted_at IS NULL`, []any{ts, ts, userID}}, } - for _, st := range stmts { + for i, st := range stmts { if _, err := tx.Exec(s.q(st.query), st.args...); err != nil { return nil, fmt.Errorf("account claim: %s: %w", st.what, err) } + // After the users row (the first statement), before any token row: + // the claimed account's delegated app grants (TASK-3399). + if i == 0 { + if err := s.revokeDelegatedGrantsTx(tx, userID, ""); err != nil { + return nil, fmt.Errorf("account claim: %w", err) + } + } } if err := tx.QueryRow(s.q(`SELECT credential_epoch FROM users WHERE id = ?`), userID).Scan(&claim.Epoch); err != nil { return nil, fmt.Errorf("account claim: read epoch: %w", err) diff --git a/internal/store/app_grants.go b/internal/store/app_grants.go new file mode 100644 index 000000000..677dc7706 --- /dev/null +++ b/internal/store/app_grants.go @@ -0,0 +1,226 @@ +package store + +import ( + "database/sql" + "errors" + "fmt" + "time" +) + +// App grants (SPEC-6 U5b-1, TASK-3399): the delegated grants a person gave +// installed apps. They are not MCP connections, and have no oauth_connections +// row (lead ruling Q1): the install fixes the workspace and the binding the +// issuance barrier wrote carries the kind, epoch and access. The console +// lists them read-only and revokes them, through the binding. + +// AppGrant is one delegated grant, as the console shows it. +type AppGrant struct { + RequestID string + InstallID string + AppName string + Origin string + WorkspaceID string + WorkspaceName string + WorkspaceSlug string + Access string // "read" or "write", as the person consented + GrantedAt time.Time +} + +// ErrAppGrantNotFound is a grant that does not exist or is not the user's; +// the two are indistinguishable on purpose. +var ErrAppGrantNotFound = errors.New("app grant not found") + +// ListUserAppGrants returns the user's live delegated app grants: a binding +// of kind delegated whose grant still has an active access or refresh token +// held by the user. Newest first. +func (s *Store) ListUserAppGrants(userID string) ([]AppGrant, error) { + rows, err := s.db.Query(s.q(` + SELECT b.request_id, b.install_id, i.origin, COALESCE(u.name, ''), b.workspace_id, w.name, w.slug, + COALESCE(b.delegated_access, ''), b.created_at + FROM app_token_bindings b + JOIN app_installs i ON i.id = b.install_id + JOIN workspaces w ON w.id = b.workspace_id + LEFT JOIN users u ON u.id = i.bot_user_id + WHERE b.auth_kind = 'delegated' AND b.delegated_user_id = ? AND b.revoked_at IS NULL + -- An epoch the install has moved past (a disable, rotate or upgrade) + -- has already ended the grant; it is not listed (codex U5b-1 r6). + AND b.auth_epoch = i.auth_epoch + AND (EXISTS (SELECT 1 FROM oauth_access_tokens t WHERE t.request_id = b.request_id AND t.active = ?) + OR EXISTS (SELECT 1 FROM oauth_refresh_tokens t WHERE t.request_id = b.request_id AND t.active = ?) + -- A grant not yet exchanged is still one the person can revoke + -- (codex U5b-1 r7): its code can mint tokens. + OR EXISTS (SELECT 1 FROM oauth_authorization_codes t WHERE t.request_id = b.request_id AND t.active = ?)) + ORDER BY b.created_at DESC`), userID, true, true, true) + if err != nil { + return nil, fmt.Errorf("list app grants: %w", err) + } + defer rows.Close() + var out []AppGrant + for rows.Next() { + var g AppGrant + var granted string + if err := rows.Scan(&g.RequestID, &g.InstallID, &g.Origin, &g.AppName, &g.WorkspaceID, &g.WorkspaceName, &g.WorkspaceSlug, + &g.Access, &granted); err != nil { + return nil, fmt.Errorf("scan app grant: %w", err) + } + g.GrantedAt = parseTime(granted) + out = append(out, g) + } + if err := rows.Err(); err != nil { + return nil, err + } + for i := range out { + if out[i].AppName == "" { + if bot, err := s.AppPrincipalForInstall(out[i].InstallID); err == nil && bot != nil { + out[i].AppName = bot.Name + } + } + if out[i].AppName == "" { + out[i].AppName = out[i].Origin + } + } + return out, nil +} + +// IsUserAppGrant reports whether requestID is a delegated app grant the +// user holds (active or not). +func (s *Store) IsUserAppGrant(userID, requestID string) (bool, error) { + var n int + err := s.db.QueryRow(s.q(` + SELECT COUNT(*) FROM app_token_bindings b + WHERE b.request_id = ? AND b.auth_kind = 'delegated' AND b.delegated_user_id = ?`), + requestID, userID).Scan(&n) + if err != nil { + return false, fmt.Errorf("check app grant: %w", err) + } + return n > 0, nil +} + +// RevokeUserAppGrant ends a delegated app grant the user holds, in one +// transaction: its access and refresh tokens are deactivated, an unused +// code or PKCE row is deleted, and the binding is tombstoned (revoked_at), +// which introspection and the issuance barrier both refuse. +// Idempotent; a grant that is not the user's is ErrAppGrantNotFound. +func (s *Store) RevokeUserAppGrant(userID, requestID string) (string, error) { + // One transaction per attempt, nothing outside it before the commit: + // retryable on a deadlock (TASK-3399). The handler publishes nothing. + attempt := 0 + var installID string + err := s.retryOnDeadlock("revoke_app_grant", func() error { + var err error + installID, err = s.revokeUserAppGrantOnce(userID, requestID, &attempt) + return err + }) + return installID, err +} + +func (s *Store) revokeUserAppGrantOnce(userID, requestID string, attempt *int) (installID string, err error) { + ok, err := s.IsUserAppGrant(userID, requestID) + if err != nil { + return "", err + } + if !ok { + return "", ErrAppGrantNotFound + } + tx, err := s.db.Begin() + if err != nil { + return "", err + } + defer func() { _ = tx.Rollback() }() + if err := tx.QueryRow(s.q(`SELECT install_id FROM app_token_bindings WHERE request_id = ?`), requestID).Scan(&installID); err != nil && + !errors.Is(err, sql.ErrNoRows) { + return "", fmt.Errorf("read app grant: %w", err) + } + // The install row lock the issuance barrier takes, taken first (codex + // U5b-1 r1): a refresh in flight either commits before this revoke, whose + // updates below then see and deactivate its rows, or reads the tombstone + // this writes and refuses. On SQLite the single writer orders them. + if s.dialect.Driver() == DriverPostgres && installID != "" { + var one int + if err := tx.QueryRow(s.q(`SELECT 1 FROM app_installs WHERE id = ? FOR UPDATE`), installID).Scan(&one); err != nil && + !errors.Is(err, sql.ErrNoRows) { + return "", fmt.Errorf("lock install for app grant revoke: %w", err) + } + } + // Child rows in teardown's order (binding, PKCE, code, refresh, access), + // after the install lock teardown and issuance also take first. + // A tombstone, not a delete: a persistence already past its own checks + // would take a missing binding for a first issuance. + if _, err := tx.Exec(s.q(`UPDATE app_token_bindings SET revoked_at = ? WHERE request_id = ? AND revoked_at IS NULL`), now(), requestID); err != nil { + return "", fmt.Errorf("revoke app grant: %w", err) + } + for _, q := range []string{ + `DELETE FROM oauth_pkce_requests WHERE request_id = ?`, + `DELETE FROM oauth_authorization_codes WHERE request_id = ?`, + } { + if _, err := tx.Exec(s.q(q), requestID); err != nil { + return "", fmt.Errorf("revoke app grant: %w", err) + } + } + for _, q := range []string{ + `UPDATE oauth_refresh_tokens SET active = ? WHERE request_id = ?`, + `UPDATE oauth_access_tokens SET active = ? WHERE request_id = ?`, + } { + if _, err := tx.Exec(s.q(q), false, requestID); err != nil { + return "", fmt.Errorf("revoke app grant: %w", err) + } + } + if err := s.injectedDeadlock("revoke_app_grant", attempt); err != nil { + return "", err + } + return installID, tx.Commit() +} + +// revokeDelegatedGrantsTx ends the delegated app grants a person holds, in +// one workspace (workspaceID) or in every one (""), inside the caller's +// transaction: their bindings tombstoned, their unexchanged PKCE rows and +// codes deleted, and their tokens deactivated. Called by member removal, +// account disable and account claim, each of which already holds the +// person's users row. +// +// LOCKS (codex U5b-1 r2 and r3). It takes no install lock: the issuance +// barrier share-locks a delegated grant's person BEFORE anything else, so +// holding the person's row already orders every issuance of their grants +// against this. An install lock here deadlocked against owner-account +// deletion, which reaches installs through their bots' foreign key in an +// order of its own. Against install teardown, which holds the install and +// then writes these same rows, both take the child rows in ONE order: +// binding, PKCE, code, refresh, access (DeleteInstallClientTx's), so the two +// cannot wait on each other in a cycle. Both lock their bindings first, in +// request_id order (lockBindingsInOrderTx), because one table order is not +// one row order. +func (s *Store) revokeDelegatedGrantsTx(tx *sql.Tx, userID, workspaceID string) error { + scope := `SELECT request_id FROM app_token_bindings WHERE auth_kind = 'delegated' AND delegated_user_id = ?` + args := []any{userID} + if workspaceID != "" { + scope += ` AND workspace_id = ?` + args = append(args, workspaceID) + } + with := func(first any) []any { return append([]any{first}, args...) } + // Every binding of the person in scope, tombstones included, locked in + // request_id order before any child row, as install teardown does + // (codex U5b-1 r4). + where := `auth_kind = 'delegated' AND delegated_user_id = ?` + if workspaceID != "" { + where += ` AND workspace_id = ?` + } + if err := s.lockBindingsInOrderTx(tx, where, args...); err != nil { + return fmt.Errorf("revoke delegated grants: %w", err) + } + for _, st := range []struct { + q string + args []any + }{ + {`UPDATE app_token_bindings SET revoked_at = ? WHERE revoked_at IS NULL AND auth_kind = 'delegated' AND delegated_user_id = ?` + + map[bool]string{true: ` AND workspace_id = ?`, false: ``}[workspaceID != ""], with(now())}, + {`DELETE FROM oauth_pkce_requests WHERE request_id IN (` + scope + `)`, args}, + {`DELETE FROM oauth_authorization_codes WHERE request_id IN (` + scope + `)`, args}, + {`UPDATE oauth_refresh_tokens SET active = ? WHERE request_id IN (` + scope + `)`, with(false)}, + {`UPDATE oauth_access_tokens SET active = ? WHERE request_id IN (` + scope + `)`, with(false)}, + } { + if _, err := tx.Exec(s.q(st.q), st.args...); err != nil { + return fmt.Errorf("revoke delegated grants: %w", err) + } + } + return nil +} diff --git a/internal/store/connected_apps.go b/internal/store/connected_apps.go index 966c38fb9..f3c12074d 100644 --- a/internal/store/connected_apps.go +++ b/internal/store/connected_apps.go @@ -71,7 +71,7 @@ func (s *Store) ListUserOAuthConnections(userID string) ([]models.OAuthConnectio rows, err := s.db.Query(s.q(` SELECT request_id, client_id, requested_at, granted_scopes FROM oauth_access_tokens - WHERE subject = ? AND active = ? + WHERE subject = ? AND active = ?`+notInstallClientSQL+` ORDER BY request_id, requested_at DESC `), userID, true) if err != nil { @@ -126,7 +126,7 @@ func (s *Store) ListUserOAuthConnections(userID string) ([]models.OAuthConnectio rrows, err := s.db.Query(s.q(` SELECT request_id, client_id, requested_at, granted_scopes FROM oauth_refresh_tokens - WHERE subject = ? AND active = ? + WHERE subject = ? AND active = ?`+notInstallClientSQL+` ORDER BY request_id, requested_at DESC `), userID, true) if err != nil { diff --git a/internal/store/deadlock_retry.go b/internal/store/deadlock_retry.go new file mode 100644 index 000000000..d506f55a9 --- /dev/null +++ b/internal/store/deadlock_retry.go @@ -0,0 +1,71 @@ +package store + +import ( + "errors" + "log/slog" + "math/rand/v2" + "time" +) + +// Bounded retry on a Postgres deadlock (TASK-3399, lead ruling (a) on codex +// U5b-1 r5). A deadlock aborts ONE transaction cleanly: nothing it wrote +// commits, so running the whole transaction again from the start is safe. +// It is a liveness cost, not a correctness one. The lock orders already in +// place (users, then installs, then a grant's binding in request_id order, +// then its child rows) keep the common pairs from cycling at all; this +// covers the rare administrative overlaps they do not order (an account +// erase cascade, the expiry sweep, grants left with no binding). +// +// Only SQLSTATE 40P01 is retried (isDeadlockError). 40001 (serialization +// failure) would be too under SERIALIZABLE, but no caller runs SERIALIZABLE +// today, so it is deliberately not matched. SQLite's "database is locked" +// is a saturation signal, never retried here (see isLockTimeoutError). +// +// THE RULE for a wrapped function: it opens, does all its work in, and +// commits ONE transaction per call, and has no effect outside that +// transaction before the commit (no event publish, email, outbox drain or +// cache write). Each wrapped site says so where it calls this. + +// deadlockRetryAttempts is the most attempts, the first included. +const deadlockRetryAttempts = 3 + +// errInjectedDeadlock is the error the test injector returns: it matches +// isDeadlockError, as a real 40P01 does. +var errInjectedDeadlock = errors.New("injected for a test: deadlock detected (SQLSTATE 40P01)") + +// SetDeadlockRetryObserver records a function called once per retry with the +// site's name; the server counts them in pad_db_deadlock_retries_total. +func (s *Store) SetDeadlockRetryObserver(fn func(site string)) { s.deadlockRetryObserver = fn } + +// retryOnDeadlock runs fn, and runs it again from the start, up to +// deadlockRetryAttempts times in all, while it fails with a deadlock. Any +// other error, and the last attempt's error, is returned unchanged. +func (s *Store) retryOnDeadlock(site string, fn func() error) error { + var err error + for attempt := 1; attempt <= deadlockRetryAttempts; attempt++ { + err = fn() + if err == nil || !isDeadlockError(err) || attempt == deadlockRetryAttempts { + return err + } + slog.Info("store: retrying a transaction after a deadlock", "site", site, "attempt", attempt, "error", err) + if s.deadlockRetryObserver != nil { + s.deadlockRetryObserver(site) + } + // Small and jittered, so two victims of one cycle do not meet again + // in step: 5-15 ms, then 10-30 ms. + base := time.Duration(attempt) * 10 * time.Millisecond + time.Sleep(base/2 + time.Duration(rand.Int64N(int64(base)))) + } + return err +} + +// injectedDeadlock is called by each wrapped function just before its +// commit: a test can make an attempt fail there, after its writes, with an +// error that reads as a deadlock (s.deadlockInjector, nil in production). +func (s *Store) injectedDeadlock(site string, attempt *int) error { + *attempt++ + if s.deadlockInjector != nil && s.deadlockInjector(site, *attempt) { + return errInjectedDeadlock + } + return nil +} diff --git a/internal/store/install_clients.go b/internal/store/install_clients.go index e68806d1a..c3ad99182 100644 --- a/internal/store/install_clients.go +++ b/internal/store/install_clients.go @@ -41,9 +41,16 @@ var ( // ErrInstallTokenSubject refuses an install-client credential whose // subject is not that install's own bot. ErrInstallTokenSubject = errors.New("an install client's service token belongs to its own app principal only") - // ErrInstallDelegatedUnsupported refuses delegated (code + PKCE) install - // grants until TASK-3399 (U5b) brings them through the barrier. - ErrInstallDelegatedUnsupported = errors.New("delegated install grants are not supported yet") + // ErrInstallDelegatedUnsupported refuses a code or PKCE persistence for + // an install grant that is not a delegated one: only a person signing in + // through the app takes the code flow (TASK-3399). + ErrInstallDelegatedUnsupported = errors.New("only a delegated install grant takes the authorization-code flow") + // ErrInstallDelegatedSubject refuses a delegated install grant whose + // subject is not a live human member of the install's workspace. + ErrInstallDelegatedSubject = errors.New("a delegated install grant belongs to a member of the install's workspace") + // ErrInstallDelegatedAccess refuses a delegated grant whose consented + // access is missing, unknown, or more than the manifest offers. + ErrInstallDelegatedAccess = errors.New("a delegated install grant's access is not one the app offers") // ErrNoInstallClient means the install has no client. ErrNoInstallClient = errors.New("app install has no client") ) @@ -55,6 +62,73 @@ var ( // after the rotate committed, still carrying the epoch from before it. const InstallEpochSessionKey = "app_install_epoch" +// InstallAuthKindSessionKey and InstallAccessSessionKey carry a delegated +// grant's kind ("delegated") and the access the person consented to ("read" +// or "write") in its session data, set by the consent decision (TASK-3399). +// A grant without the kind is a service grant. +const ( + InstallAuthKindSessionKey = "app_auth_kind" + InstallAccessSessionKey = "app_delegated_access" + // InstallPersonEpochSessionKey carries the person's credential_epoch as + // the consent request resolved them (codex U5b-1 r7): the barrier refuses + // a grant whose person's credentials changed since, so a consent posted + // from a session a disable destroyed cannot mint a code after a re-enable. + InstallPersonEpochSessionKey = "app_person_epoch" + // InstallMemberSinceSessionKey carries the person's membership created_at + // as the consent gate read it (codex U5b-1 r8): a removal and re-add + // between the consent and its code refuses the code. + InstallMemberSinceSessionKey = "app_member_since" +) + +// carriedMemberSince reads the membership created_at a delegated grant +// carries, if any. +func carriedMemberSince(sessionData string) (string, bool) { + var doc struct { + Extra map[string]any `json:"extra"` + } + if err := json.Unmarshal([]byte(sessionData), &doc); err != nil { + return "", false + } + v, ok := doc.Extra[InstallMemberSinceSessionKey].(string) + return v, ok && v != "" +} + +// carriedPersonEpoch reads the person's credential epoch a delegated grant +// carries, if any. +func carriedPersonEpoch(sessionData string) (int64, bool) { + var doc struct { + Extra map[string]any `json:"extra"` + } + dec := json.NewDecoder(strings.NewReader(sessionData)) + dec.UseNumber() + if err := dec.Decode(&doc); err != nil { + return 0, false + } + n, ok := doc.Extra[InstallPersonEpochSessionKey].(json.Number) + if !ok { + return 0, false + } + v, err := n.Int64() + return v, err == nil +} + +// carriedInstallGrant reads a grant's kind and consented access from its +// session data; kind is "service" when none is carried. +func carriedInstallGrant(sessionData string) (kind, access string) { + var doc struct { + Extra map[string]any `json:"extra"` + } + if err := json.Unmarshal([]byte(sessionData), &doc); err != nil { + return "service", "" + } + kind, _ = doc.Extra[InstallAuthKindSessionKey].(string) + access, _ = doc.Extra[InstallAccessSessionKey].(string) + if kind == "" { + kind = "service" + } + return kind, access +} + // InstallEpoch returns the install's current epoch. func (s *Store) InstallEpoch(installID string) (int64, error) { var epoch int64 @@ -70,15 +144,18 @@ func (s *Store) InstallEpoch(installID string) (int64, error) { // carriedInstallEpoch reads the epoch a grant carries in its session data. func carriedInstallEpoch(sessionData string) (int64, bool) { + // The extra map holds other keys too (a delegated grant's kind and + // access are strings), so it is decoded generically, with numbers kept + // exact, and only the epoch key is required to be a number. var doc struct { - Extra map[string]json.Number `json:"extra"` + Extra map[string]any `json:"extra"` } dec := json.NewDecoder(strings.NewReader(sessionData)) dec.UseNumber() if err := dec.Decode(&doc); err != nil { return 0, false } - n, ok := doc.Extra[InstallEpochSessionKey] + n, ok := doc.Extra[InstallEpochSessionKey].(json.Number) if !ok { return 0, false } @@ -202,6 +279,14 @@ func (s *Store) RevokeInstallClientGrantsTx(tx *sql.Tx, installID string) error if err != nil { return err } + // The bindings first, locked in request_id order (TASK-3399, codex U5b-1 + // r4): a person's delegated revocation locks ITS bindings in the same + // order before any child row, so the two meet at the first shared + // binding instead of crossing inside a token table, where one table + // order does not mean one row order. + if err := s.lockBindingsInOrderTx(tx, `client_id = ?`, clientID); err != nil { + return fmt.Errorf("revoke install grants: %w", err) + } if _, err := tx.Exec(s.q(`DELETE FROM app_token_bindings WHERE client_id = ?`), clientID); err != nil { return fmt.Errorf("revoke install grants: bindings: %w", err) } @@ -271,13 +356,29 @@ func (s *Store) installIssuanceBarrierTx(tx *sql.Tx, table string, req models.OA if !installID.Valid || installID.String == "" { return false, nil } - lockQ := `SELECT workspace_id, state, auth_epoch FROM app_installs WHERE id = ?` + // A delegated grant's person is share-locked BEFORE the install (codex + // U5b-1 r2): users, then installs, then token rows is the order account + // erase (owner, then its installs' bots), disable and member removal take, + // so issuance cannot hold an install while waiting for a person another + // transaction holds with that install next. A service grant's subject is + // the install's bot, which the install lifecycle reaches through the + // install, so it is not locked here. + kind, access := carriedInstallGrant(req.SessionData) + if kind == "delegated" && s.dialect.Driver() == DriverPostgres { + var one int + if err := tx.QueryRow(s.q(`SELECT 1 FROM users WHERE id = ? FOR SHARE`), req.Subject).Scan(&one); err != nil && + !errors.Is(err, sql.ErrNoRows) { + return true, fmt.Errorf("oauth: lock delegated subject: %w", err) + } + } + lockQ := `SELECT workspace_id, state, auth_epoch, delegated_access FROM app_installs WHERE id = ?` if s.dialect.Driver() == DriverPostgres { lockQ += ` FOR UPDATE` } var workspaceID, state string var epoch int64 - err = tx.QueryRow(s.q(lockQ), installID.String).Scan(&workspaceID, &state, &epoch) + var offered sql.NullString + err = tx.QueryRow(s.q(lockQ), installID.String).Scan(&workspaceID, &state, &epoch, &offered) if errors.Is(err, sql.ErrNoRows) { return true, ErrInstallNotActive } @@ -298,44 +399,153 @@ func (s *Store) installIssuanceBarrierTx(tx *sql.Tx, table string, req models.OA if disabledAt.Valid && disabledAt.String != "" { return true, ErrInstallClientDisabled } - if table != "oauth_access_tokens" && table != "oauth_refresh_tokens" { + // The grant's kind (read above), and for a delegated grant the access + // the person consented to, as the consent decision put them in the + // session data. + var bindAccess, bindUser, bindSince sql.NullString + var bindCredEpoch sql.NullInt64 + switch kind { + case "service": + // A service token is client_credentials: no code, no PKCE. + if table != "oauth_access_tokens" && table != "oauth_refresh_tokens" { + return true, ErrInstallDelegatedUnsupported + } + var botKind string + var botDisabled sql.NullString + // The subject must be the install's bot: bot_user_id when provisioning + // set it (U8b), with the address as the agreement check either way. + err = tx.QueryRow(s.q(`SELECT u.kind, u.disabled_at FROM users u JOIN app_installs i ON i.id = ? + WHERE u.id = ? AND u.email = ? AND (i.bot_user_id IS NULL OR i.bot_user_id = u.id)`), + installID.String, req.Subject, appPrincipalEmail(installID.String)).Scan(&botKind, &botDisabled) + if errors.Is(err, sql.ErrNoRows) || (err == nil && (botKind != models.UserKindApp || botDisabled.Valid)) { + return true, ErrInstallTokenSubject + } + if err != nil { + return true, fmt.Errorf("oauth: read install principal: %w", err) + } + case "delegated": + // TASK-3399: a person signing in through the app. The access they + // consented to must be one the manifest offers, read under the lock: + // an upgrade that withdrew delegated access refuses a grant consented + // before it. + if !delegatedAccessWithin(access, offered) { + return true, ErrInstallDelegatedAccess + } + // The subject must be a live HUMAN member of the install's workspace, + // read in this transaction: never a bot (which has its own door), and + // never someone removed since they consented. On Postgres the user and + // membership rows are share-locked (codex U5b-1 r1), so an account + // disable or a member removal waits for this issuance to commit and + // then revokes what it wrote, or commits first and is seen here. + // SQLite's single writer serializes them already. + // The workspace must be live too (codex U5b-1 r9): a consent approved + // after a soft delete persists nothing. + subjQ := `SELECT u.kind, u.disabled_at, u.credential_epoch, m.created_at FROM users u + JOIN workspace_members m ON m.user_id = u.id AND m.workspace_id = ? + JOIN workspaces w ON w.id = m.workspace_id AND w.deleted_at IS NULL + WHERE u.id = ?` + if s.dialect.Driver() == DriverPostgres { + subjQ += ` FOR SHARE` + } + var userKind, memberSince string + var userDisabled sql.NullString + var credEpoch int64 + err = tx.QueryRow(s.q(subjQ), workspaceID, req.Subject).Scan(&userKind, &userDisabled, &credEpoch, &memberSince) + if errors.Is(err, sql.ErrNoRows) || (err == nil && (userKind != models.UserKindHuman || userDisabled.Valid)) { + return true, ErrInstallDelegatedSubject + } + if err != nil { + return true, fmt.Errorf("oauth: read delegated subject: %w", err) + } + // The credentials the consent was given under must still be the + // person's: a disable or claim since then bumped them. + if carried, ok := carriedPersonEpoch(req.SessionData); !ok || carried != credEpoch { + return true, ErrInstallDelegatedSubject + } + // And the membership it was given under (a removal and re-add since + // the consent gate read it refuses). RESIDUAL, accepted by the lead + // (codex U5b-1 r9): workspace_members has no row id (its key is + // workspace_id, user_id, which a re-add reuses) and created_at has + // one-second resolution, so an owner's removal AND re-add inside one + // second, during an in-flight consent, still matches. It grants + // nothing beyond the person's current membership: they are a member at + // the consent and at this persistence, and any grant that already + // existed was revoked by the removal itself (revokeDelegatedGrantsTx). + // No generation column is added for this. + if carried, ok := carriedMemberSince(req.SessionData); !ok || carried != memberSince { + return true, ErrInstallDelegatedSubject + } + bindAccess = sql.NullString{String: access, Valid: true} + bindUser = sql.NullString{String: req.Subject, Valid: true} + bindSince = sql.NullString{String: memberSince, Valid: true} + bindCredEpoch = sql.NullInt64{Int64: credEpoch, Valid: true} + default: return true, ErrInstallDelegatedUnsupported } - var botKind string - var botDisabled sql.NullString - // The subject must be the install's bot: bot_user_id when provisioning - // set it (U8b), with the address as the agreement check either way. - err = tx.QueryRow(s.q(`SELECT u.kind, u.disabled_at FROM users u JOIN app_installs i ON i.id = ? - WHERE u.id = ? AND u.email = ? AND (i.bot_user_id IS NULL OR i.bot_user_id = u.id)`), - installID.String, req.Subject, appPrincipalEmail(installID.String)).Scan(&botKind, &botDisabled) - if errors.Is(err, sql.ErrNoRows) || (err == nil && (botKind != models.UserKindApp || botDisabled.Valid)) { - return true, ErrInstallTokenSubject - } - if err != nil { - return true, fmt.Errorf("oauth: read install principal: %w", err) - } if err := s.insertOAuthRequestRowTx(tx, table, req, requestedStr); err != nil { return true, err } + // One binding per grant (fosite keeps the request id from code to token), + // written by its first persistence and checked by every later one: the + // same epoch, kind and access, so no later step can change what the grant + // is. var bound int64 - err = tx.QueryRow(s.q(`SELECT auth_epoch FROM app_token_bindings WHERE request_id = ?`), req.RequestID).Scan(&bound) + var boundKind string + var boundAccess, boundUser, boundSince, boundRevoked sql.NullString + var boundCredEpoch sql.NullInt64 + err = tx.QueryRow(s.q(`SELECT auth_epoch, auth_kind, delegated_access, delegated_user_id, delegated_member_since, + delegated_credential_epoch, revoked_at FROM app_token_bindings WHERE request_id = ?`), + req.RequestID).Scan(&bound, &boundKind, &boundAccess, &boundUser, &boundSince, &boundCredEpoch, &boundRevoked) switch { + case errors.Is(err, sql.ErrNoRows) && kind == "delegated" && table != "oauth_authorization_codes" && table != "oauth_pkce_requests": + // Only a delegated grant's FIRST persistence (its code, at consent) + // writes its binding. A token persisting against no binding is a + // grant whose binding is gone: revoked and swept (codex U5b-1 r2), or + // erased. Never a first issuance. + return true, ErrInstallNotActive case errors.Is(err, sql.ErrNoRows): if _, err := tx.Exec(s.q(` - INSERT INTO app_token_bindings (request_id, client_id, install_id, workspace_id, auth_epoch, auth_kind, created_at) - VALUES (?, ?, ?, ?, ?, 'service', ?)`), - req.RequestID, req.ClientID, installID.String, workspaceID, epoch, now()); err != nil { + INSERT INTO app_token_bindings (request_id, client_id, install_id, workspace_id, auth_epoch, auth_kind, delegated_access, + delegated_user_id, delegated_member_since, delegated_credential_epoch, created_at) + VALUES (?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?)`), + req.RequestID, req.ClientID, installID.String, workspaceID, epoch, kind, bindAccess, bindUser, bindSince, bindCredEpoch, now()); err != nil { return true, fmt.Errorf("oauth: write token binding: %w", err) } case err != nil: return true, fmt.Errorf("oauth: read token binding: %w", err) + case boundRevoked.Valid: + // The person revoked this grant: a tombstone, never a first issuance. + return true, ErrInstallNotActive case bound != epoch: // A later persistence of a family bound under an earlier epoch. return true, ErrInstallNotActive + case boundKind != kind || boundAccess != bindAccess: + return true, ErrInstallDelegatedAccess + case boundUser != bindUser || boundSince != bindSince || boundCredEpoch != bindCredEpoch: + // Not the person, membership or credentials it was issued against: + // a disable (credential_epoch) or a removal and re-add (member since) + // between two persistences ends the grant. + return true, ErrInstallDelegatedSubject } return true, nil } +// delegatedAccessWithin reports whether a consented access is one the +// manifest offers: "read" or "write", and no more than offered. An install +// whose manifest offers no delegated access refuses every delegated grant. +func delegatedAccessWithin(access string, offered sql.NullString) bool { + if !offered.Valid { + return false + } + switch access { + case "read": + return offered.String == "read" || offered.String == "write" + case "write": + return offered.String == "write" + } + return false +} + // AppPrincipalForInstall returns the install's bot: the kind='app' user at // the address CreateAppUserTx gave it, or nil when there is none. func (s *Store) AppPrincipalForInstall(installID string) (*models.User, error) { @@ -393,7 +603,7 @@ func (s *Store) GetAppTokenState(requestID string) (*AppTokenState, error) { FROM app_token_bindings b JOIN app_installs i ON i.id = b.install_id JOIN oauth_clients c ON c.id = b.client_id - WHERE b.request_id = ?`), requestID).Scan( + WHERE b.request_id = ? AND b.revoked_at IS NULL`), requestID).Scan( &st.Binding.RequestID, &st.Binding.ClientID, &st.Binding.InstallID, &st.Binding.WorkspaceID, &st.Binding.AuthEpoch, &st.Binding.AuthKind, &st.InstallState, &st.InstallEpoch, &disabledAt) if errors.Is(err, sql.ErrNoRows) { @@ -405,3 +615,96 @@ func (s *Store) GetAppTokenState(requestID string) (*AppTokenState, error) { st.ClientDisabled = disabledAt.Valid && disabledAt.String != "" return &st, nil } + +// InstallConsentState is what the delegated consent page and decision need +// about an install (TASK-3399): where it lives, whether it is active, what +// delegated access its manifest offers, and how to name it. +type InstallConsentState struct { + InstallID string + WorkspaceID string + State string + Origin string + DelegatedAccess string // "read", "write", or "" (none offered) + AppName string // the install's bot display name, which is the app's name + // AuthEpoch is read with the state, before any check the gate makes on + // it, and carried into the grant (codex U5b-1 r8): a disable and + // re-enable after the gate refuses the code. + AuthEpoch int64 +} + +// GetInstallConsentState reads an install's consent state, or nil when there +// is no such install. +func (s *Store) GetInstallConsentState(installID string) (*InstallConsentState, error) { + st := InstallConsentState{InstallID: installID} + var access, name sql.NullString + err := s.db.QueryRow(s.q(`SELECT i.workspace_id, i.state, i.origin, i.delegated_access, u.name, i.auth_epoch + FROM app_installs i LEFT JOIN users u ON u.id = i.bot_user_id WHERE i.id = ?`), installID). + Scan(&st.WorkspaceID, &st.State, &st.Origin, &access, &name, &st.AuthEpoch) + if errors.Is(err, sql.ErrNoRows) { + return nil, nil + } + if err != nil { + return nil, fmt.Errorf("read install consent state: %w", err) + } + st.DelegatedAccess, st.AppName = access.String, name.String + if st.AppName == "" { + // An install from before bot_user_id (U8b): its bot by address. + if bot, err := s.AppPrincipalForInstall(installID); err == nil && bot != nil { + st.AppName = bot.Name + } + } + if st.AppName == "" { + st.AppName = st.Origin + } + return &st, nil +} + +// notInstallClientSQL keeps a token row's chain to ordinary OAuth clients: +// an installed app's chains (its service tokens and the delegated grants +// people give it, TASK-3399) are not MCP connections. Every reader that +// treats a token chain as a connection (the Connected Apps list, the MCP +// readiness resume count, the connections backfill) filters with it; app +// grants are listed and revoked on their own (ListUserAppGrants, +// RevokeUserAppGrant). Appended to a WHERE clause over a token table whose +// client column is client_id. +const notInstallClientSQL = ` AND client_id NOT IN (SELECT id FROM oauth_clients WHERE app_install_id IS NOT NULL AND app_install_id <> '')` + +// lockBindingsInOrderTx locks the app_token_bindings rows a predicate +// selects, tombstones included, in request_id order, on Postgres (SQLite's +// single writer serializes the callers already). Every transaction that +// writes several grants' child rows (install teardown, a person's delegated +// revocation) takes its bindings this way first, so two of them can only +// wait on each other at a binding, in one order, never in a cycle. +func (s *Store) lockBindingsInOrderTx(tx *sql.Tx, where string, args ...any) error { + if s.dialect.Driver() != DriverPostgres { + return nil + } + rows, err := tx.Query(s.q(`SELECT request_id FROM app_token_bindings WHERE `+where+` ORDER BY request_id FOR UPDATE`), args...) + if err != nil { + return fmt.Errorf("lock token bindings: %w", err) + } + for rows.Next() { + } + if err := rows.Err(); err != nil { + _ = rows.Close() + return fmt.Errorf("lock token bindings: %w", err) + } + return rows.Close() +} + +// WorkspaceMemberSince returns a membership's created_at exactly as stored, +// or "" when there is none or its workspace is deleted: the value a delegated consent carries and the +// issuance barrier compares (TASK-3399). +func (s *Store) WorkspaceMemberSince(workspaceID, userID string) (string, error) { + var since string + err := s.db.QueryRow(s.q(`SELECT m.created_at FROM workspace_members m + JOIN workspaces w ON w.id = m.workspace_id AND w.deleted_at IS NULL + WHERE m.workspace_id = ? AND m.user_id = ?`), workspaceID, userID).Scan(&since) + if errors.Is(err, sql.ErrNoRows) { + return "", nil + } + if err != nil { + return "", fmt.Errorf("read membership: %w", err) + } + return since, nil +} diff --git a/internal/store/mcp_resume.go b/internal/store/mcp_resume.go index 30657df58..5c56b5dca 100644 --- a/internal/store/mcp_resume.go +++ b/internal/store/mcp_resume.go @@ -23,9 +23,9 @@ func (s *Store) CountLiveOAuthConnections() (int, error) { var n int err := s.db.QueryRow(s.q(` SELECT COUNT(*) FROM ( - SELECT request_id FROM oauth_access_tokens WHERE active = ? + SELECT request_id FROM oauth_access_tokens WHERE active = ?`+notInstallClientSQL+` UNION - SELECT request_id FROM oauth_refresh_tokens WHERE active = ? + SELECT request_id FROM oauth_refresh_tokens WHERE active = ?`+notInstallClientSQL+` ) chains`), true, true).Scan(&n) if err != nil { return 0, fmt.Errorf("count live oauth connections: %w", err) diff --git a/internal/store/migrations/121_app_token_binding_access.sql b/internal/store/migrations/121_app_token_binding_access.sql new file mode 100644 index 000000000..a60487af1 --- /dev/null +++ b/internal/store/migrations/121_app_token_binding_access.sql @@ -0,0 +1,36 @@ +-- Migration 121: the consented access of a delegated app grant (SPEC-6 U5b, +-- TASK-3399). +-- +-- A delegated token's access is the lesser of the manifest's +-- delegated.access and what the person chose on the consent page (DOC-3371 +-- §4 step 4); the consent page offers read-only even when the manifest asks +-- for write, defaulting to it (lead ruling Q2). The choice is recorded on the +-- binding the issuance barrier writes, under the install-row lock, so every +-- token of the grant carries the same value. NULL on a service binding, +-- whose access is the install's service_access. +-- +-- delegated_user_id is the person a delegated grant acts for, written by the +-- barrier at the grant's FIRST persistence, which is its authorization code: +-- the code table has no subject column, so without it a person's unexchanged +-- delegated code could be found by nothing but its session data. Disabling +-- or claiming the account deactivates those codes through it, and erasing +-- the account cascades the binding away (introspection then refuses every +-- token of the grant as unbound). NULL on a service binding. +ALTER TABLE app_token_bindings ADD COLUMN delegated_access TEXT; +ALTER TABLE app_token_bindings ADD COLUMN delegated_user_id TEXT REFERENCES users(id) ON DELETE CASCADE; +-- +-- The barrier also remembers, for a delegated grant, what it was issued +-- against, so a change between two of its persistences is never forgotten +-- (codex U5b-1 r1): +-- - delegated_credential_epoch: the person's users.credential_epoch, which a +-- disable or an account claim bumps, so a disable and re-enable between +-- two persistences refuses the next one; +-- - delegated_member_since: the person's workspace_members.created_at, so a +-- removal and re-add between two persistences refuses the next one; +-- - revoked_at: the person's console revoke, a tombstone the barrier refuses +-- instead of a deleted binding it would take for a first issuance and +-- recreate. The sweep removes it with the grant's last row. +ALTER TABLE app_token_bindings ADD COLUMN delegated_credential_epoch INTEGER; +ALTER TABLE app_token_bindings ADD COLUMN delegated_member_since TEXT; +ALTER TABLE app_token_bindings ADD COLUMN revoked_at TEXT; +CREATE INDEX IF NOT EXISTS idx_app_token_bindings_delegated_user ON app_token_bindings (delegated_user_id); diff --git a/internal/store/nul_unprotected_baseline.txt b/internal/store/nul_unprotected_baseline.txt index a6fe5eafb..8b9f454a1 100644 --- a/internal/store/nul_unprotected_baseline.txt +++ b/internal/store/nul_unprotected_baseline.txt @@ -54,8 +54,12 @@ app_installs.workspace_id app_token_bindings.auth_kind app_token_bindings.client_id app_token_bindings.created_at +app_token_bindings.delegated_access +app_token_bindings.delegated_member_since +app_token_bindings.delegated_user_id app_token_bindings.install_id app_token_bindings.request_id +app_token_bindings.revoked_at app_token_bindings.workspace_id attachments.content_hash attachments.created_at diff --git a/internal/store/oauth_connections_backfill.go b/internal/store/oauth_connections_backfill.go index 6feef9f63..95934d1b1 100644 --- a/internal/store/oauth_connections_backfill.go +++ b/internal/store/oauth_connections_backfill.go @@ -188,13 +188,13 @@ func (s *Store) collectGrantChainsForBackfill() (map[string]backfillChain, error if err := scan(` SELECT request_id, subject, requested_at, session_data - FROM oauth_access_tokens + FROM oauth_access_tokens WHERE 1 = 1` + notInstallClientSQL + ` `); err != nil { return nil, fmt.Errorf("scan access tokens: %w", err) } if err := scan(` SELECT request_id, subject, requested_at, session_data - FROM oauth_refresh_tokens + FROM oauth_refresh_tokens WHERE 1 = 1` + notInstallClientSQL + ` `); err != nil { return nil, fmt.Errorf("scan refresh tokens: %w", err) } diff --git a/internal/store/oauth_sweep.go b/internal/store/oauth_sweep.go index 657e56bf8..532f9ceaa 100644 --- a/internal/store/oauth_sweep.go +++ b/internal/store/oauth_sweep.go @@ -1,6 +1,7 @@ package store import ( + "database/sql" "fmt" "time" ) @@ -31,12 +32,17 @@ type OAuthSweepResult struct { RefreshTokens int64 AuthorizationCodes int64 PKCERequests int64 - Capped bool + // AppTokenBindings is install-token bindings whose grant has no row left + // in any of the four tables (TASK-3399): the binding table has no + // foreign key to them, so without this an expired chain's binding + // stayed forever, for service and delegated grants alike. + AppTokenBindings int64 + Capped bool } // Total is the rows deleted across all four tables. func (r OAuthSweepResult) Total() int64 { - return r.AccessTokens + r.RefreshTokens + r.AuthorizationCodes + r.PKCERequests + return r.AccessTokens + r.RefreshTokens + r.AuthorizationCodes + r.PKCERequests + r.AppTokenBindings } // SweepExpiredOAuthRows deletes rows whose requested_at is before the @@ -81,7 +87,14 @@ func (s *Store) SweepExpiredOAuthRows(cut OAuthSweepCutoffs, batchSize, maxBatch )`) batches := 0 for { - r, err := s.db.Exec(stmt, before, batchSize) + // Each batch is one statement in its own transaction: retryable + // on a deadlock (TASK-3399). + var r sql.Result + err := s.retryOnDeadlock("oauth_sweep", func() error { + var err error + r, err = s.db.Exec(stmt, before, batchSize) + return err + }) if err != nil { return res, fmt.Errorf("sweep %s: %w", t.table, err) } @@ -104,5 +117,40 @@ func (s *Store) SweepExpiredOAuthRows(cut OAuthSweepCutoffs, batchSize, maxBatch time.Sleep(pause) } } + // Bindings orphaned by the deletes above, or by any earlier one. A + // binding is written in the same transaction as its grant's first row, + // so a committed binding without a row is never one mid-issuance. + orphans := s.q(`DELETE FROM app_token_bindings WHERE request_id IN ( + SELECT b.request_id FROM app_token_bindings b + WHERE NOT EXISTS (SELECT 1 FROM oauth_access_tokens t WHERE t.request_id = b.request_id) + AND NOT EXISTS (SELECT 1 FROM oauth_refresh_tokens t WHERE t.request_id = b.request_id) + AND NOT EXISTS (SELECT 1 FROM oauth_authorization_codes t WHERE t.request_id = b.request_id) + AND NOT EXISTS (SELECT 1 FROM oauth_pkce_requests t WHERE t.request_id = b.request_id) + LIMIT ? + )`) + for batches := 0; ; batches++ { + if batches >= maxBatches { + res.Capped = true + break + } + var r sql.Result + err := s.retryOnDeadlock("oauth_sweep", func() error { + var err error + r, err = s.db.Exec(orphans, batchSize) + return err + }) + if err != nil { + return res, fmt.Errorf("sweep app_token_bindings: %w", err) + } + n, err := r.RowsAffected() + if err != nil { + return res, fmt.Errorf("sweep app_token_bindings: rows affected: %w", err) + } + res.AppTokenBindings += n + if n < int64(batchSize) { + break + } + time.Sleep(pause) + } return res, nil } diff --git a/internal/store/pgmigrations/095_app_token_binding_access.sql b/internal/store/pgmigrations/095_app_token_binding_access.sql new file mode 100644 index 000000000..85ed0823a --- /dev/null +++ b/internal/store/pgmigrations/095_app_token_binding_access.sql @@ -0,0 +1,8 @@ +-- Migration 095: the consented access of a delegated app grant (SPEC-6 U5b, +-- TASK-3399). Mirrors SQLite migration 121; see it for the rationale. +ALTER TABLE app_token_bindings ADD COLUMN IF NOT EXISTS delegated_access TEXT; +ALTER TABLE app_token_bindings ADD COLUMN IF NOT EXISTS delegated_user_id TEXT REFERENCES users(id) ON DELETE CASCADE; +CREATE INDEX IF NOT EXISTS idx_app_token_bindings_delegated_user ON app_token_bindings (delegated_user_id); +ALTER TABLE app_token_bindings ADD COLUMN IF NOT EXISTS delegated_credential_epoch BIGINT; +ALTER TABLE app_token_bindings ADD COLUMN IF NOT EXISTS delegated_member_since TEXT; +ALTER TABLE app_token_bindings ADD COLUMN IF NOT EXISTS revoked_at TEXT; diff --git a/internal/store/store.go b/internal/store/store.go index 8b6d2599f..9dafbc745 100644 --- a/internal/store/store.go +++ b/internal/store/store.go @@ -41,6 +41,11 @@ var AllowSchemaAhead bool type Store struct { db *sql.DB dialect Dialect + // deadlockRetryObserver is told each deadlock retry's site + // (retryOnDeadlock); deadlockInjector is a TEST-ONLY hook, nil in + // production, that fails an attempt just before its commit. + deadlockRetryObserver func(site string) + deadlockInjector func(site string, attempt int) bool // usernameValidator, when set, is the rule a generated username must // pass (the server's ValidateUsername: format, length, reserved names). // An account claim regenerates one (TASK-3351). diff --git a/internal/store/task3394_install_clients_test.go b/internal/store/task3394_install_clients_test.go index 24e20692f..1c396d2a3 100644 --- a/internal/store/task3394_install_clients_test.go +++ b/internal/store/task3394_install_clients_test.go @@ -224,7 +224,9 @@ func TestTask3394_BarrierRefusesAnInactiveInstallOrDisabledClient(t *testing.T) // U5a has no delegated grants: an install client's code or PKCE row is // refused until TASK-3399. -func TestTask3394_DelegatedInstallGrantsWaitForU5b(t *testing.T) { +// A grant that carries no delegated kind is a service grant, and a service +// grant has no code flow (TASK-3399 kept this refusal for it). +func TestTask3394_AServiceGrantTakesNoCodeFlow(t *testing.T) { f := task3394Fixture(t, "inst-deleg") human := createTestUser(t, f.s, "deleg@test.com", "Deleg", "password123") if err := f.s.CreateAuthorizationCode(task3394Req(f.clientID, human.ID, "req-code")); !errors.Is(err, ErrInstallDelegatedUnsupported) { diff --git a/internal/store/task3399_deadlock_retry_test.go b/internal/store/task3399_deadlock_retry_test.go new file mode 100644 index 000000000..73486eb9f --- /dev/null +++ b/internal/store/task3399_deadlock_retry_test.go @@ -0,0 +1,107 @@ +package store + +import ( + "testing" +) + +// TASK-3399 (lead ruling (a) on codex U5b-1 r5): a deadlocked transaction is +// run again from the start, at most three times. + +func retryRecorder(s *Store) *[]string { + var sites []string + s.SetDeadlockRetryObserver(func(site string) { sites = append(sites, site) }) + return &sites +} + +// Attempt 1 does all its writes, then fails as a deadlock just before its +// commit; attempt 2 commits the whole effect, once. +func TestTask3399_ADeadlockedAttemptIsRunAgainFromTheStart(t *testing.T) { + cases := map[string]func(t *testing.T, f task3399Fix) (run func() error, check func(t *testing.T)){ + "remove_workspace_member": func(t *testing.T, f task3399Fix) (func() error, func(t *testing.T)) { + return func() error { return f.s.RemoveWorkspaceMember(f.ws.ID, f.person.ID) }, func(t *testing.T) { + if n := task3394Count(t, f.s, `SELECT COUNT(*) FROM workspace_members WHERE user_id = ? AND workspace_id = ?`, f.person.ID, f.ws.ID); n != 0 { + t.Error("the membership survived") + } + if n := task3394Count(t, f.s, `SELECT COUNT(*) FROM app_token_bindings WHERE request_id = 'req-dl' AND revoked_at IS NOT NULL`); n != 1 { + t.Error("the grant was not revoked") + } + } + }, + "disable_user": func(t *testing.T, f task3399Fix) (func() error, func(t *testing.T)) { + before := task3394Count(t, f.s, `SELECT credential_epoch FROM users WHERE id = ?`, f.person.ID) + return func() error { return f.s.DisableUserAndRevokeAccess(f.person.ID) }, func(t *testing.T) { + if n := task3394Count(t, f.s, `SELECT COUNT(*) FROM users WHERE id = ? AND disabled_at IS NOT NULL`, f.person.ID); n != 1 { + t.Error("the person is not disabled") + } + if n := task3394Count(t, f.s, `SELECT COUNT(*) FROM oauth_refresh_tokens WHERE request_id = 'req-dl' AND active = ?`, true); n != 0 { + t.Error("the grant's refresh token is still active") + } + if n := task3394Count(t, f.s, `SELECT credential_epoch FROM users WHERE id = ?`, f.person.ID); n != before+1 { + t.Errorf("credential_epoch = %d, want %d: one committed bump, not one per attempt", n, before+1) + } + } + }, + "revoke_app_grant": func(t *testing.T, f task3399Fix) (func() error, func(t *testing.T)) { + return func() error { _, err := f.s.RevokeUserAppGrant(f.person.ID, "req-dl"); return err }, func(t *testing.T) { + if n := task3394Count(t, f.s, `SELECT COUNT(*) FROM app_token_bindings WHERE request_id = 'req-dl' AND revoked_at IS NOT NULL`); n != 1 { + t.Error("the grant was not revoked") + } + } + }, + } + for site, build := range cases { + t.Run(site, func(t *testing.T) { + f := task3399Fixture(t, "inst-dl-"+site, "write") + task3399Delegated(t, f, "req-dl") + sites := retryRecorder(f.s) + attempts := 0 + f.s.deadlockInjector = func(got string, attempt int) bool { + if got != site { + return false + } + attempts = attempt + return attempt == 1 + } + run, check := build(t, f) + if err := run(); err != nil { + t.Fatalf("%s after one injected deadlock: %v", site, err) + } + if attempts != 2 { + t.Errorf("%s ran %d attempts, want 2", site, attempts) + } + if len(*sites) != 1 || (*sites)[0] != site { + t.Errorf("retry observer saw %v, want [%s]", *sites, site) + } + check(t) + }) + } +} + +// Three deadlocks in a row: the last error is returned unchanged, and +// nothing any attempt wrote is committed. +func TestTask3399_DeadlockRetriesAreBounded(t *testing.T) { + f := task3399Fixture(t, "inst-dl-bound", "write") + task3399Delegated(t, f, "req-dlb") + sites := retryRecorder(f.s) + calls := 0 + f.s.deadlockInjector = func(string, int) bool { calls++; return true } + err := f.s.RemoveWorkspaceMember(f.ws.ID, f.person.ID) + if err != errInjectedDeadlock { + t.Fatalf("err = %v, want the last attempt's deadlock error unchanged", err) + } + if calls != deadlockRetryAttempts || len(*sites) != deadlockRetryAttempts-1 { + t.Errorf("%d attempts and %d retries, want %d and %d", calls, len(*sites), deadlockRetryAttempts, deadlockRetryAttempts-1) + } + if n := task3394Count(t, f.s, `SELECT COUNT(*) FROM workspace_members WHERE user_id = ? AND workspace_id = ?`, f.person.ID, f.ws.ID); n != 1 { + t.Error("a failed attempt committed the removal") + } + if n := task3394Count(t, f.s, `SELECT COUNT(*) FROM app_token_bindings WHERE request_id = 'req-dlb' AND revoked_at IS NOT NULL`); n != 0 { + t.Error("a failed attempt committed the revoke") + } + // Any other error is returned at once, never retried. + f.s.deadlockInjector = nil + *sites = nil + if err := f.s.RemoveWorkspaceMember(f.ws.ID, "no-such-user"); err == nil || len(*sites) != 0 { + t.Errorf("a non-deadlock error: err %v, %d retries; want an error and none", err, len(*sites)) + } +} diff --git a/internal/store/task3399_delegated_barrier_test.go b/internal/store/task3399_delegated_barrier_test.go new file mode 100644 index 000000000..016c24d21 --- /dev/null +++ b/internal/store/task3399_delegated_barrier_test.go @@ -0,0 +1,586 @@ +package store + +import ( + "errors" + "strings" + "testing" + "time" + + "github.com/PerpetualSoftware/pad/internal/models" +) + +// TASK-3399 (SPEC-6 U5b-1): delegated install grants through the barrier. + +type task3399Fix struct { + task3394Fix + person *models.User +} + +// task3399Fixture is an install whose manifest offers delegated access +// (offered), with a human member who can sign in through it. +func task3399Fixture(t *testing.T, installID, offered string) task3399Fix { + t.Helper() + f := task3394Fixture(t, installID) + if offered != "" { + if _, err := f.s.db.Exec(f.s.q(`UPDATE app_installs SET delegated_access = ? WHERE id = ?`), offered, installID); err != nil { + t.Fatal(err) + } + } + person := createTestUser(t, f.s, "person-"+installID+"@test.com", "Person", "password123") + if err := f.s.AddWorkspaceMember(f.ws.ID, person.ID, "editor"); err != nil { + t.Fatal(err) + } + // A fixed membership created_at, which task3399Req carries as the + // consent's (InstallMemberSinceSessionKey). + if _, err := f.s.db.Exec(f.s.q(`UPDATE workspace_members SET created_at = ? WHERE workspace_id = ? AND user_id = ?`), + task3399MemberSince, f.ws.ID, person.ID); err != nil { + t.Fatal(err) + } + return task3399Fix{task3394Fix: f, person: person} +} + +// task3399MemberSince is every fixture member's created_at. +const task3399MemberSince = "2026-01-01T00:00:00Z" + +// task3399Req is a delegated grant's persistence request, as the consent +// decision builds its session data. +func task3399Req(clientID, subject, requestID, access string, epoch int) models.OAuthRequest { + r := task3394Req(clientID, subject, requestID) + r.SessionData = `{"extra":{"` + InstallEpochSessionKey + `":` + itoa(epoch) + `,"` + InstallAuthKindSessionKey + `":"delegated","` + + InstallAccessSessionKey + `":"` + access + `","` + InstallPersonEpochSessionKey + `":0,"` + + InstallMemberSinceSessionKey + `":"` + task3399MemberSince + `"}}` + return r +} + +// The whole code flow persists for a person: code, PKCE, access and refresh, +// under one binding carrying the delegated kind and the consented access. +func TestTask3399_ADelegatedGrantPersistsThroughTheBarrier(t *testing.T) { + f := task3399Fixture(t, "inst-d1", "write") + for _, persist := range []func(models.OAuthRequest) error{ + f.s.CreateAuthorizationCode, f.s.CreatePKCERequest, f.s.CreateAccessToken, f.s.CreateRefreshToken, + } { + if err := persist(task3399Req(f.clientID, f.person.ID, "req-d1", "read", 1)); err != nil { + t.Fatal(err) + } + } + var kind, access string + if err := f.s.db.QueryRow(f.s.q(`SELECT auth_kind, delegated_access FROM app_token_bindings WHERE request_id = 'req-d1'`)).Scan(&kind, &access); err != nil { + t.Fatal(err) + } + if kind != "delegated" || access != "read" { + t.Errorf("binding = %s/%s, want delegated/read", kind, access) + } + if n := task3394Count(t, f.s, `SELECT COUNT(*) FROM app_token_bindings WHERE request_id = 'req-d1'`); n != 1 { + t.Errorf("%d bindings for one grant, want 1", n) + } +} + +// Every persistence re-checks the epoch: a disable or rotate between +// authorize and any later step refuses that step (the lead's required race, +// at the store; the end-to-end race is in the server tests). +func TestTask3399_EveryPersistenceRechecksTheEpoch(t *testing.T) { + steps := map[string]func(*Store, models.OAuthRequest) error{ + "code": func(s *Store, r models.OAuthRequest) error { return s.CreateAuthorizationCode(r) }, + "pkce": func(s *Store, r models.OAuthRequest) error { return s.CreatePKCERequest(r) }, + "access": func(s *Store, r models.OAuthRequest) error { return s.CreateAccessToken(r) }, + "refresh": func(s *Store, r models.OAuthRequest) error { return s.CreateRefreshToken(r) }, + } + for name, step := range steps { + for change, q := range map[string]string{ + "rotate": `UPDATE app_installs SET auth_epoch = auth_epoch + 1 WHERE id = ?`, + "disable": `UPDATE app_installs SET state = 'disabling', auth_epoch = auth_epoch + 1 WHERE id = ?`, + } { + t.Run(name+"/"+change, func(t *testing.T) { + f := task3399Fixture(t, "inst-ep-"+name+"-"+change, "write") + // A token's grant has its code first (its binding); the code + // and PKCE steps are themselves the first persistence. + if name == "access" || name == "refresh" { + if err := f.s.CreateAuthorizationCode(task3399Req(f.clientID, f.person.ID, "req-ep", "read", 1)); err != nil { + t.Fatal(err) + } + } + // Authorized under epoch 1, which the change then moves. + if _, err := f.s.db.Exec(f.s.q(q), f.installID); err != nil { + t.Fatal(err) + } + if err := step(f.s, task3399Req(f.clientID, f.person.ID, "req-ep", "read", 1)); !errors.Is(err, ErrInstallNotActive) { + t.Errorf("%s after a %s: err = %v, want ErrInstallNotActive", name, change, err) + } + want := 0 + if name == "access" || name == "refresh" { + want = 1 // the code's + } + if n := task3394Count(t, f.s, `SELECT COUNT(*) FROM app_token_bindings`); n != want { + t.Errorf("a refused %s left %d bindings, want %d", name, n, want) + } + }) + } + } + // A family whose code persisted under epoch 1 cannot mint its tokens + // after the epoch moved, even carrying the new one. + f := task3399Fixture(t, "inst-ep-family", "write") + if err := f.s.CreateAuthorizationCode(task3399Req(f.clientID, f.person.ID, "req-fam", "read", 1)); err != nil { + t.Fatal(err) + } + if _, err := f.s.db.Exec(f.s.q(`UPDATE app_installs SET auth_epoch = auth_epoch + 1 WHERE id = ?`), f.installID); err != nil { + t.Fatal(err) + } + if err := f.s.CreateAccessToken(task3399Req(f.clientID, f.person.ID, "req-fam", "read", 2)); !errors.Is(err, ErrInstallNotActive) { + t.Errorf("the family's token after the epoch moved: err = %v, want ErrInstallNotActive", err) + } +} + +func TestTask3399_TheSubjectIsALiveHumanMember(t *testing.T) { + f := task3399Fixture(t, "inst-subj", "write") + outsider := createTestUser(t, f.s, "outsider@test.com", "Outsider", "password123") + disabled := createTestUser(t, f.s, "gone@test.com", "Gone", "password123") + if err := f.s.AddWorkspaceMember(f.ws.ID, disabled.ID, "editor"); err != nil { + t.Fatal(err) + } + if _, err := f.s.db.Exec(f.s.q(`UPDATE users SET disabled_at = ? WHERE id = ?`), now(), disabled.ID); err != nil { + t.Fatal(err) + } + for name, subject := range map[string]string{ + "a non-member": outsider.ID, + "a disabled member": disabled.ID, + "the install's own bot": f.bot.ID, + "an unknown user": "no-such-user", + } { + if err := f.s.CreateAuthorizationCode(task3399Req(f.clientID, subject, "req-"+subject, "read", 1)); !errors.Is(err, ErrInstallDelegatedSubject) { + t.Errorf("%s: err = %v, want ErrInstallDelegatedSubject", name, err) + } + } + if n := task3394Count(t, f.s, `SELECT COUNT(*) FROM oauth_authorization_codes`); n != 0 { + t.Errorf("refused subjects persisted %d codes", n) + } +} + +func TestTask3399_TheAccessIsWithinTheManifest(t *testing.T) { + cases := []struct { + offered, access string + ok bool + }{ + {"write", "write", true}, + {"write", "read", true}, + {"read", "read", true}, + {"read", "write", false}, + {"", "read", false}, // the manifest offers no delegated access + {"write", "", false}, + {"write", "admin", false}, + } + for i, c := range cases { + f := task3399Fixture(t, "inst-acc-"+itoa(i), c.offered) + err := f.s.CreateAuthorizationCode(task3399Req(f.clientID, f.person.ID, "req-acc", c.access, 1)) + if c.ok && err != nil { + t.Errorf("offered %q, consented %q: %v", c.offered, c.access, err) + } + if !c.ok && !errors.Is(err, ErrInstallDelegatedAccess) { + t.Errorf("offered %q, consented %q: err = %v, want ErrInstallDelegatedAccess", c.offered, c.access, err) + } + } +} + +// A later step cannot change what the grant is: its kind and access are +// fixed by the binding its first persistence wrote. +func TestTask3399_TheBindingFixesKindAndAccess(t *testing.T) { + f := task3399Fixture(t, "inst-fix", "write") + if err := f.s.CreateAuthorizationCode(task3399Req(f.clientID, f.person.ID, "req-fix", "read", 1)); err != nil { + t.Fatal(err) + } + if err := f.s.CreateAccessToken(task3399Req(f.clientID, f.person.ID, "req-fix", "write", 1)); !errors.Is(err, ErrInstallDelegatedAccess) { + t.Errorf("a token widening the consented access: err = %v, want ErrInstallDelegatedAccess", err) + } +} + +// task3399MCPChain mints an ordinary MCP chain (access + refresh) for subject. +func task3399MCPChain(t *testing.T, s *Store, clientID, subject, requestID string) { + t.Helper() + for _, persist := range []func(models.OAuthRequest) error{s.CreateAccessToken, s.CreateRefreshToken} { + r := task3394Req(clientID, subject, requestID) + r.Scopes, r.GrantedScopes, r.SessionData = "pad:read", "pad:read", `{"extra":{}}` + if err := persist(r); err != nil { + t.Fatal(err) + } + } +} + +// task3399Delegated mints a delegated grant (code, access, refresh). +func task3399Delegated(t *testing.T, f task3399Fix, requestID string) { + t.Helper() + for _, persist := range []func(models.OAuthRequest) error{f.s.CreateAuthorizationCode, f.s.CreateAccessToken, f.s.CreateRefreshToken} { + if err := persist(task3399Req(f.clientID, f.person.ID, requestID, "read", 1)); err != nil { + t.Fatal(err) + } + } +} + +// Lead ruling Q1: an installed app's chains are not MCP connections. Every +// reader that lists or counts chains as connections leaves them out; the +// app grant is listed on its own. +func TestTask3399_AppChainsAreNotMCPConnections(t *testing.T) { + f := task3399Fixture(t, "inst-readers", "write") + mcpClient := seedClient(t, f.s, "Desktop") + task3399MCPChain(t, f.s, mcpClient, f.person.ID, "req-mcp") + task3399Delegated(t, f, "req-app") + if err := f.s.CreateAccessToken(task3394Req(f.clientID, f.bot.ID, "req-svc")); err != nil { + t.Fatal(err) + } + + conns, err := f.s.ListUserOAuthConnections(f.person.ID) + if err != nil { + t.Fatal(err) + } + if len(conns) != 1 || conns[0].RequestID != "req-mcp" { + t.Errorf("Connected Apps list = %+v, want only the MCP chain", conns) + } + if n, err := f.s.CountLiveOAuthConnections(); err != nil || n != 1 { + t.Errorf("readiness resume count = %d (%v), want 1: app chains are not resumable MCP connections", n, err) + } + if _, err := f.s.BackfillOAuthConnections(); err != nil { + t.Fatal(err) + } + for req, want := range map[string]int{"req-mcp": 1, "req-app": 0, "req-svc": 0} { + if n := task3394Count(t, f.s, `SELECT COUNT(*) FROM oauth_connections WHERE request_id = ?`, req); n != want { + t.Errorf("backfill: %d connection rows for %s, want %d", n, req, want) + } + } + grants, err := f.s.ListUserAppGrants(f.person.ID) + if err != nil { + t.Fatal(err) + } + if len(grants) != 1 || grants[0].RequestID != "req-app" || grants[0].Access != "read" || grants[0].WorkspaceID != f.ws.ID { + t.Errorf("app grants = %+v, want the delegated grant", grants) + } +} + +func TestTask3399_RevokingAnAppGrantEndsItThroughTheBinding(t *testing.T) { + f := task3399Fixture(t, "inst-revoke", "write") + task3399Delegated(t, f, "req-rv") + other := createTestUser(t, f.s, "other-rv@test.com", "Other", "password123") + if _, err := f.s.RevokeUserAppGrant(other.ID, "req-rv"); !errors.Is(err, ErrAppGrantNotFound) { + t.Errorf("another user's revoke: %v, want ErrAppGrantNotFound", err) + } + if _, err := f.s.RevokeUserAppGrant(f.person.ID, "req-rv"); err != nil { + t.Fatal(err) + } + if n := task3394Count(t, f.s, `SELECT COUNT(*) FROM app_token_bindings WHERE request_id = 'req-rv' AND revoked_at IS NOT NULL`); n != 1 { + t.Error("the revoke left no tombstone on the binding") + } + if st, err := f.s.GetAppTokenState("req-rv"); err != nil || st != nil { + t.Errorf("introspection state of a revoked grant = %+v, %v; want none", st, err) + } + for _, table := range []string{"oauth_access_tokens", "oauth_refresh_tokens"} { + if n := task3394Count(t, f.s, `SELECT COUNT(*) FROM `+table+` WHERE request_id = 'req-rv' AND active = ?`, true); n != 0 { + t.Errorf("%s: %d active rows after the revoke", table, n) + } + } + if n := task3394Count(t, f.s, `SELECT COUNT(*) FROM oauth_authorization_codes WHERE request_id = 'req-rv'`); n != 0 { + t.Error("the grant's code survived the revoke") + } + if grants, _ := f.s.ListUserAppGrants(f.person.ID); len(grants) != 0 { + t.Errorf("a revoked grant is still listed: %+v", grants) + } +} + +// Lead ruling Q4: the expiry sweep removes bindings whose grant has no row +// left, for service and delegated grants alike; a live binding stays. +func TestTask3399_TheSweepRemovesOrphanBindings(t *testing.T) { + f := task3399Fixture(t, "inst-sweep", "write") + if err := f.s.CreateAccessToken(task3394Req(f.clientID, f.bot.ID, "req-gone")); err != nil { + t.Fatal(err) + } + if err := f.s.CreateAccessToken(task3394Req(f.clientID, f.bot.ID, "req-live")); err != nil { + t.Fatal(err) + } + task3399Delegated(t, f, "req-dgone") + for _, q := range []string{ + `DELETE FROM oauth_access_tokens WHERE request_id IN ('req-gone', 'req-dgone')`, + `DELETE FROM oauth_refresh_tokens WHERE request_id = 'req-dgone'`, + `DELETE FROM oauth_authorization_codes WHERE request_id = 'req-dgone'`, + } { + if _, err := f.s.db.Exec(f.s.q(q)); err != nil { + t.Fatal(err) + } + } + res, err := f.s.SweepExpiredOAuthRows(OAuthSweepCutoffs{}, 100, 10, 0) + if err != nil { + t.Fatal(err) + } + if res.AppTokenBindings != 2 { + t.Errorf("swept %d bindings, want the 2 orphans", res.AppTokenBindings) + } + if n := task3394Count(t, f.s, `SELECT COUNT(*) FROM app_token_bindings WHERE request_id = 'req-live'`); n != 1 { + t.Error("the sweep removed a live binding") + } +} + +// Disabling or claiming an account deactivates the person's unexchanged +// delegated codes through their bindings, so a re-enable cannot exchange +// one; erasing the account cascades the bindings away. +func TestTask3399_RevokingAPersonReachesTheirDelegatedCodes(t *testing.T) { + f := task3399Fixture(t, "inst-person", "write") + if err := f.s.CreateAuthorizationCode(task3399Req(f.clientID, f.person.ID, "req-pending", "read", 1)); err != nil { + t.Fatal(err) + } + if err := f.s.DisableUserAndRevokeAccess(f.person.ID); err != nil { + t.Fatal(err) + } + if n := task3394Count(t, f.s, `SELECT COUNT(*) FROM oauth_authorization_codes WHERE request_id = 'req-pending' AND active = ?`, true); n != 0 { + t.Error("a disabled person's delegated code is still active") + } + g := task3399Fixture(t, "inst-erase", "write") + task3399Delegated(t, g, "req-erase") + if err := g.s.DeleteAccountAtomic(g.person.ID); err != nil { + t.Fatal(err) + } + if n := task3394Count(t, g.s, `SELECT COUNT(*) FROM app_token_bindings WHERE request_id = 'req-erase'`); n != 0 { + t.Error("an erased person's binding survived") + } +} + +// A grant's later persistence cannot change whom it acts for: the binding +// fixes the person its first persistence named. +func TestTask3399_TheBindingFixesThePerson(t *testing.T) { + f := task3399Fixture(t, "inst-person-fix", "write") + other := createTestUser(t, f.s, "other-fix@test.com", "Other", "password123") + if err := f.s.AddWorkspaceMember(f.ws.ID, other.ID, "editor"); err != nil { + t.Fatal(err) + } + if err := f.s.CreateAuthorizationCode(task3399Req(f.clientID, f.person.ID, "req-pfix", "read", 1)); err != nil { + t.Fatal(err) + } + if err := f.s.CreateAccessToken(task3399Req(f.clientID, other.ID, "req-pfix", "read", 1)); !errors.Is(err, ErrInstallDelegatedSubject) { + t.Errorf("a token for another member under the same grant: err = %v, want ErrInstallDelegatedSubject", err) + } +} + +// Codex U5b-1 r1: a revoke, a disable and re-enable, or a removal and re-add +// between two persistences of one grant ends it; none is forgotten. +func TestTask3399_AChangeBetweenPersistencesIsRemembered(t *testing.T) { + cases := map[string]func(t *testing.T, f task3399Fix){ + "the person revoked it": func(t *testing.T, f task3399Fix) { + if _, err := f.s.RevokeUserAppGrant(f.person.ID, "req-mem"); err != nil { + t.Fatal(err) + } + }, + "a disable and re-enable": func(t *testing.T, f task3399Fix) { + if err := f.s.DisableUserAndRevokeAccess(f.person.ID); err != nil { + t.Fatal(err) + } + if err := f.s.EnableUser(f.person.ID); err != nil { + t.Fatal(err) + } + }, + "a removal and re-add": func(t *testing.T, f task3399Fix) { + if _, err := f.s.db.Exec(f.s.q(`DELETE FROM workspace_members WHERE user_id = ? AND workspace_id = ?`), f.person.ID, f.ws.ID); err != nil { + t.Fatal(err) + } + if _, err := f.s.db.Exec(f.s.q(`INSERT INTO workspace_members (workspace_id, user_id, role, created_at) VALUES (?, ?, 'editor', ?)`), + f.ws.ID, f.person.ID, "2099-01-01T00:00:00Z"); err != nil { + t.Fatal(err) + } + }, + } + for name, change := range cases { + t.Run(name, func(t *testing.T) { + f := task3399Fixture(t, "inst-mem-"+strings.ReplaceAll(name, " ", "-"), "write") + task3399Delegated(t, f, "req-mem") + change(t, f) + // The re-insert an in-flight refresh rotation makes after its checks. + // (The grant's EARLIER tokens are another matter: a membership + // removal leaves them to the app API's per-request admission, U5b-2.) + r := task3399Req(f.clientID, f.person.ID, "req-mem", "read", 1) + if err := f.s.CreateRefreshToken(r); err == nil { + t.Fatalf("after %s, the grant persisted a new refresh token", name) + } + if n := task3394Count(t, f.s, `SELECT COUNT(*) FROM oauth_refresh_tokens WHERE signature = ?`, r.Signature); n != 0 { + t.Errorf("after %s: the refused refresh token was stored", name) + } + }) + } +} + +// Codex U5b-1 r1 P1: an account disable that commits while a delegated +// issuance is between its checks and its commit does not miss the rows the +// issuance writes. The disable waits for the issuance (the user-row share +// lock on Postgres, the single writer on SQLite) and then revokes them. +func TestTask3399_ADisableIsOrderedAgainstIssuance(t *testing.T) { + f := task3399Fixture(t, "inst-order", "write") + task3399Delegated(t, f, "req-ord") + tx, err := f.s.db.Begin() + if err != nil { + t.Fatal(err) + } + r := task3399Req(f.clientID, f.person.ID, "req-ord", "read", 1) + r.Signature = "sig-ord-new" + if _, err := f.s.installIssuanceBarrierTx(tx, "oauth_refresh_tokens", r, now()); err != nil { + _ = tx.Rollback() + t.Fatal(err) + } + done := make(chan error, 1) + go func() { done <- f.s.DisableUserAndRevokeAccess(f.person.ID) }() + select { + case err := <-done: + // SQLite in-memory test stores may not block; either way the rows + // must end up revoked, which the assertion below checks. + if err != nil { + t.Logf("disable returned before the issuance committed: %v", err) + } + case <-time.After(300 * time.Millisecond): + } + if err := tx.Commit(); err != nil { + t.Fatal(err) + } + if err := <-done; err != nil { + t.Fatalf("disable: %v", err) + } + if n := task3394Count(t, f.s, `SELECT COUNT(*) FROM oauth_refresh_tokens WHERE request_id = 'req-ord' AND active = ?`, true); n != 0 { + t.Errorf("%d refresh tokens of a disabled person are still active", n) + } +} + +// Codex U5b-1 r2 P1: a revoked grant whose tombstone the sweep removed is +// not revived by a persistence already past its checks: only a grant's code +// writes its binding, so a token persisting against none is refused. +func TestTask3399_ASweptTombstoneRevivesNothing(t *testing.T) { + f := task3399Fixture(t, "inst-swept", "write") + if err := f.s.CreateAuthorizationCode(task3399Req(f.clientID, f.person.ID, "req-sw", "read", 1)); err != nil { + t.Fatal(err) + } + if _, err := f.s.RevokeUserAppGrant(f.person.ID, "req-sw"); err != nil { + t.Fatal(err) + } + if _, err := f.s.SweepExpiredOAuthRows(OAuthSweepCutoffs{}, 100, 10, 0); err != nil { + t.Fatal(err) + } + if n := task3394Count(t, f.s, `SELECT COUNT(*) FROM app_token_bindings WHERE request_id = 'req-sw'`); n != 0 { + t.Fatal("control: the sweep kept the tombstone, so this does not test its removal") + } + for _, persist := range []func(models.OAuthRequest) error{f.s.CreateAccessToken, f.s.CreateRefreshToken} { + if err := persist(task3399Req(f.clientID, f.person.ID, "req-sw", "read", 1)); !errors.Is(err, ErrInstallNotActive) { + t.Errorf("a token for a swept, revoked grant: err = %v, want ErrInstallNotActive", err) + } + } + if n := task3394Count(t, f.s, `SELECT COUNT(*) FROM app_token_bindings WHERE request_id = 'req-sw'`); n != 0 { + t.Error("a refused persistence recreated the binding") + } +} + +// Codex U5b-1 r2 P2: removing a member ends their delegated grants in that +// workspace, so a re-add in the same second revives nothing. +func TestTask3399_RemovingAMemberEndsTheirGrants(t *testing.T) { + for _, remove := range []string{"RemoveWorkspaceMember", "RemoveWorkspaceMemberAndRevokeGrants"} { + t.Run(remove, func(t *testing.T) { + f := task3399Fixture(t, "inst-rm-"+remove, "write") + task3399Delegated(t, f, "req-rm") + var err error + if remove == "RemoveWorkspaceMember" { + err = f.s.RemoveWorkspaceMember(f.ws.ID, f.person.ID) + } else { + err = f.s.RemoveWorkspaceMemberAndRevokeGrants(f.ws.ID, f.person.ID) + } + if err != nil { + t.Fatal(err) + } + if err := f.s.AddWorkspaceMember(f.ws.ID, f.person.ID, "editor"); err != nil { + t.Fatal(err) + } + if n := task3394Count(t, f.s, `SELECT COUNT(*) FROM oauth_refresh_tokens WHERE request_id = 'req-rm' AND active = ?`, true); n != 0 { + t.Errorf("%d refresh tokens survive the removal", n) + } + if err := f.s.CreateRefreshToken(task3399Req(f.clientID, f.person.ID, "req-rm", "read", 1)); err == nil { + t.Error("the grant persisted a new refresh token after a removal and same-second re-add") + } + }) + } +} + +// Codex U5b-1 r6: a grant the install's epoch has moved past (an upgrade, a +// rotate) is already unusable, and the console does not list it. +func TestTask3399_AGrantPastTheInstallEpochIsNotListed(t *testing.T) { + f := task3399Fixture(t, "inst-epoch-list", "write") + task3399Delegated(t, f, "req-el") + if grants, _ := f.s.ListUserAppGrants(f.person.ID); len(grants) != 1 { + t.Fatalf("control: %d grants listed", len(grants)) + } + if _, err := f.s.db.Exec(f.s.q(`UPDATE app_installs SET auth_epoch = auth_epoch + 1 WHERE id = ?`), f.installID); err != nil { + t.Fatal(err) + } + if grants, _ := f.s.ListUserAppGrants(f.person.ID); len(grants) != 0 { + t.Errorf("a grant past the install's epoch is listed: %+v", grants) + } +} + +// Codex U5b-1 r7 P1: a consent decided under the person's credentials as a +// session resolved them cannot mint a code after those credentials changed +// (a disable destroyed that session, then a re-enable). +func TestTask3399_AConsentFromAStaleSessionMintsNothing(t *testing.T) { + f := task3399Fixture(t, "inst-stale", "write") + if err := f.s.DisableUserAndRevokeAccess(f.person.ID); err != nil { + t.Fatal(err) + } + if err := f.s.EnableUser(f.person.ID); err != nil { + t.Fatal(err) + } + // task3399Req carries the epoch the consent saw: 0, before the disable. + if err := f.s.CreateAuthorizationCode(task3399Req(f.clientID, f.person.ID, "req-stale", "read", 1)); !errors.Is(err, ErrInstallDelegatedSubject) { + t.Errorf("a code from a consent under old credentials: err = %v, want ErrInstallDelegatedSubject", err) + } + if n := task3394Count(t, f.s, `SELECT COUNT(*) FROM oauth_authorization_codes`); n != 0 { + t.Errorf("%d codes stored", n) + } +} + +// Codex U5b-1 r7 P2: a grant consented to but not yet exchanged is listed, +// so its person can revoke its code. +func TestTask3399_AnUnexchangedGrantIsListedAndRevocable(t *testing.T) { + f := task3399Fixture(t, "inst-unexch", "write") + if err := f.s.CreateAuthorizationCode(task3399Req(f.clientID, f.person.ID, "req-ux", "read", 1)); err != nil { + t.Fatal(err) + } + grants, err := f.s.ListUserAppGrants(f.person.ID) + if err != nil || len(grants) != 1 || grants[0].RequestID != "req-ux" { + t.Fatalf("an unexchanged grant: listed %+v, %v", grants, err) + } + if _, err := f.s.RevokeUserAppGrant(f.person.ID, "req-ux"); err != nil { + t.Fatal(err) + } + if n := task3394Count(t, f.s, `SELECT COUNT(*) FROM oauth_authorization_codes WHERE request_id = 'req-ux'`); n != 0 { + t.Error("the revoked grant's code survived") + } + if err := f.s.CreateAccessToken(task3399Req(f.clientID, f.person.ID, "req-ux", "read", 1)); err == nil { + t.Error("the revoked grant minted a token") + } +} + +// Codex U5b-1 r8: a consent carries the membership its gate read; a removal +// and re-add before its code persists (no grant yet for the removal to +// revoke) refuses the code. +func TestTask3399_AConsentAcrossARemovalAndReAddMintsNothing(t *testing.T) { + f := task3399Fixture(t, "inst-readd", "write") + if _, err := f.s.db.Exec(f.s.q(`DELETE FROM workspace_members WHERE user_id = ? AND workspace_id = ?`), f.person.ID, f.ws.ID); err != nil { + t.Fatal(err) + } + if _, err := f.s.db.Exec(f.s.q(`INSERT INTO workspace_members (workspace_id, user_id, role, created_at) VALUES (?, ?, 'editor', ?)`), + f.ws.ID, f.person.ID, "2026-06-01T00:00:00Z"); err != nil { + t.Fatal(err) + } + if err := f.s.CreateAuthorizationCode(task3399Req(f.clientID, f.person.ID, "req-readd", "read", 1)); !errors.Is(err, ErrInstallDelegatedSubject) { + t.Errorf("a code from a consent given under the earlier membership: err = %v, want ErrInstallDelegatedSubject", err) + } +} + +// Codex U5b-1 r9: a consent approved after the workspace was soft-deleted +// persists nothing. +func TestTask3399_ADeletedWorkspaceTakesNoGrant(t *testing.T) { + f := task3399Fixture(t, "inst-wsdel", "write") + if _, err := f.s.db.Exec(f.s.q(`UPDATE workspaces SET deleted_at = ? WHERE id = ?`), now(), f.ws.ID); err != nil { + t.Fatal(err) + } + if err := f.s.CreateAuthorizationCode(task3399Req(f.clientID, f.person.ID, "req-wsdel", "read", 1)); !errors.Is(err, ErrInstallDelegatedSubject) { + t.Errorf("a code in a deleted workspace: err = %v, want ErrInstallDelegatedSubject", err) + } + if since, err := f.s.WorkspaceMemberSince(f.ws.ID, f.person.ID); err != nil || since != "" { + t.Errorf("the consent gate's membership read in a deleted workspace = %q, %v; want none", since, err) + } +} diff --git a/internal/store/task3399_reader_census_test.go b/internal/store/task3399_reader_census_test.go new file mode 100644 index 000000000..184b373e8 --- /dev/null +++ b/internal/store/task3399_reader_census_test.go @@ -0,0 +1,133 @@ +package store + +import ( + "go/ast" + "go/parser" + "go/token" + "os" + "path/filepath" + "sort" + "strconv" + "strings" + "testing" +) + +// TASK-3399 (lead ruling Q1): a delegated grant to an installed app writes +// no oauth_connections row, so it must not surface as an MCP connection +// anywhere. Every store function whose SQL reads the connection tables or +// treats token rows as chains is classified here. A new one fails until it +// is classified. +var task3399ReaderCensus = map[string]string{ + "account_claim.go::claimAccountTx": censusUserRevoke, + "app_grants.go::ListUserAppGrants": censusAppPath, + "connected_apps.go::ListUserOAuthConnections": censusFilters, + "connected_apps.go::RevokeUserOAuthConnection": censusPerGrant, + "install_clients.go::DeleteInstallClientTx": censusAppPath, + "mcp_resume.go::CountLiveOAuthConnections": censusFilters, + "oauth.go::CountActiveOAuthAccessTokens": censusByDesign, + "oauth.go::DeleteAccessToken": censusPerGrant, + "oauth.go::DeleteOAuthClient": censusPerGrant, + "oauth.go::DeleteRefreshToken": censusPerGrant, + "oauth.go::OldestAccessTokenIssuedAtByRequestID": censusPerGrant, + "oauth.go::queryOAuthRequestRow": censusPerGrant, + "oauth_connections.go::AddConnectionWorkspace": censusNoRow, + "oauth_connections.go::AddCreatedWorkspaceIfPermitted": censusNoRow, + "oauth_connections.go::ConnectionWorkspaceCount": censusNoRow, + "oauth_connections.go::CreateOAuthConnection": censusNoRow, + "oauth_connections.go::DeleteOAuthConnection": censusNoRow, + "oauth_connections.go::GetOAuthConnection": censusNoRow, + "oauth_connections.go::GetOAuthConnectionAccess": censusNoRow, + "oauth_connections.go::HasActiveConnectionForUser": censusNoRow, + "oauth_connections.go::IsConnectionWorkspaceAllowed": censusNoRow, + "oauth_connections.go::IsWorkspaceCoveredForUser": censusNoRow, + "oauth_connections.go::LimitConnectionToCurrentWorkspaces": censusNoRow, + "oauth_connections.go::ListConnectionWorkspaceSlugs": censusNoRow, + "oauth_connections.go::RemoveConnectionWorkspace": censusNoRow, + "oauth_connections.go::RemoveConnectionWorkspaceUnlessLast": censusNoRow, + "oauth_connections.go::RenameConnection": censusNoRow, + "oauth_connections.go::SetScopeFlags": censusNoRow, + "oauth_connections.go::SetScopeFlagsGuarded": censusNoRow, + "oauth_connections.go::assertRowAffected": censusNoRow, + "oauth_connections.go::lockConnectionTx": censusNoRow, + "oauth_connections.go::snapshotCurrentWorkspacesSQL": censusNoRow, + "oauth_connections_backfill.go::BackfillOAuthConnections": censusFilters, + "oauth_connections_backfill.go::backfillOneChain": censusFilters, + "oauth_connections_backfill.go::collectGrantChainsForBackfill": censusFilters, + "oauth_connections_narrow.go::NarrowCurrentOnlyWildcardConnections": censusNoRow, + "oauth_sweep.go::SweepExpiredOAuthRows": censusByDesign, + "users.go::disableUserAndRevokeAccessTx": censusUserRevoke, + "users.go::eraseUserTx": censusUserRevoke, +} + +const ( + censusNoRow = "reads oauth_connections rows: an install chain has none, so it is untouched by construction" + censusPerGrant = "addressed by one signature, request id or client id: never lists a user's chains" + censusFilters = "lists or counts chains as connections: filters install clients (notInstallClientSQL)" + censusAppPath = "the app-grant path itself (bindings, the barrier, app lifecycle)" + censusByDesign = "counts or sweeps every token on purpose (an ops gauge, the expiry sweep)" + censusUserRevoke = "revokes or erases every credential of one user: delegated app codes through their bindings (delegated_user_id), tokens by subject" +) + +func TestTask3399_EveryConnectionReaderIsClassified(t *testing.T) { + needles := []string{"oauth_connections", "oauth_connection_workspaces", "FROM oauth_access_tokens", "FROM oauth_refresh_tokens"} + fset := token.NewFileSet() + files, err := filepath.Glob("*.go") + if err != nil { + t.Fatal(err) + } + found := map[string]bool{} + for _, name := range files { + if strings.HasSuffix(name, "_test.go") { + continue + } + src, err := os.ReadFile(name) + if err != nil { + t.Fatal(err) + } + f, err := parser.ParseFile(fset, name, src, 0) + if err != nil { + t.Fatal(err) + } + for _, d := range f.Decls { + fn, ok := d.(*ast.FuncDecl) + if !ok || fn.Body == nil { + continue + } + hit := false + ast.Inspect(fn.Body, func(n ast.Node) bool { + lit, ok := n.(*ast.BasicLit) + if !ok || lit.Kind != token.STRING { + return true + } + v, err := strconv.Unquote(lit.Value) + if err != nil { + v = lit.Value + } + for _, nd := range needles { + if strings.Contains(v, nd) { + hit = true + } + } + return true + }) + if hit { + found[name+"::"+fn.Name.Name] = true + } + } + } + var missing []string + for k := range found { + if _, ok := task3399ReaderCensus[k]; !ok { + missing = append(missing, k) + } + } + sort.Strings(missing) + for _, k := range missing { + t.Errorf("%s reads connection or token-chain tables and is not classified in task3399ReaderCensus", k) + } + for k := range task3399ReaderCensus { + if !found[k] { + t.Errorf("task3399ReaderCensus names %s, which no longer reads those tables", k) + } + } +} diff --git a/internal/store/users.go b/internal/store/users.go index b7af69bc6..713928992 100644 --- a/internal/store/users.go +++ b/internal/store/users.go @@ -987,18 +987,26 @@ func (s *Store) requireActiveUserTx(tx *sql.Tx, userID string) error { // Nothing here is restored by EnableUser: the user signs in, mints tokens // and reconnects apps again. func (s *Store) DisableUserAndRevokeAccess(userID string) error { - tx, err := s.db.Begin() - if err != nil { - return fmt.Errorf("disable user: begin: %w", err) - } - defer func() { _ = tx.Rollback() }() - if err := s.disableUserAndRevokeAccessTx(tx, userID); err != nil { - return err - } - if err := tx.Commit(); err != nil { - return fmt.Errorf("disable user: commit: %w", err) - } - return nil + // One transaction per attempt, nothing outside it before the commit: + // retryable on a deadlock (TASK-3399). + attempt := 0 + return s.retryOnDeadlock("disable_user", func() error { + tx, err := s.db.Begin() + if err != nil { + return fmt.Errorf("disable user: begin: %w", err) + } + defer func() { _ = tx.Rollback() }() + if err := s.disableUserAndRevokeAccessTx(tx, userID); err != nil { + return err + } + if err := s.injectedDeadlock("disable_user", &attempt); err != nil { + return err + } + if err := tx.Commit(); err != nil { + return fmt.Errorf("disable user: commit: %w", err) + } + return nil + }) } // disableUserAndRevokeAccessTx is DisableUserAndRevokeAccess on the caller's @@ -1023,10 +1031,18 @@ func (s *Store) disableUserAndRevokeAccessTx(tx *sql.Tx, userID string) error { {"delete oauth pkce requests", `DELETE FROM oauth_pkce_requests WHERE request_id IN (SELECT request_id FROM oauth_connections WHERE user_id = ?)`, []any{userID}}, {"delete oauth connections", `DELETE FROM oauth_connections WHERE user_id = ?`, []any{userID}}, } - for _, st := range stmts { + for i, st := range stmts { if _, err := tx.Exec(s.q(st.query), st.args...); err != nil { return fmt.Errorf("disable user: %s: %w", st.what, err) } + // After the users row (the first statement), before any token row: + // the person's delegated app grants, through their installs + // (TASK-3399, the users -> installs -> tokens order). + if i == 0 { + if err := s.revokeDelegatedGrantsTx(tx, userID, ""); err != nil { + return fmt.Errorf("disable user: %w", err) + } + } } return nil } diff --git a/internal/store/workspace_members.go b/internal/store/workspace_members.go index 897ae55af..78f3efaf5 100644 --- a/internal/store/workspace_members.go +++ b/internal/store/workspace_members.go @@ -187,6 +187,13 @@ func (s *Store) AcceptWorkspaceInvitation(invitationID, workspaceID, userID, rol // The user's workspace tab goes with it when no grant keeps them in the // workspace (TASK-3256). func (s *Store) RemoveWorkspaceMember(workspaceID, userID string) error { + // One transaction per attempt, nothing outside it before the commit: + // retryable on a deadlock (TASK-3399). + attempt := 0 + return s.retryOnDeadlock("remove_workspace_member", func() error { return s.removeWorkspaceMemberOnce(workspaceID, userID, &attempt) }) +} + +func (s *Store) removeWorkspaceMemberOnce(workspaceID, userID string, attempt *int) error { tx, err := s.db.Begin() if err != nil { return fmt.Errorf("begin transaction: %w", err) @@ -204,6 +211,11 @@ func (s *Store) RemoveWorkspaceMember(workspaceID, userID string) error { if err := s.guardOwnerLossTx(tx, workspaceID, userID); err != nil { return err } + // Their delegated app grants in this workspace end with the membership + // (TASK-3399): a re-add does not revive them. + if err := s.revokeDelegatedGrantsTx(tx, userID, workspaceID); err != nil { + return err + } result, err := tx.Exec( s.q("DELETE FROM workspace_members WHERE workspace_id = ? AND user_id = ?"), workspaceID, userID, @@ -218,6 +230,9 @@ func (s *Store) RemoveWorkspaceMember(workspaceID, userID string) error { if err := s.pruneWorkspaceTabIfNoAccessTx(tx, userID, workspaceID); err != nil { return err } + if err := s.injectedDeadlock("remove_workspace_member", attempt); err != nil { + return err + } return tx.Commit() } @@ -225,6 +240,13 @@ func (s *Store) RemoveWorkspaceMember(workspaceID, userID string) error { // and revokes all their grants in a single transaction. This prevents the user // from retaining guest access if the member removal succeeds but grant revocation fails. func (s *Store) RemoveWorkspaceMemberAndRevokeGrants(workspaceID, userID string) error { + // One transaction per attempt, nothing outside it before the commit: + // retryable on a deadlock (TASK-3399). + attempt := 0 + return s.retryOnDeadlock("remove_workspace_member", func() error { return s.removeWorkspaceMemberAndRevokeGrantsOnce(workspaceID, userID, &attempt) }) +} + +func (s *Store) removeWorkspaceMemberAndRevokeGrantsOnce(workspaceID, userID string, attempt *int) error { tx, err := s.db.Begin() if err != nil { return fmt.Errorf("begin transaction: %w", err) @@ -243,6 +265,12 @@ func (s *Store) RemoveWorkspaceMemberAndRevokeGrants(workspaceID, userID string) return err } + // Their delegated app grants in this workspace end with the membership + // (TASK-3399): a re-add does not revive them. + if err := s.revokeDelegatedGrantsTx(tx, userID, workspaceID); err != nil { + return err + } + // Revoke grants first (before removing membership) if _, err := tx.Exec(s.q("DELETE FROM collection_grants WHERE workspace_id = ? AND user_id = ?"), workspaceID, userID); err != nil { return fmt.Errorf("revoke collection grants: %w", err) @@ -264,6 +292,9 @@ func (s *Store) RemoveWorkspaceMemberAndRevokeGrants(workspaceID, userID string) return err } + if err := s.injectedDeadlock("remove_workspace_member", attempt); err != nil { + return err + } return tx.Commit() } diff --git a/web/src/lib/api/client.ts b/web/src/lib/api/client.ts index afa6ecf6e..5a274fd7b 100644 --- a/web/src/lib/api/client.ts +++ b/web/src/lib/api/client.ts @@ -84,6 +84,7 @@ import type { AttachmentListFilters, AttachmentListResponse, ConnectedApp, + AppGrant, ClaimCodeResponse, ImportArtifactResult } from '$lib/types'; @@ -3097,7 +3098,7 @@ export const api = { // the nav link in self-host. connectedApps: { - list: () => request<{ items: ConnectedApp[] }>('/connected-apps'), + list: () => request<{ items: ConnectedApp[]; app_grants?: AppGrant[] }>('/connected-apps'), revoke: (id: string) => request(`/connected-apps/${encodeURIComponent(id)}`, { method: 'DELETE' }), diff --git a/web/src/lib/types/index.ts b/web/src/lib/types/index.ts index bec0d2233..6d19ffa12 100644 --- a/web/src/lib/types/index.ts +++ b/web/src/lib/types/index.ts @@ -2550,6 +2550,18 @@ export interface ConnectedApp { include_future_workspaces?: boolean; } +// A delegated grant a person gave an installed app (TASK-3399): the app acts +// as them, in one workspace, with the access they chose. Listed on the +// Connected apps page read-only; it can only be revoked. +export interface AppGrant { + id: string; + app_name: string; + origin: string; + workspace: { slug: string; name: string }; + access: 'read' | 'write'; + granted_at: string; +} + // ─── Helper functions ──────────────────────────────────────────────────────── export function parseFields(item: Item): Record { diff --git a/web/src/routes/console/connected-apps/+page.svelte b/web/src/routes/console/connected-apps/+page.svelte index 075295c1d..bd20e37cf 100644 --- a/web/src/routes/console/connected-apps/+page.svelte +++ b/web/src/routes/console/connected-apps/+page.svelte @@ -5,7 +5,7 @@ import Chip from '$lib/components/common/Chip.svelte'; import Button from '$lib/components/common/Button.svelte'; import PageHeader from '$lib/components/common/PageHeader.svelte'; - import type { ConnectedApp, Workspace } from '$lib/types'; + import type { AppGrant, ConnectedApp, Workspace } from '$lib/types'; // Connected Apps page (TASK-954). Lists every active OAuth grant // chain the user has authorized via the MCP API and lets them @@ -14,14 +14,18 @@ // modal exists to make that point obvious. let apps = $state([]); + // TASK-3399: delegated grants a person gave an installed app. Read-only + // here apart from Revoke, which uses the same DELETE route. + let appGrants = $state([]); let loading = $state(true); let error = $state(''); let expanded = $state>({}); let chipsExpanded = $state>({}); - // Modal state — single modal at a time, keyed by the target app id. - let confirmTarget = $state(null); + // Modal state — single modal at a time, keyed by the target id. Holds + // either an MCP connection or an installed-app grant (TASK-3399). + let confirmTarget = $state<{ id: string; label: string } | null>(null); let revoking = $state(false); let revokeError = $state(''); @@ -247,6 +251,7 @@ try { const result = await api.connectedApps.list(); apps = Array.isArray(result?.items) ? result.items : []; + appGrants = Array.isArray(result?.app_grants) ? result.app_grants : []; } catch (e) { error = e instanceof Error ? e.message : 'Failed to load connected apps'; } finally { @@ -262,8 +267,8 @@ chipsExpanded[id] = !chipsExpanded[id]; } - function openConfirm(app: ConnectedApp) { - confirmTarget = app; + function openConfirm(target: { id: string; label: string }) { + confirmTarget = target; revokeError = ''; } @@ -307,7 +312,7 @@

{error}

- {:else if apps.length === 0} + {:else if apps.length === 0 && appGrants.length === 0}

No connected apps yet.

@@ -319,288 +324,345 @@

{:else} -
- {#each apps as app (app.id)} - {@const anyWs = isAnyWorkspace(app)} - {@const wsList = anyWs ? [] : (app.allowed_workspaces ?? [])} - {@const showAllChips = chipsExpanded[app.id]} - {@const visibleChips = showAllChips ? wsList : wsList.slice(0, 3)} - {@const extraChips = Math.max(0, wsList.length - 3)} -
-
- -
-
-

{app.client_name}

- - {TIER_LABELS[app.capability_tier]} - -
- -
- {#if anyWs} - Any workspace + {#if apps.length > 0} +
+ {#each apps as app (app.id)} + {@const anyWs = isAnyWorkspace(app)} + {@const wsList = anyWs ? [] : (app.allowed_workspaces ?? [])} + {@const showAllChips = chipsExpanded[app.id]} + {@const visibleChips = showAllChips ? wsList : wsList.slice(0, 3)} + {@const extraChips = Math.max(0, wsList.length - 3)} +
+
+ - -
-
-
Connected
-
- {relativeTime(app.connected_at)} -
+
+
+

{app.client_name}

+ + {TIER_LABELS[app.capability_tier]} +
-
-
Last used
-
- {relativeTime(app.last_used_at)} -
-
-
-
Activity
-
{callsLabel(app.calls_30d)}
-
-
- - - {#if expanded[app.id]} -
-
- Scopes - {app.scope_string || '—'} -
-
- Allowed workspaces - - {#if anyWs} - Any workspace - {:else} - {wsList.join(', ')} - {/if} - -
-
- Redirect URIs -
- {#if app.redirect_uris && app.redirect_uris.length > 0} - {#each app.redirect_uris as uri (uri)} - {uri} - {/each} - {:else} - — - {/if} -
-
-
- {/if} -
-
- -
- - -
- - {#if editingId === app.id} - {@const eaWsList = (app.allowed_workspaces ?? []).filter((s) => s !== '*')} - {@const pickable = pickableFor(app)} - {@const isAll = app.all_current_workspaces ?? true} -
- {#if editErrors[app.id]} -

{editErrors[app.id]}

- {/if} - -
- -
- - -
- {#if !(app.name ?? '')} -

- Name your connection so you can tell it apart from other apps. -

- {/if} -
- -
- Scope flags - - - {#if isAll} -
- -
-

- Replaces "all" with the workspaces you belong to now. Workspaces you join - later are not included until you add them. -

- {/if} -
- - -
-
- Workspace allow-list - {#if isAll} - Inert while wildcard is on - {/if} -
-
- {#each eaWsList as ws (ws)} - - {ws} +
+ {#if anyWs} + Any workspace + {:else} + {#each visibleChips as ws (ws)} + {ws} + {/each} + {#if !showAllChips && extraChips > 0} - - {/each} - {#if eaWsList.length === 0} - - {#if isAll} - No workspaces staged — add one if you plan to switch off - "All my workspaces". - {:else} - No workspaces — add one below. - {/if} - + {:else if showAllChips && wsList.length > 3} + + {/if} {/if}
- {#if !isAll && eaWsList.length <= 1} -

- You can't remove the last workspace — switch to "All my workspaces" - first or revoke the connection. -

- {/if} -
- - -
- {#if workspacesError} -

{workspacesError}

+ +
+
+
Connected
+
+ {relativeTime(app.connected_at)} +
+
+
+
Last used
+
+ {relativeTime(app.last_used_at)} +
+
+
+
Activity
+
{callsLabel(app.calls_30d)}
+
+
+ + + + {#if expanded[app.id]} +
+
+ Scopes + {app.scope_string || '—'} +
+
+ Allowed workspaces + + {#if anyWs} + Any workspace + {:else} + {wsList.join(', ')} + {/if} + +
+
+ Redirect URIs +
+ {#if app.redirect_uris && app.redirect_uris.length > 0} + {#each app.redirect_uris as uri (uri)} + {uri} + {/each} + {:else} + — + {/if} +
+
+
{/if}
- {/if} -
- {/each} -
+ +
+ + +
+ + {#if editingId === app.id} + {@const eaWsList = (app.allowed_workspaces ?? []).filter((s) => s !== '*')} + {@const pickable = pickableFor(app)} + {@const isAll = app.all_current_workspaces ?? true} +
+ {#if editErrors[app.id]} +

{editErrors[app.id]}

+ {/if} + +
+ +
+ + +
+ {#if !(app.name ?? '')} +

+ Name your connection so you can tell it apart from other apps. +

+ {/if} +
+ +
+ Scope flags + + + {#if isAll} +
+ +
+

+ Replaces "all" with the workspaces you belong to now. Workspaces you join + later are not included until you add them. +

+ {/if} +
+ + +
+
+ Workspace allow-list + {#if isAll} + Inert while wildcard is on + {/if} +
+
+ {#each eaWsList as ws (ws)} + + {ws} + + + {/each} + {#if eaWsList.length === 0} + + {#if isAll} + No workspaces staged — add one if you plan to switch off + "All my workspaces". + {:else} + No workspaces — add one below. + {/if} + + {/if} +
+ {#if !isAll && eaWsList.length <= 1} +

+ You can't remove the last workspace — switch to "All my workspaces" + first or revoke the connection. +

+ {/if} +
+ + +
+ {#if workspacesError} +

{workspacesError}

+ {/if} +
+
+ {/if} + + {/each} + + {/if} + + {#if appGrants.length > 0} +
+

Installed apps acting as you

+

+ Apps installed in a workspace that you signed in to. They act as you in that workspace + only. +

+
+ {#each appGrants as grant (grant.id)} +
+
+ +
+
+

{grant.app_name}

+ + {grant.access === 'write' ? 'Read and write' : 'Read only'} + +
+

{grant.origin}

+
+
+
Workspace
+
{grant.workspace.name}
+
+
+
Signed in
+
+ {relativeTime(grant.granted_at)} +
+
+
+
+
+
+ +
+
+ {/each} +
+
+ {/if} {/if} @@ -616,7 +678,7 @@ > {#if confirmTarget}
- +

The app will lose access immediately. This can’t be undone.

@@ -876,6 +938,38 @@ flex-shrink: 0; } + /* TASK-3399: installed-app grants section. */ + .grants-section { + margin-top: var(--space-6); + display: flex; + flex-direction: column; + gap: var(--space-2); + } + + .section-title { + margin: 0; + font-size: 1rem; + font-weight: 600; + color: var(--text-primary); + } + + .section-desc, + .grant-origin { + margin: 0; + font-size: 0.85rem; + color: var(--text-muted); + } + + .access-label { + font-size: 0.75rem; + font-weight: 500; + color: var(--text-secondary); + padding: 2px 8px; + border-radius: 999px; + background: var(--bg-tertiary); + border: 1px solid var(--border); + } + /* Modal — surface/backdrop/Escape come from the shared primitive (TASK-2083); this wrapper just restores the inner padding + column layout. */ .revoke-modal { diff --git a/web/src/routes/console/connected-apps/appGrants.svelte.test.ts b/web/src/routes/console/connected-apps/appGrants.svelte.test.ts new file mode 100644 index 000000000..a7616df2e --- /dev/null +++ b/web/src/routes/console/connected-apps/appGrants.svelte.test.ts @@ -0,0 +1,96 @@ +import { describe, it, expect, vi, beforeEach, afterEach } from 'vitest'; +import { flushSync, mount, unmount, tick } from 'svelte'; + +/** + * TASK-3399 (SPEC-6 U5b-1): the Connected apps page lists the delegated grants + * a person gave installed apps on their own, read-only plus revoke (lead + * ruling Q1). The API client is stubbed. + */ +const list = vi.hoisted(() => vi.fn()); +const revoke = vi.hoisted(() => vi.fn()); + +vi.mock('$lib/api/client', () => ({ + api: { + connectedApps: { list: () => list(), revoke: (id: string) => revoke(id) }, + workspaces: { list: vi.fn(async () => []) } + } +})); + +const { default: Page } = await import('./+page.svelte'); + +const grant = { + id: 'req-grant-1', + app_name: 'Support Portal', + origin: 'https://portal.example', + workspace: { slug: 'acme', name: 'Acme' }, + access: 'write', + granted_at: new Date().toISOString() +}; + +let host: HTMLElement; +let app: Record | null = null; + +async function settle() { + flushSync(); + for (let i = 0; i < 4; i++) await Promise.resolve(); + await tick(); + flushSync(); +} + +async function mountWith(body: unknown) { + list.mockResolvedValue(body); + app = mount(Page, { target: host, props: {} }) as Record; + await settle(); +} + +beforeEach(() => { + host = document.createElement('div'); + document.body.appendChild(host); + list.mockReset(); + revoke.mockReset(); +}); + +afterEach(() => { + if (app) unmount(app); + app = null; + host.remove(); +}); + +describe('Connected apps: installed apps acting as you', () => { + it('lists an app grant on its own, with no MCP empty state and no edit controls', async () => { + await mountWith({ items: [], app_grants: [grant] }); + const text = host.textContent ?? ''; + expect(text).toContain('Installed apps acting as you'); + expect(text).toContain('Support Portal'); + expect(text).toContain('Acme'); + expect(text).toContain('Read and write'); + expect(text).not.toContain('No connected apps yet.'); + const section = host.querySelector('[aria-labelledby="app-grants-title"]') ?? host; + const buttons = [...section.querySelectorAll('button')].map((b) => b.textContent?.trim()); + expect(buttons).toEqual(['Revoke']); + }); + + it('shows the empty state only when there is nothing at all', async () => { + await mountWith({ items: [], app_grants: [] }); + expect(host.textContent).toContain('No connected apps yet.'); + expect(host.textContent).not.toContain('Installed apps acting as you'); + }); + + it('revokes a grant through the shared confirm and reloads', async () => { + await mountWith({ items: [], app_grants: [{ ...grant, access: 'read' }] }); + expect(host.textContent).toContain('Read only'); + const btn = [...host.querySelectorAll('button')].find((b) => b.textContent?.trim() === 'Revoke'); + btn!.click(); + await settle(); + expect(document.body.textContent).toContain('Revoke Support Portal?'); + revoke.mockResolvedValue(undefined); + list.mockResolvedValue({ items: [], app_grants: [] }); + const confirm = [...document.body.querySelectorAll('.modal-actions button')].find( + (b) => b.textContent?.trim() === 'Revoke' + ) as HTMLButtonElement; + confirm.click(); + await settle(); + expect(revoke).toHaveBeenCalledWith('req-grant-1'); + expect(list).toHaveBeenCalledTimes(2); + }); +});