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

Passkeys: Register to an existing entry #10408

Conversation

varjolintu
Copy link
Member

Shows an extra button for creating a passkey directly to an existing entry. It opens the normal import dialog, with some texts changed.

Fixes #10372.

Screenshots

Screenshot 2024-03-12 at 19 40 33
Screenshot 2024-03-12 at 19 40 37

Testing strategy

Manually.

Type of change

  • ✅ New feature (change that adds functionality)

@droidmonkey
Copy link
Member

reminder on this one, will you make any changes or leave as-is?

@varjolintu varjolintu force-pushed the feature/passkeys_register_to_existing_entry branch 3 times, most recently from 420c143 to 20bfdb9 Compare April 1, 2024 06:47
@droidmonkey droidmonkey force-pushed the feature/passkeys_register_to_existing_entry branch from 20bfdb9 to 1c228a6 Compare April 20, 2024 11:40
@varjolintu varjolintu force-pushed the feature/passkeys_register_to_existing_entry branch from 1c228a6 to 44b1adc Compare April 26, 2024 11:08
@droidmonkey
Copy link
Member

droidmonkey commented Apr 27, 2024

@varjolintu I tried to do this and it didn't add the relying party URL to the URL field in the existing entry so I couldn't use the passkey after registration.

Even after manually adding the URL I couldn't authenticate. This was on https://webauthn.io

@varjolintu
Copy link
Member Author

@droidmonkey Let me check..

@varjolintu
Copy link
Member Author

@droidmonkey I created an entry with URL httpx://webauthn2.io and added a passkey to it. And I could authenticate it with normally even if the entry URL differs.

@droidmonkey
Copy link
Member

Interesting

@varjolintu
Copy link
Member Author

Should we keep the old Update button in the popup? If the user now wants to update a new passkey for some site, the entry needs to be selected always via the new dialog. Not so handy.

@droidmonkey
Copy link
Member

droidmonkey commented Apr 27, 2024

Can we pre-populate the import dialog with the first matching entry? That would be equivalent to "update" and also allow for refinement and assurance of choice.

@varjolintu
Copy link
Member Author

Can we pre-populate the import dialog with the first matching entry? That would be equivalent to "update" and also allow for refinement and assurance of choice.

Good idea. The filtering should be easy to do.

@droidmonkey
Copy link
Member

Found the issue, KPEX_PASSKEY_CREDENTIAL_ID is not updated when you overwrite an entry

@varjolintu
Copy link
Member Author

I noticed that parents are not correctly passed to import/export dialogs. Fixing that too.

@varjolintu
Copy link
Member Author

Can we pre-populate the import dialog with the first matching entry? That would be equivalent to "update" and also allow for refinement and assurance of choice.

Tested this, and KeePassXC actually already identifies a case where we are doing an update to an existing credential. So no need to fix this.

@varjolintu varjolintu force-pushed the feature/passkeys_register_to_existing_entry branch from 59de75a to 5018674 Compare April 28, 2024 06:08
@varjolintu
Copy link
Member Author

Found the issue, KPEX_PASSKEY_CREDENTIAL_ID is not updated when you overwrite an entry

This is overridden when I tested it multiple times. Can you provide exact steps how to reproduce this?

@juvannx
Copy link

juvannx commented Apr 28, 2024

I backported this PR to release 2.7.0 just for test.
When I have already a passkey for a site I can't add a new passkey to existent entry, but I have only Update and Register buttons. I tested with 2 google account.
Is it a normal behaviour?

@varjolintu
Copy link
Member Author

I backported this PR to release 2.7.0 just for test. When I have already a passkey for a site I can't add a new passkey to existent entry, but I have only Update and Register buttons. I tested with 2 google account. Is it a normal behaviour?

That is normal behavior, because on this case your passkey's Credential ID does not change, which means the old passkey will be replaced. Otherwise the user would have two different passkeys for the same ID, and the old one won't work anymore.

@darkdragon-001
Copy link

It should be possible to use two different accounts on a single site. Maybe we need a selection dialog as we have one for passwords already where the user can choose which entry they want to use in case KeePassXC returns multiple entries for a given domain.

@varjolintu
Copy link
Member Author

varjolintu commented Apr 28, 2024

It should be possible to use two different accounts on a single site. Maybe we need a selection dialog as we have one for passwords already where the user can choose which entry they want to use in case KeePassXC returns multiple entries for a given domain.

The confirmation dialog already works as a one.
For example:
Screenshot 2024-04-28 at 15 45 32

@juvannx
Copy link

juvannx commented Apr 28, 2024

because on this case your passkey's Credential ID does not change

