From fffedb4e1d5cf47a232694a02dacca11276dc086 Mon Sep 17 00:00:00 2001 From: Sami Mazouz Date: Tue, 11 May 2021 21:35:10 +0100 Subject: [PATCH] Only check for bypassTagCounts permission for startDiscussion ability (#130) This doesn't fix anything, but we should explicitly only check the permission for the ability it is meant for, just to be safe in the future. Also add more tests. --- extensions/tags/composer.json | 2 +- extensions/tags/src/Access/GlobalPolicy.php | 10 +-- .../api/discussions/CreateTest.php | 63 +++++++++++++++++++ .../authorization/GlobalPolicyTest.php | 45 +++++++++++++ 4 files changed, 115 insertions(+), 5 deletions(-) diff --git a/extensions/tags/composer.json b/extensions/tags/composer.json index 243eed013..e1c27c20c 100644 --- a/extensions/tags/composer.json +++ b/extensions/tags/composer.json @@ -61,7 +61,7 @@ "test:setup": "Sets up a database for use with integration tests. Execute this only once." }, "require-dev": { - "flarum/core": "0.1.x-dev", + "flarum/core": "0.1.x-dev#b2d053f6865e685ebf005e457d970385377bbb28", "flarum/testing": "*@dev" } } diff --git a/extensions/tags/src/Access/GlobalPolicy.php b/extensions/tags/src/Access/GlobalPolicy.php index a33ff2d2a..726629904 100644 --- a/extensions/tags/src/Access/GlobalPolicy.php +++ b/extensions/tags/src/Access/GlobalPolicy.php @@ -36,11 +36,13 @@ class GlobalPolicy extends AbstractPolicy static $enoughPrimary; static $enoughSecondary; - if (in_array($ability, ['viewDiscussions', 'startDiscussion'])) { - if ($actor->hasPermission($ability) && $actor->hasPermission('bypassTagCounts')) { - return $this->allow(); - } + if ($ability === 'startDiscussion' + && $actor->hasPermission($ability) + && $actor->hasPermission('bypassTagCounts')) { + return $this->allow(); + } + if (in_array($ability, ['viewDiscussions', 'startDiscussion'])) { if (! isset($enoughPrimary[$actor->id][$ability])) { $enoughPrimary[$actor->id][$ability] = Tag::whereHasPermission($actor, $ability) ->where('tags.position', '!=', null) diff --git a/extensions/tags/tests/integration/api/discussions/CreateTest.php b/extensions/tags/tests/integration/api/discussions/CreateTest.php index 81bc6ac90..f8bcc0ae4 100644 --- a/extensions/tags/tests/integration/api/discussions/CreateTest.php +++ b/extensions/tags/tests/integration/api/discussions/CreateTest.php @@ -87,6 +87,34 @@ class CreateTest extends TestCase $this->assertEquals(422, $response->getStatusCode()); } + /** + * @test + */ + public function user_can_create_discussion_without_tags_if_bypass_permission_granted() + { + $this->prepareDatabase([ + 'group_permission' => [ + ['group_id' => Group::MEMBER_ID, 'permission' => 'bypassTagCounts'], + ] + ]); + + $response = $this->send( + $this->request('POST', '/api/discussions', [ + 'authenticatedAs' => 2, + 'json' => [ + 'data' => [ + 'attributes' => [ + 'title' => 'test - too-obscure', + 'content' => 'predetermined content for automated testing - too-obscure', + ], + ] + ], + ]) + ); + + $this->assertEquals(201, $response->getStatusCode()); + } + /** * @test */ @@ -145,6 +173,41 @@ class CreateTest extends TestCase $this->assertEquals(403, $response->getStatusCode()); } + /** + * @test + */ + public function user_cant_create_discussion_in_primary_tag_where_can_view_but_cant_start_with_bypass_permission_granted() + { + $this->prepareDatabase([ + 'group_permission' => [ + ['group_id' => Group::MEMBER_ID, 'permission' => 'bypassTagCounts'], + ] + ]); + + $response = $this->send( + $this->request('POST', '/api/discussions', [ + 'authenticatedAs' => 2, + 'json' => [ + 'data' => [ + 'attributes' => [ + 'title' => 'test - too-obscure', + 'content' => 'predetermined content for automated testing - too-obscure', + ], + 'relationships' => [ + 'tags' => [ + 'data' => [ + ['type' => 'tags', 'id' => 5] + ] + ] + ] + ], + ], + ]) + ); + + $this->assertEquals(403, $response->getStatusCode()); + } + /** * @test */ diff --git a/extensions/tags/tests/integration/authorization/GlobalPolicyTest.php b/extensions/tags/tests/integration/authorization/GlobalPolicyTest.php index a75b219a8..7f52858be 100644 --- a/extensions/tags/tests/integration/authorization/GlobalPolicyTest.php +++ b/extensions/tags/tests/integration/authorization/GlobalPolicyTest.php @@ -113,4 +113,49 @@ class GlobalPolicyTest extends TestCase $this->assertTrue(User::find(2)->can('startDiscussion')); } + + /** + * @test + */ + public function cant_start_discussion_globally_if_permission_in_insufficient_tags_requires_start_discussion_regardless_of_bypass() + { + $this->prepareDatabase([ + 'group_permission' => [ + ['group_id' => Group::MEMBER_ID, 'permission' => 'bypassTagCounts'], + ] + ]); + + $this->database()->table('group_permission')->where('permission', 'startDiscussion')->delete(); + + $this->assertFalse(User::find(2)->can('startDiscussion')); + } + + /** + * @test + */ + public function can_start_discussion_globally_if_start_discussion_and_bypass_allows_regardless_of_tag_count() + { + $this->prepareDatabase([ + 'group_permission' => [ + ['group_id' => Group::MEMBER_ID, 'permission' => 'bypassTagCounts'], + ] + ]); + + $this->app(); + + $this->assertTrue(User::find(2)->can('startDiscussion')); + } + + /** + * @test + */ + public function can_start_discussion_globally_if_sufficient_tags_and_allows_regardless_of_start_discussion_and_bypass() + { + $this->database()->table('group_permission')->where('permission', 'bypassTagCounts')->delete(); + + $this->setting('flarum-tags.min_primary_tags', 0); + $this->setting('flarum-tags.min_secondary_tags', 1); + + $this->assertTrue(User::find(2)->can('startDiscussion')); + } }