Add a WebDriver Virtual Authenticator extension #1256

nsatragno · 2019-07-10T17:39:29Z

This change introduces a WebDriver Extension that lets clients create and manipulate virtual authenticators. This API can be used by web developers to write tests against the WebAuthn API and it's also possible to leverage it on WPTs through testdriver.js. Being able to instantiate virtual authenticators on WPTs would be a major step towards enabling testing a larger surface area of the API across multiple browsers.

Fixes #1236

Preview | Diff

Commit the editorial suggestions Co-Authored-By: =JeffH <jdhodges@google.com>

Apply trivial changes Co-Authored-By: =JeffH <jdhodges@google.com>

equalsJeffH

LGTM, thx @nsatragno !

Add the `RemoveVirtualAuthenticator` command to ChromeDriver. A draft of the proposed addition to the specification is available at w3c/webauthn#1256 This is one in a series of patches intended to create a Testing API for WebAuthn, for use in Web Platform Tests and by external webauthn tests. For an overview of overall design, please see https://docs.google.com/document/d/1bp2cMgjm2HSpvL9-WsJoIQMsBi1oKGQY6CvWD-9WmIQ/edit?usp=sharing Bug: 922572 Change-Id: I5b6094cc5eb9a52afc8ee89d548c3452a728b1fe Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1713301 Auto-Submit: Nina Satragno <nsatragno@chromium.org> Commit-Queue: John Chen <johnchen@chromium.org> Reviewed-by: John Chen <johnchen@chromium.org> Cr-Commit-Position: refs/heads/master@{#679727}

This reverts commit cf614d4.

Add the `AddCredential` command to ChromeDriver. A draft of the proposed addition to the specification is available at w3c/webauthn#1256 This is one in a series of patches intended to create a Testing API for WebAuthn, for use in Web Platform Tests and by external webauthn tests. For an overview of overall design, please see https://docs.google.com/document/d/1bp2cMgjm2HSpvL9-WsJoIQMsBi1oKGQY6CvWD-9WmIQ/edit?usp=sharing Bug: 922572 Change-Id: Id9b9fb75ab6682126d5d1275b12785640b7f0053 Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1729963 Commit-Queue: Nina Satragno <nsatragno@chromium.org> Auto-Submit: Nina Satragno <nsatragno@chromium.org> Reviewed-by: John Chen <johnchen@chromium.org> Cr-Commit-Position: refs/heads/master@{#683364}

jcjones · 2019-08-12T22:21:31Z

Note: I've asked @AutomatedTester to do a review pass here from the Mozilla side.

index.bs

AutomatedTester · 2019-08-12T22:31:47Z

index.bs

