From 3a2b5a642821ede9bffe9c6b0382401aa40b7cab Mon Sep 17 00:00:00 2001 From: Marie PUPO JEAMMET Date: Tue, 18 Nov 2025 19:24:17 +0100 Subject: [PATCH] =?UTF-8?q?=F0=9F=9B=82(permissions)=20return=20404=20to?= =?UTF-8?q?=20users=20with=20no=20domain=20access?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit users having no domain access should be forbidden to know the domain is managed in people. To this effect, 403_FORBIDDEN responses have been replaced by 404_NOT_FOUND. --- CHANGELOG.md | 1 + .../mailbox_manager/api/client/serializers.py | 25 ++--------- .../mailbox_manager/api/client/viewsets.py | 26 ++++++----- .../mailbox_manager/api/permissions.py | 43 ++++++++++++------ .../api/aliases/test_api_aliases_create.py | 6 +-- .../api/aliases/test_api_aliases_delete.py | 4 +- .../api/aliases/test_api_aliases_list.py | 4 +- .../test_api_domain_invitations_create.py | 8 ++-- .../test_api_domain_invitations_retrieve.py | 6 +-- .../test_api_mail_domains_create.py | 9 ++-- .../test_api_mail_domains_fetch.py | 3 +- .../test_api_mail_domains_retrieve.py | 2 +- .../test_api_mail_domain_accesses_create.py | 10 ++--- .../test_api_mail_domain_accesses_delete.py | 14 +++--- .../test_api_mail_domain_accesses_list.py | 18 ++++---- .../test_api_mail_domain_accesses_retrieve.py | 6 +-- .../test_api_mail_domain_accesses_update.py | 4 +- ...il_domains_accesses_get_available_users.py | 6 +-- .../mailboxes/test_api_mailboxes_create.py | 30 +++---------- .../mailboxes/test_api_mailboxes_disable.py | 4 +- .../mailboxes/test_api_mailboxes_enable.py | 7 ++- .../api/mailboxes/test_api_mailboxes_list.py | 10 ++--- .../api/mailboxes/test_api_mailboxes_patch.py | 45 +++++++++---------- .../api/mailboxes/test_api_mailboxes_put.py | 6 +-- .../test_api_mailboxes_reset_password.py | 6 +-- .../mailboxes/test_api_mailboxes_retrieve.py | 31 ++++--------- 26 files changed, 146 insertions(+), 188 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 6f3a34b..f7a652d 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -8,6 +8,7 @@ and this project adheres to ## [Unreleased] +- 🛂(permissions) return 404 to users with no access to domain #985 - ✨(aliases) can create, list and delete aliases #974 ## [1.20.0] - 2025-10-22 diff --git a/src/backend/mailbox_manager/api/client/serializers.py b/src/backend/mailbox_manager/api/client/serializers.py index ce393a4..d885d51 100644 --- a/src/backend/mailbox_manager/api/client/serializers.py +++ b/src/backend/mailbox_manager/api/client/serializers.py @@ -238,23 +238,10 @@ class MailDomainAccessSerializer(serializers.ModelSerializer): "You must set a domain slug in kwargs to create a new domain access." ) from exc - try: - access = authenticated_user.mail_domain_accesses.get( - domain__slug=domain_slug - ) - except models.MailDomainAccess.DoesNotExist as exc: - raise exceptions.PermissionDenied( - "You are not allowed to manage accesses for this domain." - ) from exc - - # Authenticated user must be owner or admin of current domain to set new roles - if access.role not in [ - enums.MailDomainRoleChoices.OWNER, - enums.MailDomainRoleChoices.ADMIN, - ]: - raise exceptions.PermissionDenied( - "You are not allowed to manage accesses for this domain." - ) + # we're sure that an access exists. otherwise it would have failed at permissions. + access = authenticated_user.mail_domain_accesses.get( + domain__slug=domain_slug + ) # only an owner can set an owner role to another user if ( @@ -312,10 +299,6 @@ class MailDomainInvitationSerializer(serializers.ModelSerializer): ) from exc domain = models.MailDomain.objects.get(slug=domain_slug) - if not domain.get_abilities(user)["manage_accesses"]: - raise exceptions.PermissionDenied( - "You are not allowed to manage invitations for this domain." - ) attrs["domain"] = domain attrs["issuer"] = user diff --git a/src/backend/mailbox_manager/api/client/viewsets.py b/src/backend/mailbox_manager/api/client/viewsets.py index 545bd94..9b32b19 100644 --- a/src/backend/mailbox_manager/api/client/viewsets.py +++ b/src/backend/mailbox_manager/api/client/viewsets.py @@ -40,7 +40,10 @@ class MailDomainViewSet( Fetch domain status and expected config from dimail. """ - permission_classes = [permissions.DomainResourcePermission] + permission_classes = [ + permissions.DomainPermission, + permissions.DomainResourcePermission, + ] serializer_class = serializers.MailDomainSerializer filter_backends = [filters.OrderingFilter] ordering_fields = ["created_at", "name"] @@ -262,7 +265,7 @@ class MailBoxViewSet( Send a request to partially update mailbox. Cannot modify domain or local_part. """ - permission_classes = [permissions.DomainPermission] + permission_classes = [permissions.DomainResourcePermission] serializer_class = serializers.MailboxSerializer filter_backends = [filters.OrderingFilter] ordering = ["-created_at"] @@ -283,9 +286,12 @@ class MailBoxViewSet( def get_permissions(self): """Add a specific permission for domain viewers to update their own mailbox.""" - if self.action in ["update", "partial_update"]: + if self.action in ["list", "retrieve"]: + permission_classes = [permissions.DomainPermission] + elif self.action in ["update", "partial_update"]: permission_classes = [ - permissions.DomainPermission | permissions.IsMailboxOwnerPermission + permissions.DomainResourcePermission + | permissions.IsMailboxOwnerPermission ] else: return super().get_permissions() @@ -421,7 +427,7 @@ class AliasViewSet( """ lookup_field = "local_part" - permission_classes = [permissions.DomainPermission] + permission_classes = [permissions.DomainResourcePermission] serializer_class = serializers.AliasSerializer queryset = ( models.Alias.objects.all().select_related("domain").order_by("-created_at") @@ -445,14 +451,10 @@ class AliasViewSet( ).values("role") queryset = ( - # The logged-in user should be part of a domain to see its accesses - queryset.filter( - domain__accesses__user=self.request.user, - ) + queryset # Abilities are computed based on logged-in user's role and # the user role on each domain access - .annotate(user_role=Subquery(user_role_query)) - .distinct() + .annotate(user_role=Subquery(user_role_query)).distinct() ) return queryset @@ -462,7 +464,7 @@ class AliasViewSet( if self.action in ["destroy"]: permission_classes = [ permissions.DomainResourcePermission - | permissions.IsAliasDestinationPermission + | permissions.IsAliasDestinationPermission, ] else: return super().get_permissions() diff --git a/src/backend/mailbox_manager/api/permissions.py b/src/backend/mailbox_manager/api/permissions.py index f1d5f88..a2f8020 100644 --- a/src/backend/mailbox_manager/api/permissions.py +++ b/src/backend/mailbox_manager/api/permissions.py @@ -1,13 +1,40 @@ """Permission handlers for the People mailbox manager app.""" +from django.shortcuts import get_object_or_404 + from rest_framework import permissions -from core.api import permissions as core_permissions +from core.api.permissions import IsAuthenticated from mailbox_manager import models -class DomainResourcePermission(core_permissions.IsAuthenticated): +class DomainPermission(IsAuthenticated): + """Permission class to manage mailboxes and aliases for a mail domain""" + + def has_permission(self, request, view): + """Check permission based on domain.""" + + if not super().has_permission(request, view): + return False + + if not view.kwargs.get("domain_slug"): + return True + + domain = get_object_or_404( + models.MailDomain, + slug=view.kwargs.get("domain_slug", ""), + accesses__user=request.user, + ) + # domain = models.MailDomain.objects.get(slug=view.kwargs.get("domain_slug", "")) + abilities = domain.get_abilities(request.user) + if request.method.lower() == "delete": + return abilities.get("manage_accesses", False) + + return abilities.get(request.method.lower(), False) + + +class DomainResourcePermission(DomainPermission): """Permission class for access objects.""" def has_object_permission(self, request, view, obj): @@ -16,16 +43,6 @@ class DomainResourcePermission(core_permissions.IsAuthenticated): return abilities.get(request.method.lower(), False) -class DomainPermission(DomainResourcePermission): - """Permission class to manage mailboxes and aliases for a mail domain""" - - def has_permission(self, request, view): - """Check permission based on domain.""" - domain = models.MailDomain.objects.get(slug=view.kwargs.get("domain_slug", "")) - abilities = domain.get_abilities(request.user) - return abilities.get(request.method.lower(), False) - - class IsMailboxOwnerPermission(permissions.BasePermission): """Authorize update for domain viewers on their own mailbox.""" @@ -40,7 +57,7 @@ class IsMailboxOwnerPermission(permissions.BasePermission): return obj.get_email() == request.user.email -class IsAliasDestinationPermission(core_permissions.IsAuthenticated): +class IsAliasDestinationPermission(IsAuthenticated): """Can delete an alias if the alias points to their own email address.""" def has_permission(self, request, view): 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 2a6795a..55cf9eb 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 @@ -31,8 +31,8 @@ def test_api_aliases_create__anonymous(): assert not models.Alias.objects.exists() -def test_api_aliases_create__no_access_forbidden(): - """User authenticated but not having domain permission should not create aliases.""" +def test_api_aliases_create__no_access_forbidden_not_found(): + """Unrelated user not having domain permission should not create aliases.""" domain = factories.MailDomainEnabledFactory() client = APIClient() @@ -41,7 +41,7 @@ def test_api_aliases_create__no_access_forbidden(): f"/api/v1.0/mail-domains/{domain.slug}/aliases/", {"local_part": "intrusive", "destination": "intrusive@mail.com"}, ) - assert response.status_code == status.HTTP_403_FORBIDDEN + assert response.status_code == status.HTTP_404_NOT_FOUND assert not models.Alias.objects.exists() 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 14bade2..f82b714 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 @@ -29,7 +29,7 @@ def test_api_aliases_delete__anonymous(): assert models.Alias.objects.count() == 1 -def test_api_aliases_delete__no_access_forbidden(): +def test_api_aliases_delete__no_access_forbidden_not_found(): """ Authenticated users should not be allowed to delete an alias in a mail domain to which they are not related. @@ -43,7 +43,7 @@ def test_api_aliases_delete__no_access_forbidden(): f"/api/v1.0/mail-domains/{alias.domain.slug}/aliases/{alias.local_part}/", ) - assert response.status_code == status.HTTP_403_FORBIDDEN + assert response.status_code == status.HTTP_404_NOT_FOUND assert models.Alias.objects.count() == 1 diff --git a/src/backend/mailbox_manager/tests/api/aliases/test_api_aliases_list.py b/src/backend/mailbox_manager/tests/api/aliases/test_api_aliases_list.py index cf50b13..e495dae 100644 --- a/src/backend/mailbox_manager/tests/api/aliases/test_api_aliases_list.py +++ b/src/backend/mailbox_manager/tests/api/aliases/test_api_aliases_list.py @@ -25,7 +25,7 @@ def test_api_aliases_list__anonymous(): assert response.status_code == status.HTTP_401_UNAUTHORIZED -def test_api_aliases_list__no_access_forbidden(): +def test_api_aliases_list__no_access_forbidden_not_found(): """User authenticated but not having domain permission should not list aliases.""" factories.MailDomainAccessFactory() # access to another domain domain = factories.MailDomainEnabledFactory() @@ -36,7 +36,7 @@ def test_api_aliases_list__no_access_forbidden(): response = client.get( f"/api/v1.0/mail-domains/{domain.slug}/aliases/", ) - assert response.status_code == status.HTTP_403_FORBIDDEN + assert response.status_code == status.HTTP_404_NOT_FOUND @pytest.mark.parametrize( diff --git a/src/backend/mailbox_manager/tests/api/invitations/test_api_domain_invitations_create.py b/src/backend/mailbox_manager/tests/api/invitations/test_api_domain_invitations_create.py index 9b25d24..6503bb8 100644 --- a/src/backend/mailbox_manager/tests/api/invitations/test_api_domain_invitations_create.py +++ b/src/backend/mailbox_manager/tests/api/invitations/test_api_domain_invitations_create.py @@ -35,7 +35,7 @@ def test_api_domain_invitations__create__anonymous(): } -def test_api_domain_invitations__create__authenticated_outsider(): +def test_api_domain_invitations__create__no_access_forbidden_not_found(): """Users should not be permitted to send domain management invitations for a domain they don't manage.""" user = core_factories.UserFactory() @@ -52,7 +52,7 @@ def test_api_domain_invitations__create__authenticated_outsider(): invitation_values, format="json", ) - assert response.status_code == status.HTTP_403_FORBIDDEN + assert response.status_code == status.HTTP_404_NOT_FOUND @pytest.mark.parametrize( @@ -86,7 +86,7 @@ def test_api_domain_invitations__admin_should_create_invites(role): assert email.subject == "[La Suite] Vous avez été invité(e) à rejoindre la Régie" -def test_api_domain_invitations__viewers_should_not_invite_to_manage_domains(): +def test_api_domain_invitations__no_access_forbidden_not_found(): """ Domain viewers should not be able to invite new domain managers. """ @@ -110,7 +110,7 @@ def test_api_domain_invitations__viewers_should_not_invite_to_manage_domains(): ) assert response.status_code == status.HTTP_403_FORBIDDEN assert response.json() == { - "detail": "You are not allowed to manage invitations for this domain." + "detail": "You do not have permission to perform this action." } diff --git a/src/backend/mailbox_manager/tests/api/invitations/test_api_domain_invitations_retrieve.py b/src/backend/mailbox_manager/tests/api/invitations/test_api_domain_invitations_retrieve.py index 93bf457..4a82731 100644 --- a/src/backend/mailbox_manager/tests/api/invitations/test_api_domain_invitations_retrieve.py +++ b/src/backend/mailbox_manager/tests/api/invitations/test_api_domain_invitations_retrieve.py @@ -26,7 +26,7 @@ def test_api_domain_invitations__anonymous_user_should_not_retrieve_invitations( assert response.status_code == status.HTTP_401_UNAUTHORIZED -def test_api_domain_invitations__unrelated_user_should_not_retrieve_invitations(): +def test_api_domain_invitations__no_access_forbidden_not_found(): """ Authenticated unrelated users should not be able to retrieve invitations. """ @@ -40,8 +40,8 @@ def test_api_domain_invitations__unrelated_user_should_not_retrieve_invitations( f"/api/v1.0/mail-domains/{invitation.domain.slug}/invitations/", ) - assert response.status_code == status.HTTP_200_OK - assert response.json()["count"] == 0 + assert response.status_code == status.HTTP_404_NOT_FOUND + assert response.json() == {"detail": "No MailDomain matches the given query."} def test_api_domain_invitations__domain_managers_should_list_invitations(): diff --git a/src/backend/mailbox_manager/tests/api/mail_domain/test_api_mail_domains_create.py b/src/backend/mailbox_manager/tests/api/mail_domain/test_api_mail_domains_create.py index 33e9f01..88958a6 100644 --- a/src/backend/mailbox_manager/tests/api/mail_domain/test_api_mail_domains_create.py +++ b/src/backend/mailbox_manager/tests/api/mail_domain/test_api_mail_domains_create.py @@ -41,17 +41,14 @@ def test_api_mail_domains__create_name_unique(): """ Creating domain should raise an error if already existing name. """ - factories.MailDomainFactory(name="existing_domain.com") - user = core_factories.UserFactory() + existing_domain = factories.MailDomainFactory() client = APIClient() - client.force_login(user) + client.force_login(core_factories.UserFactory()) response = client.post( "/api/v1.0/mail-domains/", - { - "name": "existing_domain.com", - }, + {"name": existing_domain.name}, ) assert response.status_code == status.HTTP_400_BAD_REQUEST diff --git a/src/backend/mailbox_manager/tests/api/mail_domain/test_api_mail_domains_fetch.py b/src/backend/mailbox_manager/tests/api/mail_domain/test_api_mail_domains_fetch.py index cc2f9c4..e6038e5 100644 --- a/src/backend/mailbox_manager/tests/api/mail_domain/test_api_mail_domains_fetch.py +++ b/src/backend/mailbox_manager/tests/api/mail_domain/test_api_mail_domains_fetch.py @@ -2,7 +2,6 @@ Tests for MailDomains API endpoint in People's mailbox manager app. Focus on "fetch" action. """ -import json import re import pytest @@ -82,7 +81,7 @@ def test_api_mail_domains__fetch_from_dimail__viewer(): ], ) @responses.activate -def test_api_mail_domains__fetch_from_dimail(role): +def test_api_mail_domains__fetch_from_dimail_admin_successful(role): """ Authenticated users should be allowed to fetch a domain from dimail if they are an owner or admin. diff --git a/src/backend/mailbox_manager/tests/api/mail_domain/test_api_mail_domains_retrieve.py b/src/backend/mailbox_manager/tests/api/mail_domain/test_api_mail_domains_retrieve.py index 7e550c3..12730ed 100644 --- a/src/backend/mailbox_manager/tests/api/mail_domain/test_api_mail_domains_retrieve.py +++ b/src/backend/mailbox_manager/tests/api/mail_domain/test_api_mail_domains_retrieve.py @@ -69,7 +69,7 @@ def test_api_mail_domains__retrieve_authenticated_unrelated(): @responses.activate -def test_api_mail_domains__retrieve_authenticated_related(): +def test_api_mail_domains__retrieve_authenticated_related_successful(): """ Authenticated users should be allowed to retrieve a domain to which they have access. diff --git a/src/backend/mailbox_manager/tests/api/mail_domain_accesses/test_api_mail_domain_accesses_create.py b/src/backend/mailbox_manager/tests/api/mail_domain_accesses/test_api_mail_domain_accesses_create.py index 3314b98..979ba53 100644 --- a/src/backend/mailbox_manager/tests/api/mail_domain_accesses/test_api_mail_domain_accesses_create.py +++ b/src/backend/mailbox_manager/tests/api/mail_domain_accesses/test_api_mail_domain_accesses_create.py @@ -35,7 +35,7 @@ def test_api_mail_domain__accesses_create_anonymous(): assert models.MailDomainAccess.objects.exists() is False -def test_api_mail_domain__accesses_create_authenticated_unrelated(): +def test_api_mail_domain__accesses_create_no_access_forbidden_not_found(): """ Authenticated users should not be allowed to create domain accesses for a domain to which they are not related. @@ -56,14 +56,14 @@ def test_api_mail_domain__accesses_create_authenticated_unrelated(): format="json", ) - assert response.status_code == status.HTTP_403_FORBIDDEN + assert response.status_code == status.HTTP_404_NOT_FOUND assert response.json() == { - "detail": "You are not allowed to manage accesses for this domain." + "detail": "No MailDomain matches the given query.", } assert not models.MailDomainAccess.objects.filter(user=other_user).exists() -def test_api_mail_domain__accesses_create_authenticated_viewer(): +def test_api_mail_domain__accesses_create_viewer_forbidden(): """Viewer of a mail domain should not be allowed to create mail domain accesses.""" authenticated_user = core_factories.UserFactory() mail_domain = factories.MailDomainFactory( @@ -85,7 +85,7 @@ def test_api_mail_domain__accesses_create_authenticated_viewer(): assert response.status_code == status.HTTP_403_FORBIDDEN assert response.json() == { - "detail": "You are not allowed to manage accesses for this domain." + "detail": "You do not have permission to perform this action.", } assert not models.MailDomainAccess.objects.filter(user=other_user).exists() diff --git a/src/backend/mailbox_manager/tests/api/mail_domain_accesses/test_api_mail_domain_accesses_delete.py b/src/backend/mailbox_manager/tests/api/mail_domain_accesses/test_api_mail_domain_accesses_delete.py index 1c7dee3..1e6def2 100644 --- a/src/backend/mailbox_manager/tests/api/mail_domain_accesses/test_api_mail_domain_accesses_delete.py +++ b/src/backend/mailbox_manager/tests/api/mail_domain_accesses/test_api_mail_domain_accesses_delete.py @@ -13,7 +13,7 @@ from mailbox_manager import enums, factories, models pytestmark = pytest.mark.django_db -def test_api_mail_domain__accesses_delete_anonymous(): +def test_api_domain_accesses_delete__anonymous_forbidden(): """Anonymous users should not be allowed to destroy a mail domain access.""" access = factories.MailDomainAccessFactory() @@ -25,7 +25,7 @@ def test_api_mail_domain__accesses_delete_anonymous(): assert models.MailDomainAccess.objects.count() == 1 -def test_api_mail_domain__accesses_delete_authenticated(): +def test_api_domain_accesses_delete__unrelated_notfound(): """ Authenticated users should not be allowed to delete a mail domain access for a mail domain to which they are not related. @@ -39,11 +39,11 @@ def test_api_mail_domain__accesses_delete_authenticated(): f"/api/v1.0/mail-domains/{access.domain.slug}/accesses/{access.id!s}/", ) - assert response.status_code == status.HTTP_403_FORBIDDEN + assert response.status_code == status.HTTP_404_NOT_FOUND assert models.MailDomainAccess.objects.count() == 1 -def test_api_mail_domain__accesses_delete_viewer(): +def test_api_domain_accesses_delete__viewer_forbidden(): """ Authenticated users should not be allowed to delete a mail domain access for a mail domain in which they are a simple viewer. @@ -65,7 +65,7 @@ def test_api_mail_domain__accesses_delete_viewer(): assert models.MailDomainAccess.objects.filter(user=access.user).exists() -def test_api_mail_domain__accesses_delete_administrators(): +def test_api_domain_accesses_delete__administrators_successful(): """ Administrators of a mail domain should be allowed to delete accesses excepted owner accesses. """ @@ -89,7 +89,7 @@ def test_api_mail_domain__accesses_delete_administrators(): assert models.MailDomainAccess.objects.count() == 1 -def test_api_mail_domain__accesses_delete_owners(): +def test_api_domain_accesses_delete__owners_successful(): """ An owner should be able to delete the mail domain access of another user including a owner access. @@ -114,7 +114,7 @@ def test_api_mail_domain__accesses_delete_owners(): assert models.MailDomainAccess.objects.count() == 1 -def test_api_mail_domain__accesses_delete_owners_last_owner(): +def test_api_domain_accesses_delete__last_owner_forbidden(): """ It should not be possible to delete the last owner access from a mail domain """ diff --git a/src/backend/mailbox_manager/tests/api/mail_domain_accesses/test_api_mail_domain_accesses_list.py b/src/backend/mailbox_manager/tests/api/mail_domain_accesses/test_api_mail_domain_accesses_list.py index 6e398f5..fa4cab5 100644 --- a/src/backend/mailbox_manager/tests/api/mail_domain_accesses/test_api_mail_domain_accesses_list.py +++ b/src/backend/mailbox_manager/tests/api/mail_domain_accesses/test_api_mail_domain_accesses_list.py @@ -27,8 +27,7 @@ def test_api_mail_domain__accesses_list_anonymous(): def test_api_mail_domain__accesses_list_authenticated_unrelated(): """ - Authenticated users should not be allowed to list mail_domain accesses for a mail_domain - to which they are not related. + User with no access should not be allowed to list domain accesses. """ user = core_factories.UserFactory() mail_domain = factories.MailDomainFactory() @@ -43,12 +42,9 @@ def test_api_mail_domain__accesses_list_authenticated_unrelated(): response = client.get( f"/api/v1.0/mail-domains/{mail_domain.slug}/accesses/", ) - assert response.status_code == 200 + assert response.status_code == status.HTTP_404_NOT_FOUND assert response.json() == { - "count": 0, - "next": None, - "previous": None, - "results": [], + "detail": "No MailDomain matches the given query.", } @@ -178,8 +174,12 @@ def test_api_mail_domain__accesses_list_authenticated_constant_numqueries( # related users : # - query retrieving logged-in user for user_role annotation # - count from pagination + + # - filter on slug and domain on permission (to return 404 instead of 403 when no access) + # - count ? + # - distinct from viewset - with django_assert_num_queries(3): + with django_assert_num_queries(5): client.get( f"/api/v1.0/mail-domains/{mail_domain.slug}/accesses/", ) @@ -189,7 +189,7 @@ def test_api_mail_domain__accesses_list_authenticated_constant_numqueries( factories.MailDomainAccessFactory(domain=mail_domain) # num queries should still be the same - with django_assert_num_queries(3): + with django_assert_num_queries(5): response = client.get( f"/api/v1.0/mail-domains/{mail_domain.slug}/accesses/", ) diff --git a/src/backend/mailbox_manager/tests/api/mail_domain_accesses/test_api_mail_domain_accesses_retrieve.py b/src/backend/mailbox_manager/tests/api/mail_domain_accesses/test_api_mail_domain_accesses_retrieve.py index 156dfd0..0ac0cf4 100644 --- a/src/backend/mailbox_manager/tests/api/mail_domain_accesses/test_api_mail_domain_accesses_retrieve.py +++ b/src/backend/mailbox_manager/tests/api/mail_domain_accesses/test_api_mail_domain_accesses_retrieve.py @@ -43,7 +43,7 @@ def test_api_mail_domain__accesses_retrieve_authenticated_unrelated(): f"/api/v1.0/mail-domains/{access.domain.slug}/accesses/{access.id!s}/", ) assert response.status_code == status.HTTP_404_NOT_FOUND - assert response.json() == {"detail": "No MailDomainAccess matches the given query."} + assert response.json() == {"detail": "No MailDomain matches the given query."} # Accesses related to another mail_domain should be excluded even if the user is related to it for other_access in [ @@ -55,9 +55,7 @@ def test_api_mail_domain__accesses_retrieve_authenticated_unrelated(): ) assert response.status_code == status.HTTP_404_NOT_FOUND - assert response.json() == { - "detail": "No MailDomainAccess matches the given query." - } + assert response.json() == {"detail": "No MailDomain matches the given query."} def test_api_mail_domain__accesses_retrieve_authenticated_related(): diff --git a/src/backend/mailbox_manager/tests/api/mail_domain_accesses/test_api_mail_domain_accesses_update.py b/src/backend/mailbox_manager/tests/api/mail_domain_accesses/test_api_mail_domain_accesses_update.py index a2d2d6e..696664e 100644 --- a/src/backend/mailbox_manager/tests/api/mail_domain_accesses/test_api_mail_domain_accesses_update.py +++ b/src/backend/mailbox_manager/tests/api/mail_domain_accesses/test_api_mail_domain_accesses_update.py @@ -27,7 +27,7 @@ def test_api_mail_domain__accesses_update_anonymous(): assert access.role == enums.MailDomainRoleChoices.VIEWER -def test_api_mail_domain__accesses_update_authenticated_unrelated(): +def test_api_mail_domain__accesses_update_no_access_forbidden_not_found(): """ An authenticated user should not be allowed to update a mail domain access for a mail_domain to which they are not related. @@ -42,7 +42,7 @@ def test_api_mail_domain__accesses_update_authenticated_unrelated(): {"role": enums.MailDomainRoleChoices.ADMIN}, format="json", ) - assert response.status_code == status.HTTP_403_FORBIDDEN + assert response.status_code == status.HTTP_404_NOT_FOUND access.refresh_from_db() assert access.role == enums.MailDomainRoleChoices.VIEWER diff --git a/src/backend/mailbox_manager/tests/api/mail_domain_accesses/test_api_mail_domains_accesses_get_available_users.py b/src/backend/mailbox_manager/tests/api/mail_domain_accesses/test_api_mail_domains_accesses_get_available_users.py index 1005b78..551afb4 100644 --- a/src/backend/mailbox_manager/tests/api/mail_domain_accesses/test_api_mail_domains_accesses_get_available_users.py +++ b/src/backend/mailbox_manager/tests/api/mail_domain_accesses/test_api_mail_domains_accesses_get_available_users.py @@ -30,9 +30,7 @@ def test_api_mail_domain__available_users_anonymous(): def test_api_mail_domain__available_users_forbidden(): - """Authenticated user without accesses on maildomain should not be able to see available - users. - """ + """Users with no access should not be able to list available users.""" authenticated_user = core_factories.UserFactory() client = APIClient() client.force_login(authenticated_user) @@ -40,7 +38,7 @@ def test_api_mail_domain__available_users_forbidden(): response = client.get(f"/api/v1.0/mail-domains/{maildomain.slug}/accesses/users/") - assert response.status_code == status.HTTP_403_FORBIDDEN + assert response.status_code == status.HTTP_404_NOT_FOUND @pytest.mark.parametrize( 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 7cb21e4..d9edc0f 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 @@ -38,20 +38,23 @@ def test_api_mailboxes__create_anonymous_forbidden(mailbox_data): assert not models.Mailbox.objects.exists() -def test_api_mailboxes__create_authenticated_failure(mailbox_data): +@responses.activate +def test_api_mailboxes__create_no_access_forbidden_not_found(mailbox_data): """Authenticated users should not be able to create mailbox without specific role on mail domain.""" mail_domain = factories.MailDomainEnabledFactory() client = APIClient() client.force_login(core_factories.UserFactory()) + # We add no simulated response in Responses + # because we expect no "outside" calls to be made response = client.post( f"/api/v1.0/mail-domains/{mail_domain.slug}/mailboxes/", mailbox_data, format="json", ) - assert response.status_code == status.HTTP_403_FORBIDDEN + assert response.status_code == status.HTTP_404_NOT_FOUND assert not models.Mailbox.objects.exists() @@ -467,28 +470,6 @@ def test_api_mailboxes__existing_alias_bad_request(mailbox_data): ### REACTING TO DIMAIL-API ### We mock dimail's responses to avoid testing dimail's container too -@responses.activate -def test_api_mailboxes__unrelated_user_provisioning_api_not_called(mailbox_data): - """ - Provisioning API should not be called if an user tries - to create a mailbox on a domain they have no access to. - """ - domain = factories.MailDomainEnabledFactory() - - client = APIClient() - client.force_login(core_factories.UserFactory()) # user with no access - # We add no simulated response in RequestsMock - # because we expected no "outside" calls to be made - response = client.post( - f"/api/v1.0/mail-domains/{domain.slug}/mailboxes/", - mailbox_data, - format="json", - ) - # No exception raised by RequestsMock means no call was sent - # our API blocked the request before sending it - assert response.status_code == status.HTTP_403_FORBIDDEN - - @responses.activate def test_api_mailboxes__domain_viewer_provisioning_api_not_called(mailbox_data): """ @@ -957,7 +938,6 @@ def test_api_mailboxes__send_correct_logger_infos( assert expected_messages.issubset(actual_messages) -@pytest.mark.usefixtures() @responses.activate @mock.patch.object(Logger, "info") def test_api_mailboxes__sends_new_mailbox_notification( diff --git a/src/backend/mailbox_manager/tests/api/mailboxes/test_api_mailboxes_disable.py b/src/backend/mailbox_manager/tests/api/mailboxes/test_api_mailboxes_disable.py index e81b800..ffdd0c1 100644 --- a/src/backend/mailbox_manager/tests/api/mailboxes/test_api_mailboxes_disable.py +++ b/src/backend/mailbox_manager/tests/api/mailboxes/test_api_mailboxes_disable.py @@ -26,7 +26,7 @@ def test_api_mailboxes__disable_anonymous_forbidden(): assert models.Mailbox.objects.get().status == enums.MailboxStatusChoices.ENABLED -def test_api_mailboxes__disable_authenticated_failure(): +def test_api_mailboxes__disable_no_access_forbidden_not_found(): """Authenticated users should not be able to disable mailbox without specific role on mail domain.""" user = core_factories.UserFactory() @@ -39,7 +39,7 @@ def test_api_mailboxes__disable_authenticated_failure(): f"/api/v1.0/mail-domains/{mailbox.domain.slug}/mailboxes/{mailbox.pk}/disable/", ) - assert response.status_code == status.HTTP_403_FORBIDDEN + assert response.status_code == status.HTTP_404_NOT_FOUND assert models.Mailbox.objects.get().status == enums.MailboxStatusChoices.ENABLED diff --git a/src/backend/mailbox_manager/tests/api/mailboxes/test_api_mailboxes_enable.py b/src/backend/mailbox_manager/tests/api/mailboxes/test_api_mailboxes_enable.py index 3c27c7e..6a51b35 100644 --- a/src/backend/mailbox_manager/tests/api/mailboxes/test_api_mailboxes_enable.py +++ b/src/backend/mailbox_manager/tests/api/mailboxes/test_api_mailboxes_enable.py @@ -26,9 +26,8 @@ def test_api_mailboxes__enable_anonymous_forbidden(): assert models.Mailbox.objects.get().status == enums.MailboxStatusChoices.DISABLED -def test_api_mailboxes__enable_authenticated_failure(): - """Authenticated users should not be able to enable mailbox - without specific role on mail domain.""" +def test_api_mailboxes__enable_unrelated_not_found(): + """Unrelated users should not be able to enable mailbox.""" user = core_factories.UserFactory() client = APIClient() @@ -39,7 +38,7 @@ def test_api_mailboxes__enable_authenticated_failure(): f"/api/v1.0/mail-domains/{mailbox.domain.slug}/mailboxes/{mailbox.pk}/enable/", ) - assert response.status_code == status.HTTP_403_FORBIDDEN + assert response.status_code == status.HTTP_404_NOT_FOUND assert models.Mailbox.objects.get().status == enums.MailboxStatusChoices.DISABLED diff --git a/src/backend/mailbox_manager/tests/api/mailboxes/test_api_mailboxes_list.py b/src/backend/mailbox_manager/tests/api/mailboxes/test_api_mailboxes_list.py index b17207b..22a7abd 100644 --- a/src/backend/mailbox_manager/tests/api/mailboxes/test_api_mailboxes_list.py +++ b/src/backend/mailbox_manager/tests/api/mailboxes/test_api_mailboxes_list.py @@ -25,8 +25,8 @@ def test_api_mailboxes__list_anonymous(): } -def test_api_mailboxes__list_authenticated(): - """Authenticated users should not be able to list mailboxes""" +def test_api_mailboxes__list_unrelated_not_found(): + """Unrelated users should not be able to list mailboxes""" user = core_factories.UserFactory() client = APIClient() @@ -36,10 +36,8 @@ def test_api_mailboxes__list_authenticated(): factories.MailboxFactory.create_batch(2, domain=mail_domain) response = client.get(f"/api/v1.0/mail-domains/{mail_domain.slug}/mailboxes/") - assert response.status_code == status.HTTP_403_FORBIDDEN - assert response.json() == { - "detail": "You do not have permission to perform this action." - } + assert response.status_code == status.HTTP_404_NOT_FOUND + assert response.json() == {"detail": "No MailDomain matches the given query."} @pytest.mark.parametrize( diff --git a/src/backend/mailbox_manager/tests/api/mailboxes/test_api_mailboxes_patch.py b/src/backend/mailbox_manager/tests/api/mailboxes/test_api_mailboxes_patch.py index 6280186..def769e 100644 --- a/src/backend/mailbox_manager/tests/api/mailboxes/test_api_mailboxes_patch.py +++ b/src/backend/mailbox_manager/tests/api/mailboxes/test_api_mailboxes_patch.py @@ -28,40 +28,22 @@ def test_api_mailboxes_patch__anonymous_forbidden(): assert mailbox.secondary_email == saved_secondary -def test_api_mailboxes_patch__unauthorized_forbidden(): +def test_api_mailboxes_patch__no_access_forbidden_not_found(): """Authenticated but unauthoriezd users should not be able to update mailboxes.""" - client = APIClient() - client.force_login(core_factories.UserFactory()) - mailbox = factories.MailboxFactory() saved_secondary = mailbox.secondary_email - response = client.patch( - f"/api/v1.0/mail-domains/{mailbox.domain.slug}/mailboxes/{mailbox.pk}/", - {"secondary_email": "updated@example.com"}, - format="json", - ) - # permission denied at domain level - assert response.status_code == status.HTTP_403_FORBIDDEN - mailbox.refresh_from_db() - assert mailbox.secondary_email == saved_secondary - - -def test_api_mailboxes_patch__unauthorized_no_mailbox(): - """Authenticated but unauthoriezd users should not be able to update mailboxes.""" client = APIClient() client.force_login(core_factories.UserFactory()) - - domain = factories.MailDomainEnabledFactory() response = client.patch( - f"/api/v1.0/mail-domains/{domain.slug}/mailboxes/nonexistent_mailbox_pk/", + f"/api/v1.0/mail-domains/{mailbox.domain.slug}/mailboxes/{mailbox.local_part}/", {"secondary_email": "updated@example.com"}, format="json", ) - # permission denied at domain level - # the existence of the mailbox is not checked - assert response.status_code == status.HTTP_403_FORBIDDEN + assert response.status_code == status.HTTP_404_NOT_FOUND + mailbox.refresh_from_db() + assert mailbox.secondary_email == saved_secondary def test_api_mailboxes_patch__viewer_forbidden(): @@ -92,6 +74,23 @@ def test_api_mailboxes_patch__viewer_forbidden(): assert mailbox.secondary_email == old_value +def test_api_mailboxes_patch__unauthorized_no_mailbox(): + """No mailbox returns a 404.""" + access = factories.MailDomainAccessFactory(role=enums.MailDomainRoleChoices.ADMIN) + + client = APIClient() + client.force_login(access.user) + response = client.patch( + f"/api/v1.0/mail-domains/{access.domain.slug}/mailboxes/nonexistent_mailbox_pk/", + {"secondary_email": "updated@example.com"}, + format="json", + ) + + # permission denied at domain level + # the existence of the mailbox is not checked + assert response.status_code == status.HTTP_404_NOT_FOUND + + # UPDATABLE FIELDS : SECONDARY EMAIL FIRST NAME AND LAST NAME @pytest.mark.parametrize( "role", diff --git a/src/backend/mailbox_manager/tests/api/mailboxes/test_api_mailboxes_put.py b/src/backend/mailbox_manager/tests/api/mailboxes/test_api_mailboxes_put.py index 74f94ec..90e0a85 100644 --- a/src/backend/mailbox_manager/tests/api/mailboxes/test_api_mailboxes_put.py +++ b/src/backend/mailbox_manager/tests/api/mailboxes/test_api_mailboxes_put.py @@ -35,7 +35,7 @@ def test_api_mailboxes_put__anonymous_forbidden(): assert mailbox.secondary_email == saved_secondary -def test_api_mailboxes_put__unauthorized_forbidden(): +def test_api_mailboxes_put__no_access_forbidden_not_found(): """Authenticated but unauthoriezd users should not be able to update mailboxes.""" client = APIClient() client.force_login(core_factories.UserFactory()) @@ -49,7 +49,7 @@ def test_api_mailboxes_put__unauthorized_forbidden(): ) # permission denied at domain level - assert response.status_code == status.HTTP_403_FORBIDDEN + assert response.status_code == status.HTTP_404_NOT_FOUND mailbox.refresh_from_db() assert mailbox.secondary_email == saved_secondary @@ -68,7 +68,7 @@ def test_api_mailboxes_put__unauthorized_no_mailbox(): # permission denied at domain level # the existence of the mailbox is not checked - assert response.status_code == status.HTTP_403_FORBIDDEN + assert response.status_code == status.HTTP_404_NOT_FOUND def test_api_mailboxes_put__viewer_forbidden(): diff --git a/src/backend/mailbox_manager/tests/api/mailboxes/test_api_mailboxes_reset_password.py b/src/backend/mailbox_manager/tests/api/mailboxes/test_api_mailboxes_reset_password.py index 55aa910..3882bc2 100644 --- a/src/backend/mailbox_manager/tests/api/mailboxes/test_api_mailboxes_reset_password.py +++ b/src/backend/mailbox_manager/tests/api/mailboxes/test_api_mailboxes_reset_password.py @@ -28,7 +28,7 @@ def test_api_mailboxes__reset_password_anonymous_unauthorized(): assert response.status_code == status.HTTP_401_UNAUTHORIZED -def test_api_mailboxes__reset_password_unrelated_forbidden(): +def test_api_mailboxes__reset_password_no_access_forbidden_not_found(): """Authenticated users not managing the domain should not be able to reset its mailboxes password.""" user = core_factories.UserFactory() @@ -41,9 +41,9 @@ def test_api_mailboxes__reset_password_unrelated_forbidden(): 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.status_code == status.HTTP_404_NOT_FOUND assert response.json() == { - "detail": "You do not have permission to perform this action." + "detail": "No MailDomain matches the given query.", } diff --git a/src/backend/mailbox_manager/tests/api/mailboxes/test_api_mailboxes_retrieve.py b/src/backend/mailbox_manager/tests/api/mailboxes/test_api_mailboxes_retrieve.py index cd17d0e..13e8882 100644 --- a/src/backend/mailbox_manager/tests/api/mailboxes/test_api_mailboxes_retrieve.py +++ b/src/backend/mailbox_manager/tests/api/mailboxes/test_api_mailboxes_retrieve.py @@ -22,9 +22,9 @@ def test_api_mailboxes__retrieve_anonymous_forbidden(): assert response.status_code == status.HTTP_401_UNAUTHORIZED -def test_api_mailboxes__retrieve_unauthorized_failure(): +def test_api_mailboxes__retrieve_no_access_forbidden_not_found(): """Authenticated but unauthorized users should not be able to - retrieve mailbox.""" + retrieve mailbox. Response should be indistinguisable from normal 404""" client = APIClient() client.force_login(core_factories.UserFactory()) @@ -33,15 +33,16 @@ def test_api_mailboxes__retrieve_unauthorized_failure(): f"/api/v1.0/mail-domains/{mailbox.domain.slug}/mailboxes/{mailbox.pk}/" ) - assert response.status_code == status.HTTP_403_FORBIDDEN - # 403 or 404 for confidentiality/security purposes ? + assert response.status_code == status.HTTP_404_NOT_FOUND - # response should be the same whether the mailbox exists or not, so that - # unauthorized users can't deduce mailbox existence or nonexistence + # even if they have a mailbox on this domain + mailbox = factories.MailboxFactory() + client.force_login(core_factories.UserFactory(email=str(mailbox))) response = client.get( - f"/api/v1.0/mail-domains/{mailbox.domain.slug}/mailboxes/thismailboxdoesntexist/" + f"/api/v1.0/mail-domains/{mailbox.domain.slug}/mailboxes/{mailbox.pk}/" ) - assert response.status_code == status.HTTP_403_FORBIDDEN + + assert response.status_code == status.HTTP_404_NOT_FOUND def test_api_mailboxes__retrieve_authorized_ok(): @@ -65,17 +66,3 @@ def test_api_mailboxes__retrieve_authorized_ok(): "secondary_email": mailbox.secondary_email, "status": mailbox.status, } - - -def test_api_mailboxes__owner_not_authorized(): - """Unauthorized mailbox owner should not be able to retrieve their mailbox.""" - mailbox = factories.MailboxFactory() - user = core_factories.UserFactory(email=str(mailbox)) - - client = APIClient() - client.force_login(user) - response = client.get( - f"/api/v1.0/mail-domains/{mailbox.domain.slug}/mailboxes/{mailbox.pk}/" - ) - - assert response.status_code == status.HTTP_403_FORBIDDEN