From e2f6e50df4347579e3b6eb8e7c48bfcb79199a64 Mon Sep 17 00:00:00 2001 From: Dan Brown Date: Mon, 18 Nov 2024 15:53:21 +0000 Subject: [PATCH] ZIP Exports: Added ID checks and testing to validator --- .../ZipExports/Models/ZipExportAttachment.php | 2 +- .../ZipExports/Models/ZipExportBook.php | 2 +- .../ZipExports/Models/ZipExportChapter.php | 2 +- .../ZipExports/Models/ZipExportImage.php | 2 +- .../ZipExports/Models/ZipExportPage.php | 2 +- .../ZipExports/ZipFileReferenceRule.php | 1 - app/Exports/ZipExports/ZipUniqueIdRule.php | 26 +++++++ .../ZipExports/ZipValidationHelper.php | 24 ++++++ lang/en/validation.php | 1 + tests/Exports/ZipExportValidatorTests.php | 74 +++++++++++++++++++ 10 files changed, 130 insertions(+), 6 deletions(-) create mode 100644 app/Exports/ZipExports/ZipUniqueIdRule.php create mode 100644 tests/Exports/ZipExportValidatorTests.php diff --git a/app/Exports/ZipExports/Models/ZipExportAttachment.php b/app/Exports/ZipExports/Models/ZipExportAttachment.php index c6615e1dc..4f5b2f236 100644 --- 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 0dc4e93d4..47ab8f0a6 100644 --- 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 50440d61a..5a5fe350f 100644 --- 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 691eb918f..89083b15b 100644 --- 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 3a876e7aa..16e7e9255 100644 --- 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 bcd3c39ac..7d6c829cf 100644 --- 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 index 000000000..ea2b25392 --- /dev/null +++ b/app/Exports/ZipExports/ZipUniqueIdRule.php @@ -0,0 +1,26 @@ +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 55c86b03b..7659c228b 100644 --- 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 ":". Example: "book:2") + * which we can use to check uniqueness. + * @var array + */ + 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 bc01ac47b..fdfc3d9a9 100644 --- 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 index 000000000..4cacea95e --- /dev/null +++ b/tests/Exports/ZipExportValidatorTests.php @@ -0,0 +1,74 @@ +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']); + } +}