From a331157f06a0b9da48a40b75210ce6754ba3ba46 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Bernardo=20De=20Marco=20Gon=C3=A7alves?= Date: Sat, 11 Jan 2025 14:09:36 -0300 Subject: [PATCH 1/2] check whether account has network permissions before attempting to delete it --- .../com/cloud/user/AccountManagerImpl.java | 37 +++++++++++++------ .../cloud/user/AccountManagerImplTest.java | 36 ++++++++++++++++++ .../user/AccountManagetImplTestBase.java | 3 ++ 3 files changed, 65 insertions(+), 11 deletions(-) diff --git a/server/src/main/java/com/cloud/user/AccountManagerImpl.java b/server/src/main/java/com/cloud/user/AccountManagerImpl.java index 2d7ebf595fde..491ea364bba4 100644 --- a/server/src/main/java/com/cloud/user/AccountManagerImpl.java +++ b/server/src/main/java/com/cloud/user/AccountManagerImpl.java @@ -74,6 +74,7 @@ import org.apache.cloudstack.framework.messagebus.MessageBus; import org.apache.cloudstack.framework.messagebus.PublishScope; import org.apache.cloudstack.managed.context.ManagedContextRunnable; +import org.apache.cloudstack.network.dao.NetworkPermissionDao; import org.apache.cloudstack.region.gslb.GlobalLoadBalancerRuleDao; import org.apache.cloudstack.resourcedetail.UserDetailVO; import org.apache.cloudstack.resourcedetail.dao.UserDetailsDao; @@ -298,6 +299,8 @@ public class AccountManagerImpl extends ManagerBase implements AccountManager, M private SSHKeyPairDao _sshKeyPairDao; @Inject private UserDataDao userDataDao; + @Inject + private NetworkPermissionDao networkPermissionDao; private List _querySelectors; @@ -1857,26 +1860,38 @@ public boolean deleteUserAccount(long accountId) { // If the user is a System user, return an error. We do not allow this AccountVO account = _accountDao.findById(accountId); - if (! isDeleteNeeded(account, accountId, caller)) { + if (!isDeleteNeeded(account, accountId, caller)) { return true; } - // Account that manages project(s) can't be removed - List managedProjectIds = _projectAccountDao.listAdministratedProjectIds(accountId); - if (!managedProjectIds.isEmpty()) { - StringBuilder projectIds = new StringBuilder(); - for (Long projectId : managedProjectIds) { - projectIds.append(projectId).append(", "); - } - - throw new InvalidParameterValueException("The account id=" + accountId + " manages project(s) with ids " + projectIds + "and can't be removed"); - } + checkIfAccountManagesProjects(accountId); + checkIfAccountHasNetworkPermissions(accountId); CallContext.current().putContextParameter(Account.class, account.getUuid()); return deleteAccount(account, callerUserId, caller); } + protected void checkIfAccountManagesProjects(long accountId) { + List managedProjectIds = _projectAccountDao.listAdministratedProjectIds(accountId); + if (!CollectionUtils.isEmpty(managedProjectIds)) { + throw new InvalidParameterValueException(String.format( + "Unable to delete account [%s], because it manages the following project(s): %s. Please, remove the account from these projects first.", + accountId, managedProjectIds + )); + } + } + + protected void checkIfAccountHasNetworkPermissions(long accountId) { + List networkIds = networkPermissionDao.listPermittedNetworkIdsByAccounts(List.of(accountId)); + if (!CollectionUtils.isEmpty(networkIds)) { + throw new InvalidParameterValueException(String.format( + "Unable to delete account [%s], because it has network permissions for the following network(s): %s. Please, remove the network permissions first.", + accountId, networkIds + )); + } + } + private boolean isDeleteNeeded(AccountVO account, long accountId, Account caller) { if (account == null) { s_logger.info(String.format("The account, identified by id %d, doesn't exist", accountId )); diff --git a/server/src/test/java/com/cloud/user/AccountManagerImplTest.java b/server/src/test/java/com/cloud/user/AccountManagerImplTest.java index ed0a123d4a38..8db75e8e9752 100644 --- a/server/src/test/java/com/cloud/user/AccountManagerImplTest.java +++ b/server/src/test/java/com/cloud/user/AccountManagerImplTest.java @@ -1200,4 +1200,40 @@ public void testValidateRoleAdminCannotEscalateAdminFromNonRootDomain() { Mockito.when(roleService.findRole(2L)).thenReturn(callerRole); accountManagerImpl.validateRoleChange(account, newRole, caller); } + + @Test + public void checkIfAccountManagesProjectsTestNotThrowExceptionWhenTheAccountIsNotAProjectAdministrator() { + long accountId = 1L; + List managedProjectIds = new ArrayList<>(); + + Mockito.when(_projectAccountDao.listAdministratedProjectIds(accountId)).thenReturn(managedProjectIds); + accountManagerImpl.checkIfAccountManagesProjects(accountId); + } + + @Test(expected = InvalidParameterValueException.class) + public void checkIfAccountHasNetworkPermissionsTestThrowExceptionWhenTheAccountHasNetworkPermissions() { + long accountId = 1L; + List networkIds = List.of(1L); + + Mockito.when(networkPermissionDaoMock.listPermittedNetworkIdsByAccounts(List.of(accountId))).thenReturn(networkIds); + accountManagerImpl.checkIfAccountHasNetworkPermissions(accountId); + } + + @Test + public void checkIfAccountHasNetworkPermissionsTestNotThrowExceptionWhenTheAccountDoesNotHaveNetworkPermissions() { + long accountId = 1L; + List networkIds = new ArrayList<>(); + + Mockito.when(networkPermissionDaoMock.listPermittedNetworkIdsByAccounts(List.of(accountId))).thenReturn(networkIds); + accountManagerImpl.checkIfAccountHasNetworkPermissions(accountId); + } + + @Test(expected = InvalidParameterValueException.class) + public void checkIfAccountManagesProjectsTestThrowExceptionWhenTheAccountIsAProjectAdministrator() { + long accountId = 1L; + List managedProjectIds = List.of(1L); + + Mockito.when(_projectAccountDao.listAdministratedProjectIds(accountId)).thenReturn(managedProjectIds); + accountManagerImpl.checkIfAccountManagesProjects(accountId); + } } diff --git a/server/src/test/java/com/cloud/user/AccountManagetImplTestBase.java b/server/src/test/java/com/cloud/user/AccountManagetImplTestBase.java index 7f9fa488471a..2fa18221a940 100644 --- a/server/src/test/java/com/cloud/user/AccountManagetImplTestBase.java +++ b/server/src/test/java/com/cloud/user/AccountManagetImplTestBase.java @@ -65,6 +65,7 @@ import org.apache.cloudstack.engine.service.api.OrchestrationService; import org.apache.cloudstack.framework.config.dao.ConfigurationDao; import org.apache.cloudstack.framework.messagebus.MessageBus; +import org.apache.cloudstack.network.dao.NetworkPermissionDao; import org.apache.cloudstack.region.gslb.GlobalLoadBalancerRuleDao; import org.apache.cloudstack.resourcedetail.dao.UserDetailsDao; import org.junit.After; @@ -195,6 +196,8 @@ public class AccountManagetImplTestBase { SSHKeyPairDao _sshKeyPairDao; @Mock UserDataDao userDataDao; + @Mock + NetworkPermissionDao networkPermissionDaoMock; @Spy @InjectMocks From e63e07a3f0199fc862684103033e6587ac818c97 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Bernardo=20De=20Marco=20Gon=C3=A7alves?= Date: Sat, 11 Jan 2025 16:10:32 -0300 Subject: [PATCH 2/2] clean up network permissions when an account is deleted --- .../network/dao/NetworkPermissionDao.java | 7 +++++++ .../network/dao/NetworkPermissionDaoImpl.java | 15 +++++++++++++++ .../com/cloud/user/AccountManagerImpl.java | 16 ++++------------ .../com/cloud/user/AccountManagerImplTest.java | 18 ------------------ 4 files changed, 26 insertions(+), 30 deletions(-) diff --git a/engine/schema/src/main/java/org/apache/cloudstack/network/dao/NetworkPermissionDao.java b/engine/schema/src/main/java/org/apache/cloudstack/network/dao/NetworkPermissionDao.java index 1c8d1cf48ff2..e8b6322baeef 100644 --- a/engine/schema/src/main/java/org/apache/cloudstack/network/dao/NetworkPermissionDao.java +++ b/engine/schema/src/main/java/org/apache/cloudstack/network/dao/NetworkPermissionDao.java @@ -40,6 +40,13 @@ public interface NetworkPermissionDao extends GenericDao NetworkAndAccountSearch; private SearchBuilder NetworkIdSearch; + private SearchBuilder accountSearch; private GenericSearchBuilder FindNetworkIdsByAccount; protected NetworkPermissionDaoImpl() { @@ -47,6 +48,10 @@ protected NetworkPermissionDaoImpl() { NetworkIdSearch.and("networkId", NetworkIdSearch.entity().getNetworkId(), SearchCriteria.Op.EQ); NetworkIdSearch.done(); + accountSearch = createSearchBuilder(); + accountSearch.and("accountId", accountSearch.entity().getAccountId(), SearchCriteria.Op.EQ); + accountSearch.done(); + FindNetworkIdsByAccount = createSearchBuilder(Long.class); FindNetworkIdsByAccount.select(null, SearchCriteria.Func.DISTINCT, FindNetworkIdsByAccount.entity().getNetworkId()); FindNetworkIdsByAccount.and("account", FindNetworkIdsByAccount.entity().getAccountId(), SearchCriteria.Op.IN); @@ -71,6 +76,16 @@ public void removeAllPermissions(long networkId) { expunge(sc); } + @Override + public void removeAccountPermissions(long accountId) { + SearchCriteria sc = accountSearch.create(); + sc.setParameters("accountId", accountId); + int networkPermissionRemoved = expunge(sc); + if (networkPermissionRemoved > 0) { + s_logger.debug(String.format("Removed [%s] network permission(s) for the account with Id [%s]", networkPermissionRemoved, accountId)); + } + } + @Override public NetworkPermissionVO findByNetworkAndAccount(long networkId, long accountId) { SearchCriteria sc = NetworkAndAccountSearch.create(); diff --git a/server/src/main/java/com/cloud/user/AccountManagerImpl.java b/server/src/main/java/com/cloud/user/AccountManagerImpl.java index 491ea364bba4..1e727036d565 100644 --- a/server/src/main/java/com/cloud/user/AccountManagerImpl.java +++ b/server/src/main/java/com/cloud/user/AccountManagerImpl.java @@ -873,6 +873,9 @@ protected boolean cleanupAccount(AccountVO account, long callerUserId, Account c // delete the account from project accounts _projectAccountDao.removeAccountFromProjects(accountId); + // Delete account's network permissions + networkPermissionDao.removeAccountPermissions(accountId); + if (account.getType() != Account.Type.PROJECT) { // delete the account from group _messageBus.publish(_name, MESSAGE_REMOVE_ACCOUNT_EVENT, PublishScope.LOCAL, accountId); @@ -1865,7 +1868,6 @@ public boolean deleteUserAccount(long accountId) { } checkIfAccountManagesProjects(accountId); - checkIfAccountHasNetworkPermissions(accountId); CallContext.current().putContextParameter(Account.class, account.getUuid()); @@ -1876,22 +1878,12 @@ protected void checkIfAccountManagesProjects(long accountId) { List managedProjectIds = _projectAccountDao.listAdministratedProjectIds(accountId); if (!CollectionUtils.isEmpty(managedProjectIds)) { throw new InvalidParameterValueException(String.format( - "Unable to delete account [%s], because it manages the following project(s): %s. Please, remove the account from these projects first.", + "Unable to delete account [%s], because it manages the following project(s): %s. Please, remove the account from these projects or demote it to a regular project role first.", accountId, managedProjectIds )); } } - protected void checkIfAccountHasNetworkPermissions(long accountId) { - List networkIds = networkPermissionDao.listPermittedNetworkIdsByAccounts(List.of(accountId)); - if (!CollectionUtils.isEmpty(networkIds)) { - throw new InvalidParameterValueException(String.format( - "Unable to delete account [%s], because it has network permissions for the following network(s): %s. Please, remove the network permissions first.", - accountId, networkIds - )); - } - } - private boolean isDeleteNeeded(AccountVO account, long accountId, Account caller) { if (account == null) { s_logger.info(String.format("The account, identified by id %d, doesn't exist", accountId )); diff --git a/server/src/test/java/com/cloud/user/AccountManagerImplTest.java b/server/src/test/java/com/cloud/user/AccountManagerImplTest.java index 8db75e8e9752..0e8e1df0f3a0 100644 --- a/server/src/test/java/com/cloud/user/AccountManagerImplTest.java +++ b/server/src/test/java/com/cloud/user/AccountManagerImplTest.java @@ -1210,24 +1210,6 @@ public void checkIfAccountManagesProjectsTestNotThrowExceptionWhenTheAccountIsNo accountManagerImpl.checkIfAccountManagesProjects(accountId); } - @Test(expected = InvalidParameterValueException.class) - public void checkIfAccountHasNetworkPermissionsTestThrowExceptionWhenTheAccountHasNetworkPermissions() { - long accountId = 1L; - List networkIds = List.of(1L); - - Mockito.when(networkPermissionDaoMock.listPermittedNetworkIdsByAccounts(List.of(accountId))).thenReturn(networkIds); - accountManagerImpl.checkIfAccountHasNetworkPermissions(accountId); - } - - @Test - public void checkIfAccountHasNetworkPermissionsTestNotThrowExceptionWhenTheAccountDoesNotHaveNetworkPermissions() { - long accountId = 1L; - List networkIds = new ArrayList<>(); - - Mockito.when(networkPermissionDaoMock.listPermittedNetworkIdsByAccounts(List.of(accountId))).thenReturn(networkIds); - accountManagerImpl.checkIfAccountHasNetworkPermissions(accountId); - } - @Test(expected = InvalidParameterValueException.class) public void checkIfAccountManagesProjectsTestThrowExceptionWhenTheAccountIsAProjectAdministrator() { long accountId = 1L;