fix(api): make sure unlock() won't overwrite unmanaged pdns domains

When unlocking a user, we now try POST'ing domains to pdns if the user
created them in our API after being locked, and we do not catch the
exception that occurs when that domain is not new to pdns (like, when
the domain was created manually in the pdns database).

Previously, the exception was ignored (because the same function was
used to sync contents of existing domains to pdns, and these two cases
could not be distinguished).

We already have a test (testCantPostDomainAlreadyTakenInPdns) to make
sure that we error out correctly when the above pdns error occurs
without being caught.  I extended the test
testSuspendedUpdatesDomainCreation to also check that unlocking
triggers POST on the pdns domain endpoint, and that the exception is
actually thrown.

api/desecapi/models.py

@@ -91,9 +91,10 @@ class User(AbstractBaseUser):
         return self.is_admin
 
     def unlock(self):
-        self.locked = None
+        # self.locked is used by domain.sync_to_pdns(), so call that first
         for domain in self.domains.all():
-            domain.deploy(force=True)
+            domain.sync_to_pdns()
+        self.locked = None
         self.save()
 
 
@@ -133,24 +134,29 @@ class Domain(models.Model, mixins.SetterMixin):
 
         return name
 
-    def deploy(self, force=False):
+    def sync_to_pdns(self):
         """
         Make sure that pdns gets the latest information about this domain/zone.
         Re-syncing is relatively expensive and should not happen routinely.
+
+        This method should only be called for new domains or on user unlocking.
+        For unlocked users, it assumes that the domain is a new one.
         """
 
-        # Try to create zone, in case it does not exist yet
-        try:
-            pdns.create_zone(self, settings.DEFAULT_NS)
-            new = True
-        except pdns.PdnsException as e:
-            if force and (e.status_code == 422 \
-                    and e.detail.endswith(' already exists')):
-                new = False
-            else:
-                raise e
+        # Determine if this domain is expected to be new on pdns. This is the
+        # case if the user is not locked (by assumption) or if the domain was
+        # created after the user was locked. (If the user had this domain
+        # before locking, it is not new on pdns.)
+        new = self.owner.locked is None or self.owner.locked < self.created
 
         if new:
+            # Create zone
+            # Throws exception if pdns already knows this zone for some reason
+            # which means that it is not ours and we should not mess with it.
+            # We escalate the exception to let the next level deal with the
+            # response.
+            pdns.create_zone(self, settings.DEFAULT_NS)
+
             # Import RRsets that may have been created (e.g. during lock).
             rrsets = self.rrset_set.all()
             if rrsets:
@@ -159,6 +165,7 @@ class Domain(models.Model, mixins.SetterMixin):
             # Make our RRsets consistent with pdns (specifically, NS may exist)
             self.sync_from_pdns()
 
+            # For dedyn.io domains, propagate NS and DS delegation RRsets
             subname, parent_pdns_id = self.pdns_id.split('.', 1)
             if parent_pdns_id == 'dedyn.io.':
                 try:
@@ -179,7 +186,9 @@ class Domain(models.Model, mixins.SetterMixin):
             # do this through the pdns API (not to mention doing it atomically
             # with setting the new RRsets). So for now, we have disabled RRset
             # deletion for locked accounts.
-            pdns.set_rrsets(self, self.rrset_set.all())
+            rrsets = self.rrset_set.all()
+            if rrsets:
+                pdns.set_rrsets(self, rrsets)
 
     @transaction.atomic
     def sync_from_pdns(self):
@@ -310,7 +319,7 @@ class Domain(models.Model, mixins.SetterMixin):
         super().save(*args, **kwargs)
 
         if new and not self.owner.locked:
-            self.deploy()
+            self.sync_to_pdns()
 
     def __str__(self):
         """
@@ -435,6 +444,8 @@ class RRset(models.Model, mixins.SetterMixin):
 
     @transaction.atomic
     def delete(self, *args, **kwargs):
+        # For locked users, we can't easily sync deleted RRsets to pdns later,
+        # so let's forbid it for now.
         assert not self.domain.owner.locked
         super().delete(*args, **kwargs)
         pdns.set_rrset(self)

api/desecapi/tests/testdyndns12update.py

@@ -6,6 +6,7 @@ import base64
 import httpretty
 from django.conf import settings
 from django.utils import timezone
+from desecapi.exceptions import PdnsException
 
 
 class DynDNS12UpdateTest(APITestCase):
@@ -256,9 +257,11 @@ class DynDNS12UpdateTest(APITestCase):
         self.assertTrue('10.1.1.1' in httpretty.httpretty.latest_requests[-2].parsed_body)
 
     def testSuspendedUpdatesDomainCreation(self):
+        # Lock user
         self.owner.locked = timezone.now()
         self.owner.save()
 
+        # While in locked state, create a domain and set some records
         url = reverse('domain-list')
         newdomain = utils.generateDynDomainname()
 
@@ -281,7 +284,21 @@ class DynDNS12UpdateTest(APITestCase):
         self.assertEqual(response.status_code, status.HTTP_200_OK)
         self.assertIP(name=newdomain, ipv4='10.2.2.2')
 
-        httpretty.register_uri(httpretty.POST, settings.NSLORD_PDNS_API + '/zones')
+        # See what happens upon unlock if pdns knows this domain already
+        httpretty.register_uri(httpretty.POST,
+                               settings.NSLORD_PDNS_API + '/zones',
+                               body='{"error": "Domain \'' + newdomain + '.\' already exists"}',
+                               status=422)
+
+        with self.assertRaises(PdnsException) as cm:
+            self.owner.unlock()
+
+        self.assertEqual(str(cm.exception),
+                         "Domain '" + newdomain + ".' already exists")
+
+        # See what happens upon unlock if this domain is new to pdns
+        httpretty.register_uri(httpretty.POST,
+                               settings.NSLORD_PDNS_API + '/zones')
 
         httpretty.register_uri(httpretty.PATCH, settings.NSLORD_PDNS_API + '/zones/' + newdomain + '.')
         httpretty.register_uri(httpretty.GET,
@@ -299,6 +316,8 @@ class DynDNS12UpdateTest(APITestCase):
 
         self.owner.unlock()
 
+        self.assertEqual(httpretty.httpretty.latest_requests[-5].method, 'POST')
+        self.assertTrue((settings.NSLORD_PDNS_API + '/zones').endswith(httpretty.httpretty.latest_requests[-5].path))
         self.assertEqual(httpretty.httpretty.latest_requests[-3].method, 'PATCH')
         self.assertTrue((settings.NSLORD_PDNS_API + '/zones/' + newdomain + '.').endswith(httpretty.httpretty.latest_requests[-3].path))
         self.assertTrue('10.2.2.2' in httpretty.httpretty.latest_requests[-3].parsed_body)