Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Implementation of RBAC for KeyValuePair #5354

Merged
merged 51 commits into from
Dec 10, 2021

Conversation

ashwini-orchestral
Copy link
Contributor

@ashwini-orchestral ashwini-orchestral commented Sep 9, 2021

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.

  • Existing permission types - KEY_VALUE_VIEW, KEY_VALUE_SET, KEY_VALUE_DELETE
  • New permission types - KEY_VALUE_LIST, KEY_VALUE_ALL

RBAC is enabled in the st2.conf file. Access to a key value pair is checked in the KeyValuePair API controller.

  • SET and DELETE permissions will automatically grant LIST and VIEW permissions.
  • By default, user has access to his/her own user scoped KVPs without requiring specific permission grant.
  • A non-admin user can be explicitly granted permission to one or more system scoped KVPs.
  • A non-admin user cannot access another user's KVPs.
  • By default, admin and system_admin has ALL access to system scoped KVPs.
  • Admin has full access to another user's KVPs (behavior in current version).

This change requires RBAC backend support @ PR StackStorm/st2-rbac-backend#55.

@pull-request-size pull-request-size bot added the size/M PR that changes 30-99 lines. Good size to review. label Sep 9, 2021
@m4dcoder m4dcoder added this to the 3.6.0 milestone Sep 13, 2021
@pull-request-size pull-request-size bot added size/L PR that changes 100-499 lines. Requires some effort to review. and removed size/M PR that changes 30-99 lines. Good size to review. labels Sep 24, 2021
Copy link
Contributor

@m4dcoder m4dcoder left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Where are the unit tests?

Clean up and simplify the put method of the key value pair API.
Remove logic from the code that is redundant or no longer applies.
Clean up and simplify the delete method of the key value pair API.
Remove logic from the code that is redundant or no longer applies.
Copy link
Member

@cognifloyd cognifloyd left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

Refactor get_all_system_kvp_names_for_user to ensure there's no leakage
of non key value pair resource type or resource uid of user scoped key
value pair.
Refactor RBAC unit tests for the key value API to ensure get_all is
working properly for different scopes and for admin/non-admin users.
@m4dcoder m4dcoder modified the milestones: 3.6.0, 3.7.0 Oct 3, 2021
@m4dcoder
Copy link
Contributor

m4dcoder commented Oct 3, 2021

Moving this feature to v3.7.0 to give more time for folks to soak this in.

Copy link
Contributor

@m4dcoder m4dcoder left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Blocking this temporary until post v3.6.0 release.

st2client/st2client/commands/resource.py Outdated Show resolved Hide resolved
@arm4b
Copy link
Member

arm4b commented Dec 9, 2021

@ashwini-orchestral The PR needs a Changelog. Please add one.

@m4dcoder Additionally, do we need documentation changes for this new feature as well?

@m4dcoder m4dcoder merged commit d71b157 into StackStorm:master Dec 10, 2021
@m4dcoder m4dcoder deleted the rbac_keyvaluepair branch December 10, 2021 17:16
@m4dcoder
Copy link
Contributor

@armab Corresponding PR for docs at StackStorm/st2docs#1092

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature RBAC security size/XL PR that changes 500-999 lines. Consider splitting work into several ones that easier to review.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants