-
Notifications
You must be signed in to change notification settings - Fork 503
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
Enhancements to getAuthenticatedURL (allow response override headers, fixes for clock drift on short-expiry links) #25
base: master
Are you sure you want to change the base?
Conversation
- Added a $headers argument that can take an assoociative array of response override headers (like content-disposition, etc.) Full AMZ documentation on the various options is at: http://docs.amazonwebservices.com/AmazonS3/latest/API/RESTObjectGET.html - Added a getAWSSystemTime() method to poll AWS servers for their clock time. Used in the case of a short-expiry requests (< 120 seconds) which can be problematic on heavy-load shared servers due to clock drift. (Shockingly significant in some situations.) - Added Date header to S3Response (needed by getAWSSystemTime())
…output in the test report...dumb
A huge +1 . Being able to override the Content-Disposition is absolutely necessary to give users who download files nice filenames which make sense to them, which are different than the s3 keys they are stored by. |
FYI, I am using this patch in development and it seems to work great when setting some headers, but I am getting a SignatureDoesNotMatch error from S3 when trying to set the Content-Disposition with a filename. Perhaps I am doing something wrong? /cc @jessevondoom |
Actually, I had a typo and I was spelling "response-content-disposition" as "content-disposition" which gets ignored and then makes the returned link invalid. We may want to add some code to this which throws an error when it sees an unknown header. In any case, this code works and another big +1 to merge. |
Could it be an encoding issue maybe? (http://stackoverflow.com/questions/93551/how-to-encode-the-filename-parameter-of-content-disposition-header-in-http) — I'll try to play around with adding some tests and see if I can't get a pass. Here's the test suite we're running against it. (Want to separate it from the larger suite too at some point...) https://github.com/cashmusic/DIY/blob/master/tests/php/103_S3Seed.php |
Ah okay. Yeah I ran into that same issue trying to decipher the Amazon docs. A check (or even straight-up dropping invalid headers) is a good idea — left it out to allow for them expanding the set of allowed headers but that's not hard to code around. |
Update: I am using this pull request in production to set various headers (Content-Disposition, Content-Type and Cache-Control) and it works awesomely. |
We are also using this in production to serve content from edge cache servers using S3 as the origin. To make sure content is stored at the edge but also re-authenticated every time, we had to override the headers, and this PRs version of s3.inc works exactly as we need it to. Thanks! |
override headers (like content-disposition, etc.) Full AMZ documentation on
the various options is at:
http://docs.amazonwebservices.com/AmazonS3/latest/API/RESTObjectGET.html
Used in the case of a short-expiry requests (< 120 seconds) which can be
problematic on heavy-load shared servers due to clock drift. (Shockingly
significant in some situations.)