Skip to content

Commit ce354d0

Browse files
fix(slack-search): make source prompts ephemeral and guard OAuth deadlines
1 parent 284420c commit ce354d0

5 files changed

Lines changed: 132 additions & 34 deletions

File tree

‎apps/sim/ee/credential-groups/components/slack-managed-users-access.test.tsx‎

Lines changed: 27 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -242,6 +242,33 @@ describe('Slack member access selection', () => {
242242
expect(toast.success).not.toHaveBeenCalled()
243243
})
244244

245+
it('keeps a new authorization intact if an old deadline callback runs', async () => {
246+
vi.useFakeTimers()
247+
const timeouts = vi.spyOn(window, 'setTimeout')
248+
await render()
249+
await submit()
250+
const oldDeadline = timeouts.mock.calls.find(([, delay]) => delay === 10 * 60 * 1_000)?.[0]
251+
if (typeof oldDeadline !== 'function') throw new Error('Authorization deadline was not set')
252+
await clickButton('Cancel')
253+
254+
const nextPopup = { location: { href: '' }, closed: false, close: vi.fn() }
255+
vi.mocked(window.open).mockReturnValueOnce(nextPopup as unknown as Window)
256+
mocks.start.mockResolvedValueOnce({
257+
state: 'new-state',
258+
authorizationUrl: 'https://slack.com/oauth/v2/authorize',
259+
})
260+
await submit()
261+
await act(async () => oldDeadline())
262+
263+
expect(toast.error).not.toHaveBeenCalled()
264+
expect(nextPopup.close).not.toHaveBeenCalled()
265+
expect(document.body.textContent).toContain('Waiting for Slack...')
266+
await completeAuthorization('new-state')
267+
expect(toast.success).toHaveBeenCalledExactlyOnceWith('Slack configured')
268+
await act(async () => vi.advanceTimersByTimeAsync(10 * 60 * 1_000))
269+
expect(toast.error).not.toHaveBeenCalled()
270+
})
271+
245272
it('does not navigate or start a timeout when authorization startup finishes after cancel', async () => {
246273
vi.useFakeTimers()
247274
let finishStartup!: (value: { state: string; authorizationUrl: string }) => void

‎apps/sim/ee/credential-groups/components/slack-managed-users-modal.tsx‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -282,6 +282,7 @@ export function SlackManagedUsersModal({
282282
opened.location.href = result.authorizationUrl
283283
/** COOP can report a live OAuth popup as closed; only the deadline expires its state. */
284284
authorizationTimeout.current = window.setTimeout(() => {
285+
if (popup.current !== opened) return
285286
authorizationTimeout.current = null
286287
opened.close()
287288
popup.current = null

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

Lines changed: 59 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -275,13 +275,13 @@ describe('Slack onboarding control delivery', () => {
275275
eventId: 'Ev1',
276276
receivedAt: new Date(),
277277
} as const
278-
const send = () =>
278+
const send = (reason: 'account' | 'sources' = 'account') =>
279279
sendSlackSearchOnboarding(slackPrincipal, {
280280
job,
281281
turnId: 'turn1',
282282
leaseId: 'lease1',
283283
email: state.email,
284-
reason: 'account',
284+
reason,
285285
signal: new AbortController().signal,
286286
})
287287
it('posts a thread-scoped signup link without bot secrets or email in its URL', async () => {
@@ -301,12 +301,65 @@ describe('Slack onboarding control delivery', () => {
301301
expect.objectContaining({ turnId: 'turn1', email: state.email })
302302
)
303303
})
304-
it('rechecks the binding and lease immediately before delivery', async () => {
305-
m.authorize.mockResolvedValueOnce(context).mockResolvedValueOnce(null)
306-
await expect(send()).rejects.toThrow('disabled')
304+
it('sends Connect sources only to the requesting Slack user without requiring an active thread', async () => {
305+
await send('sources')
306+
expect(m.api).toHaveBeenLastCalledWith({
307+
accessToken: 'bot-secret',
308+
method: 'chat.postEphemeral',
309+
body: {
310+
channel: 'D1',
311+
user: job.message.userId,
312+
text: 'Connect your sources',
313+
blocks: expect.arrayContaining([
314+
expect.objectContaining({
315+
type: 'actions',
316+
elements: [
317+
expect.objectContaining({
318+
text: { type: 'plain_text', text: 'Connect sources' },
319+
url: 'https://sim.test/slack-search/connect/opaque-token',
320+
}),
321+
],
322+
}),
323+
]),
324+
},
325+
signal: expect.any(AbortSignal),
326+
})
307327
expect(m.post).not.toHaveBeenCalled()
308-
expect(m.lease).toHaveBeenCalledWith('turn1', 'lease1')
328+
expect(m.outcome).toHaveBeenCalledWith(context.installation, 'sources_required')
309329
})
330+
it.each(['rejected', 'ambiguous'] as const)(
331+
'does not replace a %s ephemeral delivery with a persistent message',
332+
async (outcome) => {
333+
m.api.mockResolvedValueOnce({
334+
status: 200,
335+
data: { ok: true, permalink: state.slackUrl },
336+
})
337+
if (outcome === 'rejected') {
338+
m.api.mockResolvedValueOnce({
339+
status: 200,
340+
data: { ok: false, error: 'user_not_in_channel' },
341+
})
342+
} else {
343+
m.api.mockRejectedValueOnce(new Error('connection lost after send'))
344+
}
345+
await expect(send('sources')).rejects.toThrow(
346+
outcome === 'rejected' ? 'Could not deliver Slack onboarding' : 'connection lost after send'
347+
)
348+
expect(m.api).toHaveBeenCalledTimes(2)
349+
expect(m.post).not.toHaveBeenCalled()
350+
expect(m.outcome).not.toHaveBeenCalled()
351+
}
352+
)
353+
it.each(['account', 'sources'] as const)(
354+
'rechecks the binding and lease immediately before %s delivery',
355+
async (reason) => {
356+
m.authorize.mockResolvedValueOnce(context).mockResolvedValueOnce(null)
357+
await expect(send(reason)).rejects.toThrow('disabled')
358+
expect(m.post).not.toHaveBeenCalled()
359+
expect(m.api).toHaveBeenCalledTimes(1)
360+
expect(m.lease).toHaveBeenCalledWith('turn1', 'lease1')
361+
}
362+
)
310363
it('does not retry an ambiguous post or fall back to another transport', async () => {
311364
m.post.mockRejectedValueOnce(new Error('connection lost after send'))
312365
await expect(send()).rejects.toThrow('connection lost after send')

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

Lines changed: 39 additions & 27 deletions
Original file line numberDiff line numberDiff line change
@@ -113,34 +113,46 @@ export async function sendSlackSearchOnboarding(
113113
await requireSlackSearchTurnLease(turnId, leaseId)
114114
if (!(await authorizeSlackSearchInstallation(principal, job)))
115115
throw new OrchestrationError('forbidden', 'Slack Search is disabled')
116-
const response = await postSlackMessage(
117-
context.secret.botToken,
118-
{
119-
channel: job.message.channelId,
120-
thread_ts: job.message.threadTs ?? job.message.messageTs,
121-
text,
122-
unfurl_links: false,
123-
unfurl_media: false,
124-
blocks: [
125-
{ type: 'section', text: { type: 'plain_text', text } },
126-
{
127-
type: 'actions',
128-
elements: [
129-
{
130-
type: 'button',
131-
text: {
132-
type: 'plain_text',
133-
text: reason === 'account' ? 'Get started with Sim' : 'Connect sources',
134-
},
135-
url,
136-
action_id: 'slack_search_onboarding',
116+
const message = {
117+
channel: job.message.channelId,
118+
text,
119+
blocks: [
120+
{ type: 'section', text: { type: 'plain_text', text } },
121+
{
122+
type: 'actions',
123+
elements: [
124+
{
125+
type: 'button',
126+
text: {
127+
type: 'plain_text',
128+
text: reason === 'account' ? 'Get started with Sim' : 'Connect sources',
137129
},
138-
],
139-
},
140-
],
141-
},
142-
signal
143-
)
130+
url,
131+
action_id: 'slack_search_onboarding',
132+
},
133+
],
134+
},
135+
],
136+
}
137+
/** Channel-level ephemeral prompts also display before the first persistent thread reply exists. */
138+
const response =
139+
reason === 'sources'
140+
? await requestSlackApi({
141+
accessToken: context.secret.botToken,
142+
method: 'chat.postEphemeral',
143+
body: { ...message, user: job.message.userId },
144+
signal,
145+
})
146+
: await postSlackMessage(
147+
context.secret.botToken,
148+
{
149+
...message,
150+
thread_ts: job.message.threadTs ?? job.message.messageTs,
151+
unfurl_links: false,
152+
unfurl_media: false,
153+
},
154+
signal
155+
)
144156
if (response.status !== 200 || response.data.ok !== true)
145157
throw new Error('Could not deliver Slack onboarding')
146158
await recordSlackSearchOutcome(

‎apps/sim/lib/slack-search/README.md‎

Lines changed: 6 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -187,7 +187,12 @@ apply. A wrong account or conflicting identity cannot see the original question
187187
or organization details on the return page.
188188

189189
For an authorized member with no accessible indexed documents, the bot sends a
190-
**Connect sources** button without starting an Assistant run. The return page
190+
**Connect sources** button in an ephemeral message visible only to that Slack
191+
user, without starting an Assistant run. It is posted in the DM rather than
192+
an ephemeral thread reply, which Slack only displays for an already active
193+
thread. It disappears when the Slack session ends; private Sim history retains
194+
the setup notice. Failed delivery never falls back to a persistent Slack message.
195+
The return page
191196
opens the organization's existing Integrations page directly, using the same
192197
source list and connection dialogs as normal visits. Slack adds only indexing,
193198
retry, and thread-return actions to its header. Shared documents that the member can already search do not

0 commit comments

Comments
 (0)