Merge lp:~trapnine/maas/b1459762 into lp:~maas-committers/maas/trunk

[Jeffrey C Jones]

API requires user to be admin to PUT any update to Node
fixes bug: https://launchpad.net/bugs/1459762

[Jeffrey C Jones (trapnine)]

Please consider if this new behavior makes sense for the MAAS API.

All Node Update PUTs via the API will REQUIRE an admin user, or you get a 403 FORBIDDEN. The tests continue to pass after making the requesting user an admin.

I fixed a couple doc typos as well.

[Gavin Panella (allenap)]

I get the feeling that access to a node is or should be more nuanced.

* A unowned node can only be updated by an admin, with one exception
  (IIRC): it can be acquired from the Ready state by a non-admin.

* An owned node can be modified by its owner. However, some parameters
  may only be modifiable by an owner who is also an admin.

This branch seems to prevent non-admins from modifying anything about a
node, and I'm not sure that's what we want.

review: Needs Information

[Blake Rouse (blake-rouse)]

@Gavin
I don't believe there is a single attribute that can be changed by a standard user when it is acquired. When you need to set the distro_series or something else those are exposed on other endpoints. The attributes that can be changed with this endpoint really are administrator only. Back when it was used to set the distro_series and other attributes it was different, this just has not been updated to fit the new flow of MAAS.

review: Approve

[Gavin Panella (allenap)]

Thanks Blake.

review: Approve

[MAAS Lander (maas-lander)]

The attempt to merge lp:~trapnine/maas/b1459762 into lp:maas failed. Below is the output from the failed tests.

Ign http://security.ubuntu.com trusty-security InRelease
Get:1 http://security.ubuntu.com trusty-security Release.gpg [933 B]
Get:2 http://security.ubuntu.com trusty-security Release [63.5 kB]
Get:3 http://security.ubuntu.com trusty-security/main Sources [94.6 kB]
Get:4 http://security.ubuntu.com trusty-security/universe Sources [30.6 kB]
Get:5 http://security.ubuntu.com trusty-security/main amd64 Packages [344 kB]
Get:6 http://security.ubuntu.com trusty-security/universe amd64 Packages [116 kB]
Hit http://security.ubuntu.com trusty-security/main Translation-en
Hit http://security.ubuntu.com trusty-security/universe Translation-en
Ign http://nova.clouds.archive.ubuntu.com trusty InRelease
Ign http://nova.clouds.archive.ubuntu.com trusty-updates InRelease
Hit http://nova.clouds.archive.ubuntu.com trusty Release.gpg
Get:7 http://nova.clouds.archive.ubuntu.com trusty-updates Release.gpg [933 B]
Hit http://nova.clouds.archive.ubuntu.com trusty Release
Get:8 http://nova.clouds.archive.ubuntu.com trusty-updates Release [63.5 kB]
Hit http://nova.clouds.archive.ubuntu.com trusty/main Sources
Hit http://nova.clouds.archive.ubuntu.com trusty/universe Sources
Hit http://nova.clouds.archive.ubuntu.com trusty/main amd64 Packages
Hit http://nova.clouds.archive.ubuntu.com trusty/universe amd64 Packages
Hit http://nova.clouds.archive.ubuntu.com trusty/main Translation-en
Hit http://nova.clouds.archive.ubuntu.com trusty/universe Translation-en
Get:9 http://nova.clouds.archive.ubuntu.com trusty-updates/main Sources [234 kB]
Get:10 http://nova.clouds.archive.ubuntu.com trusty-updates/universe Sources [136 kB]
Get:11 http://nova.clouds.archive.ubuntu.com trusty-updates/main amd64 Packages [619 kB]
Get:12 http://nova.clouds.archive.ubuntu.com trusty-updates/universe amd64 Packages [313 kB]
Hit http://nova.clouds.archive.ubuntu.com trusty-updates/main Translation-en
Hit http://nova.clouds.archive.ubuntu.com trusty-updates/universe Translation-en
Ign http://nova.clouds.archive.ubuntu.com trusty/main Translation-en_US
Ign http://nova.clouds.archive.ubuntu.com trusty/universe Translation-en_US
Fetched 2,015 kB in 3s (565 kB/s)
Reading package lists...
sudo DEBIAN_FRONTEND=noninteractive apt-get -y \
     --no-install-recommends install apache2 authbind bind9 bind9utils build-essential bzr-builddeb chromium-browser chromium-chromedriver curl daemontools debhelper dh-apport dh-systemd distro-info dnsutils firefox freeipmi-tools git gjs ipython isc-dhcp-common libjs-angularjs libjs-jquery libjs-jquery-hotkeys libjs-yui3-full libjs-yui3-min libpq-dev make nodejs-legacy npm pep8 phantomjs postgresql pyflakes python-apt python-bson python-bzrlib python-convoy python-coverage python-crochet python-cssselect python-curtin python-dev python-distro-info python-django python-django-piston python-django-south python-djorm-ext-pgarray python-docutils python-extras python-fixtures python-flake8 python-formencode python-hivex python-httplib2 python-jinja2 python-jsonschema python-lxml python-mock python-netaddr python-netifaces python-nose python-oauth python-openssl pyt...

