diff --git a/plugins/network-elements/juniper-contrail/src/test/java/org/apache/cloudstack/network/contrail/management/MockAccountManager.java b/plugins/network-elements/juniper-contrail/src/test/java/org/apache/cloudstack/network/contrail/management/MockAccountManager.java index 836bb721329..610f4aa82aa 100644 --- a/plugins/network-elements/juniper-contrail/src/test/java/org/apache/cloudstack/network/contrail/management/MockAccountManager.java +++ b/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 getApiNameList() { + return null; + } + @Override public boolean deleteUserAccount(long arg0) { // TODO Auto-generated method stub diff --git a/server/src/main/java/com/cloud/user/AccountManager.java b/server/src/main/java/com/cloud/user/AccountManager.java index c95047a6c42..6d2d1db5668 100644 --- a/server/src/main/java/com/cloud/user/AccountManager.java +++ b/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 getApiNameList(); } diff --git a/server/src/main/java/com/cloud/user/AccountManagerImpl.java b/server/src/main/java/com/cloud/user/AccountManagerImpl.java index a95b660975b..d996b684b25 100644 --- a/server/src/main/java/com/cloud/user/AccountManagerImpl.java +++ b/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 getApiNameList() { + return apiNameList; + } + @Override public boolean configure(final String name, final Map params) throws ConfigurationException { _systemAccount = _accountDao.findById(Account.ACCOUNT_ID_SYSTEM); diff --git a/server/src/main/java/org/apache/cloudstack/acl/RoleManagerImpl.java b/server/src/main/java/org/apache/cloudstack/acl/RoleManagerImpl.java index 9b6ac0e0279..bab286bb8f9 100644 --- a/server/src/main/java/org/apache/cloudstack/acl/RoleManagerImpl.java +++ b/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, Integer> findRolesByName(String name, String keyword, Long startIndex, Long limit) { if (StringUtils.isNotBlank(name) || StringUtils.isNotBlank(keyword)) { Pair, Integer> data = roleDao.findAllByName(name, keyword, startIndex, limit, isCallerRootAdmin()); - int removed = removeRootAdminRolesIfNeeded(data.first()); + int removed = removeRolesIfNeeded(data.first()); return new Pair,Integer>(ListUtils.toListOfInterface(data.first()), Integer.valueOf(data.second() - removed)); } return new Pair, Integer>(new ArrayList(), 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'. - * The actual removal is executed via {@link #removeRootAdminRoles(List)}. Therefore, if the method is called by a 'root admin', we do nothing here. + * Removes roles from the given list if the role has different or more permissions than the user's calling the method role */ - protected int removeRootAdminRolesIfNeeded(List roles) { - if (!isCallerRootAdmin()) { - return removeRootAdminRoles(roles); + protected int removeRolesIfNeeded(List roles) { + if (roles.isEmpty()) { + return 0; } - return 0; + + Long callerRoleId = getCurrentAccount().getRoleId(); + Map callerRolePermissions = getRoleRulesAndPermissions(callerRoleId); + + int count = 0; + Iterator 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 roles) { - if (CollectionUtils.isEmpty(roles)) { - return 0; - } - Iterator rolesIterator = roles.iterator(); - int count = 0; - while (rolesIterator.hasNext()) { - Role role = rolesIterator.next(); - if (RoleType.Admin == role.getRoleType()) { - count++; - rolesIterator.remove(); + protected boolean roleHasPermission(Map sourceRolePermissions, Role targetRole) { + Set rulesAlreadyCompared = new HashSet<>(); + for (RolePermission rolePermission : findAllPermissionsBy(targetRole.getId())) { + boolean permissionIsRegex = rolePermission.getRule().getRuleString().contains("*"); + + for (String apiName : accountManager.getApiNameList()) { + if (!rolePermission.getRule().matches(apiName) || rulesAlreadyCompared.contains(apiName)) { + continue; + } + + 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 getRoleRulesAndPermissions(Long roleId) { + Map 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 listRoles() { List roles = roleDao.listAll(); - removeRootAdminRolesIfNeeded(roles); + removeRolesIfNeeded(roles); return ListUtils.toListOfInterface(roles); } @Override public Pair, Integer> listRoles(Long startIndex, Long limit) { Pair, Integer> data = roleDao.listAllRoles(startIndex, limit, isCallerRootAdmin()); - int removed = removeRootAdminRolesIfNeeded(data.first()); + int removed = removeRolesIfNeeded(data.first()); return new Pair,Integer>(ListUtils.toListOfInterface(data.first()), Integer.valueOf(data.second() - removed)); } diff --git a/server/src/test/java/com/cloud/user/MockAccountManagerImpl.java b/server/src/test/java/com/cloud/user/MockAccountManagerImpl.java index fe7748b8581..334e1f33481 100644 --- a/server/src/test/java/com/cloud/user/MockAccountManagerImpl.java +++ b/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 getApiNameList() { + return null; + } } diff --git a/server/src/test/java/org/apache/cloudstack/acl/RoleManagerImplTest.java b/server/src/test/java/org/apache/cloudstack/acl/RoleManagerImplTest.java index 2bb46c0deee..e596601325e 100644 --- a/server/src/test/java/org/apache/cloudstack/acl/RoleManagerImplTest.java +++ b/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 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 lessPermissionsRolePermissions = Collections.singletonList(rolePermission1Mock); + Mockito.doReturn(1L).when(lessPermissionsRoleMock).getId(); + Mockito.when(roleManagerImpl.findAllPermissionsBy(1L)).thenReturn(lessPermissionsRolePermissions); + + List morePermissionsRolePermissions = List.of(rolePermission1Mock, rolePermission2Mock); + Mockito.doReturn(2L).when(morePermissionsRoleMock).getId(); + Mockito.when(roleManagerImpl.findAllPermissionsBy(morePermissionsRoleMock.getId())).thenReturn(morePermissionsRolePermissions); + + List 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() { - Mockito.doReturn(accountMock).when(roleManagerImpl).getCurrentAccount(); - Mockito.doReturn(true).when(accountManagerMock).isRootAdmin(accountMockId); - + public void removeRolesIfNeededTestRoleWithMoreAndSamePermissionsKeepRoles() { + setUpRoleVisibilityTests(); List roles = new ArrayList<>(); - roleManagerImpl.removeRootAdminRolesIfNeeded(roles); - Mockito.verify(roleManagerImpl, Mockito.times(0)).removeRootAdminRoles(roles); + List 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() { - Mockito.doReturn(accountMock).when(roleManagerImpl).getCurrentAccount(); - Mockito.doReturn(false).when(accountManagerMock).isRootAdmin(accountMockId); - + public void removeRolesIfNeededTestRoleWithLessPermissionsRemoveRoles() { + setUpRoleVisibilityTests(); List roles = new ArrayList<>(); - roleManagerImpl.removeRootAdminRolesIfNeeded(roles); - Mockito.verify(roleManagerImpl, Mockito.times(1)).removeRootAdminRoles(roles); + List 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 roles = new ArrayList<>(); - Role roleRootAdmin = Mockito.mock(Role.class); - Mockito.doReturn(RoleType.Admin).when(roleRootAdmin).getRoleType(); - Role roleDomainAdmin = Mockito.mock(Role.class); - Mockito.doReturn(RoleType.DomainAdmin).when(roleDomainAdmin).getRoleType(); + List callerAccountRolePermissions = Collections.singletonList(rolePermission1Mock); + Mockito.when(roleManagerImpl.findAllPermissionsBy(callerAccountRoleMock.getId())).thenReturn(callerAccountRolePermissions); - Role roleResourceAdmin = Mockito.mock(Role.class); - Mockito.doReturn(RoleType.ResourceAdmin).when(roleResourceAdmin).getRoleType(); + roles.add(callerAccountRoleMock); + roles.add(differentPermissionsRoleMock); - Role roleUser = Mockito.mock(Role.class); - Mockito.doReturn(RoleType.User).when(roleUser).getRoleType(); + roleManagerImpl.removeRolesIfNeeded(roles); - roles.add(roleRootAdmin); - roles.add(roleDomainAdmin); - roles.add(roleResourceAdmin); - roles.add(roleUser); + Assert.assertEquals(1, roles.size()); + Assert.assertEquals(callerAccountRoleMock, roles.get(0)); + } - roleManagerImpl.removeRootAdminRoles(roles); + @Test + public void roleHasPermissionTestRoleWithMoreAndSamePermissionsReturnsTrue() { + setUpRoleVisibilityTests(); + rolePermissions.put("api1", Permission.ALLOW); + rolePermissions.put("api2", Permission.ALLOW); - Assert.assertEquals(3, roles.size()); - Assert.assertEquals(roleDomainAdmin, roles.get(0)); - Assert.assertEquals(roleResourceAdmin, roles.get(1)); - Assert.assertEquals(roleUser, roles.get(2)); + boolean result = roleManagerImpl.roleHasPermission(rolePermissions, lessPermissionsRoleMock); + + Assert.assertTrue(result); + } + + @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 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 returnedRoles = roleManagerImpl.listRoles(); Assert.assertEquals(roles.size(), returnedRoles.size()); Mockito.verify(roleDaoMock).listAll(); - Mockito.verify(roleManagerImpl).removeRootAdminRolesIfNeeded(roles); + Mockito.verify(roleManagerImpl).removeRolesIfNeeded(roles); } }