diff --git a/api/src/main/java/org/apache/cloudstack/api/command/admin/user/DeleteUserCmd.java b/api/src/main/java/org/apache/cloudstack/api/command/admin/user/DeleteUserCmd.java index 560e449412c..ddf21affb53 100644 --- a/api/src/main/java/org/apache/cloudstack/api/command/admin/user/DeleteUserCmd.java +++ b/api/src/main/java/org/apache/cloudstack/api/command/admin/user/DeleteUserCmd.java @@ -84,11 +84,10 @@ public class DeleteUserCmd extends BaseCmd { public void execute() { CallContext.current().setEventDetails("UserId: " + getId()); boolean result = _regionService.deleteUser(this); - if (result) { - SuccessResponse response = new SuccessResponse(getCommandName()); - this.setResponseObject(response); - } else { + if (!result) { throw new ServerApiException(ApiErrorCode.INTERNAL_ERROR, "Failed to delete user"); } + SuccessResponse response = new SuccessResponse(getCommandName()); + this.setResponseObject(response); } } diff --git a/server/src/main/java/com/cloud/user/AccountManagerImpl.java b/server/src/main/java/com/cloud/user/AccountManagerImpl.java index 1a30f192173..a95b660975b 100644 --- a/server/src/main/java/com/cloud/user/AccountManagerImpl.java +++ b/server/src/main/java/com/cloud/user/AccountManagerImpl.java @@ -2064,13 +2064,20 @@ public class AccountManagerImpl extends ManagerBase implements AccountManager, M @Override @ActionEvent(eventType = EventTypes.EVENT_USER_DELETE, eventDescription = "deleting User") public boolean deleteUser(DeleteUserCmd deleteUserCmd) { - UserVO user = getValidUserVO(deleteUserCmd.getId()); - + final Long id = deleteUserCmd.getId(); + User caller = CallContext.current().getCallingUser(); + UserVO user = getValidUserVO(id); Account account = _accountDao.findById(user.getAccountId()); + if (caller.getId() == id) { + Domain domain = _domainDao.findById(account.getDomainId()); + throw new InvalidParameterValueException(String.format("The caller is requesting to delete itself. As a security measure, ACS will not allow this operation." + + " To delete user %s (ID: %s, Domain: %s), request to another user with permission to execute the operation.", user.getUsername(), user.getUuid(), domain.getUuid())); + } + // don't allow to delete the user from the account of type Project checkAccountAndAccess(user, account); - return _userDao.remove(deleteUserCmd.getId()); + return _userDao.remove(id); } @Override @@ -2145,7 +2152,7 @@ public class AccountManagerImpl extends ManagerBase implements AccountManager, M } } - private void checkAccountAndAccess(UserVO user, Account account) { + protected void checkAccountAndAccess(UserVO user, Account account) { // don't allow to delete the user from the account of type Project if (account.getType() == Account.Type.PROJECT) { throw new InvalidParameterValueException("Project users cannot be deleted or moved."); @@ -2155,7 +2162,7 @@ public class AccountManagerImpl extends ManagerBase implements AccountManager, M CallContext.current().putContextParameter(User.class, user.getUuid()); } - private UserVO getValidUserVO(long id) { + protected UserVO getValidUserVO(long id) { UserVO user = _userDao.findById(id); if (user == null || user.getRemoved() != null) { diff --git a/server/src/test/java/com/cloud/user/AccountManagerImplTest.java b/server/src/test/java/com/cloud/user/AccountManagerImplTest.java index 6d9211dd526..d98a4f8f058 100644 --- a/server/src/test/java/com/cloud/user/AccountManagerImplTest.java +++ b/server/src/test/java/com/cloud/user/AccountManagerImplTest.java @@ -34,6 +34,7 @@ import com.cloud.vm.UserVmVO; import com.cloud.vm.VMInstanceVO; import com.cloud.vm.snapshot.VMSnapshotVO; import org.apache.cloudstack.acl.SecurityChecker.AccessType; +import org.apache.cloudstack.api.command.admin.user.DeleteUserCmd; import org.apache.cloudstack.api.command.admin.user.GetUserKeysCmd; import org.apache.cloudstack.api.command.admin.user.UpdateUserCmd; import org.apache.cloudstack.api.response.UserTwoFactorAuthenticationSetupResponse; @@ -48,6 +49,7 @@ import org.junit.Test; import org.junit.runner.RunWith; import org.mockito.InOrder; import org.mockito.Mock; +import org.mockito.MockedStatic; import org.mockito.Mockito; import org.mockito.junit.MockitoJUnitRunner; @@ -91,6 +93,12 @@ public class AccountManagerImplTest extends AccountManagetImplTestBase { @Mock private Account accountMock; + @Mock + private DomainVO domainVoMock; + + @Mock + private AccountVO accountVoMock; + @Mock private ProjectAccountVO projectAccountVO; @Mock @@ -190,6 +198,42 @@ public class AccountManagerImplTest extends AccountManagetImplTestBase { Mockito.verify(_accountDao, Mockito.atLeastOnce()).markForCleanup(Mockito.eq(42l)); } + @Test (expected = InvalidParameterValueException.class) + public void deleteUserTestIfUserIdIsEqualToCallerIdShouldThrowException() { + try (MockedStatic callContextMocked = Mockito.mockStatic(CallContext.class)) { + DeleteUserCmd cmd = Mockito.mock(DeleteUserCmd.class); + CallContext callContextMock = Mockito.mock(CallContext.class); + callContextMocked.when(CallContext::current).thenReturn(callContextMock); + + Mockito.doReturn(userVoMock).when(callContextMock).getCallingUser(); + Mockito.doReturn(1L).when(cmd).getId(); + Mockito.doReturn(userVoMock).when(accountManagerImpl).getValidUserVO(Mockito.anyLong()); + Mockito.doReturn(accountVoMock).when(_accountDao).findById(Mockito.anyLong()); + Mockito.doReturn(domainVoMock).when(_domainDao).findById(Mockito.anyLong()); + Mockito.doReturn(1L).when(userVoMock).getId(); + + accountManagerImpl.deleteUser(cmd); + } + } + + @Test + public void deleteUserTestIfUserIdIsNotEqualToCallerIdShouldNotThrowException() { + try (MockedStatic callContextMocked = Mockito.mockStatic(CallContext.class)) { + DeleteUserCmd cmd = Mockito.mock(DeleteUserCmd.class); + CallContext callContextMock = Mockito.mock(CallContext.class); + callContextMocked.when(CallContext::current).thenReturn(callContextMock); + + Mockito.doReturn(userVoMock).when(callContextMock).getCallingUser(); + Mockito.doReturn(1L).when(cmd).getId(); + Mockito.doReturn(userVoMock).when(accountManagerImpl).getValidUserVO(Mockito.anyLong()); + Mockito.doReturn(accountVoMock).when(_accountDao).findById(Mockito.anyLong()); + Mockito.doReturn(2L).when(userVoMock).getId(); + + Mockito.doNothing().when(accountManagerImpl).checkAccountAndAccess(Mockito.any(), Mockito.any()); + accountManagerImpl.deleteUser(cmd); + } + } + @Test public void testAuthenticateUser() throws UnknownHostException { Pair successAuthenticationPair = new Pair<>(true, null);