diff --git a/apps/api/src/integration-platform/controllers/oauth.controller.spec.ts b/apps/api/src/integration-platform/controllers/oauth.controller.spec.ts index fda90c376a..2f2134e1ae 100644 --- a/apps/api/src/integration-platform/controllers/oauth.controller.spec.ts +++ b/apps/api/src/integration-platform/controllers/oauth.controller.spec.ts @@ -614,13 +614,33 @@ describe('OAuthController', () => { config: { authorizeUrl: 'https://github.com/login/oauth/authorize', tokenUrl: 'https://github.com/login/oauth/access_token', + installUrl: + 'https://github.com/apps/comp-ai-compliance/installations/new', }, }, capabilities: [], isActive: true, }; - it('blocks the GitHub App connect when the App is not installed', async () => { + const notInstalledFetch = () => + jest + .spyOn(global, 'fetch') + // 1) token exchange + .mockResolvedValueOnce({ + ok: true, + status: 200, + json: () => Promise.resolve({ access_token: 'gh_token' }), + text: () => Promise.resolve(''), + } as unknown as Response) + // 2) GET /user/installations -> none + .mockResolvedValueOnce({ + ok: true, + status: 200, + json: () => Promise.resolve({ total_count: 0, installations: [] }), + text: () => Promise.resolve(''), + } as unknown as Response); + + it('redirects to the install URL when the App is not installed (first pass)', async () => { const futureDate = new Date(Date.now() + 600000); mockOAuthStateRepository.findByState.mockResolvedValue({ state: 'gh_state', @@ -638,23 +658,60 @@ describe('OAuthController', () => { scopes: [], source: 'platform', }); + mockOAuthStateRepository.create.mockResolvedValue({ + state: 'install_state', + }); - const fetchSpy = jest - .spyOn(global, 'fetch') - // 1) token exchange - .mockResolvedValueOnce({ - ok: true, - status: 200, - json: () => Promise.resolve({ access_token: 'gh_token' }), - text: () => Promise.resolve(''), - } as unknown as Response) - // 2) GET /user/installations -> none - .mockResolvedValueOnce({ - ok: true, - status: 200, - json: () => Promise.resolve({ total_count: 0, installations: [] }), - text: () => Promise.resolve(''), - } as unknown as Response); + const fetchSpy = notInstalledFetch(); + + await controller.oauthCallback( + { code: 'auth_code', state: 'gh_state' }, + mockRequest, + mockResponse, + ); + + // No installation yet → send them to install, don't finalize. + expect(mockConnectionService.activateConnection).not.toHaveBeenCalled(); + const redirectUrl = (mockResponse.redirect as jest.Mock).mock.calls[0][0]; + expect(redirectUrl).toContain( + 'https://github.com/apps/comp-ai-compliance/installations/new', + ); + expect(redirectUrl).toContain('state=install_state'); + + // The install-handoff state must be created BEFORE the original state is + // deleted, so a create failure can't leave the outer catch double-deleting + // an already-removed record (which would hang the request). + const createOrder = + mockOAuthStateRepository.create.mock.invocationCallOrder[0]; + const deleteOrder = + mockOAuthStateRepository.delete.mock.invocationCallOrder[0]; + expect(createOrder).toBeLessThan(deleteOrder); + + fetchSpy.mockRestore(); + }); + + it('still emits an error redirect if creating the install-handoff state fails', async () => { + const futureDate = new Date(Date.now() + 600000); + mockOAuthStateRepository.findByState.mockResolvedValue({ + state: 'gh_state', + providerSlug: 'github-app', + organizationId: 'org_1', + userId: 'user_1', + codeVerifier: null, + redirectUrl: null, + expiresAt: futureDate, + }); + mockedGetManifest.mockReturnValue(githubAppManifest as never); + mockOAuthCredentialsService.getCredentials.mockResolvedValue({ + clientId: 'c', + clientSecret: 's', + scopes: [], + source: 'platform', + }); + // Creating the install-handoff state fails (e.g. DB error). + mockOAuthStateRepository.create.mockRejectedValue(new Error('db down')); + + const fetchSpy = notInstalledFetch(); await controller.oauthCallback( { code: 'auth_code', state: 'gh_state' }, @@ -662,7 +719,44 @@ describe('OAuthController', () => { mockResponse, ); - // The connection must NOT be activated when the App isn't installed. + // The original state was NOT deleted before the failed create, so the + // outer catch can still clean it up and emit an error redirect (no hang). + expect(mockOAuthStateRepository.delete).toHaveBeenCalledWith('gh_state'); + const errRedirect = (mockResponse.redirect as jest.Mock).mock.calls[0][0]; + expect(errRedirect).toContain('error='); + expect(mockConnectionService.activateConnection).not.toHaveBeenCalled(); + + fetchSpy.mockRestore(); + }); + + it('errors (no loop) when still not installed after an install attempt', async () => { + const futureDate = new Date(Date.now() + 600000); + mockOAuthStateRepository.findByState.mockResolvedValue({ + state: 'gh_state', + providerSlug: 'github-app', + organizationId: 'org_1', + userId: 'user_1', + codeVerifier: null, + redirectUrl: null, + expiresAt: futureDate, + }); + mockedGetManifest.mockReturnValue(githubAppManifest as never); + mockOAuthCredentialsService.getCredentials.mockResolvedValue({ + clientId: 'c', + clientSecret: 's', + scopes: [], + source: 'platform', + }); + + const fetchSpy = notInstalledFetch(); + + // installation_id present → we already came back from an install attempt. + await controller.oauthCallback( + { code: 'auth_code', state: 'gh_state', installation_id: '999' }, + mockRequest, + mockResponse, + ); + expect(mockConnectionService.activateConnection).not.toHaveBeenCalled(); const redirectUrl = (mockResponse.redirect as jest.Mock).mock.calls[0][0]; expect(redirectUrl).toContain('error=github_app_not_installed'); diff --git a/apps/api/src/integration-platform/controllers/oauth.controller.ts b/apps/api/src/integration-platform/controllers/oauth.controller.ts index 0b7673d86d..bd86e206a9 100644 --- a/apps/api/src/integration-platform/controllers/oauth.controller.ts +++ b/apps/api/src/integration-platform/controllers/oauth.controller.ts @@ -39,6 +39,9 @@ interface OAuthCallbackQuery { state: string; error?: string; error_description?: string; + // Present when GitHub returns from an App installation (install-time OAuth). + // Used only as a loop guard for the install redirect — never persisted. + installation_id?: string; } @Controller({ path: 'integrations/oauth', version: '1' }) @@ -335,26 +338,53 @@ export class OAuthController { oauthState.codeVerifier, ); - // GitHub App: a user can complete OAuth *without* installing the App, - // which yields a token that cannot read any repositories. If GitHub - // definitively reports no installation for this user, stop here and tell - // them to install the App instead of creating a "connected" but unusable - // integration. Fails open on any uncertainty so a valid connection is - // never blocked by a transient error. + // GitHub App: user *authorization* and app *installation* are separate on + // GitHub. A user can complete OAuth without installing the App, producing a + // token that can read no repositories. If GitHub definitively reports no + // installation, don't finalize a useless connection — send the user to + // install the App (choose org + repositories). They return through + // install-time OAuth (with an installation_id) and this check then passes. + // Fails open on any uncertainty so a valid connection is never blocked by a + // transient error. if ( oauthState.providerSlug === 'github-app' && (await this.githubAppInstallationMissing(tokens.access_token)) ) { + // First pass (came from plain authorize, no installation_id): send the + // user to install the App, then they return here via install-time OAuth. + if (!query.installation_id && oauthConfig.installUrl) { + // Create the install-handoff state BEFORE deleting the original one. + // If creation fails, the original state survives so the outer catch can + // clean it up and still emit an error redirect — deleting first would + // make the catch's delete throw (record gone) and hang the request. + const installState = await this.oauthStateRepository.create({ + providerSlug: oauthState.providerSlug, + organizationId: oauthState.organizationId, + userId: oauthState.userId, + redirectUrl: oauthState.redirectUrl ?? undefined, + }); + await this.oauthStateRepository.delete(state); + const installRedirect = new URL(oauthConfig.installUrl); + installRedirect.searchParams.set('state', installState.state); + this.logger.log( + `GitHub App authorized but not installed; redirecting org ${oauthState.organizationId} to install`, + ); + res.redirect(installRedirect.toString()); + return; + } + + // We already came back from an install attempt but still see no + // installation — stop rather than loop. await this.oauthStateRepository.delete(state); this.logger.warn( - `GitHub App authorized but not installed for org ${oauthState.organizationId}`, + `GitHub App still not installed after install attempt for org ${oauthState.organizationId}`, ); const errorUrl = this.buildRedirectUrl( oauthState.redirectUrl, { error: 'github_app_not_installed', error_description: - 'You authorized Comp AI, but the GitHub App is not installed on your organization yet. Install the App on GitHub, then reconnect.', + 'The GitHub App was not installed on your organization. Please install it (selecting at least one repository) and try again.', }, oauthState.organizationId, ); diff --git a/packages/integration-platform/src/manifests/github-app/index.ts b/packages/integration-platform/src/manifests/github-app/index.ts index dba88a881a..dcf826b5e0 100644 --- a/packages/integration-platform/src/manifests/github-app/index.ts +++ b/packages/integration-platform/src/manifests/github-app/index.ts @@ -15,19 +15,21 @@ * is left completely untouched so current connections keep working. New/concerned * customers opt into this one. * - * Connect flow (user-to-server token via the standard OAuth authorize URL): + * Connect flow (user-to-server token — GitHub separates user *authorization* + * from app *installation*, so the callback bridges the two): * 1. The customer is sent to `https://github.com/login/oauth/authorize` with - * the App's client id. First-time users get an "Install & Authorize" screen - * (install the App + pick repositories); already-installed users just - * authorize. Either way GitHub returns to our callback with a `code`. - * 2. The shared OAuth callback exchanges the `code` for a user-to-server access - * token (read-only, capped by the App's permissions) and stores it like any - * other OAuth token. - * - * We deliberately use the authorize URL rather than the App's install URL: the - * install URL dead-ends on a "manage" page (no `code`) for orgs that already have - * the App installed, whereas the authorize URL works in both cases and honors - * `redirect_uri` for per-environment callback routing. + * the App's client id. This returns a `code` to our callback whether or not + * the App is installed (unlike the install URL, which dead-ends on a + * "manage" page for already-installed orgs), and it honors `redirect_uri` + * for per-environment callback routing. + * 2. The callback exchanges the `code` for a user-to-server token, then checks + * whether the user actually has an installation (GET /user/installations): + * - Installed → finalize the connection. + * - NOT installed → redirect to `installUrl` so the user installs the App + * on their org and picks repositories. GitHub then returns through + * install-time OAuth (with an `installation_id`), the check passes, and + * the connection is finalized. The `installation_id` on the return trip + * is used only as a loop guard, never persisted. * * The five checks are reused verbatim from the `github` manifest — only the way * the token is obtained differs (read-only App vs read/write OAuth App), so there @@ -69,11 +71,14 @@ export const githubAppManifest: IntegrationManifest = { // We use login/oauth/authorize rather than the App's install URL on // purpose: the authorize URL returns a `code` whether or not the App is // already installed (the install URL dead-ends on the "manage" page for - // already-installed orgs), shows an "Install & Authorize" screen for - // first-time users, and honors redirect_uri so each environment routes to - // its own callback. + // already-installed orgs) and honors redirect_uri so each environment + // routes to its own callback. authorizeUrl: 'https://github.com/login/oauth/authorize', tokenUrl: 'https://github.com/login/oauth/access_token', + // Where to send a user who authorized but has NOT installed the App yet, so + // they can install it on their org and choose repositories. The public app + // slug is the same across environments (one App, multiple callback URLs). + installUrl: 'https://github.com/apps/comp-ai-compliance/installations/new', scopes: [], pkce: false, clientAuthMethod: 'body', diff --git a/packages/integration-platform/src/types.ts b/packages/integration-platform/src/types.ts index acbbef67da..e56123ce2b 100644 --- a/packages/integration-platform/src/types.ts +++ b/packages/integration-platform/src/types.ts @@ -32,6 +32,14 @@ export const OAuthConfigSchema = z.object({ * Default: true */ supportsRefreshToken: z.boolean().default(true), + /** + * App-installation URL for providers (e.g. GitHub Apps) where user + * authorization and app installation are separate steps. When set, the OAuth + * callback can redirect a user who authorized but has NOT installed the app to + * this URL to complete installation (choosing account/repositories) before the + * connection is finalized. + */ + installUrl: z.string().url().optional(), /** * Separate URL for token refresh (if different from tokenUrl). * Most providers use the same tokenUrl for both.