Fix NPE with ApiKeyPair during listApis call (from cmk)#13149
Fix NPE with ApiKeyPair during listApis call (from cmk)#13149sureshanaparti wants to merge 3 commits into
Conversation
|
@KlausDornsbach can you review this. |
|
@blueorangutan package |
|
@sureshanaparti a [SL] Jenkins job has been kicked to build packages. It will be bundled with no SystemVM templates. I'll keep you posted as I make progress. |
Codecov Report✅ All modified and coverable lines are covered by tests.
Additional details and impacted files@@ Coverage Diff @@
## main #13149 +/- ##
=============================================
- Coverage 18.08% 3.46% -14.63%
=============================================
Files 6037 479 -5558
Lines 542611 41162 -501449
Branches 66433 7793 -58640
=============================================
- Hits 98136 1426 -96710
+ Misses 433448 39543 -393905
+ Partials 11027 193 -10834
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
|
Packaging result [SF]: ✔️ el8 ✔️ el9 ✔️ el10 ✔️ debian ✔️ suse15. SL-JID 17820 |
@sureshanaparti, @KlausDornsbach is not active in the community anymore. I am asking @bernardodemarco to review this, as he was responsible for maintaning the keypairs PR. |
|
@sureshanaparti, thanks for the PR! I'll try to review and test it during this week. |
|
@blueorangutan test |
|
@blueorangutan test |
|
@sureshanaparti a [SL] Trillian-Jenkins test job (ol8 mgmt + kvm-ol8) has been kicked to run smoke tests |
|
[SF] Trillian test result (tid-16105)
|
|
This pull request has merge conflicts. Dear author, please fix the conflicts and sync your branch with the base branch. |
kiranchavala
left a comment
kiranchavala
left a comment
There was a problem hiding this comment.
Getting the following error on ol8
(localcloud) 🐱 > sync
panic: interface conversion: interface {} is nil, not []interface {}
goroutine 1 [running]:
github.com/apache/cloudstack-cloudmonkey/config.(*Config).UpdateCache(0xc0000e0120?, 0xc0013b0390)
/home/shwstppr/lab/shapeblue/cloudstack-cloudmonkey/config/cache.go:115 +0x1338
github.com/apache/cloudstack-cloudmonkey/cmd.init.4.func1(0xc0011ab740)
/home/shwstppr/lab/shapeblue/cloudstack-cloudmonkey/cmd/sync.go:35 +0xd0
github.com/apache/cloudstack-cloudmonkey/cli.ExecCmd({0xc001330d30, 0x1, 0x1}, 0x0)
/home/shwstppr/lab/shapeblue/cloudstack-cloudmonkey/cli/exec.go:64 +0x1bb
github.com/apache/cloudstack-cloudmonkey/cli.ExecLine({0xc00133d420, 0x4})
/home/shwstppr/lab/shapeblue/cloudstack-cloudmonkey/cli/exec.go:51 +0x21b
github.com/apache/cloudstack-cloudmonkey/cli.ExecPrompt()
/home/shwstppr/lab/shapeblue/cloudstack-cloudmonkey/cli/prompt.go:84 +0x26e
main.main()
/home/shwstppr/lab/shapeblue/cloudstack-cloudmonkey/cmk.go:101 +0x7f1
@kiranchavala no user with that api key, re-check with valid key. cmk also need a fix to address this error: (created issue: apache/cloudstack-cloudmonkey#211) |
sureshanaparti
force-pushed
the
api-keypair-npe-fix
branch
from
492b5d1 to
aa6f16b
Compare
|
@blueorangutan package |
|
@sureshanaparti a [SL] Jenkins job has been kicked to build packages. It will be bundled with no SystemVM templates. I'll keep you posted as I make progress. |
|
Packaging result [SF]: ✔️ el8 ✔️ el9 ✔️ el10 ✔️ debian ✔️ suse15. SL-JID 17930 |
|
@blueorangutan test |
|
@DaanHoogland a [SL] Trillian-Jenkins test job (ol8 mgmt + kvm-ol8) has been kicked to run smoke tests |
|
[SF] Trillian test result (tid-16144)
|
kiranchavala
left a comment
kiranchavala
left a comment
There was a problem hiding this comment.
LGTM
generatated the keys for a user > configured in cmk > sync
Able to execute the api calls
I don’t think these are related, up to you @winterhazel @weizhouapache |
| 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; |
There was a problem hiding this comment.
I did not check the whole workflow, I am not sure if it is fine to return true here
it would be better others can review and evaluate the side-effects
There was a problem hiding this comment.
I think that it is better to return false here instead. Could you have a look @bernardodemarco?
This method validates whether the keypair being used is a superset of the keypair being accessed in order to prevent a key with less privileges to get keys with more privileges. true is returned when an apiKey is not being used to call the API, which makes sense as this check is not necessary in this case. However, as a keypair is being used here, I think that it would make more sense to either inform that it is not a superset, as we could not obtain the key to confirm, or throw a runtime exception.
There was a problem hiding this comment.
@weizhouapache @winterhazel great catch guys... you are correct, we should return Boolean.FALSE here.
When I first reviewed the PR, I had not paid too much attention and I had inferred that we would have to return Boolean.TRUE because of the first conditional statement. But, as Fabricio has explained here https://github.com/apache/cloudstack/pull/13149/changes#r3320265882, the first conditional structure checks whether the API call is performed via session key.
There was a problem hiding this comment.
@winterhazel @bernardodemarco @weizhouapache sorry for the delay (not active here for sometime) agree with it, updated to return false here.
I followed the same earlier approach as apiKey is null, should we return false when apiKey is null as well?
There was a problem hiding this 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
bernardodemarco
left a comment
bernardodemarco
left a comment
There was a problem hiding this comment.
@sureshanaparti, I think that we only need to adjust this topic https://github.com/apache/cloudstack/pull/13149/changes#r3287789004. Then, the PR will be ready to go
|
@sureshanaparti could you have a look at #13149 (comment)? |
winterhazel
added
status:waiting-for-author
and removed
status:ready-for-merge
status:waiting-for-smoketest-results
labels
@sureshanaparti ping. It would be nice to include this fix in 4.23 |
|
@blueorangutan package |
There was a problem hiding this comment.
Pull request overview
This PR addresses a NullPointerException when listApis is called with an API key that does not resolve to an ApiKeyPair, by adding null-handling in API key pair lookup paths within AccountManagerImpl.
Changes:
- Add null checks when resolving an accessing
ApiKeyPairto prevent NPEs. - Handle missing
ApiKeyPairresults in permission-resolution logic and improve related logging. - Fix
getUserByApiKeyto avoid dereferencing a null key pair.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
@blueorangutan package |
|
@sureshanaparti a [SL] Jenkins job has been kicked to build packages. It will be bundled with no SystemVM templates. I'll keep you posted as I make progress. |
| ApiKeyPair accessingKeyPair = apiKeyPairService.findByApiKey(accessingApiKey); | ||
| if (userId == accessingKeyPair.getUserId()) { | ||
| if (accessingKeyPair != null && userId == accessingKeyPair.getUserId()) { | ||
| keyPair = apiKeyPairService.findByApiKey(accessingApiKey); |
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
| } catch (Exception e) { | ||
| logger.info("Error accessing API through session.", e); | ||
| } |
There was a problem hiding this comment.
This Copilot suggestion does not make sense in the context of this method
| if (apiKeyPair == null) { | ||
| logger.warn("Unable to find API key pair by API key."); | ||
| return new ArrayList<>(); | ||
| } |
| public List<RolePermissionEntity> getAllKeypairPermissions(String apiKey) { | ||
| if (apiKey == null) { | ||
| 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); |
There was a problem hiding this comment.
This does not need to be applied in this PR
|
Packaging result [SF]: ✔️ el8 ✔️ el9 ✔️ el10 ✖️ debian ✔️ suse15. SL-JID 18495 |
| if (accessingApiKey != null) { | ||
| ApiKeyPair accessingKeyPair = apiKeyPairService.findByApiKey(accessingApiKey); | ||
| if (userId == accessingKeyPair.getUserId()) { | ||
| if (accessingKeyPair != null && userId == accessingKeyPair.getUserId()) { |
There was a problem hiding this comment.
It might be better to throw an exception here, if apikeypair is not found
similar to change with getAllKeypairPermissions method
| } | ||
| ApiKeyPair apiKeyPair = keyPairManager.findByApiKey(apiKey); | ||
| if (apiKeyPair == null) { | ||
| logger.warn("Unable to find API key pair by API key."); |
|
@sureshanaparti |
|
@winterhazel, before merging this, could you review it again, please? |
Description
This PR fixes NPE issue with ApiKeyPair during listApis call (from cmk).
Types of changes
Feature/Enhancement Scale or Bug Severity
Feature/Enhancement Scale
Bug Severity
Screenshots (if appropriate):
How Has This Been Tested?
How did you try to break this feature and the system with this change?