[MAAS Lander (maas-lander)]

The attempt to merge lp:~trapnine/maas/b1459762 into lp:maas failed. Below is the output from the failed tests.

Ign http://security.ubuntu.com trusty-security InRelease
Get:1 http://security.ubuntu.com trusty-security Release.gpg [933 B]
Ign http://nova.clouds.archive.ubuntu.com trusty InRelease
Ign http://nova.clouds.archive.ubuntu.com trusty-updates InRelease
Get:2 http://security.ubuntu.com trusty-security Release [63.5 kB]
Hit http://nova.clouds.archive.ubuntu.com trusty Release.gpg
Get:3 http://nova.clouds.archive.ubuntu.com trusty-updates Release.gpg [933 B]
Hit http://nova.clouds.archive.ubuntu.com trusty Release
Get:4 http://nova.clouds.archive.ubuntu.com trusty-updates Release [63.5 kB]
Get:5 http://security.ubuntu.com trusty-security/main Sources [95.8 kB]
Hit http://nova.clouds.archive.ubuntu.com trusty/main Sources
Get:6 http://security.ubuntu.com trusty-security/universe Sources [30.6 kB]
Hit http://nova.clouds.archive.ubuntu.com trusty/universe Sources
Hit http://nova.clouds.archive.ubuntu.com trusty/main amd64 Packages
Get:7 http://security.ubuntu.com trusty-security/main amd64 Packages [344 kB]
Hit http://nova.clouds.archive.ubuntu.com trusty/universe amd64 Packages
Hit http://nova.clouds.archive.ubuntu.com trusty/main Translation-en
Hit http://nova.clouds.archive.ubuntu.com trusty/universe Translation-en
Get:8 http://nova.clouds.archive.ubuntu.com trusty-updates/main Sources [235 kB]
Get:9 http://nova.clouds.archive.ubuntu.com trusty-updates/universe Sources [136 kB]
Get:10 http://security.ubuntu.com trusty-security/universe amd64 Packages [116 kB]
Get:11 http://nova.clouds.archive.ubuntu.com trusty-updates/main amd64 Packages [620 kB]
Get:12 http://security.ubuntu.com trusty-security/main Translation-en [187 kB]
Get:13 http://security.ubuntu.com trusty-security/universe Translation-en [67.8 kB]
Get:14 http://nova.clouds.archive.ubuntu.com trusty-updates/universe amd64 Packages [312 kB]
Get:15 http://nova.clouds.archive.ubuntu.com trusty-updates/main Translation-en [300 kB]
Get:16 http://nova.clouds.archive.ubuntu.com trusty-updates/universe Translation-en [166 kB]
Ign http://nova.clouds.archive.ubuntu.com trusty/main Translation-en_US
Ign http://nova.clouds.archive.ubuntu.com trusty/universe Translation-en_US
Fetched 2,739 kB in 3s (781 kB/s)
Reading package lists...
sudo DEBIAN_FRONTEND=noninteractive apt-get -y \
     --no-install-recommends install apache2 authbind bind9 bind9utils build-essential bzr-builddeb chromium-browser chromium-chromedriver curl daemontools debhelper dh-apport dh-systemd distro-info dnsutils firefox freeipmi-tools git gjs ipython isc-dhcp-common libjs-angularjs libjs-jquery libjs-jquery-hotkeys libjs-yui3-full libjs-yui3-min libpq-dev make nodejs-legacy npm pep8 phantomjs postgresql pyflakes python-apt python-bson python-bzrlib python-convoy python-coverage python-crochet python-cssselect python-curtin python-dev python-distro-info python-django python-django-piston python-django-south python-djorm-ext-pgarray python-docutils python-extras python-fixtures python-flake8 python-formencode python-hivex python-httplib2 python-jinja2 python-jsonschema python-lxml python-mock python-netaddr python-neti...

