Skip to content

Commit fa4b65a

Browse files
fix(oauth): coalesce slack token refresh per installation and preserve provider refresh errors
1 parent 1da346b commit fa4b65a

10 files changed

Lines changed: 485 additions & 36 deletions

File tree

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

Lines changed: 33 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -27,6 +27,7 @@ vi.mock('@/lib/auth/credential-access', () => ({
2727
}))
2828

2929
import { TokenServiceAccountValidationError } from '@/lib/credentials/token-service-accounts/errors'
30+
import { OAuthRefreshError } from '@/lib/oauth/errors'
3031
import { GET, POST } from '@/app/api/auth/oauth/token/route'
3132

3233
describe('OAuth Token API Routes', () => {
@@ -301,6 +302,38 @@ describe('OAuth Token API Routes', () => {
301302
)
302303
})
303304

305+
it('should surface the provider error detail on typed refresh failures', async () => {
306+
mockAuthorizeCredentialUse.mockResolvedValueOnce({
307+
ok: true,
308+
authType: 'session',
309+
requesterUserId: 'test-user-id',
310+
credentialOwnerUserId: 'owner-user-id',
311+
})
312+
authOAuthUtilsMockFns.mockGetCredential.mockResolvedValueOnce({
313+
id: 'credential-id',
314+
accessToken: 'test-token',
315+
refreshToken: 'refresh-token',
316+
accessTokenExpiresAt: new Date(Date.now() - 3600 * 1000),
317+
providerId: 'google',
318+
})
319+
authOAuthUtilsMockFns.mockRefreshTokenIfNeeded.mockRejectedValueOnce(
320+
new OAuthRefreshError('google', 'invalid_grant', 'Token has been expired or revoked.')
321+
)
322+
323+
const req = createMockRequest('POST', {
324+
credentialId: 'credential-id',
325+
})
326+
327+
const response = await POST(req)
328+
const data = await response.json()
329+
330+
expect(response.status).toBe(401)
331+
expect(data).toHaveProperty(
332+
'error',
333+
'Failed to refresh access token: invalid_grant (google: Token has been expired or revoked.)'
334+
)
335+
})
336+
304337
describe('credentialAccountUserId + providerId path', () => {
305338
it('should reject unauthenticated requests', async () => {
306339
hybridAuthMockFns.mockCheckSessionOrInternalAuth.mockResolvedValueOnce({

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

Lines changed: 13 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -12,6 +12,7 @@ import { AuthType, checkSessionOrInternalAuth } from '@/lib/auth/hybrid'
1212
import { generateRequestId } from '@/lib/core/utils/request'
1313
import { withRouteHandler } from '@/lib/core/utils/with-route-handler'
1414
import { TokenServiceAccountValidationError } from '@/lib/credentials/token-service-accounts/errors'
15+
import { OAuthRefreshError } from '@/lib/oauth/errors'
1516
import { captureServerEvent } from '@/lib/posthog/server'
1617
import {
1718
getCredential,
@@ -300,7 +301,11 @@ export const POST = withRouteHandler(async (request: NextRequest) => {
300301
)
301302
} catch (error) {
302303
logger.error(`[${requestId}] Failed to refresh access token:`, error)
303-
return NextResponse.json({ error: 'Failed to refresh access token' }, { status: 401 })
304+
const detail = error instanceof OAuthRefreshError ? `: ${error.message}` : ''
305+
return NextResponse.json(
306+
{ error: `Failed to refresh access token${detail}` },
307+
{ status: 401 }
308+
)
304309
}
305310
} catch (error) {
306311
logger.error(`[${requestId}] Error getting access token`, error)
@@ -410,8 +415,13 @@ export const GET = withRouteHandler(async (request: NextRequest) => {
410415
},
411416
{ status: 200 }
412417
)
413-
} catch (_error) {
414-
return NextResponse.json({ error: 'Failed to refresh access token' }, { status: 401 })
418+
} catch (error) {
419+
logger.error(`[${requestId}] Failed to refresh access token:`, error)
420+
const detail = error instanceof OAuthRefreshError ? `: ${error.message}` : ''
421+
return NextResponse.json(
422+
{ error: `Failed to refresh access token${detail}` },
423+
{ status: 401 }
424+
)
415425
}
416426
} catch (error) {
417427
logger.error(`[${requestId}] Error fetching access token`, error)

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

Lines changed: 137 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -166,7 +166,28 @@ describe('OAuth Utils', () => {
166166

167167
await expect(
168168
refreshTokenIfNeeded('request-id', mockCredential, 'credential-id')
169-
).rejects.toThrow('Failed to refresh token')
169+
).rejects.toThrow('invalid_grant (google)')
170+
})
171+
172+
it('should preserve the provider error description in the thrown error', async () => {
173+
const mockCredential = {
174+
id: 'credential-id',
175+
accessToken: 'expired-token',
176+
refreshToken: 'refresh-token',
177+
accessTokenExpiresAt: new Date(Date.now() - 3600 * 1000),
178+
providerId: 'google',
179+
}
180+
181+
mockRefreshOAuthToken.mockResolvedValueOnce({
182+
ok: false,
183+
errorCode: 'invalid_grant',
184+
errorDescription: 'Token has been expired or revoked.',
185+
message: 'Failed',
186+
})
187+
188+
await expect(
189+
refreshTokenIfNeeded('request-id', mockCredential, 'credential-id')
190+
).rejects.toThrow('invalid_grant (google: Token has been expired or revoked.)')
170191
})
171192

172193
it('should not attempt refresh if no refresh token', async () => {
@@ -282,6 +303,121 @@ describe('OAuth Utils', () => {
282303
})
283304
})
284305

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

0 commit comments

Comments
 (0)