Merge pull request #4341 from mozilla/fix-domain-address-conflict-mpp…

…-3636

MPP-3636: Fix domain address conflict handling

api/exceptions.py

@@ -1,21 +1,84 @@
-from typing import Union
+from typing import Any, TypedDict
 
 from rest_framework import status
 from rest_framework.exceptions import APIException
 
+from privaterelay.ftl_bundles import main as ftl_bundle
+
 
 class ConflictError(APIException):
     status_code = status.HTTP_409_CONFLICT
     default_detail = "Request conflicts with current state of the target resource."
     default_code = "conflict_error"
 
 
-ErrorContextType = dict[str, Union[int, str]]
+ErrorContextType = dict[str, int | str]
+
+
+class OptionalErrorData(TypedDict, total=False):
+    error_context: ErrorContextType
+
+
+class ErrorData(OptionalErrorData):
+    detail: str
+    error_code: str | list[Any] | dict[str, Any] | None
 
 
 class RelayAPIException(APIException):
-    """Base class for exceptions that may be returned through API"""
+    """
+    Base class for exceptions that may be returned through the API.
+
+    Derived classes should set `default_code` to a unique string identifying the
+    exception. There should be a matching Fluent string with an `api-error-`
+    prefix. For example, the Fluent string "api-error-free-tier-limit" matches
+    the exception class with the default_code "free-tier-limit". These Fluent
+    strings are in misc.ftl.
+
+    Derived classes can set `default_detail` to a human-readable string, or they
+    can set `default_detail_template` to dynamically create the string from extra
+    context. When using default_detail_template, the Fluent string should use the same
+    variable names.
+
+    Derived classes can set `status_code` to the HTTP status code, or accept the
+    APIException default of 500 for a Server Error.
+    """
+
+    default_code: str
+    default_detail: str
+    status_code: int
+
+    def __init__(self, *args, **kwargs) -> None:
+        """Check that derived classes have set the required data."""
+        assert isinstance(self.default_code, str)
+        assert isinstance(self.status_code, int)
+        if hasattr(self, "default_detail_template"):
+            context = self.error_context()
+            assert context
+            self.default_detail = self.default_detail_template.format(**context)
+        assert isinstance(self.default_detail, str)
+        super().__init__(*args, **kwargs)
 
     def error_context(self) -> ErrorContextType:
         """Return context variables for client-side translation."""
         return {}
+
+    def error_data(self) -> ErrorData:
+        """Return extra data for API error responses."""
+
+        # For RelayAPIException classes, this is the default_code and is a string
+        error_code = self.get_codes()
+        assert isinstance(error_code, str)
+
+        # Build the Fluent error ID
+        ftl_id_sub = "api-error-"
+        ftl_id_error = error_code.replace("_", "-")
+        ftl_id = ftl_id_sub + ftl_id_error
+
+        # Replace the default message with the translated Fluent string
+        error_context = self.error_context()
+        translated_detail = ftl_bundle.format(ftl_id, error_context)
+
+        error_data = ErrorData(detail=translated_detail, error_code=error_code)
+        if error_context:
+            error_data["error_context"] = error_context
+        return error_data

api/tests/views_tests.py

@@ -5,28 +5,24 @@
 from model_bakery import baker
 import responses
 
-from django.conf import settings
 from django.contrib.auth.models import User
 from django.contrib.sites.models import Site
 from django.core.cache import cache
-from django.test import (
-    override_settings,
-    RequestFactory,
-    TestCase,
-)
+from django.test import RequestFactory, TestCase
 from django.utils import timezone
 from django.urls import reverse
 from rest_framework.test import APIClient
 
 from allauth.socialaccount.models import SocialAccount, SocialApp
+from waffle.testutils import override_flag
 
 from api.authentication import get_cache_key, INTROSPECT_TOKEN_URL
 from api.tests.authentication_tests import (
     _setup_fxa_response,
     _setup_fxa_response_no_json,
 )
 from api.views import FXA_PROFILE_URL
-from emails.models import Profile, RelayAddress
+from emails.models import Profile, RelayAddress, DomainAddress
 from emails.tests.models_tests import make_free_test_user, make_premium_test_user
 from privaterelay.tests.utils import log_extra
 
