Merge pull request #1058 from ss23/infinite_loop_test

Fix #776 - Change how can_edit works

code/model/SiteTree.php

@@ -1047,10 +1047,8 @@ class SiteTree extends DataObject implements PermissionProvider,i18nEntityProvid
 	}
 	
 	/**
-	 * This method is NOT a full replacement for the individual can*() methods, e.g. {@link canEdit()}.
+	 * A wrapper method to batch_permission_check_stage, which checks can_edit permissions on a single
-	 * Rather than checking (potentially slow) PHP logic, it relies on the database group associations,
+	 * stage.
-	 * e.g. the "CanEditType" field plus the "SiteTree_EditorGroups" many-many table.
-	 * By batch checking multiple records, we can combine the queries efficiently.
 	 *
 	 * Caches based on $typeField data. To invalidate the cache, use {@link SiteTree::reset()}
 	 * or set the $useCached property to FALSE.
@@ -1065,22 +1063,17 @@ class SiteTree extends DataObject implements PermissionProvider,i18nEntityProvid
 	 * @return Array An map of {@link SiteTree} ID keys, to boolean values
 	 */
 	static public function batch_permission_check($ids, $memberID, $typeField, $groupJoinTable, $siteConfigMethod, $globalPermission = 'CMS_ACCESS_CMSMain', $useCached = true) {
-		// Sanitise the IDs
+		// Sanatise the IDs
 		$ids = array_filter($ids, 'is_numeric');
 
-		// This is the name used on the permission cache
+		// By default, no permission
-		// converts something like 'CanEditType' to 'edit'.
-		$cacheKey = strtolower(substr($typeField, 3, -4)) . "-$memberID";
-
-		// Default result: nothing editable
 		$result = array_fill_keys($ids, false);
-		if($ids) {
 
-			// Look in the cache for values
+		$cacheKey = "canEdit-$memberID";
 		if ($useCached && isset(self::$cache_permissions[$cacheKey])) {
-				$cachedValues = array_intersect_key(self::$cache_permissions[$cacheKey], $result);
+			// Before we start, we'll check the cache
 
-				// If we can't find everything in the cache, then look up the remainder separately
+			$cachedValues = array_intersect_key(self::$cache_permissions[$cacheKey], $result);
 			$uncachedValues = array_diff_key($result, self::$cache_permissions[$cacheKey]);
 			if ($uncachedValues) {
 				$cachedValues = self::batch_permission_check(array_keys($uncachedValues), $memberID, $typeField, $groupJoinTable, $siteConfigMethod, $globalPermission, false) + $cachedValues;
@@ -1088,6 +1081,44 @@ class SiteTree extends DataObject implements PermissionProvider,i18nEntityProvid
 			return $cachedValues;
 		}
 
+		// First lets check the stage version, then the live
+		$results = self::batch_permission_check_stage($ids, $memberID, $typeField, $groupJoinTable, $siteConfigMethod, $globalPermission, 'Stage');
+		$results += self::batch_permission_check_stage($ids, $memberID, $typeField, $groupJoinTable, $siteConfigMethod, $globalPermission, 'Live');
+
+		if ($useCached) {
+			// Create an array if one doesn't already exist
+			if (empty(self::$cache_permissions[$cacheKey])) self::$cache_permissions[$cacheKey] = array();
+			// Store the cache results
+			self::$cache_permissions[$cacheKey] = $results + self::$cache_permissions[$cacheKey];
+		}
+		return $results;
+	}
+
+	/**
+	 * This method is NOT a full replacement for the individual can*() methods, e.g. {@link canEdit()}.
+	 * Rather than checking (potentially slow) PHP logic, it relies on the database group associations,
+	 * e.g. the "CanEditType" field plus the "SiteTree_EditorGroups" many-many table.
+	 * By batch checking multiple records, we can combine the queries efficiently.
+	 * 
+	 * Caches based on $typeField data. To invalidate the cache, use {@link SiteTree::reset()}
+	 * or set the $useCached property to FALSE. 
+	 * 
+	 * @param Array $ids Of {@link SiteTree} IDs
+	 * @param Int $memberID Member ID
+	 * @param String $typeField A property on the data record, e.g. "CanEditType".
+	 * @param String $groupJoinTable A many-many table name on this record, e.g. "SiteTree_EditorGroups"
+	 * @param String $siteConfigMethod Method to call on {@link SiteConfig} for toplevel items, e.g. "canEdit"
+	 * @param String $globalPermission If the member doesn't have this permission code, don't bother iterating deeper.
+	 * @param String $stage Either Stage or Live, determines which stage to check permissions for
+	 * @return Array An map of {@link SiteTree} ID keys, to boolean values
+	 */
+	static public function batch_permission_check_stage($ids, $memberID, $typeField, $groupJoinTable, $siteConfigMethod, $globalPermission = 'CMS_ACCESS_CMSMain', $stage = 'Stage') {
+		// Sanitise the IDs
+		$ids = array_filter($ids, 'is_numeric');
+		
+		// Default result: nothing editable
+		$result = array_fill_keys($ids, false);
+		if($ids) {
 			// If a member doesn't have a certain permission then they can't edit anything
 			if(!$memberID || ($globalPermission && !Permission::checkMember($memberID, $globalPermission))) {
 				return $result;
@@ -1095,21 +1126,22 @@ class SiteTree extends DataObject implements PermissionProvider,i18nEntityProvid
 
 			$SQL_idList = implode($ids, ", ");
 
-			// if page can't be viewed, don't grant edit permissions
-			// to do - implement can_view_multiple(), so this can be enabled
-			//$ids = array_keys(array_filter(self::can_view_multiple($ids, $memberID)));
-		
 			// Get the groups that the given member belongs to
 			$groupIDs = DataObject::get_by_id('Member', $memberID)->Groups()->column("ID");
 			$SQL_groupList = implode(", ", $groupIDs);
 			if (!$SQL_groupList) $SQL_groupList = '0';
 			
-			$combinedStageResult = array();
+			switch ($stage) {
+				case 'Live':
+					$table = 'SiteTree_Live';
+					break;
+				default:
+					user_error('Invalid stage passed to batch_permission_check_stage()', E_USER_WARNING);
+				case 'Stage':
+					$table = 'SiteTree';
+			}
 
-			foreach(array('Stage', 'Live') as $stage) {
 			// Start by filling the array with the pages that actually exist
-				$table = ($stage=='Stage') ? "SiteTree" : "SiteTree_$stage";
-				
 			$result = array_fill_keys(
 				($ids) ? DB::query("SELECT \"ID\" FROM \"$table\" WHERE \"ID\" IN (".implode(", ", $ids).")")->column() : array(), 
 				false
@@ -1147,7 +1179,7 @@ class SiteTree extends DataObject implements PermissionProvider,i18nEntityProvid
 				}
 
 				if($groupedByParent) {
-						$actuallyInherited = self::batch_permission_check(array_keys($groupedByParent), $memberID, $typeField, $groupJoinTable, $siteConfigMethod);
+					$actuallyInherited = self::batch_permission_check_stage(array_keys($groupedByParent), $memberID, $typeField, $groupJoinTable, $siteConfigMethod, null, $stage);
 					if($actuallyInherited) {
 						$parentIDs = array_keys(array_filter($actuallyInherited));
 						foreach($parentIDs as $parentID) {
@@ -1157,18 +1189,10 @@ class SiteTree extends DataObject implements PermissionProvider,i18nEntityProvid
 					}
 				}
 			}
-				
-				$combinedStageResult = $combinedStageResult + $result;
-				
-			}
 		}
 
-		if(isset($combinedStageResult)) {
+		if (isset($result)) {
-			// Cache the results
+			return $result;
- 			if(empty(self::$cache_permissions[$cacheKey])) self::$cache_permissions[$cacheKey] = array();
- 			self::$cache_permissions[$cacheKey] = $combinedStageResult + self::$cache_permissions[$cacheKey];
- 
-  		return $combinedStageResult;
 		} else {
 			return array();
 		}

tests/model/SiteTreePermissionsTest.php

@@ -431,4 +431,43 @@ class SiteTreePermissionsTest extends FunctionalTest {
 		$this->assertFalse($page->canEdit($user), 'Website user can\'t edit a page when set to inherit from the SiteConfig, and SiteConfig has canEdit set to OnlyTheseUsers');
 	}
 
+	public function testInfiniteRecursionBug() {
+		// Create three SiteTree objects
+
+		$page1 = new SiteTree();
+		$page1->write();
+		$page1->publish("Stage", "Live");
+
+		$page2 = new SiteTree();
+		$page2->ParentID = $page1->ID;
+		$page2->write();
+		$page2->publish("Stage", "Live");
+
+		$page3 = new SiteTree();
+		$page3->ParentID = $page2->ID;
+		$page3->write();
+		$page3->publish("Stage", "Live");
+
+		// Current Heirachechy
+		// Page 1 -> Page 2 -> Page 3
+
+		// Now lets re-arrange the state of staging to cause the loop
+		$page3->ParentID = $page1->ID;
+		$page3->write();
+
+		// Note that we write page3 before page2, otherwise we would cause a 'normal' infinite loop
+		$page2->ParentID = $page3->ID;
+		$page2->write();
+
+		// New Heirachy (of staging)
+		// Page1 -> Page 3 -> Page 2
+
+		// Now, if we can check the edit permission of page3 and page2, the bug is fixed
+		$user = $this->objFromFixture('Member', 'subadmin');
+		$this->session()->inst_set('loggedInAs', $user->ID);
+
+		$this->assertTrue($page3->canEdit($user));
+		$this->assertTrue($page2->canEdit($user));
+	}
+	
 }