Add external Policy Decision Point for Authorization#1170
Conversation
|
We're aiming to try this out on our test beamline today 🤞 |
danielballan
left a comment
There was a problem hiding this comment.
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.
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.
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.
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.
@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.
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.
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. |
* add external policy decision point * update access_policy test * Remove dls specific policy * Add docs for policy
Checklist