NEW Allow for optional can permission method for content review (#152)

* Allow for optional can permission method for content review

* Content Review permission logic

With the new `canReviewContent()` permission checker we only need to check the permission is set for the user and `canBeReviewedBy()` will always check if the page object is due for review by its owner.

Thus removed redundant logic in `canUseReviewContent()` and accordingly renamed the class filename for additional context.

Co-authored-by: Jared Dreyer <jared.dreyer@silverstripe.com>

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'

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;
         }

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()
+        // Ensure the parameter of correct data type
-            || !$record->hasMethod('canBeReviewedBy')
+        if (!$record instanceof DataObject) {
-            || !$record->canBeReviewedBy(Security::getCurrentUser())
-        ) {
             return false;
         }
-        return true;
+
+        return $this->isContentReviewable($record, Security::getCurrentUser());
     }
 }

src/Traits/PermissionChecker.php

@@ -0,0 +1,22 @@
+<?php
+
+namespace SilverStripe\ContentReview\Traits;
+
+use SilverStripe\CMS\Model\SiteTree;
+use SilverStripe\ORM\DataObject;
+use SilverStripe\Security\Member;
+use SilverStripe\Security\Security;
+
+trait PermissionChecker
+{
+    /**
+     * Checks the user has been granted special permission to review the content of the page
+     * if not fallback to canEdit() permission.
+     */
+    protected function isContentReviewable(DataObject $record, ?Member $user = null): bool
+    {
+        return $record->hasMethod('canReviewContent')
+            ? $record->canReviewContent($user)
+            : $record->canEdit();
+    }
+}

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);
     }

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');

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();
+    }
 }