Merge ~mpontillo/maas:dns-api-ui-consistency--bug-1686864 into maas:master

[Mike Pontillo]

LP: #1686864 - Optionally allow implicit DNS records to be shown using the API.

Note: testing is complete; this is ready for review.

[MAAS Lander (maas-lander)]

UNIT TESTS
-b dns-api-ui-consistency--bug-1686864 lp:~mpontillo/maas/+git/maas into -b master lp:~maas-committers/maas

STATUS: SUCCESS
COMMIT: 6a1207b47b598a5a7004428e2c9c22f0776c07fa

review: Approve

[MAAS Lander (maas-lander)]

UNIT TESTS
-b dns-api-ui-consistency--bug-1686864 lp:~mpontillo/maas/+git/maas into -b master lp:~maas-committers/maas

STATUS: SUCCESS
COMMIT: 4e85a4cda06c5242bbcc8542b56d97cfbedf3091

review: Approve

[Andres Rodriguez (andreserl)]

Thanks for finishing up this branch. Quick question.. do normal users get to see all DNS records or only the records of stuff they own/they created?

review: Needs Information

[Mike Pontillo (mpontillo)]

@Andres, this branch allows users to see implicit DNS records for any domain they have read permissions on (making what is shown consistent with the UI). Normal users can read all the DNS records, unless that has changed.

ec7d9f7... by Mike Pontillo on 2018-05-14

Refactor to filter display of records for non-superusers.

[Mike Pontillo (mpontillo)]

Given that we now filter out records for machines that aren't owned by the user, we need to decide if it's still appropriate to call the parameter that shows implicit records "all=true". We could use "implicit=true" and reserve "all=true" for a potential future command that would show records regardless of machine ownership.

But we need to make some decisions about the related end-to-end security model, such as which users are allowed to see subnet utilization and its correlation with owned nodes. (Currently, normal users can generally view the entire MAAS network model.)

[MAAS Lander (maas-lander)]

UNIT TESTS
-b dns-api-ui-consistency--bug-1686864 lp:~mpontillo/maas/+git/maas into -b master lp:~maas-committers/maas

STATUS: SUCCESS
COMMIT: 3f1a6faa30124573d0dc8049cb8784ac683e76d7

review: Approve

[Blake Rouse (blake-rouse)]

review: Needs Information

0e7b3b1... by Mike Pontillo on 2018-09-05

Add comment, per code review.

[MAAS Lander (maas-lander)]

UNIT TESTS
-b dns-api-ui-consistency--bug-1686864 lp:~mpontillo/maas/+git/maas into -b master lp:~maas-committers/maas

STATUS: SUCCESS
COMMIT: 0e7b3b158ee46625c085834b317698d3b36b9f33

review: Approve

[Blake Rouse (blake-rouse)]

Yep, land it!

review: Approve

diff --git a/src/maasserver/api/dnsresources.py b/src/maasserver/api/dnsresources.py
index 5746244..bd8e994 100644
--- a/src/maasserver/api/dnsresources.py
+++ b/src/maasserver/api/dnsresources.py
@@ -3,10 +3,13 @@
 
 """API handlers: `DNSResource`."""
 
+from django.db.models.query import QuerySet
+from formencode.validators import StringBool
 from maasserver.api.support import (
     admin_method,
     OperationsHandler,
 )
+from maasserver.api.utils import get_optional_param
 from maasserver.enum import NODE_PERMISSION
 from maasserver.exceptions import MAASAPIValidationError
 from maasserver.forms.dnsresource import DNSResourceForm
@@ -27,6 +30,78 @@ DISPLAYED_DNSRESOURCE_FIELDS = (
 )
 
 
+class DNSResourcesQuerySet(QuerySet):
+
+    def __iter__(self):
+        """Custom iterator which also includes implicit DNS records."""
+        previous_domain = None
+        for record in super().__iter__():
+            # This works because the query always either contains a single
+            # domain, or is sorted by domain.
+            if record.domain != previous_domain:
+                previous_domain = record.domain
+                rrdata = record.domain.render_json_for_related_rrdata(
+                    include_dnsdata=False, as_dict=True,
+                    user=self._user_filter)
+                for name, value in rrdata.items():
+                    name = name.split('.')[0]
+                    # Remove redundant info (this is provided in the top
+                    # level 'fqdn' field).
+                    if (self._name_filter is not None and
+                            name != self._name_filter):
+                        continue
+                    items = []
+                    for rr in value:
+                        if (self._rrtype_filter is not None and
+                                rr['rrtype'] != self._rrtype_filter):
+                            continue
+                        del rr['name']
+                        items.append(rr)
+                    if len(items) > 0:
+                        resource = DNSResource(
+                            id=-1, name=name, domain=record.domain)
+                        resource._rrdata = items
+                        yield resource
+            yield record
+
+
+def get_dnsresource_queryset(
+        all_records: bool, domainname: str=None, name: str=None,
+        rrtype: str=None, user=None):
+    # If the domain is a name, make it an id.
+    if domainname is not None:
+        if domainname.isdigit():
+            domain = Domain.objects.get_domain_or_404(
+                domainname, user=user, perm=NODE_PERMISSION.VIEW)
+        else:
+            domain = Domain.objects.get_domain_or_404(
+                "name:%s" % domainname, user=user,
+                perm=NODE_PERMISSION.VIEW)
+        query = domain.dnsresource_set.all().order_by('name')
+    else:
+        query = DNSResource.objects.all().order_by('domain_id', 'name')
+    rrtype_filter = None
+    name_filter = None
+    if name is not None:
+        query = query.filter(name=name)
+        name_filter = name
+    if rrtype is not None:
+        query = query.filter(dnsdata__rrtype=rrtype)
+        rrtype_filter = rrtype
+    query = query.prefetch_related('ip_addresses', 'dnsdata_set')
+    query = query.select_related('domain')
+    # Note: This must be done last, otherwise our hacks to show additional
+    # records won't work.
+    if all_records is True:
+        query.__class__ = DNSResourcesQuerySet
+        query._name_filter = name_filter
+        query._rrtype_filter = rrtype_filter
+        # Note: the _user_filter should be set to None we want to display
+        # all records, even for non-superusers.
+        query._user_filter = user
+    return query
+
+
 class DNSResourcesHandler(OperationsHandler):
     """Manage dnsresources."""
     api_doc_section_name = "DNSResources"
@@ -44,33 +119,25 @@ class DNSResourcesHandler(OperationsHandler):
         :param name: restrict the listing to entries of the given name.
         :param rrtype: restrict the listing to entries which have
             records of the given rrtype.
+        :param all: if True, also include implicit DNS records created for
+            nodes registered in MAAS.
         """
         data = request.GET
         fqdn = data.get('fqdn', None)
         name = data.get('name', None)
         domainname = data.get('domain', None)
         rrtype = data.get('rrtype', None)
+        if rrtype is not None:
+            rrtype = rrtype.upper()
+        _all = get_optional_param(
+            request.GET, 'all', default=False, validator=StringBool)
         if domainname is None and name is None and fqdn is not None:
             # We need a type for resource separation.  If the user didn't give
             # us a rrtype, then assume it's an address of some sort.
             (name, domainname) = separate_fqdn(fqdn, rrtype)
-        # If the domain is a name, make it an id.
-        if domainname is not None:
-            if domainname.isdigit():
-                domain = Domain.objects.get_domain_or_404(
-                    domainname, user=request.user, perm=NODE_PERMISSION.VIEW)
-            else:
-                domain = Domain.objects.get_domain_or_404(
-                    "name:%s" % domainname, user=request.user,
-                    perm=NODE_PERMISSION.VIEW)
-            query = domain.dnsresource_set.all().order_by('name')
-        else:
-            query = DNSResource.objects.all().order_by('domain_id', 'name')
-        if name is not None:
-            query = query.filter(name=name)
-        if rrtype is not None:
-            query = query.filter(dnsdata__rrtype=rrtype)
-        return query
+        user = request.user
+        return get_dnsresource_queryset(
+            _all, domainname, name, rrtype, user)
 
     @admin_method
     def create(self, request):
@@ -137,12 +204,22 @@ class DNSResourceHandler(OperationsHandler):
     @classmethod
     def ip_addresses(cls, dnsresource):
         """Return IPAddresses within the specified dnsresource."""
+        rrdata = getattr(dnsresource, '_rrdata', None)
+        if rrdata is not None:
+            return None
         return dnsresource.ip_addresses.all()
 
     @classmethod
     def resource_records(cls, dnsresource):
         """Other data for this dnsresource."""
-        return dnsresource.dnsdata_set.all().order_by('rrtype')
+        # If the _rrdata field exists in the resource object, this is a
+        # synthetic record created by the DNSResourcesQuerySet, because the
+        # user has chosen to include implicit records.
+        rrdata = getattr(dnsresource, '_rrdata', None)
+        if rrdata is not None:
+            return rrdata
+        else:
+            return dnsresource.dnsdata_set.all().order_by('rrtype')
 
     def read(self, request, id):
         """Read dnsresource.
diff --git a/src/maasserver/api/tests/test_dnsresources.py b/src/maasserver/api/tests/test_dnsresources.py
index b6a0f98..ed3aa02 100644
--- a/src/maasserver/api/tests/test_dnsresources.py
+++ b/src/maasserver/api/tests/test_dnsresources.py
@@ -10,6 +10,7 @@ import json
 import random
 
 from django.conf import settings
+from maasserver.api.dnsresources import get_dnsresource_queryset
 from maasserver.models.dnsdata import DNSData
 from maasserver.models.dnsresource import DNSResource
 from maasserver.testing.api import APITestCase
@@ -58,6 +59,27 @@ class TestDNSResourcesAPI(APITestCase.ForUser):
             ]
         self.assertItemsEqual(expected_ids, result_ids)
 
+    def test_read_all(self):
+        self.become_admin()
+        for _ in range(3):
+            factory.make_DNSResource()
+        factory.make_RegionRackController()
+        uri = get_dnsresources_uri()
+        response = self.client.get(uri, {'all': 'true'})
+
+        self.assertEqual(
+            http.client.OK, response.status_code, response.content)
+        expected_ids = [
+            dnsresource.id
+            for dnsresource in get_dnsresource_queryset(all_records=True)
+        ]
+        result_ids = [
+            dnsresource["id"]
+            for dnsresource in json.loads(
+                response.content.decode(settings.DEFAULT_CHARSET))
+        ]
+        self.assertItemsEqual(expected_ids, result_ids)
+
     def test_read_with_domain(self):
         for _ in range(3):
             factory.make_DNSResource()
@@ -76,6 +98,29 @@ class TestDNSResourcesAPI(APITestCase.ForUser):
             ]
         self.assertItemsEqual(expected_ids, result_ids)
 
+    def test_read_all_with_domain(self):
+        self.become_admin()
+        for _ in range(3):
+            factory.make_DNSResource()
+        dnsrr = DNSResource.objects.first()
+        uri = get_dnsresources_uri()
+        response = self.client.get(
+            uri,
+            {
+                'all': 'true',
+                'domain': [dnsrr.domain.name]
+            }
+        )
+        self.assertEqual(
+            http.client.OK, response.status_code, response.content)
+        expected_ids = [-1, dnsrr.id]
+        result_ids = [
+            dnsresource["id"]
+            for dnsresource in json.loads(
+                response.content.decode(settings.DEFAULT_CHARSET))
+        ]
+        self.assertItemsEqual(expected_ids, result_ids)
+
     def test_read_with_name(self):
         for _ in range(3):
             factory.make_DNSResource()
diff --git a/src/maasserver/models/domain.py b/src/maasserver/models/domain.py
index 3751a23..69a3825 100644
--- a/src/maasserver/models/domain.py
+++ b/src/maasserver/models/domain.py
@@ -12,6 +12,10 @@ __all__ = [
     "validate_domain_name",
     ]
 
+from collections import (
+    defaultdict,
+    OrderedDict,
+)
 import datetime
 import re
 
@@ -360,7 +364,9 @@ class Domain(CleanSave, TimestampedModel):
         super(Domain, self).clean(*args, **kwargs)
         self.clean_name()
 
