diff --git a/app/connectors_service/connectors/sources/outlook/client.py b/app/connectors_service/connectors/sources/outlook/client.py index 505b18522..b522d72a2 100644 --- a/app/connectors_service/connectors/sources/outlook/client.py +++ b/app/connectors_service/connectors/sources/outlook/client.py @@ -28,7 +28,7 @@ from connectors.sources.outlook.constants import ( API_SCOPE, CALENDAR_FIELDS, - CONTACT_FIELDS, + CONTACT_FOLDER_FIELDS, EWS_ENDPOINT, MAIL_FIELDS, MAIL_TYPES, @@ -226,7 +226,7 @@ async def get_user_accounts(self): ) async for user in self.get_users(): - if "searchResRef" in user["type"]: + if "searchResRef" in user.get("type", ""): continue mail = _extract_ldap_mail(user.get("attributes", {})) @@ -406,12 +406,17 @@ async def get_mails(self, account): f"Fetching {mail_type['folder']} mails for {account.primary_smtp_address}" ) try: + # Resolve folders off the event loop (blocking exchangelib call). if mail_type["folder"] == "archive": # msg_folder_root is locale-agnostic; the "Archive" leaf has no # distinguished ID, so resolve it by name and skip if absent. - folder_object = account.msg_folder_root / "Archive" + folder_object = await asyncio.to_thread( + lambda: account.msg_folder_root / "Archive" + ) else: - folder_object = getattr(account, mail_type["folder"]) + folder_object = await asyncio.to_thread( + getattr, account, mail_type["folder"] + ) except ErrorFolderNotFound: self._logger.warning( f"Could not resolve {mail_type['folder']} folder for " @@ -423,31 +428,58 @@ async def get_mails(self, account): yield mail, mail_type async def get_calendars(self, account): - for calendar in await asyncio.to_thread( - account.calendar.all().only, *CALENDAR_FIELDS - ): + # Resolve the folder off the event loop (blocking call); skip if absent. + try: + folder = await asyncio.to_thread(getattr, account, "calendar") + except ErrorFolderNotFound: + self._logger.warning( + f"Could not resolve Calendar folder for {account.primary_smtp_address}, skipping." + ) + return + for calendar in await asyncio.to_thread(folder.all().only, *CALENDAR_FIELDS): yield calendar async def get_child_calendars(self, account): - for child_calendar in account.calendar.children: + # Resolve folder and children off the event loop; skip if absent. + try: + child_calendars = await asyncio.to_thread( + lambda: list(account.calendar.children) + ) + except ErrorFolderNotFound: + self._logger.warning( + f"Could not resolve Calendar folder for {account.primary_smtp_address}, " + "skipping child calendars." + ) + return + for child_calendar in child_calendars: for calendar in await asyncio.to_thread( child_calendar.all().only, *CALENDAR_FIELDS ): yield calendar, child_calendar async def get_tasks(self, account): - for task in await asyncio.to_thread(account.tasks.all().only, *TASK_FIELDS): + # Resolve the folder off the event loop (blocking call); skip if absent. + try: + folder = await asyncio.to_thread(getattr, account, "tasks") + except ErrorFolderNotFound: + self._logger.warning( + f"Could not resolve Tasks folder for {account.primary_smtp_address}, skipping." + ) + return + for task in await asyncio.to_thread(folder.all().only, *TASK_FIELDS): yield task async def get_contacts(self, account): - # account.contacts uses a distinguished folder ID, which is locale-agnostic - # unlike name-based paths that break on non-English Exchange servers. + # account.contacts uses a locale-agnostic distinguished folder ID; resolve + # it off the event loop (blocking call); skip if absent. try: - folder = account.contacts + folder = await asyncio.to_thread(getattr, account, "contacts") except ErrorFolderNotFound: self._logger.warning( f"Could not resolve Contacts folder for {account.primary_smtp_address}, skipping." ) return - for contact in await asyncio.to_thread(folder.all().only, *CONTACT_FIELDS): + for contact in await asyncio.to_thread( + folder.all().only, *CONTACT_FOLDER_FIELDS + ): yield contact diff --git a/app/connectors_service/connectors/sources/outlook/constants.py b/app/connectors_service/connectors/sources/outlook/constants.py index 1d7db5bd8..0333ac315 100644 --- a/app/connectors_service/connectors/sources/outlook/constants.py +++ b/app/connectors_service/connectors/sources/outlook/constants.py @@ -71,6 +71,13 @@ "company_name", "birthday", ] +DISTRIBUTION_LIST_FIELDS = [ + "last_modified_time", + "display_name", + "members", +] +# Contacts folder holds both item types, so query the union of their fields. +CONTACT_FOLDER_FIELDS = list(dict.fromkeys(CONTACT_FIELDS + DISTRIBUTION_LIST_FIELDS)) TASK_FIELDS = [ "last_modified_time", "due_date", diff --git a/app/connectors_service/connectors/sources/outlook/datasource.py b/app/connectors_service/connectors/sources/outlook/datasource.py index 77280007f..e3b5bd302 100644 --- a/app/connectors_service/connectors/sources/outlook/datasource.py +++ b/app/connectors_service/connectors/sources/outlook/datasource.py @@ -14,6 +14,7 @@ iso_utc, ) from exchangelib.errors import ErrorNonExistentMailbox +from exchangelib.items import Contact, DistributionList from connectors.access_control import ACCESS_CONTROL, es_access_control_query from connectors.sources.outlook.client import OutlookClient, _extract_ldap_mail @@ -90,11 +91,13 @@ def calendar_doc_formatter(self, calendar, child_calendar, timezone): } if child_calendar in ["Folder (Birthdays)", "Birthdays (Birthdays)"]: + # calendar.start may be missing; guard against splitting a None. + birthday = ews_format_to_datetime( + source_datetime=calendar.start, timezone=timezone + ) document.update( { - "date": ews_format_to_datetime( - source_datetime=calendar.start, timezone=timezone - ).split("T", 1)[0], + "date": birthday.split("T", 1)[0] if birthday else None, } ) else: @@ -168,6 +171,23 @@ def contact_doc_formatter(self, contact, timezone): ), } + def distribution_list_doc_formatter(self, distribution_list, timezone): + # A contact group has members, not per-contact fields, so it needs its own shape. + return { + "_id": distribution_list.id, + "type": "Distribution List", + "_timestamp": ews_format_to_datetime( + source_datetime=distribution_list.last_modified_time, + timezone=timezone, + ), + "name": distribution_list.display_name, + "email_addresses": [ + member.mailbox.email_address + for member in (distribution_list.members or []) + if member and member.mailbox and member.mailbox.email_address + ], + } + def attachment_doc_formatter(self, attachment, attachment_type, timezone): attachment_id = ( attachment.attachment_id.id if attachment.attachment_id else None @@ -537,10 +557,25 @@ async def _fetch_mails(self, account, timezone): async def _fetch_contacts(self, account, timezone): self._logger.debug(f"Fetching contacts for {account.primary_smtp_address}") async for contact in self.client.get_contacts(account=account): - document = self.doc_formatter.contact_doc_formatter( - contact=contact, - timezone=timezone, - ) + # Contacts folder returns only Contact or DistributionList; any other + # type breaks that assumption, so fail loudly instead of guessing. + if isinstance(contact, Contact): + document = self.doc_formatter.contact_doc_formatter( + contact=contact, + timezone=timezone, + ) + elif isinstance(contact, DistributionList): + document = self.doc_formatter.distribution_list_doc_formatter( + distribution_list=contact, + timezone=timezone, + ) + else: + msg = ( + f"Unexpected Contacts item type {type(contact).__name__} for " + f"{account.primary_smtp_address}; expected Contact or " + "DistributionList" + ) + raise TypeError(msg) yield ( self._decorate_with_access_control( document, [account.primary_smtp_address] diff --git a/app/connectors_service/tests/sources/test_outlook.py b/app/connectors_service/tests/sources/test_outlook.py index 8896d5a44..bc00eb4af 100644 --- a/app/connectors_service/tests/sources/test_outlook.py +++ b/app/connectors_service/tests/sources/test_outlook.py @@ -5,6 +5,7 @@ # """Tests the Outlook source class methods""" +import asyncio import ssl from contextlib import asynccontextmanager from unittest import mock @@ -19,6 +20,7 @@ ErrorNonExistentMailbox, TransportError, ) +from exchangelib.items import Contact, DistributionList from exchangelib.protocol import BaseProtocol, NoVerifyHTTPAdapter from connectors.sources.outlook import OutlookDataSource @@ -231,19 +233,38 @@ def __init__(self): self.attachments = [MOCK_ATTACHMENT] -class ContactDocument: +def ContactDocument(): + # A real Contact instance so _fetch_contacts' isinstance dispatch routes it + # to the contact formatter (spec mock keeps isinstance(..., Contact) True). + contact = MagicMock() + contact.email = "dummy.user@gmail.com" + contact.phone_number = 99887776655 + + document = MagicMock(spec=Contact) + document.id = "contact_1" + document.last_modified_time = "2023-12-12T01:01:01Z" + document.display_name = "Dummy User" + document.email_addresses = [contact] + document.phone_numbers = [contact] + document.company_name = "ABC" + document.birthday = "2023-12-12T01:01:01Z" + return document + + +class DistributionListDocument: + """Mimics a DistributionList (contact group) item that Exchange can return + from the Contacts folder. It carries the shared item fields plus its members, + not the per-contact fields (email_addresses, phone_numbers, company_name, + birthday).""" + def __init__(self): - contact = MagicMock() - contact.email = "dummy.user@gmail.com" - contact.phone_number = 99887776655 + member = MagicMock() + member.mailbox.email_address = "group.member@gmail.com" - self.id = "contact_1" + self.id = "distribution_list_1" self.last_modified_time = "2023-12-12T01:01:01Z" - self.display_name = "Dummy User" - self.email_addresses = [contact] - self.phone_numbers = [contact] - self.company_name = "ABC" - self.birthday = "2023-12-12T01:01:01Z" + self.display_name = "Dummy Group" + self.members = [member] class CalendarDocument: @@ -1162,6 +1183,190 @@ async def test_get_contacts_skips_when_folder_not_found(): assert contacts == [] +@pytest.mark.asyncio +async def test_get_calendars_skips_when_folder_not_found(): + async with create_outlook_source() as source: + account = MagicMock() + account.primary_smtp_address = "alex.wilber@gmail.com" + type(account).calendar = mock.PropertyMock( + side_effect=ErrorFolderNotFound("no") + ) + + calendars = [ + calendar async for calendar in source.client.get_calendars(account) + ] + assert calendars == [] + + +@pytest.mark.asyncio +async def test_get_child_calendars_skips_when_folder_not_found(): + async with create_outlook_source() as source: + account = MagicMock() + account.primary_smtp_address = "alex.wilber@gmail.com" + type(account).calendar = mock.PropertyMock( + side_effect=ErrorFolderNotFound("no") + ) + + calendars = [ + calendar async for calendar in source.client.get_child_calendars(account) + ] + assert calendars == [] + + +@pytest.mark.asyncio +async def test_get_tasks_skips_when_folder_not_found(): + async with create_outlook_source() as source: + account = MagicMock() + account.primary_smtp_address = "alex.wilber@gmail.com" + type(account).tasks = mock.PropertyMock(side_effect=ErrorFolderNotFound("no")) + + tasks = [task async for task in source.client.get_tasks(account)] + assert tasks == [] + + +@pytest.mark.asyncio +@pytest.mark.parametrize( + "method_name, folder_attr", + [ + ("get_calendars", "calendar"), + ("get_tasks", "tasks"), + ("get_contacts", "contacts"), + ], +) +async def test_get_methods_resolve_folder_off_event_loop(method_name, folder_attr): + # exchangelib is synchronous, so resolving the distinguished folder must be + # offloaded via asyncio.to_thread instead of blocking the event loop. + async with create_outlook_source() as source: + account = MockAccount() + method = getattr(source.client, method_name) + with patch( + "connectors.sources.outlook.client.asyncio.to_thread", + wraps=asyncio.to_thread, + ) as to_thread: + _ = [item async for item in method(account)] + + assert any( + call.args[:3] == (getattr, account, folder_attr) + for call in to_thread.call_args_list + ) + + +@pytest.mark.asyncio +async def test_get_mails_resolve_folder_off_event_loop(): + async with create_outlook_source() as source: + account = MockAccount() + with patch( + "connectors.sources.outlook.client.asyncio.to_thread", + wraps=asyncio.to_thread, + ) as to_thread: + _ = [item async for item in source.client.get_mails(account)] + + # Named folders resolve via getattr, off the loop. + assert any( + call.args[:3] == (getattr, account, "inbox") + for call in to_thread.call_args_list + ) + # The Archive leaf resolves via a lambda, also off the loop. + archive_calls = [c for c in to_thread.call_args_list if len(c.args) == 1] + assert archive_calls + assert archive_calls[0].args[0]().object_type == MAIL + + +@pytest.mark.asyncio +async def test_get_child_calendars_resolve_folder_off_event_loop(): + async with create_outlook_source() as source: + account = MockAccount() + with patch( + "connectors.sources.outlook.client.asyncio.to_thread", + wraps=asyncio.to_thread, + ) as to_thread: + _ = [item async for item in source.client.get_child_calendars(account)] + + # The first offloaded call resolves the child-calendar list. + resolved = to_thread.call_args_list[0].args[0]() + assert resolved == list(account.calendar.children) + + +@pytest.mark.asyncio +async def test_get_contacts_queries_distribution_list_fields(): + # The Contacts folder query must include DistributionList fields (members), + # or contact groups come back with no email addresses. + async with create_outlook_source() as source: + account = MockAccount() + with patch( + "connectors.sources.outlook.client.asyncio.to_thread", + wraps=asyncio.to_thread, + ) as to_thread: + _ = [contact async for contact in source.client.get_contacts(account)] + + assert any("members" in call.args[1:] for call in to_thread.call_args_list) + + +@pytest.mark.asyncio +async def test_fetch_contacts_raises_on_unexpected_item_type(): + # The Contacts folder should only yield Contact or DistributionList items; + # anything else breaks that assumption and must fail loudly. + async with create_outlook_source() as source: + unexpected = MagicMock() + source.client.get_contacts = AsyncIterator([unexpected]) + account = MockAccount() + + with pytest.raises(TypeError, match="Unexpected Contacts item type"): + async for _ in source._fetch_contacts(account=account, timezone=TIMEZONE): + pass + + +@pytest.mark.asyncio +async def test_fetch_contacts_routes_distribution_list_to_group_formatter(): + # DistributionList routes to the group formatter; a contact to the contact one. + async with create_outlook_source() as source: + member = MagicMock() + member.mailbox.email_address = "group.member@gmail.com" + distribution_list = MagicMock(spec=DistributionList) + distribution_list.id = "distribution_list_1" + distribution_list.last_modified_time = "2023-12-12T01:01:01Z" + distribution_list.display_name = "Dummy Group" + distribution_list.members = [member] + + source.client.get_contacts = AsyncIterator( + [distribution_list, ContactDocument()] + ) + account = MockAccount() + + documents = [ + document + async for document, _ in source._fetch_contacts( + account=account, timezone=TIMEZONE + ) + ] + + by_id = {document["_id"]: document for document in documents} + assert by_id["distribution_list_1"]["type"] == "Distribution List" + assert by_id["distribution_list_1"]["email_addresses"] == [ + "group.member@gmail.com" + ] + assert by_id["contact_1"]["type"] == "Contact" + + +@pytest.mark.asyncio +@patch("connectors.sources.outlook.client.Account", return_value="account") +async def test_exchange_get_user_accounts_handles_missing_type_key( + mock_account, reset_http_adapter_cls +): + # Some LDAP entries may not carry a "type" key; that must not raise KeyError. + user_without_type = {"attributes": {"mail": ["user@example.com"]}} + async with create_outlook_source() as source: + source.client.is_cloud = False + source.client._get_user_instance.get_users = AsyncIterator([user_without_type]) + + accounts = [ + account + async for account in source.client._get_user_instance.get_user_accounts() + ] + + assert accounts == ["account"] + + def test_mails_doc_formatter_handles_missing_sender(): mail = MailDocument() mail.sender = None @@ -1203,6 +1408,19 @@ def test_calendar_doc_formatter_handles_occurrence_without_recurrence(): assert document["meeting_type"] == "Occurrence" +def test_calendar_doc_formatter_handles_birthday_without_start(): + calendar = CalendarDocument() + calendar.start = None + + document = OutlookDocFormatter().calendar_doc_formatter( + calendar=calendar, + child_calendar="Birthdays (Birthdays)", + timezone=TIMEZONE, + ) + + assert document["date"] is None + + def test_calendar_doc_formatter_skips_attendees_without_mailbox(): calendar = CalendarDocument() attendee_without_mailbox = MagicMock() @@ -1232,6 +1450,21 @@ def test_contact_doc_formatter_handles_missing_email_and_phone_entries(): assert document["contact_numbers"] == [] +def test_distribution_list_doc_formatter(): + # A contact group is indexed by name plus its members' emails. + distribution_list = DistributionListDocument() + + document = OutlookDocFormatter().distribution_list_doc_formatter( + distribution_list=distribution_list, + timezone=TIMEZONE, + ) + + assert document["_id"] == "distribution_list_1" + assert document["type"] == "Distribution List" + assert document["name"] == "Dummy Group" + assert document["email_addresses"] == ["group.member@gmail.com"] + + @pytest.mark.asyncio async def test_fetch_attachments_skips_attachment_without_id(): async with create_outlook_source() as source: