Skip to content

Commit 5bceb93

Browse files
committed
Fix parsing SupportedCtapOptions from empty JSON object
This commit was constructed through a series of steps: Step 1: Add the new test "SupportedCtapOptions can be parsed from an empty JSON object". It initially fails with: ``` true was not false ScalaTestFailureLocation: com.yubico.fido.metadata.MetadataBlobSpec at (MetadataBlobSpec.scala:68) org.scalatest.exceptions.TestFailedException: true was not false at org.scalatest.matchers.MatchersHelper$.indicateFailure(MatchersHelper.scala:392) at org.scalatest.matchers.should.Matchers$ShouldMethodHelperClass.shouldMatcher(Matchers.scala:7304) at org.scalatest.matchers.should.Matchers$AnyShouldWrapper.should(Matchers.scala:7347) at com.yubico.fido.metadata.MetadataBlobSpec.$anonfun$new$5(MetadataBlobSpec.scala:68) ``` At the line: ``` options.isUv should be(false) ``` This is because `SupportedCtapOptions` is still annotated with `@Jacksonized`, so Jackson uses the builder to construct the object, and the class has ``` @Builder.Default boolean uv = false; ``` so the builder initializes the builder field to the primitive `false` value, which causes the constructor argument to be `Boolean.FALSE`, which is not null, and therefore the constructor sets `this.uv` to `true`. Step 2: Remove `@Jacksonized` from `SupportedCtapOptions`. This causes Jackson to invoke the constructor directly instead of going through the builder, which bypasses the builder field defaults. The test "SupportedCtapOptions can be parsed from an empty JSON object" now fails with a different cause: ``` Cannot construct instance of `com.yubico.fido.metadata.SupportedCtapOptions`, problem: `java.lang.NullPointerException` at [Source: REDACTED (`StreamReadFeature.INCLUDE_SOURCE_IN_LOCATION` disabled); line: 1, column: 2] com.fasterxml.jackson.databind.exc.ValueInstantiationException: Cannot construct instance of `com.yubico.fido.metadata.SupportedCtapOptions`, problem: `java.lang.NullPointerException` at [Source: REDACTED (`StreamReadFeature.INCLUDE_SOURCE_IN_LOCATION` disabled); line: 1, column: 2] at com.fasterxml.jackson.databind.exc.ValueInstantiationException.from(ValueInstantiationException.java:46) [...] Caused by: java.lang.NullPointerException at com.yubico.fido.metadata.SupportedCtapOptions.<init>(SupportedCtapOptions.java:180) [...] ``` which is the line: ``` this.plat = plat; ``` The test "FIDO Metadata Service 3 blob payloads are structurally identical after multiple (de)serialization round-trips." also begins failing now. Step 3: Use `Boolean.TRUE.equals(...)` to fix `NullPointerException`s while setting `plat`, `rk` and `up` in `SupportedCtapOptions` constructor. The test "SupportedCtapOptions can be parsed from an empty JSON object" now passes. Instead the test "FIDO Metadata Service 3 blob payloads are structurally identical after multiple (de)serialization round-trips" now fails. This is why we added the test in the previous commit, to get a more focused view of this failure. This failure is: ``` TestFailedException was thrown during property evaluation. Message: SupportedCtapOptions(plat=false, rk=true, clientPin=true, up=true, uv=true, pinUvAuthToken=false, noMcGaPermissionsWithClientPin=false, largeBlobs=false, ep=true, bioEnroll=true, userVerificationMgmtPreview=true, uvBioEnroll=false, authnrCfg=false, uvAcfg=false, credMgmt=false, credentialMgmtPreview=false, setMinPINLength=false, makeCredUvNotRqd=false, alwaysUv=true) did not equal SupportedCtapOptions(plat=false, rk=true, clientPin=true, up=true, uv=true, pinUvAuthToken=false, noMcGaPermissionsWithClientPin=false, largeBlobs=false, ep=false, bioEnroll=false, userVerificationMgmtPreview=false, uvBioEnroll=false, authnrCfg=false, uvAcfg=false, credMgmt=false, credentialMgmtPreview=false, setMinPINLength=false, makeCredUvNotRqd=false, alwaysUv=false) Location: (MetadataBlobSpec.scala:108) Occurred when passed generated values ( arg0 = SupportedCtapOptions(plat=false, rk=true, clientPin=true, up=true, uv=true, pinUvAuthToken=false, noMcGaPermissionsWithClientPin=false, largeBlobs=false, ep=false, bioEnroll=false, userVerificationMgmtPreview=false, uvBioEnroll=false, authnrCfg=false, uvAcfg=false, credMgmt=false, credentialMgmtPreview=false, setMinPINLength=false, makeCredUvNotRqd=false, alwaysUv=false) ) ``` This is because the constructor sets some of the the tri-state inputs to `true` when non-null, including when `false`. This means that when the input is absent (null), it is set to `false` and then emitted as `false` when serialized back to JSON, which is then interpreted as non-null and converted to `true` when deserializing again. Step 4: Add `@JsonInclude` directives to omit the tri-state fields that are true-when-non-null when their value is false. All tests now pass!
1 parent 77a8af8 commit 5bceb93

