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

Enhancements to getAuthenticatedURL (allow response override headers, fixes for clock drift on short-expiry links) #25

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

jessevondoom
Copy link

  • 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())

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

leto commented Apr 5, 2012

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.

@leto
Copy link

leto commented Apr 6, 2012

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

@leto
Copy link

leto commented Apr 6, 2012

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.

@jessevondoom
Copy link
Author

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

@jessevondoom
Copy link
Author

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.

@leto
Copy link

leto commented Apr 11, 2012

Update: I am using this pull request in production to set various headers (Content-Disposition, Content-Type and Cache-Control) and it works awesomely.

@torgospizza
Copy link

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!

tpyo added a commit that referenced this pull request Oct 5, 2013
… an offset to apply to incorrect system times (fixes issues #23 and #25)
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.

3 participants