diff --git a/obp-api/src/main/resources/props/sample.props.template b/obp-api/src/main/resources/props/sample.props.template index 1343fe7138..71aeb66167 100644 --- a/obp-api/src/main/resources/props/sample.props.template +++ b/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 diff --git a/obp-api/src/main/scala/code/api/berlin/group/v1_3/Http4sBGv13AIS.scala b/obp-api/src/main/scala/code/api/berlin/group/v1_3/Http4sBGv13AIS.scala index b7bc9acf82..7cb6e6c71a 100644 --- a/obp-api/src/main/scala/code/api/berlin/group/v1_3/Http4sBGv13AIS.scala +++ b/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) } 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 8d01dca2b7..4d18800eee 100644 --- a/obp-api/src/main/scala/code/api/util/ConsentUtil.scala +++ b/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 } } diff --git a/obp-api/src/main/scala/code/api/v5_1_0/Http4s510.scala b/obp-api/src/main/scala/code/api/v5_1_0/Http4s510.scala index 70d4ad7a49..78e39f7e99 100644 --- a/obp-api/src/main/scala/code/api/v5_1_0/Http4s510.scala +++ b/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, diff --git a/obp-api/src/main/scala/code/consent/MappedConsent.scala b/obp-api/src/main/scala/code/consent/MappedConsent.scala index c281d3103c..d660f1b46f 100644 --- a/obp-api/src/main/scala/code/consent/MappedConsent.scala +++ b/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) diff --git a/obp-api/src/main/scala/code/views/MapperViews.scala b/obp-api/src/main/scala/code/views/MapperViews.scala index f89d18ba14..6a25480a45 100644 --- a/obp-api/src/main/scala/code/views/MapperViews.scala +++ b/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)) diff --git a/obp-api/src/main/scala/code/views/Views.scala b/obp-api/src/main/scala/code/views/Views.scala index a617eabf04..ac59c49e78 100644 --- a/obp-api/src/main/scala/code/views/Views.scala +++ b/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 diff --git a/obp-api/src/test/scala/code/api/UKOpenBanking/v4_0_1/UKOpenBankingV401AccountInfoTests.scala b/obp-api/src/test/scala/code/api/UKOpenBanking/v4_0_1/UKOpenBankingV401AccountInfoTests.scala index ef6d535c66..504969c6f1 100644 --- a/obp-api/src/test/scala/code/api/UKOpenBanking/v4_0_1/UKOpenBankingV401AccountInfoTests.scala +++ b/obp-api/src/test/scala/code/api/UKOpenBanking/v4_0_1/UKOpenBankingV401AccountInfoTests.scala @@ -54,12 +54,18 @@ class UKOpenBankingV401AccountInfoTests extends UKOpenBankingV401ServerSetup { | "Risk": {} |}""".stripMargin + // consumerId is the consumer these consents are then read with (getAuthed signs as testConsumer), + // because that is what the endpoint itself records -- createAccountAccessConsents passes the + // calling consumer straight into saveUKConsent. It used to pass None here, which produced a row + // no production path creates: one naming no lodging TPP. That was invisible while such a row was + // readable by anyone, and became a 403 the moment "records no TPP" stopped meaning "matches every + // caller". The fixture was the thing that was wrong. private def createRealConsent(): String = { val consent = Consents.consentProvider.vend.saveUKConsent( user = Some(resourceUser1), bankId = None, accountIds = None, - consumerId = None, + consumerId = Some(testConsumer.consumerId.get), permissions = consentPermissions, expirationDateTime = Some(DateWithDayFormat.parse("2030-01-01")), transactionFromDateTime = Some(DateWithDayFormat.parse("2020-01-01")), @@ -748,6 +754,47 @@ class UKOpenBankingV401AccountInfoTests extends UKOpenBankingV401ServerSetup { private def boundPsuOf(consentId: String): String = Option(storedConsent(consentId).userId).map(_.trim).getOrElse("") + // The profile defines the authentication journey as taking place in the context of one consent: + // the ConsentId is the intent identifier, and the intent id is carried into the request object so + // that "the access token that is eventually generated [is] bound to a specific consent". OBP mints + // its own OTP challenge instead of using that mechanism, so the binding the profile gets + // structurally has to be asserted here -- and the connector will not do it: LocalMappedConnector's + // validateChallengeAnswerC4 ignores the consentId it is handed and matches on challengeId, answer + // and userId alone. The Berlin Group twin of this endpoint already asserts it + // (Http4sBGv13AIS: startedChallenge.consentId.contains(consentId)). + feature("UKOB v4.0.1 a challenge only authorises the consent it was minted for") { + scenario("a challenge minted on one consent cannot authorise another -> 400, and the other consent is untouched", UKOpenBankingV401AccountInfo) { + setPropsValues("suggested_default_sca_method" -> "DUMMY") + val expiry = Some(new java.util.Date(System.currentTimeMillis() + 3600000L)) + // Two consents the same PSU may authorise. The PSU guard at the top of the authorise endpoint + // passes for both, so nothing but the binding stands between them. + val challenged = createUKConsent(None, expiry) + val other = createUKConsent(None, expiry) + + val challenge = makePostRequest(scaChallengeRequest(challenged) <@ (user1), "") + challenge.code should equal(200) + val challengeId = (challenge.body \ "challenge_id").extract[String] + + When("the challenge minted on one consent is answered against the other") + val refused = makePostRequest(authoriseRequest(other) <@ (user1), + s"""{"account_ids":["$acc"],"challenge_id":"$challengeId","answer":"123"}""") + + Then("it is refused with the same answer the Berlin Group twin gives") + refused.code should equal(400) + refused.body.extract[ErrorMessage].message should include("OBP-40022") + + // The refusal is the point, but so is what it did not do: an authorisation that got this far + // used to bind the PSU and flip the status on a consent nobody authenticated for. + And("the consent it was aimed at is left exactly as it was") + boundPsuOf(other) should equal("") + storedConsent(other).status should equal(ConsentStatus.AWAITINGAUTHORISATION.toString) + + And("so is the consent the challenge actually belonged to") + boundPsuOf(challenged) should equal("") + storedConsent(challenged).status should equal(ConsentStatus.AWAITINGAUTHORISATION.toString) + } + } + feature("UKOB v4.0.1 a refused authorisation does not claim the consent") { scenario("account_ids naming an account the PSU does not hold -> refused, consent left unbound", UKOpenBankingV401AccountInfo) { // The dummy SCA answer is `123` only when this is set; test props leave it unset, so the diff --git a/obp-api/src/test/scala/code/api/UKOpenBanking/v4_0_1/UKOpenBankingV401ConsentAccessTests.scala b/obp-api/src/test/scala/code/api/UKOpenBanking/v4_0_1/UKOpenBankingV401ConsentAccessTests.scala index 9e21e3dd4b..cc10f3892c 100644 --- a/obp-api/src/test/scala/code/api/UKOpenBanking/v4_0_1/UKOpenBankingV401ConsentAccessTests.scala +++ b/obp-api/src/test/scala/code/api/UKOpenBanking/v4_0_1/UKOpenBankingV401ConsentAccessTests.scala @@ -138,13 +138,35 @@ class UKOpenBankingV401ConsentAccessTests extends UKOpenBankingV401ServerSetup { } scenario("blank ids count as absent, not as a value to match", UKOpenBankingV401ConsentAccess) { - // A consent lodged before consumer binding existed stores no consumer id; nothing identifies a - // wrong caller, so it cannot be refused on that basis. - Consent.checkUKConsentAccess("", "", None, Some(tpp), callerIsScaFrontEnd = false) should equal(None) - Consent.checkUKConsentAccess(null, null, None, None, callerIsScaFrontEnd = false) should equal(None) - // A blank caller user id is not a PSU either -- it must not accidentally match a blank binding. + // A blank caller user id is not a PSU -- it must not accidentally match a blank binding. Consent.checkUKConsentAccess(psu, tpp, Some(" "), Some(tpp), callerIsScaFrontEnd = false) should equal(None) } + + // The two assertions that used to sit in the scenario above have inverted. They read + // checkUKConsentAccess("", "", None, Some(tpp), false) should equal(None) + // checkUKConsentAccess(null, null, None, None, false) should equal(None) + // on the reasoning that a consent lodged before consumer binding existed "cannot be refused on + // that basis" because nothing identifies a wrong caller. The other half of that is that nothing + // identifies a RIGHT one either, and the profile scopes these endpoints to "an + // account-access-consent resource that they have created" -- a row naming no creator matches no + // caller rather than every caller. 4 of 753 UK consents on a long-lived instance record no + // consumer, and until now any authenticated caller could read and revoke them. + scenario("a consent that records no lodging TPP belongs to nobody", UKOpenBankingV401ConsentAccess) { + Consent.checkUKConsentAccess("", "", None, Some(tpp), callerIsScaFrontEnd = false) should + equal(Some(ConsentDoesNotMatchConsumer)) + Consent.checkUKConsentAccess(null, null, None, None, callerIsScaFrontEnd = false) should + equal(Some(ConsentDoesNotMatchConsumer)) + } + + // Matching the PSU is necessary, not sufficient: the rule used to stop as soon as the two PSU + // ids agreed, so a second TPP holding a session for the same person reached a first TPP's + // consent. The UK case for the Consumer comparison is per-endpoint rather than blanket -- the + // Endpoints table marks GET and DELETE Client Credentials, so no PSU is party to them at all, + // and a PSU session here is an OBP extension that cannot be the thing that waives it. + scenario("a second TPP holding a session for the consent's own PSU is still refused", UKOpenBankingV401ConsentAccess) { + Consent.checkUKConsentAccess(psu, tpp, Some(psu), Some(otherTpp), callerIsScaFrontEnd = false) should + equal(Some(ConsentDoesNotMatchConsumer)) + } } // The rule above is only ever as good as the identity handed to it, and that is where this family diff --git a/obp-api/src/test/scala/code/api/UKOpenBanking/v4_0_1/UKOpenBankingV401ConsentScopingTests.scala b/obp-api/src/test/scala/code/api/UKOpenBanking/v4_0_1/UKOpenBankingV401ConsentScopingTests.scala index ef80b4f856..5d4fc7ad19 100644 --- a/obp-api/src/test/scala/code/api/UKOpenBanking/v4_0_1/UKOpenBankingV401ConsentScopingTests.scala +++ b/obp-api/src/test/scala/code/api/UKOpenBanking/v4_0_1/UKOpenBankingV401ConsentScopingTests.scala @@ -417,6 +417,37 @@ class UKOpenBankingV401ConsentScopingTests extends UKOpenBankingV401ServerSetup // Revocation used to flip a status column and leave the granted rows in the table for good. Views.views.vend.accessGrantedToUserForConsumer(principal, Constant.ALL_CONSUMERS) shouldBe empty } + + // The clean-up runs after conditionalRevoke has already committed the status change, so a throw + // in it does not undo the revoke -- it only hides that the revoke happened. The caller gets a + // 500 and the retry gets ConsentAlreadyRevoked, which leaves an AISP unable to complete the + // DELETE the profile requires of it, with no correct next step. The Berlin Group sibling a few + // lines away in MappedConsent already wraps its equivalent; this one did not. + // + // A stored JWT whose payload does not extract is enough to get there. getSignedPayloadAsJson + // does not verify the signature -- it parses the structure and hands back the claims -- so the + // extract that follows is what throws, and Box.map does not catch. The same trap is already + // documented on applyUKConsentPrincipalFromToken. + scenario("a consent whose stored JWT cannot be read is still revoked, and says so", UKConsentScoping) { + val consentId = authoriseConsentFor(testConsumer.consumerId.get, List(ReadAccountsBasic)) + + // Structurally a JWT, and the claims parse as JSON -- they are simply not a ConsentJWT. + def b64(s: String) = java.util.Base64.getUrlEncoder.withoutPadding.encodeToString(s.getBytes("UTF-8")) + Consents.consentProvider.vend.setJsonWebToken( + consentId, s"${b64("""{"alg":"HS256"}""")}.${b64("""{"not":"a consent"}""")}.signature-is-never-checked") + + val revoked = Consents.consentProvider.vend.revoke(consentId) + + withClue("the revoke threw rather than reporting the outcome: ") { + revoked.isDefined should equal(true) + } + Consents.consentProvider.vend.getConsentByConsentId(consentId) + .map(_.status) should equal(Full(ConsentStatus.REVOKED.toString)) + + // And it stays revoked: a second attempt is the ConsentAlreadyRevoked the caller used to be + // pushed into by the failure, rather than a way out of it. + Consents.consentProvider.vend.revoke(consentId).isDefined should equal(false) + } } } diff --git a/obp-api/src/test/scala/code/api/berlin/group/v1_3/BerlinGroupV13ConsentAccessTests.scala b/obp-api/src/test/scala/code/api/berlin/group/v1_3/BerlinGroupV13ConsentAccessTests.scala index 43ffe95d84..901cdbab1b 100644 --- a/obp-api/src/test/scala/code/api/berlin/group/v1_3/BerlinGroupV13ConsentAccessTests.scala +++ b/obp-api/src/test/scala/code/api/berlin/group/v1_3/BerlinGroupV13ConsentAccessTests.scala @@ -87,11 +87,46 @@ class BerlinGroupV13ConsentAccessTests extends BerlinGroupConsentFixtures { equal(Some(ConsentDoesNotMatchUser)) } + // Matching the PSU is necessary, not sufficient. It used to be sufficient: the rule returned + // None the moment the two PSU ids agreed, without ever looking at the Consumer, so a second TPP + // holding a session for the same person could read and revoke the consent a first TPP lodged. + // One TPP's mandate over a consent is not another's -- the same principle the payment guard in + // Http4sBGv13PIS states, and IG 4.11's "may only apply to resources which have been created by + // the same TPP before" admits no PSU exception. + scenario("a second TPP holding a session for the consent's own PSU is still refused", BerlinGroupV13ConsentAccess) { + Consent.checkBerlinGroupConsentAccess(psu, tpp, Some(psu), Some(otherTpp), callerIsScaFrontEnd = false) should + equal(Some(ConsentDoesNotMatchConsumer)) + } + scenario("blank ids count as absent, not as a value to match", BerlinGroupV13ConsentAccess) { - Consent.checkBerlinGroupConsentAccess(null, null, None, None, callerIsScaFrontEnd = false) should equal(None) Consent.checkBerlinGroupConsentAccess(" ", tpp, Some(psu), Some(tpp), callerIsScaFrontEnd = false) should equal(None) Consent.checkBerlinGroupConsentAccess(psu, tpp, Some(" "), Some(tpp), callerIsScaFrontEnd = false) should equal(None) } + + // This assertion is inverted from what it used to say. It read + // checkBerlinGroupConsentAccess(null, null, None, None, false) should equal(None) + // which pinned a consent recording no TPP as addressable by anybody -- "absent" was being read + // as "matches everything" rather than as "there is nobody this belongs to". Neither standard + // supports that: IG 4.11 scopes a resource to the TPP that created it, and UK scopes GET and + // DELETE to "an account-access-consent resource that they have created". A row naming no TPP + // satisfies neither, so it belongs to nobody and is refused. + // + // It is a real population, not a hypothetical: 10 of 566 Berlin Group consents and 4 of 753 UK + // consents record no consumer on a long-lived instance. They are already unreachable in Berlin + // Group today, by accident -- the hand-rolled `null == "None"` compare in the five reads is + // false for everyone -- so refusing them here changes nothing for those callers and only stops + // the UK pair, and the reads once they move onto this rule, from opening up. + scenario("a consent that records no lodging TPP belongs to nobody", BerlinGroupV13ConsentAccess) { + Consent.checkBerlinGroupConsentAccess(null, null, None, None, callerIsScaFrontEnd = false) should + equal(Some(ConsentDoesNotMatchConsumer)) + Consent.checkBerlinGroupConsentAccess(null, " ", Some(psu), Some(tpp), callerIsScaFrontEnd = false) should + equal(Some(ConsentDoesNotMatchConsumer)) + } + + scenario("an operator can restore the old behaviour for a migration window", BerlinGroupV13ConsentAccess) { + setPropsValues("consent_allow_legacy_unrecorded_tpp" -> "true") + Consent.checkBerlinGroupConsentAccess(null, null, None, None, callerIsScaFrontEnd = false) should equal(None) + } } // The Redirect approach: the PSU authenticates at the ASPSP, so these calls arrive from the @@ -259,6 +294,54 @@ class BerlinGroupV13ConsentAccessTests extends BerlinGroupConsentFixtures { private def psuIdHeader(userName: String) = List(("PSU-ID", userName)) + // The five reads of a consent resource. They each used to compare + // consent.mConsumerId.get == cc.consumer.map(_.consumerId.get).getOrElse("None") + // by hand, five separate times, rather than going through checkBerlinGroupConsentAccess like the + // authorisation pair above. Five copies of one security decision is how the two fail-opens fixed + // in the previous commit came to be fixed in the rule and not at these call sites: a hand-rolled + // Consumer compare cannot notice that the PSU half exists at all. + private def consentReads(consentId: String, session: Option[(Consumer, Token)]) = List( + "read the consent" -> makeGetRequest((V1_3_BG / "consents" / consentId).GET <@ (session)), + "read its status" -> makeGetRequest((V1_3_BG / "consents" / consentId / "status").GET <@ (session)), + "list its authorisations" -> makeGetRequest((V1_3_BG / "consents" / consentId / "authorisations").GET <@ (session)), + "read an authorisation's SCA status" -> + makeGetRequest((V1_3_BG / "consents" / consentId / "authorisations" / "any-authorisation-id").GET <@ (session)) + ) + + feature("BG v1.3 - the consent reads apply the same ownership rule as the authorisation pair") { + + 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) + + Then("the same TPP, but acting for a different person, is refused on every read") + // The Consumer matches -- this IS the TPP that lodged it -- so a Consumer-only compare says + // yes. The consent is bound to resourceUser1 and the session is resourceUser2's, and once a + // consent names a PSU the rule requires that PSU, which is what the authorisation pair has + // enforced all along and these reads did not. + consentReads(consentId, secondPsuOfTestConsumerSession).foreach { case (what, response) => + withClue(s"a second PSU of the lodging TPP was allowed to $what: ") { + response.code should equal(403) + } + } + + And("the lodging TPP on a client-credentials session still can") + // The normal Berlin Group caller: no genuine PSU in the session at all, so the PSU half does + // not apply and the Consumer decides. This is the half that must not regress. + consentReads(consentId, clientCredentialsSession).foreach { case (what, response) => + withClue(s"the lodging TPP could not $what: ") { + response.code should equal(200) + } + } + + And("so does DELETE, which is the fifth site and mutates, hence last") + makeDeleteRequest((V1_3_BG / "consents" / consentId).DELETE <@ (secondPsuOfTestConsumerSession)) + .code should equal(403) + makeDeleteRequest((V1_3_BG / "consents" / consentId).DELETE <@ (clientCredentialsSession)) + .code should equal(204) + } + } + feature("BG v1.3 - a consent's authorisation sub-resources answer only to the TPP that lodged it") { scenario("A second TPP cannot start an authorisation on a consent it did not lodge", BerlinGroupV13ConsentAccess) {