+
+The [=remote end steps=] are:
+
+ 1. If |authenticatorId| does not match any [=Virtual Authenticator=] stored in the [=Virtual Authenticator


Same question as above?

Since we are deleting it, if it is not there does it really matter is what I mean. If you feel it does matter then leave it in.

AutomatedTester · 2019-08-12T22:31:50Z

index.bs

+
+The [=remote end steps=] are:
+
+ 1. If |authenticatorId| does not match any [=Virtual Authenticator=] stored in the [=Virtual Authenticator


Do we really need to error here when there are not items that match. Can't we just try do the right thing and if not just go meh?

What do you mean exactly? If the authenticatorId doesn't match, just don't do anything and return success? If that's the case, that would potentially be a source of confusion for developers that's easily avoided by returning an error... the UA has to check for the existance of the authenticator anyway. Maybe I'm misunderstanding you?

Since we are deleting it, if it is not there does it really matter is what I mean. If you feel it does matter then leave it in.

I think it does matter. The most likely scenario for trying to delete something that is not there is a developer oversight. Catching that early means less headaches. And if say a library wanted that functionality, they could catch the error and pretend nothing happened.

This and the above conversation can be set as resolved now?

index.bs

nsatragno

Thank you for taking a look. I addressed the comments below:

index.bs

nsatragno · 2019-08-13T17:18:21Z

index.bs

+
+The [=remote end steps=] are:
+
+ 1. If |authenticatorId| does not match any [=Virtual Authenticator=] stored in the [=Virtual Authenticator


What do you mean exactly? If the authenticatorId doesn't match, just don't do anything and return success? If that's the case, that would potentially be a source of confusion for developers that's easily avoided by returning an error... the UA has to check for the existance of the authenticator anyway. Maybe I'm misunderstanding you?

index.bs

jcjones · 2019-09-03T19:32:09Z

Just a question @nsatragno:

In the algorithm for adding a Virtual Authenticator, we emit a WebDriver error when the parameter doesn't have a valid value. If, for example, a browser initially only supported protocol being ctap1/u2f, would emitting a WebDriver error for all other protocols (e.g. ctap2) permit the relevant tests to continue on, or is such an error fatal?

jcjones · 2019-09-04T00:22:31Z

Just a question @nsatragno:

In the algorithm for adding a Virtual Authenticator, we emit a WebDriver error when the parameter doesn't have a valid value. If, for example, a browser initially only supported protocol being ctap1/u2f, would emitting a WebDriver error for all other protocols (e.g. ctap2) permit the relevant tests to continue on, or is such an error fatal?

To clarify, I think the WebDriver implementation for WebAuthn could be used then for FIDO U2F's JS API, which would let us write Web Platform Tests for U2F. Which I want to get, if at all possible.

Other than this question, I am ready to grant this review from the Mozilla side.

nsatragno

If, for example, a browser initially only supported protocol being ctap1/u2f, would emitting a WebDriver error for all other protocols (e.g. ctap2) permit the relevant tests to continue on, or is such an error fatal?

To clarify, I think the WebDriver implementation for WebAuthn could be used then for FIDO U2F's JS API, which would let us write Web Platform Tests for U2F. Which I want to get, if at all possible.

If the browser only supports a subset of the protocols, tests that create authenticators using that subset should work. For example, browsers could first support ctap1/u2f to pass the U2F API WPTs and add support for ctap2 later.

Perhaps we can add a clarification to the spec as an additional step under the AddVirtualAuthenticator algo like

1. If the |parameters|' |protocol| property is not supported by the user agent,
   return a [=WebDriver error=] with [=WebDriver error code=] [=invalid argument=].

So yes, this API would let us write tests for the U2F JS API, too :)

What do you think?

nsatragno · 2019-09-05T15:08:23Z

index.bs

+
+The [=remote end steps=] are:
+
+ 1. If |authenticatorId| does not match any [=Virtual Authenticator=] stored in the [=Virtual Authenticator


I think it does matter. The most likely scenario for trying to delete something that is not there is a developer oversight. Catching that early means less headaches. And if say a library wanted that functionality, they could catch the error and pretend nothing happened.

jcjones · 2019-09-05T16:54:07Z

What do you think?

I think I hit 'approve' :)

equalsJeffH · 2019-09-10T00:19:08Z

Hi @nsatragno -- IIUC, your answer to @jcjones in #1256 (review) implies that returning a webdriver error does not necessarily halt a testing run, correct?

Also, IIUC, you have not as yet added the step proposed in #1256 (review) to the AddVirtualAuthenticator algo. Is @jcjones' appoval contingent on that being added?

HTH, equalsJeffH

jcjones · 2019-09-10T22:35:00Z

The clarification doesn't sound normative to me, but I'd be happy to wait until it is available before merging rather than worrying about it more in the future.

jabazorius · 2019-09-10T23:13:03Z

help

akshayku · 2019-09-11T18:44:08Z

Lets discuss it next week in TPAC where more Edge folks will be present and can review it more thoroughly.

akshayku

LGTM

…utomation

nsatragno and others added 25 commits June 17, 2019 17:00

Automation: AddVirtualAuthenticator

4304ea2

Automation: RemoveVirtualAuthenticator.

efc5abb

Automation: AddCredential

958cba9

More Automation commands.

5bce333

Remove attachment from API.

dfb388c

merge from master and fix conflict

b106725

Replace <a></a> with [==]

2975d59

Use correct syntax for authenticator ID URI param.

f908922

Apply suggestions from code review

a5062cd

Commit the editorial suggestions Co-Authored-By: =JeffH <jdhodges@google.com>

Add missing suggestion.

cd4222c

Add isUserPresent

29fc927

Constrain authenticatorId format.

8077ee6

Add missing `

73d6903

Use a table instead of WebIDL for addVirtualAuthenticator.

43213e1

Move authenticatorId definition.

f69855e

Trim down definitions.

7d2d819

Apply suggestions from code review

5120d50

Apply trivial changes Co-Authored-By: =JeffH <jdhodges@google.com>

Fix up addVirtualAuthenticator.

cd26692

Fix up AddCredential.

7496558

Several misc fixes.

2fcc447

Fix typo.

91cf85d

Add removeCredential and rename removeCredentials

c146c89

touch up authenticatorId definition.

107f33f

Address comments.

ceea5a8

Remove UUID recommendation.

6862588

equalsJeffH self-requested a review July 10, 2019 18:16

equalsJeffH assigned nsatragno Jul 10, 2019

equalsJeffH added the type:technical label Jul 10, 2019

equalsJeffH added this to the L2-WD-02 milestone Jul 10, 2019

equalsJeffH approved these changes Jul 10, 2019

View reviewed changes

nsatragno added 5 commits July 30, 2019 10:45

Revert "Encode private key using COSE instead."

e826609

This reverts commit cf614d4.

Add |isResidentCredential| to AddCredential.

263a57b

Add check for resident credential.

7271e6c

Constrain the pk to ECDSA P-256

dc417dd

Return an error if userHandle is null and is resident key.

9be5c17

AutomatedTester reviewed Aug 12, 2019

View reviewed changes

nsatragno commented Aug 13, 2019

View reviewed changes

LukeZielinski mentioned this pull request Aug 14, 2019

Enable testdriver to return results to tests. web-platform-tests/wpt#18431

Merged

nsatragno commented Sep 5, 2019

View reviewed changes

jcjones approved these changes Sep 5, 2019

View reviewed changes

agl approved these changes Sep 6, 2019

View reviewed changes

emlun approved these changes Sep 9, 2019

View reviewed changes

equalsJeffH mentioned this pull request Sep 20, 2019

Length restriction on user handles has no processing model associated with it #1302

Closed

akshayku approved these changes Oct 2, 2019

View reviewed changes

agl merged commit 976391d into w3c:master Oct 2, 2019

WebAuthnBot pushed a commit that referenced this pull request Oct 2, 2019

Built by Travis-CI: 976391d Merge pull request #1256 from nsatragno/a…

5492d90

…utomation

WebAuthnBot pushed a commit that referenced this pull request Oct 2, 2019

Built by Travis-CI: 976391d Merge pull request #1256 from nsatragno/a…

e5a4fca

…utomation

stephenmcgruer mentioned this pull request Nov 3, 2020

RFC: Add TestDriver support for the "Set Timezone" WebDriver extension command web-platform-tests/rfcs#66

Closed

christian-bromann mentioned this pull request Jan 5, 2021

Add new WebDriver extensions webdriverio/webdriverio#6289

Merged

8 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add a WebDriver Virtual Authenticator extension #1256

Add a WebDriver Virtual Authenticator extension #1256

nsatragno commented Jul 10, 2019 •

edited by pr-preview bot

equalsJeffH left a comment

jcjones commented Aug 12, 2019

AutomatedTester Aug 12, 2019

AutomatedTester Sep 3, 2019

AutomatedTester Aug 12, 2019

nsatragno Aug 13, 2019

AutomatedTester Sep 3, 2019

nsatragno Sep 5, 2019

equalsJeffH Sep 10, 2019

nsatragno left a comment

nsatragno Aug 13, 2019

jcjones commented Sep 3, 2019

jcjones commented Sep 4, 2019

nsatragno left a comment

nsatragno Sep 5, 2019

jcjones commented Sep 5, 2019

equalsJeffH commented Sep 10, 2019

jcjones commented Sep 10, 2019

jabazorius commented Sep 10, 2019

akshayku commented Sep 11, 2019

akshayku left a comment


		The [=remote end steps=] are:

		1. If \|authenticatorId\| does not match any [=Virtual Authenticator=] stored in the [=Virtual Authenticator

Add a WebDriver Virtual Authenticator extension #1256

Add a WebDriver Virtual Authenticator extension #1256

Conversation

nsatragno commented Jul 10, 2019 • edited by pr-preview bot

equalsJeffH left a comment

Choose a reason for hiding this comment

jcjones commented Aug 12, 2019

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

nsatragno left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jcjones commented Sep 3, 2019

jcjones commented Sep 4, 2019

nsatragno left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jcjones commented Sep 5, 2019

equalsJeffH commented Sep 10, 2019

jcjones commented Sep 10, 2019

jabazorius commented Sep 10, 2019

akshayku commented Sep 11, 2019

akshayku left a comment

Choose a reason for hiding this comment

nsatragno commented Jul 10, 2019 •

edited by pr-preview bot