Copilot commented on code in PR #13149:
URL: https://github.com/apache/cloudstack/pull/13149#discussion_r3542771042
##########
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 pair twice (first into
`accessingKeyPair`, then again into `keyPair`). Reuse the already-fetched
`accessingKeyPair` to avoid an extra DAO/service call and keep the logic
clearer.
##########
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.");
+ return new ArrayList<>();
+ }
Review Comment:
Returning an empty permission list when the API key pair cannot be found
conflates two different states ("key exists but has no explicit permissions" vs
"key does not exist") and can make callers like API discovery return an empty
API list with no clear error. Consider failing fast with a clear exception when
the API key pair lookup returns null.
##########
server/src/main/java/com/cloud/user/AccountManagerImpl.java:
##########
@@ -3334,8 +3338,8 @@ public String getAccessingApiKey(BaseCmd cmd) {
if (accessedByApiKey) {
return accessingApiKey;
}
- } catch (NullPointerException e) {
- logger.info("Accessing API through session.");
+ } catch (Exception e) {
+ logger.info("Error accessing API through session.", e);
}
Review Comment:
Catching a broad `Exception` here and logging it at INFO will likely turn
normal control flow (e.g., missing async job payload / missing params) into
noisy stack traces on production. Prefer explicit null checks and (at most) a
narrow `RuntimeException` catch logged at DEBUG so session-based API calls
don't spam logs.
##########
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);
Review Comment:
These changes adjust behavior around session-vs-apiKey detection and
handling of missing API key pairs. Add unit tests (e.g., in
`server/src/test/java/com/cloud/user/AccountManagerImplTest.java`) covering:
(1) `getKeys` when `apiKeyPairService.findByApiKey` returns null, (2)
`getAccessingApiKey` when `fullUrlParams` is null / missing apiKey, and (3)
`getAllKeypairPermissions` when the API key is unknown.
--
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]
