From 317df7e527c174695e184f8250f5f743ac91f0f2 Mon Sep 17 00:00:00 2001 From: Tim Burke Date: Thu, 14 May 2015 17:09:43 -0700 Subject: [PATCH] Remove simplejson dependency In addition to removing an unnecessary dependency, this closes another hole that was allowing raw bytes to appear in user-facing messages. Change-Id: Ia0b76426a38e5a5c368c4c7e7ba2aef286758aca --- requirements.txt | 1 - swiftclient/client.py | 16 ++++++------ swiftclient/service.py | 13 +++++----- swiftclient/utils.py | 13 +++++++++- tests/unit/test_service.py | 4 +-- tests/unit/test_shell.py | 25 +++++++++--------- tests/unit/test_swiftclient.py | 46 ++++++---------------------------- 7 files changed, 47 insertions(+), 71 deletions(-) diff --git a/requirements.txt b/requirements.txt index e7c0d41a..88b3df2c 100644 --- a/requirements.txt +++ b/requirements.txt @@ -1,4 +1,3 @@ futures>=2.1.3 requests>=1.1 -simplejson>=2.0.9 six>=1.5.2 diff --git a/swiftclient/client.py b/swiftclient/client.py index 7f4c35b2..45d479a1 100644 --- a/swiftclient/client.py +++ b/swiftclient/client.py @@ -21,10 +21,6 @@ import socket import requests import logging import warnings -try: - from simplejson import loads as json_loads -except ImportError: - from json import loads as json_loads from distutils.version import StrictVersion from requests.exceptions import RequestException, SSLError @@ -35,7 +31,8 @@ import six from swiftclient import version as swiftclient_version from swiftclient.exceptions import ClientException -from swiftclient.utils import LengthWrapper, ReadableToIterable +from swiftclient.utils import ( + LengthWrapper, ReadableToIterable, parse_api_response) AUTH_VERSIONS_V1 = ('1.0', '1', 1) AUTH_VERSIONS_V2 = ('2.0', '2', 2) @@ -520,7 +517,7 @@ def get_account(url, token, marker=None, limit=None, prefix=None, http_response_content=body) if resp.status == 204: return resp_headers, [] - return resp_headers, json_loads(body) + return resp_headers, parse_api_response(resp_headers, body) def head_account(url, token, http_conn=None): @@ -670,7 +667,7 @@ def get_container(url, token, container, marker=None, limit=None, resp_headers[header.lower()] = value if resp.status == 204: return resp_headers, [] - return resp_headers, json_loads(body) + return resp_headers, parse_api_response(resp_headers, body) def head_container(url, token, container, http_conn=None, headers=None): @@ -1154,7 +1151,10 @@ def get_capabilities(http_conn): http_host=conn.host, http_path=parsed.path, http_status=resp.status, http_reason=resp.reason, http_response_content=body) - return json_loads(body) + resp_headers = {} + for header, value in resp.getheaders(): + resp_headers[header.lower()] = value + return parse_api_response(resp_headers, body) class Connection(object): diff --git a/swiftclient/service.py b/swiftclient/service.py index c533297e..d607d65c 100644 --- a/swiftclient/service.py +++ b/swiftclient/service.py @@ -29,10 +29,7 @@ from six.moves.queue import Empty as QueueEmpty from six.moves.urllib.parse import quote, unquote from six import Iterator, string_types -try: - import simplejson as json -except ImportError: - import json +import json from swiftclient import Connection @@ -40,7 +37,8 @@ from swiftclient.command_helpers import ( stat_account, stat_container, stat_object ) from swiftclient.utils import ( - config_true_value, ReadableToIterable, LengthWrapper, EMPTY_ETAG + config_true_value, ReadableToIterable, LengthWrapper, EMPTY_ETAG, + parse_api_response ) from swiftclient.exceptions import ClientException from swiftclient.multithreading import MultiThreadingManager @@ -1515,9 +1513,10 @@ class SwiftService(object): else: raise part["error"] elif config_true_value(headers.get('x-static-large-object')): - _, manifest_data = conn.get_object( + manifest_headers, manifest_data = conn.get_object( container, obj, query_string='multipart-manifest=get') - for chunk in json.loads(manifest_data): + manifest_data = parse_api_response(manifest_headers, manifest_data) + for chunk in manifest_data: if chunk.get('sub_slo'): scont, sobj = chunk['name'].lstrip('/').split('/', 1) chunks.extend(self._get_chunk_data( diff --git a/swiftclient/utils.py b/swiftclient/utils.py index 2f4ec3f8..8edfcfa6 100644 --- a/swiftclient/utils.py +++ b/swiftclient/utils.py @@ -15,6 +15,7 @@ """Miscellaneous utility functions for use with Swift.""" import hashlib import hmac +import json import logging import time @@ -28,7 +29,7 @@ def config_true_value(value): """ Returns True if the value is either True or a string in TRUE_VALUES. Returns False otherwise. - This function come from swift.common.utils.config_true_value() + This function comes from swift.common.utils.config_true_value() """ return value is True or \ (isinstance(value, six.string_types) and value.lower() in TRUE_VALUES) @@ -108,6 +109,16 @@ def generate_temp_url(path, seconds, key, method): exp=expiration)) +def parse_api_response(headers, body): + charset = 'utf-8' + # Swift *should* be speaking UTF-8, but check content-type just in case + content_type = headers.get('content-type', '') + if '; charset=' in content_type: + charset = content_type.split('; charset=', 1)[1].split(';', 1)[0] + + return json.loads(body.decode(charset)) + + class NoopMD5(object): def __init__(self, *a, **kw): pass diff --git a/tests/unit/test_service.py b/tests/unit/test_service.py index 0e8aff10..1e0d96d1 100644 --- a/tests/unit/test_service.py +++ b/tests/unit/test_service.py @@ -910,8 +910,8 @@ class TestServiceUpload(testtools.TestCase): 'etag': md5(submanifest_etag.encode('ascii') + seg_etag.encode('ascii')).hexdigest()} mock_conn.get_object.side_effect = [ - (None, manifest), - (None, submanifest)] + ({}, manifest.encode('ascii')), + ({}, submanifest.encode('ascii'))] type(mock_conn).attempts = mock.PropertyMock(return_value=2) s = SwiftService() diff --git a/tests/unit/test_shell.py b/tests/unit/test_shell.py index b89df96f..98cef855 100644 --- a/tests/unit/test_shell.py +++ b/tests/unit/test_shell.py @@ -15,7 +15,6 @@ from genericpath import getmtime import hashlib -import json import mock import os import tempfile @@ -31,8 +30,9 @@ import swiftclient.utils from os.path import basename, dirname from tests.unit.test_swiftclient import MockHttpTest -from tests.unit.utils import (CaptureOutput, fake_get_auth_keystone, - _make_fake_import_keystone_client, FakeKeystone) +from tests.unit.utils import ( + CaptureOutput, fake_get_auth_keystone, _make_fake_import_keystone_client, + FakeKeystone, StubResponse) from swiftclient.utils import EMPTY_ETAG @@ -474,9 +474,11 @@ class TestShell(unittest.TestCase): {'x-static-large-object': 'false', # For the 2nd delete call 'content-length': '2'} ] - connection.return_value.get_object.return_value = ({}, json.dumps( - [{'name': 'container1/old_seg1'}, {'name': 'container2/old_seg2'}] - )) + connection.return_value.get_object.return_value = ( + {}, + b'[{"name": "container1/old_seg1"},' + b' {"name": "container2/old_seg2"}]' + ) connection.return_value.put_object.return_value = EMPTY_ETAG swiftclient.shell.main(argv) connection.return_value.put_object.assert_called_with( @@ -1763,13 +1765,10 @@ class TestCrossAccountObjectAccess(TestBase, MockHttpTest): def test_list_with_read_access(self): req_handler = self._fake_cross_account_auth(True, False) - resp_body = '{}' - m = hashlib.md5() - m.update(resp_body.encode()) - etag = m.hexdigest() - fake_conn = self.fake_http_connection(403, on_request=req_handler, - etags=[etag], - body=resp_body) + resp_body = b'{}' + resp = StubResponse(403, resp_body, { + 'etag': hashlib.md5(resp_body).hexdigest()}) + fake_conn = self.fake_http_connection(resp, on_request=req_handler) args, env = self._make_cmd('download', cmd_args=[self.cont]) with mock.patch('swiftclient.client._import_keystone_client', diff --git a/tests/unit/test_swiftclient.py b/tests/unit/test_swiftclient.py index 801abc64..413da815 100644 --- a/tests/unit/test_swiftclient.py +++ b/tests/unit/test_swiftclient.py @@ -14,7 +14,6 @@ # limitations under the License. import logging -import json try: from unittest import mock @@ -28,7 +27,6 @@ import warnings import tempfile from hashlib import md5 from six.moves.urllib.parse import urlparse -from six.moves import reload_module from .utils import (MockHttpTest, fake_get_auth_keystone, StubResponse, FakeKeystone, _make_fake_import_keystone_client) @@ -65,36 +63,6 @@ class TestClientException(testtools.TestCase): self.assertTrue(value in str(exc)) -class TestJsonImport(testtools.TestCase): - - def tearDown(self): - reload_module(json) - - try: - import simplejson - except ImportError: - pass - else: - reload_module(simplejson) - super(TestJsonImport, self).tearDown() - - def test_any(self): - self.assertTrue(hasattr(c, 'json_loads')) - - def test_no_simplejson_falls_back_to_stdlib_when_reloaded(self): - # break simplejson - try: - import simplejson - except ImportError: - # not installed, so we don't have to break it for these tests - pass - else: - delattr(simplejson, 'loads') # break simple json - reload_module(c) # reload to repopulate json_loads - - self.assertEqual(c.json_loads, json.loads) - - class MockHttpResponse(object): def __init__(self, status=0, headers=None, verify=False): self.status = status @@ -936,7 +904,7 @@ class TestDeleteObject(MockHttpTest): class TestGetCapabilities(MockHttpTest): def test_ok(self): - conn = self.fake_http_connection(200, body='{}') + conn = self.fake_http_connection(200, body=b'{}') http_conn = conn('http://www.test.com/info') info = c.get_capabilities(http_conn) self.assertRequests([ @@ -957,7 +925,7 @@ class TestGetCapabilities(MockHttpTest): } auth_v1_response = StubResponse(headers=auth_headers) stub_info = {'swift': {'fake': True}} - info_response = StubResponse(body=json.dumps(stub_info)) + info_response = StubResponse(body=b'{"swift":{"fake":true}}') fake_conn = self.fake_http_connection(auth_v1_response, info_response) conn = c.Connection('http://auth.example.com/auth/v1.0', @@ -975,7 +943,7 @@ class TestGetCapabilities(MockHttpTest): fake_keystone = fake_get_auth_keystone( storage_url='http://storage.example.com/v1/AUTH_test') stub_info = {'swift': {'fake': True}} - info_response = StubResponse(body=json.dumps(stub_info)) + info_response = StubResponse(body=b'{"swift":{"fake":true}}') fake_conn = self.fake_http_connection(info_response) os_options = {'project_id': 'test'} @@ -993,7 +961,7 @@ class TestGetCapabilities(MockHttpTest): def test_conn_get_capabilities_with_url_param(self): stub_info = {'swift': {'fake': True}} - info_response = StubResponse(body=json.dumps(stub_info)) + info_response = StubResponse(body=b'{"swift":{"fake":true}}') fake_conn = self.fake_http_connection(info_response) conn = c.Connection('http://auth.example.com/auth/v1.0', @@ -1009,7 +977,7 @@ class TestGetCapabilities(MockHttpTest): def test_conn_get_capabilities_with_preauthurl_param(self): stub_info = {'swift': {'fake': True}} - info_response = StubResponse(body=json.dumps(stub_info)) + info_response = StubResponse(body=b'{"swift":{"fake":true}}') fake_conn = self.fake_http_connection(info_response) storage_url = 'http://storage.example.com/v1/AUTH_test' @@ -1025,7 +993,7 @@ class TestGetCapabilities(MockHttpTest): def test_conn_get_capabilities_with_os_options(self): stub_info = {'swift': {'fake': True}} - info_response = StubResponse(body=json.dumps(stub_info)) + info_response = StubResponse(body=b'{"swift":{"fake":true}}') fake_conn = self.fake_http_connection(info_response) storage_url = 'http://storage.example.com/v1/AUTH_test' @@ -1165,7 +1133,7 @@ class TestConnection(MockHttpTest): for method, args in method_signatures: c.http_connection = self.fake_http_connection( - 200, body='[]', storage_url=static_url) + 200, body=b'[]', storage_url=static_url) method(*args) self.assertEqual(len(self.request_log), 1) for request in self.iter_request_log():