-
Notifications
You must be signed in to change notification settings - Fork 165
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
Move state updates to last in RP verification steps #1807
Conversation
This will help avoid conflicts with existing definitions, including [=scope=], as we introduce a struct for devicePubKey records as well.
The state should be updated only after verifying the signature. This change will be useful for the devicePubKey branch.
(You can also see a sneak peek of the draft for devicePubKey records here: jeffh-fix-1658-device-bound-key-extension...emlun:webauthn:dpk-credential-record#diff-5e793325cd2bfc452e268a4aa2f02b4024dd9584bd1db3c2595f61f1ecf7b985R7079) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks really good, makes sense to me.
:: The value returned from <code>|response|.{{AuthenticatorAttestationResponse/getTransports()}}</code>. | ||
|
||
: [=credential record/BE=] | ||
: [$credential record/BE$] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Any reason not to make this a bit more readable and call the column backupEligible
instead? It'll save people a click who don't know to equate "BE" to the flag's full name.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good point, thanks! But unrelated to this change, so let's tackle that in issue #1811.
:: The value of the [=authData/flags/BE=] [=flag=] in |authData|. | ||
|
||
: [=credential record/BS=] | ||
: [$credential record/BS$] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Any reason not to make this a bit more readable and call the column backupStatus
instead? It'll save people a click who don't know to equate "BS" to the flag's full name.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good point, thanks! But unrelated to this change, so let's tackle that in issue #1811.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
@MasterKale I'll consider your current comments resolved, but you're welcome to add any more comments either here (even after merging) or in #1811. |
This relates to (but does not fix) issue #1711, and will be useful for applying the new "credential record" (see #1773) abstraction to PR #1663. The stored state should be updated only after performing all validation and verification steps.
This also changes the definition type (and therefore autolink syntax) for the credential record struct members from
dfn
toabstract-op
. This will help avoid conflicts with existing definitions, including[=scope=]
, as we introduce a struct for devicePubKey records as well.Preview | Diff