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

issue 92 accessing settings object: add passing global and queue task invoke algorithm #100

Merged

Conversation

@equalsJeffH
Copy link
Collaborator

equalsJeffH commented Aug 25, 2017

STATUS AS OF 20-Oct-2018:

Much of the work necessary to thoroughly address issue #92 is accomplished in this PR in its present state. In summary:

improves #92
fixes #103
fixes #105

I've gone through all the comments below and marked the ones that are addressed as resolved, and this comprises almost all comments except for these unresolved, non-trivial comments:

#100 (comment)
#100 (review)
#100 (review)
#100 (comment)

Additionally, I've discovered that the matchable-a-priori alg is invoked from Request a Credential's in-parallel section, and matchable-a-priori in turn invokes the relevant credential interface objects alg, which in turn touches the current settings object. I am thinking we can fix this in the Request a Credential alg.

Recommendations:

  1. further review this PR (help wanted :) and merge it to master as it is now because it presently addresses the API impedance mismatch between webauthn and credman, and because this PR is already complex enough.

  2. In the near term, address the five items above in subsequent PR(s) in order to complete addressing issue #92.


Original Post:

this drafty draft implements @domenic's suggestions (#93 (comment)) in credman (only for credential creation, not yet for "request a credential" aka (in webauthn) as "get assertion").

For the webauthn side of this, see the current state of w3c/webauthn#498 (rendered diff here: https://s3.amazonaws.com/pr-preview/w3c/webauthn/cca20d3...6a011d2.html)

It would be great to please get feedback before proceeding and applying this approach to the "request a credential" alg.

Please also note that this PR is for the "issue-92-accessing-settings-object" branch (rather than master) and it fixes a few things I'd noted in my review of PR #93.


Preview | Diff

@battre battre requested a review from mikewest Aug 25, 2017
Copy link
Member

mikewest left a comment

Left some comments. Thanks for tackling this, Jeff!

index.src.html Outdated Show resolved Hide resolved
index.src.html Outdated Show resolved Hide resolved
index.src.html Outdated Show resolved Hide resolved
index.src.html Outdated Show resolved Hide resolved
index.src.html Outdated Show resolved Hide resolved
index.src.html Outdated Show resolved Hide resolved
index.src.html Outdated Show resolved Hide resolved
index.src.html Outdated Show resolved Hide resolved
@equalsJeffH

This comment has been minimized.

Copy link
Collaborator Author

equalsJeffH commented Aug 25, 2017

thanks for review @mikewest, i'll work on addressing your trenchant feedback over the next few days. See also w3c/webauthn#498 if you have time :)

@domenic

This comment has been minimized.

Copy link

domenic commented Aug 25, 2017

I plan to help review this but probably next week. It sounds like it'll be worth waiting for you to incorporate @mikewest's suggestions too.

DOM manipulation task source is good though, I can say that as a drive-by comment at least.

@equalsJeffH

This comment has been minimized.

Copy link
Collaborator Author

equalsJeffH commented Sep 8, 2017

@mikewest @battre @domenic: I took a stab at addressing @mikewest's comments, except for #100 (comment) and #100 (comment) regarding (IIUC) plumbing the origin thru those password- and federated-credential-specific algorithms (that Mike links to in his latter comments).

WRT @mikewest's latter comments, I am suspecting that the "proper" thing to do would be to alter the CredentialRequestOptions Dictionary and the CredentialCreationOptions Dictionary such that they each contain a (optional) member conveying an origin value. In this way, we would return to having all the Collect, Request, Store, and Create algorithms take only an options parameter, and the plumbing of the origin thru to those password- and federated-credential-specific algorithms would be essentially transparent. WDYT?

However, I am uncertain how to declare this. I am wondering if the correct thing to do is to add a DOMString value to each of CredentialRequestOptions Dictionary and CredentialCreationOptions Dictionary whose value is a serialized origin ... WDYT?

thanks :)

@mikewest

This comment has been minimized.

Copy link
Member

mikewest commented Sep 28, 2017

(Sorry, I've been very slow. This is on my list for today.)

@equalsJeffH

This comment has been minimized.

Copy link
Collaborator Author

equalsJeffH commented Sep 29, 2017

Please see also the related webauthn PR w3c/webauthn#498

index.src.html Outdated Show resolved Hide resolved
index.src.html Outdated Show resolved Hide resolved
index.src.html Outdated Show resolved Hide resolved
index.src.html Outdated Show resolved Hide resolved
index.src.html Outdated Show resolved Hide resolved
index.src.html Outdated Show resolved Hide resolved
index.src.html Outdated Show resolved Hide resolved
index.src.html Outdated Show resolved Hide resolved
index.src.html Outdated Show resolved Hide resolved
index.src.html Outdated Show resolved Hide resolved
index.src.html Outdated Show resolved Hide resolved
index.src.html Outdated Show resolved Hide resolved
index.src.html Outdated Show resolved Hide resolved
index.src.html Outdated Show resolved Hide resolved
index.src.html Outdated Show resolved Hide resolved
index.src.html Outdated Show resolved Hide resolved
index.src.html Outdated Show resolved Hide resolved
index.src.html Outdated Show resolved Hide resolved
index.src.html Outdated Show resolved Hide resolved
@equalsJeffH

This comment has been minimized.

Copy link
Collaborator Author

equalsJeffH commented Oct 10, 2017

Ok, I have incorporated suggestions from @mikewest and @jyasskin (thx!) up through new line 895. These changes are ensconced in commit b3bf4ee. y'all have an oppty to review them while i look into what's necessary to alter in PasswordCredential and FederatedCredential sections.

Also, here's a kdiff3 side-by-side kdiff3 of the diff between b3bf4ee and the changes queued in branch issue-92-accessing-settings-object, which this PR builds upon: http://kingsmountain.com/doc/diff/diff-credman-equalsJeffH-index.src--from--index-issue-92-accessing-settings-object-b34d4c8.src.html.pdf

fixes #103

JeffH added 2 commits Oct 10, 2017
…asswordcredential-form
@equalsJeffH

This comment has been minimized.

Copy link
Collaborator Author

equalsJeffH commented Oct 10, 2017

Ok, I think I plumbed/threaded origin thru #construct-passwordcredential-data and #construct-passwordcredential-form per @mikewest's review comment -- please review (commit 6bdadaa).

Now working on addressing same for #abstract-opdef-create-a-federatedcredential-from-federatedcredentialinit.

@equalsJeffH equalsJeffH self-assigned this Oct 11, 2017
Copy link
Member

mikewest left a comment

I think the direction here looks like what I think @jyasskin was asking for. You get an algorithm, and promise-call it with a global object.

I'd suggest simplifying the [[Create]] algorithm so that it always returns an algorithm. We'd need to remove the synchronous constructors for PasswordCredential and FederatedCredential in order to make that work, but that's probably a reasonable outcome.

+@battre who might have opinions on that.

@mikewest

This comment has been minimized.

Copy link
Member

mikewest commented Feb 19, 2018

Is this still outstanding, @equalsJeffH? I think the ball's still in your court. Please let me know if that's incorredt.

@dlongley

This comment has been minimized.

Copy link

dlongley commented Feb 20, 2018

In refactoring the API framework here I wanted to draw more attention to the extensibility approach being taken by the Web Payments WG with Payment Handlers. A similar design that could work with "Web Credentials" and "Credential Handlers" is described here -- with some working code and a demo linked.

See: #99

Adding this extensibility approach to the Credential Management API could perhaps more seamlessly handle login approaches w/Facebook/Google/so on and keep abstractions clean. It was demo'd at TPAC.

cc: @battre

JeffH JeffH
@equalsJeffH

This comment has been minimized.

Copy link
Collaborator Author

equalsJeffH commented Mar 7, 2018

@mikewest -- yes, ball is in my court, am working on it.

@equalsJeffH

This comment has been minimized.

Copy link
Collaborator Author

equalsJeffH commented Mar 8, 2018

@mikewest wrote in #100 (review)

I think the direction here looks like what I think @jyasskin was asking for.

ok, good, thx.

I'd suggest simplifying the [[Create]] algorithm so that it always returns an algorithm.

That's essentially what @jyasskin suggests in #100 (comment), yes?

We'd need to remove the synchronous constructors for PasswordCredential and FederatedCredential in order to make that work, but that's probably a reasonable outcome.

This part (wrt removing constructors) I'll have to learn more to understand the implications of (haven't looked into it in detail as yet). Hints/help welcome :)

@mikewest

This comment has been minimized.

Copy link
Member

mikewest commented May 8, 2018

This hasn't moved in a while. I think we need to get it finished up so I can split this document and give y'all the dependencies you need for WebAuthn. What do we need to do to make that happen? Do you still have bandwidth to address this, @equalsJeffH, or do we need to find someone on our end? :)

@mikewest

This comment has been minimized.

Copy link
Member

mikewest commented May 16, 2018

Ping?

@equalsJeffH

This comment has been minimized.

Copy link
Collaborator Author

equalsJeffH commented Jul 25, 2018

working on pivoting back to this.

@equalsJeffH

This comment has been minimized.

Copy link
Collaborator Author

equalsJeffH commented Oct 20, 2018

This PR's current status is now summarized up in the original post: #100 (comment)

@@ -0,0 +1,3 @@
# Code of Conduct

All documentation, code and communication under this repository are covered by the [W3C Code of Ethics and Professional Conduct](https://www.w3.org/Consortium/cepc/).

This comment has been minimized.

Copy link
@mikewest

mikewest Oct 29, 2018

Member

Can you rebase against master? I don't think you intended for this to be part of your PR.

@mikewest

This comment has been minimized.

Copy link
Member

mikewest commented Oct 29, 2018

Hey @equalsJeffH! I think the latest PR is reasonable? But it's hard to tell, because the diff shows more than a few things from master as being part of your patch. Please rebase?

@mikewest

This comment has been minimized.

Copy link
Member

mikewest commented Oct 31, 2018

Ping!

@equalsJeffH

This comment has been minimized.

Copy link
Collaborator Author

equalsJeffH commented Nov 2, 2018

Hi @mikewest

I think the latest PR is reasonable? But it's hard to tell, because the diff shows more than a few things from master as being part of your patch. Please rebase?

I think the issue of "commits from master showing up as part of this patch" is because somewhere along the line I botched a merge-from-master-into-this-branch -- my apologies :(

I am unsure how to use rebase to rectify this... I am also unsure why github here is showing 6 changed files when on my local clone of my repo the following diff commands are showing only 2 files being different:

> git diff -w --name-only master 4dc4854
index.html
index.src.html

> git diff -w --name-only upstream/master 4dc4854
index.html
index.src.html

I may play around with git rebase -i later this afternoon to see what all it might offer/rectify. Suggestions on cleaning this up are welcome. Sorry for this taking so long.

@equalsJeffH equalsJeffH changed the base branch from issue-92-accessing-settings-object to master Nov 2, 2018
@equalsJeffH

This comment has been minimized.

Copy link
Collaborator Author

equalsJeffH commented Nov 2, 2018

Aha!! I figured it out -- the issue was that I'd created my branch from @battre's issue-92-accessing-settings-object branch which made the latter the "base branch", rather than master. So I just now changed the base branch to master and things appear to be cleaned up. I'll check further and look into the unresolved comments, thanks!

@equalsJeffH equalsJeffH changed the title issue 92 accessing settings object: add passing global and queue task invoke callback issue 92 accessing settings object: add passing global and queue task invoke algorithm Nov 3, 2018
Copy link
Member

mikewest left a comment

LGTM.

I'm just going to merge this and clean it up in subsequent patches. Thanks for working through this so dilligently, @equalsJeffH!

@mikewest mikewest merged commit 99332be into w3c:master Dec 3, 2018
1 check passed
1 check passed
ipr PR deemed acceptable.
Details
mikewest added a commit that referenced this pull request Dec 3, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
5 participants
You can’t perform that action at this time.