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 0b196fa554..69e4e36505 100644 --- a/obp-api/src/main/scala/code/api/util/ConsentUtil.scala +++ b/obp-api/src/main/scala/code/api/util/ConsentUtil.scala @@ -1048,6 +1048,35 @@ object Consent extends MdcLoggable { consentJwtBox.foreach(releaseVrpMandateArtefacts(consent, _)) } + /** + * The consent statuses after which an SCA challenge raised for the consent can no longer be used: + * a REVOKED consent is re-authenticated with a fresh challenge, and REJECTED and EXPIRED are terminal. + */ + val consentChallengeEndingStatuses: Set[ConsentStatus] = + Set(ConsentStatus.REVOKED, ConsentStatus.REJECTED, ConsentStatus.EXPIRED) + + /** + * Delete the OBP_CONSENT_CHALLENGE rows of a consent once none of them can be used any more: when + * one has been answered correctly, or when the consent reaches one of consentChallengeEndingStatuses. + * + * Nothing reads these rows after the answer -- the attempt counter only matters while a challenge + * is still open -- so the row is not an audit trail, it is a hashed one-time password waiting to + * be swept. Berlin Group consent challenges are left alone (their authorisation endpoints keep + * listing them), and so are transaction request challenges. + * + * Never fails the caller: by the time this runs the answer or the status change has happened, and + * a row left behind is logged rather than turned into an error for something that succeeded. + */ + def removeConsentChallenges(consentId: String): Unit = + code.transactionChallenge.Challenges.ChallengeProvider.vend + .deleteChallengesByConsentId(consentId, com.openbankproject.commons.model.enums.ChallengeType.OBP_CONSENT_CHALLENGE.toString) match { + case Failure(msg, ex, _) => + logger.error( + s"removeConsentChallenges: the challenges of consent $consentId could not be deleted: $msg" + + ex.map(e => s" (${e.getClass.getSimpleName}: ${e.getMessage})").getOrElse("")) + case _ => + } + /** * Release the artefacts a VRP mandate created, when the consent that owns them is revoked. * 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 3bcff4c290..25ab62a618 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 @@ -4590,6 +4590,12 @@ object Http4s510 { _ <- Helper.booleanToFuture(s"$InvalidChallengeAnswer", 403, Some(cc)) { challenge.scaStatus.contains(StrongCustomerAuthenticationStatus.finalised) } + // The answered challenge is spent (validateChallenge lets one correct answer through, + // once), and any other challenge the PSU started on this consent is superseded by it, so + // they are deleted now rather than left in the table for good. Done here and not after + // the consent is AUTHORISED: if the steps below refuse the request, the PSU has to start + // a new challenge either way. + _ <- Future(Consent.removeConsentChallenges(consentId)) // Bind the consented permissions to the PSU-selected accounts, replacing the // (bank_id=null, account_id=null) dead views createUKConsentJWT wrote at consent // creation time. diff --git a/obp-api/src/main/scala/code/consent/MappedConsent.scala b/obp-api/src/main/scala/code/consent/MappedConsent.scala index d6a87b18f3..2ef02ecace 100644 --- a/obp-api/src/main/scala/code/consent/MappedConsent.scala +++ b/obp-api/src/main/scala/code/consent/MappedConsent.scala @@ -61,11 +61,14 @@ object MappedConsentProvider extends ConsentProvider with code.util.Helper.MdcLo MappedConsent.find(By(MappedConsent.mConsentId, consentId)) match { case Full(consent) => Consent.expireAllPreviousValidBerlinGroupConsents(consent, status) - tryo(consent + val updated = tryo(consent .mStatus(status.toString) .mLastActionDate(now) //maybe not right, but for the create we use the `now`, we need to update it later. .saveMe() ) + if (updated.isDefined && Consent.consentChallengeEndingStatuses.contains(status)) + Consent.removeConsentChallenges(consentId) + updated case Empty => Empty ?~! ErrorMessages.ConsentNotFound case Failure(msg, _, _) => @@ -423,6 +426,7 @@ object MappedConsentProvider extends ConsentProvider with code.util.Helper.MdcLo ex.map(e => s" (${e.getClass.getSimpleName}: ${e.getMessage})").getOrElse("")) case _ => } + Consent.removeConsentChallenges(consentId) MappedConsent.find(By(MappedConsent.mConsentId, consentId)) } else Failure(ErrorMessages.ConsentAlreadyRevoked) diff --git a/obp-api/src/main/scala/code/scheduler/ConsentScheduler.scala b/obp-api/src/main/scala/code/scheduler/ConsentScheduler.scala index 82e893139b..be2f84bc4f 100644 --- a/obp-api/src/main/scala/code/scheduler/ConsentScheduler.scala +++ b/obp-api/src/main/scala/code/scheduler/ConsentScheduler.scala @@ -212,7 +212,8 @@ object ConsentScheduler extends MdcLoggable { case Success(_) => logger.debug("|---> Task executed successfully") } } - private def expiredUKConsents(): Unit = { + // private[code] so a test can run one sweep without waiting for the schedule. + private[code] def expiredUKConsents(): Unit = { Try { logger.debug("|---> Checking for expired UK Open Banking consents...") @@ -239,6 +240,9 @@ object ConsentScheduler extends MdcLoggable { if (rows > 0) { // An expired consent must give its granted access back, exactly as a revoked one does. Consent.revokeConsentAccountAccess(consent) + // Only UK consents are authorised through OBP_CONSENT_CHALLENGEs, so this is the one + // expiry sweep that can leave such a challenge behind. + Consent.removeConsentChallenges(consent.consentId) logger.warn(message) } else logger.debug(s"|---> Skipped stale update for UK consent ${consent.id}: status already changed") diff --git a/obp-api/src/main/scala/code/transactionChallenge/ChallengeProvider.scala b/obp-api/src/main/scala/code/transactionChallenge/ChallengeProvider.scala index d88f24cdfb..b22d08684b 100644 --- a/obp-api/src/main/scala/code/transactionChallenge/ChallengeProvider.scala +++ b/obp-api/src/main/scala/code/transactionChallenge/ChallengeProvider.scala @@ -60,6 +60,13 @@ trait ChallengeProvider { def getChallengesByConsentId(consentId: String): Box[List[ChallengeTrait]] def getChallengesByBasketId(basketId: String): Box[List[ChallengeTrait]] + /** + * Deletes the challenges of the given type that were raised for this consent. + * The type is part of the match because a consent's challenges are not all alike: the Berlin Group + * authorisation endpoints keep listing theirs after they are answered. + */ + def deleteChallengesByConsentId(consentId: String, challengeType: String): Box[Boolean] + /** * There is another method: Connector.validateChallengeAnswer, it validates the challenge over CBS. * This method, will validate the answer in OBP side. diff --git a/obp-api/src/main/scala/code/transactionChallenge/MappedChallengeProvider.scala b/obp-api/src/main/scala/code/transactionChallenge/MappedChallengeProvider.scala index c01b6b20b5..9535265ae0 100644 --- a/obp-api/src/main/scala/code/transactionChallenge/MappedChallengeProvider.scala +++ b/obp-api/src/main/scala/code/transactionChallenge/MappedChallengeProvider.scala @@ -102,6 +102,12 @@ object MappedChallengeProvider extends ChallengeProvider { Full(MappedExpectedChallengeAnswer.findAll(By(MappedExpectedChallengeAnswer.ConsentId,consentId))) override def getChallengesByBasketId(basketId: String): Box[List[ChallengeTrait]] = Full(MappedExpectedChallengeAnswer.findAll(By(MappedExpectedChallengeAnswer.BasketId,basketId))) + + override def deleteChallengesByConsentId(consentId: String, challengeType: String): Box[Boolean] = + tryo(MappedExpectedChallengeAnswer.bulkDelete_!!( + By(MappedExpectedChallengeAnswer.ConsentId, consentId), + By(MappedExpectedChallengeAnswer.ChallengeType, challengeType) + )) override def validateChallenge( challengeId: String, 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 96409f8b2e..313497facd 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 @@ -33,12 +33,15 @@ import code.api.util.ErrorMessages.{ConsentDoesNotMatchConsumer, ConsentDoesNotM import code.api.util.{CallContext, CertificateUtil, Consent} import code.consent.{ConsentStatus, Consents} import code.model.UserExtended -import code.transactionChallenge.Challenges +import code.scheduler.ConsentScheduler +import code.transactionChallenge.{Challenges, MappedChallengeProvider, MappedExpectedChallengeAnswer} import code.views.Views import com.openbankproject.commons.model.{BankIdAccountId, ErrorMessage, ViewId} import com.openbankproject.commons.model.enums.ChallengeType import com.openbankproject.commons.util.ApiVersion import net.liftweb.common.{Failure, Full} +import net.liftweb.mapper.By +import org.mindrot.jbcrypt.BCrypt import org.json4s._ import org.scalatest.Tag @@ -928,4 +931,128 @@ class UKOpenBankingV401AccountInfoTests extends UKOpenBankingV401ServerSetup { storedConsent(consentId).status should equal(ConsentStatus.AWAITINGAUTHORISATION.toString) } } + + // A consent's OBP_CONSENT_CHALLENGE rows are deleted once none of them can be used any more: after + // a correct answer, and when the consent becomes REVOKED, REJECTED or EXPIRED. A wrong answer keeps + // its row, because the attempt counter on it is what limits the next guesses. Rows of other types + // on the same consent stay: the Berlin Group authorisation endpoints list theirs after answering. + private def consentChallengesOf(consentId: String, challengeType: ChallengeType.Value) = + MappedExpectedChallengeAnswer.findAll( + By(MappedExpectedChallengeAnswer.ConsentId, consentId), + By(MappedExpectedChallengeAnswer.ChallengeType, challengeType.toString)) + private def seedConsentChallenge(consentId: String, challengeType: ChallengeType.Value): String = { + val challengeId = java.util.UUID.randomUUID.toString + val salt = BCrypt.gensalt() + MappedChallengeProvider.saveChallenge( + challengeId = challengeId, + transactionRequestId = "", + salt = salt, + expectedAnswer = BCrypt.hashpw("123", salt).substring(0, 44), + expectedUserId = resourceUser1.userId, + scaMethod = None, + scaStatus = None, + consentId = Some(consentId), + basketId = None, + authenticationMethodId = None, + challengeType = challengeType.toString + ).openOrThrowException(s"challenge for consent $consentId") + challengeId + } + + feature("UKOB v4.0.1 a consent's SCA challenges are deleted once they can no longer be used") { + scenario("a correct answer deletes the consent's challenges, other challenge types stay", UKOpenBankingV401AccountInfo) { + setPropsValues("suggested_default_sca_method" -> "DUMMY") + val consentId = createUKConsent(None, Some(new java.util.Date(System.currentTimeMillis() + 3600000L))) + + val challenge = makePostRequest(scaChallengeRequest(consentId) <@ (user1), "") + challenge.code should equal(200) + val challengeId = (challenge.body \ "challenge_id").extract[String] + // A second challenge the PSU started and never answered, and a Berlin Group one on the same id. + seedConsentChallenge(consentId, ChallengeType.OBP_CONSENT_CHALLENGE) + seedConsentChallenge(consentId, ChallengeType.BERLIN_GROUP_CONSENT_CHALLENGE) + consentChallengesOf(consentId, ChallengeType.OBP_CONSENT_CHALLENGE).size should equal(2) + + When("the PSU answers the challenge correctly") + val authorised = makePostRequest(authoriseRequest(consentId) <@ (user1), + s"""{"account_ids":["$acc"],"challenge_id":"$challengeId","answer":"123"}""") + authorised.code should equal(200) + storedConsent(consentId).status should equal(ConsentStatus.AUTHORISED.toString) + + Then("both OBP consent challenges are gone, and the Berlin Group one is still there") + consentChallengesOf(consentId, ChallengeType.OBP_CONSENT_CHALLENGE) should be(empty) + Challenges.ChallengeProvider.vend.getChallenge(challengeId).isDefined should equal(false) + consentChallengesOf(consentId, ChallengeType.BERLIN_GROUP_CONSENT_CHALLENGE).size should equal(1) + } + scenario("a wrong answer keeps the challenge and its attempt count", UKOpenBankingV401AccountInfo) { + setPropsValues("suggested_default_sca_method" -> "DUMMY") + val consentId = createUKConsent(None, Some(new java.util.Date(System.currentTimeMillis() + 3600000L))) + + val challenge = makePostRequest(scaChallengeRequest(consentId) <@ (user1), "") + challenge.code should equal(200) + val challengeId = (challenge.body \ "challenge_id").extract[String] + + val refused = makePostRequest(authoriseRequest(consentId) <@ (user1), + s"""{"account_ids":["$acc"],"challenge_id":"$challengeId","answer":"999"}""") + refused.code should not equal (200) + + val stored = Challenges.ChallengeProvider.vend.getChallenge(challengeId) + .openOrThrowException(s"challenge $challengeId") + stored.attemptCounter should equal(1) + stored.successful should equal(false) + storedConsent(consentId).status should equal(ConsentStatus.AWAITINGAUTHORISATION.toString) + } + scenario("a correct answer that is then refused still spends the challenge", UKOpenBankingV401AccountInfo) { + setPropsValues("suggested_default_sca_method" -> "DUMMY") + val consentId = createUKConsent(None, Some(new java.util.Date(System.currentTimeMillis() + 3600000L))) + + // user2 does not hold testAccountId1, so the authorisation is refused after the answer is checked. + val challenge = makePostRequest(scaChallengeRequest(consentId) <@ (user2), "") + challenge.code should equal(200) + val challengeId = (challenge.body \ "challenge_id").extract[String] + + val refused = makePostRequest(authoriseRequest(consentId) <@ (user2), + s"""{"account_ids":["$acc"],"challenge_id":"$challengeId","answer":"123"}""") + refused.code should equal(403) + + consentChallengesOf(consentId, ChallengeType.OBP_CONSENT_CHALLENGE) should be(empty) + storedConsent(consentId).status should equal(ConsentStatus.AWAITINGAUTHORISATION.toString) + } + scenario("REJECTED, REVOKED and EXPIRED delete the consent's challenges, AUTHORISED does not", UKOpenBankingV401AccountInfo) { + val expiry = Some(new java.util.Date(System.currentTimeMillis() + 3600000L)) + + val authorised = createUKConsent(None, expiry) + seedConsentChallenge(authorised, ChallengeType.OBP_CONSENT_CHALLENGE) + Consents.consentProvider.vend.updateConsentStatus(authorised, ConsentStatus.AUTHORISED) + consentChallengesOf(authorised, ChallengeType.OBP_CONSENT_CHALLENGE).size should equal(1) + + // REJECTED is what the management endpoint PUT /management/banks/BANK_ID/consents/CONSENT_ID sets. + val rejected = createUKConsent(None, expiry) + seedConsentChallenge(rejected, ChallengeType.OBP_CONSENT_CHALLENGE) + seedConsentChallenge(rejected, ChallengeType.BERLIN_GROUP_CONSENT_CHALLENGE) + Consents.consentProvider.vend.updateConsentStatus(rejected, ConsentStatus.REJECTED) + consentChallengesOf(rejected, ChallengeType.OBP_CONSENT_CHALLENGE) should be(empty) + consentChallengesOf(rejected, ChallengeType.BERLIN_GROUP_CONSENT_CHALLENGE).size should equal(1) + + // Every revoke endpoint, the UK DELETE included, goes through ConsentProvider.revoke. + val revoked = createUKConsent(None, expiry) + seedConsentChallenge(revoked, ChallengeType.OBP_CONSENT_CHALLENGE) + Consents.consentProvider.vend.revoke(revoked).isDefined should equal(true) + consentChallengesOf(revoked, ChallengeType.OBP_CONSENT_CHALLENGE) should be(empty) + + val expired = createUKConsent(None, expiry) + seedConsentChallenge(expired, ChallengeType.OBP_CONSENT_CHALLENGE) + Consents.consentProvider.vend.updateConsentStatus(expired, ConsentStatus.EXPIRED) + consentChallengesOf(expired, ChallengeType.OBP_CONSENT_CHALLENGE) should be(empty) + } + scenario("the UK expiry sweep deletes the challenges of the consents it expires", UKOpenBankingV401AccountInfo) { + val consentId = createUKConsent(Some(resourceUser1), Some(new java.util.Date(System.currentTimeMillis() - 60000L))) + Consents.consentProvider.vend.updateConsentStatus(consentId, ConsentStatus.AUTHORISED) + seedConsentChallenge(consentId, ChallengeType.OBP_CONSENT_CHALLENGE) + + ConsentScheduler.expiredUKConsents() + + storedConsent(consentId).status should equal(ConsentStatus.EXPIRED.toString) + consentChallengesOf(consentId, ChallengeType.OBP_CONSENT_CHALLENGE) should be(empty) + } + } }