From 121e4d3c0e61a05c864dc1c5dfe3a20546f6e8cd Mon Sep 17 00:00:00 2001 From: Ian Morland Date: Fri, 12 Aug 2022 18:11:42 +0200 Subject: [PATCH 1/2] fix: add viewPrivate visibility scope --- .../core/src/Discussion/Access/ScopeDiscussionVisibility.php | 3 +++ 1 file changed, 3 insertions(+) diff --git a/framework/core/src/Discussion/Access/ScopeDiscussionVisibility.php b/framework/core/src/Discussion/Access/ScopeDiscussionVisibility.php index 9875eb63d7..466c6e9988 100644 --- a/framework/core/src/Discussion/Access/ScopeDiscussionVisibility.php +++ b/framework/core/src/Discussion/Access/ScopeDiscussionVisibility.php @@ -54,6 +54,9 @@ public function __invoke(User $actor, $query) ->orWhere('discussions.user_id', $actor->id) ->orWhere(function ($query) use ($actor) { $query->whereVisibleTo($actor, 'editPosts'); + }) + ->orWhere(function ($query) use ($actor) { + $query->whereVisibleTo($actor, 'viewPrivate'); }); }); } From ad47ac3266c3570992f925b1be2349b4f6c9036a Mon Sep 17 00:00:00 2001 From: Sami Mazouz Date: Sun, 21 Aug 2022 23:34:34 +0100 Subject: [PATCH 2/2] test: setup integration tests for current expected behavior Signed-off-by: Sami Mazouz --- .../InteractsWithUnapprovedContent.php | 34 ++++-------- .../integration/api/ListDiscussionsTest.php | 27 ++++----- .../tests/integration/api/ListPostsTest.php | 34 +++++------- .../Access/ScopeDiscussionVisibility.php | 3 - .../integration/api/discussions/ListTest.php | 55 +++++++++++++++++-- 5 files changed, 88 insertions(+), 65 deletions(-) diff --git a/extensions/approval/tests/integration/InteractsWithUnapprovedContent.php b/extensions/approval/tests/integration/InteractsWithUnapprovedContent.php index 3f33097a6a..8cd26d0be7 100644 --- a/extensions/approval/tests/integration/InteractsWithUnapprovedContent.php +++ b/extensions/approval/tests/integration/InteractsWithUnapprovedContent.php @@ -28,8 +28,11 @@ protected function prepareUnapprovedDatabaseContent() ['id' => 3, 'title' => __CLASS__, 'created_at' => Carbon::now(), 'last_posted_at' => Carbon::now(), 'user_id' => 4, 'first_post_id' => 3, 'comment_count' => 1, 'is_approved' => 0, 'is_private' => 1], ['id' => 4, 'title' => __CLASS__, 'created_at' => Carbon::now(), 'last_posted_at' => Carbon::now(), 'user_id' => 4, 'first_post_id' => 4, 'comment_count' => 1, 'is_approved' => 1, 'is_private' => 0], ['id' => 5, 'title' => __CLASS__, 'created_at' => Carbon::now(), 'last_posted_at' => Carbon::now(), 'user_id' => 4, 'first_post_id' => 5, 'comment_count' => 1, 'is_approved' => 1, 'is_private' => 0], - ['id' => 6, 'title' => __CLASS__, 'created_at' => Carbon::now(), 'last_posted_at' => Carbon::now(), 'user_id' => 4, 'first_post_id' => 6, 'comment_count' => 1, 'is_approved' => 0, 'is_private' => 1], + ['id' => 6, 'title' => __CLASS__, 'created_at' => Carbon::now(), 'last_posted_at' => Carbon::now(), 'user_id' => 2, 'first_post_id' => 6, 'comment_count' => 1, 'is_approved' => 0, 'is_private' => 1], ['id' => 7, 'title' => __CLASS__, 'created_at' => Carbon::now(), 'last_posted_at' => Carbon::now(), 'user_id' => 4, 'first_post_id' => 7, 'comment_count' => 1, 'is_approved' => 1, 'is_private' => 0], + + // Normal discussion with first post being private (also means comment_count = 0). + ['id' => 8, 'title' => __CLASS__, 'created_at' => Carbon::now(), 'last_posted_at' => Carbon::now(), 'user_id' => 4, 'first_post_id' => 12, 'comment_count' => 0, 'is_approved' => 1, 'is_private' => 0], ], 'posts' => [ ['id' => 1, 'discussion_id' => 1, 'user_id' => 4, 'type' => 'comment', 'content' => '

