Add an OAuth authorization server for external agents - #2415
Draft
MarceloRGonc wants to merge 23 commits into
Draft
Add an OAuth authorization server for external agents#2415MarceloRGonc wants to merge 23 commits into
MarceloRGonc wants to merge 23 commits into
Conversation
Introduces the tables backing an OAuth 2.1 authorization server: signing keys, registered clients, pending authorization requests, authorization codes, refresh tokens, and grants (one per authorized connection). Configuration is validated before any OAuth route is served. Each check exists because the misconfiguration it catches would otherwise leave a server that looks healthy while a guarantee is silently gone — most importantly, an MCP resource whose audience equals the API's, which would let the resource server accept tokens the API issued. Part of OPS-4673.
RFC 6749 error responses, opaque-token generation and timing-safe comparison, PKCE S256 verification, redirect-URI rules, and the resource registry that binds each RFC 8707 resource to a token audience. Redirect URIs are held to the same shape rules whether they arrive at registration or on an authorize request: https or loopback only, no fragment, no userinfo, and bounded length. Loopback matching ignores the port because native clients bind an ephemeral one, so those rules are what stop a presented URI carrying more than the registered one allowed. Project resolution sits behind a factory, following the convention used elsewhere in the security layer, so an edition with real per-project membership supplies its own lookups without the OAuth code changing. Part of OPS-4673.
OAuth-issued tokens are signed with a dedicated RS256 keypair, generated on first boot and published at a real JWKS, so a resource server can verify them locally and neither trust domain's key can forge the other's tokens. Keys are cached, and a stale copy is served through a database outage rather than failing verification of tokens that are still perfectly valid. Single-use credentials are claimed with a conditional UPDATE, so a replay loses the race rather than being detected after the fact. Refresh tokens rotate within a family and a replayed token revokes the family, per OAuth 2.1 — but only after the request has been validated, because revoking on the way in would let one rejected request destroy a working credential and make the client's retry look like an attack. Each authorization becomes its own connection, so a user can connect the same agent more than once and revoke either independently. Revoking cascades to that connection's refresh tokens, since a grant alone would leave the client able to mint access tokens by refreshing. Part of OPS-4673.
extractPrincipal dispatches on the signing algorithm, so OAuth-issued tokens are verified against the OAuth keys and accepted only when their audience is the API. A token minted for the MCP resource server therefore cannot authenticate anywhere in the API, including paths that call extractPrincipal directly such as websockets. Enforcing it at the single verification chokepoint means no route can skip it. The project a token may act on comes from its own required claim, so a credential's authority is fixed for its whole life and cannot be redirected by changing stored state. The claim selects a project rather than granting access to it: the grant, the user and the membership are re-checked on every request, so revoking access takes effect at the next request instead of at token expiry. Only a verdict about the token becomes a 401. A server-side failure propagates, because reporting it as an invalid credential makes OAuth clients discard their refresh token and re-authorize — turning a brief outage into a re-consent storm. An Authorization header now takes precedence over the session cookie: a caller presenting a token is stating which identity it wants to act as. Part of OPS-4673.
Registration, authorization, consent decision, token, revocation and connected-app endpoints, plus RFC 8414 discovery served with a real JWKS and no fields the server cannot honour. Everything is registered only when OPS_OAUTH_ENABLED is set, so the routes do not exist by default. Authorize validation lives in its own module and returns a discriminated result, so 'never redirect to an address we have not validated' is a property of the type rather than of reviewer diligence: an unknown client or unregistered redirect_uri renders an error, and only a validated destination can receive one. Request parameters are read as unknown and checked, never cast. A parameter that is present but not a string is rejected rather than replaced by a default, since a form parser turns nested keys into objects and substituting a default would give a client something other than what it asked for. An hourly job removes expired codes, pending requests and refresh tokens, stale registrations, and connections with no usable credential left. Part of OPS-4673.
Unit tests use in-memory repositories, so the guarantees that depend on the database cannot fail there: an inverted date predicate or a non-atomic claim looks identical. These tests run the same code against a real ORM and real SQL — concurrent redemption of one code, one pending record and one refresh token; revocation cascading to a single connection; and the cleanup job's deletes. Writing them found a defect the mocks could not: cutoffs bound as ISO strings are compared textually by drivers that store a different textual format, so every row matched, including future ones. Cutoffs are now bound as Date objects. Part of OPS-4673.
The design, a runbook for exercising the flow locally, and a script that walks discovery through revocation while printing the token claims at each step. There is no consent page yet, so the script stands in for it by calling the decision endpoint directly. Part of OPS-4673.
The default-disabled assertion read the ambient environment, so it failed for any developer whose .env enables OAuth — and passed in CI only because CI has no .env. Drive both settings through the mock instead, and assert the TTL getters read their own properties rather than whatever the machine is configured with. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The rewritten MCP server takes its allow-list as a file and reads the OpenAPI document from the API itself, so writing a pre-filtered schema no longer means anything to it. Write INCLUDED_PATHS out in the shape it expects instead, which keeps this the only place the chat's exposed surface is declared, and rename the spawn variables to the ones it now reads. Operations the running API does not serve are left out. The old schema filter dropped them implicitly; doing it deliberately matters more now, because the MCP server refuses to start on an operation it cannot find — so one stale entry would cost every tool rather than just itself. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
An MCP client completing discovery and registration opened the browser and landed on a URL the frontend did not route, which was the last gap in the flow. Add the screen the authorize endpoint already redirects to. It names the application and the project the connection will be bound to, so the user can see whose data they are handing over — the project is resolved from the same membership lookup that binds it when the code is redeemed. The screen is authenticated but renders without the application chrome: a sidebar invites the user to wander off mid-flow, and the pending authorization expires if they do. api.post now takes headers, mirroring api.put, because the decision endpoint requires the anti-CSRF header. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Consent and connection management belong in the same place: the user decides there, and returns there to see what they granted and cut any of it off. So the authorize endpoint now redirects to Settings -> Connected apps with the request id, and the consent screen is a dialog over that page rather than a standalone route. Each row is one authorization rather than one application, because that is what is independently revocable — connecting the same agent twice produces two rows, and disconnecting one leaves the other working. Disconnecting asks first, since it takes effect immediately. Dismissing the dialog denies rather than doing nothing. The application is waiting on its redirect, and telling it no beats leaving it to time out. A CONNECTED_APPS_ENABLED flag hides the page when OAuth is off, where every route it depends on is unregistered. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The project binding rests entirely on the principal type. ProjectAuthzHandler rejects a request naming a project other than the principal's, but enterprise's /switch-project is on that handler's ignore list — minting a token for another project is its purpose. What keeps an OAuth connection out of it is that the route allows only PrincipalType.USER while an OAuth token yields SERVICE. That invariant was asserted but unexplained, which is how it gets changed by someone being helpful. Say what it protects, at the assertion and in the design doc, along with the two changes that would quietly make project_id decorative. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
An agent should be able to move between projects the way its user can in the browser. The project claim was already a selector rather than a grant of authority — every request re-authorizes it — so this opens the missing half: a client may name where it wants to act when getting a token. Three ways in. refresh_token with project_id moves a direct API client. The token-exchange grant with project_id moves a resource server on an agent's behalf, which is the path an MCP client takes since it cannot mint tokens itself. And GET /v1/oauth/projects tells a connection where it may go and where it is now, allowing SERVICE so the connection itself can ask. Membership is the bound, re-read on every mint and again on every request, so a switch can never reach further than the user could. A project they are not a member of is invalid_target, and on the refresh path that refusal lands before the token is consumed — asking for the wrong project must not cost a working credential. listForUser joins the seam, and deliberately answers with the same rule as getForUser: listing less than the token endpoint permits would show a client one destination while allowing another it was never told about. The consent screen now says a connection can act in any project the user has access to, because it can. Showing only the starting project implied a fence that is not there. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Naming one project made sense while a connection was confined to it. Now that it can act wherever its user can, the row states a fact and the bullet beneath immediately says that fact is not a limit — and a reader scanning a consent screen will take a named project as a limit. Removing it leaves one honest line about projects instead of two that argue. With nothing displaying them, the projectId and projectName fields on the consent response and the describeTargetProject helper behind them are dead, so they go too. Redemption already refuses with a precise reason when a user has no reachable project, which was the only enforcement that helper contributed. consent-details.ts is left holding just the project listing, so it is renamed for what it does. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The engine resolves its code cache with path.resolve('cache','codes'), relative to the
working directory, so running a server from inside packages/server/api writes one there.
Two files from it were committed by a broad git add — node_modules was already ignored,
which is why only those two slipped through.
The existing /cache rule is anchored to the repository root and never covered it. The new
pattern needs the **/ prefix for the same reason: a pattern with an interior slash is
anchored to the directory holding .gitignore. Scoped to cache/codes/ rather than any
directory named cache, because packages/server/shared has real source in one.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Several passages still described the state before the last few changes, and a design doc that contradicts the code is worse than none — a reader trusts it. The largest was decision 5 and deviation 5, which between them said a token may act only on its project and that the OSS server deliberately builds no switching. Both describe the intermediate position, not the shipped one. Deviation 5 now records the reversal and why: /switch-project is unreachable for an OAuth connection, so declining to build a mechanism removed the capability rather than deferring it to enterprise. Also corrected: the token-exchange entry still promised the resource server cannot widen what it was given; refresh did not mention project_id; /oauth/projects was missing from the endpoint list; the consent UI section described a standalone route; audit row M5 attributed the fix to a fixed project rather than to there being no mutable state; the phase list read as outstanding work; and project switching sat under Out of scope. The M5 note now warns that the one unbuilt piece — the resource server's switch tool — is exactly where that finding came from, since a process-local map is the obvious implementation and the wrong one. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Each was written on every row and never consulted for a decision: - oauth_client.scope — a client may send one at registration, but what a token gets is settled by the resource it names, checked at /authorize. Storing the request was a second answer nothing read, and echoing it in the RFC 7591 response implied an enforcement that does not exist. The 128-character validation went with it: refusing a field we ignore is worse than ignoring it quietly. - oauth_grant.scope — restated resourceId, since each resource grants exactly one. It was also carried through the cached grant snapshot, unread. - oauth_refresh_token.userId — copied forward on every rotation while the grant remained authoritative, so the column could only ever disagree. Kept oauth_grant.revokedAt, which is also write-only: it is an audit answer to when, and status alone cannot give that. resourceId stayed too, because it records something not derivable from anything else — whether the connection came through the MCP server or the API. It is now shown on the connected-apps row, which also gives otherwise identical rows something to tell them apart. projectId left the grants response instead: a connection can move between projects, so reporting where it started would mislead. Verified by dropping the schema and letting the migration rebuild it, then running the full flow and the project-switch checks against the result. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Answering whether projectId belongs on oauth_grant turned up a bug. It was read in one place — the default when a refresh names no project — and that default was wrong. A connection that switched to another project and then renewed normally was silently put back where it started, roughly fifteen minutes later, with nothing in the request to attribute it to. The project is a property of the credential chain, not of the authorization, so it now lives on oauth_refresh_token. Rotation carries it forward unless the client asks to move, which makes a plain renewal hand back an equivalent credential — the thing a renewal is supposed to be. That leaves nothing reading the grant's copy, so the column is gone along with it, as is the projectId that had crept onto the grant snapshot. Two mocks were hiding this. tokens.service.test stubbed getForUser with a fixed membership, so every caller looked correct regardless of which project it passed; it now echoes the project it is asked about, as the real service does. The first version of the regression test then made the same mistake locally and passed against the bug. Verified by dropping the schema, letting the migration rebuild it, and driving a switch followed by a plain refresh against a second project. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Disconnect is destructive, so it now uses the destructive variant like the other destructive actions in the app. The confirmation it opens was still primary blue, which read oddly for the button that actually does the work, so ConfirmationDialog gained an optional confirmButtonVariant. It defaults to the previous behaviour, leaving the risky-flow dialog untouched. The row now follows the enterprise integrations card: a p-6 bordered box, a 48px icon square standing in for the product logo a self-registered client does not have, gap-6, a semibold title and a full-size button. One class did not survive the copy. text-primary-900 exists in the enterprise Tailwind scale but not this one, which stops at 800, so it resolved to nothing and the secondary line inherited the title colour — measured as identical near-black. Using text-muted-foreground, the convention here, restores the distinction. Dark theme could not be checked visually because DARK_THEME_ENABLED is false locally, but every colour used is a theme token. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The bordered rows already separate themselves from the description, so the rule was doing no work — and the integrations card this page follows has none either. Removing it left the Separator import unused, so that goes too. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The heading was an h3 at text-lg with a small muted description, which read as a section label rather than the page title it is. It now follows routes/settings/ai: an h1 at 24px bold with a text-base description, measured identical to that page. The sibling pages tag these with text-primary-900, which is not defined in this Tailwind scale or as a CSS variable — three files use it and it resolves to nothing. Left off here rather than copied, since it changes no pixel today. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
Pull request overview
Adds an OAuth 2.1 authorization server to the server-api (PKCE S256, JWKS/RS256 signing, token exchange, revocation, cleanup), plus a “Connected apps” UI surface in react-ui, and updates the MCP stdio integration to pass an allow-list file instead of a filtered OpenAPI schema.
Changes:
- Implement OAuth authorization server + persistence (TypeORM entities + Postgres migration), signing key management, and request authentication via RS256/audience enforcement.
- Add “Settings → Connected apps” page + consent dialog and related frontend API/hooks/tests, gated by a new
CONNECTED_APPS_ENABLEDflag. - Update MCP stdio spawn to write/pass an allow-list route file (
OPENOPS_MCP_ROUTES) and adjust related tests/tooling.
Blocking
- A repeatable
oauth-cleanupsystem job can remain scheduled in Redis after OAuth is later disabled, but the job handler won’t be registered (OAuth module isn’t loaded), causingNo handler for job oauth-cleanupfailures and retries.
Non-blocking
- None
Merge recommendation
- Do not merge
Reviewed changes
Copilot reviewed 78 out of 79 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| tools/oauth-flow.sh | Local script to walk the OAuth flow end-to-end (api/mcp modes). |
| packages/ui-components/src/components/confirmation-dialog/confirmation-dialog.tsx | Adds configurable confirm button variant for destructive confirmations. |
| packages/shared/src/lib/flag/flag.ts | Adds CONNECTED_APPS_ENABLED flag id. |
| packages/server/shared/src/lib/system/system.ts | Adds default values for OAuth-related system props (enabled + TTLs). |
| packages/server/shared/src/lib/system/system-prop.ts | Adds OAuth/MCP related AppSystemProp entries. |
| packages/server/api/test/unit/oauth/signing-key.service.test.ts | Unit tests for key generation, encryption-at-rest, JWKS publishing, and token verification. |
| packages/server/api/test/unit/oauth/resource-registry.test.ts | Tests for registered resources and scope listing. |
| packages/server/api/test/unit/oauth/redirect-uri.test.ts | Tests redirect URI registrability and strict matching rules (incl. loopback). |
| packages/server/api/test/unit/oauth/project-membership.test.ts | Tests OAuth project membership seam behavior for OSS edition. |
| packages/server/api/test/unit/oauth/pkce.test.ts | Tests PKCE verifier/challenge validation (S256-only). |
| packages/server/api/test/unit/oauth/pending-authorization.service.test.ts | Tests pending authorization single-use semantics and cleanup behavior. |
| packages/server/api/test/unit/oauth/oauth-principal.test.ts | Tests OAuth token → SERVICE principal extraction and enforcement boundaries. |
| packages/server/api/test/unit/oauth/oauth-metadata.test.ts | Tests RFC 8414/OIDC-shaped discovery metadata correctness. |
| packages/server/api/test/unit/oauth/oauth-errors.test.ts | Tests OAuth error object mapping to RFC6749 response bodies/status codes. |
| packages/server/api/test/unit/oauth/oauth-crypto.test.ts | Tests token generation, hashing, and timing-safe compares. |
| packages/server/api/test/unit/oauth/oauth-config.test.ts | Tests config normalization and default-disabled behavior. |
| packages/server/api/test/unit/oauth/oauth-config-validation.test.ts | Tests startup validation for issuer/resource/audience separation and DB constraints. |
| packages/server/api/test/unit/oauth/oauth-cleanup-job.test.ts | Tests cleanup job semantics and Date-bound cutoffs. |
| packages/server/api/test/unit/oauth/grants.service.test.ts | Tests per-authorization grants, revocation cascade, caching, and touch throttling. |
| packages/server/api/test/unit/oauth/available-projects.test.ts | Tests listing projects available for switching. |
| packages/server/api/test/unit/oauth/authorize-validation.test.ts | Tests /authorize query validation incl. open-redirect boundary behavior. |
| packages/server/api/test/unit/ai/openops-tools.test.ts | Updates MCP allow-list writing tests (routes file + env var changes). |
| packages/server/api/src/app/oauth/token-exchange.ts | Implements RFC 8693 token exchange for MCP RS → API-audience token minting. |
| packages/server/api/src/app/oauth/signing-key.service.ts | Implements RS256 signing key generation/loading, JWKS, and verification with kid. |
| packages/server/api/src/app/oauth/service-principal.ts | Builds SERVICE principals from verified OAuth token claims with per-request re-authorization. |
| packages/server/api/src/app/oauth/resource-registry.ts | Defines supported OAuth resources (api/mcp) and scope mapping. |
| packages/server/api/src/app/oauth/redirect-uri.ts | Redirect URI validation/matching rules incl. loopback port variance. |
| packages/server/api/src/app/oauth/project-membership.ts | OSS implementation of OAuth project membership seam. |
| packages/server/api/src/app/oauth/project-membership-factory.ts | Factory seam for edition-specific membership implementations. |
| packages/server/api/src/app/oauth/pkce.ts | PKCE S256 verifier/challenge validation helper. |
| packages/server/api/src/app/oauth/pending-authorization.service.ts | Stores/consumes pending authorizations (single-use) and deletes expired records safely. |
| packages/server/api/src/app/oauth/oauth.module.ts | Registers OAuth routes, custom OAuth error handler, signing key init, RS client, cleanup job. |
| packages/server/api/src/app/oauth/oauth.entity.ts | Adds TypeORM entity schemas and indexes for OAuth tables. |
| packages/server/api/src/app/oauth/oauth-well-known.controller.ts | Serves discovery documents + JWKS publicly with caching. |
| packages/server/api/src/app/oauth/oauth-query.ts | Provides Date-bound “earlierThan” helper to avoid textual timestamp comparisons. |
| packages/server/api/src/app/oauth/oauth-model.ts | Adds OAuth model types + access token claim shape. |
| packages/server/api/src/app/oauth/oauth-metadata.ts | Builds RFC 8414 metadata + issuer-path-aware well-known variants. |
| packages/server/api/src/app/oauth/oauth-errors.ts | Defines RFC 6749 error response types and helpers. |
| packages/server/api/src/app/oauth/oauth-crypto.ts | Token entropy generation + SHA-256 helpers + timing-safe compare. |
| packages/server/api/src/app/oauth/oauth-config.ts | OAuth-related config getters (issuer, TTLs, RS secret, signing key path). |
| packages/server/api/src/app/oauth/oauth-config-validation.ts | Validates OAuth configuration invariants at startup. |
| packages/server/api/src/app/oauth/oauth-cleanup-job.ts | Adds scheduled cleanup job for expired OAuth state + dead grants + unused clients. |
| packages/server/api/src/app/oauth/grants.service.ts | Implements per-authorization grants with revocation, caching, and last-used throttling. |
| packages/server/api/src/app/oauth/clients.service.ts | Implements dynamic registration + RS confidential client + Basic auth parsing/verification. |
| packages/server/api/src/app/oauth/available-projects.ts | Lists switchable projects for a user via membership + display name lookup. |
| packages/server/api/src/app/oauth/authorize-validation.ts | Validates /authorize inputs and returns safe redirect/render outcomes. |
| packages/server/api/src/app/helper/system-jobs/common.ts | Adds SystemJobName.OAUTH_CLEANUP and typed data mapping. |
| packages/server/api/src/app/flags/flag.service.ts | Adds backend-driven CONNECTED_APPS_ENABLED flag based on OAuth enablement. |
| packages/server/api/src/app/database/postgres-connection.ts | Registers OAuth migration in Postgres connection. |
| packages/server/api/src/app/database/migrations/1785312000000-CreateOAuthTables.ts | Adds OAuth tables + indexes + FKs in Postgres. |
| packages/server/api/src/app/database/database-connection.ts | Registers OAuth entities in TypeORM connection entity list. |
| packages/server/api/src/app/core/security/authn/access-token-authn-handler.ts | Prioritizes explicit Authorization header over session cookie. |
| packages/server/api/src/app/authentication/context/access-token-manager.ts | Dispatches RS256 tokens to OAuth verification/principal path; preserves HS256 internal flow. |
| packages/server/api/src/app/app.ts | Conditionally registers OAuth module when enabled. |
| packages/server/api/src/app/ai/mcp/openops-tools.ts | Writes MCP allow-list routes file; updates env var names passed to stdio MCP server. |
| packages/react-ui/src/app/routes/settings/connected-apps/index.tsx | Adds route entrypoint export for Connected apps page. |
| packages/react-ui/src/app/routes/settings/connected-apps/connected-apps-page.tsx | Implements Connected apps page, consent dialog, revoke confirmation, and error states. |
| packages/react-ui/src/app/router.tsx | Adds lazy route for /settings/connected-apps. |
| packages/react-ui/src/app/lib/api.ts | Adds optional headers parameter to api.post and merges it into request headers. |
| packages/react-ui/src/app/features/oauth/lib/tests/oauth-api.test.ts | Tests OAuth frontend API wrapper behavior (incl. consent header). |
| packages/react-ui/src/app/features/oauth/lib/oauth-api.ts | Adds OAuth frontend API wrapper for consent/grants endpoints. |
| packages/react-ui/src/app/features/oauth/hooks/use-oauth-consent.ts | Hook for loading a pending request and submitting approve/deny decision. |
| packages/react-ui/src/app/features/oauth/hooks/use-connected-apps.ts | Hook for listing and revoking connected apps with react-query invalidation. |
| packages/react-ui/src/app/features/oauth/hooks/tests/use-oauth-consent.test.tsx | Hook tests for consent flow, retries disabled, and navigation. |
| packages/react-ui/src/app/features/oauth/hooks/tests/use-connected-apps.test.tsx | Hook tests for listing, revoking, and refetch behavior. |
| packages/react-ui/src/app/features/oauth/components/consent-dialog.tsx | Consent dialog UI describing authorization impact and deny-on-dismiss behavior. |
| packages/react-ui/src/app/features/oauth/components/connected-apps-list.tsx | UI list of connected apps with revoke action and empty state. |
| packages/react-ui/src/app/constants/query-keys.ts | Adds query keys for OAuth consent request and connected apps list. |
| packages/react-ui/src/app/common/components/project-settings-layout.tsx | Adds Connected apps nav item gated by CONNECTED_APPS_ENABLED flag. |
| docs/oauth-manual-testing.md | Runbook for manual/local OAuth testing (script + real client). |
| .gitignore | Ignores engine code cache directory written under package working dirs. |
| .env.template | Adds OAuth-related environment variables and defaults. |
Contributor
Author
Claude PR Review — #2415: Add an OAuth authorization server for external agentsAuthor: MarceloRGonc | Base: VerdictSHIP — no blockers or risks survived validation; the two remaining items are a latent i18n note and a documentation question. SummaryAdds an OAuth 2.1 authorization server (PKCE S256-only, RS256/JWKS in a separate trust domain, refresh rotation with family reuse detection, RFC 8693 token exchange) behind Findings[NOTE] [QUESTION] Testing gaps
Verified acceptable (checked, deliberately not raised)
|
Three review comments on #2415, all correct. Copilot found a real bug. registerOAuthCleanupJob both registered the handler and upserted the repeatable job, and ran only when OAuth was enabled. The schedule lives in Redis and outlives the boot that created it, so an instance that enabled OAuth once and later turned it off kept firing a job with no handler — the BullMQ processor throws from getJobHandler and the job fails hourly. Handler registration is now unconditional and returns early while disabled; scheduling stays on the enabled path. A test asserts the disabled handler touches no repository, and fails if the guard is removed. Retention for revoked refresh tokens was an independent 7-day window against a 30-day refresh TTL, which the reviewer asked about. It was not a considered choice, and their alternative is better: anchoring to expiry means a rotated token is kept exactly as long as it could still be presented, so a replay is recognised as reuse — revoking the family and logging it — instead of coming back as a plain invalid token because it was old. That deletes a constant and a query rather than adding a comment defending the window. The nav-item titles called t() at module scope, which the reviewer flagged as a new instance of OPS-4318. A production chunk can evaluate before i18n.init(), and t() returns undefined until then, freezing a blank label into a top-level constant. Building the items inside the component removes all four hits in that file, so the audit script counts 17 files where it previously counted 18. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes OPS-4673.
Adds an OAuth 2.1 authorization server to the Node API so external agents — Claude Code, Codex, Claude.ai/ChatGPT connectors, M365 Copilot, partner CLIs — can hold a self-service, revocable credential instead of sharing one static
AUTH_TOKEN. Works when SSO is on and password login is disabled.Off by default: every
/v1/oauth/*route is unregistered unlessOPS_OAUTH_ENABLED=true, so this is inert until an operator turns it on.Design and rationale:
docs/oauth-design.md. Local runbook:docs/oauth-manual-testing.md.What's here
Authorization server (
packages/server/api/src/app/oauth/) — PKCE S256-only, exact redirect matching, refresh rotation with family reuse detection, RFC 8707 resource indicators, RFC 8693 token exchange, RFC 7009 revocation, RFC 9207iss, RFC 7591 dynamic registration, RFC 8414 + OIDC-shaped discovery, RFC 9728 protected-resource metadata.A dedicated RS256 keypair with a real JWKS and
kiddispatch, generated on first boot and stored encrypted. Deliberately isolated from the pre-existing HS256OPS_JWT_SECRETtrust domain, so an OAuth token can never be confused with a session token.API enforcement —
extractPrincipaldispatches on the signing algorithm and enforces the audience positively: a token minted for the MCP resource cannot authenticate anywhere in the API, including the websocket path. OAuth principals areSERVICE, neverUSER.Per-connection grants — one row per completed authorization, so connecting the same agent twice gives two independently revocable connections. Revocation cascades to that connection's refresh tokens only.
Project as a selector, not a grant of authority — every token carries a required
project_id, re-authorized against membership on every request. A connection can move between the user's projects by naming one when it gets a token (project_idon the refresh and token-exchange grants); it can never reach further than the user could in the browser.UI — consent as a dialog over a new Settings → Connected apps page, which is also where connections are reviewed and disconnected. Hidden behind a
CONNECTED_APPS_ENABLEDflag when OAuth is off.AI chat compatibility — the stdio spawn now passes the tool allow-list as a file (
OPENOPS_MCP_ROUTES), matching the rewritten MCP server inopenops-mcp.INCLUDED_PATHSstays the single source of truth for what the chat exposes.Additional Notes
This replaces the
feat/mcp-oauth-authenticationspike inopenops-internal. It is a fresh implementation informed by an adversarial security audit of that spike;docs/oauth-design.mdlists the findings (H1–H4, M1–M8, L1–L6) and where each is addressed. Highlights:request_id.jwks_urithat served nothing.Deviations from the design, and cases where the design was wrong, are recorded in the doc rather than quietly resolved — including one reversal: an intermediate decision to build no project switching at all, which turned out to remove the capability rather than defer it to enterprise, because
/switch-projectis unreachable for aSERVICEprincipal.Two behaviours are load-bearing and pinned by tests, because a plausible-looking change to either would silently make
project_iddecorative:SERVICE(test/unit/oauth/oauth-principal.test.ts)test/unit/oauth/tokens.service.test.ts)Follow-ups, not in this PR: the resource server's project-switch tool and deployment packaging both live in
openops-mcp.docs/oauth-design.mdnotes that whatever holds the switch selection must not be a process-local map — that is where audit finding M5 came from, andstateless_httpwould make it diverge across replicas.Testing Checklist
Automated: 894 server-api unit tests (293 for OAuth), 17 OAuth integration tests that initialise a real database, 504 react-ui, 197 ui-components, 149 shared. Lint and typecheck clean; all affected projects build.
The integration tests exist because the unit tests use in-memory repositories, which cannot show the guarantees that depend on database semantics: that single-use consumption of codes, pending records and refresh tokens is atomic under concurrency, that revocation cascades to one connection only, and that the cleanup job deletes what expired and nothing else. Writing them found a real defect — date cutoffs bound as ISO strings are compared textually by drivers that store a different textual format, so a cleanup pass was deleting live rows.
Manual, end to end against a running instance and a real MCP client:
mcp-audience token refused by the API (401) and only usable after exchange — the no-token-passthrough ruleinvalid_targetfor a project the user is not a member ofBackwards compatibility: additive migration, no existing table or column is altered. The MCP spawn variables were renamed, and the rewritten server accepts the previous names as fallbacks, so the two repositories can roll out independently.
Visual Changes
Two new surfaces, both under Settings → Connected apps:
request_id. Names the application, lists what it will be able to do — including that it can act in any project the user has access to — and warns against continuing unless the user started the flow themselves. Cancel and dismissal both deny, so the waiting application is always told the outcome.