fix: repair the Portal's UK and Berlin Group consent flows, and show the PSU what they are granting - #5
Merged
Merged
Conversation
The payment SCA endpoints called /obp/v1.3/berlin-group/... but OBP-API registers these routes at /berlin-group/v1.3/... with no /obp/ prefix, so both the create-authorisation POST and OTP-submit PUT calls were hitting a non-existent path.
…t approval
The SCA confirmation page submitted the OTP to
/obp/v3.1.0/banks/{bank}/consents/{id}/challenge, which only
transitions consents in status INITIATED -- Berlin Group consents are
created in status received, so this endpoint could never succeed
regardless of OTP correctness.
Call the real BG-native pair instead: start the authorisation in
load() (POST .../consents/{id}/authorisations) and submit the answer
in the form action (PUT .../consents/{id}/authorisations/{authorisationId}),
threading the authorisationId through a hidden form field.
The UK consent approval page only showed Confirm/Deny with no account selection, and the SCA page's POST .../authorise call omitted account_ids entirely. OBP-API's authorise endpoint now requires account_ids and rejects any account the PSU doesn't hold, so this call was failing with 400. Fetch the PSU's own accounts at the consent's bank (GET /my/accounts, filtered by bank_id) and let them check which ones the requested permissions apply to. Carry the selection through the SCA redirect as a comma-joined account_ids query param, then a hidden form field, and include it in the final POST .../authorise body.
SvelteKit's "?/actionName" shorthand replaces the page's entire query string rather than appending to it. load() requires CONSENT_ID/bank_id from the URL, so submitting a bare "?/confirm" or "?/deny" stripped them -- on any non-redirecting action response (e.g. a validation failure) load() re-ran against the stripped URL and threw its own "Missing required parameter" error, masking the action's real message. Preserve the existing query string per SvelteKit's documented convention for named actions on a page with search params.
…ookie
When exactly one provider is available, +page.server.ts's load() normally
auto-redirects straight to /login/{provider}, which mints a fresh
obp_oauth_state cookie. This page only renders that single-provider case
when an error/success message is suppressing the redirect (e.g. a failed
login retry) -- and it still ran a 60-second invalidateAll() interval in
that state. If invalidateAll() raced a real click on the provider link,
the interval's response could overwrite the state cookie the click's own
request had just set, causing an intermittent "Security validation
failed" state mismatch on the callback.
Skip the refresh interval whenever only one provider is available, and
disable preload on the provider link so hovering/tapping it can't mint an
extra state cookie ahead of the real navigation.
/health required every monitored dependency to be healthy and answered 503 otherwise. That is readiness logic in a liveness endpoint: an orchestrator polling it would restart a perfectly functional app because Opey or a secondary OAuth2 provider was down, even though pages keep being served either way, so the restart only widens an unrelated outage. It also flattened the nuance /status already encodes. summarizeHealth groups the OAuth2 providers, so one dead provider alongside a working one is 'partial'; the `every()` here turned any single unhealthy check into a hard 503 — including 'OAuth2: google', which is reported unhealthy simply because it was never configured. Locally that left /health returning 503 while the app was entirely usable. And "no checks registered" counted as healthy, the opposite of the rule summarizeHealth states for that case. Both apps carried the same copy; both now answer 200 unconditionally, matching what OBP-API and Hola serve at /health. Dependency health remains available in full detail at /status, which is unchanged. No probe was relying on the old behaviour: the portal and api-manager deployments in obp-local-k8s use `path: /` for readiness.
…P mandate The VRP approval screen rendered a validity window and a bank, and nothing else. The PSU was asked to authorise a standing payment authority without being shown the account it would be paid from, who could be paid, or any of the limits it would be good for. Not a missing feature so much as an unhandled shape: a VRP consent-request payload carries from_account, a to_account naming the counterparty, and a limit block, where an account-access one carries account_access and entitlements. None of the existing blocks matched, so all of it fell through silently. The page now shows the debtor account, the payee, and the four limits that bound the mandate -- per payment, per month, per year and in total, each with its transaction count where the payload sets one. counterparty_name is the TPP's own label and can be blank, so the payee row falls back to the account being paid rather than rendering an empty field on the one screen whose purpose is to say who gets the money. (Hola was in fact dropping that name; fixed separately in OBP-Hola.) Verified in a browser against a running stack: the screen shows the from-account, the payee account, and GBP 100 per payment / GBP 250 at most 1 payment per month / GBP 1200 at most 12 per year / GBP 12000 at most 120 in total.
…message
Two error shapes reach this client. OBP's own endpoints answer {code, message}; Berlin
Group answers {tppMessages: [{code, text}]}, which is what NextGenPSD2 specifies. Only the
first was recognised, so every Berlin Group failure fell past it into
`new OBPErrorBase("Error posting OBP data to ...")` -- an OBPErrorBase, not an
OBPRequestError, which the pages then fail to match and replace with their own generic
text.
The effect: an account-ownership refusal reads exactly like a transport fault. A PSU who
lodged a consent naming an account they do not hold saw only "Failed to start consent
authorisation." with nothing to act on, while the API had answered
OBP-35037: One or more of the specified account_ids is not held by the current user.
A consent may only be authorised for accounts the authorising user holds.
The SCA page compounded it: with no authorisation to answer, pressing Verify said
"Missing authorisation id. Please reload the page." Reloading re-runs the same failing
call, so that sends the PSU round the loop with the real reason still hidden. It now says
the authorisation could not be started and points at the reason above it.
All five verbs share one extractor rather than repeating the shape check.
Verified in a browser: the same consent that produced the generic message now shows the
OBP-35037 text naming the account, and a consent for an account the PSU does hold still
authorises and reads data.
The consent JWT stores request headers as [{name, values}] -- the shape OBP's HTTPParam
serialises to. This read them as {"TPP-Redirect-URI": "..."}, so the lookup found nothing,
every time. tppRedirectUri stayed empty, the redirect never fired, and the PSU was left
sitting on a "you may now close this window" page after a successful authorisation.
Under the Redirect approach that return is the last step of the ceremony, not a courtesy:
the TPP is waiting for the PSU to come back before it can use the consent.
Verified against a running stack: the page now sends the browser to the TPP's registered
redirect URI, which for the demo app lands back on its accounts page with the consent in
session.
… missing Starting a Berlin Group consent authorisation mints a fresh challenge and delivers a new OTP. It was being started from load(), and SvelteKit re-runs load() after any action that does not redirect -- so a mistyped code replaced the challenge the PSU was answering, their retry answered one whose code they had never seen, and that failure minted another. The PSU could not catch up, and every attempt sent another message. The authorisation is now started once and kept in the session for the whole ceremony, with an explicit "send a new one" button as the only path that supersedes it -- which a code that expired or never arrived now needs, since a wrong answer no longer does it by accident. The UK consent page told the PSU "you have no accounts at this bank" whenever the account list failed to load, because a failed call and an empty bank both ended as an empty array. That is a claim about the PSU with no evidence behind it: someone who does hold accounts there was told they do not, given no reason, and left with the Confirm button disabled. The two states are now distinct and the failure carries its reason.
/health became an unconditional 200 so an orchestrator would stop restarting a working instance over a dependency it does not need to serve pages. That left nothing a machine could ask about dependency health: /status renders the same verdict for a human but is a page and answers 200 whatever it finds. /ready now reports summarizeHealth's verdict as a status code. `partial` stays ready on purpose -- it means a group still has a working member, and pulling the instance out of rotation for that turns a degraded login into no login -- while `unknown` does not, because no check has reported yet and readiness on no evidence is how a starting instance takes traffic it cannot serve. The accounts the PSU ticked travelled to the SCA step in the query string, where they were editable between the screen that showed them and the answer that committed them. OBP-API refuses accounts the PSU does not hold, so the exposure was bounded to their own -- but the consent record is what an audit reads, and it could name accounts that never appeared on the screen they consented from. The selection is now held against the challenge it was minted for and read from the session. Also: the provider handed back to OBP-OIDC was read off the session's `user`, which has no such field, so it was always undefined and the parameter its own comment says OBP-OIDC needs to resolve the PSU was never sent. It lives on the oauth entry.
…hout Running the endpoint showed it answering 503 on a working instance. Opey was down -- as it usually is locally -- and summarizeHealth counts every non-OAuth2 check as core, so its overall verdict was `unhealthy`. Reading readiness off that verdict reproduced exactly the judgement /health was changed to stop making: pull a Portal that renders every page and serves every call out of rotation because an optional assistant is unavailable. Readiness is now decided here instead: every required dependency healthy, and at least one OAuth2 provider healthy. Optional services are named in a short exclusion list, so a service added later gates readiness by default and excusing it has to be written down. summarizeHealth's verdict is still reported in the body -- it is the right answer for the human view on /status, which is a different question.
setData writes to the store only when saveUninitialized is set, which this app does not set -- otherwise it just replaces the in-memory object and the change dies with the request. Every other call site in this codebase already pairs it with save(); the three added here did not. The consequence was immediate. The UK flow stored the PSU's account selection and redirected to the SCA step, which then read the session, found nothing, and answered "No accounts were selected for this consent. Please start over." to someone who had just selected them. The Berlin Group SCA page stored the authorisation it had started and forgot it before the next render, so it began minting a fresh challenge each time -- reinstating the defect it was written to fix.
SvelteKit's "?/name" shorthand replaces the page's query string rather than appending to it. Naming the action -- which the resend button made necessary, since a default action cannot coexist with named ones -- turned the form's bare "?/confirm" into a URL with no CONSENT_ID. The form had no action at all before, so it posted to the current URL and the parameter survived. A wrong OTP does not redirect, so load() re-ran against the stripped URL and the page answered "Missing required parameter: CONSENT_ID." instead of saying why the code was rejected. The sibling UK consent page already preserves its query string this way, for the same reason.
fix: show the PSU what they are granting, and why a Berlin Group call failed
/ready is unauthenticated -- a probe has to be reachable before the app can prove anything about itself -- and it was returning summarizeHealth's whole snapshot map. Those snapshots carry each dependency's configured URL and the text of its last error, so anyone who asked got the internal hostnames and ports of OBP-API, Opey and every OAuth2 provider, plus how each was currently failing. It now answers the two things a probe exists to say: whether to route traffic here, and which dependency is holding that up. A blocked instance still names the service by name, so it stays diagnosable without a shell. The detail is still on /status, which renders it for a signed-in human.
fix: stop the readiness probe publishing the deployment's internals
hongwei1
changed the title
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
17 commits, 16 files, +647/-67, all in
apps/portal. The original title described only the first commit; this is what the branch now carries.Two themes: the PSU could not see what they were about to grant, and several flows did not survive their own redirects.
What the PSU is shown
Flows that did not work
/obp/v1.3/berlin-group/..., but OBP-API registers these routes at/berlin-group/v1.3/...with no/obp/prefix (Http4sBGv13PIS.scala), so both the authorisation POST and the OTP PUT hit a path that does not exist.Session and redirect correctness
Three of these are worth naming precisely, because they are the class of bug that passes every server-side test:
setDataonsvelte-kit-sessionsonly writes to the store whensaveUninitializedis configured, which the Portal does not set. Every other call site in the repo pairs it withsave(); the new ones did not, so the UK account selection did not survive the redirect and the BG authorisation id was forgotten on each render.action="?/confirm"droppedCONSENT_IDand a mistyped OTP re-ranload()against a URL that no longer identified the consent.Probes
/healthis a liveness probe again — it must not fail because a dependency is down — and/readyis the readiness probe./readyhad been returning the full health snapshot, including each dependency's configured URL and its most recent error text, to an unauthenticated caller; it is now{ready, blocking}, still naming which service blocks. Detail stays on/status, behind login.Verification
Each flow was driven in a real browser against a local stack, not only asserted server-side. That mattered: the two session bugs above were found by clicking through the flows, and were invisible to the 95-check API suite in place at the time, because both live entirely in the browser's half of the exchange.