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

Add methods isHardwareKeySupported and refreshHardwareKeys to DBus #8055

Merged

Conversation

Zocker1999NET
Copy link
Contributor

Refs #8017 (is not a direct fix but allows an "improved workaround" which should be enough for most use cases)

It adds two methods to the DBus interface called isHardwareKeySupported and refreshHardwareKeys. The first one reflects if KeePassXC was built with hardware key support and can be used to verify that calling the second method (or future methods) makes sense. The second one allows one to force KeePassXC to refresh the list of detected hardware keys (similar to clicking on "Refresh" in the UI). This enables 3rd-party services to e.g. inform KeePassXC that a YubiKey was inserted and, like described in #8017 (comment), allows 3rd-party applications to unlock a database which is secured with a Yubikey (semi-)automatically (given the key inserted was remembered to be last used for this database).

Screenshots

feature not really visible in UI

Testing strategy

  • tested manually by
    • calling isHardwareKeySupported with and without hardware key support and checking return value
    • inserting & removing YubiKey multiple times, calling refresh via DBus and verify in UI that KeePassXC detects inserted key / key was missing again and selected key automatically if recently used
  • documentated and commented new functions properly
  • did not add any unit tests because
    • testing isHardwareKeySupported properly without rebuilding not possible and unit test would depend on same code
    • I really have no good idea how to test refreshHardwareKeys using unit tests, ideas are appreciated

Type of change

  • ✅ New feature (change that adds functionality)

@codecov-commenter
Copy link

codecov-commenter commented May 14, 2022

Codecov Report

Merging #8055 (93753b9) into develop (a4d4adb) will decrease coverage by 0.02%.
The diff coverage is 0.00%.

@@             Coverage Diff             @@
##           develop    #8055      +/-   ##
===========================================
- Coverage    64.29%   64.28%   -0.02%     
===========================================
  Files          339      339              
  Lines        43431    43443      +12     
===========================================
  Hits         27923    27923              
- Misses       15508    15520      +12     
Impacted Files Coverage Δ
src/gui/MainWindow.cpp 72.03% <0.00%> (-0.65%) ⬇️
src/core/FileWatcher.cpp 86.75% <0.00%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update a4d4adb...93753b9. Read the comment docs.

@droidmonkey
Copy link
Member

droidmonkey commented May 14, 2022

You don't need the issupported call because you return false if it's not in the refresh call. There is no circumstance where you would call issupported and NOT call refresh right afterwards if it is supported. Might as well collapse them.

@Zocker1999NET
Copy link
Contributor Author

isSupported can help with deducting what the error was. Refresh will also return false if no devices were found. Instead of calling them in the order you suggested, if you call them as described below, isSupported serves the purpose to detuct what the error was:

if refresh():
  print("Devices were found successfully")
else:
  if isSupported():
    print("User, please insert your Yubikey")
  else:
    print("Oh, you seem to use a KeePassXC version which does not support Yubikeys. You might need to reinstall another KeePassXC variant")

@droidmonkey droidmonkey merged commit de3d40b into keepassxreboot:develop Jun 11, 2022
@Zocker1999NET Zocker1999NET deleted the feature/dbus-hardwarekey branch June 11, 2022 22:03
@michaelk83
Copy link

FYI, this is missing an update in the Wiki.

@droidmonkey
Copy link
Member

I added them

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants