grafana / oncall

Developer-friendly incident response with brilliant Slack integration
GNU Affero General Public License v3.0
3.63k stars 300 forks source link

Change mapping of permission NOTIFICATIONS_READ to legacy role #5358

Open brat002 opened 2 months ago

brat002 commented 2 months ago

The problem the PR solves is a user must have EDITOR role in Grafana to be included into a Schedule.

If we compare other _READ level privileges with their legacy roles, they are mapped to VIEWER role (except API keys read).

In short, the main idea of this change is to avoid granting a user EDITOR role in a whole organization only because a user should be a part of some Schedule.

What this PR does

This PR changes a mapping of NOTIFICATIONS_READ permission from EDITOR to VIEWER role.

Which issue(s) this PR closes

Checklist

CLAassistant commented 2 months ago

CLA assistant check
All committers have signed the CLA.

alexandr-ku-MA commented 2 months ago

Guys, could you take a look? @matiasb ?

matiasb commented 2 months ago

Hey,

I see your point. But if you allow Viewers to receive notifications, I guess you will also need them to act on alert groups (requiring alert groups write perm), and/or allow them to setup their notification policies (user settings write), right? So the main decision on our side would be if we should allow Viewers to be on-call, that AFAICT was discarded at some point (but it may make sense to have a global project setting enabling this? eg. ALLOW_VIEWERS_ON_CALL).

brat002 commented 2 months ago

I see your point. But if you allow Viewers to receive notifications, I guess you will also need them to act on alert groups (requiring alert groups write perm), and/or allow them to setup their notification policies (user settings write), right? So the main decision on our side would be if we should allow Viewers to be on-call, that AFAICT was discarded at some point (but it may make sense to have a global project setting enabling this? eg. ALLOW_VIEWERS_ON_CALL).

The reasons make sense, although we use Grafana oncall mostly as an engine for Schedules. None of actual actions are expected from users.

I think adding ALLOW_VIEWERS_ON_CALL is a good tradeoff. If so, I'll add the changes.

brat002 commented 1 month ago

Added. I've found a block of settings with FEATURE_ prefix. I suppose the setting also may be considered as a FEATURE.

xemekk commented 1 month ago

We've also encountered a problem here and moving users to editors only to let them be part of schedule isn't an option. Would love to have this merged

matiasb commented 1 month ago

The reasons make sense, although we use Grafana oncall mostly as an engine for Schedules. None of actual actions are expected from users.

Ok, but I think if we have this toggle to enable Viewers to be on-call, we should allow them to perform all on-call related actions (so I would expect them to have the same perms as the OnCaller role we have defined), to have a more reusable/generally useful approach, makes sense?

brat002 commented 1 month ago

I've added dynamic definition of Permission class based on feature toggle. I think this looks better then 5 If/else. @matiasb wdyt?

brat002 commented 1 month ago

So, any suggestions, ideas, complaints? Can we merge it?