Fix NPE with ApiKeyPair during listApis call (from cmk) #13149
+14
−7
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -3202,7 +3202,7 @@ public Pair<Boolean, Map<String, String>> getKeys(GetUserKeysCmd cmd) { | |
| ApiKeyPair keyPair; | ||
| if (accessingApiKey != null) { | ||
| ApiKeyPair accessingKeyPair = apiKeyPairService.findByApiKey(accessingApiKey); | ||
| if (accessingKeyPair != null && userId == accessingKeyPair.getUserId()) { | ||
| keyPair = apiKeyPairService.findByApiKey(accessingApiKey); | ||
|
sureshanaparti marked this conversation as resolved.
Comment on lines
3204
to
3206
Member
There was a problem hiding this comment. As for this suggestion, we could apply it in this PR, if you're willing to, @sureshanaparti. But it is nitpicking IMO |
||
| } else { | ||
| keyPair = _accountService.getLatestUserKeyPair(userId); | ||
|
|
@@ -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; | ||
| } | ||
|
sureshanaparti marked this conversation as resolved.
|
||
| return isApiKeySupersetOfPermission(new ArrayList<>(getAllKeypairPermissions(accessingKeyPair.getApiKey())), new ArrayList<>(getAllKeypairPermissions(accessedKeyPair.getApiKey()))); | ||
| } | ||
|
|
||
|
|
@@ -3335,7 +3339,7 @@ public String getAccessingApiKey(BaseCmd cmd) { | |
| return accessingApiKey; | ||
| } | ||
| } catch (NullPointerException e) { | ||
| logger.info("Accessing API through session.", e); | ||
| } | ||
|
Comment on lines
+3341
to
3343
Member
There was a problem hiding this comment. This Copilot suggestion does not make sense in the context of this method |
||
| return null; | ||
|
sureshanaparti marked this conversation as resolved.
|
||
| } | ||
|
|
@@ -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); | ||
|
Comment on lines
3584
to
3588
Member
There was a problem hiding this comment. This does not need to be applied in this PR |
||
| if (apiKeyPair == null) { | ||
| logger.warn("Unable to find API key pair by API key: {}", apiKey); | ||
| return new ArrayList<>(); | ||
| } | ||
|
sureshanaparti marked this conversation as resolved.
Comment on lines
+3589
to
+3592
|
||
| Account account = _accountDao.findById(apiKeyPair.getAccountId()); | ||
| List<ApiKeyPairPermission> keyPairPermissions = keyPairManager.findAllPermissionsByKeyPairId(apiKeyPair.getId(), account.getRoleId()); | ||
| return new ArrayList<>(keyPairPermissions); | ||
|
|
@@ -3848,12 +3856,11 @@ public void buildACLViewSearchCriteria(SearchCriteria<? extends ControlledViewEn | |
| @Override | ||
| public UserAccount getUserByApiKey(String apiKey) { | ||
| ApiKeyPairVO keyPair = apiKeyPairDao.findByApiKey(apiKey); | ||
| if (keyPair != null) { | ||
| return userAccountDao.findById(keyPair.getUserId()); | ||
| } | ||
|
|
||
| return null; | ||
| } | ||
|
|
||
| @Override | ||
|
|
||
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It might be better to throw an exception here, if apikeypair is not found
similar to change with
getAllKeypairPermissionsmethodThere was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, it would be nice
Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@sureshanaparti it makes more sense to throw an exception here. Although unlikely, the way this
ifis structured right now can result in the key of user A of account X leaking when the caller is using a key to access user B of account X ifaccessingKeyPairis null.