Fix deleteAccount API to prevent deletion of the caller

[lucas-a-martins]

Description

ACS allows users to delete their own account by calling the deleteAccount API via CLI. Fixing this behavior has been previously suggested in this comment. Furthermore, there's already a feature to prevent a user from deleting their own account via UI; however, slower environments create a small window where it's possible for the user to click the button before it's disabled, allowing deletion even through the UI (see this comment).

This pull request addresses this issue by adding a validation that checks the UUID of the caller's account and compares it with the UUID of the account they are trying to delete. If they are the same, an exception is thrown.

Types of changes

• Breaking change (fix or feature that would cause existing functionality to change)
• New feature (non-breaking change which adds functionality)
• Bug fix (non-breaking change which fixes an issue)
• Enhancement (improves an existing feature and functionality)
• Cleanup (Code refactoring and cleanup, that may add test cases)
• build/CI

Feature/Enhancement Scale or Bug Severity

Feature/Enhancement Scale

• Major
• Minor

Bug Severity

• BLOCKER
• Critical
• Major
• Minor
• Trivial

Screenshots (if appropriate):

How Has This Been Tested?

I tested by creating a new account (Temp) and attempted to delete it using a user in this same account via CloudMonkey.
As expected, an exception was thrown.

Next, I attempted to delete the account via the UI by rapidly clicking the delete icon. Once again, an exception was thrown without deleting the account.

"errortext": "The caller is requesting to delete their own account. As a security measure, ACS will not allow this operation.To delete account Temp (ID: 510c8c3a-f2ab-4e42-8e52-55ac968b7291, Domain: ab95574f-7857-4a44-9510-2c72ccc770fe), request to another user with permission to execute the operation."

[codecov]

Codecov Report

Attention: Patch coverage is 0% with 7 lines in your changes missing coverage. Please review.

Project coverage is 15.54%. Comparing base (9a73a2f) to head (6f80cb9).
Report is 501 commits behind head on main.

Files	Patch %	Lines
...c/main/java/com/cloud/user/AccountManagerImpl.java	0.00%	4 Missing ⚠️
...ck/api/command/admin/account/DeleteAccountCmd.java	0.00%	3 Missing ⚠️

❗ There is a different number of reports uploaded between BASE (9a73a2f) and HEAD (6f80cb9). Click for more details.

HEAD has 18 uploads less than BASE

Flag	BASE (9a73a2f)	HEAD (6f80cb9)
simulator-marvin-tests	17	0
unit-tests	1	0

Additional details and impacted files

@@              Coverage Diff              @@
##               main    #8743       +/-   ##
=============================================
- Coverage     30.98%   15.54%   -15.44%     
+ Complexity    33526    11978    -21548     
=============================================
  Files          5361     5494      +133     
  Lines        376250   481058   +104808     
  Branches      54933    59650     +4717     
=============================================
- Hits         116564    74779    -41785     
- Misses       244259   398019   +153760     
+ Partials      15427     8260     -7167     

Flag	Coverage Δ
simulator-marvin-tests	?
uitests	4.18% <ø> (-0.19%)	⬇️
unit-tests	?
unittests	16.31% <0.00%> (?)

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[sureshanaparti]

CLGTM

[DaanHoogland]

clgtm

[weizhouapache]

I think, on many platforms, users can delete their accounts, right?
of course there are some extra confirmation.

[lucas-a-martins]

I think, on many platforms, users can delete their accounts, right? of course there are some extra confirmation.

Hey @weizhouapache,

It's true that a user can delete their accounts on many plataforms, but on these plataforms the user is responsible for creating their account, which usually isn't the case here. In order to create an account, it is necessary to access another account with permission to perform the operation. This should be the same when trying to delete. If the user has the access, they don't need the API to allow deletion of the caller.

