-
Notifications
You must be signed in to change notification settings - Fork 66
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Security #16
Comments
The DefaultBotHandler has no authorisation built in, you would have to add your own based on what security rules you needed. eg. Adding in AAD auth using https://github.com/MicrosoftDX/AuthBot and then AD group lookup. Or you may have other rules on who can be an agent based on channel, usernames etc.. |
Thanks |
@GioviQ @daltskin I have added the ability to specify permitted agent channels, so you can prevent commands from being used on non-permitted channels. You can specify a list of channel IDs in the web.config in the PermittedAgentChannels app setting. e.g. you might permit agent commands on Slack only. If the app setting value is empty or doesn't exist then all channels will accept commands This is currently on the development branch at the moment. |
@garypretty that'll help for simple understanding of how it can be locked down. Would be worth adding in userid's for the given channel as well as all users & agents likely to be on same channel for many scenarios. |
Agreed. I'll add that later today if I get time. Sent from my Google Nexus 5X using FastHub |
Maybe a password request would be a better solution, when you send "command add aggregation" (mentions do not work on Telegram and Messenger). |
Just to clarify: When I originally created the commands it was just so that it would be easy to take the bot "out for a spin". The means for managing the bot in production was left for the developer - including the security. That said, I don't mind, at all, if we build a more secure way here as long as it doesn't make the sample harder to understand. |
@tompaana I think as long as we document any changes it will be fine. I can easily see people taking the sample and using it as is to integrate it into their bots to handle handoff, so I think it is important to be adding things like this. @daltskin thoughts? It might be at some point that we spin out an advanced sample if it gets too complex. Then you have a simple and advanced option. That's a fairly common approach, but I think we are fine at the moment. |
I think we need to at least include a basic security option, as it is an obvious question to raise. Which is what @garypretty has worked on. But I'd like to see a sign-in card ideally. Also, need to handle one agent hijacking/taking over from another - this may be desirable in some situations on channel <-> scenario. |
^ really need some unit tests for this :) |
@garypretty @daltskin You do make an excellent point/points. |
@garypretty I don't have a personal preference and, to be honest, I've never written any tests for bots. I think this might be a good starting point: https://stackoverflow.com/a/41349523/3323695 |
Some testing resources here: https://blogs.msdn.microsoft.com/jamiedalton/2017/08/07/devops-for-bots-sprinkling-some-devops-on-your-bot/ |
@tompaana me neither, but there is a devops for bots series on channel 9 which discuss this as well |
@daltskin good timing! |
Anyone you send "@BOT_NAME add aggregation" can become an agent?
The text was updated successfully, but these errors were encountered: