Feature/improvments (#221)

* [TASK] change visibility of createQueryBuilder function - used only internally * [TASK] refactor functions in LogRepository where $constraints where passed by reference * [TASK] test findByFiler in LogRepository * [TASK] test findByFiler in LogRepository * [TASK] remove no group by SQL mode from default FunctionalTestCase config * [TASK] add @Covers annotation, specify which parts of the code it is supposed to test while running FunctionalTestCase * [TASK] add tests for findByFilter for UserId/PageId * [TASK] refactor and add test for logDownload function in LogRepository.php * [TASK] add tests for getFirstTimestampByFilter and getTrafficSumByFilter functions from LogRepository.php * [TASK] add TokenRefreshMiddlewareTest based on FunctionalTestCase (WIP) * [TASK] add tests when user is the same and user is different (html comes from cache), create function to build request handler * [CODE] code style fix * [BUGFIX] Resource files are now also logged #220 [TER-262] * [BUGFIX] Consider a possible file change from the previous event #215 [TER-253] * [BUGFIX] Remove unnecessary variables #217 [TER-258] * [BUGFIX] No error on Cleaning processed files anymore #216 [TER-258] * [TASK] set correct return type * [TASK] Use vendor and extension name for naming * [TASK] fix SQL-GroupBy error and use default setting in Functional Tests #222 [TER-265] --------- Co-authored-by: Peter Zar <[email protected]>
Leuchtfeuer · Nov 27, 2024 · 8012a41 · 8012a41
1 parent facdf87
commit 8012a41
Show file tree

Hide file tree

Showing 9 changed files with 495 additions and 51 deletions.
diff --git a/Classes/Controller/LogController.php b/Classes/Controller/LogController.php
@@ -110,7 +110,8 @@ private function getUsers(): array
             ->from('tx_securedownloads_domain_model_log', 'log')
             ->join('log', 'fe_users', 'users', $queryBuilder->expr()->eq('users.uid', 'log.user'))
             ->where($queryBuilder->expr()->neq('user', $queryBuilder->createNamedParameter(0, \PDO::PARAM_INT)))
-            ->groupBy('users.uid')
+            ->resetQueryPart('orderBy')
+            ->groupBy('uid', 'username')
             ->executeQuery()
             ->fetchAllAssociative();
     }
@@ -126,6 +127,7 @@ private function getFileTypes(): array
         return $queryBuilder
             ->select('media_type')
             ->from('tx_securedownloads_domain_model_log')
+            ->resetQueryPart('orderBy')
             ->groupBy('media_type')->orderBy('media_type', 'ASC')
             ->executeQuery()
             ->fetchAllAssociative();

diff --git a/Classes/Domain/Repository/LogRepository.php b/Classes/Domain/Repository/LogRepository.php
@@ -22,8 +22,6 @@
 use TYPO3\CMS\Core\Database\Query\QueryBuilder;
 use TYPO3\CMS\Core\Resource\Exception\ResourceDoesNotExistException;
 use TYPO3\CMS\Core\Resource\ResourceFactory;
-use TYPO3\CMS\Core\Utility\GeneralUtility;
-use TYPO3\CMS\Extbase\DomainObject\DomainObjectInterface;
 use TYPO3\CMS\Extbase\Persistence\Exception\InvalidQueryException;
 use TYPO3\CMS\Extbase\Persistence\Generic\Mapper\DataMapper;
 use TYPO3\CMS\Extbase\Persistence\Repository;
@@ -34,12 +32,13 @@ class LogRepository extends Repository
 
     public function __construct(
         private readonly ConnectionPool $connectionPool,
-        private readonly DataMapper $dataMapper
+        private readonly DataMapper $dataMapper,
+        private readonly ResourceFactory $resourceFactory,
     ) {
         parent::__construct();
     }
 
-    public function createQueryBuilder(): QueryBuilder
+    protected function createQueryBuilder(): QueryBuilder
     {
         $queryBuilder = $this->connectionPool->getQueryBuilderForTable(self::TABLENAME);
         $queryBuilder->getRestrictions()->removeAll();
@@ -51,32 +50,42 @@ public function createQueryBuilder(): QueryBuilder
     }
 
     /**
-     * @return DomainObjectInterface[]
+     * @return Log[]
      * @throws Exception
      */
     public function findByFilter(?Filter $filter, int $currentPage = 1, int $itemsPerPage = 20): array
     {
-        $queryBuilder = $this->createQueryBuilder();
+        if ($currentPage < 1 || $itemsPerPage < 1) {
+            return [];
+        }
 
-        $this->applyFilter($queryBuilder, $filter);
+        $queryBuilder = $this->createQueryBuilder();
 
+        $constraints = $this->getFilterConstraints($queryBuilder, $filter);
+        if ($constraints !== []) {
+            $queryBuilder->where(...$constraints);
+        }
         $result = $queryBuilder
             ->select('*')
             ->setMaxResults($itemsPerPage)
             ->setFirstResult($itemsPerPage * ($currentPage - 1))
             ->executeQuery()
-            ->fetchAllAssociative() ?? [];
+            ->fetchAllAssociative();
         return $this->dataMapper->map(Log::class, $result);
     }
 
     public function countByFilter(?Filter $filter): int
     {
         $queryBuilder = $this->createQueryBuilder();
 
-        $this->applyFilter($queryBuilder, $filter);
+        $constraints = $this->getFilterConstraints($queryBuilder, $filter);
+        if ($constraints !== []) {
+            $queryBuilder->where(...$constraints);
+        }
 
         return (int)($queryBuilder
             ->count('uid')
+            ->resetQueryPart('orderBy')
             ->executeQuery()
             ->fetchOne() ?? 0);
     }
