Skip system notifications polling for users without notifications:read permission#25387
Merged
dennisoelkers merged 5 commits intomasterfrom Mar 25, 2026
Merged
Skip system notifications polling for users without notifications:read permission#25387dennisoelkers merged 5 commits intomasterfrom
dennisoelkers merged 5 commits intomasterfrom
Conversation
kmerz
changed the title
…d permission Wrap NotificationBadge in IfPermitted so users who cannot see notifications (e.g. app owner) no longer trigger the 3-second polling to /api/system/notifications. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Verify that NotificationBadge is only rendered for users with notifications:read permission and hidden for users without it. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Use complete notification object to satisfy TypeScript type check. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
kmerz
requested review from
dennisoelkers,
linuspahl and
maxiadlovskii
and removed request for
dennisoelkers and
linuspahl
dennisoelkers
requested changes
Mar 20, 2026
Member
There was a problem hiding this comment.
In the NotificationsResource, we filter all notifications with the permission notifications:read:<type>, meaning that we can assign permissions on a notification-type level. This PR would now disable all notifications for a user that has notifications:read:foo but not notifications:read.
Move permission check from IfPermitted wrapper into NotificationBadge component. The badge now checks the current user's permissions for any notification read access and disables the useNotifications query when none exist, avoiding unnecessary 3s polling for unpermissioned users while still showing the badge for users with type-specific permissions like notifications:read:deflector_migration. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
dennisoelkers
approved these changes
Mar 25, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
Move the notification permission check from an
<IfPermitted permissions="notifications:read">wrapper inNavigation.tsxinto theNotificationBadgecomponent itself. The badge now checks the current user's permissions for any notification read access (*,notifications:*, ornotifications:read:*) and passes anenabledflag touseNotifications(), which forwards it to React Query. When disabled, the query never executes — no polling, no network requests.Motivation and Context
The original
<IfPermitted permissions="notifications:read">wrapper required the blanketnotifications:readpermission. Users who only had type-specific permissions likenotifications:read:deflector_migrationwould never see the badge, even though the backend would return notifications for them.Simply removing the wrapper (so the badge always renders) fixes visibility but still polls
GET /api/system/notificationsevery 3 seconds for users with zero notification permissions — unnecessary network traffic and server load.This approach solves both problems: users with any notification read permission see the badge and get polling; users with none see nothing and generate no traffic.
How Has This Been Tested?
NotificationBadge.test.tsx: Added test verifying that a user without notification permissions causesuseNotificationsto be called with{ enabled: false }and the badge is not rendered. Existing tests continue to pass for users with permissions.Navigation.test.tsx: Updated tests to check badge visibility based on notification data instead of permissions (the permission gating now lives inNotificationBadge, notNavigation).All existing and new tests pass.
Fixes Graylog2/graylog-plugin-enterprise#10902
Types of changes
Checklist: