Summary: | [WebAuthn] Adopt new UI for the Security Key makeCredential flow | ||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Jiewen Tan <jiewen_tan> | ||||||||
Component: | WebKit Misc. | Assignee: | Jiewen Tan <jiewen_tan> | ||||||||
Status: | RESOLVED FIXED | ||||||||||
Severity: | Normal | CC: | bfulgham, jiewen_tan, webkit-bug-importer | ||||||||
Priority: | P2 | Keywords: | InRadar | ||||||||
Version: | WebKit Nightly Build | ||||||||||
Hardware: | Unspecified | ||||||||||
OS: | Unspecified | ||||||||||
Bug Depends on: | |||||||||||
Bug Blocks: | 181943 | ||||||||||
Attachments: |
|
Description
Jiewen Tan
2020-12-09 14:56:20 PST
Created attachment 415807 [details]
Patch
Comment on attachment 415807 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=415807&action=review It looks like the bots aren't happy with this yet, but I think the overall approach is fine. Please answer my questions about whether it is okay to remove the USB-related code for existing WebAuthn flows. r=me if you fix the EWS failures. > Source/WebKit/Scripts/process-entitlements.sh:264 > + plistbuddy Add :com.apple.frontboard.launchapplications bool YES Do we need this for Catalyst apps, too? > Source/WebKit/UIProcess/WebAuthentication/AuthenticatorManager.cpp:-345 > - continue; Why is it okay to remove this code now? If we aren't using the new UI, don't we still need to do this? > Source/WebKit/UIProcess/WebAuthentication/AuthenticatorManager.cpp:-392 > - m_pendingRequestData.panelResult = result; Ditto. > Source/WebKit/UIProcess/WebAuthentication/Cocoa/AuthenticatorPresenterCoordinator.mm:76 > +#endif // PLATFORM(IOS) Is there ever going to be a macOS version of this? If we do intend to implement it, maybe we should add a 'notImplemented()' call in an #else clause? Comment on attachment 415807 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=415807&action=review Thanks Brent for r+ this patch. >> Source/WebKit/Scripts/process-entitlements.sh:264 >> + plistbuddy Add :com.apple.frontboard.launchapplications bool YES > > Do we need this for Catalyst apps, too? We have a separate radar for that. >> Source/WebKit/UIProcess/WebAuthentication/AuthenticatorManager.cpp:-345 >> - continue; > > Why is it okay to remove this code now? If we aren't using the new UI, don't we still need to do this? Yes, the code was there because there was this period when we only supported USB security keys and there wasn't any UI in Safari. Now that all Safari should have UI. >> Source/WebKit/UIProcess/WebAuthentication/Cocoa/AuthenticatorPresenterCoordinator.mm:76 >> +#endif // PLATFORM(IOS) > > Is there ever going to be a macOS version of this? If we do intend to implement it, maybe we should add a 'notImplemented()' call in an #else clause? Fixed! Created attachment 415825 [details]
Patch
Created attachment 415838 [details]
Patch
Committed r270616: <https://trac.webkit.org/changeset/270616> All reviewed patches have been landed. Closing bug and clearing flags on attachment 415838 [details]. A build fix: Committed r270619: <https://trac.webkit.org/changeset/270619> |