You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@cloudstack.apache.org by "lucas-a-martins (via GitHub)" <gi...@apache.org> on 2024/03/04 17:39:44 UTC
[PR] Fix delete account [cloudstack]
lucas-a-martins opened a new pull request, #8743:
URL: https://github.com/apache/cloudstack/pull/8743
### 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](https://github.com/apache/cloudstack/pull/7242#issuecomment-1433260909). 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](https://github.com/apache/cloudstack/pull/7242#issuecomment-1444300925)).
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)
- [x] 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
- [x] 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."
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Fix `deleteAccount` API to prevent deletion of the caller [cloudstack]
Posted by "lucas-a-martins (via GitHub)" <gi...@apache.org>.
lucas-a-martins commented on PR #8743:
URL: https://github.com/apache/cloudstack/pull/8743#issuecomment-1994997510
> 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](https://github.com/apache/cloudstack/pull/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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Fix `deleteAccount` API to prevent deletion of the caller [cloudstack]
Posted by "blueorangutan (via GitHub)" <gi...@apache.org>.
blueorangutan commented on PR #8743:
URL: https://github.com/apache/cloudstack/pull/8743#issuecomment-2075464697
Packaging result [SF]: ✔️ el7 ✔️ el8 ✔️ el9 ✔️ debian ✔️ suse15. SL-JID 9410
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Fix `deleteAccount` API to prevent deletion of the caller [cloudstack]
Posted by "blueorangutan (via GitHub)" <gi...@apache.org>.
blueorangutan commented on PR #8743:
URL: https://github.com/apache/cloudstack/pull/8743#issuecomment-1981318832
@DaanHoogland a [SL] Trillian-Jenkins test job (centos7 mgmt + kvm-centos7) has been kicked to run smoke tests
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Fix `deleteAccount` API to prevent deletion of the caller [cloudstack]
Posted by "lucas-a-martins (via GitHub)" <gi...@apache.org>.
lucas-a-martins commented on PR #8743:
URL: https://github.com/apache/cloudstack/pull/8743#issuecomment-2061892867
Hey @weizhouapache,
Are there any further concerns regarding this implementation or can we move on this PR?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Fix `deleteAccount` API to prevent deletion of the caller [cloudstack]
Posted by "KlausDornsbach (via GitHub)" <gi...@apache.org>.
KlausDornsbach commented on PR #8743:
URL: https://github.com/apache/cloudstack/pull/8743#issuecomment-1977375572
clgtm
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Fix `deleteAccount` API to prevent deletion of the caller [cloudstack]
Posted by "DaanHoogland (via GitHub)" <gi...@apache.org>.
DaanHoogland commented on PR #8743:
URL: https://github.com/apache/cloudstack/pull/8743#issuecomment-2063257877
@blueorangutan alma9 kvm-alma9 keepEnv
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Fix `deleteAccount` API to prevent deletion of the caller [cloudstack]
Posted by "weizhouapache (via GitHub)" <gi...@apache.org>.
weizhouapache commented on PR #8743:
URL: https://github.com/apache/cloudstack/pull/8743#issuecomment-2061970915
> 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
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Fix `deleteAccount` API to prevent deletion of the caller [cloudstack]
Posted by "BryanMLima (via GitHub)" <gi...@apache.org>.
BryanMLima commented on PR #8743:
URL: https://github.com/apache/cloudstack/pull/8743#issuecomment-2075284780
@blueorangutan package
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Fix `deleteAccount` API to prevent deletion of the caller [cloudstack]
Posted by "lucas-a-martins (via GitHub)" <gi...@apache.org>.
lucas-a-martins commented on code in PR #8743:
URL: https://github.com/apache/cloudstack/pull/8743#discussion_r1513234550
##########
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 " +
Review Comment:
Hey @sureshanaparti, thanks for the suggestion. What do you think about this message instead?
`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.`
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Fix `deleteAccount` API to prevent deletion of the caller [cloudstack]
Posted by "blueorangutan (via GitHub)" <gi...@apache.org>.
blueorangutan commented on PR #8743:
URL: https://github.com/apache/cloudstack/pull/8743#issuecomment-1982570957
<b>[SF] Trillian test result (tid-9408)</b>
Environment: kvm-centos7 (x2), Advanced Networking with Mgmt server 7
Total time taken: 48295 seconds
Marvin logs: https://github.com/blueorangutan/acs-prs/releases/download/trillian/pr8743-t9408-kvm-centos7.zip
Smoke tests completed. 129 look OK, 0 have errors, 0 did not run
Only failed and skipped tests results shown below:
Test | Result | Time (s) | Test File
--- | --- | --- | ---
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Fix `deleteAccount` API to prevent deletion of the caller [cloudstack]
Posted by "DaanHoogland (via GitHub)" <gi...@apache.org>.
DaanHoogland commented on PR #8743:
URL: https://github.com/apache/cloudstack/pull/8743#issuecomment-1981316423
@blueorangutan test
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Fix `deleteAccount` API to prevent deletion of the caller [cloudstack]
Posted by "DaanHoogland (via GitHub)" <gi...@apache.org>.
DaanHoogland commented on PR #8743:
URL: https://github.com/apache/cloudstack/pull/8743#issuecomment-1988645977
@blueorangutan alma9 kvm-alma9 keepEnv
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Fix `deleteAccount` API to prevent deletion of the caller [cloudstack]
Posted by "weizhouapache (via GitHub)" <gi...@apache.org>.
weizhouapache commented on PR #8743:
URL: https://github.com/apache/cloudstack/pull/8743#issuecomment-1988765898
I think, on many platforms, users can delete their accounts, right?
of course there are some extra confirmation.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Fix `deleteAccount` API to prevent deletion of the caller [cloudstack]
Posted by "sureshanaparti (via GitHub)" <gi...@apache.org>.
sureshanaparti commented on PR #8743:
URL: https://github.com/apache/cloudstack/pull/8743#issuecomment-1980476507
@blueorangutan package
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Fix `deleteAccount` API to prevent deletion of the caller [cloudstack]
Posted by "blueorangutan (via GitHub)" <gi...@apache.org>.
blueorangutan commented on PR #8743:
URL: https://github.com/apache/cloudstack/pull/8743#issuecomment-1980604484
Packaging result [SF]: ✔️ el7 ✔️ el8 ✔️ el9 ✔️ debian ✔️ suse15. SL-JID 8864
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Fix `deleteAccount` API to prevent deletion of the caller [cloudstack]
Posted by "blueorangutan (via GitHub)" <gi...@apache.org>.
blueorangutan commented on PR #8743:
URL: https://github.com/apache/cloudstack/pull/8743#issuecomment-1980479319
@sureshanaparti 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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Fix `deleteAccount` API to prevent deletion of the caller [cloudstack]
Posted by "codecov[bot] (via GitHub)" <gi...@apache.org>.
codecov[bot] commented on PR #8743:
URL: https://github.com/apache/cloudstack/pull/8743#issuecomment-1978156898
## [Codecov](https://app.codecov.io/gh/apache/cloudstack/pull/8743?src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) Report
All modified and coverable lines are covered by tests :white_check_mark:
> Project coverage is 4.36%. Comparing base [(`3baa45b`)](https://app.codecov.io/gh/apache/cloudstack/commit/3baa45bc2a814e50dd655f160794cc29fbec8a5a?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) to head [(`49e29ed`)](https://app.codecov.io/gh/apache/cloudstack/pull/8743?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache).
> Report is 19 commits behind head on main.
<details><summary>Additional details and impacted files</summary>
```diff
@@ Coverage Diff @@
## main #8743 +/- ##
============================================
- Coverage 29.92% 4.36% -25.56%
============================================
Files 5355 361 -4994
Lines 375771 28831 -346940
Branches 54912 5037 -49875
============================================
- Hits 112455 1259 -111196
+ Misses 248321 27433 -220888
+ Partials 14995 139 -14856
```
| [Flag](https://app.codecov.io/gh/apache/cloudstack/pull/8743/flags?src=pr&el=flags&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | Coverage Δ | |
|---|---|---|
| [simulator-marvin-tests](https://app.codecov.io/gh/apache/cloudstack/pull/8743/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `?` | |
| [uitests](https://app.codecov.io/gh/apache/cloudstack/pull/8743/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `4.36% <ø> (ø)` | |
| [unit-tests](https://app.codecov.io/gh/apache/cloudstack/pull/8743/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `?` | |
Flags with carried forward coverage won't be shown. [Click here](https://docs.codecov.io/docs/carryforward-flags?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#carryforward-flags-in-the-pull-request-comment) to find out more.
</details>
[:umbrella: View full report in Codecov by Sentry](https://app.codecov.io/gh/apache/cloudstack/pull/8743?src=pr&el=continue&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache).
:loudspeaker: Have feedback on the report? [Share it here](https://about.codecov.io/codecov-pr-comment-feedback/?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache).
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Fix `deleteAccount` API to prevent deletion of the caller [cloudstack]
Posted by "sureshanaparti (via GitHub)" <gi...@apache.org>.
sureshanaparti commented on code in PR #8743:
URL: https://github.com/apache/cloudstack/pull/8743#discussion_r1512288320
##########
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 " +
Review Comment:
@lucas-a-martins May need to rephrase the message, something like
`Deletion of your own account %s (ID: %s, Domain: %s) is not allowed, request another user with permissions to perform the operation.`
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Fix `deleteAccount` API to prevent deletion of the caller [cloudstack]
Posted by "DaanHoogland (via GitHub)" <gi...@apache.org>.
DaanHoogland commented on PR #8743:
URL: https://github.com/apache/cloudstack/pull/8743#issuecomment-2076521303
@blueorangutan alma9 kvm-alma9 keepEnv
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Fix `deleteAccount` API to prevent deletion of the caller [cloudstack]
Posted by "blueorangutan (via GitHub)" <gi...@apache.org>.
blueorangutan commented on PR #8743:
URL: https://github.com/apache/cloudstack/pull/8743#issuecomment-2075287161
@BryanMLima 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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org