launchpad-reviewers team mailing list archive

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

Jeroen T. Vermeulen has proposed merging lp:~jtv/maas/bug-989729 into lp:maas.

Requested reviews:
  Launchpad code reviewers (launchpad-reviewers)
Related bugs:
  Bug #989729 in MAAS: "“Start node” button breaks: no oauth key"
  https://bugs.launchpad.net/maas/+bug/989729

For more details, see:
https://code.launchpad.net/~jtv/maas/bug-989729/+merge/103883

“Starting” a node, at least in the UI, now means acquiring and starting it.  But acquiring a node requires an OAuth key, and so this breaks.

Discussed this with Raphaël.  We're not sure why we'd need that oauth key.  The Node.token field that the user's oauth token goes into seems to be completely undocumented, and its name tells us nothing except that it's an authentication token.

As a solution, then, I'm making the token an optional part of acquisition at the model level.  The API still requires it (and it could be relevant only to that usage) but the UI will no pass it.  The acquire() call also needs to know the acquiring user, which it previously got from the token; I made that an explicit parameter.

A separate branch solving this for 1.0 is underway.  The two are identical in principle, but quite different in where they affect the source tree.  The two branches have diverged too far for convenient parallel maintenance.


Jeroen
-- 
https://code.launchpad.net/~jtv/maas/bug-989729/+merge/103883
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~jtv/maas/bug-989729 into lp:maas.

=== modified file 'src/maasserver/api.py'
--- src/maasserver/api.py	2012-04-27 08:11:29 +0000
+++ src/maasserver/api.py	2012-04-27 14:14:24 +0000
@@ -591,7 +591,7 @@
             request.user, constraints=extract_constraints(request.data))
         if node is None:
             raise NodesNotAvailable("No matching node is available.")
-        node.acquire(get_oauth_token(request))
+        node.acquire(request.user, get_oauth_token(request))
         return node
 
     @classmethod

=== modified file 'src/maasserver/models/__init__.py'
--- src/maasserver/models/__init__.py	2012-04-27 08:11:29 +0000
+++ src/maasserver/models/__init__.py	2012-04-27 14:14:24 +0000
@@ -598,11 +598,12 @@
             power_type = self.power_type
         return power_type
 
-    def acquire(self, token):
-        """Mark commissioned node as acquired by the given user's token."""
+    def acquire(self, user, token=None):
+        """Mark commissioned node as acquired by the given user and token."""
         assert self.owner is None
+        assert token is None or token.user == user
         self.status = NODE_STATUS.ALLOCATED
-        self.owner = token.user
+        self.owner = user
         self.token = token
         self.save()
 

=== modified file 'src/maasserver/node_action.py'
--- src/maasserver/node_action.py	2012-04-27 08:25:19 +0000
+++ src/maasserver/node_action.py	2012-04-27 14:14:24 +0000
@@ -183,12 +183,12 @@
         return None
 
     def execute(self):
-        # Avoid circular imports.
-        from maasserver.api import get_oauth_token
+        # The UI does not use OAuth, so there is no token to pass to the
+        # 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.
-        self.node.acquire(get_oauth_token(self.request))
         Node.objects.start_nodes([self.node.system_id], self.user)
         return dedent("""\
             This node is now allocated to you.

=== modified file 'src/maasserver/tests/test_models.py'
--- src/maasserver/tests/test_models.py	2012-04-25 12:35:02 +0000
+++ src/maasserver/tests/test_models.py	2012-04-27 14:14:24 +0000
@@ -281,7 +281,7 @@
         node = factory.make_node(status=NODE_STATUS.READY)
         user = factory.make_user()
         token = create_auth_token(user)
-        node.acquire(token)
+        node.acquire(user, token)
         self.assertEqual(user, node.owner)
         self.assertEqual(NODE_STATUS.ALLOCATED, node.status)
 

=== modified file 'src/maasserver/tests/test_node_action.py'
--- src/maasserver/tests/test_node_action.py	2012-04-27 08:25:19 +0000
+++ src/maasserver/tests/test_node_action.py	2012-04-27 14:14:24 +0000
@@ -15,7 +15,6 @@
 from urlparse import urlparse
 
 from django.core.urlresolvers import reverse
-import maasserver.api
 from maasserver.enum import (
     NODE_PERMISSION,
     NODE_STATUS,
@@ -222,8 +221,6 @@
     def test_StartNode_acquires_and_starts_node(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)
         StartNode(node, user).execute()
         self.assertEqual(NODE_STATUS.ALLOCATED, node.status)
         self.assertEqual(user, node.owner)

=== modified file 'src/maasserver/tests/test_views_nodes.py'
--- src/maasserver/tests/test_views_nodes.py	2012-04-27 10:56:37 +0000
+++ src/maasserver/tests/test_views_nodes.py	2012-04-27 14:14:24 +0000
@@ -26,11 +26,11 @@
     )
 from maasserver.exceptions import NoRabbit
 from maasserver.forms import NodeActionForm
-from maasserver.node_action import StartNode
 from maasserver.models import (
     MACAddress,
     Node,
     )
+from maasserver.node_action import StartNode
 from maasserver.testing import (
     get_content_links,
     reload_object,
@@ -328,14 +328,6 @@
             "This node is now allocated to you.",
             '\n'.join(msg.message for msg in response.context['messages']))
 
-    def test_view_node_POST_without_oauth_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]),
-            data={NodeActionForm.input_name: StartNode.display})
-        self.assertEqual(httplib.UNAUTHORIZED, response.status_code)
-
 
 class NodeDeleteMacTest(LoggedInTestCase):