Fix SCIM2 /Users filter missing groups when username case differs from DB#4590
Fix SCIM2 /Users filter missing groups when username case differs from DB#4590PasinduYeshan wants to merge 4 commits intowso2:4.12.xfrom
Conversation
|
Warning Rate limit exceeded
To keep reviews running without waiting, you can enable usage-based add-on for your organization. This allows additional reviews beyond the hourly cap. Account admins can enable it under billing. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughImplements store-specific case-sensitivity handling for usernames by reading the Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes 🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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. Review rate limit: 0/1 reviews remaining, refill in 47 minutes and 10 seconds.Comment |
![wso2-engineering[bot]](https://avatars.githubusercontent.com/in/1856828?s=60&v=4){kind=small}
| } | ||
| if (!isCaseSensitiveUsernameForStore(userStore)) { | ||
| try { | ||
| String resolvedUserName = doGetUserNameFromUserID(userID); |
There was a problem hiding this comment.
Log Improvement Suggestion No: 2
| } | |
| if (!isCaseSensitiveUsernameForStore(userStore)) { | |
| try { | |
| String resolvedUserName = doGetUserNameFromUserID(userID); | |
| } | |
| if (!isCaseSensitiveUsernameForStore(userStore)) { | |
| log.debug("Username is case-insensitive, resolving canonical username for userID: " + userID); | |
| try { |
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 | ||
| #### Log Improvement Suggestion No: 3 | ||
| #### Log Improvement Suggestion No: 4 |
![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)
core/org.wso2.carbon.user.core/src/main/java/org/wso2/carbon/user/core/common/AbstractUserStoreManager.java (1)
13931-13943: Extract canonical-username resolution into one helper.The same try/catch resolution block appears twice, which increases drift risk for future fixes. A private helper returning
(resolvedUserName, shouldWriteCache)(or equivalent) would keep behavior consistent across both branches.Also applies to: 13954-13966
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@core/org.wso2.carbon.user.core/src/main/java/org/wso2/carbon/user/core/common/AbstractUserStoreManager.java` around lines 13931 - 13943, Extract the duplicated try/catch that resolves a canonical username into a private helper in AbstractUserStoreManager (e.g., resolveCanonicalUsername(userID, userStore) returning an Optional<String> or a pair like (resolvedUserName, shouldWriteCache)); replace both occurrences around isCaseSensitiveUsernameForStore(...) that call doGetUserNameFromUserID(userID) and update userName only when the helper returns a non-empty result, and preserve existing debug logging by moving the log.debug(...) into the helper's catch handling so behavior remains identical in both branches.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@core/org.wso2.carbon.user.core/src/main/java/org/wso2/carbon/user/core/common/AbstractUserStoreManager.java`:
- Around line 13931-13945: When resolving canonical usernames for
case-insensitive stores, avoid writing cache entries if canonical resolution
fails; modify the block that calls isCaseSensitiveUsernameForStore and
doGetUserNameFromUserID so that addToUserIDCacheOnRead and
addToUserNameCacheOnRead are only invoked when doGetUserNameFromUserID returns a
non-empty resolvedUserName (i.e., successful canonical lookup). Apply the same
guarding change to the other analogous block (lines referenced around
addToUserIDCacheOnRead/addToUserNameCacheOnRead) so both places check the
successful resolution before calling
addToUserIDCacheOnRead/addToUserNameCacheOnRead; use the existing methods
isCaseSensitiveUsernameForStore, doGetUserNameFromUserID,
addToUserIDCacheOnRead, and addToUserNameCacheOnRead to locate and update the
logic.
In
`@core/org.wso2.carbon.user.core/src/main/java/org/wso2/carbon/user/core/hybrid/HybridRoleManager.java`:
- Around line 845-858: The code currently uses findFirst() on userNames to set
matchedUserName, which assigns the DB role row to only one caller key and
ignores other case-variants; change this to fan out the role to all
case-insensitive matches by replacing the findFirst() usage with a search that
collects all names matching equalsIgnoreCase(dbUserName) (e.g.,
userNames.stream().filter(...).collect(...)) and for each matched name add the
role to hybridRoleListOfUsers (create the List if null) instead of only updating
the first matchedUserName; alternatively, you may dedupe userNames up front, but
ensure the logic in the block that reads roleName and updates
hybridRoleListOfUsers applies to every matching key, not just matchedUserName.
---
Nitpick comments:
In
`@core/org.wso2.carbon.user.core/src/main/java/org/wso2/carbon/user/core/common/AbstractUserStoreManager.java`:
- Around line 13931-13943: Extract the duplicated try/catch that resolves a
canonical username into a private helper in AbstractUserStoreManager (e.g.,
resolveCanonicalUsername(userID, userStore) returning an Optional<String> or a
pair like (resolvedUserName, shouldWriteCache)); replace both occurrences around
isCaseSensitiveUsernameForStore(...) that call doGetUserNameFromUserID(userID)
and update userName only when the helper returns a non-empty result, and
preserve existing debug logging by moving the log.debug(...) into the helper's
catch handling so behavior remains identical in both branches.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yml
Review profile: CHILL
Plan: Pro
Run ID: 1ea5b9a7-b38a-401d-8e59-d4754f227c04
📒 Files selected for processing (2)
core/org.wso2.carbon.user.core/src/main/java/org/wso2/carbon/user/core/common/AbstractUserStoreManager.javacore/org.wso2.carbon.user.core/src/main/java/org/wso2/carbon/user/core/hybrid/HybridRoleManager.java
| if (!isCaseSensitiveUsernameForStore(userStore)) { | ||
| try { | ||
| String resolvedUserName = doGetUserNameFromUserID(userID); | ||
| if (StringUtils.isNotEmpty(resolvedUserName)) { | ||
| userName = resolvedUserName; | ||
| } | ||
| } catch (UserStoreException | RuntimeException e) { | ||
| if (log.isDebugEnabled()) { | ||
| log.debug("Could not resolve canonical username for userID; falling back " | ||
| + "to caller-provided username for cache key.", e); | ||
| } | ||
| } | ||
| } | ||
| addToUserIDCacheOnRead(userID, userName, userStore); | ||
| addToUserNameCacheOnRead(userID, userName, userStore); |
There was a problem hiding this comment.
Avoid writing cache entries when canonical username resolution fails.
At Line 13944/Line 13945 and Line 13967/Line 13968, cache writes still happen after fallback to caller-provided casing. In case-insensitive stores, that can reintroduce the same cache-key poisoning this PR is fixing whenever canonical lookup fails.
Proposed fix
- if (!isCaseSensitiveUsernameForStore(userStore)) {
+ boolean shouldWriteCache = true;
+ if (!isCaseSensitiveUsernameForStore(userStore)) {
try {
String resolvedUserName = doGetUserNameFromUserID(userID);
if (StringUtils.isNotEmpty(resolvedUserName)) {
userName = resolvedUserName;
+ } else {
+ shouldWriteCache = false;
}
} catch (UserStoreException | RuntimeException e) {
+ shouldWriteCache = false;
if (log.isDebugEnabled()) {
log.debug("Could not resolve canonical username for userID; falling back "
+ "to caller-provided username for cache key.", e);
}
}
}
- addToUserIDCacheOnRead(userID, userName, userStore);
- addToUserNameCacheOnRead(userID, userName, userStore);
+ if (shouldWriteCache) {
+ addToUserIDCacheOnRead(userID, userName, userStore);
+ addToUserNameCacheOnRead(userID, userName, userStore);
+ }
return userID;Also applies to: 13954-13968
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@core/org.wso2.carbon.user.core/src/main/java/org/wso2/carbon/user/core/common/AbstractUserStoreManager.java`
around lines 13931 - 13945, When resolving canonical usernames for
case-insensitive stores, avoid writing cache entries if canonical resolution
fails; modify the block that calls isCaseSensitiveUsernameForStore and
doGetUserNameFromUserID so that addToUserIDCacheOnRead and
addToUserNameCacheOnRead are only invoked when doGetUserNameFromUserID returns a
non-empty resolvedUserName (i.e., successful canonical lookup). Apply the same
guarding change to the other analogous block (lines referenced around
addToUserIDCacheOnRead/addToUserNameCacheOnRead) so both places check the
successful resolution before calling
addToUserIDCacheOnRead/addToUserNameCacheOnRead; use the existing methods
isCaseSensitiveUsernameForStore, doGetUserNameFromUserID,
addToUserIDCacheOnRead, and addToUserNameCacheOnRead to locate and update the
logic.
There was a problem hiding this comment.
The fallback path matches pre-PR behavior, so no regression. The catch only fires on NotImplementedException from custom stores, transient DB errors, or a delete-race. Skipping the cache write would mean a custom store that always fails resolution gets no cache benefit at all.
There was a problem hiding this comment.
@PasinduYeshan, thanks for the detailed explanation — I should mention I'm replying only to your latest comment, as the earlier conversation was too long to be provided to me.
Your reasoning is well-founded:
- No regression on fallback — using the caller-provided
userNamematches the pre-PR cache-write behavior exactly, so any code path that worked before still works. - Narrow catch surface — the three failure modes you describe (
NotImplementedExceptionfrom custom stores, transient DB errors, delete-race) are all cases where the resolution attempt was "best-effort" anyway, and silently swallowing them (with a debug log) is appropriate. - Cache utility for non-implementing stores — skipping
addToUserIDCacheOnRead/addToUserNameCacheOnReadentirely on failure would permanently deprive custom stores (that never implementdoGetUserNameFromUserID) of any caching benefit, which is a worse trade-off than a slightly imprecise cache key.
The design is a reasonable "try to improve, but do no harm" pattern. The concern is acknowledged and understood.
✏️ Learnings added
Learnt from: PasinduYeshan
Repo: wso2/carbon-kernel PR: 4590
File: core/org.wso2.carbon.user.core/src/main/java/org/wso2/carbon/user/core/common/AbstractUserStoreManager.java:13931-13945
Timestamp: 2026-04-29T11:19:11.292Z
Learning: In `AbstractUserStoreManager.getUserIDFromUserName` (file: `core/org.wso2.carbon.user.core/src/main/java/org/wso2/carbon/user/core/common/AbstractUserStoreManager.java`), the catch block around `doGetUserNameFromUserID` for case-insensitive stores is intentional. The fallback to the caller-provided username matches pre-PR behavior (no regression). The catch fires only for edge cases: `NotImplementedException` from custom stores, transient DB errors, or delete-race conditions. Skipping the cache write on failure is intentionally avoided because custom stores that never implement `doGetUserNameFromUserID` would lose all caching benefit.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
|
PR builder started |
| String dbUserName = resultSet.getString(1); | ||
| String matchedUserName; | ||
| if (userNames.contains(dbUserName)) { | ||
| matchedUserName = dbUserName; | ||
| } else if (!caseSensitive) { | ||
| matchedUserName = userNames.stream() | ||
| .filter(name -> name != null && name.equalsIgnoreCase(dbUserName)) | ||
| .findFirst() | ||
| .orElse(null); | ||
| } else { | ||
| matchedUserName = null; | ||
| } | ||
| if (matchedUserName == null) { |
There was a problem hiding this comment.
As discussed offline with @sadilchamishka , this secondary fix will be removed since the cache fix already addresses the reported issue. Callers are expected to provide the correct usernames.
…tion The cache canonicalisation in AbstractUserStoreManager.getUserIDFromUserName addresses the root cause; the HybridRoleManager post-filter change is no longer needed for the SCIM2 /Users filter scenario. Restoring the original case-sensitive contains() and adding the abstract test for the cache fix. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@core/org.wso2.carbon.user.core/src/test/java/org/wso2/carbon/user/core/common/UserIdResolverCacheCaseTest.java`:
- Around line 58-63: The test leaves an InputStream open (created around line
~77) and only closes the DB pool on the happy path; modify the test so resources
are always cleaned up: ensure the InputStream used by
initRealmStuff()/doTestCacheCaseCorrectness is opened with try-with-resources
(or explicitly closed in a finally) and move
DatabaseUtil.closeDatabasePoolConnection() into a finally block or an `@After`
teardown method so it runs even when setup or assertions throw; update
testGetUserIDFromUserNameCachesDBStoredCase, initRealmStuff, or the helper that
opens the stream to reference the stream variable and guarantee closure, and
ensure DatabaseUtil.closeDatabasePoolConnection is invoked unconditionally.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yml
Review profile: CHILL
Plan: Pro
Run ID: ecddd178-5fb6-4107-9daf-2fd71cbbf833
📒 Files selected for processing (1)
core/org.wso2.carbon.user.core/src/test/java/org/wso2/carbon/user/core/common/UserIdResolverCacheCaseTest.java
|
PR builder completed |
jenkins-is-staging
left a comment
jenkins-is-staging
left a comment
There was a problem hiding this comment.
Approving the pull request based on the successful pr build https://github.com/wso2/product-is/actions/runs/25150542493
Renames the helper to isCaseInsensitiveUsernameForStore and inverts the return value so call sites no longer need a negation. Also removes the UserIdResolverCacheCaseTest as agreed. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
|
PR builder started |
3 participants
Summary
differed from the DB-stored case (e.g. ["test"] vs DB "TEST"), even though the SQL is
case-insensitive — the Java userNames.contains(...) check is case-sensitive.
caller-provided username instead of the DB-resolved canonical value. Any case-sensitive
consumer of the cache (HybridRoleManager, /scim2/Me, etc.) silently misbehaved afterwards.
The combination caused SCIM /Users?filter=… responses to omit groups after a single auth
call with a wrong-case username.
Changes
AbstractUserStoreManager.getUserIDFromUserName — after resolving the userID, re-resolve
the canonical username via doGetUserNameFromUserID(userID) and use that as the cache key.
Applied to both branches (unique-ID enabled, and the doGetUserClaimValues claim fallback).
this bug, so the extra call is skipped there.
any failure (custom store without the lookup, transient DB error, missing-user race)
falls back to pre-fix behavior instead of breaking auth.
defaulting to case-sensitive — same convention as the 5 existing helpers in the codebase.
HybridRoleManager.getHybridRoleListOfUsers — Java post-filter now mirrors the SQL
semantics:
attribution if two users differ only by case).
so the existing second loop finds the entry.
the post-filter.
Related Issues