Merge ~mpontillo/maas:dns-api-ui-consistency--bug-1686864 into maas:master
- Git
- lp:~mpontillo/maas
- dns-api-ui-consistency--bug-1686864
- Merge into master
Status: | Merged |
---|---|
Approved by: | Mike Pontillo |
Approved revision: | 0e7b3b158ee46625c085834b317698d3b36b9f33 |
Merge reported by: | MAAS Lander |
Merged at revision: | not available |
Proposed branch: | ~mpontillo/maas:dns-api-ui-consistency--bug-1686864 |
Merge into: | maas:master |
Diff against target: |
411 lines (+213/-41) 6 files modified
src/maasserver/api/dnsresources.py (+95/-18) src/maasserver/api/tests/test_dnsresources.py (+45/-0) src/maasserver/models/domain.py (+47/-17) src/maasserver/models/tests/test_domain.py (+23/-1) src/maasserver/testing/factory.py (+1/-1) src/maasserver/websockets/handlers/domain.py (+2/-4) |
Related bugs: |
Reviewer | Review Type | Date Requested | Status |
---|---|---|---|
Blake Rouse (community) | Approve | ||
MAAS Lander | Approve | ||
Andres Rodriguez (community) | Needs Information | ||
Review via email: mp+345379@code.launchpad.net |
Commit message
LP: #1686864 - Optionally allow implicit DNS records to be shown using the API.
Description of the change
Note: testing is complete; this is ready for review.
MAAS Lander (maas-lander) wrote : | # |
UNIT TESTS
-b dns-api-
STATUS: SUCCESS
COMMIT: 4e85a4cda06c524
Andres Rodriguez (andreserl) wrote : | # |
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?
Mike Pontillo (mpontillo) wrote : | # |
@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
-
Refactor to filter display of records for non-superusers.
Mike Pontillo (mpontillo) wrote : | # |
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) wrote : | # |
UNIT TESTS
-b dns-api-
STATUS: SUCCESS
COMMIT: 3f1a6faa3012457
Blake Rouse (blake-rouse) : | # |
Mike Pontillo (mpontillo) : | # |
- 0e7b3b1... by Mike Pontillo
-
Add comment, per code review.
MAAS Lander (maas-lander) wrote : | # |
UNIT TESTS
-b dns-api-
STATUS: SUCCESS
COMMIT: 0e7b3b158ee4662
Preview Diff
1 | diff --git a/src/maasserver/api/dnsresources.py b/src/maasserver/api/dnsresources.py |
2 | index 5746244..bd8e994 100644 |
3 | --- a/src/maasserver/api/dnsresources.py |
4 | +++ b/src/maasserver/api/dnsresources.py |
5 | @@ -3,10 +3,13 @@ |
6 | |
7 | """API handlers: `DNSResource`.""" |
8 | |
9 | +from django.db.models.query import QuerySet |
10 | +from formencode.validators import StringBool |
11 | from maasserver.api.support import ( |
12 | admin_method, |
13 | OperationsHandler, |
14 | ) |
15 | +from maasserver.api.utils import get_optional_param |
16 | from maasserver.enum import NODE_PERMISSION |
17 | from maasserver.exceptions import MAASAPIValidationError |
18 | from maasserver.forms.dnsresource import DNSResourceForm |
19 | @@ -27,6 +30,78 @@ DISPLAYED_DNSRESOURCE_FIELDS = ( |
20 | ) |
21 | |
22 | |
23 | +class DNSResourcesQuerySet(QuerySet): |
24 | + |
25 | + def __iter__(self): |
26 | + """Custom iterator which also includes implicit DNS records.""" |
27 | + previous_domain = None |
28 | + for record in super().__iter__(): |
29 | + # This works because the query always either contains a single |
30 | + # domain, or is sorted by domain. |
31 | + if record.domain != previous_domain: |
32 | + previous_domain = record.domain |
33 | + rrdata = record.domain.render_json_for_related_rrdata( |
34 | + include_dnsdata=False, as_dict=True, |
35 | + user=self._user_filter) |
36 | + for name, value in rrdata.items(): |
37 | + name = name.split('.')[0] |
38 | + # Remove redundant info (this is provided in the top |
39 | + # level 'fqdn' field). |
40 | + if (self._name_filter is not None and |
41 | + name != self._name_filter): |
42 | + continue |
43 | + items = [] |
44 | + for rr in value: |
45 | + if (self._rrtype_filter is not None and |
46 | + rr['rrtype'] != self._rrtype_filter): |
47 | + continue |
48 | + del rr['name'] |
49 | + items.append(rr) |
50 | + if len(items) > 0: |
51 | + resource = DNSResource( |
52 | + id=-1, name=name, domain=record.domain) |
53 | + resource._rrdata = items |
54 | + yield resource |
55 | + yield record |
56 | + |
57 | + |
58 | +def get_dnsresource_queryset( |
59 | + all_records: bool, domainname: str=None, name: str=None, |
60 | + rrtype: str=None, user=None): |
61 | + # If the domain is a name, make it an id. |
62 | + if domainname is not None: |
63 | + if domainname.isdigit(): |
64 | + domain = Domain.objects.get_domain_or_404( |
65 | + domainname, user=user, perm=NODE_PERMISSION.VIEW) |
66 | + else: |
67 | + domain = Domain.objects.get_domain_or_404( |
68 | + "name:%s" % domainname, user=user, |
69 | + perm=NODE_PERMISSION.VIEW) |
70 | + query = domain.dnsresource_set.all().order_by('name') |
71 | + else: |
72 | + query = DNSResource.objects.all().order_by('domain_id', 'name') |
73 | + rrtype_filter = None |
74 | + name_filter = None |
75 | + if name is not None: |
76 | + query = query.filter(name=name) |
77 | + name_filter = name |
78 | + if rrtype is not None: |
79 | + query = query.filter(dnsdata__rrtype=rrtype) |
80 | + rrtype_filter = rrtype |
81 | + query = query.prefetch_related('ip_addresses', 'dnsdata_set') |
82 | + query = query.select_related('domain') |
83 | + # Note: This must be done last, otherwise our hacks to show additional |
84 | + # records won't work. |
85 | + if all_records is True: |
86 | + query.__class__ = DNSResourcesQuerySet |
87 | + query._name_filter = name_filter |
88 | + query._rrtype_filter = rrtype_filter |
89 | + # Note: the _user_filter should be set to None we want to display |
90 | + # all records, even for non-superusers. |
91 | + query._user_filter = user |
92 | + return query |
93 | + |
94 | + |
95 | class DNSResourcesHandler(OperationsHandler): |
96 | """Manage dnsresources.""" |
97 | api_doc_section_name = "DNSResources" |
98 | @@ -44,33 +119,25 @@ class DNSResourcesHandler(OperationsHandler): |
99 | :param name: restrict the listing to entries of the given name. |
100 | :param rrtype: restrict the listing to entries which have |
101 | records of the given rrtype. |
102 | + :param all: if True, also include implicit DNS records created for |
103 | + nodes registered in MAAS. |
104 | """ |
105 | data = request.GET |
106 | fqdn = data.get('fqdn', None) |
107 | name = data.get('name', None) |
108 | domainname = data.get('domain', None) |
109 | rrtype = data.get('rrtype', None) |
110 | + if rrtype is not None: |
111 | + rrtype = rrtype.upper() |
112 | + _all = get_optional_param( |
113 | + request.GET, 'all', default=False, validator=StringBool) |
114 | if domainname is None and name is None and fqdn is not None: |
115 | # We need a type for resource separation. If the user didn't give |
116 | # us a rrtype, then assume it's an address of some sort. |
117 | (name, domainname) = separate_fqdn(fqdn, rrtype) |
118 | - # If the domain is a name, make it an id. |
119 | - if domainname is not None: |
120 | - if domainname.isdigit(): |
121 | - domain = Domain.objects.get_domain_or_404( |
122 | - domainname, user=request.user, perm=NODE_PERMISSION.VIEW) |
123 | - else: |
124 | - domain = Domain.objects.get_domain_or_404( |
125 | - "name:%s" % domainname, user=request.user, |
126 | - perm=NODE_PERMISSION.VIEW) |
127 | - query = domain.dnsresource_set.all().order_by('name') |
128 | - else: |
129 | - query = DNSResource.objects.all().order_by('domain_id', 'name') |
130 | - if name is not None: |
131 | - query = query.filter(name=name) |
132 | - if rrtype is not None: |
133 | - query = query.filter(dnsdata__rrtype=rrtype) |
134 | - return query |
135 | + user = request.user |
136 | + return get_dnsresource_queryset( |
137 | + _all, domainname, name, rrtype, user) |
138 | |
139 | @admin_method |
140 | def create(self, request): |
141 | @@ -137,12 +204,22 @@ class DNSResourceHandler(OperationsHandler): |
142 | @classmethod |
143 | def ip_addresses(cls, dnsresource): |
144 | """Return IPAddresses within the specified dnsresource.""" |
145 | + rrdata = getattr(dnsresource, '_rrdata', None) |
146 | + if rrdata is not None: |
147 | + return None |
148 | return dnsresource.ip_addresses.all() |
149 | |
150 | @classmethod |
151 | def resource_records(cls, dnsresource): |
152 | """Other data for this dnsresource.""" |
153 | - return dnsresource.dnsdata_set.all().order_by('rrtype') |
154 | + # If the _rrdata field exists in the resource object, this is a |
155 | + # synthetic record created by the DNSResourcesQuerySet, because the |
156 | + # user has chosen to include implicit records. |
157 | + rrdata = getattr(dnsresource, '_rrdata', None) |
158 | + if rrdata is not None: |
159 | + return rrdata |
160 | + else: |
161 | + return dnsresource.dnsdata_set.all().order_by('rrtype') |
162 | |
163 | def read(self, request, id): |
164 | """Read dnsresource. |
165 | diff --git a/src/maasserver/api/tests/test_dnsresources.py b/src/maasserver/api/tests/test_dnsresources.py |
166 | index b6a0f98..ed3aa02 100644 |
167 | --- a/src/maasserver/api/tests/test_dnsresources.py |
168 | +++ b/src/maasserver/api/tests/test_dnsresources.py |
169 | @@ -10,6 +10,7 @@ import json |
170 | import random |
171 | |
172 | from django.conf import settings |
173 | +from maasserver.api.dnsresources import get_dnsresource_queryset |
174 | from maasserver.models.dnsdata import DNSData |
175 | from maasserver.models.dnsresource import DNSResource |
176 | from maasserver.testing.api import APITestCase |
177 | @@ -58,6 +59,27 @@ class TestDNSResourcesAPI(APITestCase.ForUser): |
178 | ] |
179 | self.assertItemsEqual(expected_ids, result_ids) |
180 | |
181 | + def test_read_all(self): |
182 | + self.become_admin() |
183 | + for _ in range(3): |
184 | + factory.make_DNSResource() |
185 | + factory.make_RegionRackController() |
186 | + uri = get_dnsresources_uri() |
187 | + response = self.client.get(uri, {'all': 'true'}) |
188 | + |
189 | + self.assertEqual( |
190 | + http.client.OK, response.status_code, response.content) |
191 | + expected_ids = [ |
192 | + dnsresource.id |
193 | + for dnsresource in get_dnsresource_queryset(all_records=True) |
194 | + ] |
195 | + result_ids = [ |
196 | + dnsresource["id"] |
197 | + for dnsresource in json.loads( |
198 | + response.content.decode(settings.DEFAULT_CHARSET)) |
199 | + ] |
200 | + self.assertItemsEqual(expected_ids, result_ids) |
201 | + |
202 | def test_read_with_domain(self): |
203 | for _ in range(3): |
204 | factory.make_DNSResource() |
205 | @@ -76,6 +98,29 @@ class TestDNSResourcesAPI(APITestCase.ForUser): |
206 | ] |
207 | self.assertItemsEqual(expected_ids, result_ids) |
208 | |
209 | + def test_read_all_with_domain(self): |
210 | + self.become_admin() |
211 | + for _ in range(3): |
212 | + factory.make_DNSResource() |
213 | + dnsrr = DNSResource.objects.first() |
214 | + uri = get_dnsresources_uri() |
215 | + response = self.client.get( |
216 | + uri, |
217 | + { |
218 | + 'all': 'true', |
219 | + 'domain': [dnsrr.domain.name] |
220 | + } |
221 | + ) |
222 | + self.assertEqual( |
223 | + http.client.OK, response.status_code, response.content) |
224 | + expected_ids = [-1, dnsrr.id] |
225 | + result_ids = [ |
226 | + dnsresource["id"] |
227 | + for dnsresource in json.loads( |
228 | + response.content.decode(settings.DEFAULT_CHARSET)) |
229 | + ] |
230 | + self.assertItemsEqual(expected_ids, result_ids) |
231 | + |
232 | def test_read_with_name(self): |
233 | for _ in range(3): |
234 | factory.make_DNSResource() |
235 | diff --git a/src/maasserver/models/domain.py b/src/maasserver/models/domain.py |
236 | index 3751a23..69a3825 100644 |
237 | --- a/src/maasserver/models/domain.py |
238 | +++ b/src/maasserver/models/domain.py |
239 | @@ -12,6 +12,10 @@ __all__ = [ |
240 | "validate_domain_name", |
241 | ] |
242 | |
243 | +from collections import ( |
244 | + defaultdict, |
245 | + OrderedDict, |
246 | +) |
247 | import datetime |
248 | import re |
249 | |
250 | @@ -360,7 +364,9 @@ class Domain(CleanSave, TimestampedModel): |
251 | super(Domain, self).clean(*args, **kwargs) |
252 | self.clean_name() |
253 | |
254 | - def render_json_for_related_rrdata(self, for_list=False): |
255 | + def render_json_for_related_rrdata( |
256 | + self, for_list=False, include_dnsdata=True, as_dict=False, |
257 | + user=None): |
258 | """Render a representation of this domain's related non-IP data, |
259 | suitable for converting to JSON. |
260 | |
261 | @@ -369,13 +375,22 @@ class Domain(CleanSave, TimestampedModel): |
262 | DNSData, |
263 | StaticIPAddress, |
264 | ) |
265 | - rr_mapping = DNSData.objects.get_hostname_dnsdata_mapping( |
266 | - self, raw_ttl=True) |
267 | + if include_dnsdata is True: |
268 | + rr_mapping = DNSData.objects.get_hostname_dnsdata_mapping( |
269 | + self, raw_ttl=True) |
270 | + else: |
271 | + # Circular imports. |
272 | + from maasserver.models.dnsdata import HostnameRRsetMapping |
273 | + rr_mapping = defaultdict(HostnameRRsetMapping) |
274 | # Smash the IP Addresses in the rrset mapping, so that the far end |
275 | # only needs to worry about one thing. |
276 | ip_mapping = StaticIPAddress.objects.get_hostname_ip_mapping( |
277 | self, raw_ttl=True) |
278 | for hostname, info in ip_mapping.items(): |
279 | + if (user is not None and not user.is_superuser and |
280 | + info.user_id is not None and |
281 | + info.user_id != user.id): |
282 | + continue |
283 | entry = rr_mapping[hostname[:-len(self.name) - 1]] |
284 | entry.dnsresource_id = info.dnsresource_id |
285 | if info.system_id is not None: |
286 | @@ -386,20 +401,35 @@ class Domain(CleanSave, TimestampedModel): |
287 | for ip in info.ips: |
288 | record_type = 'AAAA' if IPAddress(ip).version == 6 else 'A' |
289 | entry.rrset.add((info.ttl, record_type, ip, None)) |
290 | - |
291 | - data = [] |
292 | + if as_dict is True: |
293 | + result = OrderedDict() |
294 | + else: |
295 | + result = [] |
296 | for hostname, info in rr_mapping.items(): |
297 | - data += [{ |
298 | - 'name': hostname, |
299 | - 'system_id': info.system_id, |
300 | - 'node_type': info.node_type, |
301 | - 'user_id': info.user_id, |
302 | - 'dnsresource_id': info.dnsresource_id, |
303 | - 'ttl': ttl, |
304 | - 'rrtype': rrtype, |
305 | - 'rrdata': rrdata, |
306 | - 'dnsdata_id': dnsdata_id, |
307 | + data = [ |
308 | + { |
309 | + 'name': hostname, |
310 | + 'system_id': info.system_id, |
311 | + 'node_type': info.node_type, |
312 | + 'user_id': info.user_id, |
313 | + 'dnsresource_id': info.dnsresource_id, |
314 | + 'ttl': ttl, |
315 | + 'rrtype': rrtype, |
316 | + 'rrdata': rrdata, |
317 | + 'dnsdata_id': dnsdata_id, |
318 | } |
319 | for ttl, rrtype, rrdata, dnsdata_id in info.rrset |
320 | - ] |
321 | - return data |
322 | + if (info.user_id is None or |
323 | + user is None or |
324 | + user.is_superuser or |
325 | + (info.user_id is not None and |
326 | + user is not None and |
327 | + info.user_id == user.id)) |
328 | + ] |
329 | + if as_dict is True: |
330 | + existing = result.get(hostname, []) |
331 | + existing.extend(data) |
332 | + result[hostname] = existing |
333 | + else: |
334 | + result.extend(data) |
335 | + return result |
336 | diff --git a/src/maasserver/models/tests/test_domain.py b/src/maasserver/models/tests/test_domain.py |
337 | index f2a7f77..054d082 100644 |
338 | --- a/src/maasserver/models/tests/test_domain.py |
339 | +++ b/src/maasserver/models/tests/test_domain.py |
340 | @@ -32,7 +32,11 @@ from maasserver.models.staticipaddress import StaticIPAddress |
341 | from maasserver.testing.factory import factory |
342 | from maasserver.testing.testcase import MAASServerTestCase |
343 | from netaddr import IPAddress |
344 | -from testtools.matchers import MatchesStructure |
345 | +from testtools.matchers import ( |
346 | + Equals, |
347 | + HasLength, |
348 | + MatchesStructure, |
349 | +) |
350 | from testtools.testcase import ExpectedException |
351 | |
352 | |
353 | @@ -372,6 +376,9 @@ class DomainTest(MAASServerTestCase): |
354 | dnsresource__domain_id=domain.id) |
355 | self.assertEqual("0 0 1688 %s." % target, srvrr.rrdata) |
356 | |
357 | + |
358 | +class TestRenderRRData(MAASServerTestCase): |
359 | + |
360 | def render_rrdata(self, domain, for_list=False): |
361 | rr_map = DNSData.objects.get_hostname_dnsdata_mapping( |
362 | domain, raw_ttl=True) |
363 | @@ -434,3 +441,18 @@ class DomainTest(MAASServerTestCase): |
364 | self.assertEqual(actual, expected) |
365 | for record in actual: |
366 | self.assertEqual(record['user_id'], user.id) |
367 | + |
368 | + def test_renders_as_dictionary(self): |
369 | + domain = factory.make_Domain() |
370 | + name1 = factory.make_name(prefix='a') |
371 | + name2 = factory.make_name(prefix='b') |
372 | + factory.make_DNSData(name=name1, domain=domain, rrtype='MX') |
373 | + rrdata_list = domain.render_json_for_related_rrdata(as_dict=False) |
374 | + rrdata_dict = domain.render_json_for_related_rrdata(as_dict=True) |
375 | + self.assertThat(rrdata_dict[name1], Equals([rrdata_list[0]])) |
376 | + self.assertThat(rrdata_dict[name1], HasLength(1)) |
377 | + factory.make_DNSData(name=name1, domain=domain, rrtype='MX') |
378 | + factory.make_DNSData(name=name2, domain=domain, rrtype='NS') |
379 | + rrdata_dict = domain.render_json_for_related_rrdata(as_dict=True) |
380 | + self.assertThat(rrdata_dict[name1], HasLength(2)) |
381 | + self.assertThat(rrdata_dict[name2], HasLength(1)) |
382 | diff --git a/src/maasserver/testing/factory.py b/src/maasserver/testing/factory.py |
383 | index a8a6498..c79ef07 100644 |
384 | --- a/src/maasserver/testing/factory.py |
385 | +++ b/src/maasserver/testing/factory.py |
386 | @@ -721,7 +721,7 @@ class Factory(maastesting.factory.Factory): |
387 | else ip |
388 | for ip in ip_addresses |
389 | ] |
390 | - dnsrr = DNSResource( |
391 | + dnsrr, _ = DNSResource.objects.get_or_create( |
392 | name=name, address_ttl=address_ttl, |
393 | domain=domain) |
394 | dnsrr.save() |
395 | diff --git a/src/maasserver/websockets/handlers/domain.py b/src/maasserver/websockets/handlers/domain.py |
396 | index 50c435c..a9a7c30 100644 |
397 | --- a/src/maasserver/websockets/handlers/domain.py |
398 | +++ b/src/maasserver/websockets/handlers/domain.py |
399 | @@ -56,10 +56,8 @@ class DomainHandler(TimestampedModelHandler, AdminOnlyMixin): |
400 | ] |
401 | |
402 | def dehydrate(self, domain, data, for_list=False): |
403 | - rrsets = domain.render_json_for_related_rrdata(for_list=for_list) |
404 | - if not self.user.is_superuser: |
405 | - rrsets = [ |
406 | - rrset for rrset in rrsets if rrset['user_id'] == self.user.id] |
407 | + rrsets = domain.render_json_for_related_rrdata( |
408 | + for_list=for_list, user=self.user) |
409 | if not for_list: |
410 | data["rrsets"] = rrsets |
411 | data["hosts"] = len({ |
UNIT TESTS ui-consistency- -bug-1686864 lp:~mpontillo/maas/+git/maas into -b master lp:~maas-committers/maas
-b dns-api-
STATUS: SUCCESS a7004428e2c9c22 f0776c07fa
COMMIT: 6a1207b47b598a5