From ba631fafb9c0fb708bc80985b74c4fa0ffeff5a2 Mon Sep 17 00:00:00 2001 From: Marie PUPO JEAMMET Date: Tue, 3 Sep 2024 18:29:28 +0200 Subject: [PATCH] =?UTF-8?q?=F0=9F=90=9B(dimail)=20improve=20handling=20of?= =?UTF-8?q?=20dimail=20errors=20on=20failed=20mailbox=20creation?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit dimail is called twice when creating a mailbox (once for the token, and once for the post on mailbox endpoint). we want to clarify the status_codes and messages of each error to inform user and ease debug --- CHANGELOG.md | 4 + .../mailboxes/test_api_mailboxes_create.py | 125 ++++++++++++++++++ .../tests/test_models_mailboxes.py | 9 +- src/backend/mailbox_manager/utils/dimail.py | 38 +++--- 4 files changed, 152 insertions(+), 24 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 8b8f9f0..245bc39 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -8,6 +8,10 @@ and this project adheres to ## [Unreleased] +### Fixed + +- 🐛(dimail) improve handling of dimail errors on failed mailbox creation #377 + ## [1.0.2] - 2024-08-30 ### Added 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 809262a..8304377 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 @@ -359,3 +359,128 @@ def test_api_mailboxes__domain_owner_or_admin_successful_creation_and_provisioni assert mailbox.last_name == mailbox_data["last_name"] assert mailbox.local_part == mailbox_data["local_part"] assert mailbox.secondary_email == mailbox_data["secondary_email"] + + +def test_api_mailboxes__wrong_secret_no_token_error(): + """ + API should raise a clear "permission denied" error + when receiving a 403_forbidden from dimail. + """ + # creating all needed objects + access = factories.MailDomainAccessFactory(role=enums.MailDomainRoleChoices.OWNER) + access.domain.secret = "nottherealsecret" + + client = APIClient() + client.force_login(access.user) + mailbox_data = serializers.MailboxSerializer( + factories.MailboxFactory.build(domain=access.domain) + ).data + + with responses.RequestsMock() as rsps: + # Ensure successful response using "responses": + rsps.add( + rsps.GET, + re.compile(r".*/token/"), + body='{"details": "Permission denied"}', + status=status.HTTP_403_FORBIDDEN, + content_type="application/json", + ) + + response = client.post( + f"/api/v1.0/mail-domains/{access.domain.slug}/mailboxes/", + mailbox_data, + format="json", + ) + + assert response.status_code == status.HTTP_403_FORBIDDEN + assert response.json() == { + "detail": f"Token denied - Wrong secret on mail domain {access.domain.name}" + } + assert not models.Mailbox.objects.exists() + + +def test_api_mailboxes__secret_unrelated_to_domain(): + """ + API should raise a clear "permission denied" + when secret allows for a token but is not linked to queried domain on dimail-api. + """ + # creating all needed objects + access = factories.MailDomainAccessFactory(role=enums.MailDomainRoleChoices.OWNER) + + client = APIClient() + client.force_login(access.user) + mailbox_data = serializers.MailboxSerializer( + factories.MailboxFactory.build(domain=access.domain) + ).data + + with responses.RequestsMock() as rsps: + # Ensure successful response using "responses": + rsps.add( + rsps.GET, + re.compile(r".*/token/"), + body='{"access_token": "domain_owner_token"}', + status=status.HTTP_200_OK, + content_type="application/json", + ) + rsps.add( + rsps.POST, + re.compile(rf".*/domains/{access.domain.name}/mailboxes/"), + body='{"details": "Permission denied"}', + status=status.HTTP_403_FORBIDDEN, + content_type="application/json", + ) + + response = client.post( + f"/api/v1.0/mail-domains/{access.domain.slug}/mailboxes/", + mailbox_data, + format="json", + ) + + assert response.status_code == status.HTTP_403_FORBIDDEN + assert response.json() == { + "detail": f"Secret not valid for this domain {access.domain.name}" + } + assert not models.Mailbox.objects.exists() + + +def test_api_mailboxes__handling_dimail_unexpected_error(): + """ + API should raise a clear error when dimail gives an unexpected response. + """ + # creating all needed objects + access = factories.MailDomainAccessFactory(role=enums.MailDomainRoleChoices.OWNER) + + client = APIClient() + client.force_login(access.user) + mailbox_data = serializers.MailboxSerializer( + factories.MailboxFactory.build(domain=access.domain) + ).data + + with responses.RequestsMock() as rsps: + # Ensure successful response using "responses": + rsps.add( + rsps.GET, + re.compile(r".*/token/"), + body='{"access_token": "domain_owner_token"}', + status=status.HTTP_200_OK, + content_type="application/json", + ) + rsps.add( + rsps.POST, + re.compile(rf".*/domains/{access.domain.name}/mailboxes/"), + body='{"details": "Internal server error"}', + status=status.HTTP_500_INTERNAL_SERVER_ERROR, + content_type="application/json", + ) + + with pytest.raises(SystemError): + response = client.post( + f"/api/v1.0/mail-domains/{access.domain.slug}/mailboxes/", + mailbox_data, + format="json", + ) + assert response.status_code == status.HTTP_500_INTERNAL_SERVER_ERROR + assert response.json() == { + "detail": "Unexpected response from dimail: {'details': 'Internal server error'}" + } + assert not models.Mailbox.objects.exists() diff --git a/src/backend/mailbox_manager/tests/test_models_mailboxes.py b/src/backend/mailbox_manager/tests/test_models_mailboxes.py index 19710bc..800f934 100644 --- a/src/backend/mailbox_manager/tests/test_models_mailboxes.py +++ b/src/backend/mailbox_manager/tests/test_models_mailboxes.py @@ -168,17 +168,10 @@ def test_models_mailboxes__wrong_secret(): status=status.HTTP_403_FORBIDDEN, content_type="application/json", ) - rsps.add( - rsps.POST, - re.compile(rf".*/domains/{domain.name}/mailboxes/"), - body='{"detail": "Permission denied"}', - status=status.HTTP_403_FORBIDDEN, - content_type="application/json", - ) with pytest.raises( exceptions.PermissionDenied, - match=f"Please check secret of the mail domain {domain.name}", + match=f"Token denied - Wrong secret on mail domain {domain.name}", ): mailbox = factories.MailboxFactory(use_mock=False, domain=domain) # Payload sent to mailbox provider diff --git a/src/backend/mailbox_manager/utils/dimail.py b/src/backend/mailbox_manager/utils/dimail.py index 3c1e676..f2f4359 100644 --- a/src/backend/mailbox_manager/utils/dimail.py +++ b/src/backend/mailbox_manager/utils/dimail.py @@ -38,13 +38,15 @@ class DimailAPIClient: response = requests.get( f"{self.API_URL}/token/", headers={"Authorization": f"Basic {domain.secret}"}, - timeout=status.HTTP_200_OK, + timeout=20, ) - if response.json() == "{'detail': 'Permission denied'}": - raise exceptions.PermissionDenied( - "This secret does not allow for a new token." + if response.status_code == status.HTTP_403_FORBIDDEN: + logger.error( + "[DIMAIL] 403 Forbidden: please check the mail domain secret of %s", + domain.name, ) + raise exceptions.PermissionDenied if "access_token" in response.json(): headers["Authorization"] = f"Bearer {response.json()['access_token']}" @@ -76,6 +78,10 @@ class DimailAPIClient: exc_info=error, ) raise error + except exceptions.PermissionDenied as error: + raise exceptions.PermissionDenied( + f"Token denied - Wrong secret on mail domain {mailbox.domain.name}" + ) from error if response.status_code == status.HTTP_201_CREATED: extra = {"response": response.content.decode("utf-8")} @@ -87,18 +93,18 @@ class DimailAPIClient: mailbox.domain.name, extra=extra, ) - elif response.status_code == status.HTTP_403_FORBIDDEN: - logger.error( - "[DIMAIL] 403 Forbidden: please check the mail domain secret of %s", - mailbox.domain.name, - ) + return response + + if response.status_code == status.HTTP_403_FORBIDDEN: raise exceptions.PermissionDenied( - f"Please check secret of the mail domain {mailbox.domain.name}" - ) - else: - logger.error( - "Unexpected response: %s", - response.content.decode("utf-8"), + f"Secret not valid for this domain {mailbox.domain.name}" ) - return response + # All other errors are considered 'unexpected' + logger.error( + "Unexpected response: %s", + response.content.decode("utf-8"), + ) + raise SystemError( + f"Unexpected response from dimail: {response.content.decode('utf-8')}" + )