Skip to content

Commit f1deb31

Browse files
fix(oauth): coalesce slack token refresh per installation and preserve provider refresh errors (#5723)
* fix(oauth): coalesce slack token refresh per installation and preserve provider refresh errors * fix(oauth): keep transient refresh failures errorless and size slack lock budgets past the provider timeout * fix(oauth): let slack refresh followers poll for the lock's full lifetime * chore(oauth): drop provider refresh-error surfacing to keep this PR slack-only * fix(oauth): version-guard slack chain writes against concurrent connects * fix(oauth): clear slack dead flag before connect fan-out
1 parent 7e6aeb2 commit f1deb31

7 files changed

Lines changed: 468 additions & 17 deletions

File tree

apps/sim/app/api/auth/oauth/token/route.ts

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -410,7 +410,8 @@ export const GET = withRouteHandler(async (request: NextRequest) => {
410410
},
411411
{ status: 200 }
412412
)
413-
} catch (_error) {
413+
} catch (error) {
414+
logger.error(`[${requestId}] Failed to refresh access token:`, error)
414415
return NextResponse.json({ error: 'Failed to refresh access token' }, { status: 401 })
415416
}
416417
} catch (error) {

apps/sim/app/api/auth/oauth/utils.test.ts

Lines changed: 140 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -282,6 +282,146 @@ describe('OAuth Utils', () => {
282282
})
283283
})
284284

285+
describe('Slack installation-scoped refresh', () => {
286+
const SLACK_ACCOUNT_ID = 'T08CM6ZNYBE-usr_U08USBQ9B1T-cbf46a7e-ca75-4a2e-bef5-fd467299eaae'
287+
const past = new Date(Date.now() - 3600 * 1000)
288+
const future = new Date(Date.now() + 3600 * 1000)
289+
290+
/** Select chain for getFreshestSlackChain: where() -> orderBy() -> limit(). */
291+
function mockSelectOrderedChain(limitResult: unknown[]) {
292+
const mockLimit = vi.fn().mockReturnValue(limitResult)
293+
const mockOrderBy = vi.fn().mockReturnValue({ limit: mockLimit })
294+
const mockWhere = vi.fn().mockReturnValue({ orderBy: mockOrderBy, limit: mockLimit })
295+
const mockFrom = vi.fn().mockReturnValue({ where: mockWhere })
296+
mockDb.select.mockReturnValueOnce({ from: mockFrom })
297+
return { mockWhere, mockOrderBy, mockLimit }
298+
}
299+
300+
function slackCredential(overrides: Record<string, unknown> = {}) {
301+
return {
302+
id: 'row-1',
303+
resolvedCredentialId: 'row-1',
304+
accountId: SLACK_ACCOUNT_ID,
305+
accessToken: 'stale-at',
306+
refreshToken: 'stale-rt',
307+
accessTokenExpiresAt: past,
308+
providerId: 'slack',
309+
...overrides,
310+
}
311+
}
312+
313+
it('locks per installation and refreshes with the freshest sibling refresh token', async () => {
314+
mockSelectOrderedChain([
315+
{ accessToken: 'stale-at', refreshToken: 'live-rt', accessTokenExpiresAt: past },
316+
])
317+
mockRefreshOAuthToken.mockResolvedValueOnce({
318+
ok: true,
319+
accessToken: 'new-at',
320+
expiresIn: 43200,
321+
refreshToken: 'new-rt',
322+
})
323+
const { mockSet } = mockUpdateChain()
324+
325+
const result = await refreshTokenIfNeeded('request-id', slackCredential(), 'row-1')
326+
327+
expect(result).toEqual({ accessToken: 'new-at', refreshed: true })
328+
expect(redisConfigMockFns.mockAcquireLock.mock.calls[0][0]).toBe(
329+
'oauth:refresh:slack:T08CM6ZNYBE'
330+
)
331+
expect(redisConfigMockFns.mockAcquireLock.mock.calls[0][2]).toBe(30)
332+
expect(mockRefreshOAuthToken).toHaveBeenCalledWith('slack', 'live-rt')
333+
expect(mockSet).toHaveBeenCalledWith(
334+
expect.objectContaining({ accessToken: 'new-at', refreshToken: 'new-rt' })
335+
)
336+
})
337+
338+
it('returns the freshest sibling token without refreshing when it is still valid', async () => {
339+
mockSelectOrderedChain([
340+
{ accessToken: 'sibling-at', refreshToken: 'live-rt', accessTokenExpiresAt: future },
341+
])
342+
const { mockSet } = mockUpdateChain()
343+
344+
const result = await refreshTokenIfNeeded('request-id', slackCredential(), 'row-1')
345+
346+
expect(result).toEqual({ accessToken: 'sibling-at', refreshed: true })
347+
expect(mockRefreshOAuthToken).not.toHaveBeenCalled()
348+
expect(mockSet).toHaveBeenCalledWith(
349+
expect.objectContaining({ accessToken: 'sibling-at', refreshToken: 'live-rt' })
350+
)
351+
})
352+
353+
it('keeps per-row behavior for pasted custom-bot account ids', async () => {
354+
mockRefreshOAuthToken.mockResolvedValueOnce({
355+
ok: true,
356+
accessToken: 'new-at',
357+
expiresIn: 43200,
358+
refreshToken: 'new-rt',
359+
})
360+
mockUpdateChain()
361+
362+
const result = await refreshTokenIfNeeded(
363+
'request-id',
364+
slackCredential({ accountId: 'slack-bot-1764756583292' }),
365+
'row-1'
366+
)
367+
368+
expect(result).toEqual({ accessToken: 'new-at', refreshed: true })
369+
expect(redisConfigMockFns.mockAcquireLock.mock.calls[0][0]).toBe('oauth:refresh:row-1')
370+
expect(mockRefreshOAuthToken).toHaveBeenCalledWith('slack', 'stale-rt')
371+
})
372+
373+
it('dead-flags the installation, not the row, on terminal refresh errors', async () => {
374+
const fakeRedis = {
375+
set: vi.fn().mockResolvedValue('OK'),
376+
get: vi.fn().mockResolvedValue(null),
377+
del: vi.fn().mockResolvedValue(1),
378+
}
379+
redisConfigMockFns.mockGetRedisClient.mockReturnValue(fakeRedis)
380+
mockSelectOrderedChain([
381+
{ accessToken: 'stale-at', refreshToken: 'live-rt', accessTokenExpiresAt: past },
382+
])
383+
mockRefreshOAuthToken.mockResolvedValueOnce({
384+
ok: false,
385+
errorCode: 'token_revoked',
386+
})
387+
mockSelectChain([])
388+
389+
await expect(refreshTokenIfNeeded('request-id', slackCredential(), 'row-1')).rejects.toThrow(
390+
'Failed to refresh token'
391+
)
392+
393+
expect(fakeRedis.set).toHaveBeenCalledWith(
394+
'oauth:dead:slack:T08CM6ZNYBE',
395+
'token_revoked',
396+
'EX',
397+
3600
398+
)
399+
})
400+
401+
it('skips the dead flag when the chain moved during the failed refresh', async () => {
402+
const fakeRedis = {
403+
set: vi.fn().mockResolvedValue('OK'),
404+
get: vi.fn().mockResolvedValue(null),
405+
del: vi.fn().mockResolvedValue(1),
406+
}
407+
redisConfigMockFns.mockGetRedisClient.mockReturnValue(fakeRedis)
408+
mockSelectOrderedChain([
409+
{ accessToken: 'stale-at', refreshToken: 'live-rt', accessTokenExpiresAt: past },
410+
])
411+
mockRefreshOAuthToken.mockResolvedValueOnce({
412+
ok: false,
413+
errorCode: 'token_revoked',
414+
})
415+
mockSelectChain([{ moved: new Date() }])
416+
417+
await expect(refreshTokenIfNeeded('request-id', slackCredential(), 'row-1')).rejects.toThrow(
418+
'Failed to refresh token'
419+
)
420+
421+
expect(fakeRedis.set).not.toHaveBeenCalled()
422+
})
423+
})
424+
285425
describe('resolveServiceAccountToken', () => {
286426
it('throws loudly for an unknown provider (never silently attempts Google)', async () => {
287427
await expect(resolveServiceAccountToken('cred-1', 'mystery-provider')).rejects.toThrow(

apps/sim/app/api/auth/oauth/utils.ts

Lines changed: 113 additions & 16 deletions
Original file line numberDiff line numberDiff line change
@@ -27,6 +27,13 @@ import {
2727
isMicrosoftProvider,
2828
PROACTIVE_REFRESH_THRESHOLD_DAYS,
2929
} from '@/lib/oauth/microsoft'
30+
import {
31+
extractSlackTeamId,
32+
fanOutSlackTokenChain,
33+
getFreshestSlackChain,
34+
hasSlackChainMoved,
35+
isSlackProvider,
36+
} from '@/lib/oauth/slack'
3037
import {
3138
getRecentTerminalError,
3239
isTerminalRefreshError,
@@ -658,25 +665,51 @@ interface CoalescedRefreshOptions {
658665
accountId: string
659666
providerId: string
660667
refreshToken: string
668+
/** External provider account id (`account.accountId`), used to scope Slack refreshes per installation. */
669+
providerAccountId?: string | null
661670
requestId?: string
662671
userId?: string
663672
}
664673

674+
/**
675+
* Slack lock budgets sized past `TOKEN_REFRESH_TIMEOUT_MS` (15s) in
676+
* lib/oauth/oauth.ts: installation-keyed locks make every sibling row's request
677+
* a follower of one refresh, so the TTL covers the provider call plus generous
678+
* headroom for the surrounding DB reads and the fan-out write, and followers
679+
* poll for the lock's full lifetime so a slow-but-successful refresh is still
680+
* observed rather than reported as a failure. These budgets are latency knobs,
681+
* not correctness guarantees — chain integrity under lock expiry or unlocked
682+
* concurrent writers is enforced by the version-guarded fan-out
683+
* (`ifChainUnchangedSince` in lib/oauth/slack.ts).
684+
*/
685+
const SLACK_LOCK_TTL_SEC = 30
686+
const SLACK_FOLLOWER_MAX_WAIT_MS = SLACK_LOCK_TTL_SEC * 1000
687+
665688
async function performCoalescedRefresh({
666689
accountId,
667690
providerId,
668691
refreshToken,
692+
providerAccountId,
669693
requestId,
670694
userId,
671695
}: CoalescedRefreshOptions): Promise<string | null> {
696+
/**
697+
* Slack bot tokens are per-installation (team × app): every account row for
698+
* one team holds a copy of the same rotating chain, so refreshes are locked,
699+
* dead-flagged, and written per installation rather than per row.
700+
*/
701+
const slackTeamId = isSlackProvider(providerId) ? extractSlackTeamId(providerAccountId) : null
702+
const scopeKey = slackTeamId ? `slack:${slackTeamId}` : accountId
703+
672704
const logContext = {
673705
...(requestId ? { requestId } : {}),
674706
...(userId ? { userId } : {}),
707+
...(slackTeamId ? { slackTeamId } : {}),
675708
providerId,
676709
accountId,
677710
}
678711

679-
const deadCode = await getRecentTerminalError(accountId)
712+
const deadCode = await getRecentTerminalError(scopeKey)
680713
if (deadCode) {
681714
logger.warn('Skipping refresh: credential recently failed', {
682715
...logContext,
@@ -685,39 +718,99 @@ async function performCoalescedRefresh({
685718
return null
686719
}
687720

688-
const lockKey = `oauth:refresh:${accountId}`
721+
const lockKey = `oauth:refresh:${scopeKey}`
689722

690723
const refreshPromise = coalesceLocally(lockKey, () =>
691724
withLeaderLock<string>({
692725
key: lockKey,
726+
// Installation-keyed Slack locks gather followers from every sibling row,
727+
// so their wait and the lock TTL must outlast the 15s provider timeout —
728+
// the 3s/10s defaults would fail followers early and let a second leader
729+
// start a concurrent rotation mid-refresh.
730+
...(slackTeamId ? { maxWaitMs: SLACK_FOLLOWER_MAX_WAIT_MS, ttlSec: SLACK_LOCK_TTL_SEC } : {}),
693731
onLeader: async () => {
694732
try {
695-
const result = await refreshOAuthToken(providerId, refreshToken)
733+
let refreshTokenToUse = refreshToken
734+
let slackChainVersion: Date | null = null
735+
if (slackTeamId) {
736+
const freshest = await getFreshestSlackChain(slackTeamId)
737+
if (!freshest) {
738+
throw new Error(
739+
`No refresh-capable account row found for Slack installation ${slackTeamId}`
740+
)
741+
}
742+
slackChainVersion = freshest.chainVersion
743+
if (
744+
freshest.accessToken &&
745+
freshest.accessTokenExpiresAt &&
746+
freshest.accessTokenExpiresAt > new Date()
747+
) {
748+
await fanOutSlackTokenChain(
749+
slackTeamId,
750+
{
751+
accessToken: freshest.accessToken,
752+
refreshToken: freshest.refreshToken,
753+
accessTokenExpiresAt: freshest.accessTokenExpiresAt,
754+
},
755+
{ ifChainUnchangedSince: freshest.chainVersion }
756+
)
757+
logger.info('Reused freshest Slack installation token', logContext)
758+
return freshest.accessToken
759+
}
760+
refreshTokenToUse = freshest.refreshToken
761+
}
762+
763+
const result = await refreshOAuthToken(providerId, refreshTokenToUse)
696764

697765
if (!result.ok) {
698766
logger.error('Failed to refresh token', {
699767
...logContext,
700768
errorCode: result.errorCode,
701769
})
702770
if (result.errorCode && isTerminalRefreshError(result.errorCode)) {
703-
await markCredentialDead(accountId, result.errorCode)
771+
// A refresh that lost a race with a concurrent connect fails with
772+
// a revoked/rotated-out token even though the installation just
773+
// got a live chain — dead-flagging then would take down a healthy
774+
// credential for an hour.
775+
if (
776+
slackChainVersion &&
777+
(await hasSlackChainMoved(slackTeamId!, slackChainVersion))
778+
) {
779+
logger.info('Skipping dead flag: Slack chain moved during refresh', logContext)
780+
} else {
781+
await markCredentialDead(scopeKey, result.errorCode)
782+
}
704783
}
705784
return null
706785
}
707786

708-
const updateData: Record<string, unknown> = {
709-
accessToken: result.accessToken,
710-
accessTokenExpiresAt: new Date(Date.now() + result.expiresIn * 1000),
711-
updatedAt: new Date(),
712-
}
713-
if (result.refreshToken && result.refreshToken !== refreshToken) {
714-
updateData.refreshToken = result.refreshToken
715-
}
716-
if (isMicrosoftProvider(providerId)) {
717-
updateData.refreshTokenExpiresAt = getMicrosoftRefreshTokenExpiry()
718-
}
787+
const accessTokenExpiresAt = new Date(Date.now() + result.expiresIn * 1000)
788+
789+
if (slackTeamId) {
790+
await fanOutSlackTokenChain(
791+
slackTeamId,
792+
{
793+
accessToken: result.accessToken,
794+
refreshToken: result.refreshToken || refreshTokenToUse,
795+
accessTokenExpiresAt,
796+
},
797+
{ ifChainUnchangedSince: slackChainVersion ?? undefined }
798+
)
799+
} else {
800+
const updateData: Record<string, unknown> = {
801+
accessToken: result.accessToken,
802+
accessTokenExpiresAt,
803+
updatedAt: new Date(),
804+
}
805+
if (result.refreshToken && result.refreshToken !== refreshToken) {
806+
updateData.refreshToken = result.refreshToken
807+
}
808+
if (isMicrosoftProvider(providerId)) {
809+
updateData.refreshTokenExpiresAt = getMicrosoftRefreshTokenExpiry()
810+
}
719811

720-
await db.update(account).set(updateData).where(eq(account.id, accountId))
812+
await db.update(account).set(updateData).where(eq(account.id, accountId))
813+
}
721814

722815
logger.info('Successfully refreshed access token', logContext)
723816
return result.accessToken
@@ -774,6 +867,7 @@ export async function getOAuthToken(userId: string, providerId: string): Promise
774867
const connections = await db
775868
.select({
776869
id: account.id,
870+
providerAccountId: account.accountId,
777871
accessToken: account.accessToken,
778872
refreshToken: account.refreshToken,
779873
accessTokenExpiresAt: account.accessTokenExpiresAt,
@@ -813,6 +907,7 @@ export async function getOAuthToken(userId: string, providerId: string): Promise
813907
accountId: credential.id,
814908
providerId,
815909
refreshToken: credential.refreshToken!,
910+
providerAccountId: credential.providerAccountId,
816911
userId,
817912
})
818913
if (fresh) return fresh
@@ -915,6 +1010,7 @@ export async function resolveCredentialAccessToken(
9151010
accountId: resolvedCredentialId,
9161011
providerId: credential.providerId,
9171012
refreshToken: credential.refreshToken!,
1013+
providerAccountId: credential.accountId,
9181014
requestId,
9191015
userId: credential.userId,
9201016
})
@@ -1019,6 +1115,7 @@ export async function refreshTokenIfNeeded(
10191115
accountId: resolvedCredentialId,
10201116
providerId: credential.providerId,
10211117
refreshToken: credential.refreshToken!,
1118+
providerAccountId: credential.accountId,
10221119
requestId,
10231120
userId: credential.userId,
10241121
})

0 commit comments

Comments
 (0)