launchpad-reviewers team mailing list archive

["Jeroen T. Vermeulen" <jtv@xxxxxxxxxxxxx>]

Jeroen T. Vermeulen has proposed merging lp:~jtv/maas/bug-986185-start-node-requires-ssh-key into lp:maas with lp:~jtv/maas/bug-986185-conditional-node-actions as a prerequisite.

Requested reviews:
  Launchpad code reviewers (launchpad-reviewers)
Related bugs:
  Bug #986185 in MAAS: "MAAS boots inaccessible nodes when there's no SSH registered for a given user"
  https://bugs.launchpad.net/maas/+bug/986185

For more details, see:
https://code.launchpad.net/~jtv/maas/bug-986185-start-node-requires-ssh-key/+merge/103536

This is the culmination of work on bug 986185, as discussed with Raphaël: the “Start node” button lets a user acquire and start a node before they have SSH keys set up, so that they end up with a node they can't access.

In preceding branches I have built infrastructure to allow a node action (for that is what these buttons are) to be disabled, but such that they remain visible and provide the reason that inhibits the action under the given circumstances.  That was previously special behaviour for the Delete button.  Those preceding branches are not being reviewed yet at the time of this writing.  With this last branch in the series, the Start Node action (but only for Ready nodes) makes use of the new capability to check for the presence of an SSH key.


Jeroen
-- 
https://code.launchpad.net/~jtv/maas/bug-986185-start-node-requires-ssh-key/+merge/103536
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~jtv/maas/bug-986185-start-node-requires-ssh-key into lp:maas.

=== modified file 'src/maasserver/forms.py'
--- src/maasserver/forms.py	2012-04-25 18:12:23 +0000
+++ src/maasserver/forms.py	2012-04-25 18:12:23 +0000
@@ -239,6 +239,16 @@
     Node.objects.start_nodes([node.system_id], user)
 
 
+def inhibit_acquisition(node, user, request=None):
+    """Give me one reason why `user` can't acquire and start `node`."""
+    if SSHKey.objects.get_keys_for_user(user).exists():
+        return None
+    else:
+        return (
+            "You have no means of accessing the node after starting it. "
+            "Register an SSH key first.")
+
+
 def acquire_and_start_node(node, user, request=None):
     """Acquire and start a node from the UI.  It will have no meta_data."""
     # Avoid circular imports.
@@ -287,16 +297,23 @@
 # Each action is a dict:
 #
 # {
-#     # Action's display name; will be shown in the button.
+#     # Action's display name; will be the button's label.
 #     'display': "Paint node",
-#     # Permission required to perform action.
+#
+#     # Permission required on the node to perform the action.
 #     'permission': NODE_PERMISSION.EDIT,
+#
+#     # Function that looks for reasons to disable the action.
+#     # If it returns None, the action is available; otherwise, it is
+#     # shown but disabled.  A tooltip shows the inhibiting reason.
+#     'inhibit': lambda node, user, request=None: None,
+#
 #     # Callable that performs action.
 #     # Takes parameters (node, user, request).
 #     # Even though this is not the API, the action may raise a
 #     # MAASAPIException if it wants to convey failure with a specific
 #     # http status code.
-#     'execute': lambda node, user: paint_node(
+#     'execute': lambda node, user, request=None: paint_node(
 #                    node, favourite_colour(user)),
 # }
 #
@@ -343,6 +360,7 @@
             'message': (
                 "This node is now allocated to you.  "
                 "It has been asked to start up."),
+            'inhibit': inhibit_acquisition,
         },
     ],
     NODE_STATUS.RESERVED: [

=== modified file 'src/maasserver/tests/test_forms.py'
--- src/maasserver/tests/test_forms.py	2012-04-25 18:12:23 +0000
+++ src/maasserver/tests/test_forms.py	2012-04-25 18:12:23 +0000
@@ -435,6 +435,7 @@
     def test_start_action_acquires_and_starts_ready_node_for_user(self):
         node = factory.make_node(status=NODE_STATUS.READY)
         user = factory.make_user()
+        factory.make_sshkey(user)
         consumer, token = user.get_profile().create_authorisation_token()
         self.patch(maasserver.api, 'get_oauth_token', lambda request: token)
         form = get_action_form(user)(
@@ -449,6 +450,7 @@
     def test_start_action_starts_allocated_node_for_owner(self):
         node = factory.make_node(
             status=NODE_STATUS.ALLOCATED, owner=factory.make_user())
+        factory.make_sshkey(node.owner)
         form = get_action_form(node.owner)(
             node, {NodeActionForm.input_name: "Start node"})
         form.save()
@@ -456,6 +458,23 @@
         power_status = get_provisioning_api_proxy().power_status
         self.assertEqual('start', power_status.get(node.system_id))
 
+    def test_start_action_on_ready_node_is_enabled_for_user_with_key(self):
+        node = factory.make_node(status=NODE_STATUS.READY)
+        user = factory.make_user()
+        factory.make_sshkey(user)
+        consumer, token = user.get_profile().create_authorisation_token()
+        self.patch(maasserver.api, 'get_oauth_token', lambda request: token)
+        form = get_action_form(user)(node)
+        self.assertIs(None, form.find_action("Start node")['inhibition'])
+
+    def test_start_action_on_ready_node_is_disabled_for_keyless_user(self):
+        node = factory.make_node(status=NODE_STATUS.READY)
+        user = factory.make_user()
+        consumer, token = user.get_profile().create_authorisation_token()
+        self.patch(maasserver.api, 'get_oauth_token', lambda request: token)
+        form = get_action_form(user)(node)
+        self.assertIn("SSH key", form.find_action("Start node")['inhibition'])
+
     def test_accept_and_commission_starts_commissioning(self):
         admin = factory.make_admin()
         node = factory.make_node(status=NODE_STATUS.DECLARED)

=== modified file 'src/maasserver/tests/test_views_nodes.py'
--- src/maasserver/tests/test_views_nodes.py	2012-04-25 18:12:23 +0000
+++ src/maasserver/tests/test_views_nodes.py	2012-04-25 18:12:23 +0000
@@ -320,6 +320,7 @@
             [message.message for message in response.context['messages']])
 
     def test_start_node_from_ready_displays_message(self):
+        factory.make_sshkey(self.logged_in_user)
         profile = self.logged_in_user.get_profile()
         consumer, token = profile.create_authorisation_token()
         self.patch(maasserver.api, 'get_oauth_token', lambda request: token)
@@ -339,6 +340,7 @@
             [message.message for message in response.context['messages']])
 
     def test_start_node_without_auth_returns_Unauthorized(self):
+        factory.make_sshkey(self.logged_in_user)
         node = factory.make_node(status=NODE_STATUS.READY)
         response = self.client.post(
             reverse('node-view', args=[node.system_id]),