Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

✨(dimail) allow to reset the mailbox password #834

Open
wants to merge 2 commits into
base: main
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,7 @@ and this project adheres to
- ✨(domains) define domain check interval as a settings
- ✨(oidc) add simple introspection backend #832
- 🧑‍💻(tasks) run management commands #814
- ✨(mailbox) allow to reset password on mailboxes #834

### Changed

Expand Down
2 changes: 1 addition & 1 deletion docker-compose.yml
Original file line number Diff line number Diff line change
Expand Up @@ -173,7 +173,7 @@ services:

dimail:
entrypoint: /opt/dimail-api/start-dev.sh
image: registry.mim-libre.fr/dimail/dimail-api:v0.1.1
image: registry.mim-libre.fr/dimail/dimail-api:v0.2.11
pull_policy: always
environment:
DIMAIL_MODE: FAKE
Expand Down
3 changes: 2 additions & 1 deletion src/backend/mailbox_manager/api/client/serializers.py
Original file line number Diff line number Diff line change
Expand Up @@ -54,10 +54,11 @@ def create(self, validated_data):
mailbox.save()

# send confirmation email
client.notify_mailbox_creation(
client.notify_mailbox_info(
recipient=mailbox.secondary_email,
mailbox_data=response.json(),
issuer=self.context["request"].user,
is_new_mailbox=True,
)

# actually save mailbox on our database
Expand Down
9 changes: 9 additions & 0 deletions src/backend/mailbox_manager/api/client/viewsets.py
Original file line number Diff line number Diff line change
Expand Up @@ -293,6 +293,15 @@ def enable(self, request, domain_slug, pk=None): # pylint: disable=unused-argum
mailbox.save()
return Response(serializers.MailboxSerializer(mailbox).data)

@action(detail=True, methods=["post"])
def reset_password(self, request, domain_slug, pk=None): # pylint: disable=unused-argument
"""Send a request to dimail to change password
and email new password to mailbox's secondary email."""
mailbox = self.get_object()
client = DimailAPIClient()
client.reset_password(mailbox)
return Response(serializers.MailboxSerializer(mailbox).data)


class MailDomainInvitationViewset(
mixins.CreateModelMixin,
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,115 @@
"""
Unit tests for the reset password mailbox API
"""

from unittest import mock

from django.conf import settings

import pytest
import responses
from rest_framework import status
from rest_framework.test import APIClient

from core import factories as core_factories

from mailbox_manager import enums, factories
from mailbox_manager.tests.fixtures import dimail
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

mieux vaut un import explicite ici from .fixtures.dimail import response_mailbox_created

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

oui j'ai tendance à importer tout le package pour pouvoir écrire (et lire) dimail.response_mailbox_created . Y a déjà plein de bazar différent sur ce repo, j'aime bien avoir un indice en plus d'où sont les choses.

Pourquoi tu trouves que c'est mieux ? je lis qu'en terme de perf ça fait pas une diff significative


pytestmark = pytest.mark.django_db


def test_api_mailboxes__reset_password_anonymous_unauthorized():
"""Anonymous users should not be able to reset mailboxes password."""
mailbox = factories.MailboxFactory(status=enums.MailboxStatusChoices.ENABLED)
response = APIClient().post(
f"/api/v1.0/mail-domains/{mailbox.domain.slug}/mailboxes/{mailbox.pk}/reset_password/",
)
assert response.status_code == status.HTTP_401_UNAUTHORIZED


def test_api_mailboxes__reset_password_unrelated_forbidden():
"""Authenticated users not managing the domain
should not be able to reset its mailboxes password."""
user = core_factories.UserFactory()

client = APIClient()
client.force_login(user)

mailbox = factories.MailboxFactory(status=enums.MailboxStatusChoices.ENABLED)

response = client.post(
f"/api/v1.0/mail-domains/{mailbox.domain.slug}/mailboxes/{mailbox.pk}/reset_password/"
)
assert response.status_code == status.HTTP_403_FORBIDDEN
assert response.json() == {
"detail": "You do not have permission to perform this action."
}


def test_api_mailboxes__reset_password_viewer_forbidden():
"""Domain viewers should not be able to reset passwords on mailboxes."""
mailbox = factories.MailboxEnabledFactory()
viewer_access = factories.MailDomainAccessFactory(
role=enums.MailDomainRoleChoices.VIEWER, domain=mailbox.domain
)

client = APIClient()
client.force_login(viewer_access.user)

response = client.post(
f"/api/v1.0/mail-domains/{mailbox.domain.slug}/mailboxes/{mailbox.pk}/reset_password/"
)
assert response.status_code == status.HTTP_403_FORBIDDEN
assert response.json() == {
"detail": "You do not have permission to perform this action."
}


@pytest.mark.parametrize(
"role",
[
enums.MailDomainRoleChoices.OWNER,
enums.MailDomainRoleChoices.ADMIN,
],
)
@responses.activate
def test_api_mailboxes__reset_password_admin_successful(role):
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nom à corriger

Copy link
Member Author

@mjeammet mjeammet Mar 28, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pour ajouter le fait que le owner est successful aussi ? Si c'est ça, est-ce que le mot "managers" te semble convenable pour englober admins et owners ?

"""Owner and admin users should be able to reset password on mailboxes.
New password should be sent to secondary email."""
mail_domain = factories.MailDomainEnabledFactory()
mailbox = factories.MailboxEnabledFactory(domain=mail_domain)

access = factories.MailDomainAccessFactory(role=role, domain=mail_domain)
client = APIClient()
client.force_login(access.user)
dimail_url = settings.MAIL_PROVISIONING_API_URL

responses.add(
responses.POST,
f"{dimail_url}/domains/{mail_domain.name}/mailboxes/{mailbox.local_part}/reset_password/",
body=dimail.response_mailbox_created(str(mailbox)),
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

cf remarque plus haut sur l'import explicite

status=200,
)
with mock.patch("django.core.mail.send_mail") as mock_send:
response = client.post(
f"/api/v1.0/mail-domains/{mail_domain.slug}/mailboxes/{mailbox.pk}/reset_password/"
)

assert mock_send.call_count == 1
assert "Your password has been updated" in mock_send.mock_calls[0][1][1]
assert mock_send.mock_calls[0][1][3][0] == mailbox.secondary_email

assert response.status_code == status.HTTP_200_OK


def test_api_mailboxes__reset_password_non_existing():
"""
User gets a 404 when trying to reset password of mailbox which does not exist.
"""
user = core_factories.UserFactory()
client = APIClient()
client.force_login(user)

response = client.get("/api/v1.0/mail-domains/nonexistent.domain/mailboxes/")
assert response.status_code == status.HTTP_404_NOT_FOUND
90 changes: 69 additions & 21 deletions src/backend/mailbox_manager/utils/dimail.py
Original file line number Diff line number Diff line change
@@ -1,3 +1,5 @@
# pylint: disable=line-too-long

"""A minimalist client to synchronize with mailbox provisioning API."""

import ast
Expand Down Expand Up @@ -255,40 +257,53 @@ def raise_exception_for_unexpected_response(self, response):
f"[DIMAIL] unexpected error: {response.status_code} {error_content}"
)

def notify_mailbox_creation(self, recipient, mailbox_data, issuer=None):
def notify_mailbox_info(
self,
recipient,
mailbox_data,
is_new_mailbox,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

est-ce que is_new suffirait ?

Copy link
Member Author

@mjeammet mjeammet Mar 28, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Je veux bien laisser is_new_mailbox pour que ce soit explicite.

parmi les autres sens possible, ça pourrait notamment être un attribut de la notification, qui pourrait ne pas être nouvelle mais envoyée plusieurs fois, genre "votre domaine est en échec" ou "vous êtes invités à tel espace" ou que sais-je.

issuer=None,
):
"""
Send email to confirm mailbox creation
and send new mailbox information.
Send email with new mailbox information or password reset.
"""
if is_new_mailbox:
title = _("Your new mailbox information")
template_name = "new_mailbox"
else:
title = _("Your password has been updated")
template_name = "reset_password"
context = {
"title": title,
"site": Site.objects.get_current(),
"webmail_url": settings.WEBMAIL_URL,
"mailbox_data": mailbox_data,
}

try:
with override(issuer.language if issuer else settings.LANGUAGE_CODE):
template_vars = {
"title": _("Your new mailbox information"),
"site": Site.objects.get_current(),
"webmail_url": settings.WEBMAIL_URL,
"mailbox_data": mailbox_data,
}
msg_html = render_to_string("mail/html/new_mailbox.html", template_vars)
msg_plain = render_to_string("mail/text/new_mailbox.txt", template_vars)
mail.send_mail(
template_vars["title"],
msg_plain,
context["title"],
render_to_string(f"mail/html/{template_name}.txt", context),
settings.EMAIL_FROM,
[recipient],
html_message=msg_html,
html_message=render_to_string(
f"mail/text/{template_name}.html", context
),
fail_silently=False,
)
logger.info(
"Information for mailbox %s sent to %s.",
mailbox_data["email"],
recipient,
)
except smtplib.SMTPException as exception:
logger.error(
"Mailbox confirmation email to %s was not sent: %s",
"Failed to send mailbox information to %s was not sent: %s",
recipient,
exception,
)
else:
logger.info(
"Information for mailbox %s sent to %s.",
mailbox_data["email"],
recipient,
)

def import_mailboxes(self, domain):
"""Import mailboxes from dimail - open xchange in our database.
Expand Down Expand Up @@ -409,9 +424,10 @@ def enable_pending_mailboxes(self, domain):
mailbox.save()

# send confirmation email
self.notify_mailbox_creation(
self.notify_mailbox_info(
recipient=mailbox.secondary_email,
mailbox_data=response.json(),
is_new_mailbox=True,
)

def check_domain(self, domain):
Expand Down Expand Up @@ -563,3 +579,35 @@ def fetch_domain_expected_config(self, domain):
exc_info=False,
)
return []

def reset_password(self, mailbox):
"""Send a request to reset mailbox password."""
try:
response = session.post(
f"{self.API_URL}/domains/{mailbox.domain.name}/mailboxes/{mailbox.local_part}/reset_password/",
headers={"Authorization": f"Basic {self.API_CREDENTIALS}"},
verify=True,
timeout=self.API_TIMEOUT,
)
except requests.exceptions.ConnectionError as error:
logger.exception(
"Connection error while trying to reach %s.",
self.API_URL,
exc_info=error,
)
return []

if response.status_code == status.HTTP_200_OK:
# send new password to secondary email
if mailbox.secondary_email and mailbox.secondary_email != str(mailbox):
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

mailbox.secondary_email != str(mailbox):
Ah tiens je n'y avais pas pensé ! J'ai envie de dire tant pis :D

Copy link
Member Author

@mjeammet mjeammet Mar 28, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

C'est pour le cas précis où on a utilisé notre script d'import ! Comme dimail ne stocke pas d'info d'adresse secondaire (et que l'adresse secondaire ne peut pas être vide), on a rempli avec l'adresse "principale".

Après, il faudrait peut-être aussi ajouter un validateur à la création. A vérifier.

self.notify_mailbox_info(
recipient=mailbox.secondary_email,
mailbox_data=response.json(),
is_new_mailbox=False,
)
logger.info(
"[DIMAIL] Password reset on mailbox %s.",
mailbox,
)
return response
return self.raise_exception_for_unexpected_response(response)
43 changes: 43 additions & 0 deletions src/mail/mjml/reset_password.mjml
Original file line number Diff line number Diff line change
@@ -0,0 +1,43 @@
<mjml>
<mj-include path="./partial/header.mjml" />

<mj-body mj-class="bg--blue-100">
<mj-wrapper css-class="wrapper" padding="0 40px 40px 40px">
<mj-section>
<mj-column>
<mj-image align="center" src="{% base64_static 'images/messagerie.png' %}" width="60px" height="60px" alt="{% trans 'La Messagerie' %}" />
</mj-column>
</mj-section>
<mj-section mj-class="bg--white-100" padding="0px 20px">
<mj-column>
<!-- Message -->
<mj-text font-size="30px"><b>{% trans "Your password has been reset" %}</b></mj-text>
<!-- Main Message -->
<mj-text>{% trans "Your password has been reset." %}</mj-text>
<mj-text>{% trans "Please find below your new login information: " %}</mj-text>
</mj-column>
</mj-section>
<mj-section background-color="#f3f2fe" padding="0px 20px">
<mj-column>
<mj-text>{% trans "Email address: "%}<b>{{ mailbox_data.email }}</b></mj-text>
<mj-text>{% trans "Temporary password (to be modify on first login): "%}<b>{{ mailbox_data.password }}</b></mj-text>
</mj-column>
</mj-section>
<mj-section padding="0px 20px">
<mj-column>
<mj-button background-color="#000091" color="white" href="{{ webmail_url }}">
{% trans "Go to La Messagerie" %}
</mj-button>
<!-- Signature -->
<mj-text>
<p>{% trans "Sincerely," %}</p>
<p>{% trans "La Suite Team" %}</p>
</mj-text>
</mj-column>
</mj-section>
</mj-wrapper>
</mj-body>

<mj-include path="./partial/footer.mjml" />
</mjml>

Loading