Log why a provider refused a sign-in and keep tokens through a failed reconnect (#571)

* Log why a provider refused a sign-in and keep tokens through a failed reconnect

The OAuth callback now writes the provider's reason (such as invalid_client) to the server log and the connection's activity log. Starting a reconnect no longer wipes the tokens of a working connection, a new consent no longer inherits the old refresh token, and setting lists are not fetched from the provider for a connection that never got tokens.

* Keep a pending PKCE verifier through a token refresh

With reconnect keeping the live tokens, a sync can refresh them between start and callback. The refresh used to clear the verifier, so a PKCE reconnect then failed. It is now spent only by the code exchange.

src/__tests__/features/integrations/oauth-failed-connect.test.ts

@@ -0,0 +1,180 @@
+import { NextRequest } from 'next/server'
+import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest'
+
+/**
+ * A sign-in the vendor refuses, or one the workshop walks away from, must
+ * not break anything: a connection that worked keeps its tokens, the
+ * vendor's reason is kept where the workshop and the server log can see it,
+ * and the settings page does not ask the vendor for lists with no token.
+ */
+
+vi.mock('@/lib/with-auth', () => ({
+  withAuth: (fn: (ctx: { organizationId: string; userId: string }) => unknown) =>
+    fn({ organizationId: 'org-1', userId: 'user-1' }),
+}))
+vi.mock('next/cache', () => ({ revalidatePath: vi.fn() }))
+vi.mock('@/lib/get-auth-context', () => ({
+  getAuthContext: vi.fn(async () => ({
+    userId: 'user-1',
+    organizationId: 'org-1',
+    isAdmin: true,
+  })),
+}))
+vi.mock('@/lib/audit', () => ({ logAudit: vi.fn() }))
+vi.mock('@/lib/features', async (importOriginal) => ({
+  ...(await importOriginal<typeof import('@/lib/features')>()),
+  // Every plan flag on: the gate is not what these tests are about.
+  getFeatures: vi.fn().mockResolvedValue(new Proxy({}, { get: () => true })),
+}))
+vi.mock('@/features/integrations/Lib/vault', () => ({
+  openCredentials: vi.fn((sealed: string | null | undefined) =>
+    sealed ? (JSON.parse(sealed) as Record<string, unknown>) : {}
+  ),
+  sealCredentials: vi.fn((plain: Record<string, unknown>) => JSON.stringify(plain)),
+}))
+vi.mock('@/features/integrations/Lib/connections', async (importOriginal) => ({
+  ...(await importOriginal<typeof import('@/features/integrations/Lib/connections')>()),
+  appUrl: () => 'https://app.example.com',
+  loadConnection: vi.fn(),
+  setConnectionStatus: vi.fn(),
+  writeLog: vi.fn(),
+}))
+vi.mock('@/lib/db', () => ({
+  db: {
+    integrationConnection: {
+      findUnique: vi.fn(),
+      upsert: vi.fn(),
+      update: vi.fn(),
+    },
+  },
+}))
+
+import { db } from '@/lib/db'
+import { loadConnection, writeLog } from '@/features/integrations/Lib/connections'
+import { getIntegrationRemoteOptions } from '@/features/integrations/Actions/integrationActions'
+import { GET as start } from '@/app/api/integrations/[connector]/oauth/start/route'
+import { GET as callback } from '@/app/api/integrations/[connector]/oauth/callback/route'
+
+const findUnique = vi.mocked(db.integrationConnection.findUnique)
+const upsert = vi.mocked(db.integrationConnection.upsert)
+const update = vi.mocked(db.integrationConnection.update)
+
+const LIVE_TOKENS = { accessToken: 'live-access', refreshToken: 'live-refresh', expiresAt: 1 }
+
+function xeroRow(status: string, credentials: Record<string, unknown>) {
+  return {
+    id: 'conn-1',
+    organizationId: 'org-1',
+    connectorId: 'xero',
+    status,
+    oauthState: 'state-1',
+    credentials: JSON.stringify(credentials),
+    settings: {},
+    state: {},
+  }
+}
+
+const params = { params: Promise.resolve({ connector: 'xero' }) }
+const env = { ...process.env }
+
+beforeEach(() => {
+  vi.clearAllMocks()
+  process.env.XERO_INTEGRATION_CLIENT_ID = 'platform-id'
+  process.env.XERO_INTEGRATION_CLIENT_SECRET = 'platform-secret'
+  upsert.mockResolvedValue({ id: 'conn-1' } as never)
+  update.mockResolvedValue({} as never)
+})
+afterEach(() => {
+  process.env = { ...env }
+  vi.restoreAllMocks()
+})
+
+describe('reconnecting an OAuth connector', () => {
+  it('keeps the tokens of a working connection until the new consent replaces them', async () => {
+    findUnique.mockResolvedValue(xeroRow('active', LIVE_TOKENS) as never)
+
+    const res = await start(
+      new NextRequest('https://app.example.com/api/integrations/xero/oauth/start'),
+      params
+    )
+
+    expect(res.headers.get('location')).toContain('login.xero.com')
+    const saved = JSON.parse(String(upsert.mock.calls[0][0].update.credentials))
+    expect(saved).toMatchObject({ accessToken: 'live-access', refreshToken: 'live-refresh' })
+    expect(upsert.mock.calls[0][0].update.status).toBe('active')
+  })
+})
+
+describe('a sign-in the vendor refuses', () => {
+  it('keeps the reason for the workshop and the server log, and leaves the tokens alone', async () => {
+    findUnique.mockResolvedValue(xeroRow('active', LIVE_TOKENS) as never)
+    vi.spyOn(globalThis, 'fetch').mockResolvedValueOnce(
+      new Response(JSON.stringify({ error: 'invalid_client' }), { status: 400 })
+    )
+    const serverLog = vi.spyOn(console, 'error').mockImplementation(() => undefined)
+
+    const res = await callback(
+      new NextRequest(
+        'https://app.example.com/api/integrations/xero/oauth/callback?code=c1&state=state-1'
+      ),
+      params
+    )
+
+    expect(res.headers.get('location')).toBe(
+      'https://app.example.com/settings/integrations/xero?error=exchange'
+    )
+    expect(update).toHaveBeenCalledTimes(1)
+    const data = update.mock.calls[0][0].data
+    expect(data).toMatchObject({ lastError: 'invalid_client', status: 'active', oauthState: null })
+    expect(data).not.toHaveProperty('credentials')
+    expect(writeLog).toHaveBeenCalledWith('conn-1', 'error', 'The provider refused the sign-in', {
+      error: 'invalid_client',
+    })
+    expect(serverLog).toHaveBeenCalledWith(expect.stringContaining('invalid_client'))
+  })
+
+  it('marks a connection that never worked as errored', async () => {
+    findUnique.mockResolvedValue(xeroRow('pending', {}) as never)
+    vi.spyOn(globalThis, 'fetch').mockResolvedValueOnce(
+      new Response(JSON.stringify({ error: 'invalid_client' }), { status: 400 })
+    )
+    vi.spyOn(console, 'error').mockImplementation(() => undefined)
+
+    await callback(
+      new NextRequest(
+        'https://app.example.com/api/integrations/xero/oauth/callback?code=c1&state=state-1'
+      ),
+      params
+    )
+
+    expect(update.mock.calls[0][0].data).toMatchObject({
+      status: 'error',
+      lastError: 'invalid_client',
+    })
+  })
+})
+
+describe('setting lists on a connection without tokens', () => {
+  it('stays empty without asking the vendor', async () => {
+    findUnique.mockResolvedValue(xeroRow('error', {}) as never)
+
+    const res = await getIntegrationRemoteOptions('xero', 'organisations')
+
+    expect(res).toEqual([])
+    expect(loadConnection).not.toHaveBeenCalled()
+  })
+
+  it('asks the vendor once the connection has tokens', async () => {
+    findUnique.mockResolvedValue(xeroRow('active', LIVE_TOKENS) as never)
+    const organisations = vi.fn().mockResolvedValue([{ value: 't1', label: 'Shop Ltd' }])
+    vi.mocked(loadConnection).mockResolvedValue({
+      ctx: {},
+      server: { remoteOptions: { organisations } },
+    } as never)
+
+    const res = await getIntegrationRemoteOptions('xero', 'organisations')
+
+    expect(res).toEqual([{ value: 't1', label: 'Shop Ltd' }])
+    expect(organisations).toHaveBeenCalled()
+  })
+})

src/__tests__/features/integrations/oauth.test.ts

@@ -109,6 +109,59 @@ describe('integration oauth', () => {
     expect(refreshed.refreshToken).toBe('r1')
   })
 
+  it('does not carry the refresh token of the consent a new one replaces', async () => {
+    vi.spyOn(globalThis, 'fetch').mockResolvedValueOnce(
+      new Response(JSON.stringify({ access_token: 'new', expires_in: 3600 }), { status: 200 })
+    )
+    const creds = await exchangeCode({
+      spec,
+      client: { clientId: 'ws', clientSecret: 's', ownership: 'tenant' },
+      code: 'code',
+      redirectUri: 'https://x/cb',
+      previous: {
+        accessToken: 'old',
+        refreshToken: 'old-refresh',
+        clientId: 'ws',
+        clientSecret: 's',
+      },
+    })
+    expect(creds.accessToken).toBe('new')
+    expect(creds.refreshToken).toBeUndefined()
+    expect(creds).toMatchObject({ clientId: 'ws', clientSecret: 's' })
+  })
+
+  it('keeps a pending reconnect verifier through a token refresh, and spends it on the exchange', async () => {
+    const fetchMock = vi.spyOn(globalThis, 'fetch')
+    const client = { clientId: 'cid', clientSecret: 'sec', ownership: 'platform' as const }
+    // A sync refreshes the live tokens after Reconnect was pressed.
+    fetchMock.mockResolvedValueOnce(
+      new Response(JSON.stringify({ access_token: 'a2', expires_in: 3600 }), { status: 200 })
+    )
+    const refreshed = await refreshToken({
+      spec,
+      client,
+      credentials: { accessToken: 'a1', refreshToken: 'r1', expiresAt: 0, codeVerifier: 'v' },
+    })
+    expect(refreshed.codeVerifier).toBe('v')
+
+    // The callback then finishes the reconnect with that verifier.
+    fetchMock.mockResolvedValueOnce(
+      new Response(JSON.stringify({ access_token: 'a3', refresh_token: 'r3' }), { status: 200 })
+    )
+    const reconnected = await exchangeCode({
+      spec,
+      client,
+      code: 'code',
+      redirectUri: 'https://x/cb',
+      codeVerifier: refreshed.codeVerifier,
+      previous: refreshed,
+    })
+    const sent = new URLSearchParams(String(fetchMock.mock.calls[1][1]?.body))
+    expect(sent.get('code_verifier')).toBe('v')
+    expect(reconnected.codeVerifier).toBeUndefined()
+    expect(reconnected.refreshToken).toBe('r3')
+  })
+
   it('surfaces the vendor error message on a failed exchange', async () => {
     vi.spyOn(globalThis, 'fetch').mockResolvedValueOnce(
       new Response(JSON.stringify({ error: 'invalid_grant', error_description: 'Bad code' }), {

src/app/api/integrations/[connector]/oauth/callback/route.ts

@@ -70,14 +70,20 @@ export async function GET(
       previous,
     })
   } catch (err) {
+    // The vendor's own words, such as invalid_client for a wrong secret, are
+    // the only clue to what is misconfigured. The browser only gets a code,
+    // so they go to the server log and the connection's activity log as well.
+    const reason = err instanceof Error ? err.message : 'Token exchange failed'
+    console.error(`[integrations] ${connector} refused the sign-in: ${reason}`)
     await db.integrationConnection.update({
       where: { id: connection.id },
       data: {
         oauthState: null,
         status: connection.status === 'active' ? 'active' : 'error',
-        lastError: err instanceof Error ? err.message : 'Token exchange failed',
+        lastError: reason,
       },
     })
+    await writeLog(connection.id, 'error', 'The provider refused the sign-in', { error: reason })
     return fail('exchange')
   }
 

src/app/api/integrations/[connector]/oauth/start/route.ts

@@ -52,7 +52,9 @@ export async function GET(
 
   const state = newState()
   const codeVerifier = spec.pkce ? newCodeVerifier() : undefined
-  // Keep a tenant-owned client id and secret; drop stale tokens.
+  // Keep a tenant-owned client id and secret. The tokens stay until the new
+  // consent replaces them in the callback, so a reconnect that is cancelled
+  // or refused leaves a working connection working.
   const credentials: Record<string, unknown> = {
     ...(client.ownership === 'tenant' && {
       clientId: client.clientId,
@@ -76,12 +78,7 @@ export async function GET(
     update: {
       status: existing?.status === 'active' ? 'active' : 'pending',
       oauthState: state,
-      credentials: sealCredentials({
-        ...previous,
-        ...credentials,
-        accessToken: undefined,
-        refreshToken: undefined,
-      }),
+      credentials: sealCredentials({ ...previous, ...credentials }),
     },
   })
 

src/features/integrations/Actions/integrationActions.ts

@@ -645,9 +645,17 @@ export async function getIntegrationRemoteOptions(connectorId: string, source: s
       await requireConnectorPlan(organizationId, connectorId)
       const row = await db.integrationConnection.findUnique({
         where: { organizationId_connectorId: { organizationId, connectorId } },
-        select: { id: true, status: true },
+        select: { id: true, status: true, credentials: true },
       })
       if (!row || row.status === 'pending' || row.status === 'disconnected') return []
+      // An OAuth sign-in that never finished, such as one the vendor refused,
+      // leaves no tokens. Asking the vendor would only fail; the list stays
+      // empty until the workshop connects, and the page says why.
+      const manifest = getManifest(connectorId)
+      if (manifest && oauthSpec(manifest)) {
+        const tokens = openCredentials(row.credentials)
+        if (!tokens.accessToken && !tokens.refreshToken) return []
+      }
       const { ctx, server } = await loadConnection(row.id)
       const provider = server.remoteOptions?.[source]
       if (!provider) return []

src/features/integrations/Lib/oauth.ts

@@ -143,7 +143,6 @@ function toCredentials(body: TokenResponse, previous: OAuthCredentials): OAuthCr
       Number.isFinite(expiresIn) && expiresIn > 0 ? Date.now() + expiresIn * 1000 : undefined,
     tokenType: body.token_type ?? previous.tokenType,
     scope: body.scope ?? previous.scope,
-    codeVerifier: undefined,
   }
 }
 
@@ -164,7 +163,17 @@ export async function exchangeCode(input: {
     ...(input.codeVerifier && { code_verifier: input.codeVerifier }),
     ...(input.spec.stateOnExchange && input.state && { state: input.state }),
   })
-  return toCredentials(body, input.previous ?? { accessToken: '' })
+  // A new consent starts its own tokens: a refresh token kept from the one
+  // it replaces may belong to another app or account, so it is not carried.
+  // The PKCE verifier is spent here and only here; a refresh while a
+  // reconnect is pending keeps it, or the callback could not finish.
+  return toCredentials(body, {
+    ...(input.previous ?? {}),
+    accessToken: '',
+    refreshToken: undefined,
+    expiresAt: undefined,
+    codeVerifier: undefined,
+  })
 }
 
 export async function refreshToken(input: {