From 5102e2ad085b96944baefe1bdcef7125915068f1 Mon Sep 17 00:00:00 2001 From: Popkornium18 Date: Fri, 26 Jan 2024 17:14:53 +0100 Subject: [PATCH 1/2] refactor PoetryKeyring --- src/poetry/utils/authenticator.py | 14 +-- src/poetry/utils/password_manager.py | 159 ++++++++++++++------------- tests/utils/test_password_manager.py | 34 +++--- 3 files changed, 105 insertions(+), 102 deletions(-) diff --git a/src/poetry/utils/authenticator.py b/src/poetry/utils/authenticator.py index c5e913db630..4a4901540d1 100644 --- a/src/poetry/utils/authenticator.py +++ b/src/poetry/utils/authenticator.py @@ -90,17 +90,15 @@ def get_http_credentials( **(password_manager.get_http_auth(self.name) or {}) ) - if credential.password is None: + if credential.password is not None: + return credential + + if password_manager.use_keyring: # fallback to url and netloc based keyring entries - credential = password_manager.keyring.get_credential( + credential = password_manager.get_credential( self.url, self.netloc, username=credential.username ) - if credential.password is not None: - return HTTPAuthCredential( - username=credential.username, password=credential.password - ) - return credential @@ -305,7 +303,7 @@ def _get_credentials_for_url( if credential.password is None: parsed_url = urllib.parse.urlsplit(url) netloc = parsed_url.netloc - credential = self._password_manager.keyring.get_credential( + credential = self._password_manager.get_credential( url, netloc, username=credential.username ) diff --git a/src/poetry/utils/password_manager.py b/src/poetry/utils/password_manager.py index 166386f4565..4ca2ac8caea 100644 --- a/src/poetry/utils/password_manager.py +++ b/src/poetry/utils/password_manager.py @@ -1,6 +1,7 @@ from __future__ import annotations import dataclasses +import functools import logging from contextlib import suppress @@ -8,6 +9,8 @@ if TYPE_CHECKING: + from keyring.backend import KeyringBackend + from poetry.config.config import Config logger = logging.getLogger(__name__) @@ -30,21 +33,10 @@ class HTTPAuthCredential: class PoetryKeyring: def __init__(self, namespace: str) -> None: self._namespace = namespace - self._is_available = True - - self._check() - - def is_available(self) -> bool: - return self._is_available def get_credential( self, *names: str, username: str | None = None ) -> HTTPAuthCredential: - default = HTTPAuthCredential(username=username, password=None) - - if not self.is_available(): - return default - import keyring from keyring.errors import KeyringError @@ -64,12 +56,9 @@ def get_credential( username=credential.username, password=credential.password ) - return default + return HTTPAuthCredential(username=username, password=None) def get_password(self, name: str, username: str) -> str | None: - if not self.is_available(): - return None - import keyring import keyring.errors @@ -83,9 +72,6 @@ def get_password(self, name: str, username: str) -> str | None: ) def set_password(self, name: str, username: str, password: str) -> None: - if not self.is_available(): - return - import keyring import keyring.errors @@ -99,9 +85,6 @@ def set_password(self, name: str, username: str, password: str) -> None: ) def delete_password(self, name: str, username: str) -> None: - if not self.is_available(): - return - import keyring.errors name = self.get_entry_name(name) @@ -116,69 +99,75 @@ def delete_password(self, name: str, username: str) -> None: def get_entry_name(self, name: str) -> str: return f"{self._namespace}-{name}" - def _check(self) -> None: + @classmethod + def is_available(cls) -> bool: + logger.debug("Checking if keyring is available") try: import keyring + import keyring.backend except ImportError as e: logger.debug("An error occurred while importing keyring: %s", e) - self._is_available = False + return False - return + def backend_name(backend: KeyringBackend) -> str: + name: str = backend.name + return name.split(" ")[0] - backend = keyring.get_keyring() - name = backend.name.split(" ")[0] - if name in ("fail", "null"): - logger.debug("No suitable keyring backend found") - self._is_available = False - elif "plaintext" in backend.name.lower(): - logger.debug("Only a plaintext keyring backend is available. Not using it.") - self._is_available = False - elif name == "chainer": - try: - import keyring.backend + def backend_is_valid(backend: KeyringBackend) -> bool: + name = backend_name(backend) + if name in ("chainer", "fail", "null"): + logger.debug(f"Backend {backend.name!r} is not suitable") + return False + elif "plaintext" in backend.name.lower(): + logger.debug(f"Not using plaintext keyring backend {backend.name!r}") + return False - backends = keyring.backend.get_all_keyring() + return True - self._is_available = any( - b.name.split(" ")[0] not in ["chainer", "fail", "null"] - and "plaintext" not in b.name.lower() - for b in backends - ) - except ImportError: - self._is_available = False + backend = keyring.get_keyring() + if backend_name(backend) == "chainer": + backends = keyring.backend.get_all_keyring() + valid_backend = next((b for b in backends if backend_is_valid(b)), None) + else: + valid_backend = backend if backend_is_valid(backend) else None - if not self._is_available: - logger.debug("No suitable keyring backends were found") + if valid_backend is None: + logger.debug("No valid keyring backend was found") + return False + else: + logger.debug(f"Using keyring backend {backend.name!r}") + return True class PasswordManager: def __init__(self, config: Config) -> None: self._config = config - self._keyring: PoetryKeyring | None = None - @property - def keyring(self) -> PoetryKeyring: - if self._keyring is None: - self._keyring = PoetryKeyring("poetry-repository") + @functools.cached_property + def use_keyring(self) -> bool: + return PoetryKeyring.is_available() - if not self._keyring.is_available(): - logger.debug( - "Keyring is not available, credentials will be stored and " - "retrieved from configuration files as plaintext." - ) + @functools.cached_property + def keyring(self) -> PoetryKeyring: + if not self.use_keyring: + raise PoetryKeyringError( + "Access to keyring was requested, but it is not available" + ) - return self._keyring + return PoetryKeyring("poetry-repository") @staticmethod def warn_plaintext_credentials_stored() -> None: logger.warning("Using a plaintext file to store credentials") - def set_pypi_token(self, name: str, token: str) -> None: - if not self.keyring.is_available(): + def set_pypi_token(self, repo_name: str, token: str) -> None: + if not self.use_keyring: self.warn_plaintext_credentials_stored() - self._config.auth_config_source.add_property(f"pypi-token.{name}", token) + self._config.auth_config_source.add_property( + f"pypi-token.{repo_name}", token + ) else: - self.keyring.set_password(name, "__token__", token) + self.keyring.set_password(repo_name, "__token__", token) def get_pypi_token(self, repo_name: str) -> str | None: """Get PyPi token. @@ -194,41 +183,49 @@ def get_pypi_token(self, repo_name: str) -> str | None: if token: return token - return self.keyring.get_password(repo_name, "__token__") + if self.use_keyring: + return self.keyring.get_password(repo_name, "__token__") + else: + return None - def delete_pypi_token(self, name: str) -> None: - if not self.keyring.is_available(): - return self._config.auth_config_source.remove_property(f"pypi-token.{name}") + def delete_pypi_token(self, repo_name: str) -> None: + if not self.use_keyring: + return self._config.auth_config_source.remove_property( + f"pypi-token.{repo_name}" + ) - self.keyring.delete_password(name, "__token__") + self.keyring.delete_password(repo_name, "__token__") - def get_http_auth(self, name: str) -> dict[str, str | None] | None: - username = self._config.get(f"http-basic.{name}.username") - password = self._config.get(f"http-basic.{name}.password") + def get_http_auth(self, repo_name: str) -> dict[str, str | None] | None: + username = self._config.get(f"http-basic.{repo_name}.username") + password = self._config.get(f"http-basic.{repo_name}.password") if not username and not password: return None if not password: - password = self.keyring.get_password(name, username) + if self.use_keyring: + password = self.keyring.get_password(repo_name, username) + else: + return None return { "username": username, "password": password, } - def set_http_password(self, name: str, username: str, password: str) -> None: + def set_http_password(self, repo_name: str, username: str, password: str) -> None: auth = {"username": username} - if not self.keyring.is_available(): + if not self.use_keyring: self.warn_plaintext_credentials_stored() auth["password"] = password else: - self.keyring.set_password(name, username, password) + self.keyring.set_password(repo_name, username, password) - self._config.auth_config_source.add_property(f"http-basic.{name}", auth) + self._config.auth_config_source.add_property(f"http-basic.{repo_name}", auth) - def delete_http_password(self, name: str) -> None: - auth = self.get_http_auth(name) + def delete_http_password(self, repo_name: str) -> None: + auth = self.get_http_auth(repo_name) if not auth: return @@ -237,6 +234,14 @@ def delete_http_password(self, name: str) -> None: return with suppress(PoetryKeyringError): - self.keyring.delete_password(name, username) + self.keyring.delete_password(repo_name, username) + + self._config.auth_config_source.remove_property(f"http-basic.{repo_name}") - self._config.auth_config_source.remove_property(f"http-basic.{name}") + def get_credential( + self, *names: str, username: str | None = None + ) -> HTTPAuthCredential: + if self.use_keyring: + return self.keyring.get_credential(*names, username=username) + else: + return HTTPAuthCredential(username=username, password=None) diff --git a/tests/utils/test_password_manager.py b/tests/utils/test_password_manager.py index 181ebcc98f0..bf942da233e 100644 --- a/tests/utils/test_password_manager.py +++ b/tests/utils/test_password_manager.py @@ -26,7 +26,7 @@ def test_set_http_password( ) -> None: manager = PasswordManager(config) - assert manager.keyring.is_available() + assert PoetryKeyring.is_available() manager.set_http_password("foo", "bar", "baz") assert dummy_keyring.get_password("poetry-repository-foo", "bar") == "baz" @@ -43,7 +43,7 @@ def test_get_http_auth( config.auth_config_source.add_property("http-basic.foo", {"username": "bar"}) manager = PasswordManager(config) - assert manager.keyring.is_available() + assert PoetryKeyring.is_available() auth = manager.get_http_auth("foo") assert auth is not None @@ -58,7 +58,7 @@ def test_delete_http_password( config.auth_config_source.add_property("http-basic.foo", {"username": "bar"}) manager = PasswordManager(config) - assert manager.keyring.is_available() + assert PoetryKeyring.is_available() manager.delete_http_password("foo") assert dummy_keyring.get_password("poetry-repository-foo", "bar") is None @@ -70,7 +70,7 @@ def test_set_pypi_token( ) -> None: manager = PasswordManager(config) - assert manager.keyring.is_available() + assert PoetryKeyring.is_available() manager.set_pypi_token("foo", "baz") assert config.get("pypi-token.foo") is None @@ -84,7 +84,7 @@ def test_get_pypi_token( dummy_keyring.set_password("poetry-repository-foo", "__token__", "baz") manager = PasswordManager(config) - assert manager.keyring.is_available() + assert PoetryKeyring.is_available() assert manager.get_pypi_token("foo") == "baz" @@ -94,7 +94,7 @@ def test_delete_pypi_token( dummy_keyring.set_password("poetry-repository-foo", "__token__", "baz") manager = PasswordManager(config) - assert manager.keyring.is_available() + assert PoetryKeyring.is_available() manager.delete_pypi_token("foo") assert dummy_keyring.get_password("poetry-repository-foo", "__token__") is None @@ -105,7 +105,7 @@ def test_set_http_password_with_unavailable_backend( ) -> None: manager = PasswordManager(config) - assert not manager.keyring.is_available() + assert not PoetryKeyring.is_available() manager.set_http_password("foo", "bar", "baz") auth = config.get("http-basic.foo") @@ -121,7 +121,7 @@ def test_get_http_auth_with_unavailable_backend( ) manager = PasswordManager(config) - assert not manager.keyring.is_available() + assert not PoetryKeyring.is_available() auth = manager.get_http_auth("foo") assert auth is not None @@ -137,7 +137,7 @@ def test_delete_http_password_with_unavailable_backend( ) manager = PasswordManager(config) - assert not manager.keyring.is_available() + assert not PoetryKeyring.is_available() manager.delete_http_password("foo") assert config.get("http-basic.foo") is None @@ -148,7 +148,7 @@ def test_set_pypi_token_with_unavailable_backend( ) -> None: manager = PasswordManager(config) - assert not manager.keyring.is_available() + assert not PoetryKeyring.is_available() manager.set_pypi_token("foo", "baz") assert config.get("pypi-token.foo") == "baz" @@ -160,7 +160,7 @@ def test_get_pypi_token_with_unavailable_backend( config.auth_config_source.add_property("pypi-token.foo", "baz") manager = PasswordManager(config) - assert not manager.keyring.is_available() + assert not PoetryKeyring.is_available() assert manager.get_pypi_token("foo") == "baz" @@ -170,7 +170,7 @@ def test_delete_pypi_token_with_unavailable_backend( config.auth_config_source.add_property("pypi-token.foo", "baz") manager = PasswordManager(config) - assert not manager.keyring.is_available() + assert not PoetryKeyring.is_available() manager.delete_pypi_token("foo") assert config.get("pypi-token.foo") is None @@ -179,7 +179,7 @@ def test_delete_pypi_token_with_unavailable_backend( def test_keyring_raises_errors_on_keyring_errors( mocker: MockerFixture, with_fail_keyring: None ) -> None: - mocker.patch("poetry.utils.password_manager.PoetryKeyring._check") + mocker.patch("poetry.utils.password_manager.PoetryKeyring.is_available") key_ring = PoetryKeyring("poetry") with pytest.raises(PoetryKeyringError): @@ -281,11 +281,11 @@ def test_get_http_auth_does_not_call_keyring_when_credentials_in_environment_var os.environ["POETRY_HTTP_BASIC_FOO_PASSWORD"] = "baz" manager = PasswordManager(config) - manager._keyring = MagicMock() + manager.keyring = MagicMock() auth = manager.get_http_auth("foo") assert auth == {"username": "bar", "password": "baz"} - manager._keyring.get_password.assert_not_called() + manager.keyring.get_password.assert_not_called() def test_get_http_auth_does_not_call_keyring_when_password_in_environment_variables( @@ -297,11 +297,11 @@ def test_get_http_auth_does_not_call_keyring_when_password_in_environment_variab os.environ["POETRY_HTTP_BASIC_FOO_PASSWORD"] = "baz" manager = PasswordManager(config) - manager._keyring = MagicMock() + manager.keyring = MagicMock() auth = manager.get_http_auth("foo") assert auth == {"username": "bar", "password": "baz"} - manager._keyring.get_password.assert_not_called() + manager.keyring.get_password.assert_not_called() def test_get_pypi_token_with_env_var_positive( From de19d4a81ac569cd96d0bd18c940187b7778fe85 Mon Sep 17 00:00:00 2001 From: Popkornium18 Date: Fri, 26 Jan 2024 17:18:08 +0100 Subject: [PATCH 2/2] add config option keyring.enabled to allow disabling the keyring --- docs/configuration.md | 12 +++++++++ docs/faq.md | 9 +++++++ docs/repositories.md | 6 +++++ src/poetry/config/config.py | 4 +++ src/poetry/console/commands/config.py | 1 + src/poetry/utils/password_manager.py | 2 +- tests/config/test_config.py | 14 ++++++++++ tests/console/commands/test_config.py | 6 +++++ tests/utils/test_password_manager.py | 37 +++++++++++++++++++++++++++ 9 files changed, 90 insertions(+), 1 deletion(-) diff --git a/docs/configuration.md b/docs/configuration.md index 67a0301c3cd..6e2a7fe1923 100644 --- a/docs/configuration.md +++ b/docs/configuration.md @@ -506,3 +506,15 @@ repository. Set client certificate for repository ``. See [Repositories - Configuring credentials - Custom certificate authority]({{< relref "repositories#custom-certificate-authority-and-mutual-tls-authentication" >}}) for more information. + +### `keyring.enabled`: + +**Type**: `boolean` + +**Default**: `true` + +**Environment Variable**: `POETRY_KEYRING_ENABLED` + +Enable the system keyring for storing credentials. +See [Repositories - Configuring credentials]({{< relref "repositories#configuring-credentials" >}}) +for more information. diff --git a/docs/faq.md b/docs/faq.md index 549bc609978..45e7fa2bd30 100644 --- a/docs/faq.md +++ b/docs/faq.md @@ -154,6 +154,15 @@ required variables explicitly or `passenv = "*"` to forward all of them. Linux systems may require forwarding the `DBUS_SESSION_BUS_ADDRESS` variable to allow access to the system keyring, though this may vary between desktop environments. +Alternatively, you can disable the keyring completely: + +```bash +poetry config keyring.enabled false +``` + +Be aware that this will cause Poetry to write passwords to plaintext config files. +You will need to set the credentials again after changing this setting. + ### Is Nox supported? Use the [`nox-poetry`](https://github.com/cjolowicz/nox-poetry) package to install locked versions of diff --git a/docs/repositories.md b/docs/repositories.md index 4e57bff8d10..710cf7e7525 100644 --- a/docs/repositories.md +++ b/docs/repositories.md @@ -500,6 +500,12 @@ If a system keyring is available and supported, the password is stored to and re Keyring support is enabled using the [keyring library](https://pypi.org/project/keyring/). For more information on supported backends refer to the [library documentation](https://keyring.readthedocs.io/en/latest/?badge=latest). +If you do not want to use the keyring, you can tell Poetry to disable it and store the credentials in plaintext config files: + +```bash +poetry config keyring.enabled false +``` + {{% note %}} Poetry will fallback to Pip style use of keyring so that backends like diff --git a/src/poetry/config/config.py b/src/poetry/config/config.py index 5bafb90218b..42d6d088bc2 100644 --- a/src/poetry/config/config.py +++ b/src/poetry/config/config.py @@ -137,6 +137,9 @@ class Config: "warnings": { "export": True, }, + "keyring": { + "enabled": True, + }, } def __init__( @@ -301,6 +304,7 @@ def _get_normalizer(name: str) -> Callable[[str], Any]: "installer.parallel", "solver.lazy-wheel", "warnings.export", + "keyring.enabled", }: return boolean_normalizer diff --git a/src/poetry/console/commands/config.py b/src/poetry/console/commands/config.py index 13e8b2262a0..3d0162e12c0 100644 --- a/src/poetry/console/commands/config.py +++ b/src/poetry/console/commands/config.py @@ -79,6 +79,7 @@ def unique_config_values(self) -> dict[str, tuple[Any, Any]]: ), "solver.lazy-wheel": (boolean_validator, boolean_normalizer), "warnings.export": (boolean_validator, boolean_normalizer), + "keyring.enabled": (boolean_validator, boolean_normalizer), } return unique_config_values diff --git a/src/poetry/utils/password_manager.py b/src/poetry/utils/password_manager.py index 4ca2ac8caea..f4ab1232694 100644 --- a/src/poetry/utils/password_manager.py +++ b/src/poetry/utils/password_manager.py @@ -145,7 +145,7 @@ def __init__(self, config: Config) -> None: @functools.cached_property def use_keyring(self) -> bool: - return PoetryKeyring.is_available() + return self._config.get("keyring.enabled") and PoetryKeyring.is_available() @functools.cached_property def keyring(self) -> PoetryKeyring: diff --git a/tests/config/test_config.py b/tests/config/test_config.py index 35c2a16753e..7241b622e0a 100644 --- a/tests/config/test_config.py +++ b/tests/config/test_config.py @@ -12,6 +12,7 @@ from poetry.config.config import Config from poetry.config.config import boolean_normalizer from poetry.config.config import int_normalizer +from poetry.utils.password_manager import PasswordManager from tests.helpers import flatten_dict @@ -19,6 +20,8 @@ from collections.abc import Callable from collections.abc import Iterator + from tests.conftest import DummyBackend + Normalizer = Callable[[str], Any] @@ -81,3 +84,14 @@ def test_config_expands_tilde_for_virtualenvs_path( ) -> None: config.merge({"virtualenvs": {"path": path_config}}) assert config.virtualenvs_path == expected + + +def test_disabled_keyring_is_unavailable( + config: Config, with_simple_keyring: None, dummy_keyring: DummyBackend +) -> None: + manager = PasswordManager(config) + assert manager.use_keyring + + config.config["keyring"]["enabled"] = False + manager = PasswordManager(config) + assert not manager.use_keyring diff --git a/tests/console/commands/test_config.py b/tests/console/commands/test_config.py index ee54963ac1d..d96077cc307 100644 --- a/tests/console/commands/test_config.py +++ b/tests/console/commands/test_config.py @@ -59,6 +59,7 @@ def test_list_displays_default_value_if_not_set( installer.modern-installation = true installer.no-binary = null installer.parallel = true +keyring.enabled = true solver.lazy-wheel = true virtualenvs.create = true virtualenvs.in-project = null @@ -90,6 +91,7 @@ def test_list_displays_set_get_setting( installer.modern-installation = true installer.no-binary = null installer.parallel = true +keyring.enabled = true solver.lazy-wheel = true virtualenvs.create = false virtualenvs.in-project = null @@ -142,6 +144,7 @@ def test_unset_setting( installer.modern-installation = true installer.no-binary = null installer.parallel = true +keyring.enabled = true solver.lazy-wheel = true virtualenvs.create = true virtualenvs.in-project = null @@ -172,6 +175,7 @@ def test_unset_repo_setting( installer.modern-installation = true installer.no-binary = null installer.parallel = true +keyring.enabled = true solver.lazy-wheel = true virtualenvs.create = true virtualenvs.in-project = null @@ -300,6 +304,7 @@ def test_list_displays_set_get_local_setting( installer.modern-installation = true installer.no-binary = null installer.parallel = true +keyring.enabled = true solver.lazy-wheel = true virtualenvs.create = false virtualenvs.in-project = null @@ -338,6 +343,7 @@ def test_list_must_not_display_sources_from_pyproject_toml( installer.modern-installation = true installer.no-binary = null installer.parallel = true +keyring.enabled = true repositories.foo.url = "https://foo.bar/simple/" solver.lazy-wheel = true virtualenvs.create = true diff --git a/tests/utils/test_password_manager.py b/tests/utils/test_password_manager.py index bf942da233e..7c75e7370b3 100644 --- a/tests/utils/test_password_manager.py +++ b/tests/utils/test_password_manager.py @@ -330,3 +330,40 @@ def test_get_pypi_token_with_env_var_not_available( result_token = manager.get_pypi_token(repo_name) assert result_token is None + + +def test_disabled_keyring_never_called( + config: Config, with_simple_keyring: None, dummy_keyring: DummyBackend +) -> None: + config.config["keyring"]["enabled"] = False + config.config["http-basic"] = {"onlyuser": {"username": "user"}} + + manager = PasswordManager(config) + num_public_functions = len([f for f in dir(manager) if not f.startswith("_")]) + if num_public_functions != 10: + pytest.fail( + f"A function was added to or removed from the {PasswordManager.__name__} " + "class without reflecting this change in this test." + ) + + with pytest.raises(PoetryKeyringError) as e: + _ = manager.keyring + + assert str(e.value) == "Access to keyring was requested, but it is not available" + + # We made sure that accessing a disabled keyring raises an exception. + # Now we call the PasswordManager functions that do access the keyring to + # make sure that they never do so when the keyring is disabled. + manager.set_pypi_token(repo_name="exists", token="token") + manager.get_pypi_token(repo_name="exists") + manager.get_pypi_token(repo_name="doesn't exist") + manager.delete_pypi_token(repo_name="exists") + manager.delete_pypi_token(repo_name="doesn't exist") + manager.set_http_password(repo_name="exists", username="user", password="password") + manager.get_http_auth(repo_name="exists") + manager.get_http_auth(repo_name="doesn't exist") + manager.get_http_auth(repo_name="onlyuser") + manager.delete_http_password(repo_name="exits") + manager.delete_http_password(repo_name="doesn't exist") + manager.delete_http_password(repo_name="onlyuser") + manager.get_credential("a", "b", "c", username="user")