Bug 189283

Summary: [WebAuthN] Polish WebAuthN auto-test environment
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, jiewen_tan, webkit-bug-importer, youennf
Priority: P2 Keywords: InRadar
Version: WebKit Nightly Build   
Hardware: Unspecified   
OS: Unspecified   
Bug Depends on:    
Bug Blocks: 182769    
Attachments:
Description Flags
Patch
none
Patch
cdumez: review+
Patch for landing
jiewen_tan: commit-queue+
Patch for landing none

Jiewen Tan
Reported 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.
Attachments
Patch (125.75 KB, patch)
2018-09-27 15:55 PDT, Jiewen Tan 		no flags
DetailsFormatted DiffDiff
Patch (126.00 KB, patch)
2018-09-27 16:32 PDT, Jiewen Tan 		cdumez: review+
DetailsFormatted DiffDiff
Patch for landing (125.96 KB, patch)
2018-09-28 16:02 PDT, Jiewen Tan 		jiewen_tan: commit-queue+
DetailsFormatted DiffDiff
Patch for landing (125.89 KB, patch)
2018-09-28 16:21 PDT, Jiewen Tan 		no flags
DetailsFormatted DiffDiff
Show Obsolete (2) View All Add attachment proposed patch, testcase, etc.
Radar WebKit Bug Importer
Comment 1 2018-09-04 16:34:47 PDT
<rdar://problem/44117828>
Jiewen Tan
Comment 2 2018-09-27 15:55:54 PDT
Created attachment 351016 [details] Patch
Jiewen Tan
Comment 3 2018-09-27 16:32:37 PDT
Created attachment 351019 [details] Patch
Chris Dumez
Comment 4 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; ?
Jiewen Tan
Comment 5 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.
Chris Dumez
Comment 6 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? :)
Jiewen Tan
Comment 7 2018-09-28 16:02:10 PDT Comment hidden (obsolete)
Jiewen Tan
Comment 8 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.
Jiewen Tan
Comment 9 2018-09-28 16:21:17 PDT
Created attachment 351137 [details] Patch for landing
WebKit Commit Bot
Comment 10 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>
Note You need to log in before you can comment on or make changes to this bug.