Skip to content
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

hooks: add pre-access blocking hook #1077

Open
wants to merge 6 commits into
base: main
Choose a base branch
from

Conversation

sberthier
Copy link

  • To replace pre-resume PR (hooks: add pre-resume blocking hook #1074)
  • Add Access.Mode and Access.Files to HookEvent for pre-access and pre-create (when Upload-Concat) hook.
    • Files is an array because of Concatenation extension that access many files to make a new one
    • Mode is read (Get/Head) or write (Patch/Delete)
  • Add pre-access before each http request Get/Head/Patch/Delete.
    • This new hook is disabled by default
    • Access can be rejected by setting RejectAccess in HookResponse. Http response status code will be 403, if not override in HttpResponse.
    • Useful for authentication/authorization of upload access, based on http request headers for instance

close #1074

Copy link
Member

@Acconut Acconut left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Wow, thank you very much for this huge PR! Congratulations on adding a new hook on your own. I can a look through it focusing on the implementation (and less on the tests and documentation) for now, and left a few comments. But overall this is looking great!


// All files info that will be access by http request
// Use an array because of Upload-Concat that may target several files
Files []FileInfo
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we call this Uploads, similar to the Upload field in HookEvent itself?

Files []FileInfo
}

func newHookEvent(c *httpContext, info *FileInfo, accessInfo *AccessInfo) HookEvent {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

@@ -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))
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@Acconut Acconut linked an issue Feb 8, 2024 that may be closed by this pull request
- 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
Copy link
Author

sberthier commented Feb 16, 2024

I made the changes you suggested, except the last one to set Hook.Upload to nil !

@Acconut
Copy link
Member

Acconut commented Mar 25, 2024

Thank you for the updates, I will look into this in two weeks and then we should be able to get this merged.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Authentication
2 participants