Key Manager Visibility, Gateway Visibility Bug Fixes and Other Fixes #13021
Conversation
Avishka-Shamendra
requested review from
tharindu1st,
AnuGayan,
chamilaadhi,
Arshardh,
dushaniw and
tgtshanika
as code owners
📝 WalkthroughWalkthroughThis pull request introduces several changes across multiple classes. It adds a new assignment in the copy constructor of the Changes
Suggested reviewers
📜 Recent review detailsConfiguration used: .coderabbit.yaml 📒 Files selected for processing (5)
⏰ Context from checks skipped due to timeout of 90000ms (6)
🔇 Additional comments (11)
✨ Finishing Touches
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
Documentation and Community
|
![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
🧹 Nitpick comments (4)
components/apimgt/org.wso2.carbon.apimgt.impl/src/main/java/org/wso2/carbon/apimgt/impl/utils/APIUtil.java (4)
329-332: Consider adding more detailed class-level documentationWhile the class has a basic Javadoc comment, it would be helpful to add more details about:
- The key responsibilities and purpose of this utility class
- Any important usage patterns or considerations
- Links to related classes/documentation
6814-6877: Consider breaking down this long method into smaller focused methodsThe
marketplaceAssistantDeleteServicemethod is quite long and handles multiple responsibilities including:
- URL construction
- HTTP client setup
- Request execution
- Response handling
- Error handling
Consider extracting some of these into separate helper methods for better maintainability and testability.
11196-11202: Enhance error messages with more contextThe error messages in the catch blocks could be more descriptive by including:
- The specific operation that failed
- The relevant parameters/context
- A more user-friendly message format
For example:
- throw new APIManagementException("Error encountered while connecting to service", e); + throw new APIManagementException("Failed to connect to AI service endpoint: " + endpoint + ". Please check the endpoint configuration and network connectivity.", e);
11004-11015: Consider additional input validation for securityThe
generateCodeVerifiermethod generates security-sensitive PKCE code verifiers. Consider adding:
- Validation of the generated value length
- Additional entropy checks
- Proper cleanup of sensitive data
Example:
public static String generateCodeVerifier() { SecureRandom secureRandom = new SecureRandom(); byte[] codeVerifier = new byte[32]; try { secureRandom.nextBytes(codeVerifier); + // Validate minimum entropy + if (!hasMinimumEntropy(codeVerifier)) { + throw new SecurityException("Generated code verifier does not meet minimum entropy requirements"); + } return java.util.Base64.getUrlEncoder().withoutPadding().encodeToString(codeVerifier); } finally { + // Clean up sensitive data + Arrays.fill(codeVerifier, (byte) 0); } }
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
components/apimgt/org.wso2.carbon.apimgt.api/src/main/java/org/wso2/carbon/apimgt/api/model/Environment.java(1 hunks)components/apimgt/org.wso2.carbon.apimgt.impl/src/main/java/org/wso2/carbon/apimgt/impl/APIConsumerImpl.java(2 hunks)components/apimgt/org.wso2.carbon.apimgt.impl/src/main/java/org/wso2/carbon/apimgt/impl/utils/APIUtil.java(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (4)
- GitHub Check: build-product (2, group2)
- GitHub Check: build-product (1, group1)
- GitHub Check: build-carbon
- GitHub Check: run-benchmark-test
🔇 Additional comments (4)
components/apimgt/org.wso2.carbon.apimgt.api/src/main/java/org/wso2/carbon/apimgt/api/model/Environment.java (1)
86-86: Properly initialize permissions field in the copy constructorGood addition to ensure that the
permissionsfield is properly copied from the source Environment instance. This fixes a bug where the permissions configuration would be reset to default when cloning an Environment object.components/apimgt/org.wso2.carbon.apimgt.impl/src/main/java/org/wso2/carbon/apimgt/impl/APIConsumerImpl.java (2)
165-165: New Locale import added for case-insensitive comparison.This import is required for the enhanced
hasIntersectionmethod that now usesLocale.ENGLISHfor consistent case-insensitive string comparisons.
4780-4794: Improved case-insensitive string comparison inhasIntersectionmethod.The method has been enhanced to perform case-insensitive comparisons by converting strings to lowercase using
Locale.ENGLISHbefore comparison. This is a good practice as it ensures consistent behavior across different system locales when checking for role intersections.Using
Locale.ENGLISHexplicitly instead of the default locale prevents potential inconsistencies across different deployment environments, especially in internationalized applications.components/apimgt/org.wso2.carbon.apimgt.impl/src/main/java/org/wso2/carbon/apimgt/impl/utils/APIUtil.java (1)
6372-6387: LGTM! Case-insensitive string comparison implementation looks good.The implementation correctly handles case-insensitive comparison by converting elements to lowercase using Locale.ENGLISH before checking intersection. The early return optimization and use of HashSet for O(1) lookups makes it efficient.
Avishka-Shamendra
changed the title
Purpose
Fixes wso2/api-manager#3730
Fixes wso2/api-manager#3727
Close unclosed resource streams
This pull request includes various improvements and fixes across multiple files. The most important changes include adding permissions to the
Environmentclass, ensuring case insensitivity in thehasIntersectionmethod, and updating theswaggerYamlGetmethod to useInputStreamfor reading YAML files.Enhancements to
Environmentclass:permissionsto theEnvironmentclass and updated the constructor to initialize it. (components/apimgt/org.wso2.carbon.apimgt.api/src/main/java/org/wso2/carbon/apimgt/api/model/Environment.java)Case insensitivity in
hasIntersectionmethod:hasIntersectionmethod to convert elements to lowercase before adding to the set and checking for intersections. (components/apimgt/org.wso2.carbon.apimgt.impl/src/main/java/org/wso2/carbon/apimgt/impl/APIConsumerImpl.java)hasIntersectionmethod inAPIUtilclass. (components/apimgt/org.wso2.carbon.apimgt.impl/src/main/java/org/wso2/carbon/apimgt/impl/utils/APIUtil.java)Improvements to
swaggerYamlGetmethod:swaggerYamlGetmethod to useInputStreamfor reading YAML files inSwaggerYamlApiclasses across various modules:components/apimgt/org.wso2.carbon.apimgt.governance.rest.api/src/main/java/org/wso2/carbon/apimgt/governance/rest/api/SwaggerYamlApi.javacomponents/apimgt/org.wso2.carbon.apimgt.rest.api.admin.v1/src/main/java/org/wso2/carbon/apimgt/rest/api/admin/v1/SwaggerYamlApi.javacomponents/apimgt/org.wso2.carbon.apimgt.rest.api.gateway/src/main/java/org/wso2/carbon/apimgt/rest/api/gateway/SwaggerYamlApi.javacomponents/apimgt/org.wso2.carbon.apimgt.rest.api.publisher.v1/src/main/java/org/wso2/carbon/apimgt/rest/api/publisher/v1/SwaggerYamlApi.javacomponents/apimgt/org.wso2.carbon.apimgt.rest.api.store.v1/src/main/java/org/wso2/carbon/apimgt/rest/api/store/v1/SwaggerYamlApi.java