-
Notifications
You must be signed in to change notification settings - Fork 65
Add external Policy Decision Point for Authorization #1170
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
Conversation
|
We're aiming to try this out on our test beamline today 🤞 |
danielballan
left a comment
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.
How did your local test go?
@nmaytan took a look at this and have some quick comments.
| "read:data", | ||
| "write:data", | ||
| "read:metadata", | ||
| "write:metadata", |
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.
Is the intent here to make public nodes world-writable? Generally I would expect world-readable, but not world-writable.
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 will refactor it to make it more clear but the intent is to make public tag world readable
The root node currently is world readable and when a user comes with a tag that allows them to write to this public node can write to it
| ], | ||
| "public": [ | ||
| "read:data", | ||
| "write:data", |
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 tag configures what unauthenticated requests can do. Generally I would expect those would never be allowed to write (or create, register, delete).
| result: Union[List[str], bool] | ||
|
|
||
|
|
||
| class ExternalPolicyDecisionPoint(AccessPolicy): |
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.
@nmaytan and I wonder if the TagBasedAccessPolicy could be reused, using the tag_parser argument to inject OPA-specific integration (_get_external_decision). Most of the AccessPolicy interface will be the same across our local solution, OPA, OpenFGA, and others. The TagParser abstraction might be a more tightly-scoped way of injecting framework-specific integration.
Nate will aim to find the bandwidth to implement this suggestion as a PR into your PR. But I mention this now in case you have any immediate thoughts on 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.
It would be a good thing to use the tag_parser but the only thing that concerns me is There are lots of authZ that happens in Tiled ,example
"Cannot apply empty tag list to node: only Tiled admins can apply an empty tag list."
But when you look at the ExternalPolicy it does not checks and just delegates everything to AuthZ to decide ,which I think makes it cleaner separation of concerns
1f9b5c0 to
eab6e29
Compare
tiled/access_control/dls.py
Outdated
| "session/write_to_beamline_visit", | ||
| "session/user_sessions", | ||
| "tiled/scopes", |
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.
The user_sessions and tiled/scopes queries are added in this PR DiamondLightSource/authz#293 which is in review. Internally we have to answer exactly what the route to the queries are, so it may be worth removing this from the generic functionality PR into a later Diamond implementation PR?
Could also allow us to decide where non-NSLS facility specific code should live?
|
Plan:
|
|
Some merge conflicts were introduced when I merged #1244. Would you mind resolving these, @ZohebShaikh? Then we can merge/release. |
|
Test failure is flaky/unrelated. |
Checklist