Skip to content

Commit 8e34f89

Browse files
TheodoreSpeaksTheodoreSpeaks
committed
fix(slack): scope personal revocation to affected searches
1 parent b53e234 commit 8e34f89

2 files changed

Lines changed: 108 additions & 21 deletions

File tree

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

Lines changed: 75 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -1,5 +1,5 @@
11
/** @vitest-environment node */
2-
import { slackSearchInstallation } from '@sim/db/schema'
2+
import { credential, slackSearchInstallation, slackSearchTurn } from '@sim/db/schema'
33
import { dbChainMockFns, queueTableRows, resetDbChainMock } from '@sim/testing'
44
import { beforeEach, describe, expect, it, vi } from 'vitest'
55

@@ -49,18 +49,86 @@ describe('Slack access revocation', () => {
4949
expect.objectContaining({ status: 'cancelled', outcome: 'access_revoked' })
5050
)
5151
})
52-
it('invalidates member grants without disabling the bot for personal revocation', async () => {
52+
it('cancels only affected members without rotating the shared installation revision', async () => {
53+
dbChainMockFns.returning.mockResolvedValueOnce([{ providerSubjectId: 'U1' }])
5354
await revokeSlackSearchAccess.execute({
5455
principal,
55-
input: { ...input, event: { type: 'tokens_revoked', tokens: { oauth: ['U1'] } } },
56+
input: { ...input, event: { type: 'tokens_revoked', tokens: { oauth: ['U1', 'U2'] } } },
5657
})
5758
expect(dbChainMockFns.set).toHaveBeenCalledWith(
5859
expect.objectContaining({ managedOauthStatus: 'needs_reauth' })
5960
)
60-
expect(dbChainMockFns.set).toHaveBeenCalledWith(
61-
expect.objectContaining({ lastOutcome: 'tokens_revoked' })
62-
)
63-
expect(dbChainMockFns.set.mock.calls.some(([value]) => 'enabled' in value)).toBe(false)
61+
expect(dbChainMockFns.update.mock.calls.map(([table]) => table)).toEqual([
62+
credential,
63+
slackSearchTurn,
64+
])
65+
expect(dbChainMockFns.where).toHaveBeenLastCalledWith({
66+
type: 'and',
67+
conditions: [
68+
{ type: 'eq', left: slackSearchTurn.installationId, right: 'i1' },
69+
{ type: 'inArray', column: slackSearchTurn.status, values: ['pending', 'running'] },
70+
{
71+
type: 'inArray',
72+
column: expect.objectContaining({
73+
strings: ['', " #>> '{message,userId}'"],
74+
values: [slackSearchTurn.payload],
75+
}),
76+
values: ['U1'],
77+
},
78+
],
79+
})
80+
expect(dbChainMockFns.where).toHaveBeenNthCalledWith(2, {
81+
type: 'and',
82+
conditions: expect.arrayContaining([
83+
{ type: 'eq', left: credential.organizationId, right: 'org' },
84+
{ type: 'eq', left: credential.authorizationAppId, right: 'slack:A1:T1' },
85+
{ type: 'inArray', column: credential.providerSubjectId, values: ['U1', 'U2'] },
86+
{
87+
type: 'or',
88+
conditions: [
89+
{ type: 'isNull', column: credential.grantedAt },
90+
{
91+
type: 'lte',
92+
left: credential.grantedAt,
93+
right: new Date(input.event_time * 1000),
94+
},
95+
],
96+
},
97+
]),
98+
})
99+
})
100+
it('does not cancel work when no current member grants were revoked', async () => {
101+
await revokeSlackSearchAccess.execute({
102+
principal,
103+
input: { ...input, event: { type: 'tokens_revoked', tokens: { oauth: ['U1'] } } },
104+
})
105+
expect(dbChainMockFns.update.mock.calls.map(([table]) => table)).toEqual([credential])
106+
})
107+
it.each([{ bot: ['UBOT'] }, { bot: ['UBOT'], oauth: ['U1'] }])(
108+
'cancels all installation work when its bot is revoked: %j',
109+
async (tokens) => {
110+
await revokeSlackSearchAccess.execute({
111+
principal,
112+
input: { ...input, event: { type: 'tokens_revoked', tokens } },
113+
})
114+
expect(dbChainMockFns.set).toHaveBeenCalledWith(
115+
expect.objectContaining({ enabled: false, revision: expect.any(String) })
116+
)
117+
expect(dbChainMockFns.where).toHaveBeenLastCalledWith({
118+
type: 'and',
119+
conditions: [
120+
{ type: 'eq', left: slackSearchTurn.installationId, right: 'i1' },
121+
{ type: 'inArray', column: slackSearchTurn.status, values: ['pending', 'running'] },
122+
],
123+
})
124+
}
125+
)
126+
it('does not invalidate the installation when a different bot token is revoked', async () => {
127+
await revokeSlackSearchAccess.execute({
128+
principal,
129+
input: { ...input, event: { type: 'tokens_revoked', tokens: { bot: ['OTHER'] } } },
130+
})
131+
expect(dbChainMockFns.update).not.toHaveBeenCalled()
64132
})
65133
it.each([{ appId: 'A2' }, { receivedAt: new Date(0) }, { receivedAt: new Date(Number.NaN) }])(
66134
'rejects invalid verified authority %#',

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

Lines changed: 33 additions & 14 deletions
Original file line numberDiff line numberDiff line change
@@ -1,7 +1,7 @@
11
import { db } from '@sim/db'
22
import { credential, slackSearchInstallation, slackSearchTurn } from '@sim/db/schema'
33
import { generateId } from '@sim/utils/id'
4-
import { and, eq, inArray, isNull, lte, or } from 'drizzle-orm'
4+
import { and, eq, inArray, isNull, lte, or, sql } from 'drizzle-orm'
55
import { z } from 'zod'
66
import type { OperationUseCase } from '@/lib/core/application/operation'
77
import { OrchestrationError } from '@/lib/core/orchestration/types'
@@ -74,8 +74,9 @@ export const revokeSlackSearchAccess: OperationUseCase<
7474
uninstall ||
7575
(input.event.type === 'tokens_revoked' &&
7676
(input.event.tokens.bot ?? []).includes(installation.botUserId))
77+
let revokedMemberIds: string[] = []
7778
if (uninstall || revokedUsers.length) {
78-
await tx
79+
const revokeCredentials = tx
7980
.update(credential)
8081
.set({ managedOauthStatus: 'needs_reauth', updatedAt: new Date() })
8182
.where(
@@ -90,25 +91,43 @@ export const revokeSlackSearchAccess: OperationUseCase<
9091
...(uninstall ? [] : [inArray(credential.providerSubjectId, revokedUsers)])
9192
)
9293
)
94+
if (revokeBot) await revokeCredentials
95+
else {
96+
const revokedCredentials = await revokeCredentials.returning({
97+
providerSubjectId: credential.providerSubjectId,
98+
})
99+
revokedMemberIds = revokedCredentials.flatMap(({ providerSubjectId }) =>
100+
providerSubjectId ? [providerSubjectId] : []
101+
)
102+
}
93103
}
94-
if (installation.updatedAt > occurredAt) return
95-
if (!revokeBot && !revokedUsers.length) return
96-
await tx
97-
.update(slackSearchInstallation)
98-
.set({
99-
revision: generateId(),
100-
...(revokeBot ? { enabled: false } : {}),
101-
lastOutcome: uninstall ? 'app_uninstalled' : 'tokens_revoked',
102-
updatedAt: new Date(),
103-
})
104-
.where(eq(slackSearchInstallation.id, installation.id))
104+
if (revokeBot) {
105+
if (installation.updatedAt > occurredAt) return
106+
await tx
107+
.update(slackSearchInstallation)
108+
.set({
109+
revision: generateId(),
110+
enabled: false,
111+
lastOutcome: uninstall ? 'app_uninstalled' : 'tokens_revoked',
112+
updatedAt: new Date(),
113+
})
114+
.where(eq(slackSearchInstallation.id, installation.id))
115+
} else if (!revokedMemberIds.length) return
105116
await tx
106117
.update(slackSearchTurn)
107118
.set({ status: 'cancelled', outcome: 'access_revoked', updatedAt: new Date() })
108119
.where(
109120
and(
110121
eq(slackSearchTurn.installationId, installation.id),
111-
inArray(slackSearchTurn.status, ['pending', 'running'])
122+
inArray(slackSearchTurn.status, ['pending', 'running']),
123+
...(revokeBot
124+
? []
125+
: [
126+
inArray(
127+
sql<string>`${slackSearchTurn.payload} #>> '{message,userId}'`,
128+
revokedMemberIds
129+
),
130+
])
112131
)
113132
)
114133
})

0 commit comments

Comments
 (0)