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.

Sign up for GitHub

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

Jump to bottom

Passkeys: Add support for importing Passkey to entry #9987

Merged
merged 6 commits into from
Nov 23, 2023
Merged

Passkeys: Add support for importing Passkey to entry #9987

merged 6 commits into from
Nov 23, 2023

Conversation

varjolintu
Copy link
Member

@varjolintu varjolintu commented Nov 4, 2023 •
edited
Loading

Adds support for importing Passkeys to entries. Notable changes:

  • New menu item for entry "Import Passkey".
  • Confirm if existing Passkey is being replaced.
  • Import Passkey dialog has an option to select an entry instead of group. The list of entries changes when active database and/or group is changed.
  • Move BrowserService::ADDITIONAL_URL to EntryAttributes::AdditionalUrlAttribute(no longer requires WITH_XC_BROWSER.
  • Handle Passkey's Relying Party as additional URL if Passkey is found.
  • Handle Passkey's Username instead of username if Passkey is found.

Screenshots

Simple "Import Passkey" to entry from the entry menu:
Screenshot 2023-11-04 at 20 32 19
Confirmation for existing Passkey:
Screenshot 2023-11-04 at 20 03 17
New Import Passkey dialog:
Screenshot 2023-11-04 at 23 06 05
Choosing an entry from a group:
Screenshot 2023-11-04 at 23 06 17

Testing strategy

Manually.

Type of change

  • ✅ New feature (change that adds functionality)

@varjolintu varjolintu added this to the v2.8.0 milestone Nov 4, 2023
droidmonkey
droidmonkey requested changes Nov 4, 2023
View reviewed changes
src/browser/BrowserService.cpp Outdated Show resolved Hide resolved
src/gui/DatabaseWidget.h Outdated Show resolved Hide resolved
src/gui/passkeys/PasskeyImportDialog.cpp Show resolved Hide resolved
@droidmonkey
Copy link
Member

droidmonkey commented Nov 4, 2023 •
edited
Loading

The import dialog has become a bit cluttered, I am not sure the layout makes sense.

Database selection should be on top (also should just be a combobox like the other controls), then the next control should be the group selection but the default (first) entry should be "Use Passkey Group". If an actual group is selected then the entry selection enables, the first item is "Create new entry" with the subsequent choices being the entries in the group.

Mockup:

image

image

The question "Do you want to import the passkey?" is a little awkward. Prefer just "Import the following Passkey:"

@varjolintu varjolintu force-pushed the feature/import_passkey_to_entry branch from 8c3fab3 to aa8adb1 Compare November 4, 2023 21:05
@droidmonkey
Copy link
Member

Looks perfect!

@varjolintu varjolintu mentioned this pull request Nov 5, 2023
Merged
@varjolintu
Copy link
Member Author

This change brings a possibility that a Passkey is imported to an entry that has totally different URL from the Passkey's Relying Party. Maybe we need to add an extra check for that property when doing a credential search.

@varjolintu
Copy link
Member Author

With the latest commit:

  • Move BrowserService::ADDITIONAL_URL to EntryAttributes::AdditionalUrlAttribute(no longer requires WITH_XC_BROWSER.
  • Handle Passkey's Relying Party as additional URL if Passkey is found.
  • Handle Passkey's Username instead of username if Passkey is found.

@varjolintu varjolintu mentioned this pull request Nov 6, 2023
Closed
@varjolintu varjolintu changed the title Add support for importing Passkey to entry Passkeys: Add support for importing Passkey to entry Nov 8, 2023
@droidmonkey
Copy link
Member

This doesn't need to be fixed in this PR, but I noticed when trying to import an older exported PassKey I got this error message:

image

It doesn't tell me what data is missing (it was because userId is now credentialId). Might be worthwhile telling the user what data is missing.

@droidmonkey droidmonkey force-pushed the feature/import_passkey_to_entry branch from df36beb to 29dbc3a Compare November 10, 2023 04:24
@varjolintu varjolintu force-pushed the feature/import_passkey_to_entry branch 2 times, most recently from 24bb139 to a1d6c02 Compare November 10, 2023 15:38
@varjolintu
Copy link
Member Author

varjolintu commented Nov 10, 2023 •
edited
Loading

If some data is missing from the JSON file, the popup now looks like this:
Screenshot 2023-11-10 at 17 33 02

Made a new generic function Tools::getMissingValuesFromList() that returns any missing values that are needed. This might be useful elsewhere as well.

@lindhe
Copy link
Contributor

lindhe commented Nov 16, 2023

I love the work you put into this, @varjolintu ! Thank you so much! 💪

@simonsan
Copy link

Same, lots of love from here as well, can't wait to use passkeys with KeePassXC 🚀

@droidmonkey droidmonkey force-pushed the feature/import_passkey_to_entry branch from 91b8a1a to 48c9d2a Compare November 23, 2023 03:45
@droidmonkey droidmonkey merged commit 13c88e1 into keepassxreboot:develop Nov 23, 2023
10 of 11 checks passed
@simonsan
Copy link

This is huge, thank you! 💐

@varjolintu varjolintu deleted the feature/import_passkey_to_entry branch November 23, 2023 04:49
@droidmonkey droidmonkey added the pr: backported Pull request backported to previous release label Jan 12, 2024
droidmonkey added a commit that referenced this pull request Jan 13, 2024
@droidmonkey
Passkeys: Add support for importing Passkey to entry (#9987)
641eb1a 
---------
Co-authored-by: Jonathan White <[email protected]>
droidmonkey added a commit that referenced this pull request Jan 27, 2024
@droidmonkey
Passkeys: Add support for importing Passkey to entry (#9987)
0aacdf4 
---------
Co-authored-by: Jonathan White <[email protected]>
droidmonkey added a commit that referenced this pull request Jan 27, 2024
@droidmonkey
Passkeys: Add support for importing Passkey to entry (#9987)
c5af35f 
---------
Co-authored-by: Jonathan White <[email protected]>
droidmonkey added a commit that referenced this pull request Jan 30, 2024
@droidmonkey
Passkeys: Add support for importing Passkey to entry (#9987)
c477f43 
---------
Co-authored-by: Jonathan White <[email protected]>
libf-de pushed a commit to libf-de/keepassxc-secretservice-dbus that referenced this pull request Mar 11, 2024
@release-bot
Merge tag 'tags/2.7.7' into proposed-2.7.7
a04ec08 
Release 2.7.7

- Support USB Hotplug for Hardware Key interface [keepassxreboot#10092]
- Support 1PUX and Bitwarden import [keepassxreboot#9815]
- Browser: Add support for PassKeys [keepassxreboot#8825, keepassxreboot#9987, keepassxreboot#10318]
- Build System: Move to vcpkg manifest mode [keepassxreboot#10088]

- Fix multiple TOTP issues [keepassxreboot#9874]
- Fix focus loss on save when the editor is not visible anymore [keepassxreboot#10075]
- Fix visual when removing entry from history [keepassxreboot#9947]
- Fix first entry is not selected when a search is performed [keepassxreboot#9868]
- Prevent scrollbars on entry drag/drop [keepassxreboot#9747]
- Prevent duplicate characters in "Also choose from" field of password generator  [keepassxreboot#9803]
- Security: Prevent byte-by-byte and attachment inference side channel attacks [keepassxreboot#10266]
- Browser: Fix raising Update Entry messagebox [keepassxreboot#9853]
- Browser: Fix bugs when returning credentials [keepassxreboot#9136]
- Browser: Fix crash on database open from browser [keepassxreboot#9939]
- Browser: Fix support for referenced URL fields [keepassxreboot#8788]
- MacOS: Fix crash when changing highlight/accent color [keepassxreboot#10348]
- MacOS: Fix TouchID appearing even though lid is closed [keepassxreboot#10092]
- Windows: Fix terminating KeePassXC processes with MSI installer [keepassxreboot#9822]
- FdoSecrets: Fix database merge crash when enabled [keepassxreboot#10136]

# -----BEGIN PGP SIGNATURE-----
#
# iQEzBAABCAAdFiEENIkEDB8MPuq41ValRA/GXy4MbgEFAmXs7VsACgkQRA/GXy4M
# bgHLpwf/brnyPPs3gJxZmD2pn8542D4CCsDh0fTceurOtqCe3J4Y+Fftc5euuoQu
# 6rP4vJdd586l7JX5FnYIPXvGiU9op3MudJh+y+RN/PWwKcXNIXfUItMhpZEka49n
# xnw+Wvbilg1QIHSSmZdIjBpohnEkA67qhWauc3bCacrRyEvIOzVMTxnqDTe4GUDy
# CyauaRMMKezRTpLxSsk63TDAZZgDwK4ci5lC6ysHekc1Za6IbI3fMFjz1BGj+kPU
# tMHMfDCWqK/5JZ27ZWcxy7m8tJY9m3rb+MoCyFRQz9ixaEe29yf5NqYdm9sn1Dlh
# O7aFi7/EJtsBlXdguw5BcTPbsL7XEQ==
# =Cots
# -----END PGP SIGNATURE-----
# gpg: directory '/home/runner/.gnupg' created
# gpg: keybox '/home/runner/.gnupg/pubring.kbx' created
# gpg: Signature made Sat Mar  9 23:14:35 2024 UTC
# gpg:                using RSA key 3489040C1F0C3EEAB8D556A5440FC65F2E0C6E01
# gpg: Can't check signature: No public key
@haldi4803
Copy link

haldi4803 commented Mar 13, 2024 •
edited
Loading

If you click on the "import Passkey" button while "Root" is selected and not a valid folder nothing happens.
All other options in "Entries" are greyed out, except "import Passkey".
That should also be greyed out right?

v2.7.7 Release.

@droidmonkey
Copy link
Member

Yes it should be grayed out. I assume you mean there are no entries in the root folder.

@varjolintu varjolintu mentioned this pull request Mar 13, 2024
Merged
@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
Reviewers

@droidmonkey droidmonkey droidmonkey approved these changes

@phoerious phoerious Awaiting requested review from phoerious

Assignees
No one assigned
Labels
feature: Passkeys pr: backported Pull request backported to previous release pr: new feature Pull request that adds a new feature
Projects
None yet
Milestone
v2.8.0
Development

Successfully merging this pull request may close these issues.
6 participants
@varjolintu @droidmonkey @lindhe @simonsan @haldi4803 @phoerious