Copilot commented on code in PR #13149:
URL: https://github.com/apache/cloudstack/pull/13149#discussion_r3542122199
##########
server/src/main/java/com/cloud/user/AccountManagerImpl.java:
##########
@@ -3202,7 +3202,7 @@ public Pair<Boolean, Map<String, String>>
getKeys(GetUserKeysCmd cmd) {
ApiKeyPair keyPair;
if (accessingApiKey != null) {
ApiKeyPair accessingKeyPair =
apiKeyPairService.findByApiKey(accessingApiKey);
- if (userId == accessingKeyPair.getUserId()) {
+ if (accessingKeyPair != null && userId ==
accessingKeyPair.getUserId()) {
keyPair = apiKeyPairService.findByApiKey(accessingApiKey);
Review Comment:
`getKeys` looks up the same API key twice. Since `accessingKeyPair` already
contains the key pair, reuse it to avoid a redundant DB call and ensure both
checks and returned key pair are consistent.
##########
server/src/main/java/com/cloud/user/AccountManagerImpl.java:
##########
@@ -3320,6 +3320,10 @@ private Boolean isAccessingKeypairSuperset(ApiKeyPair
accessedKeyPair, BaseCmd c
return Boolean.TRUE;
}
ApiKeyPair accessingKeyPair = apiKeyPairService.findByApiKey(apiKey);
+ if (accessingKeyPair == null) {
+ logger.warn("Unable to find API key pair for the accessing API
key: {}", apiKey);
+ return Boolean.FALSE;
+ }
Review Comment:
This warning logs the full API key value, which is sensitive and typically
should not be written to logs. Consider removing the API key from the log
message (or masking it) to avoid leaking credentials in server logs.
##########
server/src/main/java/com/cloud/user/AccountManagerImpl.java:
##########
@@ -3335,7 +3339,7 @@ public String getAccessingApiKey(BaseCmd cmd) {
return accessingApiKey;
}
} catch (NullPointerException e) {
- logger.info("Accessing API through session.");
+ logger.info("Accessing API through session.", e);
}
return null;
Review Comment:
Catching `NullPointerException` to detect session-based access is brittle
and can hide real bugs. It would be safer to perform explicit null checks on
`cmd.getJob()` / `cmd.getFullUrlParams()` and avoid logging stack traces at
INFO for control flow.
##########
server/src/main/java/com/cloud/user/AccountManagerImpl.java:
##########
@@ -3582,6 +3586,10 @@ public List<RolePermissionEntity>
getAllKeypairPermissions(String apiKey) {
throw new InvalidParameterValueException("API key not present in
the request's URL and, thus, unable to fetch API key rules.");
}
ApiKeyPair apiKeyPair = keyPairManager.findByApiKey(apiKey);
+ if (apiKeyPair == null) {
+ logger.warn("Unable to find API key pair by API key: {}", apiKey);
+ return new ArrayList<>();
+ }
Review Comment:
When the API key pair cannot be found, returning an empty permission list
will make `listApisForKeyPair` return an empty API list (since no rules match)
and can be hard to diagnose. Consider failing fast with a clear exception so
callers know the API key is invalid, and avoid logging the full API key value.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
