Bug 189277

Summary: [WebAuthN] Polish AuthenticatorManager and rename it to AuthenticatorCoordinator
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, esprehn+autocc, ews-watchlist, jiewen_tan, kondapallykalyan, mkwst, 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
none
Patch
none
Patch
cdumez: review+
Patch for landing
none
Patch for landing none

Jiewen Tan
Reported 2018-09-04 15:39:40 PDT
This task aims to polish AuthenticatorManager such that: 1) it is no longer a singleton. Instead, it will live with Page. It was a singleton simply because static PublicKeyCredential::isUserVerifyingPlatformAuthenticatorAvailable() have to access it. However, the issue can be solved by adding an attribute [CallWith=Document]. Therefore, there is no such need. Also, the singleton is illy implemented as it also owns a single IPC proxy to UI Process which means different web pages will talk to the same web page proxy. Anyway, making it live with Page should solve the problem. 2) Since we are now planning to support external authenticators, the manager of all authenticators will then have to live in UI Process which makes this AuthenticatorManager obsolete. Instead, rename it to AuthenticatorCoordinator. 3) Rename CredentialsMessenger to AuthenticatorCoordinatorClient to tight it to WebAuthN. Also, simplify the message reply model as PublicKeyCredentialCreationOptions/PublicKeyCredentialRequestOptions => ExceptionData/PublicKeyCredential for makeCredential/getAssertion operations.
Attachments
Patch (271.53 KB, patch)
2018-09-08 17:09 PDT, Jiewen Tan 		no flags
DetailsFormatted DiffDiff
Patch (271.05 KB, patch)
2018-09-08 20:21 PDT, Jiewen Tan 		no flags
DetailsFormatted DiffDiff
Patch (271.34 KB, patch)
2018-09-10 11:11 PDT, Jiewen Tan 		cdumez: review+
DetailsFormatted DiffDiff
Patch for landing (271.29 KB, patch)
2018-09-10 15:29 PDT, Jiewen Tan 		no flags
DetailsFormatted DiffDiff
Patch for landing (271.93 KB, patch)
2018-09-10 16:00 PDT, Jiewen Tan 		no flags
DetailsFormatted DiffDiff
Show Obsolete (3) View All Add attachment proposed patch, testcase, etc.
Radar WebKit Bug Importer
Comment 1 2018-09-04 15:40:29 PDT
<rdar://problem/44115936>
Jiewen Tan
Comment 2 2018-09-08 17:09:32 PDT
Created attachment 349266 [details] Patch
Jiewen Tan
Comment 3 2018-09-08 20:21:10 PDT
Created attachment 349274 [details] Patch
Jiewen Tan
Comment 4 2018-09-10 11:11:33 PDT
Created attachment 349318 [details] Patch
Chris Dumez
Comment 5 2018-09-10 12:35:41 PDT
Comment on attachment 349318 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=349318&action=review r=me with suggestions. > Source/WebCore/Modules/webauthn/PublicKeyCredentialCreationOptions.h:98 > + encoder.encodeFixedLengthData(reinterpret_cast<const uint8_t*>(user.id.data()), user.id.length(), 1); Why do we need the reinterpret_cast here? Also, it seems like this could construct a temporary IPC::DataReference and encode that. > Source/WebCore/page/Page.h:910 > + std::unique_ptr<AuthenticatorCoordinator> m_authenticatorCoordinator; Could this be a UniqueRef instead of a unique_ptr? It seems it cannot be null.
Jiewen Tan
Comment 6 2018-09-10 14:06:58 PDT
Comment on attachment 349318 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=349318&action=review Thanks Chris for r+ this patch. >> Source/WebCore/Modules/webauthn/PublicKeyCredentialCreationOptions.h:98 >> + encoder.encodeFixedLengthData(reinterpret_cast<const uint8_t*>(user.id.data()), user.id.length(), 1); > > Why do we need the reinterpret_cast here? > > Also, it seems like this could construct a temporary IPC::DataReference and encode that. It doesn't need to. IPC::DataReference is in WebKit so I could't use it in WebCore. >> Source/WebCore/page/Page.h:910 >> + std::unique_ptr<AuthenticatorCoordinator> m_authenticatorCoordinator; > > Could this be a UniqueRef instead of a unique_ptr? It seems it cannot be null. Sure, I was not aware of UniqueRef.
Jiewen Tan
Comment 7 2018-09-10 15:29:20 PDT
Created attachment 349336 [details] Patch for landing
Jiewen Tan
Comment 8 2018-09-10 16:00:01 PDT
Created attachment 349347 [details] Patch for landing
Jiewen Tan
Comment 9 2018-09-10 16:01:42 PDT
Comment on attachment 349347 [details] Patch for landing Added #include <JavaScriptCore/HeapInlines.h> to VRStageParameters.cpp to resolve the Win EWS bots linker error.
WebKit Commit Bot
Comment 10 2018-09-11 00:59:28 PDT
Comment on attachment 349347 [details] Patch for landing Clearing flags on attachment: 349347 Committed r235888: <https://trac.webkit.org/changeset/235888>
Jiewen Tan
Comment 11 2018-09-11 10:56:47 PDT
Build fix: Committed r235900: <https://trac.webkit.org/changeset/235900>
Note You need to log in before you can comment on or make changes to this bug.