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

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

Merged
merged 13 commits into from
Nov 9, 2017

Conversation

equalsJeffH
Copy link
Contributor

@equalsJeffH equalsJeffH commented Oct 30, 2017

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

Copy link
Contributor

@jcjones jcjones left a comment

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.

{{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
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
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
Contributor Author

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

@equalsJeffH
Copy link
Contributor Author

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

@nadalin
Copy link
Contributor

nadalin commented Nov 9, 2017

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

@AngeloKai
Copy link
Contributor

I think it’s ok to merge this.

Copy link
Contributor

@akshayku akshayku left a comment

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