Text

', 'is_private' => 0, 'is_approved' => 1, 'number' => 1], @@ -37,39 +40,26 @@ protected function prepareUnapprovedDatabaseContent() ['id' => 3, 'discussion_id' => 3, 'user_id' => 4, 'type' => 'comment', 'content' => '

Text

', 'is_private' => 0, 'is_approved' => 1, 'number' => 1], ['id' => 4, 'discussion_id' => 4, 'user_id' => 4, 'type' => 'comment', 'content' => '

Text

', 'is_private' => 0, 'is_approved' => 1, 'number' => 1], ['id' => 5, 'discussion_id' => 5, 'user_id' => 4, 'type' => 'comment', 'content' => '

Text

', 'is_private' => 0, 'is_approved' => 1, 'number' => 1], - ['id' => 6, 'discussion_id' => 6, 'user_id' => 4, 'type' => 'comment', 'content' => '

Text

', 'is_private' => 0, 'is_approved' => 1, 'number' => 1], + ['id' => 6, 'discussion_id' => 6, 'user_id' => 2, 'type' => 'comment', 'content' => '

Text

', 'is_private' => 0, 'is_approved' => 1, 'number' => 1], ['id' => 7, 'discussion_id' => 7, 'user_id' => 4, 'type' => 'comment', 'content' => '

Text

', 'is_private' => 0, 'is_approved' => 1, 'number' => 1], ['id' => 8, 'discussion_id' => 7, 'user_id' => 4, 'type' => 'comment', 'content' => '

Text

', 'is_private' => 0, 'is_approved' => 1, 'number' => 2], - ['id' => 9, 'discussion_id' => 7, 'user_id' => 4, 'type' => 'comment', 'content' => '

Text

', 'is_private' => 1, 'is_approved' => 0, 'number' => 3], + ['id' => 9, 'discussion_id' => 7, 'user_id' => 2, 'type' => 'comment', 'content' => '

Text

', 'is_private' => 1, 'is_approved' => 0, 'number' => 3], ['id' => 10, 'discussion_id' => 7, 'user_id' => 4, 'type' => 'comment', 'content' => '

Text

', 'is_private' => 0, 'is_approved' => 1, 'number' => 4], ['id' => 11, 'discussion_id' => 7, 'user_id' => 4, 'type' => 'comment', 'content' => '

Text

', 'is_private' => 1, 'is_approved' => 0, 'number' => 5], + + // First post of a normal discussion being private. + ['id' => 12, 'discussion_id' => 8, 'user_id' => 4, 'type' => 'comment', 'content' => '

Text

', 'is_private' => 1, 'is_approved' => 0, 'number' => 1], ], 'groups' => [ - ['id' => 4, 'name_singular' => 'Acme', 'name_plural' => 'Acme', 'is_hidden' => 0] + ['id' => 100, 'name_singular' => 'Acme', 'name_plural' => 'Acme', 'is_hidden' => 0] ], 'group_user' => [ - ['user_id' => 3, 'group_id' => 4] + ['user_id' => 3, 'group_id' => 100] ], 'group_permission' => [ - ['permission' => 'discussion.approvePosts', 'group_id' => 4] + ['permission' => 'discussion.approvePosts', 'group_id' => 100] ] ]); } - - /** - * null: Guest, 2: Normal User. - */ - public function unallowedUsers(): array - { - return [[null], [2]]; - } - - /** - * 1: Admin, 3: Permission Given, 4: Discussions Author. - */ - public function allowedUsers(): array - { - return [[1], [3], [4]]; - } } diff --git a/extensions/approval/tests/integration/api/ListDiscussionsTest.php b/extensions/approval/tests/integration/api/ListDiscussionsTest.php index 2ebb639519..128f228701 100644 --- a/extensions/approval/tests/integration/api/ListDiscussionsTest.php +++ b/extensions/approval/tests/integration/api/ListDiscussionsTest.php @@ -29,10 +29,10 @@ protected function setUp(): void } /** - * @dataProvider unallowedUsers + * @dataProvider userVisibleDiscussionsDataProvider * @test */ - public function can_only_see_approved_if_not_allowed_to_approve(?int $authenticatedAs) + public function can_only_see_approved_if_allowed(?int $authenticatedAs, array $visibleDiscussionIds) { $response = $this->send( $this->request('GET', '/api/discussions', compact('authenticatedAs')) @@ -41,22 +41,17 @@ public function can_only_see_approved_if_not_allowed_to_approve(?int $authentica $body = json_decode($response->getBody()->getContents(), true); $this->assertEquals(200, $response->getStatusCode()); - $this->assertEqualsCanonicalizing([1, 4, 5, 7], Arr::pluck($body['data'], 'id')); + $this->assertEqualsCanonicalizing($visibleDiscussionIds, Arr::pluck($body['data'], 'id')); } - /** - * @dataProvider allowedUsers - * @test - */ - public function can_see_unapproved_if_allowed_to_approve(int $authenticatedAs) + public function userVisibleDiscussionsDataProvider(): array { - $response = $this->send( - $this->request('GET', '/api/discussions', compact('authenticatedAs')) - ); - - $body = json_decode($response->getBody()->getContents(), true); - - $this->assertEquals(200, $response->getStatusCode()); - $this->assertEqualsCanonicalizing([1, 2, 3, 4, 5, 6, 7], Arr::pluck($body['data'], 'id')); + return [ + 'admin can view unapproved discussions' => [1, [1, 2, 3, 4, 5, 6, 7, 8]], + 'user with perms can view unapproved discussions' => [3, [1, 2, 3, 4, 5, 6, 7, 8]], + 'guests cannot view unapproved discussions' => [null, [1, 4, 5, 7]], + 'normal users cannot view unapproved discussions unless being an author 1' => [2, [1, 4, 5, 6, 7]], + 'normal users cannot view unapproved discussions unless being an author 2' => [4, [1, 2, 3, 4, 5, 7, 8]], + ]; } } diff --git a/extensions/approval/tests/integration/api/ListPostsTest.php b/extensions/approval/tests/integration/api/ListPostsTest.php index 844d0f7965..c18c303a74 100644 --- a/extensions/approval/tests/integration/api/ListPostsTest.php +++ b/extensions/approval/tests/integration/api/ListPostsTest.php @@ -29,10 +29,10 @@ protected function setUp(): void } /** - * @dataProvider unallowedUsers + * @dataProvider userVisiblePostsDataProvider * @test */ - public function can_only_see_approved_if_not_allowed_to_approve(?int $authenticatedAs) + public function can_only_see_approved_if_allowed(?int $authenticatedAs, array $visiblePostIds) { $response = $this->send( $this @@ -47,28 +47,22 @@ public function can_only_see_approved_if_not_allowed_to_approve(?int $authentica $body = json_decode($response->getBody()->getContents(), true); $this->assertEquals(200, $response->getStatusCode()); - $this->assertEqualsCanonicalizing([7, 8, 10], Arr::pluck($body['data'], 'id')); + $this->assertEqualsCanonicalizing($visiblePostIds, Arr::pluck($body['data'], 'id')); } - /** - * @dataProvider allowedUsers - * @test - */ - public function can_see_unapproved_if_allowed_to_approve(int $authenticatedAs) + public function userVisiblePostsDataProvider(): array { - $response = $this->send( - $this - ->request('GET', '/api/posts', compact('authenticatedAs')) - ->withQueryParams([ - 'filter' => [ - 'discussion' => 7 - ] - ]) - ); + return [ + // Admin can view unapproved posts. + [1, [7, 8, 9, 10, 11, 12]], - $body = json_decode($response->getBody()->getContents(), true); + // User with approval perms can view unapproved posts. + [3, [7, 8, 9, 10, 11, 12]], - $this->assertEquals(200, $response->getStatusCode()); - $this->assertEqualsCanonicalizing([7, 8, 9, 10, 11], Arr::pluck($body['data'], 'id')); + // Normal users cannot view unapproved posts unless being an author. + [null, [7, 8, 10]], + [2, [7, 8, 9, 10]], + [4, [7, 8, 10, 11, 12]], + ]; } } diff --git a/framework/core/src/Discussion/Access/ScopeDiscussionVisibility.php b/framework/core/src/Discussion/Access/ScopeDiscussionVisibility.php index 466c6e9988..9875eb63d7 100644 --- a/framework/core/src/Discussion/Access/ScopeDiscussionVisibility.php +++ b/framework/core/src/Discussion/Access/ScopeDiscussionVisibility.php @@ -54,9 +54,6 @@ public function __invoke(User $actor, $query) ->orWhere('discussions.user_id', $actor->id) ->orWhere(function ($query) use ($actor) { $query->whereVisibleTo($actor, 'editPosts'); - }) - ->orWhere(function ($query) use ($actor) { - $query->whereVisibleTo($actor, 'viewPrivate'); }); }); } diff --git a/framework/core/tests/integration/api/discussions/ListTest.php b/framework/core/tests/integration/api/discussions/ListTest.php index 63078ddce1..f3b0b510bb 100644 --- a/framework/core/tests/integration/api/discussions/ListTest.php +++ b/framework/core/tests/integration/api/discussions/ListTest.php @@ -27,20 +27,39 @@ protected function setUp(): void parent::setUp(); $this->prepareDatabase([ + 'users' => [ + $this->normalUser(), + ['id' => 3, 'username' => 'papi', 'email' => 'papi@machine.local', 'is_email_confirmed' => 1], + ['id' => 4, 'username' => 'acme', 'email' => 'acme@machine.local', 'is_email_confirmed' => 1], + ], 'discussions' => [ ['id' => 1, 'title' => __CLASS__, 'created_at' => Carbon::createFromDate(1975, 5, 21)->toDateTimeString(), 'last_posted_at' => Carbon::createFromDate(1975, 5, 21)->toDateTimeString(), 'user_id' => 1, 'first_post_id' => 1, 'comment_count' => 1], ['id' => 2, 'title' => 'lightsail in title', 'created_at' => Carbon::createFromDate(1985, 5, 21)->toDateTimeString(), 'last_posted_at' => Carbon::createFromDate(1985, 5, 21)->toDateTimeString(), 'user_id' => 2, 'comment_count' => 1], ['id' => 3, 'title' => 'not in title', 'created_at' => Carbon::createFromDate(1995, 5, 21)->toDateTimeString(), 'last_posted_at' => Carbon::createFromDate(1995, 5, 21)->toDateTimeString(), 'user_id' => 2, 'comment_count' => 1], ['id' => 4, 'title' => 'hidden', 'created_at' => Carbon::createFromDate(2005, 5, 21)->toDateTimeString(), 'last_posted_at' => Carbon::createFromDate(2005, 5, 21)->toDateTimeString(), 'hidden_at' => Carbon::now()->toDateTimeString(), 'user_id' => 1, 'comment_count' => 1], + + // A discussion with a private first post (which means the comment_count = 0 as well). + // comment_count=0 discussions are also considered as hidden discussions. + // @see HiddenFilterGambit + ['id' => 5, 'title' => 'first post private', 'created_at' => Carbon::createFromDate(2007, 5, 21)->toDateTimeString(), 'last_posted_at' => Carbon::createFromDate(2007, 5, 21)->toDateTimeString(), 'user_id' => 4, 'comment_count' => 0, 'first_post_id' => 5, 'is_private' => 0], ], 'posts' => [ ['id' => 1, 'discussion_id' => 1, 'created_at' => Carbon::createFromDate(1975, 5, 21)->toDateTimeString(), 'user_id' => 1, 'type' => 'comment', 'content' => '

foo bar

'], ['id' => 2, 'discussion_id' => 2, 'created_at' => Carbon::createFromDate(1985, 5, 21)->toDateTimeString(), 'user_id' => 2, 'type' => 'comment', 'content' => '

not in text

'], ['id' => 3, 'discussion_id' => 3, 'created_at' => Carbon::createFromDate(1995, 5, 21)->toDateTimeString(), 'user_id' => 2, 'type' => 'comment', 'content' => '

lightsail in text

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

lightsail in text

'], + + // Private first post. + ['id' => 5, 'discussion_id' => 5, 'created_at' => Carbon::createFromDate(2005, 5, 21)->toDateTimeString(), 'user_id' => 4, 'type' => 'comment', 'content' => '

lightsail in text

', 'is_private' => 1], ], - 'users' => [ - $this->normalUser(), + 'groups' => [ + ['id' => 100, 'name_singular' => 'Acme', 'name_plural' => 'Acme', 'is_hidden' => 0] + ], + 'group_user' => [ + ['user_id' => 3, 'group_id' => 100] + ], + 'group_permission' => [ + ['permission' => 'discussion.editPosts', 'group_id' => 100] ] ]); } @@ -200,7 +219,7 @@ public function hidden_filter_works() $data = json_decode($response->getBody()->getContents(), true)['data']; // Order-independent comparison - $this->assertEquals(['4'], Arr::pluck($data, 'id'), 'IDs do not match'); + $this->assertEquals(['5', '4'], Arr::pluck($data, 'id'), 'IDs do not match'); } /** @@ -396,7 +415,7 @@ public function hidden_gambit_works() $data = json_decode($response->getBody()->getContents(), true)['data']; // Order-independent comparison - $this->assertEquals(['4'], Arr::pluck($data, 'id'), 'IDs do not match'); + $this->assertEquals(['5', '4'], Arr::pluck($data, 'id'), 'IDs do not match'); } /** @@ -461,4 +480,32 @@ public function unread_gambit_works_when_negated() // Order-independent comparison $this->assertEqualsCanonicalizing(['1', '2'], Arr::pluck($data, 'id'), 'IDs do not match'); } + + /** + * @dataProvider userViewDiscussionPrivateFirstPostDataProvider + * @test + */ + public function user_can_only_see_discussion_with_private_first_post_if_allowed(?int $authenticatedAs, bool $canSee) + { + $response = $this->send( + $this->request('GET', '/api/discussions', compact('authenticatedAs')) + ); + + $body = json_decode($response->getBody()->getContents(), true); + + $this->assertEquals(200, $response->getStatusCode()); + $method = $canSee ? 'assertContains' : 'assertNotContains'; + $this->{$method}('5', Arr::pluck($body['data'], 'id')); + } + + public function userViewDiscussionPrivateFirstPostDataProvider(): array + { + return [ + 'admin can see discussions with private first posts' => [1, true], + 'guests users cannot see discussions with private first posts' => [null, false], + 'normal users cannot see discussions with private first posts' => [2, false], + 'users with discussion.editPosts perm can see discussions with private first posts' => [3, true], + 'author can see discussions with private first posts' => [4, true], + ]; + } }