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

RBAC: Default to plugins.app:access for plugin includes #90969

Merged
merged 3 commits into from
Jul 29, 2024

Conversation

gamab
Copy link
Contributor

@gamab gamab commented Jul 25, 2024

What is this feature?

This PR sets the permission required to access a navlink (=include) that was previously protected by role: Viewer (or
with no role/action specified) with action: plugins.app:access. This will allow users with the None role to view navlinks when they have the plugins.app:access permission.

Why do we need this feature?

Because some includes don't need any permission aside from plugins.app:access. This permission is the bare minimum you need to access a plugin. It's sort of the RBAC equivalent to reqRole = "Viewer" we had previously.

Special notes for your reviewer:

I assume I'll need to update the documentation :)

@gamab gamab self-assigned this Jul 25, 2024
@gamab gamab marked this pull request as ready for review July 25, 2024 16:17
@gamab gamab requested a review from a team as a code owner July 25, 2024 16:17
@gamab gamab requested review from wbrowne, andresmgot and oshirohugo and removed request for a team July 25, 2024 16:17
@github-actions github-actions bot added this to the 11.2.x milestone Jul 25, 2024
@gamab gamab added no-backport Skip backport of PR no-changelog Skip including change in changelog/release notes add to changelog and removed no-changelog Skip including change in changelog/release notes labels Jul 25, 2024
@gamab gamab changed the title RBAC: Default to app access for plugin includes RBAC: Default to plugins.app:access for plugin includes Jul 25, 2024
@@ -9,6 +9,8 @@ import (

const (
TypeDashboard = "dashboard"

ActionAppAccess = "plugins.app:access"
Copy link
Member

Choose a reason for hiding this comment

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

Apologies - way out of the loop on this but is the "app" in reference to the plugin type IE app plugins? I just see that technically for the CloudWatch datasource now for example, this will be returned as part of the API. I assume this is expected? I could be the only one lacking insight, but is this anything the plugin maintainers should be aware of to avoid any potential confusion?

Copy link
Contributor Author

@gamab gamab Jul 26, 2024

Choose a reason for hiding this comment

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

is the "app" in reference to the plugin type IE app plugins?

Yes it is.

I just see that technically for the CloudWatch datasource now for example, this will be returned as part of the API. I assume this is expected?

No it's not 😅

I thought Includes were restricted to app plugins 😮 But obviously now that I look at other tests, it seems that other kind of plugins can define their includes.

@gamab gamab requested a review from wbrowne July 26, 2024 08:02
@gamab gamab merged commit b982259 into main Jul 29, 2024
13 checks passed
@gamab gamab deleted the gamab/fix/default-plugin-access branch July 29, 2024 18:56
giorgioatmerqury pushed a commit to cybermerqury/grafana that referenced this pull request Aug 21, 2024
* Default to app access for includes

* Check plugin type
@aangelisc aangelisc modified the milestones: 11.2.x, 11.2.0 Aug 21, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants