8go / matrix-eno-bot

An admin and personal assistence Matrix bot built on matrix-nio and nio-template
Apache License 2.0
86 stars 13 forks source link

Feature Request: invite restriction, Access Control Lists, Permissions #20

Open toushin-taishi opened 3 years ago

toushin-taishi commented 3 years ago

Hello @8go,

Is it possible to specify the account or room that the BOT will accept invites from? I hope you would consider this request in the future release of matrix-eno-bot.

Thank you for your hard work!

troglodyne commented 3 years ago

Another thing that could potentially be of use here is restricting certain commands from execution based on the account or room. That said, currently one can implement command blacklisting/whitelisting for users on a per-command basis now with the ENO_SENDER environment variable. I could still see value in doing it on a bot level however, as then you can restrict certain commands by room as well.

My thoughts RE implementation:

As to the actual code implementation, it seems pretty straightforward, as ultimately it's a matter of just updating the command/config dictionary logic along with getting bot_commands.py to do the right stuff for commands (reply "I'm sorry, I can't let you do that, Dave" when command ACL requirements not met) and callbacks.py to do the right stuff as it regards invites (ignore invites if they are not matching entries in the ACLs).

toushin-taishi commented 3 years ago

This is fantastic, the group ACLs, invite restrictions and command ACLs make the BOT flexible in terms of who can interact with it.

8go commented 3 years ago

Wow, love :heart: your motivation @troglodyne

Needless to say that the idea is of course excellent.

To be honest, I have not thought it through, just spend some 10 minutes thinking about this.

First question that comes to mind: Could someone spoof the sender id? If so, anyone, could then make himself super-user so to speak. We are entering a dangerous zone here. Great care and precautions must be taken. One would assume that matrix prevents spoofing, but a little research on this question would not be wasted.

I sort of see two dimensions: a) users and b) rooms. Maybe it is even better to see it as a 3-dimensional problem:

Would it make sense to allow the creation of lists of groups on all these 3 dimensions. I am thinking about the sudoer file in Linux.

# users is a list of user-groups
users:
   admin-users
     @8go:matrix.org
   trused-users
      @toushin-taishi:matrix.org
      @teodesian:some.other.server
   updaters
      @toushin-taishi:matrix.org
      @teodesian:some.other.server
   rebooters
      @toushin-taishi:matrix.org
     @8go:matrix.org
   inviters
      @teodesian:some.other.server

# rooms is a list of room-groups
rooms:
   demorooms
      #demofoo:matrix.org
      #demobar:matrix.org
   testrooms
      #testfoo:matrix.org
      #testbar:matrix.org
   moviediscussions
      #matrix3:matrix.org
      #mrrobot:matrix.org
   musicrooms
      #rock:matrix.org
      #pop:matrix.org

# groups of commands
commands
   admincommands
      reboot
      reset
      changetime
      update-foo
      update-bar
   updatecommands
      update-foo
      update-bar
   roommanagement
      invite
      kick
   paidservices
      findata
      news
      stockrecommendations

# so far only groups have been defined
# now we assign permission
# maybe triples? user, room, command
# ALL is a special token, means ALL rooms, ALL commands, etc
admin-users ==> admincommands ==> ALL
# one may specify multiple users, rooms and commands in a single line, here 2 room-groups
updaters ==> updatecommands ==> musicrooms moviediscussions
# groups and individual entries (here users) can be mixed
inviters @powerful:matrix.org ==> roommanagement ==> demorooms testrooms
@somebody:matrix.org ==> some-single-command ==> #room1:matrix.org #room2:matrix.org

I am not sure if I am explaining myself with this example. Remember it is a bit like sudoer file. See https://www.linux.com/training-tutorials/configuring-linux-sudoers-file/ There they have groups for

Then they have

So, first the groups are defined in 3 dimensions, then as 4th step: the groups are combined to specify the permissions across the 3 dimensions.

My example is very similar. Also, groups and individual entries can be mixed, in sudoers groups can even be nested, a group can hold a group. In our example, nesting would allow that e.g. admincommands group could hold updatecommands group, etc.

What would the advantages/disadvantages be comparing the model from @troglodyne and this sudoer-like one?

I like @troglodyne example of hold_my_beer a lot. Why? It is the closeness of the specification. Right where you specify/configure the command is also the place where you give permissions. I also like the simplicity. But with this simplicity comes a problem: what if user(group) A can run do_something_very_dangerous.sh in rooms A1 and A2. And, in addition, user(group) B can run do_something_very_dangerous.sh in rooms B1 and B2. The syntax as-is in hold_my_beer whould not allow that.

