Skip to content

Commit

Permalink
Remove defaults for userVerification and residentKey
Browse files Browse the repository at this point in the history
By having defaults in the library, we are inadvertently suppressing warnings
that browsers may issue in the browser console when for example
`userVerification` is not set explicitly. By making the defaults in the library
not set, we fall back to the browser defaults and do not suppress any such
warnings.
  • Loading branch information
emlun committed Mar 30, 2022
1 parent c196d65 commit 68a1186
Show file tree
Hide file tree
Showing 10 changed files with 194 additions and 31 deletions.
7 changes: 7 additions & 0 deletions NEWS
Original file line number Diff line number Diff line change
Expand Up @@ -43,6 +43,13 @@ Breaking changes:
* Classes in package `com.yubico.fido.metadata` moved to
`com.yubico.webauthn.extension.uvm` to avoid name clash with
`webauthn-server-attestation` module in JPMS.
* Changed return type of
`PublicKeyCredentialRequestOptions.getUserVerification()`,
`AuthenticatorSelectionCriteria.getUserVerification()` and
`AuthenticatorSelectionCriteria.getResidentKey()` to `Optional`, and changed
defaults for `userVerification` and `residentKey` to empty. This means we
won't inadvertently suppress warnings that browsers might issue in the browser
console if for example `userVerification` is not set explicitly.

New features:

Expand Down
52 changes: 52 additions & 0 deletions doc/Migrating_from_v1.adoc
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,8 @@ Here is a high-level outline of what needs to be updated:
- Replace any implementations of `MetadataService` with
`AttestationTrustSource`.
- Rename imports of classes in `com.yubico.fido.metadata`.
- Update `getUserVerification()` and `getResidentKey()` calls
to expect `Optional` values.
== Replace dependency on `webauthn-server-core-minimal`
Expand Down Expand Up @@ -297,3 +299,53 @@ Example:
+import com.yubico.webauthn.extension.uvm.MatcherProtectionType;
+import com.yubico.webauthn.extension.uvm.UserVerificationMethod;
----------


== Update `getUserVerification()` and `getResidentKey()` calls to expect `Optional` values

The default `"preferred"` for `userVerification` has
link:https://github.com/w3c/webauthn/issues/1253[turned out to cause confusion].
Therefore, browsers have started issuing console warnings
when `userVerification` is not set explicitly.
This library has mirrored the defaults for
`PublicKeyCredentialRequestOptions.userVerification` and
`AuthenticatorSelectionCriteria.userVerification`,
but this inadvertently suppresses any browser console warnings
since the library emits parameter objects with an explicit value set,
even if the value was not explicitly set at the library level.
The defaults have therefore been removed,
and the corresponding getters now return `Optional` values.
For consistency, the same change applies to
`AuthenticatorSelectionCriteria.residentKey` as well.

The setters for these settings remain unchanged,
but if you use the getters you need to expect `Optional` values instead.

Example:

