hooks: add pre-access blocking hook #1077
Conversation
sberthier
force-pushed
the
feature/pre-access-hook
branch
from
bad9e4e
to
2467d64
Compare
pkg/handler/hooks.go
Outdated
|
|
||
| // All files info that will be access by http request | ||
| // Use an array because of Upload-Concat that may target several files | ||
| Files []FileInfo |
There was a problem hiding this comment.
Can we call this Uploads, similar to the Upload field in HookEvent itself?
pkg/handler/hooks.go
Outdated
| Files []FileInfo | ||
| } | ||
|
|
||
| func newHookEvent(c *httpContext, info *FileInfo, accessInfo *AccessInfo) HookEvent { |
There was a problem hiding this comment.
I would prefer to split the access-related logic into a new function
func newHookEvent(c *httpContext, mode AccessMode, uploads []FileInfo) HookEvent {
event := newHookEvent(c, nil)
event.Access = {
Mode: mode,
Uploads: uploads,
}
return event
}while keeping access out of newHookEvent:
func newHookEvent(c *httpContext, info *FileInfo) HookEvent {
Then we should also be able to remove newAccessInfo.
What do you think?
pkg/handler/unrouted_handler.go
Outdated
| @@ -338,7 +340,8 @@ func (handler *UnroutedHandler) PostFile(w http.ResponseWriter, r *http.Request) | |||
| } | |||
|
|
|||
| if handler.config.PreUploadCreateCallback != nil { | |||
| resp2, changes, err := handler.config.PreUploadCreateCallback(newHookEvent(c, info)) | |||
| access := newAccessInfo(AccessModeRead, partialFileInfos) | |||
| resp2, changes, err := handler.config.PreUploadCreateCallback(newHookEvent(c, &info, &access)) | |||
There was a problem hiding this comment.
For concatenation, I think we should first emit a pre-access hook to check the permissions for read access on the partial uploads. If that succeeds we can continue with the upload creation and the corresponding pre-create hook. Then users don't have to duplicate access-related logic in pre-access and pre-create.
A good location for that hook is likely a few lines above after the call to handler.sizeOfUploads.
| return HookEvent{ | ||
| Context: c, | ||
| Upload: info, | ||
| Upload: upload, |
There was a problem hiding this comment.
In hindsight, it would be great if we could set Upload: nil, but that would a require a change to the type, which would be breaking.
to prepare pre-access hook
- on Head/Get/Patch/Delete request - use new AccessInfo in HookEvent with AccessMode and FileInfo list - use RejectAccess in response to return 403 by default if rejected
when Upload-Concat is used, to check authorization for instance
- add some README.md to guide usage - fix grpc example Makefile proto path
sberthier
force-pushed
the
feature/pre-access-hook
branch
from
2467d64
to
9433698
Compare
|
I made the changes you suggested, except the last one to set |
|
Thank you for the updates, I will look into this in two weeks and then we should be able to get this merged. |
pre-resumePR (hooks: add pre-resume blocking hook #1074)Access.ModeandAccess.FilestoHookEventforpre-accessandpre-create(when Upload-Concat) hook.Filesis an array because of Concatenation extension that access many files to make a new oneModeis read (Get/Head) or write (Patch/Delete)pre-accessbefore each http request Get/Head/Patch/Delete.RejectAccessin HookResponse. Http response status code will be 403, if not override in HttpResponse.close #1074