Merge lp:~rvb/maas/release-security into lp:~maas-committers/maas/trunk

[Raphaël Badin]

[MAAS Lander (maas-lander)]

Transitioned to Git.

lp:maas has now moved from Bzr to Git.
Please propose your branches with Launchpad using Git.

git clone https://git.launchpad.net/maas

Unmerged revisions

1974. By Raphaël Badin on 2014-02-18

Add test.

1973. By Raphaël Badin on 2014-02-18

Fix tests..

1972. By Raphaël Badin on 2014-02-18

Fix api.stop().

1971. By Raphaël Badin on 2014-02-18

Fix Node.release().

1970. By Raphaël Badin on 2014-02-18

Refactor start_commissioning() and accept_enlistment().

1969. By Raphaël Badin on 2014-02-18

Remove redundant permission check.

=== modified file 'src/maasserver/api.py'
--- src/maasserver/api.py	2014-02-18 03:19:42 +0000
+++ src/maasserver/api.py	2014-02-18 15:45:33 +0000
@@ -375,7 +375,10 @@
     @operation(idempotent=False)
     def stop(self, request, system_id):
         """Shut down a node."""
-        nodes = Node.objects.stop_nodes([system_id], request.user)
+        node = Node.objects.get_node_or_404(
+            system_id=system_id, user=request.user,
+            perm=NODE_PERMISSION.EDIT)
+        nodes = Node.objects.stop_nodes([node.system_id])
         if len(nodes) == 0:
             raise PermissionDenied(
                 "You are not allowed to shut down this node.")
@@ -401,13 +404,12 @@
         series = request.POST.get('distro_series', None)
         if user_data is not None:
             user_data = b64decode(user_data)
+        node = Node.objects.get_node_or_404(
+            system_id=system_id, user=request.user,
+            perm=NODE_PERMISSION.EDIT)
         if series is not None:
-            node = Node.objects.get_node_or_404(
-                system_id=system_id, user=request.user,
-                perm=NODE_PERMISSION.EDIT)
             node.set_distro_series(series=series)
-        nodes = Node.objects.start_nodes(
-            [system_id], request.user, user_data=user_data)
+        nodes = Node.objects.start_nodes([system_id], user_data=user_data)
         if len(nodes) == 0:
             raise PermissionDenied(
                 "You are not allowed to start up this node.")
@@ -648,7 +650,7 @@
         """
         node = create_node(request)
         if request.user.is_superuser:
-            node.accept_enlistment(request.user)
+            node.accept_enlistment()
         return node
 
     def _check_system_ids_exist(self, system_ids):
@@ -699,7 +701,7 @@
                 "following node(s): %s." % (
                     ', '.join(system_ids - permitted_ids)))
         return filter(
-            None, [node.accept_enlistment(request.user) for node in nodes])
+            None, [node.accept_enlistment() for node in nodes])
 
     @operation(idempotent=False)
     def accept_all(self, request):
@@ -717,7 +719,7 @@
         nodes = Node.objects.get_nodes(
             request.user, perm=NODE_PERMISSION.ADMIN)
         nodes = nodes.filter(status=NODE_STATUS.DECLARED)
-        nodes = [node.accept_enlistment(request.user) for node in nodes]
+        nodes = [node.accept_enlistment() for node in nodes]
         return filter(None, nodes)
 
     @operation(idempotent=False)

=== modified file 'src/maasserver/models/node.py'
--- src/maasserver/models/node.py	2014-02-10 06:24:38 +0000
+++ src/maasserver/models/node.py	2014-02-18 15:45:33 +0000
@@ -287,7 +287,7 @@
         available_nodes = self.get_nodes(for_user, NODE_PERMISSION.VIEW)
         return available_nodes.filter(status=NODE_STATUS.READY)
 
-    def stop_nodes(self, ids, by_user):
+    def stop_nodes(self, ids):
         """Request on given user's behalf that the given nodes be shut down.
 
         Shutdown is only requested for nodes that the user has ownership
@@ -295,12 +295,10 @@
 
         :param ids: The `system_id` values for nodes to be shut down.
         :type ids: Sequence
