From 3d4163c1057617cad67c536fef52d297b4c4dbc3 Mon Sep 17 00:00:00 2001 From: Federico D'Ambrosio Date: Thu, 14 Apr 2022 14:39:49 +0200 Subject: [PATCH 01/17] Use logger.exception instead of logger.warning to handle keyring failures Add related test --- tests/test_auth.py | 17 +++++++++++++++++ twine/auth.py | 4 ++-- 2 files changed, 19 insertions(+), 2 deletions(-) diff --git a/tests/test_auth.py b/tests/test_auth.py index d9dde51b..566210cc 100644 --- a/tests/test_auth.py +++ b/tests/test_auth.py @@ -227,3 +227,20 @@ def test_warns_for_empty_password( assert auth.Resolver(config, auth.CredentialInput()).password == password assert caplog.messages[0].startswith(warning) + + +def test_log_exception_on_keyring_failure(config, monkeypatch, caplog): + class FailKeyring: + @staticmethod + def get_credential(system, username): + import os + + # Let's simulate the error that occurred + # in https://github.com/pypa/twine/issues/889 + os.environ["HOME"] + + monkeypatch.setattr(auth, "keyring", FailKeyring()) + + assert not auth.Resolver(config, auth.CredentialInput()).get_username_from_keyring() + + assert "KeyError: 'HOME'" in caplog.text and caplog.messages[0] == "'HOME'" diff --git a/twine/auth.py b/twine/auth.py index a873bf7c..57afff5f 100644 --- a/twine/auth.py +++ b/twine/auth.py @@ -63,7 +63,7 @@ def get_username_from_keyring(self) -> Optional[str]: # To support keyring prior to 15.2 pass except Exception as exc: - logger.warning(str(exc)) + logger.exception(exc) return None def get_password_from_keyring(self) -> Optional[str]: @@ -73,7 +73,7 @@ def get_password_from_keyring(self) -> Optional[str]: logger.info("Querying keyring for password") return cast(str, keyring.get_password(system, username)) except Exception as exc: - logger.warning(str(exc)) + logger.exception(exc) return None def username_from_keyring_or_prompt(self) -> str: From 942a76c51bc2367deabdabb7e60c58a8c733588e Mon Sep 17 00:00:00 2001 From: Federico D'Ambrosio Date: Wed, 20 Apr 2022 17:31:14 +0200 Subject: [PATCH 02/17] Revert logging back to warning Update tests --- tests/test_auth.py | 15 +++++++-------- twine/auth.py | 4 ++-- 2 files changed, 9 insertions(+), 10 deletions(-) diff --git a/tests/test_auth.py b/tests/test_auth.py index 566210cc..f83a6711 100644 --- a/tests/test_auth.py +++ b/tests/test_auth.py @@ -152,14 +152,14 @@ def test_get_username_runtime_error_suppressed( entered_username, keyring_no_backends_get_credential, caplog, config ): assert auth.Resolver(config, auth.CredentialInput()).username == "entered user" - assert caplog.messages == ["fail!"] + assert caplog.messages == ["Error from keyring"] and "fail!" in caplog.text def test_get_password_runtime_error_suppressed( entered_password, keyring_no_backends, caplog, config ): assert auth.Resolver(config, auth.CredentialInput("user")).password == "entered pw" - assert caplog.messages == ["fail!"] + assert caplog.messages == ["Error from keyring"] and "fail!" in caplog.text def test_get_username_return_none(entered_username, monkeypatch, config): @@ -233,14 +233,13 @@ def test_log_exception_on_keyring_failure(config, monkeypatch, caplog): class FailKeyring: @staticmethod def get_credential(system, username): - import os - - # Let's simulate the error that occurred - # in https://github.com/pypa/twine/issues/889 - os.environ["HOME"] + # Simulate the error from https://github.com/pypa/twine/issues/889 + environ = {} + environ["HOME"] monkeypatch.setattr(auth, "keyring", FailKeyring()) assert not auth.Resolver(config, auth.CredentialInput()).get_username_from_keyring() + assert not auth.Resolver(config, auth.CredentialInput()).get_password_from_keyring() - assert "KeyError: 'HOME'" in caplog.text and caplog.messages[0] == "'HOME'" + assert caplog.messages[0] == "Error from keyring" and "KeyError: 'HOME'" in caplog.text diff --git a/twine/auth.py b/twine/auth.py index 57afff5f..6116e7c2 100644 --- a/twine/auth.py +++ b/twine/auth.py @@ -63,7 +63,7 @@ def get_username_from_keyring(self) -> Optional[str]: # To support keyring prior to 15.2 pass except Exception as exc: - logger.exception(exc) + logger.warning("Error from keyring", exc_info=exc) return None def get_password_from_keyring(self) -> Optional[str]: @@ -73,7 +73,7 @@ def get_password_from_keyring(self) -> Optional[str]: logger.info("Querying keyring for password") return cast(str, keyring.get_password(system, username)) except Exception as exc: - logger.exception(exc) + logger.warning("Error from keyring", exc_info=exc) return None def username_from_keyring_or_prompt(self) -> str: From f851116a6e1e9b2a25180a6ddb91845526c4d1e6 Mon Sep 17 00:00:00 2001 From: Federico D'Ambrosio Date: Wed, 20 Apr 2022 17:35:07 +0200 Subject: [PATCH 03/17] Add changelog entry --- changelog/890.bugfix.rst | 1 + 1 file changed, 1 insertion(+) create mode 100644 changelog/890.bugfix.rst diff --git a/changelog/890.bugfix.rst b/changelog/890.bugfix.rst new file mode 100644 index 00000000..d4dfd1bf --- /dev/null +++ b/changelog/890.bugfix.rst @@ -0,0 +1 @@ +Improve logging when keyring fails. From 4911d58f22a36caffdc39dfb50003a862fc5dea4 Mon Sep 17 00:00:00 2001 From: Brian Rutledge Date: Sat, 21 May 2022 08:58:00 -0400 Subject: [PATCH 04/17] Parameterize test --- tests/test_auth.py | 16 ++++++++++++---- 1 file changed, 12 insertions(+), 4 deletions(-) diff --git a/tests/test_auth.py b/tests/test_auth.py index f83a6711..d0c8964e 100644 --- a/tests/test_auth.py +++ b/tests/test_auth.py @@ -229,7 +229,14 @@ def test_warns_for_empty_password( assert caplog.messages[0].startswith(warning) -def test_log_exception_on_keyring_failure(config, monkeypatch, caplog): +@pytest.mark.parametrize( + "get_credential", + [ + auth.Resolver.get_username_from_keyring, + auth.Resolver.get_password_from_keyring, + ], +) +def test_log_exception_on_keyring_failure(get_credential, config, monkeypatch, caplog): class FailKeyring: @staticmethod def get_credential(system, username): @@ -239,7 +246,8 @@ def get_credential(system, username): monkeypatch.setattr(auth, "keyring", FailKeyring()) - assert not auth.Resolver(config, auth.CredentialInput()).get_username_from_keyring() - assert not auth.Resolver(config, auth.CredentialInput()).get_password_from_keyring() + assert not get_credential(auth.Resolver(config, auth.CredentialInput())) - assert caplog.messages[0] == "Error from keyring" and "KeyError: 'HOME'" in caplog.text + assert ( + caplog.messages[0] == "Error from keyring" and "KeyError: 'HOME'" in caplog.text + ) From 3851a2bc5689910471ef57a0d479fb87bdf29d71 Mon Sep 17 00:00:00 2001 From: Brian Rutledge Date: Sat, 21 May 2022 09:05:10 -0400 Subject: [PATCH 05/17] Use re.search for caplog --- tests/test_auth.py | 9 +++++---- 1 file changed, 5 insertions(+), 4 deletions(-) diff --git a/tests/test_auth.py b/tests/test_auth.py index d0c8964e..624a3fbd 100644 --- a/tests/test_auth.py +++ b/tests/test_auth.py @@ -1,5 +1,6 @@ import getpass import logging +import re import pytest @@ -152,14 +153,14 @@ def test_get_username_runtime_error_suppressed( entered_username, keyring_no_backends_get_credential, caplog, config ): assert auth.Resolver(config, auth.CredentialInput()).username == "entered user" - assert caplog.messages == ["Error from keyring"] and "fail!" in caplog.text + assert re.search(r"Error from keyring.+fail!", caplog.text, re.DOTALL) def test_get_password_runtime_error_suppressed( entered_password, keyring_no_backends, caplog, config ): assert auth.Resolver(config, auth.CredentialInput("user")).password == "entered pw" - assert caplog.messages == ["Error from keyring"] and "fail!" in caplog.text + assert re.search(r"Error from keyring.+fail!", caplog.text, re.DOTALL) def test_get_username_return_none(entered_username, monkeypatch, config): @@ -248,6 +249,6 @@ def get_credential(system, username): assert not get_credential(auth.Resolver(config, auth.CredentialInput())) - assert ( - caplog.messages[0] == "Error from keyring" and "KeyError: 'HOME'" in caplog.text + assert re.search( + r"Error from keyring.+Traceback.+KeyError: 'HOME'", caplog.text, re.DOTALL ) From 62abc9d8db9bc9298f31bfcda1206b4c7881f5f2 Mon Sep 17 00:00:00 2001 From: Brian Rutledge Date: Sat, 21 May 2022 09:17:28 -0400 Subject: [PATCH 06/17] Group similar tests --- tests/test_auth.py | 56 +++++++++++++++++++++++++--------------------- 1 file changed, 30 insertions(+), 26 deletions(-) diff --git a/tests/test_auth.py b/tests/test_auth.py index 624a3fbd..3db2232a 100644 --- a/tests/test_auth.py +++ b/tests/test_auth.py @@ -153,14 +153,42 @@ def test_get_username_runtime_error_suppressed( entered_username, keyring_no_backends_get_credential, caplog, config ): assert auth.Resolver(config, auth.CredentialInput()).username == "entered user" - assert re.search(r"Error from keyring.+fail!", caplog.text, re.DOTALL) + assert re.search( + r"Error from keyring.+Traceback.+RuntimeError: fail!", caplog.text, re.DOTALL + ) def test_get_password_runtime_error_suppressed( entered_password, keyring_no_backends, caplog, config ): assert auth.Resolver(config, auth.CredentialInput("user")).password == "entered pw" - assert re.search(r"Error from keyring.+fail!", caplog.text, re.DOTALL) + assert re.search( + r"Error from keyring.+Traceback.+RuntimeError: fail!", caplog.text, re.DOTALL + ) + + +@pytest.mark.parametrize( + "get_credential", + [ + auth.Resolver.get_username_from_keyring, + auth.Resolver.get_password_from_keyring, + ], +) +def test_log_exception_on_keyring_failure(get_credential, config, monkeypatch, caplog): + class FailKeyring: + @staticmethod + def get_credential(system, username): + # Simulate the error from https://github.com/pypa/twine/issues/889 + environ = {} + environ["HOME"] + + monkeypatch.setattr(auth, "keyring", FailKeyring()) + + assert not get_credential(auth.Resolver(config, auth.CredentialInput())) + + assert re.search( + r"Error from keyring.+Traceback.+KeyError: 'HOME'", caplog.text, re.DOTALL + ) def test_get_username_return_none(entered_username, monkeypatch, config): @@ -228,27 +256,3 @@ def test_warns_for_empty_password( assert auth.Resolver(config, auth.CredentialInput()).password == password assert caplog.messages[0].startswith(warning) - - -@pytest.mark.parametrize( - "get_credential", - [ - auth.Resolver.get_username_from_keyring, - auth.Resolver.get_password_from_keyring, - ], -) -def test_log_exception_on_keyring_failure(get_credential, config, monkeypatch, caplog): - class FailKeyring: - @staticmethod - def get_credential(system, username): - # Simulate the error from https://github.com/pypa/twine/issues/889 - environ = {} - environ["HOME"] - - monkeypatch.setattr(auth, "keyring", FailKeyring()) - - assert not get_credential(auth.Resolver(config, auth.CredentialInput())) - - assert re.search( - r"Error from keyring.+Traceback.+KeyError: 'HOME'", caplog.text, re.DOTALL - ) From 2471d9165a4b1a7935c2de5d812b123e0845217d Mon Sep 17 00:00:00 2001 From: Brian Rutledge Date: Sat, 28 May 2022 07:41:58 -0400 Subject: [PATCH 07/17] Rework missing HOME setup --- tests/test_auth.py | 35 +++++++++++++++++++---------------- 1 file changed, 19 insertions(+), 16 deletions(-) diff --git a/tests/test_auth.py b/tests/test_auth.py index 3db2232a..4f74b805 100644 --- a/tests/test_auth.py +++ b/tests/test_auth.py @@ -167,24 +167,27 @@ def test_get_password_runtime_error_suppressed( ) -@pytest.mark.parametrize( - "get_credential", - [ - auth.Resolver.get_username_from_keyring, - auth.Resolver.get_password_from_keyring, - ], -) -def test_log_exception_on_keyring_failure(get_credential, config, monkeypatch, caplog): - class FailKeyring: - @staticmethod - def get_credential(system, username): - # Simulate the error from https://github.com/pypa/twine/issues/889 - environ = {} - environ["HOME"] +def test_keyring_username_missing_home_logs_exception(config, monkeypatch, caplog): + # Simulate environment from https://github.com/pypa/twine/issues/889 + monkeypatch.delenv("HOME") + monkeypatch.setattr("os.getuid", lambda: 999) - monkeypatch.setattr(auth, "keyring", FailKeyring()) + resolver = auth.Resolver(config, auth.CredentialInput()) + assert not resolver.get_username_from_keyring() + + assert re.search( + r"Error from keyring.+Traceback.+KeyError: 'HOME'", caplog.text, re.DOTALL + ) + + +def test_keyring_password_missing_home_logs_exception(config, monkeypatch, caplog): + # Simulate environment from https://github.com/pypa/twine/issues/889 + monkeypatch.delenv("HOME") + monkeypatch.setattr("os.getuid", lambda: 999) - assert not get_credential(auth.Resolver(config, auth.CredentialInput())) + resolver = auth.Resolver(config, auth.CredentialInput("user")) + assert resolver.username == "user" + assert not resolver.get_password_from_keyring() assert re.search( r"Error from keyring.+Traceback.+KeyError: 'HOME'", caplog.text, re.DOTALL From e8afbe410f8cb31bb2543e781b757d87a9522ccf Mon Sep 17 00:00:00 2001 From: Brian Rutledge Date: Sat, 28 May 2022 07:52:53 -0400 Subject: [PATCH 08/17] Refactor fixtures --- tests/test_auth.py | 44 ++++++++++++++++++++------------------------ 1 file changed, 20 insertions(+), 24 deletions(-) diff --git a/tests/test_auth.py b/tests/test_auth.py index 4f74b805..9084f778 100644 --- a/tests/test_auth.py +++ b/tests/test_auth.py @@ -125,49 +125,45 @@ def test_get_password_keyring_missing_non_interactive_aborts( auth.Private(config, auth.CredentialInput("user")).password -@pytest.fixture -def keyring_no_backends(monkeypatch): - """Simulate missing keyring backend raising RuntimeError on get_password.""" - +def test_get_username_keyring_runtime_error_logged( + entered_username, monkeypatch, config, caplog +): class FailKeyring: - @staticmethod - def get_password(system, username): - raise RuntimeError("fail!") + """Simulate missing keyring backend raising RuntimeError on get_credential.""" - monkeypatch.setattr(auth, "keyring", FailKeyring()) - - -@pytest.fixture -def keyring_no_backends_get_credential(monkeypatch): - """Simulate missing keyring backend raising RuntimeError on get_credential.""" - - class FailKeyring: @staticmethod def get_credential(system, username): raise RuntimeError("fail!") monkeypatch.setattr(auth, "keyring", FailKeyring()) - -def test_get_username_runtime_error_suppressed( - entered_username, keyring_no_backends_get_credential, caplog, config -): assert auth.Resolver(config, auth.CredentialInput()).username == "entered user" + assert re.search( r"Error from keyring.+Traceback.+RuntimeError: fail!", caplog.text, re.DOTALL ) -def test_get_password_runtime_error_suppressed( - entered_password, keyring_no_backends, caplog, config +def test_get_password_keyring_runtime_error_logged( + entered_username, entered_password, monkeypatch, config, caplog ): - assert auth.Resolver(config, auth.CredentialInput("user")).password == "entered pw" + class FailKeyring: + """Simulate missing keyring backend raising RuntimeError on get_password.""" + + @staticmethod + def get_password(system, username): + raise RuntimeError("fail!") + + monkeypatch.setattr(auth, "keyring", FailKeyring()) + + assert auth.Resolver(config, auth.CredentialInput()).password == "entered pw" + assert re.search( r"Error from keyring.+Traceback.+RuntimeError: fail!", caplog.text, re.DOTALL ) -def test_keyring_username_missing_home_logs_exception(config, monkeypatch, caplog): +def test_get_username_keyring_missing_home_logged(monkeypatch, config, caplog): # Simulate environment from https://github.com/pypa/twine/issues/889 monkeypatch.delenv("HOME") monkeypatch.setattr("os.getuid", lambda: 999) @@ -180,7 +176,7 @@ def test_keyring_username_missing_home_logs_exception(config, monkeypatch, caplo ) -def test_keyring_password_missing_home_logs_exception(config, monkeypatch, caplog): +def test_get_password_keyring_missing_home_logged(monkeypatch, config, caplog): # Simulate environment from https://github.com/pypa/twine/issues/889 monkeypatch.delenv("HOME") monkeypatch.setattr("os.getuid", lambda: 999) From 45c1fe4b79794c5b564b8129e2e573726472d472 Mon Sep 17 00:00:00 2001 From: Brian Rutledge Date: Sat, 28 May 2022 08:08:51 -0400 Subject: [PATCH 09/17] Normalize prompt tests --- tests/test_auth.py | 26 +++++++------------------- 1 file changed, 7 insertions(+), 19 deletions(-) diff --git a/tests/test_auth.py b/tests/test_auth.py index 9084f778..9a083010 100644 --- a/tests/test_auth.py +++ b/tests/test_auth.py @@ -14,23 +14,23 @@ def config() -> utils.RepositoryConfig: return dict(repository="system") -def test_get_password_keyring_overrides_prompt(monkeypatch, config): +def test_get_username_keyring_defers_to_prompt(monkeypatch, entered_username, config): class MockKeyring: @staticmethod - def get_password(system, user): - return f"{user}@{system} sekure pa55word" + def get_credential(system, user): + return None - monkeypatch.setattr(auth, "keyring", MockKeyring) + monkeypatch.setattr(auth, "keyring", MockKeyring()) - pw = auth.Resolver(config, auth.CredentialInput("user")).password - assert pw == "user@system sekure pa55word" + username = auth.Resolver(config, auth.CredentialInput()).username + assert username == "entered user" def test_get_password_keyring_defers_to_prompt(monkeypatch, entered_password, config): class MockKeyring: @staticmethod def get_password(system, user): - return + return None monkeypatch.setattr(auth, "keyring", MockKeyring) @@ -190,18 +190,6 @@ def test_get_password_keyring_missing_home_logged(monkeypatch, config, caplog): ) -def test_get_username_return_none(entered_username, monkeypatch, config): - """Prompt for username when it's not in keyring.""" - - class FailKeyring: - @staticmethod - def get_credential(system, username): - return None - - monkeypatch.setattr(auth, "keyring", FailKeyring()) - assert auth.Resolver(config, auth.CredentialInput()).username == "entered user" - - def test_logs_cli_values(caplog): caplog.set_level(logging.INFO, "twine") From 6246b94c6b6bc65a146b953a4c0fb6fcec3e5fe8 Mon Sep 17 00:00:00 2001 From: Brian Rutledge Date: Sat, 28 May 2022 09:13:02 -0400 Subject: [PATCH 10/17] Fix incorrect non-interactive test --- tests/test_auth.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tests/test_auth.py b/tests/test_auth.py index 9a083010..6ef0b902 100644 --- a/tests/test_auth.py +++ b/tests/test_auth.py @@ -52,7 +52,7 @@ def test_empty_password_bypasses_prompt(monkeypatch, entered_password, config): def test_no_username_non_interactive_aborts(config): with pytest.raises(exceptions.NonInteractive): - auth.Private(config, auth.CredentialInput("user")).password + auth.Private(config, auth.CredentialInput()).username def test_no_password_non_interactive_aborts(config): From 5a6f7cc4c1042123fca96177f7f47ac208005981 Mon Sep 17 00:00:00 2001 From: Brian Rutledge Date: Sat, 28 May 2022 09:20:10 -0400 Subject: [PATCH 11/17] Reload keyring to reproduce test failure --- tests/test_auth.py | 28 +++++++++++++++++++++------- 1 file changed, 21 insertions(+), 7 deletions(-) diff --git a/tests/test_auth.py b/tests/test_auth.py index 6ef0b902..3453152a 100644 --- a/tests/test_auth.py +++ b/tests/test_auth.py @@ -1,4 +1,5 @@ import getpass +import importlib import logging import re @@ -14,6 +15,20 @@ def config() -> utils.RepositoryConfig: return dict(repository="system") +@pytest.fixture(autouse=True) +def reload_keyring(): + """ + Force keyring to reinitialize its backend for every test. + + This is a hack to support the keyring_missing_home fixture, but also to avoid + surprises in the future. + + Specifically, this clears keyring.core._keyring_backend, which ensures + keyring.core.init_backend() is called in the context of a monkeypatch. + """ + importlib.reload(importlib.import_module("keyring.core")) + + def test_get_username_keyring_defers_to_prompt(monkeypatch, entered_username, config): class MockKeyring: @staticmethod @@ -163,11 +178,14 @@ def get_password(system, username): ) -def test_get_username_keyring_missing_home_logged(monkeypatch, config, caplog): - # Simulate environment from https://github.com/pypa/twine/issues/889 +@pytest.fixture +def keyring_missing_home(monkeypatch): + """Simulate environment from https://github.com/pypa/twine/issues/889.""" monkeypatch.delenv("HOME") monkeypatch.setattr("os.getuid", lambda: 999) + +def test_get_username_keyring_missing_home_logged(keyring_missing_home, config, caplog): resolver = auth.Resolver(config, auth.CredentialInput()) assert not resolver.get_username_from_keyring() @@ -176,11 +194,7 @@ def test_get_username_keyring_missing_home_logged(monkeypatch, config, caplog): ) -def test_get_password_keyring_missing_home_logged(monkeypatch, config, caplog): - # Simulate environment from https://github.com/pypa/twine/issues/889 - monkeypatch.delenv("HOME") - monkeypatch.setattr("os.getuid", lambda: 999) - +def test_get_password_keyring_missing_home_logged(keyring_missing_home, config, caplog): resolver = auth.Resolver(config, auth.CredentialInput("user")) assert resolver.username == "user" assert not resolver.get_password_from_keyring() From e7efaa83d53387b19291cdd9a55014721165aa99 Mon Sep 17 00:00:00 2001 From: Brian Rutledge Date: Sat, 28 May 2022 14:19:22 -0400 Subject: [PATCH 12/17] Patch pwd instead of os --- tests/test_auth.py | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/tests/test_auth.py b/tests/test_auth.py index 3453152a..d4aae7ce 100644 --- a/tests/test_auth.py +++ b/tests/test_auth.py @@ -181,8 +181,12 @@ def get_password(system, username): @pytest.fixture def keyring_missing_home(monkeypatch): """Simulate environment from https://github.com/pypa/twine/issues/889.""" + + def getpwuid(_): + raise KeyError("uid not found: 999") + monkeypatch.delenv("HOME") - monkeypatch.setattr("os.getuid", lambda: 999) + monkeypatch.setattr("pwd.getpwuid", getpwuid) def test_get_username_keyring_missing_home_logged(keyring_missing_home, config, caplog): From 306ec40811eb725af93c0a43d4a9445ac72b1feb Mon Sep 17 00:00:00 2001 From: Brian Rutledge Date: Sat, 28 May 2022 14:21:49 -0400 Subject: [PATCH 13/17] Make assertions more specific --- tests/test_auth.py | 20 ++++++++++++++++++-- 1 file changed, 18 insertions(+), 2 deletions(-) diff --git a/tests/test_auth.py b/tests/test_auth.py index d4aae7ce..fa569eb1 100644 --- a/tests/test_auth.py +++ b/tests/test_auth.py @@ -190,21 +190,37 @@ def getpwuid(_): def test_get_username_keyring_missing_home_logged(keyring_missing_home, config, caplog): + caplog.set_level(logging.INFO) + resolver = auth.Resolver(config, auth.CredentialInput()) assert not resolver.get_username_from_keyring() assert re.search( - r"Error from keyring.+Traceback.+KeyError: 'HOME'", caplog.text, re.DOTALL + r"Querying keyring for username" + r".+Error from keyring" + r".+Traceback" + r".+KeyError: 'HOME'" + r".+KeyError: 'uid not found: 999'", + caplog.text, + re.DOTALL, ) def test_get_password_keyring_missing_home_logged(keyring_missing_home, config, caplog): + caplog.set_level(logging.INFO) + resolver = auth.Resolver(config, auth.CredentialInput("user")) assert resolver.username == "user" assert not resolver.get_password_from_keyring() assert re.search( - r"Error from keyring.+Traceback.+KeyError: 'HOME'", caplog.text, re.DOTALL + r"Querying keyring for password" + r".+Error from keyring" + r".+Traceback" + r".+KeyError: 'HOME'" + r".+KeyError: 'uid not found: 999'", + caplog.text, + re.DOTALL, ) From 41dc6bf61f058f23dfade94c06443316af495329 Mon Sep 17 00:00:00 2001 From: Brian Rutledge Date: Sat, 28 May 2022 14:35:01 -0400 Subject: [PATCH 14/17] Move monkeypatch to keyring --- tests/test_auth.py | 48 ++++++++++++++++++---------------------------- 1 file changed, 19 insertions(+), 29 deletions(-) diff --git a/tests/test_auth.py b/tests/test_auth.py index fa569eb1..6ccecf19 100644 --- a/tests/test_auth.py +++ b/tests/test_auth.py @@ -1,5 +1,4 @@ import getpass -import importlib import logging import re @@ -15,20 +14,6 @@ def config() -> utils.RepositoryConfig: return dict(repository="system") -@pytest.fixture(autouse=True) -def reload_keyring(): - """ - Force keyring to reinitialize its backend for every test. - - This is a hack to support the keyring_missing_home fixture, but also to avoid - surprises in the future. - - Specifically, this clears keyring.core._keyring_backend, which ensures - keyring.core.init_backend() is called in the context of a monkeypatch. - """ - importlib.reload(importlib.import_module("keyring.core")) - - def test_get_username_keyring_defers_to_prompt(monkeypatch, entered_username, config): class MockKeyring: @staticmethod @@ -178,26 +163,27 @@ def get_password(system, username): ) -@pytest.fixture -def keyring_missing_home(monkeypatch): +def _raise_home_key_error(): """Simulate environment from https://github.com/pypa/twine/issues/889.""" - - def getpwuid(_): + try: + raise KeyError("HOME") + except KeyError: raise KeyError("uid not found: 999") - monkeypatch.delenv("HOME") - monkeypatch.setattr("pwd.getpwuid", getpwuid) +def test_get_username_keyring_missing_home_logged(monkeypatch, config, caplog): + class FailKeyring: + @staticmethod + def get_credential(system, username): + _raise_home_key_error() -def test_get_username_keyring_missing_home_logged(keyring_missing_home, config, caplog): - caplog.set_level(logging.INFO) + monkeypatch.setattr(auth, "keyring", FailKeyring()) resolver = auth.Resolver(config, auth.CredentialInput()) assert not resolver.get_username_from_keyring() assert re.search( - r"Querying keyring for username" - r".+Error from keyring" + r"Error from keyring" r".+Traceback" r".+KeyError: 'HOME'" r".+KeyError: 'uid not found: 999'", @@ -206,16 +192,20 @@ def test_get_username_keyring_missing_home_logged(keyring_missing_home, config, ) -def test_get_password_keyring_missing_home_logged(keyring_missing_home, config, caplog): - caplog.set_level(logging.INFO) +def test_get_password_keyring_missing_home_logged(monkeypatch, config, caplog): + class FailKeyring: + @staticmethod + def get_password(system, username): + _raise_home_key_error() + + monkeypatch.setattr(auth, "keyring", FailKeyring()) resolver = auth.Resolver(config, auth.CredentialInput("user")) assert resolver.username == "user" assert not resolver.get_password_from_keyring() assert re.search( - r"Querying keyring for password" - r".+Error from keyring" + r"Error from keyring" r".+Traceback" r".+KeyError: 'HOME'" r".+KeyError: 'uid not found: 999'", From 0e0ea1adb1a625a2308a1e6367215b22bb37a8bc Mon Sep 17 00:00:00 2001 From: Brian Rutledge Date: Sat, 28 May 2022 14:58:41 -0400 Subject: [PATCH 15/17] Test attributes instead of methods --- tests/test_auth.py | 15 ++++++++------- 1 file changed, 8 insertions(+), 7 deletions(-) diff --git a/tests/test_auth.py b/tests/test_auth.py index 6ccecf19..6b5847a5 100644 --- a/tests/test_auth.py +++ b/tests/test_auth.py @@ -171,7 +171,9 @@ def _raise_home_key_error(): raise KeyError("uid not found: 999") -def test_get_username_keyring_missing_home_logged(monkeypatch, config, caplog): +def test_get_username_keyring_key_error_logged( + entered_username, monkeypatch, config, caplog +): class FailKeyring: @staticmethod def get_credential(system, username): @@ -179,8 +181,7 @@ def get_credential(system, username): monkeypatch.setattr(auth, "keyring", FailKeyring()) - resolver = auth.Resolver(config, auth.CredentialInput()) - assert not resolver.get_username_from_keyring() + assert auth.Resolver(config, auth.CredentialInput()).username == "entered user" assert re.search( r"Error from keyring" @@ -192,7 +193,9 @@ def get_credential(system, username): ) -def test_get_password_keyring_missing_home_logged(monkeypatch, config, caplog): +def test_get_password_keyring_key_error_logged( + entered_username, entered_password, monkeypatch, config, caplog +): class FailKeyring: @staticmethod def get_password(system, username): @@ -200,9 +203,7 @@ def get_password(system, username): monkeypatch.setattr(auth, "keyring", FailKeyring()) - resolver = auth.Resolver(config, auth.CredentialInput("user")) - assert resolver.username == "user" - assert not resolver.get_password_from_keyring() + assert auth.Resolver(config, auth.CredentialInput()).password == "entered pw" assert re.search( r"Error from keyring" From 5b2275fd38f32d69db388a23ecb6fffb4eb36a44 Mon Sep 17 00:00:00 2001 From: Brian Rutledge Date: Sat, 28 May 2022 15:02:20 -0400 Subject: [PATCH 16/17] Use more specific log messages --- tests/test_auth.py | 12 ++++++++---- twine/auth.py | 4 ++-- 2 files changed, 10 insertions(+), 6 deletions(-) diff --git a/tests/test_auth.py b/tests/test_auth.py index 6b5847a5..c3a940ce 100644 --- a/tests/test_auth.py +++ b/tests/test_auth.py @@ -140,7 +140,9 @@ def get_credential(system, username): assert auth.Resolver(config, auth.CredentialInput()).username == "entered user" assert re.search( - r"Error from keyring.+Traceback.+RuntimeError: fail!", caplog.text, re.DOTALL + r"Error getting username from keyring.+Traceback.+RuntimeError: fail!", + caplog.text, + re.DOTALL, ) @@ -159,7 +161,9 @@ def get_password(system, username): assert auth.Resolver(config, auth.CredentialInput()).password == "entered pw" assert re.search( - r"Error from keyring.+Traceback.+RuntimeError: fail!", caplog.text, re.DOTALL + r"Error getting password from keyring.+Traceback.+RuntimeError: fail!", + caplog.text, + re.DOTALL, ) @@ -184,7 +188,7 @@ def get_credential(system, username): assert auth.Resolver(config, auth.CredentialInput()).username == "entered user" assert re.search( - r"Error from keyring" + r"Error getting username from keyring" r".+Traceback" r".+KeyError: 'HOME'" r".+KeyError: 'uid not found: 999'", @@ -206,7 +210,7 @@ def get_password(system, username): assert auth.Resolver(config, auth.CredentialInput()).password == "entered pw" assert re.search( - r"Error from keyring" + r"Error getting password from keyring" r".+Traceback" r".+KeyError: 'HOME'" r".+KeyError: 'uid not found: 999'", diff --git a/twine/auth.py b/twine/auth.py index 6116e7c2..83fa6181 100644 --- a/twine/auth.py +++ b/twine/auth.py @@ -63,7 +63,7 @@ def get_username_from_keyring(self) -> Optional[str]: # To support keyring prior to 15.2 pass except Exception as exc: - logger.warning("Error from keyring", exc_info=exc) + logger.warning("Error getting username from keyring", exc_info=exc) return None def get_password_from_keyring(self) -> Optional[str]: @@ -73,7 +73,7 @@ def get_password_from_keyring(self) -> Optional[str]: logger.info("Querying keyring for password") return cast(str, keyring.get_password(system, username)) except Exception as exc: - logger.warning("Error from keyring", exc_info=exc) + logger.warning("Error getting password from keyring", exc_info=exc) return None def username_from_keyring_or_prompt(self) -> str: From eecdc9f13d5b0b25966cf448e1f7b401d2b42b44 Mon Sep 17 00:00:00 2001 From: Brian Rutledge Date: Sat, 28 May 2022 15:08:00 -0400 Subject: [PATCH 17/17] Normalize keyring monkeypatch --- tests/test_auth.py | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/tests/test_auth.py b/tests/test_auth.py index c3a940ce..d5abfec7 100644 --- a/tests/test_auth.py +++ b/tests/test_auth.py @@ -20,7 +20,7 @@ class MockKeyring: def get_credential(system, user): return None - monkeypatch.setattr(auth, "keyring", MockKeyring()) + monkeypatch.setattr(auth, "keyring", MockKeyring) username = auth.Resolver(config, auth.CredentialInput()).username assert username == "entered user" @@ -135,7 +135,7 @@ class FailKeyring: def get_credential(system, username): raise RuntimeError("fail!") - monkeypatch.setattr(auth, "keyring", FailKeyring()) + monkeypatch.setattr(auth, "keyring", FailKeyring) assert auth.Resolver(config, auth.CredentialInput()).username == "entered user" @@ -156,7 +156,7 @@ class FailKeyring: def get_password(system, username): raise RuntimeError("fail!") - monkeypatch.setattr(auth, "keyring", FailKeyring()) + monkeypatch.setattr(auth, "keyring", FailKeyring) assert auth.Resolver(config, auth.CredentialInput()).password == "entered pw" @@ -183,7 +183,7 @@ class FailKeyring: def get_credential(system, username): _raise_home_key_error() - monkeypatch.setattr(auth, "keyring", FailKeyring()) + monkeypatch.setattr(auth, "keyring", FailKeyring) assert auth.Resolver(config, auth.CredentialInput()).username == "entered user" @@ -205,7 +205,7 @@ class FailKeyring: def get_password(system, username): _raise_home_key_error() - monkeypatch.setattr(auth, "keyring", FailKeyring()) + monkeypatch.setattr(auth, "keyring", FailKeyring) assert auth.Resolver(config, auth.CredentialInput()).password == "entered pw"