Automatic refresh for SAML IDP Certificates from Metadata URL#7782
Open
DilshanSenarath wants to merge 6 commits intowso2:masterfrom
Open
Automatic refresh for SAML IDP Certificates from Metadata URL#7782DilshanSenarath wants to merge 6 commits intowso2:masterfrom
DilshanSenarath wants to merge 6 commits intowso2:masterfrom
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review infoConfiguration used: Path: .coderabbit.yml Review profile: CHILL Plan: Pro 📒 Files selected for processing (4)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughAdds SAML metadata endpoint parsing to the IdentityProvider model; implements response-size limits for the external API client (default and per-invocation), streaming enforcement with fast-fail, new error codes and validations, configuration entries for default limits, and supporting unit tests and test config updates. Changes
🚥 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 |
![wso2-engineering[bot]](https://avatars.githubusercontent.com/in/1856828?s=60&v=4){kind=small}
Comment on lines
+240
to
+241
| } else if (FILE_ELEMENT_SAML_METADATA_ENDPOINT.equals(elementName)) { | ||
| IdentityProviderProperty samlMetadataEndpoint = new IdentityProviderProperty(); |
Contributor
There was a problem hiding this comment.
Log Improvement Suggestion No: 1
Suggested change
| } else if (FILE_ELEMENT_SAML_METADATA_ENDPOINT.equals(elementName)) { | |
| IdentityProviderProperty samlMetadataEndpoint = new IdentityProviderProperty(); | |
| } else if (FILE_ELEMENT_SAML_METADATA_ENDPOINT.equals(elementName)) { | |
| log.debug("Processing SAML Metadata Endpoint for Identity Provider"); | |
| IdentityProviderProperty samlMetadataEndpoint = new IdentityProviderProperty(); |
| return this; | ||
| } | ||
|
|
||
| public APIClientConfig build() throws APIClientConfigException { |
Contributor
There was a problem hiding this comment.
Log Improvement Suggestion No: 2
Suggested change
| public APIClientConfig build() throws APIClientConfigException { | |
| public APIClientConfig build() throws APIClientConfigException { | |
| log.debug("Building APIClientConfig with responseLimitInBytes: " + responseLimitInBytes); |
Comment on lines
+183
to
+184
| if (value <= 0) { | ||
| throw new APIClientConfigException(ERROR_CODE_INVALID_CONFIG_VALUE, Long.toString(value)); |
Contributor
There was a problem hiding this comment.
Log Improvement Suggestion No: 3
Suggested change
| if (value <= 0) { | |
| throw new APIClientConfigException(ERROR_CODE_INVALID_CONFIG_VALUE, Long.toString(value)); | |
| if (value <= 0) { | |
| log.error("Invalid configuration value provided: " + value); | |
| throw new APIClientConfigException(ERROR_CODE_INVALID_CONFIG_VALUE, Long.toString(value)); |
Comment on lines
+184
to
185
| return handleResponse(response, effectiveResponseLimit); | ||
| } catch (IOException e) { |
Contributor
There was a problem hiding this comment.
Log Improvement Suggestion No: 4
Suggested change
| return handleResponse(response, effectiveResponseLimit); | |
| } catch (IOException e) { | |
| } | |
| return handleResponse(response, effectiveResponseLimit); | |
| } catch (IOException e) { | |
| LOG.error(String.format("Failed to execute request to URI: %s on attempt %d/%d: %s", | |
| request.getURI(), attempt, allowedAttemptCount, e.getMessage())); | |
| if (attempt == allowedAttemptCount) { |
Comment on lines
+233
to
+235
| buffer.write(chunk, 0, bytesRead); | ||
| } | ||
| responseBody = buffer.toString(StandardCharsets.UTF_8); |
Contributor
There was a problem hiding this comment.
Log Improvement Suggestion No: 5
Suggested change
| buffer.write(chunk, 0, bytesRead); | |
| } | |
| responseBody = buffer.toString(StandardCharsets.UTF_8); | |
| buffer.write(chunk, 0, bytesRead); | |
| } | |
| responseBody = buffer.toString(StandardCharsets.UTF_8); | |
| if (LOG.isDebugEnabled()) { | |
| LOG.debug(String.format("Successfully read response body of size: %d bytes", totalBytesRead)); | |
| } | |
| } |
Comment on lines
+134
to
+137
| private static long getLongProperty(String propertyName) { | ||
|
|
||
| Object configValue = identityConfigParser.getConfiguration().get(propertyName); | ||
| try { |
Contributor
There was a problem hiding this comment.
Log Improvement Suggestion No: 6
Suggested change
| private static long getLongProperty(String propertyName) { | |
| Object configValue = identityConfigParser.getConfiguration().get(propertyName); | |
| try { | |
| private static long getLongProperty(String propertyName) { | |
| Object configValue = identityConfigParser.getConfiguration().get(propertyName); | |
| log.debug("Retrieving long property: {} with value: {}", propertyName, configValue); |
Comment on lines
+143
to
+144
| } catch (NumberFormatException e) { | ||
| throw new IllegalArgumentException( |
Contributor
There was a problem hiding this comment.
Log Improvement Suggestion No: 7
Suggested change
| } catch (NumberFormatException e) { | |
| throw new IllegalArgumentException( | |
| } catch (NumberFormatException e) { | |
| log.error("Failed to parse long property: {} with value: {}", propertyName, configValue); | |
| throw new IllegalArgumentException( |
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}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (4)
components/application-mgt/org.wso2.carbon.identity.application.common/src/main/java/org/wso2/carbon/identity/application/common/model/IdentityProvider.java (1)
240-245: Guard against blank SAML metadata endpoint values before persisting the property.At Line 240, the property is added even if
element.getText()is empty/whitespace. That can make downstream logic treat an invalid endpoint as configured.Suggested patch
} else if (FILE_ELEMENT_SAML_METADATA_ENDPOINT.equals(elementName)) { - IdentityProviderProperty samlMetadataEndpoint = new IdentityProviderProperty(); - samlMetadataEndpoint.setName(SAML_METADATA_URI); - samlMetadataEndpoint.setValue(element.getText()); - samlMetadataEndpoint.setDisplayName(SAML_METADATA_DISPLAY_NAME); - idpProperties.add(samlMetadataEndpoint); + String samlMetadataEndpointValue = element.getText(); + if (StringUtils.isNotBlank(samlMetadataEndpointValue)) { + IdentityProviderProperty samlMetadataEndpoint = new IdentityProviderProperty(); + samlMetadataEndpoint.setName(SAML_METADATA_URI); + samlMetadataEndpoint.setValue(samlMetadataEndpointValue); + samlMetadataEndpoint.setDisplayName(SAML_METADATA_DISPLAY_NAME); + idpProperties.add(samlMetadataEndpoint); + } } else if (FILE_ELEMENT_FEDERATED_AUTHENTICATOR_CONFIGS.equals(elementName)) {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@components/application-mgt/org.wso2.carbon.identity.application.common/src/main/java/org/wso2/carbon/identity/application/common/model/IdentityProvider.java` around lines 240 - 245, When parsing FILE_ELEMENT_SAML_METADATA_ENDPOINT in IdentityProvider, guard against blank or whitespace-only values from element.getText() before creating and adding the IdentityProviderProperty (samlMetadataEndpoint) to idpProperties: check that element.getText() is non-null and element.getText().trim().length() > 0 (or use isBlank/isEmpty as available) and only then instantiate samlMetadataEndpoint, set SAML_METADATA_URI, SAML_METADATA_DISPLAY_NAME and add to idpProperties; otherwise skip creating/adding the property so no empty SAML_METADATA_URI entries are persisted.components/application-mgt/org.wso2.carbon.identity.application.common/src/test/java/org/wso2/carbon/identity/application/common/model/test/model/IdentityProviderTest.java (1)
87-90: Add a null assertion before streamingidpPropertiesfor clearer failures.At Line 89, adding
assertNotNull(properties, ...)beforeArrays.stream(properties)will produce a more direct failure message if initialization behavior changes.Suggested patch
assertNotNull(identityProvider, "IdentityProvider must not be null."); IdentityProviderProperty[] properties = identityProvider.getIdpProperties(); + assertNotNull(properties, "IdpProperties must not be null."); boolean hasSamlMetadataProp = Arrays.stream(properties) .anyMatch(p -> SAML_METADATA_URI.equals(p.getName()));🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@components/application-mgt/org.wso2.carbon.identity.application.common/src/test/java/org/wso2/carbon/identity/application/common/model/test/model/IdentityProviderTest.java` around lines 87 - 90, Add a null-check assertion for the IdentityProvider idpProperties before streaming them: in IdentityProviderTest, assert that the local variable properties (from identityProvider.getIdpProperties()) is not null (e.g., using assertNotNull) immediately before calling Arrays.stream(properties) so failures are clearer; keep the existing hasSamlMetadataProp logic and only insert the assertNotNull(properties, ...) prior to the stream operation.components/external-api-framework/org.wso2.carbon.identity.external.api.client/src/main/java/org/wso2/carbon/identity/external/api/client/api/model/APIInvocationConfig.java (1)
77-92: Consider adding an explicit way to clear per-invocation response-limit override.The docs define
nullas “defer to client-level default,” but once set, this object cannot revert tonullwithout recreating it.♻️ Suggested API ergonomics improvement
public void setResponseLimitInBytes(long responseLimitInBytes) throws APIClientConfigException { if (responseLimitInBytes <= 0) { throw new APIClientConfigException(ERROR_CODE_INVALID_RESPONSE_LIMIT, String.valueOf(responseLimitInBytes)); } this.responseLimitInBytes = responseLimitInBytes; } + +/** + * Clear the per-request response size limit override and defer to the client-level default. + */ +public void clearResponseLimitInBytes() { + + this.responseLimitInBytes = null; +}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@components/external-api-framework/org.wso2.carbon.identity.external.api.client/src/main/java/org/wso2/carbon/identity/external/api/client/api/model/APIInvocationConfig.java` around lines 77 - 92, The APIInvocationConfig currently only offers setResponseLimitInBytes(long) and no way to revert the per-invocation override to null; add an explicit way to clear the override (either an overloaded setter accepting Long that treats null as “defer to client default” or a clearResponseLimitOverride() method) and update the responseLimitInBytes field handling accordingly; ensure the new API enforces the same validation (throwing APIClientConfigException for non-positive non-null values) and document that passing null (or calling clearResponseLimitOverride) resets to client-level default.components/external-api-framework/org.wso2.carbon.identity.external.api.client/src/test/java/org/wso2/carbon/identity/external/api/client/internal/service/APIClientTest.java (1)
75-79: Consider usingServerSocket(0)for dynamic port allocation.The current approach using
Math.random()to select a port in the range 8000-8999 could potentially cause port conflicts in parallel test execution or if another service is using the port.♻️ Suggested improvement for dynamic port allocation
`@BeforeMethod` public void setUp() throws Exception { - // Find an available port. - serverPort = 8000 + (int) (Math.random() * 1000); + // Find an available port dynamically. + try (java.net.ServerSocket socket = new java.net.ServerSocket(0)) { + serverPort = socket.getLocalPort(); + } // Create real APIClient with actual configuration.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@components/external-api-framework/org.wso2.carbon.identity.external.api.client/src/test/java/org/wso2/carbon/identity/external/api/client/internal/service/APIClientTest.java` around lines 75 - 79, The setUp method currently assigns serverPort using 8000 + random, risking conflicts; replace that logic by opening a ServerSocket(0) to acquire an ephemeral free port, read the port via getLocalPort(), assign it to the serverPort field, then close the ServerSocket before starting the test server; update the setUp() method (and any test teardown if needed) so serverPort is obtained safely and deterministically using ServerSocket instead of Math.random().
🤖 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/external-api-framework/org.wso2.carbon.identity.external.api.client/src/main/java/org/wso2/carbon/identity/external/api/client/internal/util/APIClientUtils.java`:
- Around line 134-147: The getLongProperty method currently allows zero or
negative values; update it to enforce that when parsing the specific
configuration key "DefaultResponseLimit" the returned long must be strictly
positive. After parsing the long in getLongProperty (method name:
getLongProperty), check if propertyName equals "DefaultResponseLimit" and if the
parsed value is <= 0, throw an IllegalArgumentException with a clear message
that the DefaultResponseLimit must be a positive long; keep existing
NumberFormatException handling intact for non-numeric values.
---
Nitpick comments:
In
`@components/application-mgt/org.wso2.carbon.identity.application.common/src/main/java/org/wso2/carbon/identity/application/common/model/IdentityProvider.java`:
- Around line 240-245: When parsing FILE_ELEMENT_SAML_METADATA_ENDPOINT in
IdentityProvider, guard against blank or whitespace-only values from
element.getText() before creating and adding the IdentityProviderProperty
(samlMetadataEndpoint) to idpProperties: check that element.getText() is
non-null and element.getText().trim().length() > 0 (or use isBlank/isEmpty as
available) and only then instantiate samlMetadataEndpoint, set
SAML_METADATA_URI, SAML_METADATA_DISPLAY_NAME and add to idpProperties;
otherwise skip creating/adding the property so no empty SAML_METADATA_URI
entries are persisted.
In
`@components/application-mgt/org.wso2.carbon.identity.application.common/src/test/java/org/wso2/carbon/identity/application/common/model/test/model/IdentityProviderTest.java`:
- Around line 87-90: Add a null-check assertion for the IdentityProvider
idpProperties before streaming them: in IdentityProviderTest, assert that the
local variable properties (from identityProvider.getIdpProperties()) is not null
(e.g., using assertNotNull) immediately before calling Arrays.stream(properties)
so failures are clearer; keep the existing hasSamlMetadataProp logic and only
insert the assertNotNull(properties, ...) prior to the stream operation.
In
`@components/external-api-framework/org.wso2.carbon.identity.external.api.client/src/main/java/org/wso2/carbon/identity/external/api/client/api/model/APIInvocationConfig.java`:
- Around line 77-92: The APIInvocationConfig currently only offers
setResponseLimitInBytes(long) and no way to revert the per-invocation override
to null; add an explicit way to clear the override (either an overloaded setter
accepting Long that treats null as “defer to client default” or a
clearResponseLimitOverride() method) and update the responseLimitInBytes field
handling accordingly; ensure the new API enforces the same validation (throwing
APIClientConfigException for non-positive non-null values) and document that
passing null (or calling clearResponseLimitOverride) resets to client-level
default.
In
`@components/external-api-framework/org.wso2.carbon.identity.external.api.client/src/test/java/org/wso2/carbon/identity/external/api/client/internal/service/APIClientTest.java`:
- Around line 75-79: The setUp method currently assigns serverPort using 8000 +
random, risking conflicts; replace that logic by opening a ServerSocket(0) to
acquire an ephemeral free port, read the port via getLocalPort(), assign it to
the serverPort field, then close the ServerSocket before starting the test
server; update the setUp() method (and any test teardown if needed) so
serverPort is obtained safely and deterministically using ServerSocket instead
of Math.random().
ℹ️ Review info
Configuration used: Path: .coderabbit.yml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (18)
components/application-mgt/org.wso2.carbon.identity.application.common/src/main/java/org/wso2/carbon/identity/application/common/model/IdentityProvider.javacomponents/application-mgt/org.wso2.carbon.identity.application.common/src/test/java/org/wso2/carbon/identity/application/common/model/test/model/IdentityProviderTest.javacomponents/application-mgt/org.wso2.carbon.identity.application.common/src/test/resources/testng.xmlcomponents/external-api-framework/org.wso2.carbon.identity.external.api.client/src/main/java/org/wso2/carbon/identity/external/api/client/api/constant/ErrorMessageConstant.javacomponents/external-api-framework/org.wso2.carbon.identity.external.api.client/src/main/java/org/wso2/carbon/identity/external/api/client/api/model/APIClientConfig.javacomponents/external-api-framework/org.wso2.carbon.identity.external.api.client/src/main/java/org/wso2/carbon/identity/external/api/client/api/model/APIInvocationConfig.javacomponents/external-api-framework/org.wso2.carbon.identity.external.api.client/src/main/java/org/wso2/carbon/identity/external/api/client/internal/service/APIClient.javacomponents/external-api-framework/org.wso2.carbon.identity.external.api.client/src/main/java/org/wso2/carbon/identity/external/api/client/internal/util/APIClientUtils.javacomponents/external-api-framework/org.wso2.carbon.identity.external.api.client/src/test/java/org/wso2/carbon/identity/external/api/client/api/model/APIClientConfigTest.javacomponents/external-api-framework/org.wso2.carbon.identity.external.api.client/src/test/java/org/wso2/carbon/identity/external/api/client/api/model/APIInvocationConfigTest.javacomponents/external-api-framework/org.wso2.carbon.identity.external.api.client/src/test/java/org/wso2/carbon/identity/external/api/client/internal/service/APIClientTest.javacomponents/external-api-framework/org.wso2.carbon.identity.external.api.client/src/test/java/org/wso2/carbon/identity/external/api/client/internal/util/APIClientUtilsTest.javacomponents/external-api-framework/org.wso2.carbon.identity.external.api.client/src/test/resources/repository/conf/identity/identity.xmlcomponents/external-api-framework/org.wso2.carbon.identity.external.api.client/src/test/resources/testng.xmlcomponents/idp-mgt/org.wso2.carbon.idp.mgt/src/main/java/org/wso2/carbon/idp/mgt/IdentityProviderManager.javafeatures/identity-core/org.wso2.carbon.identity.core.server.feature/resources/identity.xmlfeatures/identity-core/org.wso2.carbon.identity.core.server.feature/resources/identity.xml.j2features/identity-core/org.wso2.carbon.identity.core.server.feature/resources/org.wso2.carbon.identity.core.server.feature.default.json
💤 Files with no reviewable changes (1)
- components/idp-mgt/org.wso2.carbon.idp.mgt/src/main/java/org/wso2/carbon/idp/mgt/IdentityProviderManager.java
Comment on lines
+134
to
+147
| private static long getLongProperty(String propertyName) { | ||
|
|
||
| Object configValue = identityConfigParser.getConfiguration().get(propertyName); | ||
| try { | ||
| if (configValue == null) { | ||
| throw new IllegalArgumentException( | ||
| String.format("The API client configuration %s value must be a long.", propertyName)); | ||
| } | ||
| return Long.parseLong(configValue.toString()); | ||
| } catch (NumberFormatException e) { | ||
| throw new IllegalArgumentException( | ||
| String.format("The API client configuration %s value must be a long.", propertyName)); | ||
| } | ||
| } |
There was a problem hiding this comment.
Validate DefaultResponseLimit as strictly positive during config parsing.
Line 142 currently accepts 0/negative values, which conflicts with positive-only response-limit validation and can propagate an invalid default.
💡 Proposed fix
private static long getLongProperty(String propertyName) {
Object configValue = identityConfigParser.getConfiguration().get(propertyName);
try {
if (configValue == null) {
throw new IllegalArgumentException(
- String.format("The API client configuration %s value must be a long.", propertyName));
+ String.format("The API client configuration %s value must be a positive long.", propertyName));
}
- return Long.parseLong(configValue.toString());
+ long parsedValue = Long.parseLong(configValue.toString());
+ if (parsedValue <= 0) {
+ throw new IllegalArgumentException(
+ String.format("The API client configuration %s value must be a positive long.", propertyName));
+ }
+ return parsedValue;
} catch (NumberFormatException e) {
throw new IllegalArgumentException(
- String.format("The API client configuration %s value must be a long.", propertyName));
+ String.format("The API client configuration %s value must be a positive long.", propertyName));
}
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| private static long getLongProperty(String propertyName) { | |
| Object configValue = identityConfigParser.getConfiguration().get(propertyName); | |
| try { | |
| if (configValue == null) { | |
| throw new IllegalArgumentException( | |
| String.format("The API client configuration %s value must be a long.", propertyName)); | |
| } | |
| return Long.parseLong(configValue.toString()); | |
| } catch (NumberFormatException e) { | |
| throw new IllegalArgumentException( | |
| String.format("The API client configuration %s value must be a long.", propertyName)); | |
| } | |
| } | |
| private static long getLongProperty(String propertyName) { | |
| Object configValue = identityConfigParser.getConfiguration().get(propertyName); | |
| try { | |
| if (configValue == null) { | |
| throw new IllegalArgumentException( | |
| String.format("The API client configuration %s value must be a positive long.", propertyName)); | |
| } | |
| long parsedValue = Long.parseLong(configValue.toString()); | |
| if (parsedValue <= 0) { | |
| throw new IllegalArgumentException( | |
| String.format("The API client configuration %s value must be a positive long.", propertyName)); | |
| } | |
| return parsedValue; | |
| } catch (NumberFormatException e) { | |
| throw new IllegalArgumentException( | |
| String.format("The API client configuration %s value must be a positive long.", propertyName)); | |
| } | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@components/external-api-framework/org.wso2.carbon.identity.external.api.client/src/main/java/org/wso2/carbon/identity/external/api/client/internal/util/APIClientUtils.java`
around lines 134 - 147, The getLongProperty method currently allows zero or
negative values; update it to enforce that when parsing the specific
configuration key "DefaultResponseLimit" the returned long must be strictly
positive. After parsing the long in getLongProperty (method name:
getLongProperty), check if propertyName equals "DefaultResponseLimit" and if the
parsed value is <= 0, throw an IllegalArgumentException with a clear message
that the DefaultResponseLimit must be a positive long; keep existing
NumberFormatException handling intact for non-numeric values.
Contributor
There was a problem hiding this comment.
Pull request overview
This PR adds automatic refresh capability for SAML IDP certificates by implementing a response size limit feature in the external API client framework. The feature allows the system to safely fetch SAML metadata from external URLs with configurable response size limits to prevent denial-of-service attacks through oversized responses.
Changes:
- Added configurable response size limit (default 50KB) to the external API client framework with support for both client-level and per-invocation overrides
- Added SAML metadata endpoint support to the IdentityProvider model for storing metadata URLs
- Removed unused imports from IdentityProviderManager
Reviewed changes
Copilot reviewed 18 out of 18 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| org.wso2.carbon.identity.core.server.feature.default.json | Added default response limit configuration (51200 bytes) |
| identity.xml.j2 | Added template variable for response limit configuration |
| identity.xml (core feature) | Added ExternalAPIClient configuration with response limit of 51200 bytes |
| identity.xml (test resources) | Added test configuration with response limit of 1048576 bytes |
| IdentityProviderManager.java | Removed unused imports (UserCoreUtil, Collectors) |
| testng.xml (external-api-client) | Added new test class APIClientConfigTest |
| testng.xml (application-common) | Added new test class IdentityProviderTest |
| APIClientUtilsTest.java | Added test for default response limit retrieval |
| APIClientTest.java | Added comprehensive tests for response limit enforcement scenarios |
| APIInvocationConfigTest.java | Added tests for per-invocation response limit override |
| APIClientConfigTest.java | New test file for APIClientConfig response limit functionality |
| IdentityProviderTest.java | New test file for SAMLMetadataEndpoint XML parsing |
| APIClientUtils.java | Added getDefaultResponseLimit() method and long property parsing |
| APIClient.java | Implemented response size limit enforcement with chunk-based reading |
| APIInvocationConfig.java | Added per-invocation response limit override support |
| APIClientConfig.java | Added response limit configuration field and validation |
| ErrorMessageConstant.java | Added error codes for invalid response limit and limit exceeded |
| IdentityProvider.java | Added SAMLMetadataEndpoint XML element parsing and storage as idpProperty |
Comment on lines
+53
to
+54
| ERROR_CODE_INVALID_RESPONSE_LIMIT("APICLIENT-65012", "Invalid response size limit.", | ||
| "The response size limit %s must be a positive integer."), |
There was a problem hiding this comment.
The error message description says "must be a positive integer" but the field is a long type. This should be "must be a positive long" or "must be a positive number" for accuracy.
Comment on lines
+98
to
+106
| /** | ||
| * Get the response size limit in bytes. | ||
| * | ||
| * @return response size limit in bytes. | ||
| */ | ||
| public long getResponseLimitInBytes() { | ||
|
|
||
| return responseLimitInBytes; | ||
| } |
There was a problem hiding this comment.
The comment for the getter method does not end with a period. According to the coding guidelines, all comments should end with a period.
Comment on lines
+109
to
+117
| /** | ||
| * Gets the default response limit in bytes. | ||
| * | ||
| * @return default response limit in bytes. | ||
| */ | ||
| public static long getDefaultResponseLimit() { | ||
|
|
||
| return DEFAULT_RESPONSE_LIMIT; | ||
| } |
There was a problem hiding this comment.
The comment for this method does not end with a period. According to the coding guidelines, all comments should end with a period.
Comment on lines
+151
to
+161
| /** | ||
| * Set the response size limit in bytes. | ||
| * | ||
| * @param responseLimitInBytes response size limit in bytes. | ||
| * @return this builder. | ||
| */ | ||
| public APIClientConfig.Builder responseLimitInBytes(long responseLimitInBytes) { | ||
|
|
||
| this.responseLimitInBytes = responseLimitInBytes; | ||
| return this; | ||
| } |
There was a problem hiding this comment.
The comment for this method does not end with a period. According to the coding guidelines, all comments should end with a period.
Comment on lines
+51
to
+56
| /** | ||
| * Set the allowed retry count. | ||
| * | ||
| * @param allowedRetryCount allowed retry count. | ||
| * @throws APIClientConfigException if the retry count is negative. | ||
| */ |
There was a problem hiding this comment.
The comments for both @return and @param do not end with periods. According to the coding guidelines, all comments should end with a period.
There was a problem hiding this comment.
🧹 Nitpick comments (2)
features/identity-core/org.wso2.carbon.identity.core.server.feature/resources/identity.xml (2)
1604-1604: Clarify clustered behavior for non-distributed SAML cert cache.With
isDistributed="false"(Line 1604), multi-node deployments may temporarily serve different cert states per node. If that is intentional, please document it; otherwise consider an environment-specific distributed setting or explicit invalidation strategy.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@features/identity-core/org.wso2.carbon.identity.core.server.feature/resources/identity.xml` at line 1604, The SAML certificate cache entry (Cache id="saml_cert_cache", name="SAMLCertCache") is configured with isDistributed="false" which can lead to divergent cert state across cluster nodes; update documentation or config so intent is explicit: either document in the feature/resource notes that SAMLCertCache is intentionally non-distributed for per-node caching and outline an invalidation/refresh strategy, or change the environment-specific configuration to use isDistributed="true" (or wire in a distributed cache implementation) and ensure any cache invalidation logic in the code that references SAMLCertCache (look for usages of saml_cert_cache / SAMLCertCache) is adjusted to support distributed invalidation.
1350-1360: Consider non-zero retries for metadata fetch resilience.
<DefaultRetryCount>0</DefaultRetryCount>(Line 1358) makes certificate refresh fail immediately on transient network glitches. For idempotent metadata GETs, a small retry budget is safer.Suggested tweak
- <DefaultRetryCount>0</DefaultRetryCount> + <DefaultRetryCount>1</DefaultRetryCount>🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@features/identity-core/org.wso2.carbon.identity.core.server.feature/resources/identity.xml` around lines 1350 - 1360, The DefaultRetryCount under ExternalAPIClient is set to 0 which causes metadata (idempotent GET) fetches to fail immediately on transient network errors; change the <DefaultRetryCount> value in the ExternalAPIClient config (near the HTTPClient block) to a small non-zero integer (e.g., 2 or 3) and ensure the retry logic applies only to idempotent HTTP GETs for metadata refreshes so transient glitches are retried with a small backoff.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In
`@features/identity-core/org.wso2.carbon.identity.core.server.feature/resources/identity.xml`:
- Line 1604: The SAML certificate cache entry (Cache id="saml_cert_cache",
name="SAMLCertCache") is configured with isDistributed="false" which can lead to
divergent cert state across cluster nodes; update documentation or config so
intent is explicit: either document in the feature/resource notes that
SAMLCertCache is intentionally non-distributed for per-node caching and outline
an invalidation/refresh strategy, or change the environment-specific
configuration to use isDistributed="true" (or wire in a distributed cache
implementation) and ensure any cache invalidation logic in the code that
references SAMLCertCache (look for usages of saml_cert_cache / SAMLCertCache) is
adjusted to support distributed invalidation.
- Around line 1350-1360: The DefaultRetryCount under ExternalAPIClient is set to
0 which causes metadata (idempotent GET) fetches to fail immediately on
transient network errors; change the <DefaultRetryCount> value in the
ExternalAPIClient config (near the HTTPClient block) to a small non-zero integer
(e.g., 2 or 3) and ensure the retry logic applies only to idempotent HTTP GETs
for metadata refreshes so transient glitches are retried with a small backoff.
ℹ️ Review info
Configuration used: Path: .coderabbit.yml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
features/identity-core/org.wso2.carbon.identity.core.server.feature/resources/identity.xmlfeatures/identity-core/org.wso2.carbon.identity.core.server.feature/resources/identity.xml.j2features/identity-core/org.wso2.carbon.identity.core.server.feature/resources/org.wso2.carbon.identity.core.server.feature.default.json
🚧 Files skipped from review as they are similar to previous changes (2)
- features/identity-core/org.wso2.carbon.identity.core.server.feature/resources/org.wso2.carbon.identity.core.server.feature.default.json
- features/identity-core/org.wso2.carbon.identity.core.server.feature/resources/identity.xml.j2
|
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
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
Related Issue