From 796bd4f72cc021b91441b6c34a9d8f7425c53374 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Bernardo=20De=20Marco=20Gon=C3=A7alves?= Date: Wed, 15 Jan 2025 13:38:20 -0300 Subject: [PATCH] Clean up network permissions on account deletion (#10176) --- .../network/dao/NetworkPermissionDao.java | 7 +++++ .../network/dao/NetworkPermissionDaoImpl.java | 15 ++++++++++ .../com/cloud/user/AccountManagerImpl.java | 29 ++++++++++++------- .../cloud/user/AccountManagerImplTest.java | 18 ++++++++++++ .../user/AccountManagetImplTestBase.java | 3 ++ 5 files changed, 61 insertions(+), 11 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 1c8d1cf48ff..e8b6322baee 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 @@ public class NetworkPermissionDaoImpl extends GenericDaoBase 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 2d7ebf595fd..1e727036d56 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.config.dao.ConfigurationDao; 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; @@ -870,6 +873,9 @@ public class AccountManagerImpl extends ManagerBase implements AccountManager, M // 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); @@ -1857,26 +1863,27 @@ public class AccountManagerImpl extends ManagerBase implements AccountManager, M // 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); 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 or demote it to a regular project role first.", + accountId, managedProjectIds + )); + } + } + 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 ed0a123d4a3..0e8e1df0f3a 100644 --- a/server/src/test/java/com/cloud/user/AccountManagerImplTest.java +++ b/server/src/test/java/com/cloud/user/AccountManagerImplTest.java @@ -1200,4 +1200,22 @@ public class AccountManagerImplTest extends AccountManagetImplTestBase { 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 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 7f9fa488471..2fa18221a94 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.orchestration.service.NetworkOrchestrationSe 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