Fix tenant and sub-organization differentiation logic when building the PushNotificationData in push notification handler#384
Merged
Conversation
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (3)
🚧 Files skipped from review as they are similar to previous changes (2)
📝 WalkthroughWalkthroughPush notification handling now uses PrivilegedCarbonContext to detect accessing organization, adds organization detection and primary-tenant-domain utilities, populates primaryTenantDomain in PushNotificationData when available, expands unit tests for organization/non-organization flows, and bumps Carbon kernel and push provider versions in pom.xml. Changes
Sequence Diagram(s)sequenceDiagram
actor Client
participant Handler as PushNotificationHandler
participant Context as PrivilegedCarbonContext
participant Util as NotificationUtil
participant OrgMgr as OrganizationManager
participant Provider as PushProvider
Client->>Handler: handleEvent(event)
Handler->>Context: getThreadLocalCarbonContext()
Context-->>Handler: carbonContext
Handler->>Util: isOrganization(tenantDomain)
Util->>OrgMgr: OrganizationManagementUtil.isOrganization(...)
OrgMgr-->>Util: boolean
Util-->>Handler: isOrganization
alt organization tenant
Handler->>Util: resolveOrganizationId(tenantDomain)
Util->>OrgMgr: resolve organization id
OrgMgr-->>Util: organizationId
Util-->>Handler: organizationId
Handler->>Context: getAccessingOrganizationId()
Context-->>Handler: accessingOrgId
alt accessingOrgId present
Handler->>Util: getPrimaryTenantDomain(organizationId)
Util->>OrgMgr: getPrimaryOrganizationId + resolveTenantDomain(...)
OrgMgr-->>Util: primaryTenantDomain
Util-->>Handler: primaryTenantDomain
end
end
Handler->>Provider: sendNotification(pushNotificationData)
Provider-->>Handler: result
Estimated Code Review Effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly Related PRs
Suggested Reviewers
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 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. Comment |
![wso2-engineering[bot]](https://avatars.githubusercontent.com/in/1856828?s=60&v=4){kind=small}
Comment on lines
+269
to
+271
| String organizationId = null; | ||
| if (!tenantDomain.equals(organizationName)) { | ||
| organizationId = tenantDomain; | ||
| if (NotificationUtil.isOrganization(tenantDomain)) { | ||
| organizationId = NotificationUtil.getOrganizationUUID(tenantDomain); |
There was a problem hiding this comment.
Log Improvement Suggestion No: 1
Suggested change
| String organizationId = null; | |
| if (!tenantDomain.equals(organizationName)) { | |
| organizationId = tenantDomain; | |
| if (NotificationUtil.isOrganization(tenantDomain)) { | |
| organizationId = NotificationUtil.getOrganizationUUID(tenantDomain); | |
| String organizationId = null; | |
| if (NotificationUtil.isOrganization(tenantDomain)) { | |
| log.debug("Tenant domain is an organization. Retrieving organization UUID."); | |
| organizationId = NotificationUtil.getOrganizationUUID(tenantDomain); |
Comment on lines
271
to
274
| organizationId = NotificationUtil.getOrganizationUUID(tenantDomain); | ||
| } else { | ||
| // If tenant user, organizationName is null. | ||
| organizationName = null; |
There was a problem hiding this comment.
Log Improvement Suggestion No: 2
Suggested change
| organizationId = NotificationUtil.getOrganizationUUID(tenantDomain); | |
| } else { | |
| // If tenant user, organizationName is null. | |
| organizationName = null; | |
| organizationId = NotificationUtil.getOrganizationUUID(tenantDomain); | |
| } else { | |
| // If tenant user, organizationName is null. | |
| log.debug("Tenant domain is not an organization. Setting organizationName to null."); | |
| organizationName = null; |
Comment on lines
+878
to
+882
| */ | ||
| public static boolean isOrganization(String tenetDomain) throws IdentityEventException { | ||
|
|
||
| try { | ||
| return OrganizationManagementUtil.isOrganization(tenetDomain); |
There was a problem hiding this comment.
Log Improvement Suggestion No: 3
Suggested change
| */ | |
| public static boolean isOrganization(String tenetDomain) throws IdentityEventException { | |
| try { | |
| return OrganizationManagementUtil.isOrganization(tenetDomain); | |
| public static boolean isOrganization(String tenetDomain) throws IdentityEventException { | |
| if (log.isDebugEnabled()) { | |
| log.debug("Checking if tenant domain belongs to an organization: " + tenetDomain); | |
| } | |
| try { | |
| return OrganizationManagementUtil.isOrganization(tenetDomain); |
Comment on lines
+894
to
+896
| */ | ||
| public static String getOrganizationUUID(String tenantDomain) throws IdentityEventException { | ||
|
|
There was a problem hiding this comment.
Log Improvement Suggestion No: 4
Suggested change
| */ | |
| public static String getOrganizationUUID(String tenantDomain) throws IdentityEventException { | |
| public static String getOrganizationUUID(String tenantDomain) throws IdentityEventException { | |
| if (log.isDebugEnabled()) { | |
| log.debug("Retrieving organization UUID for tenant domain: " + tenantDomain); | |
| } | |
| String organizationUUID = null; |
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 |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #384 +/- ##
============================================
+ Coverage 39.00% 39.21% +0.20%
- Complexity 636 640 +4
============================================
Files 78 78
Lines 5755 5778 +23
Branches 872 874 +2
============================================
+ Hits 2245 2266 +21
- Misses 3263 3266 +3
+ Partials 247 246 -1
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
…ion and add primaryTenantDomain to push notification data if tenant org flow
…organization tenants
VihangaMunasinghe
marked this pull request as ready for review
April 7, 2026 05:58
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (2)
components/event-handler-notification/org.wso2.carbon.identity.event.handler.notification/src/test/java/org/wso2/carbon/identity/event/handler/notification/util/NotificationUtilTest.java (1)
615-623: Remove the orphaned data provider.
getOrganizationUUIDDataProvider()is not referenced by any@Test(dataProvider = ...), so it currently just adds maintenance noise.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@components/event-handler-notification/org.wso2.carbon.identity.event.handler.notification/src/test/java/org/wso2/carbon/identity/event/handler/notification/util/NotificationUtilTest.java` around lines 615 - 623, Remove the orphaned DataProvider method getOrganizationUUIDDataProvider() from the NotificationUtilTest class since it is not referenced by any `@Test`(dataProvider = ...) and only adds maintenance noise; delete the entire method (including its `@DataProvider` annotation and returned Object[][]) or alternatively wire it to a test that actually needs the cases, but prefer removing getOrganizationUUIDDataProvider() to keep the test class clean.components/event-handler-notification/org.wso2.carbon.identity.event.handler.notification/src/main/java/org/wso2/carbon/identity/event/handler/notification/PushNotificationHandler.java (1)
275-283: Reuse the organization ID thathandleEvent()already resolved.
handleEvent()storesORGANIZATION_ID_PLACEHOLDERbefore this private method runs, so resolving the same ID again here adds a secondOrganizationManagercall and another avoidable failure path.♻️ Proposed simplification
- try { - organizationId = NotificationHandlerDataHolder.getInstance() - .getOrganizationManager().resolveOrganizationId(tenantDomain); - } catch (OrganizationManagementException e) { - if (LOG.isDebugEnabled()) { - LOG.debug("Error while resolving organization ID for tenant domain: " + tenantDomain, e); - } - throw new IdentityEventException(e.getMessage(), e); - } + organizationId = (String) eventProperties.get( + NotificationConstants.EmailNotification.ORGANIZATION_ID_PLACEHOLDER); + if (StringUtils.isBlank(organizationId)) { + throw new IdentityEventException("Organization ID is not found in the event properties."); + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@components/event-handler-notification/org.wso2.carbon.identity.event.handler.notification/src/main/java/org/wso2/carbon/identity/event/handler/notification/PushNotificationHandler.java` around lines 275 - 283, The code redundantly calls OrganizationManager.resolveOrganizationId(tenantDomain) inside the private method; instead, reuse the organization ID already resolved and stored by handleEvent() under the ORGANIZATION_ID_PLACEHOLDER key. Modify the private method in PushNotificationHandler to read the organizationId from the event/context using the same ORGANIZATION_ID_PLACEHOLDER (or fall back to the resolveOrganizationId call only if the placeholder is missing) so you avoid an extra OrganizationManager invocation and duplicate failure path.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In
`@components/event-handler-notification/org.wso2.carbon.identity.event.handler.notification/src/main/java/org/wso2/carbon/identity/event/handler/notification/PushNotificationHandler.java`:
- Around line 275-283: The code redundantly calls
OrganizationManager.resolveOrganizationId(tenantDomain) inside the private
method; instead, reuse the organization ID already resolved and stored by
handleEvent() under the ORGANIZATION_ID_PLACEHOLDER key. Modify the private
method in PushNotificationHandler to read the organizationId from the
event/context using the same ORGANIZATION_ID_PLACEHOLDER (or fall back to the
resolveOrganizationId call only if the placeholder is missing) so you avoid an
extra OrganizationManager invocation and duplicate failure path.
In
`@components/event-handler-notification/org.wso2.carbon.identity.event.handler.notification/src/test/java/org/wso2/carbon/identity/event/handler/notification/util/NotificationUtilTest.java`:
- Around line 615-623: Remove the orphaned DataProvider method
getOrganizationUUIDDataProvider() from the NotificationUtilTest class since it
is not referenced by any `@Test`(dataProvider = ...) and only adds maintenance
noise; delete the entire method (including its `@DataProvider` annotation and
returned Object[][]) or alternatively wire it to a test that actually needs the
cases, but prefer removing getOrganizationUUIDDataProvider() to keep the test
class clean.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 9df0c6a7-378a-4296-adbd-8b43d7398c88
📒 Files selected for processing (5)
components/event-handler-notification/org.wso2.carbon.identity.event.handler.notification/src/main/java/org/wso2/carbon/identity/event/handler/notification/PushNotificationHandler.javacomponents/event-handler-notification/org.wso2.carbon.identity.event.handler.notification/src/main/java/org/wso2/carbon/identity/event/handler/notification/util/NotificationUtil.javacomponents/event-handler-notification/org.wso2.carbon.identity.event.handler.notification/src/test/java/org/wso2/carbon/identity/event/handler/notification/PushNotificationHandlerTest.javacomponents/event-handler-notification/org.wso2.carbon.identity.event.handler.notification/src/test/java/org/wso2/carbon/identity/event/handler/notification/util/NotificationUtilTest.javapom.xml
|
PR builder started |
|
PR builder completed |
|
PR builder started |
|
PR builder completed |
jenkins-is-staging
approved these changes
Apr 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/24069955218
ZiyamSanthosh
approved these changes
Apr 7, 2026
ZiyamSanthosh
added a commit
that referenced
this pull request
Apr 8, 2026
[Sync][master -> next][#384]: Fix tenant and sub-organization differentiation logic when building the PushNotificationData in push notification handler
3 tasks
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.
Purpose
The
PushNotificationHandlerincorrectly differentiated between tenant users and sub-organization users by comparingtenantDomainwithorganizationNamewhen building the PushNotificationData object. This string comparison is unreliable because a tenant domain and organization name can differ for legitimate tenant users as well, causing tenant users to be misidentified as organization users and resulting in incorrect organization ID values being passed downstream.GitHub Issue
Related PRs
Note: PRs marked as prerequisite must be merged before and bump the new version here.
Goals
OrganizationManagementUtil.isOrganization()instead of a string comparison.OrganizationManager.resolveOrganizationId()instead of incorrectly using the tenant domain string as the organization ID./t/{tenant}/o/{org}) usingOrganizationManager.getPrimaryOrganizationId()andresolveTenantDomain(), and pass it downstream viaPushNotificationData.primaryTenantDomain.Approach
NotificationUtil:isOrganization(String tenantDomain)— wrapsOrganizationManagementUtil.isOrganization()with proper exception handling.getPrimaryTenantDomain(String organizationId)— resolves the primary organization ID and then its tenant domain viaOrganizationManager.PushNotificationHandler.buildPushNotificationData():NotificationUtil.isOrganization()instead of the flawed!tenantDomain.equals(organizationName)check.OrganizationManager.resolveOrganizationId(tenantDomain).PrivilegedCarbonContext.getAccessingOrganizationId()to determine if this is a tenant-organization path, and if so, resolves and setsprimaryTenantDomainon thePushNotificationDatabuilder.PushNotificationHandler.User stories
As a sub-organization user, push notifications should correctly resolve my organization ID so that organization-specific notification configurations are applied. As a tenant user, the system should not incorrectly treat me as a sub-organization user.
Developer Checklist (Mandatory)
product-isissue to track any behavioral change or migration impact.Release note
Fixed an issue where the push notification handler incorrectly identified tenant users as sub-organization users due to a flawed string comparison, causing incorrect organization ID resolution. Also added primary tenant domain resolution for tenant-organization paths.
Documentation
N/A — This is an internal bug fix with no user-facing configuration or API changes.
Training
N/A
Certification
N/A — No impact on certification exams. This is an internal logic fix.
Marketing
N/A
Automation tests
NotificationUtilTest)testIsOrganization— Parameterized test verifyingtrueandfalsereturn values.testIsOrganizationThrowsException— VerifiesOrganizationManagementExceptionis wrapped asIdentityEventException.testGetPrimaryTenantDomain— Verifies correct tenant domain is returned whengetPrimaryOrganizationIdandresolveTenantDomainsucceed.testGetPrimaryTenantDomainThrowsWhenResolveTenantDomainFails— VerifiesOrganizationManagementServerExceptionis wrapped asIdentityEventException.PushNotificationHandlerTest)testHandleEventOrganizationTenantWithAccessingOrgId— Verifies organization tenant with accessing org ID setsorganizationId,organizationName, andprimaryTenantDomain.testHandleEventOrganizationTenantWithoutAccessingOrgId— Verifies organization tenant without accessing org ID setsorganizationIdandorganizationNamebutprimaryTenantDomainis null.testHandleEventNonOrganizationTenant— Verifies non-organization tenant has nullorganizationId,organizationName, andprimaryTenantDomain.Security checks
Samples
N/A
Migrations (if applicable)
N/A — No migrations required. This is a logic fix with no schema or configuration changes.
Test environment
N/A
Learning
Used
OrganizationManagementUtil.isOrganization()as the authoritative API for checking organization membership,OrganizationManager.resolveOrganizationId()for resolving the organization ID from a tenant domain, andOrganizationManager.getPrimaryOrganizationId()+resolveTenantDomain()for resolving the primary tenant domain of an organization.Summary by CodeRabbit
New Features
Tests
Chores