Skip to content

Commit 2bac238

Browse files
DennisDyalloDennisDyallo
committed
refactor: Refactored OtpErrorTransform and tests for readability and simplicity
1 parent 4f77bb3 commit 2bac238

File tree

2 files changed

+105
-70
lines changed

2 files changed

+105
-70
lines changed

‎Yubico.YubiKey/src/Yubico/YubiKey/Pipelines/OtpErrorTransform.cs

Lines changed: 54 additions & 30 deletions
Original file line numberDiff line numberDiff line change
@@ -38,56 +38,80 @@ public ResponseApdu Invoke(CommandApdu command, Type commandType, Type responseT
3838
{
3939
// If this is just a regular ReadStatusCommand, or it's a command that doesn't ask for a status response
4040
// in return, invoke the pipeline as usual.
41-
if (commandType == typeof(ReadStatusCommand)
42-
|| responseType != typeof(ReadStatusResponse))
41+
if (commandType == typeof(ReadStatusCommand) ||
42+
responseType != typeof(ReadStatusResponse))
4343
{
44-
return _nextTransform.Invoke(command, commandType, responseType);
44+
return InvokeCommand(command, commandType, responseType);
4545
}
4646

4747
// Otherwise we assume this to be a command that applies a config (and therefore looks for a status response).
48-
// In order to detect failures, we grab the status structure before applying said command so that we have a
49-
// sequence number to compare to.
50-
int beforeSequence = new ReadStatusResponse(
51-
_nextTransform.Invoke(
52-
new ReadStatusCommand().CreateCommandApdu(),
53-
typeof(ReadStatusCommand),
54-
typeof(ReadStatusResponse)))
55-
.GetData()
56-
.SequenceNumber;
57-
5848
try
5949
{
60-
var responseApdu = _nextTransform.Invoke(command, commandType, responseType);
61-
var readStatusResponse = new ReadStatusResponse(responseApdu);
62-
var otpStatus = readStatusResponse.GetData();
63-
int nextSequence = otpStatus.SequenceNumber;
64-
65-
// If we see the sequence number change, we can assume that the configuration was applied successfully. Otherwise
66-
// we just invent an error in the response.
67-
return IsValidSequenceProgression(beforeSequence, nextSequence, otpStatus)
50+
// In order to detect failures, we grab the status structure before applying said command so that we have a
51+
// sequence number to compare to.
52+
var beforeStatus = GetCurrentStatus();
53+
var responseApdu = InvokeCommand(command, commandType, responseType, out var afterStatus);
54+
55+
// If we see the sequence number change, we can assume that the configuration was applied successfully.
56+
// Otherwise we just invent an error in the response.
57+
return IsValidSequenceProgression(beforeStatus, afterStatus)
6858
? responseApdu
69-
: FailedApdu(responseApdu.Data.ToArray());
59+
: CreateFailedApdu(responseApdu.Data.ToArray());
7060
}
7161
catch (KeyboardConnectionException e)
7262
{
7363
_logger.LogWarning(e, "Handling keyboard connection exception. Translating to APDU response.");
7464

75-
return FailedApdu([]);
65+
return CreateFailedApdu([]);
7666
}
77-
78-
static ResponseApdu FailedApdu(byte[] data) => new(data, SWConstants.WarningNvmUnchanged);
7967
}
8068

8169
public void Setup() => _nextTransform.Setup();
8270

8371
public void Cleanup() => _nextTransform.Cleanup();
84-
85-
internal static bool IsValidSequenceProgression(int beforeSequence, int nextSequence, OtpStatus afterStatus)
72+
73+
// Internal for testing
74+
internal static bool IsValidSequenceProgression(OtpStatus beforeStatus, OtpStatus afterStatus)
75+
{
76+
byte before = beforeStatus.SequenceNumber;
77+
byte after = afterStatus.SequenceNumber;
78+
79+
bool normalIncrement = after == before + 1;
80+
bool validReset = before > 0 && after == 0 &&
81+
afterStatus is { LongPressConfigured: false, ShortPressConfigured: false };
82+
83+
return normalIncrement || validReset;
84+
}
85+
86+
private ResponseApdu InvokeCommand(
87+
CommandApdu commandApdu,
88+
Type commandType,
89+
Type responseType,
90+
out OtpStatus afterStatus)
8691
{
87-
const int configStatusMask = 0x1F;
92+
var responseApdu = _nextTransform.Invoke(commandApdu, commandType, responseType);
93+
afterStatus = ReadStatus(responseApdu);
94+
return responseApdu;
95+
}
96+
97+
private ResponseApdu InvokeCommand(CommandApdu commandApdu, Type commandType, Type responseType) =>
98+
_nextTransform.Invoke(commandApdu, commandType, responseType);
8899

89-
return nextSequence == beforeSequence + 1 || // Normal increment
90-
(beforeSequence > 0 && nextSequence == 0 && (afterStatus.TouchLevel & configStatusMask) == 0); // When deleting the "last" slot
100+
private OtpStatus GetCurrentStatus() =>
101+
new ReadStatusResponse(
102+
_nextTransform.Invoke(
103+
new ReadStatusCommand().CreateCommandApdu(),
104+
typeof(ReadStatusCommand),
105+
typeof(ReadStatusResponse)))
106+
.GetData();
107+
108+
private static OtpStatus ReadStatus(ResponseApdu responseApdu)
109+
{
110+
var readStatusResponse = new ReadStatusResponse(responseApdu);
111+
var afterStatus = readStatusResponse.GetData();
112+
return afterStatus;
91113
}
114+
115+
private static ResponseApdu CreateFailedApdu(byte[] data) => new(data, SWConstants.WarningNvmUnchanged);
92116
}
93117
}

