create parameter to determine whether roles are public or private #6960

GaOrtiga · 2022-12-08T11:58:40Z

Description

ACS does not allow the operator to define the visibility of roles, meaning all of them are visible to all users, with the exception of Root Admin roles.

In order to address this situation, a new parameter public_role has been created in the following APIs: createRole, updateRole and importRole. This parameter adds a new property to the role, allowing it to be hidden from the users; so, when updating, importing or creating a new role it is possible for the operator to inform if it is public (visible to all users) or private (only visible to Root Admins and the creator of the role). Also, the behavior of the API listRoles has been adjusted to return the roles according to this new property.

The following are examples of the listings using a Root Admin account and a Domain Admin account.

Listing with a Root Admin account

(localcloud) 🐱 > list roles
{
  "count": 10,
  "role": [
    {
      "description": "Default root admin role",
      "id": "64b34ae1-26de-11ec-8dcf-5254005dcdac",
      "isdefault": true,
      "ispublic": false,
      "name": "Root Admin",
      "type": "Admin"
    },
    {
      "description": "Default resource admin role",
      "id": "64b38969-26de-11ec-8dcf-5254005dcdac",
      "isdefault": true,
      "ispublic": true,
      "name": "Resource Admin",
      "type": "ResourceAdmin"
    },
    {
      "description": "Default domain admin role",
      "id": "64b48e57-26de-11ec-8dcf-5254005dcdac",
      "isdefault": true,
      "ispublic": true,
      "name": "Domain Admin",
      "type": "DomainAdmin"
    },
    {
      "description": "Default user role",
      "id": "64b4ca77-26de-11ec-8dcf-5254005dcdac",
      "isdefault": true,
      "ispublic": true,
      "name": "User",
      "type": "User"
    },
    {
      "description": "Default read-only admin role",
      "id": "68ea06f5-26de-11ec-8dcf-5254005dcdac",
      "isdefault": true,
      "ispublic": false,
      "name": "Read-Only Admin - Default",
      "type": "Admin"
    },
    {
      "description": "Default read-only user role",
      "id": "68ea3701-26de-11ec-8dcf-5254005dcdac",
      "isdefault": true,
      "ispublic": true,
      "name": "Read-Only User - Default",
      "type": "User"
    },
    {
      "description": "Default support admin role",
      "id": "68ea6ff9-26de-11ec-8dcf-5254005dcdac",
      "isdefault": true,
      "ispublic": false,
      "name": "Support Admin - Default",
      "type": "Admin"
    },
    {
      "description": "Default support user role",
      "id": "68eaab60-26de-11ec-8dcf-5254005dcdac",
      "isdefault": true,
      "ispublic": false,
      "name": "Support User - Default",
      "type": "User"
    },
    {
      "id": "2281d1f7-ba24-484e-bc1f-8519870dfc16",
      "isdefault": false,
      "ispublic": false,
      "name": "privaterole1",
      "type": "User"
    },
    {
      "id": "eebb8a56-6fce-4300-ade0-40a3e03a08a8",
      "isdefault": false,
      "ispublic": true,
      "name": "publicrole1",
      "type": "DomainAdmin"
    }
  ]
}

Listing with a Domain Admin account

(localcloud) 🐱 > list roles
{
  "count": 5,
  "role": [
    {
      "description": "Default resource admin role",
      "id": "64b38969-26de-11ec-8dcf-5254005dcdac",
      "isdefault": true,
      "ispublic": true,
      "name": "Resource Admin",
      "type": "ResourceAdmin"
    },
    {
      "description": "Default domain admin role",
      "id": "64b48e57-26de-11ec-8dcf-5254005dcdac",
      "isdefault": true,
      "ispublic": true,
      "name": "Domain Admin",
      "type": "DomainAdmin"
    },
    {
      "description": "Default user role",
      "id": "64b4ca77-26de-11ec-8dcf-5254005dcdac",
      "isdefault": true,
      "ispublic": true,
      "name": "User",
      "type": "User"
    },
    {
      "description": "Default read-only user role",
      "id": "68ea3701-26de-11ec-8dcf-5254005dcdac",
      "isdefault": true,
      "ispublic": true,
      "name": "Read-Only User - Default",
      "type": "User"
    },
    {
      "id": "eebb8a56-6fce-4300-ade0-40a3e03a08a8",
      "isdefault": false,
      "ispublic": true,
      "name": "publicrole1",
      "type": "DomainAdmin"
    }
  ]
}

