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 <sychocouldy@gmail.com>

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))

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();

extensions/tags/src/Access/ScopeDiscussionVisibility.php

@@ -1,45 +0,0 @@
-<?php
-
-/*
- * This file is part of Flarum.
- *
- * For detailed copyright and license information, please view the
- * LICENSE file that was distributed with this source code.
- */
-
-namespace Flarum\Tags\Access;
-
-use Flarum\Tags\Tag;
-use Flarum\User\User;
-use Illuminate\Database\Eloquent\Builder;
-
-class ScopeDiscussionVisibility
-{
-    /**
-     * @param User $actor
-     * @param Builder $query
-     */
-    public function __invoke(User $actor, Builder $query)
-    {
-        // Hide discussions which have tags that the user is not allowed to see, unless an extension overrides this.
-        $query->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');
-        }
-    }
-}

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
+        // Avoid an infinite recursive loop.
-        // visible, then we can check if the user has been granted that
+        if (Str::endsWith($ability, 'InRestrictedTags')) {
-        // permission for any of the discussion's tags.
+            return;
-        $query->whereIn('discussions.id', function ($query) use ($actor, $ability) {
+        }
-            return $query->select('discussion_id')
+
-                ->from('discussion_tag')
+        // `view` is a special case where the permission string is represented by `viewDiscussions`.
-                ->whereIn('tag_id', function ($query) use ($actor, $ability) {
+        $permission = $ability === 'view' ? 'viewDiscussions' : $ability;
-                    Tag::queryIdsWhereCan($query->from('tags'), $actor, 'discussion.'.$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');
+        }
     }
 }

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');
             });

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');
         });
     }
 }

extensions/tags/src/Access/TagPolicy.php

@@ -15,25 +15,21 @@ use Flarum\User\User;
 
 class TagPolicy extends AbstractPolicy
 {
-    /**
+    public function can(User $actor, string $ability, Tag $tag)
-     * @param User $actor
-     * @param Tag $tag
-     * @return bool|null
-     */
-    public function startDiscussion(User $actor, 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);
     }
 }

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
+        return $query
-            ->from('tags as s_tags')
             ->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');
     }
 }

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],
         ];
     }
 }

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' => 2],
-                                    ['type' => 'tags', 'id' => 4]
+                                    ['type' => 'tags', 'id' => 3]
                                 ]
                             ]
                         ]
@@ -231,7 +231,7 @@ class CreateTest extends TestCase
             ])
         );
 
-        $this->assertEquals(422, $response->getStatusCode());
+        $this->assertEquals(201, $response->getStatusCode());
     }
 
     /**

extensions/tags/tests/integration/api/discussions/ListTest.php

@@ -0,0 +1,128 @@
+<?php
+
+/*
+ * This file is part of Flarum.
+ *
+ * For detailed copyright and license information, please view the
+ * LICENSE file that was distributed with this source code.
+ */
+
+namespace Flarum\Tags\Tests\integration\api\discussions;
+
+use Flarum\Group\Group;
+use Flarum\Tags\Tests\integration\RetrievesRepresentativeTags;
+use Flarum\Testing\integration\RetrievesAuthorizedUsers;
+use Flarum\Testing\integration\TestCase;
+use Illuminate\Support\Arr;
+
+class ListTest extends TestCase
+{
+    use RetrievesAuthorizedUsers;
+    use RetrievesRepresentativeTags;
+
+    /**
+     * @inheritDoc
+     */
+    protected function setUp(): void
+    {
+        parent::setUp();
+
+        $this->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' => '<t><p></p></t>'],
+                ['id' => 2, 'discussion_id' => 2, 'user_id' => 1, 'type' => 'comment', 'content' => '<t><p></p></t>'],
+                ['id' => 3, 'discussion_id' => 3, 'user_id' => 1, 'type' => 'comment', 'content' => '<t><p></p></t>'],
+                ['id' => 4, 'discussion_id' => 4, 'user_id' => 1, 'type' => 'comment', 'content' => '<t><p></p></t>'],
+                ['id' => 5, 'discussion_id' => 5, 'user_id' => 1, 'type' => 'comment', 'content' => '<t><p></p></t>'],
+                ['id' => 6, 'discussion_id' => 6, 'user_id' => 1, 'type' => 'comment', 'content' => '<t><p></p></t>'],
+            ],
+            '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);
+    }
+}

extensions/tags/tests/integration/api/discussions/UpdateTest.php

@@ -0,0 +1,301 @@
+<?php
+
+/*
+ * This file is part of Flarum.
+ *
+ * For detailed copyright and license information, please view the
+ * LICENSE file that was distributed with this source code.
+ */
+
+namespace Flarum\Tags\Tests\integration\api\discussions;
+
+use Flarum\Group\Group;
+use Flarum\Tags\Tests\integration\RetrievesRepresentativeTags;
+use Flarum\Testing\integration\RetrievesAuthorizedUsers;
+use Flarum\Testing\integration\TestCase;
+
+class UpdateTest extends TestCase
+{
+    use RetrievesAuthorizedUsers;
+    use RetrievesRepresentativeTags;
+
+    /**
+     * @inheritDoc
+     */
+    protected function setUp(): void
+    {
+        parent::setUp();
+
+        $this->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' => '<t><p>Text</p></t>'],
+            ],
+            '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());
+    }
+}

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
+        $ids = Arr::pluck($data, 'id');
-        $this->assertEquals(['1', '2', '3', '4', '9', '10'], Arr::pluck($data, 'id'));
+        $this->assertEquals(['1', '2', '3', '4', '9', '10'], $ids);
     }
 }

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;

extensions/tags/tests/integration/authorization/TagPolicyTest.php

@@ -0,0 +1,112 @@
+<?php
+
+/*
+ * This file is part of Flarum.
+ *
+ * For detailed copyright and license information, please view the
+ * LICENSE file that was distributed with this source code.
+ */
+
+namespace Flarum\Tags\Tests\integration\authorization;
+
+use Flarum\Group\Group;
+use Flarum\Tags\Tag;
+use Flarum\Tags\Tests\integration\RetrievesRepresentativeTags;
+use Flarum\Testing\integration\RetrievesAuthorizedUsers;
+use Flarum\Testing\integration\TestCase;
+use Flarum\User\User;
+
+class TagPolicyTest extends TestCase
+{
+    use RetrievesAuthorizedUsers;
+    use RetrievesRepresentativeTags;
+
+    /**
+     * @inheritDoc
+     */
+    protected function setUp(): void
+    {
+        parent::setUp();
+
+        $this->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));
+    }
+}

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' => 1, 'title' => 'no tags', 'user_id' => 1, 'comment_count' => 1],
-                ['id' => 2, 'title' => 'open tags', 'created_at' => Carbon::now()->toDateTimeString(), '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', 'created_at' => Carbon::now()->toDateTimeString(), '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', 'created_at'  => Carbon::now()->toDateTimeString(), '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', 'created_at'  => Carbon::now()->toDateTimeString(), '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' => '<t><p></p></t>'],
+                ['id' => 1, 'discussion_id' => 1, 'user_id' => 1, 'type' => 'comment', 'content' => '<t><p></p></t>'],
-                ['id' => 2, 'discussion_id' => 2, 'created_at' => Carbon::now()->toDateTimeString(), 'user_id' => 1, 'type' => 'comment', 'content' => '<t><p></p></t>'],
+                ['id' => 2, 'discussion_id' => 2, 'user_id' => 1, 'type' => 'comment', 'content' => '<t><p></p></t>'],
-                ['id' => 3, 'discussion_id' => 3, 'created_at' => Carbon::now()->toDateTimeString(), 'user_id' => 1, 'type' => 'comment', 'content' => '<t><p></p></t>'],
+                ['id' => 3, 'discussion_id' => 3, 'user_id' => 1, 'type' => 'comment', 'content' => '<t><p></p></t>'],
-                ['id' => 4, 'discussion_id' => 4, 'created_at' => Carbon::now()->toDateTimeString(), 'user_id' => 1, 'type' => 'comment', 'content' => '<t><p></p></t>'],
+                ['id' => 4, 'discussion_id' => 4, 'user_id' => 1, 'type' => 'comment', 'content' => '<t><p></p></t>'],
-                ['id' => 5, 'discussion_id' => 5, 'created_at' => Carbon::now()->toDateTimeString(), 'user_id' => 1, 'type' => 'comment', 'content' => '<t><p></p></t>'],
+                ['id' => 5, 'discussion_id' => 5, 'user_id' => 1, 'type' => 'comment', 'content' => '<t><p></p></t>'],
+                ['id' => 6, 'discussion_id' => 6, 'user_id' => 1, 'type' => 'comment', 'content' => '<t><p></p></t>'],
+                ['id' => 7, 'discussion_id' => 7, 'user_id' => 1, 'type' => 'comment', 'content' => '<t><p></p></t>'],
             ],
             '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->app();
-            $this->request('GET', '/api/discussions', [
-                'authenticatedAs' => 1
-            ])
-        );
 
-        $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($discussions, 'id');
-
+        $this->assertEqualsCanonicalizing([1, 2, 3, 4, 5, 6, 7], $ids);
-        $ids = Arr::pluck($data, 'id');
-        $this->assertEqualsCanonicalizing(['1', '2', '3', '4', '5'], $ids);
     }
 
     /**
@@ -91,37 +99,54 @@ class DiscussionVisibilityTest extends TestCase
      */
     public function user_sees_where_allowed()
     {
-        $response = $this->send(
+        $this->app();
-            $this->request('GET', '/api/discussions', [
-                'authenticatedAs' => 2
-            ])
-        );
 
-        $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([1, 2, 3, 4, 7], $ids);
-        // 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);
     }
 
     /**
      * @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->database()->table('group_permission')->where('permission', 'arbitraryAbility')->delete();
-            $this->request('GET', '/api/discussions')
-        );
 
-        $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);
     }
 }