From a16f29ac8db1d745015884ab9367bc83d0a63352 Mon Sep 17 00:00:00 2001 From: Emil Lundberg Date: Tue, 26 Oct 2021 15:40:05 +0200 Subject: [PATCH] Don't overwrite per-request extension inputs in RelyingParty methods --- NEWS | 2 + .../com/yubico/webauthn/RelyingParty.java | 16 ++++--- .../data/AssertionExtensionInputs.java | 26 ++++++++--- .../data/RegistrationExtensionInputs.java | 44 ++++++++++++++----- .../com/yubico/webauthn/data/Generators.scala | 2 +- 5 files changed, 66 insertions(+), 24 deletions(-) diff --git a/NEWS b/NEWS index adfc1ba9b..c9c620313 100644 --- 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 index 286094925..1242b1906 100644 --- 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 index 68bd8520f..18d259de0 100644 --- 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 other into this. Non-null field values from this + * take precedence. + * + * @return a new {@link AssertionExtensionInputs} instance with the settings from both this + * and other. + */ + 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 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) */ 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 index 0bcb3bd5a..0e18cbe05 100644 --- 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 other into this. Non-null field values from this + * take precedence. + * + * @return a new {@link RegistrationExtensionInputs} instance with the settings from both + * this and other. + */ + 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 (appidExclude) input if * configured, empty otherwise. @@ -92,13 +106,13 @@ public Optional getAppidExclude() { * Credential Properties Extension (credProps) */ 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 getLargeBlob() * User Verification Method Extension (uvm) */ 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 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. * + *

If this is set to empty, then {@link + * RelyingParty#startRegistration(StartRegistrationOptions)} may overwrite it. + * + * @see RelyingParty#startRegistration(StartRegistrationOptions) * @see §10.2. * FIDO AppID Exclusion Extension (appidExclude) @@ -180,6 +198,10 @@ public RegistrationExtensionInputsBuilder appidExclude(Optional appidExcl * is present, then {@link RelyingParty#startRegistration(StartRegistrationOptions)} will enable * this extension automatically. * + *

If this is set to null, then {@link + * RelyingParty#startRegistration(StartRegistrationOptions)} may overwrite it. + * + * @see RelyingParty#startRegistration(StartRegistrationOptions) * @see §10.2. * FIDO AppID Exclusion Extension (appidExclude) 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 index 3d7a1238c..73060d9cf 100644 --- 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