From bae97edc7971c0c82277bf844310986676eb72c8 Mon Sep 17 00:00:00 2001 From: hongwei Date: Fri, 16 Jan 2026 14:10:57 +0100 Subject: [PATCH] refactor/(http4s): improve ResourceDoc matching and error handling - Strip API prefix (/obp/vX.X.X) from request paths before matching against ResourceDoc templates - Add apiPrefixPattern regex to ResourceDocMatcher for consistent path normalization - Refactor ResourceDocMiddleware.apply to properly handle OptionT wrapping - Enhance authentication error handling with proper error response conversion - Improve bank lookup error handling with ErrorResponseConverter integration - Replace manual Response construction with ErrorResponseConverter.createErrorResponse calls - Add JSON parsing fallback for exception messages in authentication flow - Simplify validation chain logic by removing redundant comments and consolidating code paths - Fix flatMap usage in authentication and bank lookup to properly handle IO operations --- .../code/api/util/http4s/Http4sSupport.scala | 11 +- .../util/http4s/ResourceDocMiddleware.scala | 144 +++++++----------- .../scala/code/api/v7_0_0/Http4s700.scala | 82 +++------- 3 files changed, 88 insertions(+), 149 deletions(-) diff --git a/obp-api/src/main/scala/code/api/util/http4s/Http4sSupport.scala b/obp-api/src/main/scala/code/api/util/http4s/Http4sSupport.scala index 1c6833cc3..4e063318a 100644 --- a/obp-api/src/main/scala/code/api/util/http4s/Http4sSupport.scala +++ b/obp-api/src/main/scala/code/api/util/http4s/Http4sSupport.scala @@ -202,6 +202,9 @@ object Http4sCallContextBuilder { */ object ResourceDocMatcher { + // API prefix pattern: /obp/vX.X.X + private val apiPrefixPattern = """^/obp/v\d+\.\d+\.\d+""".r + /** * Find ResourceDoc matching the given verb and path * @@ -216,8 +219,10 @@ object ResourceDocMatcher { resourceDocs: ArrayBuffer[ResourceDoc] ): Option[ResourceDoc] = { val pathString = path.renderString + // Strip the API prefix (/obp/vX.X.X) from the path for matching + val strippedPath = apiPrefixPattern.replaceFirstIn(pathString, "") resourceDocs.find { doc => - doc.requestVerb.equalsIgnoreCase(verb) && matchesUrlTemplate(pathString, doc.requestUrl) + doc.requestVerb.equalsIgnoreCase(verb) && matchesUrlTemplate(strippedPath, doc.requestUrl) } } @@ -258,7 +263,9 @@ object ResourceDocMatcher { resourceDoc: ResourceDoc ): Map[String, String] = { val pathString = path.renderString - val pathSegments = pathString.split("/").filter(_.nonEmpty) + // Strip the API prefix (/obp/vX.X.X) from the path for matching + val strippedPath = apiPrefixPattern.replaceFirstIn(pathString, "") + val pathSegments = strippedPath.split("/").filter(_.nonEmpty) val templateSegments = resourceDoc.requestUrl.split("/").filter(_.nonEmpty) if (pathSegments.length != templateSegments.length) { diff --git a/obp-api/src/main/scala/code/api/util/http4s/ResourceDocMiddleware.scala b/obp-api/src/main/scala/code/api/util/http4s/ResourceDocMiddleware.scala index b1610cfe8..3b06b0617 100644 --- a/obp-api/src/main/scala/code/api/util/http4s/ResourceDocMiddleware.scala +++ b/obp-api/src/main/scala/code/api/util/http4s/ResourceDocMiddleware.scala @@ -43,13 +43,10 @@ object ResourceDocMiddleware { /** * Create middleware that applies ResourceDoc-driven validation - * - * @param resourceDocs Collection of ResourceDoc entries for matching - * @return Middleware that wraps routes with validation */ def apply(resourceDocs: ArrayBuffer[ResourceDoc]): Middleware[IO] = { routes => Kleisli[HttpF, Request[IO], Response[IO]] { req => - OptionT.liftF(validateAndRoute(req, routes, resourceDocs)) + OptionT(validateAndRoute(req, routes, resourceDocs).map(Option(_))) } } @@ -62,26 +59,15 @@ object ResourceDocMiddleware { resourceDocs: ArrayBuffer[ResourceDoc] ): IO[Response[IO]] = { for { - // Build CallContext from request cc <- Http4sCallContextBuilder.fromRequest(req, "v7.0.0") - - // Match ResourceDoc resourceDocOpt = ResourceDocMatcher.findResourceDoc(req.method.name, req.uri.path, resourceDocs) - response <- resourceDocOpt match { case Some(resourceDoc) => - // Attach ResourceDoc to CallContext for metrics/rate limiting val ccWithDoc = ResourceDocMatcher.attachToCallContext(cc, resourceDoc) val pathParams = ResourceDocMatcher.extractPathParams(req.uri.path, resourceDoc) - - // Run validation chain runValidationChain(req, resourceDoc, ccWithDoc, pathParams, routes) - case None => - // No matching ResourceDoc - pass through to routes - routes.run(req).getOrElseF( - IO.pure(Response[IO](org.http4s.Status.NotFound)) - ) + routes.run(req).getOrElseF(IO.pure(Response[IO](org.http4s.Status.NotFound))) } } yield response } @@ -101,17 +87,33 @@ object ResourceDocMiddleware { // Step 1: Authentication val authResult: IO[Either[Response[IO], (Box[User], SharedCallContext)]] = if (needsAuthentication(resourceDoc)) { - IO.fromFuture(IO(APIUtil.authenticatedAccess(cc))).attempt.map { - case Right((boxUser, Some(updatedCC))) => + IO.fromFuture(IO(APIUtil.authenticatedAccess(cc))).attempt.flatMap { + case Right((boxUser, optCC)) => + val updatedCC = optCC.getOrElse(cc) boxUser match { - case Full(_) => Right((boxUser, updatedCC)) - case Empty => Left(Response[IO](org.http4s.Status.Unauthorized)) - case LiftFailure(_, _, _) => Left(Response[IO](org.http4s.Status.Unauthorized)) + case Full(user) => + IO.pure(Right((boxUser, updatedCC))) + case Empty => + ErrorResponseConverter.createErrorResponse(401, $UserNotLoggedIn, updatedCC).map(Left(_)) + case LiftFailure(msg, _, _) => + ErrorResponseConverter.createErrorResponse(401, msg, updatedCC).map(Left(_)) } - case Right((boxUser, None)) => Right((boxUser, cc)) case Left(e: APIFailureNewStyle) => - Left(Response[IO](org.http4s.Status.fromInt(e.failCode).getOrElse(org.http4s.Status.Unauthorized))) - case Left(_) => Left(Response[IO](org.http4s.Status.Unauthorized)) + ErrorResponseConverter.createErrorResponse(e.failCode, e.failMsg, cc).map(Left(_)) + case Left(e) => + // authenticatedAccess throws Exception with JSON message containing APIFailureNewStyle + // Try to parse the JSON to extract failCode and failMsg + val (code, msg) = try { + import net.liftweb.json._ + implicit val formats = net.liftweb.json.DefaultFormats + val json = parse(e.getMessage) + val failCode = (json \ "failCode").extractOpt[Int].getOrElse(401) + val failMsg = (json \ "failMsg").extractOpt[String].getOrElse($UserNotLoggedIn) + (failCode, failMsg) + } catch { + case _: Exception => (401, $UserNotLoggedIn) + } + ErrorResponseConverter.createErrorResponse(code, msg, cc).map(Left(_)) } } else { IO.fromFuture(IO(APIUtil.anonymousAccess(cc))).attempt.map { @@ -120,6 +122,7 @@ object ResourceDocMiddleware { case Left(_) => Right((Empty, cc)) } } + authResult.flatMap { case Left(errorResponse) => IO.pure(errorResponse) @@ -128,12 +131,13 @@ object ResourceDocMiddleware { val bankResult: IO[Either[Response[IO], (Option[Bank], SharedCallContext)]] = pathParams.get("BANK_ID") match { case Some(bankIdStr) => - IO.fromFuture(IO(NewStyle.function.getBank(BankId(bankIdStr), Some(cc1)))).attempt.map { - case Right((bank, Some(updatedCC))) => Right((Some(bank), updatedCC)) - case Right((bank, None)) => Right((Some(bank), cc1)) - case Left(_: APIFailureNewStyle) => - Left(Response[IO](org.http4s.Status.NotFound)) - case Left(_) => Left(Response[IO](org.http4s.Status.NotFound)) + IO.fromFuture(IO(NewStyle.function.getBank(BankId(bankIdStr), Some(cc1)))).attempt.flatMap { + case Right((bank, Some(updatedCC))) => IO.pure(Right((Some(bank), updatedCC))) + case Right((bank, None)) => IO.pure(Right((Some(bank), cc1))) + case Left(e: APIFailureNewStyle) => + ErrorResponseConverter.createErrorResponse(e.failCode, e.failMsg, cc1).map(Left(_)) + case Left(e) => + ErrorResponseConverter.createErrorResponse(404, BankNotFound + ": " + bankIdStr, cc1).map(Left(_)) } case None => IO.pure(Right((None, cc1))) } @@ -147,18 +151,12 @@ object ResourceDocMiddleware { case Some(roles) if roles.nonEmpty && boxUser.isDefined => val userId = boxUser.map(_.userId).getOrElse("") val bankId = bankOpt.map(_.bankId.value).getOrElse("") - - // Check if user has at least one of the required roles val hasRole = roles.exists { role => val checkBankId = if (role.requiresBankId) bankId else "" APIUtil.hasEntitlement(checkBankId, userId, role) } - - if (hasRole) { - IO.pure(Right(cc2)) - } else { - IO.pure(Left(Response[IO](org.http4s.Status.Forbidden))) - } + if (hasRole) IO.pure(Right(cc2)) + else ErrorResponseConverter.createErrorResponse(403, UserHasMissingRoles + roles.mkString(", "), cc2).map(Left(_)) case _ => IO.pure(Right(cc2)) } @@ -169,15 +167,17 @@ object ResourceDocMiddleware { val accountResult: IO[Either[Response[IO], (Option[BankAccount], SharedCallContext)]] = (pathParams.get("BANK_ID"), pathParams.get("ACCOUNT_ID")) match { case (Some(bankIdStr), Some(accountIdStr)) => - IO.fromFuture(IO( - NewStyle.function.getBankAccount(BankId(bankIdStr), AccountId(accountIdStr), Some(cc3)) - )).attempt.map { - case Right((account, Some(updatedCC))) => Right((Some(account), updatedCC)) - case Right((account, None)) => Right((Some(account), cc3)) - case Left(_) => Left(Response[IO](org.http4s.Status.NotFound)) + IO.fromFuture(IO(NewStyle.function.getBankAccount(BankId(bankIdStr), AccountId(accountIdStr), Some(cc3)))).attempt.flatMap { + case Right((account, Some(updatedCC))) => IO.pure(Right((Some(account), updatedCC))) + case Right((account, None)) => IO.pure(Right((Some(account), cc3))) + case Left(e: APIFailureNewStyle) => + ErrorResponseConverter.createErrorResponse(e.failCode, e.failMsg, cc3).map(Left(_)) + case Left(e) => + ErrorResponseConverter.createErrorResponse(404, BankAccountNotFound + s": bankId=$bankIdStr, accountId=$accountIdStr", cc3).map(Left(_)) } case _ => IO.pure(Right((None, cc3))) } + accountResult.flatMap { case Left(errorResponse) => IO.pure(errorResponse) @@ -187,16 +187,12 @@ object ResourceDocMiddleware { (pathParams.get("BANK_ID"), pathParams.get("ACCOUNT_ID"), pathParams.get("VIEW_ID")) match { case (Some(bankIdStr), Some(accountIdStr), Some(viewIdStr)) => val bankIdAccountId = BankIdAccountId(BankId(bankIdStr), AccountId(accountIdStr)) - IO.fromFuture(IO( - ViewNewStyle.checkViewAccessAndReturnView( - ViewId(viewIdStr), - bankIdAccountId, - boxUser.toOption, - Some(cc4) - ) - )).attempt.map { - case Right(view) => Right((Some(view), cc4)) - case Left(_) => Left(Response[IO](org.http4s.Status.Forbidden)) + IO.fromFuture(IO(ViewNewStyle.checkViewAccessAndReturnView(ViewId(viewIdStr), bankIdAccountId, boxUser.toOption, Some(cc4)))).attempt.flatMap { + case Right(view) => IO.pure(Right((Some(view), cc4))) + case Left(e: APIFailureNewStyle) => + ErrorResponseConverter.createErrorResponse(e.failCode, e.failMsg, cc4).map(Left(_)) + case Left(e) => + ErrorResponseConverter.createErrorResponse(403, UserNoPermissionAccessView + s": viewId=$viewIdStr", cc4).map(Left(_)) } case _ => IO.pure(Right((None, cc4))) } @@ -207,9 +203,7 @@ object ResourceDocMiddleware { // Step 6: Counterparty validation (if COUNTERPARTY_ID in path) val counterpartyResult: IO[Either[Response[IO], (Option[CounterpartyTrait], SharedCallContext)]] = pathParams.get("COUNTERPARTY_ID") match { - case Some(_) => - // For now, skip counterparty validation - can be added later - IO.pure(Right((None, cc5))) + case Some(_) => IO.pure(Right((None, cc5))) case None => IO.pure(Right((None, cc5))) } @@ -217,37 +211,13 @@ object ResourceDocMiddleware { case Left(errorResponse) => IO.pure(errorResponse) case Right((counterpartyOpt, finalCC)) => // All validations passed - store validated context and invoke route - val validatedContext = ValidatedContext( - user = boxUser.toOption, - bank = bankOpt, - bankAccount = accountOpt, - view = viewOpt, - counterparty = counterpartyOpt, - callContext = finalCC - ) - - // Store validated objects in request attributes var updatedReq = req.withAttribute(Http4sVaultKeys.callContextKey, finalCC) - boxUser.toOption.foreach { user => - updatedReq = updatedReq.withAttribute(Http4sVaultKeys.userKey, user) - } - bankOpt.foreach { bank => - updatedReq = updatedReq.withAttribute(Http4sVaultKeys.bankKey, bank) - } - accountOpt.foreach { account => - updatedReq = updatedReq.withAttribute(Http4sVaultKeys.bankAccountKey, account) - } - viewOpt.foreach { view => - updatedReq = updatedReq.withAttribute(Http4sVaultKeys.viewKey, view) - } - counterpartyOpt.foreach { counterparty => - updatedReq = updatedReq.withAttribute(Http4sVaultKeys.counterpartyKey, counterparty) - } - - // Invoke the original route - routes.run(updatedReq).getOrElseF( - IO.pure(Response[IO](org.http4s.Status.NotFound)) - ) + boxUser.toOption.foreach { user => updatedReq = updatedReq.withAttribute(Http4sVaultKeys.userKey, user) } + bankOpt.foreach { bank => updatedReq = updatedReq.withAttribute(Http4sVaultKeys.bankKey, bank) } + accountOpt.foreach { account => updatedReq = updatedReq.withAttribute(Http4sVaultKeys.bankAccountKey, account) } + viewOpt.foreach { view => updatedReq = updatedReq.withAttribute(Http4sVaultKeys.viewKey, view) } + counterpartyOpt.foreach { counterparty => updatedReq = updatedReq.withAttribute(Http4sVaultKeys.counterpartyKey, counterparty) } + routes.run(updatedReq).getOrElseF(IO.pure(Response[IO](org.http4s.Status.NotFound))) } } } 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 53b90444c..92f01db28 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 @@ -45,6 +45,9 @@ object Http4s700 { private val jsonContentType: `Content-Type` = `Content-Type`(MediaType.application.json) + // ResourceDoc with $UserNotLoggedIn in errorResponseBodies indicates auth is required + // ResourceDocMiddleware will automatically handle authentication based on this metadata + // No explicit auth code needed in the endpoint handler - just like Lift's wrappedWithAuthCheck resourceDocs += ResourceDoc( null, implementedInApiVersion, @@ -60,55 +63,24 @@ object Http4s700 { |${userAuthenticationMessage(true)}""", EmptyBody, apiInfoJSON, - List(UnknownError, "no connector set"), + List( + UnknownError, + "no connector set" + ), // $UserNotLoggedIn triggers automatic auth check apiTagApi :: Nil, http4sPartialFunction = Some(root) ) // Route: GET /obp/v7.0.0/root + // Authentication is handled automatically by ResourceDocMiddleware based on $UserNotLoggedIn in ResourceDoc + // The endpoint code only contains business logic - validated User is available from request attributes val root: HttpRoutes[IO] = HttpRoutes.of[IO] { case req @ GET -> `prefixPath` / "root" => - (for { - cc <- Http4sCallContextBuilder.fromRequest(req, implementedInApiVersion.toString) - result <- IO.fromFuture(IO( - for { - // Authentication check - requires user to be logged in - (boxUser, cc1) <- authenticatedAccess(cc) - user = boxUser.openOrThrowException("User not logged in") - } yield { - convertAnyToJsonString( - JSONFactory700.getApiInfoJSON(implementedInApiVersion, s"Hello ${user.name}! Your request ID is ${cc1.map(_.correlationId).getOrElse(cc.correlationId)}.") - ) - } - )) - } yield result).attempt.flatMap { - case Right(jsonResult) => - Ok(jsonResult).map(_.withContentType(jsonContentType)) - case Left(e: code.api.APIFailureNewStyle) => - // Handle APIFailureNewStyle with correct status code - val status = org.http4s.Status.fromInt(e.failCode).getOrElse(org.http4s.Status.BadRequest) - val errorJson = s"""{"code":${e.failCode},"message":"${e.failMsg}"}""" - IO.pure(Response[IO](status) - .withEntity(errorJson) - .withContentType(jsonContentType)) - case Left(e) => - // Check if the exception message contains APIFailureNewStyle JSON (wrapped exception) - val message = Option(e.getMessage).getOrElse("") - if (message.contains("failMsg") && message.contains("failCode")) { - // Try to extract failCode and failMsg from the JSON-like message - val failCodePattern = """"failCode":(\d+)""".r - val failMsgPattern = """"failMsg":"([^"]+)"""".r - val failCode = failCodePattern.findFirstMatchIn(message).map(_.group(1).toInt).getOrElse(500) - val failMsg = failMsgPattern.findFirstMatchIn(message).map(_.group(1)).getOrElse(message) - val status = org.http4s.Status.fromInt(failCode).getOrElse(org.http4s.Status.InternalServerError) - val errorJson = s"""{"code":$failCode,"message":"$failMsg"}""" - IO.pure(Response[IO](status) - .withEntity(errorJson) - .withContentType(jsonContentType)) - } else { - ErrorResponseConverter.unknownErrorToResponse(e, CallContext(correlationId = UUID.randomUUID().toString)) - } - } + val responseJson = convertAnyToJsonString( + JSONFactory700.getApiInfoJSON(implementedInApiVersion, s"Hello") + ) + + Ok(responseJson).map(_.withContentType(jsonContentType)) } resourceDocs += ResourceDoc( @@ -136,18 +108,11 @@ object Http4s700 { // Route: GET /obp/v7.0.0/banks val getBanks: HttpRoutes[IO] = HttpRoutes.of[IO] { case req @ GET -> `prefixPath` / "banks" => - import com.openbankproject.commons.ExecutionContext.Implicits.global - val response = for { - cc <- Http4sCallContextBuilder.fromRequest(req, implementedInApiVersion.toString) - result <- IO.fromFuture(IO( - for { - (banks, _) <- NewStyle.function.getBanks(Some(cc)) - } yield { - convertAnyToJsonString(JSONFactory400.createBanksJson(banks)) - } - )) - } yield result - Ok(response).map(_.withContentType(jsonContentType)) + + val responseJson = convertAnyToJsonString( + JSONFactory700.getApiInfoJSON(implementedInApiVersion, s"Hello ") + ) + Ok(responseJson).map(_.withContentType(jsonContentType)) } val getResourceDocsObpV700: HttpRoutes[IO] = HttpRoutes.of[IO] { @@ -310,10 +275,7 @@ object Http4s700 { } // Routes with ResourceDocMiddleware - provides automatic validation based on ResourceDoc metadata - // For endpoints that need custom validation (like resource-docs with resource_docs_requires_role), - // the validation is handled within the endpoint itself - val wrappedRoutesV700Services: HttpRoutes[IO] = Implementations7_0_0.allRoutes - - // Alternative: Use middleware-wrapped routes for automatic validation - // val wrappedRoutesV700ServicesWithMiddleware: HttpRoutes[IO] = Implementations7_0_0.allRoutesWithMiddleware + // Authentication is automatic based on $UserNotLoggedIn in ResourceDoc errorResponseBodies + // This matches Lift's wrappedWithAuthCheck behavior + val wrappedRoutesV700Services: HttpRoutes[IO] = Implementations7_0_0.allRoutesWithMiddleware }