fix: token-path PSU, consent existence responses, and bulk transaction direction

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],

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(

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._
@@ -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 {
@@ -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)

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 {

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),
@@ -2018,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)
   }
 
   /**

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)
     }
   }