Add support for private uploads / presigned URLs

Wow this is quite a patch! Seems like Drupal is really fighting us here.

1. +++ b/README.md
   @@ -53,6 +53,9 @@ $schemes = [
   +      'expires' => '+600 seconds',                // If a file is private, the
   +                                                  // expiry time of a presigned
   +                                                  // URL.
   

   I think we're commenting out optional configuration.

2. +++ b/flysystem_s3.services.yml
   @@ -0,0 +1,6 @@
   +services:
   +  flysystem_s3.file_system:
   +    public: false
   +    class: \Drupal\flysystem_s3\File\FileSystem
   +    decorates: file_system
   +    arguments: ['@settings', '@flysystem_s3.file_system.inner']
   

   I did not know about decorates. Nifty.

3. +++ b/src/File/FileSystem.php
   @@ -0,0 +1,174 @@
   +   * @param \Drupal\Core\Site\Settings $settings
   +   *   The site settings.
   +   * @param \Drupal\Core\File\FileSystemInterface $file_system
   +   *   The file system being decorated.
   +   */
   +  public function __construct(Settings $settings, FileSystemInterface $file_system) {
   +    $this->settings = $settings;
   +    $this->fileSystem = $file_system;
   

   Are we supposed to inject Settings? I've done it in the past, but I don't think there's any value. Also, it seems like an easy way to leak keys during exceptions. (That's a separate problem, but sightly related.)

4. +++ b/src/File/FileSystem.php
   @@ -0,0 +1,174 @@
   +    if ($this->isPrivateS3Scheme($scheme)) {
   +      // TODO: Can we just assume 0700 / 0600 permissions here, without this
   +      // first if?
   +      if ($mode) {
   +        $mode = $mode & 0700;
   +      }
   +      else {
   +        is_dir($uri) ? $mode = 0700 : $mode = 0600;
   +      }
   +    }
   

   Looks like it :) We might want to re-examine Drupal\flysystem_s3\Flysystem\Adapter\S3Adapter::getMetadata() and how I've hacked on directory support.

