launchpad-reviewers team mailing list archive

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

Jeroen T. Vermeulen has proposed merging lp:~jtv/maas/backport-987585 into lp:maas/1.0.

Requested reviews:
  Launchpad code reviewers (launchpad-reviewers)
Related bugs:
  Bug #987585 in MAAS: "Start Node button does not allocate node"
  https://bugs.launchpad.net/maas/+bug/987585

For more details, see:
https://code.launchpad.net/~jtv/maas/backport-987585/+merge/103643

Francis asked for this to be backported to the 1.0 branch.  What you see here not entirely a rewrite of the original fix in trunk, but neither is it a simple backport.  Too many things had changed between trunk and 1.0.  Some seemingly unrelated changes from trunk I introduced by hand (e.g. saving after acquire/release, from inside the model methods) but only minimally so (for example I didn't remove the now-obsolete saves after calls to acquire/release).  I didn't even bother with the view changes.

The backport is not of the same quality and robustness as the trunk fix; to do so would have required importing tons more changes from trunk.  We really have to get off the 1.0 branch soon.  Functionally trunk is still very similar to 1.0, but it's miles ahead in quality and polish, and pulling away.


Jeroen
-- 
https://code.launchpad.net/~jtv/maas/backport-987585/+merge/103643
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~jtv/maas/backport-987585 into lp:maas/1.0.

=== modified file 'src/maasserver/api.py'
--- src/maasserver/api.py	2012-04-16 10:00:51 +0000
+++ src/maasserver/api.py	2012-04-26 09:45:41 +0000
@@ -58,6 +58,7 @@
     "api_doc_title",
     "extract_oauth_key",
     "generate_api_doc",
+    "get_oauth_token",
     "AccountHandler",
     "AnonNodesHandler",
     "FilesHandler",
@@ -307,6 +308,19 @@
     return None
 
 
+def get_oauth_token(request):
+    """Get the OAuth :class:`piston.models.Token` used for `request`.
+
+    Raises :class:`Unauthorized` if no key is found, or if the token is
+    unknown.
+    """
+    auth_header = request.META.get('HTTP_AUTHORIZATION')
+    try:
+        return Token.objects.get(key=extract_oauth_key(auth_header))
+    except Token.DoesNotExist:
+        raise Unauthorized("Unknown OAuth token.")
+
+
 NODE_FIELDS = (
     'system_id', 'hostname', ('macaddress_set', ('mac_address',)),
     'architecture', 'status')

=== modified file 'src/maasserver/forms.py'
--- src/maasserver/forms.py	2012-04-26 07:25:55 +0000
+++ src/maasserver/forms.py	2012-04-26 09:45:41 +0000
@@ -239,11 +239,20 @@
         return node
 
 
-def start_node(node, user):
-    """Start a node from the UI.  It will have no meta_data."""
+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.
+    from maasserver.api import get_oauth_token
+
+    node.acquire(get_oauth_token(request))
     Node.objects.start_nodes([node.system_id], user)
 
 
+def start_commissioning_node(node, user, request=None):
+    """Start the commissioning process for a node."""
+    node.start_commissioning(user)
+
+
 # Node actions per status.
 #
 # This maps each NODE_STATUS to a list of actions applicable to a node
@@ -265,7 +274,11 @@
 #     'display': "Paint node",
 #     # Permission required to perform action.
 #     'permission': NODE_PERMISSION.EDIT,
-#     # Callable that performs action.  Takes parameters (node, user).
+#     # 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(
 #                    node, favourite_colour(user)),
 # }
@@ -275,7 +288,7 @@
         {
             'display': "Accept & commission",
             'permission': NODE_PERMISSION.ADMIN,
-            'execute': lambda node, user: Node.start_commissioning(node, user),
+            'execute': start_commissioning_node,
             'message': "Node commissioning started.",
         },
     ],
@@ -283,16 +296,18 @@
         {
             'display': "Retry commissioning",
             'permission': NODE_PERMISSION.ADMIN,
-            'execute': lambda node, user: Node.start_commissioning(node, user),
+            'execute': start_commissioning_node,
             'message': "Started a new attempt to commission this node.",
         },
     ],
     NODE_STATUS.READY: [
         {
             'display': "Start node",
-            'permission': NODE_PERMISSION.EDIT,
-            'execute': start_node,
-            'message': "Node started."
+            'permission': NODE_PERMISSION.VIEW,
+            'execute': acquire_and_start_node,
+            'message': (
+                "This node is now allocated to you.  "
+                "It has been asked to start up."),
         },
     ],
 }
@@ -347,13 +362,10 @@
         action_name = self.data.get(self.input_name)
         permission, execute, message = (
             self.action_dict.get(action_name, (None, None, None)))
-        if execute is not None:
-            if not self.user.has_perm(permission, self.node):
-                raise PermissionDenied()
-            execute(self.node, self.user)
-            self.display_message(message)
-        else:
+        if execute is None or not self.user.has_perm(permission, self.node):
             raise PermissionDenied()
+        execute(node=self.node, user=self.user, request=self.request)
+        self.display_message(message)
 
 
 def get_action_form(user, request=None):

