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

Feat: Add RBAC to Synthetic Monitoring #982

Merged
merged 33 commits into from
Dec 18, 2024
Merged

Feat: Add RBAC to Synthetic Monitoring #982

merged 33 commits into from
Dec 18, 2024

Conversation

VikaCep
Copy link
Contributor

@VikaCep VikaCep commented Nov 4, 2024

Closes https://github.com/grafana/synthetic-monitoring/issues/166
Closes https://github.com/grafana/synthetic-monitoring/issues/167

This PR adds the configuration for the new roles associated with the different actions that can be performed within the plugin by following the RBAC in app plugins approach, and enforces this permissions in the frontend.

There's this document where different scenarios are combined to show how the app behaves in those cases.

I've taken the list from 166 and created the following categories so that users and teams can be assigned one or several of the following:

Admin: Provides permissions to manage all aspects of the app, including enabling/disabling the plugin and managing checks, probes, alerts, thresholds, and access tokens.

Editor: Similar to Admin access but does not include enabling/disabling the plugin or creating/deleting tokens.

Reader: Grants read access to all components (checks, probes, alerts, thresholds, and access tokens) in the app.

Checks Reader: Allows users to read checks within the app.
Checks Writer: Enables users to create, edit, and delete checks.

Probes Reader: Provides read access to probes.
Probes Writer: Allows users to create, edit, and delete probes.

Alerts Reader: Permits users to read alerts.
Alerts Writer: Enables the creation, editing, and deletion of alerts.

Thresholds Reader: Grants read access to thresholds.
Thresholds Writer: Allows users to read and edit thresholds,.

Access Tokens Writer: Permits the creation and deletion of access tokens.

image
image

src/plugin.json Outdated Show resolved Hide resolved
src/plugin.json Outdated Show resolved Hide resolved
src/plugin.json Outdated Show resolved Hide resolved
src/plugin.json Outdated Show resolved Hide resolved
@VikaCep VikaCep requested a review from d0ugal November 13, 2024 18:01
Copy link
Contributor

@w1kman w1kman left a comment

Choose a reason for hiding this comment

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

Is this prep work or is it possible to test the roles?

Copy link
Contributor

Choose a reason for hiding this comment

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

Perhaps I'm not understanding this correctly, but it seems a little loose to allow someone that can write, to modify the data source. Seems like that should have its own role and only be assigned to admins.

Copy link
Contributor

Choose a reason for hiding this comment

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

I guess this has nothing to do with adding/removing the data source 😅

Copy link
Contributor Author

Choose a reason for hiding this comment

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

In order to write checks or probes, users need to be able to write the SM datasource. For POST and DELETE operations to "{{.JsonData.apiHost}}/api/v1/" I've set a reqAction of grafana-synthetic-monitoring-app:write which before, it was Editor. This custom writer role is included in roles that perform write operations (such as Checks Writer, Probes Writer, etc).

Now, for the {{.JsonData.apiHost}}/api/v1/register/save url, I changed the required permission to be grafana-synthetic-monitoring-app.plugin:enable which is only assigned to Admins.

As an example, this is what happens if I try to edit a check and I don't have the permissions defined in the POST {{.JsonData.apiHost}}/api/v1/" section of the datasources/plugin.json file:

Screen.Recording.2024-11-19.at.16.31.50.mov

src/plugin.json Outdated Show resolved Hide resolved
@VikaCep
Copy link
Contributor Author

VikaCep commented Nov 19, 2024

Is this prep work or is it possible to test the roles?

This is the roles definitions only. In this other PR I consume the roles and apply the restrictions in the UI.

@VikaCep VikaCep requested review from w1kman and d0ugal November 19, 2024 19:36
src/plugin.json Outdated Show resolved Hide resolved
src/plugin.json Outdated Show resolved Hide resolved
d0ugal
d0ugal previously approved these changes Nov 22, 2024
src/plugin.json Show resolved Hide resolved
@d0ugal d0ugal dismissed their stale review November 22, 2024 08:53

didn't mean to add approval

@VikaCep VikaCep requested a review from d0ugal November 25, 2024 21:09
ckbedwell
ckbedwell previously approved these changes Nov 27, 2024
Copy link
Contributor

@ckbedwell ckbedwell left a comment

Choose a reason for hiding this comment

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

LGTM 🥇

I went on quite a journey when I started to deep dive in the other PR when testing the roles on an individual level. I am leaving my findings below as reference for anyone else who might get confused if they do this level of testing.

All screenshots show my admin user granting permissions on the left screen; my viewer user consuming said permissions.

I started by removing every role and just adding: Synthetic Monitoring: Access tokens reader


And I can't access SM or any other plugin for that matter. This is correct because at the moment we don't grant any routes to Access tokens readers. When they can view access tokens and have a route assigned this should go back to working.

So I added: Plugins:Application Plugins Access

And I can't access SM but I can access other plugins (poorly). This also seems correct because of the same reasoning above. I do wonder if we should show the homepage and have some level of explanation that they need a level of RBAC access?

So then I removed everything and added: Synthetic Monitoring:Reader

And this is what I expected as I hadn't granted SM DS datasource access.

So I added my user 'viewer' to the SM datasource and I can see a limited view of the SM app.

