RBAC implementation for key value pair #55
Conversation
|
We couldn't resolve why circleci checks have failed. |
|
Please make sure CIs are green before asking for reviews. |
|
@ashwini-orchestral The CIs are failing with the following. If you click on "Details" above next to the failed job, do you see the details? |
| ) | ||
| UserRoleAssignment.add_or_update(role_assignment_db) | ||
|
|
||
| def test_user_resource_db_permissions(self): |
There was a problem hiding this comment.
This single test function covers too much. It's also missing some test cases.
Please separate current test function to multiple test functions (or classes/modules) for readability. Please also move the creation of required user/roles/grants to the test functions for readability. Right now, those are all put into the setup method and it's hard to figure out which users/roles/grants are for which tests.
Please systematically refactor the unit tests according to the following list.
-
System roles
- System Observer role on all system scoped KVPs
KeyValueSystemScopePermissionsResolverTestCase.test_observer_permissions_for_system_scope_kvps- Succeed on LIST permission
- Succeed on VIEW permission
- Fail on SET permission
- Fail on DELETE permission
- System Admin role on all system scoped KVPs
KeyValueSystemScopePermissionsResolverTestCase.test_admin_permissions_for_system_scope_kvps- Succeed on ALL permission
- System role should have permission to other users' KVPs (edited)
- Currently, StackStorm grants admin access permission to another users' KVPs. Keep this behavior to not introduce breaking changes.
KeyValueUserScopePermissionsResolverTestCase.test_admin_permissions_for_user_scoped_kvps
- Currently, StackStorm grants admin access permission to another users' KVPs. Keep this behavior to not introduce breaking changes.
- System Observer role on all system scoped KVPs
-
Default for user on user owned KVPs
- No specific roles and grants need to be defined for user to set/delete their own KVPs. Succeed on ALL permission to user owned KVPs.
KeyValueUserScopePermissionsResolverTestCase.test_user_permissions_for_user_scope_kvps - User should not have any permission to another user's KVPs.
KeyValueUserScopePermissionsResolverTestCase.test_user_permissions_on_another_user_kvps
- No specific roles and grants need to be defined for user to set/delete their own KVPs. Succeed on ALL permission to user owned KVPs.
-
User roles with permission on specific system scope KVPs
- User by default has no access to any system scope KVPs
KeyValueSystemScopePermissionsResolverTestCase.test_user_default_permissions_for_system_scope_kvps - Custom user role with LIST on specific system scope KVPs
KeyValueSystemScopePermissionsResolverTestCase.test_user_custom_read_permissions_for_system_scope_kvps- Succeed on LIST permission
- Should VIEW permission succeed or fail here?
- Fail on SET permission
- Fail on DELETE permission
- Custom user role with VIEW on specific system scope KVPs
KeyValueSystemScopePermissionsResolverTestCase.test_user_custom_read_permissions_for_system_scope_kvps- Succeed on LIST permission
- Succeed on VIEW permission
- Fail on SET permission
- Fail on DELETE permission
- Custom user role with SET on specific system scope KVPs
KeyValueSystemScopePermissionsResolverTestCase.test_user_custom_set_permissions_for_system_scope_kvps- Succeed on LIST permission
- Succeed on VIEW permission
- Succeed on SET permission
- Fail on DELETE permission
- Custom user role with DELETE on specific system scope KVPs
KeyValueSystemScopePermissionsResolverTestCase.test_user_custom_delete_permissions_for_system_scope_kvps- Succeed on LIST permission
- Succeed on VIEW permission
- Fail on SET permission (requires explicit set on system kvps)
- Succeed on DELETE permission
- User by default has no access to any system scope KVPs
There was a problem hiding this comment.
@ashwini-orchestral Please review the outline above. I highlighted the unit tests for each of the test cases in the outline here.
|
@m4dcoder - We have refactored the unit test cases as per your comment. Please review and let us know your feedback. |
Rollback manual code formatting changes made by developer.
The removal of empty lines in docs section of codes make them not readable.
Add RBAC permission types for key value pairs so the changes at StackStorm/st2-rbac-backend#55 to introduce RBAC to KVP can pass CI.
By default, user has access to their own user scoped KVPs. Therefore, the list permission should be assigned automatically to users.
|
LGTM - with an imminent release of 3.6 do we need to make sure that this only goes in with the paired ST2 pair? |
Currently, st2 permits administrator to have access to another user's key value pairs. To avoid introducing breaking changes, make feature backward compatible.
Add the util method user_has_system_role to check if user has one of the system roles.
Refactor unit tests for key value API controller to match test cases in the unit tests for the key value permission resolvers.
Ensure that values are encrypted and can be decrypted if user has permission.
Temporary and should be replaced with main st2 repo before merge.
The PR depends on changes from a development branch. The target repo and branch was changed to the development branch temporary.
Implemented RBAC functionality and unit tests for key-value pairs for existing and new permission types. Previously, RBAC feature for key value pairs are not yet implemented.
RBAC is enabled in the st2.conf file. Access to a key value pair is checked in the KeyValuePair API controller.
This change requires KeyValuePair API changes at StackStorm/st2#5354.