From 6a495902497dc02bcf42c5d7f4088d130b39f5d2 Mon Sep 17 00:00:00 2001 From: Arun Babu Neelicattu Date: Tue, 21 Jan 2025 15:29:25 +0100 Subject: [PATCH] feat(auth): allow passwords with empty usernames This change largely reverts the decision made in b544ed5 as it seems that it is commonplace, albeit not best practice, for private indices to require or document that users use an empty username. The change in behaviour seems to surprise users migrating from 1.x (#10085). --- docs/repositories.md | 5 ++++- src/poetry/utils/password_manager.py | 20 +++++++++++++------- tests/conftest.py | 6 ++++++ tests/utils/test_authenticator.py | 6 ++++-- tests/utils/test_password_manager.py | 19 +++++++++---------- 5 files changed, 36 insertions(+), 20 deletions(-) diff --git a/docs/repositories.md b/docs/repositories.md index 59d0cf9c981..80ef3ebdb6a 100644 --- a/docs/repositories.md +++ b/docs/repositories.md @@ -531,7 +531,10 @@ required to set an empty password. This is supported by Poetry. poetry config http-basic.foo "" ``` -**Note:** Usernames cannot be empty. Attempting to use an empty username can result in an unpredictable failure. +**Note:** Empty usernames are discouraged. However, Poetry will honour them if a password is configured without it. This +is unfortunately commonplace practice, while not best practice, for private indices that use tokens. When a password is +stored into the system keyring with an empty username, Poetry will use a literal `__poetry_source_empty_username__` as +the username to circumvent [keyring#687](https://github.com/jaraco/keyring/pull/687). {{% /note %}} ## Certificates diff --git a/src/poetry/utils/password_manager.py b/src/poetry/utils/password_manager.py index fd218524248..f576945d888 100644 --- a/src/poetry/utils/password_manager.py +++ b/src/poetry/utils/password_manager.py @@ -27,6 +27,13 @@ class HTTPAuthCredential: class PoetryKeyring: + # some private sources expect tokens to be provided as password with that can be empty + # we use a fixed literal to ensure that this can be stored in keyring (jaraco/keyring#687) + # + # Note: If this is changed, users with passwords stored with empty usernames will have to + # re-add the config. + _EMPTY_USERNAME_KEY = "__poetry_source_empty_username__" + def __init__(self, namespace: str) -> None: self._namespace = namespace @@ -41,6 +48,7 @@ def get_credential( for name in names: credential = None try: + # we do default to empty username string here since credentials support empty usernames credential = keyring.get_credential(name, username) except KeyringLocked: logger.debug("Keyring %s is locked", name) @@ -61,7 +69,7 @@ def get_password(self, name: str, username: str) -> str | None: name = self.get_entry_name(name) try: - return keyring.get_password(name, username) + return keyring.get_password(name, username or self._EMPTY_USERNAME_KEY) except (RuntimeError, keyring.errors.KeyringError): raise PoetryKeyringError( f"Unable to retrieve the password for {name} from the key ring" @@ -74,7 +82,7 @@ def set_password(self, name: str, username: str, password: str) -> None: name = self.get_entry_name(name) try: - keyring.set_password(name, username, password) + keyring.set_password(name, username or self._EMPTY_USERNAME_KEY, password) except (RuntimeError, keyring.errors.KeyringError) as e: raise PoetryKeyringError( f"Unable to store the password for {name} in the key ring: {e}" @@ -86,7 +94,7 @@ def delete_password(self, name: str, username: str) -> None: name = self.get_entry_name(name) try: - keyring.delete_password(name, username) + keyring.delete_password(name, username or self._EMPTY_USERNAME_KEY) except (RuntimeError, keyring.errors.KeyringError): raise PoetryKeyringError( f"Unable to delete the password for {name} from the key ring" @@ -196,13 +204,11 @@ def get_http_auth(self, repo_name: str) -> HTTPAuthCredential: username = self._config.get(f"http-basic.{repo_name}.username") password = self._config.get(f"http-basic.{repo_name}.password") - if not username: - return HTTPAuthCredential() - if password is None and self.use_keyring: password = self.keyring.get_password(repo_name, username) - return HTTPAuthCredential(username=username, password=password) + # we use `or None` here to ensure that empty strings are passed as None + return HTTPAuthCredential(username=username or None, password=password or None) def set_http_password(self, repo_name: str, username: str, password: str) -> None: auth = {"username": username} diff --git a/tests/conftest.py b/tests/conftest.py index d69dc6bd12d..6d4f1befee1 100644 --- a/tests/conftest.py +++ b/tests/conftest.py @@ -35,6 +35,7 @@ from poetry.utils.env import EnvManager from poetry.utils.env import SystemEnv from poetry.utils.env import VirtualEnv +from poetry.utils.password_manager import PoetryKeyring from tests.helpers import MOCK_DEFAULT_GIT_REVISION from tests.helpers import TestLocker from tests.helpers import TestRepository @@ -191,6 +192,11 @@ def get_credential( raise KeyringError() +@pytest.fixture() +def poetry_keyring() -> PoetryKeyring: + return PoetryKeyring("poetry-repository") + + @pytest.fixture() def dummy_keyring() -> DummyBackend: return DummyBackend() diff --git a/tests/utils/test_authenticator.py b/tests/utils/test_authenticator.py index 12d91ab62e7..99ea2a50772 100644 --- a/tests/utils/test_authenticator.py +++ b/tests/utils/test_authenticator.py @@ -25,6 +25,7 @@ from pytest import MonkeyPatch from pytest_mock import MockerFixture + from poetry.utils.password_manager import PoetryKeyring from tests.conftest import Config from tests.conftest import DummyBackend @@ -153,7 +154,7 @@ def test_authenticator_uses_empty_strings_as_default_password( assert request.headers["Authorization"] == f"Basic {basic_auth}" -def test_authenticator_ignores_empty_strings_as_default_username( +def test_authenticator_does_not_ignore_empty_strings_as_default_username( config: Config, mock_remote: None, repo: dict[str, dict[str, str]], @@ -170,7 +171,7 @@ def test_authenticator_ignores_empty_strings_as_default_username( authenticator.request("get", "https://foo.bar/files/foo-0.1.0.tar.gz") request = http.last_request() - assert request.headers["Authorization"] is None + assert request.headers["Authorization"] == "Basic OmJhcg==" def test_authenticator_falls_back_to_keyring_url( @@ -207,6 +208,7 @@ def test_authenticator_falls_back_to_keyring_netloc( http: type[httpretty.httpretty], with_simple_keyring: None, dummy_keyring: DummyBackend, + poetry_keyring: PoetryKeyring, ) -> None: config.merge( { diff --git a/tests/utils/test_password_manager.py b/tests/utils/test_password_manager.py index fbffd6fc440..c7be0fb77db 100644 --- a/tests/utils/test_password_manager.py +++ b/tests/utils/test_password_manager.py @@ -1,6 +1,5 @@ from __future__ import annotations -import contextlib import logging import os @@ -42,7 +41,7 @@ def test_set_http_password( ("username", "password", "is_valid"), [ ("bar", "baz", True), - ("", "baz", False), + ("", "baz", True), ("bar", "", True), ("", "", False), ], @@ -53,10 +52,10 @@ def test_get_http_auth( is_valid: bool, config: Config, with_simple_keyring: None, - dummy_keyring: DummyBackend, + poetry_keyring: PoetryKeyring, ) -> None: - with contextlib.nullcontext() if username else pytest.warns(DeprecationWarning): - dummy_keyring.set_password("poetry-repository-foo", username, password) + poetry_keyring.set_password("foo", username, password) + config.auth_config_source.add_property("http-basic.foo", {"username": username}) manager = PasswordManager(config) @@ -65,8 +64,8 @@ def test_get_http_auth( if is_valid: assert auth is not None - assert auth.username == username - assert auth.password == password + assert auth.username == (username or None) + assert auth.password == (password or None) else: assert auth.username is auth.password is None @@ -137,7 +136,7 @@ def test_set_http_password_with_unavailable_backend( ("username", "password", "is_valid"), [ ("bar", "baz", True), - ("", "baz", False), + ("", "baz", True), ("bar", "", True), ("", "", False), ], @@ -159,8 +158,8 @@ def test_get_http_auth_with_unavailable_backend( if is_valid: assert auth is not None - assert auth.username == username - assert auth.password == password + assert auth.username == (username or None) + assert auth.password == (password or None) else: assert auth.username is auth.password is None