From dfbb468744dc3748e6ab2ed859c0e28f235fce24 Mon Sep 17 00:00:00 2001 From: Alexander Skvortsov <38059171+askvortsov1@users.noreply.github.com> Date: Tue, 11 May 2021 02:34:30 -0400 Subject: [PATCH] Refactor queryIdsWhereCan (#128) - Rename to queryIdsWhereHasPermission, since that's more accurate - Make the base query an optional 3rd argument. This feels more intuitive - Add a `can` function in AbstractPolicy, so extensions that add tag scopable permissions don't need to define their own policies (e.g. with approval) - Combine ScopeDiscussionVisibilityWithAbility and ScopeDiscussionVisibility - Fix ScopeDiscussionVisibility only requiring in one tag, not all. - Add lots and lots of tests Co-authored-by: SychO9 --- extensions/tags/extend.php | 3 +- extensions/tags/src/Access/GlobalPolicy.php | 19 +- .../src/Access/ScopeDiscussionVisibility.php | 45 --- .../ScopeDiscussionVisibilityForAbility.php | 46 ++- .../tags/src/Access/ScopeFlagVisibility.php | 2 +- .../tags/src/Access/ScopeTagVisibility.php | 2 +- extensions/tags/src/Access/TagPolicy.php | 22 +- extensions/tags/src/Tag.php | 22 +- .../RetrievesRepresentativeTags.php | 3 + .../api/discussions/CreateTest.php | 6 +- .../integration/api/discussions/ListTest.php | 128 ++++++++ .../api/discussions/UpdateTest.php | 301 ++++++++++++++++++ .../tags/ListTest.php} | 14 +- .../{PolicyTest.php => GlobalPolicyTest.php} | 2 +- .../authorization/TagPolicyTest.php | 112 +++++++ .../visibility/DiscussionVisibilityTest.php | 129 +++++--- 16 files changed, 703 insertions(+), 153 deletions(-) delete mode 100644 extensions/tags/src/Access/ScopeDiscussionVisibility.php create mode 100644 extensions/tags/tests/integration/api/discussions/ListTest.php create mode 100644 extensions/tags/tests/integration/api/discussions/UpdateTest.php rename extensions/tags/tests/integration/{visibility/TagVisibilityTest.php => api/tags/ListTest.php} (88%) rename extensions/tags/tests/integration/authorization/{PolicyTest.php => GlobalPolicyTest.php} (98%) create mode 100644 extensions/tags/tests/integration/authorization/TagPolicyTest.php diff --git a/extensions/tags/extend.php b/extensions/tags/extend.php index 8f48d169a..cead40942 100644 --- a/extensions/tags/extend.php +++ b/extensions/tags/extend.php @@ -71,7 +71,7 @@ return [ ->load('post.discussion.tags'), (new Extend\ApiController(FlarumController\ListDiscussionsController::class)) - ->addInclude(['tags', 'tags.state']) + ->addInclude(['tags', 'tags.state', 'tags.parent']) ->load('tags'), (new Extend\ApiController(FlarumController\ShowDiscussionController::class)) @@ -96,7 +96,6 @@ return [ ->globalPolicy(Access\GlobalPolicy::class), (new Extend\ModelVisibility(Discussion::class)) - ->scope(Access\ScopeDiscussionVisibility::class) ->scopeAll(Access\ScopeDiscussionVisibilityForAbility::class), (new Extend\ModelVisibility(Flag::class)) diff --git a/extensions/tags/src/Access/GlobalPolicy.php b/extensions/tags/src/Access/GlobalPolicy.php index 94301e19d..a33ff2d2a 100644 --- a/extensions/tags/src/Access/GlobalPolicy.php +++ b/extensions/tags/src/Access/GlobalPolicy.php @@ -33,14 +33,27 @@ class GlobalPolicy extends AbstractPolicy */ public function can(User $actor, string $ability) { + static $enoughPrimary; + static $enoughSecondary; + if (in_array($ability, ['viewDiscussions', 'startDiscussion'])) { if ($actor->hasPermission($ability) && $actor->hasPermission('bypassTagCounts')) { return $this->allow(); } - $enoughPrimary = Tag::queryIdsWhereCan(Tag::query()->getQuery(), $actor, $ability, true, false)->count() >= $this->settings->get('flarum-tags.min_primary_tags'); - $enoughSecondary = Tag::queryIdsWhereCan(Tag::query()->getQuery(), $actor, $ability, false, true)->count() >= $this->settings->get('flarum-tags.min_secondary_tags'); - if ($enoughPrimary && $enoughSecondary) { + if (! isset($enoughPrimary[$actor->id][$ability])) { + $enoughPrimary[$actor->id][$ability] = Tag::whereHasPermission($actor, $ability) + ->where('tags.position', '!=', null) + ->count() >= $this->settings->get('flarum-tags.min_primary_tags'); + } + + if (! isset($enoughSecondary[$actor->id][$ability])) { + $enoughSecondary[$actor->id][$ability] = Tag::whereHasPermission($actor, $ability) + ->where('tags.position', '=', null) + ->count() >= $this->settings->get('flarum-tags.min_secondary_tags'); + } + + if ($enoughPrimary[$actor->id][$ability] && $enoughSecondary[$actor->id][$ability]) { return $this->allow(); } else { return $this->deny(); diff --git a/extensions/tags/src/Access/ScopeDiscussionVisibility.php b/extensions/tags/src/Access/ScopeDiscussionVisibility.php deleted file mode 100644 index 7e004b8b4..000000000 --- a/extensions/tags/src/Access/ScopeDiscussionVisibility.php +++ /dev/null @@ -1,45 +0,0 @@ -where(function ($query) use ($actor) { - $query - ->whereNotIn('discussions.id', function ($query) use ($actor) { - return $query->select('discussion_id') - ->from('discussion_tag') - ->whereNotIn('tag_id', function ($query) use ($actor) { - Tag::queryIdsWhereCan($query->from('tags'), $actor, 'viewDiscussions'); - }); - }) - ->orWhere(function ($query) use ($actor) { - $query->whereVisibleTo($actor, 'viewDiscussionsInRestrictedTags'); - }); - }); - - // Hide discussions with no tags if the user doesn't have that global - // permission. - if (! $actor->hasPermission('viewDiscussions')) { - $query->has('tags'); - } - } -} diff --git a/extensions/tags/src/Access/ScopeDiscussionVisibilityForAbility.php b/extensions/tags/src/Access/ScopeDiscussionVisibilityForAbility.php index 26a9310f7..585e4977a 100644 --- a/extensions/tags/src/Access/ScopeDiscussionVisibilityForAbility.php +++ b/extensions/tags/src/Access/ScopeDiscussionVisibilityForAbility.php @@ -12,6 +12,7 @@ namespace Flarum\Tags\Access; use Flarum\Tags\Tag; use Flarum\User\User; use Illuminate\Database\Eloquent\Builder; +use Illuminate\Support\Str; class ScopeDiscussionVisibilityForAbility { @@ -22,19 +23,46 @@ class ScopeDiscussionVisibilityForAbility */ public function __invoke(User $actor, Builder $query, $ability) { - if (substr($ability, 0, 4) === 'view') { + // Automatic scoping should be applied to the global `view` ability, + // and to arbitrary abilities that aren't subqueries of `view`. + // For example, if we want to scope discussions where the user can + // edit posts, this should apply. + // But if we are expanding a restriction of `view` (for example, + // `viewPrivate`), we shouldn't apply this query again. + if (substr($ability, 0, 4) === 'view' && $ability !== 'view') { return; } - // If a discussion requires a certain permission in order for it to be - // visible, then we can check if the user has been granted that - // permission for any of the discussion's tags. - $query->whereIn('discussions.id', function ($query) use ($actor, $ability) { - return $query->select('discussion_id') - ->from('discussion_tag') - ->whereIn('tag_id', function ($query) use ($actor, $ability) { - Tag::queryIdsWhereCan($query->from('tags'), $actor, 'discussion.'.$ability); + // Avoid an infinite recursive loop. + if (Str::endsWith($ability, 'InRestrictedTags')) { + return; + } + + // `view` is a special case where the permission string is represented by `viewDiscussions`. + $permission = $ability === 'view' ? 'viewDiscussions' : $ability; + + // Restrict discussions where users don't have necessary permissions in all tags. + // We use a double notIn instead of a doubleIn because the permission must be present in ALL tags, + // not just one. + $query->where(function ($query) use ($actor, $permission) { + $query + ->whereNotIn('discussions.id', function ($query) use ($actor, $permission) { + return $query->select('discussion_id') + ->from('discussion_tag') + ->whereNotIn('tag_id', function ($query) use ($actor, $permission) { + Tag::query()->setQuery($query->from('tags'))->whereHasPermission($actor, $permission)->select('tags.id'); + }); + }) + ->orWhere(function ($query) use ($actor, $permission) { + // Allow extensions a way to override scoping for any given permission. + $query->whereVisibleTo($actor, "${permission}InRestrictedTags"); }); }); + + // Hide discussions with no tags if the user doesn't have that global + // permission. + if (! $actor->hasPermission($permission)) { + $query->has('tags'); + } } } diff --git a/extensions/tags/src/Access/ScopeFlagVisibility.php b/extensions/tags/src/Access/ScopeFlagVisibility.php index f9547e3e0..b926fff5e 100644 --- a/extensions/tags/src/Access/ScopeFlagVisibility.php +++ b/extensions/tags/src/Access/ScopeFlagVisibility.php @@ -29,7 +29,7 @@ class ScopeFlagVisibility return $query->selectRaw('1') ->from('discussion_tag') ->whereNotIn('tag_id', function ($query) use ($actor) { - Tag::queryIdsWhereCan($query->from('tags'), $actor, 'discussion.viewFlags'); + Tag::query()->setQuery($query->from('tags'))->whereHasPermission($actor, 'viewFlags')->select('tags.id'); }) ->whereColumn('discussions.id', 'discussion_id'); }); diff --git a/extensions/tags/src/Access/ScopeTagVisibility.php b/extensions/tags/src/Access/ScopeTagVisibility.php index 2802efe0b..61274825d 100644 --- a/extensions/tags/src/Access/ScopeTagVisibility.php +++ b/extensions/tags/src/Access/ScopeTagVisibility.php @@ -22,7 +22,7 @@ class ScopeTagVisibility public function __invoke(User $actor, Builder $query) { $query->whereIn('id', function ($query) use ($actor) { - Tag::queryIdsWhereCan($query, $actor, 'viewDiscussions'); + Tag::query()->setQuery($query->from('tags'))->whereHasPermission($actor, 'viewDiscussions')->select('tags.id'); }); } } diff --git a/extensions/tags/src/Access/TagPolicy.php b/extensions/tags/src/Access/TagPolicy.php index 3578712ca..25b74c6c3 100755 --- a/extensions/tags/src/Access/TagPolicy.php +++ b/extensions/tags/src/Access/TagPolicy.php @@ -15,25 +15,21 @@ use Flarum\User\User; class TagPolicy extends AbstractPolicy { - /** - * @param User $actor - * @param Tag $tag - * @return bool|null - */ - public function startDiscussion(User $actor, Tag $tag) + public function can(User $actor, string $ability, Tag $tag) { + if ($tag->parent_id !== null && ! $actor->can($ability, $tag->parent)) { + return $this->deny(); + } + if ($tag->is_restricted) { - return $actor->hasPermission('tag'.$tag->id.'.startDiscussion') ? $this->allow() : $this->deny(); + $id = $tag->id; + + return $actor->hasPermission("tag$id.$ability"); } } - /** - * @param User $actor - * @param Tag $tag - * @return bool|null - */ public function addToDiscussion(User $actor, Tag $tag) { - return $actor->can('startDiscussion', $tag) ? $this->allow() : $this->deny(); + return $actor->can('startDiscussion', $tag); } } diff --git a/extensions/tags/src/Tag.php b/extensions/tags/src/Tag.php index 45f62a2f4..134d64e9a 100644 --- a/extensions/tags/src/Tag.php +++ b/extensions/tags/src/Tag.php @@ -15,7 +15,6 @@ use Flarum\Discussion\Discussion; use Flarum\Group\Permission; use Flarum\User\User; use Illuminate\Database\Eloquent\Builder; -use Illuminate\Database\Query\Builder as QueryBuilder; /** * @property int $id @@ -219,7 +218,7 @@ class Tag extends AbstractModel } } - public static function queryIdsWhereCan(QueryBuilder $base, User $user, string $currPermission, bool $includePrimary = true, bool $includeSecondary = true): QueryBuilder + public function scopeWhereHasPermission(Builder $query, User $user, string $currPermission): Builder { $hasGlobalPermission = $user->hasPermission($currPermission); $isAdmin = $user->isAdmin(); @@ -236,30 +235,19 @@ class Tag extends AbstractModel }) ->values(); - $validTags = $base - ->from('tags as s_tags') + return $query ->where(function ($query) use ($isAdmin, $hasGlobalPermission, $tagIdsWithPermission) { $query - ->whereIn('s_tags.id', function ($query) use ($isAdmin, $hasGlobalPermission, $tagIdsWithPermission) { + ->whereIn('tags.id', function ($query) use ($isAdmin, $hasGlobalPermission, $tagIdsWithPermission) { static::buildPermissionSubquery($query, $isAdmin, $hasGlobalPermission, $tagIdsWithPermission); }) ->where(function ($query) use ($isAdmin, $hasGlobalPermission, $tagIdsWithPermission) { $query - ->whereIn('s_tags.parent_id', function ($query) use ($isAdmin, $hasGlobalPermission, $tagIdsWithPermission) { + ->whereIn('tags.parent_id', function ($query) use ($isAdmin, $hasGlobalPermission, $tagIdsWithPermission) { static::buildPermissionSubquery($query, $isAdmin, $hasGlobalPermission, $tagIdsWithPermission); }) - ->orWhere('s_tags.parent_id', null); + ->orWhere('tags.parent_id', null); }); - }) - ->where(function ($query) use ($includePrimary, $includeSecondary) { - if (! $includePrimary) { - $query->where('s_tags.position', '=', null); - } - if (! $includeSecondary) { - $query->where('s_tags.position', '!=', null); - } }); - - return $validTags->select('s_tags.id'); } } diff --git a/extensions/tags/tests/integration/RetrievesRepresentativeTags.php b/extensions/tags/tests/integration/RetrievesRepresentativeTags.php index 06d346f91..2eafbd111 100644 --- a/extensions/tags/tests/integration/RetrievesRepresentativeTags.php +++ b/extensions/tags/tests/integration/RetrievesRepresentativeTags.php @@ -25,6 +25,9 @@ trait RetrievesRepresentativeTags ['id' => 9, 'name' => 'Secondary 1', 'slug' => 'secondary-1', 'position' => null, 'parent_id' => null], ['id' => 10, 'name' => 'Secondary 2', 'slug' => 'secondary-2', 'position' => null, 'parent_id' => null], ['id' => 11, 'name' => 'Secondary Restricted', 'slug' => 'secondary-restricted', 'position' => null, 'parent_id' => null, 'is_restricted' => true], + ['id' => 12, 'name' => 'Primary Restricted 2', 'slug' => 'primary-2-restricted', 'position' => 100, 'parent_id' => null, 'is_restricted' => true], + ['id' => 13, 'name' => 'Primary Restricted 2 Child 1', 'slug' => 'primary-2-restricted-child-1', 'position' => 101, 'parent_id' => 12], + ['id' => 14, 'name' => 'Primary Restricted 3', 'slug' => 'primary-3-restricted', 'position' => 102, 'parent_id' =>null, 'is_restricted' => true], ]; } } diff --git a/extensions/tags/tests/integration/api/discussions/CreateTest.php b/extensions/tags/tests/integration/api/discussions/CreateTest.php index a131e07ed..81bc6ac90 100644 --- a/extensions/tags/tests/integration/api/discussions/CreateTest.php +++ b/extensions/tags/tests/integration/api/discussions/CreateTest.php @@ -221,8 +221,8 @@ class CreateTest extends TestCase 'relationships' => [ 'tags' => [ 'data' => [ - ['type' => 'tags', 'id' => 3], - ['type' => 'tags', 'id' => 4] + ['type' => 'tags', 'id' => 2], + ['type' => 'tags', 'id' => 3] ] ] ] @@ -231,7 +231,7 @@ class CreateTest extends TestCase ]) ); - $this->assertEquals(422, $response->getStatusCode()); + $this->assertEquals(201, $response->getStatusCode()); } /** diff --git a/extensions/tags/tests/integration/api/discussions/ListTest.php b/extensions/tags/tests/integration/api/discussions/ListTest.php new file mode 100644 index 000000000..0290f6ff6 --- /dev/null +++ b/extensions/tags/tests/integration/api/discussions/ListTest.php @@ -0,0 +1,128 @@ +extension('flarum-tags'); + + $this->prepareDatabase([ + 'tags' => $this->tags(), + 'users' => [ + $this->normalUser(), + ], + 'group_permission' => [ + ['group_id' => Group::MEMBER_ID, 'permission' => 'tag5.viewDiscussions'], + ['group_id' => Group::MEMBER_ID, 'permission' => 'tag8.viewDiscussions'], + ['group_id' => Group::MEMBER_ID, 'permission' => 'tag11.viewDiscussions'], + ['group_id' => Group::MEMBER_ID, 'permission' => 'tag13.viewDiscussions'], + ], + 'discussions' => [ + ['id' => 1, 'title' => 'no tags', 'user_id' => 1, 'comment_count' => 1], + ['id' => 2, 'title' => 'open tags', 'user_id' => 1, 'comment_count' => 1], + ['id' => 3, 'title' => 'open tag, restricted child tag', 'user_id' => 1, 'comment_count' => 1], + ['id' => 4, 'title' => 'open tag, one restricted secondary tag', 'user_id' => 1, 'comment_count' => 1], + ['id' => 5, 'title' => 'all closed', 'user_id' => 1, 'comment_count' => 1], + ['id' => 6, 'title' => 'closed parent, open child tag', 'user_id' => 1, 'comment_count' => 1], + ], + 'posts' => [ + ['id' => 1, 'discussion_id' => 1, 'user_id' => 1, 'type' => 'comment', 'content' => '

'], + ['id' => 2, 'discussion_id' => 2, 'user_id' => 1, 'type' => 'comment', 'content' => '

'], + ['id' => 3, 'discussion_id' => 3, 'user_id' => 1, 'type' => 'comment', 'content' => '

'], + ['id' => 4, 'discussion_id' => 4, 'user_id' => 1, 'type' => 'comment', 'content' => '

'], + ['id' => 5, 'discussion_id' => 5, 'user_id' => 1, 'type' => 'comment', 'content' => '

'], + ['id' => 6, 'discussion_id' => 6, 'user_id' => 1, 'type' => 'comment', 'content' => '

'], + ], + 'discussion_tag' => [ + ['discussion_id' => 2, 'tag_id' => 1], + ['discussion_id' => 3, 'tag_id' => 2], + ['discussion_id' => 3, 'tag_id' => 5], + ['discussion_id' => 4, 'tag_id' => 1], + ['discussion_id' => 4, 'tag_id' => 11], + ['discussion_id' => 5, 'tag_id' => 6], + ['discussion_id' => 5, 'tag_id' => 7], + ['discussion_id' => 5, 'tag_id' => 8], + ['discussion_id' => 6, 'tag_id' => 12], + ['discussion_id' => 6, 'tag_id' => 13], + ], + ]); + } + + /** + * @test + */ + public function admin_sees_all() + { + $response = $this->send( + $this->request('GET', '/api/discussions', [ + 'authenticatedAs' => 1 + ]) + ); + + $this->assertEquals(200, $response->getStatusCode()); + + $data = json_decode($response->getBody()->getContents(), true)['data']; + + $ids = Arr::pluck($data, 'id'); + $this->assertEqualsCanonicalizing(['1', '2', '3', '4', '5', '6'], $ids); + } + + /** + * @test + */ + public function user_sees_where_allowed() + { + $response = $this->send( + $this->request('GET', '/api/discussions', [ + 'authenticatedAs' => 2 + ]) + ); + + $this->assertEquals(200, $response->getStatusCode()); + + $data = json_decode($response->getBody()->getContents(), true)['data']; + + $ids = Arr::pluck($data, 'id'); + $this->assertEqualsCanonicalizing(['1', '2', '3', '4'], $ids); + } + + /** + * @test + */ + public function guest_can_see_where_allowed() + { + $response = $this->send( + $this->request('GET', '/api/discussions') + ); + + $this->assertEquals(200, $response->getStatusCode()); + + $data = json_decode($response->getBody()->getContents(), true)['data']; + + $ids = Arr::pluck($data, 'id'); + $this->assertEqualsCanonicalizing(['1', '2'], $ids); + } +} diff --git a/extensions/tags/tests/integration/api/discussions/UpdateTest.php b/extensions/tags/tests/integration/api/discussions/UpdateTest.php new file mode 100644 index 000000000..b2e87f9b4 --- /dev/null +++ b/extensions/tags/tests/integration/api/discussions/UpdateTest.php @@ -0,0 +1,301 @@ +extension('flarum-tags'); + + $this->prepareDatabase([ + 'tags' => $this->tags(), + 'users' => [ + $this->normalUser(), + ], + 'group_permission' => [ + ['group_id' => Group::MEMBER_ID, 'permission' => 'tag5.viewDiscussions'], + ['group_id' => Group::MEMBER_ID, 'permission' => 'tag8.viewDiscussions'], + ['group_id' => Group::MEMBER_ID, 'permission' => 'tag8.startDiscussion'], + ['group_id' => Group::MEMBER_ID, 'permission' => 'tag11.viewDiscussions'], + ['group_id' => Group::MEMBER_ID, 'permission' => 'tag11.startDiscussion'], + ], + 'discussions' => [ + ['id' => 1, 'title' => 'Discussion with post', 'user_id' => 2, 'first_post_id' => 1, 'comment_count' => 1], + ], + 'posts' => [ + ['id' => 1, 'discussion_id' => 1, 'user_id' => 2, 'type' => 'comment', 'content' => '

Text

'], + ], + 'discussion_tag' => [ + ['discussion_id' => 1, 'tag_id' => 1] + ] + ]); + } + + /** + * @test + */ + public function user_cant_change_tags_without_setting() + { + $response = $this->send( + $this->request('PATCH', '/api/discussions/1', [ + 'authenticatedAs' => 2, + 'json' => [ + 'data' => [ + 'relationships' => [ + 'tags' => [ + 'data' => [ + ['type' => 'tags', 'id' => 2] + ] + ] + ] + ], + ], + ]) + ); + + $this->assertEquals(403, $response->getStatusCode()); + } + + /** + * @test + */ + public function user_can_change_tags_without_setting() + { + $this->setting('allow_tag_change', '-1'); + + $response = $this->send( + $this->request('PATCH', '/api/discussions/1', [ + 'authenticatedAs' => 2, + 'json' => [ + 'data' => [ + 'relationships' => [ + 'tags' => [ + 'data' => [ + ['type' => 'tags', 'id' => 2] + ] + ] + ] + ], + ], + ]) + ); + + $this->assertEquals(200, $response->getStatusCode()); + } + + /** + * @test + */ + public function admin_can_add_primary_tag_beyond_limit() + { + $response = $this->send( + $this->request('PATCH', '/api/discussions/1', [ + 'authenticatedAs' => 1, + 'json' => [ + 'data' => [ + 'relationships' => [ + 'tags' => [ + 'data' => [ + ['type' => 'tags', 'id' => 1], + ['type' => 'tags', 'id' => 2] + ] + ] + ] + ], + ], + ]) + ); + + $this->assertEquals(200, $response->getStatusCode()); + } + + /** + * @test + */ + public function user_cant_add_primary_tag_beyond_limit() + { + $this->setting('allow_tag_change', '-1'); + + $response = $this->send( + $this->request('PATCH', '/api/discussions/1', [ + 'authenticatedAs' => 2, + 'json' => [ + 'data' => [ + 'relationships' => [ + 'tags' => [ + 'data' => [ + ['type' => 'tags', 'id' => 1], + ['type' => 'tags', 'id' => 2] + ] + ] + ] + ], + ], + ]) + ); + + $this->assertEquals(422, $response->getStatusCode()); + } + + /** + * @test + */ + public function user_cant_add_tag_where_can_view_but_cant_start() + { + $this->setting('allow_tag_change', '-1'); + + $response = $this->send( + $this->request('PATCH', '/api/discussions/1', [ + 'authenticatedAs' => 2, + 'json' => [ + 'data' => [ + 'relationships' => [ + 'tags' => [ + 'data' => [ + ['type' => 'tags', 'id' => 5] + ] + ] + ] + ], + ], + ]) + ); + + $this->assertEquals(403, $response->getStatusCode()); + } + + /** + * @test + */ + public function user_can_add_tag_where_can_view_and_can_start() + { + $this->setting('allow_tag_change', '-1'); + + $response = $this->send( + $this->request('PATCH', '/api/discussions/1', [ + 'authenticatedAs' => 2, + 'json' => [ + 'data' => [ + 'relationships' => [ + 'tags' => [ + 'data' => [ + ['type' => 'tags', 'id' => 1], + ['type' => 'tags', 'id' => 11] + ] + ] + ] + ], + ], + ]) + ); + + $this->assertEquals(200, $response->getStatusCode()); + } + + /** + * @test + */ + public function user_cant_add_child_tag_without_parent_tag() + { + $this->setting('allow_tag_change', '-1'); + + $response = $this->send( + $this->request('PATCH', '/api/discussions/1', [ + 'authenticatedAs' => 2, + 'json' => [ + 'data' => [ + 'attributes' => [ + 'title' => 'test - too-obscure', + 'content' => 'predetermined content for automated testing - too-obscure', + ], + 'relationships' => [ + 'tags' => [ + 'data' => [ + ['type' => 'tags', 'id' => 4] + ] + ] + ] + ], + ], + ]) + ); + + $this->assertEquals(403, $response->getStatusCode()); + } + + /** + * @test + */ + public function user_can_add_child_tag_with_parent_tag() + { + $this->setting('allow_tag_change', '-1'); + + $response = $this->send( + $this->request('PATCH', '/api/discussions/1', [ + 'authenticatedAs' => 2, + 'json' => [ + 'data' => [ + 'relationships' => [ + 'tags' => [ + 'data' => [ + ['type' => 'tags', 'id' => 2], + ['type' => 'tags', 'id' => 3] + ] + ] + ] + ], + ], + ]) + ); + + $this->assertEquals(200, $response->getStatusCode()); + } + + /** + * @test + */ + public function primary_tag_required_by_default() + { + $this->setting('allow_tag_change', '-1'); + + $response = $this->send( + $this->request('PATCH', '/api/discussions/1', [ + 'authenticatedAs' => 2, + 'json' => [ + 'data' => [ + 'relationships' => [ + 'tags' => [ + 'data' => [ + ['type' => 'tags', 'id' => 11] + ] + ] + ] + ], + ], + ]) + ); + + $this->assertEquals(422, $response->getStatusCode()); + } +} diff --git a/extensions/tags/tests/integration/visibility/TagVisibilityTest.php b/extensions/tags/tests/integration/api/tags/ListTest.php similarity index 88% rename from extensions/tags/tests/integration/visibility/TagVisibilityTest.php rename to extensions/tags/tests/integration/api/tags/ListTest.php index 30705222b..44aa9ee5b 100644 --- a/extensions/tags/tests/integration/visibility/TagVisibilityTest.php +++ b/extensions/tags/tests/integration/api/tags/ListTest.php @@ -7,7 +7,7 @@ * LICENSE file that was distributed with this source code. */ -namespace Flarum\Tags\Tests\integration\visibility; +namespace Flarum\Tags\Tests\integration\api\tags; use Flarum\Group\Group; use Flarum\Tags\Tests\integration\RetrievesRepresentativeTags; @@ -15,7 +15,7 @@ use Flarum\Testing\integration\RetrievesAuthorizedUsers; use Flarum\Testing\integration\TestCase; use Illuminate\Support\Arr; -class TagVisibilityTest extends TestCase +class ListTest extends TestCase { use RetrievesAuthorizedUsers; use RetrievesRepresentativeTags; @@ -56,7 +56,8 @@ class TagVisibilityTest extends TestCase $data = json_decode($response->getBody()->getContents(), true)['data']; - $this->assertEquals(['1', '2', '3', '4', '5', '6', '7', '8', '9', '10', '11'], Arr::pluck($data, 'id')); + $ids = Arr::pluck($data, 'id'); + $this->assertEquals(['1', '2', '3', '4', '5', '6', '7', '8', '9', '10', '11', '12', '13', '14'], $ids); } /** @@ -77,7 +78,8 @@ class TagVisibilityTest extends TestCase // 5 isnt included because parent access doesnt necessarily give child access // 6, 7, 8 aren't included because child access shouldnt work unless parent // access is also given. - $this->assertEquals(['1', '2', '3', '4', '9', '10', '11'], Arr::pluck($data, 'id')); + $ids = Arr::pluck($data, 'id'); + $this->assertEquals(['1', '2', '3', '4', '9', '10', '11'], $ids); } /** @@ -93,7 +95,7 @@ class TagVisibilityTest extends TestCase $data = json_decode($response->getBody()->getContents(), true)['data']; - // Order-independent comparison - $this->assertEquals(['1', '2', '3', '4', '9', '10'], Arr::pluck($data, 'id')); + $ids = Arr::pluck($data, 'id'); + $this->assertEquals(['1', '2', '3', '4', '9', '10'], $ids); } } diff --git a/extensions/tags/tests/integration/authorization/PolicyTest.php b/extensions/tags/tests/integration/authorization/GlobalPolicyTest.php similarity index 98% rename from extensions/tags/tests/integration/authorization/PolicyTest.php rename to extensions/tags/tests/integration/authorization/GlobalPolicyTest.php index 9fc750dec..a75b219a8 100644 --- a/extensions/tags/tests/integration/authorization/PolicyTest.php +++ b/extensions/tags/tests/integration/authorization/GlobalPolicyTest.php @@ -15,7 +15,7 @@ use Flarum\Testing\integration\RetrievesAuthorizedUsers; use Flarum\Testing\integration\TestCase; use Flarum\User\User; -class PolicyTest extends TestCase +class GlobalPolicyTest extends TestCase { use RetrievesAuthorizedUsers; use RetrievesRepresentativeTags; diff --git a/extensions/tags/tests/integration/authorization/TagPolicyTest.php b/extensions/tags/tests/integration/authorization/TagPolicyTest.php new file mode 100644 index 000000000..42076deb4 --- /dev/null +++ b/extensions/tags/tests/integration/authorization/TagPolicyTest.php @@ -0,0 +1,112 @@ +extension('flarum-tags'); + + $this->prepareDatabase([ + 'tags' => $this->tags(), + 'users' => [ + $this->normalUser(), + ], + 'group_permission' => [ + ['group_id' => Group::MEMBER_ID, 'permission' => 'tag6.arbitraryAbility!'], + ['group_id' => Group::MEMBER_ID, 'permission' => 'tag8.arbitraryAbility!'], + ] + ]); + } + + /** + * @test + */ + public function has_ability_when_allowed_in_restricted_tag() + { + $this->app(); + + $tag = Tag::find(6); + + $this->assertTrue(User::find(2)->can('arbitraryAbility!', $tag)); + } + + /** + * @test + */ + public function has_ability_in_child_when_allowed_in_top_tag_and_child() + { + $this->app(); + + $tag = Tag::find(8); + + $this->assertTrue(User::find(2)->can('arbitraryAbility!', $tag)); + } + + /** + * @test + */ + public function doesnt_have_ability_in_child_when_allowed_in_child_but_not_parent() + { + $this->app(); + + $this->database()->table('group_permission')->where('permission', 'tag6.arbitraryAbility!')->delete(); + + $tag = Tag::find(8); + + $this->assertFalse(User::find(2)->can('arbitraryAbility!', $tag)); + } + + /** + * @test + */ + public function nonrestricted_tag_falls_back_to_global_when_allowed() + { + $this->prepareDatabase([ + 'group_permission' => [ + ['group_id' => Group::MEMBER_ID, 'permission' => 'arbitraryAbility!'] + ] + ]); + + $this->app(); + + $tag = Tag::find(1); + + $this->assertTrue(User::find(2)->can('arbitraryAbility!', $tag)); + } + + /** + * @test + */ + public function nonrestricted_tag_falls_back_to_global_when_not_allowed() + { + $this->app(); + + $tag = Tag::find(1); + + $this->assertFalse(User::find(2)->can('arbitraryAbility!', $tag)); + } +} diff --git a/extensions/tags/tests/integration/visibility/DiscussionVisibilityTest.php b/extensions/tags/tests/integration/visibility/DiscussionVisibilityTest.php index 77039dbbf..f47a5d2cf 100644 --- a/extensions/tags/tests/integration/visibility/DiscussionVisibilityTest.php +++ b/extensions/tags/tests/integration/visibility/DiscussionVisibilityTest.php @@ -7,13 +7,15 @@ * LICENSE file that was distributed with this source code. */ -namespace Flarum\Tags\Tests\integration\visibility; +namespace Flarum\Tags\Tests\integration\api\discussions; -use Carbon\Carbon; +use Flarum\Discussion\Discussion; use Flarum\Group\Group; use Flarum\Tags\Tests\integration\RetrievesRepresentativeTags; use Flarum\Testing\integration\RetrievesAuthorizedUsers; use Flarum\Testing\integration\TestCase; +use Flarum\User\Guest; +use Flarum\User\User; use Illuminate\Support\Arr; class DiscussionVisibilityTest extends TestCase @@ -31,19 +33,36 @@ class DiscussionVisibilityTest extends TestCase $this->extension('flarum-tags'); $this->prepareDatabase([ + 'tags' => $this->tags(), + 'users' => [ + $this->normalUser(), + ], + 'group_permission' => [ + ['group_id' => Group::MEMBER_ID, 'permission' => 'tag5.arbitraryAbility'], + ['group_id' => Group::MEMBER_ID, 'permission' => 'tag8.arbitraryAbility'], + ['group_id' => Group::MEMBER_ID, 'permission' => 'tag11.arbitraryAbility'], + ['group_id' => Group::MEMBER_ID, 'permission' => 'tag13.arbitraryAbility'], + ['group_id' => Group::MEMBER_ID, 'permission' => 'tag14.arbitraryAbility'], + ['group_id' => Group::MEMBER_ID, 'permission' => 'arbitraryAbility'], + ['group_id' => Group::GUEST_ID, 'permission' => 'arbitraryAbility'] + ], 'discussions' => [ - ['id' => 1, 'title' => 'no tags', 'created_at' => Carbon::now()->toDateTimeString(), 'user_id' => 1, 'comment_count' => 1], - ['id' => 2, 'title' => 'open tags', 'created_at' => Carbon::now()->toDateTimeString(), 'user_id' => 1, 'comment_count' => 1], - ['id' => 3, 'title' => 'open tag, restricted child tag', 'created_at' => Carbon::now()->toDateTimeString(), 'user_id' => 1, 'comment_count' => 1], - ['id' => 4, 'title' => 'open tag, one restricted secondary tag', 'created_at' => Carbon::now()->toDateTimeString(), 'user_id' => 1, 'comment_count' => 1], - ['id' => 5, 'title' => 'all closed', 'created_at' => Carbon::now()->toDateTimeString(), 'user_id' => 1, 'comment_count' => 1], + ['id' => 1, 'title' => 'no tags', 'user_id' => 1, 'comment_count' => 1], + ['id' => 2, 'title' => 'open tags', 'user_id' => 1, 'comment_count' => 1], + ['id' => 3, 'title' => 'open tag, restricted child tag', 'user_id' => 1, 'comment_count' => 1], + ['id' => 4, 'title' => 'open tag, one restricted secondary tag', 'user_id' => 1, 'comment_count' => 1], + ['id' => 5, 'title' => 'all closed', 'user_id' => 1, 'comment_count' => 1], + ['id' => 6, 'title' => 'closed parent, open child tag', 'user_id' => 1, 'comment_count' => 1], + ['id' => 7, 'title' => 'one closed primary tag', 'user_id' => 1, 'comment_count' => 1], ], 'posts' => [ - ['id' => 1, 'discussion_id' => 1, 'created_at' => Carbon::now()->toDateTimeString(), 'user_id' => 1, 'type' => 'comment', 'content' => '

'], - ['id' => 2, 'discussion_id' => 2, 'created_at' => Carbon::now()->toDateTimeString(), 'user_id' => 1, 'type' => 'comment', 'content' => '

'], - ['id' => 3, 'discussion_id' => 3, 'created_at' => Carbon::now()->toDateTimeString(), 'user_id' => 1, 'type' => 'comment', 'content' => '

'], - ['id' => 4, 'discussion_id' => 4, 'created_at' => Carbon::now()->toDateTimeString(), 'user_id' => 1, 'type' => 'comment', 'content' => '

'], - ['id' => 5, 'discussion_id' => 5, 'created_at' => Carbon::now()->toDateTimeString(), 'user_id' => 1, 'type' => 'comment', 'content' => '

'], + ['id' => 1, 'discussion_id' => 1, 'user_id' => 1, 'type' => 'comment', 'content' => '

'], + ['id' => 2, 'discussion_id' => 2, 'user_id' => 1, 'type' => 'comment', 'content' => '

'], + ['id' => 3, 'discussion_id' => 3, 'user_id' => 1, 'type' => 'comment', 'content' => '

'], + ['id' => 4, 'discussion_id' => 4, 'user_id' => 1, 'type' => 'comment', 'content' => '

'], + ['id' => 5, 'discussion_id' => 5, 'user_id' => 1, 'type' => 'comment', 'content' => '

'], + ['id' => 6, 'discussion_id' => 6, 'user_id' => 1, 'type' => 'comment', 'content' => '

'], + ['id' => 7, 'discussion_id' => 7, 'user_id' => 1, 'type' => 'comment', 'content' => '

'], ], 'discussion_tag' => [ ['discussion_id' => 2, 'tag_id' => 1], @@ -54,16 +73,10 @@ class DiscussionVisibilityTest extends TestCase ['discussion_id' => 5, 'tag_id' => 6], ['discussion_id' => 5, 'tag_id' => 7], ['discussion_id' => 5, 'tag_id' => 8], + ['discussion_id' => 6, 'tag_id' => 12], + ['discussion_id' => 6, 'tag_id' => 13], + ['discussion_id' => 7, 'tag_id' => 14], ], - 'tags' => $this->tags(), - 'users' => [ - $this->normalUser(), - ], - 'group_permission' => [ - ['group_id' => Group::MEMBER_ID, 'permission' => 'tag5.viewDiscussions'], - ['group_id' => Group::MEMBER_ID, 'permission' => 'tag8.viewDiscussions'], - ['group_id' => Group::MEMBER_ID, 'permission' => 'tag11.viewDiscussions'] - ] ]); } @@ -72,18 +85,13 @@ class DiscussionVisibilityTest extends TestCase */ public function admin_sees_all() { - $response = $this->send( - $this->request('GET', '/api/discussions', [ - 'authenticatedAs' => 1 - ]) - ); + $this->app(); - $this->assertEquals(200, $response->getStatusCode()); + $user = User::find(1); + $discussions = Discussion::whereVisibleTo($user, 'arbitraryAbility')->get(); - $data = json_decode($response->getBody()->getContents(), true)['data']; - - $ids = Arr::pluck($data, 'id'); - $this->assertEqualsCanonicalizing(['1', '2', '3', '4', '5'], $ids); + $ids = Arr::pluck($discussions, 'id'); + $this->assertEqualsCanonicalizing([1, 2, 3, 4, 5, 6, 7], $ids); } /** @@ -91,37 +99,54 @@ class DiscussionVisibilityTest extends TestCase */ public function user_sees_where_allowed() { - $response = $this->send( - $this->request('GET', '/api/discussions', [ - 'authenticatedAs' => 2 - ]) - ); + $this->app(); - $this->assertEquals(200, $response->getStatusCode()); + $user = User::find(2); + $discussions = Discussion::whereVisibleTo($user, 'arbitraryAbility')->get(); - $data = json_decode($response->getBody()->getContents(), true)['data']; - - // 5 isnt included because parent access doesnt necessarily give child access - // 6, 7, 8 aren't included because child access shouldnt work unless parent - // access is also given. - $ids = Arr::pluck($data, 'id'); - $this->assertEqualsCanonicalizing(['1', '2', '3', '4'], $ids); + $ids = Arr::pluck($discussions, 'id'); + $this->assertEqualsCanonicalizing([1, 2, 3, 4, 7], $ids); } /** * @test */ - public function guest_cant_see_restricted_or_children_of_restricted() + public function user_sees_only_in_restricted_tags_without_global_perm() { - $response = $this->send( - $this->request('GET', '/api/discussions') - ); + $this->database()->table('group_permission')->where('permission', 'arbitraryAbility')->delete(); - $this->assertEquals(200, $response->getStatusCode()); + $user = User::find(2); + $discussions = Discussion::whereVisibleTo($user, 'arbitraryAbility')->get(); - $data = json_decode($response->getBody()->getContents(), true)['data']; + $ids = Arr::pluck($discussions, 'id'); + $this->assertEqualsCanonicalizing([7], $ids); + } - $ids = Arr::pluck($data, 'id'); - $this->assertEqualsCanonicalizing(['1', '2'], $ids); + /** + * @test + */ + public function guest_can_see_where_allowed() + { + $this->app(); + + $user = new Guest(); + $discussions = Discussion::whereVisibleTo($user, 'arbitraryAbility')->get(); + + $ids = Arr::pluck($discussions, 'id'); + $this->assertEqualsCanonicalizing([1, 2], $ids); + } + + /** + * @test + */ + public function guest_cant_see_without_global_perm() + { + $this->database()->table('group_permission')->where('permission', 'arbitraryAbility')->delete(); + + $user = new Guest(); + $discussions = Discussion::whereVisibleTo($user, 'arbitraryAbility')->get(); + + $ids = Arr::pluck($discussions, 'id'); + $this->assertEqualsCanonicalizing([], $ids); } }