Fix: Enable concurrent operation of Resident and FS Key Managers for APIM Try Out compatibility #865
Conversation
VimukthiRajapaksha
added
Type/Bug
Product/Accelerator-4.0.0
WalkthroughThis change refactors key manager configuration handling by introducing KeyManagerConfiguration as an explicit parameter to multiple utility methods. Four existing public methods in IdentityServerUtils are updated to accept configuration context, two new public helper methods are added for endpoint and authentication header derivation, and callers in FSKeyManagerImpl are updated accordingly. A new constant OAUTH2 is added to FSKeyManagerConstants. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 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 |
![wso2-engineering[bot]](https://avatars.githubusercontent.com/in/1856828?s=60&v=4){kind=small}
...nager/src/main/java/org/wso2/financial/services/accelerator/keymanager/FSKeyManagerImpl.java
Show resolved
Hide resolved
.../main/java/org/wso2/financial/services/accelerator/keymanager/utils/IdentityServerUtils.java
Show resolved
Hide resolved
| CloseableHttpResponse response = HTTPClientUtils.getHttpsClient().execute(httpGet); | ||
| if (response.getStatusLine().getStatusCode() != 200) { | ||
| throw new FinancialServicesException("Error while getting app id from client id"); |
There was a problem hiding this comment.
Log Improvement Suggestion No: 4
| CloseableHttpResponse response = HTTPClientUtils.getHttpsClient().execute(httpGet); | |
| if (response.getStatusLine().getStatusCode() != 200) { | |
| throw new FinancialServicesException("Error while getting app id from client id"); | |
| CloseableHttpResponse response = HTTPClientUtils.getHttpsClient().execute(httpGet); | |
| if (response.getStatusLine().getStatusCode() != 200) { | |
| log.error("Failed to fetch application ID for clientId: {}. Status: {}", clientId, response.getStatusLine().getStatusCode()); | |
| throw new FinancialServicesException("Error while getting app id from client id"); |
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: 3 | ||
| #### Log Improvement Suggestion No: 4 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
financial-services-accelerator/components/org.wso2.financial.services.accelerator.keymanager/src/main/java/org/wso2/financial/services/accelerator/keymanager/utils/IdentityServerUtils.java (1)
121-157: Fix misleading error messages.The error messages on lines 152 and 155 incorrectly state "Error while getting sp application from client id" when the method is actually updating the SP application.
Apply this diff to fix the error messages:
CloseableHttpResponse response = HTTPClientUtils.getHttpsClient().execute(httpPatch); if (response.getStatusLine().getStatusCode() != 200) { - throw new FinancialServicesException("Error while getting sp application from client id"); + throw new FinancialServicesException("Error while updating sp application"); } } catch (IOException | URISyntaxException e) { - throw new FinancialServicesException("Error while getting sp application from client id", e); + throw new FinancialServicesException("Error while updating sp application", e); }
🧹 Nitpick comments (1)
financial-services-accelerator/components/org.wso2.financial.services.accelerator.keymanager/src/main/java/org/wso2/financial/services/accelerator/keymanager/utils/IdentityServerUtils.java (1)
358-371: Clarify the base URL resolution logic.The
getKeyManagerBaseUrlmethod splits the authorize endpoint on/oauth2to extract the base URL. While this works when the authorize endpoint contains/oauth2, the fallback togetIdentitySeverUrl()is appropriate when it doesn't. However, the logic could be more explicit about this assumption.Consider adding a comment to clarify the logic:
private static String getKeyManagerBaseUrl(final KeyManagerConfiguration keyManagerConfiguration) { final String keyManagerAuthEndpoint = (String) keyManagerConfiguration.getParameter(APIConstants.KeyManager.AUTHORIZE_ENDPOINT); if (StringUtils.isNotEmpty(keyManagerAuthEndpoint)) { + // Extract base URL by splitting on /oauth2 path segment (e.g., https://host:port/oauth2/authorize -> https://host:port) return keyManagerAuthEndpoint.split(FSKeyManagerConstants.OAUTH2)[0]; } + // Fallback to Identity Server URL from APIM configuration return getIdentitySeverUrl(); }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
financial-services-accelerator/components/org.wso2.financial.services.accelerator.keymanager/src/main/java/org/wso2/financial/services/accelerator/keymanager/FSKeyManagerImpl.java(5 hunks)financial-services-accelerator/components/org.wso2.financial.services.accelerator.keymanager/src/main/java/org/wso2/financial/services/accelerator/keymanager/utils/FSKeyManagerConstants.java(1 hunks)financial-services-accelerator/components/org.wso2.financial.services.accelerator.keymanager/src/main/java/org/wso2/financial/services/accelerator/keymanager/utils/IdentityServerUtils.java(8 hunks)financial-services-accelerator/components/org.wso2.financial.services.accelerator.keymanager/src/test/java/org/wso2/financial/services/accelerator/keymanager/FSKeyManagerImplTest.java(2 hunks)financial-services-accelerator/components/org.wso2.financial.services.accelerator.keymanager/src/test/java/org/wso2/financial/services/accelerator/keymanager/IdentityServerUtilsTest.java(7 hunks)
🧰 Additional context used
🧬 Code graph analysis (4)
financial-services-accelerator/components/org.wso2.financial.services.accelerator.keymanager/src/main/java/org/wso2/financial/services/accelerator/keymanager/FSKeyManagerImpl.java (1)
financial-services-accelerator/components/org.wso2.financial.services.accelerator.keymanager/src/main/java/org/wso2/financial/services/accelerator/keymanager/utils/IdentityServerUtils.java (1)
IdentityServerUtils(50-373)
financial-services-accelerator/components/org.wso2.financial.services.accelerator.keymanager/src/test/java/org/wso2/financial/services/accelerator/keymanager/FSKeyManagerImplTest.java (1)
financial-services-accelerator/components/org.wso2.financial.services.accelerator.keymanager/src/main/java/org/wso2/financial/services/accelerator/keymanager/utils/IdentityServerUtils.java (1)
IdentityServerUtils(50-373)
financial-services-accelerator/components/org.wso2.financial.services.accelerator.keymanager/src/main/java/org/wso2/financial/services/accelerator/keymanager/utils/IdentityServerUtils.java (2)
financial-services-accelerator/components/org.wso2.financial.services.accelerator.keymanager/src/main/java/org/wso2/financial/services/accelerator/keymanager/utils/FSKeyManagerConstants.java (1)
FSKeyManagerConstants(24-56)financial-services-accelerator/components/org.wso2.financial.services.accelerator.common/src/main/java/org/wso2/financial/services/accelerator/common/util/FinancialServicesUtils.java (1)
FinancialServicesUtils(51-345)
financial-services-accelerator/components/org.wso2.financial.services.accelerator.keymanager/src/test/java/org/wso2/financial/services/accelerator/keymanager/IdentityServerUtilsTest.java (1)
financial-services-accelerator/components/org.wso2.financial.services.accelerator.keymanager/src/main/java/org/wso2/financial/services/accelerator/keymanager/utils/IdentityServerUtils.java (1)
IdentityServerUtils(50-373)
🪛 Gitleaks (8.29.1)
financial-services-accelerator/components/org.wso2.financial.services.accelerator.keymanager/src/test/java/org/wso2/financial/services/accelerator/keymanager/IdentityServerUtilsTest.java
[high] 124-124: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
[high] 151-151: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
[high] 177-177: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
[high] 203-203: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
🔇 Additional comments (23)
financial-services-accelerator/components/org.wso2.financial.services.accelerator.keymanager/src/main/java/org/wso2/financial/services/accelerator/keymanager/utils/FSKeyManagerConstants.java (1)
31-31: LGTM!The new
OAUTH2constant is correctly defined and aligns with the endpoint pattern used inIdentityServerUtils.getKeyManagerBaseUrlfor splitting the authorize endpoint.financial-services-accelerator/components/org.wso2.financial.services.accelerator.keymanager/src/test/java/org/wso2/financial/services/accelerator/keymanager/FSKeyManagerImplTest.java (2)
103-103: LGTM!The mock setup correctly reflects the updated
IdentityServerUtils.getSPApplicationFromClientIdsignature that now acceptsKeyManagerConfigurationas the first parameter.
116-116: LGTM!The mock setup correctly reflects the updated
IdentityServerUtils.getSPApplicationFromClientIdsignature for the exception scenario.financial-services-accelerator/components/org.wso2.financial.services.accelerator.keymanager/src/main/java/org/wso2/financial/services/accelerator/keymanager/FSKeyManagerImpl.java (6)
68-69: LGTM!The call correctly passes
super.getKeyManagerConfiguration()to provide key manager-specific configuration context toIdentityServerUtils.getSPApplicationFromClientId.
215-216: LGTM!The call correctly passes
super.getKeyManagerConfiguration()to enable configuration-driven endpoint resolution in the create application flow.
248-249: LGTM!The call correctly passes
super.getKeyManagerConfiguration()to enable configuration-driven endpoint resolution in the update application flow.
268-269: LGTM!The call correctly passes
super.getKeyManagerConfiguration()to enable configuration-driven endpoint resolution in the retrieve application flow.
300-301: LGTM!The call correctly passes
super.getKeyManagerConfiguration()to enable configuration-driven DCR application updates.
309-310: LGTM!The call correctly passes
super.getKeyManagerConfiguration()to enable configuration-driven SP application updates.financial-services-accelerator/components/org.wso2.financial.services.accelerator.keymanager/src/test/java/org/wso2/financial/services/accelerator/keymanager/IdentityServerUtilsTest.java (7)
34-35: LGTM!The new imports for
KeyManagerConfigurationandAPIConstantssupport the refactored test setup.
88-93: LGTM!The
KeyManagerConfigurationinstance is properly initialized with the required parameters (authorize endpoint, username, password) for testing the updated IdentityServerUtils methods.
123-124: LGTM!The test correctly passes
keyManagerConfigurationtogetAppIdFromClientId.Note: The static analysis warning about a potential API key on line 124 is a false positive—
"rgyZpk8uMCBXqolzjWQyLnmPVd0a"is mock test data.
150-151: LGTM!The test correctly passes
keyManagerConfigurationtogetSPApplicationFromClientId.Note: The static analysis warning about a potential API key on line 151 is a false positive—this is mock test data.
177-177: LGTM!The test correctly passes
keyManagerConfigurationtoupdateSPApplication.Note: The static analysis warning about a potential API key on line 177 is a false positive—this is mock test data.
203-203: LGTM!The test correctly passes
keyManagerConfigurationtoupdateDCRApplication.Note: The static analysis warning about a potential API key on line 203 is a false positive—this is mock test data.
129-154: Re-enable or remove the disabledtestGetSPApplicationFromClientIdtest.The test method at lines 119-143 has its
@Testannotation commented out and was introduced in a disabled state without documented justification. Either re-enable the test by uncommenting the annotation, or remove the entire method if it is no longer needed.financial-services-accelerator/components/org.wso2.financial.services.accelerator.keymanager/src/main/java/org/wso2/financial/services/accelerator/keymanager/utils/IdentityServerUtils.java (7)
23-23: LGTM!The
StringUtilsimport supports the null/empty check for the authorize endpoint ingetKeyManagerBaseUrl.
32-33: LGTM!The new imports for
KeyManagerConfigurationandAPIConstantssupport the refactored method signatures and configuration access.
52-87: LGTM!The refactored
getAppIdFromClientIdmethod correctly acceptsKeyManagerConfigurationand uses it to resolve the endpoint and credentials dynamically.
89-119: LGTM!The refactored
getSPApplicationFromClientIdmethod correctly acceptsKeyManagerConfigurationand delegates to the updatedgetAppIdFromClientIdmethod.
177-210: LGTM!The refactored
updateDCRApplicationmethod correctly acceptsKeyManagerConfigurationand usesgetKeyManagerBaseUrlto construct the DCR endpoint URL.
335-343: LGTM!The new
getKeyManagerApplicationMgtEndpointhelper centralizes the Application Management endpoint construction, improving maintainability.
345-356: LGTM!The new
getBasicAuthHeaderFromKeyManagerConfighelper centralizes credential extraction and Basic Auth header generation fromKeyManagerConfiguration.
3 participants
Purpose
This PR enables both Resident Key Manager and Financial Services Key Manager to be active simultaneously, resolving the limitation where users had to choose between OB compliance and APIM Try Out functionality.
Related Issues
#864
Development Checklist
Testing Checklist
Automation Test Details
Conformance Tests Details
Resources
Knowledge Base: https://sites.google.com/wso2.com/open-banking/
Guides: https://sites.google.com/wso2.com/open-banking/developer-guides
Summary by CodeRabbit
Refactor
Tests
✏️ Tip: You can customize this high-level summary in your review settings.