=== modified file 'src/maasserver/api/nodes.py'
--- src/maasserver/api/nodes.py	2015-09-18 17:33:48 +0000
+++ src/maasserver/api/nodes.py	2015-09-23 01:02:15 +0000
@@ -309,6 +309,7 @@
         return Node.nodes.get_node_or_404(
             system_id=system_id, user=request.user, perm=NODE_PERMISSION.VIEW)
 
+    @admin_method
     def update(self, request, system_id):
         """Update a specific Node.
 
@@ -442,7 +443,7 @@
         encoding instead.
 
         Returns 404 if the node is not found.
-        Returns 403 if the user does not have permission to stop the node.
+        Returns 403 if the user does not have permission to start the node.
         Returns 503 if the start-up attempted to allocate an IP address,
         and there were no IP addresses available on the relevant cluster
         interface.
@@ -702,7 +703,7 @@
 
         Returns 404 if the node is not found.
         Returns 403 if the user does not have permission to mark the node
-        broken.
+        fixed.
         """
         comment = get_optional_param(request.POST, 'comment')
         node = Node.nodes.get_node_or_404(
@@ -844,8 +845,8 @@
 
         Returns 400 if the node is currently not allocated.
         Returns 404 if the node could not be found.
-        Returns 403 if the user does not have permission to abort the
-        current operation.
+        Returns 403 if the user does not have permission to set the storage
+        layout.
         """
         node = Node.nodes.get_node_or_404(
             system_id=system_id, user=request.user, perm=NODE_PERMISSION.EDIT)

=== modified file 'src/maasserver/api/tests/test_node.py'
--- src/maasserver/api/tests/test_node.py	2015-09-18 17:33:48 +0000
+++ src/maasserver/api/tests/test_node.py	2015-09-23 01:02:15 +0000
@@ -865,6 +865,7 @@
         self.assertTrue(node.skip_networking)
 
     def test_PUT_updates_node(self):
+        self.become_admin()
         # The api allows the updating of a Node.
         node = factory.make_Node(
             hostname='diane', owner=self.logged_in_user,
@@ -882,6 +883,7 @@
         self.assertEqual(1, Node.objects.filter(hostname='francis').count())
 
     def test_PUT_omitted_hostname(self):
+        self.become_admin()
         hostname = factory.make_name('hostname')
         arch = make_usable_architecture(self)
         node = factory.make_Node(
@@ -893,6 +895,7 @@
         self.assertTrue(Node.objects.filter(hostname=hostname).exists())
 
     def test_PUT_rejects_device(self):
+        self.become_admin()
         node = factory.make_Node(
             installable=False, owner=self.logged_in_user)
         response = self.client.put(self.get_node_uri(node))
@@ -900,6 +903,7 @@
             httplib.NOT_FOUND, response.status_code, response.content)
 
     def test_PUT_ignores_unknown_fields(self):
+        self.become_admin()
         node = factory.make_Node(
             owner=self.logged_in_user,
             architecture=make_usable_architecture(self))
@@ -919,11 +923,10 @@
             owner=self.logged_in_user,
             power_type=original_power_type,
             architecture=make_usable_architecture(self))
-        self.client.put(
-            self.get_node_uri(node),
-            {'power_type': new_power_type}
-        )
+        response = self.client.put(
+            self.get_node_uri(node), {'power_type': new_power_type})
 
+        self.assertEqual(httplib.OK, response.status_code)
         self.assertEqual(
             new_power_type, reload_object(node).power_type)
 
@@ -932,15 +935,15 @@
         new_power_type = factory.pick_power_type(but_not=original_power_type)
         node = factory.make_Node(
             owner=self.logged_in_user, power_type=original_power_type)
-        self.client.put(
-            self.get_node_uri(node),
-            {'power_type': new_power_type}
-        )
+        response = self.client.put(
+            self.get_node_uri(node), {'power_type': new_power_type})
 
+        self.assertEqual(httplib.FORBIDDEN, response.status_code)
         self.assertEqual(
             original_power_type, reload_object(node).power_type)
 
     def test_resource_uri_points_back_at_node(self):
+        self.become_admin()
         # When a Node is returned by the API, the field 'resource_uri'
         # provides the URI for this Node.
         node = factory.make_Node(
@@ -950,6 +953,7 @@
             self.get_node_uri(node), {'hostname': 'francis'})
         parsed_result = json.loads(response.content)
 
+        self.assertEqual(httplib.OK, response.status_code)
         self.assertEqual(
             reverse('node_handler', args=[parsed_result['system_id']]),
             parsed_result['resource_uri'])
@@ -957,6 +961,7 @@
     def test_PUT_rejects_invalid_data(self):
         # If the data provided to update a node is invalid, a 'Bad request'
         # response is returned.
+        self.become_admin()
         node = factory.make_Node(
             hostname='diane', owner=self.logged_in_user,
             architecture=make_usable_architecture(self))
@@ -969,19 +974,10 @@
             {'hostname': ["DNS name contains an empty label."]},
             parsed_result)
 
-    def test_PUT_refuses_to_update_invisible_node(self):
-        # The request to update a single node is denied if the node isn't
-        # visible by the user.
-        other_node = factory.make_Node(
-            status=NODE_STATUS.ALLOCATED, owner=factory.make_User())
-
-        response = self.client.put(self.get_node_uri(other_node))
-
-        self.assertEqual(httplib.FORBIDDEN, response.status_code)
-
     def test_PUT_refuses_to_update_nonexistent_node(self):
         # When updating a Node, the api returns a 'Not Found' (404) error
         # if no node is found.
+        self.become_admin()
         url = reverse('node_handler', args=['invalid-uuid'])
         response = self.client.put(url)
 
@@ -1218,6 +1214,14 @@
         node = reload_object(node)
         self.assertEqual(zone, node.zone)
 
+    def test_PUT_requires_admin(self):
+        node = factory.make_Node(
+            owner=self.logged_in_user,
+            architecture=make_usable_architecture(self))
+        # PUT the node with no arguments - should get FORBIDDEN
+        response = self.client.put(self.get_node_uri(node), {})
+        self.assertEqual(httplib.FORBIDDEN, response.status_code)
+
     def test_PUT_zone_change_requires_admin(self):
         new_zone = factory.make_Zone()
         node = factory.make_Node(
@@ -1229,14 +1233,13 @@
             self.get_node_uri(node),
             {'zone': new_zone.name})
 
-        # Awkwardly, the request succeeds because for non-admins, "zone" is
-        # an unknown parameter.  Unknown parameters are ignored.
-        self.assertEqual(httplib.OK, response.status_code)
-        # The node's physical zone, however, has not been updated.
+        self.assertEqual(httplib.FORBIDDEN, response.status_code)
+        # Confirm the node's physical zone has not been updated.
         node = reload_object(node)
         self.assertEqual(old_zone, node.zone)
 
     def test_PUT_sets_disable_ipv4(self):
+        self.become_admin()
         original_setting = factory.pick_bool()
         node = factory.make_Node(
             owner=self.logged_in_user,
@@ -1252,6 +1255,7 @@
         self.assertEqual(new_setting, node.disable_ipv4)
 
     def test_PUT_leaves_disable_ipv4_unchanged_by_default(self):
+        self.become_admin()
         original_setting = factory.pick_bool()
         node = factory.make_Node(
             owner=self.logged_in_user,
@@ -1267,6 +1271,7 @@
         self.assertEqual(original_setting, node.disable_ipv4)
 
     def test_PUT_updates_boot_type(self):
+        self.become_admin()
         node = factory.make_Node(
             owner=self.logged_in_user,
             architecture=make_usable_architecture(self),
@@ -1282,6 +1287,7 @@
         self.assertEqual(node.boot_type, NODE_BOOT.DEBIAN)
 
     def test_PUT_updates_swap_size(self):
+        self.become_admin()
         node = factory.make_Node(owner=self.logged_in_user,
                                  architecture=make_usable_architecture(self))
         response = self.client.put(
@@ -1293,6 +1299,7 @@
         self.assertEqual(node.swap_size, parsed_result['swap_size'])
 
     def test_PUT_updates_swap_size_suffixes(self):
+        self.become_admin()
         node = factory.make_Node(owner=self.logged_in_user,
                                  architecture=make_usable_architecture(self))
 
@@ -1325,6 +1332,7 @@
         self.assertEqual(5000000000000, parsed_result['swap_size'])
 
     def test_PUT_updates_swap_size_invalid_suffix(self):
+        self.become_admin()
         node = factory.make_Node(owner=self.logged_in_user,
                                  architecture=make_usable_architecture(self))
         response = self.client.put(

=== modified file 'src/maasserver/api/tests/test_nodes.py'
--- src/maasserver/api/tests/test_nodes.py	2015-09-15 23:19:40 +0000
+++ src/maasserver/api/tests/test_nodes.py	2015-09-23 01:02:15 +0000
@@ -1847,14 +1847,33 @@
         self.assertEqual(self.old_allocated_status, parsed_result['status'])
 
     def test_PUT_updates_node_folds_status(self):
-        node = factory.make_Node(
-            owner=self.logged_in_user, status=self.status,
-            architecture=make_usable_architecture(self))
-        response = self.client.put(
-            self.get_node_uri(node), {'hostname': factory.make_name('host')})
-        parsed_result = json.loads(response.content)
-
-        self.assertEqual(httplib.OK, response.status_code)
+        self.become_admin()
+        node = factory.make_Node(
+            owner=self.logged_in_user, status=self.status,
+            architecture=make_usable_architecture(self))
+        response = self.client.put(
+            self.get_node_uri(node), {'hostname': factory.make_name('host')})
+        parsed_result = json.loads(response.content)
+
+        self.assertEqual(httplib.OK, response.status_code)
+        self.assertEqual(self.old_allocated_status, parsed_result['status'])
+
+    def test_PUT_update_forbidden_if_not_admin(self):
+        node = factory.make_Node(
+            owner=self.logged_in_user, status=self.status,
+            architecture=make_usable_architecture(self))
+
+        # should get FORBIDDEN because user isn't admin
+        response = self.client.put(
+            self.get_node_uri(node), {'hostname': factory.make_name('host')})
+        self.assertEqual(httplib.FORBIDDEN, response.status_code)
+
+        # confirm operation succeeds as admin
+        self.become_admin()
+        response = self.client.put(
+            self.get_node_uri(node), {'hostname': factory.make_name('host')})
+        self.assertEqual(httplib.OK, response.status_code)
+        parsed_result = json.loads(response.content)
         self.assertEqual(self.old_allocated_status, parsed_result['status'])
 
     def test_GET_list_allocated_exposes_substatus(self):
@@ -1882,6 +1901,7 @@
         self.assertEqual(self.status, parsed_result['substatus'])
 
     def test_PUT_updates_exposes_substatus(self):
+        self.become_admin()
         node = factory.make_Node(
             owner=self.logged_in_user, status=self.status,
             architecture=make_usable_architecture(self))