From 89e821e70f4c7c9dc879717c7478cab819b1135a Mon Sep 17 00:00:00 2001
From: Alexander Skvortsov <38059171+askvortsov1@users.noreply.github.com>
Date: Mon, 18 Jan 2021 18:28:48 -0500
Subject: [PATCH] Policies: treat `true` as `allow`, and `false` as `deny`
 (#2534)

---
 src/User/Access/AbstractPolicy.php     | 30 ++++++++++++--
 tests/unit/User/AbstractPolicyTest.php | 54 +++++++++++++++++++-------
 2 files changed, 66 insertions(+), 18 deletions(-)

diff --git a/src/User/Access/AbstractPolicy.php b/src/User/Access/AbstractPolicy.php
index ef32c539f..3b4579d74 100644
--- a/src/User/Access/AbstractPolicy.php
+++ b/src/User/Access/AbstractPolicy.php
@@ -43,13 +43,13 @@ abstract class AbstractPolicy
      * @param User $user
      * @param string $ability
      * @param $instance
-     * @return bool|void
+     * @return string|void
      */
     public function checkAbility(User $actor, string $ability, $instance)
     { // If a specific method for this ability is defined,
         // call that and return any non-null results
         if (method_exists($this, $ability)) {
-            $result = call_user_func_array([$this, $ability], [$actor, $instance]);
+            $result = $this->sanitizeResult(call_user_func_array([$this, $ability], [$actor, $instance]));
 
             if (! is_null($result)) {
                 return $result;
@@ -58,7 +58,31 @@ abstract class AbstractPolicy
 
         // If a "total access" method is defined, try that.
         if (method_exists($this, 'can')) {
-            return call_user_func_array([$this, 'can'], [$actor, $ability, $instance]);
+            return $this->sanitizeResult(call_user_func_array([$this, 'can'], [$actor, $ability, $instance]));
         }
     }
+
+    /**
+     * Allows `true` to be used in place of `->allow()`, and `false` instead of `->deny()`
+     * This allows more concise and intuitive code, by returning boolean statements:.
+     *
+     * WITHOUT THIS:
+     * `return SOME_BOOLEAN_LOGIC ? $this->allow() : $this->deny();
+     *
+     * WITH THIS:
+     * `return SOME_BOOLEAN_LOGIC;
+     *
+     * @param mixed $result
+     * @return string|void
+     */
+    public function sanitizeResult($result)
+    {
+        if ($result === true) {
+            return $this->allow();
+        } elseif ($result === false) {
+            return $this->deny();
+        }
+
+        return $result;
+    }
 }
diff --git a/tests/unit/User/AbstractPolicyTest.php b/tests/unit/User/AbstractPolicyTest.php
index 0a32eed11..db573343c 100644
--- a/tests/unit/User/AbstractPolicyTest.php
+++ b/tests/unit/User/AbstractPolicyTest.php
@@ -9,9 +9,8 @@
 
 namespace Flarum\Tests\unit\User;
 
-use Flarum\Event\GetPermission;
 use Flarum\Tests\unit\TestCase;
-use Flarum\User\AbstractPolicy;
+use Flarum\User\Access\AbstractPolicy;
 use Flarum\User\User;
 use Illuminate\Events\Dispatcher;
 use Mockery as m;
@@ -19,7 +18,6 @@ use Mockery as m;
 class AbstractPolicyTest extends TestCase
 {
     private $policy;
-    private $dispatcher;
 
     /**
      * @inheritDoc
@@ -27,27 +25,43 @@ class AbstractPolicyTest extends TestCase
     protected function setUp(): void
     {
         $this->policy = m::mock(CustomUserPolicy::class)->makePartial();
-        $this->dispatcher = new Dispatcher();
-        $this->dispatcher->subscribe($this->policy);
-        User::setEventDispatcher($this->dispatcher);
+        User::setEventDispatcher(new Dispatcher());
+    }
+
+    /**
+     * @inheritDoc
+     */
+    protected function tearDown(): void
+    {
+        m::close();
     }
 
     public function test_policy_can_be_called_with_object()
     {
-        $this->policy->shouldReceive('edit')->andReturn(true);
+        $allowed = $this->policy->checkAbility(new User(), 'create', new User());
 
-        $allowed = $this->dispatcher->until(new GetPermission(new User(), 'edit', new User()));
-
-        $this->assertTrue($allowed);
+        $this->assertEquals(AbstractPolicy::ALLOW, $allowed);
     }
 
     public function test_policy_can_be_called_with_class()
     {
-        $this->policy->shouldReceive('create')->andReturn(true);
+        $allowed = $this->policy->checkAbility(new User(), 'edit', User::class);
 
-        $allowed = $this->dispatcher->until(new GetPermission(new User(), 'create', User::class));
+        $this->assertEquals(AbstractPolicy::DENY, $allowed);
+    }
 
-        $this->assertTrue($allowed);
+    public function test_policy_converts_true_to_ALLOW()
+    {
+        $allowed = $this->policy->checkAbility(new User(), 'somethingRandom', User::class);
+
+        $this->assertEquals(AbstractPolicy::ALLOW, $allowed);
+    }
+
+    public function test_policy_converts_false_to_DENY()
+    {
+        $allowed = $this->policy->checkAbility(new User(), 'somethingElseRandom', User::class);
+
+        $this->assertEquals(AbstractPolicy::DENY, $allowed);
     }
 }
 
@@ -57,11 +71,21 @@ class CustomUserPolicy extends AbstractPolicy
 
     public function create(User $actor)
     {
-        return true;
+        return $this->allow();
     }
 
-    public function edit(User $actor, User $user)
+    public function edit(User $actor, $target)
+    {
+        return $this->deny();
+    }
+
+    public function somethingRandom(User $actor, $target)
     {
         return true;
     }
+
+    public function somethingElseRandom(User $actor, $target)
+    {
+        return false;
+    }
 }