fix: bind the UK SCA challenge to its consent, and three ownership fixes

obp-api/src/main/resources/props/sample.props.template

@@ -1896,6 +1896,20 @@ securelogging_mask_email=true
 # already configured for Berlin Group Redirect SCA needs no edit.
 # sca_front_end_consumer_ids=
 
+# Let a consent that records no lodging TPP be addressed by any authenticated caller.
+#
+# A consent belongs to the Consumer that lodged it -- Berlin Group states it for the whole API
+# ("all methods submitted by a TPP ... may only apply to resources which have been created by the
+# same TPP before", Implementation Guidelines 4.11) and UK scopes GET and DELETE of an
+# account-access-consent to one "that they have created". A row recording no Consumer therefore
+# matches no caller, and is refused.
+#
+# Rows like that exist: they predate the Consumer being recorded on a consent at all. Turning this
+# on restores the old behaviour for them -- and the old behaviour is that ANY authenticated caller
+# can read and revoke them, which is why it is off by default and warns on every use. It is a
+# migration window, not a setting: re-lodge the affected consents and turn it back off.
+# consent_allow_legacy_unrecorded_tpp=false
+
 # Berlin Group: the Consumers that are this ASPSP's own Strong Customer Authentication front end,
 # comma separated consumer ids. Under the Redirect approach the PSU authenticates at the ASPSP, so
 # the authorisation calls on /berlin-group/v1.3/consents/{id}/authorisations arrive from that front

obp-api/src/main/scala/code/api/berlin/group/v1_3/Http4sBGv13AIS.scala

