API CHANGE Checking for SiteTree::$allowed_children in SiteTree->validate() (was only checked via JavaScript before).

BUGFIX Ensure that VirtualPage $allowed_children are checked on original classes to avoid allowing more than necessary (AIR-38)

code/model/SiteTree.php

@@ -16,6 +16,7 @@ class SiteTree extends DataObject implements PermissionProvider,i18nEntityProvid
 	 * If a classname is prefixed by "*", such as "*Page", then only that
 	 * class is allowed - no subclasses. Otherwise, the class and all its
 	 * subclasses are allowed.
+	 * To control allowed children on root level (no parent), use {@link $can_be_root}.
 	 *
 	 * @var array
 	 */
@@ -1479,6 +1480,36 @@ class SiteTree extends DataObject implements PermissionProvider,i18nEntityProvid
 		parent::onAfterDelete();
 	}
 	
+	function validate() {
+		$result = parent::validate();
+
+		// Allowed children validation 
+		$parent = $this->getParent();
+		if($parent && $parent->exists()) {
+			// No need to check for subclasses or instanceof, as allowedChildren() already 
+			// deconstructs any inheritance trees already.
+			$allowed = $parent->allowedChildren();
+			$subject = ($this instanceof VirtualPage) ? $this->CopyContentFrom() : $this;
+			if($subject->ID && !in_array($subject->ClassName, $allowed)) {
+				
+				$result->error(
+					sprintf(
+						_t(
+							'SiteTree.PageTypeNotAllowed', 
+							'Page type "%s" not allowed as child of this parent page', 
+							PR_MEDIUM,
+							'First argument is a class name'
+						),
+						$subject->i18n_singular_name()
+					),
+					'ALLOWED_CHILDREN'
+				);
+			}
+		}
+		
+		return $result;
+	}
+	
 	/**
 	 * Returns TRUE if this object has a URLSegment value that does not conflict with any other objects. This methods
 	 * checks for:
@@ -2329,9 +2360,12 @@ class SiteTree extends DataObject implements PermissionProvider,i18nEntityProvid
 	 * @return array
 	 */
 	function allowedChildren() {
+		$allowedChildren = array();
 		$candidates = $this->stat('allowed_children');
 		if($candidates && $candidates != "none" && $candidates != "SiteTree_root") {
 			foreach($candidates as $candidate) {
+				// If a classname is prefixed by "*", such as "*Page", then only that
+				// class is allowed - no subclasses. Otherwise, the class and all its subclasses are allowed.
 				if(substr($candidate,0,1) == '*') {
 					$allowedChildren[] = substr($candidate,1);
 				} else {
@@ -2341,8 +2375,9 @@ class SiteTree extends DataObject implements PermissionProvider,i18nEntityProvid
 					}
 				}
 			}
-			return $allowedChildren;
 		}
+		
+		return $allowedChildren;
 	}
 
 

tests/model/SiteTreeTest.php

@@ -10,6 +10,14 @@ class SiteTreeTest extends SapphireTest {
 		'SiteTree' => array('SiteTreeSubsites')
 	);
 	
+	protected $extraDataObjects = array(
+		'SiteTreeTest_ClassA',
+		'SiteTreeTest_ClassB',
+		'SiteTreeTest_ClassC',
+		'SiteTreeTest_ClassD',
+		'SiteTreeTest_ClassCext'
+	);
+	
 	/**
 	 * @todo Necessary because of monolithic Translatable design
 	 */
@@ -742,6 +750,81 @@ class SiteTreeTest extends SapphireTest {
 		$this->assertContains('Page', $classes, 'Page types do contain subclasses');
 	}
 	