‎Yubico.YubiKey/tests/unit/Yubico/YubiKey/Pipelines/OtpErrorTransformTests.cs

Lines changed: 51 additions & 40 deletions
Original file line numberDiff line numberDiff line change
@@ -6,67 +6,78 @@ namespace Yubico.YubiKey.Pipelines;
66
public class OtpErrorTransformTests
77
{
88
[Theory]
9-
[InlineData(0, 1)] // First increment
10-
[InlineData(5, 6)] // Normal increment
11-
[InlineData(254, 255)] // Near byte boundary
12-
[InlineData(257, 258)] // Near byte boundary
13-
14-
public void IsValidSequenceProgression_NormalIncrement_ReturnsTrue(int before, int after)
9+
[InlineData(0, 1)] // First increment
10+
[InlineData(5, 6)] // Normal increment
11+
[InlineData(254, 255)] // Near byte boundary
12+
public void IsValidSequenceProgression_NormalIncrement_ReturnsTrue(
13+
byte before,
14+
byte after)
1515
{
16-
var status = CreateOtpStatus(touchLevel: 0xFF); // TouchLevel irrelevant
17-
18-
bool isValid = OtpErrorTransform.IsValidSequenceProgression(before, after, status);
19-
16+
var beforeStatus = CreateOtpStatus(before, hasConfigs: true);
17+
var afterStatus = CreateOtpStatus(after, hasConfigs: true);
18+
19+
var isValid = OtpErrorTransform.IsValidSequenceProgression(beforeStatus, afterStatus);
20+
2021
Assert.True(isValid);
2122
}
2223

2324
[Theory]
24-
[InlineData(0, 2)] // Skip increment
25-
[InlineData(5, 7)] // Jump by 2
26-
[InlineData(10, 8)] // Backwards
27-
public void IsValidSequenceProgression_InvalidIncrement_ReturnsFalse(int before, int after)
25+
[InlineData(0, 2)] // Skip increment
26+
[InlineData(5, 7)] // Jump by 2
27+
[InlineData(10, 8)] // Backwards
28+
[InlineData(255, 0)] // Byte overflow (should be rejected as normal increment)
29+
public void IsValidSequenceProgression_InvalidIncrement_ReturnsFalse(
30+
byte before,
31+
byte after)
2832
{
29-
var status = CreateOtpStatus(touchLevel: 0x00); // TouchLevel irrelevant
30-
31-
bool isValid = OtpErrorTransform.IsValidSequenceProgression(before, after, status);
32-
33+
var beforeStatus = CreateOtpStatus(before, hasConfigs: true);
34+
var afterStatus = CreateOtpStatus(after, hasConfigs: true); // Still has configs
35+
36+
var isValid = OtpErrorTransform.IsValidSequenceProgression(beforeStatus, afterStatus);
37+
3338
Assert.False(isValid);
3439
}
3540

3641
[Theory]
37-
[InlineData(1, 0x00)] // Config bits clear
38-
[InlineData(5, 0x20)] // Upper bits set, config clear
39-
[InlineData(255, 0xE0)] // Multiple upper bits, config clear
40-
public void IsValidSequenceProgression_ValidReset_ReturnsTrue(int before, byte touchLevel)
42+
[InlineData(1)] // Delete from sequence 1
43+
[InlineData(5)] // Delete from sequence 5
44+
[InlineData(255)] // Delete from sequence 255
45+
public void IsValidSequenceProgression_ValidReset_ReturnsTrue(
46+
byte before)
4147
{
42-
var status = CreateOtpStatus(touchLevel);
43-
44-
bool isValid = OtpErrorTransform.IsValidSequenceProgression(before, 0, status);
45-
48+
var beforeStatus = CreateOtpStatus(before, hasConfigs: true);
49+
var afterStatus = CreateOtpStatus(0, hasConfigs: false); // No configs remain
50+
51+
var isValid = OtpErrorTransform.IsValidSequenceProgression(beforeStatus, afterStatus);
52+
4653
Assert.True(isValid);
4754
}
4855

4956
[Theory]
50-
[InlineData(0, 0x00)] // Already zero (invalid before state)
51-
[InlineData(1, 0x01)] // Config bit set
52-
[InlineData(5, 0x1F)] // All config bits set
53-
[InlineData(5, 0x21)] // Config + upper bits set
54-
public void IsValidSequenceProgression_InvalidReset_ReturnsFalse(int before, byte touchLevel)
57+
[InlineData(0, false)] // Already zero
58+
[InlineData(1, true)] // Reset but configs still exist
59+
[InlineData(5, true)] // Reset but configs still exist
60+
public void IsValidSequenceProgression_InvalidReset_ReturnsFalse(
61+
byte before,
62+
bool afterHasConfigs)
5563
{
56-
var status = CreateOtpStatus(touchLevel);
57-
58-
bool isValid = OtpErrorTransform.IsValidSequenceProgression(before, 0, status);
59-
64+
var beforeStatus = CreateOtpStatus(before, hasConfigs: true);
65+
var afterStatus = CreateOtpStatus(0, hasConfigs: afterHasConfigs);
66+
67+
var isValid = OtpErrorTransform.IsValidSequenceProgression(beforeStatus, afterStatus);
68+
6069
Assert.False(isValid);
6170
}
6271

63-
private static OtpStatus CreateOtpStatus(byte touchLevel) => new()
72+
private static OtpStatus CreateOtpStatus(
73+
byte sequenceNumber,
74+
bool hasConfigs) => new()
6475
{
6576
FirmwareVersion = new FirmwareVersion(),
66-
SequenceNumber = 0,
67-
TouchLevel = touchLevel,
68-
ShortPressConfigured = false,
69-
LongPressConfigured = false,
77+
SequenceNumber = sequenceNumber,
78+
TouchLevel = 0x00,
79+
ShortPressConfigured = hasConfigs,
80+
LongPressConfigured = false, // Only test one slot for simplicity
7081
ShortPressRequiresTouch = false,
7182
LongPressRequiresTouch = false,
7283
LedBehaviorInverted = false

0 commit comments

Comments
 (0)