Give precedence to toml configuration for the resident Key Manager #13015
Conversation
nimsara66
requested review from
tharindu1st,
AnuGayan,
chamilaadhi,
Arshardh,
dushaniw and
tgtshanika
as code owners
📝 WalkthroughWalkthroughThis pull request updates the configuration logic in Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant APIUtil
participant DTO as KeyManagerConfigurationDTO
Client->>APIUtil: getAndSetDefaultKeyManagerConfiguration(dto)
APIUtil->>DTO: Set AUTHSERVER_URL, REVOKE_URL, TOKEN_URL
APIUtil->>DTO: Set TOKEN_ENDPOINT, REVOKE_ENDPOINT
APIUtil-->>Client: Return updated dto
✨ 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
🔭 Outside diff range comments (2)
components/apimgt/org.wso2.carbon.apimgt.impl/src/main/java/org/wso2/carbon/apimgt/impl/utils/APIUtil.java (2)
8626-8628: 🛠️ Refactor suggestionAdd null check for keyManagerUrl before accessing it
The keyManagerUrl is accessed without validating if it's null. This could lead to NullPointerException.
+ if (StringUtils.isEmpty(keyManagerUrl)) { + throw new APIManagementException("Key Manager URL cannot be empty"); + }
11519-11538: 🛠️ Refactor suggestionAdd input validation for organization reference parameters
The method lacks validation for input parameters referenceId and organizationName. These should be validated before processing.
public static synchronized String getOrganizationIdFromExternalReference(String referenceId, String organizationName, String rootOrganization) throws APIManagementException { + if (StringUtils.isEmpty(referenceId)) { + throw new APIManagementException("External reference ID cannot be empty"); + } + if (StringUtils.isEmpty(organizationName)) { + throw new APIManagementException("Organization name cannot be empty"); + } String organizationId = null; // Rest of the code... }
🧹 Nitpick comments (4)
components/apimgt/org.wso2.carbon.apimgt.impl/src/main/java/org/wso2/carbon/apimgt/impl/utils/APIUtil.java (4)
8641-8644: Avoid redundant property settingTOKEN_ENDPOINT and REVOKE_ENDPOINT properties are being set redundantly since they are already set as TOKEN_URL and REVOKE_URL.
Consider consolidating the property setting to avoid duplication:
- keyManagerConfigurationDTO.addProperty(APIConstants.KeyManager.TOKEN_ENDPOINT, - keyManagerConfigurationDTO.getAdditionalProperties().get(APIConstants.TOKEN_URL)); - keyManagerConfigurationDTO.addProperty(APIConstants.KeyManager.REVOKE_ENDPOINT, - keyManagerConfigurationDTO.getAdditionalProperties().get(APIConstants.REVOKE_URL));
11540-11551: Add length validation for organization handleThe method should validate the length of the generated handle to ensure it meets system constraints.
public static String getOrganizationHandle(String name) { String sanatizedName = null; if (name == null) { return sanatizedName; } String nowhitespace = WHITESPACE.matcher(name).replaceAll("-"); String normalized = Normalizer.normalize(nowhitespace, Normalizer.Form.NFD); sanatizedName = NONLATIN.matcher(normalized).replaceAll(""); sanatizedName = sanatizedName.toLowerCase(Locale.ENGLISH).replaceAll("^-+|-+$", ""); + // Validate handle length + if (sanatizedName.length() > 255) { + sanatizedName = sanatizedName.substring(0, 255); + } return sanatizedName; }
11558-11585: Improve resource cleanup and exception documentationThe method should ensure proper cleanup of resources and document all possible exceptions.
- Add documentation for exceptions:
/** * Validate Api with the federated gateways * * @param api API Object + * @throws APIManagementException if validation fails or implementation class cannot be loaded + * @throws ExternalGatewayAPIValidationException if validation fails with federated gateway */
- Consider using try-with-resources for any closeable resources used by the deployer:
+ if (deployer instanceof AutoCloseable) { + try (AutoCloseable closeable = (AutoCloseable) deployer) { + // Validation logic + } catch (Exception e) { + throw new APIManagementException("Error closing deployer", e); + } + }
11322-11357: Improve hash generation methods with constants and validationThe hash generation methods could be improved with better constants, validation and performance handling.
+ private static final int MAX_PAYLOAD_SIZE = 10 * 1024 * 1024; // 10MB limit + private static final String DEFAULT_HASH_ALGORITHM = "SHA-256"; public static String generateHashValue(String payload) throws APIManagementException { + if (payload == null) { + throw new APIManagementException("Payload cannot be null"); + } + if (payload.length() > MAX_PAYLOAD_SIZE) { + throw new APIManagementException("Payload size exceeds maximum allowed size"); + } try { - MessageDigest messageDigest = MessageDigest.getInstance(hashingAlgorithm); + MessageDigest messageDigest = MessageDigest.getInstance( + StringUtils.isEmpty(hashingAlgorithm) ? DEFAULT_HASH_ALGORITHM : hashingAlgorithm); // Rest of the code... } }
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
components/apimgt/org.wso2.carbon.apimgt.impl/src/main/java/org/wso2/carbon/apimgt/impl/utils/APIUtil.java(3 hunks)
Overview
The current implementation of the resident Key Manager configuration initially relies on the settings provided in the TOML files. However, once the resident Key Manager configuration is saved, subsequent configurations are read from the database. As a result, certain changes cannot be updated via the UI, nor are they affected by modifications in the TOML configuration.
This pull request (PR) prioritizes TOML-based configurations over database-stored configurations for the resident Key Manager, ensuring that changes made in the TOML files take precedence. Additionally, it fixes the issue where the
openid-configurationendpoint is not being updated in the resident Key Manager.Issue
wso2/api-manager#3572