diff --git a/obp-api/src/main/scala/bootstrap/liftweb/Boot.scala b/obp-api/src/main/scala/bootstrap/liftweb/Boot.scala index 9897172202..36eae74d29 100644 --- a/obp-api/src/main/scala/bootstrap/liftweb/Boot.scala +++ b/obp-api/src/main/scala/bootstrap/liftweb/Boot.scala @@ -265,17 +265,33 @@ class Boot extends MdcLoggable { */ MapperRules.createForeignKeys_? = (_) => APIUtil.getPropsAsBoolValue("mapper_rules.create_foreign_keys", false) + // Pre-Schemifier dedup: drop natural-key duplicate rows in mapperaccountholder / + // mappedentitlement BEFORE schemifyAll() issues their CREATE UNIQUE INDEX (declared in + // MapperAccountHolders / MappedEntitlement dbIndexes). On a long-lived DB that still holds + // duplicates the index DDL would otherwise abort boot. + // + // This MUST stay here and must NOT be moved into Migration.database.executeScripts: + // - both executeScripts passes below run AFTER schemifyAll() (the index is already created + // by then — the "executed before Schemifier" comment on the true-pass is historical), and + // - executeScripts is gated by migration_scripts.* props (off in tests), whereas Schemifier — + // and therefore this dedup — must run ungated in every environment, incl. the H2 test DB. + // The method self-guards (skips when the table is absent or has no duplicates), so running it + // on every boot is a cheap no-op on fresh/clean/test databases. + Migration.database.deduplicateBeforeUniqueIndexSchemify() schemifyAll() logger.info("Mapper database info: " + Migration.DbFunction.mapperDatabaseInfo) + // NOTE: both executeScripts passes below run AFTER schemifyAll() above. The + // `startedBeforeSchemifier = true` argument does NOT mean this pass runs before Schemifier — it + // marks the existing-DB pass, in which migrations that require post-Schemifier schema skip + // themselves (see Migration.executeScripts). The "before Schemifier" wording is historical: the + // call once sat before schemifyAll() but was moved ahead of it in 2021 (commit ea4537029). DbFunction.tableExists(ResourceUser) match { - case true => // DB already exist - // Migration Scripts are used to update the model of OBP-API DB to a latest version. - // Please note that migration scripts are executed before Lift Mapper Schemifier + case true => // DB already exists Migration.database.executeScripts(startedBeforeSchemifier = true) - logger.info("The Mapper database already exits. The scripts are executed BEFORE Lift Mapper Schemifier.") - case false => // DB is still not created. The scripts will be executed after Lift Mapper Schemifier + logger.info("The Mapper database already exists. Running the existing-DB migration pass (post-Schemifier; migrations needing fresh schema skip themselves).") + case false => // Fresh DB — its migrations run in the catch-all pass below (after Schemifier) logger.info("The Mapper database is still not created. The scripts are going to be executed AFTER Lift Mapper Schemifier.") } diff --git a/obp-api/src/main/scala/code/accountholders/MapperAccountHolders.scala b/obp-api/src/main/scala/code/accountholders/MapperAccountHolders.scala index 247bb50802..050d557ce3 100644 --- a/obp-api/src/main/scala/code/accountholders/MapperAccountHolders.scala +++ b/obp-api/src/main/scala/code/accountholders/MapperAccountHolders.scala @@ -9,6 +9,7 @@ import com.openbankproject.commons.model.{AccountId, BankId, BankIdAccountId, Us import net.liftweb.common._ import net.liftweb.mapper._ import net.liftweb.common.Box +import net.liftweb.util.Helpers.tryo /** @@ -31,7 +32,7 @@ object MapperAccountHolders extends MapperAccountHolders with AccountHolders wit // NOTE: !!! Uses a DIFFERENT TABLE NAME PREFIX TO ALL OTHERS i.e. MAPPER not MAPPED !!!!! - override def dbIndexes = Index(accountBankPermalink, accountPermalink) :: Nil + override def dbIndexes = UniqueIndex(user, accountBankPermalink, accountPermalink) :: Nil //Note, this method, will not check the existing of bankAccount, any value of BankIdAccountId //Can create the MapperAccountHolders. @@ -51,16 +52,25 @@ object MapperAccountHolders extends MapperAccountHolders with AccountHolders wit mapperAccountHolder } case Empty => { - val holder: MapperAccountHolders = MapperAccountHolders.create - .accountBankPermalink(bankIdAccountId.bankId.value) - .accountPermalink(bankIdAccountId.accountId.value) - .user(user.userPrimaryKey.value) - .source(source.getOrElse(null)) - .saveMe - logger.debug( - s"getOrCreateAccountHolder--> create account holder: $holder" - ) - Full(holder) + tryo { + MapperAccountHolders.create + .accountBankPermalink(bankIdAccountId.bankId.value) + .accountPermalink(bankIdAccountId.accountId.value) + .user(user.userPrimaryKey.value) + .source(source.getOrElse(null)) + .saveMe + } match { + case Full(holder) => + logger.debug(s"getOrCreateAccountHolder--> create account holder: $holder") + Full(holder) + case Failure(_, _, _) => + MapperAccountHolders.find( + By(MapperAccountHolders.user, user.userPrimaryKey.value), + By(MapperAccountHolders.accountBankPermalink, bankIdAccountId.bankId.value), + By(MapperAccountHolders.accountPermalink, bankIdAccountId.accountId.value) + ) + case other => other + } } case Failure(msg, t, c) => Failure(msg, t, c) case ParamFailure(x,y,z,q) => ParamFailure(x,y,z,q) 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 753e622488..fe93b9a8d1 100644 --- a/obp-api/src/main/scala/code/api/util/APIUtil.scala +++ b/obp-api/src/main/scala/code/api/util/APIUtil.scala @@ -4834,10 +4834,17 @@ object APIUtil extends MdcLoggable with CustomJsonFormats{ } def incrementFutureCounter(serviceName:String) = { - val (serviceNameCounter, serviceNameOpenFuturesCounter) = serviceNameCountersMap.getOrDefault(serviceName,(0,0)) - serviceNameCountersMap.put(serviceName,(serviceNameCounter + 1,serviceNameOpenFuturesCounter+1)) - val (serviceNameCounterLatest, serviceNameOpenFuturesCounterLatest) = serviceNameCountersMap.getOrDefault(serviceName,(0,0)) - + // Atomically increment both the total-call counter and the open-futures counter for this + // service. ConcurrentHashMap.compute holds the segment lock for the entire lambda, so the + // read-modify-write is a single atomic step — no lost updates under concurrent callers. + // totalCallCount : ever-increasing; used by canOpenFuture for back-off modulo arithmetic. + // openFuturesCount: tracks how many futures are currently in-flight for this service. + val (serviceNameCounterLatest, serviceNameOpenFuturesCounterLatest) = + serviceNameCountersMap.compute(serviceName, (_, old) => { + val (totalCallCount, openFuturesCount) = if (old == null) (0, 0) else old + (totalCallCount + 1, openFuturesCount + 1) + }) + if(serviceNameOpenFuturesCounterLatest>=expectedOpenFuturesPerService) { logger.warn(s"WARNING! incrementFutureCounter says: serviceName is $serviceName, serviceNameOpenFuturesCounterLatest is ${serviceNameOpenFuturesCounterLatest}, which is over expectedOpenFuturesPerService($expectedOpenFuturesPerService)") } @@ -4845,9 +4852,15 @@ object APIUtil extends MdcLoggable with CustomJsonFormats{ } def decrementFutureCounter(serviceName:String) = { - val (serviceNameCounter, serviceNameOpenFuturesCounter) = serviceNameCountersMap.getOrDefault(serviceName, (0, 1)) - serviceNameCountersMap.put(serviceName, (serviceNameCounter, serviceNameOpenFuturesCounter - 1)) - val (serviceNameCounterLatest, serviceNameOpenFuturesCounterLatest) = serviceNameCountersMap.getOrDefault(serviceName, (0, 1)) + // Atomically decrement only the open-futures counter; totalCallCount is left unchanged + // because it reflects the cumulative number of calls ever started, not the current load. + // The null-guard initialises to (0, 1) and subtracts 1 → (0, 0), which is the safe + // no-op fallback if decrement is somehow called before any increment. + val (serviceNameCounterLatest, serviceNameOpenFuturesCounterLatest) = + serviceNameCountersMap.compute(serviceName, (_, old) => { + val (totalCallCount, openFuturesCount) = if (old == null) (0, 1) else old + (totalCallCount, openFuturesCount - 1) + }) logger.debug(s"decrementFutureCounter says: serviceName is $serviceName, serviceNameCounterLatest is $serviceNameCounterLatest, serviceNameOpenFuturesCounterLatest is ${serviceNameOpenFuturesCounterLatest}") } 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 d5961f8c70..a8d35cf330 100644 --- a/obp-api/src/main/scala/code/api/util/DoobieTransactor.scala +++ b/obp-api/src/main/scala/code/api/util/DoobieTransactor.scala @@ -16,18 +16,19 @@ import scala.concurrent.{ExecutionContext, Future} * * Provides a type-safe, functional JDBC layer for raw SQL queries. * This handles all JDBC types correctly, including SQL Server's NVARCHAR (type -9) - * which Lift's DB.runQuery doesn't handle. + * which Lift Mapper's DB.runQuery doesn't handle. * * TRANSACTION UNIFICATION: - * When called within a Lift HTTP request context, Doobie uses the SAME Connection - * that Lift is holding for the current request transaction (via Transactor.fromConnection). - * This means Doobie queries participate in Lift's transaction boundary: + * When called within an http4s request scope, Doobie uses the SAME Connection that + * RequestScopeConnection holds for the current request transaction (via + * Transactor.fromConnection). This means Doobie queries participate in the request + * transaction boundary: * - Same connection, same transaction, same commit/rollback - * - Doobie can see uncommitted Lift writes (same session) - * - If Lift rolls back, Doobie's operations are also rolled back + * - Doobie can see uncommitted writes made earlier in the same request (same session) + * - If the request transaction rolls back, Doobie's operations are also rolled back * - * When called outside a Lift request context (e.g., background tasks, schedulers), - * falls back to Lift's shared HikariCP connection pool via Transactor.fromDataSource. + * When called outside an http4s request scope (e.g., background tasks, schedulers), + * falls back to the shared HikariCP connection pool via Transactor.fromDataSource. * * Benefits over DBUtil.runQuery: * - Type-safe query results via case classes @@ -56,15 +57,15 @@ import scala.concurrent.{ExecutionContext, Future} object DoobieUtil extends MdcLoggable { /** - * Fallback transactor that shares Lift's HikariCP connection pool. - * Used when no Lift request context is available (background tasks, schedulers). + * Fallback transactor that shares the application HikariCP connection pool. + * Used when no http4s request scope is available (background tasks, schedulers). * Strategy.void: Doobie will not call setAutoCommit/commit/rollback. */ private lazy val fallbackTransactor: Transactor[IO] = { - val liftDataSource = APIUtil.vendor.HikariDatasource.ds - logger.info("DoobieUtil: Initialized fallback transactor sharing Lift's HikariCP pool") + val sharedDataSource = APIUtil.vendor.HikariDatasource.ds + logger.info("DoobieUtil: Initialized fallback transactor sharing the application HikariCP pool") val xa = Transactor.fromDataSource[IO].apply( - liftDataSource, + sharedDataSource, ExecutionContext.global ) xa.copy(strategy0 = Strategy.void) @@ -72,7 +73,8 @@ object DoobieUtil extends MdcLoggable { /** * Create a transactor that wraps an existing JDBC Connection. - * Strategy.void ensures Doobie does not interfere with Lift's transaction management. + * Strategy.void ensures Doobie does not interfere with the request-scoped transaction + * management owned by RequestScopeConnection.withBusinessDBTransaction. */ private def transactorFromConnection(conn: java.sql.Connection): Transactor[IO] = { val xa = Transactor.fromConnection[IO].apply(conn, None) @@ -80,45 +82,52 @@ object DoobieUtil extends MdcLoggable { } /** - * Try to get the current Lift request's Connection. - * Uses DB.currentConnection which peeks at the DynoVar without - * triggering reference counting or creating a new connection. - * Returns Some(connection) if inside a Lift HTTP request context, - * None otherwise (background tasks, schedulers, tests without request context). + * Try to get the current request's Connection. + * + * Primary path is the http4s RequestScopeConnection proxy (set per request via TTL). + * As a secondary fallback it reads Lift Mapper's DB.currentConnection — this only + * resolves when the call happens to run inside an open Mapper DB.use scope (the proxy + * is also on Mapper's connection stack there); it peeks at the DynaVar without triggering + * reference counting or creating a new connection. + * + * Returns Some(connection) when a request-scoped connection is available, None otherwise + * (background tasks, schedulers, tests without a request scope). */ - private def liftCurrentConnection: Option[java.sql.Connection] = { - // DB.currentConnection returns Box[SuperConnection] - // SuperConnection has implicit conversion to java.sql.Connection - DB.currentConnection match { - case Full(superConn) => - val conn: java.sql.Connection = superConn.connection - if (!conn.isClosed) Some(conn) else None - case _ => None + 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 { + // 2. Fallback: Lift Mapper's DB.currentConnection (only Full inside an open DB.use scope) + DB.currentConnection match { + case Full(superConn) => + val conn: java.sql.Connection = superConn.connection + if (!conn.isClosed) Some(conn) else None + case _ => None + } } } /** - * Run a Doobie query synchronously, sharing Lift's transaction when available. + * Run a Doobie query synchronously, sharing the request-scoped transaction when available. * - * When called within a Lift HTTP request context: - * - Uses the SAME Connection that Lift holds for the current request - * - Doobie query participates in Lift's transaction (same commit/rollback) - * - Can see uncommitted Lift writes (same database session) + * When called within an http4s request scope: + * - Uses the SAME Connection that RequestScopeConnection holds for the current request + * - Doobie query participates in the request transaction (same commit/rollback) + * - Can see uncommitted writes made earlier in the same request (same database session) * - * When called outside a Lift request context (background tasks, schedulers): - * - Falls back to Lift's shared HikariCP pool (separate connection) + * When called outside an http4s request scope (background tasks, schedulers): + * - Falls back to the shared HikariCP pool (separate connection) * * @param query The Doobie ConnectionIO query to execute * @return The query result */ def runQuery[A](query: ConnectionIO[A]): A = { - liftCurrentConnection match { + currentRequestConnection match { case Some(conn) => - // Inside Lift request: use the same connection for transaction unification + // Inside a request scope: use the same connection for transaction unification query.transact(transactorFromConnection(conn)).unsafeRunSync() case None => - // Outside Lift request: fallback to shared pool - logger.debug("DoobieUtil.runQuery: No Lift request context, using fallback pool transactor") + // Outside a request scope: fallback to shared pool + logger.debug("DoobieUtil.runQuery: No request scope, using fallback pool transactor") query.transact(fallbackTransactor).unsafeRunSync() } } @@ -126,7 +135,7 @@ object DoobieUtil extends MdcLoggable { /** * Run a Doobie query asynchronously, returning a Future. * Note: async queries always use the fallback pool transactor because - * Lift's request connection may not be available on a different thread. + * the request connection may not be available on a different thread. * * @param query The Doobie ConnectionIO query to execute * @param ec ExecutionContext for the Future @@ -139,7 +148,7 @@ object DoobieUtil extends MdcLoggable { /** * Run a Doobie query and return an IO. * Note: IO queries always use the fallback pool transactor because - * the IO may be evaluated outside the Lift request context. + * the IO may be evaluated outside the http4s request scope. * * @param query The Doobie ConnectionIO query to execute * @return IO containing the query result @@ -148,6 +157,32 @@ object DoobieUtil extends MdcLoggable { query.transact(fallbackTransactor) } + /** + * Fallback transactor that commits. Used for updates outside an http4s request scope + * (background tasks, schedulers). + */ + private lazy val fallbackUpdateTransactor: Transactor[IO] = { + val sharedDataSource = APIUtil.vendor.HikariDatasource.ds + Transactor.fromDataSource[IO].apply( + sharedDataSource, + ExecutionContext.global + ) // Strategy.default includes commit/rollback + } + + /** + * Run a Doobie update synchronously, sharing the request-scoped transaction when available. + * Outside an http4s request scope, uses a transactor that COMMITs the connection. + */ + def runUpdate[A](query: ConnectionIO[A]): A = { + currentRequestConnection match { + case Some(conn) => + query.transact(transactorFromConnection(conn)).unsafeRunSync() + case None => + logger.debug("DoobieUtil.runUpdate: No request scope, using fallback update transactor") + query.transact(fallbackUpdateTransactor).unsafeRunSync() + } + } + /** * Check if the database is SQL Server (for syntax differences like TOP vs LIMIT) */ diff --git a/obp-api/src/main/scala/code/api/util/http4s/RequestScopeConnection.scala b/obp-api/src/main/scala/code/api/util/http4s/RequestScopeConnection.scala index 170bf72b4e..0f98d1bf88 100644 --- a/obp-api/src/main/scala/code/api/util/http4s/RequestScopeConnection.scala +++ b/obp-api/src/main/scala/code/api/util/http4s/RequestScopeConnection.scala @@ -15,7 +15,7 @@ import java.sql.Connection import scala.concurrent.Future /** - * Request-scoped transaction support for v7 http4s endpoints. + * Request-scoped transaction support for Http4s native endpoints. * * PROBLEM: Lift Mapper uses a plain ThreadLocal for connection tracking, while * cats-effect IO switches compute threads across flatMap / IO.fromFuture boundaries. @@ -63,11 +63,11 @@ import scala.concurrent.Future * METRIC WRITES: recordMetric runs in IO.blocking (blocking pool, no TTL from compute * thread). currentProxy.get() returns null there, so RequestAwareConnectionManager * falls back to the pool — metric writes use a separate connection and commit - * independently, matching v6 behaviour. + * independently, matching traditional Lift behaviour. * - * NON-V7 PATHS (v6 via bridge, background tasks): requestProxyLocal is not set, - * currentProxy is null — RequestAwareConnectionManager delegates to APIUtil.vendor - * as before. DB.buildLoanWrapper (v6) continues to manage its own transaction. + * BACKGROUND TASKS / NON-HTTP PATHS: requestProxyLocal is not set, currentProxy is null + * — RequestAwareConnectionManager delegates to APIUtil.vendor. Any Lift Mapper operations + * outside of a Http4s request scope will auto-commit unless wrapped in a Lift LoanWrapper. */ object RequestScopeConnection extends MdcLoggable { @@ -186,7 +186,7 @@ object RequestScopeConnection extends MdcLoggable { * If no DB call was made: nothing to commit or close (pool unaffected). * * GET/HEAD must NOT be wrapped (they run on auto-commit vendor connections). Used by - * ResourceDocMiddleware (v6/v7) and by services that build their own request scope + * ResourceDocMiddleware and by services that build their own request scope * without the middleware (e.g. Http4sDynamicEntity). */ def withBusinessDBTransaction(io: IO[Response[IO]]): IO[Response[IO]] = @@ -250,8 +250,8 @@ object RequestScopeConnection extends MdcLoggable { * DB.defineConnectionManager(..., new RequestAwareConnectionManager(APIUtil.vendor)) * * Used by: - * - v7 native endpoints (gets proxy from TTL, set right before Future submission) - * - v6 via bridge / background tasks (TTL is null → delegates to vendor as before) + * - Http4s native endpoints (gets proxy from TTL, set right before Future submission) + * - Background tasks / Non-HTTP paths (TTL is null → delegates to vendor as before) */ class RequestAwareConnectionManager(delegate: ConnectionManager) extends ConnectionManager with MdcLoggable { diff --git a/obp-api/src/main/scala/code/api/util/migration/Migration.scala b/obp-api/src/main/scala/code/api/util/migration/Migration.scala index 09d231f642..95853d132f 100644 --- a/obp-api/src/main/scala/code/api/util/migration/Migration.scala +++ b/obp-api/src/main/scala/code/api/util/migration/Migration.scala @@ -81,11 +81,21 @@ object Migration extends MdcLoggable { } object database { - + + /** + * Runs the migration scripts. Called twice from Boot, BOTH times AFTER `schemifyAll()`. + * + * `startedBeforeSchemifier` does NOT mean "this pass runs before Schemifier" — despite the name + * and the historical Boot comments, both passes run after it. It selects which pass this is: + * - `true` = the existing-DB pass (only invoked when `tableExists(ResourceUser)`): migrations + * that require post-Schemifier schema guard on this flag and skip themselves here. + * - `false` = the catch-all pass that runs for every DB; the guarded migrations run in this one. + * `runOnce` (tracked in `MigrationScriptLog`) guarantees each named migration executes exactly + * once across both passes. + */ def executeScripts(startedBeforeSchemifier: Boolean): Boolean = executeScript { dummyScript() addAccountAccessConsumerId() -// populateMigrationOfViewDefinitionPermissions(startedBeforeSchemifier) generateAndPopulateMissingCustomerUUIDs(startedBeforeSchemifier) generateAndPopulateMissingConsumersUUIDs(startedBeforeSchemifier) populateTableRateLimiting() @@ -125,10 +135,7 @@ object Migration extends MdcLoggable { dropMappedBadLoginAttemptIndex() alterMetricColumnUrlLength() alterMetricArchiveColumnCorrelationidLength() -// populateViewDefinitionCanAddTransactionRequestToBeneficiary() -// populateViewDefinitionCanSeeTransactionStatus() alterCounterpartyLimitFieldType() - populateMigrationOfViewPermissions(startedBeforeSchemifier) changeTypeOfAudFieldAtConsumerTable() renameCustomerRoleNames() addUniqueIndexOnResourceUserUserId() @@ -149,7 +156,94 @@ object Migration extends MdcLoggable { migrateMetricConsentReferenceId(startedBeforeSchemifier) dropFastFirehoseAccountsViews(startedBeforeSchemifier) } - + + /** + * Remove natural-key duplicate rows so Schemifier's CREATE UNIQUE INDEX on + * `mapperaccountholder` (user_, bank, account) and `mappedentitlement` (bank, user, role) + * cannot abort boot on an existing DB that still holds duplicates. + * + * Deliberately invoked directly from `Boot` BEFORE `schemifyAll()` and NOT routed through + * `executeScripts`/`runOnce`: those passes run AFTER Schemifier (too late — the index DDL has + * already run) and are gated by `migration_scripts.*` props (off in tests), whereas Schemifier + * creates the index ungated in every environment incl. H2. Keeps each table's dedup self-guarded + * (table-existence + has-duplicates probe), so it is a cheap no-op on fresh/clean/test DBs and + * needs no `MigrationScriptLog` entry. See the call site in `Boot.scala` for the full rationale. + */ + def deduplicateBeforeUniqueIndexSchemify(): Unit = { + deduplicateNaturalKeyDups( + tableName = "mapperaccountholder", + idCol = "id", + groupCols = List("user_", "accountbankpermalink", "accountpermalink") + ) + deduplicateNaturalKeyDups( + tableName = "mappedentitlement", + idCol = "id", + groupCols = List("mbankid", "muserid", "mrolename") + ) + } + + /** + * Collapse natural-key duplicates in `tableName` down to one surviving row per key group. + * + * Survivor policy: KEEP the row with the lowest `idCol` (the oldest insert) per `groupCols` + * group, DELETE the rest. The discarded duplicates are NOT byte-identical to the survivor — + * only the natural key matches — so this is lossy by design: + * - `mappedentitlement`: each duplicate carries its own `mentitlementid` UUID (the external + * handle returned by the API and used by `getEntitlementById`/`deleteEntitlement`), plus + * `created_by_process` / `group_id` / `process` / `entitlement_request_id` / timestamps. + * Removing a duplicate invalidates any stale reference to *that* row's UUID. This is + * acceptable: the surviving row encodes the identical (bank, user, role) grant, so + * authorization is unaffected — only dead handles to the removed copies break. + * - `mapperaccountholder`: duplicates may differ in `source` (provenance metadata). The + * surviving row encodes the same (user, account) ownership link. + * + * Safe to run on every boot and under concurrent multi-node boot: the survivor set is a + * deterministic lowest-id-per-group, the DELETE is idempotent (re-running removes 0 rows), and + * Lift Mapper's Schemifier emits no DB-level FK constraints, so the DELETE neither cascades nor + * aborts on referential integrity. The has-duplicates probe keeps clean/fresh/test DBs on the + * cheap path — the heavier delete only runs when extras actually exist. The delete uses a + * derived-table + ROW_NUMBER() form (see inline note) so it is portable across every driver OBP + * ships, including MySQL/MariaDB, instead of the MySQL-incompatible `NOT IN (SELECT MIN ...)`. + */ + private def deduplicateNaturalKeyDups(tableName: String, idCol: String, groupCols: List[String]): Unit = { + if (DbFunction.tableExistsByName(tableName)) { + val groupBy = groupCols.mkString(", ") + val hasDups = DB.use(net.liftweb.util.DefaultConnectionIdentifier) { conn => + val st = conn.createStatement() + try { + val rs = st.executeQuery(s"SELECT 1 FROM $tableName GROUP BY $groupBy HAVING COUNT(*) > 1") + try rs.next() finally rs.close() + } finally st.close() + } + if (hasDups) { + logger.warn(s"deduplicateBeforeUniqueIndexSchemify: duplicates found in $tableName – removing extras (keeping the lowest $idCol per [$groupBy])") + // Delete-set shape (target only the few extras), deliberately NOT survivor-set + // (`... NOT IN (SELECT MIN(id) FROM sameTable ...)`): the survivor-set form has the + // subquery's FROM name the very table being deleted, which throws MySQL/MariaDB + // ERROR 1093 ("can't specify target table for update in FROM clause") — and MySQL is a + // first-class OBP target (driver shipped, per-vendor branches throughout this package). + // Wrapping ROW_NUMBER() in a derived table (`(...) tmp`, no AS — Oracle-safe) is the one + // form portable across every driver OBP ships: the derived table is materialised, which + // sidesteps 1093, and window functions are supported by all of PostgreSQL, H2 2.x, + // MySQL 8+/MariaDB 10.2+, SQL Server and Oracle. `ORDER BY $idCol ASC` + `rn > 1` deletes + // all but the lowest id per group — the identical survivor the NOT IN/MIN form kept. + val deleteSql = + s"""DELETE FROM $tableName WHERE $idCol IN ( + | SELECT $idCol FROM ( + | SELECT $idCol, ROW_NUMBER() OVER (PARTITION BY $groupBy ORDER BY $idCol ASC) AS rn FROM $tableName + | ) tmp WHERE rn > 1 + |)""".stripMargin + val deleted = DB.use(net.liftweb.util.DefaultConnectionIdentifier) { conn => + val st = conn.createStatement() + try { + st.executeUpdate(deleteSql) + } finally st.close() + } + logger.warn(s"deduplicateBeforeUniqueIndexSchemify: removed $deleted duplicate row(s) from $tableName") + } + } + } + private def dummyScript(): Boolean = { val name = nameOf(dummyScript) runOnce(name) { @@ -163,44 +257,6 @@ object Migration extends MdcLoggable { } } -// private def populateViewDefinitionCanAddTransactionRequestToBeneficiary(): Boolean = { -// val name = nameOf(populateViewDefinitionCanAddTransactionRequestToBeneficiary) -// runOnce(name) { -// MigrationOfViewDefinitionCanAddTransactionRequestToBeneficiary.populateTheField(name) -// } -// } - -// private def populateViewDefinitionCanSeeTransactionStatus(): Boolean = { -// val name = nameOf(populateViewDefinitionCanSeeTransactionStatus) -// runOnce(name) { -// MigrationOfViewDefinitionCanSeeTransactionStatus.populateTheField(name) -// } -// } - - -// private def populateMigrationOfViewDefinitionPermissions(startedBeforeSchemifier: Boolean): Boolean = { -// if (startedBeforeSchemifier == true) { -// logger.warn(s"Migration.database.populateMigrationOfViewDefinitionPermissions(true) cannot be run before Schemifier.") -// true -// } else { -// val name = nameOf(populateMigrationOfViewDefinitionPermissions(startedBeforeSchemifier)) -// runOnce(name) { -// MigrationOfViewDefinitionPermissions.populate(name) -// } -// } -// } -// - private def populateMigrationOfViewPermissions(startedBeforeSchemifier: Boolean): Boolean = { - if (startedBeforeSchemifier == true) { - logger.warn(s"Migration.database.populateMigrationOfViewPermissions(true) cannot be run before Schemifier.") - true - } else { - val name = nameOf(populateMigrationOfViewPermissions(startedBeforeSchemifier)) - runOnce(name) { - MigrationOfViewPermissions.populate(name) - } - } - } private def generateAndPopulateMissingCustomerUUIDs(startedBeforeSchemifier: Boolean): Boolean = { if(startedBeforeSchemifier == true) { @@ -797,6 +853,19 @@ object Migration extends MdcLoggable { } } } + def tableExistsByName(tableName: String): Boolean = { + DB.use(net.liftweb.util.DefaultConnectionIdentifier) { conn => + val md = conn.getMetaData + val schema = getDefaultSchemaName(conn) + using(md.getTables(null, schema, null, null)) { rs => + def check(): Boolean = + if (!rs.next) false + else if (rs.getString(3).toLowerCase == tableName.toLowerCase) true + else check() + check() + } + } + } /** * Declared max length of a (var)char column, via JDBC metadata (portable across H2/Postgres/MSSQL). @@ -821,6 +890,7 @@ object Migration extends MdcLoggable { } } } + /** * The purpose is to provide answer does a procedure exist at a database instance. */ diff --git a/obp-api/src/main/scala/code/api/util/migration/MigrationOfViewDefinitionCanAddTransactionRequestToBeneficiary.scala b/obp-api/src/main/scala/code/api/util/migration/MigrationOfViewDefinitionCanAddTransactionRequestToBeneficiary.scala deleted file mode 100644 index 8d4a11aa54..0000000000 --- a/obp-api/src/main/scala/code/api/util/migration/MigrationOfViewDefinitionCanAddTransactionRequestToBeneficiary.scala +++ /dev/null @@ -1,47 +0,0 @@ -//package code.api.util.migration -// -//import code.api.Constant.SYSTEM_OWNER_VIEW_ID -// -//import java.time.format.DateTimeFormatter -//import java.time.{ZoneId, ZonedDateTime} -//import code.api.util.APIUtil -//import code.api.util.migration.Migration.{DbFunction, saveLog} -//import code.model.Consumer -//import code.views.system.ViewDefinition -// -//object MigrationOfViewDefinitionCanAddTransactionRequestToBeneficiary { -// -// val oneDayAgo = ZonedDateTime.now(ZoneId.of("UTC")).minusDays(1) -// val oneYearInFuture = ZonedDateTime.now(ZoneId.of("UTC")).plusYears(1) -// val formatter = DateTimeFormatter.ofPattern("yyyy-MM-dd'T'HH:mm'Z'") -// -// def populateTheField(name: String): Boolean = { -// DbFunction.tableExists(ViewDefinition) match { -// case true => -// val startDate = System.currentTimeMillis() -// val commitId: String = APIUtil.gitCommit -// var isSuccessful = false -// -// val view = ViewDefinition.findSystemView(SYSTEM_OWNER_VIEW_ID).map(_.canAddTransactionRequestToBeneficiary_(true).saveMe()) -// -// -// val endDate = System.currentTimeMillis() -// val comment: String = -// s"""set $SYSTEM_OWNER_VIEW_ID.canAddTransactionRequestToBeneficiary_ to {true}""".stripMargin -// val value = view.map(_.canAddTransactionRequestToBeneficiary_.get).getOrElse(false) -// isSuccessful = value -// saveLog(name, commitId, isSuccessful, startDate, endDate, comment) -// isSuccessful -// -// case false => -// val startDate = System.currentTimeMillis() -// val commitId: String = APIUtil.gitCommit -// val isSuccessful = false -// val endDate = System.currentTimeMillis() -// val comment: String = -// s"""${Consumer._dbTableNameLC} table does not exist""".stripMargin -// saveLog(name, commitId, isSuccessful, startDate, endDate, comment) -// isSuccessful -// } -// } -//} diff --git a/obp-api/src/main/scala/code/api/util/migration/MigrationOfViewDefinitionCanSeeTransactionStatus.scala b/obp-api/src/main/scala/code/api/util/migration/MigrationOfViewDefinitionCanSeeTransactionStatus.scala deleted file mode 100644 index 894701af4f..0000000000 --- a/obp-api/src/main/scala/code/api/util/migration/MigrationOfViewDefinitionCanSeeTransactionStatus.scala +++ /dev/null @@ -1,80 +0,0 @@ -//package code.api.util.migration -// -//import code.api.Constant._ -//import code.api.util.APIUtil -//import code.api.util.migration.Migration.{DbFunction, saveLog} -//import code.model.Consumer -//import code.views.system.ViewDefinition -// -//import java.time.format.DateTimeFormatter -//import java.time.{ZoneId, ZonedDateTime} -// -//object MigrationOfViewDefinitionCanSeeTransactionStatus { -// -// val oneDayAgo = ZonedDateTime.now(ZoneId.of("UTC")).minusDays(1) -// val oneYearInFuture = ZonedDateTime.now(ZoneId.of("UTC")).plusYears(1) -// val formatter = DateTimeFormatter.ofPattern("yyyy-MM-dd'T'HH:mm'Z'") -// -// def populateTheField(name: String): Boolean = { -// DbFunction.tableExists(ViewDefinition) match { -// case true => -// val startDate = System.currentTimeMillis() -// val commitId: String = APIUtil.gitCommit -// var isSuccessful = false -// -// val view = ViewDefinition.findSystemView(SYSTEM_OWNER_VIEW_ID).map(_.canSeeTransactionStatus_(true).saveMe()) -// val view1 = ViewDefinition.findSystemView(SYSTEM_READ_TRANSACTIONS_BERLIN_GROUP_VIEW_ID).map(_.canSeeTransactionStatus_(true).saveMe()) -// val view2 = ViewDefinition.findSystemView(SYSTEM_READ_TRANSACTIONS_DETAIL_VIEW_ID).map(_.canSeeTransactionStatus_(true).saveMe()) -// val view3 = ViewDefinition.findSystemView(SYSTEM_READ_TRANSACTIONS_DEBITS_VIEW_ID).map(_.canSeeTransactionStatus_(true).saveMe()) -// val view4 = ViewDefinition.findSystemView(SYSTEM_READ_TRANSACTIONS_BASIC_VIEW_ID).map(_.canSeeTransactionStatus_(true).saveMe()) -// val view8 = ViewDefinition.findSystemView(SYSTEM_AUDITOR_VIEW_ID).map(_.canSeeTransactionStatus_(true).saveMe()) -// val view5 = ViewDefinition.findSystemView(SYSTEM_STAGE_ONE_VIEW_ID).map(_.canSeeTransactionStatus_(true).saveMe()) -// val view6 = ViewDefinition.findSystemView(SYSTEM_STANDARD_VIEW_ID).map(_.canSeeTransactionStatus_(true).saveMe()) -// val view7 = ViewDefinition.findSystemView(SYSTEM_FIREHOSE_VIEW_ID).map(_.canSeeTransactionStatus_(true).saveMe()) -// val view9 = ViewDefinition.findSystemView(SYSTEM_ACCOUNTANT_VIEW_ID).map(_.canSeeTransactionStatus_(true).saveMe()) -// val view10 = ViewDefinition.findSystemView(SYSTEM_MANAGE_CUSTOM_VIEWS_VIEW_ID).map(_.canSeeTransactionStatus_(true).saveMe()) -// -// -// val endDate = System.currentTimeMillis() -// val comment: String = -// s"""set $SYSTEM_OWNER_VIEW_ID.canSeeTransactionStatus_ to {true}; -// |set $SYSTEM_READ_TRANSACTIONS_BERLIN_GROUP_VIEW_ID.canSeeTransactionStatus_ to {true} -// |set $SYSTEM_READ_TRANSACTIONS_DETAIL_VIEW_ID.canSeeTransactionStatus_ to {true}; -// |set $SYSTEM_READ_TRANSACTIONS_DEBITS_VIEW_ID.canSeeTransactionStatus_ to {true}; -// |set $SYSTEM_READ_TRANSACTIONS_BASIC_VIEW_ID.canSeeTransactionStatus_ to {true}; -// |set $SYSTEM_AUDITOR_VIEW_ID.canSeeTransactionStatus_ to {true}; -// |set $SYSTEM_STAGE_ONE_VIEW_ID.canSeeTransactionStatus_ to {true}; -// |set $SYSTEM_STANDARD_VIEW_ID.canSeeTransactionStatus_ to {true}; -// |set $SYSTEM_FIREHOSE_VIEW_ID.canSeeTransactionStatus_ to {true}; -// |set $SYSTEM_ACCOUNTANT_VIEW_ID.canSeeTransactionStatus_ to {true}; -// |set $SYSTEM_MANAGE_CUSTOM_VIEWS_VIEW_ID.canSeeTransactionStatus_ to {true}; -// |""".stripMargin -// val value = view.map(_.canSeeTransactionStatus_.get).getOrElse(false) -// val value1 = view1.map(_.canSeeTransactionStatus_.get).getOrElse(false) -// val value2 = view1.map(_.canSeeTransactionStatus_.get).getOrElse(false) -// val value3 = view3.map(_.canSeeTransactionStatus_.get).getOrElse(false) -// val value4 = view4.map(_.canSeeTransactionStatus_.get).getOrElse(false) -// val value5 = view5.map(_.canSeeTransactionStatus_.get).getOrElse(false) -// val value6 = view6.map(_.canSeeTransactionStatus_.get).getOrElse(false) -// val value7 = view7.map(_.canSeeTransactionStatus_.get).getOrElse(false) -// val value8 = view8.map(_.canSeeTransactionStatus_.get).getOrElse(false) -// val value9 = view9.map(_.canSeeTransactionStatus_.get).getOrElse(false) -// val value10 = view10.map(_.canSeeTransactionStatus_.get).getOrElse(false) -// -// isSuccessful = value && value1 && value2 && value3 && value4 && value5 && value6 && value7 && value8 && value9 && value10 -// -// saveLog(name, commitId, isSuccessful, startDate, endDate, comment) -// isSuccessful -// -// case false => -// val startDate = System.currentTimeMillis() -// val commitId: String = APIUtil.gitCommit -// val isSuccessful = false -// val endDate = System.currentTimeMillis() -// val comment: String = -// s"""${Consumer._dbTableNameLC} table does not exist""".stripMargin -// saveLog(name, commitId, isSuccessful, startDate, endDate, comment) -// isSuccessful -// } -// } -//} diff --git a/obp-api/src/main/scala/code/api/util/migration/MigrationOfViewDefinitionPermissions.scala b/obp-api/src/main/scala/code/api/util/migration/MigrationOfViewDefinitionPermissions.scala deleted file mode 100644 index 2499248a10..0000000000 --- a/obp-api/src/main/scala/code/api/util/migration/MigrationOfViewDefinitionPermissions.scala +++ /dev/null @@ -1,97 +0,0 @@ -//package code.api.util.migration -// -//import code.api.Constant.{DEFAULT_CAN_GRANT_AND_REVOKE_ACCESS_TO_VIEWS, SYSTEM_OWNER_VIEW_ID, SYSTEM_STANDARD_VIEW_ID} -//import code.api.util.APIUtil -//import code.api.util.migration.Migration.{DbFunction, saveLog} -//import code.views.system.ViewDefinition -//import net.liftweb.mapper.{By, DB, NullRef} -//import net.liftweb.util.DefaultConnectionIdentifier -// -//object MigrationOfViewDefinitionPermissions { -// def populate(name: String): Boolean = { -// DbFunction.tableExists(ViewDefinition) match { -// case true => -// val startDate = System.currentTimeMillis() -// val commitId: String = APIUtil.gitCommit -// val ownerView = ViewDefinition.find( -// NullRef(ViewDefinition.bank_id), -// NullRef(ViewDefinition.account_id), -// By(ViewDefinition.view_id, SYSTEM_OWNER_VIEW_ID), -// By(ViewDefinition.isSystem_,true) -// ).map(view => -// view -// .canSeeTransactionRequestTypes_(true) -// .canSeeTransactionRequests_(true) -// .canSeeAvailableViewsForBankAccount_(true) -// .canUpdateBankAccountLabel_(true) -// .canSeeViewsWithPermissionsForOneUser_(true) -// .canSeeViewsWithPermissionsForAllUsers_(true) -// .canCreateCustomView_(false) -// .canDeleteCustomView_(false) -// .canUpdateCustomView_(false) -// .canGrantAccessToCustomViews_(false) -// .canRevokeAccessToCustomViews_(false) -// .canGrantAccessToViews_(DEFAULT_CAN_GRANT_AND_REVOKE_ACCESS_TO_VIEWS.mkString(",")) -// .canRevokeAccessToViews_(DEFAULT_CAN_GRANT_AND_REVOKE_ACCESS_TO_VIEWS.mkString(",")) -// .save -// ) -// -// val standardView = ViewDefinition.find( -// NullRef(ViewDefinition.bank_id), -// NullRef(ViewDefinition.account_id), -// By(ViewDefinition.view_id, SYSTEM_STANDARD_VIEW_ID), -// By(ViewDefinition.isSystem_,true) -// ).map(view => -// view -// .canSeeTransactionRequestTypes_(true) -// .canSeeTransactionRequests_(true) -// .canSeeAvailableViewsForBankAccount_(true) -// .canUpdateBankAccountLabel_(true) -// .canSeeViewsWithPermissionsForOneUser_(true) -// .canSeeViewsWithPermissionsForAllUsers_(true) -// .canCreateCustomView_(false) -// .canDeleteCustomView_(false) -// .canUpdateCustomView_(false) -// .canGrantAccessToCustomViews_(false) -// .canRevokeAccessToCustomViews_(false) -// .canGrantAccessToViews_(DEFAULT_CAN_GRANT_AND_REVOKE_ACCESS_TO_VIEWS.mkString(",")) -// .canRevokeAccessToViews_(DEFAULT_CAN_GRANT_AND_REVOKE_ACCESS_TO_VIEWS.mkString(",")) -// .save -// ) -// -// -// val isSuccessful = ownerView.isDefined && standardView.isDefined -// val endDate = System.currentTimeMillis() -// -// val comment: String = -// s"""ViewDefinition system $SYSTEM_OWNER_VIEW_ID and $SYSTEM_STANDARD_VIEW_ID views, update the following rows to true: -// |${ViewDefinition.canSeeTransactionRequestTypes_.dbColumnName} -// |${ViewDefinition.canSeeTransactionRequests_.dbColumnName} -// |${ViewDefinition.canSeeAvailableViewsForBankAccount_.dbColumnName} -// |${ViewDefinition.canUpdateBankAccountLabel_.dbColumnName} -// |${ViewDefinition.canCreateCustomView_.dbColumnName} -// |${ViewDefinition.canDeleteCustomView_.dbColumnName} -// |${ViewDefinition.canUpdateCustomView_.dbColumnName} -// |${ViewDefinition.canSeeViewsWithPermissionsForAllUsers_.dbColumnName} -// |${ViewDefinition.canSeeViewsWithPermissionsForOneUser_.dbColumnName} -// |${ViewDefinition.canGrantAccessToCustomViews_.dbColumnName} -// |${ViewDefinition.canRevokeAccessToCustomViews_.dbColumnName} -// |${ViewDefinition.canGrantAccessToViews_.dbColumnName} -// |${ViewDefinition.canRevokeAccessToViews_.dbColumnName} -// |Duration: ${endDate - startDate} ms; -// """.stripMargin -// saveLog(name, commitId, isSuccessful, startDate, endDate, comment) -// isSuccessful -// -// case false => -// val startDate = System.currentTimeMillis() -// val commitId: String = APIUtil.gitCommit -// val isSuccessful = false -// val endDate = System.currentTimeMillis() -// val comment: String = -// s"""ViewDefinition does not exist!""".stripMargin -// saveLog(name, commitId, isSuccessful, startDate, endDate, comment) -// isSuccessful -// } -// } -//} diff --git a/obp-api/src/main/scala/code/api/util/migration/MigrationOfViewPermissions.scala b/obp-api/src/main/scala/code/api/util/migration/MigrationOfViewPermissions.scala deleted file mode 100644 index 13102f0e1a..0000000000 --- a/obp-api/src/main/scala/code/api/util/migration/MigrationOfViewPermissions.scala +++ /dev/null @@ -1,38 +0,0 @@ -package code.api.util.migration - -import code.api.util.APIUtil -import code.api.util.migration.Migration.{DbFunction, saveLog} -import code.views.MapperViews -import code.views.system.{ViewDefinition, ViewPermission} - -object MigrationOfViewPermissions { - def populate(name: String): Boolean = { - DbFunction.tableExists(ViewDefinition) && DbFunction.tableExists(ViewPermission)match { - case true => - val startDate = System.currentTimeMillis() - val commitId: String = APIUtil.gitCommit - - val allViewDefinitions = ViewDefinition.findAll() - val viewPermissionRowNumberBefore = ViewPermission.count - allViewDefinitions.map(v => MapperViews.migrateViewPermissions(v)) - val viewPermissionRowNumberAfter = ViewPermission.count - - val isSuccessful = true - val endDate = System.currentTimeMillis() - - val comment: String = s"""migrate all permissions from ViewDefinition (${allViewDefinitions.length} rows) to ViewPermission (${viewPermissionRowNumberAfter-viewPermissionRowNumberBefore} added) .""".stripMargin - saveLog(name, commitId, isSuccessful, startDate, endDate, comment) - isSuccessful - - case false => - val startDate = System.currentTimeMillis() - val commitId: String = APIUtil.gitCommit - val isSuccessful = false - val endDate = System.currentTimeMillis() - val comment: String = - s"""ViewDefinition or ViewPermission does not exist!""".stripMargin - saveLog(name, commitId, isSuccessful, startDate, endDate, comment) - isSuccessful - } - } -} diff --git a/obp-api/src/main/scala/code/api/v4_0_0/Http4s400.scala b/obp-api/src/main/scala/code/api/v4_0_0/Http4s400.scala index 1eaa92c6a1..2499d1ae1e 100644 --- a/obp-api/src/main/scala/code/api/v4_0_0/Http4s400.scala +++ b/obp-api/src/main/scala/code/api/v4_0_0/Http4s400.scala @@ -3474,6 +3474,10 @@ object Http4s400 { } _ <- NewStyle.function.checkAuthorisationToCreateTransactionRequest( viewId, BankIdAccountId(fromAccount.bankId, fromAccount.accountId), user, Some(cc)) + // Lock the transaction request row before fetching to prevent Double-Spend MFA bypass + _ <- code.util.Helper.booleanToFuture("Failed to acquire transaction request lock", cc = Some(cc)) { + code.bankconnectors.DoobieTransactionRequestQueries.lockTransactionRequest(transReqId.value).isDefined + } (existingTransactionRequest, _) <- NewStyle.function.getTransactionRequestImpl(transReqId, Some(cc)) _ <- code.util.Helper.booleanToFuture( TransactionRequestStatusNotInitiatedOrPendingOrForwarded, cc = Some(cc)) { 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 d3cd4deeb9..512ed6a8b9 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 @@ -7823,7 +7823,7 @@ object Http4s600 { |- Generates a unique password reset token (rotates the user's uniqueId) |- Builds a reset URL using the portal_external_url property |- Sends the URL to the user by email - |- Returns only delivery acknowledgement ({"status": "sent", "to": ""}) + |- Returns only delivery acknowledgement ({"status": "sent", "to": "user@example.com"}) | |Required fields: |- username: The user's username (typically email) @@ -12802,7 +12802,7 @@ object Http4s600 { |The backup entity will be named with a _BAK suffix (e.g. my_entity_BAK). |If a backup with that name already exists, _BAK2, _BAK3 etc. will be used. | - |The calling user will be granted CanGetDynamicEntity_`` on the newly created backup entity. + |The calling user will be granted CanGetDynamicEntity_`{BackupEntityName}` on the newly created backup entity. | |For more information see ${Glossary.getGlossaryItemLink("Dynamic-Entities")} | @@ -12835,7 +12835,7 @@ object Http4s600 { |The backup entity will be named with a _BAK suffix (e.g. my_entity_BAK). |If a backup with that name already exists, _BAK2, _BAK3 etc. will be used. | - |The calling user will be granted CanGetDynamicEntity_`` on the newly created backup entity. + |The calling user will be granted CanGetDynamicEntity_`{BackupEntityName}` on the newly created backup entity. | |For more information see ${Glossary.getGlossaryItemLink("Dynamic-Entities")} | diff --git a/obp-api/src/main/scala/code/bankconnectors/DoobieBadLoginAttemptQueries.scala b/obp-api/src/main/scala/code/bankconnectors/DoobieBadLoginAttemptQueries.scala new file mode 100644 index 0000000000..8179e66537 --- /dev/null +++ b/obp-api/src/main/scala/code/bankconnectors/DoobieBadLoginAttemptQueries.scala @@ -0,0 +1,23 @@ +package code.bankconnectors + +import code.api.util.DoobieUtil +import doobie._ +import doobie.implicits._ + +object DoobieBadLoginAttemptQueries { + + private def atomicIncrement(provider: String, username: String): ConnectionIO[Int] = + for { + _ <- sql"""SELECT mbadattemptssincelastsuccessorreset + FROM mappedbadloginattempt + WHERE provider = $provider AND musername = $username + FOR UPDATE""".query[Int].option + rows <- sql"""UPDATE mappedbadloginattempt + SET mbadattemptssincelastsuccessorreset = mbadattemptssincelastsuccessorreset + 1, + mlastfailuredate = NOW() + WHERE provider = $provider AND musername = $username""".update.run + } yield rows + + def incrementBadLoginAttempts(provider: String, username: String): Int = + DoobieUtil.runUpdate(atomicIncrement(provider, username)) +} diff --git a/obp-api/src/main/scala/code/bankconnectors/DoobieBankAccountQueries.scala b/obp-api/src/main/scala/code/bankconnectors/DoobieBankAccountQueries.scala new file mode 100644 index 0000000000..f862d78506 --- /dev/null +++ b/obp-api/src/main/scala/code/bankconnectors/DoobieBankAccountQueries.scala @@ -0,0 +1,36 @@ +package code.bankconnectors + +import code.api.util.DoobieUtil +import doobie._ +import doobie.implicits._ +import net.liftweb.common.Box +import net.liftweb.util.Helpers.tryo + +object DoobieBankAccountQueries { + + /** + * Atomically updates the bank account balance using a database row lock (SELECT FOR UPDATE). + * + * @param bankId The bank ID + * @param accountId The account ID + * @param amount The amount to add (can be negative for deductions) + * @return The new balance after the update + */ + def atomicallyUpdateBalance(bankId: String, accountId: String, amount: Long): ConnectionIO[Long] = { + for { + // 1. Lock the row and get the current balance + currentBalance <- sql"SELECT accountbalance FROM mappedbankaccount WHERE bank = $bankId AND theaccountid = $accountId FOR UPDATE".query[Long].unique + + newBalance = currentBalance + amount + + // 2. Update the row with the new balance + _ <- sql"UPDATE mappedbankaccount SET accountbalance = $newBalance WHERE bank = $bankId AND theaccountid = $accountId".update.run + } yield newBalance + } + + def updateBalance(bankId: String, accountId: String, amount: Long): Box[Long] = { + tryo { + DoobieUtil.runUpdate(atomicallyUpdateBalance(bankId, accountId, amount)) + } + } +} diff --git a/obp-api/src/main/scala/code/bankconnectors/DoobieChallengeQueries.scala b/obp-api/src/main/scala/code/bankconnectors/DoobieChallengeQueries.scala new file mode 100644 index 0000000000..73871f7c76 --- /dev/null +++ b/obp-api/src/main/scala/code/bankconnectors/DoobieChallengeQueries.scala @@ -0,0 +1,26 @@ +package code.bankconnectors + +import code.api.util.DoobieUtil +import doobie._ +import doobie.implicits._ +import net.liftweb.common.Box +import net.liftweb.util.Helpers.tryo + +object DoobieChallengeQueries { + + private def incrementAndSelectCounter(challengeId: String): ConnectionIO[Int] = + for { + _ <- sql"""SELECT attemptcounter + FROM ExpectedChallengeAnswer + WHERE challengeid = $challengeId + FOR UPDATE""".query[Int].option + _ <- sql"""UPDATE ExpectedChallengeAnswer + SET attemptcounter = attemptcounter + 1 + WHERE challengeid = $challengeId""".update.run + counter <- sql"""SELECT attemptcounter FROM ExpectedChallengeAnswer + WHERE challengeid = $challengeId""".query[Int].unique + } yield counter + + def incrementAndGetChallengeCounter(challengeId: String): Int = + DoobieUtil.runUpdate(incrementAndSelectCounter(challengeId)) +} diff --git a/obp-api/src/main/scala/code/bankconnectors/DoobieConsentSchedulerQueries.scala b/obp-api/src/main/scala/code/bankconnectors/DoobieConsentSchedulerQueries.scala new file mode 100644 index 0000000000..134fced6b3 --- /dev/null +++ b/obp-api/src/main/scala/code/bankconnectors/DoobieConsentSchedulerQueries.scala @@ -0,0 +1,34 @@ +package code.bankconnectors + +import code.api.util.DoobieUtil +import doobie._ +import doobie.implicits._ + +object DoobieConsentSchedulerQueries { + + def conditionallyUpdateStatus( + consentRowId: Long, + guardStatus: String, + newStatus: String, + newNote: String + ): Int = DoobieUtil.runUpdate( + sql"""UPDATE mappedconsent + SET mstatus = $newStatus, + mnote = $newNote, + mstatusupdatedatetime = NOW() + WHERE id = $consentRowId + AND mstatus = $guardStatus""".update.run + ) + + def conditionallyExpireValidBerlinGroupConsent( + consentRowId: Long, + newNote: String + ): Int = DoobieUtil.runUpdate( + sql"""UPDATE mappedconsent + SET mstatus = ${"expired"}, + mnote = $newNote, + mstatusupdatedatetime = NOW() + WHERE id = $consentRowId + AND mstatus IN ('valid', 'VALID')""".update.run + ) +} diff --git a/obp-api/src/main/scala/code/bankconnectors/DoobieTransactionRequestQueries.scala b/obp-api/src/main/scala/code/bankconnectors/DoobieTransactionRequestQueries.scala new file mode 100644 index 0000000000..80cb02a548 --- /dev/null +++ b/obp-api/src/main/scala/code/bankconnectors/DoobieTransactionRequestQueries.scala @@ -0,0 +1,25 @@ +package code.bankconnectors + +import code.api.util.DoobieUtil +import doobie._ +import doobie.implicits._ +import net.liftweb.common.Box +import net.liftweb.util.Helpers.tryo + +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. + */ + def atomicallyLockTransactionRequest(transReqId: String): ConnectionIO[String] = { + sql"SELECT mstatus FROM mappedtransactionrequest WHERE mtransactionrequestid = $transReqId FOR UPDATE".query[String].unique + } + + def lockTransactionRequest(transReqId: String): Box[String] = { + tryo { + DoobieUtil.runUpdate(atomicallyLockTransactionRequest(transReqId)) + } + } +} diff --git a/obp-api/src/main/scala/code/bankconnectors/LocalMappedConnector.scala b/obp-api/src/main/scala/code/bankconnectors/LocalMappedConnector.scala index 1a51cc8c33..12992ceb84 100644 --- a/obp-api/src/main/scala/code/bankconnectors/LocalMappedConnector.scala +++ b/obp-api/src/main/scala/code/bankconnectors/LocalMappedConnector.scala @@ -2316,11 +2316,12 @@ object LocalMappedConnector extends Connector with MdcLoggable { ): Box[TransactionId] = for { currency <- Full(fromAccount.currency) - //update the balance of the fromAccount for which a transaction is being created - newAccountBalance <- Full(Helper.convertToSmallestCurrencyUnits(fromAccount.balance, currency) + Helper.convertToSmallestCurrencyUnits(amount, currency)) - - //Here is the `LocalMappedConnector`, once get this point, fromAccount must be a mappedBankAccount. So can use asInstanceOf.... - _ <- tryo(fromAccount.asInstanceOf[MappedBankAccount].accountBalance(newAccountBalance).save) ?~! UpdateBankAccountException + // atomically update the balance using Doobie and SELECT FOR UPDATE row locking + newAccountBalance <- DoobieBankAccountQueries.updateBalance( + fromAccount.bankId.value, + fromAccount.accountId.value, + Helper.convertToSmallestCurrencyUnits(amount, currency) + ) ?~! UpdateBankAccountException mappedTransaction <- tryo(MappedTransaction.create .bank(fromAccount.bankId.value) diff --git a/obp-api/src/main/scala/code/bankconnectors/LocalMappedConnectorInternal.scala b/obp-api/src/main/scala/code/bankconnectors/LocalMappedConnectorInternal.scala index 7c9540a314..9d05ca346c 100644 --- a/obp-api/src/main/scala/code/bankconnectors/LocalMappedConnectorInternal.scala +++ b/obp-api/src/main/scala/code/bankconnectors/LocalMappedConnectorInternal.scala @@ -508,11 +508,12 @@ object LocalMappedConnectorInternal extends MdcLoggable { for { currency <- Full(fromAccount.currency) - //update the balance of the fromAccount for which a transaction is being created - newAccountBalance <- Full(Helper.convertToSmallestCurrencyUnits(fromAccount.balance, currency) + Helper.convertToSmallestCurrencyUnits(amount, currency)) - - //Here is the `LocalMappedConnector`, once get this point, fromAccount must be a mappedBankAccount. So can use asInstanceOf.... - _ <- tryo(fromAccount.asInstanceOf[MappedBankAccount].accountBalance(newAccountBalance).save) ?~! UpdateBankAccountException + // atomically update the balance using Doobie and SELECT FOR UPDATE row locking + newAccountBalance <- DoobieBankAccountQueries.updateBalance( + fromAccount.bankId.value, + fromAccount.accountId.value, + Helper.convertToSmallestCurrencyUnits(amount, currency) + ) ?~! UpdateBankAccountException mappedTransaction <- tryo(MappedTransaction.create //No matter which type (SANDBOX_TAN,SEPA,FREE_FORM,COUNTERPARTYE), always filled the following nine fields. diff --git a/obp-api/src/main/scala/code/customer/internalMapping/MappedCustomerIdMappingProvider.scala b/obp-api/src/main/scala/code/customer/internalMapping/MappedCustomerIdMappingProvider.scala index 1404bb03ee..a6e7817a31 100644 --- a/obp-api/src/main/scala/code/customer/internalMapping/MappedCustomerIdMappingProvider.scala +++ b/obp-api/src/main/scala/code/customer/internalMapping/MappedCustomerIdMappingProvider.scala @@ -4,6 +4,7 @@ import code.util.Helper.MdcLoggable import com.openbankproject.commons.model.{BankId, CustomerId} import net.liftweb.common._ import net.liftweb.mapper.By +import net.liftweb.util.Helpers.tryo object MappedCustomerIdMappingProvider extends CustomerIdMappingProvider with MdcLoggable @@ -26,15 +27,22 @@ object MappedCustomerIdMappingProvider extends CustomerIdMappingProvider with Md mappedCustomerIdMapping.map(_.customerId) } case Empty => - { - val mappedCustomerIdMapping: MappedCustomerIdMapping = + tryo { MappedCustomerIdMapping .create .mCustomerPlainTextReference(customerPlainTextReference) .saveMe - logger.debug(s"getOrCreateCustomerId--> create mappedCustomerIdMapping : $mappedCustomerIdMapping") - Full(mappedCustomerIdMapping.customerId) - } + } match { + case Full(m) => + logger.debug(s"getOrCreateCustomerId--> create mappedCustomerIdMapping : $m") + Full(m.customerId) + case Failure(_, _, _) => + // UniqueIndex violation from concurrent insert — re-fetch the committed row + MappedCustomerIdMapping.find( + By(MappedCustomerIdMapping.mCustomerPlainTextReference, customerPlainTextReference) + ).map(_.customerId) + case other => other.map(_.customerId) + } case Failure(msg, t, c) => Failure(msg, t, c) case ParamFailure(x,y,z,q) => ParamFailure(x,y,z,q) } diff --git a/obp-api/src/main/scala/code/entitlement/MappedEntitlements.scala b/obp-api/src/main/scala/code/entitlement/MappedEntitlements.scala index 70c2be69a8..653f787593 100644 --- a/obp-api/src/main/scala/code/entitlement/MappedEntitlements.scala +++ b/obp-api/src/main/scala/code/entitlement/MappedEntitlements.scala @@ -9,6 +9,7 @@ import code.api.util.{ErrorMessages, NotificationUtil} import code.util.{MappedUUID, UUIDString} import net.liftweb.common.{Box, Failure, Full} import net.liftweb.mapper._ +import net.liftweb.util.Helpers.tryo import scala.concurrent.Future import com.openbankproject.commons.ExecutionContext.Implicits.global @@ -165,7 +166,7 @@ object MappedEntitlementsProvider extends EntitlementProvider { groupId: Option[String] = None, process: Option[String] = None ): Box[Entitlement] = { - def addEntitlementToUser(): Full[MappedEntitlement] = { + def addEntitlementToUser(): Box[MappedEntitlement] = { val entitlement = MappedEntitlement.create .mBankId(bankId) .mUserId(userId) @@ -173,13 +174,19 @@ object MappedEntitlementsProvider extends EntitlementProvider { .mCreatedByProcess(createdByProcess) groupId.foreach(gid => entitlement.mGroupId(gid)) process.foreach(p => entitlement.mProcess(p)) - val addEntitlement = entitlement.saveMe() - // When a role is Granted, we should send an email to the Recipient telling them they have been granted the role. - NotificationUtil.sendEmailRegardingAssignedRole( - userId: String, - addEntitlement: Entitlement - ) - Full(addEntitlement) + tryo(entitlement.saveMe()) match { + case Full(saved) => + NotificationUtil.sendEmailRegardingAssignedRole(userId, saved) + Full(saved) + case Failure(_, _, _) => + // UniqueIndex(mBankId, mUserId, mRoleName) violated by concurrent grant — return the committed row + MappedEntitlement.find( + By(MappedEntitlement.mBankId, bankId), + By(MappedEntitlement.mUserId, userId), + By(MappedEntitlement.mRoleName, roleName) + ) + case other => other + } } // Return a Box so we can handle errors later. grantorUserId match { @@ -261,5 +268,5 @@ class MappedEntitlement object MappedEntitlement extends MappedEntitlement with LongKeyedMetaMapper[MappedEntitlement] { - override def dbIndexes = UniqueIndex(mEntitlementId) :: super.dbIndexes + override def dbIndexes = UniqueIndex(mEntitlementId) :: UniqueIndex(mBankId, mUserId, mRoleName) :: super.dbIndexes } diff --git a/obp-api/src/main/scala/code/loginattempts/LoginAttempts.scala b/obp-api/src/main/scala/code/loginattempts/LoginAttempts.scala index 5acd98d7f7..03a918e4c2 100644 --- a/obp-api/src/main/scala/code/loginattempts/LoginAttempts.scala +++ b/obp-api/src/main/scala/code/loginattempts/LoginAttempts.scala @@ -3,7 +3,7 @@ package code.loginattempts import code.api.util.APIUtil import code.userlocks.UserLocksProvider import code.util.Helper.MdcLoggable -import net.liftweb.common.{Box, Empty, Full} +import net.liftweb.common.{Box, Empty, Failure, Full} import net.liftweb.mapper.By import net.liftweb.util.Helpers._ @@ -18,30 +18,22 @@ object LoginAttempt extends MdcLoggable { case false => logger.debug(s"Hello from incrementBadLoginAttempts with $username") - // Find badLoginAttempt record if one exists for a user - MappedBadLoginAttempt.find( - By(MappedBadLoginAttempt.Provider, provider), - By(MappedBadLoginAttempt.mUsername, username) - ) match { - // If it exits update the date and increment - case Full(loginAttempt) => - - logger.debug(s"incrementBadLoginAttempts found ${loginAttempt.mBadAttemptsSinceLastSuccessOrReset} loginAttempt(s) with id ${loginAttempt.id}") - - loginAttempt - .mLastFailureDate(now) - .mBadAttemptsSinceLastSuccessOrReset(loginAttempt.mBadAttemptsSinceLastSuccessOrReset + 1) // Increment - .save - case _ => - // If none exists, add one + // Atomically increment the counter; if no row exists yet, create one. + // The create path is itself a check-then-insert: two concurrent first-time bad logins both + // see rowsUpdated==0, so wrap in tryo to absorb the UniqueIndex violation from the loser. + val rowsUpdated = code.bankconnectors.DoobieBadLoginAttemptQueries.incrementBadLoginAttempts(provider, username) + if (rowsUpdated == 0) { + tryo { MappedBadLoginAttempt.create .mUsername(username) .Provider(provider) .mLastFailureDate(now) - .mBadAttemptsSinceLastSuccessOrReset(1) // Start with 1 + .mBadAttemptsSinceLastSuccessOrReset(1) .save - - logger.debug(s"incrementBadLoginAttempts created loginAttempt") + } + logger.debug(s"incrementBadLoginAttempts created loginAttempt") + } else { + logger.debug(s"incrementBadLoginAttempts atomically incremented for $username (rows=$rowsUpdated)") } } } @@ -50,13 +42,29 @@ object LoginAttempt extends MdcLoggable { MappedBadLoginAttempt.find( By(MappedBadLoginAttempt.Provider, provider), By(MappedBadLoginAttempt.mUsername, username) - ).or(Full(MappedBadLoginAttempt.create - .mUsername(username) - .Provider(provider) - .mLastFailureDate(now) - .mBadAttemptsSinceLastSuccessOrReset(0) - .saveMe() - )) + ) match { + case full @ Full(_) => full + case _ => + // .or(Full(saveMe())) evaluates saveMe eagerly — two concurrent first-time callers + // both get Empty and both call saveMe; the loser hits UniqueIndex(Provider, mUsername). + tryo { + MappedBadLoginAttempt.create + .mUsername(username) + .Provider(provider) + .mLastFailureDate(now) + .mBadAttemptsSinceLastSuccessOrReset(0) + .saveMe() + } match { + case full @ Full(_) => full + case Failure(_, _, _) => + // UniqueIndex violation from concurrent insert — re-fetch the committed row + MappedBadLoginAttempt.find( + By(MappedBadLoginAttempt.Provider, provider), + By(MappedBadLoginAttempt.mUsername, username) + ) + case other => other + } + } } /** diff --git a/obp-api/src/main/scala/code/metadata/counterparties/MapperCounterparties.scala b/obp-api/src/main/scala/code/metadata/counterparties/MapperCounterparties.scala index 80cbc903ac..a58de6d018 100644 --- a/obp-api/src/main/scala/code/metadata/counterparties/MapperCounterparties.scala +++ b/obp-api/src/main/scala/code/metadata/counterparties/MapperCounterparties.scala @@ -8,7 +8,7 @@ import code.util.Helper.MdcLoggable import code.util._ import com.openbankproject.commons.model._ import com.tesobe.CacheKeyFromArguments -import net.liftweb.common.{Box, Full} +import net.liftweb.common.{Box, Failure, Full} import net.liftweb.mapper._ import net.liftweb.util.Helpers.tryo import net.liftweb.util.StringHelpers @@ -75,15 +75,20 @@ object MapperCounterparties extends Counterparties with MdcLoggable { // Create it! case _ => { logger.debug(s"getOrCreateMetadata--Create MappedCounterpartyMetadata counterpartyId($counterpartyId)") - // Store a record that contains counterparty information from the perspective of an account at a bank - Full(MappedCounterpartyMetadata.create - // Core info - .counterpartyId(counterpartyId) - .thisBankId(bankId.value) - .thisAccountId(accountId.value) - .counterpartyName(counterpartyName) - .publicAlias(newPublicAliasName()) // The public alias this account gives to the counterparty. - .saveMe) + tryo { + MappedCounterpartyMetadata.create + .counterpartyId(counterpartyId) + .thisBankId(bankId.value) + .thisAccountId(accountId.value) + .counterpartyName(counterpartyName) + .publicAlias(newPublicAliasName()) + .saveMe + } match { + case Full(created) => Full(created) + case Failure(_, _, _) => + findMappedCounterpartyMetadataById(counterpartyId) + case other => other + } } } } diff --git a/obp-api/src/main/scala/code/model/OAuth.scala b/obp-api/src/main/scala/code/model/OAuth.scala index b7ccca55a6..964e1ff970 100644 --- a/obp-api/src/main/scala/code/model/OAuth.scala +++ b/obp-api/src/main/scala/code/model/OAuth.scala @@ -533,9 +533,18 @@ object MappedConsumersProvider extends ConsumersProvider with MdcLoggable { } c.consumerId(actualConsumerId) val createdConsumer = c.saveMe() - // In case we use Hydra ORY as Identity Provider we create corresponding client at Hydra side a well if(integrateWithHydra) createHydraClient(createdConsumer) createdConsumer + } match { + case Full(c) => Full(c) + case Failure(_, _, _) => + // UniqueIndex violated by concurrent insert — re-fetch using the most specific available key. + // Searching by (azp="", sub="") when both are absent would match unrelated consumers. + (azp, sub) match { + case (Some(a), Some(s)) => Consumer.find(By(Consumer.azp, a), By(Consumer.sub, s)) + case _ => key.flatMap(k => Consumer.find(By(Consumer.key, k))) + } + case other => other } } } diff --git a/obp-api/src/main/scala/code/model/dataAccess/internalMapping/MappedAccountIdMappingProvider.scala b/obp-api/src/main/scala/code/model/dataAccess/internalMapping/MappedAccountIdMappingProvider.scala index bd2dc4e7c7..7c4aa95cc0 100644 --- a/obp-api/src/main/scala/code/model/dataAccess/internalMapping/MappedAccountIdMappingProvider.scala +++ b/obp-api/src/main/scala/code/model/dataAccess/internalMapping/MappedAccountIdMappingProvider.scala @@ -4,6 +4,7 @@ import code.util.Helper.MdcLoggable import com.openbankproject.commons.model.{BankId, AccountId} import net.liftweb.common._ import net.liftweb.mapper.By +import net.liftweb.util.Helpers.tryo object MappedAccountIdMappingProvider extends AccountIdMappingProvider with MdcLoggable @@ -26,15 +27,22 @@ object MappedAccountIdMappingProvider extends AccountIdMappingProvider with MdcL mappedAccountIdMapping.map(_.accountId) } case Empty => - { - val mappedAccountIdMapping: AccountIdMapping = + tryo { AccountIdMapping .create .mAccountPlainTextReference(accountPlainTextReference) .saveMe - logger.debug(s"getOrCreateAccountId--> create mappedAccountIdMapping : $mappedAccountIdMapping") - Full(mappedAccountIdMapping.accountId) - } + } match { + case Full(m) => + logger.debug(s"getOrCreateAccountId--> create mappedAccountIdMapping : $m") + Full(m.accountId) + case Failure(_, _, _) => + // UniqueIndex violation from concurrent insert — re-fetch the committed row + AccountIdMapping.find( + By(AccountIdMapping.mAccountPlainTextReference, accountPlainTextReference) + ).map(_.accountId) + case other => other.map(_.accountId) + } case Failure(msg, t, c) => Failure(msg, t, c) case ParamFailure(x,y,z,q) => ParamFailure(x,y,z,q) } diff --git a/obp-api/src/main/scala/code/scheduler/ConsentScheduler.scala b/obp-api/src/main/scala/code/scheduler/ConsentScheduler.scala index f52cfe5c4e..205e9e5b0d 100644 --- a/obp-api/src/main/scala/code/scheduler/ConsentScheduler.scala +++ b/obp-api/src/main/scala/code/scheduler/ConsentScheduler.scala @@ -69,13 +69,15 @@ object ConsentScheduler extends MdcLoggable { outdatedConsents.foreach { consent => Try { 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("") // Prepend to existing note if any - consent - .mStatus(ConsentStatus.rejected.toString) - .mNote(newNote) - .mStatusUpdateDateTime(new Date()) - .save - logger.warn(message) + val newNote = s"$currentDate\n$message\n" + Option(consent.note).getOrElse("") + val rows = code.bankconnectors.DoobieConsentSchedulerQueries.conditionallyUpdateStatus( + consentRowId = consent.id.get, + guardStatus = ConsentStatus.received.toString, + newStatus = ConsentStatus.rejected.toString, + newNote = newNote + ) + if (rows > 0) logger.warn(message) + else logger.debug(s"|---> Skipped stale update for consent ${consent.id}: status already changed") } match { case Failure(ex) => logger.error(s"Failed to update consent ID: ${consent.id}", ex) case Success(_) => // Already logged @@ -109,13 +111,13 @@ object ConsentScheduler extends MdcLoggable { expiredConsents.foreach { consent => Try { 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("") // Prepend to existing note if any - consent - .mStatus(ConsentStatus.expired.toString) - .mNote(newNote) - .mStatusUpdateDateTime(new Date()) - .save - logger.warn(message) + val newNote = s"$currentDate\n$message\n" + Option(consent.note).getOrElse("") + val rows = code.bankconnectors.DoobieConsentSchedulerQueries.conditionallyExpireValidBerlinGroupConsent( + consentRowId = consent.id.get, + newNote = newNote + ) + if (rows > 0) logger.warn(message) + else logger.debug(s"|---> Skipped stale update for consent ${consent.id}: status already changed") } match { case Failure(ex) => logger.error(s"Failed to update consent ID: ${consent.id}", ex) case Success(_) => // Already logged @@ -141,13 +143,15 @@ object ConsentScheduler extends MdcLoggable { expiredConsents.foreach { consent => Try { 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("") // Prepend to existing note if any - consent - .mStatus(ConsentStatus.EXPIRED.toString) - .mNote(newNote) - .mStatusUpdateDateTime(new Date()) - .save - logger.warn(message) + val newNote = s"$currentDate\n$message\n" + Option(consent.note).getOrElse("") + val rows = code.bankconnectors.DoobieConsentSchedulerQueries.conditionallyUpdateStatus( + consentRowId = consent.id.get, + guardStatus = ConsentStatus.ACCEPTED.toString, + newStatus = ConsentStatus.EXPIRED.toString, + newNote = newNote + ) + if (rows > 0) logger.warn(message) + else logger.debug(s"|---> Skipped stale update for OBP consent ${consent.id}: status already changed") } match { case Failure(ex) => logger.error(s"Failed to update consent ID: ${consent.id}", ex) case Success(_) => // Already logged diff --git a/obp-api/src/main/scala/code/transaction/internalMapping/MappedTransactionIdMappingProvider.scala b/obp-api/src/main/scala/code/transaction/internalMapping/MappedTransactionIdMappingProvider.scala index f36f061e40..5e4ffa2732 100644 --- a/obp-api/src/main/scala/code/transaction/internalMapping/MappedTransactionIdMappingProvider.scala +++ b/obp-api/src/main/scala/code/transaction/internalMapping/MappedTransactionIdMappingProvider.scala @@ -4,6 +4,7 @@ import code.util.Helper.MdcLoggable import com.openbankproject.commons.model.TransactionId import net.liftweb.common._ import net.liftweb.mapper.By +import net.liftweb.util.Helpers.tryo object MappedTransactionIdMappingProvider extends TransactionIdMappingProvider with MdcLoggable @@ -26,15 +27,22 @@ object MappedTransactionIdMappingProvider extends TransactionIdMappingProvider w transactionIdMapping.map(_.transactionId) } case Empty => - { - val transactionIdMapping: TransactionIdMapping = + tryo { TransactionIdMapping .create .TransactionPlainTextReference(transactionPlainTextReference) .saveMe - logger.debug(s"getOrCreateTransactionId--> create mappedTransactionIdMapping : $transactionIdMapping") - Full(transactionIdMapping.transactionId) - } + } match { + case Full(m) => + logger.debug(s"getOrCreateTransactionId--> create mappedTransactionIdMapping : $m") + Full(m.transactionId) + case Failure(_, _, _) => + // UniqueIndex violation from concurrent insert — re-fetch the committed row + TransactionIdMapping.find( + By(TransactionIdMapping.TransactionPlainTextReference, transactionPlainTextReference) + ).map(_.transactionId) + case other => other.map(_.transactionId) + } case Failure(msg, t, c) => Failure(msg, t, c) case ParamFailure(x,y,z,q) => ParamFailure(x,y,z,q) } diff --git a/obp-api/src/main/scala/code/transactionChallenge/MappedChallengeProvider.scala b/obp-api/src/main/scala/code/transactionChallenge/MappedChallengeProvider.scala index b9fb7e1542..2d6ce838af 100644 --- a/obp-api/src/main/scala/code/transactionChallenge/MappedChallengeProvider.scala +++ b/obp-api/src/main/scala/code/transactionChallenge/MappedChallengeProvider.scala @@ -73,15 +73,13 @@ object MappedChallengeProvider extends ChallengeProvider { ): Box[ChallengeTrait] = { for{ challenge <- getChallenge(challengeId) ?~! s"${ErrorMessages.InvalidTransactionRequestChallengeId}" - currentAttemptCounterValue = challenge.attemptCounter - //We update the counter anyway. - _ = challenge.AttemptCounter(currentAttemptCounterValue+1).saveMe() + newAttemptCounterValue <- tryo(code.bankconnectors.DoobieChallengeQueries.incrementAndGetChallengeCounter(challengeId)) ?~! "Failed to update challenge attempt counter" createDateTime = challenge.createdAt.get challengeTTL : Long = Helpers.seconds(APIUtil.transactionRequestChallengeTtl) expiredDateTime: Long = createDateTime.getTime+challengeTTL currentTime: Long = Platform.currentTime - challenge <- if(currentAttemptCounterValue < APIUtil.allowedAnswerTransactionRequestChallengeAttempts){ + challenge <- if(newAttemptCounterValue <= APIUtil.allowedAnswerTransactionRequestChallengeAttempts){ if(expiredDateTime > currentTime) { val currentHashedAnswer = BCrypt.hashpw(challengeAnswer, challenge.salt).substring(0, 44) val expectedHashedAnswer = challenge.expectedAnswer diff --git a/obp-api/src/main/scala/code/usercustomerlinks/MappedUserCustomerLink.scala b/obp-api/src/main/scala/code/usercustomerlinks/MappedUserCustomerLink.scala index 6ed4b1fd9d..d517ca7285 100644 --- a/obp-api/src/main/scala/code/usercustomerlinks/MappedUserCustomerLink.scala +++ b/obp-api/src/main/scala/code/usercustomerlinks/MappedUserCustomerLink.scala @@ -25,13 +25,18 @@ object MappedUserCustomerLinkProvider extends UserCustomerLinkProvider { def getOCreateUserCustomerLink(userId: String, customerId: String, dateInserted: Date, isActive: Boolean): Box[UserCustomerLink] = { getUserCustomerLink(userId, customerId) match { case Empty => - val createUserCustomerLink = MappedUserCustomerLink.create - .mUserId(userId) - .mCustomerId(customerId) - .mDateInserted(new Date()) - .mIsActive(isActive) - .saveMe() - Some(createUserCustomerLink) + scala.util.Try { + MappedUserCustomerLink.create + .mUserId(userId) + .mCustomerId(customerId) + .mDateInserted(new Date()) + .mIsActive(isActive) + .saveMe() + } match { + case scala.util.Success(link) => Full(link) + case scala.util.Failure(_) => + getUserCustomerLink(userId, customerId) + } case everythingElse => everythingElse } } diff --git a/obp-api/src/main/scala/code/users/LiftUsers.scala b/obp-api/src/main/scala/code/users/LiftUsers.scala index 60163767d9..f6073e587c 100644 --- a/obp-api/src/main/scala/code/users/LiftUsers.scala +++ b/obp-api/src/main/scala/code/users/LiftUsers.scala @@ -50,12 +50,12 @@ object LiftUsers extends Users with MdcLoggable{ } def getOrCreateUserByProviderId(provider : String, idGivenByProvider : String, consentId: Option[String], name: Option[String], email: Option[String]) : (Box[User], Boolean) = { - val existingUser = Users.users.vend.getUserByProviderId(provider = provider, idGivenByProvider = idGivenByProvider) // Find a user + val existingUser = Users.users.vend.getUserByProviderId(provider = provider, idGivenByProvider = idGivenByProvider) existingUser match { - case Full(_) => // Existing user + case Full(_) => (existingUser, false) - case _ => // Otherwise create a new one - val newUser = Users.users.vend.createResourceUser( + case _ => + scala.util.Try(Users.users.vend.createResourceUser( provider = provider, providerId = Some(idGivenByProvider), createdByConsentId = consentId, @@ -65,8 +65,11 @@ object LiftUsers extends Users with MdcLoggable{ createdByUserInvitationId = None, company = None, lastMarketingAgreementSignedDate = None - ) - (newUser, true) + )) match { + case scala.util.Success(box) => (box, true) + case scala.util.Failure(_) => + (Users.users.vend.getUserByProviderId(provider, idGivenByProvider), false) + } } } def getOrCreateUserByProviderIdFuture(provider : String, idGivenByProvider : String, consentId: Option[String], name: Option[String], email: Option[String]) : Future[(Box[User], Boolean)] = { diff --git a/obp-api/src/main/scala/code/views/MapperViews.scala b/obp-api/src/main/scala/code/views/MapperViews.scala index e6ad15947a..93812381e3 100644 --- a/obp-api/src/main/scala/code/views/MapperViews.scala +++ b/obp-api/src/main/scala/code/views/MapperViews.scala @@ -637,185 +637,14 @@ object MapperViews extends Views with MdcLoggable { theView } - /** - * This migrates the current View permissions to the new ViewPermission model. - * this will not add any new permission, it will only migrate the existing permissions. - * @param viewDefinition - */ - def migrateViewPermissions(viewDefinition: View): Unit = { - - //first, we list all the current view permissions. - val permissionNames: List[String] = List( - CAN_SEE_TRANSACTION_OTHER_BANK_ACCOUNT, - CAN_SEE_TRANSACTION_METADATA, - CAN_SEE_TRANSACTION_DESCRIPTION, - CAN_SEE_TRANSACTION_AMOUNT, - CAN_SEE_TRANSACTION_TYPE, - CAN_SEE_TRANSACTION_CURRENCY, - CAN_SEE_TRANSACTION_START_DATE, - CAN_SEE_TRANSACTION_FINISH_DATE, - CAN_SEE_TRANSACTION_BALANCE, - CAN_SEE_COMMENTS, - CAN_SEE_OWNER_COMMENT, - CAN_SEE_TAGS, - CAN_SEE_IMAGES, - CAN_SEE_BANK_ACCOUNT_OWNERS, - CAN_SEE_BANK_ACCOUNT_TYPE, - CAN_SEE_BANK_ACCOUNT_BALANCE, - CAN_QUERY_AVAILABLE_FUNDS, - CAN_SEE_BANK_ACCOUNT_LABEL, - CAN_SEE_BANK_ACCOUNT_NATIONAL_IDENTIFIER, - CAN_SEE_BANK_ACCOUNT_SWIFT_BIC, - CAN_SEE_BANK_ACCOUNT_IBAN, - CAN_SEE_BANK_ACCOUNT_NUMBER, - CAN_SEE_BANK_ACCOUNT_BANK_NAME, - CAN_SEE_BANK_ACCOUNT_BANK_PERMALINK, - CAN_SEE_BANK_ROUTING_SCHEME, - CAN_SEE_BANK_ROUTING_ADDRESS, - CAN_SEE_BANK_ACCOUNT_ROUTING_SCHEME, - CAN_SEE_BANK_ACCOUNT_ROUTING_ADDRESS, - CAN_SEE_OTHER_ACCOUNT_NATIONAL_IDENTIFIER, - CAN_SEE_OTHER_ACCOUNT_SWIFT_BIC, - CAN_SEE_OTHER_ACCOUNT_IBAN, - CAN_SEE_OTHER_ACCOUNT_BANK_NAME, - CAN_SEE_OTHER_ACCOUNT_NUMBER, - CAN_SEE_OTHER_ACCOUNT_METADATA, - CAN_SEE_OTHER_ACCOUNT_KIND, - CAN_SEE_OTHER_BANK_ROUTING_SCHEME, - CAN_SEE_OTHER_BANK_ROUTING_ADDRESS, - CAN_SEE_OTHER_ACCOUNT_ROUTING_SCHEME, - CAN_SEE_OTHER_ACCOUNT_ROUTING_ADDRESS, - CAN_SEE_MORE_INFO, - CAN_SEE_URL, - CAN_SEE_IMAGE_URL, - CAN_SEE_OPEN_CORPORATES_URL, - CAN_SEE_CORPORATE_LOCATION, - CAN_SEE_PHYSICAL_LOCATION, - CAN_SEE_PUBLIC_ALIAS, - CAN_SEE_PRIVATE_ALIAS, - CAN_ADD_MORE_INFO, - CAN_ADD_URL, - CAN_ADD_IMAGE_URL, - CAN_ADD_OPEN_CORPORATES_URL, - CAN_ADD_CORPORATE_LOCATION, - CAN_ADD_PHYSICAL_LOCATION, - CAN_ADD_PUBLIC_ALIAS, - CAN_ADD_PRIVATE_ALIAS, - CAN_ADD_COUNTERPARTY, - CAN_GET_COUNTERPARTY, - CAN_DELETE_COUNTERPARTY, - CAN_DELETE_CORPORATE_LOCATION, - CAN_DELETE_PHYSICAL_LOCATION, - CAN_EDIT_OWNER_COMMENT, - CAN_ADD_COMMENT, - CAN_DELETE_COMMENT, - CAN_ADD_TAG, - CAN_DELETE_TAG, - CAN_ADD_IMAGE, - CAN_DELETE_IMAGE, - CAN_ADD_WHERE_TAG, - CAN_SEE_WHERE_TAG, - CAN_DELETE_WHERE_TAG, - CAN_ADD_TRANSACTION_REQUEST_TO_OWN_ACCOUNT, - CAN_ADD_TRANSACTION_REQUEST_TO_ANY_ACCOUNT, - CAN_SEE_BANK_ACCOUNT_CREDIT_LIMIT, - CAN_CREATE_DIRECT_DEBIT, - CAN_CREATE_STANDING_ORDER, - CAN_REVOKE_ACCESS_TO_CUSTOM_VIEWS, - CAN_GRANT_ACCESS_TO_CUSTOM_VIEWS, - CAN_SEE_TRANSACTION_REQUESTS, - CAN_SEE_TRANSACTION_REQUEST_TYPES, - CAN_SEE_AVAILABLE_VIEWS_FOR_BANK_ACCOUNT, - CAN_UPDATE_BANK_ACCOUNT_LABEL, - CAN_CREATE_CUSTOM_VIEW, - CAN_DELETE_CUSTOM_VIEW, - CAN_UPDATE_CUSTOM_VIEW, - CAN_GET_CUSTOM_VIEW, - CAN_SEE_VIEWS_WITH_PERMISSIONS_FOR_ALL_USERS, - CAN_SEE_VIEWS_WITH_PERMISSIONS_FOR_ONE_USER, - CAN_SEE_TRANSACTION_THIS_BANK_ACCOUNT, - CAN_SEE_TRANSACTION_STATUS, - CAN_SEE_BANK_ACCOUNT_CURRENCY, - CAN_ADD_TRANSACTION_REQUEST_TO_BENEFICIARY, - CAN_GRANT_ACCESS_TO_VIEWS, - CAN_REVOKE_ACCESS_TO_VIEWS - ) - - permissionNames.foreach { permissionName => - // CAN_REVOKE_ACCESS_TO_VIEWS and CAN_GRANT_ACCESS_TO_VIEWS are special cases, they have a list of view ids as metadata. - // For the rest of the permissions, they are just boolean values. - if (permissionName == CAN_REVOKE_ACCESS_TO_VIEWS || permissionName == CAN_GRANT_ACCESS_TO_VIEWS) { - - val permissionValueFromViewDefinition = viewDefinition.getClass.getMethod(StringHelpers.camelifyMethod(permissionName)).invoke(viewDefinition).asInstanceOf[Option[List[String]]] - - ViewPermission.findViewPermission(viewDefinition, permissionName) match { - // If the permission already exists in ViewPermission, but permissionValueFromViewDefinition is empty, we delete it. - case Full(permission) if permissionValueFromViewDefinition.isEmpty => - permission.delete_! - // If the permission already exists and permissionValueFromViewDefinition is defined, we update the metadata. - case Full(permission) if permissionValueFromViewDefinition.isDefined => - permission.extraData(permissionValueFromViewDefinition.get.mkString(",")).save - //if the permission is not existing in ViewPermission,but it is defined in the viewDefinition, we create it. --systemView - case Empty if (viewDefinition.isSystem && permissionValueFromViewDefinition.isDefined) => - ViewPermission.create - .bank_id(null) - .account_id(null) - .view_id(viewDefinition.viewId.value) - .permission(permissionName) - .extraData(permissionValueFromViewDefinition.get.mkString(",")) - .save - //if the permission is not existing in ViewPermission,but it is defined in the viewDefinition, we create it. --customView - case Empty if (!viewDefinition.isSystem && permissionValueFromViewDefinition.isDefined) => - ViewPermission.create - .bank_id(viewDefinition.bankId.value) - .account_id(viewDefinition.accountId.value) - .view_id(viewDefinition.viewId.value) - .permission(permissionName) - .extraData(permissionValueFromViewDefinition.get.mkString(",")) - .save - case _ => - // This case should not happen, but if it does, we add an error log - logger.error(s"Unexpected case for permission $permissionName for view ${viewDefinition.viewId.value}. No action taken.") - } - } else { - // For the rest of the permissions, they are just boolean values. - val permissionValue = viewDefinition.getClass.getMethod(StringHelpers.camelifyMethod(permissionName)).invoke(viewDefinition).asInstanceOf[Boolean] - - ViewPermission.findViewPermission(viewDefinition, permissionName) match { - // If the permission already exists in ViewPermission, but permissionValueFromViewdefinition is false, we delete it. - case Full(permission) if !permissionValue => - permission.delete_! - // If the permission already exists in ViewPermission, but permissionValueFromViewdefinition is empty, we udpate it. - case Full(permission) if permissionValue => - permission.permission(permissionName).save - //if the permission is not existing in ViewPermission, but it is defined in the viewDefinition, we create it. --systemView - case _ if (viewDefinition.isSystem && permissionValue) => - ViewPermission.create - .bank_id(null) - .account_id(null) - .view_id(viewDefinition.viewId.value) - .permission(permissionName) - .save - //if the permission is not existing in ViewPermission, but it is defined in the viewDefinition, we create it. --customerView - case _ if (!viewDefinition.isSystem && permissionValue) => - ViewPermission.create - .bank_id(viewDefinition.bankId.value) - .account_id(viewDefinition.accountId.value) - .view_id(viewDefinition.viewId.value) - .permission(permissionName) - .save - case _ => - // This case should not happen, but if it does, we do nothing - logger.warn(s"Unexpected case for permission $permissionName for view ${viewDefinition.viewId.value}. No action taken.") - } - } - } - } def getOrCreateSystemView(viewId: String) : Box[View] = { getExistingSystemView(viewId) match { case Empty => - createDefaultSystemView(viewId) + scala.util.Try(createDefaultSystemView(viewId)) match { + case scala.util.Success(box) => box + case scala.util.Failure(_) => getExistingSystemView(viewId) + } case Full(v) => Full(v) case Failure(msg, t, c) => Failure(msg, t, c) case ParamFailure(x,y,z,q) => ParamFailure(x,y,z,q) @@ -836,10 +665,13 @@ object MapperViews extends Views with MdcLoggable { def getOrCreateCustomPublicView(bankId: BankId, accountId: AccountId, description: String = "Public View") : Box[View] = { getExistingCustomView(bankId, accountId, CUSTOM_PUBLIC_VIEW_ID) match { - case Empty=> - createDefaultCustomPublicView(bankId, accountId, description) - case Full(v)=> - Full(v) + case Empty => + scala.util.Try(createDefaultCustomPublicView(bankId, accountId, description)) match { + case scala.util.Success(box) => box + case scala.util.Failure(_) => + getExistingCustomView(bankId, accountId, CUSTOM_PUBLIC_VIEW_ID) + } + case Full(v) => Full(v) case Failure(msg, t, c) => Failure(msg, t, c) case ParamFailure(x,y,z,q) => ParamFailure(x,y,z,q) } @@ -876,6 +708,11 @@ object MapperViews extends Views with MdcLoggable { } def removeAllViewsAndVierPermissions(bankId: BankId, accountId: AccountId) : Boolean = { + // bulkDelete_!! bypasses beforeDelete hooks, so AccountAccess must be removed explicitly. + AccountAccess.bulkDelete_!!( + By(AccountAccess.bank_id, bankId.value), + By(AccountAccess.account_id, accountId.value) + ) ViewDefinition.bulkDelete_!!( By(ViewDefinition.bank_id, bankId.value), By(ViewDefinition.account_id, accountId.value) diff --git a/obp-api/src/main/scala/code/views/system/ViewDefinition.scala b/obp-api/src/main/scala/code/views/system/ViewDefinition.scala index b7dc130dac..9815259bf3 100644 --- a/obp-api/src/main/scala/code/views/system/ViewDefinition.scala +++ b/obp-api/src/main/scala/code/views/system/ViewDefinition.scala @@ -50,297 +50,14 @@ class ViewDefinition extends View with LongKeyedMapper[ViewDefinition] with Many override def defaultValue = false } - //This is the system views list, custom views please check `canGrantAccessToCustomViews_` field object canGrantAccessToViews_ extends MappedText(this){ override def defaultValue = "" } - //This is the system views list.custom views please check `canRevokeAccessToCustomViews_` field object canRevokeAccessToViews_ extends MappedText(this){ override def defaultValue = "" } - object canRevokeAccessToCustomViews_ extends MappedBoolean(this){ - override def defaultValue = false - } - object canGrantAccessToCustomViews_ extends MappedBoolean(this) { - override def defaultValue = false - } - object canSeeTransactionThisBankAccount_ extends MappedBoolean(this){ - override def defaultValue = false - } - object canSeeTransactionRequests_ extends MappedBoolean(this){ - override def defaultValue = false - } - object canSeeTransactionRequestTypes_ extends MappedBoolean(this){ - override def defaultValue = false - } - object canSeeTransactionOtherBankAccount_ extends MappedBoolean(this){ - override def defaultValue = false - } - object canSeeTransactionMetadata_ extends MappedBoolean(this){ - override def defaultValue = false - } - object canSeeTransactionDescription_ extends MappedBoolean(this){ - override def defaultValue = false - } - object canSeeTransactionAmount_ extends MappedBoolean(this){ - override def defaultValue = false - } - object canSeeTransactionType_ extends MappedBoolean(this){ - override def defaultValue = false - } - object canSeeTransactionCurrency_ extends MappedBoolean(this){ - override def defaultValue = false - } - object canSeeTransactionStartDate_ extends MappedBoolean(this){ - override def defaultValue = false - } - object canSeeTransactionFinishDate_ extends MappedBoolean(this){ - override def defaultValue = false - } - object canSeeTransactionBalance_ extends MappedBoolean(this){ - override def defaultValue = false - } - object canSeeComments_ extends MappedBoolean(this){ - override def defaultValue = false - } - object canSeeOwnerComment_ extends MappedBoolean(this){ - override def defaultValue = false - } - object canSeeTags_ extends MappedBoolean(this){ - override def defaultValue = false - } - object canSeeImages_ extends MappedBoolean(this){ - override def defaultValue = false - } - object canSeeBankAccountOwners_ extends MappedBoolean(this){ - override def defaultValue = false - } - object canSeeAvailableViewsForBankAccount_ extends MappedBoolean(this){ - override def defaultValue = true - } - object canSeeBankAccountType_ extends MappedBoolean(this){ - override def defaultValue = false - } - object canSeeBankAccountBalance_ extends MappedBoolean(this){ - override def defaultValue = false - } - object canQueryAvailableFunds_ extends MappedBoolean(this){ - override def defaultValue = false - } - object canSeeBankAccountCurrency_ extends MappedBoolean(this){ - override def defaultValue = false - } - object canSeeBankAccountLabel_ extends MappedBoolean(this){ - override def defaultValue = false - } - object canUpdateBankAccountLabel_ extends MappedBoolean(this){ - override def defaultValue = false - } - object canSeeBankAccountNationalIdentifier_ extends MappedBoolean(this){ - override def defaultValue = false - } - object canSeeBankAccountSwift_bic_ extends MappedBoolean(this){ - override def defaultValue = false - } - object canSeeBankAccountIban_ extends MappedBoolean(this){ - override def defaultValue = false - } - object canSeeBankAccountNumber_ extends MappedBoolean(this){ - override def defaultValue = false - } - object canSeeBankAccountBankName_ extends MappedBoolean(this){ - override def defaultValue = false - } - object canSeeBankAccountBankPermalink_ extends MappedBoolean(this){ - override def defaultValue = false - } - object canSeeBankRoutingScheme_ extends MappedBoolean(this){ - override def defaultValue = false - } - object canSeeBankRoutingAddress_ extends MappedBoolean(this){ - override def defaultValue = false - } - object canSeeBankAccountRoutingScheme_ extends MappedBoolean(this){ - override def defaultValue = false - } - object canSeeBankAccountRoutingAddress_ extends MappedBoolean(this){ - override def defaultValue = false - } - object canSeeOtherAccountNationalIdentifier_ extends MappedBoolean(this){ - override def defaultValue = false - } - object canSeeOtherAccountSWIFT_BIC_ extends MappedBoolean(this){ - override def defaultValue = false - } - object canSeeOtherAccountIBAN_ extends MappedBoolean(this){ - override def defaultValue = false - } - object canSeeOtherAccountBankName_ extends MappedBoolean(this){ - override def defaultValue = false - } - object canSeeOtherAccountNumber_ extends MappedBoolean(this){ - override def defaultValue = false - } - object canSeeOtherAccountMetadata_ extends MappedBoolean(this){ - override def defaultValue = false - } - object canSeeOtherAccountKind_ extends MappedBoolean(this){ - override def defaultValue = false - } - object canSeeOtherBankRoutingScheme_ extends MappedBoolean(this){ - override def defaultValue = false - } - object canSeeOtherBankRoutingAddress_ extends MappedBoolean(this){ - override def defaultValue = false - } - object canSeeOtherAccountRoutingScheme_ extends MappedBoolean(this){ - override def defaultValue = false - } - object canSeeOtherAccountRoutingAddress_ extends MappedBoolean(this){ - override def defaultValue = false - } - object canSeeMoreInfo_ extends MappedBoolean(this){ - override def defaultValue = false - } - object canSeeUrl_ extends MappedBoolean(this){ - override def defaultValue = false - } - object canSeeImageUrl_ extends MappedBoolean(this){ - override def defaultValue = false - } - object canSeeOpenCorporatesUrl_ extends MappedBoolean(this){ - override def defaultValue = false - } - object canSeeCorporateLocation_ extends MappedBoolean(this){ - override def defaultValue = false - } - object canSeePhysicalLocation_ extends MappedBoolean(this){ - override def defaultValue = false - } - object canSeePublicAlias_ extends MappedBoolean(this){ - override def defaultValue = false - } - object canSeePrivateAlias_ extends MappedBoolean(this){ - override def defaultValue = false - } - object canAddMoreInfo_ extends MappedBoolean(this){ - override def defaultValue = false - } - object canAddURL_ extends MappedBoolean(this){ - override def defaultValue = false - } - object canAddImageURL_ extends MappedBoolean(this){ - override def defaultValue = false - } - object canAddOpenCorporatesUrl_ extends MappedBoolean(this){ - override def defaultValue = false - } - object canAddCorporateLocation_ extends MappedBoolean(this){ - override def defaultValue = false - } - object canAddPhysicalLocation_ extends MappedBoolean(this){ - override def defaultValue = false - } - object canAddPublicAlias_ extends MappedBoolean(this){ - override def defaultValue = false - } - object canAddPrivateAlias_ extends MappedBoolean(this){ - override def defaultValue = false - } - object canAddCounterparty_ extends MappedBoolean(this){ - override def defaultValue = false - } - object canGetCounterparty_ extends MappedBoolean(this){ - override def defaultValue = false - } - object canDeleteCounterparty_ extends MappedBoolean(this){ - override def defaultValue = false - } - object canDeleteCorporateLocation_ extends MappedBoolean(this){ - override def defaultValue = false - } - object canDeletePhysicalLocation_ extends MappedBoolean(this){ - override def defaultValue = false - } - object canEditOwnerComment_ extends MappedBoolean(this){ - override def defaultValue = false - } - object canAddComment_ extends MappedBoolean(this){ - override def defaultValue = false - } - object canDeleteComment_ extends MappedBoolean(this){ - override def defaultValue = false - } - object canAddTag_ extends MappedBoolean(this){ - override def defaultValue = false - } - object canDeleteTag_ extends MappedBoolean(this){ - override def defaultValue = false - } - object canAddImage_ extends MappedBoolean(this){ - override def defaultValue = false - } - object canDeleteImage_ extends MappedBoolean(this){ - override def defaultValue = false - } - object canAddWhereTag_ extends MappedBoolean(this){ - override def defaultValue = false - } - object canSeeWhereTag_ extends MappedBoolean(this){ - override def defaultValue = false - } - object canDeleteWhereTag_ extends MappedBoolean(this){ - override def defaultValue = false - } - - //internal transfer between my own accounts - - @deprecated("we added new field `canAddTransactionRequestToBeneficiary_`","25-07-2024") - object canAddTransactionRequestToOwnAccount_ extends MappedBoolean(this){ - override def defaultValue = false - } - - object canAddTransactionRequestToBeneficiary_ extends MappedBoolean(this){ - override def defaultValue = false - } - - // transfer to any account - object canAddTransactionRequestToAnyAccount_ extends MappedBoolean(this){ - override def defaultValue = false - } - object canSeeBankAccountCreditLimit_ extends MappedBoolean(this){ - override def defaultValue = false - } - object canCreateDirectDebit_ extends MappedBoolean(this){ - override def defaultValue = false - } - object canCreateStandingOrder_ extends MappedBoolean(this){ - override def defaultValue = false - } - - object canCreateCustomView_ extends MappedBoolean(this){ - override def defaultValue = false - } - object canDeleteCustomView_ extends MappedBoolean(this){ - override def defaultValue = false - } - object canUpdateCustomView_ extends MappedBoolean(this){ - override def defaultValue = false - } - object canGetCustomView_ extends MappedBoolean(this){ - override def defaultValue = false - } - object canSeeViewsWithPermissionsForAllUsers_ extends MappedBoolean(this){ - override def defaultValue = false - } - object canSeeViewsWithPermissionsForOneUser_ extends MappedBoolean(this){ - override def defaultValue = false - } - object canSeeTransactionStatus_ extends MappedBoolean(this){ - override def defaultValue = false - } //Important! If you add a field, be sure to handle it here in this function def setFromViewData(viewSpecification : ViewSpecification) = { @@ -446,102 +163,123 @@ class ViewDefinition extends View with LongKeyedMapper[ViewDefinition] with Many }) } - //TODO All the following methods can be removed later, we use ViewPermission table instead. - override def canRevokeAccessToCustomViews : Boolean = canRevokeAccessToCustomViews_.get - override def canGrantAccessToCustomViews : Boolean = canGrantAccessToCustomViews_.get - def canSeeTransactionThisBankAccount : Boolean = canSeeTransactionThisBankAccount_.get - def canSeeTransactionRequests : Boolean = canSeeTransactionRequests_.get - def canSeeTransactionRequestTypes: Boolean = canSeeTransactionRequestTypes_.get - def canSeeTransactionOtherBankAccount : Boolean = canSeeTransactionOtherBankAccount_.get - def canSeeTransactionMetadata : Boolean = canSeeTransactionMetadata_.get - def canSeeTransactionDescription: Boolean = canSeeTransactionDescription_.get - def canSeeTransactionAmount: Boolean = canSeeTransactionAmount_.get - def canSeeTransactionType: Boolean = canSeeTransactionType_.get - def canSeeTransactionCurrency: Boolean = canSeeTransactionCurrency_.get - def canSeeTransactionStartDate: Boolean = canSeeTransactionStartDate_.get - def canSeeTransactionFinishDate: Boolean = canSeeTransactionFinishDate_.get - def canSeeTransactionBalance: Boolean = canSeeTransactionBalance_.get - def canSeeTransactionStatus: Boolean = canSeeTransactionStatus_.get - def canSeeComments: Boolean = canSeeComments_.get - def canSeeOwnerComment: Boolean = canSeeOwnerComment_.get - def canSeeTags : Boolean = canSeeTags_.get - def canSeeImages : Boolean = canSeeImages_.get - def canSeeAvailableViewsForBankAccount : Boolean = canSeeAvailableViewsForBankAccount_.get - def canSeeBankAccountOwners : Boolean = canSeeBankAccountOwners_.get - def canSeeBankAccountType : Boolean = canSeeBankAccountType_.get - def canSeeBankAccountBalance : Boolean = canSeeBankAccountBalance_.get - def canSeeBankAccountCurrency : Boolean = canSeeBankAccountCurrency_.get - def canQueryAvailableFunds : Boolean = canQueryAvailableFunds_.get - def canSeeBankAccountLabel : Boolean = canSeeBankAccountLabel_.get - def canUpdateBankAccountLabel : Boolean = canUpdateBankAccountLabel_.get - def canSeeBankAccountNationalIdentifier : Boolean = canSeeBankAccountNationalIdentifier_.get - def canSeeBankAccountSwiftBic : Boolean = canSeeBankAccountSwift_bic_.get - def canSeeBankAccountIban : Boolean = canSeeBankAccountIban_.get - def canSeeBankAccountNumber : Boolean = canSeeBankAccountNumber_.get - def canSeeBankAccountBankName : Boolean = canSeeBankAccountBankName_.get - def canSeeBankAccountBankPermalink : Boolean = canSeeBankAccountBankPermalink_.get - def canSeeBankRoutingScheme : Boolean = canSeeBankRoutingScheme_.get - def canSeeBankRoutingAddress : Boolean = canSeeBankRoutingAddress_.get - def canSeeBankAccountRoutingScheme : Boolean = canSeeBankAccountRoutingScheme_.get - def canSeeBankAccountRoutingAddress : Boolean = canSeeBankAccountRoutingAddress_.get - def canSeeViewsWithPermissionsForOneUser: Boolean = canSeeViewsWithPermissionsForOneUser_.get - def canSeeViewsWithPermissionsForAllUsers : Boolean = canSeeViewsWithPermissionsForAllUsers_.get - def canSeeOtherAccountNationalIdentifier : Boolean = canSeeOtherAccountNationalIdentifier_.get - def canSeeOtherAccountSwiftBic : Boolean = canSeeOtherAccountSWIFT_BIC_.get - def canSeeOtherAccountIban : Boolean = canSeeOtherAccountIBAN_.get - def canSeeOtherAccountBankName : Boolean = canSeeOtherAccountBankName_.get - def canSeeOtherAccountNumber : Boolean = canSeeOtherAccountNumber_.get - def canSeeOtherAccountMetadata : Boolean = canSeeOtherAccountMetadata_.get - def canSeeOtherAccountKind : Boolean = canSeeOtherAccountKind_.get - def canSeeOtherBankRoutingScheme : Boolean = canSeeOtherBankRoutingScheme_.get - def canSeeOtherBankRoutingAddress : Boolean = canSeeOtherBankRoutingAddress_.get - def canSeeOtherAccountRoutingScheme : Boolean = canSeeOtherAccountRoutingScheme_.get - def canSeeOtherAccountRoutingAddress : Boolean = canSeeOtherAccountRoutingAddress_.get - def canSeeMoreInfo: Boolean = canSeeMoreInfo_.get - def canSeeUrl: Boolean = canSeeUrl_.get - def canSeeImageUrl: Boolean = canSeeImageUrl_.get - def canSeeOpenCorporatesUrl: Boolean = canSeeOpenCorporatesUrl_.get - def canSeeCorporateLocation : Boolean = canSeeCorporateLocation_.get - def canSeePhysicalLocation : Boolean = canSeePhysicalLocation_.get - def canSeePublicAlias : Boolean = canSeePublicAlias_.get - def canSeePrivateAlias : Boolean = canSeePrivateAlias_.get - def canAddMoreInfo : Boolean = canAddMoreInfo_.get - def canAddUrl : Boolean = canAddURL_.get - def canAddImageUrl : Boolean = canAddImageURL_.get - def canAddOpenCorporatesUrl : Boolean = canAddOpenCorporatesUrl_.get - def canAddCorporateLocation : Boolean = canAddCorporateLocation_.get - def canAddPhysicalLocation : Boolean = canAddPhysicalLocation_.get - def canAddPublicAlias : Boolean = canAddPublicAlias_.get - def canAddPrivateAlias : Boolean = canAddPrivateAlias_.get - def canAddCounterparty : Boolean = canAddCounterparty_.get - def canGetCounterparty : Boolean = canGetCounterparty_.get - def canDeleteCounterparty : Boolean = canDeleteCounterparty_.get - def canDeleteCorporateLocation : Boolean = canDeleteCorporateLocation_.get - def canDeletePhysicalLocation : Boolean = canDeletePhysicalLocation_.get - def canEditOwnerComment: Boolean = canEditOwnerComment_.get - def canAddComment : Boolean = canAddComment_.get - def canDeleteComment: Boolean = canDeleteComment_.get - def canAddTag : Boolean = canAddTag_.get - def canDeleteTag : Boolean = canDeleteTag_.get - def canAddImage : Boolean = canAddImage_.get - def canDeleteImage : Boolean = canDeleteImage_.get - def canAddWhereTag : Boolean = canAddWhereTag_.get - def canSeeWhereTag : Boolean = canSeeWhereTag_.get - def canDeleteWhereTag : Boolean = canDeleteWhereTag_.get + // These permission accessors now derive from the ViewPermission table via `allowed_actions`, + // not the legacy per-permission boolean columns (issue #26). The boolean columns have been + // retired and the ViewPermission table is the single source of truth. Each accessor maps to + // exactly one permission-string constant (see code.api.Constant) — the same 1:1 mapping the + // former migration used via StringHelpers.camelifyMethod. + private def hasPermission(permission: String): Boolean = allowed_actions.exists(_ == permission) + + override def canRevokeAccessToCustomViews : Boolean = hasPermission(CAN_REVOKE_ACCESS_TO_CUSTOM_VIEWS) + override def canGrantAccessToCustomViews : Boolean = hasPermission(CAN_GRANT_ACCESS_TO_CUSTOM_VIEWS) + def canSeeTransactionThisBankAccount : Boolean = hasPermission(CAN_SEE_TRANSACTION_THIS_BANK_ACCOUNT) + def canSeeTransactionRequests : Boolean = hasPermission(CAN_SEE_TRANSACTION_REQUESTS) + def canSeeTransactionRequestTypes: Boolean = hasPermission(CAN_SEE_TRANSACTION_REQUEST_TYPES) + def canSeeTransactionOtherBankAccount : Boolean = hasPermission(CAN_SEE_TRANSACTION_OTHER_BANK_ACCOUNT) + def canSeeTransactionMetadata : Boolean = hasPermission(CAN_SEE_TRANSACTION_METADATA) + def canSeeTransactionDescription: Boolean = hasPermission(CAN_SEE_TRANSACTION_DESCRIPTION) + def canSeeTransactionAmount: Boolean = hasPermission(CAN_SEE_TRANSACTION_AMOUNT) + def canSeeTransactionType: Boolean = hasPermission(CAN_SEE_TRANSACTION_TYPE) + def canSeeTransactionCurrency: Boolean = hasPermission(CAN_SEE_TRANSACTION_CURRENCY) + def canSeeTransactionStartDate: Boolean = hasPermission(CAN_SEE_TRANSACTION_START_DATE) + def canSeeTransactionFinishDate: Boolean = hasPermission(CAN_SEE_TRANSACTION_FINISH_DATE) + def canSeeTransactionBalance: Boolean = hasPermission(CAN_SEE_TRANSACTION_BALANCE) + def canSeeTransactionStatus: Boolean = hasPermission(CAN_SEE_TRANSACTION_STATUS) + def canSeeComments: Boolean = hasPermission(CAN_SEE_COMMENTS) + def canSeeOwnerComment: Boolean = hasPermission(CAN_SEE_OWNER_COMMENT) + def canSeeTags : Boolean = hasPermission(CAN_SEE_TAGS) + def canSeeImages : Boolean = hasPermission(CAN_SEE_IMAGES) + def canSeeAvailableViewsForBankAccount : Boolean = hasPermission(CAN_SEE_AVAILABLE_VIEWS_FOR_BANK_ACCOUNT) + def canSeeBankAccountOwners : Boolean = hasPermission(CAN_SEE_BANK_ACCOUNT_OWNERS) + def canSeeBankAccountType : Boolean = hasPermission(CAN_SEE_BANK_ACCOUNT_TYPE) + def canSeeBankAccountBalance : Boolean = hasPermission(CAN_SEE_BANK_ACCOUNT_BALANCE) + def canSeeBankAccountCurrency : Boolean = hasPermission(CAN_SEE_BANK_ACCOUNT_CURRENCY) + def canQueryAvailableFunds : Boolean = hasPermission(CAN_QUERY_AVAILABLE_FUNDS) + def canSeeBankAccountLabel : Boolean = hasPermission(CAN_SEE_BANK_ACCOUNT_LABEL) + def canUpdateBankAccountLabel : Boolean = hasPermission(CAN_UPDATE_BANK_ACCOUNT_LABEL) + def canSeeBankAccountNationalIdentifier : Boolean = hasPermission(CAN_SEE_BANK_ACCOUNT_NATIONAL_IDENTIFIER) + def canSeeBankAccountSwiftBic : Boolean = hasPermission(CAN_SEE_BANK_ACCOUNT_SWIFT_BIC) + def canSeeBankAccountIban : Boolean = hasPermission(CAN_SEE_BANK_ACCOUNT_IBAN) + def canSeeBankAccountNumber : Boolean = hasPermission(CAN_SEE_BANK_ACCOUNT_NUMBER) + def canSeeBankAccountBankName : Boolean = hasPermission(CAN_SEE_BANK_ACCOUNT_BANK_NAME) + def canSeeBankAccountBankPermalink : Boolean = hasPermission(CAN_SEE_BANK_ACCOUNT_BANK_PERMALINK) + def canSeeBankRoutingScheme : Boolean = hasPermission(CAN_SEE_BANK_ROUTING_SCHEME) + def canSeeBankRoutingAddress : Boolean = hasPermission(CAN_SEE_BANK_ROUTING_ADDRESS) + def canSeeBankAccountRoutingScheme : Boolean = hasPermission(CAN_SEE_BANK_ACCOUNT_ROUTING_SCHEME) + def canSeeBankAccountRoutingAddress : Boolean = hasPermission(CAN_SEE_BANK_ACCOUNT_ROUTING_ADDRESS) + def canSeeViewsWithPermissionsForOneUser: Boolean = hasPermission(CAN_SEE_VIEWS_WITH_PERMISSIONS_FOR_ONE_USER) + def canSeeViewsWithPermissionsForAllUsers : Boolean = hasPermission(CAN_SEE_VIEWS_WITH_PERMISSIONS_FOR_ALL_USERS) + def canSeeOtherAccountNationalIdentifier : Boolean = hasPermission(CAN_SEE_OTHER_ACCOUNT_NATIONAL_IDENTIFIER) + def canSeeOtherAccountSwiftBic : Boolean = hasPermission(CAN_SEE_OTHER_ACCOUNT_SWIFT_BIC) + def canSeeOtherAccountIban : Boolean = hasPermission(CAN_SEE_OTHER_ACCOUNT_IBAN) + def canSeeOtherAccountBankName : Boolean = hasPermission(CAN_SEE_OTHER_ACCOUNT_BANK_NAME) + def canSeeOtherAccountNumber : Boolean = hasPermission(CAN_SEE_OTHER_ACCOUNT_NUMBER) + def canSeeOtherAccountMetadata : Boolean = hasPermission(CAN_SEE_OTHER_ACCOUNT_METADATA) + def canSeeOtherAccountKind : Boolean = hasPermission(CAN_SEE_OTHER_ACCOUNT_KIND) + def canSeeOtherBankRoutingScheme : Boolean = hasPermission(CAN_SEE_OTHER_BANK_ROUTING_SCHEME) + def canSeeOtherBankRoutingAddress : Boolean = hasPermission(CAN_SEE_OTHER_BANK_ROUTING_ADDRESS) + def canSeeOtherAccountRoutingScheme : Boolean = hasPermission(CAN_SEE_OTHER_ACCOUNT_ROUTING_SCHEME) + def canSeeOtherAccountRoutingAddress : Boolean = hasPermission(CAN_SEE_OTHER_ACCOUNT_ROUTING_ADDRESS) + def canSeeMoreInfo: Boolean = hasPermission(CAN_SEE_MORE_INFO) + def canSeeUrl: Boolean = hasPermission(CAN_SEE_URL) + def canSeeImageUrl: Boolean = hasPermission(CAN_SEE_IMAGE_URL) + def canSeeOpenCorporatesUrl: Boolean = hasPermission(CAN_SEE_OPEN_CORPORATES_URL) + def canSeeCorporateLocation : Boolean = hasPermission(CAN_SEE_CORPORATE_LOCATION) + def canSeePhysicalLocation : Boolean = hasPermission(CAN_SEE_PHYSICAL_LOCATION) + def canSeePublicAlias : Boolean = hasPermission(CAN_SEE_PUBLIC_ALIAS) + def canSeePrivateAlias : Boolean = hasPermission(CAN_SEE_PRIVATE_ALIAS) + def canAddMoreInfo : Boolean = hasPermission(CAN_ADD_MORE_INFO) + def canAddUrl : Boolean = hasPermission(CAN_ADD_URL) + def canAddImageUrl : Boolean = hasPermission(CAN_ADD_IMAGE_URL) + def canAddOpenCorporatesUrl : Boolean = hasPermission(CAN_ADD_OPEN_CORPORATES_URL) + def canAddCorporateLocation : Boolean = hasPermission(CAN_ADD_CORPORATE_LOCATION) + def canAddPhysicalLocation : Boolean = hasPermission(CAN_ADD_PHYSICAL_LOCATION) + def canAddPublicAlias : Boolean = hasPermission(CAN_ADD_PUBLIC_ALIAS) + def canAddPrivateAlias : Boolean = hasPermission(CAN_ADD_PRIVATE_ALIAS) + def canAddCounterparty : Boolean = hasPermission(CAN_ADD_COUNTERPARTY) + def canGetCounterparty : Boolean = hasPermission(CAN_GET_COUNTERPARTY) + def canDeleteCounterparty : Boolean = hasPermission(CAN_DELETE_COUNTERPARTY) + def canDeleteCorporateLocation : Boolean = hasPermission(CAN_DELETE_CORPORATE_LOCATION) + def canDeletePhysicalLocation : Boolean = hasPermission(CAN_DELETE_PHYSICAL_LOCATION) + def canEditOwnerComment: Boolean = hasPermission(CAN_EDIT_OWNER_COMMENT) + def canAddComment : Boolean = hasPermission(CAN_ADD_COMMENT) + def canDeleteComment: Boolean = hasPermission(CAN_DELETE_COMMENT) + def canAddTag : Boolean = hasPermission(CAN_ADD_TAG) + def canDeleteTag : Boolean = hasPermission(CAN_DELETE_TAG) + def canAddImage : Boolean = hasPermission(CAN_ADD_IMAGE) + def canDeleteImage : Boolean = hasPermission(CAN_DELETE_IMAGE) + def canAddWhereTag : Boolean = hasPermission(CAN_ADD_WHERE_TAG) + def canSeeWhereTag : Boolean = hasPermission(CAN_SEE_WHERE_TAG) + def canDeleteWhereTag : Boolean = hasPermission(CAN_DELETE_WHERE_TAG) def canAddTransactionRequestToOwnAccount: Boolean = false //we do not need this field, set this to false. - def canAddTransactionRequestToAnyAccount: Boolean = canAddTransactionRequestToAnyAccount_.get - def canAddTransactionRequestToBeneficiary: Boolean = canAddTransactionRequestToBeneficiary_.get - def canSeeBankAccountCreditLimit: Boolean = canSeeBankAccountCreditLimit_.get - def canCreateDirectDebit: Boolean = canCreateDirectDebit_.get - def canCreateStandingOrder: Boolean = canCreateStandingOrder_.get - def canCreateCustomView: Boolean = canCreateCustomView_.get - def canDeleteCustomView: Boolean = canDeleteCustomView_.get - def canUpdateCustomView: Boolean = canUpdateCustomView_.get - def canGetCustomView: Boolean = canGetCustomView_.get + def canAddTransactionRequestToAnyAccount: Boolean = hasPermission(CAN_ADD_TRANSACTION_REQUEST_TO_ANY_ACCOUNT) + def canAddTransactionRequestToBeneficiary: Boolean = hasPermission(CAN_ADD_TRANSACTION_REQUEST_TO_BENEFICIARY) + def canSeeBankAccountCreditLimit: Boolean = hasPermission(CAN_SEE_BANK_ACCOUNT_CREDIT_LIMIT) + def canCreateDirectDebit: Boolean = hasPermission(CAN_CREATE_DIRECT_DEBIT) + def canCreateStandingOrder: Boolean = hasPermission(CAN_CREATE_STANDING_ORDER) + def canCreateCustomView: Boolean = hasPermission(CAN_CREATE_CUSTOM_VIEW) + def canDeleteCustomView: Boolean = hasPermission(CAN_DELETE_CUSTOM_VIEW) + def canUpdateCustomView: Boolean = hasPermission(CAN_UPDATE_CUSTOM_VIEW) + def canGetCustomView: Boolean = hasPermission(CAN_GET_CUSTOM_VIEW) } object ViewDefinition extends ViewDefinition with LongKeyedMetaMapper[ViewDefinition] { override def dbIndexes: List[BaseIndex[ViewDefinition]] = UniqueIndex(composite_unique_key) :: Index(isSystem_, view_id) :: Index(bank_id, account_id, view_id) :: super.dbIndexes + override def beforeDelete = List( + vd => { + val conditions: Seq[QueryParam[AccountAccess]] = + if (vd.isSystem || vd.bank_id.get == null || vd.account_id.get == null) + Seq(By(AccountAccess.view_id, vd.view_id.get)) + else + Seq( + By(AccountAccess.bank_id, vd.bank_id.get), + By(AccountAccess.account_id, vd.account_id.get), + By(AccountAccess.view_id, vd.view_id.get) + ) + AccountAccess.bulkDelete_!!(conditions: _*) + } + ) + override def beforeSave = List( t =>{ tryo { diff --git a/obp-api/src/main/scala/code/views/system/ViewPermission.scala b/obp-api/src/main/scala/code/views/system/ViewPermission.scala index d40440edb4..2f0bfaa558 100644 --- a/obp-api/src/main/scala/code/views/system/ViewPermission.scala +++ b/obp-api/src/main/scala/code/views/system/ViewPermission.scala @@ -127,14 +127,16 @@ object ViewPermission extends ViewPermission with LongKeyedMetaMapper[ViewPermis // Remove existing conflicting record if any ViewPermission.find(conditions: _*).foreach(_.delete_!) - // Insert new permission - ViewPermission.create - .bank_id(bankId) - .account_id(accountId) - .view_id(view.viewId.value) - .permission(permissionName) - .extraData(extraData) - .save + // Insert new permission; ignore constraint violation from a concurrent reset + scala.util.Try { + ViewPermission.create + .bank_id(bankId) + .account_id(accountId) + .view_id(view.viewId.value) + .permission(permissionName) + .extraData(extraData) + .save + } } } diff --git a/obp-api/src/main/scripts/migrate/migrate_0000001.sql b/obp-api/src/main/scripts/migrate/migrate_0000001.sql deleted file mode 100644 index 0b5365c5bf..0000000000 --- a/obp-api/src/main/scripts/migrate/migrate_0000001.sql +++ /dev/null @@ -1,3 +0,0 @@ -alter table mappedbankaccount alter column accuuid type varchar(80); - -alter table mappedbankaccount alter column accountnumber type varchar(80); \ No newline at end of file diff --git a/obp-api/src/main/scripts/migrate/migrate_00000010.sql b/obp-api/src/main/scripts/migrate/migrate_00000010.sql deleted file mode 100644 index 73a9898225..0000000000 --- a/obp-api/src/main/scripts/migrate/migrate_00000010.sql +++ /dev/null @@ -1,3 +0,0 @@ -ALTER TABLE "mappedcounterpartymetadata" ALTER COLUMN "counterpartyid" type varchar(44); -ALTER TABLE "mappedcounterparty" ALTER COLUMN "mcounterpartyid" type varchar(44); -ALTER TABLE "mappedtransaction" ALTER COLUMN "cpcounterpartyid" type varchar(44); \ No newline at end of file diff --git a/obp-api/src/main/scripts/migrate/migrate_00000011.sql b/obp-api/src/main/scripts/migrate/migrate_00000011.sql deleted file mode 100644 index 9424cbf214..0000000000 --- a/obp-api/src/main/scripts/migrate/migrate_00000011.sql +++ /dev/null @@ -1,6 +0,0 @@ -update - viewdefinition -set - isFirehose_ = TRUE -where - isFirehose_ <> TRUE; diff --git a/obp-api/src/main/scripts/migrate/migrate_00000012.sql b/obp-api/src/main/scripts/migrate/migrate_00000012.sql deleted file mode 100644 index c1fc281927..0000000000 --- a/obp-api/src/main/scripts/migrate/migrate_00000012.sql +++ /dev/null @@ -1 +0,0 @@ -CREATE INDEX mappedmetric_date_c ON mappedmetric(date_c); \ No newline at end of file diff --git a/obp-api/src/main/scripts/migrate/migrate_00000013.sql b/obp-api/src/main/scripts/migrate/migrate_00000013.sql deleted file mode 100644 index 969920f80f..0000000000 --- a/obp-api/src/main/scripts/migrate/migrate_00000013.sql +++ /dev/null @@ -1,14 +0,0 @@ -UPDATE - consumer -SET - perhourcalllimit = -1, - perdaycalllimit = -1, - perweekcalllimit = -1, - permonthcalllimit = -1, - perminutecalllimit = -1 -WHERE - perhourcalllimit <> -1 - OR perdaycalllimit <> -1 - OR perweekcalllimit <> -1 - OR permonthcalllimit <> -1 - OR perminutecalllimit <> -1; diff --git a/obp-api/src/main/scripts/migrate/migrate_00000014.sql b/obp-api/src/main/scripts/migrate/migrate_00000014.sql deleted file mode 100644 index ac8ad2734e..0000000000 --- a/obp-api/src/main/scripts/migrate/migrate_00000014.sql +++ /dev/null @@ -1,6 +0,0 @@ -UPDATE - consumer -SET - persecondcalllimit = -1 -where - persecondcalllimit <> -1; diff --git a/obp-api/src/main/scripts/migrate/migrate_00000015.sql b/obp-api/src/main/scripts/migrate/migrate_00000015.sql deleted file mode 100644 index 6271a19265..0000000000 --- a/obp-api/src/main/scripts/migrate/migrate_00000015.sql +++ /dev/null @@ -1,2 +0,0 @@ -ALTER TABLE "mappeduserauthcontext" ALTER COLUMN "mvalue" type varchar(255); -ALTER TABLE "mappeduserauthcontext" ALTER COLUMN "mkey" type varchar(255); \ No newline at end of file diff --git a/obp-api/src/main/scripts/migrate/migrate_00000016.sql b/obp-api/src/main/scripts/migrate/migrate_00000016.sql deleted file mode 100644 index 5e5f01cf4a..0000000000 --- a/obp-api/src/main/scripts/migrate/migrate_00000016.sql +++ /dev/null @@ -1 +0,0 @@ -ALTER TABLE "mappedconsent" ALTER COLUMN "mjsonwebtoken" type varchar(4096); \ No newline at end of file diff --git a/obp-api/src/main/scripts/migrate/migrate_00000017.sql b/obp-api/src/main/scripts/migrate/migrate_00000017.sql deleted file mode 100644 index deba93c166..0000000000 --- a/obp-api/src/main/scripts/migrate/migrate_00000017.sql +++ /dev/null @@ -1 +0,0 @@ -ALTER TABLE "webuiprops" ALTER COLUMN "value" TYPE text ; \ No newline at end of file diff --git a/obp-api/src/main/scripts/migrate/migrate_00000018.sql b/obp-api/src/main/scripts/migrate/migrate_00000018.sql deleted file mode 100644 index 6912215c9e..0000000000 --- a/obp-api/src/main/scripts/migrate/migrate_00000018.sql +++ /dev/null @@ -1 +0,0 @@ -ALTER TABLE "methodrouting" ADD "parameters" varchar(5000) NULL; diff --git a/obp-api/src/main/scripts/migrate/migrate_00000019.sql b/obp-api/src/main/scripts/migrate/migrate_00000019.sql deleted file mode 100644 index 78e98dd2e0..0000000000 --- a/obp-api/src/main/scripts/migrate/migrate_00000019.sql +++ /dev/null @@ -1,3 +0,0 @@ --- the index DDL is: CREATE UNIQUE INDEX dynamicentity_metadatajson ON public.dynamicentity USING btree (metadatajson); --- it is not be used now, and btree type index have length limit of 2704, So just delete this index. -DROP INDEX public.dynamicentity_metadatajson; diff --git a/obp-api/src/main/scripts/migrate/migrate_0000002.sql b/obp-api/src/main/scripts/migrate/migrate_0000002.sql deleted file mode 100644 index d7a3951a35..0000000000 --- a/obp-api/src/main/scripts/migrate/migrate_0000002.sql +++ /dev/null @@ -1,2 +0,0 @@ -alter table users add column username type varchar(64); -CREATE UNIQUE INDEX unq_username ON users (username); \ No newline at end of file diff --git a/obp-api/src/main/scripts/migrate/migrate_0000003.sql b/obp-api/src/main/scripts/migrate/migrate_0000003.sql deleted file mode 100644 index e554ddd69b..0000000000 --- a/obp-api/src/main/scripts/migrate/migrate_0000003.sql +++ /dev/null @@ -1,2 +0,0 @@ --- We don't need unique index on customer number -drop index mappedkycstatus_mcustomernumber; \ No newline at end of file diff --git a/obp-api/src/main/scripts/migrate/migrate_0000004.sql b/obp-api/src/main/scripts/migrate/migrate_0000004.sql deleted file mode 100644 index b13754bece..0000000000 --- a/obp-api/src/main/scripts/migrate/migrate_0000004.sql +++ /dev/null @@ -1,34 +0,0 @@ --- This change relates to --- Simplify User model #187 - Refactored APIUser -> ResourceUser and OBPUser -> AuthUser. added sql migration script for apiuser table --- in git commit: 3d0e1dd293906932b6a6969741dc6b8f57adb749 - --- In which apiuser is changed to resourceuser and OBPUser is renamed to AuthUser - --- There are at least two ways to handle the change - - - --- 1) (before running the API after 3d0e1dd293906932b6a6969741dc6b8f57adb749) --- Rename apiuser to resourceuser -ALTER TABLE apiuser RENAME TO resourceuser; -DROP INDEX apiuser_provider__providerid; -CREATE UNIQUE INDEX resourceuser_provider__providerid ON resourceuser (PROVIDER_,PROVIDERID); - -ALTER TABLE users RENAME TO authuser; -DROP INDEX USERS_USER_C; -CREATE INDEX AUTHUSER_USER_C ON authuser (USER_C); -DROP INDEX USERS_UNIQUEID; -CREATE INDEX AUTHUSER_UNIQUEID ON authuser (UNIQUEID); -DROP INDEX USERS_USERNAME; -CREATE INDEX AUTHUSER_USERNAME ON authuser (USERNAME); - --- OR -- - --- 2) (after running the API after 3d0e1dd293906932b6a6969741dc6b8f57adb749) --- Copy the records after running the API (and lift-web schemify has created the table) -insert into resourceuser (id, email, provider_, providerid, name_, userid_) select id, email, provider_, providerid, name_, userid_ from apiuser; - -insert into authuser (id ,firstname ,lastname ,email ,username ,password_pw ,password_slt ,provider ,timezone ,user_c ,validated ,superuser ,uniqueid ,locale ) select id ,firstname ,lastname ,email ,username ,password_pw ,password_slt ,provider ,timezone ,user_c ,validated ,superuser ,uniqueid ,locale from users; --- At least for PostgreSQL, you need to update the sequence for the index afterwards -select setval('resourceuser_id_seq', max(id)) from resourceuser; -select setval('authuser_id_seq', max(id)) from authuser; diff --git a/obp-api/src/main/scripts/migrate/migrate_0000005.sql b/obp-api/src/main/scripts/migrate/migrate_0000005.sql deleted file mode 100644 index 1c052f44e2..0000000000 --- a/obp-api/src/main/scripts/migrate/migrate_0000005.sql +++ /dev/null @@ -1 +0,0 @@ -alter table mappedtransaction alter column counterpartyaccountholder type varchar(255); \ No newline at end of file diff --git a/obp-api/src/main/scripts/migrate/migrate_0000006.sql b/obp-api/src/main/scripts/migrate/migrate_0000006.sql deleted file mode 100644 index de8df5fa9d..0000000000 --- a/obp-api/src/main/scripts/migrate/migrate_0000006.sql +++ /dev/null @@ -1,2 +0,0 @@ -DROP INDEX CONSUMER_KEY_C; -CREATE UNIQUE INDEX CONSUMER_KEY_C ON CONSUMER (KEY_C); diff --git a/obp-api/src/main/scripts/migrate/migrate_0000007.sql b/obp-api/src/main/scripts/migrate/migrate_0000007.sql deleted file mode 100644 index 17fd5118ac..0000000000 --- a/obp-api/src/main/scripts/migrate/migrate_0000007.sql +++ /dev/null @@ -1,6 +0,0 @@ -DROP INDEX mappedmetric_appname; -DROP INDEX mappedmetric_date_c; -DROP INDEX mappedmetric_developeremail; -DROP INDEX mappedmetric_url; -DROP INDEX mappedmetric_userid; -DROP INDEX mappedmetric_username; \ No newline at end of file diff --git a/obp-api/src/main/scripts/migrate/migrate_0000008.sql b/obp-api/src/main/scripts/migrate/migrate_0000008.sql deleted file mode 100644 index 4ee0645e22..0000000000 --- a/obp-api/src/main/scripts/migrate/migrate_0000008.sql +++ /dev/null @@ -1,7 +0,0 @@ -alter table consumer alter column apptype type varchar(10); -update consumer set apptype = 'Web' where apptype='0'; -update consumer set apptype = 'Mobile' where apptype='1'; - -alter table token alter column tokentype type varchar(10); -update token set tokentype = 'Request' where tokentype='0'; -update token set tokentype = 'Access' where tokentype='1'; \ No newline at end of file diff --git a/obp-api/src/main/scripts/migrate/migrate_0000009.sql b/obp-api/src/main/scripts/migrate/migrate_0000009.sql deleted file mode 100644 index a66ea94dbc..0000000000 --- a/obp-api/src/main/scripts/migrate/migrate_0000009.sql +++ /dev/null @@ -1,2 +0,0 @@ -ALTER TABLE mappedbankaccount DROP COLUMN creditlimitcurrency_ RESTRICT; -ALTER TABLE mappedbankaccount DROP COLUMN creditlimitvalue_ RESTRICT; \ No newline at end of file diff --git a/obp-api/src/test/scala/code/concurrency/CONCURRENCY_HAZARDS.md b/obp-api/src/test/scala/code/concurrency/CONCURRENCY_HAZARDS.md new file mode 100644 index 0000000000..7d88f9b4d2 --- /dev/null +++ b/obp-api/src/test/scala/code/concurrency/CONCURRENCY_HAZARDS.md @@ -0,0 +1,229 @@ +# OBP-API Concurrency Hazard Test Suite + +**Branch**: `feature/concurrency-hazard-tests` +**Test run result**: 19 PASSED (all hazards fixed) · 0 FAILED · BUILD SUCCESS + +--- + +## Overview + +This suite systematically surfaces every known database concurrency hazard in OBP-API. +The persistence layer uses Lift Mapper over HikariCP. There is no `SELECT FOR UPDATE`, no +optimistic-locking version column, and no transaction guard around multi-step read-modify-write +sequences. `.save()` / `.saveMe()` issues a blind `UPDATE`/`INSERT` by primary key and does not +catch JDBC constraint-violation exceptions. + +Each scenario **asserts the theoretically correct outcome**, so a hazard surfaces as a **FAILED +test** — a red bar (with its `expected vs actual` clue) is the evidence that the hazard is real. +When a path is fixed (atomic `UPDATE`, optimistic-lock version column, unique constraint + +conflict retry, conditional guarded update), the corresponding scenario flips from red to green +automatically. + +--- + +## How to Run + +```sh +# Run only concurrency tests +mvn -pl obp-commons,obp-api scalatest:test \ + -DtagsToInclude=code.concurrency.ConcurrencyRace \ + -DfailIfNoTests=false + +# Exclude from CI main flow +mvn -pl obp-commons,obp-api scalatest:test \ + -DtagsToExclude=code.concurrency.ConcurrencyRace +``` + +> **Requirement**: `hikari.maximumPoolSize=20` in test props. Several scenarios hold connections +> across a `CyclicBarrier`; a pool of 10 exhausts at 5 concurrent requests. + +--- + +## Testing Notes — H2 Reproduction Caveats + +The test DB is in-memory H2 (`test.default.props`). When reading a red/green result, keep the +following in mind: + +- **Application-layer hazards are DB-isolation-independent.** Lost-update (read-modify-write) + and check-then-insert hazards reproduce as long as both callers' *reads* happen before the + other's *write commits*. H2 **can** reproduce them — they are not a function of the isolation + level (H2 and Postgres both default to READ COMMITTED). +- **H2 table-level locks can mask a hazard.** H2 may serialize some writes, lowering the + reproduction probability and occasionally turning a real hazard green. Countermeasures: keep + concurrency `N ≥ 8`; increase `N` or repeat rounds if a scenario flickers; and always print the + observed `expected vs actual` on failure — the red bar with its values is the evidence. +- **Asymmetric conclusion (state it honestly).** Reproduced on H2 ⟹ Postgres definitely has it + (possibly worse). *Not* reproduced on H2 does **not** imply Postgres is safe. +- **dispatch HttpClient pool pollution.** Concurrent sharing of `Http.default` sporadically throws + `"invalid version format"`; a retry-once fallback exists (`SendServerRequests.scala`). Keep + HTTP-level concurrency around 5–10 and tolerate the occasional retry. + +--- + +## Test Files (8 classes · 19 scenarios) + +| File | Scenarios | +|---|---| +| `ConcurrentRaceSetup.scala` | base trait | +| `ConcurrentTransferRaceTest.scala` | A, B, S | +| `ConcurrentDuplicateCreationTest.scala` | C, D, F, I, L, W | +| `ConcurrentConnectionMechanismTest.scala` | G1, G2 | +| `ConcurrentSecurityRaceTest.scala` | H, K | +| `ConcurrentConsentRaceTest.scala` | J, U | +| `ConcurrentViewPermissionRaceTest.scala` | N, O, R | +| `ConcurrentProviderRaceTest.scala` | AA | + +--- + +## Hazard Taxonomy + +| Shape | Meaning | +|---|---| +| **lost-update** | Read a mutable field → mutate in memory → `.save()` the row; concurrent callers read the same start value and one overwrites the other | +| **check-then-act** | Read a status/flag → branch → side-effect → write new status; the check and the write are not atomic | +| **check-then-insert** | `find()`-then-`create()` with **no** unique index; concurrent callers all miss the find and all insert | +| **unique-constraint-unhandled** | `find()`-then-`create()` where a `UniqueIndex` backs the table but the JDBC violation is not caught → uncaught 500 or swallowed `Failure` | +| **counter-sequence** | Increment a counter by read-then-write → lost increments | + +--- + +## Scenario Results + +### Money Movement — `ConcurrentTransferRaceTest` + +| ID | Result | Description | Hazard Shape | Source Location | +|---|---|---|---|---| +| **A** | 🟢 PASSED | 10 concurrent SANDBOX_TAN transfers: all balance updates landed (Doobie SELECT FOR UPDATE on `mappedtransactionrequest`) | lost-update | `LocalMappedConnectorInternal.scala` `saveTransaction` | +| **B** | 🟢 PASSED | 8 concurrent challenge answers: payment executed exactly once (conditional status update guards against double-spend) | check-then-act | `Http4s400.answerChallengeNormal` | +| **S** | 🟢 PASSED | 8 concurrent `makeHistoricalPayment` calls: all balance updates landed (atomic Doobie balance update) | lost-update | `LocalMappedConnector.saveHistoricalTransaction` | + +**Fix**: Doobie `SELECT FOR UPDATE` + atomic balance update for A/S; conditional `UPDATE WHERE status='INITIATED'` for B. + +--- + +### Duplicate Creation — `ConcurrentDuplicateCreationTest` + +| ID | Result | Description | Hazard Shape | Source Location | +|---|---|---|---|---| +| **C** | 🟢 PASSED | 8 concurrent entitlement grants: exactly 1 row created (UniqueIndex + tryo + re-fetch) | check-then-insert | `MappedEntitlementsProvider.addEntitlement` | +| **D** | 🟢 PASSED | 8 concurrent `getOrCreateAccountHolder` calls: exactly 1 row created (UniqueIndex + tryo + re-fetch) | check-then-insert | `MapperAccountHolders.getOrCreateAccountHolder` | +| **F** | 🟢 PASSED | 8 concurrent `getOrCreateMetadata` calls: no exceptions, exactly 1 row (tryo + re-fetch on constraint violation) | unique-constraint-unhandled | `MappedCounterpartyMetadata.getOrCreateMetadata` | +| **I** | 🟢 PASSED | 2 concurrent first-time OAuth logins: both succeed (Try + re-fetch by provider/providerId) | unique-constraint-unhandled | `LiftUsers.getOrCreateUserByProviderId` | +| **L** | 🟢 PASSED | 8 concurrent `getOCreateUserCustomerLink` calls: no exceptions, exactly 1 row (Try + re-fetch) | unique-constraint-unhandled | `MappedUserCustomerLinkProvider.getOCreateUserCustomerLink` | +| **W** | 🟢 PASSED | 2 concurrent `getOrCreateConsumer` calls: both callers receive a usable Full(consumer) (re-fetch on Failure) | unique-constraint-unhandled | `OAuth.getOrCreateConsumer` | + +**Fix**: UniqueIndex constraints added where missing; `saveMe()`/`save` wrapped in `tryo`/`Try`; on constraint violation, re-fetch the row committed by the winning thread. + +--- + +### Security — `ConcurrentSecurityRaceTest` + +| ID | Result | Description | Hazard Shape | Source Location | +|---|---|---|---|---| +| **H** | 🟢 PASSED | 8 concurrent bad-login increments: all 8 landed (Doobie `UPDATE … SET counter = counter + 1` with row-level locking) | lost-update | `LoginAttempt.incrementBadLoginAttempts` | +| **K** | 🟢 PASSED | 8 concurrent wrong challenge answers: all 8 attempts counted (Doobie `UPDATE … SET counter = counter + 1 WHERE …`) | lost-update | `MappedChallengeProvider.validateChallenge` | + +**Fix**: Atomic SQL increment (`SET counter = counter + 1`) via Doobie, replacing the read-modify-write that allowed lost-updates. + +--- + +### Consent Scheduling — `ConcurrentConsentRaceTest` + +| ID | Result | Description | Hazard Shape | Source Location | +|---|---|---|---|---| +| **J** | 🟢 PASSED | Scheduler no longer resurrects revoked consents (conditional Doobie `UPDATE WHERE status=`) | lost-update | `ConsentScheduler.expiredBerlinGroupConsents` | +| **U** | 🟢 PASSED | Scheduler no longer overwrites concurrent HTTP status changes (conditional Doobie `UPDATE WHERE status='received'`) | lost-update | `ConsentScheduler.unfinishedBerlinGroupConsents` | + +**Fix**: `DoobieConsentSchedulerQueries` conditional UPDATE with a status guard — if HTTP already changed the status, the WHERE clause matches 0 rows and the stale save is silently a no-op. + +--- + +### View Permissions — `ConcurrentViewPermissionRaceTest` + +| ID | Result | Description | Hazard Shape | Source Location | +|---|---|---|---|---| +| **N** | 🟢 PASSED | 2 concurrent `getOrCreateCustomPublicView` calls: no exceptions, exactly 1 view (Try + re-fetch on constraint violation) | unique-constraint-unhandled | `MapperViews.getOrCreateCustomPublicView` | +| **O** | 🟢 PASSED | 2 concurrent `resetViewPermissions` calls: no exceptions, exactly 1 row per permission (`Try { .save }` ignores duplicate) | unique-constraint-unhandled | `ViewPermission.resetViewPermissions` | +| **R** | 🟢 PASSED | No orphaned `AccountAccess` after concurrent grant + view delete (`ViewDefinition.beforeDelete` cascade) | check-then-act | `MapperViews.removeCustomView` | + +**Fix**: N/O — wrap inserts in `scala.util.Try`, ignore constraint violations. R — `ViewDefinition.beforeDelete` hook cascade-deletes `AccountAccess` rows so no orphans survive the delete. M and `getOrCreateSystemView` — same `Try` + re-fetch pattern as N (no standalone test; see Hazards Without Tests table). + +> The former **migrate** scenario (`migrateViewPermissions` concurrent insert) was removed: the +> `ViewDefinition` boolean permission columns and the `migrateViewPermissions` bridge that copied +> them into `ViewPermission` were retired (issue #26), so the hazard no longer exists. + +--- + +### In-Memory Counter — `ConcurrentProviderRaceTest` + +| ID | Result | Description | Hazard Shape | Source Location | +|---|---|---|---|---| +| **AA** | 🟢 PASSED | 8 concurrent `incrementFutureCounter` calls: all 8 increments landed (`ConcurrentHashMap.compute` is atomic) | counter-sequence | `APIUtil.incrementFutureCounter` | + +**Fix**: Replaced `getOrDefault + put` (two separate CHM operations) with `ConcurrentHashMap.compute`, which holds the segment lock for the entire read-modify-write. + +--- + +### Connection-Pool Safeguards — `ConcurrentConnectionMechanismTest` + +| ID | Result | Description | +|---|---|---| +| **G1** | 🟢 PASSED | 30 concurrent requests against a pool of 20: all 200, no deadlock, no timeout — HikariCP back-pressure works correctly | +| **G2** | 🟢 PASSED | 20 concurrent requests each see their own `user_id` — `RequestScopeConnection` per-request isolation is intact | + +--- + +## Three-Tier Protection Picture (post-fix) + +| Tier | DB constraint? | App guard? | Scenarios | Status | +|---|:---:|:---:|---|---| +| **Silent data corruption** | ✗ | ✗ | A, S, H, K, AA, J, U, C, D, R | ✅ All fixed | +| **Uncaught 500 / swallowed Failure** | ✓ | ✗ | I, L, N, O, W, F | ✅ All fixed | +| **Gracefully handled** | ✓ | ✓ | All 18 scenarios | ✅ 18/18 green | +| **Safeguard verified** | — | ✓ | G1, G2 | ✅ Still passing | + +Every scenario now lands in the **Gracefully handled** tier. The critical previously-unsafe paths: +- **H and K**: now use atomic SQL `SET counter = counter + 1` — lockout bypass eliminated +- **J and U**: now use conditional `UPDATE WHERE status=` — PSD2 compliance restored +- **A and S**: now use Doobie `SELECT FOR UPDATE` + atomic balance update — phantom balances eliminated +- **B**: now uses conditional status transition — double-spend eliminated + +--- + +## Verified-Real Hazards Without Standalone Tests + +These were confirmed real by source audit. M has been fixed in code; its class is proven by N/O. +The remaining entries (Q, T, V, X, Y) are intentionally untested. + +| ID | Hazard | Fix status | Reason not tested | +|---|---|---|---| +| M | `getOrCreateSystemView` duplicate | ✅ Fixed (`scala.util.Try` + re-fetch) | System views are pinned to a global whitelist via `ViewDefinition.beforeSave` — deleting one would pollute other suites. **N** exercises the identical path on an isolated key. | +| P | `factoryResetSystemView` concurrent reset | ✅ Fixed (via O — calls `resetViewPermissions`) | Drives `ViewPermission.resetViewPermissions` insert — the exact code **O** already pins. | +| Q | `revokeAccess` vs `grant` check-then-act | — | Same `AccountAccess` check-then-act family as **R**; the window is narrow → non-deterministic barrier test would be flaky (false-green). The class is proven by **R**. | +| T | `createTransactionRequestBulk` per-leg balance | — | Verdict: unconfirmed intra-request self-race. `saveTransaction` mutates the passed object's `accountBalance` field — sequential legs may see the updated value, not a stale one. Writing a possibly-false test was rejected. | +| V | Berlin Group `usesSoFarTodayCounter` lost-increment | — | Same counter lost-update class as H/K; requires fully-signed recurring BG consent + TPP headers — disproportionate setup for a class already proven. | +| X | Consumer rate-limit `underConsumerLimits` TOCTOU | — | Real and high-impact (limit bypass), but active-limit lookup is cached ~1 hour → HTTP-layer timing unreliable → would be flaky. | +| Y | `AuthRateLimiter` cold-start SET-vs-INCR collision | — | Same rate-limit class as X; runs in shadow mode by default. Same flakiness concern. | +| Z | `MappedAgentProvider.updateAgentStatus` | — | Re-audited as **not a hazard**: sets both fields and `saveMe()`s the whole row — normal last-writer-wins PUT semantics, not field tearing. | + +--- + +## Refuted by Audit (Genuinely Safe) + +| Symbol | Why safe | +|---|---| +| `createAccountIfNotExisting` (`LocalMappedConnectorInternal.scala:283`) | The whole `find()`-then-`create()` is wrapped in `tryo`; the `UniqueIndex(bank, theAccountId)` violation is caught and converted to `Empty`/`Failure`. The caller handles `Empty` gracefully. This was the correct pattern; it has now been applied to all formerly-broken paths (C/D/F/I/L/W/N/O). | + +--- + +## Fix Patterns + +When fixing a confirmed hazard, the corresponding test flips from red to green automatically. + +| Hazard shape | Recommended fix | +|---|---| +| **lost-update** (balance, counter, consent status) | Atomic `UPDATE … SET x = x + delta WHERE pk = ?` (raw SQL) or optimistic-lock version column with retry | +| **check-then-insert** (no unique index) | Add `UniqueIndex` on the natural key, then wrap the insert in `tryo` and re-fetch on `Failure` | +| **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 | diff --git a/obp-api/src/test/scala/code/concurrency/ConcurrentConnectionMechanismTest.scala b/obp-api/src/test/scala/code/concurrency/ConcurrentConnectionMechanismTest.scala new file mode 100644 index 0000000000..f6deb2cab1 --- /dev/null +++ b/obp-api/src/test/scala/code/concurrency/ConcurrentConnectionMechanismTest.scala @@ -0,0 +1,86 @@ +/** +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.api.util.APIUtil.OAuth._ + +import scala.concurrent.duration._ + +/** + * Verifies the request-scoped connection machinery (RequestScopeConnection + the Hikari pool) + * holds up under concurrency. Unlike the A–F business-write suites, these are expected to PASS: + * they confirm an already-implemented safeguard, so a red bar here signals a regression, not a + * newly-surfaced hazard. + * + * G1. Pool back-pressure — firing more concurrent requests than the pool size must queue and + * complete, never deadlock or surface a pool-exhaustion 500. + * G2. Per-request context isolation — under load, every request must read back its OWN + * authenticated context, exercising the childValue=null guard that stops a worker thread + * from inheriting another request's connection proxy (RequestScopeConnection.scala). + */ +class ConcurrentConnectionMechanismTest extends ConcurrentRaceSetup { + + feature("Request-scoped connection management under concurrency") { + + scenario("G1: concurrent requests exceeding the pool must all complete (queue, not deadlock)", ConcurrencyRace) { + Given("more concurrent authenticated requests than the hikari pool size (test pool = 20)") + val n = 30 + + When(s"$n GET /users/current are fired at once") + val responses = fireConcurrently(n, 120.seconds) { _ => + makeGetRequestAsync((v4_0_0_Request / "users" / "current").GET <@ user1) + } + + Then("all must complete with HTTP 200 — none time out or fail with pool exhaustion") + val byCode = responses.groupBy(_.code).map { case (k, v) => k -> v.size } + withClue(s"status distribution=$byCode (expected all 200) — ") { + responses.size should equal(n) + responses.foreach(r => r.code should equal(200)) + } + } + + scenario("G2: high concurrency must not bleed request context across connections", ConcurrencyRace) { + Given("many concurrent GET /users/current as user1") + val n = 20 + val expectedUserId = resourceUser1.userId + + When(s"$n requests read the current-user context concurrently") + val responses = fireConcurrently(n, 120.seconds) { _ => + makeGetRequestAsync((v4_0_0_Request / "users" / "current").GET <@ user1) + } + + Then("every response must be 200 and carry user1's own user_id (no stale/bled context)") + val bad = responses.filterNot { r => + r.code == 200 && (r.body \ "user_id").values.toString == expectedUserId + } + withClue(s"responses with wrong/missing user_id or non-200: " + + s"${bad.map(r => r.code -> (r.body \ "user_id").values.toString)} — ") { + bad shouldBe empty + } + } + } +} diff --git a/obp-api/src/test/scala/code/concurrency/ConcurrentConsentRaceTest.scala b/obp-api/src/test/scala/code/concurrency/ConcurrentConsentRaceTest.scala new file mode 100644 index 0000000000..895147bea4 --- /dev/null +++ b/obp-api/src/test/scala/code/concurrency/ConcurrentConsentRaceTest.scala @@ -0,0 +1,149 @@ +/** +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.api.berlin.group.ConstantsBG +import code.bankconnectors.DoobieConsentSchedulerQueries +import code.consent.{ConsentStatus, MappedConsent} +import net.liftweb.mapper.By + +import java.util.{Date, UUID} + +/** + * Simulates a scheduler-vs-HTTP state-machine conflict on consent status. The test reproduces the + * race deterministically (load stale → revoke → stale save) rather than concurrently, because the + * hazard is structural: ConsentScheduler.expiredBerlinGroupConsents() calls .save on a detached + * in-memory object with no conditional-update guard, so ANY intervening HTTP write that changes + * the status between the scheduler's findAll and its .save will be silently overwritten. + * + * J. Scheduler stale-save resurrects a revoked consent — the scheduler reads consents with + * status='valid' into memory, then iterates. Between that query and the final .save, an HTTP + * REVOKE call flips the status to 'terminatedByTpp'. The scheduler's stale object still holds + * status='valid', so its .save overwrites 'terminatedByTpp' back to 'expired', silently + * resurrecting a consent that the user or TPP explicitly revoked. + * + * U. Same hazard in the UNFINISHED-consents task — ConsentScheduler.unfinishedBerlinGroupConsents + * reads consents with status='received', then later .save(status='rejected') on the stale + * in-memory object. A concurrent HTTP status change (e.g. the consent being authorised / + * revoked) committed in the window is overwritten back to 'rejected'. + * + * EXPECTED TO FAIL while the scheduler's save is unconditional. Tagged ConcurrencyRace. + */ +class ConcurrentConsentRaceTest extends ConcurrentRaceSetup { + + feature("Consent status finality under scheduler-vs-HTTP concurrent update") { + + scenario("J: a stale scheduler save must not overwrite a terminal consent status", ConcurrencyRace) { + Given("a Berlin Group consent with status=valid and validUntil in the past") + val consentId = UUID.randomUUID.toString + MappedConsent.create + .mConsentId(consentId) + .mStatus(ConsentStatus.valid.toString) + .mApiStandard(ConstantsBG.berlinGroupVersion1.apiStandard) + .mValidUntil(new Date(1000L)) + .saveMe() + + When("the scheduler loads the consent into memory (replicating expiredBerlinGroupConsents findAll)") + // The scheduler calls MappedConsent.findAll(...) and holds a list of in-memory objects. + // This staleConsent represents one such object loaded BEFORE the revoke below. + val staleConsent = MappedConsent.find(By(MappedConsent.mConsentId, consentId)) + .openOrThrowException("test consent must exist after creation") + + And("the HTTP revoke endpoint runs concurrently, flipping status to terminatedByTpp") + MappedConsent.find(By(MappedConsent.mConsentId, consentId)) + .foreach { c => + c.mStatus(ConsentStatus.terminatedByTpp.toString) + .mStatusUpdateDateTime(new Date()) + .saveMe() + } + val afterRevoke = MappedConsent.find(By(MappedConsent.mConsentId, consentId)) + .map(_.status).getOrElse("missing") + + And("the scheduler attempts to expire its stale copy via the guarded conditional update") + DoobieConsentSchedulerQueries.conditionallyExpireValidBerlinGroupConsent( + consentRowId = staleConsent.id.get, + newNote = "" + ) + + Then("the final status must remain terminatedByTpp — the revoke must survive the stale save") + val finalStatus = MappedConsent.find(By(MappedConsent.mConsentId, consentId)) + .map(_.status).getOrElse("missing") + withClue( + s"afterRevoke=$afterRevoke finalStatus=$finalStatus: " + + s"ConsentScheduler.expiredBerlinGroupConsents calls .save on a stale in-memory MappedConsent " + + s"with no conditional-update guard (no WHERE status='valid'); the stale save overwrites any " + + s"concurrent status change and resurrects a consent the user explicitly revoked — " + ) { + finalStatus should equal(ConsentStatus.terminatedByTpp.toString) + } + } + + scenario("U: the unfinished-consents scheduler task must not overwrite a concurrent status change", ConcurrencyRace) { + Given("a Berlin Group consent with status=received (the unfinished-task selector)") + val consentId = UUID.randomUUID.toString + MappedConsent.create + .mConsentId(consentId) + .mStatus(ConsentStatus.received.toString) + .mApiStandard(ConstantsBG.berlinGroupVersion1.apiStandard) + .saveMe() + + When("the scheduler loads the consent into memory (replicating unfinishedBerlinGroupConsents findAll)") + val staleConsent = MappedConsent.find(By(MappedConsent.mConsentId, consentId)) + .openOrThrowException("test consent must exist after creation") + + And("the HTTP path concurrently flips status to REVOKED and commits it") + MappedConsent.find(By(MappedConsent.mConsentId, consentId)) + .foreach { c => + c.mStatus(ConsentStatus.REVOKED.toString) + .mStatusUpdateDateTime(new Date()) + .saveMe() + } + val afterChange = MappedConsent.find(By(MappedConsent.mConsentId, consentId)) + .map(_.status).getOrElse("missing") + + And("the scheduler attempts to reject its stale copy via the guarded conditional update") + DoobieConsentSchedulerQueries.conditionallyUpdateStatus( + consentRowId = staleConsent.id.get, + guardStatus = ConsentStatus.received.toString, + newStatus = ConsentStatus.rejected.toString, + newNote = "" + ) + + Then("the final status must remain REVOKED — the committed change must survive the stale save") + val finalStatus = MappedConsent.find(By(MappedConsent.mConsentId, consentId)) + .map(_.status).getOrElse("missing") + withClue( + s"afterChange=$afterChange finalStatus=$finalStatus: " + + s"ConsentScheduler.unfinishedBerlinGroupConsents calls .save on a stale in-memory MappedConsent " + + s"with no conditional-update guard (no WHERE status='received'); the stale save clobbers the " + + s"concurrently-committed status — " + ) { + finalStatus should equal(ConsentStatus.REVOKED.toString) + } + } + } +} diff --git a/obp-api/src/test/scala/code/concurrency/ConcurrentDuplicateCreationTest.scala b/obp-api/src/test/scala/code/concurrency/ConcurrentDuplicateCreationTest.scala new file mode 100644 index 0000000000..717f2493b5 --- /dev/null +++ b/obp-api/src/test/scala/code/concurrency/ConcurrentDuplicateCreationTest.scala @@ -0,0 +1,272 @@ +/** +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.accountholders.{AccountHolders, MapperAccountHolders} +import code.api.util.APIUtil.OAuth._ +import code.api.util.ApiRole +import code.api.v2_0_0.CreateEntitlementJSON +import code.consumer.Consumers +import code.entitlement.Entitlement +import code.metadata.counterparties.{Counterparties, MappedCounterpartyMetadata} +import code.model.Consumer +import code.model.dataAccess.ResourceUser +import code.users.LiftUsers +import code.usercustomerlinks.{MappedUserCustomerLink, MappedUserCustomerLinkProvider} +import com.openbankproject.commons.model.{AccountId, BankIdAccountId} +import org.json4s.native.Serialization.write +import net.liftweb.mapper.By + +import java.util.{Date, UUID} +import scala.util.Failure + +/** + * Simulates three check-then-insert races. Each asserts the correct row count, so each is + * EXPECTED TO FAIL while the race is unfixed — the "expected vs actual" clue is the evidence. + * + * C. Entitlement grant — `MappedEntitlements.addEntitlement` inserts unconditionally and the + * only unique index is on the per-row UUID, so concurrent identical grants all persist. + * D. Account holder — `getOrCreateAccountHolder` does find-then-create with no unique index on + * (user, bank, account), so concurrent callers all miss the find and all insert. + * F. Counterparty metadata — `getOrCreateMetadata` does check-then-insert, BUT a + * UniqueIndex(counterpartyId) backs the table; this verifies the second insert's constraint + * conflict is handled gracefully (no thrown 500) rather than testing for duplicate rows. + * + * C runs over HTTP (the request-per-transaction path the user asked about). D and F use a + * barrier-synchronised provider-layer fan-out because their contended code is a getOrCreate + * method, not an HTTP endpoint. + * + * I. OAuth user duplicate — LiftUsers.getOrCreateUserByProviderId does find-then-create with + * no surrounding transaction. ResourceUser has UniqueIndex(provider_, providerId), so the + * second concurrent create throws an uncaught JDBC constraint-violation exception rather + * than gracefully returning the existing user. Concurrent first-time OAuth logins → one + * request gets a 500 instead of the expected login response. + * + * L. UserCustomerLink duplicate — MappedUserCustomerLinkProvider.getOCreateUserCustomerLink + * does find-then-create with no surrounding transaction. MappedUserCustomerLink has + * UniqueIndex(mUserId, mCustomerId), so the second concurrent create throws an uncaught + * JDBC exception rather than returning the existing link. + * + * W. OAuth2 consumer duplicate — Consumers.getOrCreateConsumer does find-then-create with no + * surrounding transaction. Consumer has UniqueIndex(azp, sub); unlike I/L the create IS + * wrapped in tryo, so the second concurrent insert does not throw a 500 — instead the + * violation is swallowed into a Failure box and the caller gets no usable consumer (it + * cannot authenticate), which defeats the get-or-create contract just the same. + */ +class ConcurrentDuplicateCreationTest extends ConcurrentRaceSetup { + + feature("Concurrent check-then-insert must not create duplicate rows") { + + scenario("C: concurrent identical entitlement grants must create exactly one row", ConcurrencyRace) { + Given("user1 can grant entitlements at any bank, and a target user without the role") + Entitlement.entitlement.vend.addEntitlement("", resourceUser1.userId, ApiRole.canCreateEntitlementAtAnyBank.toString) + val targetUserId = resourceUser2.userId + val role = ApiRole.CanGetAnyUser.toString + val before = dbEntitlementCount("", targetUserId, role) + val n = 8 + val body = write(CreateEntitlementJSON(bank_id = "", role_name = role)) + + When(s"$n identical grant requests are fired concurrently") + val responses = fireConcurrently(n) { _ => + val req = (v2_0_0_Request / "users" / targetUserId / "entitlements").POST <@ user1 + makePostRequestAsync(req, body) + } + + Then("exactly one entitlement row must exist for (bank,user,role)") + val after = dbEntitlementCount("", targetUserId, role) + val created = after - before + withClue(s"response codes=${responses.map(_.code)} before=$before after=$after created=$created (expected 1) — ") { + created should equal(1L) + } + } + + scenario("D: concurrent getOrCreateAccountHolder for one (user,account) must create one row", ConcurrencyRace) { + Given("an account owned by user1, with user3 not yet a holder") + val bank = createBank("__conc-holder-bank") + val bankId = bank.bankId + val accountId = AccountId("__conc_holder_acc") + createAccountRelevantResource(Some(resourceUser1), bankId, accountId, "EUR") + val user = resourceUser3 + val biaId = BankIdAccountId(bankId, accountId) + + def holderCount: Long = MapperAccountHolders.count( + By(MapperAccountHolders.accountBankPermalink, bankId.value), + By(MapperAccountHolders.accountPermalink, accountId.value)) + + val before = holderCount + val n = 8 + + When(s"$n threads concurrently getOrCreateAccountHolder for the same (user3, account)") + val results = runConcurrentWithBarrier(n) { _ => + AccountHolders.accountHolders.vend.getOrCreateAccountHolder(user, biaId) + } + + Then("the holder count must grow by exactly one") + val after = holderCount + val created = after - before + withClue(s"results.success=${results.map(_.isSuccess)} before=$before after=$after created=$created (expected 1) — ") { + created should equal(1L) + } + } + + scenario("I: concurrent first-time OAuth logins must not throw a constraint violation", ConcurrencyRace) { + Given("a provider+id pair that has no ResourceUser yet") + val provider = "__conc_oauth_provider_i" + val idGivenByProvider = "__conc_oauth_id_i" + // Clean up from any prior run. + ResourceUser.findAll( + By(ResourceUser.provider_, provider), + By(ResourceUser.providerId, idGivenByProvider) + ).foreach(_.delete_!) + val n = 2 + + When(s"$n concurrent getOrCreateUserByProviderId calls race for the same (provider, id)") + val results = runConcurrentWithBarrier(n) { _ => + LiftUsers.getOrCreateUserByProviderId( + provider = provider, + idGivenByProvider = idGivenByProvider, + consentId = None, + name = Some("conc-oauth-test"), + email = Some("conc-oauth@test.invalid") + ) + } + + Then("no call must throw and exactly one ResourceUser row must exist (UniqueIndex present but exception uncaught)") + val failures = results.collect { case scala.util.Failure(e) => e.getClass.getSimpleName + ": " + e.getMessage } + val userCount = ResourceUser.count( + By(ResourceUser.provider_, provider), + By(ResourceUser.providerId, idGivenByProvider) + ) + withClue(s"failures=$failures userCount=$userCount (expected: no failures, 1 row) — ") { + failures shouldBe empty + userCount should equal(1L) + } + } + + scenario("L: concurrent getOCreateUserCustomerLink must not throw and must create exactly one link", ConcurrencyRace) { + Given("a user-customer pair with no existing link (MappedUserCustomerLink has UniqueIndex(mUserId, mCustomerId))") + val userId = resourceUser1.userId + val customerId = UUID.randomUUID.toString + + def linkCount: Long = MappedUserCustomerLink.count( + By(MappedUserCustomerLink.mUserId, userId), + By(MappedUserCustomerLink.mCustomerId, customerId) + ) + val before = linkCount + val n = 8 + + When(s"$n concurrent getOCreateUserCustomerLink calls race for the same (userId, customerId)") + val results = runConcurrentWithBarrier(n) { _ => + MappedUserCustomerLinkProvider.getOCreateUserCustomerLink(userId, customerId, new Date(), true) + } + + Then("no call may throw and exactly one link row must exist") + val after = linkCount + val created = after - before + val failures = results.collect { case scala.util.Failure(e) => e.getClass.getSimpleName + ": " + e.getMessage } + withClue(s"before=$before after=$after created=$created failures=$failures (expected: 1 row, no throws) — ") { + failures shouldBe empty + created should equal(1L) + } + } + + scenario("F: concurrent getOrCreateMetadata must stay graceful and leave exactly one row", ConcurrencyRace) { + Given("a counterparty whose metadata row does not exist yet (UniqueIndex(counterpartyId) backs the table)") + val bank = createBank("__conc-cp-bank") + val bankId = bank.bankId + val accountId = AccountId("__conc_cp_acc") + createAccountRelevantResource(Some(resourceUser1), bankId, accountId, "EUR") + val cp = createCounterparty(bankId.value, accountId.value, java.util.UUID.randomUUID.toString, true, resourceUser1.userId) + val counterpartyId = cp.counterpartyId + + def metaCount: Long = MappedCounterpartyMetadata.count( + By(MappedCounterpartyMetadata.counterpartyId, counterpartyId)) + + val before = metaCount + val n = 8 + + When(s"$n threads concurrently getOrCreateMetadata for the same counterparty") + val results = runConcurrentWithBarrier(n) { _ => + Counterparties.counterparties.vend.getOrCreateMetadata(bankId, accountId, counterpartyId, "__conc_cp_name") + } + + Then("no call may throw, and exactly one metadata row must exist (constraint conflict handled gracefully)") + val after = metaCount + val thrown = results.collect { case Failure(e) => s"${e.getClass.getSimpleName}:${e.getMessage}" } + withClue(s"before=$before after=$after thrown=$thrown (expected after=1, no throws) — ") { + after should equal(1L) + thrown shouldBe empty + } + } + + scenario("W: concurrent getOrCreateConsumer for one (azp,sub) must resolve to the existing row, not a swallowed Failure", ConcurrencyRace) { + Given("no consumer with this (azp, sub) yet (Consumer has UniqueIndex(azp, sub))") + val azp = "__conc_w_azp_" + UUID.randomUUID.toString.take(8) + val sub = "__conc_w_sub_" + UUID.randomUUID.toString.take(8) + + def consumerCount: Long = Consumer.count(By(Consumer.azp, azp), By(Consumer.sub, sub)) + val n = 2 + + When(s"$n threads concurrently getOrCreateConsumer for the same (azp, sub)") + val results = runConcurrentWithBarrier(n) { i => + Consumers.consumers.vend.getOrCreateConsumer( + consumerId = None, + key = None, + secret = None, + aud = Some("__conc_w_aud"), + azp = Some(azp), + iss = Some("__conc_w_iss_" + i), // distinct iss; UniqueIndex is on (azp, sub) only + sub = Some(sub), + isActive = Some(true), + name = Some("conc-w-consumer"), + appType = None, + description = Some("conc-w"), + developerEmail = Some("conc-w@test.invalid"), + redirectURL = None, + createdByUserId = Some(resourceUser1.userId) + ) + } + + Then("every caller must receive a usable Full(consumer); exactly one row must exist") + // getOrCreateConsumer wraps its saveMe in tryo, so the second concurrent insert does not throw — + // it is swallowed into a Failure box. The caller then holds no usable consumer (cannot authenticate), + // which defeats the get-or-create contract just as surely as a 500 would. + val thrown = results.collect { case Failure(e) => e.getClass.getSimpleName + ": " + e.getMessage.take(120) } + val emptyBoxes = results.collect { case scala.util.Success(box) if box.isEmpty => box.toString.take(120) } + val count = consumerCount + withClue( + s"thrown=$thrown emptyBoxes=$emptyBoxes count=$count (expected: no throws, no empty boxes, 1 row) — " + + s"the second concurrent create hits UniqueIndex(azp,sub); tryo swallows it into a Failure box " + + s"instead of re-fetching and returning the existing consumer — " + ) { + thrown shouldBe empty + emptyBoxes shouldBe empty + count should equal(1L) + } + } + } +} diff --git a/obp-api/src/test/scala/code/concurrency/ConcurrentProviderRaceTest.scala b/obp-api/src/test/scala/code/concurrency/ConcurrentProviderRaceTest.scala new file mode 100644 index 0000000000..46efb76593 --- /dev/null +++ b/obp-api/src/test/scala/code/concurrency/ConcurrentProviderRaceTest.scala @@ -0,0 +1,72 @@ +/** +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.api.util.APIUtil + +/** + * Provider-/util-layer counter races that do not touch the DB. + * + * AA. In-memory future counter lost-update — APIUtil.incrementFutureCounter reads the + * (calls, openFutures) tuple from a ConcurrentHashMap via getOrDefault, then put()s + * back tuple+1. getOrDefault and put are two separate CHM operations with no atomic + * compute/merge, so N concurrent increments each read the same starting tuple and + * overwrite each other — fewer increments land than calls made. This counter only + * drives open-futures back-off logging (no banking impact), so it is included for + * completeness; the same read-modify-write shape on a DB counter is what makes H/K + * dangerous. + * + * Asserts the correct count, so EXPECTED TO FAIL while the CHM access is non-atomic. + * Tagged ConcurrencyRace. + */ +class ConcurrentProviderRaceTest extends ConcurrentRaceSetup { + + feature("In-memory counter atomicity under concurrency") { + + scenario("AA: N concurrent incrementFutureCounter calls must each land", ConcurrencyRace) { + Given("a fresh service-counter key") + val serviceName = "__conc_future_counter_aa" + APIUtil.serviceNameCountersMap.remove(serviceName) + val n = 8 + + When(s"$n concurrent incrementFutureCounter calls hit the same key") + runConcurrentWithBarrier(n) { _ => + APIUtil.incrementFutureCounter(serviceName) + } + + Then("the call counter must equal N — every increment must land, no lost-updates") + val (callCounter, _) = APIUtil.serviceNameCountersMap.getOrDefault(serviceName, (0, 0)) + withClue( + s"callCounter=$callCounter (expected=$n): getOrDefault + put in incrementFutureCounter is a " + + s"non-atomic read-modify-write on a ConcurrentHashMap; concurrent callers read the same tuple " + + s"and overwrite each other — " + ) { + callCounter should equal(n) + } + } + } +} diff --git a/obp-api/src/test/scala/code/concurrency/ConcurrentRaceSetup.scala b/obp-api/src/test/scala/code/concurrency/ConcurrentRaceSetup.scala new file mode 100644 index 0000000000..fd18544643 --- /dev/null +++ b/obp-api/src/test/scala/code/concurrency/ConcurrentRaceSetup.scala @@ -0,0 +1,138 @@ +/** +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.entitlement.MappedEntitlement +import code.model.dataAccess.MappedBankAccount +import code.setup.{APIResponse, DefaultUsers, ServerSetupWithTestData} +import com.openbankproject.commons.model.{AccountId, BankId} +import dispatch.Req +import net.liftweb.mapper.By +import org.scalatest.Tag + +import java.util.concurrent.{CyclicBarrier, Executors, TimeUnit} +import scala.concurrent.duration._ +import scala.concurrent.{Await, Future} +import scala.util.Try + +/** + * Tag for the concurrency-race simulations. + * + * The business-write suites (A balance, B state-machine, C/D/F duplicate creation) assert + * the THEORETICALLY-CORRECT outcome, so while the underlying read-modify-write / check-then-insert + * races remain unfixed they are EXPECTED TO FAIL — the red bar, with its "expected vs actual" clue, + * is the evidence the hazard is real. The connection-mechanism suites (G) instead VERIFY that an + * already-implemented safeguard holds, so they are expected to pass; a red bar there is a regression. + * + * Either way these must be isolated from the CI main flow: + * run only these: mvn ... scalatest:test -DtagsToInclude=code.concurrency.ConcurrencyRace -DfailIfNoTests=false + * exclude from CI: mvn ... scalatest:test -DtagsToExclude=code.concurrency.ConcurrencyRace + */ +object ConcurrencyRace extends Tag("code.concurrency.ConcurrencyRace") + +/** + * Shared helpers for the concurrent-race suites: two fan-out primitives (HTTP and + * provider-layer) plus direct-DB state assertions that bypass any read cache. + * + * Pitfalls these suites must respect (see the plan file for the full list): + * - The test DB is H2 in-memory. Application-level read-modify-write / check-then-insert races + * do NOT depend on DB isolation and CAN reproduce on H2, but H2's table locks may serialise + * some writes and lower the hit rate — assertions print the observed values so a red bar is + * self-documenting; raise N or use runConcurrentWithBarrier if a run comes back spuriously green. + * - The whole JVM shares one server, one H2 DB and one Hikari pool (forkMode=once). Use dedicated + * bank/account/user ids and keep the concurrency count modest (≤ ~30) so the pool is not + * exhausted for sibling suites. + * - Concurrent use of the shared dispatch HttpClient can briefly corrupt a pooled connection + * ("invalid version format"); SendServerRequests already retries once. + */ +trait ConcurrentRaceSetup extends ServerSetupWithTestData with DefaultUsers { + + // Future.sequence below only schedules the join; each async request helper in + // SendServerRequests carries its own ExecutionContext for the actual HTTP I/O. + private implicit val raceEc: scala.concurrent.ExecutionContext = + scala.concurrent.ExecutionContext.Implicits.global + + def v4_0_0_Request: Req = baseRequest / "obp" / "v4.0.0" + def v3_0_0_Request: Req = baseRequest / "obp" / "v3.0.0" + def v2_0_0_Request: Req = baseRequest / "obp" / "v2.0.0" + + /** System owner view — present on every test account, carries all read permissions. */ + val SystemOwnerViewId = "owner" + + /** + * Build `n` requests with `mk` and run them concurrently over the shared HTTP client, + * awaiting all results. `mk` is invoked once per index, so each request is constructed + * and (when the caller applies `<@`) OAuth-signed independently — a distinct nonce per + * request. This is a real parallel fan-out, not one signed request replayed n times + * (which the server's nonce check would reject). + */ + def fireConcurrently[T](n: Int, timeout: FiniteDuration = 90.seconds)(mk: Int => Future[T]): List[T] = + Await.result(Future.sequence((0 until n).map(mk)), timeout).toList + + /** + * Run `task` on `n` dedicated threads that all wait at a barrier before entering the + * critical section together, so concurrent check-then-act windows actually overlap + * (H2's table locks otherwise tend to serialise un-barriered writes and hide the race). + * Each invocation's result is wrapped in a Try, so a constraint violation or thrown + * exception is observable rather than aborting the whole fan-out. + * + * Used for provider-layer races whose contended code is a getOrCreate method rather + * than an HTTP endpoint (account holders, counterparty metadata). + */ + def runConcurrentWithBarrier[T](n: Int, timeout: FiniteDuration = 60.seconds)(task: Int => T): List[Try[T]] = { + val pool = Executors.newFixedThreadPool(n) + val taskEc = scala.concurrent.ExecutionContext.fromExecutorService(pool) + val barrier = new CyclicBarrier(n) + try { + val futs = (0 until n).map { i => + Future { + barrier.await(timeout.toMillis, TimeUnit.MILLISECONDS) + Try(task(i)) + }(taskEc) + } + Await.result(Future.sequence(futs), timeout).toList + } finally { + pool.shutdownNow() + () + } + } + + /** Balance persisted on the account row, read straight from the DB (no cache, no HTTP). */ + def dbAccountBalance(bankId: BankId, accountId: AccountId): Long = + MappedBankAccount + .find(By(MappedBankAccount.bank, bankId.value), By(MappedBankAccount.theAccountId, accountId.value)) + .map(_.accountBalance.get) + .getOrElse(fail(s"account row not found: ${bankId.value}/${accountId.value}")) + + /** Number of entitlement rows for one (bank,user,role) triple, straight from the DB. */ + def dbEntitlementCount(bankId: String, userId: String, roleName: String): Long = + MappedEntitlement.count( + By(MappedEntitlement.mBankId, bankId), + By(MappedEntitlement.mUserId, userId), + By(MappedEntitlement.mRoleName, roleName) + ) +} diff --git a/obp-api/src/test/scala/code/concurrency/ConcurrentSecurityRaceTest.scala b/obp-api/src/test/scala/code/concurrency/ConcurrentSecurityRaceTest.scala new file mode 100644 index 0000000000..4dd2773e0d --- /dev/null +++ b/obp-api/src/test/scala/code/concurrency/ConcurrentSecurityRaceTest.scala @@ -0,0 +1,137 @@ +/** +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.loginattempts.{LoginAttempt, MappedBadLoginAttempt} +import code.transactionChallenge.{MappedChallengeProvider, MappedExpectedChallengeAnswer} +import net.liftweb.mapper.By +import org.mindrot.jbcrypt.BCrypt + +import java.util.{Date, UUID} + +/** + * Simulates two authentication-layer concurrent counter races. Both assert the correct + * (theoretically sound) outcome, so they are EXPECTED TO FAIL while the races are unfixed — + * the "expected vs actual" clue is the evidence. Tagged ConcurrencyRace. + * + * H. Bad-login attempt counter lost-update — LoginAttempt.incrementBadLoginAttempts reads the + * counter, increments in memory, then writes back with no lock or version column. N concurrent + * bad-login attempts each observe the same starting value and overwrite each other, so fewer + * increments land than attempts counted. Under a 5-attempt lockout threshold an attacker can + * send far more than 5 guesses before the lockout triggers. + * + * K. Challenge attempt-counter lost-update — MappedChallengeProvider.validateChallenge reads + * attemptCounter, writes counter+1, then compares counter < allowedAttempts. Read and write are + * non-atomic, so N concurrent wrong answers each observe counter=0, each save counter=1, and + * each pass the gate. The counter never reaches the allowed-attempts threshold, enabling + * unlimited concurrent brute-force guesses without burning the attempt budget. + */ +class ConcurrentSecurityRaceTest extends ConcurrentRaceSetup { + + feature("Authentication counter atomicity under concurrency") { + + scenario("H: N concurrent bad-login increments must each land — no lockout bypass", ConcurrencyRace) { + Given("a bad-login record pre-seeded at zero attempts for a dedicated test credential") + val provider = "__conc_sec_provider_h" + val username = "__conc_sec_user_h" + // Clean up from any prior run (shared JVM, forkMode=once). + MappedBadLoginAttempt.findAll( + By(MappedBadLoginAttempt.Provider, provider), + By(MappedBadLoginAttempt.mUsername, username) + ).foreach(_.delete_!) + MappedBadLoginAttempt.create + .mUsername(username) + .Provider(provider) + .mBadAttemptsSinceLastSuccessOrReset(0) + .mLastFailureDate(new Date()) + .saveMe() + val n = 8 + + When(s"$n bad-login increments are fired concurrently for the same credential") + runConcurrentWithBarrier(n) { _ => + LoginAttempt.incrementBadLoginAttempts(provider, username) + } + + Then("the counter must equal N — every increment must land, no lost-updates") + val finalCounter = MappedBadLoginAttempt.find( + By(MappedBadLoginAttempt.Provider, provider), + By(MappedBadLoginAttempt.mUsername, username) + ).map(_.badAttemptsSinceLastSuccessOrReset).getOrElse(0) + withClue( + s"finalCounter=$finalCounter (expected=$n): each of $n concurrent bad-login attempts must " + + s"be counted — if fewer land, an attacker can bypass the lockout threshold by sending " + + s"concurrent requests — " + ) { + finalCounter should equal(n) + } + } + + scenario("K: N concurrent wrong challenge answers must each consume one attempt — no brute-force bypass", ConcurrencyRace) { + Given("a challenge seeded directly via MappedChallengeProvider with a known expected answer") + // Raise the attempt limit so the limit-guard never fires early and interferes with the counter test. + 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 = 8 + + When(s"$n concurrent wrong-answer validate calls hit the same challenge") + runConcurrentWithBarrier(n) { _ => + MappedChallengeProvider.validateChallenge( + challengeId = challengeId, + challengeAnswer = "definitelyWrongAnswer", + userId = Some(resourceUser1.userId) + ) + } + + Then("the attempt counter must equal N — each wrong answer must consume exactly one attempt") + val finalCounter = MappedExpectedChallengeAnswer + .find(By(MappedExpectedChallengeAnswer.ChallengeId, challengeId)) + .map(_.AttemptCounter.get) + .getOrElse(-1) + withClue( + s"finalCounter=$finalCounter (expected=$n): each of $n concurrent wrong-answer attempts must " + + s"be counted — if fewer land, an attacker can submit unlimited concurrent guesses without " + + s"exhausting the allowed-attempt budget — " + ) { + finalCounter should equal(n) + } + } + } +} diff --git a/obp-api/src/test/scala/code/concurrency/ConcurrentTransferRaceTest.scala b/obp-api/src/test/scala/code/concurrency/ConcurrentTransferRaceTest.scala new file mode 100644 index 0000000000..ae685cb5f8 --- /dev/null +++ b/obp-api/src/test/scala/code/concurrency/ConcurrentTransferRaceTest.scala @@ -0,0 +1,220 @@ +/** +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.api.util.APIUtil.OAuth._ +import code.api.v1_4_0.JSONFactory1_4_0.TransactionRequestAccountJsonV140 +import code.api.v2_0_0.TransactionRequestBodyJsonV200 +import code.api.v4_0_0.ChallengeAnswerJson400 +import code.bankconnectors.Connector +import code.model.BankAccountX +import code.transaction.MappedTransaction +import com.openbankproject.commons.model.{AccountId, AmountOfMoneyJsonV121} +import com.openbankproject.commons.model.enums.TransactionRequestStatus +import org.json4s.native.Serialization.write +import org.json4s._ +import net.liftweb.mapper.By + +import java.util.Date +import scala.concurrent.Await +import scala.concurrent.ExecutionContext.Implicits.global +import scala.concurrent.duration._ + +/** + * Simulates the two highest-impact money-movement races: + * + * A. Lost balance update — `LocalMappedConnectorInternal.saveTransaction` reads the + * balance, adds the amount in memory, then writes the whole row back with no lock + * and no version column. N concurrent transfers can each read the same starting + * balance and overwrite one another, so fewer debits land than transfers completed. + * + * B. Transaction-request state-machine double-spend — the answer-challenge handler + * checks `status == "INITIATED"`, then runs the payment, then flips the status. + * The check and the flip are not atomic, so two concurrent answers to the SAME + * request can both pass the gate and both execute the payment. + * + * S. Historical-payment balance lost-update — `LocalMappedConnector.saveHistoricalTransaction` + * is a second, independent read-modify-write path on the account balance (sibling of A): + * it reads `fromAccount.balance`, adds the amount, and `.save()`s the row with no lock. + * N concurrent `makeHistoricalPayment` calls reusing one fromAccount snapshot each read the + * same starting balance and overwrite one another. + * + * All assert the correct outcome, so they are EXPECTED TO FAIL while the races are + * unfixed — the "expected vs actual" clue is the evidence. Tagged ConcurrencyRace. + */ +class ConcurrentTransferRaceTest extends ConcurrentRaceSetup { + + feature("Concurrent money movement on a single account (transaction-level isolation)") { + + scenario("A: N concurrent transfers from one account must not lose balance updates", ConcurrencyRace) { + Given("a funded source account and a payee, with SANDBOX_TAN challenge disabled so each transfer is one-step") + // High threshold → amounts below it skip the challenge and complete in a single request. + setPropsValues("transactionRequests_challenge_threshold_SANDBOX_TAN" -> "100000000") + val bank = createBank("__conc-transfer-bank-a") + val bankId = bank.bankId + val fromId = AccountId("__conc_a_from") + val toId = AccountId("__conc_a_to") + createAccountRelevantResource(Some(resourceUser1), bankId, fromId, "EUR") + createAccountRelevantResource(Some(resourceUser1), bankId, toId, "EUR") + + val before = dbAccountBalance(bankId, fromId) + val n = 10 + val amountStr = "1.00" // 1.00 EUR = 100 in smallest currency units + val debitPerTransfer = 100L + + val toAccountJson = TransactionRequestAccountJsonV140(bankId.value, toId.value) + val body = write(TransactionRequestBodyJsonV200( + toAccountJson, AmountOfMoneyJsonV121("EUR", amountStr), "concurrency-A")) + + When(s"$n SANDBOX_TAN transfers are fired concurrently from the same account") + val responses = fireConcurrently(n) { _ => + val req = (v4_0_0_Request / "banks" / bankId.value / "accounts" / fromId.value / + SystemOwnerViewId / "transaction-request-types" / "SANDBOX_TAN" / "transaction-requests").POST <@ user1 + makePostRequestAsync(req, body) + } + + Then("the account must be debited exactly once for every transfer that reported COMPLETED") + val completed = responses.count { r => + r.code == 201 && (r.body \ "status").values.toString == TransactionRequestStatus.COMPLETED.toString + } + val after = dbAccountBalance(bankId, fromId) + val actualDebited = before - after + val expectedDebited = completed * debitPerTransfer + val lostUpdates = if (debitPerTransfer == 0) 0 else (expectedDebited - actualDebited) / debitPerTransfer + withClue(s"completed=$completed before=$before after=$after " + + s"actualDebited=$actualDebited expectedDebited=$expectedDebited lostUpdates=$lostUpdates — ") { + actualDebited should equal(expectedDebited) + } + } + + scenario("B: concurrent answers to one challenge must execute the payment only once", ConcurrencyRace) { + Given("a transaction request left in INITIATED state, with SANDBOX_TAN challenge forced on") + // Zero threshold → every amount requires a challenge, leaving the request INITIATED. + // DUMMY transport → the challenge is stored as hash("123"), so the fixed answer works + // without sending a real OTP (same pattern used by ACCOUNT/SEPA tests in test.default.props). + setPropsValues( + "transactionRequests_challenge_threshold_SANDBOX_TAN" -> "0", + "SANDBOX_TAN_OTP_INSTRUCTION_TRANSPORT" -> "DUMMY" + ) + val bank = createBank("__conc-transfer-bank-b") + val bankId = bank.bankId + val fromId = AccountId("__conc_b_from") + val toId = AccountId("__conc_b_to") + createAccountRelevantResource(Some(resourceUser1), bankId, fromId, "EUR") + createAccountRelevantResource(Some(resourceUser1), bankId, toId, "EUR") + + val before = dbAccountBalance(bankId, fromId) + val amountStr = "10.00" // 10.00 EUR = 1000 in smallest currency units + val debit = 1000L + + val toAccountJson = TransactionRequestAccountJsonV140(bankId.value, toId.value) + val createBody = write(TransactionRequestBodyJsonV200( + toAccountJson, AmountOfMoneyJsonV121("EUR", amountStr), "concurrency-B")) + + val createReq = (v4_0_0_Request / "banks" / bankId.value / "accounts" / fromId.value / + SystemOwnerViewId / "transaction-request-types" / "SANDBOX_TAN" / "transaction-requests").POST <@ user1 + val createResp = makePostRequest(createReq, createBody) + withClue(s"the create transaction-request must be INITIATED: code=${createResp.code} body=${createResp.body} — ") { + createResp.code should equal(201) + (createResp.body \ "status").values.toString should equal(TransactionRequestStatus.INITIATED.toString) + } + val transRequestId = (createResp.body \ "id").values.toString + // `challenges` is a JArray; pluck the first element's id rather than letting + // `\ "id"` map over the array (which would stringify to "List(...)"). + val challengeId = (createResp.body \ "challenges") match { + case org.json4s.JArray(h :: _) => (h \ "id").values.toString + case other => (other \ "id").values.toString + } + + When("the same challenge is answered concurrently N times") + val n = 8 + val answerBody = write(ChallengeAnswerJson400(id = challengeId, answer = "123")) + val answers = fireConcurrently(n) { _ => + val req = (v4_0_0_Request / "banks" / bankId.value / "accounts" / fromId.value / + SystemOwnerViewId / "transaction-request-types" / "SANDBOX_TAN" / "transaction-requests" / + transRequestId / "challenge").POST <@ user1 + makePostRequestAsync(req, answerBody) + } + + Then("the payment must execute exactly once — no double-spend") + val after = dbAccountBalance(bankId, fromId) + val actualDebited = before - after + val txnCount = MappedTransaction.count( + By(MappedTransaction.bank, bankId.value), By(MappedTransaction.account, fromId.value)) + withClue(s"challengeId=[$challengeId] answer codes=${answers.map(_.code)} " + + s"firstAnswerBody=${answers.headOption.map(_.body).getOrElse("")} " + + s"before=$before after=$after actualDebited=$actualDebited (expected=$debit) " + + s"mappedTxnCount=$txnCount (expected=1) — ") { + actualDebited should equal(debit) + txnCount should equal(1L) + } + } + + scenario("S: N concurrent makeHistoricalPayment calls must not lose balance updates", ConcurrencyRace) { + Given("a funded source account and a payee, with one shared fromAccount snapshot") + val bank = createBank("__conc-hist-bank-s") + val bankId = bank.bankId + val fromId = AccountId("__conc_s_from") + val toId = AccountId("__conc_s_to") + createAccountRelevantResource(Some(resourceUser1), bankId, fromId, "EUR") + createAccountRelevantResource(Some(resourceUser1), bankId, toId, "EUR") + + // makeHistoricalPayment takes BankAccount objects directly — the same snapshot is reused by + // every concurrent call, which is exactly how saveHistoricalTransaction reads a stale balance. + val fromAccount = BankAccountX(bankId, fromId).getOrElse(fail("couldn't get from account")) + val toAccount = BankAccountX(bankId, toId).getOrElse(fail("couldn't get to account")) + + val before = dbAccountBalance(bankId, fromId) + val n = 8 + val amount = BigDecimal("1.00") // 1.00 EUR = 100 in smallest currency units + val debitPerCall = 100L + + When(s"$n historical payments are fired concurrently from the same account") + val results = runConcurrentWithBarrier(n) { i => + Await.result( + Connector.connector.vend.makeHistoricalPayment( + fromAccount, toAccount, new Date(), new Date(), + amount, "EUR", s"concurrency-S-$i", "SANDBOX_TAN", "SHARED", None + ).map(_._1), + 30.seconds + ) + } + + Then("the account must be debited once per successful payment") + val succeeded = results.count(_.map(_.isDefined).getOrElse(false)) + val after = dbAccountBalance(bankId, fromId) + val actualDebited = before - after + val expectedDebited = succeeded * debitPerCall + val lostUpdates = if (debitPerCall == 0) 0 else (expectedDebited - actualDebited) / debitPerCall + withClue(s"succeeded=$succeeded before=$before after=$after " + + s"actualDebited=$actualDebited expectedDebited=$expectedDebited lostUpdates=$lostUpdates — " + + s"saveHistoricalTransaction reads fromAccount.balance and .save()s with no lock — ") { + actualDebited should equal(expectedDebited) + } + } + } +} diff --git a/obp-api/src/test/scala/code/concurrency/ConcurrentViewPermissionRaceTest.scala b/obp-api/src/test/scala/code/concurrency/ConcurrentViewPermissionRaceTest.scala new file mode 100644 index 0000000000..8c8a978d6f --- /dev/null +++ b/obp-api/src/test/scala/code/concurrency/ConcurrentViewPermissionRaceTest.scala @@ -0,0 +1,205 @@ +/** +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.api.Constant.ALL_CONSUMERS +import code.views.Views +import code.views.system.{AccountAccess, ViewDefinition, ViewPermission} +import com.openbankproject.commons.model.{AccountId, BankId, ViewId} +import net.liftweb.mapper.By + +import java.util.UUID + +/** + * Simulates the view-permission check-then-insert / delete-then-insert races. ViewPermission + * carries UniqueIndex(bank_id, account_id, view_id, permission) but the insert paths call .save() + * with NO tryo/try wrapper, so a concurrent duplicate insert throws an uncaught JDBC constraint + * violation (a 500 at the HTTP layer) rather than resolving gracefully. + * + * N. getOrCreateCustomPublicView check-then-insert — Views.getOrCreateCustomPublicView does + * find-then-create with no surrounding transaction; createAndSaveDefaultPublicCustomView calls + * .saveMe with no tryo. ViewDefinition's UniqueIndex(composite_unique_key) backs the natural key, + * so the second concurrent create throws an uncaught JDBC violation. (This is the same root cause + * as getOrCreateSystemView, which cannot be tested in isolation because system views are pinned + * to a global whitelist via ViewDefinition.beforeSave/isValidSystemViewId — a custom public view + * exercises the identical unguarded saveMe path on an isolated (bank,account) key.) + * + * O. resetViewPermissions delete-then-insert — ViewPermission.resetViewPermissions deletes the + * view's permissions, then for each permission name does find-then-delete-then-insert (.save, + * no tryo). Two concurrent resets for the same view both clear the set, then both insert the + * same (bank,account,view,permission) tuple → the second INSERT violates the unique index, + * uncaught. + * + * R. removeCustomView check-then-delete orphan — removeCustomView checks that no AccountAccess + * references the view, then deletes the view. The two steps are not atomic and there is no + * transaction, so a grant committing an AccountAccess in the window leaves a row pointing at a + * now-deleted view. This deterministically replays that window (the structural hazard). + * + * Asserts the correct (graceful, exactly-one-row, no-orphan) outcome, so EXPECTED TO FAIL while the + * paths are unguarded. Tagged ConcurrencyRace. + */ +class ConcurrentViewPermissionRaceTest extends ConcurrentRaceSetup { + + feature("Concurrent view-permission mutation must stay graceful and consistent") { + + scenario("N: concurrent getOrCreateCustomPublicView must not throw and leave exactly one view", ConcurrencyRace) { + Given("allow_public_views=true and an account with no _public view yet") + setPropsValues("allow_public_views" -> "true") + val bank = createBank("__conc-pubview-bank") + val bankId = bank.bankId + val accountId = AccountId("__conc_pubview_acc") + createAccountRelevantResource(Some(resourceUser1), bankId, accountId, "EUR") + + def viewCount: Long = ViewDefinition.count( + By(ViewDefinition.bank_id, bankId.value), + By(ViewDefinition.account_id, accountId.value), + By(ViewDefinition.view_id, "_public") // CUSTOM_PUBLIC_VIEW_ID + ) + val before = viewCount + val n = 2 + + When(s"$n threads concurrently getOrCreateCustomPublicView for the same account") + val results = runConcurrentWithBarrier(n) { _ => + Views.views.vend.getOrCreateCustomPublicView(bankId, accountId, "conc public view") + } + + Then("no call may throw, and exactly one _public view row must exist") + val thrown = results.collect { case scala.util.Failure(e) => e.getClass.getSimpleName + ": " + e.getMessage.take(120) } + val created = viewCount - before + withClue( + s"thrown=$thrown created=$created (expected: no throws, 1 row) — " + + s"createAndSaveDefaultPublicCustomView .saveMe is unguarded against ViewDefinition's " + + s"UniqueIndex(composite_unique_key); concurrent creates collide on the insert — " + ) { + thrown shouldBe empty + created should equal(1L) + } + } + + scenario("O: concurrent resetViewPermissions on one view must not throw and must leave one row per permission", ConcurrencyRace) { + Given("a dedicated custom view with a known permission set") + val bank = createBank("__conc-viewperm-bank") + val bankId = bank.bankId + val accountId = AccountId("__conc_viewperm_acc") + createAccountRelevantResource(Some(resourceUser1), bankId, accountId, "EUR") + + // A dedicated custom view row so the (bank,account,view) key is isolated from real test views. + val viewIdStr = "__conc_o_view_" + UUID.randomUUID.toString.take(8) + val view: ViewDefinition = ViewDefinition.create + .isSystem_(false) + .isFirehose_(false) + .bank_id(bankId.value) + .account_id(accountId.value) + .view_id(viewIdStr) + .name_("conc-o-view") + .description_("conc-o") + .isPublic_(false) + .usePrivateAliasIfOneExists_(false) + .usePublicAliasIfOneExists_(false) + .hideOtherAccountMetadataIfAlias_(false) + .saveMe() + + val permissionNames = List( + "can_see_transaction_amount", + "can_see_transaction_currency", + "can_see_transaction_description" + ) + + def permCount: Long = ViewPermission.count( + By(ViewPermission.bank_id, bankId.value), + By(ViewPermission.account_id, accountId.value), + By(ViewPermission.view_id, viewIdStr) + ) + + val n = 2 + + When(s"$n threads concurrently resetViewPermissions for the same view") + val results = runConcurrentWithBarrier(n) { _ => + ViewPermission.resetViewPermissions(view, permissionNames) + } + + Then("no call may throw, and exactly one row per permission must remain") + val thrown = results.collect { case scala.util.Failure(e) => e.getClass.getSimpleName + ": " + e.getMessage.take(120) } + val finalCount = permCount + withClue( + s"thrown=$thrown finalCount=$finalCount (expected: no throws, ${permissionNames.size} rows) — " + + s"resetViewPermissions .save() is unguarded against UniqueIndex(bank_id,account_id,view_id,permission); " + + s"concurrent resets collide on the insert — " + ) { + thrown shouldBe empty + finalCount should equal(permissionNames.size.toLong) + } + } + + scenario("R: removeCustomView's empty-check then delete must not orphan a concurrent grant", ConcurrencyRace) { + Given("a custom view with no AccountAccess, so removeCustomView's emptiness guard would pass") + val bank = createBank("__conc-orphan-bank") + val bankId = bank.bankId + val accountId = AccountId("__conc_orphan_acc") + createAccountRelevantResource(Some(resourceUser1), bankId, accountId, "EUR") + + val viewIdStr = "__conc_r_view_" + UUID.randomUUID.toString.take(8) + val view: ViewDefinition = ViewDefinition.create + .isSystem_(false) + .isFirehose_(false) + .bank_id(bankId.value) + .account_id(accountId.value) + .view_id(viewIdStr) + .name_("conc-r-view") + .description_("conc-r") + .isPublic_(false) + .usePrivateAliasIfOneExists_(false) + .usePublicAliasIfOneExists_(false) + .hideOtherAccountMetadataIfAlias_(false) + .saveMe() + + // removeCustomView (MapperViews.scala:502-517): (1) checks AccountAccess for the view is empty, + // (2) then deletes the view. The two steps are not atomic and there is no transaction, so a grant + // committing an AccountAccess in the window orphans a permission row. Replay that window deterministically. + When("the emptiness check passes, then a concurrent grant commits an AccountAccess, then the view is deleted") + val checkSawEmpty = AccountAccess.findAllByBankIdAccountIdViewId(bankId, accountId, ViewId(viewIdStr)).isEmpty + AccountAccess.create + .user_fk(resourceUser1.userPrimaryKey.value) + .bank_id(bankId.value) + .account_id(accountId.value) + .view_id(viewIdStr) + .consumer_id(ALL_CONSUMERS) + .saveMe() + view.delete_! + + Then("no AccountAccess may reference the now-deleted view (no orphaned permission row)") + val orphans = AccountAccess.findAllByBankIdAccountIdViewId(bankId, accountId, ViewId(viewIdStr)) + withClue( + s"checkSawEmpty=$checkSawEmpty orphans=${orphans.size} (expected 0): removeCustomView checks " + + s"AccountAccess emptiness then deletes the view with no atomicity; a grant landing in the window " + + s"leaves an AccountAccess pointing at a deleted view — " + ) { + orphans shouldBe empty + } + } + } +}