-
Notifications
You must be signed in to change notification settings - Fork 3k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add an interface to allow calling system keyring
#11589
Changes from 5 commits
b87ddb9
edc588c
4cbae5b
efa7f2b
7e93102
6ec0af5
f5c96b1
43abcf0
5137ce2
4fc2008
888c3b6
996d4fa
4f8a613
3a15e01
8d9ea8b
c04222f
e6e42de
14a3d93
623ac5d
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,2 @@ | ||
Enable the use of ``keyring`` found on ``PATH``. This allows ``keyring`` | ||
installed using ``pipx`` to be used by ``pip``. |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -4,8 +4,10 @@ | |
providing credentials in the context of network requests. | ||
""" | ||
|
||
import shutil | ||
import subprocess | ||
import urllib.parse | ||
from typing import Any, Dict, List, Optional, Tuple | ||
from typing import Any, Dict, List, NamedTuple, Optional, Tuple | ||
|
||
from pip._vendor.requests.auth import AuthBase, HTTPBasicAuth | ||
from pip._vendor.requests.models import Request, Response | ||
|
@@ -23,11 +25,52 @@ | |
|
||
logger = getLogger(__name__) | ||
|
||
Credentials = Tuple[str, str, str] | ||
|
||
class Credentials(NamedTuple): | ||
service_name: str | ||
username: str | ||
password: str | ||
|
||
|
||
class KeyRingCredential(NamedTuple): | ||
username: Optional[str] | ||
password: str | ||
|
||
|
||
class KeyRingCli: | ||
"""Mirror the parts of keyring's API which pip uses | ||
|
||
Instead of calling the keyring package installed alongside pip | ||
we call keyring on the command line which will enable pip to | ||
use which ever installation of keyring is available first in | ||
PATH. | ||
""" | ||
|
||
@classmethod | ||
def get_credential( | ||
cls, service_name: str, username: Optional[str] | ||
) -> Optional[KeyRingCredential]: | ||
cmd = ["keyring", "get", service_name, str(username)] | ||
res = subprocess.run(cmd, capture_output=True) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. You need to support the There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I don't think I understand this. I'm not reading from Did you mean this line There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. No, the |
||
if res.returncode: | ||
return None | ||
password = res.stdout.decode().strip("\n") | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Encoding issues possible here. On Windows, at least, it's not necessarily true that keywring will write its output in the same encoding as the pip process' default encoding. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think I've solved this using |
||
return KeyRingCredential(username=username, password=password) | ||
|
||
@classmethod | ||
def set_password(cls, service_name: str, username: str, password: str) -> None: | ||
cmd = ["keyring", "set", service_name, username] | ||
input_ = password.encode() + b"\n" | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I can imagine encoding issues here, especially on Windows. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think I've solved this using |
||
res = subprocess.run(cmd, input=input_) | ||
res.check_returncode() | ||
return None | ||
|
||
|
||
try: | ||
import keyring | ||
except ImportError: | ||
if shutil.which("keyring") is not None: | ||
keyring = KeyRingCli # type: ignore[assignment] | ||
keyring = None # type: ignore[assignment] | ||
except Exception as exc: | ||
logger.warning( | ||
|
@@ -276,7 +319,11 @@ def handle_401(self, resp: Response, **kwargs: Any) -> Response: | |
|
||
# Prompt to save the password to keyring | ||
if save and self._should_save_password_to_keyring(): | ||
self._credentials_to_save = (parsed.netloc, username, password) | ||
self._credentials_to_save = Credentials( | ||
service_name=parsed.netloc, | ||
username=username, | ||
password=password, | ||
) | ||
|
||
# Consume content and release the original connection to allow our new | ||
# request to reuse the same one. | ||
|
@@ -318,6 +365,6 @@ def save_credentials(self, resp: Response, **kwargs: Any) -> None: | |
if creds and resp.status_code < 400: | ||
try: | ||
logger.info("Saving credentials to keyring") | ||
keyring.set_password(*creds) | ||
keyring.set_password(creds.service_name, creds.username, creds.password) | ||
except Exception: | ||
logger.exception("Failed to save credentials") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why str() round username? You don't account for the possibility of
None
(allowed by the type signature) andstr()
will do nothing if it's a string.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've avoided this by only supporting
get_password
. I think this is the more correct thing to do as this is actually the function which is being called by the CLI.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've moved to mirroring
keyring
's default implementation ofget_credential
which just wrapsget_password