From 3a7688c16372c36c23d6296fd2b87f9a5f2e6ce6 Mon Sep 17 00:00:00 2001 From: Jakob Wennberg Date: Wed, 12 Aug 2026 20:49:49 +0200 Subject: [PATCH] fix(enable-banking): only pin hidden, psu-applicable decoupled auth methods (Lunar dead end) (#1529) * fix(enable-banking): only pin hidden, psu-applicable decoupled auth methods PR #854 pinned the first DECOUPLED auth method for every bank to fix Handelsbanken corporate consents (its Mobile BankID is a hidden_method that Enable Banking only uses when requested explicitly). The blanket pin also hit banks whose decoupled method is visible and whose default flow already worked: Lunar users were asked for personnummer on the hosted page, told to approve in the app, and no approval ever arrived. Now a method is pinned only when pinning is necessary (hidden_method is true, so the method is unreachable by default) and applicable (psu_types missing/empty or containing the consent's psu_type). Otherwise undefined is returned and the ASPSP default runs, matching the stated intent of #854. The connect log now records the chosen method's approach, hidden_method and psu_types so per-bank behavior can be verified in prod after deploy. Co-Authored-By: Claude Fable 5 * test(enable-banking): bind pinned auth method to the outgoing /auth request Review follow-up on the auth-method pinning branch, two findings: 1. No test bound the selection result to the outgoing request: mutating the connect route to stop forwarding the pinned method's name into startAuthorization (the exact Handelsbanken-corporate regression this code exists to prevent) passed all 157 tests. Add route-level wiring tests asserting startAuthorization receives 'BANKID' in the auth_method argument position (index 5) on both the fresh-connect and reconnect call sites, plus the inverse: unpinned resolves to an undefined auth_method. 2. The auth_method_psu_types log field printed '(aspsp default)' when a method WAS pinned but carried no psu_types (the documented real Handelsbanken shape), contradicting auth_method='BANKID' on the same line. A pinned method without psu_types now logs '(all)'; '(aspsp default)' is reserved for the unpinned case. Both fixes are mutation-verified: reverting either makes the new tests fail (wiring mutation fails 2 tests, sentinel revert fails 1). Co-Authored-By: Claude Fable 5 --------- Co-authored-by: Jakob Wennberg <311770904+jakobwennberg-oss@users.noreply.github.com> Co-authored-by: Claude Fable 5 --- .../__tests__/connect-cleanup.test.ts | 143 ++++++++++++++++++ .../__tests__/disconnect-route.test.ts | 1 + .../__tests__/session-expired.test.ts | 100 +++++++++++- extensions/general/enable-banking/index.ts | 26 +++- .../general/enable-banking/lib/api-client.ts | 67 ++++++-- 5 files changed, 314 insertions(+), 23 deletions(-) diff --git a/extensions/general/enable-banking/__tests__/connect-cleanup.test.ts b/extensions/general/enable-banking/__tests__/connect-cleanup.test.ts index 1b0a8858..cc171d35 100644 --- a/extensions/general/enable-banking/__tests__/connect-cleanup.test.ts +++ b/extensions/general/enable-banking/__tests__/connect-cleanup.test.ts @@ -17,7 +17,10 @@ vi.mock('../lib/api-client', async (importOriginal) => { return { ...actual, startAuthorization: (...args: unknown[]) => mockStartAuthorization(...args), + // index.ts resolves the pinned auth method (with metadata for logging) + // through the details variant; both point at one mock for simplicity. getPreferredAuthMethod: (...args: unknown[]) => mockGetPreferredAuthMethod(...args), + getPreferredAuthMethodDetails: (...args: unknown[]) => mockGetPreferredAuthMethod(...args), } }) @@ -197,3 +200,143 @@ describe('POST /connect never-activated row cleanup', () => { expect(sweep._calls.some((c) => c.method === 'delete')).toBe(true) }) }) + +describe('POST /connect auth-method pinning wired into startAuthorization', () => { + beforeEach(() => { + vi.clearAllMocks() + vi.mocked(requireCapability).mockResolvedValue(null) + mockStartAuthorization.mockResolvedValue({ + url: 'https://bank.example/auth', + authorization_id: 'auth-1', + }) + }) + + // Fresh-connect from() sequence: recent-pending check, zombie sweep, insert. + // Explicit psu_type in the body skips the companies entity_type lookup. + function makeFreshConnectContext() { + let call = 0 + return makeContext(() => { + call++ + if (call === 1) return makeChain({ data: null }) + if (call === 2) return makeChain({ data: [] }) + return makeChain({ data: { id: 'new-conn' } }) + }) + } + + function makeHandelsbankenRequest() { + return new Request('https://test.local/api/extensions/ext/enable-banking/connect', { + method: 'POST', + headers: { 'Content-Type': 'application/json' }, + body: JSON.stringify({ aspsp_name: 'Handelsbanken', aspsp_country: 'SE', psu_type: 'business' }), + }) + } + + it('forwards the pinned method name into the auth_method argument (Handelsbanken corporate regression)', async () => { + // The documented real Handelsbanken shape: Mobile BankID is a hidden + // DECOUPLED method carrying NO psu_types restriction. + mockGetPreferredAuthMethod.mockResolvedValue({ + name: 'BANKID', + approach: 'DECOUPLED', + hidden_method: true, + title: 'Bank ID', + }) + + const ctx = makeFreshConnectContext() + const response = await connectRoute().handler(makeHandelsbankenRequest(), ctx) + expect(response.status).toBe(200) + + expect(mockGetPreferredAuthMethod).toHaveBeenCalledWith('Handelsbanken', 'SE', 'business') + + // startAuthorization(aspspName, aspspCountry, redirectUrl, state, psuType, + // authMethod): the pinned method's NAME must land in the auth_method + // position (index 5). This binds selection to the outgoing request: the + // selection tests alone cannot catch a route that resolves 'BANKID' and + // then starts a default REDIRECT authorization anyway. + expect(mockStartAuthorization).toHaveBeenCalledTimes(1) + const args = mockStartAuthorization.mock.calls[0] + expect(args[0]).toBe('Handelsbanken') + expect(args[1]).toBe('SE') + expect(args[4]).toBe('business') + expect(args[5]).toBe('BANKID') + + // A pinned method without psu_types applies to all PSU types: the log must + // say '(all)', never '(aspsp default)', which would contradict + // auth_method='BANKID' on the same line. + expect(ctx.log.info).toHaveBeenCalledWith( + '[enable-banking] Starting bank connection', + expect.objectContaining({ + auth_method: 'BANKID', + auth_method_psu_types: '(all)', + }), + ) + }) + + it('passes undefined auth_method when no method is pinned (ASPSP default flow)', async () => { + mockGetPreferredAuthMethod.mockResolvedValue(undefined) + + const ctx = makeFreshConnectContext() + const response = await connectRoute().handler(makeHandelsbankenRequest(), ctx) + expect(response.status).toBe(200) + + expect(mockStartAuthorization).toHaveBeenCalledTimes(1) + const args = mockStartAuthorization.mock.calls[0] + expect(args[5]).toBeUndefined() + + // Only the unpinned case logs the '(aspsp default)' sentinel. + expect(ctx.log.info).toHaveBeenCalledWith( + '[enable-banking] Starting bank connection', + expect.objectContaining({ + auth_method: '(aspsp default)', + auth_method_psu_types: '(aspsp default)', + }), + ) + }) + + it('forwards the pinned method name on the reconnect path too', async () => { + mockGetPreferredAuthMethod.mockResolvedValue({ + name: 'BANKID', + approach: 'DECOUPLED', + hidden_method: true, + title: 'Bank ID', + }) + + let call = 0 + const ctx = makeContext(() => { + call++ + if (call === 1) { + // The existing connection loaded up front: reconnect derives the bank + // identity and psu_type from this row. session_id null skips the + // sibling check + revoke. + return makeChain({ + data: { + id: 'conn-1', + bank_name: 'Handelsbanken', + provider: 'handelsbanken-se', + session_id: null, + psu_type: 'business', + }, + }) + } + // CSRF-state staging update and the authorization_id follow-up write. + return makeChain({ data: null }) + }) + + const req = new Request('https://test.local/api/extensions/ext/enable-banking/connect', { + method: 'POST', + headers: { 'Content-Type': 'application/json' }, + body: JSON.stringify({ connection_id: 'conn-1' }), + }) + + const response = await connectRoute().handler(req, ctx) + expect(response.status).toBe(200) + + // The reconnect branch calls startAuthorization from its own call site: + // the pinned name must reach the auth_method position there as well. + expect(mockStartAuthorization).toHaveBeenCalledTimes(1) + const args = mockStartAuthorization.mock.calls[0] + expect(args[0]).toBe('Handelsbanken') + expect(args[1]).toBe('SE') + expect(args[4]).toBe('business') + expect(args[5]).toBe('BANKID') + }) +}) diff --git a/extensions/general/enable-banking/__tests__/disconnect-route.test.ts b/extensions/general/enable-banking/__tests__/disconnect-route.test.ts index bbaca87b..77582da5 100644 --- a/extensions/general/enable-banking/__tests__/disconnect-route.test.ts +++ b/extensions/general/enable-banking/__tests__/disconnect-route.test.ts @@ -7,6 +7,7 @@ vi.mock('../lib/api-client', () => ({ startAuthorization: vi.fn(), getASPSPs: vi.fn(), getPreferredAuthMethod: vi.fn(), + getPreferredAuthMethodDetails: vi.fn(), deleteSession: vi.fn().mockResolvedValue(undefined), isSandboxMode: vi.fn(() => true), SessionExpiredError: class SessionExpiredError extends Error {}, diff --git a/extensions/general/enable-banking/__tests__/session-expired.test.ts b/extensions/general/enable-banking/__tests__/session-expired.test.ts index b447956a..bf18c1e1 100644 --- a/extensions/general/enable-banking/__tests__/session-expired.test.ts +++ b/extensions/general/enable-banking/__tests__/session-expired.test.ts @@ -23,6 +23,7 @@ import { SessionExpiredError, getAllTransactionsWithRaw, getPreferredAuthMethod, + getPreferredAuthMethodDetails, startAuthorization, } from '../lib/api-client' import { enableBankingExtension } from '../index' @@ -404,9 +405,11 @@ describe('auth_method selection (Handelsbanken Mobile BankID)', () => { ) } - it('picks the DECOUPLED (Mobile BankID) method when the bank exposes one', async () => { - // Handelsbanken's real shape: BankID is decoupled + hidden, Redirect is the - // visible default. We must pin BankID or corporate PSUs fail after BankID. + it('pins a hidden DECOUPLED method with no psu_types (Handelsbanken Mobile BankID)', async () => { + // Handelsbanken's real shape: BankID is decoupled + HIDDEN, Redirect is the + // visible default. Hidden methods are only used when requested explicitly, + // so we must pin BankID or corporate PSUs fail after approving in the app. + // No psu_types on the method = applies to every PSU type. stubAspsps([ { name: 'Handelsbanken', @@ -422,6 +425,73 @@ describe('auth_method selection (Handelsbanken Mobile BankID)', () => { expect(await getPreferredAuthMethod('Handelsbanken', 'SE', 'business')).toBe('BANKID') }) + it('does NOT pin a VISIBLE decoupled method (Lunar-class regression from PR #854)', async () => { + // When the decoupled method is not hidden it is already part of the bank's + // own default flow. Force-pinning it overrode Lunar's working default: + // the user typed personnummer, was told to approve in the Lunar app, and + // the approval request never arrived. undefined = let the ASPSP default run. + stubAspsps([ + { + name: 'Lunar', + country: 'SE', + auth_methods: [ + { name: 'DECOUPLED', approach: 'DECOUPLED', hidden_method: false, title: 'App' }, + ], + }, + ]) + + expect(await getPreferredAuthMethod('Lunar', 'SE', 'business')).toBeUndefined() + }) + + it('does NOT pin a hidden decoupled method scoped to a different psu_type', async () => { + stubAspsps([ + { + name: 'Testbank', + country: 'SE', + auth_methods: [ + { + name: 'BANKID', + approach: 'DECOUPLED', + hidden_method: true, + psu_types: ['personal'], + }, + ], + }, + ]) + + expect(await getPreferredAuthMethod('Testbank', 'SE', 'business')).toBeUndefined() + }) + + it('pins a hidden decoupled method whose psu_types matches (or is empty)', async () => { + stubAspsps([ + { + name: 'Testbank', + country: 'SE', + auth_methods: [ + { + name: 'BANKID_BUSINESS', + approach: 'DECOUPLED', + hidden_method: true, + psu_types: ['business'], + }, + ], + }, + ]) + expect(await getPreferredAuthMethod('Testbank', 'SE', 'business')).toBe('BANKID_BUSINESS') + + // An empty psu_types array is treated like a missing one: applies to all. + stubAspsps([ + { + name: 'Testbank', + country: 'SE', + auth_methods: [ + { name: 'BANKID_ALL', approach: 'DECOUPLED', hidden_method: true, psu_types: [] }, + ], + }, + ]) + expect(await getPreferredAuthMethod('Testbank', 'SE', 'personal')).toBe('BANKID_ALL') + }) + it('returns undefined (ASPSP default) when the bank has no decoupled method', async () => { stubAspsps([ { name: 'Nordea', country: 'SE', auth_methods: [{ name: 'REDIRECT', approach: 'REDIRECT' }] }, @@ -435,6 +505,30 @@ describe('auth_method selection (Handelsbanken Mobile BankID)', () => { expect(await getPreferredAuthMethod('Handelsbanken', 'SE', 'business')).toBeUndefined() }) + it('getPreferredAuthMethodDetails returns the full method so the connect log can record it', async () => { + stubAspsps([ + { + name: 'Handelsbanken', + country: 'SE', + auth_methods: [ + { + name: 'BANKID', + approach: 'DECOUPLED', + hidden_method: true, + psu_types: ['business'], + }, + ], + }, + ]) + + expect(await getPreferredAuthMethodDetails('Handelsbanken', 'SE', 'business')).toEqual({ + name: 'BANKID', + approach: 'DECOUPLED', + hidden_method: true, + psu_types: ['business'], + }) + }) + it('startAuthorization sends auth_method in the request body when provided', async () => { const fetchMock = vi.fn(async () => ({ ok: true, diff --git a/extensions/general/enable-banking/index.ts b/extensions/general/enable-banking/index.ts index 786da78e..0a906f80 100644 --- a/extensions/general/enable-banking/index.ts +++ b/extensions/general/enable-banking/index.ts @@ -3,7 +3,7 @@ import { NextResponse } from 'next/server' import { startAuthorization, getASPSPs, - getPreferredAuthMethod, + getPreferredAuthMethodDetails, deleteSession, isSandboxMode, SessionExpiredError, @@ -366,15 +366,18 @@ export const enableBankingExtension: Extension = { // Resolve the bank's preferred auth method. Handelsbanken (and some // other Swedish banks) expose Mobile BankID only as a hidden DECOUPLED - // method; without this, Enable Banking defaults to the REDIRECT method, - // which for Handelsbanken *corporate* PSUs cannot complete with Mobile - // BankID: the user approves in the app and then hits an error. Returns - // undefined for banks with no decoupled method, leaving them untouched. - const authMethod = await getPreferredAuthMethod( + // method; without pinning it, Enable Banking defaults to the REDIRECT + // method, which for Handelsbanken *corporate* PSUs cannot complete + // with Mobile BankID: the user approves in the app and then hits an + // error. Only hidden methods applicable to this psu_type are pinned; + // banks whose decoupled method is visible (e.g. Lunar) get undefined + // so their own working default flow runs untouched. + const preferredMethod = await getPreferredAuthMethodDetails( resolvedAspspName, resolvedAspspCountry, psuType ) + const authMethod = preferredMethod?.name log.info('[enable-banking] Starting bank connection', { user_id: user.id, @@ -382,6 +385,17 @@ export const enableBankingExtension: Extension = { country: resolvedAspspCountry, psu_type: psuType, auth_method: authMethod ?? '(aspsp default)', + // Chosen method's metadata, so prod logs can verify per-bank pinning + // behavior after deploy (hidden-only + psu_types selection). A + // pinned method with no psu_types (the documented Handelsbanken + // shape) applies to all PSU types and logs '(all)': the + // '(aspsp default)' sentinel is reserved for the unpinned case, + // where it would otherwise contradict auth_method on the same line. + auth_method_approach: preferredMethod?.approach ?? '(aspsp default)', + auth_method_hidden: preferredMethod?.hidden_method ?? '(aspsp default)', + auth_method_psu_types: preferredMethod + ? (preferredMethod.psu_types ?? '(all)') + : '(aspsp default)', reconnect: isReconnect, }) diff --git a/extensions/general/enable-banking/lib/api-client.ts b/extensions/general/enable-banking/lib/api-client.ts index 4fab4d37..c67b6698 100644 --- a/extensions/general/enable-banking/lib/api-client.ts +++ b/extensions/general/enable-banking/lib/api-client.ts @@ -374,28 +374,53 @@ export async function getASPSPs(country: string = 'SE', psuType?: 'personal' | ' } /** - * Resolve the auth_method we should request for a given bank, or undefined to - * let Enable Banking use the ASPSP's visible default. + * Pick the DECOUPLED auth method worth pinning explicitly, or undefined to let + * Enable Banking run the ASPSP's default flow. * - * Why: several Swedish ASPSPs (notably Handelsbanken) expose Mobile BankID only - * as a DECOUPLED method flagged hidden_method=true. When we send no auth_method, - * Enable Banking falls back to the visible REDIRECT method, which for - * Handelsbanken *corporate* PSUs does not support Mobile BankID, so the consent - * fails right after the user approves in the BankID app ("fel efter BankID"). - * Pinning the decoupled (Mobile BankID) method makes the flow work for both - * business and personal PSUs. We return undefined when the bank exposes no - * decoupled method or the lookup fails, so banks that already work are untouched. + * A method is only pinned when pinning is both NECESSARY and APPLICABLE: + * + * - hidden_method === true: a hidden method is never used unless requested + * explicitly via auth_method, so pinning is the only way to reach it. That + * is the Handelsbanken case: its Mobile BankID (DECOUPLED) is hidden, and + * without pinning it corporate PSUs fail right after approving in the + * BankID app ("fel efter BankID"). A VISIBLE decoupled method is already + * part of the bank's own default flow; force-pinning it overrides a working + * default. That regression (from PR #854) broke Lunar-class banks: the user + * typed their personnummer on Enable Banking's page, was told to approve in + * the bank's app, and no approval request ever arrived. + * - psu_types, when present and non-empty, must include the PSU type we are + * authorizing as: a method scoped to 'personal' must never be pinned for a + * 'business' consent (and vice versa). A missing or empty psu_types means + * the method applies to all PSU types. */ -export async function getPreferredAuthMethod( +export function selectPreferredAuthMethod( + authMethods: AuthMethod[] | undefined, + psuType: 'personal' | 'business' +): AuthMethod | undefined { + return authMethods?.find( + (m) => + m.approach === 'DECOUPLED' && + m.hidden_method === true && + (!m.psu_types || m.psu_types.length === 0 || m.psu_types.includes(psuType)) + ) +} + +/** + * Resolve the auth method to pin for a given bank, with full metadata so the + * caller can log approach/hidden_method/psu_types, or undefined to let Enable + * Banking use the ASPSP's default flow. Selection rules live in + * selectPreferredAuthMethod. Returns undefined on lookup failure so banks + * that already work are untouched. + */ +export async function getPreferredAuthMethodDetails( aspspName: string, country: string, psuType: 'personal' | 'business' -): Promise { +): Promise { try { const aspsps = await getASPSPs(country, psuType) const aspsp = aspsps.find((a) => a.name === aspspName) - const decoupled = aspsp?.auth_methods?.find((m) => m.approach === 'DECOUPLED') - return decoupled?.name + return selectPreferredAuthMethod(aspsp?.auth_methods, psuType) } catch (error) { console.error('[enable-banking] getPreferredAuthMethod failed; using ASPSP default', { aspspName, @@ -407,6 +432,20 @@ export async function getPreferredAuthMethod( } } +/** + * Name-only convenience wrapper around getPreferredAuthMethodDetails: the + * value to send as auth_method on POST /auth, or undefined for the ASPSP + * default. + */ +export async function getPreferredAuthMethod( + aspspName: string, + country: string, + psuType: 'personal' | 'business' +): Promise { + const method = await getPreferredAuthMethodDetails(aspspName, country, psuType) + return method?.name +} + /** * Get list of supported banks (legacy format for backward compatibility) */