=== modified file 'src/maasserver/models.py'
--- src/maasserver/models.py	2012-04-26 02:43:36 +0000
+++ src/maasserver/models.py	2012-04-26 09:45:41 +0000
@@ -674,12 +674,14 @@
         self.status = NODE_STATUS.ALLOCATED
         self.owner = token.user
         self.token = token
+        self.save()
 
     def release(self):
         """Mark allocated or reserved node as available again."""
         self.status = NODE_STATUS.READY
         self.owner = None
         self.token = None
+        self.save()
 
 
 mac_re = re.compile(r'^([0-9a-fA-F]{2}:){5}[0-9a-fA-F]{2}$')

=== modified file 'src/maasserver/tests/test_api.py'
--- src/maasserver/tests/test_api.py	2012-04-18 09:42:59 +0000
+++ src/maasserver/tests/test_api.py	2012-04-26 09:45:41 +0000
@@ -13,6 +13,7 @@
 __all__ = []
 
 from base64 import b64encode
+from collections import namedtuple
 import httplib
 import json
 import os
@@ -27,7 +28,9 @@
 from maasserver.api import (
     extract_constraints,
     extract_oauth_key,
+    get_oauth_token,
     )
+from maasserver.exceptions import Unauthorized
 from maasserver.models import (
     ARCHITECTURE_CHOICES,
     Config,
@@ -72,6 +75,11 @@
 
 class TestModuleHelpers(TestCase):
 
+    def make_fake_request(self, auth_header):
+        """Create a very simple fake request, with just an auth header."""
+        FakeRequest = namedtuple('FakeRequest', ['META'])
+        return FakeRequest(META={'HTTP_AUTHORIZATION': auth_header})
+
     def test_extract_oauth_key_extracts_oauth_token_from_oauth_header(self):
         token = factory.getRandomString(18)
         self.assertEqual(
@@ -81,6 +89,22 @@
     def test_extract_oauth_key_returns_None_without_oauth_key(self):
         self.assertIs(None, extract_oauth_key(''))
 
+    def test_get_oauth_token_finds_token(self):
+        user = factory.make_user()
+        consumer, token = user.get_profile().create_authorisation_token()
+        self.assertEqual(
+            token,
+            get_oauth_token(
+                self.make_fake_request(
+                    factory.make_oauth_header(oauth_token=token.key))))
+
+    def test_get_oauth_token_raises_Unauthorized_for_unknown_token(self):
+        fake_token = factory.getRandomString(18)
+        header = factory.make_oauth_header(oauth_token=fake_token)
+        self.assertRaises(
+            Unauthorized,
+            get_oauth_token, self.make_fake_request(header))
+
     def test_extract_constraints_ignores_unknown_parameters(self):
         unknown_parameter = "%s=%s" % (
             factory.getRandomString(),

=== modified file 'src/maasserver/tests/test_forms.py'
--- src/maasserver/tests/test_forms.py	2012-04-26 03:30:50 +0000
+++ src/maasserver/tests/test_forms.py	2012-04-26 09:45:41 +0000
@@ -18,6 +18,7 @@
     ValidationError,
     )
 from django.http import QueryDict
+import maasserver.api
 from maasserver.forms import (
     ConfigForm,
     EditUserForm,
@@ -342,15 +343,28 @@
 
         self.assertRaises(PermissionDenied, form.save)
 
-    def test_start_action_starts_ready_node_for_admin(self):
+    def test_start_action_acquires_and_starts_ready_node_for_user(self):
         node = factory.make_node(status=NODE_STATUS.READY)
-        form = get_action_form(factory.make_admin())(
+        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, {NodeActionForm.input_name: "Start node"})
         form.save()
 
         power_status = get_provisioning_api_proxy().power_status
+        self.assertEqual(NODE_STATUS.ALLOCATED, node.status)
+        self.assertEqual(user, node.owner)
         self.assertEqual('start', power_status.get(node.system_id))
 
+    def test_accept_and_commission_starts_commissioning(self):
+        admin = factory.make_admin()
+        node = factory.make_node(status=NODE_STATUS.DECLARED)
+        form = get_action_form(admin)(
+            node, {NodeActionForm.input_name: "Accept & commission"})
+        form.save()
+        self.assertEqual(NODE_STATUS.COMMISSIONING, node.status)
+
 
 class TestHostnameFormField(TestCase):
 

=== modified file 'src/maasserver/tests/test_views.py'
--- src/maasserver/tests/test_views.py	2012-04-26 07:25:55 +0000
+++ src/maasserver/tests/test_views.py	2012-04-26 09:45:41 +0000
@@ -758,7 +758,7 @@
         response = self.client.get(node_link)
         return response
 
-    def test_start_commisionning_displays_message(self):
+    def test_start_commisioning_displays_message(self):
         self.become_admin()
         node = factory.make_node(status=NODE_STATUS.DECLARED)
         response = self.perform_action_and_get_node_page(
@@ -767,15 +767,6 @@
             "Node commissioning started.",
             [message.message for message in response.context['messages']])
 
-    def test_start_node_displays_message(self):
-        node = factory.make_node(
-            status=NODE_STATUS.READY, owner=self.logged_in_user)
-        response = self.perform_action_and_get_node_page(
-            node, "Start node")
-        self.assertIn(
-            "Node started.",
-            [message.message for message in response.context['messages']])
-
 
 class MAASExceptionHandledInView(LoggedInTestCase):