fix: unify consent ownership checks for OBP-native reads and Berlin Group authorisation - #62
Merged
Merged
Conversation
GET /obp/v5.1.0/user/current/consents/CONSENT_ID compared the consent's PSU against CallContext.userId, which returns the authenticated principal. Under Consent-Id / Consent-JWT authentication that principal is the per-consent shadow user, not the person, so the comparison could never match and the PSU was told their own consent did not exist. The subject is now CallContext.humanUser, the accessor the codebase already keeps for this distinction and the one checkUKConsent uses for the same comparison. The rule moves into Consent.checkObpConsentUserAccess so it can be stated once, argued once and tested without standing up a request, following validateUKConsentPermissions and checkUKConsentAccess. A consent with no PSU yet stays readable, deliberately: this endpoint is where a PSU inspects a consent before deciding to authorise it, and the app doing the inspecting belongs to the PSU rather than to the TPP that lodged the consent, so the Consumer fallback the standards use would break the journey instead of tightening it. That is where OBP-native's rule parts company with theirs, and why this is its own function. What it leaves open is recorded in the scaladoc: an unbound consent's metadata is readable by any authenticated caller who knows its consent id. Behaviour change: a request authenticated by a consent can now read that consent, where it previously received OBP-35001. Reads by an unrelated user are unaffected and still 404.
POST /consents/CONSENTID/authorisations and PUT .../AUTHORISATIONID are the two calls that decide who a consent ends up belonging to: the first mints the SCA challenge, the second answers it and writes the PSU onto the consent row. Neither carried an ownership guard, so any authenticated caller could raise a challenge on any consent id and then answer their own, claiming a consent lodged by a different TPP or re-binding one another PSU had already authorised -- updateConsentUser overwrites mUserId unconditionally. Leaving these consents unowned at lodging time, which the standard wants and 6060f42 implemented, widened what that reaches. The Consumer half is the standard's own blanket rule, stated once for the whole API in the Implementation Guidelines, section 4.11 API Access Methods: all methods submitted by a TPP addressing dynamically created resources may only apply to resources created by the same TPP before. A consent and its authorisation sub-resources are such resources, and deleteConsent and getConsentInformation in this same file already enforce exactly that; only the authorisation pair was left without it. The PSU half covers re-binding, which the standard leaves to the ASPSP and says so where it defines PSU-ID: the ASPSP might check whether PSU-ID and token match. That lands on the same rule as checkUKConsentAccess by a different route -- UK's rests on its Endpoints table marking these calls Client Credentials, Berlin Group's on the blanket same-TPP rule plus PSU binding happening at SCA time -- so the two now share one private implementation while each keeps its own argument. Consent.genuinePsu extracts the pseudo-user filter 6060f42 left duplicated inline. It is required here, not tidying: a client-credentials token resolves to an auto-vivified user keyed on the caller's own client key, and comparing that against a consent's real owner would refuse a legitimate TPP poll under the Redirect approach, where the PSU authenticates at the ASPSP rather than through the TPP. Behaviour change: both endpoints now return 403 OBP-35015 when the caller's Consumer did not lodge the consent, and 403 OBP-35023 when a genuine PSU tries to take over a consent already bound to someone else. Both previously succeeded. A TPP that lodges and authorises under one Consumer, which is what the Berlin Group flow describes and what the end-to-end suite exercises, is unaffected.
Follow-up work on the Berlin Group authorisation handlers builds on this function, and the case it depends on is the one that looks like an absence. Per the standard the caller of those endpoints is the TPP, with the PSU's authentication factors travelling in the request body rather than in the session, so None is the ordinary answer for a conforming call -- not a failure to defend against. Left to a scaladoc, nothing would catch that being narrowed later. Covers the four shapes: no user in the session, only the Consumer's own auto-vivified pseudo-identity, a genuine PSU, and the degenerate case where no Consumer was identified at all. The last one keeps the pseudo-user, since there is no client key to compare against, so it also asserts what checkBerlinGroupConsentAccess then does with it -- refuse on the PSU half when the consent is bound and on the Consumer half when it is not, rather than letting it through.
The new consent-access suite had its own copy of the consent body, the PSU-less consent builder and the client-credentials session, which the account-information suite already defined. The quality gate caught it as duplicated new code, and it was a maintenance trap besides: the client-credentials fixture encodes a non-obvious fact about how OAuth2 token parsing auto-vivifies a user, and a copy that drifted from it would quietly stop testing the thing it exists for. Moves them into a BerlinGroupConsentFixtures trait that both suites now extend, and drops both copies. No behaviour change -- the fixtures are the account-information suite's originals, moved rather than rewritten.
|
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
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.



