Skip to content

Conversation

DennisDyallo
Copy link
Collaborator

Description

This pull request introduces significant improvements to device enumeration and error handling in the Windows HID device listener and the CmDevice class. The main changes involve making device creation more robust against transient errors, introducing factory methods for safer instantiation, and improving logging for diagnostics. Additionally, some code has been refactored for clarity and maintainability.

These changes make device enumeration more robust and the codebase easier to maintain, especially in scenarios where devices are rapidly connected or disconnected.

Fixes: #144

Device creation and error handling improvements:

  • Added static factory methods FromDevicePath and FromDeviceInstance to CmDevice, which safely handle exceptions and log errors when devices disappear or when the Configuration Manager API fails. The constructors are now marked as obsolete and direct usage is discouraged. (Yubico.Core/src/Yubico/PlatformInterop/Windows/Cfgmgr32/CmDevice.cs)
  • Updated device enumeration methods such as GetList, Parent, and Children in CmDevice to use the new factory methods, ensuring that only valid devices are returned and errors are handled gracefully. (Yubico.Core/src/Yubico/PlatformInterop/Windows/Cfgmgr32/CmDevice.cs) [1] [2] [3]

Windows HID device listener changes:

  • Modified WindowsHidDeviceListener to use the new CmDevice.FromDevicePath method when handling device arrival events. If device creation fails, a warning is logged and the event is ignored, preventing crashes during rapid device changes. (Yubico.Core/src/Yubico/Core/Devices/Hid/WindowsHidDeviceListener.cs)

Code refactoring and clarity:

  • Moved property initialization logic in CmDevice to a dedicated InitializeProperties method, improving constructor clarity and maintainability. (Yubico.Core/src/Yubico/PlatformInterop/Windows/Cfgmgr32/CmDevice.cs)
  • Refactored some method signatures and error handling logic for better readability and .NET best practices, such as updating LocateDevNode to be static and accept an explicit instanceId. (Yubico.Core/src/Yubico/PlatformInterop/Windows/Cfgmgr32/CmDevice.cs)

Other minor improvements:

  • Improved logging and error messages throughout the device enumeration and property access code for easier diagnostics. (Yubico.Core/src/Yubico/PlatformInterop/Windows/Cfgmgr32/CmDevice.cs, Yubico.Core/src/Yubico/Core/Devices/Hid/WindowsHidDeviceListener.cs) [1] [2]
  • Minor code cleanup and style improvements in property access helper and event handler parameter formatting. (Yubico.Core/src/Yubico/PlatformInterop/Windows/Cfgmgr32/CmPropertyAccessHelper.cs, Yubico.Core/src/Yubico/Core/Devices/Hid/WindowsHidDeviceListener.cs) [1] [2] [3]

Type of change

  • Refactor (non-breaking change which improves code quality or performance)
  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)
  • This change requires a documentation update

How has this been tested?

Please describe the tests that you ran to verify your changes. Provide instructions so we can reproduce. Please also list any relevant details for your test configuration

Test configuration:

  • OS version: Windows 11
  • Firmware version: 5.4.3
  • Yubikey model1: 5 USB A

Checklist:

  • My code follows the style guidelines of this project
  • I have performed a self-review of my own code
  • I have run dotnet format to format my code
  • I have commented my code, particularly in hard-to-understand areas
  • I have made corresponding changes to the documentation
  • My changes generate no new warnings
  • I have added tests that prove my fix is effective or that my feature works
  • New and existing unit tests pass locally with my changes

Footnotes

  1. See Yubikey models (Multi-protocol, Security Key, FIPS, Bio, YubiHSM, YubiHSM FIPS)

Copy link

Test Results: Windows

    2 files      2 suites   8s ⏱️
3 972 tests 3 972 ✅ 0 💤 0 ❌
3 974 runs  3 974 ✅ 0 💤 0 ❌

Results for commit 5aa3d8d.

Copy link

Test Results: Ubuntu

    2 files      2 suites   19s ⏱️
3 964 tests 3 964 ✅ 0 💤 0 ❌
3 966 runs  3 966 ✅ 0 💤 0 ❌

Results for commit 5aa3d8d.

Copy link

Code Coverage

Package Line Rate Branch Rate Complexity Health
Yubico.Core 40% 31% 4371
Yubico.YubiKey 51% 47% 21102
Summary 49% (36168 / 73728) 44% (8886 / 20103) 25473

Minimum allowed line rate is 40%

Copy link

Test Results: MacOS

    2 files      2 suites   11s ⏱️
3 964 tests 3 964 ✅ 0 💤 0 ❌
3 966 runs  3 966 ✅ 0 💤 0 ❌

Results for commit 5aa3d8d.

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

Successfully merging this pull request may close these issues.

[BUG] KeyNotFoundException in CmDevice causes application to quit when plugging / unplugging Yubikey
1 participant