From 7a1fc6b6264fb2d798bcafc932d268b24046cdbb Mon Sep 17 00:00:00 2001 From: Sabrina Demagny Date: Tue, 8 Apr 2025 22:14:08 +0200 Subject: [PATCH] =?UTF-8?q?=E2=9C=A8(mailbox)=20remove=20secondary=20email?= =?UTF-8?q?=20as=20required=20field?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The secondary email address is no longer required for all creation processes and we should not force the user to provide and store an insecure email address. --- CHANGELOG.md | 4 ++ .../mailbox_manager/api/client/serializers.py | 19 +++++--- .../mailbox_manager/api/client/viewsets.py | 2 +- .../0025_alter_mailbox_secondary_email.py | 18 ++++++++ src/backend/mailbox_manager/models.py | 2 +- .../mailboxes/test_api_mailboxes_create.py | 44 +++++++++++++++++-- .../tests/models/test_mailboxes.py | 12 ----- src/backend/mailbox_manager/utils/dimail.py | 20 +++++---- 8 files changed, 89 insertions(+), 32 deletions(-) create mode 100644 src/backend/mailbox_manager/migrations/0025_alter_mailbox_secondary_email.py diff --git a/CHANGELOG.md b/CHANGELOG.md index c8a6629..797ba4d 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -8,6 +8,10 @@ and this project adheres to ## [Unreleased] +### Changed + +- ✨(mailbox) remove secondary email as required field + ## [1.15.0] - 2025-04-04 ### Added diff --git a/src/backend/mailbox_manager/api/client/serializers.py b/src/backend/mailbox_manager/api/client/serializers.py index e224cf1..18680dc 100644 --- a/src/backend/mailbox_manager/api/client/serializers.py +++ b/src/backend/mailbox_manager/api/client/serializers.py @@ -53,12 +53,19 @@ class MailboxSerializer(serializers.ModelSerializer): mailbox.status = enums.MailDomainStatusChoices.ENABLED mailbox.save() - # send confirmation email - client.notify_mailbox_creation( - recipient=mailbox.secondary_email, - mailbox_data=response.json(), - issuer=self.context["request"].user, - ) + if mailbox.secondary_email: + # send confirmation email + client.notify_mailbox_creation( + recipient=mailbox.secondary_email, + mailbox_data=response.json(), + issuer=self.context["request"].user, + ) + else: + logger.warning( + "Email notification for %s creation not sent " + "because no secondary email found", + mailbox, + ) # actually save mailbox on our database return mailbox diff --git a/src/backend/mailbox_manager/api/client/viewsets.py b/src/backend/mailbox_manager/api/client/viewsets.py index b376b27..d9fc790 100644 --- a/src/backend/mailbox_manager/api/client/viewsets.py +++ b/src/backend/mailbox_manager/api/client/viewsets.py @@ -241,7 +241,7 @@ class MailBoxViewSet( - first_name: str - last_name: str - local_part: str - - secondary_email: str + - secondary_email: str (optional) Sends request to email provisioning API and returns newly created mailbox POST /api//mail-domains//mailboxes//disable/ diff --git a/src/backend/mailbox_manager/migrations/0025_alter_mailbox_secondary_email.py b/src/backend/mailbox_manager/migrations/0025_alter_mailbox_secondary_email.py new file mode 100644 index 0000000..a35f8a6 --- /dev/null +++ b/src/backend/mailbox_manager/migrations/0025_alter_mailbox_secondary_email.py @@ -0,0 +1,18 @@ +# Generated by Django 5.1.8 on 2025-04-08 20:07 + +from django.db import migrations, models + + +class Migration(migrations.Migration): + + dependencies = [ + ('mailbox_manager', '0024_domaininvitation'), + ] + + operations = [ + migrations.AlterField( + model_name='mailbox', + name='secondary_email', + field=models.EmailField(blank=True, max_length=254, null=True, verbose_name='secondary email address'), + ), + ] diff --git a/src/backend/mailbox_manager/models.py b/src/backend/mailbox_manager/models.py index dbbb92f..a5c9b85 100644 --- a/src/backend/mailbox_manager/models.py +++ b/src/backend/mailbox_manager/models.py @@ -284,7 +284,7 @@ class Mailbox(AbstractBaseUser, BaseModel): blank=False, ) secondary_email = models.EmailField( - _("secondary email address"), null=False, blank=False + _("secondary email address"), null=True, blank=True ) status = models.CharField( max_length=20, 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 a69520d..b38ba73 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 @@ -197,7 +197,7 @@ def test_api_mailboxes__create_with_accent_success(role): def test_api_mailboxes__create_administrator_missing_fields(): """ Administrator users should not be able to create mailboxes - without local part or secondary mail. + without local part. """ mail_domain = factories.MailDomainEnabledFactory() access = factories.MailDomainAccessFactory( @@ -219,18 +219,54 @@ def test_api_mailboxes__create_administrator_missing_fields(): assert not models.Mailbox.objects.exists() assert response.json() == {"local_part": ["This field is required."]} + +@responses.activate +@pytest.mark.parametrize( + "role", + [enums.MailDomainRoleChoices.OWNER, enums.MailDomainRoleChoices.ADMIN], +) +def test_api_mailboxes__create_without_secondary_email(role, caplog): + """ + Creating a new mailbox should not require a secondary email. + We should be able to create a mailbox but not send any email notification. + """ + mail_domain = factories.MailDomainEnabledFactory() + access = factories.MailDomainAccessFactory(role=role, domain=mail_domain) + client = APIClient() + client.force_login(access.user) mailbox_values = serializers.MailboxSerializer( factories.MailboxFactory.build() ).data del mailbox_values["secondary_email"] + + responses.add( + responses.GET, + re.compile(r".*/token/"), + body=TOKEN_OK, + status=status.HTTP_200_OK, + content_type="application/json", + ) + responses.add( + responses.POST, + re.compile(rf".*/domains/{mail_domain.name}/mailboxes/"), + body=response_mailbox_created( + f"{mailbox_values['local_part']}@{mail_domain.name}" + ), + status=status.HTTP_201_CREATED, + content_type="application/json", + ) response = client.post( f"/api/v1.0/mail-domains/{mail_domain.slug}/mailboxes/", mailbox_values, format="json", ) - assert response.status_code == status.HTTP_400_BAD_REQUEST - assert not models.Mailbox.objects.exists() - assert response.json() == {"secondary_email": ["This field is required."]} + assert response.status_code == status.HTTP_201_CREATED + mailbox = models.Mailbox.objects.get() + assert ( + caplog.records[0].message + == f"Email notification for {mailbox} creation not sent " + "because no secondary email found" + ) @pytest.mark.parametrize( diff --git a/src/backend/mailbox_manager/tests/models/test_mailboxes.py b/src/backend/mailbox_manager/tests/models/test_mailboxes.py index 62fd3a1..2929ca1 100644 --- a/src/backend/mailbox_manager/tests/models/test_mailboxes.py +++ b/src/backend/mailbox_manager/tests/models/test_mailboxes.py @@ -81,18 +81,6 @@ def test_models_mailboxes__domain_cannot_be_null(): # SECONDARY_EMAIL FIELD -def test_models_mailboxes__secondary_email_cannot_be_empty(): - """The "secondary_email" field should not be empty.""" - with pytest.raises(exceptions.ValidationError, match="This field cannot be blank"): - factories.MailboxFactory(secondary_email="") - - -def test_models_mailboxes__secondary_email_cannot_be_null(): - """The "secondary_email" field should not be null.""" - with pytest.raises(exceptions.ValidationError, match="This field cannot be null"): - factories.MailboxFactory(secondary_email=None) - - @pytest.mark.parametrize( "domain_status", [ diff --git a/src/backend/mailbox_manager/utils/dimail.py b/src/backend/mailbox_manager/utils/dimail.py index bbed1b0..e632c49 100644 --- a/src/backend/mailbox_manager/utils/dimail.py +++ b/src/backend/mailbox_manager/utils/dimail.py @@ -334,9 +334,6 @@ class DimailAPIClient: last_name=dimail_mailbox["surName"], local_part=address.username, domain=domain, - secondary_email=dimail_mailbox["email"], - # secondary email is mandatory. Unfortunately, dimail doesn't - # store any. We temporarily give current email as secondary email. status=enums.MailboxStatusChoices.ENABLED, password=make_password(None), # unusable password ) @@ -408,11 +405,18 @@ class DimailAPIClient: mailbox.status = enums.MailDomainStatusChoices.ENABLED mailbox.save() - # send confirmation email - self.notify_mailbox_creation( - recipient=mailbox.secondary_email, - mailbox_data=response.json(), - ) + if mailbox.secondary_email: + # send confirmation email + self.notify_mailbox_creation( + recipient=mailbox.secondary_email, + mailbox_data=response.json(), + ) + else: + logger.warning( + "Email notification for %s creation not sent " + "because no secondary email found", + mailbox, + ) def check_domain(self, domain): """Send a request to dimail to check domain health."""