Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -145,35 +145,54 @@ public static String reduceStringLength(String input, int maxLength) {
@Generated(message = "Ignoring because OAuth2Util cannot be mocked with no constructors")
public static String resolveUsernameFromUserId(String userID) {

if (!startsWithUUID(userID)) {
// If the user ID is not starting with a UUID that means request has sent the username,
if (!containsUUID(userID)) {
Comment on lines 146 to +148
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Log Improvement Suggestion No: 1

Suggested change
public static String resolveUsernameFromUserId(String userID) {
if (!startsWithUUID(userID)) {
// If the user ID is not starting with a UUID that means request has sent the username,
if (!containsUUID(userID)) {
public static String resolveUsernameFromUserId(String userID) {
log.debug("Attempting to resolve username from user ID");
if (!containsUUID(userID)) {

// If the user ID doesn't have a UUID that means request has sent the username,
// return the same user ID as the username.
return userID;
}

String username = null;
String extractedUUID = extractUUID(userID);
try {
Comment on lines +154 to 155
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Log Improvement Suggestion No: 2

Suggested change
String extractedUUID = extractUUID(userID);
try {
String extractedUUID = extractUUID(userID);
if (log.isDebugEnabled()) {
log.debug("Extracted UUID from user ID: " + (extractedUUID != null ? "UUID found" : "UUID not found"));
}
try {

if (userID.contains(FinancialServicesConstants.TENANT_DOMAIN)) {
username = OAuth2Util.resolveUsernameFromUserId(FinancialServicesConstants.TENANT_DOMAIN,
userID.split("@" + FinancialServicesConstants.TENANT_DOMAIN)[0]);
} else {
username = OAuth2Util.resolveUsernameFromUserId(FinancialServicesConstants.TENANT_DOMAIN, userID);
}
return OAuth2Util.resolveUsernameFromUserId(FinancialServicesConstants.TENANT_DOMAIN, extractedUUID);
} catch (UserStoreException e) {
log.debug("Returning null since user ID is not found in the database", e);
return null;
Comment on lines 155 to 159
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major

Don't turn lookup failures into null.

DefaultConsentValidator.java, Lines 95-101 immediately call .equals() on the return value from this helper. Returning null here converts an unresolved secondary-store user into an NPE/500 instead of a normal validation failure. Please keep this method non-null on lookup miss, or surface the failure explicitly.

Possible fix
         try {
             return OAuth2Util.resolveUsernameFromUserId(FinancialServicesConstants.TENANT_DOMAIN, extractedUUID);
         } catch (UserStoreException e) {
-            log.debug("Returning null since user ID is not found in the database", e);
-            return null;
+            log.debug("Unable to resolve username from user ID. Falling back to original identifier.", e);
+            return userID;
         }
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
try {
if (userID.contains(FinancialServicesConstants.TENANT_DOMAIN)) {
username = OAuth2Util.resolveUsernameFromUserId(FinancialServicesConstants.TENANT_DOMAIN,
userID.split("@" + FinancialServicesConstants.TENANT_DOMAIN)[0]);
} else {
username = OAuth2Util.resolveUsernameFromUserId(FinancialServicesConstants.TENANT_DOMAIN, userID);
}
return OAuth2Util.resolveUsernameFromUserId(FinancialServicesConstants.TENANT_DOMAIN, extractedUUID);
} catch (UserStoreException e) {
log.debug("Returning null since user ID is not found in the database", e);
return null;
try {
return OAuth2Util.resolveUsernameFromUserId(FinancialServicesConstants.TENANT_DOMAIN, extractedUUID);
} catch (UserStoreException e) {
log.debug("Unable to resolve username from user ID. Falling back to original identifier.", e);
return userID;
}
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In
`@financial-services-accelerator/components/org.wso2.financial.services.accelerator.common/src/main/java/org/wso2/financial/services/accelerator/common/util/FinancialServicesUtils.java`
around lines 155 - 159, The helper in FinancialServicesUtils currently swallows
UserStoreException and returns null; change it so it never returns null on
lookup failure—either rethrow the UserStoreException (or wrap it in a runtime
exception like IdentityRuntimeException) from the method so callers see an
explicit failure, or return a non-null sentinel (e.g. empty string or a
constant) instead of null so callers like DefaultConsentValidator (which calls
.equals()) won't NPE; update the catch block around
OAuth2Util.resolveUsernameFromUserId to implement one of these two behaviours
and adjust any callers if you choose to surface the exception.

}
return username;
}

/**
* Method to check whether the input string starts with a UUID.
* Method to extract the UUID from the input string.
*
* @param input Input string
* @return Extracted UUID if found, else null
*/
public static String extractUUID(String input) {

if (input == null) {
return null;
}

// Use your existing constant or regex string
Pattern uuidPattern = Pattern.compile(FinancialServicesConstants.UUID_REGEX);
Matcher matcher = uuidPattern.matcher(input);

// 1. Search for the pattern
if (matcher.find()) {
// 2. Return the specific substring that matched
return matcher.group();
}

return null;
}

/**
* Method to check whether the input string has a UUID.
* @param input Input string
* @return true if the input string starts with a UUID
* @return true if the input string has a UUID
*/
public static boolean startsWithUUID(String input) {
Pattern uuidPattern = Pattern.compile("^" + FinancialServicesConstants.UUID_REGEX + ".*$");
return uuidPattern.matcher(input).matches();
public static boolean containsUUID(String input) {
Pattern uuidPattern = Pattern.compile(FinancialServicesConstants.UUID_REGEX);
return uuidPattern.matcher(input).find();
Comment on lines +193 to +195
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟡 Minor

Handle null in containsUUID.

extractUUID(null) already treats null as “no UUID”, but this method throws because Pattern.matcher(...) does not accept null. Since resolveUsernameFromUserId(...) now calls this first, a missing user ID becomes an unexpected NPE.

Possible fix
     public static boolean containsUUID(String input) {
+        if (input == null) {
+            return false;
+        }
         Pattern uuidPattern = Pattern.compile(FinancialServicesConstants.UUID_REGEX);
         return uuidPattern.matcher(input).find();
     }
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In
`@financial-services-accelerator/components/org.wso2.financial.services.accelerator.common/src/main/java/org/wso2/financial/services/accelerator/common/util/FinancialServicesUtils.java`
around lines 193 - 195, containsUUID throws NPE when input is null because
Pattern.matcher(...) doesn't accept null; update the containsUUID(String input)
method to first check for null (and optionally empty/blank) and return false
immediately, then proceed to compile/use FinancialServicesConstants.UUID_REGEX
and matcher; ensure callers like resolveUsernameFromUserId(...) will get false
for missing user IDs instead of an exception.

}

/**
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -74,11 +74,32 @@ public void testReducingStringLength() throws Exception {
}

@Test
public void testStartsWithUUID() {
public void testContainsUUID() {

Assert.assertFalse(FinancialServicesUtils.startsWithUUID("String Body"));
Assert.assertFalse(FinancialServicesUtils.containsUUID("String Body"));

Assert.assertTrue(FinancialServicesUtils.startsWithUUID(UUID.randomUUID().toString()));
Assert.assertTrue(FinancialServicesUtils.containsUUID(UUID.randomUUID().toString()));
Assert.assertTrue(FinancialServicesUtils.containsUUID("abc" + UUID.randomUUID()));
Assert.assertTrue(FinancialServicesUtils.containsUUID(UUID.randomUUID() + "abc"));
Assert.assertTrue(FinancialServicesUtils.containsUUID("abc" + UUID.randomUUID() + "abc"));
Assert.assertTrue(FinancialServicesUtils.containsUUID("abc " + UUID.randomUUID() + " abc"));
}

@Test
public void testExtractUUID() {

String uuid = UUID.randomUUID().toString();

Assert.assertEquals(FinancialServicesUtils
.extractUUID(uuid + "@carbon.super"), uuid);
Assert.assertEquals(FinancialServicesUtils
.extractUUID(uuid + "@carbon.super@carbon.super"), uuid);
Assert.assertEquals(FinancialServicesUtils
.extractUUID("SECONDARY/" + uuid + "@carbon.super@carbon.super"), uuid);
Assert.assertNull(FinancialServicesUtils
.extractUUID("SECONDARY/@carbon.super@carbon.super"));
Assert.assertNull(FinancialServicesUtils
.extractUUID("psu@gold.com"));
}

@Test
Expand Down