IBX-7316: Applied review remarks

ezsystems · Jan 12, 2024 · 92e9f53 · 92e9f53
1 parent e48f92b
commit 92e9f53
Show file tree

Hide file tree

Showing 6 changed files with 57 additions and 34 deletions.
diff --git a/eZ/Publish/Core/FieldType/BinaryBase/BinaryBaseStorage.php b/eZ/Publish/Core/FieldType/BinaryBase/BinaryBaseStorage.php
@@ -7,8 +7,8 @@
 namespace eZ\Publish\Core\FieldType\BinaryBase;
 
 use eZ\Publish\Core\Base\Exceptions\ContentFieldValidationException;
+use eZ\Publish\Core\FieldType\Validator\FileExtensionBlackListValidator;
 use eZ\Publish\Core\IO\IOServiceInterface;
-use eZ\Publish\Core\MVC\ConfigResolverInterface;
 use eZ\Publish\SPI\FieldType\BinaryBase\PathGenerator;
 use eZ\Publish\SPI\FieldType\BinaryBase\RouteAwarePathGenerator;
 use eZ\Publish\SPI\FieldType\GatewayBasedStorage;
@@ -41,21 +41,21 @@ class BinaryBaseStorage extends GatewayBasedStorage
     /** @var \eZ\Publish\Core\FieldType\BinaryBase\BinaryBaseStorage\Gateway */
     protected $gateway;
 
-    /** @var \eZ\Publish\Core\MVC\ConfigResolverInterface */
-    protected $configResolver;
+    /** @var \eZ\Publish\Core\FieldType\Validator\FileExtensionBlackListValidator */
+    protected $fileExtensionBlackListValidator;
 
     public function __construct(
         StorageGateway $gateway,
         IOServiceInterface $ioService,
         PathGenerator $pathGenerator,
         MimeTypeDetector $mimeTypeDetector,
-        ConfigResolverInterface $configResolver
+        FileExtensionBlackListValidator $fileExtensionBlackListValidator
     ) {
         parent::__construct($gateway);
         $this->ioService = $ioService;
         $this->pathGenerator = $pathGenerator;
         $this->mimeTypeDetector = $mimeTypeDetector;
-        $this->configResolver = $configResolver;
+        $this->fileExtensionBlackListValidator = $fileExtensionBlackListValidator;
     }
 
     /**
@@ -66,6 +66,11 @@ public function setDownloadUrlGenerator(PathGenerator $downloadUrlGenerator)
         $this->downloadUrlGenerator = $downloadUrlGenerator;
     }
 
+    /**
+     * @throws \eZ\Publish\Core\Base\Exceptions\InvalidArgumentException
+     * @throws \eZ\Publish\Core\Base\Exceptions\InvalidArgumentValue
+     * @throws \eZ\Publish\Core\Base\Exceptions\ContentFieldValidationException
+     */
     public function storeFieldData(VersionInfo $versionInfo, Field $field, array $context)
     {
         if ($field->value->externalData === null) {
@@ -75,16 +80,15 @@ public function storeFieldData(VersionInfo $versionInfo, Field $field, array $co
         }
 
         if (isset($field->value->externalData['inputUri'])) {
-            $extensionsBlackList = $this->configResolver->getParameter('io.file_storage.file_type_blacklist');
-            if (
-                in_array(
-                    strtolower(pathinfo($field->value->externalData['fileName'], PATHINFO_EXTENSION)),
-                    $extensionsBlackList,
-                    true
-                )
-            ) {
-                //TODO decide whether we want to show exception or log this
-                throw new ContentFieldValidationException([]);
+            $this->fileExtensionBlackListValidator->validateFileExtension($field->value->externalData['fileName']);
+            if (!empty($errors = $this->fileExtensionBlackListValidator->getErrors())) {
+                $preparedErrors = [];
+                $preparedErrors[$field->fieldDefinitionId][$field->languageCode] = $errors;
+
+                throw ContentFieldValidationException::createNewWithMultiline(
+                    $preparedErrors,
+                    $versionInfo->contentInfo->name
+                );
             }
 
             $field->value->externalData['mimeType'] = $this->mimeTypeDetector->getFromPath($field->value->externalData['inputUri']);

diff --git a/eZ/Publish/Core/FieldType/Validator/FileExtensionBlackListValidator.php b/eZ/Publish/Core/FieldType/Validator/FileExtensionBlackListValidator.php
@@ -46,10 +46,21 @@ public function validateConstraints($constraints)
      * {@inheritdoc}
      */
     public function validate(BaseValue $value)
+    {
+        $this->validateFileExtension($value->fileName);
+
+        return empty($this->errors);
+    }
+
+    public function validateFileExtension(string $fileName): void
     {
         if (
-            pathinfo($value->fileName, PATHINFO_BASENAME) !== $value->fileName ||
-            in_array(strtolower(pathinfo($value->fileName, PATHINFO_EXTENSION)), $this->constraints['extensionsBlackList'], true)
+            pathinfo($fileName, PATHINFO_BASENAME) !== $fileName
+            || in_array(
+                strtolower(pathinfo($fileName, PATHINFO_EXTENSION)),
+                $this->constraints['extensionsBlackList'],
+                true
+            )
         ) {
             $this->errors[] = new ValidationError(
                 'A valid file is required. Following file extensions are on the blacklist: %extensionsBlackList%',
@@ -59,10 +70,14 @@ public function validate(BaseValue $value)
                 ],
                 'fileExtensionBlackList'
             );
-
-            return false;
         }
+    }
 
-        return true;
+    /**
+     * @return array<\eZ\Publish\SPI\FieldType\ValidationError>
+     */
+    public function getErrors(): array
+    {
+        return $this->errors;
     }
 }
diff --git a/eZ/Publish/Core/settings/fieldtype_external_storages.yml b/eZ/Publish/Core/settings/fieldtype_external_storages.yml
@@ -6,7 +6,7 @@ services:
             $ioService: '@ezpublish.fieldType.ezbinaryfile.io_service'
             $pathGenerator: '@ezpublish.fieldType.ezbinaryfile.pathGenerator'
             $mimeTypeDetector: '@ezpublish.core.io.mimeTypeDetector'
-            $configResolver: '@ezpublish.config.resolver'
+            $fileExtensionBlackListValidator: '@ezpublish.fieldType.validator.black_list'
         tags:
             - {name: ezplatform.field_type.external_storage_handler, alias: ezbinaryfile}
 
@@ -36,7 +36,7 @@ services:
             $ioService: '@ezpublish.fieldType.ezbinaryfile.io_service'
             $pathGenerator: '@ezpublish.fieldType.ezbinaryfile.pathGenerator'
             $mimeTypeDetector: '@ezpublish.core.io.mimeTypeDetector'
-            $configResolver: '@ezpublish.config.resolver'
+            $fileExtensionBlackListValidator: '@ezpublish.fieldType.validator.black_list'
         tags:
             - {name: ezplatform.field_type.external_storage_handler, alias: ezmedia}
 

diff --git a/eZ/Publish/SPI/Tests/FieldType/BinaryFileIntegrationTest.php b/eZ/Publish/SPI/Tests/FieldType/BinaryFileIntegrationTest.php
@@ -7,6 +7,7 @@
 namespace eZ\Publish\SPI\Tests\FieldType;
 
 use eZ\Publish\Core\FieldType;
+use eZ\Publish\Core\FieldType\Validator\FileExtensionBlackListValidator;
 use eZ\Publish\Core\IO\MimeTypeDetector\FileInfo;
 use eZ\Publish\Core\Persistence\Legacy;
 use eZ\Publish\SPI\Persistence\Content;
@@ -38,6 +39,10 @@
  */
 class BinaryFileIntegrationTest extends FileBaseIntegrationTest
 {
+
+    /** @var \eZ\Publish\Core\FieldType\Validator\FileExtensionBlackListValidator&\PHPUnit\Framework\MockObject\MockObject */
+    private $fileExtensionBlackListValidator;
+
     /**
      * Returns the storage identifier prefix used by the file service.
      *
@@ -71,6 +76,7 @@ public function getCustomHandler()
         $fieldType->setTransformationProcessor($this->getTransformationProcessor());
 
         $this->ioService = self::$container->get('ezpublish.fieldType.ezbinaryfile.io_service');
+        $this->fileExtensionBlackListValidator = $this->createMock(FileExtensionBlackListValidator::class);
 
         return $this->getHandler(
             'ezbinaryfile',
@@ -80,7 +86,8 @@ public function getCustomHandler()
                 new FieldType\BinaryFile\BinaryFileStorage\Gateway\DoctrineStorage($this->getDatabaseConnection()),
                 $this->ioService,
                 new FieldType\BinaryBase\PathGenerator\LegacyPathGenerator(),
-                new FileInfo()
+                new FileInfo(),
+                $this->fileExtensionBlackListValidator
             )
         );
     }

diff --git a/eZ/Publish/SPI/Tests/FieldType/MediaIntegrationTest.php b/eZ/Publish/SPI/Tests/FieldType/MediaIntegrationTest.php
@@ -7,6 +7,7 @@
 namespace eZ\Publish\SPI\Tests\FieldType;
 
 use eZ\Publish\Core\FieldType;
+use eZ\Publish\Core\FieldType\Validator\FileExtensionBlackListValidator;
 use eZ\Publish\Core\IO\MimeTypeDetector\FileInfo;
 use eZ\Publish\Core\Persistence\Legacy;
 use eZ\Publish\SPI\Persistence\Content;
@@ -38,6 +39,9 @@
  */
 class MediaIntegrationTest extends FileBaseIntegrationTest
 {
+    /** @var \eZ\Publish\Core\FieldType\Validator\FileExtensionBlackListValidator&\PHPUnit\Framework\MockObject\MockObject */
+    private $fileExtensionBlackListValidator;
+
     /**
      * Returns the storage identifier prefix used by the file service.
      *
@@ -70,6 +74,8 @@ public function getCustomHandler()
         ]);
         $fieldType->setTransformationProcessor($this->getTransformationProcessor());
 
+        $this->fileExtensionBlackListValidator = $this->createMock(FileExtensionBlackListValidator::class);
+
         return $this->getHandler(
             'ezmedia',
             $fieldType,
@@ -78,7 +84,8 @@ public function getCustomHandler()
                 new FieldType\Media\MediaStorage\Gateway\DoctrineStorage($this->getDatabaseConnection()),
                 $this->ioService = self::$container->get('ezpublish.fieldType.ezbinaryfile.io_service'),
                 $legacyPathGenerator = new FieldType\BinaryBase\PathGenerator\LegacyPathGenerator(),
-                new FileInfo()
+                new FileInfo(),
+                $this->fileExtensionBlackListValidator
             )
         );
     }

diff --git a/phpstan-baseline.neon b/phpstan-baseline.neon
@@ -280,11 +280,6 @@ parameters:
 			count: 3
 			path: eZ/Publish/API/Repository/Tests/SearchServiceTranslationLanguageFallbackTest.php
 
-		-
-			message: "#^Undefined variable\\: \\$loader$#"
-			count: 3
-			path: eZ/Publish/API/Repository/Tests/SetupFactory/Legacy.php
-
 		-
 			message: "#^Method eZ\\\\Publish\\\\API\\\\Repository\\\\Tests\\\\URLWildcardServiceAuthorizationTest\\:\\:testCreateThrowsUnauthorizedException\\(\\) should return eZ\\\\Publish\\\\API\\\\Repository\\\\Values\\\\Content\\\\URLWildcard but return statement is missing\\.$#"
 			count: 1
@@ -505,11 +500,6 @@ parameters:
 			count: 1
 			path: eZ/Publish/Core/Persistence/Legacy/Content/Type/Handler.php
 
-		-
-			message: "#^Undefined variable\\: \\$loader$#"
-			count: 2
-			path: eZ/Publish/Core/Persistence/Legacy/Tests/HandlerTest.php
-
 		-
 			message: "#^Class eZ\\\\Publish\\\\SPI\\\\Persistence\\\\Content\\\\UrlAlias referenced with incorrect case\\: eZ\\\\Publish\\\\SPI\\\\Persistence\\\\Content\\\\URLAlias\\.$#"
 			count: 2