Limit listRoles API visibility (#8639)

Co-authored-by: Henrique Sato <henrique.sato@scclouds.com.br>

plugins/network-elements/juniper-contrail/src/test/java/org/apache/cloudstack/network/contrail/management/MockAccountManager.java

@@ -346,6 +346,11 @@ public class MockAccountManager extends ManagerBase implements AccountManager {
         return null;
     }
 
+    @Override
+    public List<String> getApiNameList() {
+        return null;
+    }
+
     @Override
     public boolean deleteUserAccount(long arg0) {
         // TODO Auto-generated method stub

server/src/main/java/com/cloud/user/AccountManager.java

@@ -195,6 +195,8 @@ public interface AccountManager extends AccountService, Configurable {
     UserTwoFactorAuthenticator getUserTwoFactorAuthenticator(final Long domainId, final Long userAccountId);
 
     void verifyUsingTwoFactorAuthenticationCode(String code, Long domainId, Long userAccountId);
+
     UserTwoFactorAuthenticationSetupResponse setupUserTwoFactorAuthentication(SetupUserTwoFactorAuthenticationCmd cmd);
 
+    List<String> getApiNameList();
 }

server/src/main/java/com/cloud/user/AccountManagerImpl.java

@@ -425,6 +425,11 @@ public class AccountManagerImpl extends ManagerBase implements AccountManager, M
         _querySelectors = querySelectors;
     }
 
+    @Override
+    public List<String> getApiNameList() {
+        return apiNameList;
+    }
+
     @Override
     public boolean configure(final String name, final Map<String, Object> params) throws ConfigurationException {
         _systemAccount = _accountDao.findById(Account.ACCOUNT_ID_SYSTEM);

server/src/main/java/org/apache/cloudstack/acl/RoleManagerImpl.java

@@ -18,9 +18,12 @@ package org.apache.cloudstack.acl;
 
 import java.util.ArrayList;
 import java.util.Collections;
+import java.util.HashMap;
+import java.util.HashSet;
 import java.util.Iterator;
 import java.util.List;
 import java.util.Map;
+import java.util.Set;
 
 import javax.inject.Inject;
 
@@ -405,40 +408,98 @@ public class RoleManagerImpl extends ManagerBase implements RoleService, Configu
     public Pair<List<Role>, Integer> findRolesByName(String name, String keyword, Long startIndex, Long limit) {
         if (StringUtils.isNotBlank(name) || StringUtils.isNotBlank(keyword)) {
             Pair<List<RoleVO>, Integer> data = roleDao.findAllByName(name, keyword, startIndex, limit, isCallerRootAdmin());
-            int removed = removeRootAdminRolesIfNeeded(data.first());
+            int removed = removeRolesIfNeeded(data.first());
             return new Pair<List<Role>,Integer>(ListUtils.toListOfInterface(data.first()), Integer.valueOf(data.second() - removed));
         }
         return new Pair<List<Role>, Integer>(new ArrayList<Role>(), 0);
     }
 
     /**
-     *  Removes roles of the given list that have the type '{@link RoleType#Admin}' if the user calling the method is not a 'root admin'.
+     *  Removes roles from the given list if the role has different or more permissions than the user's calling the method role
-     *  The actual removal is executed via {@link #removeRootAdminRoles(List)}. Therefore, if the method is called by a 'root admin', we do nothing here.
      */
-    protected int removeRootAdminRolesIfNeeded(List<? extends Role> roles) {
+    protected int removeRolesIfNeeded(List<? extends Role> roles) {
-        if (!isCallerRootAdmin()) {
+        if (roles.isEmpty()) {
-            return removeRootAdminRoles(roles);
+            return 0;
         }
-        return 0;
+
+        Long callerRoleId = getCurrentAccount().getRoleId();
+        Map<String, Permission> callerRolePermissions = getRoleRulesAndPermissions(callerRoleId);
+
+        int count = 0;
+        Iterator<? extends Role> rolesIterator = roles.iterator();
+        while (rolesIterator.hasNext()) {
+            Role role = rolesIterator.next();
+
+            if (role.getId() == callerRoleId || roleHasPermission(callerRolePermissions, role)) {
+                continue;
+            }
+
+            count++;
+            rolesIterator.remove();
+        }
+
+        return count;
     }
 
     /**
-     * Remove all roles that have the {@link RoleType#Admin}.
+     * Checks if the role of the caller account has compatible permissions of the specified role.
+     * For each permission of the role of the caller, the target role needs to contain the same permission.
+     *
+     * @param sourceRolePermissions the permissions of the caller role.
+     * @param targetRole the role that the caller role wants to access.
+     * @return True if the role can be accessed with the given permissions; false otherwise.
      */
-    protected int removeRootAdminRoles(List<? extends Role> roles) {
+    protected boolean roleHasPermission(Map<String, Permission> sourceRolePermissions, Role targetRole) {
-        if (CollectionUtils.isEmpty(roles)) {
+        Set<String> rulesAlreadyCompared = new HashSet<>();
-            return 0;
+        for (RolePermission rolePermission : findAllPermissionsBy(targetRole.getId())) {
-        }
+            boolean permissionIsRegex = rolePermission.getRule().getRuleString().contains("*");
-        Iterator<? extends Role> rolesIterator = roles.iterator();
+
-        int count = 0;
+            for (String apiName : accountManager.getApiNameList()) {
-        while (rolesIterator.hasNext()) {
+                if (!rolePermission.getRule().matches(apiName) || rulesAlreadyCompared.contains(apiName)) {
-            Role role = rolesIterator.next();
+                    continue;
-            if (RoleType.Admin == role.getRoleType()) {
+                }
-                count++;
+
-                rolesIterator.remove();
+                if (rolePermission.getPermission() == Permission.ALLOW && (!sourceRolePermissions.containsKey(apiName) || sourceRolePermissions.get(apiName) == Permission.DENY)) {
+                    return false;
+                }
+
+                rulesAlreadyCompared.add(apiName);
+
+                if (!permissionIsRegex) {
+                    break;
+                }
             }
         }
-        return count;
+
+        return true;
+    }
+
+    /**
+     * Given a role ID, returns a {@link Map} containing the API name as the key and the {@link Permission} for the API as the value.
+     *
+     * @param roleId ID from role.
+     */
+    public Map<String, Permission> getRoleRulesAndPermissions(Long roleId) {
+        Map<String, Permission> roleRulesAndPermissions = new HashMap<>();
+
+        for (RolePermission rolePermission : findAllPermissionsBy(roleId)) {
+            boolean permissionIsRegex = rolePermission.getRule().getRuleString().contains("*");
+
+            for (String apiName : accountManager.getApiNameList()) {
+                if (!rolePermission.getRule().matches(apiName)) {
+                    continue;
+                }
+
+                if (!roleRulesAndPermissions.containsKey(apiName)) {
+                    roleRulesAndPermissions.put(apiName, rolePermission.getPermission());
+                }
+
+                if (!permissionIsRegex) {
+                    break;
+                }
+            }
+        }
+        return roleRulesAndPermissions;
     }
 
     @Override
@@ -458,14 +519,14 @@ public class RoleManagerImpl extends ManagerBase implements RoleService, Configu
     @Override
     public List<Role> listRoles() {
         List<? extends Role> roles = roleDao.listAll();
-        removeRootAdminRolesIfNeeded(roles);
+        removeRolesIfNeeded(roles);
         return ListUtils.toListOfInterface(roles);
     }
 
     @Override
     public Pair<List<Role>, Integer> listRoles(Long startIndex, Long limit) {
         Pair<List<RoleVO>, Integer> data = roleDao.listAllRoles(startIndex, limit, isCallerRootAdmin());
-        int removed = removeRootAdminRolesIfNeeded(data.first());
+        int removed = removeRolesIfNeeded(data.first());
         return new Pair<List<Role>,Integer>(ListUtils.toListOfInterface(data.first()), Integer.valueOf(data.second() - removed));
     }
 

server/src/test/java/com/cloud/user/MockAccountManagerImpl.java

@@ -479,4 +479,8 @@ public class MockAccountManagerImpl extends ManagerBase implements Manager, Acco
         return null;
     }
 
+    @Override
+    public List<String> getApiNameList() {
+        return null;
+    }
 }

server/src/test/java/org/apache/cloudstack/acl/RoleManagerImplTest.java

@@ -20,7 +20,10 @@ package org.apache.cloudstack.acl;
 import com.cloud.user.Account;
 import com.cloud.user.AccountManager;
 import com.cloud.utils.Pair;
+
+import org.apache.cloudstack.acl.RolePermissionEntity.Permission;
 import org.apache.cloudstack.acl.dao.RoleDao;
+import org.apache.cloudstack.acl.dao.RolePermissionsDao;
 import org.apache.commons.collections.CollectionUtils;
 import org.junit.Assert;
 import org.junit.Before;
@@ -33,7 +36,10 @@ import org.mockito.Spy;
 import org.mockito.junit.MockitoJUnitRunner;
 
 import java.util.ArrayList;
+import java.util.Collections;
+import java.util.HashMap;
 import java.util.List;
+import java.util.Map;
 
 @RunWith(MockitoJUnitRunner.class)
 public class RoleManagerImplTest {
@@ -45,6 +51,22 @@ public class RoleManagerImplTest {
     private AccountManager accountManagerMock;
     @Mock
     private RoleDao roleDaoMock;
+    @Mock
+    private RolePermissionsDao rolePermissionsDaoMock;
+    @Mock
+    private RolePermission rolePermission1Mock;
+    @Mock
+    private RolePermission rolePermission2Mock;
+    @Mock
+    private Account callerAccountMock;
+    @Mock
+    private Role callerAccountRoleMock;
+    @Mock
+    private Role lessPermissionsRoleMock;
+    @Mock
+    private Role morePermissionsRoleMock;
+    @Mock
+    private Role differentPermissionsRoleMock;
 
     @Mock
     private Account accountMock;
@@ -54,6 +76,33 @@ public class RoleManagerImplTest {
     private RoleVO roleVoMock;
     private long roleMockId = 1l;
 
+    private Map<String, Permission> rolePermissions = new HashMap<>();
+
+    public void setUpRoleVisibilityTests() {
+        Mockito.doReturn(List.of("api1", "api2", "api3")).when(accountManagerMock).getApiNameList();
+
+        Mockito.doReturn(1L).when(callerAccountRoleMock).getId();
+        Mockito.doReturn(callerAccountMock).when(roleManagerImpl).getCurrentAccount();
+        Mockito.doReturn(callerAccountRoleMock.getId()).when(callerAccountMock).getRoleId();
+
+        Mockito.when(rolePermission1Mock.getRule()).thenReturn(new Rule("api1"));
+        Mockito.when(rolePermission2Mock.getRule()).thenReturn(new Rule("api2"));
+        Mockito.doReturn(RolePermissionEntity.Permission.ALLOW).when(rolePermission1Mock).getPermission();
+        Mockito.doReturn(RolePermissionEntity.Permission.ALLOW).when(rolePermission2Mock).getPermission();
+
+        List<RolePermission> lessPermissionsRolePermissions = Collections.singletonList(rolePermission1Mock);
+        Mockito.doReturn(1L).when(lessPermissionsRoleMock).getId();
+        Mockito.when(roleManagerImpl.findAllPermissionsBy(1L)).thenReturn(lessPermissionsRolePermissions);
+
+        List<RolePermission> morePermissionsRolePermissions = List.of(rolePermission1Mock, rolePermission2Mock);
+        Mockito.doReturn(2L).when(morePermissionsRoleMock).getId();
+        Mockito.when(roleManagerImpl.findAllPermissionsBy(morePermissionsRoleMock.getId())).thenReturn(morePermissionsRolePermissions);
+
+        List<RolePermission> differentPermissionsRolePermissions = Collections.singletonList(rolePermission2Mock);
+        Mockito.doReturn(3L).when(differentPermissionsRoleMock).getId();
+        Mockito.when(roleManagerImpl.findAllPermissionsBy(differentPermissionsRoleMock.getId())).thenReturn(differentPermissionsRolePermissions);
+    }
+
     @Before
     public void beforeTest() {
         Mockito.doReturn(accountMockId).when(accountMock).getId();
@@ -168,57 +217,104 @@ public class RoleManagerImplTest {
         Mockito.doReturn(toBeReturned).when(roleDaoMock).findAllByName(roleName, null, null, null, false);
 
         roleManagerImpl.findRolesByName(roleName);
-        Mockito.verify(roleManagerImpl).removeRootAdminRolesIfNeeded(roles);
+        Mockito.verify(roleManagerImpl).removeRolesIfNeeded(roles);
     }
 
     @Test
-    public void removeRootAdminRolesIfNeededTestRootAdmin() {
+    public void removeRolesIfNeededTestRoleWithMoreAndSamePermissionsKeepRoles() {
-        Mockito.doReturn(accountMock).when(roleManagerImpl).getCurrentAccount();
+        setUpRoleVisibilityTests();
-        Mockito.doReturn(true).when(accountManagerMock).isRootAdmin(accountMockId);
-
         List<Role> roles = new ArrayList<>();
-        roleManagerImpl.removeRootAdminRolesIfNeeded(roles);
 
-        Mockito.verify(roleManagerImpl, Mockito.times(0)).removeRootAdminRoles(roles);
+        List<RolePermission> callerAccountRolePermissions = List.of(rolePermission1Mock, rolePermission2Mock);
+        Mockito.when(roleManagerImpl.findAllPermissionsBy(callerAccountRoleMock.getId())).thenReturn(callerAccountRolePermissions);
+
+        roles.add(callerAccountRoleMock);
+        roles.add(lessPermissionsRoleMock);
+
+        roleManagerImpl.removeRolesIfNeeded(roles);
+
+        Assert.assertEquals(2, roles.size());
+        Assert.assertEquals(callerAccountRoleMock, roles.get(0));
+        Assert.assertEquals(lessPermissionsRoleMock, roles.get(1));
     }
 
     @Test
-    public void removeRootAdminRolesIfNeededTestNonRootAdminUser() {
+    public void removeRolesIfNeededTestRoleWithLessPermissionsRemoveRoles() {
-        Mockito.doReturn(accountMock).when(roleManagerImpl).getCurrentAccount();
+        setUpRoleVisibilityTests();
-        Mockito.doReturn(false).when(accountManagerMock).isRootAdmin(accountMockId);
-
         List<Role> roles = new ArrayList<>();
-        roleManagerImpl.removeRootAdminRolesIfNeeded(roles);
 
-        Mockito.verify(roleManagerImpl, Mockito.times(1)).removeRootAdminRoles(roles);
+        List<RolePermission> callerAccountRolePermissions = Collections.singletonList(rolePermission1Mock);
+        Mockito.when(roleManagerImpl.findAllPermissionsBy(callerAccountRoleMock.getId())).thenReturn(callerAccountRolePermissions);
+
+
+        roles.add(callerAccountRoleMock);
+        roles.add(morePermissionsRoleMock);
+
+        roleManagerImpl.removeRolesIfNeeded(roles);
+
+        Assert.assertEquals(1, roles.size());
+        Assert.assertEquals(callerAccountRoleMock, roles.get(0));
     }
 
     @Test
-    public void removeRootAdminRolesTest() {
+    public void removeRolesIfNeededTestRoleWithDifferentPermissionsRemoveRoles() {
+        setUpRoleVisibilityTests();
         List<Role> roles = new ArrayList<>();
-        Role roleRootAdmin = Mockito.mock(Role.class);
-        Mockito.doReturn(RoleType.Admin).when(roleRootAdmin).getRoleType();
 
-        Role roleDomainAdmin = Mockito.mock(Role.class);
+        List<RolePermission> callerAccountRolePermissions = Collections.singletonList(rolePermission1Mock);
-        Mockito.doReturn(RoleType.DomainAdmin).when(roleDomainAdmin).getRoleType();
+        Mockito.when(roleManagerImpl.findAllPermissionsBy(callerAccountRoleMock.getId())).thenReturn(callerAccountRolePermissions);
 
-        Role roleResourceAdmin = Mockito.mock(Role.class);
+        roles.add(callerAccountRoleMock);
-        Mockito.doReturn(RoleType.ResourceAdmin).when(roleResourceAdmin).getRoleType();
+        roles.add(differentPermissionsRoleMock);
 
-        Role roleUser = Mockito.mock(Role.class);
+        roleManagerImpl.removeRolesIfNeeded(roles);
-        Mockito.doReturn(RoleType.User).when(roleUser).getRoleType();
 
-        roles.add(roleRootAdmin);
+        Assert.assertEquals(1, roles.size());
-        roles.add(roleDomainAdmin);
+        Assert.assertEquals(callerAccountRoleMock, roles.get(0));
-        roles.add(roleResourceAdmin);
+    }
-        roles.add(roleUser);
 
-        roleManagerImpl.removeRootAdminRoles(roles);
+    @Test
+    public void roleHasPermissionTestRoleWithMoreAndSamePermissionsReturnsTrue() {
+        setUpRoleVisibilityTests();
+        rolePermissions.put("api1", Permission.ALLOW);
+        rolePermissions.put("api2", Permission.ALLOW);
 
-        Assert.assertEquals(3, roles.size());
+        boolean result = roleManagerImpl.roleHasPermission(rolePermissions, lessPermissionsRoleMock);
-        Assert.assertEquals(roleDomainAdmin, roles.get(0));
+
-        Assert.assertEquals(roleResourceAdmin, roles.get(1));
+        Assert.assertTrue(result);
-        Assert.assertEquals(roleUser, roles.get(2));
+    }
+
+    @Test
+    public void roleHasPermissionTestRoleAllowedApisDoesNotContainRoleToAccessAllowedApiReturnsFalse() {
+        setUpRoleVisibilityTests();
+        rolePermissions.put("api2", Permission.ALLOW);
+        rolePermissions.put("api3", Permission.ALLOW);
+
+        boolean result = roleManagerImpl.roleHasPermission(rolePermissions, morePermissionsRoleMock);
+
+        Assert.assertFalse(result);
+    }
+
+    @Test
+    public void roleHasPermissionTestRolePermissionsDeniedApiContainRoleToAccessAllowedApiReturnsFalse() {
+        setUpRoleVisibilityTests();
+        rolePermissions.put("api1", Permission.ALLOW);
+        rolePermissions.put("api2", Permission.DENY);
+
+        boolean result = roleManagerImpl.roleHasPermission(rolePermissions, morePermissionsRoleMock);
+
+        Assert.assertFalse(result);
+    }
+
+    @Test
+    public void getRolePermissionsTestRoleReturnsRolePermissions() {
+        setUpRoleVisibilityTests();
+
+        Map<String, Permission> roleRulesAndPermissions = roleManagerImpl.getRoleRulesAndPermissions(morePermissionsRoleMock.getId());
+
+        Assert.assertEquals(2, roleRulesAndPermissions.size());
+        Assert.assertEquals(roleRulesAndPermissions.get("api1"), Permission.ALLOW);
+        Assert.assertEquals(roleRulesAndPermissions.get("api2"), Permission.ALLOW);
     }
 
     @Test
@@ -277,12 +373,12 @@ public class RoleManagerImplTest {
         roles.add(Mockito.mock(Role.class));
 
         Mockito.doReturn(roles).when(roleDaoMock).listAll();
-        Mockito.doReturn(0).when(roleManagerImpl).removeRootAdminRolesIfNeeded(roles);
+        Mockito.doReturn(0).when(roleManagerImpl).removeRolesIfNeeded(roles);
 
         List<Role> returnedRoles = roleManagerImpl.listRoles();
 
         Assert.assertEquals(roles.size(), returnedRoles.size());
         Mockito.verify(roleDaoMock).listAll();
-        Mockito.verify(roleManagerImpl).removeRootAdminRolesIfNeeded(roles);
+        Mockito.verify(roleManagerImpl).removeRolesIfNeeded(roles);
     }
 }