Merge pull request #36 from desec-io/20170217_errorCodes

api: Improve error handling

api/desecapi/models.py

@@ -1,9 +1,10 @@
 from django.conf import settings
-from django.db import models
+from django.db import models, transaction
 from django.contrib.auth.models import (
     BaseUserManager, AbstractBaseUser
 )
 from django.utils import timezone
+from django.core.exceptions import ValidationError
 from desecapi import pdns
 import datetime, time
 
@@ -98,6 +99,37 @@ class Domain(models.Model):
     arecord = models.CharField(max_length=255, blank=True)
     aaaarecord = models.CharField(max_length=1024, blank=True)
     owner = models.ForeignKey(settings.AUTH_USER_MODEL, related_name='domains')
+    _dirtyName = False
+    _dirtyRecords = False
+
+    def __setattr__(self, attrname, val):
+        setter_func = 'setter_' + attrname
+        if attrname in self.__dict__ and callable(getattr(self, setter_func, None)):
+            super(Domain, self).__setattr__(attrname, getattr(self, setter_func)(val))
+        else:
+            super(Domain, self).__setattr__(attrname, val)
+
+    def setter_name(self, val):
+        if val != self.name:
+            self._dirtyName = True
+
+        return val
+
+    def setter_arecord(self, val):
+        if val != self.arecord:
+            self._dirtyRecords = True
+
+        return val
+
+    def setter_aaaarecord(self, val):
+        if val != self.aaaarecord:
+            self._dirtyRecords = True
+
+        return val
+
+    def clean(self):
+        if self._dirtyName:
+            raise ValidationError('You must not change the domain name')
 
     def pdns_resync(self):
         """
@@ -112,7 +144,7 @@ class Domain(models.Model):
         # update zone to latest information
         pdns.set_dyn_records(self.name, self.arecord, self.aaaarecord)
 
-    def pdns_sync(self):
+    def pdns_sync(self, new_domain):
         """
         Command pdns updates as indicated by the local changes.
         """
@@ -121,36 +153,37 @@ class Domain(models.Model):
             # suspend all updates
             return
 
-        new_domain = self.id is None
-        changes_required = False
-
-        # if this zone is new, create it
+        # if this zone is new, create it and set dirty flag if necessary
         if new_domain:
             pdns.create_zone(self.name)
-
-        # check if current A and AAAA record values require updating pdns
-        if new_domain:
-            changes_required = bool(self.arecord) or bool(self.aaaarecord)
-        else:
-            orig_domain = Domain.objects.get(id=self.id)
-            changes_required = self.arecord != orig_domain.arecord or self.aaaarecord != orig_domain.aaaarecord
+            self._dirtyRecords = bool(self.arecord) or bool(self.aaaarecord)
 
         # make changes if necessary
-        if changes_required:
+        if self._dirtyRecords:
             pdns.set_dyn_records(self.name, self.arecord, self.aaaarecord)
 
+        self._dirtyRecords = False
+
+    @transaction.atomic
     def delete(self, *args, **kwargs):
+        super(Domain, self).delete(*args, **kwargs)
+
         pdns.delete_zone(self.name)
         if self.name.endswith('.dedyn.io'):
             pdns.set_rrset('dedyn.io', self.name, 'DS', '')
             pdns.set_rrset('dedyn.io', self.name, 'NS', '')
-        super(Domain, self).delete(*args, **kwargs)
 
+    @transaction.atomic
     def save(self, *args, **kwargs):
+        # Record here if this is a new domain (self.pk is only None until we call super.save())
+        new_domain = self.pk is None
+
         self.updated = timezone.now()
-        self.pdns_sync()
+        self.clean()
         super(Domain, self).save(*args, **kwargs)
 
+        self.pdns_sync(new_domain)
+
     class Meta:
         ordering = ('created',)
 

api/desecapi/pdns.py

@@ -25,12 +25,16 @@ def _pdns_delete(url):
     # We thus issue a second delete request on nsmaster to delete the zone there immediately.
     r1 = requests.delete(settings.NSLORD_PDNS_API + url, headers=headers_nslord)
     if r1.status_code < 200 or r1.status_code >= 300:
-        raise Exception(r1.text)
+        # Deletion technically does not fail if the zone didn't exist in the first place
+        if r1.status_code == 422 and 'Could not find domain' in r1.text:
+            pass
+        else:
+            raise Exception(r1.text)
 
     # Delete from nsmaster as well
     r2 = requests.delete(settings.NSMASTER_PDNS_API + url, headers=headers_nsmaster)
     if r2.status_code < 200 or r2.status_code >= 300:
-        # Allow this to fail if nsmaster does not know the zone yet
+        # Deletion technically does not fail if the zone didn't exist in the first place
         if r2.status_code == 422 and 'Could not find domain' in r2.text:
             pass
         else:

api/desecapi/serializers.py

@@ -1,5 +1,7 @@
 from rest_framework import serializers
-from desecapi.models import Domain, Donation
+from desecapi.models import Domain, Donation, User
+from djoser import serializers as djoserSerializers
+
 
 class DomainSerializer(serializers.ModelSerializer):
     owner = serializers.ReadOnlyField(source='owner.email')
@@ -7,10 +9,29 @@ class DomainSerializer(serializers.ModelSerializer):
 
     class Meta:
         model = Domain
-        fields = ('id', 'name', 'owner', 'arecord', 'aaaarecord')
+        fields = ('name', 'owner', 'arecord', 'aaaarecord', 'created', 'updated')
+        read_only_fields = ('created', 'updated',)
+
 
 class DonationSerializer(serializers.ModelSerializer):
 
     class Meta:
         model = Donation
         fields = ('name', 'iban', 'bic', 'amount', 'message', 'email')
+
+
+class UserSerializer(djoserSerializers.UserSerializer):
+
+    class Meta(djoserSerializers.UserSerializer.Meta):
+        fields = tuple(User.REQUIRED_FIELDS) + (
+            User.USERNAME_FIELD,
+        )
+
+
+class UserRegistrationSerializer(djoserSerializers.UserRegistrationSerializer):
+
+    class Meta(djoserSerializers.UserRegistrationSerializer.Meta):
+        fields = tuple(User.REQUIRED_FIELDS) + (
+            User.USERNAME_FIELD,
+            'password',
+        )

api/desecapi/settings.py

@@ -111,6 +111,10 @@ DJOSER = {
     'ACTIVATION_URL': '#/activate/{uid}/{token}',
     'LOGIN_AFTER_ACTIVATION': True,
     'SEND_ACTIVATION_EMAIL': False,
+    'SERIALIZERS': {
+        'user': 'desecapi.serializers.UserSerializer',
+        'user_registration': 'desecapi.serializers.UserRegistrationSerializer',
+    },
 }
 
 TEMPLATES = [

api/desecapi/tests/testdomains.py

@@ -76,8 +76,9 @@ class AuthenticatedDomainTests(APITestCase):
 
         url = reverse('domain-detail', args=(self.otherDomains[1].pk,))
         response = self.client.delete(url)
-        self.assertEqual(response.status_code, status.HTTP_404_NOT_FOUND)
+        self.assertEqual(response.status_code, status.HTTP_204_NO_CONTENT)
         self.assertTrue(isinstance(httpretty.last_request(), httpretty.core.HTTPrettyRequestEmpty))
+        self.assertTrue(Domain.objects.filter(pk=self.otherDomains[1].pk).exists())
 
     def testCanGetOwnedDomains(self):
         url = reverse('domain-detail', args=(self.ownedDomains[1].pk,))
@@ -91,15 +92,25 @@ class AuthenticatedDomainTests(APITestCase):
         self.assertEqual(response.status_code, status.HTTP_404_NOT_FOUND)
 
     def testCanPutOwnedDomain(self):
+        url = reverse('domain-detail', args=(self.ownedDomains[1].pk,))
+        response = self.client.get(url)
+        response.data['arecord'] = '1.2.3.4'
+        response = self.client.put(url, response.data)
+        self.assertEqual(response.status_code, status.HTTP_200_OK)
+        response = self.client.get(url)
+        self.assertEqual(response.status_code, status.HTTP_200_OK)
+        self.assertEqual(response.data['arecord'], '1.2.3.4')
+
+    def testCantChangeDomainName(self):
         url = reverse('domain-detail', args=(self.ownedDomains[1].pk,))
         response = self.client.get(url)
         newname = utils.generateDomainname()
         response.data['name'] = newname
         response = self.client.put(url, response.data)
-        self.assertEqual(response.status_code, status.HTTP_200_OK)
+        self.assertEqual(response.status_code, status.HTTP_409_CONFLICT)
         response = self.client.get(url)
         self.assertEqual(response.status_code, status.HTTP_200_OK)
-        self.assertEqual(response.data['name'], newname)
+        self.assertEqual(response.data['name'], self.ownedDomains[1].name)
 
     def testCantPutOtherDomains(self):
         url = reverse('domain-detail', args=(self.otherDomains[1].pk,))
@@ -121,6 +132,18 @@ class AuthenticatedDomainTests(APITestCase):
         response = self.client.post(url, data)
         self.assertEqual(response.status_code, status.HTTP_409_CONFLICT)
 
+    def testCantPostUnavailableDomain(self):
+        name = utils.generateDomainname()
+
+        httpretty.enable()
+        httpretty.register_uri(httpretty.POST, settings.NSLORD_PDNS_API + '/zones',
+                               body='{"error": "Domain \'' + name + '.\' already exists"}', status=422)
+
+        url = reverse('domain-list')
+        data = {'name': name}
+        response = self.client.post(url, data)
+        self.assertEqual(response.status_code, status.HTTP_409_CONFLICT)
+
     def testCanPostComplicatedDomains(self):
         url = reverse('domain-list')
         data = {'name': 'very.long.domain.name.' + utils.generateDomainname()}
@@ -193,6 +216,21 @@ class AuthenticatedDomainTests(APITestCase):
         self.assertTrue(("/%d" % self.ownedDomains[1].pk) in url)
         self.assertTrue("/" + self.ownedDomains[1].name in urlByName)
 
+    def testRollback(self):
+        name = utils.generateDomainname()
+
+        httpretty.enable()
+        httpretty.register_uri(httpretty.POST, settings.NSLORD_PDNS_API + '/zones', body="some error", status=500)
+
+        url = reverse('domain-list')
+        data = {'name': name}
+        try:
+            response = self.client.post(url, data)
+        except:
+            pass
+
+        self.assertFalse(Domain.objects.filter(name=name).exists())
+
 
 class AuthenticatedDynDomainTests(APITestCase):
     def setUp(self):
@@ -235,8 +273,9 @@ class AuthenticatedDynDomainTests(APITestCase):
 
         url = reverse('domain-detail', args=(self.otherDomains[1].pk,))
         response = self.client.delete(url)
-        self.assertEqual(response.status_code, status.HTTP_404_NOT_FOUND)
+        self.assertEqual(response.status_code, status.HTTP_204_NO_CONTENT)
         self.assertTrue(isinstance(httpretty.last_request(), httpretty.core.HTTPrettyRequestEmpty))
+        self.assertTrue(Domain.objects.filter(pk=self.otherDomains[1].pk).exists())
 
     def testCanPostDynDomains(self):
         url = reverse('domain-list')

api/desecapi/views.py

@@ -18,6 +18,7 @@ from desecapi.authentication import BasicTokenAuthentication, URLParamAuthentica
 import base64
 from desecapi import settings
 from rest_framework.exceptions import ValidationError
+import django.core.exceptions
 from djoser import views, signals
 from rest_framework import status
 from datetime import datetime, timedelta
@@ -53,7 +54,7 @@ class DomainList(generics.ListCreateAPIView):
 
         queryset = Domain.objects.filter(name=serializer.validated_data['name'])
         if queryset.exists():
-            ex = ValidationError(detail={"detail": "This domain name is already registered.", "code": "domain-taken"})
+            ex = ValidationError(detail={"detail": "This domain name is unavailable.", "code": "domain-unavailable"})
             ex.status_code = status.HTTP_409_CONFLICT
             raise ex
 
@@ -62,7 +63,15 @@ class DomainList(generics.ListCreateAPIView):
             ex.status_code = status.HTTP_403_FORBIDDEN
             raise ex
 
-        obj = serializer.save(owner=self.request.user)
+        try:
+            obj = serializer.save(owner=self.request.user)
+        except Exception as e:
+            if str(e).endswith(' already exists"}'):
+                ex = ValidationError(detail={"detail": "This domain name is unavailable.", "code": "domain-unavailable"})
+                ex.status_code = status.HTTP_409_CONFLICT
+                raise ex
+            else:
+                raise e
 
         def sendDynDnsEmail(domain):
             content_tmpl = get_template('emails/domain-dyndns/content.txt')
@@ -88,9 +97,24 @@ class DomainDetail(generics.RetrieveUpdateDestroyAPIView):
     serializer_class = DomainSerializer
     permission_classes = (permissions.IsAuthenticated, IsOwner,)
 
+    def delete(self, request, *args, **kwargs):
+        try:
+            super(DomainDetail, self).delete(request, *args, **kwargs)
+        except Http404:
+            pass
+        return Response(status=status.HTTP_204_NO_CONTENT)
+
     def get_queryset(self):
         return Domain.objects.filter(owner=self.request.user.pk)
 
+    def update(self, request, *args, **kwargs):
+        try:
+            return super(DomainDetail, self).update(request, *args, **kwargs)
+        except django.core.exceptions.ValidationError as e:
+            ex = ValidationError(detail={"detail": str(e)})
+            ex.status_code = status.HTTP_409_CONFLICT
+            raise ex
+
 
 class DomainDetailByName(DomainDetail):
     lookup_field = 'name'