Retain tenant qualification for configured server URL in account recovery endpoint#8196
Conversation
📝 WalkthroughWalkthroughThe tenant qualification check in IdentityManagementEndpointUtil's getBasePath method was changed to test for the presence of a tenant context prefix in the configured server URL rather than relying on the prior tenant-check logic. A corresponding data-driven test case was added to validate this behavior. ChangesTenant Qualification Fix
Estimated code review effort3/5 (Medium) — logic change affects tenant-context resolution and is covered by new tests, but the diff is small and localized. Suggested labels: bug, tests Suggested reviewers: (based on repository conventions, assign identity-mgt component maintainers) 🤖 A tiny prefix, once misjudged, 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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 |
|
PR builder started |
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (2)
components/identity-mgt/org.wso2.carbon.identity.mgt.endpoint.util/src/main/java/org/wso2/carbon/identity/mgt/endpoint/util/IdentityManagementEndpointUtil.java (1)
901-909: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConsider a debug log at this tenant-qualification decision point.
This branch was the root cause of the
Invalid tenant 'carbon.super'regression described in the PR. A guarded DEBUG log recording the computedbasePath(and whether the tenant prefix was appended) would aid future diagnosis of similar tenant-resolution issues without touching hot-path cost, since it is guarded byisDebugEnabled().💡 Optional debug log
} else { // A configured server URL is a bare base URL, so qualify it with the tenant here unless it // already carries one. if (StringUtils.isNotBlank(tenantDomain) && !MultitenantConstants.SUPER_TENANT_DOMAIN_NAME .equalsIgnoreCase(tenantDomain) && isEndpointTenantAware && !serverUrl.contains(FrameworkConstants.TENANT_CONTEXT_PREFIX)) { basePath = serverUrl + FrameworkConstants.TENANT_CONTEXT_PREFIX + tenantDomain + context; } else { basePath = serverUrl + context; } + if (log.isDebugEnabled()) { + log.debug("Resolved base path for configured server URL: " + basePath); + } }As per path instructions, "Flag entry/exit points of critical services that don't log their invocation and suggest appropriate logs for those points if possible."
🤖 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/identity-mgt/org.wso2.carbon.identity.mgt.endpoint.util/src/main/java/org/wso2/carbon/identity/mgt/endpoint/util/IdentityManagementEndpointUtil.java` around lines 901 - 909, Add a guarded DEBUG log at the tenant-qualification branch in IdentityManagementEndpointUtil where basePath is computed, so this decision point can be traced when tenant resolution goes wrong. Use the existing tenant-aware URL logic around the serverUrl/context assignment to log the computed basePath and whether the tenant prefix was appended, and keep it behind isDebugEnabled() so there is no hot-path overhead.Source: Path instructions
components/identity-mgt/org.wso2.carbon.identity.mgt.endpoint.util/src/test/java/org/wso2/carbon/identity/mgt/endpoint/util/IdentityManagementEndpointUtilTest.java (1)
376-430: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value
inboundPathstub is inert for this branch; comment is misleading.Since
contextUrlis always non-blank in this data set,getBasePathtakes the configured-serverUrlbranch, which no longer consultsserviceURL.getPath()at all — that's exactly the fix. StubbingserviceURL.getPath()withinboundPathand labelling it "the case that regressed" doesn't actually exercise anything; the assertions pass purely based onserverUrlcontent. Worth simplifying the data provider (drop the unusedinboundPathparameter/stub) or rewording the comment so it doesn't suggest the inbound path still matters for this branch.♻️ Simplify by dropping the unused inboundPath stub
- `@DataProvider`(name = "getBasePathConfiguredServerUrlData") - public Object[][] getBasePathConfiguredServerUrlData() { - - return new Object[][] { - // contextUrl (configured server URL) - // tenantDomain - // isEndpointTenantAware - // inboundPath (stubbed serviceURL.getPath() - already tenant-qualified, the case that regressed) - // expected value - - // A configured server URL is tenant-qualified even when the inbound request path already - // carries the tenant (the fix - the tenant must not be dropped here). - { "https://foo.com", - SAMPLE_TENANT_DOMAIN, - true, - "/t/test.com/api/identity/recovery/v0.9", - "https://foo.com/t/test.com/api/identity/recovery/v0.9" - }, + `@DataProvider`(name = "getBasePathConfiguredServerUrlData") + public Object[][] getBasePathConfiguredServerUrlData() { + + return new Object[][] { + // contextUrl (configured server URL), tenantDomain, isEndpointTenantAware, expected value. + + // Bare configured server URL is qualified with the tenant. + { "https://foo.com", + SAMPLE_TENANT_DOMAIN, + true, + "https://foo.com/t/test.com/api/identity/recovery/v0.9" + }, ... - `@Test`(dataProvider = "getBasePathConfiguredServerUrlData") - public void testGetBasePathConfiguredServerUrl(String contextUrl, String tenantDomain, - boolean isEndpointTenantAware, String inboundPath, String expected) throws Exception { - - String context = IdentityManagementEndpointConstants.UserInfoRecovery.RECOVERY_API_RELATIVE_PATH; - try (MockedStatic<IdentityTenantUtil> identityTenantUtil = mockStatic(IdentityTenantUtil.class); - MockedStatic<ServiceURLBuilder> serviceURLBuilder = mockStatic(ServiceURLBuilder.class)) { - prepareGetBasePathTest(contextUrl, context, identityTenantUtil, serviceURLBuilder, true, false); - lenient().when(serviceURL.getPath()).thenReturn(inboundPath); - assertEquals(IdentityManagementEndpointUtil.getBasePath(tenantDomain, context, isEndpointTenantAware), - expected); - } - } + `@Test`(dataProvider = "getBasePathConfiguredServerUrlData") + public void testGetBasePathConfiguredServerUrl(String contextUrl, String tenantDomain, + boolean isEndpointTenantAware, String expected) throws Exception { + + String context = IdentityManagementEndpointConstants.UserInfoRecovery.RECOVERY_API_RELATIVE_PATH; + try (MockedStatic<IdentityTenantUtil> identityTenantUtil = mockStatic(IdentityTenantUtil.class); + MockedStatic<ServiceURLBuilder> serviceURLBuilder = mockStatic(ServiceURLBuilder.class)) { + prepareGetBasePathTest(contextUrl, context, identityTenantUtil, serviceURLBuilder, true, false); + assertEquals(IdentityManagementEndpointUtil.getBasePath(tenantDomain, context, isEndpointTenantAware), + expected); + } + }🤖 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/identity-mgt/org.wso2.carbon.identity.mgt.endpoint.util/src/test/java/org/wso2/carbon/identity/mgt/endpoint/util/IdentityManagementEndpointUtilTest.java` around lines 376 - 430, The test data in IdentityManagementEndpointUtilTest.testGetBasePathConfiguredServerUrl is misleading because getBasePath now always takes the configured-serverUrl branch when contextUrl is set, so the inboundPath stub on serviceURL.getPath() is unused. Simplify the test by removing the inboundPath parameter and lenient serviceURL.getPath() stubbing from getBasePathConfiguredServerUrlData/testGetBasePathConfiguredServerUrl, or reword the comments to clearly state that only contextUrl/serverUrl and tenant awareness determine the result in this branch.
🤖 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/identity-mgt/org.wso2.carbon.identity.mgt.endpoint.util/src/main/java/org/wso2/carbon/identity/mgt/endpoint/util/IdentityManagementEndpointUtil.java`:
- Around line 901-909: Add a guarded DEBUG log at the tenant-qualification
branch in IdentityManagementEndpointUtil where basePath is computed, so this
decision point can be traced when tenant resolution goes wrong. Use the existing
tenant-aware URL logic around the serverUrl/context assignment to log the
computed basePath and whether the tenant prefix was appended, and keep it behind
isDebugEnabled() so there is no hot-path overhead.
In
`@components/identity-mgt/org.wso2.carbon.identity.mgt.endpoint.util/src/test/java/org/wso2/carbon/identity/mgt/endpoint/util/IdentityManagementEndpointUtilTest.java`:
- Around line 376-430: The test data in
IdentityManagementEndpointUtilTest.testGetBasePathConfiguredServerUrl is
misleading because getBasePath now always takes the configured-serverUrl branch
when contextUrl is set, so the inboundPath stub on serviceURL.getPath() is
unused. Simplify the test by removing the inboundPath parameter and lenient
serviceURL.getPath() stubbing from
getBasePathConfiguredServerUrlData/testGetBasePathConfiguredServerUrl, or reword
the comments to clearly state that only contextUrl/serverUrl and tenant
awareness determine the result in this branch.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yml
Review profile: CHILL
Plan: Pro
Run ID: 03ec5716-1853-41b2-8223-b06a331ec2b4
📒 Files selected for processing (2)
components/identity-mgt/org.wso2.carbon.identity.mgt.endpoint.util/src/main/java/org/wso2/carbon/identity/mgt/endpoint/util/IdentityManagementEndpointUtil.javacomponents/identity-mgt/org.wso2.carbon.identity.mgt.endpoint.util/src/test/java/org/wso2/carbon/identity/mgt/endpoint/util/IdentityManagementEndpointUtilTest.java
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #8196 +/- ##
=========================================
Coverage 52.98% 52.98%
+ Complexity 21087 21041 -46
=========================================
Files 2197 2197
Lines 129494 129494
Branches 19370 19370
=========================================
+ Hits 68612 68613 +1
Misses 52506 52506
+ Partials 8376 8375 -1
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 completed |
2 participants
Purpose
IdentityManagementEndpointUtil.getBasePath()drops the/t/<tenant>segment from the account recovery endpoint's internal REST calls when a server URL is configured (identity_server_service_url) and tenant-qualified URLs are enabled.The configured-server-URL branch reused
isServerURLAlreadyTenanted(), which inspects the inbound request path rather than the configured URL. For a non-super tenant the inbound path is already tenant-qualified, so the guard dropped the tenant prefix and the internal call resolved againstcarbon.super. This caused tenant self-registration confirmation (and related recovery flows) to fail withInvalid tenant 'carbon.super'.Approach
In the configured-server-URL branch, decide tenant qualification from the configured URL itself (
serverUrl.contains("/t/")) instead of the inbound request path, and append/t/<tenant>when it is not already present. The blank-server-URL branch is unchanged. Keying off the configured URL also avoids a double/t/<tenant>/t/<tenant>prefix when a tenant is baked into the configured URL.Added regression coverage in
IdentityManagementEndpointUtilTestfor the configured-server-URL branch with a tenant-qualified inbound path (previously uncovered), plus the already-tenanted-URL, super-tenant, and non-tenant-aware cases.Testing
IdentityManagementEndpointUtilTestpasses (JDK 21, module build), including the new cases.