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-resume blocking hook #1074

Closed
wants to merge 1 commit into from

Conversation

sberthier
Copy link

pre-resume blocking hook, inspired by pre-create and pre-finish hooks. Disable by default.

Called on patch request to continue an existing upload. Upload can be rejected with RejectUpload and if so HTTPResponse is used in error http response. Otherwise response is ignored.

Useful for authentication/authorization of PATCH requests, as evoked in #669 (comment)

Useful for authentication/authorization of PATCH requests
@Acconut
Copy link
Member

Acconut commented Jan 29, 2024

Thank you for this PR and congratulations on implementing this on your own! I see the use case for such a hook, but a more general solution that is not exclusive to PATCH requests would be preferable as also mentioned in #669: An authentication hook that is executed before any request to tusd is processed would allow authenticating the users, testing their access rights to the corresponding upload, checking for user resource limits etc. I am not sure what your exact use case is but such an authentication hook would probably also serve your needs.

There has been no work started on this yet, but this PR could serve as a great template for adding the new authentication hook. Let me know what you think about this approach.

@sberthier
Copy link
Author

An global auth hook would server my needs too.

I see the interest of having that hook, however it will make call hook twice all most at the same time for some request, depending on hooks enabled (auth and pre-create for instance). Adding a pre-get hook would have allowed all routes to be protected just by a cost of an call each.

I could try implement that hook (named auth or pre-all ?), certainly in pkg/handler/unrouted_handler.go Middleware, should not be that different than this pre-resume.

@Acconut
Copy link
Member

Acconut commented Jan 29, 2024

It would be helpful if the authentication hook is invoked after the upload information has been fetched. Then this info can be passed to the authentication hook, which can use its upload ID and meta data to decide in combination with the client request to decide whether the access to upload is granted or not. In this case, the authentication hook can not be triggered in the middleware logic, but should be done inside HeadFile, PatchFile, etc.

When creating an upload, I don't think it's necessary to invoke the authentication hook since that task can be done by the pre-create hook. We can avoid the duplicate hook invocation there.

Does that make sense?

@sberthier
Copy link
Author

Yes it does.

Any preference for the hook name? Could be pre-access, invoked before any access done to the upload, Head, Get, Patch, Delete.

@Acconut
Copy link
Member

Acconut commented Jan 29, 2024

pre-access would be a good fit, yes. Thank you for working on this!

In addition to the upload information and request details, we should also include the requested access mode into the hook. For PATCH and DELETE requests this would be write and for HEAD and GET it would be read.

One thing I am not sure yet about is how to handle concatenation. When a client requests to concatenate upload A and B together, a pre-access hook should be fired with info about A and B to check whether the client has access to both. So the upload info field in the hook likely has to be an array, which would be different than the structure of other hooks, where we always only have one corresponding upload.

@Acconut
Copy link
Member

Acconut commented Feb 8, 2024

Superseeded by #1077.

@Acconut Acconut closed this Feb 8, 2024
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.

2 participants