Merge lp:~aym-frikha/maas/add-apikey-name-api into lp:~maas-committers/maas/trunk

[Aymen Frikha]

API keys can have now names assigned to them.

 * Users are now able to generate new API with a name, using:
[API] maas ubuntu account create-authorisation-token name=TestKey
[CLI] maas apikey --username=ubuntu --generate --name TestKey

 * Users can also update existing API keys name, using:
[API] maas ubuntu account update-token-name token={{token key}} name=TestKey
[CLI] maas apikey --username=ubuntu --update {{token key}} --name TestKey

* Users are now able to list API keys with their names, using:
[API] maas ubuntu account list-authorisation-tokens
[CLI] maas apikey --username=ubuntu --with-names

If a new API key is generated without any name, the existing default name (which was never exposed to the user before) is assigned to it: "MAAS Consumer". (All existing keys will also have this name assigned to them.)

[Mike Pontillo (mpontillo)]

The code looks generally good. I've got some comments below, mostly about the names of user-facing strings/documentation. (and some code style comments.)

review: Needs Fixing

[Mike Pontillo (mpontillo)]

I think this branch is looking really good. Just a few more minor nits below, mostly with regard to code style.

[Mike Pontillo (mpontillo)]

I just finished testing this branch and I think it's looking great. Here's a transcript.

http://paste.ubuntu.com/23289946/

There is one more thing I would change before I think it's okay to land this: we should swap the order of the output in "maas apikey --username admin --with-names". For example, right now I see:

MAAS consumer 9bU8Xw8GqvAEyrfjse:xXSyGnWYAZFWhEjPUq:G4fv6WnQ2KtH73c5gZ4byBqmAc3Dmzwg
Oh, I just remembered. kT85sFzqcNTckx6hNY:TUaQBgL4NK5enVkQec:czDGFca2kNV8EGrugjVSBAjcDc5zkLtQ

It would be easier for someone writing a script to parse this if we place the name *after* the key, since then you always know that everything after the first space is the key name (since key names can contain spaces).

So I would rather the output look like this:

9bU8Xw8GqvAEyrfjse:xXSyGnWYAZFWhEjPUq:G4fv6WnQ2KtH73c5gZ4byBqmAc3Dmzwg MAAS consumer
kT85sFzqcNTckx6hNY:TUaQBgL4NK5enVkQec:czDGFca2kNV8EGrugjVSBAjcDc5zkLtQ Oh, I just remembered.

It also looks better this way since the keys line up.

Thank you for your hard work and dedication on this.

review: Approve

=== modified file 'src/maasserver/api/account.py'
--- src/maasserver/api/account.py	2016-08-09 21:59:28 +0000
+++ src/maasserver/api/account.py	2016-10-09 22:36:13 +0000
@@ -5,7 +5,7 @@
 
 __all__ = [
     'AccountHandler',
-    ]
+]
 
 import http.client
 import json
@@ -15,10 +15,27 @@
     operation,
     OperationsHandler,
 )
-from maasserver.api.utils import get_mandatory_param
+from maasserver.api.utils import (
+    get_mandatory_param,
+    get_optional_param,
+)
 from piston3.utils import rc
 
 
+def _format_tokens(tokens):
+    """Converts the given tokens into a list of dictionaries to represent them.
+
+    :param tokens: The result of `profile.get_authorisation_tokens()`.
+    """
+    return [
+        {
+            "name": token.consumer.name,
+            "token": ":".join([token.consumer.key, token.key, token.secret])
+        }
+        for token in tokens
+    ]
+
+
 class AccountHandler(OperationsHandler):
     """Manage the current logged-in user."""
     api_doc_section_name = "Logged-in user"
@@ -28,19 +45,22 @@
     def create_authorisation_token(self, request):
         """Create an authorisation OAuth token and OAuth consumer.
 
-        :return: a json dict with three keys: 'token_key',
-            'token_secret' and 'consumer_key' (e.g.
+        :param name: Optional name of the token that will be generated.
+        :type name: unicode
+        :return: a json dict with four keys: 'token_key',
+            'token_secret', 'consumer_key'  and 'name'(e.g.
             {token_key: 's65244576fgqs', token_secret: 'qsdfdhv34',
-            consumer_key: '68543fhj854fg'}).
+            consumer_key: '68543fhj854fg', name: 'MAAS consumer'}).
         :rtype: string (json)
 
         """
         profile = request.user.userprofile
