-
Notifications
You must be signed in to change notification settings - Fork 103
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
Upload Tag extension #163
Upload Tag extension #163
Conversation
fix typo
…ther upload with the same ID is already ongoing.
- clarify format of the Client Reference is not limited to UUIDs - add additional error condition for when a Client Reference is not accepted by the Server - add additional headers that can be sent in response to the OPTIONS request to indicate the server's expectations and behaviour
- rename Tus-Client-Reference-Validity to Tus-Client-Reference-TTL
- addition of a length limitation for Upload-Tags - removal of Tus-Client-Reference-Foramt - removal of Tus-Client-Reference-Validity
…rement for authentication if these aren't used; minor formatting improvements.
- updates to status codes for various conditions - clarify that Upload-Tag-Secret should only be sent for POST requests and an error be returned if sent for other requests - rephrase Upload-Tag-Secret definition and add base64 encoding length information
…d rename the respective headers (remove the "Tag" part).
- add language requiring servers to reject unauthenticated requests including a Client-Tag - reorder headers in the examples for better readibility
I put together a POC/draft implementation of Client-Tag here: https://github.com/tusdotnet/tusdotnet/tree/POC/Client-Tag One can test it live at https://tustemp.azurewebsites.net/files using Postman or similar. Disclaimer: There might be bugs since it's a POC. Restrictions in place:
Here is a list of things I found doing this implementation and possible solutions (in no particular order):
into something like:
|
Thanks for moving this into a new PR, @felix-schwarz! |
@felix-schwarz Would it be OK if I add some commits to this PR to show what I mean with some of my comments? |
Wow, thanks for your feedback @Acconut & @smatsson! 😍 I'm now working through your feedback from top to bottom and will commit a revised version taking your feedback into account once done.
@Acconut Sure, that'd be fine! Preferably after I pushed my changes, though, to avoid version conflicts. |
Awesome, thanks a lot!
Agreed. I'm changing the name to Upload Tag.
As long as Upload Tags are tracked on a per-user basis, this shouldn't be a concern:
Good catch! I'll update that to:
Right now the proposal doesn't require the inclusion of |
I've just pushed all of my existing changes, so please go ahead. |
I think I was sloppy in my question here. For a HEAD request for e.g.
If a request for the latter is received without a |
I made a few changes to the PR:
Let me explain reasons for the last change: In its current form, the tus protocol does not require authentication at any point, which was a decision we made intentionally. There are applications were authentication is not needed and I would like tus server in these circumstances to also be compatible with the standard. For example: The demo server at https://master.tus.io does not require authentication because it is a public demo server. Any data uploaded there should be considered publicly available, so authorization is not needed. In fact, authorization would probably make it harder for users to try the demo server with various tus clients. I would consider this PR to be ready for merge in its current form, unless you have any feedback that should be incorporated. What do you think, @smatsson and @felix-schwarz? |
@Acconut Some minor things:
To me, this reads as there are other ways to continue the upload.
Would this imply being non-ascii and/or larger than 256 characters? Just want to double check. I think the security considerations part is spot on and would be easily expanded to work with the challenge extension. In the examples, should we remove Should we update the spec version? I know we are still discussing how to do this but this seems like a good opportunity if we want to increase the minor version? |
- Security Consideration: change "requirement" to "recommendation" to bring the statement in line with the rest of the segment
…eturned by HEAD requests with Client-Tag
Sorry it took me a while to get back to you! @Acconut Thanks for the feedback and making these changes. I agree with all of them. In the "Security Considerations" part, I changed "to fulfill this requirement" to "to fulfill this recommendation" to match the rest of the segment.
Good point. I changed that accordingly.
I think this would include any case where an
Even though authorization is only recommended, not required, I'd prefer to leave these in so that the examples show best practice. But removing them would also be ok with me. @Acconut From my side, the PR is ready to merge in its current form. |
+1 from me on the above. Ready to merge :) |
Do we wish to update the protocol version as per the discussion in #165 before merging this? |
Thank you for the update, @felix-schwarz! Looking good now! Only one thing came to my mind: We should restrict the Upload-Tag to printable ASCII characters to exclude control sequences such as the NUL character (see https://theasciicode.com.ar/ascii-control-characters/delete-ascii-code-127.html). I think these are codes 32 to 126. What do you think?
Not sure, I would like to add a few more things to 1.1 so I am not sure if it's better to bump the version in a separate PR. |
…rz/tus-resumable-upload-protocol into felix-schwarz-extension/client-tag
…to Wikipedia and applicable code range as a quick reference
@Acconut Sorry for taking so long to follow up! I think that clarification regarding printable ASCII codes is a good idea and have changed it accordingly - referencing Wikipedia (I couldn't find an RFC defining the "printable ASCII codes" term) and the 32-126 range as quick reference. |
…rz/tus-resumable-upload-protocol into felix-schwarz-extension/client-tag
Thank you very much for the final touches! I added the list of allowed ASCII characters, so it's easier to understand what we are talking about. Also, added an exclusion for spaces, to remove edge cases and potential failure. All in all, this can be merged in the 1.1 branch 🎉 |
@Acconut Thanks for the final touch improvements! Awesome to see it in the 1.1 branch! 🎉 |
This PR is a continuation of #157, with the following key differences:
The Upload Tag Extension to the tus protocol proposed here allows clients to resume an upload even if they did not receive the response to the
creation
orcreation-with-upload
request with which they initiated the upload.The mechanism for this is relatively simple:
creation
orcreation-with-upload
request.Mobile client apps benefit of this extension in various ways:
creation-with-upload
, complete uploads can be handed off to OS-managed background HTTP request queues (that continue to run when the apps themselves are already paused / suspended) for execution as soon as network connectivity is next available - without anxiety regarding the reliability of the connectioncreation-with-upload
request doesn't complete, the Upload Tag Extension can be used to retrieve all information necessary to resume the upload right from the point where it was interrupted - not wasting a single byte (of expensive cellular data) of bandwidth to retransmit data that has already been transmitted. The upload will finish eventually - even on a very unreliable connection. And on a reliable connection, there's the opportunity for the upload to finish with maximum efficiency - with a single request.creation
extension. Using that extension, however, generates a usage pattern of an initial (creation
) request and an immediate (data upload) follow-on request - a usage pattern that is detected and penalized (with long delays) for background HTTP queues, by at least iOS.In my proposal, I tried to match the language and style of the existing specification as closely as possible.
Feedback welcome! 🙂