Additionally, a single account can be used by multiple users, so I don't think it's right to give every user the power to delete a whole account and, as a consequence, every user within that account. Futhermore, not applying this change could provide a way for the user to circumvent the changes made in the deleteUser API that prevent deletion of the caller (PR #8691), but with the possibility of deleting multiple users at the same time.

I agree that this kind of flexibility is interesting in scenarios where accounts are independent, being created, used and managed by a single user, however, usually this is not the case in ACS.

[lucas-a-martins]

Hey @weizhouapache,

Are there any further concerns regarding this implementation or can we move on this PR?

[weizhouapache]

Hey @weizhouapache,

Are there any further concerns regarding this implementation or can we move on this PR?

@lucas-a-martins
sorry I missed your reply.
looks good to me

[bernardodemarco]

LGTM, I manually tested this one in a local environment.

In the environment, I had a root admin account called admin2. I logged in with that account and tried to delete it through the UI. The following error message was returned:

Through CloudMonkey, I also logged in with the admin2 account and tried to delete it, but as expected, the operation was not successful.

delete account id=427f7d98-86a4-4228-a133-ce7b61ba4564
{
  "account": "admin2",
  "accountid": "427f7d98-86a4-4228-a133-ce7b61ba4564",
  "cmd": "org.apache.cloudstack.api.command.admin.account.DeleteAccountCmd",
  "completed": "2024-07-23T11:59:34+0000",
  "created": "2024-07-23T11:59:34+0000",
  "domainid": "ba820fc7-12ea-11ef-9500-d283eea8b15e",
  "domainpath": "ROOT",
  "jobid": "edf2edf8-1ea2-467c-b571-3c2926c8e89f",
  "jobinstanceid": "427f7d98-86a4-4228-a133-ce7b61ba4564",
  "jobinstancetype": "Account",
  "jobprocstatus": 0,
  "jobresult": {
    "errorcode": 431,
    "errortext": "Deletion of your own account is not allowed. To delete account admin2 (ID: 427f7d98-86a4-4228-a133-ce7b61ba4564, Domain: ba820fc7-12ea-11ef-9500-d283eea8b15e), request to another user with permissions to perform the operation."
  },
  "jobresultcode": 431,
  "jobresulttype": "object",
  "jobstatus": 2,
  "userid": "f7d6bb91-20a0-4c78-b414-b5f3ce7c6022"
}
🙈 Error: async API failed for job edf2edf8-1ea2-467c-b571-3c2926c8e89f

[DaanHoogland]

@blueorangutan package

[blueorangutan]

@DaanHoogland a [SL] Jenkins job has been kicked to build packages. It will be bundled with KVM, XenServer and VMware SystemVM templates. I'll keep you posted as I make progress.

[blueorangutan]

Packaging result [SF]: ✔️ el7 ✔️ el8 ✔️ el9 ✔️ debian ✔️ suse15. SL-JID 10447

[DaanHoogland]

@blueorangutan package

[blueorangutan]

@DaanHoogland a [SL] Jenkins job has been kicked to build packages. It will be bundled with KVM, XenServer and VMware SystemVM templates. I'll keep you posted as I make progress.

[blueorangutan]

Packaging result [SF]: ✔️ el7 ✔️ el8 ✔️ el9 ✔️ debian ✔️ suse15. SL-JID 10448

[JoaoJandre]

@blueorangutan package

[blueorangutan]

@JoaoJandre a [SL] Jenkins job has been kicked to build packages. It will be bundled with KVM, XenServer and VMware SystemVM templates. I'll keep you posted as I make progress.

[blueorangutan]

Packaging result [SF]: ✖️ el8 ✖️ el9 ✖️ debian ✖️ suse15. SL-JID 10645

[blueorangutan]

Packaging result [SF]: ✖️ el8 ✖️ el9 ✖️ debian ✖️ suse15. SL-JID 10660

[blueorangutan]

Packaging result [SF]: ✔️ el8 ✔️ el9 ✔️ debian ✔️ suse15. SL-JID 10679

[DaanHoogland]

@blueorangutan test

[blueorangutan]

@DaanHoogland a [SL] Trillian-Jenkins test job (ol8 mgmt + kvm-ol8) has been kicked to run smoke tests

[blueorangutan]

[SF] Trillian test result (tid-11093)
Environment: kvm-ol8 (x2), Advanced Networking with Mgmt server ol8
Total time taken: 48843 seconds
Marvin logs: https://github.com/blueorangutan/acs-prs/releases/download/trillian/pr8743-t11093-kvm-ol8.zip
Smoke tests completed. 137 look OK, 2 have errors, 0 did not run
Only failed and skipped tests results shown below:

Test	Result	Time (s)	Test File
test_DeployVmAffinityGroup	Error	1.49	test_affinity_groups.py
test_01_non_strict_host_anti_affinity	Error	2.63	test_nonstrict_affinity_group.py
test_02_non_strict_host_affinity	Error	3.65	test_nonstrict_affinity_group.py