[TASK] Refactor methods in DataHandler related to page delete access 97/51497/2
authorStephan Großberndt <stephan@grossberndt.de>
Wed, 1 Feb 2017 13:05:25 +0000 (14:05 +0100)
committerJigal van Hemert <jigal.van.hemert@typo3.org>
Wed, 22 Mar 2017 21:30:41 +0000 (22:30 +0100)
* Removes usage of `rmComma()` and deprecates it
* Replaces usage of `noRecordsFromUnallowedTables()` with protected
method and deprecates it
* Removes duplicated code
* Improves variable names and type safety

Fixes: #79580
Releases: master
Change-Id: Ic9b95ede11adf73397b19f10acee58ba920624f7
Reviewed-on: https://review.typo3.org/51497
Tested-by: TYPO3com <no-reply@typo3.com>
Reviewed-by: Thomas Hohn <thomas@hohn.dk>
Tested-by: Thomas Hohn <thomas@hohn.dk>
Reviewed-by: Jigal van Hemert <jigal.van.hemert@typo3.org>
Tested-by: Jigal van Hemert <jigal.van.hemert@typo3.org>
typo3/sysext/core/Classes/DataHandling/DataHandler.php
typo3/sysext/core/Classes/Utility/GeneralUtility.php
typo3/sysext/core/Documentation/Changelog/master/Deprecation-79580-MethodsInDataHandlerRelatedToPageDeleteAccess.rst [new file with mode: 0644]

index d78d5c9..c02b9a2 100644 (file)
@@ -5270,7 +5270,7 @@ class DataHandler
     }
 
     /**
-     * Used to delete page because it will check for branch below pages and unallowed tables on the page as well.
+     * Used to delete page because it will check for branch below pages and disallowed tables on the page as well.
      *
      * @param int $uid Page id
      * @param bool $force If TRUE, pages are not checked for permission.
@@ -5286,9 +5286,9 @@ class DataHandler
         }
         // Getting list of pages to delete:
         if ($force) {
-            // Returns the branch WITHOUT permission checks (0 secures that)
-            $brExist = $this->doesBranchExist('', $uid, 0, 1);
-            $res = GeneralUtility::trimExplode(',', $brExist . $uid, true);
+            // Returns the branch WITHOUT permission checks (0 secures that), so it cannot return -1
+            $pageIdsInBranch = $this->doesBranchExist('', $uid, 0, true);
+            $res = GeneralUtility::intExplode(',', $pageIdsInBranch . $uid, true);
         } else {
             $res = $this->canDeletePage($uid);
         }
@@ -5302,7 +5302,6 @@ class DataHandler
             $flashMessage = GeneralUtility::makeInstance(FlashMessage::class, $res, '', FlashMessage::ERROR, true);
             /** @var $flashMessageService FlashMessageService */
             $flashMessageService = GeneralUtility::makeInstance(FlashMessageService::class);
