python-discord / sir-robin

Our event bot, for managing community events.
MIT License
18 stars 14 forks source link

Add `cj pin` and `cj unpin` #56

Closed D0rs4n closed 2 years ago

D0rs4n commented 2 years ago

Lets Code Jam participants pin messages in their own team channels. Check done via the Code Jam MGMT api. Closes #53

D0rs4n commented 2 years ago

In my opinion, if this command is ran outside of the code jam team category, it should silently fail. Sir-Robin has no reason to respond to this command outside of that context.

If it is inside the code jam category, then you can assume the user running the command has permission to pin the message, as no other users will be in those channels.

If we really wanted to be safe, we can still check that the user invoking the command is a participant, but it's not 100% needed with the above solution.

As for you first suggestion I agree, it should silently fail. However, I don't think we should rely on categories, those categories are created as we go, hence we don't add them as environment variables, so we would have to rely to their names, which feels like something that could easily break. (I think we discussed a similar issue when creating those categories). That's why I wanted to stick with the API.

ChrisLovering commented 2 years ago

In my opinion, if this command is ran outside of the code jam team category, it should silently fail. Sir-Robin has no reason to respond to this command outside of that context. If it is inside the code jam category, then you can assume the user running the command has permission to pin the message, as no other users will be in those channels. If we really wanted to be safe, we can still check that the user invoking the command is a participant, but it's not 100% needed with the above solution.

As for you first suggestion I agree, it should silently fail. However, I don't think we should rely on categories, those categories are created as we go, hence we don't add them as environment variables, so we would have to rely to their names, which feels like something that could easily break. (I think we discussed a similar issue when creating those categories). That's why I wanted to stick with the API.

Using the API seems fine to me.

However, we are still giving error messages to users in other channels. EG if the user isn't a participant or if they didn't reply to a message.

IMO sir-robin should never respond to this command if it is outside of a codejam channel.

Also, on the code side of things, if you have a huge if statement, and then a tiny else statement, it is usually best to do the else statement first and return early.

EG for this it would be

if not isinstance(referenced_message, discord.Message):
    ctx.send( ... )
    return

...
the rest of the code
...
D0rs4n commented 2 years ago

In my opinion, if this command is ran outside of the code jam team category, it should silently fail. Sir-Robin has no reason to respond to this command outside of that context. If it is inside the code jam category, then you can assume the user running the command has permission to pin the message, as no other users will be in those channels. If we really wanted to be safe, we can still check that the user invoking the command is a participant, but it's not 100% needed with the above solution.

As for you first suggestion I agree, it should silently fail. However, I don't think we should rely on categories, those categories are created as we go, hence we don't add them as environment variables, so we would have to rely to their names, which feels like something that could easily break. (I think we discussed a similar issue when creating those categories). That's why I wanted to stick with the API.

Using the API seems fine to me.

However, we are still giving error messages to users in other channels. EG if the user isn't a participant or if they didn't reply to a message.

IMO sir-robin should never respond to this command if it is outside of a codejam channel.

Also, on the code side of things, if you have a huge if statement, and then a tiny else statement, it is usually best to do the else statement first and return early.

EG for this it would be

if not isinstance(referenced_message, discord.Message):
    ctx.send( ... )
    return

...
the rest of the code
...

Resolved in db082ac

D0rs4n commented 2 years ago

These two commands have an authorization issue and maaaybe another one too:

1. The Events Lead cannot use `&cj pin` or `&cj unpin` (poor Kat)

2. Any helper part of the Events Team can abuse `&cj pin` and `&cj unpin` to (un)pin any messages in channels Robin has permission to do so. I have no idea what permissions Robin will be given in the actual server during normal operations, but let's verify this won't be an issue (if not block this route entirely).

1.:The Events Lead can use the command, since it's enabled for the whole Events Team. (which includes the Events Lead) 2.: No the staff cannot abuse this, since it's limited to the Code Jam categories, hence the category, if invoked outside of the Code Jam categories, it'll silently fail.

D0rs4n commented 2 years ago

2 is valid, Stalling the PR while figuring it out.

D0rs4n commented 2 years ago

Resolved all. 🤞 Except for the helper to handle team related MGMT requests, that'll be included in a separate PR.

D0rs4n commented 2 years ago

Wheee this looks so much nicer. Can confirm it works and has no permission issues. Great work @D0rs4n, and thank you @ChrisLovering for bringing some sensibility back into this mess :)

Thank you for your thorough review! :)