[source,diff]
----------
PublicKeyCredentialCreationOptions pkcco = /* ... */;
if (pkcco
.getAuthenticatorSelectionCriteria()
- .map(AuthenticatorSelectionCriteria::getUserVerification)
+ .flatMap(AuthenticatorSelectionCriteria::getUserVerification)
.equals(Optional.of(UserVerificationRequirement.REQUIRED))) {
// Do something...
}
if (pkcco
.getAuthenticatorSelectionCriteria()
- .map(AuthenticatorSelectionCriteria::getResidentKey)
+ .flatMap(AuthenticatorSelectionCriteria::getResidentKey)
.equals(Optional.of(ResidentKeyRequirement.REQUIRED))) {
// Do something...
}
PublicKeyCredentialRequestOptions pkcro = /* ... */;
if (pkcro
.getUserVerification()
- == UserVerificationRequirement.REQUIRED)) {
+ .equals(Optional.of(UserVerificationRequirement.REQUIRED))) {
// Do something...
}
----------
Original file line number Diff line number Diff line change
Expand Up @@ -413,8 +413,10 @@ class Step17 implements Step<Step18> {

@Override
public void validate() {
if (request.getPublicKeyCredentialRequestOptions().getUserVerification()
== UserVerificationRequirement.REQUIRED) {
if (request
.getPublicKeyCredentialRequestOptions()
.getUserVerification()
.equals(Optional.of(UserVerificationRequirement.REQUIRED))) {
assure(
response.getResponse().getParsedAuthenticatorData().getFlags().UV,
"User Verification is required.");
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -289,7 +289,7 @@ class Step15 implements Step<Step16> {
public void validate() {
if (request
.getAuthenticatorSelection()
.map(AuthenticatorSelectionCriteria::getUserVerification)
.flatMap(AuthenticatorSelectionCriteria::getUserVerification)
.orElse(UserVerificationRequirement.PREFERRED)
== UserVerificationRequirement.REQUIRED) {
assure(
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -53,7 +53,8 @@ public class AuthenticatorSelectionCriteria {
* Specifies the extent to which the Relying Party desires to create a client-side discoverable
* credential. For historical reasons the naming retains the deprecated “resident” terminology.
*
* <p>The default is {@link ResidentKeyRequirement#DISCOURAGED}.
* <p>By default, this is not set. When not set, the default in the browser is {@link
* ResidentKeyRequirement#DISCOURAGED}.
*
* @see ResidentKeyRequirement
* @see <a
Expand All @@ -70,6 +71,16 @@ public class AuthenticatorSelectionCriteria {
* href="https://www.w3.org/TR/2021/REC-webauthn-2-20210408/#user-verification">user
* verification</a> for the <code>navigator.credentials.create()</code> operation. Eligible
* authenticators are filtered to only those capable of satisfying this requirement.
*
* <p>By default, this is not set. When not set, the default in the browser is {@link
* UserVerificationRequirement#PREFERRED}.
*
* @see UserVerificationRequirement
* @see <a
* href="https://www.w3.org/TR/2021/REC-webauthn-2-20210408/#enum-userVerificationRequirement">§5.8.6.
* User Verification Requirement Enumeration (enum UserVerificationRequirement)</a>
* @see <a href="https://www.w3.org/TR/2021/REC-webauthn-2-20210408/#user-verification">User
* Verification</a>
*/
private UserVerificationRequirement userVerification;

Expand All @@ -82,6 +93,45 @@ public Optional<AuthenticatorAttachment> getAuthenticatorAttachment() {
return Optional.ofNullable(authenticatorAttachment);
}

/**
* Specifies the extent to which the Relying Party desires to create a client-side discoverable
* credential. For historical reasons the naming retains the deprecated “resident” terminology.
*
* <p>By default, this is not set. When not set, the default in the browser is {@link
* ResidentKeyRequirement#DISCOURAGED}.
*
* @see ResidentKeyRequirement
* @see <a
* href="https://www.w3.org/TR/2021/REC-webauthn-2-20210408/#enum-residentKeyRequirement">§5.4.6.
* Resident Key Requirement Enumeration (enum ResidentKeyRequirement)</a>
* @see <a
* href="https://www.w3.org/TR/2021/REC-webauthn-2-20210408/#client-side-discoverable-credential">Client-side
* discoverable Credential</a>
*/
public Optional<ResidentKeyRequirement> getResidentKey() {
return Optional.ofNullable(residentKey);
}

/**
* Describes the Relying Party's requirements regarding <a
* href="https://www.w3.org/TR/2021/REC-webauthn-2-20210408/#user-verification">user
* verification</a> for the <code>navigator.credentials.create()</code> operation. Eligible
* authenticators are filtered to only those capable of satisfying this requirement.
*
* <p>By default, this is not set. When not set, the default in the browser is {@link
* UserVerificationRequirement#PREFERRED}.
*
* @see UserVerificationRequirement
* @see <a
* href="https://www.w3.org/TR/2021/REC-webauthn-2-20210408/#enum-userVerificationRequirement">§5.8.6.
* User Verification Requirement Enumeration (enum UserVerificationRequirement)</a>
* @see <a href="https://www.w3.org/TR/2021/REC-webauthn-2-20210408/#user-verification">User
* Verification</a>
*/
public Optional<UserVerificationRequirement> getUserVerification() {
return Optional.ofNullable(userVerification);
}

@JsonCreator
private AuthenticatorSelectionCriteria(
@JsonProperty("authenticatorAttachment") AuthenticatorAttachment authenticatorAttachment,
Expand All @@ -90,15 +140,16 @@ private AuthenticatorSelectionCriteria(
@JsonProperty("userVerification") UserVerificationRequirement userVerification) {
this.authenticatorAttachment = authenticatorAttachment;

if (residentKey == null && requireResidentKey != null) {
if (residentKey != null) {
this.residentKey = residentKey;
} else if (requireResidentKey != null) {
this.residentKey =
requireResidentKey ? ResidentKeyRequirement.REQUIRED : ResidentKeyRequirement.DISCOURAGED;
} else {
this.residentKey = residentKey == null ? ResidentKeyRequirement.DISCOURAGED : residentKey;
this.residentKey = null;
}

this.userVerification =
userVerification == null ? UserVerificationRequirement.PREFERRED : userVerification;
this.userVerification = userVerification;
}

/** For use by the builder. */
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -86,10 +86,18 @@ public class PublicKeyCredentialRequestOptions {
* verification</a> for the <code>navigator.credentials.get()</code> operation.
*
* <p>Eligible authenticators are filtered to only those capable of satisfying this requirement.
*
* <p>By default, this is not set. When not set, the default in the browser is {@link
* UserVerificationRequirement#PREFERRED}.
*
* @see UserVerificationRequirement
* @see <a
* href="https://www.w3.org/TR/2021/REC-webauthn-2-20210408/#enum-userVerificationRequirement">§5.8.6.
* User Verification Requirement Enumeration (enum UserVerificationRequirement)</a>
* @see <a href="https://www.w3.org/TR/2021/REC-webauthn-2-20210408/#user-verification">User
* Verification</a>
*/
@NonNull @Builder.Default
private final UserVerificationRequirement userVerification =
UserVerificationRequirement.PREFERRED;
private final UserVerificationRequirement userVerification;

/**
* Additional parameters requesting additional processing by the client and authenticator.
Expand All @@ -106,7 +114,7 @@ private PublicKeyCredentialRequestOptions(
@JsonProperty("timeout") Long timeout,
@JsonProperty("rpId") String rpId,
@JsonProperty("allowCredentials") List<PublicKeyCredentialDescriptor> allowCredentials,
@NonNull @JsonProperty("userVerification") UserVerificationRequirement userVerification,
@JsonProperty("userVerification") UserVerificationRequirement userVerification,
@NonNull @JsonProperty("extensions") AssertionExtensionInputs extensions) {
this.challenge = challenge;
this.timeout = timeout;
Expand All @@ -125,6 +133,10 @@ public Optional<List<PublicKeyCredentialDescriptor>> getAllowCredentials() {
return Optional.ofNullable(allowCredentials);
}

public Optional<UserVerificationRequirement> getUserVerification() {
return Optional.ofNullable(userVerification);
}

/**
* Serialize this {@link PublicKeyCredentialRequestOptions} value to JSON suitable for sending to
* the client.
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -27,14 +27,14 @@ public void newResidentKeyOverridesOld() throws JsonProcessingException {
json.readValue(
"{\"requireResidentKey\": false, \"residentKey\": \"required\"}",
AuthenticatorSelectionCriteria.class);
assertEquals(decoded.getResidentKey(), ResidentKeyRequirement.REQUIRED);
assertEquals(decoded.getResidentKey(), Optional.of(ResidentKeyRequirement.REQUIRED));
}

@Test
public void newResidentKeyFallsBackToOld() throws JsonProcessingException {
ObjectMapper json = JacksonCodecs.json();
AuthenticatorSelectionCriteria decoded =
json.readValue("{\"requireResidentKey\": true}", AuthenticatorSelectionCriteria.class);
assertEquals(decoded.getResidentKey(), ResidentKeyRequirement.REQUIRED);
assertEquals(decoded.getResidentKey(), Optional.of(ResidentKeyRequirement.REQUIRED));
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -283,10 +283,7 @@ class RelyingPartyAssertionSpec
describe(
"respects the userVerification parameter in StartAssertionOptions."
) {

val default = UserVerificationRequirement.PREFERRED

it(s"If the parameter is not set, or set to empty, the default of ${default} is used.") {
it(s"If the parameter is not set, or set to empty, it is also empty in the result.") {
val rp = RelyingParty
.builder()
.identity(Defaults.rpId)
Expand All @@ -301,11 +298,11 @@ class RelyingPartyAssertionSpec
.build()
)

request1.getPublicKeyCredentialRequestOptions.getUserVerification should equal(
default
request1.getPublicKeyCredentialRequestOptions.getUserVerification.asScala should be(
None
)
request2.getPublicKeyCredentialRequestOptions.getUserVerification should equal(
default
request2.getPublicKeyCredentialRequestOptions.getUserVerification.asScala should be(
None
)
}

Expand All @@ -316,12 +313,15 @@ class RelyingPartyAssertionSpec
.credentialRepository(Helpers.CredentialRepository.empty)
.build()

forAll { uv: UserVerificationRequirement =>
forAll { uv: Option[UserVerificationRequirement] =>
val request = rp.startAssertion(
StartAssertionOptions.builder().userVerification(uv).build()
StartAssertionOptions
.builder()
.userVerification(uv.asJava)
.build()
)

request.getPublicKeyCredentialRequestOptions.getUserVerification should equal(
request.getPublicKeyCredentialRequestOptions.getUserVerification.asScala should equal(
uv
)
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -443,14 +443,27 @@ class RelyingPartyStartOperationSpec
.build()
)

pkccoDiscouraged.getAuthenticatorSelection.get.getResidentKey should be(
ResidentKeyRequirement.DISCOURAGED
val pkccoUnspecified = rp.startRegistration(
StartRegistrationOptions
.builder()
.user(userId)
.authenticatorSelection(
AuthenticatorSelectionCriteria.builder().build()
)
.build()
)

pkccoDiscouraged.getAuthenticatorSelection.get.getResidentKey.asScala should be(
Some(ResidentKeyRequirement.DISCOURAGED)
)
pkccoPreferred.getAuthenticatorSelection.get.getResidentKey should be(
ResidentKeyRequirement.PREFERRED
pkccoPreferred.getAuthenticatorSelection.get.getResidentKey.asScala should be(
Some(ResidentKeyRequirement.PREFERRED)
)
pkccoRequired.getAuthenticatorSelection.get.getResidentKey should be(
ResidentKeyRequirement.REQUIRED
pkccoRequired.getAuthenticatorSelection.get.getResidentKey.asScala should be(
Some(ResidentKeyRequirement.REQUIRED)
)
pkccoUnspecified.getAuthenticatorSelection.get.getResidentKey.asScala should be(
None
)
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -435,6 +435,17 @@ class JsonIoSpec
}

describe("The class PublicKeyCredentialRequestOptions") {
it("by default does not set a userVerification value.") {
forAll { challenge: ByteArray =>
val pkcro = PublicKeyCredentialRequestOptions
.builder()
.challenge(challenge)
.build()
val jsonValue = JacksonCodecs.json.valueToTree[ObjectNode](pkcro)
jsonValue.get("userVerification") should be(null)
}
}

it("""has a toCredentialsGetJson() method which returns a JSON object with the PublicKeyCredentialGetOptions set as a top-level "publicKey" property.""") {
forAll { pkcro: PublicKeyCredentialRequestOptions =>
val jsonValue = JacksonCodecs.json.readTree(pkcro.toCredentialsGetJson)
Expand All @@ -447,6 +458,21 @@ class JsonIoSpec
}
}

describe("The class AuthenticatorSelectionCriteria") {
it("by default does not set a userVerification value.") {
val asc = AuthenticatorSelectionCriteria.builder().build()
val jsonValue = JacksonCodecs.json.valueToTree[ObjectNode](asc)
jsonValue.get("userVerification") should be(null)
}

it("by default does not set a residentKey value.") {
val asc = AuthenticatorSelectionCriteria.builder().build()
val jsonValue = JacksonCodecs.json.valueToTree[ObjectNode](asc)
jsonValue.get("residentKey") should be(null)
jsonValue.get("requireResidentKey") should be(null)
}
}

describe("The class AssertionRequest") {
it("""has a toCredentialsGetJson() method which returns a JSON object with the PublicKeyCredentialGetOptions set as a top-level "publicKey" property.""") {
forAll { req: AssertionRequest =>
Expand Down

0 comments on commit 68a1186

Please sign in to comment.