Refactor tenant domain handling in TOTP authentication flow to support shared user direct login to organizations.#232
Conversation
![wso2-engineering[bot]](https://avatars.githubusercontent.com/in/1856828?s=60&v=4){kind=small}
| String userTenantDomain = authenticatedUserFromContext.getTenantDomain(); | ||
| String tenantDomain = context.getTenantDomain(); | ||
| if (StringUtils.isBlank(userTenantDomain)) { | ||
| throw new AuthenticationFailedException( |
There was a problem hiding this comment.
Log Improvement Suggestion No: 1
| String userTenantDomain = authenticatedUserFromContext.getTenantDomain(); | |
| String tenantDomain = context.getTenantDomain(); | |
| if (StringUtils.isBlank(userTenantDomain)) { | |
| throw new AuthenticationFailedException( | |
| String userTenantDomain = authenticatedUserFromContext.getTenantDomain(); | |
| String tenantDomain = context.getTenantDomain(); | |
| log.debug("Initiating TOTP authentication for user from tenant domain: " + userTenantDomain); | |
| if (StringUtils.isBlank(userTenantDomain)) { |
| Map<String, String> claims; | ||
| if (isInitialFederationAttempt) { | ||
| claims = TOTPKeyGenerator.generateClaimsForFedUserVerifySecretKey(username, tenantDomain, | ||
| context); | ||
| claims = TOTPKeyGenerator.generateClaimsForFedUserVerifySecretKey(username, |
There was a problem hiding this comment.
Log Improvement Suggestion No: 2
| Map<String, String> claims; | |
| if (isInitialFederationAttempt) { | |
| claims = TOTPKeyGenerator.generateClaimsForFedUserVerifySecretKey(username, tenantDomain, | |
| context); | |
| claims = TOTPKeyGenerator.generateClaimsForFedUserVerifySecretKey(username, | |
| Map<String, String> claims; | |
| if (isInitialFederationAttempt) { | |
| if (log.isDebugEnabled()) { | |
| log.debug("Generating claims for federated user verification for user: " + loggableUsername); | |
| } | |
| claims = TOTPKeyGenerator.generateClaimsForFedUserVerifySecretKey(username, |
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 |
There was a problem hiding this comment.
Pull request overview
Refactors tenant-domain usage in TOTPAuthenticator to correctly support shared-user direct login flows by separating the authenticated user’s tenant domain from the authentication-context tenant domain.
Changes:
- Introduces
userTenantDomain(fromAuthenticatedUser) alongsidetenantDomain(fromAuthenticationContext). - Loads application-authentication configuration using the context tenant domain, while resolving usernames/claims using the user tenant domain.
- Replaces a direct
.equals(...)check withStringUtils.equals(...)for safer super-tenant comparison.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| TOTPAuthenticatorConstants.AUTHENTICATOR_NAME); | ||
| if (!tenantDomain.equals(TOTPAuthenticatorConstants.SUPER_TENANT_DOMAIN)) { | ||
| if (!StringUtils.equals(TOTPAuthenticatorConstants.SUPER_TENANT_DOMAIN, tenantDomain)) { | ||
| IdentityHelperUtil | ||
| .loadApplicationAuthenticationXMLFromRegistry(context, getName(), tenantDomain); | ||
| } |
There was a problem hiding this comment.
tenantDomain is taken from context.getTenantDomain() and can be blank (there are other places in the codebase that explicitly return early when context.getTenantDomain() is blank). In that case, !StringUtils.equals(SUPER_TENANT_DOMAIN, tenantDomain) evaluates true and loadApplicationAuthenticationXMLFromRegistry(...) is invoked with a blank/null tenant domain, which can lead to incorrect config resolution or failures. Please guard against a blank tenantDomain here (e.g., fail fast with a clear AuthenticationFailedException, or fall back to a safe default if that’s the intended behavior) before calling into the registry loader.
| String userTenantDomain = authenticatedUserFromContext.getTenantDomain(); | ||
| String tenantDomain = context.getTenantDomain(); | ||
| if (StringUtils.isBlank(userTenantDomain)) { |
There was a problem hiding this comment.
This refactor introduces two tenant domains (userTenantDomain vs context tenant domain) but the existing unit tests for initiateAuthenticationRequest appear to only exercise the super-tenant/same-tenant path. Please add a test that covers a shared-user scenario where authenticatedUserFromContext.getTenantDomain() differs from context.getTenantDomain(), asserting that application-authentication.xml is loaded using the context tenant while username/claims are resolved using the user tenant (and that no registry load happens when the context tenant is super tenant).
This pull request refactors how tenant domains are handled within the
TOTPAuthenticatorclass to properly handle shared user direct login flows to sub-organizations. The main focus is on distinguishing between the user's tenant domain and the authentication context's tenant domain, ensuring the correct domain is used throughout the authentication process.Tenant domain handling improvements:
userTenantDomain(from the authenticated user) andtenantDomain(from the authentication context), and consistently useuserTenantDomainwhere appropriate.Dependent on
Related issue