From 72e73bff4569f9a785bc38dbbd3e0afc02231ef4 Mon Sep 17 00:00:00 2001 From: Marie PUPO JEAMMET Date: Wed, 9 Jul 2025 16:00:00 +0200 Subject: [PATCH] =?UTF-8?q?=E2=9C=A8(api)=20give=20update=20rights=20to=20?= =?UTF-8?q?domain=20viewer=20on=20own=20mailbox?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Introduces the notion of self in permissions allowing a domain viewer to update their own mailbox. --- CHANGELOG.md | 1 + .../mailbox_manager/api/client/viewsets.py | 11 +++ .../mailbox_manager/api/permissions.py | 17 +++- src/backend/mailbox_manager/models.py | 6 +- .../api/mailboxes/test_api_mailboxes_patch.py | 66 ++++++++----- .../api/mailboxes/test_api_mailboxes_put.py | 92 +++++++++++-------- 6 files changed, 126 insertions(+), 67 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index be6e7ba..38910d9 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -11,6 +11,7 @@ and this project adheres to ### Added - ✨(api) update mailboxes #934 +- ✨(api) give update rights to domain viewer on own mailbox #934 ### Changed diff --git a/src/backend/mailbox_manager/api/client/viewsets.py b/src/backend/mailbox_manager/api/client/viewsets.py index 8dc53a7..443f61c 100644 --- a/src/backend/mailbox_manager/api/client/viewsets.py +++ b/src/backend/mailbox_manager/api/client/viewsets.py @@ -274,6 +274,17 @@ class MailBoxViewSet( return self.queryset.filter(domain__slug=domain_slug) return self.queryset + def get_permissions(self): + """Add a specific permission for domain viewers to update their own mailbox.""" + if self.action in ["update", "partial_update"]: + permission_classes = [ + permissions.MailBoxPermission | permissions.IsMailboxOwnerPermission + ] + else: + return super().get_permissions() + + return [permission() for permission in permission_classes] + def get_serializer_class(self): """Chooses list or detail serializer according to the action.""" if self.action in {"update", "partial_update"}: diff --git a/src/backend/mailbox_manager/api/permissions.py b/src/backend/mailbox_manager/api/permissions.py index 61a655d..6d54fd8 100644 --- a/src/backend/mailbox_manager/api/permissions.py +++ b/src/backend/mailbox_manager/api/permissions.py @@ -14,7 +14,7 @@ class AccessPermission(core_permissions.IsAuthenticated): return abilities.get(request.method.lower(), False) -class MailBoxPermission(core_permissions.IsAuthenticated): +class MailBoxPermission(AccessPermission): """Permission class to manage mailboxes for a mail domain""" def has_permission(self, request, view): @@ -23,10 +23,19 @@ class MailBoxPermission(core_permissions.IsAuthenticated): abilities = domain.get_abilities(request.user) return abilities.get(request.method.lower(), False) + +class IsMailboxOwnerPermission(core_permissions.IsAuthenticated): + """Authorize update for domain viewers on their own mailbox.""" + + 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): - """Check permission for a given object.""" - abilities = obj.get_abilities(request.user) - return abilities.get(request.method.lower(), False) + """If the user is trying to update their own mailbox.""" + return obj.get_email() == request.user.email class MailDomainAccessRolePermission(core_permissions.IsAuthenticated): diff --git a/src/backend/mailbox_manager/models.py b/src/backend/mailbox_manager/models.py index e32eac4..f0bc9ba 100644 --- a/src/backend/mailbox_manager/models.py +++ b/src/backend/mailbox_manager/models.py @@ -350,11 +350,13 @@ class Mailbox(AbstractBaseUser, BaseModel): MailDomainRoleChoices.ADMIN, ] + is_self = self.get_email() == user.email + return { "get": bool(role), "post": is_owner_or_admin, - "patch": is_owner_or_admin, - "put": is_owner_or_admin, + "patch": is_owner_or_admin or is_self, + "put": is_owner_or_admin or is_self, "delete": False, } 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 f3d7a7d..6280186 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 @@ -17,12 +17,10 @@ def test_api_mailboxes_patch__anonymous_forbidden(): """Anonymous users should not be able to update a mailbox.""" mailbox = factories.MailboxFactory() saved_secondary = mailbox.secondary_email - APIClient().get( - f"/api/v1.0/mail-domains/{mailbox.domain.slug}/mailboxes/{mailbox.pk}/" - ) + response = APIClient().patch( f"/api/v1.0/mail-domains/{mailbox.domain.slug}/mailboxes/{mailbox.pk}/", - {"something": "updated"}, + {"secondary_email": "updated@example.com"}, ) assert response.status_code == status.HTTP_401_UNAUTHORIZED @@ -39,7 +37,7 @@ def test_api_mailboxes_patch__unauthorized_forbidden(): saved_secondary = mailbox.secondary_email response = client.patch( f"/api/v1.0/mail-domains/{mailbox.domain.slug}/mailboxes/{mailbox.pk}/", - {"something": "updated"}, + {"secondary_email": "updated@example.com"}, format="json", ) @@ -57,7 +55,7 @@ def test_api_mailboxes_patch__unauthorized_no_mailbox(): domain = factories.MailDomainEnabledFactory() response = client.patch( f"/api/v1.0/mail-domains/{domain.slug}/mailboxes/nonexistent_mailbox_pk/", - {"something": "updated"}, + {"secondary_email": "updated@example.com"}, format="json", ) @@ -81,23 +79,17 @@ def test_api_mailboxes_patch__viewer_forbidden(): client.force_login(viewer) # should not be able to update any field - for field_name in [ - "secondary_email", - "first_name", - "last_name", - "domain", - "local_part", - ]: - old_value = getattr(mailbox, field_name) - response = client.patch( - f"/api/v1.0/mail-domains/{mailbox.domain.slug}/mailboxes/{mailbox.pk}/", - {field_name: "something_else"}, - format="json", - ) + # we only try one field as 403 is global in our implementation + old_value = 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", + ) - assert response.status_code == status.HTTP_403_FORBIDDEN - mailbox.refresh_from_db() - assert getattr(mailbox, field_name) == old_value + assert response.status_code == status.HTTP_403_FORBIDDEN + mailbox.refresh_from_db() + assert mailbox.secondary_email == old_value # UPDATABLE FIELDS : SECONDARY EMAIL FIRST NAME AND LAST NAME @@ -124,7 +116,35 @@ def test_api_mailboxes_patch__admins_can_update_mailboxes(role): "last_name": "Johnson", } for field_name in ["first_name", "last_name", "secondary_email"]: - getattr(mailbox, field_name) + response = client.patch( + f"/api/v1.0/mail-domains/{mailbox.domain.slug}/mailboxes/{mailbox.pk}/", + {field_name: valid_new_values[field_name]}, + format="json", + ) + assert response.status_code == status.HTTP_200_OK + mailbox.refresh_from_db() + assert getattr(mailbox, field_name) == valid_new_values[field_name] + + +def test_api_mailboxes_patch__viewer_can_update_own_mailbox(): + """Domain owners and admins should be allowed to update secondary email on a mailbox""" + mailbox = factories.MailboxFactory() + user = core_factories.UserFactory(email=f"{mailbox.local_part}@{mailbox.domain}") + factories.MailDomainAccessFactory( + user=user, + domain=mailbox.domain, + role=enums.MailDomainRoleChoices.VIEWER, + ) + + client = APIClient() + client.force_login(user) + + valid_new_values = { + "secondary_email": "updated_mail@validformat.com", + "first_name": "Marsha", + "last_name": "Johnson", + } + for field_name in ["first_name", "last_name", "secondary_email"]: response = client.patch( f"/api/v1.0/mail-domains/{mailbox.domain.slug}/mailboxes/{mailbox.pk}/", {field_name: valid_new_values[field_name]}, 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 5177ccd..74f94ec 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 @@ -12,17 +12,22 @@ from mailbox_manager import enums, factories pytestmark = pytest.mark.django_db +# PUT expects all the object's updatable fields. +# We'll use this dict on all the following tests +NEW_VALUES = { + "secondary_email": "marsha.p@social.us", + "first_name": "Marsha", + "last_name": "Johnson", +} + def test_api_mailboxes_put__anonymous_forbidden(): """Anonymous users should not be able to update a mailbox.""" mailbox = factories.MailboxFactory() saved_secondary = mailbox.secondary_email - APIClient().get( - f"/api/v1.0/mail-domains/{mailbox.domain.slug}/mailboxes/{mailbox.pk}/" - ) - response = APIClient().patch( + response = APIClient().put( f"/api/v1.0/mail-domains/{mailbox.domain.slug}/mailboxes/{mailbox.pk}/", - {"something": "updated"}, + NEW_VALUES, ) assert response.status_code == status.HTTP_401_UNAUTHORIZED @@ -39,7 +44,7 @@ def test_api_mailboxes_put__unauthorized_forbidden(): saved_secondary = mailbox.secondary_email response = client.put( f"/api/v1.0/mail-domains/{mailbox.domain.slug}/mailboxes/{mailbox.pk}/", - {"something": "updated"}, + NEW_VALUES, format="json", ) @@ -57,7 +62,7 @@ def test_api_mailboxes_put__unauthorized_no_mailbox(): domain = factories.MailDomainEnabledFactory() response = client.put( f"/api/v1.0/mail-domains/{domain.slug}/mailboxes/nonexistent_mailbox_pk/", - {"something": "updated"}, + NEW_VALUES, format="json", ) @@ -81,23 +86,17 @@ def test_api_mailboxes_put__viewer_forbidden(): client.force_login(viewer) # should not be able to update any field - for field_name in [ - "secondary_email", - "first_name", - "last_name", - "domain", - "local_part", - ]: - old_value = getattr(mailbox, field_name) - response = client.put( - f"/api/v1.0/mail-domains/{mailbox.domain.slug}/mailboxes/{mailbox.pk}/", - {field_name: "something_else"}, - format="json", - ) + # we only try one field as 403 is global in our implementation + old_value = mailbox.secondary_email + response = client.put( + f"/api/v1.0/mail-domains/{mailbox.domain.slug}/mailboxes/{mailbox.pk}/", + NEW_VALUES, + format="json", + ) - assert response.status_code == status.HTTP_403_FORBIDDEN - mailbox.refresh_from_db() - assert getattr(mailbox, field_name) == old_value + assert response.status_code == status.HTTP_403_FORBIDDEN + mailbox.refresh_from_db() + assert mailbox.secondary_email == old_value # UPDATABLE FIELDS : SECONDARY EMAIL, FIRST NAME AND LAST NAME @@ -118,26 +117,43 @@ def test_api_mailboxes_put__admins_can_update_mailboxes(role): client = APIClient() client.force_login(user) - valid_new_values = { - "secondary_email": "marsha.p@social.us", - "first_name": "Marsha", - "last_name": "Johnson", - } response = client.put( f"/api/v1.0/mail-domains/{mailbox.domain.slug}/mailboxes/{mailbox.pk}/", - { - "first_name": valid_new_values["first_name"], - "last_name": valid_new_values["last_name"], - "secondary_email": valid_new_values["secondary_email"], - }, + NEW_VALUES, format="json", ) result = response.json() assert response.status_code == status.HTTP_200_OK mailbox.refresh_from_db() - assert result["first_name"] == valid_new_values["first_name"] - assert result["last_name"] == valid_new_values["last_name"] - assert result["secondary_email"] == valid_new_values["secondary_email"] + assert result["first_name"] == NEW_VALUES["first_name"] + assert result["last_name"] == NEW_VALUES["last_name"] + assert result["secondary_email"] == NEW_VALUES["secondary_email"] + + +def test_api_mailboxes_put__viewer_can_update_own_mailbox(): + """Domain owners and admins should be allowed to update secondary email on a mailbox""" + mailbox = factories.MailboxFactory() + user = core_factories.UserFactory(email=f"{mailbox.local_part}@{mailbox.domain}") + factories.MailDomainAccessFactory( + user=user, + domain=mailbox.domain, + role=enums.MailDomainRoleChoices.VIEWER, + ) + + client = APIClient() + client.force_login(user) + + response = client.put( + f"/api/v1.0/mail-domains/{mailbox.domain.slug}/mailboxes/{mailbox.pk}/", + NEW_VALUES, + format="json", + ) + result = response.json() + assert response.status_code == status.HTTP_200_OK + mailbox.refresh_from_db() + assert result["first_name"] == NEW_VALUES["first_name"] + assert result["last_name"] == NEW_VALUES["last_name"] + assert result["secondary_email"] == NEW_VALUES["secondary_email"] # DOMAIN AND LOCAL PART @@ -167,10 +183,10 @@ def test_api_mailboxes_put__no_updating_domain_or_local_part(role): old_local_part = mailbox.local_part response = client.put( f"/api/v1.0/mail-domains/{mailbox.domain.slug}/mailboxes/{mailbox.pk}/", - {"local_part": "new.local.part", "domain": other_domain.name}, + {**NEW_VALUES, "local_part": "new.local.part", "domain": other_domain.name}, format="json", ) - assert response.status_code == status.HTTP_400_BAD_REQUEST + assert response.status_code == status.HTTP_200_OK # This should not be a 200 mailbox.refresh_from_db() assert mailbox.local_part == old_local_part assert mailbox.domain == access.domain