From 6206d1fa98c183bb821a095df5e240804a6afae2 Mon Sep 17 00:00:00 2001 From: Hongwei Date: Wed, 12 Aug 2026 22:05:06 +0200 Subject: [PATCH] fix: stop a payment with no recorded consumer returning 500 Berlin Group names a payment by its id alone, so getOwnPaymentImpl has to establish that the caller is the party that lodged it, and it reads the lodging consumer off the stored row. That column is only written when a consumer is on the call, and the absent case is written as a literal null, so `.filter(_.nonEmpty)` dereferenced a null on every payment lodged before the consumer was recorded. 1757 of 1800 rows on a long-lived instance are in that state, and the helper guards all twelve payment-read call sites, so reading, polling the status of, listing authorisations on, or cancelling such a payment answered 500 -- for the party that lodged it. The guard's own documentation already described the intended behaviour: "Payments lodged before the consumer was recorded carry none, and fall back to the person check alone rather than becoming unaddressable." The code simply never reached it. With the read made safe, those rows are governed by the initiator check alone, as documented. Reading a possibly-absent id was already solved four times over inside ConsentUtil, each time as a local `present` -- which is how a fifth caller came to be written without it. Lifted to Consent.present and reused at all five sites, so the null-safe reading is the default rather than something each new site has to remember. The three scenarios in this feature each lodged their own payment through fifteen near-identical lines; the new one made that a third copy. Lodging is not what any of them is about, so it moves to one helper. --- .../berlin/group/v1_3/Http4sBGv13PIS.scala | 2 +- .../scala/code/api/util/ConsentUtil.scala | 26 ++-- .../PaymentInitiationServicePISApiTest.scala | 111 ++++++++++-------- 3 files changed, 81 insertions(+), 58 deletions(-) diff --git a/obp-api/src/main/scala/code/api/berlin/group/v1_3/Http4sBGv13PIS.scala b/obp-api/src/main/scala/code/api/berlin/group/v1_3/Http4sBGv13PIS.scala index d08ff72374..c4a7d0b84d 100644 --- a/obp-api/src/main/scala/code/api/berlin/group/v1_3/Http4sBGv13PIS.scala +++ b/obp-api/src/main/scala/code/api/berlin/group/v1_3/Http4sBGv13PIS.scala @@ -106,7 +106,7 @@ object Http4sBGv13PIS extends MdcLoggable { // the wire, and that model's shape is a frozen contract. lodgedByConsumer = TransactionRequests.transactionRequestProvider.vend .getMappedTransactionRequest(TransactionRequestId(paymentId)) - .map(_.mConsumerId.get).toOption.filter(_.nonEmpty) + .toOption.flatMap(tr => Consent.present(tr.mConsumerId.get)) sameTpp = lodgedByConsumer.forall(lodgedBy => callingConsumer.contains(lodgedBy)) _ <- Helper.booleanToFuture(s"$PaymentNotInitiatedByCaller Payment id: $paymentId.", 403, callContext) { sameTpp && initiators.exists(callers) 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 1ad8daeca4..8d01dca2b7 100644 --- a/obp-api/src/main/scala/code/api/util/ConsentUtil.scala +++ b/obp-api/src/main/scala/code/api/util/ConsentUtil.scala @@ -136,6 +136,24 @@ object Consent extends MdcLoggable { final lazy val challengeAnswerAtTestEnvironment = "123" + /** + * An identifier a row does not carry, read safely: None for absent, blank, or whitespace. + * + * Every id the ownership rules below compare arrives out of a MappedString, and a MappedString + * that was never written holds a Java null rather than "" -- MappedConsent and + * MappedTransactionRequestProvider both write the absent case as a literal null. So `.nonEmpty` + * on the raw value is not a blankness test, it is a NullPointerException waiting for the first + * row that predates the column, and on any long-lived instance nearly every row does. + * + * Trimming as well, because "the consent names no TPP" and "the consent names a TPP whose id is + * three spaces" have to reach the same decision -- a rule that refuses one and admits the other + * is not a rule. + * + * Public rather than local to each caller: it was written four times inside this object before a + * fifth caller in Http4sBGv13PIS omitted it and returned 500 on 1757 of 1800 stored payments. + */ + def present(s: String): Option[String] = Option(s).map(_.trim).filter(_.nonEmpty) + /** * Purpose of this helper function is to get the Consumer-Key value from a Request Headers. * @return the Consumer-Key value from a Request Header as a String @@ -1721,8 +1739,6 @@ object Consent extends MdcLoggable { callerConsumerId: Option[String], callerIsScaFrontEnd: Boolean ): Option[String] = { - def present(s: String): Option[String] = Option(s).map(_.trim).filter(_.nonEmpty) - (present(consentUserId), callerUserId.flatMap(present)) match { case (Some(psu), Some(caller)) if psu != caller => Some(ErrorMessages.ConsentDoesNotMatchUser) case (Some(_), Some(_)) => None @@ -1761,8 +1777,6 @@ object Consent extends MdcLoggable { callerUserId: Option[String], callerConsumerId: Option[String] ): Option[String] = { - def present(s: String): Option[String] = Option(s).map(_.trim).filter(_.nonEmpty) - (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. @@ -1836,8 +1850,6 @@ object Consent extends MdcLoggable { ukConsentUnresolved.filterNot(_ => requestUrl.contains(UK_CONSENT_MANAGEMENT_URL_SEGMENT)) def checkObpConsentUserAccess(consentUserId: String, callerHumanUserId: Option[String]): Option[String] = { - def present(s: String): Option[String] = Option(s).map(_.trim).filter(_.nonEmpty) - present(consentUserId) match { case Some(psu) if !callerHumanUserId.flatMap(present).contains(psu) => Some(ErrorMessages.ConsentNotFound) @@ -2001,8 +2013,6 @@ object Consent extends MdcLoggable { sessionPsuId: Option[String], headerPsuId: Option[String] ): Either[String, String] = { - def present(s: String): Option[String] = Option(s).map(_.trim).filter(_.nonEmpty) - val alreadyKnown = present(consentUserId).orElse(sessionPsuId.flatMap(present)) (alreadyKnown, headerPsuId.flatMap(present)) match { diff --git a/obp-api/src/test/scala/code/api/berlin/group/v1_3/PaymentInitiationServicePISApiTest.scala b/obp-api/src/test/scala/code/api/berlin/group/v1_3/PaymentInitiationServicePISApiTest.scala index 6b0ccce4c1..5128d01669 100644 --- a/obp-api/src/test/scala/code/api/berlin/group/v1_3/PaymentInitiationServicePISApiTest.scala +++ b/obp-api/src/test/scala/code/api/berlin/group/v1_3/PaymentInitiationServicePISApiTest.scala @@ -13,6 +13,7 @@ import code.model.dataAccess.{BankAccountRouting, MappedBankAccount} import code.model.TokenType import code.setup.{APIResponse, DefaultUsers} import code.token.Tokens +import code.transactionrequests.MappedTransactionRequest import net.liftweb.util.Helpers.randomString import net.liftweb.util.TimeHelpers.TimeSpan import code.views.Views @@ -772,41 +773,50 @@ class PaymentInitiationServicePISApiTest extends BerlinGroupServerSetupV1_3 with // caller, so without an explicit check a paymentId is a bearer token: whoever holds it can read // the payment, list and start authorisations on it, and cancel it. The payment records who lodged // it; these scenarios hold every payment-scoped route to that record. + // Every scenario in the feature below needs the same starting point: one payment lodged by user1 + // over the challenge threshold, so it sits awaiting SCA — the state in which a hijacked + // authorisation would actually move money. Shared rather than repeated because what each scenario + // is about is what happens *after* this, and three copies of the lodging made that hard to see. + private def ibanAccounts = BankAccountRouting + .findAll(By(BankAccountRouting.AccountRoutingScheme, AccountRoutingScheme.IBAN.toString)) + .filterNot(_.bankId.value == "DEFAULT_BANK_ID_NOT_SET") + + private def balanceOf(routing: BankAccountRouting) = MappedBankAccount.find( + By(MappedBankAccount.bank, routing.bankId.value), + By(MappedBankAccount.theAccountId, routing.accountId.value)) + .map(_.balance).openOrThrowException("Can not be empty here") + + private def paymentUrl(paymentId: String) = + V1_3_BG / PaymentServiceTypes.payments.toString / TransactionRequestTypes.SEPA_CREDIT_TRANSFERS.toString / paymentId + + /** Lodges a payment as user1 and returns its id alongside the two accounts it moves between. */ + private def lodgePaymentAsUser1(): (String, BankAccountRouting, BankAccountRouting) = { + val ibanFrom = ibanAccounts.head + val ibanTo = ibanAccounts.last + grantAccountAccess(ibanFrom) + val initiatePaymentJson = + s"""{ + | "debtorAccount": { "iban": "${ibanFrom.accountRouting.address}" }, + | "instructedAmount": { "currency": "EUR", "amount": "2001" }, + | "creditorAccount": { "iban": "${ibanTo.accountRouting.address}" }, + | "creditorName": "70charname" + }""".stripMargin + val responseInitiate: APIResponse = makePostRequest( + (V1_3_BG / PaymentServiceTypes.payments.toString / TransactionRequestTypes.SEPA_CREDIT_TRANSFERS.toString).POST <@ (user1), + initiatePaymentJson) + responseInitiate.code should equal(201) + (responseInitiate.body.extract[InitiatePaymentResponseJson].paymentId, ibanFrom, ibanTo) + } + feature("test the BG v1.3 - a payment is only addressable by the party that initiated it") { scenario("a second TPP can neither read, authorise, nor cancel a payment it did not initiate", BerlinGroupV1_3, PIS, initiatePayment) { - val accountsRoutingIban = BankAccountRouting.findAll(By(BankAccountRouting.AccountRoutingScheme, AccountRoutingScheme.IBAN.toString)) - .filterNot(_.bankId.value == "DEFAULT_BANK_ID_NOT_SET") - val ibanFrom = accountsRoutingIban.head - val ibanTo = accountsRoutingIban.last - - def balanceOf(routing: BankAccountRouting) = MappedBankAccount.find( - By(MappedBankAccount.bank, routing.bankId.value), - By(MappedBankAccount.theAccountId, routing.accountId.value)) - .map(_.balance).openOrThrowException("Can not be empty here") - - grantAccountAccess(ibanFrom) - - // Over the challenge threshold, so the payment stays in RCVD awaiting SCA — the state in which - // a hijacked authorisation would actually move money. - val initiatePaymentJson = - s"""{ - | "debtorAccount": { "iban": "${ibanFrom.accountRouting.address}" }, - | "instructedAmount": { "currency": "EUR", "amount": "2001" }, - | "creditorAccount": { "iban": "${ibanTo.accountRouting.address}" }, - | "creditorName": "70charname" - }""".stripMargin - When("user1 initiates a payment") - val responseInitiate: APIResponse = makePostRequest( - (V1_3_BG / PaymentServiceTypes.payments.toString / TransactionRequestTypes.SEPA_CREDIT_TRANSFERS.toString).POST <@ (user1), - initiatePaymentJson) - responseInitiate.code should equal(201) - val paymentId = responseInitiate.body.extract[InitiatePaymentResponseJson].paymentId + val (paymentId, ibanFrom, ibanTo) = lodgePaymentAsUser1() val fromBalanceBefore = balanceOf(ibanFrom) val toBalanceBefore = balanceOf(ibanTo) - val payment = V1_3_BG / PaymentServiceTypes.payments.toString / TransactionRequestTypes.SEPA_CREDIT_TRANSFERS.toString / paymentId + val payment = paymentUrl(paymentId) Then("user2 is refused on every payment-scoped route") val refusals = List( @@ -833,28 +843,9 @@ class PaymentInitiationServicePISApiTest extends BerlinGroupServerSetupV1_3 with ownerResponse.code should equal(200) } scenario("a second TPP acting for the same PSU is refused too", BerlinGroupV1_3, PIS, initiatePayment) { - val accountsRoutingIban = BankAccountRouting.findAll(By(BankAccountRouting.AccountRoutingScheme, AccountRoutingScheme.IBAN.toString)) - .filterNot(_.bankId.value == "DEFAULT_BANK_ID_NOT_SET") - val ibanFrom = accountsRoutingIban.head - val ibanTo = accountsRoutingIban.last - - grantAccountAccess(ibanFrom) - - val initiatePaymentJson = - s"""{ - | "debtorAccount": { "iban": "${ibanFrom.accountRouting.address}" }, - | "instructedAmount": { "currency": "EUR", "amount": "2001" }, - | "creditorAccount": { "iban": "${ibanTo.accountRouting.address}" }, - | "creditorName": "70charname" - }""".stripMargin - When("the PSU lodges a payment through one TPP") - val responseInitiate: APIResponse = makePostRequest( - (V1_3_BG / PaymentServiceTypes.payments.toString / TransactionRequestTypes.SEPA_CREDIT_TRANSFERS.toString).POST <@ (user1), - initiatePaymentJson) - responseInitiate.code should equal(201) - val paymentId = responseInitiate.body.extract[InitiatePaymentResponseJson].paymentId - val payment = V1_3_BG / PaymentServiceTypes.payments.toString / TransactionRequestTypes.SEPA_CREDIT_TRANSFERS.toString / paymentId + val (paymentId, _, _) = lodgePaymentAsUser1() + val payment = paymentUrl(paymentId) Then("the same PSU acting through a second TPP still cannot address it") // The person matches and only the TPP differs -- the case a person-only check waves through, @@ -866,6 +857,28 @@ class PaymentInitiationServicePISApiTest extends BerlinGroupServerSetupV1_3 with And("the TPP that lodged it still can") makeGetRequest((payment / "status").GET <@ (user1)).code should equal(200) } + scenario("a payment lodged before the consumer was recorded is still readable", BerlinGroupV1_3, PIS, initiatePayment) { + When("a payment is lodged") + val (paymentId, _, _) = lodgePaymentAsUser1() + val payment = paymentUrl(paymentId) + + And("the recorded consumer is cleared, as it is on every payment lodged before the field existed") + // Not "": MappedTransactionRequestProvider writes the absent consumer as a literal null, so + // the column is SQL NULL and MappedString reads a null back out. The overwhelming majority of + // rows on any long-lived instance are in this state, so whatever the guard does with them it + // must not be to throw. + MappedTransactionRequest.find(By(MappedTransactionRequest.mTransactionRequestId, paymentId)) + .map(_.mConsumerId(null).saveMe()) + .openOrThrowException("the payment just lodged must be findable") + + Then("the party that lodged it can still read it, its status and its authorisations") + List("read the payment" -> payment, "read its status" -> (payment / "status"), + "list its authorisations" -> (payment / "authorisations")).foreach { case (what, url) => + withClue(s"the initiator could not $what: ") { + makeGetRequest(url.GET <@ (user1)).code should equal(200) + } + } + } } // resourceUser1's own token, issued under a second consumer: same person, different TPP. // DefaultUsers has no such pair -- user2 and user3 change the person as well as the consumer.