launchpad-reviewers team mailing list archive

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

Jeroen T. Vermeulen has proposed merging lp:~jtv/maas/extract-token-handling into lp:maas.

Requested reviews:
  Launchpad code reviewers (launchpad-reviewers)

For more details, see:
https://code.launchpad.net/~jtv/maas/extract-token-handling/+merge/93977

The metadata service needs to create and retrieve oauth tokens for the node-init user, which has no profile.  To support that, I'm lifting the code for those two things out of UserProfile.  One of those was already done; in this branch I add the other.  The UserProfile methods remain in place, but forward to these underlying calls.

This also means that detailed testing for creation and retrieval of tokens goes into a separate test case independent from UserProfile.  The UserProfile model test only needs superficial tests to verify that they do get called for the right user.

A final difference is that there's no point in create_auth_token (one of those extracted functions) to return a tuple of (consumer, token), since as the old tests established, consumer == token.consumer anyway.  Returning just the token helps keep tests short.
-- 
https://code.launchpad.net/~jtv/maas/extract-token-handling/+merge/93977
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~jtv/maas/extract-token-handling into lp:maas.

=== modified file 'src/maasserver/models.py'
--- src/maasserver/models.py	2012-02-17 14:20:02 +0000
+++ src/maasserver/models.py	2012-02-21 12:29:20 +0000
@@ -10,7 +10,9 @@
 
 __metaclass__ = type
 __all__ = [
+    "create_auth_token",
     "generate_node_system_id",
+    "get_auth_tokens",
     "FileStorage",
     "NODE_STATUS",
     "Node",
@@ -400,9 +402,8 @@
 
     :param user: The user to create a token for.
     :type user: User
-    :return: A tuple containing the Consumer and the Token that were
-        created.
-    :rtype: tuple
+    :return: The created Token.
+    :rtype: Token
 
     """
     consumer = Consumer.objects.create(
@@ -416,7 +417,21 @@
         user=user, token_type=Token.ACCESS, consumer=consumer,
         is_approved=True)
     token.generate_random_codes()
-    return consumer, token
+    return token
+
+
+def get_auth_tokens(user):
+    """Fetches all the user's OAuth tokens.
+
+    :return: A QuerySet of the tokens.
+    :rtype: django.db.models.query.QuerySet_
+
+    .. _django.db.models.query.QuerySet: https://docs.djangoproject.com/
+       en/dev/ref/models/querysets/
+
+    """
+    return Token.objects.select_related().filter(
+        user=user, token_type=Token.ACCESS, is_approved=True).order_by('id')
 
 
 class UserProfileManager(models.Manager):
@@ -474,9 +489,7 @@
            en/dev/ref/models/querysets/
 
         """
-        return Token.objects.select_related().filter(
-            user=self.user, token_type=Token.ACCESS,
-            is_approved=True).order_by('id')
+        return get_auth_tokens(self.user)
 
     def create_authorisation_token(self):
         """Create a new Token and its related Consumer (OAuth authorisation).
@@ -486,7 +499,8 @@
         :rtype: tuple
 
         """
-        return create_auth_token(self.user)
+        token = create_auth_token(self.user)
+        return token.consumer, token
 
     def delete_authorisation_token(self, token_key):
         """Delete the user's OAuth token wich key token_key.

=== modified file 'src/maasserver/tests/test_models.py'
--- src/maasserver/tests/test_models.py	2012-02-17 15:40:01 +0000
+++ src/maasserver/tests/test_models.py	2012-02-21 12:29:20 +0000
@@ -23,12 +23,14 @@
     PermissionDenied,
     )
 from maasserver.models import (
+    create_auth_token,
     GENERIC_CONSUMER,
-    SYSTEM_USERS,
+    get_auth_tokens,
     MACAddress,
     Node,
     NODE_STATUS,
     NODE_STATUS_CHOICES_DICT,
+    SYSTEM_USERS,
     UserProfile,
     )
 from maasserver.testing import TestCase
@@ -269,7 +271,8 @@
         self.assertRaises(ValidationError, mac.full_clean)
 
 
-class UserProfileTest(TestCase):
+class AuthTokensTest(TestCase):
+    """Test creation and retrieval of auth tokens."""
 
     def assertTokenValid(self, token):
         self.assertIsInstance(token.key, basestring)
@@ -282,6 +285,35 @@
         self.assertEqual(KEY_SIZE, len(consumer.key))
         self.assertEqual('', consumer.secret)
 
+    def test_create_auth_token(self):
+        user = factory.make_user()
+        token = create_auth_token(user)
+        self.assertEqual(user, token.user)
+        self.assertEqual(user, token.consumer.user)
+        self.assertTrue(token.is_approved)
+        self.assertConsumerValid(token.consumer)
+        self.assertTokenValid(token)
+
+    def test_get_auth_tokens_finds_tokens_for_user(self):
+        user = factory.make_user()
+        token = create_auth_token(user)
+        self.assertIn(token, get_auth_tokens(user))
+
+    def test_get_auth_tokens_ignores_other_users(self):
+        user, other_user = factory.make_user(), factory.make_user()
+        unrelated_token = create_auth_token(other_user)
+        self.assertNotIn(unrelated_token, get_auth_tokens(user))
+
+    def test_get_auth_tokens_ignores_unapproved_tokens(self):
+        user = factory.make_user()
+        token = create_auth_token(user)
+        token.is_approved = False
+        token.save()
+        self.assertNotIn(token, get_auth_tokens(user))
+
+
+class UserProfileTest(TestCase):
+
     def test_profile_creation(self):
         # A profile is created each time a user is created.
         user = factory.make_user()
@@ -293,37 +325,26 @@
         user = factory.make_user()
         consumers = Consumer.objects.filter(user=user, name=GENERIC_CONSUMER)
         self.assertEqual([user], [consumer.user for consumer in consumers])
-        self.assertConsumerValid(consumers[0])
 
     def test_token_creation(self):
         # A token is created each time a user is created.
         user = factory.make_user()
-        tokens = Token.objects.filter(user=user)
-        self.assertEqual([user], [token.user for token in tokens])
-        self.assertTokenValid(tokens[0])
+        [token] = get_auth_tokens(user)
+        self.assertEqual(user, token.user)
 
     def test_create_authorisation_token(self):
+        # UserProfile.create_authorisation_token calls create_auth_token.
         user = factory.make_user()
         profile = user.get_profile()
         consumer, token = profile.create_authorisation_token()
-        self.assertEqual(consumer, token.consumer)
         self.assertEqual(user, token.user)
         self.assertEqual(user, consumer.user)
-        self.assertConsumerValid(consumer)
-        self.assertTokenValid(token)
 
     def test_get_authorisation_tokens(self):
+        # UserProfile.get_authorisation_tokens calls get_auth_tokens.
         user = factory.make_user()
-        other_user = factory.make_user()
-        profile = user.get_profile()
-        other_profile = other_user.get_profile()
-        _, token = profile.create_authorisation_token()
-        other_profile.create_authorisation_token()
-        tokens = profile.get_authorisation_tokens()
-        # This user has 2 tokens: the one that was created automatically
-        # when the user was created plus the one we've created manually.
-        self.assertEqual(2, tokens.count())
-        self.assertEqual(token, list(tokens.order_by('id'))[1])
+        consumer, token = user.get_profile().create_authorisation_token()
+        self.assertIn(token, user.get_profile().get_authorisation_tokens())
 
     def test_delete(self):
         # Deleting a profile also deletes the related user.

=== modified file 'src/metadataserver/models.py'
--- src/metadataserver/models.py	2012-02-16 13:45:37 +0000
+++ src/metadataserver/models.py	2012-02-21 12:29:20 +0000
@@ -39,14 +39,14 @@
         :param node: The system that is to be allowed access to the metadata
             service.
         :type node: Node
-        :return: Consumer and Token for the node to use, attached to the
+        :return: Token for the node to use.  It will belong to the
             maas-init-node user.  If passed the token's key,
             `get_node_for_key` will return `node`.
-        :rtype tuple:
+        :rtype: Token
         """
-        consumer, token = create_auth_token(get_node_init_user())
+        token = create_auth_token(get_node_init_user())
         self.create(node=node, key=token.key)
-        return consumer, token
+        return token
 
     def get_node_for_key(self, key):
         """Find the Node that `key` was created for.

=== modified file 'src/metadataserver/tests/test_api.py'
--- src/metadataserver/tests/test_api.py	2012-02-16 14:09:01 +0000
+++ src/metadataserver/tests/test_api.py	2012-02-21 12:29:20 +0000
@@ -57,7 +57,7 @@
 
     def make_node_and_auth_header(self):
         node = factory.make_node()
-        consumer, token = NodeKey.objects.create_token(node)
+        token = NodeKey.objects.create_token(node)
         header = 'oauth_token=%s' % token.key
         return node, header
 

=== modified file 'src/metadataserver/tests/test_models.py'
--- src/metadataserver/tests/test_models.py	2012-02-16 13:45:37 +0000
+++ src/metadataserver/tests/test_models.py	2012-02-21 12:29:20 +0000
@@ -21,13 +21,13 @@
 
     def test_create_key_registers_node_key(self):
         node = factory.make_node()
-        consumer, token = NodeKey.objects.create_token(node)
+        token = NodeKey.objects.create_token(node)
         nodekey = NodeKey.objects.get(node=node, key=token.key)
         self.assertNotEqual(None, nodekey)
 
     def test_get_node_for_key_finds_node(self):
         node = factory.make_node()
-        consumer, token = NodeKey.objects.create_token(node)
+        token = NodeKey.objects.create_token(node)
         self.assertEqual(node, NodeKey.objects.get_node_for_key(token.key))
 
     def test_get_node_for_key_raises_DoesNotExist_if_key_not_found(self):