@@ -85,7 +94,10 @@ public function getFirstTimestampByFilter(?Filter $filter, bool $reverse = false
     {
         $queryBuilder = $this->createQueryBuilder();
 
-        $this->applyFilter($queryBuilder, $filter);
+        $constraints = $this->getFilterConstraints($queryBuilder, $filter);
+        if ($constraints !== []) {
+            $queryBuilder->where(...$constraints);
+        }
 
         return (int)($queryBuilder
             ->select('tstamp')
@@ -98,87 +110,110 @@ public function getTrafficSumByFilter(?Filter $filter): float
     {
         $queryBuilder = $this->createQueryBuilder();
 
-        $this->applyFilter($queryBuilder, $filter);
+        $constraints = $this->getFilterConstraints($queryBuilder, $filter);
+        if ($constraints !== []) {
+            $queryBuilder->where(...$constraints);
+        }
 
         return (float)($queryBuilder
             ->selectLiteral('SUM(file_size) AS sum')
+            ->resetQueryPart('orderBy')
             ->executeQuery()
             ->fetchOne() ?? 0.0);
     }
 
-    protected function applyFilter(QueryBuilder &$queryBuilder, Filter $filter): void
+    /**
+     * @param QueryBuilder $queryBuilder
+     * @param Filter|null $filter
+     * @return string[]
+     */
+    protected function getFilterConstraints(QueryBuilder $queryBuilder, ?Filter $filter): array
     {
-        $constraints = [];
-
         if ($filter instanceof Filter) {
             try {
                 // FileType
-                $this->applyFileTypePropertyToFilter($filter->getFileType(), $queryBuilder, $constraints);
+                $constraints = $this->applyMediaTypePropertyToFilter($filter->getFileType(), $queryBuilder);
 
                 // User Type
-                $this->applyUserTypePropertyToFilter($filter, $queryBuilder, $constraints);
+                $constraints = array_merge($constraints, $this->applyUserTypePropertyToFilter($filter, $queryBuilder));
 
                 // Period
-                $this->applyPeriodPropertyToFilter($filter, $queryBuilder, $constraints);
+                $constraints = array_merge($constraints, $this->applyPeriodPropertyToFilter($filter, $queryBuilder));
 
                 // User and Page
-                $this->applyEqualPropertyToFilter((int)$filter->getFeUserId(), 'user', $queryBuilder, $constraints);
-                $this->applyEqualPropertyToFilter((int)$filter->getPageId(), 'page', $queryBuilder, $constraints);
+                $constraints = array_merge($constraints, $this->applyEqualPropertyToFilter($filter->getFeUserId(), 'user', $queryBuilder));
+                $constraints = array_merge($constraints, $this->applyEqualPropertyToFilter($filter->getPageId(), 'page', $queryBuilder));
 
-                if (count($constraints) > 0) {
-                    $queryBuilder->where(...$constraints);
-                }
+                return $constraints;
             } catch (InvalidQueryException) {
                 // Do nothing for now.
             }
         }
+        return [];
     }
 
     /**
-     * @param string[] $constraints
+     * @param string $mediaType
+     * @param QueryBuilder $queryBuilder
+     * @return string[]
      */
-    protected function applyFileTypePropertyToFilter(string $fileType, QueryBuilder $queryBuilder, array &$constraints): void
+    protected function applyMediaTypePropertyToFilter(string $mediaType, QueryBuilder $queryBuilder): array
     {
-        if ($fileType !== '' && $fileType !== '0') {
-            $constraints[] = $queryBuilder->expr()->eq('media_type', $queryBuilder->createNamedParameter($fileType));
+        if ($mediaType !== '' && $mediaType !== '0') {
+            return [$queryBuilder->expr()->eq('media_type', $queryBuilder->createNamedParameter($mediaType))];
         }
+        return [];
     }
 
     /**
-     * @param string[] $constraints
+     * @param Filter $filter
+     * @param QueryBuilder $queryBuilder
+     * @return string[]
      */
-    protected function applyUserTypePropertyToFilter(Filter $filter, QueryBuilder $queryBuilder, array &$constraints): void
+    protected function applyUserTypePropertyToFilter(Filter $filter, QueryBuilder $queryBuilder): array
     {
+        $constraints = [];
         if ($filter->getUserType() === Filter::USER_TYPE_LOGGED_ON) {
             $constraints[] = $queryBuilder->expr()->gt('user', $queryBuilder->createNamedParameter(0, Connection::PARAM_INT));
         }
         if ($filter->getUserType() === Filter::USER_TYPE_LOGGED_OFF) {
             $constraints[] = $queryBuilder->expr()->eq('user', $queryBuilder->createNamedParameter(0, Connection::PARAM_INT));
         }
+        return $constraints;
     }
 
     /**
-     * @param string[] $constraints
+     * @param Filter $filter
+     * @param QueryBuilder $queryBuilder
+     * @return string[]
      */
-    protected function applyPeriodPropertyToFilter(Filter $filter, QueryBuilder $queryBuilder, array &$constraints): void
+    protected function applyPeriodPropertyToFilter(Filter $filter, QueryBuilder $queryBuilder): array
     {
+        $constraints = [];
         if ((int)$filter->getFrom() !== 0) {
             $constraints[] = $queryBuilder->expr()->gte('tstamp', $queryBuilder->createNamedParameter($filter->getFrom(), Connection::PARAM_INT));
         }
 
         if ((int)$filter->getTill() !== 0) {
             $constraints[] = $queryBuilder->expr()->lte('tstamp', $queryBuilder->createNamedParameter($filter->getTill(), Connection::PARAM_INT));
         }
+        return $constraints;
+
     }
 
     /**
-     * @param string[] $constraints
+     * @param int $property
+     * @param string $propertyName
+     * @param QueryBuilder $queryBuilder
+     * @return string[]
      */
-    protected function applyEqualPropertyToFilter(int $property, string $propertyName, QueryBuilder $queryBuilder, array &$constraints): void
+    protected function applyEqualPropertyToFilter(int $property, string $propertyName, QueryBuilder $queryBuilder): array
     {
+        $constraints = [];
         if ($property !== 0) {
             $constraints[] = $queryBuilder->expr()->eq($propertyName, $queryBuilder->createNamedParameter($property, Connection::PARAM_INT));
         }
+        return $constraints;
     }
 
     /**
@@ -203,7 +238,7 @@ public function logDownload(AbstractToken $token, int $fileSize, string $mimeTyp
         $log->setUser($user);
         $log->setPage($token->getPage());
 
-        $fileObject = GeneralUtility::makeInstance(ResourceFactory::class)->retrieveFileOrFolderObject($token->getFile());
+        $fileObject = $this->resourceFactory->retrieveFileOrFolderObject($token->getFile());
 
         if ($fileObject) {
             $log->setFileId((string)$fileObject->getUid());

diff --git a/Classes/Factory/SecureLinkFactory.php b/Classes/Factory/SecureLinkFactory.php
@@ -19,13 +19,13 @@
 use Leuchtfeuer\SecureDownloads\Exception\InvalidClassException;
 use Leuchtfeuer\SecureDownloads\Factory\Event\EnrichPayloadEvent;
 use Leuchtfeuer\SecureDownloads\Registry\TokenRegistry;
+use Psr\Http\Message\ServerRequestInterface;
 use TYPO3\CMS\Core\Context\Context;
 use TYPO3\CMS\Core\Context\Exception\AspectNotFoundException;
 use TYPO3\CMS\Core\Context\UserAspect;
 use TYPO3\CMS\Core\Core\Environment;
 use TYPO3\CMS\Core\EventDispatcher\EventDispatcher;
 use TYPO3\CMS\Core\Http\ApplicationType;
-use TYPO3\CMS\Core\Http\ServerRequest;
 use TYPO3\CMS\Core\SingletonInterface;
 use TYPO3\CMS\Core\Utility\GeneralUtility;
 use TYPO3\CMS\Frontend\ContentObject\Exception\ContentRenderingException;
@@ -56,12 +56,14 @@ protected function initializeToken(): void
         $this->token->setExp($this->calculateLinkLifetime());
         if (!Environment::isCli()) {
             $request = $this->getRequest();
-            if (ApplicationType::fromRequest($request)->isFrontend()) {
-                $pageArguments = $request->getAttribute('routing');
-                $pageId = $pageArguments?->getPageId();
-            } elseif (ApplicationType::fromRequest($request)->isBackend()) {
-                $site = $request->getAttribute('site');
-                $pageId = $site->getRootPageId();
+            if ($request instanceof ServerRequestInterface) {
+                if (ApplicationType::fromRequest($request)->isFrontend()) {
+                    $pageArguments = $request->getAttribute('routing');
+                    $pageId = $pageArguments?->getPageId();
+                } elseif (ApplicationType::fromRequest($request)->isBackend()) {
+                    $site = $request->getAttribute('site');
+                    $pageId = $site->getRootPageId();
+                }
             }
         }
         $this->token->setPage($pageId ?? 0);
@@ -76,7 +78,7 @@ protected function initializeToken(): void
         }
     }
 
-    private function getRequest(): ServerRequest
+    private function getRequest(): ?ServerRequestInterface
     {
         return $GLOBALS['TYPO3_REQUEST'];
     }

diff --git a/Classes/Middleware/FileDeliveryMiddleware.php b/Classes/Middleware/FileDeliveryMiddleware.php
@@ -50,7 +50,7 @@ public function process(ServerRequestInterface $request, RequestHandlerInterface
             $frontendUserAuthentication->fetchGroupData($request);
 
             $cleanPath = mb_substr(urldecode($request->getUri()->getPath()), mb_strlen($this->assetPrefix));
-            [$jwt, $basePath] = explode('/', $cleanPath);
+            [$jwt] = explode('/', $cleanPath);
 
             return GeneralUtility::makeInstance(FileDelivery::class)->deliver($jwt, $request);
         }

diff --git a/Classes/Resource/FileDelivery.php b/Classes/Resource/FileDelivery.php
@@ -88,7 +88,17 @@ public function deliver(string $jsonWebToken, ServerRequestInterface $request):
         $this->dispatchAfterFileRetrievedEvent($file, $fileName);
 
         if (file_exists($file)) {
-            $fileObject = $this->resourceFactory->retrieveFileOrFolderObject($this->token->getFile());
+            $fileObject = $this->resourceFactory->retrieveFileOrFolderObject($file);
+
+            if ($this->extensionConfiguration->isLog()) {
+                $this->token->log([
+                    'fileSize' => $fileSize = (int)filesize($file),
+                    'mimeType' => (new FileInfo($file))->getMimeType()
+                        ?: $this->guessMimeTypeByFileExtension($file)
+                            ?: MimeTypes::DEFAULT_MIME_TYPE,
+                ]);
+            }
+
             if ($fileObject instanceof File) {
                 $response = $fileObject
                     ->getStorage()
@@ -222,13 +232,6 @@ protected function getResponseBody(string $file, string $fileName): StreamInterf
         $this->dispatchBeforeFileDeliverEvent($outputFunction, $header, $fileName, $mimeType, $forceDownload);
         $this->header = $header;
 
-        if ($this->extensionConfiguration->isLog()) {
-            $this->token->log([
-                'fileSize' => $fileSize,
-                'mimeType' => $mimeType,
-            ]);
-        }
-
         return $this->outputFile($outputFunction, $file) ?? 'php://temp';
     }
 

diff --git a/Tests/Functional/Domain/Repository/Fixtures/Folders/assets/red.png b/Tests/Functional/Domain/Repository/Fixtures/Folders/assets/red.png
diff --git a/Tests/Functional/Domain/Repository/Fixtures/log.csv b/Tests/Functional/Domain/Repository/Fixtures/log.csv
@@ -0,0 +1,9 @@
+"tx_securedownloads_domain_model_log",
+,"uid","pid","tstamp","crdate","file_id","file_name","file_path","file_size","file_type","media_type","user","page"
+,1,0,1727859973,1727859973,"52","fixture_1","fileadmin/_processed_/e/1/csm_sunflower_b98a5975b3",67008,"jpg","image/jpeg",1,5
+,2,0,1727859974,1727859974,"82","fixture_2","fileadmin/_processed_/4/6/csm_landscape_eb858b5476",46235,"png","image/png",2,6
+,3,0,1727859976,1727859976,"82","fixture_3","fileadmin/_processed_/4/6/csm_landscape_eb858b5476",46235,"tiff","image/tiff",0,6
+,4,0,1727859977,1727859977,"52","fixture_4","fileadmin/_processed_/e/1/csm_sunflower_b98a5975b3",67008,"jpg","image/jpeg",0,5
+,5,0,1727859980,1727859980,"52","fixture_5","fileadmin/_processed_/e/1/csm_sunflower_b98a5975b3",67008,"bmp","image/bmp",0,5
+,6,0,1727860176,1727860176,"51","fixture_6","fileadmin/_processed_/e/1/csm_sunflower_29bee2a7c2",2152,"jpg","image/jpeg",5,1
+,7,0,1727860178,1727860178,"83","fixture_7","fileadmin/_processed_/4/6/csm_landscape_226c4e299f",1147,"webp","image/webp",6,1