From 223d754f6162d87a305bcb2b041a5e73d5fae303 Mon Sep 17 00:00:00 2001 From: Goutham Pacha Ravi Date: Thu, 13 Sep 2018 16:45:45 -0600 Subject: [PATCH] Fix encoding of query parameters IETF RFC 3986 classifies "~" as a reserved character [1], however until python3.7 [2], python's url parsing used to encode this character. urllib has seen a lot of churn in various python releases, and hence we were using a six wrapper to shield ourselves, however, this backwards-incompatible change in encoding norms forces us to deal with the problem at our end. Cinder's API accepts "~" in both, its encoded or un-encoded forms. So, let's stop encoding it within cinderclient, regardless of the version of python running it. Also fix an inconsitency around the use of the generic helper method in utils added in I3a3ae90cc6011d1aa0cc39db4329d9bc08801904 (cinderclient/utils.py - build_query_param) to allow for False as a value in the query. [1] https://tools.ietf.org/html/rfc3986.html [2] https://docs.python.org/3/library/urllib.parse.html#url-quoting Change-Id: I89809694ac3e4081ce83fd4f788f9355d6772f59 Closes-Bug: #1784728 --- cinderclient/apiclient/base.py | 6 ++--- cinderclient/base.py | 7 ++---- cinderclient/tests/unit/test_utils.py | 23 +++++++++---------- cinderclient/tests/unit/v1/test_shell.py | 17 ++++++++------ cinderclient/tests/unit/v2/test_shell.py | 2 +- cinderclient/tests/unit/v3/test_shell.py | 29 ++++++++++++------------ cinderclient/utils.py | 18 ++++++++++++--- cinderclient/v1/volume_snapshots.py | 2 +- cinderclient/v1/volumes.py | 2 +- cinderclient/v2/limits.py | 5 ++-- cinderclient/v3/group_snapshots.py | 2 +- cinderclient/v3/groups.py | 10 ++------ 12 files changed, 64 insertions(+), 59 deletions(-) diff --git a/cinderclient/apiclient/base.py b/cinderclient/apiclient/base.py index e1f611cf7..050b369b4 100644 --- a/cinderclient/apiclient/base.py +++ b/cinderclient/apiclient/base.py @@ -28,9 +28,9 @@ import copy from requests import Response import six -from six.moves.urllib import parse from cinderclient.apiclient import exceptions +from cinderclient import utils from oslo_utils import encodeutils from oslo_utils import strutils @@ -330,7 +330,7 @@ class CrudManager(BaseManager): return self._list( '%(base_url)s%(query)s' % { 'base_url': self.build_url(base_url=base_url, **kwargs), - 'query': '?%s' % parse.urlencode(kwargs) if kwargs else '', + 'query': utils.build_query_param(kwargs), }, self.collection_key) @@ -369,7 +369,7 @@ class CrudManager(BaseManager): rl = self._list( '%(base_url)s%(query)s' % { 'base_url': self.build_url(base_url=base_url, **kwargs), - 'query': '?%s' % parse.urlencode(kwargs) if kwargs else '', + 'query': '?%s' % utils.build_query_param(kwargs), }, self.collection_key) num = len(rl) diff --git a/cinderclient/base.py b/cinderclient/base.py index 5f6fb2532..505a6c6cf 100644 --- a/cinderclient/base.py +++ b/cinderclient/base.py @@ -24,7 +24,6 @@ import hashlib import os import six -from six.moves.urllib import parse from cinderclient.apiclient import base as common_base from cinderclient import exceptions @@ -173,10 +172,8 @@ class Manager(common_base.HookableMixin): query_params = utils.unicode_key_value_to_string(query_params) # Transform the dict to a sequence of two-element tuples in fixed # order, then the encoded string will be consistent in Python 2&3. - query_string = "" - if query_params: - params = sorted(query_params.items(), key=lambda x: x[0]) - query_string = "?%s" % parse.urlencode(params) + + query_string = utils.build_query_param(query_params, sort=True) detail = "" if detailed: diff --git a/cinderclient/tests/unit/test_utils.py b/cinderclient/tests/unit/test_utils.py index 2ed4299f9..0cb12a699 100644 --- a/cinderclient/tests/unit/test_utils.py +++ b/cinderclient/tests/unit/test_utils.py @@ -162,6 +162,7 @@ class CaptureStdout(object): self.read = self.stringio.read +@ddt.ddt class BuildQueryParamTestCase(test_utils.TestCase): def test_build_param_without_sort_switch(self): @@ -187,19 +188,17 @@ class BuildQueryParamTestCase(test_utils.TestCase): expected = "?key1=val1&key2=val2&key3=val3" self.assertEqual(expected, result) - def test_build_param_with_none(self): - dict_param = { - 'key1': 'val1', - 'key2': None, - 'key3': False, - 'key4': '' - } - result_1 = utils.build_query_param(dict_param) - result_2 = utils.build_query_param(None) + @ddt.data({}, + None, + {'key1': 'val1', 'key2': None, 'key3': False, 'key4': ''}) + def test_build_param_with_nones(self, dict_param): + result = utils.build_query_param(dict_param) - expected = "?key1=val1" - self.assertEqual(expected, result_1) - self.assertFalse(result_2) + expected = ("key1=val1", "key3=False") if dict_param else () + for exp in expected: + self.assertIn(exp, result) + if not expected: + self.assertEqual("", result) @ddt.ddt diff --git a/cinderclient/tests/unit/v1/test_shell.py b/cinderclient/tests/unit/v1/test_shell.py index 3f4d7716e..698542cc5 100644 --- a/cinderclient/tests/unit/v1/test_shell.py +++ b/cinderclient/tests/unit/v1/test_shell.py @@ -101,7 +101,7 @@ class ShellTest(utils.TestCase): def test_list(self): self.run_command('list') # NOTE(jdg): we default to detail currently - self.assert_called('GET', '/volumes/detail') + self.assert_called('GET', '/volumes/detail?all_tenants=0') def test_list_filter_tenant_with_all_tenants(self): self.run_command('list --tenant=123 --all-tenants 1') @@ -184,11 +184,13 @@ class ShellTest(utils.TestCase): def test_list_filter_status(self): self.run_command('list --status=available') - self.assert_called('GET', '/volumes/detail?status=available') + self.assert_called('GET', + '/volumes/detail?all_tenants=0&status=available') def test_list_filter_display_name(self): self.run_command('list --display-name=1234') - self.assert_called('GET', '/volumes/detail?display_name=1234') + self.assert_called('GET', + '/volumes/detail?all_tenants=0&display_name=1234') def test_list_all_tenants(self): self.run_command('list --all-tenants=1') @@ -200,7 +202,7 @@ class ShellTest(utils.TestCase): def test_list_limit(self): self.run_command('list --limit=10') - self.assert_called('GET', '/volumes/detail?limit=10') + self.assert_called('GET', '/volumes/detail?all_tenants=0&limit=10') def test_show(self): self.run_command('show 1234') @@ -231,12 +233,13 @@ class ShellTest(utils.TestCase): def test_snapshot_list_filter_volume_id(self): self.run_command('snapshot-list --volume-id=1234') - self.assert_called('GET', '/snapshots/detail?volume_id=1234') + self.assert_called('GET', + '/snapshots/detail?all_tenants=0&volume_id=1234') def test_snapshot_list_filter_status_and_volume_id(self): self.run_command('snapshot-list --status=available --volume-id=1234') self.assert_called('GET', '/snapshots/detail?' - 'status=available&volume_id=1234') + 'all_tenants=0&status=available&volume_id=1234') def test_rename(self): # basic rename with positional arguments @@ -483,7 +486,7 @@ class ShellTest(utils.TestCase): def test_list_transfer(self): self.run_command('transfer-list') - self.assert_called('GET', '/os-volume-transfer/detail') + self.assert_called('GET', '/os-volume-transfer/detail?all_tenants=0') def test_list_transfer_all_tenants(self): self.run_command('transfer-list --all-tenants=1') diff --git a/cinderclient/tests/unit/v2/test_shell.py b/cinderclient/tests/unit/v2/test_shell.py index fd301a6d4..628412201 100644 --- a/cinderclient/tests/unit/v2/test_shell.py +++ b/cinderclient/tests/unit/v2/test_shell.py @@ -1176,7 +1176,7 @@ class ShellTest(utils.TestCase): def test_list_transfer(self): self.run_command('transfer-list') - self.assert_called('GET', '/os-volume-transfer/detail') + self.assert_called('GET', '/os-volume-transfer/detail?all_tenants=0') def test_list_transfer_all_tenants(self): self.run_command('transfer-list --all-tenants=1') diff --git a/cinderclient/tests/unit/v3/test_shell.py b/cinderclient/tests/unit/v3/test_shell.py index c9f776059..193c34ff6 100644 --- a/cinderclient/tests/unit/v3/test_shell.py +++ b/cinderclient/tests/unit/v3/test_shell.py @@ -131,37 +131,37 @@ class ShellTest(utils.TestCase): {'command': 'list --filters name~=456', 'expected': - '/volumes/detail?name%7E=456'}, + '/volumes/detail?name~=456'}, {'command': u'list --filters name~=Σ', 'expected': - '/volumes/detail?name%7E=%CE%A3'}, + '/volumes/detail?name~=%CE%A3'}, # testcases for list group {'command': 'group-list --filters name=456', 'expected': - '/groups/detail?name=456'}, + '/groups/detail?all_tenants=0&name=456'}, {'command': 'group-list --filters status=available', 'expected': - '/groups/detail?status=available'}, + '/groups/detail?all_tenants=0&status=available'}, {'command': 'group-list --filters name~=456', 'expected': - '/groups/detail?name%7E=456'}, + '/groups/detail?all_tenants=0&name~=456'}, # testcases for list group-snapshot {'command': 'group-snapshot-list --status=error --filters status=available', 'expected': - '/group_snapshots/detail?status=available'}, + '/group_snapshots/detail?all_tenants=0&status=available'}, {'command': 'group-snapshot-list --filters availability_zone=123', 'expected': - '/group_snapshots/detail?availability_zone=123'}, + '/group_snapshots/detail?all_tenants=0&availability_zone=123'}, {'command': 'group-snapshot-list --filters status~=available', 'expected': - '/group_snapshots/detail?status%7E=available'}, + '/group_snapshots/detail?all_tenants=0&status~=available'}, # testcases for list message {'command': 'message-list --event_id=123 --filters event_id=456', @@ -174,7 +174,7 @@ class ShellTest(utils.TestCase): {'command': 'message-list --filters request_id~=123', 'expected': - '/messages?request_id%7E=123'}, + '/messages?request_id~=123'}, # testcases for list attachment {'command': 'attachment-list --volume-id=123 --filters volume_id=456', @@ -187,7 +187,7 @@ class ShellTest(utils.TestCase): {'command': 'attachment-list --filters volume_id~=456', 'expected': - '/attachments?volume_id%7E=456'}, + '/attachments?volume_id~=456'}, # testcases for list backup {'command': 'backup-list --volume-id=123 --filters volume_id=456', @@ -200,7 +200,7 @@ class ShellTest(utils.TestCase): {'command': 'backup-list --filters volume_id~=456', 'expected': - '/backups/detail?volume_id%7E=456'}, + '/backups/detail?volume_id~=456'}, # testcases for list snapshot {'command': 'snapshot-list --volume-id=123 --filters volume_id=456', @@ -213,7 +213,7 @@ class ShellTest(utils.TestCase): {'command': 'snapshot-list --filters volume_id~=456', 'expected': - '/snapshots/detail?volume_id%7E=456'}, + '/snapshots/detail?volume_id~=456'}, # testcases for get pools {'command': 'get-pools --filters name=456 --detail', @@ -632,7 +632,7 @@ class ShellTest(utils.TestCase): def test_group_list(self): self.run_command('--os-volume-api-version 3.13 group-list') - self.assert_called_anytime('GET', '/groups/detail') + self.assert_called_anytime('GET', '/groups/detail?all_tenants=0') def test_group_list__with_all_tenant(self): self.run_command( @@ -691,7 +691,8 @@ class ShellTest(utils.TestCase): def test_group_snapshot_list(self): self.run_command('--os-volume-api-version 3.14 group-snapshot-list') - self.assert_called_anytime('GET', '/group_snapshots/detail') + self.assert_called_anytime('GET', + '/group_snapshots/detail?all_tenants=0') def test_group_snapshot_show(self): self.run_command('--os-volume-api-version 3.14 ' diff --git a/cinderclient/utils.py b/cinderclient/utils.py index f9af58d30..28c458dc0 100644 --- a/cinderclient/utils.py +++ b/cinderclient/utils.py @@ -206,15 +206,27 @@ def unicode_key_value_to_string(src): def build_query_param(params, sort=False): """parse list to url query parameters""" - if params is None: - params = {} + if not params: + return "" + if not sort: param_list = list(params.items()) else: param_list = list(sorted(params.items())) query_string = parse.urlencode( - [(k, v) for (k, v) in param_list if v]) + [(k, v) for (k, v) in param_list if v not in (None, '')]) + + # urllib's parse library used to adhere to RFC 2396 until + # python 3.7. The library moved from RFC 2396 to RFC 3986 + # for quoting URL strings in python 3.7 and '~' is now + # included in the set of reserved characters. [1] + # + # Below ensures "~" is never encoded. See LP 1784728 [2] for more details. + # [1] https://docs.python.org/3/library/urllib.parse.html#url-quoting + # [2] https://bugs.launchpad.net/python-cinderclient/+bug/1784728 + query_string = query_string.replace("%7E=", "~=") + if query_string: query_string = "?%s" % (query_string,) diff --git a/cinderclient/v1/volume_snapshots.py b/cinderclient/v1/volume_snapshots.py index b7840bdcf..922071a71 100644 --- a/cinderclient/v1/volume_snapshots.py +++ b/cinderclient/v1/volume_snapshots.py @@ -107,7 +107,7 @@ class SnapshotManager(base.ManagerWithFind): :rtype: list of :class:`Snapshot` """ - query_string = utils.build_query_param(search_opts, True) + query_string = utils.build_query_param(search_opts, sort=True) detail = "" if detailed: diff --git a/cinderclient/v1/volumes.py b/cinderclient/v1/volumes.py index 699c4ea04..8e25f40b1 100644 --- a/cinderclient/v1/volumes.py +++ b/cinderclient/v1/volumes.py @@ -203,7 +203,7 @@ class VolumeManager(base.ManagerWithFind): if limit: search_opts['limit'] = limit - query_string = utils.build_query_param(search_opts, True) + query_string = utils.build_query_param(search_opts, sort=True) detail = "" if detailed: diff --git a/cinderclient/v2/limits.py b/cinderclient/v2/limits.py index 80c5bf919..dd1666da0 100644 --- a/cinderclient/v2/limits.py +++ b/cinderclient/v2/limits.py @@ -14,9 +14,8 @@ # limitations under the License. """Limits interface (v2 extension)""" -from six.moves.urllib import parse - from cinderclient import base +from cinderclient import utils class Limits(base.Resource): @@ -95,6 +94,6 @@ class LimitsManager(base.Manager): if tenant_id: opts['tenant_id'] = tenant_id - query_string = "?%s" % parse.urlencode(opts) if opts else "" + query_string = utils.build_query_param(opts) return self._get("/limits%s" % query_string, "limits") diff --git a/cinderclient/v3/group_snapshots.py b/cinderclient/v3/group_snapshots.py index ed7fe7924..9225995ce 100644 --- a/cinderclient/v3/group_snapshots.py +++ b/cinderclient/v3/group_snapshots.py @@ -96,7 +96,7 @@ class GroupSnapshotManager(base.ManagerWithFind): :param search_opts: search options :rtype: list of :class:`GroupSnapshot` """ - query_string = utils.build_query_param(search_opts) + query_string = utils.build_query_param(search_opts, sort=True) detail = "" if detailed: diff --git a/cinderclient/v3/groups.py b/cinderclient/v3/groups.py index c042e2807..e42f7509f 100644 --- a/cinderclient/v3/groups.py +++ b/cinderclient/v3/groups.py @@ -14,8 +14,6 @@ # under the License. """Group interface (v3 extension).""" -from six.moves.urllib import parse - from cinderclient import api_versions from cinderclient.apiclient import base as common_base from cinderclient import base @@ -140,11 +138,7 @@ class GroupManager(base.ManagerWithFind): :rtype: :class:`Group` """ query_params = utils.unicode_key_value_to_string(kwargs) - - query_string = "" - if query_params: - params = sorted(query_params.items(), key=lambda x: x[0]) - query_string = "?%s" % parse.urlencode(params) + query_string = utils.build_query_param(query_params, sort=True) return self._get("/groups/%s" % group_id + query_string, "group") @@ -159,7 +153,7 @@ class GroupManager(base.ManagerWithFind): if not search_opts: search_opts = {} search_opts['list_volume'] = True - query_string = utils.build_query_param(search_opts) + query_string = utils.build_query_param(search_opts, sort=True) detail = "" if detailed: