Policies: treat true as allow, and false as deny (#2534)

flarum · Jan 18, 2021 · 89e821e · 89e821e
1 parent 9b2d785
commit 89e821e
Show file tree

Hide file tree

Showing 2 changed files with 66 additions and 18 deletions.
diff --git a/src/User/Access/AbstractPolicy.php b/src/User/Access/AbstractPolicy.php
@@ -43,13 +43,13 @@ protected function forceDeny()
      * @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 @@ public function checkAbility(User $actor, string $ability, $instance)
 
         // 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
@@ -9,45 +9,59 @@
 
 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;
 
 class AbstractPolicyTest extends TestCase
 {
     private $policy;
-    private $dispatcher;
 
     /**
      * @inheritDoc
      */
     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());
     }
 
-    public function test_policy_can_be_called_with_object()
+    /**
+     * @inheritDoc
+     */
+    protected function tearDown(): void
     {
-        $this->policy->shouldReceive('edit')->andReturn(true);
+        m::close();
+    }
 
-        $allowed = $this->dispatcher->until(new GetPermission(new User(), 'edit', new User()));
+    public function test_policy_can_be_called_with_object()
+    {
+        $allowed = $this->policy->checkAbility(new User(), 'create', 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, $target)
+    {
+        return $this->deny();
     }
 
-    public function edit(User $actor, User $user)
+    public function somethingRandom(User $actor, $target)
     {
         return true;
     }
+
+    public function somethingElseRandom(User $actor, $target)
+    {
+        return false;
+    }
 }