This is an automated email from the ASF dual-hosted git repository.
dahn pushed a commit to branch main
in repository https://gitbox.apache.org/repos/asf/cloudstack.git
The following commit(s) were added to refs/heads/main by this push:
new c8a4575bcd0 Fix `deleteUser` API to prevent deletion of the caller
(#8691)
c8a4575bcd0 is described below
commit c8a4575bcd00bc5342a9cbea9d3e6c693c126cb7
Author: Lucas Martins <[email protected]>
AuthorDate: Tue Feb 27 05:43:58 2024 -0300
Fix `deleteUser` API to prevent deletion of the caller (#8691)
Co-authored-by: Lucas Martins <[email protected]>
---
.../api/command/admin/user/DeleteUserCmd.java | 7 ++--
.../java/com/cloud/user/AccountManagerImpl.java | 17 ++++++---
.../com/cloud/user/AccountManagerImplTest.java | 44 ++++++++++++++++++++++
3 files changed, 59 insertions(+), 9 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 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<CallContext> 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<CallContext> 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<Boolean, UserAuthenticator.ActionOnFailedAuthentication>
successAuthenticationPair = new Pair<>(true, null);
