[Tags] custom visibility scope queries based on tag scoped permission don't return discussions with no tags

[clarkwinkelmann]

Bug Report

Current Behavior Using a custom scope like $discussionEloquentQuery->whereVisibleTo($actor, 'seeMyThing') with the intent for Tags to automatically scope it based on discussion.seeMyThing permission results in discussions with no tags not being returned, even if the global setting allows it.

Steps to Reproduce

1. Create 2 tags: "Tag1" and "Tag2"
2. Set the minimum number of tags to zero
3. Create discussions in each tag, as well as with no tags
4. Register a permission like discussion.seeMyThing
5. Set the global value to "Everyone", set a tag scope so that only Admins have that permission under "Tag1"
6. In an extension, apply $discussionEloquentQuery->whereVisibleTo($actor, 'seeMyThing') on a discussion query builder, for example Discussion::query()->whereVisibleTo($actor, 'seeMyThing')->get()
7. Try running the query as $actor guest or admin
8. Admin receives discussions with tag 1 and 2
9. Guest receives discussions with tag 2
10. Nobody receives the discussions with no tags

Expected Behavior Discussions with no tags should be returned by Tag's ScopeDiscussionVisibilityForAbility when the global permission allows it.

Environment

• Flarum version: beta 15

Possible Solution

The fix will probably need to be done in https://github.com/flarum/tags/blob/master/src/Access/ScopeDiscussionVisibilityForAbility.php

Maybe something like this (untested)

        $query->where(function(Builder $query) use($actor, $ability) {
            $query->whereIn('discussions.id', function ($query) use ($actor, $ability) {
                return $query->select('discussion_id')
                    ->from('discussion_tag')
                    ->whereIn('tag_id', Tag::getIdsWhereCan($actor, 'discussion.'.$ability));
            });

            if ($actor->hasPermission('discussion.' . $ability)) {
                $query->orWhereDoesntHave('tags');
            }
        });

Additional Context Affects https://github.com/clarkwinkelmann/flarum-ext-see-past-first-post

Not sure if the problem was already present in beta 13 or not.

[askvortsov1]

The proposed solution makes sense to me. I'd want to see some integration tests for it though when we go to implement.

[stale[bot]]

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. We do this to keep the amount of open issues to a manageable minimum. In any case, thanks for taking an interest in this software and contributing by opening the issue in the first place!

[askvortsov1]

Haven't tested, but I suspect that https://github.com/flarum/tags/pull/128 might have helped here. If not, should be a relatively easy fix: we'd just need to add an else case to https://github.com/flarum/tags/blob/6a4e9d10971c5230e4d0a53a8b1a294802eb962c/src/Access/ScopeDiscussionVisibilityForAbility.php#L64-L66

[clarkwinkelmann]

I can confirm the problem still exists on Flarum 1.0.4 and 1.1.0

There's another problem that's probably related to this as well.

If I give the permissions like this:

• Global: everyone
• Tag 1: admins and privilege group
• Tag 2: not tag-scoped

Then a discussion with both Tag 1 and Tag 2 is also not matched by the query scope when using the non-admin privilege group as actor :thinking:

I'll try to come up with a set of unit tests for this if I find some time.