Added aborted flag to the Create and DiscoverFromExternalSource methods #104

AngeloKai · 2017-09-05T21:57:11Z

This is to address w3c/webauthn#537. I will follow up soon with the corresponding webauthn PR.

Preview | Diff

battre

Looks good to me. It would be good if @mikewest could do a sanity check as well. He'll be back on Monday.

equalsJeffH · 2017-10-20T15:49:14Z

the corresponding webauthn PR is here: w3c/webauthn#544

jyasskin · 2017-10-23T17:56:37Z

index.src.html

@@ -52,6 +52,9 @@
 spec: FETCH; urlPrefix: https://fetch.spec.whatwg.org/
  type: dfn
    text: http-network-or-cache fetch; url: http-network-or-cache-fetch 
+spec: DOM; urlPrefix: https://dom.spec.whatwg.org/
+  type: interface
+    text: AbortSignal url: https://dom.spec.whatwg.org/#abortsignal


You don't need these 3 lines: Update bikeshed with

cd <bikeshed-dir> git pull --rebase bikeshed update

and AbortSignal will be there.

jyasskin · 2017-10-23T17:58:56Z

index.src.html

@@ -250,6 +254,12 @@
    ::  This attribute's getter returns the value of the object's [=interface object=]'s
        {{[[type]]}} slot, which specifies the kind of credential represented by this object.

+    :   <dfn attribute>signal</dfn>
+    ::  The credential object has an associated signal (an {{AbortSignal}} object), initially 
+        a new {{AbortSignal}} object. Developers can use AbortSignal to [=terminated|terminate=] an 


An AbortSignal attribute on the Credential would allow developers to respond to a Credential being aborted, not let them abort the .create() or .get() call.

Instead, you need to add an AbortSignal field to CredentialRequestOptions and CredentialCreationOptions, and thread that into [[Create]] and [[DiscoverFromExternalSource]]. See https://dom.spec.whatwg.org/#abortcontroller-api-integration for how to do that.

I'd lean toward not threading the AbortSignal into the returned Credential: if the caller aborts the .get() or .create() before it resolves, they won't get a Credential object at all.

AngeloKai · 2017-10-31T21:39:55Z

@jyasskin can you please review the change?

jyasskin

This looks technically sufficient for WebAuthn to start handling aborts. However, I think that Credential Manager should handle AbortSignals that were aborted before get() or create() was called. I can send a PR to do that on top of this PR.

jyasskin · 2017-11-01T04:06:49Z

index.src.html

@@ -428,7 +430,7 @@

        Note: This function was previously called `requireUserMediation()` which should be considered
        deprecated.
-  </div>
+      </div>


You probably shouldn't change this indentation in this PR.

jyasskin · 2017-11-01T04:07:32Z

index.src.html

    };
  </pre>
  <div dfn-for="CredentialRequestOptions" dfn-type="dict-member">
    :   <dfn>mediation</dfn>
    ::  This property specifies the mediation requirements for a given credential request. The
        meaning of each enum value is described below in {{CredentialMediationRequirement}}.
        Processing details are defined in [[#algorithm-request]].
+    :   <dfn>signal</dfn>
+    ::  This property lets the developer abort an ongoing 
+        {{[[DiscoverFromExternalSource]](options)}} operation and subequently all related 


spelling: subequently

jyasskin · 2017-11-01T15:51:29Z

index.src.html

+    :   <dfn>signal</dfn>
+    ::  This property lets the developer abort an ongoing 
+        {{[[DiscoverFromExternalSource]](options)}} operation and subequently all related 
+        [=authenticatorGetAssertion=] operations. Aborted promise would 


Credential Manager should probably only discuss webauthn in examples. I might write this description as

This property lets the developer abort an ongoing {{CredentialsContainer/get()}} operation. An aborted operation may complete normally (generally if the abort was received after the operation finished) or reject with an "{{AbortError}}" {{DOMException}}."

Sure. But I'd prefer calling out AuthenticatorMakeCredential because of the same logic I mentioned above. I'd prefer this PR to just webauthn and handle abort for Credman overall in another PR. I do appreciate the added detail about how an aborted operation may complete.

jyasskin · 2017-11-01T15:51:40Z

index.src.html

    };
  </pre>
+   <div dfn-for="CredentialCreationOptions" dfn-type="dict-member">
+    :   <dfn>signal</dfn>
+    ::  This property lets the developer to abort an ongoing {{Credential/[[Create]](options)}} operation


And similarly here.

jyasskin · 2017-11-01T15:52:57Z

index.src.html

@@ -282,7 +282,9 @@
  ### `Credential` Internal Methods ### {#credential-internal-methods}

  Each [=interface object=] created for interfaces which [=interface/inherit=] from {{Credential}}
-  defines several internal methods that allow retrieval and storage of {{Credential}} objects:
+  defines several internal methods that allow retrieval and storage of {{Credential}} objects. The 
+  {{Credential/[[Create]](options)}} method and {{Credential/[[DiscoverFromExternalSource]](options)}}


I wouldn't restrict this to these two methods. Any operations taking options receives an AbortSignal and can use it to abort. Today, only [[Create]] and [[DiscoverFromExternalSource]] use it that way, but the interface doesn't restrict that.

My personal preference is to let this PR do the job of unblocking WebAuthn. Then we can submit another PR that extends for password and federated credential. This keeps the progress cleaner. Plus if we are allowing the PR to extend for password and federated, I'd prefer having mikewest and dominic review the PR more thoroughly. But if this is just WebAuthn, I am fine merging this as it is.

jyasskin · 2017-11-01T16:20:49Z

.gitignore

@@ -0,0 +1 @@
+bikeshed/


This is much more for my personal benefit cuz I do keep bikeshed in a repo in the code. I will go ahead and take this out if it causes controversy.

jcjones · 2017-11-01T17:13:29Z

index.src.html

+   <div dfn-for="CredentialCreationOptions" dfn-type="dict-member">
+    :   <dfn>signal</dfn>
+    ::  This property lets the developer to abort an ongoing {{Credential/[[Create]](options)}} operation
+        and susquently all related [=authenticatorMakeCredential=] operations. Aborted romise would 


spelling: s/romise/promise/ and s/susquently/subsequently/

AngeloKai · 2017-11-02T01:00:12Z

@battre can you please merge the PR? I don't have write access. @samuelweiler @wseltzer @mikewest can you please grant me write access?

jcjones · 2017-11-02T01:43:25Z

Actually, I think we want to merge #110, which will also close this PR.

jyasskin · 2017-11-02T17:13:57Z

Merged as part of #110.

AngeloKai added 2 commits September 5, 2017 14:54

added aborted flag and gitignore file

bd5ed53

minor editorial tweak

e500f1b

AngeloKai mentioned this pull request Sep 6, 2017

Make create() and get() abortable w3c/webauthn#544

Merged

battre requested review from mikewest and battre September 6, 2017 11:38

battre approved these changes Sep 6, 2017

View reviewed changes

jyasskin requested changes Oct 23, 2017

View reviewed changes

AngeloKai added 4 commits October 27, 2017 10:34

fixed minor issues

a63ef12

added abort signal to request and create options

81bb577

updated src

bdac99b

minor grammatical fixes

35220d6

jyasskin approved these changes Nov 1, 2017

View reviewed changes

jcjones reviewed Nov 1, 2017

View reviewed changes

fix up minor details

a568516

jyasskin mentioned this pull request Nov 1, 2017

Handle the AbortSignal in get() and create() themselves. #110

Merged

jyasskin closed this Nov 2, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Added aborted flag to the Create and DiscoverFromExternalSource methods #104

Added aborted flag to the Create and DiscoverFromExternalSource methods #104

AngeloKai commented Sep 5, 2017 •

edited by pr-preview bot

battre left a comment

equalsJeffH commented Oct 20, 2017

jyasskin Oct 23, 2017

jyasskin Oct 23, 2017

AngeloKai commented Oct 31, 2017

jyasskin left a comment

jyasskin Nov 1, 2017

jyasskin Nov 1, 2017

jyasskin Nov 1, 2017

AngeloKai Nov 1, 2017

jyasskin Nov 1, 2017

jyasskin Nov 1, 2017

AngeloKai Nov 1, 2017

jyasskin Nov 1, 2017

AngeloKai Nov 1, 2017

jcjones Nov 1, 2017 •

edited

AngeloKai commented Nov 2, 2017

jcjones commented Nov 2, 2017

jyasskin commented Nov 2, 2017

		@@ -0,0 +1 @@
		bikeshed/

Added aborted flag to the Create and DiscoverFromExternalSource methods #104

Added aborted flag to the Create and DiscoverFromExternalSource methods #104

Conversation

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

battre left a comment

Choose a reason for hiding this comment

equalsJeffH commented Oct 20, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AngeloKai commented Oct 31, 2017

jyasskin 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

jcjones Nov 1, 2017 • edited

Choose a reason for hiding this comment

AngeloKai commented Nov 2, 2017

jcjones commented Nov 2, 2017

jyasskin commented Nov 2, 2017

AngeloKai commented Sep 5, 2017 •

edited by pr-preview bot

jcjones Nov 1, 2017 •

edited