-    def render_json_for_related_rrdata(self, for_list=False):
+    def render_json_for_related_rrdata(
+            self, for_list=False, include_dnsdata=True, as_dict=False,
+            user=None):
         """Render a representation of this domain's related non-IP data,
         suitable for converting to JSON.
 
@@ -369,13 +375,22 @@ class Domain(CleanSave, TimestampedModel):
             DNSData,
             StaticIPAddress,
         )
-        rr_mapping = DNSData.objects.get_hostname_dnsdata_mapping(
-            self, raw_ttl=True)
+        if include_dnsdata is True:
+            rr_mapping = DNSData.objects.get_hostname_dnsdata_mapping(
+                self, raw_ttl=True)
+        else:
+            # Circular imports.
+            from maasserver.models.dnsdata import HostnameRRsetMapping
+            rr_mapping = defaultdict(HostnameRRsetMapping)
         # Smash the IP Addresses in the rrset mapping, so that the far end
         # only needs to worry about one thing.
         ip_mapping = StaticIPAddress.objects.get_hostname_ip_mapping(
             self, raw_ttl=True)
         for hostname, info in ip_mapping.items():
+            if (user is not None and not user.is_superuser and
+                    info.user_id is not None and
+                    info.user_id != user.id):
+                continue
             entry = rr_mapping[hostname[:-len(self.name) - 1]]
             entry.dnsresource_id = info.dnsresource_id
             if info.system_id is not None:
@@ -386,20 +401,35 @@ class Domain(CleanSave, TimestampedModel):
             for ip in info.ips:
                 record_type = 'AAAA' if IPAddress(ip).version == 6 else 'A'
                 entry.rrset.add((info.ttl, record_type, ip, None))
-
-        data = []
+        if as_dict is True:
+            result = OrderedDict()
+        else:
+            result = []
         for hostname, info in rr_mapping.items():
-            data += [{
-                'name': hostname,
-                'system_id': info.system_id,
-                'node_type': info.node_type,
-                'user_id': info.user_id,
-                'dnsresource_id': info.dnsresource_id,
-                'ttl': ttl,
-                'rrtype': rrtype,
-                'rrdata': rrdata,
-                'dnsdata_id': dnsdata_id,
+            data = [
+                {
+                    'name': hostname,
+                    'system_id': info.system_id,
+                    'node_type': info.node_type,
+                    'user_id': info.user_id,
+                    'dnsresource_id': info.dnsresource_id,
+                    'ttl': ttl,
+                    'rrtype': rrtype,
+                    'rrdata': rrdata,
+                    'dnsdata_id': dnsdata_id,
                 }
                 for ttl, rrtype, rrdata, dnsdata_id in info.rrset
-                ]
-        return data
+                if (info.user_id is None or
+                    user is None or
+                    user.is_superuser or
+                    (info.user_id is not None and
+                        user is not None and
+                        info.user_id == user.id))
+            ]
+            if as_dict is True:
+                existing = result.get(hostname, [])
+                existing.extend(data)
+                result[hostname] = existing
+            else:
+                result.extend(data)
+        return result
diff --git a/src/maasserver/models/tests/test_domain.py b/src/maasserver/models/tests/test_domain.py
index f2a7f77..054d082 100644
--- a/src/maasserver/models/tests/test_domain.py
+++ b/src/maasserver/models/tests/test_domain.py
@@ -32,7 +32,11 @@ from maasserver.models.staticipaddress import StaticIPAddress
 from maasserver.testing.factory import factory
 from maasserver.testing.testcase import MAASServerTestCase
 from netaddr import IPAddress
-from testtools.matchers import MatchesStructure
+from testtools.matchers import (
+    Equals,
+    HasLength,
+    MatchesStructure,
+)
 from testtools.testcase import ExpectedException
 
 
@@ -372,6 +376,9 @@ class DomainTest(MAASServerTestCase):
                 dnsresource__domain_id=domain.id)
             self.assertEqual("0 0 1688 %s." % target, srvrr.rrdata)
 
+
+class TestRenderRRData(MAASServerTestCase):
+
     def render_rrdata(self, domain, for_list=False):
         rr_map = DNSData.objects.get_hostname_dnsdata_mapping(
             domain, raw_ttl=True)
@@ -434,3 +441,18 @@ class DomainTest(MAASServerTestCase):
         self.assertEqual(actual, expected)
         for record in actual:
             self.assertEqual(record['user_id'], user.id)
+
+    def test_renders_as_dictionary(self):
+        domain = factory.make_Domain()
+        name1 = factory.make_name(prefix='a')
+        name2 = factory.make_name(prefix='b')
+        factory.make_DNSData(name=name1, domain=domain, rrtype='MX')
+        rrdata_list = domain.render_json_for_related_rrdata(as_dict=False)
+        rrdata_dict = domain.render_json_for_related_rrdata(as_dict=True)
+        self.assertThat(rrdata_dict[name1], Equals([rrdata_list[0]]))
+        self.assertThat(rrdata_dict[name1], HasLength(1))
+        factory.make_DNSData(name=name1, domain=domain, rrtype='MX')
+        factory.make_DNSData(name=name2, domain=domain, rrtype='NS')
+        rrdata_dict = domain.render_json_for_related_rrdata(as_dict=True)
+        self.assertThat(rrdata_dict[name1], HasLength(2))
+        self.assertThat(rrdata_dict[name2], HasLength(1))
diff --git a/src/maasserver/testing/factory.py b/src/maasserver/testing/factory.py
index a8a6498..c79ef07 100644
--- a/src/maasserver/testing/factory.py
+++ b/src/maasserver/testing/factory.py
@@ -721,7 +721,7 @@ class Factory(maastesting.factory.Factory):
                 else ip
                 for ip in ip_addresses
             ]
-        dnsrr = DNSResource(
+        dnsrr, _ = DNSResource.objects.get_or_create(
             name=name, address_ttl=address_ttl,
             domain=domain)
         dnsrr.save()
diff --git a/src/maasserver/websockets/handlers/domain.py b/src/maasserver/websockets/handlers/domain.py
index 50c435c..a9a7c30 100644
--- a/src/maasserver/websockets/handlers/domain.py
+++ b/src/maasserver/websockets/handlers/domain.py
@@ -56,10 +56,8 @@ class DomainHandler(TimestampedModelHandler, AdminOnlyMixin):
         ]
 
     def dehydrate(self, domain, data, for_list=False):
-        rrsets = domain.render_json_for_related_rrdata(for_list=for_list)
-        if not self.user.is_superuser:
-            rrsets = [
-                rrset for rrset in rrsets if rrset['user_id'] == self.user.id]
+        rrsets = domain.render_json_for_related_rrdata(
+            for_list=for_list, user=self.user)
         if not for_list:
             data["rrsets"] = rrsets
         data["hosts"] = len({