From c0fba7154653effd13ef6fa6e76856248bc6fef3 Mon Sep 17 00:00:00 2001 From: Hongwei Date: Thu, 13 Aug 2026 10:15:55 +0200 Subject: [PATCH 1/3] fix: a deleted or locked PSU cannot be named in the Berlin Group PSU-ID header findPsuByPsuId resolved the header to a user without asking whether that user may still act. The header is not a credential -- under Berlin Group the caller is the TPP and the PSU arrives as a header value -- so this path never authenticates the PSU, and AfterApiAuth.checkUserIsDeletedOrLocked, which every authenticated request passes through, never runs on it. What the resolved user is then used for is the problem. startConsentAuthorisation takes it as the PSU, assertBerlinGroupConsentAccountsHeld checks it against the consent's accounts and passes -- the accounts really are theirs -- createChallengesC2 mints an SCA challenge for them and an OTP goes out to that person, and the PUT twin binds the consent to them, after which it grants access to their accounts. A lock is the ASPSP's decision that this user may not authenticate; nothing on this path consulted it, and nothing downstream would have caught it. Refused with the same Empty an unresolvable PSU-ID gives, so resolvePsuIdHeader answers UserNotFoundByProviderAndUsername at 401 -- the standard's PSU_CREDENTIALS_INVALID -- either way. A distinct "that user is locked" would tell a TPP the username exists, which is the oracle the consent reads were unified to close. The real reason is logged. Same predicates as the canonical guard rather than a second opinion about what usable means. Both scenarios are mutation-checked: with the filter disabled the unit one resolves a locked user, and the endpoint one starts the authorisation (201 where 401 is expected) -- which is the OTP actually being sent, and the reason the refusal has to land before the challenge is minted rather than after. --- .../scala/code/api/util/ConsentUtil.scala | 34 ++++++++++- .../BerlinGroupV13ConsentAccessTests.scala | 57 +++++++++++++++++++ 2 files changed, 90 insertions(+), 1 deletion(-) 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 d0600da0a6..4905a3b9a3 100644 --- a/obp-api/src/main/scala/code/api/util/ConsentUtil.scala +++ b/obp-api/src/main/scala/code/api/util/ConsentUtil.scala @@ -14,6 +14,7 @@ import code.api.{APIFailure, APIFailureNewStyle, Constant, RequestHeader} import code.bankconnectors.Connector import code.consent import code.consent.ConsentStatus.ConsentStatus +import code.loginattempts.LoginAttempt import code.consent.{ConsentStatus, Consents, MappedConsent} import code.consumer.Consumers import code.context.{ConsentAuthContextProvider, UserAuthContextProvider} @@ -2133,13 +2134,44 @@ object Consent extends MdcLoggable { * that is not local is looked up across providers and accepted only when exactly one user answers * to it. Two would make the header ambiguous, and choosing between them is not the ASPSP's to do * on the PSU's behalf. + * + * ==A deleted or locked user does not answer to it== + * + * This header names somebody the ASPSP then acts for: the PSU it resolves to is checked against + * the consent's accounts, an SCA challenge is minted for them and an OTP goes out to them, and the + * PUT twin binds the consent to them, after which it grants access to their accounts. None of that + * authenticates the PSU -- under Berlin Group the caller is the TPP and the PSU arrives as a header + * value -- so AfterApiAuth.checkUserIsDeletedOrLocked, which every authenticated request passes + * through, never runs on this path. + * + * A lock is the ASPSP's decision that this user may not authenticate. Resolving them here anyway + * routes around that decision, and nothing downstream catches it: the accounts really are theirs, + * so the holdings check passes. Same predicates as the canonical guard rather than a second + * opinion about what "usable" means. + * + * Refused as Empty, not with a reason of its own. resolvePsuIdHeader turns an unresolved header + * into UserNotFoundByProviderAndUsername at 401 -- the standard's PSU_CREDENTIALS_INVALID -- and a + * locked user has to get that same answer. A distinct "that user is locked" would tell a TPP the + * username exists, which is the existence oracle the consent reads were unified to close. */ def findPsuByPsuId(psuId: String): Box[User] = - Users.users.vend.getUserByProviderAndUsername(Constant.localIdentityProvider, psuId) or { + Users.users.vend.getUserByProviderAndUsername(Constant.localIdentityProvider, psuId).or { Users.users.vend.getUsersByUsername(psuId) match { case theOnlyOne :: Nil => Full(theOnlyOne) case _ => Empty } + }.filter { user => + val unusable = + if (user.isDeleted.getOrElse(false)) Some(ErrorMessages.UserIsDeleted) + else if (LoginAttempt.userIsLocked(user.provider, user.name)) Some(ErrorMessages.UsernameHasBeenLocked) + else None + unusable.foreach { reason => + logger.info( + s"findPsuByPsuId: PSU-ID named a user who may not act ($reason). Reported as " + + s"${ErrorMessages.UserNotFoundByProviderAndUsername} so the caller cannot tell that the " + + s"username exists.") + } + unusable.isEmpty } def createUKConsentJWT( 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 901cdbab1b..c67f90dea5 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 @@ -10,6 +10,7 @@ import code.api.util.ErrorMessages.{BerlinGroupPsuNotIdentified, ConsentDoesNotM import code.consent.{ConsentStatus, Consents} import code.model.TokenType.Access import code.token.Tokens +import code.userlocks.UserLocksProvider import code.transactionChallenge.Challenges import net.liftweb.common.{Empty, Full} import net.liftweb.util.Helpers.randomString @@ -184,6 +185,39 @@ class BerlinGroupV13ConsentAccessTests extends BerlinGroupConsentFixtures { } } + // The PSU-ID header names a person the ASPSP then acts for: it resolves to a user, that user is + // checked against the consent's accounts, an SCA challenge is minted for them and an OTP goes out + // to them, and the PUT twin binds the consent to them. None of that authenticates the PSU -- under + // Berlin Group the caller is the TPP -- so the guard every authenticated request gets from + // AfterApiAuth.checkUserIsDeletedOrLocked never runs on this path. A lock says the ASPSP has + // decided this user may not authenticate; resolving them here anyway routes around that decision, + // and every later check passes because the accounts really are theirs. + feature("Consent.findPsuByPsuId only resolves a user who may still act") { + + scenario("a live user resolves", BerlinGroupV13ConsentAccess) { + Consent.findPsuByPsuId(resourceUser1.name).map(_.userId) should equal(Full(resourceUser1.userId)) + } + + scenario("a locked user does not resolve", BerlinGroupV13ConsentAccess) { + UserLocksProvider.lockUser(resourceUser2.provider, resourceUser2.name) + try { + Consent.findPsuByPsuId(resourceUser2.name) should equal(Empty) + } finally { + UserLocksProvider.unlockUser(resourceUser2.provider, resourceUser2.name) + } + And("unlocking puts them back") + Consent.findPsuByPsuId(resourceUser2.name).map(_.userId) should equal(Full(resourceUser2.userId)) + } + + // Empty rather than a distinct "this user is locked": resolvePsuIdHeader turns an unresolved + // header into UserNotFoundByProviderAndUsername at 401, and a locked user must get that same + // answer. Telling the two apart would hand a TPP a way to confirm that a username exists, which + // is the oracle the consent reads were unified to close. + scenario("the refusal does not say which of the two it was", BerlinGroupV13ConsentAccess) { + Consent.findPsuByPsuId("no-such-user-at-all") should equal(Empty) + } + } + feature("Consent.genuinePsu") { scenario("a session with no user at all has no PSU", BerlinGroupV13ConsentAccess) { @@ -458,6 +492,29 @@ class BerlinGroupV13ConsentAccessTests extends BerlinGroupConsentFixtures { */ feature("BG v1.3 - an Embedded SCA challenge belongs to the PSU, not to the TPP relaying it") { + // The refusal has to land before the challenge is minted, not after. Starting an authorisation + // sends an OTP to the person PSU-ID names, out of band -- so a locked user being resolvable here + // means the ASPSP messages somebody it has already decided may not authenticate, and the TPP is + // one answered code away from binding their accounts to a consent. + scenario("A locked PSU named in PSU-ID gets no challenge at all", BerlinGroupV13ConsentAccess) { + setPropsValues("suggested_default_sca_method" -> "DUMMY") + val consentId = createUnclaimedBerlinGroupConsent().consentId + + UserLocksProvider.lockUser(resourceUser1.provider, resourceUser1Name) + val refused = + try startAuthorisation(consentId, clientCredentialsSession, psuIdHeader(resourceUser1Name)) + finally UserLocksProvider.unlockUser(resourceUser1.provider, resourceUser1Name) + + Then("it is refused as an unresolvable PSU-ID, saying nothing about the lock") + refused.code should equal(401) + + And("no challenge was minted, so nobody was messaged") + Challenges.ChallengeProvider.vend.getChallengesByConsentId(consentId) match { + case Full(challenges) => challenges shouldBe empty + case _ => // no rows at all is the same answer + } + } + scenario("A client-credentials TPP completes SCA for the PSU it names in PSU-ID", BerlinGroupV13ConsentAccess) { setPropsValues("suggested_default_sca_method" -> "DUMMY") val consentId = createUnclaimedBerlinGroupConsent().consentId From a554c08b711dac7ba5f60fa4d4161ddffc9153ab Mon Sep 17 00:00:00 2001 From: Hongwei Date: Thu, 13 Aug 2026 10:41:45 +0200 Subject: [PATCH 2/3] fix: the last Berlin Group consent read stops echoing the consent id Four of the five reads were unified to a bare ConsentNotFound so that a caller cannot tell "there is no such consent" from "that one is not yours". getConsentScaStatus kept spelling the id back: the replacement matched the default-status-code spelling of unboxFullOrFail and this site already passed 403 explicitly, so the pattern skipped it. Same status either way, different message -- the oracle survived at one endpoint. The scenario added here holds all five to the same answer at once, which is what would have caught the miss. It compares the status, the tppMessages code and the tppMessages text rather than the whole body. The Berlin Group error envelope carries a `path` field holding the request path, so it always contains whichever id the caller put in the URL -- they already knew it, and asserting on it would make the scenario fail for a reason that is not a leak. --- .../berlin/group/v1_3/Http4sBGv13AIS.scala | 2 +- .../BerlinGroupV13ConsentAccessTests.scala | 32 +++++++++++++++++++ 2 files changed, 33 insertions(+), 1 deletion(-) 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 b58d887491..f051cbaff8 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 @@ -356,7 +356,7 @@ object Http4sBGv13AIS extends MdcLoggable { // being fetched here purely to prove it exists, so the SCA status of anyone's consent was // readable by any AISP that knew the id. consent <- Future(Consents.consentProvider.vend.getConsentByConsentId(consentId)) map { - unboxFullOrFail(_, callContext, s"$ConsentNotFound ($consentId)", 403) + unboxFullOrFail(_, callContext, ConsentNotFound, 403) } _ <- Consent.assertBerlinGroupConsentReadAccess(consent.userId, consent.consumerId, cc) (challenges, callContext) <- NewStyle.function.getChallengesByConsentId(consentId, callContext) 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 c67f90dea5..d9188a1e47 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 @@ -344,6 +344,38 @@ class BerlinGroupV13ConsentAccessTests extends BerlinGroupConsentFixtures { feature("BG v1.3 - the consent reads apply the same ownership rule as the authorisation pair") { + // A caller 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 confirms which ids are real. Four of these five reads + // were unified to a bare ConsentNotFound; getConsentScaStatus kept spelling the id back, because + // the replacement matched the default-status-code spelling and this site already passed 403 + // explicitly. Same status either way, different body -- so the oracle survived at one endpoint. + scenario("every read answers a missing consent exactly as it answers a foreign one", BerlinGroupV13ConsentAccess) { + val someoneElses = createUnclaimedBerlinGroupConsent().consentId + val missing = "no-such-consent-at-all" + + // Status and the tppMessages entry, not the whole body: the Berlin Group error envelope + // carries a `path` field holding the request path, so it always contains whichever id the + // caller themselves put in the URL. That is not a leak -- they already knew it -- and + // comparing it would make this scenario impossible to satisfy for the wrong reason. + def answers(consentId: String) = consentReads(consentId, user2).map { case (what, r) => + val message = r.body.extract[ErrorMessagesBG].tppMessages.head + what -> (r.code, message.code, message.text) + } + + answers(someoneElses).zip(answers(missing)).foreach { case ((what, foreign), (_, absent)) => + withClue(s"'$what' tells a foreign consent from a missing one: ") { + foreign should equal(absent) + } + } + + And("and the message names no consent") + answers(missing).foreach { case (what, (_, _, text)) => + withClue(s"'$what' echoed the id: ") { + text should not include missing + } + } + } + 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) From 3346d256b7c8ff33b51f029290fce25582d997ba Mon Sep 17 00:00:00 2001 From: Hongwei Date: Thu, 13 Aug 2026 11:07:30 +0200 Subject: [PATCH 3/3] fix: a consent whose JWT cannot be read grants nobody the benefit of the doubt assertBerlinGroupConsentAccountsHeld collapsed the JWT read into `.getOrElse(Nil)`, so "this consent's JWT cannot be parsed" became "this consent names no accounts". The second is a legitimate state -- the availableAccounts shape, whose views are materialised later against the PSU's own holdings -- and the guard is right to pass on it. The first is not, and the guard passed on it too: with no account list to object to, a PSU who holds none of the consent's accounts was admitted, an OTP went out to them, and the PUT twin would bind the consent to them. Reaching it takes a structurally invalid JWT, which getSignedPayloadAsJson answers with a Failure. A consent can carry one: createConsent writes the consent row before computing and storing the JWT, so a consent whose JWT generation failed persists with none. A well-formed JWT carrying some other payload throws out of the extract instead -- Box.map does not catch -- and already failed closed with a 500; tryo brings that to the same clean refusal. Both are now the same answer, because they are the same fact: we cannot establish what this consent covers, so we do not let anyone authorise it. The guard had no test of its own. It has three now: the PSU who holds the named account may start the authorisation, one who does not may not, and an unreadable JWT refuses rather than passing vacuously -- the last of which fails with "201 equaled 201" without this change, the 201 being the OTP going out. Not included: updateViewsOfBerlinGroupConsentJWT rewrites a consent's JWT after the fact and has no callers, and it was the one thing that could have turned this into real data access -- bind under an unreadable JWT, then have a readable one written. That is moot now the guard refuses outright, so removing 60 lines of possibly-intended code no longer buys anything here; its fate is worth deciding on its own. --- .../scala/code/api/util/ConsentUtil.scala | 60 ++++++++++++------- .../BerlinGroupV13ConsentAccessTests.scala | 39 ++++++++++++ 2 files changed, 78 insertions(+), 21 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 4905a3b9a3..e76501f9d6 100644 --- a/obp-api/src/main/scala/code/api/util/ConsentUtil.scala +++ b/obp-api/src/main/scala/code/api/util/ConsentUtil.scala @@ -15,6 +15,7 @@ import code.bankconnectors.Connector import code.consent import code.consent.ConsentStatus.ConsentStatus import code.loginattempts.LoginAttempt +import net.liftweb.util.Helpers.tryo import code.consent.{ConsentStatus, Consents, MappedConsent} import code.consumer.Consumers import code.context.{ConsentAuthContextProvider, UserAuthContextProvider} @@ -1992,27 +1993,44 @@ object Consent extends MdcLoggable { callContext: Option[CallContext] ): Future[Box[Unit]] = Future { implicit val dateFormats: Formats = CustomJsonFormats.formats - val consentAccounts: List[(String, String)] = JwtUtil.getSignedPayloadAsJson(storedConsent.jsonWebToken) - .map(com.openbankproject.commons.util.JsonAliases.parse(_).extract[ConsentJWT]) - .map(_.views.map(v => (v.bank_id, v.account_id)).distinct) - .getOrElse(Nil) - .filter { case (bankId, accountId) => bankId != null && accountId != null } - - val notHeld: List[String] = consentAccounts - .groupBy(_._1) - .toList - .flatMap { case (bankId, pairs) => - val held = AccountHolders.accountHolders.vend - .getAccountsHeld(BankId(bankId), psu) - .map(_.accountId.value) - pairs.map(_._2).filterNot(held.contains) - } - - (notHeld.isEmpty, notHeld): (Boolean, List[String]) - } flatMap { case (allHeld: Boolean, notHeld: List[String]) => - Helper.booleanToFuture( - s"${ErrorMessages.ConsentAccountNotHeldByUser} Account(s): ${notHeld.mkString(", ")}", - 403, callContext)(allHeld) + // The Box is carried rather than flattened to Nil. "This consent names no accounts" is a + // legitimate state -- the availableAccounts shape described above -- and "its JWT cannot be + // read" is not, and a `getOrElse(Nil)` collapsed the second into the first: the guard then + // found nothing to object to and passed on a consent it had learned nothing about. + // + // tryo around the Box as well, because Box.map does not catch: getSignedPayloadAsJson returns a + // Failure only for a structurally invalid JWT, while a well-formed one carrying some other + // payload throws out of the extract instead. Both are the same answer here -- we cannot tell + // what this consent covers -- and both now reach it as a refusal rather than one as a 500. + val consentAccounts: Box[List[(String, String)]] = tryo { + JwtUtil.getSignedPayloadAsJson(storedConsent.jsonWebToken) + .map(com.openbankproject.commons.util.JsonAliases.parse(_).extract[ConsentJWT]) + }.flatMap(box => box) + .map(_.views.map(v => (v.bank_id, v.account_id)).distinct + .filter { case (bankId, accountId) => bankId != null && accountId != null }) + + consentAccounts.map { accounts => + accounts + .groupBy(_._1) + .toList + .flatMap { case (bankId, pairs) => + val held = AccountHolders.accountHolders.vend + .getAccountsHeld(BankId(bankId), psu) + .map(_.accountId.value) + pairs.map(_._2).filterNot(held.contains) + } + } + } flatMap { + case Full(notHeld) => + Helper.booleanToFuture( + s"${ErrorMessages.ConsentAccountNotHeldByUser} Account(s): ${notHeld.mkString(", ")}", + 403, callContext)(notHeld.isEmpty) + case unreadable => + logger.warn( + s"assertBerlinGroupConsentAccountsHeld: consent ${storedConsent.consentId} has no readable " + + s"JWT, so which accounts it covers cannot be established and the authorisation is refused: " + + s"$unreadable") + Helper.booleanToFuture(ErrorMessages.ConsentNotFound, 403, callContext)(false) } /** 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 d9188a1e47..87af96af9f 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 @@ -547,6 +547,45 @@ class BerlinGroupV13ConsentAccessTests extends BerlinGroupConsentFixtures { } } + // assertBerlinGroupConsentAccountsHeld had no test of its own. These give it one, and the third + // is the point: it read a JWT it could not parse as "this consent names no accounts", which is a + // legitimate state (the availableAccounts shape) rather than an error, so the guard passed on a + // consent it had learned nothing about. + // + // Reaching that needs a STRUCTURALLY invalid JWT. A well-formed one carrying the wrong payload + // throws inside Box.map instead and already fails closed with a 500. An empty string is the + // shape a real instance produces: createConsent writes the consent row before computing and + // storing the JWT, so a consent whose JWT generation failed persists with none. + scenario("the accounts-held guard refuses a PSU who does not hold the consent's accounts", BerlinGroupV13ConsentAccess) { + setPropsValues("suggested_default_sca_method" -> "DUMMY") + val consentId = createUnclaimedBerlinGroupConsent().consentId + + Then("the PSU who holds the named account may start the authorisation") + startAuthorisation(consentId, clientCredentialsSession, psuIdHeader(resourceUser1Name)).code should equal(201) + + And("a PSU who does not hold it may not") + val refused = startAuthorisation(consentId, clientCredentialsSession, psuIdHeader(resourceUser2.name)) + refused.code should equal(403) + } + + scenario("a consent whose JWT cannot be read grants nobody the benefit of the doubt", BerlinGroupV13ConsentAccess) { + setPropsValues("suggested_default_sca_method" -> "DUMMY") + val consentId = createUnclaimedBerlinGroupConsent().consentId + Consents.consentProvider.vend.setJsonWebToken(consentId, "") + + When("a PSU who does not hold the consent's accounts starts an authorisation") + val response = startAuthorisation(consentId, clientCredentialsSession, psuIdHeader(resourceUser2.name)) + + Then("it is refused, rather than passing because the account list came back empty") + response.code should not equal 201 + + And("no challenge was minted for them") + Challenges.ChallengeProvider.vend.getChallengesByConsentId(consentId) match { + case Full(challenges) => challenges shouldBe empty + case _ => // no rows at all is the same answer + } + } + scenario("A client-credentials TPP completes SCA for the PSU it names in PSU-ID", BerlinGroupV13ConsentAccess) { setPropsValues("suggested_default_sca_method" -> "DUMMY") val consentId = createUnclaimedBerlinGroupConsent().consentId