Fix sub-org user creation failure when root SMS sender uses client-credential auth#391
Merged
SujanSanjula96 merged 1 commit intoJul 8, 2026
Conversation
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthrough
ChangesSMS Token Refresh Persistence
Estimated code review effort: 3 (Moderate) | ~20 minutes 🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
✨ 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. Comment |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #391 +/- ##
============================================
- Coverage 40.32% 40.30% -0.02%
+ Complexity 673 668 -5
============================================
Files 78 78
Lines 5986 5979 -7
Branches 896 896
============================================
- Hits 2414 2410 -4
+ Misses 3311 3308 -3
Partials 261 261
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
VIHANGAGIT
marked this pull request as ready for review
June 25, 2026 06:01
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
components/notification-sender-config/org.wso2.carbon.identity.notification.sender.tenant.config/src/test/java/org/wso2/carbon/identity/notification/sender/tenant/config/NotificationSenderManagementServiceImplTest.java (2)
295-317: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winCover the stale cached-header path.
Seed the old header before calling
rebuildAuthHeaderWithNewToken; otherwise this test misses the case wheregetAuthHeader()returns a previously cachedBearer old-token.🧪 Proposed test hardening
SMSSenderDTO inputSender = constructClientCredentialSender("old-token"); + inputSender.getAuthentication().getAuthHeader(); Resource inheritedResource = constructClientCredentialResource("old-token");🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@components/notification-sender-config/org.wso2.carbon.identity.notification.sender.tenant.config/src/test/java/org/wso2/carbon/identity/notification/sender/tenant/config/NotificationSenderManagementServiceImplTest.java` around lines 295 - 317, The test for NotificationSenderManagementServiceImpl.rebuildAuthHeaderWithNewToken currently only covers rebuilding from the inherited resource state and misses the stale cached-header path. Before invoking rebuildAuthHeaderWithNewToken, seed the sender’s cached auth header so getAuthHeader() returns the old Bearer token, then verify the method replaces it with the new token. Use the existing helpers and symbols constructClientCredentialSender, constructClientCredentialResource, and retrieveNewAccessToken to keep the test focused on the cached-header behavior.
337-352: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick winAssert the persisted sender carries the refreshed token.
This only verifies persistence was invoked; it would still pass if
updateSMSSenderreceived the old token. Match the argument so the owned-sender persistence contract is covered.🧪 Proposed assertion
import static org.mockito.ArgumentMatchers.any; +import static org.mockito.ArgumentMatchers.argThat;- verify(defaultChannelConfigurationHandler, times(1)).updateSMSSender(any(SMSSenderDTO.class)); + verify(defaultChannelConfigurationHandler, times(1)).updateSMSSender(argThat(updatedSender -> + "new-token".equals(updatedSender.getAuthentication().getInternalProperties().get(ACCESS_TOKEN_PROP))));🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@components/notification-sender-config/org.wso2.carbon.identity.notification.sender.tenant.config/src/test/java/org/wso2/carbon/identity/notification/sender/tenant/config/NotificationSenderManagementServiceImplTest.java` around lines 337 - 352, The test for NotificationSenderManagementServiceImpl.rebuildAuthHeaderWithNewToken only verifies that defaultChannelConfigurationHandler.updateSMSSender was called, but it does not assert the persisted SMSSenderDTO contains the refreshed token. Update the verification to capture or match the argument passed to updateSMSSender and assert it carries the new access token, so the persistence contract for the owned sender is covered.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In
`@components/notification-sender-config/org.wso2.carbon.identity.notification.sender.tenant.config/src/main/java/org/wso2/carbon/identity/notification/sender/tenant/config/NotificationSenderManagementServiceImpl.java`:
- Around line 749-751: The refresh path in
NotificationSenderManagementServiceImpl is still returning a potentially cached
auth header because getAuthHeader() may reuse the old value after
buildAuthenticationHeader(). Update the return flow after rebuilding the header
so the method returns the freshly constructed header object from authentication,
and make sure the logic around ACCESS_TOKEN_PROP, buildAuthenticationHeader(),
and getAuthHeader() does not allow an expired bearer token to slip through.
---
Nitpick comments:
In
`@components/notification-sender-config/org.wso2.carbon.identity.notification.sender.tenant.config/src/test/java/org/wso2/carbon/identity/notification/sender/tenant/config/NotificationSenderManagementServiceImplTest.java`:
- Around line 295-317: The test for
NotificationSenderManagementServiceImpl.rebuildAuthHeaderWithNewToken currently
only covers rebuilding from the inherited resource state and misses the stale
cached-header path. Before invoking rebuildAuthHeaderWithNewToken, seed the
sender’s cached auth header so getAuthHeader() returns the old Bearer token,
then verify the method replaces it with the new token. Use the existing helpers
and symbols constructClientCredentialSender, constructClientCredentialResource,
and retrieveNewAccessToken to keep the test focused on the cached-header
behavior.
- Around line 337-352: The test for
NotificationSenderManagementServiceImpl.rebuildAuthHeaderWithNewToken only
verifies that defaultChannelConfigurationHandler.updateSMSSender was called, but
it does not assert the persisted SMSSenderDTO contains the refreshed token.
Update the verification to capture or match the argument passed to
updateSMSSender and assert it carries the new access token, so the persistence
contract for the owned sender is covered.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: de10830d-220d-4eab-b2a6-2c114bb6d67e
📒 Files selected for processing (2)
components/notification-sender-config/org.wso2.carbon.identity.notification.sender.tenant.config/src/main/java/org/wso2/carbon/identity/notification/sender/tenant/config/NotificationSenderManagementServiceImpl.javacomponents/notification-sender-config/org.wso2.carbon.identity.notification.sender.tenant.config/src/test/java/org/wso2/carbon/identity/notification/sender/tenant/config/NotificationSenderManagementServiceImplTest.java
VIHANGAGIT
force-pushed
the
fix/sms-client-credential-token-refresh-suborg
branch
3 times, most recently
from
June 30, 2026 11:43
4775497 to
131a846
Compare
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In
`@components/notification-sender-config/org.wso2.carbon.identity.notification.sender.tenant.config/src/main/java/org/wso2/carbon/identity/notification/sender/tenant/config/NotificationSenderManagementServiceImpl.java`:
- Line 44: The inherited sub-org sender flow is still persisting changes by
calling updateSMSSender after switching to the primary tenant, which should only
refresh the token for the current request. Update the logic in
NotificationSenderManagementServiceImpl so the primary-tenant lookup path does
not write the sender back for inherited senders; instead, keep the refreshed
token in-flight and return it without invoking the persistence/update path. Use
the existing sender-handling flow around the sub-org/primary-tenant switch to
ensure only root-owned senders are persisted.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: 3dd038bb-8914-41cf-9abf-82753f896401
📒 Files selected for processing (1)
components/notification-sender-config/org.wso2.carbon.identity.notification.sender.tenant.config/src/main/java/org/wso2/carbon/identity/notification/sender/tenant/config/NotificationSenderManagementServiceImpl.java
VIHANGAGIT
force-pushed
the
fix/sms-client-credential-token-refresh-suborg
branch
2 times, most recently
from
July 3, 2026 03:11
b100c33 to
03124f5
Compare
VIHANGAGIT
force-pushed
the
fix/sms-client-credential-token-refresh-suborg
branch
from
July 3, 2026 06:11
03124f5 to
89b7b66
Compare
Thisara-Welmilla
previously approved these changes
Jul 3, 2026
VIHANGAGIT
force-pushed
the
fix/sms-client-credential-token-refresh-suborg
branch
2 times, most recently
from
July 6, 2026 10:07
ad6fdbf to
1bd23ba
Compare
VIHANGAGIT
force-pushed
the
fix/sms-client-credential-token-refresh-suborg
branch
from
July 6, 2026 10:21
ae6951f to
993d3a3
Compare
SujanSanjula96
approved these changes
Jul 6, 2026
|
PR builder started |
Thisara-Welmilla
approved these changes
Jul 7, 2026
|
PR builder completed |
jenkins-is-staging
approved these changes
Jul 7, 2026
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/28842020848
SujanSanjula96
approved these changes
Jul 8, 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
4 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.
Purpose
Related Issue
Root cause
In
NotificationSenderManagementServiceImpl.rebuildAuthHeaderWithNewToken:getSMSSender(name), which is org-aware and falls back to the primary/root tenant.updateSMSSender(...)to persist it.updateSMSSenderlooks the resource up with the current-tenant-onlygetPublisherResource(name)and throwsERROR_CODE_NO_RESOURCE_EXISTSwhen it is absent.Changes
1. Org-aware token persistence (
NotificationSenderManagementServiceImpl)The refresh path now persists the refreshed token to the tenant that owns the sender resource,
via a new private overload
updateSMSSender(SMSSenderDTO, boolean inheritTenantSettings)(mirrorsgetSMSSender(name, boolean)):inheritTenantSettings && isOrganization && resource absent locally) → resolve the primary (root) tenant and runupdateSMSSenderinside aFrameworkUtilstenant flow switched to that tenant, so the refreshed token is written to the root resource.ERROR_CODE_NO_RESOURCE_EXISTS.A tenant flow is required because
ConfigurationManagerexposes a tenant-scoped read(
getResourceByTenantId) but no tenant-scoped write — all writes target the current carbon-contexttenant. SMS stores the token as a plain resource attribute (
auth.internal.accessToken), so thewrite is a straightforward
replaceResourcein the primary tenant.Persisting to root (rather than discarding) keeps the token cache coherent and shared across
sub-orgs: a later send reads the refreshed token via
getSMSSender's existing primary-tenantfallback, avoiding a fresh token mint on every OTP send.
2. Auth header cache refresh (
Authentication)buildAuthenticationHeader()now updates the cachedauthHeaderfield before returning it.Previously it built a fresh header but left the cache untouched, so
getAuthHeader()(used by the SMSprovider to attach the
Bearerheader on each send) could return a stale, cached token after arefresh. Now the cache is refreshed on every build, so a subsequent
getAuthHeader()returns thefreshly built header. Returned values for all auth types are unchanged — only the cache-update side effect is added.
Summary by CodeRabbit