Enforce UK consent check on aggregate endpoints, upgrade 403 assertions, fix build-script hang#2873
Open
hongwei1 wants to merge 23 commits into
Open
Enforce UK consent check on aggregate endpoints, upgrade 403 assertions, fix build-script hang#2873hongwei1 wants to merge 23 commits into
hongwei1 wants to merge 23 commits into
Conversation
… endpoints GET /aisp/balances and GET /aisp/transactions fetched every private account for the caller without validating the bearer token's UK consent binding first, unlike every sibling AISP endpoint (including their own single-account counterparts). A DirectLogin or OAuth2 token with no bound consent fell through into the account-fetch path and surfaced as a 500 Unknown Error instead of the expected 403 OBP-35035, and callers with real consent got real account data with no consent enforcement at all. Add the same checkUKConsent + passesPsd2Aisp guard already used by getAccountsAccountIdBalances and getAccountsAccountIdTransactions so all five real-data AISP endpoints share one consent contract. Also fix createTransactionsJsonNew to take the AccountId from the request path instead of deriving it from the first transaction's bank account, which returned a null AccountId whenever the account had no transactions.
# Conflicts: # obp-api/src/test/scala/code/api/UKOpenBanking/v4_0_1/UKOpenBankingV401AccountInfoTests.scala
…ubstitution Redirecting to > >(tee "$RUNTIME_LOG") makes the tee process inherit this script's own stdout. A caller that captures this script's output with out=$(./flushall_build_and_run.sh --background ...) never sees EOF, because the long-running server (and the tee backing the substitution) never exits on its own — the command substitution hangs forever even though the server started successfully. Redirect straight to the log file instead.
checkUKConsent no longer needs a live Hydra introspection endpoint — it checks the Bearer token's consent_id claim against an AUTHORISED consent. Two comments describing the balances/transactions scenarios still referred to the removed Hydra dependency; align them with the class doc above, which was already corrected during the merge from Simon/develop.
… v4.0.1 endpoints develop's Hydra removal (PR OpenBankProject#2866) rewrote ConsentUtil.checkUKConsent to read the consent_id claim directly off the Bearer access token instead of calling an external Hydra introspection endpoint. That was the only blocker keeping getAccounts/getAccountsAccountIdBalances/getAccountsAccountIdTransactions on a weak "authenticated -> not 401" assertion (mirroring the same limitation in UKOpenBankingV310AisTests for the equivalent v3.1 endpoints). The OAuth1-signed test requests carry no Bearer JWT, so the consent_id claim lookup now fails deterministically with 403 ConsentIdClaimMissing — asserted directly (status code + error message) instead of the previous placeholder.
…ent-403-assertions # Conflicts: # obp-api/src/test/scala/code/api/UKOpenBanking/v4_0_1/UKOpenBankingV401AccountInfoTests.scala
…tions' into develop-obp # Conflicts: # obp-api/src/test/scala/code/api/UKOpenBanking/v4_0_1/UKOpenBankingV401AccountInfoTests.scala
Neither flushall_build_and_run.sh nor flushall_fast_build_and_run.sh printed the port the server actually binds, only its PID. smoke_test.sh's start_and_test() parses "PID: <n>" and "PORT: <n>" from the script's captured output to know where to poll /root; with no PORT line it fails fast with "未打印实际监听端口" even after the server started and bound successfully, leaving the server running with nothing to verify it or clean it up. Print the actual dev.port value read from props/default.props (falling back to 8080) right after the PID line in both scripts' background-mode branch.
hongwei1
changed the title
Matches the codebase-wide convention of bank-scoped params before account-scoped ones.
createUKConsentJWT wrote every granted permission as ConsentView(bank_id=null, account_id=null, view_id=permission) at consent-creation time, before any account is known. That row can never match a real account (User.hasAccountAccess does plain bank_id/account_id equality, no wildcard), so a UK consent's declared Permissions had no effect on what could actually be read — access depended entirely on unrelated pre-existing AccountAccess grants. Add Consent.grantUKConsentAccountAccess, called from authoriseUKConsent once the PSU has selected accounts (via a new account_ids field on the authorise request body), mirroring how updateViewsOfBerlinGroupConsentJWT resolves Berlin Group's IBAN-keyed access into real per-account grants. Since UK consents are exercised via an opaque OAuth2 Bearer token rather than BG's per-request Consent-JWT header, the AccountAccess rows are granted eagerly at authorisation time instead of re-derived per request. Add a regression test: a consent scoped to ReadAccountsBasic grants that view but leaves ReadBalances locked.
…iews The 6 UK Open Banking v4.0.1 system views (ReadAccountsBasic/Detail, ReadBalances, ReadTransactionsBasic/Debits/Detail) previously all shared the generic SYSTEM_VIEW_PERMISSION_COMMON set, so Detail granted nothing beyond Basic and a consent scoped to Balances alone still exposed full transaction and counterparty data. Berlin Group's ReadAccountsBerlinGroup and ReadBalancesBerlinGroup had zero ViewPermission rows at all (pure membership gating), unlike ReadTransactionsBerlinGroup which already has a full permission set. Add one can_* permission constant per view in constant.scala, mapped from the UK v4.0.1 spec's Detail Permissions table, and wire each into MapperViews.applyDefaultsForSystemView in place of the shared COMMON/untouched branches. accountant keeps SYSTEM_VIEW_PERMISSION_COMMON unchanged, split into its own case. Note for reviewers: existing deployments with these views already created at boot will have stale ViewPermission rows from the old COMMON set (or none, for the two BG views) — factoryResetSystemView needs to be re-run per view, or a migration added, to pick up the new defaults.
The UK v4.0.1 Permissions enum requires ReadTransactionsCredits as an independently-selectable code alongside ReadTransactionsDebits, but only Debits (and Basic/Detail) existed as a view constant. Add the missing view id and can_* permission set (shared with ReadTransactionsDebits and ReadTransactionsBasic, since direction is a query-parameter-shaped concern, not a field-visibility one), wire it into MapperViews.applyDefaultsForSystemView and the additional_system_views boot whitelist, and create it unconditionally in the sandbox data importer alongside the other UK/BG views for consistency. Direction filtering itself (returning only credit- or debit-direction transactions per which view the consent granted) is not wired into the transactions endpoint yet — documented as a follow-up in constant.scala, since it also depends on fixing a separate, pre-existing bug where JSONFactory_UKOpenBanking_401 always reports CreditDebitIndicator as "Credit" regardless of the actual transaction. Add exact permission-set regression coverage for all 6 UK + 2 BG system views to MappedViewsTest.
The sample.props.template's documented list of valid additional_system_views values had drifted from Boot.scala's actual whitelist: it was missing ReadBalancesBerlinGroup and ReadTransactionsBerlinGroup (already live for a while) and the newly-added ReadTransactionsCredits.
Not a bug fix — a design guard-rail. Berlin Group's frequencyPerDay, recurringIndicator, and validUntil, and UK's TransactionFromDateTime, ToDateTime, and ExpirationDateTime already live on the consent record (ConsentJWT), never as a view can_* permission. As the UK/BG system view permission sets get filled in, it's easy to reach for a can_* string to express a consent's time-boxing or access-frequency limit (e.g. a hypothetical can_see_transactions_last_90_days) — that conflates the two layers. Add a note at both ends (ConsentJWT and the SYSTEM_READ_*_VIEW_PERMISSION definitions) so future changes don't cross this boundary.
GET /obp/v5.1.0/user/current/consents/CONSENT_ID hard-required an exact mUserId match, so a PSU could never view a Berlin Group consent before completing SCA (BG consents are created via client_credentials with no owner yet). Relax the check to also allow consents that have no owner assigned. Also fix createStartConsentAuthorisationJson returning authenticationMethodId as the authorisationId instead of challengeId -- the PUT endpoint resolves the authorisation by challengeId, so any caller following the documented response field got a 400 on the confirmation step.
… end Create a PSU-less Berlin Group consent directly via the provider (mirroring how POST /consents builds one for a client_credentials caller), then exercise the full authorisation flow: another logged-in user can view it before SCA, starting the authorisation returns a resolvable authorisationId, and submitting the correct OTP claims the consent for the answering PSU. A second scenario documents that a wrong OTP is rejected with 400 and leaves the consent unclaimed.
UK v4.0.1 requires AWAU/AUTH/RJCT/CANC/EXPD on the wire, but OBP was returning the internal long enum names (AWAITINGAUTHORISATION, AUTHORISED, REVOKED) verbatim -- disagreeing with the endpoint's own documented example response, which already showed AWAU. Add a status-code mapping at the JSON serialization boundary only; internal storage keeps the long names, matching how BG and OBP standard consents already work. REVOKED maps to CANC since OBP does not otherwise distinguish PSU-dashboard-cancel from AISP-DELETE-revoke and the spec has no other status for that distinction. Also add the StatusReason array required by OBReadConsentResponse1, which was previously present only in the hardcoded ResourceDoc example, never actually populated in real responses. This is a breaking wire-format change for any caller currently depending on the long status-name strings.
ExpirationDateTime/TransactionFromDateTime/TransactionToDateTime are 0..1 per the UK spec's OBReadConsent1 (open-ended if absent), but the shared v3.1.0/v4.0.1 request body class declared them as required Strings -- omitting any one failed json4s extraction. Thread Option[Date] through saveUKConsent/createUKConsentJWT (mirrors the Option[Date] validUntil pattern createBerlinGroupConsentJWT already uses) so a missing ExpirationDateTime means the consent never expires -- represented as Long.MaxValue in the JWT's exp claim, not "now" (which is what the BG sibling function defaults to when its own validUntil is absent; that default is wrong for "no limit" and not copied here). Nothing currently reads this claim for UK consents (checkUKConsent doesn't check expiry yet), so this only sets up correct behaviour for that future gap. Also fix DateWithDayFormat's silent truncation: it's a bare "yyyy-MM-dd" SimpleDateFormat, so a full ISO-8601 datetime like "2020-01-01T00:00:00+00:00" parsed leniently and silently dropped the time and offset, and a malformed value threw an uncaught ParseException that surfaced as 500 instead of 400. Add parseIso8601OrDayDate (tries full ISO-8601 first, falls back to a bare date) and route it through NewStyle.function.tryons so malformed input reaches 400 -- a bare Future.fromTry(Try(...)) doesn't get mapped to 400 by ErrorResponseConverter, since it only preserves the code from APIFailureNewStyle. Both v3.1.0 and v4.0.1 create/get handlers updated (the request body class and parser are shared); GET responses null-guard the now-possibly-absent stored dates instead of stringifying a null Date.
An AUTHORISED UK consent was effectively perpetual: checkUKConsent only checked not-before (creationDateTime), never the consent's own ExpirationDateTime or the JWT exp, and ConsentScheduler's expiry tasks filtered on apiStandard=BG / apiStandard=obp, excluding UK entirely. A PSU who consented to "until date X" kept granting access indefinitely unless the consent was manually revoked -- a minimum-necessary-access / data-minimisation problem. Add both layers: - Reactive (the actual security control): checkUKConsent now rejects an AUTHORISED consent whose ExpirationDateTime has passed, with the existing OBP-35003 ConsentExpiredIssue (401). A null ExpirationDateTime means the consent never expires (0..1 per spec, open-ended if absent) and is skipped. This closes the gap immediately regardless of scheduler cadence. - Proactive: a new ConsentScheduler.expiredUKConsents task flips long-past-expiry AUTHORISED UK consents to EXPIRED so the stored status stays accurate for GET/dashboard reads. Interval prop uk_open_banking_expired_consents_interval_in_seconds (default 601, 0 to disable), mirroring the existing BG/OBP expiry tasks. A null mExpirationDateTime never matches By_< against a Date, so perpetual consents are correctly never selected.
authoriseUKConsentChallenge and authoriseUKConsent hard-guarded status == AWAITINGAUTHORISATION, so once a consent was AUTHORISED nothing could reopen it -- a TPP whose access token was lost had to create a brand-new consent. The UK spec allows re-authentication of the same ConsentId while its status is AUTH or CANC (OBP's REVOKED) and its ExpirationDateTime has not elapsed. Relax both endpoints' status guard to ukReAuthableStatuses (AWAITINGAUTHORISATION, AUTHORISED, REVOKED); EXPIRED and REJECTED stay terminal. Add two guards that the previous single-status check made unnecessary but re-auth now requires: - not-expired: reject re-auth of a consent past its ExpirationDateTime (null = never expires), consistent with the reactive expiry check added to checkUKConsent. - same-PSU: a consent already bound to a user may only be re-authorised by that same user, so a different user of the same consumer cannot hijack it (updateConsentUser rebinds mUserId unconditionally). Re-running the SCA + grantUKConsentAccountAccess flow is already idempotent (grantAccessToViews revokes-then-regrants per view), so a second authorise cannot double-grant. Tests cover the two new rejection guards at the challenge-start step. The successful re-auth happy path needs a completed SCA/OTP ceremony, which has no test harness in this repo yet (these authorise endpoints shipped without coverage), so it is not asserted here.
POST /aisp/account-access-consents (both v3.1.0 and v4.0.1) hard-failed with AuthenticatedUserIsRequired whenever no PSU was present, so a TPP could not lodge a consent with a client-credentials (app-only) token -- contradicting the UK spec's Step 2 (the TPP creates the consent as an app; the PSU authorises it later) and OBP's own doc comment on authoriseUKConsent, which describes exactly that client-credentials lodging model. Relax the check to reject only a fully anonymous request (no consumer and no user); a consumer-only context now lodges the consent with no bound PSU (mUserId stays null until authoriseUKConsent binds it after SCA), mirroring the Berlin Group native consent flow. saveUKConsent already takes user: Option[User], so a request that does carry a user (e.g. DirectLogin) is unchanged -- createdByUser just carries it through. The consent row grants nothing until authorised, so this does not widen data access; it only lets the spec-correct app-only lodging step work. The existing "authenticated -> 201" and "unauthenticated -> 401" scenarios still pass (401 now means fully-anonymous rather than no-PSU). The pure consumer-only path can't be exercised via the DirectLogin-based test harness (DirectLogin always binds a user), so it isn't asserted directly here.
No UK v4.0.1 response carried x-fapi-interaction-id. The only related mechanism was a generic, opt-in, empty-by-default header-mirror prop that echoes a request header verbatim if present -- it never generated one when the TPP omitted it, so interaction tracing didn't work for the common case. (An earlier plan iteration pointed at getHeadersNewStyle for this, but that path is dead Lift-era code never called from the http4s stack.) Wrap the UK v4.0.1 aggregator's routes in an outer middleware that sets x-fapi-interaction-id on every response: echoed from the request when the TPP supplies one, otherwise a freshly generated UUID. Applied outside ResourceDocMiddleware so it also covers error responses. Does not introduce x-fapi-financial-id (correctly absent since v3.x).
|
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
1 participant
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.
Summary
GET /aisp/balances,GET /aisp/transactions) — previously only the per-account endpoints checked it, letting a token with no bound consent through to real data or a 500 instead of the expected 403. Also fixescreateTransactionsJsonNewto take theAccountIdfrom the request path instead of deriving it from the first transaction's bank account (returnednullwhen the account had no transactions).checkUKConsent-gated v4.0.1 endpoints (getAccounts,getAccountsAccountIdBalances,getAccountsAccountIdTransactions) from a weak "authenticated -> not 401" test assertion to a real, deterministic 403ConsentIdClaimMissingassertion — now thatcheckUKConsentreads theconsent_idclaim off the Bearer token directly (no external Hydra call), OAuth1-signed test requests deterministically fail the claim lookup.flushall_build_and_run.sh's background server launch:> >(tee "$RUNTIME_LOG")is a process substitution whoseteeinherits the script's own stdout, so a caller capturing this script's output viaout=$(...)never sees EOF (the server never exits, so neither does the tee backing the substitution) — even after the server started successfully. Redirect straight to the log file instead.PORTfrom bothflushall_build_and_run.shandflushall_fast_build_and_run.sh's background-mode output (previously onlyPIDwas printed), so callers that parse the script's output (e.g. a local smoke-test wrapper) can find the right port to poll instead of failing with "port not printed" even on a successful start.checkUKConsentmoved off the Hydra dependency.Test plan
run_tests_parallel.sh— all 4 shards passed, 3125 tests / 0 failuresUKOpenBankingV401AccountInfoTests— 61/61 passed after the consent-check + comment merge/root) — verified clean after the script fixes, twice