Example of the other APIs:

createRole

(localcloud) 🐱 > create role roleid=64b34ae1-26de-11ec-8dcf-5254005dcdac name=testRole ispublic=false description="test role"
{
  "role": {
    "description": "test role",
    "id": "777e8dde-6670-42e2-8328-876d6445cc7c",
    "ispublic": false,
    "name": "testRole",
    "type": "Admin"
  }
}

updateRole

(localcloud) 🐱 > update role id=777e8dde-6670-42e2-8328-876d6445cc7c ispublic=true 
{
  "role": {
    "description": "test role",
    "id": "777e8dde-6670-42e2-8328-876d6445cc7c",
    "ispublic": true,
    "name": "testRole",
    "type": "Admin"
  }
}

importRole

(localcloud) 🐱 > import role name=importedRoleTest ispublic=false rules[0].rule=* rules[0].permission=allow type=User
{
  "role": {
    "id": "54045972-0272-4ab8-93c2-c299331897db",
    "ispublic": false,
    "name": "importedRoleTest",
    "type": "User"
  }
}

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)

Feature/Enhancement Scale or Bug Severity

Feature/Enhancement Scale

Major
Minor

How Has This Been Tested?

I created two new roles using the createRole API. One with the parameter public_role set to false and one with it set to true. I verified that both roles were successfully created with the specified parameters.
I repeated the same steps from the first test, but this time using the importRole API instead. I verified that the roles were successfully imported with the specified parameters.
I updated the public_role parameter from two different roles using the updateRole API, the first one had the parameter as true and I updated it to false and the second had it as false and I updated it to true. Both were successfully updated.
I used the API listRoles using a Root Admin account and verified that every role was visible.
I used the API listRoles using a Domain Admin account and verified that it could not see private roles.
I created an account of the type user using a private role and verified that it could log in.
With this same account I created a new network, and a new VM, and verified that both worked normally.
I repeated tests 6 and 7 but this time with an account of the type Root Admin, and verified that it worked aswell.

codecov · 2022-12-08T12:50:15Z

Codecov Report

Merging #6960 (1963da6) into main (e035d73) will decrease coverage by 0.01%.
The diff coverage is 15.38%.

@@             Coverage Diff              @@
##               main    #6960      +/-   ##
============================================
- Coverage     12.70%   12.70%   -0.01%     
- Complexity     8691     8693       +2     
============================================
  Files          2729     2729              
  Lines        256608   256641      +33     
  Branches      39993    39994       +1     
============================================
- Hits          32605    32604       -1     
- Misses       219853   219889      +36     
+ Partials       4150     4148       -2

Impacted Files	Coverage Δ
...ava/org/apache/cloudstack/acl/dao/RoleDaoImpl.java	`0.00% <0.00%> (ø)`
...ava/org/apache/cloudstack/acl/RoleManagerImpl.java	`19.52% <23.33%> (-1.80%)`	⬇️
...rc/main/java/org/apache/cloudstack/acl/RoleVO.java	`50.00% <25.00%> (-3.58%)`	⬇️

... and 1 file with indirect coverage changes

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

github-actions · 2022-12-12T08:12:50Z

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

github-actions · 2022-12-12T10:30:37Z

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

DaanHoogland · 2022-12-12T13:43:34Z

@GaOrtiga , can you look at the conflicts?

GaOrtiga · 2022-12-12T15:37:31Z

@GaOrtiga , can you look at the conflicts?

Done.

JoaoJandre

CLGTM

github-actions · 2022-12-13T08:17:28Z

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

DaanHoogland · 2022-12-16T08:27:27Z

