jaraco / keyring

MIT License
1.24k stars 152 forks source link

guard `WinVaultKeyring._get_password()` with `if missing_deps:` #683

Closed altendky closed 2 months ago

altendky commented 2 months ago

Separately, I need to figure out how I'm getting into this state, but from the other code it looks like this guard and explicit exception are the intended pattern for handling missing imports. Though, it does seem that the traceback I am getting would be more useful. But, I'll offer this up for consideration anyways.

https://github.com/Chia-Network/chia-blockchain/actions/runs/9372142688/job/25803038861?pr=18112#step:16:877

________________ test_keyring_dump_empty[empty, full, pretty] _________________
[gw1] win32 -- Python 3.10.11 D:\a\chia-blockchain\chia-blockchain\venv\scripts\python.exe
venv\lib\site-packages\keyring\backends\Windows.py:108: in _get_password
    res = win32cred.CredRead(
E   NameError: name 'win32cred' is not defined

During handling of the above exception, another exception occurred:
venv\lib\site-packages\chia\_tests\util\test_dump_keyring.py:55: in test_keyring_dump_empty
    result = runner.invoke(dump, [*case.args, os.fspath(keyring_path)], catch_exceptions=False)
venv\lib\site-packages\click\testing.py:408: in invoke
    return_value = cli.main(args=args or (), prog_name=prog_name, **extra)
venv\lib\site-packages\click\core.py:1055: in main
    rv = self.invoke(ctx)
venv\lib\site-packages\click\core.py:1404: in invoke
    return ctx.invoke(self.callback, **ctx.params)
venv\lib\site-packages\click\core.py:760: in invoke
    return __callback(*args, **kwargs)
venv\lib\site-packages\chia\util\dump_keyring.py:52: in dump
    saved_passphrase: Optional[str] = KeyringWrapper.get_shared_instance().get_master_passphrase_from_credential_store()
venv\lib\site-packages\chia\util\keyring_wrapper.py:256: in get_master_passphrase_from_credential_store
    return passphrase_store.get_password(  # type: ignore[no-any-return]
venv\lib\site-packages\keyring\backends\Windows.py:98: in get_password
    res = self._get_password(service)
venv\lib\site-packages\keyring\backends\Windows.py:111: in _get_password
    except pywintypes.error as e:
E   NameError: name 'pywintypes' is not defined
jaraco commented 2 months ago

I'd like not to have to repeat the viability check for each and every operation. Adding this change here implies that a similar change should also be added to set_password and delete_password (and get_credential, though that is handled as it shares _get_password), but also to those methods on every other backend.

The designed intention is that one will only construct an instance of a KeyringBackend subclass if the subclass.viable is True. In your environment, I'd expect WinVaultKeyring.viable to be False.

Maybe it would make sense in KeyringBackend.__init__() to assert viability. Something like:

diff --git a/keyring/backend.py b/keyring/backend.py
index 12c8b7f..f60c270 100644
--- a/keyring/backend.py
+++ b/keyring/backend.py
@@ -46,6 +46,8 @@ class KeyringBackend(metaclass=KeyringBackendMeta):
     """

     def __init__(self):
+        # assert viability of the backend
+        self.priority
         self.set_properties_from_env()

     @properties.classproperty

Unfortunately, that diff causes failures with infinite recursion, so it's not as simple as that.

Before we attempt to tackle preventing construction of non-viable backends, let's first figure out why a non-viable backend is getting constructed and whether that's a use-case worth supporting.

Please re-file as an issue and elaborate on the investigation as to what conditions lead to the failure.