1. +++ b/composer.json
   @@ -3,5 +3,8 @@
   +  },
   +  "require-dev": {
   +    "mikey179/vfsStream": "^1.6"
   

   What's up with this?

2. +++ b/src/File/FileSystem.php
   @@ -0,0 +1,187 @@
   +/**
   + * Decorates the Drupal FileSystem service to handle chmod() for S3.
   + */
   +class FileSystem implements FileSystemInterface {
   

   Would it make sense to move this to the Flysystem module? Can we make it generic? Probably not.

This is looking pretty good. I've taken a break from D.O stuffs for a bit, but I'm back now. I will consider this shortly.

Patch in #18 works great... one issue I ran into was the itok token being added for image styles... it interferes with access to the image when using a private ACL. Addressed the issue by adding the following to my settings.php file, $config['image.settings']['suppress_itok_output'] = TRUE;

I've applied the patch and am successfully managing files in a restricted bucket from Drupal! Based on the base flysystem README

// This will be treated similarly
// Drupal's private file system.

I expected the url exposed to be a drupal private file type url but instead I see an http://s3.amazonaws.com/ - if that's not in the scope of this issue I'll open a new issue, but is this possible?

I've been using this patch successfully for a few days now; However, I've run into an issue while using this patch with jsonapi_file module.

While trying to post the files via jsonapi routes I am receiving a 500 - Internal Server Error

The controller result claims to be providing relevant cache metadata, but leaked metadata was detected. Please ensure you are not rendering content too early. Returned object class: Drupal\\jsonapi\\ResourceResponse.

This is resolved if I update the render call in S3.php from $this->renderer->render($build); to this: $this->renderer->renderPlain($build);

@matthew.perry take a look at #2883086: [PP-1] Port RequestHandler + ResourceResponseSubscriber improvements from REST module to JSON API

The use of $this->renderer->render($build) was causing a fatal error from \Drupal\Core\Render\Renderer.php:

"Render context is empty, because render() was called outside of a renderRoot() or renderPlain() call. Use renderPlain()/renderRoot() or #lazy_builder/#pre_render instead."

I got this error by using hook_page_attachments to add a Facebook image metatag using file_create_url which then calls Drupal\flysystem_s3\Flysystem\S3->getExternalUrl(). I'm thinking the render system does not like having render called during hook_page_attachments.

$page['#attached']['html_head'][] = array(
      array(
        '#tag' => 'meta',
        '#attributes' => array(
          'name' => 'og:image',
          'content' => file_create_url($photo->getImage()->entity->getFileUri()),
        ),
      ),
      'og:image',
    );

I don't know what renderPlain and renderRoot are, but I switched the patch to use renderRoot like the error suggests and that resolves the problem.

I was seeing a similar error:

LogicException: Render context is empty, because render() was called outside of a renderRoot() or renderPlain()
call. Use renderPlain()/renderRoot() or #lazy_builder/#pre_render instead. in
Drupal\Core\Render\Renderer->doRender() (line 241 of
/mnt/www/html/site/docroot/core/lib/Drupal/Core/Render/Renderer.php).

So I applied the patch in #25, which resolved that error, but when attempting to render image content in a modal, a new error appeared:

Uncaught PHP Exception LogicException: "A stray renderRoot() invocation is causing bubbling of attached assets to break." at /mnt/www/html/site/docroot/core/lib/Drupal/Core/Render/Renderer.php line 133 request_id="v-0b0fcdda-923f-11e7-82a3-0e2839910d74"

Investigating an approach that fixes both.

I've been having performance issues with this patch on pages with many images.

I tracked it down to the addition of $this->getAdapter()->getVisibility($target)['visibility'] added to getExternalUrl. This check causes an s3 request to run, which typically takes several tenths of a second. If you are showing 100 images on a page and each one has a responsive srcset of 3 images, that really adds up.

As I'm mainly using public images I was able to work around it by first checking
$this->options['ACL'] != 'public-read'
before doing the getVisibility check.

@jody-lynn we hit the same performance issue. To get our site launched in time, we switch from responsive image to a single optimized image for the image field on our view page, which with breakpoints and pixel density alternatives needed 180 image files for responsive. We also added code elsewhere to pre-populate the derivatives in S3 on save - which decreased our load time dramatically - by a factor of 15.

Our testing via blackfire showed over 6,800 curl calls to S3 on this page all tracing back to

_responsive_image_build_source_attributes()

. This seems like it's crying out for a caching strategy - which I'm happy to collaborate on - or any other ideas to speed up the performance for interaction with Drupal's image system. Until then we are limiting responsive images for S3 stored field.

I adjusted this patch to use a call to

has()

to avoid an actual request through

file_exists

. See if this improves the performance:

if (strpos($target, 'styles/') === 0 && !$this->getAdapter()->has($target)) {
  $this->generateImageStyle($target);
}

In my own implementation I've completely commented out the code here to use render to add a max-age of 0. Instead I'm adding a max-age that matches my expire value in a

hook_preprocess_image()

call.

Thanks Father Shawn! I had just found that file_exists in getExternalUrl as another place calling out to S3 as well so that patch will help a lot.

I added caching on my site around the responsive image style url generation, but every time the caches get cleared we'd be back to 15s page loads. At one point I added really aggressive 'caching' that just stored the generated responsive image URLs to a field to stay there forever, but this proved problematic each time we actually did want to adjust file sizes.

In my own implementation I've completely commented out the code here to use render to add a max-age of 0. Instead I'm adding a max-age that matches my expire value in a hook_preprocess_image() call.

Which doesn't work for the same reason that is noted in the patch. I could even see caching the url as you say, but it needs an age limit for the private aspect to work. I don't see a way around the caching issue here until #2352009: Bubbling of elements' max-age to the page's headers and the page cache is fixed because the whole page gets cached.

My only other idea is that we build a facade path similar to private files and use that to reduce the calls to S3 with cache_tags dependent on the file entity.

I've updated #29 with some fixes that were in a fork were were using to change from calling $this->generateImageStyle($target); and not returning (and affecting performance), to returning $this->generateImageUrl($target);

I think it's also silly because if you miss setting the 'expires' config, that you will not get pre-signed URLs even if the bucket/files are private, which lead to URLS that will 403. So I've moved to making the expires optional, and default it to 1 hour which is the default of the aws s3 presign cli command.

Hi @deviantintegral how to can I help you to test and solve this issue?

This is a patch for the latest beta1 version

Sorry the latest patch are wrong :P

Thanks for the patch @dmouse ! Settings this to Needs review.

Also, I added these parameters in my configuration:

 'cname' => 'cdn.custom.org',
 'cname_is_bucket' => TRUE,
 'endpoint' => 'https://cdn.custom.org',
 'bucket_endpoint' => true,
 'public' => TRUE,
 'expires' => '+10 minutes',

This version leaves the new 'public' config in the documentation above our new expires config.

I have extracted the FileSystem decorator portion of this patch to a new patch on the issue https://www.drupal.org/project/flysystem_s3/issues/3058063.

I need to fix a file permissions error I was getting by did not want to implement signed urls at this stage.

Is there a reason why the decorator class does not extend FileSystem? In my patch I extended it as my thinking is it would be less vulnerable to breaking if FileSystem is modified.

Here's an updated patch to account for the new abstract methods added to FileSystemInterface in 8.7.

Using the new interface constants will prevent this from working on existing Drupal 8.6 installs, so if we replace them with the global constants in file.inc, and do a method_exists() check on $this->fileSystem, we should make it compatible with both.

Why are you not extending the original FileSystem class? Wouldn't that prevent having to add these methods when the interface changes?

Extending is a safe option only if we know 100% for sure that no other modules would want to extend/decorate the FileSystem class. Using decorators is a much safer way to ensure that modules trying to modify a core service will work together instead of conflict with each other.

More resources:
https://www.lullabot.com/articles/get-a-decorator-for-your-drupal-home
https://symfony.com/doc/current/service_container/service_decoration.html
https://stackoverflow.com/questions/4842978/decorator-pattern-versus-sub...

Has anyone done a fix to this patch so it'll apply?

Unfortunately patching against 2.0.0rc1 gives the following error re renderRoot:

LogicException: A stray renderRoot() invocation is causing bubbling of attached assets to break. in Drupal\Core\Render\Renderer->renderRoot() (line 138 of /var/www/html/core/lib/Drupal/Core/Render/Renderer.php).

Does this affect assets themselves or just their display in the Drupal editing page? If it's the latter we may not care for our purposes.

I'm trying to migrate files to s3 private file system and getting errors because it tries to create folders. I think it shouldn't do any folder creation on S3, right?

So here it goes to the core class.

In the core class there's the prepareDestination().

And in that function this fails:

      if (!$this->prepareDirectory($dirname)) {
        $this->logger->error("The specified file '%original_source' could not be copied because the destination directory is not properly configured. This may be caused by a problem with file or directory permissions.", [
          '%original_source' => $original_source,
        ]);
        throw new DirectoryNotReadyException("The specified file '$original_source' could not be copied because the destination directory is not properly configured. This may be caused by a problem with file or directory permissions.");
      }


So I get these kind of errors in log:
<code> [error]  The specified file '/private/ts/adobe_captivate/archives/Working_with_grids.zip' could not be copied because the destination directory is not properly configured. This may be caused by a problem with file or directory permissions. 

I thought I'd do something like this in the decorator class:

 /**
   * {@inheritdoc}
   *
   * @codeCoverageIgnore
   */
  public function prepareDirectory(&$directory, $options = FILE_MODIFY_PERMISSIONS) {
    return TRUE;
  }

but as you saw from the first screenshot, it executes the original method.

What to do with this?

Giving it another try - adjusting against latest dev

1 out of 10 hunks of Interdiff fails but I'll add it here anyway.

Adding new patch.

1. I was still getting "The file permissions could not be set on sxf://2020-Downloads-Icon-TS-128x128.svg. " during migration so I copy pasted the original copy() method and removed chmod. Also could somebody explain why try to chmod files on s3 in the first place? Is that even possible?

2. Secondly I was starting to get this:
Error: Call to protected method Drupal\Core\File\FileSystem::prepareDestination() from context 'Drupal\flysystem_s3\File\FlysystemS3FileSystem' in /app/web/modules/contrib/flysystem_s3/src/File/FlysystemS3FileSystem.php on line 272 #

So I changed the code to extend FileSystem. Probably related to this https://gonzalo123.com/tag/programming/

3. I updated realpath() to get the path on S3. Flysystem itself decorates the original class and the method just returns false. Maybe we should decorate StreamWrapperManager and update the method in that way?

PS. Why are we extending all the functions and then just calling the original?

Just tried latest patch from #50

I get the following fatal error when I edit a node that already has an existing file upload:

The website encountered an unexpected error. Please try again later.
Error: Call to undefined method Drupal\flysystem_s3\Flysystem\S3::generateImageUrl() in Drupal\flysystem_s3\Flysystem\S3->getExternalUrl() (line 244 of modules/contrib/flysystem_s3/src/Flysystem/S3.php).

So I've gone searching fro this generateImageUrl() function. But can't seem to find it anywhere.
It's not in:

• The flysystem_s3 module
• The flysystem module
• Drupal core

Can anyone help me out?

Oh I think I found it in #2661588: Support delivering local image styles until remote upload is complete

I guess this patch depends on that one?

Unfortunately for us, we are tied to S3. So I have continued to work with this patch.

Here's a new patch that fixes the following updates:

• Removes the patch from #2695695: Return a generated URL instead of uploading an image style that was added in #13. The other patch never made it in, and was dependent on a patch to the flysystem module (which itself has not made it in). Removing this change fixes the error I reported in #51
• Changes $this->renderer->renderRoot($build) to $this->renderer->renderPlain($build). This fixes fatal errors reported in comments #26 #47. I was able to reproduce those errors simply by uploading an image (I would get a fatal error on the Ajax request to upload the image). So this change is crucial to actually allow image uploads (at least in my instance).
  Please note I haven't fully understood the consequences of switching to renderPlain(). But it appears to work fine for me right now.
• Adds a note to the README.md about setting $config['image.settings']['suppress_itok_output'] = TRUE; as per comment from #20

Previous patch wasn't setting max-age, so Drupal's render caching would eventually return expired urls.

I've added a simple hook_file_load() to add the correct max-age. However, this depends on the $settings['flysystem']['s3']['config']['expires'] being implicitly set in settings.php (when actually it is optional, with a default of 3600 set in S3.php). I didn't have time to work out a way to get the default value from the plugin.

Also, if page caching is enabled you will most likely get expired urls anyway.

I'm setting this to needs work, I don't think this is ready to be merged into the module as it currently stands. But it looks as though many are using the patch (or various versions of it) in production. So it's a case of use-at-your-own-risk for now :-)

New patch incoming.

I've been running into more max-age issues, and have changed the way max-age is set.

This was the code from the previous patch:

        // This informs the render system that the request has a cache
        // dependency on the time this URL is valid for.
        // TODO: The page cache does not currently respect max-age cache
        // headers. We can't set proper max-age based on the signing time until
        // https://www.drupal.org/node/2352009 is fixed. Unfortunately, this
        // also means we can't cache any pages with signed URLs at all. When we
        // can implement this, we should parse out max-age from the generated
        // URL as suggested at https://github.com/aws/aws-sdk-php/issues/1052.
        $build = [
          '#cache' => [
            'max-age' => 0,
          ],
        ];
        $this->renderer->renderRoot($build);

        // Since the above bug means this max-age isn't respected, we have to
        // kill the general page cache.
        if ($this->killSwitch) {
          $this->killSwitch->trigger();
        }

Firstly, yes page cache is still an issue, and will need to be disabled for any response that has a s3 signature. (Big caveat there for anyone wanting to use this patch).

Secondly, that renderRoot() call doesn't work at all for me (for reasons mentioned in my previous comment). But I've since realised that changing this to renderPlain() was pointless. As the whole reason of this code is to set the max-age to 0 on the current request, whereas using renderPlain() means you just set it within that isolated rendering process.

Instead, I've moved all caching logic to hook_entity_load() (including the page cache kill switch). There may be a better place for this, but this is working for me right now.

Also, a sidenote for anyone using jsonapi. I found that the dynamic_page_cache module is incompatible with this approach, and this must be disabled. This is due to the issue described in #2449749: Add #cache['downstream-ttl'] to force expiration after a certain time and fix #cache['max-age'] logic by adding #cache['age'].
Jsonapi's own cache_jsonapi_normalizations are essentially re-used in the dynamic_page_cache, so if the former is set before the latter, you will end up with expired urls being outputted to the user.
Appears to be specific to jsonapi, as outside of this the dynamic_page_cache module still works ok.

This patch still "needs work". The tests need updating, and I am certain there are situations where max-age bubbling will be an issue.

New stripped down patch:

• Rebased off latest 2.0.x-dev
• Removed all unrelated code that's been added to the patch slowly over time. This new version contains just the parts related to presigned urls.
• Removed previously added requirement of setting $config['image.settings']['suppress_itok_output'] = TRUE;. As this is is a global setting (so may break for sites running a local + s3 filesystem together). Also it looks like it might the setting may be getting removed in Drupal core. Instead we allow the itok parameter to be added to the url, but strip it out using a preprocess function. I looked at how the s3fs module was handling the itok parameter for this.

However, despite me continually working on this, I do not believe we can currently merge this patch in, due to Drupal's internal caching not supporting definitive max-ages.
From my experience, even after using the core patch to fix the internal page cache issue, we still found that expired urls would persist in the cache. It was difficult to debug, as it not clear what generated the cache entry (whether there was any re-use of existing cache's, which therefore weren't passing on the previously set max-age). We've ended up resorting to a nightly cron job to run drush cr, to ensure all our signatures are valid.

Therefore, I'm marking this now as "postponed". Which means essentially, use as-is, and be aware of the potential caching issues that can occur. If the Drupal core page cache issue is fixed, we can look at merging this in.
I've also added a related issue for the s3fs module.

Small update to pass the tests.

So it turns out that using hook_preprocess_image_style for removing the itok param isn't a great solution, as there as instances where this isn't run (we've already encountered this through JSON:API).

However, the security concern I had around using suppress_itok_output is actually incorrect. This setting just handles the output of image style urls, whereas allow_insecure_derivatives is responsible for allowing images to be generated without the token.
Therefore it's safe to use suppress_itok_output. The only issues are:

1. It's a global setting so you would be unable to run a local Drupal filesystem alongside S3+presigned urls on the same site.
2. There's a chance it will be removed at somepoint #2568517: Deprecate allow_insecure_derivatives and suppress_itok_output in image system

It's a global setting so you would be unable to run a local Drupal filesystem alongside S3+presigned urls on the same site.

That's a good reason not to use the global setting for this. You can implement hook_file_url_alter() instead, so that you only remove the token from the URLs in the scheme and/or bucket that you want to target.

Unfortunately, you cannot use hook_file_url_alter() to remove the token, as it’s run before the token is added.

The only way I could see doing it, would be to override the core ImageStyle entity, which is not ideal.
See my comment on https://www.drupal.org/project/drupal/issues/2568517

Here is a reroll of the patch in [2772847-59] against 2.0.x HEAD.

Do not use the patch in 62, it contains errors. Reroll of the reroll incoming.

Note I've discussed this with @deviantintegral and they no longer have a use for this patch. This issue may be closed once I have no further need for it unless anyone else comments that they still require it.

Needs reroll to apply against 2.1.x following #3409513: webblog 10.0.0

Apologies, my PHPStorm settings have mangled the formatting in that push. Will rectify forthwith...

For some reason* the patch generated on gitlab for the current state of this merge request won't apply against current 2.1.x, but a manually generated diff between the merge request branch and 2.1.x will. This is v odd especially as the MR will merge without conflicts.

So here's a patch.

*probably because I somehow did something wrong merging in the changes commit in #3409514: Fix PHPCS and PHPStan issues reported by Gitlab CI 🤷‍♀️