RBAC: enforce permissions in frontend using user roles #986

VikaCep · 2024-11-08T20:25:03Z

This PR integrates the newly defined roles from #982 into the app. The roles are used to restrict access to different parts of the app where applicable, providing more granular control over permissions and user access.

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

Reader

Screen.Recording.2024-11-13.at.17.55.42.mov

Editor

Screen.Recording.2024-11-13.at.18.07.33.mov

Admin

Screen.Recording.2024-11-13.at.18.10.14.mov

Mixed: Viewer + can write checks

Screen.Recording.2024-11-13.at.18.11.37.mov

Mixed: Viewer + can write probes

https://github.com/user-attachments/assets/65d268b9-62c8-4a7f-adb5-a7f5381f7c27

w1kman · 2024-11-14T19:08:13Z

Note: need to supply Grafana with an enterprise license to review this PR. :)

w1kman

The config page, probably dont need to be visible for non-writers?

Without a basic role (`orgRole), it seems I cannot do much, even if I have SM checks:reader and probe:reader/writer
The only thing that changes are the nav items.

When logged in as admin, you can clearly see that the app is already initialized

VikaCep · 2024-11-19T14:38:56Z

Without a basic role (`orgRole), it seems I cannot do much, even if I have SM checks:reader and probe:reader/writer The only thing that changes are the nav items.

When logged in as admin, you can clearly see that the app is already initialized.

The reason this happens is because if no basic roles are set (at least Viewer), you'd need to add the general datasources:reader permission to the user in order to use the plugin. If Viewer is set, this is not needed.

I tried to include this permission by default in the custom permissions we define (similar to how I added the plugin:access permission here) to avoid requiring users to manually configure it. However, it doesn't seem to be possible. I suspect the permission is too broad to be auto-granted in the plugin's definition. I wonder if @d0ugal has encountered a similar issue when implementing this in k6.

For reference, this is the error I get when trying to grant datasources:read permission.

grafana-synthetic-monitoring-app | logger=plugins.roles.registration t=2024-11-19T14:32:51.931169754Z level=warn msg="Declare plugin roles failed." pluginId=grafana-synthetic-monitoring-app error="expected action 'datasources:read' to be prefixed with any of '[plugins.app:access grafana-synthetic-monitoring-app: grafana-synthetic-monitoring-app.]'"

In any case, I think the simplest solution is to recommend to always set the Viewer role to get basic permissions, and customize from there on.

VikaCep · 2024-11-21T00:32:49Z

