diff --git a/CHANGELOG.md b/CHANGELOG.md index 08a8b1c..8326942 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -8,6 +8,7 @@ and this project adheres to ## [Unreleased] +- 🐛(dimail) allow mailboxes and aliases to have the same local part #986 - 🐛(dimail) ignore oxadmin when importing mailboxes from dimail #986 - ✨(aliases) delete all aliases in one call #1002 - ✨(aliases) fix deleting single aliases #1002 diff --git a/src/backend/mailbox_manager/api/client/serializers.py b/src/backend/mailbox_manager/api/client/serializers.py index b50f37e..6655e13 100644 --- a/src/backend/mailbox_manager/api/client/serializers.py +++ b/src/backend/mailbox_manager/api/client/serializers.py @@ -79,17 +79,6 @@ class MailboxSerializer(serializers.ModelSerializer): return mailbox - def validate_local_part(self, value): - """Validate this local part does not match a mailbox.""" - if models.Alias.objects.filter( - local_part=value, domain__slug=self.context["domain_slug"] - ): - raise exceptions.ValidationError( - f'Local part "{value}" already used by an alias.' - ) - - return value - class MailboxUpdateSerializer(MailboxSerializer): """A more restrictive serializer when updating mailboxes""" @@ -338,14 +327,3 @@ class AliasSerializer(serializers.ModelSerializer): return super().create(validated_data) return None - - def validate_local_part(self, value): - """Validate this local part does not match a mailbox.""" - if models.Mailbox.objects.filter( - local_part=value, domain__slug=self.context["domain_slug"] - ).exists(): - raise exceptions.ValidationError( - f'Local part "{value}" already used by a mailbox.' - ) - - return value diff --git a/src/backend/mailbox_manager/tests/api/aliases/test_api_aliases_create.py b/src/backend/mailbox_manager/tests/api/aliases/test_api_aliases_create.py index b085e48..f7211a4 100644 --- a/src/backend/mailbox_manager/tests/api/aliases/test_api_aliases_create.py +++ b/src/backend/mailbox_manager/tests/api/aliases/test_api_aliases_create.py @@ -81,26 +81,6 @@ def test_api_aliases_create__duplicate_forbidden(): assert models.Alias.objects.filter(domain=access.domain).count() == 1 -def test_api_aliases_create__existing_mailbox_bad_request(): - """Cannot create alias if local_part is already used by a mailbox.""" - access = factories.MailDomainAccessFactory( - role="owner", domain=factories.MailDomainEnabledFactory() - ) - mailbox = factories.MailboxFactory(domain=access.domain) - - client = APIClient() - client.force_login(access.user) - response = client.post( - f"/api/v1.0/mail-domains/{access.domain.slug}/aliases/", - {"local_part": mailbox.local_part, "destination": "someone@outsidedomain.com"}, - ) - assert response.status_code == status.HTTP_400_BAD_REQUEST - assert response.json() == { - "local_part": [f'Local part "{mailbox.local_part}" already used by a mailbox.'] - } - assert not models.Alias.objects.exists() - - @responses.activate def test_api_aliases_create__async_alias_bad_request(dimail_token_ok): """ @@ -151,17 +131,14 @@ def test_api_aliases_create__admins_ok(role, dimail_token_ok): client.force_login(access.user) # Prepare responses # token response in fixtures - responses.add( - responses.POST, + responses.post( re.compile(rf".*/domains/{access.domain.name}/aliases/"), - body=json.dumps( - { - "username": "contact", - "domain": access.domain.name, - "destination": "someone@outsidedomain.com", - "allow_to_send": True, - } - ), + json={ + "username": "contact", + "domain": access.domain.name, + "destination": "someone@outsidedomain.com", + "allow_to_send": True, + }, status=status.HTTP_201_CREATED, content_type="application/json", ) @@ -174,3 +151,34 @@ def test_api_aliases_create__admins_ok(role, dimail_token_ok): alias = models.Alias.objects.get() assert alias.local_part == "contact" assert alias.destination == "someone@outsidedomain.com" + + +@responses.activate +def test_api_aliases_create__existing_mailbox_ok(dimail_token_ok): + """Can create alias even if local_part is already used by a mailbox.""" + access = factories.MailDomainAccessFactory( + role="owner", domain=factories.MailDomainEnabledFactory() + ) + mailbox = factories.MailboxFactory(domain=access.domain) + + client = APIClient() + client.force_login(access.user) + + responses.post( + re.compile(rf".*/domains/{access.domain.name}/aliases/"), + json={ + "username": mailbox.local_part, + "domain": access.domain.name, + "destination": "someone@outsidedomain.com", + "allow_to_send": False, + }, + status=status.HTTP_201_CREATED, + content_type="application/json", + ) + + response = client.post( + f"/api/v1.0/mail-domains/{access.domain.slug}/aliases/", + {"local_part": mailbox.local_part, "destination": "someone@outsidedomain.com"}, + ) + assert response.status_code == status.HTTP_201_CREATED + assert models.Alias.objects.exists() diff --git a/src/backend/mailbox_manager/tests/api/mailboxes/test_api_mailboxes_create.py b/src/backend/mailbox_manager/tests/api/mailboxes/test_api_mailboxes_create.py index d9edc0f..c31a2e0 100644 --- a/src/backend/mailbox_manager/tests/api/mailboxes/test_api_mailboxes_create.py +++ b/src/backend/mailbox_manager/tests/api/mailboxes/test_api_mailboxes_create.py @@ -439,10 +439,9 @@ def test_api_mailboxes__create_pending_mailboxes(domain_status, mailbox_data): assert mailbox.status == "pending" -def test_api_mailboxes__existing_alias_bad_request(mailbox_data): - """ - Cannot create mailbox if local_part is already used by an alias. - """ +@responses.activate +def test_api_mailboxes__existing_alias_ok(mailbox_data, dimail_token_ok): + """Can create mailbox even if local_part is already used by an alias.""" alias = factories.AliasFactory() access = factories.MailDomainAccessFactory( role=enums.MailDomainRoleChoices.ADMIN, domain=alias.domain @@ -450,7 +449,16 @@ def test_api_mailboxes__existing_alias_bad_request(mailbox_data): client = APIClient() client.force_login(access.user) - # No response because we expect no outside calls to be made + + responses.post( + re.compile(rf".*/domains/{access.domain.name}/mailboxes/"), + body=response_mailbox_created( + f"{mailbox_data['local_part']}@{access.domain.name}" + ), + status=status.HTTP_201_CREATED, + content_type="application/json", + ) + response = client.post( f"/api/v1.0/mail-domains/{access.domain.slug}/mailboxes/", { @@ -461,11 +469,8 @@ def test_api_mailboxes__existing_alias_bad_request(mailbox_data): }, format="json", ) - assert response.status_code == status.HTTP_400_BAD_REQUEST - assert response.json() == { - "local_part": [f'Local part "{alias.local_part}" already used by an alias.'] - } - assert not models.Mailbox.objects.exists() + assert response.status_code == status.HTTP_201_CREATED + assert models.Mailbox.objects.exists() ### REACTING TO DIMAIL-API 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 c18cd5d..d5b54a8 100644 --- a/src/backend/mailbox_manager/tests/test_utils_dimail_client.py +++ b/src/backend/mailbox_manager/tests/test_utils_dimail_client.py @@ -66,8 +66,8 @@ def test_dimail_synchronization__already_sync(dimail_token_ok): @responses.activate def test_dimail_synchronization__synchronize_mailboxes(caplog, dimail_token_ok): # pylint: disable=W0613, R0914 - """A mailbox existing solely on dimail should be synchronized - upon calling sync function on its domain""" + """Importing mailboxes from dimail should synchronize valid mailboxes + and log errors for invalid ones.""" caplog.set_level(logging.INFO) domain = factories.MailDomainEnabledFactory() @@ -143,7 +143,7 @@ def test_dimail_synchronization__synchronize_mailboxes(caplog, dimail_token_ok): imported_mailboxes = dimail_client.import_mailboxes(domain) # 4 imports failed: oxadmin, wrong domain, HeaderParseError, NonASCIILocalPartDefect - assert len(caplog.records) == 6 + assert len(caplog.records) == 5 log_messages = [record.message for record in caplog.records] expected_messages = [ @@ -152,21 +152,21 @@ def test_dimail_synchronization__synchronize_mailboxes(caplog, dimail_token_ok): f"Import of email {mailbox_with_invalid_domain['email']} failed with error Invalid Domain", f"Import of email {mailbox_with_invalid_local_part['email']} failed with error local-part \ contains non-ASCII characters)", - f"{existing_alias.local_part} already used in an existing alias.", ] for message in expected_messages: assert message in log_messages - assert imported_mailboxes == [mailbox_valid["email"]] - mailbox = models.Mailbox.objects.get() - assert mailbox.local_part == "validmailbox" - assert mailbox.status == enums.MailboxStatusChoices.ENABLED + assert models.Mailbox.objects.count() == 2 + assert imported_mailboxes == [ + mailbox_valid["email"], + mailbox_existing_alias["email"], + ] @responses.activate def test_dimail_synchronization__synchronize_aliases(dimail_token_ok): # pylint: disable=unused-argument - """Should import aliases from dimail if they don't already exist - and if username is not already used for mailbox""" + """Importing aliases from dimail should synchronize valid aliases + and log errors for invalid ones.""" alias = factories.AliasFactory() dimail_client = DimailAPIClient() @@ -178,11 +178,11 @@ def test_dimail_synchronization__synchronize_aliases(dimail_token_ok): # pylint { "username": "contact", "domain": alias.domain.name, - "destination": alias.destination, # same destination + "destination": alias.destination, # same destination = ok "allow_to_send": False, }, { - "username": alias.local_part, # same username + "username": alias.local_part, # same username = ok "domain": alias.domain.name, "destination": "maheius.endorecles@somethingelse.com", "allow_to_send": False, @@ -193,7 +193,7 @@ def test_dimail_synchronization__synchronize_aliases(dimail_token_ok): # pylint "destination": alias.destination, "allow_to_send": False, }, - { # username already used for a mailbox + { # mailbox with same username = ok "username": existing_mailbox.local_part, "domain": alias.domain.name, "destination": existing_mailbox.secondary_email, @@ -209,8 +209,8 @@ def test_dimail_synchronization__synchronize_aliases(dimail_token_ok): # pylint imported_aliases = dimail_client.import_aliases(alias.domain) - assert len(imported_aliases) == 2 - assert models.Alias.objects.count() == 3 + assert len(imported_aliases) == 3 + assert models.Alias.objects.count() == 4 @pytest.mark.parametrize( diff --git a/src/backend/mailbox_manager/utils/dimail.py b/src/backend/mailbox_manager/utils/dimail.py index 831459b..cad338e 100644 --- a/src/backend/mailbox_manager/utils/dimail.py +++ b/src/backend/mailbox_manager/utils/dimail.py @@ -394,16 +394,6 @@ class DimailAPIClient: ) continue - if address.username in [ - alias_.local_part - for alias_ in models.Alias.objects.filter(domain=domain) - ]: - logger.warning( - "%s already used in an existing alias.", - address.username, - ) - continue - if str(address) not in [ str(people_mailbox) for people_mailbox in people_mailboxes ]: @@ -842,18 +832,13 @@ class DimailAPIClient: (known_alias.local_part, known_alias.destination) for known_alias in models.Alias.objects.filter(domain=domain) ] - known_mailboxes = [ - known_mailbox.local_part - for known_mailbox in models.Mailbox.objects.filter(domain=domain) - ] + imported_aliases = [] for incoming_alias in incoming_aliases: if ( incoming_alias["username"], incoming_alias["destination"], - ) not in known_aliases and incoming_alias[ - "username" - ] not in known_mailboxes: + ) not in known_aliases: try: new_alias = models.Alias.objects.create( local_part=incoming_alias["username"],