-        consumer, token = profile.create_authorisation_token()
+        consumer_name = get_optional_param(request.data, 'name')
+        consumer, token = profile.create_authorisation_token(consumer_name)
         auth_info = {
             'token_key': token.key, 'token_secret': token.secret,
-            'consumer_key': consumer.key,
-            }
+            'consumer_key': consumer.key, 'name': consumer.name
+        }
         return HttpResponse(
             json.dumps(auth_info),
             content_type='application/json; charset=utf-8',
@@ -58,6 +78,39 @@
         profile.delete_authorisation_token(token_key)
         return rc.DELETED
 
+    @operation(idempotent=False)
+    def update_token_name(self, request):
+        """Modify the consumer name of an authorisation OAuth token.
+
+        :param token: Can be the whole token or only the token key.
+        :type token: unicode
+        :param name: New name of the token.
+        :type name: unicode
+        """
+        profile = request.user.userprofile
+        token = get_mandatory_param(request.data, 'token')
+        token_fields = token.split(":")
+        if len(token_fields) == 3:
+            token_key = token_fields[1]
+        else:
+            token_key = token
+        consumer_name = get_mandatory_param(request.data, 'name')
+        profile.modify_consumer_name(token_key, consumer_name)
+        return rc.ACCEPTED
+
+    @operation(idempotent=True)
+    def list_authorisation_tokens(self, request):
+        """List authorisation tokens available to the currently logged-in user.
+
+        :return: list of dictionaries representing each key's name and token.
+        """
+        profile = request.user.userprofile
+        tokens = _format_tokens(profile.get_authorisation_tokens())
+        return HttpResponse(
+            json.dumps(tokens, indent=4),
+            content_type='application/json; charset=utf-8',
+            status=int(http.client.OK))
+
     @classmethod
     def resource_uri(cls, *args, **kwargs):
         return ('account_handler', [])

=== modified file 'src/maasserver/api/tests/test_api.py'
--- src/maasserver/api/tests/test_api.py	2016-09-23 19:21:31 +0000
+++ src/maasserver/api/tests/test_api.py	2016-10-09 22:36:13 +0000
@@ -181,7 +181,8 @@
 
     def test_create_authorisation_token(self):
         # The api operation create_authorisation_token returns a json dict
-        # with the consumer_key, the token_key and the token_secret in it.
+        # with the consumer_key, the token_key, the token_secret and the
+        # consumer_name in it.
         response = self.client.post(
             reverse('account_handler'), {'op': 'create_authorisation_token'})
         self.assertEqual(http.client.OK, response.status_code)
@@ -189,11 +190,27 @@
             'application/json; charset=utf-8', response["content-type"])
         parsed_result = json_load_bytes(response.content)
         self.assertEqual(
-            ['consumer_key', 'token_key', 'token_secret'],
+            ['consumer_key', 'name', 'token_key', 'token_secret'],
             sorted(parsed_result))
         self.assertIsInstance(parsed_result['consumer_key'], str)
         self.assertIsInstance(parsed_result['token_key'], str)
         self.assertIsInstance(parsed_result['token_secret'], str)
+        self.assertIsInstance(parsed_result['name'], str)
+
+    def test_create_authorisation_token_with_token_name(self):
+        # The api operation create_authorisation_token can also accept
+        # a new name for the generated token.
+        token_name = 'Test_Token'
+        response = self.client.post(
+            reverse('account_handler'), {
+                'op': 'create_authorisation_token',
+                'name': token_name
+            })
+        self.assertEqual(http.client.OK, response.status_code)
+        self.assertEqual(
+            'application/json; charset=utf-8', response["content-type"])
+        parsed_result = json_load_bytes(response.content)
+        self.assertEqual(parsed_result['name'], token_name)
 
     def test_delete_authorisation_token_not_found(self):
         # If the provided token_key does not exist (for the currently