Several ways of going about it:

a) a central approach: all is stored in a, let's call it, acl.yaml file. All groups and all permissions are there.

b) commands.yaml gets extended to hold/define the groups (rooms and users) somewhere, say on top. Then further down, with each command there is a list of 2-tuples to specify room-user pairs. E.g. command hold_my_beer has 2 new lines:

c) a hybrid of a) and b) . Everything is done like in (a) (centrallized in acl.yaml) but the user has the options to add additional permissions to the commands.yaml file (like in option (b)).

What do you guys @troglodyne @toushin-taishi think about all this? Any feedback welcome.

toushin-taishi commented 3 years ago

Hello @8go, To be honest, when I first open the feature request - my idea of how it will be implemented is almost the same as @troglodyne. But after reading your post, I liked the idea it resembles the sudoers file and the ACL being flexible enough to handle different use-case scenarios is just awesome.

8go commented 3 years ago

Hi @troglodyne and @toushin-taishi ,

I thought about it a little bit more, and I came up with a secondary question, a question that only becomes relevant once the overall strategy/idea is fixed.

Question: Once the idea is thought through and decided on, and one starts implementation, would it make sense to NOT create a separate acl.yaml file and place all the ACL info into the existing commands.yaml file?

The info stays the same, no change to info, just a change to where the info is placed. Is it better to place it into the existing commands.yaml file (instead of creating a new acl.yaml file) just for the purpose of simplicity and keeping the number of config files as small as possible? Just a question, food for thought.

8go commented 3 years ago

See also discussion in Feature Request, issue #3.

Issue #3 and Issue #20 are very related.

Basically, issue #3 says: permissions should not only be given to "users", i.e. @mr_smith:matrix.org but possibly even on "device-id". Mr. Smith could have 2 devices: PC and cell phone. And only the cell-phone device of Mr. Smith should be permitted to do something.

This would make permissioning and ACLs even more fine-grained.

I am not saying that should be implemented right away, but any suggested solution should not prevent future extension to include device-ids for ACL purposes.

Dual-0 commented 1 year ago

my quick and dirty whitelist implementation to prevent someone else from using the bot:

config option under matrix: in config.yaml

matrix:
  whitelist: "@my-user:domain.tld domain.org another-domain.org"

config.py (ende from __init__ method)

        # Whitelist
        self.whitelist = self._get_cfg(
            ["matrix", "whitelist"], default="", required=False)

        # Check if the whitelist option is in the correct format
        if self.whitelist:
            whitelist_items = self.whitelist.split()
            domain_regex = re.compile(r'^[a-zA-Z0-9\-]+\.[a-zA-Z]+$')
            user_id_regex = re.compile(r'^@[a-zA-Z0-9\.\_\=\-\/]+:[a-zA-Z0-9\-]+\.[a-zA-Z]+$')
            for item in whitelist_items:
                if domain_regex.match(item) and ":" not in item and "@" not in item:
                    # Valid server name
                    pass
                elif user_id_regex.match(item):
                    # Valid user ID
                    pass
                else:
                    # Invalid format
                    raise ConfigError(
                        "Whitelist contains invalid whitelist item: {}".format(item))

new def in Callbacks class in callback.py

    def _is_sender_whitelisted(self, sender):
        """Return True if the sender is whitelisted."""
        # Check if the whitelist option is defined in config
        if not hasattr(self.config, "whitelist") or not self.config.whitelist:
            return True

        # Split the whitelist into server names and user IDs
        whitelist = self.config.whitelist.split()
        server_names = []
        user_ids = []
        for item in whitelist:
            if ":" not in item and "@" not in item:
                server_names.append(item)
            elif ":" in item and "@" in item:
                user_ids.append(item)

        # Check if the sender's user ID or server name is in the whitelist
        sender_server = sender.split(":")[1]
        sender_user_id = sender if ":" in sender else None
        if sender_user_id in user_ids or sender_server in server_names:
            return True

        return False

in async def message in callback.py to only answer whitlisted users/servers

        # Check if the sender is whitelisted
        if not self._is_sender_whitelisted(event.sender):
            logger.debug(f"Sender {event.sender} is not whitelisted.")
            return

in async def invite in callback.py to only join invites from whitlisted users/servers

        # Check if the sender is whitelisted
        if not self._is_sender_whitelisted(event.sender):
            logger.debug(f"Sender {event.sender} is not whitelisted.")
            return

I have never created a pull-request, but if there is a need to implement this I will be happy to do so.

8go commented 1 year ago

Thank you @Dual-0 for your contribution. Let's see if someone else has a need for this feature.