Skip to content

Commit

Permalink
Add support for pulling username from keyring subprocess provider
Browse files Browse the repository at this point in the history
This fixes pypa#12543 (and probably
pypa#12661).

Somewhat confusingly, when using using `PIP_KEYRING_PROVIDER=import`,
pip was able to fetch both a username and a password from keyring, but
when using `PIP_KEYRING_PROVIDER=subprocess`, it needed the username.

I opted not to write a new test for this, as the existing
[test_prompt_for_keyring_if_needed](https://github.com/pypa/pip/blob/86b8b23c1eaaac5c420a28519daf91de830f1182/tests/functional/test_install_config.py#L392)
feels sufficient to me.

TODO/need feedback:
- I haven't implemented any "feature detection" to see if the `keyring`
  subprocess supports `--mode=creds`. This feature was added quite
  recently
  (jaraco/keyring@7830a64
  landed in keyring v25.2.0). It looks like there's an older `getcreds`
  subcommand which we could probably feature detect as well. I
  personally would inclined to keep things simple and only support the
  latest version of the `keyring` cli, but I will gladly implement
  whatever y'all think is best.
  • Loading branch information
jfly committed Jun 5, 2024
1 parent 86b8b23 commit 1bf71c8
Show file tree
Hide file tree
Showing 3 changed files with 16 additions and 18 deletions.
1 change: 1 addition & 0 deletions news/12543.feature.rst
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
Add support for pulling username from keyring subprocess provider
24 changes: 13 additions & 11 deletions src/pip/_internal/network/auth.py
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@
providing credentials in the context of network requests.
"""

import json
import logging
import os
import shutil
Expand Down Expand Up @@ -115,23 +116,22 @@ def __init__(self, cmd: str) -> None:
self.keyring = cmd

def get_auth_info(self, url: str, username: Optional[str]) -> Optional[AuthInfo]:
# This is the default implementation of keyring.get_credential
# https://github.com/jaraco/keyring/blob/97689324abcf01bd1793d49063e7ca01e03d7d07/keyring/backend.py#L134-L139
if username is not None:
password = self._get_password(url, username)
if password is not None:
return username, password
return None
return self._get_creds(url, username)

def save_auth_info(self, url: str, username: str, password: str) -> None:
return self._set_password(url, username, password)

def _get_password(self, service_name: str, username: str) -> Optional[str]:
"""Mirror the implementation of keyring.get_password using cli"""
def _get_creds(
self, service_name: str, username: Optional[str]
) -> Optional[AuthInfo]:
"""Mirror the implementation of keyring.get_credential using cli"""
if self.keyring is None:
return None

cmd = [self.keyring, "get", service_name, username]
cmd = [self.keyring, "--mode=creds", "--output=json", "get", service_name]
if username is not None:
cmd.append(username)

env = os.environ.copy()
env["PYTHONIOENCODING"] = "utf-8"
res = subprocess.run(
Expand All @@ -142,7 +142,9 @@ def _get_password(self, service_name: str, username: str) -> Optional[str]:
)
if res.returncode:
return None
return res.stdout.decode("utf-8").strip(os.linesep)

data = json.loads(res.stdout.decode("utf-8"))
return (data["username"], data["password"])

def _set_password(self, service_name: str, username: str, password: str) -> None:
"""Mirror the implementation of keyring.set_password using cli"""
Expand Down
9 changes: 2 additions & 7 deletions tests/functional/test_install_config.py
Original file line number Diff line number Diff line change
Expand Up @@ -501,12 +501,7 @@ def set_password(self, url, username):
"simple",
)

function_name = (
"get_credential"
if keyring_provider_implementation == "import"
else "get_password"
)
if auth_needed:
assert function_name + " was called" in result.stderr
assert "get_credential was called" in result.stderr
else:
assert function_name + " was called" not in result.stderr
assert "get_credential was called" not in result.stderr

0 comments on commit 1bf71c8

Please sign in to comment.