Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

non-modal "conditionally mediated" UI #1576

Merged
merged 8 commits into from
Jun 29, 2022

Conversation

equalsJeffH
Copy link
Contributor

@equalsJeffH equalsJeffH commented Feb 18, 2021

This PR enables a non-modal "conditionally mediated" UI feature for WebAuthn which RPs may utilize to provide a credential selection UI only if the user has a discoverable credential registered with the Relying Party on their authenticator (the latter being the "condition"). The credential is displayed alongside an autofilled username or password input field. This helps RPs solve the "bootstrapping problem" when migrating their user base from traditional username and password to WebAuthn: websites can fire a WebAuthn call while showing their typical username and/or password prompt without worrying about showing a modal dialog error if the device lacks appropriate credentials.

See also: Explainer: WebAuthn Conditional/Hinted UI,
as well as this related Credential Management PR #155 (merged)

fixes #1545

Overview "omnibus" issue: #1637
See also discussion in Issues #1356 #1533 #1568


Preview | Diff

index.bs Outdated Show resolved Hide resolved
pull bot pushed a commit to luojiguicai/chromium that referenced this pull request Mar 26, 2021
Update the WebAuthn Conditional UI API to match the proposed
specification, replacing the conditionalPublicKey attribute with a
"conditional" mediation value. Attempting to set the mediation to
"conditional" for password or federated credentials will result in a
NotSupportedError. Setting the value for WebAuthn requests will be
ignored unless the WebAuthenticationConditionalUI flag is enabled.

PR spec: w3c/webauthn#1576

Design doc:
https://docs.google.com/document/d/1KzEWP0aoLMZ0asfw6d3-7UHJ6csTtxLA478EgptCvkk

Bug: 1171985
Change-Id: Ia0045d7fd9becadeadaa362b29ddd56dc38a79b2
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2780274
Reviewed-by: Martin Kreichgauer <[email protected]>
Commit-Queue: Nina Satragno <[email protected]>
Cr-Commit-Position: refs/heads/master@{#866725}
index.bs Outdated
exactly one of those, it then calls <code>PublicKeyCredential.{{PublicKeyCredential/[[DiscoverFromExternalSource]]()}}</code> to have the user select a [=credential source=].

In any case, the user agent SHOULD show some UI to the user to guide them in selecting and
authorizing an authenticator with which to complete the operation. [=[RPS]=] can provide a hint that a prominent, modal UI <em>should not</em> be used for this process by setting <code>|options|.{{CredentialRequestOptions/mediation}}</code> to "conditional". This is known as a <dfn>Conditional UI Flow</dfn>. [=[RP]=] script SHOULD first check that <code>navigator.credentials.{{CredentialsContainer/conditionalMediationSupported}}</code> is [TRUE] in order to avoid the possiblity of causing a user-visible error to be returned if the user agent does not support [=conditional UI flow=].
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I just caught myself up on this proposal. On the whole I think this is a great idea. However I suggest we consider the following alternative to the naming of the boolean conditionalMediationSupported :

navigator.credentials.isConditionalMediationAvailable()

I suggest this because we already have the existing helper method isUserVerifyingPlatformAuthenticatorAvailable() that establishes an is...Available() naming convention for these kinds of helper methods. I'm also suggesting this becomes a method instead of a boolean because I believe it will eliminate confusion over whether a particular feature check in the WebAuthn API is a simple boolean, or something that needs to be invoked. If they're all invoked methods that return booleans then there's less mental overhead for RP developers working with this API.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @MasterKale, looks like this feedback belongs also in w3c/webappsec-credential-management#155 which is what would add that attribute.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for the nudge, I went ahead and cross-posted. Honestly I feel like this suggestion is less compelling over there without something like isUserVerifyingPlatformAuthenticatorAvailable() in the credential management API (which there isn't)...

If conditional mediation goes through with conditionalMediationSupported, is there any value in trying to update PublicKeyCredential with a new static method isConditionalMediationAvailable() that returns the value of navigator.credentials.conditionalMediationSupported? 😛

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A drive-by thought: Is it certain that this kind of support will always be a single boolean? The function leaves open the possibility of arguments in the future, in case the level of support depends on certain properties (e.g. of the registration parameters) in a non-trivial way.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ISTM @MasterKale makes good suggestions wrt naming and it being a method rather than a simple boolean.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree with the arguments for making it a method, but I think the argument should be "public-key" rather than "publicKey", because "public-key" is the value defined in PublicKeyCredentialType and returned in PublicKeyCredential responses. The publicKey property name in PublicKeyCredential{Creation,Request}Options should arguably have also been "public-key" in order to be formally compatible with the Credential Management API.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The publicKey property name in PublicKeyCredential{Creation,Request}Options should arguably have also been "public-key" in order to be formally compatible with the Credential Management API.

Indeed, please see w3c/webappsec-credential-management#147 ( which is on the fairly near-term to-do list.. )

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm aiming for consistency, so if WebAuthn and CredMan APIs can sync up on "public-key" then that'd be great!

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Okay so -- what about

PublicKeyCredential.isConditionalMediationAvailable()

  • works for different credential types in the future!
  • very similar to isUserVerifyingPlatformAuthenticatorAvailable()!
  • don't have to worry about the whole public-key vs publicKey thing that still hasn't been resolved!

I need to make a bunch more changes here but I'm very happy with the way feature discovery is tenatively spec'd right now.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@emlun, @MasterKale (and other reviewers!):
WRT @nsatragno's #1576 (comment) immediately above regarding isConditionalMediationAvailable() being usable by devs as simply PublicKeyCredential.isConditionalMediationAvailable(), please also see the current state of w3c/webappsec-credential-management#155 -- We're feeling it is getting very close to being baked.

@equalsJeffH equalsJeffH marked this pull request as draft May 5, 2021 23:05
pull bot pushed a commit to jamlee-t/chromium that referenced this pull request Aug 23, 2021
Have the password autofill wait for username if a form field is
annotated as `autofill="webauthn"` and the
WebAuthenticationConditionalUI flag is enabled. This is the first part
of an integration between WebAuthn Conditional UI and password autofill.

PR spec: w3c/webauthn#1576

Design doc:
https://docs.google.com/document/d/1KzEWP0aoLMZ0asfw6d3-7UHJ6csTtxLA478EgptCvkk

Bug: 1171985
Change-Id: I4c9879e36fc3923555731da8f7bf4a957e080e71
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/3107526
Commit-Queue: Nina Satragno <[email protected]>
Reviewed-by: Vasilii Sukhanov <[email protected]>
Cr-Commit-Position: refs/heads/main@{#914458}
pull bot pushed a commit to luojiguicai/chromium that referenced this pull request Sep 23, 2021
Integrate the WebAuthn Conditional UI mode with the password manager
autofill prompt. When generating suggestions for the autofill popup
dialog, have the browser inject a priori discoverable credentials that
are eligible to fulfill an ongoing request. If an error occurs during a
request, it is not aborted. Instead, the request is silently restarted
so the credentials remain a valid option if the user interacts with the
input field again.

This is a first, rough version for the integration without proper i18n
support and pending UX review.

This feature is gated behind the WebAuthenticationConditionalUI flag.

PR spec: w3c/webauthn#1576

Screenshot: https://screenshot.googleplex.com/Ajp5vePhXrN4sZP

Design doc:
https://docs.google.com/document/d/1KzEWP0aoLMZ0asfw6d3-7UHJ6csTtxLA478EgptCvkk

Bug: 1171985
Change-Id: I40e05f386b23dfcd7fed40519c72669d11aab70b
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/3140504
Commit-Queue: Nina Satragno <[email protected]>
Reviewed-by: Mohamed Amir Yosef <[email protected]>
Reviewed-by: Martin Kreichgauer <[email protected]>
Cr-Commit-Position: refs/heads/main@{#924407}
@nsatragno nsatragno force-pushed the jeffh-conditional-ui-via-mediation branch from ceefdde to f4d48c2 Compare October 15, 2021 19:27
Copy link
Contributor Author

@equalsJeffH equalsJeffH left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi @nsatragno, here's some high-level comments on the latest commit 2e58c0f, which overall is looking good...

  1. IIUC we are intending conditional mediation to be applicable to all available authenticators, including available roaming authenticators.
  2. The user is being prompted from both credman's "request a credential" (step 7.5) and here in [[DiscoverFromExternalSource]]'s step 20's "If conditionalFlow is true and the user interacts with an input form control with a "webauthn" autocomplete autofill hint set," branch?
  3. To override Credential's isConditionalMediationAvailable()'s default implementation, we define an implementation here, rather than attempt to override it using webidl's "partial interface". See the very last paragraph of section 5.1. PublicKeyCredential Interface: "PublicKeyCredential's interface object inherits Credential's implementation of ... and defines its own implementation of ... " (yeah, it's subtle/obtuse)

@nsatragno
Copy link
Member

Hey @equalsJeffH, thank you for your quick review on my rough draft!

IIUC we are intending conditional mediation to be applicable to all available authenticators, including available roaming authenticators.

Fixed to be "authenticators supporting credential discovery".

The user is being prompted from both credman's "request a credential" (step 7.5) and here in [[DiscoverFromExternalSource]]'s step 20's "If conditionalFlow is true and the user interacts with an input form control with a "webauthn" autocomplete autofill hint set," branch?

That's already the case for vanilla webauthn! Credman is a little confusing there since the "request a credential" step asks the user to "choose a credential" before even calling [[DiscoverFromExternalSource]]! Credman allows the user to select an "interface object" that represents a kind of credential (WebAuthn being the only one that exists). In reality, that selection never actually happens because A) WebAuthn is not supported in conjunction with other credential types so it's the only available option and B) webauthn credential selection requires some user interaction regardless of the mediation requirement.

