Multiple Active Client Secret Support for OAuth Applications #13549
Conversation
hisanhunais
requested review from
AnuGayan,
Arshardh,
HiranyaKavishani,
RakhithaRR,
chamilaadhi,
dushaniw,
senthuran16,
tgtshanika,
tharikaGitHub and
tharindu1st
as code owners
|
Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. 📝 WalkthroughWalkthroughAdds consumer-secret lifecycle support: new APIs and models for generating, listing, and revoking application consumer secrets, Key Manager & KM-client extensions, masking utilities, configuration flags, and error codes; implemented across interfaces, core impl, REST endpoints, DTO mappings, and OpenAPI specs. Changes
Sequence Diagram(s)mermaid mermaid mermaid Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes 🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 ast-grep (0.40.5)components/apimgt/org.wso2.carbon.apimgt.impl/src/main/java/org/wso2/carbon/apimgt/impl/utils/APIUtil.javacomponents/apimgt/org.wso2.carbon.apimgt.impl/src/main/java/org/wso2/carbon/apimgt/impl/APIConstants.javacomponents/apimgt/org.wso2.carbon.apimgt.impl/src/main/java/org/wso2/carbon/apimgt/impl/APIConsumerImpl.javaThanks 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}
| default ConsumerSecretInfo generateConsumerSecret(String keyManagerName, | ||
| ConsumerSecretRequest consumerSecretRequest) | ||
| throws APIManagementException { | ||
| throw new UnsupportedOperationException("Generating new consumer secret is not supported"); | ||
| } |
There was a problem hiding this comment.
Log Improvement Suggestion No: 1
| default ConsumerSecretInfo generateConsumerSecret(String keyManagerName, | |
| ConsumerSecretRequest consumerSecretRequest) | |
| throws APIManagementException { | |
| throw new UnsupportedOperationException("Generating new consumer secret is not supported"); | |
| } | |
| default ConsumerSecretInfo generateConsumerSecret(String keyManagerName, | |
| ConsumerSecretRequest consumerSecretRequest) | |
| throws APIManagementException { | |
| log.warn("generateConsumerSecret called but not implemented for keyManager: " + keyManagerName); | |
| throw new UnsupportedOperationException("Generating new consumer secret is not supported"); | |
| } |
| default void deleteConsumerSecret(String secretId, String keyManagerName, | ||
| ConsumerSecretRequest consumerSecretRequest) | ||
| throws APIManagementException { | ||
| throw new UnsupportedOperationException("Deleting consumer secret is not supported"); | ||
| } |
There was a problem hiding this comment.
Log Improvement Suggestion No: 2
| default void deleteConsumerSecret(String secretId, String keyManagerName, | |
| ConsumerSecretRequest consumerSecretRequest) | |
| throws APIManagementException { | |
| throw new UnsupportedOperationException("Deleting consumer secret is not supported"); | |
| } | |
| default void deleteConsumerSecret(String secretId, String keyManagerName, | |
| ConsumerSecretRequest consumerSecretRequest) | |
| throws APIManagementException { | |
| log.warn("deleteConsumerSecret called but not implemented for secretId: " + secretId + ", keyManager: " + keyManagerName); | |
| throw new UnsupportedOperationException("Deleting consumer secret is not supported"); | |
| } |
| default ConsumerSecretInfo generateNewApplicationConsumerSecret(ConsumerSecretRequest consumerSecretRequest) | ||
| throws APIManagementException { | ||
| throw new UnsupportedOperationException("Generating new consumer secret is not supported"); | ||
| } |
There was a problem hiding this comment.
Log Improvement Suggestion No: 3
| default ConsumerSecretInfo generateNewApplicationConsumerSecret(ConsumerSecretRequest consumerSecretRequest) | |
| throws APIManagementException { | |
| throw new UnsupportedOperationException("Generating new consumer secret is not supported"); | |
| } | |
| default ConsumerSecretInfo generateNewApplicationConsumerSecret(ConsumerSecretRequest consumerSecretRequest) | |
| throws APIManagementException { | |
| if (log.isDebugEnabled()) { | |
| log.debug("Attempting to generate new consumer secret for client: " + consumerSecretRequest.getClientId()); | |
| } | |
| throw new UnsupportedOperationException("Generating new consumer secret is not supported"); | |
| } |
| default void deleteApplicationConsumerSecret(String secretId, ConsumerSecretRequest consumerSecretRequest) | ||
| throws APIManagementException { | ||
| throw new UnsupportedOperationException("Generating new consumer secret is not supported"); | ||
| } |
There was a problem hiding this comment.
Log Improvement Suggestion No: 4
| default void deleteApplicationConsumerSecret(String secretId, ConsumerSecretRequest consumerSecretRequest) | |
| throws APIManagementException { | |
| throw new UnsupportedOperationException("Generating new consumer secret is not supported"); | |
| } | |
| default void deleteApplicationConsumerSecret(String secretId, ConsumerSecretRequest consumerSecretRequest) | |
| throws APIManagementException { | |
| if (log.isDebugEnabled()) { | |
| log.debug("Attempting to delete consumer secret with ID: " + secretId); | |
| } | |
| throw new UnsupportedOperationException("Generating new consumer secret is not supported"); | |
| } |
| try { | ||
| clientSecret = dcrClient.generateNewApplicationSecret(encodedClientId, clientSecretRequest); |
There was a problem hiding this comment.
Log Improvement Suggestion No: 5
| try { | |
| clientSecret = dcrClient.generateNewApplicationSecret(encodedClientId, clientSecretRequest); | |
| try { | |
| clientSecret = dcrClient.generateNewApplicationSecret(encodedClientId, clientSecretRequest); | |
| log.info("Generated new consumer secret for clientId: " + clientId); | |
| } catch (KeyManagerClientException e) { |
| String encodedSecretId = Base64.getUrlEncoder().encodeToString(secretId.getBytes(StandardCharsets.UTF_8)); | ||
| try { |
There was a problem hiding this comment.
Log Improvement Suggestion No: 6
| String encodedSecretId = Base64.getUrlEncoder().encodeToString(secretId.getBytes(StandardCharsets.UTF_8)); | |
| try { | |
| try { | |
| dcrClient.deleteApplicationSecret(encodedClientId, encodedSecretId); | |
| log.info("Deleted consumer secret with secretId: " + secretId + " for clientId: " + clientId); | |
| } catch (KeyManagerClientException e) { |
| public ConsumerSecretInfo generateConsumerSecret(String keyManagerName, ConsumerSecretRequest consumerSecretRequest) | ||
| throws APIManagementException { | ||
|
|
There was a problem hiding this comment.
Log Improvement Suggestion No: 7
| public ConsumerSecretInfo generateConsumerSecret(String keyManagerName, ConsumerSecretRequest consumerSecretRequest) | |
| throws APIManagementException { | |
| public ConsumerSecretInfo generateConsumerSecret(String keyManagerName, ConsumerSecretRequest consumerSecretRequest) | |
| throws APIManagementException { | |
| log.info("Generating new consumer secret for key manager: " + keyManagerName); |
| public void deleteConsumerSecret(String secretId, String keyManagerName, | ||
| ConsumerSecretRequest consumerSecretRequest) | ||
| throws APIManagementException { |
There was a problem hiding this comment.
Log Improvement Suggestion No: 8
| public void deleteConsumerSecret(String secretId, String keyManagerName, | |
| ConsumerSecretRequest consumerSecretRequest) | |
| throws APIManagementException { | |
| public void deleteConsumerSecret(String secretId, String keyManagerName, | |
| ConsumerSecretRequest consumerSecretRequest) | |
| throws APIManagementException { | |
| log.info("Deleting consumer secret with ID: " + secretId + " for key manager: " + keyManagerName); |
| public static String maskSecret(String secret) { | ||
| boolean isHashingEnabled = OAuthServerConfiguration.getInstance().isClientSecretHashEnabled(); | ||
| if (secret == null || secret.isEmpty() || isHashingEnabled) { |
There was a problem hiding this comment.
Log Improvement Suggestion No: 9
| public static String maskSecret(String secret) { | |
| boolean isHashingEnabled = OAuthServerConfiguration.getInstance().isClientSecretHashEnabled(); | |
| if (secret == null || secret.isEmpty() || isHashingEnabled) { | |
| public static String maskSecret(String secret) { | |
| boolean isHashingEnabled = OAuthServerConfiguration.getInstance().isClientSecretHashEnabled(); | |
| if (log.isDebugEnabled()) { | |
| log.debug("Masking secret. Hashing enabled: " + isHashingEnabled); | |
| } | |
| if (secret == null || secret.isEmpty() || isHashingEnabled) { |
| public static boolean isMultipleClientSecretsEnabled() { | ||
|
|
||
| return ServiceReferenceHolder.getInstance().getOauthServerConfiguration().isMultipleClientSecretsEnabled(); |
There was a problem hiding this comment.
Log Improvement Suggestion No: 10
| public static boolean isMultipleClientSecretsEnabled() { | |
| return ServiceReferenceHolder.getInstance().getOauthServerConfiguration().isMultipleClientSecretsEnabled(); | |
| public static boolean isMultipleClientSecretsEnabled() { | |
| boolean isEnabled = ServiceReferenceHolder.getInstance().getOauthServerConfiguration().isMultipleClientSecretsEnabled(); | |
| if (log.isDebugEnabled()) { | |
| log.debug("Multiple client secrets enabled: " + isEnabled); | |
| } | |
| return isEnabled; |
| String username = RestApiCommonUtil.getLoggedInUsername(); | ||
| Set<APIKey> applicationKeys = getApplicationKeys(applicationId); | ||
| if (applicationKeys == null) { |
There was a problem hiding this comment.
Log Improvement Suggestion No: 11
| String username = RestApiCommonUtil.getLoggedInUsername(); | |
| Set<APIKey> applicationKeys = getApplicationKeys(applicationId); | |
| if (applicationKeys == null) { | |
| String username = RestApiCommonUtil.getLoggedInUsername(); | |
| log.debug("Generating consumer secret for application: {} and key mapping: {}", applicationId, keyMappingId); | |
| Set<APIKey> applicationKeys = getApplicationKeys(applicationId); |
| ConsumerSecretRequest consumerSecretRequest = ApplicationKeyMappingUtil. | ||
| fromDTOtoConsumerSecretRequest(clientId, consumerSecretDeletionRequestDTO); | ||
| apiConsumer.deleteConsumerSecret(consumerSecretDeletionRequestDTO.getSecretId(), | ||
| applicationKeyDTO.getKeyManager(), consumerSecretRequest); |
There was a problem hiding this comment.
Log Improvement Suggestion No: 12
| ConsumerSecretRequest consumerSecretRequest = ApplicationKeyMappingUtil. | |
| fromDTOtoConsumerSecretRequest(clientId, consumerSecretDeletionRequestDTO); | |
| apiConsumer.deleteConsumerSecret(consumerSecretDeletionRequestDTO.getSecretId(), | |
| applicationKeyDTO.getKeyManager(), consumerSecretRequest); | |
| apiConsumer.deleteConsumerSecret(consumerSecretDeletionRequestDTO.getSecretId(), | |
| applicationKeyDTO.getKeyManager(), consumerSecretRequest); | |
| log.info("Consumer secret revoked for application: {} and key mapping: {}", applicationId, keyMappingId); | |
| return Response.noContent().build(); |
| public static ConsumerSecretRequest fromDTOtoConsumerSecretRequest(String clientId, | ||
| ConsumerSecretCreationRequestDTO consumerSecretCreationRequestDTO) { |
There was a problem hiding this comment.
Log Improvement Suggestion No: 13
| public static ConsumerSecretRequest fromDTOtoConsumerSecretRequest(String clientId, | |
| ConsumerSecretCreationRequestDTO consumerSecretCreationRequestDTO) { | |
| public static ConsumerSecretRequest fromDTOtoConsumerSecretRequest(String clientId, | |
| ConsumerSecretCreationRequestDTO consumerSecretCreationRequestDTO) { | |
| log.debug("Creating consumer secret request for clientId: {}", clientId); |
| return consumerSecretDTO; | ||
| } | ||
|
|
||
| public static ConsumerSecretListDTO fromConsumerSecretListToDTO(List<ConsumerSecretInfo> consumerSecrets) { |
There was a problem hiding this comment.
Log Improvement Suggestion No: 14
| public static ConsumerSecretListDTO fromConsumerSecretListToDTO(List<ConsumerSecretInfo> consumerSecrets) { | |
| public static ConsumerSecretListDTO fromConsumerSecretListToDTO(List<ConsumerSecretInfo> consumerSecrets) { | |
| if (log.isDebugEnabled()) { | |
| log.debug("Converting consumer secrets list to DTO. Count: {}", consumerSecrets != null ? consumerSecrets.size() : 0); | |
| } |
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: 13
🤖 Fix all issues with AI Agents
In
@components/apimgt/org.wso2.carbon.apimgt.api/src/main/java/org/wso2/carbon/apimgt/api/APIConsumer.java:
- Around line 642-645: The default APIConsumer.retrieveConsumerSecrets method
throws an UnsupportedOperationException with an incorrect message; update the
exception text in the retrieveConsumerSecrets default implementation to reflect
that retrieving consumer secrets is not supported (e.g., "Retrieving consumer
secrets is not supported") instead of the current "Generating new consumer
secret is not supported", ensuring the message clearly references retrieval and
the method name retrieveConsumerSecrets for clarity.
In
@components/apimgt/org.wso2.carbon.apimgt.impl/src/main/java/org/wso2/carbon/apimgt/impl/AMDefaultKeyManagerImpl.java:
- Line 69: Remove the unused import
org.wso2.carbon.identity.oauth.config.OAuthServerConfiguration from the
AMDefaultKeyManagerImpl class: open the AMDefaultKeyManagerImpl.java file and
delete the import statement so only referenced imports remain; then recompile or
run the project's import/static-analysis checks to ensure no other unused
imports remain.
- Around line 685-692: The loop assumes clientSecretList.getList() is non-null
which can cause an NPE; in AMDefaultKeyManagerImpl before iterating check if
clientSecretList.getList() == null and return an empty List<ConsumerSecretInfo>
(or Collections.emptyList()) or otherwise guard the for-loop, then proceed to
map each ClientSecret to getConsumerSecretInfo(clientSecret, true); ensure you
reference clientSecretList.getList() and getConsumerSecretInfo when making the
change.
- Around line 704-708: In AMDefaultKeyManagerImpl inside the catch block
catching KeyManagerClientException for the delete consumer secret flow, update
the exception code passed to the APIManagementException from
ExceptionCodes.CLIENT_SECRET_RETRIEVAL_FAILED to
ExceptionCodes.CLIENT_SECRET_DELETION_FAILED so the thrown
APIManagementException accurately reflects a deletion failure for clientId (keep
the existing errMsg and chained exception).
In
@components/apimgt/org.wso2.carbon.apimgt.rest.api.common/src/main/resources/devportal-api.yaml:
- Around line 3232-3377: The revoke-secret endpoint and its curl example are
inconsistent: update the x-code-samples for operationId revokeConsumerSecret to
use -X POST against
/applications/{applicationId}/oauth-keys/{keyMappingId}/revoke-secret and show a
minimal JSON request body containing the secretId as defined by the
ConsumerSecretDeletionRequest; then either remove the unused
components.parameters entry named secretId (if you don't want a path param) or
add a new path variant that includes /secrets/{secretId} and wire that path to
the appropriate operation to keep the spec consistent.
- Around line 7590-7599: Update the Forbidden response example in the devportal
API spec so the example HTTP status code matches the response: change the
example object's "code" value from 401 to 403 in the response whose description
is "Forbidden. The user do not have permission to perform this action" (the
response referencing '#/components/schemas/Error'); keep the rest of the example
(message: "Forbidden", description, moreInfo) unchanged to maintain consistency.
In
@components/apimgt/org.wso2.carbon.apimgt.rest.api.store.v1/src/main/java/org/wso2/carbon/apimgt/rest/api/store/v1/impl/ApplicationsApiServiceImpl.java:
- Around line 1312-1336: In revokeConsumerSecret, when
getApplicationKeyByAppIDAndKeyMapping(applicationId, keyMappingId) returns null
you currently return null; change this to return a proper 404 response like the
other methods: detect applicationKeyDTO == null and return an HTTP 404 Not Found
response (with a clear message such as "Application key mapping not found" or
similar) instead of null so callers receive a proper error status.
- Around line 1287-1310: In getConsumerSecrets, when
getApplicationKeyByAppIDAndKeyMapping(applicationId, keyMappingId) returns null
you must return a 404 error response (not null) using the same pattern applied
in generateConsumerSecret: construct and return a Response with HTTP 404 and a
descriptive error message indicating the applicationId/keyMappingId were not
found (or throw the same APIManagementException used in generateConsumerSecret
that results in a 404); update the null branch in getConsumerSecrets accordingly
to mirror that error response behavior.
- Around line 1259-1285: The method generateConsumerSecret currently returns
null when applicationKeyDTO is null, leaving callers without a 404; change the
null branch to return an explicit 404 Response (e.g.,
Response.status(Response.Status.NOT_FOUND).entity(a descriptive error DTO or
message that includes keyMappingId) .build()) or throw an APIManagementException
with an appropriate NOT_FOUND ExceptionCodes value; update the block checking
applicationKeyDTO (variable applicationKeyDTO in generateConsumerSecret) to
produce that explicit error response instead of returning null.
In
@components/apimgt/org.wso2.carbon.apimgt.rest.api.store.v1/src/main/resources/devportal-api.yaml:
- Around line 3232-3288: The cURL code sample for operationId
generateConsumerSecret is missing the required JSON request body (schema
ConsumerSecretCreationRequest); update the x-code-samples Curl entry to include
a minimal request payload (either inline with -d
'{"expiresIn":3600,"description":"regen secret"}' or reference a file with -d
@data.json) and ensure Content-Type remains application/json so the example
matches the API contract.
- Around line 7589-7599: The OpenAPI reusable response "Forbidden" has an
example payload with "code: 401" which is inconsistent with the 403 status;
update the example under the Forbidden response to use "code: 403" (and keep
"message: Forbidden") so the example matches the documented 403 HTTP status;
locate the "Forbidden" response block in the YAML (the node named Forbidden with
example showing code: 401) and change the numeric code value to 403.
- Around line 3335-3377: The cURL example for operationId revokeConsumerSecret
is wrong: update the x-code-samples entry to call the POST path
/applications/{applicationId}/oauth-keys/{keyMappingId}/revoke-secret (not
DELETE /.../secrets/{secretId}), set -X POST, include the Content-Type:
application/json header and send the JSON body (e.g. -d @data.json) that
conforms to the ConsumerSecretDeletionRequest schema (data.json should contain {
"secretId": "sec_123456" } or equivalent); ensure the example uses the same path
parameters placeholders as the operation definition and includes the
Authorization header.
🧹 Nitpick comments (15)
components/apimgt/org.wso2.carbon.apimgt.impl/src/main/java/org/wso2/carbon/apimgt/impl/kmclient/model/ClientSecretList.java (1)
1-24: LGTM!This is a straightforward DTO implementation following standard Java POJO conventions. The class is appropriate for its use case as a KM client model for returning client secret collections.
Optional: Consider returning an empty list instead of null from
getList()to prevent potentialNullPointerExceptionin callers:🔎 Optional null-safe getter
public List<ClientSecret> getList() { - return list; + return list != null ? list : Collections.emptyList(); }You would also need to add the import:
import java.util.Collections;components/apimgt/org.wso2.carbon.apimgt.impl/src/main/java/org/wso2/carbon/apimgt/impl/kmclient/model/ClientSecretRequest.java (2)
5-9: Consider adding validation constraints and JavaDoc.The model fields lack validation, which could allow invalid values:
expiresIncould be negative or zerodescriptioncould be excessively longWhile validation may occur downstream, adding constraints here would provide fail-fast behavior and clearer contracts.
Additionally, JavaDoc would clarify field semantics (e.g., units for
expiresIn, whether fields are optional).💡 Example: Adding validation
+import javax.validation.constraints.Positive; +import javax.validation.constraints.Size; + +/** + * Request model for generating a new client secret. + */ public class ClientSecretRequest { + /** + * Expiration time in seconds. Must be positive if specified. + */ @SerializedName("expires_in") + @Positive private Integer expiresIn; + + /** + * Optional description for the client secret. + */ @SerializedName("description") + @Size(max = 255) private String description;
11-25: Optional: Add standard Object methods for better usability.Consider implementing
equals(),hashCode(), andtoString()to improve debugging, testing, and collection operations. These are standard best practices for model classes.💡 Example implementation
public void setDescription(String description) { this.description = description; } + + @Override + public boolean equals(Object o) { + if (this == o) return true; + if (o == null || getClass() != o.getClass()) return false; + ClientSecretRequest that = (ClientSecretRequest) o; + return Objects.equals(expiresIn, that.expiresIn) && + Objects.equals(description, that.description); + } + + @Override + public int hashCode() { + return Objects.hash(expiresIn, description); + } + + @Override + public String toString() { + return "ClientSecretRequest{" + + "expiresIn=" + expiresIn + + ", description='" + description + '\'' + + '}'; + } }Alternatively, consider using Lombok's
@Dataannotation to generate these methods automatically.components/apimgt/org.wso2.carbon.apimgt.impl/src/main/java/org/wso2/carbon/apimgt/impl/utils/APIUtil.java (2)
4892-4917: Clarify masking behavior when multiple client secrets are enabled
getEffectiveSecretmasks only whenisMultipleClientSecretsEnabled()is true, butmaskSecretitself only fully masks when client-secret hashing is enabled; otherwise it reveals up to 3 leading characters and pads to 16. That means in multi‑secret mode with hashing disabled, callers will still see a prefix of each secret.If the intent is to completely hide secrets whenever multi‑secret support is enabled (independent of hashing), consider either:
- Making
maskSecretaware of the multi‑secret flag and always returning a fully masked value in that mode; or- Changing
getEffectiveSecretto bypass the “show 3 chars” path in multi‑secret mode (e.g., a separatefullyMaskSecrethelper).Otherwise, a brief Javadoc note on both methods explaining the intended interplay between multi‑secret and hashing would avoid future misuse.
6093-6134: Consider caching SSLContext and improving error loggingFunctionally this is fine and has a safe fallback to the default
SSLContext, but two refinements would help:
getSSLContextreads and loads the trust store on every HTTPSgetHttpClientcall, which can be expensive under load. Consider caching the constructedSSLContextin a static/volatile field and rebuilding only when configuration changes (or on failure).- The catch blocks log only
e.getMessage(). Usinglog.error("...", e)would preserve stack traces and greatly simplify debugging misconfigurations.Neither affects correctness, but they are worthwhile operational improvements.
components/apimgt/org.wso2.carbon.apimgt.api/src/main/java/org/wso2/carbon/apimgt/api/model/KeyManager.java (1)
104-117: Clarify default behavior for new consumer-secret operationsThe new default methods extend the KeyManager API cleanly, but a couple of details could be tightened:
- All three default implementations throw
UnsupportedOperationExceptionwith the same “Generating new consumer secret is not supported” message, even for retrieval and deletion, which can be confusing at call sites.- Callers that currently only expect
APIManagementExceptionwill now also see uncheckedUnsupportedOperationExceptionif a KeyManager implementation isn’t upgraded.Consider either:
- Throwing an
APIManagementExceptioncarrying the “operation not supported” semantics, or- At minimum, using operation-specific messages (e.g., “Retrieving client secrets is not supported”) and ensuring all KeyManager implementations that should support multi-secret flows override these defaults.
components/apimgt/org.wso2.carbon.apimgt.api/src/main/java/org/wso2/carbon/apimgt/api/model/ConsumerSecretRequest.java (1)
23-46: Consider adding input validation and a getter for the parameters map.While the current implementation is functional, consider these optional improvements:
- Add validation: The
clientIdand parameter keys/values are not validated. Consider adding null checks if required.- Add
getParameters()method: Currently there's no direct way to retrieve the entire parameters map, only individual parameters. This limits flexibility for callers who may need to iterate or inspect all parameters.- Consider defensive copying: The
putAllmethod directly modifies the internal map. If immutability or encapsulation is a concern, consider defensive copying.Example: Adding getParameters() method
public void putAll(Map<String,Object> parameters){ this.parameters.putAll(parameters); } + +public Map<String, Object> getParameters() { + return parameters; +}components/apimgt/org.wso2.carbon.apimgt.api/src/main/java/org/wso2/carbon/apimgt/api/model/ConsumerSecretInfo.java (1)
44-46: Consider returning an unmodifiable view of the parameters map.The
getParameters()method returns a direct reference to the internalHashMap, allowing external callers to modify the map's contents. Consider returning an unmodifiable view to prevent unintended modifications.Example: Return unmodifiable map
+import java.util.Collections; + public Map<String, Object> getParameters() { - return parameters; + return Collections.unmodifiableMap(parameters); }components/apimgt/org.wso2.carbon.apimgt.impl/src/main/java/org/wso2/carbon/apimgt/impl/APIConsumerImpl.java (2)
69-70: Unify Key Manager resolution & validation for consumer secret operationsThe new consumer secret methods follow the same name/UUID resolution pattern as
renewConsumerSecret, but there are a few consistency and robustness gaps:
- None of
generateConsumerSecret,retrieveConsumerSecrets, ordeleteConsumerSecretvalidate that the resolvedKeyManagerConfigurationDTOis enabled, nor thatKeyManagerHolder.getKeyManagerInstance(...)actually returns a non‑null instance. Other flows (e.g.,renewAccessToken) explicitly checkisEnabled()and throwKEY_MANAGER_INITIALIZATION_FAILEDwhen KM initialization fails, instead of risking an NPE.- When resolving via UUID (
getKeyManagerConfigurationByUUID), theKeyManagerConfigurationDTOcarries its own organization/tenant; here you still callKeyManagerHolder.getKeyManagerInstance(tenantDomain, keyManagerName). In cross‑org KM setups, you may need to usekeyManagerConfigurationDTO.getOrganization()instead, similar toupdateAuthClient.deleteConsumerSecretassumesconsumerSecretRequestis non‑null. If any caller ever wants to delete a secret without extra properties, this will throw; either enforce non‑null at the API boundary or guard here.I recommend extracting a small helper that (a) resolves by name/UUID, (b) checks
isEnabled(), (c) picks the correct KM tenant/org, and (d) validates theKeyManagerinstance, then using it fromrenewConsumerSecret,generateConsumerSecret,retrieveConsumerSecrets, anddeleteConsumerSecretto keep behavior consistent and avoid subtle KM‑specific bugs.Example of aligning KM resolution & null checks (illustrative)
- KeyManagerConfigurationDTO keyManagerConfigurationDTO = - apiMgtDAO.getKeyManagerConfigurationByName(tenantDomain, keyManagerName); + KeyManagerConfigurationDTO keyManagerConfigurationDTO = + apiMgtDAO.getKeyManagerConfigurationByName(tenantDomain, keyManagerName); if (keyManagerConfigurationDTO == null) { keyManagerConfigurationDTO = apiMgtDAO.getKeyManagerConfigurationByUUID(keyManagerName); if (keyManagerConfigurationDTO != null) { keyManagerName = keyManagerConfigurationDTO.getName(); } else { log.error("Key Manager: " + keyManagerName + " not found in database."); throw new APIManagementException("Key Manager " + keyManagerName + " not found in database.", ExceptionCodes.KEY_MANAGER_NOT_FOUND); } } - - KeyManager keyManager = KeyManagerHolder.getKeyManagerInstance(tenantDomain, keyManagerName); + if (!keyManagerConfigurationDTO.isEnabled()) { + throw new APIManagementException("Key Manager " + keyManagerName + " not enabled", + ExceptionCodes.KEY_MANAGER_NOT_ENABLED); + } + + String kmTenant = keyManagerConfigurationDTO.getOrganization(); + KeyManager keyManager = KeyManagerHolder.getKeyManagerInstance(kmTenant, keyManagerName); + if (keyManager == null) { + throw new APIManagementException("Key Manager " + keyManagerName + " not initialized", + ExceptionCodes.KEY_MANAGER_INITIALIZATION_FAILED); + }You can then reuse the same pattern in all three consumer‑secret methods (and optionally in
renewConsumerSecret) to centralize KM handling.Also applies to: 328-346, 348-365, 367-385
4165-4176: Verify organization vs organizationId usage andisMoresemantics in searchPaginatedAPIsThe refactor to a shared private
searchPaginatedAPIs(...)helper looks good for reducing duplication, but there are a couple of behavioral points worth double‑checking:
- In the
OrganizationInfooverload you passnew Organization(organizationInfo.getSuperOrganization())into persistence, while usingorganizationInfo.getOrganizationId()asorganizationIDforAPIUtil.updateAvailableTiersByOrganization(...). That asymmetry may be intentional for super‑org vs child‑org behavior, but it’s subtle—please confirm this is the expected mapping for multi‑org setups.result.put("isMore", true)is set wheneversearchAPIs != null, regardless ofstart/endvssearchAPIs.getTotalAPIsCount(). If clients rely onisMoreto know whether another page exists, you might want to compute it based onstart + apiList.size() < totalCount. If it’s effectively a legacy “hasResults” flag, the current behavior is fine but a short comment would help clarify.Also applies to: 4178-4191, 4193-4249
components/apimgt/org.wso2.carbon.apimgt.rest.api.store.v1/src/main/java/org/wso2/carbon/apimgt/rest/api/store/v1/mappings/ApplicationKeyMappingUtil.java (1)
28-35: Clarify secret field propagation and consider masking/defensive handling in ConsumerSecret mappingsThe new helpers are generally clean and symmetric, but a few details are worth tightening:
- Both
fromDTOtoConsumerSecretRequest(...)overloads only passgetAdditionalProperties()intobuildConsumerSecretRequest. If the DTOs define any first‑class fields (e.g., areferenceIdproperty introduced by this feature), those need to be explicitly added into theConsumerSecretRequest(viaaddParameter/putAll) or also be conveyed insideadditionalProperties. Otherwise, KM‑side logic depending on them will never see the values. Please verify the DTO definitions and update if any such fields exist.buildConsumerSecretRequestblindly copies all entries fromadditionalPropertiesinto the internal request. If there are reserved keys (e.g., anything that should not be client‑controlled), you may want a small allow‑list/deny‑list filter to avoid clients overwriting internal parameters carried inConsumerSecretRequest.fromConsumerSecretToDTOmapsclientSecretdirectly intosecretValue. For create flows this is expected, but for “list all secrets” flows returning multiple active secrets, product requirements often call for masking or omitting the actual secret value. Since you already have separate list and single‑item conversions, consider:
- returning the raw
clientSecretonly from the create/retrieve‑single endpoint, and- either masking or nulling it in the list DTOs, or ensuring
ConsumerSecretInfo.clientSecretis pre‑masked before it gets here.fromConsumerSecretListToDTOassumesconsumerSecretsis non‑null. If any caller passesnull, this will NPE; guarding with an empty list (Collections.emptyList()) or an early return would make the mapper more robust.Illustrative tweaks for robustness & optional masking
- public static ConsumerSecretRequest fromDTOtoConsumerSecretRequest(String clientId, - ConsumerSecretCreationRequestDTO consumerSecretCreationRequestDTO) { - return buildConsumerSecretRequest(clientId, consumerSecretCreationRequestDTO.getAdditionalProperties()); - } + public static ConsumerSecretRequest fromDTOtoConsumerSecretRequest(String clientId, + ConsumerSecretCreationRequestDTO dto) { + ConsumerSecretRequest req = buildConsumerSecretRequest(clientId, dto.getAdditionalProperties()); + // Example: preserve first-class fields if they exist on the DTO + // if (dto.getReferenceId() != null) { + // req.addParameter("referenceId", dto.getReferenceId()); + // } + return req; + } @@ - public static ConsumerSecretListDTO fromConsumerSecretListToDTO(List<ConsumerSecretInfo> consumerSecrets) { + public static ConsumerSecretListDTO fromConsumerSecretListToDTO(List<ConsumerSecretInfo> consumerSecrets) { ConsumerSecretListDTO consumerSecretListDTO = new ConsumerSecretListDTO(); - List<ConsumerSecretDTO> consumerSecretDTOs = new ArrayList<>(); - for (ConsumerSecretInfo consumerSecret : consumerSecrets) { - consumerSecretDTOs.add(fromConsumerSecretToDTO(consumerSecret)); - } + List<ConsumerSecretDTO> consumerSecretDTOs = new ArrayList<>(); + if (consumerSecrets != null) { + for (ConsumerSecretInfo consumerSecret : consumerSecrets) { + consumerSecretDTOs.add(fromConsumerSecretToDTO(consumerSecret)); + } + } consumerSecretListDTO.setCount(consumerSecretDTOs.size()); consumerSecretListDTO.setList(consumerSecretDTOs); return consumerSecretListDTO; }If the API contract requires masked values in list responses, you could additionally adjust
fromConsumerSecretToDTO(or introduce a separate “list view” mapper) to avoid returning the rawclientSecretthere.Also applies to: 148-185
components/apimgt/org.wso2.carbon.apimgt.rest.api.store.v1/src/main/resources/devportal-api.yaml (2)
7430-7499: Clarify ConsumerSecret schemas (masking semantics, list typing, and additionalProperties)A couple of minor polish points around the new secret types:
ConsumerSecret.secretValueis described as “The actual secret string”, but the PR also mentions improved masking. If values are masked (which is safer), consider clarifying the description (“The secret value, which may be masked depending on configuration”) and/or marking itreadOnly: trueto signal it’s only ever returned, not supplied.ConsumerSecret.additionalPropertiesand the request/deletionadditionalPropertiesfields are defined asadditionalProperties: { type: object }, but the examples are simple scalars (expiresAt,expiresIn,description). If you only expect scalar metadata, you could narrow this totype: string/numberfor cleaner client models; otherwise, the currentobjectmap is fine but may generate very loose types.ConsumerSecretListis missing an explicittype: object, whereas other*Listschemas (e.g.,APIList,ApplicationKeyList) declare it. Addingtype: objectwould keep the spec consistent and avoids edge cases with validators.These are all non-breaking but will make the OpenAPI friendlier for client generation and documentation.
7730-7737: Remove or wire up unusedsecretIdpath parameter
secretIdis declared as a path parameter, but no path template in this spec uses{secretId}(the revoke flow uses a POST body instead). Keeping an unused parameter incomponents.parameterscan confuse consumers and tooling.Either:
- Remove
secretIdfromcomponents.parametersfor now, or- Introduce a corresponding
/.../secrets/{secretId}operation that uses it, and align it withConsumerSecretDeletionRequestif that’s the intended design.components/apimgt/org.wso2.carbon.apimgt.impl/src/main/java/org/wso2/carbon/apimgt/impl/AMDefaultKeyManagerImpl.java (1)
829-833: Clarify the masking strategy difference.The code uses
APIUtil.getEffectiveSecret()here, which conditionally masks based on configuration (multiple client secrets enabled), whilegetConsumerSecretInfousesAPIUtil.maskSecret()which always masks. This appears intentional for backward compatibility, but consider adding a brief comment explaining this distinction to prevent future confusion.🔎 Suggested documentation
String secret = appResponse.getClientSecret(); if (shouldMaskSecret) { + // Use getEffectiveSecret for backward compatibility - only masks when multiple + // client secrets feature is enabled, unlike maskSecret which always masks secret = APIUtil.getEffectiveSecret(secret); } oAuthApplicationInfo.setClientSecret(secret);components/apimgt/org.wso2.carbon.apimgt.rest.api.common/src/main/resources/devportal-api.yaml (1)
7430-7500: Tighten ConsumerSecret schemas and document secret sensitivity*The new secret-related schemas are a good fit for the new endpoints, but a couple of minor schema tweaks would improve consistency and tooling behavior:
ConsumerSecretListis missingtype: object, whereas all other*Listmodels here declare it explicitly. Adding it keeps generators and validators consistent.ConsumerSecret.secretValuerepresents a credential. It would be safer and clearer to:
- Mark it as
writeOnly: trueif it should only appear on creation, or- Explicitly document whether GET
/secretsreturns the raw value, a masked value, or omits it, so generated clients and UIs don’t accidentally log plaintext secrets.Optional example diff:
Suggested schema tweaks
ConsumerSecret: type: object properties: @@ secretValue: type: string - description: The actual secret string + description: The secret string returned on creation. Should not be logged or stored in plaintext by clients. + writeOnly: true @@ ConsumerSecretList: - title: Client Secret List - properties: + title: Client Secret List + type: object + properties:
.../apimgt/org.wso2.carbon.apimgt.api/src/main/java/org/wso2/carbon/apimgt/api/APIConsumer.java
Show resolved
Hide resolved
| public String getClientSecret() { | ||
| return clientSecret; | ||
| } | ||
|
|
||
| public void setClientSecret(String clientSecret) { | ||
| this.clientSecret = clientSecret; | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# Find the full ConsumerSecretInfo class to understand its structure
find . -type f -name "ConsumerSecretInfo.java" | head -5Repository: wso2/carbon-apimgt
Length of output: 179
🏁 Script executed:
# Search for all usages of ConsumerSecretInfo in the codebase
rg -l "ConsumerSecretInfo" --type=java | head -20Repository: wso2/carbon-apimgt
Length of output: 919
🏁 Script executed:
# Search for masking utilities and functions related to secrets
rg -nP "mask|Mask" --type=java -i -g "*Util*.java" -g "*API*.java" | head -40Repository: wso2/carbon-apimgt
Length of output: 9458
🏁 Script executed:
# Look for DTO conversion and mapping code
rg -l "ConsumerSecret" --type=java | grep -i "dto\|mapper\|mapping\|convert" | head -20Repository: wso2/carbon-apimgt
Length of output: 1787
🏁 Script executed:
# Check ApplicationKeyMappingUtil.java for ConsumerSecretInfo handling
cat components/apimgt/org.wso2.carbon.apimgt.rest.api.store.v1/src/main/java/org/wso2/carbon/apimgt/rest/api/store/v1/mappings/ApplicationKeyMappingUtil.java | head -100Repository: wso2/carbon-apimgt
Length of output: 5131
🏁 Script executed:
# Check if there's specific masking for clientSecret in ApplicationKeyMappingUtil
rg -n "clientSecret|ConsumerSecretInfo" components/apimgt/org.wso2.carbon.apimgt.rest.api.store.v1/src/main/java/org/wso2/carbon/apimgt/rest/api/store/v1/mappings/ApplicationKeyMappingUtil.java -A 10 -B 2Repository: wso2/carbon-apimgt
Length of output: 1740
🏁 Script executed:
# Check the ConsumerSecretDTO structure
cat components/apimgt/org.wso2.carbon.apimgt.rest.api.store.v1/src/gen/java/org/wso2/carbon/apimgt/rest/api/store/v1/dto/ConsumerSecretDTO.java | head -150Repository: wso2/carbon-apimgt
Length of output: 3741
🏁 Script executed:
# Check ApplicationsApiServiceImpl for consumer secret endpoints
rg -n "consumersecret|ConsumerSecret" components/apimgt/org.wso2.carbon.apimgt.rest.api.store.v1/src/main/java/org/wso2/carbon/apimgt/rest/api/store/v1/impl/ApplicationsApiServiceImpl.java -i -A 5 -B 2 | head -80Repository: wso2/carbon-apimgt
Length of output: 5275
🏁 Script executed:
# Check ApplicationsApiServiceImpl for the generateConsumerSecret endpoint
rg -n "generateConsumerSecret" components/apimgt/org.wso2.carbon.apimgt.rest.api.store.v1/src/main/java/org/wso2/carbon/apimgt/rest/api/store/v1/impl/ApplicationsApiServiceImpl.java -A 30 | head -50Repository: wso2/carbon-apimgt
Length of output: 3355
🏁 Script executed:
# Check if there's a response filter or interceptor that masks ConsumerSecretDTO
find . -type f -name "*.java" -path "*/rest/api/store/*" | xargs rg -l "Filter|Interceptor|ConsumerSecretDTO" | head -10Repository: wso2/carbon-apimgt
Length of output: 1233
🏁 Script executed:
# Check for any masking happening in ConsumerSecretDTO or related classes
rg -n "mask|Mask|hide|Hide" components/apimgt/org.wso2.carbon.apimgt.rest.api.store.v1/src/gen/java/org/wso2/carbon/apimgt/rest/api/store/v1/dto/ConsumerSecretDTO.java -iRepository: wso2/carbon-apimgt
Length of output: 44
🏁 Script executed:
# Look for ConsumerSecret endpoint implementations and response handling
rg -n "ConsumerSecret|revokeConsumerSecret\|listConsumerSecrets" components/apimgt/org.wso2.carbon.apimgt.rest.api.store.v1/src/main/java/org/wso2/carbon/apimgt/rest/api/store/v1/impl/ApplicationsApiServiceImpl.java -A 20 | head -100Repository: wso2/carbon-apimgt
Length of output: 6388
🏁 Script executed:
# Check the full fromConsumerSecretToDTO method and surrounding context
cat -n components/apimgt/org.wso2.carbon.apimgt.rest.api.store.v1/src/main/java/org/wso2/carbon/apimgt/rest/api/store/v1/mappings/ApplicationKeyMappingUtil.java | sed -n '160,190p'Repository: wso2/carbon-apimgt
Length of output: 1577
🏁 Script executed:
# Check if there are any REST response filters or interceptors
find . -type f -name "*.java" -path "*/rest/api/*" | xargs rg -l "ResponseFilter|ContainerResponseFilter|@Provider" | grep -E "store|common" | head -5Repository: wso2/carbon-apimgt
Length of output: 44
🏁 Script executed:
# Check ApplicationsApi (generated interface) for response handling
rg -n "generateConsumerSecret|getConsumerSecrets" components/apimgt/org.wso2.carbon.apimgt.rest.api.store.v1/src/gen/java/org/wso2/carbon/apimgt/rest/api/store/v1/ApplicationsApi.java -A 15 | head -60Repository: wso2/carbon-apimgt
Length of output: 3244
Mask clientSecret in API responses before serialization.
The clientSecret field is exposed unmasked in REST API responses via ConsumerSecretDTO. Update ApplicationKeyMappingUtil.fromConsumerSecretToDTO() (line 171) to mask the secret value:
- Return masked value for GET operations (e.g.,
getConsumerSecrets()) - Only expose plain secret in initial generation response (
generateConsumerSecret())
Ensure all serialization paths through ConsumerSecretDTO, ConsumerSecretListDTO, and related endpoints mask secrets appropriately before JSON serialization.
...o2.carbon.apimgt.impl/src/main/java/org/wso2/carbon/apimgt/impl/AMDefaultKeyManagerImpl.java
Outdated
Show resolved
Hide resolved
...o2.carbon.apimgt.impl/src/main/java/org/wso2/carbon/apimgt/impl/AMDefaultKeyManagerImpl.java
Show resolved
Hide resolved
...o2.carbon.apimgt.impl/src/main/java/org/wso2/carbon/apimgt/impl/AMDefaultKeyManagerImpl.java
Show resolved
Hide resolved
.../src/main/java/org/wso2/carbon/apimgt/rest/api/store/v1/impl/ApplicationsApiServiceImpl.java
Show resolved
Hide resolved
.../src/main/java/org/wso2/carbon/apimgt/rest/api/store/v1/impl/ApplicationsApiServiceImpl.java
Show resolved
Hide resolved
...onents/apimgt/org.wso2.carbon.apimgt.rest.api.store.v1/src/main/resources/devportal-api.yaml
Outdated
Show resolved
Hide resolved
| post: | ||
| tags: | ||
| - Application Secrets | ||
| summary: | | ||
| Revoke a Consumer Secret | ||
| description: | | ||
| This operation can be used to revoke a consumer secret for an application for the give key type | ||
| operationId: revokeConsumerSecret | ||
| parameters: | ||
| - $ref: '#/components/parameters/applicationId' | ||
| - $ref: '#/components/parameters/keyMappingId' | ||
| requestBody: | ||
| description: | | ||
| Request payload containing details for revoking a new consumer secret | ||
| content: | ||
| application/json: | ||
| schema: | ||
| $ref: '#/components/schemas/ConsumerSecretDeletionRequest' | ||
| required: true | ||
| responses: | ||
| 204: | ||
| description: | | ||
| OK. | ||
| Consumer secret deleted. | ||
| content: { } | ||
| 400: | ||
| $ref: '#/components/responses/BadRequest' | ||
| 401: | ||
| $ref: '#/components/responses/Unauthorized' | ||
| 403: | ||
| $ref: '#/components/responses/Forbidden' | ||
| 404: | ||
| $ref: '#/components/responses/NotFound' | ||
| 500: | ||
| $ref: '#/components/responses/InternalServerError' | ||
| security: | ||
| - OAuth2Security: | ||
| - apim:subscribe | ||
| - apim:app_manage | ||
| x-code-samples: | ||
| - lang: Curl | ||
| source: 'curl -k -H "Authorization: Bearer ae4eae22-3f65-387b-a171-d37eaa366fa8" | ||
| -H "Content-Type: application/json" -X DELETE "https://localhost:9443/api/am/devportal/v3/applications/16cd2684-9657-4a01-a956-4efd89e96077/oauth-keys/df972173-c957-46d4-96ac-99be8e303584/secrets/4efd2684-9657-4a01-a956-16cd89e96077"' |
There was a problem hiding this comment.
Fix revoke-secret cURL example (method and URL don’t match the defined operation)
The revoke-secret endpoint is defined as POST /applications/{applicationId}/oauth-keys/{keyMappingId}/revoke-secret with a JSON body (ConsumerSecretDeletionRequest), but the cURL example:
- Uses
-X DELETE - Calls
/.../secrets/{secretId}instead of/revoke-secret - Omits the required JSON request body
This will cause consumers to call a non-existent or wrong endpoint.
Proposed example fix
- source: 'curl -k -H "Authorization: Bearer ae4eae22-3f65-387b-a171-d37eaa366fa8"
- -H "Content-Type: application/json" -X DELETE "https://localhost:9443/api/am/devportal/v3/applications/16cd2684-9657-4a01-a956-4efd89e96077/oauth-keys/df972173-c957-46d4-96ac-99be8e303584/secrets/4efd2684-9657-4a01-a956-16cd89e96077"'
+ source: 'curl -k -H "Authorization: Bearer ae4eae22-3f65-387b-a171-d37eaa366fa8" \
+ -H "Content-Type: application/json" -X POST \
+ -d @data.json \
+ "https://localhost:9443/api/am/devportal/v3/applications/16cd2684-9657-4a01-a956-4efd89e96077/oauth-keys/df972173-c957-46d4-96ac-99be8e303584/revoke-secret"'And data.json should contain a ConsumerSecretDeletionRequest (e.g. { "secretId": "sec_123456" } plus any additional properties if needed).
🧰 Tools
🪛 Gitleaks (8.30.0)
[high] 3376-3376: Discovered a potential authorization token provided in a curl command header, which could compromise the curl accessed resource.
(curl-auth-header)
🤖 Prompt for AI Agents
In
@components/apimgt/org.wso2.carbon.apimgt.rest.api.store.v1/src/main/resources/devportal-api.yaml
around lines 3335-3377, The cURL example for operationId revokeConsumerSecret is
wrong: update the x-code-samples entry to call the POST path
/applications/{applicationId}/oauth-keys/{keyMappingId}/revoke-secret (not
DELETE /.../secrets/{secretId}), set -X POST, include the Content-Type:
application/json header and send the JSON body (e.g. -d @data.json) that
conforms to the ConsumerSecretDeletionRequest schema (data.json should contain {
"secretId": "sec_123456" } or equivalent); ensure the example uses the same path
parameters placeholders as the operation definition and includes the
Authorization header.
...onents/apimgt/org.wso2.carbon.apimgt.rest.api.store.v1/src/main/resources/devportal-api.yaml
Show resolved
Hide resolved
| example: | ||
| code: 401 | ||
| message: Forbidden | ||
| description: The user do not have permission to perform this action |
There was a problem hiding this comment.
The error message uses inconsistent grammar. It should be "The user does not have permission" instead of "The user do not have permission". The subject-verb agreement is incorrect.
| summary: | | ||
| Retrieve Consumer Secrets | ||
| description: | | ||
| This operation can be used to retrieve consumer secrets of an application for the give key type |
There was a problem hiding this comment.
There is a typo in the word "give". It should be "given" in the context "for the given key type".
| This operation can be used to retrieve consumer secrets of an application for the give key type | |
| This operation can be used to retrieve consumer secrets of an application for the given key type |
| summary: | | ||
| Generate a New Consumer Secret | ||
| description: | | ||
| This operation can be used to generate a new consumer secret for an application for the give key type |
There was a problem hiding this comment.
There is a typo in the word "give". It should be "given" in the context "for the given key type".
| This operation can be used to generate a new consumer secret for an application for the give key type | |
| This operation can be used to generate a new consumer secret for an application for the given key type |
| example: | ||
| code: 401 | ||
| message: Forbidden | ||
| description: The user do not have permission to perform this action |
There was a problem hiding this comment.
The error message uses inconsistent grammar. It should be "The user does not have permission" instead of "The user do not have permission". The subject-verb agreement is incorrect.
| String clientId = applicationKeyDTO.getConsumerKey(); | ||
| ConsumerSecretRequest consumerSecretRequest = ApplicationKeyMappingUtil. | ||
| fromDTOtoConsumerSecretRequest(clientId, consumerSecretDeletionRequestDTO); | ||
| apiConsumer.deleteConsumerSecret(consumerSecretDeletionRequestDTO.getSecretId(), |
There was a problem hiding this comment.
The secretId is extracted from the DTO twice - once directly from consumerSecretDeletionRequestDTO.getSecretId() and then the DTO is converted to ConsumerSecretRequest which only copies additionalProperties. This is inconsistent. The secretId should either be part of the ConsumerSecretRequest or handled separately, but the current approach is confusing as the ConsumerSecretRequest doesn't actually contain the secretId being passed separately.
| apiConsumer.deleteConsumerSecret(consumerSecretDeletionRequestDTO.getSecretId(), | |
| apiConsumer.deleteConsumerSecret(consumerSecretRequest.getSecretId(), |
|
|
||
| public static ConsumerSecretRequest fromDTOtoConsumerSecretRequest(String clientId, | ||
| ConsumerSecretDeletionRequestDTO consumerSecretDeletionRequestDTO) { | ||
| return buildConsumerSecretRequest(clientId, consumerSecretDeletionRequestDTO.getAdditionalProperties()); |
There was a problem hiding this comment.
The method parameter name is inconsistent. The first overload uses "ConsumerSecretCreationRequestDTO" while the second uses "ConsumerSecretDeletionRequestDTO", but both build the same ConsumerSecretRequest type. Consider using a more generic parameter name or ensuring these methods properly handle the secretId field from the deletion request.
| return buildConsumerSecretRequest(clientId, consumerSecretDeletionRequestDTO.getAdditionalProperties()); | |
| Map<String, Object> additionalProperties = consumerSecretDeletionRequestDTO.getAdditionalProperties(); | |
| if (additionalProperties == null) { | |
| additionalProperties = new HashMap<>(); | |
| } | |
| additionalProperties.put("secretId", consumerSecretDeletionRequestDTO.getSecretId()); | |
| return buildConsumerSecretRequest(clientId, additionalProperties); |
| schema: | ||
| $ref: '#/components/schemas/Error' | ||
| example: | ||
| code: 401 |
There was a problem hiding this comment.
The error response code is set to 401, but this is a Forbidden response which should use HTTP status code 403. The code and message are mismatched.
| code: 401 | |
| code: 403 |
| moreInfo: "" | ||
| error: [] | ||
| Forbidden: | ||
| description: Forbidden. The user do not have permission to perform this action. |
There was a problem hiding this comment.
The error message uses inconsistent grammar. It should be "The user does not have permission" instead of "The user do not have permission". The subject-verb agreement is incorrect.
| moreInfo: "" | ||
| error: [] | ||
| Forbidden: | ||
| description: Forbidden. The user do not have permission to perform this action. |
There was a problem hiding this comment.
The error message uses inconsistent grammar. It should be "The user does not have permission" instead of "The user do not have permission". The subject-verb agreement is incorrect.
| } catch (KeyManagerClientException e) { | ||
| String errMsg = "Error while deleting consumer secret of clientId : " + clientId; | ||
| throw new APIManagementException(errMsg, e, ExceptionCodes | ||
| .from(ExceptionCodes.CLIENT_SECRET_RETRIEVAL_FAILED, clientId)); |
There was a problem hiding this comment.
The exception code CLIENT_SECRET_RETRIEVAL_FAILED is being used for a deletion operation. This should use CLIENT_SECRET_DELETION_FAILED instead, which is already defined in the ExceptionCodes enum.
| .from(ExceptionCodes.CLIENT_SECRET_RETRIEVAL_FAILED, clientId)); | |
| .from(ExceptionCodes.CLIENT_SECRET_DELETION_FAILED, clientId)); |
There was a problem hiding this comment.
Actionable comments posted: 7
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
components/apimgt/org.wso2.carbon.apimgt.impl/src/main/java/org/wso2/carbon/apimgt/impl/AMDefaultKeyManagerImpl.java (2)
90-97: Duplicate import ofArrayList.
java.util.ArrayListis imported twice on lines 90 and 97. Remove one of the duplicate imports to avoid compilation warnings.import java.util.Arrays; import java.util.ArrayList; import java.util.Base64; import java.util.HashMap; import java.util.HashSet; import java.util.Map; import java.util.Set; import java.util.List; -import java.util.ArrayList;
878-887: Javadoc parameter mismatch.The Javadoc documents a
@param shouldMaskSecretparameter (line 883), but the method signature doesn't include this parameter. Either remove the outdated documentation or update the method signature if the parameter was intended./** * Builds an OAuthApplicationInfo object using the ClientInfo response * * `@param` appResponse ClientInfo response object * `@param` oAuthApplicationInfo original OAuthApplicationInfo object - * `@param` shouldMaskSecret whether to mask the client secret in the response * `@return` OAuthApplicationInfo object with response information added */
🤖 Fix all issues with AI agents
In
`@components/apimgt/org.wso2.carbon.apimgt.impl/src/main/java/org/wso2/carbon/apimgt/impl/AMDefaultKeyManagerImpl.java`:
- Around line 703-707: The expiresIn handling currently only accepts Integer
from consumerSecretRequest.getParameter(ApplicationConstants.SECRET_EXPIRES_IN),
which can miss valid numeric types; change the check to accept Number (e.g.,
instanceof Number) and convert to an int/Integer before calling
clientSecretRequest.setExpiresIn(...), mirroring the numeric handling used
elsewhere in this class (see other expiry handling around methods handling
Number conversions) so Long/Double JSON-deserialized values are handled
correctly.
In
`@components/apimgt/org.wso2.carbon.apimgt.impl/src/main/java/org/wso2/carbon/apimgt/impl/kmclient/model/ClientSecretRequest.java`:
- Around line 1-43: Change the ClientSecretRequest.expiresIn field from Integer
to Long to match other expiry types and avoid silent skips; update the private
field type, its getter getExpiresIn(), and setter setExpiresIn(Long expiresIn)
in class ClientSecretRequest, and ensure any deserialization/usage that expects
`@SerializedName`("expires_in") continues to work (this aligns with
TokenInfo.expiry and ClientInfo.clientSecretExpiresIn and avoids the instanceof
Integer check in AMDefaultKeyManagerImpl).
In
`@components/apimgt/org.wso2.carbon.apimgt.rest.api.common/src/main/resources/devportal-api.yaml`:
- Around line 7440-7510: The schemas ConsumerSecret,
ConsumerSecretCreationRequest, and ConsumerSecretDeletionRequest currently
declare their additionalProperties as "type: object" while the examples use
primitives (numbers/strings); update each schema's additionalProperties
definition so it allows any JSON value (e.g., replace the nested
"additionalProperties: { type: object }" with "additionalProperties: true" or
with an empty schema "{}"/no type) for the property named additionalProperties
in ConsumerSecret, ConsumerSecretCreationRequest, and
ConsumerSecretDeletionRequest so primitive examples like
expiresIn/expiresAt/description validate correctly.
- Around line 3232-3339: The GET
/applications/{applicationId}/oauth-keys/{keyMappingId}/secrets (operationId
getConsumerSecrets) exposes raw secrets by mapping
consumerSecret.getClientSecret() into the ConsumerSecret objects returned in
ConsumerSecretList; change the mapping so the GET response does not leak raw
secrets—either (A) mask the value (e.g., replace with fixed characters like
"****" or partial redaction) before populating the ConsumerSecret.secretValue
for the getConsumerSecrets response, or (B) use a separate response schema/DTO
for the list that omits secretValue entirely and map to that DTO instead; ensure
POST generateConsumerSecret still returns the unmasked secret if intended, and
update the OpenAPI schemas (ConsumerSecret / ConsumerSecretList) or mapping code
accordingly.
In
`@components/apimgt/org.wso2.carbon.apimgt.rest.api.store.v1/src/main/java/org/wso2/carbon/apimgt/rest/api/store/v1/mappings/ApplicationKeyMappingUtil.java`:
- Around line 205-211: The method fromConsumerSecretToDTO lacks a null guard for
its input; add an early null check at the start of
fromConsumerSecretToDTO(ConsumerSecretInfo consumerSecret) (e.g., if
consumerSecret == null) and decide consistent behavior (return null or throw
IllegalArgumentException) with the rest of the mapper codebase — if returning
null, simply return null immediately; otherwise throw a clear exception. Ensure
the rest of the method (creating ConsumerSecretDTO, calling secretValue,
setSecretId, setAdditionalProperties) only runs when consumerSecret is non-null.
- Around line 219-228: fromConsumerSecretListToDTO currently iterates
consumerSecrets without a null check which will NPE if null; add a defensive
null guard at the start of fromConsumerSecretListToDTO (e.g., if consumerSecrets
== null) to return an empty ConsumerSecretListDTO with count 0 and an empty list
(or initialize consumerSecretDTOs and proceed only if non-null), ensuring
consumerSecretListDTO.setList(...) and setCount(...) are always called with safe
values.
In
`@components/apimgt/org.wso2.carbon.apimgt.rest.api.store.v1/src/main/resources/devportal-api.yaml`:
- Around line 7440-7510: The additionalProperties entries in ConsumerSecret,
ConsumerSecretCreationRequest, and ConsumerSecretDeletionRequest are declared as
"type: object" but their examples use primitives; update each schema's
additionalProperties to allow arbitrary JSON values (e.g., set
additionalProperties to true or to a schema permitting any type) or replace the
free-form map with a dedicated schema that enumerates fields like
expiresIn/expiresAt/description; adjust ConsumerSecret.additionalProperties,
ConsumerSecretCreationRequest.additionalProperties, and
ConsumerSecretDeletionRequest.additionalProperties accordingly so examples
validate and codegen won't fail.
♻️ Duplicate comments (8)
components/apimgt/org.wso2.carbon.apimgt.rest.api.store.v1/src/main/resources/devportal-api.yaml (2)
3339-3388: FixrevokeConsumerSecretcURL example URL (currently points to a different endpoint).
The operation path is/revoke-secret, but the sample calls/secrets/{secretId}which will mislead users and generated SDK docs.Proposed fix
/applications/{applicationId}/oauth-keys/{keyMappingId}/revoke-secret: post: @@ x-code-samples: - lang: Curl source: | curl -k \ -H "Authorization: Bearer <ACCESS_TOKEN>" \ -H "Content-Type: application/json" \ -X POST \ -d '{"secretId":"sec_123456"}' \ - "https://localhost:9443/api/am/devportal/v3/applications/16cd2684-9657-4a01-a956-4efd89e96077/oauth-keys/df972173-c957-46d4-96ac-99be8e303584/secrets/4efd2684-9657-4a01-a956-16cd89e96077" + "https://localhost:9443/api/am/devportal/v3/applications/16cd2684-9657-4a01-a956-4efd89e96077/oauth-keys/df972173-c957-46d4-96ac-99be8e303584/revoke-secret"
3236-3240: Minor doc polish: “given key type” + Forbidden grammar.
There are a few typos/grammar issues in the newly added descriptions (“give” → “given”; “user do not” → “user does not”).Proposed fix
- This operation can be used to generate a new consumer secret for an application for the given key type + This operation can be used to generate a new consumer secret for an application for the given key type. - This operation can be used to retrieve consumer secrets of an application for the given key type + This operation can be used to retrieve consumer secrets of an application for the given key type. - This operation can be used to revoke a consumer secret for an application for the give key type + This operation can be used to revoke a consumer secret for an application for the given key type. - description: Forbidden. The user do not have permission to perform this action. + description: Forbidden. The user does not have permission to perform this action.Also applies to: 3299-3302, 3343-3347, 7599-7601
components/apimgt/org.wso2.carbon.apimgt.rest.api.common/src/main/resources/devportal-api.yaml (4)
3232-3388: Gitleaks: Bearer tokens in new x-code-samples likely to trip scannersNew curl examples include Authorization Bearer tokens (e.g., Lines 3288-3293, 3337-3338, 3382-3387) and are already flagged by gitleaks. If these are intentionally static doc tokens, consider adding an allowlist entry for these specific lines/patterns rather than changing the documentation style.
3232-3388: Fix revoke-secret curl example URL (still points to/secrets/{secretId})The
POST /applications/{applicationId}/oauth-keys/{keyMappingId}/revoke-secretoperation’s curl sample is still calling/secrets/{secretId}(a path that isn’t defined in this spec), which will mislead consumers.Proposed diff
/applications/{applicationId}/oauth-keys/{keyMappingId}/revoke-secret: @@ x-code-samples: - lang: Curl source: | curl -k \ -H "Authorization: Bearer ae4eae22-3f65-387b-a171-d37eaa366fa8" \ -H "Content-Type: application/json" \ -X POST \ -d '{"secretId":"sec_123456"}' \ - "https://localhost:9443/api/am/devportal/v3/applications/16cd2684-9657-4a01-a956-4efd89e96077/oauth-keys/df972173-c957-46d4-96ac-99be8e303584/secrets/4efd2684-9657-4a01-a956-16cd89e96077" + "https://localhost:9443/api/am/devportal/v3/applications/16cd2684-9657-4a01-a956-4efd89e96077/oauth-keys/df972173-c957-46d4-96ac-99be8e303584/revoke-secret"
3239-3347: Typos/wording in new secret endpoints (“give key type”, “revoking a new consumer secret”)A few descriptions read like copy/paste:
- “for the give[n] key type”
- “revoking a new consumer secret” (should be “an existing consumer secret”)
7599-7610: Forbidden response description grammar mismatch
Forbidden.descriptionsays “The user do not have permission…”, while the example uses “does not have permission…”. Please align grammar (“does not”).components/apimgt/org.wso2.carbon.apimgt.rest.api.store.v1/src/main/java/org/wso2/carbon/apimgt/rest/api/store/v1/mappings/ApplicationKeyMappingUtil.java (1)
177-180:secretIdfrom deletion DTO is not included in theConsumerSecretRequest.The
ConsumerSecretDeletionRequestDTOcontains asecretIdfield, but this mapping method only transfersadditionalProperties. The caller must separately extractsecretIdfrom the DTO, which is inconsistent with how creation requests work. Consider adding thesecretIdto the request's parameters for a more self-contained design.♻️ Suggested improvement
public static ConsumerSecretRequest fromDTOtoConsumerSecretRequest(String clientId, ConsumerSecretDeletionRequestDTO consumerSecretDeletionRequestDTO) { - return buildConsumerSecretRequest(clientId, consumerSecretDeletionRequestDTO.getAdditionalProperties()); + Map<String, Object> additionalProperties = consumerSecretDeletionRequestDTO.getAdditionalProperties(); + if (additionalProperties == null) { + additionalProperties = new HashMap<>(); + } + if (consumerSecretDeletionRequestDTO.getSecretId() != null) { + additionalProperties.put("secretId", consumerSecretDeletionRequestDTO.getSecretId()); + } + return buildConsumerSecretRequest(clientId, additionalProperties); }components/apimgt/org.wso2.carbon.apimgt.rest.api.store.v1/src/main/java/org/wso2/carbon/apimgt/rest/api/store/v1/impl/ApplicationsApiServiceImpl.java (1)
1333-1336: InconsistentsecretIdhandling between direct extraction and DTO conversion.The
secretIdis extracted directly from the DTO for the first parameter but the convertedConsumerSecretRequest(third parameter) doesn't contain it. This design requires the caller to understand thatsecretIdmust be passed separately, which is error-prone.This is related to the mapping utility issue where
fromDTOtoConsumerSecretRequestfor deletion doesn't include thesecretId.
🧹 Nitpick comments (3)
components/apimgt/org.wso2.carbon.apimgt.impl/src/main/java/org/wso2/carbon/apimgt/impl/kmclient/model/ClientSecretList.java (1)
1-41: Consider null-safe defaults forlistfield to reduce NPE risk in callers.The
getList()method currently returns null, requiring callers to null-check (as seen inAMDefaultKeyManagerImpl.retrieveApplicationConsumerSecrets()). Initializinglistto an empty list or returningCollections.emptyList()would align with patterns in the codebase (e.g.,OpenIdConnectConfigurationinitializes List fields withnew ArrayList<>()) and eliminate defensive null-checks in downstream code.Optional tweak
public class ClientSecretList { private int count; private List<ClientSecret> list; @@ public List<ClientSecret> getList() { - return list; + return list == null ? java.util.Collections.emptyList() : list; }components/apimgt/org.wso2.carbon.apimgt.impl/src/main/java/org/wso2/carbon/apimgt/impl/AMDefaultKeyManagerImpl.java (2)
447-450: Missing debug log level check.Other similar blocks in this method wrap debug logging with
if (log.isDebugEnabled())(e.g., lines 270-272, 298-300). This block logs directly without the guard, which could have a minor performance impact.} catch (NumberFormatException e) { // No need to throw as it's due to a non-numeric value. - log.debug("Invalid client secret expires-in value given for " + oauthClientName, e); + if (log.isDebugEnabled()) { + log.debug("Invalid client secret expires-in value given for " + oauthClientName, e); + } }
721-736: LGTM with a minor observation.The helper method is well-structured. One minor inconsistency:
descriptionis checked for null before adding (line 730), butclientSecretExpiresAtis added unconditionally (line 733). Consider applying the same null check for consistency, or document that null values are intentional for expiry.
| Object expiresInObj = | ||
| consumerSecretRequest.getParameter(ApplicationConstants.SECRET_EXPIRES_IN); | ||
| if (expiresInObj instanceof Integer) { | ||
| clientSecretRequest.setExpiresIn((Integer) expiresInObj); | ||
| } |
There was a problem hiding this comment.
Potential type mismatch for expiresIn parameter.
The code checks for instanceof Integer only, but when values are deserialized from JSON, they may come as Long or Double (depending on the JSON library). This could silently ignore valid numeric values. Consider handling Number type similar to other expiry time handling in this class (e.g., lines 260-266).
🔧 Suggested fix
Object expiresInObj =
consumerSecretRequest.getParameter(ApplicationConstants.SECRET_EXPIRES_IN);
- if (expiresInObj instanceof Integer) {
- clientSecretRequest.setExpiresIn((Integer) expiresInObj);
+ if (expiresInObj instanceof Number) {
+ clientSecretRequest.setExpiresIn(((Number) expiresInObj).intValue());
}📝 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.
| Object expiresInObj = | |
| consumerSecretRequest.getParameter(ApplicationConstants.SECRET_EXPIRES_IN); | |
| if (expiresInObj instanceof Integer) { | |
| clientSecretRequest.setExpiresIn((Integer) expiresInObj); | |
| } | |
| Object expiresInObj = | |
| consumerSecretRequest.getParameter(ApplicationConstants.SECRET_EXPIRES_IN); | |
| if (expiresInObj instanceof Number) { | |
| clientSecretRequest.setExpiresIn(((Number) expiresInObj).intValue()); | |
| } |
🤖 Prompt for AI Agents
In
`@components/apimgt/org.wso2.carbon.apimgt.impl/src/main/java/org/wso2/carbon/apimgt/impl/AMDefaultKeyManagerImpl.java`
around lines 703 - 707, The expiresIn handling currently only accepts Integer
from consumerSecretRequest.getParameter(ApplicationConstants.SECRET_EXPIRES_IN),
which can miss valid numeric types; change the check to accept Number (e.g.,
instanceof Number) and convert to an int/Integer before calling
clientSecretRequest.setExpiresIn(...), mirroring the numeric handling used
elsewhere in this class (see other expiry handling around methods handling
Number conversions) so Long/Double JSON-deserialized values are handled
correctly.
| /** | ||
| * Copyright (c) 2026, WSO2 LLC. (https://www.wso2.com). | ||
| * | ||
| * WSO2 LLC. licenses this file to you under the Apache License, | ||
| * Version 2.0 (the "License"); you may not use this file except | ||
| * in compliance with the License. | ||
| * You may obtain a copy of the License at | ||
| * | ||
| * http://www.apache.org/licenses/LICENSE-2.0 | ||
| * | ||
| * Unless required by applicable law or agreed to in writing, | ||
| * software distributed under the License is distributed on an | ||
| * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY | ||
| * KIND, either express or implied. See the License for the | ||
| * specific language governing permissions and limitations | ||
| * under the License. | ||
| */ | ||
| package org.wso2.carbon.apimgt.impl.kmclient.model; | ||
|
|
||
| import com.google.gson.annotations.SerializedName; | ||
|
|
||
| public class ClientSecretRequest { | ||
| @SerializedName("expires_in") | ||
| private Integer expiresIn; | ||
| @SerializedName("description") | ||
| private String description; | ||
|
|
||
| public Integer getExpiresIn() { | ||
| return expiresIn; | ||
| } | ||
|
|
||
| public void setExpiresIn(Integer expiresIn) { | ||
| this.expiresIn = expiresIn; | ||
| } | ||
|
|
||
| public String getDescription() { | ||
| return description; | ||
| } | ||
|
|
||
| public void setDescription(String description) { | ||
| this.description = description; | ||
| } | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Search for expires_in usage in the KM client code
echo "=== Searching for expires_in string references ==="
rg -n --no-heading -S '"expires_in"' components/apimgt/org.wso2.carbon.apimgt.impl/src/main/java/org/wso2/carbon/apimgt/impl
echo ""
echo "=== Searching for expiresIn field references ==="
rg -n --no-heading -S '\bexpiresIn\b' components/apimgt/org.wso2.carbon.apimgt.impl/src/main/java/org/wso2/carbon/apimgt/implRepository: wso2/carbon-apimgt
Length of output: 2314
🏁 Script executed:
#!/bin/bash
# Search more broadly for ClientSecretRequest usage and related classes
echo "=== Searching for ClientSecretRequest class usage ==="
rg -n 'ClientSecretRequest' components/apimgt/org.wso2.carbon.apimgt.impl/src/main/java/org/wso2/carbon/apimgt/impl
echo ""
echo "=== Searching for token expiry/expiration handling in KM code ==="
rg -n --no-heading -i 'expir' components/apimgt/org.wso2.carbon.apimgt.impl/src/main/java/org/wso2/carbon/apimgt/impl/kmclient | head -30Repository: wso2/carbon-apimgt
Length of output: 5993
🏁 Script executed:
#!/bin/bash
# Check if there are any Key Manager API specifications or documentation files
echo "=== Looking for KM contract/spec files ==="
fd -e 'md|txt|json' -i 'key.*manager\|contract' components/apimgt/org.wso2.carbon.apimgt.impl | head -20
echo ""
echo "=== Check kmclient directory structure ==="
fd -type f components/apimgt/org.wso2.carbon.apimgt.impl/src/main/java/org/wso2/carbon/apimgt/impl/kmclientRepository: wso2/carbon-apimgt
Length of output: 336
🏁 Script executed:
#!/bin/bash
# Check DCRClient usage of ClientSecretRequest
sed -n '50,70p' components/apimgt/org.wso2.carbon.apimgt.impl/src/main/java/org/wso2/carbon/apimgt/impl/kmclient/DCRClient.java
echo ""
echo "=== Check where DCRClient sends ClientSecretRequest ==="
rg -B5 -A10 'ClientSecretRequest clientSecretRequest' components/apimgt/org.wso2.carbon.apimgt.impl/src/main/java/org/wso2/carbon/apimgt/impl/kmclient/DCRClient.javaRepository: wso2/carbon-apimgt
Length of output: 442
🏁 Script executed:
#!/bin/bash
# Check how ClientSecretRequest is instantiated and used in AMDefaultKeyManagerImpl
echo "=== ClientSecretRequest instantiation and usage ==="
sed -n '690,720p' components/apimgt/org.wso2.carbon.apimgt.impl/src/main/java/org/wso2/carbon/apimgt/impl/AMDefaultKeyManagerImpl.java
echo ""
echo "=== Check if expiresIn is ever set on ClientSecretRequest ==="
rg -B3 -A3 'setExpiresIn' components/apimgt/org.wso2.carbon.apimgt.impl/src/main/java/org/wso2/carbon/apimgt/implRepository: wso2/carbon-apimgt
Length of output: 3856
🏁 Script executed:
#!/bin/bash
# Compare TokenInfo structure which also uses expires_in field
echo "=== TokenInfo.java to compare type handling ==="
cat -n components/apimgt/org.wso2.carbon.apimgt.impl/src/main/java/org/wso2/carbon/apimgt/impl/kmclient/model/TokenInfo.javaRepository: wso2/carbon-apimgt
Length of output: 2195
Change expiresIn field type from Integer to Long for consistency.
The field uses the same @SerializedName("expires_in") as TokenInfo.expiry (which is typed as long primitive) and aligns with other expiry-related fields in the model (ClientInfo.clientSecretExpiresIn, ClientSecret.clientSecretExpiresAt—both Long). Additionally, AMDefaultKeyManagerImpl line 709 enforces an instanceof Integer check that will skip setting the field if a Long value is passed, which is a potential silent failure. Using Long ensures consistency across the kmclient.model package and accommodates larger expiry values the Key Manager may provide.
🤖 Prompt for AI Agents
In
`@components/apimgt/org.wso2.carbon.apimgt.impl/src/main/java/org/wso2/carbon/apimgt/impl/kmclient/model/ClientSecretRequest.java`
around lines 1 - 43, Change the ClientSecretRequest.expiresIn field from Integer
to Long to match other expiry types and avoid silent skips; update the private
field type, its getter getExpiresIn(), and setter setExpiresIn(Long expiresIn)
in class ClientSecretRequest, and ensure any deserialization/usage that expects
`@SerializedName`("expires_in") continues to work (this aligns with
TokenInfo.expiry and ClientInfo.clientSecretExpiresIn and avoids the instanceof
Integer check in AMDefaultKeyManagerImpl).
| /applications/{applicationId}/oauth-keys/{keyMappingId}/generate-secret: | ||
| post: | ||
| tags: | ||
| - Application Secrets | ||
| summary: | | ||
| Generate a New Consumer Secret | ||
| description: | | ||
| This operation can be used to generate a new consumer secret for an application for the given key type | ||
| operationId: generateConsumerSecret | ||
| parameters: | ||
| - $ref: '#/components/parameters/applicationId' | ||
| - $ref: '#/components/parameters/keyMappingId' | ||
| requestBody: | ||
| description: | | ||
| Request payload containing details for creating a new consumer secret | ||
| content: | ||
| application/json: | ||
| schema: | ||
| $ref: '#/components/schemas/ConsumerSecretCreationRequest' | ||
| required: true | ||
| responses: | ||
| 201: | ||
| description: | | ||
| Created. | ||
| Successful response with the newly created object as entity in the body. | ||
| Location header contains URL of newly created entity. | ||
| headers: | ||
| Location: | ||
| description: | | ||
| Location to the newly created secret entity. | ||
| schema: | ||
| type: string | ||
| Content-Type: | ||
| description: | | ||
| The content type of the body. | ||
| schema: | ||
| type: string | ||
| content: | ||
| application/json: | ||
| schema: | ||
| $ref: '#/components/schemas/ConsumerSecret' | ||
| 400: | ||
| $ref: '#/components/responses/BadRequest' | ||
| 401: | ||
| $ref: '#/components/responses/Unauthorized' | ||
| 403: | ||
| $ref: '#/components/responses/Forbidden' | ||
| 500: | ||
| $ref: '#/components/responses/InternalServerError' | ||
| security: | ||
| - OAuth2Security: | ||
| - apim:subscribe | ||
| - apim:app_manage | ||
| x-code-samples: | ||
| - lang: Curl | ||
| source: | | ||
| curl -k \ | ||
| -H "Authorization: Bearer ae4eae22-3f65-387b-a171-d37eaa366fa8" \ | ||
| -H "Content-Type: application/json" \ | ||
| -X POST \ | ||
| -d '{"additionalProperties":{"description":"pizza application secret7","expiresIn":86400}}' \ | ||
| "https://localhost:9443/api/am/devportal/v3/applications/16cd2684-9657-4a01-a956-4efd89e96077/oauth-keys/df972173-c957-46d4-96ac-99be8e303584/generate-secret" | ||
| /applications/{applicationId}/oauth-keys/{keyMappingId}/secrets: | ||
| get: | ||
| tags: | ||
| - Application Secrets | ||
| summary: | | ||
| Retrieve Consumer Secrets | ||
| description: | | ||
| This operation can be used to retrieve consumer secrets of an application for the given key type | ||
| operationId: getConsumerSecrets | ||
| parameters: | ||
| - $ref: '#/components/parameters/applicationId' | ||
| - $ref: '#/components/parameters/keyMappingId' | ||
| responses: | ||
| 200: | ||
| description: | | ||
| OK. | ||
| Consumer secrets are retrieved. | ||
| headers: | ||
| Content-Type: | ||
| description: | | ||
| The content type of the body. | ||
| schema: | ||
| type: string | ||
| content: | ||
| application/json: | ||
| schema: | ||
| $ref: '#/components/schemas/ConsumerSecretList' | ||
| 400: | ||
| $ref: '#/components/responses/BadRequest' | ||
| 401: | ||
| $ref: '#/components/responses/Unauthorized' | ||
| 403: | ||
| $ref: '#/components/responses/Forbidden' | ||
| 404: | ||
| $ref: '#/components/responses/NotFound' | ||
| 500: | ||
| $ref: '#/components/responses/InternalServerError' | ||
| security: | ||
| - OAuth2Security: | ||
| - apim:subscribe | ||
| - apim:app_manage | ||
| x-code-samples: | ||
| - lang: Curl | ||
| source: 'curl -k -H "Authorization: Bearer ae4eae22-3f65-387b-a171-d37eaa366fa8" | ||
| -H "Content-Type: application/json" -X GET "https://localhost:9443/api/am/devportal/v3/applications/16cd2684-9657-4a01-a956-4efd89e96077/oauth-keys/df972173-c957-46d4-96ac-99be8e303584/secrets"' | ||
| /applications/{applicationId}/oauth-keys/{keyMappingId}/revoke-secret: |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, let's check the structure and find schema definitions
wc -l components/apimgt/org.wso2.carbon.apimgt.rest.api.common/src/main/resources/devportal-api.yamlRepository: wso2/carbon-apimgt
Length of output: 161
🏁 Script executed:
# Let's find and read the ConsumerSecret and ConsumerSecretList schema definitions
# First, check around line 7443-7510 to see what "Also applies to" refers to
sed -n '7440,7515p' components/apimgt/org.wso2.carbon.apimgt.rest.api.common/src/main/resources/devportal-api.yamlRepository: wso2/carbon-apimgt
Length of output: 2608
🏁 Script executed:
# Search for ConsumerSecret schema definitions
rg "ConsumerSecret(List)?:" components/apimgt/org.wso2.carbon.apimgt.rest.api.common/src/main/resources/devportal-api.yaml -A 20Repository: wso2/carbon-apimgt
Length of output: 1312
🏁 Script executed:
# Search for secretValue in the file to see how it's used
rg "secretValue" components/apimgt/org.wso2.carbon.apimgt.rest.api.common/src/main/resources/devportal-api.yaml -B 2 -A 2Repository: wso2/carbon-apimgt
Length of output: 239
🏁 Script executed:
# Look at the full POST endpoint response (generate-secret) around lines 3232-3270
sed -n '3232,3280p' components/apimgt/org.wso2.carbon.apimgt.rest.api.common/src/main/resources/devportal-api.yamlRepository: wso2/carbon-apimgt
Length of output: 1792
🏁 Script executed:
# Search for any masking or mask-related patterns in the codebase
rg -i "mask" components/apimgt/org.wso2.carbon.apimgt.rest.api.common/src/main/resources/devportal-api.yamlRepository: wso2/carbon-apimgt
Length of output: 75
🏁 Script executed:
# Check for any other secret schemas or variations that might be masked
rg "Secret" components/apimgt/org.wso2.carbon.apimgt.rest.api.common/src/main/resources/devportal-api.yaml | grep -i "schema\|type:"Repository: wso2/carbon-apimgt
Length of output: 387
🏁 Script executed:
# Look for implementation hints - check if there's any backend code or documentation mentioning masking
find components/apimgt -name "*.java" -type f | xargs rg -l "secret" | head -5Repository: wso2/carbon-apimgt
Length of output: 631
🏁 Script executed:
# Look for context around the "mask" reference
rg -i "mask" components/apimgt/org.wso2.carbon.apimgt.rest.api.common/src/main/resources/devportal-api.yaml -B 5 -A 5Repository: wso2/carbon-apimgt
Length of output: 304
🏁 Script executed:
# Check the PR/commit message or related files for hints about masking behavior
# Let's search for any OAuth key or secret related implementation
rg "getConsumerSecrets\|generateConsumerSecret" components/apimgt -lRepository: wso2/carbon-apimgt
Length of output: 44
🏁 Script executed:
# Look for implementation of secret masking in the REST API layer
rg -i "secretvalue.*mask\|mask.*secret" components/apimgt -B 2 -A 2Repository: wso2/carbon-apimgt
Length of output: 44
🏁 Script executed:
# Check if there are separate response schemas or transformation logic
rg "secretValue\|ConsumerSecret" components/apimgt/org.wso2.carbon.apimgt.rest.api.admin/src/main/java -l 2>/dev/null | head -3Repository: wso2/carbon-apimgt
Length of output: 44
🏁 Script executed:
# Search for the REST API implementation files
find components/apimgt -path "*rest/api*" -name "*.java" | grep -i "application\|secret" | head -10Repository: wso2/carbon-apimgt
Length of output: 1519
🏁 Script executed:
# Look for any ApplicationsApi or related controllers
find components/apimgt -name "*ApplicationsApi*.java" -o -name "*ApplicationResource*.java" | head -5Repository: wso2/carbon-apimgt
Length of output: 756
🏁 Script executed:
# Broader search for secret-related implementations
find components/apimgt -name "*.java" -exec grep -l "ConsumerSecret\|generateConsumerSecret\|getConsumerSecrets" {} \; | head -10Repository: wso2/carbon-apimgt
Length of output: 2425
🏁 Script executed:
# Check the devportal-api.yaml file around the lines to understand if both endpoints reuse the same schema
sed -n '3220,3340p' components/apimgt/org.wso2.carbon.apimgt.rest.api.common/src/main/resources/devportal-api.yaml | grep -A 30 "responses:"Repository: wso2/carbon-apimgt
Length of output: 1966
🏁 Script executed:
# Examine the ConsumerSecretInfo model to understand the backend implementation
cat components/apimgt/org.wso2.carbon.apimgt.api/src/main/java/org/wso2/carbon/apimgt/api/model/ConsumerSecretInfo.javaRepository: wso2/carbon-apimgt
Length of output: 1507
🏁 Script executed:
# Check the devportal REST API implementation
find components/apimgt -path "*rest/api*devportal*" -name "*.java" | grep -i impl | head -5Repository: wso2/carbon-apimgt
Length of output: 44
🏁 Script executed:
# Look for any DevPortal ApplicationResource or ApplicationsApi implementation
find components/apimgt -path "*rest/api*devportal*" -name "*ApplicationResource*" -o -path "*rest/api*devportal*" -name "*ApplicationsApi*" | head -5Repository: wso2/carbon-apimgt
Length of output: 44
🏁 Script executed:
# Search for the actual implementation of getConsumerSecrets or generateConsumerSecret
find components/apimgt -path "*devportal*" -name "*.java" -exec grep -l "getConsumerSecrets\|generateConsumerSecret" {} \; 2>/dev/nullRepository: wso2/carbon-apimgt
Length of output: 44
🏁 Script executed:
# Broader search for devportal REST API implementations
find components/apimgt -type d -name "*devportal*"Repository: wso2/carbon-apimgt
Length of output: 359
🏁 Script executed:
# Search for OAuth keys related implementation
find components/apimgt -path "*/rest/api*" -name "*.java" | xargs grep -l "oauth-keys\|oauth.keys" 2>/dev/null | head -5Repository: wso2/carbon-apimgt
Length of output: 195
🏁 Script executed:
# Look for the key mapping and secret generation logic
find components/apimgt -name "*.java" | xargs grep -l "generateConsumerSecret\|generate.*secret" 2>/dev/null | head -5Repository: wso2/carbon-apimgt
Length of output: 650
🏁 Script executed:
# Check PR summary or commit messages for context about masking
git log --oneline --all | head -20Repository: wso2/carbon-apimgt
Length of output: 194
🏁 Script executed:
# Look at the store.v1 ApplicationsApi implementation
find components/apimgt/org.wso2.carbon.apimgt.rest.api.store.v1 -name "*.java" -path "*/impl/*" | head -10Repository: wso2/carbon-apimgt
Length of output: 1546
🏁 Script executed:
# Check for any secret-related DTOs in store API
find components/apimgt/org.wso2.carbon.apimgt.rest.api.store.v1 -name "*Secret*" -o -name "*Oauth*"Repository: wso2/carbon-apimgt
Length of output: 655
🏁 Script executed:
# Let's search for ConsumerSecret in the generated classes
find components/apimgt -name "*ConsumerSecret*.java" 2>/dev/nullRepository: wso2/carbon-apimgt
Length of output: 890
🏁 Script executed:
# Look at the PR context - check if there's a commit message or description
git log -1 --format="%B" 2>/dev/null || echo "No detailed commit info"Repository: wso2/carbon-apimgt
Length of output: 82
🏁 Script executed:
# Search for the specific method in APIConsumer interface
rg "getConsumerSecrets\|generateConsumerSecret" components/apimgt/org.wso2.carbon.apimgt.api/src/main/java/org/wso2/carbon/apimgt/api/APIConsumer.javaRepository: wso2/carbon-apimgt
Length of output: 44
🏁 Script executed:
# Examine the ConsumerSecretDTO to see how it handles secretValue
cat components/apimgt/org.wso2.carbon.apimgt.rest.api.store.v1/src/gen/java/org/wso2/carbon/apimgt/rest/api/store/v1/dto/ConsumerSecretDTO.javaRepository: wso2/carbon-apimgt
Length of output: 3741
🏁 Script executed:
# Also check ConsumerSecretListDTO
cat components/apimgt/org.wso2.carbon.apimgt.rest.api.store.v1/src/gen/java/org/wso2/carbon/apimgt/rest/api/store/v1/dto/ConsumerSecretListDTO.javaRepository: wso2/carbon-apimgt
Length of output: 2688
🏁 Script executed:
# Look for the ApplicationsApiServiceImpl in store.v1 that implements the secret endpoints
find components/apimgt/org.wso2.carbon.apimgt.rest.api.store.v1 -name "*ApplicationsApiServiceImpl*"Repository: wso2/carbon-apimgt
Length of output: 212
🏁 Script executed:
# Search for actual implementation of the secret-related methods
find components/apimgt/org.wso2.carbon.apimgt.rest.api.store.v1 -name "*.java" -path "*/impl/*" | xargs grep -l "Secret" 2>/dev/null | head -5Repository: wso2/carbon-apimgt
Length of output: 212
🏁 Script executed:
# Examine the ApplicationsApiServiceImpl to see if there's any masking logic
cat components/apimgt/org.wso2.carbon.apimgt.rest.api.store.v1/src/main/java/org/wso2/carbon/apimgt/rest/api/store/v1/impl/ApplicationsApiServiceImpl.java | head -200Repository: wso2/carbon-apimgt
Length of output: 11192
🏁 Script executed:
# Search for the specific methods in ApplicationsApiServiceImpl
grep -n "generateConsumerSecret\|getConsumerSecrets" components/apimgt/org.wso2.carbon.apimgt.rest.api.store.v1/src/main/java/org/wso2/carbon/apimgt/rest/api/store/v1/impl/ApplicationsApiServiceImpl.javaRepository: wso2/carbon-apimgt
Length of output: 393
🏁 Script executed:
# Get the line count of the file first
wc -l components/apimgt/org.wso2.carbon.apimgt.rest.api.store.v1/src/main/java/org/wso2/carbon/apimgt/rest/api/store/v1/impl/ApplicationsApiServiceImpl.javaRepository: wso2/carbon-apimgt
Length of output: 217
🏁 Script executed:
# Read the generateConsumerSecret method
sed -n '1260,1290p' components/apimgt/org.wso2.carbon.apimgt.rest.api.store.v1/src/main/java/org/wso2/carbon/apimgt/rest/api/store/v1/impl/ApplicationsApiServiceImpl.javaRepository: wso2/carbon-apimgt
Length of output: 2031
🏁 Script executed:
# Read the getConsumerSecrets method
sed -n '1290,1340p' components/apimgt/org.wso2.carbon.apimgt.rest.api.store.v1/src/main/java/org/wso2/carbon/apimgt/rest/api/store/v1/impl/ApplicationsApiServiceImpl.javaRepository: wso2/carbon-apimgt
Length of output: 3180
🏁 Script executed:
# Let's see more context after getConsumerSecrets
sed -n '1290,1350p' components/apimgt/org.wso2.carbon.apimgt.rest.api.store.v1/src/main/java/org/wso2/carbon/apimgt/rest/api/store/v1/impl/ApplicationsApiServiceImpl.javaRepository: wso2/carbon-apimgt
Length of output: 3487
🏁 Script executed:
# Check the ApplicationKeyMappingUtil for the conversion logic
find components/apimgt/org.wso2.carbon.apimgt.rest.api.store.v1 -name "ApplicationKeyMappingUtil.java"Repository: wso2/carbon-apimgt
Length of output: 215
🏁 Script executed:
# Search for the fromConsumerSecretToDTO and fromConsumerSecretListToDTO methods
rg "fromConsumerSecretToDTO|fromConsumerSecretListToDTO" components/apimgt/org.wso2.carbon.apimgt.rest.api.store.v1 -lRepository: wso2/carbon-apimgt
Length of output: 366
🏁 Script executed:
# Look at the mapping utility file
find components/apimgt/org.wso2.carbon.apimgt.rest.api.store.v1 -path "*/mappings/*" -name "*.java" | head -10Repository: wso2/carbon-apimgt
Length of output: 1573
🏁 Script executed:
# Get the line count first
wc -l components/apimgt/org.wso2.carbon.apimgt.rest.api.store.v1/src/main/java/org/wso2/carbon/apimgt/rest/api/store/v1/mappings/ApplicationKeyMappingUtil.javaRepository: wso2/carbon-apimgt
Length of output: 219
🏁 Script executed:
# Search for the conversion methods
grep -n "fromConsumerSecretToDTO\|fromConsumerSecretListToDTO" components/apimgt/org.wso2.carbon.apimgt.rest.api.store.v1/src/main/java/org/wso2/carbon/apimgt/rest/api/store/v1/mappings/ApplicationKeyMappingUtil.javaRepository: wso2/carbon-apimgt
Length of output: 359
🏁 Script executed:
# Read the conversion methods
rg "fromConsumerSecretToDTO|fromConsumerSecretListToDTO" components/apimgt/org.wso2.carbon.apimgt.rest.api.store.v1/src/main/java/org/wso2/carbon/apimgt/rest/api/store/v1/mappings/ApplicationKeyMappingUtil.java -A 30Repository: wso2/carbon-apimgt
Length of output: 1289
Security concern: GET /secrets should mask secretValue or use separate response schema
The GET endpoint returns ConsumerSecretList containing ConsumerSecret items with unmasked secretValue (example: s3cr3tV@lu3). The current mapping logic directly exposes the raw secret from consumerSecret.getClientSecret() without any masking. For security best practices, consider either:
- Returning masked values in GET responses (e.g., example:
****) while keeping POST unmasked - Using a separate schema for list responses that omits
secretValueentirely - Or clarifying in the spec that secrets are intentionally unmasked in both POST and GET
Also applies to: 7443-7510
🧰 Tools
🪛 Gitleaks (8.30.0)
[high] 3288-3289: Discovered a potential authorization token provided in a curl command header, which could compromise the curl accessed resource.
(curl-auth-header)
[high] 3337-3337: Discovered a potential authorization token provided in a curl command header, which could compromise the curl accessed resource.
(curl-auth-header)
🤖 Prompt for AI Agents
In
`@components/apimgt/org.wso2.carbon.apimgt.rest.api.common/src/main/resources/devportal-api.yaml`
around lines 3232 - 3339, The GET
/applications/{applicationId}/oauth-keys/{keyMappingId}/secrets (operationId
getConsumerSecrets) exposes raw secrets by mapping
consumerSecret.getClientSecret() into the ConsumerSecret objects returned in
ConsumerSecretList; change the mapping so the GET response does not leak raw
secrets—either (A) mask the value (e.g., replace with fixed characters like
"****" or partial redaction) before populating the ConsumerSecret.secretValue
for the getConsumerSecrets response, or (B) use a separate response schema/DTO
for the list that omits secretValue entirely and map to that DTO instead; ensure
POST generateConsumerSecret still returns the unmasked secret if intended, and
update the OpenAPI schemas (ConsumerSecret / ConsumerSecretList) or mapping code
accordingly.
| #----------------------------------------------------- | ||
| # Consumer Secret Object | ||
| #----------------------------------------------------- | ||
| ConsumerSecret: | ||
| type: object | ||
| properties: | ||
| secretId: | ||
| type: string | ||
| description: Unique identifier for the secret | ||
| example: sec_123456 | ||
| secretValue: | ||
| type: string | ||
| description: The actual secret string | ||
| example: s3cr3tV@lu3 | ||
| additionalProperties: | ||
| type: object | ||
| description: Additional dynamic properties for the secret creation request. | ||
| additionalProperties: | ||
| type: object | ||
| example: | ||
| expiresAt: 1761568483853 | ||
| description: "pizza application secret" | ||
| #----------------------------------------------------- | ||
| # Consumer Secret Request Object | ||
| #----------------------------------------------------- | ||
| ConsumerSecretCreationRequest: | ||
| type: object | ||
| properties: | ||
| additionalProperties: | ||
| type: object | ||
| description: Additional properties for the secret creation request. | ||
| additionalProperties: | ||
| type: object | ||
| example: | ||
| expiresIn: 86400 | ||
| description: "pizza application secret" | ||
| example: | ||
| additionalProperties: | ||
| expiresIn: 86400 | ||
| description: "pizza application secret" | ||
| #----------------------------------------------------- | ||
| # Consumer Secret Deletion Object | ||
| #----------------------------------------------------- | ||
| ConsumerSecretDeletionRequest: | ||
| type: object | ||
| properties: | ||
| secretId: | ||
| type: string | ||
| description: Unique identifier for the secret | ||
| example: sec_123456 | ||
| additionalProperties: | ||
| type: object | ||
| description: Additional properties for the secret deletion request. | ||
| additionalProperties: | ||
| type: object | ||
| #----------------------------------------------------- | ||
| # Consumer Secret List Object | ||
| #----------------------------------------------------- | ||
| ConsumerSecretList: | ||
| title: Client Secret List | ||
| properties: | ||
| count: | ||
| type: integer | ||
| description: | | ||
| Number of consumer secrets returned. | ||
| example: 1 | ||
| list: | ||
| type: array | ||
| items: | ||
| $ref: '#/components/schemas/ConsumerSecret' | ||
|
|
There was a problem hiding this comment.
Schema mismatch: additionalProperties values typed as object but examples use primitives
ConsumerSecret.additionalProperties, ConsumerSecretCreationRequest.additionalProperties, and ConsumerSecretDeletionRequest.additionalProperties define additionalProperties: { type: object }, but the examples include integer/string values (e.g., expiresIn: 86400, expiresAt: 176..., description: "..."). This will break schema validation and many generators.
Proposed diff (allow any JSON types)
ConsumerSecret:
type: object
properties:
@@
additionalProperties:
type: object
description: Additional dynamic properties for the secret creation request.
- additionalProperties:
- type: object
+ additionalProperties: {}
@@
ConsumerSecretCreationRequest:
type: object
properties:
additionalProperties:
type: object
description: Additional properties for the secret creation request.
- additionalProperties:
- type: object
+ additionalProperties: {}
@@
ConsumerSecretDeletionRequest:
type: object
properties:
@@
additionalProperties:
type: object
description: Additional properties for the secret deletion request.
- additionalProperties:
- type: object
+ additionalProperties: {}🤖 Prompt for AI Agents
In
`@components/apimgt/org.wso2.carbon.apimgt.rest.api.common/src/main/resources/devportal-api.yaml`
around lines 7440 - 7510, The schemas ConsumerSecret,
ConsumerSecretCreationRequest, and ConsumerSecretDeletionRequest currently
declare their additionalProperties as "type: object" while the examples use
primitives (numbers/strings); update each schema's additionalProperties
definition so it allows any JSON value (e.g., replace the nested
"additionalProperties: { type: object }" with "additionalProperties: true" or
with an empty schema "{}"/no type) for the property named additionalProperties
in ConsumerSecret, ConsumerSecretCreationRequest, and
ConsumerSecretDeletionRequest so primitive examples like
expiresIn/expiresAt/description validate correctly.
| public static ConsumerSecretDTO fromConsumerSecretToDTO(ConsumerSecretInfo consumerSecret) { | ||
| ConsumerSecretDTO consumerSecretDTO = new ConsumerSecretDTO(); | ||
| consumerSecretDTO.setSecretId(consumerSecret.getSecretId()); | ||
| consumerSecretDTO.secretValue(consumerSecret.getClientSecret()); | ||
| consumerSecretDTO.setAdditionalProperties(consumerSecret.getParameters()); | ||
| return consumerSecretDTO; | ||
| } |
There was a problem hiding this comment.
Missing null check for input parameter.
If consumerSecret is null, this method will throw a NullPointerException. Consider adding a null guard at the start of the method.
🛡️ Suggested defensive check
public static ConsumerSecretDTO fromConsumerSecretToDTO(ConsumerSecretInfo consumerSecret) {
+ if (consumerSecret == null) {
+ return null;
+ }
ConsumerSecretDTO consumerSecretDTO = new ConsumerSecretDTO();
consumerSecretDTO.setSecretId(consumerSecret.getSecretId());
consumerSecretDTO.secretValue(consumerSecret.getClientSecret());
consumerSecretDTO.setAdditionalProperties(consumerSecret.getParameters());
return consumerSecretDTO;
}📝 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.
| public static ConsumerSecretDTO fromConsumerSecretToDTO(ConsumerSecretInfo consumerSecret) { | |
| ConsumerSecretDTO consumerSecretDTO = new ConsumerSecretDTO(); | |
| consumerSecretDTO.setSecretId(consumerSecret.getSecretId()); | |
| consumerSecretDTO.secretValue(consumerSecret.getClientSecret()); | |
| consumerSecretDTO.setAdditionalProperties(consumerSecret.getParameters()); | |
| return consumerSecretDTO; | |
| } | |
| public static ConsumerSecretDTO fromConsumerSecretToDTO(ConsumerSecretInfo consumerSecret) { | |
| if (consumerSecret == null) { | |
| return null; | |
| } | |
| ConsumerSecretDTO consumerSecretDTO = new ConsumerSecretDTO(); | |
| consumerSecretDTO.setSecretId(consumerSecret.getSecretId()); | |
| consumerSecretDTO.secretValue(consumerSecret.getClientSecret()); | |
| consumerSecretDTO.setAdditionalProperties(consumerSecret.getParameters()); | |
| return consumerSecretDTO; | |
| } |
🤖 Prompt for AI Agents
In
`@components/apimgt/org.wso2.carbon.apimgt.rest.api.store.v1/src/main/java/org/wso2/carbon/apimgt/rest/api/store/v1/mappings/ApplicationKeyMappingUtil.java`
around lines 205 - 211, The method fromConsumerSecretToDTO lacks a null guard
for its input; add an early null check at the start of
fromConsumerSecretToDTO(ConsumerSecretInfo consumerSecret) (e.g., if
consumerSecret == null) and decide consistent behavior (return null or throw
IllegalArgumentException) with the rest of the mapper codebase — if returning
null, simply return null immediately; otherwise throw a clear exception. Ensure
the rest of the method (creating ConsumerSecretDTO, calling secretValue,
setSecretId, setAdditionalProperties) only runs when consumerSecret is non-null.
| public static ConsumerSecretListDTO fromConsumerSecretListToDTO(List<ConsumerSecretInfo> consumerSecrets) { | ||
| ConsumerSecretListDTO consumerSecretListDTO = new ConsumerSecretListDTO(); | ||
| List<ConsumerSecretDTO> consumerSecretDTOs = new ArrayList<>(); | ||
| for (ConsumerSecretInfo consumerSecret : consumerSecrets) { | ||
| consumerSecretDTOs.add(fromConsumerSecretToDTO(consumerSecret)); | ||
| } | ||
| consumerSecretListDTO.setCount(consumerSecretDTOs.size()); | ||
| consumerSecretListDTO.setList(consumerSecretDTOs); | ||
| return consumerSecretListDTO; | ||
| } |
There was a problem hiding this comment.
Null input will cause NullPointerException.
If consumerSecrets is null, the for-each loop at line 222 will throw a NullPointerException. Add a null guard for defensive programming.
🛡️ Suggested fix
public static ConsumerSecretListDTO fromConsumerSecretListToDTO(List<ConsumerSecretInfo> consumerSecrets) {
ConsumerSecretListDTO consumerSecretListDTO = new ConsumerSecretListDTO();
+ if (consumerSecrets == null) {
+ consumerSecretListDTO.setCount(0);
+ consumerSecretListDTO.setList(new ArrayList<>());
+ return consumerSecretListDTO;
+ }
List<ConsumerSecretDTO> consumerSecretDTOs = new ArrayList<>();
for (ConsumerSecretInfo consumerSecret : consumerSecrets) {
consumerSecretDTOs.add(fromConsumerSecretToDTO(consumerSecret));
}
consumerSecretListDTO.setCount(consumerSecretDTOs.size());
consumerSecretListDTO.setList(consumerSecretDTOs);
return consumerSecretListDTO;
}📝 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.
| public static ConsumerSecretListDTO fromConsumerSecretListToDTO(List<ConsumerSecretInfo> consumerSecrets) { | |
| ConsumerSecretListDTO consumerSecretListDTO = new ConsumerSecretListDTO(); | |
| List<ConsumerSecretDTO> consumerSecretDTOs = new ArrayList<>(); | |
| for (ConsumerSecretInfo consumerSecret : consumerSecrets) { | |
| consumerSecretDTOs.add(fromConsumerSecretToDTO(consumerSecret)); | |
| } | |
| consumerSecretListDTO.setCount(consumerSecretDTOs.size()); | |
| consumerSecretListDTO.setList(consumerSecretDTOs); | |
| return consumerSecretListDTO; | |
| } | |
| public static ConsumerSecretListDTO fromConsumerSecretListToDTO(List<ConsumerSecretInfo> consumerSecrets) { | |
| ConsumerSecretListDTO consumerSecretListDTO = new ConsumerSecretListDTO(); | |
| if (consumerSecrets == null) { | |
| consumerSecretListDTO.setCount(0); | |
| consumerSecretListDTO.setList(new ArrayList<>()); | |
| return consumerSecretListDTO; | |
| } | |
| List<ConsumerSecretDTO> consumerSecretDTOs = new ArrayList<>(); | |
| for (ConsumerSecretInfo consumerSecret : consumerSecrets) { | |
| consumerSecretDTOs.add(fromConsumerSecretToDTO(consumerSecret)); | |
| } | |
| consumerSecretListDTO.setCount(consumerSecretDTOs.size()); | |
| consumerSecretListDTO.setList(consumerSecretDTOs); | |
| return consumerSecretListDTO; | |
| } |
🤖 Prompt for AI Agents
In
`@components/apimgt/org.wso2.carbon.apimgt.rest.api.store.v1/src/main/java/org/wso2/carbon/apimgt/rest/api/store/v1/mappings/ApplicationKeyMappingUtil.java`
around lines 219 - 228, fromConsumerSecretListToDTO currently iterates
consumerSecrets without a null check which will NPE if null; add a defensive
null guard at the start of fromConsumerSecretListToDTO (e.g., if consumerSecrets
== null) to return an empty ConsumerSecretListDTO with count 0 and an empty list
(or initialize consumerSecretDTOs and proceed only if non-null), ensuring
consumerSecretListDTO.setList(...) and setCount(...) are always called with safe
values.
| #----------------------------------------------------- | ||
| # Consumer Secret Object | ||
| #----------------------------------------------------- | ||
| ConsumerSecret: | ||
| type: object | ||
| properties: | ||
| secretId: | ||
| type: string | ||
| description: Unique identifier for the secret | ||
| example: sec_123456 | ||
| secretValue: | ||
| type: string | ||
| description: The actual secret string | ||
| example: s3cr3tV@lu3 | ||
| additionalProperties: | ||
| type: object | ||
| description: Additional dynamic properties for the secret creation request. | ||
| additionalProperties: | ||
| type: object | ||
| example: | ||
| expiresAt: 1761568483853 | ||
| description: "pizza application secret" | ||
| #----------------------------------------------------- | ||
| # Consumer Secret Request Object | ||
| #----------------------------------------------------- | ||
| ConsumerSecretCreationRequest: | ||
| type: object | ||
| properties: | ||
| additionalProperties: | ||
| type: object | ||
| description: Additional properties for the secret creation request. | ||
| additionalProperties: | ||
| type: object | ||
| example: | ||
| expiresIn: 86400 | ||
| description: "pizza application secret" | ||
| example: | ||
| additionalProperties: | ||
| expiresIn: 86400 | ||
| description: "pizza application secret" | ||
| #----------------------------------------------------- | ||
| # Consumer Secret Deletion Object | ||
| #----------------------------------------------------- | ||
| ConsumerSecretDeletionRequest: | ||
| type: object | ||
| properties: | ||
| secretId: | ||
| type: string | ||
| description: Unique identifier for the secret | ||
| example: sec_123456 | ||
| additionalProperties: | ||
| type: object | ||
| description: Additional properties for the secret deletion request. | ||
| additionalProperties: | ||
| type: object | ||
| #----------------------------------------------------- | ||
| # Consumer Secret List Object | ||
| #----------------------------------------------------- | ||
| ConsumerSecretList: | ||
| title: Client Secret List | ||
| properties: | ||
| count: | ||
| type: integer | ||
| description: | | ||
| Number of consumer secrets returned. | ||
| example: 1 | ||
| list: | ||
| type: array | ||
| items: | ||
| $ref: '#/components/schemas/ConsumerSecret' | ||
|
|
There was a problem hiding this comment.
Fix additionalProperties schema typing (examples use primitives but schema restricts values to objects).
Right now, the schema says “map of objects”, but examples include expiresIn: 86400 and description: "..." (non-object values). This mismatch can break validation/codegen.
One option: allow arbitrary JSON values for the map
ConsumerSecret:
type: object
properties:
@@
additionalProperties:
type: object
description: Additional dynamic properties for the secret creation request.
- additionalProperties:
- type: object
+ additionalProperties: true
@@
ConsumerSecretCreationRequest:
type: object
properties:
additionalProperties:
type: object
description: Additional properties for the secret creation request.
- additionalProperties:
- type: object
+ additionalProperties: true
@@
ConsumerSecretDeletionRequest:
type: object
properties:
@@
additionalProperties:
type: object
description: Additional properties for the secret deletion request.
- additionalProperties:
- type: object
+ additionalProperties: true(Alternative, if you want stricter typing: introduce a dedicated schema with explicit fields like expiresIn, expiresAt, description instead of a free-form map.)
📝 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.
| #----------------------------------------------------- | |
| # Consumer Secret Object | |
| #----------------------------------------------------- | |
| ConsumerSecret: | |
| type: object | |
| properties: | |
| secretId: | |
| type: string | |
| description: Unique identifier for the secret | |
| example: sec_123456 | |
| secretValue: | |
| type: string | |
| description: The actual secret string | |
| example: s3cr3tV@lu3 | |
| additionalProperties: | |
| type: object | |
| description: Additional dynamic properties for the secret creation request. | |
| additionalProperties: | |
| type: object | |
| example: | |
| expiresAt: 1761568483853 | |
| description: "pizza application secret" | |
| #----------------------------------------------------- | |
| # Consumer Secret Request Object | |
| #----------------------------------------------------- | |
| ConsumerSecretCreationRequest: | |
| type: object | |
| properties: | |
| additionalProperties: | |
| type: object | |
| description: Additional properties for the secret creation request. | |
| additionalProperties: | |
| type: object | |
| example: | |
| expiresIn: 86400 | |
| description: "pizza application secret" | |
| example: | |
| additionalProperties: | |
| expiresIn: 86400 | |
| description: "pizza application secret" | |
| #----------------------------------------------------- | |
| # Consumer Secret Deletion Object | |
| #----------------------------------------------------- | |
| ConsumerSecretDeletionRequest: | |
| type: object | |
| properties: | |
| secretId: | |
| type: string | |
| description: Unique identifier for the secret | |
| example: sec_123456 | |
| additionalProperties: | |
| type: object | |
| description: Additional properties for the secret deletion request. | |
| additionalProperties: | |
| type: object | |
| #----------------------------------------------------- | |
| # Consumer Secret List Object | |
| #----------------------------------------------------- | |
| ConsumerSecretList: | |
| title: Client Secret List | |
| properties: | |
| count: | |
| type: integer | |
| description: | | |
| Number of consumer secrets returned. | |
| example: 1 | |
| list: | |
| type: array | |
| items: | |
| $ref: '#/components/schemas/ConsumerSecret' | |
| `#-----------------------------------------------------` | |
| # Consumer Secret Object | |
| `#-----------------------------------------------------` | |
| ConsumerSecret: | |
| type: object | |
| properties: | |
| secretId: | |
| type: string | |
| description: Unique identifier for the secret | |
| example: sec_123456 | |
| secretValue: | |
| type: string | |
| description: The actual secret string | |
| example: s3cr3tV@lu3 | |
| additionalProperties: | |
| type: object | |
| description: Additional dynamic properties for the secret creation request. | |
| additionalProperties: true | |
| example: | |
| expiresAt: 1761568483853 | |
| description: "pizza application secret" | |
| `#-----------------------------------------------------` | |
| # Consumer Secret Request Object | |
| `#-----------------------------------------------------` | |
| ConsumerSecretCreationRequest: | |
| type: object | |
| properties: | |
| additionalProperties: | |
| type: object | |
| description: Additional properties for the secret creation request. | |
| additionalProperties: true | |
| example: | |
| expiresIn: 86400 | |
| description: "pizza application secret" | |
| example: | |
| additionalProperties: | |
| expiresIn: 86400 | |
| description: "pizza application secret" | |
| `#-----------------------------------------------------` | |
| # Consumer Secret Deletion Object | |
| `#-----------------------------------------------------` | |
| ConsumerSecretDeletionRequest: | |
| type: object | |
| properties: | |
| secretId: | |
| type: string | |
| description: Unique identifier for the secret | |
| example: sec_123456 | |
| additionalProperties: | |
| type: object | |
| description: Additional properties for the secret deletion request. | |
| additionalProperties: true | |
| `#-----------------------------------------------------` | |
| # Consumer Secret List Object | |
| `#-----------------------------------------------------` | |
| ConsumerSecretList: | |
| title: Client Secret List | |
| properties: | |
| count: | |
| type: integer | |
| description: | | |
| Number of consumer secrets returned. | |
| example: 1 | |
| list: | |
| type: array | |
| items: | |
| $ref: '#/components/schemas/ConsumerSecret' |
🤖 Prompt for AI Agents
In
`@components/apimgt/org.wso2.carbon.apimgt.rest.api.store.v1/src/main/resources/devportal-api.yaml`
around lines 7440 - 7510, The additionalProperties entries in ConsumerSecret,
ConsumerSecretCreationRequest, and ConsumerSecretDeletionRequest are declared as
"type: object" but their examples use primitives; update each schema's
additionalProperties to allow arbitrary JSON values (e.g., set
additionalProperties to true or to a schema permitting any type) or replace the
free-form map with a dedicated schema that enumerates fields like
expiresIn/expiresAt/description; adjust ConsumerSecret.additionalProperties,
ConsumerSecretCreationRequest.additionalProperties, and
ConsumerSecretDeletionRequest.additionalProperties accordingly so examples
validate and codegen won't fail.
1 participant
Purpose