From 5487951ea592cc7a1b8c1dfba9d8334281d3779a Mon Sep 17 00:00:00 2001 From: Justin Blumencranz <96924014+j15z@users.noreply.github.com> Date: Tue, 7 Jul 2026 12:44:13 -0700 Subject: [PATCH 1/3] feat(credentials): agent-initiated oauth credential reconnect --- .../api/auth/oauth2/authorize/route.test.ts | 289 ++++++++++++++++++ .../app/api/auth/oauth2/authorize/route.ts | 87 +++++- .../connect-oauth-modal.tsx | 53 +--- .../special-tags/special-tags.test.tsx | 47 ++- .../components/special-tags/special-tags.tsx | 58 ++-- .../lib/api/contracts/oauth-connections.ts | 1 + .../lib/copilot/generated/tool-catalog-v1.ts | 5 + .../lib/copilot/generated/tool-schemas-v1.ts | 5 + .../lib/copilot/tools/handlers/oauth.test.ts | 188 ++++++++++++ apps/sim/lib/copilot/tools/handlers/oauth.ts | 56 +++- apps/sim/lib/credentials/display-name.test.ts | 57 ++++ apps/sim/lib/credentials/display-name.ts | 49 +++ 12 files changed, 809 insertions(+), 86 deletions(-) create mode 100644 apps/sim/app/api/auth/oauth2/authorize/route.test.ts create mode 100644 apps/sim/lib/copilot/tools/handlers/oauth.test.ts create mode 100644 apps/sim/lib/credentials/display-name.test.ts create mode 100644 apps/sim/lib/credentials/display-name.ts diff --git a/apps/sim/app/api/auth/oauth2/authorize/route.test.ts b/apps/sim/app/api/auth/oauth2/authorize/route.test.ts new file mode 100644 index 00000000000..64c9b12bdfc --- /dev/null +++ b/apps/sim/app/api/auth/oauth2/authorize/route.test.ts @@ -0,0 +1,289 @@ +/** + * @vitest-environment node + */ +import { createMockRequest, dbChainMock, dbChainMockFns, resetDbChainMock } from '@sim/testing' +import { beforeEach, describe, expect, it, vi } from 'vitest' + +const { + mockGetSession, + mockOAuth2LinkAccount, + mockCheckWorkspaceAccess, + mockGetCredentialActorContext, +} = vi.hoisted(() => ({ + mockGetSession: vi.fn(), + mockOAuth2LinkAccount: vi.fn(), + mockCheckWorkspaceAccess: vi.fn(), + mockGetCredentialActorContext: vi.fn(), +})) + +vi.mock('@sim/db', () => dbChainMock) + +vi.mock('@/lib/auth/auth', () => ({ + auth: { api: { oAuth2LinkAccount: mockOAuth2LinkAccount } }, + getSession: mockGetSession, +})) + +vi.mock('@/lib/workspaces/permissions/utils', () => ({ + checkWorkspaceAccess: mockCheckWorkspaceAccess, +})) + +vi.mock('@/lib/credentials/access', () => ({ + getCredentialActorContext: mockGetCredentialActorContext, +})) + +vi.mock('@/lib/oauth/utils', () => ({ + getAllOAuthServices: vi.fn(() => [{ providerId: 'google-email', name: 'Gmail' }]), +})) + +import { GET } from '@/app/api/auth/oauth2/authorize/route' + +const BASE_URL = 'https://sim.test' +const WORKSPACE_ID = 'ws-1' +const USER_ID = 'user-1' +const CREDENTIAL_ID = 'cred-1' +const LINK_URL = 'https://provider.example/authorize?state=abc' + +function authorizeRequest(query: Record) { + const url = new URL(`${BASE_URL}/api/auth/oauth2/authorize`) + for (const [key, value] of Object.entries(query)) { + url.searchParams.set(key, value) + } + return createMockRequest('GET', undefined, {}, url.toString()) +} + +function oauthCredentialActor(overrides: Record = {}) { + return { + credential: { + id: CREDENTIAL_ID, + workspaceId: WORKSPACE_ID, + type: 'oauth', + providerId: 'google-email', + ...((overrides.credential as Record) ?? {}), + }, + member: null, + hasWorkspaceAccess: true, + canWriteWorkspace: true, + isAdmin: true, + ...Object.fromEntries(Object.entries(overrides).filter(([key]) => key !== 'credential')), + } +} + +describe('OAuth2 authorize route', () => { + beforeEach(() => { + vi.clearAllMocks() + resetDbChainMock() + process.env.NEXT_PUBLIC_APP_URL = BASE_URL + mockGetSession.mockResolvedValue({ user: { id: USER_ID } }) + mockCheckWorkspaceAccess.mockResolvedValue({ + hasAccess: true, + canWrite: true, + canAdmin: false, + workspace: { id: WORKSPACE_ID }, + }) + mockOAuth2LinkAccount.mockResolvedValue({ + ok: true, + status: 200, + json: async () => ({ url: LINK_URL }), + headers: { getSetCookie: () => ['better-auth.state=xyz; Path=/'] }, + }) + }) + + describe('plain connect (no credentialId)', () => { + it('creates a draft with credentialId null and redirects to the provider', async () => { + const response = await GET( + authorizeRequest({ providerId: 'google-email', workspaceId: WORKSPACE_ID }) + ) + + expect(response.headers.get('location')).toBe(LINK_URL) + expect(mockGetCredentialActorContext).not.toHaveBeenCalled() + expect(dbChainMockFns.values).toHaveBeenCalledWith( + expect.objectContaining({ + userId: USER_ID, + workspaceId: WORKSPACE_ID, + providerId: 'google-email', + credentialId: null, + }) + ) + expect(dbChainMockFns.onConflictDoUpdate).toHaveBeenCalledWith( + expect.objectContaining({ + set: expect.objectContaining({ credentialId: null }), + }) + ) + }) + + it('numbers the draft display name when the default collides with an existing credential', async () => { + dbChainMockFns.where + .mockImplementationOnce(() => Promise.resolve([{ name: 'Justin' }])) + .mockImplementationOnce(() => Promise.resolve([{ displayName: "Justin's Gmail" }])) + + await GET(authorizeRequest({ providerId: 'google-email', workspaceId: WORKSPACE_ID })) + + expect(dbChainMockFns.values).toHaveBeenCalledWith( + expect.objectContaining({ displayName: "Justin's Gmail 2" }) + ) + }) + + it('nulls out credentialId in the upsert set so a stale reconnect draft cannot leak into a plain connect', async () => { + await GET(authorizeRequest({ providerId: 'google-email', workspaceId: WORKSPACE_ID })) + + const [{ set }] = dbChainMockFns.onConflictDoUpdate.mock.calls[0] + expect(set).toHaveProperty('credentialId', null) + }) + + it('redirects to login when unauthenticated', async () => { + mockGetSession.mockResolvedValue(null) + + const response = await GET( + authorizeRequest({ providerId: 'google-email', workspaceId: WORKSPACE_ID }) + ) + + expect(response.headers.get('location')).toContain('/login') + expect(dbChainMockFns.values).not.toHaveBeenCalled() + }) + + it('rejects without workspace write access', async () => { + mockCheckWorkspaceAccess.mockResolvedValue({ + hasAccess: true, + canWrite: false, + canAdmin: false, + workspace: { id: WORKSPACE_ID }, + }) + + const response = await GET( + authorizeRequest({ providerId: 'google-email', workspaceId: WORKSPACE_ID }) + ) + + expect(response.headers.get('location')).toBe( + `${BASE_URL}/workspace?error=workspace_access_denied` + ) + expect(dbChainMockFns.values).not.toHaveBeenCalled() + expect(mockOAuth2LinkAccount).not.toHaveBeenCalled() + }) + }) + + describe('reconnect (credentialId present)', () => { + it('creates a reconnect draft carrying credentialId in values and upsert set', async () => { + mockGetCredentialActorContext.mockResolvedValue(oauthCredentialActor()) + + const response = await GET( + authorizeRequest({ + providerId: 'google-email', + workspaceId: WORKSPACE_ID, + credentialId: CREDENTIAL_ID, + }) + ) + + expect(response.headers.get('location')).toBe(LINK_URL) + expect(mockGetCredentialActorContext).toHaveBeenCalledWith( + CREDENTIAL_ID, + USER_ID, + expect.objectContaining({ workspaceAccess: expect.anything() }) + ) + expect(dbChainMockFns.values).toHaveBeenCalledWith( + expect.objectContaining({ credentialId: CREDENTIAL_ID }) + ) + expect(dbChainMockFns.onConflictDoUpdate).toHaveBeenCalledWith( + expect.objectContaining({ + set: expect.objectContaining({ credentialId: CREDENTIAL_ID }), + }) + ) + }) + + it('rejects when the caller is not a credential admin and writes no draft', async () => { + mockGetCredentialActorContext.mockResolvedValue(oauthCredentialActor({ isAdmin: false })) + + const response = await GET( + authorizeRequest({ + providerId: 'google-email', + workspaceId: WORKSPACE_ID, + credentialId: CREDENTIAL_ID, + }) + ) + + expect(response.headers.get('location')).toBe( + `${BASE_URL}/workspace?error=credential_access_denied` + ) + expect(dbChainMockFns.values).not.toHaveBeenCalled() + expect(mockOAuth2LinkAccount).not.toHaveBeenCalled() + }) + + it('rejects when the credential belongs to a different workspace', async () => { + mockGetCredentialActorContext.mockResolvedValue( + oauthCredentialActor({ credential: { workspaceId: 'ws-other' } }) + ) + + const response = await GET( + authorizeRequest({ + providerId: 'google-email', + workspaceId: WORKSPACE_ID, + credentialId: CREDENTIAL_ID, + }) + ) + + expect(response.headers.get('location')).toBe( + `${BASE_URL}/workspace?error=credential_access_denied` + ) + expect(dbChainMockFns.values).not.toHaveBeenCalled() + }) + + it('rejects when the credential does not exist', async () => { + mockGetCredentialActorContext.mockResolvedValue({ + credential: null, + member: null, + hasWorkspaceAccess: false, + canWriteWorkspace: false, + isAdmin: false, + }) + + const response = await GET( + authorizeRequest({ + providerId: 'google-email', + workspaceId: WORKSPACE_ID, + credentialId: 'cred-missing', + }) + ) + + expect(response.headers.get('location')).toBe( + `${BASE_URL}/workspace?error=credential_access_denied` + ) + expect(dbChainMockFns.values).not.toHaveBeenCalled() + }) + + it('rejects a non-oauth credential', async () => { + mockGetCredentialActorContext.mockResolvedValue( + oauthCredentialActor({ credential: { type: 'env_workspace' } }) + ) + + const response = await GET( + authorizeRequest({ + providerId: 'google-email', + workspaceId: WORKSPACE_ID, + credentialId: CREDENTIAL_ID, + }) + ) + + expect(response.headers.get('location')).toBe( + `${BASE_URL}/workspace?error=credential_access_denied` + ) + expect(dbChainMockFns.values).not.toHaveBeenCalled() + }) + + it('rejects when the query providerId does not match the credential provider', async () => { + mockGetCredentialActorContext.mockResolvedValue(oauthCredentialActor()) + + const response = await GET( + authorizeRequest({ + providerId: 'slack', + workspaceId: WORKSPACE_ID, + credentialId: CREDENTIAL_ID, + }) + ) + + expect(response.headers.get('location')).toBe( + `${BASE_URL}/workspace?error=credential_provider_mismatch` + ) + expect(dbChainMockFns.values).not.toHaveBeenCalled() + expect(mockOAuth2LinkAccount).not.toHaveBeenCalled() + }) + }) +}) diff --git a/apps/sim/app/api/auth/oauth2/authorize/route.ts b/apps/sim/app/api/auth/oauth2/authorize/route.ts index 65bb6a773b9..6ca3b5cf54e 100644 --- a/apps/sim/app/api/auth/oauth2/authorize/route.ts +++ b/apps/sim/app/api/auth/oauth2/authorize/route.ts @@ -1,5 +1,5 @@ import { db } from '@sim/db' -import { pendingCredentialDraft, user } from '@sim/db/schema' +import { credential, pendingCredentialDraft, user } from '@sim/db/schema' import { createLogger } from '@sim/logger' import { generateId } from '@sim/utils/id' import { and, eq, lt } from 'drizzle-orm' @@ -9,6 +9,8 @@ import { parseRequest } from '@/lib/api/server' import { auth, getSession } from '@/lib/auth/auth' import { getBaseUrl } from '@/lib/core/utils/urls' import { withRouteHandler } from '@/lib/core/utils/with-route-handler' +import { getCredentialActorContext } from '@/lib/credentials/access' +import { defaultCredentialDisplayName } from '@/lib/credentials/display-name' import { getAllOAuthServices } from '@/lib/oauth/utils' import { checkWorkspaceAccess } from '@/lib/workspaces/permissions/utils' @@ -30,22 +32,38 @@ async function createConnectDraft(params: { userId: string workspaceId: string providerId: string + credentialId?: string }): Promise { - const { userId, workspaceId, providerId } = params + const { userId, workspaceId, providerId, credentialId } = params const service = getAllOAuthServices().find((s) => s.providerId === providerId) const serviceName = service?.name ?? providerId - let displayName = serviceName + let userName: string | null = null try { const [row] = await db.select({ name: user.name }).from(user).where(eq(user.id, userId)) - if (row?.name) { - displayName = `${row.name}'s ${serviceName}` - } + userName = row?.name ?? null + } catch { + // Fall back to the "My {Service}" default + } + + // Auto-number against existing workspace credentials so repeat connects for + // the same provider stay distinguishable — same behavior as the connect + // modal, which computes this client-side. Best effort: on failure the name + // simply skips deduplication. + let takenNames: ReadonlySet = new Set() + try { + const rows = await db + .select({ displayName: credential.displayName }) + .from(credential) + .where(and(eq(credential.workspaceId, workspaceId), eq(credential.type, 'oauth'))) + takenNames = new Set(rows.map((row) => row.displayName.toLowerCase())) } catch { - // Fall back to service name only + // Best effort — proceed without collision numbering } + const displayName = defaultCredentialDisplayName(userName, serviceName, takenNames) + const now = new Date() const expiresAt = new Date(now.getTime() + DRAFT_TTL_MS) await db @@ -61,6 +79,7 @@ async function createConnectDraft(params: { workspaceId, providerId, displayName, + credentialId: credentialId ?? null, expiresAt, createdAt: now, }) @@ -70,10 +89,18 @@ async function createConnectDraft(params: { pendingCredentialDraft.providerId, pendingCredentialDraft.workspaceId, ], - set: { displayName, expiresAt, createdAt: now }, + // credentialId must be written on BOTH paths: a plain connect that reuses a + // stale reconnect draft row would otherwise silently rebind the old + // credential instead of creating a new one. + set: { displayName, credentialId: credentialId ?? null, expiresAt, createdAt: now }, }) - logger.info('Created OAuth connect credential draft', { userId, workspaceId, providerId }) + logger.info('Created OAuth connect credential draft', { + userId, + workspaceId, + providerId, + credentialId: credentialId ?? null, + }) } /** @@ -92,7 +119,12 @@ export const GET = withRouteHandler(async (request: NextRequest) => { const parsed = await parseRequest(authorizeOAuth2Contract, request, {}) if (!parsed.success) return parsed.response - const { providerId, workspaceId, callbackURL: requestedCallback } = parsed.data.query + const { + providerId, + workspaceId, + callbackURL: requestedCallback, + credentialId, + } = parsed.data.query const callbackURL = requestedCallback?.startsWith(`${baseUrl}/`) ? requestedCallback @@ -109,10 +141,43 @@ export const GET = withRouteHandler(async (request: NextRequest) => { return NextResponse.redirect(`${baseUrl}/workspace?error=workspace_access_denied`) } + if (credentialId) { + // Reconnect: the OAuth callback will rebind this credential to the fresh + // account, so require the same credential-admin access as the draft POST + // route — workspace write alone must not be enough to swap someone's tokens. + const actor = await getCredentialActorContext(credentialId, userId, { + workspaceAccess: access, + }) + if ( + !actor.credential || + actor.credential.workspaceId !== workspaceId || + actor.credential.type !== 'oauth' || + !actor.isAdmin + ) { + logger.warn('Credential admin access denied for OAuth2 reconnect', { + userId, + workspaceId, + providerId, + credentialId, + }) + return NextResponse.redirect(`${baseUrl}/workspace?error=credential_access_denied`) + } + if (actor.credential.providerId !== providerId) { + logger.warn('Provider mismatch for OAuth2 reconnect', { + userId, + workspaceId, + providerId, + credentialId, + credentialProviderId: actor.credential.providerId, + }) + return NextResponse.redirect(`${baseUrl}/workspace?error=credential_provider_mismatch`) + } + } + // Create the draft before initiating the link so it is guaranteed to exist // (and freshly clocked) when the OAuth callback's `account.create.after` // hook runs. If this throws, we never start the OAuth flow. - await createConnectDraft({ userId, workspaceId, providerId }) + await createConnectDraft({ userId, workspaceId, providerId, credentialId }) const linkResponse = await auth.api.oAuth2LinkAccount({ body: { providerId, callbackURL }, diff --git a/apps/sim/app/workspace/[workspaceId]/components/connect-oauth-modal/connect-oauth-modal.tsx b/apps/sim/app/workspace/[workspaceId]/components/connect-oauth-modal/connect-oauth-modal.tsx index fa55168db32..bb9f2618984 100644 --- a/apps/sim/app/workspace/[workspaceId]/components/connect-oauth-modal/connect-oauth-modal.tsx +++ b/apps/sim/app/workspace/[workspaceId]/components/connect-oauth-modal/connect-oauth-modal.tsx @@ -18,6 +18,7 @@ import { getErrorMessage } from '@sim/utils/errors' import { useSession } from '@/lib/auth/auth-client' import type { OAuthReturnContext } from '@/lib/credentials/client-state' import { ADD_CONNECTOR_SEARCH_PARAM, writeOAuthReturnContext } from '@/lib/credentials/client-state' +import { defaultCredentialDisplayName } from '@/lib/credentials/display-name' import { getProviderIdFromServiceId, OAUTH_PROVIDERS, @@ -30,18 +31,6 @@ import { useConnectOAuthService } from '@/hooks/queries/oauth/oauth-connections' const logger = createLogger('ConnectOAuthModal') -/** Server-enforced max for `WorkspaceCredential.displayName` — see `lib/api/contracts/credentials.ts`. */ -const DISPLAY_NAME_MAX_LENGTH = 255 - -/** - * Reserved tail budget when truncating the username so the auto-numbering - * disambiguator (e.g. `" 9999"`) always fits within {@link DISPLAY_NAME_MAX_LENGTH}. - */ -const COLLISION_SUFFIX_RESERVATION = 5 - -/** Upper bound for the auto-numbering search — pathological if ever reached. */ -const MAX_COLLISION_INDEX = 10000 - const EMPTY_SCOPES: readonly string[] = [] type ServiceIcon = ComponentType<{ className?: string }> @@ -51,44 +40,6 @@ function isHiddenScope(scope: string): boolean { return scope.includes('userinfo.email') || scope.includes('userinfo.profile') } -/** - * Default credential display name. Produces `"{Name}'s {Service}"` when the - * user's name is known, falling back to `"My {Service}"` otherwise. The - * username is truncated so the full string (including any auto-numbering - * disambiguator) stays within {@link DISPLAY_NAME_MAX_LENGTH}. - * - * When the base name collides with an existing credential in `takenNames`, - * `" 2"`, `" 3"`, ... are appended until an unused name is found. Comparison - * is case-insensitive to match the duplicate-detection used elsewhere in the - * modal. - */ -function defaultDisplayName( - userName: string | null | undefined, - serviceName: string, - takenNames: ReadonlySet -): string { - const trimmed = userName?.trim() - let base: string - if (trimmed) { - const suffix = `'s ${serviceName}` - const nameBudget = Math.max( - 0, - DISPLAY_NAME_MAX_LENGTH - suffix.length - COLLISION_SUFFIX_RESERVATION - ) - const safeName = trimmed.length > nameBudget ? trimmed.slice(0, nameBudget) : trimmed - base = `${safeName}${suffix}` - } else { - base = `My ${serviceName}` - } - - if (!takenNames.has(base.toLowerCase())) return base - for (let n = 2; n < MAX_COLLISION_INDEX; n++) { - const candidate = `${base} ${n}` - if (!takenNames.has(candidate.toLowerCase())) return candidate - } - return base -} - /** * Resolves the display name + icon for an OAuth `provider`/`serviceId` pair, * preferring the most specific service entry and falling back to the base @@ -255,7 +206,7 @@ export function ConnectOAuthModal(props: ConnectOAuthModalProps) { } if (!isConnect || prefilled.current || credentialsLoading) return prefilled.current = true - setDisplayName(defaultDisplayName(userName, providerName, takenNames)) + setDisplayName(defaultCredentialDisplayName(userName, providerName, takenNames)) setDescription('') setValidationError(null) setSubmitError(null) diff --git a/apps/sim/app/workspace/[workspaceId]/home/components/message-content/components/special-tags/special-tags.test.tsx b/apps/sim/app/workspace/[workspaceId]/home/components/message-content/components/special-tags/special-tags.test.tsx index 15580127c64..a23bc0427d5 100644 --- a/apps/sim/app/workspace/[workspaceId]/home/components/message-content/components/special-tags/special-tags.test.tsx +++ b/apps/sim/app/workspace/[workspaceId]/home/components/message-content/components/special-tags/special-tags.test.tsx @@ -5,8 +5,9 @@ import { act } from 'react' import { createRoot, type Root } from 'react-dom/client' import { beforeEach, describe, expect, it, vi } from 'vitest' -const { mockUseUserPermissionsContext } = vi.hoisted(() => ({ +const { mockUseUserPermissionsContext, mockUseWorkspaceCredential } = vi.hoisted(() => ({ mockUseUserPermissionsContext: vi.fn(), + mockUseWorkspaceCredential: vi.fn(), })) vi.mock('@/app/workspace/[workspaceId]/providers/workspace-permissions-provider', () => ({ @@ -17,6 +18,10 @@ vi.mock('next/navigation', () => ({ useParams: () => ({ workspaceId: 'workspace-1' }), })) +vi.mock('@/hooks/queries/credentials', () => ({ + useWorkspaceCredential: mockUseWorkspaceCredential, +})) + import type { CredentialTagData } from '@/app/workspace/[workspaceId]/home/components/message-content/components/special-tags/special-tags' import { parseSpecialTags, @@ -41,6 +46,7 @@ describe('CredentialDisplay link tag', () => { beforeEach(() => { vi.clearAllMocks() mockUseUserPermissionsContext.mockReturnValue({ canEdit: true }) + mockUseWorkspaceCredential.mockReturnValue({ data: null }) }) it('does not render an anchor for a javascript: scheme value', () => { @@ -91,6 +97,45 @@ describe('CredentialDisplay link tag', () => { expect(container.querySelector('a')).toBeNull() act(() => root.unmount()) }) + + it('does not query a credential for a plain connect URL', () => { + const { root } = renderCredentialLink({ + type: 'link', + provider: 'github', + value: 'https://github.com/login/oauth/authorize?client_id=abc', + }) + + expect(mockUseWorkspaceCredential).toHaveBeenCalledWith(undefined) + act(() => root.unmount()) + }) + + it('labels a reconnect URL with the credential display name', () => { + mockUseWorkspaceCredential.mockReturnValue({ + data: { id: 'cred-1', displayName: "Justin's Gmail" }, + }) + const { container, root } = renderCredentialLink({ + type: 'link', + provider: 'google-email', + value: + 'https://sim.test/api/auth/oauth2/authorize?providerId=google-email&workspaceId=ws-1&credentialId=cred-1', + }) + + expect(mockUseWorkspaceCredential).toHaveBeenCalledWith('cred-1') + expect(container.textContent).toContain("Reconnect Justin's Gmail") + act(() => root.unmount()) + }) + + it('falls back to the provider label while the reconnect credential is unresolved', () => { + const { container, root } = renderCredentialLink({ + type: 'link', + provider: 'google-email', + value: + 'https://sim.test/api/auth/oauth2/authorize?providerId=google-email&workspaceId=ws-1&credentialId=cred-1', + }) + + expect(container.textContent).toContain('Reconnect google-email') + act(() => root.unmount()) + }) }) describe('parseSpecialTags sim_key placeholder', () => { diff --git a/apps/sim/app/workspace/[workspaceId]/home/components/message-content/components/special-tags/special-tags.tsx b/apps/sim/app/workspace/[workspaceId]/home/components/message-content/components/special-tags/special-tags.tsx index 9fd8c7393e1..f99670b75a0 100644 --- a/apps/sim/app/workspace/[workspaceId]/home/components/message-content/components/special-tags/special-tags.tsx +++ b/apps/sim/app/workspace/[workspaceId]/home/components/message-content/components/special-tags/special-tags.tsx @@ -24,6 +24,7 @@ import type { MothershipResource, } from '@/app/workspace/[workspaceId]/home/types' import { useUserPermissionsContext } from '@/app/workspace/[workspaceId]/providers/workspace-permissions-provider' +import { useWorkspaceCredential } from '@/hooks/queries/credentials' import { usePersonalEnvironment, useSavePersonalEnvironment, @@ -867,32 +868,51 @@ function SecretInputDisplay({ data }: { data: CredentialTagData }) { ) } -function CredentialDisplay({ data }: { data: CredentialTagData }) { +function CredentialLinkDisplay({ data }: { data: CredentialTagData }) { const { canEdit } = useUserPermissionsContext() + // A connect URL carrying a credentialId re-authorizes that existing + // credential in place (reconnect) rather than creating a new one. + const reconnectCredentialId = useMemo(() => { + if (!data.value) return undefined + try { + return new URL(data.value).searchParams.get('credentialId') ?? undefined + } catch { + return undefined + } + }, [data.value]) + const { data: reconnectCredential } = useWorkspaceCredential(reconnectCredentialId) + + // Connecting a credential mutates the workspace — hide it from read-only members. + if (!data.provider || !canEdit) return null + // The connect link value comes from the streamed model output, so only + // render it as a clickable link when it resolves to a real http(s) URL. + if (!data.value || !isSafeHttpUrl(data.value)) return null + const Icon = getCredentialIcon(data.provider) ?? LockIcon + const label = reconnectCredentialId + ? `Reconnect ${reconnectCredential?.displayName ?? data.provider}` + : `Connect ${data.provider}` + return ( + + {createElement(Icon, { className: 'size-[16px] shrink-0' })} + {label} + + + ) +} + +function CredentialDisplay({ data }: { data: CredentialTagData }) { if (data.type === 'secret_input') { return } if (data.type === 'link') { - // Connecting a credential mutates the workspace — hide it from read-only members. - if (!data.provider || !canEdit) return null - // The connect link value comes from the streamed model output, so only - // render it as a clickable link when it resolves to a real http(s) URL. - if (!data.value || !isSafeHttpUrl(data.value)) return null - const Icon = getCredentialIcon(data.provider) ?? LockIcon - return ( - - {createElement(Icon, { className: 'size-[16px] shrink-0' })} - Connect {data.provider} - - - ) + return } if (data.type === 'sim_key') { diff --git a/apps/sim/lib/api/contracts/oauth-connections.ts b/apps/sim/lib/api/contracts/oauth-connections.ts index f451f555c1f..4fd6e09a0b8 100644 --- a/apps/sim/lib/api/contracts/oauth-connections.ts +++ b/apps/sim/lib/api/contracts/oauth-connections.ts @@ -195,6 +195,7 @@ export const authorizeOAuth2QuerySchema = z.object({ providerId: z.string().min(1, 'providerId is required'), workspaceId: workspaceIdSchema, callbackURL: z.string().min(1).optional(), + credentialId: z.string().min(1).optional(), }) export const authorizeOAuth2Contract = defineRouteContract({ diff --git a/apps/sim/lib/copilot/generated/tool-catalog-v1.ts b/apps/sim/lib/copilot/generated/tool-catalog-v1.ts index 4006ff7b2d3..6cf35b7fdcc 100644 --- a/apps/sim/lib/copilot/generated/tool-catalog-v1.ts +++ b/apps/sim/lib/copilot/generated/tool-catalog-v1.ts @@ -2779,6 +2779,11 @@ export const OauthGetAuthLink: ToolCatalogEntry = { parameters: { type: 'object', properties: { + credentialId: { + type: 'string', + description: + 'Optional. The id of an EXISTING credential (from environment/credentials.json) to reconnect/re-authorize in place. Only when the user explicitly asks to reconnect or repair that credential — never for adding another account.', + }, providerName: { type: 'string', description: diff --git a/apps/sim/lib/copilot/generated/tool-schemas-v1.ts b/apps/sim/lib/copilot/generated/tool-schemas-v1.ts index 81db4e65bc5..f9bb621223e 100644 --- a/apps/sim/lib/copilot/generated/tool-schemas-v1.ts +++ b/apps/sim/lib/copilot/generated/tool-schemas-v1.ts @@ -2572,6 +2572,11 @@ export const TOOL_RUNTIME_SCHEMAS: Record = { parameters: { type: 'object', properties: { + credentialId: { + type: 'string', + description: + 'Optional. The id of an EXISTING credential (from environment/credentials.json) to reconnect/re-authorize in place. Only when the user explicitly asks to reconnect or repair that credential — never for adding another account.', + }, providerName: { type: 'string', description: diff --git a/apps/sim/lib/copilot/tools/handlers/oauth.test.ts b/apps/sim/lib/copilot/tools/handlers/oauth.test.ts new file mode 100644 index 00000000000..b9f564d18bd --- /dev/null +++ b/apps/sim/lib/copilot/tools/handlers/oauth.test.ts @@ -0,0 +1,188 @@ +/** + * @vitest-environment node + */ +import { beforeEach, describe, expect, it, vi } from 'vitest' + +const { mockEnsureWorkspaceAccess, mockGetCredentialActorContext } = vi.hoisted(() => ({ + mockEnsureWorkspaceAccess: vi.fn(), + mockGetCredentialActorContext: vi.fn(), +})) + +vi.mock('@/lib/copilot/tools/handlers/access', () => ({ + ensureWorkspaceAccess: mockEnsureWorkspaceAccess, +})) + +vi.mock('@/lib/credentials/access', () => ({ + getCredentialActorContext: mockGetCredentialActorContext, +})) + +vi.mock('@/lib/oauth/utils', () => ({ + getAllOAuthServices: vi.fn(() => [ + { providerId: 'google-email', name: 'Gmail' }, + { providerId: 'slack', name: 'Slack' }, + { providerId: 'trello', name: 'Trello' }, + { providerId: 'shopify', name: 'Shopify' }, + ]), +})) + +import type { ExecutionContext } from '@/lib/copilot/request/types' +import { executeOAuthGetAuthLink } from '@/lib/copilot/tools/handlers/oauth' + +const BASE_URL = 'https://sim.test' +const WORKSPACE_ID = 'ws-1' +const USER_ID = 'user-1' +const CREDENTIAL_ID = 'cred-1' + +const context = { + workspaceId: WORKSPACE_ID, + userId: USER_ID, + chatId: 'chat-1', +} as unknown as ExecutionContext + +function oauthCredentialActor(overrides: Record = {}) { + return { + credential: { + id: CREDENTIAL_ID, + workspaceId: WORKSPACE_ID, + type: 'oauth', + providerId: 'google-email', + ...((overrides.credential as Record) ?? {}), + }, + member: null, + hasWorkspaceAccess: true, + canWriteWorkspace: true, + isAdmin: true, + ...Object.fromEntries(Object.entries(overrides).filter(([key]) => key !== 'credential')), + } +} + +describe('executeOAuthGetAuthLink', () => { + beforeEach(() => { + vi.clearAllMocks() + process.env.NEXT_PUBLIC_APP_URL = BASE_URL + mockEnsureWorkspaceAccess.mockResolvedValue(undefined) + }) + + describe('connect (no credentialId)', () => { + it('returns an authorize URL without a credentialId param', async () => { + const result = await executeOAuthGetAuthLink({ providerName: 'google-email' }, context) + + expect(result.success).toBe(true) + const url = new URL((result.output as { oauth_url: string }).oauth_url) + expect(url.pathname).toBe('/api/auth/oauth2/authorize') + expect(url.searchParams.get('providerId')).toBe('google-email') + expect(url.searchParams.get('credentialId')).toBeNull() + expect(mockGetCredentialActorContext).not.toHaveBeenCalled() + }) + }) + + describe('reconnect (credentialId passed)', () => { + it('returns an authorize URL carrying the credentialId and a reconnect message', async () => { + mockGetCredentialActorContext.mockResolvedValue(oauthCredentialActor()) + + const result = await executeOAuthGetAuthLink( + { providerName: 'google-email', credentialId: CREDENTIAL_ID }, + context + ) + + expect(result.success).toBe(true) + const output = result.output as { oauth_url: string; message: string } + const url = new URL(output.oauth_url) + expect(url.searchParams.get('credentialId')).toBe(CREDENTIAL_ID) + expect(output.message).toContain('Reconnect') + expect(output.message).toContain(CREDENTIAL_ID) + }) + + it('fails with an agent-visible error for a nonexistent credential', async () => { + mockGetCredentialActorContext.mockResolvedValue({ + credential: null, + member: null, + hasWorkspaceAccess: false, + canWriteWorkspace: false, + isAdmin: false, + }) + + const result = await executeOAuthGetAuthLink( + { providerName: 'google-email', credentialId: 'cred-hallucinated' }, + context + ) + + expect(result.success).toBe(false) + expect(result.error).toContain('not found in this workspace') + }) + + it('fails when the credential belongs to another workspace', async () => { + mockGetCredentialActorContext.mockResolvedValue( + oauthCredentialActor({ credential: { workspaceId: 'ws-other' } }) + ) + + const result = await executeOAuthGetAuthLink( + { providerName: 'google-email', credentialId: CREDENTIAL_ID }, + context + ) + + expect(result.success).toBe(false) + expect(result.error).toContain('not found in this workspace') + }) + + it('fails when the credential is not an OAuth credential', async () => { + mockGetCredentialActorContext.mockResolvedValue( + oauthCredentialActor({ credential: { type: 'env_workspace' } }) + ) + + const result = await executeOAuthGetAuthLink( + { providerName: 'google-email', credentialId: CREDENTIAL_ID }, + context + ) + + expect(result.success).toBe(false) + expect(result.error).toContain('not an OAuth credential') + }) + + it('fails naming the actual provider when providerName does not match the credential', async () => { + mockGetCredentialActorContext.mockResolvedValue(oauthCredentialActor()) + + const result = await executeOAuthGetAuthLink( + { providerName: 'slack', credentialId: CREDENTIAL_ID }, + context + ) + + expect(result.success).toBe(false) + expect(result.error).toContain('google-email') + }) + + it('fails when the caller is not a credential admin', async () => { + mockGetCredentialActorContext.mockResolvedValue(oauthCredentialActor({ isAdmin: false })) + + const result = await executeOAuthGetAuthLink( + { providerName: 'google-email', credentialId: CREDENTIAL_ID }, + context + ) + + expect(result.success).toBe(false) + expect(result.error).toContain('Admin access') + }) + + it('rejects reconnect for Trello and directs the user to the integrations page', async () => { + const result = await executeOAuthGetAuthLink( + { providerName: 'trello', credentialId: CREDENTIAL_ID }, + context + ) + + expect(result.success).toBe(false) + expect(result.error).toContain('integrations page') + expect(mockGetCredentialActorContext).not.toHaveBeenCalled() + }) + + it('rejects reconnect for Shopify and directs the user to the integrations page', async () => { + const result = await executeOAuthGetAuthLink( + { providerName: 'shopify', credentialId: CREDENTIAL_ID }, + context + ) + + expect(result.success).toBe(false) + expect(result.error).toContain('integrations page') + expect(mockGetCredentialActorContext).not.toHaveBeenCalled() + }) + }) +}) diff --git a/apps/sim/lib/copilot/tools/handlers/oauth.ts b/apps/sim/lib/copilot/tools/handlers/oauth.ts index 3919b0ba557..ecaec8cfc7d 100644 --- a/apps/sim/lib/copilot/tools/handlers/oauth.ts +++ b/apps/sim/lib/copilot/tools/handlers/oauth.ts @@ -2,6 +2,7 @@ import { toError } from '@sim/utils/errors' import type { ExecutionContext, ToolCallResult } from '@/lib/copilot/request/types' import { ensureWorkspaceAccess } from '@/lib/copilot/tools/handlers/access' import { getBaseUrl } from '@/lib/core/utils/urls' +import { getCredentialActorContext } from '@/lib/credentials/access' import { getAllOAuthServices } from '@/lib/oauth/utils' export async function executeOAuthGetAuthLink( @@ -9,6 +10,8 @@ export async function executeOAuthGetAuthLink( context: ExecutionContext ): Promise { const providerName = String(rawParams.providerName || rawParams.provider_name || '') + const rawCredentialId = rawParams.credentialId || rawParams.credential_id + const credentialId = rawCredentialId ? String(rawCredentialId) : undefined const baseUrl = getBaseUrl() try { if (!context.workspaceId || !context.userId) { @@ -20,14 +23,18 @@ export async function executeOAuthGetAuthLink( context.workflowId, context.chatId, providerName, - baseUrl + baseUrl, + credentialId ? { credentialId, userId: context.userId } : undefined ) + const action = credentialId ? 'reconnect' : 'connect' return { success: true, output: { - message: `Authorization URL generated for ${result.serviceName}.`, + message: credentialId + ? `Reconnect authorization URL generated for ${result.serviceName}. Completing it re-authorizes credential ${credentialId} in place — its id stays the same.` + : `Authorization URL generated for ${result.serviceName}.`, oauth_url: result.url, - instructions: `Open this URL in your browser to connect ${result.serviceName}: ${result.url}`, + instructions: `Open this URL in your browser to ${action} ${result.serviceName}: ${result.url}`, provider: result.serviceName, providerId: result.providerId, }, @@ -72,13 +79,20 @@ export async function executeOAuthRequestAccess( * calls Better Auth, so the draft's TTL starts at click and the signed `state` * cookie is planted in the user's browser and the OAuth callback's state check * passes. + * + * When `reconnect` is set, the URL carries the existing credential id so the + * authorize endpoint creates a reconnect draft and the OAuth callback rebinds + * the credential in place instead of creating a new one. Validation happens + * here too (not just at click time) so a bad id fails in the tool result where + * the agent can see it, rather than as a silent browser redirect. */ async function generateOAuthLink( workspaceId: string | undefined, workflowId: string | undefined, chatId: string | undefined, providerName: string, - baseUrl: string + baseUrl: string, + reconnect?: { credentialId: string; userId: string } ): Promise<{ url: string; providerId: string; serviceName: string }> { if (!workspaceId) { throw new Error('workspaceId is required to generate an OAuth link') @@ -105,6 +119,37 @@ async function generateOAuthLink( } const { providerId, name: serviceName } = matched + + if (reconnect) { + if (providerId === 'trello' || providerId === 'shopify') { + throw new Error( + `Reconnect is not supported for ${serviceName} from chat. Ask the user to open the ` + + `integrations page and press Reconnect on the credential there.` + ) + } + const actor = await getCredentialActorContext(reconnect.credentialId, reconnect.userId) + if (!actor.credential || actor.credential.workspaceId !== workspaceId) { + throw new Error( + `Credential "${reconnect.credentialId}" was not found in this workspace. Read ` + + `environment/credentials.json for valid credential ids.` + ) + } + if (actor.credential.type !== 'oauth') { + throw new Error( + `Credential "${reconnect.credentialId}" is not an OAuth credential and cannot be reconnected.` + ) + } + if (actor.credential.providerId !== providerId) { + throw new Error( + `Credential "${reconnect.credentialId}" belongs to provider "${actor.credential.providerId}", ` + + `not "${providerId}". Pass the matching providerName.` + ) + } + if (!actor.isAdmin) { + throw new Error('Admin access on the credential is required to reconnect it.') + } + } + const callbackURL = workflowId && workspaceId ? `${baseUrl}/workspace/${workspaceId}/w/${workflowId}` @@ -138,6 +183,9 @@ async function generateOAuthLink( authorizeUrl.searchParams.set('providerId', providerId) authorizeUrl.searchParams.set('workspaceId', workspaceId) authorizeUrl.searchParams.set('callbackURL', callbackURL) + if (reconnect) { + authorizeUrl.searchParams.set('credentialId', reconnect.credentialId) + } return { url: authorizeUrl.toString(), providerId, serviceName } } diff --git a/apps/sim/lib/credentials/display-name.test.ts b/apps/sim/lib/credentials/display-name.test.ts new file mode 100644 index 00000000000..36d23dfc8b9 --- /dev/null +++ b/apps/sim/lib/credentials/display-name.test.ts @@ -0,0 +1,57 @@ +/** + * @vitest-environment node + */ +import { describe, expect, it } from 'vitest' +import { + DISPLAY_NAME_MAX_LENGTH, + defaultCredentialDisplayName, +} from '@/lib/credentials/display-name' + +const NONE: ReadonlySet = new Set() + +describe('defaultCredentialDisplayName', () => { + it("produces {Name}'s {Service} when the user name is known", () => { + expect(defaultCredentialDisplayName('Justin', 'Gmail', NONE)).toBe("Justin's Gmail") + }) + + it('trims surrounding whitespace from the user name', () => { + expect(defaultCredentialDisplayName(' Justin ', 'Gmail', NONE)).toBe("Justin's Gmail") + }) + + it.each([null, undefined, '', ' '])('falls back to My {Service} for user name %j', (name) => { + expect(defaultCredentialDisplayName(name, 'Gmail', NONE)).toBe('My Gmail') + }) + + it('appends " 2" when the base name is taken', () => { + const taken = new Set(["justin's gmail"]) + expect(defaultCredentialDisplayName('Justin', 'Gmail', taken)).toBe("Justin's Gmail 2") + }) + + it('skips taken numbered names and picks the next free slot', () => { + const taken = new Set(["justin's gmail", "justin's gmail 2", "justin's gmail 3"]) + expect(defaultCredentialDisplayName('Justin', 'Gmail', taken)).toBe("Justin's Gmail 4") + }) + + it('compares collisions case-insensitively', () => { + const taken = new Set(["justin's gmail"]) + expect(defaultCredentialDisplayName('JUSTIN', 'Gmail', taken)).toBe("JUSTIN's Gmail 2") + }) + + it('numbers the My {Service} fallback on collision too', () => { + const taken = new Set(['my gmail']) + expect(defaultCredentialDisplayName(null, 'Gmail', taken)).toBe('My Gmail 2') + }) + + it('truncates a long user name so name + suffix + disambiguator fit the max length', () => { + const longName = 'x'.repeat(400) + const result = defaultCredentialDisplayName(longName, 'Gmail', NONE) + + expect(result.endsWith("'s Gmail")).toBe(true) + expect(result.length).toBeLessThanOrEqual(DISPLAY_NAME_MAX_LENGTH) + + const taken = new Set([result.toLowerCase()]) + const numbered = defaultCredentialDisplayName(longName, 'Gmail', taken) + expect(numbered).toBe(`${result} 2`) + expect(numbered.length).toBeLessThanOrEqual(DISPLAY_NAME_MAX_LENGTH) + }) +}) diff --git a/apps/sim/lib/credentials/display-name.ts b/apps/sim/lib/credentials/display-name.ts new file mode 100644 index 00000000000..9b946f31e56 --- /dev/null +++ b/apps/sim/lib/credentials/display-name.ts @@ -0,0 +1,49 @@ +/** Server-enforced max for `WorkspaceCredential.displayName` — see `lib/api/contracts/credentials.ts`. */ +export const DISPLAY_NAME_MAX_LENGTH = 255 + +/** + * Reserved tail budget when truncating the username so the auto-numbering + * disambiguator (e.g. `" 9999"`) always fits within {@link DISPLAY_NAME_MAX_LENGTH}. + */ +const COLLISION_SUFFIX_RESERVATION = 5 + +/** Upper bound for the auto-numbering search — pathological if ever reached. */ +const MAX_COLLISION_INDEX = 10000 + +/** + * Default credential display name. Produces `"{Name}'s {Service}"` when the + * user's name is known, falling back to `"My {Service}"` otherwise. The + * username is truncated so the full string (including any auto-numbering + * disambiguator) stays within {@link DISPLAY_NAME_MAX_LENGTH}. + * + * When the base name collides with an existing credential in `takenNames`, + * `" 2"`, `" 3"`, ... are appended until an unused name is found. `takenNames` + * must contain lowercased names; comparison is case-insensitive to match the + * duplicate-detection in the connect modal. + */ +export function defaultCredentialDisplayName( + userName: string | null | undefined, + serviceName: string, + takenNames: ReadonlySet +): string { + const trimmed = userName?.trim() + let base: string + if (trimmed) { + const suffix = `'s ${serviceName}` + const nameBudget = Math.max( + 0, + DISPLAY_NAME_MAX_LENGTH - suffix.length - COLLISION_SUFFIX_RESERVATION + ) + const safeName = trimmed.length > nameBudget ? trimmed.slice(0, nameBudget) : trimmed + base = `${safeName}${suffix}` + } else { + base = `My ${serviceName}` + } + + if (!takenNames.has(base.toLowerCase())) return base + for (let n = 2; n < MAX_COLLISION_INDEX; n++) { + const candidate = `${base} ${n}` + if (!takenNames.has(candidate.toLowerCase())) return candidate + } + return base +} From dae3f074b114de711638be29607821c3329c8db1 Mon Sep 17 00:00:00 2001 From: Justin Blumencranz <96924014+j15z@users.noreply.github.com> Date: Tue, 7 Jul 2026 14:12:33 -0700 Subject: [PATCH 2/3] fix(credentials): address reconnect review findings --- .../api/auth/oauth2/authorize/route.test.ts | 34 ++++++++ .../app/api/auth/oauth2/authorize/route.ts | 77 +++++++++++++------ apps/sim/lib/copilot/tools/handlers/access.ts | 9 ++- .../lib/copilot/tools/handlers/oauth.test.ts | 23 +++++- apps/sim/lib/copilot/tools/handlers/oauth.ts | 15 +++- 5 files changed, 124 insertions(+), 34 deletions(-) diff --git a/apps/sim/app/api/auth/oauth2/authorize/route.test.ts b/apps/sim/app/api/auth/oauth2/authorize/route.test.ts index 64c9b12bdfc..894e543c3b2 100644 --- a/apps/sim/app/api/auth/oauth2/authorize/route.test.ts +++ b/apps/sim/app/api/auth/oauth2/authorize/route.test.ts @@ -58,6 +58,7 @@ function oauthCredentialActor(overrides: Record = {}) { workspaceId: WORKSPACE_ID, type: 'oauth', providerId: 'google-email', + displayName: 'Work Gmail', ...((overrides.credential as Record) ?? {}), }, member: null, @@ -189,6 +190,39 @@ describe('OAuth2 authorize route', () => { ) }) + it("uses the credential's actual display name for the reconnect draft (audit accuracy)", async () => { + mockGetCredentialActorContext.mockResolvedValue( + oauthCredentialActor({ credential: { displayName: 'Renamed By User' } }) + ) + + await GET( + authorizeRequest({ + providerId: 'google-email', + workspaceId: WORKSPACE_ID, + credentialId: CREDENTIAL_ID, + }) + ) + + expect(dbChainMockFns.values).toHaveBeenCalledWith( + expect.objectContaining({ displayName: 'Renamed By User' }) + ) + }) + + it('rejects reconnect for custom-flow providers (trello/shopify) and writes no draft', async () => { + for (const providerId of ['trello', 'shopify']) { + const response = await GET( + authorizeRequest({ providerId, workspaceId: WORKSPACE_ID, credentialId: CREDENTIAL_ID }) + ) + + expect(response.headers.get('location')).toBe( + `${BASE_URL}/workspace?error=credential_reconnect_unsupported` + ) + } + expect(mockGetCredentialActorContext).not.toHaveBeenCalled() + expect(dbChainMockFns.values).not.toHaveBeenCalled() + expect(mockOAuth2LinkAccount).not.toHaveBeenCalled() + }) + it('rejects when the caller is not a credential admin and writes no draft', async () => { mockGetCredentialActorContext.mockResolvedValue(oauthCredentialActor({ isAdmin: false })) diff --git a/apps/sim/app/api/auth/oauth2/authorize/route.ts b/apps/sim/app/api/auth/oauth2/authorize/route.ts index 6ca3b5cf54e..6763dac9143 100644 --- a/apps/sim/app/api/auth/oauth2/authorize/route.ts +++ b/apps/sim/app/api/auth/oauth2/authorize/route.ts @@ -33,37 +33,42 @@ async function createConnectDraft(params: { workspaceId: string providerId: string credentialId?: string + /** Reconnect only: the credential's actual name, so audit records stay accurate. */ + displayName?: string }): Promise { const { userId, workspaceId, providerId, credentialId } = params - const service = getAllOAuthServices().find((s) => s.providerId === providerId) - const serviceName = service?.name ?? providerId + let displayName = params.displayName + if (!displayName) { + const service = getAllOAuthServices().find((s) => s.providerId === providerId) + const serviceName = service?.name ?? providerId + + let userName: string | null = null + try { + const [row] = await db.select({ name: user.name }).from(user).where(eq(user.id, userId)) + userName = row?.name ?? null + } catch { + // Fall back to the "My {Service}" default + } - let userName: string | null = null - try { - const [row] = await db.select({ name: user.name }).from(user).where(eq(user.id, userId)) - userName = row?.name ?? null - } catch { - // Fall back to the "My {Service}" default - } + // Auto-number against existing workspace credentials so repeat connects for + // the same provider stay distinguishable — same behavior as the connect + // modal, which computes this client-side. Best effort: on failure the name + // simply skips deduplication. + let takenNames: ReadonlySet = new Set() + try { + const rows = await db + .select({ displayName: credential.displayName }) + .from(credential) + .where(and(eq(credential.workspaceId, workspaceId), eq(credential.type, 'oauth'))) + takenNames = new Set(rows.map((row) => row.displayName.toLowerCase())) + } catch { + // Best effort — proceed without collision numbering + } - // Auto-number against existing workspace credentials so repeat connects for - // the same provider stay distinguishable — same behavior as the connect - // modal, which computes this client-side. Best effort: on failure the name - // simply skips deduplication. - let takenNames: ReadonlySet = new Set() - try { - const rows = await db - .select({ displayName: credential.displayName }) - .from(credential) - .where(and(eq(credential.workspaceId, workspaceId), eq(credential.type, 'oauth'))) - takenNames = new Set(rows.map((row) => row.displayName.toLowerCase())) - } catch { - // Best effort — proceed without collision numbering + displayName = defaultCredentialDisplayName(userName, serviceName, takenNames) } - const displayName = defaultCredentialDisplayName(userName, serviceName, takenNames) - const now = new Date() const expiresAt = new Date(now.getTime() + DRAFT_TTL_MS) await db @@ -141,7 +146,22 @@ export const GET = withRouteHandler(async (request: NextRequest) => { return NextResponse.redirect(`${baseUrl}/workspace?error=workspace_access_denied`) } + let reconnectDisplayName: string | undefined if (credentialId) { + // Trello and Shopify authorize through their own custom flows that bypass + // this endpoint, so a reconnect draft written here would linger unconsumed + // and could later be picked up by their token-store callbacks, silently + // rebinding the credential. Mirror the copilot tool and reject reconnect. + if (providerId === 'trello' || providerId === 'shopify') { + logger.warn('Reconnect not supported for custom-flow provider', { + userId, + workspaceId, + providerId, + credentialId, + }) + return NextResponse.redirect(`${baseUrl}/workspace?error=credential_reconnect_unsupported`) + } + // Reconnect: the OAuth callback will rebind this credential to the fresh // account, so require the same credential-admin access as the draft POST // route — workspace write alone must not be enough to swap someone's tokens. @@ -172,12 +192,19 @@ export const GET = withRouteHandler(async (request: NextRequest) => { }) return NextResponse.redirect(`${baseUrl}/workspace?error=credential_provider_mismatch`) } + reconnectDisplayName = actor.credential.displayName } // Create the draft before initiating the link so it is guaranteed to exist // (and freshly clocked) when the OAuth callback's `account.create.after` // hook runs. If this throws, we never start the OAuth flow. - await createConnectDraft({ userId, workspaceId, providerId, credentialId }) + await createConnectDraft({ + userId, + workspaceId, + providerId, + credentialId, + displayName: reconnectDisplayName, + }) const linkResponse = await auth.api.oAuth2LinkAccount({ body: { providerId, callbackURL }, diff --git a/apps/sim/lib/copilot/tools/handlers/access.ts b/apps/sim/lib/copilot/tools/handlers/access.ts index 7391a829ead..2f5d592269e 100644 --- a/apps/sim/lib/copilot/tools/handlers/access.ts +++ b/apps/sim/lib/copilot/tools/handlers/access.ts @@ -1,6 +1,6 @@ import { authorizeWorkflowByWorkspacePermission } from '@sim/platform-authz/workflow' import type { getWorkflowById } from '@/lib/workflows/utils' -import { checkWorkspaceAccess } from '@/lib/workspaces/permissions/utils' +import { checkWorkspaceAccess, type WorkspaceAccess } from '@/lib/workspaces/permissions/utils' import { listAccessibleWorkspaceRowsForUser } from '@/lib/workspaces/utils' type WorkflowRecord = NonNullable>> @@ -47,22 +47,23 @@ export async function ensureWorkspaceAccess( workspaceId: string, userId: string, level: 'read' | 'write' | 'admin' = 'read' -): Promise { +): Promise { const access = await checkWorkspaceAccess(workspaceId, userId) if (!access.exists || !access.hasAccess) { throw new Error(`Workspace ${workspaceId} not found`) } - if (level === 'read') return + if (level === 'read') return access if (level === 'admin') { if (!access.canAdmin) { throw new Error('Admin access required for this workspace') } - return + return access } if (!access.canWrite) { throw new Error('Write or admin access required for this workspace') } + return access } diff --git a/apps/sim/lib/copilot/tools/handlers/oauth.test.ts b/apps/sim/lib/copilot/tools/handlers/oauth.test.ts index b9f564d18bd..30870b9d636 100644 --- a/apps/sim/lib/copilot/tools/handlers/oauth.test.ts +++ b/apps/sim/lib/copilot/tools/handlers/oauth.test.ts @@ -39,6 +39,14 @@ const context = { chatId: 'chat-1', } as unknown as ExecutionContext +const WORKSPACE_ACCESS = { + exists: true, + hasAccess: true, + canWrite: true, + canAdmin: false, + workspace: { id: WORKSPACE_ID }, +} + function oauthCredentialActor(overrides: Record = {}) { return { credential: { @@ -60,7 +68,7 @@ describe('executeOAuthGetAuthLink', () => { beforeEach(() => { vi.clearAllMocks() process.env.NEXT_PUBLIC_APP_URL = BASE_URL - mockEnsureWorkspaceAccess.mockResolvedValue(undefined) + mockEnsureWorkspaceAccess.mockResolvedValue(WORKSPACE_ACCESS) }) describe('connect (no credentialId)', () => { @@ -93,6 +101,19 @@ describe('executeOAuthGetAuthLink', () => { expect(output.message).toContain(CREDENTIAL_ID) }) + it('reuses the already-resolved workspace access for the credential lookup', async () => { + mockGetCredentialActorContext.mockResolvedValue(oauthCredentialActor()) + + await executeOAuthGetAuthLink( + { providerName: 'google-email', credentialId: CREDENTIAL_ID }, + context + ) + + expect(mockGetCredentialActorContext).toHaveBeenCalledWith(CREDENTIAL_ID, USER_ID, { + workspaceAccess: WORKSPACE_ACCESS, + }) + }) + it('fails with an agent-visible error for a nonexistent credential', async () => { mockGetCredentialActorContext.mockResolvedValue({ credential: null, diff --git a/apps/sim/lib/copilot/tools/handlers/oauth.ts b/apps/sim/lib/copilot/tools/handlers/oauth.ts index ecaec8cfc7d..f0a5f9dbbb0 100644 --- a/apps/sim/lib/copilot/tools/handlers/oauth.ts +++ b/apps/sim/lib/copilot/tools/handlers/oauth.ts @@ -4,6 +4,7 @@ import { ensureWorkspaceAccess } from '@/lib/copilot/tools/handlers/access' import { getBaseUrl } from '@/lib/core/utils/urls' import { getCredentialActorContext } from '@/lib/credentials/access' import { getAllOAuthServices } from '@/lib/oauth/utils' +import type { WorkspaceAccess } from '@/lib/workspaces/permissions/utils' export async function executeOAuthGetAuthLink( rawParams: Record, @@ -17,14 +18,18 @@ export async function executeOAuthGetAuthLink( if (!context.workspaceId || !context.userId) { throw new Error('workspaceId and userId are required to generate an OAuth link') } - await ensureWorkspaceAccess(context.workspaceId, context.userId, 'write') + const workspaceAccess = await ensureWorkspaceAccess( + context.workspaceId, + context.userId, + 'write' + ) const result = await generateOAuthLink( context.workspaceId, context.workflowId, context.chatId, providerName, baseUrl, - credentialId ? { credentialId, userId: context.userId } : undefined + credentialId ? { credentialId, userId: context.userId, workspaceAccess } : undefined ) const action = credentialId ? 'reconnect' : 'connect' return { @@ -92,7 +97,7 @@ async function generateOAuthLink( chatId: string | undefined, providerName: string, baseUrl: string, - reconnect?: { credentialId: string; userId: string } + reconnect?: { credentialId: string; userId: string; workspaceAccess: WorkspaceAccess } ): Promise<{ url: string; providerId: string; serviceName: string }> { if (!workspaceId) { throw new Error('workspaceId is required to generate an OAuth link') @@ -127,7 +132,9 @@ async function generateOAuthLink( `integrations page and press Reconnect on the credential there.` ) } - const actor = await getCredentialActorContext(reconnect.credentialId, reconnect.userId) + const actor = await getCredentialActorContext(reconnect.credentialId, reconnect.userId, { + workspaceAccess: reconnect.workspaceAccess, + }) if (!actor.credential || actor.credential.workspaceId !== workspaceId) { throw new Error( `Credential "${reconnect.credentialId}" was not found in this workspace. Read ` + From 1aad91b504b54c6c1f484eac137bd03b76a56e7d Mon Sep 17 00:00:00 2001 From: Justin Blumencranz <96924014+j15z@users.noreply.github.com> Date: Tue, 7 Jul 2026 15:29:08 -0700 Subject: [PATCH 3/3] improvement(credentials): log when connect draft name lookups degrade --- .../app/api/auth/oauth2/authorize/route.ts | 20 +++++++++++++++---- 1 file changed, 16 insertions(+), 4 deletions(-) diff --git a/apps/sim/app/api/auth/oauth2/authorize/route.ts b/apps/sim/app/api/auth/oauth2/authorize/route.ts index 6763dac9143..884762d3afd 100644 --- a/apps/sim/app/api/auth/oauth2/authorize/route.ts +++ b/apps/sim/app/api/auth/oauth2/authorize/route.ts @@ -47,8 +47,14 @@ async function createConnectDraft(params: { try { const [row] = await db.select({ name: user.name }).from(user).where(eq(user.id, userId)) userName = row?.name ?? null - } catch { - // Fall back to the "My {Service}" default + } catch (error) { + // Cosmetic only — fall back to the "My {Service}" default + logger.warn('User name lookup failed for connect draft display name', { + userId, + workspaceId, + providerId, + error, + }) } // Auto-number against existing workspace credentials so repeat connects for @@ -62,8 +68,14 @@ async function createConnectDraft(params: { .from(credential) .where(and(eq(credential.workspaceId, workspaceId), eq(credential.type, 'oauth'))) takenNames = new Set(rows.map((row) => row.displayName.toLowerCase())) - } catch { - // Best effort — proceed without collision numbering + } catch (error) { + // Cosmetic only — proceed without collision numbering + logger.warn('Credential name lookup failed for connect draft deduplication', { + userId, + workspaceId, + providerId, + error, + }) } displayName = defaultCredentialDisplayName(userName, serviceName, takenNames)