Make create() and get() abortable #544

AngeloKai · 2017-09-06T00:10:55Z

NOTE: this is closely related to: w3c/webappsec-credential-management#110 (and formerly #104), which is now (as of 2-Nov-2017) merged to master.

As @leshi explained to me, Google's current two step verification step involves prompting a wizard such as this.. If the user clicks cancel on the wizard, the UA needs to withdraw all the calls to authenticators, which should stop blinking subsequently. I'd call out the PR is not only to fulfill this niche use case but to give us a clean interface overall.

The PR makes use of the https://dom.spec.whatwg.org/#abortsignal-aborted-flag) object to achieve such functionality. This would be an addition to the API interface. AFAIK, both Edge and Firefox have implemented AbortSignal. Chrome is working on the feature too.

There is a corresponding PR standing at CredMan land: w3c/webappsec-credential-management#104. The CredMan PR adds the abort functionality to the CredMan surface, though this may not be needed depending on @mikewest and Dominic's preference.

The AbortSignal feature is new to HTML overall. Fetch is the first one to utilize the feature: whatwg/fetch#447. However, we can't completely follow fetch because there are still nuances here and there.

Preview | Diff

…3c#466

…tly 1 member fixes w3c#472

…3c#466

…tly 1 member fixes w3c#472

AngeloKai · 2017-09-06T00:11:55Z

The PR is based on JeffH's recent changes so there are many commits attached to this. After JeffH's PR is merged, I will rebase to make things clearer.

AngeloKai · 2017-09-06T00:14:36Z

The PR aims at addressing the following issues: #537, #380, #283, #316, and finally #292.

#292 will need both this PR and #498

jyasskin

I probably won't get a chance to review the fixes to my comments before TPAC. @jcjones or one of the other reviewers is welcome to declare my comments fixed.

jyasskin · 2017-11-01T22:10:47Z

index.bs

@@ -570,7 +570,9 @@ To support obtaining assertions via {{CredentialsContainer/get()|navigator.crede
 {{CredentialsContainer/create()|navigator.credentials.create()}} to request the creation of a new [=credential key pair=]
 and {{PublicKeyCredential}}, managed by an [=authenticator=].
 On success, the returned {{promise}} will be resolved with a {{PublicKeyCredential}} containing an
-{{AuthenticatorAttestationResponse}} object.
+{{AuthenticatorAttestationResponse}} object. If an {{AbortSignal}} object is passed in,
+the developer can abort on going {{CredentialsContainer/create()|navigator.credentials.create()}} operation with 


"can abort an ongoing"?

jyasskin · 2017-11-01T22:11:13Z

index.bs

@@ -735,6 +737,12 @@ When this method is invoked, the user agent MUST execute the following algorithm
        ::  [=set/For each=] |authenticator| in |issuedRequests| invoke the [=authenticatorCancel=] operation on |authenticator|
            and [=set/remove=] |authenticator| from |issuedRequests|.

+        :   If the <code>|options|.{{MakePublicKeyCredentialOptions/signal}}</code> is [=present=] and and the 


"If <code>|options|..." and remove the second "and and".

jyasskin · 2017-11-01T22:11:33Z

index.bs

@@ -735,6 +737,12 @@ When this method is invoked, the user agent MUST execute the following algorithm
        ::  [=set/For each=] |authenticator| in |issuedRequests| invoke the [=authenticatorCancel=] operation on |authenticator|
            and [=set/remove=] |authenticator| from |issuedRequests|.

+        :   If the <code>|options|.{{MakePublicKeyCredentialOptions/signal}}</code> is [=present=] and and the 
+            [=AbortSignal/aborted flag=] is set to false,


If this is the step intended to handle the developer aborting the create() call, I think you mean "is set to true" here.

jyasskin · 2017-11-01T22:13:19Z

index.bs

@@ -735,6 +737,12 @@ When this method is invoked, the user agent MUST execute the following algorithm
        ::  [=set/For each=] |authenticator| in |issuedRequests| invoke the [=authenticatorCancel=] operation on |authenticator|
            and [=set/remove=] |authenticator| from |issuedRequests|.

+        :   If the <code>|options|.{{MakePublicKeyCredentialOptions/signal}}</code> is [=present=] and and the 


Assuming you keep Jeff's suggestion to put the AbortSignal in CredentialCreationOptions instead of MakePublicKeyCredentialOptions (which I agree with, as mentioned in w3c/webappsec-credential-management#104), you'll need to pull the signal out before step 2 where you overwrite |options|.

AbortSignal was put in credman's CredentialCreationOptions via credman PR w3c/webappsec-credential-management#110

jyasskin · 2017-11-01T22:16:34Z

index.bs

+        :   If the <code>|options|.{{MakePublicKeyCredentialOptions/signal}}</code> is [=present=] and and the 
+            [=AbortSignal/aborted flag=] is set to false,
+        ::  [=set/For each=] |authenticator| in |issuedRequests| invoke the [=authenticatorCancel=] operation on |authenticator|
+            and [=set/remove=] |authenticator| from |issuedRequests|.</dd> Set the [=AbortSignal/aborted flag=] be true. Then 


But then I'm not sure why you'd change the aborted flag in this step.

jyasskin · 2017-11-01T22:40:38Z

index.bs

@@ -3754,6 +3781,39 @@ extension for transaction authorization.
    });
 </pre>

