fix #700 & #701: add same origin with ancestors param #702

equalsJeffH · 2017-11-28T22:26:50Z

This fixes issues #700 & #701.

include in WD-07 ?

please review :)

Preview | Diff

…mprove #701

…e inconsistent parameter lists

jcjones · 2017-11-28T23:18:59Z

index.bs

@@ -652,6 +653,11 @@ This method accepts two arguments:
    ::  This argument is a {{CredentialCreationOptions}} object whose
        <code>|options|.{{CredentialCreationOptions/publicKey}}</code> member contains a {{MakePublicKeyCredentialOptions}}
        object specifying the desired attributes of the to-be-created [=public key credential=].
+
+    :   <dfn>sameOriginWithAncestors</dfn>
+    ::  This argument is a boolean which is true iff the caller's [=environment settings object=] is


Do we use "iff" anywhere else in the document? Is it normally well understood for W3 specs?

@mikewest uses it in credman. we can spell it out i spose.

jcjones · 2017-11-28T23:19:19Z

index.bs

@@ -973,6 +983,10 @@ The <code>{{PublicKeyCredential}}.<dfn for="PublicKeyCredential" method>\[[Disco
    ::  This argument is a {{CredentialRequestOptions}} object whose
        <code>|options|.{{CredentialRequestOptions/publicKey}}</code> member contains a {{PublicKeyCredentialRequestOptions}}
        object specifying the desired attributes of the [=public key credential=] to discover.
+
+    :   <dfn>sameOriginWithAncestors</dfn>
+    ::  This argument is a boolean which is true iff the caller's [=environment settings object=] is


jcjones · 2017-11-28T23:19:34Z

index.bs

@@ -982,14 +996,18 @@ When this method is invoked, the user agent MUST execute the following algorithm

 1. Assert: <code>|options|.{{CredentialRequestOptions/publicKey}}</code> is [=present=].

+1. If |sameOriginWithAncestors is `false`, return a "{{NotAllowedError}}" {{DOMException}}.


missing a closing |

emlun

Looks good, save for some small issues pointed out in review details.

emlun · 2017-11-29T14:46:55Z

index.bs

@@ -661,14 +667,19 @@ When this method is invoked, the user agent MUST execute the following algorithm

 1. Assert: <code>|options|.{{CredentialCreationOptions/publicKey}}</code> is [=present=].

+1. If <var ignore>OriginWithAncestors</var> is `false`, return a "{{NotAllowedError}}" {{DOMException}}.


You lost the same here in the last commit. :)

LOL!! "haste makes waste..." thx for catching that o_O

emlun · 2017-11-29T14:50:09Z

index.bs


-The <dfn for="PublicKeyCredential" method>\[[Store]](credential)</dfn> method is not supported
+The <dfn for="PublicKeyCredential" method>\[[Store]](credential, sameOriginWithAncestors)</dfn> method is not supported
 for Web Authentication's {{PublicKeyCredential}} type, so it always returns an error.

 Note: This algorithm is synchronous; the {{Promise}} resolution/rejection is handled by
 {{CredentialsContainer/store()|navigator.credentials.store()}}.

 This method accepts a single argument:


This is technically no longer true, although it seems a little weird to specify the parameters at all when they are in effect ignored.

equalsJeffH · 2017-11-29T15:42:31Z

thx for review @emlun & @jcjones, please take another look

emlun · 2017-11-29T18:14:13Z

index.bs

 for Web Authentication's {{PublicKeyCredential}} type, so it always returns an error.

 Note: This algorithm is synchronous; the {{Promise}} resolution/rejection is handled by
 {{CredentialsContainer/store()|navigator.credentials.store()}}.

-This method accepts a single argument:
+This [=internal method=] accepts two arguments:


This says two arguments, but the following list has only one argument.

equalsJeffH · 2017-11-29T18:24:53Z

per 29-Nov-2017 webauthn call: add note wrt the effect of having sameoriginwithancestors, and note that feature-policy once stable will be the right way to go in the future

AngeloKai · 2017-11-29T18:28:12Z

At 11-29-2017 WG meeting, we agreed we would allow same origin iframe for webauthn and add a note to the algorithm that when feature policy becomes "stable" (whether by spec status or by popularity in terms of implementation), we would move to allow iframe with feature policy do webauthn with cross-origin iframe. Browser vendors with feature policy implementation may choose to allow cross-origin iframe.

equalsJeffH · 2017-11-29T22:43:17Z

Ok, I've updated this PR per the call today as noted in:

..please re-review, thanks!

jcjones · 2017-11-29T23:47:05Z

index.bs

+    However, in the future, this specification (in conjunction with 
+    [[CREDENTIAL-MANAGEMENT-1]]) may provide [=[RPS]=] with more fine-grained control--e.g., ranging
+    from allowing only top-level access to Web Authentication functionality,
+    to allowing cross-origin embedded cases--by leveraging


AngeloKai · 2017-11-30T00:50:49Z

I am looking at it.

equalsJeffH · 2017-11-30T16:14:56Z

Ok, we have @jcjones and @emlun approvals -- any others reviews? when shall we merge this?

akshayku

Looks OK to me.

nadalin · 2017-11-30T16:51:37Z

@christiaanbrand please have someone review so we can merge

AngeloKai

Lgtm

equalsJeffH · 2017-11-30T19:24:55Z

Ok, this is also approved by reviews from @akshayku and @AngeloKai -- merged.

…estors param (#702)

JeffH added 2 commits November 28, 2017 14:07

fix #700 add sameOriginWithAncestors parameter to internal methods, i…

c955f00

…mprove #701

fix #701 [[Create]] and [[DiscoverFromExternalSource]] references hav…

44c0e22

…e inconsistent parameter lists

equalsJeffH added stat:breaking subtype:algorithms/WebIDL type:technical labels Nov 28, 2017

equalsJeffH added this to the WD-07 milestone Nov 28, 2017

equalsJeffH self-assigned this Nov 28, 2017

equalsJeffH requested review from jcjones, emlun, AngeloKai and akshayku November 28, 2017 22:26

jcjones reviewed Nov 28, 2017

View reviewed changes

JeffH added 4 commits November 28, 2017 15:41

address jcjones' comments, thx!

c3df94b

fix missing '(' problem

f89bb97

make xrefs to credman Origin Confusion nicer

4f5a4de

fix 'var used only once' warning

ca072a6

emlun requested changes Nov 29, 2017

View reviewed changes

JeffH added 2 commits November 29, 2017 07:31

fix cut'n'paste error, thx emlun!

62f79e7

editorial: normalize argument exposition across internal methods

e784432

equalsJeffH added the type:editorial label Nov 29, 2017

jcjones approved these changes Nov 29, 2017

View reviewed changes

nadalin requested a review from christiaanbrand November 29, 2017 18:12

emlun requested changes Nov 29, 2017

View reviewed changes

jcjones mentioned this pull request Nov 29, 2017

restrict WebAuthentication API to only top level browsing context #374

Closed

fix another sloppiness instance

c1c0f79

emlun approved these changes Nov 29, 2017

View reviewed changes

add ref to Note wrt leveraging Feature Policy spec in future

7d8fdeb

JeffH added 2 commits November 29, 2017 14:27

minor edit

6761071

further wordsmithing

7bfe682

jcjones approved these changes Nov 29, 2017

View reviewed changes

emlun approved these changes Nov 30, 2017

View reviewed changes

akshayku approved these changes Nov 30, 2017

View reviewed changes

AngeloKai approved these changes Nov 30, 2017

View reviewed changes

equalsJeffH merged commit 2f0b13e into master Nov 30, 2017

equalsJeffH deleted the jeffh-fix-700-add-sameOriginWithAncestors-param branch November 30, 2017 19:25

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

Built by Travis-CI: 2f0b13e fix #700 & #701: add same origin with anc…

a4c0d67

…estors param (#702)

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix #700 & #701: add same origin with ancestors param #702

fix #700 & #701: add same origin with ancestors param #702

equalsJeffH commented Nov 28, 2017 •

edited by pr-preview bot

Loading

jcjones Nov 28, 2017

equalsJeffH Nov 28, 2017

jcjones Nov 28, 2017

equalsJeffH Nov 28, 2017

jcjones Nov 28, 2017

emlun left a comment

emlun Nov 29, 2017

equalsJeffH Nov 29, 2017

emlun Nov 29, 2017

equalsJeffH commented Nov 29, 2017

emlun Nov 29, 2017

equalsJeffH Nov 29, 2017 •

edited

Loading

equalsJeffH commented Nov 29, 2017 •

edited

Loading

AngeloKai commented Nov 29, 2017

equalsJeffH commented Nov 29, 2017

jcjones Nov 29, 2017

equalsJeffH Nov 30, 2017

AngeloKai commented Nov 30, 2017

equalsJeffH commented Nov 30, 2017

akshayku left a comment

nadalin commented Nov 30, 2017

AngeloKai left a comment

equalsJeffH commented Nov 30, 2017

		@@ -982,14 +996,18 @@ When this method is invoked, the user agent MUST execute the following algorithm

		1. Assert: <code>\|options\|.{{CredentialRequestOptions/publicKey}}</code> is [=present=].

		1. If \|sameOriginWithAncestors is `false`, return a "{{NotAllowedError}}" {{DOMException}}.

		@@ -661,14 +667,19 @@ When this method is invoked, the user agent MUST execute the following algorithm

		1. Assert: <code>\|options\|.{{CredentialCreationOptions/publicKey}}</code> is [=present=].

		1. If <var ignore>OriginWithAncestors</var> is `false`, return a "{{NotAllowedError}}" {{DOMException}}.

fix #700 & #701: add same origin with ancestors param #702

fix #700 & #701: add same origin with ancestors param #702

Conversation

equalsJeffH commented Nov 28, 2017 • edited by pr-preview bot 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

emlun 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

equalsJeffH commented Nov 29, 2017

Choose a reason for hiding this comment

equalsJeffH Nov 29, 2017 • edited Loading

Choose a reason for hiding this comment

equalsJeffH commented Nov 29, 2017 • edited Loading

AngeloKai commented Nov 29, 2017

equalsJeffH commented Nov 29, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AngeloKai commented Nov 30, 2017

equalsJeffH commented Nov 30, 2017

akshayku left a comment

Choose a reason for hiding this comment

nadalin commented Nov 30, 2017

AngeloKai left a comment

Choose a reason for hiding this comment

equalsJeffH commented Nov 30, 2017

equalsJeffH commented Nov 28, 2017 •

edited by pr-preview bot

Loading

equalsJeffH Nov 29, 2017 •

edited

Loading

equalsJeffH commented Nov 29, 2017 •

edited

Loading