Skip to content
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

Add Teams connector #1679

Merged
merged 32 commits into from
Jul 30, 2021
Merged

Conversation

jacobtomlinson
Copy link
Member

@jacobtomlinson jacobtomlinson commented Nov 25, 2020

@FabioRosado and I started working on a Teams connector today.

Things are functional, the bot can receive messages and respond.

Teams is a little weird because the bot has to be @ mentioned in every message in order for it to process it. And to send notification messages to a room the user has to have spoken to the bot in that room at least once. Other than that things are pretty standard.

Imgur

  • Tests
  • Docs

@codecov
Copy link

codecov bot commented Nov 25, 2020

Codecov Report

Merging #1679 (32bc6e9) into master (cc45e5a) will increase coverage by 0.00%.
The diff coverage is 100.00%.

Impacted file tree graph

@@           Coverage Diff           @@
##           master    #1679   +/-   ##
=======================================
  Coverage   99.28%   99.29%           
=======================================
  Files          78       80    +2     
  Lines        4633     4695   +62     
=======================================
+ Hits         4600     4662   +62     
  Misses         33       33           
Impacted Files Coverage Δ
opsdroid/connector/teams/__init__.py 100.00% <100.00%> (ø)
opsdroid/connector/teams/teams.py 100.00% <100.00%> (ø)
opsdroid/testing/utils.py 100.00% <100.00%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update cc45e5a...32bc6e9. Read the comment docs.

@stale
Copy link

stale bot commented Dec 19, 2020

This pull request has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

@stale stale bot added the stale label Dec 19, 2020
@cognifloyd
Copy link
Member

Is the @ only restriction due to using BotFramework?

@FabioRosado
Copy link
Member

Is the @ only restriction due to using BotFramework?

When I tried to implement this connector I've used the webhooks approach and that was still the only way to trigger the webhook.

Even the docs say that for you to interact with a bot you need to @ it so it must be a design choice

Copy link
Member

@cognifloyd cognifloyd left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Very cool. I just went surfing through the docs to see if I could find anything helpful. Here is what I found.

opsdroid/connector/teams/teams.py Outdated Show resolved Hide resolved
teams_channel_id = self.parse_channel_id(message.target)
try:
connector_client = await self.adapter.create_connector_client(
self.service_endpoints[teams_channel_id]
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm. I wonder if there's another API we could use to get the list of users (and therefore endpoints) in a channel...

botbuilder.core.teams.teams_get_team_info looks promising. It returns a TeamInfo object with methods like get_team_details, get_team_channels, and get_team_members. I wonder if that could be used to generate a list of service_endpoints.
https://docs.microsoft.com/en-us/python/api/botbuilder-core/botbuilder.core.teams.teamsinfo?view=botbuilder-py-latest

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Another possibility to allow sending messages to channels is to use "incoming webhooks": https://docs.microsoft.com/en-us/microsoftteams/platform/webhooks-and-connectors/how-to/add-incoming-webhook
Sending cards requires using "the actionable message card format".

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe Proactive messaging could also be used to allow sending messages.

Here's an interesting quote:

you can use the Microsoft Graph API to proactively install your bot for your users

And a key piece:

When your app is installed for the user, the bot will receive a conversationUpdate event notification that will contain the necessary information to send the proactive message.

Which would provide all those service endpoints.

Reference docs for how to do this in an app:
https://docs.microsoft.com/en-us/microsoftteams/platform/bots/how-to/conversations/send-proactive-messages?tabs=python

from botbuilder.core import (
BotFrameworkAdapterSettings,
BotFrameworkAdapter,
MessageFactory,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

core.CardFactory might be helpful in addition to MessageFactory

@cognifloyd
Copy link
Member

Here's the relevant bit about @ mentioning bots:

Bots in channel and group chat conversations require the user to @ mention the bot to invoke it in a channel.
Bots in a one-to-one conversation do not require an @ mention. All messages sent by the user will be routed to your bot.

https://docs.microsoft.com/en-us/microsoftteams/platform/bots/how-to/conversations/conversation-basics

@cognifloyd
Copy link
Member

cognifloyd commented Jan 4, 2021

OOH! look what I found:
https://docs.microsoft.com/en-us/graph/api/subscription-post-subscriptions?view=graph-rest-1.0&tabs=http

So, if we step outside of the BotFramework supported APIs and use the graph API, it looks like the bot COULD subscribe to all messages in a channel by subscribing to /teams/{id}/channels/{id}/messages

To use this, there are additional "protected API" restrictions: https://docs.microsoft.com/en-us/graph/teams-protected-apis which means everyone (ie every teams admin adding OpsDroid to their teams tenant(s)) would have to apply for access to use the API for their OpsDroid teams-app in their tenant with https://aka.ms/teamsgraph/requestaccess.

@jacobtomlinson
Copy link
Member Author

Interesting. It seems strange that you are required to request additional permissions via some web form that is reviewed by a human. Sounds like it could be inconsistent. I wonder what the criteria are for approving these?

I'm not saying we shouldn't do it. But it should probably be in addition to the regular bot APIs.

@cognifloyd
Copy link
Member

cognifloyd commented Jan 13, 2021

Found out why the tests are freezing. Pip is endlessly backtracking.

This is the culprit:

https://github.com/microsoft/botbuilder-python/blob/4.11.0/libraries/botframework-connector/setup.py#L12

botframework-connector specifies PyJWT==1.5.3 which conflicts with ibm-watson's (well ibm-cloud-sdk-core's) requirement: PyJWT>=1.7.1

I didn't notice this locally because I have an old version of pip installed so I wasn't using the new pip resolver.

It is stripping "/" from the end of URLs now probably due to:
matrix-nio/matrix-nio@574a2ee

But, it looks like that was their intention all along, so this is a bugfix.

This adjusts our tests to not expect the final "/" anymore.
@cognifloyd
Copy link
Member

stale bot: You're wrong.

@stale stale bot removed the stale label Jun 9, 2021
@stale
Copy link

stale bot commented Jun 26, 2021

This pull request has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

@stale stale bot added the stale label Jun 26, 2021
@pintman
Copy link

pintman commented Jul 7, 2021

I hope the feature will be integrated.

@stale stale bot removed the stale label Jul 7, 2021
@cognifloyd
Copy link
Member

I wonder if there are any hooks we can use to control uuid generation in the teams lib. Maybe a fixture that makes uuid.uuid() or similar (whatever it is using to generate the uuid) return something consistent?

@jacobtomlinson
Copy link
Member Author

I wonder if there are any hooks we can use to control uuid generation in the teams lib.

Possibly! I'd rather avoid digging too deep into the teams SDK. I was thinking about trying to write a custom VCR handler which stripped the uuid. But patching uuid.uuid() might be a better call.

Copy link
Member

@cognifloyd cognifloyd left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Wow. 😥
Looking at the coverage, I think adding a bit to the test will test the rest of the significant code.

Also, I'm not sure what the docker/pip test failure is about.

@jacobtomlinson
Copy link
Member Author

@cognifloyd you've been great at reviewing here. Do you want to give this a last pass and hit the big green button?

@cognifloyd cognifloyd merged commit a45490d into opsdroid:master Jul 30, 2021
@jacobtomlinson jacobtomlinson deleted the teams-connector branch July 30, 2021 14:00
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants