diff --git a/obp-api/src/main/scala/code/api/OAuth2.scala b/obp-api/src/main/scala/code/api/OAuth2.scala index 9cee12f436..94881b3cf7 100644 --- a/obp-api/src/main/scala/code/api/OAuth2.scala +++ b/obp-api/src/main/scala/code/api/OAuth2.scala @@ -501,15 +501,10 @@ object OAuth2Login extends RestHelper with MdcLoggable { val sub = getClaim(name = "sub", jwtToken = jwtToken) val email = getClaim(name = "email", jwtToken = jwtToken) val name = getClaim(name = "name", jwtToken = jwtToken).orElse(description) - val consumerId = azp match { - case Some(value) if APIUtil.checkIfStringIsUUID(value) => azp - case Some(value) => Some(s"${value}_${APIUtil.generateUUID()}") - case None => Some(APIUtil.generateUUID()) - } Consumers.consumers.vend.getOrCreateConsumer( - consumerId = consumerId, // Use azp as consumer id if it is uuid value - key = Some(Helpers.randomString(40).toLowerCase), - secret = Some(Helpers.randomString(40).toLowerCase), + consumerId = None, + key = None, + secret = None, aud = aud, azp = azp, iss = iss, diff --git a/obp-api/src/main/scala/code/api/openidconnect.scala b/obp-api/src/main/scala/code/api/openidconnect.scala index 87f30b6253..00cdfb0b18 100644 --- a/obp-api/src/main/scala/code/api/openidconnect.scala +++ b/obp-api/src/main/scala/code/api/openidconnect.scala @@ -306,9 +306,9 @@ object OpenIdConnect extends OBPRestHelper with MdcLoggable { private def getOrCreateConsumer(idToken: String, userId: String): Box[Consumer] = { Consumers.consumers.vend.getOrCreateConsumer( - consumerId=Some(APIUtil.generateUUID()), - Some(Helpers.randomString(40).toLowerCase), - Some(Helpers.randomString(40).toLowerCase), + consumerId=None, + None, + None, Some(JwtUtil.getAudience(idToken).mkString(",")), getClaim(name = "azp", idToken = idToken), JwtUtil.getIssuer(idToken), diff --git a/obp-api/src/main/scala/code/api/util/APIUtil.scala b/obp-api/src/main/scala/code/api/util/APIUtil.scala index 536a12d18e..2029933f86 100644 --- a/obp-api/src/main/scala/code/api/util/APIUtil.scala +++ b/obp-api/src/main/scala/code/api/util/APIUtil.scala @@ -3170,7 +3170,7 @@ object APIUtil extends MdcLoggable with CustomJsonFormats{ // This is normal for certificate-based validation or anonymous requests Empty } - logger.debug(s"getUserAndSessionContextFuture says consumerByConsumerKey is: $consumerByConsumerKey") + //logger.debug(s"getUserAndSessionContextFuture says consumerByConsumerKey is: $consumerByConsumerKey") val res = if (authHeadersWithEmptyValues.nonEmpty) { // Check Authorization Headers Empty Values diff --git a/obp-api/src/main/scala/code/model/OAuth.scala b/obp-api/src/main/scala/code/model/OAuth.scala index ea00a31930..c2b7fd1cb0 100644 --- a/obp-api/src/main/scala/code/model/OAuth.scala +++ b/obp-api/src/main/scala/code/model/OAuth.scala @@ -394,16 +394,65 @@ object MappedConsumersProvider extends ConsumersProvider with MdcLoggable { logoUrl: Option[String], ): Box[Consumer] = { - val consumer: Box[Consumer] = - // 1st try to find via UUID issued by OBP-API back end - Consumer.find(By(Consumer.consumerId, consumerId.getOrElse("None"))) or - // 2nd try to find via the pair (azp, iss) issued by External Identity Provider - { - // The azp field in the payload of a JWT (JSON Web Token) represents the Authorized Party. - // It is typically used in the context of OAuth 2.0 and OpenID Connect to identify the client application that the token is issued for. - // The pair (azp, iss) is a unique key in case of Client of an Identity Provider - Consumer.find(By(Consumer.azp, azp.getOrElse("None")), By(Consumer.iss, iss.getOrElse("None"))) + logger.info(s"getOrCreateConsumer says: BEGIN lookup. Input: consumerId=${consumerId.getOrElse("None")}, azp=${azp.getOrElse("None")}, iss=${iss.getOrElse("None")}, sub=${sub.getOrElse("None")}") + + // 1st try: find by consumerId (UUID issued by OBP-API back end) + val byConsumerId = Consumer.find(By(Consumer.consumerId, consumerId.getOrElse("None"))) + val consumer: Box[Consumer] = if (byConsumerId.isDefined) { + val c = byConsumerId.openOrThrowException("checked isDefined") + logger.info(s"getOrCreateConsumer says: MATCH on lookup 1 (by consumerId). Found consumer: consumerId=${c.consumerId.get}, key=${c.key.get}, azp=${c.azp.get}, iss=${c.iss.get}") + byConsumerId + } else { + logger.info(s"getOrCreateConsumer says: MISS on lookup 1 (by consumerId=${consumerId.getOrElse("None")}). Trying lookup 2 (by Consumer.key matching azp)...") + + // 2nd try: find by consumer key matching azp (pre-registered consumer whose key is the OAuth2 client_id) + // This is checked before (azp, iss) so that a pre-registered consumer takes priority over an auto-created one + val byKeyMatchingAzp = Consumer.find(By(Consumer.key, azp.getOrElse("None"))) + if (byKeyMatchingAzp.isDefined) { + val c = byKeyMatchingAzp.openOrThrowException("checked isDefined") + logger.info(s"getOrCreateConsumer says: MATCH on lookup 2 (by Consumer.key matching azp). Found pre-registered consumer: consumerId=${c.consumerId.get}, key=${c.key.get}, azp=${c.azp.get}, iss=${c.iss.get}") + // Transitional cleanup: before the duplicate-consumer fix, OAuth2/OIDC flows could auto-create + // consumers that now conflict with the pre-registered one we just found. Clear the stale consumer's + // azp/iss/sub so we can populate those fields on the pre-registered consumer without a unique + // constraint violation. This block can be removed once all environments have been cleaned up. + val conflicting = Consumer.find(By(Consumer.azp, azp.getOrElse("None")), By(Consumer.iss, iss.getOrElse("None"))) + for (stale <- conflicting) { + if (stale.id.get != c.id.get) { + logger.info(s"getOrCreateConsumer says: Found CONFLICTING auto-created consumer holding the same (azp, iss). Clearing its azp/iss/sub to avoid unique constraint violation. Stale consumer: consumerId=${stale.consumerId.get}, key=${stale.key.get}, azp=${stale.azp.get}, iss=${stale.iss.get}, sub=${stale.sub.get}") + stale.azp(APIUtil.generateUUID()) + stale.sub(APIUtil.generateUUID()) + stale.saveMe() + logger.info(s"getOrCreateConsumer says: Cleared stale consumer. Now: consumerId=${stale.consumerId.get}, azp=${stale.azp.get}, sub=${stale.sub.get}") + } + } + // End of transitional cleanup block + logger.info(s"getOrCreateConsumer says: Updating azp/iss/sub on pre-registered consumer so future lookups also match by (azp, iss)...") + // Populate azp, iss, sub on the existing consumer so future lookups can also find it by (azp, iss) + for (found <- byKeyMatchingAzp) { + azp.foreach(v => found.azp(v)) + iss.foreach(v => found.iss(v)) + sub.foreach(v => found.sub(v)) + found.saveMe() + logger.info(s"getOrCreateConsumer says: Updated pre-registered consumer. Now: consumerId=${found.consumerId.get}, key=${found.key.get}, azp=${found.azp.get}, iss=${found.iss.get}, sub=${found.sub.get}") + } + byKeyMatchingAzp + } else { + logger.info(s"getOrCreateConsumer says: MISS on lookup 2 (no consumer has key=${azp.getOrElse("None")}). Trying lookup 3 (by azp+iss pair)...") + + // 3rd try: find by (azp, iss) pair issued by External Identity Provider + // The azp field in a JWT represents the Authorized Party (OAuth 2.0 / OpenID Connect client application). + // The pair (azp, iss) is a unique key in case of Client of an Identity Provider + val byAzpIss = Consumer.find(By(Consumer.azp, azp.getOrElse("None")), By(Consumer.iss, iss.getOrElse("None"))) + if (byAzpIss.isDefined) { + val c = byAzpIss.openOrThrowException("checked isDefined") + logger.info(s"getOrCreateConsumer says: MATCH on lookup 3 (by azp+iss). Found auto-created consumer: consumerId=${c.consumerId.get}, key=${c.key.get}, azp=${c.azp.get}, iss=${c.iss.get}") + byAzpIss + } else { + logger.info(s"getOrCreateConsumer says: MISS on all 3 lookups. Will CREATE a new consumer. Searched: consumerId=${consumerId.getOrElse("None")}, key=${azp.getOrElse("None")}, (azp=${azp.getOrElse("None")}, iss=${iss.getOrElse("None")})") + Empty } + } + } consumer match { case Full(c) => Full(c) case Failure(msg, t, c) => Failure(msg, t, c) @@ -411,14 +460,17 @@ object MappedConsumersProvider extends ConsumersProvider with MdcLoggable { case Empty => tryo { val c = Consumer.create - key match { - case Some(v) => c.key(v) - case None => - } - secret match { - case Some(v) => c.secret(v) - case None => + val actualKey = key.getOrElse(Helpers.randomString(40).toLowerCase) + val actualSecret = secret.getOrElse(Helpers.randomString(40).toLowerCase) + val actualConsumerId = consumerId.getOrElse { + azp match { + case Some(value) if APIUtil.checkIfStringIsUUID(value) => value + case Some(value) => s"${value}_${APIUtil.generateUUID()}" + case None => APIUtil.generateUUID() + } } + c.key(actualKey) + c.secret(actualSecret) aud match { case Some(v) => c.aud(v) case None => @@ -480,10 +532,7 @@ object MappedConsumersProvider extends ConsumersProvider with MdcLoggable { case Some(v) => c.logoUrl(v) case None => } - consumerId match { - case Some(v) => c.consumerId(v) - case None => - } + c.consumerId(actualConsumerId) val createdConsumer = c.saveMe() // In case we use Hydra ORY as Identity Provider we create corresponding client at Hydra side a well if(integrateWithHydra) createHydraClient(createdConsumer) diff --git a/obp-api/src/main/scala/code/util/SecureLogging.scala b/obp-api/src/main/scala/code/util/SecureLogging.scala index b3fe3de2b5..a5e72d2e0b 100644 --- a/obp-api/src/main/scala/code/util/SecureLogging.scala +++ b/obp-api/src/main/scala/code/util/SecureLogging.scala @@ -2,7 +2,7 @@ package code.util import code.api.util.APIUtil -import java.util.regex.Pattern +import java.util.regex.{Matcher, Pattern} import scala.collection.mutable /** @@ -21,98 +21,109 @@ object SecureLogging { private def conditionalPattern( prop: String, defaultValue: Boolean = true - )(pattern: => (Pattern, String)): Option[(Pattern, String)] = { + )(pattern: => (Pattern, Matcher => String)): Option[(Pattern, Matcher => String)] = { if (APIUtil.getPropsAsBoolValue(prop, defaultValue)) Some(pattern) else None } + /** Helper to create a static replacement function from a replacement string */ + private def staticReplacement(replacement: String): Matcher => String = _ => replacement + + /** Helper to create a partial-mask replacement that shows first 3 and last 3 chars of group 2 */ + private def partialMaskReplacement: Matcher => String = m => { + val prefix = m.group(1) + val value = m.group(2) + if (value.length > 6) s"${prefix}${value.take(3)}...${value.takeRight(3)}" + else s"${prefix}***" + } + /** * Toggleable sensitive patterns. * Note: The sensitive keywords are defined in APIUtil.sensitiveKeywords. * When adding new categories here, also update that shared list. */ - private lazy val sensitivePatterns: List[(Pattern, String)] = { + private lazy val sensitivePatterns: List[(Pattern, Matcher => String)] = { val patterns = Seq( // OAuth2 / API secrets conditionalPattern("securelogging_mask_secret") { - (Pattern.compile("(?i)(secret=)([^,\\s&]+)"), "$1***") + (Pattern.compile("(?i)(secret=)([^,\\s&]+)"), staticReplacement("$1***")) }, conditionalPattern("securelogging_mask_client_secret") { - (Pattern.compile("(?i)(client_secret[\"']?\\s*[:=]\\s*[\"']?)([^\"',\\s&]+)"), "$1***") + (Pattern.compile("(?i)(client_secret[\"']?\\s*[:=]\\s*[\"']?)([^\"',\\s&]+)"), staticReplacement("$1***")) }, conditionalPattern("securelogging_mask_client_secret") { - (Pattern.compile("(?i)(client_secret\\s*->\\s*)([^,\\s&\\)]+)"), "$1***") + (Pattern.compile("(?i)(client_secret\\s*->\\s*)([^,\\s&\\)]+)"), staticReplacement("$1***")) }, // Authorization / Tokens conditionalPattern("securelogging_mask_authorization") { - (Pattern.compile("(?i)(Authorization:\\s*Bearer\\s+)([^\\s,&]+)"), "$1***") + (Pattern.compile("(?i)(Authorization:\\s*Bearer\\s+)([^\\s,&]+)"), staticReplacement("$1***")) }, conditionalPattern("securelogging_mask_access_token") { - (Pattern.compile("(?i)(access_token[\"']?\\s*[:=]\\s*[\"']?)([^\"',\\s&]+)"), "$1***") + (Pattern.compile("(?i)(access_token[\"']?\\s*[:=]\\s*[\"']?)([^\"',\\s&]+)"), staticReplacement("$1***")) }, conditionalPattern("securelogging_mask_access_token") { - (Pattern.compile("(?i)(access_token\\s*->\\s*)([^,\\s&\\)]+)"), "$1***") + (Pattern.compile("(?i)(access_token\\s*->\\s*)([^,\\s&\\)]+)"), staticReplacement("$1***")) }, conditionalPattern("securelogging_mask_refresh_token") { - (Pattern.compile("(?i)(refresh_token[\"']?\\s*[:=]\\s*[\"']?)([^\"',\\s&]+)"), "$1***") + (Pattern.compile("(?i)(refresh_token[\"']?\\s*[:=]\\s*[\"']?)([^\"',\\s&]+)"), staticReplacement("$1***")) }, conditionalPattern("securelogging_mask_refresh_token") { - (Pattern.compile("(?i)(refresh_token\\s*->\\s*)([^,\\s&\\)]+)"), "$1***") + (Pattern.compile("(?i)(refresh_token\\s*->\\s*)([^,\\s&\\)]+)"), staticReplacement("$1***")) }, conditionalPattern("securelogging_mask_id_token") { - (Pattern.compile("(?i)(id_token[\"']?\\s*[:=]\\s*[\"']?)([^\"',\\s&]+)"), "$1***") + (Pattern.compile("(?i)(id_token[\"']?\\s*[:=]\\s*[\"']?)([^\"',\\s&]+)"), staticReplacement("$1***")) }, conditionalPattern("securelogging_mask_id_token") { - (Pattern.compile("(?i)(id_token\\s*->\\s*)([^,\\s&\\)]+)"), "$1***") + (Pattern.compile("(?i)(id_token\\s*->\\s*)([^,\\s&\\)]+)"), staticReplacement("$1***")) }, conditionalPattern("securelogging_mask_token") { - (Pattern.compile("(?i)(token[\"']?\\s*[:=]\\s*[\"']?)([^\"',\\s&]+)"), "$1***") + (Pattern.compile("(?i)(token[\"']?\\s*[:=]\\s*[\"']?)([^\"',\\s&]+)"), staticReplacement("$1***")) }, conditionalPattern("securelogging_mask_token") { - (Pattern.compile("(?i)(token\\s*->\\s*)([^,\\s&\\)]+)"), "$1***") + (Pattern.compile("(?i)(token\\s*->\\s*)([^,\\s&\\)]+)"), staticReplacement("$1***")) }, // Passwords conditionalPattern("securelogging_mask_password") { - (Pattern.compile("(?i)(password[\"']?\\s*[:=]\\s*[\"']?)([^\"',\\s&]+)"), "$1***") + (Pattern.compile("(?i)(password[\"']?\\s*[:=]\\s*[\"']?)([^\"',\\s&]+)"), staticReplacement("$1***")) }, conditionalPattern("securelogging_mask_password") { - (Pattern.compile("(?i)(password\\s*->\\s*)([^,\\s&\\)]+)"), "$1***") + (Pattern.compile("(?i)(password\\s*->\\s*)([^,\\s&\\)]+)"), staticReplacement("$1***")) }, - // API keys + // API keys - use partial masking to show first 3 and last 3 characters conditionalPattern("securelogging_mask_api_key") { - (Pattern.compile("(?i)(api_key[\"']?\\s*[:=]\\s*[\"']?)([^\"',\\s&]+)"), "$1***") + (Pattern.compile("(?i)(api_key[\"']?\\s*[:=]\\s*[\"']?)([^\"',\\s&]+)"), partialMaskReplacement) }, conditionalPattern("securelogging_mask_api_key") { - (Pattern.compile("(?i)(api_key\\s*->\\s*)([^,\\s&\\)]+)"), "$1***") + (Pattern.compile("(?i)(api_key\\s*->\\s*)([^,\\s&\\)]+)"), partialMaskReplacement) }, conditionalPattern("securelogging_mask_key") { - (Pattern.compile("(?i)(key[\"']?\\s*[:=]\\s*[\"']?)([^\"',\\s&]+)"), "$1***") + (Pattern.compile("(?i)(key[\"']?\\s*[:=]\\s*[\"']?)([^\"',\\s&]+)"), partialMaskReplacement) }, conditionalPattern("securelogging_mask_key") { - (Pattern.compile("(?i)(key\\s*->\\s*)([^,\\s&\\)]+)"), "$1***") + (Pattern.compile("(?i)(key\\s*->\\s*)([^,\\s&\\)]+)"), partialMaskReplacement) }, conditionalPattern("securelogging_mask_private_key") { - (Pattern.compile("(?i)(private_key[\"']?\\s*[:=]\\s*[\"']?)([^\"',\\s&]+)"), "$1***") + (Pattern.compile("(?i)(private_key[\"']?\\s*[:=]\\s*[\"']?)([^\"',\\s&]+)"), staticReplacement("$1***")) }, conditionalPattern("securelogging_mask_private_key") { - (Pattern.compile("(?i)(private_key\\s*->\\s*)([^,\\s&\\)]+)"), "$1***") + (Pattern.compile("(?i)(private_key\\s*->\\s*)([^,\\s&\\)]+)"), staticReplacement("$1***")) }, // Database conditionalPattern("securelogging_mask_jdbc") { - (Pattern.compile("(?i)(jdbc:[^\\s]+://[^:]+:)([^@\\s]+)(@)"), "$1***$3") + (Pattern.compile("(?i)(jdbc:[^\\s]+://[^:]+:)([^@\\s]+)(@)"), staticReplacement("$1***$3")) }, // Credit card conditionalPattern("securelogging_mask_credit_card") { - (Pattern.compile("\\b([0-9]{4})[\\s-]?([0-9]{4})[\\s-]?([0-9]{4})[\\s-]?([0-9]{3,7})\\b"), "$1-****-****-$4") + (Pattern.compile("\\b([0-9]{4})[\\s-]?([0-9]{4})[\\s-]?([0-9]{4})[\\s-]?([0-9]{3,7})\\b"), staticReplacement("$1-****-****-$4")) }, // Email addresses conditionalPattern("securelogging_mask_email") { - (Pattern.compile("(?i)(email[\"']?\\s*[:=]\\s*[\"']?)([^\"',\\s&]+@[^\"',\\s&]+)"), "$1***@***.***") + (Pattern.compile("(?i)(email[\"']?\\s*[:=]\\s*[\"']?)([^\"',\\s&]+@[^\"',\\s&]+)"), staticReplacement("$1***@***.***")) } ) @@ -129,8 +140,22 @@ object SecureLogging { val msgString = Option(msg).map(_.toString).getOrElse("") if (msgString.isEmpty) return msgString - sensitivePatterns.foldLeft(msgString) { case (acc, (pattern, replacement)) => - pattern.matcher(acc).replaceAll(replacement) + sensitivePatterns.foldLeft(msgString) { case (acc, (pattern, replaceFn)) => + val matcher = pattern.matcher(acc) + val sb = new StringBuffer() + while (matcher.find()) { + val replacement = replaceFn(matcher) + // If the function returns a string with $ references (static replacements), + // use appendReplacement which handles group references. + // Otherwise, quote the replacement to avoid $ interpretation. + if (replacement.contains("$1") || replacement.contains("$2") || replacement.contains("$3") || replacement.contains("$4")) { + matcher.appendReplacement(sb, replacement) + } else { + matcher.appendReplacement(sb, Matcher.quoteReplacement(replacement)) + } + } + matcher.appendTail(sb) + sb.toString } } diff --git a/obp-api/src/main/scala/code/views/MapperViews.scala b/obp-api/src/main/scala/code/views/MapperViews.scala index 01cacb7d69..06897b9780 100644 --- a/obp-api/src/main/scala/code/views/MapperViews.scala +++ b/obp-api/src/main/scala/code/views/MapperViews.scala @@ -6,6 +6,7 @@ import code.api.Constant._ import code.api.util.APIUtil._ import code.api.util.ErrorMessages._ import code.api.util.{APIUtil, CallContext} +import code.model.dataAccess.ResourceUser import code.util.Helper.MdcLoggable import code.views.system.ViewDefinition.create import code.views.system.{AccountAccess, ViewDefinition, ViewPermission} @@ -45,31 +46,100 @@ object MapperViews extends Views with MdcLoggable { ViewDefinition.findCustomView(accountAccess.bank_id.get, accountAccess.account_id.get, accountAccess.view_id.get) } } + + /** + * Batch-load all views for a list of AccountAccess records in minimal DB queries, + * instead of one query per AccountAccess row (N+1 problem). + * + * Returns a list of (AccountAccess, ViewDefinition) pairs for access records + * that have a valid, private view. + */ + private def batchLoadViewsForAccountAccess( + accountAccessList: List[AccountAccess] + ): List[(AccountAccess, ViewDefinition)] = { + if (accountAccessList.isEmpty) return Nil + + // 1. Separate system vs custom view access + val (systemAccessList, customAccessList) = accountAccessList.partition(a => isValidSystemViewId(a.view_id.get)) + + // 2. Batch-load system views: one query per distinct view_id (small fixed set). + // We cache which view_ids exist to avoid repeated DB lookups, but we must call + // findSystemView per access record because the returned ViewDefinition is mutable + // and we need to set bank_id/account_id per access record without cross-contamination. + val distinctSystemViewIds = systemAccessList.map(_.view_id.get).distinct + val validSystemViewIds: Set[String] = distinctSystemViewIds + .filter(vid => ViewDefinition.findSystemView(vid).isDefined) + .toSet + + val systemPairs: List[(AccountAccess, ViewDefinition)] = systemAccessList.flatMap { aa => + // Skip view_ids we already know don't exist in the DB (avoids pointless queries) + if (validSystemViewIds.contains(aa.view_id.get)) { + // We must call findSystemView per access record (not cache the ViewDefinition) because + // ViewDefinition is a mutable Mapper object. v.bank_id(...) is a setter that mutates v + // in place — caching one instance and reusing it across access records would cause each + // call to overwrite the previous bank_id/account_id, corrupting earlier results. + // System views are stored without bank_id/account_id (they're generic), so we stamp + // each fresh instance with the specific account's context from the access record. + ViewDefinition.findSystemView(aa.view_id.get).toList.map { v => + (aa, v.bank_id(aa.bank_id.get).account_id(aa.account_id.get)) + } + } else Nil + } + + // 3. Batch-load custom views: one query using ByList on view_id, then filter in memory + val customPairs: List[(AccountAccess, ViewDefinition)] = if (customAccessList.nonEmpty) { + val distinctCustomViewIds = customAccessList.map(_.view_id.get).distinct + val allCustomViews = ViewDefinition.findAll( + By(ViewDefinition.isSystem_, false), + ByList(ViewDefinition.view_id, distinctCustomViewIds) + ) + // Index by (bank_id, account_id, view_id) for fast lookup + val customViewMap: Map[(String, String, String), ViewDefinition] = allCustomViews + .map(v => (v.bank_id.get, v.account_id.get, v.view_id.get) -> v) + .toMap + + customAccessList.flatMap { aa => + customViewMap.get((aa.bank_id.get, aa.account_id.get, aa.view_id.get)).map(v => (aa, v)) + } + } else Nil + + // 4. Combine and filter for private views only + val allPairs = systemPairs ::: customPairs + allPairs.filter { case (_, view) => + view.isPrivate || allowPublicViews + } + } private def getViewsCommonPart(accountAccessList: List[AccountAccess]): List[View] = { - //we need to get views from accountAccess - val views: List[ViewDefinition] = accountAccessList.flatMap(getViewFromAccountAccess).filter( - v => - if (allowPublicViews) { - true // All views - } else { - v.isPrivate == true // Only private views - } - ) - views + batchLoadViewsForAccountAccess(accountAccessList).map(_._2) } def permissions(account : BankIdAccountId) : List[Permission] = { - - val users = AccountAccess.findAll( + // 1. Single query: get all AccountAccess for this account + val allAccountAccess = AccountAccess.findAll( By(AccountAccess.bank_id, account.bankId.value), By(AccountAccess.account_id, account.accountId.value) - ).flatMap(_.user_fk.obj.toList).distinct - - for { - user <- users - } yield { - Permission(user, getViewsForUserAndAccount(user, account)) + ) + + // 2. Batch-load users: one query using ByList instead of N individual FK lookups + val distinctUserFks = allAccountAccess.map(_.user_fk.get).distinct + val usersMap: Map[Long, ResourceUser] = if (distinctUserFks.nonEmpty) { + ResourceUser.findAll(ByList(ResourceUser.id, distinctUserFks)) + .map(u => u.id.get -> u).toMap + } else Map.empty + + // 3. Batch-load views for all access records + val viewPairs = batchLoadViewsForAccountAccess(allAccountAccess) + + // 4. Group views by user FK and build Permission objects + val viewsByUserFk: Map[Long, List[View]] = viewPairs + .groupBy(_._1.user_fk.get) + .map { case (userFk, pairs) => userFk -> pairs.map(_._2: View) } + + distinctUserFks.flatMap { userFk => + usersMap.get(userFk).map { user => + Permission(user, viewsByUserFk.getOrElse(userFk, Nil)) + } } } @@ -548,54 +618,34 @@ object MapperViews extends Views with MdcLoggable { } def privateViewsUserCanAccess(user: User): (List[View], List[AccountAccess]) ={ - val accountAccess = AccountAccess.findAllByUserPrimaryKey(user.userPrimaryKey) - .filter(accountAccess => { - val view = getViewFromAccountAccess(accountAccess) - view.isDefined && view.map(_.isPrivate)==Full(true) - }) - val privateViews = accountAccess.map(getViewFromAccountAccess).flatten.distinct - (privateViews, accountAccess) + val allAccountAccess = AccountAccess.findAllByUserPrimaryKey(user.userPrimaryKey) + val pairs = batchLoadViewsForAccountAccess(allAccountAccess) + (pairs.map(_._2).distinct, pairs.map(_._1)) } def privateViewsUserCanAccess(user: User, viewIds: List[ViewId]): (List[View], List[AccountAccess]) ={ - val accountAccess = AccountAccess.findAll( + val allAccountAccess = AccountAccess.findAll( By(AccountAccess.user_fk, user.userPrimaryKey.value), ByList(AccountAccess.view_id, viewIds.map(_.value)) - ).filter(accountAccess => { - val view = getViewFromAccountAccess(accountAccess) - view.isDefined && view.map(_.isPrivate) == Full(true) - }) - PrivateViewsUserCanAccessCommon(accountAccess) + ) + val pairs = batchLoadViewsForAccountAccess(allAccountAccess) + (pairs.map(_._2), pairs.map(_._1)) } def privateViewsUserCanAccessAtBank(user: User, bankId: BankId): (List[View], List[AccountAccess]) ={ - val accountAccess = AccountAccess.findAll( + val allAccountAccess = AccountAccess.findAll( By(AccountAccess.user_fk, user.userPrimaryKey.value), By(AccountAccess.bank_id, bankId.value) - ).filter(accountAccess => { - val view = getViewFromAccountAccess(accountAccess) - view.isDefined && view.map(_.isPrivate) == Full(true) - }) - PrivateViewsUserCanAccessCommon(accountAccess) + ) + val pairs = batchLoadViewsForAccountAccess(allAccountAccess) + (pairs.map(_._2), pairs.map(_._1)) } def getAccountAccessAtBankThroughView(user: User, bankId: BankId, viewId: ViewId): (List[View], List[AccountAccess]) ={ - val accountAccess = AccountAccess.findAll( + val allAccountAccess = AccountAccess.findAll( By(AccountAccess.user_fk, user.userPrimaryKey.value), By(AccountAccess.bank_id, bankId.value), By(AccountAccess.view_id, viewId.value) - ).filter(accountAccess => { - val view = getViewFromAccountAccess(accountAccess) - view.isDefined && view.map(_.isPrivate) == Full(true) - }) - PrivateViewsUserCanAccessCommon(accountAccess) - } - - private def PrivateViewsUserCanAccessCommon(accountAccess: List[AccountAccess]): (List[ViewDefinition], List[AccountAccess]) = { - val listOfTuples: List[(AccountAccess, Box[ViewDefinition])] = accountAccess.map( - accountAccess => (accountAccess, getViewFromAccountAccess(accountAccess)) - ) - val privateViews = listOfTuples.flatMap( - tuple => tuple._2.map(v => v.bank_id(tuple._1.bank_id.get).account_id(tuple._1.account_id.get)) ) - (privateViews, accountAccess) + val pairs = batchLoadViewsForAccountAccess(allAccountAccess) + (pairs.map(_._2), pairs.map(_._1)) } def privateViewsUserCanAccessForAccount(user: User, bankIdAccountId : BankIdAccountId) : List[View] = { @@ -604,7 +654,8 @@ object MapperViews extends Views with MdcLoggable { bankIdAccountId.accountId, user.userPrimaryKey ) - accountAccess.map(getViewFromAccountAccess).flatten.filter(view => view.isPrivate == true).distinct + val pairs = batchLoadViewsForAccountAccess(accountAccess) + pairs.map(_._2).distinct } diff --git a/obp-api/src/test/scala/code/views/PrivateViewsUserCanAccessTest.scala b/obp-api/src/test/scala/code/views/PrivateViewsUserCanAccessTest.scala new file mode 100644 index 0000000000..afa9d6cf41 --- /dev/null +++ b/obp-api/src/test/scala/code/views/PrivateViewsUserCanAccessTest.scala @@ -0,0 +1,161 @@ +package code.views + +import code.api.Constant +import code.api.Constant.ALL_CONSUMERS +import code.setup.{DefaultUsers, ServerSetup} +import code.views.system.{AccountAccess, ViewDefinition} +import com.openbankproject.commons.model.{AccountId, BankId, BankIdAccountId} +import net.liftweb.common.Full + +class PrivateViewsUserCanAccessTest extends ServerSetup with DefaultUsers { + + override def beforeAll(): Unit = { + super.beforeAll() + } + + override def afterEach(): Unit = { + super.afterEach() + AccountAccess.bulkDelete_!!() + ViewDefinition.bulkDelete_!!() + } + + val bankId1 = BankId("test-bank-1") + val bankId2 = BankId("test-bank-2") + val accountId1 = AccountId("test-account-1") + val accountId2 = AccountId("test-account-2") + val accountId3 = AccountId("test-account-3") + + private def createSystemViewAndGrantAccess(bankId: BankId, accountId: AccountId, viewId: String, user: code.model.dataAccess.ResourceUser): Unit = { + // Ensure the system view exists + MapperViews.getOrCreateSystemViewFromCbs(viewId) + // Grant access + val view = ViewDefinition.findSystemView(viewId).openOrThrowException(s"System view $viewId not found") + MapperViews.grantAccessToSystemView(bankId, accountId, view, user) + } + + feature("privateViewsUserCanAccess") { + + scenario("User with no account access returns empty lists") { + val (views, accountAccess) = MapperViews.privateViewsUserCanAccess(resourceUser1) + views should be(empty) + accountAccess should be(empty) + } + + scenario("User with one system view access returns that view") { + createSystemViewAndGrantAccess(bankId1, accountId1, Constant.SYSTEM_OWNER_VIEW_ID, resourceUser1) + + val (views, accountAccess) = MapperViews.privateViewsUserCanAccess(resourceUser1) + views.size should be(1) + accountAccess.size should be(1) + views.head.viewId.value should equal(Constant.SYSTEM_OWNER_VIEW_ID.toLowerCase()) + accountAccess.head.bank_id.get should equal(bankId1.value) + accountAccess.head.account_id.get should equal(accountId1.value) + } + + scenario("User with access to multiple accounts returns all views") { + createSystemViewAndGrantAccess(bankId1, accountId1, Constant.SYSTEM_OWNER_VIEW_ID, resourceUser1) + createSystemViewAndGrantAccess(bankId1, accountId2, Constant.SYSTEM_OWNER_VIEW_ID, resourceUser1) + createSystemViewAndGrantAccess(bankId2, accountId3, Constant.SYSTEM_OWNER_VIEW_ID, resourceUser1) + + val (views, accountAccess) = MapperViews.privateViewsUserCanAccess(resourceUser1) + accountAccess.size should be(3) + // All three account access records should be for the owner view + accountAccess.map(_.view_id.get).distinct should equal(List(Constant.SYSTEM_OWNER_VIEW_ID.toLowerCase())) + // Check all bank/account combinations are present + val bankAccountPairs = accountAccess.map(a => (a.bank_id.get, a.account_id.get)).toSet + bankAccountPairs should contain((bankId1.value, accountId1.value)) + bankAccountPairs should contain((bankId1.value, accountId2.value)) + bankAccountPairs should contain((bankId2.value, accountId3.value)) + } + + scenario("User with multiple view types on the same account") { + createSystemViewAndGrantAccess(bankId1, accountId1, Constant.SYSTEM_OWNER_VIEW_ID, resourceUser1) + createSystemViewAndGrantAccess(bankId1, accountId1, "accountant", resourceUser1) + + val (views, accountAccess) = MapperViews.privateViewsUserCanAccess(resourceUser1) + accountAccess.size should be(2) + views.map(_.viewId.value).toSet should contain(Constant.SYSTEM_OWNER_VIEW_ID.toLowerCase()) + views.map(_.viewId.value).toSet should contain("accountant") + } + + scenario("Different users have independent access") { + createSystemViewAndGrantAccess(bankId1, accountId1, Constant.SYSTEM_OWNER_VIEW_ID, resourceUser1) + createSystemViewAndGrantAccess(bankId1, accountId2, Constant.SYSTEM_OWNER_VIEW_ID, resourceUser2) + + val (views1, access1) = MapperViews.privateViewsUserCanAccess(resourceUser1) + val (views2, access2) = MapperViews.privateViewsUserCanAccess(resourceUser2) + + access1.size should be(1) + access1.head.account_id.get should equal(accountId1.value) + + access2.size should be(1) + access2.head.account_id.get should equal(accountId2.value) + } + + scenario("Views are distinct even when user has access to same view type across accounts") { + createSystemViewAndGrantAccess(bankId1, accountId1, Constant.SYSTEM_OWNER_VIEW_ID, resourceUser1) + createSystemViewAndGrantAccess(bankId1, accountId2, Constant.SYSTEM_OWNER_VIEW_ID, resourceUser1) + + val (views, accountAccess) = MapperViews.privateViewsUserCanAccess(resourceUser1) + accountAccess.size should be(2) + // The underlying ViewDefinition for system views is the same, so distinct views should be + // based on the view definition, but with bank_id/account_id set per account access + views.size should be >= 1 + } + + scenario("Returned accountAccess entries match returned views") { + createSystemViewAndGrantAccess(bankId1, accountId1, Constant.SYSTEM_OWNER_VIEW_ID, resourceUser1) + createSystemViewAndGrantAccess(bankId1, accountId1, "accountant", resourceUser1) + createSystemViewAndGrantAccess(bankId2, accountId2, Constant.SYSTEM_OWNER_VIEW_ID, resourceUser1) + + val (views, accountAccess) = MapperViews.privateViewsUserCanAccess(resourceUser1) + accountAccess.size should be(3) + + // Every accountAccess view_id should correspond to a returned view + val viewIds = views.map(_.viewId.value).toSet + accountAccess.foreach { aa => + viewIds should contain(aa.view_id.get) + } + } + } + + feature("privateViewsUserCanAccessAtBank") { + + scenario("Filters to only the requested bank") { + createSystemViewAndGrantAccess(bankId1, accountId1, Constant.SYSTEM_OWNER_VIEW_ID, resourceUser1) + createSystemViewAndGrantAccess(bankId2, accountId2, Constant.SYSTEM_OWNER_VIEW_ID, resourceUser1) + + val (views, accountAccess) = MapperViews.privateViewsUserCanAccessAtBank(resourceUser1, bankId1) + accountAccess.size should be(1) + accountAccess.head.bank_id.get should equal(bankId1.value) + } + + scenario("Returns empty for bank with no access") { + createSystemViewAndGrantAccess(bankId1, accountId1, Constant.SYSTEM_OWNER_VIEW_ID, resourceUser1) + + val (views, accountAccess) = MapperViews.privateViewsUserCanAccessAtBank(resourceUser1, bankId2) + views should be(empty) + accountAccess should be(empty) + } + } + + feature("privateViewsUserCanAccessForAccount") { + + scenario("Returns views for the specific account only") { + createSystemViewAndGrantAccess(bankId1, accountId1, Constant.SYSTEM_OWNER_VIEW_ID, resourceUser1) + createSystemViewAndGrantAccess(bankId1, accountId1, "accountant", resourceUser1) + createSystemViewAndGrantAccess(bankId1, accountId2, Constant.SYSTEM_OWNER_VIEW_ID, resourceUser1) + + val views = MapperViews.privateViewsUserCanAccessForAccount(resourceUser1, BankIdAccountId(bankId1, accountId1)) + views.size should be(2) + views.map(_.viewId.value).toSet should equal(Set(Constant.SYSTEM_OWNER_VIEW_ID.toLowerCase(), "accountant")) + } + + scenario("Returns empty for account with no access") { + createSystemViewAndGrantAccess(bankId1, accountId1, Constant.SYSTEM_OWNER_VIEW_ID, resourceUser1) + + val views = MapperViews.privateViewsUserCanAccessForAccount(resourceUser1, BankIdAccountId(bankId1, accountId2)) + views should be(empty) + } + } +} diff --git a/obp-commons/pom.xml b/obp-commons/pom.xml index a229fcf2ae..bd0b365cb1 100644 --- a/obp-commons/pom.xml +++ b/obp-commons/pom.xml @@ -75,6 +75,7 @@ com.google.guava guava + 32.0.0-jre