Limit listRoles API visibility#8639
Conversation
DaanHoogland
left a comment
DaanHoogland
left a comment
There was a problem hiding this comment.
clgtm and unit-test included, but i think some monkey testing is still in order
Codecov ReportAttention:
Additional details and impacted files@@ Coverage Diff @@
## main #8639 +/- ##
============================================
+ Coverage 30.37% 30.78% +0.41%
- Complexity 32633 33113 +480
============================================
Files 5352 5353 +1
Lines 374419 374635 +216
Branches 54609 54645 +36
============================================
+ Hits 113719 115348 +1629
+ Misses 245523 243994 -1529
- Partials 15177 15293 +116
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
| * @param rolePermissions the permissions of the caller role. | ||
| * @param roleToAccess the role that the caller role wants to access. | ||
| * @return True if the role can be accessed with the given permissions; false otherwise. |
There was a problem hiding this comment.
@hsato03, what do you think about naming these parameters as sourceRolePermissions and targetRole? IMO, it seems more intuitive. What do you think?
|
@blueorangutan package |
|
@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. |
|
Packaging result [SF]: ✔️ el7 ✔️ el8 ✔️ el9 ✔️ debian ✔️ suse15. SL-JID 9277 |
|
@DaanHoogland @sureshanaparti @rohityadavcloud @shwstppr could we run the CI here? |
|
@blueorangutan test |
|
@DaanHoogland a [SL] Trillian-Jenkins test job (centos7 mgmt + kvm-centos7) has been kicked to run smoke tests |
|
[SF] Trillian Build Failed (tid-9857) |
|
@blueorangutan package |
|
@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. |
|
Packaging result [SF]: ✔️ el7 ✔️ el8 ✔️ el9 ✔️ debian ✔️ suse15. SL-JID 9407 |
|
@blueorangutan test |
|
@DaanHoogland a [SL] Trillian-Jenkins test job (centos7 mgmt + kvm-centos7) has been kicked to run smoke tests |
|
[SF] Trillian test result (tid-10015)
|
bernardodemarco
left a comment
bernardodemarco
left a comment
There was a problem hiding this comment.
LGTM, I manually tested the PR in a local environment.
- I created a role called
list-roles-testbased on theUserrole. - I added the
listRolesAPI to the created role. - I created an account called
list-roles-test-accountwith the custom role. - Through CloudMonkey, I created a profile for the account and I executed the
synccommand. - I called the
listRolesAPI and I got the following output:
(test-roles-api) 🐱 > listRoles
{
"count": 2,
"role": [
{
"description": "Default user role",
"id": "8cead084-771e-11ee-8e59-5254003754dc",
"isdefault": true,
"ispublic": true,
"name": "User",
"type": "User"
},
{
"id": "ca565c1b-8589-4a78-b7eb-531d5505cadd",
"isdefault": false,
"ispublic": true,
"name": "list-roles-test",
"type": "User"
}
]
}- As it can be noticed, default admin roles and the ones with more permission than the
list-roles-testrole do not appear in the API's response.
Co-authored-by: Henrique Sato <henrique.sato@scclouds.com.br>
7 participants
Description
When calling the
listRolesAPI, users can see roles with more permissions than theirs.Therefore, the behavior of the
listRolesAPI was changed so that users can only see roles that their role has permission to access (roles with same and less permissions).Types of changes
Feature/Enhancement Scale or Bug Severity
Feature/Enhancement Scale
Bug Severity
Screenshots (if appropriate):
How Has This Been Tested?
listRolesAPI to it.listRolesAPI via CloudMonkey and verfied that the roles with more permissions than mine were not listed, such as default admin roles.