diff --git a/obp-api/src/test/scala/code/api/util/http4s/Http4sJsonContentTypeTest.scala b/obp-api/src/test/scala/code/api/util/http4s/Http4sJsonContentTypeTest.scala new file mode 100644 index 0000000000..a097270f8c --- /dev/null +++ b/obp-api/src/test/scala/code/api/util/http4s/Http4sJsonContentTypeTest.scala @@ -0,0 +1,88 @@ +package code.api.util.http4s + +import cats.effect.IO +import cats.effect.unsafe.implicits.global +import code.api.util.CallContext +import code.api.util.http4s.Http4sRequestAttributes.{EndpointHelpers, callContextKey} +import net.liftweb.json.{DefaultFormats, Formats} +import org.http4s.{Request, Response} +import org.scalatest.{FeatureSpec, GivenWhenThen, Matchers} +import org.typelevel.ci.CIString + +import scala.concurrent.Future + +/** + * Regression tests for the native http4s endpoint response helpers in + * `Http4sRequestAttributes.EndpointHelpers`. + * + * These helpers render a Lift-JSON String and hand it to http4s' `Ok`/`Created`. Passing a + * raw String uses the default `EntityEncoder[String]`, which labels the response + * `Content-Type: text/plain; charset=UTF-8`. OBP only ever returns JSON from these helpers, + * so the content type must be `application/json` — otherwise strict clients (e.g. the + * API Manager frontend) reject a perfectly good JSON body. + * + * This was a real bug on the http4s-migrated dynamic-entity CRUD endpoints. The older Lift + * endpoints and the Lift -> http4s bridge set `application/json` correctly (covered by + * Http4sResponseConversionTest); this test pins the *native* http4s builders. + */ +class Http4sJsonContentTypeTest extends FeatureSpec with Matchers with GivenWhenThen { + + private implicit val formats: Formats = DefaultFormats + + /** A request carrying a (default) CallContext, as the helpers expect. */ + private def reqWithCallContext: Request[IO] = + Request[IO]().withAttribute(callContextKey, CallContext()) + + private def contentTypeOf(resp: Response[IO]): String = + resp.headers.get(CIString("Content-Type")).map(_.head.value).getOrElse("") + + feature("Native http4s endpoint helpers label JSON responses as application/json") { + + scenario("executeAndRespond (200 OK) sets application/json") { + Given("a 200 helper returning a JSON object") + When("the response is built") + val resp = EndpointHelpers + .executeAndRespond(reqWithCallContext)(_ => Future.successful(Map("message" -> "ok"))) + .unsafeRunSync() + + Then("status is 200 and Content-Type is application/json") + resp.status.code should equal(200) + contentTypeOf(resp) should include("application/json") + } + + scenario("executeFutureCreated (201 Created) sets application/json") { + Given("a 201 helper returning a JSON object") + When("the response is built") + val resp = EndpointHelpers + .executeFutureCreated(reqWithCallContext)(Future.successful(Map("id" -> "123"))) + .unsafeRunSync() + + Then("status is 201 and Content-Type is application/json") + resp.status.code should equal(201) + contentTypeOf(resp) should include("application/json") + } + + scenario("executeFutureWithStatus sets application/json for a custom status") { + Given("a helper returning a JSON object with an explicit 202 status") + When("the response is built") + val resp = EndpointHelpers + .executeFutureWithStatus(reqWithCallContext)(Future.successful((Map("queued" -> "true"), 202))) + .unsafeRunSync() + + Then("status is 202 and Content-Type is application/json") + resp.status.code should equal(202) + contentTypeOf(resp) should include("application/json") + } + + scenario("regression: helpers must NOT fall back to text/plain") { + Given("the 201 helper (the path the dynamic-entity create endpoint uses)") + When("the response is built") + val resp = EndpointHelpers + .executeFutureCreated(reqWithCallContext)(Future.successful(Map("k" -> "v"))) + .unsafeRunSync() + + Then("the Content-Type must not be text/plain") + contentTypeOf(resp) should not include "text/plain" + } + } +} diff --git a/obp-api/src/test/scala/code/api/v4_0_0/DynamicEntityTest.scala b/obp-api/src/test/scala/code/api/v4_0_0/DynamicEntityTest.scala index e7f44266cf..5d423f39fb 100644 --- a/obp-api/src/test/scala/code/api/v4_0_0/DynamicEntityTest.scala +++ b/obp-api/src/test/scala/code/api/v4_0_0/DynamicEntityTest.scala @@ -1145,11 +1145,16 @@ class DynamicEntityTest extends V400ServerSetup { val requestCreateFoobar = (dynamicEntity_Request / "FooBar").POST <@(user1) val responseCreateFoobar = makePostRequest(requestCreateFoobar, write(foobarObject)) responseCreateFoobar.code should equal(201) + // Regression: the native http4s dynamic-entity endpoints must label JSON responses as + // application/json. http4s' default String EntityEncoder otherwise sets text/plain, which + // strict JSON clients (e.g. the API Manager frontend) reject even though the body is valid JSON. + responseCreateFoobar.headers.map(_.get("Content-Type")).getOrElse("").toLowerCase should include("application/json") val dynamicEntityId = (responseCreateFoobar.body \ "foo_bar" \ "foo_bar_id").asInstanceOf[JString].s val requestGetFoobars = (dynamicEntity_Request / "FooBar").GET <@(user1) val responseGetFoobars = makeGetRequest(requestGetFoobars) responseGetFoobars.code should equal(200) + responseGetFoobars.headers.map(_.get("Content-Type")).getOrElse("").toLowerCase should include("application/json") val requestGetFoobar = (dynamicEntity_Request / "FooBar" / dynamicEntityId ).GET <@(user1) val responseGetFoobar = makeGetRequest(requestGetFoobar) @@ -1171,6 +1176,10 @@ class DynamicEntityTest extends V400ServerSetup { val requestCreateFoobar = (dynamicEntity_Request / "FooBar").POST <@(user2) val responseCreateFoobar = makePostRequest(requestCreateFoobar, write(foobarObject)) responseCreateFoobar.code should equal(403) + // Regression: error responses on the native http4s dynamic-entity path (rendered by + // ErrorResponseConverter, which bypasses ResourceDocMiddleware) must also be labelled + // application/json, not http4s' default text/plain. + responseCreateFoobar.headers.map(_.get("Content-Type")).getOrElse("").toLowerCase should include("application/json") And("error should be " + UserHasMissingRoles) responseCreateFoobar.body.extract[ErrorMessage].message contains (UserHasMissingRoles) should be (true) responseCreateFoobar.body.extract[ErrorMessage].message contains ("CanCreateDynamicEntity_SystemFooBar") should be (true) diff --git a/todo_shared_ensure_json_content_type.md b/todo_shared_ensure_json_content_type.md new file mode 100644 index 0000000000..8978795be1 --- /dev/null +++ b/todo_shared_ensure_json_content_type.md @@ -0,0 +1,60 @@ +# TODO: single shared `ensureJsonContentType` (DRY the JSON content-type guarantee) + +## Context + +OBP guarantees `Content-Type: application/json` on http4s responses in **three independent +places**, each with its own private copy of the same `Content-Type(MediaType.application.json)` +value and/or the same "if not JSON, force JSON" logic: + +1. `code/api/util/http4s/ResourceDocMiddleware.scala` + - `jsonContentType` (~line 52) + `ensureJsonContentType(response)` (~line 561), applied to + every response from the per-version services (v1.2.1 → v7, UK OB, Berlin Group). +2. `code/api/util/http4s/ErrorResponseConverter.scala` + - `jsonContentType` (~line 40), set on every error response branch (~lines 122, 156, 171, + 188, 204). +3. `code/api/util/http4s/Http4sSupport.scala` (`EndpointHelpers`) + - `jsonContentType` (added when fixing the dynamic-entity `text/plain` bug), applied in + `toJsonOk` / `executeFutureCreated` / `executeFutureWithStatus`. + +This duplication is why the `text/plain` bug was possible: the per-version services were +silently saved by `ResourceDocMiddleware.ensureJsonContentType`, while the services that +**bypass** that middleware leaked `text/plain` until `EndpointHelpers` was fixed at the source. + +## Why it matters + +The services that intentionally bypass `ResourceDocMiddleware` (runtime-mutable / lighter +gates) have **no runtime net** equivalent to the version services — they rely entirely on every +response being built through `EndpointHelpers` or `ErrorResponseConverter`. Today that holds +(verified: `Http4sDynamicEntity` builds no raw responses of its own), and it is now pinned by +content-type assertions in `DynamicEntityTest` (success path + 403 error path). But a future +contributor adding a new response path that skips both helpers would regress silently for: + +- `dynamicEntityRoutes` (`Http4sApp.scala:148`) +- `dynamicEndpointRoutes` (`Http4sApp.scala:149`) +- `DirectLoginRoutes` (`Http4sApp.scala:150`) +- `AliveCheckRoutes` (`Http4sApp.scala:151`) + +## Proposed work + +1. Extract one shared helper (e.g. `Http4sJson.ensureJsonContentType(resp): Response[IO]` and a + single shared `jsonContentType` val) into a small util, and have all three sites above call it + instead of their private copies. Pure refactor, no behaviour change. +2. (Optional, decided separately) Give the middleware-bypassing services the same runtime net the + version services have: wrap the four routes at the `Http4sApp` wiring (lines 148–151) with the + shared `ensureJsonContentType`, rather than minting a per-service wrapper. This makes the + source-level fix in `EndpointHelpers`/`ErrorResponseConverter` belt-and-suspenders rather than + load-bearing for those routes. + +## Acceptance + +- Exactly one definition of `jsonContentType` and one `ensureJsonContentType` in the codebase. +- `ResourceDocMiddleware`, `ErrorResponseConverter`, and `EndpointHelpers` all delegate to it. +- Existing content-type assertions (`Http4sJsonContentTypeTest`, `DynamicEntityTest` success + + error path, `Http4sResponseConversionTest`, `AliveCheckRoutesTest`) stay green. + +## Origin + +Follow-up to the dynamic-entity `text/plain` content-type fix in `Http4sSupport.scala` +(JSON responses were mislabelled because http4s' default `EntityEncoder[String]` sets +`text/plain`; the dynamic-entity service bypasses `ResourceDocMiddleware` so nothing +re-normalised it).