errbotio / errbot

Errbot is a chatbot, a daemon that connects to your favorite chat service and bring your tools and some fun into the conversation.
http://errbot.io
GNU General Public License v3.0
3.12k stars 614 forks source link

Don't use str(room) for acl matches #1530

Closed webpigeon closed 2 years ago

webpigeon commented 3 years ago

While fixing the documentation, I noticed that rooms are using str(room) for acl matches. __str__ is meant to be a human-readable object representation which could be something other than the room ID - although as no one spotted this I guess that in most cases it is.

This commit makes it so that rooms have an aclattr, just like people do. This can then be used to store a suitable version of the room for ACL matches.

I've tried to maintain backwards compatibility in two ways:

  1. aclattr defaults to the old behaviour str(self)
  2. the getter can detect if the room (Occupant) is a room or a str, and if it's a str use that instead. I know for the Matrix backend I'd incorrectly been returning a string and it still worked - if other backends are doing that it'll throw an exception without this mitigation.

I've checked using tox, and it passed the tests :).

class MatrixRoom(MatrixIdentifier, backend.Room):
    """Representation of a matrix room.

    Provides a way to do room-related things. We need to be careful because this is one of the main places
    that the async and sync code mixes.
    """

    def __init__(self, mxid: str):
        super().__init__(mxid)
        self.mxid = mxid

   @property
    def aclattr(self) -> str:
        return self.mxid

    def __str__(self):
        return "{} ({})".format(self.display_name, self.machine_name)

in terms of bot usage differences, there shouldn't be any vs the original room-string based approach.

sijis commented 2 years ago

@webpigeon Would it be possible to include an example ACL in your description? Also, would you be able to add a few additional tests to validate the new behavior?

webpigeon commented 2 years ago

Sure - I'll do that this weekend :)

sijis commented 2 years ago

I would still appreciate an example for this.

webpigeon commented 2 years ago

Sorry - got sidetracked, I've added two tests and implementation of the acl attribute for testing purposes. The existing tests should be evidence that the old behaviour is kept if there is no change.

sijis commented 2 years ago

@webpigeon Thank you for your contribution!