diff --git a/crates/engram-coordinator/src/oauth_redirect.rs b/crates/engram-coordinator/src/oauth_redirect.rs index 86ce747e1..21ab32765 100644 --- a/crates/engram-coordinator/src/oauth_redirect.rs +++ b/crates/engram-coordinator/src/oauth_redirect.rs @@ -640,7 +640,11 @@ impl OAuthManager { .redirect(reqwest::redirect::Policy::none()) .build() .map_err(|e| driver_err("http_client", e.to_string()))?; - let request = http.post(token_url); + // RFC 6749 answers in JSON, but some providers (GitHub) answer the + // token request in form encoding unless the client asks for JSON. + let request = http + .post(token_url) + .header(reqwest::header::ACCEPT, "application/json"); let request = match body { TokenRequestBody::Form => request.form(form), TokenRequestBody::Json => { @@ -688,7 +692,9 @@ impl OAuthManager { format!("no access token under {grant_path:?}") } }); - let code = if err == "invalid_grant" { + // GitHub answers a dead refresh token with HTTP 200 and its own + // `bad_refresh_token` code, not the RFC's `invalid_grant`. + let code = if err == "invalid_grant" || err == "bad_refresh_token" { "invalid_grant" } else { "exchange_failed" diff --git a/crates/engram-coordinator/tests/oauth_redirect.rs b/crates/engram-coordinator/tests/oauth_redirect.rs index 882e6f2bf..82f6e1f59 100644 --- a/crates/engram-coordinator/tests/oauth_redirect.rs +++ b/crates/engram-coordinator/tests/oauth_redirect.rs @@ -7,6 +7,7 @@ use std::collections::BTreeMap; use std::sync::Arc; use axum::extract::State; +use axum::http::HeaderMap; use axum::routing::{get, post}; use axum::{Json, Router}; use engram_coordinator::oauth::OAuthManager; @@ -28,6 +29,8 @@ use serde_json::{json, Value}; struct Provider { token_response: Arc>, token_requests: Arc>>>, + /// The `Accept` header of each token request. + token_accepts: Arc>>, viewer_response: Arc>, } @@ -35,13 +38,20 @@ async fn serve(provider: Provider) -> String { let router = Router::new() .route( "/oauth/token", - post(|State(p): State, body: String| async move { - let form: BTreeMap = url::form_urlencoded::parse(body.as_bytes()) - .map(|(k, v)| (k.into_owned(), v.into_owned())) - .collect(); - p.token_requests.lock().push(form); - Json(p.token_response.lock().clone()) - }), + post( + |State(p): State, headers: HeaderMap, body: String| async move { + let accept = headers.get("accept").and_then(|v| v.to_str().ok()); + p.token_accepts + .lock() + .push(accept.unwrap_or_default().to_owned()); + let form: BTreeMap = + url::form_urlencoded::parse(body.as_bytes()) + .map(|(k, v)| (k.into_owned(), v.into_owned())) + .collect(); + p.token_requests.lock().push(form); + Json(p.token_response.lock().clone()) + }, + ), ) .route( "/graphql", @@ -258,6 +268,11 @@ async fn complete_publishes_bundle_with_rotation_and_probe_metadata() { assert_eq!(reqs[0]["client_id"], "client-123"); assert_eq!(reqs[0]["client_secret"], "secret-456"); assert_eq!(reqs[0]["redirect_uri"], "https://cb.example.com/x"); + // GitHub answers in form encoding unless the request asks for JSON. + assert_eq!( + *fx.provider.token_accepts.lock(), + vec!["application/json".to_owned()] + ); // Probe metadata extracted; expiry mirrored outside the ciphertext. assert_eq!(row.metadata.account_id, "app-user-1"); @@ -746,6 +761,33 @@ async fn invalid_grant_breaks_the_credential_and_reconnect_repairs_it() { assert_eq!(resolved.secret, "at-3"); } +#[tokio::test] +async fn github_bad_refresh_token_breaks_the_credential() { + let fx = fixture().await; + let sp = spec(&fx.host, false); + connected(&fx, &sp).await; + + // GitHub's shape: HTTP 200, its own error code. + *fx.provider.token_response.lock() = json!({ + "error": "bad_refresh_token", + "error_description": "The refresh token passed is incorrect or expired.", + }); + fx.clock.advance(std::time::Duration::from_secs(19 * 3600)); + let sweep = fx + .manager + .run_connector_refresh_once() + .await + .expect("sweep"); + assert_eq!(sweep.broke, 1, "{sweep:?}"); + let row = fx + .meta + .get_oauth_credential(&linear_key()) + .await + .expect("get") + .expect("row"); + assert_eq!(row.broken_reason.as_deref(), Some("invalid_grant")); +} + #[tokio::test] async fn non_refreshable_expiring_token_breaks_at_expiry_not_before() { let fx = fixture().await; diff --git a/crates/engram-git-github/src/lib.rs b/crates/engram-git-github/src/lib.rs index a7efc7b72..f087a033d 100644 --- a/crates/engram-git-github/src/lib.rs +++ b/crates/engram-git-github/src/lib.rs @@ -560,7 +560,9 @@ async fn ensure_ok( /// ID + RSA private-key PEM. An admin supplies both via the integrations UI (the /// PEM sealed in the org-secret store); the coordinator resolves the two fields /// (org secrets `github_app.app_id` / `github_app.private_key_pem`, name = -/// `.`) and calls `build` to construct the engine lazily. This is +/// `.`) and calls `build` to construct the engine lazily. The +/// optional `client_id` / `client_secret` are the App's OAuth client, which +/// the GitHub connector's personal OAuth flow reads. This is /// the data-driven frame around minting — the mint logic stays in [`GitHubApp`]. pub fn github_app_descriptor() -> MintKindDescriptor { MintKindDescriptor { @@ -580,6 +582,21 @@ pub fn github_app_descriptor() -> MintKindDescriptor { field_kind: MintFieldKind::SealedSecret, required: true, }, + // The App's own OAuth client. Optional: with both set, members + // can connect their personal GitHub account through the App + // instead of pasting a token. Minting does not use them. + MintFieldSchema { + name: "client_id", + label: "Client ID (optional, for personal connect)", + field_kind: MintFieldKind::Config, + required: false, + }, + MintFieldSchema { + name: "client_secret", + label: "Client secret (optional, for personal connect)", + field_kind: MintFieldKind::SealedSecret, + required: false, + }, ], build: |fields: &ResolvedFields| { let app_id = fields.get("app_id").ok_or_else(|| { diff --git a/crates/engram-git-github/tests/github.rs b/crates/engram-git-github/tests/github.rs index bc63018b4..87837ab32 100644 --- a/crates/engram-git-github/tests/github.rs +++ b/crates/engram-git-github/tests/github.rs @@ -121,7 +121,10 @@ fn github_app_descriptor_metadata() { // The field names are the org-secret suffixes the coordinator resolves; the // Plane-A form (C4) renders from this metadata, so pin the contract. let names: Vec<&str> = d.fields.iter().map(|f| f.name).collect(); - assert_eq!(names, vec!["app_id", "private_key_pem"]); + assert_eq!( + names, + vec!["app_id", "private_key_pem", "client_id", "client_secret"] + ); let pem = d .fields .iter() @@ -131,6 +134,18 @@ fn github_app_descriptor_metadata() { assert!(pem.required); let app_id = d.fields.iter().find(|f| f.name == "app_id").unwrap(); assert_eq!(app_id.field_kind, MintFieldKind::Config); + // The App's OAuth client serves personal connect only; minting works + // without it. + let client_secret = d.fields.iter().find(|f| f.name == "client_secret").unwrap(); + assert_eq!(client_secret.field_kind, MintFieldKind::SealedSecret); + assert!(!client_secret.required); + assert!( + !d.fields + .iter() + .find(|f| f.name == "client_id") + .unwrap() + .required + ); } #[test] diff --git a/orchestrator/src/__tests__/connectors.test.ts b/orchestrator/src/__tests__/connectors.test.ts index 4ec63db56..2a2d5dd1b 100644 --- a/orchestrator/src/__tests__/connectors.test.ts +++ b/orchestrator/src/__tests__/connectors.test.ts @@ -767,7 +767,17 @@ describe("on-disk registry", () => { expect(userScopes).toContain("channels:write"); expect(userScopes).toContain("chat:write"); expect(reg.get("sentry")!.userCredential?.token?.hint).toContain("User Auth Tokens"); - expect(reg.get("github")!.userCredential?.oauth).toBeUndefined(); + // GitHub's personal OAuth is the App's own user flow: it exchanges the + // code on github.com and names the account by its login. + expect(reg.get("github")!.userCredential?.oauth).toBe(true); + expect(reg.get("github")!.userCredential?.oauthFacet).toMatchObject({ + authorizeUrl: "https://github.com/login/oauth/authorize", + tokenUrl: "https://github.com/login/oauth/access_token", + scopes: [], + clientIdRef: "github_app.client_id", + clientSecretRef: "github_app.client_secret", + metadata: { probe: { path: "/user", map: { accountId: "id", displayName: "login" } } }, + }); expect(reg.get("github")!.userCredential?.token?.identity).toEqual({ path: "/user", map: { displayName: "login" }, @@ -1495,16 +1505,60 @@ describe("user-scoped credentials (ADR 0115)", () => { expect(() => parseConnector(noInject, "t")).toThrow(/"inject" is required on a mint connector/); }); - test("rejects oauth mode on a mint connector", () => { - const withOauth = { - ...acmeMintRaw, - userCredential: { - oauth: true, - token: { hint: "x" }, - inject: { header: "Authorization", template: "Bearer {}" }, + // A mint connector has no org OAuth facet, so its personal flow is a + // complete authorization-code flow of its own (GitHub's App user flow). + const mintFlow = { + authorizeUrl: "https://acmemint.test/login/oauth/authorize", + acquisitionHosts: ["acmemint.test"], + tokenUrl: "https://acmemint.test/login/oauth/access_token", + clientIdRef: "acme_app.client_id", + clientSecretRef: "acme_app.client_secret", + }; + const withMintOauth = (oauth: unknown) => ({ + ...acmeMintRaw, + userCredential: { + oauth, + token: { hint: "x" }, + inject: { header: "Authorization", template: "Bearer {}" }, + }, + }); + + test("accepts a mint connector's own personal OAuth flow", () => { + const c = parseConnector(withMintOauth(mintFlow), "acmemint"); + expect(c.userCredential?.oauth).toBe(true); + // The token URL may sit on an acquisition host, and no scopes is fine. + expect(c.userCredential?.oauthFacet).toMatchObject({ + grant: "authorization_code", + tokenUrl: "https://acmemint.test/login/oauth/access_token", + scopes: [], + clientIdRef: "acme_app.client_id", + }); + }); + + test("rejects a mint connector's oauth that is not a flow of its own", () => { + expect(() => parseConnector(withMintOauth(true), "t")).toThrow(/must declare its own flow/); + expect(() => + parseConnector(withMintOauth({ ...mintFlow, grant: "client_credentials" }), "t"), + ).toThrow(/"authorization_code" for a personal flow/); + expect(() => + parseConnector( + withMintOauth({ ...mintFlow, tokenUrl: "https://elsewhere.test/token" }), + "t", + ), + ).toThrow(/"oauth.tokenUrl" host "elsewhere.test"/); + }); + + test("an org OAuth facet keeps its token URL on the connector's hosts", () => { + // The relaxation is for a personal flow only. + const org = { + ...acmeRaw, + credential: { + source: "inject", + injects: [{ header: "Authorization", template: "Bearer {}" }], }, + oauth: { ...mintFlow, scopes: ["read"], tokenUrl: "https://acmemint.test/token" }, }; - expect(() => parseConnector(withOauth, "t")).toThrow(/token" mode only/); + expect(() => parseConnector(org, "t")).toThrow(/"oauth.tokenUrl" host/); }); test("rejects oauth mode without the top-level oauth facet", () => { diff --git a/orchestrator/src/__tests__/me-oauth.test.ts b/orchestrator/src/__tests__/me-oauth.test.ts index 21182fa5c..4ac8a8a62 100644 --- a/orchestrator/src/__tests__/me-oauth.test.ts +++ b/orchestrator/src/__tests__/me-oauth.test.ts @@ -27,10 +27,16 @@ const acmeConnector = { userCredential: { token: { hint: "Create a PAT." } }, }; +/** The org secret names the fake coordinator holds. */ +let orgSecretNames: string[] = []; + function appWith(oauth: OAuthCredentialClient) { return makeMeRoute({ secrets, oauth, + orgSecrets: { + listSecrets: async () => ({ secrets: orgSecretNames.map((name) => ({ name })) }), + }, getSession: async () => ({ user: { id: "user-42", role: "user" } }), connectors: { list: async () => [{ provider: "acme", config: acmeConnector }] }, harnessCatalog: { @@ -127,7 +133,10 @@ describe("/me OAuth credentials", () => { }); describe("/me connector credentials (ADR 0115)", () => { - beforeEach(() => invalidateRegistry()); + beforeEach(() => { + invalidateRegistry(); + orgSecretNames = []; + }); function fakeOauth() { const calls: { @@ -222,6 +231,23 @@ describe("/me connector credentials (ADR 0115)", () => { ); }); + test("offers OAuth connect only when the app's client id and secret are set", async () => { + const { oauth } = fakeOauth(); + const githubModes = async () => { + const listed = await appWith(oauth).request("/api/v1/me/credentials"); + const body = (await listed.json()) as { credentials: Array> }; + return body.credentials.find( + (entry) => entry.kind === "connector" && entry.provider === "github", + )?.modes; + }; + // Without the GitHub App's OAuth client, a Connect button would only fail. + expect(await githubModes()).toEqual({ oauth: false, token: true }); + orgSecretNames = ["github_app.client_id"]; + expect(await githubModes()).toEqual({ oauth: false, token: true }); + orgSecretNames = ["github_app.client_id", "github_app.client_secret"]; + expect(await githubModes()).toEqual({ oauth: true, token: true }); + }); + test("PUT 404s for a provider without token mode and 400s an empty value", async () => { const { oauth, calls } = fakeOauth(); const app = appWith(oauth); diff --git a/orchestrator/src/__tests__/slack-integration.test.ts b/orchestrator/src/__tests__/slack-integration.test.ts index c03b2013b..a2e83771f 100644 --- a/orchestrator/src/__tests__/slack-integration.test.ts +++ b/orchestrator/src/__tests__/slack-integration.test.ts @@ -654,6 +654,39 @@ describe("integration oauth route", () => { }); }); + test("a mint connector's own personal flow: GitHub authorize and callback", async () => { + const { client, calls } = fakeRedirectClient({ + flowSubject: { kind: OauthSubjectKind.USER_CONNECTOR, id: "u2" }, + }); + const app = makeIntegrationOauthRoute({ + connectors: emptySource, + oauthCredential: client as never, + connectionIdFor, + getSession: memberSession, + }); + const begun = await app.request("/api/v1/me/connector-credentials/github/oauth/authorize"); + expect(begun.status).toBe(302); + const spec = calls.begin[0]!.spec!; + expect(calls.begin[0]!.subject).toMatchObject({ kind: OauthSubjectKind.USER_CONNECTOR, id: "u2" }); + expect(spec.authorizeUrl).toBe("https://github.com/login/oauth/authorize"); + expect(spec.tokenUrl).toBe("https://github.com/login/oauth/access_token"); + expect(spec.scopes).toEqual([]); + expect(spec.clientIdRef).toBe("github_app.client_id"); + expect(spec.metadata?.probe).toMatchObject({ + host: "api.github.com", + path: "/user", + map: { account_id: "id", display_name: "login" }, + }); + + const done = await app.request( + "/api/v1/integrations/github/oauth/callback?code=abc&state=10600000-0000-4000-8000-0000000000aa", + ); + expect(done.status).toBe(302); + expect(done.headers.get("location")).toBe("/settings/credentials?connected=github"); + // The completion uses the same spec the flow began with. + expect(calls.complete[0]!.spec?.tokenUrl).toBe("https://github.com/login/oauth/access_token"); + }); + test("callback: a user-subject flow is fenced to its owner", async () => { const { client, calls } = fakeRedirectClient({ flowSubject: { kind: OauthSubjectKind.USER_CONNECTOR, id: "someone-else" }, diff --git a/orchestrator/src/connectors/github.json b/orchestrator/src/connectors/github.json index 3116b80d5..afd56be72 100644 --- a/orchestrator/src/connectors/github.json +++ b/orchestrator/src/connectors/github.json @@ -17,6 +17,24 @@ } }, "userCredential": { + "oauth": { + "authorizeUrl": "https://github.com/login/oauth/authorize", + "acquisitionHosts": [ + "github.com" + ], + "tokenUrl": "https://github.com/login/oauth/access_token", + "clientIdRef": "github_app.client_id", + "clientSecretRef": "github_app.client_secret", + "metadata": { + "probe": { + "path": "/user", + "map": { + "accountId": "id", + "displayName": "login" + } + } + } + }, "token": { "hint": "Create a fine-grained personal access token under GitHub Settings \u2192 Developer settings, with the permissions you want sessions to hold.", "identity": { diff --git a/orchestrator/src/connectors/oauth-spec.ts b/orchestrator/src/connectors/oauth-spec.ts index 6c39620f8..81018a047 100644 --- a/orchestrator/src/connectors/oauth-spec.ts +++ b/orchestrator/src/connectors/oauth-spec.ts @@ -7,7 +7,12 @@ */ import { create } from "@bufbuild/protobuf"; -import type { Connector, OauthMetadataField, OauthMetadataProbe } from "./registry.ts"; +import type { + Connector, + OauthFacet, + OauthMetadataField, + OauthMetadataProbe, +} from "./registry.ts"; import { ClientCredentialsSpecSchema, RedirectMetadataProbeSchema, @@ -58,8 +63,10 @@ export function userTokenIdentityProbe(connector: Connector): RedirectMetadataPr /** Build the wire spec for a connector's oauth facet. `extraParams` lets a * caller append flow extras (e.g. `prompt: "consent"` on a reconnect); facet * params win on collision so a connector's declared behavior is stable. */ -function wireMetadata(connector: Connector): RedirectMetadataSpec | undefined { - const oauth = connector.oauth; +function wireMetadata( + connector: Connector, + oauth: OauthFacet | undefined = connector.oauth, +): RedirectMetadataSpec | undefined { if (!oauth?.metadata) return undefined; return create(RedirectMetadataSpecSchema, { fromTokenResponse: wireMap(oauth.metadata.fromTokenResponse), @@ -111,10 +118,18 @@ export function redirectOauthSpec( ): RedirectOauthSpec { const oauth = connector.oauth; if (!oauth) throw new Error(`connector "${connector.provider}" has no oauth facet`); + return redirectSpecFromFacet(connector, oauth, extraParams); +} + +function redirectSpecFromFacet( + connector: Connector, + oauth: OauthFacet, + extraParams?: Record, +): RedirectOauthSpec { if (oauth.grant === "client_credentials" || oauth.authorizeUrl === undefined) { throw new Error(`connector "${connector.provider}" has no authorization-code flow`); } - const metadata = wireMetadata(connector); + const metadata = wireMetadata(connector, oauth); return create(RedirectOauthSpecSchema, { authorizeUrl: oauth.authorizeUrl, tokenUrl: oauth.tokenUrl, @@ -141,6 +156,10 @@ export function userRedirectOauthSpec( connector: Connector, extraParams?: Record, ): RedirectOauthSpec { + // A mint connector's personal flow is its own facet (GitHub: the App's + // user authorization); nothing org-side is inherited, so it applies as is. + const own = connector.userCredential?.oauthFacet; + if (own) return redirectSpecFromFacet(connector, own, extraParams); const oauth = connector.oauth; const mode = connector.userCredential?.oauth; if (!oauth || mode === undefined) { diff --git a/orchestrator/src/connectors/registry.ts b/orchestrator/src/connectors/registry.ts index 985deb4ca..830eeb840 100644 --- a/orchestrator/src/connectors/registry.ts +++ b/orchestrator/src/connectors/registry.ts @@ -325,7 +325,9 @@ export interface OauthMetadataProbe { * into the launching user's PERSONAL credential for human sessions. * * - `oauth: true` reuses the top-level {@link OauthFacet} (same BYO app, same - * scopes) with a user subject in the sealed store. + * scopes) with a user subject in the sealed store. A mint connector has no + * top-level facet, so its `oauth` is a complete authorization-code flow of + * its own (GitHub: the App's user authorization). * - `token` accepts a pasted personal access token; `hint` is the setup line * shown in Settings → Credentials. * - `inject` is required for MINT connectors only (the mint engine renders @@ -358,8 +360,12 @@ export interface UserOauthOverrides { export interface UserCredentialFacet { /** `true` reuses the facet as-is (minus its authorize extras); an object - * overrides the user flow's shape. */ + * overrides the user flow's shape. On a mint connector the JSON object is + * the flow itself, parsed into `oauthFacet`, and this is `true`. */ oauth?: true | UserOauthOverrides; + /** A mint connector's own personal OAuth flow (GitHub: the App's user + * authorization). Mint connectors have no org OAuth facet to reuse. */ + oauthFacet?: OauthFacet; token?: UserTokenFacet; inject?: { header: string; template: string }; } @@ -1303,9 +1309,27 @@ function parseMetadataProbe( * admin-authored connector can't ship it elsewhere); `authorizeUrl` may instead * sit on the facet's own `acquisitionHosts` — a browser-redirect surface that is * never compiled into the session egress policy. */ -function parseOauth(where: string, raw: unknown, hosts: string[]): OauthFacet { +function parseOauth( + where: string, + raw: unknown, + hosts: string[], + personal = false, +): OauthFacet { if (typeof raw !== "object" || raw === null) fail(where, '"oauth" must be an object'); const o0 = raw as Record; + // A personal flow of its own (a mint connector has no org OAuth facet to + // reuse) is a member's consent screen: authorization code only, with no + // org-side secrets. Its token URL may sit on an acquisition host — GitHub + // exchanges codes on github.com, and putting github.com into `hosts` would + // open it to session injection. + if (personal) { + if (o0.grant !== undefined && o0.grant !== "authorization_code") { + fail(where, '"oauth.grant" must be "authorization_code" for a personal flow'); + } + for (const field of ["signingSecretRef", "signingSecretHint", "serviceAccountKeyRef"]) { + if (o0[field] !== undefined) fail(where, `"oauth.${field}" does not apply to a personal flow`); + } + } let grant: OauthFacet["grant"] = "authorization_code"; if (o0.grant !== undefined) { if ( @@ -1383,9 +1407,16 @@ function parseOauth(where: string, raw: unknown, hosts: string[]): OauthFacet { ? undefined : httpsUrlOn("authorizeUrl", o.authorizeUrl, [...hosts, ...(acquisitionHosts ?? [])]); // Strict: the token exchange carries the client secret. - const tokenUrl = httpsUrlOn("tokenUrl", o.tokenUrl, hosts); + const tokenUrl = httpsUrlOn( + "tokenUrl", + o.tokenUrl, + personal ? [...hosts, ...(acquisitionHosts ?? [])] : hosts, + ); - const scopes = asStringArray(`${where} oauth`, "scopes", o.scopes); + // A personal flow may name no scopes: a GitHub App's user token carries the + // App's permissions, and GitHub ignores the scope parameter. + const scopes = + personal && o.scopes === undefined ? [] : asStringArray(`${where} oauth`, "scopes", o.scopes); if (scopes.length > MAX_OAUTH_SCOPES) fail(where, `"oauth.scopes" has ${scopes.length} entries (max ${MAX_OAUTH_SCOPES})`); for (const s of scopes) { if (/[\s\r\n]/.test(s)) fail(where, `"oauth.scopes" entry "${s}" must not contain whitespace`); @@ -2257,9 +2288,10 @@ export function parseConnector( // - at least one mode (oauth / token); // - inject connectors need exactly ONE header (the user value renders // through it) and must NOT carry a `userCredential.inject`; - // - mint connectors are token-only and MUST carry `userCredential.inject` - // (the mint engine renders the org header, so user mode needs its own); - // - `oauth: true` requires the top-level oauth facet. + // - mint connectors MUST carry `userCredential.inject` (the mint engine + // renders the org header, so user mode needs its own), and their + // `oauth` is a full personal flow of its own; + // - `oauth: true` on an inject connector requires the top-level facet. let userCredential: UserCredentialFacet | undefined; if (o.userCredential !== undefined) { const uw = `${where} userCredential`; @@ -2268,7 +2300,17 @@ export function parseConnector( } const uc = o.userCredential as Record; let userOauth: true | UserOauthOverrides | undefined; - if (uc.oauth !== undefined) { + let userOauthFacet: OauthFacet | undefined; + if (uc.oauth !== undefined && credential.source === "mint") { + if (typeof uc.oauth !== "object" || uc.oauth === null || Array.isArray(uc.oauth)) { + fail( + uw, + '"oauth" on a mint connector must declare its own flow (authorizeUrl, tokenUrl, client refs): there is no org OAuth facet to reuse', + ); + } + userOauthFacet = parseOauth(uw, uc.oauth, hosts, true); + userOauth = true; + } else if (uc.oauth !== undefined) { if (uc.oauth === true) { userOauth = true; } else if (typeof uc.oauth === "object" && uc.oauth !== null && !Array.isArray(uc.oauth)) { @@ -2426,15 +2468,13 @@ export function parseConnector( fail(uw, '"inject" is only for mint connectors (inject connectors render the user value through their existing header)'); } } else { - if (userOauth !== undefined) { - fail(uw, 'mint connectors support "token" mode only (user-to-server OAuth is not implemented)'); - } if (userInject === undefined) { fail(uw, '"inject" is required on a mint connector (the mint engine renders the org header, so user mode needs its own header spec)'); } } userCredential = { ...(userOauth !== undefined ? { oauth: userOauth } : {}), + ...(userOauthFacet !== undefined ? { oauthFacet: userOauthFacet } : {}), ...(token ? { token } : {}), ...(userInject ? { inject: userInject } : {}), }; diff --git a/orchestrator/src/routes/integration-oauth.ts b/orchestrator/src/routes/integration-oauth.ts index a03c69cf6..309446511 100644 --- a/orchestrator/src/routes/integration-oauth.ts +++ b/orchestrator/src/routes/integration-oauth.ts @@ -112,6 +112,20 @@ export function makeIntegrationOauthRoute(deps?: IntegrationOauthDeps): Hono { return conn; } + /** A connector a member can connect with their own OAuth consent: through + * the org facet (Linear, Slack) or through a personal flow of its own on a + * mint connector (GitHub). */ + async function userOauthConnector(provider: string): Promise { + const registry = await loadRegistry(connectorSource()); + const conn = registry.get(provider); + if (conn?.userCredential?.oauth === undefined) { + throw new HTTPException(404, { + message: `connector "${provider}" has no user-scoped OAuth flow`, + }); + } + return conn; + } + async function subjectFor(connector: Connector) { const id = await connectionIdFor( connector.provider, @@ -162,12 +176,7 @@ export function makeIntegrationOauthRoute(deps?: IntegrationOauthDeps): Hono { app.get("/api/v1/me/connector-credentials/:provider/oauth/authorize", async (c) => { const user = await requireUser(c.req.raw.headers); const provider = c.req.param("provider"); - const connector = await oauthConnector(provider); - if (connector.userCredential?.oauth === undefined) { - throw new HTTPException(404, { - message: `connector "${provider}" has no user-scoped OAuth flow`, - }); - } + const connector = await userOauthConnector(provider); const force = c.req.query("force") === "1"; const { authorizeUrl } = await oauthCredential.beginRedirectFlow({ subject: { kind: OauthSubjectKind.USER_CONNECTOR, id: user.id }, @@ -267,7 +276,9 @@ export function makeIntegrationOauthRoute(deps?: IntegrationOauthDeps): Hono { await requireAdmin(c.req.raw.headers); } - const connector = await oauthConnector(provider); + const connector = isUserFlow + ? await userOauthConnector(provider) + : await oauthConnector(provider); const done = isUserFlow ? finishUser : finish; try { await oauthCredential.completeRedirectFlow({ diff --git a/orchestrator/src/routes/me.ts b/orchestrator/src/routes/me.ts index c7d20f877..2422f8c40 100644 --- a/orchestrator/src/routes/me.ts +++ b/orchestrator/src/routes/me.ts @@ -30,7 +30,10 @@ import { Hono } from "hono"; import { HTTPException } from "hono/http-exception"; import { makeUserSecretStore, type UserSecretStore } from "../db/user-secrets.ts"; import { harnessCatalog as defaultHarnessCatalog } from "../control-plane/client.ts"; -import { oauthCredential as defaultOAuthCredential } from "../control-plane/client.ts"; +import { + oauthCredential as defaultOAuthCredential, + orgSecret as defaultOrgSecret, +} from "../control-plane/client.ts"; import { OauthSubjectKind, type RedirectMetadataProbe } from "../gen/engram/app/v1/oauth_pb.ts"; import { userTokenIdentityProbe } from "../connectors/oauth-spec.ts"; import { Code, ConnectError } from "@connectrpc/connect"; @@ -84,6 +87,13 @@ export interface MeDeps { oauth?: OAuthCredentialClient; /** ADR 0115: custom-connector source for the user-credential surface. */ connectors?: CustomConnectorSource; + /** The org secret names, to offer OAuth connect only when the app's client + * id and secret are set. */ + orgSecrets?: OrgSecretNames; +} + +export interface OrgSecretNames { + listSecrets(req: Record): Promise<{ secrets: Array<{ name: string }> }>; } export interface OAuthCredentialClient { @@ -139,6 +149,9 @@ interface UserConnectorEntry { tokenHint?: string; /** Names the account behind a pasted token, when the connector declares it. */ identity?: RedirectMetadataProbe; + /** The org secrets the personal OAuth flow needs (the app's client id and + * secret), when the connector has one. */ + oauthClientRefs?: string[]; } // --------------------------------------------------------------------------- @@ -155,6 +168,18 @@ export function makeMeRoute(deps?: MeDeps): Hono { deps?.harnessCatalog ?? (defaultHarnessCatalog as unknown as HarnessCatalogReader); const resolveOAuth = (): OAuthCredentialClient => deps?.oauth ?? defaultOAuthCredential; + const resolveOrgSecrets = (): OrgSecretNames => deps?.orgSecrets ?? defaultOrgSecret; + + /** The org secret names, or undefined when the listing fails. */ + async function orgSecretNames(): Promise | undefined> { + try { + const listed = await resolveOrgSecrets().listSecrets({}); + return new Set(listed.secrets.map((secret) => secret.name)); + } catch (error) { + console.error(`me: org-secret listing unavailable — ${(error as Error).message}`); + return undefined; + } + } const resolveSession: GetSession = deps?.getSession ?? @@ -225,12 +250,16 @@ export function makeMeRoute(deps?: MeDeps): Hono { for (const connector of registry.values()) { const uc = connector.userCredential; if (!uc) continue; + const oauthFacet = uc.oauth === undefined ? undefined : (uc.oauthFacet ?? connector.oauth); union.set(connector.provider, { name: connector.display.name, oauth: uc.oauth !== undefined, token: uc.token !== undefined, ...(uc.token ? { tokenHint: uc.token.hint } : {}), identity: userTokenIdentityProbe(connector), + ...(oauthFacet + ? { oauthClientRefs: [oauthFacet.clientIdRef, oauthFacet.clientSecretRef] } + : {}), }); } return union; @@ -293,13 +322,15 @@ export function makeMeRoute(deps?: MeDeps): Hono { // existing sealed-value flow; OAuth entries expose metadata and lifecycle. app.get("/api/v1/me/credentials", async (c) => { const user = await requireUser(c.req.raw.headers); - const [envVars, providers, connectors, oauthRows, connectorRows] = await Promise.all([ - userEnvUnion(), - oauthUnion(), - connectorUnion(), - resolveOAuth().listCredentials({ subject: oauthSubject(user.id) }), - resolveOAuth().listCredentials({ subject: connectorSubject(user.id) }), - ]); + const [envVars, providers, connectors, oauthRows, connectorRows, secretNames] = + await Promise.all([ + userEnvUnion(), + oauthUnion(), + connectorUnion(), + resolveOAuth().listCredentials({ subject: oauthSubject(user.id) }), + resolveOAuth().listCredentials({ subject: connectorSubject(user.id) }), + orgSecretNames(), + ]); const byProvider = new Map(oauthRows.credentials.map((row) => [row.provider, row])); const connectorByProvider = new Map( connectorRows.credentials.map((row) => [row.provider, row]), @@ -308,11 +339,18 @@ export function makeMeRoute(deps?: MeDeps): Hono { // against the caller's personal credential row (PAT or OAuth — one slot). const connectorEntries = [...connectors.entries()].map(([provider, entry]) => { const row = connectorByProvider.get(provider); + // OAuth connect needs the app's client id and secret in the org store; + // without them the button would only fail. A failed listing keeps the + // button, and the flow reports the real error. + const oauthReady = + entry.oauth && + (secretNames === undefined || + (entry.oauthClientRefs ?? []).every((ref) => secretNames.has(ref))); return { kind: "connector" as const, provider, display: { name: entry.name }, - modes: { oauth: entry.oauth, token: entry.token }, + modes: { oauth: oauthReady, token: entry.token }, ...(entry.tokenHint ? { tokenHint: entry.tokenHint } : {}), connected: row?.connected ?? false, status: row?.status ?? "", diff --git a/site/src/content/docs/docs/guides/connect-github.md b/site/src/content/docs/docs/guides/connect-github.md index e636000ab..110009fef 100644 --- a/site/src/content/docs/docs/guides/connect-github.md +++ b/site/src/content/docs/docs/guides/connect-github.md @@ -12,7 +12,7 @@ pull request review. ## Create the App -Settings → Integrations → GitHub walks through four steps. +Settings → Integrations → GitHub walks through four steps, and an optional fifth. 1. **Register the App.** Either open GitHub's new-app page and fill it in, or copy the manifest from the panel and use GitHub's "register from a manifest" flow. The manifest @@ -30,7 +30,13 @@ Settings → Integrations → GitHub walks through four steps. owns the repositories, generate a private key, and paste the numeric App ID and the downloaded key into the connect sheet. -The App ID and the key are stored as org secrets; the key is sealed and never returned. +5. **Let people connect their own account (optional).** Set the App's callback URL to + `/api/v1/integrations/github/oauth/callback`; the manifest already sets it. Generate + a client secret, and paste the App's client ID and the client secret into the optional + fields of the connect sheet. + +The App ID and the key are stored as org secrets; the key is sealed and never returned. The +client secret is sealed in the same way. ## What sessions get @@ -58,10 +64,18 @@ To enroll a repository, open Settings → Automations → PR review → Inputs a The handle defaults to the App's own login. The other inputs are the reviewer profile, the review categories, and free-text instructions the reviewers read. The Reviews page lists -every reviewed pull request with its findings and what was posted. +every reviewed pull request with its findings and what was posted. Select **Mine** to see +only the pull requests you opened. It matches the GitHub login of your personal GitHub +credential. ## Personal GitHub credentials -A person can add a fine-grained personal access token under Settings → Credentials. A -profile set to act as the launching person uses it instead of the App token, so commits and -pull requests carry that person's identity. +A person connects their own GitHub account under Settings → Credentials. A profile set to act +as the launching person uses it instead of the App token, so commits and pull requests carry +that person's identity. There are two ways to connect: + +- **Connect** authorizes the App to act as you. It is available when an admin completed step + 5. The token has the App's permissions, limited to what you can access, and engrams renews + it. +- **Add token** saves a fine-grained personal access token. engrams asks GitHub for the + token's login when you save it, and does not save a token that GitHub refuses. diff --git a/web/src/components/integrations/GithubSetupPanel.tsx b/web/src/components/integrations/GithubSetupPanel.tsx index c37f1c6cd..eb7f917b7 100644 --- a/web/src/components/integrations/GithubSetupPanel.tsx +++ b/web/src/components/integrations/GithubSetupPanel.tsx @@ -21,6 +21,7 @@ import { CopyIcon, KeyRoundIcon, ShieldCheckIcon, + UserRoundIcon, WebhookIcon, } from "lucide-react"; @@ -29,6 +30,7 @@ import { Text } from "@/components/ui/text"; import { WEBHOOK_EVENTS, buildGithubManifest, + githubOAuthCallbackUrl, githubWebhookUrl, isMinimumPermission, permissionLabel, @@ -40,14 +42,15 @@ import type { ConnectorView } from "./useConnectorViews"; const NEW_APP_URL = "https://github.com/settings/apps/new"; export function GithubSetupPanel({ view }: { view: ConnectorView }) { - const [copied, setCopied] = useState<"url" | "manifest" | null>(null); + const [copied, setCopied] = useState<"url" | "callback" | "manifest" | null>(null); const origin = API_ORIGIN || (typeof window !== "undefined" ? window.location.origin : ""); const permissions = permissionsForCapabilities(view.capabilities); const webhookUrl = githubWebhookUrl(origin); + const callbackUrl = githubOAuthCallbackUrl(origin); const manifest = buildGithubManifest({ origin, permissions, events: WEBHOOK_EVENTS }); - const copy = (what: "url" | "manifest", text: string) => { + const copy = (what: "url" | "callback" | "manifest", text: string) => { if (typeof navigator === "undefined" || !navigator.clipboard) return; void navigator.clipboard.writeText(text).then(() => { setCopied(what); @@ -200,6 +203,42 @@ export function GithubSetupPanel({ view }: { view: ConnectorView }) {

+ + {/* Step 5 — optional personal connect */} +
+ +

+ So members can connect GitHub in Settings → Credentials instead of pasting a token, set + the App's Callback URL to this URL. Then generate a + client secret, and copy the Client ID and the{" "} + client secret into the optional fields below. The + manifest already sets the callback URL. +

+
+ + + Callback URL + + + {callbackUrl} + +
+ +
+
+
); } diff --git a/web/src/lib/githubSetup.test.ts b/web/src/lib/githubSetup.test.ts index 8881a4944..be94a2273 100644 --- a/web/src/lib/githubSetup.test.ts +++ b/web/src/lib/githubSetup.test.ts @@ -93,6 +93,12 @@ describe("buildGithubManifest", () => { expect(m.hook_attributes.active).toBe(true); }); + test("registers the OAuth callback members connect through", () => { + expect(parse().callback_urls).toEqual([ + "https://engrams.example.com/api/v1/integrations/github/oauth/callback", + ]); + }); + test("carries the derived permissions as default_permissions", () => { const m = parse(); expect(m.default_permissions.metadata).toBe("read"); diff --git a/web/src/lib/githubSetup.ts b/web/src/lib/githubSetup.ts index 8f3c3a142..6edb9e84a 100644 --- a/web/src/lib/githubSetup.ts +++ b/web/src/lib/githubSetup.ts @@ -102,6 +102,15 @@ export function githubWebhookUrl(origin: string): string { return `${origin}/api/v1/integrations/github/events`; } +/** + * Where GitHub sends a member back after they authorize the App to act as + * them — the orchestrator's shared OAuth callback. It is the App's + * "Callback URL", and it must match exactly. + */ +export function githubOAuthCallbackUrl(origin: string): string { + return `${origin}/api/v1/integrations/github/oauth/callback`; +} + /** The engrams App as it appears in the target org. */ const APP_NAME = "engrams"; @@ -130,6 +139,8 @@ export function buildGithubManifest({ origin, permissions, events }: GithubManif url: origin, hook_attributes: { url: githubWebhookUrl(origin), active: true }, redirect_url: `${origin}/settings/integrations/github`, + // Members can connect their own GitHub account through the App. + callback_urls: [githubOAuthCallbackUrl(origin)], public: false, default_permissions, default_events: events,