From 7375754896e0b562bb27d8e95f10ad88e1bb5332 Mon Sep 17 00:00:00 2001 From: James Gerity Date: Wed, 29 Jun 2022 00:30:47 -0400 Subject: [PATCH 1/8] Add --enable-keyring flag --- src/pip/_internal/cli/cmdoptions.py | 9 +++++++++ src/pip/_internal/cli/req_command.py | 1 + 2 files changed, 10 insertions(+) diff --git a/src/pip/_internal/cli/cmdoptions.py b/src/pip/_internal/cli/cmdoptions.py index 84e0e783869..87f5e088833 100644 --- a/src/pip/_internal/cli/cmdoptions.py +++ b/src/pip/_internal/cli/cmdoptions.py @@ -1024,6 +1024,15 @@ def check_list_path_option(options: Values) -> None: help=("Enable deprecated functionality, that will be removed in the future."), ) +keyring: Callable[..., Option] = partial( + Option, + "--enable-keyring", + dest="keyring", + action="store_true", + default=False, + help="Enable keyring authentication", +) + ########## # groups # diff --git a/src/pip/_internal/cli/req_command.py b/src/pip/_internal/cli/req_command.py index 1044809f040..a2575d63ecd 100644 --- a/src/pip/_internal/cli/req_command.py +++ b/src/pip/_internal/cli/req_command.py @@ -260,6 +260,7 @@ def __init__(self, *args: Any, **kw: Any) -> None: super().__init__(*args, **kw) self.cmd_opts.add_option(cmdoptions.no_clean()) + self.cmd_opts.add_option(cmdoptions.keyring()) @staticmethod def determine_resolver_variant(options: Values) -> str: From b4c18331eac0f7e735ba0cd544b1f25016198d69 Mon Sep 17 00:00:00 2001 From: James Gerity Date: Wed, 29 Jun 2022 00:54:02 -0400 Subject: [PATCH 2/8] Pass keyring flag down to PipSession, MultiDomainBasicAuth --- src/pip/_internal/cli/req_command.py | 1 + src/pip/_internal/network/auth.py | 10 ++++++---- src/pip/_internal/network/session.py | 5 ++++- 3 files changed, 11 insertions(+), 5 deletions(-) diff --git a/src/pip/_internal/cli/req_command.py b/src/pip/_internal/cli/req_command.py index a2575d63ecd..d836dc04315 100644 --- a/src/pip/_internal/cli/req_command.py +++ b/src/pip/_internal/cli/req_command.py @@ -128,6 +128,7 @@ def _build_session( trusted_hosts=options.trusted_hosts, index_urls=self._get_index_urls(options), ssl_context=ssl_context, + allow_keyring=options.keyring, ) # Handle custom ca-bundles from the user diff --git a/src/pip/_internal/network/auth.py b/src/pip/_internal/network/auth.py index ca42798bd95..7aa9b979e0f 100644 --- a/src/pip/_internal/network/auth.py +++ b/src/pip/_internal/network/auth.py @@ -72,11 +72,15 @@ def get_keyring_auth(url: Optional[str], username: Optional[str]) -> Optional[Au class MultiDomainBasicAuth(AuthBase): def __init__( - self, prompting: bool = True, index_urls: Optional[List[str]] = None + self, + prompting: bool = True, + index_urls: Optional[List[str]] = None, + allow_keyring: bool = False, ) -> None: self.prompting = prompting self.index_urls = index_urls self.passwords: Dict[str, AuthInfo] = {} + self.allow_keyring = allow_keyring # When the user is prompted to enter credentials and keyring is # available, we will offer to save them. If the user accepts, # this value is set to the credentials they entered. After the @@ -110,7 +114,6 @@ def _get_new_credentials( self, original_url: str, allow_netrc: bool = True, - allow_keyring: bool = False, ) -> AuthInfo: """Find and return credentials for the specified URL.""" # Split the credentials and netloc from the url. @@ -148,7 +151,7 @@ def _get_new_credentials( return netrc_auth # If we don't have a password and keyring is available, use it. - if allow_keyring: + if self.allow_keyring: # The index url is more specific than the netloc, so try it first # fmt: off kr_auth = ( @@ -261,7 +264,6 @@ def handle_401(self, resp: Response, **kwargs: Any) -> Response: username, password = self._get_new_credentials( resp.url, allow_netrc=False, - allow_keyring=True, ) # Prompt the user for a new username and password diff --git a/src/pip/_internal/network/session.py b/src/pip/_internal/network/session.py index e512ac78464..0ef9c9c0eaa 100644 --- a/src/pip/_internal/network/session.py +++ b/src/pip/_internal/network/session.py @@ -327,6 +327,7 @@ def __init__( trusted_hosts: Sequence[str] = (), index_urls: Optional[List[str]] = None, ssl_context: Optional["SSLContext"] = None, + allow_keyring: bool = False, **kwargs: Any, ) -> None: """ @@ -343,7 +344,9 @@ def __init__( self.headers["User-Agent"] = user_agent() # Attach our Authentication handler to the session - self.auth = MultiDomainBasicAuth(index_urls=index_urls) + self.auth = MultiDomainBasicAuth( + index_urls=index_urls, allow_keyring=allow_keyring + ) # Create our urllib3.Retry instance which will allow us to customize # how we handle retries. From 27fc549fe0418b0e439c24753ac504f9f3bb8a4c Mon Sep 17 00:00:00 2001 From: James Gerity Date: Wed, 29 Jun 2022 01:15:15 -0400 Subject: [PATCH 3/8] Add news fragment --- news/8719.feature.rst | 1 + 1 file changed, 1 insertion(+) create mode 100644 news/8719.feature.rst diff --git a/news/8719.feature.rst b/news/8719.feature.rst new file mode 100644 index 00000000000..51de5c2ca70 --- /dev/null +++ b/news/8719.feature.rst @@ -0,0 +1 @@ +Make support for the ``keyring`` library opt-in with the ``--enable-keyring`` flag From bd25b57c236b3a7064236b7e2c0216a88421e980 Mon Sep 17 00:00:00 2001 From: James Gerity Date: Wed, 29 Jun 2022 01:20:51 -0400 Subject: [PATCH 4/8] Update tests for new MultiDomainBasicAuth signature --- tests/unit/test_network_auth.py | 32 ++++++++++++++++---------------- 1 file changed, 16 insertions(+), 16 deletions(-) diff --git a/tests/unit/test_network_auth.py b/tests/unit/test_network_auth.py index 5c0e5746281..44f567de3e0 100644 --- a/tests/unit/test_network_auth.py +++ b/tests/unit/test_network_auth.py @@ -91,10 +91,10 @@ def test_get_credentials_uses_cached_credentials_only_username() -> None: def test_get_index_url_credentials() -> None: - auth = MultiDomainBasicAuth(index_urls=["http://foo:bar@example.com/path"]) - get = functools.partial( - auth._get_new_credentials, allow_netrc=False, allow_keyring=False + auth = MultiDomainBasicAuth( + index_urls=["http://foo:bar@example.com/path"], allow_keyring=False ) + get = functools.partial(auth._get_new_credentials, allow_netrc=False) # Check resolution of indexes assert get("http://example.com/path/path2") == ("foo", "bar") @@ -139,9 +139,11 @@ def test_keyring_get_password( ) -> None: keyring = KeyringModuleV1() monkeypatch.setattr("pip._internal.network.auth.keyring", keyring) - auth = MultiDomainBasicAuth(index_urls=["http://example.com/path2"]) + auth = MultiDomainBasicAuth( + index_urls=["http://example.com/path2"], allow_keyring=True + ) - actual = auth._get_new_credentials(url, allow_netrc=False, allow_keyring=True) + actual = auth._get_new_credentials(url, allow_netrc=False) assert actual == expect @@ -185,10 +187,10 @@ def test_keyring_get_password_username_in_index( ) -> None: keyring = KeyringModuleV1() monkeypatch.setattr("pip._internal.network.auth.keyring", keyring) - auth = MultiDomainBasicAuth(index_urls=["http://user@example.com/path2"]) - get = functools.partial( - auth._get_new_credentials, allow_netrc=False, allow_keyring=True + auth = MultiDomainBasicAuth( + index_urls=["http://user@example.com/path2"], allow_keyring=True ) + get = functools.partial(auth._get_new_credentials, allow_netrc=False) assert get("http://example.com/path2/path3") == ("user", "user!url") assert get("http://example.com/path4/path1") == (None, None) @@ -294,12 +296,12 @@ def test_keyring_get_credential( monkeypatch: pytest.MonkeyPatch, url: str, expect: str ) -> None: monkeypatch.setattr(pip._internal.network.auth, "keyring", KeyringModuleV2()) - auth = MultiDomainBasicAuth(index_urls=["http://example.com/path2"]) - - assert ( - auth._get_new_credentials(url, allow_netrc=False, allow_keyring=True) == expect + auth = MultiDomainBasicAuth( + index_urls=["http://example.com/path2"], allow_keyring=True ) + assert auth._get_new_credentials(url, allow_netrc=False) == expect + class KeyringModuleBroken: """Represents the current supported API of keyring, but broken""" @@ -316,12 +318,10 @@ def test_broken_keyring_disables_keyring(monkeypatch: pytest.MonkeyPatch) -> Non keyring_broken = KeyringModuleBroken() monkeypatch.setattr(pip._internal.network.auth, "keyring", keyring_broken) - auth = MultiDomainBasicAuth(index_urls=["http://example.com/"]) + auth = MultiDomainBasicAuth(index_urls=["http://example.com/"], allow_keyring=True) assert keyring_broken._call_count == 0 for i in range(5): url = "http://example.com/path" + str(i) - assert auth._get_new_credentials( - url, allow_netrc=False, allow_keyring=True - ) == (None, None) + assert auth._get_new_credentials(url, allow_netrc=False) == (None, None) assert keyring_broken._call_count == 1 From a36d18dabcd8e78e668ee06563fd84e6ab157274 Mon Sep 17 00:00:00 2001 From: James Gerity Date: Wed, 29 Jun 2022 01:32:32 -0400 Subject: [PATCH 5/8] Add keyring option to failing test --- tests/unit/test_commands.py | 12 +++++++----- 1 file changed, 7 insertions(+), 5 deletions(-) diff --git a/tests/unit/test_commands.py b/tests/unit/test_commands.py index 7a5c4e8319d..734bb445033 100644 --- a/tests/unit/test_commands.py +++ b/tests/unit/test_commands.py @@ -76,14 +76,14 @@ def has_option_no_index(command: Command) -> bool: @pytest.mark.parametrize("command_name", EXPECTED_INDEX_GROUP_COMMANDS) @pytest.mark.parametrize( - "disable_pip_version_check, no_index, expected_called", + "disable_pip_version_check, no_index, keyring, expected_called", [ # pip_self_version_check() is only called when both # disable_pip_version_check and no_index are False. - (False, False, True), - (False, True, False), - (True, False, False), - (True, True, False), + (False, False, False, True), + (False, True, False, False), + (True, False, False, False), + (True, True, False, False), ], ) @mock.patch("pip._internal.cli.req_command.pip_self_version_check") @@ -92,6 +92,7 @@ def test_index_group_handle_pip_version_check( command_name: str, disable_pip_version_check: bool, no_index: bool, + keyring: bool, expected_called: bool, ) -> None: """ @@ -103,6 +104,7 @@ def test_index_group_handle_pip_version_check( options = command.parser.get_default_values() options.disable_pip_version_check = disable_pip_version_check options.no_index = no_index + options.keyring = keyring command.handle_pip_version_check(options) if expected_called: From 4768497d2f15891511ccba1f910e3f2bb72ec274 Mon Sep 17 00:00:00 2001 From: James Gerity Date: Wed, 29 Jun 2022 02:07:27 -0400 Subject: [PATCH 6/8] Revert "Add keyring option to failing test" This reverts commit aba9e96271b720d2761b2f7beaffbc70cf926182. --- tests/unit/test_commands.py | 12 +++++------- 1 file changed, 5 insertions(+), 7 deletions(-) diff --git a/tests/unit/test_commands.py b/tests/unit/test_commands.py index 734bb445033..7a5c4e8319d 100644 --- a/tests/unit/test_commands.py +++ b/tests/unit/test_commands.py @@ -76,14 +76,14 @@ def has_option_no_index(command: Command) -> bool: @pytest.mark.parametrize("command_name", EXPECTED_INDEX_GROUP_COMMANDS) @pytest.mark.parametrize( - "disable_pip_version_check, no_index, keyring, expected_called", + "disable_pip_version_check, no_index, expected_called", [ # pip_self_version_check() is only called when both # disable_pip_version_check and no_index are False. - (False, False, False, True), - (False, True, False, False), - (True, False, False, False), - (True, True, False, False), + (False, False, True), + (False, True, False), + (True, False, False), + (True, True, False), ], ) @mock.patch("pip._internal.cli.req_command.pip_self_version_check") @@ -92,7 +92,6 @@ def test_index_group_handle_pip_version_check( command_name: str, disable_pip_version_check: bool, no_index: bool, - keyring: bool, expected_called: bool, ) -> None: """ @@ -104,7 +103,6 @@ def test_index_group_handle_pip_version_check( options = command.parser.get_default_values() options.disable_pip_version_check = disable_pip_version_check options.no_index = no_index - options.keyring = keyring command.handle_pip_version_check(options) if expected_called: From 1cb2cd54585272b7bc920d4edabb98a1135546e7 Mon Sep 17 00:00:00 2001 From: James Gerity Date: Wed, 29 Jun 2022 02:08:45 -0400 Subject: [PATCH 7/8] Move keyring to general_group --- src/pip/_internal/cli/cmdoptions.py | 1 + src/pip/_internal/cli/req_command.py | 1 - 2 files changed, 1 insertion(+), 1 deletion(-) diff --git a/src/pip/_internal/cli/cmdoptions.py b/src/pip/_internal/cli/cmdoptions.py index 87f5e088833..645c75b951b 100644 --- a/src/pip/_internal/cli/cmdoptions.py +++ b/src/pip/_internal/cli/cmdoptions.py @@ -1054,6 +1054,7 @@ def check_list_path_option(options: Values) -> None: proxy, retries, timeout, + keyring, exists_action, trusted_host, cert, diff --git a/src/pip/_internal/cli/req_command.py b/src/pip/_internal/cli/req_command.py index d836dc04315..1490e433c34 100644 --- a/src/pip/_internal/cli/req_command.py +++ b/src/pip/_internal/cli/req_command.py @@ -261,7 +261,6 @@ def __init__(self, *args: Any, **kw: Any) -> None: super().__init__(*args, **kw) self.cmd_opts.add_option(cmdoptions.no_clean()) - self.cmd_opts.add_option(cmdoptions.keyring()) @staticmethod def determine_resolver_variant(options: Values) -> str: From ec0348ddb0b8f6f5336e70c9e234b9895e21fd59 Mon Sep 17 00:00:00 2001 From: James Gerity Date: Wed, 29 Jun 2022 09:44:34 -0400 Subject: [PATCH 8/8] Pass --enable-keyring in keyring test --- tests/functional/test_install_config.py | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-) diff --git a/tests/functional/test_install_config.py b/tests/functional/test_install_config.py index 66043fa1f08..56083b2fb55 100644 --- a/tests/functional/test_install_config.py +++ b/tests/functional/test_install_config.py @@ -400,7 +400,7 @@ def get_credential(url, username): keyring_path.write_text(keyring_content) with server_running(server): - result = script.pip( + args = [ "install", "--index-url", url, @@ -409,7 +409,10 @@ def get_credential(url, username): "--client-cert", cert_path, "simple", - ) + ] + if auth_needed: + args.append("--enable-keyring") + result = script.pip(*args) if auth_needed: assert "get_credential was called" in result.stderr