From 4564bea1cc52182899ce5dde3bbcc2b10ea18fa2 Mon Sep 17 00:00:00 2001 From: lukmzig <30526586+lukmzig@users.noreply.github.com> Date: Tue, 7 May 2024 15:44:39 +0200 Subject: [PATCH] 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 --- qodana.yaml | 5 ++ .../SearchResult/SearchResultItem/Image.php | 4 +- .../Permission/ElementPermissionService.php | 63 ++++++++++++------- .../ElementPermissionServiceInterface.php | 7 --- src/Service/Permission/PermissionService.php | 6 +- .../Asset/AssetSearchService.php | 6 +- .../Asset/AssetSearchServiceInterface.php | 6 +- .../DataObject/DataObjectSearchService.php | 4 +- .../DataObjectSearchServiceInterface.php | 3 +- .../Document/DocumentSearchService.php | 4 +- .../DocumentSearchServiceInterface.php | 6 +- src/Service/SearchIndex/IndexQueueService.php | 2 +- .../DocumentSerializationHandler.php | 17 ++++- .../ImageSerializationHandler.php | 18 +++++- .../VideoSerializationHandler.php | 17 ++++- 15 files changed, 114 insertions(+), 54 deletions(-) diff --git a/qodana.yaml b/qodana.yaml index b51a5655..68f707e4 100644 --- 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 index 90126e29..76213a27 100644 --- 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 index 063dc5a8..d85a9439 100644 --- 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 index 32ab489e..43597152 100644 --- 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 index 0fb434ee..e0652e48 100644 --- 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 index e9002875..3c2adb94 100644 --- 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 index 35bce868..01842f77 100644 --- 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 index 83d713c1..62dc1b40 100644 --- 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 index ea6c90cc..8cb2386b 100644 --- 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 index 9bb7ab53..2632dffd 100644 --- 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 index ff81bf66..4673793d 100644 --- 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 index d52c23db..3435ece6 100644 --- 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 index 0e15f3a4..3e5095b1 100644 --- 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 index be037b93..95308fc0 100644 --- 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; } } diff --git a/src/Service/Serializer/AssetTypeSerializationHandler/VideoSerializationHandler.php b/src/Service/Serializer/AssetTypeSerializationHandler/VideoSerializationHandler.php index e11df3b5..e48fcf00 100644 --- a/src/Service/Serializer/AssetTypeSerializationHandler/VideoSerializationHandler.php +++ b/src/Service/Serializer/AssetTypeSerializationHandler/VideoSerializationHandler.php @@ -20,12 +20,15 @@ use Pimcore\Bundle\GenericDataIndexBundle\Enum\SearchIndex\FieldCategory\SystemField\Asset\VideoSystemField; 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; use Pimcore\Model\Asset\Video; class VideoSerializationHandler extends AbstractHandler { + use LoggerAwareTrait; + /** * @throws Exception */ @@ -52,8 +55,18 @@ public function createSearchResultModel(array $indexData): AssetSearchResultItem ->setHeight(VideoSystemField::HEIGHT->getData($indexData)); } - private function getImageThumbnail(Video $video): string + private function getImageThumbnail(Video $video): ?string { - return $video->getImageThumbnail(Image\Thumbnail\Config::getPreviewConfig())->getPath(); + try { + return $video->getImageThumbnail(Image\Thumbnail\Config::getPreviewConfig())->getPath(); + } catch (Exception $e) { + $this->logger->error('Thumbnail generation failed for video asset: ' . + $video->getId() . + ' error ' . + $e->getMessage() + ); + } + + return null; } }