Merge pull request #165 from christopherdarling/1

FIX FlysystemAssetStore::getAsURL() only grant for protected filesystems

src/Flysystem/FlysystemAssetStore.php

@@ -217,9 +217,6 @@ public function getAsString($filename, $hash, $variant = null)
 
     public function getAsURL($filename, $hash, $variant = null, $grant = true)
     {
-        if ($grant) {
-            $this->grant($filename, $hash);
-        }
         $fileID = $this->getFileID($filename, $hash, $variant);
 
         // Check with filesystem this asset exists in
@@ -229,11 +226,15 @@ public function getAsURL($filename, $hash, $variant = null, $grant = true)
             /** @var PublicAdapter $publicAdapter */
             $publicAdapter = $public->getAdapter();
             return $publicAdapter->getPublicUrl($fileID);
-        } else {
-            /** @var ProtectedAdapter $protectedAdapter */
-            $protectedAdapter = $protected->getAdapter();
-            return $protectedAdapter->getProtectedUrl($fileID);
         }
+
+        if ($grant) {
+            $this->grant($filename, $hash);
+        }
+
+        /** @var ProtectedAdapter $protectedAdapter */
+        $protectedAdapter = $protected->getAdapter();
+        return $protectedAdapter->getProtectedUrl($fileID);
     }
 
     public function setFromLocalFile($path, $filename = null, $hash = null, $variant = null, $config = array())

src/Shortcodes/ImageShortcodeProvider.php

@@ -52,7 +52,8 @@ public static function handle_shortcode($args, $content, $parser, $shortcode, $e
             /** @var AssetStore $store */
             $store = Injector::inst()->get(AssetStore::class);
             if (!empty($item['filename'])) {
-                $store->grant($item['filename'], $item['hash']);
+                // Initiate a protected asset grant if necessary
+                $store->getAsURL($item['filename'], $item['hash']);
             }
             return $item['markup'];
         }

tests/php/Flysystem/FlysystemAssetStoreTest.php

@@ -0,0 +1,109 @@
+<?php
+
+namespace SilverStripe\Assets\Tests\Flysystem;
+
+use League\Flysystem\Filesystem;
+use PHPUnit_Framework_MockObject_MockObject;
+use SilverStripe\Assets\Flysystem\FlysystemAssetStore;
+use SilverStripe\Assets\Flysystem\ProtectedAssetAdapter;
+use SilverStripe\Assets\Flysystem\PublicAssetAdapter;
+use SilverStripe\Dev\SapphireTest;
+
+class FlysystemAssetStoreTest extends SapphireTest
+{
+    /**
+     * @var PublicAssetAdapter
+     */
+    protected $publicAdapter;
+
+    /**
+     * @var Filesystem
+     */
+    protected $publicFilesystem;
+
+    /**
+     * @var ProtectedAssetAdapter
+     */
+    protected $protectedAdapter;
+
+    /**
+     * @var Filesystem
+     */
+    protected $protectedFilesystem;
+
+    protected function setUp()
+    {
+        parent::setUp();
+
+        $this->publicAdapter = $this->getMockBuilder(PublicAssetAdapter::class)
+            ->setMethods(['getPublicUrl'])
+            ->getMock();
+
+        $this->publicFilesystem = $this->getMockBuilder(Filesystem::class)
+            ->setMethods(['has'])
+            ->setConstructorArgs([$this->publicAdapter])
+            ->getMock();
+
+        $this->protectedAdapter = $this->getMockBuilder(ProtectedAssetAdapter::class)
+            ->setMethods(['getProtectedUrl'])
+            ->getMock();
+
+        $this->protectedFilesystem = $this->getMockBuilder(Filesystem::class)
+            ->setMethods(['has'])
+            ->setConstructorArgs([$this->protectedAdapter])
+            ->getMock();
+    }
+
+    public function testGetAsUrlDoesntGrantForPublicAssets()
+    {
+        $this->publicFilesystem->expects($this->once())->method('has')->willReturn(true);
+        $this->publicAdapter->expects($this->once())->method('getPublicUrl')->willReturn('public.jpg');
+        $this->protectedFilesystem->expects($this->never())->method('has');
+
+        /** @var FlysystemAssetStore|PHPUnit_Framework_MockObject_MockObject $assetStore */
+        $assetStore = $this->getMockBuilder(FlysystemAssetStore::class)
+            ->setMethods(['getFileID'])
+            ->getMock();
+        $assetStore->expects($this->once())->method('getFileID')->willReturn('public.jpg');
+
+        $assetStore->setPublicFilesystem($this->publicFilesystem);
+        $assetStore->setProtectedFilesystem($this->protectedFilesystem);
+
+        $this->assertSame('public.jpg', $assetStore->getAsURL('foo', 'bar'));
+    }
+
+    /**
+     * @param boolean $grant
+     * @dataProvider protectedUrlGrantProvider
+     */
+    public function testGetAsUrlWithGrant($grant)
+    {
+        $this->publicFilesystem->expects($this->once())->method('has')->willReturn(false);
+        $this->publicAdapter->expects($this->never())->method('getPublicUrl');
+        $this->protectedFilesystem->expects($this->once())->method('has')->willReturn(true);
+        $this->protectedAdapter->expects($this->once())->method('getProtectedUrl')->willReturn('protected.jpg');
+
+        /** @var FlysystemAssetStore|PHPUnit_Framework_MockObject_MockObject $assetStore */
+        $assetStore = $this->getMockBuilder(FlysystemAssetStore::class)
+            ->setMethods(['getFileID', 'grant'])
+            ->getMock();
+        $assetStore->expects($this->once())->method('getFileID')->willReturn('protected.jpg');
+        $assetStore->expects($grant ? $this->once() : $this->never())->method('grant');
+
+        $assetStore->setPublicFilesystem($this->publicFilesystem);
+        $assetStore->setProtectedFilesystem($this->protectedFilesystem);
+
+        $this->assertSame('protected.jpg', $assetStore->getAsURL('foo', 'bar', 'baz', $grant));
+    }
+
+    /**
+     * @return array[]
+     */
+    public function protectedUrlGrantProvider()
+    {
+        return [
+            [true],
+            [false],
+        ];
+    }
+}