refactoring object-operation endpoints to use permission descriptors #9416
Conversation
There was a problem hiding this comment.
This looks like a great strategy, thanks!
I think many edits of authorize() calls are missing here. This is great if we're doing a trial run for this implementation strategy, but of course blocks actually pulling this PR. So obviously blocking on that - but that does not itself signal any issues!
pkg/api/controller.go
Outdated
| @@ -111,6 +111,10 @@ type Controller struct { | |||
| licenseManager license.Manager | |||
| } | |||
|
|
|||
| type AuthOpts struct { | |||
| callback func(w http.ResponseWriter, r *http.Request, code int, v interface{}) | |||
There was a problem hiding this comment.
Please add at least one, and possibly all, of these:
- a type for v
- documentation for the fields code and v
- documentation of callback: when is it called? what does it do?
There was a problem hiding this comment.
Currently all endpoints except 2 call authorize() which calls authorizeCallback() with writeError as callback. There are 2 endpoints which call authorizeCallback directly with a different callback.
So when a function is supplied here it is used as callback instead of writeError.
I used the same signature with the same parameter names as in authorizeCallback, because I think it's clearer if they match. after your comment I changed the variable names in both places to be more informative, tell me what you think.
pkg/api/controller.go
Outdated
| }) { | ||
| if !c.authorizeReq(w, r, "HeadObject", permissions.PermissionParams{Repository: &repository, Path: ¶ms.Path}, | ||
| &AuthOpts{callback: func(w http.ResponseWriter, r *http.Request, code int, v interface{}) { | ||
| writeResponse(w, r, code, nil) |
There was a problem hiding this comment.
Why the different callback here?
There was a problem hiding this comment.
to be clear, I didn't change the callback that is used here. but I believe the reason is that authorize() uses writeError as callback, which adds an error message to the response body when authorizeCallback fails. HeadObject is a Head endpoint, so it shouldn't have a body in the response.
I can add this as a comment.
|
@arielshaqed As I mentioned in the description, this PR only handles the endpoints that use a single-node permission with ObjectArn. Alternatively, I can do each resource type in a different commit, which can also make it easier to review. Let me know what you prefer, or if you have other suggestions. |
There was a problem hiding this comment.
Thanks for the clear explanations!
I agree with everything you wrote. However I also do not want to overload all our developers with having to understand two models of authorization at the same time.
What I would like to do: let's keep this PR open. And do the rest (or the majority) of API calls as additional PRs, into this branch. At the end, I will merge this PR into trunk and we will have a single change. I will commit to adding any auth changes that happened in the meantime.
WDYT?
|
That makes sense, seems like a good solution. So I'll make my next PR branch off this branch. |
|
Hi, Could you create a branch here so I can open PRs against it? |
|
[...]
Did it! Note that I added a "feature/" prefix to the branch, it is now |
There was a problem hiding this comment.
I created the branch. I believe I am not blocking in any way, so submitting a comment to make it "not my turn" on reviews
Related to #8201
Following the discussion here
This pr introduces permission objects to be used both in the auth flow and later in doc generation flow.
This pr deals with endpoints that use ObjectArn resources (with no AND nodes).
I tested manually committing new objects to branch, which means UploadObject and StageObject work.
@arielshaqed
Assuming this direction seems good, do you have a suggestion how to make this more reviewable \ testable?
I guess I shouldn't create a PR for each resource type, but having everything in one pr seems hard to manage.
I also wonder if there is a way to avoid trying every single endpoint manually.