@@ -140,10 +136,13 @@ def test_patch_premium_user_subdomain_cannot_be_changed(
 
 def test_patch_profile_fields_are_read_only_by_default(premium_user, prem_api_client):
     """
-    A field in the Profile model should be read only by default, and return a 400 response code
-    (see StrictReadOnlyFieldsMixin in api/serializers/__init__.py), if it is not mentioned in the ProfileSerializer class fields.
+    A field in the Profile model should be read only by default, and return a 400
+    response code (see StrictReadOnlyFieldsMixin in api/serializers/__init__.py), if it
+    is not mentioned in the ProfileSerializer class fields.
 
-    Two fields were tested, num_address_deleted, and sent_welcome_email to see if the behavior matches what is described here: https://www.django-rest-framework.org/api-guide/serializers/#specifying-read-only-fields
+    Two fields were tested, num_address_deleted, and sent_welcome_email to see if the
+    behavior matches what is described here:
+    https://www.django-rest-framework.org/api-guide/serializers/#specifying-read-only-fields
     """
     premium_profile = premium_user.profile
     expected_num_address_deleted = premium_profile.num_address_deleted
@@ -172,7 +171,8 @@ def test_profile_non_read_only_fields_update_correctly(premium_user, prem_api_cl
     """
     A field that is not read only should update correctly on a patch request.
 
-    "Not read only" meaning that it was defined in the serializers fields, but not read_only_fields.
+    "Not read only" meaning that it was defined in the serializers fields, but not
+    read_only_fields.
     """
     premium_profile = premium_user.profile
     old_onboarding_state = premium_profile.onboarding_state
@@ -244,7 +244,10 @@ def test_profile_patch_with_non_read_only_and_read_only_fields(
 
 
 def test_profile_patch_fields_that_dont_exist(premium_user, prem_api_client):
-    """A request sent with only fields that don't exist give a 200 response (this is the default behavior django provides, we decided to leave it as is)"""
+    """
+    A request sent with only fields that don't exist give a 200 response (this is the
+    default behavior django provides, we decided to leave it as is)
+    """
     response = prem_api_client.patch(
         reverse(viewname="profiles-detail", args=[premium_user.profile.id]),
         data={
@@ -287,10 +290,13 @@ def test_post_domainaddress_bad_address_error(prem_api_client) -> None:
 
     assert response.status_code == 400
     ret_data = response.json()
-    # Add unicode characters to get around Fluent.js using unicode isolation.
-    # See https://github.com/projectfluent/fluent.js/wiki/Unicode-Isolation for more info
+    # Add unicode characters to get around Fluent.js using unicode isolation. See:
+    # https://github.com/projectfluent/fluent.js/wiki/Unicode-Isolation
     assert ret_data == {
-        "detail": "“\u2068myNewAlias\u2069” could not be created. Please try again with a different mask name.",
+        "detail": (
+            "“\u2068myNewAlias\u2069” could not be created."
+            " Please try again with a different mask name."
+        ),
         "error_code": "address_unavailable",
         "error_context": {"unavailable_address": "myNewAlias"},
     }
@@ -304,15 +310,64 @@ def test_post_domainaddress_free_user_error(free_api_client):
 
     assert response.status_code == 403
     ret_data = response.json()
-    # Add unicode characters to get around Fluent.js using unicode isolation.
-    # See https://github.com/projectfluent/fluent.js/wiki/Unicode-Isolation for more info
+    # Add unicode characters to get around Fluent.js using unicode isolation. See:
+    # https://github.com/projectfluent/fluent.js/wiki/Unicode-Isolation
     assert ret_data == {
-        "detail": "Your free account does not include custom subdomains for masks. To create custom masks, upgrade to \u2068Relay Premium\u2069.",
+        "detail": (
+            "Your free account does not include custom subdomains for masks."
+            " To create custom masks, upgrade to \u2068Relay Premium\u2069."
+        ),
         "error_code": "free_tier_no_subdomain_masks",
     }
 
 
-def test_post_relayaddress_success(settings, free_api_client) -> None:
+@pytest.mark.django_db(transaction=True)
+def test_post_domainaddress_conflict_existing(prem_api_client, premium_user):
+    """A user can not create a duplicate domain address."""
+    DomainAddress.objects.create(user=premium_user, address="my-new-alias")
+    response = prem_api_client.post(
+        reverse("domainaddress-list"), data={"address": "my-new-alias"}, format="json"
+    )
+
+    assert response.status_code == 409
+    ret_data = response.json()
+    # Add unicode characters to get around Fluent.js using unicode isolation. See:
+    # https://github.com/projectfluent/fluent.js/wiki/Unicode-Isolation
+    assert ret_data == {
+        "detail": (
+            "“\u2068my-new-alias\u2069” already exists."
+            " Please try again with a different mask name."
+        ),
+        "error_code": "duplicate_address",
+        "error_context": {"duplicate_address": "my-new-alias"},
+    }
+
+
+@pytest.mark.django_db(transaction=True)
+@override_flag("custom_domain_management_redesign", active=True)
+def test_post_domainaddress_conflict_deleted(prem_api_client, premium_user):
+    """A user can not create a domain address that matches a deleted address."""
+    existing = DomainAddress.objects.create(user=premium_user, address="my-new-alias")
+    existing.delete()
+    response = prem_api_client.post(
+        reverse("domainaddress-list"), data={"address": "my-new-alias"}, format="json"
+    )
+
+    assert response.status_code == 400
+    ret_data = response.json()
+    # Add unicode characters to get around Fluent.js using unicode isolation. See:
+    # https://github.com/projectfluent/fluent.js/wiki/Unicode-Isolation
+    assert ret_data == {
+        "detail": (
+            "“\u2068my-new-alias\u2069” could not be created."
+            " Please try again with a different mask name."
+        ),
+        "error_code": "address_unavailable",
+        "error_context": {"unavailable_address": "my-new-alias"},
+    }
+
+
+def test_post_relayaddress_success(free_api_client, free_user, caplog) -> None:
     """A free user is able to create a random address."""
     response = free_api_client.post(
         reverse("relayaddress-list"), data={}, format="json"
@@ -334,13 +389,15 @@ def test_post_relayaddress_free_mask_email_limit_error(
 
     assert response.status_code == 403
     ret_data = response.json()
-    # Add unicode characters to get around Fluent.js using unicode isolation.
-    # See https://github.com/projectfluent/fluent.js/wiki/Unicode-Isolation for more info
+    # Add unicode characters to get around Fluent.js using unicode isolation. See:
+    # https://github.com/projectfluent/fluent.js/wiki/Unicode-Isolation
 
     assert ret_data == {
         "detail": (
             "You’ve used all"
-            f" \u2068{settings.MAX_NUM_FREE_ALIASES}\u2069 email masks included with your free account. You can reuse an existing mask, but using a unique mask for each account is the most secure option."
+            f" \u2068{settings.MAX_NUM_FREE_ALIASES}\u2069 email masks included with"
+            " your free account. You can reuse an existing mask, but using a unique"
+            " mask for each account is the most secure option."
         ),
         "error_code": "free_tier_limit",
         "error_context": {"free_tier_limit": 5},