fix: certain YubiKeyDevice properties were not being updated in the YubiKeyDevice

[DennisDyallo]

This pull request improves the YubiKey device API by ensuring that device properties are updated immediately after configuration changes, and strengthens integration tests to verify this behavior. It also enhances documentation for several methods, clarifying property update timing and requirements.

Immediate Property Update After Configuration Changes

• Updated YubiKeyDevice.Instance.cs so that properties like ChallengeResponseTimeout, AutoEjectTimeout, IsNfcRestricted, DeviceFlags, and ConfigurationLocked are set immediately after their respective configuration methods are called, ensuring the API reflects the latest device state without requiring a refresh. [1] [2] [3] [4] [5] [6]

API Documentation Improvements

• Enhanced XML documentation in IYubiKeyDevice.cs to clearly state that properties are updated immediately after configuration changes, and to specify requirements and exceptions for each method. [1] [2] [3] [4] [5] [6]

Integration Test Strengthening

• Refactored integration tests in YubiKeyDeviceTests.cs to use [SkippableFact] and [SkippableTheory] for better error handling when devices are not found, and replaced the device retrieval helper with a more robust method. [1] [2] [3] [4] [5] [6]
• Added new tests to verify that properties (IsNfcRestricted, DeviceFlags, AutoEjectTimeout, ChallengeResponseTimeout, ConfigurationLocked) are updated immediately after configuration changes, addressing GitHub Issue [BUG] IsNfcRestricted needs to be invalidated #192.

Test Code Refactoring

• Simplified device retrieval logic and removed the old TryGetSkyDevice pattern in favor of a direct method that throws if no device is found, improving test reliability and clarity.

Limitations

This fix only applies to the mentioned properties. Setting different YubiKeyDevice Capabilities (USB, NFC over different applications) will not be affected by this fix and would require a re-enumeration of the device.

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: Arch Linux
• Firmware version: 5.7.2
• Yubikey model¹: 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) ↩

[Copilot]

Pull Request Overview

This PR fixes a bug where device configuration properties were not updated immediately after being changed via API calls, requiring a device re-enumeration to see the changes. The fix updates the in-memory cached properties immediately after successful configuration commands. Additionally, the PR strengthens integration tests to verify this immediate update behavior and improves API documentation to clearly describe when properties are updated.

• Implemented immediate property updates in the device API after configuration changes
• Added comprehensive integration tests verifying immediate property updates for all affected properties
• Enhanced XML documentation to clarify property update timing and requirements

Reviewed Changes

Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.

File	Description
Yubico.YubiKey/src/Yubico/YubiKey/YubiKeyDevice.Instance.cs	Added immediate property updates after successful configuration commands for ChallengeResponseTimeout, AutoEjectTimeout, IsNfcRestricted, DeviceFlags, and ConfigurationLocked
Yubico.YubiKey/src/Yubico/YubiKey/IYubiKeyDevice.cs	Enhanced XML documentation to specify that properties update immediately after configuration changes and clarified method requirements
Yubico.YubiKey/tests/integration/Yubico/YubiKey/YubiKeyDeviceTests.cs	Refactored existing tests to use SkippableFact/SkippableTheory and added new tests verifying immediate property updates; replaced TryGetSkyDevice with GetSkyDevice for clearer error handling

[DennisDyallo]

@claude Can you address the failing build regarding XML docs?

[claude]

Claude finished @DennisDyallo's task —— View job

---

Fixed XML Documentation Build Errors ✓

I've successfully fixed the XML documentation errors that were causing the build to fail.

Tasks

• Check CI build status to identify specific XML doc errors
• Review the changed files for documentation issues
• Fix XML documentation errors
• Address review comment about "unrestrict" terminology (already fixed)
• Commit and push fixes

Issues Fixed

The build was failing due to unresolved cref attributes in XML documentation comments in IYubiKeyDevice.cs. These properties are defined in the parent interface IYubiKeyDeviceInfo, so they needed to be fully qualified:

• Line 404: ChallengeResponseTimeout → IYubiKeyDeviceInfo.ChallengeResponseTimeout
• Line 437: AutoEjectTimeout → IYubiKeyDeviceInfo.AutoEjectTimeout
• Lines 510 & 549: ConfigurationLocked → IYubiKeyDeviceInfo.ConfigurationLocked (2 occurrences)
• Line 664: IsNfcRestricted → IYubiKeyDeviceInfo.IsNfcRestricted

The review comment about "unrestrict" vs "allow NFC" terminology was already addressed in the existing code.

Changes Committed

The fixes have been committed and pushed to the dennisdyallo/fix-device-props branch. The build should now pass without XML documentation errors.

--- | Branch: dennisdyallo/fix-device-props

[github-actions]

Test Results: Windows

    2 files      2 suites   16s ⏱️
4 027 tests 4 008 ✅ 19 💤 0 ❌
4 029 runs  4 010 ✅ 19 💤 0 ❌

Results for commit cdb58df.

[github-actions]

Test Results: Ubuntu

    2 files      2 suites   48s ⏱️
4 019 tests 4 000 ✅ 19 💤 0 ❌
4 021 runs  4 002 ✅ 19 💤 0 ❌

Results for commit cdb58df.

[github-actions]

Package	Line Rate	Branch Rate	Complexity	Health
Yubico.Core	45%	35%	4557	➖
Yubico.YubiKey	51%	47%	21147	➖
Summary	50% (37245 / 74841)	45% (9080 / 20325)	25704	➖

Minimum allowed line rate is 40%

[github-actions]

Test Results: MacOS

    4 files      4 suites   32s ⏱️
4 001 tests 4 001 ✅ 0 💤 0 ❌
4 003 runs  4 003 ✅ 0 💤 0 ❌

Results for commit cdb58df.

[DennisDyallo]

@claude What's your take on this PR?

[claude]

Claude finished @DennisDyallo's task —— View job

---

Code Review: Property Update Implementation

I've reviewed the PR changes that fix the issue where certain YubiKeyDevice properties weren't being updated immediately after configuration changes. Here's my comprehensive assessment:

---

Overall Assessment: Strong Implementation ✓

This PR addresses GitHub Issue #192 effectively by ensuring device properties reflect their new values immediately after configuration changes, without requiring device re-enumeration. The implementation is clean, well-tested, and properly documented.

---

What Works Well

1. Correct Implementation Pattern ✓

The property update pattern is consistent and well-placed across all configuration methods in YubiKeyDevice.Instance.cs:

// Lines 377-381, 405-408, 428-431, 449-452, 486-489, 518-521
if (_yubiKeyDeviceInfo is YubiKeyDeviceInfo deviceInfo)
{
    deviceInfo.PropertyName = newValue;
}

This pattern:

• Updates properties only after successful command execution
• Uses safe type checking before casting
• Maintains consistency across all methods
• Ensures the cached state matches the actual device state

2. Comprehensive Test Coverage ✓

The new integration tests (YubiKeyDeviceTests.cs:140-263) thoroughly verify immediate property updates for all affected properties:

• SetIsNfcRestricted_UpdatesPropertyImmediately
• SetDeviceFlags_UpdatesPropertyImmediately
• SetAutoEjectTimeout_UpdatesPropertyImmediately
• SetChallengeResponseTimeout_UpdatesPropertyImmediately
• LockUnlockConfiguration_UpdatesPropertyImmediately

Each test follows the pattern: get initial value → change it → verify immediate update → restore → verify restoration.

3. Improved Documentation ✓

The XML documentation updates in IYubiKeyDevice.cs clearly communicate the immediate update behavior:

• Lines 403-408: Explicitly states ChallengeResponseTimeout updates immediately
• Lines 436-440: Same clarity for AutoEjectTimeout
• Lines 469-474, 509-510, 548-549: Consistent messaging throughout

4. Better Test Infrastructure ✓

The refactoring from TryGetSkyDevice pattern to GetSkyDevice() with [SkippableFact]/[SkippableTheory] attributes is an improvement:

• More explicit error handling when devices aren't available
• Cleaner test code with less boilerplate
• Better test failure messages

---

Potential Concerns & Questions

1. Thread Safety Consideration ⚠️

File: YubiKeyDevice.Instance.cs

The property updates modify _yubiKeyDeviceInfo without synchronization. While this is likely fine for typical single-threaded usage:

// Lines 377-381
if (_yubiKeyDeviceInfo is YubiKeyDeviceInfo deviceInfo)
{
    deviceInfo.ChallengeResponseTimeout = (byte)seconds;
}

