Merge lp:~gmb/maas/acquire-and-start-node-bug-1381007 into lp:~maas-committers/maas/trunk

[Graham Binns]

Update the "Start node" button to show "Acquire and start node" only when the node is READY and there is no owner; otherwise it displays "Start node".

Previously, "Acquire and start node" would appear for admins, even when the node had been allocated.

[Christian Reis (kiko)]

How about a test that guarantees that you can't start+allocate a node if you aren't the owner? I know the UI doesn't let you do that, but shouldn't we have an internal guarantee that we don't mess this up because the UI changed?

review: Needs Information

[Graham Binns (gmb)]

On 14 October 2014 12:50, Christian Reis <email address hidden> wrote:
> How about a test that guarantees that you can't start+allocate a node if you aren't the owner? I know the UI doesn't let you do that, but shouldn't we have an internal guarantee that we don't mess this up because the UI changed?

Good point. I've added two tests:

 1. To show that StartNode.execute() won't re-allocate the node when
clicked by someone (an admin) who doesn't own it.
 2. To show that NodeManager.start_nodes() won't start nodes that
don't belong to the user passed to it.

Does that cover what you're looking for?

[Christian Reis (kiko)]

Thanks!

It's a bit weird that it silently throws away nodes which the user doesn't own (i.e. in your first test you provide two nodes and only one comes back), enough that it might have an XXX to cover it where it's done. If you can do that approved by me.

Doing anything more than a comment is too much for 1.7.

review: Approve

[Graham Binns (gmb)]

On 14 October 2014 15:15, Christian Reis <email address hidden> wrote:
> Review: Approve
>
> Thanks!
>
> It's a bit weird that it silently throws away nodes which the user doesn't own (i.e. in your first test you provide two nodes and only one comes back), enough that it might have an XXX to cover it where it's done. If you can do that approved by me.
>
> Doing anything more than a comment is too much for 1.7.
>
> Diff comments:
>
>> === modified file 'src/maasserver/models/tests/test_node.py'
>> --- src/maasserver/models/tests/test_node.py 2014-10-14 03:50:09 +0000
>> +++ src/maasserver/models/tests/test_node.py 2014-10-14 12:16:36 +0000
>> @@ -2335,6 +2335,22 @@
>> [node1.system_id, node2.system_id], user)
>> self.assertItemsEqual([node1], nodes_started)
>>
>> + def test__does_not_try_to_start_nodes_not_allocated_to_user(self):
>> + user1 = factory.make_User()
>> + [node1] = self.make_acquired_nodes_with_macs(user1, count=1)
>> + node1.power_type = 'ether_wake' # can be started.
>> + node1.save()
>> + user2 = factory.make_User()
>> + [node2] = self.make_acquired_nodes_with_macs(user2, count=1)
>> + node2.power_type = 'ether_wake' # can be started.
>> + node2.save()
>> +
>> + self.patch(node_module, 'power_on_nodes')
>> + self.patch(node_module, 'wait_for_power_commands')
>> + nodes_started = Node.objects.start_nodes(
>> + [node1.system_id, node2.system_id], user1)
>> + self.assertItemsEqual([node1], nodes_started)
>
> ^^^

Will add.

>> +
>>
>> class NodeManagerTest_StopNodes(MAASServerTestCase):
>>
>>
>> === modified file 'src/maasserver/node_action.py'
>> --- src/maasserver/node_action.py 2014-10-13 10:40:43 +0000
>> +++ src/maasserver/node_action.py 2014-10-14 12:16:36 +0000
>> @@ -311,10 +311,18 @@
>>
>> @property
>> def display(self):
>> - if self.node.owner == self.user:
>> + # We explictly check for owner is None here, rather than owner
>> + # != self.user, because in practice the only people who are
>> + # going to see this button other than the node's owner are
>> + # administrators (because once once the node's owned, you need
>> + # EDIT permission to see this button).
>> + # We can safely assume that if you're seeing this and you don't
>> + # own the node, you're an admin, so you can still start it even
>> + # though you don't own it.
>> + if self.node.status == NODE_STATUS.READY and self.node.owner is None:
>> + label = "Acquire and start node"
>> + else:
>> label = "Start node"
>> - else:
>> - label = "Acquire and start node"
>> return label
>>
>> def inhibit(self):
>>
>> === modified file 'src/maasserver/tests/test_node_action.py'
>> --- src/maasserver/tests/test_node_action.py 2014-10-13 10:38:39 +0000
>> +++ src/maasserver/tests/test_node_action.py 2014-10-14 12:16:36 +0000
>> @@ -355,7 +355,7 @@
>> action = StartNode(node, user)
>> self.assertEqual("Acquire and start node", action.display)
>>
>> - def test_StartNode_label_hids_alloc...

[Julian Edwards (julian-edwards)]

Point of order! →

=== modified file 'src/maasserver/models/tests/test_node.py'
--- src/maasserver/models/tests/test_node.py	2014-10-14 03:50:09 +0000
+++ src/maasserver/models/tests/test_node.py	2014-10-14 14:31:16 +0000
@@ -2335,6 +2335,27 @@
             [node1.system_id, node2.system_id], user)
         self.assertItemsEqual([node1], nodes_started)
 
+    def test__does_not_try_to_start_nodes_not_allocated_to_user(self):
+        user1 = factory.make_User()
+        [node1] = self.make_acquired_nodes_with_macs(user1, count=1)
+        node1.power_type = 'ether_wake'  # can be started.
+        node1.save()
+        user2 = factory.make_User()
+        [node2] = self.make_acquired_nodes_with_macs(user2, count=1)
+        node2.power_type = 'ether_wake'  # can be started.
+        node2.save()
+
+        self.patch(node_module, 'power_on_nodes')
+        self.patch(node_module, 'wait_for_power_commands')
+        nodes_started = Node.objects.start_nodes(
+            [node1.system_id, node2.system_id], user1)
+
+        # Since no power commands were sent to the node, it isn't
+        # returned by start_nodes().
+        # test__only_returns_nodes_for_which_power_commands_have_been_sent()
+        # demonstrates this behaviour.
+        self.assertItemsEqual([node1], nodes_started)
+
 
 class NodeManagerTest_StopNodes(MAASServerTestCase):
 

=== modified file 'src/maasserver/node_action.py'
--- src/maasserver/node_action.py	2014-10-13 10:40:43 +0000
+++ src/maasserver/node_action.py	2014-10-14 14:31:16 +0000
@@ -311,10 +311,18 @@
 
     @property
     def display(self):
-        if self.node.owner == self.user:
+        # We explictly check for owner is None here, rather than owner
+        # != self.user, because in practice the only people who are
+        # going to see this button other than the node's owner are
+        # administrators (because once once the node's owned, you need
+        # EDIT permission to see this button).
+        # We can safely assume that if you're seeing this and you don't
+        # own the node, you're an admin, so you can still start it even
+        # though you don't own it.
+        if self.node.status == NODE_STATUS.READY and self.node.owner is None:
+            label = "Acquire and start node"
+        else:
             label = "Start node"
-        else:
-            label = "Acquire and start node"
         return label
 
     def inhibit(self):

=== modified file 'src/maasserver/tests/test_node_action.py'
--- src/maasserver/tests/test_node_action.py	2014-10-13 10:38:39 +0000
+++ src/maasserver/tests/test_node_action.py	2014-10-14 14:31:16 +0000
@@ -355,7 +355,7 @@
         action = StartNode(node, user)
         self.assertEqual("Acquire and start node", action.display)
 
-    def test_StartNode_label_hids_allocate_if_allocated(self):
+    def test_StartNode_label_hides_allocate_if_allocated(self):
         self.patch(Node.objects, "start_nodes")
         user = factory.make_User()
         node = factory.make_Node(status=NODE_STATUS.READY)
@@ -363,6 +363,30 @@
         action = StartNode(node, user)
         self.assertEqual("Start node", action.display)
 
+    def test_StartNode_label_hides_acquire_for_non_owner_admin(self):
+        user = factory.make_User()
+        admin = factory.make_admin()
+        node = factory.make_Node(status=NODE_STATUS.READY)
+        node.acquire(user)
+        action = StartNode(node, admin)
+        self.assertEqual("Start node", action.display)
+
+    def test_StartNode_does_not_reallocate_when_run_by_non_owner(self):
+        self.patch(Node.objects, "start_nodes")
+        user = factory.make_User()
+        admin = factory.make_admin()
+        node = factory.make_Node(status=NODE_STATUS.READY)
+        node.acquire(user)
+        action = StartNode(node, admin)
+
+        # This action.execute() will not fail because the non-owner is
+        # an admin, so they can start the node. Even if they weren't an
+        # admin, the node still wouldn't start;
+        # NodeManager.start_nodes() would ignore it.
+        action.execute()
+        self.assertEqual(user, node.owner)
+        self.assertEqual(NODE_STATUS.ALLOCATED, node.status)
+
 
 class TestStopNodeNodeAction(MAASServerTestCase):