fixup algs contd 3 by equalsJeffH · Pull Request #498 · w3c/webauthn

equalsJeffH · 2017-06-16T23:15:43Z

this builds on PRs #371 and #495. it resolves these issues:
improves #254 (#getAssertion section will be updated in a separate PR and issue #254 will be fixed at that time)
fix #466
fix #472
fix #536
fix #559

Preview | Diff

…466

…tly 1 member fixes #472

equalsJeffH

I have a couple items below that I'd like to alter in this and solicit feedback. Please see the below comments. thx.

equalsJeffH · 2017-06-19T22:32:19Z

+                            configuration knowledge of the appropriate transport to use with |authenticator| in making its
+                            selection.
+
+                            Then, using |transport|, invoke [=in parallel=] the [=authenticatorGetAssertion=] operation on


I am thinking that the "in parallel" clauses in these substeps (of current step 16) ought to be replaced by a single "in parallel" clause on Step 16 itself (new line 822, above).

If others agree, will do that in a subsequent commit.

hold on there pardner, see @domenic's thoughts in this comment on issue #472: #472 (comment)

I think it is fine as it is right now. Don't see the benefit of replacing the three in parallels with a single one.
Not that concrete implementations could still use a single in parallel invocation of authenticatorGet line-of-code and only prepare the parameters in the current is-empty/is-not-empty branches (as that is semantically equivalent). Even code generators might automatically perform this transformation.

actually, as suggested in #472 (comment) (mentioned above), we will remove those [=in parallel=] invocations because we are already on a background thread given how we are invoked from https://w3c.github.io/webappsec-credential-management/#algorithm-request

equalsJeffH · 2017-06-19T22:34:45Z

@@ -855,28 +883,29 @@ When this method is invoked, the user agent MUST execute the following algorithm

    <dl class="switch">


new line 881 above should be a substep of the step above it at new line 878. If others agree, will make that change in a subsequent commit.

I'm thinking this is a "go" given @domenic's thoughts in this comment on issue #472: #472 (comment) and other examples of how algs are written, e.g. in credman.

equalsJeffH · 2017-06-20T22:57:48Z

See @domenic's comments in issue #472. summary: this alg needs more work to get the thread-handling and JS-object construction stuff correct.

AngeloKai · 2017-06-22T23:44:33Z

Based on domenic's comment, there are still some works on how to address exception and creation of exception object. @equalsJeffH do you plan to do that in a separate PR? Or do you prefer to address them in this one altogether?

equalsJeffH · 2017-06-24T00:35:15Z

@AngeloKai asked

Based on domenic's comment, there are still some works on how to address exception and creation of exception object. @equalsJeffH do you plan to do that in a separate PR? Or do you prefer to address them in this one altogether?

thx, nominally I'd prefer to do it all in the context of this PR, unless there's some really good reasons to merge this as-is.

rlin1 · 2017-06-27T09:05:55Z

+    1. [=In parallel=], invoke the [=authenticatorMakeCredential=] operation on |authenticator| with
+        |clientDataHash|, <code>|options|.{{MakeCredentialOptions/rp}}</code>, <code>|options|.{{MakeCredentialOptions/user}}</code>,
        |normalizedParameters|, |excludeCredentialDescriptorList|, and |authenticatorExtensions| as parameters.
    1. [=set/Append=] |authenticator| to |issuedRequests|.


I would typically do this step before invoking the authenticatorMakeCredential in parallel in order to avoid race conditions, in which the authenticatorMakeCredential already returns and hence would invoke the "authenticator indicates success" code without having this task in the list.

hm, ok, what do others think, eg @jyasskin @jcjones ?

I think it's a readability improvement, but I believe it doesn't affect the correctness. Specifically, "If any authenticator indicates success," is checked later within the main thread, so there's no way that authenticatorMakeCredential returning early could run those steps before the authenticator is added to issuedRequests.

plus, this entire alg is now running "in parallel" and the [=in parallel=] on old line 678 is now gone. So I'm not sure there's anything to change here.

rlin1 · 2017-06-27T09:09:35Z

+                    authenticator is being asked to exercise any credential it may possess that is bound to the [=[RP]=].
        </dl>

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


see my previous command on reversing the order to appending to issuedRequests and invoking the request in parallel (and potential race conditions).

rlin1

I have added two comments about the order of statements.
Other than that: LGTM.

rlin1 · 2017-08-02T07:46:38Z

I think we found another limitation of the algorithm specified in section 4.1.4:
If the authenticator is used as first multifactor, i.e. user not yet known (e.g. no password auth, no cookies, ...), then the current algorithm sends no credentialIDs to the authenticator.
That is exactly the right thing to do. If the authenticator has a UI, the Authenticator could ask the user to select one of the existing credentialIDs for the specific rpID.

