Fix #574 - Change language for Create and Get to support hotplugging #655

jcjones · 2017-10-18T20:47:59Z

This is an incomplete fix; a full fix is intended to be handled [by the resolution to] Issue #613.

This reorders [a portion of the guts of] the Create and Get operations to
indicate that the algorithms for
interacting with devices should be applied as devices are hotplugged / arrive.
It does not specify what happens when devices are removed, nor does it use
precise language. I'm not sure what language would be appropriate in this world,
so this patch is just to make things "better" not "correct".

Preview | Diff

equalsJeffH

this looks nominally OK. is it enough to do this for WD-07 and resolve issue #613 for CR (since at least one implementation presently handles this anyway) ?

equalsJeffH · 2017-10-18T22:27:23Z

index.bs

+
+    Note: The definitions of "overall lifetime" and "becomes available" are intended to represent how
+    devices are hotplugged into browsers, and are under-specified. Resolving this with good definitions
+    or some other means will be handled in [Issue #613](https://github.com/w3c/webauthn/issues/613).


pedantic nit: s/handled in/addressed by resolving/

ditto wrt "Issue:"

equalsJeffH · 2017-10-18T22:27:36Z

index.bs

-    "{{ConstraintError}}", and terminate this algoritm.
+    Note: The definitions of "overall lifetime" and "becomes available" are intended to represent how
+    devices are hotplugged into browsers, and are under-specified. Resolving this with good definitions
+    or some other means will be handled in [Issue #613](https://github.com/w3c/webauthn/issues/613).


pedantic nit: s/handled in/addressed by resolving/

also, rather than a "Note:", perhaps should use "Issue:" here ?

equalsJeffH · 2017-10-18T22:41:45Z

index.bs

-    1. If {{AuthenticatorSelectionCriteria/requireUserVerification}} is set to |true| and the
-        |authenticator| is not capable of performing [=user verification=], [=iteration/continue=].
-    1. [=set/Append=] |authenticator| to |selectedAuthenticators|.
+1. [=set/For each=] |authenticator| that becomes available on this platform within the overall


Offhand, I'm thinking that the "Start a timer for adjustedTimeout milliseconds", that is set down below in this alg, would come up to around this here step, which would then define "overall lifetime". but perhaps that's something to wait to incorp in the resolution of issue #613.

equalsJeffH · 2017-10-18T22:42:45Z

index.bs

 1. Let |authenticator| be a platform-specific handle whose value identifies an [=authenticator=].

-1. For each |authenticator| currently available on this platform, perform the following steps:
+1. For each |authenticator| that becomes available on this platform within the overall lifetime of


ditto here wrt "Start a timer for adjustedTimeout milliseconds" being done here.

equalsJeffH

LGTM. it's fine to merge this now as it seems to not interfere with #498.

equalsJeffH · 2017-10-19T23:33:05Z

This "improves" issue #613.

equalsJeffH · 2017-10-20T15:13:10Z

would be good to get at least one other review on this by someone(s) other than @jcjones and myself before we merge!
cc: @akshayku @AngeloKai @jyasskin

AngeloKai · 2017-10-20T16:44:39Z

The PR looks nominally ok given it is one step toward the right direction. I have the same question as @equalsJeffH about whether we think the PR is good enough to address hot-plugged authenticator before CR. If so, I encourage us to think about what error should be thrown if there is only one authenticator connected via NFC and the user moves the authenticator in the middle. AbortError seems like a reasonable candidate here. Developers can act based on the error to ask the user to put the authenticator back on again.

AngeloKai

Assuming this is just the beginning of addressing hot-plugged authenticator, I will approve this.

AngeloKai · 2017-10-20T16:37:26Z

index.bs

-1. If |currentlyAvailableAuthenticators| [=list/is empty=], return a {{DOMException}} whose name is
-    "{{NotFoundError}}", and terminate this algorithm.
+    Issue: The definitions of "lifetime of" and "becomes available" are intended to represent how
+    devices are hotplugged into browsers, and are under-specified. Resolving this with good definitions


If we intend to address what hotplug is after CR, I'd recommend giving an example such as NFC which people relate to more.

AngeloKai · 2017-10-20T16:39:54Z

index.bs

@@ -682,30 +682,25 @@ When this method is invoked, the user agent MUST execute the following algorithm

 1. Let |clientDataHash| be the [=hash of the serialized client data=] represented by |clientDataJSON|.

-1. Let |currentlyAvailableAuthenticators| be a new [=ordered set=] consisting of all [=authenticators=]
-    currently available on this platform.
+1. Let |lifetimeTimer| be a timer for |adjustedTimeout| milliseconds.


Do we need to add a new name? We can just change adjustedTimeout to lifetimeTimer.

Note: this is more of a nitpick. Feel free to merge the PR without changing the adjustedTimeout naming.

@equalsJeffH

This is an incomplete fix; a full fix is intended to be handled in Issue w3c#613. This reorders the Create and Get operations to indicate that the algorithms for interacting with devices should be applied as devices are hotplugged / arrive. It does not specify what happens when devices are removed, nor does it use precise language. I'm not sure what language would be appropriate in this world, so this patch is just to make things "better" not "correct". Resolve @equalsJeffH's comments: 1. Define |lifetimeTimer| and make it available to the line that starts the hotplugging 2. Use the |lifetimeTimer| for references later in those algorithms to reduce confusion 3. Reword the Notes 4. Change the Notes to Issues

jcjones · 2017-10-20T17:02:58Z

Thanks for the review, @AngeloKai. I've made both sets of corrections. Assuming it passes bikeshed, I think it's ready to merge - maybe with one last look by @equalsJeffH ?

equalsJeffH

a few pedantic, and optional, nits. otherwise LGTM.

equalsJeffH · 2017-10-20T21:55:07Z

index.bs

    <dl class="switch">

-        :   If the |adjustedTimeout| timer expires,
+        :   If the |lifetimeTimer| timer expires,


s/If the |lifetimeTimer| timer/If |lifetimeTimer|/

equalsJeffH · 2017-10-20T21:56:39Z

index.bs

-1. [=While=] |issuedRequests| [=list/is not empty=], perform the following actions depending upon the |adjustedTimeout| timer
-    and responses from the authenticators:
+1. [=While=] |issuedRequests| [=list/is not empty=], perform the following actions depending upon the
+    |lifetimeTimer| timer and responses from the authenticators:


s/the |lifetimeTimer| timer/|lifetimeTimer|/

equalsJeffH · 2017-10-20T21:57:21Z

index.bs

-    these [=tasks=] is the [=dom manipulation task source=].
-
-    1. While |issuedRequests| [=list/is not empty=], perform the following actions depending upon the |adjustedTimeout| timer
+    1. While |issuedRequests| [=list/is not empty=], perform the following actions depending upon the |lifetimeTimer| timer


s/the |lifetimeTimer| timer/|lifetimeTimer|/

equalsJeffH · 2017-10-20T21:57:49Z

index.bs

        and responses from the authenticators:

        <dl class="switch">

-            :   If the |adjustedTimeout| timer expires,
+            :   If the |lifetimeTimer| timer expires,


s/If the |lifetimeTimer| timer/If |lifetimeTimer|/

equalsJeffH · 2017-10-20T21:58:48Z

re-reviewed, thanks!

…hotplugging

jcjones added subtype:impl-cons priority:implementation type:technical labels Oct 18, 2017

jcjones added this to the WD-07 milestone Oct 18, 2017

jcjones requested review from equalsJeffH and AngeloKai October 18, 2017 20:48

jcjones force-pushed the 574-hotplugging branch from d176e5b to 25a4428 Compare October 18, 2017 20:52

equalsJeffH approved these changes Oct 18, 2017

View reviewed changes

jcjones force-pushed the 574-hotplugging branch from cf04b29 to 5f2dd9b Compare October 19, 2017 22:55

equalsJeffH requested review from jyasskin and akshayku October 20, 2017 15:08

AngeloKai approved these changes Oct 20, 2017

View reviewed changes

jcjones added 3 commits October 20, 2017 10:02

Address @AngeloKai's comments

7bc3a3e

Address @AngeloKai's comments about lifetimeTimer

df88d55

jcjones force-pushed the 574-hotplugging branch from 3f52c80 to df88d55 Compare October 20, 2017 17:02

equalsJeffH approved these changes Oct 20, 2017

View reviewed changes

Last minute nitfixes

83da7ff

jcjones merged commit 579f6cf into w3c:master Oct 20, 2017

jcjones deleted the 574-hotplugging branch October 20, 2017 22:05

WebAuthnBot pushed a commit that referenced this pull request Oct 20, 2017

Built by Travis-CI: 579f6cf Merge pull request #655 from jcjones/574-…

004f905

…hotplugging

emlun mentioned this pull request Nov 29, 2017

Authenticator selection extension - should makeCredential fail if no specified authenticator can be found? #227

Closed

emlun mentioned this pull request Jan 17, 2018

Privacy concerns with blacklist/whitelist #184

Closed

jcjones mentioned this pull request Jan 19, 2018

Allow hot-plugged authenticators? #613

Closed

equalsJeffH mentioned this pull request Jan 19, 2018

document preventions of RP-driven de-anonymization attempts in privacy-cons #743

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix #574 - Change language for Create and Get to support hotplugging #655

Fix #574 - Change language for Create and Get to support hotplugging #655

jcjones commented Oct 18, 2017 •

edited by pr-preview bot

Loading

equalsJeffH left a comment

equalsJeffH Oct 18, 2017

equalsJeffH Oct 18, 2017

equalsJeffH Oct 18, 2017

equalsJeffH Oct 18, 2017

equalsJeffH Oct 18, 2017

equalsJeffH Oct 18, 2017

equalsJeffH left a comment

equalsJeffH commented Oct 19, 2017

equalsJeffH commented Oct 20, 2017

AngeloKai commented Oct 20, 2017 •

edited

Loading

AngeloKai left a comment

AngeloKai Oct 20, 2017

AngeloKai Oct 20, 2017

jcjones commented Oct 20, 2017

equalsJeffH left a comment

equalsJeffH Oct 20, 2017

equalsJeffH Oct 20, 2017

equalsJeffH Oct 20, 2017

equalsJeffH Oct 20, 2017

equalsJeffH commented Oct 20, 2017

Fix #574 - Change language for Create and Get to support hotplugging #655

Fix #574 - Change language for Create and Get to support hotplugging #655

Conversation

jcjones commented Oct 18, 2017 • edited by pr-preview bot Loading

equalsJeffH left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

equalsJeffH left a comment

Choose a reason for hiding this comment

equalsJeffH commented Oct 19, 2017

equalsJeffH commented Oct 20, 2017

AngeloKai commented Oct 20, 2017 • edited Loading

AngeloKai left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jcjones commented Oct 20, 2017

equalsJeffH left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

equalsJeffH commented Oct 20, 2017

jcjones commented Oct 18, 2017 •

edited by pr-preview bot

Loading

AngeloKai commented Oct 20, 2017 •

edited

Loading