Skip to content

Commit a9f6fb0

Browse files
simonredfernsimonredfern
authored
Merge pull request #2736 from simonredfern/develop
Account Access performance Improvements and tests. Dynamic Consumer creation from Consent tweaks and logging.
2 parents c426484 + c0a4ea3 commit a9f6fb0

8 files changed

Lines changed: 396 additions & 114 deletions

File tree

‎obp-api/src/main/scala/code/api/OAuth2.scala‎

Lines changed: 3 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -501,15 +501,10 @@ object OAuth2Login extends RestHelper with MdcLoggable {
501501
val sub = getClaim(name = "sub", jwtToken = jwtToken)
502502
val email = getClaim(name = "email", jwtToken = jwtToken)
503503
val name = getClaim(name = "name", jwtToken = jwtToken).orElse(description)
504-
val consumerId = azp match {
505-
case Some(value) if APIUtil.checkIfStringIsUUID(value) => azp
506-
case Some(value) => Some(s"${value}_${APIUtil.generateUUID()}")
507-
case None => Some(APIUtil.generateUUID())
508-
}
509504
Consumers.consumers.vend.getOrCreateConsumer(
510-
consumerId = consumerId, // Use azp as consumer id if it is uuid value
511-
key = Some(Helpers.randomString(40).toLowerCase),
512-
secret = Some(Helpers.randomString(40).toLowerCase),
505+
consumerId = None,
506+
key = None,
507+
secret = None,
513508
aud = aud,
514509
azp = azp,
515510
iss = iss,

‎obp-api/src/main/scala/code/api/openidconnect.scala‎

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -306,9 +306,9 @@ object OpenIdConnect extends OBPRestHelper with MdcLoggable {
306306

307307
private def getOrCreateConsumer(idToken: String, userId: String): Box[Consumer] = {
308308
Consumers.consumers.vend.getOrCreateConsumer(
309-
consumerId=Some(APIUtil.generateUUID()),
310-
Some(Helpers.randomString(40).toLowerCase),
311-
Some(Helpers.randomString(40).toLowerCase),
309+
consumerId=None,
310+
None,
311+
None,
312312
Some(JwtUtil.getAudience(idToken).mkString(",")),
313313
getClaim(name = "azp", idToken = idToken),
314314
JwtUtil.getIssuer(idToken),

‎obp-api/src/main/scala/code/api/util/APIUtil.scala‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -3170,7 +3170,7 @@ object APIUtil extends MdcLoggable with CustomJsonFormats{
31703170
// This is normal for certificate-based validation or anonymous requests
31713171
Empty
31723172
}
3173-
logger.debug(s"getUserAndSessionContextFuture says consumerByConsumerKey is: $consumerByConsumerKey")
3173+
//logger.debug(s"getUserAndSessionContextFuture says consumerByConsumerKey is: $consumerByConsumerKey")
31743174

31753175
val res =
31763176
if (authHeadersWithEmptyValues.nonEmpty) { // Check Authorization Headers Empty Values

‎obp-api/src/main/scala/code/model/OAuth.scala‎

Lines changed: 69 additions & 20 deletions
Original file line numberDiff line numberDiff line change
@@ -394,31 +394,83 @@ object MappedConsumersProvider extends ConsumersProvider with MdcLoggable {
394394
logoUrl: Option[String],
395395
): Box[Consumer] = {
396396

397-
val consumer: Box[Consumer] =
398-
// 1st try to find via UUID issued by OBP-API back end
399-
Consumer.find(By(Consumer.consumerId, consumerId.getOrElse("None"))) or
400-
// 2nd try to find via the pair (azp, iss) issued by External Identity Provider
401-
{
402-
// The azp field in the payload of a JWT (JSON Web Token) represents the Authorized Party.
403-
// 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.
404-
// The pair (azp, iss) is a unique key in case of Client of an Identity Provider
405-
Consumer.find(By(Consumer.azp, azp.getOrElse("None")), By(Consumer.iss, iss.getOrElse("None")))
397+
logger.info(s"getOrCreateConsumer says: BEGIN lookup. Input: consumerId=${consumerId.getOrElse("None")}, azp=${azp.getOrElse("None")}, iss=${iss.getOrElse("None")}, sub=${sub.getOrElse("None")}")
398+
399+
// 1st try: find by consumerId (UUID issued by OBP-API back end)
400+
val byConsumerId = Consumer.find(By(Consumer.consumerId, consumerId.getOrElse("None")))
401+
val consumer: Box[Consumer] = if (byConsumerId.isDefined) {
402+
val c = byConsumerId.openOrThrowException("checked isDefined")
403+
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}")
404+
byConsumerId
405+
} else {
406+
logger.info(s"getOrCreateConsumer says: MISS on lookup 1 (by consumerId=${consumerId.getOrElse("None")}). Trying lookup 2 (by Consumer.key matching azp)...")
407+
408+
// 2nd try: find by consumer key matching azp (pre-registered consumer whose key is the OAuth2 client_id)
409+
// This is checked before (azp, iss) so that a pre-registered consumer takes priority over an auto-created one
410+
val byKeyMatchingAzp = Consumer.find(By(Consumer.key, azp.getOrElse("None")))
411+
if (byKeyMatchingAzp.isDefined) {
412+
val c = byKeyMatchingAzp.openOrThrowException("checked isDefined")
413+
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}")
414+
// Transitional cleanup: before the duplicate-consumer fix, OAuth2/OIDC flows could auto-create
415+
// consumers that now conflict with the pre-registered one we just found. Clear the stale consumer's
416+
// azp/iss/sub so we can populate those fields on the pre-registered consumer without a unique
417+
// constraint violation. This block can be removed once all environments have been cleaned up.
418+
val conflicting = Consumer.find(By(Consumer.azp, azp.getOrElse("None")), By(Consumer.iss, iss.getOrElse("None")))
419+
for (stale <- conflicting) {
420+
if (stale.id.get != c.id.get) {
421+
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}")
422+
stale.azp(APIUtil.generateUUID())
423+
stale.sub(APIUtil.generateUUID())
424+
stale.saveMe()
425+
logger.info(s"getOrCreateConsumer says: Cleared stale consumer. Now: consumerId=${stale.consumerId.get}, azp=${stale.azp.get}, sub=${stale.sub.get}")
426+
}
427+
}
428+
// End of transitional cleanup block
429+
logger.info(s"getOrCreateConsumer says: Updating azp/iss/sub on pre-registered consumer so future lookups also match by (azp, iss)...")
430+
// Populate azp, iss, sub on the existing consumer so future lookups can also find it by (azp, iss)
431+
for (found <- byKeyMatchingAzp) {
432+
azp.foreach(v => found.azp(v))
433+
iss.foreach(v => found.iss(v))
434+
sub.foreach(v => found.sub(v))
435+
found.saveMe()
436+
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}")
437+
}
438+
byKeyMatchingAzp
439+
} else {
440+
logger.info(s"getOrCreateConsumer says: MISS on lookup 2 (no consumer has key=${azp.getOrElse("None")}). Trying lookup 3 (by azp+iss pair)...")
441+
442+
// 3rd try: find by (azp, iss) pair issued by External Identity Provider
443+
// The azp field in a JWT represents the Authorized Party (OAuth 2.0 / OpenID Connect client application).
444+
// The pair (azp, iss) is a unique key in case of Client of an Identity Provider
445+
val byAzpIss = Consumer.find(By(Consumer.azp, azp.getOrElse("None")), By(Consumer.iss, iss.getOrElse("None")))
446+
if (byAzpIss.isDefined) {
447+
val c = byAzpIss.openOrThrowException("checked isDefined")
448+
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}")
449+
byAzpIss
450+
} else {
451+
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")})")
452+
Empty
406453
}
454+
}
455+
}
407456
consumer match {
408457
case Full(c) => Full(c)
409458
case Failure(msg, t, c) => Failure(msg, t, c)
410459
case ParamFailure(x,y,z,q) => ParamFailure(x,y,z,q)
411460
case Empty =>
412461
tryo {
413462
val c = Consumer.create
414-
key match {
415-
case Some(v) => c.key(v)
416-
case None =>
417-
}
418-
secret match {
419-
case Some(v) => c.secret(v)
420-
case None =>
463+
val actualKey = key.getOrElse(Helpers.randomString(40).toLowerCase)
464+
val actualSecret = secret.getOrElse(Helpers.randomString(40).toLowerCase)
465+
val actualConsumerId = consumerId.getOrElse {
466+
azp match {
467+
case Some(value) if APIUtil.checkIfStringIsUUID(value) => value
468+
case Some(value) => s"${value}_${APIUtil.generateUUID()}"
469+
case None => APIUtil.generateUUID()
470+
}
421471
}
472+
c.key(actualKey)
473+
c.secret(actualSecret)
422474
aud match {
423475
case Some(v) => c.aud(v)
424476
case None =>
@@ -480,10 +532,7 @@ object MappedConsumersProvider extends ConsumersProvider with MdcLoggable {
480532
case Some(v) => c.logoUrl(v)
481533
case None =>
482534
}
483-
consumerId match {
484-
case Some(v) => c.consumerId(v)
485-
case None =>
486-
}
535+
c.consumerId(actualConsumerId)
487536
val createdConsumer = c.saveMe()
488537
// In case we use Hydra ORY as Identity Provider we create corresponding client at Hydra side a well
489538
if(integrateWithHydra) createHydraClient(createdConsumer)

‎obp-api/src/main/scala/code/util/SecureLogging.scala‎

Lines changed: 54 additions & 29 deletions
Original file line numberDiff line numberDiff line change
@@ -2,7 +2,7 @@ package code.util
22

33
import code.api.util.APIUtil
44

5-
import java.util.regex.Pattern
5+
import java.util.regex.{Matcher, Pattern}
66
import scala.collection.mutable
77

88
/**
@@ -21,98 +21,109 @@ object SecureLogging {
2121
private def conditionalPattern(
2222
prop: String,
2323
defaultValue: Boolean = true
24-
)(pattern: => (Pattern, String)): Option[(Pattern, String)] = {
24+
)(pattern: => (Pattern, Matcher => String)): Option[(Pattern, Matcher => String)] = {
2525
if (APIUtil.getPropsAsBoolValue(prop, defaultValue)) Some(pattern) else None
2626
}
2727

28+
/** Helper to create a static replacement function from a replacement string */
29+
private def staticReplacement(replacement: String): Matcher => String = _ => replacement
30+
31+
/** Helper to create a partial-mask replacement that shows first 3 and last 3 chars of group 2 */
32+
private def partialMaskReplacement: Matcher => String = m => {
33+
val prefix = m.group(1)
34+
val value = m.group(2)
35+
if (value.length > 6) s"${prefix}${value.take(3)}...${value.takeRight(3)}"
36+
else s"${prefix}***"
37+
}
38+
2839
/**
2940
* Toggleable sensitive patterns.
3041
* Note: The sensitive keywords are defined in APIUtil.sensitiveKeywords.
3142
* When adding new categories here, also update that shared list.
3243
*/
33-
private lazy val sensitivePatterns: List[(Pattern, String)] = {
44+
private lazy val sensitivePatterns: List[(Pattern, Matcher => String)] = {
3445
val patterns = Seq(
3546
// OAuth2 / API secrets
3647
conditionalPattern("securelogging_mask_secret") {
37-
(Pattern.compile("(?i)(secret=)([^,\\s&]+)"), "$1***")
48+
(Pattern.compile("(?i)(secret=)([^,\\s&]+)"), staticReplacement("$1***"))
3849
},
3950
conditionalPattern("securelogging_mask_client_secret") {
40-
(Pattern.compile("(?i)(client_secret[\"']?\\s*[:=]\\s*[\"']?)([^\"',\\s&]+)"), "$1***")
51+
(Pattern.compile("(?i)(client_secret[\"']?\\s*[:=]\\s*[\"']?)([^\"',\\s&]+)"), staticReplacement("$1***"))
4152
},
4253
conditionalPattern("securelogging_mask_client_secret") {
43-
(Pattern.compile("(?i)(client_secret\\s*->\\s*)([^,\\s&\\)]+)"), "$1***")
54+
(Pattern.compile("(?i)(client_secret\\s*->\\s*)([^,\\s&\\)]+)"), staticReplacement("$1***"))
4455
},
4556

4657
// Authorization / Tokens
4758
conditionalPattern("securelogging_mask_authorization") {
48-
(Pattern.compile("(?i)(Authorization:\\s*Bearer\\s+)([^\\s,&]+)"), "$1***")
59+
(Pattern.compile("(?i)(Authorization:\\s*Bearer\\s+)([^\\s,&]+)"), staticReplacement("$1***"))
4960
},
5061
conditionalPattern("securelogging_mask_access_token") {
51-
(Pattern.compile("(?i)(access_token[\"']?\\s*[:=]\\s*[\"']?)([^\"',\\s&]+)"), "$1***")
62+
(Pattern.compile("(?i)(access_token[\"']?\\s*[:=]\\s*[\"']?)([^\"',\\s&]+)"), staticReplacement("$1***"))
5263
},
5364
conditionalPattern("securelogging_mask_access_token") {
54-
(Pattern.compile("(?i)(access_token\\s*->\\s*)([^,\\s&\\)]+)"), "$1***")
65+
(Pattern.compile("(?i)(access_token\\s*->\\s*)([^,\\s&\\)]+)"), staticReplacement("$1***"))
5566
},
5667
conditionalPattern("securelogging_mask_refresh_token") {
57-
(Pattern.compile("(?i)(refresh_token[\"']?\\s*[:=]\\s*[\"']?)([^\"',\\s&]+)"), "$1***")
68+
(Pattern.compile("(?i)(refresh_token[\"']?\\s*[:=]\\s*[\"']?)([^\"',\\s&]+)"), staticReplacement("$1***"))
5869
},
5970
conditionalPattern("securelogging_mask_refresh_token") {
60-
(Pattern.compile("(?i)(refresh_token\\s*->\\s*)([^,\\s&\\)]+)"), "$1***")
71+
(Pattern.compile("(?i)(refresh_token\\s*->\\s*)([^,\\s&\\)]+)"), staticReplacement("$1***"))
6172
},
6273
conditionalPattern("securelogging_mask_id_token") {
63-
(Pattern.compile("(?i)(id_token[\"']?\\s*[:=]\\s*[\"']?)([^\"',\\s&]+)"), "$1***")
74+
(Pattern.compile("(?i)(id_token[\"']?\\s*[:=]\\s*[\"']?)([^\"',\\s&]+)"), staticReplacement("$1***"))
6475
},
6576
conditionalPattern("securelogging_mask_id_token") {
66-
(Pattern.compile("(?i)(id_token\\s*->\\s*)([^,\\s&\\)]+)"), "$1***")
77+
(Pattern.compile("(?i)(id_token\\s*->\\s*)([^,\\s&\\)]+)"), staticReplacement("$1***"))
6778
},
6879
conditionalPattern("securelogging_mask_token") {
69-
(Pattern.compile("(?i)(token[\"']?\\s*[:=]\\s*[\"']?)([^\"',\\s&]+)"), "$1***")
80+
(Pattern.compile("(?i)(token[\"']?\\s*[:=]\\s*[\"']?)([^\"',\\s&]+)"), staticReplacement("$1***"))
7081
},
7182
conditionalPattern("securelogging_mask_token") {
72-
(Pattern.compile("(?i)(token\\s*->\\s*)([^,\\s&\\)]+)"), "$1***")
83+
(Pattern.compile("(?i)(token\\s*->\\s*)([^,\\s&\\)]+)"), staticReplacement("$1***"))
7384
},
7485

7586
// Passwords
7687
conditionalPattern("securelogging_mask_password") {
77-
(Pattern.compile("(?i)(password[\"']?\\s*[:=]\\s*[\"']?)([^\"',\\s&]+)"), "$1***")
88+
(Pattern.compile("(?i)(password[\"']?\\s*[:=]\\s*[\"']?)([^\"',\\s&]+)"), staticReplacement("$1***"))
7889
},
7990
conditionalPattern("securelogging_mask_password") {
80-
(Pattern.compile("(?i)(password\\s*->\\s*)([^,\\s&\\)]+)"), "$1***")
91+
(Pattern.compile("(?i)(password\\s*->\\s*)([^,\\s&\\)]+)"), staticReplacement("$1***"))
8192
},
8293

83-
// API keys
94+
// API keys - use partial masking to show first 3 and last 3 characters
8495
conditionalPattern("securelogging_mask_api_key") {
85-
(Pattern.compile("(?i)(api_key[\"']?\\s*[:=]\\s*[\"']?)([^\"',\\s&]+)"), "$1***")
96+
(Pattern.compile("(?i)(api_key[\"']?\\s*[:=]\\s*[\"']?)([^\"',\\s&]+)"), partialMaskReplacement)
8697
},
8798
conditionalPattern("securelogging_mask_api_key") {
88-
(Pattern.compile("(?i)(api_key\\s*->\\s*)([^,\\s&\\)]+)"), "$1***")
99+
(Pattern.compile("(?i)(api_key\\s*->\\s*)([^,\\s&\\)]+)"), partialMaskReplacement)
89100
},
90101
conditionalPattern("securelogging_mask_key") {
91-
(Pattern.compile("(?i)(key[\"']?\\s*[:=]\\s*[\"']?)([^\"',\\s&]+)"), "$1***")
102+
(Pattern.compile("(?i)(key[\"']?\\s*[:=]\\s*[\"']?)([^\"',\\s&]+)"), partialMaskReplacement)
92103
},
93104
conditionalPattern("securelogging_mask_key") {
94-
(Pattern.compile("(?i)(key\\s*->\\s*)([^,\\s&\\)]+)"), "$1***")
105+
(Pattern.compile("(?i)(key\\s*->\\s*)([^,\\s&\\)]+)"), partialMaskReplacement)
95106
},
96107
conditionalPattern("securelogging_mask_private_key") {
97-
(Pattern.compile("(?i)(private_key[\"']?\\s*[:=]\\s*[\"']?)([^\"',\\s&]+)"), "$1***")
108+
(Pattern.compile("(?i)(private_key[\"']?\\s*[:=]\\s*[\"']?)([^\"',\\s&]+)"), staticReplacement("$1***"))
98109
},
99110
conditionalPattern("securelogging_mask_private_key") {
100-
(Pattern.compile("(?i)(private_key\\s*->\\s*)([^,\\s&\\)]+)"), "$1***")
111+
(Pattern.compile("(?i)(private_key\\s*->\\s*)([^,\\s&\\)]+)"), staticReplacement("$1***"))
101112
},
102113

103114
// Database
104115
conditionalPattern("securelogging_mask_jdbc") {
105-
(Pattern.compile("(?i)(jdbc:[^\\s]+://[^:]+:)([^@\\s]+)(@)"), "$1***$3")
116+
(Pattern.compile("(?i)(jdbc:[^\\s]+://[^:]+:)([^@\\s]+)(@)"), staticReplacement("$1***$3"))
106117
},
107118

108119
// Credit card
109120
conditionalPattern("securelogging_mask_credit_card") {
110-
(Pattern.compile("\\b([0-9]{4})[\\s-]?([0-9]{4})[\\s-]?([0-9]{4})[\\s-]?([0-9]{3,7})\\b"), "$1-****-****-$4")
121+
(Pattern.compile("\\b([0-9]{4})[\\s-]?([0-9]{4})[\\s-]?([0-9]{4})[\\s-]?([0-9]{3,7})\\b"), staticReplacement("$1-****-****-$4"))
111122
},
112123

113124
// Email addresses
114125
conditionalPattern("securelogging_mask_email") {
115-
(Pattern.compile("(?i)(email[\"']?\\s*[:=]\\s*[\"']?)([^\"',\\s&]+@[^\"',\\s&]+)"), "$1***@***.***")
126+
(Pattern.compile("(?i)(email[\"']?\\s*[:=]\\s*[\"']?)([^\"',\\s&]+@[^\"',\\s&]+)"), staticReplacement("$1***@***.***"))
116127
}
117128
)
118129

@@ -129,8 +140,22 @@ object SecureLogging {
129140
val msgString = Option(msg).map(_.toString).getOrElse("")
130141
if (msgString.isEmpty) return msgString
131142

132-
sensitivePatterns.foldLeft(msgString) { case (acc, (pattern, replacement)) =>
133-
pattern.matcher(acc).replaceAll(replacement)
143+
sensitivePatterns.foldLeft(msgString) { case (acc, (pattern, replaceFn)) =>
144+
val matcher = pattern.matcher(acc)
145+
val sb = new StringBuffer()
146+
while (matcher.find()) {
147+
val replacement = replaceFn(matcher)
148+
// If the function returns a string with $ references (static replacements),
149+
// use appendReplacement which handles group references.
150+
// Otherwise, quote the replacement to avoid $ interpretation.
151+
if (replacement.contains("$1") || replacement.contains("$2") || replacement.contains("$3") || replacement.contains("$4")) {
152+
matcher.appendReplacement(sb, replacement)
153+
} else {
154+
matcher.appendReplacement(sb, Matcher.quoteReplacement(replacement))
155+
}
156+
}
157+
matcher.appendTail(sb)
158+
sb.toString
134159
}
135160
}
136161

0 commit comments

Comments
 (0)