Skip to content

Commit 8300f8f

Browse files
fix(connected-accounts): handle state upgrades and align regression coverage
1 parent 5279d61 commit 8300f8f

21 files changed

Lines changed: 155 additions & 41 deletions

File tree

apps/sim/app/(auth)/oauth/sign-in/route.test.ts

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -151,7 +151,7 @@ describe('OAuth login bridge', () => {
151151
})
152152
)
153153
expect(response.status).toBe(307)
154-
expect(response.headers.get('location')).toBe('https://sim.test/workspace')
154+
expect(response.headers.get('location')).toBe('https://sim.test/home')
155155
}
156156
})
157157

apps/sim/app/api/credential-groups/oauth-callback.ts

Lines changed: 7 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -5,6 +5,7 @@ import { NextResponse } from 'next/server'
55
import type { CredentialGroupOAuthCallbackQuery } from '@/lib/api/contracts/credential-groups'
66
import { credentialGroupOAuthAttemptPrincipal } from '@/lib/credential-groups/application/enrollment-auth'
77
import { completePublicCredentialGroupOAuth } from '@/lib/credential-groups/application/public-enrollment'
8+
import { CredentialGroupOAuthStateVersionError } from '@/lib/credential-groups/oauth-attempt-version'
89
import { consumeCredentialGroupOAuthAttempt } from '@/lib/credential-groups/oauth-state'
910
import {
1011
CredentialGroupInvitationUnavailableError,
@@ -38,6 +39,12 @@ export async function handleCredentialGroupOAuthCallback({
3839
try {
3940
attempt = await consumeCredentialGroupOAuthAttempt(state)
4041
} catch (error) {
42+
if (error instanceof CredentialGroupOAuthStateVersionError) {
43+
return NextResponse.json(
44+
{ error: error.message },
45+
{ status: 400, headers: { 'Cache-Control': 'no-store' } }
46+
)
47+
}
4148
logger.error('Failed to consume credential group OAuth state', {
4249
error: getErrorMessage(error),
4350
})

apps/sim/app/api/credential-groups/oauth/[provider]/callback/route.test.ts

Lines changed: 11 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -3,6 +3,7 @@
33
*/
44
import { NextRequest, NextResponse } from 'next/server'
55
import { beforeEach, describe, expect, it, vi } from 'vitest'
6+
import { CredentialGroupOAuthStateVersionError } from '@/lib/credential-groups/oauth-attempt-version'
67

78
const mocks = vi.hoisted(() => ({
89
authenticate: vi.fn(),
@@ -249,4 +250,14 @@ describe('credential group OAuth callback', () => {
249250
expect(response.headers.get('location')).toBe('/credential-groups/complete?oauth=rate_limited')
250251
expect(mocks.completeOAuth).not.toHaveBeenCalled()
251252
})
253+
it('reports a state protocol change as an explicit restart without exchanging a code', async () => {
254+
mocks.consumeAttempt.mockRejectedValue(new CredentialGroupOAuthStateVersionError())
255+
const response = await GET(request('state=state-1&code=code-1'), context)
256+
expect(response.status).toBe(400)
257+
expect(await response.json()).toEqual({
258+
error: expect.stringContaining('Reopen your invitation and connect again'),
259+
})
260+
expect(mocks.authenticate).not.toHaveBeenCalled()
261+
expect(mocks.completeOAuth).not.toHaveBeenCalled()
262+
})
252263
})

apps/sim/app/api/mcp/oauth/callback/route.test.ts

Lines changed: 10 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -10,6 +10,7 @@ import {
1010
} from '@sim/testing'
1111
import { NextRequest } from 'next/server'
1212
import { beforeEach, describe, expect, it, vi } from 'vitest'
13+
import { CredentialGroupOAuthStateVersionError } from '@/lib/credential-groups/oauth-attempt-version'
1314

1415
const {
1516
mockAuthenticateEnrollment,
@@ -195,4 +196,13 @@ describe('MCP OAuth callback route', () => {
195196
expect(mockConsumeManagedAttempt).not.toHaveBeenCalled()
196197
expect(mockCompleteManagedMcpOAuth).not.toHaveBeenCalled()
197198
})
199+
it('reports a state protocol change without exchanging a code or loading an enrollment', async () => {
200+
mockConsumeManagedAttempt.mockRejectedValue(new CredentialGroupOAuthStateVersionError())
201+
const response = await GET(
202+
new NextRequest('http://localhost:3000/api/mcp/oauth/callback?state=mcp_cg_old&code=code')
203+
)
204+
expect(await response.text()).toContain('Reopen your invitation and connect again')
205+
expect(mockAuthenticateEnrollment).not.toHaveBeenCalled()
206+
expect(mockCompleteManagedMcpOAuth).not.toHaveBeenCalled()
207+
})
198208
})

apps/sim/app/api/mcp/oauth/callback/route.ts

Lines changed: 10 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -15,6 +15,7 @@ import {
1515
consumeCredentialGroupMcpOAuthAttempt,
1616
isCredentialGroupMcpOAuthState,
1717
} from '@/lib/credential-groups/mcp-oauth-state'
18+
import { CredentialGroupOAuthStateVersionError } from '@/lib/credential-groups/oauth-attempt-version'
1819
import { enforcePublicCredentialGroupIpRateLimit } from '@/lib/credential-groups/rate-limit'
1920
import {
2021
assertSafeOauthServerUrl,
@@ -84,7 +85,15 @@ async function completeManagedMcpCallback(params: {
8485
code?: string
8586
error?: string
8687
}): Promise<NextResponse> {
87-
const attempt = await consumeCredentialGroupMcpOAuthAttempt(params.state)
88+
let attempt
89+
try {
90+
attempt = await consumeCredentialGroupMcpOAuthAttempt(params.state)
91+
} catch (error) {
92+
if (error instanceof CredentialGroupOAuthStateVersionError) {
93+
return htmlClose(error.message, false, 'invalid_state', undefined, params.state)
94+
}
95+
throw error
96+
}
8897
if (!attempt) {
8998
return htmlClose('Invalid or expired authorization state.', false, 'invalid_state')
9099
}

apps/sim/app/api/organizations/[id]/connected-accounts/[groupId]/slack-managed-users/route.test.ts

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -18,7 +18,8 @@ import { credentialGroupOperations } from '@/lib/credential-groups/application/o
1818
import { POST } from '@/app/api/organizations/[id]/connected-accounts/[groupId]/slack-managed-users/route'
1919

2020
const body = {
21-
slackBotCredentialId: '11111111-1111-4111-8111-111111111111',
21+
appId: 'A123',
22+
teamId: 'T123',
2223
clientId: 'fixture-client-id',
2324
clientSecret: 'fixture-client-secret',
2425
}

apps/sim/app/api/workspaces/invitations/route.test.ts

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -119,7 +119,9 @@ describe('POST /api/workspaces/invitations/batch', () => {
119119
beforeEach(() => {
120120
vi.clearAllMocks()
121121
resetDbChainMock()
122+
queueTableRows(schemaMock.user, [{ id: 'user-1', name: 'Owner User', email: 'owner@test.com' }])
122123
mockGetSession.mockResolvedValue({
124+
session: { id: 'session-1' },
123125
user: { id: 'user-1', email: 'owner@test.com', name: 'Owner User' },
124126
})
125127
mockGetWorkspaceWithOwner.mockResolvedValue({
@@ -171,6 +173,7 @@ describe('POST /api/workspaces/invitations/batch', () => {
171173

172174
afterAll(() => {
173175
resetDbChainMock()
176+
queueTableRows(schemaMock.user, [{ id: 'user-1', name: 'Owner User', email: 'owner@test.com' }])
174177
})
175178

176179
it('blocks invites for personal workspaces with an upgrade prompt', async () => {

apps/sim/app/credential-groups/enroll/[token]/page.test.tsx

Lines changed: 7 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1,5 +1,6 @@
11
/** @vitest-environment jsdom */
22
import type { ReactNode } from 'react'
3+
import { authMockFns } from '@sim/testing'
34
import { renderToStaticMarkup } from 'react-dom/server'
45
import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest'
56
import type { PublicCredentialGroupEnrollment } from '@/lib/credential-groups/enrollments'
@@ -21,6 +22,8 @@ vi.mock('@/lib/credential-groups/rate-limit', () => ({
2122
enforcePublicCredentialGroupIpRateLimit: mocks.rateLimit,
2223
}))
2324
vi.mock('@/lib/credential-groups/providers', () => ({
25+
CREDENTIAL_GROUP_PROVIDER_IDS: ['confluence', 'slack'],
26+
CREDENTIAL_GROUP_STANDARD_OAUTH_PROVIDER_IDS: ['confluence'],
2427
getCredentialGroupProviderService: (provider: string) => ({
2528
providerId: provider,
2629
name: provider === 'confluence' ? 'Confluence' : 'Slack',
@@ -77,6 +80,10 @@ function oauthLinks() {
7780

7881
beforeEach(() => {
7982
vi.clearAllMocks()
83+
authMockFns.mockGetSession.mockResolvedValue({
84+
user: { id: 'member', email: 'member@example.test', emailVerified: true },
85+
session: { id: 'session-1' },
86+
})
8087
mocks.authenticate.mockResolvedValue(principal)
8188
mocks.rateLimit.mockResolvedValue(null)
8289
enrollment = {

apps/sim/app/workspace/[workspaceId]/w/[workflowId]/components/panel/components/editor/components/sub-block/components/dropdown/dropdown.test.tsx

Lines changed: 4 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -76,6 +76,9 @@ vi.mock(
7676
'@/app/workspace/[workspaceId]/w/[workflowId]/components/panel/components/editor/components/sub-block/components/workflow-search-highlight',
7777
() => ({ getWorkflowSearchLabelHighlight: () => undefined })
7878
)
79+
vi.mock('@/hooks/queries/organization-accounts', () => ({
80+
useWorkspaceOrganizationAccounts: () => ({ data: { allowed: false } }),
81+
}))
7982
vi.mock('@/hooks/use-operation-access', () => ({
8083
useOperationAccess: () => ({
8184
getDeniedOperations: () => new Set<string>(),
@@ -89,7 +92,7 @@ vi.mock('@/stores/workflows/workflow/store', () => ({
8992
}))
9093
vi.mock('@/stores/workflows/registry/store', () => ({
9194
useWorkflowRegistry: (selector: (state: unknown) => unknown) =>
92-
selector({ activeWorkflowId: 'wf-1' }),
95+
selector({ activeWorkflowId: 'wf-1', hydration: { workspaceId: 'workspace-1' } }),
9396
}))
9497
vi.mock('@/stores/workflows/subblock/store', () => ({
9598
useSubBlockStore: (selector: (state: unknown) => unknown) => selector({ workflowValues: {} }),

apps/sim/blocks/blocks/credential-group.test.ts

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -9,7 +9,8 @@ import { CredentialGroupBlock } from '@/blocks/blocks/credential-group'
99

1010
describe('Connected Accounts block', () => {
1111
it('uses the workspace container without selectable or manual group inputs', () => {
12-
expect(CredentialGroupBlock.name).toBe('Connected Accounts')
12+
expect(CredentialGroupBlock.name).toBe('Connected Accounts (Legacy)')
13+
expect(CredentialGroupBlock.hideFromToolbar).toBe(true)
1314
const operation = CredentialGroupBlock.subBlocks.find((field) => field.id === 'operation')
1415
expect(operation?.options).toEqual([
1516
{ label: 'List Credentials', id: 'list_credentials' },

0 commit comments

Comments
 (0)