Conversation
|
Naduni Pamudika seems not to be a GitHub user. You need a GitHub account to be able to sign the CLA. If you have already a GitHub account, please add the email address used for this commit to your account. You have signed the CLA already but the status is still pending? Let us recheck it. |
|
Important Review skippedDraft detected. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the Use the checkbox below for a quick retry:
📝 WalkthroughWalkthroughThis pull request introduces opaque API key management functionality across the API Gateway Manager system. It adds new interfaces, data models, authentication flows, event publishing, database operations, and REST API endpoints to support generating, revoking, regenerating, and tracking opaque API keys with display names and metadata. Changes
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🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
1 similar comment
✅ Actions performedReview triggered.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
Note
Due to the large number of review comments, Critical severity comments were prioritized as inline comments.
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.gateway/src/main/java/org/wso2/carbon/apimgt/gateway/utils/GatewayUtils.java (1)
921-921:⚠️ Potential issue | 🔴 CriticalUnsafe cast to
Long— may throwClassCastExceptionfor small numeric values.
application.get(APPLICATION_ID)returns anObjectfrom the JSON parser, which could beIntegerorLongdepending on the magnitude of the value. Casting directly toLongwill fail with aClassCastExceptionwhen the value is parsed asInteger. The other overload at line 820 avoids this by usingInteger.parseInt(application.getAsString(...)), which is safer.🐛 Proposed fix — use consistent and safe parsing
- appId = ((Long) application.get(APIConstants.JwtTokenConstants.APPLICATION_ID)).intValue(); + appId = Integer.parseInt(application.getAsString(APIConstants.JwtTokenConstants.APPLICATION_ID));components/apimgt/org.wso2.carbon.apimgt.impl/src/main/java/org/wso2/carbon/apimgt/impl/utils/APIKeyUtils.java (1)
29-29:⚠️ Potential issue | 🟡 MinorBug: Logger initialized with wrong class.
LogFactory.getLog(JWTUtil.class)should beLogFactory.getLog(APIKeyUtils.class). This causes all log messages from this class (including the new method's error logging at line 64) to be attributed toJWTUtilinstead ofAPIKeyUtils.Proposed fix
- private static final Log log = LogFactory.getLog(JWTUtil.class); + private static final Log log = LogFactory.getLog(APIKeyUtils.class);
🤖 Fix all issues with AI agents
In
`@components/apimgt/org.wso2.carbon.apimgt.gateway/src/main/java/org/wso2/carbon/apimgt/gateway/handlers/security/apikey/ApiKeyAuthenticator.java`:
- Around line 237-238: The hardcoded lookupSecret in ApiKeyAuthenticator (the
String lookupSecret and its use in APIUtil.generateLookupKey(apiKey,
lookupSecret)) must be removed and loaded from external configuration or a
vault; change the logic to fetch the secret from APIManagerConfiguration (or a
secure vault provider) at startup or when needed, validate it is present (log
and fail the authentication flow if missing) and pass that configured secret
into APIUtil.generateLookupKey instead of the literal; ensure any
caching/rotation is supported (do not store the secret in source code or
constants) and add clear error handling/logging when the configured secret is
unavailable.
In
`@components/apimgt/org.wso2.carbon.apimgt.gateway/src/main/java/org/wso2/carbon/apimgt/gateway/internal/DataHolder.java`:
- Around line 157-179: The removal method removeOpaqueAPIKeyInfo currently uses
a different identifier (apiKeyHash) than the map key
(apiKeyInfo.getLookupKey()), causing entries to never be removed; fix it either
by changing removeOpaqueAPIKeyInfo to accept and use the lookupKey (rename the
parameter to lookupKey and call apiKeyInfoHashMap.remove(lookupKey)) if
lookupKey is the intended key, or implement removal-by-hash by iterating
apiKeyInfoHashMap.entrySet(), comparing each APIKeyInfo.getApiKeyHash() (or the
correct hash accessor) and calling iterator.remove() when matched; update the
method signature and javadoc accordingly (references: addOpaqueAPIKeyInfo,
getOpaqueAPIKeyInfo, removeOpaqueAPIKeyInfo, apiKeyInfoHashMap,
APIKeyInfo.getLookupKey()).
In
`@components/apimgt/org.wso2.carbon.apimgt.impl/src/main/java/org/wso2/carbon/apimgt/impl/publishers/OpaqueApiKeyPublisher.java`:
- Around line 40-47: The constructor for OpaqueApiKeyPublisher should guard
against nulls before calling init: check that realtimeNotifierProperties != null
AND opaqueApiKeyNotifier != null before invoking opaqueApiKeyNotifier.init(...),
and only set realtimeNotifierEnabled = true when both are present; if either is
null, skip calling opaqueApiKeyNotifier.init and ensure realtimeNotifierEnabled
remains false and add a debug/warn log explaining why; this prevents the NPE
thrown from OpaqueAPIKeyNotifierImpl.init() when realTimeNotifierProperties is
null and avoids calling methods on a null opaqueApiKeyNotifier instance.
In
`@components/apimgt/org.wso2.carbon.apimgt.impl/src/main/java/org/wso2/carbon/apimgt/impl/utils/APIUtil.java`:
- Around line 9211-9213: generateSalt() currently returns a zeroed 16-byte array
which makes all salts identical; change it to produce cryptographically-random
bytes (e.g., allocate a 16-byte array and fill it via SecureRandom.nextBytes) so
sha256HashWithSalt() receives a proper random salt when APIConsumerImpl creates
API keys; consider using a SecureRandom instance (static final or
SecureRandom.getInstanceStrong()) to generate the bytes before returning them.
In
`@components/apimgt/org.wso2.carbon.apimgt.notification/src/main/java/org/wso2/carbon/apimgt/notification/OpaqueAPIKeyNotifierImpl.java`:
- Around line 104-107: The init method in OpaqueAPIKeyNotifierImpl currently
calls realTimeNotifierProperties.clone() and will NPE if passed null; change
init(Properties realTimeNotifierProperties) to check for null before cloning –
if realTimeNotifierProperties is non-null assign this.realTimeNotifierProperties
= (Properties) realTimeNotifierProperties.clone(), otherwise assign an empty new
Properties() (or Collections.emptyProperties equivalent) so the field is never
null; refer to the init method and the this.realTimeNotifierProperties field
when making this change.
🟠 Major comments (16)
components/apimgt/org.wso2.carbon.apimgt.gateway/src/main/java/org/wso2/carbon/apimgt/gateway/utils/GatewayUtils.java-699-703 (1)
699-703:⚠️ Potential issue | 🟠 MajorPotential NPE when
payloadis null andapiKeyValidationInfoDTOis also null.Line 702 accesses
apiKeyValidationInfoDTO.getEndUserName()without a null guard. While theapiKeyValidationInfoDTO != nullcheck exists at line 705, the else-branch at line 702 is reached before that check. If a caller passes bothpayload = nullandapiKeyValidationInfoDTO = null, this will throw aNullPointerException.🛡️ Proposed fix
if (payload != null) { - authContext.setUsername(payload.getSubject()); + authContext.setUsername(payload.getSubject()); + } else if (apiKeyValidationInfoDTO != null) { + authContext.setUsername(apiKeyValidationInfoDTO.getEndUserName()); } else { - authContext.setUsername(apiKeyValidationInfoDTO.getEndUserName()); + authContext.setUsername(null); }components/apimgt/org.wso2.carbon.apimgt.impl/src/main/java/org/wso2/carbon/apimgt/impl/dao/constants/SQLConstants.java-3925-3931 (1)
3925-3931:⚠️ Potential issue | 🟠 MajorFilter revoked API keys from lookups/updates.
GET_API_KEY_FROM_DISPLAY_NAME_SQLandUPDATE_API_KEY_LAST_USED_SQLdon’t restrict by STATUS, so revoked keys can still be returned/updated. Align with the active-only expectation used byGET_API_KEY_SQLto avoid inadvertently treating revoked keys as valid.✅ Suggested fix
public static final String GET_API_KEY_FROM_DISPLAY_NAME_SQL = - "SELECT API_KEY_PROPERTIES, AUTHZ_USER, VALIDITY_PERIOD, LAST_USED FROM AM_API_KEY WHERE APPLICATION_ID = ? AND KEY_TYPE = ? AND API_KEY_NAME = ?"; + "SELECT API_KEY_PROPERTIES, AUTHZ_USER, VALIDITY_PERIOD, LAST_USED FROM AM_API_KEY " + + "WHERE APPLICATION_ID = ? AND KEY_TYPE = ? AND API_KEY_NAME = ? AND STATUS = 'ACTIVE'"; public static final String DELETE_API_KEY_SQL = "UPDATE AM_API_KEY SET STATUS = 'REVOKED' WHERE APPLICATION_ID = ? AND KEY_TYPE = ? AND API_KEY_NAME = ?"; public static final String UPDATE_API_KEY_LAST_USED_SQL = - "UPDATE AM_API_KEY SET LAST_USED = ? WHERE API_KEY_HASH = ?"; + "UPDATE AM_API_KEY SET LAST_USED = ? WHERE API_KEY_HASH = ? AND STATUS = 'ACTIVE'";components/apimgt/org.wso2.carbon.apimgt.gateway/src/main/java/org/wso2/carbon/apimgt/gateway/internal/DataHolder.java-62-62 (1)
62-62:⚠️ Potential issue | 🟠 MajorPlain
HashMapmay cause data races under concurrent access from JMS listener threads.
apiKeyInfoHashMapis a regularHashMap, but it will be mutated by JMS listener threads (viaOpaqueAPIKeyInfoListener) and read by gateway request-handling threads. This can lead to corrupted state or infinite loops inHashMap.get()under concurrent modification. Consider usingConcurrentHashMapinstead.I note that other maps in this class (e.g.,
llmProviderMap) also useHashMap, so this may be an accepted pattern. However, since opaque API key lookups are on the hot request path, the risk is higher here.components/apimgt/org.wso2.carbon.apimgt.jms.listener/src/main/java/org/wso2/carbon/apimgt/jms/listener/utils/APIKeyUsageListener.java-54-63 (1)
54-63: 🛠️ Refactor suggestion | 🟠 MajorUse the established JMS payload extraction pattern.
The existing JMS listeners in this module (e.g.,
CertificateManagerJMSMessageListener,KeyManagerJMSMessageListener) usepayloadData.get(APIConstants.CONSTANT_NAME).asText()rather thanpath(...).asText(). Thepath()approach silently returns a "missing node" instead ofnullon absent keys, masking missing data. Additionally, consider using constants fromAPIConstantsfor the field names"apiKeyHash"and"lastUsedTime"if they exist.Also, there's no validation that the extracted
apiKeyHashorlastUsedTimeare non-empty before calling the DAO update, which could write empty strings to the database.Proposed fix
- ObjectMapper objectMapper = new ObjectMapper(); - // Navigate to payloadData - JsonNode payload = null; - payload = objectMapper.readTree(textMessage) - .path("event") - .path("payloadData"); - - APIKeyInfo apiKeyInfo = new APIKeyInfo(); - apiKeyInfo.setApiKeyHash(payload.path("apiKeyHash").asText()); - apiKeyInfo.setLastUsedTime(payload.path("lastUsedTime").asText()); - - // Add to AM_API_KEY - ApiMgtDAO.getInstance().updateAPIKeyUsage(apiKeyInfo.getApiKeyHash(), apiKeyInfo.getLastUsedTime()); + ObjectMapper objectMapper = new ObjectMapper(); + JsonNode payloadData = objectMapper.readTree(textMessage) + .path("event") + .path("payloadData"); + + String apiKeyHash = payloadData.get("apiKeyHash").asText(); + String lastUsedTime = payloadData.get("lastUsedTime").asText(); + + if (apiKeyHash != null && !apiKeyHash.isEmpty()) { + ApiMgtDAO.getInstance().updateAPIKeyUsage(apiKeyHash, lastUsedTime); + } else { + log.warn("Received API key usage event with empty apiKeyHash"); + }Based on learnings: In WSO2 API Manager JMS listeners, the standard pattern for extracting payload data is
payloadData.get(APIConstants.CONSTANT_NAME).asText(), notpath(...).asText(null).components/apimgt/org.wso2.carbon.apimgt.rest.api.common/src/main/resources/admin-api.yaml-7103-7127 (1)
7103-7127:⚠️ Potential issue | 🟠 MajorAPI key list response lacks revocation identifiers and exposes raw secrets.
Clients can’t revoke keys from the list response becauseapplicationId,keyType, andkeyDisplayNamearen’t returned. Also, listing raw API keys is risky; prefer returning masked values plus metadata.🔧 Proposed schema adjustment
APIKey: title: API Key details to invoke APIs type: object properties: - apikey: - type: string - description: API Key - example: eyJoZWxsbyI6IndvcmxkIn0=.eyJ3c28yIjoiYXBpbSJ9.eyJ3c28yIjoic2lnbmF0dXJlIn0= + apiKeyMasked: + type: string + description: Masked API key value (never return the full key) + example: "****" + applicationId: + type: string + description: Application UUID + example: d7cf8523-9180-4255-84fa-6cb171c1f779 + keyType: + type: string + enum: + - PRODUCTION + - SANDBOX + keyDisplayName: + type: string + description: Name of the API key validityTime: type: integer format: int32 example: 3600components/apimgt/org.wso2.carbon.apimgt.rest.api.common/src/main/resources/admin-api.yaml-4583-4641 (1)
4583-4641:⚠️ Potential issue | 🟠 MajorEdit the source Admin API spec, not this generated copy.
This file is auto-generated during the build; changes here will be overwritten. Please move these endpoint additions to the source spec filecomponents/apimgt/org.wso2.carbon.apimgt.rest.api.admin.v1/src/main/resources/admin-api.yaml.Based on learnings: "In wso2/carbon-apimgt, the file components/apimgt/org.wso2.carbon.apimgt.rest.api.common/src/main/resources/admin-api.yaml is an automated copy file that gets updated during compilation/build from the source file components/apimgt/org.wso2.carbon.apimgt.rest.api.admin.v1/src/main/resources/admin-api.yaml; changes should be made to the source file only."
components/apimgt/org.wso2.carbon.apimgt.rest.api.admin.v1/src/main/resources/admin-api.yaml-7103-7127 (1)
7103-7127:⚠️ Potential issue | 🟠 MajorSeparate list response schema to avoid exposing full API keys.
GET /api-keys returns
APIKeyListcontaining fullAPIKeyobjects with raw secrets in the response. While this endpoint is restricted to admin users (apim:adminscope), the schema should use a separate response type (e.g.,APIKeyListItemorAPIKeySummary) that omits the raw key and provides only metadata (display name, validity time, etc.). Reserve the fullAPIKeyschema for create/regenerate endpoints where users need the secret.components/apimgt/org.wso2.carbon.apimgt.impl/src/main/java/org/wso2/carbon/apimgt/impl/dao/ApiMgtDAO.java-16648-16672 (1)
16648-16672:⚠️ Potential issue | 🟠 MajorResultSet resource leak —
rsis never closed.
rsis assigned at Line 16660 butAPIMgtDBUtil.closeAllConnections(ps, conn, null)at Line 16672 passesnullinstead ofrs. This leaks theResultSet. The other read methods (getAPIKeys,getAllAPIKeys) correctly use try-with-resources for theResultSet; this method should follow the same pattern.🐛 Proposed fix — use try-with-resources for the ResultSet
- ResultSet rs = null; try { conn = APIMgtDBUtil.getConnection(); - conn.setAutoCommit(false); // This query to access the AM_API_KEY table String sqlQuery = SQLConstants.GET_API_KEY_FROM_DISPLAY_NAME_SQL; // Retrieving data from the AM_API_KEY table ps = conn.prepareStatement(sqlQuery); ps.setString(1, applicationId); ps.setString(2, keyType); ps.setString(3, keyDisplayName); - rs = ps.executeQuery(); - if (rs.next()) { - keyInfo.setKeyDisplayName(keyDisplayName); - keyInfo.setValidityPeriod(rs.getLong("VALIDITY_PERIOD")); - keyInfo.setLastUsedTime(rs.getString("LAST_USED")); - keyInfo.setApplicationId(applicationId); - keyInfo.setKeyType(keyType); - keyInfo.setProperties(rs.getBytes("API_KEY_PROPERTIES")); + try (ResultSet rs = ps.executeQuery()) { + if (rs.next()) { + keyInfo.setKeyDisplayName(keyDisplayName); + keyInfo.setValidityPeriod(rs.getLong("VALIDITY_PERIOD")); + keyInfo.setLastUsedTime(rs.getString("LAST_USED")); + keyInfo.setApplicationId(applicationId); + keyInfo.setKeyType(keyType); + keyInfo.setProperties(rs.getBytes("API_KEY_PROPERTIES")); + } } } catch (SQLException e) {components/apimgt/org.wso2.carbon.apimgt.gateway/src/main/java/org/wso2/carbon/apimgt/gateway/handlers/security/apikey/ApiKeyAuthenticator.java-265-269 (1)
265-269:⚠️ Potential issue | 🟠 MajorRaw API key logged in debug output — security/compliance risk.
Line 268 logs the full raw API key:
"Token: " + apiKey. Even at debug level, this is a security concern as keys can end up in log aggregators and audit trails. Use a masked representation instead.Proposed fix
if (log.isDebugEnabled()) { log.debug("User is subscribed to the API: " + apiContext + ", " + - "version: " + apiVersion + ". Token: " + apiKey); + "version: " + apiVersion + ". Token: " + GatewayUtils.getMaskedToken(apiKey)); }components/apimgt/org.wso2.carbon.apimgt.gateway/src/main/java/org/wso2/carbon/apimgt/gateway/handlers/security/apikey/ApiKeyAuthenticator.java-291-293 (1)
291-293:⚠️ Potential issue | 🟠 MajorRaw API key passed as
tokenIdentifiertogenerateAuthenticationContext.On line 293, the raw
apiKeyvalue is passed as the first argument togenerateAuthenticationContext(apiKey, null, ...). In the JWT path (line 178), this parameter is the JWT ID (tokenIdentifier). Passing the raw opaque key here means it may be stored in theAuthenticationContextand propagated to analytics, logs, or downstream headers. Consider using theapiKeyHashorlookupKeyinstead.Proposed fix
- return GatewayUtils.generateAuthenticationContext(apiKey, null, apiKeyValidationInfoDTO, null); + return GatewayUtils.generateAuthenticationContext(lookupKey, null, apiKeyValidationInfoDTO, null);Note:
lookupKeywould need to be passed as a parameter or recomputed. Alternatively, useapiKeyHash.components/apimgt/org.wso2.carbon.apimgt.impl/src/main/java/org/wso2/carbon/apimgt/impl/publishers/OpaqueApiKeyPublisher.java-57-74 (1)
57-74:⚠️ Potential issue | 🟠 Major
publishApiKeyUsageEvents/publishApiKeyInfoEventswill NPE if notifier is null.Both methods invoke
opaqueApiKeyNotifier.sendLastUsedTimeOnRealtime(...)/sendApiKeyInfoOnRealtime(...)whenrealtimeNotifierEnabledis true, but don't guard against a nullopaqueApiKeyNotifier. If the constructor fix above is applied (disablingrealtimeNotifierEnabledwhen notifier is null), this becomes safe. Otherwise, add a null guard here as well.components/apimgt/org.wso2.carbon.apimgt.gateway/src/main/java/org/wso2/carbon/apimgt/gateway/utils/ApiKeyAuthenticatorUtils.java-299-303 (1)
299-303:⚠️ Potential issue | 🟠 MajorNPE when
additionalPropertiesisnullfor opaque keys without additional properties.When
payloadisnull(opaque key path), the code directly callsadditionalProperties.get(...)without a null check. IfAPIKeyInfo.getAdditionalProperties()returnsnull(e.g., no IP/referrer restrictions were set), this will throw aNullPointerException.Proposed fix
} else { // Taking values from the DB for an opaque API key - permittedIPList = additionalProperties.get("permittedIP"); - permittedRefererList = additionalProperties.get("permittedReferer"); + if (additionalProperties != null) { + permittedIPList = additionalProperties.get(APIConstants.JwtTokenConstants.PERMITTED_IP); + permittedRefererList = additionalProperties.get(APIConstants.JwtTokenConstants.PERMITTED_REFERER); + } }components/apimgt/org.wso2.carbon.apimgt.impl/src/main/java/org/wso2/carbon/apimgt/impl/APIConsumerImpl.java-3654-3657 (1)
3654-3657:⚠️ Potential issue | 🟠 MajorPublish API_KEY_INFO event for regenerated opaque keys.
generateApiKeyemits API key info events, but regeneration only writes to the DB. If gateways depend on the info stream for cache hydration, the new key won’t be recognized immediately. Consider reusingsendAPIKeyInfoEventafter insertion.components/apimgt/org.wso2.carbon.apimgt.impl/src/main/java/org/wso2/carbon/apimgt/impl/APIConsumerImpl.java-420-422 (1)
420-422:⚠️ Potential issue | 🟠 MajorRemove hardcoded lookup secret.
Embedding a fixed lookup secret in source makes lookup keys predictable and non-rotatable. Load this from secure configuration/secret store and fail fast if missing.
components/apimgt/org.wso2.carbon.apimgt.impl/src/main/java/org/wso2/carbon/apimgt/impl/APIConsumerImpl.java-3621-3648 (1)
3621-3648:⚠️ Potential issue | 🟠 MajorGuard against missing key info and null property values in regeneration.
getAPIKey(...)can return null (especially after revocation), andProperties#setPropertythrows if values are null. Fetch before revoking and default missing properties.Proposed fix
- // Revoke the existing key - revokeAPIKey(applicationId, keyType, keyDisplayName, tenantDomain); - // Generate a new key with the same display name and other additional properties - APIKeyInfo apiKeyInfo = apiMgtDAO.getAPIKey(applicationId, keyType, keyDisplayName); + // Load existing metadata before revocation (revocation may remove/alter it) + APIKeyInfo apiKeyInfo = apiMgtDAO.getAPIKey(applicationId, keyType, keyDisplayName); + if (apiKeyInfo == null) { + throw new APIMgtResourceNotFoundException( + "API key not found for display name: " + keyDisplayName); + } + // Revoke the existing key + revokeAPIKey(applicationId, keyType, keyDisplayName, tenantDomain); @@ - props.setProperty("permittedIP", oldProps.getProperty("permittedIP")); - props.setProperty("permittedReferer", oldProps.getProperty("permittedReferer")); + props.setProperty("permittedIP", + StringUtils.defaultString(oldProps.getProperty("permittedIP"))); + props.setProperty("permittedReferer", + StringUtils.defaultString(oldProps.getProperty("permittedReferer")));components/apimgt/org.wso2.carbon.apimgt.impl/src/main/java/org/wso2/carbon/apimgt/impl/APIConsumerImpl.java-3603-3618 (1)
3603-3618:⚠️ Potential issue | 🟠 MajorPublish the actual API key value, not the display name, to the revocation event.
The gateway cache lookup uses the actual API key to invalidate entries, not the display name. Publishing
keyDisplayName(e.g., "Test_Key") will fail to remove the cached API key since the gateway'sremoveApiKeyFromGatewayCache()searches by the actual token value.Fetch the key details using
apiMgtDAO.getAPIKey(applicationId, keyType, keyDisplayName)and publish eitherapiKeyInfo.getApiKey()orapiKeyInfo.getLookupKey()instead ofkeyDisplayName.
🟡 Minor comments (18)
components/apimgt/org.wso2.carbon.apimgt.gateway/src/main/java/org/wso2/carbon/apimgt/gateway/utils/GatewayUtils.java-727-727 (1)
727-727:⚠️ Potential issue | 🟡 MinorRedundant null check —
StringUtils.isNotEmptyis already null-safe.
StringUtils.isNotEmpty(endUserToken)returnsfalsewhenendUserTokenisnull, so the trailing&& endUserToken != nullis redundant. Additionally, the logical order is inverted: the null check should precede the content check if both were needed. The previous version of this line used onlyStringUtils.isNotEmpty(endUserToken), which was already correct.♻️ Proposed fix
- if (StringUtils.isNotEmpty(endUserToken) && endUserToken != null) { + if (StringUtils.isNotEmpty(endUserToken)) {components/apimgt/org.wso2.carbon.apimgt.jms.listener/src/main/java/org/wso2/carbon/apimgt/jms/listener/utils/APIKeyUsageListener.java-71-72 (1)
71-72:⚠️ Potential issue | 🟡 MinorError log message is misleading for non-JMS exceptions.
The catch block catches
JMSException | JsonProcessingException | APIManagementExceptionbut the log message always says "JMSException occurred". Use a generic message instead.Proposed fix
- } catch (JMSException | JsonProcessingException | APIManagementException e) { - log.error("JMSException occurred when processing the received message ", e); + } catch (JMSException | JsonProcessingException | APIManagementException e) { + log.error("Error occurred when processing the API key usage message ", e); }components/apimgt/org.wso2.carbon.apimgt.jms.listener/src/main/java/org/wso2/carbon/apimgt/jms/listener/utils/APIKeyUsageListener.java-45-45 (1)
45-45:⚠️ Potential issue | 🟡 MinorRemove debug artifact from production code.
The
log.info("🔥 API Key Usage JMS message RECEIVED")line with an emoji is clearly a development/debug leftover. It should either be removed or converted tolog.debugwith a professional message, consistent with other JMS listeners in this module.Proposed fix
- log.info("🔥 API Key Usage JMS message RECEIVED"); + if (log.isDebugEnabled()) { + log.debug("API Key Usage JMS message received"); + }components/apimgt/org.wso2.carbon.apimgt.rest.api.admin.v1/src/main/resources/admin-api.yaml-4609-4611 (1)
4609-4611:⚠️ Potential issue | 🟡 MinorReplace hardcoded Bearer token in the GET sample.
This looks like a real token and will trip secret scanners. Use a placeholder instead.
🧩 Proposed change
- source: 'curl -k -H "Authorization: Bearer ae4eae22-3f65-387b-a171-d37eaa366fa8" + source: 'curl -k -H "Authorization: Bearer <ACCESS_TOKEN>"components/apimgt/org.wso2.carbon.apimgt.rest.api.admin.v1/src/main/resources/admin-api.yaml-7481-7487 (1)
7481-7487:⚠️ Potential issue | 🟡 MinorClarify encoding constraints for
keyDisplayNamepath param.Display names often include spaces or reserved characters; as a path segment, this needs URL‑encoding guidance to prevent routing issues.
🧩 Proposed change
keyDisplayName: name: keyDisplayName in: path description: | - Name of the API key. + Name of the API key. URL-encode this value if it contains reserved path characters.components/apimgt/org.wso2.carbon.apimgt.rest.api.store.v1/src/main/resources/devportal-api.yaml-3327-3330 (1)
3327-3330:⚠️ Potential issue | 🟡 MinorChange If-Match to If-None-Match for this GET endpoint.
Line 3329 uses
If-Matchon a GET endpoint for retrieving API keys. This is inconsistent with REST API semantics and the pattern used across all other GET endpoints in this spec, which useIf-None-Matchfor conditional request validation and caching.If-Matchis intended for mutating operations (PUT/DELETE/PATCH) to validate preconditions before modification.🔧 Suggested fix
- - $ref: '#/components/parameters/If-Match' + - $ref: '#/components/parameters/If-None-Match'components/apimgt/org.wso2.carbon.apimgt.rest.api.store.v1/src/main/resources/devportal-api.yaml-5993-6012 (1)
5993-6012:⚠️ Potential issue | 🟡 MinorUpdate timestamp field examples and descriptions for clarity.
The
issuedOnexample2026-02-06 23:45:07does not match backend behavior, which returns epoch milliseconds as a string (e.g.,1738893907000). Additionally,lastUsedcan be either a timestamp string or the sentinelNOT_USED, which needs clarification.Keep both fields as
type: stringfor backward compatibility, but fix the example values and descriptions:🧭 Suggested fix
issuedOn: type: string - description: Created Time - example: 2026-02-06 23:45:07 + description: Created time as epoch milliseconds (numeric string). + example: 1738893907000 @@ lastUsed: type: string - description: Last Used Time - example: NOT_USED + description: Last used time as epoch milliseconds, or NOT_USED if never used. + example: NOT_USEDcomponents/apimgt/org.wso2.carbon.apimgt.rest.api.store.v1/src/main/resources/devportal-api.yaml-3368-3377 (1)
3368-3377:⚠️ Potential issue | 🟡 MinorAdd 404 response for consistency with similar endpoints.
The
/regenerateendpoint with the samekeyDisplayNameparameter pattern documents a 404 response, so the revoke endpoint should too. This provides clients consistent error handling across similar operations on API keys.Suggested fix
$ref: '#/components/responses/BadRequest' + 404: + $ref: '#/components/responses/NotFound' 412: $ref: '#/components/responses/PreconditionFailed'components/apimgt/org.wso2.carbon.apimgt.rest.api.store.v1/src/main/java/org/wso2/carbon/apimgt/rest/api/store/v1/mappings/ApplicationKeyMappingUtil.java-152-164 (1)
152-164:⚠️ Potential issue | 🟡 MinorNarrowing cast from
longtointonvalidityPeriodcan silently truncate.
APIKeyInfo.getValidityPeriod()returnslong, butdto.setValidityPeriod((int) src.getValidityPeriod())on Line 158 truncates it. If validity periods are ever stored in milliseconds or as very large values (e.g.,-1Lfor "never expires" is fine, but values >Integer.MAX_VALUEwill wrap), this silently produces incorrect results.The same pattern appears in
ApplicationsApiServiceImplLine 803–804 with the regenerate flow. If the DTO'svalidityPeriodfield type cannot be changed tolong, consider adding a bounds check or usingMath.toIntExact()(which throwsArithmeticExceptionon overflow) to fail explicitly rather than silently.🔧 Safer cast option
- dto.setValidityPeriod((int) src.getValidityPeriod()); + dto.setValidityPeriod(Math.toIntExact(src.getValidityPeriod()));components/apimgt/org.wso2.carbon.apimgt.rest.api.store.v1/src/main/java/org/wso2/carbon/apimgt/rest/api/store/v1/impl/ApplicationsApiServiceImpl.java-748-789 (1)
748-789:⚠️ Potential issue | 🟡 MinorMultiple issues in the DELETE endpoint.
Double negative typo (Line 771):
"doesn't not exist"→ should be"doesn't exist"or"does not exist".Misleading error message (Line 773): When the application is not found, the error returned is
"Validation failed for the given token". It should indicate the application was not found, similar to other endpoints that useRestApiUtil.handleResourceNotFoundError.Unguarded debug log (Line 785):
log.debug(...)is not guarded byif (log.isDebugEnabled()), unlike Lines 762 and 770 in the same method. This forces string concatenation even when debug is disabled.🐛 Suggested fixes
} else { - if(log.isDebugEnabled()) { - log.debug("Application with given id " + applicationId + " doesn't not exist "); - } - RestApiUtil.handleBadRequest("Validation failed for the given token ", log); + RestApiUtil.handleResourceNotFoundError(RestApiConstants.RESOURCE_APPLICATION, + applicationId, log); }} else { - log.debug("Provided API Key " + keyDisplayName + " is not valid"); + if (log.isDebugEnabled()) { + log.debug("Provided API Key display name " + keyDisplayName + " is not valid"); + } RestApiUtil.handleBadRequest("Provided API Key isn't valid ", log); }components/apimgt/org.wso2.carbon.apimgt.rest.api.store.v1/src/main/java/org/wso2/carbon/apimgt/rest/api/store/v1/impl/ApplicationsApiServiceImpl.java-791-834 (1)
791-834:⚠️ Potential issue | 🟡 MinorSame issues repeated in the REGENERATE endpoint.
Double negative typo (Line 816):
"doesn't not exist"— same fix needed as in DELETE.Misleading error message (Line 818): Should use
handleResourceNotFoundErrorinstead ofhandleBadRequestwhen the application is not found.Unguarded debug log (Line 830): Same issue —
log.debug(...)not guarded.
longtointtruncation (Line 803–804):(int) apiKeyInfo.getValidityPeriod()— same narrowing cast concern flagged inApplicationKeyMappingUtil.🐛 Suggested fixes
} else { - if(log.isDebugEnabled()) { - log.debug("Application with given id " + applicationId + " doesn't not exist "); - } - RestApiUtil.handleBadRequest("Validation failed for the given API Key ", log); + RestApiUtil.handleResourceNotFoundError(RestApiConstants.RESOURCE_APPLICATION, + applicationId, log); }} else { - log.debug("Provided API Key " + keyDisplayName + " is not valid"); + if (log.isDebugEnabled()) { + log.debug("Provided API Key display name " + keyDisplayName + " is not valid"); + } RestApiUtil.handleBadRequest("Provided API Key isn't valid ", log); }components/apimgt/org.wso2.carbon.apimgt.rest.api.store.v1/src/main/java/org/wso2/carbon/apimgt/rest/api/store/v1/impl/ApplicationsApiServiceImpl.java-719-746 (1)
719-746:⚠️ Potential issue | 🟡 MinorAdd keyType validation to DELETE and REGENERATE endpoints for consistency.
The GET endpoint (lines 731–734) validates that
keyTypeis eitherPRODUCTIONorSANDBOX. The DELETE (line 759) and REGENERATE (line 802) endpoints passkeyTypedirectly to the backend without validation. The backend methods do not validate either. For consistency and to prevent unexpected behavior, add the same keyType validation to DELETE and REGENERATE endpoints before calling the consumer methods.components/apimgt/org.wso2.carbon.apimgt.rest.api.common/src/main/resources/devportal-api.yaml-7781-7788 (1)
7781-7788:⚠️ Potential issue | 🟡 MinorDocument/encode
keyDisplayNamepath parameter to avoid routing issues.Display names can contain spaces/special chars; clarifying URL encoding (or marking encoded) prevents 404s from unencoded paths.
🔧 Suggested update
keyDisplayName: name: keyDisplayName in: path description: | - Name of the API key. + Name of the API key. URL-encode reserved characters. required: true schema: type: string + x-encoded: truecomponents/apimgt/org.wso2.carbon.apimgt.impl/src/main/java/org/wso2/carbon/apimgt/impl/dao/ApiMgtDAO.java-16714-16715 (1)
16714-16715:⚠️ Potential issue | 🟡 MinorTypo in Javadoc: "LAst" → "Last".
- * `@param` lastUsedTime LAst used time + * `@param` lastUsedTime Last used timecomponents/apimgt/org.wso2.carbon.apimgt.impl/src/main/java/org/wso2/carbon/apimgt/impl/dao/ApiMgtDAO.java-16576-16576 (1)
16576-16576:⚠️ Potential issue | 🟡 MinorPotential NPE if
TIME_CREATEDisNULLin the database.
rs.getTimestamp("TIME_CREATED").toString()will throw aNullPointerExceptionif the column value isNULL. The same pattern appears at Line 16615 ingetAllAPIKeys.🛡️ Proposed defensive fix
- keyInfo.setCreatedTime(rs.getTimestamp("TIME_CREATED").toString()); + Timestamp createdTime = rs.getTimestamp("TIME_CREATED"); + keyInfo.setCreatedTime(createdTime != null ? createdTime.toString() : null);components/apimgt/org.wso2.carbon.apimgt.notification/src/main/java/org/wso2/carbon/apimgt/notification/OpaqueAPIKeyNotifierImpl.java-86-86 (1)
86-86:⚠️ Potential issue | 🟡 Minor
Long.parseLongon a potentially null property —NumberFormatException.
properties.getProperty(APIConstants.NotificationEvent.VALIDITY_PERIOD)may returnnullif the property is not set, causingLong.parseLong(null)to throwNumberFormatException. A default value or null check is needed.Proposed fix
- long validityPeriod = Long.parseLong(properties.getProperty(APIConstants.NotificationEvent.VALIDITY_PERIOD)); + String validityPeriodStr = properties.getProperty(APIConstants.NotificationEvent.VALIDITY_PERIOD); + long validityPeriod = validityPeriodStr != null ? Long.parseLong(validityPeriodStr) : 0L;components/apimgt/org.wso2.carbon.apimgt.gateway/src/main/java/org/wso2/carbon/apimgt/gateway/listeners/OpaqueAPIKeyInfoListener.java-43-43 (1)
43-43:⚠️ Potential issue | 🟡 MinorRemove debug artifact: emoji log at INFO level.
This
log.info("🔥 Opaque API Key JMS message RECEIVED")is clearly a development/debug artifact. It should be removed or converted to alog.debugcall without the emoji before merging.Proposed fix
- log.info("🔥 Opaque API Key JMS message RECEIVED"); + if (log.isDebugEnabled()) { + log.debug("Opaque API Key JMS message received"); + }components/apimgt/org.wso2.carbon.apimgt.gateway/src/main/java/org/wso2/carbon/apimgt/gateway/listeners/OpaqueAPIKeyInfoListener.java-68-75 (1)
68-75:⚠️ Potential issue | 🟡 Minor
asText()on a missing JSON node returns"", notnull— null check is ineffective.
payload.path("additionalProperties").asText()returns an empty string""when the node is absent, so thenullcheck on line 70 will always pass. IfadditionalPropertiesis genuinely missing,StringEscapeUtils.unescapeJson("")produces""andobjectMapper.readValue("", ...)throwsJsonProcessingException—caught but misleading.Check for
StringUtils.isNotEmptyinstead, or useasText(null)which returnsnullfor missing nodes.Proposed fix
- String additionalPropsEscaped = payload.path("additionalProperties").asText(); + String additionalPropsEscaped = payload.path("additionalProperties").asText(null); Map<String, String> additionalPropsMap = null; if (additionalPropsEscaped != null) {
🧹 Nitpick comments (19)
components/apimgt/org.wso2.carbon.apimgt.api/src/main/java/org/wso2/carbon/apimgt/api/model/APIKeyInfo.java (2)
23-38: Consider usinglongfor timestamp fields instead ofString.
lastUsedTimeandcreatedTimeare declared asString, but timestamps are typically represented aslong(epoch millis) in Java model classes. String timestamps are fragile—they depend on consistent formatting across producers and consumers. The JMS listener (inAPIKeyUsageListener) passes these strings directly to the DAO, which could fail silently or store inconsistent formats.That said, if the REST API contract requires string representation for backward compatibility, this may be intentional.
112-118: Mutablebyte[]exposed via getter/setter.
getProperties()returns the internalbyte[]reference, allowing callers to mutate the object's state. For a model class used across modules, consider defensive copying. This is a minor concern but worth noting.components/apimgt/org.wso2.carbon.apimgt.api/src/main/java/org/wso2/carbon/apimgt/api/APIConsumer.java (1)
720-738: Javadoc forregenerateAPIKeyis missing@returnand@param usernametags.The
revokeAPIKeyJavadoc also omits@throwsdescription, though that's consistent with other methods in this interface. TheregenerateAPIKeyJavadoc is notably missing documentation for theusernameparameter and the return type.📝 Suggested Javadoc fix
/** * Regenerate opaque api key for the given key display name with same properties * `@param` applicationId Id of the application * `@param` keyType Key type of the token * `@param` keyDisplayName Api key name * `@param` tenantDomain Tenant domain + * `@param` username Username of the user requesting regeneration + * `@return` APIKeyInfo containing the regenerated key details * `@throws` APIManagementException */components/apimgt/org.wso2.carbon.apimgt.impl/src/main/java/org/wso2/carbon/apimgt/impl/dto/APIKeyDTO.java (2)
1-101: New DTO looks reasonable; note the naming overlap with the Admin REST API'sAPIKeyDTO.There is already an
APIKeyDTOinorg.wso2.carbon.apimgt.rest.api.admin.v1.dto(with different fields:apikey+validityTime). While they reside in separate packages, the identical class name may confuse developers working across modules. Consider whether a more distinctive name (e.g.,OpaqueAPIKeyDTO) would reduce ambiguity. This is a minor concern since packages disambiguate at compile time.
54-60:getApiKeyProperties()exposes the internal mutablebyte[]array directly.Callers can mutate the DTO's internal state through the returned reference. For a
SerializableDTO, consider returning a defensive copy if immutability matters in your usage context.🛡️ Defensive copy option
public byte[] getApiKeyProperties() { - return apiKeyProperties; + return apiKeyProperties != null ? apiKeyProperties.clone() : null; } public void setApiKeyProperties(byte[] apiKeyProperties) { - this.apiKeyProperties = apiKeyProperties; + this.apiKeyProperties = apiKeyProperties != null ? apiKeyProperties.clone() : null; }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)
36-37: Wildcard importjava.util.*replaces specific imports.Minor style nit — specific imports are generally preferred for readability and avoiding accidental namespace collisions, consistent with other files in this PR that use explicit imports.
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)
62-62: Wildcard import forstore.v1.dto.*.Same observation as in
ApplicationKeyMappingUtil— explicit imports are generally preferred. This replaces what were specific imports, reducing import-level clarity.components/apimgt/org.wso2.carbon.apimgt.rest.api.common/src/main/resources/devportal-api.yaml (1)
5994-6025: Clarify timestamp vs sentinel values inAPIKeyInfo.
issuedOnlooks like a timestamp whilelastUsedcan be"NOT_USED"; document the expected format and sentinel value to avoid client parsing ambiguity.📝 Suggested doc clarification
issuedOn: type: string - description: Created Time + description: Created time (e.g., "yyyy-MM-dd HH:mm:ss") example: 2026-02-06 23:45:07 validityPeriod: type: integer format: int32 example: 3600 lastUsed: type: string - description: Last Used Time + description: Last used time in the same format as issuedOn; use "NOT_USED" when the key has never been used. example: NOT_USEDcomponents/apimgt/org.wso2.carbon.apimgt.impl/src/main/java/org/wso2/carbon/apimgt/impl/dao/ApiMgtDAO.java (4)
45-45: Wildcard imports reduce readability and can cause ambiguous references.Wildcard imports (
org.wso2.carbon.apimgt.api.model.*andorg.wso2.carbon.apimgt.impl.dto.*at Line 76) make it harder to determine which specific classes are used and can lead to conflicts if new classes are added to those packages in the future. Consider using explicit imports instead.
16513-16547: Missing explicit rollback on failure in write operations.
addAPIKey,revokeAPIKey, andupdateAPIKeyUsageall callconn.setAutoCommit(false)andconn.commit()on success, but none issueconn.rollback()in thecatchblock beforehandleExceptionrethrows. While most connection pools auto-rollback on close, the JDBC spec does not guarantee this. An explicit rollback is safer.This applies to all three write methods in this PR (lines 16513, 16685, 16717).
♻️ Example fix for addAPIKey (apply same pattern to revokeAPIKey and updateAPIKeyUsage)
} catch (SQLException e) { + if (conn != null) { + try { + conn.rollback(); + } catch (SQLException rollbackEx) { + log.error("Failed to rollback adding API key", rollbackEx); + } + } handleException("Failed to add generated API keys", e); } finally {
16561-16563: UnnecessarysetAutoCommit(false)on read-only queries.
getAPIKeys,getAllAPIKeys, andgetAPIKeyall disable auto-commit for pure SELECT operations. This opens an explicit transaction that is never committed, holding database resources until the connection is returned to the pool. For read-only methods, simply removeconn.setAutoCommit(false)or use the default auto-commit behavior.♻️ Proposed fix (apply similarly to getAllAPIKeys and getAPIKey)
conn = APIMgtDBUtil.getConnection(); - conn.setAutoCommit(false);
16693-16701: Misleading SQL constant nameDELETE_API_KEY_SQLfor a soft-delete (status update).The comment at Line 16695 says "Updating data… by setting STATUS to REVOKED", but the constant is named
DELETE_API_KEY_SQL. If this is truly a status update rather than aDELETE FROMstatement, the constant name is misleading. Consider renaming it to something likeREVOKE_API_KEY_SQLfor clarity.components/apimgt/org.wso2.carbon.apimgt.impl/src/main/java/org/wso2/carbon/apimgt/impl/utils/APIKeyUtils.java (1)
55-67: New method follows existing pattern — consider extracting a shared helper.
isJWTAPIKeyGenerationEnabled()duplicates the structure ofisLightweightAPIKeyGenerationEnabled()line-for-line with only the config property key differing. A small private helper likegetBooleanConfig(String propertyKey, boolean defaultValue)would reduce duplication.Example refactor
+ private static boolean getBooleanConfig(String propertyKey, boolean defaultValue) { + try { + APIManagerConfiguration config = ServiceReferenceHolder.getInstance() + .getAPIManagerConfigurationService().getAPIManagerConfiguration(); + if (config != null) { + return Boolean.parseBoolean(config.getFirstProperty(propertyKey)); + } + } catch (Exception e) { + log.error("Error while reading configuration: " + propertyKey, e); + } + return defaultValue; + } + public static boolean isLightweightAPIKeyGenerationEnabled() { - try { - APIManagerConfiguration config = ServiceReferenceHolder.getInstance().getAPIManagerConfigurationService() - .getAPIManagerConfiguration(); - if (config != null) { - String lightweightAPIKeyGenerationEnabled = config.getFirstProperty(APIConstants.LIGHTWEIGHT_API_KEY_GENERATION_ENABLED); - return Boolean.parseBoolean(lightweightAPIKeyGenerationEnabled); - } - } catch (Exception e) { - log.error("Error while reading Lightweight API Key Generation configuration", e); - } - return true; + return getBooleanConfig(APIConstants.LIGHTWEIGHT_API_KEY_GENERATION_ENABLED, true); } public static boolean isJWTAPIKeyGenerationEnabled() { - try { - APIManagerConfiguration config = ServiceReferenceHolder.getInstance().getAPIManagerConfigurationService() - .getAPIManagerConfiguration(); - if (config != null) { - String jwtAPIKeyGenerationEnabled = config.getFirstProperty(APIConstants.JWT_API_KEY_GENERATION_ENABLED); - return Boolean.parseBoolean(jwtAPIKeyGenerationEnabled); - } - } catch (Exception e) { - log.error("Error while reading JWT API Key Generation configuration", e); - } - return true; + return getBooleanConfig(APIConstants.JWT_API_KEY_GENERATION_ENABLED, true); }components/apimgt/org.wso2.carbon.apimgt.gateway/src/main/java/org/wso2/carbon/apimgt/gateway/listeners/OpaqueAPIKeyInfoListener.java (1)
52-52: Avoid creatingObjectMapperinstances per message — use a shared static instance.
ObjectMapperis thread-safe for read operations and expensive to construct. Creating two instances per message (lines 52 and 73) adds unnecessary allocation overhead on the JMS message-processing hot path.Proposed fix
public class OpaqueAPIKeyInfoListener implements MessageListener { private static final Log log = LogFactory.getLog(OpaqueAPIKeyInfoListener.class); + private static final ObjectMapper objectMapper = new ObjectMapper(); public void onMessage(Message message) { ... - ObjectMapper objectMapper = new ObjectMapper(); // Navigate to payloadData - JsonNode payload = null; - payload = objectMapper.readTree(textMessage) + JsonNode payload = objectMapper.readTree(textMessage) .path("event") .path("payloadData"); ... - additionalPropsMap = new ObjectMapper().readValue(unescaped, + additionalPropsMap = objectMapper.readValue(unescaped, new TypeReference<Map<String, String>>() {});Also applies to: 73-74
components/apimgt/org.wso2.carbon.apimgt.gateway/src/main/java/org/wso2/carbon/apimgt/gateway/utils/ApiKeyAuthenticatorUtils.java (1)
54-56: Unused imports:ByteArrayInputStream,IOException,ObjectInputStream.These three imports don't appear to be used in the current file. They may be leftovers from an earlier iteration.
components/apimgt/org.wso2.carbon.apimgt.gateway/src/main/java/org/wso2/carbon/apimgt/gateway/handlers/security/apikey/ApiKeyAuthenticator.java (2)
133-196: RedundantvalidateAPIKeyFormatcall after length check.Line 136 checks
splitToken.length == 3and line 137 callsvalidateAPIKeyFormat(splitToken)which only throws if length ≠ 3. The call is always a no-op here.Proposed fix — remove the redundant guard or the redundant call
if (StringUtils.isNotEmpty(apiKey) && apiKey.contains(APIConstants.DOT)) { String[] splitToken = apiKey.split("\\."); if (splitToken.length == 3) { - ApiKeyAuthenticatorUtils.validateAPIKeyFormat(splitToken); SignedJWT signedJWT = (SignedJWT) JWTParser.parse(apiKey);
288-289: TODO: Missing expiry check and caching for opaque API keys.These TODO comments indicate that expiry validation and caching are not yet implemented. Without expiry checks, revoked or expired opaque keys may continue to authenticate successfully until the gateway restarts or the cache entry is evicted.
Would you like me to help draft the expiry check and caching logic, or open an issue to track this?
components/apimgt/org.wso2.carbon.apimgt.impl/src/main/java/org/wso2/carbon/apimgt/impl/utils/APIUtil.java (2)
9215-9220: Avoid duplicate byte-to-hex helpers.
bytesToHexalready exists; reuse it to prevent drift.♻️ Proposed refactor
public static String convertBytesToHex(byte[] bytes) { - StringBuilder hex = new StringBuilder(bytes.length * 2); - for (byte b : bytes) { - hex.append(String.format("%02x", b)); - } - return hex.toString(); + return bytesToHex(bytes); }
9198-9208: Add defensive validation forsharedSecretparameter.
While the current call sites use non-blank hardcoded values, validating the parameter follows the defensive programming pattern used throughout APIUtil and protects against future misconfiguration. Consider adding a null/blank check at method entry to fail fast with a clear error.🛠️ Suggested improvement
public static String generateLookupKey(String apiKey, String sharedSecret) throws APIManagementException { + if (StringUtils.isBlank(sharedSecret)) { + throw new APIManagementException("Shared secret is not configured for API key lookup"); + } try { Mac mac = Mac.getInstance(APIConstants.HMAC_SHA_256); SecretKeySpec keySpec = new SecretKeySpec(sharedSecret.getBytes(StandardCharsets.UTF_8),
...c/main/java/org/wso2/carbon/apimgt/gateway/handlers/security/apikey/ApiKeyAuthenticator.java
Show resolved
Hide resolved
....carbon.apimgt.gateway/src/main/java/org/wso2/carbon/apimgt/gateway/internal/DataHolder.java
Show resolved
Hide resolved
....apimgt.impl/src/main/java/org/wso2/carbon/apimgt/impl/publishers/OpaqueApiKeyPublisher.java
Show resolved
Hide resolved
...mgt/org.wso2.carbon.apimgt.impl/src/main/java/org/wso2/carbon/apimgt/impl/utils/APIUtil.java
Show resolved
Hide resolved
...notification/src/main/java/org/wso2/carbon/apimgt/notification/OpaqueAPIKeyNotifierImpl.java
Show resolved
Hide resolved
...mgt.gateway/src/main/java/org/wso2/carbon/apimgt/gateway/utils/ApiKeyAuthenticatorUtils.java
Show resolved
Hide resolved
...2.carbon.apimgt.gateway/src/main/java/org/wso2/carbon/apimgt/gateway/utils/GatewayUtils.java
Show resolved
Hide resolved
...mgt/org.wso2.carbon.apimgt.impl/src/main/java/org/wso2/carbon/apimgt/impl/dao/ApiMgtDAO.java
Show resolved
Hide resolved
4 participants
Fixes related to wso2/api-manager#4642