[OB4] Fixes username resolving issue for secondary user store#912
Merged
[OB4] Fixes username resolving issue for secondary user store#912
Conversation
![wso2-engineering[bot]](https://avatars.githubusercontent.com/in/1856828?s=60&v=4){kind=small}
Comment on lines
146
to
+148
| 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)) { |
Contributor
There was a problem hiding this comment.
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)) { |
Comment on lines
+154
to
155
| String extractedUUID = extractUUID(userID); | ||
| try { |
Contributor
There was a problem hiding this comment.
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 { |
Contributor
There was a problem hiding this comment.
AI Agent Log Improvement Checklist
- The log-related comments and suggestions in this review were generated by an AI tool to assist with identifying potential improvements. Purpose of reviewing the code for log improvements is to improve the troubleshooting capabilities of our products.
- Please make sure to manually review and validate all suggestions before applying any changes. Not every code suggestion would make sense or add value to our purpose. Therefore, you have the freedom to decide which of the suggestions are helpful.
✅ Before merging this pull request:
- Review all AI-generated comments for accuracy and relevance.
- Complete and verify the table below. We need your feedback to measure the accuracy of these suggestions and the value they add. If you are rejecting a certain code suggestion, please mention the reason briefly in the suggestion for us to capture it.
| Comment | Accepted (Y/N) | Reason |
|---|---|---|
| #### Log Improvement Suggestion No: 1 | ||
| #### Log Improvement Suggestion No: 2 |
WalkthroughUUID utility methods in a financial services accelerator are refactored to check for UUID presence anywhere in strings instead of only at the start, and a new extraction utility is introduced. The test suite is updated to reflect the method renames and includes comprehensive test coverage for the new extraction functionality. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Tip Try Coding Plans. Let us write the prompt for your AI agent so you can ship faster (with fewer bugs). Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
financial-services-accelerator/components/org.wso2.financial.services.accelerator.common/src/test/java/org/wso2/financial/services/accelerator/common/test/util/FinancialServicesUtilsTest.java (1)
77-103: Add a regression test forresolveUsernameFromUserId(...)itself.These tests validate the regex helpers, but the bug fixed in this PR is in the wrapper that extracts the UUID and delegates to
OAuth2Util. A direct test forSECONDARY/<uuid>@carbon.superwould protect the actual regression path.🤖 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/test/java/org/wso2/financial/services/accelerator/common/test/util/FinancialServicesUtilsTest.java` around lines 77 - 103, Add a unit test in FinancialServicesUtilsTest that verifies resolveUsernameFromUserId correctly handles the "SECONDARY/<uuid>@carbon.super" form and delegates to OAuth2Util: generate a UUID, stub/mock OAuth2Util.getUserFromUserId(...) to return a known username, call FinancialServicesUtils.resolveUsernameFromUserId("SECONDARY/"+uuid+"@carbon.super"), and assert the returned value equals the stubbed username; this directly tests the wrapper method resolveUsernameFromUserId and prevents regressions in the UUID-extraction + delegation path.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@financial-services-accelerator/components/org.wso2.financial.services.accelerator.common/src/main/java/org/wso2/financial/services/accelerator/common/util/FinancialServicesUtils.java`:
- Around line 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.
- Around line 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.
---
Nitpick comments:
In
`@financial-services-accelerator/components/org.wso2.financial.services.accelerator.common/src/test/java/org/wso2/financial/services/accelerator/common/test/util/FinancialServicesUtilsTest.java`:
- Around line 77-103: Add a unit test in FinancialServicesUtilsTest that
verifies resolveUsernameFromUserId correctly handles the
"SECONDARY/<uuid>@carbon.super" form and delegates to OAuth2Util: generate a
UUID, stub/mock OAuth2Util.getUserFromUserId(...) to return a known username,
call
FinancialServicesUtils.resolveUsernameFromUserId("SECONDARY/"+uuid+"@carbon.super"),
and assert the returned value equals the stubbed username; this directly tests
the wrapper method resolveUsernameFromUserId and prevents regressions in the
UUID-extraction + delegation path.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 625c7119-25a8-4b0d-8483-bfd6cb520d20
📒 Files selected for processing (2)
financial-services-accelerator/components/org.wso2.financial.services.accelerator.common/src/main/java/org/wso2/financial/services/accelerator/common/util/FinancialServicesUtils.javafinancial-services-accelerator/components/org.wso2.financial.services.accelerator.common/src/test/java/org/wso2/financial/services/accelerator/common/test/util/FinancialServicesUtilsTest.java
Comment on lines
155
to
159
| 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; |
There was a problem hiding this comment.
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.
Comment on lines
+193
to
+195
| public static boolean containsUUID(String input) { | ||
| Pattern uuidPattern = Pattern.compile(FinancialServicesConstants.UUID_REGEX); | ||
| return uuidPattern.matcher(input).find(); |
There was a problem hiding this comment.
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.
VimukthiRajapaksha
approved these changes
Mar 10, 2026
Ashi1993
approved these changes
Mar 10, 2026
Ashi1993
approved these changes
Mar 11, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
[OB4] Fixes username resolving issue for secondary user store
This PR fixes this issue by searching for a valid UUID pattern in the string and then passing that to the OAuth2Util.resolveUsernameFromUserId method which handles both these scenarios.
Issue link: **
Doc Issue: Optional, link issue from documentation repository
Applicable Labels: Spec, product, version, type (specify requested labels)
Development Checklist
Secure Development Checklist
Testing Checklist
Automation Test Details
Conformance Tests Details
Summary by CodeRabbit
Refactor
Tests