launchpad-reviewers team mailing list archive

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

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

Requested reviews:
  Launchpad code reviewers (launchpad-reviewers)

For more details, see:
https://code.launchpad.net/~jtv/maas/pre-987585-get_oauth_token/+merge/103312

I need this for a branch I'm working on, so while I'm at it, I might as well convert existing usage that repeats the same code.

This does replace one or two assertions with “server error” response codes, but given the nature of assertions, I don't think it matters much.


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

=== modified file 'src/maasserver/api.py'
--- src/maasserver/api.py	2012-04-20 15:16:41 +0000
+++ src/maasserver/api.py	2012-04-24 15:49:19 +0000
@@ -56,8 +56,8 @@
 __all__ = [
     "api_doc",
     "api_doc_title",
-    "extract_oauth_key",
     "generate_api_doc",
+    "get_oauth_token",
     "AccountHandler",
     "AnonNodesHandler",
     "FilesHandler",
@@ -293,7 +293,7 @@
         return value
 
 
-def extract_oauth_key(auth_data):
+def extract_oauth_key_from_auth_header(auth_data):
     """Extract the oauth key from auth data in HTTP header.
 
     :param auth_data: {string} The HTTP Authorization header.
@@ -309,9 +309,36 @@
     return None
 
 
+def extract_oauth_key(request):
+    """Extract the oauth key from a request's headers.
+
+    Raises :class:`Unauthorized` if no key is found.
+    """
+    auth_header = request.META.get('HTTP_AUTHORIZATION')
+    if auth_header is None:
+        raise Unauthorized("No authorization header received.")
+    key = extract_oauth_key_from_auth_header(auth_header)
+    if key is None:
+        raise Unauthorized("Did not find request's oauth token.")
+    return key
+
+
+def get_oauth_token(request):
+    """Get the OAuth :class:`piston.models.Token` used for `request`.
+
+    Raises :class:`Unauthorized` if no key is found, or
+    :class:`piston.models.Token.DoesNotExist` if the token is unknown.
+    """
+    return Token.objects.get(key=extract_oauth_key(request))
+
+
 NODE_FIELDS = (
-    'system_id', 'hostname', ('macaddress_set', ('mac_address',)),
-    'architecture', 'status')
+    'system_id',
+    'hostname',
+    ('macaddress_set', ('mac_address',)),
+    'architecture',
+    'status',
+    )
 
 
 @api_operations
@@ -548,17 +575,7 @@
     @api_exported('list_allocated', 'GET')
     def list_allocated(self, request):
         """Fetch Nodes that were allocated to the User/oauth token."""
-        auth_header = request.META.get("HTTP_AUTHORIZATION")
-        # A plain assertion is fine here because to get this far we
-        # should already have a valid authorization. If the assertion
-        # fails it is a genuine bug in the code and this will return a
-        # 500 response which is appropriate.
-        assert auth_header is not None, (
-            "HTTP_AUTHORIZATION not set on request")
-        key = extract_oauth_key(auth_header)
-        assert key is not None, (
-            "Invalid Authorization header on request.")
-        token = Token.objects.get(key=key)
+        token = get_oauth_token(request)
         match_ids = request.GET.getlist('id')
         if match_ids == []:
             match_ids = None
@@ -572,12 +589,7 @@
             request.user, constraints=extract_constraints(request.data))
         if node is None:
             raise NodesNotAvailable("No matching node is available.")
-        auth_header = request.META.get("HTTP_AUTHORIZATION")
-        assert auth_header is not None, (
-            "HTTP_AUTHORIZATION not set on request")
-        key = extract_oauth_key(auth_header)
-        token = Token.objects.get(key=key)
-        node.acquire(token)
+        node.acquire(get_oauth_token(request))
         node.save()
         return node
 

=== modified file 'src/maasserver/tests/test_api.py'
--- src/maasserver/tests/test_api.py	2012-04-23 10:13:57 +0000
+++ src/maasserver/tests/test_api.py	2012-04-24 15:49:19 +0000
@@ -13,6 +13,7 @@
 __all__ = []
 
 from base64 import b64encode
+from collections import namedtuple
 import httplib
 import json
 import os
@@ -27,6 +28,8 @@
 from maasserver.api import (
     extract_constraints,
     extract_oauth_key,
+    extract_oauth_key_from_auth_header,
+    get_oauth_token,
     )
 from maasserver.enum import (
     ARCHITECTURE_CHOICES,
@@ -34,6 +37,7 @@
     NODE_STATUS,
     NODE_STATUS_CHOICES_DICT,
     )
+from maasserver.exceptions import Unauthorized
 from maasserver.models import (
     Config,
     create_auth_token,
@@ -59,6 +63,7 @@
     NodeUserData,
     )
 from metadataserver.nodeinituser import get_node_init_user
+from piston.models import Token
 from provisioningserver.enum import POWER_TYPE
 
 
@@ -75,14 +80,53 @@
 
 class TestModuleHelpers(TestCase):
 
-    def test_extract_oauth_key_extracts_oauth_token_from_oauth_header(self):
-        token = factory.getRandomString(18)
-        self.assertEqual(
-            token,
-            extract_oauth_key(factory.make_oauth_header(oauth_token=token)))
-
-    def test_extract_oauth_key_returns_None_without_oauth_key(self):
-        self.assertIs(None, extract_oauth_key(''))
+    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_from_auth_header_returns_key(self):
+        token = factory.getRandomString(18)
+        self.assertEqual(
+            token,
+            extract_oauth_key_from_auth_header(
+                factory.make_oauth_header(oauth_token=token)))
+
+    def test_extract_oauth_key_from_auth_header_returns_None_if_missing(self):
+        self.assertIs(None, extract_oauth_key_from_auth_header(''))
+
+    def test_extract_oauth_key_raises_Unauthorized_if_no_auth_header(self):
+        self.assertRaises(
+            Unauthorized,
+            extract_oauth_key, self.make_fake_request(None))
+
+    def test_extract_oauth_key_raises_Unauthorized_if_no_key(self):
+        self.assertRaises(
+            Unauthorized,
+            extract_oauth_key, self.make_fake_request(''))
+
+    def test_extract_oauth_key_returns_key(self):
+        token = factory.getRandomString(18)
+        self.assertEqual(
+            token,
+            extract_oauth_key(self.make_fake_request(
+                factory.make_oauth_header(oauth_token=token))))
+
+    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_DoesNotExist_for_unknown_token(self):
+        fake_token = factory.getRandomString(18)
+        header = factory.make_oauth_header(oauth_token=fake_token)
+        self.assertRaises(
+            Token.DoesNotExist,
+            get_oauth_token, self.make_fake_request(header))
 
     def test_extract_constraints_ignores_unknown_parameters(self):
         unknown_parameter = "%s=%s" % (

=== modified file 'src/metadataserver/api.py'
--- src/metadataserver/api.py	2012-04-20 15:16:41 +0000
+++ src/metadataserver/api.py	2012-04-24 15:49:19 +0000
@@ -33,7 +33,6 @@
     MAASAPIBadRequest,
     MAASAPINotFound,
     NodeStateViolation,
-    Unauthorized,
     )
 from maasserver.models import SSHKey
 from metadataserver.models import (
@@ -55,12 +54,7 @@
 
 def get_node_for_request(request):
     """Return the `Node` that `request` is authorized to query for."""
-    auth_header = request.META.get('HTTP_AUTHORIZATION')
-    if auth_header is None:
-        raise Unauthorized("No authorization header received.")
-    key = extract_oauth_key(auth_header)
-    if key is None:
-        raise Unauthorized("No oauth token found for metadata request.")
+    key = extract_oauth_key(request)
     try:
         return NodeKey.objects.get_node_for_key(key)
     except NodeKey.DoesNotExist: