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.

Sign up for GitHub

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

Jump to bottom

RBAC: Cover plugin routes #80578

Merged
merged 15 commits into from
Jan 17, 2024
Merged

RBAC: Cover plugin routes #80578

merged 15 commits into from
Jan 17, 2024

Conversation

gamab
Copy link
Contributor

@gamab gamab commented Jan 15, 2024 •
edited
Loading

What is this feature?

This PR adds the possibility to protect plugin proxied routes with an RBAC action check.

Why do we need this feature?

This has been requested by plugin developers.

Who is this feature for?

[Add information on what kind of user the feature is for.]

Which issue(s) does this PR fix?:

Special notes for your reviewer:

We had covered in #57582 includes.

Schema updates: #80592

Once this is merged I need to update the plugin-tools docs

Please check that:

  • It works as expected from a user's perspective.
  • If this is a pre-GA feature, it is behind a feature toggle.
  • The docs are updated, and if this is a notable improvement, it's added to our What's New doc.

@gamab gamab self-assigned this Jan 15, 2024
@gamab gamab added no-backport Skip backport of PR no-changelog Skip including change in changelog/release notes labels Jan 15, 2024
@gamab gamab modified the milestones: 10.5.x, 10.4.x Jan 15, 2024
@gamab gamab added add to changelog and removed no-changelog Skip including change in changelog/release notes labels Jan 16, 2024
gamab
gamab commented Jan 16, 2024
View reviewed changes
pkg/plugins/plugindef/plugindef.cue Outdated
@@ -364,6 +364,9 @@ schemas: [{
reqSignedIn?: bool
reqRole?: string

// RBAC action the user must have to access the route
action?: string
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Do we prefer reqAction to be consistent with reqRole or action to be consistent with the includes' action field?

Copy link
Contributor

Choose a reason for hiding this comment

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

I'd go with reqAction to make it clear it's a requirement. Also an example will help for people to understand what this does.

Unrelated but does this support multiple actions or only one?

Copy link
Contributor Author

@gamab gamab Jan 17, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Ok 👌

Also an example will help for people to understand what this does.

What do you have in mind? Plugin example? Or plugin-tools docs update with an example?

Unrelated but does this support multiple actions or only one?

It supports only one action. We'll see if we ever need to support something more complex.

Copy link
Contributor

@andresmgot andresmgot Jan 17, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

What do you have in mind? Plugin example? Or plugin-tools docs update with an example?

For this case, I only mean in the comment: // RBAC action the user must have to access the route i.e. plugin-id.projects:read

It supports only one action

👍

@gamab gamab marked this pull request as ready for review January 16, 2024 09:18
@gamab gamab requested review from a team as code owners January 16, 2024 09:18
@gamab gamab requested review from wbrowne, andresmgot, oshirohugo, papagian, undef1nd and yangkb09 and removed request for a team January 16, 2024 09:18
andresmgot
andresmgot reviewed Jan 17, 2024
View reviewed changes
Copy link
Contributor

@andresmgot andresmgot left a comment

Choose a reason for hiding this comment

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

Code LGTM, I only have a suggestion regarding naming / docs

pkg/plugins/plugindef/plugindef.cue Outdated
@@ -364,6 +364,9 @@ schemas: [{
reqSignedIn?: bool
reqRole?: string

// RBAC action the user must have to access the route
action?: string
Copy link
Contributor

Choose a reason for hiding this comment

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

I'd go with reqAction to make it clear it's a requirement. Also an example will help for people to understand what this does.

Unrelated but does this support multiple actions or only one?

andresmgot
andresmgot approved these changes Jan 17, 2024
View reviewed changes
Copy link
Contributor

@andresmgot andresmgot left a comment

Choose a reason for hiding this comment

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

LGTM

gamab and others added 2 commits January 17, 2024 16:02
@gamab @andresmgot
Add example
ce7c6c4 
Co-authored-by: Andres Martinez Gotor <andres.martinez@grafana.com>
@gamab
Merge remote-tracking branch 'origin/main' into gamab/rbac/cover-routes
ddbb03f
@gamab gamab merged commit 6b95416 into main Jan 17, 2024
12 checks passed
@gamab gamab deleted the gamab/rbac/cover-routes branch January 17, 2024 15:32
s0lesurviv0r pushed a commit to s0lesurviv0r/grafana that referenced this pull request Feb 3, 2024
@gamab @andresmgot @s0lesurviv0r
RBAC: Cover plugin routes (grafana#80578)
a0e6b4c 
* RBAC: Cover plugin routes

* Action instead of ReqAction

* Fix test initializations

* Fix NewPluginProxy call

* Duplicate test to add RBAC checks

* Cover legacy access control as well

* Fix typo

* action -> reqAction

* Add example

Co-authored-by: Andres Martinez Gotor <andres.martinez@grafana.com>

---------

Co-authored-by: Andres Martinez Gotor <andres.martinez@grafana.com>
Ukochka pushed a commit that referenced this pull request Feb 14, 2024
@gamab @andresmgot @Ukochka
RBAC: Cover plugin routes (#80578)
0d302eb 
* RBAC: Cover plugin routes

* Action instead of ReqAction

* Fix test initializations

* Fix NewPluginProxy call

* Duplicate test to add RBAC checks

* Cover legacy access control as well

* Fix typo

* action -> reqAction

* Add example

Co-authored-by: Andres Martinez Gotor <andres.martinez@grafana.com>

---------

Co-authored-by: Andres Martinez Gotor <andres.martinez@grafana.com>
@aangelisc aangelisc modified the milestones: 10.4.x, 10.4.0 Mar 6, 2024
@BrewTestBot BrewTestBot mentioned this pull request Mar 6, 2024
Closed
@BrewTestBot BrewTestBot mentioned this pull request Mar 14, 2024
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@andresmgot andresmgot andresmgot approved these changes

@wbrowne wbrowne Awaiting requested review from wbrowne wbrowne is a code owner automatically assigned from grafana/plugins-platform-backend

@oshirohugo oshirohugo Awaiting requested review from oshirohugo oshirohugo is a code owner automatically assigned from grafana/plugins-platform-backend

@papagian papagian Awaiting requested review from papagian papagian was automatically assigned from grafana/backend-platform

@undef1nd undef1nd Awaiting requested review from undef1nd undef1nd was automatically assigned from grafana/backend-platform

@yangkb09 yangkb09 Awaiting requested review from yangkb09 yangkb09 was automatically assigned from grafana/backend-platform

Assignees

@gamab gamab

Labels
add to changelog area/backend no-backport Skip backport of PR
Projects
None yet
Milestone
10.4.0
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@gamab @andresmgot @aangelisc