From a13653811b4208b6b73fb9a40e333397f3ffecc5 Mon Sep 17 00:00:00 2001 From: Lucas Martins Date: Fri, 1 Mar 2024 12:47:54 -0300 Subject: [PATCH 1/6] Fix API deleteAccount deleting caller's account --- .../api/command/admin/account/DeleteAccountCmd.java | 7 +++---- .../main/java/com/cloud/user/AccountManagerImpl.java | 11 ++++++++++- 2 files changed, 13 insertions(+), 5 deletions(-) diff --git a/api/src/main/java/org/apache/cloudstack/api/command/admin/account/DeleteAccountCmd.java b/api/src/main/java/org/apache/cloudstack/api/command/admin/account/DeleteAccountCmd.java index 36e22acff91e..a90fc4aebe9c 100644 --- a/api/src/main/java/org/apache/cloudstack/api/command/admin/account/DeleteAccountCmd.java +++ b/api/src/main/java/org/apache/cloudstack/api/command/admin/account/DeleteAccountCmd.java @@ -89,12 +89,11 @@ public void execute() { CallContext.current().setEventDetails("Account ID: " + (account != null ? account.getUuid() : getId())); // Account not found is already handled by service boolean result = _regionService.deleteUserAccount(this); - if (result) { - SuccessResponse response = new SuccessResponse(getCommandName()); - setResponseObject(response); - } else { + if (!result) { throw new ServerApiException(ApiErrorCode.INTERNAL_ERROR, "Failed to delete user account and all corresponding users"); } + SuccessResponse response = new SuccessResponse(getCommandName()); + setResponseObject(response); } @Override diff --git a/server/src/main/java/com/cloud/user/AccountManagerImpl.java b/server/src/main/java/com/cloud/user/AccountManagerImpl.java index 1a30f192173a..8f09eea7a1bc 100644 --- a/server/src/main/java/com/cloud/user/AccountManagerImpl.java +++ b/server/src/main/java/com/cloud/user/AccountManagerImpl.java @@ -1815,7 +1815,15 @@ 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 (caller.getId() == accountId) { + Domain domain = _domainDao.findById(account.getDomainId()); + throw new InvalidParameterValueException(String.format("The caller is requesting to delete their own account. As a security measure, ACS will not allow this " + + "operation." + + "To delete account %s (ID: %s, Domain: %s), request to another user with permission to execute the operation.", + account.getAccountName(), account.getUuid(), domain.getUuid())); + } + + if (!isDeleteNeeded(account, accountId, caller)) { return true; } @@ -1830,6 +1838,7 @@ public boolean deleteUserAccount(long accountId) { throw new InvalidParameterValueException("The account id=" + accountId + " manages project(s) with ids " + projectIds + "and can't be removed"); } + CallContext.current().putContextParameter(Account.class, account.getUuid()); return deleteAccount(account, callerUserId, caller); From 49e29ed5f7c53a5be94432457e9297dc407ac4a2 Mon Sep 17 00:00:00 2001 From: Lucas Martins Date: Fri, 1 Mar 2024 13:41:57 -0300 Subject: [PATCH 2/6] Remove extra line --- server/src/main/java/com/cloud/user/AccountManagerImpl.java | 1 - 1 file changed, 1 deletion(-) diff --git a/server/src/main/java/com/cloud/user/AccountManagerImpl.java b/server/src/main/java/com/cloud/user/AccountManagerImpl.java index 8f09eea7a1bc..ff05cf552e65 100644 --- a/server/src/main/java/com/cloud/user/AccountManagerImpl.java +++ b/server/src/main/java/com/cloud/user/AccountManagerImpl.java @@ -1838,7 +1838,6 @@ public boolean deleteUserAccount(long accountId) { throw new InvalidParameterValueException("The account id=" + accountId + " manages project(s) with ids " + projectIds + "and can't be removed"); } - CallContext.current().putContextParameter(Account.class, account.getUuid()); return deleteAccount(account, callerUserId, caller); From c6608808e628ea02b5b9d3fea473f12571c00f72 Mon Sep 17 00:00:00 2001 From: Lucas Martins Date: Tue, 5 Mar 2024 16:40:43 -0300 Subject: [PATCH 3/6] Add unit tests and change message --- .../com/cloud/user/AccountManagerImpl.java | 7 ++-- .../cloud/user/AccountManagerImplTest.java | 32 +++++++++++++++++++ 2 files changed, 35 insertions(+), 4 deletions(-) diff --git a/server/src/main/java/com/cloud/user/AccountManagerImpl.java b/server/src/main/java/com/cloud/user/AccountManagerImpl.java index 6968f0ce87a2..059af94703d3 100644 --- a/server/src/main/java/com/cloud/user/AccountManagerImpl.java +++ b/server/src/main/java/com/cloud/user/AccountManagerImpl.java @@ -1817,9 +1817,8 @@ public boolean deleteUserAccount(long accountId) { if (caller.getId() == accountId) { Domain domain = _domainDao.findById(account.getDomainId()); - throw new InvalidParameterValueException(String.format("The caller is requesting to delete their own account. As a security measure, ACS will not allow this " + - "operation." + - "To delete account %s (ID: %s, Domain: %s), request to another user with permission to execute the operation.", + throw new InvalidParameterValueException(String.format("Deletion of your own account is not allowed. To delete account %s (ID: %s, Domain: %s), " + + "request to another user with permissions to perform the operation.", account.getAccountName(), account.getUuid(), domain.getUuid())); } @@ -1843,7 +1842,7 @@ public boolean deleteUserAccount(long accountId) { return deleteAccount(account, callerUserId, caller); } - private boolean isDeleteNeeded(AccountVO account, long accountId, Account caller) { + protected boolean isDeleteNeeded(AccountVO account, long accountId, Account caller) { if (account == null) { logger.info(String.format("The account, identified by id %d, doesn't exist", accountId )); return false; diff --git a/server/src/test/java/com/cloud/user/AccountManagerImplTest.java b/server/src/test/java/com/cloud/user/AccountManagerImplTest.java index d98a4f8f0587..81bfe23189a2 100644 --- a/server/src/test/java/com/cloud/user/AccountManagerImplTest.java +++ b/server/src/test/java/com/cloud/user/AccountManagerImplTest.java @@ -198,6 +198,38 @@ public void deleteUserAccountCleanup() { Mockito.verify(_accountDao, Mockito.atLeastOnce()).markForCleanup(Mockito.eq(42l)); } + @Test (expected = InvalidParameterValueException.class) + public void deleteUserAccountTestIfAccountIdIsEqualToCallerIdShouldThrowException() { + try (MockedStatic callContextMocked = Mockito.mockStatic(CallContext.class)) { + CallContext callContextMock = Mockito.mock(CallContext.class); + callContextMocked.when(CallContext::current).thenReturn(callContextMock); + long accountId = 1L; + + Mockito.doReturn(accountVoMock).when(callContextMock).getCallingAccount(); + Mockito.doReturn(accountVoMock).when(_accountDao).findById(Mockito.anyLong()); + Mockito.doReturn(domainVoMock).when(_domainDao).findById(Mockito.anyLong()); + Mockito.doReturn(1L).when(accountVoMock).getId(); + + accountManagerImpl.deleteUserAccount(accountId); + } + } + @Test + public void deleteUserAccountTestIfAccountIdIsNotEqualToCallerAccountIdShouldNotThrowException() { + try (MockedStatic callContextMocked = Mockito.mockStatic(CallContext.class)){ + CallContext callContextMock = Mockito.mock(CallContext.class); + callContextMocked.when((CallContext::current)).thenReturn(callContextMock); + long accountId = 1L; + + Mockito.doReturn(accountVoMock).when(callContextMock).getCallingAccount(); + Mockito.doReturn(accountVoMock).when(_accountDao).findById(Mockito.anyLong()); + Mockito.doReturn(2L).when(accountVoMock).getId(); + Mockito.doReturn(true).when(accountManagerImpl).isDeleteNeeded(Mockito.any(), Mockito.anyLong(), Mockito.any()); + Mockito.doReturn(new ArrayList()).when(_projectAccountDao).listAdministratedProjectIds(Mockito.anyLong()); + + accountManagerImpl.deleteUserAccount(accountId); + } + } + @Test (expected = InvalidParameterValueException.class) public void deleteUserTestIfUserIdIsEqualToCallerIdShouldThrowException() { try (MockedStatic callContextMocked = Mockito.mockStatic(CallContext.class)) { From 9d1dc28e7844ee07c2f36b6b7f1299bae59b59c0 Mon Sep 17 00:00:00 2001 From: Lucas Martins <56271185+lucas-a-martins@users.noreply.github.com> Date: Tue, 23 Jul 2024 14:28:16 -0300 Subject: [PATCH 4/6] apply suggestion MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Co-authored-by: Bernardo De Marco Gonçalves --- server/src/test/java/com/cloud/user/AccountManagerImplTest.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/server/src/test/java/com/cloud/user/AccountManagerImplTest.java b/server/src/test/java/com/cloud/user/AccountManagerImplTest.java index 81bfe23189a2..404a8035c2ba 100644 --- a/server/src/test/java/com/cloud/user/AccountManagerImplTest.java +++ b/server/src/test/java/com/cloud/user/AccountManagerImplTest.java @@ -215,7 +215,7 @@ public void deleteUserAccountTestIfAccountIdIsEqualToCallerIdShouldThrowExceptio } @Test public void deleteUserAccountTestIfAccountIdIsNotEqualToCallerAccountIdShouldNotThrowException() { - try (MockedStatic callContextMocked = Mockito.mockStatic(CallContext.class)){ + try (MockedStatic callContextMocked = Mockito.mockStatic(CallContext.class)) { CallContext callContextMock = Mockito.mock(CallContext.class); callContextMocked.when((CallContext::current)).thenReturn(callContextMock); long accountId = 1L; From f3fd289fd96a69d0add6c4c5ebbb3713e606df77 Mon Sep 17 00:00:00 2001 From: Lucas Martins <56271185+lucas-a-martins@users.noreply.github.com> Date: Tue, 23 Jul 2024 14:29:59 -0300 Subject: [PATCH 5/6] remove extra parentheses MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Co-authored-by: Bernardo De Marco Gonçalves --- server/src/test/java/com/cloud/user/AccountManagerImplTest.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/server/src/test/java/com/cloud/user/AccountManagerImplTest.java b/server/src/test/java/com/cloud/user/AccountManagerImplTest.java index 404a8035c2ba..364cb29193a1 100644 --- a/server/src/test/java/com/cloud/user/AccountManagerImplTest.java +++ b/server/src/test/java/com/cloud/user/AccountManagerImplTest.java @@ -217,7 +217,7 @@ public void deleteUserAccountTestIfAccountIdIsEqualToCallerIdShouldThrowExceptio public void deleteUserAccountTestIfAccountIdIsNotEqualToCallerAccountIdShouldNotThrowException() { try (MockedStatic callContextMocked = Mockito.mockStatic(CallContext.class)) { CallContext callContextMock = Mockito.mock(CallContext.class); - callContextMocked.when((CallContext::current)).thenReturn(callContextMock); + callContextMocked.when(CallContext::current).thenReturn(callContextMock); long accountId = 1L; Mockito.doReturn(accountVoMock).when(callContextMock).getCallingAccount(); From 6f80cb9a18e6cf2c0e357250788f88e548db76e1 Mon Sep 17 00:00:00 2001 From: Lucas Martins <56271185+lucas-a-martins@users.noreply.github.com> Date: Tue, 23 Jul 2024 14:30:13 -0300 Subject: [PATCH 6/6] Update server/src/test/java/com/cloud/user/AccountManagerImplTest.java MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Co-authored-by: Bernardo De Marco Gonçalves --- server/src/test/java/com/cloud/user/AccountManagerImplTest.java | 1 + 1 file changed, 1 insertion(+) diff --git a/server/src/test/java/com/cloud/user/AccountManagerImplTest.java b/server/src/test/java/com/cloud/user/AccountManagerImplTest.java index 364cb29193a1..8ffd42861840 100644 --- a/server/src/test/java/com/cloud/user/AccountManagerImplTest.java +++ b/server/src/test/java/com/cloud/user/AccountManagerImplTest.java @@ -213,6 +213,7 @@ public void deleteUserAccountTestIfAccountIdIsEqualToCallerIdShouldThrowExceptio accountManagerImpl.deleteUserAccount(accountId); } } + @Test public void deleteUserAccountTestIfAccountIdIsNotEqualToCallerAccountIdShouldNotThrowException() { try (MockedStatic callContextMocked = Mockito.mockStatic(CallContext.class)) {