Consent ownership was decided three different ways across the codebase and, on the two Berlin Group calls that actually bind a consent to a person, not at all. This fixes both remaining gaps: the OBP-native read compared against the wrong subject and hid a PSU's own consent from them, and the Berlin Group authorisation pair let any authenticated caller claim a consent that was not theirs.
Structure follows #57: each rule is a named function in
ConsentUtil, argued in its scaladoc and testable without standing up a request. The rules themselves are derived separately per standard — where they agree, the agreement is stated rather than assumed.By file
code/api/util/ConsentUtil.scalacheckObpConsentUserAccess— new. The OBP-native read's rule: a bound consent belongs to the human the request is on behalf of; an unbound one stays readable. The second half is deliberate and load-bearing, not inherited sloppiness — this endpoint is where a PSU inspects a consent before authorising it, and the app doing the inspecting belongs to the PSU, not to the TPP that lodged the consent, so it is a different Consumer by design. Adding the Consumer fallback the two standards use would break the journey. That divergence is why this is its own function rather than a third caller of the shared rule, and the residual it leaves open (an unbound consent's metadata is readable by anyone who knows its id) is written down in the scaladoc.checkBerlinGroupConsentAccess— new. Derived from the Implementation Guidelines' own blanket rule, §4.11 API Access Methods, p.24: "all methods submitted by a TPP, which are addressing dynamically created resources in this API, may only apply to resources which have been created by the same TPP before." A consent and its authorisation sub-resources are exactly that, which is already whatdeleteConsentandgetConsentInformationenforce inline in the same file. The PSU half — no taking over a consent someone else authorised — rests on the spec delegating PSU identity to the ASPSP where it definesPSU-ID: "the ASPSP might check whether PSU-ID and token match, according to ASPSP documentation."psuOrLodgingTppRefusal— the rule UK and Berlin Group turned out to share, extracted fromcheckUKConsentAccess, which becomes a thin delegate with its scaladoc untouched. The two derivations are different (UK's rests on its Endpoints table marking these calls Client Credentials; BG's on the blanket same-TPP rule plus SCA-time binding), so both keep their own argument and only the mechanism is shared. No behaviour change for UK.genuinePsu— extracts the pseudo-user filter6060f4252left duplicated inline, as that commit said it would once this file was free. Required, not tidying: see the trade-off note below.code/api/v5_1_0/Http4s510.scala—getConsentByConsentIdnow askscheckObpConsentUserAccesswithcc.humanUserinstead of comparing againstcc.userId.cc.userIdreturns the authenticated principal, which underConsent-Id/Consent-JWTauthentication is the per-consent shadow user, so the comparison could never match a consent's real owner.code/api/berlin/group/v1_3/Http4sBGv13AIS.scala— the guard is added to bothstartConsentAuthorisationAllandupdateConsentsPsuDataAll, before any side effect. Guarding only the PUT would have been half a fix: the POST is what mints the SCA challenge, so an unguarded POST lets a caller raise a challenge on any consent id and then answer their own. Both handlers dispatch on the body shape, and the guard sits in thetransactionAuthorisationbranch — the other three variants return mocked responses and never reach a consent.Tests — two new suites rather than edits to existing files, so the diff does not collide with in-flight work on those files.
Testing
Both gaps were pinned red before the fix. With the new
ConsentUtilfunctions present but the two call sites reverted, the four behavioural scenarios failed with exactly the symptoms described above:With the call sites restored: 24/24 across the two new suites (20, plus the four
genuinePsucontract scenarios added in the third commit).Consent-family regression — Berlin Group, all UK Open Banking, and the OBP-native consent suites, 21 wildcardSuites: 487 succeeded, 0 failed across 42 suites. That set includes the Berlin Group end-to-end SCA regression and
UKOpenBankingV401ConsentAccessTestsfrom #57, both untouched.Full local suite (
./run_tests_parallel.sh, JDK 25): 3309 tests, 0 failures, 0 errors, 0 skipped — all shards passed. CI on the fork: compile, report and all 9 test shards green.Trade-offs and behaviour changes
Tightening — Berlin Group.
POST /consents/CONSENTID/authorisationsandPUT .../AUTHORISATIONIDnow return 403 OBP-35015 when the caller's Consumer did not lodge the consent, and 403 OBP-35023 when a genuine PSU tries to take over a consent already bound to someone else. Both previously succeeded. A TPP that lodges and authorises under one Consumer — which is what the standard describes and what the end-to-end suite exercises — is unaffected; the Berlin Group journey stays green.Loosening — OBP-native.
GET /obp/v5.1.0/user/current/consents/CONSENT_IDnow resolves the human behind the request, so a call authenticated by a consent can read that consent. It previously returned OBP-35001. Reads by an unrelated user are unchanged and still 404.Why
genuinePsuis load-bearing. A client-credentials token does not leavecc.userempty — it resolves to an auto-vivified pseudo-user keyed on the caller's own client key. Feeding that to the guard as if it were a PSU would refuse a legitimate TPP polling its own bound consent, which is precisely the Redirect approach, where the PSU authenticates at the ASPSP rather than through the TPP. There is a scenario pinning this discriminating case.genuinePsu— contractSpelled out because follow-up work on the Berlin Group authorisation handlers depends on it.
genuinePsu(cc): Option[User]returns:cc.userempty)Noneuser.idGivenByProvider == cc.consumer.key)NoneSome(user)Some(user)— no key to compare againstSo
Noneis the normal, expected answer for a pure client-credentials call, which per the standard is what these two Berlin Group endpoints see: the caller is the TPP, and the PSU's factors travel in the body (psuData/scaAuthenticationData), never in the session.checkBerlinGroupConsentAccessis written for that: aNonecaller skips the PSU comparison entirely and is judged on the Consumer alone.The last row is the one degenerate case, and it fails closed rather than open: with no consumer identified,
callerConsumerIdis alsoNone, so a bound consent is refused on the PSU half and an unbound one on the Consumer half. All four rows are pinned by tests rather than left to the scaladoc, since narrowing them later would silently break the work built on top.What this does not do: the
val u = cc.user.openOrThrowException(AuthenticatedUserIsRequired)at the top of both handlers is untouched and still there. The new guard does not depend on it — it reads the session throughgenuinePsuinstead — but that line will still throw once a client-credentials token stops auto-vivifying a user. Reworking it belongs with the handler rework, not here.Deliberately not fixed here. The same shadow-user comparison still exists in
revokeMyConsent,revokeConsentAtBank, v3.1.0revokeConsent, and the two UK authorise guards inHttp4s510. They are a loosening in the same direction rather than a hole, and were left out to keep this diff to the two gaps it set out to close.Relationship to other PRs
Built on #57 (
checkUKConsentAccess, refactored here onto the shared rule) and #60 (which established, for the Berlin Group read twins, that the PSU never calls the API — the premise the Consumer half of the new guard relies on). Both merged before this branch was cut; it is based ondevelop-obpatb5d556d20and needs no rebase.