Fix checkRoleEscalation performance and bugs in access checking#12973
Fix checkRoleEscalation performance and bugs in access checking#12973nvazquez wants to merge 15 commits intoapache:4.22from
Conversation
Replace per-API-command loop (~1,484 DB calls) with batch getApisAllowedToAccount calls using cached role permissions, reducing to ~2 DB calls (or 0 with warm cache). Fix NPE on pde.getAccount(), format string mismatch (3 %s but 4 args), and use cached permissions in DynamicRoleBasedAPIAccessChecker.checkAccess(Account, String).
…ccount Add tests for DynamicRoleBasedAPIAccessChecker covering checkAccess(Account, String) and getApisAllowedToAccount including cache verification, admin pass-through, allow/deny filtering, and annotation fallback. Add tests for AccountManagerImpl.checkRoleEscalation covering same permissions, caller superset, escalation detection, empty API list, and multi-checker chaining.
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## 4.22 #12973 +/- ##
=========================================
Coverage 17.60% 17.61%
- Complexity 15677 15691 +14
=========================================
Files 5918 5919 +1
Lines 531681 531726 +45
Branches 65005 65014 +9
=========================================
+ Hits 93623 93649 +26
- Misses 427498 427514 +16
- Partials 10560 10563 +3
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
@blueorangutan package |
|
@nvazquez a [SL] Jenkins job has been kicked to build packages. It will be bundled with KVM, XenServer and VMware SystemVM templates. I'll keep you posted as I make progress. |
|
Packaging result [SF]: ✔️ el8 ✔️ el9 ✔️ el10 ✖️ debian ✔️ suse15. SL-JID 17387 |
|
@blueorangutan test |
|
@nvazquez a [SL] Trillian-Jenkins test job (ol8 mgmt + kvm-ol8) has been kicked to run smoke tests |
|
[SF] Trillian test result (tid-15819)
|
Description
This PR fixes performance issues and bugs in AccountManagerImpl.checkRoleEscalation(Account, Account).
Performance problem: The method iterated all ~742 API commands individually, calling checkApiAccess for each command through 2 API checkers. DynamicRoleBasedAPIAccessChecker.checkAccess(Account, String) made a redundant uncached DB call on every invocation, ignoring already-cached data from
getRolePermissionsUsingCache(). Additionally, ProjectRoleBasedApiAccessChecker.checkAccess(Account, String) always returns true but was still called 742 times.
Impact before fix: ~1,484 DB calls per checkRoleEscalation invocation for non-admin roles.
Impact after fix: ~2 DB calls (or 0 with warm cache).
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?