Exports: Updated perm checking for images in ZIP exports

For #5885
Adds to, uses and cleans-up central permission checking in ImageService
to mirror that which would be experienced by users in the UI to result
in the same image access conditions.

Adds testing to cover.

diff --git a/app/Exports/ZipExports/ZipExportReferences.php b/app/Exports/ZipExports/ZipExportReferences.php
index 64107cf21aa0a7ba68503f6adf6260addfb14cd7..a79988d44fe6244665cb0dfd189ba5e22be99bd5 100644 (file)
--- a/app/Exports/ZipExports/ZipExportReferences.php
+++ b/app/Exports/ZipExports/ZipExportReferences.php
@@ -15,6 +15,7 @@ use BookStack\Exports\ZipExports\Models\ZipExportPage;
 use BookStack\Permissions\Permission;
 use BookStack\Uploads\Attachment;
 use BookStack\Uploads\Image;
+use BookStack\Uploads\ImageService;
 
 class ZipExportReferences
 {
@@ -33,6 +34,7 @@ class ZipExportReferences
 
     public function __construct(
         protected ZipReferenceParser $parser,
+        protected ImageService $imageService,
     ) {
     }
 
@@ -133,10 +135,17 @@ class ZipExportReferences
                 return "[[bsexport:image:{$model->id}]]";
             }
 
-            // Find and include images if in visibility
+            // Get the page which we'll reference this image upon
             $page = $model->getPage();
-            $pageExportModel = $this->pages[$page->id] ?? ($exportModel instanceof ZipExportPage ? $exportModel : null);
-            if (isset($this->images[$model->id]) || ($page && $pageExportModel && userCan(Permission::PageView, $page))) {
+            $pageExportModel = null;
+            if ($page && isset($this->pages[$page->id])) {
+                $pageExportModel = $this->pages[$page->id];
+            } elseif ($exportModel instanceof ZipExportPage) {
+                $pageExportModel = $exportModel;
+            }
+
+            // Add the image to the export if it's accessible or just return the existing reference if already added
+            if (isset($this->images[$model->id]) || ($pageExportModel && $this->imageService->imageAccessible($model))) {
                 if (!isset($this->images[$model->id])) {
                     $exportImage = ZipExportImage::fromModel($model, $files);
                     $this->images[$model->id] = $exportImage;
@@ -144,6 +153,7 @@ class ZipExportReferences
                 }
                 return "[[bsexport:image:{$model->id}]]";
             }
+
             return null;
         }
 

diff --git a/app/Uploads/Image.php b/app/Uploads/Image.php
index 20def9de655804f9edb8aaa5441dce4425b38cb4..81b6db6fd22a96f65668bb92e6dd5de14b14864e 100644 (file)
--- a/app/Uploads/Image.php
+++ b/app/Uploads/Image.php
@@ -13,14 +13,14 @@ use Illuminate\Database\Eloquent\Factories\HasFactory;
 use Illuminate\Database\Eloquent\Relations\HasMany;
 
 /**
- * @property int    $id
- * @property string $name
- * @property string $url
- * @property string $path
- * @property string $type
- * @property int    $uploaded_to
- * @property int    $created_by
- * @property int    $updated_by
+ * @property int      $id
+ * @property string   $name
+ * @property string   $url
+ * @property string   $path
+ * @property string   $type
+ * @property int|null $uploaded_to
+ * @property int      $created_by
+ * @property int      $updated_by
  */
 class Image extends Model implements OwnableInterface
 {

diff --git a/app/Uploads/ImageService.php b/app/Uploads/ImageService.php
index fadafc8e527b5bde2012323e9db1fe692517ae80..a26a04ac5c63d82fe10af97d1c34c2e0c6d8553b 100644 (file)
--- a/app/Uploads/ImageService.php
+++ b/app/Uploads/ImageService.php
@@ -148,7 +148,7 @@ class ImageService
     }
 
     /**
-     * Destroy an image along with its revisions, thumbnails and remaining folders.
+     * Destroy an image along with its revisions, thumbnails, and remaining folders.
      *
      * @throws Exception
      */
@@ -252,33 +252,48 @@ class ImageService
     {
         $disk = $this->storage->getDisk('gallery');
 
+        return $disk->usingSecureImages() && $this->pathAccessible($imagePath);
+    }
+
+    /**
+     * Check if the given path exists and is accessible depending on the current settings.
+     */
+    public function pathAccessible(string $imagePath): bool
+    {
         if ($this->storage->usingSecureRestrictedImages() && !$this->checkUserHasAccessToRelationOfImageAtPath($imagePath)) {
             return false;
         }
 
-        // Check local_secure is active
-        return $disk->usingSecureImages()
-            // Check the image file exists
-            && $disk->exists($imagePath)
-            // Check the file is likely an image file
-            && str_starts_with($disk->mimeType($imagePath), 'image/');
+        if ($this->storage->usingSecureImages() && user()->isGuest()) {
+            return false;
+        }
+
+        return $this->imageFileExists($imagePath, 'gallery');
     }
 
     /**
-     * Check if the given path exists and is accessible depending on the current settings.
+     * Check if the given image should be accessible to the current user.
      */