@@ -209,9 +226,131 @@
         # delete_authorisation_token. It it is not present in the request's
         # parameters, the api returns a 'Bad Request' (400) error.
         response = self.client.post(
-            reverse('account_handler'), {'op': 'delete_authorisation_token'})
-
-        self.assertEqual(http.client.BAD_REQUEST, response.status_code)
+            reverse('account_handler'), {
+                'op': 'delete_authorisation_token'
+            })
+
+        self.assertEqual(http.client.BAD_REQUEST, response.status_code)
+
+    def test_update_authorisation_token(self):
+        token_name_orig = 'Test_Token'
+        token_name_updated = 'Test_Token update'
+        response_creation = self.client.post(
+            reverse('account_handler'), {
+                'op': 'create_authorisation_token',
+                'name': token_name_orig
+            })
+        parsed_creation_response = json_load_bytes(response_creation.content)
+        created_token = ":".join([
+            parsed_creation_response['consumer_key'],
+            parsed_creation_response['token_key'],
+            parsed_creation_response['token_secret']
+        ])
+        self.client.post(
+            reverse('account_handler'), {
+                'op': 'update_token_name', 'token': created_token,
+                'name': token_name_updated
+            })
+        response_list = self.client.get(
+            reverse('account_handler'), {
+                'op': 'list_authorisation_tokens'
+            })
+        parsed_list_response = json_load_bytes(response_list.content)
+        for token in parsed_list_response:
+            if token['token'] == created_token:
+                self.assertEqual(token['name'], token_name_updated)
+
+    def test_update_authorisation_token_with_token_key(self):
+        # We use only "token_key" portion of the authorisation token
+        # to update the token name.
+        token_name_orig = 'Test_Token'
+        token_name_updated = 'Test_Token update'
+        response_creation = self.client.post(
+            reverse('account_handler'), {
+                'op': 'create_authorisation_token',
+                'name': token_name_orig
+            })
+        parsed_creation_response = json_load_bytes(response_creation.content)
+        created_token = ":".join([
+            parsed_creation_response['consumer_key'],
+            parsed_creation_response['token_key'],
+            parsed_creation_response['token_secret']
+        ])
+        self.client.post(
+            reverse('account_handler'), {
+                'op': 'update_token_name',
+                'token': parsed_creation_response['token_key'],
+                'name': token_name_updated
+            })
+        response_list = self.client.get(
+            reverse('account_handler'), {
+                'op': 'list_authorisation_tokens'
+            })
+        parsed_list_response = json_load_bytes(response_list.content)
+        for token in parsed_list_response:
+            if token['token'] == created_token:
+                self.assertEqual(token['name'], token_name_updated)
+
+    def test_update_authorisation_token_name_not_found(self):
+        # If the provided token_key does not exist (for the currently
+        # logged-in user), the api returns a 'Not Found' (404) error.
+        response = self.client.post(
+            reverse('account_handler'), {
+                'op': 'update_token_name', 'token': 'no-such-token',
+                'name': 'test_name'
+            })
+
+        self.assertEqual(http.client.NOT_FOUND, response.status_code)
+
+    def test_update_authorisation_token_name_bad_request_no_token(self):
+        # `token` and `name` are mandatory parameters when calling
+        # update_token_name. If it is not present in the request's
+        # parameters, the api returns a 'Bad Request' (400) error.
+        response = self.client.post(
+            reverse('account_handler'), {
+                'op': 'update_token_name'
+            })
+
+        self.assertEqual(http.client.BAD_REQUEST, response.status_code)
+
+    def test_list_tokens(self):
+        token1_name = "Test Token 1"
+        response_creation = self.client.post(
+            reverse('account_handler'), {
+                'op': 'create_authorisation_token',
+                'name': token1_name
+            })
+        parsed_creation_response = json_load_bytes(response_creation.content)
+        response = self.client.get(
+            reverse('account_handler'), {
+                'op': 'list_authorisation_tokens'
+            })
+        parsed_list_response = json_load_bytes(response.content)
+        self.assertEqual(len(json_load_bytes(response.content)), 2)
+        for token in parsed_list_response:
+            if token['name'] == token1_name:
+                token_fields = token['token'].split(":")
+                self.assertEqual(
+                    token_fields[0],
+                    parsed_creation_response['consumer_key'])
+                self.assertEqual(
+                    token_fields[1],
+                    parsed_creation_response['token_key'])
+                self.assertEqual(
+                    token_fields[2],
+                    parsed_creation_response['token_secret'])
+
+    def test_list_tokens_format(self):
+        self.client.post(
+            reverse('account_handler'), {'op': 'create_authorisation_token'})
+        response = self.client.get(
+            reverse('account_handler'), {'op': 'list_authorisation_tokens'})
+        parsed_list_response = json_load_bytes(response.content)
+        self.assertIsInstance(parsed_list_response, list)
+        for token in parsed_list_response:
+            self.assertEqual(['name', 'token'], sorted(token))
+            self.assertIsInstance(token['name'], str)
+            self.assertIsInstance(token['token'], str)
 
 
 class TestSSHKeyHandlers(APITestCase.ForUser):

