✨(dimail) synchronize mailboxes from dimail to our db #453

mjeammet · 2024-10-09T15:25:19Z

Purpose

I want to synchronize mailboxes FROM dimail TO people, by instance if a domain has already been existing for a while (i.e. beta.gouv)

Proposal

I don't think this synchronization needs to be on a CRON. Create a route below /domains/ to call for a synchronization.

"synchronize from dimail" function in dimail client
link function to admin view : in domain admin, you can select a list of domains, and start import from dimail
test an email is successfully added when a single email is missing
test nothing is added when people and dimail are in sync 💞

sdemagny · 2024-10-30T15:35:26Z

src/backend/mailbox_manager/utils/dimail.py

+        This is not considered a new email and will not trigger any mail notification."""
+
+        domain = models.MailDomain.objects.get(name=domain_name)
+        people_mailboxes = models.Mailbox.objects.filter(domain=domain)


models.Mailbox.objects.values_list("local_part", flat=True)

sdemagny · 2024-10-31T18:18:08Z

src/backend/mailbox_manager/utils/dimail.py

+                    ],  # secondary email is mandatory. Unfortunately, dimail doesn't store any.
+                    # We temporarily give current email as secondary email.
+                )
+        return None


est-ce qu'on pourrait pas renvoyer des infos utiles comme combien de boite mail on était importé pour chaque domain synchronisé ?

J'ai implémenté ma proposition dans un second commit. Je rajouterai des tests si c'est ok.

sdemagny · 2024-11-04T10:50:53Z

src/backend/mailbox_manager/utils/dimail.py

+                mailbox = models.Mailbox.objects.create(
+                    first_name=dimail_mailbox["givenName"],
+                    last_name=dimail_mailbox["surName"],
+                    local_part=dimail_mailbox["email"].split("@")[0],


Est-ce qu'on ne devrait pas vérifier que la deuxième partie correspond bien au domain name ?

Can you consider using from email.headerregistry import Address here to prevent any CVE (I did the same mistake elsewhere)? I know we should trust dimail but anyway I think it's safer to use battle-tested Python helpers here, just in case.

Bonne question. Je préfèrerais qu'on laisse dimail tester de son côté que son API fonctionne bien comme il faut.

Si tu veux vérifier si le fix a été poussé, tu peux vérifier sur la prod de dimail que betagouv ne renvoie bien que des adresses betagouv (ou dans leur codebase que le test existe bien de leur côté)

Mais si tu veux faire ceinture+bretelle, on peut ajouter un filtre ligne 196 pour ne garder que les dimail_mailboxes qui correspondent au domaine.

EDIT : je confirme qu'au moment de mon test lundi 13h, dimail a toujours au moins un domaine avec plusieurs domaines dans les adresses.

sdemagny · 2024-11-04T10:51:39Z

src/backend/mailbox_manager/tests/test_utils_dimail_client.py

+                    {
+                        "type": "mailbox",
+                        "status": "broken",
+                        "email": "[email protected]",


le nom de domaine ne match pas avec le factory au dessus

qbey · 2024-11-04T11:05:33Z

src/backend/mailbox_manager/utils/dimail.py

+        dimail_mailboxes = ast.literal_eval(
+            response.content.decode("utf-8")
+        )  # format output str to proper list


Why is response.json() not proper here?

I think dimail's response is not json-ready !

qbey · 2024-11-04T11:08:28Z

src/backend/mailbox_manager/utils/dimail.py

+                mailbox = models.Mailbox.objects.create(
+                    first_name=dimail_mailbox["givenName"],
+                    last_name=dimail_mailbox["surName"],
+                    local_part=dimail_mailbox["email"].split("@")[0],


Can you consider using from email.headerregistry import Address here to prevent any CVE (I did the same mistake elsewhere)? I know we should trust dimail but anyway I think it's safer to use battle-tested Python helpers here, just in case.

Synchronize mailboxes existing on dimail's api and not on our side, on domains we are administrating.

mjeammet added the backend label Oct 9, 2024

mjeammet requested a review from sdemagny October 9, 2024 15:25

mjeammet assigned sdemagny and mjeammet Oct 9, 2024

mjeammet force-pushed the mpj/dimail/synchronize-mailboxes-from-dimail branch 2 times, most recently from b5ae653 to 7376efc Compare October 9, 2024 16:25

Morendil force-pushed the mpj/dimail/synchronize-mailboxes-from-dimail branch from 7376efc to f4d82cc Compare October 30, 2024 15:08

sdemagny reviewed Oct 30, 2024

View reviewed changes

mjeammet force-pushed the mpj/dimail/synchronize-mailboxes-from-dimail branch from f4d82cc to 4357e59 Compare October 30, 2024 15:49

sdemagny marked this pull request as ready for review October 31, 2024 14:55

sdemagny force-pushed the mpj/dimail/synchronize-mailboxes-from-dimail branch 4 times, most recently from 68f4532 to 439bf14 Compare October 31, 2024 18:06

sdemagny approved these changes Oct 31, 2024

View reviewed changes

sdemagny force-pushed the mpj/dimail/synchronize-mailboxes-from-dimail branch 2 times, most recently from 3d3d958 to 12e85cd Compare October 31, 2024 18:17

sdemagny reviewed Oct 31, 2024

View reviewed changes

sdemagny reviewed Nov 4, 2024

View reviewed changes

sdemagny force-pushed the mpj/dimail/synchronize-mailboxes-from-dimail branch from 60e3fa0 to 60b68c4 Compare November 4, 2024 10:52

qbey reviewed Nov 4, 2024

View reviewed changes

sdemagny force-pushed the mpj/dimail/synchronize-mailboxes-from-dimail branch from 7f3375a to d26c03e Compare November 8, 2024 14:51

qbey approved these changes Nov 8, 2024

View reviewed changes

sdemagny force-pushed the mpj/dimail/synchronize-mailboxes-from-dimail branch from d26c03e to 2175e3d Compare November 8, 2024 15:34

✨(dimail) synchronize mailboxes from dimail to our db

edde9c8

Synchronize mailboxes existing on dimail's api and not on our side, on domains we are administrating.

sdemagny force-pushed the mpj/dimail/synchronize-mailboxes-from-dimail branch from 2175e3d to edde9c8 Compare November 8, 2024 15:40

sdemagny merged commit edde9c8 into main Nov 8, 2024
20 checks passed

sdemagny deleted the mpj/dimail/synchronize-mailboxes-from-dimail branch November 8, 2024 15:51

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

✨(dimail) synchronize mailboxes from dimail to our db #453

✨(dimail) synchronize mailboxes from dimail to our db #453

mjeammet commented Oct 9, 2024 •

edited

Loading

sdemagny Oct 30, 2024 •

edited

Loading

sdemagny Oct 31, 2024

sdemagny Oct 31, 2024 •

edited

Loading

sdemagny Nov 4, 2024

qbey Nov 4, 2024

mjeammet Nov 4, 2024 •

edited

Loading

sdemagny Nov 4, 2024

qbey Nov 4, 2024

mjeammet Nov 4, 2024

qbey Nov 4, 2024

✨(dimail) synchronize mailboxes from dimail to our db #453

✨(dimail) synchronize mailboxes from dimail to our db #453

Conversation

mjeammet commented Oct 9, 2024 • edited Loading

Purpose

Proposal

sdemagny Oct 30, 2024 • edited Loading

Choose a reason for hiding this comment

sdemagny Oct 31, 2024

Choose a reason for hiding this comment

sdemagny Oct 31, 2024 • edited Loading

Choose a reason for hiding this comment

sdemagny Nov 4, 2024

Choose a reason for hiding this comment

qbey Nov 4, 2024

Choose a reason for hiding this comment

mjeammet Nov 4, 2024 • edited Loading

Choose a reason for hiding this comment

sdemagny Nov 4, 2024

Choose a reason for hiding this comment

qbey Nov 4, 2024

Choose a reason for hiding this comment

mjeammet Nov 4, 2024

Choose a reason for hiding this comment

qbey Nov 4, 2024

Choose a reason for hiding this comment

mjeammet commented Oct 9, 2024 •

edited

Loading

sdemagny Oct 30, 2024 •

edited

Loading

sdemagny Oct 31, 2024 •

edited

Loading

mjeammet Nov 4, 2024 •

edited

Loading