From 12dfcc5c7960e8dc1e35e9d0a10d071923b39a76 Mon Sep 17 00:00:00 2001 From: Sami Mazouz Date: Tue, 10 Jan 2023 12:45:29 +0100 Subject: [PATCH] Merge pull request from GHSA-hph3-hv3c-7725 * test: add reply creation tests Signed-off-by: Sami Mazouz * fix: access checking being bypassed for post creation when first post is deleted Signed-off-by: Sami Mazouz * chore: recover tests Signed-off-by: Sami Mazouz * chore: make provider public Signed-off-by: Sami Mazouz Signed-off-by: Sami Mazouz --- .../Command/StartDiscussionHandler.php | 2 +- framework/core/src/Post/Command/PostReply.php | 8 +++- .../src/Post/Command/PostReplyHandler.php | 2 +- .../integration/api/posts/CreateTest.php | 47 ++++++++++++++++--- 4 files changed, 50 insertions(+), 9 deletions(-) diff --git a/framework/core/src/Discussion/Command/StartDiscussionHandler.php b/framework/core/src/Discussion/Command/StartDiscussionHandler.php index bda8cb439..25f7b09bd 100644 --- a/framework/core/src/Discussion/Command/StartDiscussionHandler.php +++ b/framework/core/src/Discussion/Command/StartDiscussionHandler.php @@ -79,7 +79,7 @@ class StartDiscussionHandler // We will do this by running the PostReply command. try { $post = $this->bus->dispatch( - new PostReply($discussion->id, $actor, $data, $ipAddress) + new PostReply($discussion->id, $actor, $data, $ipAddress, true) ); } catch (Exception $e) { $discussion->delete(); diff --git a/framework/core/src/Post/Command/PostReply.php b/framework/core/src/Post/Command/PostReply.php index ae1d4c03c..226464926 100644 --- a/framework/core/src/Post/Command/PostReply.php +++ b/framework/core/src/Post/Command/PostReply.php @@ -41,17 +41,23 @@ class PostReply */ public $ipAddress; + /** + * @var bool + */ + public $isFirstPost; + /** * @param int $discussionId The ID of the discussion to post the reply to. * @param User $actor The user who is performing the action. * @param array $data The attributes to assign to the new post. * @param string $ipAddress The IP address of the actor. */ - public function __construct($discussionId, User $actor, array $data, $ipAddress = null) + public function __construct($discussionId, User $actor, array $data, $ipAddress = null, bool $isFirstPost = false) { $this->discussionId = $discussionId; $this->actor = $actor; $this->data = $data; $this->ipAddress = $ipAddress; + $this->isFirstPost = $isFirstPost; } } diff --git a/framework/core/src/Post/Command/PostReplyHandler.php b/framework/core/src/Post/Command/PostReplyHandler.php index b0bfb8877..f634a2dac 100644 --- a/framework/core/src/Post/Command/PostReplyHandler.php +++ b/framework/core/src/Post/Command/PostReplyHandler.php @@ -74,7 +74,7 @@ class PostReplyHandler // If this is the first post in the discussion, it's technically not a // "reply", so we won't check for that permission. - if ($discussion->first_post_id !== null) { + if (! $command->isFirstPost) { $actor->assertCan('reply', $discussion); } diff --git a/framework/core/tests/integration/api/posts/CreateTest.php b/framework/core/tests/integration/api/posts/CreateTest.php index a69d42783..41b61276d 100644 --- a/framework/core/tests/integration/api/posts/CreateTest.php +++ b/framework/core/tests/integration/api/posts/CreateTest.php @@ -10,6 +10,7 @@ namespace Flarum\Tests\integration\api\posts; use Carbon\Carbon; +use Flarum\Group\Group; use Flarum\Testing\integration\RetrievesAuthorizedUsers; use Flarum\Testing\integration\TestCase; @@ -26,36 +27,70 @@ class CreateTest extends TestCase $this->prepareDatabase([ 'discussions' => [ - ['id' => 1, 'title' => __CLASS__, 'created_at' => Carbon::now()->toDateTimeString(), 'user_id' => 2], + ['id' => 1, 'title' => __CLASS__, 'created_at' => Carbon::now()->toDateTimeString(), 'user_id' => 2, 'first_post_id' => 1], + // Discussion with deleted first post. + ['id' => 2, 'title' => __CLASS__, 'created_at' => Carbon::now()->toDateTimeString(), 'user_id' => 2, 'first_post_id' => null], + ], + 'posts' => [ + ['id' => 1, 'discussion_id' => 1, 'number' => 1, 'created_at' => Carbon::now()->subDay()->toDateTimeString(), 'user_id' => 2, 'type' => 'comment', 'content' => ''], ], 'users' => [ $this->normalUser(), - ] + ['id' => 3, 'username' => 'restricted', 'email' => 'restricted@machine.local', 'is_email_confirmed' => 1], + ], + 'groups' => [ + ['id' => 40, 'name_singular' => 'tess', 'name_plural' => 'tess'], + ], + 'group_user' => [ + ['group_id' => 40, 'user_id' => 3], + ], + 'group_permission' => [ + ['group_id' => 40, 'permission' => 'discussion.reply'], + ], ]); } /** + * @dataProvider discussionRepliesPrvider * @test */ - public function can_create_reply() + public function can_create_reply_if_allowed(int $actorId, int $discussionId, int $responseStatus) { + // Reset permissions for normal users group. + $this->database() + ->table('group_permission') + ->where('permission', 'discussion.reply') + ->where('group_id', Group::MEMBER_ID) + ->delete(); + $response = $this->send( $this->request('POST', '/api/posts', [ - 'authenticatedAs' => 2, + 'authenticatedAs' => $actorId, 'json' => [ 'data' => [ 'attributes' => [ 'content' => 'reply with predetermined content for automated testing - too-obscure', ], 'relationships' => [ - 'discussion' => ['data' => ['id' => 1]], + 'discussion' => ['data' => ['id' => $discussionId]], ], ], ], ]) ); - $this->assertEquals(201, $response->getStatusCode()); + $this->assertEquals($responseStatus, $response->getStatusCode()); + } + + public function discussionRepliesPrvider(): array + { + return [ + // [$actorId, $discussionId, $responseStatus] + 'can_create_reply_with_ability' => [3, 1, 201], + 'cannot_create_reply_without_ability' => [2, 1, 403], + 'can_create_reply_with_ability_when_first_post_is_deleted' => [3, 2, 201], + 'cannot_create_reply_without_ability_when_first_post_is_deleted' => [2, 2, 403], + ]; } /**