At least that's my understanding after reading the whole thing several times, I might be wrong!

To override Credential's isConditionalMediationAvailable()'s default implementation, we define an implementation here, rather than attempt to override it using webidl's "partial interface". See the very last paragraph of section 5.1. PublicKeyCredential Interface: "PublicKeyCredential's interface object inherits Credential's implementation of ... and defines its own implementation of ... " (yeah, it's subtle/obtuse)

Done.

--

I added a bunch of things that were missing and moved things around. Can you take another look? Thank you! ^_^

Copy link
Contributor Author

@equalsJeffH equalsJeffH left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi @nsatragno, here's some high-level comments on the latest commit 2c09264...

  1. We do not need to define isConditionalMediationAvailable() in this spec (in the way you have it presently) -- unless you wish to move that method from the Credential interface to the PublicKeyCredential interface -- because we are defining it as part of Credential base class (or whatever the webidl term is) in Credential Management PR #155. Again, to override Credential's isConditionalMediationAvailable()'s default implementation, we define an implementation here, rather than attempt to override it using webidl's "partial interface". See the very last paragraph of section 5.1. PublicKeyCredential Interface: "PublicKeyCredential's interface object inherits Credential's implementation of ... and defines its own implementation of ... " (yeah, it's subtle/obtuse). E.g., note how WebAuthn overrides[[Store]]'s implementation. Similarly, [[Create]] and [[DiscoverFromExternalSource]] in webauthn are overriding their default impls in credman. I can write this if it'd be helpful.

update 2021-10-28 wrt (1): I equalsJeffH am effectively wrong here (apologies!), we can do this as @nsatragno proposes. Credman PR #155 defines isConditionalMediationAvailable() as a method (on the Credential interface), and as @nsatragno points out, the WebIDL spec says in 2.2. Interfaces: "Interfaces may specify an interface member that has the same name as one from an inherited interface. Objects that implement the derived interface will expose the member on the derived interface. It is language binding specific whether the overridden member can be accessed on the object." We are assuming that this will work fine for JS's language binding. (@nsatragno to test)

  1. The silentCredentialDiscovery operation simply gathers discoverable creds per a given RP from an authenticator? If so I'm thinking we can say and spec that more straightforwardly. I have some ideas.

  2. we ought to be more careful/precise with how we are using credential sources -- i think we are just using some of the items they contain, and "entire" cred sources ought not be passed in-whole to the client platform because technically doing so reveals the cred private key. See also new issue Lookup Credential Source by Credential ID Algorithm returns sensitive data such as the credential private key #1678.

index.bs Outdated Show resolved Hide resolved
index.bs Outdated Show resolved Hide resolved
Copy link
Member

