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 <noreply@anthropic.com> * 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 <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
2fb3667e8d
commit
3a7688c163
@@ -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')
|
||||
})
|
||||
})
|
||||
|
||||
@@ -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 {},
|
||||
|
||||
@@ -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,
|
||||
|
||||
Reference in New Issue
Block a user