From dbde076d69b731e565bb6ccbe48aeafe48b759c9 Mon Sep 17 00:00:00 2001 From: Philipp Reitter Date: Fri, 25 Nov 2022 15:12:35 +0100 Subject: [PATCH 1/4] reuse the digest auth challenge to avoid sending twice as many requests --- httpx/_auth.py | 17 +++++++++++----- tests/client/test_auth.py | 41 +++++++++++++++++++++++++++++++++++++++ tests/test_auth.py | 36 ++++++++++++++++++++++++++++++++++ 3 files changed, 89 insertions(+), 5 deletions(-) diff --git a/httpx/_auth.py b/httpx/_auth.py index 2b00b49d17..254fb443ea 100644 --- a/httpx/_auth.py +++ b/httpx/_auth.py @@ -155,8 +155,13 @@ def __init__( ) -> None: self._username = to_bytes(username) self._password = to_bytes(password) + self._last_challenge: typing.Optional[_DigestAuthChallenge] = None + self._nonce_count = 1 def auth_flow(self, request: Request) -> typing.Generator[Request, Response, None]: + if self._last_challenge: + request.headers["Authorization"] = self._build_auth_header(request, self._last_challenge) + response = yield request if response.status_code != 401 or "www-authenticate" not in response.headers: @@ -172,8 +177,10 @@ def auth_flow(self, request: Request) -> typing.Generator[Request, Response, Non # header, then we don't need to build an authenticated request. return - challenge = self._parse_challenge(request, response, auth_header) - request.headers["Authorization"] = self._build_auth_header(request, challenge) + self._last_challenge = self._parse_challenge(request, response, auth_header) + self._nonce_count = 1 + + request.headers["Authorization"] = self._build_auth_header(request, self._last_challenge) yield request def _parse_challenge( @@ -222,9 +229,9 @@ def digest(data: bytes) -> bytes: # TODO: implement auth-int HA2 = digest(A2) - nonce_count = 1 # TODO: implement nonce counting - nc_value = b"%08x" % nonce_count - cnonce = self._get_client_nonce(nonce_count, challenge.nonce) + nc_value = b"%08x" % self._nonce_count + cnonce = self._get_client_nonce(self._nonce_count, challenge.nonce) + self._nonce_count += 1 HA1 = digest(A1) if challenge.algorithm.lower().endswith("-sess"): diff --git a/tests/client/test_auth.py b/tests/client/test_auth.py index 8caaeb5d49..cdee5b75ac 100644 --- a/tests/client/test_auth.py +++ b/tests/client/test_auth.py @@ -527,6 +527,47 @@ async def test_digest_auth_incorrect_credentials() -> None: assert len(response.history) == 1 +@pytest.mark.asyncio +async def test_digest_auth_reuses_challenge() -> None: + url = "https://example.org/" + auth = DigestAuth(username="tomchristie", password="password123") + app = DigestApp() + + async with httpx.AsyncClient(transport=httpx.MockTransport(app)) as client: + response_1 = await client.get(url, auth=auth) + response_2 = await client.get(url, auth=auth) + + assert response_1.status_code == 200 + assert response_2.status_code == 200 + + assert len(response_1.history) == 1 + assert len(response_2.history) == 0 + + +@pytest.mark.asyncio +async def test_digest_auth_resets_nonce_count_after_401() -> None: + url = "https://example.org/" + auth = DigestAuth(username="tomchristie", password="password123") + app = DigestApp() + + async with httpx.AsyncClient(transport=httpx.MockTransport(app)) as client: + response_1 = await client.get(url, auth=auth) + assert response_1.status_code == 200 + assert len(response_1.history) == 1 + first_nonce = response_1.json()["auth"].split("nonce=")[1].split(",")[0] + first_nc = response_1.json()["auth"].split("nc=")[1].split(",")[0] + + app.send_response_after_attempt = 2 + response_2 = await client.get(url, auth=auth) + assert response_1.status_code == 200 + assert len(response_2.history) == 1 + second_nonce = response_2.json()["auth"].split("nonce=")[1].split(",")[0] + second_nc = response_2.json()["auth"].split("nc=")[1].split(",")[0] + + assert first_nonce != second_nonce + assert first_nc == second_nc + + @pytest.mark.parametrize( "auth_header", [ diff --git a/tests/test_auth.py b/tests/test_auth.py index 20c666a88c..025e82ae60 100644 --- a/tests/test_auth.py +++ b/tests/test_auth.py @@ -61,3 +61,39 @@ def test_digest_auth_with_401(): response = httpx.Response(content=b"Hello, world!", status_code=200) with pytest.raises(StopIteration): flow.send(response) + + +def test_digest_auth_with_401_nonce_counting(): + auth = httpx.DigestAuth(username="user", password="pass") + request = httpx.Request("GET", "https://www.example.com") + + # The initial request should not include an auth header. + flow = auth.sync_auth_flow(request) + request = next(flow) + assert "Authorization" not in request.headers + + # If a 401 response is returned, then a digest auth request is made. + headers = { + "WWW-Authenticate": 'Digest realm="...", qop="auth", nonce="...", opaque="..."' + } + response = httpx.Response( + content=b"Auth required", status_code=401, headers=headers + ) + first_request = flow.send(response) + assert first_request.headers["Authorization"].startswith("Digest") + + # Each subsequent request contains the digest header by default... + request = httpx.Request("GET", "https://www.example.com") + flow = auth.sync_auth_flow(request) + second_request = next(flow) + assert second_request.headers["Authorization"].startswith("Digest") + + # ... and the client nonce count (nc) is increased + first_nonce = first_request.headers["Authorization"].split(",")[-2].split("=")[1] + second_nonce = second_request.headers["Authorization"].split(",")[-2].split("=")[1] + assert int(first_nonce, 16) + 1 == int(second_nonce, 16) + + # No other requests are made. + response = httpx.Response(content=b"Hello, world!", status_code=200) + with pytest.raises(StopIteration): + flow.send(response) From addfdd1c03caba095d5a1a0acfc6a81808355c17 Mon Sep 17 00:00:00 2001 From: Philipp Reitter Date: Fri, 25 Nov 2022 15:28:37 +0100 Subject: [PATCH 2/4] fix for digest testcase --- tests/client/test_auth.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tests/client/test_auth.py b/tests/client/test_auth.py index cdee5b75ac..648677b838 100644 --- a/tests/client/test_auth.py +++ b/tests/client/test_auth.py @@ -559,7 +559,7 @@ async def test_digest_auth_resets_nonce_count_after_401() -> None: app.send_response_after_attempt = 2 response_2 = await client.get(url, auth=auth) - assert response_1.status_code == 200 + assert response_2.status_code == 200 assert len(response_2.history) == 1 second_nonce = response_2.json()["auth"].split("nonce=")[1].split(",")[0] second_nc = response_2.json()["auth"].split("nc=")[1].split(",")[0] From c845929f54b3861273432273501cfdd0815221d0 Mon Sep 17 00:00:00 2001 From: Philipp Reitter Date: Fri, 25 Nov 2022 15:32:04 +0100 Subject: [PATCH 3/4] ran testing/linting scripts --- httpx/_auth.py | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-) diff --git a/httpx/_auth.py b/httpx/_auth.py index 254fb443ea..f7c24a7577 100644 --- a/httpx/_auth.py +++ b/httpx/_auth.py @@ -160,7 +160,9 @@ def __init__( def auth_flow(self, request: Request) -> typing.Generator[Request, Response, None]: if self._last_challenge: - request.headers["Authorization"] = self._build_auth_header(request, self._last_challenge) + request.headers["Authorization"] = self._build_auth_header( + request, self._last_challenge + ) response = yield request @@ -180,7 +182,9 @@ def auth_flow(self, request: Request) -> typing.Generator[Request, Response, Non self._last_challenge = self._parse_challenge(request, response, auth_header) self._nonce_count = 1 - request.headers["Authorization"] = self._build_auth_header(request, self._last_challenge) + request.headers["Authorization"] = self._build_auth_header( + request, self._last_challenge + ) yield request def _parse_challenge( From 6a1f2c38c186931d3e94eab79dde6b84b5c630f3 Mon Sep 17 00:00:00 2001 From: Philipp Reitter Date: Sun, 27 Nov 2022 20:23:43 +0100 Subject: [PATCH 4/4] codereview changes, removed tomchristie username from all authentication tests --- tests/client/test_auth.py | 84 +++++++++++++++++++++++---------------- tests/test_auth.py | 10 +++-- 2 files changed, 57 insertions(+), 37 deletions(-) diff --git a/tests/client/test_auth.py b/tests/client/test_auth.py index 648677b838..0a7cf745e6 100644 --- a/tests/client/test_auth.py +++ b/tests/client/test_auth.py @@ -8,6 +8,7 @@ import os import threading import typing +from urllib.request import parse_keqv_list import pytest @@ -151,14 +152,14 @@ async def async_auth_flow( @pytest.mark.asyncio async def test_basic_auth() -> None: url = "https://example.org/" - auth = ("tomchristie", "password123") + auth = ("user", "password123") app = App() async with httpx.AsyncClient(transport=httpx.MockTransport(app)) as client: response = await client.get(url, auth=auth) assert response.status_code == 200 - assert response.json() == {"auth": "Basic dG9tY2hyaXN0aWU6cGFzc3dvcmQxMjM="} + assert response.json() == {"auth": "Basic dXNlcjpwYXNzd29yZDEyMw=="} @pytest.mark.asyncio @@ -167,7 +168,7 @@ async def test_basic_auth_with_stream() -> None: See: https://github.com/encode/httpx/pull/1312 """ url = "https://example.org/" - auth = ("tomchristie", "password123") + auth = ("user", "password123") app = App() async with httpx.AsyncClient( @@ -177,25 +178,25 @@ async def test_basic_auth_with_stream() -> None: await response.aread() assert response.status_code == 200 - assert response.json() == {"auth": "Basic dG9tY2hyaXN0aWU6cGFzc3dvcmQxMjM="} + assert response.json() == {"auth": "Basic dXNlcjpwYXNzd29yZDEyMw=="} @pytest.mark.asyncio async def test_basic_auth_in_url() -> None: - url = "https://tomchristie:password123@example.org/" + url = "https://user:password123@example.org/" app = App() async with httpx.AsyncClient(transport=httpx.MockTransport(app)) as client: response = await client.get(url) assert response.status_code == 200 - assert response.json() == {"auth": "Basic dG9tY2hyaXN0aWU6cGFzc3dvcmQxMjM="} + assert response.json() == {"auth": "Basic dXNlcjpwYXNzd29yZDEyMw=="} @pytest.mark.asyncio async def test_basic_auth_on_session() -> None: url = "https://example.org/" - auth = ("tomchristie", "password123") + auth = ("user", "password123") app = App() async with httpx.AsyncClient( @@ -204,7 +205,7 @@ async def test_basic_auth_on_session() -> None: response = await client.get(url) assert response.status_code == 200 - assert response.json() == {"auth": "Basic dG9tY2hyaXN0aWU6cGFzc3dvcmQxMjM="} + assert response.json() == {"auth": "Basic dXNlcjpwYXNzd29yZDEyMw=="} @pytest.mark.asyncio @@ -279,7 +280,7 @@ async def test_trust_env_auth() -> None: @pytest.mark.asyncio async def test_auth_disable_per_request() -> None: url = "https://example.org/" - auth = ("tomchristie", "password123") + auth = ("user", "password123") app = App() async with httpx.AsyncClient( @@ -317,13 +318,13 @@ async def test_auth_property() -> None: async with httpx.AsyncClient(transport=httpx.MockTransport(app)) as client: assert client.auth is None - client.auth = ("tomchristie", "password123") # type: ignore + client.auth = ("user", "password123") # type: ignore assert isinstance(client.auth, BasicAuth) url = "https://example.org/" response = await client.get(url) assert response.status_code == 200 - assert response.json() == {"auth": "Basic dG9tY2hyaXN0aWU6cGFzc3dvcmQxMjM="} + assert response.json() == {"auth": "Basic dXNlcjpwYXNzd29yZDEyMw=="} @pytest.mark.asyncio @@ -347,7 +348,7 @@ async def test_auth_invalid_type() -> None: @pytest.mark.asyncio async def test_digest_auth_returns_no_auth_if_no_digest_header_in_response() -> None: url = "https://example.org/" - auth = DigestAuth(username="tomchristie", password="password123") + auth = DigestAuth(username="user", password="password123") app = App() async with httpx.AsyncClient(transport=httpx.MockTransport(app)) as client: @@ -360,7 +361,7 @@ async def test_digest_auth_returns_no_auth_if_no_digest_header_in_response() -> def test_digest_auth_returns_no_auth_if_alternate_auth_scheme() -> None: url = "https://example.org/" - auth = DigestAuth(username="tomchristie", password="password123") + auth = DigestAuth(username="user", password="password123") auth_header = "Token ..." app = App(auth_header=auth_header, status_code=401) @@ -375,7 +376,7 @@ def test_digest_auth_returns_no_auth_if_alternate_auth_scheme() -> None: @pytest.mark.asyncio async def test_digest_auth_200_response_including_digest_auth_header() -> None: url = "https://example.org/" - auth = DigestAuth(username="tomchristie", password="password123") + auth = DigestAuth(username="user", password="password123") auth_header = 'Digest realm="realm@host.com",qop="auth",nonce="abc",opaque="xyz"' app = App(auth_header=auth_header, status_code=200) @@ -390,7 +391,7 @@ async def test_digest_auth_200_response_including_digest_auth_header() -> None: @pytest.mark.asyncio async def test_digest_auth_401_response_without_digest_auth_header() -> None: url = "https://example.org/" - auth = DigestAuth(username="tomchristie", password="password123") + auth = DigestAuth(username="user", password="password123") app = App(auth_header="", status_code=401) async with httpx.AsyncClient(transport=httpx.MockTransport(app)) as client: @@ -419,7 +420,7 @@ async def test_digest_auth( algorithm: str, expected_hash_length: int, expected_response_length: int ) -> None: url = "https://example.org/" - auth = DigestAuth(username="tomchristie", password="password123") + auth = DigestAuth(username="user", password="password123") app = DigestApp(algorithm=algorithm) async with httpx.AsyncClient(transport=httpx.MockTransport(app)) as client: @@ -435,7 +436,7 @@ async def test_digest_auth( response_fields = [field.strip() for field in fields.split(",")] digest_data = dict(field.split("=") for field in response_fields) - assert digest_data["username"] == '"tomchristie"' + assert digest_data["username"] == '"user"' assert digest_data["realm"] == '"httpx@example.org"' assert "nonce" in digest_data assert digest_data["uri"] == '"/"' @@ -450,7 +451,7 @@ async def test_digest_auth( @pytest.mark.asyncio async def test_digest_auth_no_specified_qop() -> None: url = "https://example.org/" - auth = DigestAuth(username="tomchristie", password="password123") + auth = DigestAuth(username="user", password="password123") app = DigestApp(qop="") async with httpx.AsyncClient(transport=httpx.MockTransport(app)) as client: @@ -469,7 +470,7 @@ async def test_digest_auth_no_specified_qop() -> None: assert "qop" not in digest_data assert "nc" not in digest_data assert "cnonce" not in digest_data - assert digest_data["username"] == '"tomchristie"' + assert digest_data["username"] == '"user"' assert digest_data["realm"] == '"httpx@example.org"' assert len(digest_data["nonce"]) == 64 + 2 # extra quotes assert digest_data["uri"] == '"/"' @@ -482,7 +483,7 @@ async def test_digest_auth_no_specified_qop() -> None: @pytest.mark.asyncio async def test_digest_auth_qop_including_spaces_and_auth_returns_auth(qop: str) -> None: url = "https://example.org/" - auth = DigestAuth(username="tomchristie", password="password123") + auth = DigestAuth(username="user", password="password123") app = DigestApp(qop=qop) async with httpx.AsyncClient(transport=httpx.MockTransport(app)) as client: @@ -495,7 +496,7 @@ async def test_digest_auth_qop_including_spaces_and_auth_returns_auth(qop: str) @pytest.mark.asyncio async def test_digest_auth_qop_auth_int_not_implemented() -> None: url = "https://example.org/" - auth = DigestAuth(username="tomchristie", password="password123") + auth = DigestAuth(username="user", password="password123") app = DigestApp(qop="auth-int") async with httpx.AsyncClient(transport=httpx.MockTransport(app)) as client: @@ -506,7 +507,7 @@ async def test_digest_auth_qop_auth_int_not_implemented() -> None: @pytest.mark.asyncio async def test_digest_auth_qop_must_be_auth_or_auth_int() -> None: url = "https://example.org/" - auth = DigestAuth(username="tomchristie", password="password123") + auth = DigestAuth(username="user", password="password123") app = DigestApp(qop="not-auth") async with httpx.AsyncClient(transport=httpx.MockTransport(app)) as client: @@ -517,7 +518,7 @@ async def test_digest_auth_qop_must_be_auth_or_auth_int() -> None: @pytest.mark.asyncio async def test_digest_auth_incorrect_credentials() -> None: url = "https://example.org/" - auth = DigestAuth(username="tomchristie", password="password123") + auth = DigestAuth(username="user", password="password123") app = DigestApp(send_response_after_attempt=2) async with httpx.AsyncClient(transport=httpx.MockTransport(app)) as client: @@ -530,7 +531,7 @@ async def test_digest_auth_incorrect_credentials() -> None: @pytest.mark.asyncio async def test_digest_auth_reuses_challenge() -> None: url = "https://example.org/" - auth = DigestAuth(username="tomchristie", password="password123") + auth = DigestAuth(username="user", password="password123") app = DigestApp() async with httpx.AsyncClient(transport=httpx.MockTransport(app)) as client: @@ -547,25 +548,40 @@ async def test_digest_auth_reuses_challenge() -> None: @pytest.mark.asyncio async def test_digest_auth_resets_nonce_count_after_401() -> None: url = "https://example.org/" - auth = DigestAuth(username="tomchristie", password="password123") + auth = DigestAuth(username="user", password="password123") app = DigestApp() async with httpx.AsyncClient(transport=httpx.MockTransport(app)) as client: response_1 = await client.get(url, auth=auth) assert response_1.status_code == 200 assert len(response_1.history) == 1 - first_nonce = response_1.json()["auth"].split("nonce=")[1].split(",")[0] - first_nc = response_1.json()["auth"].split("nc=")[1].split(",")[0] + first_nonce = parse_keqv_list( + response_1.request.headers["Authorization"].split(", ") + )["nonce"] + first_nc = parse_keqv_list( + response_1.request.headers["Authorization"].split(", ") + )["nc"] + + # with this we now force a 401 on a subsequent (but initial) request app.send_response_after_attempt = 2 + + # we expect the client again to try to authenticate, i.e. the history length must be 1 response_2 = await client.get(url, auth=auth) assert response_2.status_code == 200 assert len(response_2.history) == 1 - second_nonce = response_2.json()["auth"].split("nonce=")[1].split(",")[0] - second_nc = response_2.json()["auth"].split("nc=")[1].split(",")[0] - assert first_nonce != second_nonce - assert first_nc == second_nc + second_nonce = parse_keqv_list( + response_2.request.headers["Authorization"].split(", ") + )["nonce"] + second_nc = parse_keqv_list( + response_2.request.headers["Authorization"].split(", ") + )["nc"] + + assert first_nonce != second_nonce # ensures that the auth challenge was reset + assert ( + first_nc == second_nc + ) # ensures the nonce count is reset when the authentication failed @pytest.mark.parametrize( @@ -580,7 +596,7 @@ async def test_async_digest_auth_raises_protocol_error_on_malformed_header( auth_header: str, ) -> None: url = "https://example.org/" - auth = DigestAuth(username="tomchristie", password="password123") + auth = DigestAuth(username="user", password="password123") app = App(auth_header=auth_header, status_code=401) async with httpx.AsyncClient(transport=httpx.MockTransport(app)) as client: @@ -599,7 +615,7 @@ def test_sync_digest_auth_raises_protocol_error_on_malformed_header( auth_header: str, ) -> None: url = "https://example.org/" - auth = DigestAuth(username="tomchristie", password="password123") + auth = DigestAuth(username="user", password="password123") app = App(auth_header=auth_header, status_code=401) with httpx.Client(transport=httpx.MockTransport(app)) as client: @@ -670,7 +686,7 @@ async def handle_async_request(self, request: Request) -> Response: @pytest.mark.asyncio async def test_digest_auth_unavailable_streaming_body(): url = "https://example.org/" - auth = DigestAuth(username="tomchristie", password="password123") + auth = DigestAuth(username="user", password="password123") app = DigestApp() async def streaming_body(): diff --git a/tests/test_auth.py b/tests/test_auth.py index 025e82ae60..a1997c2fe2 100644 --- a/tests/test_auth.py +++ b/tests/test_auth.py @@ -3,6 +3,8 @@ Integration tests also exist in tests/client/test_auth.py """ +from urllib.request import parse_keqv_list + import pytest import httpx @@ -89,9 +91,11 @@ def test_digest_auth_with_401_nonce_counting(): assert second_request.headers["Authorization"].startswith("Digest") # ... and the client nonce count (nc) is increased - first_nonce = first_request.headers["Authorization"].split(",")[-2].split("=")[1] - second_nonce = second_request.headers["Authorization"].split(",")[-2].split("=")[1] - assert int(first_nonce, 16) + 1 == int(second_nonce, 16) + first_nc = parse_keqv_list(first_request.headers["Authorization"].split(", "))["nc"] + second_nc = parse_keqv_list(second_request.headers["Authorization"].split(", "))[ + "nc" + ] + assert int(first_nc, 16) + 1 == int(second_nc, 16) # No other requests are made. response = httpx.Response(content=b"Hello, world!", status_code=200)