-
Notifications
You must be signed in to change notification settings - Fork 1
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
auth/clientcredentials: add schema-based scope enforcement interceptor #61
Conversation
27b3fc2
to
9344924
Compare
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.
logger.Error("attempt to authenticate using SAMS token without required scope", | ||
log.Error(err)) | ||
// Return an opaque error | ||
return info, connect.NewError(connect.CodePermissionDenied, errors.New("insufficient scope")) |
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.
I think:
- Logging service-side errors here is non-actionable, after all this is client's problem to have insufficient scope.
- The error returned to the client does not need to be opaque (no security concerns), and being explicit greatly help the client debug scope issues.
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.
Removed the log, and made the error non-opaque 👍
Co-authored-by: Joe Chen <joe@sourcegraph.com>
def9a18
to
bcfd685
Compare
Adds an interceptor,
clientcredentials.Interceptor
, that enforces required SAMS scopes based on a proto schema extension. This will reduce boilerplate in RPC implementations and reduce the chance of mistakes/out-of-sync problems between a schema and the implementationUsage example:
Allows you to set required scopes as method options:
This generates
E_SamsRequiredScopes
that can be used to point to where we can extractsams_required_scopes
.Test plan
Unit tests