fix(api): improvements for authenticated actions

* include domain in AuthenticatedActivateUserAction state
* rename check_expiration to is_expired, simplify API
* add test to check action signature expiration
* rename check_mac to validate_mac
* uniquely identify action types using class name

api/desecapi/migrations/0006_authenticated_actions.py

@@ -3,6 +3,7 @@
 import datetime
 from django.db import migrations, models
 import django.db.models.deletion
+from django.utils import timezone
 
 
 class Migration(migrations.Migration):
@@ -16,7 +17,7 @@ class Migration(migrations.Migration):
             name='AuthenticatedAction',
             fields=[
                 ('id', models.AutoField(auto_created=True, primary_key=True, serialize=False, verbose_name='ID')),
-                ('created', models.PositiveIntegerField(default=lambda: int(datetime.timestamp(datetime.now())))),
+                ('created', models.PositiveIntegerField(default=lambda: int(datetime.timestamp(timezone.now())))),
             ],
             options={
                 'managed': False,

api/desecapi/models.py

@@ -432,7 +432,7 @@ class AuthenticatedAction(models.Model):
 
     AuthenticatedAction provides the `mac` property that returns a Message Authentication Code (MAC) based on the
     state. By default, the state contains the action's name (defined by the `name` property) and a timestamp; the
-    state can be extended by (carefully) overriding the `mac_state` method. Any AuthenticatedAction instance of
+    state can be extended by (carefully) overriding the `_mac_state` property. Any AuthenticatedAction instance of
     the same subclass and state will deterministically have the same MAC, effectively allowing authenticated
     procedure calls by third parties according to the following protocol:
 
@@ -441,7 +441,7 @@ class AuthenticatedAction(models.Model):
     (3) when provided with data that allows instantiation and a valid MAC, take the defined action, possibly with
         additional parameters chosen by the third party that do not belong to the verified state.
     """
-    created = models.PositiveIntegerField(default=lambda: int(datetime.timestamp(datetime.now())))
+    created = models.PositiveIntegerField(default=lambda: int(datetime.timestamp(timezone.now())))
 
     class Meta:
         managed = False
@@ -451,23 +451,16 @@ class AuthenticatedAction(models.Model):
         kwargs.pop('mac', None)
         super().__init__(*args, **kwargs)
 
-    @property
-    def name(self):
-        """
-        Returns a human-readable string containing the name of this action class that uniquely identifies this action.
-        """
-        return NotImplementedError
-
     @property
     def mac(self):
         """
         Deterministically generates a message authentication code (MAC) for this action, based on the state as defined
-        by `self.mac_state`. Identical state is guaranteed to yield identical MAC.
+        by `self._mac_state`. Identical state is guaranteed to yield identical MAC.
         :return:
         """
-        return Signer().signature(json.dumps(self.mac_state))
+        return Signer().signature(json.dumps(self._mac_state))
 
-    def check_mac(self, mac):
+    def validate_mac(self, mac):
         """
         Checks if the message authentication code (MAC) provided by the first argument matches the MAC of this action.
         Note that expiration is not verified by this method.
@@ -479,20 +472,17 @@ class AuthenticatedAction(models.Model):
             self.mac,
         )
 
-    def check_expiration(self, validity_period: timedelta, check_time: datetime = None):
+    def is_expired(self):
         """
-        Checks if the action's timestamp is no older than the given validity period. Note that the message
+        Checks if the action's timestamp is older than the given validity period. Note that the message
         authentication code itself is not verified by this method.
-        :param validity_period: How long after issuance the MAC of this action is considered valid.
-        :param check_time: Point in time for which to check the expiration. Defaults to datetime.now().
-        :return: True if valid, False if expired.
+        :return: True if expired, False otherwise.
         """
-        issue_time = datetime.fromtimestamp(self.created)
-        check_time = check_time or datetime.now()
-        return check_time - issue_time <= validity_period
+        created = datetime.fromtimestamp(self.created, tz=timezone.utc)
+        return timezone.now() - created > settings.VALIDITY_PERIOD_VERIFICATION_SIGNATURE
 
     @property
-    def mac_state(self):
+    def _mac_state(self):
         """
         Returns a list that defines the state of this action (used for MAC calculation).
 
@@ -506,14 +496,15 @@ class AuthenticatedAction(models.Model):
         in valid signatures when they were intended to be invalid. The suggested method for overriding is
 
             @property
-            def mac_state:
-                return super().mac_state + [self.important_value, self.another_added_value]
+            def _mac_state:
+                return super()._mac_state + [self.important_value, self.another_added_value]
 
         :return: List of values to be signed.
         """
         # TODO consider adding a "last change" attribute of the user to the state to avoid code
         #  re-use after the the state has been changed and changed back.
-        return [self.created, self.name]
+        name = '.'.join([self.__module__, self.__class__.__qualname__])
+        return [name, self.created]
 
     def act(self):
         """
@@ -534,12 +525,8 @@ class AuthenticatedUserAction(AuthenticatedAction):
         managed = False
 
     @property
-    def name(self):
-        raise NotImplementedError
-
-    @property
-    def mac_state(self):
-        return super().mac_state + [self.user.id, self.user.email, self.user.password, self.user.is_active]
+    def _mac_state(self):
+        return super()._mac_state + [self.user.id, self.user.email, self.user.password, self.user.is_active]
 
     def act(self):
         raise NotImplementedError
@@ -552,8 +539,8 @@ class AuthenticatedActivateUserAction(AuthenticatedUserAction):
         managed = False
 
     @property
-    def name(self):
-        return 'user/activate'
+    def _mac_state(self):
+        return super()._mac_state + [self.domain]
 
     def act(self):
         self.user.activate()
@@ -566,12 +553,8 @@ class AuthenticatedChangeEmailUserAction(AuthenticatedUserAction):
         managed = False
 
     @property
-    def name(self):
-        return 'user/change_email'
-
-    @property
-    def mac_state(self):
-        return super().mac_state + [self.new_email]
+    def _mac_state(self):
+        return super()._mac_state + [self.new_email]
 
     def act(self):
         self.user.change_email(self.new_email)
@@ -583,10 +566,6 @@ class AuthenticatedResetPasswordUserAction(AuthenticatedUserAction):
     class Meta:
         managed = False
 
-    @property
-    def name(self):
-        return 'user/reset_password'
-
     def act(self):
         self.user.change_password(self.new_password)
 
@@ -596,10 +575,6 @@ class AuthenticatedDeleteUserAction(AuthenticatedUserAction):
     class Meta:
         managed = False
 
-    @property
-    def name(self):
-        return 'user/delete'
-
     def act(self):
         self.user.delete()
 

api/desecapi/serializers.py

@@ -615,13 +615,11 @@ class AuthenticatedActionSerializer(serializers.ModelSerializer):
             self.instance = self.Meta.model(**attrs)  # TODO This creates an attribute on self. Side-effect intended?
 
         # check if expired
-        expired = not self.instance.check_expiration(settings.VALIDITY_PERIOD_VERIFICATION_SIGNATURE)
-        if expired:
+        if self.instance.is_expired():
             raise ValidationError(detail='Code expired, please restart the process.', code='expired')
 
         # check if MAC valid
-        mac_valid = self.instance.check_mac(attrs['mac'])
-        if not mac_valid:
+        if not self.instance.validate_mac(attrs['mac']):
             raise ValidationError(detail='Bad signature.', code='bad_sig')
 
         return attrs

api/desecapi/tests/test_user_management.py

@@ -14,14 +14,19 @@ This involves testing five separate endpoints:
 """
 import random
 import re
+from unittest import mock
+from urllib.parse import urlparse
 
 from django.core import mail
+from django.urls import resolve
+from django.utils import timezone
 from rest_framework import status
 from rest_framework.reverse import reverse
 from rest_framework.test import APIClient
 
 from api import settings
 from desecapi.models import Domain, User, Captcha
+from desecapi.serializers import AuthenticatedActionSerializer
 from desecapi.tests.base import DesecTestCase, PublicSuffixMockMixin
 
 
@@ -332,6 +337,13 @@ class UserManagementTestCase(DesecTestCase, PublicSuffixMockMixin):
             status_code=status.HTTP_400_BAD_REQUEST
         )
 
