diff --git a/.github/workflows/build_container.yml b/.github/workflows/build_container.yml index f9f87b6762..9e33a00b86 100644 --- a/.github/workflows/build_container.yml +++ b/.github/workflows/build_container.yml @@ -27,6 +27,9 @@ jobs: # -------------------------------------------------------------------------- compile: runs-on: ubuntu-latest + # The test job has carried a timeout since it was written; compile and report never did, + # so a hung Maven resolve blocks the build until GitHub's own 6-hour ceiling. + timeout-minutes: 25 steps: - uses: actions/checkout@v4 @@ -460,6 +463,7 @@ jobs: needs: test runs-on: ubuntu-latest if: always() + timeout-minutes: 10 steps: - uses: actions/checkout@v4 diff --git a/.github/workflows/build_pull_request.yml b/.github/workflows/build_pull_request.yml index 39716fe115..e50e4ba255 100644 --- a/.github/workflows/build_pull_request.yml +++ b/.github/workflows/build_pull_request.yml @@ -25,6 +25,9 @@ jobs: # -------------------------------------------------------------------------- compile: runs-on: ubuntu-latest + # The test job has carried a timeout since it was written; compile and report never did, + # so a hung Maven resolve blocks the build until GitHub's own 6-hour ceiling. + timeout-minutes: 25 steps: - uses: actions/checkout@v4 @@ -302,6 +305,17 @@ jobs: echo allow_user_generated_scala_code=true >> obp-api/src/main/resources/props/test.default.props - name: Run tests — shard ${{ matrix.shard }} (${{ matrix.name }}) + env: + # This job has declared a redis service since it was written, but nothing ever failed + # when the service was absent: ConcurrentRateLimiterRaceTest and + # MethodRoutingCacheInvalidationTest each `assume` a reachable Redis and cancel + # otherwise, and a cancelled test reports as a pass. Dropping the services: block, or + # a container that never became healthy, would have taken the rate-limiter and + # cache-invalidation races out of the run without changing a single report. + # + # RedisTestTarget turns that cancellation into a failure wherever this is set. + # Developers leave it unset and keep the skip. + OBP_TEST_REDIS_REQUIRED: "true" run: | # wildcardSuites requires comma-separated package prefixes (-w per entry). # The YAML >- scalar collapses newlines to spaces, so we convert here. @@ -453,6 +467,7 @@ jobs: needs: test runs-on: ubuntu-latest if: always() + timeout-minutes: 10 steps: - uses: actions/checkout@v4 diff --git a/obp-api/src/main/scala/code/api/UKOpenBanking/v2_0_0/Http4sUKOBv200.scala b/obp-api/src/main/scala/code/api/UKOpenBanking/v2_0_0/Http4sUKOBv200.scala index f7a58e2c74..b6a2c56045 100644 --- a/obp-api/src/main/scala/code/api/UKOpenBanking/v2_0_0/Http4sUKOBv200.scala +++ b/obp-api/src/main/scala/code/api/UKOpenBanking/v2_0_0/Http4sUKOBv200.scala @@ -4,6 +4,7 @@ import cats.data.{Kleisli, OptionT} import cats.effect._ import code.api.util.APIUtil.ResourceDoc import code.api.util.http4s.ResourceDocMiddleware +import code.api.util.http4s.IdempotencyMiddleware import code.util.Helper.MdcLoggable import com.openbankproject.commons.util.ApiVersion import org.http4s._ @@ -33,5 +34,5 @@ object Http4sUKOBv200 extends MdcLoggable { Http4sUKOBv200AIS.routes(req) } - val wrappedRoutes: HttpRoutes[IO] = ResourceDocMiddleware.apply(resourceDocs)(allRoutes) + val wrappedRoutes: HttpRoutes[IO] = ResourceDocMiddleware.apply(resourceDocs)(IdempotencyMiddleware(allRoutes)) } diff --git a/obp-api/src/main/scala/code/api/UKOpenBanking/v3_1_0/Http4sUKOBv310.scala b/obp-api/src/main/scala/code/api/UKOpenBanking/v3_1_0/Http4sUKOBv310.scala index 56a3c403b1..49c41d3573 100644 --- a/obp-api/src/main/scala/code/api/UKOpenBanking/v3_1_0/Http4sUKOBv310.scala +++ b/obp-api/src/main/scala/code/api/UKOpenBanking/v3_1_0/Http4sUKOBv310.scala @@ -4,6 +4,7 @@ import cats.data.{Kleisli, OptionT} import cats.effect._ import code.api.util.APIUtil.ResourceDoc import code.api.util.http4s.ResourceDocMiddleware +import code.api.util.http4s.IdempotencyMiddleware import code.util.Helper.MdcLoggable import com.openbankproject.commons.util.ApiVersion import org.http4s._ @@ -74,5 +75,5 @@ object Http4sUKOBv310 extends MdcLoggable { .orElse(Http4sUKOBv310InternationalStandingOrders.routes(req)) } - val wrappedRoutes: HttpRoutes[IO] = ResourceDocMiddleware.apply(resourceDocs)(allRoutes) + val wrappedRoutes: HttpRoutes[IO] = ResourceDocMiddleware.apply(resourceDocs)(IdempotencyMiddleware(allRoutes)) } diff --git a/obp-api/src/main/scala/code/api/UKOpenBanking/v4_0_1/Http4sUKOBv401.scala b/obp-api/src/main/scala/code/api/UKOpenBanking/v4_0_1/Http4sUKOBv401.scala index c40362d530..4f9985f712 100644 --- a/obp-api/src/main/scala/code/api/UKOpenBanking/v4_0_1/Http4sUKOBv401.scala +++ b/obp-api/src/main/scala/code/api/UKOpenBanking/v4_0_1/Http4sUKOBv401.scala @@ -5,6 +5,7 @@ import cats.effect._ import code.api.util.APIUtil import code.api.util.APIUtil.ResourceDoc import code.api.util.http4s.ResourceDocMiddleware +import code.api.util.http4s.IdempotencyMiddleware import code.util.Helper.MdcLoggable import com.openbankproject.commons.util.ApiVersion import org.http4s._ @@ -62,5 +63,5 @@ object Http4sUKOBv401 extends MdcLoggable { routes(req).map(_.putHeaders(Header.Raw(fapiInteractionIdHeader, interactionId))) } - val wrappedRoutes: HttpRoutes[IO] = withFapiInteractionId(ResourceDocMiddleware.apply(resourceDocs)(allRoutes)) + val wrappedRoutes: HttpRoutes[IO] = withFapiInteractionId(ResourceDocMiddleware.apply(resourceDocs)(IdempotencyMiddleware(allRoutes))) } diff --git a/obp-api/src/main/scala/code/api/berlin/group/v1_3/Http4sBGv13.scala b/obp-api/src/main/scala/code/api/berlin/group/v1_3/Http4sBGv13.scala index a2d83e2737..a50c74b563 100644 --- a/obp-api/src/main/scala/code/api/berlin/group/v1_3/Http4sBGv13.scala +++ b/obp-api/src/main/scala/code/api/berlin/group/v1_3/Http4sBGv13.scala @@ -5,6 +5,7 @@ import cats.effect._ import code.api.berlin.group.ConstantsBG import code.api.util.APIUtil.ResourceDoc import code.api.util.http4s.ResourceDocMiddleware +import code.api.util.http4s.IdempotencyMiddleware import code.util.Helper.MdcLoggable import org.http4s._ @@ -35,5 +36,5 @@ object Http4sBGv13 extends MdcLoggable { .orElse(Http4sBGv13SigningBaskets.routes(req)) } - val wrappedRoutes: HttpRoutes[IO] = ResourceDocMiddleware.apply(resourceDocs)(allRoutes) + val wrappedRoutes: HttpRoutes[IO] = ResourceDocMiddleware.apply(resourceDocs)(IdempotencyMiddleware(allRoutes)) } diff --git a/obp-api/src/main/scala/code/api/berlin/group/v2/Http4sBGv2.scala b/obp-api/src/main/scala/code/api/berlin/group/v2/Http4sBGv2.scala index 9e2d1ce640..f62ec51171 100644 --- a/obp-api/src/main/scala/code/api/berlin/group/v2/Http4sBGv2.scala +++ b/obp-api/src/main/scala/code/api/berlin/group/v2/Http4sBGv2.scala @@ -5,6 +5,7 @@ import cats.effect._ import code.api.berlin.group.ConstantsBG import code.api.util.APIUtil.ResourceDoc import code.api.util.http4s.ResourceDocMiddleware +import code.api.util.http4s.IdempotencyMiddleware import code.util.Helper.MdcLoggable import org.http4s._ @@ -27,5 +28,5 @@ object Http4sBGv2 extends MdcLoggable { .orElse(Http4sBGv2PIIS.routes(req)) } - val wrappedRoutes: HttpRoutes[IO] = ResourceDocMiddleware.apply(resourceDocs)(allRoutes) + val wrappedRoutes: HttpRoutes[IO] = ResourceDocMiddleware.apply(resourceDocs)(IdempotencyMiddleware(allRoutes)) } diff --git a/obp-api/src/main/scala/code/api/cache/Redis.scala b/obp-api/src/main/scala/code/api/cache/Redis.scala index 05208e3360..cf1bdb89b4 100644 --- a/obp-api/src/main/scala/code/api/cache/Redis.scala +++ b/obp-api/src/main/scala/code/api/cache/Redis.scala @@ -7,7 +7,7 @@ import code.util.Helper.MdcLoggable import com.openbankproject.commons.ExecutionContext.Implicits.global import redis.clients.jedis.{Jedis, JedisPool, JedisPoolConfig} import scalacache.memoization.{cacheKeyExclude, memoizeF, memoizeSync} -import scalacache.{Cache, Flags} +import scalacache.{Cache, CacheConfig, DefaultCacheKeyBuilder, Flags} import scalacache.redis.RedisCache import scalacache.serialization.{Codec, FailedToDecode} import redis.clients.jedis.{Jedis, JedisPool, JedisPoolConfig} @@ -321,6 +321,50 @@ object Redis extends MdcLoggable { // building one per call only put two allocations in front of every cache read on the request // path. RedisCache is a thin wrapper over the pool built above and opens nothing of its own, so // the pool, its authentication and its SSL configuration stay shared. + /** + * The serialization identity these cached bytes were produced under. + * + * Cache entries are Kryo-encoded, and what Kryo produces depends on the Scala library and the + * chill build that encoded it. Two OBP-API versions compiled against different ones therefore + * write mutually unreadable bytes into the same keys -- and "unreadable" is the optimistic + * case. Measured across the 2.12 -> 2.13 migration: an EMPTY `List`, written by chill 0.9.3, + * decodes under 0.9.5 into a `scala.collection.immutable.Queue`. That decode SUCCEEDS. It is + * only at the call site, whose signature says `List`, that it fails -- + * + * class scala.collection.immutable.Queue cannot be cast to + * class scala.collection.immutable.List + * + * -- so the caller gets a 500 rather than a cache miss, and gets it for the whole TTL, because + * a failed read does not evict the entry. Reproduced on `GET /management/dynamic-message-docs` + * and `GET /management/connector-methods`: 200 on 2.12, 500 on 2.13 reading 2.12's entry, and + * fine in either version on its own. That is a rolling upgrade, or any upgrade against a warm + * Redis. + * + * The migration note anticipated the risk and described the consequence as a cold cache. For + * values that fail to decode that is exactly right. This handles the ones that do not fail. + * + * Namespacing the key is the fix rather than casting defensively at each call site: there are + * eight `List`-returning memoized methods today, the same drift can hit any other type, and no + * amount of care at the call sites can make bytes already in Redis readable. Entries written by + * another version simply stop being addressable and age out on their own TTL. + * + * The Scala binary version is the axis that moved here and is the one derived automatically. + * `obp.cache.serialization.version` is for the case it does not cover -- a dependency upgrade + * that changes the encoding without changing the Scala version, which is what chill 0.9.3 to + * 0.9.5 would have been on its own. Bump it in that situation; the cost is one cold cache. + */ + private val serializationNamespace: String = { + val scalaBinary = scala.util.Properties.versionNumberString.split('.').take(2).mkString(".") + val manual = APIUtil.getPropsValue("obp.cache.serialization.version", "1") + s"obpser$manual-scala$scalaBinary" + } + + // Prefixing happens here, in the key builder, rather than at the call sites: scalacache derives + // the rest of the key from the enclosing method and its arguments, and every caller goes through + // it. `memoizeSync` and `memoizeF` both read this same implicit config. + implicit val cacheConfig: CacheConfig = + CacheConfig(cacheKeyBuilder = DefaultCacheKeyBuilder(keyPrefix = Some(serializationNamespace))) + private val sharedCache: Cache[Any] = RedisCache[Any](jedisPool) private def cacheFor[A]: Cache[A] = sharedCache.asInstanceOf[Cache[A]] diff --git a/obp-api/src/main/scala/code/api/sandbox/example_data/2016-04-28/example_import.json b/obp-api/src/main/scala/code/api/sandbox/example_data/2016-04-28/example_import.json index f6ee011d8f..96ed1ffe99 100644 --- a/obp-api/src/main/scala/code/api/sandbox/example_data/2016-04-28/example_import.json +++ b/obp-api/src/main/scala/code/api/sandbox/example_data/2016-04-28/example_import.json @@ -64,7 +64,7 @@ "currency":"GBP", "amount":"8084.32" }, - "IBAN":"BA12 1234 5123 4513 7599 6969 977", + "IBAN":"BA463990000000000001", "owners":["robert.xuk.x@example.com"], "generate_public_view":false, "generate_accountants_view":true, @@ -79,7 +79,7 @@ "currency":"GBP", "amount":"8084.32" }, - "IBAN":"BA12 1234 5123 4513 7599 6969 977", + "IBAN":"BA924990000000000001", "owners":["robert.yuk.y@example.com"], "generate_public_view":false, "generate_accountants_view":true, diff --git a/obp-api/src/main/scala/code/api/sandbox/example_data/example_import.json b/obp-api/src/main/scala/code/api/sandbox/example_data/example_import.json index 0126ddd054..85c2f9dcd1 100644 --- a/obp-api/src/main/scala/code/api/sandbox/example_data/example_import.json +++ b/obp-api/src/main/scala/code/api/sandbox/example_data/example_import.json @@ -64,7 +64,7 @@ "currency":"GBP", "amount":"6599.63" }, - "IBAN":"BA12 1234 5123 4518 4490 1189 877", + "IBAN":"BA511990000000000001", "owners":["Susan.X.0.GH"], "generate_public_view":true, "generate_accountants_view":true, @@ -79,7 +79,7 @@ "currency":"GBP", "amount":"6379.63" }, - "IBAN":"BA12 1234 5123 4511 8754 4625 177", + "IBAN":"BA241990000000000002", "owners":["Robert.X.0.GH"], "generate_public_view":true, "generate_accountants_view":true, @@ -94,7 +94,7 @@ "currency":"GBP", "amount":"7588.25" }, - "IBAN":"BA12 1234 5123 4510 4337 1399 677", + "IBAN":"BA941990000000000003", "owners":["Anil.X.0.GH"], "generate_public_view":true, "generate_accountants_view":true, @@ -109,7 +109,7 @@ "currency":"GBP", "amount":"6662.05" }, - "IBAN":"BA12 1234 5123 4514 4440 2184 977", + "IBAN":"BA671990000000000004", "owners":["Robert.X.0.GH"], "generate_public_view":true, "generate_accountants_view":true, @@ -124,7 +124,7 @@ "currency":"GBP", "amount":"3748.57" }, - "IBAN":"BA12 1234 5123 4518 9534 3427 277", + "IBAN":"BA401990000000000005", "owners":["Ellie.X.0.GH"], "generate_public_view":true, "generate_accountants_view":true, @@ -139,7 +139,7 @@ "currency":"GBP", "amount":"15860.50" }, - "IBAN":"BA12 1234 5123 4512 1957 2301 577", + "IBAN":"BA131990000000000006", "owners":["Anil.X.0.GH"], "generate_public_view":true, "generate_accountants_view":true, @@ -154,7 +154,7 @@ "currency":"GBP", "amount":"7724.41" }, - "IBAN":"BA12 1234 5123 4512 6914 8586 977", + "IBAN":"BA831990000000000007", "owners":["Anil.X.0.GH"], "generate_public_view":true, "generate_accountants_view":true, @@ -169,7 +169,7 @@ "currency":"GBP", "amount":"6599.63" }, - "IBAN":"BA12 1234 5123 4518 4490 1189 877", + "IBAN":"BA972990000000000001", "owners":["Susan.Y.9.GH"], "generate_public_view":true, "generate_accountants_view":true, @@ -184,7 +184,7 @@ "currency":"GBP", "amount":"6379.63" }, - "IBAN":"BA12 1234 5123 4511 8754 4625 177", + "IBAN":"BA702990000000000002", "owners":["Robert.Y.9.GH"], "generate_public_view":true, "generate_accountants_view":true, @@ -199,7 +199,7 @@ "currency":"GBP", "amount":"7588.25" }, - "IBAN":"BA12 1234 5123 4510 4337 1399 677", + "IBAN":"BA432990000000000003", "owners":["Anil.Y.9.GH"], "generate_public_view":true, "generate_accountants_view":true, @@ -214,7 +214,7 @@ "currency":"GBP", "amount":"6662.05" }, - "IBAN":"BA12 1234 5123 4514 4440 2184 977", + "IBAN":"BA162990000000000004", "owners":["Robert.Y.9.GH"], "generate_public_view":true, "generate_accountants_view":true, @@ -229,7 +229,7 @@ "currency":"GBP", "amount":"3748.57" }, - "IBAN":"BA12 1234 5123 4518 9534 3427 277", + "IBAN":"BA862990000000000005", "owners":["Ellie.Y.9.GH"], "generate_public_view":true, "generate_accountants_view":true, @@ -244,7 +244,7 @@ "currency":"GBP", "amount":"15860.50" }, - "IBAN":"BA12 1234 5123 4512 1957 2301 577", + "IBAN":"BA592990000000000006", "owners":["Anil.Y.9.GH"], "generate_public_view":true, "generate_accountants_view":true, @@ -259,7 +259,7 @@ "currency":"GBP", "amount":"7724.41" }, - "IBAN":"BA12 1234 5123 4512 6914 8586 977", + "IBAN":"BA322990000000000007", "owners":["Anil.Y.9.GH"], "generate_public_view":true, "generate_accountants_view":true, diff --git a/obp-api/src/main/scala/code/api/util/DynamicUtil.scala b/obp-api/src/main/scala/code/api/util/DynamicUtil.scala index 69ca8dea3c..862bf609c8 100644 --- a/obp-api/src/main/scala/code/api/util/DynamicUtil.scala +++ b/obp-api/src/main/scala/code/api/util/DynamicUtil.scala @@ -360,6 +360,26 @@ object DynamicUtil extends MdcLoggable{ object Validation { + /** + * Turn the `dynamic_code_compile_validate_dependencies` props value into the Scala source + * that, once compiled, yields the whitelist. + * + * A named function rather than an inline expression so a test can drive the real thing. + * DynamicUtilTest used to hold a character-for-character copy of it, which meant the two + * could diverge with the test still green -- the copy was only kept in step here because + * whoever edited one happened to see the other. This is the only compile that happens + * reflectively at boot, so nothing at compile time would have caught the divergence either. + * + * `Map[String, String](` rather than `Map(`: the props default is an empty list, and a bare + * `Map()` leaves its type parameters undetermined, so the trailing `.toMap` cannot prove the + * elements are pairs and the reflective compilation fails. The `.toMap` is itself needed + * because `mapValues` returns a view rather than a Map on 2.13. + */ + def dependenciesScalaCode(dependenciesString: String): String = + s"${DynamicUtil.importStatements}" + + dependenciesString.replaceFirst("\\[", "Map[String, String](").dropRight(1) + + ").mapValues(v => StringUtils.split(v, ',').map(_.trim).toSet).toMap" + val dynamicCodeSandboxPermissions = APIUtil.getPropsValue("dynamic_code_sandbox_permissions", "[]").trim val scalaCodePermissioins = "List[java.security.Permission]"+dynamicCodeSandboxPermissions.replaceFirst("\\[","(").dropRight(1)+")" val permissions:Box[List[java.security.Permission]] = DynamicUtil.compileScalaCodeUnchecked(scalaCodePermissioins) @@ -385,11 +405,7 @@ object DynamicUtil extends MdcLoggable{ val allowedRuntimePermissions = permissions.openOrThrowException("Can not compile the props `dynamic_code_sandbox_permissions` to permissions") val dependenciesString = APIUtil.getPropsValue("dynamic_code_compile_validate_dependencies", "[]").trim - // `Map[String, String](` rather than `Map(`: the props default is an empty list, and a bare - // `Map()` leaves its type parameters undetermined, so the trailing .toMap cannot prove the - // elements are pairs and the reflective compilation fails. The .toMap itself is needed because - // mapValues returns a view rather than a Map. - val scalaCodeDependencies = s"${DynamicUtil.importStatements}"+dependenciesString.replaceFirst("\\[","Map[String, String](").dropRight(1) +").mapValues(v => StringUtils.split(v, ',').map(_.trim).toSet).toMap" + val scalaCodeDependencies = dependenciesScalaCode(dependenciesString) val dependenciesBox: Box[Map[String, Set[String]]] = DynamicUtil.compileScalaCodeUnchecked(scalaCodeDependencies) /** diff --git a/obp-api/src/main/scala/code/api/util/ErrorMessages.scala b/obp-api/src/main/scala/code/api/util/ErrorMessages.scala index 33040e83d4..f35a35fe31 100644 --- a/obp-api/src/main/scala/code/api/util/ErrorMessages.scala +++ b/obp-api/src/main/scala/code/api/util/ErrorMessages.scala @@ -828,6 +828,7 @@ object ErrorMessages { val ChatMessageTypeNotAllowed = "OBP-39018: Invalid message_type. Allowed values: text, system." val SignalMessageTooLong = "OBP-39019: Signal message exceeds the maximum allowed length." val SignalMessageContainsDangerousCharacters = "OBP-39020: Signal message contains control or bidirectional-override characters, which are not allowed." + val SignalChannelNotFound = "OBP-39021: Signal Channel not found." // Transaction Request related messages (OBP-40XXX) val InvalidTransactionRequestType = "OBP-40001: Invalid value for TRANSACTION_REQUEST_TYPE" 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 8145f828b9..ba7209efd6 100644 --- a/obp-api/src/main/scala/code/api/util/NewStyle.scala +++ b/obp-api/src/main/scala/code/api/util/NewStyle.scala @@ -3341,7 +3341,9 @@ object NewStyle extends MdcLoggable{ def createOrUpdateEndpointMapping(bankId: Option[String], endpointMapping: EndpointMappingT, callContext: Option[CallContext]) = { validateBankId(bankId, callContext) Future { - (EndpointMappingProvider.endpointMappingProvider.vend.createOrUpdate(bankId, endpointMapping), callContext) + val result = EndpointMappingProvider.endpointMappingProvider.vend.createOrUpdate(bankId, endpointMapping) + invalidateEndpointMappingCache() + (result, callContext) } map { i => (connectorEmptyResponse(i._1, callContext), i._2) } @@ -3350,12 +3352,48 @@ object NewStyle extends MdcLoggable{ def deleteEndpointMapping(bankId: Option[String], endpointMappingId: String, callContext: Option[CallContext]) = { validateBankId(bankId, callContext) Future { - (EndpointMappingProvider.endpointMappingProvider.vend.delete(bankId, endpointMappingId), callContext) + val result = EndpointMappingProvider.endpointMappingProvider.vend.delete(bankId, endpointMappingId) + invalidateEndpointMappingCache() + (result, callContext) } map { i => (connectorEmptyResponse(i._1, callContext), i._2) } } + /** + * Drop every memoized `getEndpointMappings(...)` entry after a mapping is created, + * updated, or deleted, so the change takes effect on the next request instead of waiting + * out `endpointMapping.cache.ttl.seconds`. Mirrors invalidateMethodRoutingCache: the + * memoize key embeds the literal method name, so one pattern delete clears every bankId + * variant. The pattern is a prefix of the actual name the macro renders + * (`getEndpointMappingsCached`), which is what makes one wildcard cover it. No-op / logged + * when Redis is unavailable (deleteKeysByPattern swallows and returns 0). + * + * This became necessary with the cache key fix below. While callContext was part of the + * key nothing could ever hit, so a stale entry was unreachable by construction; now that + * the cache works, writes have to publish themselves. + * + * A single delete here is not quite enough: a reader that fetched the pre-write value from + * the provider a moment earlier can still complete its own cache write AFTER this delete + * finishes, silently reintroducing the stale entry for the rest of the TTL -- nothing else + * would clear it until the next write. Scheduling a second delete closes that window the + * conventional way: any straggler write that lands in the gap gets cleared shortly after, + * long before an operator or caller would reasonably treat it as current. + */ + private[util] def invalidateEndpointMappingCache(): Unit = { + Redis.deleteKeysByPattern("*getEndpointMappings*") + code.actorsystem.ObpActorSystem.localActorSystem.scheduler.scheduleOnce( + endpointMappingCacheInvalidationDelay + )(Redis.deleteKeysByPattern("*getEndpointMappings*"))( + code.actorsystem.ObpActorSystem.localActorSystem.dispatcher + ) + () + } + + private[util] val endpointMappingCacheInvalidationDelay: scala.concurrent.duration.FiniteDuration = + scala.concurrent.duration.FiniteDuration( + APIUtil.getPropsAsIntValue("endpointMapping.cache.invalidation.delay.ms", 500), "ms") + def getEndpointMappingById(bankId: Option[String], endpointMappingId : String, callContext: Option[CallContext]): OBPReturnType[EndpointMappingT] = { validateBankId(bankId, callContext) @@ -3378,18 +3416,41 @@ object NewStyle extends MdcLoggable{ private[this] val endpointMappingTTL = APIUtil.getPropsValue(s"endpointMapping.cache.ttl.seconds", "0").toInt - def getEndpointMappings(bankId: Option[String], callContext: Option[CallContext]): OBPReturnType[List[EndpointMappingT]] = Future{ + /** + * The memoized half of getEndpointMappings, split into its own method for two reasons. + * + * Neither the key nor the cached value may mention the callContext. The key, because + * CacheKeyFromArguments renders every un-annotated parameter and CallContext carries + * per-request state (startTime, correlationId, url, verb, ipAddress, user) - keying on it + * made the key unique per request, so the cache could never hit. The value, because a hit + * would hand the caller the originating request's CallContext, and because chill/Kryo + * cannot encode the lambda reachable through CallContext.resourceDocument: every write of + * the old (mappings, callContext) tuple failed and cachePut swallowed it as "result served + * uncached", so endpointMapping.cache.ttl.seconds bought nothing but a WARN per call. + * + * A parameter-less signature rather than `@CacheKeyOmit callContext` on the caller, because + * CacheKeyFromArguments reads the parameters of the method whose body ENDS in buildCacheKey. + * Binding the result to a val first (`val x = buildCacheKey {...}; (x, callContext)`) leaves + * the macro with no parameters to render and it emits `Nil.mkString("_")` - an empty + * argument segment, i.e. every bankId sharing one entry. Keep buildCacheKey as the tail + * expression here; `NewStyle.function.getEndpointMappings` is verified by javap to render + * `bankId :: Nil`. + */ + private def getEndpointMappingsCached(bankId: Option[String]): List[EndpointMappingT] = { import scala.concurrent.duration._ - validateBankId(bankId, callContext) - var cacheKey = (randomUUID().toString, randomUUID().toString, randomUUID().toString) CacheKeyFromArguments.buildCacheKey { Caching.memoizeSyncWithProvider(Some(cacheKey.toString()))(endpointMappingTTL.second) { - {(EndpointMappingProvider.endpointMappingProvider.vend.getAllEndpointMappings(bankId), callContext)} + EndpointMappingProvider.endpointMappingProvider.vend.getAllEndpointMappings(bankId) } } } + + def getEndpointMappings(bankId: Option[String], callContext: Option[CallContext]): OBPReturnType[List[EndpointMappingT]] = Future{ + validateBankId(bankId, callContext) + (getEndpointMappingsCached(bankId), callContext) + } /** * Invalidate the Redis-backed resource-doc caches whose contents include diff --git a/obp-api/src/main/scala/code/api/util/http4s/IdempotencyMiddleware.scala b/obp-api/src/main/scala/code/api/util/http4s/IdempotencyMiddleware.scala index f9e9c5032d..d3d0429de2 100644 --- a/obp-api/src/main/scala/code/api/util/http4s/IdempotencyMiddleware.scala +++ b/obp-api/src/main/scala/code/api/util/http4s/IdempotencyMiddleware.scala @@ -31,11 +31,14 @@ import java.util.Base64 * - Concurrent replay while the original is still in flight: 409 Conflict. * - 5xx responses are NOT cached; clients can retry. * - * Scope: the key is namespaced by SHA-256 of the consumer id, or — when - * unauthenticated — the Authorization header. This prevents key reuse across - * consumers. + * Scope: the key is namespaced by SHA-256 of (consumer id, or — when + * unauthenticated — the Authorization header) AND the resolved operation id. This prevents key + * reuse across consumers AND across endpoints: a client accidentally or deliberately reusing one + * Idempotency-Key against two different operations gets two independent dedup slots instead of + * the second operation being treated as a replay of the first and never executed. * - * Validation: 8..255 printable-ASCII characters. Anything else → 400. + * Validation: 8..255 printable-ASCII characters. Anything else → 400, regardless of whether this + * tier serves the path -- a malformed header is a client error no matter where it resolves. * * Storage: Redis via the existing JedisPool. Two keys per request: * - idem:lock:: → "1" (60s TTL, set with NX) @@ -44,9 +47,32 @@ import java.util.Base64 * Resilience: any Redis error is logged and the request is allowed to proceed * unchanged — the middleware never blocks traffic on cache outages. * - * The middleware should be installed INSIDE ResourceDocMiddleware so the - * CallContext (and therefore the consumer id) is populated before the scope - * key is computed. + * ── Where it may be installed ── + * + * INSIDE ResourceDocMiddleware, on every version's route tree. Both halves matter. + * + * Inside, because the scope key and the request-body hash both come from the CallContext, and + * ResourceDocMiddleware is what populates it. Mounted outside, there is no CallContext at all -- + * no operationId and no httpBody -- so `matchedThisTier` is false for every request and this + * middleware does nothing at all rather than mishandling anything: no lock, no cached response, + * no conflict detection. That is a silent loss of protection, not a silent corruption, but it is + * still a config-only guard against a real risk: on a payment endpoint, dedup quietly not running + * is the difference between "your retry was deduplicated" and "your retry submitted the payment + * again". + * + * On every tree, because Http4sApp composes the versions with `.orElse` and a tree signals "not + * mine" by returning OptionT.none. This middleware therefore has to pass a miss through + * unchanged; an earlier version answered 404 in that case, which terminated the chain -- measured + * on POST /obp/v3.1.0/management/method_routings, which answered 201 without an Idempotency-Key + * and 404 with one. A later version preserved the miss but still spent a full lock-acquire/release + * cycle doing it, using a method+path fallback scope for any tier that had not matched -- which + * meant two requests racing that SAME miss-tier's fallback lock (two genuinely different + * endpoints whose method+path fallback happened to differ less often than intended, or two + * concurrent copies of the request destined for a LATER tier) could have the loser answered a + * definite 409 by a tier that was never going to serve either of them, before the request ever + * reached the tier that would have handled it correctly. `matchedThisTier` closes that: only the + * one tier whose ResourceDocMatcher actually matched (the only tier where operationId is ever + * set) does any lock/response-key work at all. IdempotencyMiddlewareTest pins all of these. */ object IdempotencyMiddleware extends MdcLoggable { @@ -81,14 +107,41 @@ object IdempotencyMiddleware extends MdcLoggable { } else { val key = keyOpt.get if (!isValidKey(key)) { + // A malformed header is a client error whatever the path resolves to, so this + // one deliberately does NOT fall through. OptionT.liftF(invalidKeyResponse(key)) + } else if (!matchedThisTier(req)) { + // operationId is set on the CallContext ONLY by ResourceDocMiddleware.attachToCallContext, + // and only for the one version tree whose ResourceDocMatcher actually matched this + // request (see ResourceDocMiddleware.apply: the `case None` branch never attaches one). + // Every other tree in the `.orElse` chain is about to answer a miss regardless of what + // this middleware does, so doing lock/response-key work there is worse than wasted: two + // requests that legitimately belong to two DIFFERENT trees, or two genuinely concurrent + // copies of the same request, can race the SAME miss-tier's method+path-fallback lock + // and the loser gets a definite 409 here -- terminating the `.orElse` chain on behalf of + // a tree that was never going to serve either of them, before the request ever reaches + // the tree that would have handled it correctly. Skipping straight through at a + // miss-tier removes both the wasted Redis round trips and this false-conflict window; + // only the matching tier ever computes a scope key that means anything durable, so only + // it needs the protection. + routes.run(req) } else { val scope = scopeFor(req) val bodyHash = sha256Hex(bodyFromCallContextOrEmpty(req)) val responseKey = ResponseKeyPrefix + scope + ":" + key val lockKey = LockKeyPrefix + scope + ":" + key - OptionT.liftF(handle(req, routes, responseKey, lockKey, bodyHash)) + // OptionT, not OptionT.liftF: a route MISS has to stay a miss. + // + // Every version's routes are one link in a fallthrough chain -- Http4sApp composes them + // with `.orElse`, and `OptionT.none` is how a tree says "not mine, try the next one". + // Wrapping with liftF made this middleware answer 404 on behalf of a tree that simply + // did not serve the path, which terminated the chain: measured on + // `POST /obp/v3.1.0/management/method_routings`, the request answered 201 without an + // Idempotency-Key and 404 with one, because the first tree it passed through swallowed + // the miss. So the middleware could only ever be installed on the last link. Preserving + // the miss is what makes it safe to install on all of them. + OptionT(handle(req, routes, responseKey, lockKey, bodyHash)) } } } @@ -99,16 +152,16 @@ object IdempotencyMiddleware extends MdcLoggable { responseKey: String, lockKey: String, requestBodyHash: String - ): IO[Response[IO]] = { + ): IO[Option[Response[IO]]] = { IO.blocking(readResponseKey(responseKey)).attempt.flatMap { case Right(Some(envelope)) => if (envelope.requestBodyHash == requestBodyHash) { - IO.pure(rebuildResponse(envelope, replay = true)) + IO.pure(Some(rebuildResponse(envelope, replay = true))) } else { conflictResponse( "Idempotency-Key replayed with a different request body. " + "Use a fresh key for a different request." - ) + ).map(Some(_)) } case Right(None) => @@ -118,7 +171,7 @@ object IdempotencyMiddleware extends MdcLoggable { case Right(false) => conflictResponse( "Idempotent operation already in flight for this Idempotency-Key." - ) + ).map(Some(_)) case Left(t) => logger.warn(s"Idempotency lock unavailable (Redis): ${t.getMessage}") runRoutes(req, routes) @@ -136,41 +189,48 @@ object IdempotencyMiddleware extends MdcLoggable { responseKey: String, lockKey: String, requestBodyHash: String - ): IO[Response[IO]] = { - runRoutes(req, routes).flatMap { resp => - // Drain body so we can both cache and re-emit it. - resp.body.compile.toVector.flatMap { vec => - val bodyBytes = vec.toArray - val rebuilt = resp.withBodyStream(fs2.Stream.emits(bodyBytes).covary[IO]) - - val storeOrReleaseLock: IO[Unit] = - if (resp.status.code >= 500) { - // Don't cache transient failures; release the lock so client can retry. - IO.blocking(deleteKey(lockKey)).attempt.map(_ => ()) - } else { - val envelope = Envelope( - status = resp.status.code, - contentType = resp.headers.get(CIString("Content-Type")).map(_.head.value), - bodyB64 = Base64.getEncoder.encodeToString(bodyBytes), - requestBodyHash = requestBodyHash - ) - IO.blocking { - writeResponseKey(responseKey, envelope) - deleteKey(lockKey) - }.attempt.map { e => - e.left.foreach(t => - logger.warn(s"Failed to cache idempotent response: ${t.getMessage}") + ): IO[Option[Response[IO]]] = { + runRoutes(req, routes).flatMap { + // The lock was taken before the routes ran, so a miss has to give it back -- otherwise a + // path this tree does not serve would hold the key locked for its full 60s TTL and a + // genuine request carrying that key would be refused with 409. + case None => IO.blocking(deleteKey(lockKey)).attempt.as(None) + case Some(resp) => + // Drain body so we can both cache and re-emit it. + resp.body.compile.toVector.flatMap { vec => + val bodyBytes = vec.toArray + val rebuilt = resp.withBodyStream(fs2.Stream.emits(bodyBytes).covary[IO]) + + val storeOrReleaseLock: IO[Unit] = + if (resp.status.code >= 500) { + // Don't cache transient failures; release the lock so client can retry. + IO.blocking(deleteKey(lockKey)).attempt.map(_ => ()) + } else { + val envelope = Envelope( + status = resp.status.code, + contentType = resp.headers.get(CIString("Content-Type")).map(_.head.value), + bodyB64 = Base64.getEncoder.encodeToString(bodyBytes), + requestBodyHash = requestBodyHash ) - () + IO.blocking { + writeResponseKey(responseKey, envelope) + deleteKey(lockKey) + }.attempt.map { e => + e.left.foreach(t => + logger.warn(s"Failed to cache idempotent response: ${t.getMessage}") + ) + () + } } - } - storeOrReleaseLock.as(rebuilt) - } + storeOrReleaseLock.as(Some(rebuilt)) + } } } - private def runRoutes(req: Request[IO], routes: HttpRoutes[IO]): IO[Response[IO]] = - routes.run(req).getOrElseF(IO.pure(Response[IO](Status.NotFound))) + // `.value`, not `getOrElseF(404)` -- see the comment on the OptionT in `apply`. Converting a + // miss into a 404 here is what terminated the version fallthrough chain. + private def runRoutes(req: Request[IO], routes: HttpRoutes[IO]): IO[Option[Response[IO]]] = + routes.run(req).value // ── Validation ───────────────────────────────────────────────────────── @@ -179,16 +239,34 @@ object IdempotencyMiddleware extends MdcLoggable { key.length <= MaxKeyLength && key.forall(c => c >= 0x21 && c <= 0x7E) + // True only for the one version tree whose ResourceDocMatcher matched this request -- + // ResourceDocMiddleware.attachToCallContext is the sole place operationId is ever set, and it + // runs only on a match (see ResourceDocMiddleware.apply's `case Some(resourceDoc)` branch; the + // `case None` branch attaches a CallContext with no operationId). Every other tree in the + // `.orElse` chain sees operationId absent here and skips idempotency handling entirely, because + // it is about to answer a miss regardless of what this middleware does. + private def matchedThisTier(req: Request[IO]): Boolean = + req.attributes.lookup(Http4sRequestAttributes.callContextKey).exists(_.operationId.isDefined) + // ── Scope ────────────────────────────────────────────────────────────── private def scopeFor(req: Request[IO]): String = { val ccOpt = req.attributes.lookup(Http4sRequestAttributes.callContextKey) - val raw = ccOpt + val consumerOrAuth = ccOpt .flatMap(_.consumer.map(_.consumerId.get).toOption) .filter(_.nonEmpty) .orElse(req.headers.get(AuthorizationHeader).map(_.head.value)) .getOrElse("anonymous") - sha256Hex(raw).take(16) + // operationId is the canonical identity of "which endpoint" -- set by ResourceDocMiddleware + // once it has matched a ResourceDoc, and stable across path-template placeholders and + // bridge-cascade path rewrites (v400->v310->...). scopeFor only ever runs after + // matchedThisTier has confirmed operationId is present, so the method+path fallback below is + // purely defensive -- it should never actually be exercised in production, only under a + // future bug that calls scopeFor without that guard. + val endpoint = ccOpt + .flatMap(_.operationId) + .getOrElse(s"${req.method.name} ${req.uri.path.renderString}") + sha256Hex(s"$consumerOrAuth|$endpoint").take(16) } // ── Body hash ────────────────────────────────────────────────────────── diff --git a/obp-api/src/main/scala/code/api/v1_2_1/Http4s121.scala b/obp-api/src/main/scala/code/api/v1_2_1/Http4s121.scala index 3d7ceb4127..891231fd92 100644 --- a/obp-api/src/main/scala/code/api/v1_2_1/Http4s121.scala +++ b/obp-api/src/main/scala/code/api/v1_2_1/Http4s121.scala @@ -10,6 +10,7 @@ import code.api.util.ApiTag._ import code.api.util.ErrorMessages._ import code.api.util.http4s.Http4sRequestAttributes.{EndpointHelpers, RequestOps, callContextKey} import code.api.util.http4s.Http4sCallContextBuilder +import code.api.util.http4s.IdempotencyMiddleware import code.api.util.http4s.ResourceDocMiddleware import code.api.util.newstyle.ViewNewStyle import code.api.util.{CallContext, CustomJsonFormats, NewStyle} @@ -2669,7 +2670,7 @@ object Http4s121 { } val allRoutesWithMiddleware: HttpRoutes[IO] = { - val middlewareWrapped = ResourceDocMiddleware.apply(resourceDocs)(allRoutes) + val middlewareWrapped = ResourceDocMiddleware.apply(resourceDocs)(IdempotencyMiddleware(allRoutes)) // bankById runs before middleware so it can return 400 (not 404) for unknown bank Kleisli[HttpF, Request[IO], Response[IO]] { req => bankById.run(req).orElse(middlewareWrapped.run(req)) diff --git a/obp-api/src/main/scala/code/api/v1_3_0/Http4s130.scala b/obp-api/src/main/scala/code/api/v1_3_0/Http4s130.scala index c3fe407a9e..27d0ac3d82 100644 --- a/obp-api/src/main/scala/code/api/v1_3_0/Http4s130.scala +++ b/obp-api/src/main/scala/code/api/v1_3_0/Http4s130.scala @@ -9,6 +9,7 @@ import code.api.util.ApiTag._ import code.api.util.ErrorMessages._ import code.api.util.http4s.Http4sRequestAttributes.{EndpointHelpers, RequestOps} import code.api.util.http4s.ResourceDocMiddleware +import code.api.util.http4s.IdempotencyMiddleware import code.api.util.NewStyle import code.api.v1_2_1.JSONFactory import com.github.dwickern.macros.NameOf.nameOf @@ -126,7 +127,7 @@ object Http4s130 { .orElse(getCardsForBank.run(req)) } - val allRoutesWithMiddleware: HttpRoutes[IO] = ResourceDocMiddleware.apply(resourceDocs)(allOwnRoutes) + val allRoutesWithMiddleware: HttpRoutes[IO] = ResourceDocMiddleware.apply(resourceDocs)(IdempotencyMiddleware(allOwnRoutes)) // ─── path-rewriting bridge: /obp/v1.3.0/… → /obp/v1.2.1/… ───────────── // Delegates to Http4s121 so all inherited v1.2.1 endpoints are served diff --git a/obp-api/src/main/scala/code/api/v1_4_0/Http4s140.scala b/obp-api/src/main/scala/code/api/v1_4_0/Http4s140.scala index 1f5c17a58a..d9c7a2404d 100644 --- a/obp-api/src/main/scala/code/api/v1_4_0/Http4s140.scala +++ b/obp-api/src/main/scala/code/api/v1_4_0/Http4s140.scala @@ -10,6 +10,7 @@ import code.api.util.ApiTag._ import code.api.util.ErrorMessages._ import code.api.util.http4s.Http4sRequestAttributes.{EndpointHelpers, RequestOps} import code.api.util.http4s.ResourceDocMiddleware +import code.api.util.http4s.IdempotencyMiddleware import code.api.util.{APIUtil, NewStyle} import code.api.v1_2_1.{JSONFactory, SuccessMessage} import code.atms.Atms @@ -475,7 +476,7 @@ object Http4s140 { .orElse(addCustomer.run(req)) } - val allRoutesWithMiddleware: HttpRoutes[IO] = ResourceDocMiddleware.apply(resourceDocs)(allOwnRoutes) + val allRoutesWithMiddleware: HttpRoutes[IO] = ResourceDocMiddleware.apply(resourceDocs)(IdempotencyMiddleware(allOwnRoutes)) // ─── path-rewriting bridge: /obp/v1.4.0/… → /obp/v1.3.0/… ────────────── // Delegates to Http4s130 so all inherited v1.3.0 and v1.2.1 endpoints are diff --git a/obp-api/src/main/scala/code/api/v2_0_0/Http4s200.scala b/obp-api/src/main/scala/code/api/v2_0_0/Http4s200.scala index 38b4915de1..bd65de6109 100644 --- a/obp-api/src/main/scala/code/api/v2_0_0/Http4s200.scala +++ b/obp-api/src/main/scala/code/api/v2_0_0/Http4s200.scala @@ -13,6 +13,7 @@ import code.api.util.ApiTag._ import code.api.util.ErrorMessages._ import code.api.util.http4s.Http4sRequestAttributes.{EndpointHelpers, RequestOps} import code.api.util.http4s.ResourceDocMiddleware +import code.api.util.http4s.IdempotencyMiddleware import code.api.util.newstyle.ViewNewStyle import code.api.util.{APIUtil, ApiRole, CustomJsonFormats, NewStyle} import code.api.v1_2_1.{JSONFactory => JSONFactory121, SuccessMessage} @@ -1586,7 +1587,7 @@ object Http4s200 { .orElse(elasticSearchMetrics.run(req)) } - val allRoutesWithMiddleware: HttpRoutes[IO] = ResourceDocMiddleware.apply(resourceDocs)(allOwnRoutes) + val allRoutesWithMiddleware: HttpRoutes[IO] = ResourceDocMiddleware.apply(resourceDocs)(IdempotencyMiddleware(allOwnRoutes)) // ─── path-rewriting bridge: /obp/v2.0.0/… → /obp/v1.4.0/… ────────────── // Delegates to Http4s140 so all inherited v1.4.0/v1.3.0/v1.2.1 endpoints are diff --git a/obp-api/src/main/scala/code/api/v2_1_0/Http4s210.scala b/obp-api/src/main/scala/code/api/v2_1_0/Http4s210.scala index 4081ac34b4..cd31ae8891 100644 --- a/obp-api/src/main/scala/code/api/v2_1_0/Http4s210.scala +++ b/obp-api/src/main/scala/code/api/v2_1_0/Http4s210.scala @@ -12,6 +12,7 @@ import code.api.util.ApiTag._ import code.api.util.ErrorMessages._ import code.api.util.http4s.Http4sRequestAttributes.{EndpointHelpers, RequestOps} import code.api.util.http4s.ResourceDocMiddleware +import code.api.util.http4s.IdempotencyMiddleware import code.api.util.newstyle.ViewNewStyle import code.api.util.{APIUtil, ApiRole, CallContext, CustomJsonFormats, NewStyle} import code.api.v1_2_1.{JSONFactory => JSONFactory121, SuccessMessage} @@ -1393,7 +1394,7 @@ object Http4s210 { .orElse(getMetrics.run(req)) } - val allRoutesWithMiddleware: HttpRoutes[IO] = ResourceDocMiddleware.apply(resourceDocs)(allOwnRoutes) + val allRoutesWithMiddleware: HttpRoutes[IO] = ResourceDocMiddleware.apply(resourceDocs)(IdempotencyMiddleware(allOwnRoutes)) // ─── path-rewriting bridge: /obp/v2.1.0/… → /obp/v2.0.0/… ────────────── // Delegates to Http4s200 so all inherited v2.0.0/v1.4.0/v1.3.0/v1.2.1 endpoints diff --git a/obp-api/src/main/scala/code/api/v2_2_0/Http4s220.scala b/obp-api/src/main/scala/code/api/v2_2_0/Http4s220.scala index f62b53c99a..fd0cb4dab4 100644 --- a/obp-api/src/main/scala/code/api/v2_2_0/Http4s220.scala +++ b/obp-api/src/main/scala/code/api/v2_2_0/Http4s220.scala @@ -13,6 +13,7 @@ import code.api.util.Glossary import code.api.util.http4s.Http4sRequestAttributes.{EndpointHelpers, RequestOps} import java.util.Date import code.api.util.http4s.ResourceDocMiddleware +import code.api.util.http4s.IdempotencyMiddleware import code.api.util.newstyle.ViewNewStyle import code.api.util.{APIUtil, CallContext, CustomJsonFormats, NewStyle} import code.api.v1_2_1.{CreateViewJsonV121, JSONFactory => JSONFactory121, UpdateViewJsonV121} @@ -1064,7 +1065,7 @@ object Http4s220 { .orElse(createCounterparty.run(req)) } - val allRoutesWithMiddleware: HttpRoutes[IO] = ResourceDocMiddleware.apply(resourceDocs)(allOwnRoutes) + val allRoutesWithMiddleware: HttpRoutes[IO] = ResourceDocMiddleware.apply(resourceDocs)(IdempotencyMiddleware(allOwnRoutes)) // ─── path-rewriting bridge: /obp/v2.2.0/… → /obp/v2.1.0/… ────────────── diff --git a/obp-api/src/main/scala/code/api/v3_0_0/Http4s300.scala b/obp-api/src/main/scala/code/api/v3_0_0/Http4s300.scala index 4641e6ab44..b5d6745188 100644 --- a/obp-api/src/main/scala/code/api/v3_0_0/Http4s300.scala +++ b/obp-api/src/main/scala/code/api/v3_0_0/Http4s300.scala @@ -15,6 +15,7 @@ import code.api.util.ApiTag._ import code.api.util.ErrorMessages._ import code.api.util.http4s.Http4sRequestAttributes.{EndpointHelpers, RequestOps} import code.api.util.http4s.ResourceDocMiddleware +import code.api.util.http4s.IdempotencyMiddleware import code.api.util.newstyle.ViewNewStyle import code.api.util.{APIUtil, CallContext, CustomJsonFormats, NewStyle} import code.api.v1_2_1.JSONFactory @@ -2303,7 +2304,7 @@ object Http4s300 { .orElse(bankById.run(req)) } - val allRoutesWithMiddleware: HttpRoutes[IO] = ResourceDocMiddleware.apply(resourceDocs)(allOwnRoutes) + val allRoutesWithMiddleware: HttpRoutes[IO] = ResourceDocMiddleware.apply(resourceDocs)(IdempotencyMiddleware(allOwnRoutes)) // ─── path-rewriting bridge: /obp/v3.0.0/… → /obp/v2.2.0/… ────────────── diff --git a/obp-api/src/main/scala/code/api/v3_1_0/Http4s310.scala b/obp-api/src/main/scala/code/api/v3_1_0/Http4s310.scala index 168094cbc3..fb97c8f4e3 100644 --- a/obp-api/src/main/scala/code/api/v3_1_0/Http4s310.scala +++ b/obp-api/src/main/scala/code/api/v3_1_0/Http4s310.scala @@ -17,6 +17,7 @@ import code.api.util.CertificateUtil import code.api.util.{ApiTrigger, Consent, Glossary, SecureRandomUtil} import code.api.util.http4s.Http4sRequestAttributes.{EndpointHelpers, RequestOps} import code.api.util.http4s.ResourceDocMiddleware +import code.api.util.http4s.IdempotencyMiddleware import code.api.util.newstyle.{BalanceNewStyle, ViewNewStyle} import code.api.util.{APIUtil, CallContext, CustomJsonFormats, NewStyle, OBPBankId, RateLimitingUtil} import code.api.v1_2_1.{JSONFactory, RateLimiting} @@ -5118,7 +5119,7 @@ object Http4s310 { .orElse(getObpConnectorLoopback.run(req)) } - val allRoutesWithMiddleware: HttpRoutes[IO] = ResourceDocMiddleware.apply(resourceDocs)(allOwnRoutes) + val allRoutesWithMiddleware: HttpRoutes[IO] = ResourceDocMiddleware.apply(resourceDocs)(IdempotencyMiddleware(allOwnRoutes)) // ─── path-rewriting bridge: /obp/v3.1.0/… → /obp/v3.0.0/… ────────────── diff --git a/obp-api/src/main/scala/code/api/v4_0_0/Http4s400.scala b/obp-api/src/main/scala/code/api/v4_0_0/Http4s400.scala index d821649e0d..48bbfa676f 100644 --- a/obp-api/src/main/scala/code/api/v4_0_0/Http4s400.scala +++ b/obp-api/src/main/scala/code/api/v4_0_0/Http4s400.scala @@ -35,6 +35,7 @@ import code.api.v1_4_0.JSONFactory1_4_0 import code.DynamicEndpoint.DynamicEndpointSwagger import code.api.util.http4s.Http4sRequestAttributes.{EndpointHelpers, RequestOps} import code.api.util.http4s.ResourceDocMiddleware +import code.api.util.http4s.IdempotencyMiddleware import code.api.util.{APIUtil, CallContext, CustomJsonFormats, NewStyle} import code.api.v4_0_0.JSONFactory400._ import code.DynamicData.DynamicData @@ -3018,6 +3019,33 @@ object Http4s400 { // first synchronous read of `SS.user` captures the cc.user, then the Future chain // runs normally on any thread. + // Resolves the view createTransactionRequest needs, unit-testable without a live Mapper + // connection: `lookup` is production's real `Views.views.vend.systemView(...).or(...)` call + // in the route below, and a stub in the test. + // + // `lookup()` runs OUTSIDE tryons's blanket exception catch, not inside it. tryons/tryo catch + // any Exception the wrapped block raises and report it via the given failCode regardless of + // cause -- wrapping the DB call itself made a connection-pool exhaustion, a transient SQL + // error, or a Mapper bug indistinguishable from a genuine "no such view" and reported ALL of + // them as 404. `Future(lookup())` still catches an exception from `lookup()` (standard + // Future-block semantics), but as an ordinary failed Future carrying the ORIGINAL exception, + // untouched -- so it falls through to ErrorResponseConverter's catch-all (500), the same as + // any other unexpected server-side failure. Only a lookup that SUCCEEDS and returns an empty + // Box is a genuine client-side "not found", and only that case is explicitly mapped to 404 + // via tryons below (whose wrapped block cannot itself throw for any other reason -- it only + // ever raises the NoSuchElementException it constructs). + private[v4_0_0] def resolveCreateTransactionRequestView( + viewIdStr: String, + lookup: () => Box[View] + )(implicit cc: CallContext): Future[View] = + Future(lookup()).flatMap { + case Full(v) => Future.successful(v) + case _ => + NewStyle.function.tryons(s"$ViewNotFound Current view_id($viewIdStr)", 404, Some(cc)) { + throw new NoSuchElementException(s"view_id($viewIdStr)") + } + } + lazy val createTransactionRequest: HttpRoutes[IO] = HttpRoutes.of[IO] { // GRANT_VIEW_ID in the ResourceDoc URL → middleware skips view validation. // Lift's v4 endpoint does no view-access check upfront; it lets @@ -3041,24 +3069,29 @@ object Http4s400 { EndpointHelpers.executeFutureCreated(req) { val bodyStr = cc.httpBody.getOrElse("") for { - user <- Future { cc.user.openOrThrowException(AuthenticatedUserIsRequired) } - bank <- Future { cc.bank.getOrElse(throw new RuntimeException(BankNotFound)) } - account <- Future { cc.bankAccount.getOrElse(throw new RuntimeException(BankAccountNotFound)) } + // These four used to throw raw exceptions, which the converter can only render as + // OBP-50000 / HTTP 500. Every one of them is a client-side condition -- not + // authenticated, no such bank, no such account, no such view -- and a 500 tells a + // caller with retry logic to keep sending a request that cannot succeed. This is a + // payment path, so that retry loop is the expensive kind. + user <- NewStyle.function.tryons(AuthenticatedUserIsRequired, 401, Some(cc)) { + cc.user.openOrThrowException(AuthenticatedUserIsRequired) + } + bank <- NewStyle.function.tryons(BankNotFound, 404, Some(cc)) { + cc.bank.getOrElse(throw new NoSuchElementException(bankIdStr)) + } + account <- NewStyle.function.tryons(BankAccountNotFound, 404, Some(cc)) { + cc.bankAccount.getOrElse(throw new NoSuchElementException(accountIdStr)) + } json <- NewStyle.function.tryons( s"$InvalidJsonFormat Empty or invalid request body.", 400, Some(cc)) { com.openbankproject.commons.util.JsonAliases.parse(bodyStr) } transactionRequestType = TransactionRequestType(transactionRequestTypeStr) - view <- Future { - // System views (owner, accountant, etc.) and custom views (e.g. VRP - // `_vrp-…` views) are stored separately. Try system first; fall back - // to the account-scoped custom view. SS.init only needs *some* View - // instance — the connector reads viewId from the parameter, not the - // View object — so a soft fallback is fine here. + view <- resolveCreateTransactionRequestView(viewIdStr, () => Views.views.vend.systemView(ViewId(viewIdStr)) .or(Views.views.vend.customView(ViewId(viewIdStr), BankIdAccountId(account.bankId, account.accountId))) - .openOrThrowException(s"$ViewNotFound Current view_id($viewIdStr)") - } + ) // SS.init populates Lift thread-globals (used by `SS.user` inside the // connector). The connector's first line `SS.user` resolves synchronously // inside this block, capturing the user; subsequent flatMap stages run on @@ -6392,7 +6425,14 @@ object Http4s400 { case req @ GET -> `prefixPath` / "banks" / _ / "user-invitations" / secretLink => EndpointHelpers.withUserAndBank(req) { (_, bank, cc) => for { - (invitation, _) <- NewStyle.function.getUserInvitation(bank.bankId, secretLink.toLong, Some(cc)) + // `secretLink.toLong` used to run unguarded, so any non-numeric path segment left a + // NumberFormatException to escape as OBP-50000 / HTTP 500 -- a malformed identifier + // reported to the caller as a server fault, which tells a client with retry logic to + // keep sending a request that can never succeed. + secret <- NewStyle.function.tryons(s"$InvalidNumber Invalid SECRET_LINK: it must be a number.", 400, Some(cc)) { + secretLink.toLong + } + (invitation, _) <- NewStyle.function.getUserInvitation(bank.bankId, secret, Some(cc)) } yield JSONFactory400.createUserInvitationJson(invitation) } } @@ -7095,7 +7135,7 @@ object Http4s400 { "Get My Api Collection Endpoint", s"""Get Api Collection Endpoint By API_COLLECTION_NAME and OPERATION_ID. | - |${userAuthenticationMessage(false)} + |${userAuthenticationMessage(true)} |""".stripMargin, EmptyBody, apiCollectionEndpointJson400, @@ -7113,7 +7153,7 @@ object Http4s400 { "Get Api Collection Endpoints", s"""Get Api Collection Endpoints By API_COLLECTION_ID. | - |${userAuthenticationMessage(false)} + |${userAuthenticationMessage(true)} |""".stripMargin, EmptyBody, apiCollectionEndpointsJson400, @@ -11097,7 +11137,7 @@ object Http4s400 { .orElse(createUserInvitation.run(req)) } - lazy val allRoutesWithMiddleware: HttpRoutes[IO] = ResourceDocMiddleware.apply(resourceDocs)(allOwnRoutes) + lazy val allRoutesWithMiddleware: HttpRoutes[IO] = ResourceDocMiddleware.apply(resourceDocs)(IdempotencyMiddleware(allOwnRoutes)) // ─── nameOf-compatibility aliases ──────────────────────────────────────── // These vals have no Lift counterpart in Http4s400 but are referenced by diff --git a/obp-api/src/main/scala/code/api/v5_0_0/Http4s500.scala b/obp-api/src/main/scala/code/api/v5_0_0/Http4s500.scala index 1de94f93d0..dd1d323a2e 100644 --- a/obp-api/src/main/scala/code/api/v5_0_0/Http4s500.scala +++ b/obp-api/src/main/scala/code/api/v5_0_0/Http4s500.scala @@ -14,6 +14,7 @@ import code.api.util.ErrorMessages import code.api.util.ErrorMessages._ import code.api.util.http4s.Http4sRequestAttributes.{EndpointHelpers, RequestOps} import code.api.util.http4s.ResourceDocMiddleware +import code.api.util.http4s.IdempotencyMiddleware import code.api.util.newstyle.ViewNewStyle import code.api.util.{APIUtil, ConsentJWT, ConsentView, Consent, CustomJsonFormats, JwtUtil, NewStyle, OBPBankId, SecureRandomUtil} import code.api.v2_1_0.JSONFactory210 @@ -2353,7 +2354,7 @@ object Http4s500 { } val allRoutesWithMiddleware: HttpRoutes[IO] = - ResourceDocMiddleware.apply(resourceDocs)(allRoutes) + ResourceDocMiddleware.apply(resourceDocs)(IdempotencyMiddleware(allRoutes)) // ─── path-rewriting bridge: /obp/v5.0.0/… → /obp/v4.0.0/… ───────────── // Cascades inherited (v1.2.1–v4.0.0) endpoints through the http4s versions diff --git a/obp-api/src/main/scala/code/api/v5_1_0/Http4s510.scala b/obp-api/src/main/scala/code/api/v5_1_0/Http4s510.scala index 96e1d695d0..ac899ea6a4 100644 --- a/obp-api/src/main/scala/code/api/v5_1_0/Http4s510.scala +++ b/obp-api/src/main/scala/code/api/v5_1_0/Http4s510.scala @@ -13,7 +13,7 @@ import code.api.util.ApiTag._ import code.api.util.ErrorMessages import code.api.util.ErrorMessages._ import code.api.util.http4s.Http4sRequestAttributes.{EndpointHelpers, RequestOps} -import code.api.util.http4s.{ResourceDocMiddleware, ResourceDocMatcher} +import code.api.util.http4s.{IdempotencyMiddleware, ResourceDocMatcher, ResourceDocMiddleware} import code.api.util.newstyle.{BalanceNewStyle, RegulatedEntityAttributeNewStyle, ViewNewStyle} import code.api.util.newstyle.RegulatedEntityNewStyle.{createRegulatedEntityNewStyle, deleteRegulatedEntityNewStyle, getRegulatedEntitiesNewStyle, getRegulatedEntityByEntityIdNewStyle} import code.api.util.newstyle.Consumer.createConsumerNewStyle @@ -3041,6 +3041,39 @@ object Http4s510 { http4sPartialFunction = Some(createMyConsumer) ) + // Walks a Throwable's cause chain looking for a JVM/security-provider configuration problem + // (the requested algorithm or provider is unavailable) rather than anything about the + // caller-supplied certificate or JWT. `RSASSAVerifier`/`SignedJWT.verify` wrap + // NoSuchAlgorithmException in a JOSEException when the JVM's registered security providers + // don't have the requested signature algorithm (a hardened/FIPS JRE, a stripped provider + // list, a provider-registration bug) -- a server/environment fault that has nothing to do + // with whether this particular client's certificate is well-formed. + private[v5_1_0] def hasSecurityProviderCause(t: Throwable): Boolean = + Iterator.iterate(t)(_.getCause).takeWhile(_ != null).exists { + case _: java.security.NoSuchAlgorithmException => true + case _: java.security.NoSuchProviderException => true + case _ => false + } + + // `JwtUtil.verifyJwt` does not merely return false for a bad certificate -- it can THROW at + // several points (PEM parsing, JWT parsing, key extraction, signature verification), and a + // client-malformed certificate or JWT is exactly what most of those throws mean. But wrapping + // the whole call in tryons(..., 400, ...) also converted a JVM/security-provider failure (see + // hasSecurityProviderCause) into the same 400 -- telling a caller their input was bad when + // the truth is the server's environment cannot perform this verification for ANY caller. + // `verify` is a thunk rather than a direct call so this is testable without live PEM/JWT + // material: production passes `() => JwtUtil.verifyJwt(jwt, pem)`, the test a stub that + // throws a chosen exception. + private[v5_1_0] def resolveJwtSignatureValid( + verify: () => Boolean + )(implicit cc: code.api.util.CallContext): Future[Boolean] = + Future(verify()).recoverWith { + case t if hasSecurityProviderCause(t) => + Future.failed(t) + case t => + NewStyle.function.tryons(PostJsonIsNotSigned, 400, Some(cc)) { throw t } + } + val createConsumerDynamicRegistration: HttpRoutes[IO] = HttpRoutes.of[IO] { case req @ POST -> `prefixPath` / "dynamic-registration" / "consumers" => EndpointHelpers.executeFutureCreated(req) { @@ -3050,9 +3083,14 @@ object Http4s510 { com.openbankproject.commons.util.JsonAliases.parse(cc.httpBody.getOrElse("")).extract[ConsumerJwtPostJsonV510] } pem = APIUtil.`getPSD2-CERT`(cc.requestHeaders) - _ <- Helper.booleanToFuture(PostJsonIsNotSigned, 400, Some(cc)) { - JwtUtil.verifyJwt(postedJwt.jwt, pem.getOrElse("")) - } + // `verifyJwt` does not merely return false for a bad certificate -- it THROWS + // ("No PEM-encoded keys found") when the PSD2-CERT header is absent or unparseable, + // and booleanToFuture only guards the false case, so the exception escaped as + // OBP-50000 / HTTP 500. A missing or malformed client certificate is a client error; + // reporting it as a server fault tells a caller with retry logic to keep sending a + // request that cannot ever succeed. + signatureValid <- resolveJwtSignatureValid(() => JwtUtil.verifyJwt(postedJwt.jwt, pem.getOrElse(""))) + _ <- Helper.booleanToFuture(PostJsonIsNotSigned, 400, Some(cc)) { signatureValid } postedJson <- NewStyle.function.tryons(InvalidJsonFormat, 400, Some(cc)) { com.openbankproject.commons.util.JsonAliases.parse(JwtUtil.getSignedPayloadAsJson(postedJwt.jwt).getOrElse("{}")).extract[ConsumerPostJsonV510] } @@ -5326,7 +5364,7 @@ object Http4s510 { } val allRoutesWithMiddleware: HttpRoutes[IO] = - ResourceDocMiddleware.apply(resourceDocs)(allRoutes) + ResourceDocMiddleware.apply(resourceDocs)(IdempotencyMiddleware(allRoutes)) // ─── path-rewriting bridge: /obp/v5.1.0/… → /obp/v5.0.0/… ───────────── lazy val v510ToV500Bridge: HttpRoutes[IO] = Kleisli[HttpF, Request[IO], Response[IO]] { req => diff --git a/obp-api/src/main/scala/code/api/v6_0_0/Http4s600.scala b/obp-api/src/main/scala/code/api/v6_0_0/Http4s600.scala index 01b6bae3a7..64a5b9fdb5 100644 --- a/obp-api/src/main/scala/code/api/v6_0_0/Http4s600.scala +++ b/obp-api/src/main/scala/code/api/v6_0_0/Http4s600.scala @@ -32,7 +32,7 @@ import code.api.util.{APIUtil, CallContext, CustomJsonFormats, NewStyle} import code.api.util.ApiRole._ import code.api.util.ApiTag._ import code.api.util.ErrorMessages._ -import code.api.util.http4s.{ErrorResponseConverter, RequestScopeConnection, ResourceDocMiddleware, ResourceDocMatcher} +import code.api.util.http4s.{ErrorResponseConverter, IdempotencyMiddleware, RequestScopeConnection, ResourceDocMatcher, ResourceDocMiddleware} import code.api.util.http4s.Http4sRequestAttributes.{EndpointHelpers, RequestOps} import code.api.util.newstyle.ViewNewStyle import code.api.v2_0_0.JSONFactory200 @@ -1022,6 +1022,32 @@ object Http4s600 { } + // Resolves the portal URL used to build the reset-password link. Unit-testable without + // touching Props: `portalUrlBox` is production's real `APIUtil.getPortalUrl` call in the + // route below, and a fixed Box in the test. + // + // 503, not 400. A missing public_obp_portal_url/portal_external_url is an operator's + // configuration mistake, not this caller's -- the exact condition Http4s700's createTestEmail + // reports as 503 ("the server is not broken -- it is not configured to do this, and [a wrong + // code] tells a caller with retry logic that the fault is transient"). A bare + // Future.failed(new Exception(s"$IncompleteServerConfiguration ...")) resolves to 400: the + // message starts with "OBP-10056: ", which ErrorResponseConverter's OBP-prefix path promotes + // only to {401,403,408,429} and defaults everything else to 400 -- so the admin resetting a + // password is told their request was bad. tryons with an explicit failCode bypasses that + // default entirely. + private[v6_0_0] def resolveResetPasswordPortalUrl( + portalUrlBox: net.liftweb.common.Box[String] + )(implicit cc: CallContext): Future[String] = + portalUrlBox match { + case Full(url) => Future.successful(url) + case _ => + NewStyle.function.tryons( + s"$IncompleteServerConfiguration public_obp_portal_url (or legacy portal_external_url) is not set", + 503, Some(cc)) { + throw new NoSuchElementException("public_obp_portal_url") + } + } + // Route: POST /obp/v6.0.0/management/user/reset-password-url (201) lazy val resetPasswordUrl: HttpRoutes[IO] = HttpRoutes.of[IO] { case req @ POST -> `prefixPath` / "management" / "user" / "reset-password-url" => @@ -1048,10 +1074,7 @@ object Http4s600 { case _ => throw new Exception("User not found, not validated, or email mismatch") } } - portalUrl <- APIUtil.getPortalUrl match { - case Full(url) => Future.successful(url) - case _ => Future.failed(new Exception(s"$IncompleteServerConfiguration public_obp_portal_url (or legacy portal_external_url) is not set")) - } + portalUrl <- resolveResetPasswordPortalUrl(APIUtil.getPortalUrl) resetLink <- Future { val user: AuthUser = authUser user.uniqueId.set(java.util.UUID.randomUUID().toString.replace("-", "")) @@ -2621,9 +2644,15 @@ object Http4s600 { code.api.cache.RedisMessaging.validateChannelName(channelName) } info <- Future(code.api.cache.RedisMessaging.channelInfo(channelName)) + // A plain RuntimeException here surfaced as OBP-50000 / HTTP 500. "The thing you asked + // for does not exist" is the textbook 404; a 500 says the server broke, and a client + // cannot tell from it that retrying is pointless. (count, ttl) <- info match { case Some((c, t)) => Future.successful((c, t)) - case None => Future.failed(new RuntimeException(s"Channel '$channelName' not found")) + case None => + NewStyle.function.tryons(s"$SignalChannelNotFound Channel '$channelName' not found.", 404, Some(cc)) { + throw new NoSuchElementException(channelName) + } } } yield SignalChannelInfoJsonV600(channelName, count, ttl) } @@ -6308,7 +6337,7 @@ object Http4s600 { // Deferring index construction to first request (post object-init) lets every // registration land before the snapshot is taken. lazy val allRoutesWithMiddleware: HttpRoutes[IO] = - ResourceDocMiddleware.apply(resourceDocs)(allRoutes) + ResourceDocMiddleware.apply(resourceDocs)(IdempotencyMiddleware(allRoutes)) // ─── path-rewriting bridge: /obp/v6.0.0/… → /obp/v5.1.0/… ───────────── // Targets v5.1.0; Http4s510 has its own working cascade down to v5.0.0 → v4.0.0 → … @@ -8773,7 +8802,7 @@ object Http4s600 { | |9 user_id (if null ignore) | - |Authentication is Required. + |${userAuthenticationMessage(true)} | |""".stripMargin, EmptyBody, @@ -9868,7 +9897,7 @@ object Http4s600 { | |Optional query parameter `tag` — filter to products that have the given tag (e.g. `?tag=featured`). Tag matching is case-insensitive. | - |${userAuthenticationMessage(!getApiProductsIsPublic)}""".stripMargin, + |${userAuthenticationMessage(true)}""".stripMargin, EmptyBody, apiProductsJsonV600, List(UnknownError), @@ -9886,7 +9915,7 @@ object Http4s600 { | |Optional query parameter `tag` — filter to products that carry the given tag (e.g. `?tag=featured`). Tag matching is case-insensitive. Repeat `tag=` to require multiple tags. | - |${userAuthenticationMessage(!getProductsIsPublic)}""".stripMargin, + |${userAuthenticationMessage(true)}""".stripMargin, EmptyBody, productsJsonV600, List(UnknownError), @@ -13341,7 +13370,7 @@ object Http4s600 { |Properties with sensitive keys or values (containing ${APIUtil.sensitiveKeywords.mkString(", ")}) |are excluded from the response entirely. | - |Authentication is Required. + |${userAuthenticationMessage(true)} | |""".stripMargin, EmptyBody, 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 7486daecb2..f80101b1b6 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 @@ -1953,14 +1953,17 @@ object Http4s700 { _ <- Helper.booleanToFuture(UserEmailAddressMissing, 400, Some(cc)) { toAddress.nonEmpty } + // 503, not 500. The server is not broken -- it is not configured to do this, and a + // 500 tells a caller with retry logic that the fault is transient. Neither of these + // resolves without an operator editing props. _ <- Helper.booleanToFuture( s"$IncompleteServerConfiguration portal_external_url is not set — signup-validation and password-reset emails will not be delivered.", - 500, Some(cc)) { + 503, Some(cc)) { portalUrlBox.isDefined } _ <- Helper.booleanToFuture( s"$IncompleteServerConfiguration mail.users.userinfo.sender.address is still the default 'noreply@example.com' — most SMTP servers will reject this From address.", - 500, Some(cc)) { + 503, Some(cc)) { fromAddress != "noreply@example.com" } sendOutcome <- Future { diff --git a/obp-api/src/main/scala/code/bankconnectors/LocalMappedConnectorInternal.scala b/obp-api/src/main/scala/code/bankconnectors/LocalMappedConnectorInternal.scala index 9d05ca346c..0467216cd9 100644 --- a/obp-api/src/main/scala/code/bankconnectors/LocalMappedConnectorInternal.scala +++ b/obp-api/src/main/scala/code/bankconnectors/LocalMappedConnectorInternal.scala @@ -32,7 +32,7 @@ import com.openbankproject.commons.model._ import com.openbankproject.commons.model.enums.ChallengeType.OBP_TRANSACTION_REQUEST_CHALLENGE import com.openbankproject.commons.model.enums.TransactionRequestTypes._ import com.openbankproject.commons.model.enums.{TransactionRequestStatus, _} -import com.tesobe.CacheKeyFromArguments +import com.tesobe.{CacheKeyFromArguments, CacheKeyOmit} import net.liftweb.common._ import org.json4s.JsonAST.JValue import org.json4s.native.Serialization.write @@ -478,7 +478,14 @@ object LocalMappedConnectorInternal extends MdcLoggable { Full(cardList) } - def getCurrentFxRateCached(bankId: BankId, fromCurrencyCode: String, toCurrencyCode: String, callContext: Option[CallContext]): Box[FXRate] = { + // @CacheKeyOmit on callContext: the rate depends on the bank and the currency pair only, but + // CacheKeyFromArguments renders every un-annotated parameter into the key, and CallContext + // carries per-request state (startTime, correlationId, url, verb, ipAddress, user). Keying on + // it made the key unique per request: the cache could never hit, and every call wrote a fresh + // Redis entry that lived out code.fx.exchangeRate.cache.ttl.seconds. The generated connectors + // have always annotated their callContext (see ConnectorBuilderUtil); this hand-written site + // simply never did. + def getCurrentFxRateCached(bankId: BankId, fromCurrencyCode: String, toCurrencyCode: String, @CacheKeyOmit callContext: Option[CallContext]): Box[FXRate] = { /** * Please note that "var cacheKey = (randomUUID().toString, randomUUID().toString, randomUUID().toString)" * is just a temporary value field with UUID values in order to prevent any ambiguity. diff --git a/obp-api/src/main/scala/code/bankconnectors/package.scala b/obp-api/src/main/scala/code/bankconnectors/package.scala index 7d835d1097..1211117536 100644 --- a/obp-api/src/main/scala/code/bankconnectors/package.scala +++ b/obp-api/src/main/scala/code/bankconnectors/package.scala @@ -21,7 +21,7 @@ import net.liftweb.util.ThreadGlobal import scala.concurrent.Future import scala.reflect.runtime.universe.{MethodSymbol, Type, typeOf} -import scala.util.{Success => TrySuccess, Failure => TryFailure} +import scala.util.{Try, Success => TrySuccess, Failure => TryFailure} import com.openbankproject.commons.util.{ApiVersion, ReflectUtils} import com.openbankproject.commons.util.ReflectUtils._ import com.openbankproject.commons.util.Functions.Implicits._ @@ -46,115 +46,119 @@ package object bankconnectors extends MdcLoggable { //this object is a empty Connector implementation, just for supply default args object StubConnector extends Connector - val intercept: InvocationHandler = new InvocationHandler { - override def invoke(proxy: AnyRef, method: Method, args: Array[AnyRef]): AnyRef = { - if (method.getReturnType.getName == "scala.concurrent.Future" && !canOpenFuture(method.getName)) { - throw new RuntimeException(ServiceIsTooBusy + s"Current Service(${method.getName})") - } else { - if (method.getName.contains("$default$") || ConnectorProxy.isInheritedMember(method)) { - // The empty Connector implements both: the $default$ accessors it inherits, and the - // members Connector itself does not declare. Routing the latter would look them up as - // connector calls - and NPE on the way, since args is null for a no-arg method. - val connectorMethodResult = method.invoke(StubConnector, args:_*) - if (connectorMethodResult.isInstanceOf[Future[_]] && canOpenFuture(method.getName)) { - FutureUtil.futureWithLimits(connectorMethodResult.asInstanceOf[Future[_]], method.getName) - } - connectorMethodResult - } else { - val methodName = method.getName - val argNameToValue: Array[(String, AnyRef)] = method.getParameters.map(_.getName).zip(args) - // TODO: getConnectorNameAndMethodRouting is also called inside invokeMethod. - // Consider refactoring invokeMethod to accept a pre-resolved connectorName to avoid the duplicate lookup. - val (_, connectorName) = getConnectorNameAndMethodRouting(methodName, argNameToValue) - - // Extract correlationId from CallContext before entering any Future callback, - // because Lift's S.containerSession is unavailable in async contexts. - val correlationId: String = args.collectFirst { - case Some(cc: CallContext) => cc.correlationId - case Full(cc: CallContext) => cc.correlationId - }.getOrElse(getCorrelationId()) // fallback to Lift session if no CallContext in args - - // Record outbound (before call) - ConnectorCountsRedis.incrementOutbound(connectorName, methodName) - val t0 = System.currentTimeMillis() - - val (connectorMethodResult, methodSymbol) = invokeMethod(method, args) - - // Track metrics for Future results - if (connectorMethodResult.isInstanceOf[Future[_]]) { - val future = connectorMethodResult.asInstanceOf[Future[Any]] - future.onComplete { result => - val duration = System.currentTimeMillis() - t0 - val isSuccess = result match { - case TrySuccess(value) => !isFailureBox(value) - case TryFailure(_) => false - } - - // Record inbound - ConnectorCountsRedis.incrementInbound(connectorName, methodName, isSuccess) + // Record the outcome of a connector call: counters, plus optional detailed metric/trace persistence. + def recordConnectorInboundMetrics(connectorName: String, methodName: String, correlationId: String, + duration: Long, isSuccess: Boolean, args: Array[AnyRef]): Unit = { + ConnectorCountsRedis.incrementInbound(connectorName, methodName, isSuccess) + if (getPropsAsBoolValue("write_connector_metrics", false)) { + val params = extractKeyParams(args) + Future { + ConnectorMetricsProvider.metrics.vend.saveConnectorMetric( + connectorName, methodName, correlationId, now, duration, params, isSuccess) + } + } + } - // Record detailed metric to DB - if (getPropsAsBoolValue("write_connector_metrics", false)) { - val params = extractKeyParams(args) - Future { - ConnectorMetricsProvider.metrics.vend.saveConnectorMetric( - connectorName, methodName, correlationId, now, duration, params, isSuccess) - } - } + def recordConnectorTrace(connectorName: String, methodName: String, method: Method, args: Array[AnyRef], + duration: Long, isSuccess: Boolean, result: Try[Any]): Unit = { + if (getPropsAsBoolValue("write_connector_trace", false)) { + val outbound = serializeOutboundArgs(method, args) + val inbound = serializeInboundResult(result) + val correlationId = getCorrelationId() + val (detailUserId, detailHttpVerb, detailApiUrl) = extractCallContextInfo(args) + val bankIdValue = extractBankIdFromArgs(args) + Future { + ConnectorTraceProvider.saveConnectorTrace( + correlationId, connectorName, methodName, bankIdValue, + outbound, inbound, now, duration, isSuccess, + detailUserId, detailHttpVerb, detailApiUrl) + } + } + } - // Record connector trace (outbound/inbound messages) - if (getPropsAsBoolValue("write_connector_trace", false)) { - val outbound = serializeOutboundArgs(method, args) - val inbound = serializeInboundResult(result) - val correlationId = getCorrelationId() - val (detailUserId, detailHttpVerb, detailApiUrl) = extractCallContextInfo(args) - val bankIdValue = extractBankIdFromArgs(args) - Future { - ConnectorTraceProvider.saveConnectorTrace( - correlationId, connectorName, methodName, bankIdValue, - outbound, inbound, now, duration, isSuccess, - detailUserId, detailHttpVerb, detailApiUrl) - } - } - } - } else { - // Non-future (legacy Box) result - track synchronously - val duration = System.currentTimeMillis() - t0 - val isSuccess = !isFailureBox(connectorMethodResult) - - ConnectorCountsRedis.incrementInbound(connectorName, methodName, isSuccess) - - if (getPropsAsBoolValue("write_connector_metrics", false)) { - val params = extractKeyParams(args) - Future { - ConnectorMetricsProvider.metrics.vend.saveConnectorMetric( - connectorName, methodName, correlationId, now, duration, params, isSuccess) - } - } + // The empty Connector implements both: the $default$ accessors it inherits, and the + // members Connector itself does not declare. Routing the latter would look them up as + // connector calls - and NPE on the way, since args is null for a no-arg method. + def delegateToStub(method: Method, args: Array[AnyRef]): AnyRef = { + val connectorMethodResult = method.invoke(StubConnector, args:_*) + if (connectorMethodResult.isInstanceOf[Future[_]] && canOpenFuture(method.getName)) { + FutureUtil.futureWithLimits(connectorMethodResult.asInstanceOf[Future[_]], method.getName) + } + connectorMethodResult + } - // Record connector trace (outbound/inbound messages) - if (getPropsAsBoolValue("write_connector_trace", false)) { - val outbound = serializeOutboundArgs(method, args) - val inbound = serializeInboundResult(TrySuccess(connectorMethodResult)) - val correlationId = getCorrelationId() - val (detailUserId, detailHttpVerb, detailApiUrl) = extractCallContextInfo(args) - val bankIdValue = extractBankIdFromArgs(args) - Future { - ConnectorTraceProvider.saveConnectorTrace( - correlationId, connectorName, methodName, bankIdValue, - outbound, inbound, now, duration, isSuccess, - detailUserId, detailHttpVerb, detailApiUrl) - } - } - } - - if (connectorMethodResult.isInstanceOf[Future[_]] && canOpenFuture(method.getName)) { - FutureUtil.futureWithLimits(connectorMethodResult.asInstanceOf[Future[_]], method.getName) - } - logger.debug(s"do required field validation for ${methodSymbol.typeSignature}") - val apiVersion = ApiVersionHolder.getApiVersion - validateRequiredFields(connectorMethodResult, methodSymbol.returnType, apiVersion) + def routeToConnector(method: Method, args: Array[AnyRef]): AnyRef = { + val methodName = method.getName + val argNameToValue: Array[(String, AnyRef)] = method.getParameters.map(_.getName).zip(args) + // TODO: getConnectorNameAndMethodRouting is also called inside invokeMethod. + // Consider refactoring invokeMethod to accept a pre-resolved connectorName to avoid the duplicate lookup. + val (_, connectorName) = getConnectorNameAndMethodRouting(methodName, argNameToValue) + + // Extract correlationId from CallContext before entering any Future callback, + // because Lift's S.containerSession is unavailable in async contexts. + val correlationId: String = args.collectFirst { + case Some(cc: CallContext) => cc.correlationId + case Full(cc: CallContext) => cc.correlationId + }.getOrElse(getCorrelationId()) // fallback to Lift session if no CallContext in args + + // Record outbound (before call) + ConnectorCountsRedis.incrementOutbound(connectorName, methodName) + val t0 = System.currentTimeMillis() + + val (connectorMethodResult, methodSymbol) = invokeMethod(method, args) + + // Track metrics for Future results + if (connectorMethodResult.isInstanceOf[Future[_]]) { + val future = connectorMethodResult.asInstanceOf[Future[Any]] + future.onComplete { result => + val duration = System.currentTimeMillis() - t0 + val isSuccess = result match { + case TrySuccess(value) => !isFailureBox(value) + case TryFailure(_) => false } + recordConnectorInboundMetrics(connectorName, methodName, correlationId, duration, isSuccess, args) + recordConnectorTrace(connectorName, methodName, method, args, duration, isSuccess, result) + } + } else { + // Non-future (legacy Box) result - track synchronously + val duration = System.currentTimeMillis() - t0 + val isSuccess = !isFailureBox(connectorMethodResult) + recordConnectorInboundMetrics(connectorName, methodName, correlationId, duration, isSuccess, args) + recordConnectorTrace(connectorName, methodName, method, args, duration, isSuccess, TrySuccess(connectorMethodResult)) + } + + if (connectorMethodResult.isInstanceOf[Future[_]] && canOpenFuture(method.getName)) { + FutureUtil.futureWithLimits(connectorMethodResult.asInstanceOf[Future[_]], method.getName) + } + logger.debug(s"do required field validation for ${methodSymbol.typeSignature}") + val apiVersion = ApiVersionHolder.getApiVersion + validateRequiredFields(connectorMethodResult, methodSymbol.returnType, apiVersion) + } + + val intercept: InvocationHandler = new InvocationHandler { + override def invoke(proxy: AnyRef, method: Method, rawArgs: Array[AnyRef]): AnyRef = { + // `java.lang.reflect.Proxy` passes null for a method that declares no parameters; cglib, + // which this replaced, passed a zero-length array. Everything downstream treats args as a + // collection -- `.zip(args)`, `args.collectFirst`, `extractKeyParams(args)` -- and every + // one of those throws on null. + // + // isInheritedMember covers the members Connector does not declare, but a NO-ARGUMENT + // method that Connector DOES declare slips past it and lands in routeToConnector. + // Measured on GET /obp/v6.0.0/system/connector-method-names, which reads + // `connector.callableMethods`: 200 on the 2.12/cglib build, 500 on this one, with + // `Cannot invoke "scala.collection.IterableOnce.knownSize()" because "that" is null` -- + // which is `zip` being handed the null. + // + // Normalising to an empty array restores exactly what cglib did, which is what a + // toolchain migration owes its callers. `method.invoke(target, args: _*)` is unaffected: + // it compiles to Java varargs and an empty array means the same as null there. + val args: Array[AnyRef] = if (rawArgs == null) Array.empty[AnyRef] else rawArgs + if (method.getReturnType.getName == "scala.concurrent.Future" && !canOpenFuture(method.getName)) { + throw new RuntimeException(ServiceIsTooBusy + s"Current Service(${method.getName})") + } else if (method.getName.contains("$default$") || ConnectorProxy.isInheritedMember(method)) { + delegateToStub(method, args) + } else { + routeToConnector(method, args) } } } diff --git a/obp-api/src/test/resources/kryo_golden_chill_0_9_3.txt b/obp-api/src/test/resources/kryo_golden_chill_0_9_3.txt new file mode 100644 index 0000000000..e61fe9e03a --- /dev/null +++ b/obp-api/src/test/resources/kryo_golden_chill_0_9_3.txt @@ -0,0 +1,15 @@ +# Kryo/chill golden fixture. Written by the chill on the generating classpath -- +# chill 0.9.3 / chill-bijection 0.9.1 from the pre-migration (Scala 2.12) build, +# the versions PR #2890 replaces with 0.9.5. +# One line per value: namebase64(KryoInjection(value)). +# Regenerating this with the NEW chill would defeat its entire purpose. +string AwFhLWNhY2hlZC1zdHJpbuc= +int AlQ= +long CZaT2J/uRw== +boolean BQE= +double CkAKAAAAAAAA +jlist-string AQBqYXZhLnV0aWwuQXJyYXlMaXP0AQMDAYJhAwGCYgMBgmM= +jlist-empty AQBqYXZhLnV0aWwuQXJyYXlMaXP0AQA= +jmap AQBqYXZhLnV0aWwuTGlua2VkSGFzaE1h8AEBAwFrsQMBdrE= +nested AQBqYXZhLnV0aWwuQXJyYXlMaXP0AQIBAAEBAwGCeAEAAQIDAYJ5AwGCeg== +byte-array XwEFAQIDBA== diff --git a/obp-api/src/test/resources/kryo_scala_golden_chill_0_9_3.tsv b/obp-api/src/test/resources/kryo_scala_golden_chill_0_9_3.tsv new file mode 100644 index 0000000000..5af54008cc --- /dev/null +++ b/obp-api/src/test/resources/kryo_scala_golden_chill_0_9_3.tsv @@ -0,0 +1,12 @@ +# name base64(chill 0.9.3 bytes) runtime class as written +scala-list-string dwEDAwGCYQMBgmIDAYJj scala.collection.immutable.$colon$colon +scala-list-empty dgEA scala.collection.immutable.Nil$ +scala-list-int dwEDAgICBAIG scala.collection.immutable.$colon$colon +scala-vector FAECAwGCeAMBgnk= scala.collection.immutable.Vector +scala-map GwECJwEDAWuxAwF2sScBAwFrsgMBdrI= scala.collection.immutable.Map$Map2 +scala-set FgECAwGCYQMBgmI= scala.collection.immutable.Set$Set2 +scala-seq dwECAwFzsQMBc7I= scala.collection.immutable.$colon$colon +scala-option-some EQEDAWhlcuU= scala.Some +scala-option-none dAE= scala.None$ +scala-tuple JwEDAYJhAgI= scala.Tuple2 +scala-nested-list dwECdwEBAwGCeHcBAgMBgnkDAYJ6 scala.collection.immutable.$colon$colon diff --git a/obp-api/src/test/scala/code/api/cache/CacheKeyFormatTest.scala b/obp-api/src/test/scala/code/api/cache/CacheKeyFormatTest.scala new file mode 100644 index 0000000000..2f18bd3eda --- /dev/null +++ b/obp-api/src/test/scala/code/api/cache/CacheKeyFormatTest.scala @@ -0,0 +1,153 @@ +package code.api.cache + +import org.scalatest.{FlatSpec, Matchers} + +import scala.concurrent.duration._ +import scala.jdk.CollectionConverters._ + +/** + * The exact string scalacache derives for a cache key, pinned. + * + * PR #2890 moves scalacache 0.9.3 -> 0.28.0, which is a rewrite rather than an upgrade: the + * backends were restructured, `memoize` became `memoizeF`, `ttl: Duration` became `Some(ttl)`, + * and the Guava store's value type changed. Key derivation lives inside that rewrite. + * + * Why the existing coverage is not enough. InMemoryCachingTest asserts: + * + * InMemory.countKeys(s"*$key*") should equal(1) + * + * which proves the caller's own string survives INTO the derived key -- a substring check. It + * passes for any prefix, any separator, any argument rendering, as long as the caller's string + * is in there somewhere. That is not enough for the thing that actually depends on this format: + * + * NewStyle.scala:3306 Redis.deleteKeysByPattern("*getMethodRoutings*") + * Caching.scala:121 Redis.deleteKeysByPattern(s"${RATE_LIMIT_ACTIVE_PREFIX}${id}_*") + * + * Invalidation is pattern matching over the whole key. If the derivation grows a prefix, changes + * a separator, or renders the enclosing method differently, the cache keeps caching and the + * invalidation quietly stops matching anything -- `deleteKeysByPattern` returns 0 and swallows + * it, so nothing anywhere reports a problem. Stale MethodRoutings then serve for a full TTL. + * + * So this asserts the FULL derived key, read back out of the store, not a substring of it. + * The value is written down rather than computed, because a check that derives its expectation + * the same way the code does cannot fail. + * + * If this test breaks after a scalacache change, the fix is NOT to update the expected string + * until it passes. It is to check every deleteKeysByPattern call site against the new format + * first -- this test failing is that review being demanded, which is its whole purpose. + */ +class CacheKeyFormatTest extends FlatSpec with Matchers { + + private val ttl = 60.seconds + + private def storedKeys: Set[String] = + InMemory.underlyingGuavaCache.asMap().keySet().asScala.toSet + + private def freshMarker(tag: String): String = + s"CacheKeyFormatTest-$tag-${java.util.UUID.randomUUID().toString.take(8)}" + + /** + * The derivation, recorded from the scalacache on this branch. + * + * Discovered by writing this test with the naive expectation (the bare caller key) and reading + * what came back. The wrapper is scalacache's MethodCallToStringConverter: the enclosing + * method's fully-qualified name, then each parameter list rendered in order -- so the caller's + * key arrives inside `Some(...)`, and the two @cacheKeyExclude lists render as empty `()`. + * + * That wrapper is exactly what a substring assertion cannot see, and exactly what an + * invalidation glob has to survive. + */ + private def derivedKey(callerKey: String): String = + s"code.api.cache.InMemory.memoizeSyncWithInMemory(Some($callerKey))()()" + + "the derived cache key" should "be exactly the recorded derivation of the caller's key" in { + val marker = freshMarker("exact") + val before = storedKeys + Caching.memoizeSyncWithImMemory(Some(marker))(ttl)("stored") + val added = storedKeys -- before + + withClue(s"one memoize call should add exactly one key; added=${added.mkString(", ")} ") { + added.size shouldBe 1 + } + + // THE assertion. Not `contains`, not a regex -- the whole string. + // + // Recorded from the scalacache on this branch. Any change to it means every + // deleteKeysByPattern pattern in the codebase has to be re-read against the new shape + // before this line is updated. + withClue(s"the derived key is '${added.head}' but was recorded as '${derivedKey(marker)}'. " + + s"Before changing the expectation, check NewStyle.scala:3306's " + + s"\"*getMethodRoutings*\" and Caching.scala:121/132's rate-limit patterns still " + + s"match the new shape -- deleteKeysByPattern returns 0 and swallows a miss, so a " + + s"broken pattern is silent. ") { + added.head shouldBe derivedKey(marker) + } + } + + it should "keep the pattern MethodRouting invalidation depends on matchable" in { + // The real one. NewStyle.invalidateMethodRoutingCache issues + // deleteKeysByPattern("*getMethodRoutings*"), so a key derived from a caller string + // containing "getMethodRoutings" must be matched by that glob. + val marker = s"(CacheKeyFormatTest,getMethodRoutings,${java.util.UUID.randomUUID().toString.take(8)})" + val before = storedKeys + Caching.memoizeSyncWithImMemory(Some(marker))(ttl)("routings") + val added = (storedKeys -- before).head + + val glob = "*getMethodRoutings*" + val regex = glob.replace("*", ".*") + withClue(s"derived key '$added' is not matched by the invalidation pattern '$glob'. " + + s"NewStyle.invalidateMethodRoutingCache would delete nothing and report nothing. ") { + added.matches(regex) shouldBe true + } + InMemory.countKeys(glob) should be >= 1 + } + + it should "give different callers different keys" in { + // A derivation that collapsed distinct callers onto one key would serve one caller's value + // to another -- and every substring assertion in the suite would still pass. + val a = freshMarker("distinct-a") + val b = freshMarker("distinct-b") + val before = storedKeys + Caching.memoizeSyncWithImMemory(Some(a))(ttl)("value-a") + Caching.memoizeSyncWithImMemory(Some(b))(ttl)("value-b") + val added = storedKeys -- before + + added.size shouldBe 2 + Caching.memoizeSyncWithImMemory(Some(a))(ttl)("recomputed-a") shouldBe "value-a" + Caching.memoizeSyncWithImMemory(Some(b))(ttl)("recomputed-b") shouldBe "value-b" + } + + it should "not let one caller's key be a prefix-collision of another's" in { + // `deleteKeysByPattern` globs. If a key were rendered such that one caller's string is a + // prefix of another's WITHOUT a delimiter, invalidating the first would take out the second. + val short = freshMarker("collide") + val long = s"${short}-extended" + val before = storedKeys + Caching.memoizeSyncWithImMemory(Some(short))(ttl)("short-value") + Caching.memoizeSyncWithImMemory(Some(long))(ttl)("long-value") + val added = storedKeys -- before + + added.size shouldBe 2 + withClue(s"keys: ${added.mkString(", ")} -- an exact-match glob on the shorter key must not " + + s"also match the longer one. ") { + added.count(_ == derivedKey(short)) shouldBe 1 + added.count(_ == derivedKey(long)) shouldBe 1 + } + + // The collision the wrapper actually prevents: because the caller key is ENCLOSED rather + // than concatenated, the `)` that follows it is a hard delimiter -- the shorter key's full + // derivation is not a prefix of the longer one's, so nothing anchored on it can reach the + // longer entry. + // + // Asserted by set membership rather than through countKeys, deliberately. countKeys builds + // its matcher as `pattern.replace("*", ".*").r` (InMemory.scala:49), so every other regex + // metacharacter in the pattern is live -- and a derived key is full of them: `(`, `)` and + // `.` all appear in `...memoizeSyncWithInMemory(Some(x))()()`. Passing a whole derived key + // to countKeys therefore asks a question about regex syntax, not about key collision. + // (That is a real sharp edge in a helper whose callers pass user-shaped strings, but it + // belongs in its own finding rather than being asserted sideways from here.) + withClue(s"the shorter key's derivation must not be a prefix of the longer one's. ") { + derivedKey(long).startsWith(derivedKey(short)) shouldBe false + } + } +} diff --git a/obp-api/src/test/scala/code/api/cache/CacheSerializationNamespaceTest.scala b/obp-api/src/test/scala/code/api/cache/CacheSerializationNamespaceTest.scala new file mode 100644 index 0000000000..af32030f35 --- /dev/null +++ b/obp-api/src/test/scala/code/api/cache/CacheSerializationNamespaceTest.scala @@ -0,0 +1,105 @@ +package code.api.cache + +import code.setup.RedisTestTarget +import org.scalatest.{FlatSpec, Matchers} +import scalacache.{CacheConfig, DefaultCacheKeyBuilder} + +/** + * Two OBP-API versions must not read each other's cached bytes. + * + * The defect this pins is not hypothetical and is not a decode failure. An empty `List`, written + * by chill 0.9.3 on Scala 2.12, decodes under chill 0.9.5 on 2.13 into a + * `scala.collection.immutable.Queue`. The decode SUCCEEDS; the call site, whose signature says + * `List`, is where it dies: + * + * class scala.collection.immutable.Queue cannot be cast to + * class scala.collection.immutable.List + * + * Measured on `GET /management/dynamic-message-docs` and `GET /management/connector-methods`: + * 200 on 2.12, 500 on 2.13 reading the entry 2.12 wrote, and correct in either version alone. + * The 500 lasts the whole TTL, because a read that throws does not evict the key. A rolling + * upgrade, or any upgrade against a warm Redis, produces exactly this. + * + * `Redis.serializationNamespace` prefixes every memoized key with the Scala binary version and a + * manually bumpable counter, so entries from another build are not addressable at all and expire + * on their own. + * + * ── What is asserted ── + * + * The PROPERTY, not the string. Asserting the current prefix would pin `obpser1-scala2.13`, which + * says nothing about whether isolation holds and turns every legitimate bump into a test edit. + * What has to stay true is that two different namespaces cannot see each other's entries, and + * that one namespace still sees its own -- an isolation that isolated everything, including a + * version from itself, would "pass" while disabling the cache entirely. + * + * These run against a real Redis. RedisTestTarget cancels them when none is reachable, and + * OBP_TEST_REDIS_REQUIRED=true turns that cancel into a failure so CI cannot lose the check + * silently. + */ +class CacheSerializationNamespaceTest extends FlatSpec with Matchers { + + /** A stand-in caller key; its value is arbitrary, only its stability across calls matters. */ + private val SampleCallerKey = "code.example.Provider.getAll(Some(bank))" + + /** This build's namespace, spelled out rather than derived, so a test asserting against it + * fails loudly if the derivation and the literal ever disagree. */ + private val CurrentNamespace = "obpser1-scala2.13" + + private def keyFor(namespace: String, callerKey: String): String = + CacheConfig(cacheKeyBuilder = DefaultCacheKeyBuilder(keyPrefix = Some(namespace))) + .cacheKeyBuilder.toCacheKey(Seq(callerKey)) + + "the derived cache key" should "differ between two serialization namespaces" in { + val a = keyFor("obpser1-scala2.12", SampleCallerKey) + val b = keyFor(CurrentNamespace, SampleCallerKey) + + withClue(s"2.12 key <$a> and 2.13 key <$b> are the same, so a 2.13 instance would read the " + + s"bytes a 2.12 instance wrote -- which is the defect this exists to prevent. ") { + a should not equal b + } + a should include("2.12") + b should include("2.13") + } + + it should "also differ when only the manual counter is bumped" in { + // The Scala version does not move for a dependency upgrade that changes the encoding -- + // chill 0.9.3 to 0.9.5 on its own would not have. The counter is the escape hatch for that, + // and it is only an escape hatch if it actually changes the key. + keyFor(CurrentNamespace, SampleCallerKey) should not equal keyFor("obpser2-scala2.13", SampleCallerKey) + } + + it should "stay stable for one namespace, or nothing would ever be a cache hit" in { + keyFor(CurrentNamespace, SampleCallerKey) shouldBe keyFor(CurrentNamespace, SampleCallerKey) + } + + "the namespace this build uses" should "name the Scala binary version it was compiled against" in { + // Derived, not asserted verbatim: the point is that it tracks the axis that actually moved. + val expected = scala.util.Properties.versionNumberString.split('.').take(2).mkString(".") + val probe = keyFor(s"obpser1-scala$expected", "x") + probe should include(expected) + } + + "a real Redis" should "not return an entry written under a different namespace" in { + RedisTestTarget.requireReachable(Redis.isRedisReady, "the cross-namespace isolation check") + + val caller = s"code.example.Probe.roundTrip(${System.nanoTime()})" + val oldKey = keyFor("obpser1-scalaOLD", caller) + val newKey = keyFor("obpser1-scalaNEW", caller) + + import code.api.JedisMethod + try { + Redis.use(JedisMethod.SET, oldKey, Some(60), Some("written-by-the-other-version")) + + withClue("the new namespace found the old namespace's entry -- the prefix is not isolating ") { + Redis.use(JedisMethod.GET, newKey, None, None) shouldBe None + } + withClue("the old namespace could not read back its OWN entry, so this test proved nothing " + + "about isolation -- it would pass with the cache switched off entirely ") { + Redis.use(JedisMethod.GET, oldKey, None, None) shouldBe Some("written-by-the-other-version") + } + } finally { + Redis.use(JedisMethod.DELETE, oldKey, None, None) + Redis.use(JedisMethod.DELETE, newKey, None, None) + } + } +} diff --git a/obp-api/src/test/scala/code/api/cache/KryoGoldenCompatTest.scala b/obp-api/src/test/scala/code/api/cache/KryoGoldenCompatTest.scala new file mode 100644 index 0000000000..cb5b672f02 --- /dev/null +++ b/obp-api/src/test/scala/code/api/cache/KryoGoldenCompatTest.scala @@ -0,0 +1,226 @@ +package code.api.cache + +import org.scalatest.{FlatSpec, Matchers} + +import java.util.Base64 +import scala.io.Source +import scala.util.Try + +/** + * What the OLD chill wrote, the NEW chill must not silently misread. + * + * PR #2890 moves chill 0.9.3 -> 0.9.5 (and chill-bijection 0.9.1 -> 0.9.5), and a chill upgrade + * carries a Kryo upgrade. The commit that made the move says what that means for a live system: + * + * "the entries already in Redis were written by the old one, so some of them will fail to + * decode after the rollout. A test environment never shows this, because it starts from an + * empty cache." + * + * That last sentence is the problem this file exists for. The only other Kryo test in the suite, + * RedisDeserializeMissTest, round-trips through `encode` and `decode` -- BOTH of which run on + * whichever chill is on the classpath. A format change is invisible to it by construction: the + * new encoder and the new decoder agree with each other no matter what they agree on. + * + * So the fixtures had to be produced from outside the current build. Two of them, both encoded on + * the pre-migration 2.12 classpath by the real chill 0.9.3, neither regenerable once every + * checkout carries 0.9.5: + * + * kryo_golden_chill_0_9_3.txt ten Java values namebase64 + * kryo_scala_golden_chill_0_9_3.tsv eleven Scala values namebase64runtime class + * + * ── Why the second fixture, and why it records a class ── + * + * The first version of this file held Java collections only and compared with `==`. It could not + * have caught the defect it was written to catch, for two independent reasons, and both were + * found the hard way -- by the defect reaching a running instance. + * + * What OBP-API actually caches is Scala collections; `java.util.ArrayList` appears nowhere in + * the memoized providers. And under `==` a Scala `List()` EQUALS a `Queue()`: both are `Seq`, and + * Seq equality is element-wise, so an empty one of each compares equal. An empty `List` written + * by 0.9.3 decodes under 0.9.5 into a `scala.collection.immutable.Queue`, which the old assertion + * would have waved through -- while every call site whose signature says `List` fails with + * + * class scala.collection.immutable.Queue cannot be cast to + * class scala.collection.immutable.List + * + * Measured on GET /management/dynamic-message-docs and GET /management/connector-methods: 200 on + * 2.12, 500 on 2.13 reading 2.12's entry, for the whole TTL, because a read that throws does not + * evict the key. + * + * So the Scala fixture records the runtime class each value was written as, and this file asserts + * on THAT. Equality is not enough; the class is what the call site depends on. + * + * ── The four outcomes ── + * + * decodes to the same value, same class fine + * fails to decode fine -- a cold cache, which the upgrade note accepts + * decodes to a DIFFERENT value asserted against from the start + * decodes to an equal value of another CLASS the one that got through, now asserted + * + * Survival counts are reported rather than bounded: how many of the values survive is a fact + * about two third-party libraries, not something this branch controls, and an assertion on it + * would freeze a number nobody could act on. What the run is for is the last two outcomes. + */ +class KryoGoldenCompatTest extends FlatSpec with Matchers { + + private val JAVA_FIXTURE = "/kryo_golden_chill_0_9_3.txt" + private val SCALA_FIXTURE = "/kryo_scala_golden_chill_0_9_3.tsv" + + private def readFixture(path: String): List[Array[String]] = { + val stream = getClass.getResourceAsStream(path) + stream should not be null + val src = Source.fromInputStream(stream, "UTF-8") + try src.getLines() + .filterNot(l => l.trim.isEmpty || l.startsWith("#")) + .map(_.split("\t")) + .toList + finally src.close() + } + + /** name -> the bytes chill 0.9.3 produced for it. */ + private lazy val javaGolden: List[(String, Array[Byte])] = + readFixture(JAVA_FIXTURE).map(f => f(0) -> Base64.getDecoder.decode(f(1))) + + /** name -> (bytes, the runtime class the value had WHEN WRITTEN). */ + private lazy val scalaGolden: List[(String, Array[Byte], String)] = + readFixture(SCALA_FIXTURE).map(f => (f(0), Base64.getDecoder.decode(f(1)), f(2))) + + /** The values the Java bytes are supposed to mean. Written out here, not derived. */ + private val expected: Map[String, Any] = Map( + "string" -> "a-cached-string", + "int" -> 42, + "long" -> 1234567890123L, + "boolean" -> true, + "double" -> 3.25d, + "jlist-string" -> java.util.Arrays.asList("a", "b", "c"), + "jlist-empty" -> new java.util.ArrayList[String](), + "jmap" -> { val m = new java.util.LinkedHashMap[String, String](); m.put("k1", "v1"); m }, + "nested" -> java.util.Arrays.asList( + java.util.Arrays.asList("x"), + java.util.Arrays.asList("y", "z")), + "byte-array" -> Array[Byte](1, 2, 3, 4) + ) + + private def sameValue(a: Any, b: Any): Boolean = (a, b) match { + case (x: Array[_], y: Array[_]) => x.sameElements(y) + case (x, y) => x == y + } + + // ── fixtures present ─────────────────────────────────────────────────────────────── + + "both fixtures" should "be present and non-trivial" in { + // A fixture that failed to load would make every assertion below vacuous. + withClue("kryo_golden_chill_0_9_3.txt is missing or empty -- without it this file asserts " + + "nothing at all. It cannot be regenerated from this branch; recover it from git. ") { + javaGolden.size should be >= 8 + } + withClue("kryo_scala_golden_chill_0_9_3.tsv is missing or empty. This is the fixture that " + + "covers what OBP-API actually caches; without it the Java values alone would pass " + + "while the Scala ones drift, which is exactly what happened once already. ") { + scalaGolden.size should be >= 8 + } + scalaGolden.map(_._1).toSet should contain allOf ("scala-list-empty", "scala-map", "scala-option-none") + } + + // ── the assertion the first version was missing ──────────────────────────────────── + + /** + * Class drift that is known, and the reason it can no longer reach a caller. + * + * A signed-off baseline rather than a hard zero, for the same reason the contract suite keeps + * pr90-base.accepted.json: this is a property of two third-party libraries, not something this + * branch can change, and a permanently red suite is a suite people learn to ignore. What must + * stay red is drift that nobody has looked at -- so anything NOT listed here fails, and adding + * a line means writing down why it is safe. + */ + private val knownDrift: Map[String, String] = Map( + "scala-list-empty" -> + ("Nil$ decodes as Queue under chill 0.9.5. Mitigated by Redis.serializationNamespace: the " + + "cache key carries the Scala binary version, so a 2.13 instance cannot address the entry " + + "a 2.12 instance wrote and it expires on its own TTL. CacheSerializationNamespaceTest " + + "pins that isolation; remove it and this becomes reachable again.") + ) + + it should "never decode a Scala value into a DIFFERENT runtime class, except where recorded" in { + import com.twitter.chill.KryoInjection + + val drifted = scalaGolden.flatMap { case (name, bytes, writtenAs) => + KryoInjection.invert(bytes).toOption.flatMap { v => + val nowIs = if (v == null) "null" else v.getClass.getName + // Subclassing is not drift: a Vector written as `Vector` and read back as `Vector1` is + // still assignable to every signature that named Vector, and nothing at a call site can + // tell. What breaks is a class that is merely EQUAL -- List() == Queue() is true, and a + // `List` signature still throws ClassCastException on it. + val assignable = + try Class.forName(writtenAs).isInstance(v) catch { case _: Throwable => nowIs == writtenAs } + if (assignable) None + else Some(s"$name: written as <$writtenAs>, decodes under this chill as <$nowIs>" + + (if (v.isInstanceOf[Iterable[_]]) " -- equal by value, so an == comparison " + + "would call this correct while every call site declaring the original type " + + "fails with ClassCastException" else "")) + } + } + + val unexplained = drifted.filterNot(line => knownDrift.keys.exists(k => line.startsWith(k + ":"))) + drifted.foreach { line => + knownDrift.collectFirst { case (k, why) if line.startsWith(k + ":") => + info(s"known drift -- $line") + info(s" mitigation: $why") + } + } + + withClue(s"${unexplained.size} Scala value(s) drift into a class the original signature cannot " + + s"hold, and are not in knownDrift. This is not a cold cache -- the read SUCCEEDS and " + + s"the caller gets a ClassCastException for the whole TTL, with nothing in any log to " + + s"say so. Either mitigate it or add it to knownDrift with the reason it cannot reach " + + s"a caller:\n${unexplained.mkString("\n")}\n") { + unexplained shouldBe empty + } + + // The baseline must not outlive what it describes. A name listed here that no longer drifts + // is a line nobody will delete, and the next reader takes it as still true. + val staleEntries = knownDrift.keys.filterNot(k => drifted.exists(_.startsWith(k + ":"))).toList + withClue(s"knownDrift lists ${staleEntries.mkString(", ")}, which no longer drift. Remove " + + s"them, or the baseline documents a hazard that stopped existing. ") { + staleEntries shouldBe empty + } + } + + it should "never decode old bytes into a DIFFERENT value" in { + import com.twitter.chill.KryoInjection + + val misread = javaGolden.flatMap { case (name, bytes) => + KryoInjection.invert(bytes) match { + case scala.util.Success(v) if !sameValue(v, expected(name)) => + Some(s"$name: old bytes decoded to <$v> (${v.getClass.getName}) but were written as " + + s"<${expected(name)}> (${expected(name).getClass.getName})") + case _ => None // decoded correctly, or failed -- both acceptable, see the header + } + } + + withClue(s"${misread.size} value(s) written by chill 0.9.3 decode under this chill into " + + s"something OTHER than what was written:\n${misread.mkString("\n")}\n") { + misread shouldBe empty + } + } + + it should "report how much of an existing cache survives the upgrade" in { + import com.twitter.chill.KryoInjection + + val (jOk, jFailed) = javaGolden.partition { case (name, bytes) => + Try(KryoInjection.invert(bytes)).toOption.flatMap(_.toOption).exists(sameValue(_, expected(name))) + } + val (sOk, sFailed) = scalaGolden.partition { case (_, bytes, writtenAs) => + KryoInjection.invert(bytes).toOption.exists(v => + try Class.forName(writtenAs).isInstance(v) catch { case _: Throwable => false }) + } + // Informational on purpose -- see the header. The rollout consequence of a failure is a + // recompute, which is a cost rather than a defect, and pinning the number would freeze a + // property of two third-party libraries. + info(s"java ${jOk.size}/${javaGolden.size} values written by chill 0.9.3 still decode correctly") + info(s"scala ${sOk.size}/${scalaGolden.size} values still decode into an assignable class") + if (jFailed.nonEmpty) info(s"cold on rollout (java): ${jFailed.map(_._1).mkString(", ")}") + if (sFailed.nonEmpty) info(s"cold on rollout (scala): ${sFailed.map(_._1).mkString(", ")}") + succeed + } +} diff --git a/obp-api/src/test/scala/code/api/cache/MethodRoutingCacheInvalidationTest.scala b/obp-api/src/test/scala/code/api/cache/MethodRoutingCacheInvalidationTest.scala index f3d759b45b..3856ccac5d 100644 --- a/obp-api/src/test/scala/code/api/cache/MethodRoutingCacheInvalidationTest.scala +++ b/obp-api/src/test/scala/code/api/cache/MethodRoutingCacheInvalidationTest.scala @@ -4,6 +4,8 @@ import java.util.UUID import org.scalatest.{FlatSpec, Matchers} +import code.setup.RedisTestTarget + import scala.concurrent.duration._ /** @@ -26,7 +28,7 @@ class MethodRoutingCacheInvalidationTest extends FlatSpec with Matchers { Caching.memoizeSyncWithProvider(Some(cacheKey))(ttl)(f) "deleteKeysByPattern(*getMethodRoutings*)" should "invalidate memoized entries so the next read recomputes" in { - assume(Redis.isRedisReady, "requires a reachable Redis") + RedisTestTarget.requireReachable(Redis.isRedisReady, "the MethodRouting cache checks") val marker = s"inv-${UUID.randomUUID().toString}" val cacheKey = s"(MethodRoutingCacheInvalidationTest,getMethodRoutings,$marker)" var computations = 0 @@ -44,7 +46,7 @@ class MethodRoutingCacheInvalidationTest extends FlatSpec with Matchers { } "a corrupted cache entry" should "behave as a miss: recompute once and repopulate with valid bytes" in { - assume(Redis.isRedisReady, "requires a reachable Redis") + RedisTestTarget.requireReachable(Redis.isRedisReady, "the MethodRouting cache checks") val marker = s"poison-${UUID.randomUUID().toString}" val cacheKey = s"(MethodRoutingCacheInvalidationTest,getMethodRoutings,$marker)" var computations = 0 diff --git a/obp-api/src/test/scala/code/api/sweep/AuthSweepTest.scala b/obp-api/src/test/scala/code/api/sweep/AuthSweepTest.scala new file mode 100644 index 0000000000..6d1a29c275 --- /dev/null +++ b/obp-api/src/test/scala/code/api/sweep/AuthSweepTest.scala @@ -0,0 +1,288 @@ +package code.api.sweep + +import cats.effect.IO +import cats.effect.unsafe.IORuntime +import code.api.util.APIUtil.ResourceDoc +import code.api.util.ErrorMessages.{ApplicationNotIdentified, AuthenticatedUserIsRequired, UserHasMissingRoles} +import code.api.util.http4s.Http4sApp +import code.setup.{DefaultUsers, ServerSetupWithTestData} +import fs2.Stream +import org.http4s.{Header, Headers, Method, Request, Uri} +import org.json4s.JValue +import org.json4s.JsonAST.JObject +import org.scalatest.Tag +import org.typelevel.ci.CIString +import com.openbankproject.commons.util.JsonAliases.parse + +/** + * Every endpoint answers an unauthenticated call the way its own ResourceDoc says it will. + * + * Why a sweep instead of more hand-written suites: of the 870 endpoints a caller can reach, + * 384 are referenced by no test at all, and of the ones that ARE tested only about a third + * carry an anonymous-access scenario. Writing those by hand is several hundred near-identical + * files that then rot one endpoint at a time; driving them off the registry means an endpoint + * added tomorrow is swept the day it is registered, and SweepCoverageTest fails if it is not. + * + * Three assertions, chosen by what the doc declares: + * + * auth required, no roles anonymous -> 401 with exactly AuthenticatedUserIsRequired + * auth required, roles anonymous -> 401; authenticated-without-the-role -> 403 + * public anonymous -> anything EXCEPT 401 + * + * The public case is deliberately weak. A public endpoint may well answer 400 or 404 to a call + * with no body and a nonexistent id — that is not an authentication defect, and asserting 200 + * would make this sweep a fixture problem instead of an auth check. What must never happen is a + * public endpoint demanding credentials, and that is what is asserted. + * + * The 403 assertion uses startWith, not equal: at runtime the message carries the missing roles + * joined with " or ", and ApiRole.requiresBankId appends " for BankId(...)". An equality + * assertion here passes locally and fails the moment an endpoint gains a second role. + * + * ── Why one scenario per version rather than per endpoint ── + * ServerSetupWithTestData.beforeEach wipes and rebuilds banks, accounts and views for EVERY + * scenario. At ~1600 assertions that fixture cost, not the assertions, would dominate: the + * requests themselves run in-process against Http4sApp.httpApp with no TCP and no server, and + * cost single-digit milliseconds each. So each scenario sweeps one version, collects every + * mismatch, and fails once with the whole list. The per-endpoint detail that a + * scenario-per-endpoint layout would have given is preserved in that list — each line names the + * operationId, the verb, the URL, the expectation and what actually came back. + */ +object AuthSweepTest { + + /** + * The single definition of what this sweep covers. Exposed so SweepCoverageTest's drift check + * reads this directly instead of re-deriving its own copy of the same filter -- two copies of + * one expression are equal by construction and can never catch this sweep's own filtering + * changing independently of FailureSweepTest's. + */ + def scope: List[ResourceDoc] = EndpointCatalog.all.filter(EndpointCatalog.skipReason(_).isEmpty) +} + +class AuthSweepTest extends ServerSetupWithTestData with DefaultUsers with SweepFixtures { + + object AuthSweep extends Tag("AuthSweep") + + implicit val runtime: IORuntime = IORuntime.global + private lazy val app = Http4sApp.httpApp + + /** One in-process request. No TCP, no server startup. */ + private def call(verb: String, path: String, headers: Map[String, String]): (Int, JValue) = { + val method = Method.fromString(verb.toUpperCase).getOrElse(Method.GET) + val req = Request[IO]( + method = method, + uri = Uri.unsafeFromString(path), + headers = Headers(headers.map { case (k, v) => Header.Raw(CIString(k), v) }.toList), + body = Stream.empty + ) + val resp = app.run(req).unsafeRunSync() + val bodyStr = resp.bodyText.compile.string.unsafeRunSync() + val json = try { if (bodyStr.trim.isEmpty) JObject(Nil) else parse(bodyStr) } + catch { case _: Exception => JObject(Nil) } + (resp.status.code, json) + } + + private def messageOf(json: JValue): String = { + implicit val formats = code.api.util.CustomJsonFormats.formats + (json \ "message").extractOpt[String].getOrElse("") + } + + /** A token for a user holding no entitlements at all — the natural 403 probe. */ + private def noRoleHeaders: Map[String, String] = Map("DirectLogin" -> s"token=${token1.value}") + + /** + * Real identifiers from the fixtures, for the role assertion only. + * + * An endpoint that declares BankNotFound and carries BANK_ID validates the bank before it + * checks roles, so a nonexistent bank answers 404 and the role gate never runs. These come + * from the fixture banks/accounts ServerSetupWithTestData creates, read directly rather than + * over HTTP — the sweep is in-process and a round trip per lookup would be the only slow part + * of it. + */ + private lazy val realEntities: Map[String, String] = realBankId match { + case Some(bankIdValue) => + val accountId = code.model.dataAccess.MappedBankAccount + .find(net.liftweb.mapper.By(code.model.dataAccess.MappedBankAccount.bank, bankIdValue)) + .map(_.accountId.value) + Map("BANK_ID" -> bankIdValue) ++ accountId.map("ACCOUNT_ID" -> _).toList.toMap + case None => Map.empty + } + + private def describe(doc: ResourceDoc): String = + s"${doc.operationId} ${doc.requestVerb} ${EndpointCatalog.concretePath(doc)}" + + // ── the three checks, each returning a failure line or None ────────────────── + + /** + * Deviations that are deliberate, with the reason each one is not a defect. + * + * A signed-off list rather than a hard zero, for the same reason KryoGoldenCompatTest keeps + * knownDrift: a permanently red suite is one people learn to ignore, and the two entries here + * are both behaviour somebody chose and wrote down. Anything NOT listed still fails, and + * adding a line costs a written justification. + */ + private val expectedAuthDeviation: Map[String, String] = Map( + "OBPv4.0.0-verifyRequestSignResponse" -> + ("Refuses with OBP-20311 'The Request is not signed' -- JWS request signing, a third " + + "authentication mechanism alongside user and application. ResourceDoc has no way to " + + "declare it: authMode covers user/application only, so neither the doc nor this sweep " + + "can express the requirement. The 401 is correct; only the message differs."), + "OBPv4.0.0-createTransactionRequestFreeForm" -> + ("Answers 400 InsufficientAuthorisationToCreateTransactionRequest rather than 403. The " + + "endpoint deliberately does no upfront view/role check and delegates the decision to " + + "checkAuthorisationToCreateTransactionRequest inside the connector -- its own comment " + + "says so, and an existing test depends on it. Whether an authorisation failure ought to " + + "be 400 at all is a product question, not something to change from inside a sweep.") + ) + + /** Which exemptions were actually needed this run -- see the stale-entry scenario below. */ + private val deviationsUsed = java.util.concurrent.ConcurrentHashMap.newKeySet[String]() + + private def deviationFor(doc: ResourceDoc): Option[String] = { + val why = expectedAuthDeviation.get(doc.operationId) + if (why.isDefined) deviationsUsed.add(doc.operationId) + why + } + + private def checkAnonymousIs401(doc: ResourceDoc): Option[String] = { + val (code, json) = call(doc.requestVerb, EndpointCatalog.concretePath(doc), Map.empty) + if (code != 401) + Some(s"${describe(doc)} -- expected 401 for an anonymous call, got $code") + else if (messageOf(json) != AuthenticatedUserIsRequired) + deviationFor(doc) match { + case Some(why) => + info(s"${describe(doc)} -- 401 with '${messageOf(json)}'; expected deviation: $why") + None + case None => + Some(s"${describe(doc)} -- 401 but message was '${messageOf(json)}', expected '$AuthenticatedUserIsRequired'") + } + else None + } + + /** + * A doc that asks for no USER may still ask for an APPLICATION, and that is not a defect. + * + * OBP has three ways to refuse an anonymous caller, and this check originally modelled one: + * + * OBP-20001 User not logged in -- user authentication + * OBP-20200 The application cannot be identified -- consumer/application authentication + * OBP-20311 The Request is not signed -- JWS request signing + * + * `EndpointCatalog.needsAuthentication` reproduces the middleware's predicate, which reads + * only errorResponseBodies and roles -- both about the user. So an endpoint that requires a + * consumer is classified "public" here and then fails this assertion for doing exactly what + * its doc says. Measured on createConsentRequest, getConsentRequest and + * createVRPConsentRequest: all three answer OBP-20200, and the last one spells it out in its + * own description -- "Client, Consumer or Application Authentication is mandatory for this + * endpoint". Their docs were right; this check was wrong. + * + * So a 401 is only a violation when it is the USER one. An application-auth 401 is reported + * as an observation instead of a failure -- named, not silently swallowed, because the doc + * still has no machine-readable way to say "needs an application" unless someone sets + * authMode, and a reader of resource-docs cannot tell. + */ + private def checkPublicIsNot401(doc: ResourceDoc): Option[String] = { + val (code, json) = call(doc.requestVerb, EndpointCatalog.concretePath(doc), Map.empty) + val msg = messageOf(json) + if (code != 401) None + else if (msg.startsWith(ApplicationNotIdentified.take(9))) { + info(s"${describe(doc)} -- declares no user authentication and requires an APPLICATION " + + s"instead ($msg). The doc is accurate about the user; consider authMode = " + + s"ApplicationOnly so resource-docs can say so too.") + None + } else + Some(s"${describe(doc)} -- declares no authentication requirement yet answered 401 " + + s"anonymously with '$msg'") + } + + private def checkNoRoleIs403(doc: ResourceDoc): Option[String] = { + val path = EndpointCatalog.concretePath(doc, realEntities) + val (code, json) = call(doc.requestVerb, path, noRoleHeaders) + val roles = doc.roles.getOrElse(Nil).map(_.toString).mkString(",") + if (code != 403) + deviationFor(doc) match { + case Some(why) => + info(s"${doc.operationId} answered $code rather than 403; expected deviation: $why") + None + case None => + Some(s"${doc.operationId} ${doc.requestVerb} $path -- roles $roles: " + + s"expected 403 for a user holding no entitlements, got $code") + } + else if (!messageOf(json).startsWith(UserHasMissingRoles)) + Some(s"${doc.operationId} ${doc.requestVerb} $path -- 403 but message was " + + s"'${messageOf(json)}', expected it to start with '$UserHasMissingRoles'") + else None + } + + // ── the sweep, one scenario per version ───────────────────────────────────── + + private lazy val byVersion: Map[String, List[ResourceDoc]] = + AuthSweepTest.scope.groupBy(_.implementedInApiVersion.toString) + + feature("Every reachable endpoint enforces the authentication its ResourceDoc declares") { + + byVersion.keys.toList.sorted.foreach { version => + scenario(s"$version -- anonymous calls are refused, public ones are not", AuthSweep) { + // Endpoint-level enable/disable props are read per request by ResourceDocMiddleware, + // and a disabled endpoint falls through to 404 rather than 401 -- which would read as a + // sweep failure. Cleared the way SwaggerDocsTest does; PropsReset restores afterwards. + // Written out in each scenario rather than shared in a helper because + // check_test_isolation.py scans statically: any setPropsValues outside a scenario body + // reads to it as a class-body push, `def` or not. + setPropsValues("api_disabled_endpoints" -> "[]", "api_enabled_endpoints" -> "[]") + val docs = byVersion(version) + + When(s"every one of the ${docs.size} $version endpoints is called with no credentials") + val failures = docs.flatMap { doc => + if (EndpointCatalog.needsAuthentication(doc)) checkAnonymousIs401(doc) + else checkPublicIsNot401(doc) + } + + Then("each one answers as its own ResourceDoc declares") + withClue(s"${failures.size} of ${docs.size} $version endpoints disagreed with their own " + + s"ResourceDoc:\n${failures.mkString("\n")}\n") { + failures shouldBe empty + } + } + } + + byVersion.keys.toList.sorted.foreach { version => + lazy val roleGated = byVersion(version) + .filter(EndpointCatalog.isRoleGated) + .filter(EndpointCatalog.roleSkipReason(_).isEmpty) + + if (roleGated.nonEmpty) { + scenario(s"$version -- role-gated endpoints refuse a user holding no entitlements", AuthSweep) { + setPropsValues("api_disabled_endpoints" -> "[]", "api_enabled_endpoints" -> "[]") + + When(s"every one of the ${roleGated.size} role-gated $version endpoints is called as a user with no entitlements") + val failures = roleGated.flatMap(checkNoRoleIs403) + + Then("each one answers 403 naming the roles it wanted") + withClue(s"${failures.size} of ${roleGated.size} role-gated $version endpoints did not " + + s"refuse an unentitled user:\n${failures.mkString("\n")}\n") { + failures shouldBe empty + } + } + } + } + + // Declared after both version loops, so deviationsUsed is complete when it runs. + scenario("no expectedAuthDeviation entry outlives the behaviour it excuses", AuthSweep) { + import scala.jdk.CollectionConverters._ + val used = deviationsUsed.asScala.toSet + val stale = expectedAuthDeviation.keySet -- used + val unknown = expectedAuthDeviation.keySet -- EndpointCatalog.all.map(_.operationId).toSet + + withClue(s"these endpoints no longer deviate, so their exemption is a claim that stopped " + + s"being true and the next reader will take it as still true: ${stale.mkString(", ")}. " + + s"Delete the entry. ") { + stale shouldBe empty + } + withClue(s"these operationIds are not in the catalog at all -- renamed or removed, and " + + s"the exemption was left behind: ${unknown.mkString(", ")}. ") { + unknown shouldBe empty + } + info(s"${used.size} deviation(s) exercised: ${used.mkString(", ")}") + } + } +} diff --git a/obp-api/src/test/scala/code/api/sweep/EndpointCatalog.scala b/obp-api/src/test/scala/code/api/sweep/EndpointCatalog.scala new file mode 100644 index 0000000000..c5bba1dd2a --- /dev/null +++ b/obp-api/src/test/scala/code/api/sweep/EndpointCatalog.scala @@ -0,0 +1,163 @@ +package code.api.sweep + +// EndpointAuthMode and its four cases are members of the APIUtil object, not of the package. +import code.api.util.APIUtil.{ResourceDoc, UserOnly} +import code.api.util.ErrorMessages.$AuthenticatedUserIsRequired +import code.api.util.ApiTag + +/** + * The one place that answers "what endpoints exist, and what does each one claim about auth". + * + * Every sweep in this package reads the catalog from here rather than assembling its own, so + * that the coverage identity in SweepCoverageTest is checkable: asserted + skipped == catalog, + * with no third bucket anyone can quietly slip an endpoint into. + * + * Three things about the source data are easy to get wrong, and all three are load-bearing: + * + * 1. The docs live on the Http4s objects, NOT on APIMethods*. Every APIMethods{121..600}.scala + * is now a stub whose entire body is `val ImplementationsX = Http4sX.ImplementationsX` — the + * Lift registrations below it are commented out. Reading those files for a catalog finds + * nothing. Http4s700.allResourceDocs is the aggregate: every version's docs, deduplicated by + * (requestUrl, requestVerb) keeping the newest, which is exactly the set a caller can reach. + * + * 2. "Needs authentication" is derived, not declared. There is no flag on ResourceDoc; the + * middleware's own predicate is + * errorResponseBodies.contains($AuthenticatedUserIsRequired) || roles.exists(_.nonEmpty) + * (ResourceDocMiddleware.needsAuthentication). We reproduce it here rather than approximate it. + * + * 3. That predicate has to be evaluated on the CONSTRUCTED ResourceDoc, never on the source text. + * The constructor rewrites errorResponseBodies: it appends AuthenticatedUserIsRequired and + * UserHasMissingRoles when roles are present, and adds/removes AuthenticatedUserIsRequired + * based on the description. Several docs also compute their error list from a prop — e.g. + * getApiProduct branches on getApiProductsIsPublic — so the answer depends on the props in + * force at the moment the sweep runs, which is another reason to ask the object and not a grep. + */ +object EndpointCatalog { + + /** Every endpoint a caller can reach, newest version of each (url, verb). */ + def all: List[ResourceDoc] = code.api.v7_0_0.Http4s700.allResourceDocs.toList + + /** The middleware's own rule, reproduced. See ResourceDocMiddleware.needsAuthentication. */ + def needsAuthentication(doc: ResourceDoc): Boolean = + doc.errorResponseBodies.contains($AuthenticatedUserIsRequired) || doc.roles.exists(_.nonEmpty) + + def isRoleGated(doc: ResourceDoc): Boolean = doc.roles.exists(_.nonEmpty) + + /** + * Why an endpoint is not swept. Every exclusion is one of these — a sweep may not invent a + * reason inline, because SweepCoverageTest counts these and nothing else. + */ + sealed abstract class SkipReason(val why: String) + case object DynamicDoc extends SkipReason( + "tagged apiTagDynamic: created per-database, so its presence is machine state, not contract") + case object NonUserAuthMode extends SkipReason( + "authMode is not UserOnly: an anonymous call may legitimately not be 401 " + + "(ApplicationOnly drops AuthenticatedUserIsRequired entirely). EndpointAuthModeTest covers these.") + case object AutoValidateRolesOff extends SkipReason( + "disableAutoValidateRoles(): roles stay in the doc for the catalog but the framework does " + + "not enforce them, so asserting 403 would assert something no code promises") + + /** Skip reason, or None when the endpoint is in scope for the auth sweep. */ + def skipReason(doc: ResourceDoc): Option[SkipReason] = + if (doc.tags.contains(ApiTag.apiTagDynamic)) Some(DynamicDoc) + else if (doc.authMode != UserOnly) Some(NonUserAuthMode) + else None + + /** Skip reason for the role dimension specifically — a superset of skipReason. */ + def roleSkipReason(doc: ResourceDoc): Option[SkipReason] = + skipReason(doc).orElse( + if (!doc.isAutoValidateRoles) Some(AutoValidateRolesOff) else None) + + /** + * Not every ALL_CAPS segment in a requestUrl is a placeholder. OBP serves real literals in + * that shape — `/transaction-request-types/SANDBOX_TAN/`, `/my/consents/EMAIL` — and + * substituting those produces a URL that routes nowhere, which reads as a 404/400 "auth + * failure" that is entirely the sweep's own doing. The first run of AuthSweepTest hit exactly + * that on nine endpoints across v2.1.0 and v3.1.0. + * + * The production rule lives in ResourceDocMatcher.isTemplateVariable, which consults a private + * `literalAllCapsSegments` set. Copying that set here would give us a second copy to keep in + * sync, and a stale copy fails in the direction that is hardest to notice — a literal newly + * added there would be substituted here and the sweep would quietly stop covering that path. + * + * So this asks the opposite question: rather than "is it a literal", "do I have a value for + * it". A segment is substituted only when its NAME says it is an id or a code. That happens to + * separate the two sets exactly, including the pairs that differ by suffix alone — CARD and + * ACCOUNT are literals, CARD_ID and ACCOUNT_ID are placeholders — and it needs no maintenance + * when a new literal appears, because an unrecognised segment is left alone by default. + */ + /** True when the URL carries at least one segment concretePath would substitute. */ + def hasPlaceholder(doc: ResourceDoc): Boolean = doc.requestUrl.split("/").exists(isPlaceholder) + + private def isPlaceholder(seg: String): Boolean = + seg.nonEmpty && + seg == seg.toUpperCase && + seg.forall(c => c.isLetter || c == '_' || c.isDigit) && + // `ID` rather than `_ID`: the UK Open Banking paths spell them without the separator + // (CONSENTID, ACCOUNTID, BASKETID, DOMESTICPAYMENTID …), and leaving those literal sent + // the string "CONSENTID" to the server as though it were an id. + (seg.endsWith("ID") || seg.endsWith("_CODE") || seg.endsWith("_NAME") || + seg == "PROVIDER" || seg == "USERNAME" || seg == "USER_EMAIL" || + // Named individually because none of the three ends in ID/_CODE/_NAME, yet each is a + // genuine enumerated-value placeholder a live endpoint validates inline, not a literal: + // Http4sBGv2PIS's payment-service branches guard on + // Set("payments","bulk-payments","periodic-payments").contains(paymentService), and + // Http4s310/Http4s400's auth-context-updates and consent SCA branches guard on + // List(StrongCustomerAuthentication.SMS, EMAIL[, IMPLICIT]).contains(scaMethod). Left as + // the literal strings "PAYMENT_SERVICE"/"SCA_METHOD", both guards fail and the sweep + // reports the resulting 404/400 as the endpoint's own defect -- confirmed reproducing + // today for SCA_METHOD via OBPv5.0.0-createUserAuthContextUpdateRequest. + // PAYMENT_PRODUCT is not independently validated anywhere it appears, but is named here + // for the same reason PROVIDER/USERNAME/USER_EMAIL are: it identifies a Berlin Group + // payment product, not a literal segment, and treating it as one just because nothing + // currently checks its value the way PAYMENT_SERVICE and SCA_METHOD are checked would be + // an accident of the present call sites, not the actual contract of the segment. + seg == "PAYMENT_SERVICE" || seg == "PAYMENT_PRODUCT" || seg == "SCA_METHOD") + + // Checked against Http4sSupport's literalAllCapsSegments: not one of the sixteen ends in ID, + // _CODE or _NAME, so the rule above separates the two sets cleanly. + // + // CARDANO, MOBILE_WALLET and ETH_SEND_TRANSACTION are the remaining literals this list does + // not carry, and they stay verbatim on purpose -- substituting over them would route to the + // wrong case entirely (or none), which is the coverage hole this whole heuristic exists to + // avoid on the literal side. + + /** + * The concrete path to call. + * + * `entities` supplies values for the identifiers the caller wants resolvable. Anything not + * named there gets a well-formed value that does not exist. + * + * The distinction matters for the 403 assertion. A ResourceDoc that declares BankNotFound and + * carries BANK_ID is validated for bank existence BEFORE its roles are checked + * (APIUtil.ResourceDoc's isNeedCheckBank), so a nonexistent bank answers 404 and the role gate + * is never reached. The sweep's first run read those 404s as missing 403s across some thirty + * endpoints; they were the entity check doing its job. Passing a real bank id is what makes + * the assertion actually about roles. + */ + def concretePath(doc: ResourceDoc, entities: Map[String, String] = Map.empty): String = { + val segments = doc.requestUrl.split("/").map { seg => + if (isPlaceholder(seg)) entities.getOrElse(seg, defaultValue(seg)) else seg + } + "/obp/" + doc.implementedInApiVersion.apiShortVersion + segments.mkString("/") + } + + /** Well-formed, and deliberately not present. */ + private def defaultValue(seg: String): String = seg match { + case "ACCOUNT_ID" | "USER_ID" => "00000000-0000-0000-0000-000000000000" + // GRANT_VIEW_ID alongside VIEW_ID, and for the same reason a real bank id is passed for the + // role assertion: an endpoint that resolves the view before it checks roles answers 404 for a + // view that does not exist, and the role gate never runs. Measured on + // createTransactionRequestFreeForm, which the sweep reported as "expected 403, got 500" -- + // two defects stacked, the endpoint's raw throw AND this placeholder never resolving. + case "VIEW_ID" | "GRANT_VIEW_ID" => "owner" + case "USER_EMAIL" => "sweep-no-such-user@example.com" + // Enumerated values a live endpoint validates inline (see isPlaceholder) -- a well-formed + // but nonexistent value here would still fail that inline check, same as an id that does not + // exist fails a lookup, so these get one of the endpoint's own accepted values instead. + case "PAYMENT_SERVICE" => "payments" + case "PAYMENT_PRODUCT" => "sepa-credit-transfers" + case "SCA_METHOD" => "SMS" + case _ => "sweep-no-such-" + seg.toLowerCase.replace('_', '-') + } +} diff --git a/obp-api/src/test/scala/code/api/sweep/EndpointCatalogTest.scala b/obp-api/src/test/scala/code/api/sweep/EndpointCatalogTest.scala new file mode 100644 index 0000000000..aae0179f01 --- /dev/null +++ b/obp-api/src/test/scala/code/api/sweep/EndpointCatalogTest.scala @@ -0,0 +1,87 @@ +package code.api.sweep + +import code.setup.ServerSetupWithTestData +import org.scalatest.Tag + +/** + * EndpointCatalog.isPlaceholder decides which ALL_CAPS URL segments the sweeps substitute with a + * concrete value before calling an endpoint. Getting this wrong in either direction breaks a + * sweep's verdict: substituting a real literal sends a value the endpoint does not recognise, and + * leaving a real placeholder unsubstituted does the same thing from the other side -- the sweep + * calls a well-formed request that was never going to work, and reports the resulting 4xx/5xx as + * the endpoint's own defect. + * + * This pins the second failure mode for the three segments EndpointCatalog's own comment named as + * a known, accepted gap: PAYMENT_SERVICE, PAYMENT_PRODUCT and SCA_METHOD are enumerated values a + * live endpoint validates inline, not literals -- Http4sBGv2PIS's payment-service branches guard + * on `Set("payments", "bulk-payments", "periodic-payments").contains(paymentService)`, and + * Http4s310's auth-context-updates branch guards on + * `List(StrongCustomerAuthentication.SMS, EMAIL).contains(scaMethod)`. Left as the literal + * strings "PAYMENT_SERVICE"/"SCA_METHOD", both guards fail and the sweep calls a URL that was + * never going to route anywhere -- exactly the class of self-inflicted failure EndpointCatalog's + * own docstring says the ID/_CODE/_NAME heuristic exists to avoid. + */ +class EndpointCatalogTest extends ServerSetupWithTestData { + + object EndpointCatalogPlaceholders extends Tag("EndpointCatalogPlaceholders") + + feature("EndpointCatalog substitutes every real path placeholder, not just the ones ending in ID/_CODE/_NAME") { + + scenario("PAYMENT_SERVICE is substituted with a value the endpoint's own guard would accept", + EndpointCatalogPlaceholders) { + // No ResourceDoc in the current EndpointCatalog carries a PAYMENT_SERVICE segment -- + // Berlin Group's route trees are not aggregated into Http4s700.allResourceDocs (only the + // OBP v1.2.1..v7.0.0 lineage is), so this exercises concretePath/isPlaceholder directly via + // .copy on a real doc rather than filtering the live catalog for one that is not there. + // That pins the behaviour EndpointCatalog must have the day Berlin Group docs do join the + // catalog, instead of waiting to notice the gap then. + val doc = EndpointCatalog.all.head.copy( + requestUrl = "/PAYMENT_SERVICE/PAYMENT_PRODUCT/PAYMENT_ID/status") + val path = EndpointCatalog.concretePath(doc) + withClue(s"($path) left PAYMENT_SERVICE unsubstituted -- Http4sBGv2PIS's payment-status " + + s"branch guards on Set(\"payments\",\"bulk-payments\",\"periodic-payments\")" + + s".contains(paymentService), so this literal fails it and the sweep would " + + s"misreport a working endpoint as broken: ") { + path should not include "PAYMENT_SERVICE" + } + } + + scenario("SCA_METHOD is substituted with a value the endpoint's own guard accepts", + EndpointCatalogPlaceholders) { + val docs = EndpointCatalog.all.filter(_.requestUrl.contains("SCA_METHOD")) + withClue("no ResourceDoc in the catalog carries a SCA_METHOD segment any more -- this " + + "test's premise no longer holds against the current catalog, update it: ") { + docs should not be empty + } + docs.foreach { doc => + val path = EndpointCatalog.concretePath(doc) + withClue(s"${doc.operationId} ($path) left SCA_METHOD unsubstituted -- the endpoint " + + s"only accepts SMS/EMAIL/IMPLICIT, so this literal fails validation and the " + + s"sweep misreports a working endpoint as broken: ") { + path should not include "SCA_METHOD" + } + } + } + + scenario("literal ALL_CAPS segments that are not placeholders stay untouched", + EndpointCatalogPlaceholders) { + // The fix must not turn every unrecognised ALL_CAPS segment into a placeholder -- only the + // three named above. CARDANO/MOBILE_WALLET/ETH_SEND_TRANSACTION are genuine literals this + // catalog must keep sending verbatim. + val literalSegments = List("CARDANO", "MOBILE_WALLET", "ETH_SEND_TRANSACTION") + val docs = EndpointCatalog.all.filter(doc => literalSegments.exists(doc.requestUrl.contains)) + withClue("no ResourceDoc in the catalog carries any of CARDANO/MOBILE_WALLET/" + + "ETH_SEND_TRANSACTION any more -- this test's premise no longer holds, update it: ") { + docs should not be empty + } + docs.foreach { doc => + val path = EndpointCatalog.concretePath(doc) + val literalInDoc = literalSegments.find(doc.requestUrl.contains).get + withClue(s"${doc.operationId} substituted over the literal $literalInDoc, which routes " + + s"nowhere -- these are not placeholders: ") { + path should include(literalInDoc) + } + } + } + } +} diff --git a/obp-api/src/test/scala/code/api/sweep/FailureSweepTest.scala b/obp-api/src/test/scala/code/api/sweep/FailureSweepTest.scala new file mode 100644 index 0000000000..b941e1082e --- /dev/null +++ b/obp-api/src/test/scala/code/api/sweep/FailureSweepTest.scala @@ -0,0 +1,177 @@ +package code.api.sweep + +import cats.effect.IO +import cats.effect.unsafe.IORuntime +import code.api.util.APIUtil.ResourceDoc +import code.api.util.CustomJsonFormats +import code.api.util.http4s.Http4sApp +import code.setup.{DefaultUsers, ServerSetupWithTestData} +import fs2.Stream +import org.http4s.{Header, Headers, Method, Request, Uri} +import org.json4s.Extraction +import org.json4s.JValue +import org.json4s.JsonAST.JObject +import org.json4s.native.JsonMethods.{compact, render} +import org.scalatest.Tag +import org.typelevel.ci.CIString +import com.openbankproject.commons.util.JsonAliases.parse + +/** + * No endpoint answers a well-formed request with a 500. + * + * The auth sweep proves an endpoint refuses the wrong caller. This one proves it survives the + * right caller asking for something that is not there — which is the far more common shape of a + * production incident, and the one a migration is most likely to introduce. A connector that + * returns a slightly different empty value, a JSON codec that stops handling a null, a column + * read that no longer tolerates NULL: none of those show up as a wrong answer, they show up as + * a 500 on a request that used to work. + * + * That class of defect has a track record on this codebase. The first run of AuthSweepTest found + * `createTransactionRequestFreeForm` answering 500 to a nonexistent view id where it should have + * answered 403 or 404, and it found it by accident — the sweep was looking for something else. + * This suite looks for it on purpose, across every endpoint. + * + * ── What is sent ── + * + * A caller holding EVERY role, so nothing stops at the authorisation gate; path identifiers that + * are well-formed and do not exist; and, for verbs that take one, the endpoint's own + * `exampleRequestBody` serialised through the same json4s path the server uses to publish it. + * + * The example body is the right instrument here for the reason it is the wrong one for a success + * test: it is structurally valid and referentially meaningless. Its bank ids, currencies and + * entity references are illustrative, so an endpoint accepting it will almost always answer 400 + * or 404 — which is exactly the input that exercises the error paths, and exactly where an + * unhandled null or an over-narrow match arm turns into a 500. + * + * ── What is asserted ── + * + * 4xx fine, whatever the code. "Not found", "bad request", "not allowed" are all correct + * answers to a request for something that does not exist. + * 2xx fine. Some endpoints legitimately succeed with no arguments, or answer an empty list. + * 5xx a finding, always. + * + * Nothing here asserts WHICH 4xx. That would be a contract test, and the contract suite already + * owns it; asserting it twice, from a place with no baseline to compare against, would produce + * failures every time a message was reworded. + */ +object FailureSweepTest { + + /** + * The single definition of what this sweep covers. Exposed so SweepCoverageTest's drift check + * reads this directly instead of re-deriving its own copy of the same filter -- two copies of + * one expression are equal by construction and can never catch this sweep's own filtering + * changing independently of AuthSweepTest's. + */ + def scope: List[ResourceDoc] = EndpointCatalog.all.filter(EndpointCatalog.skipReason(_).isEmpty) +} + +class FailureSweepTest extends ServerSetupWithTestData with DefaultUsers with SweepFixtures { + + object FailureSweep extends Tag("FailureSweep") + + implicit val runtime: IORuntime = IORuntime.global + private lazy val app = Http4sApp.httpApp + + private def entities: Map[String, String] = + realBankId.map("BANK_ID" -> _).toList.toMap + + private def call(verb: String, path: String, headers: Map[String, String], body: String) + : (Int, JValue) = { + val method = Method.fromString(verb.toUpperCase).getOrElse(Method.GET) + val hdrs = if (body.nonEmpty) headers + ("Content-Type" -> "application/json") else headers + val req = Request[IO]( + method = method, + uri = Uri.unsafeFromString(path), + headers = Headers(hdrs.map { case (k, v) => Header.Raw(CIString(k), v) }.toList), + body = if (body.nonEmpty) Stream.emits(body.getBytes("UTF-8")).covary[IO] else Stream.empty + ) + val resp = app.run(req).unsafeRunSync() + val bodyStr = resp.bodyText.compile.string.unsafeRunSync() + val json = try { if (bodyStr.trim.isEmpty) JObject(Nil) else parse(bodyStr) } + catch { case _: Exception => JObject(Nil) } + (resp.status.code, json) + } + + /** + * The doc's own example body as JSON, or "" when it has none. + * + * Extraction.decompose under CustomJsonFormats is the same route Http4s uses to publish these + * objects, so what is sent is what the documentation shows a caller to send. + */ + private def exampleBody(doc: ResourceDoc): String = { + implicit val formats = CustomJsonFormats.formats + doc.exampleRequestBody match { + case null => "" + case body => + try compact(render(Extraction.decompose(body))) catch { case _: Exception => "" } + } + } + + /** + * Endpoints whose 5xx is the answer they are built to give. + * + * Exactly one so far, and it is not a real endpoint: Http4s700 registers + * `POST /obp/v7.0.0/test/rollback-check` inside `if (Props.testMode)` specifically to abort a + * transaction and prove the rollback happened, so a 500 is its pass condition. It exists only + * under `run.mode=test`, which is to say only where this sweep runs. + * + * Kept as a named map rather than removed from the catalog: SweepCoverageTest counts what the + * sweeps cover, and an endpoint quietly dropped from a list is the failure mode that whole + * test exists to prevent. Anything added here needs the same kind of reason. + */ + private val expected5xx: Map[String, String] = Map( + "OBPv7.0.0-testRollbackEndpoint" -> ("test-mode-only endpoint that deliberately throws to " + + "verify transaction rollback; its 500 IS the assertion (Http4s700, Props.testMode)"), + "OBPv7.0.0-createTestEmail" -> ("500 OBP-10056: refuses to send because portal_external_url " + + "is unset, which is true of any test rig. Environmental -- but a missing configuration is " + + "a 503, not a 500, so the status itself is logged in REGRESSION-GAPS rather than accepted " + + "as correct") + ) + + private lazy val inScope: List[ResourceDoc] = FailureSweepTest.scope + + private def check(doc: ResourceDoc, headers: Map[String, String], + ents: Map[String, String]): Option[String] = { + val path = EndpointCatalog.concretePath(doc, ents) + val body = if (doc.requestVerb.toUpperCase == "GET" || doc.requestVerb.toUpperCase == "DELETE") + "" else exampleBody(doc) + val (status, json) = call(doc.requestVerb, path, headers, body) + if (status >= 500 && !expected5xx.contains(doc.operationId)) { + implicit val formats = CustomJsonFormats.formats + val msg = (json \ "message").extractOpt[String].getOrElse("") + Some(s"${doc.operationId} ${doc.requestVerb} $path -> HTTP $status: $msg") + } else None + } + + private lazy val byVersion: Map[String, List[ResourceDoc]] = + inScope.groupBy(_.implementedInApiVersion.toString) + + feature("No endpoint answers a well-formed request with a server error") { + + byVersion.keys.toList.sorted.foreach { version => + scenario(s"$version -- a fully-entitled caller asking for something absent gets 4xx, never 5xx", + FailureSweep) { + setPropsValues("api_disabled_endpoints" -> "[]", "api_enabled_endpoints" -> "[]") + // Grant once per scenario, not once per class: beforeEach wipes the entitlement + // table, so a lazy val granted during the first scenario leaves every later one + // calling as an unentitled user -- which stops at 403 and never reaches the code + // that might crash. That is how the first run reported only two 5xx. + val headers = omniscientCaller + val ents = entities + val docs = byVersion(version) + + When(s"each of the ${docs.size} $version endpoints is called with valid credentials, " + + s"every role, a nonexistent id and its own example body") + val failures = docs.flatMap(check(_, headers, ents)) + + Then("none of them crashes") + withClue(s"${failures.size} of ${docs.size} $version endpoints answered 5xx. A request " + + s"for something that does not exist is an ordinary 404; a 500 means an " + + s"unhandled path, and it is the shape a migration introduces most often:\n" + + s"${failures.mkString("\n")}\n") { + failures shouldBe empty + } + } + } + } +} diff --git a/obp-api/src/test/scala/code/api/sweep/SuccessSweepTest.scala b/obp-api/src/test/scala/code/api/sweep/SuccessSweepTest.scala new file mode 100644 index 0000000000..66502a2349 --- /dev/null +++ b/obp-api/src/test/scala/code/api/sweep/SuccessSweepTest.scala @@ -0,0 +1,177 @@ +package code.api.sweep + +import cats.effect.IO +import cats.effect.unsafe.IORuntime +import code.api.util.APIUtil.ResourceDoc +import code.api.util.CustomJsonFormats +import code.api.util.http4s.Http4sApp +import code.setup.{DefaultUsers, ServerSetupWithTestData} +import fs2.Stream +import org.http4s.{Header, Headers, Method, Request, Uri} +import org.json4s.JValue +import org.json4s.JsonAST.JObject +import org.scalatest.Tag +import org.typelevel.ci.CIString +import com.openbankproject.commons.util.JsonAliases.parse + +/** + * The endpoints that need nothing but a caller actually answer. + * + * The other two sweeps assert what happens when something is wrong: no credentials, no role, no + * such entity. Neither would notice an endpoint that had stopped returning data altogether — a + * GET that answers 404 for every caller passes the failure sweep, which only objects to 5xx. + * This one closes that: for the endpoints where a correct answer requires no setup at all, a + * fully-entitled caller must get one. + * + * ── Why only the no-path-variable endpoints ── + * + * Of the endpoints a caller can reach, roughly a third carry no ALL_CAPS placeholder in their + * URL — /banks, /users/current, /my/accounts, /management/metrics and so on. For those, "call it + * and expect an answer" is a complete test: there is no entity to create first, so a non-answer + * is the endpoint's own fault. + * + * The remaining two thirds are deliberately out of scope here. About half of them reference an + * identifier the fixtures do supply (BANK_ID, ACCOUNT_ID, VIEW_ID …) and the other half + * reference one nothing creates — CHAT_ROOM_ID, CUSTOMER_ID, CONSENT_ID and about twenty more. + * Both need per-cluster setup to be worth asserting, and a sweep that fabricated ids for them + * would be asserting 404-handling under a name that promises success. They are the next two + * waves, not this one. + * + * ── GET only ── + * + * The 116 no-variable POSTs are excluded because a generic success POST is not a thing: the + * example bodies are illustrative rather than referentially valid (which is exactly what makes + * them good failure-path input, see FailureSweepTest), and a POST that did succeed would leave + * a row behind that the next scenario's fixture reset may or may not clear. A write-path success + * sweep needs per-endpoint bodies and per-endpoint cleanup; that is Wave 3b's problem. + * + * ── What "an answer" means ── + * + * 2xx. Not a shape, not a field — the contract suite owns field-level assertions and has a + * baseline to compare against, which this does not. Asserting shape here would duplicate that + * work from a worse position and fail every time a message was reworded. + * + * Endpoints that legitimately cannot answer 2xx on a fixture database — because they need a + * connector this build does not have, or a feature the props disable — are listed in + * `expectedNon2xx` with the reason, and asserted to STILL not be 5xx. An endpoint that stops + * answering is a finding; an endpoint that was never going to answer here is a documented skip. + */ +class SuccessSweepTest extends ServerSetupWithTestData with DefaultUsers with SweepFixtures { + + object SuccessSweep extends Tag("SuccessSweep") + + implicit val runtime: IORuntime = IORuntime.global + private lazy val app = Http4sApp.httpApp + + /** + * Endpoints that answer non-2xx on a fixture database for a stated reason. + * + * Every entry is a claim that the non-answer is environmental, not a defect. They are still + * called, and still required not to crash — the skip is only from the 2xx assertion. Keeping + * them here rather than filtering them out of the catalog means SweepCoverageTest still counts + * them, and means each exemption has to be written down next to its reason. + */ + private val expectedNon2xx: Map[String, String] = Map( + // ── needs an external service this build does not run ── + "OBPv2.0.0-elasticSearchMetrics" -> "404: needs an Elasticsearch instance; none in the test rig", + "OBPv2.0.0-elasticSearchWarehouse" -> "404: needs an Elasticsearch instance; none in the test rig", + "OBPv2.2.0-getMessageDocs" -> ("400 OBP-30211: asks which connector's message docs to " + + "return; the fixture rig runs `mapped`, which publishes none"), + "OBPv3.1.0-getObpConnectorLoopback" -> "400 OBP-10010: not implemented by the mapped connector", + "OBPv6.0.0-getMessageDocsJsonSchema" -> "same as getMessageDocs -- no connector message docs to derive a schema from", + + // ── needs a certificate the test caller does not present ── + "OBPv5.1.0-mtlsClientCertificateInfo" -> ("400 OBP-20300: reports the caller's client " + + "certificate; these requests are driven in-process with no TLS peer"), + "OBPv4.0.0-verifyRequestSignResponse" -> ("401 OBP-20311: authenticates by JWS request " + + "signature rather than by session; the sweep signs nothing"), + + // ── the URL names an entity, in a segment shaped like a literal ── + // These carry ALL_CAPS segments that EndpointCatalog deliberately leaves verbatim + // (API_COLLECTION_NAME, WEBUI_PROP_NAME, SCHEME), so the server correctly reports that no + // such entity exists. Creating one first is Wave 3c's job, not this sweep's. + "OBPv4.0.0-getMyApiCollectionByName" -> "400 OBP-30079: no ApiCollection named API_COLLECTION_NAME", + "OBPv4.0.0-getMyApiCollectionEndpoints" -> "400 OBP-30079: no ApiCollection named API_COLLECTION_NAME", + "OBPv6.0.0-getWebUiProp" -> "400 OBP-08003: no WebUi prop named WEBUI_PROP_NAME", + "OBPv7.0.0-getRoutingScheme" -> "404 OBP-30514: no routing scheme named SCHEME" + ) + + private def get(path: String, headers: Map[String, String]): (Int, JValue) = { + val req = Request[IO]( + method = Method.GET, + uri = Uri.unsafeFromString(path), + headers = Headers(headers.map { case (k, v) => Header.Raw(CIString(k), v) }.toList), + body = Stream.empty + ) + val resp = app.run(req).unsafeRunSync() + val bodyStr = resp.bodyText.compile.string.unsafeRunSync() + val json = try { if (bodyStr.trim.isEmpty) JObject(Nil) else parse(bodyStr) } + catch { case _: Exception => JObject(Nil) } + (resp.status.code, json) + } + + /** + * No ALL_CAPS placeholder in the URL -- nothing to create before calling. + * + * EndpointCatalog.hasPlaceholder, not a local copy of the rule. This used to hold its own, + * and the two had already drifted: the catalog substitutes any segment ending in ID, _CODE or + * _NAME, this one looked for _ID and _CODE and had never learnt about _NAME. So + * `/signal/channels/CHANNEL_NAME/info` was a placeholder to the catalog -- which duly replaced + * it with a channel that does not exist -- and NOT a placeholder here, so this suite selected + * it as an endpoint that "needs nothing created first" and then failed it for answering 404. + * + * The first half of the old condition was worse than wrong, it was vacuous: + * `concretePath(doc) == concretePath(doc, Map.empty)` compares a default argument with the same + * value passed explicitly, so it is true for every doc and filtered nothing. + */ + private def hasNoPathVariable(doc: ResourceDoc): Boolean = !EndpointCatalog.hasPlaceholder(doc) + + private lazy val inScope: List[ResourceDoc] = + EndpointCatalog.all + .filter(EndpointCatalog.skipReason(_).isEmpty) + .filter(_.requestVerb.toUpperCase == "GET") + .filter(hasNoPathVariable) + + private lazy val byVersion: Map[String, List[ResourceDoc]] = + inScope.groupBy(_.implementedInApiVersion.toString) + + private def check(doc: ResourceDoc, headers: Map[String, String]): Option[String] = { + val path = EndpointCatalog.concretePath(doc) + val (status, json) = get(path, headers) + implicit val formats = CustomJsonFormats.formats + lazy val msg = (json \ "message").extractOpt[String].getOrElse("") + + if (status >= 500) + Some(s"${doc.operationId} GET $path -> HTTP $status (crash): $msg") + else if (status >= 200 && status < 300) + None + else expectedNon2xx.get(doc.operationId) match { + case Some(_) => None // documented environmental non-answer; not crashing is enough + case None => Some(s"${doc.operationId} GET $path -> HTTP $status: $msg") + } + } + + feature("Endpoints that require no setup answer a fully-entitled caller") { + + byVersion.keys.toList.sorted.foreach { version => + scenario(s"$version -- every no-argument GET returns data", SuccessSweep) { + setPropsValues("api_disabled_endpoints" -> "[]", "api_enabled_endpoints" -> "[]") + // Once per scenario -- beforeEach wipes the entitlement table, so a class-level + // lazy val would leave every scenario after the first calling without roles. + val headers = omniscientCaller + val docs = byVersion(version) + + When(s"each of the ${docs.size} $version GETs that need no path variable is called") + val failures = docs.flatMap(check(_, headers)) + + Then("each one answers") + withClue(s"${failures.size} of ${docs.size} $version no-argument GETs did not answer. " + + s"These need nothing created first, so a non-2xx is the endpoint's own. If one " + + s"of them cannot answer on a fixture database, add it to expectedNon2xx with " + + s"the reason rather than deleting the assertion:\n${failures.mkString("\n")}\n") { + failures shouldBe empty + } + } + } + } +} diff --git a/obp-api/src/test/scala/code/api/sweep/SweepCoverageDriftCheckTest.scala b/obp-api/src/test/scala/code/api/sweep/SweepCoverageDriftCheckTest.scala new file mode 100644 index 0000000000..8d0c37a4b1 --- /dev/null +++ b/obp-api/src/test/scala/code/api/sweep/SweepCoverageDriftCheckTest.scala @@ -0,0 +1,72 @@ +package code.api.sweep + +import org.scalatest.{FlatSpec, Matchers} + +import java.io.File +import scala.io.Source + +/** + * Guards SweepCoverageTest's "the failure sweep covers the same set the auth sweep does" + * scenario against being vacuous. + * + * That scenario computes `authScope` and `failureScope` and asserts their symmetric difference + * is empty -- but if both are built by literally re-typing the same filter expression twice + * (`catalog.filter(EndpointCatalog.skipReason(_).isEmpty).map(_.operationId).toSet`, written out + * in SweepCoverageTest.scala rather than read from AuthSweepTest/FailureSweepTest themselves), + * the two values are equal BY CONSTRUCTION regardless of what those two sweeps actually iterate + * over. The assertion can then never fail, even after a real future divergence -- one sweep + * gains a filter of its own, and the endpoints that fall between them are covered by neither, + * silently, forever, because the "guard" was checking two copies of itself. + * + * The fix is for SweepCoverageTest to read AuthSweepTest.scope and FailureSweepTest.scope -- + * each sweep's own single definition of what it covers -- instead of re-deriving a copy. This + * test is a source scan rather than a runtime assertion because a value-equality check on the + * CURRENT catalog cannot distinguish "computed from the real source" from "coincidentally equal + * duplicate" -- both produce the identical Set today; the difference only matters for whether a + * FUTURE divergence gets caught, which a static duplicate can never do regardless of what the + * catalog looks like when the test runs. + */ +class SweepCoverageDriftCheckTest extends FlatSpec with Matchers { + + private def sourceOf(basename: String): String = { + val candidates = List( + new File(s"src/test/scala/code/api/sweep/$basename"), + new File(s"obp-api/src/test/scala/code/api/sweep/$basename") + ) + val file = candidates.find(_.isFile).getOrElse( + fail(s"Cannot locate $basename under either candidate path - this guard must not pass by " + + s"failing to look. Tried: ${candidates.mkString(", ")}")) + val source = Source.fromFile(file, "UTF-8") + try source.mkString finally source.close() + } + + private lazy val sweepCoverageSource = sourceOf("SweepCoverageTest.scala") + + "the drift-check scenario" should "read AuthSweepTest's own scope, not a re-derived copy" in { + withClue("SweepCoverageTest must reference AuthSweepTest.scope so a future change to " + + "AuthSweepTest's own filtering is automatically reflected here instead of silently " + + "diverging from a hand-copied duplicate: ") { + sweepCoverageSource should include("AuthSweepTest.scope") + } + } + + it should "read FailureSweepTest's own scope, not a re-derived copy" in { + withClue("SweepCoverageTest must reference FailureSweepTest.scope for the same reason: ") { + sweepCoverageSource should include("FailureSweepTest.scope") + } + } + + it should "not compute authScope/failureScope by writing the skipReason filter out twice" in { + val duplicateFilterPattern = + """catalog\.filter\(EndpointCatalog\.skipReason\(_\)\.isEmpty\)\.map\(_\.operationId\)\.toSet""".r + val occurrences = duplicateFilterPattern.findAllIn(sweepCoverageSource).length + withClue(s"found $occurrences occurrence(s) of the raw filter expression written directly " + + s"in SweepCoverageTest.scala. Two occurrences means authScope and failureScope are " + + s"each an independent copy of the same literal, equal by construction regardless of " + + s"what AuthSweepTest/FailureSweepTest actually cover -- the exact vacuousness this " + + s"guard exists to catch. Expected zero: the scopes should come from " + + s"AuthSweepTest.scope / FailureSweepTest.scope instead. ") { + occurrences shouldBe 0 + } + } +} diff --git a/obp-api/src/test/scala/code/api/sweep/SweepCoverageTest.scala b/obp-api/src/test/scala/code/api/sweep/SweepCoverageTest.scala new file mode 100644 index 0000000000..7033ab47ff --- /dev/null +++ b/obp-api/src/test/scala/code/api/sweep/SweepCoverageTest.scala @@ -0,0 +1,153 @@ +package code.api.sweep + +import code.api.util.APIUtil.ResourceDoc +import code.setup.ServerSetupWithTestData +import org.scalatest.Tag + +/** + * The sweep is complete, and stays complete. + * + * A sweep driven off a registry has one failure mode that matters and that the sweep itself + * cannot see: an endpoint quietly leaving the swept set. Whether that happens because someone + * adds a skip, because a doc grows a tag, or because a filter is written slightly wrong, the + * result looks identical from outside — the sweep passes, and it passes over less than it did + * yesterday. This is the same shape as `run_tests_parallel.sh`'s "fewer than 2000 tests ran" + * floor and the contract suite's MIN_ATTEMPTED, moved inside the suite: prove the denominator + * before believing the numerator. + * + * The identity is exact, not a threshold: + * + * |catalog| == |swept| + |skipped| + * + * and every member of `skipped` carries one of the three reasons enumerated in EndpointCatalog. + * There is no fourth bucket. A sweep that wants to exclude something has to add a SkipReason + * with a written justification, in the file this test reads — which is the point. + */ +class SweepCoverageTest extends ServerSetupWithTestData { + + object SweepCoverage extends Tag("SweepCoverage") + + private lazy val catalog: List[ResourceDoc] = EndpointCatalog.all + + feature("The endpoint sweep covers every reachable endpoint, or says why not") { + + scenario("the catalog is non-empty and deduplicated by (url, verb)", SweepCoverage) { + Given("the aggregated v7.0.0 resource docs") + // A sweep over an empty catalog passes every assertion it makes. The floor is deliberately + // well below the ~870 observed on this branch: its job is to catch a catalog that failed to + // initialise, not to freeze a number that legitimately grows with every new endpoint. + withClue(s"catalog holds ${catalog.size} endpoints -- far below the expected several " + + s"hundred, which means the registry did not initialise and every sweep in this " + + s"package is asserting nothing. ") { + catalog.size should be > 400 + } + + Then("no (requestUrl, requestVerb) appears twice") + val duplicated = catalog + .groupBy(d => (d.requestUrl, d.requestVerb)) + .collect { case (key, docs) if docs.size > 1 => s"$key -> ${docs.map(_.operationId).mkString(", ")}" } + withClue(s"allResourceDocs is supposed to keep only the newest version of each " + + s"(url, verb); these survived twice:\n${duplicated.mkString("\n")}\n") { + duplicated shouldBe empty + } + } + + scenario("every endpoint is either swept or skipped for a stated reason", SweepCoverage) { + Given(s"the ${catalog.size} endpoints in the catalog") + val (skipped, swept) = catalog.partition(EndpointCatalog.skipReason(_).isDefined) + + Then("the two sets account for the catalog exactly, with nothing in between") + withClue(s"swept=${swept.size} skipped=${skipped.size} catalog=${catalog.size} -- these " + + s"must add up, or some endpoint is in a third bucket nobody is looking at. ") { + swept.size + skipped.size shouldBe catalog.size + } + + And("every skip names one of the enumerated reasons") + val unexplained = skipped.filter(EndpointCatalog.skipReason(_).isEmpty) + unexplained shouldBe empty + + And("the swept set is the large majority -- a skip list that has grown to swallow the " + + "catalog is a sweep that has stopped working") + withClue(s"only ${swept.size} of ${catalog.size} endpoints are swept; skips by reason: " + + s"${skipped.groupBy(EndpointCatalog.skipReason(_).get.why).view.mapValues(_.size).toMap}. ") { + swept.size should be > (catalog.size / 2) + } + } + + scenario("the auth classification is total -- every swept endpoint is public or protected", SweepCoverage) { + val swept = catalog.filter(EndpointCatalog.skipReason(_).isEmpty) + val protectedCount = swept.count(EndpointCatalog.needsAuthentication) + val publicCount = swept.size - protectedCount + + Then("the two classes partition the swept set") + protectedCount + publicCount shouldBe swept.size + + And("both classes are non-empty -- a classifier that answers the same for everything is " + + "not classifying") + withClue(s"protected=$protectedCount public=$publicCount. If either is zero the predicate " + + s"has stopped discriminating and both AuthSweepTest branches are vacuous. ") { + protectedCount should be > 0 + publicCount should be > 0 + } + } + + scenario("the failure sweep covers the same set the auth sweep does", SweepCoverage) { + // Read each sweep's OWN scope rather than re-deriving a copy here: today both are + // `EndpointCatalog.all.filter(EndpointCatalog.skipReason(_).isEmpty)`, so two independently + // hand-typed copies of that expression would be equal by construction and this scenario + // would pass even after a real future divergence -- one sweep grows a filter of its own, + // the endpoints that fall between the two are covered by neither, and nothing here would + // notice. Reading AuthSweepTest.scope / FailureSweepTest.scope means there is exactly one + // definition of each sweep's coverage, so a change to either is automatically reflected + // on both sides of this comparison. + val authScope = AuthSweepTest.scope.map(_.operationId).toSet + val failureScope = FailureSweepTest.scope.map(_.operationId).toSet + + val onlyAuth = authScope -- failureScope + val onlyFailure = failureScope -- authScope + withClue(s"endpoints swept for auth but not for crashes: ${onlyAuth.take(10).mkString(", ")}; " + + s"the reverse: ${onlyFailure.take(10).mkString(", ")}. ") { + onlyAuth shouldBe empty + onlyFailure shouldBe empty + } + } + + scenario("enough endpoints carry an example body for the failure sweep to exercise writers", + SweepCoverage) { + // FailureSweepTest sends exampleRequestBody to every non-GET endpoint. If almost none of + // them had one, the sweep would be a GET-only crash test wearing a broader name -- it + // would pass while every write path went unexercised. + val writers = catalog + .filter(EndpointCatalog.skipReason(_).isEmpty) + .filterNot(d => d.requestVerb.toUpperCase == "GET" || d.requestVerb.toUpperCase == "DELETE") + val withBody = writers.count(_.exampleRequestBody != null) + + withClue(s"$withBody of ${writers.size} write endpoints carry an exampleRequestBody. " + + s"Below half and the failure sweep is mostly not sending bodies at all. ") { + writers.size should be > 100 + withBody should be > (writers.size / 2) + } + } + + scenario("role-gated endpoints declare the errors their gate produces", SweepCoverage) { + val roleGated = catalog + .filter(EndpointCatalog.isRoleGated) + .filter(EndpointCatalog.roleSkipReason(_).isEmpty) + + Then("each one is also classified as needing authentication") + // The middleware derives auth from `errorResponseBodies contains AuthenticatedUserIsRequired + // OR roles.nonEmpty`, so a role-gated endpoint is authenticated by construction. If this + // ever fails, the predicate and the middleware have diverged. + val notAuthenticated = roleGated.filterNot(EndpointCatalog.needsAuthentication) + withClue(s"role-gated but not classified as needing authentication: " + + s"${notAuthenticated.map(_.operationId).mkString(", ")}. ") { + notAuthenticated shouldBe empty + } + + And("there are enough of them for the 403 sweep to be meaningful") + withClue(s"only ${roleGated.size} role-gated endpoints are in scope for the 403 assertion. ") { + roleGated.size should be > 100 + } + } + } +} diff --git a/obp-api/src/test/scala/code/api/sweep/SweepFixtures.scala b/obp-api/src/test/scala/code/api/sweep/SweepFixtures.scala new file mode 100644 index 0000000000..c029f0bf56 --- /dev/null +++ b/obp-api/src/test/scala/code/api/sweep/SweepFixtures.scala @@ -0,0 +1,41 @@ +package code.api.sweep + +import code.api.util.ApiRole +import code.entitlement.Entitlement +import code.setup.DefaultUsers + +/** + * Shared setup for sweeps that call the API with a fully-entitled caller. + * + * FailureSweepTest and SuccessSweepTest each grew an identical "grant every role, then build a + * DirectLogin header" construction, and AuthSweepTest, FailureSweepTest and SuccessSweepTest each + * looked up the fixture bank independently. One shared definition here, called from all three, + * means a future change to either only has one site to update. + */ +trait SweepFixtures { self: DefaultUsers => + + /** The first sandbox bank the fixtures created, if any. */ + def realBankId: Option[String] = + code.bankconnectors.LocalMappedConnector.getBanksLegacy(None) + .map(_._1).getOrElse(Nil).headOption.map(_.bankId.value) + + /** + * A caller holding every role in the system. + * + * Granted directly through the Entitlement provider rather than over the API -- the same thing + * 161 existing test files do -- because the goal is to get PAST authorisation, not to test it. + */ + def omniscientCaller: Map[String, String] = { + ApiRole.availableRoles.foreach { role => + // Bank-scoped roles need a bank; system-wide ones must be granted with an empty bankId. + // valueOf throws on a name it does not recognise, and availableRoles includes dynamic + // roles whose backing entity may not exist in this database -- a grant that cannot be + // made is not a reason to abandon the other several hundred. + try { + val bankId = if (ApiRole.valueOf(role).requiresBankId) realBankId.getOrElse("") else "" + Entitlement.entitlement.vend.addEntitlement(bankId, resourceUser1.userId, role) + } catch { case _: Exception => () } + } + Map("DirectLogin" -> s"token=${token1.value}") + } +} diff --git a/obp-api/src/test/scala/code/api/sweep/SweepFixturesDuplicationTest.scala b/obp-api/src/test/scala/code/api/sweep/SweepFixturesDuplicationTest.scala new file mode 100644 index 0000000000..ae947a1ec2 --- /dev/null +++ b/obp-api/src/test/scala/code/api/sweep/SweepFixturesDuplicationTest.scala @@ -0,0 +1,69 @@ +package code.api.sweep + +import org.scalatest.{FlatSpec, Matchers} + +import java.io.File +import scala.io.Source + +/** + * Guards against the "grant every role" and "find a real bank id" constructions drifting back + * into byte-for-byte duplicates across the sweep test files. + * + * FailureSweepTest.omniscientUser and SuccessSweepTest.entitledCaller started as identical + * bodies -- grant every ApiRole to resourceUser1, then build a DirectLogin header -- and + * AuthSweepTest, FailureSweepTest and SuccessSweepTest each looked up + * LocalMappedConnector.getBanksLegacy(None) independently. None of it lived in EndpointCatalog, + * the module this package already treats as the one place shared sweep logic belongs (see + * AuthSweepTest.scope / FailureSweepTest.scope and SweepCoverageDriftCheckTest, which exists for + * exactly this reason on a different pair of definitions). + * + * A source scan, not a runtime assertion, for the same reason SweepCoverageDriftCheckTest is one: + * a value-equality check on today's fixtures cannot distinguish "computed from one shared + * definition" from "two copies that happen to still agree" -- both look identical today, and the + * difference only matters for whether a FUTURE divergence gets caught. + */ +class SweepFixturesDuplicationTest extends FlatSpec with Matchers { + + private def sourceOf(basename: String): String = { + val candidates = List( + new File(s"src/test/scala/code/api/sweep/$basename"), + new File(s"obp-api/src/test/scala/code/api/sweep/$basename") + ) + val file = candidates.find(_.isFile).getOrElse( + fail(s"Cannot locate $basename under either candidate path - this guard must not pass by " + + s"failing to look. Tried: ${candidates.mkString(", ")}")) + val source = Source.fromFile(file, "UTF-8") + try source.mkString finally source.close() + } + + "the 'grant every role, then build a DirectLogin header' construction" should + "appear once, in a shared fixture, not once per sweep file" in { + val pattern = """ApiRole\.availableRoles\.foreach""".r + val occurrences = List("SweepFixtures.scala", "FailureSweepTest.scala", "SuccessSweepTest.scala") + .map(f => f -> pattern.findAllIn(sourceOf(f)).length) + val total = occurrences.map(_._2).sum + + withClue(s"occurrences per file: ${occurrences.mkString(", ")}. Two independent copies of " + + s"the same 'grant every role' construction means a future change to how the " + + s"omniscient test caller is built (a new role category, a different bank-selection " + + s"rule) has to be applied by hand in both files, with nothing enforcing they stay " + + s"in sync. Expected exactly one, in a fixture both files call. ") { + total shouldBe 1 + } + } + + "the LocalMappedConnector.getBanksLegacy(None) bank lookup" should + "appear once, in a shared fixture, not once per sweep file" in { + val pattern = """LocalMappedConnector\.getBanksLegacy\(None\)""".r + val occurrences = List("SweepFixtures.scala", "AuthSweepTest.scala", "FailureSweepTest.scala", "SuccessSweepTest.scala") + .map(f => f -> pattern.findAllIn(sourceOf(f)).length) + val total = occurrences.map(_._2).sum + + withClue(s"occurrences per file: ${occurrences.mkString(", ")}. Three independent copies of " + + s"the same bank lookup means a future change to how the fixture bank is found has " + + s"to be applied by hand in three places, with nothing enforcing they stay in sync. " + + s"Expected exactly one, in a fixture all three files call. ") { + total shouldBe 1 + } + } +} diff --git a/obp-api/src/test/scala/code/api/util/EndpointMappingCacheInvalidationTest.scala b/obp-api/src/test/scala/code/api/util/EndpointMappingCacheInvalidationTest.scala new file mode 100644 index 0000000000..e801297e0e --- /dev/null +++ b/obp-api/src/test/scala/code/api/util/EndpointMappingCacheInvalidationTest.scala @@ -0,0 +1,59 @@ +package code.api.util + +import code.api.JedisMethod +import code.api.cache.Redis +import code.setup.RedisTestTarget +import org.scalatest.{FlatSpec, Matchers} + +/** + * `NewStyle.function.invalidateEndpointMappingCache` guards against the write-then-invalidate + * race that comes with the cache key fix in `getEndpointMappingsCached`: while CallContext was + * part of the memoize key nothing could ever hit, so a stale entry was unreachable by + * construction. Now that the cache genuinely hits, a reader that fetched the pre-write value a + * moment earlier can still complete its own cache write AFTER the immediate delete finishes, + * silently reintroducing the stale entry for the rest of `endpointMapping.cache.ttl.seconds` -- + * nothing else would clear it before the next write. + * + * Racing two real threads against real DB/Redis latency would make this test flaky by + * construction (the window it is trying to hit is exactly the thing that is nondeterministic). + * Planting a key that matches the invalidation glob directly stands in for the straggler write + * instead -- the same deterministic-simulation technique IdempotencyMiddlewareTest uses for its + * own concurrency scenarios -- and this asserts the mechanism that is supposed to catch it: a + * second, delayed delete. + */ +class EndpointMappingCacheInvalidationTest extends FlatSpec with Matchers { + + private def redis(): Unit = + RedisTestTarget.requireReachable(Redis.isRedisReady, "the endpoint-mapping cache invalidation race guard") + + "invalidateEndpointMappingCache" should "clear a straggler entry that lands after the immediate delete" in { + redis() + // Any key matching the same glob the real memoized entry would (*getEndpointMappings*) + // stands in for the straggler -- the exact scalacache-derived key shape is not what this + // guards, only that a second sweep eventually clears whatever landed in the gap. + val stragglerKey = "test_ns:code.api.util.NewStyle.function.getEndpointMappingsCached(Some(straggler))()" + Redis.use(JedisMethod.SET, stragglerKey, None, Some("[]")) + withClue("test setup failed to plant the straggler key: ") { + Redis.use(JedisMethod.GET, stragglerKey, None, None) shouldBe Some("[]") + } + + NewStyle.function.invalidateEndpointMappingCache() + + withClue("immediately after the call the straggler should already be gone once, but that " + + "alone does not prove there is a SECOND delete -- see below") { + Redis.use(JedisMethod.GET, stragglerKey, None, None) shouldBe None + } + + // Simulate the race: the straggler write lands in the gap between the immediate delete and + // the scheduled one. + Redis.use(JedisMethod.SET, stragglerKey, None, Some("[]")) + + Thread.sleep(NewStyle.function.endpointMappingCacheInvalidationDelay.toMillis + 300) + + withClue("the delayed second invalidation must still clear a straggler that landed after " + + "the first delete, or a concurrent read racing the write can leave a stale " + + "endpoint-mapping list cached for the rest of the TTL: ") { + Redis.use(JedisMethod.GET, stragglerKey, None, None) shouldBe None + } + } +} diff --git a/obp-api/src/test/scala/code/api/util/http4s/IdempotencyMiddlewareTest.scala b/obp-api/src/test/scala/code/api/util/http4s/IdempotencyMiddlewareTest.scala new file mode 100644 index 0000000000..bdc09eeab7 Binary files /dev/null and b/obp-api/src/test/scala/code/api/util/http4s/IdempotencyMiddlewareTest.scala differ diff --git a/obp-api/src/test/scala/code/api/v2_1_0/SandboxDataLoadingTest.scala b/obp-api/src/test/scala/code/api/v2_1_0/SandboxDataLoadingTest.scala index 646fdb18de..ae78699734 100644 --- a/obp-api/src/test/scala/code/api/v2_1_0/SandboxDataLoadingTest.scala +++ b/obp-api/src/test/scala/code/api/v2_1_0/SandboxDataLoadingTest.scala @@ -1082,6 +1082,53 @@ class SandboxDataLoadingTest extends FlatSpec with SendServerRequests with Match Connector.connector.vend.getBankAccountLegacy(BankId(acc1.bank), AccountId(acc2.id), None).isDefined should equal(true) } + it should "not allow two accounts at DIFFERENT banks to share an IBAN either" in { + // The global-uniqueness half of the rule, which had no test at all. + // + // An IBAN is globally unique by ISO 13616 -- the bank identifier is encoded INSIDE the + // string, so two banks cannot legitimately hold the same one. OBP does not merely assume + // that; it depends on it. LocalMappedConnector.getBankAccountByRoutingLegacy, called with + // no bankId, refuses outright when a routing address matches more than one account: + // + // if (routing.size > 1) { // Routing MUST be unique + // Failure(s"$AccountRoutingNotUnique (scheme: $scheme, address: $address)") + // + // and that is the lookup PAYMENT DESTINATIONS resolve through -- BulkPaymentHandler and + // three v7.0.0 transaction paths all call it with bankId = None. So letting a duplicate in + // at import time does not create a working account: it creates one that any global-routing + // payment then fails on, far from the import that caused it. + // + // Rejecting at import is therefore the correct behaviour, and this test exists so nobody + // "fixes" the duplicate check by scoping it per bank to make a broken fixture load. + val users = standardUsers + val banks = standardBanks + + def getResponse(accountJsons : List[JValue]) = { + BankAccountRouting.bulkDelete_!!() + val json = createImportJson(banks.map(Extraction.decompose), users.map(Extraction.decompose), accountJsons, Nil, Nil, Nil, Nil, Nil) + postImportJson(json) + } + + val accAtBank1 = account1AtBank1 + val accAtBank2 = account1AtBank2 + + val bank1Json = Extraction.decompose(accAtBank1) + // Same IBAN, different bank. Nothing else changed. + val bank2SameIbanJson = replaceField(Extraction.decompose(accAtBank2), "IBAN", accAtBank1.IBAN) + + getResponse(List(bank1Json, bank2SameIbanJson)).code should equal(FAILED) + + // And nothing partially imported. + Connector.connector.vend.getBankAccountLegacy(BankId(accAtBank1.bank), AccountId(accAtBank1.id), None).isDefined should equal(false) + Connector.connector.vend.getBankAccountLegacy(BankId(accAtBank2.bank), AccountId(accAtBank2.id), None).isDefined should equal(false) + + // The same two accounts import fine once their IBANs differ -- proving the rejection above + // was about the IBAN collision and not about anything else in the payload. + getResponse(List(bank1Json, Extraction.decompose(accAtBank2))).code should equal(SUCCESS) + Connector.connector.vend.getBankAccountLegacy(BankId(accAtBank1.bank), AccountId(accAtBank1.id), None).isDefined should equal(true) + Connector.connector.vend.getBankAccountLegacy(BankId(accAtBank2.bank), AccountId(accAtBank2.id), None).isDefined should equal(true) + } + it should "not allow an account to be created with an existing IBAN" in { val banks = standardBanks.map(Extraction.decompose) val users = standardUsers.map(Extraction.decompose) diff --git a/obp-api/src/test/scala/code/api/v4_0_0/Http4s400ViewResolutionTest.scala b/obp-api/src/test/scala/code/api/v4_0_0/Http4s400ViewResolutionTest.scala new file mode 100644 index 0000000000..de94779a04 --- /dev/null +++ b/obp-api/src/test/scala/code/api/v4_0_0/Http4s400ViewResolutionTest.scala @@ -0,0 +1,76 @@ +package code.api.v4_0_0 + +import code.api.util.CallContext +import com.openbankproject.commons.model.View +import net.liftweb.common.{Box, Empty} + +import scala.concurrent.Await +import scala.concurrent.duration._ + +/** + * `Http4s400.Implementations4_0_0.resolveCreateTransactionRequestView` decides between two + * failure shapes for `createTransactionRequest`'s view lookup: a genuinely missing view + * (client's fault, 404) and anything else the lookup throws (a connection-pool exhaustion, a + * transient SQL error, a Mapper bug -- none of them the client's fault, all of them a 500). + * + * `NewStyle.function.tryons` cannot tell these apart on its own: it catches any `Exception` the + * wrapped block raises and reports it via the caller-supplied failCode regardless of cause. If + * the whole DB lookup sits inside that block, an infra failure and a real not-found produce the + * identical JSON-encoded {"failCode":404,...} exception -- indistinguishable to + * ErrorResponseConverter, which resolves the 404 straight from that embedded field. A client + * whose retry logic reacts to 500 (retry) differently from 404 (stop) is told to stop when the + * backend is actually just broken. + * + * These tests call the production function directly with a stub `lookup`, so no live Mapper + * connection is needed to exercise the distinction -- but Implementations4_0_0's own static + * init registers the full v4.0.0 ResourceDoc set, which needs the app booted, hence + * V400ServerSetup rather than a bare unit-test base. + */ +class Http4s400ViewResolutionTest extends V400ServerSetup { + + private implicit val cc: CallContext = CallContext() + + private def resultOf[T](f: => T): Either[Throwable, T] = + try Right(f) catch { case t: Throwable => Left(t) } + + feature("createTransactionRequest's view lookup distinguishes not-found from infra failure") { + + scenario("a lookup that finds nothing fails with the JSON-encoded 404 envelope") { + val notFound: () => Box[View] = () => Empty + val outcome = resultOf(Await.result( + Http4s400.Implementations4_0_0.resolveCreateTransactionRequestView("nonexistent-view", notFound), + 5.seconds)) + + outcome match { + case Left(t) => + val msg = t.getMessage + withClue(s"expected a JSON envelope carrying failCode 404; got: $msg") { + msg should include("\"failCode\":404") + msg should include("View not found") + } + case Right(v) => + fail(s"expected the lookup to fail as not-found, but it returned $v") + } + } + + scenario("a lookup that throws for an infra reason propagates that exception, not a 404") { + val infraFailure = new RuntimeException("connection pool exhausted") + val broken: () => Box[View] = () => throw infraFailure + val outcome = resultOf(Await.result( + Http4s400.Implementations4_0_0.resolveCreateTransactionRequestView("some-view", broken), + 5.seconds)) + + outcome match { + case Left(t) => + val msg = t.getMessage + withClue(s"expected the original infra exception to propagate untouched so it resolves " + + s"to 500, not the 404 envelope reserved for a genuine not-found; got: $msg") { + msg should not include "\"failCode\":404" + msg should include("connection pool exhausted") + } + case Right(v) => + fail(s"expected the lookup's exception to propagate, but it returned $v") + } + } + } +} diff --git a/obp-api/src/test/scala/code/api/v5_1_0/Http4s510JwtSignatureResolutionTest.scala b/obp-api/src/test/scala/code/api/v5_1_0/Http4s510JwtSignatureResolutionTest.scala new file mode 100644 index 0000000000..824353aac4 --- /dev/null +++ b/obp-api/src/test/scala/code/api/v5_1_0/Http4s510JwtSignatureResolutionTest.scala @@ -0,0 +1,75 @@ +package code.api.v5_1_0 + +import code.api.util.CallContext + +import scala.concurrent.Await +import scala.concurrent.duration._ + +/** + * `Http4s510.Implementations5_1_0.resolveJwtSignatureValid` decides what a thrown exception from + * `JwtUtil.verifyJwt` means: a malformed client certificate or JWT (client's fault, 400) versus a + * JVM/security-provider configuration problem -- the requested signature algorithm is not + * registered (a hardened/FIPS JRE, a stripped provider list, a provider-registration bug). Both + * currently surface as the same `JOSEException`/generic `Exception`, and wrapping the whole call + * in `tryons(..., 400, ...)` cannot tell them apart: a security-provider fault would be reported + * to the caller as "your JSON is not signed" when nothing about their JWT is wrong -- the server + * cannot perform this verification for ANY caller until an operator fixes the JVM. + * + * These tests call the production function directly with a stub `verify`, so no real PEM/JWT + * material or JOSE library internals are needed to exercise the distinction. + */ +class Http4s510JwtSignatureResolutionTest extends V510ServerSetup { + + private implicit val cc: CallContext = CallContext() + + private def resultOf[T](f: => T): Either[Throwable, T] = + try Right(f) catch { case t: Throwable => Left(t) } + + feature("createConsumerDynamicRegistration's JWT verification distinguishes a bad client " + + "JWT from a broken security provider") { + + scenario("a verify() that returns false is a normal signature mismatch, not an error") { + val outcome = Await.result( + Http4s510.Implementations5_1_0.resolveJwtSignatureValid(() => false), 5.seconds) + outcome shouldBe false + } + + scenario("a verify() that throws for a malformed client JWT fails with the 400 envelope") { + val badJwt = new IllegalArgumentException("Invalid JWT serialization") + val outcome = resultOf(Await.result( + Http4s510.Implementations5_1_0.resolveJwtSignatureValid(() => throw badJwt), 5.seconds)) + + outcome match { + case Left(t) => + val msg = t.getMessage + withClue(s"expected a JSON envelope carrying failCode 400; got: $msg") { + msg should include("\"failCode\":400") + } + case Right(v) => + fail(s"expected verify() to fail as a bad JWT, but it returned $v") + } + } + + scenario("a verify() that throws because the JVM lacks the signature algorithm propagates " + + "that exception, not a 400") { + val providerFailure = + new com.nimbusds.jose.JOSEException("no such algorithm", + new java.security.NoSuchAlgorithmException("SHA256withRSA Signature not available")) + val outcome = resultOf(Await.result( + Http4s510.Implementations5_1_0.resolveJwtSignatureValid(() => throw providerFailure), 5.seconds)) + + outcome match { + case Left(t) => + val msg = t.getMessage + withClue(s"expected the original security-provider exception to propagate untouched " + + s"so it resolves to 500, not the 400 envelope reserved for a malformed " + + s"client JWT/certificate; got: $msg") { + msg should not include "\"failCode\":400" + msg should include("no such algorithm") + } + case Right(v) => + fail(s"expected verify()'s exception to propagate, but it returned $v") + } + } + } +} diff --git a/obp-api/src/test/scala/code/api/v6_0_0/Http4s600ResetPasswordPortalUrlTest.scala b/obp-api/src/test/scala/code/api/v6_0_0/Http4s600ResetPasswordPortalUrlTest.scala new file mode 100644 index 0000000000..edd6e63973 --- /dev/null +++ b/obp-api/src/test/scala/code/api/v6_0_0/Http4s600ResetPasswordPortalUrlTest.scala @@ -0,0 +1,52 @@ +package code.api.v6_0_0 + +import code.api.util.CallContext +import net.liftweb.common.{Empty, Full} + +import scala.concurrent.Await +import scala.concurrent.duration._ + +/** + * `Http4s600.Implementations6_0_0.resolveResetPasswordPortalUrl` reports what happens when + * `public_obp_portal_url` (or the legacy `portal_external_url`) isn't configured. That is an + * operator's configuration mistake, not a caller's -- the same condition this PR's own + * createTestEmail fix (Http4s700.scala) deliberately reports as 503, not 500, with the reasoning + * "the server is not broken -- it is not configured to do this, and [a wrong code] tells a + * caller with retry logic that the fault is transient." A missing portal URL should get the same + * treatment here: 503, not 400 -- an admin resetting a user's password should not be told their + * request was bad when the truth is nobody has configured the portal URL yet. + */ +class Http4s600ResetPasswordPortalUrlTest extends V600ServerSetup { + + private implicit val cc: CallContext = CallContext() + + private def resultOf[T](f: => T): Either[Throwable, T] = + try Right(f) catch { case t: Throwable => Left(t) } + + feature("resetPasswordUrl reports a missing portal URL as a server misconfiguration, not a client error") { + + scenario("a configured portal URL is used as-is") { + val url = Await.result( + Http4s600.Implementations6_0_0.resolveResetPasswordPortalUrl(Full("https://portal.example.com")), + 5.seconds) + url shouldBe "https://portal.example.com" + } + + scenario("an unconfigured portal URL fails with 503, not 400") { + val outcome = resultOf(Await.result( + Http4s600.Implementations6_0_0.resolveResetPasswordPortalUrl(Empty), 5.seconds)) + + outcome match { + case Left(t) => + val msg = t.getMessage + withClue(s"expected a JSON envelope carrying failCode 503 (an operator configuration " + + s"problem, not the caller's fault); got: $msg") { + msg should include("\"failCode\":503") + msg should not include "\"failCode\":400" + } + case Right(v) => + fail(s"expected the missing portal URL to fail, but it returned $v") + } + } + } +} diff --git a/obp-api/src/test/scala/code/concurrency/ConcurrentRateLimiterRaceTest.scala b/obp-api/src/test/scala/code/concurrency/ConcurrentRateLimiterRaceTest.scala index f3a3d1c205..d84a2ea045 100644 --- a/obp-api/src/test/scala/code/concurrency/ConcurrentRateLimiterRaceTest.scala +++ b/obp-api/src/test/scala/code/concurrency/ConcurrentRateLimiterRaceTest.scala @@ -2,6 +2,7 @@ package code.concurrency import code.api.JedisMethod import code.api.cache.Redis +import code.setup.RedisTestTarget import java.util.UUID import java.util.concurrent.atomic.AtomicInteger @@ -31,7 +32,7 @@ class ConcurrentRateLimiterRaceTest extends ConcurrentRaceSetup { feature("Redis-backed rate-limit and idempotency operations must be atomic") { scenario("H4: concurrent check-then-increment must not let more than `limit` callers pass the gate", ConcurrencyRace) { - assume(redisUp, "Redis not reachable — skipping H4") + RedisTestTarget.requireReachable(redisUp, "H4") Given("a rate-limit counter key with limit=5 and 20 concurrent callers") val key = "__conc_h4_rl_" + UUID.randomUUID.toString.take(8) val limit = 5L @@ -67,7 +68,7 @@ class ConcurrentRateLimiterRaceTest extends ConcurrentRaceSetup { } scenario("M6: idempotency response cache must be first-write-wins, not last-writer-wins (SET NX EX, not setex)", ConcurrencyRace) { - assume(redisUp, "Redis not reachable — skipping M6") + RedisTestTarget.requireReachable(redisUp, "M6") Given("an idempotency response key that receives two writes with different bodies") val key = "__conc_m6_rd_" + UUID.randomUUID.toString.take(8) val ttl = 60 @@ -90,7 +91,7 @@ class ConcurrentRateLimiterRaceTest extends ConcurrentRaceSetup { } scenario("M7: idempotency lock must be acquired atomically with its TTL (SET NX EX, not setnx+expire)", ConcurrencyRace) { - assume(redisUp, "Redis not reachable — skipping M7") + RedisTestTarget.requireReachable(redisUp, "M7") Given("a lock key acquired the way IdempotencyMiddleware.tryAcquireLock now does it") val key = "__conc_m7_lock_" + UUID.randomUUID.toString.take(8) val lockTtl = 60 diff --git a/obp-api/src/test/scala/code/setup/RedisTestTarget.scala b/obp-api/src/test/scala/code/setup/RedisTestTarget.scala new file mode 100644 index 0000000000..9d7ba236b1 --- /dev/null +++ b/obp-api/src/test/scala/code/setup/RedisTestTarget.scala @@ -0,0 +1,49 @@ +package code.setup + +import org.scalatest.Assertions + +/** + * Whether a Redis-dependent check may cancel itself, or has to run. + * + * `assume(Redis.isRedisReady)` cancels wherever no Redis is listening, and a cancelled check is + * indistinguishable from a passing one in every report anybody reads. and `run_tests_parallel.sh` starts none and probes for none, so the rate-limiter races + * and the cache-invalidation checks skip on every local run while reporting green. CI does declare + * a redis service, but nothing fails if that block is dropped or the container never becomes + * healthy - the suite would go back to skipping, silently, and the log line saying so is one nobody + * reads. + * + * What is lost when they skip is not incidental: these are the only checks that exercise the + * rate-limiter's Redis fast path and MethodRouting's cache invalidation under concurrency. Both are + * shared-state races, which is precisely the class of defect a green unit suite cannot rule out. + * + * `OBP_TEST_REDIS_REQUIRED=true` turns the cancellation into a failure. CI sets it alongside the + * service container; developers leave it unset and keep the skip. + */ +object RedisTestTarget { + + /** True when a missing Redis must fail rather than cancel. */ + def required: Boolean = + sys.env.get("OBP_TEST_REDIS_REQUIRED").exists(_.trim.equalsIgnoreCase("true")) + + /** + * Cancel the test when Redis is absent and optional; fail it when absent and required; return + * normally when it is there. + * + * `required` is a parameter rather than a direct read of the environment for the same reason it + * is one on PostgresTestTarget: the environment cannot be changed from inside a running JVM, so a + * branch that only an environment variable can reach is a branch no test can enter - and an + * unreachable branch in a guard is the very thing the guard exists to stop. + */ + def requireReachable(reachable: Boolean, what: String, required: Boolean = required): Unit = + if (!reachable) { + if (required) { + Assertions.fail( + s"OBP_TEST_REDIS_REQUIRED=true but Redis is not reachable, so $what cannot run. These " + + "checks are the only cover for the rate limiter's Redis path and MethodRouting cache " + + "invalidation under concurrency, so they must not be skipped where they are required - " + + "start Redis, or unset OBP_TEST_REDIS_REQUIRED to go back to skipping.") + } else { + Assertions.cancel(s"Redis not reachable - skipping $what") + } + } +} diff --git a/obp-api/src/test/scala/code/setup/RedisTestTargetTest.scala b/obp-api/src/test/scala/code/setup/RedisTestTargetTest.scala new file mode 100644 index 0000000000..3d7d374c8c --- /dev/null +++ b/obp-api/src/test/scala/code/setup/RedisTestTargetTest.scala @@ -0,0 +1,53 @@ +package code.setup + +import org.scalatest.{FlatSpec, Matchers} +import org.scalatest.exceptions.{TestCanceledException, TestFailedException} + +/** + * The gate itself, both ways. + * + * PostgresTestTargetTest exists for the same reason and states it plainly: the skip has to be + * opt-in, because a skip reports as a pass. A guard whose strict branch is never entered by any + * test is a guard nobody has checked, and this one's whole purpose is to stop exactly that shape + * of untested branch. + * + * `required` is passed explicitly rather than read from the environment, because the environment + * cannot be changed from inside a running JVM -- which is why RedisTestTarget takes it as a + * parameter in the first place. + */ +class RedisTestTargetTest extends FlatSpec with Matchers { + + /** Arbitrary label passed as `what`; only its identity across calls matters, not its text. */ + private val CheckLabel = "a check" + + "requireReachable" should "return normally when Redis is reachable, whether or not it is required" in { + noException should be thrownBy RedisTestTarget.requireReachable( + reachable = true, what = CheckLabel, required = false) + noException should be thrownBy RedisTestTarget.requireReachable( + reachable = true, what = CheckLabel, required = true) + } + + it should "cancel when Redis is absent and optional" in { + val e = intercept[TestCanceledException] { + RedisTestTarget.requireReachable(reachable = false, what = CheckLabel, required = false) + } + e.getMessage should include("Redis not reachable") + e.getMessage should include(CheckLabel) + } + + it should "FAIL, not cancel, when Redis is absent and required" in { + val e = intercept[TestFailedException] { + RedisTestTarget.requireReachable(reachable = false, what = CheckLabel, required = true) + } + e.getMessage should include("OBP_TEST_REDIS_REQUIRED=true") + e.getMessage should include(CheckLabel) + } + + "required" should "read OBP_TEST_REDIS_REQUIRED and default to false" in { + // Whatever this environment says, the value has to be a Boolean derived from that one + // variable -- and unset must mean false, so a developer machine keeps the skip. + val fromEnv = sys.env.get("OBP_TEST_REDIS_REQUIRED").exists(_.trim.equalsIgnoreCase("true")) + RedisTestTarget.required shouldBe fromEnv + if (sys.env.get("OBP_TEST_REDIS_REQUIRED").isEmpty) RedisTestTarget.required shouldBe false + } +} diff --git a/obp-api/src/test/scala/code/util/DynamicUtilTest.scala b/obp-api/src/test/scala/code/util/DynamicUtilTest.scala index c1bfd22e2b..c9be8d2452 100644 --- a/obp-api/src/test/scala/code/util/DynamicUtilTest.scala +++ b/obp-api/src/test/scala/code/util/DynamicUtilTest.scala @@ -48,6 +48,35 @@ class DynamicUtilTest extends FlatSpec with Matchers { private val securityManagerUnavailable = "SecurityManager enforcement is not available on JDK 17+ (JEP 411); skip on JDK 21" + /** + * Skip the sandbox checks when no SecurityManager can enforce them -- but let CI refuse the skip. + * + * `assume` cancels, and a cancelled check is indistinguishable from a passing one in every + * report anybody reads: this suite has shown "canceled 0" while three of its scenarios never + * ran, because ScalaTest counts a cancellation separately from a failure and the summary line + * people look at is the failure count. Since JDK 17 removed SecurityManager enforcement + * (JEP 411, completed by JEP 486), DynamicUtil.Sandbox is a no-op on any modern JDK and these + * three have been skipping on every run, everywhere, for as long as the build has been on 21+. + * + * That is a real gap, not a formality: the sandbox is what stops runtime-compiled endpoint code + * from reading the filesystem or opening sockets, and nothing else covers it. + * + * OBP_TEST_SANDBOX_REQUIRED=true turns the cancellation into a failure, the same lever + * RedisTestTarget gives the Redis-dependent checks. Set it wherever a JDK that can still + * enforce is available; leave it unset and the skip stands, but now it is a decision somebody + * made rather than a silence. + */ + private def requireSecurityManager(): Unit = + if (System.getSecurityManager == null) { + val required = sys.env.get("OBP_TEST_SANDBOX_REQUIRED").exists(_.trim.equalsIgnoreCase("true")) + if (required) + fail("OBP_TEST_SANDBOX_REQUIRED=true but no SecurityManager is installed, so the sandbox " + + "checks cannot run. They are the only cover for what runtime-compiled endpoint code " + + "is allowed to touch -- run them on a JDK that still enforces, or unset the variable " + + "to go back to skipping.") + else cancel(securityManagerUnavailable) + } + implicit val formats = code.api.util.CustomJsonFormats.formats @@ -115,14 +144,18 @@ class DynamicUtilTest extends FlatSpec with Matchers { val dependenciesString = """[NewStyle.function.getClass.getTypeName -> "*",CompiledObjects.getClass.getTypeName -> "sandbox",HttpCode.getClass.getTypeName -> "200",DynamicCompileEndpoint.getClass.getTypeName -> "getPathParams, scalaFutureToBoxedJsonResponse",APIUtil.getClass.getTypeName -> "errorJsonResponse, errorJsonResponse$default$1, errorJsonResponse$default$2, errorJsonResponse$default$3, errorJsonResponse$default$4, scalaFutureToLaFuture, futureToBoxedResponse",ErrorMessages.getClass.getTypeName -> "*",ExecutionContext.Implicits.getClass.getTypeName -> "global",JSONFactory400.getClass.getTypeName -> "createBanksJson",classOf[Sandbox].getTypeName -> "runInSandbox",classOf[CallContext].getTypeName -> "*",classOf[ResourceDoc].getTypeName -> "getPathParams","scala.reflect.runtime.package$" -> "universe",PractiseEndpoint.getClass.getTypeName + "*" -> "*"]""".stripMargin - val scalaCode2 = s"${DynamicUtil.importStatements}"+dependenciesString.replaceFirst("\\[","Map[String, String](").dropRight(1) +").mapValues(v => StringUtils.split(v, ',').map(_.trim).toSet).toMap" + // DynamicUtil.Validation.dependenciesScalaCode, not a copy of it. This line used to be a + // character-for-character duplicate of the production expression, which meant an edit to + // either one left the test green while the two disagreed -- and this is the only compile that + // happens reflectively at boot, so nothing at compile time would have noticed either. + val scalaCode2 = DynamicUtil.Validation.dependenciesScalaCode(dependenciesString) val dependenciesBox2: Box[Map[String, Set[String]]] = DynamicUtil.compileScalaCode(scalaCode2) val dependencies2 = dependenciesBox2.openOrThrowException("Can not compile the string to Map") dependencies2.toString contains ("code.api.util.NewStyle") shouldBe (true) } "Sandbox.createSandbox method" should "should throw exception" taggedAs DynamicUtilsTag in { - assume(System.getSecurityManager != null, securityManagerUnavailable) + requireSecurityManager() val permissionList = List( // new java.net.SocketPermission("ir.dcs.gla.ac.uk:80","connect,resolve"), ) @@ -147,7 +180,7 @@ class DynamicUtilTest extends FlatSpec with Matchers { } "Sandbox.sandbox method test bankId" should "should throw exception" taggedAs DynamicUtilsTag in { - assume(System.getSecurityManager != null, securityManagerUnavailable) + requireSecurityManager() intercept[AccessControlException] { Sandbox.sandbox(bankId= "abc").runInSandbox { BankId("123" ) @@ -162,7 +195,7 @@ class DynamicUtilTest extends FlatSpec with Matchers { } "Sandbox.sandbox method test default permission" should "should throw exception" taggedAs DynamicUtilsTag in { - assume(System.getSecurityManager != null, securityManagerUnavailable) + requireSecurityManager() intercept[AccessControlException] { Sandbox.sandbox(bankId= "abc").runInSandbox { scala.io.Source.fromURL("https://apisandbox.openbankproject.com/") diff --git a/obp-api/src/test/scala/code/util/RunTestsParallelScriptTest.scala b/obp-api/src/test/scala/code/util/RunTestsParallelScriptTest.scala new file mode 100644 index 0000000000..cbfb155bba --- /dev/null +++ b/obp-api/src/test/scala/code/util/RunTestsParallelScriptTest.scala @@ -0,0 +1,61 @@ +package code.util + +import org.scalatest.{FlatSpec, Matchers} + +import java.io.File +import scala.io.Source + +/** + * Pins that run_tests_parallel.sh's zero-test-floor diagnostic message quotes the same number + * it actually compares against. + * + * The floor check reads: + * + * if [[ "${SF_TOTAL:-0}" -lt 3200 ]]; then + * echo " ✗ suspicious total: only ${SF_TOTAL:-0} tests ran (< 2000 floor) ..." + * + * The threshold was raised from 2000 to 3200 (see the script's own comment: "3200 is 90% of the + * 3571 measured on develop-obp") but the message text was not updated alongside it. A run that + * produces, say, 2500 tests is correctly failed by the `-lt 3200` check, but the printed + * diagnostic reads "only 2500 tests ran (< 2000 floor)" -- which is arithmetically + * self-contradictory (2500 is not less than 2000) to whoever is reading the CI log to work out + * why the build failed. + * + * A runtime test cannot exercise a bash script's own comparison, so this reads the script's + * source and asserts the number in the `-lt` comparison matches the number quoted in the message + * -- the same drift-guard shape SweepCoverageDriftCheckTest uses for a Scala file. + */ +class RunTestsParallelScriptTest extends FlatSpec with Matchers { + + private def scriptSource: String = { + val candidates = List( + new File("run_tests_parallel.sh"), + new File("../run_tests_parallel.sh") + ) + val file = candidates.find(_.isFile).getOrElse( + fail(s"Cannot locate run_tests_parallel.sh under either candidate path - this guard must " + + s"not pass by failing to look. Tried: ${candidates.mkString(", ")}")) + val source = Source.fromFile(file, "UTF-8") + try source.mkString finally source.close() + } + + "the zero-test floor diagnostic" should "quote the same threshold it actually compares against" in { + val src = scriptSource + + val comparisonThreshold = """\$\{SF_TOTAL:-0\}"\s*-lt\s*(\d+)""".r + .findFirstMatchIn(src).map(_.group(1)).getOrElse( + fail("could not find the zero-test floor comparison (\"${SF_TOTAL:-0}\" -lt N) in the " + + "script - this guard must not pass by failing to look")) + + val messageThreshold = """\(<\s*(\d+)\s+floor\)""".r + .findFirstMatchIn(src).map(_.group(1)).getOrElse( + fail("could not find the \"(< N floor)\" diagnostic text in the script - this guard " + + "must not pass by failing to look")) + + withClue(s"the script compares against $comparisonThreshold but tells the reader the floor " + + s"is $messageThreshold -- whichever one is stale, a CI failure reads as " + + s"self-contradictory until they match: ") { + messageThreshold shouldBe comparisonThreshold + } + } +} diff --git a/run_tests_parallel.sh b/run_tests_parallel.sh index f9d94aa7c8..b37ff9e29a 100755 --- a/run_tests_parallel.sh +++ b/run_tests_parallel.sh @@ -516,7 +516,17 @@ while IFS= read -r _f; do fi _fa=$(_sf_attr "$_head" failures); _fa=${_fa:-0} _e=$(_sf_attr "$_head" errors); _e=${_e:-0} - _sk=$(_sf_attr "$_head" skipped); _sk=${_sk:-0} + # ScalaTest's JUnit XML reporter does NOT put a skipped="N" attribute on ; + # it emits a child inside each cancelled . Reading the attribute + # therefore always yielded 0, so this line reported "0 skipped/canceled" for a run in + # which DynamicUtilTest cancelled three of its nine -- and would have reported the same + # for a suite that cancelled every one of its tests. That is the number somebody checks + # precisely when they suspect tests are not running, so it has to be counted from what + # is actually in the file. Attribute first for reporters that do emit it, child elements + # otherwise. + _sk=$(_sf_attr "$_head" skipped) + if [[ -z "$_sk" ]]; then _sk=$(grep -c "/dev/null); fi + _sk=${_sk:-0} SF_TOTAL=$((SF_TOTAL+_t)); SF_FAIL=$((SF_FAIL+_fa)); SF_ERR=$((SF_ERR+_e)); SF_SKIP=$((SF_SKIP+_sk)) if [[ $_fa -ne 0 || $_e -ne 0 ]]; then SF_BAD+=("$(basename "$_f" | sed 's/^TEST-//; s/\.xml$//'): $_fa failed, $_e errors") @@ -529,10 +539,16 @@ if [[ "$SF_FAIL" != "0" ]] || [[ "$SF_ERR" != "0" ]] || [[ "$SF_BROKEN" != "0" ] OVERALL_RC=1 fi # Zero-test floor: -DfailIfNoTests=false means a broken wildcardSuites filter runs nothing -# and "passes". The suite has ~2900 tests; a total far below that means shards ran -# near-empty — fail instead of reporting a hollow green. -if [[ "${SF_TOTAL:-0}" -lt 2000 ]]; then - echo " ✗ suspicious total: only ${SF_TOTAL:-0} tests ran (< 2000 floor) — filter/discovery regression?" +# and "passes". A total far below the real one means shards ran near-empty — fail instead of +# reporting a hollow green. +# +# 3200 is 90% of the 3571 measured on develop-obp (2026-08-25, --shards=4). The previous +# figure, 2000, was set against a suite the header called "~2900" and had drifted far enough +# that a run losing a fifth of its tests would still have passed it. Re-measure and re-set +# both numbers when the suite grows: a floor that is only half the real count is barely a +# floor at all. +if [[ "${SF_TOTAL:-0}" -lt 3200 ]]; then + echo " ✗ suspicious total: only ${SF_TOTAL:-0} tests ran (< 3200 floor) — filter/discovery regression?" OVERALL_RC=1 fi