From 8caa6495d002408b504984d66f88c4560acb85c8 Mon Sep 17 00:00:00 2001 From: Hongwei Date: Wed, 12 Aug 2026 22:33:27 +0200 Subject: [PATCH 1/5] fix: refuse an SCA challenge raised for a different UK consent authoriseUKConsent fetched the started challenge and discarded it, so nothing tied the challenge to the consent being authorised. validateChallengeAnswerC4 is handed the consentId but the connector ignores it -- LocalMappedConnector matches on challengeId, supplied answer and expected user alone -- so the consent id travelled the whole way without ever being compared. A PSU could therefore answer a challenge raised for one of their consents against another. The two need not carry the same Permissions, so an OTP the PSU was given for a narrow consent authorised a wider one: measured on a local instance, a challenge minted on a ReadAccountsBasic consent moved a ReadAccountsBasic+ReadAccountsDetail+ReadBalances consent to AUTHORISED and bound the PSU to it, while the consent that was actually challenged stayed AWAITINGAUTHORISATION. UK Open Banking defines the authentication journey as taking place in the context of one consent -- the ConsentId is the intent identifier, carried into the request object so that the token that follows is bound to a specific consent. OBP mints its own OTP challenge instead of using that mechanism, so the binding the profile gets structurally has to be asserted here. The Berlin Group half of this same change already asserts it, getting the binding from its own URL shape; this brings the UK half into line, with the same guard, message and code. It also refuses a transaction-request challenge here for free, since that carries no consentId at all. --- .../scala/code/api/v5_1_0/Http4s510.scala | 17 +++++++- .../UKOpenBankingV401AccountInfoTests.scala | 41 +++++++++++++++++++ 2 files changed, 57 insertions(+), 1 deletion(-) diff --git a/obp-api/src/main/scala/code/api/v5_1_0/Http4s510.scala b/obp-api/src/main/scala/code/api/v5_1_0/Http4s510.scala index 70d4ad7a49..78e39f7e99 100644 --- a/obp-api/src/main/scala/code/api/v5_1_0/Http4s510.scala +++ b/obp-api/src/main/scala/code/api/v5_1_0/Http4s510.scala @@ -4396,7 +4396,22 @@ object Http4s510 { _ <- Helper.booleanToFuture(s"$InvalidJsonFormat The Json body should be the $PostUKConsentAuthoriseJsonV510 (account_ids must not be empty) ", 400, Some(cc)) { authJson.account_ids.nonEmpty } - (_, _) <- NewStyle.function.getChallenge(authJson.challenge_id, Some(cc)) + (startedChallenge, _) <- NewStyle.function.getChallenge(authJson.challenge_id, Some(cc)) + // The consent this challenge was minted for is the consent it may authorise, and nothing + // below establishes that: validateChallengeAnswerC4 is handed the consentId but the + // connector ignores it and matches on challengeId, answer and expected user alone. So a + // PSU holding an OTP raised for one consent could answer it against another of their own + // -- and since the two need not carry the same Permissions, an OTP the PSU was given for + // a narrow consent authorised a wider one, which is precisely the dynamic linking the + // profile is asking for ("all authentication journeys take place in the context of a + // consent"; the ConsentId is the intent identifier). + // + // Same guard, message and code as the Berlin Group twin in Http4sBGv13AIS, which gets + // the binding structurally by nesting authorisations under the consent's own path. It + // also refuses a transaction-request challenge here for free: that carries no consentId. + _ <- Helper.booleanToFuture(s"$InvalidChallengeChallengeId Current challengeId(${authJson.challenge_id}) does not belong to CONSENTID($consentId) ", 400, Some(cc)) { + startedChallenge.consentId.contains(consentId) + } (challenge, _) <- NewStyle.function.validateChallengeAnswerC4( ChallengeType.OBP_CONSENT_CHALLENGE, None, diff --git a/obp-api/src/test/scala/code/api/UKOpenBanking/v4_0_1/UKOpenBankingV401AccountInfoTests.scala b/obp-api/src/test/scala/code/api/UKOpenBanking/v4_0_1/UKOpenBankingV401AccountInfoTests.scala index ef6d535c66..f9dfd565c7 100644 --- a/obp-api/src/test/scala/code/api/UKOpenBanking/v4_0_1/UKOpenBankingV401AccountInfoTests.scala +++ b/obp-api/src/test/scala/code/api/UKOpenBanking/v4_0_1/UKOpenBankingV401AccountInfoTests.scala @@ -748,6 +748,47 @@ class UKOpenBankingV401AccountInfoTests extends UKOpenBankingV401ServerSetup { private def boundPsuOf(consentId: String): String = Option(storedConsent(consentId).userId).map(_.trim).getOrElse("") + // The profile defines the authentication journey as taking place in the context of one consent: + // the ConsentId is the intent identifier, and the intent id is carried into the request object so + // that "the access token that is eventually generated [is] bound to a specific consent". OBP mints + // its own OTP challenge instead of using that mechanism, so the binding the profile gets + // structurally has to be asserted here -- and the connector will not do it: LocalMappedConnector's + // validateChallengeAnswerC4 ignores the consentId it is handed and matches on challengeId, answer + // and userId alone. The Berlin Group twin of this endpoint already asserts it + // (Http4sBGv13AIS: startedChallenge.consentId.contains(consentId)). + feature("UKOB v4.0.1 a challenge only authorises the consent it was minted for") { + scenario("a challenge minted on one consent cannot authorise another -> 400, and the other consent is untouched", UKOpenBankingV401AccountInfo) { + setPropsValues("suggested_default_sca_method" -> "DUMMY") + val expiry = Some(new java.util.Date(System.currentTimeMillis() + 3600000L)) + // Two consents the same PSU may authorise. The PSU guard at the top of the authorise endpoint + // passes for both, so nothing but the binding stands between them. + val challenged = createUKConsent(None, expiry) + val other = createUKConsent(None, expiry) + + val challenge = makePostRequest(scaChallengeRequest(challenged) <@ (user1), "") + challenge.code should equal(200) + val challengeId = (challenge.body \ "challenge_id").extract[String] + + When("the challenge minted on one consent is answered against the other") + val refused = makePostRequest(authoriseRequest(other) <@ (user1), + s"""{"account_ids":["$acc"],"challenge_id":"$challengeId","answer":"123"}""") + + Then("it is refused with the same answer the Berlin Group twin gives") + refused.code should equal(400) + refused.body.extract[ErrorMessage].message should include("OBP-40022") + + // The refusal is the point, but so is what it did not do: an authorisation that got this far + // used to bind the PSU and flip the status on a consent nobody authenticated for. + And("the consent it was aimed at is left exactly as it was") + boundPsuOf(other) should equal("") + storedConsent(other).status should equal(ConsentStatus.AWAITINGAUTHORISATION.toString) + + And("so is the consent the challenge actually belonged to") + boundPsuOf(challenged) should equal("") + storedConsent(challenged).status should equal(ConsentStatus.AWAITINGAUTHORISATION.toString) + } + } + feature("UKOB v4.0.1 a refused authorisation does not claim the consent") { scenario("account_ids naming an account the PSU does not hold -> refused, consent left unbound", UKOpenBankingV401AccountInfo) { // The dummy SCA answer is `123` only when this is set; test props leave it unset, so the From cadb320cb3030b1391870be544027f9a71114409 Mon Sep 17 00:00:00 2001 From: Hongwei Date: Wed, 12 Aug 2026 23:06:29 +0200 Subject: [PATCH 2/5] fix: a consent belongs to one TPP, and one that names none belongs to nobody Two ways the shared consent-ownership rule answered "allowed" when it should not have. Matching the PSU ended the enquiry. psuOrLodgingTppRefusal returned None as soon as the two user ids agreed, without ever comparing the Consumer, so a second TPP holding a session for the same person could read and revoke a consent the first TPP had lodged. One TPP's mandate over a consent is not another's: Berlin Group states it for the whole API -- "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" (Implementation Guidelines 4.11) -- and UK reaches the same place per endpoint, scoping GET and DELETE of an account-access-consent to one "that they have created", on a client-credentials token where no PSU is party to the call at all. A PSU session on those endpoints is an OBP extension on top of the standard, so it cannot be the thing that waives the standard's own check. A PSU match is now necessary rather than sufficient, and falls through to the Consumer comparison. The exception is the ASPSP's declared SCA front end, which under Redirect arrives under its own Consumer by definition -- that is why it has to be declared at all -- so it keeps passing, and only on a consent whose PSU it already agrees with. A consent recording no Consumer was addressable by everyone. The check read `owner.forall(...)`, which is vacuously true when there is no owner, so "absent" was being read as "matches every caller" where it means "there is nobody this belongs to". These rows are real: 10 of 566 Berlin Group, 50 of 463 OBP-native and 4 of 753 UK consents record no consumer on a long-lived instance. In Berlin Group they were already unreachable by accident -- the hand-rolled `consent.mConsumerId.get == ...` compare in the five consent reads is false for everyone when the stored value is null -- but the UK pair let anyone in. They are now refused, with consent_allow_legacy_unrecorded_tpp for an operator who needs a migration window and accepts that it means any authenticated caller can read and revoke them. Default false, documented, and it warns on every use. Three existing assertions invert, and say at the assertion why: they pinned the fail-open rather than the rule. UKOpenBankingV401AccountInfoTests.createRealConsent also stopped passing consumerId = None -- that produced a row no production path creates, since createAccountAccessConsents records the calling consumer, and the fixture only got away with it while such a row was readable by anyone. --- .../resources/props/sample.props.template | 14 ++++++ .../scala/code/api/util/ConsentUtil.scala | 48 +++++++++++++++---- .../UKOpenBankingV401AccountInfoTests.scala | 8 +++- .../UKOpenBankingV401ConsentAccessTests.scala | 32 +++++++++++-- .../BerlinGroupV13ConsentAccessTests.scala | 37 +++++++++++++- 5 files changed, 122 insertions(+), 17 deletions(-) diff --git a/obp-api/src/main/resources/props/sample.props.template b/obp-api/src/main/resources/props/sample.props.template index 1343fe7138..71aeb66167 100644 --- a/obp-api/src/main/resources/props/sample.props.template +++ b/obp-api/src/main/resources/props/sample.props.template @@ -1896,6 +1896,20 @@ securelogging_mask_email=true # already configured for Berlin Group Redirect SCA needs no edit. # sca_front_end_consumer_ids= +# Let a consent that records no lodging TPP be addressed by any authenticated caller. +# +# A consent belongs to the Consumer that lodged it -- Berlin Group states it for the whole API +# ("all methods submitted by a TPP ... may only apply to resources which have been created by the +# same TPP before", Implementation Guidelines 4.11) and UK scopes GET and DELETE of an +# account-access-consent to one "that they have created". A row recording no Consumer therefore +# matches no caller, and is refused. +# +# Rows like that exist: they predate the Consumer being recorded on a consent at all. Turning this +# on restores the old behaviour for them -- and the old behaviour is that ANY authenticated caller +# can read and revoke them, which is why it is off by default and warns on every use. It is a +# migration window, not a setting: re-lodge the affected consents and turn it back off. +# consent_allow_legacy_unrecorded_tpp=false + # Berlin Group: the Consumers that are this ASPSP's own Strong Customer Authentication front end, # comma separated consumer ids. Under the Redirect approach the PSU authenticates at the ASPSP, so # the authorisation calls on /berlin-group/v1.3/consents/{id}/authorisations arrive from that front diff --git a/obp-api/src/main/scala/code/api/util/ConsentUtil.scala b/obp-api/src/main/scala/code/api/util/ConsentUtil.scala index 8d01dca2b7..beb427738e 100644 --- a/obp-api/src/main/scala/code/api/util/ConsentUtil.scala +++ b/obp-api/src/main/scala/code/api/util/ConsentUtil.scala @@ -1741,7 +1741,12 @@ object Consent extends MdcLoggable { ): Option[String] = { (present(consentUserId), callerUserId.flatMap(present)) match { case (Some(psu), Some(caller)) if psu != caller => Some(ErrorMessages.ConsentDoesNotMatchUser) - case (Some(_), Some(_)) => None + // The PSU matches, which is necessary but not sufficient: the Consumer still has to, and that + // comparison is below. The exception is the ASPSP's own front end, which under Redirect is by + // definition not the Consumer that lodged the consent -- that is the whole reason it has to be + // declared. Without this case a declared front end could not complete an SCA it is allowed to + // start, because psuOrLodgingTppRefusal would refuse it on the Consumer. + case (Some(_), Some(_)) if callerIsScaFrontEnd => None // Only while the consent is still unclaimed. A caller with no PSU used to reach this too, so a // declared front end presenting client credentials could drive the authorisation of a consent // already bound to somebody else -- the opposite of what the paragraph above promises. @@ -1777,16 +1782,39 @@ object Consent extends MdcLoggable { callerUserId: Option[String], callerConsumerId: Option[String] ): Option[String] = { + // The Consumer that lodged the consent is what identifies a legitimate caller, and a consent + // recording none belongs to nobody. This used to read `owner.forall(...)`, which is vacuously + // true when there is no owner -- so a consent with no lodging TPP was addressable by every + // authenticated caller. "Absent" was being read as "matches everything" where it means "there is + // nobody this belongs to", and neither standard supports the first reading: Berlin Group scopes + // a resource to "the same TPP" that created it (IG 4.11), and UK scopes GET and DELETE to "an + // account-access-consent resource that they have created". + // + // It is a live population -- 10 of 566 Berlin Group, 50 of 463 OBP-native and 4 of 753 UK + // consents record no consumer on a long-lived instance -- so the prop is there for an operator + // who needs a migration window and accepts what it means. Default false, and it says so every + // time it is used. + def lodgingTppRefusal: Option[String] = present(consentConsumerId) match { + case Some(owner) if callerConsumerId.flatMap(present).contains(owner) => None + case Some(_) => Some(ErrorMessages.ConsentDoesNotMatchConsumer) + case None if APIUtil.getPropsAsBoolValue("consent_allow_legacy_unrecorded_tpp", defaultValue = false) => + logger.warn( + "A consent recording no lodging TPP was addressed and allowed through because " + + "consent_allow_legacy_unrecorded_tpp is set. Any authenticated caller can read and revoke " + + "such a consent while that stays on. Re-lodge the affected consents and turn it off.") + None + case None => Some(ErrorMessages.ConsentDoesNotMatchConsumer) + } + (present(consentUserId), callerUserId.flatMap(present)) match { - case (Some(psu), Some(caller)) => - // The consent belongs to a PSU and the caller is acting as one: they must be the same PSU. - if (psu == caller) None else Some(ErrorMessages.ConsentDoesNotMatchUser) - case _ => - // Either the consent has no PSU yet, or the caller is not acting as one. Either way the - // Consumer that lodged it is what identifies a legitimate caller. - val owner = present(consentConsumerId) - if (owner.forall(id => callerConsumerId.flatMap(present).contains(id))) None - else Some(ErrorMessages.ConsentDoesNotMatchConsumer) + // The consent belongs to a PSU and the caller is acting as one: they must be the same PSU. + case (Some(psu), Some(caller)) if psu != caller => Some(ErrorMessages.ConsentDoesNotMatchUser) + // Matching the PSU is necessary, not sufficient. This used to return None here, so agreeing on + // the person ended the enquiry and the Consumer was never looked at -- a second TPP holding a + // session for the same PSU could read and revoke the consent a first TPP lodged. One TPP's + // mandate over a consent is not another's, and IG 4.11's same-TPP rule admits no PSU + // exception. So a PSU match falls through to the Consumer comparison rather than skipping it. + case _ => lodgingTppRefusal } } diff --git a/obp-api/src/test/scala/code/api/UKOpenBanking/v4_0_1/UKOpenBankingV401AccountInfoTests.scala b/obp-api/src/test/scala/code/api/UKOpenBanking/v4_0_1/UKOpenBankingV401AccountInfoTests.scala index f9dfd565c7..504969c6f1 100644 --- a/obp-api/src/test/scala/code/api/UKOpenBanking/v4_0_1/UKOpenBankingV401AccountInfoTests.scala +++ b/obp-api/src/test/scala/code/api/UKOpenBanking/v4_0_1/UKOpenBankingV401AccountInfoTests.scala @@ -54,12 +54,18 @@ class UKOpenBankingV401AccountInfoTests extends UKOpenBankingV401ServerSetup { | "Risk": {} |}""".stripMargin + // consumerId is the consumer these consents are then read with (getAuthed signs as testConsumer), + // because that is what the endpoint itself records -- createAccountAccessConsents passes the + // calling consumer straight into saveUKConsent. It used to pass None here, which produced a row + // no production path creates: one naming no lodging TPP. That was invisible while such a row was + // readable by anyone, and became a 403 the moment "records no TPP" stopped meaning "matches every + // caller". The fixture was the thing that was wrong. private def createRealConsent(): String = { val consent = Consents.consentProvider.vend.saveUKConsent( user = Some(resourceUser1), bankId = None, accountIds = None, - consumerId = None, + consumerId = Some(testConsumer.consumerId.get), permissions = consentPermissions, expirationDateTime = Some(DateWithDayFormat.parse("2030-01-01")), transactionFromDateTime = Some(DateWithDayFormat.parse("2020-01-01")), diff --git a/obp-api/src/test/scala/code/api/UKOpenBanking/v4_0_1/UKOpenBankingV401ConsentAccessTests.scala b/obp-api/src/test/scala/code/api/UKOpenBanking/v4_0_1/UKOpenBankingV401ConsentAccessTests.scala index 9e21e3dd4b..cc10f3892c 100644 --- a/obp-api/src/test/scala/code/api/UKOpenBanking/v4_0_1/UKOpenBankingV401ConsentAccessTests.scala +++ b/obp-api/src/test/scala/code/api/UKOpenBanking/v4_0_1/UKOpenBankingV401ConsentAccessTests.scala @@ -138,13 +138,35 @@ class UKOpenBankingV401ConsentAccessTests extends UKOpenBankingV401ServerSetup { } scenario("blank ids count as absent, not as a value to match", UKOpenBankingV401ConsentAccess) { - // A consent lodged before consumer binding existed stores no consumer id; nothing identifies a - // wrong caller, so it cannot be refused on that basis. - Consent.checkUKConsentAccess("", "", None, Some(tpp), callerIsScaFrontEnd = false) should equal(None) - Consent.checkUKConsentAccess(null, null, None, None, callerIsScaFrontEnd = false) should equal(None) - // A blank caller user id is not a PSU either -- it must not accidentally match a blank binding. + // A blank caller user id is not a PSU -- it must not accidentally match a blank binding. Consent.checkUKConsentAccess(psu, tpp, Some(" "), Some(tpp), callerIsScaFrontEnd = false) should equal(None) } + + // The two assertions that used to sit in the scenario above have inverted. They read + // checkUKConsentAccess("", "", None, Some(tpp), false) should equal(None) + // checkUKConsentAccess(null, null, None, None, false) should equal(None) + // on the reasoning that a consent lodged before consumer binding existed "cannot be refused on + // that basis" because nothing identifies a wrong caller. The other half of that is that nothing + // identifies a RIGHT one either, and the profile scopes these endpoints to "an + // account-access-consent resource that they have created" -- a row naming no creator matches no + // caller rather than every caller. 4 of 753 UK consents on a long-lived instance record no + // consumer, and until now any authenticated caller could read and revoke them. + scenario("a consent that records no lodging TPP belongs to nobody", UKOpenBankingV401ConsentAccess) { + Consent.checkUKConsentAccess("", "", None, Some(tpp), callerIsScaFrontEnd = false) should + equal(Some(ConsentDoesNotMatchConsumer)) + Consent.checkUKConsentAccess(null, null, None, None, callerIsScaFrontEnd = false) should + equal(Some(ConsentDoesNotMatchConsumer)) + } + + // Matching the PSU is necessary, not sufficient: the rule used to stop as soon as the two PSU + // ids agreed, so a second TPP holding a session for the same person reached a first TPP's + // consent. The UK case for the Consumer comparison is per-endpoint rather than blanket -- the + // Endpoints table marks GET and DELETE Client Credentials, so no PSU is party to them at all, + // and a PSU session here is an OBP extension that cannot be the thing that waives it. + scenario("a second TPP holding a session for the consent's own PSU is still refused", UKOpenBankingV401ConsentAccess) { + Consent.checkUKConsentAccess(psu, tpp, Some(psu), Some(otherTpp), callerIsScaFrontEnd = false) should + equal(Some(ConsentDoesNotMatchConsumer)) + } } // The rule above is only ever as good as the identity handed to it, and that is where this family diff --git a/obp-api/src/test/scala/code/api/berlin/group/v1_3/BerlinGroupV13ConsentAccessTests.scala b/obp-api/src/test/scala/code/api/berlin/group/v1_3/BerlinGroupV13ConsentAccessTests.scala index 43ffe95d84..5119e46007 100644 --- a/obp-api/src/test/scala/code/api/berlin/group/v1_3/BerlinGroupV13ConsentAccessTests.scala +++ b/obp-api/src/test/scala/code/api/berlin/group/v1_3/BerlinGroupV13ConsentAccessTests.scala @@ -87,11 +87,46 @@ class BerlinGroupV13ConsentAccessTests extends BerlinGroupConsentFixtures { equal(Some(ConsentDoesNotMatchUser)) } + // Matching the PSU is necessary, not sufficient. It used to be sufficient: the rule returned + // None the moment the two PSU ids agreed, without ever looking at the Consumer, so a second TPP + // holding a session for the same person could read and revoke the consent a first TPP lodged. + // One TPP's mandate over a consent is not another's -- the same principle the payment guard in + // Http4sBGv13PIS states, and IG 4.11's "may only apply to resources which have been created by + // the same TPP before" admits no PSU exception. + scenario("a second TPP holding a session for the consent's own PSU is still refused", BerlinGroupV13ConsentAccess) { + Consent.checkBerlinGroupConsentAccess(psu, tpp, Some(psu), Some(otherTpp), callerIsScaFrontEnd = false) should + equal(Some(ConsentDoesNotMatchConsumer)) + } + scenario("blank ids count as absent, not as a value to match", BerlinGroupV13ConsentAccess) { - Consent.checkBerlinGroupConsentAccess(null, null, None, None, callerIsScaFrontEnd = false) should equal(None) Consent.checkBerlinGroupConsentAccess(" ", tpp, Some(psu), Some(tpp), callerIsScaFrontEnd = false) should equal(None) Consent.checkBerlinGroupConsentAccess(psu, tpp, Some(" "), Some(tpp), callerIsScaFrontEnd = false) should equal(None) } + + // This assertion is inverted from what it used to say. It read + // checkBerlinGroupConsentAccess(null, null, None, None, false) should equal(None) + // which pinned a consent recording no TPP as addressable by anybody -- "absent" was being read + // as "matches everything" rather than as "there is nobody this belongs to". Neither standard + // supports that: IG 4.11 scopes a resource to the TPP that created it, and UK scopes GET and + // DELETE to "an account-access-consent resource that they have created". A row naming no TPP + // satisfies neither, so it belongs to nobody and is refused. + // + // It is a real population, not a hypothetical: 10 of 566 Berlin Group consents and 4 of 753 UK + // consents record no consumer on a long-lived instance. They are already unreachable in Berlin + // Group today, by accident -- the hand-rolled `null == "None"` compare in the five reads is + // false for everyone -- so refusing them here changes nothing for those callers and only stops + // the UK pair, and the reads once they move onto this rule, from opening up. + scenario("a consent that records no lodging TPP belongs to nobody", BerlinGroupV13ConsentAccess) { + Consent.checkBerlinGroupConsentAccess(null, null, None, None, callerIsScaFrontEnd = false) should + equal(Some(ConsentDoesNotMatchConsumer)) + Consent.checkBerlinGroupConsentAccess(null, " ", Some(psu), Some(tpp), callerIsScaFrontEnd = false) should + equal(Some(ConsentDoesNotMatchConsumer)) + } + + scenario("an operator can restore the old behaviour for a migration window", BerlinGroupV13ConsentAccess) { + setPropsValues("consent_allow_legacy_unrecorded_tpp" -> "true") + Consent.checkBerlinGroupConsentAccess(null, null, None, None, callerIsScaFrontEnd = false) should equal(None) + } } // The Redirect approach: the PSU authenticates at the ASPSP, so these calls arrive from the From 2e619acbea1b285857a8ebf8dbdeaf6ecd571ae2 Mon Sep 17 00:00:00 2001 From: Hongwei Date: Wed, 12 Aug 2026 23:30:57 +0200 Subject: [PATCH 3/5] refactor: the Berlin Group consent reads apply the shared ownership rule Five endpoints that read or delete a consent resource each compared the lodging Consumer by hand -- deleteConsent, getConsentAuthorisation, getConsentInformation, getConsentScaStatus and getConsentStatus -- rather than going through checkBerlinGroupConsentAccess like the authorisation pair in the same file. The cost of five copies showed up immediately: the previous commit tightened the shared rule on two axes, and none of it reached these call sites. A consent bound to one PSU stayed readable by the lodging TPP acting for a different one, because a hand-rolled Consumer compare cannot notice that a PSU half exists at all. That is what the new scenario pins, on all five sites at once. They also gain the declared-SCA-front-end exemption, which the reads had no way to express. Under the Redirect approach the PSU authenticates at the ASPSP, so these calls arrive from the ASPSP's own screen rather than the lodging TPP -- and Implementation Guidelines 6.3.3 says the consent read exists precisely for that case, "returning the data for the TPP especially in cases, where the consent was directly managed between ASPSP and PSU e.g. in a re-direct SCA Approach". A front end that may start an authorisation could not read the status of the one it started. The answer these endpoints give is unchanged: ConsentNotFound at 403 whatever the reason, so a caller cannot tell "not yours" from "no such consent". The rule's own more specific reason is logged rather than returned. Whether that is the right code at all is a separate question, decided once for both standards in a later commit rather than drifting here. --- .../berlin/group/v1_3/Http4sBGv13AIS.scala | 24 ++-------- .../scala/code/api/util/ConsentUtil.scala | 31 ++++++++++++ .../BerlinGroupV13ConsentAccessTests.scala | 48 +++++++++++++++++++ 3 files changed, 84 insertions(+), 19 deletions(-) diff --git a/obp-api/src/main/scala/code/api/berlin/group/v1_3/Http4sBGv13AIS.scala b/obp-api/src/main/scala/code/api/berlin/group/v1_3/Http4sBGv13AIS.scala index b7bc9acf82..7cb6e6c71a 100644 --- a/obp-api/src/main/scala/code/api/berlin/group/v1_3/Http4sBGv13AIS.scala +++ b/obp-api/src/main/scala/code/api/berlin/group/v1_3/Http4sBGv13AIS.scala @@ -163,11 +163,7 @@ object Http4sBGv13AIS extends MdcLoggable { consent <- Future(Consents.consentProvider.vend.getConsentByConsentId(consentId)) map { unboxFullOrFail(_, callContext, ConsentNotFound, 403) } - consumerIdFromConsent = consent.mConsumerId.get - consumerIdFromCurrentCall = callContext.map(_.consumer.map(_.consumerId.get).getOrElse("None")).getOrElse("None") - _ <- booleanToFuture(failMsg = ConsentNotFound, failCode = 403, cc = callContext) { - consumerIdFromConsent == consumerIdFromCurrentCall - } + _ <- Consent.assertBerlinGroupConsentReadAccess(consent.userId, consent.consumerId, cc) _ <- Future(Consents.consentProvider.vend.revokeBerlinGroupConsent(consentId)) map { i => connectorEmptyResponse(i, callContext) } @@ -324,9 +320,7 @@ object Http4sBGv13AIS extends MdcLoggable { consent <- Future(Consents.consentProvider.vend.getConsentByConsentId(consentId)) map { unboxFullOrFail(_, callContext, s"$ConsentNotFound ($consentId)") } - _ <- booleanToFuture(failMsg = ConsentNotFound, failCode = 403, cc = callContext) { - consent.mConsumerId.get == cc.consumer.map(_.consumerId.get).getOrElse("None") - } + _ <- Consent.assertBerlinGroupConsentReadAccess(consent.userId, consent.consumerId, cc) (challenges, callContext) <- NewStyle.function.getChallengesByConsentId(consentId, callContext) } yield { JSONFactory_BERLIN_GROUP_1_3.AuthorisationJsonV13(challenges.map(_.challengeId)) @@ -344,11 +338,7 @@ object Http4sBGv13AIS extends MdcLoggable { consent <- Future(Consents.consentProvider.vend.getConsentByConsentId(consentId)) map { unboxFullOrFail(_, callContext, s"$ConsentNotFound ($consentId)") } - consumerIdFromConsent = consent.mConsumerId.get - consumerIdFromCurrentCall = callContext.map(_.consumer.map(_.consumerId.get).getOrElse("None")).getOrElse("None") - _ <- booleanToFuture(failMsg = ConsentNotFound, failCode = 403, cc = callContext) { - consumerIdFromConsent == consumerIdFromCurrentCall - } + _ <- Consent.assertBerlinGroupConsentReadAccess(consent.userId, consent.consumerId, cc) } yield { createGetConsentResponseJson(consent) } @@ -368,9 +358,7 @@ object Http4sBGv13AIS extends MdcLoggable { consent <- Future(Consents.consentProvider.vend.getConsentByConsentId(consentId)) map { unboxFullOrFail(_, callContext, s"$ConsentNotFound ($consentId)", 403) } - _ <- booleanToFuture(failMsg = ConsentNotFound, failCode = 403, cc = callContext) { - consent.mConsumerId.get == cc.consumer.map(_.consumerId.get).getOrElse("None") - } + _ <- Consent.assertBerlinGroupConsentReadAccess(consent.userId, consent.consumerId, cc) (challenges, callContext) <- NewStyle.function.getChallengesByConsentId(consentId, callContext) } yield { val challengeStatus = challenges.filter(_.challengeId == authorisationId) @@ -393,9 +381,7 @@ object Http4sBGv13AIS extends MdcLoggable { // Same ownership test as the three sibling reads of this consent. Without it the status of // any consent was readable by any AISP holding its id -- which is enough to confirm the // consent exists and to watch it move through authorisation. - _ <- booleanToFuture(failMsg = ConsentNotFound, failCode = 403, cc = callContext) { - consent.mConsumerId.get == cc.consumer.map(_.consumerId.get).getOrElse("None") - } + _ <- Consent.assertBerlinGroupConsentReadAccess(consent.userId, consent.consumerId, cc) } yield { JSONFactory_BERLIN_GROUP_1_3.ConsentStatusJsonV13(consent.status) } diff --git a/obp-api/src/main/scala/code/api/util/ConsentUtil.scala b/obp-api/src/main/scala/code/api/util/ConsentUtil.scala index beb427738e..4d18800eee 100644 --- a/obp-api/src/main/scala/code/api/util/ConsentUtil.scala +++ b/obp-api/src/main/scala/code/api/util/ConsentUtil.scala @@ -1755,6 +1755,37 @@ object Consent extends MdcLoggable { } } + /** + * checkBerlinGroupConsentAccess for the five endpoints that read or delete a consent resource, + * refusing with the answer those endpoints have always given. + * + * They answer `ConsentNotFound` at 403 whatever the reason, which is deliberate: a caller who is + * not entitled to a consent should not be able to tell "it is not yours" from "there is no such + * consent", or the endpoint becomes a way to confirm that an id exists. The rule's own + * ConsentDoesNotMatchUser / ConsentDoesNotMatchConsumer say which, so the reason is logged rather + * than returned. The authorisation pair does return it -- there the caller is mid-ceremony on a + * consent it has already been handed, and needs to know why it was stopped. + * + * Not a general-purpose wrapper: assertUKConsentAccess next to it passes the reason through, and + * that difference is the point rather than an inconsistency to iron out. + */ + def assertBerlinGroupConsentReadAccess( + consentUserId: String, + consentConsumerId: String, + callContext: CallContext + ): Future[Box[Unit]] = { + val refusal = checkBerlinGroupConsentAccess( + consentUserId, consentConsumerId, + genuinePsu(callContext).map(_.userId), callContext.consumer.map(_.consumerId.get), + isScaFrontEnd(callContext.consumer.map(_.consumerId.get))) + refusal.foreach { reason => + logger.info( + s"A consent read was refused: $reason. Reported as ${ErrorMessages.ConsentNotFound} so the " + + s"caller cannot tell a consent that is not theirs from one that does not exist.") + } + Helper.booleanToFuture(ErrorMessages.ConsentNotFound, 403, Some(callContext))(refusal.isEmpty) + } + /** * Whether the Consumer making this call is one the ASPSP declared as its own SCA front end. * diff --git a/obp-api/src/test/scala/code/api/berlin/group/v1_3/BerlinGroupV13ConsentAccessTests.scala b/obp-api/src/test/scala/code/api/berlin/group/v1_3/BerlinGroupV13ConsentAccessTests.scala index 5119e46007..901cdbab1b 100644 --- a/obp-api/src/test/scala/code/api/berlin/group/v1_3/BerlinGroupV13ConsentAccessTests.scala +++ b/obp-api/src/test/scala/code/api/berlin/group/v1_3/BerlinGroupV13ConsentAccessTests.scala @@ -294,6 +294,54 @@ class BerlinGroupV13ConsentAccessTests extends BerlinGroupConsentFixtures { private def psuIdHeader(userName: String) = List(("PSU-ID", userName)) + // The five reads of a consent resource. They each used to compare + // consent.mConsumerId.get == cc.consumer.map(_.consumerId.get).getOrElse("None") + // by hand, five separate times, rather than going through checkBerlinGroupConsentAccess like the + // authorisation pair above. Five copies of one security decision is how the two fail-opens fixed + // in the previous commit came to be fixed in the rule and not at these call sites: a hand-rolled + // Consumer compare cannot notice that the PSU half exists at all. + private def consentReads(consentId: String, session: Option[(Consumer, Token)]) = List( + "read the consent" -> makeGetRequest((V1_3_BG / "consents" / consentId).GET <@ (session)), + "read its status" -> makeGetRequest((V1_3_BG / "consents" / consentId / "status").GET <@ (session)), + "list its authorisations" -> makeGetRequest((V1_3_BG / "consents" / consentId / "authorisations").GET <@ (session)), + "read an authorisation's SCA status" -> + makeGetRequest((V1_3_BG / "consents" / consentId / "authorisations" / "any-authorisation-id").GET <@ (session)) + ) + + feature("BG v1.3 - the consent reads apply the same ownership rule as the authorisation pair") { + + scenario("the lodging TPP acting for a second PSU cannot read a consent bound to the first", BerlinGroupV13ConsentAccess) { + val consentId = createUnclaimedBerlinGroupConsent().consentId + Consents.consentProvider.vend.updateConsentUser(consentId, resourceUser1) + + Then("the same TPP, but acting for a different person, is refused on every read") + // The Consumer matches -- this IS the TPP that lodged it -- so a Consumer-only compare says + // yes. The consent is bound to resourceUser1 and the session is resourceUser2's, and once a + // consent names a PSU the rule requires that PSU, which is what the authorisation pair has + // enforced all along and these reads did not. + consentReads(consentId, secondPsuOfTestConsumerSession).foreach { case (what, response) => + withClue(s"a second PSU of the lodging TPP was allowed to $what: ") { + response.code should equal(403) + } + } + + And("the lodging TPP on a client-credentials session still can") + // The normal Berlin Group caller: no genuine PSU in the session at all, so the PSU half does + // not apply and the Consumer decides. This is the half that must not regress. + consentReads(consentId, clientCredentialsSession).foreach { case (what, response) => + withClue(s"the lodging TPP could not $what: ") { + response.code should equal(200) + } + } + + And("so does DELETE, which is the fifth site and mutates, hence last") + makeDeleteRequest((V1_3_BG / "consents" / consentId).DELETE <@ (secondPsuOfTestConsumerSession)) + .code should equal(403) + makeDeleteRequest((V1_3_BG / "consents" / consentId).DELETE <@ (clientCredentialsSession)) + .code should equal(204) + } + } + feature("BG v1.3 - a consent's authorisation sub-resources answer only to the TPP that lodged it") { scenario("A second TPP cannot start an authorisation on a consent it did not lodge", BerlinGroupV13ConsentAccess) { From 3913c4a213c1cd62eb38188403d2317bcead0dcc Mon Sep 17 00:00:00 2001 From: Hongwei Date: Wed, 12 Aug 2026 23:48:52 +0200 Subject: [PATCH 4/5] refactor: remove the consumer-scoped grant API, which no caller could use safely grantAccessToSystemViewForConsumer and grantAccessToCustomViewForConsumer have had no callers since the consent flows stopped using them, and could not have gained one without reintroducing the bug that removed them. Every consent grant is written under Constant.ALL_CONSUMERS, and that is a literal rather than a wildcard: revokeConsentAccountAccess sweeps a revoked consent's rows by asking accessGrantedToUserForConsumer for exactly ALL_CONSUMERS, which matches the column with By(AccountAccess.consumer_id, ...). A row written under a real consumer id is therefore invisible to the sweep, and the access outlives the consent that granted it. Their own comment recommended the opposite -- "keeping it on its own row is what stops a second TPP's consent from rewriting it" -- while the hazard was already documented on grantConsentAccountAccess a few hundred lines away. Two descriptions of the same column, and the wrong one sat on the API a caller would reach for. The remaining ...ForConsumer methods stay: revokeAccessToViewForUserAndConsumer and accessGrantedToUserForConsumer are what the consent flows use, both against ALL_CONSUMERS. The trait comment now says why the pair is absent, so it reads as a decision rather than a gap for someone to fill in. --- .../main/scala/code/views/MapperViews.scala | 26 ------------------- obp-api/src/main/scala/code/views/Views.scala | 14 ++++++---- 2 files changed, 9 insertions(+), 31 deletions(-) diff --git a/obp-api/src/main/scala/code/views/MapperViews.scala b/obp-api/src/main/scala/code/views/MapperViews.scala index f89d18ba14..6a25480a45 100644 --- a/obp-api/src/main/scala/code/views/MapperViews.scala +++ b/obp-api/src/main/scala/code/views/MapperViews.scala @@ -196,32 +196,6 @@ object MapperViews extends Views with MdcLoggable { } } - // The consumer-scoped counterparts of the two grants above, mirroring the revoke...ForConsumer - // pair further down. Access granted through one application's consent belongs to that application - // alone: keeping it on its own row is what stops a second TPP's consent from rewriting it, and - // what makes narrowing a consent narrow only that consent. - def grantAccessToSystemViewForConsumer(bankId: BankId, accountId: AccountId, view: View, user: User, consumerId: String): Box[View] = { - { view.isPublic && !allowPublicViews } match { - case true => Failure(PublicViewsNotAllowedOnThisInstance) - case false => getOrGrantAccessToViewCommon(user, view, bankId.value, accountId.value, consumerId) - } - } - - def grantAccessToCustomViewForConsumer(bankIdAccountIdViewId: BankIdAccountIdViewId, user: User, consumerId: String): Box[View] = { - val viewDefinition = ViewDefinition.findCustomView( - bankIdAccountIdViewId.bankId.value, - bankIdAccountIdViewId.accountId.value, - bankIdAccountIdViewId.viewId.value) - - viewDefinition match { - case Full(v) => - if (v.isPublic && !allowPublicViews) Failure(PublicViewsNotAllowedOnThisInstance) - else getOrGrantAccessToViewCommon(user, v, bankIdAccountIdViewId.bankId.value, bankIdAccountIdViewId.accountId.value, consumerId) - case _ => - Empty ~> APIFailure(s"View ${bankIdAccountIdViewId.viewId} not found", 404) - } - } - def grantAccessToMultipleViews(views: List[BankIdAccountIdViewId], user: User, callContext: Option[CallContext]): Box[List[View]] = { val viewDefinitions: List[(ViewDefinition, BankIdAccountIdViewId)] = views.map { uid => ViewDefinition.findCustomView(uid.bankId.value,uid.accountId.value, uid.viewId.value).map((_, uid)) diff --git a/obp-api/src/main/scala/code/views/Views.scala b/obp-api/src/main/scala/code/views/Views.scala index a617eabf04..ac59c49e78 100644 --- a/obp-api/src/main/scala/code/views/Views.scala +++ b/obp-api/src/main/scala/code/views/Views.scala @@ -36,11 +36,15 @@ trait Views { def revokeAccessToSystemViewForConsumer(bankId: BankId, accountId: AccountId, view : View, consumerId : String) : Box[Boolean] def revokeAccessToCustomViewForConsumer(view : View, consumerId : String) : Box[Boolean] - // Grant/revoke a single application's access, rather than access shared by every application. - // Used by the consent flows: what one TPP's consent grants is that TPP's alone, so narrowing or - // re-authorising it must not touch another TPP's grants on the same account. - def grantAccessToSystemViewForConsumer(bankId: BankId, accountId: AccountId, view : View, user : User, consumerId : String) : Box[View] - def grantAccessToCustomViewForConsumer(bankIdAccountIdViewId : BankIdAccountIdViewId, user : User, consumerId : String) : Box[View] + // Revoke and enumerate one application's access, rather than access shared by every application. + // The consent flows use these against Constant.ALL_CONSUMERS, which is the literal every consent + // grant is written under -- not a wildcard, and every lookup matches it by equality. + // + // There is deliberately no grant...ForConsumer counterpart. Two existed and had no callers, and + // could not have gained one safely: a row written under a real consumer id is invisible to + // revokeConsentAccountAccess, which sweeps by asking for exactly ALL_CONSUMERS, so the access + // would outlive the consent that granted it. Grants go through grantAccessToSystemView / + // grantAccessToCustomView, which write ALL_CONSUMERS and are therefore revocable. def revokeAccessToViewForUserAndConsumer(bankIdAccountIdViewId : BankIdAccountIdViewId, user : User, consumerId : String) : Box[Boolean] // Everything a user currently holds under one application's scope. The consent flows reconcile // against this: a consent's granted views are whatever its JWT says, so the rows that back it are From 93e125b4c6071764910990457854936a6e8c5dbe Mon Sep 17 00:00:00 2001 From: Hongwei Date: Thu, 13 Aug 2026 00:11:00 +0200 Subject: [PATCH 5/5] fix: a revoke that cannot give the access back still reports the revoke MappedConsent.revoke calls revokeConsentAccountAccess after conditionalRevoke has already committed the status change, and it was not guarded. A throw there could not undo the revoke -- it could only hide that it had happened. The caller got a 500, and the retry got ConsentAlreadyRevoked, so an AISP was left unable to complete the DELETE both standards require of it, with no correct next step. Reaching it takes less than it sounds. revokeConsentAccountAccess reads the stored JWT through Box.map, which does not catch, and getSignedPayloadAsJson does not verify the signature -- it parses the structure and hands back the claims -- so a payload that no longer extracts to ConsentJWT arrives as a MappingException. The same trap is already documented on applyUKConsentPrincipalFromToken. Wrapped in tryo, matching the Berlin Group sibling twenty lines below, and logged at error rather than swallowed: the consent IS revoked and the API is right to say so, but rows that should have been dropped are still live, and that is the one thing about a revoked consent nobody comes back for. --- .../scala/code/consent/MappedConsent.scala | 20 +++++++++++- ...UKOpenBankingV401ConsentScopingTests.scala | 31 +++++++++++++++++++ 2 files changed, 50 insertions(+), 1 deletion(-) diff --git a/obp-api/src/main/scala/code/consent/MappedConsent.scala b/obp-api/src/main/scala/code/consent/MappedConsent.scala index c281d3103c..d660f1b46f 100644 --- a/obp-api/src/main/scala/code/consent/MappedConsent.scala +++ b/obp-api/src/main/scala/code/consent/MappedConsent.scala @@ -377,7 +377,25 @@ object MappedConsentProvider extends ConsentProvider with code.util.Helper.MdcLo // Every revoke endpoint funnels through here, so this is the one place that has to give // the granted access back. The status flip alone leaves the AccountAccess rows live in // the table for anything that reads them without asking the consent first. - code.api.util.Consent.revokeConsentAccountAccess(consent) + // + // Guarded because conditionalRevoke above has already committed, so a throw here cannot + // undo the revoke -- it can only hide that it happened. The caller got a 500 and the + // retry got ConsentAlreadyRevoked, which leaves an AISP unable to complete the DELETE the + // standards require of it and with no correct next step. It does not take much to get + // there: revokeConsentAccountAccess extracts the stored JWT through Box.map, which does + // not catch, so a payload that no longer matches ConsentJWT arrives as a MappingException. + // + // Logged at error, not swallowed. The consent IS revoked and the API is right to say so, + // but rows that should have been dropped are still live, and that is the one thing about + // a revoked consent nobody comes back for. Same shape as the sibling below. + tryo(code.api.util.Consent.revokeConsentAccountAccess(consent)) match { + case Failure(msg, ex, _) => + logger.error( + s"revoke: consent $consentId is revoked, but its account access could not be given " + + s"back and those rows are still live: $msg" + + ex.map(e => s" (${e.getClass.getSimpleName}: ${e.getMessage})").getOrElse("")) + case _ => + } MappedConsent.find(By(MappedConsent.mConsentId, consentId)) } else Failure(ErrorMessages.ConsentAlreadyRevoked) diff --git a/obp-api/src/test/scala/code/api/UKOpenBanking/v4_0_1/UKOpenBankingV401ConsentScopingTests.scala b/obp-api/src/test/scala/code/api/UKOpenBanking/v4_0_1/UKOpenBankingV401ConsentScopingTests.scala index ef80b4f856..5d4fc7ad19 100644 --- a/obp-api/src/test/scala/code/api/UKOpenBanking/v4_0_1/UKOpenBankingV401ConsentScopingTests.scala +++ b/obp-api/src/test/scala/code/api/UKOpenBanking/v4_0_1/UKOpenBankingV401ConsentScopingTests.scala @@ -417,6 +417,37 @@ class UKOpenBankingV401ConsentScopingTests extends UKOpenBankingV401ServerSetup // Revocation used to flip a status column and leave the granted rows in the table for good. Views.views.vend.accessGrantedToUserForConsumer(principal, Constant.ALL_CONSUMERS) shouldBe empty } + + // The clean-up runs after conditionalRevoke has already committed the status change, so a throw + // in it does not undo the revoke -- it only hides that the revoke happened. The caller gets a + // 500 and the retry gets ConsentAlreadyRevoked, which leaves an AISP unable to complete the + // DELETE the profile requires of it, with no correct next step. The Berlin Group sibling a few + // lines away in MappedConsent already wraps its equivalent; this one did not. + // + // A stored JWT whose payload does not extract is enough to get there. getSignedPayloadAsJson + // does not verify the signature -- it parses the structure and hands back the claims -- so the + // extract that follows is what throws, and Box.map does not catch. The same trap is already + // documented on applyUKConsentPrincipalFromToken. + scenario("a consent whose stored JWT cannot be read is still revoked, and says so", UKConsentScoping) { + val consentId = authoriseConsentFor(testConsumer.consumerId.get, List(ReadAccountsBasic)) + + // Structurally a JWT, and the claims parse as JSON -- they are simply not a ConsentJWT. + def b64(s: String) = java.util.Base64.getUrlEncoder.withoutPadding.encodeToString(s.getBytes("UTF-8")) + Consents.consentProvider.vend.setJsonWebToken( + consentId, s"${b64("""{"alg":"HS256"}""")}.${b64("""{"not":"a consent"}""")}.signature-is-never-checked") + + val revoked = Consents.consentProvider.vend.revoke(consentId) + + withClue("the revoke threw rather than reporting the outcome: ") { + revoked.isDefined should equal(true) + } + Consents.consentProvider.vend.getConsentByConsentId(consentId) + .map(_.status) should equal(Full(ConsentStatus.REVOKED.toString)) + + // And it stays revoked: a second attempt is the ConsentAlreadyRevoked the caller used to be + // pushed into by the failure, rather than a way out of it. + Consents.consentProvider.vend.revoke(consentId).isDefined should equal(false) + } } }