fix(enable-banking): reconnect supersedes the old connection and stops duplicate imports (#1728)
* fix(enable-banking): supersede the old connection on bank reconnect and stop renewal duplicates
A renewal performed via the bank list ("Anslut ny bank") created a second
bank_connections row and left the old one parked in 'expired' forever: an
eternal "Åtgärd krävs" card, a red status chip, transactions stranded on the
dead row (so the picker's gap-fill probe read the renewal as a first
connect), and re-imported history for no-IBAN accounts whose provider uids
change on re-authorization.
- New migration: additive superseded_by uuid (FK, ON DELETE SET NULL) +
superseded_at + partial index on bank_connections. Status 'revoked' is
reused for superseded rows (no CHECK change); superseded_by disambiguates
a supersede from a user disconnect. File only: not applied anywhere yet.
- New lib/supersede.ts: after the OAuth callback finalizes, park same-bank
siblings matched by IBAN overlap (an ACTIVE sibling without overlap is
never touched; no-IBAN fallback only for dead siblings when neither side
has IBANs), revoke their EB session only when countLiveSiblings says
nobody shares it, re-point their transactions in id batches, demote
leftover cash_accounts claims (the mirror then promotes them by IBAN),
carry last_synced_at + initial_sync_* onto the survivor, and emit the new
bank_connection.superseded audit event.
- /connect fresh path: 409 { code: 'EXISTING_CONNECTION',
existing_connection_id } when a non-revoked same-bank row exists, unless
the body carries force_new: true (escape hatch for a second login at the
same bank). Runs after the zombie sweep; reconnect-in-place unaffected.
- Dedup scope stability: StoredAccount.dedup_scope pins the external_id
account scope at first ingest (normalized IBAN, else the uid of that
moment), is carried across in-place reconnects and supersedes by IBAN
match, and sync.ts uses dedup_scope ?? IBAN ?? uid (stamping legacy rows
lazily). The external_id FORMAT is untouched.
- AccountPickerDialog gap-fill probe also includes superseded connection
ids so the renewal default never races the transaction re-point.
- Sync toast (BankSyncNowButton) now also reports skipped duplicates
(sv+en strings) so a correctly deduped renewal does not look broken.
Tests: supersede unit tests, /connect 409 + force_new, callback supersede
wiring + dedup-scope carry, sync external_id stability across uid changes.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
* fix(enable-banking): scope the connect 409 to dead siblings and harden supersede ordering
- POST /connect only 409s when the same-bank sibling is expired/error/
pending_selection: an active row (a second legitimate login at the same
bank) never blocks a fresh connect; force_new bypass kept. The 409 text
now names the bank and points at Fornya samtycke.
- supersede parks the sibling row BEFORE revoking its EB session, and skips
the revoke entirely (logged) when the park update fails, so a failed park
can no longer leave a live-looking row with a dead session.
- callback keeps a survivor account's explicit dedup_scope instead of
letting a carried sibling scope clobber it; carried scopes only apply
when the survivor's scope was derived (IBAN/uid fallback).
- sync-now toast joins its two sentences with '. ' so the imported and
skipped-duplicates messages no longer run together.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
---------
Co-authored-by: Jakob Wennberg <311770904+jakobwennberg-oss@users.noreply.github.com>
Co-authored-by: Claude Fable 5 <noreply@anthropic.com>
This commit is contained in:
Jakob Wennberg
co-authored by
Claude Fable 5
Jakob Wennberg
Claude Fable 5
Jakob Wennberg
parent
1ded1af8fe
commit
3de5dee553
@@ -9,13 +9,21 @@ vi.mock('@/extensions/general/enable-banking/lib/api-client', () => ({
|
||||
}))
|
||||
|
||||
// Use hoisted to safely create mock objects referenced in vi.mock factories
|
||||
const { mockFrom, mockUpsertFromPsd2, mockAllocate } = vi.hoisted(() => {
|
||||
const { mockFrom, mockUpsertFromPsd2, mockAllocate, mockSupersede } = vi.hoisted(() => {
|
||||
const mockFrom = vi.fn()
|
||||
const mockUpsertFromPsd2 = vi.fn()
|
||||
const mockAllocate = vi.fn()
|
||||
return { mockFrom, mockUpsertFromPsd2, mockAllocate }
|
||||
const mockSupersede = vi.fn()
|
||||
return { mockFrom, mockUpsertFromPsd2, mockAllocate, mockSupersede }
|
||||
})
|
||||
|
||||
// The supersede pass has its own unit tests (extensions/general/enable-banking/
|
||||
// __tests__/supersede.test.ts); here it is mocked so these tests assert the
|
||||
// callback WIRES it correctly without scripting its internal queries.
|
||||
vi.mock('@/extensions/general/enable-banking/lib/supersede', () => ({
|
||||
supersedeSiblingConnections: (...args: unknown[]) => mockSupersede(...args),
|
||||
}))
|
||||
|
||||
vi.mock('@/lib/supabase/server', () => ({
|
||||
createServiceClient: vi.fn().mockResolvedValue({
|
||||
from: mockFrom,
|
||||
@@ -45,6 +53,13 @@ vi.mock('@/lib/cash-accounts/service', () => ({
|
||||
},
|
||||
defaultLedgerForCurrency: (currency: string) =>
|
||||
CURRENCY_DEFAULTS[currency.toUpperCase()] ?? '1930',
|
||||
// Real (trivial) implementation: the route normalizes IBANs when stamping
|
||||
// dedup scopes onto accounts_data.
|
||||
normalizeIban: (iban?: string | null) => {
|
||||
if (!iban) return null
|
||||
const normalized = iban.replace(/\s+/g, '').toUpperCase()
|
||||
return normalized || null
|
||||
},
|
||||
}))
|
||||
|
||||
vi.stubEnv('NEXT_PUBLIC_APP_URL', 'http://localhost:3000')
|
||||
@@ -74,6 +89,7 @@ describe('GET /api/extensions/enable-banking/callback', () => {
|
||||
beforeEach(() => {
|
||||
vi.clearAllMocks()
|
||||
mockUpsertFromPsd2.mockResolvedValue(undefined)
|
||||
mockSupersede.mockResolvedValue({ supersededIds: [], dedupScopeByIban: new Map() })
|
||||
// Allocator stand-in mirroring the real behavior: currency default first,
|
||||
// then the next free 1931–1959 slot (skipping other currency defaults).
|
||||
mockAllocate.mockImplementation(
|
||||
@@ -326,6 +342,248 @@ describe('GET /api/extensions/enable-banking/callback', () => {
|
||||
expect(mirrored.external_uid).toBe('acc-new')
|
||||
})
|
||||
|
||||
it('runs the same-bank supersede pass with the new session, accounts, and connection identity', async () => {
|
||||
// Fresh connect while an EXPIRED sibling to the same bank exists: the
|
||||
// supersede pass (unit-tested separately) must be handed everything it
|
||||
// needs to park the sibling and re-point its transactions.
|
||||
let callIndex = 0
|
||||
mockFrom.mockImplementation(() => {
|
||||
callIndex++
|
||||
if (callIndex === 1) {
|
||||
return mockChain({
|
||||
data: { id: 'conn-1', user_id: 'user-1', company_id: 'company-1', bank_name: 'TestBank', status: 'pending' },
|
||||
error: null,
|
||||
})
|
||||
}
|
||||
const chain: Record<string, unknown> = {}
|
||||
chain.update = vi.fn(() => chain)
|
||||
chain.eq = vi.fn().mockReturnValue(chain)
|
||||
chain.select = vi.fn().mockReturnValue(chain)
|
||||
chain.single = vi.fn().mockResolvedValue({
|
||||
data: { id: 'conn-1', bank_name: 'TestBank', company_id: 'company-1', user_id: 'user-1' },
|
||||
error: null,
|
||||
})
|
||||
chain.then = (resolve: (v: unknown) => void) => resolve({ data: null, error: null })
|
||||
return chain
|
||||
})
|
||||
|
||||
mockCreateSession.mockResolvedValue({
|
||||
session_id: 'sess-1',
|
||||
accounts: [
|
||||
{ uid: 'acc-1', account_id: { iban: 'SE1234' }, name: 'Företagskonto', currency: 'SEK' },
|
||||
],
|
||||
access: { valid_until: '2024-12-31T00:00:00Z' },
|
||||
aspsp: { name: 'TestBank', country: 'SE' },
|
||||
})
|
||||
|
||||
const response = await GET(makeRequest({ code: 'auth-code', state: 'valid-state' }))
|
||||
expect(response.status).toBe(200)
|
||||
// Reading the body drives the stream, which awaits the finalize work.
|
||||
await response.text()
|
||||
|
||||
expect(mockSupersede).toHaveBeenCalledTimes(1)
|
||||
const [, input] = mockSupersede.mock.calls[0] as [unknown, {
|
||||
companyId: string
|
||||
userId: string
|
||||
newConnectionId: string
|
||||
bankName: string | null
|
||||
newSessionId: string | null
|
||||
newAccounts: Array<{ uid: string; dedup_scope?: string }>
|
||||
}]
|
||||
expect(input.companyId).toBe('company-1')
|
||||
expect(input.userId).toBe('user-1')
|
||||
expect(input.newConnectionId).toBe('conn-1')
|
||||
expect(input.bankName).toBe('TestBank')
|
||||
expect(input.newSessionId).toBe('sess-1')
|
||||
expect(input.newAccounts).toHaveLength(1)
|
||||
// First-connect accounts get their dedup scope pinned to the normalized
|
||||
// IBAN (byte-identical to what lib/sync.ts derives).
|
||||
expect(input.newAccounts[0].dedup_scope).toBe('SE1234')
|
||||
})
|
||||
|
||||
it('applies dedup scopes carried from superseded siblings to accounts_data', async () => {
|
||||
mockSupersede.mockResolvedValue({
|
||||
supersededIds: ['old-1'],
|
||||
// The sibling's account was first ingested under its old provider uid.
|
||||
dedupScopeByIban: new Map([['SE1234', 'legacy-uid']]),
|
||||
})
|
||||
|
||||
const capturedUpdates: Record<string, unknown>[] = []
|
||||
let callIndex = 0
|
||||
mockFrom.mockImplementation(() => {
|
||||
callIndex++
|
||||
if (callIndex === 1) {
|
||||
return mockChain({
|
||||
data: { id: 'conn-1', user_id: 'user-1', company_id: 'company-1', bank_name: 'TestBank', status: 'pending' },
|
||||
error: null,
|
||||
})
|
||||
}
|
||||
const chain: Record<string, unknown> = {}
|
||||
chain.update = vi.fn((payload: Record<string, unknown>) => {
|
||||
capturedUpdates.push(payload)
|
||||
return chain
|
||||
})
|
||||
chain.eq = vi.fn().mockReturnValue(chain)
|
||||
chain.select = vi.fn().mockReturnValue(chain)
|
||||
chain.single = vi.fn().mockResolvedValue({
|
||||
data: { id: 'conn-1', bank_name: 'TestBank', company_id: 'company-1', user_id: 'user-1' },
|
||||
error: null,
|
||||
})
|
||||
chain.then = (resolve: (v: unknown) => void) => resolve({ data: null, error: null })
|
||||
return chain
|
||||
})
|
||||
|
||||
mockCreateSession.mockResolvedValue({
|
||||
session_id: 'sess-1',
|
||||
accounts: [
|
||||
{ uid: 'acc-1', account_id: { iban: 'SE1234' }, name: 'Företagskonto', currency: 'SEK' },
|
||||
],
|
||||
access: { valid_until: '2024-12-31T00:00:00Z' },
|
||||
aspsp: { name: 'TestBank', country: 'SE' },
|
||||
})
|
||||
|
||||
const response = await GET(makeRequest({ code: 'auth-code', state: 'valid-state' }))
|
||||
expect(response.status).toBe(200)
|
||||
await response.text()
|
||||
|
||||
// The follow-up accounts_data write persists the carried scope so the
|
||||
// renewal keeps minting the sibling's external_ids.
|
||||
const followUp = capturedUpdates[capturedUpdates.length - 1]
|
||||
const persisted = followUp.accounts_data as Array<{ uid: string; dedup_scope?: string }>
|
||||
expect(persisted.find((a) => a.uid === 'acc-1')?.dedup_scope).toBe('legacy-uid')
|
||||
})
|
||||
|
||||
it('carries the prior accounts_data dedup scope across an in-place reconnect', async () => {
|
||||
const capturedUpdates: Record<string, unknown>[] = []
|
||||
let callIndex = 0
|
||||
mockFrom.mockImplementation(() => {
|
||||
callIndex++
|
||||
if (callIndex === 1) {
|
||||
// The established row being reconnected in place: its account was
|
||||
// first ingested under the uid the ASPSP has since replaced.
|
||||
return mockChain({
|
||||
data: {
|
||||
id: 'conn-1',
|
||||
user_id: 'user-1',
|
||||
company_id: 'company-1',
|
||||
bank_name: 'TestBank',
|
||||
status: 'expired',
|
||||
session_id: null,
|
||||
accounts_data: [
|
||||
{ uid: 'uid-old', iban: 'SE1234', currency: 'SEK', dedup_scope: 'uid-first' },
|
||||
],
|
||||
},
|
||||
error: null,
|
||||
})
|
||||
}
|
||||
const chain: Record<string, unknown> = {}
|
||||
chain.update = vi.fn((payload: Record<string, unknown>) => {
|
||||
capturedUpdates.push(payload)
|
||||
return chain
|
||||
})
|
||||
chain.eq = vi.fn().mockReturnValue(chain)
|
||||
chain.select = vi.fn().mockReturnValue(chain)
|
||||
chain.single = vi.fn().mockResolvedValue({
|
||||
data: { id: 'conn-1', bank_name: 'TestBank', company_id: 'company-1', user_id: 'user-1' },
|
||||
error: null,
|
||||
})
|
||||
chain.then = (resolve: (v: unknown) => void) => resolve({ data: null, error: null })
|
||||
return chain
|
||||
})
|
||||
|
||||
mockCreateSession.mockResolvedValue({
|
||||
session_id: 'sess-2',
|
||||
accounts: [
|
||||
// Same IBAN, freshly minted uid.
|
||||
{ uid: 'uid-new', account_id: { iban: 'SE1234' }, name: 'Företagskonto', currency: 'SEK' },
|
||||
],
|
||||
access: { valid_until: '2024-12-31T00:00:00Z' },
|
||||
aspsp: { name: 'TestBank', country: 'SE' },
|
||||
})
|
||||
|
||||
const response = await GET(makeRequest({ code: 'auth-code', state: 'valid-state' }))
|
||||
expect(response.status).toBe(200)
|
||||
await response.text()
|
||||
|
||||
const connectionWrite = capturedUpdates[0]
|
||||
const accountsData = connectionWrite.accounts_data as Array<{
|
||||
uid: string
|
||||
dedup_scope?: string
|
||||
}>
|
||||
expect(accountsData).toHaveLength(1)
|
||||
expect(accountsData[0].uid).toBe('uid-new')
|
||||
// The scope pinned at first ingest survives the uid change.
|
||||
expect(accountsData[0].dedup_scope).toBe('uid-first')
|
||||
})
|
||||
|
||||
it('prefers the survivor account explicit dedup scope over a carried sibling scope', async () => {
|
||||
// A superseded sibling shares the IBAN but was ingested under a different
|
||||
// scope. The survivor's own row already pinned an explicit scope for this
|
||||
// account: that is what its external_ids were minted under, so the
|
||||
// sibling's scope must NOT clobber it.
|
||||
mockSupersede.mockResolvedValue({
|
||||
supersededIds: ['old-1'],
|
||||
dedupScopeByIban: new Map([['SE1234', 'sibling-scope']]),
|
||||
})
|
||||
|
||||
const capturedUpdates: Record<string, unknown>[] = []
|
||||
let callIndex = 0
|
||||
mockFrom.mockImplementation(() => {
|
||||
callIndex++
|
||||
if (callIndex === 1) {
|
||||
return mockChain({
|
||||
data: {
|
||||
id: 'conn-1',
|
||||
user_id: 'user-1',
|
||||
company_id: 'company-1',
|
||||
bank_name: 'TestBank',
|
||||
status: 'expired',
|
||||
session_id: null,
|
||||
accounts_data: [
|
||||
{ uid: 'uid-old', iban: 'SE1234', currency: 'SEK', dedup_scope: 'survivor-scope' },
|
||||
],
|
||||
},
|
||||
error: null,
|
||||
})
|
||||
}
|
||||
const chain: Record<string, unknown> = {}
|
||||
chain.update = vi.fn((payload: Record<string, unknown>) => {
|
||||
capturedUpdates.push(payload)
|
||||
return chain
|
||||
})
|
||||
chain.eq = vi.fn().mockReturnValue(chain)
|
||||
chain.select = vi.fn().mockReturnValue(chain)
|
||||
chain.single = vi.fn().mockResolvedValue({
|
||||
data: { id: 'conn-1', bank_name: 'TestBank', company_id: 'company-1', user_id: 'user-1' },
|
||||
error: null,
|
||||
})
|
||||
chain.then = (resolve: (v: unknown) => void) => resolve({ data: null, error: null })
|
||||
return chain
|
||||
})
|
||||
|
||||
mockCreateSession.mockResolvedValue({
|
||||
session_id: 'sess-2',
|
||||
accounts: [
|
||||
{ uid: 'uid-new', account_id: { iban: 'SE1234' }, name: 'Företagskonto', currency: 'SEK' },
|
||||
],
|
||||
access: { valid_until: '2024-12-31T00:00:00Z' },
|
||||
aspsp: { name: 'TestBank', country: 'SE' },
|
||||
})
|
||||
|
||||
const response = await GET(makeRequest({ code: 'auth-code', state: 'valid-state' }))
|
||||
expect(response.status).toBe(200)
|
||||
await response.text()
|
||||
|
||||
// Every accounts_data write keeps the survivor's explicit scope: neither
|
||||
// the connection write nor any carried-scope follow-up flips it.
|
||||
const accountsWrites = capturedUpdates.filter((u) => Array.isArray(u.accounts_data))
|
||||
expect(accountsWrites.length).toBeGreaterThan(0)
|
||||
for (const write of accountsWrites) {
|
||||
const accountsData = write.accounts_data as Array<{ uid: string; dedup_scope?: string }>
|
||||
expect(accountsData.find((a) => a.uid === 'uid-new')?.dedup_scope).toBe('survivor-scope')
|
||||
}
|
||||
})
|
||||
|
||||
it('deletes the fresh row and streams an error redirect when the session exchange fails', async () => {
|
||||
const deleteCalls: unknown[] = []
|
||||
const updateCalls: unknown[] = []
|
||||
|
||||
Reference in New Issue
Block a user