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.