Merge ~ack/maas:dns-records-filter-owned into maas:master

[Alberto Donato]

LP: #1767033 - filter DNS entries only showing those related to owned resources for limited users

[MAAS Lander (maas-lander)]

UNIT TESTS
-b dns-records-filter-owned lp:~ack/maas/+git/maas into -b master lp:~maas-committers/maas

STATUS: SUCCESS
COMMIT: 4233e2c4f7780af0dea1654582fb5109e50a0bde

review: Approve

[Björn Tillenius (bjornt)]

Looks good, assuming that it's ok not to show all the entries on the page any more. Not sure what the purpose of showing the domain for non-admin users, though. Was there any discussion about that?

Assuming that was discussed, +1. Of not, it would be good to bring it up first.

review: Approve

[Alberto Donato (ack)]

> Looks good, assuming that it's ok not to show all the entries on the page any
> more. Not sure what the purpose of showing the domain for non-admin users,
> though. Was there any discussion about that?
>
> Assuming that was discussed, +1. Of not, it would be good to bring it up
> first.

There's probably still some use in showing entries for the nodes owned by non-admin.
Technically, it's also possible that a user that was previously an admin could still have static IPs or dns entries created, so it's somewhat useful to see that.

[Andres Rodriguez (andreserl)]

Machines deployed by a normal user have a hostname on a specific domain, so i think it is a good observability.

That also raises a good question though, are normal users allowed to see the DNS records via the API ? I would imagine they are? What about the domains ?

diff --git a/src/maasserver/models/dnsdata.py b/src/maasserver/models/dnsdata.py
index 8a9f412..028b7ac 100644
--- a/src/maasserver/models/dnsdata.py
+++ b/src/maasserver/models/dnsdata.py
@@ -69,15 +69,17 @@ class HostnameRRsetMapping:
 
     def __init__(
             self, system_id=None, rrset: set=None, node_type=None,
-            dnsresource_id=None):
+            dnsresource_id=None, user_id=None):
         self.system_id = system_id
         self.node_type = node_type
         self.dnsresource_id = dnsresource_id
+        self.user_id = user_id
         self.rrset = set() if rrset is None else rrset.copy()
 
     def __repr__(self):
-        return "HostnameRRSetMapping(%r, %r, %r, %r)" % (
-            self.system_id, self.rrset, self.node_type, self.dnsresource_id)
+        return "HostnameRRSetMapping(%r, %r, %r, %r, %r)" % (
+            self.system_id, self.rrset, self.node_type, self.dnsresource_id,
+            self.user_id)
 
     def __eq__(self, other):
         return self.__dict__ == other.__dict__
@@ -153,6 +155,7 @@ class DNSDataManager(Manager, DNSDataQueriesMixin):
                 domain.name,
                 node.system_id,
                 node.node_type,
+                node.user_id,
                 dnsdata.id,
                 """ + ttl_clause + """ AS ttl,
                 dnsdata.rrtype,
@@ -173,6 +176,7 @@ class DNSDataManager(Manager, DNSDataQueriesMixin):
                         nd.hostname AS hostname,
                         nd.system_id AS system_id,
                         nd.node_type AS node_type,
+                        nd.owner_id AS user_id ,
                         nd.domain_id AS domain_id,
                         CONCAT(nd.hostname, '.', dom.name) AS fqdn
                     FROM maasserver_node AS nd
@@ -220,7 +224,7 @@ class DNSDataManager(Manager, DNSDataQueriesMixin):
         mapping = defaultdict(HostnameRRsetMapping)
         cursor.execute(sql_query, (domain.id,))
         for (dnsresource_id, name, d_name, system_id, node_type,
-                dnsdata_id, ttl, rrtype, rrdata) in cursor.fetchall():
+             user_id, dnsdata_id, ttl, rrtype, rrdata) in cursor.fetchall():
             if name == '@' and d_name != domain.name:
                 name, d_name = d_name.split('.', 1)
                 # Since we don't allow more than one label in dnsresource
@@ -228,14 +232,16 @@ class DNSDataManager(Manager, DNSDataQueriesMixin):
                 assert d_name == domain.name, (
                     "Invalid domain; expected '%s' == '%s'" % (
                         d_name, domain.name))
-            mapping[name].node_type = node_type
-            mapping[name].system_id = system_id
+            entry = mapping[name]
+            entry.node_type = node_type
+            entry.system_id = system_id
+            entry.user_id = user_id
             if with_ids:
-                mapping[name].dnsresource_id = dnsresource_id
+                entry.dnsresource_id = dnsresource_id
                 rrtuple = (ttl, rrtype, rrdata, dnsdata_id)
             else:
                 rrtuple = (ttl, rrtype, rrdata)
-            mapping[name].rrset.add(rrtuple)
+            entry.rrset.add(rrtuple)
         return mapping
 
 
diff --git a/src/maasserver/models/domain.py b/src/maasserver/models/domain.py
index 4735698..3751a23 100644
--- a/src/maasserver/models/domain.py
+++ b/src/maasserver/models/domain.py
@@ -376,24 +376,24 @@ class Domain(CleanSave, TimestampedModel):
         ip_mapping = StaticIPAddress.objects.get_hostname_ip_mapping(
             self, raw_ttl=True)
         for hostname, info in ip_mapping.items():
-            hostname = hostname[:-len(self.name) - 1]
-            rr_mapping[hostname].dnsresource_id = info.dnsresource_id
+            entry = rr_mapping[hostname[:-len(self.name) - 1]]
+            entry.dnsresource_id = info.dnsresource_id
             if info.system_id is not None:
-                rr_mapping[hostname].system_id = info.system_id
-                rr_mapping[hostname].node_type = info.node_type
+                entry.system_id = info.system_id
+                entry.node_type = info.node_type
+            if info.user_id is not None:
+                entry.user_id = info.user_id
             for ip in info.ips:
-                if IPAddress(ip).version == 6:
-                    rr_mapping[hostname].rrset.add(
-                        (info.ttl, 'AAAA', ip, None))
-                else:
-                    rr_mapping[hostname].rrset.add(
-                        (info.ttl, 'A', ip, None))
+                record_type = 'AAAA' if IPAddress(ip).version == 6 else 'A'
+                entry.rrset.add((info.ttl, record_type, ip, None))
+
         data = []
         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,
diff --git a/src/maasserver/models/staticipaddress.py b/src/maasserver/models/staticipaddress.py
index 837045d..bfc9d61 100644
--- a/src/maasserver/models/staticipaddress.py
+++ b/src/maasserver/models/staticipaddress.py
@@ -64,6 +64,7 @@ _mapping_base_fields = (
     'fqdn',
     'system_id',
     'node_type',
+    'user_id',
     'ttl',
     'ip',
 )
@@ -96,17 +97,18 @@ class HostnameIPMapping:
 
     def __init__(
             self, system_id=None, ttl=None, ips: set=None, node_type=None,
-            dnsresource_id=None):
+            dnsresource_id=None, user_id=None):
         self.system_id = system_id
         self.node_type = node_type
         self.ttl = ttl
         self.ips = set() if ips is None else ips.copy()
         self.dnsresource_id = dnsresource_id
+        self.user_id = user_id
 
     def __repr__(self):
-        return "HostnameIPMapping(%r, %r, %r, %r, %r)" % (
+        return "HostnameIPMapping(%r, %r, %r, %r, %r, %r)" % (
             self.system_id, self.ttl, self.ips, self.node_type,
-            self.dnsresource_id)
+            self.dnsresource_id, self.user_id)
 
     def __eq__(self, other):
         return self.__dict__ == other.__dict__
@@ -335,6 +337,7 @@ class StaticIPAddressManager(Manager):
                 COALESCE(dnsrr.fqdn, node.fqdn) AS fqdn,
                 node.system_id,
                 node.node_type,
+                staticip.user_id,
                 """ + ttl_clause + """ AS ttl,
                 staticip.ip,
                 dnsrr.id AS dnsresource_id
@@ -374,6 +377,7 @@ class StaticIPAddressManager(Manager):
                     dom.name as dom_name,
                     nd.system_id,
                     nd.node_type,
+                    nd.owner_id AS user_id,
                     nd.domain_id,
                     nd.address_ttl,
                     dom.ttl,
@@ -461,13 +465,16 @@ class StaticIPAddressManager(Manager):
             # TTL.  It is left as an exercise for the admin to make sure that
             # the any non-default TTL applied to the Node and DNSResource are
             # equal.
+            entry = mapping[fqdn]
             if result.system_id is not None:
-                mapping[fqdn].node_type = result.node_type
-                mapping[fqdn].system_id = result.system_id
+                entry.node_type = result.node_type
+                entry.system_id = result.system_id
             if result.ttl is not None:
-                mapping[fqdn].ttl = result.ttl
-            mapping[fqdn].ips.add(result.ip)
-            mapping[fqdn].dnsresource_id = result.dnsresource_id
+                entry.ttl = result.ttl
+            if result.user_id is not None:
+                entry.user_id = result.user_id
+            entry.ips.add(result.ip)
+            entry.dnsresource_id = result.dnsresource_id
         return mapping
 
     def get_hostname_ip_mapping(self, domain_or_subnet, raw_ttl=False):
@@ -501,6 +508,7 @@ class StaticIPAddressManager(Manager):
                 CONCAT(node.hostname, '.', domain.name) AS fqdn,
                 node.system_id,
                 node.node_type,
+                staticip.user_id,
                 """ + ttl_clause + """ AS ttl,
                 staticip.ip,
                 COALESCE(
@@ -594,6 +602,7 @@ class StaticIPAddressManager(Manager):
                 CONCAT(node.hostname, '.', domain.name) AS fqdn,
                 node.system_id,
                 node.node_type,
+                node.owner_id AS user_id,
                 """ + ttl_clause + """ AS ttl,
                 staticip.ip,
                 interface.name,
@@ -655,14 +664,17 @@ class StaticIPAddressManager(Manager):
         # interface IPs.  See Bug#1584850
         for result in cursor.fetchall():
             result = MappingQueryResult(*result)
-            mapping[result.fqdn].node_type = result.node_type
-            mapping[result.fqdn].system_id = result.system_id
-            mapping[result.fqdn].ttl = result.ttl
+            entry = mapping[result.fqdn]
+            entry.node_type = result.node_type
+            entry.system_id = result.system_id
+            if result.user_id is not None:
+                entry.user_id = result.user_id
+            entry.ttl = result.ttl
             if result.is_boot:
                 iface_is_boot[result.fqdn] = True
             # If we have an IP on the right interface type, save it.
             if result.is_boot == iface_is_boot[result.fqdn]:
-                mapping[result.fqdn].ips.add(result.ip)
+                entry.ips.add(result.ip)
         # Next, get all the addresses, on all the interfaces, and add the ones
         # that are not already present on the FQDN as $IFACE.$FQDN.  Exclude
         # any discovered addresses once there are any non-discovered addresses.
@@ -675,11 +687,14 @@ class StaticIPAddressManager(Manager):
             # node, then consider adding the IP.
             if result.assigned or not assigned_ips[result.fqdn]:
                 if result.ip not in mapping[result.fqdn].ips:
-                    name = "%s.%s" % (result.iface_name, result.fqdn)
-                    mapping[name].node_type = result.node_type
-                    mapping[name].system_id = result.system_id
-                    mapping[name].ttl = result.ttl
-                    mapping[name].ips.add(result.ip)
+                    entry = mapping[
+                        "%s.%s" % (result.iface_name, result.fqdn)]
+                    entry.node_type = result.node_type
+                    entry.system_id = result.system_id
+                    if result.user_id is not None:
+                        entry.user_id = result.user_id
+                    entry.ttl = result.ttl
+                    entry.ips.add(result.ip)
         return mapping
 
     def filter_by_ip_family(self, family):
diff --git a/src/maasserver/models/tests/test_dnsdata.py b/src/maasserver/models/tests/test_dnsdata.py
index fa4923e..3b769be 100644
--- a/src/maasserver/models/tests/test_dnsdata.py
+++ b/src/maasserver/models/tests/test_dnsdata.py
@@ -287,6 +287,17 @@ class TestDNSDataMapping(MAASServerTestCase):
         actual = DNSData.objects.get_hostname_dnsdata_mapping(domain)
         self.assertEqual(expected_mapping, actual)
 
+    def test_get_hostname_dnsdata_mapping_includes_node_owner_id(self):
+        domain = Domain.objects.get_default_domain()
+        user = factory.make_User()
+        node_name = factory.make_name("node")
+        factory.make_Node_with_Interface_on_Subnet(
+            hostname=node_name, domain=domain, owner=user)
+        dnsrr = factory.make_DNSResource(domain=domain, name=node_name)
+        factory.make_DNSData(dnsresource=dnsrr, ip_addresses=True)
+        mapping = DNSData.objects.get_hostname_dnsdata_mapping(domain)
+        self.assertEqual(mapping[node_name].user_id, user.id)
+
     def test_get_hostname_dnsdata_mapping_returns_mapping_at_domain(self):
         parent = Domain.objects.get_default_domain()
         name = factory.make_name("node")
diff --git a/src/maasserver/models/tests/test_domain.py b/src/maasserver/models/tests/test_domain.py
index c47ab37..f2a7f77 100644
--- a/src/maasserver/models/tests/test_domain.py
+++ b/src/maasserver/models/tests/test_domain.py
@@ -381,6 +381,8 @@ class DomainTest(MAASServerTestCase):
             hostname = hostname[:-len(domain.name) - 1]
             if info.system_id is not None:
                 rr_map[hostname].system_id = info.system_id
+            if info.user_id is not None:
+                rr_map[hostname].user_id = info.user_id
             for ip in info.ips:
                 if IPAddress(ip).version == 4:
                     rr_map[hostname].rrset.add((info.ttl, 'A', ip, None))
@@ -392,6 +394,7 @@ class DomainTest(MAASServerTestCase):
                 'name': name,
                 'system_id': info.system_id,
                 'node_type': info.node_type,
+                'user_id': info.user_id,
                 'dnsresource_id': info.dnsresource_id,
                 'ttl': ttl,
                 'rrtype': rrtype,
@@ -417,3 +420,17 @@ class DomainTest(MAASServerTestCase):
         expected = self.render_rrdata(domain, for_list=False)
         actual = domain.render_json_for_related_rrdata(for_list=False)
         self.assertItemsEqual(expected, actual)
+
+    def test_render_json_for_related_rrdata_includes_user_id(self):
+        domain = factory.make_Domain()
+        node_name = factory.make_name('node')
+        user = factory.make_User()
+        factory.make_Node_with_Interface_on_Subnet(
+            hostname=node_name, domain=domain, owner=user)
+        dnsrr = factory.make_DNSResource(domain=domain, name=node_name)
+        factory.make_DNSData(dnsresource=dnsrr, ip_addresses=True)
+        expected = self.render_rrdata(domain, for_list=False)
+        actual = domain.render_json_for_related_rrdata(for_list=True)
+        self.assertEqual(actual, expected)
+        for record in actual:
+            self.assertEqual(record['user_id'], user.id)
diff --git a/src/maasserver/models/tests/test_staticipaddress.py b/src/maasserver/models/tests/test_staticipaddress.py
index f0fcda1..7b72861 100644
--- a/src/maasserver/models/tests/test_staticipaddress.py
+++ b/src/maasserver/models/tests/test_staticipaddress.py
@@ -382,6 +382,22 @@ class TestStaticIPAddressManagerMapping(MAASServerTestCase):
         mapping = StaticIPAddress.objects.get_hostname_ip_mapping(domain)
         self.assertEqual(expected_mapping, mapping)
 
+    def test_get_hostname_ip_mapping_includes_user_id(self):
+        user = factory.make_User()
+        domain = Domain.objects.get_default_domain()
+        node_name = factory.make_name("node")
+        node = factory.make_Node(
+            hostname=node_name, domain=domain, owner=user, interface=True)
+        boot_interface = node.get_boot_interface()
+        subnet = factory.make_Subnet()
+        factory.make_StaticIPAddress(
+            alloc_type=IPADDRESS_TYPE.STICKY,
+            ip=factory.pick_ip_in_Subnet(subnet),
+            subnet=subnet, interface=boot_interface, user=user)
+        mapping = StaticIPAddress.objects.get_hostname_ip_mapping(domain)
+        fqdn = '{}.{}'.format(node.hostname, domain.name)
+        self.assertEqual(mapping[fqdn].user_id, user.id)
+
     def test_get_hostname_ip_mapping_returns_all_mappings_for_subnet(self):
         domain = Domain.objects.get_default_domain()
         expected_mapping = {}
diff --git a/src/maasserver/websockets/handlers/domain.py b/src/maasserver/websockets/handlers/domain.py
index 35b50ab..50c435c 100644
--- a/src/maasserver/websockets/handlers/domain.py
+++ b/src/maasserver/websockets/handlers/domain.py
@@ -57,6 +57,9 @@ 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]
         if not for_list:
             data["rrsets"] = rrsets
         data["hosts"] = len({
diff --git a/src/maasserver/websockets/handlers/tests/test_domain.py b/src/maasserver/websockets/handlers/tests/test_domain.py
index fe83a7d..f04f2db 100644
--- a/src/maasserver/websockets/handlers/tests/test_domain.py
+++ b/src/maasserver/websockets/handlers/tests/test_domain.py
@@ -61,6 +61,8 @@ class TestDomainHandler(MAASServerTestCase):
             name = name[:-domainname_len - 1]
             if info.system_id is not None:
                 rr_map[name].system_id = info.system_id
+            if info.user_id is not None:
+                rr_map[name].user_id = info.user_id
             for ip in info.ips:
                 if IPAddress(ip).version == 4:
                     rr_map[name].rrset.add((info.ttl, 'A', ip, None))
@@ -72,6 +74,7 @@ class TestDomainHandler(MAASServerTestCase):
                 '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,
@@ -94,7 +97,7 @@ class TestDomainHandler(MAASServerTestCase):
         return data
 
     def test_get(self):
-        user = factory.make_User()
+        user = factory.make_admin()
         handler = DomainHandler(user, {})
         domain = factory.make_Domain()
         factory.make_DNSData(domain=domain)
@@ -104,6 +107,25 @@ class TestDomainHandler(MAASServerTestCase):
             self.dehydrate_domain(domain),
             handler.get({"id": domain.id}))
 
+    def test_get_normal_user_only_owned_entries(self):
+        user = factory.make_User()
+        other_user = factory.make_User()
+        handler = DomainHandler(user, {})
+        domain = factory.make_Domain()
+        hostname1 = factory.make_name("node")
+        factory.make_Node_with_Interface_on_Subnet(
+            hostname=hostname1, domain=domain, owner=user)
+        dnsrr1 = factory.make_DNSResource(domain=domain, name=hostname1)
+        factory.make_DNSData(dnsresource=dnsrr1, ip_addresses=True)
+        hostname2 = factory.make_name("node")
+        factory.make_Node_with_Interface_on_Subnet(
+            hostname=hostname2, domain=domain, owner=other_user)
+        dnsrr2 = factory.make_DNSResource(domain=domain, name=hostname2)
+        factory.make_DNSData(dnsresource=dnsrr2, ip_addresses=True)
+        details = handler.get({"id": domain.id})
+        for entry in details['rrsets']:
+            self.assertEqual(entry['user_id'], user.id)
+
     def test_list(self):
         user = factory.make_User()
         handler = DomainHandler(user, {})