ACL's are definitely broken with Slack backend

[rsalmond]

I just noticed that the ACL check bases it's decision on the senders handle field.

The Slack backend sets this value with the name field it gets from the Slack API.

On free Slack account the name field is set to my displayname, but on my work account the name field is set to my actual first name. (Possibly this is something the admins optionally enabled? I will try to find out more during the week.) Some testing with the API testing tool reveals that every user in my org has the name field set to their first name lowercased.

So anyone named "rob" is going to be able to run the bot commands restricted to me. I don't know what else it might break but a naive fix would be to update the backend to use the displayname field. If you think that is okay I will submit a PR.

[rsalmond]

Update, I have conferred with some folks on other paid slack accounts. They do not see this same behaviour. Something interesting about our account, I will get in touch with slack to find out what it is.

[rsalmond]

Update again. Heard back from Slack support. Their response: "Display names are a fairly new concept at Slack, there used only be usernames but those are getting slowly phased out. If you look further down on your response you'll see values for display_name . Some user's display name and username are sometimes the same but not always, this depends on how the user set up their profile when initially added to the Workspace. When making API calls always use the user id, these values never change."

For kicks I tried changing my display name / @mention to be identical to my colleague and Slack was happy to allow both of us to use the same name. So I guess neither of these fields is suitable for ACL usage and we should consider using Slack user ID's for this purpose.

[chillipeper]

I guess I am one of the lucky ones that got around this. on my org for some reason the ['user']['name'] attribute in slack is set to the ldap id of each one of us (which also means its unique), so instead of mapping the handle, I have mapped the "name", and acls are working for me.

[rsalmond]

@chillipeper heh, nothing like a nice non deterministic API to deliver a satisfying developer experience! I've been thinking about how to solve this problem. Slack's follow up response on my support case was to suggest using the user ID field as that does not change but it makes for a hard to create and hard to read config.py.

For this use case I feel like email address is globally unique enough and secure enough for our needs. If somebody already owns your email address enough to be able to convince Slack to let them use it then bypassing Will ACL's is probably low on your list of concerns.

That said it may simply be that trying to find a suitable unique field for checking ACL's that works across Backends is just not feasible. Eg. in #342 there's a discussion about adding a full blown SMS IO backend. ACL's based on email would go straight out the window then, but there's no reason we have to make that decision for users I don't think.

Perhaps the ACL configuration could to be backend specific? It could read something like this maybe.

acl = {
  'admins': [
    {'backend': 'slack', 'field': 'email', 'value': <email_address_a>},
    {'backend': 'sms', 'field': 'phone_number', 'value': <phone_number_a>},
    ...
  ]
}

[pastorhudson]

I've been thinking about the need for a web control panel to make it easier to control settings like whitelisting rooms(something I built for our internal bot), and turning announcements on and off. It may be time to look at a real user system for admins?

<brain dump>

In the spirit of what @skoczen just posted about making it easy it may be time for a web dashboard.

• We create a default admin:password login
• We build a basic user admin dashboard that you login using the admin
• We make a wizard that asks what plugins you want enabled
• We connect those plugins using OAuth 2 rather than tokens in environment variables
• Once you have a backend connected you could then select the users that need to be made admins and we could handle the storing of the necessary info without requiring the custom coding of config.py acl.

</dump>

When I first installed willbot getting the ACL working was super frustrating. It took a lot of trial and error. I have people running pcobot by clicking a "Deploy to Heroku" button. These people do not know how to edit a config.py file. I've tried to make adding people to the acl easier. I wrote an acl plugin that lists the acl as a troubleshooting step. This could be expanded to add and remove people as a stop gap before we get a web-dashboard if that is what we want to do.

Currently I use the depriciated WILL_ADMINS= in an environment variable. You have to put the first part of their slack email before the @ sign.
So if you want to add john.doe@gmail.com and billgates@microsoft.com you'd put:

WILL_ADMINS=john.doe;billgates

And that will work with slack.
Alternatively you can put:

ACL = {
    "admins": ["john.doe", "billgates"],
}

And that will work for slack too, but it's not very secure.

[chillipeper]

@rsalmond +1 on the approach of ACLs being backend specific. This will give us the granularity in acls we need across backends.

@pastorhudson the idea of having an admin web control panel sounds interesting, but I don't understand fully how could this be implemented with the current ACLs we have.

[pastorhudson]

@chillipeper I admit I may be totally off base. I really appreciate being a part of this community and the feedback pushes me to think and be a better dev.

Assumptions:
My current use of the ACL is very basic. I have "bot-admins" and then everybody else is just not in the ACL. But I think this could still work if you have say "admins, engineers, team-lead" etc.

Non-Breaking Change:

1. Using the current Config.py and current ACL we would have the admin web panel read and write the config.py file. So initial "admin" for web-control panel would be a default or randomly generated password on first run. That web admin would be a separate user and password the acl.

2. Once your're logged into the web-admin you could get users from the current back ends and groups from the current config.py acl. You could add groups, and add users too groups using the web panel. The logic for exactly what needs to go in the config.py acl for each back end to work would be handled by the web panel. To the user they would just be selecting Slack users to add to the groups, and rocket chat users to add to the group.

Slack wants the first part of my email address. Theoretical SMS Twillio back end needs a phone number. So I select the TWILLIO users and slack users to add to "engineers" group and both phone and first part of email would be added. If another back end needs full email then that full email would be added too.

To make this more user friendly we could implement an sqlite database, or use Redis to store a real User Model that has Auto-generated Primary Key, Name, Email, Phone, and any relevant back end handles. Then as a user is updated the acl would be automatically updated.

Breaking Changes:
If we carried this further we could remove ACL from config.py and just store it in sqlite, or Redis. This would allow us to update the acl through a nice UI without restarting the bot.

[rsalmond]

There are a few ideas floating around here I want to tease apart.

1. Backend specific ACL's

The idea here is to select a per io_backend field which is guaranteed to uniquely identify users and have the ACL system check that field for access tests. This would resolve the issue we're seeing on my work Slack account.

2. On the fly Will configuration

A potent idea I am game to explore further. Some little bells in the back of my head are ringing about some thread I saw somewhere in which @skoczen pushed back on that idea but I cannot find it. Perhaps I am misremembering?

At any rate, I have no objection to moving ACL config out of config.py and into storage.

3. A web admin interface

If we take the plunge of moving config into storage then a web control panel could be very handy. Wouldn't exactly be a requirement, Will could trivially reconfigure himself in response to chat commands, but definitely handy.

Unless anyone feels strongly that these ideas need to be pursued in a single effort I suggest we tackle 1 to fix the Slack ACL bug and open issues to further discuss 2 and 3.

[pastorhudson]

@rsalmond I think this is a sane approach.

[rsalmond]

^^ So this doesn't address my initial hesitation about user ID's being a crummy config.py experience but I've tried to mitigate that with some extra help in the config comments and updating the example.