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

Show a clear error if the key is found but the slots are not configured #11609

Conversation

w15eacre
Copy link
Contributor

@w15eacre w15eacre commented Dec 29, 2024

  • Fix the error message when a key is connected, but no slots are configured
  • The solution is to check connected devices. If they exist, show the error: "Hardware keys found, but no slots are configured."
  • Fix missing SCardDisconnect
  • Fix outdated test cases

Fixes issue #11543

Screenshots

image

image

Testing strategy

  • Check the fix on YubiKeys
  • Add a unit test to verify the series numbers of connected devices

Type of change

  • ✅ Bug fix (non-breaking change that fixes an issue)

@w15eacre w15eacre force-pushed the feature/11543_show_error_about_slots_state branch 3 times, most recently from d15b262 to d60c589 Compare December 29, 2024 22:26
@droidmonkey droidmonkey added this to the v2.7.10 milestone Dec 30, 2024
@droidmonkey
Copy link
Member

droidmonkey commented Dec 31, 2024

Our minimum version of Qt is 5.12 now so you should be able to use qScopeGuard template function instead of redefining a ScopeGuard. https://doc.qt.io/qt-5/qscopeguard.html#qScopeGuardx

This message should also appear on the database open widget. We can show the same disabled drop down box as the database security view and also disable the checkbox too.

@w15eacre
Copy link
Contributor Author

w15eacre commented Dec 31, 2024

I tried the first time but Ubuntu build failed on CI

@droidmonkey
Copy link
Member

Note the case, qScopeGuard not QScopeGuard

@w15eacre w15eacre marked this pull request as draft January 1, 2025 17:31
@w15eacre w15eacre force-pushed the feature/11543_show_error_about_slots_state branch from 2f7c9de to 5f144ae Compare January 1, 2025 18:35
@w15eacre w15eacre marked this pull request as ready for review January 1, 2025 18:36
@w15eacre
Copy link
Contributor Author

w15eacre commented Jan 1, 2025

@droidmonkey Thanks for your review; I have fixed it.
In my opinion, If I added a new item in the combobox I would refactor the noHardwareKeysFoundLabel and move it into this combobox. This behavior would be the same as in the Database Security view.

@droidmonkey
Copy link
Member

droidmonkey commented Jan 2, 2025

I don't like that the keys are basically polled twice just to get an error message. Instead, I tweaked the findValidKeys function to include a reference parameter that populates the number of keys actually found. This is then stored in the Yubikey class for use in the new connectedKeys function. This actually unwinds a lot of the new code you provided, but is way more elegant.

What do you think?

@droidmonkey droidmonkey force-pushed the feature/11543_show_error_about_slots_state branch from 5f144ae to 53b5327 Compare January 2, 2025 04:11
* Also fix delayed polling on window activation
@droidmonkey droidmonkey force-pushed the feature/11543_show_error_about_slots_state branch from 53b5327 to c41f27b Compare January 2, 2025 04:16
@w15eacre
Copy link
Contributor Author

w15eacre commented Jan 2, 2025

Yes, it's better and safer. Besides, polling twice caused an issue with the calling function that could freeze the UI thread. I suggest replacing:

  • int& → unsigned int8_t& or size_t&. This value is in the range [0, 4).
  • int Delay → std::chrono::milliseconds for a clearer understanding of this interval.

@droidmonkey What do you think about it?

@droidmonkey
Copy link
Member

int& → unsigned int8_t& or size_t&. This value is in the range [0, 4).

The sign of the counter variable is irrelevant in this context and can actually produce underflow errors if you were to code in a -- by accident (ie, instead of -1 you'll get UINT_MAX). Either way this won't happen or matter because that isn't how this variable is handled in any context of the code. Generally speaking, the "only" time you need to use unsigned int is to use the full 32-bit length of an int to represent the numbers you need.

int Delay → std::chrono::milliseconds for a clearer understanding of this interval.

You cannot do this because std::chrono::milliseconds is actually a class and not convertible to an int to be used with Qt. It is not meant to indicate the variable contents by a name.

@droidmonkey
Copy link
Member

Also, I tested this code with my 5 yubikeys and NFC reader, it worked flawlessly and reliably. Merging now.

@droidmonkey droidmonkey merged commit 9e29b5c into keepassxreboot:develop Jan 3, 2025
9 checks passed
droidmonkey added a commit that referenced this pull request Jan 3, 2025
)

Fixes #11543

Also fix delayed polling on window activation

---------

Co-authored-by: w15dev <[email protected]>
Co-authored-by: Jonathan White <[email protected]>
@droidmonkey droidmonkey added the pr: backported Pull request backported to previous release label Jan 19, 2025
pull bot pushed a commit to Graysonbarton/keepassxc that referenced this pull request Jan 26, 2025
…passxreboot#11609)

Fixes keepassxreboot#11543

Also fix delayed polling on window activation

---------

Co-authored-by: w15dev <[email protected]>
Co-authored-by: Jonathan White <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature: Hardware Keys pr: backported Pull request backported to previous release ux
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants