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

fixup algs contd 2 #495

Merged
merged 5 commits into from
Jul 5, 2017
Merged

fixup algs contd 2 #495

merged 5 commits into from
Jul 5, 2017

Conversation

equalsJeffH
Copy link
Contributor

@equalsJeffH equalsJeffH commented Jun 16, 2017

Presently, this (hopefully) fixes #480.

This also fixes #475 ("Level 1" editorial addition).

update 28-Jun-2017: this also (re-) fixes #268


Preview | Diff

Copy link
Contributor

@AngeloKai AngeloKai left a comment

Choose a reason for hiding this comment

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

This would be a step closer to fix the algorithm. We should merge this to get us on track soon.

Copy link
Contributor

@AngeloKai AngeloKai left a comment

Choose a reason for hiding this comment

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

I looked through too quickly. PR 495 and 498 are actually two approaches instead of improving on top of the other. I will comment on 498.

@equalsJeffH
Copy link
Contributor Author

@AngeloKai

PR 495 and 498 are actually two approaches instead of improving on top of the other

actually they are not two approaches to the same thing -- they address different problems. pr #495 is more simple and ready-to-merge AFAIK. PR #498 needs more work.

Copy link
Contributor

@rlin1 rlin1 left a comment

Choose a reason for hiding this comment

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

LGTM

index.bs Outdated
@@ -859,7 +859,8 @@ When this method is invoked, the user agent MUST execute the following algorithm
and |clientExtensions| as parameters.

Note: In this case, the [=[RP]=] did not supply a list of acceptable credential descriptors. Thus the
authenticator is being asked to exercise any credential it may possess that is bound to the [=[RP]=].
authenticator is being asked to exercise any credential it may possess that is bound to
the [=[RP]=] identified by |rpId|.
Copy link
Contributor Author

Choose a reason for hiding this comment

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

s/identified/as identified/

@equalsJeffH
Copy link
Contributor Author

equalsJeffH commented Jun 27, 2017

@rlin1 -- thanks. I have minor comment on the clarification you added -- i can fix it if you like.
update: I fixed it in below commit 9ff5bbf

Copy link
Contributor

@AngeloKai AngeloKai left a comment

Choose a reason for hiding this comment

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

Thx Jeff for the clarification. If this is considered a standalone PR, I'd consider it approved.

@equalsJeffH
Copy link
Contributor Author

can we go ahead and merge this given @rlin1's and @AngeloKai's reviews? thx.

@jcjones jcjones merged commit 49da4d6 into master Jul 5, 2017
WebAuthnBot pushed a commit that referenced this pull request Jul 5, 2017
@emlun emlun deleted the jeffh-fixup-algs-contd-2 branch April 23, 2019 14:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants