From 2dd96a4050c74554c82b3ee42a3ee65a3eeea602 Mon Sep 17 00:00:00 2001 From: Ingo Schommer Date: Thu, 6 Oct 2011 16:47:59 +0200 Subject: [PATCH] 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 | 37 ++++++++++- tests/model/SiteTreeTest.php | 109 ++++++++++++++++++++++++++++++++ tests/model/VirtualPageTest.php | 50 +++++++++++++++ 3 files changed, 195 insertions(+), 1 deletion(-) diff --git a/code/model/SiteTree.php b/code/model/SiteTree.php index 3a9f7e70..ce7775de 100644 --- a/code/model/SiteTree.php +++ b/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; } diff --git a/tests/model/SiteTreeTest.php b/tests/model/SiteTreeTest.php index 8ad36a3e..cf429926 100644 --- a/tests/model/SiteTreeTest.php +++ b/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"); + } } /**#@+ @@ -773,4 +856,30 @@ class SiteTreeTest_NullHtmlCleaner extends HTMLCleaner { function cleanHTML($html) { 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'); } \ No newline at end of file diff --git a/tests/model/VirtualPageTest.php b/tests/model/VirtualPageTest.php index 5b96bf5b..8a77d2dc 100644 --- a/tests/model/VirtualPageTest.php +++ b/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(); } \ No newline at end of file