From 21a5efa7fa7d2f426632eb16e6f8b7d2fba006d9 Mon Sep 17 00:00:00 2001 From: Marie PUPO JEAMMET Date: Thu, 19 Dec 2024 16:07:23 +0100 Subject: [PATCH] =?UTF-8?q?=F0=9F=90=9B(dimail)=20fix=20mailboxes=20import?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit fix mailboxes import --- .../tests/test_utils_dimail_client.py | 87 ++++++++++++------- src/backend/mailbox_manager/utils/dimail.py | 10 +-- 2 files changed, 59 insertions(+), 38 deletions(-) diff --git a/src/backend/mailbox_manager/tests/test_utils_dimail_client.py b/src/backend/mailbox_manager/tests/test_utils_dimail_client.py index 5aa59c97b..21e6a2c55 100644 --- a/src/backend/mailbox_manager/tests/test_utils_dimail_client.py +++ b/src/backend/mailbox_manager/tests/test_utils_dimail_client.py @@ -2,10 +2,8 @@ Unit tests for dimail client """ +import logging import re -from email.errors import HeaderParseError, NonASCIILocalPartDefect -from logging import Logger -from unittest import mock import pytest import responses @@ -49,10 +47,11 @@ def test_dimail_synchronization__already_sync(): "givenName": mailbox.first_name, "surName": mailbox.last_name, "displayName": f"{mailbox.first_name} {mailbox.last_name}", + "additionalSenders": [], } for mailbox in pre_sync_mailboxes ] - ), + ).replace("'", '"'), status=status.HTTP_200_OK, content_type="application/json", ) @@ -64,10 +63,11 @@ def test_dimail_synchronization__already_sync(): assert set(models.Mailbox.objects.filter(domain=domain)) == set(pre_sync_mailboxes) -@mock.patch.object(Logger, "warning") -def test_dimail_synchronization__synchronize_mailboxes(mock_warning): - """A mailbox existing solely on dimail should be synchronized - upon calling sync function on its domain""" +def test_dimail_synchronization__synchronize_mailboxes(caplog): + """A mailbox existing only on dimail should be synchronized + upon calling sync function on its domain.""" + caplog.set_level(logging.INFO) + domain = factories.MailDomainEnabledFactory() assert not models.Mailbox.objects.exists() @@ -89,6 +89,16 @@ def test_dimail_synchronization__synchronize_mailboxes(mock_warning): "givenName": "Admin", "surName": "Context", "displayName": "Context Admin", + "additionalSenders": [], + } + mailbox_valid_additional_senders = { + "type": "mailbox", + "status": "broken", + "email": f"prudence_crandall@{domain.name}", + "givenName": "Admin", + "surName": "Context", + "displayName": "Context Admin", + "additionalSenders": ["prudence@ct.gov"], } mailbox_with_wrong_domain = { "type": "mailbox", @@ -98,7 +108,7 @@ def test_dimail_synchronization__synchronize_mailboxes(mock_warning): "surName": "Doe", "displayName": "John Doe", } - mailbox_with_invalid_domain = { + mailbox_invalid_domain = { "type": "mailbox", "status": "broken", "email": f"naw@ake@{domain.name}", @@ -121,36 +131,47 @@ def test_dimail_synchronization__synchronize_mailboxes(mock_warning): body=str( [ mailbox_valid, + mailbox_valid_additional_senders, mailbox_with_wrong_domain, - mailbox_with_invalid_domain, + mailbox_invalid_domain, mailbox_with_invalid_local_part, ] - ), + ).replace("'", '"'), status=status.HTTP_200_OK, content_type="application/json", ) imported_mailboxes = dimail_client.import_mailboxes(domain) - # 3 imports failed: wrong domain, HeaderParseError, NonASCIILocalPartDefect - assert mock_warning.call_count == 3 - - # first we try to import email with a wrong domain - assert mock_warning.call_args_list[0][0] == ( - "Import of email %s failed because of a wrong domain", - mailbox_with_wrong_domain["email"], - ) - - # then we try to import email with invalid domain - invalid_mailbox_log = mock_warning.call_args_list[1][0] - assert invalid_mailbox_log[1] == mailbox_with_invalid_domain["email"] - assert isinstance(invalid_mailbox_log[2], HeaderParseError) - - # finally we try to import email with non ascii local part - non_ascii_mailbox_log = mock_warning.call_args_list[2][0] - assert non_ascii_mailbox_log[1] == mailbox_with_invalid_local_part["email"] - assert isinstance(non_ascii_mailbox_log[2], NonASCIILocalPartDefect) - - mailbox = models.Mailbox.objects.get() - assert mailbox.local_part == "oxadmin" - assert imported_mailboxes == [mailbox_valid["email"]] + # 1 token requested + # + 3 imports failed: wrong domain, HeaderParseError, NonASCIILocalPartDefect + assert len(caplog.records) == 4 + + # first we try to import email with a wrong domain + assert ( + caplog.records[1].message + == f"Import of email {mailbox_with_wrong_domain["email"]} failed because of a wrong domain" + ) + + # then we try to import email with invalid domain + invalid_mailbox_log = caplog.records[2] + assert ( + invalid_mailbox_log.message + == f"Import of email {mailbox_invalid_domain['email']} failed with error Invalid Domain" + ) + + # then we try to import email with non ascii local part + non_ascii_mailbox_log = caplog.records[3] + email = mailbox_with_invalid_local_part["email"] + assert ( + non_ascii_mailbox_log.message + == f"Import of email {email} failed with error local-part contains non-ASCII characters)" + ) + + assert imported_mailboxes == [ + mailbox_valid["email"], + mailbox_valid_additional_senders["email"], + ] + mailboxes = models.Mailbox.objects.all() + assert mailboxes[0].local_part == "oxadmin" + assert mailboxes[1].local_part == "prudence_crandall" diff --git a/src/backend/mailbox_manager/utils/dimail.py b/src/backend/mailbox_manager/utils/dimail.py index b00515611..a1a94f1b6 100644 --- a/src/backend/mailbox_manager/utils/dimail.py +++ b/src/backend/mailbox_manager/utils/dimail.py @@ -1,6 +1,5 @@ """A minimalist client to synchronize with mailbox provisioning API.""" -import ast import json import smtplib from email.errors import HeaderParseError, NonASCIILocalPartDefect @@ -312,9 +311,7 @@ def import_mailboxes(self, domain): if response.status_code != status.HTTP_200_OK: return self.raise_exception_for_unexpected_response(response) - dimail_mailboxes = ast.literal_eval( - response.content.decode("utf-8") - ) # format output str to proper list + dimail_mailboxes = response.json() people_mailboxes = models.Mailbox.objects.filter(domain=domain) imported_mailboxes = [] @@ -326,7 +323,10 @@ def import_mailboxes(self, domain): # sometimes dimail api returns email from another domain, # so we decide to exclude this kind of email address = Address(addr_spec=dimail_mailbox["email"]) - if address.domain == domain.name: + if ( + address.domain == domain.name + and dimail_mailbox["givenName"] is not None + ): # creates a mailbox on our end mailbox = models.Mailbox.objects.create( first_name=dimail_mailbox["givenName"],