Skip to content

Commit 3dc14a4

Browse files
committed
fix(oauth): guard revocation records against in-flight reconnects
- Record a revocation only when the row's updated_at has not moved since the leader read it, so a reconnect that keeps the same refresh token while a rejected refresh is in flight is not undone. - Reread the row under the refresh lock, so a revocation another process just recorded is honored instead of calling the provider again. - A re-probe whose transient failure meets a moved chain reports no revocation. - Slack fan-out clears the record only for a chain the provider just issued, not when re-spreading a stored one. - A reconnect whose cleanup fails now fails the callback. - Revocation rejections log at WARN in oauth.ts too; the pure refresh error classifiers move to refresh-error-codes.ts so that client-reachable module can use them. - The connector path leaves the revocation log to executeSync.
1 parent f0c1adf commit 3dc14a4

13 files changed

Lines changed: 206 additions & 126 deletions

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

Lines changed: 8 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -100,6 +100,8 @@ describe('OAuth Utils', () => {
100100
}
101101

102102
it('locks per installation and refreshes with the freshest sibling refresh token', async () => {
103+
// The leader rereads the row under the lock.
104+
mockSelectChain([])
103105
mockSelectOrderedChain([
104106
{ accessToken: 'stale-at', refreshToken: 'live-rt', accessTokenExpiresAt: past },
105107
])
@@ -127,6 +129,8 @@ describe('OAuth Utils', () => {
127129
})
128130

129131
it('returns the freshest sibling token without refreshing when it is still valid', async () => {
132+
// The leader rereads the row under the lock.
133+
mockSelectChain([])
130134
mockSelectOrderedChain([
131135
{ accessToken: 'sibling-at', refreshToken: 'live-rt', accessTokenExpiresAt: future },
132136
])
@@ -148,6 +152,8 @@ describe('OAuth Utils', () => {
148152
del: vi.fn().mockResolvedValue(1),
149153
}
150154
redisConfigMockFns.mockGetRedisClient.mockReturnValue(fakeRedis)
155+
// The leader rereads the row under the lock.
156+
mockSelectChain([])
151157
mockSelectOrderedChain([
152158
{ accessToken: 'stale-at', refreshToken: 'live-rt', accessTokenExpiresAt: past },
153159
])
@@ -177,6 +183,8 @@ describe('OAuth Utils', () => {
177183
del: vi.fn().mockResolvedValue(1),
178184
}
179185
redisConfigMockFns.mockGetRedisClient.mockReturnValue(fakeRedis)
186+
// The leader rereads the row under the lock.
187+
mockSelectChain([])
180188
mockSelectOrderedChain([
181189
{ accessToken: 'stale-at', refreshToken: 'live-rt', accessTokenExpiresAt: past },
182190
])

‎apps/sim/lib/auth/auth.ts‎

Lines changed: 12 additions & 14 deletions
Original file line numberDiff line numberDiff line change
@@ -560,11 +560,15 @@ export const auth = betterAuth({
560560
// proves the installation has live tokens, and a fan-out
561561
// failure must not leave the hour-long flag blocking refreshes.
562562
await clearOAuthRefreshDeadFlag(`slack:${teamId}`)
563-
await fanOutSlackTokenChain(teamId, {
564-
accessToken: account.accessToken,
565-
refreshToken: account.refreshToken ?? null,
566-
accessTokenExpiresAt: account.accessTokenExpiresAt ?? null,
567-
})
563+
await fanOutSlackTokenChain(
564+
teamId,
565+
{
566+
accessToken: account.accessToken,
567+
refreshToken: account.refreshToken ?? null,
568+
accessTokenExpiresAt: account.accessTokenExpiresAt ?? null,
569+
},
570+
{ freshlyIssued: true }
571+
)
568572
logger.info('[account.create.after] Propagated Slack installation token chain', {
569573
userId: account.userId,
570574
teamId,
@@ -711,15 +715,9 @@ export const auth = betterAuth({
711715
after: async (account, context) => {
712716
const path = context?.path
713717
if (!path?.startsWith('/oauth2/callback/') && !path?.startsWith('/callback/')) return
714-
try {
715-
await clearOAuthRefreshFailure(account.id)
716-
} catch (error) {
717-
logger.error('[account.update.after] Failed to clear recorded refresh failures', {
718-
accountId: account.id,
719-
providerId: account.providerId,
720-
error,
721-
})
722-
}
718+
// Fails the callback like the draft hooks do, so a reconnect never reports success
719+
// while the old revocation still blocks the credential.
720+
await clearOAuthRefreshFailure(account.id)
723721
},
724722
},
725723
},

‎apps/sim/lib/auth/connectors/managed-oauth.ts‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -9,8 +9,8 @@ import { readResponseJsonWithLimit } from '@/lib/core/utils/stream-limits'
99
import { getDocusignOAuthUrl } from '@/lib/oauth/docusign'
1010
import { verifyGitHubRepositoriesIdentity } from '@/lib/oauth/github-repositories'
1111
import { deriveMicrosoftEmailVerified, mapMicrosoftProfileToUser } from '@/lib/oauth/microsoft'
12+
import { isTerminalRefreshError } from '@/lib/oauth/refresh-error-codes'
1213
import { SALESFORCE_LOGIN_HOSTS } from '@/lib/oauth/salesforce'
13-
import { isTerminalRefreshError } from '@/lib/oauth/terminal-errors'
1414
import { getCanonicalScopesForProvider, isScopeSatisfiedBy } from '@/lib/oauth/utils'
1515
import { MONDAY_API_URL, MONDAY_API_VERSION } from '@/tools/monday/utils'
1616

‎apps/sim/lib/knowledge/connectors/sync-engine.ts‎

Lines changed: 7 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -96,7 +96,7 @@ import {
9696
import { hardDeleteDocuments } from '@/lib/knowledge/documents/service'
9797
import { getRetryAfterMs, isRateLimitError } from '@/lib/knowledge/documents/utils'
9898
import { getCredentialTerminalRefreshError } from '@/lib/oauth/credential-service'
99-
import { isCredentialRevocationError } from '@/lib/oauth/terminal-errors'
99+
import { isCredentialRevocationError } from '@/lib/oauth/refresh-error-codes'
100100
import { connectorHasAuthSource } from '@/connectors/auth'
101101
import { CONNECTOR_REGISTRY } from '@/connectors/registry.server'
102102
import type {
@@ -823,19 +823,15 @@ async function resolveAccessToken(
823823
connectorConfig.auth.mode === 'oauth' && connector.credentialId
824824
? await getCredentialRevocationError(connector.credentialId)
825825
: null
826-
const logContext = {
827-
credentialId: connector.credentialId,
828-
userId,
829-
authMode: connectorConfig.auth.mode,
830-
}
826+
// executeSync logs the revocation once when it unschedules the connector.
831827
if (revocationError && connector.credentialId) {
832-
logger.warn(`[${requestId}] Connector credential was revoked by the source`, {
833-
...logContext,
834-
errorCode: revocationError,
835-
})
836828
throw new ConnectorCredentialRevokedError(connector.credentialId, revocationError)
837829
}
838-
logger.error(`[${requestId}] Connector credential resolved no access token`, logContext)
830+
logger.error(`[${requestId}] Connector credential resolved no access token`, {
831+
credentialId: connector.credentialId,
832+
userId,
833+
authMode: connectorConfig.auth.mode,
834+
})
839835
throw new Error(`Failed to obtain access token for credential ${connector.credentialId}`)
840836
}
841837

‎apps/sim/lib/oauth/credential-revocation.integration.ts‎

Lines changed: 25 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -332,6 +332,31 @@ describe('OAuth refresh-token revocation against PostgreSQL and Redis', () => {
332332
expect(providerRequests).toBe(2)
333333
})
334334

335+
it('keeps a reconnect that lands while a rejected refresh is in flight', async () => {
336+
pausedProvider = { reached: createDeferred<void>(), release: createDeferred<void>() }
337+
const inFlight = resolveToken().catch((error: unknown) => error)
338+
await pausedProvider.reached.promise
339+
await db
340+
.update(account)
341+
.set({
342+
accessToken: 'relinked-access',
343+
accessTokenExpiresAt: new Date(Date.now() + 3_600_000),
344+
updatedAt: new Date(),
345+
})
346+
.where(eq(account.id, accountId))
347+
await handleReconnectCredential({
348+
draft: { credentialId },
349+
newAccountId: accountId,
350+
workspaceId,
351+
userId,
352+
now: new Date(),
353+
})
354+
pausedProvider.release.resolve()
355+
356+
expect(await inFlight).not.toBeInstanceOf(CredentialRevokedError)
357+
expect((await storedAccount()).refreshRevokedAt).toBeNull()
358+
})
359+
335360
it('answers a waiting refresh from the revocation another process recorded', async () => {
336361
await expect(resolveToken()).rejects.toBeInstanceOf(CredentialRevokedError)
337362
const { refreshRevokedAt, refreshRevokedCode, refreshRevokedTokenHash } = await storedAccount()

‎apps/sim/lib/oauth/credential-service.test.ts‎

Lines changed: 13 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -57,9 +57,12 @@ vi.mock('@/lib/oauth/slack', () => ({
5757

5858
vi.mock('@/lib/oauth/terminal-errors', () => ({
5959
getRecentTerminalError: hoisted.getRecentTerminalError,
60+
markCredentialDead: vi.fn(),
61+
}))
62+
63+
vi.mock('@/lib/oauth/refresh-error-codes', () => ({
6064
isCredentialRevocationError: vi.fn(() => false),
6165
isTerminalRefreshError: vi.fn(() => false),
62-
markCredentialDead: vi.fn(),
6366
}))
6467

6568
import {
@@ -74,8 +77,9 @@ import {
7477
import { isInstagramProvider, shouldProactivelyRefreshInstagramToken } from '@/lib/oauth/instagram'
7578
import { isMicrosoftProvider } from '@/lib/oauth/microsoft'
7679
import { getOAuthRefreshCoordinationIdentity } from '@/lib/oauth/refresh-coordination'
80+
import { isTerminalRefreshError } from '@/lib/oauth/refresh-error-codes'
7781
import { fanOutSlackTokenChain } from '@/lib/oauth/slack'
78-
import { isTerminalRefreshError, markCredentialDead } from '@/lib/oauth/terminal-errors'
82+
import { markCredentialDead } from '@/lib/oauth/terminal-errors'
7983
import { GOOGLE_SERVICE_ACCOUNT_PROVIDER_ID } from '@/lib/oauth/types'
8084

8185
const serviceLogger = getMockLogger('OAuthCredentialService')
@@ -505,6 +509,7 @@ describe('OAuth access-token refresh headroom', () => {
505509
it('returns no token when the rotation write finds the account gone', async () => {
506510
queueCredentialAccount(createOAuthAccount())
507511
dbChainMockFns.returning.mockResolvedValueOnce([])
512+
queueTableRows(account, [createOAuthAccount()])
508513
queueTableRows(account, [])
509514
await expect(
510515
resolveCredentialTokenBundle(RAW_CREDENTIAL_ID, RAW_USER_ID, 'test')
@@ -515,6 +520,7 @@ describe('OAuth access-token refresh headroom', () => {
515520
queueCredentialAccount(createOAuthAccount())
516521
vi.mocked(isTerminalRefreshError).mockReturnValue(true)
517522
mocks.refreshOAuthToken.mockResolvedValue({ ok: false, errorCode: 'invalid_client' })
523+
queueTableRows(account, [createOAuthAccount()])
518524
queueTableRows(account, [
519525
{
520526
...createOAuthAccount(3_600_000),
@@ -533,6 +539,7 @@ describe('OAuth access-token refresh headroom', () => {
533539
vi.mocked(isTerminalRefreshError).mockReturnValue(true)
534540
mocks.refreshOAuthToken.mockResolvedValue({ ok: false, errorCode: 'invalid_client' })
535541
queueTableRows(account, [createOAuthAccount()])
542+
queueTableRows(account, [createOAuthAccount()])
536543
await expect(
537544
resolveCredentialTokenBundle(RAW_CREDENTIAL_ID, RAW_USER_ID, 'test')
538545
).resolves.toBeNull()
@@ -543,6 +550,7 @@ describe('OAuth access-token refresh headroom', () => {
543550
queueCredentialAccount(createOAuthAccount())
544551
/** Another writer rotated first: no row still holds the token this refresh started from. */
545552
dbChainMockFns.returning.mockResolvedValueOnce([])
553+
queueTableRows(account, [createOAuthAccount()])
546554
queueTableRows(account, [{ ...createOAuthAccount(3_600_000), accessToken: 'winner-token' }])
547555
await expect(
548556
resolveCredentialTokenBundle(RAW_CREDENTIAL_ID, RAW_USER_ID, 'test')
@@ -578,7 +586,9 @@ describe('OAuth access-token refresh headroom', () => {
578586
expect.objectContaining({
579587
accessToken: refresh ? 'refreshed-access-token' : 'installation-access-token',
580588
}),
581-
{ ifChainUnchangedSince: chainVersion }
589+
refresh
590+
? { ifChainUnchangedSince: chainVersion, freshlyIssued: true }
591+
: { ifChainUnchangedSince: chainVersion }
582592
)
583593
}
584594
)

0 commit comments

Comments
 (0)