Bug 189283 - [WebAuthN] Polish WebAuthN auto-test environment
Summary: [WebAuthN] Polish WebAuthN auto-test environment
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: 182769
  Show dependency treegraph
 
Reported: 2018-09-04 16:31 PDT by Jiewen Tan
Modified: 2018-10-01 13:55 PDT (History)
7 users (show)

See Also:


Attachments
Patch (125.75 KB, patch)
2018-09-27 15:55 PDT, Jiewen Tan
no flags Details | Formatted Diff | Diff
Patch (126.00 KB, patch)
2018-09-27 16:32 PDT, Jiewen Tan
cdumez: review+
Details | Formatted Diff | Diff
Patch for landing (125.96 KB, patch)
2018-09-28 16:02 PDT, Jiewen Tan
jiewen_tan: commit-queue+
Details | Formatted Diff | Diff
Patch for landing (125.89 KB, patch)
2018-09-28 16:21 PDT, 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-09-04 16:31:58 PDT
Currently, tests of WebAuthN spread in three places:
1) LayoutTests/http/tests/webauthn/
2) LayoutTests/http/tests/wpt/webauthn/
3) TestWebKitAPI/Tests/ios/LocalAuthenticator.mm

It will be great to have them all in LayoutTests/http/tests/wpt/webauthn/. This could be achieved by:
1) making tests under LayoutTests/http/tests/webauthn/ become wpt async tests.
2) mocking transport layer abstraction, says HIDService, and providing necessary test configuration functions in testRunner.idl such that layout tests could then directly manipulate the most underlying layer of WebAuthN to have test behaviors/results they expected.
Comment 1 Radar WebKit Bug Importer 2018-09-04 16:34:47 PDT
<rdar://problem/44117828>
Comment 2 Jiewen Tan 2018-09-27 15:55:54 PDT
Created attachment 351016 [details]
Patch
Comment 3 Jiewen Tan 2018-09-27 16:32:37 PDT
Created attachment 351019 [details]
Patch
Comment 4 Chris Dumez 2018-09-28 15:42:15 PDT
Comment on attachment 351019 [details]
Patch

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

r=me

> Source/WebKit/UIProcess/API/C/WKWebsiteDataStoreRef.cpp:582
> +        MockWebAuthenticationConfiguration::Local local;

Any reason we are using a local variable instead of assigning straight into configuration.local?

> Source/WebKit/UIProcess/WebAuthentication/Mock/MockLocalConnection.mm:77
> +        if (!configuration.local.value().acceptAuthentication) {

Cannot we use onfiguration.local->acceptAuthentication ?

> Source/WebKit/UIProcess/WebAuthentication/Mock/MockLocalConnection.mm:90
> +        if (!configuration.local.value().acceptAuthentication) {

ditto.

> Source/WebKit/UIProcess/WebAuthentication/Mock/MockLocalConnection.mm:105
> +        if (!configuration.local.value().acceptAttestation) {

ditto.

> Source/WebKit/UIProcess/WebAuthentication/Mock/MockLocalConnection.mm:118
> +            (__bridge CFDataRef)adoptNS([[NSData alloc] initWithBase64EncodedString:configuration.local.value().privateKeyBase64 options:NSDataBase64DecodingIgnoreUnknownCharacters]).get(),

ditto.

> Source/WebKit/UIProcess/WebAuthentication/Mock/MockLocalService.cpp:45
> +    if (!m_configuration.local)

return !!m_configuration.local; ?
Comment 5 Jiewen Tan 2018-09-28 15:58:24 PDT
Comment on attachment 351019 [details]
Patch

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

Thanks Chris for r+ my patch.

>> Source/WebKit/UIProcess/API/C/WKWebsiteDataStoreRef.cpp:582
>> +        MockWebAuthenticationConfiguration::Local local;
> 
> Any reason we are using a local variable instead of assigning straight into configuration.local?

configuration.local is a std::optional. How?

>> Source/WebKit/UIProcess/WebAuthentication/Mock/MockLocalConnection.mm:77
>> +        if (!configuration.local.value().acceptAuthentication) {
> 
> Cannot we use onfiguration.local->acceptAuthentication ?

Sure.

>> Source/WebKit/UIProcess/WebAuthentication/Mock/MockLocalConnection.mm:90
>> +        if (!configuration.local.value().acceptAuthentication) {
> 
> ditto.

Fixed.

>> Source/WebKit/UIProcess/WebAuthentication/Mock/MockLocalConnection.mm:105
>> +        if (!configuration.local.value().acceptAttestation) {
> 
> ditto.

Fixed.

>> Source/WebKit/UIProcess/WebAuthentication/Mock/MockLocalConnection.mm:118
>> +            (__bridge CFDataRef)adoptNS([[NSData alloc] initWithBase64EncodedString:configuration.local.value().privateKeyBase64 options:NSDataBase64DecodingIgnoreUnknownCharacters]).get(),
> 
> ditto.

Fixed.

>> Source/WebKit/UIProcess/WebAuthentication/Mock/MockLocalService.cpp:45
>> +    if (!m_configuration.local)
> 
> return !!m_configuration.local; ?

I don't think so.
Comment 6 Chris Dumez 2018-09-28 15:59:44 PDT
Comment on attachment 351019 [details]
Patch

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

>>> Source/WebKit/UIProcess/API/C/WKWebsiteDataStoreRef.cpp:582
>>> +        MockWebAuthenticationConfiguration::Local local;
>> 
>> Any reason we are using a local variable instead of assigning straight into configuration.local?
> 
> configuration.local is a std::optional. How?

Ah, I missed that it was an optional. Please disregard this comment.

>>> Source/WebKit/UIProcess/WebAuthentication/Mock/MockLocalService.cpp:45
>>> +    if (!m_configuration.local)
>> 
>> return !!m_configuration.local; ?
> 
> I don't think so.

Why not? :)
Comment 7 Jiewen Tan 2018-09-28 16:02:10 PDT Comment hidden (obsolete)
Comment 8 Jiewen Tan 2018-09-28 16:04:50 PDT
Comment on attachment 351019 [details]
Patch

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

>>>> Source/WebKit/UIProcess/WebAuthentication/Mock/MockLocalService.cpp:45
>>>> +    if (!m_configuration.local)
>>> 
>>> return !!m_configuration.local; ?
>> 
>> I don't think so.
> 
> Why not? :)

I was thinking you suggested me to replace !m_configuration.local with !!m_configuration.local. lol.
Yup. Why not.
Comment 9 Jiewen Tan 2018-09-28 16:21:17 PDT
Created attachment 351137 [details]
Patch for landing
Comment 10 WebKit Commit Bot 2018-09-28 17:02:49 PDT
Comment on attachment 351137 [details]
Patch for landing

Clearing flags on attachment: 351137

Committed r236625: <https://trac.webkit.org/changeset/236625>