From a8ff0325e04ca29ed2e2853ef8f92ac4d3b4c83d Mon Sep 17 00:00:00 2001 From: Hongwei Date: Thu, 13 Aug 2026 00:34:56 +0200 Subject: [PATCH 1/3] fix: a UK access token may only exercise a consent belonging to its own subject applyUKConsentPrincipalFromToken took the PSU from the token and never compared it to the consent. So a token belonging to anyone, carrying a consent_id that is not theirs, was swapped onto that consent's shadow user -- and that principal holds the consent's account access. `consenter` was set to the token's holder rather than to the person the consent is about. checkUKConsent did catch it, comparing the consent's mUserId against `consenter` and finding them different. But only the UK read endpoints call checkUKConsent: on every other endpoint family the swapped principal stood. The decision belongs where the swap happens, which runs for every request and whose refusal ResourceDocMiddleware enforces everywhere. So the PSU now comes off the consent row, as it already does on the header path in applyUKRules, and the token's subject must be that PSU. Both halves are load-bearing and the second is easy to lose: deriving `consenter` from mUserId without also comparing the token would leave checkUKConsent comparing the consent's user with itself, passing for anybody. The comparison moves here rather than going away, and ukTokenPathPsuId states it on its own so it can be tested without standing up a request. A consent naming no PSU is refused rather than falling back to the token's holder: it was never authorised, so there is nobody it is on behalf of, and that is a reason to serve nothing rather than everything that holder can see. Both halves are mutation-checked. Removing the comparison fails the unit scenario and the integration one, and the integration failure is the interesting one -- it shows the principal being swapped onto a consent the caller has nothing to do with. --- .../scala/code/api/util/ConsentUtil.scala | 43 ++++++++++++-- ...UKOpenBankingV401ConsentScopingTests.scala | 58 +++++++++++++++++++ 2 files changed, 96 insertions(+), 5 deletions(-) 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 4d18800eee..848ee97616 100644 --- a/obp-api/src/main/scala/code/api/util/ConsentUtil.scala +++ b/obp-api/src/main/scala/code/api/util/ConsentUtil.scala @@ -1078,6 +1078,33 @@ object Consent extends MdcLoggable { * checkUKConsent still applies everything it did before as well: wrong standard, revoked, expired, * bound to a different PSU, held by a different consumer. */ + /** + * Which PSU a UK consent named on an access token runs for, or the reason to refuse. + * + * The PSU comes off the consent row, as it does on the header path in applyUKRules -- the consent + * is the record of whose data this is, and the token is a claim about it. The token's subject then + * has to BE that PSU, which is the substance of the profile binding a token to one consent and one + * person: the intent id is carried into the request object so that "the access token that is + * eventually generated [is] bound to a specific consent". + * + * Both halves matter and one of them is easy to lose. Deriving the PSU from the consent and + * dropping the comparison would make checkUKConsent's own check -- mUserId against consenter -- + * compare the consent's user with itself and pass for anybody's token. So the comparison moves + * here rather than going away, and moving it is the gain: this runs on every request and its + * refusal is enforced by ResourceDocMiddleware, whereas checkUKConsent runs only on the UK read + * endpoints. + * + * A consent naming no PSU is refused rather than falling back to the token's user. Such a consent + * was never authorised, so there is nobody it is on behalf of, and "we cannot tell whose this is" + * is a reason to serve nothing. + */ + def ukTokenPathPsuId(consentUserId: String, tokenUserId: String): Either[String, String] = + present(consentUserId) match { + case None => Left(ErrorMessages.ConsentNotFound) + case Some(psuId) if psuId != tokenUserId => Left(ErrorMessages.ConsentDoesNotMatchUser) + case Some(psuId) => Right(psuId) + } + def applyUKConsentPrincipalFromToken(user: Box[User], callContext: Option[CallContext]): (Box[User], Option[CallContext]) = { // Whether this request is exercising a UK consent at all. None at any step means it is not -- @@ -1100,23 +1127,29 @@ object Consent extends MdcLoggable { // "carry on as the PSU": that serves everything the PSU can see, which is the whole of what the // consent exists to narrow. ConsentNotFound for an unreadable JWT matches what applyUKRules // answers on the header path for the same row. - def resolve(psu: User, storedConsent: MappedConsent): Box[User] = for { + // Returns the principal to run as and the PSU it is running on behalf of, in that order. + def resolve(tokenUser: User, storedConsent: MappedConsent): Box[(User, User)] = for { consentJwt <- { implicit val dateFormats = CustomJsonFormats.formats JwtUtil.getSignedPayloadAsJson(storedConsent.jsonWebToken).map(parse(_).extract[ConsentJWT]) } ?~! ErrorMessages.ConsentNotFound + psuId <- ukTokenPathPsuId(storedConsent.userId, tokenUser.userId) match { + case Right(id) => Full(id) + case Left(reason) => Failure(reason) + } + psu <- Users.users.vend.getUserByUserId(psuId) ?~! ErrorMessages.ConsentNotFound principal <- resolveUKConsentPrincipal(storedConsent, consentJwt, psu) - } yield principal + } yield (principal, psu) namedConsent match { case None => (user, callContext) - case Some((cc, psu, storedConsent)) => + case Some((cc, tokenUser, storedConsent)) => // A throw is as unresolved as a Failure, and for the same reason it must not fall through: // the fallback is the PSU. Box.map does not catch, so an unextractable ConsentJWT arrives // here as a MappingException rather than as a Failure. - val outcome = Try(resolve(psu, storedConsent)) + val outcome = Try(resolve(tokenUser, storedConsent)) outcome match { - case Success(Full(principal)) => + case Success(Full((principal, psu))) => (Full(principal), Some(cc.copy( user = Full(principal), consenter = Full(psu), 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 5d4fc7ad19..f7a9e4152d 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 @@ -266,6 +266,35 @@ class UKOpenBankingV401ConsentScopingTests extends UKOpenBankingV401ServerSetup principal.map(_.userId) should equal(Full(resourceUser1.userId)) callContext.flatMap(_.consenter.toOption) should equal(None) } + + // The PSU used to be taken from the token rather than from the consent, and nothing here + // compared the two. So a token belonging to anyone at all, carrying a consent_id that is not + // theirs, was swapped onto that consent's shadow user -- with `consenter` set to the token's + // holder rather than to the person the consent is about. + // + // checkUKConsent then caught it, because it compares the consent's mUserId against `consenter` + // and they disagreed. But only the UK read endpoints call checkUKConsent: on every other + // endpoint family the swapped principal stood, and it carries the consent's account access. The + // refusal has to be decided here, where it is recorded on the CallContext and enforced for every + // endpoint by ResourceDocMiddleware. + scenario("a token whose subject is not the consent's PSU is refused, not swapped", UKConsentScoping) { + val consentId = authoriseConsentFor(testConsumer.consumerId.get, List(ReadAccountsBasic), + accountIds = List(acc)) + + Given("resourceUser2's session presenting a consent authorised by resourceUser1") + val (principal, callContext) = + Consent.applyUKConsentPrincipalFromToken(Full(resourceUser2), Some(bearerContextFor(consentId, testConsumer))) + val cc = callContext.getOrElse(fail("token path dropped the CallContext")) + + Then("the principal is left alone rather than swapped onto the consent") + principal.map(_.userId) should equal(Full(resourceUser2.userId)) + cc.consenter.toOption should equal(None) + + And("the refusal is recorded, so every endpoint family enforces it and not just the UK reads") + cc.ukConsentUnresolved should equal(Some(ErrorMessages.ConsentDoesNotMatchUser)) + Consent.unresolvedUKConsentRefusal(cc.ukConsentUnresolved, "/obp/v5.1.0/my/accounts") should + equal(Some(ErrorMessages.ConsentDoesNotMatchUser)) + } } /** @@ -383,6 +412,35 @@ class UKOpenBankingV401ConsentScopingTests extends UKOpenBankingV401ServerSetup None, "/open-banking/v4.0.1/aisp/account-access-consents") should equal(None) } + /** + * Which PSU the token path runs for. Extracted for the same reason as the rule above: the + * decision is worth pinning and the path that uses it cannot be driven from this suite. + * + * The consent row is the source of truth, as it already is on the header path, and the token's + * subject must agree with it. Losing the second half is the easy mistake, and it is silent: + * checkUKConsent compares the consent's mUserId against `consenter`, so deriving `consenter` + * from mUserId without also comparing the token would have that check compare the consent's user + * with itself and pass for anybody's token. + */ + scenario("the token path takes its PSU from the consent, and the token must agree", UKConsentScoping) { + val psu = "the-psu-user-id" + val someoneElse = "a-different-user-id" + + Given("a consent bound to a PSU") + Then("a token for that PSU resolves to them") + Consent.ukTokenPathPsuId(psu, psu) should equal(Right(psu)) + + And("a token for anyone else is refused, which is the check that must not be lost") + Consent.ukTokenPathPsuId(psu, someoneElse) should equal(Left(ErrorMessages.ConsentDoesNotMatchUser)) + + And("a consent naming no PSU was never authorised, so there is nobody it is on behalf of") + // Not a fallback to the token's user: that would serve everything that user can see, which is + // the whole of what a consent exists to narrow. + Consent.ukTokenPathPsuId("", psu) should equal(Left(ErrorMessages.ConsentNotFound)) + Consent.ukTokenPathPsuId(null, psu) should equal(Left(ErrorMessages.ConsentNotFound)) + Consent.ukTokenPathPsuId(" ", psu) should equal(Left(ErrorMessages.ConsentNotFound)) + } + scenario("the consent stays inspectable and revocable by the TPP that lodged it", UKConsentScoping) { val consentId = unresolvableConsent(List(ReadAccountsBasic), bindAccounts = false) val (principal, cc) = swapFor(consentId) From 738085d2a0f3b0591dc8c2c5bdfae064bb86674b Mon Sep 17 00:00:00 2001 From: Hongwei Date: Thu, 13 Aug 2026 01:02:20 +0200 Subject: [PATCH 2/3] fix: a consent that is not yours and one that does not exist answer the same The UK consent GET and DELETE told the two apart. A consent id matching nothing answered 400 and spelled the id back in the message; one that existed but belonged to somebody else answered 403 with the reason. That is enough to walk a list of ids and learn which are real, and the id echo confirms it a second time. Berlin Group's equivalents already answered ConsentNotFound either way; two of its five sites had drifted to the UK shape, and those move back. Both now answer 403 ConsentNotFound, with no id, on UK v3.1 and v4.0.1 and on all five Berlin Group consent reads. The specific reason -- ConsentDoesNotMatchUser or ConsentDoesNotMatchConsumer -- is logged rather than returned. 403 rather than 400 because that is what Berlin Group's three untouched sites already answered, so it is the smaller move and it leaves the two standards saying the same thing. Neither standard prescribes a code for these two cases. Six existing assertions change wording. None of them changes what it was protecting: the IDOR regressions still assert 403 and still assert the consent was left untouched, and only the message they match on is now the generic one. Each says so at the assertion. Deliberately not included: a consent the TPP has DELETEd should answer 400 on subsequent reads (read-write-data-api-profile, "Changes to an Intent's Authorized State"), and it currently answers 200 with Status CANC. Fixing that needs OBP to tell a TPP-initiated DELETE from a PSU cancelling through the ASPSP -- the profile wants 400 for the first and 200 with CANC for the second -- and both currently write the same REVOKED status through the same MappedConsent.revoke. Recording the origin is a schema change and its own piece of work. --- .../v3_1_0/Http4sUKOBv310AccountAccess.scala | 4 +- .../v4_0_1/Http4sUKOBv401AccountInfo.scala | 4 +- .../berlin/group/v1_3/Http4sBGv13AIS.scala | 4 +- .../scala/code/api/util/ConsentUtil.scala | 15 ++++- .../v3_1_0/UKOpenBankingV310AisTests.scala | 23 ++++++-- .../UKOpenBankingV401AccountInfoTests.scala | 57 ++++++++++++++++--- 6 files changed, 83 insertions(+), 24 deletions(-) diff --git a/obp-api/src/main/scala/code/api/UKOpenBanking/v3_1_0/Http4sUKOBv310AccountAccess.scala b/obp-api/src/main/scala/code/api/UKOpenBanking/v3_1_0/Http4sUKOBv310AccountAccess.scala index 225686d143..0a876e0556 100644 --- a/obp-api/src/main/scala/code/api/UKOpenBanking/v3_1_0/Http4sUKOBv310AccountAccess.scala +++ b/obp-api/src/main/scala/code/api/UKOpenBanking/v3_1_0/Http4sUKOBv310AccountAccess.scala @@ -188,7 +188,7 @@ object Http4sUKOBv310AccountAccess extends MdcLoggable { for { _ <- passesPsd2Aisp(Some(cc)) consent <- Future(Consents.consentProvider.vend.getConsentByConsentId(consentId)) map { - unboxFullOrFail(_, Some(cc), ConsentNotFound) + unboxFullOrFail(_, Some(cc), ConsentNotFound, 403) } _ <- Consent.assertUKConsentAccess(consent.userId, consent.consumerId, cc) _ <- Future(Consents.consentProvider.vend.revoke(consentId)) map { @@ -222,7 +222,7 @@ object Http4sUKOBv310AccountAccess extends MdcLoggable { EndpointHelpers.executeAndRespond(req) { cc => for { consent <- Future(Consents.consentProvider.vend.getConsentByConsentId(consentId)) map { - unboxFullOrFail(_, Some(cc), s"$ConsentNotFound ($consentId)") + unboxFullOrFail(_, Some(cc), ConsentNotFound, 403) } _ <- Consent.assertUKConsentAccess(consent.userId, consent.consumerId, cc) consentViews <- Future(JwtUtil.getSignedPayloadAsJson(consent.jsonWebToken).map( diff --git a/obp-api/src/main/scala/code/api/UKOpenBanking/v4_0_1/Http4sUKOBv401AccountInfo.scala b/obp-api/src/main/scala/code/api/UKOpenBanking/v4_0_1/Http4sUKOBv401AccountInfo.scala index 949d31bc27..513222f8e9 100644 --- a/obp-api/src/main/scala/code/api/UKOpenBanking/v4_0_1/Http4sUKOBv401AccountInfo.scala +++ b/obp-api/src/main/scala/code/api/UKOpenBanking/v4_0_1/Http4sUKOBv401AccountInfo.scala @@ -229,7 +229,7 @@ object Http4sUKOBv401AccountInfo extends MdcLoggable { EndpointHelpers.executeAndRespond(req) { cc => for { consent <- Future(Consents.consentProvider.vend.getConsentByConsentId(consentId)) map { - unboxFullOrFail(_, Some(cc), s"$ConsentNotFound ($consentId)") + unboxFullOrFail(_, Some(cc), ConsentNotFound, 403) } _ <- Consent.assertUKConsentAccess(consent.userId, consent.consumerId, cc) consentViews <- Future(JwtUtil.getSignedPayloadAsJson(consent.jsonWebToken).map( @@ -275,7 +275,7 @@ object Http4sUKOBv401AccountInfo extends MdcLoggable { for { _ <- passesPsd2Aisp(Some(cc)) consent <- Future(Consents.consentProvider.vend.getConsentByConsentId(consentId)) map { - unboxFullOrFail(_, Some(cc), ConsentNotFound) + unboxFullOrFail(_, Some(cc), ConsentNotFound, 403) } _ <- Consent.assertUKConsentAccess(consent.userId, consent.consumerId, cc) _ <- Future(Consents.consentProvider.vend.revoke(consentId)) map { 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 7cb6e6c71a..b58d887491 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 @@ -318,7 +318,7 @@ object Http4sBGv13AIS extends MdcLoggable { // than access, but the asymmetry between two neighbouring reads of the same consent was an // oversight, not a decision. consent <- Future(Consents.consentProvider.vend.getConsentByConsentId(consentId)) map { - unboxFullOrFail(_, callContext, s"$ConsentNotFound ($consentId)") + unboxFullOrFail(_, callContext, ConsentNotFound, 403) } _ <- Consent.assertBerlinGroupConsentReadAccess(consent.userId, consent.consumerId, cc) (challenges, callContext) <- NewStyle.function.getChallengesByConsentId(consentId, callContext) @@ -336,7 +336,7 @@ object Http4sBGv13AIS extends MdcLoggable { for { _ <- passesPsd2Aisp(callContext) consent <- Future(Consents.consentProvider.vend.getConsentByConsentId(consentId)) map { - unboxFullOrFail(_, callContext, s"$ConsentNotFound ($consentId)") + unboxFullOrFail(_, callContext, ConsentNotFound, 403) } _ <- Consent.assertBerlinGroupConsentReadAccess(consent.userId, consent.consumerId, cc) } yield { 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 848ee97616..d0600da0a6 100644 --- a/obp-api/src/main/scala/code/api/util/ConsentUtil.scala +++ b/obp-api/src/main/scala/code/api/util/ConsentUtil.scala @@ -2051,9 +2051,18 @@ object Consent extends MdcLoggable { consentUserId, consentConsumerId, actingPsu(callContext).map(_.userId), callContext.consumer.map(_.consumerId.get), isScaFrontEnd(callContext.consumer.map(_.consumerId.get))) - // booleanToFuture only reads failMsg when the statement is false, so the empty default is never - // the message anyone sees. - Helper.booleanToFuture(refusal.getOrElse(""), 403, Some(callContext))(refusal.isEmpty) + // ConsentNotFound whatever the reason, and the same answer these endpoints give for a consent id + // that matches nothing at all. A caller who is not entitled to a consent must not be able to + // tell "there is no such consent" from "that one is not yours", or the endpoint is a way to + // confirm which ids exist. The rule's own ConsentDoesNotMatchUser / ConsentDoesNotMatchConsumer + // say which, so the reason is logged rather than returned -- same shape as the Berlin Group + // read wrapper above, which is the answer this one was brought into line with. + refusal.foreach { reason => + logger.info( + s"A UK consent read was refused: $reason. Reported as ${ErrorMessages.ConsentNotFound} so " + + s"the caller cannot tell a consent that is not theirs from one that does not exist.") + } + Helper.booleanToFuture(ErrorMessages.ConsentNotFound, 403, Some(callContext))(refusal.isEmpty) } /** diff --git a/obp-api/src/test/scala/code/api/UKOpenBanking/v3_1_0/UKOpenBankingV310AisTests.scala b/obp-api/src/test/scala/code/api/UKOpenBanking/v3_1_0/UKOpenBankingV310AisTests.scala index 20da0faa34..78a2737b57 100644 --- a/obp-api/src/test/scala/code/api/UKOpenBanking/v3_1_0/UKOpenBankingV310AisTests.scala +++ b/obp-api/src/test/scala/code/api/UKOpenBanking/v3_1_0/UKOpenBankingV310AisTests.scala @@ -1,7 +1,7 @@ package code.api.UKOpenBanking.v3_1_0 import code.api.util.APIUtil.{DateWithDayFormat, ResourceDoc, UserOrApplication, buildOperationId} -import code.api.util.ErrorMessages.ConsentDoesNotMatchConsumer +import code.api.util.ErrorMessages.ConsentNotFound import code.consent.Consents import com.openbankproject.commons.model.ErrorMessage import com.openbankproject.commons.util.ApiVersion @@ -75,13 +75,19 @@ class UKOpenBankingV310AisTests extends UKOpenBankingV310ServerSetup { // Http4sUKOBv310AccountAccess.deleteAccountAccessConsentsConsentId only guards against a // *different bound user*, not a *different consumer*. deleteAuthedAsUser2 authenticates as // consumer2 (see DefaultUsers), a different OAuth1 consumer than the one that created this - // pending consent (testConsumer/consumer). Once fixed, this must be 403 - // ConsentDoesNotMatchConsumer, and the consent must be left untouched. + // pending consent (testConsumer/consumer). It is refused with 403, and the consent must be + // left untouched. The refusal says ConsentNotFound rather than naming the consumer: these + // endpoints answer the same thing for a consent that is not yours and one that does not exist, + // so that a caller cannot use them to find out which ids are real. scenario("authenticated as a different consumer than the creator, pending consent -> 403, and consent is left untouched", UKOpenBankingV310) { val consentId = createPendingConsentForConsumer1() val response = deleteAuthedAsUser2("account-access-consents", consentId) response.code should equal(403) - response.body.extract[ErrorMessage].message should startWith(ConsentDoesNotMatchConsumer) + // The refusal is still a 403 and still has no side effect, which is what this scenario + // guards. Only the wording moved: these endpoints now answer ConsentNotFound whatever the + // reason, so a caller cannot tell "that one is not yours" from "there is no such consent". + // The specific reason is logged instead. + response.body.extract[ErrorMessage].message should startWith(ConsentNotFound) Consents.consentProvider.vend.getConsentByConsentId(consentId).isDefined should equal(true) } @@ -97,12 +103,17 @@ class UKOpenBankingV310AisTests extends UKOpenBankingV310ServerSetup { // Cross-consumer regression (currently RED): same root cause as the DELETE gap above -- // a pending consent is currently readable by ANY authenticated party. getAuthedAsUser2 // authenticates as consumer2, a different OAuth1 consumer than the one that created this - // pending consent. Once fixed, this must be 403 ConsentDoesNotMatchConsumer. + // pending consent. It is refused with 403 ConsentNotFound -- the same answer an id that + // matches nothing gets, deliberately. scenario("authenticated as a different consumer than the creator, pending consent -> 403, not 200", UKOpenBankingV310) { val consentId = createPendingConsentForConsumer1() val response = getAuthedAsUser2("account-access-consents", consentId) response.code should equal(403) - response.body.extract[ErrorMessage].message should startWith(ConsentDoesNotMatchConsumer) + // The refusal is still a 403 and still has no side effect, which is what this scenario + // guards. Only the wording moved: these endpoints now answer ConsentNotFound whatever the + // reason, so a caller cannot tell "that one is not yours" from "there is no such consent". + // The specific reason is logged instead. + response.body.extract[ErrorMessage].message should startWith(ConsentNotFound) } } 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 504969c6f1..29338fff11 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 @@ -2,7 +2,7 @@ package code.api.UKOpenBanking.v4_0_1 import code.api.Constant import code.api.util.APIUtil.{DateWithDayFormat, ResourceDoc, UserOrApplication, buildOperationId} -import code.api.util.ErrorMessages.{ConsentDoesNotMatchConsumer, ConsentDoesNotMatchUser, ConsentExpiredIssue, ConsentIdClaimMissing} +import code.api.util.ErrorMessages.{ConsentDoesNotMatchConsumer, ConsentDoesNotMatchUser, ConsentExpiredIssue, ConsentIdClaimMissing, ConsentNotFound} import code.api.util.{CallContext, CertificateUtil, Consent} import code.consent.{ConsentStatus, Consents} import code.model.UserExtended @@ -221,8 +221,30 @@ class UKOpenBankingV401AccountInfoTests extends UKOpenBankingV401ServerSetup { // freshly-created consent is AWAITINGAUTHORISATION → wire code AWAU (response.body \ "Data" \ "Status").extract[String] should equal("AWAU") } - scenario("authenticated with unknown consent -> 400", UKOpenBankingV401AccountInfo) { - getAuthed("aisp", "account-access-consents", "fake-consentid").code should equal(400) + scenario("authenticated with unknown consent -> 403", UKOpenBankingV401AccountInfo) { + getAuthed("aisp", "account-access-consents", "fake-consentid").code should equal(403) + } + // A caller who is not entitled to a consent must not be able to tell "there is no such consent" + // from "that one is not yours". The two used to answer differently -- 400 with the id spelled + // back, against 403 -- which turns the endpoint into a way of confirming that an id exists. + // Berlin Group's equivalents already answer the same thing both ways. + scenario("a consent that does not exist and one that is not yours answer identically", UKOpenBankingV401AccountInfo) { + val someoneElses = createRealConsent() // resourceUser1's, lodged under testConsumer + + val missing = getAuthedAsUser2("aisp", "account-access-consents", "no-such-consent-at-all") + val foreign = getAuthedAsUser2("aisp", "account-access-consents", someoneElses) + + withClue("the two answers differ, so the endpoint confirms which ids exist: ") { + missing.code should equal(foreign.code) + missing.body should equal(foreign.body) + } + missing.code should equal(403) + + And("neither answer names a consent") + val message = missing.body.extract[ErrorMessage].message + message should startWith(ConsentNotFound) + message should not include someoneElses + message should not include "no-such-consent-at-all" } scenario("unauthenticated -> 401", UKOpenBankingV401AccountInfo) { getUnauthed("aisp", "account-access-consents", "fake-consentid").code should equal(401) @@ -237,7 +259,11 @@ class UKOpenBankingV401AccountInfoTests extends UKOpenBankingV401ServerSetup { val consentId = createRealConsent() // owned by resourceUser1 val response = getAuthedAsUser2("aisp", "account-access-consents", consentId) response.code should equal(403) - response.body.extract[ErrorMessage].message should startWith(ConsentDoesNotMatchUser) + // The refusal is still a 403 and still has no side effect, which is what this scenario + // guards. Only the wording moved: these endpoints now answer ConsentNotFound whatever the + // reason, so a caller cannot tell "that one is not yours" from "there is no such consent". + // The specific reason is logged instead. + response.body.extract[ErrorMessage].message should startWith(ConsentNotFound) } // Cross-consumer regression (currently RED): a pending consent (no bound PSU yet) is // currently readable by ANY authenticated party, because the ownership check @@ -250,7 +276,11 @@ class UKOpenBankingV401AccountInfoTests extends UKOpenBankingV401ServerSetup { val consentId = createPendingConsentForConsumer1() val response = getAuthedAsUser2("aisp", "account-access-consents", consentId) response.code should equal(403) - response.body.extract[ErrorMessage].message should startWith(ConsentDoesNotMatchConsumer) + // The refusal is still a 403 and still has no side effect, which is what this scenario + // guards. Only the wording moved: these endpoints now answer ConsentNotFound whatever the + // reason, so a caller cannot tell "that one is not yours" from "there is no such consent". + // The specific reason is logged instead. + response.body.extract[ErrorMessage].message should startWith(ConsentNotFound) } } feature("UKOB v4.0.1 DELETE /aisp/account-access-consents/CONSENT_ID") { @@ -266,8 +296,9 @@ class UKOpenBankingV401AccountInfoTests extends UKOpenBankingV401ServerSetup { // stored status is REVOKED, but the v4.0.1 wire format reports the spec's CANC code (afterDelete.body \ "Data" \ "Status").extract[String] should equal("CANC") } - scenario("authenticated with unknown consent -> 400", UKOpenBankingV401AccountInfo) { - deleteAuthed("aisp", "account-access-consents", "fake-consentid").code should equal(400) + scenario("authenticated with unknown consent -> 403", UKOpenBankingV401AccountInfo) { + // Same answer as a consent that exists but is not the caller's, on purpose -- see the GET twin. + deleteAuthed("aisp", "account-access-consents", "fake-consentid").code should equal(403) } scenario("unauthenticated -> 401", UKOpenBankingV401AccountInfo) { deleteUnauthed("aisp", "account-access-consents", "fake-consentid").code should equal(401) @@ -282,7 +313,11 @@ class UKOpenBankingV401AccountInfoTests extends UKOpenBankingV401ServerSetup { val consentId = createRealConsent() // owned by resourceUser1 val response = deleteAuthedAsUser2("aisp", "account-access-consents", consentId) response.code should equal(403) - response.body.extract[ErrorMessage].message should startWith(ConsentDoesNotMatchUser) + // The refusal is still a 403 and still has no side effect, which is what this scenario + // guards. Only the wording moved: these endpoints now answer ConsentNotFound whatever the + // reason, so a caller cannot tell "that one is not yours" from "there is no such consent". + // The specific reason is logged instead. + response.body.extract[ErrorMessage].message should startWith(ConsentNotFound) val stillThere = Consents.consentProvider.vend.getConsentByConsentId(consentId).openOrThrowException("consent") stillThere.status should equal(ConsentStatus.AWAITINGAUTHORISATION.toString) @@ -297,7 +332,11 @@ class UKOpenBankingV401AccountInfoTests extends UKOpenBankingV401ServerSetup { val consentId = createPendingConsentForConsumer1() val response = deleteAuthedAsUser2("aisp", "account-access-consents", consentId) response.code should equal(403) - response.body.extract[ErrorMessage].message should startWith(ConsentDoesNotMatchConsumer) + // The refusal is still a 403 and still has no side effect, which is what this scenario + // guards. Only the wording moved: these endpoints now answer ConsentNotFound whatever the + // reason, so a caller cannot tell "that one is not yours" from "there is no such consent". + // The specific reason is logged instead. + response.body.extract[ErrorMessage].message should startWith(ConsentNotFound) val stillThere = Consents.consentProvider.vend.getConsentByConsentId(consentId).openOrThrowException("consent") stillThere.status should equal(ConsentStatus.AWAITINGAUTHORISATION.toString) From 55aea15ef9aa21d1d92eb37850a8f0ec8bf11d05 Mon Sep 17 00:00:00 2001 From: Hongwei Date: Thu, 13 Aug 2026 01:29:46 +0200 Subject: [PATCH 3/3] fix: the across-accounts transactions endpoint returns data, and only what was consented GET /aisp/transactions returned nothing at all for any account not held at the instance's default bank. It passed the default Bank into getModeratedTransactions, and moderateTransactionsWithSameAccount builds the moderated account from whatever Bank it is handed and then refuses every transaction that does not belong to it -- logging "Attempted to moderate a transaction using the incorrect moderated account" once per row. The `.getOrElse(Nil)` closing the per-account comprehension turned that into an empty list, so the endpoint answered 200 with no transactions and the failure was visible only in the log. Measured on a local instance: the per-account endpoint returned two transactions for a consent the bulk endpoint answered with zero. Each account's own Bank is now resolved -- one lookup per distinct bank, not per account, since a consent can name accounts at several. With data flowing, the direction permissions apply here too. ReadTransactionsCredits and ReadTransactionsDebits restrict which rows a consent may see, and the profile's Permissions table lists /transactions FIRST in the endpoint column for both of them, ahead of /accounts/{AccountId}/transactions; the data cluster reads "Ability to read only credit transactions". The same three calls the per-account path makes through UKTransactionsQuery are applied per account, because a consent can grant different directions on different ones: the query param so the database applies the restriction together with the page limit, and the filter so it still holds when the connector ignores the param. warnIfPageWasTrimmed becomes shared rather than copied, so both paths report a short page the same way. The gap survived because uk_direction.py, the probe written for this rule, only ever drove the per-account endpoint -- the one the profile lists second. It now drives both, and it was the bulk half of it that was red: zero rows for a Credits-only, a Debits-only and a both-directions consent alike. --- .../UKOpenBanking/UKTransactionsQuery.scala | 2 +- .../v4_0_1/Http4sUKOBv401AccountInfo.scala | 29 ++++++++++++++++--- .../UKOpenBankingV401AccountInfoTests.scala | 7 +++++ 3 files changed, 33 insertions(+), 5 deletions(-) diff --git a/obp-api/src/main/scala/code/api/UKOpenBanking/UKTransactionsQuery.scala b/obp-api/src/main/scala/code/api/UKOpenBanking/UKTransactionsQuery.scala index ed55bf4be3..5e0aa9b624 100644 --- a/obp-api/src/main/scala/code/api/UKOpenBanking/UKTransactionsQuery.scala +++ b/obp-api/src/main/scala/code/api/UKOpenBanking/UKTransactionsQuery.scala @@ -101,7 +101,7 @@ object UKTransactionsQuery extends MdcLoggable { * that lost rows to the filter is the exact signature, and it tells an operator which connector * still needs to honour the param. */ - private def warnIfPageWasTrimmed( + def warnIfPageWasTrimmed( fetched: List[ModeratedTransaction], kept: List[ModeratedTransaction], params: List[OBPQueryParam], diff --git a/obp-api/src/main/scala/code/api/UKOpenBanking/v4_0_1/Http4sUKOBv401AccountInfo.scala b/obp-api/src/main/scala/code/api/UKOpenBanking/v4_0_1/Http4sUKOBv401AccountInfo.scala index 513222f8e9..6d31782d30 100644 --- a/obp-api/src/main/scala/code/api/UKOpenBanking/v4_0_1/Http4sUKOBv401AccountInfo.scala +++ b/obp-api/src/main/scala/code/api/UKOpenBanking/v4_0_1/Http4sUKOBv401AccountInfo.scala @@ -11,7 +11,7 @@ import code.api.util.APIUtil.{EmptyBody, ResourceDoc, HTTPParam, UserOrApplicati import code.api.util.ApiTag import code.api.util.CallContext import code.api.util.CustomJsonFormats -import code.api.util.ErrorMessages.{AuthenticatedUserIsRequired, ConsentNotFound, ConsentViewNotFund, InvalidJsonFormat, InvalidUKConsentPermissions, UnknownError} +import code.api.util.ErrorMessages.{AuthenticatedUserIsRequired, BankNotFound, ConsentNotFound, ConsentViewNotFund, InvalidJsonFormat, InvalidUKConsentPermissions, UnknownError} import code.api.util.http4s.Http4sRequestAttributes.{EndpointHelpers, RequestOps} import code.api.util.newstyle.ViewNewStyle import code.api.util.{APIUtil, Consent, ConsentJWT, JwtUtil, NewStyle} @@ -24,7 +24,7 @@ import com.github.dwickern.macros.NameOf.nameOf import com.openbankproject.commons.model.{AccountId, BankId, BankIdAccountId, TransactionAttribute, View, ViewId} import com.openbankproject.commons.util.{ApiVersion, ScannedApiVersion} import com.openbankproject.commons.util.JsonAliases -import net.liftweb.common.Full +import net.liftweb.common.{Box, Full} import org.json4s.{Formats, JObject} import org.http4s._ import org.http4s.dsl.io._ @@ -3503,6 +3503,11 @@ object Http4sUKOBv401AccountInfo extends MdcLoggable { (bank, _) <- NewStyle.function.getBank(BankId(defaultBankId), Some(cc)) availablePrivateAccounts <- Views.views.vend.getPrivateBankAccountsFuture(u) (accounts, _) <- NewStyle.function.getBankAccounts(availablePrivateAccounts, Some(cc)) + // One lookup per distinct bank rather than one per account: a consent may name accounts at + // several banks, and moderation needs each account's own. + banksById <- Future.sequence(accounts.map(_.bankId).distinct.map { bankId => + NewStyle.function.getBank(bankId, Some(cc)).map { case (b, _) => bankId -> b } + }).map(_.toMap) allTxns <- Future { val detailViewId = ViewId(Constant.SYSTEM_READ_TRANSACTIONS_DETAIL_VIEW_ID) val basicViewId = ViewId(Constant.SYSTEM_READ_TRANSACTIONS_BASIC_VIEW_ID) @@ -3515,9 +3520,25 @@ object Http4sUKOBv401AccountInfo extends MdcLoggable { // per-account endpoint does. view <- APIUtil.checkViewAccessAndReturnView(detailViewId, BankIdAccountId(bankAccount.bankId, bankAccount.accountId), Full(u), Some(cc)) .or(APIUtil.checkViewAccessAndReturnView(basicViewId, BankIdAccountId(bankAccount.bankId, bankAccount.accountId), Full(u), Some(cc))) + // The account's own bank, not the instance's default one. moderateTransactionsWithSameAccount + // builds the moderated account from whatever Bank it is handed and then refuses every + // transaction that does not belong to it -- so passing the default bank returned an empty + // list for every account not held there, logging "Attempted to moderate a transaction using + // the incorrect moderated account" once per row. The `.getOrElse(Nil)` below is what made + // that look like "this account has no transactions" rather than like a failure. + accountBank <- Box(banksById.get(bankAccount.bankId)) ?~! s"$BankNotFound ${bankAccount.bankId.value}" params = createQueriesByHttpParams(req.headers.headers.toList.map(h => HTTPParam(h.name.toString, List(h.value)))).getOrElse(Nil) - (transactions, _) <- BankAccountExtended(bankAccount).getModeratedTransactions(bank, Full(u), view, BankIdAccountId(bankAccount.bankId, bankAccount.accountId), Some(cc), params) - } yield transactions).getOrElse(Nil) + // Resolved per account, because a consent can grant different directions on different + // accounts. Same three calls the per-account endpoint makes through UKTransactionsQuery: + // the query param so the database applies the restriction with the page limit, and the + // filter so the restriction still holds when the connector ignores the param. + grantsCredits = UKAmounts.grantsView(Constant.SYSTEM_READ_TRANSACTIONS_CREDITS_VIEW_ID, bankAccount.bankId, bankAccount.accountId, u, cc) + grantsDebits = UKAmounts.grantsView(Constant.SYSTEM_READ_TRANSACTIONS_DEBITS_VIEW_ID, bankAccount.bankId, bankAccount.accountId, u, cc) + directedParams = params ++ UKAmounts.directionQueryParam(grantsCredits, grantsDebits) + (transactions, _) <- BankAccountExtended(bankAccount).getModeratedTransactions(accountBank, Full(u), view, BankIdAccountId(bankAccount.bankId, bankAccount.accountId), Some(cc), directedParams) + directed = UKAmounts.filterByGrantedDirections(transactions, grantsCredits, grantsDebits) + _ = UKTransactionsQuery.warnIfPageWasTrimmed(transactions, directed, directedParams, cc) + } yield directed).getOrElse(Nil) } } } yield JSONFactory_UKOpenBanking_401.createTransactionsJson(bank.bankId, allTxns) 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 29338fff11..c1bf21c458 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 @@ -35,6 +35,13 @@ import scala.concurrent.duration._ // token, which let a token with no bound consent reach 500 instead of the 403 // OBP-35035 every other AISP data endpoint gives it. // +// That 401/not-401 limit is also why the two aggregate endpoints' substance is covered out of +// repo. What getTransactions does per account -- resolve that account's own bank for moderation, +// and resolve and apply the consent's granted transaction directions -- only happens once a real +// consent is in play, which needs a Bearer token this suite cannot mint. uk_direction.py drives it +// against a running instance with a real consent, on the bulk path as well as the per-account one; +// it was the bulk path being absent from that probe that let the gap stand. +// // The remaining 80 endpoints are still static spec-faithful stubs; their tests // are unchanged (two scenarios: authenticated -> fixed code, unauthenticated -> 401). class UKOpenBankingV401AccountInfoTests extends UKOpenBankingV401ServerSetup {