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

[equalsJeffH]

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 accessing settings object from in-parallel steps? #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

[mikewest]

Left some comments. Thanks for tackling this, Jeff!

[equalsJeffH]

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]

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]

@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]

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

[equalsJeffH]

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

[equalsJeffH]

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

[equalsJeffH]

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.

[mikewest]

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]

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

[dlongley]

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

[equalsJeffH]

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

[equalsJeffH]

@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 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]

Ping?

[equalsJeffH]

working on pivoting back to this.

[equalsJeffH]

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

[mikewest]

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

[mikewest]

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]

Ping!

[equalsJeffH]

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]

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!

[mikewest]

LGTM.

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