Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions ENVIRONMENT.md
Original file line number Diff line number Diff line change
Expand Up @@ -95,6 +95,7 @@ Manage shared web env var additions and rotations with `pnpm web:env set <VARIAB
- `GITHUB_LITE_APP_ID` - Lighter/secondary GitHub App ID for select integrations. `[SECRET]`
- `GITHUB_LITE_APP_PRIVATE_KEY` - Private key for the lite GitHub App. `[SECRET]`
- `GITHUB_LITE_APP_CLIENT_ID` - OAuth Client ID for the lite GitHub App install/login flow. [PUBLIC]
- `GITHUB_MULTIPLE_INSTALLATION_ORGANIZATION_IDS` - Comma-separated Kilo organization UUIDs allowed to connect multiple GitHub App installations. Unset or empty disables multiple installations for all organizations. [SERVER]
- `GITHUB_ADMIN_STATS_TOKEN` - Token for admin GitHub API stats lookups; used in `apps/web/src/scripts/backfill-pr-author-github-ids.ts`. `[SECRET]`
- `GITHUB_CLI_PAT` - GitHub personal access token for `gh` CLI operations inside contractors; used in `services/gastown/container/src/process-manager.ts`. `[SECRET]`
- `GITHUB_TOKEN` - Generic GitHub token for API calls used as fallback when `GIT_TOKEN` or `GITHUB_CLI_PAT` is absent; used in `services/gastown/container/src/process-manager.ts`. `[SECRET]`
Expand Down
32 changes: 32 additions & 0 deletions apps/web/src/app/api/integrations/github/callback/route.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -1065,6 +1065,38 @@ describe('GET /api/integrations/github/callback admin proof', () => {
expectRedirectLocation(response, `/integrations/github?error=installation_already_claimed`);
});

test('redirects when multiple installations are disabled for the organization', async () => {
const organizationId = '00000000-0000-4000-8000-000000000001';
mockedConsumeInstallState.mockResolvedValue({
token: INSTALL_STATE_TOKEN,
kilo_user_id: USER_ID,
owner_type: 'org',
owner_id: organizationId,
github_app_type: 'standard',
return_to: null,
expires_at: new Date(Date.now() + 300_000).toISOString(),
consumed_at: null,
created_at: new Date().toISOString(),
});
mockedUpsertPlatformIntegrationForOwner.mockResolvedValue({
ok: false,
reason: 'multiple_installations_disabled',
});

const { GET } = await import('./route');
const response = await GET(
makeRequest(
`/api/integrations/github/callback?installation_id=${INSTALLATION_ID}&setup_action=install&state=${INSTALL_STATE_TOKEN}&code=abc`
) as never
);

expect(response.status).toBe(307);
expectRedirectLocation(
response,
`/organizations/${organizationId}/integrations/github?error=multiple_installations_disabled`
);
});

test('logs distinct messages for code-absent vs non-admin', async () => {
const logSpy = jest.spyOn(console, 'log').mockImplementation(() => {});

Expand Down
10 changes: 6 additions & 4 deletions apps/web/src/app/api/integrations/github/callback/route.ts
Original file line number Diff line number Diff line change
Expand Up @@ -636,13 +636,15 @@ async function handleCoreInstallFlow(params: {
});

if (!upsertResult.ok) {
const error =
upsertResult.reason === 'multiple_installations_disabled'
? 'multiple_installations_disabled'
: 'installation_already_claimed';
if (isAppInitiated) {
return NextResponse.redirect(
new URL(appFallbackPath('error=installation_already_claimed'), APP_URL)
);
return NextResponse.redirect(new URL(appFallbackPath(`error=${error}`), APP_URL));
}
return NextResponse.redirect(
new URL(appendQueryParam(redirectPath, 'error=installation_already_claimed'), APP_URL)
new URL(appendQueryParam(redirectPath, `error=${error}`), APP_URL)
);
}
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -56,6 +56,15 @@ describe('GitHubIntegrationDetails fromApp outcome CTA behavior', () => {
expect(view.href).toBe('/cloud/sessions?error=installation_already_claimed');
});

it('non-retryable multiple-installation error: Back and no retry', () => {
const view = buildAppReturnOutcomeView({ error: 'multiple_installations_disabled' });
expect(view.kind).toBe('blocked');
expect(view.cta).toBe('Back');
expect(view.description).toBe(
'This Kilo organization can currently connect only one GitHub organization.'
);
});

it('non-retryable user mismatch: Back, no retry, mismatch copy preserved', () => {
const view = buildAppReturnOutcomeView({ error: 'install_state_user_mismatch' });
expect(view.kind).toBe('blocked');
Expand Down
15 changes: 11 additions & 4 deletions apps/web/src/components/integrations/GitHubIntegrationDetails.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -76,7 +76,8 @@ export function buildAppReturnOutcomeView(input: {
const isNonRetryable =
input.error === 'install_state_user_mismatch' ||
input.error === 'not_installation_admin' ||
input.error === 'installation_already_claimed';
input.error === 'installation_already_claimed' ||
input.error === 'multiple_installations_disabled';
const returnQuery = isSuccess
? 'github_install=success'
: isPending
Expand All @@ -97,9 +98,11 @@ export function buildAppReturnOutcomeView(input: {
? 'Only a GitHub admin of that account can connect it. Ask an organization admin to install Kilo.'
: input.error === 'installation_already_claimed'
? 'That GitHub installation is already connected to another Kilo account. Disconnect it there first.'
: input.error === 'install_state_user_mismatch'
? 'This connection was started from the Kilo App signed in as a different account. Sign in to the web with that account, or start again from the app.'
: 'The installation did not complete. Try again or return to the Kilo App.';
: input.error === 'multiple_installations_disabled'
? 'This Kilo organization can currently connect only one GitHub organization.'
: input.error === 'install_state_user_mismatch'
? 'This connection was started from the Kilo App signed in as a different account. Sign in to the web with that account, or start again from the app.'
: 'The installation did not complete. Try again or return to the Kilo App.';
const cta = isSuccess ? 'Continue' : isPending ? 'Done' : isNonRetryable ? 'Back' : 'Try again';

return {
Expand Down Expand Up @@ -152,6 +155,10 @@ function GitHubIntegrationOutcomeToasts({
'That GitHub installation is already connected to another Kilo account. Disconnect it there first.',
{ duration: 8000 }
);
} else if (error === 'multiple_installations_disabled') {
toast.error('This organization can currently connect only one GitHub organization.', {
duration: 8000,
});
} else if (error === 'github_authorization_required') {
toast.error('GitHub did not return an authorization. Start the connection again.', {
duration: 8000,
Expand Down
19 changes: 19 additions & 0 deletions apps/web/src/lib/integrations/db/platform-integrations.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -116,6 +116,25 @@ describe('upsertPlatformIntegrationForOwner', () => {
expect(row.owned_by_organization_id).toBe(orgId);
});

test('rejects a second GitHub installation for an organization outside the allowlist', async () => {
const owner: Owner = { type: 'org', id: orgId };
await upsertPlatformIntegrationForOwner(owner, baseInstallData(INSTALLATION_ID));

const result = await upsertPlatformIntegrationForOwner(
owner,
baseInstallData(`${INSTALLATION_ID}-second`)
);

expect(result).toEqual({ ok: false, reason: 'multiple_installations_disabled' });

const rows = await db
.select()
.from(platform_integrations)
.where(eq(platform_integrations.owned_by_organization_id, orgId));
expect(rows).toHaveLength(1);
expect(rows[0]?.platform_installation_id).toBe(INSTALLATION_ID);
});

test('same-owner refresh updates the existing row (by primary key)', async () => {
const owner: Owner = { type: 'user', id: userId };

Expand Down
21 changes: 20 additions & 1 deletion apps/web/src/lib/integrations/db/platform-integrations.ts
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,7 @@ import type { IntegrationStatus } from '../core/constants';
import { platformIntegrationHealthSql } from '../core/health';
import { PendingInstallationMetadataWrapperSchema } from '../core/schemas';
import type { GitHubAppType } from '../platforms/github/app-selector';
import { canOrganizationUseMultipleGitHubInstallations } from '../github/multiple-installations';

/**
* Finds a platform integration by installation ID.
Expand Down Expand Up @@ -724,7 +725,10 @@ export async function unsuspendIntegrationForOwner(

export type UpsertPlatformIntegrationResult =
| { ok: true }
| { ok: false; reason: 'claimed_by_other_owner' };
| {
ok: false;
reason: 'claimed_by_other_owner' | 'multiple_installations_disabled';
};

/**
* Owner-aware upsert for platform integrations.
Expand Down Expand Up @@ -776,6 +780,21 @@ export async function upsertPlatformIntegrationForOwner(
// Step 2: if the insert was blocked, re-read the row and determine
// whether this is a same-owner refresh or a cross-owner claim.
if (data.platform === 'github') {
if (owner.type === 'org' && !canOrganizationUseMultipleGitHubInstallations(owner.id)) {
const ownerIntegrations = await getIntegrationsByOrganization(owner.id, PLATFORM.GITHUB);
const hasExistingInstallation = ownerIntegrations.some(
integration => integration.platform_installation_id !== null
);
const isExistingInstallation = ownerIntegrations.some(
integration =>
integration.platform_installation_id === data.platformInstallationId &&
integration.github_app_type === appType
);
if (hasExistingInstallation && !isExistingInstallation) {
return { ok: false, reason: 'multiple_installations_disabled' };
}
}

const inserted = await db
.insert(platform_integrations)
.values(values)
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,62 @@
import { getEnvVariable } from '@/lib/dotenvx';
import {
canOrganizationUseMultipleGitHubInstallations,
parseMultipleGitHubInstallationOrganizationIds,
} from './multiple-installations';

jest.mock('@/lib/dotenvx', () => ({
getEnvVariable: jest.fn(),
}));

const mockedGetEnvVariable = jest.mocked(getEnvVariable);

describe('canOrganizationUseMultipleGitHubInstallations', () => {
beforeEach(() => {
mockedGetEnvVariable.mockReturnValue(
'9d278969-5453-4ae3-a51f-a8d2274a7b56,30f1620a-4aad-4456-bf4d-550f335e6f55'
);
});

it.each(['9d278969-5453-4ae3-a51f-a8d2274a7b56', '30f1620a-4aad-4456-bf4d-550f335e6f55'])(
'enables multiple installations for %s',
organizationId => {
expect(canOrganizationUseMultipleGitHubInstallations(organizationId)).toBe(true);
}
);

it('keeps multiple installations disabled for other organizations', () => {
expect(
canOrganizationUseMultipleGitHubInstallations('00000000-0000-4000-8000-000000000001')
).toBe(false);
});

it('keeps multiple installations disabled when the environment variable is unset', () => {
mockedGetEnvVariable.mockReturnValue('');

expect(
canOrganizationUseMultipleGitHubInstallations('9d278969-5453-4ae3-a51f-a8d2274a7b56')
).toBe(false);
});
});

describe('parseMultipleGitHubInstallationOrganizationIds', () => {
it('trims and deduplicates comma-separated organization IDs', () => {
expect(
parseMultipleGitHubInstallationOrganizationIds(
' 9d278969-5453-4ae3-a51f-a8d2274a7b56,30f1620a-4aad-4456-bf4d-550f335e6f55,9d278969-5453-4ae3-a51f-a8d2274a7b56 '
)
).toEqual(
new Set(['9d278969-5453-4ae3-a51f-a8d2274a7b56', '30f1620a-4aad-4456-bf4d-550f335e6f55'])
);
});

it('returns an empty set for an empty value', () => {
expect(parseMultipleGitHubInstallationOrganizationIds('')).toEqual(new Set());
});

it('rejects malformed organization IDs', () => {
expect(() => parseMultipleGitHubInstallationOrganizationIds('not-an-organization-id')).toThrow(
'GITHUB_MULTIPLE_INSTALLATION_ORGANIZATION_IDS must be a comma-separated list of UUIDs'
);
});
});
26 changes: 26 additions & 0 deletions apps/web/src/lib/integrations/github/multiple-installations.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,26 @@
import 'server-only';

import { z } from 'zod';
import { getEnvVariable } from '@/lib/dotenvx';

export function parseMultipleGitHubInstallationOrganizationIds(value: string): Set<string> {
const organizationIds = value
.split(',')
.map(organizationId => organizationId.trim())
.filter(Boolean);

const result = z.array(z.uuid()).safeParse(organizationIds);
if (!result.success) {
throw new Error(
'GITHUB_MULTIPLE_INSTALLATION_ORGANIZATION_IDS must be a comma-separated list of UUIDs'
);
}

return new Set(result.data);
}

export function canOrganizationUseMultipleGitHubInstallations(organizationId: string): boolean {
return parseMultipleGitHubInstallationOrganizationIds(
getEnvVariable('GITHUB_MULTIPLE_INSTALLATION_ORGANIZATION_IDS')
).has(organizationId);
}
62 changes: 59 additions & 3 deletions apps/web/src/routers/github-apps-router.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -128,6 +128,7 @@ beforeAll(async () => {
});

const organizationId = '00000000-0000-4000-8000-000000000001';
const multiInstallationOrganizationId = '9d278969-5453-4ae3-a51f-a8d2274a7b56';
const integrationId = '00000000-0000-4000-8000-000000000002';
const organizationRoles = [
'owner',
Expand Down Expand Up @@ -172,6 +173,8 @@ function organizationIntegration(): PlatformIntegration {
describe('githubAppsRouter organization install capability', () => {
beforeEach(() => {
jest.clearAllMocks();
process.env.GITHUB_MULTIPLE_INSTALLATION_ORGANIZATION_IDS =
'9d278969-5453-4ae3-a51f-a8d2274a7b56,30f1620a-4aad-4456-bf4d-550f335e6f55';
mockEnsureOrganizationAccess.mockResolvedValue('member');
mockGetGitHubAppTypeForOrganization.mockResolvedValue('standard');
mockCreateInstallState.mockResolvedValue('install-token');
Expand Down Expand Up @@ -222,17 +225,70 @@ describe('githubAppsRouter organization install capability', () => {
}
);

it.each(organizationRoles)('reports add capability for organization %s roles', async role => {
mockEnsureOrganizationAccess.mockResolvedValue(role);
it.each(organizationRoles)(
'reports first-install capability for organization %s roles',
async role => {
mockEnsureOrganizationAccess.mockResolvedValue(role);
const caller = createCaller({ user: { id: 'user-1', is_admin: false } as User });

const listed = await caller.listOrganizationInstallations({ organizationId });

expect(listed.canAdd).toBe(role === 'owner' || role === 'admin');
expect(listed.installations).toHaveLength(0);
}
);

it('hides additional installation capability for organizations outside the allowlist', async () => {
mockEnsureOrganizationAccess.mockResolvedValue('owner');
mockListIntegrations.mockResolvedValue([organizationIntegration()]);
const caller = createCaller({ user: { id: 'user-1', is_admin: false } as User });

const listed = await caller.listOrganizationInstallations({ organizationId });

expect(listed.canAdd).toBe(role === 'owner' || role === 'admin');
expect(listed.canAdd).toBe(false);
expect(listed.installations).toHaveLength(1);
});

it('reports additional installation capability for allowlisted organizations', async () => {
mockEnsureOrganizationAccess.mockResolvedValue('owner');
mockListIntegrations.mockResolvedValue([
{ ...organizationIntegration(), owned_by_organization_id: multiInstallationOrganizationId },
]);
const caller = createCaller({ user: { id: 'user-1', is_admin: false } as User });

const listed = await caller.listOrganizationInstallations({
organizationId: multiInstallationOrganizationId,
});

expect(listed.canAdd).toBe(true);
});

it('refuses to mint another install state outside the allowlist', async () => {
mockEnsureOrganizationAccess.mockResolvedValue('owner');
mockListIntegrations.mockResolvedValue([organizationIntegration()]);
const caller = createCaller({ user: { id: 'user-1', is_admin: false } as User });

await expect(caller.mintInstallState({ organizationId })).rejects.toMatchObject({
code: 'FORBIDDEN',
});
expect(mockCreateInstallState).not.toHaveBeenCalled();
});

it('mints another install state for an allowlisted organization', async () => {
mockEnsureOrganizationAccess.mockResolvedValue('owner');
mockListIntegrations.mockResolvedValue([
{ ...organizationIntegration(), owned_by_organization_id: multiInstallationOrganizationId },
]);
const caller = createCaller({ user: { id: 'user-1', is_admin: false } as User });

await expect(
caller.mintInstallState({ organizationId: multiInstallationOrganizationId })
).resolves.toEqual({ token: 'install-token' });
expect(mockCreateInstallState).toHaveBeenCalledWith(
expect.objectContaining({ ownerId: multiInstallationOrganizationId })
);
});

it('still denies callers outside the organization role matrix before minting state', async () => {
mockEnsureOrganizationAccess.mockRejectedValue(
new TRPCError({ code: 'UNAUTHORIZED', message: 'Organization access required' })
Expand Down
Loading
Loading