@ckbedwell ckbedwell dismissed their stale review November 27, 2024 10:57

Have some questions based on exploration in the other PR.

Copy link
Contributor

@ckbedwell ckbedwell left a comment

Choose a reason for hiding this comment

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

image

So based on the comment above. I think we need to support some sort of parred back homepage for users with limited access. Here I have the Synthetic Monitoring: Alerts reader role and if I go back to the homepage I get given an Access denied alert.

There is a wider conversation towards having an 'adaptable' homepage dependent on what your role and permissions are but that would be a lot of work and beyond the scope of the ticket.

@VikaCep
Copy link
Contributor Author

VikaCep commented Nov 27, 2024

@ckbedwell I have changed the required permissions that are needed to read the SM home page, which previously were requiring Check Reader access. Please let me know if this addresses your questions.

Here I have the Synthetic Monitoring: Alerts reader role and if I go back to the homepage I get given an Access denied alert.

After changing the mentioned permission, now in the scenario you described we get this view:

image

BTW, any custom SM role that you add (like Checks reader, Alerts reader, etc) will automatically include the { "action": "plugins.app:access", "scope": "plugins:id:grafana-synthetic-monitoring-app" }, permission so it's not necessary to add the Plugins:Application Plugins Access permission to the user manually.

@VikaCep VikaCep requested a review from ckbedwell November 27, 2024 21:36
@ckbedwell
Copy link
Contributor

After changing the mentioned permission, now in the scenario you described we get this view:

You only get this view if you don't have SMDS Query permissions. Once you add them you get a skeleton view of the Scenes homepage with the checks (which I shouldn't have because I don't have 'Checks reader' permissions).

A screenshot of Synthetic Monitoring homepage with the checks table with a single column displaying job names.

I wonder if we should add some sort of testing matrix to try and ensure this remains bug-free for the future or to document what the expected behaviour is?

@VikaCep
Copy link
Contributor Author

VikaCep commented Nov 28, 2024

I wonder if we should consider adding a testing matrix to help ensure this remains bug-free going forward, or perhaps document the expected behavior more clearly?

Yes, given the number of testing scenarios involved, I'll work on creating a document that outlines all possible cases and their expected behavior to help streamline the review process.

Additionally, some of the issues raised here should be addressed in this PR, as my focus here was primarily on the roles in the plugin.json file. My original plan was to submit two separate PRs—one for defining the new roles and permissions, and another for implementing the frontend enforcements—to first ensure consensus on the proposed roles. Now that we've mostly agreed on that, I think it makes sense to consolidate both PRs into one for easier testing. I'll proceed with merging PR #986 into this one.

* feat: create useUserPermissions hook

* feat: define PluginPermissions type

* feat: enforce Check permissions using RBAC

* feat: enforce Probe permissions usign RBAC

* feat: enforce Alert permissions using RBAC

* fix: lint

* feat: enforce Config permissions using RBAC

* feat: apply new permissions to plugin installation

* fix: remove console.log

* fix: fallback to basic user roles contemplating roles hierarchy

* fix: change PluginPermission to use write instead of edit

* fix: add tests

* fix: update types for access-tokens permissions

* fix: lint

* fix: tests

* fix: show missing permissions alert

* fix: adjust types to match plugin definitions

* fix: refactor getUserPermissions function

* fix: change plugin permissions to use template literal types

* fix: uppercase RBAC in function names

* fix: updates after rebasing with main

* fix: adapt after rebasing with main

* fix: remove useCanWriteSM hook

- instead, we should query permissions from getUserPermissions

* fix: lint

* fix: check for metrics ds query access in order to display alerts
- There's no use for them right now
- Remove card to get rid of hover effect
- Only dispay the relevant message
- When the RBAC FF is off, canWriteTokens requires a min role of Editor to respect current behavior
@VikaCep VikaCep requested review from ckbedwell and w1kman December 17, 2024 18:58
@VikaCep VikaCep merged commit a41e15f into main Dec 18, 2024
5 checks passed
@VikaCep VikaCep deleted the rbac-roles branch December 18, 2024 22:08
VikaCep added a commit that referenced this pull request Dec 19, 2024
VikaCep added a commit that referenced this pull request Dec 19, 2024
* Revert "refactor: move ChooseCheckGroup to page folder (#1021)"

This reverts commit cf67891.

* Revert "Feat: Add RBAC to Synthetic Monitoring (#982)"

This reverts commit a41e15f.

* Revert "chore(main): release 1.16.10 (#1015)"

This reverts commit 92350bd.

* Revert "Chore: update checks UI styling (#1020)"

This reverts commit 2b69b02.

* Revert "chore(deps): bump nanoid from 3.3.7 to 3.3.8 (#1018)"

This reverts commit 77032d0.

* Revert "Feat: prompt on unsaved unload (#1002)"

This reverts commit 9747b27.

* Revert "fix: add synthetic-monitoring-dev as owners of release files (#1010)"

This reverts commit d703e42.

* Revert "Chore/autumn clean (#1005)"

This reverts commit 279272d.

* fix: update package version

* fix: bring back changelog entries from 1.16.10

* fix: change release-please-manifest version to 1.16.10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants