Merge lp:~trapnine/maas/fix-1576417-controllers-admin-only into lp:~maas-committers/maas/trunk

[Jeffrey C Jones]

Non-admins can't see controllers.

Non-admins can't see controllers.

[Gavin Panella (allenap)]

Looks good, except that isSuperUser needs a simple test.

review: Approve

[Jeffrey C Jones (trapnine)]

Approved by Gavin.

review: Approve

=== modified file 'src/maasserver/api/rackcontrollers.py'
--- src/maasserver/api/rackcontrollers.py	2016-04-28 01:11:50 +0000
+++ src/maasserver/api/rackcontrollers.py	2016-05-16 16:20:02 +0000
@@ -154,6 +154,7 @@
             "Import of boot images started on %s" % rack.hostname,
             content_type=("text/plain; charset=%s" % settings.DEFAULT_CHARSET))
 
+    @admin_method
     @operation(idempotent=True)
     def list_boot_images(self, request, system_id):
         """List all available boot images.
@@ -192,6 +193,7 @@
             "Import of boot images started on all rack controllers",
             content_type=("text/plain; charset=%s" % settings.DEFAULT_CHARSET))
 
+    @admin_method
     @operation(idempotent=True)
     def describe_power_types(self, request):
         """Query all of the rack controllers for power information.

=== modified file 'src/maasserver/api/tests/test_events.py'
--- src/maasserver/api/tests/test_events.py	2016-02-02 14:20:45 +0000
+++ src/maasserver/api/tests/test_events.py	2016-05-16 16:20:02 +0000
@@ -143,6 +143,7 @@
         # Even when node ids are passed to "list," events for nodes are
         # returned in event id order, not necessarily in the order of the
         # node id arguments.
