launchpad-reviewers team mailing list archive
-
launchpad-reviewers team
-
Mailing list archive
-
Message #10865
[Merge] lp:~jtv/maas/factor-api-creds-formats into lp:maas
Jeroen T. Vermeulen has proposed merging lp:~jtv/maas/factor-api-creds-formats into lp:maas.
Requested reviews:
Launchpad code reviewers (launchpad-reviewers)
For more details, see:
https://code.launchpad.net/~jtv/maas/factor-api-creds-formats/+merge/119301
This is the outcome of a review remark by Julian (with whom I also discussed the solution you see here). Our API credentials come in 3 forms: a Token model object with its associated Consumer; a tuple of 3 items extracted from the Token and Consumer; and that same tuple represented as a colon-separated string.
In this branch I extract the conversions:
* maasserver.models.user.get_creds_tuple extracts the triplet from a Token.
* apiclient.creds.convert_tuple_to_string represents the tuple as a colon-separated string.
* apiclient.creds.convert_string_to_tuple reconstructs the tuple from a colon-separated string.
The reasoning behind this division of labour is that of all the code that needed this, only maasserver has access to the Token object so that's where the first one should go. The second is used by maasserver as well but tightly coupled to the third, so they should be colocated. The third conversion is used in the worker, not maasserver, which makes this ugly. And importing maasserver items into worker code is not good practice, so where do we put these conversions? Ultimately they are for the API client's benefit, so the API client might as well specify a single canonical string format for the tuple. We may find places in Juju or cloud-init that could benefit from reuse later.
The conversion from Token to colon-separated string that occurred in two places is now a double-step process. It just didn't seem worthwhile to provide a shortcut for that.
We also have one other conversion, from 3 individual parameters to a colon-separated string, in prefs.js. But that is its own function, not entangled with anything else, and I felt it was fine just where it is.
Jeroen
--
https://code.launchpad.net/~jtv/maas/factor-api-creds-formats/+merge/119301
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~jtv/maas/factor-api-creds-formats into lp:maas.
=== added file 'src/apiclient/creds.py'
--- src/apiclient/creds.py 1970-01-01 00:00:00 +0000
+++ src/apiclient/creds.py 2012-08-13 05:53:32 +0000
@@ -0,0 +1,46 @@
+# Copyright 2012 Canonical Ltd. This software is licensed under the
+# GNU Affero General Public License version 3 (see the file LICENSE).
+
+"""Handling of MAAS API credentials.
+
+The API client deals with credentials consisting of 3 elements: consumer
+key, resource token, and resource secret. These are in OAuth, but the
+consumer secret is hardwired to the empty string.
+
+Credentials are represented internally as tuples of these three elements,
+but can also be converted to a colon-separated string format for easy
+transport between processes.
+"""
+
+from __future__ import (
+ absolute_import,
+ print_function,
+ unicode_literals,
+ )
+
+__metaclass__ = type
+__all__ = [
+ 'convert_string_to_tuple',
+ 'convert_tuple_to_string',
+ ]
+
+
+def convert_tuple_to_string(creds_tuple):
+ """Represent a MAAS API credentials tuple as a colon-separated string."""
+ if len(creds_tuple) != 3:
+ raise ValueError(
+ "Credentials tuple does not consist of 3 elements as expected; "
+ "it contains %d."
+ % len(creds_tuple))
+ return ':'.join(creds_tuple)
+
+
+def convert_string_to_tuple(creds_string):
+ """Recreate a MAAS API credentials tuple from a colon-separated string."""
+ creds_tuple = tuple(creds_string.split(':'))
+ if len(creds_tuple) != 3:
+ raise ValueError(
+ "Malformed credentials string. Expected 3 colon-separated items, "
+ "got '%s'"
+ % creds_string)
+ return creds_tuple
=== added file 'src/apiclient/tests/test_creds.py'
--- src/apiclient/tests/test_creds.py 1970-01-01 00:00:00 +0000
+++ src/apiclient/tests/test_creds.py 2012-08-13 05:53:32 +0000
@@ -0,0 +1,72 @@
+# Copyright 2012 Canonical Ltd. This software is licensed under the
+# GNU Affero General Public License version 3 (see the file LICENSE).
+
+"""Tests for handling of MAAS API credentials."""
+
+from __future__ import (
+ absolute_import,
+ print_function,
+ unicode_literals,
+ )
+
+__metaclass__ = type
+__all__ = []
+
+from apiclient.creds import (
+ convert_string_to_tuple,
+ convert_tuple_to_string,
+ )
+from maastesting.factory import factory
+from maastesting.testcase import TestCase
+
+
+class TestCreds(TestCase):
+
+ def make_tuple(self):
+ return (
+ factory.make_name('consumer-key'),
+ factory.make_name('resource-token'),
+ factory.make_name('resource-secret'),
+ )
+
+ def test_convert_tuple_to_string_converts_tuple_to_string(self):
+ creds_tuple = self.make_tuple()
+ self.assertEqual(
+ ':'.join(creds_tuple), convert_tuple_to_string(creds_tuple))
+
+ def test_convert_tuple_to_string_rejects_undersized_tuple(self):
+ self.assertRaises(
+ ValueError,
+ convert_tuple_to_string,
+ self.make_tuple()[:-1])
+
+ def test_convert_tuple_to_string_rejects_oversized_tuple(self):
+ self.assertRaises(
+ ValueError,
+ convert_tuple_to_string,
+ self.make_tuple() + self.make_tuple()[:1])
+
+ def test_convert_string_to_tuple_converts_string_to_tuple(self):
+ creds_tuple = self.make_tuple()
+ creds_string = ':'.join(creds_tuple)
+ self.assertEqual(creds_tuple, convert_string_to_tuple(creds_string))
+
+ def test_convert_string_to_tuple_detects_malformed_string(self):
+ broken_tuple = self.make_tuple()[:-1]
+ self.assertRaises(
+ ValueError,
+ convert_string_to_tuple,
+ ':'.join(broken_tuple))
+
+ def test_convert_string_to_tuple_detects_spurious_colons(self):
+ broken_tuple = self.make_tuple() + self.make_tuple()[:1]
+ self.assertRaises(
+ ValueError,
+ convert_string_to_tuple,
+ ':'.join(broken_tuple))
+
+ def test_convert_string_to_tuple_inverts_convert_tuple_to_string(self):
+ creds_tuple = self.make_tuple()
+ self.assertEqual(
+ creds_tuple,
+ convert_string_to_tuple(convert_tuple_to_string(creds_tuple)))
=== modified file 'src/maasserver/models/user.py'
--- src/maasserver/models/user.py 2012-07-17 08:19:33 +0000
+++ src/maasserver/models/user.py 2012-08-13 05:53:32 +0000
@@ -14,6 +14,7 @@
'create_auth_token',
'create_user',
'get_auth_tokens',
+ 'get_creds_tuple',
]
from maasserver import worker_user
@@ -84,3 +85,16 @@
# Create initial authorisation token.
profile.create_authorisation_token()
+
+
+def get_creds_tuple(token):
+ """Return API credentials as tuple, as used in :class:`MAASOAuth`.
+
+ Returns a tuple of (consumer key, resource token, resource secret).
+ The consumer secret is hard-wired to the empty string.
+ """
+ return (
+ token.consumer.key,
+ token.key,
+ token.secret,
+ )
=== modified file 'src/maasserver/refresh_worker.py'
--- src/maasserver/refresh_worker.py 2012-08-10 15:41:52 +0000
+++ src/maasserver/refresh_worker.py 2012-08-13 05:53:32 +0000
@@ -14,6 +14,8 @@
'refresh_worker',
]
+from apiclient.creds import convert_tuple_to_string
+from maasserver.models.user import get_creds_tuple
from provisioningserver.tasks import refresh_secrets
@@ -35,11 +37,8 @@
if nodegroup.dhcp_key is not None and len(nodegroup.dhcp_key) > 0:
items['omapi_shared_key'] = nodegroup.dhcp_key
- items['api_credentials'] = ':'.join([
- nodegroup.api_token.consumer.key,
- nodegroup.api_token.key,
- nodegroup.api_token.secret,
- ])
+ items['api_credentials'] = convert_tuple_to_string(
+ get_creds_tuple(nodegroup.api_token))
# TODO: Route this to the right worker, once we have multiple.
refresh_secrets.delay(**items)
=== modified file 'src/maasserver/tests/test_refresh_worker.py'
--- src/maasserver/tests/test_refresh_worker.py 2012-08-10 15:37:55 +0000
+++ src/maasserver/tests/test_refresh_worker.py 2012-08-13 05:53:32 +0000
@@ -12,6 +12,8 @@
__metaclass__ = type
__all__ = []
+from apiclient.creds import convert_tuple_to_string
+from maasserver.models.user import get_creds_tuple
from maasserver.refresh_worker import refresh_worker
from maasserver.testing.factory import factory
from maasserver.testing.testcase import TestCase
@@ -42,11 +44,8 @@
refresh_functions = self.patch_refresh_functions()
nodegroup = factory.make_node_group()
refresh_worker(nodegroup)
- creds_string = ':'.join([
- nodegroup.api_token.consumer.key,
- nodegroup.api_token.key,
- nodegroup.api_token.secret,
- ])
+ creds_string = convert_tuple_to_string(
+ get_creds_tuple(nodegroup.api_token))
self.assertEqual(
[(creds_string, )],
refresh_functions['api_credentials'].extract_args())
=== modified file 'src/maasserver/tests/test_user.py'
--- src/maasserver/tests/test_user.py 2012-06-20 17:40:53 +0000
+++ src/maasserver/tests/test_user.py 2012-08-13 05:53:32 +0000
@@ -12,9 +12,14 @@
__metaclass__ = type
__all__ = []
+from apiclient.creds import (
+ convert_string_to_tuple,
+ convert_tuple_to_string,
+ )
from maasserver.models.user import (
create_auth_token,
get_auth_tokens,
+ get_creds_tuple,
)
from maasserver.testing.factory import factory
from maasserver.testing.testcase import TestCase
@@ -63,3 +68,15 @@
token.is_approved = False
token.save()
self.assertNotIn(token, get_auth_tokens(user))
+
+ def test_get_creds_tuple_returns_creds(self):
+ token = create_auth_token(factory.make_user())
+ self.assertEqual(
+ (token.consumer.key, token.key, token.secret),
+ get_creds_tuple(token))
+
+ def test_get_creds_tuple_integrates_with_api_client(self):
+ creds_tuple = get_creds_tuple(create_auth_token(factory.make_user()))
+ self.assertEqual(
+ creds_tuple,
+ convert_string_to_tuple(convert_tuple_to_string(creds_tuple)))
=== modified file 'src/maasserver/tests/test_views_prefs.py'
--- src/maasserver/tests/test_views_prefs.py 2012-04-27 12:38:18 +0000
+++ src/maasserver/tests/test_views_prefs.py 2012-08-13 05:53:32 +0000
@@ -15,10 +15,12 @@
import httplib
+from apiclient.creds import convert_tuple_to_string
from django.contrib.auth.models import User
from django.core.urlresolvers import reverse
from lxml.html import fromstring
from maasserver.models import SSHKey
+from maasserver.models.user import get_creds_tuple
from maasserver.testing import (
extract_redirect,
get_content_links,
@@ -54,11 +56,9 @@
doc = fromstring(response.content)
# The OAuth tokens are displayed.
for token in user.get_profile().get_authorisation_tokens():
- consumer = token.consumer
# The token string is a compact representation of the keys.
- token_string = '%s:%s:%s' % (consumer.key, token.key, token.secret)
self.assertSequenceEqual(
- [token_string],
+ [convert_tuple_to_string(get_creds_tuple(token))],
[elem.value.strip() for elem in
doc.cssselect('input#%s' % token.key)])
=== modified file 'src/provisioningserver/auth.py'
--- src/provisioningserver/auth.py 2012-08-10 13:11:19 +0000
+++ src/provisioningserver/auth.py 2012-08-13 05:53:32 +0000
@@ -18,6 +18,8 @@
'record_nodegroup_name',
]
+from apiclient.creds import convert_string_to_tuple
+
# API credentials as last sent by the server. The worker uses these
# credentials to access the MAAS API.
# Shared between threads.
@@ -57,7 +59,7 @@
if credentials_string is None:
return None
else:
- return tuple(credentials_string.split(':'))
+ return convert_string_to_tuple(credentials_string)
def record_nodegroup_name(nodegroup_name):
=== modified file 'src/provisioningserver/tests/test_auth.py'
--- src/provisioningserver/tests/test_auth.py 2012-08-10 12:32:22 +0000
+++ src/provisioningserver/tests/test_auth.py 2012-08-13 05:53:32 +0000
@@ -12,6 +12,7 @@
__metaclass__ = type
__all__ = []
+from apiclient.creds import convert_tuple_to_string
from maastesting.factory import factory
from maastesting.testcase import TestCase
from provisioningserver import auth
@@ -26,23 +27,18 @@
)
-def represent_credentials(credentials):
- """Represent a tuple of API credentials as a credentials string."""
- return ':'.join(credentials)
-
-
class TestAuth(TestCase):
def test_record_api_credentials_records_credentials_string(self):
self.patch(auth, 'recorded_api_credentials', None)
- creds_string = represent_credentials(make_credentials())
+ creds_string = convert_tuple_to_string(make_credentials())
auth.record_api_credentials(creds_string)
self.assertEqual(creds_string, auth.recorded_api_credentials)
def test_get_recorded_api_credentials_returns_credentials_as_tuple(self):
self.patch(auth, 'recorded_api_credentials', None)
creds = make_credentials()
- auth.record_api_credentials(represent_credentials(creds))
+ auth.record_api_credentials(convert_tuple_to_string(creds))
self.assertEqual(creds, auth.get_recorded_api_credentials())
def test_get_recorded_api_credentials_returns_None_without_creds(self):
=== modified file 'src/provisioningserver/tests/test_tasks.py'
--- src/provisioningserver/tests/test_tasks.py 2012-08-10 13:20:48 +0000
+++ src/provisioningserver/tests/test_tasks.py 2012-08-13 05:53:32 +0000
@@ -16,6 +16,7 @@
import random
from subprocess import CalledProcessError
+from apiclient.creds import convert_tuple_to_string
from maastesting.celery import CeleryFixture
from maastesting.factory import factory
from maastesting.fakemethod import (
@@ -113,7 +114,8 @@
factory.make_name('secret'),
)
self.patch(auth, 'recorded_api_credentials', None)
- refresh_secrets(api_credentials=':'.join(credentials))
+ refresh_secrets(
+ api_credentials=convert_tuple_to_string(credentials))
self.assertEqual(credentials, auth.get_recorded_api_credentials())
def test_updates_nodegroup_name(self):
