Revert "Fix checkRoleEscalation performance and bugs in access checking" - #13881
Open
RosiKyu wants to merge 1 commit into
Open
Revert "Fix checkRoleEscalation performance and bugs in access checking"#13881RosiKyu wants to merge 1 commit into
RosiKyu wants to merge 1 commit into
Conversation
Contributor
There was a problem hiding this comment.
Pull request overview
This PR reverts prior changes from #12973 related to role escalation checks and API access checker behavior, removing cache-refresh hooks and simplifying the role escalation verification path back to per-command access checks.
Changes:
- Reverts
checkRoleEscalationto validate privileges by iteratingapiNameListand invokingcheckApiAccessper command. - Removes cache refresh/invalidation plumbing tied to role-permission mutations (and related API surface changes in
AccountManager/APIChecker). - Removes unit tests that specifically covered
checkRoleEscalation, account-level access checks, and cache behavior.
Reviewed changes
Copilot reviewed 10 out of 10 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| server/src/test/java/com/cloud/user/AccountManagerImplTest.java | Removes dedicated checkRoleEscalation unit tests. |
| server/src/main/java/org/apache/cloudstack/acl/RoleManagerImpl.java | Removes cache-refresh calls on role permission create/delete. |
| server/src/main/java/com/cloud/user/AccountManagerImpl.java | Reverts role escalation logic to per-API checkApiAccess; removes ACL-checker-specific helpers and refresh method. |
| server/src/main/java/com/cloud/user/AccountManager.java | Removes the refreshRoleCheckersCacheOnPermissionsChange API method. |
| plugins/network-elements/juniper-contrail/src/test/java/org/apache/cloudstack/network/contrail/management/MockAccountManager.java | Updates mock to match removed AccountManager method. |
| plugins/acl/static-role-based/src/main/java/org/apache/cloudstack/acl/StaticRoleBasedAPIAccessChecker.java | Removes account-level API list filtering method. |
| plugins/acl/project-role-based/src/main/java/org/apache/cloudstack/acl/ProjectRoleBasedApiAccessChecker.java | Removes account-level API list filtering method. |
| plugins/acl/dynamic-role-based/src/test/java/org/apache/cloudstack/acl/DynamicRoleBasedAPIAccessCheckerTest.java | Removes tests for account-level checks and cache behavior. |
| plugins/acl/dynamic-role-based/src/main/java/org/apache/cloudstack/acl/DynamicRoleBasedAPIAccessChecker.java | Stops using cached permissions list for account-level checkAccess (DB fetch per call). |
| api/src/main/java/org/apache/cloudstack/acl/APIChecker.java | Removes default helper methods (account API filtering, cache refresh hook, logger). |
Suppressed comments (1)
plugins/acl/dynamic-role-based/src/main/java/org/apache/cloudstack/acl/DynamicRoleBasedAPIAccessChecker.java:180
- checkAccess(Account, ...) fetches role permissions from RoleService on every call, even when cachePeriod > 0 and getRolePermissionsUsingCache(...) already provides a cached permission list. This negates the dynamic API checker cache for account-level checks and can trigger repeated DB reads when iterating many APIs (e.g., during role escalation checks).
List<RolePermission> allPermissions = roleService.findAllPermissionsBy(accountRole.getId());
if (checkApiPermissionByRole(accountRole, commandName, allPermissions)) {
return true;
}
throw new UnavailableCommandException(String.format("The API [%s] does not exist or is not available for the account %s.", commandName, account));
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Comment on lines
+1458
to
1464
| checkApiAccess(apiCheckers, caller, command); | ||
| } catch (PermissionDeniedException pde) { | ||
| String msg = String.format("User of Account %s and domain %s can not create an account with access to more privileges they have themself.", | ||
| caller, _domainMgr.getDomain(caller.getDomainId())); | ||
| logger.warn(msg); | ||
| throw new PermissionDeniedException(msg,pde); | ||
| } |
Comment on lines
392
to
396
| if (findRolePermissionByRoleIdAndRule(role.getId(), rule.toString()) != null) { | ||
| throw new PermissionDeniedException("Rule already exists for the role: " + role.getName()); | ||
| } | ||
|
|
||
| accountManager.refreshRoleCheckersCacheOnPermissionsChange(role); | ||
|
|
||
| return Transaction.execute(new TransactionCallback<RolePermissionVO>() { |
Comment on lines
1583
to
1587
| Mockito.lenient().doThrow(PermissionDeniedException.class).when(accountManagerImpl).checkRoleEscalation(callingAccount, accountMock); | ||
|
|
||
| accountManagerImpl.checkCallerApiPermissionsForUserOrAccountOperations(accountMock); | ||
| } | ||
|
|
||
| // --- Tests for checkRoleEscalation --- | ||
|
|
||
| private void setPrivateField(Object target, String fieldName, Object value) throws Exception { | ||
| Class<?> clazz = target.getClass(); | ||
| while (clazz != null) { | ||
| try { | ||
| java.lang.reflect.Field field = clazz.getDeclaredField(fieldName); | ||
| field.setAccessible(true); | ||
| field.set(target, value); | ||
| return; | ||
| } catch (NoSuchFieldException e) { | ||
| clazz = clazz.getSuperclass(); | ||
| } | ||
| } | ||
| throw new NoSuchFieldException(fieldName); | ||
| } | ||
|
|
||
| @Test | ||
| public void testCheckRoleEscalationSamePermissionsShouldPass() throws Exception { | ||
| APIChecker checker = Mockito.mock(APIChecker.class); | ||
| List<String> apis = Arrays.asList("api1", "api2", "api3"); | ||
| Mockito.when(checker.isEnabled()).thenReturn(true); | ||
|
|
||
| Account caller = Mockito.mock(Account.class); | ||
| Account requested = Mockito.mock(Account.class); | ||
|
|
||
| accountManagerImpl.setApiAccessCheckers(Arrays.asList(checker)); | ||
| setPrivateField(accountManagerImpl, "apiNameList", new ArrayList<>(apis)); | ||
|
|
||
| accountManagerImpl.checkRoleEscalation(caller, requested); | ||
| } | ||
|
|
||
| @Test | ||
| public void testCheckRoleEscalationCallerHasMorePermissionsShouldPass() throws Exception { | ||
| List<String> allApis = Arrays.asList("api1", "api2", "api3"); | ||
| List<String> requestedApis = Arrays.asList("api1", "api2"); | ||
|
|
||
| APIAclChecker checker = Mockito.mock(APIAclChecker.class); | ||
| Mockito.when(checker.isEnabled()).thenReturn(true); | ||
|
|
||
| Account caller = Mockito.mock(Account.class); | ||
| Account requested = Mockito.mock(Account.class); | ||
|
|
||
| Mockito.when(checker.getApisAllowedToAccount(Mockito.eq(requested), Mockito.anyList())).thenReturn(requestedApis); | ||
| Mockito.when(checker.getApisAllowedToAccount(Mockito.eq(caller), Mockito.anyList())).thenReturn(allApis); | ||
|
|
||
| accountManagerImpl.setApiAccessCheckers(Arrays.asList(checker)); | ||
| setPrivateField(accountManagerImpl, "apiNameList", new ArrayList<>(allApis)); | ||
|
|
||
| accountManagerImpl.checkRoleEscalation(caller, requested); | ||
| } | ||
|
|
||
| @Test(expected = PermissionDeniedException.class) | ||
| public void testCheckRoleEscalationRequestedHasMorePermissionsShouldThrow() throws Exception { | ||
| List<String> allApis = Arrays.asList("api1", "api2", "api3"); | ||
| List<String> requestedApis = Arrays.asList("api1", "api2", "api3"); | ||
| List<String> callerApis = Arrays.asList("api1"); | ||
|
|
||
| APIAclChecker checker = Mockito.mock(APIAclChecker.class); | ||
| Mockito.when(checker.isEnabled()).thenReturn(true); | ||
|
|
||
| Account caller = Mockito.mock(Account.class); | ||
| Account requested = Mockito.mock(Account.class); | ||
|
|
||
| Mockito.when(checker.getApisAllowedToAccount(Mockito.eq(requested), Mockito.anyList())).thenReturn(requestedApis); | ||
| Mockito.when(checker.getApisAllowedToAccount(Mockito.eq(caller), Mockito.anyList())).thenReturn(callerApis); | ||
|
|
||
| accountManagerImpl.setApiAccessCheckers(Arrays.asList(checker)); | ||
| setPrivateField(accountManagerImpl, "apiNameList", new ArrayList<>(allApis)); | ||
|
|
||
| accountManagerImpl.checkRoleEscalation(caller, requested); | ||
| } | ||
|
|
||
| @Test | ||
| public void testCheckRoleEscalationEmptyApiListShouldPass() throws Exception { | ||
| APIAclChecker checker = Mockito.mock(APIAclChecker.class); | ||
| Mockito.when(checker.isEnabled()).thenReturn(true); | ||
| Mockito.when(checker.getApisAllowedToAccount(Mockito.any(Account.class), Mockito.anyList())).thenReturn(Collections.emptyList()); | ||
|
|
||
| Account caller = Mockito.mock(Account.class); | ||
| Account requested = Mockito.mock(Account.class); | ||
|
|
||
| accountManagerImpl.setApiAccessCheckers(Arrays.asList(checker)); | ||
| setPrivateField(accountManagerImpl, "apiNameList", new ArrayList<>()); | ||
|
|
||
| accountManagerImpl.checkRoleEscalation(caller, requested); | ||
| } | ||
|
|
||
| @Test | ||
| public void testCheckRoleEscalationMultipleCheckersAppliedSequentially() throws Exception { | ||
| List<String> allApis = Arrays.asList("api1", "api2", "api3"); | ||
| List<String> afterChecker1 = Arrays.asList("api1", "api2"); | ||
| List<String> afterChecker2 = Arrays.asList("api1"); | ||
|
|
||
| APIAclChecker checker1 = Mockito.mock(APIAclChecker.class); | ||
| Mockito.when(checker1.isEnabled()).thenReturn(true); | ||
| APIAclChecker checker2 = Mockito.mock(APIAclChecker.class); | ||
| Mockito.when(checker2.isEnabled()).thenReturn(true); | ||
|
|
||
| Account caller = Mockito.mock(Account.class); | ||
| Account requested = Mockito.mock(Account.class); | ||
|
|
||
| // requested: checker1 filters to [api1, api2], checker2 further filters to [api1] | ||
| Mockito.when(checker1.getApisAllowedToAccount(Mockito.eq(requested), Mockito.eq(allApis))).thenReturn(afterChecker1); | ||
| Mockito.when(checker2.getApisAllowedToAccount(Mockito.eq(requested), Mockito.eq(afterChecker1))).thenReturn(afterChecker2); | ||
| // caller: same filtering, so no escalation | ||
| Mockito.when(checker1.getApisAllowedToAccount(Mockito.eq(caller), Mockito.eq(afterChecker2))).thenReturn(afterChecker2); | ||
| Mockito.when(checker2.getApisAllowedToAccount(Mockito.eq(caller), Mockito.eq(afterChecker2))).thenReturn(afterChecker2); | ||
|
|
||
| accountManagerImpl.setApiAccessCheckers(Arrays.asList(checker1, checker2)); | ||
| setPrivateField(accountManagerImpl, "apiNameList", new ArrayList<>(allApis)); | ||
|
|
||
| accountManagerImpl.checkRoleEscalation(caller, requested); | ||
| } | ||
| } |
Comment on lines
193
to
198
| Mockito.doReturn(Collections.singletonList(permission)).when(roleServiceMock).findAllPermissionsBy(Mockito.anyLong()); | ||
|
|
||
| List<String> apisReceived = apiAccessCheckerSpy.getApisAllowedToUser(getTestRole(), getTestUser(), apiNames); | ||
| Assert.assertEquals(0, apisReceived.size()); | ||
| } | ||
|
|
||
| // --- Tests for checkAccess(Account, String) --- | ||
|
|
||
| @Test(expected = PermissionDeniedException.class) | ||
| public void testCheckAccessAccountNullRoleShouldThrow() { | ||
| Mockito.when(roleServiceMock.findRole(Mockito.anyLong())).thenReturn(null); | ||
| apiAccessCheckerSpy.checkAccess(getTestAccount(), "someApi"); | ||
| } | ||
|
|
||
| @Test | ||
| public void testCheckAccessAccountAdminShouldAllow() { | ||
| Account adminAccount = new AccountVO("root admin", 1L, null, Account.Type.ADMIN, "admin-uuid"); | ||
| Mockito.when(roleServiceMock.findRole(Mockito.anyLong())).thenReturn(new RoleVO(1L, "Admin", RoleType.Admin, "default admin role")); | ||
| assertTrue(apiAccessCheckerSpy.checkAccess(adminAccount, "anyApi")); | ||
| } | ||
|
|
||
| @Test | ||
| public void testCheckAccessAccountAllowedApi() { | ||
| final String allowedApiName = "someAllowedApi"; | ||
| final RolePermission permission = new RolePermissionVO(1L, allowedApiName, Permission.ALLOW, null); | ||
| Mockito.when(roleServiceMock.findAllPermissionsBy(Mockito.anyLong())).thenReturn(Collections.singletonList(permission)); | ||
| assertTrue(apiAccessCheckerSpy.checkAccess(getTestAccount(), allowedApiName)); | ||
| } | ||
|
|
||
| @Test(expected = PermissionDeniedException.class) | ||
| public void testCheckAccessAccountDeniedApi() { | ||
| final String deniedApiName = "someDeniedApi"; | ||
| final RolePermission permission = new RolePermissionVO(1L, deniedApiName, Permission.DENY, null); | ||
| Mockito.when(roleServiceMock.findAllPermissionsBy(Mockito.anyLong())).thenReturn(Collections.singletonList(permission)); | ||
| apiAccessCheckerSpy.checkAccess(getTestAccount(), deniedApiName); | ||
| } | ||
|
|
||
| @Test | ||
| public void testCheckAccessAccountUsesCachedPermissions() throws Exception { | ||
| // Enable caching by setting a positive cachePeriod | ||
| Field cachePeriodField = DynamicRoleBasedAPIAccessChecker.class.getDeclaredField("cachePeriod"); | ||
| cachePeriodField.setAccessible(true); | ||
| cachePeriodField.set(apiAccessCheckerSpy, 1); | ||
|
|
||
| Field rpCacheField = DynamicRoleBasedAPIAccessChecker.class.getDeclaredField("rolePermissionsCache"); | ||
| rpCacheField.setAccessible(true); | ||
| rpCacheField.set(apiAccessCheckerSpy, new LazyCache<Long, Pair<Role, List<RolePermission>>>(32, 1, apiAccessCheckerSpy::getRolePermissions)); | ||
|
|
||
| final String allowedApiName = "someAllowedApi"; | ||
| final RolePermission permission = new RolePermissionVO(1L, allowedApiName, Permission.ALLOW, null); | ||
| Mockito.when(roleServiceMock.findAllPermissionsBy(Mockito.anyLong())).thenReturn(Collections.singletonList(permission)); | ||
|
|
||
| // First call should populate the cache | ||
| apiAccessCheckerSpy.checkAccess(getTestAccount(), allowedApiName); | ||
| // Second call should use cached permissions and not hit the DAO again | ||
| apiAccessCheckerSpy.checkAccess(getTestAccount(), allowedApiName); | ||
|
|
||
| Mockito.verify(roleServiceMock, Mockito.times(1)).findAllPermissionsBy(Mockito.anyLong()); | ||
| } | ||
|
|
||
| // --- Tests for getApisAllowedToAccount --- | ||
|
|
||
| @Test | ||
| public void testGetApisAllowedToAccountDisabledShouldReturnAll() { | ||
| Mockito.doReturn(false).when(apiAccessCheckerSpy).isEnabled(); | ||
| List<String> input = new ArrayList<>(Arrays.asList("api1", "api2", "api3")); | ||
| List<String> result = apiAccessCheckerSpy.getApisAllowedToAccount(getTestAccount(), input); | ||
| Assert.assertEquals(3, result.size()); | ||
| } | ||
|
|
||
| @Test(expected = PermissionDeniedException.class) | ||
| public void testGetApisAllowedToAccountNullRoleShouldThrow() { | ||
| Mockito.when(roleServiceMock.findRole(Mockito.anyLong())).thenReturn(null); | ||
| apiAccessCheckerSpy.getApisAllowedToAccount(getTestAccount(), new ArrayList<>(Arrays.asList("api1"))); | ||
| } | ||
|
|
||
| @Test | ||
| public void testGetApisAllowedToAccountAdminShouldReturnAll() { | ||
| Account adminAccount = new AccountVO("root admin", 1L, null, Account.Type.ADMIN, "admin-uuid"); | ||
| Mockito.when(roleServiceMock.findRole(Mockito.anyLong())).thenReturn(new RoleVO(1L, "Admin", RoleType.Admin, "default admin role")); | ||
| List<String> input = new ArrayList<>(Arrays.asList("api1", "api2", "api3")); | ||
| List<String> result = apiAccessCheckerSpy.getApisAllowedToAccount(adminAccount, input); | ||
| Assert.assertEquals(3, result.size()); | ||
| Assert.assertEquals(input, result); | ||
| } | ||
|
|
||
| @Test | ||
| public void testGetApisAllowedToAccountFiltersCorrectly() { | ||
| final RolePermission allowPermission = new RolePermissionVO(1L, "allowedApi", Permission.ALLOW, null); | ||
| final RolePermission denyPermission = new RolePermissionVO(1L, "deniedApi", Permission.DENY, null); | ||
| Mockito.when(roleServiceMock.findAllPermissionsBy(Mockito.anyLong())).thenReturn(Arrays.asList(allowPermission, denyPermission)); | ||
| List<String> input = new ArrayList<>(Arrays.asList("allowedApi", "deniedApi", "unknownApi")); | ||
| List<String> result = apiAccessCheckerSpy.getApisAllowedToAccount(getTestAccount(), input); | ||
| Assert.assertEquals(1, result.size()); | ||
| Assert.assertEquals("allowedApi", result.get(0)); | ||
| } | ||
|
|
||
| @Test | ||
| public void testGetApisAllowedToAccountAnnotationFallback() { | ||
| Mockito.when(roleServiceMock.findAllPermissionsBy(Mockito.anyLong())).thenReturn(Collections.emptyList()); | ||
| apiAccessCheckerSpy.addApiToRoleBasedAnnotationsMap(RoleType.User, "annotatedApi"); | ||
| List<String> input = new ArrayList<>(Arrays.asList("annotatedApi", "unknownApi")); | ||
| List<String> result = apiAccessCheckerSpy.getApisAllowedToAccount(getTestAccount(), input); | ||
| Assert.assertEquals(1, result.size()); | ||
| Assert.assertEquals("annotatedApi", result.get(0)); | ||
| } | ||
|
|
||
| @Test | ||
| public void testGetApisAllowedToAccountUsesCachedPermissions() { | ||
| try { | ||
| // Ensure caching is enabled by setting a positive cachePeriod | ||
| Field cachePeriodField = DynamicRoleBasedAPIAccessChecker.class.getDeclaredField("cachePeriod"); | ||
| cachePeriodField.setAccessible(true); | ||
| cachePeriodField.set(apiAccessCheckerSpy, 1); | ||
|
|
||
| Field rpCacheField = DynamicRoleBasedAPIAccessChecker.class.getDeclaredField("rolePermissionsCache"); | ||
| rpCacheField.setAccessible(true); | ||
| rpCacheField.set(apiAccessCheckerSpy, new LazyCache<Long, Pair<Role, List<RolePermission>>>(32, 1, apiAccessCheckerSpy::getRolePermissions)); | ||
|
|
||
| final RolePermission permission = new RolePermissionVO(1L, "api1", Permission.ALLOW, null); | ||
| Mockito.when(roleServiceMock.findAllPermissionsBy(Mockito.anyLong())).thenReturn(Collections.singletonList(permission)); | ||
|
|
||
| Account account = getTestAccount(); | ||
| List<String> apis = new ArrayList<>(Arrays.asList("api1")); | ||
|
|
||
| // First call should load permissions from the DAO and populate the cache | ||
| apiAccessCheckerSpy.getApisAllowedToAccount(account, apis); | ||
| // Second call should use cached permissions and not hit the DAO again | ||
| apiAccessCheckerSpy.getApisAllowedToAccount(account, apis); | ||
|
|
||
| Mockito.verify(roleServiceMock, Mockito.times(1)).findAllPermissionsBy(Mockito.anyLong()); | ||
| } catch (NoSuchFieldException | IllegalAccessException e) { | ||
| Assert.fail("Failed to set cachePeriod for test: " + e.getMessage()); | ||
| } | ||
| } | ||
| } |
Comment on lines
+1438
to
1451
| try { | ||
| checkApiAccess(apiCheckers, requested, command); | ||
| } catch (PermissionDeniedException pde) { | ||
| if (logger.isTraceEnabled()) { | ||
| logger.trace(String.format( | ||
| "Checking for permission to \"%s\" is irrelevant as it is not requested for %s [%s]", | ||
| command, | ||
| requested.getAccountName(), | ||
| requested.getUuid() | ||
| ) | ||
| ); | ||
| } | ||
| continue; | ||
| } |
nvazquez
approved these changes
Aug 14, 2026
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## 4.22 #13881 +/- ##
============================================
- Coverage 17.71% 17.70% -0.02%
+ Complexity 15858 15836 -22
============================================
Files 5926 5925 -1
Lines 533613 533539 -74
Branches 65285 65274 -11
============================================
- Hits 94517 94444 -73
- Misses 428415 428419 +4
+ Partials 10681 10676 -5
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:
|
|
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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
Reverts #12973