-            /** @var $defaultFlashMessageQueue \TYPO3\CMS\Core\Messaging\FlashMessageQueue */
             $flashMessageService->getMessageQueueByIdentifier()->addMessage($flashMessage);
 
             if ($this->enableLogging) {
@@ -5402,52 +5401,45 @@ class DataHandler
      * Used to evaluate if a page can be deleted
      *
      * @param int $uid Page id
-     * @return array|string If array: List of page uids to traverse and delete (means OK), if string: error message.
+     * @return int[]|string If array: List of page uids to traverse and delete (means OK), if string: error message.
      */
     public function canDeletePage($uid)
     {
+        $uid = (int)$uid;
+
         // If we may at all delete this page
         if (!$this->doesRecordExist('pages', $uid, 'delete')) {
             return 'Attempt to delete page without permissions';
         }
 
+        $pageIdsInBranch = $this->doesBranchExist('', $uid, $this->pMap['delete'], true);
+
         if ($this->deleteTree) {
-            // Returns the branch
-            $brExist = $this->doesBranchExist('', $uid, $this->pMap['delete'], 1);
-            // Checks if we had permissions
-            if ($brExist == -1) {
+            if ($pageIdsInBranch === -1) {
                 return 'Attempt to delete pages in branch without permissions';
             }
 
-            if (!$this->noRecordsFromUnallowedTables($brExist . $uid)) {
-                return 'Attempt to delete records from disallowed tables';
-            }
-
-            $pagesInBranch = GeneralUtility::trimExplode(',', $brExist . $uid, true);
-            foreach ($pagesInBranch as $pageInBranch) {
-                if (!$this->BE_USER->recordEditAccessInternals('pages', $pageInBranch, false, false, true)) {
-                    return 'Attempt to delete page which has prohibited localizations.';
-                }
-            }
-            return $pagesInBranch;
+            $pagesInBranch = GeneralUtility::intExplode(',', $pageIdsInBranch . $uid, true);
         } else {
-            // returns the branch
-            $brExist = $this->doesBranchExist('', $uid, $this->pMap['delete'], 1);
-            // Checks if branch exists
-            if ($brExist != '') {
+            if ($pageIdsInBranch === -1) {
+                return 'Attempt to delete page without permissions';
+            } elseif ($pageIdsInBranch !== '') {
                 return 'Attempt to delete page which has subpages';
             }
 
-            if (!$this->noRecordsFromUnallowedTables($uid)) {
-                return 'Attempt to delete records from disallowed tables';
-            }
+            $pagesInBranch = [$uid];
+        }
 
-            if ($this->BE_USER->recordEditAccessInternals('pages', $uid, false, false, true)) {
-                return [$uid];
-            } else {
+        if (!$this->checkForRecordsFromDisallowedTables($pagesInBranch)) {
+            return 'Attempt to delete records from disallowed tables';
+        }
+
+        foreach ($pagesInBranch as $pageInBranch) {
+            if (!$this->BE_USER->recordEditAccessInternals('pages', $pageInBranch, false, false, true)) {
                 return 'Attempt to delete page which has prohibited localizations.';
             }
         }
+        return $pagesInBranch;
     }
 
     /**
@@ -6671,15 +6663,14 @@ class DataHandler
     /**
      * Checks if a whole branch of pages exists
      *
-     * Tests the branch under $pid (like doesRecordExist). It doesn't test the page with $pid as uid. Use doesRecordExist() for this purpose
-     * Returns an ID-list or "" if OK. Else -1 which means that somewhere there was no permission (eg. to delete).
-     * if $recurse is set, then the function will follow subpages. This MUST be set, if we need the idlist for deleting pages or else we get an incomplete list
+     * Tests the branch under $pid like doesRecordExist(), but it doesn't test the page with $pid as uid - use doesRecordExist() for this purpose.
+     * If $recurse is set, the function will follow subpages. This MUST be set, if we need the id-list for deleting pages or else we get an incomplete list
      *
-     * @param string $inList List of page uids, this is added to and outputted in the end
+     * @param string $inList List of page uids, this is added to and returned in the end
      * @param int $pid Page ID to select subpages from.
      * @param int $perms Perms integer to check each page record for.
      * @param bool $recurse Recursion flag: If set, it will go out through the branch.
-     * @return string List of integers in branch
+     * @return string|int List of page IDs in branch, if there are subpages, empty string if there are none or -1 if no permission
      */
     public function doesBranchExist($inList, $pid, $perms, $recurse)
     {
@@ -6701,7 +6692,7 @@ class DataHandler
                     if ($recurse) {
                         // Follow the subpages recursively...
                         $inList = $this->doesBranchExist($inList, $row['uid'], $perms, $recurse);
-                        if ($inList == -1) {
+                        if ($inList === -1) {
                             return -1;
                         }
                     }
@@ -7689,9 +7680,11 @@ class DataHandler
      *
      * @param string $input Input string
      * @return string Output string with any comma in the end removed, if any.
+     * @deprecated since TYPO3 v8, will be removed in TYPO3 v9
      */
     public function rmComma($input)
     {
+        GeneralUtility::logDeprecatedFunction();
         return rtrim($input, ',');
     }
 
@@ -8229,16 +8222,28 @@ class DataHandler
      *
      * @param string $inList List of page integers
      * @return bool Return TRUE, if permission granted
+     * @deprecated since TYPO3 v8, will be removed in TYPO3 v9
      */
     public function noRecordsFromUnallowedTables($inList)
     {
-        if (strpos($inList, ',') !== false) {
-            $pids = GeneralUtility::intExplode(',', $inList, true);
-        } else {
-            $inList = trim($this->rmComma(trim($inList)));
-            $pids = [$inList];
+        GeneralUtility::logDeprecatedFunction();
+        return $this->checkForRecordsFromDisallowedTables(GeneralUtility::intExplode(',', $inList, true));
+    }
+
+    /**
+     * Check if there are records from tables on the pages to be deleted which the current user is not allowed to
+     *
+     * @param int[] $pageIds IDs of pages which should be checked
+     * @return bool Return TRUE, if permission granted
+     * @see canDeletePage()
+     */
+    protected function checkForRecordsFromDisallowedTables(array $pageIds)
+    {
+        if ($this->admin) {
+            return true;
         }
-        if ($inList && !$this->admin) {
+
+        if (!empty($pageIds)) {
             foreach ($GLOBALS['TCA'] as $table => $_) {
                 $query = GeneralUtility::makeInstance(ConnectionPool::class)->getQueryBuilderForTable($table);
                 $query->getRestrictions()
@@ -8248,7 +8253,7 @@ class DataHandler
                     ->from($table)
                     ->where($query->expr()->in(
                         'pid',
-                        $query->createNamedParameter($pids, Connection::PARAM_INT_ARRAY)
+                        $query->createNamedParameter($pageIds, Connection::PARAM_INT_ARRAY)
                     ))
                     ->execute()
                     ->fetchColumn(0);
index 47f714a..97bf2c8 100644 (file)
@@ -1210,7 +1210,7 @@ class GeneralUtility
      *************************/
 
     /**
-     * Explodes a $string delimited by $delim and casts each item in the array to (int).
+     * Explodes a $string delimited by $delimiter and casts each item in the array to (int).
      * Corresponds to \TYPO3\CMS\Core\Utility\GeneralUtility::trimExplode(), but with conversion to integers for all values.
      *
      * @param string $delimiter Delimiter string to explode with
diff --git a/typo3/sysext/core/Documentation/Changelog/master/Deprecation-79580-MethodsInDataHandlerRelatedToPageDeleteAccess.rst b/typo3/sysext/core/Documentation/Changelog/master/Deprecation-79580-MethodsInDataHandlerRelatedToPageDeleteAccess.rst
new file mode 100644 (file)
index 0000000..a65a1b6
--- /dev/null
@@ -0,0 +1,35 @@
+.. include:: ../../Includes.txt
+
+====================================================================================
+Deprecation: #79580 - Deprecate methods in DataHandler related to page delete access
+====================================================================================
+
+See :issue:`79580`
+
+Description
+===========
+
+The following methods have been marked as deprecated:
+
+* :code:`TYPO3\CMS\Core\DataHandling\DataHandler->rmComma()`
+* :code:`TYPO3\CMS\Core\DataHandling\DataHandler->noRecordsFromUnallowedTables()`
+
+Impact
+======
+
+Calling these methods will trigger a deprecation log entry.
+
+
+Affected Installations
+======================
+
+Any TYPO3 extension calling one of these methods.
+
+
+Migration
+=========
+
+Use native :php:`rtrim($input, ',')` instead of :code:`TYPO3\CMS\Core\DataHandling\DataHandler->rmComma()`.
+No migration available for :code:`TYPO3\CMS\Core\DataHandling\DataHandler->noRecordsFromUnallowedTables()`.
+
+.. index:: Database, PHP-API