=== modified file 'src/maasserver/management/commands/apikey.py'
--- src/maasserver/management/commands/apikey.py	2016-06-22 17:03:02 +0000
+++ src/maasserver/management/commands/apikey.py	2016-10-09 22:36:13 +0000
@@ -36,17 +36,34 @@
         parser.add_argument(
             '--delete', default=None,
             help="Delete the supplied api key.")
+        parser.add_argument(
+            '--update', default=None,
+            help="Update the supplied api key name")
+        parser.add_argument(
+            '--name', default=None, dest="api_key_name",
+            help="Name of the token. This argument should be passed to "
+                 "--update or --generate options")
+        parser.add_argument(
+            '--with-names ', dest="with_names", action="store_true",
+            help="Display tokens with their names.")
 
     def _print_token(self, token):
-        """Write `token` to stdout in the standard format."""
-        self.stdout.write(convert_tuple_to_string(get_creds_tuple(token)))
+        """Write `token` to stdout in the standard format (with names if
+        --with-names option is enabled)"""
+        if self.display_names:
+            self.stdout.write("%s %s" % (
+                convert_tuple_to_string(get_creds_tuple(token)),
+                token.consumer.name
+            ))
+        else:
+            self.stdout.write(convert_tuple_to_string(get_creds_tuple(token)))
         # In Django 1.5+, self.stdout.write() adds a newline character at
         # the end of the message.
         if django.VERSION < (1, 5):
             self.stdout.write('\n')
 
-    def _generate_token(self, user):
-        _, token = user.userprofile.create_authorisation_token()
+    def _generate_token(self, user, consumer_name=None):
+        _, token = user.userprofile.create_authorisation_token(consumer_name)
         self._print_token(token)
 
     def _delete_token(self, user, key_to_delete):
@@ -61,6 +78,17 @@
         except Http404:
             raise CommandError("No matching api key found.")
 
+    def _update_token(self, user, key_to_update, consumer_name):
+        try:
+            creds_tuple = convert_string_to_tuple(key_to_update)
+        except ValueError as e:
+            raise CommandError(e)
+        _, token_key, _ = creds_tuple
+        try:
+            user.userprofile.modify_consumer_name(token_key, consumer_name)
+        except Http404:
+            raise CommandError("No matching api key found.")
+
     def handle(self, *args, **options):
         username = options.get('username', None)
         if username is None:
@@ -68,16 +96,30 @@
 
         generate = options.get('generate')
         key_to_delete = options.get('delete', None)
+        key_to_update = options.get('update', None)
+        consumer_name = options.get('api_key_name', None)
+        self.display_names = options.get('with_names', None)
+
         if generate and key_to_delete is not None:
             raise CommandError("Specify one of --generate or --delete.")
 
+        if generate and key_to_update is not None:
+            raise CommandError("Specify one of --generate or --update.")
+
+        if key_to_delete and key_to_update is not None:
+            raise CommandError("Specify one of --delete or --update.")
+
+        if key_to_update is not None and consumer_name is None:
+            raise CommandError("Should specify new name for the updated key .")
+
         user = get_one(User.objects.filter(username=username))
+
         if user is None:
             raise CommandError("User does not exist.")
 
         if generate:
             # Generate a new api key.
-            self._generate_token(user)
+            self._generate_token(user, consumer_name)
             return
 
         elif key_to_delete is not None:
@@ -85,6 +127,11 @@
             self._delete_token(user, key_to_delete)
             return
 
+        elif key_to_update is not None:
+            # Update an existing key name.
+            self._update_token(user, key_to_update, consumer_name)
+            return
+
         else:
             # No mutating action requested, so just print existing keys.
             tokens = user.userprofile.get_authorisation_tokens()

=== modified file 'src/maasserver/models/user.py'
--- src/maasserver/models/user.py	2015-12-02 20:43:30 +0000
+++ src/maasserver/models/user.py	2016-10-09 22:36:13 +0000
@@ -29,17 +29,21 @@
 GENERIC_CONSUMER = 'MAAS consumer'
 
 
-def create_auth_token(user):
+def create_auth_token(user, consumer_name=None):
     """Create new Token and Consumer (OAuth authorisation) for `user`.
 
     :param user: The user to create a token for.
     :type user: User
+    :param consumer_name: Name of the consumer to be assigned to the newly
+     generated token.
     :return: The created Token.
     :rtype: piston.models.Token
 
     """
