Skip to content

fix #254: credman alignment: update #getAssertion section a la PR #498#665

Merged
equalsJeffH merged 13 commits into
masterfrom
jeffh-fixup-algs-contd-4
Nov 9, 2017
Merged

fix #254: credman alignment: update #getAssertion section a la PR #498#665
equalsJeffH merged 13 commits into
masterfrom
jeffh-fixup-algs-contd-4

Conversation

@equalsJeffH

@equalsJeffH equalsJeffH commented Oct 30, 2017

Copy link
Copy Markdown
Contributor

This is intended to do what the subject says.
fix #671
fix #663
fix #661
fix #254 (finally)

Note: requisite mods to credman (w3c/webappsec-credential-management#100) remain in-progress


Preview (#createCreden…) (#discover-fro…) (#op-get-asser…) | Diff

@jcjones jcjones left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks really good to me.

Comment thread index.bs
{{PublicKeyCredentialDescriptor}}.{{PublicKeyCredentialDescriptor/id}} and set its value to <code>|allowCredentialDescriptorList|[0].id</code>'s
value (see [here](#authenticatorGetAssertion-return-values) in [[#op-get-assertion]] for more information).

Issue: The foregoing step _may_ be incorrect, in that we are attempting to create |savedCredentialId|

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We'll see what others think, but this is clear enough to me to be implementable. Practically speaking, tracking the CredentialId for whatever credential works is going to be done by the hardware interaction code -- either keeping track of which ID worked on the wire or getting back the ID that came on the wire. So it's really a matter of plumbing it out of that hardware code, not even necessarily of this algorithm defining an internal slot to keep track of it.

@equalsJeffH

Copy link
Copy Markdown
Contributor Author

@jyasskin is unable to review this in the near term. Any others able to review today? Given @jcjones' review & approval, perhaps we ought to merge it today with no further changes?

@equalsJeffH

Copy link
Copy Markdown
Contributor Author

fixed conflicts with master, fixed issue #671. please review.

@equalsJeffH

Copy link
Copy Markdown
Contributor Author

should we go ahead and merge this, or wait for the f2f meeting at TPAC tomorrow?

@nadalin

nadalin commented Nov 9, 2017

Copy link
Copy Markdown
Contributor

@akshayku @AngeloKai Can you both review ? @equalsJeffH if you get @akshayku and @AngeloKai to approve then merge

@AngeloKai

Copy link
Copy Markdown
Contributor

I think it’s ok to merge this.

@akshayku akshayku left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks ok to me.

@equalsJeffH equalsJeffH merged commit d468a75 into master Nov 9, 2017
@equalsJeffH equalsJeffH deleted the jeffh-fixup-algs-contd-4 branch November 9, 2017 15:49
WebAuthnBot pushed a commit that referenced this pull request Nov 9, 2017
@equalsJeffH equalsJeffH restored the jeffh-fixup-algs-contd-4 branch November 19, 2017 04:09
@emlun emlun deleted the jeffh-fixup-algs-contd-4 branch June 12, 2019 11:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

5 participants