diff --git a/CHANGELOG.md b/CHANGELOG.md index 0c1f3fe..438d282 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -8,6 +8,7 @@ and this project adheres to ## [Unreleased] +- ✨(aliases) fix deleting single aliases #1002 - 🔥(plugins) remove CommuneCreation plugin ## [1.21.0] - 2025-12-05 diff --git a/src/backend/mailbox_manager/api/client/serializers.py b/src/backend/mailbox_manager/api/client/serializers.py index d885d51..b50f37e 100644 --- a/src/backend/mailbox_manager/api/client/serializers.py +++ b/src/backend/mailbox_manager/api/client/serializers.py @@ -4,6 +4,7 @@ from logging import getLogger from django.contrib.auth.hashers import make_password from django.core import exceptions as django_exceptions +from django.shortcuts import get_object_or_404 from requests.exceptions import HTTPError from rest_framework import exceptions, serializers @@ -306,7 +307,9 @@ class MailDomainInvitationSerializer(serializers.ModelSerializer): class AliasSerializer(serializers.ModelSerializer): - """Serialize mailbox.""" + """Serialize aliases.""" + + domain = MailDomainSerializer(default="") class Meta: model = models.Alias @@ -314,8 +317,13 @@ class AliasSerializer(serializers.ModelSerializer): "id", "local_part", "destination", + "domain", ] - read_only_fields = ["id"] + read_only_fields = ["id", "domain"] + + def validate_domain(self, value): # pylint: disable=unused-argument + """Forcefully set domain field to url domain.""" + return get_object_or_404(models.MailDomain, slug=self.context["domain_slug"]) def create(self, validated_data): """ diff --git a/src/backend/mailbox_manager/api/client/viewsets.py b/src/backend/mailbox_manager/api/client/viewsets.py index 9b32b19..76ec520 100644 --- a/src/backend/mailbox_manager/api/client/viewsets.py +++ b/src/backend/mailbox_manager/api/client/viewsets.py @@ -1,6 +1,7 @@ """API endpoints""" from django.db.models import Q, Subquery +from django.http import Http404 from rest_framework import exceptions, filters, mixins, status, viewsets from rest_framework.decorators import action @@ -422,11 +423,11 @@ class AliasViewSet( - destination: str Return a newly created alias - DELETE /api//mail-domains//accesses// + DELETE /api//mail-domains//aliases// Delete targeted alias """ - lookup_field = "local_part" + lookup_field = "pk" permission_classes = [permissions.DomainResourcePermission] serializer_class = serializers.AliasSerializer queryset = ( @@ -459,38 +460,19 @@ class AliasViewSet( return queryset - def get_permissions(self): - """Add a specific permission for domain viewers to delete their aliases.""" - if self.action in ["destroy"]: - permission_classes = [ - permissions.DomainResourcePermission - | permissions.IsAliasDestinationPermission, - ] - else: - return super().get_permissions() - - return [permission() for permission in permission_classes] - - def perform_create(self, serializer): - """Create new mailbox.""" - domain_slug = self.kwargs.get("domain_slug", "") - if domain_slug: - serializer.validated_data["domain"] = models.MailDomain.objects.get( - slug=domain_slug - ) - super().perform_create(serializer) - def destroy(self, request, *args, **kwargs): - """Override destroy method to send a delete request to dimail - and return clear message if domain out of sync.""" + """ + Override destroy method to delete specific alias and send request to dimail. + """ instance = self.get_object() self.perform_destroy(instance) client = DimailAPIClient() dimail_response = client.delete_alias(instance) + if dimail_response.status_code == status.HTTP_404_NOT_FOUND: return Response( - "Alias already deleted. Domain out of sync, please contact our support.", + "Domain out of sync with mailbox provider, please contact our support.", status=status.HTTP_200_OK, ) diff --git a/src/backend/mailbox_manager/api/permissions.py b/src/backend/mailbox_manager/api/permissions.py index a2f8020..9a730a1 100644 --- a/src/backend/mailbox_manager/api/permissions.py +++ b/src/backend/mailbox_manager/api/permissions.py @@ -55,17 +55,3 @@ class IsMailboxOwnerPermission(permissions.BasePermission): def has_object_permission(self, request, view, obj): """If the user is trying to update their own mailbox.""" return obj.get_email() == request.user.email - - -class IsAliasDestinationPermission(IsAuthenticated): - """Can delete an alias if the alias points to their own email address.""" - - def has_permission(self, request, view): - """This permission is specifically about updates""" - domain = models.MailDomain.objects.get(slug=view.kwargs.get("domain_slug", "")) - abilities = domain.get_abilities(request.user) - return abilities["get"] - - def has_object_permission(self, request, view, obj): - """If the user is trying to update their own mailbox.""" - return obj.destination == request.user.email diff --git a/src/backend/mailbox_manager/migrations/0029_alter_alias_unique_together_alias_no_duplicate.py b/src/backend/mailbox_manager/migrations/0029_alter_alias_unique_together_alias_no_duplicate.py new file mode 100644 index 0000000..f396b07 --- /dev/null +++ b/src/backend/mailbox_manager/migrations/0029_alter_alias_unique_together_alias_no_duplicate.py @@ -0,0 +1,21 @@ +# Generated by Django 5.2.9 on 2025-12-17 17:01 + +from django.db import migrations, models + + +class Migration(migrations.Migration): + + dependencies = [ + ('mailbox_manager', '0028_alias'), + ] + + operations = [ + migrations.AlterUniqueTogether( + name='alias', + unique_together=set(), + ), + migrations.AddConstraint( + model_name='alias', + constraint=models.UniqueConstraint(fields=('domain', 'local_part', 'destination'), name='no_duplicate'), + ), + ] diff --git a/src/backend/mailbox_manager/models.py b/src/backend/mailbox_manager/models.py index d033549..29bfcde 100644 --- a/src/backend/mailbox_manager/models.py +++ b/src/backend/mailbox_manager/models.py @@ -89,6 +89,8 @@ class MailDomain(BaseModel): def __str__(self): return self.name + objects = models.Manager() + def save(self, *args, **kwargs): """Override save function to compute the slug.""" self.slug = self.get_slug() @@ -471,8 +473,12 @@ class Alias(BaseModel): db_table = "people_aliases" verbose_name = _("Alias") verbose_name_plural = _("Aliases") - unique_together = ("local_part", "destination") ordering = ["-created_at"] + constraints = [ + models.UniqueConstraint( + fields=["domain", "local_part", "destination"], name="no_duplicate" + ) + ] def __str__(self): return f"{self.local_part} to {self.destination}" 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 55cf9eb..b085e48 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 @@ -60,8 +60,9 @@ def test_api_aliases_create__viewer_forbidden(): assert not models.Alias.objects.exists() +@responses.activate def test_api_aliases_create__duplicate_forbidden(): - """Cannot create alias if same local part + destination.""" + """Cannot create alias if existing alias same domain + local part + destination.""" access = factories.MailDomainAccessFactory( role="owner", domain=factories.MailDomainEnabledFactory() ) diff --git a/src/backend/mailbox_manager/tests/api/aliases/test_api_aliases_delete.py b/src/backend/mailbox_manager/tests/api/aliases/test_api_aliases_delete.py index f82b714..b712cc8 100644 --- a/src/backend/mailbox_manager/tests/api/aliases/test_api_aliases_delete.py +++ b/src/backend/mailbox_manager/tests/api/aliases/test_api_aliases_delete.py @@ -1,6 +1,6 @@ """ Tests for aliases API endpoint in People's app mailbox_manager. -Focus on "list" action. +Focus on "delete" action. """ # pylint: disable=W0613 @@ -20,11 +20,12 @@ pytestmark = pytest.mark.django_db def test_api_aliases_delete__anonymous(): """Anonymous user should not be able to delete aliases.""" - alias = factories.AliasFactory() + alias_ = factories.AliasFactory() response = APIClient().delete( - f"/api/v1.0/mail-domains/{alias.domain.slug}/aliases/{alias.local_part}/", + f"/api/v1.0/mail-domains/{alias_.domain.slug}/aliases/{alias_.pk}/" ) + assert response.status_code == status.HTTP_401_UNAUTHORIZED assert models.Alias.objects.count() == 1 @@ -35,12 +36,12 @@ def test_api_aliases_delete__no_access_forbidden_not_found(): mail domain to which they are not related. """ authenticated_user = core_factories.UserFactory() - alias = factories.AliasFactory() + alias_ = factories.AliasFactory() client = APIClient() client.force_login(authenticated_user) response = client.delete( - f"/api/v1.0/mail-domains/{alias.domain.slug}/aliases/{alias.local_part}/", + f"/api/v1.0/mail-domains/{alias_.domain.slug}/aliases/{alias_.pk}/" ) assert response.status_code == status.HTTP_404_NOT_FOUND @@ -56,59 +57,40 @@ def test_api_aliases_delete__viewer_forbidden(): mail_domain = factories.MailDomainFactory( users=[(authenticated_user, enums.MailDomainRoleChoices.VIEWER)] ) - alias = factories.AliasFactory(domain=mail_domain) + alias_ = factories.AliasFactory(domain=mail_domain) client = APIClient() client.force_login(authenticated_user) response = client.delete( - f"/api/v1.0/mail-domains/{mail_domain.slug}/aliases/{alias.local_part}/", + f"/api/v1.0/mail-domains/{mail_domain.slug}/aliases/{alias_.pk}/" ) assert response.status_code == status.HTTP_403_FORBIDDEN assert models.Alias.objects.count() == 1 -@responses.activate -def test_api_aliases_delete__viewer_can_delete_self_alias(dimail_token_ok): - """ - Authenticated users should be allowed to delete aliases when linking - to their own mailbox. - """ - authenticated_user = core_factories.UserFactory() - mail_domain = factories.MailDomainFactory( - users=[(authenticated_user, enums.MailDomainRoleChoices.VIEWER)] - ) - alias = factories.AliasFactory( - domain=mail_domain, destination=authenticated_user.email - ) - - # Mock dimail response - responses.delete( - re.compile(r".*/aliases/"), - status=status.HTTP_204_NO_CONTENT, - content_type="application/json", - ) - - client = APIClient() - client.force_login(authenticated_user) - response = client.delete( - f"/api/v1.0/mail-domains/{mail_domain.slug}/aliases/{alias.local_part}/", - ) - - assert response.status_code == status.HTTP_204_NO_CONTENT - assert not models.Alias.objects.exists() - - @responses.activate def test_api_aliases_delete__administrators_allowed(dimail_token_ok): """ - Administrators of a mail domain should be allowed to delete accesses excepted owner accesses. + Administrators of a mail domain should be allowed to delete aliases. """ authenticated_user = core_factories.UserFactory() mail_domain = factories.MailDomainFactory( users=[(authenticated_user, enums.MailDomainRoleChoices.ADMIN)] ) - alias = factories.AliasFactory(domain=mail_domain) + alias_ = factories.AliasFactory(domain=mail_domain) + factories.AliasFactory.create_batch( + 2, domain=mail_domain, local_part=alias_.local_part + ) + + # additional aliases that shouldn't be affected + factories.AliasFactory.create_batch( + 2, domain=mail_domain, destination=alias_.destination + ) + factories.AliasFactory( + local_part=alias_.local_part, + destination=alias_.destination, + ) # Mock dimail response responses.delete( @@ -120,11 +102,10 @@ def test_api_aliases_delete__administrators_allowed(dimail_token_ok): client = APIClient() client.force_login(authenticated_user) response = client.delete( - f"/api/v1.0/mail-domains/{mail_domain.slug}/aliases/{alias.local_part}/", + f"/api/v1.0/mail-domains/{mail_domain.slug}/aliases/{alias_.pk}/" ) - assert response.status_code == status.HTTP_204_NO_CONTENT - assert not models.Alias.objects.exists() + assert models.Alias.objects.count() == 5 @responses.activate @@ -136,8 +117,7 @@ def test_api_aliases_delete__404_out_of_sync(dimail_token_ok): mail_domain = factories.MailDomainFactory( users=[(authenticated_user, enums.MailDomainRoleChoices.ADMIN)] ) - alias = factories.AliasFactory(domain=mail_domain) - + alias_ = factories.AliasFactory(domain=mail_domain) # Mock dimail response responses.delete( re.compile(r".*/aliases/"), @@ -149,11 +129,11 @@ def test_api_aliases_delete__404_out_of_sync(dimail_token_ok): client = APIClient() client.force_login(authenticated_user) response = client.delete( - f"/api/v1.0/mail-domains/{mail_domain.slug}/aliases/{alias.local_part}/", + f"/api/v1.0/mail-domains/{mail_domain.slug}/aliases/{alias_.pk}/" ) assert response.status_code == status.HTTP_200_OK assert ( response.json() - == "Alias already deleted. Domain out of sync, please contact our support." + == "Domain out of sync with mailbox provider, please contact our support." ) assert not models.Alias.objects.exists()