diff --git a/apps/web/src/app/api/integrations/linear/callback/route.test.ts b/apps/web/src/app/api/integrations/linear/callback/route.test.ts index 3382a614321..a1e34bef4e6 100644 --- a/apps/web/src/app/api/integrations/linear/callback/route.test.ts +++ b/apps/web/src/app/api/integrations/linear/callback/route.test.ts @@ -1,11 +1,16 @@ import { beforeEach, describe, expect, test } from '@jest/globals'; import { NextRequest } from 'next/server'; +import { eq } from 'drizzle-orm'; +import { db } from '@kilocode/web-shared/lib/drizzle'; +import { kilocode_users, platform_integrations } from '@kilocode/db/schema'; +import { insertTestUser } from '@kilocode/web-shared/tests/helpers/user.helper'; import { getUserFromAuth } from '@kilocode/web-shared/lib/user/server'; import { verifyOAuthState, createOAuthState } from '@/lib/integrations/oauth-state'; import { bot } from '@/lib/bot'; import { exchangeLinearOAuthCode, fetchLinearOAuthIdentity, + getConflictingLinearInstallation, LinearWorkspaceAlreadyConnectedError, revokeLinearToken, upsertLinearInstallation, @@ -20,18 +25,9 @@ import { PLATFORM } from '@/lib/integrations/core/constants'; import { createSignedToken } from '@/lib/signed-token'; const mockIsEnabledForBot = jest.fn(); -const mockHandleOAuthCallback = jest.fn(); const mockConsumeProviderOAuthAttempt = jest.fn(async (_input: unknown) => true); const mockCancelProviderOAuthAttempt = jest.fn(async (_input: unknown) => true); -const mockLinearOrganization = jest.fn(async () => ({ name: 'Acme Workspace', urlKey: 'acme' })); - -jest.mock('@linear/sdk', () => ({ - LinearClient: jest.fn().mockImplementation(() => ({ - get organization() { - return mockLinearOrganization(); - }, - })), -})); + jest.mock('@kilocode/web-shared/lib/user/server'); jest.mock('@/lib/bot', () => ({ bot: { @@ -47,6 +43,7 @@ jest.mock('@/lib/integrations/linear-service', () => { upsertLinearInstallation: jest.fn(), exchangeLinearOAuthCode: jest.fn(), fetchLinearOAuthIdentity: jest.fn(), + getConflictingLinearInstallation: jest.fn(), revokeLinearToken: jest.fn(async () => true), }; }); @@ -84,6 +81,7 @@ const mockedUpsertLinearInstallation = jest.mocked(upsertLinearInstallation); const mockedUnlinkTeamKiloUsers = jest.mocked(unlinkTeamKiloUsers); const mockedExchangeLinearOAuthCode = jest.mocked(exchangeLinearOAuthCode); const mockedFetchLinearOAuthIdentity = jest.mocked(fetchLinearOAuthIdentity); +const mockedGetConflictingLinearInstallation = jest.mocked(getConflictingLinearInstallation); const mockedRevokeLinearToken = jest.mocked(revokeLinearToken); const mockedLinkKiloUser = jest.mocked(linkKiloUser); const mockedVerifyLinearBotLinkState = jest.mocked(verifyLinearBotLinkState); @@ -122,6 +120,10 @@ async function callLinearCallback(request: NextRequest) { } describe('GET /api/integrations/linear/callback', () => { + let getInstallation: jest.Mock, [string]>; + let setInstallation: jest.Mock, [string, unknown]>; + let deleteInstallation: jest.Mock, [string]>; + beforeEach(() => { jest.clearAllMocks(); mockConsumeProviderOAuthAttempt.mockResolvedValue(true); @@ -137,21 +139,28 @@ describe('GET /api/integrations/linear/callback', () => { authFailedResponse: null, } as never); - mockHandleOAuthCallback.mockResolvedValue({ + mockedExchangeLinearOAuthCode.mockResolvedValue({ + accessToken: 'lin_app_tok', + refreshToken: null, + expiresIn: null, + scope: null, + }); + mockedFetchLinearOAuthIdentity.mockResolvedValue({ + viewerId: 'linear-bot-user', + viewerName: 'Kilo', organizationId: ORGANIZATION_ID, - installation: { - accessToken: 'tok', - botUserId: 'bot-1', - organizationId: ORGANIZATION_ID, - expiresAt: null, - }, + organizationName: 'Acme Workspace', }); + mockedGetConflictingLinearInstallation.mockResolvedValue(null); + + getInstallation = jest.fn, [string]>(async () => null); + setInstallation = jest.fn, [string, unknown]>(async () => undefined); + deleteInstallation = jest.fn, [string]>(async () => undefined); mockedBotGetAdapter.mockReturnValue({ - handleOAuthCallback: mockHandleOAuthCallback, - deleteInstallation: jest.fn(async () => undefined), + getInstallation, + setInstallation, + deleteInstallation, } as never); - - mockLinearOrganization.mockResolvedValue({ name: 'Acme Workspace', urlKey: 'acme' }); }); test('redirects to sign-in when user is unauthenticated', async () => { @@ -174,7 +183,7 @@ describe('GET /api/integrations/linear/callback', () => { expectRedirectLocation(response, '/integrations/linear?error=access_denied'); expect(mockCancelProviderOAuthAttempt).toHaveBeenCalledTimes(1); expect(mockConsumeProviderOAuthAttempt).not.toHaveBeenCalled(); - expect(mockHandleOAuthCallback).not.toHaveBeenCalled(); + expect(mockedExchangeLinearOAuthCode).not.toHaveBeenCalled(); expect(mockedUpsertLinearInstallation).not.toHaveBeenCalled(); }); @@ -212,7 +221,7 @@ describe('GET /api/integrations/linear/callback', () => { expectRedirectLocation(response, '/integrations/linear?error=missing_code'); expect(mockCancelProviderOAuthAttempt).toHaveBeenCalledTimes(1); expect(mockConsumeProviderOAuthAttempt).not.toHaveBeenCalled(); - expect(mockHandleOAuthCallback).not.toHaveBeenCalled(); + expect(mockedExchangeLinearOAuthCode).not.toHaveBeenCalled(); expect(mockedUpsertLinearInstallation).not.toHaveBeenCalled(); }); @@ -248,7 +257,7 @@ describe('GET /api/integrations/linear/callback', () => { ); expectRedirectLocation(response, '/integrations?error=invalid_state'); - expect(mockHandleOAuthCallback).not.toHaveBeenCalled(); + expect(mockedExchangeLinearOAuthCode).not.toHaveBeenCalled(); expect(mockedUpsertLinearInstallation).not.toHaveBeenCalled(); expect(mockConsumeProviderOAuthAttempt).not.toHaveBeenCalled(); expect(mockCancelProviderOAuthAttempt).not.toHaveBeenCalled(); @@ -267,7 +276,7 @@ describe('GET /api/integrations/linear/callback', () => { makeRequest(`/api/integrations/linear/callback?code=abc&state=${state}`) ); expectRedirectLocation(response, '/integrations?error=invalid_state'); - expect(mockHandleOAuthCallback).not.toHaveBeenCalled(); + expect(mockedExchangeLinearOAuthCode).not.toHaveBeenCalled(); expect(mockedUpsertLinearInstallation).not.toHaveBeenCalled(); expect(mockConsumeProviderOAuthAttempt).not.toHaveBeenCalled(); expect(mockCancelProviderOAuthAttempt).not.toHaveBeenCalled(); @@ -282,7 +291,7 @@ describe('GET /api/integrations/linear/callback', () => { makeRequest(`/api/integrations/linear/callback?code=abc&state=${state}`) ); expectRedirectLocation(response, '/integrations?error=invalid_state'); - expect(mockHandleOAuthCallback).not.toHaveBeenCalled(); + expect(mockedExchangeLinearOAuthCode).not.toHaveBeenCalled(); expect(mockedUpsertLinearInstallation).not.toHaveBeenCalled(); expect(mockConsumeProviderOAuthAttempt).not.toHaveBeenCalled(); expect(mockCancelProviderOAuthAttempt).not.toHaveBeenCalled(); @@ -311,32 +320,46 @@ describe('GET /api/integrations/linear/callback', () => { makeRequest(`/api/integrations/linear/callback?code=abc&state=${state}`) ); expectRedirectLocation(response, '/integrations?error=invalid_state'); - expect(mockHandleOAuthCallback).not.toHaveBeenCalled(); + expect(mockedExchangeLinearOAuthCode).not.toHaveBeenCalled(); expect(mockedUpsertLinearInstallation).not.toHaveBeenCalled(); expect(mockConsumeProviderOAuthAttempt).not.toHaveBeenCalled(); expect(mockCancelProviderOAuthAttempt).not.toHaveBeenCalled(); }); - test('invokes upsertLinearInstallation with the workspace name from Linear GraphQL', async () => { - mockedUpsertLinearInstallation.mockResolvedValue({} as never); - const state = createProviderState(); - const response = await callLinearCallback( - makeRequest(`/api/integrations/linear/callback?code=abc&state=${state}`) + test('persists the installation and the integration row', async () => { + await insertTestUser({ id: USER_ID }); + mockedUpsertLinearInstallation.mockImplementation( + jest.requireActual<{ upsertLinearInstallation: typeof upsertLinearInstallation }>( + '@/lib/integrations/linear-service' + ).upsertLinearInstallation ); + try { + const state = createProviderState(); + const response = await callLinearCallback( + makeRequest(`/api/integrations/linear/callback?code=abc&state=${state}`) + ); - expect(mockedUpsertLinearInstallation).toHaveBeenCalledWith( - expect.objectContaining({ - owner: { type: 'user', id: USER_ID }, + expectRedirectLocation(response, '/integrations/linear?success=installed'); + expect(setInstallation).toHaveBeenCalledWith(ORGANIZATION_ID, { + accessToken: 'lin_app_tok', + botUserId: 'linear-bot-user', + expiresAt: null, organizationId: ORGANIZATION_ID, - organizationName: 'Acme Workspace', - }), - expect.objectContaining({ - getChatSdkAccessToken: expect.any(Function), - deleteChatSdkInstallation: expect.any(Function), - deleteChatSdkIdentityCache: expect.any(Function), - }) - ); - expectRedirectLocation(response, '/integrations/linear?success=installed'); + }); + const [stored] = await db + .select() + .from(platform_integrations) + .where(eq(platform_integrations.owned_by_user_id, USER_ID)); + expect(stored).toMatchObject({ + platform: PLATFORM.LINEAR, + platform_installation_id: ORGANIZATION_ID, + platform_account_login: 'Acme Workspace', + metadata: expect.objectContaining({ bot_enabled: true, bot_user_id: 'linear-bot-user' }), + }); + } finally { + mockedUpsertLinearInstallation.mockReset(); + await db.delete(kilocode_users).where(eq(kilocode_users.id, USER_ID)); + } }); test('redirects successful installs to returnTo when signed state carries one', async () => { @@ -364,56 +387,84 @@ describe('GET /api/integrations/linear/callback', () => { ); }); - test('falls back to organizationId when the Linear GraphQL query fails', async () => { - mockLinearOrganization.mockRejectedValue(new Error('Unauthorized')); + test('carries the token lifetime and refresh token into the stored installation', async () => { + mockedExchangeLinearOAuthCode.mockResolvedValue({ + accessToken: 'lin_app_tok', + refreshToken: 'lin_refresh_tok', + expiresIn: 3600, + scope: null, + }); mockedUpsertLinearInstallation.mockResolvedValue({} as never); + + const before = Date.now(); const state = createProviderState(); await callLinearCallback( makeRequest(`/api/integrations/linear/callback?code=abc&state=${state}`) ); + const after = Date.now(); - expect(mockedUpsertLinearInstallation).toHaveBeenCalledWith( - expect.objectContaining({ - organizationName: ORGANIZATION_ID, - }), - expect.anything() + const installation = setInstallation.mock.calls[0]?.[1] as { + expiresAt: number; + refreshToken?: string; + }; + expect(installation.refreshToken).toBe('lin_refresh_tok'); + expect(installation.expiresAt).toBeGreaterThanOrEqual(before + 3_600_000); + expect(installation.expiresAt).toBeLessThanOrEqual(after + 3_600_000); + }); + + test('leaves the stored installation and identity links untouched when another owner holds the workspace', async () => { + mockedGetConflictingLinearInstallation.mockResolvedValue({ id: 'pi_other' } as never); + + const state = createProviderState(); + const response = await callLinearCallback( + makeRequest(`/api/integrations/linear/callback?code=abc&state=${state}`) ); + + expect(setInstallation).not.toHaveBeenCalled(); + expect(deleteInstallation).not.toHaveBeenCalled(); + expect(mockedUnlinkTeamKiloUsers).not.toHaveBeenCalled(); + expect(mockedUpsertLinearInstallation).not.toHaveBeenCalled(); + expectRedirectLocation(response, '/integrations/linear?error=workspace_already_connected'); }); - test('rolls back the Chat SDK installation when the workspace is already connected', async () => { + test('restores the installation it replaced when the workspace is claimed after the check', async () => { + const previousInstallation = { + accessToken: 'prior_tok', + botUserId: 'linear-bot-user', + expiresAt: null, + organizationId: ORGANIZATION_ID, + }; + getInstallation.mockResolvedValue(previousInstallation); mockedUpsertLinearInstallation.mockRejectedValue( new LinearWorkspaceAlreadyConnectedError('Acme') ); - const deleteInstallation = jest.fn, [string]>(async () => undefined); - const adapter = { - handleOAuthCallback: jest.fn(async () => ({ - organizationId: ORGANIZATION_ID, - installation: { - accessToken: 'tok', - botUserId: 'bot-1', - organizationId: ORGANIZATION_ID, - expiresAt: null, - }, - })), - withInstallation: jest.fn(async (_orgId: unknown, fn: () => Promise | T) => fn()), - getUser: jest.fn(async () => ({ fullName: 'Acme', userName: 'acme' })), - deleteInstallation, - }; - mockedBotGetAdapter.mockReturnValue(adapter as never); const state = createProviderState(); const response = await callLinearCallback( makeRequest(`/api/integrations/linear/callback?code=abc&state=${state}`) ); + expect(setInstallation).toHaveBeenCalledTimes(2); + expect(setInstallation).toHaveBeenLastCalledWith(ORGANIZATION_ID, previousInstallation); + expect(deleteInstallation).not.toHaveBeenCalled(); + expect(mockedUnlinkTeamKiloUsers).not.toHaveBeenCalled(); + expectRedirectLocation(response, '/integrations/linear?error=workspace_already_connected'); + }); + + test('drops only the installation it created when the workspace is claimed after the check', async () => { + mockedUpsertLinearInstallation.mockRejectedValue( + new LinearWorkspaceAlreadyConnectedError('Acme') + ); + + const state = createProviderState(); + const response = await callLinearCallback( + makeRequest(`/api/integrations/linear/callback?code=abc&state=${state}`) + ); + + expect(getInstallation).toHaveBeenCalledWith(ORGANIZATION_ID); expect(deleteInstallation).toHaveBeenCalledTimes(1); expect(deleteInstallation).toHaveBeenCalledWith(ORGANIZATION_ID); - expect(mockedUnlinkTeamKiloUsers).toHaveBeenCalledTimes(1); - expect(mockedUnlinkTeamKiloUsers).toHaveBeenCalledWith( - expect.anything(), - 'linear', - ORGANIZATION_ID - ); + expect(mockedUnlinkTeamKiloUsers).not.toHaveBeenCalled(); expectRedirectLocation(response, '/integrations/linear?error=workspace_already_connected'); }); }); @@ -433,11 +484,12 @@ describe('GET /api/integrations/linear/callback bot-link flow', () => { authFailedResponse: null, } as never); - // The default install-flow adapter must NOT be invoked when the bot-link - // state matches; tests assert this. + // The install-flow adapter must NOT be invoked when the bot-link state + // matches; tests assert the install branch never persists an installation. mockedBotGetAdapter.mockReturnValue({ - handleOAuthCallback: jest.fn(async () => { - throw new Error('handleOAuthCallback should not be called for bot-link'); + getInstallation: jest.fn(async () => null), + setInstallation: jest.fn(async () => { + throw new Error('the workspace install flow must not run for a bot-link callback'); }), deleteInstallation: jest.fn(async () => undefined), } as never); @@ -642,24 +694,11 @@ describe('GET /api/integrations/linear/callback bot-link flow', () => { }); test('does not invoke the workspace install flow when a bot-link state matches', async () => { - const handleOAuthCallback = jest.fn(async () => ({ - organizationId: ORGANIZATION_ID, - installation: { - accessToken: 'tok', - botUserId: 'bot-1', - organizationId: ORGANIZATION_ID, - expiresAt: null, - }, - })); - mockedBotGetAdapter.mockReturnValue({ - handleOAuthCallback, - deleteInstallation: jest.fn(async () => undefined), - } as never); - await callLinearCallback( makeRequest('/api/integrations/linear/callback?code=abc&state=signed-bot-link') ); - expect(handleOAuthCallback).not.toHaveBeenCalled(); + expect(mockedGetConflictingLinearInstallation).not.toHaveBeenCalled(); + expect(mockedUpsertLinearInstallation).not.toHaveBeenCalled(); }); }); diff --git a/apps/web/src/app/api/integrations/oauth-install-conflict.test.ts b/apps/web/src/app/api/integrations/oauth-install-conflict.test.ts new file mode 100644 index 00000000000..2b9ebf843db --- /dev/null +++ b/apps/web/src/app/api/integrations/oauth-install-conflict.test.ts @@ -0,0 +1,444 @@ +import { afterEach, beforeAll, beforeEach, describe, expect, test } from '@jest/globals'; +import { NextRequest } from 'next/server'; +import { setImmediate } from 'node:timers/promises'; +import { and, eq, sql } from 'drizzle-orm'; +import { db } from '@kilocode/web-shared/lib/drizzle'; +import { insertTestUser } from '@kilocode/web-shared/tests/helpers/user.helper'; +import { createTestOrganization } from '@kilocode/web-shared/tests/helpers/organization.helper'; +import { organizations, platform_integrations, type User } from '@kilocode/db/schema'; +import { getUserFromAuth } from '@kilocode/web-shared/lib/user/server'; +import { createOAuthState } from '@/lib/integrations/oauth-state'; +import { INTEGRATION_STATUS, PLATFORM } from '@/lib/integrations/core/constants'; + +const mockInstallations = new Map(); +const mockIdentityLinks = new Map>(); +const mockGetInstallation = jest.fn(async (installationId: string) => { + return mockInstallations.get(installationId) ?? null; +}); +const mockSetInstallation = jest.fn(async (installationId: string, installation: unknown) => { + mockInstallations.set(installationId, installation); +}); +const mockDeleteInstallation = jest.fn(async (installationId: string) => { + mockInstallations.delete(installationId); +}); +const mockUnlinkTeamKiloUsers = jest.fn( + async (_state: unknown, platform: string, teamId: string) => { + mockIdentityLinks.delete(`${platform}:${teamId}`); + return 0; + } +); +const mockLinearOrganizationId = { current: 'linear-workspace' }; +const mockSlackAccess = jest.fn(); +const mockConsumeProviderOAuthAttempt = jest.fn(async (_input: unknown) => true); + +jest.mock('@kilocode/web-shared/lib/user/server'); +jest.mock('@kilocode/web-shared/routers/organizations/utils', () => ({ + ensureOrganizationAccess: jest.fn(), +})); +jest.mock('@/lib/slack-bot/model-allow-list', () => ({ + getDefaultAllowedModel: jest.fn(async () => 'test/default-model'), +})); +jest.mock('@kilocode/web-shared/lib/config.server', () => ({ + ...jest.requireActual('@kilocode/web-shared/lib/config.server'), + LINEAR_CLIENT_ID: 'test-linear-client-id', + LINEAR_CLIENT_SECRET: 'test-linear-client-secret', + SLACK_CLIENT_ID: 'test-slack-client-id', + SLACK_CLIENT_SECRET: 'test-slack-client-secret', +})); +jest.mock('@sentry/nextjs', () => ({ + captureException: jest.fn(), + captureMessage: jest.fn(), +})); +jest.mock('@linear/sdk', () => ({ + LinearClient: jest.fn(() => ({ + get viewer() { + return Promise.resolve({ + id: 'linear-bot-user', + displayName: 'Kilo', + get organization() { + return Promise.resolve({ + id: mockLinearOrganizationId.current, + name: 'Victim Workspace', + urlKey: 'victim', + }); + }, + }); + }, + })), +})); +jest.mock('@slack/web-api', () => ({ + WebClient: jest.fn(() => ({ oauth: { v2: { access: mockSlackAccess } } })), +})); +jest.mock('@/lib/bot', () => ({ + bot: { + initialize: jest.fn(async () => undefined), + getAdapter: jest.fn(() => ({ + getInstallation: (installationId: string) => mockGetInstallation(installationId), + setInstallation: (installationId: string, installation: unknown) => + mockSetInstallation(installationId, installation), + deleteInstallation: (installationId: string) => mockDeleteInstallation(installationId), + })), + getState: jest.fn(() => ({ kind: 'state' })), + }, +})); +jest.mock('@/lib/bot-identity', () => ({ + linkKiloUser: jest.fn(async () => undefined), + unlinkTeamKiloUsers: (state: unknown, platform: string, teamId: string) => + mockUnlinkTeamKiloUsers(state, platform, teamId), +})); +jest.mock('@/lib/bot/linear-link-state', () => ({ + verifyLinearBotLinkState: jest.fn(() => null), +})); +jest.mock('@/lib/bot/platform-helpers', () => ({ + getPlatformIntegrationById: jest.fn(), + canKiloUserAccessPlatformIntegration: jest.fn(), +})); +jest.mock('@/lib/bot/platforms', () => ({ + botPlatforms: { require: jest.fn() }, +})); +jest.mock('@/lib/integrations/provider-oauth-attempts', () => ({ + consumeProviderOAuthAttempt: (input: unknown) => mockConsumeProviderOAuthAttempt(input), + cancelProviderOAuthAttempt: jest.fn(async () => true), +})); + +const mockedGetUserFromAuth = jest.mocked(getUserFromAuth); + +const createProviderState = (userId: string, platform: 'linear' | 'slack') => + createOAuthState(`user_${userId}`, userId, undefined, { + provider: platform, + purpose: 'provider_install', + }); + +async function callCallback(platform: 'linear' | 'slack', query: string) { + const { GET } = await import('./[platform]/callback/route'); + return GET( + new NextRequest(`http://localhost:3000/api/integrations/${platform}/callback${query}`), + { + params: Promise.resolve({ platform }), + } + ); +} + +function redirectPath(response: Response) { + const location = response.headers.get('location'); + const url = new URL(location ?? ''); + return `${url.pathname}${url.search}`; +} + +async function seedIntegration(user: User, platform: 'linear' | 'slack', installationId: string) { + const [row] = await db + .insert(platform_integrations) + .values({ + owned_by_user_id: user.id, + owned_by_organization_id: null, + platform, + integration_type: 'oauth', + platform_installation_id: installationId, + platform_account_id: installationId, + platform_account_login: 'Existing Workspace', + scopes: [], + integration_status: INTEGRATION_STATUS.ACTIVE, + metadata: { bot_enabled: true }, + installed_at: '2026-01-01T00:00:00.000Z', + }) + .returning(); + return row; +} + +async function findIntegration(ownerUserId: string, platform: 'linear' | 'slack') { + const [row] = await db + .select() + .from(platform_integrations) + .where( + and( + eq(platform_integrations.owned_by_user_id, ownerUserId), + eq(platform_integrations.platform, platform) + ) + ); + return row ?? null; +} + +const existingLinearInstallation = { + accessToken: 'existing-linear-token', + botUserId: 'linear-bot-user', + expiresAt: null, + organizationId: 'linear-workspace', +}; +const existingSlackInstallation = { + botToken: 'xoxb-existing-token', + botUserId: 'U0EXISTING', + teamName: 'Existing Team', +}; + +let owner: User; +let otherOwner: User; + +beforeEach(() => { + mockInstallations.clear(); + mockIdentityLinks.clear(); + jest.clearAllMocks(); + mockConsumeProviderOAuthAttempt.mockResolvedValue(true); + mockLinearOrganizationId.current = 'linear-workspace'; + + globalThis.fetch = (async (input: RequestInfo | URL) => { + const url = typeof input === 'string' ? input : input instanceof URL ? input.href : input.url; + if (url === 'https://api.linear.app/oauth/token') { + return new Response(JSON.stringify({ access_token: 'lin-app-token', expires_in: 86_400 }), { + status: 200, + headers: { 'content-type': 'application/json' }, + }); + } + throw new Error(`unexpected outbound request: ${url}`); + }) as typeof fetch; + + mockSlackAccess.mockResolvedValue({ + ok: true, + access_token: 'xoxb-new-token', + bot_user_id: 'U0BOT', + team: { id: 'T0TEAM', name: 'Acme Workspace' }, + is_enterprise_install: false, + }); + + mockedGetUserFromAuth.mockResolvedValue({ + user: { id: otherOwner.id }, + authFailedResponse: null, + } as never); +}); + +beforeAll(async () => { + owner = await insertTestUser({ + google_user_email: 'oauth-conflict-owner@example.com', + google_user_name: 'OAuth Conflict Owner', + }); + otherOwner = await insertTestUser({ + google_user_email: 'oauth-conflict-other@example.com', + google_user_name: 'OAuth Conflict Other', + }); +}); + +afterEach(async () => { + for (const user of [owner, otherOwner]) { + await db + .delete(platform_integrations) + .where(eq(platform_integrations.owned_by_user_id, user.id)); + } +}); + +describe('Linear install callback against the real integration row', () => { + test('rejects a workspace held by another owner without changing its installation or links', async () => { + const victimRow = await seedIntegration(owner, PLATFORM.LINEAR, 'linear-workspace'); + mockInstallations.set('linear-workspace', existingLinearInstallation); + mockIdentityLinks.set('linear:linear-workspace', new Set(['linear-user-1'])); + + const response = await callCallback( + 'linear', + `?code=abc&state=${createProviderState(otherOwner.id, 'linear')}` + ); + + expect(redirectPath(response)).toBe('/integrations/linear?error=workspace_already_connected'); + expect(mockSetInstallation).not.toHaveBeenCalled(); + expect(mockDeleteInstallation).not.toHaveBeenCalled(); + expect(mockInstallations.get('linear-workspace')).toBe(existingLinearInstallation); + expect(mockIdentityLinks.get('linear:linear-workspace')).toEqual(new Set(['linear-user-1'])); + expect(mockUnlinkTeamKiloUsers).not.toHaveBeenCalled(); + + const unchanged = await findIntegration(owner.id, PLATFORM.LINEAR); + expect(unchanged).toMatchObject({ + id: victimRow.id, + platform_installation_id: 'linear-workspace', + platform_account_login: 'Existing Workspace', + integration_status: INTEGRATION_STATUS.ACTIVE, + metadata: { bot_enabled: true }, + updated_at: victimRow.updated_at, + }); + expect(await findIntegration(otherOwner.id, PLATFORM.LINEAR)).toBeNull(); + }); + + test('connects an unclaimed workspace and stores its installation and row', async () => { + mockLinearOrganizationId.current = 'linear-workspace-new'; + + const response = await callCallback( + 'linear', + `?code=abc&state=${createProviderState(otherOwner.id, 'linear')}` + ); + + expect(redirectPath(response)).toBe('/integrations/linear?success=installed'); + expect(mockInstallations.get('linear-workspace-new')).toMatchObject({ + accessToken: 'lin-app-token', + botUserId: 'linear-bot-user', + organizationId: 'linear-workspace-new', + }); + + const row = await findIntegration(otherOwner.id, PLATFORM.LINEAR); + expect(row).toMatchObject({ + platform_installation_id: 'linear-workspace-new', + platform_account_login: 'Victim Workspace', + integration_status: INTEGRATION_STATUS.ACTIVE, + metadata: expect.objectContaining({ bot_enabled: true, bot_user_id: 'linear-bot-user' }), + }); + }); +}); + +describe('Slack install callback against the real integration row', () => { + test('rejects a workspace held by another owner without changing its installation or links', async () => { + const victimRow = await seedIntegration(owner, PLATFORM.SLACK, 'T0TEAM'); + mockInstallations.set('T0TEAM', existingSlackInstallation); + mockIdentityLinks.set('slack:T0TEAM', new Set(['slack-user-1'])); + + const response = await callCallback( + 'slack', + `?code=abc&state=${createProviderState(otherOwner.id, 'slack')}` + ); + + expect(redirectPath(response)).toBe('/integrations/slack?error=workspace_already_connected'); + expect(mockSetInstallation).not.toHaveBeenCalled(); + expect(mockDeleteInstallation).not.toHaveBeenCalled(); + expect(mockInstallations.get('T0TEAM')).toBe(existingSlackInstallation); + expect(mockIdentityLinks.get('slack:T0TEAM')).toEqual(new Set(['slack-user-1'])); + + const unchanged = await findIntegration(owner.id, PLATFORM.SLACK); + expect(unchanged).toMatchObject({ + id: victimRow.id, + platform_installation_id: 'T0TEAM', + integration_status: INTEGRATION_STATUS.ACTIVE, + updated_at: victimRow.updated_at, + }); + expect(await findIntegration(otherOwner.id, PLATFORM.SLACK)).toBeNull(); + }); + + test('connects an unclaimed workspace and stores its installation and row', async () => { + mockSlackAccess.mockResolvedValue({ + ok: true, + access_token: 'xoxb-new-token', + bot_user_id: 'U0BOT', + team: { id: 'T0NEWTEAM', name: 'New Workspace' }, + is_enterprise_install: false, + }); + + const response = await callCallback( + 'slack', + `?code=abc&state=${createProviderState(otherOwner.id, 'slack')}` + ); + + expect(redirectPath(response)).toBe('/integrations/slack?success=installed'); + expect(mockInstallations.get('T0NEWTEAM')).toMatchObject({ + botToken: 'xoxb-new-token', + botUserId: 'U0BOT', + teamName: 'New Workspace', + }); + + const row = await findIntegration(otherOwner.id, PLATFORM.SLACK); + expect(row).toMatchObject({ + platform_installation_id: 'T0NEWTEAM', + platform_account_login: 'New Workspace', + integration_status: INTEGRATION_STATUS.ACTIVE, + metadata: expect.objectContaining({ bot_user_id: 'U0BOT' }), + }); + }); + + test('completes an authorized organization workspace update', async () => { + const organization = await createTestOrganization('Callback Workspace', otherOwner.id, 0); + try { + const [existing] = await db + .insert(platform_integrations) + .values({ + owned_by_organization_id: organization.id, + platform: PLATFORM.SLACK, + integration_type: 'oauth', + platform_installation_id: 'T0PREVIOUS', + platform_account_id: 'T0PREVIOUS', + platform_account_login: 'Previous Workspace', + integration_status: INTEGRATION_STATUS.ACTIVE, + metadata: { model_slug: 'test/selected-model' }, + }) + .returning(); + const state = createOAuthState(`org_${organization.id}`, otherOwner.id, undefined, { + provider: 'slack', + purpose: 'provider_install', + }); + const response = await callCallback('slack', `?code=abc&state=${state}`); + + expect(redirectPath(response)).toBe( + `/organizations/${organization.id}/integrations/slack?success=installed` + ); + const [stored] = await db + .select() + .from(platform_integrations) + .where(eq(platform_integrations.id, existing.id)); + expect(stored).toMatchObject({ + owned_by_organization_id: organization.id, + platform_installation_id: 'T0TEAM', + integration_status: INTEGRATION_STATUS.ACTIVE, + metadata: expect.objectContaining({ model_slug: 'test/selected-model' }), + }); + expect(mockInstallations.get('T0TEAM')).toMatchObject({ botUserId: 'U0BOT' }); + } finally { + await db.delete(organizations).where(eq(organizations.id, organization.id)); + } + }); +}); + +test.each(['slack', 'linear'] as const)( + '%s workspace installation serialization', + async platform => { + const installationId = platform === 'slack' ? 'T0TEAM' : 'linear-workspace'; + const firstWrite = Promise.withResolvers(); + const releaseFirst = Promise.withResolvers(); + mockSetInstallation.mockImplementationOnce(async (id, installation) => { + mockInstallations.set(id, installation); + firstWrite.resolve(); + await releaseFirst.promise; + }); + mockedGetUserFromAuth + .mockResolvedValueOnce({ user: { id: owner.id }, authFailedResponse: null } as never) + .mockResolvedValueOnce({ user: { id: otherOwner.id }, authFailedResponse: null } as never); + + const winner = callCallback( + platform, + `?code=first&state=${createProviderState(owner.id, platform)}` + ); + await firstWrite.promise; + const storedInstallation = mockInstallations.get(installationId); + const contender = callCallback( + platform, + `?code=second&state=${createProviderState(otherOwner.id, platform)}` + ); + + const abort = new AbortController(); + const waitingForLock = (async () => { + const deadline = Date.now() + 2000; + while (!abort.signal.aborted && Date.now() < deadline) { + const waiting = await db.execute(sql` + SELECT 1 FROM pg_locks + WHERE locktype = 'advisory' AND NOT granted + AND database = (SELECT oid FROM pg_database WHERE datname = current_database()) + `); + if (waiting.rows.length > 0) return 'waiting'; + await setImmediate(); + } + return 'timeout'; + })(); + let progress; + try { + progress = await Promise.race([waitingForLock, contender.then(() => 'completed')]); + } finally { + abort.abort(); + await waitingForLock; + releaseFirst.resolve(); + } + + const [winnerResponse, contenderResponse] = await Promise.all([winner, contender]); + expect(progress).toBe('waiting'); + expect(redirectPath(winnerResponse)).toBe(`/integrations/${platform}?success=installed`); + expect(redirectPath(contenderResponse)).toBe( + `/integrations/${platform}?error=workspace_already_connected` + ); + expect(mockInstallations.get(installationId)).toBe(storedInstallation); + expect(mockDeleteInstallation).not.toHaveBeenCalled(); + expect(await findIntegration(owner.id, platform)).toMatchObject({ + platform_installation_id: installationId, + integration_status: INTEGRATION_STATUS.ACTIVE, + }); + expect(await findIntegration(otherOwner.id, platform)).toBeNull(); + } +); diff --git a/apps/web/src/app/api/integrations/slack/callback/route.test.ts b/apps/web/src/app/api/integrations/slack/callback/route.test.ts new file mode 100644 index 00000000000..88fec2871de --- /dev/null +++ b/apps/web/src/app/api/integrations/slack/callback/route.test.ts @@ -0,0 +1,222 @@ +import { beforeEach, describe, expect, test } from '@jest/globals'; +import { NextRequest } from 'next/server'; +import { eq } from 'drizzle-orm'; +import { db } from '@kilocode/web-shared/lib/drizzle'; +import { kilocode_users, platform_integrations } from '@kilocode/db/schema'; +import { insertTestUser } from '@kilocode/web-shared/tests/helpers/user.helper'; +import { getUserFromAuth } from '@kilocode/web-shared/lib/user/server'; +import { createOAuthState } from '@/lib/integrations/oauth-state'; +import { bot } from '@/lib/bot'; +import { + exchangeSlackOAuthCode, + getConflictingSlackInstallation, + SlackWorkspaceAlreadyConnectedError, + upsertSlackInstallation, +} from '@/lib/integrations/slack-service'; + +const mockConsumeProviderOAuthAttempt = jest.fn(async (_input: unknown) => true); +const mockCancelProviderOAuthAttempt = jest.fn(async (_input: unknown) => true); + +jest.mock('@kilocode/web-shared/lib/user/server'); +jest.mock('@/lib/bot', () => ({ + bot: { + initialize: jest.fn(async () => undefined), + getAdapter: jest.fn(), + getState: jest.fn(() => ({ kind: 'state' })), + }, +})); +jest.mock('@/lib/integrations/slack-service', () => { + const actual = jest.requireActual('@/lib/integrations/slack-service'); + return { + ...actual, + upsertSlackInstallation: jest.fn(), + exchangeSlackOAuthCode: jest.fn(), + getConflictingSlackInstallation: jest.fn(), + }; +}); +jest.mock('@/lib/integrations/provider-oauth-attempts', () => ({ + consumeProviderOAuthAttempt: (input: unknown) => mockConsumeProviderOAuthAttempt(input), + cancelProviderOAuthAttempt: (input: unknown) => mockCancelProviderOAuthAttempt(input), +})); +jest.mock('@kilocode/web-shared/routers/organizations/utils', () => ({ + ensureOrganizationAccess: jest.fn(), +})); +jest.mock('@sentry/nextjs', () => ({ + captureException: jest.fn(), + captureMessage: jest.fn(), +})); + +const mockedGetUserFromAuth = jest.mocked(getUserFromAuth); +const mockedBotGetAdapter = jest.mocked(bot.getAdapter); +const mockedExchangeSlackOAuthCode = jest.mocked(exchangeSlackOAuthCode); +const mockedGetConflictingSlackInstallation = jest.mocked(getConflictingSlackInstallation); +const mockedUpsertSlackInstallation = jest.mocked(upsertSlackInstallation); + +const USER_ID = '034489e8-19e0-4479-9d69-2edad719e847'; +const TEAM_ID = 'T024BE7LD'; +const createProviderState = (returnTo?: string) => + createOAuthState(`user_${USER_ID}`, USER_ID, returnTo, { + provider: 'slack', + purpose: 'provider_install', + }); + +const INSTALLATION = { + botToken: 'xoxb-new-token', + botUserId: 'U0BOT', + teamName: 'Acme Workspace', +}; + +function makeRequest(pathWithQuery: string) { + return new NextRequest(`http://localhost:3000${pathWithQuery}`); +} + +function expectRedirectLocation(response: Response, expectedPathWithQuery: string) { + const location = response.headers.get('location'); + expect(location).toBeTruthy(); + const url = new URL(location ?? ''); + expect(`${url.pathname}${url.search}`).toBe(expectedPathWithQuery); +} + +async function callSlackCallback(request: NextRequest) { + const { GET } = await import('../../[platform]/callback/route'); + return GET(request, { params: Promise.resolve({ platform: 'slack' }) }); +} + +describe('GET /api/integrations/slack/callback', () => { + let getInstallation: jest.Mock, [string]>; + let setInstallation: jest.Mock, [string, unknown]>; + let deleteInstallation: jest.Mock, [string]>; + + beforeEach(() => { + jest.clearAllMocks(); + mockConsumeProviderOAuthAttempt.mockResolvedValue(true); + mockCancelProviderOAuthAttempt.mockResolvedValue(true); + + mockedGetUserFromAuth.mockResolvedValue({ + user: { id: USER_ID }, + authFailedResponse: null, + } as never); + + mockedExchangeSlackOAuthCode.mockResolvedValue({ + installationId: TEAM_ID, + enterpriseId: undefined, + isEnterpriseInstall: false, + installation: INSTALLATION, + }); + mockedGetConflictingSlackInstallation.mockResolvedValue(null); + + getInstallation = jest.fn, [string]>(async () => null); + setInstallation = jest.fn, [string, unknown]>(async () => undefined); + deleteInstallation = jest.fn, [string]>(async () => undefined); + mockedBotGetAdapter.mockReturnValue({ + getInstallation, + setInstallation, + deleteInstallation, + } as never); + }); + + test('redirects to sign-in when the user is unauthenticated', async () => { + mockedGetUserFromAuth.mockResolvedValue({ + user: null, + authFailedResponse: new Response(null, { status: 401 }), + } as never); + + const response = await callSlackCallback(makeRequest('/api/integrations/slack/callback')); + + expect(response.headers.get('location')).toContain('/users/sign_in'); + }); + + test('persists the installation and the integration row', async () => { + await insertTestUser({ id: USER_ID }); + mockedUpsertSlackInstallation.mockImplementation( + jest.requireActual<{ upsertSlackInstallation: typeof upsertSlackInstallation }>( + '@/lib/integrations/slack-service' + ).upsertSlackInstallation + ); + try { + const state = createProviderState(); + const response = await callSlackCallback( + makeRequest(`/api/integrations/slack/callback?code=abc&state=${state}`) + ); + + expectRedirectLocation(response, '/integrations/slack?success=installed'); + expect(setInstallation).toHaveBeenCalledWith(TEAM_ID, INSTALLATION); + const [stored] = await db + .select() + .from(platform_integrations) + .where(eq(platform_integrations.owned_by_user_id, USER_ID)); + expect(stored).toMatchObject({ + platform: 'slack', + platform_installation_id: TEAM_ID, + platform_account_login: 'Acme Workspace', + metadata: expect.objectContaining({ bot_user_id: 'U0BOT', access_token: 'xoxb-new-token' }), + }); + } finally { + mockedUpsertSlackInstallation.mockReset(); + await db.delete(kilocode_users).where(eq(kilocode_users.id, USER_ID)); + } + }); + + test('redirects successful installs to returnTo when signed state carries one', async () => { + mockedUpsertSlackInstallation.mockResolvedValue({} as never); + const state = createProviderState('/claw/new?step=slack'); + + const response = await callSlackCallback( + makeRequest(`/api/integrations/slack/callback?code=abc&state=${state}`) + ); + + expectRedirectLocation(response, '/claw/new?step=slack&success=slack_installed'); + }); + + test('leaves the stored installation untouched when another owner holds the workspace', async () => { + mockedGetConflictingSlackInstallation.mockResolvedValue({ id: 'pi_other' } as never); + const state = createProviderState(); + + const response = await callSlackCallback( + makeRequest(`/api/integrations/slack/callback?code=abc&state=${state}`) + ); + + expect(setInstallation).not.toHaveBeenCalled(); + expect(deleteInstallation).not.toHaveBeenCalled(); + expect(mockedUpsertSlackInstallation).not.toHaveBeenCalled(); + expectRedirectLocation(response, '/integrations/slack?error=workspace_already_connected'); + }); + + test('restores the installation it replaced when the workspace is claimed after the check', async () => { + const previousInstallation = { + botToken: 'xoxb-prior-token', + botUserId: 'U0PRIOR', + teamName: 'Prior Workspace', + }; + getInstallation.mockResolvedValue(previousInstallation); + mockedUpsertSlackInstallation.mockRejectedValue( + new SlackWorkspaceAlreadyConnectedError('Prior Workspace') + ); + const state = createProviderState(); + + const response = await callSlackCallback( + makeRequest(`/api/integrations/slack/callback?code=abc&state=${state}`) + ); + + expect(setInstallation).toHaveBeenCalledTimes(2); + expect(setInstallation).toHaveBeenLastCalledWith(TEAM_ID, previousInstallation); + expect(deleteInstallation).not.toHaveBeenCalled(); + expectRedirectLocation(response, '/integrations/slack?error=workspace_already_connected'); + }); + + test('drops only the installation it created when the workspace is claimed after the check', async () => { + mockedUpsertSlackInstallation.mockRejectedValue( + new SlackWorkspaceAlreadyConnectedError('Acme Workspace') + ); + const state = createProviderState(); + + const response = await callSlackCallback( + makeRequest(`/api/integrations/slack/callback?code=abc&state=${state}`) + ); + + expect(getInstallation).toHaveBeenCalledWith(TEAM_ID); + expect(deleteInstallation).toHaveBeenCalledTimes(1); + expect(deleteInstallation).toHaveBeenCalledWith(TEAM_ID); + expectRedirectLocation(response, '/integrations/slack?error=workspace_already_connected'); + }); +}); diff --git a/apps/web/src/lib/integrations/linear-service.test.ts b/apps/web/src/lib/integrations/linear-service.test.ts index 128834962c2..ea9f85d5305 100644 --- a/apps/web/src/lib/integrations/linear-service.test.ts +++ b/apps/web/src/lib/integrations/linear-service.test.ts @@ -86,6 +86,76 @@ describe('upsertLinearInstallation', () => { ); }); + test('stores no model when the caller supplies null for the model', async () => { + const result = await upsertLinearInstallation( + { + owner: { type: 'user', id: user.id }, + organizationId: 'workspace-unsupplied-model', + organizationName: 'Unsupplied Model Workspace', + botUserId: 'bot-1', + }, + { defaultModel: null } + ); + + expect(result.metadata).not.toHaveProperty('model_slug'); + }); + + test('uses the supplied transaction for installation ownership and updates', async () => { + const rollback = new Error('Rollback test transaction'); + await expect( + db.transaction(async tx => { + const result = await upsertLinearInstallation( + { + owner: { type: 'user', id: user.id }, + organizationId: 'workspace-transaction', + organizationName: 'Transaction Workspace', + botUserId: 'bot-1', + }, + { defaultModel: 'test/default-model' }, + tx + ); + const updated = await upsertLinearInstallation( + { + owner: { type: 'user', id: user.id }, + organizationId: 'workspace-transaction', + organizationName: 'Updated Transaction Workspace', + botUserId: 'bot-2', + }, + { defaultModel: 'test/replacement-model' }, + tx + ); + expect(updated.id).toBe(result.id); + expect(updated.metadata).toEqual( + expect.objectContaining({ model_slug: 'test/default-model', bot_user_id: 'bot-2' }) + ); + await expect( + upsertLinearInstallation( + { + owner: { type: 'user', id: otherUser.id }, + organizationId: 'workspace-transaction', + organizationName: 'Transaction Workspace', + botUserId: 'bot-3', + }, + {}, + tx + ) + ).rejects.toBeInstanceOf(LinearWorkspaceAlreadyConnectedError); + throw rollback; + }) + ).rejects.toBe(rollback); + + const integrations = await db + .select() + .from(platform_integrations) + .where( + and( + eq(platform_integrations.platform, PLATFORM.LINEAR), + eq(platform_integrations.owned_by_user_id, user.id) + ) + ); + expect(integrations).toEqual([]); + }); + test('rejects when another owner already holds the same Linear workspace', async () => { await upsertLinearInstallation({ owner: { type: 'user', id: user.id }, diff --git a/apps/web/src/lib/integrations/linear-service.ts b/apps/web/src/lib/integrations/linear-service.ts index 183bd77a4b2..eb18fd53c84 100644 --- a/apps/web/src/lib/integrations/linear-service.ts +++ b/apps/web/src/lib/integrations/linear-service.ts @@ -1,7 +1,7 @@ import 'server-only'; import { captureException, captureMessage } from '@sentry/nextjs'; import { LinearClient } from '@linear/sdk'; -import { db } from '@kilocode/web-shared/lib/drizzle'; +import { db, type DrizzleTransaction } from '@kilocode/web-shared/lib/drizzle'; import type { PlatformIntegration } from '@kilocode/db/schema'; import { platform_integrations } from '@kilocode/db/schema'; import { eq, and, isNull } from 'drizzle-orm'; @@ -160,11 +160,14 @@ export type LinearOAuthTokenResponse = { }; /** - * Exchange a Linear OAuth authorization code for a transient user-actor - * access token. This is intentionally separate from - * `linearAdapter.handleOAuthCallback` — that path persists a workspace - * installation, but the bot-link flow only needs the token long enough - * to query the viewer's identity, after which it is revoked. + * Exchange a Linear OAuth authorization code at Linear's token endpoint. + * + * Two callers share it: + * - the bot-link flow, which only needs the token long enough to query the + * viewer's identity, after which it is revoked; + * - the workspace install callback, which resolves the target workspace from + * the token before persisting a Chat SDK installation, so a workspace held + * by a different owner never has its installation replaced. */ export async function exchangeLinearOAuthCode(code: string): Promise { if (!LINEAR_CLIENT_ID || !LINEAR_CLIENT_SECRET) { @@ -216,19 +219,19 @@ export type LinearOAuthIdentity = { }; /** - * Query `viewer` and `organization` with a freshly-issued user-actor - * access token. The returned `viewerId` is the Linear user id we will - * link to the Kilo user; `organizationId` is checked against the signed - * state so a multi-workspace user cannot link from a different workspace - * than the one the link token was issued for. + * Query `viewer` — and the workspace the viewer belongs to — with a + * freshly-issued Linear access token. The returned `viewerId` is the Linear + * user id for `actor=user` tokens and the app user id for `actor=app` install + * tokens; `organizationId` comes from `viewer { organization { id } }`, the + * same lookup the Chat SDK Linear adapter uses to key an installation. * * `LinearClient` wraps the GraphQL request / response handling and * throws `LinearGraphQLError` for both HTTP and GraphQL-level failures, * so errors propagate to the caller with a useful message. */ export async function fetchLinearOAuthIdentity(accessToken: string): Promise { - const linear = new LinearClient({ accessToken }); - const [viewer, organization] = await Promise.all([linear.viewer, linear.organization]); + const viewer = await new LinearClient({ accessToken }).viewer; + const organization = await viewer.organization; return { viewerId: viewer.id, @@ -238,8 +241,11 @@ export async function fetchLinearOAuthIdentity(accessToken: string): Promise { - const [integration] = await db +export async function getInstallation( + owner: Owner, + database: typeof db | DrizzleTransaction = db +): Promise { + const [integration] = await database .select() .from(platform_integrations) .where( @@ -273,11 +279,18 @@ function isOwnedBy(integration: PlatformIntegration, owner: Owner): boolean { : integration.owned_by_organization_id === owner.id && integration.owned_by_user_id === null; } -async function getConflictingLinearInstallation( +/** + * Find a Linear `platform_integrations` row for `organizationId` that belongs to + * a different Kilo owner. Callers use this before persisting a workspace + * installation so a workspace owned elsewhere is rejected without writing to + * the shared Chat SDK installation state. + */ +export async function getConflictingLinearInstallation( owner: Owner, - organizationId: string + organizationId: string, + database: typeof db | DrizzleTransaction = db ): Promise { - const integrations = await db + const integrations = await database .select() .from(platform_integrations) .where( @@ -311,6 +324,11 @@ export function getOwnerFromInstallation(integration: PlatformIntegration): Owne } type LinearUpsertOptions = { + /** + * Model resolved by the caller before the install transaction. `null` means the + * owner's policy allows no candidate model; `undefined` resolves it here. + */ + defaultModel?: string | null; getChatSdkAccessToken?: (organizationId: string) => Promise; deleteChatSdkInstallation?: (organizationId: string) => Promise; deleteChatSdkIdentityCache?: (organizationId: string) => Promise; @@ -318,10 +336,10 @@ type LinearUpsertOptions = { /** * Create or update a Linear `platform_integrations` row after a successful - * OAuth exchange. The OAuth access/refresh tokens are persisted inside the - * Chat SDK's state adapter by `linearAdapter.handleOAuthCallback`; this row - * stores only non-secret metadata used by the tRPC router / UI and to gate - * bot processing via `metadata.bot_enabled`. + * OAuth exchange. The OAuth access/refresh tokens live in the Chat SDK state + * adapter, written by the install callback after it has confirmed the workspace + * belongs to this owner; this row stores only non-secret metadata used by the + * tRPC router / UI and to gate bot processing via `metadata.bot_enabled`. * * When the same Kilo owner reinstalls onto a *different* Linear workspace * (`existing.platform_installation_id !== organizationId`), the optional @@ -343,11 +361,12 @@ export async function upsertLinearInstallation( organizationName: string; botUserId: string | null; }, - options: LinearUpsertOptions = {} + options: LinearUpsertOptions = {}, + database: typeof db | DrizzleTransaction = db ): Promise { - const existing = await getInstallation(owner); + const existing = await getInstallation(owner, database); - const conflicting = await getConflictingLinearInstallation(owner, organizationId); + const conflicting = await getConflictingLinearInstallation(owner, organizationId, database); if (conflicting) { throw new LinearWorkspaceAlreadyConnectedError(organizationName); } @@ -371,9 +390,11 @@ export async function upsertLinearInstallation( // stores no model and the settings UI keeps its model placeholder. // For user integrations, use the shared bot default model. const defaultModel = - owner.type === 'org' - ? await getDefaultAllowedModel(owner.id, DEFAULT_BOT_MODEL) - : DEFAULT_BOT_MODEL; + options.defaultModel !== undefined + ? options.defaultModel + : owner.type === 'org' + ? await getDefaultAllowedModel(owner.id, DEFAULT_BOT_MODEL) + : DEFAULT_BOT_MODEL; const existingMetadata = existing?.metadata && typeof existing.metadata === 'object' ? existing.metadata : {}; @@ -397,7 +418,7 @@ export async function upsertLinearInstallation( if (existing) { try { - const [updated] = await db + const [updated] = await database .update(platform_integrations) .set({ platform_installation_id: organizationId, @@ -423,7 +444,7 @@ export async function upsertLinearInstallation( } try { - const [created] = await db + const [created] = await database .insert(platform_integrations) .values({ owned_by_user_id: owner.type === 'user' ? owner.id : null, diff --git a/apps/web/src/lib/integrations/oauth/platforms/linear-callback.ts b/apps/web/src/lib/integrations/oauth/platforms/linear-callback.ts index 1c1278052bd..8d068e66d49 100644 --- a/apps/web/src/lib/integrations/oauth/platforms/linear-callback.ts +++ b/apps/web/src/lib/integrations/oauth/platforms/linear-callback.ts @@ -4,11 +4,10 @@ import { getUserFromAuth } from '@kilocode/web-shared/lib/user/server'; import { ensureOrganizationAccess } from '@kilocode/web-shared/routers/organizations/utils'; import { captureException, captureMessage } from '@sentry/nextjs'; import { getCallbackReportParams } from '@/lib/integrations/oauth/callback-params'; -import { LinearClient } from '@linear/sdk'; import { exchangeLinearOAuthCode, fetchLinearOAuthIdentity, - LINEAR_REDIRECT_URI, + getConflictingLinearInstallation, LinearWorkspaceAlreadyConnectedError, type LinearOAuthIdentity, revokeLinearToken, @@ -36,6 +35,9 @@ import { cancelMissingCodeProviderOAuthAttempt, } from '@/lib/integrations/oauth/common'; import { consumeProviderOAuthAttempt } from '@/lib/integrations/provider-oauth-attempts'; +import { withWorkspaceInstallationLock } from '@/lib/integrations/oauth/workspace-installation-lock'; +import { getDefaultAllowedModel } from '@/lib/slack-bot/model-allow-list'; +import { DEFAULT_BOT_MODEL } from '@/lib/bot/constants'; async function getChatSdkLinearAccessToken(organizationId: string): Promise { const installation = await bot.getAdapter('linear').getInstallation(organizationId); @@ -50,34 +52,6 @@ async function deleteChatSdkLinearIdentityCache(organizationId: string): Promise await unlinkTeamKiloUsers(bot.getState(), PLATFORM.LINEAR, organizationId); } -/** - * Fetch the workspace name for a freshly installed Linear installation by - * querying Linear's `organization` GraphQL field with the freshly-issued - * access token. We cannot use `linearAdapter.getUser('me')` for this: with - * `actor=app` installs (which is what `getLinearOAuthUrl` uses), the - * authenticated viewer is the app actor — the configured `userName` - * (`'kilo'` / `'kilo-dev'`) — not the human installer or the workspace. - * - * Falls back to the organizationId on failure so the install still succeeds - * with a human-readable label. - */ -async function fetchLinearWorkspaceName( - accessToken: string, - organizationId: string -): Promise { - try { - const organization = await new LinearClient({ accessToken }).organization; - return organization.name || organization.urlKey || organizationId; - } catch (error) { - captureMessage('Failed to fetch Linear workspace name', { - level: 'warning', - tags: { endpoint: 'linear/callback', source: 'linear_oauth' }, - extra: { organizationId, error: error instanceof Error ? error.message : String(error) }, - }); - return organizationId; - } -} - function htmlPage(title: string, message: string, status = 200): Response { return new Response( ` @@ -335,60 +309,83 @@ export async function handleLinearOAuthCallback(request: NextRequest) { throw new Error('Linear OAuth attempt is invalid, expired, or already used'); } - // Chat SDK exchanges the code, persists the per-workspace installation in - // its state adapter, and returns the Linear organizationId + installation. await bot.initialize(); const linearAdapter = bot.getAdapter('linear'); - const { organizationId, installation } = await linearAdapter.handleOAuthCallback(request, { - redirectUri: LINEAR_REDIRECT_URI, - }); - const workspaceName = await fetchLinearWorkspaceName(installation.accessToken, organizationId); - - try { - await upsertLinearInstallation( - { - owner, - organizationId, - organizationName: workspaceName, - botUserId: installation.botUserId, - }, - { - getChatSdkAccessToken: getChatSdkLinearAccessToken, - deleteChatSdkInstallation: deleteChatSdkLinearInstallation, - deleteChatSdkIdentityCache: deleteChatSdkLinearIdentityCache, - } - ); - } catch (error) { - if (error instanceof LinearWorkspaceAlreadyConnectedError) { - // The Chat SDK adapter already persisted the freshly-issued OAuth - // token under linear:installation:${organizationId} during - // handleOAuthCallback. Since the uniqueness check rejected this - // install, we must roll that state back — otherwise we overwrite the - // original installer's token and any future bot/webhook traffic for - // that workspace runs with mismatched credentials. - await linearAdapter.deleteInstallation(organizationId); - await unlinkTeamKiloUsers(bot.getState(), PLATFORM.LINEAR, organizationId); - return NextResponse.redirect( - new URL( - buildIntegrationOAuthRedirectPathFromOwner( - PLATFORM.LINEAR, - verifiedOwner, - 'error=workspace_already_connected', - verified?.returnTo - ), - APP_URL - ) + const { accessToken, refreshToken, expiresIn } = await exchangeLinearOAuthCode(code); + const identity = await fetchLinearOAuthIdentity(accessToken); + const organizationId = identity.organizationId; + // Resolve the owner's model policy before taking the workspace locks, so the + // transaction only performs the ownership check and the integration write. + const defaultModel = + owner.type === 'org' + ? await getDefaultAllowedModel(owner.id, DEFAULT_BOT_MODEL) + : DEFAULT_BOT_MODEL; + + const conflictRedirect = NextResponse.redirect( + new URL( + buildIntegrationOAuthRedirectPathFromOwner( + PLATFORM.LINEAR, + verifiedOwner, + 'error=workspace_already_connected', + verified?.returnTo + ), + APP_URL + ) + ); + + return await withWorkspaceInstallationLock(PLATFORM.LINEAR, owner, organizationId, async tx => { + const conflicting = await getConflictingLinearInstallation(owner, organizationId, tx); + if (conflicting) { + return conflictRedirect; + } + + const previousInstallation = await linearAdapter.getInstallation(organizationId); + + await linearAdapter.setInstallation(organizationId, { + accessToken, + botUserId: identity.viewerId, + expiresAt: expiresIn === null ? null : Date.now() + expiresIn * 1000, + organizationId, + ...(refreshToken ? { refreshToken } : {}), + }); + + const workspaceName = identity.organizationName ?? organizationId; + + try { + await upsertLinearInstallation( + { + owner, + organizationId, + organizationName: workspaceName, + botUserId: identity.viewerId, + }, + { + getChatSdkAccessToken: getChatSdkLinearAccessToken, + deleteChatSdkInstallation: deleteChatSdkLinearInstallation, + deleteChatSdkIdentityCache: deleteChatSdkLinearIdentityCache, + defaultModel, + }, + tx ); + } catch (error) { + if (error instanceof LinearWorkspaceAlreadyConnectedError) { + if (previousInstallation) { + await linearAdapter.setInstallation(organizationId, previousInstallation); + } else { + await linearAdapter.deleteInstallation(organizationId); + } + return conflictRedirect; + } + throw error; } - throw error; - } - const successPath = verified.returnTo - ? appendIntegrationOAuthRedirectQuery(verified.returnTo, 'success=linear_installed') - : buildIntegrationOAuthRedirectPath(PLATFORM.LINEAR, owner, 'success=installed'); + const successPath = verified.returnTo + ? appendIntegrationOAuthRedirectQuery(verified.returnTo, 'success=linear_installed') + : buildIntegrationOAuthRedirectPath(PLATFORM.LINEAR, owner, 'success=installed'); - return NextResponse.redirect(new URL(successPath, APP_URL)); + return NextResponse.redirect(new URL(successPath, APP_URL)); + }); } catch (error) { const searchParams = request.nextUrl.searchParams; const state = searchParams.get('state'); diff --git a/apps/web/src/lib/integrations/oauth/platforms/slack-callback.test.ts b/apps/web/src/lib/integrations/oauth/platforms/slack-callback.test.ts index c2e574d77db..4c7c87aff2f 100644 --- a/apps/web/src/lib/integrations/oauth/platforms/slack-callback.test.ts +++ b/apps/web/src/lib/integrations/oauth/platforms/slack-callback.test.ts @@ -3,7 +3,7 @@ process.env.NEXTAUTH_SECRET ||= 'test-nextauth-secret'; const mockEnsureOrganizationAccess = jest.fn(); const mockConsumeProviderOAuthAttempt = jest.fn(); const mockUpsertSlackInstallation = jest.fn(); -const mockHandleOAuthCallback = jest.fn(); +const mockExchangeSlackOAuthCode = jest.fn(); const mockCaptureException = jest.fn(); const mockCaptureMessage = jest.fn(); @@ -22,10 +22,15 @@ jest.mock('@/lib/bot', () => ({ bot: { initialize: jest.fn(async () => {}), getAdapter: () => ({ - handleOAuthCallback: (...args: unknown[]) => mockHandleOAuthCallback(...args), + getInstallation: jest.fn(async () => null), + setInstallation: jest.fn(async () => {}), + deleteInstallation: jest.fn(async () => {}), }), }, })); +jest.mock('@/lib/slack-bot/model-allow-list', () => ({ + getDefaultAllowedModel: jest.fn(async () => 'test/default-model'), +})); jest.mock('@/lib/integrations/provider-oauth-attempts', () => ({ beginProviderOAuthAttempt: jest.fn(), cancelProviderOAuthAttempt: jest.fn(), @@ -40,6 +45,8 @@ jest.mock('@/lib/integrations/slack-service', () => { } return { SlackWorkspaceAlreadyConnectedError, + exchangeSlackOAuthCode: (...args: unknown[]) => mockExchangeSlackOAuthCode(...args), + getConflictingSlackInstallation: jest.fn(async () => null), upsertSlackInstallation: (...args: unknown[]) => mockUpsertSlackInstallation(...args), }; }); @@ -87,7 +94,7 @@ describe('handleSlackOAuthCallback organization authority', () => { mockEnsureOrganizationAccess.mockResolvedValue('owner'); mockConsumeProviderOAuthAttempt.mockResolvedValue(true); mockUpsertSlackInstallation.mockResolvedValue({ id: 'integration-1' }); - mockHandleOAuthCallback.mockResolvedValue({ teamId: 'T123', installation }); + mockExchangeSlackOAuthCode.mockResolvedValue({ installationId: 'T123', installation }); }); it('refuses an organization completion by a plain member', async () => { @@ -105,7 +112,7 @@ describe('handleSlackOAuthCallback organization authority', () => { ORGANIZATION_MANAGE_ROLES ); expect(mockUpsertSlackInstallation).not.toHaveBeenCalled(); - expect(mockHandleOAuthCallback).not.toHaveBeenCalled(); + expect(mockExchangeSlackOAuthCode).not.toHaveBeenCalled(); expect(mockCaptureException).not.toHaveBeenCalled(); }); @@ -121,50 +128,26 @@ describe('handleSlackOAuthCallback organization authority', () => { 'permission_required' ); expect(mockUpsertSlackInstallation).not.toHaveBeenCalled(); - expect(mockHandleOAuthCallback).not.toHaveBeenCalled(); - }); - - it('completes an organization install for an admin and authorizes a workspace change', async () => { - mockEnsureOrganizationAccess.mockResolvedValue('admin'); - - const { response } = await callCallback(`org_${ORGANIZATION_ID}`); - - expect(response.status).toBe(307); - expect(new URL(response.headers.get('location') ?? '').pathname).toBe( - '/organizations/' + ORGANIZATION_ID + '/integrations/slack' - ); - expect(mockUpsertSlackInstallation).toHaveBeenCalledWith({ - owner: { type: 'org', id: ORGANIZATION_ID }, - teamId: 'T123', - installation, - allowWorkspaceRebind: true, - }); + expect(mockExchangeSlackOAuthCode).not.toHaveBeenCalled(); }); - it('keeps a same-workspace re-install working for an organization admin', async () => { + it('completes an organization installation', async () => { mockEnsureOrganizationAccess.mockResolvedValue('owner'); - mockHandleOAuthCallback.mockResolvedValue({ teamId: 'T123', installation }); const { response } = await callCallback(`org_${ORGANIZATION_ID}`); expect(new URL(response.headers.get('location') ?? '').searchParams.get('success')).toBe( 'installed' ); - expect(mockUpsertSlackInstallation).toHaveBeenCalledWith( - expect.objectContaining({ teamId: 'T123', allowWorkspaceRebind: true }) - ); }); it('does not require organization authority for a personal completion', async () => { const { response } = await callCallback(`user_${USER_ID}`); expect(new URL(response.headers.get('location') ?? '').pathname).toBe('/integrations/slack'); + expect(new URL(response.headers.get('location') ?? '').searchParams.get('success')).toBe( + 'installed' + ); expect(mockEnsureOrganizationAccess).not.toHaveBeenCalled(); - expect(mockUpsertSlackInstallation).toHaveBeenCalledWith({ - owner: { type: 'user', id: USER_ID }, - teamId: 'T123', - installation, - allowWorkspaceRebind: false, - }); }); }); diff --git a/apps/web/src/lib/integrations/oauth/platforms/slack-callback.ts b/apps/web/src/lib/integrations/oauth/platforms/slack-callback.ts index 0563d9da9fe..9601104ba37 100644 --- a/apps/web/src/lib/integrations/oauth/platforms/slack-callback.ts +++ b/apps/web/src/lib/integrations/oauth/platforms/slack-callback.ts @@ -6,6 +6,8 @@ import { ensureOrganizationAccess } from '@kilocode/web-shared/routers/organizat import { captureException, captureMessage } from '@sentry/nextjs'; import { getCallbackReportParams } from '@/lib/integrations/oauth/callback-params'; import { + exchangeSlackOAuthCode, + getConflictingSlackInstallation, SlackWorkspaceAlreadyConnectedError, upsertSlackInstallation, } from '@/lib/integrations/slack-service'; @@ -13,7 +15,6 @@ import { verifyOAuthState } from '@/lib/integrations/oauth-state'; import { APP_URL } from '@kilocode/web-shared/lib/constants'; import { bot } from '@/lib/bot'; import { PLATFORM } from '@/lib/integrations/core/constants'; -import { getPlatformOAuthCallbackUrl } from '@/lib/integrations/oauth/urls'; import { appendIntegrationOAuthRedirectQuery, buildIntegrationOAuthRedirectPath, @@ -23,8 +24,9 @@ import { organizationAccessDenialErrorCode, } from '@/lib/integrations/oauth/common'; import { consumeProviderOAuthAttempt } from '@/lib/integrations/provider-oauth-attempts'; - -const SLACK_REDIRECT_URI = getPlatformOAuthCallbackUrl(PLATFORM.SLACK); +import { withWorkspaceInstallationLock } from '@/lib/integrations/oauth/workspace-installation-lock'; +import { getDefaultAllowedModel } from '@/lib/slack-bot/model-allow-list'; +import { DEFAULT_BOT_MODEL } from '@/lib/bot/constants'; /** * Slack OAuth Callback @@ -145,47 +147,67 @@ export async function handleSlackOAuthCallback(request: NextRequest) { throw new Error('Slack OAuth attempt is invalid, expired, or already used'); } - // 7. Let the Chat SDK exchange the code and seed its installation state await bot.initialize(); const slackAdapter = bot.getAdapter('slack'); - const url = new URL(request.url); - url.searchParams.set('redirect_uri', SLACK_REDIRECT_URI); - const patchedRequest = new Request(url, request); - const { teamId, installation } = await slackAdapter.handleOAuthCallback(patchedRequest); - - // 8. Store installation in database. The organization path is the only caller - // allowed to move an existing organization connection to another workspace, - // and only after the organization-management check above. - try { - await upsertSlackInstallation({ - owner, - teamId, - installation, - allowWorkspaceRebind: owner.type === 'org', - }); - } catch (error) { - if (error instanceof SlackWorkspaceAlreadyConnectedError) { - return NextResponse.redirect( - new URL( - buildIntegrationOAuthRedirectPathFromState( - PLATFORM.SLACK, - state, - 'error=workspace_already_connected' - ), - APP_URL - ) - ); + + const { installationId, installation } = await exchangeSlackOAuthCode(code); + // Resolve the owner's model policy before taking the workspace locks, so the + // transaction only performs the ownership check and the integration write. + const defaultModel = + owner.type === 'org' + ? await getDefaultAllowedModel(owner.id, DEFAULT_BOT_MODEL) + : DEFAULT_BOT_MODEL; + + const conflictRedirect = NextResponse.redirect( + new URL( + buildIntegrationOAuthRedirectPathFromState( + PLATFORM.SLACK, + state, + 'error=workspace_already_connected' + ), + APP_URL + ) + ); + + return await withWorkspaceInstallationLock(PLATFORM.SLACK, owner, installationId, async tx => { + const conflicting = await getConflictingSlackInstallation(owner, installationId, tx); + if (conflicting) { + return conflictRedirect; } - throw error; - } + const previousInstallation = await slackAdapter.getInstallation(installationId); + await slackAdapter.setInstallation(installationId, installation); + + try { + await upsertSlackInstallation( + { + owner, + teamId: installationId, + installation, + allowWorkspaceRebind: owner.type === 'org', + defaultModel, + }, + tx + ); + } catch (error) { + if (error instanceof SlackWorkspaceAlreadyConnectedError) { + if (previousInstallation) { + await slackAdapter.setInstallation(installationId, previousInstallation); + } else { + await slackAdapter.deleteInstallation(installationId); + } + return conflictRedirect; + } + + throw error; + } - // 9. Redirect to success page - const successPath = verified.returnTo - ? appendIntegrationOAuthRedirectQuery(verified.returnTo, 'success=slack_installed') - : buildIntegrationOAuthRedirectPath(PLATFORM.SLACK, owner, 'success=installed'); + const successPath = verified.returnTo + ? appendIntegrationOAuthRedirectQuery(verified.returnTo, 'success=slack_installed') + : buildIntegrationOAuthRedirectPath(PLATFORM.SLACK, owner, 'success=installed'); - return NextResponse.redirect(new URL(successPath, APP_URL)); + return NextResponse.redirect(new URL(successPath, APP_URL)); + }); } catch (error) { console.error('Error handling Slack OAuth callback:', error); diff --git a/apps/web/src/lib/integrations/oauth/workspace-installation-lock.ts b/apps/web/src/lib/integrations/oauth/workspace-installation-lock.ts new file mode 100644 index 00000000000..e3ef07954de --- /dev/null +++ b/apps/web/src/lib/integrations/oauth/workspace-installation-lock.ts @@ -0,0 +1,20 @@ +import { db, type DrizzleTransaction } from '@kilocode/web-shared/lib/drizzle'; +import { sql } from 'drizzle-orm'; +import type { Owner } from '@/lib/integrations/core/types'; + +export async function withWorkspaceInstallationLock( + platform: 'linear' | 'slack', + owner: Owner, + installationId: string, + operation: (tx: DrizzleTransaction) => Promise +): Promise { + return db.transaction(async tx => { + await tx.execute(sql`SET LOCAL lock_timeout = '5s'`); + await tx.execute(sql`SET LOCAL statement_timeout = '30s'`); + const ownerKey = `integration-installation-owner:${platform}:${owner.type}:${owner.id}`; + await tx.execute(sql`SELECT pg_advisory_xact_lock(hashtextextended(${ownerKey}, 0))`); + const key = `integration-installation:${platform}:${installationId}`; + await tx.execute(sql`SELECT pg_advisory_xact_lock(hashtextextended(${key}, 0))`); + return operation(tx); + }); +} diff --git a/apps/web/src/lib/integrations/platforms/slack/credential-store.test.ts b/apps/web/src/lib/integrations/platforms/slack/credential-store.test.ts index 63ed84a333b..8b7ca6d0296 100644 --- a/apps/web/src/lib/integrations/platforms/slack/credential-store.test.ts +++ b/apps/web/src/lib/integrations/platforms/slack/credential-store.test.ts @@ -1,5 +1,5 @@ import { afterEach, beforeAll, beforeEach, describe, expect, it } from '@jest/globals'; -import { generateKeyPairSync } from 'node:crypto'; +import { generateKeyPairSync, randomUUID } from 'node:crypto'; import { eq } from 'drizzle-orm'; const testKeyPair = generateKeyPairSync('rsa', { @@ -99,6 +99,75 @@ describe('writeSlackCredential', () => { expect(decryptSlackBotToken(stored, owner)).toBe('xoxb-first-token'); }); + it('writes credentials in the supplied installation transaction', async () => { + await db.delete(platform_integrations).where(eq(platform_integrations.id, integrationId)); + const rollback = new Error('Rollback test transaction'); + + await expect( + db.transaction(async tx => { + const [integration] = await tx + .insert(platform_integrations) + .values({ + owned_by_user_id: userId, + platform: 'slack', + integration_type: 'oauth', + platform_installation_id: teamId, + platform_account_id: teamId, + platform_account_login: 'Test Workspace', + integration_status: 'active', + }) + .returning(); + const written = await writeSlackCredential( + { + integrationId: integration.id, + slackTeamId: teamId, + owner, + botToken: 'test-installation-token', + }, + tx + ); + + const [stored] = await tx + .select() + .from(slack_oauth_credentials) + .where(eq(slack_oauth_credentials.id, written.id)); + expect(stored.platform_integration_id).toBe(integration.id); + integrationId = integration.id; + throw rollback; + }) + ).rejects.toBe(rollback); + + expect(await getSlackCredentialByIntegrationId(integrationId)).toBeNull(); + }); + + it('commits the installation after a credential savepoint failure', async () => { + await db.transaction(async tx => { + await expect( + writeSlackCredential( + { + integrationId: randomUUID(), + slackTeamId: teamId, + owner, + botToken: 'test-installation-token', + }, + tx + ) + ).rejects.toMatchObject({ cause: { code: '23503' } }); + + await tx + .update(platform_integrations) + .set({ platform_account_login: 'Saved Workspace' }) + .where(eq(platform_integrations.id, integrationId)); + }); + + const [stored] = await db + .select() + .from(platform_integrations) + .where(eq(platform_integrations.id, integrationId)); + expect(stored.platform_account_login).toBe('Saved Workspace'); + expect(await getSlackCredentialByIntegrationId(integrationId)).toBeNull(); + }); + it('bumps credential_version on replacement and keeps the new token readable', async () => { await writeSlackCredential({ integrationId, diff --git a/apps/web/src/lib/integrations/platforms/slack/credential-store.ts b/apps/web/src/lib/integrations/platforms/slack/credential-store.ts index 36fc017a96a..1eabd085ee3 100644 --- a/apps/web/src/lib/integrations/platforms/slack/credential-store.ts +++ b/apps/web/src/lib/integrations/platforms/slack/credential-store.ts @@ -7,7 +7,7 @@ import { buildSlackCredentialLockKey, type SlackCredentialOwner, } from '@kilocode/worker-utils/slack-credential'; -import { db } from '@kilocode/web-shared/lib/drizzle'; +import { db, type DrizzleTransaction } from '@kilocode/web-shared/lib/drizzle'; import { decryptSlackCredentialSecret, encryptSlackCredentialSecret, @@ -58,9 +58,10 @@ export type SlackCredentialWriteOutcome = * interleave a read of the current version with a write of the next one. */ export async function writeSlackCredential( - input: WriteSlackCredentialInput + input: WriteSlackCredentialInput, + database: typeof db | DrizzleTransaction = db ): Promise { - const outcome = await upsertSlackCredential(input, { onlyIfAbsent: false }); + const outcome = await upsertSlackCredential(input, { onlyIfAbsent: false }, database); if (outcome.status === 'skipped_existing') { // Unreachable: only `onlyIfAbsent` can skip. Asserted rather than cast so a future // change to the skip conditions cannot silently drop a credential write. @@ -86,9 +87,10 @@ export async function createSlackCredentialIfAbsent( async function upsertSlackCredential( input: WriteSlackCredentialInput, - { onlyIfAbsent }: { onlyIfAbsent: boolean } + { onlyIfAbsent }: { onlyIfAbsent: boolean }, + database: typeof db | DrizzleTransaction = db ): Promise { - return db.transaction(async tx => { + return database.transaction(async tx => { await tx.execute( sql`SELECT pg_advisory_xact_lock(hashtextextended(${buildSlackCredentialLockKey(input.integrationId)}, 0))` ); diff --git a/apps/web/src/lib/integrations/slack-service.test.ts b/apps/web/src/lib/integrations/slack-service.test.ts index 2731573aa3b..7817849a633 100644 --- a/apps/web/src/lib/integrations/slack-service.test.ts +++ b/apps/web/src/lib/integrations/slack-service.test.ts @@ -462,42 +462,6 @@ describe('upsertSlackInstallation', () => { ); }); - it('dual-writes the bot token into the encrypted credential store on a new install', async () => { - mockLimit.mockResolvedValueOnce([]).mockResolvedValueOnce([]); - - const installation = { - botToken: 'xoxb-new-token', - botUserId: 'U_NEW_BOT', - teamName: 'Kilo Team', - } satisfies SlackInstallation; - - await upsertSlackInstallation({ owner, teamId: 'T123', installation }); - - expect(mockWriteSlackCredential).toHaveBeenCalledWith({ - integrationId: 'integration-1', - slackTeamId: 'T123', - owner, - botToken: 'xoxb-new-token', - botUserId: 'U_NEW_BOT', - }); - }); - - it('dual-writes the bot token when refreshing an existing install', async () => { - mockLimit.mockResolvedValue([buildSlackIntegration()]); - - const installation = { - botToken: 'xoxb-rotated-token', - botUserId: 'U_NEW_BOT', - teamName: 'Kilo Team', - } satisfies SlackInstallation; - - await upsertSlackInstallation({ owner, teamId: 'T123', installation }); - - expect(mockWriteSlackCredential).toHaveBeenCalledWith( - expect.objectContaining({ botToken: 'xoxb-rotated-token', slackTeamId: 'T123' }) - ); - }); - // The encrypted store is write-only until reads move onto it, so a failure there // must not take down Slack installs. This expectation flips in the step that // repoints the read paths. @@ -555,6 +519,42 @@ describe('upsertSlackInstallation organization workspace binding', () => { mockGetDefaultAllowedModel.mockResolvedValue(DEFAULT_BOT_MODEL); }); + it('uses the supplied default model without resolving organization policy again', async () => { + mockLimit.mockResolvedValueOnce([organizationIntegration('T123')]).mockResolvedValueOnce([]); + + await upsertSlackInstallation({ + owner: organizationOwner, + teamId: 'T123', + installation, + defaultModel: 'test/default-model', + }); + + expect(mockGetDefaultAllowedModel).not.toHaveBeenCalled(); + expect(mockUpdateSet).toHaveBeenCalledWith( + expect.objectContaining({ + metadata: expect.objectContaining({ model_slug: 'test/default-model' }), + }) + ); + }); + + it('stores no model when the supplied model is null', async () => { + mockLimit.mockResolvedValueOnce([organizationIntegration('T123')]).mockResolvedValueOnce([]); + + await upsertSlackInstallation({ + owner: organizationOwner, + teamId: 'T123', + installation, + defaultModel: null, + }); + + expect(mockGetDefaultAllowedModel).not.toHaveBeenCalled(); + expect(mockUpdateSet).toHaveBeenCalledWith( + expect.objectContaining({ + metadata: expect.not.objectContaining({ model_slug: expect.anything() }), + }) + ); + }); + it('refuses to move an organization connection without caller authorization', async () => { mockLimit.mockResolvedValueOnce([organizationIntegration('T_OLD')]).mockResolvedValueOnce([]); diff --git a/apps/web/src/lib/integrations/slack-service.ts b/apps/web/src/lib/integrations/slack-service.ts index aabb99c7246..475ef3a716f 100644 --- a/apps/web/src/lib/integrations/slack-service.ts +++ b/apps/web/src/lib/integrations/slack-service.ts @@ -1,5 +1,5 @@ import 'server-only'; -import { db } from '@kilocode/web-shared/lib/drizzle'; +import { db, type DrizzleTransaction } from '@kilocode/web-shared/lib/drizzle'; import type { PlatformIntegration } from '@kilocode/db/schema'; import { platform_integrations } from '@kilocode/db/schema'; import { eq, and, isNull } from 'drizzle-orm'; @@ -7,7 +7,7 @@ import { TRPCError } from '@trpc/server'; import type { Owner } from '@/lib/integrations/core/types'; import { INTEGRATION_STATUS, PLATFORM } from '@/lib/integrations/core/constants'; import { getPlatformOAuthCallbackUrl } from '@/lib/integrations/oauth/urls'; -import { SLACK_CLIENT_ID } from '@kilocode/web-shared/lib/config.server'; +import { SLACK_CLIENT_ID, SLACK_CLIENT_SECRET } from '@kilocode/web-shared/lib/config.server'; import { WebClient } from '@slack/web-api'; import type { SlackInstallation } from '@chat-adapter/slack'; import { getDefaultAllowedModel } from '@/lib/slack-bot/model-allow-list'; @@ -97,6 +97,59 @@ export function getSlackOAuthUrl(state: string): string { return `https://slack.com/oauth/v2/authorize?${params.toString()}`; } +export type SlackOAuthExchangeResult = { + installationId: string; + enterpriseId: string | undefined; + isEnterpriseInstall: boolean; + installation: SlackInstallation; +}; + +/** + * Exchange a Slack OAuth V2 authorization code for a workspace installation. + * + * Produces the same installation record `slackAdapter.handleOAuthCallback` + * persists, but without writing it, so the callback can resolve which workspace + * is being connected and check its ownership first. Enterprise Grid org-wide + * installs are keyed on the enterprise ID, matching the adapter. + */ +export async function exchangeSlackOAuthCode(code: string): Promise { + if (!SLACK_CLIENT_ID || !SLACK_CLIENT_SECRET) { + throw new Error('SLACK_CLIENT_ID / SLACK_CLIENT_SECRET are not configured'); + } + + const result = await new WebClient().oauth.v2.access({ + client_id: SLACK_CLIENT_ID, + client_secret: SLACK_CLIENT_SECRET, + code, + redirect_uri: SLACK_REDIRECT_URI, + }); + + const isEnterpriseInstall = Boolean(result.is_enterprise_install); + const enterpriseId = result.enterprise?.id; + const installationId = isEnterpriseInstall ? enterpriseId : result.team?.id; + + if (!result.ok || !result.access_token || !installationId) { + throw new Error( + isEnterpriseInstall + ? 'Slack OAuth failed: missing access_token or enterprise id' + : 'Slack OAuth failed: missing access_token or team id' + ); + } + + return { + installationId, + enterpriseId, + isEnterpriseInstall, + installation: { + botToken: result.access_token, + botUserId: result.bot_user_id, + teamName: result.team?.name ?? result.enterprise?.name, + ...(enterpriseId ? { enterpriseId } : {}), + ...(isEnterpriseInstall ? { isEnterpriseInstall } : {}), + }, + }; +} + /** * Revoke Slack access token */ @@ -117,8 +170,11 @@ export async function revokeSlackToken(accessToken: string): Promise { * For user-owned integrations, we explicitly check that owned_by_organization_id is null * to avoid returning organization-owned integrations */ -export async function getInstallation(owner: Owner): Promise { - const [integration] = await db +export async function getInstallation( + owner: Owner, + database: typeof db | DrizzleTransaction = db +): Promise { + const [integration] = await database .select() .from(platform_integrations) .where( @@ -154,11 +210,18 @@ function isOwnedBy(integration: PlatformIntegration, owner: Owner): boolean { : integration.owned_by_organization_id === owner.id && integration.owned_by_user_id === null; } -async function getConflictingSlackInstallation( +/** + * Find a Slack `platform_integrations` row for `teamId` that belongs to a + * different Kilo owner. Callers use this before persisting a workspace + * installation so a workspace owned elsewhere is rejected without writing to + * the shared Chat SDK installation state. + */ +export async function getConflictingSlackInstallation( owner: Owner, - teamId: string + teamId: string, + database: typeof db | DrizzleTransaction = db ): Promise { - const integrations = await db + const integrations = await database .select() .from(platform_integrations) .where( @@ -205,30 +268,35 @@ export function getOwnerFromInstallation(integration: PlatformIntegration): Owne /** * Dual-write the bot token into the encrypted `slack_oauth_credentials` store. * - * Deliberately best-effort for now: nothing reads that store yet, so an unconfigured - * or failing encryption keyset must not break Slack installs. Step 3 moves the read - * paths onto the store, at which point this write becomes required and must move - * inside the same transaction as the `platform_integrations` write. + * Deliberately best-effort while readers still use installation metadata. A + * supplied installation transaction also contains this write, with a savepoint + * keeping a credential failure from aborting the installation. */ -async function mirrorSlackCredential({ - integration, - owner, - teamId, - installation, -}: { - integration: PlatformIntegration; - owner: Owner; - teamId: string; - installation: SlackInstallation; -}): Promise { +async function mirrorSlackCredential( + { + integration, + owner, + teamId, + installation, + }: { + integration: PlatformIntegration; + owner: Owner; + teamId: string; + installation: SlackInstallation; + }, + database: typeof db | DrizzleTransaction +): Promise { try { - await writeSlackCredential({ - integrationId: integration.id, - slackTeamId: teamId, - owner, - botToken: installation.botToken, - botUserId: installation.botUserId ?? null, - }); + await writeSlackCredential( + { + integrationId: integration.id, + slackTeamId: teamId, + owner, + botToken: installation.botToken, + botUserId: installation.botUserId ?? null, + }, + database + ); } catch (error) { captureException(error, { level: 'error', @@ -245,23 +313,32 @@ async function mirrorSlackCredential({ * it to another Slack workspace is refused unless the caller passes * `allowWorkspaceRebind` after verifying organization-management authority. */ -export async function upsertSlackInstallation({ - owner, - teamId, - installation, - allowWorkspaceRebind = false, -}: { - owner: Owner; - teamId: string; - installation: SlackInstallation; - allowWorkspaceRebind?: boolean; -}): Promise { - const existing = await getInstallation(owner); +export async function upsertSlackInstallation( + { + owner, + teamId, + installation, + allowWorkspaceRebind = false, + defaultModel: suppliedDefaultModel, + }: { + owner: Owner; + teamId: string; + installation: SlackInstallation; + allowWorkspaceRebind?: boolean; + /** + * Model resolved by the caller before the install transaction. `null` means + * the owner's policy allows no candidate model; `undefined` resolves it here. + */ + defaultModel?: string | null; + }, + database: typeof db | DrizzleTransaction = db +): Promise { + const existing = await getInstallation(owner, database); const teamName = installation.teamName || 'Unknown Team'; // Legacy rows can carry the workspace id only in `platform_account_id`. const boundTeamId = existing?.platform_installation_id ?? existing?.platform_account_id ?? null; - const conflicting = await getConflictingSlackInstallation(owner, teamId); + const conflicting = await getConflictingSlackInstallation(owner, teamId, database); if (conflicting) { throw new SlackWorkspaceAlreadyConnectedError(teamName); } @@ -281,9 +358,11 @@ export async function upsertSlackInstallation({ // stores no `model_slug` and the UI keeps its "Select a model" state. // For user integrations, use the shared bot default model. const defaultModel = - owner.type === 'org' - ? await getDefaultAllowedModel(owner.id, DEFAULT_BOT_MODEL) - : DEFAULT_BOT_MODEL; + suppliedDefaultModel !== undefined + ? suppliedDefaultModel + : owner.type === 'org' + ? await getDefaultAllowedModel(owner.id, DEFAULT_BOT_MODEL) + : DEFAULT_BOT_MODEL; const existingModelSlug = existing?.metadata && @@ -307,7 +386,7 @@ export async function upsertSlackInstallation({ if (existing) { try { - const [updated] = await db + const [updated] = await database .update(platform_integrations) .set({ platform_installation_id: teamId, @@ -323,7 +402,7 @@ export async function upsertSlackInstallation({ .where(eq(platform_integrations.id, existing.id)) .returning(); - await mirrorSlackCredential({ integration: updated, owner, teamId, installation }); + await mirrorSlackCredential({ integration: updated, owner, teamId, installation }, database); return updated; } catch (error) { @@ -335,7 +414,7 @@ export async function upsertSlackInstallation({ } try { - const [created] = await db + const [created] = await database .insert(platform_integrations) .values({ owned_by_user_id: owner.type === 'user' ? owner.id : null, @@ -352,7 +431,7 @@ export async function upsertSlackInstallation({ }) .returning(); - await mirrorSlackCredential({ integration: created, owner, teamId, installation }); + await mirrorSlackCredential({ integration: created, owner, teamId, installation }, database); return created; } catch (error) { diff --git a/docs/security-architecture-overview.md b/docs/security-architecture-overview.md index 4147e7cf0d9..26a4d86b183 100644 --- a/docs/security-architecture-overview.md +++ b/docs/security-architecture-overview.md @@ -234,6 +234,8 @@ Kilo Cloud distinguishes among several authorization contexts: Repository code includes token-version checks in shared JWT validation paths and revocation-pepper checks in selected web and Worker authorization paths. The exact lifetime, rotation cadence, and emergency-revocation process for production credentials are operational controls managed outside this overview. +Slack and Linear installation callbacks serialize persistence by provider, owner, and workspace using primary-PostgreSQL transaction advisory locks, acquired in owner-then-workspace order. Each transaction sets a five-second lock timeout and a thirty-second SQL statement timeout. Ownership reads and integration-row writes share that transaction; installation-state updates and restoration complete while the locks are held. Organization model defaults are resolved before entering the transaction. Slack credential mirroring uses a transaction savepoint and retains its best-effort behavior. + ### 5.3 Tenant and execution scoping Application data is commonly scoped to either a user or an organization. Cloud Agent durable state is session-scoped. KiloClaw runtimes are owner-scoped rather than shared global assistant processes. Some product areas are actively evolving from user-specific instances toward broader owner-scoped multi-instance support; this document describes the stable security boundary rather than transitional routing details.