Support SSO for shared users across sub-organizations.#8156
Open
Yasasr1 wants to merge 4 commits into
Open
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:
📝 WalkthroughWalkthroughThe PR extends the authentication framework to reuse previously satisfied authenticators across organizations. ChangesCross-org session reuse
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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}
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.
![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: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
components/authentication-framework/org.wso2.carbon.identity.application.authentication.framework/src/main/java/org/wso2/carbon/identity/application/authentication/framework/handler/request/impl/DefaultAuthenticationRequestHandler.java (1)
511-528:⚠️ Potential issue | 🟠 Major | ⚡ Quick winUse the resolved root tenant for all session-cache reads/removals too.
The cache writes now use
resolveRootTenantDomain(context), but this method still reads and removes withcontext.getLoginTenantDomain(). For shared/org logins, that can miss the existing root-tenant session and create a duplicate session instead of updating the reused one.🐛 Proposed fix
+ String rootTenantDomain = resolveRootTenantDomain(context); + // When forceAuthenticate is true, it will not check for the existing session and create a new session // for the user. if (!context.isForceAuthenticate()) { @@ if (sessionContextKey != null) { SessionContext loadedSessionContext = FrameworkUtils.getSessionContextFromCache( - sessionContextKey, context.getLoginTenantDomain()); + sessionContextKey, rootTenantDomain); @@ } FrameworkUtils.removeSessionContextFromCache(sessionContextKey, - context.getLoginTenantDomain()); + rootTenantDomain); } } } @@ FrameworkUtils.addSessionContextToCache(sessionContextKey, sessionContext, applicationTenantDomain, - resolveRootTenantDomain(context), organizationId); + rootTenantDomain, organizationId); @@ FrameworkUtils.addSessionContextToCache(sessionContextKey, sessionContext, applicationTenantDomain, - resolveRootTenantDomain(context), orgId); + rootTenantDomain, orgId); @@ SessionContext cachedSessionContext = - FrameworkUtils.getSessionContextFromCache(sessionContextKey, context.getLoginTenantDomain()); + FrameworkUtils.getSessionContextFromCache(sessionContextKey, rootTenantDomain);Also applies to: 666-667, 735-736, 801-802
🤖 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/authentication-framework/org.wso2.carbon.identity.application.authentication.framework/src/main/java/org/wso2/carbon/identity/application/authentication/framework/handler/request/impl/DefaultAuthenticationRequestHandler.java` around lines 511 - 528, In the DefaultAuthenticationRequestHandler.java file, the cache reads and removals for session context are inconsistent with cache writes. The writes use resolveRootTenantDomain(context) but the reads/removals at lines 511-528 use context.getLoginTenantDomain() in both the getSessionContextFromCache and removeSessionContextFromCache method calls. Replace all occurrences of context.getLoginTenantDomain() passed to getSessionContextFromCache and removeSessionContextFromCache with resolveRootTenantDomain(context) at the anchor location (lines 511-528) and at the sibling locations (lines 666-667, 735-736, and 801-802) to ensure consistent tenant domain resolution across all session cache operations and prevent duplicate sessions in shared/org logins.
🧹 Nitpick comments (2)
components/authentication-framework/org.wso2.carbon.identity.application.authentication.framework/src/main/java/org/wso2/carbon/identity/application/authentication/framework/util/FrameworkUtils.java (1)
1466-1471: ⚡ Quick winLog tenant-scoped session cache misses.
This new overload is the tenant-aware session lookup path; when the explicit tenant scope misses, the cross-org SSO flow silently falls back to re-auth. Add a guarded DEBUG log that avoids session keys and user data. As per coding guidelines, significant auth/session operations should include safe, non-repetitive logs.
Suggested logging addition
SessionContextCacheEntry cacheEntry = sessionContextCache.getSessionContextCacheEntry(cacheKey, tenantDomain); if (cacheEntry != null) { sessionContext = cacheEntry.getContext(); boolean isSessionExpired = sessionContextCache.isSessionExpired(cacheKey, cacheEntry); if (isSessionExpired) { triggerSessionExpireEvent(request, context, sessionContext); if (log.isDebugEnabled()) { log.debug("A SESSION_EXPIRE event was fired for the expired session found corresponding " + "to the key: " + cacheKey.getContextId()); } return null; } + } else if (log.isDebugEnabled()) { + log.debug("No session context cache entry found for tenant domain: " + tenantDomain); }🤖 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/authentication-framework/org.wso2.carbon.identity.application.authentication.framework/src/main/java/org/wso2/carbon/identity/application/authentication/framework/util/FrameworkUtils.java` around lines 1466 - 1471, Add a guarded DEBUG log to handle tenant-scoped session cache misses in the SessionContextCache lookup. After the call to sessionContextCache.getSessionContextCacheEntry(cacheKey, tenantDomain), check if the returned cacheEntry is null. When it is null, log a DEBUG-level message (guarded with a check to see if DEBUG logging is enabled) indicating a session cache miss has occurred. The log message should be descriptive about the cache miss without including sensitive data like session keys or user identifiers, following the pattern of safe non-repetitive logging for significant authentication and session operations.Source: Coding guidelines
components/authentication-framework/org.wso2.carbon.identity.application.authentication.framework/src/main/java/org/wso2/carbon/identity/application/authentication/framework/handler/request/impl/DefaultRequestCoordinator.java (1)
1642-1644: ⚡ Quick winLog when org discovery input disables session reuse.
This is a significant auth/SSO decision point, but it returns silently. Add a short DEBUG log before returning so discovery-driven skips are diagnosable without logging request values. As per coding guidelines, “Place logs around significant decision points/functional branches in the auth/SSO/session-merging flows.”
🪵 Proposed log
if (hasOrganizationDiscoveryParameters(request)) { + if (log.isDebugEnabled()) { + log.debug("Organization discovery parameters found. Skipping previous organization session reuse."); + } return Optional.empty(); }🤖 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/authentication-framework/org.wso2.carbon.identity.application.authentication.framework/src/main/java/org/wso2/carbon/identity/application/authentication/framework/handler/request/impl/DefaultRequestCoordinator.java` around lines 1642 - 1644, In the DefaultRequestCoordinator class, the conditional block checking hasOrganizationDiscoveryParameters(request) silently returns Optional.empty() without logging. Add a DEBUG level log statement before the return to document that organization discovery parameters are preventing session reuse. The log should provide context about this significant auth/SSO decision point without including request values, making the flow diagnosable during troubleshooting.Source: Coding guidelines
🤖 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/authentication-framework/org.wso2.carbon.identity.application.authentication.framework/src/main/java/org/wso2/carbon/identity/application/authentication/framework/handler/request/impl/DefaultAuthenticationRequestHandler.java`:
- Around line 553-560: The code at line 556 uses putAll() on
authenticatedOrgData.getAuthenticatedIdPs(), which overwrites existing IdP
entries instead of merging them and can drop previously stored authenticators.
Replace this putAll() call with the merge helper method that is mentioned in the
comment and reused elsewhere in the codebase to properly combine the new IdP
data from context.getCurrentAuthenticatedIdPs() with the existing IdP data in
authenticatedOrgData while preserving all authenticator information.
In
`@components/authentication-framework/org.wso2.carbon.identity.application.authentication.framework/src/main/java/org/wso2/carbon/identity/application/authentication/framework/handler/request/impl/DefaultRequestCoordinator.java`:
- Around line 1231-1239: The org-discovery gating logic needs to be applied to
prevent auto-login when org discovery parameters are present. In the
DefaultRequestCoordinator class, there are two locations where
populateContextWithPreviousAuthenticatedOrganizationSessions() is called (at the
anchor site around lines 1231-1239 and at the sibling site around lines
1316-1321). Both of these call sites must be guarded by the same org-discovery
gating check that is applied in handleOrganizationSessions() to ensure that when
discovery parameters are explicitly provided in the request, the automatic reuse
of authenticated organization sessions does not occur. Add a condition that
checks whether org discovery parameters are present before allowing either of
these fallback branches to execute the
populateContextWithPreviousAuthenticatedOrganizationSessions() calls.
- Around line 1829-1838: The session cache lookup in DefaultRequestCoordinator
is using rootTenantDomain which can be overridden by organization login data,
causing existing sessions to be invisible in non-org or cross-tenant flows. Use
context.getTenantDomain() (the login tenant) as the default cache key for
FrameworkUtils.getSessionContextFromCache() instead of rootTenantDomain to
ensure sessions are properly looked up by the login tenant rather than the
organization's root tenant, while preserving the existing organization login
data logic for other purposes if needed.
---
Outside diff comments:
In
`@components/authentication-framework/org.wso2.carbon.identity.application.authentication.framework/src/main/java/org/wso2/carbon/identity/application/authentication/framework/handler/request/impl/DefaultAuthenticationRequestHandler.java`:
- Around line 511-528: In the DefaultAuthenticationRequestHandler.java file, the
cache reads and removals for session context are inconsistent with cache writes.
The writes use resolveRootTenantDomain(context) but the reads/removals at lines
511-528 use context.getLoginTenantDomain() in both the
getSessionContextFromCache and removeSessionContextFromCache method calls.
Replace all occurrences of context.getLoginTenantDomain() passed to
getSessionContextFromCache and removeSessionContextFromCache with
resolveRootTenantDomain(context) at the anchor location (lines 511-528) and at
the sibling locations (lines 666-667, 735-736, and 801-802) to ensure consistent
tenant domain resolution across all session cache operations and prevent
duplicate sessions in shared/org logins.
---
Nitpick comments:
In
`@components/authentication-framework/org.wso2.carbon.identity.application.authentication.framework/src/main/java/org/wso2/carbon/identity/application/authentication/framework/handler/request/impl/DefaultRequestCoordinator.java`:
- Around line 1642-1644: In the DefaultRequestCoordinator class, the conditional
block checking hasOrganizationDiscoveryParameters(request) silently returns
Optional.empty() without logging. Add a DEBUG level log statement before the
return to document that organization discovery parameters are preventing session
reuse. The log should provide context about this significant auth/SSO decision
point without including request values, making the flow diagnosable during
troubleshooting.
In
`@components/authentication-framework/org.wso2.carbon.identity.application.authentication.framework/src/main/java/org/wso2/carbon/identity/application/authentication/framework/util/FrameworkUtils.java`:
- Around line 1466-1471: Add a guarded DEBUG log to handle tenant-scoped session
cache misses in the SessionContextCache lookup. After the call to
sessionContextCache.getSessionContextCacheEntry(cacheKey, tenantDomain), check
if the returned cacheEntry is null. When it is null, log a DEBUG-level message
(guarded with a check to see if DEBUG logging is enabled) indicating a session
cache miss has occurred. The log message should be descriptive about the cache
miss without including sensitive data like session keys or user identifiers,
following the pattern of safe non-repetitive logging for significant
authentication and session operations.
🪄 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: 75b8a6fd-d5e3-4f82-bfe4-ea526b00508d
📒 Files selected for processing (6)
components/authentication-framework/org.wso2.carbon.identity.application.authentication.framework/src/main/java/org/wso2/carbon/identity/application/authentication/framework/handler/request/impl/DefaultAuthenticationRequestHandler.javacomponents/authentication-framework/org.wso2.carbon.identity.application.authentication.framework/src/main/java/org/wso2/carbon/identity/application/authentication/framework/handler/request/impl/DefaultRequestCoordinator.javacomponents/authentication-framework/org.wso2.carbon.identity.application.authentication.framework/src/main/java/org/wso2/carbon/identity/application/authentication/framework/model/PrimaryAppData.javacomponents/authentication-framework/org.wso2.carbon.identity.application.authentication.framework/src/main/java/org/wso2/carbon/identity/application/authentication/framework/util/FrameworkUtils.javacomponents/authentication-framework/org.wso2.carbon.identity.application.authentication.framework/src/test/java/org/wso2/carbon/identity/application/authentication/framework/handler/request/impl/DefaultAuthenticationRequestHandlerTest.javacomponents/authentication-framework/org.wso2.carbon.identity.application.authentication.framework/src/test/java/org/wso2/carbon/identity/application/authentication/framework/handler/request/impl/DefaultRequestCoordinatorTest.java
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## master #8156 +/- ##
============================================
- Coverage 52.72% 52.43% -0.29%
- Complexity 20757 21226 +469
============================================
Files 2172 2198 +26
Lines 128216 132289 +4073
Branches 18911 19847 +936
============================================
+ Hits 67597 69365 +1768
- Misses 52355 54417 +2062
- Partials 8264 8507 +243
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:
|
|
PR builder started |
|
PR builder completed |
|
PR builder started |
|
PR builder completed |
|
PR builder started |
|
PR builder completed |
|
PR builder started |
|
PR builder completed |
|
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (4)
components/authentication-framework/org.wso2.carbon.identity.application.authentication.framework/src/main/java/org/wso2/carbon/identity/application/authentication/framework/util/FrameworkUtils.java (1)
1342-1349: 🧹 Nitpick | 🔵 Trivial | ⚡ Quick winAdd a guarded DEBUG log for cache-only session writes.
This new method performs a key session-cache write path, but there’s no local trace point for tenant/org routing during troubleshooting.
♻️ Suggested update
public static void addSessionContextToCacheWithoutPersisting(String key, SessionContext sessionContext, String tenantDomain, String loginTenantDomain, String orgId) { SessionContextCacheKey cacheKey = new SessionContextCacheKey(key); SessionContextCacheEntry cacheEntry = buildSessionContextCacheEntry(key, sessionContext, tenantDomain, orgId); + if (log.isDebugEnabled()) { + log.debug("Caching session context without persistence. key: " + key + ", loginTenantDomain: " + + loginTenantDomain + ", orgId: " + orgId); + } SessionContextCache.getInstance().addToCacheWithoutPersisting(cacheKey, cacheEntry, loginTenantDomain); }As per coding guidelines, “flag functions that perform significant operations but lack log statements” and “add logs around major method executions.”
🤖 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/authentication-framework/org.wso2.carbon.identity.application.authentication.framework/src/main/java/org/wso2/carbon/identity/application/authentication/framework/util/FrameworkUtils.java` around lines 1342 - 1349, The method addSessionContextToCacheWithoutPersisting performs a significant session-cache write operation but lacks any logging for troubleshooting purposes. Add a guarded DEBUG log statement at the beginning of this method that logs relevant tenant and organization routing information. The log should include the key, tenantDomain, loginTenantDomain, and orgId parameters to provide visibility into cache-only session writes during debugging. Use a guard condition to check if DEBUG logging is enabled before logging to avoid performance overhead.Source: Coding guidelines
components/authentication-framework/org.wso2.carbon.identity.application.authentication.framework/src/main/java/org/wso2/carbon/identity/application/authentication/framework/handler/request/impl/DefaultAuthenticationRequestHandler.java (1)
890-893: 🧹 Nitpick | 🔵 Trivial | ⚡ Quick winUse WARN instead of ERROR for recoverable tenant domain resolution failures.
The system continues processing other organizations when this exception occurs, making this a recoverable deviation rather than a feature failure. Per logging guidelines, WARN is appropriate for "deviations/expected-but-problematic cases where the system continues."
♻️ Suggested fix
} catch (OrganizationManagementException e) { - log.error("Error while resolving the tenant domain of the organization with id: " + + log.warn("Error while resolving the tenant domain of the organization with id: " + authenticatedOrgId, e); }🤖 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/authentication-framework/org.wso2.carbon.identity.application.authentication.framework/src/main/java/org/wso2/carbon/identity/application/authentication/framework/handler/request/impl/DefaultAuthenticationRequestHandler.java` around lines 890 - 893, The OrganizationManagementException catch block in DefaultAuthenticationRequestHandler.java is using log.error() for a recoverable failure case where the system continues processing other organizations. Change the log.error() call to log.warn() since WARN level is appropriate for deviations where the system continues operating, and ERROR should be reserved for actual feature failures.Source: Coding guidelines
components/authentication-framework/org.wso2.carbon.identity.application.authentication.framework/src/test/java/org/wso2/carbon/identity/application/authentication/framework/handler/request/impl/DefaultRequestCoordinatorTest.java (1)
1115-1131: 🧹 Nitpick | 🔵 Trivial | ⚡ Quick winAdd explicit coverage for the
UserIdNotFoundExceptionbranch.Line 1115 currently validates only the
OrganizationManagementExceptionpath, butisAuthenticatedUserSharedToAccessingOrg(...)also handlesUserIdNotFoundExceptionby returningfalse. Add a sibling test to pin that behavior and prevent regressions.🤖 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/authentication-framework/org.wso2.carbon.identity.application.authentication.framework/src/test/java/org/wso2/carbon/identity/application/authentication/framework/handler/request/impl/DefaultRequestCoordinatorTest.java` around lines 1115 - 1131, The test `testIsAuthenticatedUserSharedToAccessingOrgOnException` only covers the OrganizationManagementException path, but the `isAuthenticatedUserSharedToAccessingOrg` method also handles UserIdNotFoundException by returning false. Add a new sibling test method following the same pattern as `testIsAuthenticatedUserSharedToAccessingOrgOnException` that instead mocks the `OrganizationUserSharingService` to throw a `UserIdNotFoundException` when `getUserAssociationOfAssociatedUserByOrgId` is called, verifying that the method still returns false and the exception is properly caught and handled.components/authentication-framework/org.wso2.carbon.identity.application.authentication.framework/src/main/java/org/wso2/carbon/identity/application/authentication/framework/handler/request/impl/DefaultRequestCoordinator.java (1)
1640-1643: 🧹 Nitpick | 🔵 Trivial | 💤 Low valueConsider adding a DEBUG log when skipping session reuse due to org discovery parameters.
This decision point affects SSO behavior and a DEBUG log would aid troubleshooting when session reuse is unexpectedly skipped.
📝 Suggested log addition
if (hasOrganizationDiscoveryParameters(request)) { + if (log.isDebugEnabled()) { + log.debug("Skipping organization session reuse due to presence of organization discovery parameters."); + } return Optional.empty(); }Based on learnings: Per LOG_ENHANCEMENT_GUIDELINES.md, DEBUG logs should be added around meaningful business logic decisions and guarded with
isDebugEnabled().🤖 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/authentication-framework/org.wso2.carbon.identity.application.authentication.framework/src/main/java/org/wso2/carbon/identity/application/authentication/framework/handler/request/impl/DefaultRequestCoordinator.java` around lines 1640 - 1643, Add a DEBUG log statement before the return Optional.empty() call in the hasOrganizationDiscoveryParameters check block. Guard the log with isDebugEnabled() to avoid unnecessary string construction, and include a meaningful message that explains the session reuse is being skipped due to organization discovery parameters. This will help with troubleshooting when SSO behavior is unexpectedly affected by this decision point.Source: Coding guidelines
🤖 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.
Nitpick comments:
In
`@components/authentication-framework/org.wso2.carbon.identity.application.authentication.framework/src/main/java/org/wso2/carbon/identity/application/authentication/framework/handler/request/impl/DefaultAuthenticationRequestHandler.java`:
- Around line 890-893: The OrganizationManagementException catch block in
DefaultAuthenticationRequestHandler.java is using log.error() for a recoverable
failure case where the system continues processing other organizations. Change
the log.error() call to log.warn() since WARN level is appropriate for
deviations where the system continues operating, and ERROR should be reserved
for actual feature failures.
In
`@components/authentication-framework/org.wso2.carbon.identity.application.authentication.framework/src/main/java/org/wso2/carbon/identity/application/authentication/framework/handler/request/impl/DefaultRequestCoordinator.java`:
- Around line 1640-1643: Add a DEBUG log statement before the return
Optional.empty() call in the hasOrganizationDiscoveryParameters check block.
Guard the log with isDebugEnabled() to avoid unnecessary string construction,
and include a meaningful message that explains the session reuse is being
skipped due to organization discovery parameters. This will help with
troubleshooting when SSO behavior is unexpectedly affected by this decision
point.
In
`@components/authentication-framework/org.wso2.carbon.identity.application.authentication.framework/src/main/java/org/wso2/carbon/identity/application/authentication/framework/util/FrameworkUtils.java`:
- Around line 1342-1349: The method addSessionContextToCacheWithoutPersisting
performs a significant session-cache write operation but lacks any logging for
troubleshooting purposes. Add a guarded DEBUG log statement at the beginning of
this method that logs relevant tenant and organization routing information. The
log should include the key, tenantDomain, loginTenantDomain, and orgId
parameters to provide visibility into cache-only session writes during
debugging. Use a guard condition to check if DEBUG logging is enabled before
logging to avoid performance overhead.
In
`@components/authentication-framework/org.wso2.carbon.identity.application.authentication.framework/src/test/java/org/wso2/carbon/identity/application/authentication/framework/handler/request/impl/DefaultRequestCoordinatorTest.java`:
- Around line 1115-1131: The test
`testIsAuthenticatedUserSharedToAccessingOrgOnException` only covers the
OrganizationManagementException path, but the
`isAuthenticatedUserSharedToAccessingOrg` method also handles
UserIdNotFoundException by returning false. Add a new sibling test method
following the same pattern as
`testIsAuthenticatedUserSharedToAccessingOrgOnException` that instead mocks the
`OrganizationUserSharingService` to throw a `UserIdNotFoundException` when
`getUserAssociationOfAssociatedUserByOrgId` is called, verifying that the method
still returns false and the exception is properly caught and handled.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yml
Review profile: CHILL
Plan: Pro
Run ID: d6966382-b1a0-418e-8b4b-47c7ee3172cb
📒 Files selected for processing (6)
components/authentication-framework/org.wso2.carbon.identity.application.authentication.framework/src/main/java/org/wso2/carbon/identity/application/authentication/framework/cache/SessionContextCache.javacomponents/authentication-framework/org.wso2.carbon.identity.application.authentication.framework/src/main/java/org/wso2/carbon/identity/application/authentication/framework/handler/request/impl/DefaultAuthenticationRequestHandler.javacomponents/authentication-framework/org.wso2.carbon.identity.application.authentication.framework/src/main/java/org/wso2/carbon/identity/application/authentication/framework/handler/request/impl/DefaultRequestCoordinator.javacomponents/authentication-framework/org.wso2.carbon.identity.application.authentication.framework/src/main/java/org/wso2/carbon/identity/application/authentication/framework/util/FrameworkUtils.javacomponents/authentication-framework/org.wso2.carbon.identity.application.authentication.framework/src/test/java/org/wso2/carbon/identity/application/authentication/framework/handler/request/impl/DefaultAuthenticationRequestHandlerTest.javacomponents/authentication-framework/org.wso2.carbon.identity.application.authentication.framework/src/test/java/org/wso2/carbon/identity/application/authentication/framework/handler/request/impl/DefaultRequestCoordinatorTest.java
💤 Files with no reviewable changes (1)
- components/authentication-framework/org.wso2.carbon.identity.application.authentication.framework/src/test/java/org/wso2/carbon/identity/application/authentication/framework/handler/request/impl/DefaultAuthenticationRequestHandlerTest.java
|
PR builder started |
|
PR builder completed |
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
Support SSO for shared users across sub-organizations. When a user who is shared into multiple sub-organizations has already authenticated while accessing one sub-org application, the framework now honors that existing session when the same user accesses an application in a different sub-org — instead of forcing a fresh login — provided the user is shared into the accessing organization.
Previously a session was only reused when the accessing organization already had its own entry in the session context's authenticated organization data. This PR extends that so authenticators already satisfied in any of the user's organizations can be carried over to satisfy the current sequence.
Changes
DefaultRequestCoordinatorpopulateContextWithPreviousAuthenticatedOrganizationSessions(...): when the accessing org has no session entry but other orgs do, it walks the effective sequence steps, marks each step already satisfied in another org as authenticated, switches the authenticated user's accessing organization to the current one, and carries the merged authenticated IdP data into the context to enable downstream SSO.isAuthenticatedUserSharedToAccessingOrg(...): guards the above by confirming (viaOrganizationUserSharingService) that the authenticated user actually has a user association in the accessing organization.hasOrganizationDiscoveryParameters(...): skips auto login to the latest accessed org when the request carries org discovery parameters (orgId/orgHandle/org/login_hint).DefaultAuthenticationRequestHandlerAuthenticatedOrgDatais created for it, merging the previous and current authenticated IdPs.mergeAuthenticatedIdPs(...)/mergeAuthenticatedIdPsInto(...): merge authenticated IdP maps, appending only authenticators not already present for a shared IdP name (clones entries to avoid mutating cached data).resolveRootTenantDomain(...): store the session context under the root org tenant domain when available.PrimaryAppData— added atenantDomainfield (getter/setter) so the primary app's tenant can be propagated.FrameworkUtils— added an overloadedgetSessionContextFromCache(request, context, sessionContextKey, tenantDomain)allowing the caller to specify the tenant domain explicitly; the existing method now delegates to it.Tests
resolveRootTenantDomainandmergeAuthenticatedIdPsinDefaultAuthenticationRequestHandlerTest.hasOrganizationDiscoveryParameters,isAuthenticatedUserSharedToAccessingOrg(shared / not-shared / blank-org / no-user / exception paths), andpopulateContextWithPreviousAuthenticatedOrganizationSessions(no org data, user-not-shared no-op, and the carry-over happy path) inDefaultRequestCoordinatorTest.testFindPreviousAuthenticatedSessionto stub the new 4-arggetSessionContextFromCacheoverload.Related issue