I don't know very well passkeys, so for 2 or more different accounts on Google the credential ID is the same?
I tested 2 different google accounts and for the second account it ask me to update or register.
If I register I have 2 different credential ID, so I was expecting the possibility to add passkeys in existing entry also for the second account.

@droidmonkey
Copy link
Member

droidmonkey commented Apr 28, 2024

Every time you make a new passkey with a service, it gets a new credential id. You can have multiple credential id's associated with one account, but NOT in the same KeePassXC entry. Each credential id needs it's own entry.

@juvannx
Copy link

juvannx commented Apr 28, 2024

Each credential id needs it's own entry.

So, it is correct that it should ask to add also there is another entry with passkey, at most it can offer 3 possibilities update, add to, register.

@juvannx
Copy link

juvannx commented Apr 28, 2024

I don't have much experience with C, C++ and the QT library but I think that in addition to checking existingEntries.isEmpty() it need to check if the user who is registering is new or exists in the list, in case it doesn't exist instead of Update is proposed Add to

@droidmonkey
Copy link
Member

We have been discussing this above, thank you though

@droidmonkey
Copy link
Member

@varjolintu I am having a lot of problems with this PR. I can register a new entry and add to an existing one. I never see the "update" button. When I try to register a new passkey with the same username I get the OS dialog. This was all on https://webauthn.io.

@droidmonkey droidmonkey self-requested a review April 28, 2024 22:39
@droidmonkey droidmonkey force-pushed the feature/passkeys_register_to_existing_entry branch from 5018674 to 9c4531f Compare April 28, 2024 22:39
@varjolintu
Copy link
Member Author

@varjolintu I am having a lot of problems with this PR. I can register a new entry and add to an existing one. I never see the "update" button. When I try to register a new passkey with the same username I get the OS dialog. This was all on https://webauthn.io.

This happens because webauthn.io always creates a new user ID. The OS dialog appears when you are having existing credentials included in the server request. If you test it against this PR, you'll get the error message as you should.

If you want to test the Update button, you can try registering a new passkey using this site: https://webauthn.me/debugger# where the ID does not change between requests.

@droidmonkey
Copy link
Member

ahhh ok thank you, I will realign my testing site :-)

Copy link
Member

@droidmonkey droidmonkey left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Need to run translation update

src/browser/BrowserPasskeysConfirmationDialog.cpp Outdated Show resolved Hide resolved
@varjolintu varjolintu force-pushed the feature/passkeys_register_to_existing_entry branch from def6316 to 9849b84 Compare May 5, 2024 05:24
@varjolintu varjolintu force-pushed the feature/passkeys_register_to_existing_entry branch from 9849b84 to d0f9758 Compare May 5, 2024 05:29
@droidmonkey droidmonkey merged commit 92b30ae into keepassxreboot:develop May 5, 2024
11 checks passed
@varjolintu varjolintu deleted the feature/passkeys_register_to_existing_entry branch May 5, 2024 18:20
@droidmonkey droidmonkey added the pr: backported Pull request backported to previous release label May 5, 2024
libf-de pushed a commit to libf-de/keepassxc-secretservice-dbus that referenced this pull request May 6, 2024
Release 2.7.8

