Overhaul permissions

Get rid of Permissible - too complex and inefficient. Replace with:
- a “Locked” trait which works similarly but only evaluates logic on
hydrated models.
- a “VisibleScope” trait which also works similarly but only scopes
queries

This is all we need, Permissible is overkill. There is only one
instance where we have to duplicate some logic
(Discussion::scopeVisiblePosts and Post::allow(‘view’, …)) but it’s
barely anything.

Haven’t decoupled for now, we can definitely look at doing that later.

Permissions table seeder slightly updated.

Also did a bit of a query audit, there’s still a lot to be done but
it’s much better than it was. Some relatively low-hanging fruit
detailed in EloquentPostRepository.

framework/core/migrations/2015_02_24_000000_create_users_discussions_table.php

@@ -14,7 +14,6 @@ class CreateUsersDiscussionsTable extends Migration
     public function up()
     {
         Schema::create('users_discussions', function (Blueprint $table) {
-        
             $table->integer('user_id')->unsigned();
             $table->integer('discussion_id')->unsigned();
             $table->dateTime('read_time')->nullable();

framework/core/src/Api/Actions/Discussions/ShowAction.php

@@ -93,7 +93,7 @@ class ShowAction extends SerializeResourceAction
 
         $discussion = $this->discussions->findOrFail($request->get('id'), $user);
 
-        $discussion->posts_ids = $discussion->posts()->whereCan($user, 'view')->get(['id'])->fetch('id')->all();
+        $discussion->posts_ids = $discussion->visiblePosts($user)->lists('id');
 
         if (in_array('posts', $request->include)) {
             $length = strlen($prefix = 'posts.');

framework/core/src/Api/Serializers/BaseSerializer.php

@@ -55,16 +55,7 @@ abstract class BaseSerializer extends SerializerAbstract
                 $data = $relation($model, $include);
             } else {
                 if ($include) {
-                    if (! is_null($model->$relation)) {
-                        $data = $model->$relation;
-                    } else {
-                        $relation = $model->$relation();
-                        if ($relation instanceof Relation) {
-                            $data = $relation->getResults();
-                        } else {
-                            $data = $relation->get();
-                        }
-                    }
+                    $data = $model->getRelation($relation);
                 } elseif ($many) {
                     $relationIds = $relation.'_ids';
                     $data = $model->$relationIds ?: $model->$relation()->get(['id'])->fetch('id')->all();

framework/core/src/Core/CoreServiceProvider.php

@@ -185,71 +185,78 @@ class CoreServiceProvider extends ServiceProvider
         $this->extend(
             new Permission('forum.view'),
             new Permission('forum.startDiscussion'),
-            new Permission('discussion.rename'),
-            new Permission('discussion.delete'),
             new Permission('discussion.reply'),
-            new Permission('post.edit'),
-            new Permission('post.delete')
+            new Permission('discussion.editPosts'),
+            new Permission('discussion.deletePosts'),
+            new Permission('discussion.rename'),
+            new Permission('discussion.delete')
         );
 
-        Forum::grantPermission(function ($grant, $user, $permission) {
-            return $user->hasPermission('forum.'.$permission);
+        Forum::allow('*', function ($forum, $user, $action) {
+            if ($user->hasPermission('forum.'.$action)) {
+                return true;
+            }
         });
 
-        Post::grantPermission(function ($grant, $user, $permission) {
-            return $user->hasPermission('post'.$permission);
+        Post::allow('*', function ($post, $user, $action) {
+            if ($user->hasPermission('post.'.$action)) {
+                return true;
+            }
         });
 
-        // Grant view access to a post only if the user can also view the
-        // discussion which the post is in. Also, the if the post is hidden,
-        // the user must have edit permissions too.
-        Post::grantPermission('view', function ($grant) {
-            $grant->whereCan('view', 'discussion');
-        });
-
-        Post::demandPermission('view', function ($demand) {
-            $demand->whereNull('hide_user_id')
-                   ->orWhereCan('edit');
-        });
-
-        // Allow a user to edit their own post, unless it has been hidden by
-        // someone else.
-        Post::grantPermission('edit', function ($grant, $user) {
-            $grant->where('user_id', $user->id)
-                  ->where(function ($query) use ($user) {
+        // 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.
+        Discussion::scopeVisiblePosts(function ($query, User $user, Discussion $discussion) {
+            if (! $discussion->can($user, 'editPosts')) {
+                $query->where(function ($query) use ($user) {
                     $query->whereNull('hide_user_id')
                           ->orWhere('hide_user_id', $user->id);
-                  });
-            // @todo add limitations to time etc. according to a config setting
+                });
+            }
         });
 
-        User::grantPermission(function ($grant, $user, $permission) {
-            return $user->hasPermission('user.'.$permission);
+        Post::allow('view', function ($post, $user) {
+            if (! $post->hide_user_id || $post->can($user, 'edit')) {
+                return true;
+            }
         });
 
-        // Grant view access to a user if the user can view the forum.
-        User::grantPermission('view', function ($grant, $user) {
-            $grant->whereCan('view', 'forum');
+        // 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.
+        Post::allow('edit', function ($post, $user) {
+            if ($post->discussion->can($user, 'editPosts') ||
+                ($post->user_id == $user->id && (! $post->hide_user_id || $post->hide_user_id == $user->id))
+            ) {
+                return true;
+            }
         });
 
-        // Allow a user to edit their own account.
-        User::grantPermission(['edit', 'delete'], function ($grant, $user) {
-            $grant->where('id', $user->id);
+        User::allow('*', function ($discussion, $user, $action) {
+            if ($user->hasPermission('user.'.$action)) {
+                return true;
+            }
         });
 
-        Discussion::grantPermission(function ($grant, $user, $permission) {
-            return $user->hasPermission('discussion.'.$permission);
+        User::allow(['edit', 'delete'], function ($user, $actor) {
+            if ($user->id == $actor->id) {
+                return true;
+            }
         });
 
-        // Grant view access to a discussion if the user can view the forum.
-        Discussion::grantPermission('view', function ($grant, $user) {
-            $grant->whereCan('view', 'forum');
+        Discussion::allow('*', function ($discussion, $user, $action) {
+            if ($user->hasPermission('discussion.'.$action)) {
+                return true;
+            }
         });
 
         // Allow a user to rename their own discussion.
-        Discussion::grantPermission('rename', function ($grant, $user) {
-            $grant->where('start_user_id', $user->id);
-            // @todo add limitations to time etc. according to a config setting
+        Discussion::allow('rename', function ($discussion, $user) {
+            if ($discussion->start_user_id == $user->id) {
+                return true;
+                // @todo add limitations to time etc. according to a config setting
+            }
         });
     }
 }

framework/core/src/Core/Models/Discussion.php

@@ -1,7 +1,8 @@
 <?php namespace Flarum\Core\Models;
 
-use Tobscure\Permissible\Permissible;
 use Flarum\Core\Support\EventGenerator;
+use Flarum\Core\Support\Locked;
+use Flarum\Core\Support\VisibleScope;
 use Flarum\Core\Events\DiscussionWasDeleted;
 use Flarum\Core\Events\DiscussionWasStarted;
 use Flarum\Core\Events\DiscussionWasRenamed;
@@ -9,7 +10,8 @@ use Flarum\Core\Models\User;
 
 class Discussion extends Model
 {
-    use Permissible;
+    use Locked;
+    use VisibleScope;
 
     /**
      * The validation rules for this model.
@@ -215,6 +217,24 @@ class Discussion extends Model
         return $this->hasMany('Flarum\Core\Models\Post');
     }
 
+    protected static $visiblePostsScopes = [];
+
+    public static function scopeVisiblePosts($scope)
+    {
+        static::$visiblePostsScopes[] = $scope;
+    }
+
+    public function visiblePosts(User $user)
+    {
+        $query = $this->posts();
+
+        foreach (static::$visiblePostsScopes as $scope) {
+            $scope($query, $user, $this);
+        }
+
+        return $query;
+    }
+
     /**
      * Define the relationship with the discussion's comments.
      *
@@ -297,9 +317,8 @@ class Discussion extends Model
      */
     public function stateFor(User $user)
     {
-        $loadedState = array_get($this->relations, 'state');
-        if ($loadedState && $loadedState->user_id === $user->id) {
-            return $loadedState;
+        if ($this->isRelationLoaded('state')) {
+            return $this->relations['state'];
         }
 
         $state = $this->state($user)->first();

framework/core/src/Core/Models/Forum.php

@@ -1,11 +1,11 @@
 <?php namespace Flarum\Core\Models;
 
-use Tobscure\Permissible\Permissible;
+use Flarum\Core\Support\Locked;
 use Flarum\Core;
 
 class Forum extends Model
 {
-    use Permissible;
+    use Locked;
 
     protected static $relationships = [];
 

framework/core/src/Core/Models/Model.php

@@ -169,37 +169,22 @@ class Model extends Eloquent
         return $rules;
     }
 
-    /**
-     * Assert that the user has permission to view this model, throwing an
-     * exception if they don't.
-     *
-     * @param  \Flarum\Core\Models\User  $user
-     * @return void
-     *
-     * @throws  \Illuminate\Database\Eloquent\ModelNotFoundException
-     */
-    public function assertVisibleTo(User $user)
+    public function isRelationLoaded($relation)
     {
-        if (! $this->can($user, 'view')) {
-            throw new ModelNotFoundException;
-        }
+        return array_key_exists($relation, $this->relations);
     }
 
-    /**
-     * Assert that the user has a certain permission for this model, throwing
-     * an exception if they don't.
-     *
-     * @param  \Flarum\Core\Models\User  $user
-     * @param  string  $permission
-     * @return void
-     *
-     * @throws  \Flarum\Core\Exceptions\PermissionDeniedException
-     */
-    public function assertCan(User $user, $permission)
+    public function getRelation($relation)
     {
-        if (! $this->can($user, $permission)) {
-            throw new PermissionDeniedException;
+        if (isset($this->$relation)) {
+            return $this->$relation;
         }
+
+        if (! $this->isRelationLoaded($relation)) {
+            $this->relations[$relation] = $this->$relation()->getResults();
+        }
+
+        return $this->relations[$relation];
     }
 
     /**

framework/core/src/Core/Models/Post.php

@@ -1,11 +1,11 @@
 <?php namespace Flarum\Core\Models;
 
-use Tobscure\Permissible\Permissible;
 use Flarum\Core\Events\PostWasDeleted;
+use Flarum\Core\Support\Locked;
 
 class Post extends Model
 {
-    use Permissible;
+    use Locked;
 
     /**
      * The validation rules for this model.

framework/core/src/Core/Models/User.php

@@ -1,7 +1,6 @@
 <?php namespace Flarum\Core\Models;
 
 use Illuminate\Contracts\Hashing\Hasher;
-use Tobscure\Permissible\Permissible;
 use Flarum\Core\Formatter\FormatterManager;
 use Flarum\Core\Exceptions\InvalidConfirmationTokenException;
 use Flarum\Core\Events\UserWasDeleted;
@@ -14,10 +13,13 @@ use Flarum\Core\Events\UserAvatarWasChanged;
 use Flarum\Core\Events\UserWasActivated;
 use Flarum\Core\Events\UserEmailWasConfirmed;
 use Flarum\Core\Events\UserEmailChangeWasRequested;
+use Flarum\Core\Support\Locked;
+use Flarum\Core\Support\VisibleScope;
 
 class User extends Model
 {
-    use Permissible;
+    use Locked;
+    use VisibleScope;
 
     /**
      * The text formatter instance.

framework/core/src/Core/Repositories/EloquentDiscussionRepository.php

@@ -30,7 +30,7 @@ class EloquentDiscussionRepository implements DiscussionRepositoryInterface
     {
         $query = Discussion::where('id', $id);
 
-        return $this->scopeVisibleForUser($query, $user)->firstOrFail();
+        return $this->scopeVisibleTo($query, $user)->firstOrFail();
     }
 
     /**
@@ -54,10 +54,10 @@ class EloquentDiscussionRepository implements DiscussionRepositoryInterface
      * @param  \Flarum\Core\Models\User  $user
      * @return \Illuminate\Database\Eloquent\Builder
      */
-    protected function scopeVisibleForUser(Builder $query, User $user = null)
+    protected function scopeVisibleTo(Builder $query, User $user = null)
     {
         if ($user !== null) {
-            $query->whereCan($user, 'view');
+            $query->whereVisibleTo($user);
         }
 
         return $query;

framework/core/src/Core/Repositories/EloquentPostRepository.php

@@ -1,10 +1,32 @@
 <?php namespace Flarum\Core\Repositories;
 
 use Illuminate\Database\Eloquent\Builder;
+use Illuminate\Database\Eloquent\ModelNotFoundException;
 use Flarum\Core\Models\Post;
 use Flarum\Core\Models\User;
+use Flarum\Core\Models\Discussion;
 use Flarum\Core\Search\Discussions\Fulltext\DriverInterface;
 
+// TODO: In some cases, the use of a post repository incurs extra query expense,
+// because for every post retrieved we need to check if the discussion it's in
+// is visible. Especially when retrieving a discussion's posts, we can end up
+// with an inefficient chain of queries like this:
+// 1. Api\Discussions\ShowAction: get discussion (will exit if not visible)
+// 2. Discussion@visiblePosts: get discussion tags (for post visibility purposes)
+// 3. Discussion@visiblePosts: get post IDs
+// 4. EloquentPostRepository@getIndexForNumber: get discussion
+// 5. EloquentPostRepository@getIndexForNumber: get discussion tags (for post visibility purposes)
+// 6. EloquentPostRepository@getIndexForNumber: get post index for number
+// 7. EloquentPostRepository@findWhere: get post IDs for discussion to check for discussion visibility
+// 8. EloquentPostRepository@findWhere: get post IDs in visible discussions
+// 9. EloquentPostRepository@findWhere: get posts
+// 10. EloquentPostRepository@findWhere: eager load discussion onto posts
+// 11. EloquentPostRepository@findWhere: get discussion tags to filter visible posts
+// 12. Api\Discussions\ShowAction: eager load users
+// 13. Api\Discussions\ShowAction: eager load groups
+// 14. Api\Discussions\ShowAction: eager load mentions
+// 14. Serializers\DiscussionSerializer: load discussion-user state
+
 class EloquentPostRepository implements PostRepositoryInterface
 {
     protected $fulltext;
@@ -26,9 +48,13 @@ class EloquentPostRepository implements PostRepositoryInterface
      */
     public function findOrFail($id, User $user = null)
     {
-        $query = Post::where('id', $id);
+        $posts = $this->findByIds([$id], $user);
 
-        return $this->scopeVisibleForUser($query, $user)->firstOrFail();
+        if (! count($posts)) {
+            throw new ModelNotFoundException;
+        }
+
+        return $posts->first();
     }
 
     /**
@@ -52,7 +78,9 @@ class EloquentPostRepository implements PostRepositoryInterface
             $query->orderBy($field, $order);
         }
 
-        return $this->scopeVisibleForUser($query, $user)->get();
+        $ids = $query->lists('id');
+
+        return $this->findByIds($ids, $user);
     }
 
     /**
@@ -65,9 +93,11 @@ class EloquentPostRepository implements PostRepositoryInterface
      */
     public function findByIds(array $ids, User $user = null)
     {
-        $query = Post::whereIn('id', (array) $ids);
+        $ids = $this->filterDiscussionVisibleTo($ids, $user);
 
-        return $this->scopeVisibleForUser($query, $user)->get();
+        $posts = Post::with('discussion')->whereIn('id', (array) $ids)->get();
+
+        return $this->filterVisibleTo($posts, $user);
     }
 
     /**
@@ -82,13 +112,17 @@ class EloquentPostRepository implements PostRepositoryInterface
     {
         $ids = $this->fulltext->match($string);
 
+        $ids = $this->filterDiscussionVisibleTo($ids, $user);
+
         $query = Post::select('id', 'discussion_id')->whereIn('id', $ids);
 
         foreach ($ids as $id) {
             $query->orderByRaw('id != ?', [$id]);
         }
 
-        return $this->scopeVisibleForUser($query, $user)->get();
+        $posts = $query->get();
+
+        return $this->filterVisibleTo($posts, $user);
     }
 
     /**
@@ -103,7 +137,8 @@ class EloquentPostRepository implements PostRepositoryInterface
      */
     public function getIndexForNumber($discussionId, $number, User $user = null)
     {
-        $query = Post::where('discussion_id', $discussionId)
+        $query = Discussion::find($discussionId)
+            ->visiblePosts($user)
             ->where('time', '<', function ($query) use ($discussionId, $number) {
                 $query->select('time')
                       ->from('posts')
@@ -116,22 +151,32 @@ class EloquentPostRepository implements PostRepositoryInterface
                       ->orderByRaw('ABS(CAST(number AS SIGNED) - '.(int) $number.')');
             });
 
-        return $this->scopeVisibleForUser($query, $user)->count();
+        return $query->count();
     }
 
-    /**
-     * Scope a query to only include records that are visible to a user.
-     *
-     * @param  \Illuminate\Database\Eloquent\Builder  $query
-     * @param  \Flarum\Core\Models\User  $user
-     * @return \Illuminate\Database\Eloquent\Builder
-     */
-    protected function scopeVisibleForUser(Builder $query, User $user = null)
+    protected function filterDiscussionVisibleTo($ids, $user)
     {
-        if ($user !== null) {
-            $query->whereCan($user, 'view');
+        // For each post ID, we need to make sure that the discussion it's in
+        // is visible to the user.
+        if ($user) {
+            $ids = Discussion::join('posts', 'discussions.id', '=', 'posts.discussion_id')
+                ->whereIn('posts.id', $ids)
+                ->whereVisibleTo($user)
+                ->get(['posts.id'])
+                ->lists('id');
         }
 
-        return $query;
+        return $ids;
+    }
+
+    protected function filterVisibleTo($posts, $user)
+    {
+        if ($user) {
+            $posts = $posts->filter(function ($post) use ($user) {
+                return $post->can($user, 'view');
+            });
+        }
+
+        return $posts;
     }
 }

framework/core/src/Core/Repositories/EloquentUserRepository.php

@@ -29,7 +29,7 @@ class EloquentUserRepository implements UserRepositoryInterface
     {
         $query = User::where('id', $id);
 
-        return $this->scopeVisibleForUser($query, $user)->firstOrFail();
+        return $this->scopeVisibleTo($query, $user)->firstOrFail();
     }
 
     /**
@@ -67,7 +67,7 @@ class EloquentUserRepository implements UserRepositoryInterface
     {
         $query = User::where('username', 'like', $username);
 
-        return $this->scopeVisibleForUser($query, $user)->pluck('id');
+        return $this->scopeVisibleTo($query, $user)->pluck('id');
     }
 
     /**
@@ -85,7 +85,7 @@ class EloquentUserRepository implements UserRepositoryInterface
             ->orderByRaw('username = ? desc', [$string])
             ->orderByRaw('username like ? desc', [$string.'%']);
 
-        return $this->scopeVisibleForUser($query, $user)->lists('id');
+        return $this->scopeVisibleTo($query, $user)->lists('id');
     }
 
     /**
@@ -95,10 +95,10 @@ class EloquentUserRepository implements UserRepositoryInterface
      * @param  \Flarum\Core\Models\User  $user
      * @return \Illuminate\Database\Eloquent\Builder
      */
-    protected function scopeVisibleForUser(Builder $query, User $user = null)
+    protected function scopeVisibleTo(Builder $query, User $user = null)
     {
         if ($user !== null) {
-            $query->whereCan($user, 'view');
+            $query->whereVisibleTo($user);
         }
 
         return $query;

framework/core/src/Core/Search/Discussions/DiscussionSearcher.php

@@ -59,7 +59,7 @@ class DiscussionSearcher implements SearcherInterface
     public function search(DiscussionSearchCriteria $criteria, $limit = null, $offset = 0, $load = [])
     {
         $this->user = $criteria->user;
-        $this->query = $this->discussions->query()->whereCan($criteria->user, 'view');
+        $this->query = $this->discussions->query()->whereVisibleTo($criteria->user);
 
         $this->gambits->apply($criteria->query, $this);
 

framework/core/src/Core/Seeders/PermissionsTableSeeder.php

@@ -27,8 +27,8 @@ class PermissionsTableSeeder extends Seeder
             // Moderators can edit + delete stuff and suspend users
             [4, 'discussion.delete'],
             [4, 'discussion.rename'],
-            [4, 'post.delete'],
-            [4, 'post.edit'],
+            [4, 'discussion.editPosts'],
+            [4, 'discussion.deletePosts'],
             [4, 'user.suspend'],
 
         ];

framework/core/src/Core/Support/Locked.php

@@ -0,0 +1,57 @@
+<?php namespace Flarum\Core\Support;
+
+use Flarum\Core\Exceptions\PermissionDeniedException;
+use Flarum\Core\Models\User;
+use Closure;
+
+trait Locked
+{
+    protected static $conditions = [];
+
+    protected static function getConditions($action)
+    {
+        $conditions = isset(static::$conditions[$action]) ? static::$conditions[$action] : [];
+        $all = isset(static::$conditions['*']) ? static::$conditions['*'] : [];
+
+        return array_merge($conditions, $all);
+    }
+
+    public static function allow($action, Closure $condition)
+    {
+        foreach ((array) $action as $action) {
+            if (! isset(static::$conditions[$action])) {
+                static::$conditions[$action] = [];
+            }
+
+            static::$conditions[$action][] = $condition;
+        }
+    }
+
+    public function can(User $user, $action)
+    {
+        foreach ($this->getConditions($action) as $condition) {
+            $can = $condition($this, $user, $action);
+
+            if ($can !== null) {
+                return $can;
+            }
+        }
+    }
+
+    /**
+     * Assert that the user has a certain permission for this model, throwing
+     * an exception if they don't.
+     *
+     * @param \Flarum\Core\Models\User $user
+     * @param string $permission
+     * @return void
+     *
+     * @throws \Flarum\Core\Exceptions\PermissionDeniedException
+     */
+    public function assertCan(User $user, $action)
+    {
+        if (! $this->can($user, $action)) {
+            throw new PermissionDeniedException;
+        }
+    }
+}

framework/core/src/Core/Support/VisibleScope.php

@@ -0,0 +1,20 @@
+<?php namespace Flarum\Core\Support;
+
+use Flarum\Core\Models\User;
+
+trait VisibleScope
+{
+    protected static $visibleScopes = [];
+
+    public static function scopeVisible($scope)
+    {
+        static::$visibleScopes[] = $scope;
+    }
+
+    public function scopeWhereVisibleTo($query, User $user)
+    {
+        foreach (static::$visibleScopes as $scope) {
+            $scope($query, $user);
+        }
+    }
+}