+## Aborting Authentication Operations ## {#sample-aborting}
+
+The below example showcases how a developer may use the AbortSignal parameter to abort an


s/showcases/shows/?

jyasskin · 2017-11-01T22:41:06Z

index.bs

+        .then(function (attestation) {
+            // Register the user. 
+        }).catch(function (error) {
+            if (error = "AbortError") {


error.name ==, right?

jyasskin · 2017-11-01T22:41:32Z

index.bs

+        });
+
+    // Assume widget shows up whenever auth occurs. 
+    if (widget = "disappear") {


jyasskin · 2017-11-01T22:42:56Z

index.bs

+    if (widget = "disappear") {
+        authAbortSignal.abort(); 
+
+        authAbortSignal.onabort = function () {


This function will never run, since https://dom.spec.whatwg.org/#abortsignal-signal-abort fired the event 2 lines up.

jyasskin · 2017-11-01T22:43:55Z

index.bs

+            // Register the user. 
+        }).catch(function (error) {
+            if (error = "AbortError") {
+                // Let the server know a key hasn't been created. 


The exact error type probably doesn't affect what the server gets told. It might affect the UI shown to the user?

AngeloKai · 2017-11-02T18:02:48Z

Btw the corresponding credman PR w3c/webappsec-credential-management#110 is merged.

equalsJeffH · 2017-11-03T18:04:11Z

index.bs

+            [=AbortSignal/aborted flag=] is set to false,
+        ::  [=set/For each=] |authenticator| in |issuedRequests| invoke the [=authenticatorCancel=] operation on |authenticator|
+            and [=set/remove=] |authenticator| from |issuedRequests|.</dd> Set the [=AbortSignal/aborted flag=] be true. Then 
+            reject the promise with an {{AbortError}} {{DOMException}}. 


whoops, we can't reject promises here because we don't have any promises -- this is a synchronous alg that's called on a background thread and (now) returns a (callback) alg to the caller. unfortunately, we've been working at a hectic pace and we have some loose ends in the spec, like mentioning promises where we now ought not. See issue #671.

It seems like a bigger problem to fix here. If we do this here, it may be inconsistent. As jake mentioned in the other comment, our parallel calls here are problematic. I can work with you to submit a PR to clean this up.

equalsJeffH

have some additional comments, plus agree with @jyasskin's comments/review. It seems this abort notion is a tricky thing to incorporate.

equalsJeffH · 2017-11-03T19:23:29Z

index.bs

+    }
+
+    var options = {
+        // A list of options. 


should signal be in options like so:

var options = { signal: authAbortSignal`, // additional options.... }

..? i.e., per credman now?

@equalsJeffH I think authenticatorCancel should be sufficient to address the issue for now. We may need a more polished algorithm overall to describe the life cycle. But that is something we can do at PR time. I can collaborate with you and @jyasskin on that.

equalsJeffH · 2017-11-03T19:24:47Z

index.bs

+
+    navigator.credentials.create({
+        publicKey: options, 
+        signal: authAbortSignal})


signal is a part of options yes?
so this would be..

navigator.credentials.create({ publicKey: options })

..?

the current design is that the signal can be a separate field (as described in the example and in my other comment).

yeah, doh, nevermind, sorry.

equalsJeffH · 2017-11-03T19:43:41Z

index.bs

@@ -974,7 +982,8 @@ method is invoked, the user agent MUST:

    1. [=set/Append=] |authenticator| to |issuedRequests|.

-1. Execute the following steps [=in parallel=]. The [=task source=] for these [=tasks=] is the [=dom manipulation task source=].
+1. Execute the following steps [=in parallel=] but abort if the steps are [=terminated=]. The [=task source=] for these [=tasks=]


agree w/@jyasskin that we may need to employ https://dom.spec.whatwg.org/#abortsignal-add techniques similar to how fetch handles aborting. It also seems that fetch has a notion of "An ongoing fetch can be terminated
with flag aborted" which is local to the fetch spec, and not part of the DOM's "aborting ongoing activities". I wonder whether we might need to implement something similar.

AngeloKai · 2017-11-03T21:36:27Z

@equalsJeffH @jyasskin @jcjones please review. This is now fully polished. Previously when I pushed commit, they were work in progress.

equalsJeffH

Is getting closer, but there is impedance mismatch between proposed changes here and how credman is invoking webauthn (webauthn does not have promises to reject). credman has abortSignal nominally incorporated, but its algs may need to employ it in a more fine grained fashion.

equalsJeffH · 2017-11-03T22:38:18Z

index.bs

@@ -547,6 +557,7 @@ the {{CredentialCreationOptions}} dictionary as follows:
 <pre class="idl">
    partial dictionary CredentialCreationOptions {
        MakePublicKeyCredentialOptions      publicKey;
+        AbortSignal                         signal;


hm, webappsec-credential-management/#dictdef-credentialcreationoptions already has a ssignal member of type AbortSignal

I added this to deal with a bikeshed linking error. I can take it out.

equalsJeffH · 2017-11-03T22:39:05Z

index.bs

@@ -558,6 +569,7 @@ To support obtaining assertions via {{CredentialsContainer/get()|navigator.crede
 <pre class="idl">
    partial dictionary CredentialRequestOptions {
        PublicKeyCredentialRequestOptions      publicKey;
+        AbortSignal                            signal;


ditto: webappsec-credential-management/#dictdef-credentialrequestoptions

equalsJeffH · 2017-11-03T22:43:01Z

index.bs

@@ -688,6 +702,10 @@ 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. If the <code>|options|.{{CredentialCreationOptions/signal}}</code> is [=present=] and its 
+    [=AbortSignal/aborted flag=] is set to true, return [=a promise rejected with=] an "{{AbortError}}" 


we cannot return a promise here because we're just a synchronous algorithm; the Promise resolution/rejection is handled by navigator.credentials.create(). see w3c/webappsec-credential-management#100

webappsec-credential-management/#algorithm-create already is checking for options.signal's aborted flag being set, but may need to also be doing it in the "in parallel" section. maybe we need to have webauthn's [[Create]]() alg simply return as quickly as possible if an abort situation arises, and then have webappsec-credential-management/#algorithm-create check for abort right after webauthn's [[Create]]() alg returns to it ?

Does the note in the abort section help explain this? The reality is abort is a new concept to web platform APIs and our current spec languages don't handle this that well. This is why the section mentions we are checking the aborted flag three times to "fake" the effect of rejecting immediately.

I'm not sure. this will ostensibly get review from folks who are more familiar with specifying this new abort hotness and we will see.

equalsJeffH · 2017-11-03T22:47:07Z

index.bs

+        :   If the <code>|options|.{{CredentialCreationOptions/signal}}</code> is [=present=] and its 
+            [=AbortSignal/aborted flag=] is set to true, 
+        ::  [=set/For each=] |authenticator| in |issuedRequests| invoke the [=authenticatorCancel=]
+            operation on |authenticator| and [=set/remove=] |authenticator| from |issuedRequests|. Then return [=a promise rejected with=] 


ditto: can't reject promise here. may need to use https://dom.spec.whatwg.org/#abortsignal-add magic here a la how fetch is doing it. e.g. see step 8 of https://fetch.spec.whatwg.org/#fetch-method

equalsJeffH · 2017-11-03T22:47:25Z

index.bs

@@ -906,6 +930,10 @@ method is invoked, the user agent MUST:

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

+1. If the <code>|options|.{{CredentialRequestOptions/signal}}</code> is [=present=] and its 
+    [=AbortSignal/aborted flag=] is set to true, return [=a promise rejected with=] an "{{AbortError}}" 


ditto

webappsec-credential-management/#algorithm-request is checking options.signal's aborted flag. same thoughts as above, i.e., [[DiscoverFromExternalSource]]() needs to promptly return and webappsec-credential-management/#algorithm-request ought to check immediately for abort situation?

equalsJeffH · 2017-11-03T22:48:49Z

index.bs

+                true,
+            ::  [=set/For each=] |authenticator| in |issuedRequests| invoke the [=authenticatorCancel=] operation on |authenticator|
+                and [=set/remove=] |authenticator| from |issuedRequests|. Then 
+                return [=a promise rejected with=] an "{{AbortError}}" {{DOMException}}.


ditto wrt no promise to return and perhaps needing abortsignal-add magic

equalsJeffH · 2017-11-03T22:49:46Z

index.bs

+
+    navigator.credentials.create({
+        publicKey: options, 
+        signal: authAbortSignal})


yeah, doh, nevermind, sorry.

equalsJeffH · 2017-11-03T23:02:20Z

index.bs

+    Issue: The WHATWG HTML WG is discussing whether to provide a hook when a browsing context gains or
+    loses focuses. If a hook is provided, the above paragraph will be updated to include the hook.
+    See [WHATWG HTML WG Issue #2711](https://github.com/whatwg/html/issues/2711) for more details. 
+


above is helpful (nice!) but will need updating per however we end up polishing this abort functionality between webauthn & credman.

equalsJeffH · 2017-11-03T23:07:11Z

further review: #544 (review) Thanks for tackling this @AngeloKai -- it's a gnarly one.

jeffrey said he likely would get to the review in near term but anyone else can review this on behalf of him to check if his points got addressed.

AngeloKai · 2017-11-03T23:28:45Z

s/jeffrey said he likely would get to the review in near term/jeffrey said he won't able to get the review in near term/

AngeloKai · 2017-11-03T23:36:01Z

As a related note, Firefox has PRs in review to abort webauthn codes when the user switches tabs: https://bugzilla.mozilla.org/show_bug.cgi?id=1383799 and https://bugzilla.mozilla.org/show_bug.cgi?id=1409202. Edge kind of has an abort mechanism in place as well. This PR is just exposing the API to developers to let them abort.

jcjones

This looks ready to merge to me. Major kudos to Angelo for this.

jcjones · 2017-11-07T03:55:39Z

I think this needs your re-review, @equalsJeffH, and then merge it if you're good with it.

equalsJeffH · 2017-11-08T23:40:18Z

given @jcjones' approval, i suppose we can merge this -- though, there are IMV unresolved aspects of it such as @jyasskin's comment #544 (comment) and these other ones:

#544 (comment)
#544 (comment)
#544 (comment)

this seems good enough for now

jcjones · 2017-11-09T01:40:19Z

Thanks, I agree. It gets the functionality in here, and IMO the right wording for what should happen on aborts will be more clear when we actually try to do the thing. (Which is ... soon!)

JeffH and others added 26 commits June 16, 2017 14:43

do not call authenticatorMakeCredential() with separate |rpId| fixes w…

ed25b6c

…3c#466

credID returned by authnrGetAssn() is optional if allowCreds has exac…

31a8f85

…tly 1 member fixes w3c#472

fixup global object reference per domenic, improves w3c#472

9c3940f

indent 4.1.4 step 18et al to clarify relation to prior step

be43665

fix line indent

9ce4331

Merge branch 'master' into jeffh-fixup-algs-contd-3

78d6d2c

Merge branch 'master' into jeffh-fixup-algs-contd-3

9ddf29d

do not call authenticatorMakeCredential() with separate |rpId| fixes w…

5f543b8

…3c#466

credID returned by authnrGetAssn() is optional if allowCreds has exac…

d8fd53b

…tly 1 member fixes w3c#472

fixup global object reference per domenic, improves w3c#472

c9b40d5

indent 4.1.4 step 18et al to clarify relation to prior step

d492a50

fix line indent

2934f9e

post rebase-on-master, fix dangling MakeCredentialOptions

a1be0f6

fix error in resolving rebase conflicts

28f52af

further rebase conflict resolution error fixups

5fada18

merge with this remote branch via pull, somehow they were out of sync

6a011d2

convert switch steps to colon-denotation

5fb2f8c

tag 'while'

51706b3

primary changes for improving w3c#472 mostly complete

5e4a7b0

further issue w3c#472 cleanups

d6d6c19

del 'cancel the timer' from #creatCredential fixes w3c#535

a76de0b

polish constructResultantCredentialCallback method description

97f0ae2

marked authenticator model section as non-normative

8622aea

marked relying party operation section as non-normative

e8ca2be

fix proper subset tweak

84c12bb

Added abort signal object and steps to webauthn

2263888

AngeloKai requested review from jyasskin and equalsJeffH September 6, 2017 00:11

jyasskin previously requested changes Nov 1, 2017

View reviewed changes

AngeloKai added 2 commits November 2, 2017 12:37

updated grammars of the example based on feedback

ad5a888

update example text

c142b5a

equalsJeffH reviewed Nov 3, 2017

View reviewed changes

equalsJeffH suggested changes Nov 3, 2017

View reviewed changes

AngeloKai added 4 commits November 3, 2017 12:48

Updated with the section on switching tab; complete the PR

3fa4a3f

minor tweak

14a7659

finished polishing the spec

2cb344c

whoops one leftover

2430dcd

finally figured out how to remove last two linking errors

ad85e75

equalsJeffH previously requested changes Nov 3, 2017

View reviewed changes

take out abortsignal from extension; edit promise rejection

78b0912

jcjones approved these changes Nov 7, 2017

View reviewed changes

equalsJeffH merged commit 931b46e into w3c:master Nov 8, 2017

equalsJeffH deleted the abort branch November 8, 2017 23:42

WebAuthnBot pushed a commit that referenced this pull request Nov 9, 2017

Built by Travis-CI: 931b46e Make create() and get() abortable (#544)

0eeb1b1

This was referenced Nov 9, 2017

Should the WebAuth API have a cancel() method? #380

Closed

Make create() and get() abortable #537

Closed

AngeloKai mentioned this pull request Nov 9, 2017

Define what happens when the Document loses focus #316

Closed

felixfbecker mentioned this pull request Aug 28, 2018

Pass AbortSignal to callbacks passed to defer(), switchMap() etc ReactiveX/rxjs#4051

Closed

Make create() and get() abortable #544

Make create() and get() abortable #544

Conversation

AngeloKai commented Sep 6, 2017 • edited by pr-preview bot Loading

AngeloKai commented Sep 6, 2017

AngeloKai commented Sep 6, 2017

jyasskin left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jyasskin Nov 1, 2017 • edited Loading

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AngeloKai commented Nov 2, 2017

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

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

AngeloKai commented Nov 3, 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

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 commented Nov 3, 2017

AngeloKai commented Nov 3, 2017

AngeloKai commented Nov 3, 2017

jcjones left a comment

Choose a reason for hiding this comment

jcjones commented Nov 7, 2017

equalsJeffH commented Nov 8, 2017

jcjones commented Nov 9, 2017

AngeloKai commented Sep 6, 2017 •

edited by pr-preview bot

Loading

jyasskin Nov 1, 2017 •

edited

Loading