Changes
- Add hotkey for showing search help [keepassxreboot#10591]
- Add hotkey for group switching (Ctrl+Shift+PgUp/PgDown) [keepassxreboot#10625]
- Add per-database auto-save delay setting [keepassxreboot#9100]
- Add setting to hide menubar [keepassxreboot#10341]
- Improve Bitwarden 1PUX import and support organization collections [keepassxreboot#10499]
- Show advanced settings checkbox only for settings that have them [keepassxreboot#6513]
- Remove obsolete setting for requiring repeated password entry [keepassxreboot#9722]
- Passkeys: Allow registering Passkeys to existing entries [keepassxreboot#10408]
- Passkeys: Show warning about data being unencrypted before Passkey export [keepassxreboot#10411]
- Passkeys: Support NFC and USB transports [keepassxreboot#10402]
- Passkeys: Pass extension JSON data to browser [keepassxreboot#10615]
- SSH Agent: Do not use entries from recycle bin [keepassxreboot#10518]
- Linux: Change hotkey sequence used for {CLEARFIELD} Auto-Type [keepassxreboot#10008]
- Windows: Improve DACL memory access protection [keepassxreboot#10618]

Fixes
- Fix crash when deleting history items [keepassxreboot#10451]
- Fix crash on screen lock or computer sleep [keepassxreboot#10458]
- Fix search field not being focused after unlock [keepassxreboot#10459]
- Fix loss of window focus when Auto-Type needs to unlock a database [keepassxreboot#10555]
- Fix inconsistent TOTP visibility on unlock [keepassxreboot#10009]
- Fix CSV import skipping over single-name groups [keepassxreboot#10575]
- Fix key file folder being remembered even if disabled in settings [keepassxreboot#10636]
- Fix issues with entry editing and database locking [keepassxreboot#10667]
- Fix key file text when provided on command line [keepassxreboot#10642]
- Fix issues with hardware key auto detection [keepassxreboot#10663]
- Do not override monospace font size [keepassxreboot#10282]
- Perform group sort only when group view is in focus [keepassxreboot#10202]
- Do not show decimals for attachment sizes in Bytes [keepassxreboot#10595]
- Prevent merging of global custom data when merging databases [keepassxreboot#10452]
- Fix minor translation issues [keepassxreboot#10635]
- Passkeys: Fix StrongBox incompatibility [keepassxreboot#10420]
- Passkeys: Set RP ID to effective domain if unset instead of returning an error [keepassxreboot#10384]
- Passkeys: Various UI fixes and improvements [keepassxreboot#10427, keepassxreboot#10608, keepassxreboot#10609]
- AppImage: Fix URL opening [keepassxreboot#10624]
- Flatpak: Fix application autostart [keepassxreboot#10563]
- Linux/macOS: Fix button sizes on modal alert popups [keepassxreboot#10500]
- Linux: Fix clipboard clear on Wayland [keepassxreboot#10500]
- Windows: Preserve file-hidden attribute [keepassxreboot#10343]

# -----BEGIN PGP SIGNATURE-----
#
# iQIzBAABCAAdFiEE6A9nU7OzJX8m9B8ILPQdKqhDj5kFAmY4A30ACgkQLPQdKqhD
# j5npgBAApBCGfhdugBE3X9iCkGQ69LKKWizgp44AzmezxU2ee7KEoZgSmZpOCPyO
# bg9EIgwac+3yCh4i4hJrTvnwIemrUKNsNLE18Kn/Uw3HJBCtsb40CeIFcZktOegu
# RQ5G7jhBtnAopnTKQhdwcwJ0Yq6ZSTSiSuo+miDAN22DjnWVd7BLMOioSBPgxFUT
# td+2MAPeydLoMdFRmkuBaDSStLWThdCz6DrWcBYQSK2b6Mu+3mzmtE24zNM1jCKu
# Tl0t6fRkOhqWSRyWBSMzIH3uMuV95yQNudjDMnuOVWVE9Ai+A1RPFHtf8Zj1ydh9
# n9JGPDyloWRcYQdDBgbn6lFHWnwSaYVCRpRPPmjpmXVwt5/AdtB8wN+6uGbcYTzw
# u9l0YYWx84W0kNPkJ0ZejF33qioQ7FaZruJv2ej++NtO0FJP48UVyrQ4EMG6V+17
# AcQ0aoSWWTb5AYhJXLjImDG7DNY1mbgW6deJLKVS7pkoRke1uSLGqYTUAJCFaXnq
# d9uZt4HRUUMeq6x8dvFNvIcZhsfRUaO/iXjp81nl8hlWIeTYNTj22eww3yapFs+S
# cdmdCmfGZAx5FWCXaszXwD3gLF8Bg6S63l9TvbjEHGR2riYKOO1IbFz8JXXjWpdN
# l4SIcWJfdO2mNz0MWfzNtmMYNu9LBfU2Hod5JHJQYiQh3dh4EG4=
# =MrBi
# -----END PGP SIGNATURE-----
# gpg: directory '/home/runner/.gnupg' created
# gpg: keybox '/home/runner/.gnupg/pubring.kbx' created
# gpg: Signature made Sun May  5 22:09:01 2024 UTC
# gpg:                using RSA key E80F6753B3B3257F26F41F082CF41D2AA8438F99
# gpg: Can't check signature: No public key
@MikaHD666
Copy link

Just saw that this feature was added in with the latest update. Is it possible to merge two already existing entries (one with passkey, one with pw and otp)? Not sure if i need to remake each Passkey to merge them with this new dialogue.

@droidmonkey
Copy link
Member

You need to export the passkey from one entry then import it to the other. Or you can copy paste the advanced attributes. Your easiest method is to migrate the non-passkey entry data to the passkey one.

@phoerious phoerious added pr: new feature Pull request that adds a new feature and removed new feature labels Nov 22, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature: Passkeys pr: backported Pull request backported to previous release pr: new feature Pull request that adds a new feature
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Allow newly created Passkeys to be added to an existing entry
6 participants