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

Change mapping of permission NOTIFICATIONS_READ to legacy role #5358

Open
wants to merge 1 commit into
base: dev
Choose a base branch
from

Conversation

brat002
Copy link

@brat002 brat002 commented Dec 12, 2024

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

  • Unit, integration, and e2e (if applicable) tests updated
  • Documentation added (or pr:no public docs PR label added if not required)
  • Added the relevant release notes label (see labels prefixed w/ release:). These labels dictate how your PR will
    show up in the autogenerated release notes.

@CLAassistant
Copy link

CLAassistant commented Dec 12, 2024

CLA assistant check
Thank you for your submission! We really appreciate it. Like many open source projects, we ask that you sign our Contributor License Agreement before we can accept your contribution.
You have signed the CLA already but the status is still pending? Let us recheck it.

@brat002 brat002 marked this pull request as ready for review December 12, 2024 12:26
@brat002 brat002 requested a review from a team as a code owner December 12, 2024 12:26
@brat002 brat002 changed the title Change mapping of permission NORIFICATION_READ to legacy role Change mapping of permission NOTIFICATION_READ to legacy role Dec 12, 2024
@brat002 brat002 changed the title Change mapping of permission NOTIFICATION_READ to legacy role Change mapping of permission NOTIFICATIONS_READ to legacy role Dec 12, 2024
@alexandr-ku-MA
Copy link

Guys, could you take a look?
@matiasb ?

@matiasb
Copy link
Contributor

matiasb commented Dec 17, 2024

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
Copy link
Author

brat002 commented Dec 17, 2024

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
Copy link
Author

brat002 commented Dec 19, 2024

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

@xemekk
Copy link

xemekk commented Dec 23, 2024

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

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.

5 participants