From 7340d4f7b85f979b4582192e599467112d96158c Mon Sep 17 00:00:00 2001 From: Axel Suarez Date: Fri, 7 Jun 2019 16:54:43 -0700 Subject: [PATCH 1/3] Added middleware to address transitional location of tenant_id field for teams in BotFrameworkAdater. More testing on adapter pending. --- .../botbuilder/core/bot_framework_adapter.py | 35 +++- .../tests/test_bot_framework_adapter.py | 166 ++++++++++++++++++ .../botbuilder/schema/_models.py | 8 + .../botbuilder/schema/_models_py3.py | 12 +- 4 files changed, 215 insertions(+), 6 deletions(-) create mode 100644 libraries/botbuilder-core/tests/test_bot_framework_adapter.py diff --git a/libraries/botbuilder-core/botbuilder/core/bot_framework_adapter.py b/libraries/botbuilder-core/botbuilder/core/bot_framework_adapter.py index bbebb3e27..13c13e947 100644 --- a/libraries/botbuilder-core/botbuilder/core/bot_framework_adapter.py +++ b/libraries/botbuilder-core/botbuilder/core/bot_framework_adapter.py @@ -7,17 +7,34 @@ ConversationAccount, ConversationParameters, ConversationReference, ConversationsResult, ConversationResourceResponse) -from botframework.connector import ConnectorClient +from botframework.connector import ConnectorClient, Channels from botframework.connector.auth import (MicrosoftAppCredentials, JwtTokenValidation, SimpleCredentialProvider) from . import __version__ from .bot_adapter import BotAdapter from .turn_context import TurnContext +from .middleware_set import Middleware USER_AGENT = f"Microsoft-BotFramework/3.1 (BotBuilder Python/{__version__})" +class TenantIdWorkaroundForTeamsMiddleware(Middleware): + """ + Middleware to assign tenant_id from channelData to Conversation.tenant_id. + MS Teams currently sends the tenant ID in channelData and the correct behavior is to expose this value in Activity.Conversation.tenant_id. + This code copies the tenant ID from channelData to Activity.Conversation.tenant_id. + Once MS Teams sends the tenant_id in the Conversation property, this middleware can be removed. + """ + async def on_process_request(self, context: TurnContext, next: Callable[[TurnContext], Awaitable]): + if Channels.ms_teams == context.activity.channel_id and context.activity.conversation is not None and not context.activity.conversation.tenant_id: + teams_channel_data = context.activity.channel_data + if teams_channel_data.get("tenant", {}).get("id", None): + context.activity.conversation.tenant_id = str(teams_channel_data["tenant"]["id"]) + + await next() + + class BotFrameworkAdapterSettings(object): def __init__(self, app_id: str, app_password: str, channel_auth_tenant: str= None): self.app_id = app_id @@ -34,6 +51,8 @@ def __init__(self, settings: BotFrameworkAdapterSettings): self.settings.channel_auth_tenant) self._credential_provider = SimpleCredentialProvider(self.settings.app_id, self.settings.app_password) + self.use(TenantIdWorkaroundForTeamsMiddleware()) + async def continue_conversation(self, reference: ConversationReference, logic): """ Continues a conversation with a user. This is often referred to as the bots "Proactive Messaging" @@ -64,6 +83,14 @@ async def create_conversation(self, reference: ConversationReference, logic: Cal parameters = ConversationParameters(bot=reference.bot) client = self.create_connector_client(reference.service_url) + # Mix in the tenant ID if specified. This is required for MS Teams. + if reference.conversation is not None and reference.conversation.tenant_id: + # Putting tenant_id in channel_data is a temporary while we wait for the Teams API to be updated + parameters.channel_data = {'tenant': {'id': reference.conversation.tenant_id}} + + # Permanent solution is to put tenant_id in parameters.tenant_id + parameters.tenant_id = reference.conversation.tenant_id + resource_response = await client.conversations.create_conversation(parameters) request = TurnContext.apply_conversation_reference(Activity(), reference, is_incoming=True) request.conversation = ConversationAccount(id=resource_response.id) @@ -123,12 +150,12 @@ async def validate_activity(activity: Activity): if not isinstance(activity.type, str): raise TypeError('BotFrameworkAdapter.parse_request(): invalid or missing activity type.') return True - if not isinstance(req, Activity): # If the req is a raw HTTP Request, try to deserialize it into an Activity and return the Activity. - if hasattr(req, 'body'): + if getattr(req, 'body_exists', False): try: - activity = Activity().deserialize(req.body) + body = await req.json() + activity = Activity().deserialize(body) is_valid_activity = await validate_activity(activity) if is_valid_activity: return activity diff --git a/libraries/botbuilder-core/tests/test_bot_framework_adapter.py b/libraries/botbuilder-core/tests/test_bot_framework_adapter.py new file mode 100644 index 000000000..a6aedee55 --- /dev/null +++ b/libraries/botbuilder-core/tests/test_bot_framework_adapter.py @@ -0,0 +1,166 @@ +# Copyright (c) Microsoft Corporation. All rights reserved. +# Licensed under the MIT License. + +import aiounittest +import unittest +from copy import copy +from unittest.mock import Mock + +from botbuilder.core import BotFrameworkAdapter, BotFrameworkAdapterSettings, TurnContext +from botbuilder.schema import Activity, ActivityTypes, ConversationAccount, ConversationReference, ChannelAccount +from botframework.connector import ConnectorClient +from botframework.connector.auth import ClaimsIdentity + +reference = ConversationReference( + activity_id='1234', + channel_id='test', + service_url='https://example.org/channel', + user=ChannelAccount( + id='user', + name='User Name' + ), + bot=ChannelAccount( + id='bot', + name='Bot Name' + ), + conversation=ConversationAccount( + id='convo1' + ) +) + +test_activity = Activity(text='test', type=ActivityTypes.message) + +incoming_message = TurnContext.apply_conversation_reference(copy(test_activity), reference, True) +outgoing_message = TurnContext.apply_conversation_reference(copy(test_activity), reference) +incoming_invoke = TurnContext.apply_conversation_reference(Activity(type=ActivityTypes.invoke), reference, True) + + +class AdapterUnderTest(BotFrameworkAdapter): + + def __init__(self, settings=None): + super().__init__(settings) + self.tester = aiounittest.AsyncTestCase() + self.fail_auth = False + self.fail_operation = False + self.expect_auth_header = '' + self.new_service_url = None + + def aux_test_authenticate_request(self, request: Activity, auth_header: str): + return super().authenticate_request(request, auth_header) + + def aux_test_create_connector_client(self, service_url: str): + return super().create_connector_client(service_url) + + async def authenticate_request(self, request: Activity, auth_header: str): + self.tester.assertIsNotNone(request, 'authenticate_request() not passed request.') + self.tester.assertEqual(auth_header, self.expect_auth_header, 'authenticateRequest() not passed expected authHeader.') + return not self.fail_auth + + def create_connector_client(self, service_url: str) -> ConnectorClient: + self.tester.assertIsNotNone(service_url, 'create_connector_client() not passed service_url.') + connector_client_mock = Mock() + + def mock_reply_to_activity(conversation_id, activity_id, activity): + nonlocal self + self.tester.assertIsNotNone(conversation_id, 'reply_to_activity not passed conversation_id') + self.tester.assertIsNotNone(activity_id, 'reply_to_activity not passed activity_id') + self.tester.assertIsNotNone(activity, 'reply_to_activity not passed activity') + return not self.fail_auth + + def mock_send_to_conversation(conversation_id, activity): + nonlocal self + self.tester.assertIsNotNone(conversation_id, 'send_to_conversation not passed conversation_id') + self.tester.assertIsNotNone(activity, 'send_to_conversation not passed activity') + return not self.fail_auth + + def mock_update_activity(conversation_id, activity_id, activity): + nonlocal self + self.tester.assertIsNotNone(conversation_id, 'update_activity not passed conversation_id') + self.tester.assertIsNotNone(activity_id, 'update_activity not passed activity_id') + self.tester.assertIsNotNone(activity, 'update_activity not passed activity') + return not self.fail_auth + + def mock_delete_activity(conversation_id, activity_id): + nonlocal self + self.tester.assertIsNotNone(conversation_id, 'delete_activity not passed conversation_id') + self.tester.assertIsNotNone(activity_id, 'delete_activity not passed activity_id') + return not self.fail_auth + + def mock_create_conversation(parameters): + nonlocal self + self.tester.assertIsNotNone(parameters, 'create_conversation not passed parameters') + return not self.fail_auth + + connector_client_mock.conversations.reply_to_activity.side_effect = mock_reply_to_activity + connector_client_mock.conversations.send_to_conversation.side_effect = mock_send_to_conversation + connector_client_mock.conversations.update_activity.side_effect = mock_update_activity + connector_client_mock.conversations.delete_activity.side_effect = mock_delete_activity + connector_client_mock.conversations.create_conversation.side_effect = mock_create_conversation + + return connector_client_mock + + +async def process_activity(channel_id: str, channel_data_tenant_id: str, conversation_tenant_id: str): + activity = None + mock_claims = unittest.mock.create_autospec(ClaimsIdentity) + mock_credential_provider = unittest.mock.create_autospec(BotFrameworkAdapterSettings) + + sut = BotFrameworkAdapter(mock_credential_provider) + + async def aux_func(context): + nonlocal activity + activity = context.Activity + + await sut.process_activity( + Activity( + channel_id=channel_id, + service_url="https://smba.trafficmanager.net/amer/", + channel_data={"tenant": {"id": channel_data_tenant_id}}, + conversation=ConversationAccount( + tenant_id=conversation_tenant_id + ), + ), + mock_claims, + aux_func) + return activity + + +class TestBotFrameworkAdapter(aiounittest.AsyncTestCase): + + def test_should_create_connector_client(self): + adapter = AdapterUnderTest() + client = adapter.aux_test_create_connector_client(reference.service_url) + self.assertIsNotNone(client, 'client not returned.') + self.assertIsNotNone(client.conversations, 'invalid client returned.') + + async def test_should_process_activity(self): + called = False + adapter = AdapterUnderTest() + + async def aux_func_assert_context(context): + self.assertIsNotNone(context, 'context not passed.') + nonlocal called + called = True + + await adapter.process_activity(incoming_message, '', aux_func_assert_context) + self.assertTrue(called, 'bot logic not called.') + + async def test_should_migrate_tenant_id_for_msteams(self): + incoming = TurnContext.apply_conversation_reference( + activity=Activity( + type=ActivityTypes.message, + text='foo', + channel_data={'tenant': {'id': '1234'}} + ), + reference=reference, + is_incoming=True + ) + + incoming.channel_id = 'msteams' + adapter = AdapterUnderTest() + + async def aux_func_assert_tenant_id_copied(context): + self.assertEquals(context.activity.conversation.tenant_id, '1234', 'should have copied tenant id from ' + 'channel_data to conversation address') + + await adapter.process_activity(incoming, '', aux_func_assert_tenant_id_copied) diff --git a/libraries/botbuilder-schema/botbuilder/schema/_models.py b/libraries/botbuilder-schema/botbuilder/schema/_models.py index d1fd5966d..00673fd4b 100644 --- a/libraries/botbuilder-schema/botbuilder/schema/_models.py +++ b/libraries/botbuilder-schema/botbuilder/schema/_models.py @@ -628,6 +628,8 @@ class ConversationAccount(Model): :param role: Role of the entity behind the account (Example: User, Bot, etc.). Possible values include: 'user', 'bot' :type role: str or ~botframework.connector.models.RoleTypes + :param tenant_id: This conversation's tenant ID + :type tenant_id: str """ _attribute_map = { @@ -637,6 +639,7 @@ class ConversationAccount(Model): 'name': {'key': 'name', 'type': 'str'}, 'aad_object_id': {'key': 'aadObjectId', 'type': 'str'}, 'role': {'key': 'role', 'type': 'str'}, + 'tenant_id': {'key': 'tenantID', 'type': 'str'}, } def __init__(self, **kwargs): @@ -647,6 +650,7 @@ def __init__(self, **kwargs): self.name = kwargs.get('name', None) self.aad_object_id = kwargs.get('aad_object_id', None) self.role = kwargs.get('role', None) + self.tenant_id = kwargs.get('tenant_id', None) class ConversationMembers(Model): @@ -687,6 +691,8 @@ class ConversationParameters(Model): :param channel_data: Channel specific payload for creating the conversation :type channel_data: object + :param tenant_id: (Optional) The tenant ID in which the conversation should be created + :type tenant_id: str """ _attribute_map = { @@ -696,6 +702,7 @@ class ConversationParameters(Model): 'topic_name': {'key': 'topicName', 'type': 'str'}, 'activity': {'key': 'activity', 'type': 'Activity'}, 'channel_data': {'key': 'channelData', 'type': 'object'}, + 'tenant_id': {'key': 'tenantID', 'type': 'str'}, } def __init__(self, **kwargs): @@ -706,6 +713,7 @@ def __init__(self, **kwargs): self.topic_name = kwargs.get('topic_name', None) self.activity = kwargs.get('activity', None) self.channel_data = kwargs.get('channel_data', None) + self.tenant_id = kwargs.get('tenant_id', None) class ConversationReference(Model): diff --git a/libraries/botbuilder-schema/botbuilder/schema/_models_py3.py b/libraries/botbuilder-schema/botbuilder/schema/_models_py3.py index e6c0f9acc..dfe407819 100644 --- a/libraries/botbuilder-schema/botbuilder/schema/_models_py3.py +++ b/libraries/botbuilder-schema/botbuilder/schema/_models_py3.py @@ -628,6 +628,8 @@ class ConversationAccount(Model): :param role: Role of the entity behind the account (Example: User, Bot, etc.). Possible values include: 'user', 'bot' :type role: str or ~botframework.connector.models.RoleTypes + :param tenant_id: This conversation's tenant ID + :type tenant_id: str """ _attribute_map = { @@ -637,9 +639,10 @@ class ConversationAccount(Model): 'name': {'key': 'name', 'type': 'str'}, 'aad_object_id': {'key': 'aadObjectId', 'type': 'str'}, 'role': {'key': 'role', 'type': 'str'}, + 'tenant_id': {'key': 'tenantID', 'type': 'str'}, } - def __init__(self, *, is_group: bool=None, conversation_type: str=None, id: str=None, name: str=None, aad_object_id: str=None, role=None, **kwargs) -> None: + def __init__(self, *, is_group: bool=None, conversation_type: str=None, id: str=None, name: str=None, aad_object_id: str=None, role=None, tenant_id=None, **kwargs) -> None: super(ConversationAccount, self).__init__(**kwargs) self.is_group = is_group self.conversation_type = conversation_type @@ -647,6 +650,7 @@ def __init__(self, *, is_group: bool=None, conversation_type: str=None, id: str= self.name = name self.aad_object_id = aad_object_id self.role = role + self.tenant_id = tenant_id class ConversationMembers(Model): @@ -687,6 +691,8 @@ class ConversationParameters(Model): :param channel_data: Channel specific payload for creating the conversation :type channel_data: object + :param tenant_id: (Optional) The tenant ID in which the conversation should be created + :type tenant_id: str """ _attribute_map = { @@ -696,9 +702,10 @@ class ConversationParameters(Model): 'topic_name': {'key': 'topicName', 'type': 'str'}, 'activity': {'key': 'activity', 'type': 'Activity'}, 'channel_data': {'key': 'channelData', 'type': 'object'}, + 'tenant_id': {'key': 'tenantID', 'type': 'str'}, } - def __init__(self, *, is_group: bool=None, bot=None, members=None, topic_name: str=None, activity=None, channel_data=None, **kwargs) -> None: + def __init__(self, *, is_group: bool=None, bot=None, members=None, topic_name: str=None, activity=None, channel_data=None, tenant_id=None, **kwargs) -> None: super(ConversationParameters, self).__init__(**kwargs) self.is_group = is_group self.bot = bot @@ -706,6 +713,7 @@ def __init__(self, *, is_group: bool=None, bot=None, members=None, topic_name: s self.topic_name = topic_name self.activity = activity self.channel_data = channel_data + self.tenant_id = tenant_id class ConversationReference(Model): From 6b2f4a33c2bee8f7d585d794d880d3e316781b38 Mon Sep 17 00:00:00 2001 From: Axel Suarez Date: Tue, 11 Jun 2019 15:29:05 -0700 Subject: [PATCH 2/3] moved teams fix from middleware in BotFrameworkAdapter to the run_pipeline in BotAdapter --- .../botbuilder/core/bot_adapter.py | 14 ++++++++++++++ .../botbuilder/core/bot_framework_adapter.py | 18 ------------------ 2 files changed, 14 insertions(+), 18 deletions(-) diff --git a/libraries/botbuilder-core/botbuilder/core/bot_adapter.py b/libraries/botbuilder-core/botbuilder/core/bot_adapter.py index aef52f17b..223a9b3d3 100644 --- a/libraries/botbuilder-core/botbuilder/core/bot_adapter.py +++ b/libraries/botbuilder-core/botbuilder/core/bot_adapter.py @@ -4,6 +4,7 @@ from abc import ABC, abstractmethod from typing import List, Callable, Awaitable from botbuilder.schema import Activity, ConversationReference +from botframework.connector import Channels from . import conversation_reference_extension from .bot_assert import BotAssert @@ -77,6 +78,19 @@ async def run_pipeline(self, context: TurnContext, callback: Callable[[TurnConte BotAssert.context_not_none(context) if context.activity is not None: + + # Middleware to assign tenant_id from channelData to Conversation.tenant_id. + # MS Teams currently sends the tenant ID in channelData and the correct behavior is to expose + # this value in Activity.Conversation.tenant_id. + # This code copies the tenant ID from channelData to Activity.Conversation.tenant_id. + # Once MS Teams sends the tenant_id in the Conversation property, this middleware can be removed. + if (Channels.ms_teams == context.activity.channel_id and context.activity.conversation is not None + and not context.activity.conversation.tenant_id): + teams_channel_data = context.activity.channel_data + if teams_channel_data.get("tenant", {}).get("id", None): + context.activity.conversation.tenant_id = str(teams_channel_data["tenant"]["id"]) + + try: return await self._middleware.receive_activity_with_status(context, callback) except Exception as error: diff --git a/libraries/botbuilder-core/botbuilder/core/bot_framework_adapter.py b/libraries/botbuilder-core/botbuilder/core/bot_framework_adapter.py index 13c13e947..887607083 100644 --- a/libraries/botbuilder-core/botbuilder/core/bot_framework_adapter.py +++ b/libraries/botbuilder-core/botbuilder/core/bot_framework_adapter.py @@ -19,22 +19,6 @@ USER_AGENT = f"Microsoft-BotFramework/3.1 (BotBuilder Python/{__version__})" -class TenantIdWorkaroundForTeamsMiddleware(Middleware): - """ - Middleware to assign tenant_id from channelData to Conversation.tenant_id. - MS Teams currently sends the tenant ID in channelData and the correct behavior is to expose this value in Activity.Conversation.tenant_id. - This code copies the tenant ID from channelData to Activity.Conversation.tenant_id. - Once MS Teams sends the tenant_id in the Conversation property, this middleware can be removed. - """ - async def on_process_request(self, context: TurnContext, next: Callable[[TurnContext], Awaitable]): - if Channels.ms_teams == context.activity.channel_id and context.activity.conversation is not None and not context.activity.conversation.tenant_id: - teams_channel_data = context.activity.channel_data - if teams_channel_data.get("tenant", {}).get("id", None): - context.activity.conversation.tenant_id = str(teams_channel_data["tenant"]["id"]) - - await next() - - class BotFrameworkAdapterSettings(object): def __init__(self, app_id: str, app_password: str, channel_auth_tenant: str= None): self.app_id = app_id @@ -51,8 +35,6 @@ def __init__(self, settings: BotFrameworkAdapterSettings): self.settings.channel_auth_tenant) self._credential_provider = SimpleCredentialProvider(self.settings.app_id, self.settings.app_password) - self.use(TenantIdWorkaroundForTeamsMiddleware()) - async def continue_conversation(self, reference: ConversationReference, logic): """ Continues a conversation with a user. This is often referred to as the bots "Proactive Messaging" From fa36deb4ab1e9bb3b2cffafc774562f9198096a5 Mon Sep 17 00:00:00 2001 From: Axel Suarez Date: Tue, 11 Jun 2019 17:04:13 -0700 Subject: [PATCH 3/3] tenant_id teams fix moved to BotFrameworkAdapter --- .../botbuilder-core/botbuilder/core/bot_adapter.py | 14 -------------- .../botbuilder/core/bot_framework_adapter.py | 14 +++++++++++++- 2 files changed, 13 insertions(+), 15 deletions(-) diff --git a/libraries/botbuilder-core/botbuilder/core/bot_adapter.py b/libraries/botbuilder-core/botbuilder/core/bot_adapter.py index 223a9b3d3..aef52f17b 100644 --- a/libraries/botbuilder-core/botbuilder/core/bot_adapter.py +++ b/libraries/botbuilder-core/botbuilder/core/bot_adapter.py @@ -4,7 +4,6 @@ from abc import ABC, abstractmethod from typing import List, Callable, Awaitable from botbuilder.schema import Activity, ConversationReference -from botframework.connector import Channels from . import conversation_reference_extension from .bot_assert import BotAssert @@ -78,19 +77,6 @@ async def run_pipeline(self, context: TurnContext, callback: Callable[[TurnConte BotAssert.context_not_none(context) if context.activity is not None: - - # Middleware to assign tenant_id from channelData to Conversation.tenant_id. - # MS Teams currently sends the tenant ID in channelData and the correct behavior is to expose - # this value in Activity.Conversation.tenant_id. - # This code copies the tenant ID from channelData to Activity.Conversation.tenant_id. - # Once MS Teams sends the tenant_id in the Conversation property, this middleware can be removed. - if (Channels.ms_teams == context.activity.channel_id and context.activity.conversation is not None - and not context.activity.conversation.tenant_id): - teams_channel_data = context.activity.channel_data - if teams_channel_data.get("tenant", {}).get("id", None): - context.activity.conversation.tenant_id = str(teams_channel_data["tenant"]["id"]) - - try: return await self._middleware.receive_activity_with_status(context, callback) except Exception as error: diff --git a/libraries/botbuilder-core/botbuilder/core/bot_framework_adapter.py b/libraries/botbuilder-core/botbuilder/core/bot_framework_adapter.py index 887607083..24e18b74c 100644 --- a/libraries/botbuilder-core/botbuilder/core/bot_framework_adapter.py +++ b/libraries/botbuilder-core/botbuilder/core/bot_framework_adapter.py @@ -94,13 +94,25 @@ async def process_activity(self, req, auth_header: str, logic: Callable): :param auth_header: :param logic: :return: - """ + """ activity = await self.parse_request(req) auth_header = auth_header or '' await self.authenticate_request(activity, auth_header) context = self.create_context(activity) + # Fix to assign tenant_id from channelData to Conversation.tenant_id. + # MS Teams currently sends the tenant ID in channelData and the correct behavior is to expose + # this value in Activity.Conversation.tenant_id. + # This code copies the tenant ID from channelData to Activity.Conversation.tenant_id. + # Once MS Teams sends the tenant_id in the Conversation property, this code can be removed. + if (Channels.ms_teams == context.activity.channel_id and context.activity.conversation is not None + and not context.activity.conversation.tenant_id): + teams_channel_data = context.activity.channel_data + if teams_channel_data.get("tenant", {}).get("id", None): + context.activity.conversation.tenant_id = str(teams_channel_data["tenant"]["id"]) + + return await self.run_pipeline(context, logic) async def authenticate_request(self, request: Activity, auth_header: str):