fix: three consent-access defects left over after #2882

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)

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(

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