However, if the authenticator has no UI (e.g. like PQI My Lockey 360°), the authenticator would need platform support for the credentialID selection.

It would be great to add such support (e.g. by (1) letting the platform know whether or not the authenticator has a UI and (2) by letting the platform retrieve a list of credentialIDs from the authenticator - after user verification). If we feel that this is too much of a heavy lifting at this stage, we should at least document the supported scenarios and the limitations. At this stage it is not very obvious for a reader.

equalsJeffH · 2017-10-16T15:58:59Z

new, improved "return an algorithm" technique -- employed in the #createCredential section -- is ready to review.
Rendered HTML and text-only diffs from current state of master branch are here:

rendered html: http://kingsmountain.com/doc/diff/diff-webauthn-index-jeffh-fixup-algs-contd-3-7b272f1--from--index-master-ee174c2.html

rendered text-only: http://kingsmountain.com/doc/diff/diff-webauthn-index-jeffh-fixup-algs-contd-3-7b272f1--from--index-master-ee174c2.html.pdf

jyasskin · 2017-10-17T03:57:01Z

+
+                :   <code><dfn>extensionOutputsMap</dfn></code>
+                ::  whose value is an [=ordered map=] with [=map/keys=] of type [=extension identifier=]
+                    and [=map/values=] of type [=client extension output=]. <code>[=extensionOutputsMap=]</code>'s


Note that "client extension output" isn't a type. I believe the old wording was pretty close to fine, with "AuthenticationExtensions" replaced by "ordered map". The biggest problem there is that it's only implied that the map keeps the order provided by clientDataJSON.clientExtensions, but I wouldn't bother fixing that in this PR.

good catch, IIUC. Tho, this parag will now different because we merged PR #633 -- am updating this parag accordingly

well, actually ended up using the "old wording" you referred to. Tho this raises issue #657.

jyasskin · 2017-10-17T04:01:59Z

+                    the [=client extension outputs=], for each [=client extension=] in
+                    <code>{{AuthenticatorResponse/clientDataJSON}}.clientExtensions</code>.
+
+            1.  Let |constructCredentialAlg| be an algorithm that takes a [=environment settings object/global object=]


takes a [=global object=], which should resolve to https://html.spec.whatwg.org/multipage/webappapis.html#global-object.

jyasskin · 2017-10-17T04:05:56Z

+        :   If any |authenticator| indicates success,
+        ::  1.  [=set/Remove=] |authenticator| from |issuedRequests|.
+
+            1.  Let |credentialCreationData| be a [=struct=] whose [=items=] are:


This is fine, but I believe you'd also be fine just defining three variables with the values you've put into items here, and doing that would be a bit shorter.

Ok, tho will leave this as-is for now since it is OK and we want to get this merged.

jyasskin · 2017-10-17T04:11:05Z

+
+            1. Return |constructCredentialAlg| and terminate this algorithm.
+
+            Issue: Do we need to replicitly return both |constructCredentialAlg| and |credentialCreationData| here?


I believe that it's sufficient to return only constructCredentialAlg, on the theory that algorithm variables are captured by any subroutines that use them and only garbage-collected when all capturing routines are done. The main thing you have to guarantee is that no variable is used and modified in parallel, which we have to do by inspection whether or not you explicitly return the set of captured variables.

ok, thanks, I'll remove that "Issue:"

…CreationData|

equalsJeffH · 2017-10-19T19:56:20Z

per 18-Oct webauthn call, I've addressed @jyasskin's latest comments. This is ready to merge AFAIK. Since I did make some modest changes in response to the latter comments, perhaps someone(s) oughta review prior to merge?

Making similar changes to the #getAssertion section is yet to do in a separate soon-to-follow PR.

Regarding Rolf's oldish review that is requesting changes, It seems to me that there is nothing to do in response to it, unless I am misunderstanding things. I ping'd him on-list a few weeks ago about this, but have not heard back. So are we ok to "dismiss" is review in order to merge this?

thanks!

jcjones

I think this is ready to go. I'll go re-review Rolf's comments from earlier though and make a recommendation.

Since Rolf previously said "LGTM" other than the in-parallel concern, which has been handled, I think he would be in agreement to proceed.

#665) * actually improve #254, and fix #661 * DiscoFrmExtSource(options) -> (origin, options) * make [[DiscoFrmExtSource]]'s exposition match [[Create]]'s * deal with yet another fix #254 straggler in [[Create]] * get rid of |global| in [[DiscoFrmExtSource]] * remove 'in parallel' and 'global' stuff from #discover-from-external-source alg * work on #discover-from-external-source alg to improve #254 * finish (one hopes) work on #discover-from-external-source alg to fix #254 * minor editorial * repair #createCredential intro parag, improves issue #671 * complete fix #671

…ssertion section a la PR #498 (#665)

JeffH added 2 commits June 16, 2017 14:43

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

ed25b6c

…466

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

