server: simplify role change validation by shwstppr · Pull Request #9173 · apache/cloudstack

shwstppr · 2024-06-05T08:27:34Z

Description

Fixes#9015

Simplifies role change checks with the following conditions:

Caller should not be of Unknown role type
New role's type should not be Unknown
Caller should not be able to escalate or de-escalate an account's role which is of higher role type
New role should not be of type Admin with domain other than ROOT domain

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

Screenshots (if appropriate):

How Has This Been Tested?

How did you try to break this feature and the system with this change?

Fixesapache#9015 Signed-off-by: Abhishek Kumar <abhishek.mrt22@gmail.com>

shwstppr · 2024-06-05T08:35:05Z

@blueorangutan package

codecov · 2024-06-05T08:35:46Z

Codecov Report

Attention: Patch coverage is 80.00000% with 3 lines in your changes missing coverage. Please review.

Project coverage is 15.12%. Comparing base (ef1a58d) to head (690188c).
Report is 10 commits behind head on 4.19.

Files with missing lines	Patch %	Lines
...c/main/java/com/cloud/user/AccountManagerImpl.java	80.00%	1 Missing and 2 partials ⚠️

Additional details and impacted files

@@ Coverage Diff @@## 4.19 #9173 +/- ##
=========================================
Coverage 15.12% 15.12% - Complexity 11255 11261 +6 
=========================================
Files 5408 5408 Lines 473838 473844 +6 Branches 57770 57774 +4 =========================================
+ Hits 71676 71687 +11 + Misses 394165 394157 -8 - Partials 7997 8000 +3

Flag	Coverage Δ
uitests	`4.30% <ø> (ø)`
unittests	`15.84% <80.00%> (+<0.01%)`	⬆️

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.

blueorangutan · 2024-06-05T08:36:03Z

@shwstppr 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.

Signed-off-by: Abhishek Kumar <abhishek.mrt22@gmail.com>

blueorangutan · 2024-06-05T09:00:44Z

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

shwstppr · 2024-06-05T09:08:54Z

@blueorangutan package

blueorangutan · 2024-06-05T09:10:03Z

@shwstppr 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 · 2024-06-05T10:25:03Z

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

DaanHoogland

clgtm, good cleanup

Co-authored-by: dahn <daan.hoogland@gmail.com>

DaanHoogland · 2024-06-06T07:13:29Z

@blueorangutan test

blueorangutan · 2024-06-06T07:14:03Z

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

blueorangutan · 2024-06-06T19:32:46Z

[SF] Trillian test result (tid-10374)
Environment: kvm-centos7 (x2), Advanced Networking with Mgmt server 7
Total time taken: 42525 seconds
Marvin logs: https://github.com/blueorangutan/acs-prs/releases/download/trillian/pr9173-t10374-kvm-centos7.zip
Smoke tests completed. 125 look OK, 6 have errors, 0 did not run
Only failed and skipped tests results shown below:

Test	Result	Time (s)	Test File
test_DeployVmAntiAffinityGroup_in_project	`Error`	85.90	test_affinity_groups_projects.py
test_DeployVmAntiAffinityGroup	`Error`	35.64	test_affinity_groups.py
test_03_deploy_and_scale_kubernetes_cluster	`Failure`	26.74	test_kubernetes_clusters.py
test_08_upgrade_kubernetes_ha_cluster	`Failure`	0.06	test_kubernetes_clusters.py
test_01_non_strict_host_anti_affinity	`Failure`	116.25	test_nonstrict_affinity_group.py
test_02_non_strict_host_affinity	`Error`	82.31	test_nonstrict_affinity_group.py
test_02_trigger_shutdown	`Failure`	346.80	test_safe_shutdown.py
test_hostha_enable_ha_when_host_in_maintenance	`Error`	302.83	test_hostha_kvm.py

yadvr

LGTM - didn't test it though

shwstppr · 2024-06-13T07:49:09Z

@DaanHoogland @rohityadavcloud
Thanks for the review but I'm not sure if we agree on the behaviour which is why this is still in draft. I will try to summarise impact of this change

ROOT admin can promote or demote all roles except when it tries to make a domain-admin from a subdomain to admin there will be an error
Domain-admins (if allowed based on API permission) can promote or demote an account to roles lower than domain-admin level. They won't be able to make a normal user account into domain-admin or vice-versa.
Normal user accounts (if allowed based on API permission) can promote or demote an account to roles lower than normal account level (probably none).

DaanHoogland · 2024-06-13T08:27:56Z

Thanks @shwstppr

