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

Alerting: Implement correct RBAC checks for creating new notification templates #83767

Merged
merged 2 commits into from
Mar 1, 2024

Conversation

gillesdemey
Copy link
Member

What is this feature?

Prior to this PR the button to add a new notification template was always shown regardless of wether it was supported for the current Alertmanager context or if the user has permissions to do so.

Changes in this PR bring the behaviour in line with the rest of the buttons; hiding it when not supported for the Alertmanager in context and disabling the action for users without sufficient permissions.

@gillesdemey gillesdemey added this to the 11.0.x milestone Mar 1, 2024
@gillesdemey gillesdemey requested a review from a team as a code owner March 1, 2024 14:02
@gillesdemey gillesdemey requested review from konrad147 and soniaAguilarPeiron and removed request for a team March 1, 2024 14:02
<LinkButton icon="plus" variant="primary" href="/alerting/notifications/templates/new">
Add notification template
</LinkButton>
{createTemplateSupported && (
Copy link
Contributor

Choose a reason for hiding this comment

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

No need to check createTemplateAllowed too?

Copy link
Member Author

Choose a reason for hiding this comment

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

The logic that we're applying to all of the buttons are:

  1. hide when not supported
  2. disable when user doesn't have sufficient permissions

Perhaps we should add some rationale to the button for why it's disabled in the future but I felt like hiding the actions when you don't have sufficient permissions might be confusing when the docs may reference these buttons / tabs / actions.

Copy link
Contributor

Choose a reason for hiding this comment

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

Ah, I didn't noticed the disabled prop!

Copy link
Contributor

@konrad147 konrad147 left a comment

Choose a reason for hiding this comment

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

LGTM!

@gillesdemey gillesdemey merged commit 582fd53 into main Mar 1, 2024
24 checks passed
@gillesdemey gillesdemey deleted the alerting/fix-create-notification-template-rbac branch March 1, 2024 17:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

3 participants