chore(api): move subname and type validation to serializer

api/desecapi/serializers.py

@@ -1,3 +1,4 @@
+from django.core.validators import RegexValidator
 from rest_framework import serializers
 from rest_framework.exceptions import ValidationError
 from desecapi.models import Domain, Donation, User, RR, RRset, Token
@@ -48,9 +49,11 @@ class RRsetBulkListSerializer(BulkListSerializer):
         return self.child._save([None] * len(validated_data), validated_data)
 
 
-class RRsetTypeField(serializers.CharField):
+class RequiredOnPartialUpdateCharField(serializers.CharField):
+    """
+    This field is always required, even for partial updates (e.g. using PATCH).
+    """
     def validate_empty_values(self, data):
-        # The type field is always required, regardless of PATCH or not
         if data is empty:
             self.fail('required')
 
@@ -69,11 +72,20 @@ class SlugRRField(serializers.SlugRelatedField):
 
 class RRsetSerializer(BulkSerializerMixin, serializers.ModelSerializer):
     domain = serializers.StringRelatedField()
-    subname = serializers.CharField(allow_blank=True, required=False)
-    type = RRsetTypeField()
+    subname = serializers.CharField(
+        allow_blank=True,
+        required=False,
+        validators=[
+            RegexValidator(regex=r'^\*?[a-zA-Z\.\-_0-9]*$', message='Subname malformed.', code='invalid_subname'),
+        ]
+    )
+    type = RequiredOnPartialUpdateCharField(
+        allow_blank=False,
+        required=True,
+        validators=[RegexValidator(regex=r'^[A-Z][A-Z0-9]*$', message='Type malformed.', code='invalid_type')]
+    )
     records = SlugRRField(many=True)
 
-
     class Meta:
         model = RRset
         fields = ('id', 'domain', 'subname', 'name', 'records', 'ttl', 'type',)

api/desecapi/tests/testrrsets.py

@@ -325,11 +325,12 @@ class AuthenticatedRRsetTests(APITestCase):
 
     def testCanPutOwnRRsetApex(self):
         url = reverse('v1:rrsets', args=(self.ownedDomains[1].name,))
-        data = {'records': ['1.2.3.4'], 'ttl': 60, 'type': 'A'}
+        data = {'records': ['1.2.3.4'], 'ttl': 60, 'subname': 'sub', 'type': 'A'}
         response = self.client.post(url, json.dumps(data), content_type='application/json')
         self.assertEqual(response.status_code, status.HTTP_201_CREATED)
 
-        url = reverse('v1:rrset@', args=(self.ownedDomains[1].name, '', 'A',))
+        # Put a non-empty subdomain here to also test URLs like rrsets/<subname>@/
+        url = reverse('v1:rrset@', args=(self.ownedDomains[1].name, 'sub', 'A',))
 
         data = {'records': ['2.2.3.4'], 'ttl': 30}
         response = self.client.put(url, json.dumps(data), content_type='application/json')

api/desecapi/urls/version_1.py

@@ -36,13 +36,13 @@ api_urls = [
     path('domains/', views.DomainList.as_view(), name='domain-list'),
     path('domains/<name>/', views.DomainDetail.as_view(), name='domain-detail'),
     path('domains/<name>/rrsets/', views.RRsetList.as_view(), name='rrsets'),
-    path('domains/<name>/rrsets/.../<type>/', views.RRsetDetail.as_view()),
-    re_path(r'domains/(?P<name>[^/]+)/rrsets/(?P<subname>(\*)?[a-zA-Z.\-_0-9]*)\.\.\./(?P<type>[A-Z][A-Z0-9]*)/',
+    path('domains/<name>/rrsets/.../<type>/', views.RRsetDetail.as_view(), kwargs={'subname': ''}),
+    re_path(r'domains/(?P<name>[^/]+)/rrsets/(?P<subname>[^/]*)\.\.\./(?P<type>[^/]+)/',
                 views.RRsetDetail.as_view(), name='rrset'),
-    path('domains/<name>/rrsets/@/<type>/', views.RRsetDetail.as_view()),
-    path('domains/<name>/rrsets/<subname>/<type>/', views.RRsetDetail.as_view()),
-    re_path(r'domains/(?P<name>[^/]+)/rrsets/(?P<subname>(\*)?[a-zA-Z.\-_0-9]*)@/(?P<type>[A-Z][A-Z0-9]*)/',
+    path('domains/<name>/rrsets/@/<type>/', views.RRsetDetail.as_view(), kwargs={'subname': ''}),
+    re_path(r'domains/(?P<name>[^/]+)/rrsets/(?P<subname>[^/]*)@/(?P<type>[^/]+)/',
             views.RRsetDetail.as_view(), name='rrset@'),
+    path('domains/<name>/rrsets/<subname>/<type>/', views.RRsetDetail.as_view()),
 
     # DNS query endpoint
     # TODO remove?

api/desecapi/views.py

@@ -1,5 +1,7 @@
 from __future__ import unicode_literals
 from django.core.mail import EmailMessage
+from rest_framework.generics import get_object_or_404
+
 from desecapi.models import Domain, User, RRset, Token
 from desecapi.serializers import (
     DomainSerializer, RRsetSerializer, DonationSerializer, TokenSerializer)
@@ -180,7 +182,6 @@ class DomainDetail(generics.RetrieveUpdateDestroyAPIView):
 
 
 class RRsetDetail(generics.RetrieveUpdateDestroyAPIView):
-    lookup_field = 'type'
     serializer_class = RRsetSerializer
     permission_classes = (permissions.IsAuthenticated, IsDomainOwner, IsUnlocked,)
 
@@ -192,16 +193,26 @@ class RRsetDetail(generics.RetrieveUpdateDestroyAPIView):
         return Response(status=status.HTTP_204_NO_CONTENT)
 
     def get_queryset(self):
-        name = self.kwargs['name']
-        subname = self.kwargs.get('subname', '')
-        type_ = self.kwargs['type']
+        domain_name = self.kwargs['name']
+
+        try:
+            return self.request.user.domains.get(name=domain_name).rrset_set
+        except Domain.DoesNotExist:
+            raise Http404()
+
+    def get_object(self):
+        queryset = self.filter_queryset(self.get_queryset())
+
+        rrset_type = self.kwargs['type']
+
+        if rrset_type in RRset.RESTRICTED_TYPES:
+            raise PermissionDenied("You cannot tinker with the %s RRset." % rrset_type)
+
+        obj = get_object_or_404(queryset, type=rrset_type, subname=self.kwargs['subname'])
 
-        if type_ in RRset.RESTRICTED_TYPES:
-            raise PermissionDenied("You cannot tinker with the %s RRset." % type_)
+        self.check_object_permissions(self.request, obj)
 
-        return RRset.objects.filter(
-            domain__owner=self.request.user.pk,
-            domain__name=name, subname=subname, type=type_)
+        return obj
 
     def update(self, request, *args, **kwargs):
         if not isinstance(request.data, dict):
@@ -212,8 +223,12 @@ class RRsetDetail(generics.RetrieveUpdateDestroyAPIView):
         if request.data.get('records') == []:
             return self.delete(request, *args, **kwargs)
 
-        request.data['type'] = request.data.pop('type', self.kwargs['type'])
-        request.data['subname'] = request.data.pop('subname', self.kwargs.get('subname', ''))
+        # Attach URL parameters (self.kwargs) to the request body object (request.data),
+        # the latter having preference with both are given.
+        for k in ('type', 'subname'):
+            # This works because we exclusively use JSONParser which causes request.data to be
+            # a dict (and not an immutable QueryDict, as is the case for other parsers)
+            request.data[k] = request.data.pop(k, self.kwargs[k])
 
         try:
             return super().update(request, *args, **kwargs)

test/e2e/spec/api_spec.js

@@ -374,7 +374,7 @@ describe("API v1", function () {
                     return expect(chakram.post(
                             '/domains/' + domain + '/rrsets/',
                             {'subname': '想不出来', 'type': 'A', 'records': ['127.0.0.1'], 'ttl': 60}
-                        )).to.have.status(422);
+                        )).to.have.status(400);
                 });
 
                 describe("can set a wildcard AAAA RRset with multiple records", function () {