search

tus / tusd

Reference server implementation in Go of tus: the open protocol for resumable file uploads
https://tus.github.io/tusd
MIT License
2.93k stars 465 forks source link

hooks: add pre-resume blocking hook #1074

Closed sberthier closed 4 months ago

sberthier commented 5 months ago

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 https://github.com/tus/tusd/issues/669#issuecomment-1051924886

Acconut commented 5 months ago

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 commented 5 months ago

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 commented 5 months ago

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 commented 5 months ago

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 commented 5 months ago

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 commented 4 months ago

Superseeded by https://github.com/tus/tusd/pull/1077.