-
Notifications
You must be signed in to change notification settings - Fork 188
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
Rework auth to include scopes as well as groups #2092
Conversation
@pschoenfelder mind taking a look on this one? Biggest open question I have is if we should keep the defaultReadOnlyGroups config around or not. Need to check if our setup is creating requests with top level groups as the default read only, or if Singularity is adding that right now. Not sure if we want that default behavior to be that permissive (though it is current I guess) |
} | ||
|
||
@Override | ||
public void checkAdminAuthorization(SingularityUser user) { |
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.
Looks like this and several other methods below follow the same patten of checkGrantedByScopes -> check groups authorizer -> maybe warn. Is there a way to factor the pattern out?
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.
yeah, I can take another pass at slimming it down after I do a few initial tests in staging. Did one pass already to factor some things out, but I think you're right that I can DRY it up a bit more
Looks good to me. Nice unit tests 👍 Always happy to see that. As far as removing defaultReadOnlyGroups goes, I would lean towards keeping it, at least for now while these changes get rolled out. |
Bug in here somewhere still, dual authorizer returned all 401s on staging |
🚢 |
The previous setup was originally meant for a very simple auth scheme. We now need a bit more granularity, since a set of groups only allows us to do things at a mostly global level for permissioning.
This PR initially refactors the authorization helper to an interface with a groups as well as a groups+scopes implementation. Still TODO:
cc @pschoenfelder