jaraco / keyring

MIT License
1.24k stars 152 forks source link

get_credential(service, None) does not find anything after deleting one of two usernames #664

Open mkaut opened 8 months ago

mkaut commented 8 months ago

on Windows 10, using the default backend:

keyring.set_password('test', 'user-1', 'pwd-1')
keyring.set_password('test', 'user-2', 'pwd-2')
cred = keyring.get_credential('test', None)
print(cred.username, cred.password)

This returns user-2 pwd-2. So far, so good.

keyring.delete_password('test', 'user-2')
cred = keyring.get_credential('test', 'user-1')
print(cred.username, cred.password)
cred = keyring.get_credential('test', None)
print(cred)

Here, the first output is user-1 pwd-1, showing that the first credential is still there. However, the second output None, i.e. the credential is not found without a username.

Is this a bug, or an expected behaviour?

jaraco commented 5 months ago

At first blush, it looks like it's a bug... and probably has to do with the way that Windows doesn't support per-user credentials so keyring hacks it by creating "compound names" when the user names conflict for a given service.

After the first set_password, the credential store looks like:

system user
test user-1

Then adding the second credential causes the first entry to get the username prepended:

system user
user-1@test user-1
test user-2

At this stage, it's possible to get the most recent credential using just the system name, but without the username, it's not possible to get user-1. Even after you delete user-2, the state is:

system user
user-1@test user-1

It's no longer possible to query simply for system='test'.

When retrieving a credential, the following logic is used:

https://github.com/jaraco/keyring/blob/3727268f0de9d5ab56d94e2cff0a794153769c18/keyring/backends/Windows.py#L163-L169

See #47 for the motivation for this approach.

I can't imagine a way for keyring to locate the compound name without being given the original username unless there were some way to enumerate all credentials. In #555, there was an attempt to implement some changes that would expose enumerating all credentials. If we had that capability, we could potentially scan all credentials for those where the username matches the {user}@{system} format. I'm not confident that could be done in a performant way.

I think we may be stuck with assuming that if the user is storing multiple passwords for the same system that they must always supply the username to retrieve them (and maybe document that expectation).

I'm open to other ideas.

rtr1 commented 5 months ago

To me the most straightforward path is to create an escape hatch by enabling an option like use_compound_name=False.

In my use cases, I much prefer the consistency that if I call keyring.set_password(a,b,c) twice, it’s updating the same entry in Windows Credential Manager. That’s usually what I want, e.g. when I’m updating a connection string, I just want to overwrite what’s already there. And I prefer that if I give it a compound name like keyring.set_password(‘user@service’, …) I get a single user@service entry, whereas the current approach will create a new entry named user@user@service, which doesn’t smell like the right path to me. At that point, we might as well store JSON strings as the password value to facilitate multiple usernames for a single service.

We’d need to keep use_compound_name=True as the default for backward compatibility of course.

Thoughts?

mkaut commented 5 months ago

I can see a couple of different solutions, though I should stress that I don't know much about either this app or Windows credential manager - so feel free to ignore anything I write.