Skip to content

Commit 9a77cbd

Browse files
authored
Merge pull request #3374 from trycompai/main
[comp] Production Deploy
2 parents 7318544 + fe63eb1 commit 9a77cbd

4 files changed

Lines changed: 178 additions & 41 deletions

File tree

‎apps/api/src/integration-platform/controllers/oauth.controller.spec.ts‎

Lines changed: 112 additions & 18 deletions
Original file line numberDiff line numberDiff line change
@@ -614,13 +614,33 @@ describe('OAuthController', () => {
614614
config: {
615615
authorizeUrl: 'https://github.com/login/oauth/authorize',
616616
tokenUrl: 'https://github.com/login/oauth/access_token',
617+
installUrl:
618+
'https://github.com/apps/comp-ai-compliance/installations/new',
617619
},
618620
},
619621
capabilities: [],
620622
isActive: true,
621623
};
622624

623-
it('blocks the GitHub App connect when the App is not installed', async () => {
625+
const notInstalledFetch = () =>
626+
jest
627+
.spyOn(global, 'fetch')
628+
// 1) token exchange
629+
.mockResolvedValueOnce({
630+
ok: true,
631+
status: 200,
632+
json: () => Promise.resolve({ access_token: 'gh_token' }),
633+
text: () => Promise.resolve(''),
634+
} as unknown as Response)
635+
// 2) GET /user/installations -> none
636+
.mockResolvedValueOnce({
637+
ok: true,
638+
status: 200,
639+
json: () => Promise.resolve({ total_count: 0, installations: [] }),
640+
text: () => Promise.resolve(''),
641+
} as unknown as Response);
642+
643+
it('redirects to the install URL when the App is not installed (first pass)', async () => {
624644
const futureDate = new Date(Date.now() + 600000);
625645
mockOAuthStateRepository.findByState.mockResolvedValue({
626646
state: 'gh_state',
@@ -638,31 +658,105 @@ describe('OAuthController', () => {
638658
scopes: [],
639659
source: 'platform',
640660
});
661+
mockOAuthStateRepository.create.mockResolvedValue({
662+
state: 'install_state',
663+
});
641664

642-
const fetchSpy = jest
643-
.spyOn(global, 'fetch')
644-
// 1) token exchange
645-
.mockResolvedValueOnce({
646-
ok: true,
647-
status: 200,
648-
json: () => Promise.resolve({ access_token: 'gh_token' }),
649-
text: () => Promise.resolve(''),
650-
} as unknown as Response)
651-
// 2) GET /user/installations -> none
652-
.mockResolvedValueOnce({
653-
ok: true,
654-
status: 200,
655-
json: () => Promise.resolve({ total_count: 0, installations: [] }),
656-
text: () => Promise.resolve(''),
657-
} as unknown as Response);
665+
const fetchSpy = notInstalledFetch();
666+
667+
await controller.oauthCallback(
668+
{ code: 'auth_code', state: 'gh_state' },
669+
mockRequest,
670+
mockResponse,
671+
);
672+
673+
// No installation yet → send them to install, don't finalize.
674+
expect(mockConnectionService.activateConnection).not.toHaveBeenCalled();
675+
const redirectUrl = (mockResponse.redirect as jest.Mock).mock.calls[0][0];
676+
expect(redirectUrl).toContain(
677+
'https://github.com/apps/comp-ai-compliance/installations/new',
678+
);
679+
expect(redirectUrl).toContain('state=install_state');
680+
681+
// The install-handoff state must be created BEFORE the original state is
682+
// deleted, so a create failure can't leave the outer catch double-deleting
683+
// an already-removed record (which would hang the request).
684+
const createOrder =
685+
mockOAuthStateRepository.create.mock.invocationCallOrder[0];
686+
const deleteOrder =
687+
mockOAuthStateRepository.delete.mock.invocationCallOrder[0];
688+
expect(createOrder).toBeLessThan(deleteOrder);
689+
690+
fetchSpy.mockRestore();
691+
});
692+
693+
it('still emits an error redirect if creating the install-handoff state fails', async () => {
694+
const futureDate = new Date(Date.now() + 600000);
695+
mockOAuthStateRepository.findByState.mockResolvedValue({
696+
state: 'gh_state',
697+
providerSlug: 'github-app',
698+
organizationId: 'org_1',
699+
userId: 'user_1',
700+
codeVerifier: null,
701+
redirectUrl: null,
702+
expiresAt: futureDate,
703+
});
704+
mockedGetManifest.mockReturnValue(githubAppManifest as never);
705+
mockOAuthCredentialsService.getCredentials.mockResolvedValue({
706+
clientId: 'c',
707+
clientSecret: 's',
708+
scopes: [],
709+
source: 'platform',
710+
});
711+
// Creating the install-handoff state fails (e.g. DB error).
712+
mockOAuthStateRepository.create.mockRejectedValue(new Error('db down'));
713+
714+
const fetchSpy = notInstalledFetch();
658715

659716
await controller.oauthCallback(
660717
{ code: 'auth_code', state: 'gh_state' },
661718
mockRequest,
662719
mockResponse,
663720
);
664721

665-
// The connection must NOT be activated when the App isn't installed.
722+
// The original state was NOT deleted before the failed create, so the
723+
// outer catch can still clean it up and emit an error redirect (no hang).
724+
expect(mockOAuthStateRepository.delete).toHaveBeenCalledWith('gh_state');
725+
const errRedirect = (mockResponse.redirect as jest.Mock).mock.calls[0][0];
726+
expect(errRedirect).toContain('error=');
727+
expect(mockConnectionService.activateConnection).not.toHaveBeenCalled();
728+
729+
fetchSpy.mockRestore();
730+
});
731+
732+
it('errors (no loop) when still not installed after an install attempt', async () => {
733+
const futureDate = new Date(Date.now() + 600000);
734+
mockOAuthStateRepository.findByState.mockResolvedValue({
735+
state: 'gh_state',
736+
providerSlug: 'github-app',
737+
organizationId: 'org_1',
738+
userId: 'user_1',
739+
codeVerifier: null,
740+
redirectUrl: null,
741+
expiresAt: futureDate,
742+
});
743+
mockedGetManifest.mockReturnValue(githubAppManifest as never);
744+
mockOAuthCredentialsService.getCredentials.mockResolvedValue({
745+
clientId: 'c',
746+
clientSecret: 's',
747+
scopes: [],
748+
source: 'platform',
749+
});
750+
751+
const fetchSpy = notInstalledFetch();
752+
753+
// installation_id present → we already came back from an install attempt.
754+
await controller.oauthCallback(
755+
{ code: 'auth_code', state: 'gh_state', installation_id: '999' },
756+
mockRequest,
757+
mockResponse,
758+
);
759+
666760
expect(mockConnectionService.activateConnection).not.toHaveBeenCalled();
667761
const redirectUrl = (mockResponse.redirect as jest.Mock).mock.calls[0][0];
668762
expect(redirectUrl).toContain('error=github_app_not_installed');

‎apps/api/src/integration-platform/controllers/oauth.controller.ts‎

Lines changed: 38 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -39,6 +39,9 @@ interface OAuthCallbackQuery {
3939
state: string;
4040
error?: string;
4141
error_description?: string;
42+
// Present when GitHub returns from an App installation (install-time OAuth).
43+
// Used only as a loop guard for the install redirect — never persisted.
44+
installation_id?: string;
4245
}
4346

4447
@Controller({ path: 'integrations/oauth', version: '1' })
@@ -335,26 +338,53 @@ export class OAuthController {
335338
oauthState.codeVerifier,
336339
);
337340

338-
// GitHub App: a user can complete OAuth *without* installing the App,
339-
// which yields a token that cannot read any repositories. If GitHub
340-
// definitively reports no installation for this user, stop here and tell
341-
// them to install the App instead of creating a "connected" but unusable
342-
// integration. Fails open on any uncertainty so a valid connection is
343-
// never blocked by a transient error.
341+
// GitHub App: user *authorization* and app *installation* are separate on
342+
// GitHub. A user can complete OAuth without installing the App, producing a
343+
// token that can read no repositories. If GitHub definitively reports no
344+
// installation, don't finalize a useless connection — send the user to
345+
// install the App (choose org + repositories). They return through
346+
// install-time OAuth (with an installation_id) and this check then passes.
347+
// Fails open on any uncertainty so a valid connection is never blocked by a
348+
// transient error.
344349
if (
345350
oauthState.providerSlug === 'github-app' &&
346351
(await this.githubAppInstallationMissing(tokens.access_token))
347352
) {
353+
// First pass (came from plain authorize, no installation_id): send the
354+
// user to install the App, then they return here via install-time OAuth.
355+
if (!query.installation_id && oauthConfig.installUrl) {
356+
// Create the install-handoff state BEFORE deleting the original one.
357+
// If creation fails, the original state survives so the outer catch can
358+
// clean it up and still emit an error redirect — deleting first would
359+
// make the catch's delete throw (record gone) and hang the request.
360+
const installState = await this.oauthStateRepository.create({
361+
providerSlug: oauthState.providerSlug,
362+
organizationId: oauthState.organizationId,
363+
userId: oauthState.userId,
364+
redirectUrl: oauthState.redirectUrl ?? undefined,
365+
});
366+
await this.oauthStateRepository.delete(state);
367+
const installRedirect = new URL(oauthConfig.installUrl);
368+
installRedirect.searchParams.set('state', installState.state);
369+
this.logger.log(
370+
`GitHub App authorized but not installed; redirecting org ${oauthState.organizationId} to install`,
371+
);
372+
res.redirect(installRedirect.toString());
373+
return;
374+
}
375+
376+
// We already came back from an install attempt but still see no
377+
// installation — stop rather than loop.
348378
await this.oauthStateRepository.delete(state);
349379
this.logger.warn(
350-
`GitHub App authorized but not installed for org ${oauthState.organizationId}`,
380+
`GitHub App still not installed after install attempt for org ${oauthState.organizationId}`,
351381
);
352382
const errorUrl = this.buildRedirectUrl(
353383
oauthState.redirectUrl,
354384
{
355385
error: 'github_app_not_installed',
356386
error_description:
357-
'You authorized Comp AI, but the GitHub App is not installed on your organization yet. Install the App on GitHub, then reconnect.',
387+
'The GitHub App was not installed on your organization. Please install it (selecting at least one repository) and try again.',
358388
},
359389
oauthState.organizationId,
360390
);

‎packages/integration-platform/src/manifests/github-app/index.ts‎

Lines changed: 20 additions & 15 deletions
Original file line numberDiff line numberDiff line change
@@ -15,19 +15,21 @@
1515
* is left completely untouched so current connections keep working. New/concerned
1616
* customers opt into this one.
1717
*
18-
* Connect flow (user-to-server token via the standard OAuth authorize URL):
18+
* Connect flow (user-to-server token — GitHub separates user *authorization*
19+
* from app *installation*, so the callback bridges the two):
1920
* 1. The customer is sent to `https://github.com/login/oauth/authorize` with
20-
* the App's client id. First-time users get an "Install & Authorize" screen
21-
* (install the App + pick repositories); already-installed users just
22-
* authorize. Either way GitHub returns to our callback with a `code`.
23-
* 2. The shared OAuth callback exchanges the `code` for a user-to-server access
24-
* token (read-only, capped by the App's permissions) and stores it like any
25-
* other OAuth token.
26-
*
27-
* We deliberately use the authorize URL rather than the App's install URL: the
28-
* install URL dead-ends on a "manage" page (no `code`) for orgs that already have
29-
* the App installed, whereas the authorize URL works in both cases and honors
30-
* `redirect_uri` for per-environment callback routing.
21+
* the App's client id. This returns a `code` to our callback whether or not
22+
* the App is installed (unlike the install URL, which dead-ends on a
23+
* "manage" page for already-installed orgs), and it honors `redirect_uri`
24+
* for per-environment callback routing.
25+
* 2. The callback exchanges the `code` for a user-to-server token, then checks
26+
* whether the user actually has an installation (GET /user/installations):
27+
* - Installed → finalize the connection.
28+
* - NOT installed → redirect to `installUrl` so the user installs the App
29+
* on their org and picks repositories. GitHub then returns through
30+
* install-time OAuth (with an `installation_id`), the check passes, and
31+
* the connection is finalized. The `installation_id` on the return trip
32+
* is used only as a loop guard, never persisted.
3133
*
3234
* The five checks are reused verbatim from the `github` manifest — only the way
3335
* the token is obtained differs (read-only App vs read/write OAuth App), so there
@@ -69,11 +71,14 @@ export const githubAppManifest: IntegrationManifest = {
6971
// We use login/oauth/authorize rather than the App's install URL on
7072
// purpose: the authorize URL returns a `code` whether or not the App is
7173
// already installed (the install URL dead-ends on the "manage" page for
72-
// already-installed orgs), shows an "Install & Authorize" screen for
73-
// first-time users, and honors redirect_uri so each environment routes to
74-
// its own callback.
74+
// already-installed orgs) and honors redirect_uri so each environment
75+
// routes to its own callback.
7576
authorizeUrl: 'https://github.com/login/oauth/authorize',
7677
tokenUrl: 'https://github.com/login/oauth/access_token',
78+
// Where to send a user who authorized but has NOT installed the App yet, so
79+
// they can install it on their org and choose repositories. The public app
80+
// slug is the same across environments (one App, multiple callback URLs).
81+
installUrl: 'https://github.com/apps/comp-ai-compliance/installations/new',
7782
scopes: [],
7883
pkce: false,
7984
clientAuthMethod: 'body',

‎packages/integration-platform/src/types.ts‎

Lines changed: 8 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -32,6 +32,14 @@ export const OAuthConfigSchema = z.object({
3232
* Default: true
3333
*/
3434
supportsRefreshToken: z.boolean().default(true),
35+
/**
36+
* App-installation URL for providers (e.g. GitHub Apps) where user
37+
* authorization and app installation are separate steps. When set, the OAuth
38+
* callback can redirect a user who authorized but has NOT installed the app to
39+
* this URL to complete installation (choosing account/repositories) before the
40+
* connection is finalized.
41+
*/
42+
installUrl: z.string().url().optional(),
3543
/**
3644
* Separate URL for token refresh (if different from tokenUrl).
3745
* Most providers use the same tokenUrl for both.

0 commit comments

Comments
 (0)