ZIP Exports: Added ID checks and testing to validator

BookStackApp · Nov 18, 2024 · e2f6e50 · e2f6e50
1 parent c2c64e2
commit e2f6e50
Show file tree

Hide file tree

Showing 10 changed files with 130 additions and 6 deletions.
diff --git a/app/Exports/ZipExports/Models/ZipExportAttachment.php b/app/Exports/ZipExports/Models/ZipExportAttachment.php
@@ -43,7 +43,7 @@ public static function fromModelArray(array $attachmentArray, ZipExportFiles $fi
     public static function validate(ZipValidationHelper $context, array $data): array
     {
         $rules = [
-            'id'    => ['nullable', 'int'],
+            'id'    => ['nullable', 'int', $context->uniqueIdRule('attachment')],
             'name'  => ['required', 'string', 'min:1'],
             'link'  => ['required_without:file', 'nullable', 'string'],
             'file'  => ['required_without:link', 'nullable', 'string', $context->fileReferenceRule()],

diff --git a/app/Exports/ZipExports/Models/ZipExportBook.php b/app/Exports/ZipExports/Models/ZipExportBook.php
@@ -70,7 +70,7 @@ public static function fromModel(Book $model, ZipExportFiles $files): self
     public static function validate(ZipValidationHelper $context, array $data): array
     {
         $rules = [
-            'id'    => ['nullable', 'int'],
+            'id'    => ['nullable', 'int', $context->uniqueIdRule('book')],
             'name'  => ['required', 'string', 'min:1'],
             'description_html' => ['nullable', 'string'],
             'cover' => ['nullable', 'string', $context->fileReferenceRule()],

diff --git a/app/Exports/ZipExports/Models/ZipExportChapter.php b/app/Exports/ZipExports/Models/ZipExportChapter.php
@@ -59,7 +59,7 @@ public static function fromModelArray(array $chapterArray, ZipExportFiles $files
     public static function validate(ZipValidationHelper $context, array $data): array
     {
         $rules = [
-            'id'    => ['nullable', 'int'],
+            'id'    => ['nullable', 'int', $context->uniqueIdRule('chapter')],
             'name'  => ['required', 'string', 'min:1'],
             'description_html' => ['nullable', 'string'],
             'priority' => ['nullable', 'int'],

diff --git a/app/Exports/ZipExports/Models/ZipExportImage.php b/app/Exports/ZipExports/Models/ZipExportImage.php
@@ -33,7 +33,7 @@ public function metadataOnly(): void
     public static function validate(ZipValidationHelper $context, array $data): array
     {
         $rules = [
-            'id'    => ['nullable', 'int'],
+            'id'    => ['nullable', 'int', $context->uniqueIdRule('image')],
             'name'  => ['required', 'string', 'min:1'],
             'file'  => ['required', 'string', $context->fileReferenceRule()],
             'type'  => ['required', 'string', Rule::in(['gallery', 'drawio'])],

diff --git a/app/Exports/ZipExports/Models/ZipExportPage.php b/app/Exports/ZipExports/Models/ZipExportPage.php
@@ -68,7 +68,7 @@ public static function fromModelArray(array $pageArray, ZipExportFiles $files):
     public static function validate(ZipValidationHelper $context, array $data): array
     {
         $rules = [
-            'id'    => ['nullable', 'int'],
+            'id'    => ['nullable', 'int', $context->uniqueIdRule('page')],
             'name'  => ['required', 'string', 'min:1'],
             'html' => ['nullable', 'string'],
             'markdown' => ['nullable', 'string'],

diff --git a/app/Exports/ZipExports/ZipFileReferenceRule.php b/app/Exports/ZipExports/ZipFileReferenceRule.php
@@ -4,7 +4,6 @@
 
 use Closure;
 use Illuminate\Contracts\Validation\ValidationRule;
-use ZipArchive;
 
 class ZipFileReferenceRule implements ValidationRule
 {

diff --git a/app/Exports/ZipExports/ZipUniqueIdRule.php b/app/Exports/ZipExports/ZipUniqueIdRule.php
@@ -0,0 +1,26 @@
+<?php
+
+namespace BookStack\Exports\ZipExports;
+
+use Closure;
+use Illuminate\Contracts\Validation\ValidationRule;
+
+class ZipUniqueIdRule implements ValidationRule
+{
+    public function __construct(
+        protected ZipValidationHelper $context,
+        protected string $modelType,
+    ) {
+    }
+
+
+    /**
+     * @inheritDoc
+     */
+    public function validate(string $attribute, mixed $value, Closure $fail): void
+    {
+        if ($this->context->hasIdBeenUsed($this->modelType, $value)) {
+            $fail('validation.zip_unique')->translate(['attribute' => $attribute]);
+        }
+    }
+}
diff --git a/app/Exports/ZipExports/ZipValidationHelper.php b/app/Exports/ZipExports/ZipValidationHelper.php
@@ -9,6 +9,13 @@ class ZipValidationHelper
 {
     protected Factory $validationFactory;
 
+    /**
+     * Local store of validated IDs (in format "<type>:<id>". Example: "book:2")
+     * which we can use to check uniqueness.
+     * @var array<string, bool>
+     */
+    protected array $validatedIds = [];
+
     public function __construct(
         public ZipExportReader $zipReader,
     ) {
@@ -31,6 +38,23 @@ public function fileReferenceRule(): ZipFileReferenceRule
         return new ZipFileReferenceRule($this);
     }
 
+    public function uniqueIdRule(string $type): ZipUniqueIdRule
+    {
+        return new ZipUniqueIdRule($this, $type);
+    }
+
+    public function hasIdBeenUsed(string $type, int $id): bool
+    {
+        $key = $type . ':' . $id;
+        if (isset($this->validatedIds[$key])) {
+            return true;
+        }
+
+        $this->validatedIds[$key] = true;
+
+        return false;
+    }
+
     /**
      * Validate an array of relation data arrays that are expected
      * to be for the given ZipExportModel.

diff --git a/lang/en/validation.php b/lang/en/validation.php
@@ -107,6 +107,7 @@
 
     'zip_file' => 'The :attribute needs to reference a file within the ZIP.',
     'zip_model_expected' => 'Data object expected but ":type" found.',
+    'zip_unique' => 'The :attribute must be unique for the object type within the ZIP.',
 
     // Custom validation lines
     'custom' => [

diff --git a/tests/Exports/ZipExportValidatorTests.php b/tests/Exports/ZipExportValidatorTests.php
@@ -0,0 +1,74 @@
+<?php
+
+namespace Tests\Exports;
+
+use BookStack\Entities\Models\Book;
+use BookStack\Entities\Models\Chapter;
+use BookStack\Entities\Models\Page;
+use BookStack\Exports\ZipExports\ZipExportReader;
+use BookStack\Exports\ZipExports\ZipExportValidator;
+use BookStack\Exports\ZipExports\ZipImportRunner;
+use BookStack\Uploads\Image;
+use Tests\TestCase;
+
+class ZipExportValidatorTests extends TestCase
+{
+    protected array $filesToRemove = [];
+
+    protected function tearDown(): void
+    {
+        foreach ($this->filesToRemove as $file) {
+            unlink($file);
+        }
+
+        parent::tearDown();
+    }
+
+    protected function getValidatorForData(array $zipData, array $files = []): ZipExportValidator
+    {
+        $upload = ZipTestHelper::zipUploadFromData($zipData, $files);
+        $path = $upload->getRealPath();
+        $this->filesToRemove[] = $path;
+        $reader = new ZipExportReader($path);
+        return new ZipExportValidator($reader);
+    }
+
+    public function test_ids_have_to_be_unique()
+    {
+        $validator = $this->getValidatorForData([
+            'book' => [
+                'id' => 4,
+                'name' => 'My book',
+                'pages' => [
+                    [
+                        'id' => 4,
+                        'name' => 'My page',
+                        'markdown' => 'hello',
+                        'attachments' => [
+                            ['id' => 4, 'name' => 'Attachment A', 'link' => 'https://example.com'],
+                            ['id' => 4, 'name' => 'Attachment B', 'link' => 'https://example.com']
+                        ],
+                        'images' => [
+                            ['id' => 4, 'name' => 'Image A', 'type' => 'gallery', 'file' => 'cat'],
+                            ['id' => 4, 'name' => 'Image b', 'type' => 'gallery', 'file' => 'cat'],
+                        ],
+                    ],
+                    ['id' => 4, 'name' => 'My page', 'markdown' => 'hello'],
+                ],
+                'chapters' => [
+                    ['id' => 4, 'name' => 'Chapter 1'],
+                    ['id' => 4, 'name' => 'Chapter 2']
+                ]
+            ]
+        ], ['cat' => $this->files->testFilePath('test-image.png')]);
+
+        $results = $validator->validate();
+        $this->assertCount(4, $results);
+
+        $expectedMessage = 'The id must be unique for the object type within the ZIP.';
+        $this->assertEquals($expectedMessage, $results['book.pages.0.attachments.1.id']);
+        $this->assertEquals($expectedMessage, $results['book.pages.0.images.1.id']);
+        $this->assertEquals($expectedMessage, $results['book.pages.1.id']);
+        $this->assertEquals($expectedMessage, $results['book.chapters.1.id']);
+    }
+}