File tree

2 files changed

+50
-12
lines changed

2 files changed

+50
-12
lines changed

webauthn-server-attestation/src/main/java/com/yubico/fido/metadata/SupportedCtapOptions.java

Lines changed: 25 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -2,10 +2,10 @@
22

33
import com.fasterxml.jackson.annotation.JsonAlias;
44
import com.fasterxml.jackson.annotation.JsonCreator;
5+
import com.fasterxml.jackson.annotation.JsonInclude;
56
import com.fasterxml.jackson.annotation.JsonProperty;
67
import lombok.Builder;
78
import lombok.Value;
8-
import lombok.extern.jackson.Jacksonized;
99

1010
/**
1111
* A fixed-keys map of CTAP2 option names to Boolean values representing whether an authenticator
@@ -17,7 +17,6 @@
1717
*/
1818
@Value
1919
@Builder
20-
@Jacksonized
2120
public class SupportedCtapOptions {
2221

2322
/**
@@ -39,7 +38,9 @@ public class SupportedCtapOptions {
3938
* href="https://fidoalliance.org/specs/fido-v2.1-ps-20210615/fido-client-to-authenticator-protocol-v2.1-ps-20210615.html#authenticatorGetInfo">Client
4039
* to Authenticator Protocol (CTAP) §6.4. authenticatorGetInfo (0x04)</a>
4140
*/
42-
@Builder.Default boolean clientPin = false;
41+
@JsonInclude(JsonInclude.Include.NON_DEFAULT)
42+
@Builder.Default
43+
boolean clientPin = false;
4344

4445
/**
4546
* @see <a
@@ -53,7 +54,9 @@ public class SupportedCtapOptions {
5354
* href="https://fidoalliance.org/specs/fido-v2.1-ps-20210615/fido-client-to-authenticator-protocol-v2.1-ps-20210615.html#authenticatorGetInfo">Client
5455
* to Authenticator Protocol (CTAP) §6.4. authenticatorGetInfo (0x04)</a>
5556
*/
56-
@Builder.Default boolean uv = false;
57+
@JsonInclude(JsonInclude.Include.NON_DEFAULT)
58+
@Builder.Default
59+
boolean uv = false;
5760

5861
/**
5962
* @see <a
@@ -83,21 +86,27 @@ public class SupportedCtapOptions {
8386
* href="https://fidoalliance.org/specs/fido-v2.1-ps-20210615/fido-client-to-authenticator-protocol-v2.1-ps-20210615.html#authenticatorGetInfo">Client
8487
* to Authenticator Protocol (CTAP) §6.4. authenticatorGetInfo (0x04)</a>
8588
*/
86-
@Builder.Default boolean ep = false;
89+
@JsonInclude(JsonInclude.Include.NON_DEFAULT)
90+
@Builder.Default
91+
boolean ep = false;
8792

8893
/**
8994
* @see <a
9095
* href="https://fidoalliance.org/specs/fido-v2.1-ps-20210615/fido-client-to-authenticator-protocol-v2.1-ps-20210615.html#authenticatorGetInfo">Client
9196
* to Authenticator Protocol (CTAP) §6.4. authenticatorGetInfo (0x04)</a>
9297
*/
93-
@Builder.Default boolean bioEnroll = false;
98+
@JsonInclude(JsonInclude.Include.NON_DEFAULT)
99+
@Builder.Default
100+
boolean bioEnroll = false;
94101

95102
/**
96103
* @see <a
97104
* href="https://fidoalliance.org/specs/fido-v2.1-ps-20210615/fido-client-to-authenticator-protocol-v2.1-ps-20210615.html#authenticatorGetInfo">Client
98105
* to Authenticator Protocol (CTAP) §6.4. authenticatorGetInfo (0x04)</a>
99106
*/
100-
@Builder.Default boolean userVerificationMgmtPreview = false;
107+
@JsonInclude(JsonInclude.Include.NON_DEFAULT)
108+
@Builder.Default
109+
boolean userVerificationMgmtPreview = false;
101110

102111
/**
103112
* @see <a
@@ -134,7 +143,9 @@ public class SupportedCtapOptions {
134143
* href="https://fidoalliance.org/specs/fido-v2.1-ps-20210615/fido-client-to-authenticator-protocol-v2.1-ps-20210615.html#authenticatorGetInfo">Client
135144
* to Authenticator Protocol (CTAP) §6.4. authenticatorGetInfo (0x04)</a>
136145
*/
137-
@Builder.Default boolean credentialMgmtPreview = false;
146+
@JsonInclude(JsonInclude.Include.NON_DEFAULT)
147+
@Builder.Default
148+
boolean credentialMgmtPreview = false;
138149

139150
/**
140151
* @see <a
@@ -155,7 +166,9 @@ public class SupportedCtapOptions {
155166
* href="https://fidoalliance.org/specs/fido-v2.1-ps-20210615/fido-client-to-authenticator-protocol-v2.1-ps-20210615.html#authenticatorGetInfo">Client
156167
* to Authenticator Protocol (CTAP) §6.4. authenticatorGetInfo (0x04)</a>
157168
*/
158-
@Builder.Default boolean alwaysUv = false;
169+
@JsonInclude(JsonInclude.Include.NON_DEFAULT)
170+
@Builder.Default
171+
boolean alwaysUv = false;
159172

160173
@JsonCreator
161174
private SupportedCtapOptions(
@@ -178,10 +191,10 @@ private SupportedCtapOptions(
178191
@JsonProperty("setMinPINLength") Boolean setMinPINLength,
179192
@JsonProperty("makeCredUvNotRqd") Boolean makeCredUvNotRqd,
180193
@JsonProperty("alwaysUv") Boolean alwaysUv) {
181-
this.plat = plat;
182-
this.rk = rk;
194+
this.plat = Boolean.TRUE.equals(plat);
195+
this.rk = Boolean.TRUE.equals(rk);
183196
this.clientPin = clientPin != null;
184-
this.up = up;
197+
this.up = Boolean.TRUE.equals(up);
185198
this.uv = uv != null;
186199
this.pinUvAuthToken = Boolean.TRUE.equals(pinUvAuthToken);
187200
this.noMcGaPermissionsWithClientPin = Boolean.TRUE.equals(noMcGaPermissionsWithClientPin);

webauthn-server-attestation/src/test/scala/com/yubico/fido/metadata/MetadataBlobSpec.scala

Lines changed: 25 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -57,6 +57,31 @@ class MetadataBlobSpec
5757
}
5858

5959
describe("SupportedCtapOptions") {
60+
it("can be parsed from an empty JSON object.") {
61+
val options = JacksonCodecs
62+
.json()
63+
.readValue("{}", classOf[SupportedCtapOptions])
64+
options should not be null
65+
options.isPlat should be(false)
66+
options.isRk should be(false)
67+
options.isUp should be(false)
68+
options.isUv should be(false)
69+
options.isPinUvAuthToken should be(false)
70+
options.isNoMcGaPermissionsWithClientPin should be(false)
71+
options.isLargeBlobs should be(false)
72+
options.isEp should be(false)
73+
options.isBioEnroll should be(false)
74+
options.isUserVerificationMgmtPreview should be(false)
75+
options.isUvBioEnroll should be(false)
76+
options.isAuthnrCfg should be(false)
77+
options.isUvAcfg should be(false)
78+
options.isCredMgmt should be(false)
79+
options.isCredentialMgmtPreview should be(false)
80+
options.isSetMinPINLength should be(false)
81+
options.isMakeCredUvNotRqd should be(false)
82+
options.isAlwaysUv should be(false)
83+
}
84+
6085
it(
6186
"are structurally identical after multiple (de)serialization round-trips."
6287
) {

0 commit comments

Comments
 (0)