Merge lp:~blake-rouse/maas/fix-1580260 into lp:~maas-committers/maas/trunk

[Blake Rouse]

Allow standard users to create reserved IP ranges and update/delete their own ranges.

[Andres Rodriguez (andreserl)]

lgtm! Please improve the messages before landing.

review: Approve

=== modified file 'src/maasserver/api/ipranges.py'
--- src/maasserver/api/ipranges.py	2016-03-28 13:54:47 +0000
+++ src/maasserver/api/ipranges.py	2016-05-13 21:37:01 +0000
@@ -3,11 +3,12 @@
 
 """API handlers: `ip-ranges`."""
 
-from maasserver.api.support import (
-    admin_method,
-    OperationsHandler,
+from maasserver.api.support import OperationsHandler
+from maasserver.enum import IPRANGE_TYPE
+from maasserver.exceptions import (
+    MAASAPIForbidden,
+    MAASAPIValidationError,
 )
-from maasserver.exceptions import MAASAPIValidationError
 from maasserver.forms_iprange import IPRangeForm
 from maasserver.models import IPRange
 from piston3.utils import rc
@@ -24,6 +25,12 @@
 )
 
 
+def raise_error_if_not_owner(iprange, user):
+    if not user.is_superuser and iprange.user_id != user.id:
+        raise MAASAPIForbidden(
+            "Unable to modify IP range. You don't own the IP range.")
+
+
 class IPRangesHandler(OperationsHandler):
     """Manage IP ranges."""
     api_doc_section_name = "IP Ranges"
@@ -39,7 +46,6 @@
         """List all IP ranges."""
         return IPRange.objects.all()
 
-    @admin_method
     def create(self, request):
         """Create an IP range.
 
@@ -47,7 +53,16 @@
         :param start_ip: Start IP address of this range (inclusive).
         :param end_ip: End IP address of this range (inclusive).
         :param subnet: Subnet this range is associated with. (optional)
+
+        Returns 403 if standard users tries to create a dynamic IP range.
         """
+        if ('type' in request.data and
+                request.data['type'] == IPRANGE_TYPE.DYNAMIC and
+                not request.user.is_superuser):
+            raise MAASAPIForbidden(
+                "Unable to create dynamic IP range. "
+                "You don't have sufficient privileges.")
+
         form = IPRangeForm(data=request.data, request=request)
         if form.is_valid():
             return form.save()
@@ -84,9 +99,11 @@
         :param start_ip: Start IP address of this range (inclusive).
         :param end_ip: End IP address of this range (inclusive).
 
+        Returns 403 if not owner of IP range.
         Returns 404 if the IP Range is not found.
         """
         iprange = IPRange.objects.get_iprange_or_404(iprange_id)
+        raise_error_if_not_owner(iprange, request.user)
         form = IPRangeForm(instance=iprange, data=request.data)
         if form.is_valid():
             return form.save()
@@ -96,8 +113,10 @@
     def delete(self, request, iprange_id):
         """Delete IP range.
 