Question: Are IYubiKeyDevice instances intended to be thread-safe? If multiple threads could call configuration methods simultaneously, these updates could have race conditions. Consider documenting thread-safety expectations or adding synchronization if needed.

2. Partial Update on Multi-Property Methods 📝

File: YubiKeyDevice.Instance.cs:318-333, 336-355

Methods like SetEnabledNfcCapabilities and SetEnabledUsbCapabilities don't update their cached properties because they trigger a device reboot (ResetAfterConfig = true).

Observation: This is correct behavior since the object becomes stale after reboot (as documented). However, it creates an asymmetry:

• Properties that don't require reboot: updated immediately ✓
• Properties that do require reboot: not updated (object becomes stale)

This is fine, but worth noting the design is intentional and documented.

3. Missing Property Update in Legacy Method? 🤔

File: YubiKeyDevice.Instance.cs:525-610

The SetLegacyDeviceConfiguration method doesn't update cached properties even though it modifies:

• EnabledUsbCapabilities
• ChallengeResponseTimeout
• DeviceFlags
• AutoEjectTimeout

Question: Should this method also update the cached properties for consistency? Or is the reboot requirement (for firmware < 5) the reason it's omitted? The comment at line 531 mentions older YubiKeys require power cycles, which might make the object stale regardless.

Recommendation: Consider adding a comment explaining why properties aren't updated in this method to maintain clarity.

4. Type Check Pattern 💡

File: YubiKeyDevice.Instance.cs

The pattern if (_yubiKeyDeviceInfo is YubiKeyDeviceInfo deviceInfo) is used consistently, which is good. However:

Question: Under what circumstances would _yubiKeyDeviceInfo not be of type YubiKeyDeviceInfo? Looking at the constructors (lines 160-204), it seems like _yubiKeyDeviceInfo is always assigned from parameters of type IYubiKeyDeviceInfo.

Suggestion: If this type check is defensive programming, that's fine. If there are scenarios where it could be a different implementation, consider documenting them.

---

Code Quality Observations

✓ Strengths:

1. Error handling preserved - Property updates occur after successful response checks
2. No breaking changes - Public API remains unchanged
3. Consistent code style - Matches existing codebase patterns
4. Good test isolation - Each test is independent and properly cleans up
5. Documentation accuracy - XML docs accurately describe new behavior

📋 Minor Suggestions:

1. Line 673 in IYubiKeyDevice.cs:

   /// <param name="enabled">Set this value to true to restrict NFC, false to allow NFC.</param>

   The earlier review comment about "unrestrict" has been addressed. Good work! ✓

2. Test device requirements:
   The new tests use StandardTestDevice.Fw5 and StandardTestDevice.Fw5Fips, which is appropriate since these features require firmware ≥ 5.0.0. This matches the XML documentation requirements.

---

Security & Correctness

✓ No Security Issues Identified

• Property updates don't expose sensitive data
• Lock/unlock operations still require correct lock codes
• No bypass of existing security mechanisms

✓ Correctness Verified

• Properties updated only after successful command execution (checking response.Status)
• Exception handling preserved from original implementation
• Integration tests verify the fix works with real hardware

---

Documentation Alignment

The PR description states:

"This fix only applies to the mentioned properties. Setting different YubiKeyDevice Capabilities (USB, NFC over different applications) will not be affected by this fix and would require a re-enumeration of the device."

This is accurately reflected in the implementation - capability changes (SetEnabledNfcCapabilities, SetEnabledUsbCapabilities) trigger reboots and don't update cached properties, while the other properties are updated immediately.

---

Final Recommendation

Verdict: Approve with Minor Suggestions ✅

This is a solid PR that fixes a real usability issue. The implementation is clean, well-tested, and properly documented. The concerns raised above are mostly questions for clarification rather than blocking issues.

Suggestions for consideration:

1. Document thread-safety expectations for IYubiKeyDevice instances
2. Add a comment in SetLegacyDeviceConfiguration explaining why properties aren't updated
3. Consider documenting when _yubiKeyDeviceInfo might not be YubiKeyDeviceInfo type

Merge readiness: Ready to merge after PR review process completion. All tests pass, code coverage is good (50%, above the 40% minimum), and the implementation solves the stated problem effectively.

---

Great work on this fix! The immediate property updates will significantly improve the developer experience when configuring YubiKey devices. 👍

--- | Branch: dennisdyallo/fix-device-props