-
Notifications
You must be signed in to change notification settings - Fork 11.7k
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: separate out silence auth service preconditions checks #87998
Conversation
Will be useful for subsequent PR that adds metadata to silence response
1e93a64
to
ed8d0ff
Compare
user: newUser(), | ||
expected: []*models.Silence{}, | ||
expectedErr: ErrAuthorizationBase, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is not actually a change, previously it was caught by middleware.
hasAccess, err = s.HasAccess(ctx, user, readRuleSilenceEvaluator(ns)) | ||
if err != nil { | ||
return nil, err | ||
hasAccess = s.authorizeReadSilence(ctx, user, silWithFolder) == nil |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
creation of the authz error is a bit more costly than regular error, I wonder if we can have just bool here. This is not a blocker, though.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
My comments are non-blocking and can be implemented in a follow-up PR that already there.
Feel free to merge it as is. LGTM
Will be useful for subsequent PR that adds metadata to silence response
What is this feature?
Separates out precondition checks from silence auth service
AuthorizeXSilence
methods.Why do we need this feature?
This is so precondition checks can be used separately in a subsequent PR that will add permission metadata information to the GET silence responses.
Special notes for your reviewer:
This is mostly a no-op refactor but introduces a new short circuit for silence reads when a user has wildcard folder permissions.