Skip to content

Commit e4b09dc

Browse files
fix(credentials): bind OAuth links to connection intent
1 parent f0767d3 commit e4b09dc

4 files changed

Lines changed: 74 additions & 22 deletions

File tree

apps/sim/app/api/auth/oauth2/authorize/route.test.ts

Lines changed: 7 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -214,9 +214,7 @@ describe('OAuth2 authorize route', () => {
214214
})
215215
)
216216
expect(dbChainMockFns.onConflictDoUpdate).toHaveBeenCalledWith(
217-
expect.objectContaining({
218-
set: expect.objectContaining({ credentialId: null }),
219-
})
217+
expect.objectContaining({ setWhere: expect.anything() })
220218
)
221219
})
222220

@@ -232,11 +230,12 @@ describe('OAuth2 authorize route', () => {
232230
)
233231
})
234232

235-
it('nulls out credentialId in the upsert set so a stale reconnect draft cannot leak into a plain connect', async () => {
233+
it('does not overwrite a reconnect intent when refreshing a plain connect', async () => {
236234
await GET(authorizeRequest({ providerId: 'google-email', workspaceId: WORKSPACE_ID }))
237235

238-
const [{ set }] = dbChainMockFns.onConflictDoUpdate.mock.calls[0]
239-
expect(set).toHaveProperty('credentialId', null)
236+
const [{ set, setWhere }] = dbChainMockFns.onConflictDoUpdate.mock.calls[0]
237+
expect(set).not.toHaveProperty('credentialId')
238+
expect(setWhere).toBeDefined()
240239
})
241240

242241
it('rejects an OAuth client that is not configured for the deployment', async () => {
@@ -283,7 +282,7 @@ describe('OAuth2 authorize route', () => {
283282
})
284283

285284
describe('reconnect (credentialId present)', () => {
286-
it('creates a reconnect draft carrying credentialId in values and upsert set', async () => {
285+
it('creates a reconnect draft and guards conflict refreshes by intent', async () => {
287286
mockGetCredentialActorContext.mockResolvedValue(oauthCredentialActor())
288287

289288
const response = await GET(
@@ -304,9 +303,7 @@ describe('OAuth2 authorize route', () => {
304303
expect.objectContaining({ credentialId: CREDENTIAL_ID })
305304
)
306305
expect(dbChainMockFns.onConflictDoUpdate).toHaveBeenCalledWith(
307-
expect.objectContaining({
308-
set: expect.objectContaining({ credentialId: CREDENTIAL_ID }),
309-
})
306+
expect.objectContaining({ setWhere: expect.anything() })
310307
)
311308
})
312309

apps/sim/app/api/v2/credential-connections/route.test.ts

Lines changed: 30 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -12,6 +12,7 @@ import {
1212
import { NextRequest } from 'next/server'
1313
import { beforeEach, describe, expect, it, vi } from 'vitest'
1414
import { WorkspaceApiKeyScopeAuthorizationError } from '@/lib/core/application'
15+
import { OrchestrationError } from '@/lib/core/orchestration/types'
1516

1617
const mocks = vi.hoisted(() => ({ execute: vi.fn() }))
1718

@@ -147,4 +148,33 @@ describe('POST /api/v2/credential-connections', () => {
147148
error: { code: 'NOT_FOUND', message: 'Workspace not found' },
148149
})
149150
})
151+
152+
it('returns a conflict when another intent already owns the active provider draft', async () => {
153+
mocks.execute.mockRejectedValueOnce(
154+
new OrchestrationError(
155+
'conflict',
156+
'A different OAuth connection flow is already active for this provider'
157+
)
158+
)
159+
160+
const response = await POST(
161+
new NextRequest('http://localhost:3000/api/v2/credential-connections', {
162+
method: 'POST',
163+
headers: { 'content-type': 'application/json' },
164+
body: JSON.stringify({
165+
workspaceId: WORKSPACE_ID,
166+
providerId: 'google-email',
167+
displayName: 'Work Gmail',
168+
}),
169+
})
170+
)
171+
172+
expect(response.status).toBe(409)
173+
expect(await response.json()).toEqual({
174+
error: {
175+
code: 'CONFLICT',
176+
message: 'A different OAuth connection flow is already active for this provider',
177+
},
178+
})
179+
})
150180
})

apps/sim/lib/credentials/connect-draft.test.ts

Lines changed: 22 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -19,7 +19,7 @@ describe('createConnectDraft', () => {
1919
mockGenerateId.mockReturnValue('new-draft-id')
2020
})
2121

22-
it('preserves the active draft ID when refreshing the same connection intent', async () => {
22+
it('refreshes the expiry without changing an active connection intent', async () => {
2323
const expiresAt = new Date('2026-08-13T20:15:00.000Z')
2424
dbChainMockFns.returning.mockResolvedValueOnce([{ id: 'active-draft-id', expiresAt }])
2525

@@ -34,13 +34,29 @@ describe('createConnectDraft', () => {
3434
expect.objectContaining({ id: 'new-draft-id' })
3535
)
3636
const conflict = dbChainMockFns.onConflictDoUpdate.mock.calls[0]?.[0] as
37-
| { set?: Record<string, unknown> }
37+
| { set?: Record<string, unknown>; setWhere?: unknown }
3838
| undefined
3939
expect(conflict?.set).not.toHaveProperty('id')
40-
expect(conflict?.set).toMatchObject({
41-
displayName: 'Work Gmail',
42-
credentialId: null,
43-
})
40+
expect(conflict?.set).not.toHaveProperty('displayName')
41+
expect(conflict?.set).not.toHaveProperty('credentialId')
42+
expect(conflict?.setWhere).toBeDefined()
4443
expect(result).toEqual({ id: 'active-draft-id', expiresAt })
4544
})
45+
46+
it('fails fast when an active draft has a different connection intent', async () => {
47+
dbChainMockFns.returning.mockResolvedValueOnce([])
48+
49+
await expect(
50+
createConnectDraft({
51+
userId: 'user-1',
52+
workspaceId: 'workspace-1',
53+
providerId: 'google-email',
54+
credentialId: 'credential-1',
55+
displayName: 'Existing Gmail',
56+
})
57+
).rejects.toMatchObject({
58+
code: 'conflict',
59+
message: 'A different OAuth connection flow is already active for this provider',
60+
})
61+
})
4662
})

apps/sim/lib/credentials/connect-draft.ts

Lines changed: 15 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -2,7 +2,8 @@ import { db } from '@sim/db'
22
import { credential, pendingCredentialDraft, user } from '@sim/db/schema'
33
import { createLogger } from '@sim/logger'
44
import { generateId } from '@sim/utils/id'
5-
import { and, eq, gt, lt } from 'drizzle-orm'
5+
import { and, eq, gt, isNull, lt } from 'drizzle-orm'
6+
import { OrchestrationError } from '@/lib/core/orchestration/types'
67
import { defaultCredentialDisplayName } from '@/lib/credentials/display-name'
78
import { credentialProviderMatchesService, getAllOAuthServices } from '@/lib/oauth/utils'
89

@@ -84,14 +85,22 @@ export async function createConnectDraft(params: {
8485
pendingCredentialDraft.providerId,
8586
pendingCredentialDraft.workspaceId,
8687
],
87-
// credentialId must be written on BOTH paths: a plain connect that reuses a
88-
// stale reconnect draft row would otherwise silently rebind the old
89-
// credential instead of creating a new one.
90-
set: { displayName, credentialId: credentialId ?? null, expiresAt, createdAt: now },
88+
set: { expiresAt, createdAt: now },
89+
setWhere: and(
90+
eq(pendingCredentialDraft.displayName, displayName),
91+
credentialId
92+
? eq(pendingCredentialDraft.credentialId, credentialId)
93+
: isNull(pendingCredentialDraft.credentialId)
94+
),
9195
})
9296
.returning({ id: pendingCredentialDraft.id, expiresAt: pendingCredentialDraft.expiresAt })
9397

94-
if (!draft) throw new Error('OAuth connect draft insert returned no row')
98+
if (!draft) {
99+
throw new OrchestrationError(
100+
'conflict',
101+
'A different OAuth connection flow is already active for this provider'
102+
)
103+
}
95104

96105
logger.info('Created OAuth connect credential draft', {
97106
userId,

0 commit comments

Comments
 (0)