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_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,25 @@
>>          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)
>> +        action.execute()
>
> In illustration of what I said above, shouldn't this actually fail?

No. Why would it? Admins can start or stop (or release) nodes at will.
You may be looking for an extra safeguard against this bit of code
working, but that happens outside of the StartNode action — if it has
an owner it's just plain not shown to users who don't own the node.
That start_nodes() won't start nodes you don't own is an added
safeguard.