Repository navigation
Set the error code on user store exceptions thrown for non existing users - #4641
Conversation
|
Navigate logical layers of code changes, visualize relationships, and explore their blast radius. No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configuration
📒 Files selected for processing (2)
Included review availability: This review used your included allowance. Your plan provides up to 2 included reviews per hour; 1 remain after this review. 📝 SummarySummary by CodeRabbit
WalkthroughMissing-user exception paths in ChangesMissing-user error handling
Priority: ⬇️ Low Estimated code review effort: 2 (Simple) | ~10 minutes Change: Bug fix Merge Risk: ⚪ Minimal · up to The missing-user exceptions now retain their error codes without changing their messages, and the added tests check both singular and plural user-ID claim lookups. No issue identified here warrants delaying merge. Security Architecture ReviewSecurity architecture risk: 🔵 Low · up to The change preserves existing failure messages and security checks while making missing-user error codes available to callers. No new access path or permission expansion was identified. Production callers may classify these failures differently, and those integrations were not fully verified. Retained concerns Security review detailsSecurity Blast Radius
Trust Boundaries and Controls
🚥 Pre-merge checks | ✅ 4 | ❓ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (4 passed)
Full details: Docstring CoverageExplanation Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 3 functions across 1 files. (1 skipped: 1 too large.) ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
There was a problem hiding this comment.
Copilot review overview
🟢 Approval recommended
The change is a verified, mechanical, message-preserving fix that correctly uses the (message, errorCode) constructor at every non-existing-user throw site and is covered by a new test.
Review effort: Balanced
Findings: None
What changed in this PR
This PR fixes an error-code loss in AbstractUserStoreManager. When a user-store operation fails because the user does not exist, the code built a 30007 (ERROR_CODE_NON_EXISTING_USER) error code but threw new UserStoreException(errorCode + " - " + errorMessage) — the single-argument constructor leaves the exception's errorCode field null, so the code survived only as a message prefix. Callers that branch on getErrorCode() to distinguish a client-side "missing user" from a server-side store failure could not do so. The change passes errorCode to the two-argument UserStoreException(message, errorCode) constructor at the remaining throw sites, making the whole class consistent with the one site that already did this, and adds a focused test. This is a core class in WSO2 identity management, but the change is purely additive to exception construction and leaves message text unchanged.
Changes:
- Pass
errorCodeto the two-argumentUserStoreExceptionconstructor at 19ERROR_CODE_NON_EXISTING_USERthrow sites across the name-based and ID-based user APIs. - Add
AbstractUserStoreManagerErrorCodeTestverifying the error code is carried on the exception forgetUserClaimValuesWithIDandgetUserClaimValueWithID.
| File | Description |
|---|---|
| core/org.wso2.carbon.user.core/src/main/java/org/wso2/carbon/user/core/common/AbstractUserStoreManager.java | Adds errorCode as the second constructor argument at 19 non-existing-user throw sites so getErrorCode() returns 30007 instead of null; message text unchanged. |
| core/org.wso2.carbon.user.core/src/test/java/org/wso2/carbon/user/core/common/AbstractUserStoreManagerErrorCodeTest.java | New TestNG test asserting the error code is propagated on the exception for two representative claim-read methods. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
|
PR builder started |
|
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/37252397041
Purpose
AbstractUserStoreManagerbuilds the non-existing-user error code and then drops it on the floor:UserStoreException(String)leaves theerrorCodefield null, so the code survives only as a prefix of the message. A caller that branches ongetErrorCode()to tell a missing user (a client side condition, usually worth a debug line) from a broken user store (a server side failure worth an error line) cannot do so, and every such guard silently takes the failure branch.callSecurepropagates the null faithfully, so the loss is invisible all the way up the stack.No GitHub issue; found while tracing why a downstream guard on
getErrorCode()never matched.Related Issues
Related PRs
Goals
Make the non-existing-user error code available on the exception, so callers can classify the failure without parsing the message.
Approach
Pass
errorCodeto the two-argumentUserStoreExceptionconstructor at the 19 remainingERROR_CODE_NON_EXISTING_USERthrow sites inAbstractUserStoreManager. One site already did this, so the change makes the class consistent rather than introducing a new convention.Message text is unchanged, so anything matching on the message keeps working. No UI impact.
User stories
As a component that calls the user store, I can tell "this user does not exist" from "the user store failed" by reading
UserStoreException.getErrorCode(), so I can log and handle the two differently.Developer Checklist (Mandatory)
product-isissue to track any behavioral change or migration impact.No
product-isissue was raised. This fills a field that was already supposed to carry the code and does not change any message, control flow, API signature, or persisted data, so there is no behavioural change or migration impact to track. Happy to raise one if a reviewer disagrees.Release note
Non-existing-user failures from
AbstractUserStoreManagernow carry the error code on the exception instead of only inside the message.Documentation
N/A. Internal exception detail, not a documented or configurable behaviour.
Training
N/A.
Certification
N/A. The change is not observable in any product configuration or administration task an exam would cover.
Marketing
N/A.
Automation tests
Unit tests
AbstractUserStoreManagerErrorCodeTestcovers both claim-reading entry points on the non-existing-user path:getUserClaimValuesWithIDandgetUserClaimValueWithID. Each assertsgetErrorCode()equals30007.Verified both ways on
org.wso2.carbon.user.core:Tests run: 19, Failures: 0, Errors: 0AbstractUserStoreManagerreverted and the tests kept:Failures: 2 — expected [30007] but found [null]The remaining 17 throw sites take the identical one-token edit and are not individually covered.
Integration tests
None added. There is no externally observable behaviour to assert: no message, status code, or API response changes.
Security checks
Note the error message already contains the user identifier and is unchanged by this PR; the error code added to the exception is the constant
"30007"and carries no user data.Samples
N/A.
Related PRs
getErrorCode()guard is affected by this. It does not depend on this PR; it handles the message-prefix form so it works against released kernels too.Migrations (if applicable)
N/A. No schema, configuration, or persisted data is touched.
Test environment
carbon-p2-pluginMaven extension and JDK 17 fails withinvalid target release: 21, so 21 is the only version this branch builds on)Learning
The investigation started from a downstream guard on
getErrorCode()that never matched, and the question of whether the fix belonged in the caller or here. ReadingcallSecure(AbstractUserStoreManager:253-268) showed it preserves whatever code the throw site set, which narrowed it to the throw sites themselves. GreppingerrorCode + " - " + errorMessagein this class showed 33 single-argument throws against 2 that pass the code, so the convention already existed and was simply not applied consistently. Scope was limited to theERROR_CODE_NON_EXISTING_USERsites, since those are the ones callers are known to branch on.The JUnit 3 tests under
src/test/java/.../jdbc/are not run by Surefire in this module — the TestNG provider is selected, so those classes reportTests run: 0while the build still passes. The new test is TestNG, andAbstractUserStoreManagerturned out to be testable without a database:mock(UniqueIDJDBCUserStoreManager.class, CALLS_REAL_METHODS), setrealmConfigby reflection, then stub the protectedgetUserStoreWithIDanddoCheckExistingUserWithID.