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/main/scala/code/api/util/ConsentUtil.scala b/obp-api/src/main/scala/code/api/util/ConsentUtil.scala index d0600da0a6..e76501f9d6 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,8 @@ import code.api.{APIFailure, APIFailureNewStyle, Constant, RequestHeader} 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} @@ -1991,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) } /** @@ -2133,13 +2152,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..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 @@ -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) { @@ -310,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) @@ -458,6 +524,68 @@ 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 + } + } + + // 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