Without a basic role (`orgRole), it seems I cannot do much, even if I have SM checks:reader and probe:reader/writer The only thing that changes are the nav items.

I've discussed this issue with @d0ugal, as it seems to be a common scenario. If a user is assigned a custom role but lacks the necessary fixed roles required for an action, the custom role alone will not be sufficient. In these cases, it’s important to highlight the missing roles in the UI so that users understand what’s happening and can request the required roles.
As an example, this PR shows how it's handled in k6, by clarifying what are the missing permissions when an action is not allowed.

In Synthetic Monitoring, if a user is not assigned the global datasources: read role then the app won't display any data. This role is a required global permission that must be assigned to users in order for them to use the app (unless the Viewer role is assigned). I’ve updated the message (as shown in the screenshot) to clarify this so that it's not confused with the "app not initialized" state.

Note no datasource:read permission assigned:

Note the new error message indicating missing permissions:

When the user is assigned the datasources:read permission, then the app can be used, even if no org role is assigned along the custom ones:

Note datasources:read is assigned:

Note the app loads correctly:

w1kman

Love the fix for a missing base role 🥳
There are some nits that you may or may not wanna change.

src/components/ConfigActions.tsx

src/components/ProgrammaticManagement.tsx

src/hooks/useUserPermissions.ts

src/types.ts

ckbedwell

I've done some testing but need to go through in detail to double-check. Wanted to get the initial review done with some feedback -- looks ace, found a few small things to take a look at.

src/hooks/useUserPermissions.ts

src/components/ProbeCard/ProbeCard.tsx

src/test/utils.ts

src/components/ProbeEditor/ProbeEditor.tsx

ckbedwell · 2024-11-22T14:48:38Z

src/page/AlertingPage.tsx

@@ -39,7 +39,7 @@ const Alerting = () => {
          Learn more about alerting for Synthetic Monitoring.
        </a>
      </p>
-      {canRead ? <AlertingPageContent /> : <InsufficientPermissions />}
+      {canReadAlerts ? <AlertingPageContent /> : <InsufficientPermissions />}


I saw the insufficient permissions when I just had the basic 'Viewer' role (I didn't look into why). Should we provide more detail what permissions are missing as there are two places you would need to update to have the right permissions?

Hmm I'm confused as I'm not sure how you got to that screen.

Users with only the Viewer role assigned, should see this:

assigned roles:

Users without the Viewer role, but with the Alerts reader role, see the following:

assigned roles:

The difference lies in that Viewer assigns fixed:alerting:reader by default and without it it needs to be manually added.

- instead, we should query permissions from getUserPermissions

ckbedwell · 2024-11-27T10:51:04Z

src/hooks/useAlertPermissions.ts

+  const canEditAlertInDs = useDSPermission(`metrics`, `alert.instances.external:write`);
+
+  return {
+    canReadAlerts,


I removed Query permissions for Viewer in my metrics datasource and I get faced with an infinitely loading spinner. This should be a simple case of checking if there is a value being returned for useMetricsDS() and adding it to the return for each of the canReadAlerts / canWriteAlerts / canDeleteAlerts.

Nice catch. I've added that check and changed the message to indicate the missing permissions:

* 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

* feat: define new roles * feat: add role requirements for routes and pages * fix: add plugin access permission for all roles * fix: configure general read/write permissions for ds plugin.json config * fix: remove redundant granted roles * fix: improve names and remove token writer access for editors * fix: rename edit permission * fix: rename tokens to access-tokens * fix: restrict config page to writers * fix: change required permissions to register a ds * fix: change access-token create for write to match convention * fix: add missing threshold: delete permission in Threshold Writer role * fix: change required permissions to see SM home page * RBAC: enforce permissions in frontend using user roles (#986) * 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 * fix: lint * fix: add generic UnauthorizedPage and enforce permissions on home and view pages * fix: display missing write alerts permission * fix: lint * fix: consolidate enable/disable plugin actions into a single write one * fix: lint * fix: prevent displaying missing write message to readers * fix: addressing review comments * chore: remove ConfigActions as not user after rebase with main * fix: add message when missing access token write permission * fix: remove Access Tokens Reader role * fix: remove access-tokens: read and delete actions - There's no use for them right now * fix: list missing permissions to initialize plugin * fix: change Unauthorized page layout - Remove card to get rid of hover effect - Only dispay the relevant message * fix: restrict terraform access * fix: lint * chore: add rbac feature flag * fix: update test after rebase with main * fix: change fallback role required to generate access tokens - When the RBAC FF is off, canWriteTokens requires a min role of Editor to respect current behavior

VikaCep self-assigned this Nov 8, 2024

VikaCep force-pushed the rbac-roles branch from f4fe1c0 to eb905b0 Compare November 12, 2024 19:48

VikaCep force-pushed the rbac-permissions-front branch 3 times, most recently from f167e0b to 75b7eb3 Compare November 13, 2024 20:46

VikaCep marked this pull request as ready for review November 13, 2024 21:21

VikaCep requested a review from a team as a code owner November 13, 2024 21:21

w1kman reviewed Nov 14, 2024

View reviewed changes

VikaCep mentioned this pull request Nov 19, 2024

Feat: Add RBAC to Synthetic Monitoring #982

Merged

VikaCep force-pushed the rbac-permissions-front branch from 75b7eb3 to e9c3969 Compare November 19, 2024 19:24

VikaCep requested a review from w1kman November 22, 2024 14:09

w1kman requested changes Nov 22, 2024

View reviewed changes

ckbedwell requested changes Nov 22, 2024

View reviewed changes

VikaCep force-pushed the rbac-permissions-front branch from 588556f to d11f6d4 Compare November 26, 2024 20:32

VikaCep added 13 commits November 26, 2024 18:38

feat: create useUserPermissions hook

46f1029

feat: define PluginPermissions type

9dfa373

feat: enforce Check permissions using RBAC

6209cfb

feat: enforce Probe permissions usign RBAC

e3914fc

feat: enforce Alert permissions using RBAC

e221f1f

fix: lint

b0ca71c

feat: enforce Config permissions using RBAC

05e90e6

feat: apply new permissions to plugin installation

093b378

fix: remove console.log

f0867bf

fix: fallback to basic user roles contemplating roles hierarchy

e44903e

fix: change PluginPermission to use write instead of edit

a8b6bf6

fix: add tests

4f75006

fix: update types for access-tokens permissions

2d9c690

VikaCep added 8 commits November 26, 2024 18:47

fix: lint

2b6e96b

fix: tests

7b0683a

fix: show missing permissions alert

a3e9839

fix: adjust types to match plugin definitions

76541af

fix: refactor getUserPermissions function

c3e9a38

fix: change plugin permissions to use template literal types

ddc0ba7

fix: uppercase RBAC in function names

9a34072

fix: updates after rebasing with main

55981c9

VikaCep force-pushed the rbac-roles branch from 1ec89df to 6b81251 Compare November 26, 2024 22:00

VikaCep added 2 commits November 26, 2024 19:23

fix: adapt after rebasing with main

fd78792

fix: remove useCanWriteSM hook

96541a2

- instead, we should query permissions from getUserPermissions

VikaCep force-pushed the rbac-permissions-front branch from 64e4f79 to 96541a2 Compare November 26, 2024 22:27

VikaCep requested review from ckbedwell and w1kman November 26, 2024 22:28

fix: lint

3dd5a10

ckbedwell reviewed Nov 27, 2024

View reviewed changes

fix: check for metrics ds query access in order to display alerts

08f9a6d

VikaCep requested a review from ckbedwell November 27, 2024 21:36

VikaCep merged commit 61036cc into rbac-roles Nov 28, 2024
3 checks passed

VikaCep deleted the rbac-permissions-front branch November 28, 2024 14:28

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

RBAC: enforce permissions in frontend using user roles #986

RBAC: enforce permissions in frontend using user roles #986

VikaCep commented Nov 8, 2024 •

edited

Loading

w1kman commented Nov 14, 2024

w1kman left a comment •

edited

Loading

VikaCep commented Nov 19, 2024 •

edited

Loading

VikaCep commented Nov 21, 2024

w1kman left a comment

ckbedwell left a comment

ckbedwell Nov 22, 2024

VikaCep Nov 26, 2024 •

edited

Loading

ckbedwell Nov 27, 2024

VikaCep Nov 27, 2024

RBAC: enforce permissions in frontend using user roles #986

RBAC: enforce permissions in frontend using user roles #986

Conversation

VikaCep commented Nov 8, 2024 • edited Loading

w1kman commented Nov 14, 2024

w1kman left a comment • edited Loading

Choose a reason for hiding this comment

VikaCep commented Nov 19, 2024 • edited Loading

VikaCep commented Nov 21, 2024

w1kman left a comment

Choose a reason for hiding this comment

ckbedwell left a comment

Choose a reason for hiding this comment

ckbedwell Nov 22, 2024

Choose a reason for hiding this comment

VikaCep Nov 26, 2024 • edited Loading

Choose a reason for hiding this comment

ckbedwell Nov 27, 2024

Choose a reason for hiding this comment

VikaCep Nov 27, 2024

Choose a reason for hiding this comment

VikaCep commented Nov 8, 2024 •

edited

Loading

w1kman left a comment •

edited

Loading

VikaCep commented Nov 19, 2024 •

edited

Loading

VikaCep Nov 26, 2024 •

edited

Loading