Sorry @GaOrtiga , conflicts again. This happens a lot as we get close to release :(

GaOrtiga · 2022-12-16T17:43:23Z

Sorry @GaOrtiga , conflicts again. This happens a lot as we get close to release :(

Yes, I resolved them, thanks!

api/src/main/java/org/apache/cloudstack/api/command/admin/acl/ListRolesCmd.java

DaanHoogland · 2022-12-19T09:42:36Z

@blueorangutan package

blueorangutan · 2022-12-19T09:43:03Z

@DaanHoogland a 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 · 2022-12-19T11:24:09Z

Packaging result: ✔️ el7 ✔️ el8 ✔️ debian ✔️ suse15. SL-JID 5023

DaanHoogland · 2022-12-19T11:34:06Z

@blueorangutan test

DaanHoogland · 2022-12-19T11:34:38Z

@GaOrtiga do you think regression/integration tests as sensible for this change?

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

GaOrtiga · 2022-12-19T11:56:19Z

@GaOrtiga do you think regression/integration tests as sensible for this change?

Do you mean for the PR as a whole or for this last change submitted?

If it is for the whole PR, I have run some regression tests and all of them worked out properly, however if you have concerns about any specific functionality, I can run some more.

If it is for this last change I don´t think any other testing is necessary, since the arguments deleted were really not being used.

DaanHoogland · 2022-12-19T13:44:04Z

No, I meant for the functionality in this PR in general. And I mean in an automated fashion so they can serve as regression tests in the future. I.e. automate the scenario you described under "How Has This Been Tested?".

GaOrtiga · 2022-12-19T15:19:35Z

@DaanHoogland Oh, I see.
Okay, I will look into it.

blueorangutan · 2022-12-20T01:46:33Z

Trillian test result (tid-5576)
Environment: kvm-centos7 (x2), Advanced Networking with Mgmt server 7
Total time taken: 41973 seconds
Marvin logs: https://github.com/blueorangutan/acs-prs/releases/download/trillian/pr6960-t5576-kvm-centos7.zip
Smoke tests completed. 103 look OK, 2 have errors, 0 did not run
Only failed and skipped tests results shown below:

Test	Result	Time (s)	Test File
test_create_pvlan_network	`Error`	0.05	test_pvlan.py
test_08_upgrade_kubernetes_ha_cluster	`Failure`	577.10	test_kubernetes_clusters.py

test/integration/smoke/test_private_roles.py

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

github-actions · 2023-04-15T18:09:07Z

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

sonarqubecloud · 2023-04-19T14:33:22Z

SonarCloud Quality Gate failed.

0 Bugs
0 Vulnerabilities
0 Security Hotspots
3 Code Smells

33.0% Coverage
0.0% Duplication

GaOrtiga · 2023-04-25T18:49:28Z

@DaanHoogland Can we proceed with this PR or is there any other concern regarding it?

DaanHoogland · 2023-04-28T08:58:42Z

@DaanHoogland Can we proceed with this PR or is there any other concern regarding it?

no concerns

DaanHoogland · 2023-04-28T08:58:57Z

@blueorangutan package

blueorangutan · 2023-04-28T09:00:03Z

@DaanHoogland a 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 · 2023-04-28T10:05:46Z

Packaging result: ✔️ el7 ✔️ el8 ✔️ el9 ✔️ debian ✔️ suse15. SL-JID 5996

DaanHoogland · 2023-04-28T10:22:25Z

@blueorangutan test

blueorangutan · 2023-04-28T10:24:05Z

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

blueorangutan · 2023-04-28T23:17:28Z

Trillian test result (tid-6488)
Environment: kvm-centos7 (x2), Advanced Networking with Mgmt server 7
Total time taken: 44847 seconds
Marvin logs: https://github.com/blueorangutan/acs-prs/releases/download/trillian/pr6960-t6488-kvm-centos7.zip
Smoke tests completed. 110 look OK, 0 have errors, 0 did not run
Only failed and skipped tests results shown below:

Test	Result	Time (s)	Test File

shwstppr · 2023-06-01T11:14:54Z

@DaanHoogland this has been merged in main branch but the schema changes are in 4.18.0 to 4.18.1 upgrade path

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

DaanHoogland · 2023-06-01T11:38:22Z

@DaanHoogland this has been merged in main branch but the schema changes are in 4.18.0 to 4.18.1 upgrade path

thanks @shwstppr , I missed that. I can revert, but I see you added a commit on top of this. What do you propose we do?

DaanHoogland · 2023-06-01T11:40:47Z

Note how the smoke tests passed and didn't warn me about my error. An upgrade test would have caught this :|

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

boring-cyborg bot added the component:api label Dec 8, 2022

stephankruggg approved these changes Dec 8, 2022

View reviewed changes

github-actions bot added the status:has-conflicts label Dec 12, 2022

github-actions bot removed the status:has-conflicts label Dec 12, 2022

JoaoJandre approved these changes Dec 12, 2022

View reviewed changes

github-actions bot added the status:has-conflicts label Dec 13, 2022

create parameter to determine whether roles are public or private

f4d435f

GaOrtiga force-pushed the create_public_parameter_on_roles branch from 73fb2bb to f4d435f Compare December 16, 2022 17:38

github-actions bot removed the status:has-conflicts label Dec 16, 2022

DaanHoogland reviewed Dec 19, 2022

View reviewed changes

api/src/main/java/org/apache/cloudstack/api/command/admin/acl/ListRolesCmd.java Outdated Show resolved Hide resolved

Update api/src/main/java/org/apache/cloudstack/api/command/admin/acl/…

4ed5ad2

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

BryanMLima approved these changes Dec 30, 2022

View reviewed changes

github-actions bot added the status:has-conflicts label Jan 4, 2023

boring-cyborg bot added component:marvin Python Warning... Python code Ahead! labels Apr 4, 2023

github-actions bot removed the status:has-conflicts label Apr 4, 2023

fix class name

3a09ff9

GaOrtiga requested a review from DaanHoogland April 4, 2023 12:23

DaanHoogland reviewed Apr 5, 2023

View reviewed changes

GaOrtiga and others added 2 commits April 6, 2023 10:32

Apply suggestions from code review

39f3ed3

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

Update test_private_roles.py

5946fcc

github-actions bot added the status:has-conflicts label Apr 15, 2023

Gabriel added 2 commits April 19, 2023 09:28

Merge branch 'main' into create_public_parameter_on_roles

52b490b

fix merge

1963da6

github-actions bot removed the status:has-conflicts label Apr 19, 2023

DaanHoogland merged commit 8b5bfb1 into apache:main May 1, 2023

shwstppr added a commit to shapeblue/cloudstack that referenced this pull request Jun 1, 2023

engine-schema: fix schema change for apache#6960

7c3edf1

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

shwstppr mentioned this pull request Jun 1, 2023

engine-schema: fix schema change for #6960 #7579

Merged

12 tasks

rohityadavcloud pushed a commit that referenced this pull request Jun 1, 2023

engine-schema: fix schema change for #6960 (#7579)

841fd72

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

hsato03 mentioned this pull request Sep 18, 2023

ui: extend role isPublic parameter #7973

Merged

10 tasks

create parameter to determine whether roles are public or private #6960

create parameter to determine whether roles are public or private #6960

Uh oh!

Conversation

GaOrtiga commented Dec 8, 2022 • 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

How Has This Been Tested?

Uh oh!

codecov bot commented Dec 8, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

github-actions bot commented Dec 12, 2022

Uh oh!

github-actions bot commented Dec 12, 2022

Uh oh!

DaanHoogland commented Dec 12, 2022

Uh oh!

GaOrtiga commented Dec 12, 2022

Uh oh!

JoaoJandre left a comment

Choose a reason for hiding this comment

Uh oh!

github-actions bot commented Dec 13, 2022

Uh oh!

DaanHoogland commented Dec 16, 2022

Uh oh!

GaOrtiga commented Dec 16, 2022

Uh oh!

Uh oh!

DaanHoogland commented Dec 19, 2022

Uh oh!

blueorangutan commented Dec 19, 2022

Uh oh!

blueorangutan commented Dec 19, 2022

Uh oh!

DaanHoogland commented Dec 19, 2022

Uh oh!

DaanHoogland commented Dec 19, 2022

Uh oh!

GaOrtiga commented Dec 19, 2022

Uh oh!

DaanHoogland commented Dec 19, 2022

Uh oh!

GaOrtiga commented Dec 19, 2022

Uh oh!

blueorangutan commented Dec 20, 2022

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

github-actions bot commented Apr 15, 2023

Uh oh!

sonarqubecloud bot commented Apr 19, 2023

Uh oh!

GaOrtiga commented Apr 25, 2023

Uh oh!

DaanHoogland commented Apr 28, 2023

Uh oh!

DaanHoogland commented Apr 28, 2023

Uh oh!

blueorangutan commented Apr 28, 2023

Uh oh!

blueorangutan commented Apr 28, 2023

Uh oh!

DaanHoogland commented Apr 28, 2023

Uh oh!

blueorangutan commented Apr 28, 2023

Uh oh!

blueorangutan commented Apr 28, 2023

Uh oh!

shwstppr commented Jun 1, 2023

Uh oh!

GaOrtiga commented Dec 8, 2022 •

edited

Loading

codecov bot commented Dec 8, 2022 •

edited

Loading