Bug 189289

Summary: [WebAuthN] Import CTAP HID message and packet structure from Chromium
Product: WebKit Reporter: Jiewen Tan <jiewen_tan>
Component: WebCore Misc.Assignee: Jiewen Tan <jiewen_tan>
Status: RESOLVED FIXED    
Severity: Normal CC: alex.gaynor, bfulgham, cdumez, commit-queue, ews-watchlist, jiewen_tan, webkit-bug-importer, youennf
Priority: P2 Keywords: InRadar
Version: WebKit Nightly Build   
Hardware: Unspecified   
OS: Unspecified   
Bug Depends on:    
Bug Blocks: 181943    
Attachments:
Description Flags
Patch
bfulgham: review+
Patch for landing none

Description Jiewen Tan 2018-09-04 17:31:55 PDT
This task aims to import CTAP message coder from Chromium including but not limited to:
1. Commands: CtapMakeCredentialRequest, CtapGetAssertionRequest.
2. Responses: device_response_converter.
3. CTAP to U2F: u2f_command_constructor, AuthenticatorMakeCredentialResponse::CreateFromU2fRegisterResponse, AuthenticatorGetAssertionResponse::CreateFromU2fSignResponse.
Comment 1 Radar WebKit Bug Importer 2018-09-04 17:33:26 PDT
<rdar://problem/44120310>
Comment 2 Jiewen Tan 2018-10-09 00:55:32 PDT
Created attachment 351870 [details]
Patch
Comment 3 EWS Watchlist 2018-10-09 00:58:40 PDT
Attachment 351870 [details] did not pass style-queue:


ERROR: Source/WebCore/Modules/webauthn/fido/FidoHidMessage.h:65:  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/FidoHidMessage.h:66:  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/FidoHidMessage.h:67:  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/FidoHidPacket.h:53:  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/FidoHidPacket.h:54:  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/FidoHidPacket.h:80:  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/FidoHidPacket.h:81:  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/FidoHidPacket.h:104:  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: 8 in 15 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 4 Brent Fulgham 2018-10-09 09:23:08 PDT
Comment on attachment 351870 [details]
Patch

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

> Source/WebCore/Modules/webauthn/fido/FidoHidMessage.cpp:89
> +    if (initPacket == nullptr)

if (!initPacket)

> Source/WebKit/UIProcess/WebAuthentication/AuthenticatorManager.cpp:-194
> -

Please remove this whitespace change.
Comment 5 Brent Fulgham 2018-10-09 09:26:12 PDT
Comment on attachment 351870 [details]
Patch

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

> Source/WebCore/Modules/webauthn/fido/FidoHidMessage.h:67
> +    const Deque<std::unique_ptr<FidoHidPacket>>& getPacketsForTesting() const { return m_packets; }

Since this is a WEBCORE_EXPORT class, you should move these inlines to the implementation file to avoid the compiler getting angry:

"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]"

> Source/WebCore/Modules/webauthn/fido/FidoHidPacket.h:54
> +    uint32_t channelId() const { return m_channelId; }

Since this is a WEBCORE_EXPORT class, you should move these inlines to the implementation file to avoid the compiler getting angry:

"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]"

> Source/WebCore/Modules/webauthn/fido/FidoHidPacket.h:81
> +    uint16_t payloadLength() const { return m_payloadLength; }

Since this is a WEBCORE_EXPORT class, you should move these inlines to the implementation file to avoid the compiler getting angry:

"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]"

> Source/WebCore/Modules/webauthn/fido/FidoHidPacket.h:104
> +    uint8_t sequence() const { return m_sequence; }

Since this is a WEBCORE_EXPORT class, you should move these inlines to the implementation file to avoid the compiler getting angry:

"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]"
Comment 6 Jiewen Tan 2018-10-09 11:41:37 PDT
Comment on attachment 351870 [details]
Patch

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

Thanks Brent for r+ this patch.

>> Source/WebCore/Modules/webauthn/fido/FidoHidMessage.cpp:89
>> +    if (initPacket == nullptr)
> 
> if (!initPacket)

Fixed.

>> Source/WebCore/Modules/webauthn/fido/FidoHidMessage.h:67
>> +    const Deque<std::unique_ptr<FidoHidPacket>>& getPacketsForTesting() const { return m_packets; }
> 
> Since this is a WEBCORE_EXPORT class, you should move these inlines to the implementation file to avoid the compiler getting angry:
> 
> "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]"

Fixed.

>> Source/WebCore/Modules/webauthn/fido/FidoHidPacket.h:54
>> +    uint32_t channelId() const { return m_channelId; }
> 
> Since this is a WEBCORE_EXPORT class, you should move these inlines to the implementation file to avoid the compiler getting angry:
> 
> "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]"

Fixed.

>> Source/WebCore/Modules/webauthn/fido/FidoHidPacket.h:81
>> +    uint16_t payloadLength() const { return m_payloadLength; }
> 
> Since this is a WEBCORE_EXPORT class, you should move these inlines to the implementation file to avoid the compiler getting angry:
> 
> "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]"

Fixed.

>> Source/WebCore/Modules/webauthn/fido/FidoHidPacket.h:104
>> +    uint8_t sequence() const { return m_sequence; }
> 
> Since this is a WEBCORE_EXPORT class, you should move these inlines to the implementation file to avoid the compiler getting angry:
> 
> "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]"

Fixed.

>> Source/WebKit/UIProcess/WebAuthentication/AuthenticatorManager.cpp:-194
>> -
> 
> Please remove this whitespace change.

Fixed.
Comment 7 Jiewen Tan 2018-10-09 11:47:46 PDT
Comment on attachment 351870 [details]
Patch

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

>>> Source/WebCore/Modules/webauthn/fido/FidoHidMessage.h:67
>>> +    const Deque<std::unique_ptr<FidoHidPacket>>& getPacketsForTesting() const { return m_packets; }
>> 
>> Since this is a WEBCORE_EXPORT class, you should move these inlines to the implementation file to avoid the compiler getting angry:
>> 
>> "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]"
> 
> Fixed.

Just confirmed with Alex that inline methods are allowed in classes that are marked as WEBCORE_EXPORT and it is the style checker bug.
Comment 8 Jiewen Tan 2018-10-09 11:52:23 PDT
Created attachment 351900 [details]
Patch for landing
Comment 9 WebKit Commit Bot 2018-10-09 12:32:51 PDT
Comment on attachment 351900 [details]
Patch for landing

Clearing flags on attachment: 351900

Committed r236976: <https://trac.webkit.org/changeset/236976>