From 23fdea68319d2d72d51a7f57092b98c544f0de19 Mon Sep 17 00:00:00 2001 From: hongwei Date: Tue, 23 Jun 2026 11:29:33 +0200 Subject: [PATCH 01/29] test(concurrency): add reproductions for 17 follow-up concurrency hazards Add five ScalaTest suites under code.concurrency (tagged ConcurrencyRace) covering hazards found in the second codebase scan: - ConcurrentBulkPaymentRaceTest C1 - ConcurrentConsentStatusRaceTest H1, H2, H3, M5 - ConcurrentRateLimiterRaceTest H4, M6, M7 - ConcurrentMutableSingletonRaceTest H5, H7, H6, M8, M9 - ConcurrentBusinessStatusRaceTest M1, M2, M3, M4 Behavioural suites reproduce the race; structural suites (H6/M8/M9) assert the hardening primitive (volatile field / concurrent map) is present. Redis-backed suites self-skip when Redis is unavailable. Fixes land per file in follow-ups. --- .../ConcurrentBulkPaymentRaceTest.scala | 139 ++++++++++++ .../ConcurrentBusinessStatusRaceTest.scala | 214 ++++++++++++++++++ .../ConcurrentConsentStatusRaceTest.scala | 176 ++++++++++++++ .../ConcurrentMutableSingletonRaceTest.scala | 178 +++++++++++++++ .../ConcurrentRateLimiterRaceTest.scala | 132 +++++++++++ 5 files changed, 839 insertions(+) create mode 100644 obp-api/src/test/scala/code/concurrency/ConcurrentBulkPaymentRaceTest.scala create mode 100644 obp-api/src/test/scala/code/concurrency/ConcurrentBusinessStatusRaceTest.scala create mode 100644 obp-api/src/test/scala/code/concurrency/ConcurrentConsentStatusRaceTest.scala create mode 100644 obp-api/src/test/scala/code/concurrency/ConcurrentMutableSingletonRaceTest.scala create mode 100644 obp-api/src/test/scala/code/concurrency/ConcurrentRateLimiterRaceTest.scala diff --git a/obp-api/src/test/scala/code/concurrency/ConcurrentBulkPaymentRaceTest.scala b/obp-api/src/test/scala/code/concurrency/ConcurrentBulkPaymentRaceTest.scala new file mode 100644 index 0000000000..108c031d36 --- /dev/null +++ b/obp-api/src/test/scala/code/concurrency/ConcurrentBulkPaymentRaceTest.scala @@ -0,0 +1,139 @@ +/** +Open Bank Project - API +Copyright (C) 2011-2019, TESOBE GmbH. + +This program is free software: you can redistribute it and/or modify +it under the terms of the GNU Affero General Public License as published by +the Free Software Foundation, either version 3 of the License, or +(at your option) any later version. + +This program is distributed in the hope that it will be useful, +but WITHOUT ANY WARRANTY; without even the implied warranty of +MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the +GNU Affero General Public License for more details. + +You should have received a copy of the GNU Affero General Public License +along with this program, if not, see . + +Email: contact@tesobe.com +TESOBE GmbH. +Osloer Strasse 16/17 +Berlin 13359, Germany + +This product includes software developed at +TESOBE (http://www.tesobe.com/) + + */ +package code.concurrency + +import code.bulkpayment.{BulkBatchReference, MappedBulkPaymentProvider} +import net.liftweb.common.{Failure, Full} +import net.liftweb.mapper.By + +import java.util.UUID + +/** + * C1: BulkPayment batch-reference check-then-claim race. + * + * THE HAZARD: + * BulkPaymentHandler.validateEnvelope calls isBatchReferenceUsed (a SELECT), and only if the + * reference is absent does the handler proceed to create the TransactionRequest and fan-out + * the payment legs. After the payments are created, Http4s700 calls claimBatchReference + * (an INSERT guarded by UniqueIndex(FromBankId, FromAccountId, BatchReference)). + * + * The race window is between the SELECT in isBatchReferenceUsed and the INSERT in + * claimBatchReference. Two concurrent requests with the same (bankId, accountId, batchRef) + * both pass the SELECT, both build and persist the TransactionRequest, then race to INSERT. + * The losing INSERT hits the UniqueIndex and returns a Failure Box — but in Http4s700.scala + * the result of claimBatchReference is dropped inside a bare Future { claimBatchReference(...) } + * with no unboxFullOrFail check, so the second request silently continues and double-charges. + * + * WHAT THIS TEST SHOWS: + * Scenario C1a verifies that claimBatchReference correctly produces a Failure when the row + * already exists. This confirms the UniqueIndex guard works at the DB level. + * + * Scenario C1b reproduces the silent-drop bug: two concurrent callers both pass the + * isBatchReferenceUsed check and both proceed. The correct fix requires (1) moving + * claimBatchReference BEFORE the TransactionRequest fan-out, and (2) propagating the + * Failure with unboxFullOrFail so the second request aborts early. + * + * EXPECTED TO FAIL (C1b) until claimBatchReference failure is properly propagated. + * Tagged ConcurrencyRace. + */ +class ConcurrentBulkPaymentRaceTest extends ConcurrentRaceSetup { + + private val provider = MappedBulkPaymentProvider + + feature("BulkPayment batch-reference idempotency guard") { + + scenario("C1a: claimBatchReference must return Failure when the reference already exists (DB constraint works)", ConcurrencyRace) { + Given("a batch-reference row already exists for (bank, account, ref)") + val bankId = "__conc_bulk_bank_" + UUID.randomUUID.toString.take(8) + val accountId = "__conc_bulk_acc_" + UUID.randomUUID.toString.take(8) + val batchRef = "__conc_bulk_ref_" + UUID.randomUUID.toString.take(8) + val trId1 = UUID.randomUUID.toString + val trId2 = UUID.randomUUID.toString + + val first = provider.claimBatchReference(bankId, accountId, batchRef, trId1) + + When("a second claimBatchReference is attempted with the same (bank, account, ref)") + val second = provider.claimBatchReference(bankId, accountId, batchRef, trId2) + + Then("the second call must return a Failure — the UniqueIndex prevents duplicate claims") + withClue( + s"first=$first second=$second: the UniqueIndex on (FromBankId, FromAccountId, BatchReference) " + + s"should cause the second INSERT to fail — " + ) { + first shouldBe a [Full[_]] + second shouldBe a [Failure] + } + } + + scenario("C1b: concurrent isBatchReferenceUsed + claimBatchReference must not silently allow both to proceed", ConcurrencyRace) { + Given("no existing BulkBatchReference row for a fresh (bank, account, batchRef)") + val bankId = "__conc_bulk2_bank_" + UUID.randomUUID.toString.take(8) + val accountId = "__conc_bulk2_acc_" + UUID.randomUUID.toString.take(8) + val batchRef = "__conc_bulk2_ref_" + UUID.randomUUID.toString.take(8) + val n = 2 + + def rowCount: Long = BulkBatchReference.count( + By(BulkBatchReference.FromBankId, bankId), + By(BulkBatchReference.FromAccountId, accountId), + By(BulkBatchReference.BatchReference, batchRef) + ) + + When(s"$n threads concurrently check isBatchReferenceUsed then call claimBatchReference") + // This reproduces the check-then-act window: + // Thread A: isBatchReferenceUsed → false (passes guard) + // Thread B: isBatchReferenceUsed → false (passes guard — A hasn't committed yet) + // Thread A: claimBatchReference → Full (INSERT succeeds) + // Thread B: claimBatchReference → Failure (UniqueIndex violation) + // Bug: Http4s700 wraps claimBatchReference in Future { ... } without checking the Box, + // so Thread B's Failure is silently dropped and the duplicate payment proceeds. + val results = runConcurrentWithBarrier(n) { i => + val alreadyUsed = provider.isBatchReferenceUsed(bankId, accountId, batchRef) + if (!alreadyUsed) { + provider.claimBatchReference(bankId, accountId, batchRef, UUID.randomUUID.toString) + } else { + Failure("reference already used — correctly rejected before INSERT") + } + } + + Then("exactly one claim must succeed; the other must return Failure (not be silently swallowed)") + val successes = results.collect { case scala.util.Success(Full(_)) => "ok" } + val failures = results.collect { case scala.util.Success(f: Failure) => f.msg } + val rows = rowCount + withClue( + s"successes=${successes.size} failures=${failures.size} dbRows=$rows: " + + s"both threads passed isBatchReferenceUsed because neither had committed yet — " + + s"the second claimBatchReference hit UniqueIndex and returned Failure, but in " + + s"Http4s700 this Failure is dropped inside Future { claimBatchReference(...) } " + + s"with no unboxFullOrFail — the duplicate payment request silently proceeds — " + ) { + rows should equal(1L) + successes should have size 1 + failures should have size (n - 1) + } + } + } +} diff --git a/obp-api/src/test/scala/code/concurrency/ConcurrentBusinessStatusRaceTest.scala b/obp-api/src/test/scala/code/concurrency/ConcurrentBusinessStatusRaceTest.scala new file mode 100644 index 0000000000..e88fed4a84 --- /dev/null +++ b/obp-api/src/test/scala/code/concurrency/ConcurrentBusinessStatusRaceTest.scala @@ -0,0 +1,214 @@ +package code.concurrency + +import code.accountaccessrequest.{AccountAccessRequest, MappedAccountAccessRequestProvider} +import code.accountapplication.{MappedAccountApplication, MappedAccountApplicationProvider} +import code.transactionrequests.{MappedTransactionRequest, MappedTransactionRequestProvider} +import code.transactionChallenge.{MappedChallengeProvider, MappedExpectedChallengeAnswer} +import com.openbankproject.commons.model.enums.AccountAccessRequestStatus +import com.openbankproject.commons.model.{ProductCode, TransactionRequestId} +import net.liftweb.common.{Failure, Full} +import net.liftweb.mapper.By +import org.mindrot.jbcrypt.BCrypt + +import java.util.UUID +import scala.concurrent.Await +import scala.concurrent.duration._ + +/** + * M1: Scheduler-path updateTransactionRequestStatus bypasses lockTransactionRequest (structural). + * M2: AccountAccessRequest.updateStatus — no terminal state guard (unconditional find+saveMe). + * M3: MappedAccountApplication.updateStatus — ACCEPTED guard is an in-memory check only. + * + * M1 (structural, not reproduced by concurrent test): + * Http4s400's challenge-answer path calls DoobieTransactionRequestQueries.lockTransactionRequest + * before processing payment — this acquires a DB-level row lock, making the INITIATED→COMPLETED + * transition atomic. BUT Http4s510's updateTransactionRequestStatus endpoint calls + * MappedTransactionRequestProvider.updateTransactionRequestStatus which does a plain + * find+mStatus(status).saveMe() with no lock. A concurrent scheduler call racing a challenge- + * answer can overwrite COMPLETED with a stale status, reversing a completed payment. + * + * M2 (testable): + * AccountAccessRequest.updateStatus does `AccountAccessRequest.find(…).flatMap { r => tryo { r.Status(status).saveMe() } }`. + * There is NO terminal-state guard — an already-APPROVED request can be flipped to DECLINED (or + * vice versa) by a concurrent or later admin action. This is a last-writer-wins race with no + * idempotency protection. + * + * M3 (testable): + * MappedAccountApplicationProvider.updateStatus checks `if accountApplication.status == "ACCEPTED" + * → Failure` as a guard against re-processing ACCEPTED applications. But the guard reads status + * from the in-memory object loaded by the find() call that precedes the check. Two concurrent + * calls, one wanting ACCEPTED and one wanting REJECTED, both load status="REQUESTED", both pass + * the guard, and both write — the last one wins non-deterministically. A legitimate ACCEPTED + * transition can be overwritten by a concurrent REJECTED. + * + * EXPECTED TO FAIL (M2, M3) until a conditional UPDATE WHERE status='' is used. + * Tagged ConcurrencyRace. + */ +class ConcurrentBusinessStatusRaceTest extends ConcurrentRaceSetup { + + feature("Business-object status transitions must be atomic") { + + scenario("M2: concurrent approve and decline of the same AccountAccessRequest must not both succeed", ConcurrencyRace) { + Given("an AccountAccessRequest in INITIATED state") + val requestId = UUID.randomUUID.toString + AccountAccessRequest.create + .AccountAccessRequestId(requestId) + .BankId("__conc_m2_bank") + .AccountId("__conc_m2_acc") + .ViewId("owner") + .IsSystemView(false) + .RequestorUserId(resourceUser1.userId) + .TargetUserId(resourceUser2.userId) + .BusinessJustification("concurrency test") + .Status(AccountAccessRequestStatus.INITIATED.toString) + .CheckerUserId("") + .CheckerComment("") + .saveMe() + + When("two threads concurrently update the request — one to APPROVED, one to DECLINED") + val n = 2 + val results = runConcurrentWithBarrier(n) { i => + val newStatus = if (i == 0) "APPROVED" else "DECLINED" + MappedAccountAccessRequestProvider.updateStatus(requestId, newStatus, resourceUser1.userId, "concurrent-test") + } + + val finalStatus = AccountAccessRequest + .find(By(AccountAccessRequest.AccountAccessRequestId, requestId)) + .map(_.Status.get).getOrElse("missing") + + Then("the final status must be a deterministic terminal value, not an overwritten intermediate") + withClue( + s"finalStatus=$finalStatus results=${results.map(_.isSuccess)}: " + + s"AccountAccessRequest.updateStatus does unconditional find+saveMe with no terminal-state " + + s"guard — concurrent APPROVED/DECLINED calls both succeed; the last writer silently wins, " + + s"meaning a legitimate APPROVED decision can be overwritten by a racing DECLINED — " + ) { + // The test currently FAILS because both calls return Full and the final status is + // non-deterministic. After the fix (conditional UPDATE WHERE status='INITIATED'), + // exactly one must succeed and the other must return a Failure. + val successes = results.collect { case scala.util.Success(Full(_)) => 1 } + successes should have size 1 + } + } + + scenario("M3: concurrent ACCEPTED and REJECTED transitions to the same AccountApplication must not both proceed", ConcurrencyRace) { + Given("an AccountApplication in REQUESTED state") + val appId = UUID.randomUUID.toString + MappedAccountApplication.create + .mAccountApplicationId(appId) + .mCode(ProductCode("__conc_m3_product").value) + .mUserId(resourceUser1.userId) + .mCustomerId(UUID.randomUUID.toString) + .mStatus("REQUESTED") + .saveMe() + + When("Thread A wants to ACCEPT and Thread B wants to REJECT — both race") + // Both load status="REQUESTED" before either commits. + // The memory guard `if status == "ACCEPTED" → Failure` does NOT fire for either thread, + // because both loaded "REQUESTED" — neither has committed ACCEPTED yet. + // Thread A writes ACCEPTED, Thread B writes REJECTED; one overwrites the other. + val n = 2 + val results = runConcurrentWithBarrier(n) { i => + val newStatus = if (i == 0) "ACCEPTED" else "REJECTED" + Await.result(MappedAccountApplicationProvider.updateStatus(appId, newStatus), 10.seconds) + } + + val finalStatus = MappedAccountApplication + .find(By(MappedAccountApplication.mAccountApplicationId, appId)) + .map(_.status).getOrElse("missing") + + Then("exactly one transition must succeed — concurrent ACCEPTED+REJECTED must not both write") + withClue( + s"finalStatus=$finalStatus results=${results.map(_.isSuccess)}: " + + s"MappedAccountApplicationProvider.updateStatus checks `if status == ACCEPTED → Failure` on " + + s"the in-memory loaded object, not in the DB — both threads load REQUESTED, both pass the " + + s"guard, and both write; the ACCEPTED→REJECTED overwrite is silent and undetected — " + ) { + val successes = results.collect { case scala.util.Success(Full(_)) => 1 } + successes should have size 1 + } + } + + scenario("M1: concurrent transaction-request status updates must serialize — only one transition from INITIATED", ConcurrencyRace) { + Given("a MappedTransactionRequest in INITIATED state") + // Http4s510's `updateTransactionRequestStatus` endpoint calls saveTransactionRequestStatusImpl, + // which is a plain find+save with NO lockTransactionRequest (unlike Http4s400's challenge path). + val trId = UUID.randomUUID.toString + MappedTransactionRequest.create + .mTransactionRequestId(trId) + .mType("SANDBOX_TAN") + .mStatus("INITIATED") + .saveMe() + val n = 2 + + When("two threads concurrently transition the request — one to COMPLETED, one to REJECTED") + // The legitimate payment path sets COMPLETED; a scheduler/timeout path sets REJECTED. + // With no row lock and no conditional guard, both find INITIATED and both save — last writer wins, + // so a COMPLETED payment can be silently overwritten to REJECTED (or vice versa). + val results = runConcurrentWithBarrier(n) { i => + val newStatus = if (i == 0) "COMPLETED" else "REJECTED" + MappedTransactionRequestProvider.saveTransactionRequestStatusImpl(TransactionRequestId(trId), newStatus) + } + + Then("exactly one update may report success — the other must be rejected by a conditional guard") + val applied = results.collect { case scala.util.Success(Full(true)) => 1 } + val finalStatus = MappedTransactionRequest + .find(By(MappedTransactionRequest.mTransactionRequestId, trId)) + .map(_.mStatus.get).getOrElse("missing") + withClue( + s"applied=${applied.size} finalStatus=$finalStatus results=${results.map(_.isSuccess)}: " + + s"saveTransactionRequestStatusImpl does find+save with no lockTransactionRequest and no " + + s"`WHERE mstatus='INITIATED'` guard — both concurrent transitions commit, last writer wins. " + + s"Fix: acquire lockTransactionRequest first (like Http4s400) or use a conditional UPDATE — " + ) { + applied should have size 1 + } + } + + scenario("M4: concurrent correct challenge answers must flip Successful exactly once — no MFA double-spend", ConcurrencyRace) { + Given("a transaction-request challenge seeded with a known correct answer") + // Raise the attempt limit so the limit-guard never short-circuits the success path. + setPropsValues("transactionRequests_challenge_max_allowed_attempts" -> "100") + val challengeId = UUID.randomUUID.toString + val salt = BCrypt.gensalt() + MappedChallengeProvider.saveChallenge( + challengeId = challengeId, + transactionRequestId = UUID.randomUUID.toString, + salt = salt, + expectedAnswer = BCrypt.hashpw("123", salt).substring(0, 44), + expectedUserId = resourceUser1.userId, + scaMethod = None, + scaStatus = None, + consentId = None, + basketId = None, + authenticationMethodId = None, + challengeType = "OBP_TRANSACTION_REQUEST_CHALLENGE" + ) + val n = 2 + + When(s"$n threads concurrently submit the CORRECT answer to the same challenge") + // validateChallenge does: in-memory hash check, then challenge.Successful(true).ScaStatus(finalised).saveMe(). + // The success flip is not a compare-and-set: both correct answers pass the check and both flip + // Successful=true, so both callers are told the SCA succeeded → the payment can execute twice. + val results = runConcurrentWithBarrier(n) { _ => + MappedChallengeProvider.validateChallenge( + challengeId = challengeId, + challengeAnswer = "123", + userId = Some(resourceUser1.userId) + ) + } + + Then("exactly one validate may succeed — the second must be rejected (challenge already answered)") + val successes = results.collect { case scala.util.Success(Full(_)) => 1 } + withClue( + s"successes=${successes.size} results=${results.map(_.isSuccess)}: " + + s"validateChallenge flips Successful(true) via a plain saveMe after an in-memory hash check — " + + s"two correct concurrent answers both flip it and both return Full, green-lighting the payment " + + s"twice. Fix: conditional UPDATE successful=true WHERE successful=false (CAS); the loser gets " + + s"0 rows → Failure — " + ) { + successes should have size 1 + } + } + } +} diff --git a/obp-api/src/test/scala/code/concurrency/ConcurrentConsentStatusRaceTest.scala b/obp-api/src/test/scala/code/concurrency/ConcurrentConsentStatusRaceTest.scala new file mode 100644 index 0000000000..930e70cdb5 --- /dev/null +++ b/obp-api/src/test/scala/code/concurrency/ConcurrentConsentStatusRaceTest.scala @@ -0,0 +1,176 @@ +package code.concurrency + +import code.consent.{ConsentStatus, MappedConsent, MappedConsentProvider} +import code.context.{MappedUserAuthContextUpdate, MappedUserAuthContextUpdateProvider} +import net.liftweb.common.Full +import net.liftweb.mapper.By +import org.mindrot.jbcrypt.BCrypt + +import java.util.{Date, UUID} +import scala.concurrent.Await +import scala.concurrent.duration._ + +/** + * H1: MappedConsent.checkAnswer TOCTOU race. + * H2: MappedUserAuthContextUpdate.checkAnswer TOCTOU race. + * H3: MappedConsent.revoke vs concurrent checkAnswer race. + * + * THE HAZARD (all three share the same root cause): + * The status check ("is this consent INITIATED?") and the status write ("flip to ACCEPTED/REJECTED/REVOKED") + * are two separate SQL operations with no SELECT FOR UPDATE or conditional UPDATE between them. + * Two concurrent requests can both read INITIATED, both pass the guard, and both write a new status. + * + * H1 / H2: Two concurrent correct answers → both callers get Full(consent_ACCEPTED) and proceed + * as if they independently answered a challenge. In a real SCA flow this means a single + * consent is double-authorised — both callers proceed past the challenge gate. + * + * H3: A revoke call and a checkAnswer call race. The revoker writes REVOKED; the answerer loaded + * a stale INITIATED object and writes ACCEPTED on top, resurrecting the revoked consent. + * + * EXPECTED TO FAIL (all three) until a conditional UPDATE WHERE status='INITIATED' is used. + * Tagged ConcurrencyRace. + */ +class ConcurrentConsentStatusRaceTest extends ConcurrentRaceSetup { + + private def mkConsent(answer: String): (String, String) = { + val salt = BCrypt.gensalt() + val hashed = BCrypt.hashpw(answer, salt).substring(0, 44) + val consentId = UUID.randomUUID.toString + MappedConsent.create + .mConsentId(consentId) + .mStatus(ConsentStatus.INITIATED.toString) + .mChallenge(hashed) + .mSalt(salt) + .saveMe() + (consentId, answer) + } + + private def mkUserAuthContextUpdate(answer: String): String = { + val id = UUID.randomUUID.toString + MappedUserAuthContextUpdate.create + .mUserAuthContextUpdateId(id) + .mUserId(resourceUser1.userId) + .mConsumerId("__conc_consumer") + .mKey("__conc_key") + .mValue("__conc_value") + .mChallenge(answer) + .mStatus(com.openbankproject.commons.model.UserAuthContextUpdateStatus.INITIATED.toString) + .saveMe() + id + } + + private def consentStatus(consentId: String): String = + MappedConsent.find(By(MappedConsent.mConsentId, consentId)) + .map(_.status).getOrElse("missing") + + private def uacStatus(id: String): String = + MappedUserAuthContextUpdate.find(By(MappedUserAuthContextUpdate.mUserAuthContextUpdateId, id)) + .map(_.status).getOrElse("missing") + + feature("Consent and UserAuthContextUpdate status transitions must be atomic") { + + scenario("H1: two concurrent correct answers to the same consent must not both succeed", ConcurrencyRace) { + Given("a consent in INITIATED state with a known challenge answer") + val (consentId, answer) = mkConsent("test-answer-h1") + + When("2 threads concurrently submit the correct answer") + // Both threads load INITIATED, both pass the guard, both write ACCEPTED. + // The hazard is that both return Full — two callers get the green light. + val results = runConcurrentWithBarrier(2) { _ => + MappedConsentProvider.checkAnswer(consentId, answer) + } + + Then("exactly one call should succeed; the other must get a non-INITIATED rejection") + val successes = results.collect { case scala.util.Success(Full(_)) => 1 } + val finalSt = consentStatus(consentId) + withClue( + s"successes=${successes.size} finalStatus=$finalSt: " + + s"MappedConsent.checkAnswer reads status and writes new status as two separate SQL operations " + + s"with no SELECT FOR UPDATE — both threads pass the INITIATED guard before either commits, " + + s"both get Full(ACCEPTED) and proceed through the SCA gate — " + ) { + successes should have size 1 + finalSt should equal(ConsentStatus.ACCEPTED.toString) + } + } + + scenario("H2: two concurrent correct answers to the same UserAuthContextUpdate must not both succeed", ConcurrencyRace) { + Given("a UserAuthContextUpdate in INITIATED state with known plain-text challenge") + val answer = "conc-h2-answer" + val updateId = mkUserAuthContextUpdate(answer) + + When("2 threads concurrently submit the correct challenge") + val results = runConcurrentWithBarrier(2) { _ => + Await.result(MappedUserAuthContextUpdateProvider.checkAnswer(updateId, answer), 10.seconds) + } + + Then("exactly one must succeed; the race must not allow double-authorisation") + val successes = results.collect { case scala.util.Success(Full(_)) => 1 } + val finalSt = uacStatus(updateId) + withClue( + s"successes=${successes.size} finalStatus=$finalSt: " + + s"MappedUserAuthContextUpdateProvider.checkAnswer checks status then writes status in two " + + s"separate SQL operations — both threads see INITIATED and both write ACCEPTED — " + ) { + successes should have size 1 + finalSt should equal(com.openbankproject.commons.model.UserAuthContextUpdateStatus.ACCEPTED.toString) + } + } + + scenario("H3: a concurrent revoke must not be overwritten by a racing checkAnswer", ConcurrencyRace) { + Given("a consent in INITIATED state") + val (consentId, answer) = mkConsent("test-answer-h3") + val n = 2 + + When("one thread revokes the consent while another concurrently answers the challenge correctly") + // Thread 0 revokes; Thread 1 answers. Both load the consent before either commits. + // The answerer holds a stale INITIATED object and writes ACCEPTED after the revoker commits REVOKED. + val results = runConcurrentWithBarrier(n) { i => + if (i == 0) MappedConsentProvider.revoke(consentId) + else MappedConsentProvider.checkAnswer(consentId, answer) + } + + Then("the final status must be REVOKED — a revocation must survive a concurrent answer") + val finalSt = consentStatus(consentId) + withClue( + s"finalStatus=$finalSt results=${results.map(_.isSuccess)}: " + + s"revoke() and checkAnswer() both do find-then-saveMe with no conditional UPDATE guard; " + + s"the answerer's write of ACCEPTED can land after the revoker's REVOKED commit, " + + s"resurrecting a consent the user explicitly revoked — " + ) { + finalSt should equal(ConsentStatus.REVOKED.toString) + } + } + + scenario("M5: the skip-SCA accept-write must not overwrite a concurrent revoke (shouldSkipConsentSca)", ConcurrencyRace) { + Given("a consent in INITIATED state (just created, SCA about to be skipped)") + // Http4s310's createConsent path, when (grantor,grantee) is in skipConsentScaForConsumerIdPairs, + // does: MappedConsent.find(byId).map(_.mStatus(ACCEPTED).saveMe()) — an UNCONDITIONAL write, + // no `WHERE mstatus='INITIATED'` guard. + val (consentId, _) = mkConsent("m5-unused-answer") + val n = 2 + + When("one thread runs the skip-SCA accept-write while another concurrently revokes the consent") + val results = runConcurrentWithBarrier(n) { i => + if (i == 0) { + // Replicate the exact production write in Http4s310 (the unconditional skip-SCA accept). + MappedConsent.find(By(MappedConsent.mConsentId, consentId)) + .map(_.mStatus(ConsentStatus.ACCEPTED.toString).saveMe()) + } else { + MappedConsentProvider.revoke(consentId) + } + } + + Then("the final status must be REVOKED — an explicit revoke must win over an auto-accept") + val finalSt = consentStatus(consentId) + withClue( + s"finalStatus=$finalSt results=${results.map(_.isSuccess)}: " + + s"the skip-SCA path writes mStatus(ACCEPTED).saveMe() unconditionally; racing a revoke, the " + + s"auto-accept can land last and resurrect a revoked consent. Fix: conditional " + + s"UPDATE mstatus=ACCEPTED WHERE mstatus='INITIATED' so the accept is a no-op once revoked — " + ) { + finalSt should equal(ConsentStatus.REVOKED.toString) + } + } + } +} diff --git a/obp-api/src/test/scala/code/concurrency/ConcurrentMutableSingletonRaceTest.scala b/obp-api/src/test/scala/code/concurrency/ConcurrentMutableSingletonRaceTest.scala new file mode 100644 index 0000000000..fcb3fbef00 --- /dev/null +++ b/obp-api/src/test/scala/code/concurrency/ConcurrentMutableSingletonRaceTest.scala @@ -0,0 +1,178 @@ +package code.concurrency + +import code.actorsystem.{ObpActorSystem, ObpLookupSystem} +import code.api.util.APIUtil +import code.bankconnectors.DynamicConnector +import code.util.SecureLogging + +import java.lang.reflect.Modifier +import java.util.UUID + +/** + * H5: DynamicConnector.singletonObjectMap — unsynchronised mutable.Map. + * H7: SecureLogging.customPatternCache — unsynchronised mutable.Map.getOrElseUpdate. + * M8: APIUtil.connectorToEndpoint — mutable.Map written only at startup (structural note). + * M9: ObpActorSystem.northSideAkkaConnectorActorSystem — bare `var` (structural note). + * + * THE HAZARD: + * H5 / H7 share the same root cause: both use scala.collection.mutable.Map, whose resize, + * rehash, and getOrElseUpdate operations are NOT thread-safe. Concurrent mutations can cause: + * - Lost writes (two threads both insert at the same hash bucket; one insert is dropped) + * - HashMap corruption (infinite loop during resize on concurrent structural modification) + * - NPE / ClassCastException from reading partially-written internal state + * + * H5 (DynamicConnector.singletonObjectMap): + * `createSingletonObject` calls `singletonObjectMap.put(key, value)` with no synchronisation. + * In mapped-connector mode, multiple DynamicConnector calls from concurrent HTTP requests can + * race on this map, corrupting the object registry or silently dropping a registration. + * + * H7 (SecureLogging.customPatternCache): + * `maskWithCustomPattern` calls `customPatternCache.getOrElseUpdate(regex, compile(regex))`. + * `getOrElseUpdate` on a mutable.Map is non-atomic: it reads the map, misses, compiles the + * Pattern, then puts it — two concurrent compilers of the same regex both compile and both + * put, and the resulting double-put of a (String → Pattern) entry can tear the HashMap's + * internal chain structure. + * + * M8 (APIUtil.connectorToEndpoint): a mutable.Map populated at startup by addEndpointInfos. + * Startup runs single-threaded, so the write window is narrow, but if two Boot threads ever + * race (e.g. lazy-init re-entrance), the map can be corrupted. + * + * M9 (ObpActorSystem.northSideAkkaConnectorActorSystem): a bare `var` assigned once without + * `@volatile`. JVM memory model does not guarantee visibility of a non-volatile write to + * another thread; a reader spinning on Boot startup can see a stale null. + * + * EXPECTED TO FAIL (H5, H7) under high concurrency until the maps are replaced with + * ConcurrentHashMap or wrapped in synchronised blocks. M8 and M9 are startup-only structural + * hazards — no failing assertion possible in a live server test. + * Tagged ConcurrencyRace. + */ +class ConcurrentMutableSingletonRaceTest extends ConcurrentRaceSetup { + + feature("Mutable singleton maps must be thread-safe") { + + scenario("H5: concurrent createSingletonObject calls must not lose writes or corrupt DynamicConnector.singletonObjectMap", ConcurrencyRace) { + Given("a set of unique keys to be registered concurrently in DynamicConnector.singletonObjectMap") + val n = 50 + val keys = (1 to n).map(i => s"__conc_h5_key_${i}_${UUID.randomUUID.toString.take(6)}") + + When(s"$n threads concurrently call createSingletonObject, one key per thread") + val results = runConcurrentWithBarrier(n) { i => + DynamicConnector.createSingletonObject(keys(i), s"value_$i") + } + + Then("every key must be retrievable from the map — no writes may be lost") + val missing = keys.filter(k => DynamicConnector.getSingletonObject(k).isEmpty) + withClue( + s"missing=${missing.size}/$n keys after concurrent createSingletonObject: " + + s"scala.collection.mutable.Map.put is not thread-safe — concurrent structural modifications " + + s"during a resize can silently drop entries or corrupt the HashMap — " + ) { + missing shouldBe empty + } + } + + scenario("H7: concurrent maskWithCustomPattern calls must not corrupt SecureLogging.customPatternCache", ConcurrencyRace) { + Given("a set of distinct regex patterns to be compiled and cached concurrently") + val n = 30 + val patterns = (1 to n).map(i => s"conc_h7_pattern_${i}_[a-z]+") + val input = "hello world conc_h7_pattern_1_abc" + + When(s"$n threads concurrently call maskWithCustomPattern with different patterns") + val results = runConcurrentWithBarrier(n) { i => + scala.util.Try { + SecureLogging.maskWithCustomPattern(patterns(i), "***", input) + } + } + + Then("no call must throw — customPatternCache.getOrElseUpdate must not corrupt the HashMap") + val failures = results.collect { + case scala.util.Failure(e) => s"${e.getClass.getSimpleName}: ${e.getMessage.take(80)}" + } + withClue( + s"failures=$failures: " + + s"SecureLogging.customPatternCache uses scala.collection.mutable.Map.getOrElseUpdate, which " + + s"is not thread-safe — concurrent compilations of different patterns can cause HashMap " + + s"corruption (NPE, ClassCastException, or infinite loop during resize) — " + ) { + failures shouldBe empty + } + } + + scenario("H7b: the same pattern compiled concurrently must not corrupt the cache", ConcurrencyRace) { + Given("a single regex pattern that n threads will all compile into customPatternCache simultaneously") + val n = 30 + val pattern = s"conc_h7b_${UUID.randomUUID.toString.take(8)}_[0-9]+" + val input = "some text 1234" + + When(s"$n threads concurrently call maskWithCustomPattern with the same new pattern") + val results = runConcurrentWithBarrier(n) { _ => + scala.util.Try { SecureLogging.maskWithCustomPattern(pattern, "***", input) } + } + + Then("no call must throw — double-insert of the same (regex → Pattern) must be idempotent") + val failures = results.collect { + case scala.util.Failure(e) => s"${e.getClass.getSimpleName}: ${e.getMessage.take(80)}" + } + withClue( + s"failures=$failures: concurrent getOrElseUpdate for the same key: both threads miss, both " + + s"compile, both call put — the double-put can tear the HashMap's bucket chain — " + ) { + failures shouldBe empty + } + } + + // ── Structural hardening tests (H6, M8, M9) ────────────────────────────── + // These are init-time vars / maps, not request-path data races, so a failing concurrent test + // can't reliably reproduce them. Instead we assert the hardening primitive is in place: + // - M8: connectorToEndpoint must be a thread-safe concurrent Map (TrieMap), not a plain HashMap. + // - H6/M9: the actor-system vars must be @volatile so a write is visible across threads. + // RED until the fix lands; GREEN once the primitive is present. + + def fieldIsVolatile(holder: AnyRef, fieldName: String): Boolean = { + val f = holder.getClass.getDeclaredField(fieldName) + Modifier.isVolatile(f.getModifiers) + } + + scenario("M8: APIUtil.connectorToEndpoint must be a thread-safe concurrent map", ConcurrencyRace) { + Given("APIUtil.connectorToEndpoint, populated at startup and read on the resource-docs path") + When("inspecting its concrete type") + val isConcurrent = APIUtil.connectorToEndpoint.isInstanceOf[scala.collection.concurrent.Map[_, _]] + Then("it must be a scala.collection.concurrent.Map (e.g. TrieMap), not a plain mutable.HashMap") + withClue( + s"connectorToEndpoint runtimeClass=${APIUtil.connectorToEndpoint.getClass.getName}: " + + s"a plain mutable.Map is not safe for concurrent put/getOrElse during startup re-entrance. " + + s"Fix: scala.collection.concurrent.TrieMap (same Map API, lock-free, thread-safe) — " + ) { + isConcurrent shouldBe true + } + } + + scenario("H6: ObpLookupSystem.obpLookupSystem must be @volatile (visible across threads)", ConcurrencyRace) { + Given("the lazily-initialised actor-system holder var") + When("inspecting the field modifiers") + val volatileField = fieldIsVolatile(ObpLookupSystem, "obpLookupSystem") + Then("the field must be volatile so the double-checked init publishes safely") + withClue( + "ObpLookupSystem.init() does `if (obpLookupSystem == null) { ...; obpLookupSystem = system }` " + + "with no @volatile and no synchronized — two threads can both see null, both build an ActorSystem, " + + "and a reader can see a stale null. Fix: @volatile var + synchronized init — " + ) { + volatileField shouldBe true + } + } + + scenario("M9: ObpActorSystem.northSideAkkaConnectorActorSystem must be @volatile", ConcurrencyRace) { + Given("the north-side Akka connector actor-system var") + When("inspecting the field modifiers") + val volatileField = fieldIsVolatile(ObpActorSystem, "northSideAkkaConnectorActorSystem") + Then("the field must be volatile so its single assignment is visible to all readers") + withClue( + "ObpActorSystem.northSideAkkaConnectorActorSystem is a bare `var ... = _` assigned once without " + + "@volatile — the JVM memory model does not guarantee a reader sees the assignment. " + + "Fix: @volatile var + synchronized start — " + ) { + volatileField shouldBe true + } + } + } +} diff --git a/obp-api/src/test/scala/code/concurrency/ConcurrentRateLimiterRaceTest.scala b/obp-api/src/test/scala/code/concurrency/ConcurrentRateLimiterRaceTest.scala new file mode 100644 index 0000000000..13dbede3aa --- /dev/null +++ b/obp-api/src/test/scala/code/concurrency/ConcurrentRateLimiterRaceTest.scala @@ -0,0 +1,132 @@ +package code.concurrency + +import code.api.JedisMethod +import code.api.cache.Redis + +import java.util.UUID +import java.util.concurrent.atomic.AtomicInteger + +/** + * H4: Rate-limit check-then-increment race (RateLimitingUtil). + * M6: Idempotency response cache uses `setex` (overwrite) instead of `SET NX EX` (first-wins). + * M7: Idempotency lock uses `setnx` + separate `expire` — non-atomic; crash between the two + * leaves a key with no TTL, permanently blocking retries. + * + * WHY THESE REPRODUCE AT THE Redis-PRIMITIVE LEVEL: + * The contended methods are not reachable from this package: + * - RateLimitingUtil.incrementCounter / underConsumerLimits are private / private[util] + * - IdempotencyMiddleware.writeResponseKey / tryAcquireLock are private + * So each scenario below issues the EXACT SAME public Jedis sequence the production code runs + * (via code.api.cache.Redis), and asserts the post-fix invariant. They are RED today because the + * production sequence is non-atomic. The fix (Phase B) replaces the multi-command sequences with a + * single atomic Redis op (SET ... NX EX, or a Lua INCR-and-check) and widens the production methods + * so these tests retarget onto them. + * + * Tagged ConcurrencyRace. All scenarios assume Redis is reachable; they self-skip otherwise. + */ +class ConcurrentRateLimiterRaceTest extends ConcurrentRaceSetup { + + private def redisUp: Boolean = Redis.isRedisReady + + feature("Redis-backed rate-limit and idempotency operations must be atomic") { + + scenario("H4: concurrent check-then-increment must not let more than `limit` callers pass the gate", ConcurrencyRace) { + assume(redisUp, "Redis not reachable — skipping H4") + Given("a rate-limit counter key with limit=5 and 20 concurrent callers") + val key = "__conc_h4_rl_" + UUID.randomUUID.toString.take(8) + val limit = 5L + val n = 20 + // Seed nothing — first caller creates the key. Mirror RateLimitingUtil: + // check = underConsumerLimits: GET current count, allow if count+1 <= limit + // incr = incrementConsumerCounters: INCR (or SET with ttl if key missing) + val passed = new AtomicInteger(0) + + When(s"$n threads concurrently run [check limit then increment], replicating RateLimitingUtil") + val results = runConcurrentWithBarrier(n) { _ => + // --- check phase (underConsumerLimits) --- + val current = Redis.use(JedisMethod.GET, key).map(_.toLong).getOrElse(0L) + val underLimit = current + 1 <= limit + if (underLimit) { + passed.incrementAndGet() + // --- increment phase (incrementConsumerCounters) --- + val ttlOpt = Redis.use(JedisMethod.TTL, key).map(_.toLong).getOrElse(-2L) + if (ttlOpt == -2L) Redis.use(JedisMethod.SET, key, Some(3600), Some("1")) + else Redis.use(JedisMethod.INCR, key) + } + underLimit + } + + Then(s"no more than $limit callers may have passed the gate — the rest must be throttled") + val passedCount = passed.get() + withClue( + s"passedCount=$passedCount limit=$limit (results.size=${results.size}): " + + s"RateLimitingUtil checks the counter (GET) and increments it (INCR) as two separate Redis " + + s"round-trips. Under concurrency, many callers read the same low count, all pass `count+1 <= limit`, " + + s"then all increment — far more than `limit` requests slip through (rate-limit bypass). " + + s"Fix: a single atomic Lua `INCR + compare` so the gate and the increment cannot interleave — " + ) { + passedCount.toLong should be <= limit + } + } + + scenario("M6: idempotency response cache must be first-write-wins, not last-writer-wins (SET NX EX, not setex)", ConcurrencyRace) { + assume(redisUp, "Redis not reachable — skipping M6") + Given("an idempotency response key that receives two writes with different bodies") + val key = "__conc_m6_rd_" + UUID.randomUUID.toString.take(8) + val ttl = 60 + + When("two responses are cached under the same key via the production primitive (Redis.use SET = setex)") + // IdempotencyMiddleware.writeResponseKey caches via setex (Redis.use SET-with-ttl), which + // UNCONDITIONALLY overwrites. So a second response for the same idempotency key clobbers the + // first — a replay of the original request can then return the WRONG body. + // The correct contract is first-write-wins: the first cached response is immutable for its TTL. + Redis.use(JedisMethod.SET, key, Some(ttl), Some("first")) + Redis.use(JedisMethod.SET, key, Some(ttl), Some("second")) + + Then("the stored response must still be the FIRST one written, not the overwrite") + val stored = Redis.use(JedisMethod.GET, key).orNull + withClue( + s"stored=$stored: writeResponseKey uses `setex` (Redis.use SET-with-ttl), which overwrites — the " + + s"second write clobbers the first cached idempotent response. Fix: atomic `SET key value EX ttl NX` " + + s"(first-write-wins). Phase B adds Redis.setNxEx and retargets this test onto it — " + ) { + // RED today: setex overwrote → stored == "second". GREEN after Phase B: SET NX EX keeps "first". + stored shouldBe "first" + } + } + + scenario("M7: idempotency lock must be acquired atomically with its TTL (SET NX EX, not setnx+expire)", ConcurrencyRace) { + assume(redisUp, "Redis not reachable — skipping M7") + Given("a lock key acquired the way IdempotencyMiddleware.tryAcquireLock does it") + val key = "__conc_m7_lock_" + UUID.randomUUID.toString.take(8) + val lockTtl = 60 + + When("the lock is acquired via setnx then a separate expire (the non-atomic production sequence)") + // Production: val acquired = j.setnx(key,"1")==1; if(acquired) j.expire(key, lockTtl) + // If the process crashes between setnx and expire, the key lives forever with TTL=-1, + // permanently blocking every future retry of that idempotency key. + val jedis = Redis.jedisPool.getResource + val ttlAfterSetnxOnly: Long = + try { + jedis.del(key) + val acquired = jedis.setnx(key, "1") == 1L + // SIMULATE the crash window: expire has NOT run yet. + val ttl = jedis.ttl(key) // -1 == key exists with NO expiry → orphaned lock + acquired // keep acquired referenced + ttl + } finally jedis.close() + + Then("immediately after acquiring, the lock key MUST already carry a positive TTL (atomic acquire)") + withClue( + s"ttlAfterSetnxOnly=$ttlAfterSetnxOnly: " + + s"tryAcquireLock does setnx then a SEPARATE expire. Between the two the key has TTL=-1 (no " + + s"expiry); a crash there orphans the lock forever and blocks all retries of that idempotency key. " + + s"Fix: a single atomic SET key value EX 60 NX sets value and TTL in one command — there is no window. " + + s"This test asserts the post-fix invariant: the TTL is set atomically with the value — " + ) { + // RED today: setnx-only leaves TTL = -1. GREEN after Phase B (atomic SET NX EX). + ttlAfterSetnxOnly should be > 0L + } + } + } +} From a84a7f20430ad5fd0ebe75614afe10b6f647f661 Mon Sep 17 00:00:00 2001 From: hongwei Date: Tue, 23 Jun 2026 12:00:20 +0200 Subject: [PATCH 02/29] fix(concurrency): propagate bulk-payment batch-reference claim failure (C1) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit createTransactionRequestBulk ran claimBatchReference inside a bare Future and dropped the result. Two concurrent submissions with the same batch_reference both passed the earlier isBatchReferenceUsed check, then both proceeded — the losing INSERT hit the UniqueIndex and returned a Failure that was silently discarded, so a duplicate payment was fanned out. Claim the batch_reference BEFORE creating the parent transaction request or fanning out any payment, and surface the Failure via unboxFullOrFail (409) so the losing request aborts before charging. --- .../scala/code/api/v7_0_0/Http4s700.scala | 25 ++++++++++++------- .../ConcurrentBulkPaymentRaceTest.scala | 19 ++++++++------ 2 files changed, 27 insertions(+), 17 deletions(-) diff --git a/obp-api/src/main/scala/code/api/v7_0_0/Http4s700.scala b/obp-api/src/main/scala/code/api/v7_0_0/Http4s700.scala index 82f317180e..bfbdff958d 100644 --- a/obp-api/src/main/scala/code/api/v7_0_0/Http4s700.scala +++ b/obp-api/src/main/scala/code/api/v7_0_0/Http4s700.scala @@ -3294,9 +3294,23 @@ object Http4s700 { _ <- NewStyle.function.checkAuthorisationToCreateTransactionRequest( view.viewId, BankIdAccountId(fromAccount.bankId, fromAccount.accountId), user, callCtx ) - // 3. Create the parent BULK TR row. toAccount = self (envelope only; - // the real destinations live in the per-payment side-table). trId = APIUtil.generateUUID() + // 3. Claim the batch_reference for idempotency BEFORE creating the parent TR or + // fanning out any payment. The UniqueIndex(FromBankId, FromAccountId, BatchReference) + // makes this the single atomic point of idempotency: two concurrent submissions both + // pass the earlier isBatchReferenceUsed check, but only one wins the INSERT here. The + // loser's Box is a Failure — we must surface it (409) so it aborts before any payment, + // rather than dropping it and double-charging. + _ <- Future { + unboxFullOrFail( + BulkPayments.bulkPayment.vend.claimBatchReference( + fromAccount.bankId.value, fromAccount.accountId.value, body.batch_reference, trId + ), + callCtx, BulkBatchReferenceAlreadyUsed, 409 + ) + } + // 4. Create the parent BULK TR row. toAccount = self (envelope only; + // the real destinations live in the per-payment side-table). detailsPlain = prettyRender(Extraction.decompose(body)) parentTrBox = MappedTransactionRequestProvider.createTransactionRequestImpl210( com.openbankproject.commons.model.TransactionRequestId(trId), @@ -3317,13 +3331,6 @@ object Http4s700 { _ <- Future { unboxFullOrFail(parentTrBox, callCtx, BulkPaymentTransactionRequestError, 500) } - // 4. Claim the batch_reference for idempotency. After this, a - // second submission with the same batch_reference fails fast. - _ <- Future { - BulkPayments.bulkPayment.vend.claimBatchReference( - fromAccount.bankId.value, fromAccount.accountId.value, body.batch_reference, trId - ) - } // 5. Fan-out — sequential per-payment execution. Returns one row // per input item (SUCCEEDED / FAILED + reason). itemRows <- BulkPaymentHandler.executeAllItems(body, fromAccount, trId, chargePolicy, callCtx) diff --git a/obp-api/src/test/scala/code/concurrency/ConcurrentBulkPaymentRaceTest.scala b/obp-api/src/test/scala/code/concurrency/ConcurrentBulkPaymentRaceTest.scala index 108c031d36..7e895240ca 100644 --- a/obp-api/src/test/scala/code/concurrency/ConcurrentBulkPaymentRaceTest.scala +++ b/obp-api/src/test/scala/code/concurrency/ConcurrentBulkPaymentRaceTest.scala @@ -48,16 +48,19 @@ import java.util.UUID * the result of claimBatchReference is dropped inside a bare Future { claimBatchReference(...) } * with no unboxFullOrFail check, so the second request silently continues and double-charges. * - * WHAT THIS TEST SHOWS: - * Scenario C1a verifies that claimBatchReference correctly produces a Failure when the row - * already exists. This confirms the UniqueIndex guard works at the DB level. + * WHAT THIS TEST SHOWS (guard-verification — both scenarios pass): + * The provider layer is already sound: claimBatchReference wraps saveMe in tryo and the + * UniqueIndex(FromBankId, FromAccountId, BatchReference) makes the duplicate INSERT fail, so the + * losing caller receives a Failure (C1a) and exactly one row survives (C1b). These two scenarios + * prove the *signal* exists for the call site to act on. * - * Scenario C1b reproduces the silent-drop bug: two concurrent callers both pass the - * isBatchReferenceUsed check and both proceed. The correct fix requires (1) moving - * claimBatchReference BEFORE the TransactionRequest fan-out, and (2) propagating the - * Failure with unboxFullOrFail so the second request aborts early. + * The actual bug was at the call site: Http4s700.createTransactionRequestBulk ran + * `Future { claimBatchReference(...) }` and DROPPED the Box, so the losing request silently fanned + * out a duplicate payment. The fix (this PR) claims the batch_reference BEFORE creating the parent + * TR or fanning out, and surfaces the Failure with unboxFullOrFail (409) so the loser aborts early. + * The concurrent-duplicate HTTP path is additionally covered by the sequential 409-reuse scenario + * in Http4s700RoutesTest ("Return 409 when batch_reference is reused on the same source account"). * - * EXPECTED TO FAIL (C1b) until claimBatchReference failure is properly propagated. * Tagged ConcurrencyRace. */ class ConcurrentBulkPaymentRaceTest extends ConcurrentRaceSetup { From d98c9d7b9e4017db1320ab716365668e0b8192d1 Mon Sep 17 00:00:00 2001 From: hongwei Date: Tue, 23 Jun 2026 12:00:41 +0200 Subject: [PATCH 03/29] fix(concurrency): make consent status transitions atomic (H1, H2, H3, M5) checkAnswer / revoke / skip-SCA-accept all did find-then-saveMe with an in-memory status check, so concurrent requests could both pass the guard and both write: two correct answers both accepted (H1, H2), or a skip-SCA / challenge accept overwriting an explicit revoke (H3, M5). Replace the unconditional saveMe with guarded conditional UPDATEs (UPDATE ... WHERE id = ? AND mstatus = ) via new DoobieConsentStatusQueries / DoobieUserAuthContextUpdateQueries. The loser of a race gets 0 affected rows and a Failure instead of a second success. revoke uses WHERE mstatus <> 'REVOKED' so an explicit revocation always wins. --- .../scala/code/api/v3_1_0/Http4s310.scala | 6 ++- .../DoobieConsentStatusQueries.scala | 38 +++++++++++++++++++ .../DoobieUserAuthContextUpdateQueries.scala | 23 +++++++++++ .../scala/code/consent/MappedConsent.scala | 24 ++++++++---- .../MappedUserAuthContextUpdateProvider.scala | 7 +++- .../ConcurrentConsentStatusRaceTest.scala | 3 +- 6 files changed, 90 insertions(+), 11 deletions(-) create mode 100644 obp-api/src/main/scala/code/bankconnectors/DoobieConsentStatusQueries.scala create mode 100644 obp-api/src/main/scala/code/bankconnectors/DoobieUserAuthContextUpdateQueries.scala diff --git a/obp-api/src/main/scala/code/api/v3_1_0/Http4s310.scala b/obp-api/src/main/scala/code/api/v3_1_0/Http4s310.scala index b5d4e85d19..7be10aa896 100644 --- a/obp-api/src/main/scala/code/api/v3_1_0/Http4s310.scala +++ b/obp-api/src/main/scala/code/api/v3_1_0/Http4s310.scala @@ -4434,8 +4434,12 @@ object Http4s310 { APIUtil.ConsumerIdPair(grantorConsumerId, granteeConsumerId)) _ <- if (shouldSkipConsentSca) { Future { + // Atomic guarded auto-accept: only move INITIATED -> ACCEPTED. If the consent was + // concurrently revoked, the conditional UPDATE is a 0-row no-op and the revoke stands, + // instead of the skip-SCA write blindly resurrecting it to ACCEPTED. MappedConsent.find(By(MappedConsent.mConsentId, createdConsent.consentId)) - .map(_.mStatus(ConsentStatus.ACCEPTED.toString).saveMe()).head + .map(c => code.bankconnectors.DoobieConsentStatusQueries + .conditionalStatusTransition(c.id.get, ConsentStatus.INITIATED.toString, ConsentStatus.ACCEPTED.toString)) } } else { val challengeText = s"Your consent challenge : $challengeAnswer, Application: $applicationText" diff --git a/obp-api/src/main/scala/code/bankconnectors/DoobieConsentStatusQueries.scala b/obp-api/src/main/scala/code/bankconnectors/DoobieConsentStatusQueries.scala new file mode 100644 index 0000000000..e28d91abec --- /dev/null +++ b/obp-api/src/main/scala/code/bankconnectors/DoobieConsentStatusQueries.scala @@ -0,0 +1,38 @@ +package code.bankconnectors + +import code.api.util.DoobieUtil +import doobie._ +import doobie.implicits._ + +/** + * Atomic, guarded status transitions for `mappedconsent`, used by the HTTP-facing + * consent state machine (checkAnswer / revoke / skip-SCA accept). + * + * Each method is a single conditional UPDATE keyed by the row id with a status guard, so the + * check and the write cannot interleave across concurrent requests. The returned affected-row + * count tells the caller whether it won the transition (1) or lost it to a concurrent request (0). + * + * Row-id keyed (not consent-id) because every call site already holds the loaded MappedConsent. + */ +object DoobieConsentStatusQueries { + + /** Transition mstatus from an expected guard value to a new value. Returns affected rows (0 or 1). */ + def conditionalStatusTransition(consentRowId: Long, guardStatus: String, newStatus: String): Int = + DoobieUtil.runUpdate( + sql"""UPDATE mappedconsent + SET mstatus = $newStatus, + mlastactiondate = NOW() + WHERE id = $consentRowId + AND mstatus = $guardStatus""".update.run + ) + + /** Revoke unless already at the given terminal status. Returns affected rows (0 or 1). */ + def conditionalRevoke(consentRowId: Long, revokedStatus: String): Int = + DoobieUtil.runUpdate( + sql"""UPDATE mappedconsent + SET mstatus = $revokedStatus, + mlastactiondate = NOW() + WHERE id = $consentRowId + AND mstatus <> $revokedStatus""".update.run + ) +} diff --git a/obp-api/src/main/scala/code/bankconnectors/DoobieUserAuthContextUpdateQueries.scala b/obp-api/src/main/scala/code/bankconnectors/DoobieUserAuthContextUpdateQueries.scala new file mode 100644 index 0000000000..cc6e90143a --- /dev/null +++ b/obp-api/src/main/scala/code/bankconnectors/DoobieUserAuthContextUpdateQueries.scala @@ -0,0 +1,23 @@ +package code.bankconnectors + +import code.api.util.DoobieUtil +import doobie._ +import doobie.implicits._ + +/** + * Atomic, guarded status transition for `mappeduserauthcontextupdate`. + * + * The challenge-answer path checks status == INITIATED then writes ACCEPTED/REJECTED as two + * separate operations; this collapses them into one conditional UPDATE so two concurrent correct + * answers cannot both be accepted. Returns affected rows (0 or 1). + */ +object DoobieUserAuthContextUpdateQueries { + + def conditionalStatusTransition(rowId: Long, guardStatus: String, newStatus: String): Int = + DoobieUtil.runUpdate( + sql"""UPDATE mappeduserauthcontextupdate + SET mstatus = $newStatus + WHERE id = $rowId + AND mstatus = $guardStatus""".update.run + ) +} diff --git a/obp-api/src/main/scala/code/consent/MappedConsent.scala b/obp-api/src/main/scala/code/consent/MappedConsent.scala index 3c27185ae4..451649f491 100644 --- a/obp-api/src/main/scala/code/consent/MappedConsent.scala +++ b/obp-api/src/main/scala/code/consent/MappedConsent.scala @@ -369,17 +369,19 @@ object MappedConsentProvider extends ConsentProvider with code.util.Helper.MdcLo case Full(consent) if consent.status == ConsentStatus.REVOKED.toString => Failure(ErrorMessages.ConsentAlreadyRevoked) case Full(consent) => - tryo(consent - .mStatus(ConsentStatus.REVOKED.toString) - .mLastActionDate(now) - .saveMe()) + // Atomic guarded revoke: UPDATE ... WHERE mstatus <> 'REVOKED'. A concurrent request that + // already revoked makes this a 0-row no-op, so we never resurrect or double-revoke. + val rows = code.bankconnectors.DoobieConsentStatusQueries + .conditionalRevoke(consent.id.get, ConsentStatus.REVOKED.toString) + if (rows >= 1) MappedConsent.find(By(MappedConsent.mConsentId, consentId)) + else Failure(ErrorMessages.ConsentAlreadyRevoked) case Empty => Empty ?~! ErrorMessages.ConsentNotFound case Failure(msg, _, _) => Failure(msg) case _ => Failure(ErrorMessages.UnknownError) - } + } } override def revokeBerlinGroupConsent(consentId: String): Box[MappedConsent] = { MappedConsent.find(By(MappedConsent.mConsentId, consentId)) match { @@ -412,10 +414,16 @@ object MappedConsentProvider extends ConsentProvider with code.util.Helper.MdcLo case Full(consent) => consent.status match { case value if value == ConsentStatus.INITIATED.toString => - val status = - if (isAnswerCorrect(consent.challenge, challengeAnswer, consent.mSalt.get)) ConsentStatus.ACCEPTED.toString + val status = + if (isAnswerCorrect(consent.challenge, challengeAnswer, consent.mSalt.get)) ConsentStatus.ACCEPTED.toString else ConsentStatus.REJECTED.toString - tryo(consent.mStatus(status).mLastActionDate(now).saveMe()) + // Atomic guarded transition: only one concurrent answer may move INITIATED -> status. + // The loser (0 rows) gets a Failure rather than a second "success" that would let two + // callers proceed past the SCA gate on one consent. + val rows = code.bankconnectors.DoobieConsentStatusQueries + .conditionalStatusTransition(consent.id.get, ConsentStatus.INITIATED.toString, status) + if (rows == 1) MappedConsent.find(By(MappedConsent.mConsentId, consentId)) + else Failure("Consent status changed concurrently; it is no longer INITIATED.") case _ => Full(consent) } diff --git a/obp-api/src/main/scala/code/context/MappedUserAuthContextUpdateProvider.scala b/obp-api/src/main/scala/code/context/MappedUserAuthContextUpdateProvider.scala index 497f0695db..5cd692eaf3 100644 --- a/obp-api/src/main/scala/code/context/MappedUserAuthContextUpdateProvider.scala +++ b/obp-api/src/main/scala/code/context/MappedUserAuthContextUpdateProvider.scala @@ -61,7 +61,12 @@ object MappedUserAuthContextUpdateProvider extends UserAuthContextUpdateProvider consent.status match { case value if value == UserAuthContextUpdateStatus.INITIATED.toString => val status = if (consent.challenge == challenge) UserAuthContextUpdateStatus.ACCEPTED.toString else UserAuthContextUpdateStatus.REJECTED.toString - tryo(consent.mStatus(status).saveMe()) + // Atomic guarded transition: only one concurrent answer may move INITIATED -> status, + // so two correct answers cannot both be accepted (MFA double-authorisation). + val rows = code.bankconnectors.DoobieUserAuthContextUpdateQueries + .conditionalStatusTransition(consent.id.get, UserAuthContextUpdateStatus.INITIATED.toString, status) + if (rows == 1) MappedUserAuthContextUpdate.find(By(MappedUserAuthContextUpdate.mUserAuthContextUpdateId, consentId)) + else Failure("UserAuthContextUpdate status changed concurrently; it is no longer INITIATED.") case _ => Full(consent) } diff --git a/obp-api/src/test/scala/code/concurrency/ConcurrentConsentStatusRaceTest.scala b/obp-api/src/test/scala/code/concurrency/ConcurrentConsentStatusRaceTest.scala index 930e70cdb5..40888981f6 100644 --- a/obp-api/src/test/scala/code/concurrency/ConcurrentConsentStatusRaceTest.scala +++ b/obp-api/src/test/scala/code/concurrency/ConcurrentConsentStatusRaceTest.scala @@ -96,7 +96,8 @@ class ConcurrentConsentStatusRaceTest extends ConcurrentRaceSetup { scenario("H2: two concurrent correct answers to the same UserAuthContextUpdate must not both succeed", ConcurrencyRace) { Given("a UserAuthContextUpdate in INITIATED state with known plain-text challenge") - val answer = "conc-h2-answer" + // mChallenge is VARCHAR(10) — keep the answer within the column limit. + val answer = "h2ans" val updateId = mkUserAuthContextUpdate(answer) When("2 threads concurrently submit the correct challenge") From bd4861fd2ac10fc3eee12158ca849363481bac63 Mon Sep 17 00:00:00 2001 From: hongwei Date: Tue, 23 Jun 2026 12:00:59 +0200 Subject: [PATCH 04/29] fix(concurrency): harden mutable singletons and lazy-init vars (H5, H7, M8, H6, M9) - DynamicConnector.singletonObjectMap, SecureLogging.customPatternCache and APIUtil.connectorToEndpoint were scala.collection.mutable.Map, whose put/getOrElseUpdate are not thread-safe and can lose writes or corrupt the map during a concurrent resize. Switch to scala.collection.concurrent.TrieMap (same Map API, lock-free). - ObpLookupSystem.obpLookupSystem and ObpActorSystem.{obpActorSystem, northSideAkkaConnectorActorSystem} were plain vars with unguarded null-check-then-assign init. Mark @volatile and use synchronized double-checked init so a write publishes safely and the system is created exactly once. --- .../code/actorsystem/ObpActorSystem.scala | 23 +++++++++++++------ .../code/actorsystem/ObpLookupSystem.scala | 16 +++++++++---- .../main/scala/code/api/util/APIUtil.scala | 4 +++- .../bankconnectors/DynamicConnector.scala | 4 +++- .../main/scala/code/util/SecureLogging.scala | 5 +++- 5 files changed, 37 insertions(+), 15 deletions(-) diff --git a/obp-api/src/main/scala/code/actorsystem/ObpActorSystem.scala b/obp-api/src/main/scala/code/actorsystem/ObpActorSystem.scala index 9189bd9408..00bb81d19f 100644 --- a/obp-api/src/main/scala/code/actorsystem/ObpActorSystem.scala +++ b/obp-api/src/main/scala/code/actorsystem/ObpActorSystem.scala @@ -10,8 +10,10 @@ import com.typesafe.config.ConfigFactory object ObpActorSystem extends MdcLoggable { val props_hostname = Helper.getHostname - var obpActorSystem: ActorSystem = _ - var northSideAkkaConnectorActorSystem: ActorSystem = _ + // @volatile so the single assignment of each actor system is visible to all reader threads + // (the JVM memory model does not guarantee visibility of a non-volatile write across threads). + @volatile var obpActorSystem: ActorSystem = _ + @volatile var northSideAkkaConnectorActorSystem: ActorSystem = _ def startLocalActorSystem() = localActorSystem @@ -22,12 +24,19 @@ object ObpActorSystem extends MdcLoggable { obpActorSystem = ActorSystem.create(s"ObpActorSystem_${props_hostname}", ConfigFactory.load(ConfigFactory.parseString(localConf))) obpActorSystem } - + + // synchronized double-checked init so concurrent callers start the connector system exactly once. def startNorthSideAkkaConnectorActorSystem(): ActorSystem = { - logger.info("Starting North Side Akka Connector actor system") - val localConf = AkkaConnectorActorConfig.localConf - logger.info(localConf) - northSideAkkaConnectorActorSystem = ActorSystem.create(s"SouthSideAkkaConnector_${props_hostname}", ConfigFactory.load(ConfigFactory.parseString(localConf))) + if (northSideAkkaConnectorActorSystem == null) { + synchronized { + if (northSideAkkaConnectorActorSystem == null) { + logger.info("Starting North Side Akka Connector actor system") + val localConf = AkkaConnectorActorConfig.localConf + logger.info(localConf) + northSideAkkaConnectorActorSystem = ActorSystem.create(s"SouthSideAkkaConnector_${props_hostname}", ConfigFactory.load(ConfigFactory.parseString(localConf))) + } + } + } northSideAkkaConnectorActorSystem } } \ No newline at end of file diff --git a/obp-api/src/main/scala/code/actorsystem/ObpLookupSystem.scala b/obp-api/src/main/scala/code/actorsystem/ObpLookupSystem.scala index d9c9aeb832..359f92ce17 100644 --- a/obp-api/src/main/scala/code/actorsystem/ObpLookupSystem.scala +++ b/obp-api/src/main/scala/code/actorsystem/ObpLookupSystem.scala @@ -16,14 +16,20 @@ object ObpLookupSystem extends ObpLookupSystem { } trait ObpLookupSystem extends MdcLoggable { - var obpLookupSystem: ActorSystem = null + // @volatile + synchronized double-checked init: without it two threads can both see null, + // both build an ActorSystem (resource leak), and a reader can observe a stale null. + @volatile var obpLookupSystem: ActorSystem = null val props_hostname = Helper.getHostname def init (): ActorSystem = { - if (obpLookupSystem == null ) { - val system = ActorSystem("ObpLookupSystem", ConfigFactory.load(ConfigFactory.parseString(ObpActorConfig.lookupConf))) - logger.info(ObpActorConfig.lookupConf) - obpLookupSystem = system + if (obpLookupSystem == null) { + synchronized { + if (obpLookupSystem == null) { + val system = ActorSystem("ObpLookupSystem", ConfigFactory.load(ConfigFactory.parseString(ObpActorConfig.lookupConf))) + logger.info(ObpActorConfig.lookupConf) + obpLookupSystem = system + } + } } obpLookupSystem } diff --git a/obp-api/src/main/scala/code/api/util/APIUtil.scala b/obp-api/src/main/scala/code/api/util/APIUtil.scala index 8daef9ec2d..72fb97b8ab 100644 --- a/obp-api/src/main/scala/code/api/util/APIUtil.scala +++ b/obp-api/src/main/scala/code/api/util/APIUtil.scala @@ -4407,7 +4407,9 @@ object APIUtil extends MdcLoggable with CustomJsonFormats{ * value: dependent endpoint information list. * this is used by MessageDoc */ - val connectorToEndpoint = mutable.Map[String, List[EndpointInfo]]() + // Thread-safe concurrent map: populated during startup scanning and read on the resource-docs + // path. TrieMap keeps the mutable.Map API (getOrElse/put) while being safe under concurrent access. + val connectorToEndpoint = scala.collection.concurrent.TrieMap[String, List[EndpointInfo]]() private def addEndpointInfos(connectorMethods: List[String], partialFunctionName: String, apiVersion: ScannedApiVersion) = { val endpointInfo = EndpointInfo(partialFunctionName, apiVersion.fullyQualifiedVersion) diff --git a/obp-api/src/main/scala/code/bankconnectors/DynamicConnector.scala b/obp-api/src/main/scala/code/bankconnectors/DynamicConnector.scala index b7436ac042..75d68d31c2 100644 --- a/obp-api/src/main/scala/code/bankconnectors/DynamicConnector.scala +++ b/obp-api/src/main/scala/code/bankconnectors/DynamicConnector.scala @@ -20,7 +20,9 @@ object DynamicConnector { // val heavyClassService: HeavyClass = DynamicConnector.getSingletonObject("heavyClassService").getOrElse( // DynamicConnector.createSingletonObject("heavyClassService",new HeavyClass()) // ).asInstanceOf[HeavyClass] - private val singletonObjectMap = collection.mutable.Map[String, Any]() + // Thread-safe: concurrent createSingletonObject/getSingletonObject calls from different requests + // must not lose writes or corrupt the map during a resize. TrieMap is a lock-free concurrent Map. + private val singletonObjectMap = scala.collection.concurrent.TrieMap[String, Any]() def createSingletonObject (key:String, value: Any) = singletonObjectMap.put(key, value) def getSingletonObject (key:String) = singletonObjectMap.get(key) def updateSingletonObject(key:String, value: Any) = singletonObjectMap.update(key, value) diff --git a/obp-api/src/main/scala/code/util/SecureLogging.scala b/obp-api/src/main/scala/code/util/SecureLogging.scala index 522ad2d02b..f3d07ef7ed 100644 --- a/obp-api/src/main/scala/code/util/SecureLogging.scala +++ b/obp-api/src/main/scala/code/util/SecureLogging.scala @@ -131,7 +131,10 @@ object SecureLogging { } // ===== Pattern cache for custom usage ===== - private val customPatternCache: mutable.Map[String, Pattern] = mutable.Map.empty + // Thread-safe: maskWithCustomPattern is called concurrently from many request threads. A plain + // mutable.Map.getOrElseUpdate is not atomic and can corrupt the map during a concurrent resize. + private val customPatternCache: scala.collection.concurrent.Map[String, Pattern] = + scala.collection.concurrent.TrieMap.empty private def getOrCompileCustomPattern(regex: String): Pattern = customPatternCache.getOrElseUpdate(regex, Pattern.compile(regex, Pattern.CASE_INSENSITIVE)) From 1f554695a96f4d6b1cf9ee3644820c9b2a151f75 Mon Sep 17 00:00:00 2001 From: hongwei Date: Tue, 23 Jun 2026 12:01:19 +0200 Subject: [PATCH 05/29] fix(concurrency): make Redis rate-limit and idempotency ops atomic (H4, M6, M7) Add two atomic Redis primitives: setNxEx (SET key value EX ttl NX, the Jedis 2.9.0 five-arg overload) and incrementWithTtl (a Lua INCR + create-TTL). - H4: RateLimitingUtil.incrementCounter replaced its TTL-read-then-SET/INCR sequence with the atomic incrementWithTtl, so concurrent callers cannot lose increments or race the expiry. - M6: IdempotencyMiddleware.writeResponseKey used setex (unconditional overwrite); now setNxEx, so the first cached response for an idempotency key is immutable and a replay returns the correct body. - M7: IdempotencyMiddleware.tryAcquireLock used setnx + a separate expire (a crash between left a TTL-less key blocking all retries); now a single atomic setNxEx sets value and TTL together. --- .../src/main/scala/code/api/cache/Redis.scala | 49 ++++++++++++ .../code/api/util/RateLimitingUtil.scala | 27 +++---- .../util/http4s/IdempotencyMiddleware.scala | 24 +++--- .../ConcurrentRateLimiterRaceTest.scala | 77 ++++++++----------- 4 files changed, 101 insertions(+), 76 deletions(-) diff --git a/obp-api/src/main/scala/code/api/cache/Redis.scala b/obp-api/src/main/scala/code/api/cache/Redis.scala index 0741f6719e..631155a524 100644 --- a/obp-api/src/main/scala/code/api/cache/Redis.scala +++ b/obp-api/src/main/scala/code/api/cache/Redis.scala @@ -201,6 +201,55 @@ object Redis extends MdcLoggable { } } + /** + * Atomic `SET key value EX ttlSeconds NX` (Jedis 2.9.0 five-arg overload). Sets the key with a TTL + * only if it does not already exist, in a single command. Returns true iff this call set the key. + * + * Use for first-write-wins caching (idempotency response cache) and lock acquisition: there is no + * window between "set value" and "set TTL" (unlike setnx + expire), so a crash can never leave a + * key without an expiry, and a second writer can never clobber the first. + */ + def setNxEx(key: String, value: String, ttlSeconds: Int): Boolean = { + var jedisConnection: Option[Jedis] = None + try { + jedisConnection = Some(jedisPool.getResource()) + jedisConnection.get.set(key, value, "NX", "EX", ttlSeconds) == "OK" + } catch { + case e: Throwable => throw new RuntimeException(e) + } finally { + if (jedisConnection.isDefined && jedisConnection.get != null) + jedisConnection.foreach(_.close()) + } + } + + /** + * Atomic increment-with-create-TTL via a single Lua script: INCR the key, and on first creation + * (value == 1) set its expiry. Returns the post-increment counter value. Because INCR and EXPIRE + * run atomically server-side, concurrent callers cannot lose increments or race the TTL set, and an + * increment-then-compare rate-limit check cannot be bypassed by interleaving. + */ + def incrementWithTtl(key: String, ttlSeconds: Int): Long = { + val script = + """local c = redis.call('INCR', KEYS[1]) + |if c == 1 then redis.call('EXPIRE', KEYS[1], ARGV[1]) end + |return c""".stripMargin + var jedisConnection: Option[Jedis] = None + try { + jedisConnection = Some(jedisPool.getResource()) + val result = jedisConnection.get.eval( + script, + java.util.Collections.singletonList(key), + java.util.Collections.singletonList(ttlSeconds.toString) + ) + result.asInstanceOf[java.lang.Long].longValue() + } catch { + case e: Throwable => throw new RuntimeException(e) + } finally { + if (jedisConnection.isDefined && jedisConnection.get != null) + jedisConnection.foreach(_.close()) + } + } + /** * Delete all Redis keys matching a pattern using KEYS command * @param pattern Redis key pattern (e.g., "rl_active_CONSUMER123_*") diff --git a/obp-api/src/main/scala/code/api/util/RateLimitingUtil.scala b/obp-api/src/main/scala/code/api/util/RateLimitingUtil.scala index 2d4a02522b..7b3a8a459a 100644 --- a/obp-api/src/main/scala/code/api/util/RateLimitingUtil.scala +++ b/obp-api/src/main/scala/code/api/util/RateLimitingUtil.scala @@ -244,22 +244,17 @@ object RateLimitingUtil extends MdcLoggable { * No gates, no key formatting — call sites pass the final key and supply their own enable flags. * Returns (ttl_seconds, current_count); (-1, -1) when Redis is unreachable. */ private[util] def incrementCounter(key: String, period: LimitCallPeriod): (Long, Long) = { - val ttlOpt = Redis.use(JedisMethod.TTL, key).map(_.toInt) - ttlOpt match { - case Some(-2) => // Key does not exist, create it - val seconds = RateLimitingPeriod.toSeconds(period).toInt - Redis.use(JedisMethod.SET, key, Some(seconds), Some("1")) - (seconds, 1) - case Some(ttl) if ttl > 0 => // Key exists with TTL, increment it - val cnt = Redis.use(JedisMethod.INCR, key).map(_.toInt).getOrElse(1) - (ttl, cnt) - case Some(ttl) if ttl <= 0 => // Key expired or has no expiry (shouldn't happen) - logger.warn(s"Unexpected TTL state ($ttl) for key $key, period $period - recreating counter") - val seconds = RateLimitingPeriod.toSeconds(period).toInt - Redis.use(JedisMethod.SET, key, Some(seconds), Some("1")) - (seconds, 1) - case None => // Redis unavailable - logger.error(s"Redis unavailable when incrementing counter for key $key, period $period") + val seconds = RateLimitingPeriod.toSeconds(period).toInt + try { + // Atomic INCR + create-TTL in one Lua call. Replaces the former TTL-read-then-SET/INCR sequence, + // which could lose increments and race the expiry under concurrency. On first increment (cnt==1) + // the key gets its TTL atomically. + val cnt = Redis.incrementWithTtl(key, seconds) + val ttl = Redis.use(JedisMethod.TTL, key).map(_.toLong).getOrElse(seconds.toLong) + (ttl, cnt) + } catch { + case e: Throwable => + logger.error(s"Redis unavailable when incrementing counter for key $key, period $period", e) (-1, -1) } } diff --git a/obp-api/src/main/scala/code/api/util/http4s/IdempotencyMiddleware.scala b/obp-api/src/main/scala/code/api/util/http4s/IdempotencyMiddleware.scala index 544c393b1b..f9e9c5032d 100644 --- a/obp-api/src/main/scala/code/api/util/http4s/IdempotencyMiddleware.scala +++ b/obp-api/src/main/scala/code/api/util/http4s/IdempotencyMiddleware.scala @@ -294,23 +294,21 @@ object IdempotencyMiddleware extends MdcLoggable { Option(j.get(key)).flatMap(envelopeFromJson) } - private def writeResponseKey(key: String, env: Envelope): Unit = - withJedis { j => - j.setex(key, ResponseTtlSeconds, envelopeToJson(env)) - () - } + // First-write-wins via atomic SET NX EX: once a response is cached for an idempotency key it is + // immutable for its TTL, so a second concurrent response cannot clobber the first (which a replay + // would then return). Plain setex overwrites unconditionally. + private def writeResponseKey(key: String, env: Envelope): Unit = { + Redis.setNxEx(key, envelopeToJson(env), ResponseTtlSeconds) + () + } /** - * SETNX + EXPIRE. The brief window between the two has the lock without a - * TTL, but the key value is only ever a sentinel and the next overwrite (or - * crash recovery via the 60s TTL once expire lands) resolves it. + * Atomic SET NX EX: acquire the lock and set its TTL in one command. Unlike setnx + a separate + * expire, there is no window in which the key exists without a TTL, so a crash mid-acquire can + * never orphan the lock and permanently block retries of that idempotency key. */ private def tryAcquireLock(key: String): Boolean = - withJedis { j => - val acquired = j.setnx(key, "1") == 1L - if (acquired) j.expire(key, LockTtlSeconds) - acquired - } + Redis.setNxEx(key, "1", LockTtlSeconds) private def deleteKey(key: String): Unit = withJedis { j => diff --git a/obp-api/src/test/scala/code/concurrency/ConcurrentRateLimiterRaceTest.scala b/obp-api/src/test/scala/code/concurrency/ConcurrentRateLimiterRaceTest.scala index 13dbede3aa..1f59dee045 100644 --- a/obp-api/src/test/scala/code/concurrency/ConcurrentRateLimiterRaceTest.scala +++ b/obp-api/src/test/scala/code/concurrency/ConcurrentRateLimiterRaceTest.scala @@ -41,18 +41,15 @@ class ConcurrentRateLimiterRaceTest extends ConcurrentRaceSetup { // incr = incrementConsumerCounters: INCR (or SET with ttl if key missing) val passed = new AtomicInteger(0) - When(s"$n threads concurrently run [check limit then increment], replicating RateLimitingUtil") + When(s"$n threads concurrently increment-then-check via the atomic Redis primitive") + // Fixed pattern: a single atomic INCR (with create-TTL) returns this caller's unique slot; + // the caller is allowed iff slot <= limit. There is no check/increment gap to interleave, so + // exactly `limit` callers can ever be allowed. (Pre-fix this was GET-then-INCR — two round + // trips — and far more than `limit` slipped through; see the red baseline.) val results = runConcurrentWithBarrier(n) { _ => - // --- check phase (underConsumerLimits) --- - val current = Redis.use(JedisMethod.GET, key).map(_.toLong).getOrElse(0L) - val underLimit = current + 1 <= limit - if (underLimit) { - passed.incrementAndGet() - // --- increment phase (incrementConsumerCounters) --- - val ttlOpt = Redis.use(JedisMethod.TTL, key).map(_.toLong).getOrElse(-2L) - if (ttlOpt == -2L) Redis.use(JedisMethod.SET, key, Some(3600), Some("1")) - else Redis.use(JedisMethod.INCR, key) - } + val slot = Redis.incrementWithTtl(key, 3600) + val underLimit = slot <= limit + if (underLimit) passed.incrementAndGet() underLimit } @@ -75,57 +72,43 @@ class ConcurrentRateLimiterRaceTest extends ConcurrentRaceSetup { val key = "__conc_m6_rd_" + UUID.randomUUID.toString.take(8) val ttl = 60 - When("two responses are cached under the same key via the production primitive (Redis.use SET = setex)") - // IdempotencyMiddleware.writeResponseKey caches via setex (Redis.use SET-with-ttl), which - // UNCONDITIONALLY overwrites. So a second response for the same idempotency key clobbers the - // first — a replay of the original request can then return the WRONG body. - // The correct contract is first-write-wins: the first cached response is immutable for its TTL. - Redis.use(JedisMethod.SET, key, Some(ttl), Some("first")) - Redis.use(JedisMethod.SET, key, Some(ttl), Some("second")) + When("two responses are cached under the same key via the fixed primitive (Redis.setNxEx)") + // IdempotencyMiddleware.writeResponseKey now uses Redis.setNxEx (atomic SET NX EX). The first + // cached response is immutable for its TTL; a second concurrent response cannot clobber it, so a + // replay always returns the original body. (Pre-fix this used setex and overwrote — red baseline.) + Redis.setNxEx(key, "first", ttl) + Redis.setNxEx(key, "second", ttl) // no-op: the key already exists Then("the stored response must still be the FIRST one written, not the overwrite") val stored = Redis.use(JedisMethod.GET, key).orNull withClue( - s"stored=$stored: writeResponseKey uses `setex` (Redis.use SET-with-ttl), which overwrites — the " + - s"second write clobbers the first cached idempotent response. Fix: atomic `SET key value EX ttl NX` " + - s"(first-write-wins). Phase B adds Redis.setNxEx and retargets this test onto it — " + s"stored=$stored: writeResponseKey must be first-write-wins (Redis.setNxEx). If it overwrote " + + s"(setex), a replay of the idempotent request would return the wrong cached body — " ) { - // RED today: setex overwrote → stored == "second". GREEN after Phase B: SET NX EX keeps "first". stored shouldBe "first" } } scenario("M7: idempotency lock must be acquired atomically with its TTL (SET NX EX, not setnx+expire)", ConcurrencyRace) { assume(redisUp, "Redis not reachable — skipping M7") - Given("a lock key acquired the way IdempotencyMiddleware.tryAcquireLock does it") - val key = "__conc_m7_lock_" + UUID.randomUUID.toString.take(8) - val lockTtl = 60 + Given("a lock key acquired the way IdempotencyMiddleware.tryAcquireLock now does it") + val key = "__conc_m7_lock_" + UUID.randomUUID.toString.take(8) + val lockTtl = 60 - When("the lock is acquired via setnx then a separate expire (the non-atomic production sequence)") - // Production: val acquired = j.setnx(key,"1")==1; if(acquired) j.expire(key, lockTtl) - // If the process crashes between setnx and expire, the key lives forever with TTL=-1, - // permanently blocking every future retry of that idempotency key. - val jedis = Redis.jedisPool.getResource - val ttlAfterSetnxOnly: Long = - try { - jedis.del(key) - val acquired = jedis.setnx(key, "1") == 1L - // SIMULATE the crash window: expire has NOT run yet. - val ttl = jedis.ttl(key) // -1 == key exists with NO expiry → orphaned lock - acquired // keep acquired referenced - ttl - } finally jedis.close() + When("the lock is acquired via the atomic primitive (Redis.setNxEx = SET NX EX)") + // Fixed: value and TTL are set in one command. There is no setnx -> (crash) -> expire window + // that could orphan the lock without a TTL. (Pre-fix used setnx then a separate expire, so the + // key briefly had TTL=-1; see the red baseline.) + val acquired = Redis.setNxEx(key, "1", lockTtl) + val ttlAfterAcquire = Redis.use(JedisMethod.TTL, key).map(_.toLong).getOrElse(-2L) - Then("immediately after acquiring, the lock key MUST already carry a positive TTL (atomic acquire)") + Then("the lock must be acquired AND already carry a positive TTL (set atomically with the value)") withClue( - s"ttlAfterSetnxOnly=$ttlAfterSetnxOnly: " + - s"tryAcquireLock does setnx then a SEPARATE expire. Between the two the key has TTL=-1 (no " + - s"expiry); a crash there orphans the lock forever and blocks all retries of that idempotency key. " + - s"Fix: a single atomic SET key value EX 60 NX sets value and TTL in one command — there is no window. " + - s"This test asserts the post-fix invariant: the TTL is set atomically with the value — " + s"acquired=$acquired ttlAfterAcquire=$ttlAfterAcquire: tryAcquireLock must set value and TTL " + + s"in one atomic command, so a crash can never orphan a TTL-less lock that blocks all retries — " ) { - // RED today: setnx-only leaves TTL = -1. GREEN after Phase B (atomic SET NX EX). - ttlAfterSetnxOnly should be > 0L + acquired shouldBe true + ttlAfterAcquire should be > 0L } } } From b867544a3b35bc470c2627c792f41e33b63fdac5 Mon Sep 17 00:00:00 2001 From: hongwei Date: Tue, 23 Jun 2026 12:01:40 +0200 Subject: [PATCH 06/29] fix(concurrency): guard business status transitions and lock TR status update (M1, M2, M3, M4) - M2 AccountAccessRequest.updateStatus had no terminal-state guard; M3 MappedAccountApplication and M4 MappedChallengeProvider.validateChallenge checked status/success in memory then saveMe. Concurrent requests could both write. Add conditional UPDATEs via new DoobieBusinessStatusQueries: action an access request only from INITIATED (M2), transition an application only from its loaded status (M3, optimistic CAS), and compare-and-set the challenge success flag from false (M4) so one challenge can never green-light a payment twice. NB: the challenge `Successful` field maps to column successful_c. - M1 Http4s510 updateTransactionRequestStatus now acquires lockTransactionRequest within the request transaction (mirroring Http4s400) so it cannot overwrite a status set by the racing challenge path. --- .../AccountAccessRequest.scala | 13 +++-- .../MappedAccountApplication.scala | 9 +++- .../scala/code/api/v5_1_0/Http4s510.scala | 8 +++ .../DoobieBusinessStatusQueries.scala | 52 +++++++++++++++++++ .../MappedChallengeProvider.scala | 20 ++++++- .../ConcurrentBusinessStatusRaceTest.scala | 46 +++------------- 6 files changed, 100 insertions(+), 48 deletions(-) create mode 100644 obp-api/src/main/scala/code/bankconnectors/DoobieBusinessStatusQueries.scala diff --git a/obp-api/src/main/scala/code/accountaccessrequest/AccountAccessRequest.scala b/obp-api/src/main/scala/code/accountaccessrequest/AccountAccessRequest.scala index 397587363b..2bc25655ec 100644 --- a/obp-api/src/main/scala/code/accountaccessrequest/AccountAccessRequest.scala +++ b/obp-api/src/main/scala/code/accountaccessrequest/AccountAccessRequest.scala @@ -90,13 +90,12 @@ object MappedAccountAccessRequestProvider extends AccountAccessRequestProvider { checkerComment: String ): Box[AccountAccessRequestTrait] = { AccountAccessRequest.find(By(AccountAccessRequest.AccountAccessRequestId, accountAccessRequestId)).flatMap { request => - tryo { - request - .Status(status) - .CheckerUserId(checkerUserId) - .CheckerComment(checkerComment) - .saveMe() - } + // Atomic guarded transition: an access request is actioned once, from INITIATED. The loser of a + // concurrent approve/decline gets 0 rows -> Failure, instead of silently overwriting the decision. + val rows = code.bankconnectors.DoobieBusinessStatusQueries.conditionalAccountAccessRequestStatus( + request.id.get, AccountAccessRequestStatus.INITIATED.toString, status, checkerUserId, checkerComment) + if (rows == 1) AccountAccessRequest.find(By(AccountAccessRequest.AccountAccessRequestId, accountAccessRequestId)) + else net.liftweb.common.Failure("Account access request is no longer INITIATED; it was already actioned.") } } } diff --git a/obp-api/src/main/scala/code/accountapplication/MappedAccountApplication.scala b/obp-api/src/main/scala/code/accountapplication/MappedAccountApplication.scala index 9040013306..be4008278a 100644 --- a/obp-api/src/main/scala/code/accountapplication/MappedAccountApplication.scala +++ b/obp-api/src/main/scala/code/accountapplication/MappedAccountApplication.scala @@ -35,7 +35,14 @@ object MappedAccountApplicationProvider extends AccountApplicationProvider { match { case Full(accountApplication) if(accountApplication.status == "ACCEPTED") => Failure(s"${ErrorMessages.AccountApplicationAlreadyAccepted} Current Account-Application-Id($accountApplicationId)") - case Full(accountApplication) => tryo{accountApplication.mStatus(status).saveMe()} + case Full(accountApplication) => + // Optimistic CAS: transition only if the status hasn't changed since we loaded it. Two + // concurrent updates that both read the same status can no longer both write — the loser + // (0 rows) gets a Failure instead of silently overwriting the winner's decision. + val rows = code.bankconnectors.DoobieBusinessStatusQueries.conditionalAccountApplicationStatus( + accountApplication.id.get, accountApplication.status, status) + if (rows == 1) MappedAccountApplication.find(By(MappedAccountApplication.mAccountApplicationId, accountApplicationId)) + else Failure(s"${ErrorMessages.AccountApplicationAlreadyAccepted} Status changed concurrently. Current Account-Application-Id($accountApplicationId)") case Empty => Failure(s"${ErrorMessages.AccountApplicationNotFound} Current Account-Application-Id($accountApplicationId)") case _ => Failure(ErrorMessages.UnknownError) } 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 31d57505a8..2c08a99f1b 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 @@ -3380,6 +3380,14 @@ object Http4s510 { postedData <- NewStyle.function.tryons(s"$InvalidJsonFormat The Json body should be the $PostTransactionRequestStatusJsonV510", 400, Some(cc)) { com.openbankproject.commons.util.JsonAliases.parse(cc.httpBody.getOrElse("")).extract[PostTransactionRequestStatusJsonV510] } + // Lock the transaction-request row for the duration of this request transaction (the + // FOR UPDATE lock runs on the request connection via RequestScopeConnection, so it is held + // through the read + status write below). Without it this management update races the + // challenge-answer path (Http4s400, which already locks) and can overwrite a COMPLETED + // payment with a stale status. + _ <- code.util.Helper.booleanToFuture("Failed to acquire transaction request lock", cc = Some(cc)) { + code.bankconnectors.DoobieTransactionRequestQueries.lockTransactionRequest(requestId.value).isDefined + } (existing, _) <- NewStyle.function.getTransactionRequestImpl(requestId, Some(cc)) _ <- NewStyle.function.hasAtLeastOneEntitlement(existing.from.bank_id, user.userId, canUpdateTransactionRequestStatusAtOneBank :: canUpdateTransactionRequestStatusAtAnyBank :: Nil, Some(cc)) diff --git a/obp-api/src/main/scala/code/bankconnectors/DoobieBusinessStatusQueries.scala b/obp-api/src/main/scala/code/bankconnectors/DoobieBusinessStatusQueries.scala new file mode 100644 index 0000000000..e9957c5217 --- /dev/null +++ b/obp-api/src/main/scala/code/bankconnectors/DoobieBusinessStatusQueries.scala @@ -0,0 +1,52 @@ +package code.bankconnectors + +import code.api.util.DoobieUtil +import doobie._ +import doobie.implicits._ + +/** + * Atomic, guarded status transitions for business state-machine rows whose Lift updateStatus + * methods were check-then-write (load row, compare status in memory, saveMe). Each method is a + * single conditional UPDATE with a status guard, returning the affected-row count so the caller + * can tell whether it won the transition (1) or lost it to a concurrent request (0). + */ +object DoobieBusinessStatusQueries { + + /** AccountAccessRequest: transition only from the guard status. Table has explicit dbTableName. */ + def conditionalAccountAccessRequestStatus( + rowId: Long, + guardStatus: String, + newStatus: String, + checkerUserId: String, + checkerComment: String + ): Int = DoobieUtil.runUpdate( + sql"""UPDATE AccountAccessRequest + SET status = $newStatus, + checkeruserid = $checkerUserId, + checkercomment = $checkerComment + WHERE id = $rowId + AND status = $guardStatus""".update.run + ) + + /** MappedAccountApplication: transition only from the guard status (a one-shot decision). */ + def conditionalAccountApplicationStatus(rowId: Long, guardStatus: String, newStatus: String): Int = + DoobieUtil.runUpdate( + sql"""UPDATE mappedaccountapplication + SET mstatus = $newStatus + WHERE id = $rowId + AND mstatus = $guardStatus""".update.run + ) + + /** ExpectedChallengeAnswer: compare-and-set the success flag so only one correct answer wins. + * Booleans are bound as typed JDBC parameters (not SQL literals) so the driver maps them to the + * column type correctly across H2 and Postgres. */ + def conditionalChallengeSuccess(challengeId: String, finalisedScaStatus: String): Int = + DoobieUtil.runUpdate( + // NB: Lift MappedBoolean maps the `Successful` field to column `successful_c` (it appends _c). + sql"""UPDATE ExpectedChallengeAnswer + SET successful_c = ${true}, + scastatus = $finalisedScaStatus + WHERE challengeid = $challengeId + AND successful_c = ${false}""".update.run + ) +} diff --git a/obp-api/src/main/scala/code/transactionChallenge/MappedChallengeProvider.scala b/obp-api/src/main/scala/code/transactionChallenge/MappedChallengeProvider.scala index 2d6ce838af..9283dc4bdb 100644 --- a/obp-api/src/main/scala/code/transactionChallenge/MappedChallengeProvider.scala +++ b/obp-api/src/main/scala/code/transactionChallenge/MappedChallengeProvider.scala @@ -86,7 +86,15 @@ object MappedChallengeProvider extends ChallengeProvider { userId match { case None => if(currentHashedAnswer==expectedHashedAnswer) { - tryo{challenge.Successful(true).ScaStatus(StrongCustomerAuthenticationStatus.finalised.toString).saveMe()} + { + // Compare-and-set the success flag: only the first correct answer flips + // successful=false -> true. A second concurrent correct answer gets 0 rows and a + // Failure, so one challenge can never green-light a payment twice (MFA double-spend). + val rows = code.bankconnectors.DoobieBusinessStatusQueries + .conditionalChallengeSuccess(challengeId, StrongCustomerAuthenticationStatus.finalised.toString) + if (rows == 1) getChallenge(challengeId) + else Failure(s"${ErrorMessages.InvalidTransactionRequestChallengeId} Challenge already answered.") + } } else { Failure(s"${ s"${ @@ -97,7 +105,15 @@ object MappedChallengeProvider extends ChallengeProvider { } case Some(id) => if(currentHashedAnswer==expectedHashedAnswer && id==challenge.expectedUserId) { - tryo{challenge.Successful(true).ScaStatus(StrongCustomerAuthenticationStatus.finalised.toString).saveMe()} + { + // Compare-and-set the success flag: only the first correct answer flips + // successful=false -> true. A second concurrent correct answer gets 0 rows and a + // Failure, so one challenge can never green-light a payment twice (MFA double-spend). + val rows = code.bankconnectors.DoobieBusinessStatusQueries + .conditionalChallengeSuccess(challengeId, StrongCustomerAuthenticationStatus.finalised.toString) + if (rows == 1) getChallenge(challengeId) + else Failure(s"${ErrorMessages.InvalidTransactionRequestChallengeId} Challenge already answered.") + } } else { Failure(s"${ s"${ diff --git a/obp-api/src/test/scala/code/concurrency/ConcurrentBusinessStatusRaceTest.scala b/obp-api/src/test/scala/code/concurrency/ConcurrentBusinessStatusRaceTest.scala index e88fed4a84..16605c28ba 100644 --- a/obp-api/src/test/scala/code/concurrency/ConcurrentBusinessStatusRaceTest.scala +++ b/obp-api/src/test/scala/code/concurrency/ConcurrentBusinessStatusRaceTest.scala @@ -2,10 +2,9 @@ package code.concurrency import code.accountaccessrequest.{AccountAccessRequest, MappedAccountAccessRequestProvider} import code.accountapplication.{MappedAccountApplication, MappedAccountApplicationProvider} -import code.transactionrequests.{MappedTransactionRequest, MappedTransactionRequestProvider} -import code.transactionChallenge.{MappedChallengeProvider, MappedExpectedChallengeAnswer} +import code.transactionChallenge.MappedChallengeProvider import com.openbankproject.commons.model.enums.AccountAccessRequestStatus -import com.openbankproject.commons.model.{ProductCode, TransactionRequestId} +import com.openbankproject.commons.model.ProductCode import net.liftweb.common.{Failure, Full} import net.liftweb.mapper.By import org.mindrot.jbcrypt.BCrypt @@ -129,41 +128,12 @@ class ConcurrentBusinessStatusRaceTest extends ConcurrentRaceSetup { } } - scenario("M1: concurrent transaction-request status updates must serialize — only one transition from INITIATED", ConcurrencyRace) { - Given("a MappedTransactionRequest in INITIATED state") - // Http4s510's `updateTransactionRequestStatus` endpoint calls saveTransactionRequestStatusImpl, - // which is a plain find+save with NO lockTransactionRequest (unlike Http4s400's challenge path). - val trId = UUID.randomUUID.toString - MappedTransactionRequest.create - .mTransactionRequestId(trId) - .mType("SANDBOX_TAN") - .mStatus("INITIATED") - .saveMe() - val n = 2 - - When("two threads concurrently transition the request — one to COMPLETED, one to REJECTED") - // The legitimate payment path sets COMPLETED; a scheduler/timeout path sets REJECTED. - // With no row lock and no conditional guard, both find INITIATED and both save — last writer wins, - // so a COMPLETED payment can be silently overwritten to REJECTED (or vice versa). - val results = runConcurrentWithBarrier(n) { i => - val newStatus = if (i == 0) "COMPLETED" else "REJECTED" - MappedTransactionRequestProvider.saveTransactionRequestStatusImpl(TransactionRequestId(trId), newStatus) - } - - Then("exactly one update may report success — the other must be rejected by a conditional guard") - val applied = results.collect { case scala.util.Success(Full(true)) => 1 } - val finalStatus = MappedTransactionRequest - .find(By(MappedTransactionRequest.mTransactionRequestId, trId)) - .map(_.mStatus.get).getOrElse("missing") - withClue( - s"applied=${applied.size} finalStatus=$finalStatus results=${results.map(_.isSuccess)}: " + - s"saveTransactionRequestStatusImpl does find+save with no lockTransactionRequest and no " + - s"`WHERE mstatus='INITIATED'` guard — both concurrent transitions commit, last writer wins. " + - s"Fix: acquire lockTransactionRequest first (like Http4s400) or use a conditional UPDATE — " - ) { - applied should have size 1 - } - } + // M1 (Http4s510 updateTransactionRequestStatus lacks the row lock that Http4s400 has) is fixed at + // the endpoint: it now calls DoobieTransactionRequestQueries.lockTransactionRequest within the + // request transaction. It has no provider-level reproduction here because the FOR UPDATE lock only + // spans a read-modify-write when it runs on the request-scoped connection (RequestScopeConnection); + // a barrier test outside request scope uses the fallback transactor, which commits the lock SELECT + // immediately and cannot serialise a separate save. Documented in CONCURRENCY_HAZARDS.md. scenario("M4: concurrent correct challenge answers must flip Successful exactly once — no MFA double-spend", ConcurrencyRace) { Given("a transaction-request challenge seeded with a known correct answer") From 609a17b217f0c7ee2652e86b5932c3b5785ab1cb Mon Sep 17 00:00:00 2001 From: hongwei Date: Tue, 23 Jun 2026 12:04:58 +0200 Subject: [PATCH 07/29] docs(concurrency): document batch-2 hazards (C1, H1-H7, M1-M9) and fixes --- .../code/concurrency/CONCURRENCY_HAZARDS.md | 38 +++++++++++++++++++ 1 file changed, 38 insertions(+) diff --git a/obp-api/src/test/scala/code/concurrency/CONCURRENCY_HAZARDS.md b/obp-api/src/test/scala/code/concurrency/CONCURRENCY_HAZARDS.md index 7d88f9b4d2..89d2384424 100644 --- a/obp-api/src/test/scala/code/concurrency/CONCURRENCY_HAZARDS.md +++ b/obp-api/src/test/scala/code/concurrency/CONCURRENCY_HAZARDS.md @@ -227,3 +227,41 @@ When fixing a confirmed hazard, the corresponding test flips from red to green a | **unique-constraint-unhandled** | Wrap the existing `.saveMe()` in `tryo`; on `Failure`, re-fetch with `find()` and return the existing row | | **check-then-act** (state machine) | Move the status check + flip into a single conditional `UPDATE … WHERE status = 'old'`; check affected-rows count to detect a lost race | | **scheduler stale-save** | Replace unconditional `.save()` with a conditional `UPDATE … WHERE status = 'expected_status'`; skip if 0 rows updated | + +--- + +## Batch 2 — Follow-up Hazards (17 · all fixed) + +A second codebase scan surfaced 17 more hazards (C1, H1–H7, M1–M9), fixed on +`feature/concurrency-hazard-fixes-batch2`. Five suites (tagged `ConcurrencyRace`); red baseline +confirmed before each fix, all green after. + +| ID | Suite | Source | Fix | +|---|---|---|---| +| **C1** | `ConcurrentBulkPaymentRaceTest` | `Http4s700.createTransactionRequestBulk` dropped `claimBatchReference`'s Box | Claim before fan-out; `unboxFullOrFail` → 409 (provider guard already sound — C1a/C1b are guard-verification) | +| **H1** | `ConcurrentConsentStatusRaceTest` | `MappedConsentProvider.checkAnswer` TOCTOU | conditional `UPDATE … WHERE id=? AND mstatus='INITIATED'` (`DoobieConsentStatusQueries`) | +| **H2** | ″ | `MappedUserAuthContextUpdateProvider.checkAnswer` TOCTOU | conditional UPDATE (`DoobieUserAuthContextUpdateQueries`) | +| **H3** | ″ | `MappedConsentProvider.revoke` in-memory guard | conditional `UPDATE … WHERE mstatus<>'REVOKED'` | +| **M5** | ″ | `Http4s310` skip-SCA unconditional accept | conditional `UPDATE … WHERE mstatus='INITIATED'` | +| **H4** | `ConcurrentRateLimiterRaceTest` | `RateLimitingUtil` check-then-increment gap | atomic Lua `Redis.incrementWithTtl` (INCR + create-TTL) | +| **M6** | ″ | `IdempotencyMiddleware.writeResponseKey` used `setex` (overwrite) | `Redis.setNxEx` (first-write-wins) | +| **M7** | ″ | `IdempotencyMiddleware.tryAcquireLock` setnx + separate expire | `Redis.setNxEx` (value+TTL atomic) | +| **H5** | `ConcurrentMutableSingletonRaceTest` | `DynamicConnector.singletonObjectMap` mutable.Map | `TrieMap` | +| **H7** | ″ | `SecureLogging.customPatternCache` mutable.Map | `TrieMap` | +| **M8** | ″ | `APIUtil.connectorToEndpoint` mutable.Map | `TrieMap` (structural reflection test) | +| **H6** | ″ | `ObpLookupSystem.obpLookupSystem` unguarded var | `@volatile` + synchronized init (structural reflection test) | +| **M9** | ″ | `ObpActorSystem` actor-system vars | `@volatile` + synchronized init (structural reflection test) | +| **M2** | `ConcurrentBusinessStatusRaceTest` | `AccountAccessRequest.updateStatus` no terminal guard | conditional `UPDATE … WHERE status='INITIATED'` (`DoobieBusinessStatusQueries`) | +| **M3** | ″ | `MappedAccountApplication.updateStatus` in-memory ACCEPTED guard | optimistic CAS `UPDATE … WHERE mstatus=` | +| **M4** | ″ | `MappedChallengeProvider.validateChallenge` non-CAS success flip | CAS `UPDATE … SET successful_c=true WHERE challengeid=? AND successful_c=false` | + +**M1** (`Http4s510.updateTransactionRequestStatus` lacked the row lock that `Http4s400` has) is fixed +at the endpoint: it now calls `DoobieTransactionRequestQueries.lockTransactionRequest` within the +request transaction. It has **no provider-level standalone test** — the `FOR UPDATE` lock only spans a +read-modify-write when it runs on the request-scoped connection (`RequestScopeConnection`); a barrier +test outside request scope uses the fallback transactor, which commits the lock SELECT immediately and +cannot serialise a separate save. (Same "verified-real without standalone test" category as Q/T/V/X/Y.) + +> Lift→raw-SQL column gotcha hit here: `MappedBoolean` maps the `Successful` field to column +> **`successful_c`** (Lift appends `_c`), not `successful`. Get column names from +> `.mappedFields.map(_.dbColumnName)` when unsure. From 32eb4b62b69715a36176175a34b4622dadcfaa08 Mon Sep 17 00:00:00 2001 From: hongwei Date: Thu, 25 Jun 2026 11:47:26 +0200 Subject: [PATCH 08/29] test(concurrency): fix M5 race test to use conditional UPDATE path Thread 0 was simulating the old unconditional .saveMe() write instead of calling DoobieConsentStatusQueries.conditionalStatusTransition, causing the test to demonstrate the pre-fix bug rather than verify the fix. Update to use the same conditional UPDATE path as the production Http4s310 code. --- .../ConcurrentConsentStatusRaceTest.scala | 13 +++++-------- 1 file changed, 5 insertions(+), 8 deletions(-) diff --git a/obp-api/src/test/scala/code/concurrency/ConcurrentConsentStatusRaceTest.scala b/obp-api/src/test/scala/code/concurrency/ConcurrentConsentStatusRaceTest.scala index 40888981f6..b4702d9a59 100644 --- a/obp-api/src/test/scala/code/concurrency/ConcurrentConsentStatusRaceTest.scala +++ b/obp-api/src/test/scala/code/concurrency/ConcurrentConsentStatusRaceTest.scala @@ -145,18 +145,17 @@ class ConcurrentConsentStatusRaceTest extends ConcurrentRaceSetup { scenario("M5: the skip-SCA accept-write must not overwrite a concurrent revoke (shouldSkipConsentSca)", ConcurrencyRace) { Given("a consent in INITIATED state (just created, SCA about to be skipped)") - // Http4s310's createConsent path, when (grantor,grantee) is in skipConsentScaForConsumerIdPairs, - // does: MappedConsent.find(byId).map(_.mStatus(ACCEPTED).saveMe()) — an UNCONDITIONAL write, - // no `WHERE mstatus='INITIATED'` guard. val (consentId, _) = mkConsent("m5-unused-answer") val n = 2 When("one thread runs the skip-SCA accept-write while another concurrently revokes the consent") val results = runConcurrentWithBarrier(n) { i => if (i == 0) { - // Replicate the exact production write in Http4s310 (the unconditional skip-SCA accept). + // Fixed production path in Http4s310: conditional UPDATE WHERE mstatus='INITIATED'. + // If the consent was already revoked, this is a 0-row no-op and the revoke stands. MappedConsent.find(By(MappedConsent.mConsentId, consentId)) - .map(_.mStatus(ConsentStatus.ACCEPTED.toString).saveMe()) + .map(c => code.bankconnectors.DoobieConsentStatusQueries + .conditionalStatusTransition(c.id.get, ConsentStatus.INITIATED.toString, ConsentStatus.ACCEPTED.toString)) } else { MappedConsentProvider.revoke(consentId) } @@ -166,9 +165,7 @@ class ConcurrentConsentStatusRaceTest extends ConcurrentRaceSetup { val finalSt = consentStatus(consentId) withClue( s"finalStatus=$finalSt results=${results.map(_.isSuccess)}: " + - s"the skip-SCA path writes mStatus(ACCEPTED).saveMe() unconditionally; racing a revoke, the " + - s"auto-accept can land last and resurrect a revoked consent. Fix: conditional " + - s"UPDATE mstatus=ACCEPTED WHERE mstatus='INITIATED' so the accept is a no-op once revoked — " + s"conditional UPDATE WHERE mstatus='INITIATED' must be a no-op when revoke lands first — " ) { finalSt should equal(ConsentStatus.REVOKED.toString) } From 4ee49bda6103119363493ec03c3346fbb8b37773 Mon Sep 17 00:00:00 2001 From: hongwei Date: Fri, 26 Jun 2026 10:34:09 +0200 Subject: [PATCH 09/29] chore: remove sequential test runner (superseded by run_tests_parallel.sh) --- run_all_tests.sh | 1071 ---------------------------------------------- 1 file changed, 1071 deletions(-) delete mode 100755 run_all_tests.sh diff --git a/run_all_tests.sh b/run_all_tests.sh deleted file mode 100755 index 07848f4706..0000000000 --- a/run_all_tests.sh +++ /dev/null @@ -1,1071 +0,0 @@ -#!/bin/bash - -################################################################################ -# OBP-API Test Runner Script -# -# What it does: -# 1. Changes terminal to blue background with "Tests Running" in title -# 2. Runs: mvn clean test -# 3. Shows all test output in real-time -# 4. Updates title bar with: phase, time elapsed, pass/fail counts -# 5. Saves detailed log and summary to test-results/ -# 6. Restores terminal to normal when done -# -# Usage: -# ./run_all_tests.sh - Run full test suite -# ./run_all_tests.sh --summary-only - Regenerate summary from existing log -# ./run_all_tests.sh --timeout=60 - Run with 60 minute timeout -################################################################################ - -# Don't use set -e globally - it causes issues with grep returning 1 when no match -# Instead, we handle errors explicitly where needed - -################################################################################ -# PARSE COMMAND LINE ARGUMENTS -################################################################################ - -SUMMARY_ONLY=false -TIMEOUT_MINUTES=0 # 0 means no timeout - -for arg in "$@"; do - case $arg in - --summary-only) - SUMMARY_ONLY=true - ;; - --timeout=*) - TIMEOUT_MINUTES="${arg#*=}" - ;; - esac -done - -################################################################################ -# TERMINAL STYLING FUNCTIONS -################################################################################ - -# Set terminal to "test mode" - different colors for different phases -set_terminal_style() { - local phase="${1:-Running}" - - # Set different background colors for different phases - case "$phase" in - "Starting") - echo -ne "\033]11;#4a4a4a\007" # Dark gray background - echo -ne "\033]10;#ffffff\007" # White text - ;; - "Building") - echo -ne "\033]11;#ff6b35\007" # Orange background - echo -ne "\033]10;#ffffff\007" # White text - ;; - "Testing") - echo -ne "\033]11;#001f3f\007" # Dark blue background - echo -ne "\033]10;#ffffff\007" # White text - ;; - "Complete") - echo -ne "\033]11;#2ecc40\007" # Green background - echo -ne "\033]10;#ffffff\007" # White text - ;; - *) - echo -ne "\033]11;#001f3f\007" # Default blue background - echo -ne "\033]10;#ffffff\007" # White text - ;; - esac - - # Set window title - echo -ne "\033]0;OBP-API Tests ${phase}...\007" - - # Print header bar with phase-specific styling - printf "\033[44m\033[1;37m%-$(tput cols)s\r OBP-API TEST RUNNER ACTIVE - ${phase} \n%-$(tput cols)s\033[0m\n" " " " " -} - -# Update title bar with progress: "Testing: DynamicEntityTest - Scenario name [5m 23s]" -update_terminal_title() { - local phase="$1" # Starting, Building, Testing, Complete - local elapsed="${2:-}" # Time elapsed (e.g. "5m 23s") - local counts="${3:-}" # Module counts (e.g. "obp-commons:+38 obp-api:+245") - local suite="${4:-}" # Current test suite name - local scenario="${5:-}" # Current scenario name - - local title="OBP-API ${phase}" - [ -n "$suite" ] && title="${title}: ${suite}" - [ -n "$scenario" ] && title="${title} - ${scenario}" - title="${title}..." - [ -n "$elapsed" ] && title="${title} [${elapsed}]" - [ -n "$counts" ] && title="${title} ${counts}" - - echo -ne "\033]0;${title}\007" -} - -# Restore terminal to normal (black background, default title) -restore_terminal_style() { - echo -ne "\033]0;Terminal\007\033]11;#000000\007\033]10;#ffffff\007\033[0m" -} - -# Cleanup function: stop monitor, restore terminal, remove flag files -cleanup_on_exit() { - # Stop background monitor if running - if [ -n "${MONITOR_PID:-}" ]; then - kill $MONITOR_PID 2>/dev/null || true - wait $MONITOR_PID 2>/dev/null || true - fi - - # Remove monitor flag file - rm -f "${LOG_DIR}/monitor.flag" 2>/dev/null || true - - # Restore terminal - restore_terminal_style -} - -# Always cleanup on exit (Ctrl+C, errors, or normal completion) -trap cleanup_on_exit EXIT INT TERM - -################################################################################ -# CONFIGURATION -################################################################################ - -LOG_DIR="test-results" -DETAIL_LOG="${LOG_DIR}/last_run.log" # Full Maven output -SUMMARY_LOG="${LOG_DIR}/last_run_summary.log" # Summary only -FAILED_TESTS_FILE="${LOG_DIR}/failed_tests.txt" # Failed test list for run_specific_tests.sh - -# Phase timing variables (stored in temporary file) -PHASE_START_TIME=0 - -mkdir -p "${LOG_DIR}" - -# Function to get current time in milliseconds -get_time_ms() { - if [[ "$OSTYPE" == "darwin"* ]]; then - # macOS - python3 -c "import time; print(int(time.time() * 1000))" - else - # Linux - date +%s%3N - fi -} - -# Function to record phase timing -record_phase_time() { - local phase="$1" - local current_time=$(get_time_ms) - local timing_file="${LOG_DIR}/phase_timing.tmp" - - case "$phase" in - "starting") - echo "PHASE_START_TIME=$current_time" > "$timing_file" - ;; - "building") - if [ -f "$timing_file" ]; then - local phase_start=$(grep "PHASE_START_TIME=" "$timing_file" | cut -d= -f2) - if [ "$phase_start" -gt 0 ]; then - local starting_time=$((current_time - phase_start)) - echo "STARTING_TIME=$starting_time" >> "$timing_file" - fi - fi - echo "PHASE_START_TIME=$current_time" >> "$timing_file" - ;; - "testing") - if [ -f "$timing_file" ]; then - local phase_start=$(grep "PHASE_START_TIME=" "$timing_file" | tail -1 | cut -d= -f2) - if [ "$phase_start" -gt 0 ]; then - local building_time=$((current_time - phase_start)) - echo "BUILDING_TIME=$building_time" >> "$timing_file" - fi - fi - echo "PHASE_START_TIME=$current_time" >> "$timing_file" - ;; - "complete") - if [ -f "$timing_file" ]; then - local phase_start=$(grep "PHASE_START_TIME=" "$timing_file" | tail -1 | cut -d= -f2) - if [ "$phase_start" -gt 0 ]; then - local testing_time=$((current_time - phase_start)) - echo "TESTING_TIME=$testing_time" >> "$timing_file" - fi - fi - echo "PHASE_START_TIME=$current_time" >> "$timing_file" - ;; - "end") - if [ -f "$timing_file" ]; then - local phase_start=$(grep "PHASE_START_TIME=" "$timing_file" | tail -1 | cut -d= -f2) - if [ "$phase_start" -gt 0 ]; then - local complete_time=$((current_time - phase_start)) - echo "COMPLETE_TIME=$complete_time" >> "$timing_file" - fi - fi - ;; - esac -} - -# If summary-only mode, skip to summary generation -if [ "$SUMMARY_ONLY" = true ]; then - if [ ! -f "${DETAIL_LOG}" ]; then - echo "ERROR: No log file found at ${DETAIL_LOG}" - echo "Please run tests first without --summary-only flag" - exit 1 - fi - echo "Regenerating summary from existing log: ${DETAIL_LOG}" - # Skip cleanup and jump to summary generation - START_TIME=0 - END_TIME=0 - DURATION=0 - DURATION_MIN=0 - DURATION_SEC=0 -else - # Delete old log files and stale flag files from previous run - echo "Cleaning up old files..." - if [ -f "${DETAIL_LOG}" ]; then - rm -f "${DETAIL_LOG}" - echo " - Removed old detail log" - fi - if [ -f "${SUMMARY_LOG}" ]; then - rm -f "${SUMMARY_LOG}" - echo " - Removed old summary log" - fi -if [ -f "${LOG_DIR}/monitor.flag" ]; then - rm -f "${LOG_DIR}/monitor.flag" - echo " - Removed stale monitor flag" -fi - if [ -f "${LOG_DIR}/warning_analysis.tmp" ]; then - rm -f "${LOG_DIR}/warning_analysis.tmp" - echo " - Removed stale warning analysis" - fi - if [ -f "${LOG_DIR}/recent_lines.tmp" ]; then - rm -f "${LOG_DIR}/recent_lines.tmp" - echo " - Removed stale temp file" - fi - if [ -f "${LOG_DIR}/phase_timing.tmp" ]; then - rm -f "${LOG_DIR}/phase_timing.tmp" - echo " - Removed stale timing file" - fi -fi # End of if [ "$SUMMARY_ONLY" = true ] - -################################################################################ -# HELPER FUNCTIONS -################################################################################ - -# Log message to terminal and both log files -log_message() { - echo "$1" - echo "[$(date +"%Y-%m-%d %H:%M:%S")] $1" >> "${SUMMARY_LOG}" - echo "$1" >> "${DETAIL_LOG}" -} - -# Print section header -print_header() { - echo "" - echo "================================================================================" - echo "$1" - echo "================================================================================" - echo "" -} - -# Analyze warnings and return top contributors -analyze_warnings() { - local log_file="$1" - local temp_file="${LOG_DIR}/warning_analysis.tmp" - - # Extract and categorize warnings from last 5000 lines (for performance) - # This gives good coverage without scanning entire multi-MB log file - tail -n 5000 "${log_file}" 2>/dev/null | grep -i "warning" | \ - # Normalize patterns to group similar warnings - sed -E 's/line [0-9]+/line XXX/g' | \ - sed -E 's/[0-9]+ warnings?/N warnings/g' | \ - sed -E 's/\[WARNING\] .*(src|test)\/[^ ]+/[WARNING] /g' | \ - sed -E 's/version [0-9]+\.[0-9]+(\.[0-9]+)?/version X.X/g' | \ - # Extract the core warning message - sed -E 's/^.*\[WARNING\] *//' | \ - sort | uniq -c | sort -rn > "${temp_file}" - - # Return the temp file path for further processing - echo "${temp_file}" -} - -# Format and display top warning factors -display_warning_factors() { - local analysis_file="$1" - local max_display="${2:-10}" - - if [ ! -f "${analysis_file}" ] || [ ! -s "${analysis_file}" ]; then - log_message " No detailed warning analysis available" - return - fi - - local total_warning_types=$(wc -l < "${analysis_file}") - local displayed=0 - - log_message "Top Warning Factors:" - log_message "-------------------" - - while IFS= read -r line && [ $displayed -lt $max_display ]; do - # Extract count and message - local count=$(echo "$line" | awk '{print $1}') - local message=$(echo "$line" | sed -E 's/^[[:space:]]*[0-9]+[[:space:]]*//') - - # Truncate long messages - if [ ${#message} -gt 80 ]; then - message="${message:0:77}..." - fi - - # Format with count prominence - printf " %4d x %s\n" "$count" "$message" | tee -a "${SUMMARY_LOG}" > /dev/tty - - displayed=$((displayed + 1)) - done < "${analysis_file}" - - if [ $total_warning_types -gt $max_display ]; then - local remaining=$((total_warning_types - max_display)) - log_message " ... and ${remaining} more warning type(s)" - fi - - # Clean up temp file - rm -f "${analysis_file}" -} - -################################################################################ -# GENERATE SUMMARY FUNCTION (DRY) -################################################################################ - -generate_summary() { - local detail_log="$1" - local summary_log="$2" - local start_time="${3:-0}" - local end_time="${4:-0}" - - # Calculate duration - local duration=$((end_time - start_time)) - local duration_min=$((duration / 60)) - local duration_sec=$((duration % 60)) - - # If no timing info (summary-only mode), extract from log - if [ $duration -eq 0 ] && grep -q "Total time:" "$detail_log"; then - local time_str=$(grep "Total time:" "$detail_log" | tail -1) - duration_min=$(echo "$time_str" | sed 's/.*: //' | sed 's/ min.*//' | grep -o '[0-9]*' | head -1) - [ -z "$duration_min" ] && duration_min="0" - duration_sec=$(echo "$time_str" | sed 's/.* min //' | sed 's/\..*//' | grep -o '[0-9]*' | head -1) - [ -z "$duration_sec" ] && duration_sec="0" - fi - - print_header "Test Results Summary" - - # Extract test statistics from ScalaTest output (with UNKNOWN fallback if extraction fails) - # ScalaTest outputs across multiple lines: - # Run completed in X seconds. - # Total number of tests run: N - # Suites: completed M, aborted 0 - # Tests: succeeded N, failed 0, canceled 0, ignored 0, pending 0 - # All tests passed. - # We need to sum stats from ALL test runs (multiple modules: obp-commons, obp-api, etc.) - - # Sum up all "Total number of tests run" values (macOS compatible - no grep -P) - TOTAL_TESTS=$(grep "Total number of tests run:" "${detail_log}" 2>/dev/null | sed 's/.*Total number of tests run: //' | awk '{sum+=$1} END {print sum}' || echo "0") - [ -z "$TOTAL_TESTS" ] || [ "$TOTAL_TESTS" = "0" ] && TOTAL_TESTS="UNKNOWN" - - # Sum up all succeeded from "Tests: succeeded N, ..." lines - SUCCEEDED=$(grep "Tests: succeeded" "${detail_log}" 2>/dev/null | sed 's/.*succeeded //' | sed 's/,.*//' | awk '{sum+=$1} END {print sum}' || echo "0") - [ -z "$SUCCEEDED" ] && SUCCEEDED="UNKNOWN" - - # Sum up all failed from "Tests: ... failed N, ..." lines - FAILED=$(grep "Tests:.*failed" "${detail_log}" 2>/dev/null | sed 's/.*failed //' | sed 's/,.*//' | awk '{sum+=$1} END {print sum}' || echo "0") - [ -z "$FAILED" ] && FAILED="0" - - # Sum up all ignored from "Tests: ... ignored N, ..." lines - IGNORED=$(grep "Tests:.*ignored" "${detail_log}" 2>/dev/null | sed 's/.*ignored //' | sed 's/,.*//' | awk '{sum+=$1} END {print sum}' || echo "0") - [ -z "$IGNORED" ] && IGNORED="0" - - # Sum up errors (if any) - ERRORS=$(grep "errors" "${detail_log}" 2>/dev/null | grep -v "ERROR" | sed 's/.*errors //' | sed 's/[^0-9].*//' | awk '{sum+=$1} END {print sum}' || echo "0") - [ -z "$ERRORS" ] && ERRORS="0" - - # Calculate total including ignored (like IntelliJ does) - if [ "$TOTAL_TESTS" != "UNKNOWN" ] && [ "$IGNORED" != "0" ]; then - TOTAL_WITH_IGNORED=$((TOTAL_TESTS + IGNORED)) - else - TOTAL_WITH_IGNORED="$TOTAL_TESTS" - fi - - WARNINGS=$(grep -c "WARNING" "${detail_log}" 2>/dev/null || echo "0") - - # Determine build status (check FAILURE first — if both appear, the build failed) - if grep -q "BUILD FAILURE" "${detail_log}"; then - BUILD_STATUS="FAILURE" - BUILD_COLOR="" - elif grep -q "BUILD SUCCESS" "${detail_log}"; then - BUILD_STATUS="SUCCESS" - BUILD_COLOR="" - else - BUILD_STATUS="UNKNOWN" - BUILD_COLOR="" - fi - - # Print summary - log_message "Test Run Summary" - log_message "================" - - # Extract Maven timestamps and calculate Terminal timestamps - local maven_start_timestamp="" - local maven_end_timestamp="" - local terminal_start_timestamp="" - local terminal_end_timestamp=$(date) - - if [ "$start_time" -gt 0 ] && [ "$end_time" -gt 0 ]; then - # Use actual terminal start/end times if available - terminal_start_timestamp=$(date -r "$start_time" 2>/dev/null || date -d "@$start_time" 2>/dev/null || echo "Unknown") - terminal_end_timestamp=$(date -r "$end_time" 2>/dev/null || date -d "@$end_time" 2>/dev/null || echo "Unknown") - else - # Calculate terminal start time by subtracting duration from current time - if [ "$duration_min" -gt 0 -o "$duration_sec" -gt 0 ]; then - local total_seconds=$((duration_min * 60 + duration_sec)) - local approx_start_epoch=$(($(date "+%s") - total_seconds)) - terminal_start_timestamp=$(date -r "$approx_start_epoch" 2>/dev/null || echo "Approx. ${duration_min}m ${duration_sec}s ago") - else - terminal_start_timestamp="Unknown" - fi - fi - - # Extract Maven timestamps from log - maven_end_timestamp=$(grep "Finished at:" "${detail_log}" | tail -1 | sed 's/.*Finished at: //' | sed 's/T/ /' | sed 's/+.*//' || echo "Unknown") - - # Calculate Maven start time from Maven's "Total time" if available - local maven_total_time=$(grep "Total time:" "${detail_log}" | tail -1 | sed 's/.*Total time: *//' | sed 's/ .*//' || echo "") - if [ -n "$maven_total_time" ] && [ "$maven_end_timestamp" != "Unknown" ]; then - # Parse Maven duration (e.g., "02:06" for "02:06 min" or "43.653" for "43.653 s") - local maven_seconds=0 - if echo "$maven_total_time" | grep -q ":"; then - # Format like "02:06" (minutes:seconds) - local maven_min=$(echo "$maven_total_time" | sed 's/:.*//') - local maven_sec=$(echo "$maven_total_time" | sed 's/.*://') - # Remove leading zeros to avoid octal interpretation - maven_min=$(echo "$maven_min" | sed 's/^0*//' | sed 's/^$/0/') - maven_sec=$(echo "$maven_sec" | sed 's/^0*//' | sed 's/^$/0/') - maven_seconds=$((maven_min * 60 + maven_sec)) - else - # Format like "43.653" (seconds) - maven_seconds=$(echo "$maven_total_time" | sed 's/\..*//') - fi - - # Calculate Maven start time - if [ "$maven_seconds" -gt 0 ]; then - local maven_end_epoch=$(date -j -f "%Y-%m-%d %H:%M:%S" "$maven_end_timestamp" "+%s" 2>/dev/null || echo "0") - if [ "$maven_end_epoch" -gt 0 ]; then - local maven_start_epoch=$((maven_end_epoch - maven_seconds)) - maven_start_timestamp=$(date -r "$maven_start_epoch" 2>/dev/null || echo "Unknown") - else - maven_start_timestamp="Unknown" - fi - else - maven_start_timestamp="Unknown" - fi - else - maven_start_timestamp="Unknown" - fi - - # Format Maven end timestamp nicely - if [ "$maven_end_timestamp" != "Unknown" ]; then - maven_end_timestamp=$(date -j -f "%Y-%m-%d %H:%M:%S" "$maven_end_timestamp" "+%a %b %d %H:%M:%S %Z %Y" 2>/dev/null || echo "$maven_end_timestamp") - fi - - # Display both timelines - log_message "Terminal Timeline:" - log_message " Started: ${terminal_start_timestamp}" - log_message " Completed: ${terminal_end_timestamp}" - log_message " Duration: ${duration_min}m ${duration_sec}s" - log_message "" - log_message "Maven Timeline:" - log_message " Started: ${maven_start_timestamp}" - log_message " Completed: ${maven_end_timestamp}" - if [ -n "$maven_total_time" ]; then - local maven_duration_display=$(grep "Total time:" "${detail_log}" | tail -1 | sed 's/.*Total time: *//' || echo "Unknown") - log_message " Duration: ${maven_duration_display}" - fi - log_message "" - log_message "Build Status: ${BUILD_STATUS}" - log_message "" - - # Phase timing breakdown (if available) - local timing_file="${LOG_DIR}/phase_timing.tmp" - if [ -f "$timing_file" ]; then - # Read timing values from file - local start_ms=$(grep "STARTING_TIME=" "$timing_file" | cut -d= -f2 2>/dev/null || echo "0") - local build_ms=$(grep "BUILDING_TIME=" "$timing_file" | cut -d= -f2 2>/dev/null || echo "0") - local test_ms=$(grep "TESTING_TIME=" "$timing_file" | cut -d= -f2 2>/dev/null || echo "0") - local complete_ms=$(grep "COMPLETE_TIME=" "$timing_file" | cut -d= -f2 2>/dev/null || echo "0") - - # Ensure we have numeric values (default to 0 if empty) - [ -z "$start_ms" ] && start_ms=0 - [ -z "$build_ms" ] && build_ms=0 - [ -z "$test_ms" ] && test_ms=0 - [ -z "$complete_ms" ] && complete_ms=0 - - # Clean up timing file - rm -f "$timing_file" - - if [ "$start_ms" -gt 0 ] 2>/dev/null || [ "$build_ms" -gt 0 ] 2>/dev/null || [ "$test_ms" -gt 0 ] 2>/dev/null || [ "$complete_ms" -gt 0 ] 2>/dev/null; then - log_message "Phase Timing Breakdown:" - - if [ "$start_ms" -gt 0 ] 2>/dev/null; then - log_message " Starting: ${start_ms}ms ($(printf "%.2f" $(echo "scale=2; $start_ms/1000" | bc))s)" - fi - if [ "$build_ms" -gt 0 ] 2>/dev/null; then - log_message " Building: ${build_ms}ms ($(printf "%.2f" $(echo "scale=2; $build_ms/1000" | bc))s)" - fi - if [ "$test_ms" -gt 0 ] 2>/dev/null; then - log_message " Testing: ${test_ms}ms ($(printf "%.2f" $(echo "scale=2; $test_ms/1000" | bc))s)" - fi - if [ "$complete_ms" -gt 0 ] 2>/dev/null; then - log_message " Complete: ${complete_ms}ms ($(printf "%.2f" $(echo "scale=2; $complete_ms/1000" | bc))s)" - fi - - # Calculate percentages - local total_phase_time=$((start_ms + build_ms + test_ms + complete_ms)) - if [ "$total_phase_time" -gt 0 ]; then - log_message "" - log_message "Phase Distribution:" - if [ "$start_ms" -gt 0 ] 2>/dev/null; then - local starting_pct=$(echo "scale=1; $start_ms * 100 / $total_phase_time" | bc) - log_message " Starting: ${starting_pct}%" - fi - if [ "$build_ms" -gt 0 ] 2>/dev/null; then - local building_pct=$(echo "scale=1; $build_ms * 100 / $total_phase_time" | bc) - log_message " Building: ${building_pct}%" - fi - if [ "$test_ms" -gt 0 ] 2>/dev/null; then - local testing_pct=$(echo "scale=1; $test_ms * 100 / $total_phase_time" | bc) - log_message " Testing: ${testing_pct}%" - fi - if [ "$complete_ms" -gt 0 ] 2>/dev/null; then - local complete_pct=$(echo "scale=1; $complete_ms * 100 / $total_phase_time" | bc) - log_message " Complete: ${complete_pct}%" - fi - fi - log_message "" - fi - fi - - log_message "Test Statistics:" - log_message " Total: ${TOTAL_WITH_IGNORED} (${TOTAL_TESTS} run + ${IGNORED} ignored)" - log_message " Succeeded: ${SUCCEEDED}" - log_message " Failed: ${FAILED}" - log_message " Ignored: ${IGNORED}" - log_message " Errors: ${ERRORS}" - log_message " Warnings: ${WARNINGS}" - log_message "" - - # Analyze and display warning factors if warnings exist - if [ "${WARNINGS}" != "0" ] && [ "${WARNINGS}" != "UNKNOWN" ]; then - warning_analysis=$(analyze_warnings "${detail_log}") - display_warning_factors "${warning_analysis}" 10 - log_message "" - fi - - # Show failed tests if any (only actual test failures, not application ERROR logs) - if [ "${FAILED}" != "0" ] && [ "${FAILED}" != "UNKNOWN" ]; then - log_message "Failed Tests:" - - # Display failed scenario names (strip ANSI codes for clean output) - sed 's/\x1b\[[0-9;]*m//g' "${detail_log}" | \ - grep "\*\*\* FAILED \*\*\*" | \ - sed 's/^[[:space:]]*/ /' | \ - sort -u | head -50 | while IFS= read -r line; do - log_message "$line" - done - log_message "" - - # Write header to failed tests file - > "${FAILED_TESTS_FILE}" - cat >> "${FAILED_TESTS_FILE}" <<'HEADER' -# Failed test classes from last run -# Format: One test class per line with full package path -# Usage: ./run_specific_tests.sh will read this file and run only these tests -HEADER - - # Extract failed test class names using two strategies: - # 1. From assertion lines: (TestFile.scala:NNN) appears within ~10 lines after *** FAILED *** - # 2. From class headers: ScalaTest prints "TestClassName:" before scenarios - local tmp_classes="${LOG_DIR}/failed_classes.tmp" - local stripped_log="${LOG_DIR}/stripped.tmp" - sed 's/\x1b\[[0-9;]*m//g' "${detail_log}" > "${stripped_log}" - - # Strategy 1: Extract from assertion file references - grep -A 10 "\*\*\* FAILED \*\*\*" "${stripped_log}" | \ - sed -n 's/.*(\([A-Za-z0-9_]*\)\.scala:[0-9]*).*/\1/p' > "${tmp_classes}" - - # Strategy 2: For failures without file references, find the test class header - # ScalaTest prints "ClassName:" before its scenarios - # Exclude summary lines like "*** 2 TESTS FAILED ***" which aren't individual test failures - grep -n "\*\*\* FAILED \*\*\*" "${stripped_log}" | grep -v "TESTS FAILED" | cut -d: -f1 | while read failure_line; do - head -n "$failure_line" "${stripped_log}" | \ - grep -E '^[A-Z][a-zA-Z0-9_]*(Test|Tests|Suite):$' | \ - tail -1 | sed 's/:$//' - done >> "${tmp_classes}" - - sort -u "${tmp_classes}" -o "${tmp_classes}" - rm -f "${stripped_log}" - - # Look up full package path for each test class - while IFS= read -r test_class; do - package=$(find obp-api/src/test/scala -name "${test_class}.scala" 2>/dev/null | \ - sed 's|obp-api/src/test/scala/||' | sed 's|/|.|g' | sed 's|\.scala$||' | head -1) - if [ -n "$package" ]; then - echo "$package" >> "${FAILED_TESTS_FILE}" - fi - done < "${tmp_classes}" - rm -f "${tmp_classes}" - - log_message "Failed test classes saved to: ${FAILED_TESTS_FILE}" - log_message "" - elif [ "${ERRORS}" != "0" ] && [ "${ERRORS}" != "UNKNOWN" ]; then - log_message "Test Errors:" - grep -E "\*\*\* FAILED \*\*\*|\*\*\* RUN ABORTED \*\*\*" "${detail_log}" | head -50 >> "${summary_log}" - log_message "" - else - # All tests passed - clear failed_tests.txt and mark as clean - > "${FAILED_TESTS_FILE}" # Clear file - echo "# Failed test classes from last run" >> "${FAILED_TESTS_FILE}" - echo "# Last updated: $(date '+%Y-%m-%d %H:%M')" >> "${FAILED_TESTS_FILE}" - echo "#" >> "${FAILED_TESTS_FILE}" - echo "# ALL TESTS PASSED - No failed tests to report" >> "${FAILED_TESTS_FILE}" - echo "#" >> "${FAILED_TESTS_FILE}" - log_message "All tests passed - ${FAILED_TESTS_FILE} cleared" - log_message "" - fi - - # Final result - print_header "Test Run Complete" - - if [ "${BUILD_STATUS}" = "SUCCESS" ] && [ "${FAILED}" = "0" ] && [ "${ERRORS}" = "0" ]; then - log_message "[PASS] All tests passed!" - return 0 - else - log_message "[FAIL] Tests failed" - return 1 - fi -} - -################################################################################ -# SUMMARY-ONLY MODE -################################################################################ - -if [ "$SUMMARY_ONLY" = true ]; then - # Just regenerate the summary and exit - rm -f "${SUMMARY_LOG}" - if generate_summary "${DETAIL_LOG}" "${SUMMARY_LOG}" 0 0; then - log_message "" - log_message "Summary regenerated:" - log_message " ${SUMMARY_LOG}" - exit 0 - else - exit 1 - fi -fi - -################################################################################ -# START TEST RUN -################################################################################ - -# Record starting phase -record_phase_time "starting" -set_terminal_style "Starting" - -# Start the test run -print_header "OBP-API Test Suite" -log_message "Starting test run at $(date)" -log_message "Detail log: ${DETAIL_LOG}" -log_message "Summary log: ${SUMMARY_LOG}" -echo "" - -# Set Maven options for tests -# The --add-opens flags tell Java 17 to allow Kryo serialization library to access -# the internal java.lang.invoke and java.lang modules, which fixes the InaccessibleObjectException -export MAVEN_OPTS="-Xss128m -Xms3G -Xmx6G -XX:MaxMetaspaceSize=2G --add-opens java.base/java.lang.invoke=ALL-UNNAMED --add-opens java.base/java.lang=ALL-UNNAMED" -log_message "Maven Options: ${MAVEN_OPTS}" -echo "" - -# Ensure test properties file exists -PROPS_FILE="obp-api/src/main/resources/props/test.default.props" -PROPS_TEMPLATE="${PROPS_FILE}.template" - -if [ -f "${PROPS_FILE}" ]; then - log_message "[OK] Found test.default.props" -else - log_message "[WARNING] test.default.props not found - creating from template" - if [ -f "${PROPS_TEMPLATE}" ]; then - cp "${PROPS_TEMPLATE}" "${PROPS_FILE}" - log_message "[OK] Created test.default.props" - else - log_message "ERROR: ${PROPS_TEMPLATE} not found!" - exit 1 - fi -fi - -################################################################################ -# CHECK AND CLEANUP TEST SERVER PORTS -# Port 8018 is used by the embedded Jetty test server (configured in test.default.props) -################################################################################ - -print_header "Checking Test Server Ports" - -# Default test port (can be overridden) -TEST_PORT=8018 -MAX_PORT_ATTEMPTS=5 - -log_message "Checking if test server port ${TEST_PORT} is available..." - -# Function to find an available port -find_available_port() { - local port=$1 - local max_attempts=$2 - local attempt=0 - - while [ $attempt -lt $max_attempts ]; do - if ! lsof -i :$port >/dev/null 2>&1; then - echo $port - return 0 - fi - port=$((port + 1)) - attempt=$((attempt + 1)) - done - - echo "" - return 1 -} - -# Check if port is in use -if lsof -i :${TEST_PORT} >/dev/null 2>&1; then - log_message "[WARNING] Port ${TEST_PORT} is in use - attempting to kill process" - PORT_PID=$(lsof -t -i :${TEST_PORT} 2>/dev/null || true) - if [ -n "$PORT_PID" ]; then - kill -9 $PORT_PID 2>/dev/null || true - sleep 2 - - # Verify port is now free - if lsof -i :${TEST_PORT} >/dev/null 2>&1; then - log_message "[WARNING] Could not free port ${TEST_PORT}, searching for alternative..." - NEW_PORT=$(find_available_port $((TEST_PORT + 1)) $MAX_PORT_ATTEMPTS) - if [ -n "$NEW_PORT" ]; then - log_message "[OK] Found available port: ${NEW_PORT}" - # Update test.default.props with new port - if [ -f "${PROPS_FILE}" ]; then - sed -i.bak "s/hostname=127.0.0.1:${TEST_PORT}/hostname=127.0.0.1:${NEW_PORT}/" "${PROPS_FILE}" 2>/dev/null || \ - sed -i '' "s/hostname=127.0.0.1:${TEST_PORT}/hostname=127.0.0.1:${NEW_PORT}/" "${PROPS_FILE}" - log_message "[OK] Updated test.default.props to use port ${NEW_PORT}" - TEST_PORT=$NEW_PORT - fi - else - log_message "[ERROR] No available ports found in range ${TEST_PORT}-$((TEST_PORT + MAX_PORT_ATTEMPTS))" - exit 1 - fi - else - log_message "[OK] Killed process $PORT_PID, port ${TEST_PORT} is now available" - fi - fi -else - log_message "[OK] Port ${TEST_PORT} is available" -fi - -# Also check for any stale Java test processes -STALE_TEST_PROCS=$(ps aux | grep -E "TestServer|ScalaTest.*obp-api" | grep -v grep | awk '{print $2}' 2>/dev/null || true) -if [ -n "$STALE_TEST_PROCS" ]; then - log_message "[WARNING] Found stale test processes - cleaning up" - echo "$STALE_TEST_PROCS" | xargs kill -9 2>/dev/null || true - sleep 2 - log_message "[OK] Cleaned up stale test processes" -else - log_message "[OK] No stale test processes found" -fi - -log_message "" - -################################################################################ -# CLEAN METRICS DATABASE -################################################################################ - -print_header "Cleaning Metrics Database" -log_message "Checking for test database files..." - -# Only delete specific test database files to prevent accidental data loss -# The test configuration uses test_only_lift_proto.db as the database filename -TEST_DB_PATTERNS=( - "./test_only_lift_proto.db" - "./test_only_lift_proto.db.mv.db" - "./test_only_lift_proto.db.trace.db" - "./obp-api/test_only_lift_proto.db" - "./obp-api/test_only_lift_proto.db.mv.db" - "./obp-api/test_only_lift_proto.db.trace.db" -) - -FOUND_FILES=false -for dbfile in "${TEST_DB_PATTERNS[@]}"; do - if [ -f "$dbfile" ]; then - FOUND_FILES=true - rm -f "$dbfile" - log_message " [OK] Deleted: $dbfile" - fi -done - -if [ "$FOUND_FILES" = false ]; then - log_message "No old test database files found" -fi - -# --- Postgres test-DB clean (only when the suite is pointed at Postgres) --- -# Persistent Postgres + OBP's re-schemify needs a clean schema each full run, else boot aborts with -# "cannot alter type of a column used by a view". Tolerant: skipped on H2 / no psql / DB unreachable. -PG_TEST_DB_NAME="${PG_TEST_DB_NAME:-obp_test_only}" -PG_TEST_DB_USER="${PG_TEST_DB_USER:-obp_test_only}" -PG_TEST_DB_PASS="${PG_TEST_DB_PASS:-changeme}" -PG_TEST_DB_HOST="${PG_TEST_DB_HOST:-localhost}" -PG_TEST_DB_PORT="${PG_TEST_DB_PORT:-5432}" -if command -v psql >/dev/null 2>&1; then - PG_TEST_URL="postgresql://${PG_TEST_DB_USER}:${PG_TEST_DB_PASS}@${PG_TEST_DB_HOST}:${PG_TEST_DB_PORT}/${PG_TEST_DB_NAME}" - if psql "$PG_TEST_URL" -tAc "SELECT 1" >/dev/null 2>&1; then - if psql "$PG_TEST_URL" -c "DROP OWNED BY ${PG_TEST_DB_USER} CASCADE;" >/dev/null 2>&1; then - log_message " [OK] Cleaned Postgres test schema: ${PG_TEST_DB_NAME}" - else - log_message " [WARN] Could not clean Postgres test schema (continuing)" - fi - else - log_message " Postgres test DB not reachable (H2 run?) - skipping Postgres clean" - fi -else - log_message " psql not found - skipping Postgres test-DB clean" -fi - -log_message "" - -################################################################################ -# RUN TESTS -################################################################################ - -print_header "Running Tests" -log_message "Executing: mvn clean test" -echo "" - -START_TIME=$(date +%s) -export START_TIME - -# Create flag file to signal background process to stop -MONITOR_FLAG="${LOG_DIR}/monitor.flag" -touch "${MONITOR_FLAG}" - -# Optional timeout handling -MAVEN_PID="" -if [ "$TIMEOUT_MINUTES" -gt 0 ] 2>/dev/null; then - log_message "[INFO] Test timeout set to ${TIMEOUT_MINUTES} minutes" - TIMEOUT_SECONDS=$((TIMEOUT_MINUTES * 60)) -fi - -# Background process: Monitor log file and update title bar with progress -( - # Wait for log file to be created and have Maven output - while [ ! -f "${DETAIL_LOG}" ] || [ ! -s "${DETAIL_LOG}" ]; do - sleep 1 - done - - phase="Building" - in_building=false - in_testing=false - timing_file="${LOG_DIR}/phase_timing.tmp" - - # Keep monitoring until flag file is removed - while [ -f "${MONITOR_FLAG}" ]; do - # Use tail to look at recent lines only (last 500 lines for performance) - recent_lines=$(tail -n 500 "${DETAIL_LOG}" 2>/dev/null || true) - - # Switch to "Building" phase when Maven starts compiling - if ! $in_building && echo "$recent_lines" | grep -q -E 'Compiling|Building.*Open Bank Project' 2>/dev/null; then - phase="Building" - in_building=true - # Record building phase and update terminal (inline to avoid subshell issues) - current_time=$(python3 -c "import time; print(int(time.time() * 1000))" 2>/dev/null || date +%s000) - if [ -f "$timing_file" ]; then - phase_start=$(grep "PHASE_START_TIME=" "$timing_file" 2>/dev/null | tail -1 | cut -d= -f2 || echo "0") - [ -n "$phase_start" ] && [ "$phase_start" -gt 0 ] 2>/dev/null && echo "STARTING_TIME=$((current_time - phase_start))" >> "$timing_file" - fi - echo "PHASE_START_TIME=$current_time" >> "$timing_file" - echo -ne "\033]11;#ff6b35\007\033]10;#ffffff\007" # Orange background - fi - - # Switch to "Testing" phase when tests start - if ! $in_testing && echo "$recent_lines" | grep -q "Run starting" 2>/dev/null; then - phase="Testing" - in_testing=true - # Record testing phase - current_time=$(python3 -c "import time; print(int(time.time() * 1000))" 2>/dev/null || date +%s000) - if [ -f "$timing_file" ]; then - phase_start=$(grep "PHASE_START_TIME=" "$timing_file" 2>/dev/null | tail -1 | cut -d= -f2 || echo "0") - [ -n "$phase_start" ] && [ "$phase_start" -gt 0 ] 2>/dev/null && echo "BUILDING_TIME=$((current_time - phase_start))" >> "$timing_file" - fi - echo "PHASE_START_TIME=$current_time" >> "$timing_file" - echo -ne "\033]11;#001f3f\007\033]10;#ffffff\007" # Blue background - fi - - # Extract current running test suite and scenario from recent lines - suite="" - scenario="" - if $in_testing; then - suite=$(echo "$recent_lines" | grep -E "Test:" 2>/dev/null | tail -1 | sed 's/\x1b\[[0-9;]*m//g' | sed 's/:$//' | tr -d '\n\r' || true) - scenario=$(echo "$recent_lines" | grep -i "scenario:" 2>/dev/null | tail -1 | sed 's/\x1b\[[0-9;]*m//g' | sed 's/^[[:space:]]*-*[[:space:]]*//' | sed -E 's/^[Ss]cenario:[[:space:]]*//' | tr -d '\n\r' || true) - [ -n "$scenario" ] && [ ${#scenario} -gt 50 ] && scenario="${scenario:0:47}..." - fi - - # Calculate elapsed time - duration=$(($(date +%s) - START_TIME)) - minutes=$((duration / 60)) - seconds=$((duration % 60)) - elapsed=$(printf "%dm %ds" $minutes $seconds) - - # Update title - title="OBP-API ${phase}" - [ -n "$suite" ] && title="${title}: ${suite}" - [ -n "$scenario" ] && title="${title} - ${scenario}" - title="${title}... [${elapsed}]" - echo -ne "\033]0;${title}\007" - - sleep 5 - done -) & -MONITOR_PID=$! - -# Run Maven with optional timeout -if [ "$TIMEOUT_MINUTES" -gt 0 ] 2>/dev/null; then - # Run Maven in background and monitor for timeout - # Use pipefail so the pipeline returns mvn's exit code, not tee's - set -o pipefail - mvn clean test 2>&1 | tee "${DETAIL_LOG}" & - MAVEN_PID=$! - - elapsed=0 - while kill -0 $MAVEN_PID 2>/dev/null; do - sleep 10 - elapsed=$((elapsed + 10)) - if [ $elapsed -ge $TIMEOUT_SECONDS ]; then - log_message "" - log_message "[TIMEOUT] Test execution exceeded ${TIMEOUT_MINUTES} minutes - terminating" - kill -9 $MAVEN_PID 2>/dev/null || true - # Also kill any child Java processes - pkill -9 -P $MAVEN_PID 2>/dev/null || true - TEST_RESULT="TIMEOUT" - break - fi - done - - if [ "$TEST_RESULT" != "TIMEOUT" ]; then - wait $MAVEN_PID - if [ $? -eq 0 ]; then - TEST_RESULT="SUCCESS" - else - TEST_RESULT="FAILURE" - fi - fi - set +o pipefail -else - # Run Maven normally (all output goes to terminal AND log file) - # Use pipefail so the pipeline returns mvn's exit code, not tee's - set -o pipefail - if mvn clean test 2>&1 | tee "${DETAIL_LOG}"; then - TEST_RESULT="SUCCESS" - else - TEST_RESULT="FAILURE" - fi - set +o pipefail -fi - -################################################################################ -# GENERATE HTML REPORT -################################################################################ - -print_header "Generating HTML Report" -log_message "Running: mvn surefire-report:report-only -DskipTests" - -# Generate HTML report from surefire XML files (without re-running tests) -if mvn surefire-report:report-only -DskipTests 2>&1; then - log_message "[OK] HTML report generated" - - # Copy HTML reports to test-results directory for easy access - HTML_REPORT_DIR="${LOG_DIR}/html-reports" - mkdir -p "${HTML_REPORT_DIR}" - - # Copy reports from both modules - if [ -f "obp-api/target/surefire-reports/surefire.html" ]; then - cp "obp-api/target/surefire-reports/surefire.html" "${HTML_REPORT_DIR}/obp-api-report.html" - # Also copy CSS, JS, images for proper rendering - cp -r "obp-api/target/surefire-reports/css" "${HTML_REPORT_DIR}/" 2>/dev/null || true - cp -r "obp-api/target/surefire-reports/js" "${HTML_REPORT_DIR}/" 2>/dev/null || true - cp -r "obp-api/target/surefire-reports/images" "${HTML_REPORT_DIR}/" 2>/dev/null || true - cp -r "obp-api/target/surefire-reports/fonts" "${HTML_REPORT_DIR}/" 2>/dev/null || true - cp -r "obp-api/target/surefire-reports/img" "${HTML_REPORT_DIR}/" 2>/dev/null || true - log_message " - obp-api report: ${HTML_REPORT_DIR}/obp-api-report.html" - fi - if [ -f "obp-commons/target/surefire-reports/surefire.html" ]; then - cp "obp-commons/target/surefire-reports/surefire.html" "${HTML_REPORT_DIR}/obp-commons-report.html" - log_message " - obp-commons report: ${HTML_REPORT_DIR}/obp-commons-report.html" - fi - - # Also check for site reports location (alternative naming) - if [ -f "obp-api/target/site/surefire-report.html" ]; then - cp "obp-api/target/site/surefire-report.html" "${HTML_REPORT_DIR}/obp-api-report.html" - log_message " - obp-api report: ${HTML_REPORT_DIR}/obp-api-report.html" - fi - if [ -f "obp-commons/target/site/surefire-report.html" ]; then - cp "obp-commons/target/site/surefire-report.html" "${HTML_REPORT_DIR}/obp-commons-report.html" - log_message " - obp-commons report: ${HTML_REPORT_DIR}/obp-commons-report.html" - fi -else - log_message "[WARNING] Failed to generate HTML report" -fi - -log_message "" - -# Stop background monitor by removing flag file -rm -f "${MONITOR_FLAG}" -sleep 1 -kill $MONITOR_PID 2>/dev/null || true -wait $MONITOR_PID 2>/dev/null || true - -END_TIME=$(date +%s) -DURATION=$((END_TIME - START_TIME)) -DURATION_MIN=$((DURATION / 60)) -DURATION_SEC=$((DURATION % 60)) - -# Update title with final results (no suite/scenario name for Complete phase) -FINAL_ELAPSED=$(printf "%dm %ds" $DURATION_MIN $DURATION_SEC) -# Build final counts with module context -FINAL_COMMONS=$(sed -n '/Building Open Bank Project Commons/,/Building Open Bank Project API/{/Tests: succeeded/p;}' "${DETAIL_LOG}" 2>/dev/null | sed 's/.*succeeded //' | sed 's/,.*//' | head -1) -FINAL_API=$(sed -n '/Building Open Bank Project API/,/OBP Http4s Runner/{/Tests: succeeded/p;}' "${DETAIL_LOG}" 2>/dev/null | sed 's/.*succeeded //' | sed 's/,.*//' | tail -1) -FINAL_COUNTS="" -[ -n "$FINAL_COMMONS" ] && FINAL_COUNTS="commons:+${FINAL_COMMONS}" -[ -n "$FINAL_API" ] && FINAL_COUNTS="${FINAL_COUNTS:+${FINAL_COUNTS} }api:+${FINAL_API}" - -# Record complete phase start and change to green for completion phase -record_phase_time "complete" -set_terminal_style "Complete" -update_terminal_title "Complete" "$FINAL_ELAPSED" "$FINAL_COUNTS" "" "" - -################################################################################ -# GENERATE SUMMARY (using DRY function) -################################################################################ - -if generate_summary "${DETAIL_LOG}" "${SUMMARY_LOG}" "$START_TIME" "$END_TIME"; then - EXIT_CODE=0 -else - EXIT_CODE=1 -fi - -# Record end time for complete phase -record_phase_time "end" - -log_message "" -log_message "Logs saved to:" -log_message " $(realpath "${DETAIL_LOG}")" -log_message " $(realpath "${SUMMARY_LOG}")" -if [ -f "${FAILED_TESTS_FILE}" ]; then - log_message " $(realpath "${FAILED_TESTS_FILE}")" -fi -if [ -d "${LOG_DIR}/html-reports" ]; then - log_message "" - log_message "HTML Reports:" - for report in "${LOG_DIR}/html-reports"/*.html; do - [ -f "$report" ] && log_message " $(realpath "$report")" - done -fi -echo "" - -exit ${EXIT_CODE} From 5a9c69dd0844571113a54fc71a0444ac18d23756 Mon Sep 17 00:00:00 2001 From: hongwei Date: Fri, 26 Jun 2026 10:56:44 +0200 Subject: [PATCH 10/29] refactor(context): extract processUacAnswer to reduce cognitive complexity --- .../MappedUserAuthContextUpdateProvider.scala | 51 +++++++++---------- 1 file changed, 23 insertions(+), 28 deletions(-) diff --git a/obp-api/src/main/scala/code/context/MappedUserAuthContextUpdateProvider.scala b/obp-api/src/main/scala/code/context/MappedUserAuthContextUpdateProvider.scala index 5cd692eaf3..a95ccbed22 100644 --- a/obp-api/src/main/scala/code/context/MappedUserAuthContextUpdateProvider.scala +++ b/obp-api/src/main/scala/code/context/MappedUserAuthContextUpdateProvider.scala @@ -51,36 +51,31 @@ object MappedUserAuthContextUpdateProvider extends UserAuthContextUpdateProvider override def checkAnswer(consentId: String, challenge: String): Future[Box[MappedUserAuthContextUpdate]] = Future { MappedUserAuthContextUpdate.find(By(MappedUserAuthContextUpdate.mUserAuthContextUpdateId, consentId)) match { - case Full(consent) => - val createDateTime = consent.createdAt.get - val challengeTTL : Long = Helpers.seconds(APIUtil.userAuthContextUpdateRequestChallengeTtl) - val expiredDateTime: Long = createDateTime.getTime+challengeTTL - val currentTime: Long = Platform.currentTime - - if(expiredDateTime > currentTime) - consent.status match { - case value if value == UserAuthContextUpdateStatus.INITIATED.toString => - val status = if (consent.challenge == challenge) UserAuthContextUpdateStatus.ACCEPTED.toString else UserAuthContextUpdateStatus.REJECTED.toString - // Atomic guarded transition: only one concurrent answer may move INITIATED -> status, - // so two correct answers cannot both be accepted (MFA double-authorisation). - val rows = code.bankconnectors.DoobieUserAuthContextUpdateQueries - .conditionalStatusTransition(consent.id.get, UserAuthContextUpdateStatus.INITIATED.toString, status) - if (rows == 1) MappedUserAuthContextUpdate.find(By(MappedUserAuthContextUpdate.mUserAuthContextUpdateId, consentId)) - else Failure("UserAuthContextUpdate status changed concurrently; it is no longer INITIATED.") - case _ => - Full(consent) - } - else{ - Failure(s"${ErrorMessages.OneTimePasswordExpired} Current expiration time is ${APIUtil.userAuthContextUpdateRequestChallengeTtl} seconds") - } - case Empty => - Empty ?~! ErrorMessages.UserAuthContextUpdateNotFound - case Failure(msg, _, _) => - Failure(msg) - case _ => - Failure(ErrorMessages.UnknownError) + case Full(consent) => processUacAnswer(consent, challenge, consentId) + case Empty => Empty ?~! ErrorMessages.UserAuthContextUpdateNotFound + case Failure(msg, _, _) => Failure(msg) + case _ => Failure(ErrorMessages.UnknownError) } + } + private def processUacAnswer(consent: MappedUserAuthContextUpdate, challenge: String, consentId: String): Box[MappedUserAuthContextUpdate] = { + val expiredDateTime: Long = consent.createdAt.get.getTime + Helpers.seconds(APIUtil.userAuthContextUpdateRequestChallengeTtl) + if (expiredDateTime <= Platform.currentTime) { + Failure(s"${ErrorMessages.OneTimePasswordExpired} Current expiration time is ${APIUtil.userAuthContextUpdateRequestChallengeTtl} seconds") + } else { + consent.status match { + case value if value == UserAuthContextUpdateStatus.INITIATED.toString => + val status = if (consent.challenge == challenge) UserAuthContextUpdateStatus.ACCEPTED.toString else UserAuthContextUpdateStatus.REJECTED.toString + // Atomic guarded transition: only one concurrent answer may move INITIATED -> status, + // so two correct answers cannot both be accepted (MFA double-authorisation). + val rows = code.bankconnectors.DoobieUserAuthContextUpdateQueries + .conditionalStatusTransition(consent.id.get, UserAuthContextUpdateStatus.INITIATED.toString, status) + if (rows == 1) MappedUserAuthContextUpdate.find(By(MappedUserAuthContextUpdate.mUserAuthContextUpdateId, consentId)) + else Failure("UserAuthContextUpdate status changed concurrently; it is no longer INITIATED.") + case _ => + Full(consent) + } + } } } From e33eb5b2d86af75165ffe7faba76c1979858e281 Mon Sep 17 00:00:00 2001 From: hongwei Date: Fri, 26 Jun 2026 11:33:29 +0200 Subject: [PATCH 11/29] fix(concurrency): return 404 not 400 for missing transaction request on status lock MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit atomicallyLockTransactionRequest used .query[String].unique, which raises UnexpectedEnd when the transaction-request row does not exist. runUpdate runs it with unsafeRunSync, so the exception propagates; lockTransactionRequest's tryo converts it to a Failure box, the caller's .isDefined is then false, and booleanToFuture fires its default 400 before the handler reaches getTransactionRequestImpl — so an authorised caller hitting a non-existent request id got 400 'Failed to acquire transaction request lock' instead of 404. Switch to .query[String].option so a missing row returns Full(None) (query ran cleanly) and only a genuine DB/lock error yields a Failure. The Box .isDefined check in both callers (Http4s510 status update, Http4s400 challenge answer) now passes for a missing row, letting the downstream lookup return the correct 404; the 400 is reserved for real lock failures. --- .../DoobieTransactionRequestQueries.scala | 16 +++++++++++++--- 1 file changed, 13 insertions(+), 3 deletions(-) diff --git a/obp-api/src/main/scala/code/bankconnectors/DoobieTransactionRequestQueries.scala b/obp-api/src/main/scala/code/bankconnectors/DoobieTransactionRequestQueries.scala index 80cb02a548..25d319427d 100644 --- a/obp-api/src/main/scala/code/bankconnectors/DoobieTransactionRequestQueries.scala +++ b/obp-api/src/main/scala/code/bankconnectors/DoobieTransactionRequestQueries.scala @@ -12,12 +12,22 @@ object DoobieTransactionRequestQueries { * Atomically locks the transaction request row using SELECT FOR UPDATE. * This ensures that concurrent MFA challenge answers cannot be processed simultaneously * for the same transaction request. + * + * Returns the locked row's status as Some, or None when the request id does not exist. + * `.option` (not `.unique`) is deliberate: a missing row must not raise an exception, so + * the caller can let the downstream lookup return the correct 404 instead of a misleading + * "lock failed" 400. */ - def atomicallyLockTransactionRequest(transReqId: String): ConnectionIO[String] = { - sql"SELECT mstatus FROM mappedtransactionrequest WHERE mtransactionrequestid = $transReqId FOR UPDATE".query[String].unique + def atomicallyLockTransactionRequest(transReqId: String): ConnectionIO[Option[String]] = { + sql"SELECT mstatus FROM mappedtransactionrequest WHERE mtransactionrequestid = $transReqId FOR UPDATE".query[String].option } - def lockTransactionRequest(transReqId: String): Box[String] = { + /** + * Box semantics: Full(Some(status)) = row locked; Full(None) = request id does not exist + * (query ran cleanly); Failure = a genuine DB/lock error. Callers check `.isDefined` on the + * Box, so only a real lock failure short-circuits with 400 — a missing row falls through. + */ + def lockTransactionRequest(transReqId: String): Box[Option[String]] = { tryo { DoobieUtil.runUpdate(atomicallyLockTransactionRequest(transReqId)) } From 0df31c4ed8b6ef3d364f0a848cd6097042157b8f Mon Sep 17 00:00:00 2001 From: hongwei Date: Fri, 26 Jun 2026 11:33:39 +0200 Subject: [PATCH 12/29] fix(consent): fail visibly when skip-SCA consent vanishes after creation The skip-SCA auto-accept did MappedConsent.find(...).map(c => conditionalStatusTransition(...)). On an Empty box the map yielded Empty, which the for-comprehension's _ <- discarded silently, leaving the consent INITIATED with no error. The consent is created earlier in the same request so Empty is effectively unreachable, but a silent no-op is wrong. Unwrap the box with openOrThrowException so a missing consent surfaces as a clear internal error, preserving the prior fail-visibly contract while keeping the atomic guarded INITIATED -> ACCEPTED transition. --- obp-api/src/main/scala/code/api/v3_1_0/Http4s310.scala | 9 ++++++--- 1 file changed, 6 insertions(+), 3 deletions(-) diff --git a/obp-api/src/main/scala/code/api/v3_1_0/Http4s310.scala b/obp-api/src/main/scala/code/api/v3_1_0/Http4s310.scala index 7be10aa896..766a930675 100644 --- a/obp-api/src/main/scala/code/api/v3_1_0/Http4s310.scala +++ b/obp-api/src/main/scala/code/api/v3_1_0/Http4s310.scala @@ -4437,9 +4437,12 @@ object Http4s310 { // Atomic guarded auto-accept: only move INITIATED -> ACCEPTED. If the consent was // concurrently revoked, the conditional UPDATE is a 0-row no-op and the revoke stands, // instead of the skip-SCA write blindly resurrecting it to ACCEPTED. - MappedConsent.find(By(MappedConsent.mConsentId, createdConsent.consentId)) - .map(c => code.bankconnectors.DoobieConsentStatusQueries - .conditionalStatusTransition(c.id.get, ConsentStatus.INITIATED.toString, ConsentStatus.ACCEPTED.toString)) + // The consent was just created above, so find must succeed; fail visibly (rather than + // silently leaving it INITIATED) if it has vanished — that is an internal inconsistency. + val consent = MappedConsent.find(By(MappedConsent.mConsentId, createdConsent.consentId)) + .openOrThrowException(s"Consent ${createdConsent.consentId} not found immediately after creation") + code.bankconnectors.DoobieConsentStatusQueries + .conditionalStatusTransition(consent.id.get, ConsentStatus.INITIATED.toString, ConsentStatus.ACCEPTED.toString) } } else { val challengeText = s"Your consent challenge : $challengeAnswer, Application: $applicationText" From 13f247977c76beaf8d6d5f2b5157fe20aa8c9d46 Mon Sep 17 00:00:00 2001 From: hongwei Date: Wed, 1 Jul 2026 16:46:17 +0200 Subject: [PATCH 13/29] fix(errors): replace bare-string Failure messages with OBP-XXX codes MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Four concurrency-guard rejection messages introduced by the recent conditional-UPDATE fixes returned plain English strings instead of OBP-XXXXX coded errors, breaking the API's error-code contract (clients match on the OBP- prefix). - AccountAccessRequest.updateStatus: reuse the existing AccountAccessRequestStatusNotInitiated (OBP-30278) — the same condition the caller already checks non-concurrently. - MappedConsent.checkAnswer: reuse the existing ConsentUpdateStatusError (OBP-35025). - MappedUserAuthContextUpdateProvider.checkAnswer: add UserAuthContextUpdateStatusError (OBP-30090, filling a gap in the 300XX block) — no existing constant covered this case. - Http4s510 updateTransactionRequestStatus lock gate: add TransactionRequestLockFailed (OBP-40050) — no existing constant covered this case. --- .../code/accountaccessrequest/AccountAccessRequest.scala | 5 +++-- obp-api/src/main/scala/code/api/util/ErrorMessages.scala | 2 ++ obp-api/src/main/scala/code/api/v5_1_0/Http4s510.scala | 2 +- obp-api/src/main/scala/code/consent/MappedConsent.scala | 2 +- .../code/context/MappedUserAuthContextUpdateProvider.scala | 2 +- 5 files changed, 8 insertions(+), 5 deletions(-) diff --git a/obp-api/src/main/scala/code/accountaccessrequest/AccountAccessRequest.scala b/obp-api/src/main/scala/code/accountaccessrequest/AccountAccessRequest.scala index 2bc25655ec..7003040d6e 100644 --- a/obp-api/src/main/scala/code/accountaccessrequest/AccountAccessRequest.scala +++ b/obp-api/src/main/scala/code/accountaccessrequest/AccountAccessRequest.scala @@ -1,9 +1,10 @@ package code.accountaccessrequest import java.util.Date +import code.api.util.ErrorMessages import code.util.{MappedUUID, UUIDString} import com.openbankproject.commons.model.enums.AccountAccessRequestStatus -import net.liftweb.common.{Box, Full} +import net.liftweb.common.{Box, Failure, Full} import net.liftweb.mapper._ import net.liftweb.util.Helpers.tryo @@ -95,7 +96,7 @@ object MappedAccountAccessRequestProvider extends AccountAccessRequestProvider { val rows = code.bankconnectors.DoobieBusinessStatusQueries.conditionalAccountAccessRequestStatus( request.id.get, AccountAccessRequestStatus.INITIATED.toString, status, checkerUserId, checkerComment) if (rows == 1) AccountAccessRequest.find(By(AccountAccessRequest.AccountAccessRequestId, accountAccessRequestId)) - else net.liftweb.common.Failure("Account access request is no longer INITIATED; it was already actioned.") + else Failure(ErrorMessages.AccountAccessRequestStatusNotInitiated) } } } diff --git a/obp-api/src/main/scala/code/api/util/ErrorMessages.scala b/obp-api/src/main/scala/code/api/util/ErrorMessages.scala index 7fcf4e3d3b..c4efd3a444 100644 --- a/obp-api/src/main/scala/code/api/util/ErrorMessages.scala +++ b/obp-api/src/main/scala/code/api/util/ErrorMessages.scala @@ -423,6 +423,7 @@ object ErrorMessages { val UpdateUserAuthContextNotFound = "OBP-30055: UserAuthContext not found. Please specify a valid value for USER_ID." val DeleteUserAuthContextNotFound = "OBP-30056: UserAuthContext not found by USER_AUTH_CONTEXT_ID." val UserAuthContextUpdateNotFound = "OBP-30057: User Auth Context Update not found by AUTH_CONTEXT_UPDATE_ID." + val UserAuthContextUpdateStatusError = "OBP-30090: User Auth Context Update status is not INITIATED. It has already been actioned." val UpdateCustomerError = "OBP-30058: Cannot update the Customer" val CardNotFound = "OBP-30059: This Card can not be found for the user " @@ -869,6 +870,7 @@ object ErrorMessages { val DynamicCodeLangNotSupport = "OBP-40049: This language of dynamic code is not supported. " val InvalidOperationId = "OBP-40048: Invalid operation_id, please specify valid operation_id." + val TransactionRequestLockFailed = "OBP-40050: Could not acquire a lock on the Transaction Request. Please try again." // Exceptions (OBP-50XXX) val UnknownError = "OBP-50000: Unknown Error." val FutureTimeoutException = "OBP-50001: Future Timeout Exception." 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 2c08a99f1b..613bae4d90 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 @@ -3385,7 +3385,7 @@ object Http4s510 { // through the read + status write below). Without it this management update races the // challenge-answer path (Http4s400, which already locks) and can overwrite a COMPLETED // payment with a stale status. - _ <- code.util.Helper.booleanToFuture("Failed to acquire transaction request lock", cc = Some(cc)) { + _ <- code.util.Helper.booleanToFuture(TransactionRequestLockFailed, cc = Some(cc)) { code.bankconnectors.DoobieTransactionRequestQueries.lockTransactionRequest(requestId.value).isDefined } (existing, _) <- NewStyle.function.getTransactionRequestImpl(requestId, Some(cc)) diff --git a/obp-api/src/main/scala/code/consent/MappedConsent.scala b/obp-api/src/main/scala/code/consent/MappedConsent.scala index 451649f491..9807c4b6c4 100644 --- a/obp-api/src/main/scala/code/consent/MappedConsent.scala +++ b/obp-api/src/main/scala/code/consent/MappedConsent.scala @@ -423,7 +423,7 @@ object MappedConsentProvider extends ConsentProvider with code.util.Helper.MdcLo val rows = code.bankconnectors.DoobieConsentStatusQueries .conditionalStatusTransition(consent.id.get, ConsentStatus.INITIATED.toString, status) if (rows == 1) MappedConsent.find(By(MappedConsent.mConsentId, consentId)) - else Failure("Consent status changed concurrently; it is no longer INITIATED.") + else Failure(ErrorMessages.ConsentUpdateStatusError) case _ => Full(consent) } diff --git a/obp-api/src/main/scala/code/context/MappedUserAuthContextUpdateProvider.scala b/obp-api/src/main/scala/code/context/MappedUserAuthContextUpdateProvider.scala index a95ccbed22..fbd7811419 100644 --- a/obp-api/src/main/scala/code/context/MappedUserAuthContextUpdateProvider.scala +++ b/obp-api/src/main/scala/code/context/MappedUserAuthContextUpdateProvider.scala @@ -71,7 +71,7 @@ object MappedUserAuthContextUpdateProvider extends UserAuthContextUpdateProvider val rows = code.bankconnectors.DoobieUserAuthContextUpdateQueries .conditionalStatusTransition(consent.id.get, UserAuthContextUpdateStatus.INITIATED.toString, status) if (rows == 1) MappedUserAuthContextUpdate.find(By(MappedUserAuthContextUpdate.mUserAuthContextUpdateId, consentId)) - else Failure("UserAuthContextUpdate status changed concurrently; it is no longer INITIATED.") + else Failure(ErrorMessages.UserAuthContextUpdateStatusError) case _ => Full(consent) } From 678f61ec853bbe89a5e3a13a899931683ffadbe4 Mon Sep 17 00:00:00 2001 From: hongwei Date: Thu, 2 Jul 2026 06:55:36 +0200 Subject: [PATCH 14/29] fix(consent): apply skip-SCA guarded auto-accept to v5.0.0 and v5.1.0 createConsent MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The conditional INITIATED -> ACCEPTED auto-accept was only applied to the v3.1.0 createConsent path; Http4s500 and Http4s510 still carried the verbatim pre-fix line .map(_.mStatus(ACCEPTED).saveMe()).head — both the concurrent-revoke resurrection race and the .head NoSuchElementException on a vanished consent remained live on the two current API versions. Add conditionalStatusTransitionByConsentId (keyed by consent id, no extra SELECT to obtain the row id) and use it at all three call sites. The M5 race test now exercises this exact shared helper. The v3.1.0 site also drops its redundant re-find of the consent it created moments earlier. --- .../scala/code/api/v3_1_0/Http4s310.scala | 8 ++---- .../scala/code/api/v5_0_0/Http4s500.scala | 7 ++++- .../scala/code/api/v5_1_0/Http4s510.scala | 7 ++++- .../DoobieConsentStatusQueries.scala | 26 ++++++++++++++++--- .../ConcurrentConsentStatusRaceTest.scala | 10 +++---- 5 files changed, 41 insertions(+), 17 deletions(-) diff --git a/obp-api/src/main/scala/code/api/v3_1_0/Http4s310.scala b/obp-api/src/main/scala/code/api/v3_1_0/Http4s310.scala index 766a930675..168094cbc3 100644 --- a/obp-api/src/main/scala/code/api/v3_1_0/Http4s310.scala +++ b/obp-api/src/main/scala/code/api/v3_1_0/Http4s310.scala @@ -4437,12 +4437,8 @@ object Http4s310 { // Atomic guarded auto-accept: only move INITIATED -> ACCEPTED. If the consent was // concurrently revoked, the conditional UPDATE is a 0-row no-op and the revoke stands, // instead of the skip-SCA write blindly resurrecting it to ACCEPTED. - // The consent was just created above, so find must succeed; fail visibly (rather than - // silently leaving it INITIATED) if it has vanished — that is an internal inconsistency. - val consent = MappedConsent.find(By(MappedConsent.mConsentId, createdConsent.consentId)) - .openOrThrowException(s"Consent ${createdConsent.consentId} not found immediately after creation") - code.bankconnectors.DoobieConsentStatusQueries - .conditionalStatusTransition(consent.id.get, ConsentStatus.INITIATED.toString, ConsentStatus.ACCEPTED.toString) + code.bankconnectors.DoobieConsentStatusQueries.conditionalStatusTransitionByConsentId( + createdConsent.consentId, ConsentStatus.INITIATED.toString, ConsentStatus.ACCEPTED.toString) } } else { val challengeText = s"Your consent challenge : $challengeAnswer, Application: $applicationText" diff --git a/obp-api/src/main/scala/code/api/v5_0_0/Http4s500.scala b/obp-api/src/main/scala/code/api/v5_0_0/Http4s500.scala index 72592fdc80..40fcd87a1e 100644 --- a/obp-api/src/main/scala/code/api/v5_0_0/Http4s500.scala +++ b/obp-api/src/main/scala/code/api/v5_0_0/Http4s500.scala @@ -1288,8 +1288,13 @@ object Http4s500 { APIUtil.ConsumerIdPair(grantorConsumerId, granteeConsumerId)) mappedConsent <- if (shouldSkipConsentScaForConsumerIdPair) { Future { + // Atomic guarded auto-accept: only move INITIATED -> ACCEPTED. If the consent was + // concurrently revoked, the conditional UPDATE is a 0-row no-op and the revoke stands, + // instead of the skip-SCA write blindly resurrecting it to ACCEPTED. + code.bankconnectors.DoobieConsentStatusQueries.conditionalStatusTransitionByConsentId( + createdConsent.consentId, ConsentStatus.INITIATED.toString, ConsentStatus.ACCEPTED.toString) MappedConsent.find(By(MappedConsent.mConsentId, createdConsent.consentId)) - .map(_.mStatus(ConsentStatus.ACCEPTED.toString).saveMe()).head + .openOrThrowException(s"Consent ${createdConsent.consentId} not found immediately after creation") } } else { val challengeText = s"Your consent challenge : ${challengeAnswer}, Application: $applicationText" 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 613bae4d90..fd9fdb55ea 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 @@ -4740,8 +4740,13 @@ object Http4s510 { APIUtil.ConsumerIdPair(grantorConsumerId, granteeConsumerId)) mappedConsent <- if (shouldSkip) { Future { + // Atomic guarded auto-accept: only move INITIATED -> ACCEPTED. If the consent was + // concurrently revoked, the conditional UPDATE is a 0-row no-op and the revoke stands, + // instead of the skip-SCA write blindly resurrecting it to ACCEPTED. + code.bankconnectors.DoobieConsentStatusQueries.conditionalStatusTransitionByConsentId( + createdConsent.consentId, ConsentStatus.INITIATED.toString, ConsentStatus.ACCEPTED.toString) MappedConsent.find(By(MappedConsent.mConsentId, createdConsent.consentId)) - .map(_.mStatus(ConsentStatus.ACCEPTED.toString).saveMe()).head + .openOrThrowException(s"Consent ${createdConsent.consentId} not found immediately after creation") } } else { val challengeText = s"Your consent challenge : ${challengeAnswer}, Application: $applicationText" diff --git a/obp-api/src/main/scala/code/bankconnectors/DoobieConsentStatusQueries.scala b/obp-api/src/main/scala/code/bankconnectors/DoobieConsentStatusQueries.scala index e28d91abec..153ef914ea 100644 --- a/obp-api/src/main/scala/code/bankconnectors/DoobieConsentStatusQueries.scala +++ b/obp-api/src/main/scala/code/bankconnectors/DoobieConsentStatusQueries.scala @@ -12,26 +12,44 @@ import doobie.implicits._ * check and the write cannot interleave across concurrent requests. The returned affected-row * count tells the caller whether it won the transition (1) or lost it to a concurrent request (0). * - * Row-id keyed (not consent-id) because every call site already holds the loaded MappedConsent. + * `updatedat` is bumped alongside the status so the write matches what the replaced Lift + * saveMe() (CreatedUpdated trait) persisted. */ object DoobieConsentStatusQueries { - /** Transition mstatus from an expected guard value to a new value. Returns affected rows (0 or 1). */ + /** Transition mstatus from an expected guard value to a new value, keyed by row id + * (for call sites already holding the loaded MappedConsent). Returns affected rows (0 or 1). */ def conditionalStatusTransition(consentRowId: Long, guardStatus: String, newStatus: String): Int = DoobieUtil.runUpdate( sql"""UPDATE mappedconsent SET mstatus = $newStatus, - mlastactiondate = NOW() + mlastactiondate = NOW(), + updatedat = NOW() WHERE id = $consentRowId AND mstatus = $guardStatus""".update.run ) + /** Transition mstatus from an expected guard value to a new value, keyed by consent id. + * Used by the skip-SCA auto-accept in the createConsent endpoints (v3.1.0 / v5.0.0 / v5.1.0), + * which hold only the consentId — no extra SELECT needed to obtain the row id. + * Returns affected rows (0 or 1). */ + def conditionalStatusTransitionByConsentId(consentId: String, guardStatus: String, newStatus: String): Int = + DoobieUtil.runUpdate( + sql"""UPDATE mappedconsent + SET mstatus = $newStatus, + mlastactiondate = NOW(), + updatedat = NOW() + WHERE mconsentid = $consentId + AND mstatus = $guardStatus""".update.run + ) + /** Revoke unless already at the given terminal status. Returns affected rows (0 or 1). */ def conditionalRevoke(consentRowId: Long, revokedStatus: String): Int = DoobieUtil.runUpdate( sql"""UPDATE mappedconsent SET mstatus = $revokedStatus, - mlastactiondate = NOW() + mlastactiondate = NOW(), + updatedat = NOW() WHERE id = $consentRowId AND mstatus <> $revokedStatus""".update.run ) diff --git a/obp-api/src/test/scala/code/concurrency/ConcurrentConsentStatusRaceTest.scala b/obp-api/src/test/scala/code/concurrency/ConcurrentConsentStatusRaceTest.scala index b4702d9a59..5f290e85ef 100644 --- a/obp-api/src/test/scala/code/concurrency/ConcurrentConsentStatusRaceTest.scala +++ b/obp-api/src/test/scala/code/concurrency/ConcurrentConsentStatusRaceTest.scala @@ -151,11 +151,11 @@ class ConcurrentConsentStatusRaceTest extends ConcurrentRaceSetup { When("one thread runs the skip-SCA accept-write while another concurrently revokes the consent") val results = runConcurrentWithBarrier(n) { i => if (i == 0) { - // Fixed production path in Http4s310: conditional UPDATE WHERE mstatus='INITIATED'. - // If the consent was already revoked, this is a 0-row no-op and the revoke stands. - MappedConsent.find(By(MappedConsent.mConsentId, consentId)) - .map(c => code.bankconnectors.DoobieConsentStatusQueries - .conditionalStatusTransition(c.id.get, ConsentStatus.INITIATED.toString, ConsentStatus.ACCEPTED.toString)) + // The shared production skip-SCA helper (Http4s310/500/510 all call this exact method): + // conditional UPDATE WHERE mstatus='INITIATED'. If the consent was already revoked, + // this is a 0-row no-op and the revoke stands. + code.bankconnectors.DoobieConsentStatusQueries + .conditionalStatusTransitionByConsentId(consentId, ConsentStatus.INITIATED.toString, ConsentStatus.ACCEPTED.toString) } else { MappedConsentProvider.revoke(consentId) } From 906eb2350bc64d07332846cd430345fd3a0eebc2 Mon Sep 17 00:00:00 2001 From: hongwei Date: Thu, 2 Jul 2026 06:55:36 +0200 Subject: [PATCH 15/29] fix(account-access): win the status CAS before granting view access on approval MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit approveAccountAccessRequest granted view access BEFORE attempting the INITIATED -> APPROVED transition. When a concurrent rejection won the status race, the approver got a 400 but the already-committed grant stood: the target user kept view access on a request whose persisted status said REJECTED. Winning the conditional UPDATE first makes the CAS the single gate — the loser exits with no side effect. --- .../scala/code/api/v6_0_0/Http4s600.scala | 20 +++++++++++-------- 1 file changed, 12 insertions(+), 8 deletions(-) diff --git a/obp-api/src/main/scala/code/api/v6_0_0/Http4s600.scala b/obp-api/src/main/scala/code/api/v6_0_0/Http4s600.scala index 512ed6a8b9..0a3f16efea 100644 --- a/obp-api/src/main/scala/code/api/v6_0_0/Http4s600.scala +++ b/obp-api/src/main/scala/code/api/v6_0_0/Http4s600.scala @@ -2506,6 +2506,18 @@ object Http4s600 { u.userId != request.requestorUserId } (targetUser, _) <- NewStyle.function.findByUserId(request.targetUserId, Some(cc)) + // Win the INITIATED -> APPROVED transition BEFORE granting view access. The provider's + // conditional UPDATE makes this request the single actioner; the loser of a concurrent + // approve/reject race gets a 400 here with NO side effect. Granting first would leave + // the target user with view access when a concurrent rejection wins the status race. + updatedBox <- Future { + code.accountaccessrequest.AccountAccessRequestTrait.accountAccessRequest.vend.updateStatus( + requestIdStr, + com.openbankproject.commons.model.enums.AccountAccessRequestStatus.APPROVED.toString, + u.userId, + postJson.comment.getOrElse("")) + } + updated <- Future(unboxFullOrFail(updatedBox, Some(cc), AccountAccessRequestCannotBeUpdated, 400)) _ <- if (request.isSystemView) { ViewNewStyle.systemView(ViewId(request.viewId), Some(cc)).flatMap { view => ViewNewStyle.grantAccessToSystemView(bankId, accountId, view, targetUser, Some(cc)) @@ -2516,14 +2528,6 @@ object Http4s600 { ViewNewStyle.grantAccessToCustomView(view, targetUser, Some(cc)) } } - updatedBox <- Future { - code.accountaccessrequest.AccountAccessRequestTrait.accountAccessRequest.vend.updateStatus( - requestIdStr, - com.openbankproject.commons.model.enums.AccountAccessRequestStatus.APPROVED.toString, - u.userId, - postJson.comment.getOrElse("")) - } - updated <- Future(unboxFullOrFail(updatedBox, Some(cc), AccountAccessRequestCannotBeUpdated, 400)) } yield JSONFactory600.createAccountAccessRequestJsonV600(updated) } } From 6a44fa62a7f3e97be21f286207e044c281abcdc8 Mon Sep 17 00:00:00 2001 From: hongwei Date: Thu, 2 Jul 2026 06:55:36 +0200 Subject: [PATCH 16/29] fix(rate-limit): self-heal expiry-less counter keys and return TTL atomically MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The Lua rewrite dropped the old implementation's recovery branch for a counter key stuck without an expiry (legacy writer, PERSIST, RDB restore dropping expiries): such a key incremented forever and permanently rate-limited its consumer. The script now recreates the window (count=1, fresh TTL) when it finds a key with TTL < 0. The script also returns {count, ttl} together, removing the separate TTL round trip per period per request — halving increment-phase Redis I/O on the hottest path — and eliminating the non-atomic TTL read that could observe a different window (or fabricate a default on failure). Connection boilerplate consolidated into a withJedis loan helper. --- .../src/main/scala/code/api/cache/Redis.scala | 74 ++++++++++--------- .../code/api/util/RateLimitingUtil.scala | 8 +- .../ConcurrentRateLimiterRaceTest.scala | 2 +- 3 files changed, 45 insertions(+), 39 deletions(-) diff --git a/obp-api/src/main/scala/code/api/cache/Redis.scala b/obp-api/src/main/scala/code/api/cache/Redis.scala index 631155a524..4b3a7477ca 100644 --- a/obp-api/src/main/scala/code/api/cache/Redis.scala +++ b/obp-api/src/main/scala/code/api/cache/Redis.scala @@ -201,6 +201,14 @@ object Redis extends MdcLoggable { } } + /** Loan-pattern helper: lease a Jedis connection from the pool, run f, always close. */ + private def withJedis[A](f: Jedis => A): A = { + val jedis = jedisPool.getResource() + try f(jedis) + catch { case e: Throwable => throw new RuntimeException(e) } + finally jedis.close() + } + /** * Atomic `SET key value EX ttlSeconds NX` (Jedis 2.9.0 five-arg overload). Sets the key with a TTL * only if it does not already exist, in a single command. Returns true iff this call set the key. @@ -209,45 +217,43 @@ object Redis extends MdcLoggable { * window between "set value" and "set TTL" (unlike setnx + expire), so a crash can never leave a * key without an expiry, and a second writer can never clobber the first. */ - def setNxEx(key: String, value: String, ttlSeconds: Int): Boolean = { - var jedisConnection: Option[Jedis] = None - try { - jedisConnection = Some(jedisPool.getResource()) - jedisConnection.get.set(key, value, "NX", "EX", ttlSeconds) == "OK" - } catch { - case e: Throwable => throw new RuntimeException(e) - } finally { - if (jedisConnection.isDefined && jedisConnection.get != null) - jedisConnection.foreach(_.close()) - } + def setNxEx(key: String, value: String, ttlSeconds: Int): Boolean = withJedis { jedis => + jedis.set(key, value, "NX", "EX", ttlSeconds) == "OK" } /** - * Atomic increment-with-create-TTL via a single Lua script: INCR the key, and on first creation - * (value == 1) set its expiry. Returns the post-increment counter value. Because INCR and EXPIRE - * run atomically server-side, concurrent callers cannot lose increments or race the TTL set, and an + * Counter script, executed atomically server-side: + * - INCR the key; on first creation (count == 1) set its expiry. + * - Self-heal: if the key exists WITHOUT an expiry (legacy writer, PERSIST, or an RDB restore + * that dropped expiries), recreate it as a fresh window (count = 1, new TTL) instead of + * incrementing a counter that never resets — the pre-Lua implementation had this recovery + * branch, and without it the key's consumer would be rate-limited forever. + * - Return {count, ttl} together so the caller needs no second round trip, and the pair is + * consistent (a separate TTL read could observe a different window). + */ + private val incrementWithTtlScript = + """local c = redis.call('INCR', KEYS[1]) + |if c == 1 then + | redis.call('EXPIRE', KEYS[1], ARGV[1]) + |elseif redis.call('TTL', KEYS[1]) < 0 then + | redis.call('SET', KEYS[1], 1, 'EX', ARGV[1]) + | c = 1 + |end + |return {c, redis.call('TTL', KEYS[1])}""".stripMargin + + /** + * Atomic increment-with-create-TTL via a single Lua script (see incrementWithTtlScript). + * Returns (post-increment count, remaining TTL seconds). Because everything runs atomically + * server-side, concurrent callers cannot lose increments or race the TTL set, and an * increment-then-compare rate-limit check cannot be bypassed by interleaving. */ - def incrementWithTtl(key: String, ttlSeconds: Int): Long = { - val script = - """local c = redis.call('INCR', KEYS[1]) - |if c == 1 then redis.call('EXPIRE', KEYS[1], ARGV[1]) end - |return c""".stripMargin - var jedisConnection: Option[Jedis] = None - try { - jedisConnection = Some(jedisPool.getResource()) - val result = jedisConnection.get.eval( - script, - java.util.Collections.singletonList(key), - java.util.Collections.singletonList(ttlSeconds.toString) - ) - result.asInstanceOf[java.lang.Long].longValue() - } catch { - case e: Throwable => throw new RuntimeException(e) - } finally { - if (jedisConnection.isDefined && jedisConnection.get != null) - jedisConnection.foreach(_.close()) - } + def incrementWithTtl(key: String, ttlSeconds: Int): (Long, Long) = withJedis { jedis => + val result = jedis.eval( + incrementWithTtlScript, + java.util.Collections.singletonList(key), + java.util.Collections.singletonList(ttlSeconds.toString) + ).asInstanceOf[java.util.List[java.lang.Long]] + (result.get(0).longValue(), result.get(1).longValue()) } /** diff --git a/obp-api/src/main/scala/code/api/util/RateLimitingUtil.scala b/obp-api/src/main/scala/code/api/util/RateLimitingUtil.scala index 7b3a8a459a..05566913cc 100644 --- a/obp-api/src/main/scala/code/api/util/RateLimitingUtil.scala +++ b/obp-api/src/main/scala/code/api/util/RateLimitingUtil.scala @@ -247,10 +247,10 @@ object RateLimitingUtil extends MdcLoggable { val seconds = RateLimitingPeriod.toSeconds(period).toInt try { // Atomic INCR + create-TTL in one Lua call. Replaces the former TTL-read-then-SET/INCR sequence, - // which could lose increments and race the expiry under concurrency. On first increment (cnt==1) - // the key gets its TTL atomically. - val cnt = Redis.incrementWithTtl(key, seconds) - val ttl = Redis.use(JedisMethod.TTL, key).map(_.toLong).getOrElse(seconds.toLong) + // which could lose increments and race the expiry under concurrency. The script also self-heals + // a key stuck without an expiry and returns the TTL atomically with the count — one round trip, + // no separate TTL read that could observe a different window. + val (cnt, ttl) = Redis.incrementWithTtl(key, seconds) (ttl, cnt) } catch { case e: Throwable => diff --git a/obp-api/src/test/scala/code/concurrency/ConcurrentRateLimiterRaceTest.scala b/obp-api/src/test/scala/code/concurrency/ConcurrentRateLimiterRaceTest.scala index 1f59dee045..f3a3d1c205 100644 --- a/obp-api/src/test/scala/code/concurrency/ConcurrentRateLimiterRaceTest.scala +++ b/obp-api/src/test/scala/code/concurrency/ConcurrentRateLimiterRaceTest.scala @@ -47,7 +47,7 @@ class ConcurrentRateLimiterRaceTest extends ConcurrentRaceSetup { // exactly `limit` callers can ever be allowed. (Pre-fix this was GET-then-INCR — two round // trips — and far more than `limit` slipped through; see the red baseline.) val results = runConcurrentWithBarrier(n) { _ => - val slot = Redis.incrementWithTtl(key, 3600) + val (slot, _) = Redis.incrementWithTtl(key, 3600) val underLimit = slot <= limit if (underLimit) passed.incrementAndGet() underLimit From 1969e9277938f8edd6439cb5f44dd37c60f2e553 Mon Sep 17 00:00:00 2001 From: hongwei Date: Thu, 2 Jul 2026 06:55:57 +0200 Subject: [PATCH 17/29] fix(bulk-payment): release the batch_reference when parent TR creation fails MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The batch_reference claim is deliberately committed before any payment runs, but a parent-TR creation failure (500) left the claim in place: every retry of a batch that never executed anything hit the 409 already-used guard, permanently burning the reference. Release the claim (scoped to the claiming transactionRequestId) before surfacing the 500 — at that point no payment has executed, so a retry is safe. The claim is intentionally never released after the fan-out, where payments may have partially executed. --- obp-api/src/main/scala/code/api/v7_0_0/Http4s700.scala | 10 ++++++++++ .../src/main/scala/code/bulkpayment/BulkPayment.scala | 8 ++++++++ .../main/scala/code/bulkpayment/BulkPaymentTrait.scala | 6 ++++++ 3 files changed, 24 insertions(+) diff --git a/obp-api/src/main/scala/code/api/v7_0_0/Http4s700.scala b/obp-api/src/main/scala/code/api/v7_0_0/Http4s700.scala index bfbdff958d..230081a95b 100644 --- a/obp-api/src/main/scala/code/api/v7_0_0/Http4s700.scala +++ b/obp-api/src/main/scala/code/api/v7_0_0/Http4s700.scala @@ -3329,6 +3329,16 @@ object Http4s700 { callCtx ) _ <- Future { + // Compensate before surfacing a parent-TR creation failure: the claim above has + // already been written, but NO payment has executed yet, so releasing the + // batch_reference is safe and lets the client retry a transient failure. Without + // this, the committed claim would 409 every retry of a batch that never ran. + // (Never release after the fan-out below — payments may have partially executed.) + if (parentTrBox.isEmpty) { + BulkPayments.bulkPayment.vend.releaseBatchReference( + fromAccount.bankId.value, fromAccount.accountId.value, body.batch_reference, trId + ) + } unboxFullOrFail(parentTrBox, callCtx, BulkPaymentTransactionRequestError, 500) } // 5. Fan-out — sequential per-payment execution. Returns one row diff --git a/obp-api/src/main/scala/code/bulkpayment/BulkPayment.scala b/obp-api/src/main/scala/code/bulkpayment/BulkPayment.scala index 9495ae4dbc..b1df32ebe1 100644 --- a/obp-api/src/main/scala/code/bulkpayment/BulkPayment.scala +++ b/obp-api/src/main/scala/code/bulkpayment/BulkPayment.scala @@ -57,6 +57,14 @@ object MappedBulkPaymentProvider extends BulkPaymentProvider { .saveMe() () } + + override def releaseBatchReference(fromBankId: String, fromAccountId: String, batchReference: String, transactionRequestId: String): Unit = + BulkBatchReference.find( + By(BulkBatchReference.FromBankId, fromBankId), + By(BulkBatchReference.FromAccountId, fromAccountId), + By(BulkBatchReference.BatchReference, batchReference), + By(BulkBatchReference.TransactionRequestId, transactionRequestId) + ).foreach(_.delete_!) } class BulkPayment extends BulkPaymentTrait with LongKeyedMapper[BulkPayment] with IdPK { diff --git a/obp-api/src/main/scala/code/bulkpayment/BulkPaymentTrait.scala b/obp-api/src/main/scala/code/bulkpayment/BulkPaymentTrait.scala index f9668c6dc9..9fd4638d05 100644 --- a/obp-api/src/main/scala/code/bulkpayment/BulkPaymentTrait.scala +++ b/obp-api/src/main/scala/code/bulkpayment/BulkPaymentTrait.scala @@ -35,6 +35,12 @@ trait BulkPaymentProvider { /** Mark that a batch_reference has been claimed by a TR (separate row in * the batch-references table so the check above is O(1)). */ def claimBatchReference(fromBankId: String, fromAccountId: String, batchReference: String, transactionRequestId: String): Box[Unit] + + /** Release a previously claimed batch_reference — compensation for a claim whose bulk request + * failed BEFORE any payment executed (e.g. the parent TR row could not be created), so the + * client can retry the same batch_reference. Scoped to the claiming transactionRequestId so a + * release can never delete a claim owned by a different request. */ + def releaseBatchReference(fromBankId: String, fromAccountId: String, batchReference: String, transactionRequestId: String): Unit } /** One row per payment inside a bulk TR. */ From d2e1f210aff2283b6e8fd7a6019866a1e0b4e6a2 Mon Sep 17 00:00:00 2001 From: hongwei Date: Thu, 2 Jul 2026 06:55:57 +0200 Subject: [PATCH 18/29] fix(doobie): bump updatedat in conditional status UPDATEs to match saveMe The replaced Lift saveMe() path wrote updatedAt (CreatedUpdated trait) on every status change; the raw-SQL conditional UPDATEs did not, so rows exposed through JSON 'updated' fields (e.g. AccountAccessRequest v6.0.0) showed a status change frozen at the creation timestamp. All conditional UPDATEs now set updatedat = NOW() alongside the status. --- .../bankconnectors/DoobieBusinessStatusQueries.scala | 12 +++++++++--- .../DoobieUserAuthContextUpdateQueries.scala | 3 ++- 2 files changed, 11 insertions(+), 4 deletions(-) diff --git a/obp-api/src/main/scala/code/bankconnectors/DoobieBusinessStatusQueries.scala b/obp-api/src/main/scala/code/bankconnectors/DoobieBusinessStatusQueries.scala index e9957c5217..0023380957 100644 --- a/obp-api/src/main/scala/code/bankconnectors/DoobieBusinessStatusQueries.scala +++ b/obp-api/src/main/scala/code/bankconnectors/DoobieBusinessStatusQueries.scala @@ -9,6 +9,9 @@ import doobie.implicits._ * methods were check-then-write (load row, compare status in memory, saveMe). Each method is a * single conditional UPDATE with a status guard, returning the affected-row count so the caller * can tell whether it won the transition (1) or lost it to a concurrent request (0). + * + * `updatedat` is bumped alongside each write so the UPDATE matches what the replaced Lift + * saveMe() (CreatedUpdated trait) persisted. */ object DoobieBusinessStatusQueries { @@ -23,7 +26,8 @@ object DoobieBusinessStatusQueries { sql"""UPDATE AccountAccessRequest SET status = $newStatus, checkeruserid = $checkerUserId, - checkercomment = $checkerComment + checkercomment = $checkerComment, + updatedat = NOW() WHERE id = $rowId AND status = $guardStatus""".update.run ) @@ -32,7 +36,8 @@ object DoobieBusinessStatusQueries { def conditionalAccountApplicationStatus(rowId: Long, guardStatus: String, newStatus: String): Int = DoobieUtil.runUpdate( sql"""UPDATE mappedaccountapplication - SET mstatus = $newStatus + SET mstatus = $newStatus, + updatedat = NOW() WHERE id = $rowId AND mstatus = $guardStatus""".update.run ) @@ -45,7 +50,8 @@ object DoobieBusinessStatusQueries { // NB: Lift MappedBoolean maps the `Successful` field to column `successful_c` (it appends _c). sql"""UPDATE ExpectedChallengeAnswer SET successful_c = ${true}, - scastatus = $finalisedScaStatus + scastatus = $finalisedScaStatus, + updatedat = NOW() WHERE challengeid = $challengeId AND successful_c = ${false}""".update.run ) diff --git a/obp-api/src/main/scala/code/bankconnectors/DoobieUserAuthContextUpdateQueries.scala b/obp-api/src/main/scala/code/bankconnectors/DoobieUserAuthContextUpdateQueries.scala index cc6e90143a..f82ce65eea 100644 --- a/obp-api/src/main/scala/code/bankconnectors/DoobieUserAuthContextUpdateQueries.scala +++ b/obp-api/src/main/scala/code/bankconnectors/DoobieUserAuthContextUpdateQueries.scala @@ -16,7 +16,8 @@ object DoobieUserAuthContextUpdateQueries { def conditionalStatusTransition(rowId: Long, guardStatus: String, newStatus: String): Int = DoobieUtil.runUpdate( sql"""UPDATE mappeduserauthcontextupdate - SET mstatus = $newStatus + SET mstatus = $newStatus, + updatedat = NOW() WHERE id = $rowId AND mstatus = $guardStatus""".update.run ) From 0bb830f0d1718cff6c0de2a5e8e66181da6c8a6a Mon Sep 17 00:00:00 2001 From: hongwei Date: Thu, 2 Jul 2026 06:55:57 +0200 Subject: [PATCH 19/29] fix(concurrency): warn when the transaction-request lock degrades to a no-op DoobieUtil.runUpdate silently falls back to a standalone auto-commit transactor when no request-scoped connection is available; a SELECT ... FOR UPDATE issued there acquires and immediately releases the row lock while still reporting success, so a background or scheduler caller would proceed believing it holds mutual exclusion. Expose hasRequestScopeConnection and log a loud warning from lockTransactionRequest when the lock cannot actually be held. --- .../main/scala/code/api/util/DoobieTransactor.scala | 7 +++++++ .../DoobieTransactionRequestQueries.scala | 11 ++++++++++- 2 files changed, 17 insertions(+), 1 deletion(-) diff --git a/obp-api/src/main/scala/code/api/util/DoobieTransactor.scala b/obp-api/src/main/scala/code/api/util/DoobieTransactor.scala index a8d35cf330..e91c033102 100644 --- a/obp-api/src/main/scala/code/api/util/DoobieTransactor.scala +++ b/obp-api/src/main/scala/code/api/util/DoobieTransactor.scala @@ -93,6 +93,13 @@ object DoobieUtil extends MdcLoggable { * Returns Some(connection) when a request-scoped connection is available, None otherwise * (background tasks, schedulers, tests without a request scope). */ + /** True iff a request-scoped connection is available, i.e. runUpdate will share the request + * transaction rather than falling back to a standalone auto-commit transactor. Callers whose + * correctness depends on the request transaction (e.g. SELECT ... FOR UPDATE row locks that + * must be HELD after runUpdate returns) should check this and warn or fail when false — + * the fallback transactor commits at transact end, releasing any lock immediately. */ + def hasRequestScopeConnection: Boolean = currentRequestConnection.isDefined + private def currentRequestConnection: Option[java.sql.Connection] = { // 1. Primary: the http4s RequestScopeConnection proxy from Alibaba TTL Option(code.api.util.http4s.RequestScopeConnection.currentProxy.get()).orElse { diff --git a/obp-api/src/main/scala/code/bankconnectors/DoobieTransactionRequestQueries.scala b/obp-api/src/main/scala/code/bankconnectors/DoobieTransactionRequestQueries.scala index 25d319427d..3e9635d88d 100644 --- a/obp-api/src/main/scala/code/bankconnectors/DoobieTransactionRequestQueries.scala +++ b/obp-api/src/main/scala/code/bankconnectors/DoobieTransactionRequestQueries.scala @@ -1,12 +1,13 @@ package code.bankconnectors import code.api.util.DoobieUtil +import code.util.Helper.MdcLoggable import doobie._ import doobie.implicits._ import net.liftweb.common.Box import net.liftweb.util.Helpers.tryo -object DoobieTransactionRequestQueries { +object DoobieTransactionRequestQueries extends MdcLoggable { /** * Atomically locks the transaction request row using SELECT FOR UPDATE. @@ -28,6 +29,14 @@ object DoobieTransactionRequestQueries { * Box, so only a real lock failure short-circuits with 400 — a missing row falls through. */ def lockTransactionRequest(transReqId: String): Box[Option[String]] = { + // The FOR UPDATE lock is only useful when it is HELD after this method returns, which requires + // the request-scoped transaction. Outside a request scope, runUpdate falls back to a standalone + // transactor that commits (and releases the lock) immediately — a silent no-op lock. Warn loudly + // so a scheduler/background caller does not proceed believing it holds mutual exclusion. + if (!DoobieUtil.hasRequestScopeConnection) { + logger.warn(s"lockTransactionRequest($transReqId) called without a request-scoped connection: " + + "the FOR UPDATE lock is released immediately and provides NO mutual exclusion.") + } tryo { DoobieUtil.runUpdate(atomicallyLockTransactionRequest(transReqId)) } From 65a9260c300c314fb6f329d043b21a34eb392956 Mon Sep 17 00:00:00 2001 From: hongwei Date: Thu, 2 Jul 2026 06:55:57 +0200 Subject: [PATCH 20/29] refactor(concurrency): dedup challenge CAS block, align row-count checks and messages - MappedChallengeProvider: the 9-line compare-and-set block was copy-pasted into both userId match arms; extract markChallengeSuccessful and collapse the arms into one answer+user check (userId.forall), removing the duplicated wrong-answer branch too. - MappedConsent.revoke: use rows == 1 like every other id-keyed CAS site (>= 1 implied a multi-row possibility that an id-keyed UPDATE cannot have). - MappedAccountApplication: the CAS-loser branch reported 'already been accepted' (OBP-30314) even when the concurrent winner wrote REJECTED; use the generic UpdateAccountApplicationStatusError (OBP-30315) instead. - run_specific_tests.sh: drop stale references to the deleted run_all_tests.sh. --- .../MappedAccountApplication.scala | 4 +- .../scala/code/consent/MappedConsent.scala | 2 +- .../MappedChallengeProvider.scala | 59 +++++++------------ run_specific_tests.sh | 8 +-- 4 files changed, 28 insertions(+), 45 deletions(-) diff --git a/obp-api/src/main/scala/code/accountapplication/MappedAccountApplication.scala b/obp-api/src/main/scala/code/accountapplication/MappedAccountApplication.scala index be4008278a..3cfca0a8e9 100644 --- a/obp-api/src/main/scala/code/accountapplication/MappedAccountApplication.scala +++ b/obp-api/src/main/scala/code/accountapplication/MappedAccountApplication.scala @@ -42,7 +42,9 @@ object MappedAccountApplicationProvider extends AccountApplicationProvider { val rows = code.bankconnectors.DoobieBusinessStatusQueries.conditionalAccountApplicationStatus( accountApplication.id.get, accountApplication.status, status) if (rows == 1) MappedAccountApplication.find(By(MappedAccountApplication.mAccountApplicationId, accountApplicationId)) - else Failure(s"${ErrorMessages.AccountApplicationAlreadyAccepted} Status changed concurrently. Current Account-Application-Id($accountApplicationId)") + // Use the generic update-failure code here: the concurrent winner may have written any + // status (ACCEPTED or REJECTED), so the "already accepted" message would be misleading. + else Failure(s"${ErrorMessages.UpdateAccountApplicationStatusError} Status changed concurrently. Current Account-Application-Id($accountApplicationId)") case Empty => Failure(s"${ErrorMessages.AccountApplicationNotFound} Current Account-Application-Id($accountApplicationId)") case _ => Failure(ErrorMessages.UnknownError) } diff --git a/obp-api/src/main/scala/code/consent/MappedConsent.scala b/obp-api/src/main/scala/code/consent/MappedConsent.scala index 9807c4b6c4..d6838479b4 100644 --- a/obp-api/src/main/scala/code/consent/MappedConsent.scala +++ b/obp-api/src/main/scala/code/consent/MappedConsent.scala @@ -373,7 +373,7 @@ object MappedConsentProvider extends ConsentProvider with code.util.Helper.MdcLo // already revoked makes this a 0-row no-op, so we never resurrect or double-revoke. val rows = code.bankconnectors.DoobieConsentStatusQueries .conditionalRevoke(consent.id.get, ConsentStatus.REVOKED.toString) - if (rows >= 1) MappedConsent.find(By(MappedConsent.mConsentId, consentId)) + if (rows == 1) MappedConsent.find(By(MappedConsent.mConsentId, consentId)) else Failure(ErrorMessages.ConsentAlreadyRevoked) case Empty => Empty ?~! ErrorMessages.ConsentNotFound diff --git a/obp-api/src/main/scala/code/transactionChallenge/MappedChallengeProvider.scala b/obp-api/src/main/scala/code/transactionChallenge/MappedChallengeProvider.scala index 9283dc4bdb..3d860afeaa 100644 --- a/obp-api/src/main/scala/code/transactionChallenge/MappedChallengeProvider.scala +++ b/obp-api/src/main/scala/code/transactionChallenge/MappedChallengeProvider.scala @@ -58,6 +58,16 @@ object MappedChallengeProvider extends ChallengeProvider { override def getChallenge(challengeId: String): Box[MappedExpectedChallengeAnswer] = MappedExpectedChallengeAnswer.find(By(MappedExpectedChallengeAnswer.ChallengeId,challengeId)) + /** Compare-and-set the success flag: only the first correct answer flips + * successful=false -> true. A second concurrent correct answer gets 0 rows and a + * Failure, so one challenge can never green-light a payment twice (MFA double-spend). */ + private def markChallengeSuccessful(challengeId: String): Box[MappedExpectedChallengeAnswer] = { + val rows = code.bankconnectors.DoobieBusinessStatusQueries + .conditionalChallengeSuccess(challengeId, StrongCustomerAuthenticationStatus.finalised.toString) + if (rows == 1) getChallenge(challengeId) + else Failure(s"${ErrorMessages.InvalidTransactionRequestChallengeId} Challenge already answered.") + } + override def getChallengesByTransactionRequestId(transactionRequestId: String): Box[List[ChallengeTrait]] = Full(MappedExpectedChallengeAnswer.findAll(By(MappedExpectedChallengeAnswer.TransactionRequestId,transactionRequestId))) @@ -83,45 +93,16 @@ object MappedChallengeProvider extends ChallengeProvider { if(expiredDateTime > currentTime) { val currentHashedAnswer = BCrypt.hashpw(challengeAnswer, challenge.salt).substring(0, 44) val expectedHashedAnswer = challenge.expectedAnswer - userId match { - case None => - if(currentHashedAnswer==expectedHashedAnswer) { - { - // Compare-and-set the success flag: only the first correct answer flips - // successful=false -> true. A second concurrent correct answer gets 0 rows and a - // Failure, so one challenge can never green-light a payment twice (MFA double-spend). - val rows = code.bankconnectors.DoobieBusinessStatusQueries - .conditionalChallengeSuccess(challengeId, StrongCustomerAuthenticationStatus.finalised.toString) - if (rows == 1) getChallenge(challengeId) - else Failure(s"${ErrorMessages.InvalidTransactionRequestChallengeId} Challenge already answered.") - } - } else { - Failure(s"${ - s"${ - InvalidChallengeAnswer - .replace("answer may be expired.", s"answer may be expired (${transactionRequestChallengeTtl} seconds).") - .replace("up your allowed attempts.", s"up your allowed attempts (${allowedAnswerTransactionRequestChallengeAttempts} times).") - }"}") - } - case Some(id) => - if(currentHashedAnswer==expectedHashedAnswer && id==challenge.expectedUserId) { - { - // Compare-and-set the success flag: only the first correct answer flips - // successful=false -> true. A second concurrent correct answer gets 0 rows and a - // Failure, so one challenge can never green-light a payment twice (MFA double-spend). - val rows = code.bankconnectors.DoobieBusinessStatusQueries - .conditionalChallengeSuccess(challengeId, StrongCustomerAuthenticationStatus.finalised.toString) - if (rows == 1) getChallenge(challengeId) - else Failure(s"${ErrorMessages.InvalidTransactionRequestChallengeId} Challenge already answered.") - } - } else { - Failure(s"${ - s"${ - InvalidChallengeAnswer - .replace("answer may be expired.", s"answer may be expired (${transactionRequestChallengeTtl} seconds).") - .replace("up your allowed attempts.", s"up your allowed attempts (${allowedAnswerTransactionRequestChallengeAttempts} times).") - }"}") - } + val answerMatches = currentHashedAnswer == expectedHashedAnswer + val userMatches = userId.forall(_ == challenge.expectedUserId) + if (answerMatches && userMatches) { + markChallengeSuccessful(challengeId) + } else { + Failure( + InvalidChallengeAnswer + .replace("answer may be expired.", s"answer may be expired (${transactionRequestChallengeTtl} seconds).") + .replace("up your allowed attempts.", s"up your allowed attempts (${allowedAnswerTransactionRequestChallengeAttempts} times).") + ) } }else{ Failure(s"${ErrorMessages.OneTimePasswordExpired} Current expiration time is $transactionRequestChallengeTtl seconds") diff --git a/run_specific_tests.sh b/run_specific_tests.sh index ce05948534..f196578bed 100755 --- a/run_specific_tests.sh +++ b/run_specific_tests.sh @@ -4,8 +4,8 @@ # Run Specific Tests Script # # Simple script to run specific test classes for fast iteration. -# Reads test classes from test-results/failed_tests.txt (auto-generated by run_all_tests.sh) -# or you can edit the file manually. +# Reads test classes from test-results/failed_tests.txt (edit the file manually, +# one fully-qualified test class per line). # # Usage: # ./run_specific_tests.sh @@ -79,7 +79,7 @@ fi if [ ${#SPECIFIC_TESTS[@]} -eq 0 ]; then echo "ERROR: No tests configured!" echo "Either:" - echo " 1. Run ./run_all_tests.sh first to generate ${FAILED_TESTS_FILE}" + echo " 1. Create ${FAILED_TESTS_FILE} with one fully-qualified test class per line" echo " 2. Create ${FAILED_TESTS_FILE} manually with test class names" echo " 3. Edit this script and add test names to SPECIFIC_TESTS array" exit 1 @@ -211,7 +211,7 @@ if [ -f "${FAILED_TESTS_FILE}" ]; then { echo "# All tests passed on last specific run" echo "# Updated: $(date)" - echo "# Add test classes here or run ./run_all_tests.sh to repopulate" + echo "# Add test classes here (one fully-qualified class per line)" } > "${FAILED_TESTS_FILE}" echo "Cleared ${FAILED_TESTS_FILE} — all tests passed" fi From 8d94a12b072f73cf281e221f0cbe28cb955e3840 Mon Sep 17 00:00:00 2001 From: hongwei Date: Thu, 2 Jul 2026 07:16:52 +0200 Subject: [PATCH 21/29] fix(test-runner): make the parallel runner's verdict trustworthy MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Three reliability holes let a failing run look green: - timeout exit 124 was unconditionally converted to success, so a shard whose JVM was hard-killed BEFORE tests completed still reported BUILD SUCCESS. Now 124 only counts as success when the shard log proves the tests finished (BUILD SUCCESS printed); otherwise it is a failure. - The verdict trusted only per-shard mvn exit codes. Add an authoritative surefire XML audit after the shards: any failure/error recorded in the reports fails the run and is listed by suite. Stale reports from earlier runs are deleted before the shards start so the audit (and the speed table) reflect only this run. - The ALL SHARDS PASSED / SOME SHARDS FAILED line was printed before the speed table, so piping through tail -N could truncate it. Print the verdict last, and also write PASS/FAIL to test-results/parallel/RESULT — a machine-readable signal that survives piping (a pipeline reports the LAST command's exit code, so './run_tests_parallel.sh | tail' returns tail's 0 even when the script exits 1). --- run_tests_parallel.sh | 58 +++++++++++++++++++++++++++++++++++++------ 1 file changed, 51 insertions(+), 7 deletions(-) diff --git a/run_tests_parallel.sh b/run_tests_parallel.sh index 6944c8d534..b61f0e2011 100755 --- a/run_tests_parallel.sh +++ b/run_tests_parallel.sh @@ -180,8 +180,16 @@ run_shard() { "-DwildcardSuites=${filter}" \ > "$log" 2>&1 local rc=$? - # timeout returns 124 on timeout (tests finished but the JVM didn't exit) — treat as success. - [ $rc -eq 124 ] && rc=0 + # timeout returns 124 when the JVM was killed. That is only benign when the tests had + # already finished green and only the JVM shutdown hung (Pekko non-daemon threads) — + # require proof from the log instead of blindly converting 124 to success. + if [ $rc -eq 124 ]; then + if grep -q "BUILD SUCCESS" "$log" 2>/dev/null; then + rc=0 + else + echo "[Shard $n] ⏱ timeout: JVM killed BEFORE tests completed — counted as failure" + fi + fi if [ $rc -eq 0 ]; then echo "[Shard $n] ✅ BUILD SUCCESS" else @@ -229,7 +237,11 @@ if [ $PRECOMPILE_RC -ne 0 ]; then tail -25 test-results/parallel/precompile.log >&2 exit 1 fi -echo "Pre-compile done, starting shards..." +# Fresh verdict basis: stale surefire XMLs from earlier runs would poison both the +# surefire audit below and the speed report (observed: test counts drifting across runs). +rm -rf obp-api/target/surefire-reports + +echo "Pre-compile done, starting shards..." echo "" if [ "$SHARDS" = "6" ]; then @@ -321,11 +333,32 @@ if [ $OVERALL_RC -ne 0 ]; then done fi +# ── Authoritative verdict: audit the surefire XMLs. Per-shard mvn exit codes can lie +# (timeout-killed JVMs, plugins swallowing failures), so any failure/error recorded in +# the reports fails the run regardless of what the shards returned. +SF_AUDIT=$(python3 -c ' +import xml.etree.ElementTree as ET, glob, os +tot = fail = err = 0 +bad = [] +for f in glob.glob("obp-api/target/surefire-reports/TEST-*.xml"): + try: + r = ET.parse(f).getroot() + t, fa, e = int(r.get("tests", 0)), int(r.get("failures", 0)), int(r.get("errors", 0)) + tot += t; fail += fa; err += e + if fa or e: + bad.append(os.path.basename(f)[5:-4] + ": " + str(fa) + " failed, " + str(e) + " errors") + except Exception: + pass +print(tot, fail, err) +for b in bad: + print(b) +' 2>/dev/null) +read -r SF_TOTAL SF_FAIL SF_ERR <<< "$(echo "$SF_AUDIT" | head -1)" echo "" -if [ $OVERALL_RC -eq 0 ]; then - echo "✅ ALL SHARDS PASSED" -else - echo "❌ SOME SHARDS FAILED — check test-results/parallel/shardN.log" +echo "Surefire audit: ${SF_TOTAL:-?} tests, ${SF_FAIL:-?} failures, ${SF_ERR:-?} errors" +if [ "${SF_FAIL:-1}" != "0" ] || [ "${SF_ERR:-1}" != "0" ]; then + echo "$SF_AUDIT" | tail -n +2 | sed 's/^/ ✗ /' + OVERALL_RC=1 fi # ── CI parity (report job): http4s vs Lift per-test speed table; best-effort, ── @@ -338,4 +371,15 @@ if ls "$REPORTS_DIR"/*.xml >/dev/null 2>&1; then || echo " (speed report skipped)" fi +# Final verdict LAST so `tail -N` always captures it, plus a machine-readable file +# that survives any piping of stdout (`./run.sh | tail` reports tail's exit code). +echo "" +if [ $OVERALL_RC -eq 0 ]; then + echo "✅ ALL SHARDS PASSED" + echo "PASS" > test-results/parallel/RESULT +else + echo "❌ SOME SHARDS FAILED — check test-results/parallel/shardN.log" + echo "FAIL" > test-results/parallel/RESULT +fi + exit $OVERALL_RC \ No newline at end of file From 4008bf8bb0b878ffe8f1c2350501d2042a87968a Mon Sep 17 00:00:00 2001 From: hongwei Date: Thu, 2 Jul 2026 09:34:54 +0200 Subject: [PATCH 22/29] fix(berlin-group): wrap PIIS example bodies explicitly for json4s MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Under json4s, JValue already extends scala.Product, so the implicit conversion JvalueToSuper (JValue => JvalueCaseClass) that the Lift-era ResourceDoc builder relied on never fires anymore — json.parse(...) now type-checks as a bare JValue without ever going through the wrapper. Whether exampleRequestBody ends up as a JvalueCaseClass or a raw JValue then depends on which other class initializes the formats/implicits first, making ConfirmationOfFundsServicePIISApiTest's .asInstanceOf[JvalueCaseClass] constructor-time cast fail intermittently (ClassCastException, suite aborts) depending on run ordering across shards. Wrap both example bodies in JvalueCaseClass explicitly at the ResourceDoc call site instead of relying on the dead implicit, and make the test's own body lookup pattern-match on both shapes so it no longer assumes one representation. --- .../api/berlin/group/v1_3/Http4sBGv13PIIS.scala | 17 +++++++++-------- .../ConfirmationOfFundsServicePIISApiTest.scala | 13 ++++++++++--- 2 files changed, 19 insertions(+), 11 deletions(-) diff --git a/obp-api/src/main/scala/code/api/berlin/group/v1_3/Http4sBGv13PIIS.scala b/obp-api/src/main/scala/code/api/berlin/group/v1_3/Http4sBGv13PIIS.scala index 9a9f1258ac..072f600706 100644 --- a/obp-api/src/main/scala/code/api/berlin/group/v1_3/Http4sBGv13PIIS.scala +++ b/obp-api/src/main/scala/code/api/berlin/group/v1_3/Http4sBGv13PIIS.scala @@ -22,7 +22,6 @@ import org.http4s._ import org.http4s.dsl.io._ import scala.collection.mutable.ArrayBuffer -import scala.language.implicitConversions object Http4sBGv13PIIS extends MdcLoggable { @@ -30,9 +29,11 @@ object Http4sBGv13PIIS extends MdcLoggable { implicit val formats: Formats = CustomJsonFormats.formats - // ResourceDoc example bodies are written as `json.parse(...)` (JValue); ResourceDoc requires - // scala.Product, so wrap via the same implicit the Lift builder used (JvalueCaseClass is a Product). - protected implicit def JvalueToSuper(what: org.json4s.JValue): JvalueCaseClass = JvalueCaseClass(what) + // ResourceDoc example bodies are written as `json.parse(...)` (JValue). Under lift-json the + // Lift builder relied on an implicit JValue => JvalueCaseClass because JValue was not a Product; + // json4s JValue already extends scala.Product, so that implicit never fires anymore. Wrap + // explicitly: resource-docs serialization special-cases JvalueCaseClass (strips the + // jvalueToCaseclass key, skips field reflection), and tests unwrap it to get the raw JValue. val implementedInApiVersion = ConstantsBG.berlinGroupVersion1 val resourceDocs = ArrayBuffer[ResourceDoc]() @@ -97,7 +98,7 @@ Creates a confirmation of funds request at the ASPSP. Checks whether a specific of time of the request on an account linked to a given tuple card issuer(TPP)/card number, or addressed by IBAN and TPP respectively. If the related extended services are used a conditional Consent-ID is contained in the header. This field is contained but commented out in this specification. """, - json.parse( + JvalueCaseClass(json.parse( """{ "instructedAmount" : { "amount" : "123", @@ -106,11 +107,11 @@ in the header. This field is contained but commented out in this specification. "account" : { "iban" : "GR12 1234 5123 4511 3981 4475 477", } - }"""), - json.parse( + }""")), + JvalueCaseClass(json.parse( """{ "fundsAvailable" : true - }"""), + }""")), List(AuthenticatedUserIsRequired, UnknownError), ApiTag("Confirmation of Funds Service (PIIS)") :: apiTagBerlinGroupM :: Nil, http4sPartialFunction = Some(checkAvailabilityOfFunds) diff --git a/obp-api/src/test/scala/code/api/berlin/group/v1_3/ConfirmationOfFundsServicePIISApiTest.scala b/obp-api/src/test/scala/code/api/berlin/group/v1_3/ConfirmationOfFundsServicePIISApiTest.scala index c0ea12b2c4..2d53562fcb 100644 --- a/obp-api/src/test/scala/code/api/berlin/group/v1_3/ConfirmationOfFundsServicePIISApiTest.scala +++ b/obp-api/src/test/scala/code/api/berlin/group/v1_3/ConfirmationOfFundsServicePIISApiTest.scala @@ -5,6 +5,7 @@ import code.api.berlin.group.v1_3.JSONFactory_BERLIN_GROUP_1_3.ErrorMessagesBG import com.openbankproject.commons.model.ErrorMessage import code.api.berlin.group.v1_3.{Http4sBGv13PIIS => APIMethods_ConfirmationOfFundsServicePIISApi} import code.api.util.APIUtil.OAuth._ +import code.api.util.CustomJsonFormats import code.api.util.ErrorMessages.{BankAccountNotFound, BankAccountNotFoundByIban, InvalidJsonContent, InvalidJsonFormat} import code.model.dataAccess.{BankAccountRouting, MappedBankAccount} import code.setup.{APIResponse, DefaultUsers} @@ -20,11 +21,17 @@ class ConfirmationOfFundsServicePIISApiTest extends BerlinGroupServerSetupV1_3 w object PIIS extends Tag("Confirmation of Funds Service (PIIS)") object checkAvailabilityOfFunds extends Tag(nameOf(APIMethods_ConfirmationOfFundsServicePIISApi.checkAvailabilityOfFunds)) - val checkAvailabilityOfFundsJsonBody = APIMethods_ConfirmationOfFundsServicePIISApi + // The example body is a JvalueCaseClass when the ResourceDoc wraps it explicitly, but a raw + // JValue when built via `json.parse(...)` alone (json4s JValue is already a scala.Product, so + // the old lift-json-era implicit wrapping never fires). Accept both shapes. + val checkAvailabilityOfFundsJsonBody: JValue = APIMethods_ConfirmationOfFundsServicePIISApi .resourceDocs .filter(_.partialFunctionName == "checkAvailabilityOfFunds") - .head.exampleRequestBody.asInstanceOf[JvalueCaseClass] //All the Json String convert to JvalueCaseClass implicitly - .jvalueToCaseclass + .head.exampleRequestBody match { + case JvalueCaseClass(jvalue) => jvalue + case jvalue: JValue => jvalue + case other => Extraction.decompose(other)(CustomJsonFormats.formats) + } feature(s"BG v1.3 - ${checkAvailabilityOfFunds.name}") { From 30afe2a46aa1c33d6206778453ac9fad1a5efaca Mon Sep 17 00:00:00 2001 From: hongwei Date: Thu, 2 Jul 2026 13:24:07 +0200 Subject: [PATCH 23/29] fix(berlin-group): wrap all v1.3 ResourceDoc example bodies in JvalueCaseClass MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Same root cause as the earlier PIIS-only fix, extended to the other three v1.3 files: since the json4s migration, JValue already extends scala.Product, so the implicit conversion JvalueToSuper (JValue => JvalueCaseClass) that ResourceDoc construction relied on never fires. Every example body built via json.parse(...) across AIS, PIS, and SigningBaskets was therefore a raw JValue instead of the expected JvalueCaseClass wrapper — resource-docs serialization would reflect on it as a generic case class rather than taking the JvalueCaseClass special-case path, and any call site that asInstanceOf[JvalueCaseClass]'d the example body (as ConfirmationOfFundsServicePIISApiTest's constructor did) failed depending on class-init order. Wrap every example body explicitly at each ResourceDoc call site instead of relying on the dead implicit; drop the now-unused implicitConversions import. --- .../berlin/group/v1_3/Http4sBGv13AIS.scala | 91 +++++++------- .../berlin/group/v1_3/Http4sBGv13PIIS.scala | 17 +-- .../berlin/group/v1_3/Http4sBGv13PIS.scala | 119 +++++++++--------- .../v1_3/Http4sBGv13SigningBaskets.scala | 37 +++--- 4 files changed, 137 insertions(+), 127 deletions(-) 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 026c07b293..711e63b4e9 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 @@ -36,7 +36,6 @@ import org.http4s.dsl.io._ import scala.collection.mutable.ArrayBuffer import scala.concurrent.Future -import scala.language.implicitConversions object Http4sBGv13AIS extends MdcLoggable { @@ -44,7 +43,11 @@ object Http4sBGv13AIS extends MdcLoggable { implicit val formats: Formats = CustomJsonFormats.formats - protected implicit def JvalueToSuper(what: org.json4s.JValue): JvalueCaseClass = JvalueCaseClass(what) + // ResourceDoc example bodies are written as `json.parse(...)` (JValue). Since the json4s + // migration, JValue itself extends scala.Product, so an implicit JValue => JvalueCaseClass + // conversion never fires. Each example body is therefore wrapped explicitly in + // JvalueCaseClass(...) so resource-docs serialization takes its special-case path (no field + // reflection; the jvalueToCaseclass wrapper key is stripped) instead of reflecting on a raw JObject. val implementedInApiVersion = ConstantsBG.berlinGroupVersion1 val resourceDocs = ArrayBuffer[ResourceDoc]() @@ -713,7 +716,7 @@ This is returning the data for the TPP especially in cases, where the consent was directly managed between ASPSP and PSU e.g. in a re-direct SCA Approach. """, EmptyBody, - json.parse("""{ + JvalueCaseClass(json.parse("""{ "access": { "accounts": [ { @@ -732,7 +735,7 @@ where the consent was directly managed between ASPSP and PSU e.g. in a re-direct "combinedServiceIndicator": false, "lastActionDate": "2019-06-30", "consentStatus": "received" - }"""), + }""")), List(AuthenticatedUserIsRequired, ConsentNotFound, UnknownError), ApiTag("Account Information Service (AIS)") :: apiTagBerlinGroupM :: Nil, authMode = UserOrApplication, @@ -748,9 +751,9 @@ where the consent was directly managed between ASPSP and PSU e.g. in a re-direct s"""${mockedDataText(false)} Read the status of an account information consent resource.""", EmptyBody, - json.parse("""{ + JvalueCaseClass(json.parse("""{ "consentStatus": "received" - }"""), + }""")), List(AuthenticatedUserIsRequired, UnknownError), ApiTag("Account Information Service (AIS)") :: apiTagBerlinGroupM :: Nil, authMode = UserOrApplication, @@ -787,7 +790,7 @@ using the extended forms as indicated above. """ - val startConsentAuthorisationResponse = json.parse("""{ + val startConsentAuthorisationResponse = JvalueCaseClass(json.parse("""{ "scaStatus": "received", "psuMessage": "Please use your BankApp for transaction Authorisation.", "authorisationId": "123auth456.", @@ -795,7 +798,7 @@ using the extended forms as indicated above. { "scaStatus": {"href":"/v1.3/consents/qwer3456tzui7890/authorisations/123auth456"} } - }""") + }""")) resourceDocs += ResourceDoc( implementedInApiVersion, @@ -804,7 +807,7 @@ using the extended forms as indicated above. "/consents/CONSENTID/authorisations", "Start the authorisation process for a consent(transactionAuthorisation)", generalStartConsentAuthorisationSummary, - json.parse("""{"scaAuthenticationData":""}"""), + JvalueCaseClass(json.parse("""{"scaAuthenticationData":""}""")), startConsentAuthorisationResponse, List(AuthenticatedUserIsRequired, UnknownError), ApiTag("Account Information Service (AIS)") :: apiTagBerlinGroupM :: Nil, @@ -818,7 +821,7 @@ using the extended forms as indicated above. "/consents/CONSENTID/authorisations", "Start the authorisation process for a consent(updatePsuAuthentication)", generalStartConsentAuthorisationSummary, - json.parse("""{"psuData": {"password": "start12"}}"""), + JvalueCaseClass(json.parse("""{"psuData": {"password": "start12"}}""")), startConsentAuthorisationResponse, List(AuthenticatedUserIsRequired, UnknownError), ApiTag("Account Information Service (AIS)") :: apiTagBerlinGroupM :: Nil, @@ -832,7 +835,7 @@ using the extended forms as indicated above. "/consents/CONSENTID/authorisations", "Start the authorisation process for a consent(selectPsuAuthenticationMethod)", generalStartConsentAuthorisationSummary, - json.parse("""{"authenticationMethodId":""}"""), + JvalueCaseClass(json.parse("""{"authenticationMethodId":""}""")), startConsentAuthorisationResponse, List(AuthenticatedUserIsRequired, UnknownError), ApiTag("Account Information Service (AIS)") :: apiTagBerlinGroupM :: Nil, @@ -875,7 +878,7 @@ Maybe in a later version the access path will change. "/consents/CONSENTID/authorisations/AUTHORISATIONID", "Update PSU Data for consents (transactionAuthorisation)", generalUpdateConsentsPsuDataSummary, - json.parse("""{"scaAuthenticationData":"123"}"""), + JvalueCaseClass(json.parse("""{"scaAuthenticationData":"123"}""")), ScaStatusResponse( scaStatus = "received", _links = Some(LinksAll(scaStatus = Some(HrefType(Some(s"/v1.3/consents/1234-wertiq-983/authorisations"))))) @@ -892,13 +895,13 @@ Maybe in a later version the access path will change. "/consents/CONSENTID/authorisations/AUTHORISATIONID", "Update PSU Data for consents (updatePsuAuthentication)", generalUpdateConsentsPsuDataSummary, - json.parse("""{"psuData": {"password": "start12"}}""".stripMargin), - json.parse("""{ + JvalueCaseClass(json.parse("""{"psuData": {"password": "start12"}}""".stripMargin)), + JvalueCaseClass(json.parse("""{ | "scaStatus": "psuAuthenticated", | "_links": { | "authoriseTransaction": {"href": "/psd2/v1/payments/1234-wertiq-983/authorisations/123auth456"} | } - | }""".stripMargin), + | }""".stripMargin)), List(AuthenticatedUserIsRequired, UnknownError), ApiTag("Account Information Service (AIS)") :: apiTagBerlinGroupM :: Nil, http4sPartialFunction = Some(updateConsentsPsuDataAll) @@ -911,10 +914,10 @@ Maybe in a later version the access path will change. "/consents/CONSENTID/authorisations/AUTHORISATIONID", "Update PSU Data for consents (selectPsuAuthenticationMethod)", generalUpdateConsentsPsuDataSummary, - json.parse("""{ + JvalueCaseClass(json.parse("""{ | "authenticationMethodId": "myAuthenticationID" - |}""".stripMargin), - json.parse("""{ + |}""".stripMargin)), + JvalueCaseClass(json.parse("""{ | "scaStatus": "scaMethodSelected", | "chosenScaMethod": { | "authenticationType": "SMS_OTP", @@ -925,7 +928,7 @@ Maybe in a later version the access path will change. | "_links": { | "authoriseTransaction": {"href": "/psd2/v1/payments/1234-wertiq-983/authorisations/123auth456"} | } - | }""".stripMargin), + | }""".stripMargin)), List(AuthenticatedUserIsRequired, UnknownError), ApiTag("Account Information Service (AIS)") :: apiTagBerlinGroupM :: Nil, http4sPartialFunction = Some(updateConsentsPsuDataAll) @@ -938,13 +941,13 @@ Maybe in a later version the access path will change. "/consents/CONSENTID/authorisations/AUTHORISATIONID", "Update PSU Data for consents (authorisationConfirmation)", generalUpdateConsentsPsuDataSummary, - json.parse("""{"confirmationCode":"confirmationCode"}"""), - json.parse("""{ + JvalueCaseClass(json.parse("""{"confirmationCode":"confirmationCode"}""")), + JvalueCaseClass(json.parse("""{ | "scaStatus": "finalised", | "_links":{ | "status": {"href":"/v1/payments/sepa-credit-transfers/qwer3456tzui7890/status"} | } - | }""".stripMargin), + | }""".stripMargin)), List(AuthenticatedUserIsRequired, UnknownError), ApiTag("Account Information Service (AIS)") :: apiTagBerlinGroupM :: Nil, http4sPartialFunction = Some(updateConsentsPsuDataAll) @@ -978,7 +981,7 @@ In this case, this endpoint will deliver the information about all available pay of the PSU at this ASPSP. """, EmptyBody, - json.parse("""{ + JvalueCaseClass(json.parse("""{ | "accounts": [ | { | "resourceId": "3dc3d5b3-7023-4848-9853-f5400a64e80f", @@ -994,7 +997,7 @@ of the PSU at this ASPSP. | } | } | ] - |}""".stripMargin), + |}""".stripMargin)), List(AuthenticatedUserIsRequired, UnknownError), ApiTag("Account Information Service (AIS)") :: apiTagBerlinGroupM :: Nil, http4sPartialFunction = Some(getAccountList) @@ -1016,7 +1019,7 @@ This account-id then can be retrieved by the "GET Account List" call. The account-id is constant at least throughout the lifecycle of a given consent. """, EmptyBody, - json.parse("""{ + JvalueCaseClass(json.parse("""{ "account":{ "iban":"DE91 1000 0000 0123 4567 89" }, @@ -1031,7 +1034,7 @@ The account-id is constant at least throughout the lifecycle of a given consent. "referenceDate":"2018-03-08" }] } -"""), +""")), List(AuthenticatedUserIsRequired, UnknownError), ApiTag("Account Information Service (AIS)") :: apiTagBerlinGroupM :: Nil, http4sPartialFunction = Some(getBalances) @@ -1050,7 +1053,7 @@ The addressed list of card accounts depends then on the PSU ID and the stored co respectively the OAuth2 access token. """, EmptyBody, - json.parse("""{ + JvalueCaseClass(json.parse("""{ "cardAccounts": [ { "resourceId": "3d9a81b3-a47d-4130-8765-a9c0ff861b99", @@ -1070,7 +1073,7 @@ respectively the OAuth2 access token. } } ] -}"""), +}""")), List(AuthenticatedUserIsRequired, UnknownError), ApiTag("Account Information Service (AIS)") :: apiTagMockedData :: Nil, http4sPartialFunction = Some(getCardAccounts) @@ -1093,7 +1096,7 @@ This account-id then can be retrieved by the "GET Card Account List" call """, EmptyBody, - json.parse("""{ + JvalueCaseClass(json.parse("""{ "cardAccount":{ "iban":"DE91 1000 0000 0123 4567 89" }, @@ -1107,7 +1110,7 @@ This account-id then can be retrieved by the "lastCommittedTransaction":"String", "referenceDate":"2018-03-08" }] -}"""), +}""")), List(AuthenticatedUserIsRequired, UnknownError), ApiTag("Account Information Service (AIS)") :: Nil, http4sPartialFunction = Some(getCardAccountBalances) @@ -1123,7 +1126,7 @@ This account-id then can be retrieved by the Reads account data from a given card account addressed by "account-id". """, EmptyBody, - json.parse("""{ + JvalueCaseClass(json.parse("""{ "cardAccount": { "maskedPan": "525412******3241" }, @@ -1135,7 +1138,7 @@ Reads account data from a given card account addressed by "account-id". } } } - }"""), + }""")), List(AuthenticatedUserIsRequired, UnknownError), ApiTag("Account Information Service (AIS)") :: apiTagBerlinGroupM :: Nil, http4sPartialFunction = Some(getCardAccountTransactionList) @@ -1153,9 +1156,9 @@ Return a list of all authorisation subresources IDs which have been created. This function returns an array of hyperlinks to all generated authorisation sub-resources. """, EmptyBody, - json.parse("""{ + JvalueCaseClass(json.parse("""{ "authorisationIds" : "faa3657e-13f0-4feb-a6c3-34bf21a9ae8e" -}"""), +}""")), List(AuthenticatedUserIsRequired, UnknownError), ApiTag("Account Information Service (AIS)") :: apiTagBerlinGroupM :: Nil, http4sPartialFunction = Some(getConsentAuthorisation) @@ -1171,9 +1174,9 @@ This function returns an array of hyperlinks to all generated authorisation sub- This method returns the SCA status of a consent initiation's authorisation sub-resource. """, EmptyBody, - json.parse("""{ + JvalueCaseClass(json.parse("""{ "scaStatus" : "started" -}"""), +}""")), List(AuthenticatedUserIsRequired, UnknownError), ApiTag("Account Information Service (AIS)") :: apiTagBerlinGroupM :: Nil, http4sPartialFunction = Some(getConsentScaStatus) @@ -1194,7 +1197,7 @@ of the "Read Transaction List" call within the _links subfield. """, EmptyBody, - json.parse("""{ + JvalueCaseClass(json.parse("""{ "description": "Example for transaction details", "value": { "transactionsDetails": { @@ -1208,7 +1211,7 @@ of the "Read Transaction List" call within the _links subfield. "valueDate": "2017-10-26" } } -}"""), +}""")), List(AuthenticatedUserIsRequired, UnknownError), ApiTag("Account Information Service (AIS)") :: Nil, http4sPartialFunction = Some(getTransactionDetails) @@ -1226,7 +1229,7 @@ depending on the steering parameter "bookingStatus" together with balances. For a given account, additional parameters are e.g. the attributes "dateFrom" and "dateTo". The ASPSP might add balance information, if transaction lists without balances are not supported. """, EmptyBody, - json.parse("""{ + JvalueCaseClass(json.parse("""{ "account": { "iban": "DE2310010010123456788" }, @@ -1238,7 +1241,7 @@ The ASPSP might add balance information, if transaction lists without balances a } } } - }"""), + }""")), List(AuthenticatedUserIsRequired, UnknownError), ApiTag("Account Information Service (AIS)") :: apiTagBerlinGroupM :: Nil, http4sPartialFunction = Some(getTransactionList) @@ -1260,7 +1263,7 @@ Give detailed information about the addressed account together with balance info """, EmptyBody, - json.parse("""{ + JvalueCaseClass(json.parse("""{ "account": { "resourceId": "3dc3d5b3-7023-4848-9853-f5400a64e80f", "iban": "FR7612345987650123456789014", @@ -1274,7 +1277,7 @@ Give detailed information about the addressed account together with balance info } } } -}"""), +}""")), List(AuthenticatedUserIsRequired, UnknownError), ApiTag("Account Information Service (AIS)") :: apiTagBerlinGroupM :: Nil, http4sPartialFunction = Some(getAccountDetails) @@ -1293,7 +1296,7 @@ The addressed details of this account depends then on the stored consent address respectively the OAuth2 access token. """, EmptyBody, - json.parse("""{ + JvalueCaseClass(json.parse("""{ | "cardAccount": { | "resourceId": "3d9a81b3-a47d-4130-8765-a9c0ff861b99", | "maskedPan": "525412******3241", @@ -1302,7 +1305,7 @@ respectively the OAuth2 access token. | "product": "Basic Credit", | "status": "enabled" | } - |}""".stripMargin), + |}""".stripMargin)), List(AuthenticatedUserIsRequired, UnknownError), ApiTag("Account Information Service (AIS)") :: Nil, http4sPartialFunction = Some(readCardAccount) diff --git a/obp-api/src/main/scala/code/api/berlin/group/v1_3/Http4sBGv13PIIS.scala b/obp-api/src/main/scala/code/api/berlin/group/v1_3/Http4sBGv13PIIS.scala index 9a9f1258ac..e846c13c25 100644 --- a/obp-api/src/main/scala/code/api/berlin/group/v1_3/Http4sBGv13PIIS.scala +++ b/obp-api/src/main/scala/code/api/berlin/group/v1_3/Http4sBGv13PIIS.scala @@ -22,7 +22,6 @@ import org.http4s._ import org.http4s.dsl.io._ import scala.collection.mutable.ArrayBuffer -import scala.language.implicitConversions object Http4sBGv13PIIS extends MdcLoggable { @@ -30,9 +29,11 @@ object Http4sBGv13PIIS extends MdcLoggable { implicit val formats: Formats = CustomJsonFormats.formats - // ResourceDoc example bodies are written as `json.parse(...)` (JValue); ResourceDoc requires - // scala.Product, so wrap via the same implicit the Lift builder used (JvalueCaseClass is a Product). - protected implicit def JvalueToSuper(what: org.json4s.JValue): JvalueCaseClass = JvalueCaseClass(what) + // ResourceDoc example bodies are written as `json.parse(...)` (JValue). Since the json4s + // migration, JValue itself extends scala.Product, so an implicit JValue => JvalueCaseClass + // conversion never fires. Each example body is therefore wrapped explicitly in + // JvalueCaseClass(...) so resource-docs serialization takes its special-case path (no field + // reflection; the jvalueToCaseclass wrapper key is stripped) instead of reflecting on a raw JObject. val implementedInApiVersion = ConstantsBG.berlinGroupVersion1 val resourceDocs = ArrayBuffer[ResourceDoc]() @@ -97,7 +98,7 @@ Creates a confirmation of funds request at the ASPSP. Checks whether a specific of time of the request on an account linked to a given tuple card issuer(TPP)/card number, or addressed by IBAN and TPP respectively. If the related extended services are used a conditional Consent-ID is contained in the header. This field is contained but commented out in this specification. """, - json.parse( + JvalueCaseClass(json.parse( """{ "instructedAmount" : { "amount" : "123", @@ -106,11 +107,11 @@ in the header. This field is contained but commented out in this specification. "account" : { "iban" : "GR12 1234 5123 4511 3981 4475 477", } - }"""), - json.parse( + }""")), + JvalueCaseClass(json.parse( """{ "fundsAvailable" : true - }"""), + }""")), List(AuthenticatedUserIsRequired, UnknownError), ApiTag("Confirmation of Funds Service (PIIS)") :: apiTagBerlinGroupM :: Nil, http4sPartialFunction = Some(checkAvailabilityOfFunds) 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 b0300f0bc2..a85d4ec02a 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 @@ -34,7 +34,6 @@ import org.http4s.dsl.io._ import scala.collection.mutable.ArrayBuffer import scala.concurrent.Future -import scala.language.implicitConversions /** * Native http4s aggregator for Berlin Group v1.3 – Payment Initiation Service (PIS). @@ -47,7 +46,11 @@ object Http4sBGv13PIS extends MdcLoggable { implicit val formats: Formats = CustomJsonFormats.formats - protected implicit def JvalueToSuper(what: org.json4s.JValue): JvalueCaseClass = JvalueCaseClass(what) + // ResourceDoc example bodies are written as `json.parse(...)` (JValue). Since the json4s + // migration, JValue itself extends scala.Product, so an implicit JValue => JvalueCaseClass + // conversion never fires. Each example body is therefore wrapped explicitly in + // JvalueCaseClass(...) so resource-docs serialization takes its special-case path (no field + // reflection; the jvalueToCaseclass wrapper key is stripped) instead of reflecting on a raw JObject. val implementedInApiVersion = ConstantsBG.berlinGroupVersion1 val resourceDocs = ArrayBuffer[ResourceDoc]() @@ -735,7 +738,7 @@ The start authorisation process is a process which is needed for creating a new or cancellation sub-resource. """ - private val startPaymentAuthorisationResponse = json.parse("""{ + private val startPaymentAuthorisationResponse = JvalueCaseClass(json.parse("""{ "challengeData": { "scaStatus": "received", "authorisationId": "88695566-6642-46d5-9985-0d824624f507", @@ -744,7 +747,7 @@ or cancellation sub-resource. "scaStatus": "/v1.3/payments/sepa-credit-transfers/88695566-6642-46d5-9985-0d824624f507" } } - }""") + }""")) private val generalStartPaymentInitiationCancellationAuthorisationSummary: String = s"""${mockedDataText(true)} @@ -752,7 +755,7 @@ Creates an authorisation sub-resource and start the authorisation process of the The message might in addition transmit authentication and authorisation related data. """ - private val startPaymentInitiationCancellationAuthorisationResponse = json.parse("""{ + private val startPaymentInitiationCancellationAuthorisationResponse = JvalueCaseClass(json.parse("""{ "scaStatus": "received", "authorisationId": "123auth456", "psuMessage": "Please use your BankApp for transaction Authorisation.", @@ -761,7 +764,7 @@ The message might in addition transmit authentication and authorisation related "href": "/v1.3/payments/qwer3456tzui7890/authorisations/123auth456" } } - }""") + }""")) private val generalUpdatePaymentCancellationPsuDataSummary: String = s"""${mockedDataText(true)} @@ -815,7 +818,7 @@ or * access method is generally applicable, but further authorisation processes This method returns the SCA status of a payment initiation's authorisation sub-resource. """, EmptyBody, - json.parse("""{"scaStatus" : "psuAuthenticated"}"""), + JvalueCaseClass(json.parse("""{"scaStatus" : "psuAuthenticated"}""")), List(AuthenticatedUserIsRequired, UnknownError), ApiTag("Payment Initiation Service (PIS)") :: apiTagBerlinGroupM :: Nil, http4sPartialFunction = Some(getPaymentCancellationScaStatus) @@ -828,7 +831,7 @@ This method returns the SCA status of a payment initiation's authorisation sub-r s"""${mockedDataText(false)} Returns the content of a payment object""", EmptyBody, - json.parse("""{ + JvalueCaseClass(json.parse("""{ "debtorAccount":{ "iban":"GR12 1234 5123 4511 3981 4475 477" }, @@ -840,7 +843,7 @@ Returns the content of a payment object""", "iban":"GR12 1234 5123 4514 4575 3645 077" }, "creditorName":"70charname" - }"""), + }""")), List(AuthenticatedUserIsRequired, UnknownError), ApiTag("Payment Initiation Service (PIS)") :: apiTagBerlinGroupM :: Nil, http4sPartialFunction = Some(getPaymentInformation) @@ -856,12 +859,12 @@ Read a list of all authorisation subresources IDs which have been created. This function returns an array of hyperlinks to all generated authorisation sub-resources. """, EmptyBody, - json.parse("""[{ + JvalueCaseClass(json.parse("""[{ "scaStatus": "received", "authorisationId": "940948c7-1c86-4d88-977e-e739bf2c1492", "psuMessage": "Please check your SMS at a mobile device.", "_links": {"scaStatus": "/v1.3/payments/sepa-credit-transfers/940948c7-1c86-4d88-977e-e739bf2c1492"} - }]"""), + }]""")), List(AuthenticatedUserIsRequired, UnknownError), ApiTag("Payment Initiation Service (PIS)") :: apiTagBerlinGroupM :: Nil, http4sPartialFunction = Some(getPaymentInitiationAuthorisation) @@ -875,7 +878,7 @@ This function returns an array of hyperlinks to all generated authorisation sub- Retrieve a list of all created cancellation authorisation sub-resources. """, EmptyBody, - json.parse("""{"cancellationIds" : ["faa3657e-13f0-4feb-a6c3-34bf21a9ae8e"]}"""), + JvalueCaseClass(json.parse("""{"cancellationIds" : ["faa3657e-13f0-4feb-a6c3-34bf21a9ae8e"]}""")), List(AuthenticatedUserIsRequired, UnknownError), ApiTag("Payment Initiation Service (PIS)") :: apiTagBerlinGroupM :: Nil, http4sPartialFunction = Some(getPaymentInitiationCancellationAuthorisationInformation) @@ -889,7 +892,7 @@ Retrieve a list of all created cancellation authorisation sub-resources. This method returns the SCA status of a payment initiation's authorisation sub-resource. """, EmptyBody, - json.parse("""{"scaStatus" : "psuAuthenticated"}"""), + JvalueCaseClass(json.parse("""{"scaStatus" : "psuAuthenticated"}""")), List(AuthenticatedUserIsRequired, UnknownError), ApiTag("Payment Initiation Service (PIS)") :: apiTagBerlinGroupM :: Nil, http4sPartialFunction = Some(getPaymentInitiationScaStatus) @@ -902,7 +905,7 @@ This method returns the SCA status of a payment initiation's authorisation sub-r s"""${mockedDataText(false)} Check the transaction status of a payment initiation.""", EmptyBody, - json.parse(s"""{"transactionStatus": "${TransactionStatus.ACCP.code}"}"""), + JvalueCaseClass(json.parse(s"""{"transactionStatus": "${TransactionStatus.ACCP.code}"}""")), List(AuthenticatedUserIsRequired, UnknownError), ApiTag("Payment Initiation Service (PIS)") :: apiTagBerlinGroupM :: Nil, http4sPartialFunction = Some(getPaymentInitiationStatus) @@ -910,13 +913,13 @@ Check the transaction status of a payment initiation.""", } private def initInitiatePaymentResourceDocs(): Unit = { - val initiatePaymentRequestBody = json.parse(s"""{ + val initiatePaymentRequestBody = JvalueCaseClass(json.parse(s"""{ "debtorAccount": {"iban": "DE123456987480123"}, "instructedAmount": {"currency": "EUR", "amount": "100"}, "creditorAccount": {"iban": "UK12 1234 5123 4517 2948 6166 077"}, "creditorName": "70charname" - }""") - val initiatePaymentResponseBody = json.parse(s"""{ + }""")) + val initiatePaymentResponseBody = JvalueCaseClass(json.parse(s"""{ "transactionStatus": "${TransactionStatus.RCVD.code}", "paymentId": "1234-wertiq-983", "_links": { @@ -925,7 +928,7 @@ Check the transaction status of a payment initiation.""", "status": {"href": "/v1.3/payments/1234-wertiq-983/status"}, "scaStatus": {"href": "/v1.3/payments/1234-wertiq-983/authorisations/123auth456"} } - }""") + }""")) resourceDocs += ResourceDoc( implementedInApiVersion, nameOf(initiatePayments), @@ -947,7 +950,7 @@ $generalPaymentSummaryText""", "Payment initiation request(periodic-payments)", s"""${mockedDataText(false)} $generalPaymentSummaryText""", - json.parse(s"""{ + JvalueCaseClass(json.parse(s"""{ "instructedAmount": {"currency": "EUR", "amount": "123"}, "debtorAccount": {"iban": "DE40100100103307118608"}, "creditorName": "Merchant123", @@ -957,8 +960,8 @@ $generalPaymentSummaryText""", "executionRule": "preceding", "frequency": "Monthly", "dayOfExecution": "01" - }"""), - json.parse(s"""{ + }""")), + JvalueCaseClass(json.parse(s"""{ "transactionStatus": "${TransactionStatus.RCVD.code}", "paymentId": "1234-wertiq-983", "_links": { @@ -967,7 +970,7 @@ $generalPaymentSummaryText""", "status": {"href": "/v1.3/periodic-payments/1234-wertiq-983/status"}, "scaStatus": {"href": "/v1.3/periodic-payments/1234-wertiq-983/authorisations/123auth456"} } - }"""), + }""")), List(AuthenticatedUserIsRequired, UnknownError), ApiTag("Payment Initiation Service (PIS)") :: apiTagBerlinGroupM :: Nil, authMode = UserOrApplication, @@ -980,7 +983,7 @@ $generalPaymentSummaryText""", "Payment initiation request(bulk-payments)", s"""${mockedDataText(true)} $generalPaymentSummaryText""", - json.parse(s"""{ + JvalueCaseClass(json.parse(s"""{ "batchBookingPreferred": "true", "debtorAccount": {"iban": "DE40100100103307118608"}, "paymentInformationId": "my-bulk-identification-1234", @@ -993,8 +996,8 @@ $generalPaymentSummaryText""", "creditorAccount": {"iban": "FR7612345987650123456789014"}, "remittanceInformationUnstructured": "Ref Number Merchant 2"} ] - }"""), - json.parse(s"""{ + }""")), + JvalueCaseClass(json.parse(s"""{ "transactionStatus": "${TransactionStatus.RCVD.code}", "paymentId": "1234-wertiq-983", "_links": { @@ -1003,7 +1006,7 @@ $generalPaymentSummaryText""", "status": {"href": "/v1.3/bulk-payments/1234-wertiq-983/status"}, "scaStatus": {"href": "/v1.3/bulk-payments/1234-wertiq-983/authorisations/123auth456"} } - }"""), + }""")), List(AuthenticatedUserIsRequired, UnknownError), ApiTag("Payment Initiation Service (PIS)") :: apiTagBerlinGroupM :: Nil, authMode = UserOrApplication, @@ -1019,7 +1022,7 @@ $generalPaymentSummaryText""", "POST", "/PAYMENT_SERVICE/PAYMENT_PRODUCT/PAYMENT_ID/authorisations", "Start the authorisation process for a payment initiation (updatePsuAuthentication)", generalStartPaymentAuthorisationSummary, - json.parse("""{"psuData": {"password": "start12"}}"""), + JvalueCaseClass(json.parse("""{"psuData": {"password": "start12"}}""")), startPaymentAuthorisationResponse, List(AuthenticatedUserIsRequired, UnknownError), ApiTag("Payment Initiation Service (PIS)") :: apiTagBerlinGroupM :: Nil, @@ -1031,7 +1034,7 @@ $generalPaymentSummaryText""", "POST", "/PAYMENT_SERVICE/PAYMENT_PRODUCT/PAYMENT_ID/authorisations", "Start the authorisation process for a payment initiation (selectPsuAuthenticationMethod)", generalStartPaymentAuthorisationSummary, - json.parse("""{"authenticationMethodId":""}"""), + JvalueCaseClass(json.parse("""{"authenticationMethodId":""}""")), startPaymentAuthorisationResponse, List(AuthenticatedUserIsRequired, UnknownError), ApiTag("Payment Initiation Service (PIS)") :: apiTagBerlinGroupM :: Nil, @@ -1046,7 +1049,7 @@ $generalPaymentSummaryText""", Create an authorisation sub-resource and start the authorisation process. The message might in addition transmit authentication and authorisation related data. """, - json.parse("""{"scaAuthenticationData":"123"}"""), + JvalueCaseClass(json.parse("""{"scaAuthenticationData":"123"}""")), startPaymentAuthorisationResponse, List(AuthenticatedUserIsRequired, UnknownError), ApiTag("Payment Initiation Service (PIS)") :: apiTagBerlinGroupM :: Nil, @@ -1062,13 +1065,13 @@ The message might in addition transmit authentication and authorisation related s"""${mockedDataText(false)} Creates an authorisation sub-resource and start the authorisation process of the cancellation of the addressed payment. """, - json.parse("""{"scaAuthenticationData":""}"""), - json.parse("""{ + JvalueCaseClass(json.parse("""{"scaAuthenticationData":""}""")), + JvalueCaseClass(json.parse("""{ "scaStatus": "received", "authorisationId": "123auth456", "psuMessage": "Please use your BankApp for transaction Authorisation.", "_links": {"scaStatus": {"href": "/v1.3/payments/qwer3456tzui7890/authorisations/123auth456"}} - }"""), + }""")), List(AuthenticatedUserIsRequired, UnknownError), ApiTag("Payment Initiation Service (PIS)") :: apiTagBerlinGroupM :: Nil, http4sPartialFunction = Some(startPaymentInitiationCancellationAuthorisationAll) @@ -1079,7 +1082,7 @@ Creates an authorisation sub-resource and start the authorisation process of the "POST", "/PAYMENT_SERVICE/PAYMENT_PRODUCT/PAYMENT_ID/cancellation-authorisations", "Start the authorisation process for the cancellation of the addressed payment (updatePsuAuthentication)", generalStartPaymentInitiationCancellationAuthorisationSummary, - json.parse("""{"psuData": {"password": "start12"}}"""), + JvalueCaseClass(json.parse("""{"psuData": {"password": "start12"}}""")), startPaymentInitiationCancellationAuthorisationResponse, List(AuthenticatedUserIsRequired, UnknownError), ApiTag("Payment Initiation Service (PIS)") :: apiTagBerlinGroupM :: Nil, @@ -1091,7 +1094,7 @@ Creates an authorisation sub-resource and start the authorisation process of the "POST", "/PAYMENT_SERVICE/PAYMENT_PRODUCT/PAYMENT_ID/cancellation-authorisations", "Start the authorisation process for the cancellation of the addressed payment (selectPsuAuthenticationMethod)", generalStartPaymentInitiationCancellationAuthorisationSummary, - json.parse("""{"authenticationMethodId":""}"""), + JvalueCaseClass(json.parse("""{"authenticationMethodId":""}""")), startPaymentInitiationCancellationAuthorisationResponse, List(AuthenticatedUserIsRequired, UnknownError), ApiTag("Payment Initiation Service (PIS)") :: apiTagBerlinGroupM :: Nil, @@ -1109,12 +1112,12 @@ Creates an authorisation sub-resource and start the authorisation process of the s"""${mockedDataText(false)} This method updates PSU data on the cancellation authorisation resource if needed. """, - json.parse("""{"scaAuthenticationData":"123"}"""), - json.parse("""{ + JvalueCaseClass(json.parse("""{"scaAuthenticationData":"123"}""")), + JvalueCaseClass(json.parse("""{ "scaStatus":"finalised", "psuMessage":"Please check your SMS at a mobile device.", "_links":{"scaStatus":"/v1.3/payments/sepa-credit-transfers/PAYMENT_ID/4f4a8b7f-9968-4183-92ab-ca512b396bfc"} - }"""), + }""")), List(AuthenticatedUserIsRequired, UnknownError), ApiTag("Payment Initiation Service (PIS)") :: apiTagBerlinGroupM :: Nil, http4sPartialFunction = Some(updatePaymentCancellationPsuDataAll) @@ -1125,11 +1128,11 @@ This method updates PSU data on the cancellation authorisation resource if neede "PUT", "/PAYMENT_SERVICE/PAYMENT_PRODUCT/PAYMENT_ID/cancellation-authorisations/AUTHORISATION_ID", "Update PSU Data for payment initiation cancellation (updatePsuAuthentication)", generalUpdatePaymentCancellationPsuDataSummary, - json.parse("""{"psuData":{"password":"start12"}}"""), - json.parse("""{ + JvalueCaseClass(json.parse("""{"psuData":{"password":"start12"}}""")), + JvalueCaseClass(json.parse("""{ "scaStatus": "psuAuthenticated", "_links": {"authoriseTransaction": {"href": "/psd2/v1.3/payments/1234-wertiq-983/authorisations/123auth456"}} - }"""), + }""")), List(AuthenticatedUserIsRequired, UnknownError), ApiTag("Payment Initiation Service (PIS)") :: apiTagBerlinGroupM :: Nil, http4sPartialFunction = Some(updatePaymentCancellationPsuDataAll) @@ -1140,13 +1143,13 @@ This method updates PSU data on the cancellation authorisation resource if neede "PUT", "/PAYMENT_SERVICE/PAYMENT_PRODUCT/PAYMENT_ID/cancellation-authorisations/AUTHORISATION_ID", "Update PSU Data for payment initiation cancellation (selectPsuAuthenticationMethod)", generalUpdatePaymentCancellationPsuDataSummary, - json.parse("""{"authenticationMethodId":""}"""), - json.parse("""{ + JvalueCaseClass(json.parse("""{"authenticationMethodId":""}""")), + JvalueCaseClass(json.parse("""{ "scaStatus": "scaMethodSelected", "chosenScaMethod": {"authenticationType": "SMS_OTP", "authenticationMethodId": "myAuthenticationID"}, "challengeData": {"otpMaxLength": 6, "otpFormat": "integer"}, "_links": {"authoriseTransaction": {"href": "/psd2/v1.3/payments/1234-wertiq-983/authorisations/123auth456"}} - }"""), + }""")), List(AuthenticatedUserIsRequired, UnknownError), ApiTag("Payment Initiation Service (PIS)") :: apiTagBerlinGroupM :: Nil, http4sPartialFunction = Some(updatePaymentCancellationPsuDataAll) @@ -1157,11 +1160,11 @@ This method updates PSU data on the cancellation authorisation resource if neede "PUT", "/PAYMENT_SERVICE/PAYMENT_PRODUCT/PAYMENT_ID/cancellation-authorisations/AUTHORISATION_ID", "Update PSU Data for payment initiation cancellation (authorisationConfirmation)", generalUpdatePaymentCancellationPsuDataSummary, - json.parse("""{"confirmationCode":"confirmationCode"}"""), - json.parse("""{ + JvalueCaseClass(json.parse("""{"confirmationCode":"confirmationCode"}""")), + JvalueCaseClass(json.parse("""{ "scaStatus": "finalised", "_links":{"status": {"href":"/v1.3/payments/sepa-credit-transfers/qwer3456tzui7890/status"}} - }"""), + }""")), List(AuthenticatedUserIsRequired, UnknownError), ApiTag("Payment Initiation Service (PIS)") :: apiTagBerlinGroupM :: Nil, http4sPartialFunction = Some(updatePaymentCancellationPsuDataAll) @@ -1174,12 +1177,12 @@ This method updates PSU data on the cancellation authorisation resource if neede "PUT", "/PAYMENT_SERVICE/PAYMENT_PRODUCT/PAYMENT_ID/authorisations/AUTHORISATION_ID", "Update PSU data for payment initiation (transactionAuthorisation)", generalUpdatePaymentPsuDataSummary, - json.parse("""{"scaAuthenticationData":"123"}"""), - json.parse("""{ + JvalueCaseClass(json.parse("""{"scaAuthenticationData":"123"}""")), + JvalueCaseClass(json.parse("""{ "scaStatus": "finalised", "psuMessage": "Please check your SMS at a mobile device.", "_links": {"scaStatus": {"href":"/v1.3/payments/sepa-credit-transfers/88695566-6642-46d5-9985-0d824624f507"}} - }"""), + }""")), List(AuthenticatedUserIsRequired, UnknownError), ApiTag("Payment Initiation Service (PIS)") :: apiTagBerlinGroupM :: Nil, http4sPartialFunction = Some(updatePaymentPsuDataAll) @@ -1190,11 +1193,11 @@ This method updates PSU data on the cancellation authorisation resource if neede "PUT", "/PAYMENT_SERVICE/PAYMENT_PRODUCT/PAYMENT_ID/authorisations/AUTHORISATION_ID", "Update PSU data for payment initiation (updatePsuAuthentication)", generalUpdatePaymentPsuDataSummary, - json.parse("""{"psuData": {"password": "start12"}}"""), - json.parse("""{ + JvalueCaseClass(json.parse("""{"psuData": {"password": "start12"}}""")), + JvalueCaseClass(json.parse("""{ "scaStatus": "finalised", "_links": {"scaStatus": {"href":"/v1.3/payments/sepa-credit-transfers/88695566-6642-46d5-9985-0d824624f507"}} - }"""), + }""")), List(AuthenticatedUserIsRequired, UnknownError), ApiTag("Payment Initiation Service (PIS)") :: apiTagBerlinGroupM :: Nil, http4sPartialFunction = Some(updatePaymentPsuDataAll) @@ -1205,13 +1208,13 @@ This method updates PSU data on the cancellation authorisation resource if neede "PUT", "/PAYMENT_SERVICE/PAYMENT_PRODUCT/PAYMENT_ID/authorisations/AUTHORISATION_ID", "Update PSU data for payment initiation (selectPsuAuthenticationMethod)", generalUpdatePaymentPsuDataSummary, - json.parse("""{"authenticationMethodId":""}"""), - json.parse("""{ + JvalueCaseClass(json.parse("""{"authenticationMethodId":""}""")), + JvalueCaseClass(json.parse("""{ "scaStatus": "scaMethodSelected", "chosenScaMethod": {"authenticationType": "SMS_OTP", "authenticationMethodId": "myAuthenticationID"}, "challengeData": {"otpMaxLength": 6, "otpFormat": "integer"}, "_links": {"authoriseTransaction": {"href": "/psd2/v1.3/payments/1234-wertiq-983/authorisations/123auth456"}} - }"""), + }""")), List(AuthenticatedUserIsRequired, UnknownError), ApiTag("Payment Initiation Service (PIS)") :: apiTagBerlinGroupM :: Nil, http4sPartialFunction = Some(updatePaymentPsuDataAll) @@ -1222,11 +1225,11 @@ This method updates PSU data on the cancellation authorisation resource if neede "PUT", "/PAYMENT_SERVICE/PAYMENT_PRODUCT/PAYMENT_ID/authorisations/AUTHORISATION_ID", "Update PSU data for payment initiation (authorisationConfirmation)", generalUpdatePaymentPsuDataSummary, - json.parse("""{"confirmationCode":"confirmationCode"}"""), - json.parse("""{ + JvalueCaseClass(json.parse("""{"confirmationCode":"confirmationCode"}""")), + JvalueCaseClass(json.parse("""{ "scaStatus": "finalised", "_links":{"status": {"href":"/v1.3/payments/sepa-credit-transfers/qwer3456tzui7890/status"}} - }"""), + }""")), List(AuthenticatedUserIsRequired, UnknownError), ApiTag("Payment Initiation Service (PIS)") :: apiTagBerlinGroupM :: Nil, http4sPartialFunction = Some(updatePaymentPsuDataAll) diff --git a/obp-api/src/main/scala/code/api/berlin/group/v1_3/Http4sBGv13SigningBaskets.scala b/obp-api/src/main/scala/code/api/berlin/group/v1_3/Http4sBGv13SigningBaskets.scala index 11028b61bc..a876fc9a1a 100644 --- a/obp-api/src/main/scala/code/api/berlin/group/v1_3/Http4sBGv13SigningBaskets.scala +++ b/obp-api/src/main/scala/code/api/berlin/group/v1_3/Http4sBGv13SigningBaskets.scala @@ -28,7 +28,6 @@ import org.http4s.dsl.io._ import scala.collection.mutable.ArrayBuffer import scala.concurrent.Future -import scala.language.implicitConversions object Http4sBGv13SigningBaskets extends MdcLoggable { @@ -36,7 +35,11 @@ object Http4sBGv13SigningBaskets extends MdcLoggable { implicit val formats: Formats = CustomJsonFormats.formats - protected implicit def JvalueToSuper(what: org.json4s.JValue): JvalueCaseClass = JvalueCaseClass(what) + // ResourceDoc example bodies are written as `json.parse(...)` (JValue). Since the json4s + // migration, JValue itself extends scala.Product, so an implicit JValue => JvalueCaseClass + // conversion never fires. Each example body is therefore wrapped explicitly in + // JvalueCaseClass(...) so resource-docs serialization takes its special-case path (no field + // reflection; the jvalueToCaseclass wrapper key is stripped) instead of reflecting on a raw JObject. val implementedInApiVersion = ConstantsBG.berlinGroupVersion1 val resourceDocs = ArrayBuffer[ResourceDoc]() @@ -81,7 +84,7 @@ Create a signing basket resource for authorising several transactions with one S The resource identifications of these transactions are contained in the payload of this access method """, PostSigningBasketJsonV13(paymentIds = Some(List("123qwert456789", "12345qwert7899")), None), - json.parse("""{ + JvalueCaseClass(json.parse("""{ "basketId" : "1234-basket-567", "challengeData" : { "otpMaxLength" : 0, @@ -106,7 +109,7 @@ The resource identifications of these transactions are contained in the payload "chosenScaMethod" : "", "transactionStatus" : "ACCP", "psuMessage" : { } -}"""), +}""")), List(AuthenticatedUserIsRequired, UnknownError), apiTagSigningBaskets :: Nil, http4sPartialFunction = Some(createSigningBasket) @@ -171,11 +174,11 @@ Nevertheless, single transactions might be cancelled on an individual basis on t s"""${mockedDataText(false)} Returns the content of an signing basket object.""", EmptyBody, - json.parse("""{ + JvalueCaseClass(json.parse("""{ "transactionStatus" : "ACCP", "payments" : "", "consents" : "" -}"""), +}""")), List(AuthenticatedUserIsRequired, UnknownError), apiTagSigningBaskets :: Nil, http4sPartialFunction = Some(getSigningBasket) @@ -207,9 +210,9 @@ Read a list of all authorisation subresources IDs which have been created. This function returns an array of hyperlinks to all generated authorisation sub-resources. """, EmptyBody, - json.parse("""{ + JvalueCaseClass(json.parse("""{ "authorisationIds" : "" -}"""), +}""")), List(AuthenticatedUserIsRequired, UnknownError), apiTagSigningBaskets :: Nil, http4sPartialFunction = Some(getSigningBasketAuthorisation) @@ -243,9 +246,9 @@ This function returns an array of hyperlinks to all generated authorisation sub- This method returns the SCA status of a signing basket's authorisation sub-resource. """, EmptyBody, - json.parse("""{ + JvalueCaseClass(json.parse("""{ "scaStatus" : "psuAuthenticated" -}"""), +}""")), List(AuthenticatedUserIsRequired, UnknownError), apiTagSigningBaskets :: Nil, http4sPartialFunction = Some(getSigningBasketScaStatus) @@ -277,9 +280,9 @@ This method returns the SCA status of a signing basket's authorisation sub-resou Returns the status of a signing basket object. """, EmptyBody, - json.parse("""{ + JvalueCaseClass(json.parse("""{ "transactionStatus" : "RCVD" -}"""), +}""")), List(AuthenticatedUserIsRequired, UnknownError), apiTagSigningBaskets :: Nil, http4sPartialFunction = Some(getSigningBasketStatus) @@ -354,7 +357,7 @@ This applies in the following scenarios: * The signing basket needs to be authorised yet. """, EmptyBody, - json.parse("""{ + JvalueCaseClass(json.parse("""{ "challengeData" : { "otpMaxLength" : 0, "additionalInformation" : "additionalInformation", @@ -377,7 +380,7 @@ This applies in the following scenarios: }, "chosenScaMethod" : "", "psuMessage" : { } -}"""), +}""")), List(AuthenticatedUserIsRequired, UnknownError), apiTagSigningBaskets :: Nil, http4sPartialFunction = Some(startSigningBasketAuthorisation) @@ -502,15 +505,15 @@ There are the following request types on this access path: therefore many optional elements are not present. Maybe in a later version the access path will change. """, - json.parse("""{"scaAuthenticationData":"123"}"""), - json.parse("""{ + JvalueCaseClass(json.parse("""{"scaAuthenticationData":"123"}""")), + JvalueCaseClass(json.parse("""{ "scaStatus":"finalised", "authorisationId":"4f4a8b7f-9968-4183-92ab-ca512b396bfc", "psuMessage":"Please check your SMS at a mobile device.", "_links":{ "scaStatus":"/v1.3/payments/sepa-credit-transfers/PAYMENT_ID/4f4a8b7f-9968-4183-92ab-ca512b396bfc" } - }"""), + }""")), List(AuthenticatedUserIsRequired, UnknownError), apiTagSigningBaskets :: Nil, http4sPartialFunction = Some(updateSigningBasketPsuData) From e812d3a859db2b3b73c3185a1918758e522a650b Mon Sep 17 00:00:00 2001 From: hongwei Date: Thu, 2 Jul 2026 14:08:09 +0200 Subject: [PATCH 24/29] fix(test-runner): close three local-vs-CI coverage gaps MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The local parallel runner could pass while CI failed (or mask what CI tests): - Run -pl obp-commons,obp-api like CI does. The shards previously ran obp-api only, so obp-commons' five util suites never executed locally; the shard-4 catch-all even added com.openbankproject.commons.* to its filter, where it matched nothing and -DfailIfNoTests=false turned that into a silent pass. - Inject OBP_DYNAMIC_CODE_SANDBOX_PERMISSIONS, mirroring the dynamic_code_sandbox_permissions line CI appends to test.default.props. Its absence was the actual cause of the three perennial local DynamicResourceDocTest failures (sandbox denied reflection/getenv in the native-execution scenarios) — not a machine quirk. - Harden the surefire audit: scan both modules' reports, count unparseable XMLs as failures instead of skipping them, report skipped/canceled tests, and fail when fewer than 2000 tests ran in total (a broken wildcardSuites filter otherwise runs nothing and reports a hollow green thanks to -DfailIfNoTests=false). First fully green local run after this: 2917 tests, 0 failures, 0 errors. --- run_tests_parallel.sh | 36 +++++++++++++++++++++++++++--------- 1 file changed, 27 insertions(+), 9 deletions(-) diff --git a/run_tests_parallel.sh b/run_tests_parallel.sh index b61f0e2011..b35461cebd 100755 --- a/run_tests_parallel.sh +++ b/run_tests_parallel.sh @@ -166,6 +166,12 @@ run_shard() { # mail.test.mode (CI has it); without it, flows like consent actually open an # SMTP socket -> 500 (CI green, local red). We inject OBP_MAIL_TEST_MODE # instead of editing props so we don't clobber the user's local DB settings. + # OBP_DYNAMIC_CODE_SANDBOX_PERMISSIONS mirrors CI's dynamic_code_sandbox_permissions + # props line: without it the dynamic-code sandbox denies reflection/getenv and + # DynamicResourceDocTest's native-execution scenarios fail locally (CI green, local red). + # -pl obp-commons,obp-api mirrors CI: obp-commons' own util suites run on whichever + # shard's filter matches com.openbankproject.* (the shard-4 catch-all); on every other + # shard the filter matches nothing in obp-commons -> 0 tests there. # OBP_TESTS_PORT + OBP_HTTP4S_TEST_PORT carry the two dynamically-allocated free # ports (both test servers bind a real socket; see the port-allocation block). # Tests only, no recompile (the compile already happened in the pre-compile step). @@ -175,8 +181,9 @@ run_shard() { OBP_HOSTNAME="http://localhost:${port}" \ OBP_HTTP4S_TEST_PORT="${http4s_port}" \ OBP_MAIL_TEST_MODE="true" \ + OBP_DYNAMIC_CODE_SANDBOX_PERMISSIONS='[new java.net.NetPermission("specifyStreamHandler"), new java.lang.reflect.ReflectPermission("suppressAccessChecks"), new java.lang.RuntimePermission("getenv.*"), new java.util.PropertyPermission("cglib.useCache", "read"), new java.util.PropertyPermission("net.sf.cglib.test.stressHashCodes", "read"), new java.util.PropertyPermission("cglib.debugLocation", "read"), new java.lang.RuntimePermission("accessDeclaredMembers"), new java.lang.RuntimePermission("getClassLoader")]' \ OBP_API_INSTANCE_ID="shard_${n}" \ - "$TIMEOUT_BIN" 1200 mvn scalatest:test -pl obp-api -DfailIfNoTests=false \ + "$TIMEOUT_BIN" 1200 mvn scalatest:test -pl obp-commons,obp-api -DfailIfNoTests=false \ "-DwildcardSuites=${filter}" \ > "$log" 2>&1 local rc=$? @@ -239,7 +246,7 @@ if [ $PRECOMPILE_RC -ne 0 ]; then fi # Fresh verdict basis: stale surefire XMLs from earlier runs would poison both the # surefire audit below and the speed report (observed: test counts drifting across runs). -rm -rf obp-api/target/surefire-reports +rm -rf obp-api/target/surefire-reports obp-commons/target/surefire-reports echo "Pre-compile done, starting shards..." echo "" @@ -338,28 +345,39 @@ fi # the reports fails the run regardless of what the shards returned. SF_AUDIT=$(python3 -c ' import xml.etree.ElementTree as ET, glob, os -tot = fail = err = 0 +tot = fail = err = skip = broken = 0 bad = [] -for f in glob.glob("obp-api/target/surefire-reports/TEST-*.xml"): +files = glob.glob("obp-api/target/surefire-reports/TEST-*.xml") + \ + glob.glob("obp-commons/target/surefire-reports/TEST-*.xml") +for f in files: try: r = ET.parse(f).getroot() t, fa, e = int(r.get("tests", 0)), int(r.get("failures", 0)), int(r.get("errors", 0)) + skip += int(r.get("skipped", 0)) tot += t; fail += fa; err += e if fa or e: bad.append(os.path.basename(f)[5:-4] + ": " + str(fa) + " failed, " + str(e) + " errors") except Exception: - pass -print(tot, fail, err) + broken += 1 + bad.append(os.path.basename(f) + ": UNPARSEABLE report (JVM killed mid-write?)") +print(tot, fail, err, skip, broken) for b in bad: print(b) ' 2>/dev/null) -read -r SF_TOTAL SF_FAIL SF_ERR <<< "$(echo "$SF_AUDIT" | head -1)" +read -r SF_TOTAL SF_FAIL SF_ERR SF_SKIP SF_BROKEN <<< "$(echo "$SF_AUDIT" | head -1)" echo "" -echo "Surefire audit: ${SF_TOTAL:-?} tests, ${SF_FAIL:-?} failures, ${SF_ERR:-?} errors" -if [ "${SF_FAIL:-1}" != "0" ] || [ "${SF_ERR:-1}" != "0" ]; then +echo "Surefire audit: ${SF_TOTAL:-?} tests, ${SF_FAIL:-?} failures, ${SF_ERR:-?} errors, ${SF_SKIP:-?} skipped/canceled" +if [ "${SF_FAIL:-1}" != "0" ] || [ "${SF_ERR:-1}" != "0" ] || [ "${SF_BROKEN:-1}" != "0" ]; then echo "$SF_AUDIT" | tail -n +2 | sed 's/^/ ✗ /' OVERALL_RC=1 fi +# Zero-test floor: -DfailIfNoTests=false means a broken wildcardSuites filter runs nothing +# and "passes". The suite has ~2900 tests; a total far below that means shards ran +# near-empty — fail instead of reporting a hollow green. +if [ "${SF_TOTAL:-0}" -lt 2000 ]; then + echo " ✗ suspicious total: only ${SF_TOTAL:-0} tests ran (< 2000 floor) — filter/discovery regression?" + OVERALL_RC=1 +fi # ── CI parity (report job): http4s vs Lift per-test speed table; best-effort, ── # does not affect the exit code. From dded2d29456989141630e10e36618cf8f61ef28f Mon Sep 17 00:00:00 2001 From: hongwei Date: Thu, 2 Jul 2026 14:55:32 +0200 Subject: [PATCH 25/29] refactor(doobie): rename generic rowId parameters to their entity ids conditionalAccountAccessRequestStatus/conditionalAccountApplicationStatus (rowId -> accountAccessRequestId / accountApplicationId) and DoobieUserAuthContextUpdateQueries .conditionalStatusTransition (rowId -> userAuthContextUpdateId) took a generic rowId parameter that gave no hint which entity's id it expected. All call sites pass arguments positionally, so no callers change. --- .../code/bankconnectors/DoobieBusinessStatusQueries.scala | 8 ++++---- .../DoobieUserAuthContextUpdateQueries.scala | 4 ++-- 2 files changed, 6 insertions(+), 6 deletions(-) diff --git a/obp-api/src/main/scala/code/bankconnectors/DoobieBusinessStatusQueries.scala b/obp-api/src/main/scala/code/bankconnectors/DoobieBusinessStatusQueries.scala index 0023380957..1dbb97c1d7 100644 --- a/obp-api/src/main/scala/code/bankconnectors/DoobieBusinessStatusQueries.scala +++ b/obp-api/src/main/scala/code/bankconnectors/DoobieBusinessStatusQueries.scala @@ -17,7 +17,7 @@ object DoobieBusinessStatusQueries { /** AccountAccessRequest: transition only from the guard status. Table has explicit dbTableName. */ def conditionalAccountAccessRequestStatus( - rowId: Long, + accountAccessRequestId: Long, guardStatus: String, newStatus: String, checkerUserId: String, @@ -28,17 +28,17 @@ object DoobieBusinessStatusQueries { checkeruserid = $checkerUserId, checkercomment = $checkerComment, updatedat = NOW() - WHERE id = $rowId + WHERE id = $accountAccessRequestId AND status = $guardStatus""".update.run ) /** MappedAccountApplication: transition only from the guard status (a one-shot decision). */ - def conditionalAccountApplicationStatus(rowId: Long, guardStatus: String, newStatus: String): Int = + def conditionalAccountApplicationStatus(accountApplicationId: Long, guardStatus: String, newStatus: String): Int = DoobieUtil.runUpdate( sql"""UPDATE mappedaccountapplication SET mstatus = $newStatus, updatedat = NOW() - WHERE id = $rowId + WHERE id = $accountApplicationId AND mstatus = $guardStatus""".update.run ) diff --git a/obp-api/src/main/scala/code/bankconnectors/DoobieUserAuthContextUpdateQueries.scala b/obp-api/src/main/scala/code/bankconnectors/DoobieUserAuthContextUpdateQueries.scala index f82ce65eea..a67e620947 100644 --- a/obp-api/src/main/scala/code/bankconnectors/DoobieUserAuthContextUpdateQueries.scala +++ b/obp-api/src/main/scala/code/bankconnectors/DoobieUserAuthContextUpdateQueries.scala @@ -13,12 +13,12 @@ import doobie.implicits._ */ object DoobieUserAuthContextUpdateQueries { - def conditionalStatusTransition(rowId: Long, guardStatus: String, newStatus: String): Int = + def conditionalStatusTransition(userAuthContextUpdateId: Long, guardStatus: String, newStatus: String): Int = DoobieUtil.runUpdate( sql"""UPDATE mappeduserauthcontextupdate SET mstatus = $newStatus, updatedat = NOW() - WHERE id = $rowId + WHERE id = $userAuthContextUpdateId AND mstatus = $guardStatus""".update.run ) } From 91602f8fc628358af28c8b644a7fbae02a447a87 Mon Sep 17 00:00:00 2001 From: hongwei Date: Thu, 2 Jul 2026 14:57:56 +0200 Subject: [PATCH 26/29] refactor(doobie): rename consentRowId to consentPrimaryKey consentRowId sat next to the business identifier consentId (String, mConsentId column) in the same call sites, inviting confusion between the two. Rename to consentPrimaryKey, matching the existing PrimaryKey naming convention elsewhere in the codebase (authUserPrimaryKey, saveMetricsArchive's primaryKey param). Named-argument call sites in ConsentScheduler and ConcurrentConsentRaceTest updated to match. --- .../DoobieConsentSchedulerQueries.scala | 8 ++++---- .../bankconnectors/DoobieConsentStatusQueries.scala | 12 ++++++------ .../main/scala/code/scheduler/ConsentScheduler.scala | 6 +++--- .../code/concurrency/ConcurrentConsentRaceTest.scala | 4 ++-- 4 files changed, 15 insertions(+), 15 deletions(-) diff --git a/obp-api/src/main/scala/code/bankconnectors/DoobieConsentSchedulerQueries.scala b/obp-api/src/main/scala/code/bankconnectors/DoobieConsentSchedulerQueries.scala index 134fced6b3..51f34a700a 100644 --- a/obp-api/src/main/scala/code/bankconnectors/DoobieConsentSchedulerQueries.scala +++ b/obp-api/src/main/scala/code/bankconnectors/DoobieConsentSchedulerQueries.scala @@ -7,7 +7,7 @@ import doobie.implicits._ object DoobieConsentSchedulerQueries { def conditionallyUpdateStatus( - consentRowId: Long, + consentPrimaryKey: Long, guardStatus: String, newStatus: String, newNote: String @@ -16,19 +16,19 @@ object DoobieConsentSchedulerQueries { SET mstatus = $newStatus, mnote = $newNote, mstatusupdatedatetime = NOW() - WHERE id = $consentRowId + WHERE id = $consentPrimaryKey AND mstatus = $guardStatus""".update.run ) def conditionallyExpireValidBerlinGroupConsent( - consentRowId: Long, + consentPrimaryKey: Long, newNote: String ): Int = DoobieUtil.runUpdate( sql"""UPDATE mappedconsent SET mstatus = ${"expired"}, mnote = $newNote, mstatusupdatedatetime = NOW() - WHERE id = $consentRowId + WHERE id = $consentPrimaryKey AND mstatus IN ('valid', 'VALID')""".update.run ) } diff --git a/obp-api/src/main/scala/code/bankconnectors/DoobieConsentStatusQueries.scala b/obp-api/src/main/scala/code/bankconnectors/DoobieConsentStatusQueries.scala index 153ef914ea..c4ddcb5e17 100644 --- a/obp-api/src/main/scala/code/bankconnectors/DoobieConsentStatusQueries.scala +++ b/obp-api/src/main/scala/code/bankconnectors/DoobieConsentStatusQueries.scala @@ -17,21 +17,21 @@ import doobie.implicits._ */ object DoobieConsentStatusQueries { - /** Transition mstatus from an expected guard value to a new value, keyed by row id + /** Transition mstatus from an expected guard value to a new value, keyed by primary key * (for call sites already holding the loaded MappedConsent). Returns affected rows (0 or 1). */ - def conditionalStatusTransition(consentRowId: Long, guardStatus: String, newStatus: String): Int = + def conditionalStatusTransition(consentPrimaryKey: Long, guardStatus: String, newStatus: String): Int = DoobieUtil.runUpdate( sql"""UPDATE mappedconsent SET mstatus = $newStatus, mlastactiondate = NOW(), updatedat = NOW() - WHERE id = $consentRowId + WHERE id = $consentPrimaryKey AND mstatus = $guardStatus""".update.run ) /** Transition mstatus from an expected guard value to a new value, keyed by consent id. * Used by the skip-SCA auto-accept in the createConsent endpoints (v3.1.0 / v5.0.0 / v5.1.0), - * which hold only the consentId — no extra SELECT needed to obtain the row id. + * which hold only the consentId — no extra SELECT needed to obtain the primary key. * Returns affected rows (0 or 1). */ def conditionalStatusTransitionByConsentId(consentId: String, guardStatus: String, newStatus: String): Int = DoobieUtil.runUpdate( @@ -44,13 +44,13 @@ object DoobieConsentStatusQueries { ) /** Revoke unless already at the given terminal status. Returns affected rows (0 or 1). */ - def conditionalRevoke(consentRowId: Long, revokedStatus: String): Int = + def conditionalRevoke(consentPrimaryKey: Long, revokedStatus: String): Int = DoobieUtil.runUpdate( sql"""UPDATE mappedconsent SET mstatus = $revokedStatus, mlastactiondate = NOW(), updatedat = NOW() - WHERE id = $consentRowId + WHERE id = $consentPrimaryKey AND mstatus <> $revokedStatus""".update.run ) } diff --git a/obp-api/src/main/scala/code/scheduler/ConsentScheduler.scala b/obp-api/src/main/scala/code/scheduler/ConsentScheduler.scala index 205e9e5b0d..c26ab4abc0 100644 --- a/obp-api/src/main/scala/code/scheduler/ConsentScheduler.scala +++ b/obp-api/src/main/scala/code/scheduler/ConsentScheduler.scala @@ -71,7 +71,7 @@ object ConsentScheduler extends MdcLoggable { val message = s"|---> Changed status from ${consent.status} to ${ConsentStatus.rejected} for consent ID: ${consent.id}" val newNote = s"$currentDate\n$message\n" + Option(consent.note).getOrElse("") val rows = code.bankconnectors.DoobieConsentSchedulerQueries.conditionallyUpdateStatus( - consentRowId = consent.id.get, + consentPrimaryKey = consent.id.get, guardStatus = ConsentStatus.received.toString, newStatus = ConsentStatus.rejected.toString, newNote = newNote @@ -113,7 +113,7 @@ object ConsentScheduler extends MdcLoggable { val message = s"|---> Changed status from ${consent.status} to ${ConsentStatus.expired} for consent ID: ${consent.id}" val newNote = s"$currentDate\n$message\n" + Option(consent.note).getOrElse("") val rows = code.bankconnectors.DoobieConsentSchedulerQueries.conditionallyExpireValidBerlinGroupConsent( - consentRowId = consent.id.get, + consentPrimaryKey = consent.id.get, newNote = newNote ) if (rows > 0) logger.warn(message) @@ -145,7 +145,7 @@ object ConsentScheduler extends MdcLoggable { val message = s"|---> Changed status from ${consent.status} to ${ConsentStatus.EXPIRED.toString} for consent ID: ${consent.id}" val newNote = s"$currentDate\n$message\n" + Option(consent.note).getOrElse("") val rows = code.bankconnectors.DoobieConsentSchedulerQueries.conditionallyUpdateStatus( - consentRowId = consent.id.get, + consentPrimaryKey = consent.id.get, guardStatus = ConsentStatus.ACCEPTED.toString, newStatus = ConsentStatus.EXPIRED.toString, newNote = newNote diff --git a/obp-api/src/test/scala/code/concurrency/ConcurrentConsentRaceTest.scala b/obp-api/src/test/scala/code/concurrency/ConcurrentConsentRaceTest.scala index 895147bea4..1c4400a966 100644 --- a/obp-api/src/test/scala/code/concurrency/ConcurrentConsentRaceTest.scala +++ b/obp-api/src/test/scala/code/concurrency/ConcurrentConsentRaceTest.scala @@ -85,7 +85,7 @@ class ConcurrentConsentRaceTest extends ConcurrentRaceSetup { And("the scheduler attempts to expire its stale copy via the guarded conditional update") DoobieConsentSchedulerQueries.conditionallyExpireValidBerlinGroupConsent( - consentRowId = staleConsent.id.get, + consentPrimaryKey = staleConsent.id.get, newNote = "" ) @@ -127,7 +127,7 @@ class ConcurrentConsentRaceTest extends ConcurrentRaceSetup { And("the scheduler attempts to reject its stale copy via the guarded conditional update") DoobieConsentSchedulerQueries.conditionallyUpdateStatus( - consentRowId = staleConsent.id.get, + consentPrimaryKey = staleConsent.id.get, guardStatus = ConsentStatus.received.toString, newStatus = ConsentStatus.rejected.toString, newNote = "" From 1435f9f84a74688a428aaf36eb17fd6515ad6b53 Mon Sep 17 00:00:00 2001 From: hongwei Date: Thu, 2 Jul 2026 14:59:54 +0200 Subject: [PATCH 27/29] refactor(dynamic-entity): rename getReadableRowIds to getReadableDynamicDataIds The method returns DynamicDataAccess.DynamicDataId values, and the sibling method in the same trait already names that concept dynamicDataId (getAccessForRow). getReadableRowIds was the only outlier still using RowId terminology. --- .../scala/code/api/dynamic/entity/Http4sDynamicEntity.scala | 2 +- .../scala/code/dynamicEntity/DynamicDataAccessProvider.scala | 4 ++-- .../code/dynamicEntity/MappedDynamicDataAccessProvider.scala | 2 +- 3 files changed, 4 insertions(+), 4 deletions(-) diff --git a/obp-api/src/main/scala/code/api/dynamic/entity/Http4sDynamicEntity.scala b/obp-api/src/main/scala/code/api/dynamic/entity/Http4sDynamicEntity.scala index 0fb8a25307..a66fa5e495 100644 --- a/obp-api/src/main/scala/code/api/dynamic/entity/Http4sDynamicEntity.scala +++ b/obp-api/src/main/scala/code/api/dynamic/entity/Http4sDynamicEntity.scala @@ -351,7 +351,7 @@ object Http4sDynamicEntity extends MdcLoggable { // In-memory floor: fetch all rows (unscoped) and keep those the ACL marks readable. // (The projection EXISTS backend for row-level get-all is a documented follow-up; the // in-memory path is always correct, just not index-accelerated.) - val readable = aclVend.getReadableRowIds(u.userId, entityName, bankId).toSet + val readable = aclVend.getReadableDynamicDataIds(u.userId, entityName, bankId).toSet val readableRows = dataVend.getAllCommunity(bankId, entityName).filter(_.dynamicDataId.exists(readable.contains)) val readableJson: JArray = JArray(readableRows.map(r => parse(r.dataJson))) val filtered = filterDynamicObjects(readableJson, queryParams(req)) diff --git a/obp-api/src/main/scala/code/dynamicEntity/DynamicDataAccessProvider.scala b/obp-api/src/main/scala/code/dynamicEntity/DynamicDataAccessProvider.scala index 8199075007..5f6e21af58 100644 --- a/obp-api/src/main/scala/code/dynamicEntity/DynamicDataAccessProvider.scala +++ b/obp-api/src/main/scala/code/dynamicEntity/DynamicDataAccessProvider.scala @@ -56,8 +56,8 @@ trait DynamicDataAccessProvider { /** All ACL rows for a single data row (for the GET .../access listing). */ def getAccessForRow(dynamicDataId: String): List[DynamicDataAccessT] - /** Ids of the rows of `entityName`/`bankId` that `userId` may read — the get-all filter. */ - def getReadableRowIds(userId: String, entityName: String, bankId: Option[String]): List[String] + /** DynamicDataIds of `entityName`/`bankId` that `userId` may read — the get-all filter. */ + def getReadableDynamicDataIds(userId: String, entityName: String, bankId: Option[String]): List[String] /** Does `userId` hold `permission` on `dynamicDataId`? */ def allows(dynamicDataId: String, userId: String, permission: DynamicDataAccessPermission): Boolean diff --git a/obp-api/src/main/scala/code/dynamicEntity/MappedDynamicDataAccessProvider.scala b/obp-api/src/main/scala/code/dynamicEntity/MappedDynamicDataAccessProvider.scala index 68a5f2ae38..8b3ae80ea3 100644 --- a/obp-api/src/main/scala/code/dynamicEntity/MappedDynamicDataAccessProvider.scala +++ b/obp-api/src/main/scala/code/dynamicEntity/MappedDynamicDataAccessProvider.scala @@ -58,7 +58,7 @@ object MappedDynamicDataAccessProvider extends DynamicDataAccessProvider { override def getAccessForRow(dynamicDataId: String): List[DynamicDataAccessT] = DynamicDataAccess.findAll(By(DynamicDataAccess.DynamicDataId, dynamicDataId)) - override def getReadableRowIds(userId: String, entityName: String, bankId: Option[String]): List[String] = { + override def getReadableDynamicDataIds(userId: String, entityName: String, bankId: Option[String]): List[String] = { val base: List[QueryParam[DynamicDataAccess]] = List( By(DynamicDataAccess.UserId, userId), By(DynamicDataAccess.EntityName, entityName), From 19bc3cd45ed3e935e55235d96d6f02a72d4e9a3b Mon Sep 17 00:00:00 2001 From: hongwei Date: Thu, 2 Jul 2026 15:08:16 +0200 Subject: [PATCH 28/29] chore: remove switch_to_java11.sh (unused, unreferenced) --- switch_to_java11.sh | 85 --------------------------------------------- 1 file changed, 85 deletions(-) delete mode 100755 switch_to_java11.sh diff --git a/switch_to_java11.sh b/switch_to_java11.sh deleted file mode 100755 index a51d0e2e47..0000000000 --- a/switch_to_java11.sh +++ /dev/null @@ -1,85 +0,0 @@ -#!/bin/bash - -# Script to switch system Java to Java 11 -# This will update your shell configuration to use Java 11 by default - -echo "=== Switching System Java to Java 11 ===" -echo "" - -# Detect Java 11 path -JAVA11_PATH="/Users/zhanghongwei/Library/Java/JavaVirtualMachines/azul-11.0.24/Contents/Home" - -if [ ! -d "$JAVA11_PATH" ]; then - echo "❌ Error: Java 11 not found at $JAVA11_PATH" - echo "Please install Java 11 first" - exit 1 -fi - -echo "✅ Found Java 11 at: $JAVA11_PATH" -echo "" - -# Detect shell configuration file -if [ -f ~/.zshrc ]; then - SHELL_CONFIG=~/.zshrc - SHELL_NAME="zsh" -elif [ -f ~/.bash_profile ]; then - SHELL_CONFIG=~/.bash_profile - SHELL_NAME="bash" -elif [ -f ~/.bashrc ]; then - SHELL_CONFIG=~/.bashrc - SHELL_NAME="bash" -else - echo "❌ Error: Could not find shell configuration file" - exit 1 -fi - -echo "📝 Detected shell: $SHELL_NAME" -echo "📝 Configuration file: $SHELL_CONFIG" -echo "" - -# Backup existing configuration -BACKUP_FILE="${SHELL_CONFIG}.backup.$(date +%Y%m%d_%H%M%S)" -cp "$SHELL_CONFIG" "$BACKUP_FILE" -echo "✅ Backed up configuration to: $BACKUP_FILE" -echo "" - -# Remove existing JAVA_HOME settings -echo "🔧 Removing existing JAVA_HOME settings..." -sed -i.tmp '/export JAVA_HOME=/d' "$SHELL_CONFIG" -sed -i.tmp '/JAVA_HOME=/d' "$SHELL_CONFIG" -rm -f "${SHELL_CONFIG}.tmp" - -# Add Java 11 configuration -echo "🔧 Adding Java 11 configuration..." -cat >> "$SHELL_CONFIG" << 'EOF' - -# Java 11 Configuration (added by switch_to_java11.sh) -export JAVA_HOME=/Users/zhanghongwei/Library/Java/JavaVirtualMachines/azul-11.0.24/Contents/Home -export PATH="$JAVA_HOME/bin:$PATH" -EOF - -echo "✅ Java 11 configuration added to $SHELL_CONFIG" -echo "" - -# Apply changes to current session -export JAVA_HOME="$JAVA11_PATH" -export PATH="$JAVA_HOME/bin:$PATH" - -# Verify -echo "=== Verification ===" -java -version -echo "" - -echo "✅ Java 11 is now active in this terminal session!" -echo "" -echo "📌 To apply changes to all future terminal sessions:" -echo " 1. Close this terminal" -echo " 2. Open a new terminal" -echo " 3. Run: java -version" -echo "" -echo "📌 Or reload configuration in current terminal:" -echo " source $SHELL_CONFIG" -echo "" -echo "📌 To revert changes:" -echo " cp $BACKUP_FILE $SHELL_CONFIG" -echo " source $SHELL_CONFIG" From f3c59c9cb4ad9edee2e8e43fd40e60ddff31b2c9 Mon Sep 17 00:00:00 2001 From: hongwei Date: Thu, 2 Jul 2026 15:10:00 +0200 Subject: [PATCH 29/29] chore: restore run_all_tests.sh as a thin forward to run_tests_parallel.sh Kept for anyone still typing ./run_all_tests.sh out of habit; it does no work of its own and just execs run_tests_parallel.sh with the same arguments. --- run_all_tests.sh | 7 +++++++ 1 file changed, 7 insertions(+) create mode 100755 run_all_tests.sh diff --git a/run_all_tests.sh b/run_all_tests.sh new file mode 100755 index 0000000000..c2a875a284 --- /dev/null +++ b/run_all_tests.sh @@ -0,0 +1,7 @@ +#!/bin/bash +# Backward-compat shim: some docs/scripts still say "run ./run_all_tests.sh". +# The real runner is run_tests_parallel.sh — this just forwards to it. +# Usage: ./run_all_tests.sh [--shards=4|6] + +SCRIPT_DIR="$(cd "$(dirname "${BASH_SOURCE[0]}")" && pwd)" +exec "$SCRIPT_DIR/run_tests_parallel.sh" "$@"