Don't overwrite per-request extension inputs in RelyingParty methods

Yubico · Oct 26, 2021 · a16f29a · a16f29a
1 parent c9dda48
commit a16f29a
Show file tree

Hide file tree

Showing 5 changed files with 66 additions and 24 deletions.
diff --git a/NEWS b/NEWS
@@ -13,6 +13,8 @@ Deprecated features will be removed in the next major version release.
 
 Changes:
 
+* `RelyingParty.startAssertion()` no longer overwrites the `appid` extension
+  input in the `StartAssertionOptions` argument.
 * `RelyingParty.appId` setting now also activates the `appidExclude` extension in
   addition to the `appid` extension.
 * `RelyingParty.startRegistration()` now enables the `credProps` extension by

diff --git a/webauthn-server-core/src/main/java/com/yubico/webauthn/RelyingParty.java b/webauthn-server-core/src/main/java/com/yubico/webauthn/RelyingParty.java
@@ -405,10 +405,13 @@ public PublicKeyCredentialCreationOptions startRegistration(
                     startRegistrationOptions.getUser().getName()))
             .authenticatorSelection(startRegistrationOptions.getAuthenticatorSelection())
             .extensions(
-                startRegistrationOptions.getExtensions().toBuilder()
-                    .appidExclude(appId)
-                    .credProps()
-                    .build())
+                startRegistrationOptions
+                    .getExtensions()
+                    .merge(
+                        RegistrationExtensionInputs.builder()
+                            .appidExclude(appId)
+                            .credProps()
+                            .build()))
             .timeout(startRegistrationOptions.getTimeout());
     attestationConveyancePreference.ifPresent(builder::attestation);
     return builder.build();
@@ -469,7 +472,10 @@ public AssertionRequest startAssertion(StartAssertionOptions startAssertionOptio
                     .map(
                         un ->
                             new ArrayList<>(credentialRepository.getCredentialIdsForUsername(un))))
-            .extensions(startAssertionOptions.getExtensions().toBuilder().appid(appId).build())
+            .extensions(
+                startAssertionOptions
+                    .getExtensions()
+                    .merge(startAssertionOptions.getExtensions().toBuilder().appid(appId).build()))
             .timeout(startAssertionOptions.getTimeout());
 
     startAssertionOptions.getUserVerification().ifPresent(pkcro::userVerification);

