From 0e26c066440d2591401e84d9688cbeef0595afcc Mon Sep 17 00:00:00 2001 From: Damian Mooyman Date: Tue, 20 Feb 2018 11:37:27 +1300 Subject: [PATCH 1/2] BUG Fix behaviour towards versioned but unstagable records --- .../GridField/GridFieldVersionedState.php | 18 +++-- src/ORM/DatabaseAdmin.php | 3 +- src/ORM/Hierarchy/Hierarchy.php | 44 ++++++----- src/Security/InheritedPermissions.php | 3 +- .../FormFactoryTest/ControllerExtension.php | 7 +- .../php/Security/InheritedPermissionsTest.php | 73 ++++++++++++++++++- .../php/Security/InheritedPermissionsTest.yml | 67 +++++++++++++++++ .../TestPermissionNode.php | 1 - .../InheritedPermissionsTest/UnstagedNode.php | 68 +++++++++++++++++ 9 files changed, 250 insertions(+), 34 deletions(-) create mode 100644 tests/php/Security/InheritedPermissionsTest/UnstagedNode.php diff --git a/src/Forms/GridField/GridFieldVersionedState.php b/src/Forms/GridField/GridFieldVersionedState.php index 44500f1b6..f69d7ab8e 100644 --- a/src/Forms/GridField/GridFieldVersionedState.php +++ b/src/Forms/GridField/GridFieldVersionedState.php @@ -5,6 +5,7 @@ namespace SilverStripe\Forms\GridField; use SilverStripe\ORM\DataObject; use SilverStripe\Versioned\Versioned; use SilverStripe\Core\Convert; +use SilverStripe\View\HTML; /** * @todo Move to siverstripe/versioned module @@ -46,8 +47,9 @@ class GridFieldVersionedState implements GridField_ColumnProvider return; } - $model = $gridField->getModelClass(); - $isModelVersioned = $model::has_extension(Versioned::class); + /** @var Versioned|DataObject $model */ + $model = DataObject::singleton($gridField->getModelClass()); + $isModelVersioned = $model->hasExtension(Versioned::class) && $model->hasStages(); if (!$isModelVersioned) { return; } @@ -92,10 +94,12 @@ class GridFieldVersionedState implements GridField_ColumnProvider if (is_string($data)) { $data = array('text' => $data); } - $flagContent .= sprintf( - " %s", - 'status-' . Convert::raw2xml($class), - (isset($data['title'])) ? sprintf(' title="%s"', Convert::raw2xml($data['title'])) : '', + $flagContent .= HTML::createTag( + 'span', + array_merge( + [ 'class' => 'ss-gridfield-badge badge status-' . $class ], + array_intersect_key($data, ['title' => true]) + ), Convert::raw2xml($data['text']) ); } @@ -156,7 +160,7 @@ class GridFieldVersionedState implements GridField_ColumnProvider */ protected function getStatusFlags($record) { - if (!$record->hasExtension(Versioned::class)) { + if (!$record->hasExtension(Versioned::class) || !$record->hasStages()) { return []; } diff --git a/src/ORM/DatabaseAdmin.php b/src/ORM/DatabaseAdmin.php index 4d87bc98c..c556c7bb0 100644 --- a/src/ORM/DatabaseAdmin.php +++ b/src/ORM/DatabaseAdmin.php @@ -337,8 +337,9 @@ class DatabaseAdmin extends Controller $updateQueries = [sprintf($updateQuery, '')]; // Remap versioned table ClassName values as well + /** @var Versioned|DataObject $class */ $class = DataObject::singleton($newClassName); - if ($class->has_extension(Versioned::class)) { + if ($class->hasExtension(Versioned::class)) { if ($class->hasStages()) { $updateQueries[] = sprintf($updateQuery, '_Live'); } diff --git a/src/ORM/Hierarchy/Hierarchy.php b/src/ORM/Hierarchy/Hierarchy.php index 73f0a4e9b..b47775085 100644 --- a/src/ORM/Hierarchy/Hierarchy.php +++ b/src/ORM/Hierarchy/Hierarchy.php @@ -208,12 +208,14 @@ class Hierarchy extends DataExtension */ public function AllChildrenIncludingDeleted() { - $stageChildren = $this->owner->stageChildren(true); + /** @var DataObject|Hierarchy|Versioned $owner */ + $owner = $this->owner; + $stageChildren = $owner->stageChildren(true); // Add live site content that doesn't exist on the stage site, if required. - if ($this->owner->hasExtension(Versioned::class)) { + if ($owner->hasExtension(Versioned::class) && $owner->hasStages()) { // Next, go through the live children. Only some of these will be listed - $liveChildren = $this->owner->liveChildren(true, true); + $liveChildren = $owner->liveChildren(true, true); if ($liveChildren) { $merged = new ArrayList(); $merged->merge($stageChildren); @@ -221,7 +223,7 @@ class Hierarchy extends DataExtension $stageChildren = $merged; } } - $this->owner->extend("augmentAllChildrenIncludingDeleted", $stageChildren); + $owner->extend("augmentAllChildrenIncludingDeleted", $stageChildren); return $stageChildren; } @@ -233,15 +235,19 @@ class Hierarchy extends DataExtension */ public function AllHistoricalChildren() { - if (!$this->owner->hasExtension(Versioned::class)) { - throw new Exception('Hierarchy->AllHistoricalChildren() only works with Versioned extension applied'); + /** @var DataObject|Versioned|Hierarchy $owner */ + $owner = $this->owner; + if (!$owner->hasExtension(Versioned::class) || !$owner->hasStages()) { + throw new Exception( + 'Hierarchy->AllHistoricalChildren() only works with Versioned extension applied with staging' + ); } - $baseTable = $this->owner->baseTable(); - $parentIDColumn = $this->owner->getSchema()->sqlColumnForField($this->owner, 'ParentID'); + $baseTable = $owner->baseTable(); + $parentIDColumn = $owner->getSchema()->sqlColumnForField($owner, 'ParentID'); return Versioned::get_including_deleted( - $this->owner->baseClass(), - [ $parentIDColumn => $this->owner->ID ], + $owner->baseClass(), + [ $parentIDColumn => $owner->ID ], "\"{$baseTable}\".\"ID\" ASC" ); } @@ -337,15 +343,17 @@ class Hierarchy extends DataExtension */ public function liveChildren($showAll = false, $onlyDeletedFromStage = false) { - if (!$this->owner->hasExtension(Versioned::class)) { - throw new Exception('Hierarchy->liveChildren() only works with Versioned extension applied'); + /** @var Versioned|DataObject|Hierarchy $owner */ + $owner = $this->owner; + if (!$owner->hasExtension(Versioned::class) || !$owner->hasStages()) { + throw new Exception('Hierarchy->liveChildren() only works with Versioned extension applied with staging'); } - $hideFromHierarchy = $this->owner->config()->hide_from_hierarchy; - $hideFromCMSTree = $this->owner->config()->hide_from_cms_tree; - $children = DataObject::get($this->owner->baseClass()) - ->filter('ParentID', (int)$this->owner->ID) - ->exclude('ID', (int)$this->owner->ID) + $hideFromHierarchy = $owner->config()->hide_from_hierarchy; + $hideFromCMSTree = $owner->config()->hide_from_cms_tree; + $children = DataObject::get($owner->baseClass()) + ->filter('ParentID', (int)$owner->ID) + ->exclude('ID', (int)$owner->ID) ->setDataQueryParam(array( 'Versioned.mode' => $onlyDeletedFromStage ? 'stage_unique' : 'stage', 'Versioned.stage' => 'Live' @@ -356,7 +364,7 @@ class Hierarchy extends DataExtension if ($hideFromCMSTree && $this->showingCMSTree()) { $children = $children->exclude('ClassName', $hideFromCMSTree); } - if (!$showAll && DataObject::getSchema()->fieldSpec($this->owner, 'ShowInMenus')) { + if (!$showAll && DataObject::getSchema()->fieldSpec($owner, 'ShowInMenus')) { $children = $children->filter('ShowInMenus', 1); } diff --git a/src/Security/InheritedPermissions.php b/src/Security/InheritedPermissions.php index df69fac2b..cb1ecccc7 100644 --- a/src/Security/InheritedPermissions.php +++ b/src/Security/InheritedPermissions.php @@ -577,8 +577,9 @@ class InheritedPermissions implements PermissionChecker if (!class_exists(Versioned::class)) { return false; } + /** @var Versioned|DataObject $singleton */ $singleton = DataObject::singleton($this->getBaseClass()); - return $singleton->hasExtension(Versioned::class); + return $singleton->hasExtension(Versioned::class) && $singleton->hasStages(); } public function clearCache() diff --git a/tests/php/Forms/FormFactoryTest/ControllerExtension.php b/tests/php/Forms/FormFactoryTest/ControllerExtension.php index c0fc749b2..49ba30dfe 100644 --- a/tests/php/Forms/FormFactoryTest/ControllerExtension.php +++ b/tests/php/Forms/FormFactoryTest/ControllerExtension.php @@ -8,6 +8,7 @@ use SilverStripe\Core\Extension; use SilverStripe\Forms\FieldList; use SilverStripe\Forms\FormAction; use SilverStripe\Forms\LiteralField; +use SilverStripe\ORM\DataObject; use SilverStripe\Versioned\Versioned; /** @@ -40,8 +41,9 @@ class ControllerExtension extends Extension if (empty($context['Record'])) { return; } + /** @var Versioned|DataObject $record */ $record = $context['Record']; - if ($record->hasExtension(Versioned::class)) { + if ($record->hasExtension(Versioned::class) && $record->hasStages()) { $actions->push(new FormAction('publish', 'Publish')); } } @@ -60,8 +62,9 @@ class ControllerExtension extends Extension if (empty($context['Record'])) { return; } + /** @var Versioned|DataObject $record */ $record = $context['Record']; - if ($record->hasExtension(Versioned::class)) { + if ($record->hasExtension(Versioned::class) && $record->hasStages()) { $link = $controller->Link('preview'); $fields->unshift( new LiteralField( diff --git a/tests/php/Security/InheritedPermissionsTest.php b/tests/php/Security/InheritedPermissionsTest.php index 5b9258908..fa14da8d0 100644 --- a/tests/php/Security/InheritedPermissionsTest.php +++ b/tests/php/Security/InheritedPermissionsTest.php @@ -10,6 +10,7 @@ use SilverStripe\Security\Member; use SilverStripe\Security\PermissionChecker; use SilverStripe\Security\Test\InheritedPermissionsTest\TestPermissionNode; use SilverStripe\Security\Test\InheritedPermissionsTest\TestDefaultPermissionChecker; +use SilverStripe\Security\Test\InheritedPermissionsTest\UnstagedNode; use SilverStripe\Versioned\Versioned; class InheritedPermissionsTest extends SapphireTest @@ -18,6 +19,7 @@ class InheritedPermissionsTest extends SapphireTest protected static $extra_dataobjects = [ TestPermissionNode::class, + UnstagedNode::class, ]; /** @@ -27,18 +29,38 @@ class InheritedPermissionsTest extends SapphireTest protected function setUp() { - parent::setUp(); + $this->rootPermissions = new TestDefaultPermissionChecker(); // Register root permissions - $permission = InheritedPermissions::create(TestPermissionNode::class) + $permission1 = InheritedPermissions::create(TestPermissionNode::class) ->setGlobalEditPermissions(['TEST_NODE_ACCESS']) - ->setDefaultPermissions($this->rootPermissions = new TestDefaultPermissionChecker()); + ->setDefaultPermissions($this->rootPermissions); Injector::inst()->registerService( - $permission, + $permission1, PermissionChecker::class . '.testpermissions' ); // Reset root permission + $permission2 = InheritedPermissions::create(UnstagedNode::class) + ->setGlobalEditPermissions(['TEST_NODE_ACCESS']) + ->setDefaultPermissions($this->rootPermissions); + Injector::inst()->registerService( + $permission2, + PermissionChecker::class . '.unstagedpermissions' + ); + + parent::setUp(); + + $permission1->clearCache(); + $permission2->clearCache(); + } + + protected function tearDown() + { + Injector::inst()->unregisterNamedObject(PermissionChecker::class . '.testpermissions'); + Injector::inst()->unregisterNamedObject(PermissionChecker::class . '.unstagedpermissions'); + $this->rootPermissions = null; + parent::tearDown(); } public function testEditPermissions() @@ -160,6 +182,49 @@ class InheritedPermissionsTest extends SapphireTest $this->assertFalse($history->canView($editor)); } + public function testUnstagedViewPermissions() + { + $history = $this->objFromFixture(UnstagedNode::class, 'history'); + $contact = $this->objFromFixture(UnstagedNode::class, 'contact'); + $contactForm = $this->objFromFixture(UnstagedNode::class, 'contact-form'); + $secret = $this->objFromFixture(UnstagedNode::class, 'secret'); + $secretNested = $this->objFromFixture(UnstagedNode::class, 'secret-nested'); + $protected = $this->objFromFixture(UnstagedNode::class, 'protected'); + $protectedChild = $this->objFromFixture(UnstagedNode::class, 'protected-child'); + $editor = $this->objFromFixture(Member::class, 'editor'); + + // Not logged in user can only access Inherit or Anyone pages + Member::actAs( + null, + function () use ($protectedChild, $secretNested, $protected, $secret, $history, $contact, $contactForm) { + $this->assertTrue($history->canView()); + $this->assertTrue($contact->canView()); + $this->assertTrue($contactForm->canView()); + // Protected + $this->assertFalse($secret->canView()); + $this->assertFalse($secretNested->canView()); + $this->assertFalse($protected->canView()); + $this->assertFalse($protectedChild->canView()); + } + ); + + // Editor can view pages restricted to logged in users + $this->assertTrue($secret->canView($editor)); + $this->assertTrue($secretNested->canView($editor)); + + // Cannot read admin-only pages + $this->assertFalse($protected->canView($editor)); + $this->assertFalse($protectedChild->canView($editor)); + + // Check root permissions + $this->assertTrue($history->canView($editor)); + + UnstagedNode::getInheritedPermissions()->clearCache(); + $this->rootPermissions->setCanView(false); + + $this->assertFalse($history->canView($editor)); + } + /** * Test that draft permissions deny unrestricted live permissions */ diff --git a/tests/php/Security/InheritedPermissionsTest.yml b/tests/php/Security/InheritedPermissionsTest.yml index 9e8ebb625..21fa846da 100644 --- a/tests/php/Security/InheritedPermissionsTest.yml +++ b/tests/php/Security/InheritedPermissionsTest.yml @@ -100,3 +100,70 @@ SilverStripe\Security\Test\InheritedPermissionsTest\TestPermissionNode: Title: Child CanViewType: Inherit Parent: =>SilverStripe\Security\Test\InheritedPermissionsTest\TestPermissionNode.protected + +SilverStripe\Security\Test\InheritedPermissionsTest\UnstagedNode: + about: + Title: About Us + CanEditType: OnlyTheseUsers + EditorGroups: =>SilverStripe\Security\Group.admins + about-staff: + Title: Staff + CanEditType: Inherit + Parent: =>SilverStripe\Security\Test\InheritedPermissionsTest\UnstagedNode.about + about-staff-ceo: + Title: CEO + Parent: =>SilverStripe\Security\Test\InheritedPermissionsTest\UnstagedNode.about-staff + about-staffduplicate: + Title: Staff + Parent: =>SilverStripe\Security\Test\InheritedPermissionsTest\UnstagedNode.about + products: + Title: Products + CanEditType: OnlyTheseUsers + EditorGroups: =>SilverStripe\Security\Group.editors + products-product1: + Title: 1.1 Test Product + Parent: =>SilverStripe\Security\Test\InheritedPermissionsTest\UnstagedNode.products + CanEditType: Inherit + products-product2: + Title: Another Product + Parent: =>SilverStripe\Security\Test\InheritedPermissionsTest\UnstagedNode.products + CanEditType: Inherit + products-product3: + Title: Another Product + Parent: =>SilverStripe\Security\Test\InheritedPermissionsTest\UnstagedNode.products + CanEditType: Inherit + products-product4: + Title: Another Product + Parent: =>SilverStripe\Security\Test\InheritedPermissionsTest\UnstagedNode.products + CanEditType: OnlyTheseUsers + EditorGroups: =>SilverStripe\Security\Group.admins + history: + Title: History + CanViewType: Inherit + CanEditType: Inherit + history-gallery: + Title: Gallery + CanViewType: Inherit + Parent: =>SilverStripe\Security\Test\InheritedPermissionsTest\UnstagedNode.history + contact: + Title: Contact Us + CanViewType: Anyone + contact-form: + Title: Send us a message + CanViewType: Inherit + Parent: =>SilverStripe\Security\Test\InheritedPermissionsTest\UnstagedNode.contact + secret: + Title: Secret + CanViewType: LoggedInUsers + secret-nested: + Title: Nested + CanViewType: Inherit + Parent: =>SilverStripe\Security\Test\InheritedPermissionsTest\UnstagedNode.secret + protected: + Title: Protected + CanViewType: OnlyTheseUsers + ViewerGroups: =>SilverStripe\Security\Group.admins + protected-child: + Title: Child + CanViewType: Inherit + Parent: =>SilverStripe\Security\Test\InheritedPermissionsTest\UnstagedNode.protected diff --git a/tests/php/Security/InheritedPermissionsTest/TestPermissionNode.php b/tests/php/Security/InheritedPermissionsTest/TestPermissionNode.php index 2e06d6149..f78c9ea11 100644 --- a/tests/php/Security/InheritedPermissionsTest/TestPermissionNode.php +++ b/tests/php/Security/InheritedPermissionsTest/TestPermissionNode.php @@ -7,7 +7,6 @@ use SilverStripe\Dev\TestOnly; use SilverStripe\ORM\DataObject; use SilverStripe\Security\InheritedPermissions; use SilverStripe\Security\InheritedPermissionsExtension; -use SilverStripe\Security\Member; use SilverStripe\Security\PermissionChecker; use SilverStripe\Security\Security; use SilverStripe\Versioned\Versioned; diff --git a/tests/php/Security/InheritedPermissionsTest/UnstagedNode.php b/tests/php/Security/InheritedPermissionsTest/UnstagedNode.php new file mode 100644 index 000000000..2d99ffaad --- /dev/null +++ b/tests/php/Security/InheritedPermissionsTest/UnstagedNode.php @@ -0,0 +1,68 @@ + "Varchar(255)", + ]; + + private static $has_one = [ + "Parent" => self::class, + ]; + + private static $table_name = 'InheritedPermissionsTest_UnstagedNode'; + + private static $extensions = [ + Versioned::class . '.versioned', + InheritedPermissionsExtension::class, + ]; + + /** + * @return InheritedPermissions + */ + public static function getInheritedPermissions() + { + /** @var InheritedPermissions $permissions */ + return Injector::inst()->get(PermissionChecker::class . '.unstagedpermissions'); + } + + public function canEdit($member = null) + { + if (!$member) { + $member = Security::getCurrentUser(); + } + return static::getInheritedPermissions()->canEdit($this->ID, $member); + } + + public function canView($member = null) + { + if (!$member) { + $member = Security::getCurrentUser(); + } + return static::getInheritedPermissions()->canView($this->ID, $member); + } + + public function canDelete($member = null) + { + if (!$member) { + $member = Security::getCurrentUser(); + } + return static::getInheritedPermissions()->canDelete($this->ID, $member); + } +} From 2f2390be59f7b2e5f7ef2056ee10da4ed99ff571 Mon Sep 17 00:00:00 2001 From: Fred Condo Date: Tue, 20 Feb 2018 16:51:51 -0800 Subject: [PATCH 2/2] DOCS: remove backticks from Markdown link Markdown does not render links within backticks. --- docs/en/02_Developer_Guides/05_Extending/04_Shortcodes.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/docs/en/02_Developer_Guides/05_Extending/04_Shortcodes.md b/docs/en/02_Developer_Guides/05_Extending/04_Shortcodes.md index abfd9f8b6..d33f6c3ac 100644 --- a/docs/en/02_Developer_Guides/05_Extending/04_Shortcodes.md +++ b/docs/en/02_Developer_Guides/05_Extending/04_Shortcodes.md @@ -115,7 +115,7 @@ Links to internal `File` database records work exactly the same, but with the `[ ### Images Images inserted through the "Insert Media" form (WYSIWYG editor) need to retain a relationship with -the underlying `[Image](api:SilverStripe\Assets\Image)` database record. The `[image]` shortcode saves this database reference +the underlying [Image](api:SilverStripe\Assets\Image) database record. The `[image]` shortcode saves this database reference instead of hard-linking to the filesystem path of a given image. ```html