Skip to content

Commit df1c8f4

Browse files
committed
fix(slack): snapshot the installation set for member setup, lock order, desktop fixture
1 parent 623ea82 commit df1c8f4

5 files changed

Lines changed: 64 additions & 23 deletions

File tree

‎apps/desktop/e2e/source-connect.spec.ts‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -336,6 +336,7 @@ test('source authorization returns to its desktop screen and refreshes live', as
336336
if (path === '/api/knowledge/slack') {
337337
json({
338338
sharedAppAvailable: true,
339+
memberAppKind: installed ? 'shared' : null,
339340
bots: [],
340341
installations: installed
341342
? [

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

Lines changed: 21 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -141,10 +141,10 @@ describe('Slack member accounts on the shared app without a Search bot', () => {
141141
},
142142
})
143143

144-
const startSetup = (credentialGroupId: string) =>
144+
const startSetup = (credentialGroupId: string, app?: { appId: string; teamId: string }) =>
145145
startSlackCredentialGroupConfiguration.execute({
146146
principal: principal(),
147-
input: { organizationId: ids.organization, credentialGroupId },
147+
input: { organizationId: ids.organization, credentialGroupId, ...app },
148148
})
149149

150150
const completeSetup = (state: string) =>
@@ -317,7 +317,24 @@ describe('Slack member accounts on the shared app without a Search bot', () => {
317317
await installBot(CUSTOM_APP_ID, OTHER_TEAM)
318318
provideSlackConsent(MEMBER_TEAM)
319319
await expect(completeSetup(pending.searchParams.get('state') ?? '')).rejects.toThrow(
320-
'installed during authorization'
320+
'installations changed during authorization'
321+
)
322+
expect(await accountsGroup()).toEqual(before)
323+
})
324+
325+
it('refuses member setup when another bot joins an installed one during authorization', async () => {
326+
const groupId = (await approveSlack()).memberAccounts?.groupId
327+
if (!groupId) throw new Error('Approval did not provision organization accounts')
328+
await installBot(SHARED_APP_ID, MEMBER_TEAM)
329+
const pending = new URL(
330+
(await startSetup(groupId, { appId: SHARED_APP_ID, teamId: MEMBER_TEAM })).authorizationUrl
331+
)
332+
expect(pending.searchParams.get('team')).toBe(MEMBER_TEAM)
333+
const before = await accountsGroup()
334+
await installBot(CUSTOM_APP_ID, OTHER_TEAM)
335+
provideSlackConsent(MEMBER_TEAM)
336+
await expect(completeSetup(pending.searchParams.get('state') ?? '')).rejects.toThrow(
337+
'installations changed during authorization'
321338
)
322339
expect(await accountsGroup()).toEqual(before)
323340
})
@@ -354,7 +371,7 @@ describe('Slack member accounts on the shared app without a Search bot', () => {
354371
release.resolve()
355372
await install
356373
}
357-
await expect(completion).rejects.toThrow('installed during authorization')
374+
await expect(completion).rejects.toThrow('installations changed during authorization')
358375
expect(await accountsGroup()).toEqual(before)
359376
})
360377

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

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -171,6 +171,7 @@ export const completeSlackCredentialGroupConfiguration: OperationUseCase<
171171
attempt.appRevision !== pending.appRevision ||
172172
attempt.searchApproval?.approved !== pending.searchApproval?.approved ||
173173
attempt.searchApproval?.updatedAt !== pending.searchApproval?.updatedAt ||
174+
attempt.installationsRevision !== pending.installationsRevision ||
174175
attempt.clientId !== pending.clientId ||
175176
attempt.redirectUri !== pending.redirectUri ||
176177
credentialGroupScopePolicyVersion(attempt.requiredScopes) !==

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

Lines changed: 34 additions & 16 deletions
Original file line numberDiff line numberDiff line change
@@ -13,6 +13,7 @@ import { sha256Hex } from '@sim/security/hash'
1313
import { getErrorMessage } from '@sim/utils/errors'
1414
import { generateId } from '@sim/utils/id'
1515
import { isRecordLike } from '@sim/utils/object'
16+
import { compareStrings } from '@sim/utils/string'
1617
import { and, eq, inArray, isNull, or } from 'drizzle-orm'
1718
import { getRedisClient } from '@/lib/core/config/redis'
1819
import { resourceScopeFields, resourceScopeFromOwner } from '@/lib/core/resource-scope'
@@ -80,6 +81,8 @@ interface SlackSearchApprovalSnapshot {
8081
type StoredSlackManagedUsersAttempt = {
8182
appRevision?: string
8283
searchApproval?: SlackSearchApprovalSnapshot
84+
/** Organization attempts: the Search installations they were authorized against. */
85+
installationsRevision?: string
8386
version: typeof SLACK_MANAGED_USERS_ATTEMPT_VERSION
8487
workspaceId?: string
8588
organizationId?: string
@@ -103,6 +106,7 @@ type StoredSlackManagedUsersAttempt = {
103106
export interface SlackManagedUsersAttempt {
104107
appRevision?: string
105108
searchApproval?: SlackSearchApprovalSnapshot
109+
installationsRevision?: string
106110
workspaceId?: string
107111
organizationId?: string
108112
userId: string
@@ -183,6 +187,9 @@ function isStoredAttempt(value: unknown): value is StoredSlackManagedUsersAttemp
183187
typeof candidate.credentialGroupId === 'string' &&
184188
typeof candidate.credentialGroupUpdatedAt === 'number' &&
185189
(candidate.appRevision === undefined || typeof candidate.appRevision === 'string') &&
190+
(candidate.installationsRevision === undefined ||
191+
(typeof candidate.installationsRevision === 'string' &&
192+
candidate.organizationId !== undefined)) &&
186193
(candidate.organizationId !== undefined
187194
? isRecordLike(approval) &&
188195
typeof approval.approved === 'boolean' &&
@@ -487,6 +494,21 @@ export function getSlackManagedUsersRedirectUri(): string {
487494
return `${getBaseUrl()}${SLACK_MANAGED_USER_CONFIGURATION_CALLBACK_PATH}`
488495
}
489496

497+
/**
498+
* Names an organization's Search installation set. Installing, rebinding, enabling, disabling,
499+
* and removing a bot each rotate or drop a row's revision.
500+
*/
501+
async function organizationInstallationsRevision(executor: DbOrTx, organizationId: string) {
502+
const rows = await executor
503+
.select({ id: slackSearchInstallation.id, revision: slackSearchInstallation.revision })
504+
.from(slackSearchInstallation)
505+
.where(eq(slackSearchInstallation.organizationId, organizationId))
506+
.limit(101)
507+
return sha256Hex(
508+
JSON.stringify(rows.map(({ id, revision }) => `${id}:${revision}`).sort(compareStrings))
509+
)
510+
}
511+
490512
/** An organization app the Search bot installed; its installation fixes the Slack workspace. */
491513
async function resolveInstalledOrganizationSlackApp(
492514
organizationId: string,
@@ -599,6 +621,7 @@ export async function createSlackManagedUsersAttempt(params: {
599621
let clientSecret = params.clientSecret
600622
let appRevision: string | undefined
601623
let searchApproval: SlackSearchApprovalSnapshot | undefined
624+
let installationsRevision: string | undefined
602625
if (scope.kind === 'organization') {
603626
if (
604627
params.slackBotCredentialId ||
@@ -660,6 +683,7 @@ export async function createSlackManagedUsersAttempt(params: {
660683
}
661684
if (searchApproval.approved)
662685
requiredScopes = [...new Set([...requiredScopes, ...SLACK_SEARCH_USER_SCOPES])]
686+
installationsRevision = await organizationInstallationsRevision(db, scope.organizationId)
663687
} else {
664688
if (!params.slackBotCredentialId)
665689
throw new SlackManagedUsersError('Select a custom Slack bot.', 'invalid_response')
@@ -701,6 +725,7 @@ export async function createSlackManagedUsersAttempt(params: {
701725
clientId,
702726
...(appRevision ? { appRevision } : {}),
703727
...(searchApproval ? { searchApproval } : {}),
728+
...(installationsRevision ? { installationsRevision } : {}),
704729
...(sharedApp && scope.kind === 'organization'
705730
? { credentialSource: 'environment' as const, organizationId: scope.organizationId }
706731
: { encryptedClientSecret: (await encryptSecret(clientSecret)).encrypted }),
@@ -779,6 +804,9 @@ async function parseSlackManagedUsersAttempt(
779804
clientId: parsed.clientId,
780805
...(parsed.appRevision ? { appRevision: parsed.appRevision } : {}),
781806
...(parsed.searchApproval ? { searchApproval: parsed.searchApproval } : {}),
807+
...(parsed.installationsRevision
808+
? { installationsRevision: parsed.installationsRevision }
809+
: {}),
782810
clientSecret,
783811
redirectUri: parsed.redirectUri,
784812
requiredScopes: parsed.requiredScopes,
@@ -845,8 +873,9 @@ export async function exchangeAndConfigureSlackManagedUsers(params: {
845873

846874
return db.transaction(async (tx) => {
847875
if (params.attempt.organizationId) {
848-
await lockOrganizationSearchApproval(tx, params.attempt.organizationId)
876+
/** Taken first so waiting on a bot install never holds the approval lock other writes need. */
849877
await lockOrganizationSlackApp(tx, params.attempt.organizationId)
878+
await lockOrganizationSearchApproval(tx, params.attempt.organizationId)
850879
const sharedApp = getSharedSlackSearchAppConfiguration(params.attempt.expectedAppId)
851880
if (sharedApp) await ensureSharedSlackAppRow(tx, sharedApp)
852881
const [app] = await tx
@@ -930,24 +959,13 @@ export async function exchangeAndConfigureSlackManagedUsers(params: {
930959
'Search approval changed during authorization. Start again.',
931960
'invalid_state'
932961
)
933-
/** Bot installs share the Slack organization lock, so one added meanwhile is seen here. */
934-
const installations = await tx
935-
.select({
936-
slackAppId: slackSearchInstallation.slackAppId,
937-
teamId: slackSearchInstallation.teamId,
938-
})
939-
.from(slackSearchInstallation)
940-
.where(eq(slackSearchInstallation.organizationId, params.attempt.organizationId))
941-
.limit(101)
942962
if (
943-
installations.length > 0 &&
944-
!installations.some(
945-
(installation) =>
946-
installation.slackAppId === grant.appId && installation.teamId === grant.teamId
947-
)
963+
params.attempt.installationsRevision !== undefined &&
964+
(await organizationInstallationsRevision(tx, params.attempt.organizationId)) !==
965+
params.attempt.installationsRevision
948966
)
949967
throw new SlackManagedUsersError(
950-
'A Slack app was installed during authorization. Start again.',
968+
'Slack Search installations changed during authorization. Start again.',
951969
'invalid_state'
952970
)
953971
}

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

Lines changed: 7 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -86,10 +86,14 @@ export const listSlackSearchInstallations = defineAuthorizedKnowledgeUseCase({
8686
readSharedSlackSearchApp(context.organizationId),
8787
loadOrganizationSlackMemberApps(context.organizationId),
8888
])
89-
const memberAppId = memberApps[0]?.configuration.slack?.appId
89+
const memberAppIds = memberApps.flatMap((row) =>
90+
row.configuration.slack ? [row.configuration.slack.appId] : []
91+
)
92+
/** Any custom member app blocks the shared-app path, so it wins over a shared one. */
9093
let memberAppKind: 'shared' | 'custom' | null = null
91-
if (memberAppId)
92-
memberAppKind = getSharedSlackSearchAppConfiguration(memberAppId) ? 'shared' : 'custom'
94+
if (memberAppIds.some((appId) => !getSharedSlackSearchAppConfiguration(appId)))
95+
memberAppKind = 'custom'
96+
else if (memberAppIds.length) memberAppKind = 'shared'
9397
if (installations.length > 100 || bots.length > 100)
9498
throw new OrchestrationError(
9599
'validation',

0 commit comments

Comments
 (0)