diff --git a/webauthn-server-core/src/main/java/com/yubico/webauthn/data/AssertionExtensionInputs.java b/webauthn-server-core/src/main/java/com/yubico/webauthn/data/AssertionExtensionInputs.java
@@ -54,10 +54,8 @@
 public class AssertionExtensionInputs implements ExtensionInputs {
 
   private final AppId appid;
-
   private final Extensions.LargeBlob.LargeBlobAuthenticationInput largeBlob;
-
-  private final boolean uvm;
+  private final Boolean uvm;
 
   @JsonCreator
   private AssertionExtensionInputs(
@@ -66,7 +64,21 @@ private AssertionExtensionInputs(
       @JsonProperty("uvm") Boolean uvm) {
     this.appid = appid;
     this.largeBlob = largeBlob;
-    this.uvm = uvm != null && uvm;
+    this.uvm = (uvm != null && uvm) ? true : null;
+  }
+
+  /**
+   * Merge <code>other</code> into <code>this</code>. Non-null field values from <code>this</code>
+   * take precedence.
+   *
+   * @return a new {@link AssertionExtensionInputs} instance with the settings from both <code>this
+   *     </code> and <code>other</code>.
+   */
+  public AssertionExtensionInputs merge(AssertionExtensionInputs other) {
+    return new AssertionExtensionInputs(
+        this.appid != null ? this.appid : other.appid,
+        this.largeBlob != null ? this.largeBlob : other.largeBlob,
+        this.uvm != null ? this.uvm : other.uvm);
   }
 
   /**
@@ -83,7 +95,7 @@ public Set<String> getExtensionIds() {
     if (largeBlob != null) {
       ids.add(Extensions.LargeBlob.EXTENSION_ID);
     }
-    if (uvm) {
+    if (getUvm()) {
       ids.add(Extensions.Uvm.EXTENSION_ID);
     }
     return ids;
@@ -229,12 +241,12 @@ private Extensions.LargeBlob.LargeBlobAuthenticationInput getLargeBlobJson() {
    *     User Verification Method Extension (uvm)</a>
    */
   public boolean getUvm() {
-    return uvm;
+    return uvm != null && uvm;
   }
 
   /** For JSON serialization, to omit false values. */
   @JsonProperty("uvm")
   private Boolean getUvmJson() {
-    return uvm ? true : null;
+    return getUvm() ? true : null;
   }
 }
diff --git a/webauthn-server-core/src/main/java/com/yubico/webauthn/data/RegistrationExtensionInputs.java b/webauthn-server-core/src/main/java/com/yubico/webauthn/data/RegistrationExtensionInputs.java
@@ -54,23 +54,37 @@
 public final class RegistrationExtensionInputs implements ExtensionInputs {
 
   private final AppId appidExclude;
-
-  private final boolean credProps;
+  private final Boolean credProps;
   private final Extensions.LargeBlob.LargeBlobRegistrationInput largeBlob;
-  private final boolean uvm;
+  private final Boolean uvm;
 
   @JsonCreator
   private RegistrationExtensionInputs(
       @JsonProperty("appidExclude") AppId appidExclude,
-      @JsonProperty("credProps") boolean credProps,
+      @JsonProperty("credProps") Boolean credProps,
       @JsonProperty("largeBlob") Extensions.LargeBlob.LargeBlobRegistrationInput largeBlob,
-      @JsonProperty("uvm") boolean uvm) {
+      @JsonProperty("uvm") Boolean uvm) {
     this.appidExclude = appidExclude;
     this.credProps = credProps;
     this.largeBlob = largeBlob;
     this.uvm = uvm;
   }
 
+  /**
+   * Merge <code>other</code> into <code>this</code>. Non-null field values from <code>this</code>
+   * take precedence.
+   *
+   * @return a new {@link RegistrationExtensionInputs} instance with the settings from both <code>
+   *     this</code> and <code>other</code>.
+   */
+  public RegistrationExtensionInputs merge(RegistrationExtensionInputs other) {
+    return new RegistrationExtensionInputs(
+        this.appidExclude != null ? this.appidExclude : other.appidExclude,
+        this.credProps != null ? this.credProps : other.credProps,
+        this.largeBlob != null ? this.largeBlob : other.largeBlob,
+        this.uvm != null ? this.uvm : other.uvm);
+  }
+
   /**
    * @return The value of the FIDO AppID Exclusion Extension (<code>appidExclude</code>) input if
    *     configured, empty otherwise.
@@ -92,13 +106,13 @@ public Optional<AppId> getAppidExclude() {
    *     Credential Properties Extension (credProps)</a>
    */
   public boolean getCredProps() {
-    return credProps;
+    return credProps != null && credProps;
   }
 
   /** For JSON serialization, to omit false values. */
   @JsonProperty("credProps")
   private Boolean getCredPropsJson() {
-    return credProps ? true : null;
+    return getCredProps() ? true : null;
   }
 
   /**
@@ -124,13 +138,13 @@ public Optional<Extensions.LargeBlob.LargeBlobRegistrationInput> getLargeBlob()
    *     User Verification Method Extension (uvm)</a>
    */
   public boolean getUvm() {
-    return uvm;
+    return uvm != null && uvm;
   }
 
   /** For JSON serialization, to omit false values. */
   @JsonProperty("uvm")
   private Boolean getUvmJson() {
-    return uvm ? true : null;
+    return getUvm() ? true : null;
   }
 
   /**
@@ -144,13 +158,13 @@ public Set<String> getExtensionIds() {
     if (appidExclude != null) {
       ids.add(Extensions.AppidExclude.EXTENSION_ID);
     }
-    if (credProps) {
+    if (getCredProps()) {
       ids.add(Extensions.CredentialProperties.EXTENSION_ID);
     }
     if (largeBlob != null) {
       ids.add(Extensions.LargeBlob.EXTENSION_ID);
     }
-    if (uvm) {
+    if (getUvm()) {
       ids.add(Extensions.Uvm.EXTENSION_ID);
     }
     return Collections.unmodifiableSet(ids);
@@ -164,6 +178,10 @@ public static class RegistrationExtensionInputsBuilder {
      * is present, then {@link RelyingParty#startRegistration(StartRegistrationOptions)} will enable
      * this extension automatically.
      *
+     * <p>If this is set to empty, then {@link
+     * RelyingParty#startRegistration(StartRegistrationOptions)} may overwrite it.
+     *
+     * @see RelyingParty#startRegistration(StartRegistrationOptions)
      * @see <a
      *     href="https://www.w3.org/TR/2021/REC-webauthn-2-20210408/#sctn-appid-exclude-extension">§10.2.
      *     FIDO AppID Exclusion Extension (appidExclude)</a>
@@ -180,6 +198,10 @@ public RegistrationExtensionInputsBuilder appidExclude(Optional<AppId> appidExcl
      * is present, then {@link RelyingParty#startRegistration(StartRegistrationOptions)} will enable
      * this extension automatically.
      *
+     * <p>If this is set to null, then {@link
+     * RelyingParty#startRegistration(StartRegistrationOptions)} may overwrite it.
+     *
+     * @see RelyingParty#startRegistration(StartRegistrationOptions)
      * @see <a
      *     href="https://www.w3.org/TR/2021/REC-webauthn-2-20210408/#sctn-appid-exclude-extension">§10.2.
      *     FIDO AppID Exclusion Extension (appidExclude)</a>

diff --git a/webauthn-server-core/src/test/scala/com/yubico/webauthn/data/Generators.scala b/webauthn-server-core/src/test/scala/com/yubico/webauthn/data/Generators.scala
@@ -350,7 +350,7 @@ object Generators {
         largeBlobGen: Gen[
           Option[com.yubico.webauthn.data.Extensions.LargeBlob.LargeBlobRegistrationInput]
         ] = Gen.option(LargeBlob.largeBlobRegistrationInput),
-        uvmGen: Gen[Option[Boolean]] = Gen.option(true),
+        uvmGen: Gen[Option[Boolean]] = Gen.option(Gen.const(true)),
     ): Gen[RegistrationExtensionInputs] =
       for {
         appidExclude <- appidExcludeGen