bernardodemarco commented on code in PR #13149:
URL: https://github.com/apache/cloudstack/pull/13149#discussion_r3543995256
##########
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.TRUE;
Review Comment:
It is a bit confusing, but the first conditional expression has a different
semantics. It retrieves the API key existing in the request payload, i.e., (i)
if the request was authenticated with an API key pair, the corresponding API
key is returned, else (ii), the request was authenticated via session and,
therefore, `null` is returned.
Thus, if `null` is returned (authentication through session), it is
confirmed that the user should have access to all its API key pairs and, as a
consequence, `true` should be returned
##########
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()) {
Review Comment:
Yes, it would be nice
##########
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:
This does not need to be applied in this PR
##########
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:
This Copilot suggestion does not make sense in the context of this method
##########
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:
As for this suggestion, we could apply it in this PR, if you're willing to,
@sureshanaparti. But it is nitpicking IMO
--
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]
