jaraco / keyring

MIT License
1.24k stars 152 forks source link

`NameError: name 'pywintypes' is not defined` when calling `WinVaultKeyring.get_password()` #684

Closed altendky closed 1 month ago

altendky commented 2 months ago

Describe the bug

Following from https://github.com/jaraco/keyring/pull/683#issuecomment-2150571124, I'll describe how it was I got into the state of having .get_password() raising NameError: name 'pywintypes' is not defined. If you think this is a case you want to improve but would like a minimal example or test to work from, I can additionally provide that. If you aren't concerned about this corner case, that's fine. I would say that my 'real issue' can be addressed by improving pywin32-ctypes as referenced below.

1) Why are the imports attempted by keyring failing?

Over in pywin32-ctypes I've started https://github.com/enthought/pywin32-ctypes/pull/130 trying to address their use of deprecated import customization features. As is, their code works but raises a warning. In our tests (the Chia project tests) we enable warnings as errors in pytest to make them 'louder'. As such, the import of pywin32-ctypes fails since the deprecation warning for their import customization is converted to an exception. This is how we trigger the keyring imports to fail.

2) How do we get to the NameError situation?

Below is our code that calls WinVaultKeyring.get_password(). Please note that this is not 'my code' and I make no claims it is using the keyring library 'appropriately'. As mentioned above, what I consider to be the actual bug here is in pywin32-ctypes and the PR I created here for keyring was a side note hoping to help the project along the way, which may not be the result I'm having.

https://github.com/Chia-Network/chia-blockchain/blob/4f0fc830065154c93abf36aa41eb96b268586d13/chia/util/keyring_wrapper.py#L252-L263

    def get_master_passphrase_from_credential_store(self) -> Optional[str]:
        passphrase_store: Optional[OSPassphraseStore] = get_os_passphrase_store()
        if passphrase_store is not None:
            try:
                return passphrase_store.get_password(  # type: ignore[no-any-return]
                    MASTER_PASSPHRASE_SERVICE_NAME,
                    MASTER_PASSPHRASE_USER_NAME,
                )
            except KeyringError as e:
                if not warn_if_macos_errSecInteractionNotAllowed(e):
                    raise
        return None

In the problem case, our function get_os_passphrase_store() is returning an instance of WinVaultKeyring it created directly via the class itself.

https://github.com/Chia-Network/chia-blockchain/blob/4f0fc830065154c93abf36aa41eb96b268586d13/chia/util/keyring_wrapper.py#L29-L34

def get_os_passphrase_store() -> Optional[OSPassphraseStore]:
    if platform == "darwin":
        return MacKeyring()  # type: ignore[no-untyped-call]
    elif platform == "win32" or platform == "cygwin":
        return WinKeyring()  # type: ignore[no-untyped-call]
    return None

NOTE: I can provide a minimal recreation or test if there is interest in improving the corner case being discussed.

To Reproduce Steps to reproduce the behavior:

  1. Go to '...'
  2. Click on '....'
  3. Scroll down to '....'
  4. See error

Expected behavior A clear and concise description of what you expected to happen.

Environment

$ pip list | grep keyring
...
$ keyring --list-backends
...

Additional context Add any other context about the problem here.

jaraco commented 1 month ago

Thanks for the description. I think I understand.

I'm leaning toward not wanting to support this edge case. I'm thinking keyring_wrapper should be checking that whatever backend it constructs is viable before constructing or returning it.

Also, I recommend in tests that if you're transforming warnings to errors that you disable the warnings aggressively after reporting them upstream. See the setuptools pytest.ini for an example of how that's done.

altendky commented 1 month ago

Yes, I had ignored this one in pytest.ini awhile back but was going back through after a couple years to try to weed out the list of deprecations we hadn't handled yet, issues that had been addressed, things I hadn't gone through the effort to report upstream usefully yet...

Thanks for the consideration and continued maintenance in this project and elsewhere.