diff --git a/obp-api/src/main/scala/code/api/UKOpenBanking/UKTransactionsQuery.scala b/obp-api/src/main/scala/code/api/UKOpenBanking/UKTransactionsQuery.scala index ed55bf4be3..5e0aa9b624 100644 --- a/obp-api/src/main/scala/code/api/UKOpenBanking/UKTransactionsQuery.scala +++ b/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], diff --git a/obp-api/src/main/scala/code/api/UKOpenBanking/v3_1_0/Http4sUKOBv310AccountAccess.scala b/obp-api/src/main/scala/code/api/UKOpenBanking/v3_1_0/Http4sUKOBv310AccountAccess.scala index 225686d143..0a876e0556 100644 --- a/obp-api/src/main/scala/code/api/UKOpenBanking/v3_1_0/Http4sUKOBv310AccountAccess.scala +++ b/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( diff --git a/obp-api/src/main/scala/code/api/UKOpenBanking/v4_0_1/Http4sUKOBv401AccountInfo.scala b/obp-api/src/main/scala/code/api/UKOpenBanking/v4_0_1/Http4sUKOBv401AccountInfo.scala index 949d31bc27..6d31782d30 100644 --- a/obp-api/src/main/scala/code/api/UKOpenBanking/v4_0_1/Http4sUKOBv401AccountInfo.scala +++ b/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) 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 7cb6e6c71a..b58d887491 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 @@ -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 { 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 4d18800eee..d0600da0a6 100644 --- a/obp-api/src/main/scala/code/api/util/ConsentUtil.scala +++ b/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) } /** diff --git a/obp-api/src/test/scala/code/api/UKOpenBanking/v3_1_0/UKOpenBankingV310AisTests.scala b/obp-api/src/test/scala/code/api/UKOpenBanking/v3_1_0/UKOpenBankingV310AisTests.scala index 20da0faa34..78a2737b57 100644 --- a/obp-api/src/test/scala/code/api/UKOpenBanking/v3_1_0/UKOpenBankingV310AisTests.scala +++ b/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) } } 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 504969c6f1..c1bf21c458 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 @@ -2,7 +2,7 @@ package code.api.UKOpenBanking.v4_0_1 import code.api.Constant import code.api.util.APIUtil.{DateWithDayFormat, ResourceDoc, UserOrApplication, buildOperationId} -import code.api.util.ErrorMessages.{ConsentDoesNotMatchConsumer, ConsentDoesNotMatchUser, ConsentExpiredIssue, ConsentIdClaimMissing} +import code.api.util.ErrorMessages.{ConsentDoesNotMatchConsumer, ConsentDoesNotMatchUser, ConsentExpiredIssue, ConsentIdClaimMissing, ConsentNotFound} import code.api.util.{CallContext, CertificateUtil, Consent} import code.consent.{ConsentStatus, Consents} import code.model.UserExtended @@ -35,6 +35,13 @@ import scala.concurrent.duration._ // token, which let a token with no bound consent reach 500 instead of the 403 // OBP-35035 every other AISP data endpoint gives it. // +// That 401/not-401 limit is also why the two aggregate endpoints' substance is covered out of +// repo. What getTransactions does per account -- resolve that account's own bank for moderation, +// and resolve and apply the consent's granted transaction directions -- only happens once a real +// consent is in play, which needs a Bearer token this suite cannot mint. uk_direction.py drives it +// against a running instance with a real consent, on the bulk path as well as the per-account one; +// it was the bulk path being absent from that probe that let the gap stand. +// // The remaining 80 endpoints are still static spec-faithful stubs; their tests // are unchanged (two scenarios: authenticated -> fixed code, unauthenticated -> 401). class UKOpenBankingV401AccountInfoTests extends UKOpenBankingV401ServerSetup { @@ -221,8 +228,30 @@ class UKOpenBankingV401AccountInfoTests extends UKOpenBankingV401ServerSetup { // freshly-created consent is AWAITINGAUTHORISATION → wire code AWAU (response.body \ "Data" \ "Status").extract[String] should equal("AWAU") } - scenario("authenticated with unknown consent -> 400", UKOpenBankingV401AccountInfo) { - getAuthed("aisp", "account-access-consents", "fake-consentid").code should equal(400) + scenario("authenticated with unknown consent -> 403", UKOpenBankingV401AccountInfo) { + getAuthed("aisp", "account-access-consents", "fake-consentid").code should equal(403) + } + // 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". The two used to answer differently -- 400 with the id spelled + // back, against 403 -- which turns the endpoint into a way of confirming that an id exists. + // Berlin Group's equivalents already answer the same thing both ways. + scenario("a consent that does not exist and one that is not yours answer identically", UKOpenBankingV401AccountInfo) { + val someoneElses = createRealConsent() // resourceUser1's, lodged under testConsumer + + val missing = getAuthedAsUser2("aisp", "account-access-consents", "no-such-consent-at-all") + val foreign = getAuthedAsUser2("aisp", "account-access-consents", someoneElses) + + withClue("the two answers differ, so the endpoint confirms which ids exist: ") { + missing.code should equal(foreign.code) + missing.body should equal(foreign.body) + } + missing.code should equal(403) + + And("neither answer names a consent") + val message = missing.body.extract[ErrorMessage].message + message should startWith(ConsentNotFound) + message should not include someoneElses + message should not include "no-such-consent-at-all" } scenario("unauthenticated -> 401", UKOpenBankingV401AccountInfo) { getUnauthed("aisp", "account-access-consents", "fake-consentid").code should equal(401) @@ -237,7 +266,11 @@ class UKOpenBankingV401AccountInfoTests extends UKOpenBankingV401ServerSetup { val consentId = createRealConsent() // owned by resourceUser1 val response = getAuthedAsUser2("aisp", "account-access-consents", consentId) response.code should equal(403) - response.body.extract[ErrorMessage].message should startWith(ConsentDoesNotMatchUser) + // 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) } // Cross-consumer regression (currently RED): a pending consent (no bound PSU yet) is // currently readable by ANY authenticated party, because the ownership check @@ -250,7 +283,11 @@ class UKOpenBankingV401AccountInfoTests extends UKOpenBankingV401ServerSetup { val consentId = createPendingConsentForConsumer1() val response = getAuthedAsUser2("aisp", "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) } } feature("UKOB v4.0.1 DELETE /aisp/account-access-consents/CONSENT_ID") { @@ -266,8 +303,9 @@ class UKOpenBankingV401AccountInfoTests extends UKOpenBankingV401ServerSetup { // stored status is REVOKED, but the v4.0.1 wire format reports the spec's CANC code (afterDelete.body \ "Data" \ "Status").extract[String] should equal("CANC") } - scenario("authenticated with unknown consent -> 400", UKOpenBankingV401AccountInfo) { - deleteAuthed("aisp", "account-access-consents", "fake-consentid").code should equal(400) + scenario("authenticated with unknown consent -> 403", UKOpenBankingV401AccountInfo) { + // Same answer as a consent that exists but is not the caller's, on purpose -- see the GET twin. + deleteAuthed("aisp", "account-access-consents", "fake-consentid").code should equal(403) } scenario("unauthenticated -> 401", UKOpenBankingV401AccountInfo) { deleteUnauthed("aisp", "account-access-consents", "fake-consentid").code should equal(401) @@ -282,7 +320,11 @@ class UKOpenBankingV401AccountInfoTests extends UKOpenBankingV401ServerSetup { val consentId = createRealConsent() // owned by resourceUser1 val response = deleteAuthedAsUser2("aisp", "account-access-consents", consentId) response.code should equal(403) - response.body.extract[ErrorMessage].message should startWith(ConsentDoesNotMatchUser) + // 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) val stillThere = Consents.consentProvider.vend.getConsentByConsentId(consentId).openOrThrowException("consent") stillThere.status should equal(ConsentStatus.AWAITINGAUTHORISATION.toString) @@ -297,7 +339,11 @@ class UKOpenBankingV401AccountInfoTests extends UKOpenBankingV401ServerSetup { val consentId = createPendingConsentForConsumer1() val response = deleteAuthedAsUser2("aisp", "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) val stillThere = Consents.consentProvider.vend.getConsentByConsentId(consentId).openOrThrowException("consent") stillThere.status should equal(ConsentStatus.AWAITINGAUTHORISATION.toString) 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 5d4fc7ad19..f7a9e4152d 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 @@ -266,6 +266,35 @@ class UKOpenBankingV401ConsentScopingTests extends UKOpenBankingV401ServerSetup principal.map(_.userId) should equal(Full(resourceUser1.userId)) callContext.flatMap(_.consenter.toOption) should equal(None) } + + // The PSU used to be taken from the token rather than from the consent, and nothing here + // compared the two. So a token belonging to anyone at all, carrying a consent_id that is not + // theirs, was swapped onto that consent's shadow user -- with `consenter` set to the token's + // holder rather than to the person the consent is about. + // + // checkUKConsent then caught it, because it compares the consent's mUserId against `consenter` + // and they disagreed. But only the UK read endpoints call checkUKConsent: on every other + // endpoint family the swapped principal stood, and it carries the consent's account access. The + // refusal has to be decided here, where it is recorded on the CallContext and enforced for every + // endpoint by ResourceDocMiddleware. + scenario("a token whose subject is not the consent's PSU is refused, not swapped", UKConsentScoping) { + val consentId = authoriseConsentFor(testConsumer.consumerId.get, List(ReadAccountsBasic), + accountIds = List(acc)) + + Given("resourceUser2's session presenting a consent authorised by resourceUser1") + val (principal, callContext) = + Consent.applyUKConsentPrincipalFromToken(Full(resourceUser2), Some(bearerContextFor(consentId, testConsumer))) + val cc = callContext.getOrElse(fail("token path dropped the CallContext")) + + Then("the principal is left alone rather than swapped onto the consent") + principal.map(_.userId) should equal(Full(resourceUser2.userId)) + cc.consenter.toOption should equal(None) + + And("the refusal is recorded, so every endpoint family enforces it and not just the UK reads") + cc.ukConsentUnresolved should equal(Some(ErrorMessages.ConsentDoesNotMatchUser)) + Consent.unresolvedUKConsentRefusal(cc.ukConsentUnresolved, "/obp/v5.1.0/my/accounts") should + equal(Some(ErrorMessages.ConsentDoesNotMatchUser)) + } } /** @@ -383,6 +412,35 @@ class UKOpenBankingV401ConsentScopingTests extends UKOpenBankingV401ServerSetup None, "/open-banking/v4.0.1/aisp/account-access-consents") should equal(None) } + /** + * Which PSU the token path runs for. Extracted for the same reason as the rule above: the + * decision is worth pinning and the path that uses it cannot be driven from this suite. + * + * The consent row is the source of truth, as it already is on the header path, and the token's + * subject must agree with it. Losing the second half is the easy mistake, and it is silent: + * checkUKConsent compares the consent's mUserId against `consenter`, so deriving `consenter` + * from mUserId without also comparing the token would have that check compare the consent's user + * with itself and pass for anybody's token. + */ + scenario("the token path takes its PSU from the consent, and the token must agree", UKConsentScoping) { + val psu = "the-psu-user-id" + val someoneElse = "a-different-user-id" + + Given("a consent bound to a PSU") + Then("a token for that PSU resolves to them") + Consent.ukTokenPathPsuId(psu, psu) should equal(Right(psu)) + + And("a token for anyone else is refused, which is the check that must not be lost") + Consent.ukTokenPathPsuId(psu, someoneElse) should equal(Left(ErrorMessages.ConsentDoesNotMatchUser)) + + And("a consent naming no PSU was never authorised, so there is nobody it is on behalf of") + // Not a fallback to the token's user: that would serve everything that user can see, which is + // the whole of what a consent exists to narrow. + Consent.ukTokenPathPsuId("", psu) should equal(Left(ErrorMessages.ConsentNotFound)) + Consent.ukTokenPathPsuId(null, psu) should equal(Left(ErrorMessages.ConsentNotFound)) + Consent.ukTokenPathPsuId(" ", psu) should equal(Left(ErrorMessages.ConsentNotFound)) + } + scenario("the consent stays inspectable and revocable by the TPP that lodged it", UKConsentScoping) { val consentId = unresolvableConsent(List(ReadAccountsBasic), bindAccounts = false) val (principal, cc) = swapFor(consentId)