+    if consumer_name is None:
+        consumer_name = GENERIC_CONSUMER
     consumer = Consumer.objects.create(
-        user=user, name=GENERIC_CONSUMER, status='accepted')
+        user=user, name=consumer_name, status='accepted')
     consumer.generate_random_codes()
     # This is a 'generic' consumer aimed to service many clients, hence
     # we don't authenticate the consumer with key/secret key.

=== modified file 'src/maasserver/models/userprofile.py'
--- src/maasserver/models/userprofile.py	2016-09-22 02:53:33 +0000
+++ src/maasserver/models/userprofile.py	2016-10-09 22:36:13 +0000
@@ -92,7 +92,7 @@
 
         return get_auth_tokens(self.user)
 
-    def create_authorisation_token(self):
+    def create_authorisation_token(self, consumer_name=None):
         """Create a new Token and its related Consumer (OAuth authorisation).
 
         :return: A tuple containing the Consumer and the Token that were
@@ -103,7 +103,7 @@
         # Avoid circular imports.
         from maasserver.models.user import create_auth_token
 
-        token = create_auth_token(self.user)
+        token = create_auth_token(self.user, consumer_name)
         return token.consumer, token
 
     def delete_authorisation_token(self, token_key):
@@ -119,5 +119,20 @@
         token.consumer.delete()
         token.delete()
 
+    def modify_consumer_name(self, token_key, consumer_name):
+        """Modify consumer name of an existing token key.
+
+        :param token_key: The key of the token to be deleted.
+        :type token_key: string
+        :param consumer_name: Name of the token consumer.
+        :type consumer_name: string
+        :raises: `django.http.Http404`
+        """
+        token = get_object_or_404(
+            Token, user=self.user, token_type=Token.ACCESS, key=token_key)
+        token.consumer.name = consumer_name
+        token.consumer.save()
+        token.save()
+
     def __str__(self):
         return self.user.username

=== modified file 'src/maasserver/tests/test_commands.py'
--- src/maasserver/tests/test_commands.py	2016-09-23 18:03:06 +0000
+++ src/maasserver/tests/test_commands.py	2016-10-09 22:36:13 +0000
@@ -427,3 +427,59 @@
             self, 'apikey', username=user.username, delete=token_string)
         self.assertIn(
             "No matching api key found", error_text)
+
+    def test_success_modify_apikey_name(self):
+        stderr = StringIO()
+        stdout = StringIO()
+        fake_api_key_name = "Test Key Name"
+        user = factory.make_User()
+        existing_token = get_one(
+            user.userprofile.get_authorisation_tokens())
+        token_string = convert_tuple_to_string(
+            get_creds_tuple(existing_token))
+        call_command(
+            'apikey', username=user.username, update=token_string,
+            api_key_name=fake_api_key_name, stderr=stderr, stdout=stdout)
+        self.assertThat(stderr, IsEmpty)
+
+    def test_api_key_rejects_update_of_nonexistent_key(self):
+        stderr = StringIO()
+        user = factory.make_User()
+        fake_api_key_name = "Test Key Name"
+        existing_token = get_one(
+            user.userprofile.get_authorisation_tokens())
+        token_string = convert_tuple_to_string(
+            get_creds_tuple(existing_token))
+        call_command(
+            'apikey', username=user.username, delete=token_string,
+            stderr=stderr)
+        self.assertThat(stderr, IsEmpty)
+
+        # Try to update the deleted token.
+        error_text = assertCommandErrors(
+            self, 'apikey', username=user.username, update=token_string,
+            api_key_name=fake_api_key_name)
+        self.assertIn(
+            "No matching api key found", error_text)
+
+    def test_api_key_rejects_update_without_key_name(self):
+        user = factory.make_User()
+        existing_token = get_one(
+            user.userprofile.get_authorisation_tokens())
+        token_string = convert_tuple_to_string(
+            get_creds_tuple(existing_token))
+        error_text = assertCommandErrors(
+            self, 'apikey', username=user.username, update=token_string)
+        self.assertIn("Should specify new name", error_text)
+
+    def test_api_key_update_and_generate_mutually_exclusive_options(self):
+        user = factory.make_User()
+        fake_api_key_name = "Test Key Name"
+        existing_token = get_one(
+            user.userprofile.get_authorisation_tokens())
+        token_string = convert_tuple_to_string(
+            get_creds_tuple(existing_token))
+        error_text = assertCommandErrors(
+            self, 'apikey', username=user.username, generate=True,
+            api_key_name=fake_api_key_name, update=token_string)
+        self.assertIn("Specify one of --generate or --update.", error_text)