ZIP Exports: Added ID checks and testing to validator

diff --git a/app/Exports/ZipExports/Models/ZipExportAttachment.php b/app/Exports/ZipExports/Models/ZipExportAttachment.php
index c6615e1dc492ab3e3b7b19ea4fde8f90f935c0a2..4f5b2f2369915601f3112ed2d782f05b0b3fd723 100644 (file)
--- a/app/Exports/ZipExports/Models/ZipExportAttachment.php
+++ b/app/Exports/ZipExports/Models/ZipExportAttachment.php
@@ -43,7 +43,7 @@ class ZipExportAttachment extends ZipExportModel
     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
index 0dc4e93d43cb34070a2c53a065259ef0787c44dc..47ab8f0a699ce82f58fee261443eeae9b07e45ee 100644 (file)
--- a/app/Exports/ZipExports/Models/ZipExportBook.php
+++ b/app/Exports/ZipExports/Models/ZipExportBook.php
@@ -70,7 +70,7 @@ class ZipExportBook extends ZipExportModel
     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
index 50440d61a5a771b7c01371dc4eddac0c51ed8001..5a5fe350f3a06c5d3519f79c30591e59434e1132 100644 (file)
--- a/app/Exports/ZipExports/Models/ZipExportChapter.php
+++ b/app/Exports/ZipExports/Models/ZipExportChapter.php
@@ -59,7 +59,7 @@ class ZipExportChapter extends ZipExportModel
     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
index 691eb918fc68c346f51333c9b8c2d133b780f41b..89083b15be116bbe1e97e2833559131265177cc6 100644 (file)
--- a/app/Exports/ZipExports/Models/ZipExportImage.php
+++ b/app/Exports/ZipExports/Models/ZipExportImage.php
@@ -33,7 +33,7 @@ class ZipExportImage extends ZipExportModel
     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
index 3a876e7aaff22a57ec47d5282b2b978877c40664..16e7e925539eb90678a60a260c9f9b84c621d7e4 100644 (file)
--- a/app/Exports/ZipExports/Models/ZipExportPage.php
+++ b/app/Exports/ZipExports/Models/ZipExportPage.php
@@ -68,7 +68,7 @@ class ZipExportPage extends ZipExportModel
     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
index bcd3c39acf054d28a6e24edad5c8e830e6887507..7d6c829cf037c78242bc79725c1ead3f82f7d054 100644 (file)
--- a/app/Exports/ZipExports/ZipFileReferenceRule.php
+++ b/app/Exports/ZipExports/ZipFileReferenceRule.php
@@ -4,7 +4,6 @@ namespace BookStack\Exports\ZipExports;
 
 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
new file mode 100644 (file)
index 0000000..ea2b253
--- /dev/null
+++ 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
index 55c86b03b5ba1ee982e34f37bb7410922ecff40a..7659c228bcdedca5d780b5ee903c33bee9652ece 100644 (file)
--- 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 @@ class ZipValidationHelper
         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
index bc01ac47b948f53cdbe140c1fde43f3ca056498d..fdfc3d9a9b9ec9f12888bbac50b45a3db7debc26 100644 (file)
--- a/lang/en/validation.php
+++ b/lang/en/validation.php
@@ -107,6 +107,7 @@ return [
 
     '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
new file mode 100644 (file)
index 0000000..4cacea9
--- /dev/null
+++ 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']);
+    }
+}