From 2f37a8bd5838493d8022c80a84bdb003823c8c74 Mon Sep 17 00:00:00 2001 From: Sami Mazouz Date: Mon, 20 Sep 2021 16:19:43 +0100 Subject: [PATCH] Fix discussions hidden from all users including admins (#140) * test: Start by adding tests for this case scenario * fix: All discussions hidden from everyone when minSecondary > 0 and no secondary tags * test: Test the scenario for primary tags as well * fix: Properly fix the general case of more tags required than available * Adjust to only check with the `viewForum` ability * Test Cases for private discussions --- extensions/tags/src/Access/GlobalPolicy.php | 24 +++- .../integration/api/discussions/ListTest.php | 120 ++++++++++++++++++ 2 files changed, 140 insertions(+), 4 deletions(-) diff --git a/extensions/tags/src/Access/GlobalPolicy.php b/extensions/tags/src/Access/GlobalPolicy.php index dc62f9ae9..5c3a27c52 100644 --- a/extensions/tags/src/Access/GlobalPolicy.php +++ b/extensions/tags/src/Access/GlobalPolicy.php @@ -44,15 +44,31 @@ class GlobalPolicy extends AbstractPolicy if (in_array($ability, ['viewForum', 'startDiscussion'])) { if (! isset($enoughPrimary[$actor->id][$ability])) { - $enoughPrimary[$actor->id][$ability] = Tag::whereHasPermission($actor, $ability) + $primaryTagsWhereNeedsPermission = $this->settings->get('flarum-tags.min_primary_tags'); + $primaryTagsWhereHasPermission = Tag::whereHasPermission($actor, $ability) ->where('tags.position', '!=', null) - ->count() >= $this->settings->get('flarum-tags.min_primary_tags'); + ->count(); + + if ($ability === 'viewForum') { + $primaryTagsCount = Tag::query()->where('position', '!=', null)->count(); + $enoughPrimary[$actor->id][$ability] = $primaryTagsWhereHasPermission >= min($primaryTagsCount, $primaryTagsWhereNeedsPermission); + } else { + $enoughPrimary[$actor->id][$ability] = $primaryTagsWhereHasPermission >= $primaryTagsWhereNeedsPermission; + } } if (! isset($enoughSecondary[$actor->id][$ability])) { - $enoughSecondary[$actor->id][$ability] = Tag::whereHasPermission($actor, $ability) + $secondaryTagsWhereNeedsPermission = $this->settings->get('flarum-tags.min_secondary_tags'); + $secondaryTagsWhereHasPermission = Tag::whereHasPermission($actor, $ability) ->where('tags.position', '=', null) - ->count() >= $this->settings->get('flarum-tags.min_secondary_tags'); + ->count(); + + if ($ability === 'viewForum') { + $secondaryTagsCount = Tag::query()->where(['position' => null, 'parent_id' => null])->count(); + $enoughSecondary[$actor->id][$ability] = $secondaryTagsWhereHasPermission >= min($secondaryTagsCount, $secondaryTagsWhereNeedsPermission); + } else { + $enoughSecondary[$actor->id][$ability] = $secondaryTagsWhereHasPermission >= $secondaryTagsWhereNeedsPermission; + } } if ($enoughPrimary[$actor->id][$ability] && $enoughSecondary[$actor->id][$ability]) { diff --git a/extensions/tags/tests/integration/api/discussions/ListTest.php b/extensions/tags/tests/integration/api/discussions/ListTest.php index c204067e3..1a70409eb 100644 --- a/extensions/tags/tests/integration/api/discussions/ListTest.php +++ b/extensions/tags/tests/integration/api/discussions/ListTest.php @@ -47,6 +47,12 @@ class ListTest extends TestCase ['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' => 'private, no tags', 'user_id' => 1, 'comment_count' => 1, 'is_private' => 1], + ['id' => 8, 'title' => 'private, open tags', 'user_id' => 1, 'comment_count' => 1, 'is_private' => 1], + ['id' => 9, 'title' => 'private, open tag, restricted child tag', 'user_id' => 1, 'comment_count' => 1, 'is_private' => 1], + ['id' => 10, 'title' => 'private, open tag, one restricted secondary tag', 'user_id' => 1, 'comment_count' => 1, 'is_private' => 1], + ['id' => 11, 'title' => 'private, all closed', 'user_id' => 1, 'comment_count' => 1, 'is_private' => 1], + ['id' => 12, 'title' => 'private, closed parent, open child tag', 'user_id' => 1, 'comment_count' => 1, 'is_private' => 1], ], 'posts' => [ ['id' => 1, 'discussion_id' => 1, 'user_id' => 1, 'type' => 'comment', 'content' => '

'], @@ -55,18 +61,34 @@ class ListTest extends TestCase ['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' => '

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

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

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

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

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

'], ], 'discussion_tag' => [ ['discussion_id' => 2, 'tag_id' => 1], + ['discussion_id' => 8, 'tag_id' => 1], ['discussion_id' => 3, 'tag_id' => 2], ['discussion_id' => 3, 'tag_id' => 5], + ['discussion_id' => 9, 'tag_id' => 2], + ['discussion_id' => 9, 'tag_id' => 5], ['discussion_id' => 4, 'tag_id' => 1], ['discussion_id' => 4, 'tag_id' => 11], + ['discussion_id' => 10, 'tag_id' => 1], + ['discussion_id' => 10, 'tag_id' => 11], ['discussion_id' => 5, 'tag_id' => 6], ['discussion_id' => 5, 'tag_id' => 7], ['discussion_id' => 5, 'tag_id' => 8], + ['discussion_id' => 11, 'tag_id' => 6], + ['discussion_id' => 11, 'tag_id' => 7], + ['discussion_id' => 11, 'tag_id' => 8], ['discussion_id' => 6, 'tag_id' => 12], ['discussion_id' => 6, 'tag_id' => 13], + ['discussion_id' => 12, 'tag_id' => 12], + ['discussion_id' => 12, 'tag_id' => 13], ], ]); } @@ -125,4 +147,102 @@ class ListTest extends TestCase $ids = Arr::pluck($data, 'id'); $this->assertEqualsCanonicalizing(['1', '2'], $ids); } + + /** + * @test + */ + public function admin_can_see_where_allowed_when_more_primary_tags_are_required_than_available() + { + $this->actor_can_see_where_allowed_when_more_primary_tags_are_required_than_available(1, ['1', '2', '3', '4', '5', '6']); + } + + /** + * @test + */ + public function user_can_see_where_allowed_when_more_primary_tags_are_required_than_available() + { + $this->actor_can_see_where_allowed_when_more_primary_tags_are_required_than_available(2, ['1', '2', '3', '4']); + } + + /** + * @test + */ + public function guest_can_see_where_allowed_when_more_primary_tags_are_required_than_available() + { + $this->actor_can_see_where_allowed_when_more_primary_tags_are_required_than_available(0, ['1', '2']); + } + + /** + * @test + */ + public function admin_can_see_where_allowed_when_more_secondary_tags_are_required_than_available() + { + $this->actor_can_see_where_allowed_when_more_secondary_tags_are_required_than_available(1, ['1', '2', '3', '4', '5', '6']); + } + + /** + * @test + */ + public function user_can_see_where_allowed_when_more_secondary_tags_are_required_than_available() + { + $this->actor_can_see_where_allowed_when_more_secondary_tags_are_required_than_available(2, ['1', '2', '3', '4']); + } + + /** + * @test + */ + public function guest_can_see_where_allowed_when_more_secondary_tags_are_required_than_available() + { + $this->actor_can_see_where_allowed_when_more_secondary_tags_are_required_than_available(0, ['1', '2']); + } + + public function actor_can_see_where_allowed_when_more_primary_tags_are_required_than_available(int $actorId, array $expectedDiscussions) + { + $this->setting('flarum-tags.min_primary_tags', 100); + $this->database()->table('tags') + ->where('position', '!=', null) + ->update(['position' => null]); + + if ($actorId) { + $reqParams = [ + 'authenticatedAs' => $actorId + ]; + } + + $response = $this->send( + $this->request('GET', '/api/discussions', $reqParams ?? []) + ); + + $this->assertEquals(200, $response->getStatusCode()); + + $data = json_decode($response->getBody()->getContents(), true)['data']; + + $ids = Arr::pluck($data, 'id'); + $this->assertEqualsCanonicalizing($expectedDiscussions, $ids); + } + + public function actor_can_see_where_allowed_when_more_secondary_tags_are_required_than_available(int $actorId, array $expectedDiscussions) + { + $this->setting('flarum-tags.min_secondary_tags', 1); + $this->database()->table('tags') + ->where(['position' => null, 'parent_id' => null]) + ->update(['position' => 200]); + + if ($actorId) { + $reqParams = [ + 'authenticatedAs' => $actorId + ]; + } + + $response = $this->send( + $this->request('GET', '/api/discussions', $reqParams ?? []) + ); + + $this->assertEquals(200, $response->getStatusCode()); + + $data = json_decode($response->getBody()->getContents(), true)['data']; + + $ids = Arr::pluck($data, 'id'); + $this->assertEqualsCanonicalizing($expectedDiscussions, $ids); + } }