From cef47c68e50ef791f5e2a3db341d26bed814bdb5 Mon Sep 17 00:00:00 2001 From: Zhihao Yuan Date: Wed, 2 Oct 2013 10:33:41 -0400 Subject: [PATCH] feat(api): Client-ID as a real UUID We store the UUID in binary form in DBs, and perform checking on user inputs. Compared with the hex form we currently using, the binary form saves half space to store. In addition, by enforcing UUID on the server side, we can minimize the chance of client ID collision. Change-Id: Ic3048a0d2aa21bd201e2d2d9cd8a562662cf8f8e Closes-Bug: 1233420 --- marconi/queues/storage/base.py | 5 +- marconi/queues/storage/sqlite/driver.py | 14 +++++- marconi/queues/storage/sqlite/messages.py | 7 +-- marconi/queues/transport/wsgi/messages.py | 9 ++-- marconi/queues/transport/wsgi/utils.py | 19 ++++++++ marconi/tests/queues/storage/base.py | 46 +++++++++++-------- tests/unit/queues/transport/wsgi/test_auth.py | 4 +- .../unit/queues/transport/wsgi/test_claims.py | 11 +++-- .../queues/transport/wsgi/test_media_type.py | 8 +++- .../queues/transport/wsgi/test_messages.py | 19 +++++++- .../queues/transport/wsgi/test_validation.py | 3 +- .../transport/wsgi/test_default_limits.py | 3 +- 12 files changed, 108 insertions(+), 40 deletions(-) diff --git a/marconi/queues/storage/base.py b/marconi/queues/storage/base.py index b558e2cea..7ba2464d4 100644 --- a/marconi/queues/storage/base.py +++ b/marconi/queues/storage/base.py @@ -184,8 +184,7 @@ def list(self, queue, project=None, marker=None, messages to return. :param echo: (Default False) Boolean expressing whether or not this client should receive its own messages. - :param client_uuid: Client's unique identifier. This param - is required when echo=False. + :param client_uuid: A UUID object. Required when echo=False. :returns: An iterator giving a sequence of messages and the marker of the next page. @@ -245,7 +244,7 @@ def post(self, queue, messages, client_uuid, project=None): :param messages: Messages to post to queue, an iterable yielding 1 or more elements. An empty iterable results in undefined behavior. - :param client_uuid: Client's unique identifier. + :param client_uuid: A UUID object. :param project: Project id :returns: List of message ids diff --git a/marconi/queues/storage/sqlite/driver.py b/marconi/queues/storage/sqlite/driver.py index 111d1a7cc..5d0f17545 100644 --- a/marconi/queues/storage/sqlite/driver.py +++ b/marconi/queues/storage/sqlite/driver.py @@ -15,6 +15,7 @@ import contextlib import sqlite3 +import uuid import msgpack @@ -43,11 +44,22 @@ def pack(o): :param o: a Python str, unicode, int, long, float, bool, None or a dict or list of %o """ - return buffer(msgpack.dumps(o)) + return sqlite3.Binary(msgpack.dumps(o)) sqlite3.register_converter('DOCUMENT', lambda s: msgpack.loads(s, encoding='utf-8')) + @staticmethod + def uuid(o): + """Converts a UUID object to a custom SQlite `UUID`. + + :param o: a UUID object + """ + return sqlite3.Binary(o.bytes) + + sqlite3.register_converter('UUID', lambda s: + uuid.UUID(hex=s)) + def run(self, sql, *args): """Performs a SQL query. diff --git a/marconi/queues/storage/sqlite/messages.py b/marconi/queues/storage/sqlite/messages.py index 9069c1a39..caf908a3e 100644 --- a/marconi/queues/storage/sqlite/messages.py +++ b/marconi/queues/storage/sqlite/messages.py @@ -35,7 +35,7 @@ def __init__(self, driver): qid INTEGER, ttl INTEGER, content DOCUMENT, - client TEXT, + client UUID, created DATETIME, -- seconds since the Julian day PRIMARY KEY(id), FOREIGN KEY(qid) references Queues(id) on delete cascade @@ -157,7 +157,7 @@ def list(self, queue, project, marker=None, limit=None, if not echo: sql += ''' and M.client != ?''' - args += [client_uuid] + args += [self.driver.uuid(client_uuid)] if marker: sql += ''' @@ -214,7 +214,8 @@ def post(self, queue, messages, client_uuid, project): def it(): for m in messages: yield (my['newid'], qid, m['ttl'], - self.driver.pack(m['body']), client_uuid) + self.driver.pack(m['body']), + self.driver.uuid(client_uuid)) my['newid'] += 1 self.driver.run_multiple(''' diff --git a/marconi/queues/transport/wsgi/messages.py b/marconi/queues/transport/wsgi/messages.py index b5b7ebd2b..89f509fd9 100644 --- a/marconi/queues/transport/wsgi/messages.py +++ b/marconi/queues/transport/wsgi/messages.py @@ -74,8 +74,7 @@ def _get_by_id(self, base_path, project_id, queue_name, ids): return messages def _get(self, req, project_id, queue_name): - uuid = req.get_header('Client-ID', required=True) - + client_uuid = wsgi_utils.get_client_uuid(req) kwargs = {} # NOTE(kgriffs): This syntax ensures that @@ -90,7 +89,7 @@ def _get(self, req, project_id, queue_name): results = self.message_controller.list( queue_name, project=project_id, - client_uuid=uuid, + client_uuid=client_uuid, **kwargs) # Buffer messages @@ -136,7 +135,7 @@ def on_post(self, req, resp, project_id, queue_name): u'project: %(project)s') % {'queue': queue_name, 'project': project_id}) - uuid = req.get_header('Client-ID', required=True) + client_uuid = wsgi_utils.get_client_uuid(req) # Place JSON size restriction before parsing if req.content_length > CFG.content_max_length: @@ -164,7 +163,7 @@ def on_post(self, req, resp, project_id, queue_name): queue_name, messages=messages, project=project_id, - client_uuid=uuid) + client_uuid=client_uuid) except validate.ValidationFailed as ex: raise wsgi_exceptions.HTTPBadRequestAPI(six.text_type(ex)) diff --git a/marconi/queues/transport/wsgi/utils.py b/marconi/queues/transport/wsgi/utils.py index 67a7fa2d7..c4b122de9 100644 --- a/marconi/queues/transport/wsgi/utils.py +++ b/marconi/queues/transport/wsgi/utils.py @@ -14,6 +14,8 @@ # See the License for the specific language governing permissions and # limitations under the License. +import uuid + import marconi.openstack.common.log as logging from marconi.queues.transport import utils @@ -153,3 +155,20 @@ def get_checked_field(document, name, value_type): description = _(u'The value of the "{name}" field must be a {vtype}.') description = description.format(name=name, vtype=value_type.__name__) raise exceptions.HTTPBadRequestBody(description) + + +def get_client_uuid(req): + """Read a required Client-ID from a request. + + :param req: A falcon.Request object + :raises: HTTPBadRequest if the Client-ID header is missing or + does not represent a valid UUID + :returns: A UUID object + """ + + try: + return uuid.UUID(req.get_header('Client-ID', required=True)) + + except ValueError: + description = _(u'Malformed hexadecimal UUID.') + raise exceptions.HTTPBadRequestAPI(description) diff --git a/marconi/tests/queues/storage/base.py b/marconi/tests/queues/storage/base.py index 32b6c3d17..2303fb6dd 100644 --- a/marconi/tests/queues/storage/base.py +++ b/marconi/tests/queues/storage/base.py @@ -15,6 +15,7 @@ import datetime import time +import uuid import ddt from testtools import matchers @@ -121,9 +122,11 @@ def test_queue_lifecycle(self): metadata = self.controller.get_metadata('test', project=self.project) self.assertEqual(metadata['meta'], 'test_meta') + client_uuid = uuid.uuid4() + # Test queue statistic _insert_fixtures(self.message_controller, 'test', - project=self.project, client_uuid='my_uuid', + project=self.project, client_uuid=client_uuid, num=6) # NOTE(kgriffs): We can't get around doing this, because @@ -132,7 +135,7 @@ def test_queue_lifecycle(self): time.sleep(1) _insert_fixtures(self.message_controller, 'test', - project=self.project, client_uuid='my_uuid', + project=self.project, client_uuid=client_uuid, num=6) stats = self.controller.stats('test', project=self.project) @@ -231,7 +234,7 @@ def test_message_lifecycle(self): # Test Message Creation created = list(self.controller.post(queue_name, messages, project=self.project, - client_uuid='unused')) + client_uuid=uuid.uuid4())) self.assertEqual(len(created), 1) # Test Message Get @@ -245,8 +248,10 @@ def test_message_lifecycle(self): self.controller.get(queue_name, created[0], project=self.project) def test_get_multi(self): + client_uuid = uuid.uuid4() + _insert_fixtures(self.controller, self.queue_name, - project=self.project, client_uuid='my_uuid', num=15) + project=self.project, client_uuid=client_uuid, num=15) def load_messages(expected, *args, **kwargs): interaction = self.controller.list(*args, **kwargs) @@ -256,7 +261,7 @@ def load_messages(expected, *args, **kwargs): # Test all messages, echo False and uuid load_messages(0, self.queue_name, project=self.project, - client_uuid='my_uuid') + client_uuid=client_uuid) # Test all messages and limit load_messages(15, self.queue_name, project=self.project, limit=20, @@ -265,17 +270,17 @@ def load_messages(expected, *args, **kwargs): # Test all messages, echo True, and uuid interaction = load_messages(10, self.queue_name, echo=True, project=self.project, - client_uuid='my_uuid') + client_uuid=client_uuid) # Test all messages, echo True, uuid and marker load_messages(5, self.queue_name, echo=True, project=self.project, - marker=next(interaction), client_uuid='my_uuid') + marker=next(interaction), client_uuid=client_uuid) def test_multi_ids(self): messages_in = [{'ttl': 120, 'body': 0}, {'ttl': 240, 'body': 1}] ids = self.controller.post(self.queue_name, messages_in, project=self.project, - client_uuid='my_uuid') + client_uuid=uuid.uuid4()) messages_out = self.controller.bulk_get(self.queue_name, ids, project=self.project) @@ -292,13 +297,15 @@ def test_multi_ids(self): next(result) def test_claim_effects(self): + client_uuid = uuid.uuid4() + _insert_fixtures(self.controller, self.queue_name, - project=self.project, client_uuid='my_uuid', num=12) + project=self.project, client_uuid=client_uuid, num=12) def list_messages(include_claimed=None): kwargs = { 'project': self.project, - 'client_uuid': 'my_uuid', + 'client_uuid': client_uuid, 'echo': True, } @@ -360,14 +367,15 @@ def list_messages(include_claimed=None): @testing.is_slow(condition=lambda self: self.gc_interval != 0) def test_expired_messages(self): messages = [{'body': 3.14, 'ttl': 0}] + client_uuid = uuid.uuid4() [msgid] = self.controller.post(self.queue_name, messages, project=self.project, - client_uuid='my_uuid') + client_uuid=client_uuid) [msgid] = self.controller.post(self.queue_name, messages, project=self.project, - client_uuid='my_uuid') + client_uuid=client_uuid) time.sleep(self.gc_interval) @@ -400,7 +408,7 @@ def test_bad_claim_id(self): [msgid] = self.controller.post(self.queue_name, [{'body': {}, 'ttl': 10}], project=self.project, - client_uuid='my_uuid') + client_uuid=uuid.uuid4()) bad_claim_id = '; DROP TABLE queues' self.controller.delete(self.queue_name, @@ -412,6 +420,7 @@ def test_bad_marker(self): bad_marker = 'xyz' interaction = self.controller.list(self.queue_name, project=self.project, + client_uuid=uuid.uuid4(), marker=bad_marker) messages = list(next(interaction)) @@ -442,7 +451,8 @@ def tearDown(self): def test_claim_lifecycle(self): _insert_fixtures(self.message_controller, self.queue_name, - project=self.project, client_uuid='my_uuid', num=20) + project=self.project, client_uuid=uuid.uuid4(), + num=20) meta = {'ttl': 70, 'grace': 30} @@ -500,7 +510,7 @@ def test_claim_lifecycle(self): def test_extend_lifetime(self): _insert_fixtures(self.message_controller, self.queue_name, - project=self.project, client_uuid='my_uuid', + project=self.project, client_uuid=uuid.uuid4(), num=20, ttl=120) meta = {'ttl': 777, 'grace': 0} @@ -513,7 +523,7 @@ def test_extend_lifetime(self): def test_extend_lifetime_with_grace_1(self): _insert_fixtures(self.message_controller, self.queue_name, - project=self.project, client_uuid='my_uuid', + project=self.project, client_uuid=uuid.uuid4(), num=20, ttl=120) meta = {'ttl': 777, 'grace': 23} @@ -526,7 +536,7 @@ def test_extend_lifetime_with_grace_1(self): def test_extend_lifetime_with_grace_2(self): _insert_fixtures(self.message_controller, self.queue_name, - project=self.project, client_uuid='my_uuid', + project=self.project, client_uuid=uuid.uuid4(), num=20, ttl=120) # Although ttl is less than the message's TTL, the grace @@ -541,7 +551,7 @@ def test_extend_lifetime_with_grace_2(self): def test_do_not_extend_lifetime(self): _insert_fixtures(self.message_controller, self.queue_name, - project=self.project, client_uuid='my_uuid', + project=self.project, client_uuid=uuid.uuid4(), num=20, ttl=120) # Choose a ttl that is less than the message's current TTL diff --git a/tests/unit/queues/transport/wsgi/test_auth.py b/tests/unit/queues/transport/wsgi/test_auth.py index 7068c99ac..7a18a8093 100644 --- a/tests/unit/queues/transport/wsgi/test_auth.py +++ b/tests/unit/queues/transport/wsgi/test_auth.py @@ -14,6 +14,8 @@ # limitations under the License. """Test Auth.""" +import uuid + import falcon from falcon import testing from keystoneclient.middleware import auth_token @@ -27,7 +29,7 @@ class TestWSGIAuth(base.TestBase): def setUp(self): super(TestWSGIAuth, self).setUp() - self.headers = {'Client-ID': '30387f00'} + self.headers = {'Client-ID': str(uuid.uuid4())} def test_auth_install(self): self.assertTrue(isinstance(self.app, diff --git a/tests/unit/queues/transport/wsgi/test_claims.py b/tests/unit/queues/transport/wsgi/test_claims.py index cdb6ce11b..3fb7bf998 100644 --- a/tests/unit/queues/transport/wsgi/test_claims.py +++ b/tests/unit/queues/transport/wsgi/test_claims.py @@ -14,6 +14,7 @@ # limitations under the License. import json +import uuid import ddt import falcon @@ -46,7 +47,7 @@ def setUp(self): doc = json.dumps([{'body': 239, 'ttl': 300}] * 10) self.simulate_post(self.queue_path + '/messages', self.project_id, - body=doc, headers={'Client-ID': '30387f00'}) + body=doc, headers={'Client-ID': str(uuid.uuid4())}) self.assertEquals(self.srmock.status, falcon.HTTP_201) def tearDown(self): @@ -122,15 +123,19 @@ def test_lifecycle(self): query_string='limit=3') self.assertEquals(self.srmock.status, falcon.HTTP_204) + headers = { + 'Client-ID': str(uuid.uuid4()), + } + # Listing messages, by default, won't include claimed body = self.simulate_get(self.messages_path, self.project_id, - headers={'Client-ID': 'foo'}) + headers=headers) self.assertEquals(self.srmock.status, falcon.HTTP_204) # Include claimed messages this time body = self.simulate_get(self.messages_path, self.project_id, query_string='include_claimed=true', - headers={'Client-ID': 'foo'}) + headers=headers) listed = json.loads(body[0]) self.assertEquals(self.srmock.status, falcon.HTTP_200) self.assertEquals(len(listed['messages']), len(claimed)) diff --git a/tests/unit/queues/transport/wsgi/test_media_type.py b/tests/unit/queues/transport/wsgi/test_media_type.py index 212763ae2..9f36b81c3 100644 --- a/tests/unit/queues/transport/wsgi/test_media_type.py +++ b/tests/unit/queues/transport/wsgi/test_media_type.py @@ -13,6 +13,8 @@ # See the License for the specific language governing permissions and # limitations under the License. +import uuid + import ddt import falcon from falcon import testing @@ -36,8 +38,10 @@ class TestWSGIMediaType(base.TestBase): ('GET', '/v1/health'), ) def test_json_only_endpoints(self, (method, endpoint)): - headers = {'Client-ID': '30387f00', - 'Accept': 'application/xml'} + headers = { + 'Client-ID': str(uuid.uuid4()), + 'Accept': 'application/xml', + } env = testing.create_environ(endpoint, method=method, diff --git a/tests/unit/queues/transport/wsgi/test_messages.py b/tests/unit/queues/transport/wsgi/test_messages.py index 634705af7..7864202d7 100644 --- a/tests/unit/queues/transport/wsgi/test_messages.py +++ b/tests/unit/queues/transport/wsgi/test_messages.py @@ -14,6 +14,7 @@ # limitations under the License. import json +import uuid import ddt import falcon @@ -44,7 +45,7 @@ def setUp(self): self.simulate_put(self.queue_path, self.project_id, body=doc) self.headers = { - 'Client-ID': '30387f00', + 'Client-ID': str(uuid.uuid4()), } def tearDown(self): @@ -187,6 +188,20 @@ def test_post_to_missing_queue(self): self._post_messages('/v1/queues/nonexistent/messages') self.assertEquals(self.srmock.status, falcon.HTTP_404) + @ddt.data('', '0xdeadbeef', '550893e0-2b6e-11e3-835a-5cf9dd72369') + def test_bad_client_id(self, text_id): + self.simulate_post(self.queue_path + '/messages', + body='{"ttl": 60, "body": ""}', + headers={'Client-ID': text_id}) + + self.assertEquals(self.srmock.status, falcon.HTTP_400) + + self.simulate_get(self.queue_path + '/messages', + query_string='limit=3&echo=true', + headers={'Client-ID': text_id}) + + self.assertEquals(self.srmock.status, falcon.HTTP_400) + @ddt.data(None, '[', '[]', '{}', '.') def test_post_bad_message(self, document): self.simulate_post(self.queue_path + '/messages', @@ -426,7 +441,7 @@ def test_simple(self): path = '/v1/queues/fizbit/messages' doc = '[{"body": 239, "ttl": 100}]' headers = { - 'Client-ID': '30387f00', + 'Client-ID': str(uuid.uuid4()), } self.simulate_post(path, project_id, diff --git a/tests/unit/queues/transport/wsgi/test_validation.py b/tests/unit/queues/transport/wsgi/test_validation.py index 1aecb42e7..39461b2ae 100644 --- a/tests/unit/queues/transport/wsgi/test_validation.py +++ b/tests/unit/queues/transport/wsgi/test_validation.py @@ -14,6 +14,7 @@ # limitations under the License. import json +import uuid import falcon @@ -33,7 +34,7 @@ def setUp(self): self.simulate_put(self.queue_path, self.project_id) self.headers = { - 'Client-ID': '30387f00', + 'Client-ID': str(uuid.uuid4()), } def tearDown(self): diff --git a/tests/unit/transport/wsgi/test_default_limits.py b/tests/unit/transport/wsgi/test_default_limits.py index 36c39b019..3af012744 100644 --- a/tests/unit/transport/wsgi/test_default_limits.py +++ b/tests/unit/transport/wsgi/test_default_limits.py @@ -14,6 +14,7 @@ # limitations under the License. import json +import uuid import falcon @@ -59,7 +60,7 @@ def test_message_listing(self): self.__prepare_messages(10) result = self.simulate_get(self.messages_path, - headers={'Client-ID': 'audience'}) + headers={'Client-ID': str(uuid.uuid4())}) self.assertEquals(self.srmock.status, falcon.HTTP_200)