Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

WebAuthn WD-07 api tests #9237

Merged

Conversation

Projects
None yet
4 participants
@apowers313
Copy link
Contributor

apowers313 commented Jan 29, 2018

new tests based on Working Draft 7

@w3c-bots

This comment has been minimized.

Copy link

w3c-bots commented Jan 29, 2018

Build PASSED

Started: 2018-02-07 23:52:41
Finished: 2018-02-08 00:33:25

Failing Jobs

  • MicrosoftEdge:16.16299

View more information about this build on:

@@ -17,7 +17,7 @@
// Example 1
// http://example.com/ opened in a top-level browsing context is not a secure context, as it was not delivered over an authenticated and encrypted channel.
test (() => {
assert_false (typeof navigator.credentials.create === "function");
assert_false (typeof navigator.credentials === "object" && typeof navigator.credentials.create === "function");

This comment has been minimized.

Copy link
@jcjones

jcjones Jan 29, 2018

Contributor

Note: This throws on line helpers.js:426 in ensureInterface because navigator.credentials doesn't exist, so navigator.credentials.create throws with an uncaught TypeError.

Perhaps that could be a separate commit?

This comment has been minimized.

Copy link
@apowers313

apowers313 Jan 29, 2018

Author Contributor

I had fixed i in one file but not the other... should be right in a0a0e8c

This comment has been minimized.

This comment has been minimized.

Copy link
@jcjones

jcjones Jan 30, 2018

Contributor

Fixed, thanks! I'm reviewing the rest still, but looks good so far! I'll be done by noon.

@jcjones
Copy link
Contributor

jcjones left a comment

This looks pretty much good to go. I've got those two comments, and otherwise I think we're good to merge. Thanks, Adam!

new CreateCredentialsTest("options.publicKey.user.id", new ArrayBuffer(0)).runTest("Bad user: id is empty ArrayBuffer", new TypeError());
new CreateCredentialsTest("options.publicKey.user.id", new ArrayBuffer(65)).runTest("Bad user: id is too long (65 bytes)", new TypeError());
new CreateCredentialsTest("options.publicKey.user.id", new Int16Array(33)).runTest("Bad user: id is too long (66 bytes)", new TypeError());
new CreateCredentialsTest("options.publicKey.user.id", new Int32Array(17)).runTest("Bad user: id is too long (68 bytes)", new TypeError());

This comment has been minimized.

Copy link
@jcjones

jcjones Feb 7, 2018

Contributor

These duplicate "Bad user: id is too long (68 bytes)", "Bad user: id is too long (65 bytes)" tests make for test harness errors on Firefox; they'll need de-duped names.

This comment has been minimized.

Copy link
@apowers313

apowers313 Feb 7, 2018

Author Contributor

Good catch, thanks.

This comment has been minimized.

Copy link
@apowers313

apowers313 Feb 7, 2018

Author Contributor

fixed in e5da0e2

// bad timeout values
new CreateCredentialsTest({path: "options.publicKey.timeout", value: -1}).runTest("Bad timeout: negative", new TypeError());
new CreateCredentialsTest({path: "options.publicKey.timeout", value: 4294967295 + 1}).runTest("Bad timeout: too big", new TypeError());

This comment has been minimized.

Copy link
@jcjones

jcjones Feb 7, 2018

Contributor

The spec doesn't impose an upper limit on the timeout size; I don't know if this is a valid test to assume it should be a TypeError. (Step 4 of Section 5.1.3)

This comment has been minimized.

Copy link
@apowers313

apowers313 Feb 7, 2018

Author Contributor

The timeout property is defined as unsigned long. The value is MAX_UNSIGNED_LONG + 1. I'll comment it out with a TODO while I research the proper behavior. :)

This comment has been minimized.

Copy link
@jcjones

jcjones Feb 7, 2018

Contributor

Sounds good. That one might be tricky!

This comment has been minimized.

Copy link
@apowers313

apowers313 Feb 7, 2018

Author Contributor

fixed in 60f7ad1

let challengeBytes = new Uint8Array(16);
window.crypto.getRandomValues(challengeBytes);
createArgs.options.publicKey.challenge = challengeBytes;
createArgs.options.publicKey.user.id = new Uint8Array();

This comment has been minimized.

Copy link
@jcjones

jcjones Feb 7, 2018

Contributor

This has to be initialized, or the test Bad user: id is empty ArrayBuffer is a lie 😁
So let's make this new Uint8Array(16);

This comment has been minimized.

Copy link
@jcjones

jcjones Feb 7, 2018

Contributor

Actually, we need to do the same thing in CreateCredentialsTest after the line:
// cloneObject can't clone the BufferSource in user.id, so let's recreate it.

This comment has been minimized.

Copy link
@apowers313

apowers313 Feb 7, 2018

Author Contributor

fixed in 87ed972

"use strict";
// bad excludeCredentials values
new CreateCredentialsTest({path: "options.publicKey. excludeCredentials", value: undefined}).runTest("Bad user: excludeCredentials missing", new TypeError());

This comment has been minimized.

Copy link
@jcjones

jcjones Feb 7, 2018

Contributor

Unexpected spaces in these paths

This comment has been minimized.

Copy link
@jcjones

jcjones Feb 7, 2018

Contributor

Also: I think setting excludeCredentials to undefined is valid, actually. That's as if we didn't pass the array, which is legal. So I think line 15 goes away, and 16 + 17 have that space trimmed.

This comment has been minimized.

Copy link
@apowers313

apowers313 Feb 7, 2018

Author Contributor

fixed in 9a708e1

"use strict";
// bad timeout values
new CreateCredentialsTest({path: "options.publicKey.timeout", value: -1}).runTest("Bad timeout: negative", new TypeError());

This comment has been minimized.

Copy link
@jcjones

jcjones Feb 7, 2018

Contributor

I don't think we can test this, either, actually. In Gecko at least, since the timeout is a unsigned integer, if JS gives a signed integer it gets converted to unsigned (lossily) and arrives into at least my WebAuthn code as valid.

I think if we wanted to check this, we'd have to define the IDL to be signed.

var credPromise = createCredential();
// bad timeout values
new GetCredentialsTest({path: "options.publicKey.timeout", value: -1})

This comment has been minimized.

Copy link
@jcjones

jcjones Feb 7, 2018

Contributor

As before; this might not be testable.

@jcjones

jcjones approved these changes Feb 7, 2018

@wpt-pr-bot wpt-pr-bot added the infra label Feb 7, 2018

@wpt-pr-bot wpt-pr-bot requested a review from jgraham Feb 7, 2018

@jcjones

This comment has been minimized.

Copy link
Contributor

jcjones commented Feb 7, 2018

For me there are still a bunch of type coercion issues, but this is a great place to merge and make sure everyone's on the same page. Thanks so much, Adam!

@apowers313

This comment has been minimized.

Copy link
Contributor Author

apowers313 commented Feb 7, 2018

Yea, totally agree -- this is a good first guess and we can iterate from here. I'm also wondering if people handle errors the same way. I'm thinking we will find out pretty quickly.

@jcjones

This comment has been minimized.

Copy link
Contributor

jcjones commented Feb 7, 2018

Oops, one more change needed to helpers.js:
Line 65 in createCredential:

    createArgs.options.publicKey.user.id = new Uint8Array();

needs to also be non-zero. Something like

    createArgs.options.publicKey.user.id = new Uint8Array(2);
@apowers313

This comment has been minimized.

Copy link
Contributor Author

apowers313 commented Feb 7, 2018

I had made the change but didn't save the file... it's in cdbe7ce now.

@apowers313 apowers313 merged commit 025a557 into web-platform-tests:master Feb 8, 2018

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

moz-v2v-gh pushed a commit to mozilla/gecko-dev that referenced this pull request Feb 8, 2018

Bug 1436473 - Rename WebAuthn dict to PublicKeyCredentialCreationOpti…
…ons r=baku

Late-breaking rename pre-CR in Web Authentication [1] renamed a dictionary. It's
not an interop issue, really, which must be why it was let through. This is a
WebIDL and Web Platform Tests-only issue. (The WPT updates are happening at
Github [2])

[1] https://github.com/w3c/webauthn/pull/779/files
[2] web-platform-tests/wpt#9237

MozReview-Commit-ID: KEIlqIYbzKp

--HG--
extra : rebase_source : 4204ea62a41f374a6731a9367552af122d354145

mykmelez pushed a commit to mozilla/gecko that referenced this pull request Feb 8, 2018

Bug 1436473 - Rename WebAuthn dict to PublicKeyCredentialCreationOpti…
…ons r=baku

Late-breaking rename pre-CR in Web Authentication [1] renamed a dictionary. It's
not an interop issue, really, which must be why it was let through. This is a
WebIDL and Web Platform Tests-only issue. (The WPT updates are happening at
Github [2])

[1] https://github.com/w3c/webauthn/pull/779/files
[2] web-platform-tests/wpt#9237

MozReview-Commit-ID: KEIlqIYbzKp
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.