From fb939da8986846213b3f59e2123824fe77209ded Mon Sep 17 00:00:00 2001
From: Toby Zerner <toby.zerner@gmail.com>
Date: Sat, 28 May 2016 09:35:08 +0930
Subject: [PATCH] Fix permission logic priorities
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

This helps to fix a bug in flarum-ext-tags where a user could not rename or edit the tags of their own discussion if it was in a restricted tag. This was due to the order of GetPermission event listeners – the logic that determines that a user *can't* perform an action because of a restrictive tag was running before (and thus instead of) the logic that determines that a user *can* edit their own stuff.

The solution is to change the "catch-all" methods on Policies to "after" instead of "before" – that is, they will run only if the per-ability methods return null.

We also simplify the GetPermission event by passing the model as a sole "argument", as I can't imagine any cases where we'll need more than one argument.
---
 .../core/src/Core/Access/AbstractPolicy.php   | 28 ++++++++---------
 .../core/src/Core/Access/DiscussionPolicy.php |  4 +--
 .../core/src/Core/Access/GroupPolicy.php      |  2 +-
 framework/core/src/Core/Access/PostPolicy.php | 30 +++----------------
 framework/core/src/Core/Access/UserPolicy.php |  2 +-
 .../core/src/Core/CoreServiceProvider.php     |  2 +-
 framework/core/src/Event/GetPermission.php    | 10 +++----
 7 files changed, 27 insertions(+), 51 deletions(-)

diff --git a/framework/core/src/Core/Access/AbstractPolicy.php b/framework/core/src/Core/Access/AbstractPolicy.php
index aa73ea15b..6d0246dfb 100644
--- a/framework/core/src/Core/Access/AbstractPolicy.php
+++ b/framework/core/src/Core/Access/AbstractPolicy.php
@@ -27,6 +27,7 @@ abstract class AbstractPolicy
     public function subscribe(Dispatcher $events)
     {
         $events->listen(GetPermission::class, [$this, 'getPermission']);
+        $events->listen(GetPermission::class, [$this, 'getPermissionAfter'], -100);
         $events->listen(ScopeModelVisibility::class, [$this, 'scopeModelVisibility']);
     }
 
@@ -36,22 +37,19 @@ abstract class AbstractPolicy
      */
     public function getPermission(GetPermission $event)
     {
-        if (isset($event->arguments[0]) && $event->arguments[0] instanceof $this->model) {
-            if (method_exists($this, 'before')) {
-                $arguments = array_merge([$event->actor, $event->ability], $event->arguments);
+        if ($event->model instanceof $this->model && method_exists($this, $event->ability)) {
+            return call_user_func_array([$this, $event->ability], [$event->actor, $event->model]);
+        }
+    }
 
-                $result = call_user_func_array([$this, 'before'], $arguments);
-
-                if (! is_null($result)) {
-                    return $result;
-                }
-            }
-
-            if (method_exists($this, $event->ability)) {
-                $arguments = array_merge([$event->actor], $event->arguments);
-
-                return call_user_func_array([$this, $event->ability], $arguments);
-            }
+    /**
+     * @param GetPermission $event
+     * @return bool|null
+     */
+    public function getPermissionAfter(GetPermission $event)
+    {
+        if ($event->model instanceof $this->model && method_exists($this, 'after')) {
+            return call_user_func_array([$this, 'after'], [$event->actor, $event->ability, $event->model]);
         }
     }
 
diff --git a/framework/core/src/Core/Access/DiscussionPolicy.php b/framework/core/src/Core/Access/DiscussionPolicy.php
index 66bbf5560..c9a5ab975 100644
--- a/framework/core/src/Core/Access/DiscussionPolicy.php
+++ b/framework/core/src/Core/Access/DiscussionPolicy.php
@@ -56,7 +56,7 @@ class DiscussionPolicy extends AbstractPolicy
      * @param string $ability
      * @return bool|null
      */
-    public function before(User $actor, $ability)
+    public function after(User $actor, $ability)
     {
         if ($actor->hasPermission('discussion.'.$ability)) {
             return true;
@@ -107,7 +107,7 @@ class DiscussionPolicy extends AbstractPolicy
      * @param Discussion $discussion
      * @return bool|null
      */
-    public function delete(User $actor, Discussion $discussion)
+    public function hide(User $actor, Discussion $discussion)
     {
         if ($discussion->start_user_id == $actor->id && $discussion->participants_count <= 1) {
             return true;
diff --git a/framework/core/src/Core/Access/GroupPolicy.php b/framework/core/src/Core/Access/GroupPolicy.php
index d01858fa0..44b35cfa9 100644
--- a/framework/core/src/Core/Access/GroupPolicy.php
+++ b/framework/core/src/Core/Access/GroupPolicy.php
@@ -25,7 +25,7 @@ class GroupPolicy extends AbstractPolicy
      * @param string $ability
      * @return bool|null
      */
-    public function before(User $actor, $ability)
+    public function after(User $actor, $ability)
     {
         if ($actor->hasPermission('group.'.$ability)) {
             return true;
diff --git a/framework/core/src/Core/Access/PostPolicy.php b/framework/core/src/Core/Access/PostPolicy.php
index 146c9e618..4d918decc 100644
--- a/framework/core/src/Core/Access/PostPolicy.php
+++ b/framework/core/src/Core/Access/PostPolicy.php
@@ -29,19 +29,12 @@ class PostPolicy extends AbstractPolicy
      */
     protected $settings;
 
-    /**
-     * @var Gate
-     */
-    protected $gate;
-
     /**
      * @param SettingsRepositoryInterface $settings
-     * @param Gate $gate
      */
-    public function __construct(SettingsRepositoryInterface $settings, Gate $gate)
+    public function __construct(SettingsRepositoryInterface $settings)
     {
         $this->settings = $settings;
-        $this->gate = $gate;
     }
 
     /**
@@ -60,9 +53,9 @@ class PostPolicy extends AbstractPolicy
      * @param Post $post
      * @return bool|null
      */
-    public function before(User $actor, $ability, Post $post)
+    public function after(User $actor, $ability, Post $post)
     {
-        if ($this->discussionAllows($actor, $ability, $post)) {
+        if ($actor->can($ability.'Posts', $post->discussion)) {
             return true;
         }
     }
@@ -75,7 +68,7 @@ class PostPolicy extends AbstractPolicy
         // When fetching a discussion's posts: if the user doesn't have permission
         // to moderate the discussion, then they can't see posts that have been
         // hidden by someone other than themself.
-        if ($this->gate->forUser($event->actor)->denies('editPosts', $event->discussion)) {
+        if ($event->actor->cannot('editPosts', $event->discussion)) {
             $event->query->where(function ($query) use ($event) {
                 $query->whereNull('hide_time')
                     ->orWhere('user_id', $event->actor->id);
@@ -90,10 +83,6 @@ class PostPolicy extends AbstractPolicy
      */
     public function edit(User $actor, Post $post)
     {
-        if ($this->discussionAllows($actor, 'edit', $post)) {
-            return true;
-        }
-
         // A post is allowed to be edited if the user has permission to moderate
         // the discussion which it's in, or if they are the author and the post
         // hasn't been deleted by someone else.
@@ -107,15 +96,4 @@ class PostPolicy extends AbstractPolicy
             }
         }
     }
-
-    /**
-     * @param User $actor
-     * @param string $ability
-     * @param Post $post
-     * @return bool
-     */
-    protected function discussionAllows(User $actor, $ability, Post $post)
-    {
-        return $this->gate->forUser($actor)->allows($ability.'Posts', $post->discussion);
-    }
 }
diff --git a/framework/core/src/Core/Access/UserPolicy.php b/framework/core/src/Core/Access/UserPolicy.php
index 1bc0be78e..201eccc0c 100644
--- a/framework/core/src/Core/Access/UserPolicy.php
+++ b/framework/core/src/Core/Access/UserPolicy.php
@@ -25,7 +25,7 @@ class UserPolicy extends AbstractPolicy
      * @param string $ability
      * @return bool|null
      */
-    public function before(User $actor, $ability)
+    public function after(User $actor, $ability)
     {
         if ($actor->hasPermission('user.'.$ability)) {
             return true;
diff --git a/framework/core/src/Core/CoreServiceProvider.php b/framework/core/src/Core/CoreServiceProvider.php
index 2323b2720..478baf97d 100644
--- a/framework/core/src/Core/CoreServiceProvider.php
+++ b/framework/core/src/Core/CoreServiceProvider.php
@@ -77,7 +77,7 @@ class CoreServiceProvider extends AbstractServiceProvider
             // this permission query and explicitly grant or deny the
             // permission.
             $allowed = $this->app->make('events')->until(
-                new GetPermission($actor, $ability, $model ? [$model] : [])
+                new GetPermission($actor, $ability, $model)
             );
 
             if (! is_null($allowed)) {
diff --git a/framework/core/src/Event/GetPermission.php b/framework/core/src/Event/GetPermission.php
index 96a53b9bc..a8ce33881 100644
--- a/framework/core/src/Event/GetPermission.php
+++ b/framework/core/src/Event/GetPermission.php
@@ -25,19 +25,19 @@ class GetPermission
     public $ability;
 
     /**
-     * @var array
+     * @var mixed
      */
-    public $arguments;
+    public $model;
 
     /**
      * @param User $actor
      * @param string $ability
-     * @param array $arguments
+     * @param mixed $model
      */
-    public function __construct(User $actor, $ability, array $arguments)
+    public function __construct(User $actor, $ability, $model)
     {
         $this->actor = $actor;
         $this->ability = $ability;
-        $this->arguments = $arguments;
+        $this->model = $model;
     }
 }