-        :param by_user: Requesting user.
-        :type by_user: User_
         :return: Those Nodes for which shutdown was actually requested.
         :rtype: list
         """
-        nodes = self.get_nodes(by_user, NODE_PERMISSION.EDIT, ids=ids)
+        nodes = self.filter_by_ids(Node.objects.all(), ids=ids)
         processed_nodes = []
         for node in nodes:
             power_params = node.get_effective_power_parameters()
@@ -321,7 +319,7 @@
             processed_nodes.append(node)
         return processed_nodes
 
-    def start_nodes(self, ids, by_user, user_data=None):
+    def start_nodes(self, ids, user_data=None):
         """Request on given user's behalf that the given nodes be started up.
 
         Power-on is only requested for nodes that the user has ownership
@@ -332,8 +330,6 @@
 
         :param ids: The `system_id` values for nodes to be started.
         :type ids: Sequence
-        :param by_user: Requesting user.
-        :type by_user: User_
         :param user_data: Optional blob of user-data to be made available to
             the nodes through the metadata service.  If not given, any
             previous user data is used.
@@ -344,7 +340,7 @@
         # Avoid circular imports.
         from metadataserver.models import NodeUserData
 
-        nodes = self.get_nodes(by_user, NODE_PERMISSION.EDIT, ids=ids)
+        nodes = self.filter_by_ids(Node.objects.all(), ids=ids)
         for node in nodes:
             NodeUserData.objects.set_user_data(node, user_data)
         processed_nodes = []
@@ -617,7 +613,7 @@
         if mac:
             mac.delete()
 
-    def accept_enlistment(self, user):
+    def accept_enlistment(self):
         """Accept this node's (anonymous) enlistment.
 
         This call makes sense only on a node in Declared state, i.e. one that
@@ -637,10 +633,10 @@
                 "Cannot accept node enlistment: node %s is in state %s."
                 % (self.system_id, NODE_STATUS_CHOICES_DICT[self.status]))
 
-        self.start_commissioning(user)
+        self.start_commissioning()
         return self
 
-    def start_commissioning(self, user):
+    def start_commissioning(self):
         """Install OS and self-test a new node."""
         # Avoid circular imports.
         from metadataserver.commissioning.user_data import generate_user_data
@@ -652,7 +648,7 @@
         self.save()
         # The commissioning profile is handled in start_nodes.
         Node.objects.start_nodes(
-            [self.system_id], user, user_data=commissioning_user_data)
+            [self.system_id], user_data=commissioning_user_data)
 
     def delete(self):
         # Allocated nodes can't be deleted.
@@ -804,7 +800,7 @@
 
     def release(self):
         """Mark allocated or reserved node as available again and power off."""
-        Node.objects.stop_nodes([self.system_id], self.owner)
+        Node.objects.stop_nodes([self.system_id])
         self.status = NODE_STATUS.READY
         self.owner = None
         self.token = None

=== modified file 'src/maasserver/models/tests/test_node.py'
--- src/maasserver/models/tests/test_node.py	2014-02-14 03:44:32 +0000
+++ src/maasserver/models/tests/test_node.py	2014-02-18 15:45:33 +0000
@@ -289,7 +289,6 @@
         # through the motions right up to the point where we'd normally
         # run shell commands.
         self.patch(PowerAction, 'run_shell', lambda *args, **kwargs: ('', ''))
-        user = factory.make_admin()
         nodes = [
             factory.make_node(power_type=power_type)
             for power_type in configless_power_types]
@@ -299,7 +298,7 @@
             node: node.get_effective_power_type()
             for node in nodes}
         started_nodes = Node.objects.start_nodes(
-            [node.system_id for node in list(node_power_types.keys())], user)
+            [node.system_id for node in list(node_power_types.keys())])
         successful_types = [node_power_types[node] for node in started_nodes]
         self.assertItemsEqual(configless_power_types, successful_types)
 
@@ -489,7 +488,7 @@
         target_state = NODE_STATUS.COMMISSIONING
 
         node = factory.make_node(status=NODE_STATUS.DECLARED)
-        return_value = node.accept_enlistment(factory.make_user())
+        return_value = node.accept_enlistment()
         self.assertEqual((node, target_state), (return_value, node.status))
 
     def test_accept_enlistment_does_nothing_if_already_accepted(self):
@@ -505,7 +504,7 @@
             for status in accepted_states}
 
         return_values = {
-            status: node.accept_enlistment(factory.make_user())
+            status: node.accept_enlistment()
             for status, node in nodes.items()}
 
         self.assertEqual(
@@ -533,7 +532,7 @@
         exceptions = {status: False for status in unacceptable_states}
         for status, node in nodes.items():
             try:
-                node.accept_enlistment(factory.make_user())
+                node.accept_enlistment()
             except NodeStateViolation:
                 exceptions[status] = True
 
@@ -547,7 +546,7 @@
         node = factory.make_node(
             status=NODE_STATUS.DECLARED, power_type='ether_wake')
         factory.make_mac_address(node=node)
-        node.start_commissioning(factory.make_admin())
+        node.start_commissioning()
 
         expected_attrs = {
             'status': NODE_STATUS.COMMISSIONING,
@@ -557,25 +556,13 @@
             ['provisioningserver.tasks.power_on'],
             [task['task'].name for task in self.celery.tasks])
 
-    def test_start_commisssioning_doesnt_start_nodes_for_non_admin_users(self):
-        node = factory.make_node(
-            status=NODE_STATUS.DECLARED, power_type='ether_wake')
-        factory.make_mac_address(node=node)
-        node.start_commissioning(factory.make_user())
-
-        expected_attrs = {
-            'status': NODE_STATUS.COMMISSIONING,
-        }
-        self.assertAttributes(node, expected_attrs)
-        self.assertEqual([], self.celery.tasks)
-
     def test_start_commissioning_sets_user_data(self):
         node = factory.make_node(status=NODE_STATUS.DECLARED)
         user_data = factory.getRandomString().encode('ascii')
         self.patch(
             commissioning.user_data, 'generate_user_data'
             ).return_value = user_data
-        node.start_commissioning(factory.make_admin())
+        node.start_commissioning()
         self.assertEqual(user_data, NodeUserData.objects.get_user_data(node))
         commissioning.user_data.generate_user_data.assert_called_with(
             nodegroup=node.nodegroup)
@@ -586,7 +573,7 @@
             node, factory.getRandomString(),
             random.randint(0, 10),
             Bin(factory.getRandomBytes()))
-        node.start_commissioning(factory.make_admin())
+        node.start_commissioning()
         self.assertItemsEqual([], node.nodecommissionresult_set.all())
 
     def test_start_commissioning_ignores_other_commissioning_results(self):
@@ -597,7 +584,7 @@
         NodeCommissionResult.objects.store_data(
             node, filename, script_result, Bin(data))
         other_node = factory.make_node(status=NODE_STATUS.DECLARED)
-        other_node.start_commissioning(factory.make_admin())
+        other_node.start_commissioning()
         self.assertEqual(
             data, NodeCommissionResult.objects.get_data(node, filename))
 
@@ -987,7 +974,7 @@
         self.patch(PowerAction, 'run_shell', lambda *args, **kwargs: ('', ''))
         user = factory.make_user()
         node, mac = self.make_node_with_mac(user, power_type='virsh')
-        output = Node.objects.stop_nodes([node.system_id], user)
+        output = Node.objects.stop_nodes([node.system_id])
 
         self.assertItemsEqual([node], output)
         self.assertEqual(
@@ -1001,21 +988,10 @@
         user = factory.make_user()
         node, mac = self.make_node_with_mac(user, power_type='virsh')
         task = self.patch(node_module, 'power_off')
-        Node.objects.stop_nodes([node.system_id], user)
+        Node.objects.stop_nodes([node.system_id])
         args, kwargs = task.apply_async.call_args
         self.assertEqual(node.work_queue, kwargs['queue'])
 
-    def test_stop_nodes_ignores_uneditable_nodes(self):
-        nodes = [
-            self.make_node_with_mac(
-                factory.make_user(), power_type='ether_wake')
-            for counter in range(3)]
-        ids = [node.system_id for node, mac in nodes]
-        stoppable_node = nodes[0][0]
-        self.assertItemsEqual(
-            [stoppable_node],
-            Node.objects.stop_nodes(ids, stoppable_node.owner))
-
     def test_stop_nodes_does_not_attempt_power_task_if_no_power_type(self):
         # If the node has a power_type set to UNKNOWN_POWER_TYPE
         # NodeManager.stop_node(this_node) won't create a power event
@@ -1023,7 +999,7 @@
         user = factory.make_user()
         node, unused = self.make_node_with_mac(
             user, power_type='')
-        output = Node.objects.stop_nodes([node.system_id], user)
+        output = Node.objects.stop_nodes([node.system_id])
 
         self.assertItemsEqual([], output)
         self.assertEqual(0, len(self.celery.tasks))
@@ -1032,7 +1008,7 @@
         user = factory.make_user()
         node, mac = self.make_node_with_mac(
             user, power_type='ether_wake')
-        output = Node.objects.start_nodes([node.system_id], user)
+        output = Node.objects.start_nodes([node.system_id])
 
         self.assertItemsEqual([node], output)
         self.assertEqual(
@@ -1048,7 +1024,7 @@
         node, mac = self.make_node_with_mac(
             user, power_type='ether_wake')
         task = self.patch(node_module, 'power_on')
-        Node.objects.start_nodes([node.system_id], user)
+        Node.objects.start_nodes([node.system_id])
         args, kwargs = task.apply_async.call_args
         self.assertEqual(node.work_queue, kwargs['queue'])
 
@@ -1059,7 +1035,7 @@
         user = factory.make_user()
         node, unused = self.make_node_with_mac(
             user, power_type='')
-        output = Node.objects.start_nodes([node.system_id], user)
+        output = Node.objects.start_nodes([node.system_id])
 
         self.assertItemsEqual([], output)
         self.assertEqual(0, len(self.celery.tasks))
@@ -1067,12 +1043,11 @@
     def test_start_nodes_wakeonlan_prefers_power_parameters(self):
         # If power_parameters is set we should prefer it to sifting
         # through related MAC addresses.
-        user = factory.make_user()
         preferred_mac = factory.getRandomMACAddress()
         node, mac = self.make_node_with_mac(
-            user, power_type='ether_wake',
+            power_type='ether_wake',
             power_parameters=dict(mac_address=preferred_mac))
-        output = Node.objects.start_nodes([node.system_id], user)
+        output = Node.objects.start_nodes([node.system_id])
 
         self.assertItemsEqual([node], output)
         self.assertEqual(
@@ -1086,64 +1061,38 @@
     def test_start_nodes_wakeonlan_ignores_invalid_parameters(self):
         # If node.power_params is set but doesn't have "mac_address" in it,
         # then the node shouldn't be started.
-        user = factory.make_user()
         node, mac = self.make_node_with_mac(
-            user, power_type='ether_wake',
+            power_type='ether_wake',
             power_parameters=dict(jarjar="binks"))
-        output = Node.objects.start_nodes([node.system_id], user)
+        output = Node.objects.start_nodes([node.system_id])
         self.assertItemsEqual([], output)
         self.assertEqual([], self.celery.tasks)
 
     def test_start_nodes_wakeonlan_ignores_empty_mac_address_parameter(self):
-        user = factory.make_user()
         node, mac = self.make_node_with_mac(
-            user, power_type='ether_wake',
+            power_type='ether_wake',
             power_parameters=dict(mac_address=""))
-        output = Node.objects.start_nodes([node.system_id], user)
+        output = Node.objects.start_nodes([node.system_id])
         self.assertItemsEqual([], output)
         self.assertEqual([], self.celery.tasks)
 
     def test_start_nodes_ignores_nodes_without_mac(self):
-        user = factory.make_user()
-        node = self.make_node(user)
-        output = Node.objects.start_nodes([node.system_id], user)
+        node = self.make_node()
+        output = Node.objects.start_nodes([node.system_id])
 
         self.assertItemsEqual([], output)
 
-    def test_start_nodes_ignores_uneditable_nodes(self):
-        nodes = [
-            self.make_node_with_mac(
-                factory.make_user(), power_type='ether_wake')[0]
-            for counter in range(3)
-        ]
-        ids = [node.system_id for node in nodes]
-        startable_node = nodes[0]
-        self.assertItemsEqual(
-            [startable_node],
-            Node.objects.start_nodes(ids, startable_node.owner))
-
     def test_start_nodes_stores_user_data(self):
         node = factory.make_node(owner=factory.make_user())
         user_data = self.make_user_data()
-        Node.objects.start_nodes(
-            [node.system_id], node.owner, user_data=user_data)
+        Node.objects.start_nodes([node.system_id], user_data=user_data)
         self.assertEqual(user_data, NodeUserData.objects.get_user_data(node))
 
-    def test_start_nodes_does_not_store_user_data_for_uneditable_nodes(self):
-        node = factory.make_node(owner=factory.make_user())
-        original_user_data = self.make_user_data()
-        NodeUserData.objects.set_user_data(node, original_user_data)
-        Node.objects.start_nodes(
-            [node.system_id], factory.make_user(),
-            user_data=self.make_user_data())
-        self.assertEqual(
-            original_user_data, NodeUserData.objects.get_user_data(node))
-
     def test_start_nodes_without_user_data_clears_existing_data(self):
         node = factory.make_node(owner=factory.make_user())
         user_data = self.make_user_data()
         NodeUserData.objects.set_user_data(node, user_data)
-        Node.objects.start_nodes([node.system_id], node.owner, user_data=None)
+        Node.objects.start_nodes([node.system_id], user_data=None)
         self.assertRaises(
             NodeUserData.DoesNotExist,
             NodeUserData.objects.get_user_data, node)
@@ -1152,8 +1101,7 @@
         node = factory.make_node(owner=factory.make_user())
         NodeUserData.objects.set_user_data(node, self.make_user_data())
         user_data = self.make_user_data()
-        Node.objects.start_nodes(
-            [node.system_id], node.owner, user_data=user_data)
+        Node.objects.start_nodes([node.system_id], user_data=user_data)
         self.assertEqual(user_data, NodeUserData.objects.get_user_data(node))
 
     def test_netboot_on(self):

=== modified file 'src/maasserver/node_action.py'
--- src/maasserver/node_action.py	2013-11-15 18:15:51 +0000
+++ src/maasserver/node_action.py	2014-02-18 15:45:33 +0000
@@ -177,7 +177,7 @@
 
     def execute(self, allow_redirect=True):
         """See `NodeAction.execute`."""
-        self.node.start_commissioning(self.user)
+        self.node.start_commissioning()
         return "Node commissioning started."
 
 
@@ -242,9 +242,7 @@
         # acquire() call.
         self.node.acquire(self.user, token=None)
 
-        # Be sure to acquire before starting, or start_nodes will think
-        # the node ineligible based on its un-acquired status.
-        Node.objects.start_nodes([self.node.system_id], self.user)
+        Node.objects.start_nodes([self.node.system_id])
         return dedent("""\
             This node is now allocated to you.
             It has been asked to start up.

=== modified file 'src/maasserver/tests/test_node_action.py'
--- src/maasserver/tests/test_node_action.py	2014-01-14 00:43:21 +0000
+++ src/maasserver/tests/test_node_action.py	2014-02-18 15:45:33 +0000
@@ -263,6 +263,12 @@
             'provisioningserver.tasks.power_off',
             self.celery.tasks[0]['task'].name)
 
+    def test_StopNode_is_not_permitted_to_non_owner(self):
+        node = factory.make_node(
+            mac=True, status=NODE_STATUS.ALLOCATED,
+            power_type='ether_wake', owner=factory.make_user())
+        self.assertFalse(StopNode(node, factory.make_user()).is_permitted())
+
 
 class TestUseCurtinNodeAction(MAASServerTestCase):