[Starboard] Role-channel separation for white/blacklist and refactor for checks

[quachtridat]

Is your feature request related to a problem? Please describe.

This is related to #261, #266, #267, and #269.

Currently (as of commit eccfad), when a post is starred, before posting to an appropriate starboard, the cog checks if both of the following conditions are satisfied:

• The person who starred the post has a permitted role, or no roles at all.
• The channel containing the starred post is a permitted channel.

A permitted role is a Discord role, and:

1. Must be present in the whitelist if the whitelist contains any roles or channels. If the whitelist is empty,
2. Must be absent from the blacklist if the blacklist contains any roles or channels.

The same logic applies for checking whether a Discord (text/category) channel is a permitted channel.

In other words, having at least one channel or one role in the whitelist means that the whitelist is effective, and the similar applies for the blacklist. However, the whitelist takes absolute priority against the blacklist, which means:

• If no blacklist and no whitelist are used, all roles are permitted.
• If only the blacklist is used, only roles that are absent from the blacklist are permitted.
• If only the whitelist is used, only roles that are present in the whitelist are permitted.
• If both the blacklist and the whitelist are used, the whitelist takes priority and only roles that are present in the whitelist are permitted.

This logic indicates that, in order for the cog to work properly, at any time either only the whitelist is used, or only the blacklist is used. Posting this issue, I believe it is better for the cog to be extended so that it can work even when both the whitelist and the blacklist are used. (1)

One more thing worth noting is that the white/blacklist contains both channels and roles. I believe it is better for us to have them separated, because we could have certain channels that are blacklisted, and certain roles that are whitelisted. People who aren't whitelisted won't be able to star things, however for those who are whitelisted they should not be able to star things in blacklisted channels either. With how starboard works right now, it is not possible because the whitelist would then take priority and thus would prevent starring in any other channels even when the channels are not blacklisted, (and it is) because they are not whitelisted. This is essentially what is described in #269. (2)

Describe the solution you'd like

The below is what I believe would work best, and I am ready to have a pull request for it.

When a post is starred, before posting to an appropriate starboard, the cog checks if the person who starred the post has a permitted role (or maybe no roles at all), no any blacklisted roles, and the channel containing the starred post is a permitted channel.

A permitted role is a Discord role, and, satisfies both of these conditions:

1. Must be absent from the role blacklist if the blacklist contains any roles. It is fine if the role blacklist is empty.
2. Must be present in the role whitelist if the whitelist contains any roles. It is fine if the role whitelist is empty.

The similar logic applies for checking whether a Discord (text/category) channel is a permitted channel. The only difference is that the channel blacklist/whitelist contains only (zero or more) channels.

This helps resolve (1) because a role is by default permitted, and it only gets blocked when it is present in the role blacklist, or, if the role blacklist is empty, not present in the role whitelist if the role whitelist is effective. The similar applies for channel check. Because the blacklist is checked first, the cog easily determines whether the role/channel is outright blocked, and if it is not, there is still a whitelist check, in which case the role/channel is considered permitted only if the whitelist is empty, or the whitelist is not empty and contains the said role/channel. The difference is that the cog would not outright reject or permit a role/channel without going through both the role blacklist and whitelist (or the channel blacklist and whitelist). Again, there are 4 lists: a blacklist for roles, a blacklist for channels, a whitelist for roles, and a whitelist for channels.

This also helps resolve (2) because there is a separation between roles and channels. In other words, any one channel only gets checked against the channel blacklist and the channel whitelist, and the similar applies for roles. If I have a whitelisted role, and the channel whitelist is not used, I could star things in any other channels, except for those channels that are blacklisted. However, if my role is present in both the role blacklist and the role whitelist, I cannot star things because my role is in the blacklist.

To summarize, in general, I believe it is good to prioritize blacklists over whitelists, and to have roles separated from channels for such lists.

Describe alternatives you've considered

I have considered sticking to how the cog works at the moment. However, it is often times not uncommon for a server to have a good number of channels, a good number of roles, some particular channels (e.g., spamming or non-NSFW sensitive/private topics) that should be blocked from starboard(s), and some certain roles that are whitelisted (e.g., verified role or member role). We can probably come up with more situations, but the common theme is that the number of channels and roles can be a high number. Adding them one by one into blacklist(s) or whitelist(s) is not a good experience, and even if we extend the commands so that they can accept multiple arguments, it may look overwhelming when we need to check information of a starboard (via [p]starboard info).

Additional context

I do share the problem with #261 and #269 so I decided to suggest a solution. Feedbacks are welcomed and I am ready to discuss further either here or in @TrustyJAID Discord server if needed.

[TrustyJAID]

Hello, thanks for the well put together issue. I must admit I did not read it all but I had an inkling in the back of my head on what the actual issue is and implemented what I think might be a decent fix for the issues described. Please update the cog and let me know if it solves your problem!

[quachtridat]

@TrustyJAID I did test it on my server, however it did not work. The problem was here.

Trusty-cogs/starboard/starboard_entry.py

Lines 77 to 95 in d463313

	whitelisted_roles = [
	guild.get_role(rid).id for rid in self.whitelist if guild.get_role(rid) is not None
	]
	blacklisted_roles = [
	guild.get_role(rid).id for rid in self.blacklist if guild.get_role(rid) is not None
	]
	if whitelisted_roles:
	# only count if the whitelist contains actual roles
	for role in whitelisted_roles:
	if role in member.roles:
	return True
	return False
	# Since we'd normally return True
	# if there is a whitelist we want to ensure only whitelisted
	# roles can starboard something
	if blacklisted_roles:
	for role in blacklisted_roles:
	if role in member.roles:
	return False

The two lists blacklist_roles and whitelist_roles contain role IDs, which are ints, yet the if condition checks each of those ints against discord.Role objects in member.roles. Therefore, the condition, I believe, would always result in False.

I'm using this temp-fix on my end.

        member_roles = [role.id for role in member.roles]
        whitelisted_roles = [
            guild.get_role(rid).id for rid in self.whitelist if guild.get_role(rid) is not None
        ]
        blacklisted_roles = [
            guild.get_role(rid).id for rid in self.blacklist if guild.get_role(rid) is not None
        ]
        if whitelisted_roles:
            # only count if the whitelist contains actual roles
            for role in whitelisted_roles:
                if role in member_roles:
                    return True
            return False
            # Since we'd normally return True
            # if there is a whitelist we want to ensure only whitelisted
            # roles can starboard something
        if blacklisted_roles:
            for role in blacklisted_roles:
                if role in member_roles:
                    return False