From d1dfa758e47a15574c2893b3c8a9d9946effaeed Mon Sep 17 00:00:00 2001 From: Alexander Skvortsov <38059171+askvortsov1@users.noreply.github.com> Date: Tue, 8 Dec 2020 19:10:06 -0500 Subject: [PATCH] Policy Extender and Tests (#2461) Policy application has also been refactored, so that policies return one of `allow`, `deny`, `forceAllow`, `forceDeny`. The result of a set of policies is no longer the first non-null result, but rather the highest priority result (forceDeny > forceAllow > deny > allow, so if a single forceDeny is present, that beats out all other returned results). This removes order in which extensions boot as a factor. --- .../{ => Access}/DiscussionPolicy.php | 24 +- src/Discussion/DiscussionServiceProvider.php | 1 - src/Event/GetPermission.php | 3 + src/Extend/Policy.php | 69 +++++ src/Group/{ => Access}/GroupPolicy.php | 11 +- src/Group/GroupServiceProvider.php | 3 - src/Post/{ => Access}/PostPolicy.php | 24 +- src/Post/PostServiceProvider.php | 3 - src/User/Access/AbstractPolicy.php | 64 ++++ src/User/Access/Gate.php | 131 ++++++++ src/User/{ => Access}/UserPolicy.php | 11 +- src/User/Gate.php | 60 ---- src/User/User.php | 6 +- src/User/UserServiceProvider.php | 25 +- tests/integration/extenders/PolicyTest.php | 287 ++++++++++++++++++ 15 files changed, 597 insertions(+), 125 deletions(-) rename src/Discussion/{ => Access}/DiscussionPolicy.php (84%) create mode 100644 src/Extend/Policy.php rename src/Group/{ => Access}/GroupPolicy.php (75%) rename src/Post/{ => Access}/PostPolicy.php (83%) create mode 100644 src/User/Access/AbstractPolicy.php create mode 100644 src/User/Access/Gate.php rename src/User/{ => Access}/UserPolicy.php (78%) delete mode 100644 src/User/Gate.php create mode 100644 tests/integration/extenders/PolicyTest.php diff --git a/src/Discussion/DiscussionPolicy.php b/src/Discussion/Access/DiscussionPolicy.php similarity index 84% rename from src/Discussion/DiscussionPolicy.php rename to src/Discussion/Access/DiscussionPolicy.php index 183626f37..3f07310c3 100644 --- a/src/Discussion/DiscussionPolicy.php +++ b/src/Discussion/Access/DiscussionPolicy.php @@ -7,38 +7,28 @@ * LICENSE file that was distributed with this source code. */ -namespace Flarum\Discussion; +namespace Flarum\Discussion\Access; +use Flarum\Discussion\Discussion; use Flarum\Settings\SettingsRepositoryInterface; -use Flarum\User\AbstractPolicy; +use Flarum\User\Access\AbstractPolicy; use Flarum\User\User; use Illuminate\Contracts\Events\Dispatcher; class DiscussionPolicy extends AbstractPolicy { - /** - * {@inheritdoc} - */ - protected $model = Discussion::class; - /** * @var SettingsRepositoryInterface */ protected $settings; - /** - * @var Dispatcher - */ - protected $events; - /** * @param SettingsRepositoryInterface $settings * @param Dispatcher $events */ - public function __construct(SettingsRepositoryInterface $settings, Dispatcher $events) + public function __construct(SettingsRepositoryInterface $settings) { $this->settings = $settings; - $this->events = $events; } /** @@ -49,7 +39,7 @@ class DiscussionPolicy extends AbstractPolicy public function can(User $actor, $ability) { if ($actor->hasPermission('discussion.'.$ability)) { - return true; + return $this->allow(); } } @@ -66,7 +56,7 @@ class DiscussionPolicy extends AbstractPolicy if ($allowRenaming === '-1' || ($allowRenaming === 'reply' && $discussion->participant_count <= 1) || ($discussion->created_at->diffInMinutes() < $allowRenaming)) { - return true; + return $this->allow(); } } } @@ -83,7 +73,7 @@ class DiscussionPolicy extends AbstractPolicy && (! $discussion->hidden_at || $discussion->hidden_user_id == $actor->id) && $actor->can('reply', $discussion) ) { - return true; + return $this->allow(); } } } diff --git a/src/Discussion/DiscussionServiceProvider.php b/src/Discussion/DiscussionServiceProvider.php index c5f1cc383..e1394e90a 100644 --- a/src/Discussion/DiscussionServiceProvider.php +++ b/src/Discussion/DiscussionServiceProvider.php @@ -23,7 +23,6 @@ class DiscussionServiceProvider extends AbstractServiceProvider $events = $this->app->make('events'); $events->subscribe(DiscussionMetadataUpdater::class); - $events->subscribe(DiscussionPolicy::class); $events->listen( Renamed::class, diff --git a/src/Event/GetPermission.php b/src/Event/GetPermission.php index b7b20fae5..876795c38 100644 --- a/src/Event/GetPermission.php +++ b/src/Event/GetPermission.php @@ -11,6 +11,9 @@ namespace Flarum\Event; use Flarum\User\User; +/** + * @deprecated beta 15, remove beta 16 + */ class GetPermission { /** diff --git a/src/Extend/Policy.php b/src/Extend/Policy.php new file mode 100644 index 000000000..69b63d1b9 --- /dev/null +++ b/src/Extend/Policy.php @@ -0,0 +1,69 @@ +globalPolicies[] = $policy; + + return $this; + } + + /** + * Add a custom policy for when an ability check is ran on an instance of a model. + * + * @param string $modelClass The ::class attribute of the model you are applying policies to. + * This model should extend from \Flarum\Database\AbstractModel. + * @param string $policy ::class attribute of policy class, which must extend Flarum\User\AbstractPolicy + */ + public function modelPolicy(string $modelClass, string $policy) + { + if (! array_key_exists($modelClass, $this->modelPolicies)) { + $this->modelPolicies[$modelClass] = []; + } + + $this->modelPolicies[$modelClass][] = $policy; + + return $this; + } + + public function extend(Container $container, Extension $extension = null) + { + $container->extend('flarum.policies', function ($existingPolicies) { + foreach ($this->modelPolicies as $modelClass => $addPolicies) { + if (! array_key_exists($modelClass, $existingPolicies)) { + $existingPolicies[$modelClass] = []; + } + + foreach ($addPolicies as $policy) { + $existingPolicies[$modelClass][] = $policy; + } + } + + $existingPolicies[AbstractPolicy::GLOBAL] = array_merge($existingPolicies[AbstractPolicy::GLOBAL], $this->globalPolicies); + + return $existingPolicies; + }); + } +} diff --git a/src/Group/GroupPolicy.php b/src/Group/Access/GroupPolicy.php similarity index 75% rename from src/Group/GroupPolicy.php rename to src/Group/Access/GroupPolicy.php index e3f6d0874..965cfdf37 100644 --- a/src/Group/GroupPolicy.php +++ b/src/Group/Access/GroupPolicy.php @@ -7,18 +7,13 @@ * LICENSE file that was distributed with this source code. */ -namespace Flarum\Group; +namespace Flarum\Group\Access; -use Flarum\User\AbstractPolicy; +use Flarum\User\Access\AbstractPolicy; use Flarum\User\User; class GroupPolicy extends AbstractPolicy { - /** - * {@inheritdoc} - */ - protected $model = Group::class; - /** * @param User $actor * @param string $ability @@ -27,7 +22,7 @@ class GroupPolicy extends AbstractPolicy public function can(User $actor, $ability) { if ($actor->hasPermission('group.'.$ability)) { - return true; + return $this->allow(); } } } diff --git a/src/Group/GroupServiceProvider.php b/src/Group/GroupServiceProvider.php index 0a0234695..49a278e88 100644 --- a/src/Group/GroupServiceProvider.php +++ b/src/Group/GroupServiceProvider.php @@ -19,9 +19,6 @@ class GroupServiceProvider extends AbstractServiceProvider */ public function boot() { - $events = $this->app->make('events'); - $events->subscribe(GroupPolicy::class); - Group::registerVisibilityScoper(new ScopeGroupVisibility(), 'view'); } } diff --git a/src/Post/PostPolicy.php b/src/Post/Access/PostPolicy.php similarity index 83% rename from src/Post/PostPolicy.php rename to src/Post/Access/PostPolicy.php index 7f27b3173..ff2768625 100644 --- a/src/Post/PostPolicy.php +++ b/src/Post/Access/PostPolicy.php @@ -7,39 +7,27 @@ * LICENSE file that was distributed with this source code. */ -namespace Flarum\Post; +namespace Flarum\Post\Access; use Carbon\Carbon; +use Flarum\Post\Post; use Flarum\Settings\SettingsRepositoryInterface; -use Flarum\User\AbstractPolicy; +use Flarum\User\Access\AbstractPolicy; use Flarum\User\User; -use Illuminate\Contracts\Events\Dispatcher; class PostPolicy extends AbstractPolicy { - /** - * {@inheritdoc} - */ - protected $model = Post::class; - /** * @var SettingsRepositoryInterface */ protected $settings; - /** - * @var Dispatcher - */ - protected $events; - /** * @param SettingsRepositoryInterface $settings - * @param Dispatcher $events */ - public function __construct(SettingsRepositoryInterface $settings, Dispatcher $events) + public function __construct(SettingsRepositoryInterface $settings) { $this->settings = $settings; - $this->events = $events; } /** @@ -51,7 +39,7 @@ class PostPolicy extends AbstractPolicy public function can(User $actor, $ability, Post $post) { if ($actor->can($ability.'Posts', $post->discussion)) { - return true; + return $this->allow(); } } @@ -71,7 +59,7 @@ class PostPolicy extends AbstractPolicy if ($allowEditing === '-1' || ($allowEditing === 'reply' && $post->number >= $post->discussion->last_post_number) || ($post->created_at->diffInMinutes(new Carbon) < $allowEditing)) { - return true; + return $this->allow(); } } } diff --git a/src/Post/PostServiceProvider.php b/src/Post/PostServiceProvider.php index 9168c4f61..795d9ab13 100644 --- a/src/Post/PostServiceProvider.php +++ b/src/Post/PostServiceProvider.php @@ -51,9 +51,6 @@ class PostServiceProvider extends AbstractServiceProvider $this->setPostTypes(); - $events = $this->app->make('events'); - $events->subscribe(PostPolicy::class); - Post::registerVisibilityScoper(new ScopePostVisibility(), 'view'); } diff --git a/src/User/Access/AbstractPolicy.php b/src/User/Access/AbstractPolicy.php new file mode 100644 index 000000000..ef32c539f --- /dev/null +++ b/src/User/Access/AbstractPolicy.php @@ -0,0 +1,64 @@ + false, + AbstractPolicy::FORCE_ALLOW => true, + AbstractPolicy::DENY => false, + AbstractPolicy::ALLOW => true, + ]; + + /** + * @var Container + */ + protected $container; + + /** + * @var Dispatcher + */ + protected $events; + + /** + * @var array + */ + protected $policyClasses; + + /** + * @var array + */ + protected $policies; + + /** + * @param Dispatcher $events + */ + public function __construct(Container $container, Dispatcher $events, array $policyClasses) + { + $this->container = $container; + $this->events = $events; + $this->policyClasses = $policyClasses; + } + + /** + * Determine if the given ability should be granted for the current user. + * + * @param User $actor + * @param string $ability + * @param string|AbstractModel $model + * @return bool + */ + public function allows(User $actor, string $ability, $model): bool + { + $results = []; + $appliedPolicies = []; + + if ($model) { + $modelClasses = is_string($model) ? [$model] : array_merge(class_parents(($model)), [get_class($model)]); + + foreach ($modelClasses as $class) { + $appliedPolicies = array_merge($appliedPolicies, $this->getPolicies($class)); + } + } else { + $appliedPolicies = $this->getPolicies(AbstractPolicy::GLOBAL); + } + + foreach ($appliedPolicies as $policy) { + $results[] = $policy->checkAbility($actor, $ability, $model); + } + + foreach (static::EVALUATION_CRITERIA_PRIORITY as $criteria => $decision) { + if (in_array($criteria, $results, true)) { + return $decision; + } + } + + // START OLD DEPRECATED SYSTEM + + // Fire an event so that core and extension modelPolicies can hook into + // this permission query and explicitly grant or deny the + // permission. + $allowed = $this->events->until( + new GetPermission($actor, $ability, $model) + ); + + if (! is_null($allowed)) { + return $allowed; + } + // END OLD DEPRECATED SYSTEM + + // If no policy covered this permission query, we will only grant + // the permission if the actor's groups have it. Otherwise, we will + // not allow the user to perform this action. + if ($actor->isAdmin() || ($actor->hasPermission($ability))) { + return true; + } + + return false; + } + + /** + * Get all policies for a given model and ability. + */ + protected function getPolicies(string $model) + { + $compiledPolicies = Arr::get($this->policies, $model); + if (is_null($compiledPolicies)) { + $policyClasses = Arr::get($this->policyClasses, $model, []); + $compiledPolicies = array_map(function ($policyClass) { + return $this->container->make($policyClass); + }, $policyClasses); + Arr::set($this->policies, $model, $compiledPolicies); + } + + return $compiledPolicies; + } +} diff --git a/src/User/UserPolicy.php b/src/User/Access/UserPolicy.php similarity index 78% rename from src/User/UserPolicy.php rename to src/User/Access/UserPolicy.php index 95bb98e39..6612f53c7 100644 --- a/src/User/UserPolicy.php +++ b/src/User/Access/UserPolicy.php @@ -7,15 +7,12 @@ * LICENSE file that was distributed with this source code. */ -namespace Flarum\User; +namespace Flarum\User\Access; + +use Flarum\User\User; class UserPolicy extends AbstractPolicy { - /** - * {@inheritdoc} - */ - protected $model = User::class; - /** * @param User $actor * @param string $ability @@ -24,7 +21,7 @@ class UserPolicy extends AbstractPolicy public function can(User $actor, $ability) { if ($actor->hasPermission('user.'.$ability)) { - return true; + return $this->allow(); } } } diff --git a/src/User/Gate.php b/src/User/Gate.php deleted file mode 100644 index c3a8d54f4..000000000 --- a/src/User/Gate.php +++ /dev/null @@ -1,60 +0,0 @@ -events = $events; - } - - /** - * Determine if the given ability should be granted for the current user. - * - * @param User $actor - * @param string $ability - * @param array|mixed $arguments - * @return bool - */ - public function allows($actor, $ability, $arguments) - { - // Fire an event so that core and extension policies can hook into - // this permission query and explicitly grant or deny the - // permission. - $allowed = $this->events->until( - new GetPermission($actor, $ability, $arguments) - ); - - if (! is_null($allowed)) { - return $allowed; - } - - // If no policy covered this permission query, we will only grant - // the permission if the actor's groups have it. Otherwise, we will - // not allow the user to perform this action. - if ($actor->isAdmin() || ($actor->hasPermission($ability))) { - return true; - } - - return false; - } -} diff --git a/src/User/User.php b/src/User/User.php index 24029fe43..32ac7c538 100644 --- a/src/User/User.php +++ b/src/User/User.php @@ -624,7 +624,7 @@ class User extends AbstractModel * @param mixed $arguments * @throws PermissionDeniedException */ - public function assertCan($ability, $arguments = []) + public function assertCan($ability, $arguments = null) { $this->assertPermission( $this->can($ability, $arguments) @@ -762,7 +762,7 @@ class User extends AbstractModel * @param array|mixed $arguments * @return bool */ - public function can($ability, $arguments = []) + public function can($ability, $arguments = null) { return static::$gate->allows($this, $ability, $arguments); } @@ -772,7 +772,7 @@ class User extends AbstractModel * @param array|mixed $arguments * @return bool */ - public function cannot($ability, $arguments = []) + public function cannot($ability, $arguments = null) { return ! $this->can($ability, $arguments); } diff --git a/src/User/UserServiceProvider.php b/src/User/UserServiceProvider.php index bea62a4b9..ab6148ee6 100644 --- a/src/User/UserServiceProvider.php +++ b/src/User/UserServiceProvider.php @@ -9,8 +9,14 @@ namespace Flarum\User; +use Flarum\Discussion\Access\DiscussionPolicy; +use Flarum\Discussion\Discussion; use Flarum\Foundation\AbstractServiceProvider; use Flarum\Foundation\ContainerUtil; +use Flarum\Group\Access\GroupPolicy; +use Flarum\Group\Group; +use Flarum\Post\Access\PostPolicy; +use Flarum\Post\Post; use Flarum\Settings\SettingsRepositoryInterface; use Flarum\User\Access\ScopeUserVisibility; use Flarum\User\DisplayName\DriverInterface; @@ -36,6 +42,16 @@ class UserServiceProvider extends AbstractServiceProvider $this->app->singleton('flarum.user.group_processors', function () { return []; }); + + $this->app->singleton('flarum.policies', function () { + return [ + Access\AbstractPolicy::GLOBAL => [], + Discussion::class => [DiscussionPolicy::class], + Group::class => [GroupPolicy::class], + Post::class => [PostPolicy::class], + User::class => [Access\UserPolicy::class], + ]; + }); } protected function registerDisplayNameDrivers() @@ -81,18 +97,17 @@ class UserServiceProvider extends AbstractServiceProvider User::addGroupProcessor(ContainerUtil::wrapCallback($callback, $this->app)); } - User::setHasher($this->app->make('hash')); - User::setGate($this->app->make(Gate::class)); - User::setDisplayNameDriver($this->app->make('flarum.user.display_name.driver')); - $events = $this->app->make('events'); + User::setHasher($this->app->make('hash')); + User::setGate($this->app->makeWith(Access\Gate::class, ['policyClasses' => $this->app->make('flarum.policies')])); + User::setDisplayNameDriver($this->app->make('flarum.user.display_name.driver')); + $events->listen(Saving::class, SelfDemotionGuard::class); $events->listen(Registered::class, AccountActivationMailer::class); $events->listen(EmailChangeRequested::class, EmailConfirmationMailer::class); $events->subscribe(UserMetadataUpdater::class); - $events->subscribe(UserPolicy::class); User::registerPreference('discloseOnline', 'boolval', true); User::registerPreference('indexProfile', 'boolval', true); diff --git a/tests/integration/extenders/PolicyTest.php b/tests/integration/extenders/PolicyTest.php new file mode 100644 index 000000000..d45c2cdc5 --- /dev/null +++ b/tests/integration/extenders/PolicyTest.php @@ -0,0 +1,287 @@ + 2, 'json' => ['data' => ['attributes' => ['isHidden' => true]]]]; + + private function prepDb() + { + $this->prepareDatabase([ + 'users' => [ + $this->adminUser(), + $this->normalUser(), + ], + 'discussions' => [ + ['id' => 1, 'title' => 'Unrelated Discussion', 'created_at' => Carbon::now()->toDateTimeString(), 'user_id' => 1, 'first_post_id' => 1, 'comment_count' => 1, 'is_private' => 0], + ], + 'posts' => [ + ['id' => 1, 'discussion_id' => 1, 'created_at' => Carbon::now()->toDateTimeString(), 'user_id' => 1, 'type' => 'comment', 'content' => '

a normal reply - too-obscure

'], + ] + ]); + } + + /** + * @test + */ + public function unrelated_user_cant_hide_discussion_by_default() + { + $this->prepDb(); + + $response = $this->send( + $this->request('PATCH', '/api/discussions/1', $this->hideQuery) + ); + + $this->assertEquals(403, $response->getStatusCode()); + } + + /** + * @test + */ + public function unrelated_user_can_hide_discussion_if_allowed() + { + $this->extend( + (new Extend\Policy()) + ->modelPolicy(Discussion::class, CustomPolicy::class) + ); + + $this->prepDb(); + + $response = $this->send( + $this->request('PATCH', '/api/discussions/1', $this->hideQuery) + ); + + $this->assertEquals(200, $response->getStatusCode()); + } + + /** + * @test + */ + public function unrelated_user_cant_hide_discussion_if_denied() + { + $this->extend( + (new Extend\Policy()) + ->modelPolicy(Discussion::class, DenyHidePolicy::class) + ->modelPolicy(Discussion::class, CustomPolicy::class) + ); + + $this->prepDb(); + + $response = $this->send( + $this->request('PATCH', '/api/discussions/1', $this->hideQuery) + ); + + $this->assertEquals(403, $response->getStatusCode()); + } + + /** + * @test + */ + public function unrelated_user_can_hide_discussion_if_force_allowed() + { + $this->extend( + (new Extend\Policy()) + ->modelPolicy(Discussion::class, ForceAllowHidePolicy::class) + ->modelPolicy(Discussion::class, DenyHidePolicy::class) + ->modelPolicy(Discussion::class, CustomPolicy::class) + ); + + $this->prepDb(); + + $response = $this->send( + $this->request('PATCH', '/api/discussions/1', $this->hideQuery) + ); + + $this->assertEquals(200, $response->getStatusCode()); + } + + /** + * @test + */ + public function unrelated_user_cant_hide_discussion_if_force_denied() + { + $this->extend( + (new Extend\Policy()) + ->modelPolicy(Discussion::class, DenyHidePolicy::class) + ->modelPolicy(Discussion::class, ForceDenyHidePolicy::class) + ->modelPolicy(Discussion::class, CustomPolicy::class) + ->modelPolicy(Discussion::class, ForceAllowHidePolicy::class) + ); + + $this->prepDb(); + + $response = $this->send( + $this->request('PATCH', '/api/discussions/1', $this->hideQuery) + ); + + $this->assertEquals(403, $response->getStatusCode()); + } + + /** + * @test + */ + public function regular_user_cant_start_discussions_by_default() + { + $this->prepDb(); + + $user = User::find(2); + + $this->assertEquals(false, $user->can('startDiscussion')); + } + + /** + * @test + */ + public function regular_user_can_start_discussions_if_granted_by_global_policy() + { + $this->extend( + (new Extend\Policy) + ->globalPolicy(GlobalStartDiscussionPolicy::class) + ); + + $this->prepDb(); + + $user = User::find(2); + + $this->assertEquals(true, $user->can('startDiscussion')); + } + + /** + * @test + */ + public function global_policy_doesnt_apply_if_argument_provided() + { + $this->extend( + (new Extend\Policy) + ->globalPolicy(GlobalStartDiscussionPolicy::class) + ); + + $this->prepDb(); + + $user = User::find(2); + + $this->assertEquals(false, $user->can('startDiscussion', Discussion::find(1))); + } + + /** + * @test + */ + public function unrelated_user_cant_hide_post_by_default() + { + $this->prepDb(); + + $user = User::find(2); + + $this->assertEquals(false, $user->can('hide', Post::find(1))); + } + + /** + * @test + */ + public function unrelated_user_can_hide_post_if_allowed() + { + $this->extend( + (new Extend\Policy)->modelPolicy(CommentPost::class, CommentPostChildClassPolicy::class) + ); + $this->prepDb(); + + $user = User::find(2); + + $this->assertEquals(true, $user->can('hide', Post::find(1))); + } + + /** + * @test + */ + public function policies_are_inherited_to_child_classes() + { + $this->extend( + (new Extend\Policy)->modelPolicy(Post::class, PostParentClassPolicy::class), + (new Extend\Policy)->modelPolicy(CommentPost::class, CommentPostChildClassPolicy::class) + ); + $this->prepDb(); + + $user = User::find(2); + + $this->assertEquals(false, $user->can('hide', Post::find(1))); + } +} + +class CustomPolicy extends AbstractPolicy +{ + protected function hide(User $user, Discussion $discussion) + { + return $this->allow(); + } +} + +class DenyHidePolicy extends AbstractPolicy +{ + protected function hide(User $user, Discussion $discussion) + { + return $this->deny(); + } +} + +class ForceAllowHidePolicy extends AbstractPolicy +{ + protected function hide(User $user, Discussion $discussion) + { + return $this->forceAllow(); + } +} + +class ForceDenyHidePolicy extends AbstractPolicy +{ + protected function hide(User $user, Discussion $discussion) + { + return $this->forceDeny(); + } +} + +class GlobalStartDiscussionPolicy extends AbstractPolicy +{ + protected function startDiscussion(User $user) + { + return $this->allow(); + } +} + +class PostParentClassPolicy extends AbstractPolicy +{ + protected function hide(User $user, Post $post) + { + return $this->deny(); + } +} + +class CommentPostChildClassPolicy extends AbstractPolicy +{ + protected function hide(User $user, CommentPost $post) + { + return $this->allow(); + } +}