Skip to content

Commit 62f609b

Browse files
waleedlatif1waleedlatif1
authored
fix(slack): refuse binding a Search bot to a different app or workspace than members (#9109)
* fix(slack): refuse binding a Search bot to a different app or workspace than members * fix(slack): check Search app availability on the caller's transaction
1 parent e25a6cf commit 62f609b

8 files changed

Lines changed: 185 additions & 62 deletions

File tree

‎apps/sim/lib/credential-groups/__integration__/slack-shared-member-accounts.integration.ts‎

Lines changed: 103 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -47,11 +47,14 @@ import { encryptManagedOAuthTokenSet } from '@/lib/credentials/managed-oauth'
4747
import type { DbOrTx, DbTransaction } from '@/lib/db/types'
4848
import { approveSearchIntegration } from '@/lib/knowledge/application/search-integrations'
4949
import {
50+
configureSlackSearchInstallation,
5051
listSlackSearchInstallations,
5152
removeSlackSearchInstallation,
5253
} from '@/lib/knowledge/application/slack-search/installations'
5354
import { revokeSlackSearchAccess } from '@/lib/knowledge/application/slack-search/lifecycle'
55+
import { SLACK_CUSTOM_BOT_PROVIDER_ID, SLACK_CUSTOM_BOT_SECRET_TYPE } from '@/lib/oauth/types'
5456
import { defaultLiveSearchPolicy } from '@/lib/sim-search/live/policy-schema'
57+
import { SLACK_SHARED_SEARCH_BOT_SCOPES } from '@/lib/slack-search/constants'
5558
import { getSharedSlackSearchAppConfiguration } from '@/lib/slack-search/shared-app-env'
5659

5760
vi.mock('@/lib/core/config/env-flags', async (importOriginal) => ({
@@ -156,8 +159,8 @@ describe('Slack member accounts on the shared app without a Search bot', () => {
156159
input: { state, code: 'fixture-code' },
157160
})
158161

159-
async function installBot(appId: string, teamId: string, executor: DbOrTx = db) {
160-
const secret = (await encryptSecret('fixture-bot')).encrypted
162+
async function upsertSlackApp(appId: string, executor: DbOrTx = db) {
163+
const secret = (await encryptSecret('fixture-app')).encrypted
161164
await executor
162165
.insert(slackApp)
163166
.values(
@@ -174,6 +177,11 @@ describe('Slack member accounts on the shared app without a Search bot', () => {
174177
}
175178
)
176179
.onConflictDoNothing()
180+
}
181+
182+
async function installBot(appId: string, teamId: string, executor: DbOrTx = db) {
183+
await upsertSlackApp(appId, executor)
184+
const secret = (await encryptSecret('fixture-bot')).encrypted
177185
const botId = generateId()
178186
await executor.insert(credential).values({
179187
id: botId,
@@ -281,6 +289,99 @@ describe('Slack member accounts on the shared app without a Search bot', () => {
281289
restoreSlackHttp = () => spy.mockRestore()
282290
}
283291

292+
/** Answers bot verification as a bot of `appId` installed in `teamId`. */
293+
function provideSlackBot(appId: string, teamId: string) {
294+
restoreSlackHttp?.()
295+
const spy = vi.spyOn(globalThis, 'fetch').mockImplementation(async (input) => {
296+
const url = new URL(String(input))
297+
switch (`${url.origin}${url.pathname}`) {
298+
case 'https://slack.com/api/auth.test':
299+
return Response.json(
300+
{ ok: true, team_id: teamId, team: 'Fixture team', user_id: 'UBOT', bot_id: 'BBOT' },
301+
{ headers: { 'x-oauth-scopes': SLACK_SHARED_SEARCH_BOT_SCOPES.join(',') } }
302+
)
303+
case 'https://slack.com/api/bots.info':
304+
return Response.json({ ok: true, bot: { id: 'BBOT', app_id: appId } })
305+
default:
306+
throw new Error(`Unexpected Slack fixture request: ${url}`)
307+
}
308+
})
309+
restoreSlackHttp = () => spy.mockRestore()
310+
}
311+
312+
async function connectMembersOnSharedApp() {
313+
const groupId = (await approveSlack()).memberAccounts?.groupId
314+
if (!groupId) throw new Error('Approval did not provision organization accounts')
315+
const pending = new URL((await startSetup(groupId)).authorizationUrl)
316+
provideSlackConsent(MEMBER_TEAM)
317+
await completeSetup(pending.searchParams.get('state') ?? '')
318+
}
319+
320+
/** A bot credential left without a Search installation, as after removing its binding. */
321+
async function insertLeftoverBot(appId: string, teamId: string) {
322+
await upsertSlackApp(appId)
323+
const { encrypted } = await encryptSecret(
324+
JSON.stringify({
325+
type: SLACK_CUSTOM_BOT_SECRET_TYPE,
326+
botToken: 'xoxb-fixture',
327+
teamId,
328+
botUserId: 'UBOT',
329+
teamName: 'Fixture team',
330+
})
331+
)
332+
const credentialId = generateId()
333+
await db.insert(credential).values({
334+
id: credentialId,
335+
organizationId: ids.organization,
336+
type: 'service_account',
337+
providerId: SLACK_CUSTOM_BOT_PROVIDER_ID,
338+
displayName: 'Leftover Slack bot',
339+
createdBy: ids.owner,
340+
encryptedServiceAccountKey: encrypted,
341+
slackAppId: appId,
342+
})
343+
return credentialId
344+
}
345+
346+
const enableBot = (credentialId: string) =>
347+
configureSlackSearchInstallation.execute({
348+
principal: principal(),
349+
input: { organizationId: ids.organization, credentialId, enabled: true },
350+
})
351+
352+
const installations = () =>
353+
db
354+
.select()
355+
.from(slackSearchInstallation)
356+
.where(eq(slackSearchInstallation.organizationId, ids.organization))
357+
358+
it('refuses to bind a leftover bot to a different app or workspace than members', async () => {
359+
await connectMembersOnSharedApp()
360+
const credentialId = await insertLeftoverBot(CUSTOM_APP_ID, OTHER_TEAM)
361+
provideSlackBot(CUSTOM_APP_ID, OTHER_TEAM)
362+
await expect(enableBot(credentialId)).rejects.toMatchObject({
363+
code: 'conflict',
364+
message: 'Install the same Slack app and workspace connected by your members',
365+
})
366+
expect(await installations()).toEqual([])
367+
})
368+
369+
it('binds a leftover shared-app bot in the members workspace', async () => {
370+
await connectMembersOnSharedApp()
371+
const credentialId = await insertLeftoverBot(SHARED_APP_ID, MEMBER_TEAM)
372+
provideSlackBot(SHARED_APP_ID, MEMBER_TEAM)
373+
const created = await enableBot(credentialId)
374+
expect(await installations()).toEqual([
375+
expect.objectContaining({
376+
id: created.id,
377+
credentialId,
378+
appId: SHARED_APP_ID,
379+
teamId: MEMBER_TEAM,
380+
enabled: true,
381+
}),
382+
])
383+
})
384+
284385
it('approves Slack, verifies the workspace from the admin grant, and resolves member sign-in', async () => {
285386
const approval = await approveSlack()
286387
expect(approval).toMatchObject({ approved: true })

‎apps/sim/lib/credential-groups/organization-slack-app.ts‎

Lines changed: 52 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -5,6 +5,7 @@ import { OrchestrationError } from '@/lib/core/orchestration/types'
55
import {
66
decryptCredentialGroupProviderConfiguration,
77
encryptCredentialGroupProviderConfiguration,
8+
type SlackCredentialGroupConfiguration,
89
} from '@/lib/credential-groups/provider-configuration'
910
import { acquireAdvisoryXactLock } from '@/lib/db/advisory-locks'
1011
import type { DbOrTx, DbTransaction } from '@/lib/db/types'
@@ -49,6 +50,57 @@ export async function loadOrganizationSlackMemberApps(
4950
return configurations.filter((row) => row.configuration.slack !== undefined)
5051
}
5152

53+
/**
54+
* Resolves the one Slack app and workspace the organization's members connect through, with the
55+
* union of their scopes. Fails when member configurations reference more than one app.
56+
*/
57+
export async function resolveOrganizationSlackMemberApp(
58+
organizationId: string,
59+
executor: DbOrTx = db
60+
) {
61+
const rows = await loadOrganizationSlackMemberApps(organizationId, executor)
62+
const configurations = rows.flatMap((row) =>
63+
row.configuration.slack ? [row.configuration.slack] : []
64+
)
65+
const first = configurations[0]
66+
if (
67+
first &&
68+
configurations.some(
69+
(configuration) =>
70+
configuration.appId !== first.appId || configuration.teamId !== first.teamId
71+
)
72+
)
73+
throw new OrchestrationError(
74+
'conflict',
75+
'Organization Slack member accounts reference multiple apps. Consolidate them before configuring Search.'
76+
)
77+
return {
78+
app: first ? { appId: first.appId, teamId: first.teamId } : null,
79+
scopes: [...new Set(configurations.flatMap((configuration) => configuration.scopes))],
80+
}
81+
}
82+
83+
type SlackAppIdentity = Pick<SlackCredentialGroupConfiguration, 'appId' | 'teamId'>
84+
85+
/**
86+
* Refuses a Search bot outside the members' Slack workspace, or on a different app unless the
87+
* bot runs on the shared Sim Search app.
88+
*/
89+
export function requireSearchBotMatchesMemberApp(
90+
memberApp: SlackAppIdentity | null | undefined,
91+
bot: SlackAppIdentity,
92+
sharedBot: boolean
93+
) {
94+
if (
95+
memberApp &&
96+
((!sharedBot && memberApp.appId !== bot.appId) || memberApp.teamId !== bot.teamId)
97+
)
98+
throw new OrchestrationError(
99+
'conflict',
100+
'Install the same Slack app and workspace connected by your members'
101+
)
102+
}
103+
52104
/** Replaces legacy duplicated secrets without changing member grants or their scope policies. */
53105
export async function adoptOrganizationSlackMemberApp(
54106
executor: DbOrTx,

‎apps/sim/lib/credential-groups/provider-configuration.ts‎

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -159,7 +159,8 @@ async function resolveSlackConfiguration(
159159
.limit(1)
160160
if (!app) throw new Error('Organization Slack app configuration is missing')
161161
/** Member grants are verified against the app and team directly; the Search bot is optional. */
162-
if (app.kind === 'shared') await requireSlackSearchAppAvailable(app.id, params.organizationId)
162+
if (app.kind === 'shared')
163+
await requireSlackSearchAppAvailable(app.id, params.organizationId, params.executor)
163164
const resolved = await resolveSlackAppCredentials(app)
164165
return {
165166
appId: app.id,

‎apps/sim/lib/credential-groups/slack-managed-users.ts‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -918,7 +918,7 @@ export async function exchangeAndConfigureSlackManagedUsers(params: {
918918
'This Slack app belongs to another installation owner'
919919
)
920920
if (app?.kind === 'shared')
921-
await requireSlackSearchAppAvailable(app.id, params.attempt.organizationId)
921+
await requireSlackSearchAppAvailable(app.id, params.attempt.organizationId, tx)
922922
const resolved = app ? await resolveSlackAppCredentials(app) : null
923923
if (
924924
!resolved ||

‎apps/sim/lib/knowledge/application/slack-search/installations.ts‎

Lines changed: 5 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -7,6 +7,8 @@ import { OrchestrationError } from '@/lib/core/orchestration/types'
77
import {
88
loadOrganizationSlackMemberApps,
99
lockOrganizationSlackApp,
10+
requireSearchBotMatchesMemberApp,
11+
resolveOrganizationSlackMemberApp,
1012
} from '@/lib/credential-groups/organization-slack-app'
1113
import { acquireAdvisoryXactLock } from '@/lib/db/advisory-locks'
1214
import {
@@ -173,7 +175,7 @@ export const configureSlackSearchInstallation = defineAuthorizedKnowledgeUseCase
173175
? await tx.select().from(slackApp).where(eq(slackApp.id, current.slackAppId)).limit(1)
174176
: []
175177
if (input.enabled && current.slackAppId)
176-
await requireSlackSearchAppAvailable(current.slackAppId, context.organizationId)
178+
await requireSlackSearchAppAvailable(current.slackAppId, context.organizationId, tx)
177179
if (current.slackAppId && !app) throw new Error('Slack app configuration is missing')
178180
const appRevision =
179181
app && secret ? (await resolveSlackAppCredentials(app)).revision : undefined
@@ -239,6 +241,8 @@ export const configureSlackSearchInstallation = defineAuthorizedKnowledgeUseCase
239241
}
240242
if (!identity || !secret)
241243
throw new OrchestrationError('not_found', 'Slack Search installation not found')
244+
const member = await resolveOrganizationSlackMemberApp(context.organizationId, tx)
245+
requireSearchBotMatchesMemberApp(member.app, identity, app?.kind === 'shared')
242246
const [created] = await tx
243247
.insert(slackSearchInstallation)
244248
.values({

‎apps/sim/lib/knowledge/application/slack-search/setup.test.ts‎

Lines changed: 9 additions & 18 deletions
Original file line numberDiff line numberDiff line change
@@ -24,7 +24,7 @@ const hoisted = vi.hoisted(() => ({
2424
shared: vi.fn(),
2525
revoke: vi.fn(),
2626
validateGrant: vi.fn(),
27-
memberApps: vi.fn(),
27+
memberApp: vi.fn(),
2828
adoptMemberApp: vi.fn(),
2929
insert: vi.fn(),
3030
update: vi.fn(),
@@ -45,8 +45,11 @@ vi.mock('@/lib/internal/slack/oauth', () => ({
4545
validateSlackBotAuthorization: hoisted.validateGrant,
4646
}))
4747
vi.mock('@/lib/credential-groups/service', () => credentialGroupsServiceMock)
48-
vi.mock('@/lib/credential-groups/organization-slack-app', () => ({
49-
loadOrganizationSlackMemberApps: hoisted.memberApps,
48+
vi.mock('@/lib/credential-groups/organization-slack-app', async (importOriginal) => ({
49+
requireSearchBotMatchesMemberApp: (
50+
await importOriginal<typeof import('@/lib/credential-groups/organization-slack-app')>()
51+
).requireSearchBotMatchesMemberApp,
52+
resolveOrganizationSlackMemberApp: hoisted.memberApp,
5053
adoptOrganizationSlackMemberApp: hoisted.adoptMemberApp,
5154
lockOrganizationSlackApp: vi.fn(),
5255
}))
@@ -100,7 +103,7 @@ beforeEach(() => {
100103
m.revoke.mockResolvedValue(undefined)
101104
m.validateGrant.mockReset()
102105
m.ensureGroup.mockResolvedValue({ id: 'accounts' })
103-
m.memberApps.mockReset().mockResolvedValue([])
106+
m.memberApp.mockReset().mockResolvedValue({ app: null, scopes: [] })
104107
urlsMockFns.mockGetBaseUrl.mockReturnValue('https://sim.test')
105108
m.membership.mockResolvedValue([{ role: 'admin' }])
106109
m.rows.mockReset().mockResolvedValue([])
@@ -311,9 +314,7 @@ describe('shared app completion', () => {
311314
}
312315

313316
beforeEach(() => {
314-
m.memberApps.mockResolvedValue([
315-
{ configuration: { slack: { ...memberApp, scopes: ['im:history'] } } },
316-
])
317+
m.memberApp.mockResolvedValue({ app: memberApp, scopes: ['im:history'] })
317318
m.consume.mockResolvedValue({
318319
...attempt,
319320
sharedApp: { id: sharedApp.id, revision: sharedApp.revision },
@@ -428,17 +429,7 @@ describe('connect an app already installed through a Slack manifest', () => {
428429
})
429430

430431
it('rejects a token from a different app than member indexing', async () => {
431-
m.memberApps.mockResolvedValue([
432-
{
433-
configuration: {
434-
slack: {
435-
appId: 'AOTHER',
436-
teamId: 'T1',
437-
scopes: [],
438-
},
439-
},
440-
},
441-
])
432+
m.memberApp.mockResolvedValue({ app: { appId: 'AOTHER', teamId: 'T1' }, scopes: [] })
442433
await expect(connect()).rejects.toThrow('same Slack app and workspace')
443434
expect(db.transaction).not.toHaveBeenCalled()
444435
})

‎apps/sim/lib/knowledge/application/slack-search/setup.ts‎

Lines changed: 6 additions & 37 deletions
Original file line numberDiff line numberDiff line change
@@ -10,12 +10,12 @@ import { decryptSecret, encryptSecret } from '@/lib/core/security/encryption'
1010
import { getBaseUrl } from '@/lib/core/utils/urls'
1111
import {
1212
adoptOrganizationSlackMemberApp,
13-
loadOrganizationSlackMemberApps,
1413
lockOrganizationSlackApp,
14+
requireSearchBotMatchesMemberApp,
15+
resolveOrganizationSlackMemberApp,
1516
} from '@/lib/credential-groups/organization-slack-app'
1617
import { configureSharedSlackMemberApp } from '@/lib/credential-groups/shared-slack-app'
1718
import { acquireAdvisoryXactLock } from '@/lib/db/advisory-locks'
18-
import type { DbOrTx } from '@/lib/db/types'
1919
import { buildSlackAppCreationUrl } from '@/lib/integrations/slack-manifest'
2020
import {
2121
exchangeSlackBotAuthorization,
@@ -63,36 +63,13 @@ interface CompleteInput {
6363
error?: string
6464
}
6565

66-
async function existingMemberApp(organizationId: string, executor: DbOrTx = db) {
67-
const rows = await loadOrganizationSlackMemberApps(organizationId, executor)
68-
const configurations = rows.flatMap((row) =>
69-
row.configuration.slack ? [row.configuration.slack] : []
70-
)
71-
const first = configurations[0]
72-
if (
73-
first &&
74-
configurations.some(
75-
(configuration) =>
76-
configuration.appId !== first.appId || configuration.teamId !== first.teamId
77-
)
78-
)
79-
throw new OrchestrationError(
80-
'conflict',
81-
'Organization Slack member accounts reference multiple apps. Consolidate them before configuring Search.'
82-
)
83-
return {
84-
app: first ? { appId: first.appId, teamId: first.teamId } : null,
85-
scopes: [...new Set(configurations.flatMap((configuration) => configuration.scopes))],
86-
}
87-
}
88-
8966
export const prepareSlackSearchSetup = defineAuthorizedKnowledgeUseCase({
9067
operation: knowledgeOperations.prepareSlackInstallation,
9168
resolveContext: ({ input }: { input: PrepareInput }) =>
9269
resolveKnowledgeOrganizationContext(input),
9370
async execute({ input, context }) {
9471
await requireOrganizationSearchAvailable(context.organizationId)
95-
const member = await existingMemberApp(context.organizationId)
72+
const member = await resolveOrganizationSlackMemberApp(context.organizationId)
9673
const manifest = createSlackSearchManifest(
9774
input.name,
9875
input.description,
@@ -114,7 +91,7 @@ async function prepareInstallationAttempt(
11491
input: StartInput
11592
): Promise<SlackSearchOAuthAttempt> {
11693
const origin = getBaseUrl()
117-
const member = await existingMemberApp(input.organizationId)
94+
const member = await resolveOrganizationSlackMemberApp(input.organizationId)
11895
const [installation] = input.installationId
11996
? await db
12097
.select()
@@ -279,15 +256,7 @@ async function saveInstallation(
279256
attempt.customInstallation.appId === identity.appId)
280257
)
281258
throw new OrchestrationError('conflict', 'Install Sim Search in the same Slack workspace')
282-
if (
283-
attempt.memberApp &&
284-
((!attempt.sharedApp && attempt.memberApp.appId !== identity.appId) ||
285-
attempt.memberApp.teamId !== identity.teamId)
286-
)
287-
throw new OrchestrationError(
288-
'conflict',
289-
'Install the same Slack app and workspace connected by your members'
290-
)
259+
requireSearchBotMatchesMemberApp(attempt.memberApp, identity, Boolean(attempt.sharedApp))
291260
const { encrypted: encryptedToken } = await encryptSecret(
292261
JSON.stringify({
293262
type: SLACK_CUSTOM_BOT_SECRET_TYPE,
@@ -440,7 +409,7 @@ async function saveInstallation(
440409
.values(appValues)
441410
.onConflictDoUpdate({ target: slackApp.id, set: appValues })
442411
}
443-
const member = await existingMemberApp(attempt.organizationId, tx)
412+
const member = await resolveOrganizationSlackMemberApp(attempt.organizationId, tx)
444413
if (
445414
member.app?.appId !== attempt.memberApp?.appId ||
446415
member.app?.teamId !== attempt.memberApp?.teamId

0 commit comments

Comments
 (0)