From 2ba37607dc582b18bbc64219dbe4cd2e0daaa9f8 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Miro=20Hron=C4=8Dok?= Date: Wed, 22 Jul 2020 18:30:59 +0200 Subject: [PATCH] Only query the keyring for URLs that actually trigger error 401 Fixes https://github.com/pypa/pip/issues/8090 --- news/8090.bugfix | 3 ++ src/pip/_internal/network/auth.py | 11 +++++- tests/functional/test_install_config.py | 49 +++++++++++++++++++++++++ tests/lib/server.py | 23 +++++++++--- tests/unit/test_network_auth.py | 2 +- 5 files changed, 80 insertions(+), 8 deletions(-) create mode 100644 news/8090.bugfix diff --git a/news/8090.bugfix b/news/8090.bugfix new file mode 100644 index 00000000000..ff8b80f3c8e --- /dev/null +++ b/news/8090.bugfix @@ -0,0 +1,3 @@ +Only query the keyring for URLs that actually trigger error 401. +This prevents an unnecessary keyring unlock prompt on every pip install +invocation (even with default index URL which is not password protected). diff --git a/src/pip/_internal/network/auth.py b/src/pip/_internal/network/auth.py index 174eb834875..e58d8031186 100644 --- a/src/pip/_internal/network/auth.py +++ b/src/pip/_internal/network/auth.py @@ -112,7 +112,7 @@ def _get_index_url(self, url): return None def _get_new_credentials(self, original_url, allow_netrc=True, - allow_keyring=True): + allow_keyring=False): # type: (str, bool, bool) -> AuthInfo """Find and return credentials for the specified URL.""" # Split the credentials and netloc from the url. @@ -252,8 +252,15 @@ def handle_401(self, resp, **kwargs): parsed = urllib.parse.urlparse(resp.url) + # Query the keyring for credentials: + username, password = self._get_new_credentials(resp.url, + allow_netrc=False, + allow_keyring=True) + # Prompt the user for a new username and password - username, password, save = self._prompt_for_password(parsed.netloc) + save = False + if not username or not password: + username, password, save = self._prompt_for_password(parsed.netloc) # Store the new username and password to use for future requests self._credentials_to_save = None diff --git a/tests/functional/test_install_config.py b/tests/functional/test_install_config.py index 41be6fbbbb6..27e4f0b0cc4 100644 --- a/tests/functional/test_install_config.py +++ b/tests/functional/test_install_config.py @@ -274,3 +274,52 @@ def test_do_not_prompt_for_authentication(script, data, cert_factory): '--no-input', 'simple', expect_error=True) assert "ERROR: HTTP error 401" in result.stderr + + +@pytest.mark.parametrize("auth_needed", (True, False)) +def test_prompt_for_keyring_if_needed(script, data, cert_factory, auth_needed): + """Test behaviour while installing from a index url + requiring authentication and keyring is possible. + """ + cert_path = cert_factory() + ctx = ssl.SSLContext(ssl.PROTOCOL_SSLv23) + ctx.load_cert_chain(cert_path, cert_path) + ctx.load_verify_locations(cafile=cert_path) + ctx.verify_mode = ssl.CERT_REQUIRED + + response = authorization_response if auth_needed else file_response + + server = make_mock_server(ssl_context=ctx) + server.mock.side_effect = [ + package_page({ + "simple-3.0.tar.gz": "/files/simple-3.0.tar.gz", + }), + response(str(data.packages / "simple-3.0.tar.gz")), + response(str(data.packages / "simple-3.0.tar.gz")), + ] + + url = "https://{}:{}/simple".format(server.host, server.port) + + keyring_content = textwrap.dedent("""\ + import os + import sys + from collections import namedtuple + + Cred = namedtuple("Cred", ["username", "password"]) + + def get_credential(url, username): + sys.stderr.write("get_credential was called" + os.linesep) + return Cred("USERNAME", "PASSWORD") + """) + keyring_path = script.site_packages_path / 'keyring.py' + keyring_path.write_text(keyring_content) + + with server_running(server): + result = script.pip('install', "--index-url", url, + "--cert", cert_path, "--client-cert", cert_path, + 'simple') + + if auth_needed: + assert "get_credential was called" in result.stderr + else: + assert "get_credential was called" not in result.stderr diff --git a/tests/lib/server.py b/tests/lib/server.py index cd3c522bfec..dbeff934d6f 100644 --- a/tests/lib/server.py +++ b/tests/lib/server.py @@ -2,6 +2,7 @@ import signal import ssl import threading +from base64 import b64encode from contextlib import contextmanager from textwrap import dedent @@ -219,14 +220,26 @@ def responder(environ, start_response): def authorization_response(path): + # type: (str) -> Responder + correct_auth = "Basic " + b64encode(b"USERNAME:PASSWORD").decode("ascii") + def responder(environ, start_response): # type: (Environ, StartResponse) -> Body - start_response( - "401 Unauthorized", [ - ("WWW-Authenticate", "Basic"), - ], - ) + if environ.get('HTTP_AUTHORIZATION') == correct_auth: + size = os.stat(path).st_size + start_response( + "200 OK", [ + ("Content-Type", "application/octet-stream"), + ("Content-Length", str(size)), + ], + ) + else: + start_response( + "401 Unauthorized", [ + ("WWW-Authenticate", "Basic"), + ], + ) with open(path, 'rb') as f: return [f.read()] diff --git a/tests/unit/test_network_auth.py b/tests/unit/test_network_auth.py index 44c739d864f..a5de733569d 100644 --- a/tests/unit/test_network_auth.py +++ b/tests/unit/test_network_auth.py @@ -267,4 +267,4 @@ def test_broken_keyring_disables_keyring(monkeypatch): assert auth._get_new_credentials( url, allow_netrc=False, allow_keyring=True ) == (None, None) - assert keyring_broken._call_count == 1 + assert keyring_broken._call_count == 1