From 51475ec9930140db552307a803e97656e962c9df Mon Sep 17 00:00:00 2001 From: Dos Moonen Date: Fri, 8 Apr 2022 23:26:32 +0200 Subject: [PATCH 1/5] Add failing test for the use case of supplying --no-input and having keyring queried for credentials. Since there is currently no possible way to get pip to query the keyring when --no-input is used I am not marking the new scenario xfail. That should change once there is some way to do that. --- tests/functional/test_install_config.py | 26 ++++++++++++++++++++++++- tests/unit/test_network_auth.py | 5 +++-- 2 files changed, 28 insertions(+), 3 deletions(-) diff --git a/tests/functional/test_install_config.py b/tests/functional/test_install_config.py index 99e59b97b18..aaefa29bdaa 100644 --- a/tests/functional/test_install_config.py +++ b/tests/functional/test_install_config.py @@ -3,6 +3,7 @@ import sys import tempfile import textwrap +import typing import pytest @@ -361,16 +362,38 @@ def test_do_not_prompt_for_authentication( assert "ERROR: HTTP error 401" in result.stderr +@pytest.fixture(params=(True, False), ids=("auth_needed", "auth_not_needed")) +def auth_needed(request: pytest.FixtureRequest) -> bool: + return request.param # type: ignore[attr-defined] + + +@pytest.fixture( + params=( + False, + True, + ), + ids=("default", "no_input"), +) +def flags(request: pytest.FixtureRequest) -> typing.List[str]: + no_input = request.param # type: ignore[attr-defined] + + flags = [] + if no_input: + flags.append("--no-input") + + return flags + + @pytest.mark.skipif( sys.platform == "linux" and sys.version_info < (3, 8), reason="Custom SSL certification not running well in CI", ) -@pytest.mark.parametrize("auth_needed", (True, False)) def test_prompt_for_keyring_if_needed( script: PipTestEnvironment, data: TestData, cert_factory: CertFactory, auth_needed: bool, + flags: typing.List[str], ) -> None: """Test behaviour while installing from a index url requiring authentication and keyring is possible. @@ -421,6 +444,7 @@ def get_credential(url, username): cert_path, "--client-cert", cert_path, + *flags, "simple", ) diff --git a/tests/unit/test_network_auth.py b/tests/unit/test_network_auth.py index 625a20a48f5..5e9e325a15f 100644 --- a/tests/unit/test_network_auth.py +++ b/tests/unit/test_network_auth.py @@ -1,4 +1,5 @@ import functools +import os import sys from typing import Any, Dict, Iterable, List, Optional, Tuple @@ -360,7 +361,7 @@ def __call__( self.returncode = 1 else: # Passwords are returned encoded with a newline appended - self.stdout = password.encode("utf-8") + b"\n" + self.stdout = (password + os.linesep).encode("utf-8") if cmd[1] == "set": assert stdin is None @@ -369,7 +370,7 @@ def __call__( assert input is not None # Input from stdin is encoded - self.set_password(cmd[2], cmd[3], input.decode("utf-8").strip("\n")) + self.set_password(cmd[2], cmd[3], input.decode("utf-8").strip(os.linesep)) return self From 497761c96c112d69258ff24999efa945b38a0c1d Mon Sep 17 00:00:00 2001 From: Dos Moonen Date: Fri, 8 Apr 2022 09:55:42 +0200 Subject: [PATCH 2/5] Add cli flag `--force-keyring`. Currently, Pip is conservative and does not query keyring at all when `--no-input` is used because the keyring might require user interaction such as prompting the user on the console. This commit adds a flag to force keyring usage if it is installed. It defaults to `False`, making this opt-in behaviour. Tools such as Pipx and Pipenv use Pip and have their own progress indicator that hides output from the subprocess Pip runs in. They should pass `--no-input` in my opinion, but Pip should provide some mechanism to still query the keyring in that case. Just not by default. So here we are. --- src/pip/_internal/cli/cmdoptions.py | 14 +++++++++++ src/pip/_internal/cli/req_command.py | 4 ++++ src/pip/_internal/network/auth.py | 36 +++++++++++++++++----------- 3 files changed, 40 insertions(+), 14 deletions(-) diff --git a/src/pip/_internal/cli/cmdoptions.py b/src/pip/_internal/cli/cmdoptions.py index 661c489c73e..1e0f7396229 100644 --- a/src/pip/_internal/cli/cmdoptions.py +++ b/src/pip/_internal/cli/cmdoptions.py @@ -244,6 +244,19 @@ class PipOption(Option): help="Disable prompting for input.", ) +force_keyring: Callable[..., Option] = partial( + Option, + "--force-keyring", + dest="force_keyring", + action="store_true", + default=False, + help=( + "Always query the keyring, regardless of pip's --no-input option. Note" + " that this may cause problems if the keyring expects to be able to" + " prompt the user interactively and no interactive user is available." + ), +) + proxy: Callable[..., Option] = partial( Option, "--proxy", @@ -1019,6 +1032,7 @@ def check_list_path_option(options: Values) -> None: quiet, log, no_input, + force_keyring, proxy, retries, timeout, diff --git a/src/pip/_internal/cli/req_command.py b/src/pip/_internal/cli/req_command.py index 1044809f040..4bbbed9d312 100644 --- a/src/pip/_internal/cli/req_command.py +++ b/src/pip/_internal/cli/req_command.py @@ -151,6 +151,10 @@ def _build_session( # Determine if we can prompt the user for authentication or not session.auth.prompting = not options.no_input + # We won't use keyring when --no-input is passed unless + # --force-keyring is passed as well because it might require + # user interaction + session.auth.use_keyring = session.auth.prompting or options.force_keyring return session diff --git a/src/pip/_internal/network/auth.py b/src/pip/_internal/network/auth.py index 68b5a5f45be..777074fe076 100644 --- a/src/pip/_internal/network/auth.py +++ b/src/pip/_internal/network/auth.py @@ -128,7 +128,7 @@ def _get_password(self, service_name: str, username: str) -> Optional[str]: ) if res.returncode: return None - return res.stdout.decode("utf-8").strip("\n") + return res.stdout.decode("utf-8").strip(os.linesep) def _set_password(self, service_name: str, username: str, password: str) -> None: """Mirror the implementation of keyring.set_password using cli""" @@ -136,7 +136,7 @@ def _set_password(self, service_name: str, username: str, password: str) -> None return None cmd = [self.keyring, "set", service_name, username] - input_ = password.encode("utf-8") + b"\n" + input_ = (password + os.linesep).encode("utf-8") env = os.environ.copy() env["PYTHONIOENCODING"] = "utf-8" res = subprocess.run(cmd, input=input_, env=env) @@ -190,10 +190,14 @@ 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, + use_keyring: bool = True, ) -> None: self.prompting = prompting self.index_urls = index_urls + self.use_keyring = use_keyring self.passwords: Dict[str, AuthInfo] = {} # When the user is prompted to enter credentials and keyring is # available, we will offer to save them. If the user accepts, @@ -227,7 +231,8 @@ def _get_index_url(self, url: str) -> Optional[str]: def _get_new_credentials( self, original_url: str, - allow_netrc: bool = True, + *, + allow_netrc: bool = False, allow_keyring: bool = False, ) -> AuthInfo: """Find and return credentials for the specified URL.""" @@ -348,7 +353,7 @@ def __call__(self, req: Request) -> Request: def _prompt_for_password( self, netloc: str ) -> Tuple[Optional[str], Optional[str], bool]: - username = ask_input(f"User for {netloc}: ") + username = ask_input(f"User for {netloc}: ") if self.prompting else None if not username: return None, None, False auth = get_keyring_auth(netloc, username) @@ -359,7 +364,7 @@ def _prompt_for_password( # Factored out to allow for easy patching in tests def _should_save_password_to_keyring(self) -> bool: - if get_keyring_provider() is None: + if not self.prompting or get_keyring_provider() is None: return False return ask("Save credentials to keyring [y/N]: ", ["y", "n"]) == "y" @@ -369,19 +374,22 @@ def handle_401(self, resp: Response, **kwargs: Any) -> Response: if resp.status_code != 401: return resp + username, password = None, None + + # Query the keyring for credentials: + if self.use_keyring: + username, password = self._get_new_credentials( + resp.url, + allow_netrc=False, + allow_keyring=True, + ) + # We are not able to prompt the user so simply return the response - if not self.prompting: + if not self.prompting and not username and not password: return resp 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 save = False if not username and not password: From 629de94ac0e572770125e7fd962f51e7982afb2a Mon Sep 17 00:00:00 2001 From: Dos Moonen Date: Fri, 8 Apr 2022 21:41:19 +0200 Subject: [PATCH 3/5] Parameterized tests/functional/test_install_config.py::test_prompt_for_keyring_if_needed to add a few --no-input and --force-keyring scenarios --- tests/functional/test_install_config.py | 28 +++++++++++++++++++------ 1 file changed, 22 insertions(+), 6 deletions(-) diff --git a/tests/functional/test_install_config.py b/tests/functional/test_install_config.py index aaefa29bdaa..e619b7827be 100644 --- a/tests/functional/test_install_config.py +++ b/tests/functional/test_install_config.py @@ -369,18 +369,34 @@ def auth_needed(request: pytest.FixtureRequest) -> bool: @pytest.fixture( params=( - False, - True, + (False, False, False), + (False, True, False), + (True, True, False), + (True, False, True), + ), + ids=( + "default-default", + "default-force_keyring", + "no_input-force_keyring", + "no_input-default", ), - ids=("default", "no_input"), ) -def flags(request: pytest.FixtureRequest) -> typing.List[str]: - no_input = request.param # type: ignore[attr-defined] +def flags(request: pytest.FixtureRequest, auth_needed: bool) -> typing.List[str]: + ( + no_input, + force_keyring, + xfail, + ) = request.param # type: ignore[attr-defined] flags = [] if no_input: flags.append("--no-input") - + if force_keyring: + flags.append( + "--force-keyring", + ) + if auth_needed and xfail: + request.applymarker(pytest.mark.xfail()) return flags From fe03e9c4cb1ebee78a63a3c992e2d3cae785e9a3 Mon Sep 17 00:00:00 2001 From: Dos Moonen Date: Sat, 9 Apr 2022 21:31:59 +0200 Subject: [PATCH 4/5] Added a news entry and some documentation about `--force-keyring` --- docs/html/topics/authentication.md | 19 +++++++++++++++++++ news/11020.feature.rst | 3 +++ 2 files changed, 22 insertions(+) create mode 100644 news/11020.feature.rst diff --git a/docs/html/topics/authentication.md b/docs/html/topics/authentication.md index 981aab5abd7..7381af73a98 100644 --- a/docs/html/topics/authentication.md +++ b/docs/html/topics/authentication.md @@ -74,6 +74,25 @@ $ echo "your-password" | keyring set pypi.company.com your-username $ pip install your-package --index-url https://pypi.company.com/ ``` +Pip is conservative and does not query keyring at all when `--no-input` is used +because the keyring might require user interaction such as prompting the user +on the console. You can force keyring usage by passing `--force-keyring` or one +of the following: + +```bash +# possibly with --user, --global or --site +$ pip config set global.force-keyring true +# or +$ export PIP_FORCE_KEYRING=1 +``` + +```{warning} +Be careful when doing this since it could cause tools such as Pipx and Pipenv +to appear to hang. They show their own progress indicator while hiding output +from the subprocess in which they run Pip. You won't know whether the keyring +backend is waiting the user input or not in such situations. +``` + Note that `keyring` (the Python package) needs to be installed separately from pip. This can create a bootstrapping issue if you need the credentials stored in the keyring to download and install keyring. diff --git a/news/11020.feature.rst b/news/11020.feature.rst new file mode 100644 index 00000000000..87baa973104 --- /dev/null +++ b/news/11020.feature.rst @@ -0,0 +1,3 @@ +Add ``--force-keyring`` flag which allows ``keyring`` lookups in combination +with ``--no-input``. See the Authentication page in the documentation for +more info. From 7fd045c5cc3fc62dda7bd642192effe2246dbd89 Mon Sep 17 00:00:00 2001 From: Dos Moonen Date: Mon, 17 Oct 2022 13:54:36 +0200 Subject: [PATCH 5/5] Unpin pytest in .pre-commit-config.yaml and remove `type: ignore[attr-defined]` comments that are no longer needed as of pytest 7.1.3 --- .pre-commit-config.yaml | 2 +- tests/functional/test_install_config.py | 4 ++-- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/.pre-commit-config.yaml b/.pre-commit-config.yaml index 20a85438c5a..6fc1986c971 100644 --- a/.pre-commit-config.yaml +++ b/.pre-commit-config.yaml @@ -47,7 +47,7 @@ repos: additional_dependencies: [ 'keyring==23.0.1', 'nox==2021.6.12', - 'pytest==7.1.1', + 'pytest', 'types-docutils==0.18.3', 'types-setuptools==57.4.14', 'types-freezegun==1.1.9', diff --git a/tests/functional/test_install_config.py b/tests/functional/test_install_config.py index e619b7827be..b47b50274e7 100644 --- a/tests/functional/test_install_config.py +++ b/tests/functional/test_install_config.py @@ -364,7 +364,7 @@ def test_do_not_prompt_for_authentication( @pytest.fixture(params=(True, False), ids=("auth_needed", "auth_not_needed")) def auth_needed(request: pytest.FixtureRequest) -> bool: - return request.param # type: ignore[attr-defined] + return request.param @pytest.fixture( @@ -386,7 +386,7 @@ def flags(request: pytest.FixtureRequest, auth_needed: bool) -> typing.List[str] no_input, force_keyring, xfail, - ) = request.param # type: ignore[attr-defined] + ) = request.param flags = [] if no_input: