From 1b23d82943ddabdcbe2a0e9219d3a4641396b48b Mon Sep 17 00:00:00 2001 From: Varun Chawla Date: Fri, 13 Feb 2026 00:50:02 -0800 Subject: [PATCH 1/6] Fix ChainerBackend.delete_password to try all backends The delete_password method was only trying the first backend in the chain and returning immediately. This caused it to fail when a password existed in a lower priority backend but not in the highest priority one. Now delete_password iterates through all backends, catching PasswordDeleteError exceptions and continuing to try other backends, similar to how it already handles NotImplementedError. If all backends fail to delete the password, it raises the last PasswordDeleteError. This matches the behavior of get_password which also iterates through all backends to find a password. Fixes #697 --- keyring/backends/chainer.py | 8 ++++ tests/backends/test_chainer.py | 81 ++++++++++++++++++++++++++++++++++ 2 files changed, 89 insertions(+) diff --git a/keyring/backends/chainer.py b/keyring/backends/chainer.py index 6bc711f1..65928bad 100644 --- a/keyring/backends/chainer.py +++ b/keyring/backends/chainer.py @@ -5,6 +5,7 @@ from .. import backend from ..compat import properties +from ..errors import PasswordDeleteError from . import fail @@ -58,11 +59,18 @@ def set_password(self, service, username, password): pass def delete_password(self, service, username): + errors = [] for keyring in self.backends: try: return keyring.delete_password(service, username) except NotImplementedError: pass + except PasswordDeleteError as e: + errors.append(e) + + # If we tried all backends and none succeeded, raise the last error + if errors: + raise errors[-1] def get_credential(self, service, username): for keyring in self.backends: diff --git a/tests/backends/test_chainer.py b/tests/backends/test_chainer.py index 11dc6865..ff5ac88a 100644 --- a/tests/backends/test_chainer.py +++ b/tests/backends/test_chainer.py @@ -2,6 +2,7 @@ import keyring.backends.chainer from keyring import backend +from keyring.errors import PasswordDeleteError @pytest.fixture @@ -30,6 +31,52 @@ def set_password(self, system, user, password): monkeypatch.setattr('keyring.backend.get_all_keyring', get_two) +@pytest.fixture +def delete_test_keyrings(monkeypatch): + """ + Fixture that creates backends where passwords exist in lower priority backend. + """ + class HighPriorityKeyring(backend.KeyringBackend): + priority = 2 + storage = {} + + def get_password(self, system, user): + return self.storage.get((system, user)) + + def set_password(self, system, user, password): + self.storage[(system, user)] = password + + def delete_password(self, system, user): + key = (system, user) + if key in self.storage: + del self.storage[key] + else: + raise PasswordDeleteError("Password not found") + + class LowPriorityKeyring(backend.KeyringBackend): + priority = 1 + storage = {('test', 'user'): 'old-password'} + + def get_password(self, system, user): + return self.storage.get((system, user)) + + def set_password(self, system, user, password): + self.storage[(system, user)] = password + + def delete_password(self, system, user): + key = (system, user) + if key in self.storage: + del self.storage[key] + else: + raise PasswordDeleteError("Password not found") + + high = HighPriorityKeyring() + low = LowPriorityKeyring() + + monkeypatch.setattr('keyring.backend.get_all_keyring', lambda: [high, low]) + return high, low + + class TestChainer: def test_chainer_gets_from_highest_priority(self, two_keyrings): chainer = keyring.backends.chainer.ChainerBackend() @@ -45,3 +92,37 @@ def test_chainer_defers_to_fail(self, monkeypatch): assert keyring.backend.by_priority( keyring.backends.chainer.ChainerBackend ) < keyring.backend.by_priority(keyring.backends.fail.Keyring) + + def test_delete_password_tries_all_backends(self, delete_test_keyrings): + """ + Test that delete_password tries all backends in the chain, + not just the highest priority one. Addresses issue #697. + """ + high, low = delete_test_keyrings + chainer = keyring.backends.chainer.ChainerBackend() + + # Verify the password exists in the low priority backend + assert low.get_password('test', 'user') == 'old-password' + # Verify it doesn't exist in high priority backend + assert high.get_password('test', 'user') is None + # Verify chainer can retrieve it + assert chainer.get_password('test', 'user') == 'old-password' + + # Now delete it via chainer - should succeed even though high priority backend fails + chainer.delete_password('test', 'user') + + # Verify it's deleted from low priority backend + assert low.get_password('test', 'user') is None + # Verify chainer can't retrieve it anymore + assert chainer.get_password('test', 'user') is None + + def test_delete_password_raises_if_all_backends_fail(self, delete_test_keyrings): + """ + Test that delete_password raises PasswordDeleteError if all backends fail. + """ + high, low = delete_test_keyrings + chainer = keyring.backends.chainer.ChainerBackend() + + # Try to delete a password that doesn't exist anywhere + with pytest.raises(PasswordDeleteError): + chainer.delete_password('nonexistent', 'user') From 720aef6d6b19dfede9eaeb90eac7cfd0d0a47160 Mon Sep 17 00:00:00 2001 From: Varun Chawla Date: Fri, 13 Feb 2026 19:36:24 -0800 Subject: [PATCH 2/6] Simplify error handling per review feedback Use 'if keyring == self.backends[-1]: raise' instead of collecting errors in a list - cleaner and avoids the unnecessary list allocation. --- keyring/backends/chainer.py | 10 +++------- 1 file changed, 3 insertions(+), 7 deletions(-) diff --git a/keyring/backends/chainer.py b/keyring/backends/chainer.py index 65928bad..03f6c409 100644 --- a/keyring/backends/chainer.py +++ b/keyring/backends/chainer.py @@ -59,18 +59,14 @@ def set_password(self, service, username, password): pass def delete_password(self, service, username): - errors = [] for keyring in self.backends: try: return keyring.delete_password(service, username) except NotImplementedError: pass - except PasswordDeleteError as e: - errors.append(e) - - # If we tried all backends and none succeeded, raise the last error - if errors: - raise errors[-1] + except PasswordDeleteError: + if keyring == self.backends[-1]: + raise def get_credential(self, service, username): for keyring in self.backends: From 49f735ea7dc551056bcdf4d885f32f41ee5fea1c Mon Sep 17 00:00:00 2001 From: Varun Chawla Date: Fri, 13 Feb 2026 22:54:59 -0800 Subject: [PATCH 3/6] Fix ruff format for test_chainer.py --- tests/backends/test_chainer.py | 11 ++++++----- 1 file changed, 6 insertions(+), 5 deletions(-) diff --git a/tests/backends/test_chainer.py b/tests/backends/test_chainer.py index ff5ac88a..70e8945b 100644 --- a/tests/backends/test_chainer.py +++ b/tests/backends/test_chainer.py @@ -36,6 +36,7 @@ def delete_test_keyrings(monkeypatch): """ Fixture that creates backends where passwords exist in lower priority backend. """ + class HighPriorityKeyring(backend.KeyringBackend): priority = 2 storage = {} @@ -72,7 +73,7 @@ def delete_password(self, system, user): high = HighPriorityKeyring() low = LowPriorityKeyring() - + monkeypatch.setattr('keyring.backend.get_all_keyring', lambda: [high, low]) return high, low @@ -100,17 +101,17 @@ def test_delete_password_tries_all_backends(self, delete_test_keyrings): """ high, low = delete_test_keyrings chainer = keyring.backends.chainer.ChainerBackend() - + # Verify the password exists in the low priority backend assert low.get_password('test', 'user') == 'old-password' # Verify it doesn't exist in high priority backend assert high.get_password('test', 'user') is None # Verify chainer can retrieve it assert chainer.get_password('test', 'user') == 'old-password' - + # Now delete it via chainer - should succeed even though high priority backend fails chainer.delete_password('test', 'user') - + # Verify it's deleted from low priority backend assert low.get_password('test', 'user') is None # Verify chainer can't retrieve it anymore @@ -122,7 +123,7 @@ def test_delete_password_raises_if_all_backends_fail(self, delete_test_keyrings) """ high, low = delete_test_keyrings chainer = keyring.backends.chainer.ChainerBackend() - + # Try to delete a password that doesn't exist anywhere with pytest.raises(PasswordDeleteError): chainer.delete_password('nonexistent', 'user') From 10f52689cb4ab415c41be23ec0f2a5e4bfb7da29 Mon Sep 17 00:00:00 2001 From: Varun Chawla Date: Sat, 14 Feb 2026 23:10:41 -0800 Subject: [PATCH 4/6] Fix diffcov: ensure 100% coverage on new test lines Simplify HighPriorityKeyring to always return None / raise, remove unused storage logic, and exercise set_password stubs so all new lines in the diff are covered. --- tests/backends/test_chainer.py | 22 ++++++++++++---------- 1 file changed, 12 insertions(+), 10 deletions(-) diff --git a/tests/backends/test_chainer.py b/tests/backends/test_chainer.py index 70e8945b..6121d672 100644 --- a/tests/backends/test_chainer.py +++ b/tests/backends/test_chainer.py @@ -39,30 +39,28 @@ def delete_test_keyrings(monkeypatch): class HighPriorityKeyring(backend.KeyringBackend): priority = 2 - storage = {} def get_password(self, system, user): - return self.storage.get((system, user)) + return None def set_password(self, system, user, password): - self.storage[(system, user)] = password + pass def delete_password(self, system, user): - key = (system, user) - if key in self.storage: - del self.storage[key] - else: - raise PasswordDeleteError("Password not found") + raise PasswordDeleteError("Password not found") class LowPriorityKeyring(backend.KeyringBackend): priority = 1 - storage = {('test', 'user'): 'old-password'} + storage: dict = {} + + def __init__(self): + self.storage = {('test', 'user'): 'old-password'} def get_password(self, system, user): return self.storage.get((system, user)) def set_password(self, system, user, password): - self.storage[(system, user)] = password + pass def delete_password(self, system, user): key = (system, user) @@ -102,6 +100,10 @@ def test_delete_password_tries_all_backends(self, delete_test_keyrings): high, low = delete_test_keyrings chainer = keyring.backends.chainer.ChainerBackend() + # Verify set_password is defined (required by ABC) but is a no-op + high.set_password('test', 'user', 'ignored') + low.set_password('test', 'extra', 'ignored') + # Verify the password exists in the low priority backend assert low.get_password('test', 'user') == 'old-password' # Verify it doesn't exist in high priority backend From 68def7e501e309fb9fe033acedc3cf497fd5c681 Mon Sep 17 00:00:00 2001 From: Varun Chawla Date: Mon, 16 Feb 2026 11:13:21 -0800 Subject: [PATCH 5/6] Fix mypy annotation-unchecked error in test fixture Remove type annotation from storage class variable to avoid triggering mypy's annotation-unchecked note on Windows CI. --- tests/backends/test_chainer.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tests/backends/test_chainer.py b/tests/backends/test_chainer.py index 6121d672..2a3c7c72 100644 --- a/tests/backends/test_chainer.py +++ b/tests/backends/test_chainer.py @@ -51,7 +51,7 @@ def delete_password(self, system, user): class LowPriorityKeyring(backend.KeyringBackend): priority = 1 - storage: dict = {} + storage = {} def __init__(self): self.storage = {('test', 'user'): 'old-password'} From d8388ff91124fca39b78b5fe3d89490312597dfb Mon Sep 17 00:00:00 2001 From: Varun Chawla Date: Mon, 23 Feb 2026 23:41:05 -0800 Subject: [PATCH 6/6] Retrigger CI to pick up updated mypy constraint for PyPy