From f0df751465d0afcd7202da71487aba45fd1b1e96 Mon Sep 17 00:00:00 2001 From: Toby Zerner Date: Tue, 16 Jun 2015 17:33:56 +0930 Subject: [PATCH] Overhaul permissions MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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. --- ..._000000_create_users_discussions_table.php | 1 - src/Api/Actions/Discussions/ShowAction.php | 2 +- src/Api/Serializers/BaseSerializer.php | 11 +-- src/Core/CoreServiceProvider.php | 93 ++++++++++--------- src/Core/Models/Discussion.php | 29 +++++- src/Core/Models/Forum.php | 4 +- src/Core/Models/Model.php | 37 +++----- src/Core/Models/Post.php | 4 +- src/Core/Models/User.php | 6 +- .../EloquentDiscussionRepository.php | 6 +- .../Repositories/EloquentPostRepository.php | 83 +++++++++++++---- .../Repositories/EloquentUserRepository.php | 10 +- .../Search/Discussions/DiscussionSearcher.php | 2 +- src/Core/Seeders/PermissionsTableSeeder.php | 4 +- src/Core/Support/Locked.php | 57 ++++++++++++ src/Core/Support/VisibleScope.php | 20 ++++ 16 files changed, 247 insertions(+), 122 deletions(-) create mode 100644 src/Core/Support/Locked.php create mode 100644 src/Core/Support/VisibleScope.php diff --git a/migrations/2015_02_24_000000_create_users_discussions_table.php b/migrations/2015_02_24_000000_create_users_discussions_table.php index bdd5bfea5..33bf57e5b 100644 --- a/migrations/2015_02_24_000000_create_users_discussions_table.php +++ b/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(); diff --git a/src/Api/Actions/Discussions/ShowAction.php b/src/Api/Actions/Discussions/ShowAction.php index 3c59872d7..1fa0f48a3 100644 --- a/src/Api/Actions/Discussions/ShowAction.php +++ b/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.'); diff --git a/src/Api/Serializers/BaseSerializer.php b/src/Api/Serializers/BaseSerializer.php index d34e44b08..a5483d678 100644 --- a/src/Api/Serializers/BaseSerializer.php +++ b/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(); diff --git a/src/Core/CoreServiceProvider.php b/src/Core/CoreServiceProvider.php index 5ff1ec549..dcd4fe216 100644 --- a/src/Core/CoreServiceProvider.php +++ b/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 + } }); } } diff --git a/src/Core/Models/Discussion.php b/src/Core/Models/Discussion.php index d36ec93e4..7718644ec 100755 --- a/src/Core/Models/Discussion.php +++ b/src/Core/Models/Discussion.php @@ -1,7 +1,8 @@ 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(); diff --git a/src/Core/Models/Forum.php b/src/Core/Models/Forum.php index 078af04d7..5980317f1 100755 --- a/src/Core/Models/Forum.php +++ b/src/Core/Models/Forum.php @@ -1,11 +1,11 @@ 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]; } /** diff --git a/src/Core/Models/Post.php b/src/Core/Models/Post.php index 5fb1ade7f..baaef5045 100755 --- a/src/Core/Models/Post.php +++ b/src/Core/Models/Post.php @@ -1,11 +1,11 @@ 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; diff --git a/src/Core/Repositories/EloquentPostRepository.php b/src/Core/Repositories/EloquentPostRepository.php index b08d12ffb..393b668d7 100644 --- a/src/Core/Repositories/EloquentPostRepository.php +++ b/src/Core/Repositories/EloquentPostRepository.php @@ -1,10 +1,32 @@ 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; } } diff --git a/src/Core/Repositories/EloquentUserRepository.php b/src/Core/Repositories/EloquentUserRepository.php index ac584950f..89a143083 100644 --- a/src/Core/Repositories/EloquentUserRepository.php +++ b/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; diff --git a/src/Core/Search/Discussions/DiscussionSearcher.php b/src/Core/Search/Discussions/DiscussionSearcher.php index 82b571901..81e1fbfd5 100644 --- a/src/Core/Search/Discussions/DiscussionSearcher.php +++ b/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); diff --git a/src/Core/Seeders/PermissionsTableSeeder.php b/src/Core/Seeders/PermissionsTableSeeder.php index ae48890d2..85292636c 100644 --- a/src/Core/Seeders/PermissionsTableSeeder.php +++ b/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'], ]; diff --git a/src/Core/Support/Locked.php b/src/Core/Support/Locked.php new file mode 100644 index 000000000..4d0cbd8a9 --- /dev/null +++ b/src/Core/Support/Locked.php @@ -0,0 +1,57 @@ +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; + } + } +} diff --git a/src/Core/Support/VisibleScope.php b/src/Core/Support/VisibleScope.php new file mode 100644 index 000000000..cbe1a5b21 --- /dev/null +++ b/src/Core/Support/VisibleScope.php @@ -0,0 +1,20 @@ +