Add "conditional mediation" #155

equalsJeffH · 2021-02-18T00:31:56Z

This PR adds the Credential Management-layer underpinnings necessary for credential types to effect a "Conditional UI" with WebAuthn being the driving use case, as explained in Explainer: WebAuthn Conditional/Hinted UI.

The related WebAuthn issues and PRs are:

Preview | Diff

See also [WebAuthn Conditional/Hinted UI](https://docs.google.com/document/d/11hWpUPAnblPtkn1f7AIQW0ujoiu_BAKzlMVhZKQPiW8/).

mikewest · 2021-02-18T08:47:19Z

Thanks for including a link to the explainer, that's really helpful. The approach seems reasonable, but it seems like something we should run through the TAG before we'd ship it in Chromium. Would you mind filing a review request?

Are other vendors on board with the approach/extra enum value?

see also w3c/webappsec-credential-management#155 and [Explainer: WebAuthn Conditional/Hinted UI](https://docs.google.com/document/d/11hWpUPAnblPtkn1f7AIQW0ujoiu_BAKzlMVhZKQPiW8/)

nsatragno

A few thoughts that came up after more thinking.

(also, did you mean to update the html? seems like a bunch of unrelated changes but if the file is autogenerated maybe it doesn't matter?)

index.src.html

equalsJeffH · 2021-04-20T00:04:06Z

(also, did you mean to update the html? seems like a bunch of unrelated changes but if the file is autogenerated maybe it doesn't matter?)

Yeah, I think editors are expected to re-gen the html file at the time of pushing changes to the .bs source file up to the repo here, and I probably didn't do it properly.

see also w3c/webappsec-credential-management#155 and [Explainer: WebAuthn Conditional/Hinted UI](https://docs.google.com/document/d/11hWpUPAnblPtkn1f7AIQW0ujoiu_BAKzlMVhZKQPiW8/)

* Fix some links * Discover availability by calling a method on each Credential interface object to discriminate between credential types * Instruct the browser to avoid revealing credential availability when using conditional UI on error.

I think it was brought by an earlier merge commit. Should be fine if we squash and rebase this into main later.

nsatragno · 2021-10-25T19:54:29Z

@equalsJeffH please take a look, I've addressed the comments (and merged here for ease of maintenance)

equalsJeffH

Thanks @nsatragno, this is really close. On detailed review I wonder about whether declaring isConditionalMediationAvailable() as static is what we want, and then a few modest editorial suggestions/fixes.

index.bs

Co-authored-by: =JeffH <jdhodges@google.com>

index.bs

jyasskin · 2021-11-03T23:55:55Z

index.bs

+        If no credentials are discovered, the user agent MAY prompt the user to take action in a way
+        that depends on the type of credential (e.g. to insert a device containing credentials).
+        Either way, the `get()` method MUST NOT return immediately with `null` to avoid revealing
+        the lack of applicable credentials to the website.


I'm nervous about having normative requirements in this section. They need to be reflected in the algorithm below regardless, and having them here too introduces the possibility for them to get out of sync.

👀

I think it's fine since the whole not returning empty-handed is critical to Conditional UI anyway. Either way I changed "return immediately" by "resolve immediately".

Yeah, good point about the MUST NOT. Where does the "MAY prompt" happen? It seems like it'd fit just before the call to "ask the user to choose a Credential, but that doesn't call back to the particular credential types in the list.

This might be a pre-existing difference between the spec and implementations, in which case feel free to defer it to a separate PR.

Where does the "MAY prompt" happen?

I'm not sure I understand this.

It seems like it'd fit just before the call to "ask the user to choose a Credential,

We could definitely add a note there for credential types other than PublicKeyCredential.

that doesn't call back to the particular credential types in the list.

WebAuthn loosely specifies its own credential chooser on [[PublicKeyCredential/DiscoverFromExternalSource]], and there's a bunch of conditional mediation logic I'm adding there. The implementation matches the spec in a non obvious way: only one credential type is supported at a time, so CredMan's "ask the user to choose a Credential" does not actually prompt the user ("PublicKeyCredential" is always selected as the default) and the webauthn "picker" is shown instead (and the user being prompted to interact with their authenticator satisfies the mediation requests).

I think the way this PR deals with choosing credentials (and the spec before this PR) is correct, or at least good enough. Hopefully that makes sense?

index.bs

jyasskin · 2021-11-04T20:49:14Z

index.bs

+        If no credentials are discovered, the user agent MAY prompt the user to take action in a way
+        that depends on the type of credential (e.g. to insert a device containing credentials).
+        Either way, the `get()` method MUST NOT return immediately with `null` to avoid revealing
+        the lack of applicable credentials to the website.


Yeah, good point about the MUST NOT. Where does the "MAY prompt" happen? It seems like it'd fit just before the call to "ask the user to choose a Credential, but that doesn't call back to the particular credential types in the list.

This might be a pre-existing difference between the spec and implementations, in which case feel free to defer it to a separate PR.

index.bs

Co-authored-by: Jeffrey Yasskin <jyasskin@chromium.org>

jyasskin

Looks good once you've fixed the build. (This spec ran into whatwg/dom#1027 which renamed the 'aborted flag'.)

nsatragno · 2021-11-09T20:06:50Z

~~Thanks! I changed it to reference the aborted attribute to keep the same language. Probably nbd either way.~~

nvm I filed pr #176 for this

equalsJeffH · 2021-11-12T23:15:01Z

This needs a merge-from-main in order to pick up the changes from PR #176 (now merged), which will fix the failing build (the w3c spec-prod machinery stipulates a "die on warning" and so will not build correctly unless all the references and such are properly resolving)

equalsJeffH

LGTM, ready for reviews :)
Thanks @nsatragno !

@nsatragno

SHA: b273b4a Reason: push, by @nsatragno Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>

see also w3c/webappsec-credential-management#155 and [Explainer: WebAuthn Conditional/Hinted UI](https://docs.google.com/document/d/11hWpUPAnblPtkn1f7AIQW0ujoiu_BAKzlMVhZKQPiW8/)

Add "conditional mediation"

d40af4b

See also [WebAuthn Conditional/Hinted UI](https://docs.google.com/document/d/11hWpUPAnblPtkn1f7AIQW0ujoiu_BAKzlMVhZKQPiW8/).

equalsJeffH added enhancement CREDENTIAL labels Feb 18, 2021

equalsJeffH self-assigned this Feb 18, 2021

equalsJeffH mentioned this pull request Feb 18, 2021

non-modal "conditionally mediated" UI w3c/webauthn#1576

Merged

nsatragno reviewed Feb 23, 2021

View reviewed changes

index.src.html Outdated Show resolved Hide resolved

index.src.html Outdated Show resolved Hide resolved

index.src.html Outdated Show resolved Hide resolved

equalsJeffH marked this pull request as draft March 11, 2021 23:53

MasterKale reviewed Apr 19, 2021

View reviewed changes

index.src.html Outdated Show resolved Hide resolved

w3c deleted a comment Oct 18, 2021

equalsJeffH mentioned this pull request Oct 18, 2021

Update Conditional UI mediation #170

Merged

equalsJeffH and others added 6 commits October 18, 2021 14:27

merge from main and fix conflict

8009c48

Merge branch 'main' into jeffh-add-conditional-mediation-approach

0ad1ff8

it is {{conditional}} not [=conditional=]

67f5689

oops, it is really {{CredentialMediationRequirement/conditional}}

2209e8f

Make several improvements

3537786

* Fix some links * Discover availability by calling a method on each Credential interface object to discriminate between credential types * Instruct the browser to avoid revealing credential availability when using conditional UI on error.

Remove index.html

9dc6bc8

I think it was brought by an earlier merge commit. Should be fine if we squash and rebase this into main later.

nsatragno force-pushed the jeffh-add-conditional-mediation-approach branch from 535006d to 9dc6bc8 Compare October 25, 2021 19:37

Do not eat up errors.

9d9300b

equalsJeffH commented Oct 30, 2021

View reviewed changes

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

equalsJeffH and others added 2 commits November 1, 2021 10:13

Merge branch 'main' into jeffh-add-conditional-mediation-approach

590437a

Apply suggestions from code review

c8c709e

Co-authored-by: =JeffH <jdhodges@google.com>

nsatragno self-assigned this Nov 3, 2021

nsatragno marked this pull request as ready for review November 3, 2021 19:43

equalsJeffH requested a review from dveditz November 3, 2021 20:40

equalsJeffH requested a review from jyasskin November 3, 2021 20:40

jyasskin reviewed Nov 4, 2021

View reviewed changes

Address comments.

b8624db

equalsJeffH requested a review from mikewest November 4, 2021 16:05

equalsJeffH mentioned this pull request Nov 4, 2021

Add support for non-modal UI w3c/webauthn#1545

Closed

jyasskin reviewed Nov 4, 2021

View reviewed changes

nsatragno and others added 2 commits November 9, 2021 11:48

Apply suggestions from code review

146e74c

Co-authored-by: Jeffrey Yasskin <jyasskin@chromium.org>

Address comment.

5d07770

jyasskin approved these changes Nov 9, 2021

View reviewed changes

nsatragno mentioned this pull request Nov 10, 2021

Fix the "aborted flag" reference #175

Closed

nsatragno force-pushed the jeffh-add-conditional-mediation-approach branch from 8fa9b22 to 5d07770 Compare November 10, 2021 16:03

merge from main and fix conflicts

f970f51

equalsJeffH commented Nov 12, 2021

View reviewed changes

nsatragno mentioned this pull request Nov 25, 2021

Credential Management: Conditional Mediation w3ctag/design-reviews#692

Closed

1 task

nsatragno merged commit b273b4a into main Dec 6, 2021

nsatragno deleted the jeffh-add-conditional-mediation-approach branch December 6, 2021 21:40

github-actions bot added a commit that referenced this pull request Dec 6, 2021

Add "Conditional Mediation" (#155)

258c955

SHA: b273b4a Reason: push, by @nsatragno Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>

equalsJeffH mentioned this pull request Dec 9, 2021

Possible experiences in a future WebAuthn w3c/webauthn#1637

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add "conditional mediation" #155

Add "conditional mediation" #155

equalsJeffH commented Feb 18, 2021 •

edited by pr-preview bot

Loading

mikewest commented Feb 18, 2021

nsatragno left a comment

equalsJeffH commented Apr 20, 2021

nsatragno commented Oct 25, 2021

equalsJeffH left a comment

jyasskin Nov 3, 2021

nsatragno Nov 4, 2021

jyasskin Nov 4, 2021

nsatragno Nov 9, 2021

jyasskin Nov 4, 2021

jyasskin left a comment

nsatragno commented Nov 9, 2021 •

edited

Loading

equalsJeffH commented Nov 12, 2021

equalsJeffH left a comment

Add "conditional mediation" #155

Add "conditional mediation" #155

Conversation

equalsJeffH commented Feb 18, 2021 • edited by pr-preview bot Loading

mikewest commented Feb 18, 2021

nsatragno left a comment

Choose a reason for hiding this comment

equalsJeffH commented Apr 20, 2021

nsatragno commented Oct 25, 2021

equalsJeffH left a comment

Choose a reason for hiding this comment

jyasskin Nov 3, 2021

Choose a reason for hiding this comment

nsatragno Nov 4, 2021

Choose a reason for hiding this comment

jyasskin Nov 4, 2021

Choose a reason for hiding this comment

nsatragno Nov 9, 2021

Choose a reason for hiding this comment

jyasskin Nov 4, 2021

Choose a reason for hiding this comment

jyasskin left a comment

Choose a reason for hiding this comment

nsatragno commented Nov 9, 2021 • edited Loading

equalsJeffH commented Nov 12, 2021

equalsJeffH left a comment

Choose a reason for hiding this comment

equalsJeffH commented Feb 18, 2021 •

edited by pr-preview bot

Loading

nsatragno commented Nov 9, 2021 •

edited

Loading