diff --git a/src/Extensions/ContentReviewCMSExtension.php b/src/Extensions/ContentReviewCMSExtension.php index c7da960..3768819 100644 --- a/src/Extensions/ContentReviewCMSExtension.php +++ b/src/Extensions/ContentReviewCMSExtension.php @@ -6,10 +6,10 @@ use SilverStripe\Admin\LeftAndMain; use SilverStripe\Admin\LeftAndMainExtension; use SilverStripe\CMS\Model\SiteTree; use SilverStripe\ContentReview\Forms\ReviewContentHandler; +use SilverStripe\ContentReview\Traits\PermissionChecker; use SilverStripe\Control\HTTPRequest; use SilverStripe\Control\HTTPResponse; use SilverStripe\Control\HTTPResponse_Exception; -use SilverStripe\Core\Convert; use SilverStripe\Forms\Form; use SilverStripe\ORM\ValidationResult; use SilverStripe\Security\Security; @@ -20,6 +20,8 @@ use SilverStripe\Security\Security; */ class ContentReviewCMSExtension extends LeftAndMainExtension { + use PermissionChecker; + private static $allowed_actions = [ 'ReviewContentForm', 'savereview', @@ -48,7 +50,7 @@ class ContentReviewCMSExtension extends LeftAndMainExtension { $page = $this->findRecord(['ID' => $id]); $user = Security::getCurrentUser(); - if (!$page->canEdit() || ($page->hasMethod('canBeReviewedBy') && !$page->canBeReviewedBy($user))) { + if (!$this->isContentReviewable($page, $user)) { $this->owner->httpError(403, _t( __CLASS__.'.ErrorItemPermissionDenied', 'It seems you don\'t have the necessary permissions to review this content' diff --git a/src/Extensions/SiteTreeContentReview.php b/src/Extensions/SiteTreeContentReview.php index 2bf5672..8d02167 100644 --- a/src/Extensions/SiteTreeContentReview.php +++ b/src/Extensions/SiteTreeContentReview.php @@ -512,11 +512,11 @@ class SiteTreeContentReview extends DataExtension implements PermissionProvider */ public function canBeReviewedBy(Member $member = null) { - if (!$this->owner->obj("NextReviewDate")->exists()) { + if (!$this->owner->obj('NextReviewDate')->exists()) { return false; } - if ($this->owner->obj("NextReviewDate")->InFuture()) { + if ($this->owner->obj('NextReviewDate')->InFuture()) { return false; } @@ -542,6 +542,11 @@ class SiteTreeContentReview extends DataExtension implements PermissionProvider return true; } + // Check whether this user is allowed to review the content of the page. + if ($this->owner->hasMethod("canReviewContent") && !$this->owner->canReviewContent($member)) { + return false; + } + if ($member->inGroups($options->OwnerGroups())) { return true; } diff --git a/src/Forms/ReviewContentHandler.php b/src/Forms/ReviewContentHandler.php index 9ddf58f..408cdcf 100644 --- a/src/Forms/ReviewContentHandler.php +++ b/src/Forms/ReviewContentHandler.php @@ -3,6 +3,7 @@ namespace SilverStripe\ContentReview\Forms; use SilverStripe\ContentReview\Extensions\SiteTreeContentReview; +use SilverStripe\ContentReview\Traits\PermissionChecker; use SilverStripe\Control\Controller; use SilverStripe\Control\Director; use SilverStripe\Control\HTTPResponse; @@ -19,6 +20,7 @@ use SilverStripe\Security\Security; class ReviewContentHandler { use Injectable; + use PermissionChecker; /** * Parent controller for this form @@ -120,12 +122,11 @@ class ReviewContentHandler */ public function canSubmitReview($record) { - if (!$record->canEdit() - || !$record->hasMethod('canBeReviewedBy') - || !$record->canBeReviewedBy(Security::getCurrentUser()) - ) { + // Ensure the parameter of correct data type + if (!$record instanceof DataObject) { return false; } - return true; + + return $this->isContentReviewable($record, Security::getCurrentUser()); } } diff --git a/src/Traits/PermissionChecker.php b/src/Traits/PermissionChecker.php new file mode 100644 index 0000000..0b619b9 --- /dev/null +++ b/src/Traits/PermissionChecker.php @@ -0,0 +1,22 @@ +hasMethod('canReviewContent') + ? $record->canReviewContent($user) + : $record->canEdit(); + } +} diff --git a/tests/php/Extensions/ContentReviewCMSExtensionTest.php b/tests/php/Extensions/ContentReviewCMSExtensionTest.php index d53abd8..a3a0401 100644 --- a/tests/php/Extensions/ContentReviewCMSExtensionTest.php +++ b/tests/php/Extensions/ContentReviewCMSExtensionTest.php @@ -2,6 +2,7 @@ namespace SilverStripe\ContentReview\Tests\Extensions; +use SilverStripe\CMS\Model\SiteTree; use SilverStripe\ContentReview\Extensions\ContentReviewCMSExtension; use SilverStripe\ContentReview\Forms\ReviewContentHandler; use SilverStripe\Control\Controller; @@ -50,7 +51,7 @@ class ContentReviewCMSExtensionTest extends SapphireTest $mock->setOwner(new Controller); // Return a DataObject without the content review extension applied - $mock->expects($this->once())->method('findRecord')->with(['ID' => 123])->willReturn(new Member); + $mock->expects($this->once())->method('findRecord')->with(['ID' => 123])->willReturn(new SiteTree); $mock->getReviewContentForm(123); } diff --git a/tests/php/Forms/ReviewContentHandlerTest.php b/tests/php/Forms/ReviewContentHandlerTest.php index 8b01163..a544562 100644 --- a/tests/php/Forms/ReviewContentHandlerTest.php +++ b/tests/php/Forms/ReviewContentHandlerTest.php @@ -45,6 +45,13 @@ class ReviewContentHandlerTest extends SapphireTest ReviewContentHandler::create()->submitReview(new Member, ['foo' => 'bar']); } + public function testExceptionThrownWhenSubmittingReviewForInvalidDataObject() + { + $this->expectException(ValidationException::class); + $this->expectExceptionMessage('It seems you don\'t have the necessary permissions to submit a content review'); + ReviewContentHandler::create()->submitReview(new Controller, ['foo' => 'bar']); + } + public function testAddReviewNoteCalledWhenSubmittingReview() { $this->logInWithPermission('ADMIN'); diff --git a/tests/php/SiteTreeContentReviewTest.php b/tests/php/SiteTreeContentReviewTest.php index 395947d..63c6e93 100644 --- a/tests/php/SiteTreeContentReviewTest.php +++ b/tests/php/SiteTreeContentReviewTest.php @@ -9,6 +9,7 @@ use SilverStripe\ContentReview\Extensions\ContentReviewCMSExtension; use SilverStripe\ContentReview\Extensions\ContentReviewDefaultSettings; use SilverStripe\ContentReview\Extensions\ContentReviewOwner; use SilverStripe\ContentReview\Extensions\SiteTreeContentReview; +use SilverStripe\Core\Injector\Injector; use SilverStripe\Forms\LiteralField; use SilverStripe\ORM\FieldType\DBDate; use SilverStripe\ORM\FieldType\DBDatetime; @@ -16,6 +17,7 @@ use SilverStripe\Security\Group; use SilverStripe\Security\Member; use SilverStripe\SiteConfig\SiteConfig; use SilverStripe\Versioned\Versioned; +use SilverStripe\ORM\ArrayList; class SiteTreeContentReviewTest extends ContentReviewBaseTest { @@ -365,4 +367,38 @@ class SiteTreeContentReviewTest extends ContentReviewBaseTest DBDatetime::clear_mock_now(); } + + public function testPermissionCheckByOnDataObject() + { + $reviewer = $this->objFromFixture(Member::class, 'editor'); + + // Mock Page class with canReviewContent method to return true on first call and false on second call + $mock = $this->getMockBuilder(Page::class) + ->setMethods(['canReviewContent', 'NextReviewDate', 'OwnerUsers']) + ->getMock(); + $mock->expects($this->exactly(2))->method('canReviewContent')->willReturnOnConsecutiveCalls(false, true); + $mock->method('NextReviewDate')->willReturn('2020-02-20 12:00:00'); + $mock->method('OwnerUsers')->willReturn(ArrayList::create([$reviewer])); + $mock->ContentReviewType = 'Custom'; + + /** @var SiteTreeContentReview $extension */ + $extension = Injector::inst()->get(SiteTreeContentReview::class); + $extension->setOwner($mock); + + // Assert that the user is not allowed to review content + $author = $this->objFromFixture(Member::class, 'author'); + $this->assertFalse($extension->canBeReviewedBy($author)); + + DBDatetime::set_mock_now("2020-03-01 12:00:00"); + + // Assert that the user is allowed to review content + $this->assertTrue($extension->canBeReviewedBy($reviewer)); + + // Assert tht canBeReviewedBy return true if no user logged in + // This is for CLI execution for ContentReviewEmails task + $this->logOut(); + $this->assertTrue($extension->canBeReviewedBy()); + + DBDatetime::clear_mock_now(); + } }