Skip to content

Raise from fail.Keyring.get_credential when the username is omitted - #771

Open
feiiiiii5 wants to merge 2 commits into
jaraco:mainfrom
feiiiiii5:fix/fail-get-credential-raises
Open

feiiiiii5 wants to merge 2 commits into
jaraco:mainfrom
feiiiiii5:fix/fail-get-credential-raises

Conversation

@feiiiiii5

Copy link
Copy Markdown

keyring.backends.fail.Keyring is documented as "Keyring that raises error on every operation", and it expresses that by aliasing the operations to the one that raises:

    set_password = delete_password = get_password

get_credential was not in that list, so it used the base implementation, whose guard returns None before it ever reaches get_password:

    def get_credential(self, service, username):
        # The default implementation requires a username here.
        if username is not None:
            password = self.get_password(service, username)
            ...
        return None

So on a machine with no usable backend:

call result
get_password('svc', 'user') NoKeyringError
set_password('svc', 'user', 'pw') NoKeyringError
delete_password('svc', 'user') NoKeyringError
get_credential('svc', 'user') NoKeyringError
get_credential('svc', None) None

That last row is the problem: a caller asking for a credential without a username is told "no such credential", when the truth is "there is no keyring at all". keyring get --mode creds svc exits 1 with empty stdout and no diagnostic, which is the shape of #711's "nothing is returned and a non-zero return code".

The fix is one token on the existing alias line:

    get_credential = set_password = delete_password = get_password

The alias is the only correct site. The base class must keep returning None for an omitted username — that is the documented contract for a backend that needs a username, and keyring/testing/backend.py::test_credential asserts get_credential('service', None) is None for every real backend. Changing the base would break all of them.

Two things I checked before proposing it. The fix goes silent→loud, not the other way round: nothing that used to return a value now returns a wrong value, it raises instead. And fail.Keyring is never a chainer child, so no shared path changes: ChainerBackend.backends filters on keyring.priority > 0 and fail.priority is 0, so the chainer sees an empty child list.

git log puts the alias line on 7564a007 ("Fix typo in fail.Keyring method name", 2018-02-15), which corrected delete_pasword → delete_password but left the omission. get_credential was added to the base class later, in f32f39c1 ("Fix #350 Add get_credential() API", 2018-10-27) — after the alias line, which is how the gap opened. No commit ever added it to the alias.

Tests

tests/backends/test_fail.py, parametrized over the username so the one-sided nature of the bug is pinned:

@pytest.mark.parametrize('username', [None, 'user'])
def test_get_credential_raises(username, monkeypatch):

On 7603e7c the [None] case fails with Failed: DID NOT RAISE NoKeyringError and the ['user'] case already passes; both pass here.

Validation: pytest -q is 4 failed, 48 passed, 80 skipped against 4 failed, 46 passed, 80 skipped on 7603e7c — the two new parametrizations and nothing else. The four failures are pre-existing environment gaps that fail identically on the base commit (keyring/core.py::keyring.core.disable, tests/test_multiprocess.py::test_multiprocess_get, ::test_multiprocess_get_after_native_get, tests/test_packaging.py::test_entry_point). ruff check and ruff format --check are clean on both files.

One consequence worth your call

keyring get --mode creds svc on a backend-less machine will now print a NoKeyringError traceback where it previously exited 1 silently. That is more informative but uglier; catching it in keyring/cli.py would be the tidy version, but that path has been reshaped by #678 and I would rather not widen this diff. If you would prefer an explicit get_credential method on fail.Keyring that raises, rather than extending the alias, that is a one-line difference and I am happy to switch.

The class raises on every operation by aliasing them to get_password,
but get_credential was not in the alias list, so it used the base
implementation — whose `if username is not None:` guard returns None
before ever calling get_password. A caller asking for a credential with
no username therefore got "no such credential" on a machine that has no
keyring backend at all.

The base class keeps its None-on-omitted-username behavior, which
keyring/testing/backend.py::test_credential asserts for every real
backend, so the alias is the only correct site.
feiiiiii5 pushed a commit to feiiiiii5/keyring that referenced this pull request Sep 30, 2026
@feiiiiii5
feiiiiii5 force-pushed the fix/fail-get-credential-raises branch from 8787eb6 to 02e3b14 Compare October 8, 2026 06:29
@feiiiiii5

Copy link
Copy Markdown
Author

Hi @jaraco — still 3 files (+19/−1) at 02e3b14, and I have not pushed anything since.

fail.Keyring documents itself as the keyring that "raises error on every operation", and expresses that by aliasing set_password/delete_password onto get_password. get_credential was left out of that list, so it runs the base implementation, whose guard returns None when username is None before it ever calls get_password — so a caller that has no username yet gets "no entry for this user" out of a backend whose entire job is to say "no keyring is configured".

One thing I would like your call on: I added get_credential to the alias list. If you would rather not tie it to get_password, the alternative I saw is making the base get_credential raise when a subclass declares it has no credential support, which would cover other backends too but is a wider change. Happy to do either in one commit.

@feiiiiii5

Copy link
Copy Markdown
Author

Following up on the two options from my Oct 10 note (still exactly 3 files, +19/−1, at 02e3b14): (a) add get_credential to the fail.Keyring alias list — small and scoped, or (b) make the base get_credential raise for backends that declare no credential support — wider but covers more backends. Point me at (a) or (b) and I'll do it in one commit; if you'd rather take it from here, that's fine too.

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.

Add get_username_and_password API

1 participant