@@ -163,11 +163,7 @@ object Http4sBGv13AIS extends MdcLoggable {
           consent <- Future(Consents.consentProvider.vend.getConsentByConsentId(consentId)) map {
             unboxFullOrFail(_, callContext, ConsentNotFound, 403)
           }
-          consumerIdFromConsent = consent.mConsumerId.get
-          consumerIdFromCurrentCall = callContext.map(_.consumer.map(_.consumerId.get).getOrElse("None")).getOrElse("None")
-          _ <- booleanToFuture(failMsg = ConsentNotFound, failCode = 403, cc = callContext) {
-            consumerIdFromConsent == consumerIdFromCurrentCall
-          }
+          _ <- Consent.assertBerlinGroupConsentReadAccess(consent.userId, consent.consumerId, cc)
           _ <- Future(Consents.consentProvider.vend.revokeBerlinGroupConsent(consentId)) map {
             i => connectorEmptyResponse(i, callContext)
           }
@@ -324,9 +320,7 @@ object Http4sBGv13AIS extends MdcLoggable {
           consent <- Future(Consents.consentProvider.vend.getConsentByConsentId(consentId)) map {
             unboxFullOrFail(_, callContext, s"$ConsentNotFound ($consentId)")
           }
-          _ <- booleanToFuture(failMsg = ConsentNotFound, failCode = 403, cc = callContext) {
-            consent.mConsumerId.get == cc.consumer.map(_.consumerId.get).getOrElse("None")
-          }
+          _ <- Consent.assertBerlinGroupConsentReadAccess(consent.userId, consent.consumerId, cc)
           (challenges, callContext) <- NewStyle.function.getChallengesByConsentId(consentId, callContext)
         } yield {
           JSONFactory_BERLIN_GROUP_1_3.AuthorisationJsonV13(challenges.map(_.challengeId))
@@ -344,11 +338,7 @@ object Http4sBGv13AIS extends MdcLoggable {
           consent <- Future(Consents.consentProvider.vend.getConsentByConsentId(consentId)) map {
             unboxFullOrFail(_, callContext, s"$ConsentNotFound ($consentId)")
           }
-          consumerIdFromConsent = consent.mConsumerId.get
-          consumerIdFromCurrentCall = callContext.map(_.consumer.map(_.consumerId.get).getOrElse("None")).getOrElse("None")
-          _ <- booleanToFuture(failMsg = ConsentNotFound, failCode = 403, cc = callContext) {
-            consumerIdFromConsent == consumerIdFromCurrentCall
-          }
+          _ <- Consent.assertBerlinGroupConsentReadAccess(consent.userId, consent.consumerId, cc)
         } yield {
           createGetConsentResponseJson(consent)
         }
@@ -368,9 +358,7 @@ object Http4sBGv13AIS extends MdcLoggable {
           consent <- Future(Consents.consentProvider.vend.getConsentByConsentId(consentId)) map {
             unboxFullOrFail(_, callContext, s"$ConsentNotFound ($consentId)", 403)
           }
-          _ <- booleanToFuture(failMsg = ConsentNotFound, failCode = 403, cc = callContext) {
-            consent.mConsumerId.get == cc.consumer.map(_.consumerId.get).getOrElse("None")
-          }
+          _ <- Consent.assertBerlinGroupConsentReadAccess(consent.userId, consent.consumerId, cc)
           (challenges, callContext) <- NewStyle.function.getChallengesByConsentId(consentId, callContext)
         } yield {
           val challengeStatus = challenges.filter(_.challengeId == authorisationId)
@@ -393,9 +381,7 @@ object Http4sBGv13AIS extends MdcLoggable {
           // Same ownership test as the three sibling reads of this consent. Without it the status of
           // any consent was readable by any AISP holding its id -- which is enough to confirm the
           // consent exists and to watch it move through authorisation.
-          _ <- booleanToFuture(failMsg = ConsentNotFound, failCode = 403, cc = callContext) {
-            consent.mConsumerId.get == cc.consumer.map(_.consumerId.get).getOrElse("None")
-          }
+          _ <- Consent.assertBerlinGroupConsentReadAccess(consent.userId, consent.consumerId, cc)
         } yield {
           JSONFactory_BERLIN_GROUP_1_3.ConsentStatusJsonV13(consent.status)
         }

obp-api/src/main/scala/code/api/util/ConsentUtil.scala

@@ -1741,7 +1741,12 @@ object Consent extends MdcLoggable {
   ): Option[String] = {
     (present(consentUserId), callerUserId.flatMap(present)) match {
       case (Some(psu), Some(caller)) if psu != caller => Some(ErrorMessages.ConsentDoesNotMatchUser)
-      case (Some(_), Some(_)) => None
+      // The PSU matches, which is necessary but not sufficient: the Consumer still has to, and that
+      // comparison is below. The exception is the ASPSP's own front end, which under Redirect is by
+      // definition not the Consumer that lodged the consent -- that is the whole reason it has to be
+      // declared. Without this case a declared front end could not complete an SCA it is allowed to
+      // start, because psuOrLodgingTppRefusal would refuse it on the Consumer.
+      case (Some(_), Some(_)) if callerIsScaFrontEnd => None
       // Only while the consent is still unclaimed. A caller with no PSU used to reach this too, so a
       // declared front end presenting client credentials could drive the authorisation of a consent
       // already bound to somebody else -- the opposite of what the paragraph above promises.
@@ -1750,6 +1755,37 @@ object Consent extends MdcLoggable {
     }
   }
 
+  /**
+   * checkBerlinGroupConsentAccess for the five endpoints that read or delete a consent resource,
+   * refusing with the answer those endpoints have always given.
+   *
+   * They answer `ConsentNotFound` at 403 whatever the reason, which is deliberate: a caller who is
+   * not entitled to a consent should not be able to tell "it is not yours" from "there is no such
+   * consent", or the endpoint becomes a way to confirm that an id exists. The rule's own
+   * ConsentDoesNotMatchUser / ConsentDoesNotMatchConsumer say which, so the reason is logged rather
+   * than returned. The authorisation pair does return it -- there the caller is mid-ceremony on a
+   * consent it has already been handed, and needs to know why it was stopped.
+   *
+   * Not a general-purpose wrapper: assertUKConsentAccess next to it passes the reason through, and
+   * that difference is the point rather than an inconsistency to iron out.
+   */
+  def assertBerlinGroupConsentReadAccess(
+    consentUserId: String,
+    consentConsumerId: String,
+    callContext: CallContext
+  ): Future[Box[Unit]] = {
+    val refusal = checkBerlinGroupConsentAccess(
+      consentUserId, consentConsumerId,
+      genuinePsu(callContext).map(_.userId), callContext.consumer.map(_.consumerId.get),
+      isScaFrontEnd(callContext.consumer.map(_.consumerId.get)))
+    refusal.foreach { reason =>
+      logger.info(
+        s"A consent read was refused: $reason. Reported as ${ErrorMessages.ConsentNotFound} so the " +
+        s"caller cannot tell a consent that is not theirs from one that does not exist.")
+    }
+    Helper.booleanToFuture(ErrorMessages.ConsentNotFound, 403, Some(callContext))(refusal.isEmpty)
+  }
+
   /**
    * Whether the Consumer making this call is one the ASPSP declared as its own SCA front end.
    *
@@ -1777,16 +1813,39 @@ object Consent extends MdcLoggable {
     callerUserId: Option[String],
     callerConsumerId: Option[String]
   ): Option[String] = {
+    // The Consumer that lodged the consent is what identifies a legitimate caller, and a consent
+    // recording none belongs to nobody. This used to read `owner.forall(...)`, which is vacuously
+    // true when there is no owner -- so a consent with no lodging TPP was addressable by every
+    // authenticated caller. "Absent" was being read as "matches everything" where it means "there is
+    // nobody this belongs to", and neither standard supports the first reading: Berlin Group scopes
+    // a resource to "the same TPP" that created it (IG 4.11), and UK scopes GET and DELETE to "an
+    // account-access-consent resource that they have created".
+    //
+    // It is a live population -- 10 of 566 Berlin Group, 50 of 463 OBP-native and 4 of 753 UK
+    // consents record no consumer on a long-lived instance -- so the prop is there for an operator
+    // who needs a migration window and accepts what it means. Default false, and it says so every
+    // time it is used.
+    def lodgingTppRefusal: Option[String] = present(consentConsumerId) match {
+      case Some(owner) if callerConsumerId.flatMap(present).contains(owner) => None
+      case Some(_) => Some(ErrorMessages.ConsentDoesNotMatchConsumer)
+      case None if APIUtil.getPropsAsBoolValue("consent_allow_legacy_unrecorded_tpp", defaultValue = false) =>
+        logger.warn(
+          "A consent recording no lodging TPP was addressed and allowed through because " +
+          "consent_allow_legacy_unrecorded_tpp is set. Any authenticated caller can read and revoke " +
+          "such a consent while that stays on. Re-lodge the affected consents and turn it off.")
+        None
+      case None => Some(ErrorMessages.ConsentDoesNotMatchConsumer)
+    }
+
     (present(consentUserId), callerUserId.flatMap(present)) match {
-      case (Some(psu), Some(caller)) =>
-        // The consent belongs to a PSU and the caller is acting as one: they must be the same PSU.
-        if (psu == caller) None else Some(ErrorMessages.ConsentDoesNotMatchUser)
-      case _ =>
-        // Either the consent has no PSU yet, or the caller is not acting as one. Either way the
-        // Consumer that lodged it is what identifies a legitimate caller.
-        val owner = present(consentConsumerId)
-        if (owner.forall(id => callerConsumerId.flatMap(present).contains(id))) None
-        else Some(ErrorMessages.ConsentDoesNotMatchConsumer)
+      // The consent belongs to a PSU and the caller is acting as one: they must be the same PSU.
+      case (Some(psu), Some(caller)) if psu != caller => Some(ErrorMessages.ConsentDoesNotMatchUser)
+      // Matching the PSU is necessary, not sufficient. This used to return None here, so agreeing on
+      // the person ended the enquiry and the Consumer was never looked at -- a second TPP holding a
+      // session for the same PSU could read and revoke the consent a first TPP lodged. One TPP's
+      // mandate over a consent is not another's, and IG 4.11's same-TPP rule admits no PSU
+      // exception. So a PSU match falls through to the Consumer comparison rather than skipping it.
+      case _ => lodgingTppRefusal
     }
   }
 

obp-api/src/main/scala/code/api/v5_1_0/Http4s510.scala

@@ -4396,7 +4396,22 @@ object Http4s510 {
             _ <- Helper.booleanToFuture(s"$InvalidJsonFormat The Json body should be the $PostUKConsentAuthoriseJsonV510 (account_ids must not be empty) ", 400, Some(cc)) {
               authJson.account_ids.nonEmpty
             }
-            (_, _) <- NewStyle.function.getChallenge(authJson.challenge_id, Some(cc))
+            (startedChallenge, _) <- NewStyle.function.getChallenge(authJson.challenge_id, Some(cc))
+            // The consent this challenge was minted for is the consent it may authorise, and nothing
+            // below establishes that: validateChallengeAnswerC4 is handed the consentId but the
+            // connector ignores it and matches on challengeId, answer and expected user alone. So a
+            // PSU holding an OTP raised for one consent could answer it against another of their own
+            // -- and since the two need not carry the same Permissions, an OTP the PSU was given for
+            // a narrow consent authorised a wider one, which is precisely the dynamic linking the
+            // profile is asking for ("all authentication journeys take place in the context of a
+            // consent"; the ConsentId is the intent identifier).
+            //
+            // Same guard, message and code as the Berlin Group twin in Http4sBGv13AIS, which gets
+            // the binding structurally by nesting authorisations under the consent's own path. It
+            // also refuses a transaction-request challenge here for free: that carries no consentId.
+            _ <- Helper.booleanToFuture(s"$InvalidChallengeChallengeId Current challengeId(${authJson.challenge_id}) does not belong to CONSENTID($consentId) ", 400, Some(cc)) {
+              startedChallenge.consentId.contains(consentId)
+            }
             (challenge, _) <- NewStyle.function.validateChallengeAnswerC4(
               ChallengeType.OBP_CONSENT_CHALLENGE,
               None,

obp-api/src/main/scala/code/consent/MappedConsent.scala

@@ -377,7 +377,25 @@ object MappedConsentProvider extends ConsentProvider with code.util.Helper.MdcLo
           // Every revoke endpoint funnels through here, so this is the one place that has to give
           // the granted access back. The status flip alone leaves the AccountAccess rows live in
           // the table for anything that reads them without asking the consent first.
-          code.api.util.Consent.revokeConsentAccountAccess(consent)
+          //
+          // Guarded because conditionalRevoke above has already committed, so a throw here cannot
+          // undo the revoke -- it can only hide that it happened. The caller got a 500 and the
+          // retry got ConsentAlreadyRevoked, which leaves an AISP unable to complete the DELETE the
+          // standards require of it and with no correct next step. It does not take much to get
+          // there: revokeConsentAccountAccess extracts the stored JWT through Box.map, which does
+          // not catch, so a payload that no longer matches ConsentJWT arrives as a MappingException.
+          //
+          // Logged at error, not swallowed. The consent IS revoked and the API is right to say so,
+          // but rows that should have been dropped are still live, and that is the one thing about
+          // a revoked consent nobody comes back for. Same shape as the sibling below.
+          tryo(code.api.util.Consent.revokeConsentAccountAccess(consent)) match {
+            case Failure(msg, ex, _) =>
+              logger.error(
+                s"revoke: consent $consentId is revoked, but its account access could not be given " +
+                s"back and those rows are still live: $msg" +
+                ex.map(e => s" (${e.getClass.getSimpleName}: ${e.getMessage})").getOrElse(""))
+            case _ =>
+          }
           MappedConsent.find(By(MappedConsent.mConsentId, consentId))
         }
         else Failure(ErrorMessages.ConsentAlreadyRevoked)

obp-api/src/main/scala/code/views/MapperViews.scala

@@ -196,32 +196,6 @@ object MapperViews extends Views with MdcLoggable {
     }
   }
 
-  // The consumer-scoped counterparts of the two grants above, mirroring the revoke...ForConsumer
-  // pair further down. Access granted through one application's consent belongs to that application
-  // alone: keeping it on its own row is what stops a second TPP's consent from rewriting it, and
-  // what makes narrowing a consent narrow only that consent.
-  def grantAccessToSystemViewForConsumer(bankId: BankId, accountId: AccountId, view: View, user: User, consumerId: String): Box[View] = {
-    { view.isPublic && !allowPublicViews } match {
-      case true => Failure(PublicViewsNotAllowedOnThisInstance)
-      case false => getOrGrantAccessToViewCommon(user, view, bankId.value, accountId.value, consumerId)
-    }
-  }
-
-  def grantAccessToCustomViewForConsumer(bankIdAccountIdViewId: BankIdAccountIdViewId, user: User, consumerId: String): Box[View] = {
-    val viewDefinition = ViewDefinition.findCustomView(
-      bankIdAccountIdViewId.bankId.value,
-      bankIdAccountIdViewId.accountId.value,
-      bankIdAccountIdViewId.viewId.value)
-
-    viewDefinition match {
-      case Full(v) =>
-        if (v.isPublic && !allowPublicViews) Failure(PublicViewsNotAllowedOnThisInstance)
-        else getOrGrantAccessToViewCommon(user, v, bankIdAccountIdViewId.bankId.value, bankIdAccountIdViewId.accountId.value, consumerId)
-      case _ =>
-        Empty ~> APIFailure(s"View ${bankIdAccountIdViewId.viewId} not found", 404)
-    }
-  }
-
   def grantAccessToMultipleViews(views: List[BankIdAccountIdViewId], user: User, callContext: Option[CallContext]): Box[List[View]] = {
     val viewDefinitions: List[(ViewDefinition, BankIdAccountIdViewId)] = views.map {
       uid => ViewDefinition.findCustomView(uid.bankId.value,uid.accountId.value, uid.viewId.value).map((_, uid))

obp-api/src/main/scala/code/views/Views.scala

@@ -36,11 +36,15 @@ trait Views {
   def revokeAccessToSystemViewForConsumer(bankId: BankId, accountId: AccountId, view : View, consumerId : String) : Box[Boolean]
   def revokeAccessToCustomViewForConsumer(view : View, consumerId : String) : Box[Boolean]
 
-  // Grant/revoke a single application's access, rather than access shared by every application.
-  // Used by the consent flows: what one TPP's consent grants is that TPP's alone, so narrowing or
-  // re-authorising it must not touch another TPP's grants on the same account.
-  def grantAccessToSystemViewForConsumer(bankId: BankId, accountId: AccountId, view : View, user : User, consumerId : String) : Box[View]
-  def grantAccessToCustomViewForConsumer(bankIdAccountIdViewId : BankIdAccountIdViewId, user : User, consumerId : String) : Box[View]
+  // Revoke and enumerate one application's access, rather than access shared by every application.
+  // The consent flows use these against Constant.ALL_CONSUMERS, which is the literal every consent
+  // grant is written under -- not a wildcard, and every lookup matches it by equality.
+  //
+  // There is deliberately no grant...ForConsumer counterpart. Two existed and had no callers, and
+  // could not have gained one safely: a row written under a real consumer id is invisible to
+  // revokeConsentAccountAccess, which sweeps by asking for exactly ALL_CONSUMERS, so the access
+  // would outlive the consent that granted it. Grants go through grantAccessToSystemView /
+  // grantAccessToCustomView, which write ALL_CONSUMERS and are therefore revocable.
   def revokeAccessToViewForUserAndConsumer(bankIdAccountIdViewId : BankIdAccountIdViewId, user : User, consumerId : String) : Box[Boolean]
   // Everything a user currently holds under one application's scope. The consent flows reconcile
   // against this: a consent's granted views are whatever its JWT says, so the rows that back it are