From 04ddb6b20698b762dd5a0333e132adfad56119d3 Mon Sep 17 00:00:00 2001
From: Jared Dreyer <jared.dreyer@silverstripe.com>
Date: Tue, 21 Jun 2022 17:41:20 +1200
Subject: [PATCH] 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.
---
 src/Extensions/ContentReviewCMSExtension.php |  6 ++---
 src/Extensions/SiteTreeContentReview.php     |  6 ++---
 src/Forms/ReviewContentHandler.php           |  6 ++---
 src/Traits/PermissionChecker.php             | 22 ++++++++++++++++
 src/Traits/ReviewPermission.php              | 27 --------------------
 5 files changed, 31 insertions(+), 36 deletions(-)
 create mode 100644 src/Traits/PermissionChecker.php
 delete mode 100644 src/Traits/ReviewPermission.php

diff --git a/src/Extensions/ContentReviewCMSExtension.php b/src/Extensions/ContentReviewCMSExtension.php
index 3885c96..3768819 100644
--- a/src/Extensions/ContentReviewCMSExtension.php
+++ b/src/Extensions/ContentReviewCMSExtension.php
@@ -6,7 +6,7 @@ use SilverStripe\Admin\LeftAndMain;
 use SilverStripe\Admin\LeftAndMainExtension;
 use SilverStripe\CMS\Model\SiteTree;
 use SilverStripe\ContentReview\Forms\ReviewContentHandler;
-use SilverStripe\ContentReview\Traits\ReviewPermission;
+use SilverStripe\ContentReview\Traits\PermissionChecker;
 use SilverStripe\Control\HTTPRequest;
 use SilverStripe\Control\HTTPResponse;
 use SilverStripe\Control\HTTPResponse_Exception;
@@ -20,7 +20,7 @@ use SilverStripe\Security\Security;
  */
 class ContentReviewCMSExtension extends LeftAndMainExtension
 {
-    use ReviewPermission;
+    use PermissionChecker;
 
     private static $allowed_actions = [
         'ReviewContentForm',
@@ -50,7 +50,7 @@ class ContentReviewCMSExtension extends LeftAndMainExtension
     {
         $page = $this->findRecord(['ID' => $id]);
         $user = Security::getCurrentUser();
-        if (!$this->canUseReviewContent($page, $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 359e2df..c1b3ab1 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,7 +542,7 @@ class SiteTreeContentReview extends DataExtension implements PermissionProvider
             return true;
         }
 
-        // Whether or not a user is allowed to review the content of the page.
+        // Check whether this user is allowed to review the content of the page.
         if ($this->owner->hasMethod("canReviewContent") && !$this->owner->canReviewContent($member)) {
             return false;
         }
diff --git a/src/Forms/ReviewContentHandler.php b/src/Forms/ReviewContentHandler.php
index b072e3e..408cdcf 100644
--- a/src/Forms/ReviewContentHandler.php
+++ b/src/Forms/ReviewContentHandler.php
@@ -3,7 +3,7 @@
 namespace SilverStripe\ContentReview\Forms;
 
 use SilverStripe\ContentReview\Extensions\SiteTreeContentReview;
-use SilverStripe\ContentReview\Traits\ReviewPermission;
+use SilverStripe\ContentReview\Traits\PermissionChecker;
 use SilverStripe\Control\Controller;
 use SilverStripe\Control\Director;
 use SilverStripe\Control\HTTPResponse;
@@ -20,7 +20,7 @@ use SilverStripe\Security\Security;
 class ReviewContentHandler
 {
     use Injectable;
-    use ReviewPermission;
+    use PermissionChecker;
 
     /**
      * Parent controller for this form
@@ -127,6 +127,6 @@ class ReviewContentHandler
             return false;
         }
 
-        return $this->canUseReviewContent($record, Security::getCurrentUser());
+        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 @@
+<?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();
+    }
+}
diff --git a/src/Traits/ReviewPermission.php b/src/Traits/ReviewPermission.php
deleted file mode 100644
index 82365ba..0000000
--- a/src/Traits/ReviewPermission.php
+++ /dev/null
@@ -1,27 +0,0 @@
-<?php
-
-namespace SilverStripe\ContentReview\Traits;
-
-use SilverStripe\CMS\Model\SiteTree;
-use SilverStripe\ORM\DataObject;
-use SilverStripe\Security\Member;
-use SilverStripe\Security\Security;
-
-trait ReviewPermission
-{
-    /**
-     * Whether or not a user is allowed use content review
-     */
-    protected function canUseReviewContent(DataObject $record, ?Member $user = null): bool
-    {
-        // Whether or not the user is a reviewer. User must be allowed to view the page
-        $isReviewer = $record->canView($user) &&
-            $record->hasMethod('canBeReviewedBy') &&
-            $record->canBeReviewedBy($user);
-        // Whether or not the user is allowed to review the content of the page
-        // Fallback to canEdit as it the original implementation
-        $canEdit = $record->hasMethod('canReviewContent') ? $record->canReviewContent($user) : $record->canEdit();
-
-        return $canEdit || $isReviewer;
-    }
-}