Copilot commented on code in PR #13881:
URL: https://github.com/apache/cloudstack/pull/13881#discussion_r3787562764
##########
server/src/main/java/com/cloud/user/AccountManagerImpl.java:
##########
@@ -1434,34 +1433,35 @@ protected void checkRoleEscalation(Account caller,
Account requested) {
requested.getUuid(),
requested.getRoleId()));
}
-
- List<APIAclChecker> aclCheckers = getApiACLCheckers();
-
- List<String> allApis = new ArrayList<>(apiNameList);
- List<String> requestedAllowed = allApis;
- List<String> callerAllowed = new ArrayList<>();
- try {
- for (final APIAclChecker apiChecker : aclCheckers) {
- requestedAllowed =
apiChecker.getApisAllowedToAccount(requested, requestedAllowed);
+ List<APIChecker> apiCheckers = getEnabledApiCheckers();
+ for (String command : apiNameList) {
+ 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;
}
- callerAllowed = requestedAllowed;
- for (final APIAclChecker apiChecker : aclCheckers) {
- callerAllowed = apiChecker.getApisAllowedToAccount(caller,
callerAllowed);
+ // so requested can, now make sure caller can as well
+ try {
+ if (logger.isTraceEnabled()) {
+ logger.trace(String.format("permission to \"%s\" is
requested",
+ command));
+ }
+ 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);
}
Review Comment:
When the caller lacks a requested permission, the thrown message has grammar
issues ("can not", "they have themself") and it also wraps
RequestLimitException as a generic PermissionDeniedException, obscuring the
real failure mode (rate limiting). It’s better to propagate
RequestLimitException unchanged and keep the escalation message clear.
##########
plugins/acl/dynamic-role-based/src/test/java/org/apache/cloudstack/acl/DynamicRoleBasedAPIAccessCheckerTest.java:
##########
@@ -197,134 +195,4 @@ public void
getApisAllowedToUserTestPermissionDenyForGivenApiShouldReturnEmptyLi
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());
- }
- }
}
Review Comment:
Tests for DynamicRoleBasedAPIAccessChecker.checkAccess(Account, ...) and
cache behavior were removed, so account-level access enforcement and permission
caching/refresh semantics are no longer covered by unit tests.
##########
server/src/test/java/com/cloud/user/AccountManagerImplTest.java:
##########
@@ -1587,119 +1584,4 @@ public void
testcheckCallerApiPermissionsForUserOperationsNotAllowedApis() {
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);
- }
}
Review Comment:
The dedicated unit tests for AccountManagerImpl.checkRoleEscalation were
removed, leaving this security-critical escalation behavior untested. Existing
tests in this file only stub checkRoleEscalation rather than exercising it.
##########
server/src/main/java/org/apache/cloudstack/acl/RoleManagerImpl.java:
##########
@@ -393,8 +393,6 @@ public RolePermission createRolePermission(final Role role,
final Rule rule, fin
throw new PermissionDeniedException("Rule already exists for the
role: " + role.getName());
}
- accountManager.refreshRoleCheckersCacheOnPermissionsChange(role);
-
return Transaction.execute(new TransactionCallback<RolePermissionVO>()
{
Review Comment:
Role permission create/delete no longer triggers any invalidation/refresh of
API checker caches. If DynamicRoleBasedAPIAccessChecker caching is enabled
(dynamic.apichecker.cache.period > 0), permission changes may not take effect
until cache expiry for user-based checks that use rolePermissionsCache.
##########
server/src/main/java/com/cloud/user/AccountManagerImpl.java:
##########
@@ -1434,34 +1433,35 @@ protected void checkRoleEscalation(Account caller,
Account requested) {
requested.getUuid(),
requested.getRoleId()));
}
-
- List<APIAclChecker> aclCheckers = getApiACLCheckers();
-
- List<String> allApis = new ArrayList<>(apiNameList);
- List<String> requestedAllowed = allApis;
- List<String> callerAllowed = new ArrayList<>();
- try {
- for (final APIAclChecker apiChecker : aclCheckers) {
- requestedAllowed =
apiChecker.getApisAllowedToAccount(requested, requestedAllowed);
+ List<APIChecker> apiCheckers = getEnabledApiCheckers();
+ for (String command : apiNameList) {
+ 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;
}
Review Comment:
checkRoleEscalation currently treats any PermissionDeniedException while
checking the *requested* account as "irrelevant" and continues. This also
swallows RequestLimitException (it extends PermissionDeniedException), which
means rate-limiting can silently bypass the escalation verification instead of
failing the operation.
--
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]
