Bug 190784 - [WebAuthN] Import CTAP device request/response converters from Chromium
Summary: [WebAuthN] Import CTAP device request/response converters from Chromium
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebCore Misc. (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Jiewen Tan
URL:
Keywords: InRadar
Depends on:
Blocks: 181943
  Show dependency treegraph
 
Reported: 2018-10-21 18:37 PDT by Jiewen Tan
Modified: 2018-11-08 16:24 PST (History)
8 users (show)

See Also:


Attachments
Patch (144.45 KB, patch)
2018-10-21 19:10 PDT, Jiewen Tan
no flags Details | Formatted Diff | Diff
Patch (144.64 KB, patch)
2018-10-22 18:33 PDT, Jiewen Tan
no flags Details | Formatted Diff | Diff
Patch (144.64 KB, patch)
2018-10-22 18:38 PDT, Jiewen Tan
no flags Details | Formatted Diff | Diff
Patch (145.11 KB, patch)
2018-10-22 19:06 PDT, Jiewen Tan
no flags Details | Formatted Diff | Diff
Patch (145.01 KB, patch)
2018-10-30 19:02 PDT, Jiewen Tan
no flags Details | Formatted Diff | Diff
Patch (145.18 KB, patch)
2018-10-31 12:22 PDT, Jiewen Tan
no flags Details | Formatted Diff | Diff
Patch (145.19 KB, patch)
2018-10-31 12:43 PDT, Jiewen Tan
no flags Details | Formatted Diff | Diff
Patch (145.20 KB, patch)
2018-10-31 13:56 PDT, Jiewen Tan
bfulgham: review+
Details | Formatted Diff | Diff
Patch for landing (144.72 KB, patch)
2018-11-08 01:33 PST, Jiewen Tan
jiewen_tan: commit-queue-
Details | Formatted Diff | Diff
Patch for landing (144.62 KB, patch)
2018-11-08 01:50 PST, Jiewen Tan
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Jiewen Tan 2018-10-21 18:37:25 PDT
Import CTAP device request/response converters from Chromium such that we can convert WebAuthN request into proper binaries that CTAP devices could understand and understand their binary responses.
Comment 1 Jiewen Tan 2018-10-21 19:10:29 PDT
Created attachment 352879 [details]
Patch
Comment 2 EWS Watchlist 2018-10-21 19:12:30 PDT Comment hidden (obsolete)
Comment 3 Radar WebKit Bug Importer 2018-10-22 11:38:21 PDT
<rdar://problem/45460333>
Comment 4 Jiewen Tan 2018-10-22 18:33:33 PDT
Created attachment 352940 [details]
Patch
Comment 5 EWS Watchlist 2018-10-22 18:36:07 PDT Comment hidden (obsolete)
Comment 6 Jiewen Tan 2018-10-22 18:38:00 PDT
Created attachment 352941 [details]
Patch
Comment 7 EWS Watchlist 2018-10-22 18:41:24 PDT Comment hidden (obsolete)
Comment 8 Jiewen Tan 2018-10-22 19:06:51 PDT
Created attachment 352944 [details]
Patch
Comment 9 EWS Watchlist 2018-10-22 19:09:35 PDT Comment hidden (obsolete)
Comment 10 Jiewen Tan 2018-10-30 19:02:55 PDT
Created attachment 353455 [details]
Patch
Comment 11 EWS Watchlist 2018-10-30 19:04:57 PDT Comment hidden (obsolete)
Comment 12 Jiewen Tan 2018-10-31 12:22:26 PDT
Created attachment 353518 [details]
Patch
Comment 13 EWS Watchlist 2018-10-31 12:25:45 PDT Comment hidden (obsolete)
Comment 14 Jiewen Tan 2018-10-31 12:43:10 PDT
Created attachment 353519 [details]
Patch
Comment 15 EWS Watchlist 2018-10-31 12:45:37 PDT Comment hidden (obsolete)
Comment 16 Jiewen Tan 2018-10-31 13:56:09 PDT
Created attachment 353527 [details]
Patch
Comment 17 EWS Watchlist 2018-10-31 13:58:10 PDT
Attachment 353527 [details] did not pass style-queue:


ERROR: Tools/TestWebKitAPI/TestWebKitAPI.xcodeproj/project.pbxproj:3586:  developmentRegion is not en.  [xcodeproj/settings] [5]
ERROR: Source/WebCore/Modules/webauthn/fido/AuthenticatorSupportedOptions.h:70:  Inline functions should not be in classes annotated with WEBCORE_EXPORT. Remove the macro from the class and apply it to each appropriate method, or move the inline function definition out-of-line.  [build/webcore_export] [4]
ERROR: Source/WebCore/Modules/webauthn/fido/AuthenticatorSupportedOptions.h:71:  Inline functions should not be in classes annotated with WEBCORE_EXPORT. Remove the macro from the class and apply it to each appropriate method, or move the inline function definition out-of-line.  [build/webcore_export] [4]
ERROR: Source/WebCore/Modules/webauthn/fido/AuthenticatorSupportedOptions.h:72:  Inline functions should not be in classes annotated with WEBCORE_EXPORT. Remove the macro from the class and apply it to each appropriate method, or move the inline function definition out-of-line.  [build/webcore_export] [4]
ERROR: Source/WebCore/Modules/webauthn/fido/AuthenticatorSupportedOptions.h:73:  Inline functions should not be in classes annotated with WEBCORE_EXPORT. Remove the macro from the class and apply it to each appropriate method, or move the inline function definition out-of-line.  [build/webcore_export] [4]
ERROR: Source/WebCore/Modules/webauthn/fido/AuthenticatorSupportedOptions.h:74:  Inline functions should not be in classes annotated with WEBCORE_EXPORT. Remove the macro from the class and apply it to each appropriate method, or move the inline function definition out-of-line.  [build/webcore_export] [4]
ERROR: Source/WebCore/Modules/webauthn/cbor/CBORValue.h:165:  Inline functions should not be in classes annotated with WEBCORE_EXPORT. Remove the macro from the class and apply it to each appropriate method, or move the inline function definition out-of-line.  [build/webcore_export] [4]
ERROR: Source/WebCore/Modules/webauthn/fido/AuthenticatorGetInfoResponse.h:56:  Inline functions should not be in classes annotated with WEBCORE_EXPORT. Remove the macro from the class and apply it to each appropriate method, or move the inline function definition out-of-line.  [build/webcore_export] [4]
ERROR: Source/WebCore/Modules/webauthn/fido/AuthenticatorGetInfoResponse.h:57:  Inline functions should not be in classes annotated with WEBCORE_EXPORT. Remove the macro from the class and apply it to each appropriate method, or move the inline function definition out-of-line.  [build/webcore_export] [4]
ERROR: Source/WebCore/Modules/webauthn/fido/AuthenticatorGetInfoResponse.h:58:  Inline functions should not be in classes annotated with WEBCORE_EXPORT. Remove the macro from the class and apply it to each appropriate method, or move the inline function definition out-of-line.  [build/webcore_export] [4]
ERROR: Source/WebCore/Modules/webauthn/fido/AuthenticatorGetInfoResponse.h:59:  Inline functions should not be in classes annotated with WEBCORE_EXPORT. Remove the macro from the class and apply it to each appropriate method, or move the inline function definition out-of-line.  [build/webcore_export] [4]
ERROR: Source/WebCore/Modules/webauthn/fido/AuthenticatorGetInfoResponse.h:60:  Inline functions should not be in classes annotated with WEBCORE_EXPORT. Remove the macro from the class and apply it to each appropriate method, or move the inline function definition out-of-line.  [build/webcore_export] [4]
ERROR: Source/WebCore/Modules/webauthn/fido/AuthenticatorGetInfoResponse.h:61:  Inline functions should not be in classes annotated with WEBCORE_EXPORT. Remove the macro from the class and apply it to each appropriate method, or move the inline function definition out-of-line.  [build/webcore_export] [4]
Total errors found: 13 in 30 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 18 Brent Fulgham 2018-11-07 13:47:37 PST
Comment on attachment 353527 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=353527&action=review

Looks good. I had a minor suggestion for the decode logic (labelling), but not necessary to land the code.

> Source/WebCore/Modules/webauthn/fido/DeviceResponseConverter.cpp:98
> +    auto it = decodedMap.find(CBOR(1));

It seems like these ordinal values for the decodeMap might be more meaningful with constant definitions. Is the only meaning of this map entry that it is the first of 'n' values? Or is the first value always a specific piece of information? Just like the request map is accessed through keys, it would be nice to access the response with keys, too (for legibility).

Still, since this is Google code, maybe we want to keep it as pristine as possible to aid in porting over changes.
Comment 19 Jiewen Tan 2018-11-07 14:18:03 PST
Comment on attachment 353527 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=353527&action=review

Thanks Brent for r+ the patch.

> Source/WebCore/Modules/webauthn/PublicKeyCredentialRequestOptions.h:42
> +    std::optional<UserVerificationRequirement> userVerification { UserVerificationRequirement::Preferred };

It doesn't need to be std::optional since it has a default value.

>> Source/WebCore/Modules/webauthn/fido/DeviceResponseConverter.cpp:98
>> +    auto it = decodedMap.find(CBOR(1));
> 
> It seems like these ordinal values for the decodeMap might be more meaningful with constant definitions. Is the only meaning of this map entry that it is the first of 'n' values? Or is the first value always a specific piece of information? Just like the request map is accessed through keys, it would be nice to access the response with keys, too (for legibility).
> 
> Still, since this is Google code, maybe we want to keep it as pristine as possible to aid in porting over changes.

Agreed. The original code is using auto, and I didn't think too much when I changed the coding style.

> Source/WebCore/Modules/webgpu/WebGPUAdapter.cpp:34
> +#include <wtf/Ref.h>

Should have remove this.
Comment 20 Jiewen Tan 2018-11-08 01:33:02 PST
Created attachment 354221 [details]
Patch for landing
Comment 21 EWS Watchlist 2018-11-08 01:34:47 PST Comment hidden (obsolete)
Comment 22 Jiewen Tan 2018-11-08 01:50:34 PST
Created attachment 354222 [details]
Patch for landing
Comment 23 WebKit Commit Bot 2018-11-08 02:30:06 PST
Comment on attachment 354222 [details]
Patch for landing

Clearing flags on attachment: 354222

Committed r237983: <https://trac.webkit.org/changeset/237983>
Comment 24 Jiewen Tan 2018-11-08 11:42:29 PST
Landed a build fix for iOS device build:
Committed r237993: <https://trac.webkit.org/changeset/237993>
Comment 25 Jiewen Tan 2018-11-08 16:24:46 PST
Landed a proper build fix:
Committed r238010: <https://trac.webkit.org/changeset/238010>