+        self.become_admin()
         nodes = [factory.make_Node() for _ in range(3)]
         events = [factory.make_Event(node=node) for node in nodes]
         response = self.client.get(
@@ -310,6 +311,7 @@
         self.assertEqual(parsed_result['count'], len(events))
 
     def test_GET_query_doesnt_list_devices(self):
+        self.become_admin()
         machines = [
             factory.make_Node(
                 agent_name=factory.make_name('agent-name'),

=== modified file 'src/maasserver/api/tests/test_rackcontroller.py'
--- src/maasserver/api/tests/test_rackcontroller.py	2016-04-15 22:14:33 +0000
+++ src/maasserver/api/tests/test_rackcontroller.py	2016-05-16 16:20:02 +0000
@@ -6,6 +6,7 @@
 import http.client
 
 from django.core.urlresolvers import reverse
+from maasserver.api import rackcontrollers
 from maasserver.models import node as node_module
 from maasserver.testing.api import (
     APITestCase,
@@ -17,6 +18,7 @@
 from maastesting.matchers import (
     MockCalledOnce,
     MockCalledOnceWith,
+    MockNotCalled,
 )
 
 
@@ -87,6 +89,7 @@
 
     def test_GET_list_boot_images(self):
         rack = factory.make_RackController(owner=factory.make_User())
+        self.become_admin()
         response = self.client.get(
             self.get_rack_uri(rack), {'op': 'list_boot_images'})
         self.assertEqual(
@@ -96,6 +99,14 @@
             ['connected', 'images', 'status'],
             json_load_bytes(response.content))
 
+    def test_GET_list_boot_images_denied_if_not_admin(self):
+        rack = factory.make_RackController(owner=factory.make_User())
+        response = self.client.get(
+            self.get_rack_uri(rack), {'op': 'list_boot_images'})
+        self.assertEqual(
+            http.client.FORBIDDEN, response.status_code,
+            explain_unexpected_response(http.client.FORBIDDEN, response))
+
 
 class TestRackControllersAPI(APITestCase):
     """Tests for /api/2.0/rackcontrollers/."""
@@ -110,6 +121,7 @@
             '/api/2.0/rackcontrollers/', reverse('rackcontrollers_handler'))
 
     def test_read_returns_limited_fields(self):
+        self.become_admin()
         factory.make_RackController(owner=self.logged_in_user)
         response = self.client.get(reverse('rackcontrollers_handler'))
         parsed_result = json_load_bytes(response.content)
@@ -159,3 +171,24 @@
         self.assertEqual(
             http.client.FORBIDDEN, response.status_code,
             explain_unexpected_response(http.client.FORBIDDEN, response))
+
+    def test_GET_describe_power_types(self):
+        get_all_power_types_from_clusters = self.patch(
+            rackcontrollers, "get_all_power_types_from_clusters")
+        self.become_admin()
+        response = self.client.get(
+            self.get_rack_uri(), {'op': 'describe_power_types'})
+        self.assertEqual(
+            http.client.OK, response.status_code,
+            explain_unexpected_response(http.client.OK, response))
+        self.assertThat(get_all_power_types_from_clusters, MockCalledOnce())
+
+    def test_GET_describe_power_types_denied_if_not_admin(self):
+        get_all_power_types_from_clusters = self.patch(
+            rackcontrollers, "get_all_power_types_from_clusters")
+        response = self.client.get(
+            self.get_rack_uri(), {'op': 'describe_power_types'})
+        self.assertEqual(
+            http.client.FORBIDDEN, response.status_code,
+            explain_unexpected_response(http.client.FORBIDDEN, response))
+        self.assertThat(get_all_power_types_from_clusters, MockNotCalled())

=== modified file 'src/maasserver/api/tests/test_regioncontroller.py'
--- src/maasserver/api/tests/test_regioncontroller.py	2016-04-13 02:20:16 +0000
+++ src/maasserver/api/tests/test_regioncontroller.py	2016-05-16 16:20:02 +0000
@@ -54,6 +54,7 @@
             reverse('regioncontrollers_handler'))
 
     def test_read_returns_limited_fields(self):
+        self.become_admin()
         factory.make_RegionController()
         response = self.client.get(reverse('regioncontrollers_handler'))
         parsed_result = json_load_bytes(response.content)

=== modified file 'src/maasserver/api/tests/test_tag.py'
--- src/maasserver/api/tests/test_tag.py	2016-05-12 19:07:37 +0000
+++ src/maasserver/api/tests/test_tag.py	2016-05-16 16:20:02 +0000
@@ -147,11 +147,8 @@
         self.assertEqual(http.client.OK, response.status_code)
         parsed_result = json.loads(
             response.content.decode(settings.DEFAULT_CHARSET))
-        # XXX lamont Bug#1576417 : Region controllers should not be here
-        # either, but are the subject of said bug.
         self.assertItemsEqual(
-            [machine.system_id, device.system_id,
-             region.system_id],
+            [machine.system_id, device.system_id],
             [r['system_id'] for r in parsed_result])
 
     def test_GET_machines_returns_machines(self):
@@ -196,8 +193,6 @@
             [device.system_id],
             [r['system_id'] for r in parsed_result])
 
-    # XXX lamont Bug#1576417: Add a test that non-admins do not get to fetch
-    # rack controllers.
     def test_GET_rack_controllers_returns_rack_controllers(self):
         self.become_admin()
         tag = factory.make_Tag()
@@ -221,7 +216,28 @@
             [rack.system_id],
             [r['system_id'] for r in parsed_result])
 
+    def test_GET_rack_controllers_returns_no_rack_controllers_nonadmin(self):
+        tag = factory.make_Tag()
+        machine = factory.make_Node()
+        device = factory.make_Device()
+        rack = factory.make_RackController()
+        region = factory.make_RegionController()
+        # Create a second node that isn't tagged.
+        factory.make_Node()
+        machine.tags.add(tag)
+        device.tags.add(tag)
+        rack.tags.add(tag)
+        region.tags.add(tag)
+        response = self.client.get(
+            self.get_tag_uri(tag), {'op': 'rack_controllers'})
+
+        self.assertEqual(http.client.OK, response.status_code)
+        parsed_result = json.loads(
+            response.content.decode(settings.DEFAULT_CHARSET))
+        self.assertItemsEqual([], parsed_result)
+
     def test_GET_region_controllers_returns_region_controllers(self):
+        self.become_admin()
         tag = factory.make_Tag()
         machine = factory.make_Node()
         device = factory.make_Device()
@@ -243,6 +259,26 @@
             [region.system_id],
             [r['system_id'] for r in parsed_result])
 
+    def test_GET_region_controllers_returns_no_controllers_nonadmin(self):
+        tag = factory.make_Tag()
+        machine = factory.make_Node()
+        device = factory.make_Device()
+        rack = factory.make_RackController()
+        region = factory.make_RegionController()
+        # Create a second node that isn't tagged.
+        factory.make_Node()
+        machine.tags.add(tag)
+        device.tags.add(tag)
+        rack.tags.add(tag)
+        region.tags.add(tag)
+        response = self.client.get(
+            self.get_tag_uri(tag), {'op': 'region_controllers'})
+
+        self.assertEqual(http.client.OK, response.status_code)
+        parsed_result = json.loads(
+            response.content.decode(settings.DEFAULT_CHARSET))
+        self.assertItemsEqual([], parsed_result)
+
     def test_GET_nodes_hides_invisible_nodes(self):
         user2 = factory.make_User()
         node1 = factory.make_Node()

=== modified file 'src/maasserver/models/node.py'
--- src/maasserver/models/node.py	2016-05-12 19:07:37 +0000
+++ src/maasserver/models/node.py	2016-05-16 16:20:02 +0000
@@ -409,7 +409,14 @@
         if user.is_superuser:
             # Admin is allowed to see all nodes.
             return nodes
-        elif perm == NODE_PERMISSION.VIEW:
+        # Non-admins aren't allowed to see controllers.
+        nodes = nodes.exclude(
+            Q(node_type__in=[
+                NODE_TYPE.RACK_CONTROLLER,
+                NODE_TYPE.REGION_CONTROLLER,
+                NODE_TYPE.REGION_AND_RACK_CONTROLLER,
+                ]))
+        if perm == NODE_PERMISSION.VIEW:
             return nodes.filter(Q(owner__isnull=True) | Q(owner=user))
         elif perm == NODE_PERMISSION.EDIT:
             return nodes.filter(owner=user)

=== modified file 'src/maasserver/models/tests/test_node.py'
--- src/maasserver/models/tests/test_node.py	2016-05-12 19:07:37 +0000
+++ src/maasserver/models/tests/test_node.py	2016-05-16 16:20:02 +0000
@@ -3255,6 +3255,9 @@
     def test_get_nodes_with_admin_perm_returns_all_nodes_for_admin(self):
         user = factory.make_User()
         nodes = [self.make_node(user) for counter in range(5)]
+        nodes.append(factory.make_RackController())
+        nodes.append(factory.make_RegionController())
+        nodes.append(factory.make_RegionRackController())
         self.assertItemsEqual(
             nodes,
             Node.objects.get_nodes(
@@ -3282,6 +3285,24 @@
                 from_nodes=Node.objects.all())
         )
 
+    def test_get_nodes_non_admin_hides_controllers(self):
+        user = factory.make_User()
+        user_visible_nodes = [self.make_node(user), self.make_node(None)]
+        admin_visible_nodes = user_visible_nodes + [
+            self.make_node(factory.make_User()),
+            factory.make_RackController(owner=user),
+            factory.make_RackController(owner=None),
+            factory.make_RegionController(),
+            factory.make_RegionRackController(),
+        ]
+        self.assertItemsEqual(
+            admin_visible_nodes,
+            Node.objects.get_nodes(
+                factory.make_admin(), NODE_PERMISSION.ADMIN))
+        self.assertItemsEqual(
+            user_visible_nodes,
+            Node.objects.get_nodes(user, NODE_PERMISSION.VIEW))
+
     def test_filter_nodes_by_spaces(self):
         # Create a throwaway node and a throwaway space.
         # (to ensure they are filtered out.)