+        Returns 403 if not owner of IP range.
         Returns 404 if the IP range is not found.
         """
         iprange = IPRange.objects.get_iprange_or_404(iprange_id)
+        raise_error_if_not_owner(iprange, request.user)
         iprange.delete()
         return rc.DELETED

=== modified file 'src/maasserver/api/tests/test_ipranges.py'
--- src/maasserver/api/tests/test_ipranges.py	2016-04-26 19:52:34 +0000
+++ src/maasserver/api/tests/test_ipranges.py	2016-05-13 21:37:01 +0000
@@ -59,7 +59,7 @@
             ]
         self.assertItemsEqual(expected_ids, result_ids)
 
-    def test_create(self):
+    def test_create_dynamic(self):
         self.become_admin()
         uri = get_ipranges_uri()
         subnet = factory.make_Subnet(cidr='10.0.0.0/24')
@@ -76,12 +76,23 @@
         self.assertThat(data['end_ip'], Equals('10.0.0.20'))
         self.assertThat(data['subnet']['id'], Equals(subnet.id))
 
+    def test_create_dynamic_requires_admin(self):
+        uri = get_ipranges_uri()
+        subnet = factory.make_Subnet(cidr='10.0.0.0/24')
+        response = self.client.post(uri, {
+            "type": "dynamic",
+            "start_ip": "10.0.0.10",
+            "end_ip": "10.0.0.20",
+            "subnet": "%d" % subnet.id,
+        })
+        self.assertEqual(
+            http.client.FORBIDDEN, response.status_code, response.content)
+
     def test_create_does_not_require_subnet(self):
-        self.become_admin()
         uri = get_ipranges_uri()
         subnet = factory.make_Subnet(cidr='10.0.0.0/24')
         response = self.client.post(uri, {
-            "type": "dynamic",
+            "type": "reserved",
             "start_ip": "10.0.0.10",
             "end_ip": "10.0.0.20",
         })
@@ -93,7 +104,6 @@
         self.assertThat(data['subnet']['id'], Equals(subnet.id))
 
     def test_create_requires_type_and_reports_simple_error_if_missing(self):
-        self.become_admin()
         uri = get_ipranges_uri()
         factory.make_Subnet(cidr='10.0.0.0/24')
         response = self.client.post(uri, {
@@ -105,6 +115,20 @@
         self.assertThat(response.content, Equals(
             b'{"type": ["This field is required."]}'))
 
+    def test_create_sets_user_to_authenticated_user(self):
+        uri = get_ipranges_uri()
+        factory.make_Subnet(cidr='10.0.0.0/24')
+        response = self.client.post(uri, {
+            "type": "reserved",
+            "start_ip": "10.0.0.10",
+            "end_ip": "10.0.0.20",
+        })
+        self.assertEqual(
+            http.client.OK, response.status_code, response.content)
+        data = json.loads(response.content.decode(settings.DEFAULT_CHARSET))
+        self.assertThat(
+            data['user']['username'], Equals(self.logged_in_user.username))
+
 
 class TestIPRangeAPI(APITestCase):
 
@@ -144,7 +168,8 @@
 
     def test_update(self):
         subnet = factory.make_Subnet(cidr="10.0.0.0/24")
-        iprange = factory.make_IPRange(subnet, '10.0.0.2', '10.0.0.10')
+        iprange = factory.make_IPRange(
+            subnet, '10.0.0.2', '10.0.0.10', user=self.logged_in_user)
         uri = get_iprange_uri(iprange)
         comment = factory.make_name("comment")
         response = self.client.put(uri, {
@@ -158,15 +183,42 @@
                 response.content.decode(settings.DEFAULT_CHARSET))['comment'])
         self.assertEqual(comment, reload_object(iprange).comment)
 
+    def test_update_403_when_not_user(self):
+        subnet = factory.make_Subnet(cidr="10.0.0.0/24")
+        iprange = factory.make_IPRange(
+            subnet, '10.0.0.2', '10.0.0.10', user=factory.make_User())
+        uri = get_iprange_uri(iprange)
+        response = self.client.put(uri, {})
+        self.assertEqual(
+            http.client.FORBIDDEN, response.status_code, response.content)
+
+    def test_update_404_when_invalid_id(self):
+        uri = reverse(
+            'iprange_handler', args=[random.randint(100, 1000)])
+        response = self.client.put(uri, {})
+        self.assertEqual(
+            http.client.NOT_FOUND, response.status_code, response.content)
+
     def test_delete_deletes_iprange(self):
         subnet = factory.make_Subnet(cidr="10.0.0.0/24")
-        iprange = factory.make_IPRange(subnet, '10.0.0.2', '10.0.0.10')
+        iprange = factory.make_IPRange(
+            subnet, '10.0.0.2', '10.0.0.10', user=self.logged_in_user)
         uri = get_iprange_uri(iprange)
         response = self.client.delete(uri)
         self.assertEqual(
             http.client.NO_CONTENT, response.status_code, response.content)
         self.assertIsNone(reload_object(iprange))
 
+    def test_delete_403_when_not_user(self):
+        subnet = factory.make_Subnet(cidr="10.0.0.0/24")
+        iprange = factory.make_IPRange(
+            subnet, '10.0.0.2', '10.0.0.10', user=factory.make_User())
+        uri = get_iprange_uri(iprange)
+        response = self.client.delete(uri)
+        self.assertEqual(
+            http.client.FORBIDDEN, response.status_code, response.content)
+        self.assertIsNotNone(reload_object(iprange))
+
     def test_delete_404_when_invalid_id(self):
         uri = reverse(
             'iprange_handler', args=[random.randint(100, 1000)])