31a8f85

…tly 1 member fixes #472

equalsJeffH added subtype:algorithms/WebIDL type:technical labels Jun 16, 2017

equalsJeffH added this to the WD-06 milestone Jun 16, 2017

equalsJeffH self-assigned this Jun 16, 2017

equalsJeffH requested review from AngeloKai, jcjones, jyasskin, leshi, rlin1 and selfissued June 16, 2017 23:15

equalsJeffH commented Jun 19, 2017

View reviewed changes

fixup global object reference per domenic, improves #472

9c3940f

equalsJeffH changed the title ~~Jeffh fixup algs contd 3~~ fixup algs contd 3 Jun 21, 2017

equalsJeffH mentioned this pull request Jun 27, 2017

fixup algs contd 2 #495

Merged

rlin1 reviewed Jun 27, 2017

View reviewed changes

rlin1 previously requested changes Jun 27, 2017

View reviewed changes

JeffH added 3 commits June 28, 2017 15:41

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

nadalin modified the milestones: WD-07, WD-06 Jul 26, 2017

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

9ddf29d

equalsJeffH mentioned this pull request Aug 16, 2017

There is no "current settings object" in algorithm steps that are executing in parallel #254

Closed

equalsJeffH mentioned this pull request Oct 11, 2017

parameter lists in #createCredential and #op-make-cred do not match #536

Closed

JeffH added 4 commits October 13, 2017 12:42

merge from master fix conflicts

8fe61c6

re- fix #466 (due to merge-from-master), fix #536

a8799aa

eliminate callback and just return an algorithm from #createCredential

f5a08fb

merge from master

7b272f1

equalsJeffH mentioned this pull request Oct 16, 2017

Only exclude CredIDs matching the RPID #651

Merged

jyasskin approved these changes Oct 17, 2017

View reviewed changes

merge from master

6f51ef6

equalsJeffH mentioned this pull request Oct 18, 2017

authenticatorMakeCredential: Only exclude CredIDs matching the RPID #654

Closed

JeffH added 2 commits October 18, 2017 11:41

merge from master

928f83b

continue fix conflicts from merge from master

3d84d14

equalsJeffH mentioned this pull request Oct 18, 2017

authenticatorMakeCredential operation credential filtering #650

Closed

fix a couple of issue #466 stragglers in #op-make-cred

9d1db3a

equalsJeffH mentioned this pull request Oct 18, 2017

where does return value of PublicKeyCredential.getClientExtensionResults() come from? #657

Closed

JeffH added 5 commits October 19, 2017 10:44

revert to prior AuthenticationExtensions language per jyasskin

7e035e9

add inline spec issue pointing to issue #657

dbee092

minor cleanups, remove issue wrt not explicitly returning |credential…

942e7b4

…CreationData|

fix annoying bikeshed warning wrt 'rpEntity'

32e2eb6

correctly fix warning as well as other incorrect markup

4b2193e

jcjones approved these changes Oct 19, 2017

View reviewed changes

equalsJeffH merged commit f1f5495 into master Oct 19, 2017

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

Built by Travis-CI: f1f5495 fixup algs contd 3 (#498)

b226816

This was referenced Oct 25, 2017

revamped [[Create]]() method lacks origin argument #661

Closed

fix #254: credman alignment: update #getAssertion section a la PR #498 #663

Closed

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

Built by Travis-CI: d468a75 fix #254: credman alignment: update #getA…

261c1f1

…ssertion section a la PR #498 (#665)

emlun deleted the jeffh-fixup-algs-contd-3 branch June 12, 2019 11:27

		@@ -855,28 +883,29 @@ When this method is invoked, the user agent MUST execute the following algorithm

		<dl class="switch">


		1. Return \|constructCredentialAlg\| and terminate this algorithm.

		Issue: Do we need to replicitly return both \|constructCredentialAlg\| and \|credentialCreationData\| here?

Conversation

equalsJeffH commented Jun 16, 2017 • edited by pr-preview Bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

equalsJeffH left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

equalsJeffH Jun 20, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

equalsJeffH Jun 20, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

equalsJeffH commented Jun 20, 2017

Uh oh!

AngeloKai commented Jun 22, 2017

Uh oh!

equalsJeffH commented Jun 24, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rlin1 left a comment

Choose a reason for hiding this comment

Uh oh!

rlin1 commented Aug 2, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

equalsJeffH commented Oct 16, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

equalsJeffH commented Oct 19, 2017

Uh oh!

jcjones left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

equalsJeffH commented Jun 16, 2017 •

edited by pr-preview Bot

Loading

equalsJeffH Jun 20, 2017 •

edited

Loading

equalsJeffH Jun 20, 2017 •

edited

Loading

rlin1 commented Aug 2, 2017 •

edited

Loading

equalsJeffH commented Oct 16, 2017 •

edited

Loading