+    def assertVerificationFailureExpiredCodeResponse(self, response):
+        return self.assertContains(
+            response=response,
+            text="Code expired",
+            status_code=status.HTTP_400_BAD_REQUEST
+        )
+
     def assertVerificationFailureUnknownUserResponse(self, response):
         return self.assertContains(
             response=response,
@@ -351,7 +363,8 @@ class UserManagementTestCase(DesecTestCase, PublicSuffixMockMixin):
         self.assertPassword(email, password)
         return email, password
 
-    def _test_registration_with_domain(self, email=None, password=None, domain=None, expect_failure_response=None):
+    def _test_registration_with_domain(self, email=None, password=None, domain=None, expect_failure_response=None,
+                                       tampered_domain=None):
         domain = domain or self.random_domain_name()
 
         email, password, response = self.register_user(email, password, domain=domain)
@@ -365,6 +378,18 @@ class UserManagementTestCase(DesecTestCase, PublicSuffixMockMixin):
         self.assertPassword(email, password)
 
         confirmation_link = self.assertRegistrationEmail(email)
+
+        if tampered_domain is not None:
+            path = urlparse(confirmation_link).path
+            code = resolve(path).kwargs.get('code')
+            data = AuthenticatedActionSerializer._unpack_code(code)
+            data['domain'] = tampered_domain
+            tampered_code = AuthenticatedActionSerializer._pack_code(data)
+            confirmation_link = confirmation_link.replace(code, tampered_code)
+            response = self.client.verify(confirmation_link)
+            self.assertVerificationFailureInvalidCodeResponse(response)
+            return
+
         if domain.endswith('.dedyn.io'):
             cm = self.requests_desec_domain_creation_auto_delegation(domain)
         else:
@@ -448,6 +473,11 @@ class NoUserAccountTestCase(UserLifeCycleTestCase):
         with self.get_psl_context_manager(local_public_suffix):
             self._test_registration_with_domain(domain=self.random_domain_name(suffix=local_public_suffix))
 
+    def test_registration_with_tampered_domain(self):
+        PublicSuffixMockMixin.setUpMockPatch(self)
+        with self.get_psl_context_manager('.'):
+            self._test_registration_with_domain(tampered_domain='evil.com')
+
     def test_registration_known_account(self):
         email, _ = self._test_registration()
         self.assertRegistrationSuccessResponse(self.register_user(email, self.random_password())[2])
@@ -556,7 +586,7 @@ class HasUserAccountTestCase(UserManagementTestCase):
     def test_view_account_read_only(self):
         # Should this test ever be removed (to allow writeable fields), make sure to
         # add new tests for each read-only field individually (such as limit_domains)!
-        for method in [self.client.put, self.client.post, self.client.delete]:
+        for method in [self.client.patch, self.client.put, self.client.post, self.client.delete]:
             response = method(
                 reverse('v1:account'),
                 {'limit_domains': 99},
@@ -728,3 +758,34 @@ class HasUserAccountTestCase(UserManagementTestCase):
         password = self.random_password()
         self._test_reset_password(self.email, password, code='foobar')
         self.assertPassword(self.email, password)
+
+    def test_action_code_expired(self):
+        self.assertResetPasswordSuccessResponse(self.reset_password(self.email))
+        confirmation_link = self.assertResetPasswordEmail(self.email)
+
+        with mock.patch('desecapi.models.timezone.now',
+                        return_value=timezone.now() + settings.VALIDITY_PERIOD_VERIFICATION_SIGNATURE):
+            response = self.client.verify(confirmation_link, new_password=self.random_password())
+        self.assertVerificationFailureExpiredCodeResponse(response)
+
+    def test_action_code_confusion(self):
+        # Obtain change password code
+        self.assertResetPasswordSuccessResponse(self.reset_password(self.email))
+        reset_password_link = self.assertResetPasswordEmail(self.email)
+        path = urlparse(reset_password_link).path
+        reset_password_code = resolve(path).kwargs.get('code')
+
+        # Obtain deletion code
+        self.assertDeleteAccountSuccessResponse(self.delete_account(self.email, self.password))
+        delete_link = self.assertDeleteAccountEmail(self.email)
+        path = urlparse(delete_link).path
+        deletion_code = resolve(path).kwargs.get('code')
+
+        # Swap codes
+        self.assertNotEqual(reset_password_code, deletion_code)
+        delete_link = delete_link.replace(deletion_code, reset_password_code)
+        reset_password_link = reset_password_link.replace(reset_password_code, deletion_code)
+
+        # Make sure links don't work
+        self.assertVerificationFailureInvalidCodeResponse(self.client.verify(delete_link))
+        self.assertVerificationFailureInvalidCodeResponse(self.client.verify(reset_password_link, new_password='dummy'))