Domain-admins (if allowed based on API permission) can promote or demote an account to roles lower than domain-admin level. They won't be able to make a normal user account into domain-admin or vice-versa.

I would expect a Domain Admin to promote a user up to their own level.

Normal user accounts (if allowed based on API permission) can promote or demote an account to roles lower than normal account level (probably none).

well, that mean they can make other users read only. I am not sure if this is desired.

shwstppr · 2024-06-13T08:40:14Z

I would expect a Domain Admin to promote a user up to their own level.

I feel this shouldn't be allowed. It should be done by account with higher privileges, ROOT admin here. We can change if others agree on the same.

well, that mean they can make other users read only. I am not sure if this is desired.

No, they won't be able to. Check is based on the RoleType (https://github.com/apache/cloudstack/blob/4.19/api/src/main/java/org/apache/cloudstack/acl/RoleType.java#L30-L34). So, they won't be able to do anything even if API allows unless we add a new RoleType in the code.

shwstppr · 2024-07-22T06:47:53Z

@blueorangutan package

blueorangutan · 2024-07-22T06:50:05Z

@shwstppr 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 · 2024-07-22T08:53:59Z

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

blueorangutan · 2024-09-18T10:42:04Z

@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 · 2024-09-18T11:45:43Z

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

DaanHoogland · 2024-09-18T12:39:01Z

@blueorangutan test

blueorangutan · 2024-09-18T12:40:03Z

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

blueorangutan · 2024-09-19T02:14:32Z

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

Test	Result	Time (s)	Test File

DaanHoogland · 2024-11-08T15:05:28Z

@shwstppr , do we still have functional doubts on the current state of the code? (or can we postpone further discussion to a next issue/PR)

github-actions · 2024-12-04T07:23:27Z

This pull request has merge conflicts. Dear author, please fix the conflicts and sync your branch with the base branch.

…check

shwstppr · 2024-12-04T07:31:25Z

@DaanHoogland I think this is okay from my side but will need some testing.

@blueorangutan package

blueorangutan · 2024-12-04T07:32:03Z

@shwstppr 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.

DaanHoogland · 2024-12-04T07:40:10Z

thanks @shwstppr , on the list ;)

blueorangutan · 2024-12-04T08:56:42Z

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

DaanHoogland · 2024-12-04T09:25:01Z

@blueorangutan test keepEnv

blueorangutan · 2024-12-04T09:26:04Z

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

blueorangutan · 2024-12-05T00:40:24Z

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

Test	Result	Time (s)	Test File

DaanHoogland · 2024-12-14T19:25:58Z

tested with a user-, user+, domadmin- and domadmin+ role to see wich was possible.
I gave the domadmin roles updateRole and updateRolePermissions to try and edit roles betond their priviledges.
the - accounts had no activateProject permission
and the + account had createAccount
the domadmin- could not assign a + role to a user
no rootadmin could be assigned in a child domain.

* 4.20: VR: apply iptables rules when add/remove static routes (#10064) Certificate and VM hostname validation improvements (#10051) set ulimit for server according to redhat spec (#10040) kvm-storage: provide isVMMigrate information to storage plugins (#10093) Allow config drive deletion of migrated VM, on host maintenance (#10045) linstor: improve heartbeat check with also asking linstor (#10105) server: simplify role change validation (#9173) UI: create VPC network offering with conserve mode (#10082) server: fix typo removeaccessvpn in VirtualRouterElement (#10086) UI: remove duplicated Instance Name in Public IP details page (#10087) UI: Fixes in the Usage UI (#10000) SAML2: add cookie with HttpOnly too #10013 (#10047) ui: Allow font-awesome icon usage and optimise icon size inconsistency (#9744)

Signed-off-by: Abhishek Kumar <abhishek.mrt22@gmail.com> Co-authored-by: dahn <daan.hoogland@gmail.com>

server: simplify role change validation
e781150
Fixesapache#9015 Signed-off-by: Abhishek Kumar <abhishek.mrt22@gmail.com>

shwstppr linked an issue Jun 5, 2024 that may be closed by this pull request
admin should be allowed to elevate user from role User to Domain Admin#9015
Closed

shwstppr mentioned this pull request Jun 5, 2024
admin should be allowed to elevate user from role User to Domain Admin#9015
Closed

fix build failures
1064cb7
Signed-off-by: Abhishek Kumar <abhishek.mrt22@gmail.com>

DaanHoogland approved these changes Jun 5, 2024
View reviewed changes

Comment threadserver/src/main/java/com/cloud/user/AccountManagerImpl.java Outdated

refactor error message
ccb30ca
Co-authored-by: dahn <daan.hoogland@gmail.com>

shwstppr commented Jun 5, 2024
View reviewed changes

Comment threadserver/src/main/java/com/cloud/user/AccountManagerImpl.java Outdated

Update server/src/main/java/com/cloud/user/AccountManagerImpl.java
1bd9766

yadvr added this to the 4.19.1.0 milestone Jun 10, 2024

yadvr approved these changes Jun 13, 2024
View reviewed changes

DaanHoogland modified the milestones: 4.19.1.0, 4.19.2 Jun 18, 2024

Merge branch '4.19' into simplify-role-change-check
de8a1c2

github-actionsBot added the status:has-conflicts label Aug 6, 2024

github-actionsBot added the status:has-conflicts label Dec 4, 2024

shwstppr added 2 commits December 4, 2024 12:57

Merge remote-tracking branch 'apache/4.19' into simplify-role-change-…
bc1b2eb
…check

Merge remote-tracking branch 'apache/4.19' into simplify-role-change-…
690188c
…check

github-actionsBot removed the status:has-conflicts label Dec 4, 2024

DaanHoogland added the status:needs-testing label Dec 4, 2024

DaanHoogland marked this pull request as ready for review December 4, 2024 07:39

DaanHoogland assigned DaanHoogland and unassigned DaanHoogland Dec 4, 2024

DaanHoogland assigned borisstoyanov Dec 10, 2024

DaanHoogland mentioned this pull request Dec 12, 2024
Can't create DomainAdmin Account to Domain #10096
Closed

DaanHoogland removed the status:needs-testing label Dec 14, 2024

DaanHoogland merged commit 8639ba8 into apache:4.19Dec 14, 2024

DaanHoogland deleted the simplify-role-change-check branch December 14, 2024 19:26

dhslove pushed a commit to ablecloud-team/ablestack-cloud that referenced this pull request Dec 26, 2024
server: simplify role change validation (apache#9173)
92ff36d
Signed-off-by: Abhishek Kumar <abhishek.mrt22@gmail.com> Co-authored-by: dahn <daan.hoogland@gmail.com>

Uh oh!

Conversation

shwstppr commented Jun 5, 2024• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Types of changes

Feature/Enhancement Scale or Bug Severity

Feature/Enhancement Scale

Bug Severity

Screenshots (if appropriate):

How Has This Been Tested?

How did you try to break this feature and the system with this change?

Uh oh!

shwstppr commented Jun 5, 2024

Uh oh!

codecovBot commented Jun 5, 2024• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

blueorangutan commented Jun 5, 2024

Uh oh!

blueorangutan commented Jun 5, 2024

Uh oh!

shwstppr commented Jun 5, 2024

Uh oh!

blueorangutan commented Jun 5, 2024

Uh oh!

blueorangutan commented Jun 5, 2024

Uh oh!

DaanHoogland left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

DaanHoogland commented Jun 6, 2024

Uh oh!

blueorangutan commented Jun 6, 2024

Uh oh!

blueorangutan commented Jun 6, 2024

Uh oh!

yadvr left a comment

Choose a reason for hiding this comment

Uh oh!

shwstppr commented Jun 13, 2024

Uh oh!

DaanHoogland commented Jun 13, 2024

Uh oh!

shwstppr commented Jun 13, 2024

Uh oh!

shwstppr commented Jul 22, 2024

Uh oh!

blueorangutan commented Jul 22, 2024

Uh oh!

blueorangutan commented Jul 22, 2024

Uh oh!

blueorangutan commented Sep 18, 2024

Uh oh!

blueorangutan commented Sep 18, 2024

Uh oh!

DaanHoogland commented Sep 18, 2024

Uh oh!

blueorangutan commented Sep 18, 2024

Uh oh!

blueorangutan commented Sep 19, 2024

Uh oh!

DaanHoogland commented Nov 8, 2024

Uh oh!

github-actionsBot commented Dec 4, 2024

Uh oh!

shwstppr commented Dec 4, 2024

Uh oh!

blueorangutan commented Dec 4, 2024

Uh oh!

DaanHoogland commented Dec 4, 2024

Uh oh!

blueorangutan commented Dec 4, 2024

Uh oh!

DaanHoogland commented Dec 4, 2024

Uh oh!

blueorangutan commented Dec 4, 2024

shwstppr commented Jun 5, 2024•
edited
Loading

codecovBot commented Jun 5, 2024•
edited
Loading