From 75ec12bde8e662e1b6e72c4e153feb25d6e4cf33 Mon Sep 17 00:00:00 2001 From: c0llab0rat0r <78339685+c0llab0rat0r@users.noreply.github.com> Date: Tue, 29 Jun 2021 23:24:15 -0500 Subject: [PATCH 1/3] Fix authentication bug with httpx usage bug; add status_code property to StatusError class; change PartialErrorResponse to inherit from Error instead of ErrorResponse due to lack of status code --- ipfshttpclient/exceptions.py | 34 ++++++--- ipfshttpclient/http_httpx.py | 19 +++-- ipfshttpclient/http_requests.py | 19 +++-- test/functional/test_auth.py | 128 ++++++++++++++++++++++++++++++++ 4 files changed, 178 insertions(+), 22 deletions(-) create mode 100644 test/functional/test_auth.py diff --git a/ipfshttpclient/exceptions.py b/ipfshttpclient/exceptions.py index 2b0c5f7e..33f215eb 100644 --- a/ipfshttpclient/exceptions.py +++ b/ipfshttpclient/exceptions.py @@ -14,10 +14,10 @@ │ ├── ProtocolError │ ├── StatusError │ ├── ErrorResponse - │ │ └── PartialErrorResponse │ ├── ConnectionError │ └── TimeoutError - └── MatcherSpecInvalidError + ├── MatcherSpecInvalidError + └── PartialErrorResponse """ @@ -137,8 +137,7 @@ class CommunicationError(Error): original: ty.Optional[Exception] - def __init__(self, original: ty.Optional[Exception], - _message: ty.Optional[str] = None) -> None: + def __init__(self, original: ty.Optional[Exception], _message: ty.Optional[str] = None) -> None: self.original = original if _message: @@ -158,7 +157,15 @@ class ProtocolError(CommunicationError): class StatusError(CommunicationError): """Raised when the daemon responds with an error to our request.""" - __slots__ = () + __slots__ = ('status_code',) + + def __init__(self, status_code: int, message: str, original: ty.Optional[Exception]) -> None: + super().__init__( + _message=message, + original=original + ) + + self.status_code = status_code class ErrorResponse(StatusError): @@ -166,17 +173,20 @@ class ErrorResponse(StatusError): requested operation could not be carried out.""" __slots__ = () - def __init__(self, message: str, original: ty.Optional[Exception]) -> None: - super().__init__(original, message) + def __init__(self, status_code: int, message: str, original: ty.Optional[Exception]) -> None: + super().__init__( + status_code=status_code, + message=message, + original=original + ) -class PartialErrorResponse(ErrorResponse): +class PartialErrorResponse(Error): """Raised when the daemon has responded with an error message after having already returned some data.""" - __slots__ = () - - def __init__(self, message: str, original: ty.Optional[Exception] = None) -> None: - super().__init__(message, original) + + def __init__(self, message: str) -> None: + super().__init__(message) class ConnectionError(CommunicationError): diff --git a/ipfshttpclient/http_httpx.py b/ipfshttpclient/http_httpx.py index 76e06f62..0ed461bc 100644 --- a/ipfshttpclient/http_httpx.py +++ b/ipfshttpclient/http_httpx.py @@ -129,8 +129,9 @@ def _make_session(self) -> httpx.Client: return httpx.Client(**self._session_kwargs, base_url = self._session_base, transport = connection_pool) - - def _do_raise_for_status(self, response: httpx.Response) -> None: + + @staticmethod + def _do_raise_for_status(response: httpx.Response) -> None: try: response.raise_for_status() except httpx.HTTPError as error: @@ -150,9 +151,17 @@ def _do_raise_for_status(self, response: httpx.Response) -> None: and isinstance(content[0], dict) \ and "Message" in content[0]: msg: str = content[0]["Message"] - raise exceptions.ErrorResponse(msg, error) from error + raise exceptions.ErrorResponse( + status_code=response.status_code, + message=msg, + original=error + ) from error else: - raise exceptions.StatusError(error) from error + raise exceptions.StatusError( + status_code=response.status_code, + message=str(error), + original=error + ) from error def _request( self, method: str, path: str, params: ty.Sequence[ty.Tuple[str, str]], *, @@ -179,7 +188,7 @@ def _request( url=path, **map_args_to_httpx( params=params, - auth=auth, + auth=auth or session.auth, # type: ignore[arg-type] headers=headers, timeout=timeout, ), diff --git a/ipfshttpclient/http_requests.py b/ipfshttpclient/http_requests.py index f7f10b1a..7318485e 100644 --- a/ipfshttpclient/http_requests.py +++ b/ipfshttpclient/http_requests.py @@ -117,8 +117,9 @@ def _make_session(self) -> requests.Session: # type: ignore[name-defined] except: # pragma: no cover session.close() raise - - def _do_raise_for_status(self, response: requests.Request) -> None: # type: ignore[name-defined] + + @staticmethod + def _do_raise_for_status(response: requests.Response) -> None: # type: ignore[name-defined] try: response.raise_for_status() except requests.exceptions.HTTPError as error: # type: ignore[attr-defined] @@ -137,10 +138,18 @@ def _do_raise_for_status(self, response: requests.Request) -> None: # type: ign if len(content) == 1 \ and isinstance(content[0], dict) \ and "Message" in content[0]: - msg = content[0]["Message"] - raise exceptions.ErrorResponse(msg, error) from error + msg: str = content[0]["Message"] + raise exceptions.ErrorResponse( + status_code=response.status_code, + message=msg, + original=error + ) from error else: - raise exceptions.StatusError(error) from error + raise exceptions.StatusError( + status_code=response.status_code, + message=str(error), + original=error + ) from error def _request( self, method: str, path: str, params: ty.Sequence[ty.Tuple[str, str]], *, diff --git a/test/functional/test_auth.py b/test/functional/test_auth.py new file mode 100644 index 00000000..9464e799 --- /dev/null +++ b/test/functional/test_auth.py @@ -0,0 +1,128 @@ + +import base64 +import ipfshttpclient +import json +import pytest + +from ipaddress import ip_address, IPv4Address, IPv6Address +from ipfshttpclient.exceptions import StatusError +from multiaddr import Multiaddr +from _pytest.fixtures import FixtureRequest +from pytest_localserver.http import ContentServer +from urllib.parse import urlparse +from werkzeug import Request, Response + + +BASIC_USERNAME = 'basic_username' +BASIC_PASSWORD = 'basic_password' + + +def _basic_auth_token(username: str, password: str) -> str: + return base64.b64encode(f'{username}:{password}'.encode('utf-8')).decode('utf-8') + + +def _url_to_multiaddr(url: str) -> Multiaddr: + parsed = urlparse(url) + + try: + ip = ip_address(parsed.hostname) + except ValueError: + ip = None + + if ip is None: + prefix = '/dns' + elif isinstance(ip, IPv4Address): + prefix = '/ip4' + elif isinstance(ip, IPv6Address): + prefix = '/ip6' + else: + raise TypeError(f"Don't know how to convert {ip} to a {Multiaddr.__name__}") + + return Multiaddr(f'{prefix}/{parsed.hostname}/tcp/{parsed.port}/{parsed.scheme}') + + +class AuthenticatingServer(ContentServer): + @staticmethod + def _is_authorized(expected_credentials: str, request: Request) -> bool: + authorizations = request.headers.get_all('Authorization') + + if authorizations and len(authorizations) == 1: + authorization = authorizations[0] + + return authorization == f'Basic {expected_credentials}' + else: + return False + + def __call__(self, environ, start_response) -> Response: + request = Request(environ) + self.requests.append(request) + + expected_credentials = _basic_auth_token(BASIC_USERNAME, BASIC_PASSWORD) + + if self._is_authorized(expected_credentials, request): + response = Response(status=self.code) + response.headers.clear() + response.headers.extend(self.headers) + + response.data = self.content + else: + response = Response(status=401) + response.headers.clear() + response.data = 'Unauthorized' + + return response(environ, start_response) + + +@pytest.fixture(scope='module') +def authenticating_server(request: FixtureRequest) -> ContentServer: + server = AuthenticatingServer() + server.start() + request.addfinalizer(server.stop) + + return server + + +def test_basic_auth_failure(authenticating_server: ContentServer) -> None: + headers = { + 'Content-Type': 'text/json' + } + + version = '0.0.1' + + response = { + 'Version': version + } + + authenticating_server.serve_content(json.dumps(response), headers=headers) + + address = _url_to_multiaddr(authenticating_server.url) + + with pytest.raises(StatusError) as failure: + ipfshttpclient.connect( + addr=address, + auth=('wrong', 'wrong') + ) + + assert failure.value.status_code == 401 + + +def test_basic_auth_success(authenticating_server: ContentServer) -> None: + headers = { + 'Content-Type': 'text/json' + } + + version = '0.0.1' + + response = { + 'Version': version + } + + authenticating_server.serve_content(json.dumps(response), headers=headers) + + address = _url_to_multiaddr(authenticating_server.url) + + with ipfshttpclient.connect( + addr=address, + auth=(BASIC_USERNAME, BASIC_PASSWORD) + ) as client: + assert client.version()['Version'] == version From 567a00ab6f8201c95e440c117c98a2f00751d828 Mon Sep 17 00:00:00 2001 From: c0llab0rat0r <78339685+c0llab0rat0r@users.noreply.github.com> Date: Tue, 29 Jun 2021 23:58:20 -0500 Subject: [PATCH 2/3] Deduplicate error response parsing --- ipfshttpclient/http_common.py | 31 +++++++++++++++++++++++ ipfshttpclient/http_httpx.py | 27 ++++---------------- ipfshttpclient/http_requests.py | 25 +++---------------- test/unit/test_http_common.py | 44 +++++++++++++++++++++++++++++++++ 4 files changed, 84 insertions(+), 43 deletions(-) create mode 100644 test/unit/test_http_common.py diff --git a/ipfshttpclient/http_common.py b/ipfshttpclient/http_common.py index 5d1257cd..3c9be3e6 100644 --- a/ipfshttpclient/http_common.py +++ b/ipfshttpclient/http_common.py @@ -684,3 +684,34 @@ def download( finally: for closable in closables: closable.close() + + @staticmethod + def _raise_for_response_status( + error: Exception, + status_code: int, + content: ty.List[object] + ) -> None: + + """ + If we have decoded an error response from the server, + use that as the exception message; otherwise, just pass + the exception on to the caller. + """ + + if len(content) == 1: + item = content[0] + + if isinstance(item, dict) and "Message" in item: + msg: str = item["Message"] + + raise exceptions.ErrorResponse( + status_code=status_code, + message=msg, + original=error + ) from error + + raise exceptions.StatusError( + status_code=status_code, + message=str(error), + original=error + ) from error diff --git a/ipfshttpclient/http_httpx.py b/ipfshttpclient/http_httpx.py index 0ed461bc..7d2c4aaf 100644 --- a/ipfshttpclient/http_httpx.py +++ b/ipfshttpclient/http_httpx.py @@ -130,8 +130,8 @@ def _make_session(self) -> httpx.Client: base_url = self._session_base, transport = connection_pool) - @staticmethod - def _do_raise_for_status(response: httpx.Response) -> None: + @classmethod + def _do_raise_for_status(cls, response: httpx.Response) -> None: try: response.raise_for_status() except httpx.HTTPError as error: @@ -143,26 +143,9 @@ def _do_raise_for_status(response: httpx.Response) -> None: content += list(decoder.parse_finalize()) except exceptions.DecodingError: pass - - # If we have decoded an error response from the server, - # use that as the exception message; otherwise, just pass - # the exception on to the caller. - if len(content) == 1 \ - and isinstance(content[0], dict) \ - and "Message" in content[0]: - msg: str = content[0]["Message"] - raise exceptions.ErrorResponse( - status_code=response.status_code, - message=msg, - original=error - ) from error - else: - raise exceptions.StatusError( - status_code=response.status_code, - message=str(error), - original=error - ) from error - + + cls._raise_for_response_status(error, response.status_code, content) + def _request( self, method: str, path: str, params: ty.Sequence[ty.Tuple[str, str]], *, auth: auth_t, diff --git a/ipfshttpclient/http_requests.py b/ipfshttpclient/http_requests.py index 7318485e..7bc7fee2 100644 --- a/ipfshttpclient/http_requests.py +++ b/ipfshttpclient/http_requests.py @@ -118,8 +118,8 @@ def _make_session(self) -> requests.Session: # type: ignore[name-defined] session.close() raise - @staticmethod - def _do_raise_for_status(response: requests.Response) -> None: # type: ignore[name-defined] + @classmethod + def _do_raise_for_status(cls, response: requests.Response) -> None: # type: ignore[name-defined] try: response.raise_for_status() except requests.exceptions.HTTPError as error: # type: ignore[attr-defined] @@ -132,25 +132,8 @@ def _do_raise_for_status(response: requests.Response) -> None: # type: ignore[n except exceptions.DecodingError: pass - # If we have decoded an error response from the server, - # use that as the exception message; otherwise, just pass - # the exception on to the caller. - if len(content) == 1 \ - and isinstance(content[0], dict) \ - and "Message" in content[0]: - msg: str = content[0]["Message"] - raise exceptions.ErrorResponse( - status_code=response.status_code, - message=msg, - original=error - ) from error - else: - raise exceptions.StatusError( - status_code=response.status_code, - message=str(error), - original=error - ) from error - + cls._raise_for_response_status(error, response.status_code, content) + def _request( self, method: str, path: str, params: ty.Sequence[ty.Tuple[str, str]], *, auth: auth_t, diff --git a/test/unit/test_http_common.py b/test/unit/test_http_common.py new file mode 100644 index 00000000..cd37e884 --- /dev/null +++ b/test/unit/test_http_common.py @@ -0,0 +1,44 @@ + +import pytest + +from ipfshttpclient.exceptions import ErrorResponse, StatusError +from ipfshttpclient.http_common import ClientSyncBase + + +def test_client_sync_base_raises_for_standardized_failure(): + original = Exception('foo') + + with pytest.raises(ErrorResponse) as failure: + ClientSyncBase._raise_for_response_status( + error=original, + status_code=405, + content=[ + { + 'Message': 'bar' + } + ] + ) + + assert failure.value.status_code == 405 + assert str(failure.value) == 'bar' + assert failure.value.original is original + + +@pytest.mark.parametrize('content', [ + [], + [{'wrong': 'ignored'}], + [{'Message': 'too'}, {'Message': 'many'}] +]) +def test_client_sync_base_raises_for_non_standard_failure(content): + original = Exception('qux') + + with pytest.raises(StatusError) as failure: + ClientSyncBase._raise_for_response_status( + error=original, + status_code=406, + content=content + ) + + assert failure.value.status_code == 406 + assert str(failure.value) == 'qux' + assert failure.value.original is original From b59d4cc58261fbfd9b6f89bd8c19865817c68b14 Mon Sep 17 00:00:00 2001 From: c0llab0rat0r <78339685+c0llab0rat0r@users.noreply.github.com> Date: Wed, 30 Jun 2021 00:26:30 -0500 Subject: [PATCH 3/3] Remove Python 3.5 checks from run_tests.py --- ipfshttpclient/http_requests.py | 9 ++++++--- test/run-tests.py | 14 ++++++++------ 2 files changed, 14 insertions(+), 9 deletions(-) diff --git a/ipfshttpclient/http_requests.py b/ipfshttpclient/http_requests.py index 7bc7fee2..9111c26c 100644 --- a/ipfshttpclient/http_requests.py +++ b/ipfshttpclient/http_requests.py @@ -1,6 +1,6 @@ """HTTP client for API requests based on good old requests library -This exists mainly for Python 3.5 compatibility. +This exists while httpx remains in beta. """ import math @@ -20,8 +20,11 @@ Closable, ) -PATCH_REQUESTS = (os.environ.get("PY_IPFS_HTTP_CLIENT_PATCH_REQUESTS", "yes").lower() - not in ("false", "no")) +PATCH_REQUESTS = ( + os.environ.get("PY_IPFS_HTTP_CLIENT_PATCH_REQUESTS", "yes").lower() + not in ("false", "no") +) + if PATCH_REQUESTS: from . import requests_wrapper as requests elif not ty.TYPE_CHECKING: # pragma: no cover (always enabled in production) diff --git a/test/run-tests.py b/test/run-tests.py index a079ce8e..46c8dc95 100755 --- a/test/run-tests.py +++ b/test/run-tests.py @@ -113,16 +113,18 @@ def pytest_pyfunc_call(self, pyfuncitem): with tempfile.NamedTemporaryFile("r+") as coveragerc: coverage_args = [] if os.name != "nt": - PREFER_HTTPX = (os.environ.get("PY_IPFS_HTTP_CLIENT_PREFER_HTTPX", "no").lower() - not in ("0", "f", "false", "n", "no")) + PREFER_HTTPX = ( + os.environ.get("PY_IPFS_HTTP_CLIENT_PREFER_HTTPX", "no").lower() + not in ("0", "f", "false", "n", "no") + ) # Assemble list of files to exclude from coverage analysis omitted_files = [ "ipfshttpclient/requests_wrapper.py", ] - if PREFER_HTTPX and sys.version_info >= (3, 6): + if PREFER_HTTPX: omitted_files.append("ipfshttpclient/http_requests.py") - else: #PY35: Fallback to old requests-based code instead of HTTPX + else: omitted_files.append("ipfshttpclient/http_httpx.py") # Assemble list of coverage data exclusion patterns (also escape the @@ -152,9 +154,9 @@ def pytest_pyfunc_call(self, pyfuncitem): "|".join(map(str, range(sys.version_info.minor + 1, 20))) )) - if PREFER_HTTPX and sys.version_info >= (3, 6): + if PREFER_HTTPX: exclusions.append(r"\# pragma: http-backend=requests") - else: #PY35: Fallback to old requests-based code instead of HTTPX + else: exclusions.append(r"\# pragma: http-backend=httpx") # Create temporary file with extended *coverage.py* configuration data