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

Add support for Private Attachments #398

Merged
merged 12 commits into from
May 20, 2020
Merged

Add support for Private Attachments #398

merged 12 commits into from
May 20, 2020

Conversation

joehoyle
Copy link
Member

This is an experiment to support private / unpublished uploads. These files are pushed to S3 with a private ACL, and then URLs are generated and signed when pages are generated, with a time-based expiry.

This is an experiment to support private / unpublished uploads. These files are pushed to S3 with a private ACL, and then URLs are generated and signed when pages are generated, with a time-based expiry.
Copy link

@hm-linter hm-linter bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Linting failed (1005 errors).

(984 notices occurred in your codebase, but were on files/lines not included in this PR.)

@joehoyle
Copy link
Member Author

Left to do:

  • Support for detecting the object-acl on upload. Right now this is set in the sreamwrapper, so there is no Attachment ID in context, need to somehow pull that out.
  • Right now there's no UI or way to mark an attachment as private.
  • The expiry time of the signed URLs should be configurable.

@joehoyle joehoyle requested a review from rmccue April 20, 2020 12:25
Copy link
Contributor

@stuartshields stuartshields left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@joehoyle I'm sure I'll have more to come.

inc/class-s3-uploads.php Outdated Show resolved Hide resolved
@joehoyle
Copy link
Member Author

joehoyle commented May 4, 2020

@stuartshields hey, wanted to check how you got on with this -- I noticed you add a couple of commits. Is this branch working for you now?

@stuartshields
Copy link
Contributor

@joehoyle The branch is working for me, I pushed it to our dev server to make sure it works there. The only issue we're facing is Tachyon but I think updating the Tachyon version should resolve this.

When using temporary IAM credentials (such as IAM instance profiles do) then signed URLs only last for 6 hours.
README.md Outdated Show resolved Hide resolved
IAM instance profiles can only sign urls for up to 6 hours, so it's better to use 6 as a default.
@joehoyle
Copy link
Member Author

@stuartshields did you do an implementation for updating / replacing the URLs in post content? Thinking that should probably be added to S3 Uploads in this PR, as things are kinda broken without it.

This is an open source repo, but maybe you can DM me with any specific links to code where you have implemented that.

* @param integer $post_id
* @return array|false
*/
public function add_s3_signed_params_to_attachment_image_src( $image, int $post_id ) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@joehoyle So we didn't take into the account that wp_get_attachment_image_src can accept both array|false as seen here https://developer.wordpress.org/reference/functions/wp_get_attachment_image_src/ I actually came across this on the project.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good catch, thanks @stuartshields

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