-
Notifications
You must be signed in to change notification settings - Fork 41
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
session and authn interfaces should do authz checks #912
Merged
Merged
Changes from 8 commits
Commits
Show all changes
9 commits
Select commit
Hold shift + click to select a range
7afaff1
first cut
davepacheco a51651a
partway through a more significant rework
davepacheco 9f16671
compiles (no tests), but want to change Nexus-level interface
davepacheco ac64b8d
compiles (no tests)
davepacheco e544cc9
fix style
davepacheco 9be6f2a
XXX for myself
davepacheco de3be72
add Oso init logging; fix invalid role name; fix a bunch of tests
davepacheco 7a8402e
fix style
davepacheco 61d37e3
fix typo
davepacheco File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains 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
This file contains 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
This file contains 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
This file contains 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
This file contains 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
This file contains 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
This file contains 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
Oops, something went wrong.
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.
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.
Why don't we need something like this to represent the ability to create the various other kinds of
FleetChild
?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.
Good question. There's one set of permissions and roles on Fleet, and it's basically written to be pretty locked-down. You have to have Fleet "collaborator" to have the Fleet
create_child
permission. But we want the "external-authenticator" to be able to create sessions without them having full write access to the Fleet.A similar problem comes up with global images (in another PR).
Another approach would be to create more different actions on Fleet and have more roles that grant those permissions to do those actions. The upside is we wouldn't need this synthetic resource. I'm not sure it's clearer to do that, though. It's a little funny to have these synthetic resources, but I think they neatly describe what's really going on (which is there's a collection -- albeit one not represented in the API -- that "internal-authenticator" has permission to create children in). There's a more practical downside to creating a bunch more actions: right now, there's only one
authz::Action
enum with the union of all actions on all resources, so it would wind up growing a bunch of variants that aren't applicable for most resources. I've been wondering if we could split this up. The challenge is it's an argument toOpContext::authorize()
. Maybe it could be an associated type of theAuthorizedResource
that you're doing a check on. I haven't dug into this.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.
Ok good, that's all helpful and matches the picture in my head to the extent I had one — that the synthetic resource is an alternative to adding a very narrow action to the enum for this purpose. I imagine @plotnick has an opinion about this.