-    public function pathAccessible(string $imagePath): bool
+    public function imageAccessible(Image $image): bool
     {
-        $disk = $this->storage->getDisk('gallery');
+        if ($this->storage->usingSecureRestrictedImages() && !$this->checkUserHasAccessToRelationOfImage($image)) {
+            return false;
+        }
 
-        if ($this->storage->usingSecureRestrictedImages() && !$this->checkUserHasAccessToRelationOfImageAtPath($imagePath)) {
+        if ($this->storage->usingSecureImages() && user()->isGuest()) {
             return false;
         }
 
-        // Check local_secure is active
-        return $disk->exists($imagePath)
-            // Check the file is likely an image file
-            && str_starts_with($disk->mimeType($imagePath), 'image/');
+        return $this->imageFileExists($image->path, $image->type);
+    }
+
+    /**
+     * Check if the given image path exists for the given image type and that it is likely an image file.
+     */
+    protected function imageFileExists(string $imagePath, string $imageType): bool
+    {
+        $disk = $this->storage->getDisk($imageType);
+        return $disk->exists($imagePath) && str_starts_with($disk->mimeType($imagePath), 'image/');
     }
 
     /**
@@ -307,6 +322,11 @@ class ImageService
             return false;
         }
 
+        return $this->checkUserHasAccessToRelationOfImage($image);
+    }
+
+    protected function checkUserHasAccessToRelationOfImage(Image $image): bool
+    {
         $imageType = $image->type;
 
         // Allow user or system (logo) images

diff --git a/app/Uploads/ImageStorage.php b/app/Uploads/ImageStorage.php
index ddaa26a9400c343b2dea6bf32dd118f1a5e79bfc..38a22e3b498e3f811a0526921147f19bf3297a67 100644 (file)
--- a/app/Uploads/ImageStorage.php
+++ b/app/Uploads/ImageStorage.php
@@ -34,6 +34,15 @@ class ImageStorage
         return config('filesystems.images') === 'local_secure_restricted';
     }
 
+    /**
+     * Check if "local secure" (Fetched behind auth, either with or without permissions enforced)
+     * is currently active in the instance.
+     */
+    public function usingSecureImages(): bool
+    {
+        return config('filesystems.images') === 'local_secure' || $this->usingSecureRestrictedImages();
+    }
+
     /**
      * Clean up an image file name to be both URL and storage safe.
      */

diff --git a/tests/Exports/ZipExportTest.php b/tests/Exports/ZipExportTest.php
index 692a5910f3c2c05bc067679fc6f8e856d3655352..063f6d9425a0aea0dded5a6b5f09a72fa99fe9dd 100644 (file)
--- a/tests/Exports/ZipExportTest.php
+++ b/tests/Exports/ZipExportTest.php
@@ -374,6 +374,54 @@ class ZipExportTest extends TestCase
         $this->assertStringContainsString("<a href=\"{$ref}\">Original URL</a><a href=\"{$ref}\">Storage URL</a>", $pageData['html']);
     }
 
+    public function test_orphaned_images_can_be_used_on_default_local_storage()
+    {
+        $this->asEditor();
+        $page = $this->entities->page();
+        $result = $this->files->uploadGalleryImageToPage($this, $page);
+        $displayThumb = $result['response']->thumbs->gallery ?? '';
+        $page->html = '<p><img src="' . $displayThumb . '" alt="My image"></p>';
+        $page->save();
+
+        $image = Image::findOrFail($result['response']->id);
+        $image->uploaded_to = null;
+        $image->save();
+
+        $zipResp = $this->asEditor()->get($page->getUrl("/export/zip"));
+        $zipResp->assertOk();
+        $zip = ZipTestHelper::extractFromZipResponse($zipResp);
+        $pageData = $zip->data['page'];
+
+        $this->assertCount(1, $pageData['images']);
+        $imageData = $pageData['images'][0];
+        $this->assertEquals($image->id, $imageData['id']);
+
+        $this->assertEquals('<p><img src="[[bsexport:image:' . $imageData['id'] . ']]" alt="My image"></p>', $pageData['html']);
+    }
+
+    public function test_orphaned_images_cannot_be_used_on_local_secure_restricted()
+    {
+        config()->set('filesystems.images', 'local_secure_restricted');
+
+        $this->asEditor();
+        $page = $this->entities->page();
+        $result = $this->files->uploadGalleryImageToPage($this, $page);
+        $displayThumb = $result['response']->thumbs->gallery ?? '';
+        $page->html = '<p><img src="' . $displayThumb . '" alt="My image"></p>';
+        $page->save();
+
+        $image = Image::findOrFail($result['response']->id);
+        $image->uploaded_to = null;
+        $image->save();
+
+        $zipResp = $this->asEditor()->get($page->getUrl("/export/zip"));
+        $zipResp->assertOk();
+        $zip = ZipTestHelper::extractFromZipResponse($zipResp);
+        $pageData = $zip->data['page'];
+
+        $this->assertCount(0, $pageData['images']);
+    }
+
     public function test_cross_reference_links_external_to_export_are_not_converted()
     {
         $page = $this->entities->page();