+	function testAllowedChildren() {
+		$page = new SiteTree();
+		$this->assertContains(
+			'VirtualPage', 
+			$page->allowedChildren(),
+			'Includes core subclasses by default'
+		);
+		
+		$classA = new SiteTreeTest_ClassA();
+		$this->assertEquals(
+			array('SiteTreeTest_ClassB'), 
+			$classA->allowedChildren(),
+			'Direct setting of allowed children'
+		);
+		
+		$classB = new SiteTreeTest_ClassB();
+		$this->assertEquals(
+			array('SiteTreeTest_ClassC', 'SiteTreeTest_ClassCext'), 
+			$classB->allowedChildren(),
+			'Includes subclasses'
+		);
+		
+		$classD = new SiteTreeTest_ClassD();
+		$this->assertEquals(
+			array('SiteTreeTest_ClassC'), 
+			$classD->allowedChildren(),
+			'Excludes subclasses if class is prefixed by an asterisk'
+		);
+		
+		$classC = new SiteTreeTest_ClassC();
+		$this->assertEquals(
+			array(), 
+			$classC->allowedChildren(),
+			'Null setting'
+		);
+	}
+
+	function testAllowedChildrenValidation() {
+		$page = new SiteTree();
+		$page->write();
+		$classA = new SiteTreeTest_ClassA();
+		$classA->write();
+		$classB = new SiteTreeTest_ClassB();
+		$classB->write();
+		$classC = new SiteTreeTest_ClassC();
+		$classC->write();
+		$classD = new SiteTreeTest_ClassD();
+		$classD->write();
+		$classCext = new SiteTreeTest_ClassCext();
+		$classCext->write();
+		
+		$classB->ParentID = $page->ID;
+		$valid = $classB->validate();
+		$this->assertTrue($valid->valid(), "Does allow children on unrestricted parent");
+		
+		$classB->ParentID = $classA->ID;
+		$valid = $classB->validate();
+		$this->assertTrue($valid->valid(), "Does allow child specifically allowed by parent");
+
+		$classC->ParentID = $classA->ID;
+		$valid = $classC->validate();
+		$this->assertFalse($valid->valid(), "Doesnt allow child on parents specifically restricting children");
+		
+		$classB->ParentID = $classC->ID;
+		$valid = $classB->validate();
+		$this->assertFalse($valid->valid(), "Doesnt allow child on parents disallowing all children");
+		
+		$classB->ParentID = $classC->ID;
+		$valid = $classB->validate();
+		$this->assertFalse($valid->valid(), "Doesnt allow child on parents disallowing all children");
+		
+		$classCext->ParentID = $classD->ID;
+		$valid = $classCext->validate();
+		$this->assertFalse($valid->valid(), "Doesnt allow child where only parent class is allowed on parent node, and asterisk prefixing is used");
+	}
 }
 
 /**#@+
@@ -774,3 +857,29 @@ class SiteTreeTest_NullHtmlCleaner extends HTMLCleaner {
 		return $html;
 	}
 }
+
+class SiteTreeTest_ClassA extends Page implements TestOnly {
+
+	static $need_permission = array('ADMIN', 'CMS_ACCESS_CMSMain');
+	
+	static $allowed_children = array('SiteTreeTest_ClassB');
+}
+
+class SiteTreeTest_ClassB extends Page implements TestOnly {
+	// Also allowed subclasses
+	static $allowed_children = array('SiteTreeTest_ClassC'); 
+}
+
+class SiteTreeTest_ClassC extends Page implements TestOnly {
+	static $allowed_children = array();
+}
+
+class SiteTreeTest_ClassD extends Page implements TestOnly {
+	// Only allows this class, no children classes
+	static $allowed_children = array('*SiteTreeTest_ClassC');
+}
+
+class SiteTreeTest_ClassCext extends SiteTreeTest_ClassC implements TestOnly {
+	// Override SiteTreeTest_ClassC definitions
+	static $allowed_children = array('SiteTreeTest_ClassB');
+}

tests/model/VirtualPageTest.php

@@ -3,6 +3,11 @@
 class VirtualPageTest extends SapphireTest {
 	static $fixture_file = 'VirtualPageTest.yml';
 	
+	protected $extraDataObjects = array(
+		'VirtualPageTest_ClassA',
+		'VirtualPageTest_ClassB',
+	);
+	
 	/**
 	 * Test that, after you update the source page of a virtual page, all the virtual pages
 	 * are updated
@@ -321,4 +326,49 @@ class VirtualPageTest extends SapphireTest {
 		$vp = DataObject::get_by_id('SiteTree', $vp->ID);
 		$this->assertEquals(1, $vp->HasBrokenLink);
 	}	
+	
+	/**
+	 * Base functionality tested in {@link SiteTreeTest->testAllowedChildrenValidation()}.
+	 */
+	function testAllowedChildrenLimitedOnVirtualPages() {
+		$classA = new SiteTreeTest_ClassA();
+		$classA->write();
+		$classB = new SiteTreeTest_ClassB();
+		$classB->write();
+		$classBVirtual = new VirtualPage();
+		$classBVirtual->CopyContentFromID = $classB->ID;
+		$classBVirtual->write();
+		$classC = new SiteTreeTest_ClassC();
+		$classC->write();
+		$classCVirtual = new VirtualPage();
+		$classCVirtual->CopyContentFromID = $classC->ID;
+		$classCVirtual->write();
+		
+		$classBVirtual->ParentID = $classA->ID;
+		$valid = $classBVirtual->validate();
+		$this->assertTrue($valid->valid(), "Does allow child linked to virtual page type allowed by parent");
+		
+		$classCVirtual->ParentID = $classA->ID;
+		$valid = $classCVirtual->validate();
+		$this->assertFalse($valid->valid(), "Doesn't allow child linked to virtual page type disallowed by parent");
+	}
+}
+
+class VirtualPageTest_ClassA extends Page implements TestOnly {
+	
+	static $db = array(
+		'MyInitiallyCopiedField' => 'Text',
+		'MyVirtualField' => 'Text',
+		'MyNonVirtualField' => 'Text',
+	);
+	
+	static $allowed_children = array('VirtualPageTest_ClassB');
+}
+
+class VirtualPageTest_ClassB extends Page implements TestOnly {
+	static $allowed_children = array('VirtualPageTest_ClassC'); 
+}
+
+class VirtualPageTest_ClassC extends Page implements TestOnly {
+	static $allowed_children = array();
 }