=== modified file 'src/maasserver/static/js/angular/controllers/nodes_list.js'
--- src/maasserver/static/js/angular/controllers/nodes_list.js	2016-04-29 19:31:59 +0000
+++ src/maasserver/static/js/angular/controllers/nodes_list.js	2016-05-16 16:20:02 +0000
@@ -585,6 +585,11 @@
             });
         };
 
+        // Return true if the authenticated user is super user.
+        $scope.isSuperUser = function() {
+            return UsersManager.isSuperUser();
+        };
+
         // Load the required managers for this controller. The ServicesManager
         // is required by the maasControllerStatus directive that is used
         // in the partial for this controller.

=== modified file 'src/maasserver/static/js/angular/controllers/tests/test_nodes_list.js'
--- src/maasserver/static/js/angular/controllers/tests/test_nodes_list.js	2016-04-29 07:41:09 +0000
+++ src/maasserver/static/js/angular/controllers/tests/test_nodes_list.js	2016-05-16 16:20:02 +0000
@@ -134,6 +134,22 @@
         return null;
     }
 
+    describe("isSuperUser", function() {
+        it("returns true if the user is a superuser", function() {
+            var controller = makeController();
+            spyOn(UsersManager, "getAuthUser").and.returnValue(
+                { is_superuser: true });
+            expect($scope.isSuperUser()).toBe(true);
+        });
+
+        it("returns false if the user is not a superuser", function() {
+            var controller = makeController();
+            spyOn(UsersManager, "getAuthUser").and.returnValue(
+                { is_superuser: false });
+            expect($scope.isSuperUser()).toBe(false);
+        });
+    });
+
     it("sets title and page on $rootScope", function() {
         var controller = makeController();
         expect($rootScope.title).toBe("Machines");

=== modified file 'src/maasserver/static/partials/nodes-list.html'
--- src/maasserver/static/partials/nodes-list.html	2016-05-11 19:01:48 +0000
+++ src/maasserver/static/partials/nodes-list.html	2016-05-16 16:20:02 +0000
@@ -11,8 +11,8 @@
                     data-ng-class="{ active: currentpage === 'devices' }"
                     data-ng-click="toggleTab('devices')">{$ devices.length $} <ng-pluralize count="devices.length" when="{'one': 'Device', 'other': 'Devices'}"></ng-pluralize>
                 </a>
-                <span class="divide"></span>
-                <a href=""
+                <span data-ng-show="isSuperUser()" class="divide"></span>
+                <a href="" data-ng-show="isSuperUser()"
                     data-ng-class="{ active: currentpage === 'controllers' }"
                     data-ng-click="toggleTab('controllers')">{$ controllers.length $} <ng-pluralize count="controllers.length" when="{'one': 'Controller', 'other': 'Controllers'}"></ng-pluralize>
                 </a>

=== modified file 'src/maasserver/websockets/handlers/tests/test_controller.py'
--- src/maasserver/websockets/handlers/tests/test_controller.py	2016-04-22 16:25:08 +0000
+++ src/maasserver/websockets/handlers/tests/test_controller.py	2016-05-16 16:20:02 +0000
@@ -21,7 +21,7 @@
             factory.make_RackController()
 
     def test_last_image_sync(self):
-        owner = factory.make_User()
+        owner = factory.make_admin()
         handler = ControllerHandler(owner, {})
         node = factory.make_RackController(owner=owner)
         result = handler.list({})
@@ -34,7 +34,7 @@
             node.last_image_sync))
 
     def test_last_image_sync_returns_none_for_none(self):
-        owner = factory.make_User()
+        owner = factory.make_admin()
         handler = ControllerHandler(owner, {})
         node = factory.make_RackController(owner=owner, last_image_sync=None)
         result = handler.list({})
@@ -45,7 +45,7 @@
         self.assertIsNone(data.get("last_image_sync"))
 
     def test_list_ignores_devices_and_nodes(self):
-        owner = factory.make_User()
+        owner = factory.make_admin()
         handler = ControllerHandler(owner, {})
         # Create a device.
         factory.make_Node(owner=owner, node_type=NODE_TYPE.DEVICE)