From 6a1b5506aa9b0ac26c87ff4996a3a752df38e1f0 Mon Sep 17 00:00:00 2001 From: Matthieu Bizien Date: Fri, 23 Sep 2022 19:24:52 +0200 Subject: [PATCH 1/6] fix: remove exception when keyring is locked #1917 --- src/poetry/utils/password_manager.py | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/src/poetry/utils/password_manager.py b/src/poetry/utils/password_manager.py index 620556043c0..abba9b01235 100644 --- a/src/poetry/utils/password_manager.py +++ b/src/poetry/utils/password_manager.py @@ -48,7 +48,11 @@ def get_credential( import keyring for name in names: - credential = keyring.get_credential(name, username) + try: + credential = keyring.get_credential(name, username) + except keyring.errors.KeyringLocked: + logger.debug("Keyring %s is locked", name) + credential = None if credential: return HTTPAuthCredential( username=credential.username, password=credential.password From b69737fc37949eace90dc4ca262ca3aa7f408cf5 Mon Sep 17 00:00:00 2001 From: real-yfprojects Date: Mon, 24 Jul 2023 08:06:06 +0200 Subject: [PATCH 2/6] Catch `KeyringError` and `RuntimeError` when accessing keyring. * src/poetry/utils/password_manager.py (PoetryKeyring.get_credential) --- src/poetry/utils/password_manager.py | 10 ++++++++-- 1 file changed, 8 insertions(+), 2 deletions(-) diff --git a/src/poetry/utils/password_manager.py b/src/poetry/utils/password_manager.py index abba9b01235..166386f4565 100644 --- a/src/poetry/utils/password_manager.py +++ b/src/poetry/utils/password_manager.py @@ -47,12 +47,18 @@ def get_credential( import keyring + from keyring.errors import KeyringError + from keyring.errors import KeyringLocked + for name in names: + credential = None try: credential = keyring.get_credential(name, username) - except keyring.errors.KeyringLocked: + except KeyringLocked: logger.debug("Keyring %s is locked", name) - credential = None + except (KeyringError, RuntimeError): + logger.debug("Accessing keyring %s failed", name, exc_info=True) + if credential: return HTTPAuthCredential( username=credential.username, password=credential.password From fc81f0d35063f16f4203ccbdcf2c73b946342417 Mon Sep 17 00:00:00 2001 From: real-yfprojects <62463991+real-yfprojects@users.noreply.github.com> Date: Sun, 13 Aug 2023 18:43:16 +0200 Subject: [PATCH 3/6] Test locked keyring * tests/conftest.py : Implement `LockedBackend` inheriting `KeyringBackend` and throwing `KeyringLocked` on access. * tests\utils\test_authenticator.py : Test that locked keyrings still lead to a request. * tests\utils\test_password_manager.py : Test that locked keyrings do not fail. --- tests/conftest.py | 37 +++++++++++++++++++--------- tests/utils/test_authenticator.py | 13 ++++++++++ tests/utils/test_password_manager.py | 13 ++++++++++ 3 files changed, 51 insertions(+), 12 deletions(-) diff --git a/tests/conftest.py b/tests/conftest.py index 8733082ca75..0fcb1b1e055 100644 --- a/tests/conftest.py +++ b/tests/conftest.py @@ -12,9 +12,11 @@ from typing import Any import httpretty +import keyring import pytest from keyring.backend import KeyringBackend +from keyring.errors import KeyringLocked from poetry.config.config import Config as BaseConfig from poetry.config.dict_config_source import DictConfigSource @@ -113,6 +115,24 @@ def delete_password(self, service: str, username: str | None) -> None: del self._passwords[service][username] +class LockedBackend(KeyringBackend): + @classmethod + def priority(cls) -> int: + return 42 + + def set_password(self, service: str, username: str | None, password: Any) -> None: + raise KeyringLocked() + + def get_password(self, service: str, username: str | None) -> Any: + raise KeyringLocked() + + def get_credential(self, service: str, username: str | None) -> Any: + raise KeyringLocked() + + def delete_password(self, service: str, username: str | None) -> None: + raise KeyringLocked() + + @pytest.fixture() def dummy_keyring() -> DummyBackend: return DummyBackend() @@ -120,24 +140,23 @@ def dummy_keyring() -> DummyBackend: @pytest.fixture() def with_simple_keyring(dummy_keyring: DummyBackend) -> None: - import keyring - keyring.set_keyring(dummy_keyring) @pytest.fixture() def with_fail_keyring() -> None: - import keyring - from keyring.backends.fail import Keyring keyring.set_keyring(Keyring()) # type: ignore[no-untyped-call] @pytest.fixture() -def with_null_keyring() -> None: - import keyring +def with_locked_keyring() -> None: + keyring.set_keyring(LockedBackend()) # type: ignore[no-untyped-call] + +@pytest.fixture() +def with_null_keyring() -> None: from keyring.backends.null import Keyring keyring.set_keyring(Keyring()) # type: ignore[no-untyped-call] @@ -151,8 +170,6 @@ def with_chained_fail_keyring(mocker: MockerFixture) -> None: "keyring.backend.get_all_keyring", lambda: [Keyring()], # type: ignore[no-untyped-call] ) - import keyring - from keyring.backends.chainer import ChainerBackend keyring.set_keyring(ChainerBackend()) # type: ignore[no-untyped-call] @@ -166,8 +183,6 @@ def with_chained_null_keyring(mocker: MockerFixture) -> None: "keyring.backend.get_all_keyring", lambda: [Keyring()], # type: ignore[no-untyped-call] ) - import keyring - from keyring.backends.chainer import ChainerBackend keyring.set_keyring(ChainerBackend()) # type: ignore[no-untyped-call] @@ -206,8 +221,6 @@ def config( auth_config_source: DictConfigSource, mocker: MockerFixture, ) -> Config: - import keyring - from keyring.backends.fail import Keyring keyring.set_keyring(Keyring()) # type: ignore[no-untyped-call] diff --git a/tests/utils/test_authenticator.py b/tests/utils/test_authenticator.py index 2a0d1bee564..08dbbb78d47 100644 --- a/tests/utils/test_authenticator.py +++ b/tests/utils/test_authenticator.py @@ -94,6 +94,19 @@ def test_authenticator_uses_username_only_credentials( assert request.headers["Authorization"] == "Basic Zm9vMDAxOg==" +def test_authenticator_ignores_locked_keyring( + mock_config: Config, + mock_remote: None, + http: type[httpretty.httpretty], + with_locked_keyring: None, +) -> None: + authenticator = Authenticator(mock_config, NullIO()) + authenticator.request("get", "https://foo001@foo.bar/files/foo-0.1.0.tar.gz") + request = http.last_request() + + assert request.headers["Authorization"] == "Basic Zm9vMDAxOg==" + + def test_authenticator_uses_password_only_credentials( mock_config: Config, mock_remote: None, http: type[httpretty.httpretty] ) -> None: diff --git a/tests/utils/test_password_manager.py b/tests/utils/test_password_manager.py index f2cbb7b7bce..c03470291b1 100644 --- a/tests/utils/test_password_manager.py +++ b/tests/utils/test_password_manager.py @@ -190,6 +190,13 @@ def test_keyring_raises_errors_on_keyring_errors( key_ring.delete_password("foo", "bar") +def test_keyring_returns_none_on_locked_keyring(with_locked_keyring: None) -> None: + key_ring = PoetryKeyring("poetry") + + cred = key_ring.get_credential("any password", "any name") + assert cred.password is None + + def test_keyring_with_chainer_backend_and_fail_keyring_should_be_unavailable( with_chained_fail_keyring: None, ) -> None: @@ -222,6 +229,12 @@ def test_fail_keyring_should_be_unavailable( assert not key_ring.is_available() +def test_locked_keyring_should_be_available(with_locked_keyring: None) -> None: + key_ring = PoetryKeyring("poetry") + + assert key_ring.is_available() + + def test_get_http_auth_from_environment_variables( environ: None, config: Config ) -> None: From 6292400657967deb0fc3e01bc514528dc014e125 Mon Sep 17 00:00:00 2001 From: real-yfprojects <62463991+real-yfprojects@users.noreply.github.com> Date: Sun, 3 Sep 2023 10:41:31 +0200 Subject: [PATCH 4/6] WIP erroneous backend tests --- tests/conftest.py | 18 ++++++++++++++++++ tests/utils/test_password_manager.py | 6 ++++++ 2 files changed, 24 insertions(+) diff --git a/tests/conftest.py b/tests/conftest.py index 0fcb1b1e055..4692d25f1bf 100644 --- a/tests/conftest.py +++ b/tests/conftest.py @@ -133,6 +133,19 @@ def delete_password(self, service: str, username: str | None) -> None: raise KeyringLocked() +from keyring import backends +from keyring.errors import KeyringError + + +class ErroneousBackend(backends.fail.Keyring): + @classmethod + def priority(cls) -> int: + return 42 + + def get_credential(self, service: str, username: str | None) -> Any: + raise KeyringError() + + @pytest.fixture() def dummy_keyring() -> DummyBackend: return DummyBackend() @@ -155,6 +168,11 @@ def with_locked_keyring() -> None: keyring.set_keyring(LockedBackend()) # type: ignore[no-untyped-call] +@pytest.fixture() +def with_erroneous_keyring() -> None: + keyring.set_keyring(ErroneousBackend()) # type: ignore[no-untyped-call] + + @pytest.fixture() def with_null_keyring() -> None: from keyring.backends.null import Keyring diff --git a/tests/utils/test_password_manager.py b/tests/utils/test_password_manager.py index c03470291b1..ac634000a03 100644 --- a/tests/utils/test_password_manager.py +++ b/tests/utils/test_password_manager.py @@ -235,6 +235,12 @@ def test_locked_keyring_should_be_available(with_locked_keyring: None) -> None: assert key_ring.is_available() +def test_erroneous_keyring_should_be_available(with_erroneous_keyring: None) -> None: + key_ring = PoetryKeyring("poetry") + + assert key_ring.is_available() + + def test_get_http_auth_from_environment_variables( environ: None, config: Config ) -> None: From 5e00b1810be5047539571088b39f0d9b312ea8d8 Mon Sep 17 00:00:00 2001 From: real-yfprojects Date: Sun, 3 Sep 2023 10:57:35 +0200 Subject: [PATCH 5/6] Test a keyring raising `KeyringError` on access. * tests/utils/test_authenticator.py * tests/conftest.py --- tests/conftest.py | 6 ++---- tests/utils/test_authenticator.py | 13 +++++++++++++ 2 files changed, 15 insertions(+), 4 deletions(-) diff --git a/tests/conftest.py b/tests/conftest.py index 4692d25f1bf..10c5617d1a6 100644 --- a/tests/conftest.py +++ b/tests/conftest.py @@ -15,7 +15,9 @@ import keyring import pytest +from keyring import backends from keyring.backend import KeyringBackend +from keyring.errors import KeyringError from keyring.errors import KeyringLocked from poetry.config.config import Config as BaseConfig @@ -133,10 +135,6 @@ def delete_password(self, service: str, username: str | None) -> None: raise KeyringLocked() -from keyring import backends -from keyring.errors import KeyringError - - class ErroneousBackend(backends.fail.Keyring): @classmethod def priority(cls) -> int: diff --git a/tests/utils/test_authenticator.py b/tests/utils/test_authenticator.py index 08dbbb78d47..cda447fa479 100644 --- a/tests/utils/test_authenticator.py +++ b/tests/utils/test_authenticator.py @@ -107,6 +107,19 @@ def test_authenticator_ignores_locked_keyring( assert request.headers["Authorization"] == "Basic Zm9vMDAxOg==" +def test_authenticator_ignores_failing_keyring( + mock_config: Config, + mock_remote: None, + http: type[httpretty.httpretty], + with_erroneous_keyring: None, +) -> None: + authenticator = Authenticator(mock_config, NullIO()) + authenticator.request("get", "https://foo001@foo.bar/files/foo-0.1.0.tar.gz") + request = http.last_request() + + assert request.headers["Authorization"] == "Basic Zm9vMDAxOg==" + + def test_authenticator_uses_password_only_credentials( mock_config: Config, mock_remote: None, http: type[httpretty.httpretty] ) -> None: From aea09575eadbf1dc9c104e088ae3b23670614c71 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Randy=20D=C3=B6ring?= <30527984+radoering@users.noreply.github.com> Date: Tue, 3 Oct 2023 17:08:16 +0200 Subject: [PATCH 6/6] improve tests --- tests/utils/test_authenticator.py | 61 ++++++++++++++++------------ tests/utils/test_password_manager.py | 25 +++++++++++- 2 files changed, 57 insertions(+), 29 deletions(-) diff --git a/tests/utils/test_authenticator.py b/tests/utils/test_authenticator.py index cda447fa479..485982f8ca9 100644 --- a/tests/utils/test_authenticator.py +++ b/tests/utils/test_authenticator.py @@ -1,6 +1,7 @@ from __future__ import annotations import base64 +import logging import re import uuid @@ -20,6 +21,7 @@ if TYPE_CHECKING: + from _pytest.logging import LogCaptureFixture from _pytest.monkeypatch import MonkeyPatch from pytest_mock import MockerFixture @@ -65,8 +67,8 @@ def test_authenticator_uses_url_provided_credentials( authenticator.request("get", "https://foo001:bar002@foo.bar/files/foo-0.1.0.tar.gz") request = http.last_request() - - assert request.headers["Authorization"] == "Basic Zm9vMDAxOmJhcjAwMg==" + basic_auth = base64.b64encode(b"foo001:bar002").decode() + assert request.headers["Authorization"] == f"Basic {basic_auth}" def test_authenticator_uses_credentials_from_config_if_not_provided( @@ -76,8 +78,8 @@ def test_authenticator_uses_credentials_from_config_if_not_provided( authenticator.request("get", "https://foo.bar/files/foo-0.1.0.tar.gz") request = http.last_request() - - assert request.headers["Authorization"] == "Basic YmFyOmJheg==" + basic_auth = base64.b64encode(b"bar:baz").decode() + assert request.headers["Authorization"] == f"Basic {basic_auth}" def test_authenticator_uses_username_only_credentials( @@ -90,34 +92,38 @@ def test_authenticator_uses_username_only_credentials( authenticator.request("get", "https://foo001@foo.bar/files/foo-0.1.0.tar.gz") request = http.last_request() - - assert request.headers["Authorization"] == "Basic Zm9vMDAxOg==" + basic_auth = base64.b64encode(b"foo001:").decode() + assert request.headers["Authorization"] == f"Basic {basic_auth}" def test_authenticator_ignores_locked_keyring( - mock_config: Config, mock_remote: None, http: type[httpretty.httpretty], with_locked_keyring: None, + caplog: LogCaptureFixture, ) -> None: - authenticator = Authenticator(mock_config, NullIO()) - authenticator.request("get", "https://foo001@foo.bar/files/foo-0.1.0.tar.gz") - request = http.last_request() + caplog.set_level(logging.DEBUG, logger="poetry.utils.password_manager") + authenticator = Authenticator() + authenticator.request("get", "https://foo.bar/files/foo-0.1.0.tar.gz") - assert request.headers["Authorization"] == "Basic Zm9vMDAxOg==" + request = http.last_request() + assert request.headers["Authorization"] is None + assert "Keyring foo.bar is locked" in caplog.messages def test_authenticator_ignores_failing_keyring( - mock_config: Config, mock_remote: None, http: type[httpretty.httpretty], with_erroneous_keyring: None, + caplog: LogCaptureFixture, ) -> None: - authenticator = Authenticator(mock_config, NullIO()) - authenticator.request("get", "https://foo001@foo.bar/files/foo-0.1.0.tar.gz") - request = http.last_request() + caplog.set_level(logging.DEBUG, logger="poetry.utils.password_manager") + authenticator = Authenticator() + authenticator.request("get", "https://foo.bar/files/foo-0.1.0.tar.gz") - assert request.headers["Authorization"] == "Basic Zm9vMDAxOg==" + request = http.last_request() + assert request.headers["Authorization"] is None + assert "Accessing keyring foo.bar failed" in caplog.messages def test_authenticator_uses_password_only_credentials( @@ -127,8 +133,8 @@ def test_authenticator_uses_password_only_credentials( authenticator.request("get", "https://:bar002@foo.bar/files/foo-0.1.0.tar.gz") request = http.last_request() - - assert request.headers["Authorization"] == "Basic OmJhcjAwMg==" + basic_auth = base64.b64encode(b":bar002").decode() + assert request.headers["Authorization"] == f"Basic {basic_auth}" def test_authenticator_uses_empty_strings_as_default_password( @@ -149,8 +155,8 @@ def test_authenticator_uses_empty_strings_as_default_password( authenticator.request("get", "https://foo.bar/files/foo-0.1.0.tar.gz") request = http.last_request() - - assert request.headers["Authorization"] == "Basic YmFyOg==" + basic_auth = base64.b64encode(b"bar:").decode() + assert request.headers["Authorization"] == f"Basic {basic_auth}" def test_authenticator_uses_empty_strings_as_default_username( @@ -170,8 +176,8 @@ def test_authenticator_uses_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"] == "Basic OmJhcg==" + basic_auth = base64.b64encode(b":bar").decode() + assert request.headers["Authorization"] == f"Basic {basic_auth}" def test_authenticator_falls_back_to_keyring_url( @@ -196,8 +202,8 @@ def test_authenticator_falls_back_to_keyring_url( authenticator.request("get", "https://foo.bar/files/foo-0.1.0.tar.gz") request = http.last_request() - - assert request.headers["Authorization"] == "Basic Zm9vOmJhcg==" + basic_auth = base64.b64encode(b"foo:bar").decode() + assert request.headers["Authorization"] == f"Basic {basic_auth}" def test_authenticator_falls_back_to_keyring_netloc( @@ -220,8 +226,8 @@ def test_authenticator_falls_back_to_keyring_netloc( authenticator.request("get", "https://foo.bar/files/foo-0.1.0.tar.gz") request = http.last_request() - - assert request.headers["Authorization"] == "Basic Zm9vOmJhcg==" + basic_auth = base64.b64encode(b"foo:bar").decode() + assert request.headers["Authorization"] == f"Basic {basic_auth}" @pytest.mark.filterwarnings("ignore::pytest.PytestUnhandledThreadExceptionWarning") @@ -356,7 +362,8 @@ def test_authenticator_uses_env_provided_credentials( request = http.last_request() - assert request.headers["Authorization"] == "Basic YmFyOmJheg==" + basic_auth = base64.b64encode(b"bar:baz").decode() + assert request.headers["Authorization"] == f"Basic {basic_auth}" @pytest.mark.parametrize( diff --git a/tests/utils/test_password_manager.py b/tests/utils/test_password_manager.py index ac634000a03..5efcf7f721b 100644 --- a/tests/utils/test_password_manager.py +++ b/tests/utils/test_password_manager.py @@ -1,5 +1,6 @@ from __future__ import annotations +import logging import os from typing import TYPE_CHECKING @@ -13,6 +14,7 @@ if TYPE_CHECKING: + from _pytest.logging import LogCaptureFixture from pytest_mock import MockerFixture from tests.conftest import Config @@ -190,11 +192,30 @@ def test_keyring_raises_errors_on_keyring_errors( key_ring.delete_password("foo", "bar") -def test_keyring_returns_none_on_locked_keyring(with_locked_keyring: None) -> None: +def test_keyring_returns_none_on_locked_keyring( + with_locked_keyring: None, + caplog: LogCaptureFixture, +) -> None: + caplog.set_level(logging.DEBUG, logger="poetry.utils.password_manager") key_ring = PoetryKeyring("poetry") - cred = key_ring.get_credential("any password", "any name") + cred = key_ring.get_credential("foo") + + assert cred.password is None + assert "Keyring foo is locked" in caplog.messages + + +def test_keyring_returns_none_on_erroneous_keyring( + with_erroneous_keyring: None, + caplog: LogCaptureFixture, +) -> None: + caplog.set_level(logging.DEBUG, logger="poetry.utils.password_manager") + key_ring = PoetryKeyring("poetry") + + cred = key_ring.get_credential("foo") + assert cred.password is None + assert "Accessing keyring foo failed" in caplog.messages def test_keyring_with_chainer_backend_and_fail_keyring_should_be_unavailable(