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

Store/update credential record last in RP ops #2215

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

emlun
Copy link
Member

@emlun emlun commented Nov 27, 2024

Closes #2204.


Preview | Diff

the [=[RP]=] SHOULD fail the [=registration ceremony=].
1. If all the above steps are successful,
store |credentialRecord| in the [=user account=] that was denoted in <code>|pkOptions|.{{PublicKeyCredentialCreationOptions/user}}</code>
and continue the [=registration ceremony=] as appropriate.
Copy link
Contributor

@zacknewman zacknewman Nov 27, 2024

Choose a reason for hiding this comment

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

What does "continue the registration ceremony" mean? The way I interpret that is that RPs may have specific criteria in addition to what's stated in the spec, and this line means for the RP to continue with said criteria. However if that is true, then it's possible the ceremony fails; but we already stated to store the credentialRecord. If my interpretation is correct, then this needs to be re-worded like:

If all the above steps are successful, continue the registration ceremony as appropriate. If the ceremony is successful, then store credentialRecord

Ditto for the authentication ceremony section.

In other words the last "portion"/clause of the last sentence of the last step should be about storing/updating the credential unless subsequent clauses or sentences do not describe processes that even have the possibility for causing the ceremony to fail.

Copy link
Contributor

@sbweeden sbweeden left a comment

Choose a reason for hiding this comment

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

I am fine with these changes.

@emlun
Copy link
Member Author

emlun commented Nov 27, 2024

2024-11-27: @emlun to resolve @zacknewman's comments, then OK to merge.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Should steps 28 and 29 occur before Step 27 in the registration ceremony
3 participants