From b807fff0d6bc1eb79c71fbf121204812b5fc552b Mon Sep 17 00:00:00 2001 From: Lucas Martins Date: Tue, 20 Feb 2024 16:32:25 -0300 Subject: [PATCH 1/2] Fix API deleteUser deleting caller --- .../api/command/admin/user/DeleteUserCmd.java | 7 +++---- .../java/com/cloud/user/AccountManagerImpl.java | 13 ++++++++++--- 2 files changed, 13 insertions(+), 7 deletions(-) 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 560e449412c2..ddf21affb535 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 ApiCommandResourceType getApiResourceType() { 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 1a30f192173a..fb2a3b0ea39e 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 AccountVO updateAccount(UpdateAccountCmd cmd) { @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 From fb606c6cf610e01df72d14cb6746683a2f50d844 Mon Sep 17 00:00:00 2001 From: Lucas Martins Date: Fri, 23 Feb 2024 17:19:50 -0300 Subject: [PATCH 2/2] Add unit tests --- .../com/cloud/user/AccountManagerImpl.java | 4 +- .../cloud/user/AccountManagerImplTest.java | 44 +++++++++++++++++++ 2 files changed, 46 insertions(+), 2 deletions(-) diff --git a/server/src/main/java/com/cloud/user/AccountManagerImpl.java b/server/src/main/java/com/cloud/user/AccountManagerImpl.java index fb2a3b0ea39e..a95b660975b9 100644 --- a/server/src/main/java/com/cloud/user/AccountManagerImpl.java +++ b/server/src/main/java/com/cloud/user/AccountManagerImpl.java @@ -2152,7 +2152,7 @@ private void checkIfNotMovingAcrossDomains(long domainId, Account newAccount) { } } - 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."); @@ -2162,7 +2162,7 @@ private void checkAccountAndAccess(UserVO user, Account account) { 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 6d9211dd526d..d98a4f8f0587 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.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.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 void deleteUserAccountCleanup() { 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);