webauthn: Make PublicKeyCredential.toJSON() conform to the spec again [chromium/src : main]
39 views
Skip to first unread message
Martin Kreichgauer (Gerrit)
Jun 21, 2024, 1:18:19 PM (8 days ago) Jun 21
to Martin Kreichgauer, Adam Langley, Chromium LUCI CQ, Christian Biesinger, chromium...@chromium.org, Kentaro Hara, Kaan Icer, blink-revie...@chromium.org, blink-...@chromium.org, jmedle...@chromium.org, npm+...@chromium.org, yigu+...@chromium.org
Martin Kreichgauer voted and added 1 comment![]( "Open in Gerrit")
Votes added by Martin Kreichgauer
| Commit-Queue | +2 |
1 comment
File third_party/blink/renderer/modules/credentialmanagement/public_key_credential.cc
Line 141, Patchset 3:
[this, script_state](auto* json, auto* response) {Adam Langley . resolved
Martin KreichgauerI don't know how C++ works any more.
Lambdas with auto parameters are just fancy generics. I.e. `[](auto t) {...}` is basically syntactic sugar for something like `template <typename T> [](T t) {...}`. So I think all that's happening here is that `std::visit()` is instantiating two specializations of the generic lambda.
Though, now that I'm reading this again, I think just duplicating the code is easier to read.
Related details
Attention set is empty
Submit Requirements:
Code-Coverage
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Martin Kreichgauer (Gerrit)
Jun 21, 2024, 1:18:51 PM (8 days ago) Jun 21
to Martin Kreichgauer, Adam Langley, Chromium LUCI CQ, Christian Biesinger, chromium...@chromium.org, Kentaro Hara, Kaan Icer, blink-revie...@chromium.org, blink-...@chromium.org, jmedle...@chromium.org, npm+...@chromium.org, yigu+...@chromium.org
Attention needed from Martin Kreichgauer
Martin Kreichgauer voted and added 1 comment![]( "Open in Gerrit")
Votes added by Martin Kreichgauer
| Commit-Queue | +2 |
1 comment
Commit Message
Line 7, Patchset 3:
webauthn: Make PublicKeyCredential.toJSON() confirm to the spec againAdam Langley . resolved
Martin Kreichgauertypo: "conform"
Done
Related details
Attention is currently required from:
- Martin Kreichgauer
Submit Requirements:
Code-Coverage
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Chromium LUCI CQ (Gerrit)
Jun 21, 2024, 1:59:33 PM (8 days ago) Jun 21
to Martin Kreichgauer, Adam Langley, Christian Biesinger, chromium...@chromium.org, Kentaro Hara, Kaan Icer, blink-revie...@chromium.org, blink-...@chromium.org, jmedle...@chromium.org, npm+...@chromium.org, yigu+...@chromium.org
Chromium LUCI CQ submitted the change with unreviewed changes![]( "Open in Gerrit")
Unreviewed changes
3 is the latest approved patch-set.
The change was submitted with unreviewed changes in the following files:
```
The name of the file: third_party/blink/renderer/modules/credentialmanagement/public_key_credential.cc
Insertions: 30, Deletions: 27.
@@ -128,43 +128,46 @@
// if returned from a get() call. In the former case, the spec wants us to
// return a RegistrationResponseJSON, and in the latter an
// AuthenticationResponseJSON. We can't reflect the type of `response_`
- // though, so we serialize it to JSON first and branch on the output.
+ // though, so we serialize it to JSON first and branch on the result type.
absl::variant<AuthenticatorAssertionResponseJSON*,
AuthenticatorAttestationResponseJSON*>
response_json = response_->toJSON();
- // Depending on the underlying type held by `response_json`, this lambda will
- // either initialize a `RegistrationResponseJSON`, or an
- // `AuthenticationResponseJSON`. The two only differ only in the type of their
- // `response` members.
- auto init_registration_response_json_or_authentication_response_json =
- [this, script_state](auto* json, auto* response) {
- json->setId(id());
- json->setRawId(WebAuthnBase64UrlEncode(rawId()));
- json->setResponse(response);
- if (authenticator_attachment_.has_value()) {
- json->setAuthenticatorAttachment(*authenticator_attachment_);
- }
- json->setClientExtensionResults(
- AuthenticationExtensionsClientOutputsToJSON(script_state,
- *extension_outputs_));
- json->setType(type());
- };
-
+ // The return type of `toJSON()` is `PublicKeyCredentialJSON` which just
+ // aliases `object`, and thus this method just returns a `Value`.
v8::Local<v8::Value> result;
absl::visit(
base::Overloaded{
[&](AuthenticatorAttestationResponseJSON* attestation_response) {
- auto* json = RegistrationResponseJSON::Create();
- init_registration_response_json_or_authentication_response_json(
- json, attestation_response);
- result = json->ToV8(script_state);
+ auto* registration_response = RegistrationResponseJSON::Create();
+ registration_response->setId(id());
+ registration_response->setRawId(WebAuthnBase64UrlEncode(rawId()));
+ registration_response->setResponse(attestation_response);
+ if (authenticator_attachment_.has_value()) {
+ registration_response->setAuthenticatorAttachment(
+ *authenticator_attachment_);
+ }
+ registration_response->setClientExtensionResults(
+ AuthenticationExtensionsClientOutputsToJSON(
+ script_state, *extension_outputs_));
+ registration_response->setType(type());
+ result = registration_response->ToV8(script_state);
},
[&](AuthenticatorAssertionResponseJSON* assertion_response) {
- auto* json = AuthenticationResponseJSON::Create();
- init_registration_response_json_or_authentication_response_json(
- json, assertion_response);
- result = json->ToV8(script_state);
+ auto* authentication_response =
+ AuthenticationResponseJSON::Create();
+ authentication_response->setId(id());
+ authentication_response->setRawId(WebAuthnBase64UrlEncode(rawId()));
+ authentication_response->setResponse(assertion_response);
+ if (authenticator_attachment_.has_value()) {
+ authentication_response->setAuthenticatorAttachment(
+ *authenticator_attachment_);
+ }
+ authentication_response->setClientExtensionResults(
+ AuthenticationExtensionsClientOutputsToJSON(
+ script_state, *extension_outputs_));
+ authentication_response->setType(type());
+ result = authentication_response->ToV8(script_state);
}},
response_json);
return result;
```
Change information
Commit message:
webauthn: Make PublicKeyCredential.toJSON() conform to the spec again
There have been several changes since the original implementation:
- Some IDL dictionary members are now `required`
- `AuthenticatorAttestationResponseJSON` has additional fields
- `PublicKeyCredentialJSON` is now a typedef for `object`
This CL addresses all of these. This also makes the WPT pass again,
which had already been updated upstream to match the spec behavior
and was thus failing.
Bug: 40250593,626703
Change-Id: Iac37f5a364aa5ae3c3ccaa73f7b7e6be18a4e716
Reviewed-by: Adam Langley <a...@chromium.org>
Commit-Queue: Martin Kreichgauer <mart...@google.com>
Cr-Commit-Position: refs/heads/main@{#1318053}
Files:
- M third_party/blink/renderer/bindings/generated_in_modules.gni
- M third_party/blink/renderer/modules/credentialmanagement/authentication_response_json.idl
- M third_party/blink/renderer/modules/credentialmanagement/authenticator_assertion_response_json.idl
- M third_party/blink/renderer/modules/credentialmanagement/authenticator_attestation_response.cc
- M third_party/blink/renderer/modules/credentialmanagement/authenticator_attestation_response_json.idl
- M third_party/blink/renderer/modules/credentialmanagement/public_key_credential.cc
- M third_party/blink/renderer/modules/credentialmanagement/public_key_credential.h
- M third_party/blink/renderer/modules/credentialmanagement/public_key_credential_json.idl
- M third_party/blink/renderer/modules/credentialmanagement/registration_response_json.idl
- M third_party/blink/web_tests/TestExpectations
Change size: M
Delta: 10 files changed, 71 insertions(+), 58 deletions(-)
Branch: refs/heads/main
Submit Requirements:
Code-Review: +1 by Adam Langley
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Reply all
Reply to author
Forward
0 new messages