Adapt element permission service (#153)

* Adapt element permission service * fix: PHP STAN * fix: improve error handling * Apply php-cs-fixer changes * set admin boolean permissions * exclude inspection --------- Co-authored-by: lukmzig <[email protected]>
pimcore · May 7, 2024 · 4564bea · 4564bea
1 parent d7a41f3
commit 4564bea
Show file tree

Hide file tree

Showing 15 changed files with 114 additions and 54 deletions.
diff --git a/qodana.yaml b/qodana.yaml
@@ -42,6 +42,11 @@ exclude:
   - name: PhpMethodNamingConventionInspection
     paths:
         - src/Migrations
+  - name: PhpDqlBuilderUnknownModelInspection
+    paths:
+      - src/Repository/IndexQueueRepository.php
+      - src/Service/SearchIndex/IndexService/ElementTypeAdapter/AssetTypeAdapter.php
+      - src/Service/SearchIndex/IndexService/ElementTypeAdapter/DataObjectTypeAdapter.php
 include:
   - name: PhpTaintFunctionInspection
   - name: PhpVulnerablePathsInspection

diff --git a/src/Model/Search/Asset/SearchResult/SearchResultItem/Image.php b/src/Model/Search/Asset/SearchResult/SearchResultItem/Image.php
@@ -20,13 +20,13 @@
 
 class Image extends AssetSearchResultItem
 {
-    private string $thumbnail;
+    private ?string $thumbnail;
 
     private int $width;
 
     private int $height;
 
-    public function getThumbnail(): string
+    public function getThumbnail(): ?string
     {
         return $this->thumbnail;
     }

diff --git a/src/Service/Permission/ElementPermissionService.php b/src/Service/Permission/ElementPermissionService.php
@@ -16,29 +16,34 @@
 
 namespace Pimcore\Bundle\GenericDataIndexBundle\Service\Permission;
 
-use Exception;
+use Pimcore\Bundle\GenericDataIndexBundle\Exception\AssetSearchException;
+use Pimcore\Bundle\GenericDataIndexBundle\Exception\DataObjectSearchException;
+use Pimcore\Bundle\GenericDataIndexBundle\Exception\DocumentSearchException;
 use Pimcore\Bundle\GenericDataIndexBundle\Service\Search\SearchService\Asset\AssetSearchServiceInterface;
 use Pimcore\Bundle\GenericDataIndexBundle\Service\Search\SearchService\DataObject\DataObjectSearchServiceInterface;
 use Pimcore\Bundle\GenericDataIndexBundle\Service\Search\SearchService\Document\DocumentSearchServiceInterface;
+use Pimcore\Bundle\GenericDataIndexBundle\Traits\LoggerAwareTrait;
 use Pimcore\Model\Asset;
 use Pimcore\Model\DataObject;
 use Pimcore\Model\Document;
 use Pimcore\Model\Element\ElementInterface;
 use Pimcore\Model\User;
 
-final readonly class ElementPermissionService implements ElementPermissionServiceInterface
+/**
+ * @internal
+ */
+final class ElementPermissionService implements ElementPermissionServiceInterface
 {
+    use LoggerAwareTrait;
+
     public function __construct(
-        private AssetSearchServiceInterface $assetSearchService,
-        private DataObjectSearchServiceInterface $dataObjectSearchService,
-        private DocumentSearchServiceInterface $documentSearchService,
-        private PermissionServiceInterface $permissionService
+        private readonly AssetSearchServiceInterface $assetSearchService,
+        private readonly DataObjectSearchServiceInterface $dataObjectSearchService,
+        private readonly DocumentSearchServiceInterface $documentSearchService,
+        private readonly PermissionServiceInterface $permissionService
     ) {
     }
 
-    /**
-     * @throws Exception
-     */
     public function isAllowed(
         string $permission,
         ElementInterface $element,
@@ -52,15 +57,19 @@ public function isAllowed(
         };
     }
 
-    /**
-     * @throws Exception
-     */
     private function isAssetAllowed(
         string $permission,
         Asset $asset,
         User $user
     ): bool {
-        $assetResult = $this->assetSearchService->byId($asset->getId(), $user);
+        try {
+            $assetResult = $this->assetSearchService->byId($asset->getId(), $user);
+        } catch (AssetSearchException $e) {
+            $this->logger->error('Asset search failed in the element permission check: ' . $e->getMessage());
+
+            return false;
+        }
+
         if (!$assetResult) {
             return false;
         }
@@ -73,15 +82,21 @@ private function isAssetAllowed(
         return $this->permissionService->getPermissionValue($assetPermissions, $permission);
     }
 
-    /**
-     * @throws Exception
-     */
     private function isDataObjectAllowed(
         DataObject $dataObject,
         string $permission,
         User $user
     ): bool {
-        $dataObjectResult = $this->dataObjectSearchService->byId($dataObject->getId(), $user);
+        try {
+            $dataObjectResult = $this->dataObjectSearchService->byId($dataObject->getId(), $user);
+        } catch (DataObjectSearchException $e) {
+            $this->logger->error(
+                'Data Object search failed in the element permission check: ' . $e->getMessage()
+            );
+
+            return false;
+        }
+
         if (!$dataObjectResult) {
             return false;
         }
@@ -94,15 +109,21 @@ private function isDataObjectAllowed(
         return $this->permissionService->getPermissionValue($permissions, $permission);
     }
 
-    /**
-     * @throws Exception
-     */
     private function isDocumentAllowed(
         Document $document,
         string $permission,
         User $user
     ): bool {
-        $documentResult = $this->documentSearchService->byId($document->getId(), $user);
+        try {
+            $documentResult = $this->documentSearchService->byId($document->getId(), $user);
+        } catch (DocumentSearchException $e) {
+            $this->logger->error(
+                'Document search failed in the element permission check: ' . $e->getMessage()
+            );
+
+            return false;
+        }
+
         if (!$documentResult) {
             return false;
         }

diff --git a/src/Service/Permission/ElementPermissionServiceInterface.php b/src/Service/Permission/ElementPermissionServiceInterface.php
@@ -16,18 +16,11 @@
 
 namespace Pimcore\Bundle\GenericDataIndexBundle\Service\Permission;
 
-use Exception;
 use Pimcore\Model\Element\ElementInterface;
 use Pimcore\Model\User;
 
-/**
- * @internal
- */
 interface ElementPermissionServiceInterface
 {
-    /**
-     * @throws Exception
-     */
     public function isAllowed(
         string $permission,
         ElementInterface $element,

diff --git a/src/Service/Permission/PermissionService.php b/src/Service/Permission/PermissionService.php
@@ -150,8 +150,10 @@ private function getAdminUserPermissions(
 
         $properties = $permissions->getClassProperties();
         foreach ($properties as $property => $value) {
-            $setter = 'set' . ucfirst($property);
-            $permissions->$setter(true);
+            if (is_bool($value)) {
+                $setter = 'set' . ucfirst($property);
+                $permissions->$setter(true);
+            }
         }
 
         return $permissions;

diff --git a/src/Service/Search/SearchService/Asset/AssetSearchService.php b/src/Service/Search/SearchService/Asset/AssetSearchService.php
@@ -45,7 +45,7 @@ public function __construct(
     }
 
     /**
-     * @throws Exception
+     * @throws AssetSearchException
      */
     public function search(SearchInterface $assetSearch): AssetSearchResult
     {
@@ -86,7 +86,7 @@ public function search(SearchInterface $assetSearch): AssetSearchResult
     }
 
     /**
-     * @throws Exception
+     * @throws AssetSearchException
      */
     public function byId(
         int $id,
@@ -112,7 +112,7 @@ public function byId(
     }
 
     /**
-     * @throws Exception
+     * @throws AssetSearchException
      */
     private function searchAssetById(int $id, ?User $user = null): ?AssetSearchResultItem
     {

diff --git a/src/Service/Search/SearchService/Asset/AssetSearchServiceInterface.php b/src/Service/Search/SearchService/Asset/AssetSearchServiceInterface.php
@@ -16,7 +16,7 @@
 
 namespace Pimcore\Bundle\GenericDataIndexBundle\Service\Search\SearchService\Asset;
 
-use Exception;
+use Pimcore\Bundle\GenericDataIndexBundle\Exception\AssetSearchException;
 use Pimcore\Bundle\GenericDataIndexBundle\Model\Search\Asset\SearchResult\AssetSearchResult;
 use Pimcore\Bundle\GenericDataIndexBundle\Model\Search\Asset\SearchResult\AssetSearchResultItem;
 use Pimcore\Bundle\GenericDataIndexBundle\Model\Search\Interfaces\SearchInterface;
@@ -25,12 +25,12 @@
 interface AssetSearchServiceInterface
 {
     /**
-     * @throws Exception
+     * @throws AssetSearchException
      */
     public function search(SearchInterface $assetSearch): AssetSearchResult;
 
     /**
-     * @throws Exception
+     * @throws AssetSearchException
      */
     public function byId(int $id, ?User $user = null): ?AssetSearchResultItem;
 }
diff --git a/src/Service/Search/SearchService/DataObject/DataObjectSearchService.php b/src/Service/Search/SearchService/DataObject/DataObjectSearchService.php
@@ -89,7 +89,7 @@ public function search(DataObjectSearchInterface $dataObjectSearch): DataObjectS
     }
 
     /**
-     * @throws Exception
+     * @throws DataObjectSearchException
      */
     public function byId(
         int $id,
@@ -115,7 +115,7 @@ public function byId(
     }
 
     /**
-     * @throws Exception
+     * @throws DataObjectSearchException
      */
     private function searchObjectById(int $id, ?User $user = null): ?DataObjectSearchResultItem
     {

diff --git a/src/Service/Search/SearchService/DataObject/DataObjectSearchServiceInterface.php b/src/Service/Search/SearchService/DataObject/DataObjectSearchServiceInterface.php
@@ -16,7 +16,6 @@
 
 namespace Pimcore\Bundle\GenericDataIndexBundle\Service\Search\SearchService\DataObject;
 
-use Exception;
 use Pimcore\Bundle\GenericDataIndexBundle\Exception\DataObjectSearchException;
 use Pimcore\Bundle\GenericDataIndexBundle\Model\Search\DataObject\DataObjectSearchInterface;
 use Pimcore\Bundle\GenericDataIndexBundle\Model\Search\DataObject\SearchResult\DataObjectSearchResult;
@@ -31,7 +30,7 @@ interface DataObjectSearchServiceInterface
     public function search(DataObjectSearchInterface $dataObjectSearch): DataObjectSearchResult;
 
     /**
-     * @throws Exception
+     * @throws DataObjectSearchException
      */
     public function byId(int $id, ?User $user = null): ?DataObjectSearchResultItem;
 }
diff --git a/src/Service/Search/SearchService/Document/DocumentSearchService.php b/src/Service/Search/SearchService/Document/DocumentSearchService.php
@@ -86,7 +86,7 @@ public function search(SearchInterface $documentSearch): DocumentSearchResult
     }
 
     /**
-     * @throws Exception
+     * @throws DocumentSearchException
      */
     public function byId(
         int $id,
@@ -112,7 +112,7 @@ public function byId(
     }
 
     /**
-     * @throws Exception
+     * @throws DocumentSearchException
      */
     private function searchDocumentById(int $id, ?User $user = null): ?DocumentSearchResultItem
     {

diff --git a/src/Service/Search/SearchService/Document/DocumentSearchServiceInterface.php b/src/Service/Search/SearchService/Document/DocumentSearchServiceInterface.php
@@ -16,7 +16,7 @@
 
 namespace Pimcore\Bundle\GenericDataIndexBundle\Service\Search\SearchService\Document;
 
-use Exception;
+use Pimcore\Bundle\GenericDataIndexBundle\Exception\DocumentSearchException;
 use Pimcore\Bundle\GenericDataIndexBundle\Model\Search\Document\SearchResult\DocumentSearchResult;
 use Pimcore\Bundle\GenericDataIndexBundle\Model\Search\Document\SearchResult\DocumentSearchResultItem;
 use Pimcore\Bundle\GenericDataIndexBundle\Model\Search\Interfaces\SearchInterface;
@@ -25,12 +25,12 @@
 interface DocumentSearchServiceInterface
 {
     /**
-     * @throws Exception
+     * @throws DocumentSearchException
      */
     public function search(SearchInterface $documentSearch): DocumentSearchResult;
 
     /**
-     * @throws Exception
+     * @throws DocumentSearchException
      */
     public function byId(int $id, ?User $user = null): ?DocumentSearchResultItem;
 }
diff --git a/src/Service/SearchIndex/IndexQueueService.php b/src/Service/SearchIndex/IndexQueueService.php
@@ -72,7 +72,7 @@ public function updateIndexQueue(
 
             $this->pathService->rewriteChildrenIndexPaths($element);
         } catch (Exception $e) {
-            $this->logger->warning(
+            $this->logger->error(
                 sprintf(
                     'Update indexQueue in database-table %s failed! Error: %s',
                     IndexQueue::TABLE,

diff --git a/src/Service/Serializer/AssetTypeSerializationHandler/DocumentSerializationHandler.php b/src/Service/Serializer/AssetTypeSerializationHandler/DocumentSerializationHandler.php
@@ -20,12 +20,15 @@
 use Pimcore\Bundle\GenericDataIndexBundle\Enum\SearchIndex\FieldCategory\SystemField\Asset\DocumentSystemField;
 use Pimcore\Bundle\GenericDataIndexBundle\Model\Search\Asset\SearchResult\AssetSearchResultItem;
 use Pimcore\Bundle\GenericDataIndexBundle\Model\Search\Asset\SearchResult\SearchResultItem;
+use Pimcore\Bundle\GenericDataIndexBundle\Traits\LoggerAwareTrait;
 use Pimcore\Model\Asset;
 use Pimcore\Model\Asset\Document;
 use Pimcore\Model\Asset\Image;
 
 class DocumentSerializationHandler extends AbstractHandler
 {
+    use LoggerAwareTrait;
+
     /**
      * @throws Exception
      */
@@ -49,8 +52,18 @@ public function createSearchResultModel(array $indexData): AssetSearchResultItem
             ->setPageCount(DocumentSystemField::PAGE_COUNT->getData($indexData));
     }
 
-    private function getImageThumbnail(Document $document): string
+    private function getImageThumbnail(Document $document): ?string
     {
-        return $document->getImageThumbnail(Image\Thumbnail\Config::getPreviewConfig())->getPath();
+        try {
+            return $document->getImageThumbnail(Image\Thumbnail\Config::getPreviewConfig())->getPath();
+        } catch (Exception $e) {
+            $this->logger->error('Thumbnail generation failed for document asset: ' .
+                $document->getId() .
+                ' error ' .
+                $e->getMessage()
+            );
+        }
+
+        return null;
     }
 }
diff --git a/src/Service/Serializer/AssetTypeSerializationHandler/ImageSerializationHandler.php b/src/Service/Serializer/AssetTypeSerializationHandler/ImageSerializationHandler.php
@@ -16,14 +16,18 @@
 
 namespace Pimcore\Bundle\GenericDataIndexBundle\Service\Serializer\AssetTypeSerializationHandler;
 
+use Exception;
 use Pimcore\Bundle\GenericDataIndexBundle\Enum\SearchIndex\FieldCategory\SystemField\Asset\ImageSystemField;
 use Pimcore\Bundle\GenericDataIndexBundle\Model\Search\Asset\SearchResult\AssetSearchResultItem;
 use Pimcore\Bundle\GenericDataIndexBundle\Model\Search\Asset\SearchResult\SearchResultItem;
+use Pimcore\Bundle\GenericDataIndexBundle\Traits\LoggerAwareTrait;
 use Pimcore\Model\Asset;
 use Pimcore\Model\Asset\Image;
 
 class ImageSerializationHandler extends AbstractHandler
 {
+    use LoggerAwareTrait;
+
     public function getAdditionalSystemFields(Asset $asset): array
     {
         if(!$asset instanceof Image) {
@@ -45,8 +49,18 @@ public function createSearchResultModel(array $indexData): AssetSearchResultItem
             ->setHeight(ImageSystemField::HEIGHT->getData($indexData));
     }
 
-    private function getThumbnail(Image $image): string
+    private function getThumbnail(Image $image): ?string
     {
-        return $image->getThumbnail(Image\Thumbnail\Config::getPreviewConfig())->getPath();
+        try {
+            return $image->getThumbnail(Image\Thumbnail\Config::getPreviewConfig())->getPath();
+        } catch (Exception $e) {
+            $this->logger->error('Thumbnail generation failed for image asset: ' .
+                $image->getId() .
+                ' error ' .
+                $e->getMessage()
+            );
+        }
+
+        return null;
     }
 }