@nsatragno nsatragno left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I addressed your comments and rewrote silentCredentialDiscovery to use a new struct DiscoverableCredentialMetadata which should alleviate the issue with the private key.

index.bs Outdated Show resolved Hide resolved
index.bs Outdated Show resolved Hide resolved
Copy link
Contributor Author

@equalsJeffH equalsJeffH left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi @nsatragno -- this is looking pretty good overall, thanks! there's a bunch of detail-level comments mostly having to do with maintaining the spec's alg style and use of the Infra spec. Also a few suggestions for adding context for readers.

index.bs Outdated Show resolved Hide resolved
index.bs Outdated
@@ -1947,6 +1973,106 @@ This
{{CredentialsContainer/get()|navigator.credentials.get()}} operation can be aborted by leveraging the {{AbortController}};
see [[dom#abortcontroller-api-integration]] for detailed instructions.

#### <dfn for="PublicKeyCredential" algorithm="Issuing a request to an authenticator">Issuing a request to an authenticator</dfn> #### {#sctn-issuing-request-to-authenticator}
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We probably ought to add more context to this alg's description.

In that vein, "CredentialRequest" is the (sub-)term used for the options passed to it, so we can make this match naming-wise:

Suggested change
#### <dfn for="PublicKeyCredential" algorithm="Issuing a request to an authenticator">Issuing a request to an authenticator</dfn> #### {#sctn-issuing-request-to-authenticator}
#### <dfn for="PublicKeyCredential" algorithm="Issuing a credential request to an authenticator">Issuing a Credential Request to an Authenticator</dfn> #### {#sctn-issuing-cred-request-to-authenticator}

Copy link
Contributor Author

@equalsJeffH equalsJeffH Oct 31, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I suggest we add an introductory paragraph, e.g. (this is a rough initial draft):

This algorithm encompasses the specific UI context-independent steps necessary for requesting a [=credential=] from a given [=authenticator=], using given {{CredentialRequestOptions}}. It is called by {{PublicKeyCredential/[DiscoverFromExternalSource]}} from various points depending upon whether the present [=authentication ceremony=] is subject to [=user mediation=] (e.g.: {{CredentialMediationRequirement/conditional}} mediation).

( I'm also thinking that this alg actually ought to follow [[DiscoverFromExternalSource]] rather than precede it, because the opening paragraphs of section 5.1.4 serve to introduce [[DiscoverFromExternalSource]] per se )

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm also thinking that this alg actually ought to follow [[DiscoverFromExternalSource]] rather than precede it, because the opening paragraphs of section 5.1.4 serve to introduce [[DiscoverFromExternalSource]] per se

[[DiscoverFromExternalSource]] calls this algorithm though, so maybe that's an argument to keep the order like this?

I added an introductory paragraph.

Copy link
Contributor Author

@equalsJeffH equalsJeffH Nov 8, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[[DiscoverFromExternalSource]] calls this algorithm though, so maybe that's an argument to keep the order like this?

I suppose. I'm just thinking from perspective of reader who reads the section 5.1.4 intro text, which intros [[Get]](options), which in turn calls [[DiscoverFromExternalSource]](), but then the next subsection ("issuing a cred request to an authnr") is an internal function of the latter. I suppose with all the hyperlinks that it does not matter terribly.

index.bs Show resolved Hide resolved
index.bs Outdated Show resolved Hide resolved
index.bs Outdated Show resolved Hide resolved
index.bs Outdated Show resolved Hide resolved
index.bs Show resolved Hide resolved
index.bs Outdated Show resolved Hide resolved
index.bs Outdated Show resolved Hide resolved
index.bs Outdated Show resolved Hide resolved
@nsatragno
Copy link
Member

@w3c/webauthn contributors interested in reviewing the credential management side of this patch can do so at w3c/webappsec-credential-management/155. I would also suggest considering joining the webappsec wg since that spec lives under that umbrella.

@nsatragno
Copy link
Member

@equalsJeffH I believe I've addressed all comments in some way or another, please take another look

@nsatragno nsatragno force-pushed the jeffh-conditional-ui-via-mediation branch from e9ae207 to 98d2b5e Compare June 15, 2022 18:08
@nsatragno
Copy link
Member

Since the last review:

This is ready to land from my side

@emlun
Copy link
Member

emlun commented Jun 20, 2022

  • Had isConditionalMediationAvailable() return a promise instead of a raw boolean, see issue 1745

Hm, on closer inspection... can we actually do this? The function definition in Credential returns a raw boolean, so I don't actually think we can override it with a different return type, can we?

index.bs Outdated Show resolved Hide resolved
index.bs Show resolved Hide resolved
index.bs Show resolved Hide resolved
index.bs Show resolved Hide resolved
index.bs Outdated Show resolved Hide resolved
index.bs Outdated Show resolved Hide resolved
Copy link
Member

@nsatragno nsatragno left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for the detailed review @emlun!

Hm, on closer inspection... can we actually do this? The function definition in Credential returns a raw boolean, so I don't actually think we can override it with a different return type, can we?

I am aware of this, I filed w3c/webappsec-credential-management#199. I was planning to get that change merged into credman after bringing this issue to the wg.

index.bs Show resolved Hide resolved
index.bs Outdated Show resolved Hide resolved
index.bs Outdated Show resolved Hide resolved
index.bs Show resolved Hide resolved
index.bs Show resolved Hide resolved
index.bs Outdated Show resolved Hide resolved
index.bs Outdated Show resolved Hide resolved
index.bs Outdated Show resolved Hide resolved
index.bs Show resolved Hide resolved
index.bs Outdated Show resolved Hide resolved
nsatragno and others added 2 commits June 22, 2022 14:43
Fixes:

```
FATAL ERROR: Line 2246 isn't indented enough (needs 1 indent) to be valid Markdown:
"   |savedCredentialIds| and |options|.{{CredentialRequestOptions/publicKey}}."
LINE ~2379: No 'dfn' refs found for 'extension ids'.
[=extension IDs=]
LINE ~4158: Multiple possible 'dfn' local refs for 'type'.
Randomly chose one of them; other instances might get a different random choice.
[=type=]
```

Co-authored-by: Nina Satragno <[email protected]>
Copy link
Member

@emlun emlun left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @nsatragno !

@wseltzer
Copy link
Member

Note: @equalsJeffH was a member at the time of the PR, so the IPR flag can be overridden.

Copy link
Contributor

@ve7jtb ve7jtb left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@agl
Copy link
Contributor

agl commented Jun 29, 2022

From the call of 2022-06-29: we're happy to merge this once the conflict has been resolved. The IPR bot might object to Jeff, but Wendy or others can override because Wendy is happy that Jeff was a member when he did the work.

@nsatragno nsatragno merged commit 8ad9448 into w3c:main Jun 29, 2022
@nsatragno nsatragno deleted the jeffh-conditional-ui-via-mediation branch June 29, 2022 19:34
github-actions bot added a commit that referenced this pull request Jun 29, 2022
SHA: 8ad9448
Reason: push, by @nsatragno

Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
emlun referenced this pull request in Yubico/developers.yubico.com Sep 1, 2022
mjfroman pushed a commit to mjfroman/moz-libwebrtc-third-party that referenced this pull request Oct 14, 2022
Update the WebAuthn Conditional UI API to match the proposed
specification, replacing the conditionalPublicKey attribute with a
"conditional" mediation value. Attempting to set the mediation to
"conditional" for password or federated credentials will result in a
NotSupportedError. Setting the value for WebAuthn requests will be
ignored unless the WebAuthenticationConditionalUI flag is enabled.

PR spec: w3c/webauthn#1576

Design doc:
https://docs.google.com/document/d/1KzEWP0aoLMZ0asfw6d3-7UHJ6csTtxLA478EgptCvkk

Bug: 1171985
Change-Id: Ia0045d7fd9becadeadaa362b29ddd56dc38a79b2
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2780274
Reviewed-by: Martin Kreichgauer <[email protected]>
Commit-Queue: Nina Satragno <[email protected]>
Cr-Commit-Position: refs/heads/master@{#866725}
GitOrigin-RevId: dd63c7ffc1616b8925982859c890ffe3cfa47251
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add support for non-modal UI