From a9c9def7282ecb3cbe44f3d42a1490c03425750b Mon Sep 17 00:00:00 2001 From: Tomasz Kryszan Date: Wed, 10 Jan 2024 11:57:31 +0100 Subject: [PATCH 1/7] IBX-7502: Added file size validation for image asset field type https://issues.ibexa.co/browse/IBX-7502 --- src/lib/FieldType/ImageAsset/Type.php | 52 +++++++++++++++++++++++---- 1 file changed, 46 insertions(+), 6 deletions(-) diff --git a/src/lib/FieldType/ImageAsset/Type.php b/src/lib/FieldType/ImageAsset/Type.php index 461e4a7192..46be6d82df 100644 --- a/src/lib/FieldType/ImageAsset/Type.php +++ b/src/lib/FieldType/ImageAsset/Type.php @@ -13,6 +13,7 @@ use Ibexa\Contracts\Core\Repository\ContentService; use Ibexa\Contracts\Core\Repository\ContentTypeService; use Ibexa\Contracts\Core\Repository\Exceptions\NotFoundException; +use Ibexa\Contracts\Core\Repository\Values\Content\Content; use Ibexa\Contracts\Core\Repository\Values\Content\ContentInfo; use Ibexa\Contracts\Core\Repository\Values\Content\Relation; use Ibexa\Contracts\Core\Repository\Values\ContentType\FieldDefinition; @@ -64,6 +65,10 @@ public function __construct( * @param \Ibexa\Core\FieldType\ImageAsset\Value $fieldValue The field value for which an action is performed * * @return \Ibexa\Contracts\Core\FieldType\ValidationError[] + * + * @throws \Ibexa\Contracts\Core\Repository\Exceptions\InvalidArgumentException + * @throws \Ibexa\Contracts\Core\Repository\Exceptions\NotFoundException + * @throws \Ibexa\Contracts\Core\Repository\Exceptions\UnauthorizedException */ public function validate(FieldDefinition $fieldDefinition, SPIValue $fieldValue): array { @@ -77,16 +82,17 @@ public function validate(FieldDefinition $fieldDefinition, SPIValue $fieldValue) (int)$fieldValue->destinationContentId ); - if (!$this->assetMapper->isAsset($content)) { - $currentContentType = $this->contentTypeService->loadContentType( - (int)$content->contentInfo->contentTypeId - ); - + if ($this->assetMapper->isAsset($content)) { + $validationError = $this->validateMaxFileSize($content); + if (null !== $validationError) { + $errors[] = $validationError; + } + } else { $errors[] = new ValidationError( 'Content %type% is not a valid asset target', null, [ - '%type%' => $currentContentType->identifier, + '%type%' => $content->getContentType()->identifier, ], 'destinationContentId' ); @@ -296,6 +302,40 @@ public static function getTranslationMessages(): array Message::create('ezimageasset.name', 'ibexa_fieldtypes')->setDesc('Image Asset'), ]; } + + /** + * @throws \Ibexa\Contracts\Core\Repository\Exceptions\NotFoundException + * @throws \Ibexa\Contracts\Core\Repository\Exceptions\InvalidArgumentException + */ + private function validateMaxFileSize(Content $content): ?ValidationError + { + $fileSize = $this->assetMapper + ->getAssetValue($content) + ->getFileSize(); + + $assetValidatorConfiguration = $this->assetMapper + ->getAssetFieldDefinition() + ->getValidatorConfiguration(); + + $maxFileSizeMB = $assetValidatorConfiguration['FileSizeValidator']['maxFileSize']; + $maxFileSizeKB = $maxFileSizeMB * 1024 * 1024; + + if ( + $maxFileSizeKB > 0 + && $fileSize > $maxFileSizeKB + ) { + return new ValidationError( + 'The file size cannot exceed %size% megabyte.', + 'The file size cannot exceed %size% megabytes.', + [ + '%size%' => $maxFileSizeMB, + ], + 'fileSize' + ); + } + + return null; + } } class_alias(Type::class, 'eZ\Publish\Core\FieldType\ImageAsset\Type'); From 1c98ce474ef6cfe7ead864b1c05521ebf0b103fc Mon Sep 17 00:00:00 2001 From: Tomasz Kryszan Date: Wed, 10 Jan 2024 11:58:51 +0100 Subject: [PATCH 2/7] [Tests] Adjusted unit tests --- tests/lib/FieldType/ImageAssetTest.php | 99 +++++++++++++++++++------- 1 file changed, 72 insertions(+), 27 deletions(-) diff --git a/tests/lib/FieldType/ImageAssetTest.php b/tests/lib/FieldType/ImageAssetTest.php index e7e1ba5218..02cc4b752e 100644 --- a/tests/lib/FieldType/ImageAssetTest.php +++ b/tests/lib/FieldType/ImageAssetTest.php @@ -19,6 +19,7 @@ use Ibexa\Contracts\Core\Repository\Values\ContentType\ContentType; use Ibexa\Contracts\Core\Repository\Values\ContentType\FieldDefinition; use Ibexa\Core\Base\Exceptions\InvalidArgumentException; +use Ibexa\Core\FieldType\Image\Value; use Ibexa\Core\FieldType\ImageAsset; use Ibexa\Core\FieldType\ValidationError; @@ -241,23 +242,17 @@ public function testValidateNonAsset() { $destinationContentId = 7; $destinationContent = $this->createMock(Content::class); - $invalidContentTypeId = 789; $invalidContentTypeIdentifier = 'article'; $invalidContentType = $this->createMock(ContentType::class); - - $destinationContentInfo = $this->createMock(ContentInfo::class); - - $destinationContentInfo + $invalidContentType ->expects($this->once()) ->method('__get') - ->with('contentTypeId') - ->willReturn($invalidContentTypeId); + ->with('identifier') + ->willReturn($invalidContentTypeIdentifier); $destinationContent - ->expects($this->once()) - ->method('__get') - ->with('contentInfo') - ->willReturn($destinationContentInfo); + ->method('getContentType') + ->willReturn($invalidContentType); $this->contentServiceMock ->expects($this->once()) @@ -271,18 +266,6 @@ public function testValidateNonAsset() ->with($destinationContent) ->willReturn(false); - $this->contentTypeServiceMock - ->expects($this->once()) - ->method('loadContentType') - ->with($invalidContentTypeId) - ->willReturn($invalidContentType); - - $invalidContentType - ->expects($this->once()) - ->method('__get') - ->with('identifier') - ->willReturn($invalidContentTypeIdentifier); - $validationErrors = $this->doValidate([], new ImageAsset\Value($destinationContentId)); $this->assertIsArray($validationErrors); @@ -311,8 +294,15 @@ public function provideValidDataForValidate(): array ]; } - public function testValidateValidNonEmptyAssetValue() - { + /** + * @dataProvider provideDataForTestValidateValidNonEmptyAssetValue + * + * @param array<\Ibexa\Core\FieldType\ValidationError> $expectedValidationErrors + */ + public function testValidateValidNonEmptyAssetValue( + int $fileSize, + array $expectedValidationErrors + ) { $destinationContentId = 7; $destinationContent = $this->createMock(Content::class); @@ -328,10 +318,65 @@ public function testValidateValidNonEmptyAssetValue() ->with($destinationContent) ->willReturn(true); + $assetValueMock = $this->createMock(Value::class); + $assetValueMock + ->method('getFileSize') + ->willReturn($fileSize); + + $this->assetMapperMock + ->expects($this->once()) + ->method('getAssetValue') + ->with($destinationContent) + ->willReturn($assetValueMock); + + $fieldDefinitionMock = $this->createMock(FieldDefinition::class); + $fieldDefinitionMock + ->method('getValidatorConfiguration') + ->willReturn( + [ + 'FileSizeValidator' => [ + 'maxFileSize' => 1.4, + ], + ] + ); + + $this->assetMapperMock + ->method('getAssetFieldDefinition') + ->willReturn($fieldDefinitionMock); + $validationErrors = $this->doValidate([], new ImageAsset\Value($destinationContentId)); + $this->assertEquals( + $expectedValidationErrors, + $validationErrors + ); + } - $this->assertIsArray($validationErrors); - $this->assertEmpty($validationErrors, "Got value:\n" . var_export($validationErrors, true)); + /** + * @return iterable, + * }> + */ + public function provideDataForTestValidateValidNonEmptyAssetValue(): iterable + { + yield 'No validation errors' => [ + 123456, + [], + ]; + + yield 'Maximum file size exceeded' => [ + 12345678912356, + [ + new ValidationError( + 'The file size cannot exceed %size% megabyte.', + 'The file size cannot exceed %size% megabytes.', + [ + '%size%' => 1.4, + ], + 'fileSize' + ), + ], + ]; } /** From 12ddb474a4e75f0b9852079c6105c7496eb755dd Mon Sep 17 00:00:00 2001 From: Tomasz Kryszan Date: Wed, 10 Jan 2024 11:59:17 +0100 Subject: [PATCH 3/7] [PHPStan] Regenerated baseline --- phpstan-baseline.neon | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/phpstan-baseline.neon b/phpstan-baseline.neon index 293e5cf7e8..da78e4bbd3 100644 --- a/phpstan-baseline.neon +++ b/phpstan-baseline.neon @@ -9755,6 +9755,11 @@ parameters: count: 1 path: src/lib/FieldType/ImageAsset/Type.php + - + message: "#^Property Ibexa\\\\Core\\\\FieldType\\\\ImageAsset\\\\Type\\:\\:\\$contentTypeService is never read, only written\\.$#" + count: 1 + path: src/lib/FieldType/ImageAsset/Type.php + - message: "#^Access to an undefined property Ibexa\\\\Contracts\\\\Core\\\\FieldType\\\\Value\\:\\:\\$value\\.$#" count: 1 @@ -22689,6 +22694,7 @@ parameters: message: "#^Method Ibexa\\\\Core\\\\Search\\\\Legacy\\\\Content\\\\Gateway\\\\CriterionHandler\\\\Ancestor\\:\\:handle\\(\\) has parameter \\$languageSettings with no value type specified in iterable type array\\.$#" count: 1 path: src/lib/Search/Legacy/Content/Gateway/CriterionHandler/Ancestor.php + - message: "#^Method Ibexa\\\\Core\\\\Search\\\\Legacy\\\\Content\\\\Gateway\\\\CriterionHandler\\\\LocationId\\:\\:handle\\(\\) has parameter \\$languageSettings with no value type specified in iterable type array\\.$#" count: 1 From c5238c524b735090cea4afa267591cf39160b5f9 Mon Sep 17 00:00:00 2001 From: Tomasz Kryszan Date: Thu, 11 Jan 2024 08:59:46 +0100 Subject: [PATCH 4/7] Dropped contentTypeService argument --- phpstan-baseline.neon | 5 ----- src/lib/FieldType/ImageAsset/Type.php | 12 ------------ src/lib/Resources/settings/fieldtypes.yml | 1 - tests/lib/FieldType/ImageAssetTest.php | 6 ------ 4 files changed, 24 deletions(-) diff --git a/phpstan-baseline.neon b/phpstan-baseline.neon index da78e4bbd3..8a9dcda7fa 100644 --- a/phpstan-baseline.neon +++ b/phpstan-baseline.neon @@ -9755,11 +9755,6 @@ parameters: count: 1 path: src/lib/FieldType/ImageAsset/Type.php - - - message: "#^Property Ibexa\\\\Core\\\\FieldType\\\\ImageAsset\\\\Type\\:\\:\\$contentTypeService is never read, only written\\.$#" - count: 1 - path: src/lib/FieldType/ImageAsset/Type.php - - message: "#^Access to an undefined property Ibexa\\\\Contracts\\\\Core\\\\FieldType\\\\Value\\:\\:\\$value\\.$#" count: 1 diff --git a/src/lib/FieldType/ImageAsset/Type.php b/src/lib/FieldType/ImageAsset/Type.php index 46be6d82df..85c5ca1128 100644 --- a/src/lib/FieldType/ImageAsset/Type.php +++ b/src/lib/FieldType/ImageAsset/Type.php @@ -11,7 +11,6 @@ use Ibexa\Contracts\Core\FieldType\Value as SPIValue; use Ibexa\Contracts\Core\Persistence\Content\Handler as SPIContentHandler; use Ibexa\Contracts\Core\Repository\ContentService; -use Ibexa\Contracts\Core\Repository\ContentTypeService; use Ibexa\Contracts\Core\Repository\Exceptions\NotFoundException; use Ibexa\Contracts\Core\Repository\Values\Content\Content; use Ibexa\Contracts\Core\Repository\Values\Content\ContentInfo; @@ -31,29 +30,18 @@ class Type extends FieldType implements TranslationContainerInterface /** @var \Ibexa\Contracts\Core\Repository\ContentService */ private $contentService; - /** @var \Ibexa\Contracts\Core\Repository\ContentTypeService */ - private $contentTypeService; - /** @var \Ibexa\Core\FieldType\ImageAsset\AssetMapper */ private $assetMapper; /** @var \Ibexa\Contracts\Core\Persistence\Content\Handler */ private $handler; - /** - * @param \Ibexa\Contracts\Core\Repository\ContentService $contentService - * @param \Ibexa\Contracts\Core\Repository\ContentTypeService $contentTypeService - * @param \Ibexa\Core\FieldType\ImageAsset\AssetMapper $mapper - * @param \Ibexa\Contracts\Core\Persistence\Content\Handler $handler - */ public function __construct( ContentService $contentService, - ContentTypeService $contentTypeService, AssetMapper $mapper, SPIContentHandler $handler ) { $this->contentService = $contentService; - $this->contentTypeService = $contentTypeService; $this->assetMapper = $mapper; $this->handler = $handler; } diff --git a/src/lib/Resources/settings/fieldtypes.yml b/src/lib/Resources/settings/fieldtypes.yml index 6e46018552..df19e8b151 100644 --- a/src/lib/Resources/settings/fieldtypes.yml +++ b/src/lib/Resources/settings/fieldtypes.yml @@ -549,7 +549,6 @@ services: parent: Ibexa\Core\FieldType\FieldType arguments: - '@ibexa.api.service.content' - - '@ibexa.api.service.content_type' - '@Ibexa\Core\FieldType\ImageAsset\AssetMapper' - '@Ibexa\Core\Persistence\Cache\ContentHandler' tags: diff --git a/tests/lib/FieldType/ImageAssetTest.php b/tests/lib/FieldType/ImageAssetTest.php index 02cc4b752e..24ef837be2 100644 --- a/tests/lib/FieldType/ImageAssetTest.php +++ b/tests/lib/FieldType/ImageAssetTest.php @@ -12,7 +12,6 @@ use Ibexa\Contracts\Core\Persistence\Content\Handler as SPIContentHandler; use Ibexa\Contracts\Core\Persistence\Content\VersionInfo; use Ibexa\Contracts\Core\Repository\ContentService; -use Ibexa\Contracts\Core\Repository\ContentTypeService; use Ibexa\Contracts\Core\Repository\Values\Content\Content; use Ibexa\Contracts\Core\Repository\Values\Content\ContentInfo; use Ibexa\Contracts\Core\Repository\Values\Content\Relation; @@ -34,9 +33,6 @@ class ImageAssetTest extends FieldTypeTest /** @var \Ibexa\Contracts\Core\Repository\ContentService|\PHPUnit\Framework\MockObject\MockObject */ private $contentServiceMock; - /** @var \Ibexa\Contracts\Core\Repository\ContentTypeService|\PHPUnit\Framework\MockObject\MockObject */ - private $contentTypeServiceMock; - /** @var \Ibexa\Core\FieldType\ImageAsset\AssetMapper|\PHPUnit\Framework\MockObject\MockObject */ private $assetMapperMock; @@ -51,7 +47,6 @@ protected function setUp(): void parent::setUp(); $this->contentServiceMock = $this->createMock(ContentService::class); - $this->contentTypeServiceMock = $this->createMock(ContentTypeService::class); $this->assetMapperMock = $this->createMock(ImageAsset\AssetMapper::class); $this->contentHandlerMock = $this->createMock(SPIContentHandler::class); $versionInfo = new VersionInfo([ @@ -96,7 +91,6 @@ protected function createFieldTypeUnderTest() { return new ImageAsset\Type( $this->contentServiceMock, - $this->contentTypeServiceMock, $this->assetMapperMock, $this->contentHandlerMock ); From 44d9fbb5f1e12a7dada0d9b88a151a4cafa701a5 Mon Sep 17 00:00:00 2001 From: Tomasz Kryszan Date: Fri, 12 Jan 2024 13:58:10 +0100 Subject: [PATCH 5/7] Added early return from validation when content is not an asset --- src/lib/FieldType/ImageAsset/Type.php | 30 ++++++++++++++------------- 1 file changed, 16 insertions(+), 14 deletions(-) diff --git a/src/lib/FieldType/ImageAsset/Type.php b/src/lib/FieldType/ImageAsset/Type.php index 85c5ca1128..ac77946f59 100644 --- a/src/lib/FieldType/ImageAsset/Type.php +++ b/src/lib/FieldType/ImageAsset/Type.php @@ -70,20 +70,22 @@ public function validate(FieldDefinition $fieldDefinition, SPIValue $fieldValue) (int)$fieldValue->destinationContentId ); - if ($this->assetMapper->isAsset($content)) { - $validationError = $this->validateMaxFileSize($content); - if (null !== $validationError) { - $errors[] = $validationError; - } - } else { - $errors[] = new ValidationError( - 'Content %type% is not a valid asset target', - null, - [ - '%type%' => $content->getContentType()->identifier, - ], - 'destinationContentId' - ); + if (!$this->assetMapper->isAsset($content)) { + return [ + new ValidationError( + 'Content %type% is not a valid asset target', + null, + [ + '%type%' => $content->getContentType()->identifier, + ], + 'destinationContentId' + ), + ]; + } + + $validationError = $this->validateMaxFileSize($content); + if (null !== $validationError) { + $errors[] = $validationError; } return $errors; From 64737839cd9398f125280aff882bc72a481b1221 Mon Sep 17 00:00:00 2001 From: ciastektk Date: Mon, 15 Jan 2024 09:44:55 +0100 Subject: [PATCH 6/7] Update tests/lib/FieldType/ImageAssetTest.php Co-authored-by: Konrad Oboza --- tests/lib/FieldType/ImageAssetTest.php | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tests/lib/FieldType/ImageAssetTest.php b/tests/lib/FieldType/ImageAssetTest.php index 24ef837be2..fe6d5c32fe 100644 --- a/tests/lib/FieldType/ImageAssetTest.php +++ b/tests/lib/FieldType/ImageAssetTest.php @@ -296,7 +296,7 @@ public function provideValidDataForValidate(): array public function testValidateValidNonEmptyAssetValue( int $fileSize, array $expectedValidationErrors - ) { + ): void { $destinationContentId = 7; $destinationContent = $this->createMock(Content::class); From 09d7b8ded3348e1aacb207505cd0991417b9dcfa Mon Sep 17 00:00:00 2001 From: Tomasz Kryszan Date: Mon, 15 Jan 2024 09:59:32 +0100 Subject: [PATCH 7/7] [PHPStan] Regenerated baseline --- phpstan-baseline.neon | 5 ----- 1 file changed, 5 deletions(-) diff --git a/phpstan-baseline.neon b/phpstan-baseline.neon index 8a9dcda7fa..a5e92326c3 100644 --- a/phpstan-baseline.neon +++ b/phpstan-baseline.neon @@ -43585,11 +43585,6 @@ parameters: count: 1 path: tests/lib/FieldType/ImageAssetTest.php - - - message: "#^Method Ibexa\\\\Tests\\\\Core\\\\FieldType\\\\ImageAssetTest\\:\\:testValidateValidNonEmptyAssetValue\\(\\) has no return type specified\\.$#" - count: 1 - path: tests/lib/FieldType/ImageAssetTest.php - - message: "#^Access to an undefined property Ibexa\\\\Tests\\\\Core\\\\FieldType\\\\ImageTest\\:\\:\\$mimeTypeDetectorMock\\.$#" count: 1