Limit OAuth2 issuer resolution only for sub organizations#3145
Open
HasiniSama wants to merge 2 commits intowso2-extensions:masterfrom
Open
Limit OAuth2 issuer resolution only for sub organizations#3145HasiniSama wants to merge 2 commits intowso2-extensions:masterfrom
HasiniSama wants to merge 2 commits intowso2-extensions:masterfrom
Conversation
![wso2-engineering[bot]](https://avatars.githubusercontent.com/in/1856828?s=60&v=4){kind=small}
...arbon.identity.oauth/src/main/java/org/wso2/carbon/identity/oauth/OAuthAdminServiceImpl.java
Show resolved
Hide resolved
...arbon.identity.oauth/src/main/java/org/wso2/carbon/identity/oauth/OAuthAdminServiceImpl.java
Show resolved
Hide resolved
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.
| Comment | Accepted (Y/N) | Reason |
|---|---|---|
| #### Log Improvement Suggestion No: 1 | N | This is a redundant log that may cause noise. |
| #### Log Improvement Suggestion No: 2 | N | This is a redundant log that may cause noise. |
|
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:
📝 WalkthroughWalkthroughReplaced primary-organization checks with Changes
Sequence Diagram(s)sequenceDiagram
participant Client as Client
participant OAuthAdmin as OAuthAdminServiceImpl
participant OrgUtil as OrganizationManagementUtil
participant OrgMgr as OrganizationManager
participant IssuerSvc as OAuth2OIDCConfigOrgUsageScopeMgtService
Client->>OAuthAdmin: register/update OAuth app request
OAuthAdmin->>OrgUtil: isOrganization(tenantDomain)?
alt tenantDomain is an organization AND feature enabled AND not fragment app
OAuthAdmin->>OrgMgr: resolveOrganizationId(tenantDomain)
OrgMgr-->>OAuthAdmin: orgId
OAuthAdmin->>IssuerSvc: getAllowedIssuerDetails(orgId)
IssuerSvc-->>OAuthAdmin: issuer details
else
OrgUtil-->>OAuthAdmin: false / skip resolution
end
OAuthAdmin-->>Client: respond with created/updated app
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (1 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 |
9 tasks
|
Note Unit test generation is a public access feature. Expect some limitations and changes as we gather feedback and continue to improve it. Generating unit tests... This may take up to 20 minutes. |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #3145 +/- ##
============================================
- Coverage 59.39% 57.99% -1.40%
- Complexity 10298 10597 +299
============================================
Files 708 709 +1
Lines 58160 62104 +3944
Branches 14257 15362 +1105
============================================
+ Hits 34542 36020 +1478
- Misses 19026 21251 +2225
- Partials 4592 4833 +241
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:
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
components/org.wso2.carbon.identity.oauth/src/test/java/org/wso2/carbon/identity/oauth/OAuthAdminServiceImplTest.java (1)
319-324: Add a positive-path test for the new sub-org branch.This only stabilizes the non-organization path, so the new behavior is still unverified here. Please add a case with
OrganizationManagementUtil.isOrganization(...)returningtrueand the issuer-selection flag enabled, then verifyresolveOrganizationId(...)is invoked; the update path changed in the same way and deserves an equivalent assertion.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@components/org.wso2.carbon.identity.oauth/src/test/java/org/wso2/carbon/identity/oauth/OAuthAdminServiceImplTest.java` around lines 319 - 324, Add a positive-path unit test in OAuthAdminServiceImplTest that exercises the new sub-organization branch by mocking OrganizationManagementUtil.isOrganization(...) to return true and enabling the issuer-selection flag used by the code under test; in that test call the same method under test used by the existing case (the create/update flows exercised in the current test) and verify OrganizationManagementUtil.resolveOrganizationId(...) is invoked (use a MockedStatic verification) and that the update path assertion mirrors the create path assertion (i.e., also assert resolveOrganizationId(...) was called when performing the update flow with the flag enabled).
🤖 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/org.wso2.carbon.identity.oauth/src/test/java/org/wso2/carbon/identity/oauth/OAuthAdminServiceImplTest.java`:
- Around line 319-324: Add a positive-path unit test in
OAuthAdminServiceImplTest that exercises the new sub-organization branch by
mocking OrganizationManagementUtil.isOrganization(...) to return true and
enabling the issuer-selection flag used by the code under test; in that test
call the same method under test used by the existing case (the create/update
flows exercised in the current test) and verify
OrganizationManagementUtil.resolveOrganizationId(...) is invoked (use a
MockedStatic verification) and that the update path assertion mirrors the create
path assertion (i.e., also assert resolveOrganizationId(...) was called when
performing the update flow with the flag enabled).
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: a589f8e0-0238-464d-92d7-bb8c75300cfb
📒 Files selected for processing (2)
components/org.wso2.carbon.identity.oauth/src/main/java/org/wso2/carbon/identity/oauth/OAuthAdminServiceImpl.javacomponents/org.wso2.carbon.identity.oauth/src/test/java/org/wso2/carbon/identity/oauth/OAuthAdminServiceImplTest.java
|
❌ Failed to create PR with unit tests: AGENT_CHAT: Failed to open pull request |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
components/org.wso2.carbon.identity.oauth/src/test/java/org/wso2/carbon/identity/oauth/OAuthAdminServiceImplTest.java (2)
195-214: ResetOAuthAdminServiceImplstatic caches insetUp().These tests now mock
OAuthServerConfiguration, butOAuthAdminServiceImpl.allowedGrantsandallowedScopeValidatorssurvive across methods. If a previous test populated them, the new mocks are bypassed and the suite becomes order-dependent.Suggested fix
`@BeforeMethod` public void setUp() throws Exception { MockitoAnnotations.openMocks(this); + OAuthAdminServiceImpl.allowedGrants = null; + OAuthAdminServiceImpl.allowedScopeValidators = null; System.setProperty("carbon.home", System.getProperty("user.dir") + File.separator + "src" + File.separator + "test" + File.separator + "resources");🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@components/org.wso2.carbon.identity.oauth/src/test/java/org/wso2/carbon/identity/oauth/OAuthAdminServiceImplTest.java` around lines 195 - 214, The static caches OAuthAdminServiceImpl.allowedGrants and OAuthAdminServiceImpl.allowedScopeValidators must be reset in setUp() so prior test state doesn't leak; add code at the start of setUp() to clear or null out those static fields (use the existing public reset/clear API if present, otherwise use reflection to set the private static fields to null or empty collections) so the mocked OAuthServerConfiguration is honored for each test run.
1817-1974: Cover the feature-flag-disabled path explicitly.Every new issuer-resolution case forces
ISSUER_SELECTION_ENABLED_FOR_SUB_ORG_APPStotrue. A regression that ignores that flag would still pass this matrix. Add at least onefalsecase and assert neitherOrganizationManagementUtil.isOrganization(...)norresolveOrganizationId(...)is touched.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@components/org.wso2.carbon.identity.oauth/src/test/java/org/wso2/carbon/identity/oauth/OAuthAdminServiceImplTest.java` around lines 1817 - 1974, The tests always enable the feature flag ISSUER_SELECTION_ENABLED_FOR_SUB_ORG_APPS, so add/cover a disabled-path case and assert no org-resolution calls occur: update the issuerConfigResolutionData or add an extra test invocation so one scenario sets IdentityUtil.getProperty(ISSUER_SELECTION_ENABLED_FOR_SUB_ORG_APPS) to "false" (via the identityUtil mock used in testRegisterOAuthAppIssuerConfigResolution and testUpdateConsumerAppIssuerConfigResolution) and then verify that OrganizationManagementUtil.isOrganization(...) is never invoked and mockOrganizationManager.resolveOrganizationId(...) is never invoked; reference the existing test methods (testRegisterOAuthAppIssuerConfigResolution, testUpdateConsumerAppIssuerConfigResolution) and the symbols IdentityUtil.getProperty, OrganizationManagementUtil.isOrganization, and resolveOrganizationId to locate where to add the flag-disabled branch and assertions.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@components/org.wso2.carbon.identity.oauth/src/main/java/org/wso2/carbon/identity/oauth/OAuthAdminServiceImpl.java`:
- Around line 605-615: The call to
OrganizationManagementUtil.isOrganization(tenantDomain) is executed
unconditionally and can throw before issuer-selection is checked; move that org
lookup and any resolveOrganizationId(tenantDomain) calls inside the feature-flag
+ fragment-app guard so it only runs when issuer selection is enabled and the
app is a sub-organization fragment check passes. Update
OAuthAdminServiceImpl.registerAndRetrieveOAuthApplicationData() and
updateConsumerApplication() (and the similar block around lines ~1130-1140) to
defer calling OrganizationManagementUtil.isOrganization(tenantDomain) and
OAuth2ServiceComponentHolder.getInstance().getOrganizationManager().resolveOrganizationId(tenantDomain)
until after the
Boolean.parseBoolean(IdentityUtil.getProperty(ISSUER_SELECTION_ENABLED_FOR_SUB_ORG_APPS))
and isSubOrg && !application.getIsFragmentApp() checks succeed.
---
Nitpick comments:
In
`@components/org.wso2.carbon.identity.oauth/src/test/java/org/wso2/carbon/identity/oauth/OAuthAdminServiceImplTest.java`:
- Around line 195-214: The static caches OAuthAdminServiceImpl.allowedGrants and
OAuthAdminServiceImpl.allowedScopeValidators must be reset in setUp() so prior
test state doesn't leak; add code at the start of setUp() to clear or null out
those static fields (use the existing public reset/clear API if present,
otherwise use reflection to set the private static fields to null or empty
collections) so the mocked OAuthServerConfiguration is honored for each test
run.
- Around line 1817-1974: The tests always enable the feature flag
ISSUER_SELECTION_ENABLED_FOR_SUB_ORG_APPS, so add/cover a disabled-path case and
assert no org-resolution calls occur: update the issuerConfigResolutionData or
add an extra test invocation so one scenario sets
IdentityUtil.getProperty(ISSUER_SELECTION_ENABLED_FOR_SUB_ORG_APPS) to "false"
(via the identityUtil mock used in testRegisterOAuthAppIssuerConfigResolution
and testUpdateConsumerAppIssuerConfigResolution) and then verify that
OrganizationManagementUtil.isOrganization(...) is never invoked and
mockOrganizationManager.resolveOrganizationId(...) is never invoked; reference
the existing test methods (testRegisterOAuthAppIssuerConfigResolution,
testUpdateConsumerAppIssuerConfigResolution) and the symbols
IdentityUtil.getProperty, OrganizationManagementUtil.isOrganization, and
resolveOrganizationId to locate where to add the flag-disabled branch and
assertions.
🪄 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: df8d1806-c3f2-47c1-ad82-694166e33c2a
📒 Files selected for processing (2)
components/org.wso2.carbon.identity.oauth/src/main/java/org/wso2/carbon/identity/oauth/OAuthAdminServiceImpl.javacomponents/org.wso2.carbon.identity.oauth/src/test/java/org/wso2/carbon/identity/oauth/OAuthAdminServiceImplTest.java
...arbon.identity.oauth/src/main/java/org/wso2/carbon/identity/oauth/OAuthAdminServiceImpl.java
Show resolved
Hide resolved
HasiniSama
force-pushed
the
issuer-resolution-master
branch
from
d47dd14 to
337cbbc
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
components/org.wso2.carbon.identity.oauth/src/test/java/org/wso2/carbon/identity/oauth/OAuthAdminServiceImplTest.java (3)
1976-1991: Rename helper method for Java naming consistency.
getoAuthConsumerAppDTO(...)is harder to read; prefergetOAuthConsumerAppDTO(...).Suggested rename
- private static OAuthConsumerAppDTO getoAuthConsumerAppDTO(String userName, String consumerKey, + private static OAuthConsumerAppDTO getOAuthConsumerAppDTO(String userName, String consumerKey, String consumerSecret, String oauthVersion) { @@ - OAuthConsumerAppDTO oAuthConsumerAppDTO = getoAuthConsumerAppDTO(userName, consumerKey, consumerSecret, oauthVersion); + OAuthConsumerAppDTO oAuthConsumerAppDTO = getOAuthConsumerAppDTO(userName, consumerKey, consumerSecret, oauthVersion);Also applies to: 343-344, 1872-1874, 1944-1946
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@components/org.wso2.carbon.identity.oauth/src/test/java/org/wso2/carbon/identity/oauth/OAuthAdminServiceImplTest.java` around lines 1976 - 1991, Rename the helper method getoAuthConsumerAppDTO to getOAuthConsumerAppDTO for Java naming consistency: change the method declaration (private static OAuthConsumerAppDTO getoAuthConsumerAppDTO(...)) to getOAuthConsumerAppDTO and update every call site that invokes getoAuthConsumerAppDTO(...) (including the other occurrences noted) to the new name; keep the method signature and behavior unchanged so tests compile and run.
198-198: Close theopenMockssession after each test.At Line 198,
MockitoAnnotations.openMocks(this)should be closed in@AfterMethodto avoid mock-session/resource leakage between tests.Suggested fix
@@ public class OAuthAdminServiceImplTest { + private AutoCloseable closeable; @@ `@BeforeMethod` public void setUp() throws Exception { - MockitoAnnotations.openMocks(this); + closeable = MockitoAnnotations.openMocks(this); @@ - public void tearDown() { + public void tearDown() throws Exception { identityTenantUtil.close(); loggerUtils.close(); + if (closeable != null) { + closeable.close(); + } }Also applies to: 217-221
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@components/org.wso2.carbon.identity.oauth/src/test/java/org/wso2/carbon/identity/oauth/OAuthAdminServiceImplTest.java` at line 198, The Mockito mock session opened with MockitoAnnotations.openMocks(this) must be closed after each test to prevent resource leakage: store the AutoCloseable returned by MockitoAnnotations.openMocks(this) in a field (e.g., AutoCloseable mocks) during test initialization (setUp) and implement an `@AfterMethod` tearDown that calls mocks.close(); update any other test setup blocks that call MockitoAnnotations.openMocks(this) (the occurrences around the other setup) to follow the same pattern so every opened mock session is closed in its corresponding `@AfterMethod`.
1817-1825: Add feature-flag-disabled cases to issuer-resolution data provider.Current data only tests with
ISSUER_SELECTION_ENABLED_FOR_SUB_ORG_APPS=true. Add explicitfalsecases so regressions in flag gating are caught.Suggested test expansion
@@ - // isSubOrg, isFragmentApp, expectIssuerResolutionCalled. + // isSubOrg, isFragmentApp, issuerSelectionEnabled, expectIssuerResolutionCalled. return new Object[][] { - {true, false, true}, - {false, false, false}, - {true, true, false}, + {true, false, true, true}, + {true, false, false, false}, + {false, false, true, false}, + {true, true, true, false}, }; @@ -public void testRegisterOAuthAppIssuerConfigResolution(boolean isSubOrg, boolean isFragmentApp, - boolean expectIssuerResolutionCalled) throws Exception { +public void testRegisterOAuthAppIssuerConfigResolution(boolean isSubOrg, boolean isFragmentApp, + boolean issuerSelectionEnabled, + boolean expectIssuerResolutionCalled) throws Exception { @@ - .thenReturn(String.valueOf(true)); + .thenReturn(String.valueOf(issuerSelectionEnabled)); @@ -public void testUpdateConsumerAppIssuerConfigResolution(boolean isSubOrg, boolean isFragmentApp, - boolean expectIssuerResolutionCalled) throws Exception { +public void testUpdateConsumerAppIssuerConfigResolution(boolean isSubOrg, boolean isFragmentApp, + boolean issuerSelectionEnabled, + boolean expectIssuerResolutionCalled) throws Exception { @@ - .thenReturn(String.valueOf(true)); + .thenReturn(String.valueOf(issuerSelectionEnabled));Also applies to: 1828-1831, 1881-1882, 1893-1896, 1962-1963
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@components/org.wso2.carbon.identity.oauth/src/test/java/org/wso2/carbon/identity/oauth/OAuthAdminServiceImplTest.java` around lines 1817 - 1825, Add test cases to the issuerConfigResolutionData data provider to cover ISSUER_SELECTION_ENABLED_FOR_SUB_ORG_APPS=false (i.e., duplicate the existing rows but with the feature flag off) so the suite asserts issuer resolution is not called when the flag is disabled; update the expectations accordingly in the test that consumes issuerConfigResolutionData (method OAuthAdminServiceImplTest) to assert no issuer-resolution invocation for the false-flag rows. Repeat the same expansion for the other related data providers and test inputs noted in the comment ranges (the other provider blocks near lines 1828-1831, 1881-1882, 1893-1896, 1962-1963) so each scenario is exercised with both true and false values of ISSUER_SELECTION_ENABLED_FOR_SUB_ORG_APPS and expectations adjusted to match the disabled behavior.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@components/org.wso2.carbon.identity.oauth/src/main/java/org/wso2/carbon/identity/oauth/OAuthAdminServiceImpl.java`:
- Around line 608-614: The current guard around
resolveApplicationLevelTokenIssuerConfig uses
OrganizationManagementUtil.isOrganization(tenantDomain) allowing primary orgs
through; update the condition to also exclude the primary organization by adding
!OrganizationManagementUtil.isPrimaryOrganization(tenantDomain) so the block
only runs for sub-orgs when
IdentityUtil.getProperty(ISSUER_SELECTION_ENABLED_FOR_SUB_ORG_APPS) is true and
application.getIsFragmentApp() is false; apply the same additional primary-org
exclusion to the other occurrence in OAuthAdminServiceImpl where
resolveApplicationLevelTokenIssuerConfig is invoked (the block that calls
OAuth2ServiceComponentHolder.getInstance().getOrganizationManager().resolveOrganizationId
and resolveApplicationLevelTokenIssuerConfig).
---
Nitpick comments:
In
`@components/org.wso2.carbon.identity.oauth/src/test/java/org/wso2/carbon/identity/oauth/OAuthAdminServiceImplTest.java`:
- Around line 1976-1991: Rename the helper method getoAuthConsumerAppDTO to
getOAuthConsumerAppDTO for Java naming consistency: change the method
declaration (private static OAuthConsumerAppDTO getoAuthConsumerAppDTO(...)) to
getOAuthConsumerAppDTO and update every call site that invokes
getoAuthConsumerAppDTO(...) (including the other occurrences noted) to the new
name; keep the method signature and behavior unchanged so tests compile and run.
- Line 198: The Mockito mock session opened with
MockitoAnnotations.openMocks(this) must be closed after each test to prevent
resource leakage: store the AutoCloseable returned by
MockitoAnnotations.openMocks(this) in a field (e.g., AutoCloseable mocks) during
test initialization (setUp) and implement an `@AfterMethod` tearDown that calls
mocks.close(); update any other test setup blocks that call
MockitoAnnotations.openMocks(this) (the occurrences around the other setup) to
follow the same pattern so every opened mock session is closed in its
corresponding `@AfterMethod`.
- Around line 1817-1825: Add test cases to the issuerConfigResolutionData data
provider to cover ISSUER_SELECTION_ENABLED_FOR_SUB_ORG_APPS=false (i.e.,
duplicate the existing rows but with the feature flag off) so the suite asserts
issuer resolution is not called when the flag is disabled; update the
expectations accordingly in the test that consumes issuerConfigResolutionData
(method OAuthAdminServiceImplTest) to assert no issuer-resolution invocation for
the false-flag rows. Repeat the same expansion for the other related data
providers and test inputs noted in the comment ranges (the other provider blocks
near lines 1828-1831, 1881-1882, 1893-1896, 1962-1963) so each scenario is
exercised with both true and false values of
ISSUER_SELECTION_ENABLED_FOR_SUB_ORG_APPS and expectations adjusted to match the
disabled behavior.
🪄 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: d8f75e1d-57da-4953-8bb6-29778b9ae665
📒 Files selected for processing (2)
components/org.wso2.carbon.identity.oauth/src/main/java/org/wso2/carbon/identity/oauth/OAuthAdminServiceImpl.javacomponents/org.wso2.carbon.identity.oauth/src/test/java/org/wso2/carbon/identity/oauth/OAuthAdminServiceImplTest.java
...arbon.identity.oauth/src/main/java/org/wso2/carbon/identity/oauth/OAuthAdminServiceImpl.java
Show resolved
Hide resolved
HasiniSama
force-pushed
the
issuer-resolution-master
branch
from
337cbbc to
1585133
Compare
There was a problem hiding this comment.
♻️ Duplicate comments (1)
components/org.wso2.carbon.identity.oauth/src/main/java/org/wso2/carbon/identity/oauth/OAuthAdminServiceImpl.java (1)
605-615:⚠️ Potential issue | 🟠 MajorDefer the
isOrganization(...)call as well.The sub-org predicate is fine, but
OrganizationManagementUtil.isOrganization(tenantDomain)still runs before the feature-flag / fragment-app guard, so create/update can fail on org-management errors even when issuer selection should be skipped.♻️ Suggested fix
- boolean isSubOrg = OrganizationManagementUtil.isOrganization(tenantDomain); - /* - If the app is not registering under a primary organization, and it is not a fragment app, - validate the issuer organization and set the issuer org of the app. - */ - if (Boolean.parseBoolean( - IdentityUtil.getProperty(ISSUER_SELECTION_ENABLED_FOR_SUB_ORG_APPS))) { - if (isSubOrg && !application.getIsFragmentApp()) { + /* + If the app is registering under a sub-organization, and it is not a fragment app, + validate the issuer organization and set the issuer org of the app. + */ + if (Boolean.parseBoolean( + IdentityUtil.getProperty(ISSUER_SELECTION_ENABLED_FOR_SUB_ORG_APPS)) && + !application.getIsFragmentApp()) { + if (OrganizationManagementUtil.isOrganization(tenantDomain)) { String orgId = OAuth2ServiceComponentHolder.getInstance().getOrganizationManager(). resolveOrganizationId(tenantDomain); resolveApplicationLevelTokenIssuerConfig(application, app, tenantDomain, orgId); } }- boolean isSubOrg = OrganizationManagementUtil.isOrganization(tenantDomain); - /* - If the app is not updating under a primary organization, and it is not a fragment app, - validate the issuer organization and set the issuer org of the app. - */ - if (Boolean.parseBoolean( - IdentityUtil.getProperty(ISSUER_SELECTION_ENABLED_FOR_SUB_ORG_APPS))) { - if (isSubOrg && !consumerAppDTO.getIsFragmentApp()) { + /* + If the app is updating under a sub-organization, and it is not a fragment app, + validate the issuer organization and set the issuer org of the app. + */ + if (Boolean.parseBoolean( + IdentityUtil.getProperty(ISSUER_SELECTION_ENABLED_FOR_SUB_ORG_APPS)) && + !consumerAppDTO.getIsFragmentApp()) { + if (OrganizationManagementUtil.isOrganization(tenantDomain)) { String orgId = OAuth2ServiceComponentHolder.getInstance().getOrganizationManager(). resolveOrganizationId(tenantDomain); resolveApplicationLevelTokenIssuerConfig(consumerAppDTO, oAuthAppDO, tenantDomain, orgId); } }Based on learnings,
OrganizationManagementUtil.isOrganization(tenantDomain)is already the correct sub-org-only guard here; only the unconditional call placement needs fixing.Also applies to: 1130-1140
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@components/org.wso2.carbon.identity.oauth/src/main/java/org/wso2/carbon/identity/oauth/OAuthAdminServiceImpl.java` around lines 605 - 615, The call to OrganizationManagementUtil.isOrganization(tenantDomain) runs unconditionally and can throw org-management errors even when issuer-selection is disabled; move/defer that call inside the feature-flag and fragment-app guard so it only executes when Boolean.parseBoolean(ISSUER_SELECTION_ENABLED_FOR_SUB_ORG_APPS) is true and application.getIsFragmentApp() is false, then call resolveOrganizationId(...) and resolveApplicationLevelTokenIssuerConfig(...) as before; apply the same change to the duplicate block around resolveApplicationLevelTokenIssuerConfig (the later occurrence near 1130-1140).
🧹 Nitpick comments (3)
components/org.wso2.carbon.identity.oauth/src/main/java/org/wso2/carbon/identity/oauth/OAuthAdminServiceImpl.java (1)
161-162: ReduceISSUER_SELECTION_ENABLED_FOR_SUB_ORG_APPSvisibility to package-private.The constant is only used within the
org.wso2.carbon.identity.oauthpackage (internally inOAuthAdminServiceImpland byOAuthAdminServiceImplTest).publicis unnecessarily broad; package-private visibility is sufficient.Proposed change
- public static final String ISSUER_SELECTION_ENABLED_FOR_SUB_ORG_APPS = + static final String ISSUER_SELECTION_ENABLED_FOR_SUB_ORG_APPS = "OAuth.AllowIssuerSelectionForSubOrgApplications";🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@components/org.wso2.carbon.identity.oauth/src/main/java/org/wso2/carbon/identity/oauth/OAuthAdminServiceImpl.java` around lines 161 - 162, Change the visibility of the constant ISSUER_SELECTION_ENABLED_FOR_SUB_ORG_APPS in class OAuthAdminServiceImpl from public static final to package-private (remove the public modifier) since it is only used within the org.wso2.carbon.identity.oauth package and by tests in the same package; update any imports/usages if they referenced the constant from outside the package (there should be none) and run/adjust OAuthAdminServiceImplTest to ensure it still accesses the constant from the package scope.components/org.wso2.carbon.identity.oauth/src/test/java/org/wso2/carbon/identity/oauth/OAuthAdminServiceImplTest.java (2)
196-214: HandleAutoCloseablereturned byopenMocks()to prevent resource leaks.
MockitoAnnotations.openMocks(this)returns anAutoCloseablethat should be closed after tests to release mock resources. Consider storing it and closing intearDown().♻️ Suggested fix
+ private AutoCloseable mockitoCloseable; + `@BeforeMethod` public void setUp() throws Exception { - MockitoAnnotations.openMocks(this); + mockitoCloseable = MockitoAnnotations.openMocks(this); System.setProperty("carbon.home",Then in
tearDown():`@AfterMethod` public void tearDown() { identityTenantUtil.close(); loggerUtils.close(); + if (mockitoCloseable != null) { + try { + mockitoCloseable.close(); + } catch (Exception ignored) { + } + } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@components/org.wso2.carbon.identity.oauth/src/test/java/org/wso2/carbon/identity/oauth/OAuthAdminServiceImplTest.java` around lines 196 - 214, The call to MockitoAnnotations.openMocks(this) in setUp() returns an AutoCloseable that must be closed to avoid mock resource leaks; modify setUp() to capture that return value into a private AutoCloseable field (e.g., mocksCloseable) and implement/extend the existing tearDown() to call mocksCloseable.close() (handling or rethrowing InterruptedException/Exception as appropriate) and nulling the field; update references to MockitoAnnotations.openMocks(...) and ensure tearDown() always closes the AutoCloseable even if other cleanup fails.
1976-1991: Good helper method to reduce duplication.This helper effectively centralizes the
OAuthConsumerAppDTOcreation logic, improving maintainability.Optional nitpick: Java naming convention typically capitalizes the first letter after
get(e.g.,getOAuthConsumerAppDTOinstead ofgetoAuthConsumerAppDTO).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@components/org.wso2.carbon.identity.oauth/src/test/java/org/wso2/carbon/identity/oauth/OAuthAdminServiceImplTest.java` around lines 1976 - 1991, Rename the helper method getoAuthConsumerAppDTO to follow Java naming conventions (e.g., getOAuthConsumerAppDTO) to capitalize the 'O' after 'get'; update all references to this method (calls in tests) to the new name and ensure the method signature in OAuthAdminServiceImplTest (getoAuthConsumerAppDTO) is renamed consistently to avoid compile errors.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In
`@components/org.wso2.carbon.identity.oauth/src/main/java/org/wso2/carbon/identity/oauth/OAuthAdminServiceImpl.java`:
- Around line 605-615: The call to
OrganizationManagementUtil.isOrganization(tenantDomain) runs unconditionally and
can throw org-management errors even when issuer-selection is disabled;
move/defer that call inside the feature-flag and fragment-app guard so it only
executes when Boolean.parseBoolean(ISSUER_SELECTION_ENABLED_FOR_SUB_ORG_APPS) is
true and application.getIsFragmentApp() is false, then call
resolveOrganizationId(...) and resolveApplicationLevelTokenIssuerConfig(...) as
before; apply the same change to the duplicate block around
resolveApplicationLevelTokenIssuerConfig (the later occurrence near 1130-1140).
---
Nitpick comments:
In
`@components/org.wso2.carbon.identity.oauth/src/main/java/org/wso2/carbon/identity/oauth/OAuthAdminServiceImpl.java`:
- Around line 161-162: Change the visibility of the constant
ISSUER_SELECTION_ENABLED_FOR_SUB_ORG_APPS in class OAuthAdminServiceImpl from
public static final to package-private (remove the public modifier) since it is
only used within the org.wso2.carbon.identity.oauth package and by tests in the
same package; update any imports/usages if they referenced the constant from
outside the package (there should be none) and run/adjust
OAuthAdminServiceImplTest to ensure it still accesses the constant from the
package scope.
In
`@components/org.wso2.carbon.identity.oauth/src/test/java/org/wso2/carbon/identity/oauth/OAuthAdminServiceImplTest.java`:
- Around line 196-214: The call to MockitoAnnotations.openMocks(this) in setUp()
returns an AutoCloseable that must be closed to avoid mock resource leaks;
modify setUp() to capture that return value into a private AutoCloseable field
(e.g., mocksCloseable) and implement/extend the existing tearDown() to call
mocksCloseable.close() (handling or rethrowing InterruptedException/Exception as
appropriate) and nulling the field; update references to
MockitoAnnotations.openMocks(...) and ensure tearDown() always closes the
AutoCloseable even if other cleanup fails.
- Around line 1976-1991: Rename the helper method getoAuthConsumerAppDTO to
follow Java naming conventions (e.g., getOAuthConsumerAppDTO) to capitalize the
'O' after 'get'; update all references to this method (calls in tests) to the
new name and ensure the method signature in OAuthAdminServiceImplTest
(getoAuthConsumerAppDTO) is renamed consistently to avoid compile errors.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: a8d5dedf-518f-48ec-a563-c1fee16ac3f4
📒 Files selected for processing (2)
components/org.wso2.carbon.identity.oauth/src/main/java/org/wso2/carbon/identity/oauth/OAuthAdminServiceImpl.javacomponents/org.wso2.carbon.identity.oauth/src/test/java/org/wso2/carbon/identity/oauth/OAuthAdminServiceImplTest.java
| private static final int MAX_RETRY_ATTEMPTS = 3; | ||
| private static final String BASE_URL_PLACEHOLDER = "<PROTOCOL>://<HOSTNAME>:<PORT>"; | ||
| private static final String ISSUER_SELECTION_ENABLED_FOR_SUB_ORG_APPS = | ||
| public static final String ISSUER_SELECTION_ENABLED_FOR_SUB_ORG_APPS = |
Contributor
There was a problem hiding this comment.
The only usage of converting this to a public variable is for the tests right? Since we are not using this in actual flows, let's make this private and introduce a variable in the tests.
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
2 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.
Proposed changes in this pull request
This fix ensure that issuer config resolution is only applied for sub-organization apps and since primary org Id check will be misleading in scenarios where sub org start level is different.
Summary by CodeRabbit
Bug Fixes
Tests
Refactor