From 25f384baf550ba0eedd7608c9711ee4be3f8c572 Mon Sep 17 00:00:00 2001 From: simonredfern Date: Mon, 21 Sep 2026 23:44:57 +0200 Subject: [PATCH 1/5] Use SYS instead of null for bank_id in a system level dynamic entitiy. To solve issue with unique dynamic entity ids when not using UUID e.g. two letter country codes in different dynamic entities (at same bank or not) --- .../main/scala/bootstrap/liftweb/Boot.scala | 4 + .../scala/code/api/constant/constant.scala | 24 +++ .../dynamic/entity/Http4sDynamicEntity.scala | 30 ++-- .../entity/projection/ProjectionStore.scala | 15 +- .../main/scala/code/api/util/NewStyle.scala | 2 +- .../code/api/util/migration/Migration.scala | 155 +++++++++++++++++ .../DynamicDataAccessProvider.scala | 43 +++-- .../MappedDynamicDataAccessProvider.scala | 103 +++++++---- .../MapppedDynamicDataProvider.scala | 38 ++-- ...ynamicEntityJoinQueryIntegrationTest.scala | 5 +- .../DynamicEntitySystemLevelBankIdTest.scala | 163 ++++++++++++++++++ 11 files changed, 493 insertions(+), 89 deletions(-) create mode 100644 obp-api/src/test/scala/code/api/v6_0_0/DynamicEntitySystemLevelBankIdTest.scala diff --git a/obp-api/src/main/scala/bootstrap/liftweb/Boot.scala b/obp-api/src/main/scala/bootstrap/liftweb/Boot.scala index 94e6049506..90b92794eb 100644 --- a/obp-api/src/main/scala/bootstrap/liftweb/Boot.scala +++ b/obp-api/src/main/scala/bootstrap/liftweb/Boot.scala @@ -280,6 +280,10 @@ class Boot extends MdcLoggable { // 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() + // Same reasoning, for the Dynamic Entity tables: their unique indexes are becoming + // space-scoped, which needs the system level rows off SQL NULL and the superseded + // single-column indexes dropped, both before Schemifier issues the new index DDL. + Migration.database.prepareDynamicEntitySpaceScopedIndexes() schemifyAll() logger.info("Mapper database info: " + Migration.DbFunction.mapperDatabaseInfo) diff --git a/obp-api/src/main/scala/code/api/constant/constant.scala b/obp-api/src/main/scala/code/api/constant/constant.scala index 020b134fe2..fb8dc9ca94 100644 --- a/obp-api/src/main/scala/code/api/constant/constant.scala +++ b/obp-api/src/main/scala/code/api/constant/constant.scala @@ -58,6 +58,30 @@ object Constant extends MdcLoggable { // identified by their group_id. final val group_membership = "GROUP_MEMBERSHIP" + /** + * This is the value the bank id column holds for a Dynamic Entity record that belongs to no bank. + * + * A Dynamic Entity record is either scoped to one bank, which the Dynamic Entity feature calls a + * space, or it is system level and belongs to the instance as a whole. The system level case used + * to be written as a SQL NULL. That could not stay, because the uniqueness of a record's id has to + * be enforced per space rather than across the whole instance: two spaces may each legitimately + * hold a record whose natural key is the country code DE, and before this change the second one + * was refused. Postgres treats NULLs as distinct inside a unique index, so a composite unique + * index over the bank id would have stopped enforcing anything at all for the system level rows -- + * the very rows every existing instance is full of. Writing a real value instead removes that + * exception, and the index in DynamicData.dbIndexes can then say plainly what it means. + * + * The value is three characters long, and every endpoint that accepts a caller supplied bank id + * requires at least four (APIUtil.checkShortString plus a per endpoint minimum length check), so + * no caller can create a bank that collides with it. DynamicEntitySystemLevelBankIdTest holds that + * property down, because the minimum length check is written out separately in each endpoint + * rather than shared, and so is the part of the rule most likely to drift. + * + * This value is an internal storage detail and is never published. DynamicData.bankId filters it + * back out, so every reader still sees None for a system level record exactly as before. + */ + final val DYNAMIC_ENTITY_SYSTEM_LEVEL_BANK_ID = "SYS" + object Pagination { final val offset = 0 final val limit = 50 diff --git a/obp-api/src/main/scala/code/api/dynamic/entity/Http4sDynamicEntity.scala b/obp-api/src/main/scala/code/api/dynamic/entity/Http4sDynamicEntity.scala index 460847187b..c1b19fe8eb 100644 --- a/obp-api/src/main/scala/code/api/dynamic/entity/Http4sDynamicEntity.scala +++ b/obp-api/src/main/scala/code/api/dynamic/entity/Http4sDynamicEntity.scala @@ -406,7 +406,7 @@ object Http4sDynamicEntity extends MdcLoggable { // In-memory floor: fetch all rows (unscoped) and keep those the ACL marks readable. // (The projection EXISTS backend for row-level get-all is a documented follow-up; the // in-memory path is always correct, just not index-accelerated.) - val readable = aclVend.getReadableDynamicDataIds(u.userId, entityName, bankId).toSet + val readable = aclVend.getReadableDynamicDataIds(bankId, entityName, u.userId).toSet val readableRows = dataVend.getAllCommunity(bankId, entityName).filter(_.dynamicDataId.exists(readable.contains)) val readableJson: JArray = JArray(readableRows.map(r => parse(r.dataJson))) val filtered = filterDynamicObjects(readableJson, queryParams(req)) @@ -416,7 +416,7 @@ object Http4sDynamicEntity extends MdcLoggable { for { // Hide existence: a row you cannot read is indistinguishable from a missing row (404). _ <- Helper.booleanToFuture(notFoundMsg(entityName, id, bankId), 404, cc = callContext) { - box.isDefined && aclVend.allows(id, u.userId, DynamicDataAccessPermission.Read) + box.isDefined && aclVend.allows(bankId, entityName, id, u.userId, DynamicDataAccessPermission.Read) } } yield { val singleObject: JValue = unboxResult(box, entityName) @@ -438,7 +438,7 @@ object Http4sDynamicEntity extends MdcLoggable { existing: Box[JValue] = dataVend.getCommunity(bankId, entityName, id).map(it => parse(it.dataJson)) _ <- Helper.booleanToFuture(notFoundMsg(entityName, id, bankId), 404, cc = callContext) { existing.isDefined } _ <- Helper.booleanToFuture(s"$UserHasMissingRoles update access on this row", 403, cc = callContext) { - aclVend.allows(id, u.userId, DynamicDataAccessPermission.Update) } + aclVend.allows(bankId, entityName, id, u.userId, DynamicDataAccessPermission.Update) } // Field-level write roles still apply on top of the row ACL. updateJson = preserveRestrictedOnPut(json.asInstanceOf[JObject], existing, writeRestrictedFieldsOf(bankId, entityName)) box: Box[JValue] = dataVend.updateCommunity(bankId, entityName, updateJson, id).map(it => parse(it.dataJson)) @@ -458,7 +458,7 @@ object Http4sDynamicEntity extends MdcLoggable { bodyObj = json.asInstanceOf[JObject] // Row ACL replaces the entity-update role; per-field write roles still apply (requireEntityRole = false). _ <- Helper.booleanToFuture(s"$UserHasMissingRoles update access on this row", 403, cc = callContext) { - aclVend.allows(id, u.userId, DynamicDataAccessPermission.Update) } + aclVend.allows(bankId, entityName, id, u.userId, DynamicDataAccessPermission.Update) } missingRoles = missingPatchRoleNames(bodyObj.obj.map(_.name), bankId, entityName, u.userId, code.api.util.APIUtil.getConsumerPrimaryKey(callContext), requireEntityRole = false) _ <- Helper.booleanToFuture(s"$UserHasMissingRoles ${missingRoles.mkString(", ")}", 403, cc = callContext) { missingRoles.isEmpty } existing: Box[JValue] = dataVend.getCommunity(bankId, entityName, id).map(it => parse(it.dataJson)) @@ -480,9 +480,9 @@ object Http4sDynamicEntity extends MdcLoggable { existing: Box[JValue] = dataVend.getCommunity(bankId, entityName, id).map(it => parse(it.dataJson)) _ <- Helper.booleanToFuture(notFoundMsg(entityName, id, bankId), 404, cc = callContext) { existing.isDefined } _ <- Helper.booleanToFuture(s"$UserHasMissingRoles delete access on this row", 403, cc = callContext) { - aclVend.allows(id, u.userId, DynamicDataAccessPermission.Delete) } + aclVend.allows(bankId, entityName, id, u.userId, DynamicDataAccessPermission.Delete) } _ = dataVend.deleteCommunity(bankId, entityName, id) - _ = aclVend.deleteAllForRow(id) // cascade the ACL rows for the deleted data row + _ = aclVend.deleteAllForRow(bankId, entityName, id) // cascade the ACL rows for the deleted data row } yield JObject(Nil) } @@ -503,8 +503,8 @@ object Http4sDynamicEntity extends MdcLoggable { ("can_grant" -> a.canGrant) ~ ("granted_by" -> a.grantedBy) - private def rowAccessListJson(dataId: String): JObject = - ("access" -> JArray(aclVend.getAccessForRow(dataId).map(rowAccessRowJson))) + private def rowAccessListJson(bankId: Option[String], entityName: String, dataId: String): JObject = + ("access" -> JArray(aclVend.getAccessForRow(bankId, entityName, dataId).map(rowAccessRowJson))) // Shared preamble: before-intercept, auth, bank, flag-off (400), grant authorisation (403). private def rowAccessAuthorise(cc: CallContext, bankId: Option[String], entityName: String, id: String, @@ -517,7 +517,7 @@ object Http4sDynamicEntity extends MdcLoggable { (_, callContext2) <- bankCheck(bankId, callContext) _ <- Helper.booleanToFuture(RowLevelAccessNotEnabled, 400, cc = callContext2) { isRowLevel(bankId, entityName) } _ <- Helper.booleanToFuture(s"$UserHasMissingRoles grant access on this row", 403, cc = callContext2) { - aclVend.allows(id, u.userId, DynamicDataAccessPermission.Grant) || + aclVend.allows(bankId, entityName, id, u.userId, DynamicDataAccessPermission.Grant) || hasEntitlement(bankId.getOrElse(""), u.userId, DynamicEntityInfo.canGrantRowAccessRole(entityName, bankId)) } } yield (u, callContext2) @@ -527,7 +527,7 @@ object Http4sDynamicEntity extends MdcLoggable { EndpointHelpers.executeAndRespond(req) { cc => for { _ <- rowAccessAuthorise(cc, bankId, entityName, id, GET_ALL) - } yield rowAccessListJson(id) + } yield rowAccessListJson(bankId, entityName, id) } private def upsertRowAccess(req: Request[IO], bankId: Option[String], entityName: String, id: String): IO[Response[IO]] = @@ -544,21 +544,21 @@ object Http4sDynamicEntity extends MdcLoggable { entries.nonEmpty && entries.forall(o => strField(o, "user_id").isDefined) } _ = entries.foreach { o => - aclVend.grant(id, strField(o, "user_id").get, + aclVend.grant(bankId, entityName, id, strField(o, "user_id").get, canRead = boolField(o, "can_read", default = false), canUpdate = boolField(o, "can_update", default = false), canDelete = boolField(o, "can_delete", default = false), canGrant = boolField(o, "can_grant", default = true), // §8.1: re-share by default - entityName, bankId, grantedBy = granter.userId) + grantedBy = granter.userId) } - } yield rowAccessListJson(id) + } yield rowAccessListJson(bankId, entityName, id) } private def revokeRowAccess(req: Request[IO], bankId: Option[String], entityName: String, id: String, grantUserId: String): IO[Response[IO]] = EndpointHelpers.executeAndRespond(req) { cc => for { _ <- rowAccessAuthorise(cc, bankId, entityName, id, DELETE) - removed = aclVend.revoke(id, grantUserId).getOrElse(0) // cascades to downstream grants (§7) + removed = aclVend.revoke(bankId, entityName, id, grantUserId).getOrElse(0) // cascades to downstream grants (§7) } yield (("revoked_count" -> removed): JObject) } @@ -637,7 +637,7 @@ object Http4sDynamicEntity extends MdcLoggable { // edit, and share their own record with no role and no meta-admin hop (§4 / §8.1). _ = if (isRowLevel(bankId, entityName)) (singleObject \ DynamicEntityHelper.createEntityId(entityName)) match { case JString(rid) => - userIdOpt.foreach(uid => aclVend.grant(rid, uid, canRead = true, canUpdate = true, canDelete = true, canGrant = true, entityName, bankId, grantedBy = uid)) + userIdOpt.foreach(uid => aclVend.grant(bankId, entityName, rid, uid, canRead = true, canUpdate = true, canDelete = true, canGrant = true, grantedBy = uid)) case _ => } } yield wrapBankId(bankId, (singleName(entityName) -> singleObject)) diff --git a/obp-api/src/main/scala/code/api/dynamic/entity/projection/ProjectionStore.scala b/obp-api/src/main/scala/code/api/dynamic/entity/projection/ProjectionStore.scala index 6422a928f0..905770397f 100644 --- a/obp-api/src/main/scala/code/api/dynamic/entity/projection/ProjectionStore.scala +++ b/obp-api/src/main/scala/code/api/dynamic/entity/projection/ProjectionStore.scala @@ -28,6 +28,7 @@ TESOBE (http://www.tesobe.com/) package code.api.dynamic.entity.projection import code.DynamicData.DynamicData +import code.api.Constant.DYNAMIC_ENTITY_SYSTEM_LEVEL_BANK_ID import doobie._ import doobie.implicits._ @@ -84,17 +85,19 @@ object ProjectionStore { .query[(String, String)].to[List] /** - * Scope predicate mirroring `MappedDynamicDataProvider`'s get-all: entity name always; bankId via - * IS NOT DISTINCT FROM (handles system-level NULL); personal flag; userId only when personal. + * Scope predicate mirroring `MappedDynamicDataProvider`'s get-all: entity name always; bankId + * always (a system-level record stores a sentinel, never NULL); personal flag; userId only when + * personal, and that column IS still nullable so it keeps its null-safe comparison. * Returned without the `WHERE` keyword so callers can AND it with index predicates. */ def scope(bankId: Option[String], entityName: String, isPersonalEntity: Boolean, userId: Option[String], alias: String = ""): Fragment = { val p = if (alias.isEmpty) "" else alias + "." - // Bind as Option[String]: a system-level entity has bankId=None, which must bind SQL NULL — `orNull` - // as a plain String trips doobie's non-nullable Put[String] ("oops, null"). Put[Option[String]] - // emits NULL for None, and `IS NOT DISTINCT FROM NULL` is the intended null-safe match. + // The bank id column holds Constant.DYNAMIC_ENTITY_SYSTEM_LEVEL_BANK_ID for a record that belongs + // to no bank, never a SQL NULL, so this is a plain equality. It used to bind an Option and + // compare with IS NOT DISTINCT FROM; that stopped matching the moment the sentinel replaced the + // NULL, and this is the one place outside the Mapper queries that reads the column directly. val byEntity = Fragment.const(p + entityNameColumn) ++ fr"=" ++ fr0"$entityName" - val byBank = Fragment.const(p + bankIdColumn) ++ fr"IS NOT DISTINCT FROM" ++ fr0"${bankId: Option[String]}" + val byBank = Fragment.const(p + bankIdColumn) ++ fr"=" ++ fr0"${bankId.getOrElse(DYNAMIC_ENTITY_SYSTEM_LEVEL_BANK_ID)}" val byPersonal = Fragment.const(p + personalColumn) ++ fr"=" ++ fr0"$isPersonalEntity" val base = byEntity ++ fr"AND" ++ byBank ++ fr"AND" ++ byPersonal if (isPersonalEntity) base ++ fr"AND" ++ Fragment.const(p + userIdColumn) ++ fr"IS NOT DISTINCT FROM" ++ fr0"${userId: Option[String]}" diff --git a/obp-api/src/main/scala/code/api/util/NewStyle.scala b/obp-api/src/main/scala/code/api/util/NewStyle.scala index 390a57bb10..57b3e419cf 100644 --- a/obp-api/src/main/scala/code/api/util/NewStyle.scala +++ b/obp-api/src/main/scala/code/api/util/NewStyle.scala @@ -3620,7 +3620,7 @@ object NewStyle extends MdcLoggable{ DynamicEntityInfo.roleNames(entity.entityName, entity.bankId).foreach(ApiRole.removeDynamicApiRole(_)) // Cascade row-level ACL rows for this entity (§7) — safety net for any rows not already // cleaned up by per-row delete; no-op for non-row-level entities. - code.DynamicData.DynamicDataAccessProvider.provider.vend.deleteAllForEntity(entity.entityName, entity.bankId) + code.DynamicData.DynamicDataAccessProvider.provider.vend.deleteAllForEntity(entity.bankId, entity.entityName) } deleteEntitleMentResult } 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 abaa974c2a..2f272ccbc4 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 @@ -29,6 +29,7 @@ package code.api.util.migration import code.api.util.APIUtil.{getPropsAsBoolValue, getPropsValue} import code.api.util.{APIUtil, ApiPropsWithAlias} +import code.api.Constant.DYNAMIC_ENTITY_SYSTEM_LEVEL_BANK_ID import code.api.v4_0_0.DatabaseInfoJson import code.consumer.Consumers import code.context.MappedUserAuthContextUpdate @@ -192,6 +193,135 @@ object Migration extends MdcLoggable { alterDynamicDataIdLength() } + /** + * What one step of prepareDynamicEntitySpaceScopedIndexes actually did. + * + * The description is written for whoever later reads the migration log asking what an upgrade + * did to their database, so it says what was found as well as what was changed -- "nothing to + * move" is as much of an answer as "moved 4 rows", and a step that claims work it did not do is + * worse than no log at all. + */ + private case class SchemaPreparationOutcome(description: String, changedSomething: Boolean, failed: Boolean) + + /** + * Prepare the two Dynamic Entity tables for their new, space-scoped unique indexes. + * + * A Dynamic Entity record's id used to be unique across the whole instance, because the unique + * index named that column alone. That was wrong: an id is only meaningful within one space and + * one entity, and two spaces may each hold a record whose natural key is the country code DE. + * The index now names the bank id and the entity name as well, which Schemifier creates from + * DynamicData.dbIndexes and DynamicDataAccess.dbIndexes. + * + * Two things have to happen for that to be correct on an existing database. The bank id column + * has to stop holding SQL NULL for a system level record, because Postgres treats NULLs as + * distinct inside a unique index and the new index would therefore enforce nothing at all for + * exactly those rows; the sentinel written instead is + * Constant.DYNAMIC_ENTITY_SYSTEM_LEVEL_BANK_ID. And the superseded single-column unique indexes + * have to be dropped, because Lift's Schemifier only ever creates indexes and never removes + * one, so an old index would survive and keep refusing the duplicate ids the new index exists + * to allow. + * + * Invoked directly from Boot BEFORE schemifyAll() and deliberately not routed through + * executeScripts/runOnce, for the same reason set out on deduplicateBeforeUniqueIndexSchemify: + * those passes are gated by the migration_scripts.* props, which are off in tests, whereas + * Schemifier creates the new index ungated in every environment. A gated back fill would leave + * a test database still carrying the old index, still refusing the ids this change allows. + * + * Because it is not a runOnce, it runs on every boot and is written to be a cheap no-op once + * there is nothing left to do. It still writes a MigrationScriptLog entry on the boots where it + * did something or failed, so the work shows up where an operator looks for it; the entry + * reports every step, including the ones that found nothing to do. + * + * The back fill cannot produce a duplicate. The old unique index made every id unique across + * the instance, so no two system level rows can already share one. + */ + def prepareDynamicEntitySpaceScopedIndexes(): Unit = { + val name = "prepareDynamicEntitySpaceScopedIndexes" + val startDate = System.currentTimeMillis() + val outcomes = List( + adoptSystemLevelBankIdSentinel("dynamicdata"), + adoptSystemLevelBankIdSentinel("dynamicdataaccess"), + dropSupersededIndex("dynamicdata", "dynamicdata_dynamicdataid"), + dropSupersededIndex("dynamicdataaccess", "dynamicdataaccess_dynamicdataid_userid") + ) + val endDate = System.currentTimeMillis() + val didSomething = outcomes.exists(_.changedSomething) + val anythingFailed = outcomes.exists(_.failed) + val alreadyRecorded = MigrationScriptLogProvider.migrationScriptLogProvider.vend.isExecuted(name) + // An entry is written on the boot that does the work, on any boot that fails, and on the first + // boot that finds the schema already correct. That last case matters: an instance whose data + // was converted by a build predating this logging, or a database created fresh with the new + // index already in DynamicData.dbIndexes, would otherwise have nothing here at all, and the + // operator could not tell "this ran and there was nothing to do" from "this never ran". + // Every instance therefore ends up with exactly one entry, and it says which of the two it was. + if (didSomething || anythingFailed || !alreadyRecorded) { + val summary = + if (anythingFailed) "Completed with failures" + else if (didSomething) "Applied" + else "No change needed, the schema was already space-scoped" + saveLog(name, APIUtil.gitCommit, isSuccessful = !anythingFailed, startDate, endDate, + s"$summary: ${outcomes.map(_.description).mkString("; ")}") + } + } + + /** Replace the SQL NULLs in `tableName`'s bankid column with the system level sentinel. */ + private def adoptSystemLevelBankIdSentinel(tableName: String): SchemaPreparationOutcome = { + if (!DbFunction.tableExistsByName(tableName)) { + SchemaPreparationOutcome(s"$tableName: table not present, so no bank ids to move", changedSomething = false, failed = false) + } else { + val sql = s"UPDATE $tableName SET bankid = '$DYNAMIC_ENTITY_SYSTEM_LEVEL_BANK_ID' WHERE bankid IS NULL" + try { + val moved = DB.use(net.liftweb.util.DefaultConnectionIdentifier) { conn => + val st = conn.createStatement() + try st.executeUpdate(sql) finally st.close() + } + if (moved > 0) { + logger.warn(s"prepareDynamicEntitySpaceScopedIndexes: moved $moved system level row(s) in " + + s"$tableName from a NULL bank id to '$DYNAMIC_ENTITY_SYSTEM_LEVEL_BANK_ID'") + SchemaPreparationOutcome(s"$sql -- moved $moved row(s)", changedSomething = true, failed = false) + } else { + SchemaPreparationOutcome(s"$tableName: no row had a NULL bank id, so none was moved", changedSomething = false, failed = false) + } + } catch { + case e: SQLException => + logger.error(s"prepareDynamicEntitySpaceScopedIndexes: $sql failed", e) + SchemaPreparationOutcome(s"$sql -- FAILED: ${e.getMessage}", changedSomething = false, failed = true) + } + } + } + + /** + * Drop an index that a wider one has replaced, if it is still there. + * + * Whether the index was present is established first rather than relying on IF EXISTS, so that + * the log can distinguish an index this actually removed from one that was already gone. SQL + * Server needs the table named in the statement; every other driver OBP ships takes the plain + * form. + */ + private def dropSupersededIndex(tableName: String, indexName: String): SchemaPreparationOutcome = { + if (!DbFunction.tableExistsByName(tableName)) { + SchemaPreparationOutcome(s"$tableName: table not present, so index $indexName cannot be either", changedSomething = false, failed = false) + } else if (!DbFunction.indexExistsByName(tableName, indexName)) { + SchemaPreparationOutcome(s"$indexName: already absent from $tableName, so nothing was dropped", changedSomething = false, failed = false) + } else { + val isSqlServer = getPropsValue("db.driver") + .exists(_.contains("com.microsoft.sqlserver.jdbc.SQLServerDriver")) + val sql = if (isSqlServer) s"DROP INDEX $indexName ON $tableName" else s"DROP INDEX $indexName" + try { + DB.use(net.liftweb.util.DefaultConnectionIdentifier) { conn => + val st = conn.createStatement() + try st.execute(sql) finally st.close() + } + logger.warn(s"prepareDynamicEntitySpaceScopedIndexes: dropped the superseded index $indexName on $tableName") + SchemaPreparationOutcome(s"$sql -- dropped from $tableName", changedSomething = true, failed = false) + } catch { + case e: SQLException => + logger.error(s"prepareDynamicEntitySpaceScopedIndexes: $sql failed", e) + SchemaPreparationOutcome(s"$sql -- FAILED: ${e.getMessage}", changedSomething = false, failed = true) + } + } + } + /** * Remove natural-key duplicate rows so Schemifier's CREATE UNIQUE INDEX on * `mapperaccountholder` (user_, bank, account) and `mappedentitlement` (bank, user, role) @@ -969,6 +1099,31 @@ object Migration extends MdcLoggable { } } + /** + * Is an index of this name present on this table, according to JDBC metadata? + * + * This exists so that a step which removes a superseded index can report whether it actually + * removed one or found it already gone. `DROP INDEX IF EXISTS` cannot tell the two apart, and a + * migration log that says an index was dropped when it was never there is worse than no log. + * Index names are compared without regard to case, because each database stores them in its own. + */ + def indexExistsByName(tableName: String, indexName: String): Boolean = { + DB.use(net.liftweb.util.DefaultConnectionIdentifier) { conn => + val md = conn.getMetaData + val schema = getDefaultSchemaName(conn) + using(md.getIndexInfo(null, schema, tableName, false, true)) { rs => + def check(): Boolean = + if (!rs.next) false + else Option(rs.getString(6)) match { + // A null index name is a table statistics row, not an index; skip it. + case Some(found) if found.equalsIgnoreCase(indexName) => true + case _ => check() + } + check() + } + } + } + /** * Declared max length of a (var)char column, via JDBC metadata (portable across H2/Postgres/MSSQL). * `None` if the column is absent or has no size (e.g. not a character type). Used by `alterColumn*` diff --git a/obp-api/src/main/scala/code/dynamicEntity/DynamicDataAccessProvider.scala b/obp-api/src/main/scala/code/dynamicEntity/DynamicDataAccessProvider.scala index 1eb4d69e2a..38191a638d 100644 --- a/obp-api/src/main/scala/code/dynamicEntity/DynamicDataAccessProvider.scala +++ b/obp-api/src/main/scala/code/dynamicEntity/DynamicDataAccessProvider.scala @@ -66,32 +66,43 @@ trait DynamicDataAccessT { trait DynamicDataAccessProvider { /** - * Upsert one ACL row: grant (or update) `userId`'s permissions on `dynamicDataId`. + * Every method here is scoped by the space and the entity as well as by the record id. + * + * A record id is only unique within one space and one entity -- two spaces may each hold a record + * whose natural key is the country code DE -- so a method that took the record id alone could not + * tell those two records apart, and an access grant made in one space would be read as a grant in + * the other. The parameters are ordered the way the things they name come into existence: the + * bank (the space) first, then the entity defined within it, then the record. + */ + + /** + * Upsert one ACL row: grant (or update) `userId`'s permissions on the named record. * `grantedBy` records the userId who created the grant, for the revoke cascade. */ - def grant(dynamicDataId: String, userId: String, + def grant(bankId: Option[String], entityName: String, dynamicDataId: String, userId: String, canRead: Boolean, canUpdate: Boolean, canDelete: Boolean, canGrant: Boolean, - entityName: String, bankId: Option[String], grantedBy: String): Box[DynamicDataAccessT] + grantedBy: String): Box[DynamicDataAccessT] /** - * Revoke `userId`'s access to `dynamicDataId` AND cascade: every grant transitively - * made by `userId` on the same row is removed too (walk `grantedBy` with a visited-set + * Revoke `userId`'s access to the named record AND cascade: every grant transitively + * made by `userId` on the same record is removed too (walk `grantedBy` with a visited-set * so re-share cycles terminate). Returns the number of ACL rows removed. */ - def revoke(dynamicDataId: String, userId: String): Box[Int] + def revoke(bankId: Option[String], entityName: String, dynamicDataId: String, userId: String): Box[Int] - /** All ACL rows for a single data row (for the GET .../access listing). */ - def getAccessForRow(dynamicDataId: String): List[DynamicDataAccessT] + /** All ACL rows for a single record (for the GET .../access listing). */ + def getAccessForRow(bankId: Option[String], entityName: String, dynamicDataId: String): List[DynamicDataAccessT] - /** DynamicDataIds of `entityName`/`bankId` that `userId` may read — the get-all filter. */ - def getReadableDynamicDataIds(userId: String, entityName: String, bankId: Option[String]): List[String] + /** DynamicDataIds of the entity in this space that `userId` may read -- the get-all filter. */ + def getReadableDynamicDataIds(bankId: Option[String], entityName: String, userId: String): List[String] - /** Does `userId` hold `permission` on `dynamicDataId`? */ - def allows(dynamicDataId: String, userId: String, permission: DynamicDataAccessPermission): Boolean + /** Does `userId` hold `permission` on the named record? */ + def allows(bankId: Option[String], entityName: String, dynamicDataId: String, userId: String, + permission: DynamicDataAccessPermission): Boolean - /** Cascade on row delete: remove every ACL row for the data row. */ - def deleteAllForRow(dynamicDataId: String): Box[Boolean] + /** Cascade on record delete: remove every ACL row for that record. */ + def deleteAllForRow(bankId: Option[String], entityName: String, dynamicDataId: String): Box[Boolean] - /** Cascade on entity delete: remove every ACL row for the entity/bank. */ - def deleteAllForEntity(entityName: String, bankId: Option[String]): Box[Boolean] + /** Cascade on entity delete: remove every ACL row for the entity in this space. */ + def deleteAllForEntity(bankId: Option[String], entityName: String): Box[Boolean] } diff --git a/obp-api/src/main/scala/code/dynamicEntity/MappedDynamicDataAccessProvider.scala b/obp-api/src/main/scala/code/dynamicEntity/MappedDynamicDataAccessProvider.scala index c5a33d3897..be4696612e 100644 --- a/obp-api/src/main/scala/code/dynamicEntity/MappedDynamicDataAccessProvider.scala +++ b/obp-api/src/main/scala/code/dynamicEntity/MappedDynamicDataAccessProvider.scala @@ -27,6 +27,7 @@ TESOBE (http://www.tesobe.com/) package code.DynamicData +import code.api.Constant.DYNAMIC_ENTITY_SYSTEM_LEVEL_BANK_ID import net.liftweb.common.Box import net.liftweb.mapper._ import net.liftweb.util.Helpers.tryo @@ -35,27 +36,58 @@ import scala.collection.mutable object MappedDynamicDataAccessProvider extends DynamicDataAccessProvider { - override def grant(dynamicDataId: String, userId: String, + /** + * This turns the optional bank id the caller supplies into the value actually stored in the + * BankId column. An ACL row for a record that belongs to no bank is stored under + * Constant.DYNAMIC_ENTITY_SYSTEM_LEVEL_BANK_ID rather than as a SQL NULL, for the same reason the + * record itself is: the unique index over these rows has to include the bank id, and Postgres + * treats NULLs as distinct, so a nullable column would make the index enforce nothing for the + * system level rows. + */ + private def storedBankId(bankId: Option[String]): String = + bankId.getOrElse(DYNAMIC_ENTITY_SYSTEM_LEVEL_BANK_ID) + + /** + * The parameters that pin an ACL row to one record: the space, the entity and the record id. + * + * A record id alone does not identify a record, because ids are only unique within one space and + * one entity. Every query below starts from this list so that a grant made on the country code DE + * in one space can never be read, revoked or cascaded as a grant on DE in another. + */ + private def scopeOf(bankId: Option[String], entityName: String, dynamicDataId: String): List[QueryParam[DynamicDataAccess]] = + List( + By(DynamicDataAccess.BankId, storedBankId(bankId)), + By(DynamicDataAccess.EntityName, entityName), + By(DynamicDataAccess.DynamicDataId, dynamicDataId) + ) + + override def grant(bankId: Option[String], entityName: String, dynamicDataId: String, userId: String, canRead: Boolean, canUpdate: Boolean, canDelete: Boolean, canGrant: Boolean, - entityName: String, bankId: Option[String], grantedBy: String): Box[DynamicDataAccessT] = tryo { + grantedBy: String): Box[DynamicDataAccessT] = tryo { val row = DynamicDataAccess.find( - By(DynamicDataAccess.DynamicDataId, dynamicDataId), - By(DynamicDataAccess.UserId, userId) - ).getOrElse(DynamicDataAccess.create.DynamicDataId(dynamicDataId).UserId(userId)) + (By(DynamicDataAccess.UserId, userId) :: scopeOf(bankId, entityName, dynamicDataId)): _* + ).getOrElse( + DynamicDataAccess.create + .DynamicDataId(dynamicDataId) + .UserId(userId) + .EntityName(entityName) + .BankId(storedBankId(bankId)) + ) row.CanRead(canRead) .CanUpdate(canUpdate) .CanDelete(canDelete) .CanGrant(canGrant) .EntityName(entityName) - .BankId(bankId.getOrElse(null)) + .BankId(storedBankId(bankId)) .GrantedBy(grantedBy) .saveMe() } - override def revoke(dynamicDataId: String, userId: String): Box[Int] = tryo { + override def revoke(bankId: Option[String], entityName: String, dynamicDataId: String, userId: String): Box[Int] = tryo { // Walk the GrantedBy edges within this single data row: remove the target user and // everyone they granted downstream. The visited-set makes re-share cycles terminate // and absorbs the owner row's self-edge (GrantedBy == UserId). + val scope = scopeOf(bankId, entityName, dynamicDataId) val toRemove = mutable.LinkedHashSet[String](userId) val visited = mutable.Set[String]() var frontier = List(userId) @@ -65,8 +97,7 @@ object MappedDynamicDataAccessProvider extends DynamicDataAccessProvider { if (!visited.contains(current)) { visited += current val children = DynamicDataAccess.findAll( - By(DynamicDataAccess.DynamicDataId, dynamicDataId), - By(DynamicDataAccess.GrantedBy, current) + (By(DynamicDataAccess.GrantedBy, current) :: scope): _* ).map(_.UserId.get).filterNot(visited.contains) children.foreach { child => toRemove += child @@ -75,34 +106,26 @@ object MappedDynamicDataAccessProvider extends DynamicDataAccessProvider { } } toRemove.toList.flatMap { uid => - DynamicDataAccess.findAll( - By(DynamicDataAccess.DynamicDataId, dynamicDataId), - By(DynamicDataAccess.UserId, uid) - ) + DynamicDataAccess.findAll((By(DynamicDataAccess.UserId, uid) :: scope): _*) }.map(_.delete_!).count(identity) } - override def getAccessForRow(dynamicDataId: String): List[DynamicDataAccessT] = - DynamicDataAccess.findAll(By(DynamicDataAccess.DynamicDataId, dynamicDataId)) + override def getAccessForRow(bankId: Option[String], entityName: String, dynamicDataId: String): List[DynamicDataAccessT] = + DynamicDataAccess.findAll(scopeOf(bankId, entityName, dynamicDataId): _*) - override def getReadableDynamicDataIds(userId: String, entityName: String, bankId: Option[String]): List[String] = { - val base: List[QueryParam[DynamicDataAccess]] = List( + override def getReadableDynamicDataIds(bankId: Option[String], entityName: String, userId: String): List[String] = + DynamicDataAccess.findAll( By(DynamicDataAccess.UserId, userId), By(DynamicDataAccess.EntityName, entityName), - By(DynamicDataAccess.CanRead, true) - ) - val scoped = bankId match { - case Some(b) => By(DynamicDataAccess.BankId, b) :: base - case None => NullRef(DynamicDataAccess.BankId) :: base - } - DynamicDataAccess.findAll(scoped: _*).map(_.DynamicDataId.get) - } + By(DynamicDataAccess.CanRead, true), + By(DynamicDataAccess.BankId, storedBankId(bankId)) + ).map(_.DynamicDataId.get) - override def allows(dynamicDataId: String, userId: String, permission: DynamicDataAccessPermission): Boolean = { + override def allows(bankId: Option[String], entityName: String, dynamicDataId: String, userId: String, + permission: DynamicDataAccessPermission): Boolean = { import DynamicDataAccessPermission._ DynamicDataAccess.find( - By(DynamicDataAccess.DynamicDataId, dynamicDataId), - By(DynamicDataAccess.UserId, userId) + (By(DynamicDataAccess.UserId, userId) :: scopeOf(bankId, entityName, dynamicDataId)): _* ).map { row => permission match { case Read => row.CanRead.get @@ -113,16 +136,15 @@ object MappedDynamicDataAccessProvider extends DynamicDataAccessProvider { }.getOrElse(false) } - override def deleteAllForRow(dynamicDataId: String): Box[Boolean] = tryo { - DynamicDataAccess.findAll(By(DynamicDataAccess.DynamicDataId, dynamicDataId)).forall(_.delete_!) + override def deleteAllForRow(bankId: Option[String], entityName: String, dynamicDataId: String): Box[Boolean] = tryo { + DynamicDataAccess.findAll(scopeOf(bankId, entityName, dynamicDataId): _*).forall(_.delete_!) } - override def deleteAllForEntity(entityName: String, bankId: Option[String]): Box[Boolean] = tryo { - val params: List[QueryParam[DynamicDataAccess]] = bankId match { - case Some(b) => List(By(DynamicDataAccess.EntityName, entityName), By(DynamicDataAccess.BankId, b)) - case None => List(By(DynamicDataAccess.EntityName, entityName), NullRef(DynamicDataAccess.BankId)) - } - DynamicDataAccess.findAll(params: _*).forall(_.delete_!) + override def deleteAllForEntity(bankId: Option[String], entityName: String): Box[Boolean] = tryo { + DynamicDataAccess.findAll( + By(DynamicDataAccess.EntityName, entityName), + By(DynamicDataAccess.BankId, storedBankId(bankId)) + ).forall(_.delete_!) } } @@ -148,12 +170,17 @@ class DynamicDataAccess extends DynamicDataAccessT with LongKeyedMapper[DynamicD override def canGrant: Boolean = CanGrant.get override def grantedBy: String = GrantedBy.get override def entityName: String = EntityName.get - override def bankId: Option[String] = Option(BankId.get) + // A system level ACL row stores the sentinel rather than a SQL NULL; it is filtered back out + // here so every reader still sees None, exactly as before. + override def bankId: Option[String] = Option(BankId.get).filterNot(_ == DYNAMIC_ENTITY_SYSTEM_LEVEL_BANK_ID) } object DynamicDataAccess extends DynamicDataAccess with LongKeyedMetaMapper[DynamicDataAccess] { override def dbIndexes = - UniqueIndex(DynamicDataId, UserId) :: + // One ACL row per (space, entity, record, user). DynamicDataId alone does not identify a + // record -- ids repeat across spaces -- so this index carries the same discriminators, in the + // same existence order, as DynamicData's own unique index. + UniqueIndex(BankId, EntityName, DynamicDataId, UserId) :: Index(UserId, EntityName, BankId) :: Index(DynamicDataId, GrantedBy) :: super.dbIndexes diff --git a/obp-api/src/main/scala/code/dynamicEntity/MapppedDynamicDataProvider.scala b/obp-api/src/main/scala/code/dynamicEntity/MapppedDynamicDataProvider.scala index 6bc839b26b..5558d70ddd 100644 --- a/obp-api/src/main/scala/code/dynamicEntity/MapppedDynamicDataProvider.scala +++ b/obp-api/src/main/scala/code/dynamicEntity/MapppedDynamicDataProvider.scala @@ -31,6 +31,7 @@ import org.json4s._ import code.api.util.CustomJsonFormats import code.api.util.ErrorMessages.DynamicDataNotFound import code.api.util.APIUtil.generateUUID +import code.api.Constant.DYNAMIC_ENTITY_SYSTEM_LEVEL_BANK_ID import net.liftweb.common.{Box, Failure, Full} import com.openbankproject.commons.util.json import org.json4s.JObject @@ -92,7 +93,7 @@ object MappedDynamicDataProvider extends DynamicDataProvider with CustomJsonForm By(DynamicData.DynamicDataId, id), By(DynamicData.DynamicEntityName, entityName), By(DynamicData.IsPersonalEntity, false), - NullRef(DynamicData.BankId) + By(DynamicData.BankId, DYNAMIC_ENTITY_SYSTEM_LEVEL_BANK_ID) ) match { case Full(dynamicData) => Full(dynamicData) case _ => Failure(s"$DynamicDataNotFound dynamicEntityName=$entityName, dynamicDataId=$id") @@ -102,7 +103,7 @@ object MappedDynamicDataProvider extends DynamicDataProvider with CustomJsonForm By(DynamicData.DynamicDataId, id), By(DynamicData.DynamicEntityName, entityName), By(DynamicData.UserId, userId.getOrElse(null)), - NullRef(DynamicData.BankId) + By(DynamicData.BankId, DYNAMIC_ENTITY_SYSTEM_LEVEL_BANK_ID) ) match { case Full(dynamicData) => Full(dynamicData) case _ => Failure(s"$DynamicDataNotFound dynamicEntityName=$entityName, dynamicDataId=$id, userId = $userId") @@ -144,13 +145,13 @@ object MappedDynamicDataProvider extends DynamicDataProvider with CustomJsonForm DynamicData.findAll( By(DynamicData.DynamicEntityName, entityName), By(DynamicData.IsPersonalEntity, false), - NullRef(DynamicData.BankId), + By(DynamicData.BankId, DYNAMIC_ENTITY_SYSTEM_LEVEL_BANK_ID), ) } else if(bankId.isEmpty && isPersonalEntity){ //isPersonalEntity == true, get all the data for specific userId (regardless of how it was created). DynamicData.findAll( By(DynamicData.DynamicEntityName, entityName), By(DynamicData.UserId, userId.getOrElse(null)), - NullRef(DynamicData.BankId) + By(DynamicData.BankId, DYNAMIC_ENTITY_SYSTEM_LEVEL_BANK_ID) ) } else if(bankId.isDefined && !isPersonalEntity){ //isPersonalEntity == false, get all the data, no need for specific userId. DynamicData.findAll( @@ -181,7 +182,7 @@ object MappedDynamicDataProvider extends DynamicDataProvider with CustomJsonForm if (bankId.isEmpty) { DynamicData.findAll( By(DynamicData.DynamicEntityName, entityName), - NullRef(DynamicData.BankId), + By(DynamicData.BankId, DYNAMIC_ENTITY_SYSTEM_LEVEL_BANK_ID), ) } else { DynamicData.findAll( @@ -202,7 +203,7 @@ object MappedDynamicDataProvider extends DynamicDataProvider with CustomJsonForm DynamicData.find( By(DynamicData.DynamicDataId, id), By(DynamicData.DynamicEntityName, entityName), - NullRef(DynamicData.BankId) + By(DynamicData.BankId, DYNAMIC_ENTITY_SYSTEM_LEVEL_BANK_ID) ) match { case Full(dynamicData) => Full(dynamicData) case _ => Failure(s"$DynamicDataNotFound dynamicEntityName=$entityName, dynamicDataId=$id") @@ -241,7 +242,7 @@ object MappedDynamicDataProvider extends DynamicDataProvider with CustomJsonForm if(bankId.isEmpty && !isPersonalEntity){//isPersonalEntity == false, get all the data, no need for specific userId. DynamicData.find( By(DynamicData.DynamicEntityName, dynamicEntityName), - NullRef(DynamicData.BankId), + By(DynamicData.BankId, DYNAMIC_ENTITY_SYSTEM_LEVEL_BANK_ID), By(DynamicData.IsPersonalEntity, false) ).isDefined } else if(bankId.isDefined && !isPersonalEntity){//isPersonalEntity == false, get all the data, no need for specific userId. @@ -253,7 +254,7 @@ object MappedDynamicDataProvider extends DynamicDataProvider with CustomJsonForm } else if(bankId.isEmpty && isPersonalEntity){ //isPersonalEntity == true, check if data exists for specific userId (regardless of how it was created). DynamicData.find( By(DynamicData.DynamicEntityName, dynamicEntityName), - NullRef(DynamicData.BankId), + By(DynamicData.BankId, DYNAMIC_ENTITY_SYSTEM_LEVEL_BANK_ID), By(DynamicData.UserId, userId.getOrElse(null)) ).nonEmpty } else { //isPersonalEntity == true, check if data exists for specific userId (regardless of how it was created). @@ -271,7 +272,7 @@ object MappedDynamicDataProvider extends DynamicDataProvider with CustomJsonForm val dataStr = json.compactRender(requestBody) val saved = data.DataJson(dataStr) .DynamicEntityName(entityName) - .BankId(bankId.getOrElse(null)) + .BankId(bankId.getOrElse(DYNAMIC_ENTITY_SYSTEM_LEVEL_BANK_ID)) .UserId(userId.getOrElse(null)) .IsPersonalEntity(isPersonalEntity) .saveMe() @@ -317,12 +318,27 @@ class DynamicData extends DynamicDataT with LongKeyedMapper[DynamicData] with Id override def dynamicDataId: Option[String] = Option(DynamicDataId.get) override def dynamicEntityName: String = DynamicEntityName.get override def dataJson: String = DataJson.get - override def bankId: Option[String] = Option(BankId.get) + // A system level record stores the sentinel rather than a SQL NULL, so it is filtered back out + // here: every caller of this method still sees None for such a record, exactly as before. + override def bankId: Option[String] = Option(BankId.get).filterNot(_ == DYNAMIC_ENTITY_SYSTEM_LEVEL_BANK_ID) override def userId: Option[String] = Option(UserId.get) override def isPersonalEntity: Boolean = IsPersonalEntity.get } object DynamicData extends DynamicData with LongKeyedMetaMapper[DynamicData] { - override def dbIndexes = UniqueIndex(DynamicDataId) :: super.dbIndexes + /** + * A record's id is unique within one space and one entity, not across the whole instance. + * + * The columns are named in the order the things they identify come into existence: the bank (the + * space) exists first, the entity is defined within it, and the record is created last. The index + * used to name DynamicDataId alone, which meant two spaces could not each hold a record with the + * same natural key -- one country table holding DE stopped any other from holding it. The + * discriminators were already sitting in the same row, simply unused by the index. + * + * The bank id column never holds a SQL NULL; see Constant.DYNAMIC_ENTITY_SYSTEM_LEVEL_BANK_ID for + * why that matters here, since Postgres treats NULLs as distinct and a nullable column in this + * index would enforce nothing for system level records. + */ + override def dbIndexes = UniqueIndex(BankId, DynamicEntityName, DynamicDataId) :: super.dbIndexes } diff --git a/obp-api/src/test/scala/code/api/v6_0_0/DynamicEntityJoinQueryIntegrationTest.scala b/obp-api/src/test/scala/code/api/v6_0_0/DynamicEntityJoinQueryIntegrationTest.scala index 2d70ef7e91..22a0d9ebf4 100644 --- a/obp-api/src/test/scala/code/api/v6_0_0/DynamicEntityJoinQueryIntegrationTest.scala +++ b/obp-api/src/test/scala/code/api/v6_0_0/DynamicEntityJoinQueryIntegrationTest.scala @@ -117,8 +117,9 @@ class DynamicEntityJoinQueryIntegrationTest extends V600ServerSetup { val d1 = saveRec(Deal, "partner_ref" -> JString(p1)) saveRec(Deal, "partner_ref" -> JString(p2)) // d2: granted to nobody - DynamicDataAccessProvider.provider.vend.grant(d1, userA, canRead = true, canUpdate = false, - canDelete = false, canGrant = false, entityName = Deal, bankId = None, grantedBy = owner) + DynamicDataAccessProvider.provider.vend.grant(bankId = None, entityName = Deal, dynamicDataId = d1, + userId = userA, canRead = true, canUpdate = false, canDelete = false, canGrant = false, + grantedBy = owner) // --- provision AFTER writing, so the backfill populates projections from the blobs. // (This makes the test independent of dynamic_entity.indexing.backend; provisioning's backfill + diff --git a/obp-api/src/test/scala/code/api/v6_0_0/DynamicEntitySystemLevelBankIdTest.scala b/obp-api/src/test/scala/code/api/v6_0_0/DynamicEntitySystemLevelBankIdTest.scala new file mode 100644 index 0000000000..7b89d31133 --- /dev/null +++ b/obp-api/src/test/scala/code/api/v6_0_0/DynamicEntitySystemLevelBankIdTest.scala @@ -0,0 +1,163 @@ +/** +Open Bank Project - API +Copyright (C) 2011-2026, 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.api.v6_0_0 + +import java.io.File + +import code.DynamicData.DynamicDataProvider +import code.api.Constant.DYNAMIC_ENTITY_SYSTEM_LEVEL_BANK_ID +import code.api.util.APIUtil +import code.setup.ServerSetup +import net.liftweb.common.Full +import org.json4s.JsonAST.JObject +import org.json4s.JsonDSL._ +import org.scalatest.Tag + +import scala.io.Source + +/** + * This suite covers the rule that a Dynamic Entity record's identifier is unique within one space + * and one entity, rather than across the whole instance. + * + * A space is a bank, and a record may be given a natural key instead of a generated identifier: a + * country code, or the name of a scheme. Two spaces may therefore each legitimately hold a record + * called DE, and until the unique index carried the bank id and the entity name as well, the second + * one was refused over an identifier that was never meant to be unique instance-wide. + * + * Allowing that needs the bank id column to hold a real value for a record belonging to no bank, + * because Postgres treats SQL NULLs as distinct inside a unique index, so an index over a nullable + * bank id would enforce nothing at all for precisely those rows. That value is + * Constant.DYNAMIC_ENTITY_SYSTEM_LEVEL_BANK_ID, and the second feature below is what stops a real + * bank ever being created under the same identifier and quietly merging with the system level data. + */ +class DynamicEntitySystemLevelBankIdTest extends ServerSetup { + + object DynamicEntitySpaceScope extends Tag("DynamicEntitySpaceScope") + + private def dataProvider = DynamicDataProvider.connectorMethodProvider.vend + + /** + * A record body carrying its own id field, which is what gives the record its natural key. + * The field name has to match what MappedDynamicDataProvider.getIdName derives from the entity + * name, otherwise the supplied key is ignored and a UUID is generated instead. + */ + private def idFieldNameOf(entityName: String): String = + s"${entityName}_Id".replaceAll("(?<=[a-z0-9])(?=[A-Z])|-", "_").toLowerCase + + private def bodyWithId(entityName: String, id: String): JObject = + (idFieldNameOf(entityName) -> id) ~ ("name" -> s"record $id") + + feature("A record's identifier is unique within its space, not across the whole instance") { + + scenario("two spaces can each hold a record with the same natural key", DynamicEntitySpaceScope) { + val entityName = "CountryScopeTest" + val first = dataProvider.save(Some("bank_one"), entityName, bodyWithId(entityName, "DE"), None, false) + val second = dataProvider.save(Some("bank_two"), entityName, bodyWithId(entityName, "DE"), None, false) + first shouldBe a[Full[_]] + second shouldBe a[Full[_]] + first.map(_.bankId) shouldBe Full(Some("bank_one")) + second.map(_.bankId) shouldBe Full(Some("bank_two")) + } + + scenario("a system level record and a record in a space can share one", DynamicEntitySpaceScope) { + val entityName = "CountrySystemScopeTest" + dataProvider.save(None, entityName, bodyWithId(entityName, "DE"), None, false) shouldBe a[Full[_]] + dataProvider.save(Some("bank_three"), entityName, bodyWithId(entityName, "DE"), None, false) shouldBe a[Full[_]] + } + + // The sentinel is a storage detail. Everything above the provider still describes a system + // level record as belonging to no bank at all, which is what keeps the rest of the codebase + // behaving as before -- including the projection table naming, which hashes this very Option. + scenario("a system level record still reads back as belonging to no bank", DynamicEntitySpaceScope) { + val entityName = "CountryReadBackTest" + dataProvider.save(None, entityName, bodyWithId(entityName, "FR"), None, false) + .map(_.bankId) shouldBe Full(None) + dataProvider.get(None, entityName, "FR", None, false).map(_.bankId) shouldBe Full(None) + } + + scenario("one space still cannot hold the same key twice", DynamicEntitySpaceScope) { + val entityName = "CountryDuplicateTest" + dataProvider.save(Some("bank_four"), entityName, bodyWithId(entityName, "ES"), None, false) shouldBe a[Full[_]] + dataProvider.save(Some("bank_four"), entityName, bodyWithId(entityName, "ES"), None, false) should not be a[Full[_]] + } + } + + feature("The system level bank id can never be created as a real bank id") { + + /** + * Every endpoint accepting a caller supplied bank id writes its own minimum length check rather + * than calling a shared one, so this walks the sources and holds all of them to the same floor + * at once. That duplication is exactly why the rule is worth pinning: a further endpoint that + * forgets the line, or a relaxation of one that has it, would make the sentinel creatable and + * let a real bank quietly merge with the system level records. + */ + scenario("every minimum length rule on BANK_ID requires more characters than it has", DynamicEntitySpaceScope) { + val root = List(new File("src/main/scala/code/api"), new File("obp-api/src/main/scala/code/api")) + .find(_.isDirectory) + .getOrElse(fail("cannot locate the api sources - this guard must not pass by failing to look")) + + def scalaFilesUnder(dir: File): List[File] = + Option(dir.listFiles).toList.flatten.flatMap { f => + if (f.isDirectory) scalaFilesUnder(f) + else if (f.getName.endsWith(".scala")) List(f) else Nil + } + + // Matches the comparison itself, e.g. `bank.id.length > 3` or `postJson.bank_id.length > 3`. + val minimumLengthCheck = """\.length\s*>\s*(\d+)""".r + + val checks = scalaFilesUnder(root).flatMap { file => + val source = Source.fromFile(file, "UTF-8") + try { + source.getLines().toList.zipWithIndex.collect { + case (line, i) if (line.contains("bank.id") || line.contains("bank_id") || + line.contains("postJson.id")) && + minimumLengthCheck.findFirstMatchIn(line).isDefined => + val smallestAccepted = minimumLengthCheck.findFirstMatchIn(line).get.group(1).toInt + 1 + (s"${file.getPath}:${i + 1}", smallestAccepted) + } + } finally source.close() + } + + withClue("no minimum length rule was found at all - the scan itself has stopped working: ") { + checks.size should be >= 5 + } + checks.foreach { case (where, smallestAccepted) => + withClue(s"$where accepts a bank id of $smallestAccepted characters, which no longer excludes " + + s"'$DYNAMIC_ENTITY_SYSTEM_LEVEL_BANK_ID': ") { + smallestAccepted should be > DYNAMIC_ENTITY_SYSTEM_LEVEL_BANK_ID.length + } + } + } + + // Stated on its own because it is the half of the rule that does NOT hold. The sentinel passes + // the shared charset and maximum length check, so if the minimum length rules above ever go, + // there is nothing standing behind them. + scenario("the shared charset and length rule does not by itself exclude it", DynamicEntitySpaceScope) { + APIUtil.checkShortString(DYNAMIC_ENTITY_SYSTEM_LEVEL_BANK_ID) shouldBe code.util.Helper.SILENCE_IS_GOLDEN + } + } +} From 5fe48c08abd93d67671fccab8771242d80ce491f Mon Sep 17 00:00:00 2001 From: simonredfern Date: Tue, 22 Sep 2026 02:58:57 +0200 Subject: [PATCH 2/5] Refactor whereClauseBankOrSystemAndEntity and reduce paths due to bank_id is SYS means system in Dynamic Entity queries. --- .../MappedDynamicDataAccessProvider.scala | 41 ++-- .../MapppedDynamicDataProvider.scala | 211 +++++++----------- 2 files changed, 103 insertions(+), 149 deletions(-) diff --git a/obp-api/src/main/scala/code/dynamicEntity/MappedDynamicDataAccessProvider.scala b/obp-api/src/main/scala/code/dynamicEntity/MappedDynamicDataAccessProvider.scala index be4696612e..f84014e248 100644 --- a/obp-api/src/main/scala/code/dynamicEntity/MappedDynamicDataAccessProvider.scala +++ b/obp-api/src/main/scala/code/dynamicEntity/MappedDynamicDataAccessProvider.scala @@ -53,8 +53,13 @@ object MappedDynamicDataAccessProvider extends DynamicDataAccessProvider { * A record id alone does not identify a record, because ids are only unique within one space and * one entity. Every query below starts from this list so that a grant made on the country code DE * in one space can never be read, revoked or cascaded as a grant on DE in another. + * + * The columns are always named in the order the things they identify come into existence -- the + * bank, then the entity defined within it, then the record, then the user the grant is for -- + * which is also the column order of the unique index these rows live under. Anything a query adds + * beyond these three is appended after them, so every query in this file reads the same way down. */ - private def scopeOf(bankId: Option[String], entityName: String, dynamicDataId: String): List[QueryParam[DynamicDataAccess]] = + private def whereClauseBankOrSystemAndEntityAndDataId(bankId: Option[String], entityName: String, dynamicDataId: String): List[QueryParam[DynamicDataAccess]] = List( By(DynamicDataAccess.BankId, storedBankId(bankId)), By(DynamicDataAccess.EntityName, entityName), @@ -65,20 +70,20 @@ object MappedDynamicDataAccessProvider extends DynamicDataAccessProvider { canRead: Boolean, canUpdate: Boolean, canDelete: Boolean, canGrant: Boolean, grantedBy: String): Box[DynamicDataAccessT] = tryo { val row = DynamicDataAccess.find( - (By(DynamicDataAccess.UserId, userId) :: scopeOf(bankId, entityName, dynamicDataId)): _* + (whereClauseBankOrSystemAndEntityAndDataId(bankId, entityName, dynamicDataId) :+ By(DynamicDataAccess.UserId, userId)): _* ).getOrElse( DynamicDataAccess.create + .BankId(storedBankId(bankId)) + .EntityName(entityName) .DynamicDataId(dynamicDataId) .UserId(userId) - .EntityName(entityName) - .BankId(storedBankId(bankId)) ) - row.CanRead(canRead) + row.BankId(storedBankId(bankId)) + .EntityName(entityName) + .CanRead(canRead) .CanUpdate(canUpdate) .CanDelete(canDelete) .CanGrant(canGrant) - .EntityName(entityName) - .BankId(storedBankId(bankId)) .GrantedBy(grantedBy) .saveMe() } @@ -87,7 +92,7 @@ object MappedDynamicDataAccessProvider extends DynamicDataAccessProvider { // Walk the GrantedBy edges within this single data row: remove the target user and // everyone they granted downstream. The visited-set makes re-share cycles terminate // and absorbs the owner row's self-edge (GrantedBy == UserId). - val scope = scopeOf(bankId, entityName, dynamicDataId) + val whereThisRecord = whereClauseBankOrSystemAndEntityAndDataId(bankId, entityName, dynamicDataId) val toRemove = mutable.LinkedHashSet[String](userId) val visited = mutable.Set[String]() var frontier = List(userId) @@ -97,7 +102,7 @@ object MappedDynamicDataAccessProvider extends DynamicDataAccessProvider { if (!visited.contains(current)) { visited += current val children = DynamicDataAccess.findAll( - (By(DynamicDataAccess.GrantedBy, current) :: scope): _* + (whereThisRecord :+ By(DynamicDataAccess.GrantedBy, current)): _* ).map(_.UserId.get).filterNot(visited.contains) children.foreach { child => toRemove += child @@ -106,26 +111,26 @@ object MappedDynamicDataAccessProvider extends DynamicDataAccessProvider { } } toRemove.toList.flatMap { uid => - DynamicDataAccess.findAll((By(DynamicDataAccess.UserId, uid) :: scope): _*) + DynamicDataAccess.findAll((whereThisRecord :+ By(DynamicDataAccess.UserId, uid)): _*) }.map(_.delete_!).count(identity) } override def getAccessForRow(bankId: Option[String], entityName: String, dynamicDataId: String): List[DynamicDataAccessT] = - DynamicDataAccess.findAll(scopeOf(bankId, entityName, dynamicDataId): _*) + DynamicDataAccess.findAll(whereClauseBankOrSystemAndEntityAndDataId(bankId, entityName, dynamicDataId): _*) override def getReadableDynamicDataIds(bankId: Option[String], entityName: String, userId: String): List[String] = DynamicDataAccess.findAll( - By(DynamicDataAccess.UserId, userId), + By(DynamicDataAccess.BankId, storedBankId(bankId)), By(DynamicDataAccess.EntityName, entityName), - By(DynamicDataAccess.CanRead, true), - By(DynamicDataAccess.BankId, storedBankId(bankId)) + By(DynamicDataAccess.UserId, userId), + By(DynamicDataAccess.CanRead, true) ).map(_.DynamicDataId.get) override def allows(bankId: Option[String], entityName: String, dynamicDataId: String, userId: String, permission: DynamicDataAccessPermission): Boolean = { import DynamicDataAccessPermission._ DynamicDataAccess.find( - (By(DynamicDataAccess.UserId, userId) :: scopeOf(bankId, entityName, dynamicDataId)): _* + (whereClauseBankOrSystemAndEntityAndDataId(bankId, entityName, dynamicDataId) :+ By(DynamicDataAccess.UserId, userId)): _* ).map { row => permission match { case Read => row.CanRead.get @@ -137,13 +142,13 @@ object MappedDynamicDataAccessProvider extends DynamicDataAccessProvider { } override def deleteAllForRow(bankId: Option[String], entityName: String, dynamicDataId: String): Box[Boolean] = tryo { - DynamicDataAccess.findAll(scopeOf(bankId, entityName, dynamicDataId): _*).forall(_.delete_!) + DynamicDataAccess.findAll(whereClauseBankOrSystemAndEntityAndDataId(bankId, entityName, dynamicDataId): _*).forall(_.delete_!) } override def deleteAllForEntity(bankId: Option[String], entityName: String): Box[Boolean] = tryo { DynamicDataAccess.findAll( - By(DynamicDataAccess.EntityName, entityName), - By(DynamicDataAccess.BankId, storedBankId(bankId)) + By(DynamicDataAccess.BankId, storedBankId(bankId)), + By(DynamicDataAccess.EntityName, entityName) ).forall(_.delete_!) } } diff --git a/obp-api/src/main/scala/code/dynamicEntity/MapppedDynamicDataProvider.scala b/obp-api/src/main/scala/code/dynamicEntity/MapppedDynamicDataProvider.scala index 5558d70ddd..ac21e03d9d 100644 --- a/obp-api/src/main/scala/code/dynamicEntity/MapppedDynamicDataProvider.scala +++ b/obp-api/src/main/scala/code/dynamicEntity/MapppedDynamicDataProvider.scala @@ -61,6 +61,66 @@ object MappedDynamicDataProvider extends DynamicDataProvider with CustomJsonForm private def ownerOf(userId: Option[String]): Option[String] = userId.map(id => code.users.Users.users.vend.attributedUserId(id, code.users.UserReference.DynamicData_UserId).openOr(id)) + /** + * The bank and the entity that a lookup is confined to. + * + * Every query in this provider starts from these two, so that a record held in one space can + * never be found, updated or deleted through a request naming another. A record belonging to no + * bank is stored under Constant.DYNAMIC_ENTITY_SYSTEM_LEVEL_BANK_ID rather than as a SQL NULL, + * and that is what lets the system level case share this one query: it differs from a space only + * in the value bound here. While the column held NULL the two needed different SQL -- NullRef + * against a plain equality -- and each method below carried a separate copy of itself for each. + * + * The two are named in the order the things they identify come into existence, the bank first and + * then the entity defined within it, which is also the order of the columns in DynamicData's + * unique index. A query needing more than these two appends it -- the record id next, then the + * ownership predicate -- so every query in this file reads in that same order. + */ + private def whereClauseBankOrSystemAndEntity(bankId: Option[String], entityName: String): List[QueryParam[DynamicData]] = + List( + By(DynamicData.BankId, bankId.getOrElse(DYNAMIC_ENTITY_SYSTEM_LEVEL_BANK_ID)), + By(DynamicData.DynamicEntityName, entityName) + ) + + /** + * Whose records of an entity the caller is asking about. + * + * Each record of a personal entity belongs to one user, and is selected by naming that user. + * Records of any other entity are selected by the flag alone, with no user named at all. + */ + private def byOwnership(isPersonalEntity: Boolean, userId: Option[String]): QueryParam[DynamicData] = + if (isPersonalEntity) By(DynamicData.UserId, userId.getOrElse(null)) + else By(DynamicData.IsPersonalEntity, false) + + /** + * The wording of the not-found failure for an owner-scoped read, which varies with what the + * caller named. + * + * These four spellings are reproduced exactly as they stood when each case had its own copy of + * the query, because they reach callers as the body of a not-found response. Two of them are + * irregular and are deliberately left that way: one renders the user id as an Option, so it reads + * "userId = Some(x)", and the bank id is preceded by a space here that the community wording + * below does not have. The one deviation is that a missing user id used to be read with .get in + * the last case, which threw while building the message instead of producing one; it now reads + * as null, and no message that previously rendered is changed. + */ + private def notFoundMessage(entityName: String, id: String, bankId: Option[String], + userId: Option[String], isPersonalEntity: Boolean): String = { + val base = s"$DynamicDataNotFound dynamicEntityName=$entityName, dynamicDataId=$id" + (bankId.isDefined, isPersonalEntity) match { + case (false, false) => base + case (false, true) => s"$base, userId = $userId" + case (true, false) => s"$base, bankId= ${bankId.get}" + case (true, true) => s"$base, bankId= ${bankId.get}, userId = ${userId.getOrElse(null)}" + } + } + + /** As notFoundMessage, for the community reads, whose bank id carries no leading space. */ + private def notFoundCommunityMessage(entityName: String, id: String, bankId: Option[String]): String = { + val base = s"$DynamicDataNotFound dynamicEntityName=$entityName, dynamicDataId=$id" + bankId.map(theBank => s"$base, bankId=$theBank").getOrElse(base) + } + override def save(bankId: Option[String], entityName: String, requestBody: JObject, userId: Option[String], isPersonalEntity: Boolean): Box[DynamicDataT] = { val idName = getIdName(entityName) val JString(idValue) = (requestBody \ idName).asInstanceOf[JString] @@ -78,59 +138,23 @@ object MappedDynamicDataProvider extends DynamicDataProvider with CustomJsonForm def existsById(entityName: String, id: String): Boolean = { println(s"========== Reference validation: checking if DynamicDataId='$id' exists for DynamicEntityName='$entityName' ==========") val exists = DynamicData.count( - By(DynamicData.DynamicDataId, id), - By(DynamicData.DynamicEntityName, entityName) + By(DynamicData.DynamicEntityName, entityName), + By(DynamicData.DynamicDataId, id) ) > 0 println(s"========== Reference validation result: exists=$exists ==========") exists } - override def get(bankId: Option[String],entityName: String, id: String, callerUserId: Option[String], isPersonalEntity: Boolean): Box[DynamicDataT] = { + override def get(bankId: Option[String], entityName: String, id: String, callerUserId: Option[String], isPersonalEntity: Boolean): Box[DynamicDataT] = { val userId = ownerOf(callerUserId) - if(bankId.isEmpty && !isPersonalEntity ){ //isPersonalEntity == false, get all the data, no need for specific userId. - //forced the empty also to a error here. this is get Dynamic by Id, if it return Empty, better show the error in this level. - DynamicData.find( - By(DynamicData.DynamicDataId, id), - By(DynamicData.DynamicEntityName, entityName), - By(DynamicData.IsPersonalEntity, false), - By(DynamicData.BankId, DYNAMIC_ENTITY_SYSTEM_LEVEL_BANK_ID) - ) match { - case Full(dynamicData) => Full(dynamicData) - case _ => Failure(s"$DynamicDataNotFound dynamicEntityName=$entityName, dynamicDataId=$id") - } - } else if(bankId.isEmpty && isPersonalEntity){ //isPersonalEntity == true, get the data for specific userId (regardless of how it was created). - DynamicData.find( - By(DynamicData.DynamicDataId, id), - By(DynamicData.DynamicEntityName, entityName), - By(DynamicData.UserId, userId.getOrElse(null)), - By(DynamicData.BankId, DYNAMIC_ENTITY_SYSTEM_LEVEL_BANK_ID) - ) match { - case Full(dynamicData) => Full(dynamicData) - case _ => Failure(s"$DynamicDataNotFound dynamicEntityName=$entityName, dynamicDataId=$id, userId = $userId") - } - } else if(bankId.isDefined && !isPersonalEntity ){ //isPersonalEntity == false, get all the data, no need for specific userId. - //forced the empty also to a error here. this is get Dynamic by Id, if it return Empty, better show the error in this level. - DynamicData.find( - By(DynamicData.DynamicDataId, id), - By(DynamicData.DynamicEntityName, entityName), - By(DynamicData.IsPersonalEntity, false), - By(DynamicData.BankId, bankId.get), - ) match { - case Full(dynamicData) => Full(dynamicData) - case _ => Failure(s"$DynamicDataNotFound dynamicEntityName=$entityName, dynamicDataId=$id, bankId= ${bankId.get}") - } - }else{ //isPersonalEntity == true, get the data for specific userId (regardless of how it was created). - DynamicData.find( - By(DynamicData.DynamicDataId, id), - By(DynamicData.DynamicEntityName, entityName), - By(DynamicData.BankId, bankId.get), - By(DynamicData.UserId, userId.get) - ) match { - case Full(dynamicData) => Full(dynamicData) - case _ => Failure(s"$DynamicDataNotFound dynamicEntityName=$entityName, dynamicDataId=$id, bankId= ${bankId.get}, userId = ${userId.get}") - } + // An Empty is turned into a Failure rather than passed up: this is get-by-id, and the reason + // nothing came back is worth stating at this level. + DynamicData.find( + (whereClauseBankOrSystemAndEntity(bankId, entityName) :+ By(DynamicData.DynamicDataId, id) :+ byOwnership(isPersonalEntity, userId)): _* + ) match { + case Full(dynamicData) => Full(dynamicData) + case _ => Failure(notFoundMessage(entityName, id, bankId, userId, isPersonalEntity)) } - } override def getAllDataJson(bankId: Option[String], entityName: String, userId: Option[String], isPersonalEntity: Boolean): List[JObject] = { @@ -141,31 +165,7 @@ object MappedDynamicDataProvider extends DynamicDataProvider with CustomJsonForm override def getAll(bankId: Option[String], entityName: String, callerUserId: Option[String], isPersonalEntity: Boolean): List[DynamicDataT] = { val userId = ownerOf(callerUserId) - if(bankId.isEmpty && !isPersonalEntity){ //isPersonalEntity == false, get all the data, no need for specific userId. - DynamicData.findAll( - By(DynamicData.DynamicEntityName, entityName), - By(DynamicData.IsPersonalEntity, false), - By(DynamicData.BankId, DYNAMIC_ENTITY_SYSTEM_LEVEL_BANK_ID), - ) - } else if(bankId.isEmpty && isPersonalEntity){ //isPersonalEntity == true, get all the data for specific userId (regardless of how it was created). - DynamicData.findAll( - By(DynamicData.DynamicEntityName, entityName), - By(DynamicData.UserId, userId.getOrElse(null)), - By(DynamicData.BankId, DYNAMIC_ENTITY_SYSTEM_LEVEL_BANK_ID) - ) - } else if(bankId.isDefined && !isPersonalEntity){ //isPersonalEntity == false, get all the data, no need for specific userId. - DynamicData.findAll( - By(DynamicData.DynamicEntityName, entityName), - By(DynamicData.IsPersonalEntity, false), - By(DynamicData.BankId, bankId.get), - ) - }else{ - DynamicData.findAll(//isPersonalEntity == true, get all the data for specific userId (regardless of how it was created). - By(DynamicData.DynamicEntityName, entityName), - By(DynamicData.BankId, bankId.get), - By(DynamicData.UserId, userId.getOrElse(null)) - ) - } + DynamicData.findAll((whereClauseBankOrSystemAndEntity(bankId, entityName) :+ byOwnership(isPersonalEntity, userId)): _*) } override def delete(bankId: Option[String], entityName: String, id: String, userId: Option[String], isPersonalEntity: Boolean) = { @@ -178,19 +178,8 @@ object MappedDynamicDataProvider extends DynamicDataProvider with CustomJsonForm } // Community access: return ALL records regardless of userId/IsPersonalEntity - override def getAllCommunity(bankId: Option[String], entityName: String): List[DynamicDataT] = { - if (bankId.isEmpty) { - DynamicData.findAll( - By(DynamicData.DynamicEntityName, entityName), - By(DynamicData.BankId, DYNAMIC_ENTITY_SYSTEM_LEVEL_BANK_ID), - ) - } else { - DynamicData.findAll( - By(DynamicData.DynamicEntityName, entityName), - By(DynamicData.BankId, bankId.get), - ) - } - } + override def getAllCommunity(bankId: Option[String], entityName: String): List[DynamicDataT] = + DynamicData.findAll(whereClauseBankOrSystemAndEntity(bankId, entityName): _*) override def getAllDataJsonCommunity(bankId: Option[String], entityName: String): List[JObject] = { getAllCommunity(bankId, entityName) @@ -198,27 +187,11 @@ object MappedDynamicDataProvider extends DynamicDataProvider with CustomJsonForm .map(_.asInstanceOf[JObject]) } - override def getCommunity(bankId: Option[String], entityName: String, id: String): Box[DynamicDataT] = { - if (bankId.isEmpty) { - DynamicData.find( - By(DynamicData.DynamicDataId, id), - By(DynamicData.DynamicEntityName, entityName), - By(DynamicData.BankId, DYNAMIC_ENTITY_SYSTEM_LEVEL_BANK_ID) - ) match { - case Full(dynamicData) => Full(dynamicData) - case _ => Failure(s"$DynamicDataNotFound dynamicEntityName=$entityName, dynamicDataId=$id") - } - } else { - DynamicData.find( - By(DynamicData.DynamicDataId, id), - By(DynamicData.DynamicEntityName, entityName), - By(DynamicData.BankId, bankId.get), - ) match { - case Full(dynamicData) => Full(dynamicData) - case _ => Failure(s"$DynamicDataNotFound dynamicEntityName=$entityName, dynamicDataId=$id, bankId=${bankId.get}") - } + override def getCommunity(bankId: Option[String], entityName: String, id: String): Box[DynamicDataT] = + DynamicData.find((whereClauseBankOrSystemAndEntity(bankId, entityName) :+ By(DynamicData.DynamicDataId, id)): _*) match { + case Full(dynamicData) => Full(dynamicData) + case _ => Failure(notFoundCommunityMessage(entityName, id, bankId)) } - } override def updateCommunity(bankId: Option[String], entityName: String, requestBody: JObject, id: String): Box[DynamicDataT] = { val dynamicData = getCommunity(bankId, entityName, id) @@ -239,40 +212,16 @@ object MappedDynamicDataProvider extends DynamicDataProvider with CustomJsonForm override def existsData(bankId: Option[String], dynamicEntityName: String, callerUserId: Option[String], isPersonalEntity: Boolean): Boolean = { val userId = ownerOf(callerUserId) - if(bankId.isEmpty && !isPersonalEntity){//isPersonalEntity == false, get all the data, no need for specific userId. - DynamicData.find( - By(DynamicData.DynamicEntityName, dynamicEntityName), - By(DynamicData.BankId, DYNAMIC_ENTITY_SYSTEM_LEVEL_BANK_ID), - By(DynamicData.IsPersonalEntity, false) - ).isDefined - } else if(bankId.isDefined && !isPersonalEntity){//isPersonalEntity == false, get all the data, no need for specific userId. - DynamicData.find( - By(DynamicData.DynamicEntityName, dynamicEntityName), - By(DynamicData.BankId, bankId.get), - By(DynamicData.IsPersonalEntity, false) - ).nonEmpty - } else if(bankId.isEmpty && isPersonalEntity){ //isPersonalEntity == true, check if data exists for specific userId (regardless of how it was created). - DynamicData.find( - By(DynamicData.DynamicEntityName, dynamicEntityName), - By(DynamicData.BankId, DYNAMIC_ENTITY_SYSTEM_LEVEL_BANK_ID), - By(DynamicData.UserId, userId.getOrElse(null)) - ).nonEmpty - } else { //isPersonalEntity == true, check if data exists for specific userId (regardless of how it was created). - DynamicData.find( - By(DynamicData.DynamicEntityName, dynamicEntityName), - By(DynamicData.BankId, bankId.get), - By(DynamicData.UserId, userId.getOrElse(null)) - ).nonEmpty - } + DynamicData.find((whereClauseBankOrSystemAndEntity(bankId, dynamicEntityName) :+ byOwnership(isPersonalEntity, userId)): _*).isDefined } private def saveOrUpdate(bankId: Option[String], entityName: String, requestBody: JObject, userId: Option[String], isPersonalEntity: Boolean, dynamicData: => DynamicData): Box[DynamicData] = { val data: DynamicData = dynamicData tryo { val dataStr = json.compactRender(requestBody) - val saved = data.DataJson(dataStr) + val saved = data.BankId(bankId.getOrElse(DYNAMIC_ENTITY_SYSTEM_LEVEL_BANK_ID)) .DynamicEntityName(entityName) - .BankId(bankId.getOrElse(DYNAMIC_ENTITY_SYSTEM_LEVEL_BANK_ID)) + .DataJson(dataStr) .UserId(userId.getOrElse(null)) .IsPersonalEntity(isPersonalEntity) .saveMe() From 38c5d85bfadf92726fc8955d9b4716df246d398e Mon Sep 17 00:00:00 2001 From: simonredfern Date: Tue, 22 Sep 2026 14:48:45 +0200 Subject: [PATCH 3/5] Adding extra props for MCP --- .../resources/props/sample.props.template | 33 ++++++++++++++++++- .../SwaggerDefinitionsJSON.scala | 3 +- .../main/scala/code/api/util/APIUtil.scala | 9 +++-- .../main/scala/code/api/util/Glossary.scala | 6 +++- .../scala/code/api/util/http4s/AppsPage.scala | 3 +- .../scala/code/api/v7_0_0/Http4s700.scala | 27 ++++++++++++--- release_notes.md | 30 +++++++++++++++++ 7 files changed, 100 insertions(+), 11 deletions(-) diff --git a/obp-api/src/main/resources/props/sample.props.template b/obp-api/src/main/resources/props/sample.props.template index 0882711cb1..38eef34cf2 100644 --- a/obp-api/src/main/resources/props/sample.props.template +++ b/obp-api/src/main/resources/props/sample.props.template @@ -369,7 +369,38 @@ public_keycloak_url=http://localhost:7787 # Docker (Dockerfile EXPOSE 8087) -> 8087 (or override via SERVER_PORT env var) # The default below matches Docker; for local non-Docker dev use http://localhost:48123 instead. public_obp_hola_url=http://localhost:8087 -public_obp_mcp_url=http://localhost:9100 +# --------------------------------------------------------------------------- +# OBP-MCP — the Model Context Protocol tool surface for OBP-API. +# +# Two instances may run side by side. They expose the same tools; they differ +# in who they serve, and therefore how they authenticate: +# +# external run_server_oauth.sh, :9101 (mirrors k8s-mcp2) +# AUTH_PROVIDER=obp-oidc, OBP_AUTHORIZATION_VIA=oauth +# Full OAuth 2.1 + Dynamic Client Registration. For MCP clients +# that sign a user in and cannot drive a consent flow themselves +# — Claude Code, VS Code, Claude Desktop. +# +# internal run_server.sh, :9100 +# AUTH_PROVIDER=bearer-only, OBP_AUTHORIZATION_VIA=consent +# For agents running inside the deployment — Opey reaches it via +# its own mcp_servers.json, not via this prop. +# +# public_obp_mcp_url is the one the App Directory advertises, so it must name +# an instance an outside client can actually authenticate against. Pointed at +# the internal instance it yields a server that connects and lists tools, then +# fails every call with consent_required. +# +# The public_ prefix marks a prop as an App Directory entry (see +# APIUtil.publicAppUrlDefaults) — it is not a claim that the target is +# internet-facing. The internal instance is listed so operators can see and +# health-probe it. +# +# Each instance reports its own live mode at /status, which is the source +# of truth. These props only say where the instances are. +# --------------------------------------------------------------------------- +public_obp_mcp_url=http://localhost:9101 +public_obp_mcp_internal_url=http://localhost:9100 public_obp_opey_url=http://localhost:5000 # OBP-Stripe subscription / payment service (Go server, default port 4242) public_obp_stripe_url=http://localhost:4242 diff --git a/obp-api/src/main/scala/code/api/ResourceDocs1_4_0/SwaggerDefinitionsJSON.scala b/obp-api/src/main/scala/code/api/ResourceDocs1_4_0/SwaggerDefinitionsJSON.scala index b9e0586341..1549b7fcc9 100644 --- a/obp-api/src/main/scala/code/api/ResourceDocs1_4_0/SwaggerDefinitionsJSON.scala +++ b/obp-api/src/main/scala/code/api/ResourceDocs1_4_0/SwaggerDefinitionsJSON.scala @@ -6390,7 +6390,8 @@ object SwaggerDefinitionsJSON { ConfigPropJsonV600("public_obp_oidc_url", "http://localhost:9000"), ConfigPropJsonV600("public_keycloak_url", "http://localhost:7787"), ConfigPropJsonV600("public_obp_hola_url", "http://localhost:48123"), - ConfigPropJsonV600("public_obp_mcp_url", "http://localhost:9100"), + ConfigPropJsonV600("public_obp_mcp_url", "http://localhost:9101"), + ConfigPropJsonV600("public_obp_mcp_internal_url", "http://localhost:9100"), ConfigPropJsonV600("public_obp_opey_url", "http://localhost:5000"), ConfigPropJsonV600("public_obp_stripe_url", "http://localhost:4242") ) 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 62f51a4274..1ef2aa71be 100644 --- a/obp-api/src/main/scala/code/api/util/APIUtil.scala +++ b/obp-api/src/main/scala/code/api/util/APIUtil.scala @@ -3772,8 +3772,10 @@ object APIUtil extends MdcLoggable with CustomJsonFormats{ } // Convention-based public app URL props. - // Any prop starting with "public_" and ending with "_url" is included in the App Directory. - // Register known defaults so they appear in getConfigPropsPairs when set. + // Every key registered here appears in getConfigPropsPairs and so in the App Directory. + // The set is fixed in code: an operator-added public_*_url prop is NOT picked up + // automatically, because getConfigPropsPairs reads getRegisteredDefaults rather than + // scanning the props file. // Note: public_obp_api_url falls back to hostname prop if not explicitly set. // Note: public_obp_portal_url falls back to portal_external_url if not explicitly set. val publicAppUrlDefaults: Map[String, String] = Map( @@ -3785,7 +3787,8 @@ object APIUtil extends MdcLoggable with CustomJsonFormats{ "public_obp_oidc_url" -> getPropsValue("public_obp_oidc_url").openOr("http://localhost:9000"), "public_keycloak_url" -> getPropsValue("public_keycloak_url").openOr("http://localhost:7787"), "public_obp_hola_url" -> getPropsValue("public_obp_hola_url").openOr("http://localhost:48123"), - "public_obp_mcp_url" -> getPropsValue("public_obp_mcp_url").openOr("http://localhost:9100"), + "public_obp_mcp_url" -> getPropsValue("public_obp_mcp_url").openOr("http://localhost:9101"), + "public_obp_mcp_internal_url" -> getPropsValue("public_obp_mcp_internal_url").openOr("http://localhost:9100"), "public_obp_opey_url" -> getPropsValue("public_obp_opey_url").openOr("http://localhost:5000"), "public_obp_stripe_url" -> getPropsValue("public_obp_stripe_url").openOr("http://localhost:4242"), "public_rabbit_cats_adapter_url" -> getPropsValue("public_rabbit_cats_adapter_url").openOr("http://localhost:8089") diff --git a/obp-api/src/main/scala/code/api/util/Glossary.scala b/obp-api/src/main/scala/code/api/util/Glossary.scala index 5ab7a69472..2f9407be12 100644 --- a/obp-api/src/main/scala/code/api/util/Glossary.scala +++ b/obp-api/src/main/scala/code/api/util/Glossary.scala @@ -6320,6 +6320,8 @@ object Glossary extends MdcLoggable { | |OBP provides a built-in Chat / Messaging API that allows users and applications to communicate within the platform. | + |Chat is the persistent, human-facing side of messaging: rooms, threads, reactions and read markers, all stored in the database. Messages between AI agents belong somewhere else. For short-lived agent-to-agent messages, discovery and presence, see [Signal Channels](/glossary#Signal-Channels), which are Redis-backed, are never written to the database, and expire when a channel goes quiet. + | |Chat Rooms can be scoped to a specific Bank (bank-level) or be system-wide (system-level). | |## Key Concepts @@ -6461,7 +6463,7 @@ object Glossary extends MdcLoggable { |The Glossary is one resource and it reads without a token. | |* `GET /obp/v7.0.0/api/glossary/TITLE` — **one Item**. Ask for what you need by title; the whole Glossary is about a megabyte and you rarely want all of it. The title is matched case insensitively, with hyphens, underscores, slashes and spaces all treated alike, so the `Signal-Channels` form you meet in a `/glossary#Signal-Channels` link finds the Item titled `Signal Channels`. Titles contain spaces, so url-encode the segment. - |* `GET /obp/v7.0.0/api/glossary` — the whole Glossary when you do want it. `?search=consent` narrows by title, `?limit=` and `?offset=` page, and `total_count` says how many matched. + |* `GET /obp/v7.0.0/api/glossary` — the whole Glossary when you do want it. `?search=consent` narrows to the Items that mention it, looking at both the title and the text of each Item, so you can search for what a thing does when you do not know what it is called here: `?search=agent messages` finds the Item titled `Signal Channels`. Every word you type has to appear, and Items matching in the title are listed first. `?limit=` and `?offset=` page, and `total_count` says how many matched. |* Each entry says where it came from: `is_dynamic` is false for Items shipped with the API and true for Items an operator added to this instance. An Item's `description` arrives as both `markdown` and rendered `html`. | |Same path, same version, for the Items an operator maintains: `POST /obp/v7.0.0/api/glossary` adds one, `PUT` and `DELETE` on `/obp/v7.0.0/api/glossary/TITLE` change or remove it. Those need a Role, which you will not have; they are listed here so you can tell a human what to ask for. @@ -6507,6 +6509,8 @@ object Glossary extends MdcLoggable { | |Not to be confused with [Chat](/glossary#Chat), which is the persistent, human-facing messaging surface (rooms, threads, reactions, read markers). | + |**Other names for the same thing.** People and agents arrive here looking for agent messages, agent messaging, agent-to-agent (A2A) communication, inter-agent messaging, a message bus, a pub/sub or publish and subscribe channel, broadcast messages, agent discovery, or agent presence. Signal Channels are what the Open Bank Project calls all of those. If you are an agent meeting this instance for the first time, read [Hello AI Agents](/glossary#Hello-AI-Agents) first; it says how to get credentials and where to announce yourself. + | |## Lifecycle |- Channels are auto-created on first publish; no registration step. Creating channels is rate limited per caller (scope `signal_channel_create`, see [Rate Limiting](/glossary#Rate-Limiting)); publishing to an existing channel is not. |- On this instance a channel expires ${code.api.cache.RedisMessaging.channelTtlSeconds} seconds after its last publish, and holds at most ${code.api.cache.RedisMessaging.channelMaxMessages} messages (oldest are trimmed). diff --git a/obp-api/src/main/scala/code/api/util/http4s/AppsPage.scala b/obp-api/src/main/scala/code/api/util/http4s/AppsPage.scala index 3a923a511e..c0c5cd7a7f 100644 --- a/obp-api/src/main/scala/code/api/util/http4s/AppsPage.scala +++ b/obp-api/src/main/scala/code/api/util/http4s/AppsPage.scala @@ -53,7 +53,7 @@ object AppsPage { } } - private val acronyms = Set("obp", "api", "mcp") + private val acronyms = Set("obp", "api", "mcp", "oidc") // Render order for probe endpoints (also controls which endpoints are known). private val probeEndpoints = List("status", "health", "ready") @@ -68,6 +68,7 @@ object AppsPage { "public_obp_opey_url" -> Set("status", "health"), "public_obp_api_explorer_url" -> Set("status", "health"), "public_obp_mcp_url" -> Set("status", "health", "ready"), + "public_obp_mcp_internal_url" -> Set("status", "health", "ready"), "public_obp_hola_url" -> Set("status", "health"), "public_obp_stripe_url" -> Set("status"), ) diff --git a/obp-api/src/main/scala/code/api/v7_0_0/Http4s700.scala b/obp-api/src/main/scala/code/api/v7_0_0/Http4s700.scala index bb8452b27a..97f27fd377 100644 --- a/obp-api/src/main/scala/code/api/v7_0_0/Http4s700.scala +++ b/obp-api/src/main/scala/code/api/v7_0_0/Http4s700.scala @@ -3641,13 +3641,32 @@ object Http4s700 { _ <- glossaryReadIsAllowed(cc) _ <- Helper.booleanToFuture(InvalidGlossarySource, 400, Some(cc))(Set("all", "static", "dynamic")(source)) } yield { - val matching = glossaryEntries.filter { case (item, _) => - (source match { + // Search looks at the text of an Item as well as its title, and takes the words of the + // search separately rather than as one string. Someone who does not yet know what a + // thing is called here cannot guess its title, so a search for "agent messages" has to + // find the Item titled "Signal Channels"; matching the whole phrase against titles + // alone found nothing at all. Every word must appear somewhere in the Item, and the + // Items whose titles hold all of them are listed first, so an exact title still leads. + val searchWords: List[String] = search.toList.flatMap(_.split("\\s+")).filter(_.nonEmpty) + val inSource = glossaryEntries.filter { case (item, _) => + source match { case "static" => !item.isDynamic case "dynamic" => item.isDynamic case _ => true - }) && search.forall(s => item.title.toLowerCase.contains(s)) + } } + val matching = + if (searchWords.isEmpty) inSource + else { + val (titleMatches, bodyMatches) = inSource.filter { case (item, _) => + val titleAndText = s"${item.title}\n${item.textDescription}".toLowerCase + searchWords.forall(titleAndText.contains) + }.partition { case (item, _) => + val title = item.title.toLowerCase + searchWords.forall(title.contains) + } + titleMatches ++ bodyMatches + } // Paging is opt in: without limit the Glossary answers whole, as it has for years. val page = limit match { case Some(n) => matching.slice(offset, offset + n.max(0)) @@ -3684,7 +3703,7 @@ object Http4s700 { | |**Optional query parameters:** | - |* `search` — only Items whose title contains this value, case insensitively. + |* `search` — only Items that contain what you type, case insensitively, looking at the title and at the text of the Item. The words are matched one by one and an Item has to contain all of them, so `search=agent messages` finds the Item titled `Signal Channels` even though neither word is in its title. Items whose titles contain every word are listed before those that only match on their text. |* `source` — `all` (default), `static` or `dynamic`. |* `limit` and `offset` — page the result. Without `limit` the whole Glossary is returned, as it always has been. `total_count` counts what matched before paging. | diff --git a/release_notes.md b/release_notes.md index 9a7ded3d3f..3cd53b6b57 100644 --- a/release_notes.md +++ b/release_notes.md @@ -3,6 +3,36 @@ ### Most recent changes at top of file ``` Date Commit Action +22/09/2026 TBD CONFIG CHANGE: public_obp_mcp_url now denotes the MCP instance that + external clients can authenticate against (AUTH_PROVIDER=obp-oidc, + OBP_AUTHORIZATION_VIA=oauth, full OAuth 2.1 + Dynamic Client + Registration), rather than simply "the MCP server". Its built-in + default moves from http://localhost:9100 to http://localhost:9101. + + A second prop, public_obp_mcp_internal_url, names the internal + instance that Opey uses (AUTH_PROVIDER=bearer-only, + OBP_AUTHORIZATION_VIA=consent), default http://localhost:9100. + + DevOps action: in each environment, check what + OBP_PUBLIC_OBP_MCP_URL is set to. If it points at the internal / + Opey instance, move that value to OBP_PUBLIC_OBP_MCP_INTERNAL_URL + and set OBP_PUBLIC_OBP_MCP_URL to the external OAuth instance. If it + already points at the external instance, leave it and add the + internal one. If either variable is unset, note that the built-in + default for public_obp_mcp_url has moved port. + + Why it matters: these props feed the public App Directory (GET /apps, + authentication not required), which external clients and agents use + for discovery. If public_obp_mcp_url names the internal instance, a + client connects and can list tools but every call then fails with + consent_required - a failure that presents as a healthy server. + + Verify with: curl -H 'Accept: application/json' /apps and confirm + both MCP entries appear with the expected URLs. Each instance reports + its own live mode at /status (field: Outbound to OBP-API + (OBP_AUTHORIZATION_VIA)); that is the source of truth, while the props + only say where the instances are. + 15/08/2026 614e7294e BUILD/DEPLOY CHANGE: obp-api and obp-commons are built with Scala 2.13. The class files this produces are Java 25, where 2.12 emitted Java 8 whatever -release said - 2.13 honours -release fully. Anything loading From a15bf63767cc11b473b2923ebbe9da0b968d0e31 Mon Sep 17 00:00:00 2001 From: simonredfern Date: Tue, 22 Sep 2026 15:45:42 +0200 Subject: [PATCH 4/5] Update DynamicGlossaryItemTest.scala --- .../api/v7_0_0/DynamicGlossaryItemTest.scala | 26 +++++++++++++++++++ 1 file changed, 26 insertions(+) diff --git a/obp-api/src/test/scala/code/api/v7_0_0/DynamicGlossaryItemTest.scala b/obp-api/src/test/scala/code/api/v7_0_0/DynamicGlossaryItemTest.scala index 6f51d7b252..f3c8457993 100644 --- a/obp-api/src/test/scala/code/api/v7_0_0/DynamicGlossaryItemTest.scala +++ b/obp-api/src/test/scala/code/api/v7_0_0/DynamicGlossaryItemTest.scala @@ -493,6 +493,32 @@ class DynamicGlossaryItemTest extends ServerSetupWithTestData { delete(title, user1).code should equal(204) } + scenario("search reads the text of an Item, so a reader who does not know the title still finds it", ApiEndpoint6, VersionOfApi) { + When("someone searches for what a thing does rather than for what it is called here") + val byWords = makeGetRequest((v7 / "api" / "glossary").GET < Date: Tue, 22 Sep 2026 15:54:52 +0200 Subject: [PATCH 5/5] Tweaking migration --- .../scala/code/api/util/migration/Migration.scala | 12 ++++++++++-- 1 file changed, 10 insertions(+), 2 deletions(-) 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 2f272ccbc4..3031d46d78 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 @@ -247,14 +247,22 @@ object Migration extends MdcLoggable { val endDate = System.currentTimeMillis() val didSomething = outcomes.exists(_.changedSomething) val anythingFailed = outcomes.exists(_.failed) - val alreadyRecorded = MigrationScriptLogProvider.migrationScriptLogProvider.vend.isExecuted(name) + // Boot calls this before Schemifier, so on a database Schemifier has not created yet there is + // no migrationscriptlog table to read or to write. Reading it there raises a SQL error, and an + // error thrown at this point aborts the whole boot rather than one migration -- which is what + // a completely fresh database, such as the empty H2 every CI test shard starts from, gets. The + // steps above each found nothing to prepare on such a database, so there is nothing to record + // either, and the log is skipped until a later boot has a table to write it to. + val migrationLogTableExists = DbFunction.tableExistsByName("migrationscriptlog") + val alreadyRecorded = migrationLogTableExists && + MigrationScriptLogProvider.migrationScriptLogProvider.vend.isExecuted(name) // An entry is written on the boot that does the work, on any boot that fails, and on the first // boot that finds the schema already correct. That last case matters: an instance whose data // was converted by a build predating this logging, or a database created fresh with the new // index already in DynamicData.dbIndexes, would otherwise have nothing here at all, and the // operator could not tell "this ran and there was nothing to do" from "this never ran". // Every instance therefore ends up with exactly one entry, and it says which of the two it was. - if (didSomething || anythingFailed || !alreadyRecorded) { + if (migrationLogTableExists && (didSomething || anythingFailed || !alreadyRecorded)) { val summary = if (anythingFailed) "Completed with failures" else if (didSomething) "Applied"