Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Closed
wants to merge 1 commit into from

Conversation

altendky
Copy link

@altendky altendky commented Jun 4, 2024

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
Copy link
Owner

jaraco commented Jun 5, 2024

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants