Bug 219708

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:

Keywords:

InRadar

Version:

WebKit Nightly Build

Hardware:

Unspecified

OS:

Unspecified

Bug Depends on:

Bug Blocks:

181943

Attachments:

Description	Flags
Patch	none
Patch	none
Patch	ews-feeder: commit-queue-

Jiewen Tan

Reported 2020-12-09 14:56:20 PST

Adopt new UI for the Security Key makeCredential flow.

Attachments
Patch (53.18 KB, patch) 2020-12-09 16:07 PST, Jiewen Tan	no flags	Details Formatted Diff Diff
Patch (53.30 KB, patch) 2020-12-09 19:49 PST, Jiewen Tan	no flags	Details Formatted Diff Diff
Patch (53.33 KB, patch) 2020-12-09 23:11 PST, Jiewen Tan	ews-feeder: commit-queue-	Details Formatted Diff Diff
Show Obsolete (2) View All Add attachment proposed patch, testcase, etc.

Radar WebKit Bug Importer

Comment 1 2020-12-09 14:56:40 PST

<rdar://problem/72154735>

Jiewen Tan

Comment 2 2020-12-09 16:07:37 PST

Created attachment 415807 [details] Patch

Brent Fulgham

Comment 3 2020-12-09 16:34:46 PST

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?

Jiewen Tan

Comment 4 2020-12-09 19:37:43 PST

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!

Jiewen Tan

Comment 5 2020-12-09 19:49:26 PST

Created attachment 415825 [details] Patch

Jiewen Tan

Comment 6 2020-12-09 23:11:29 PST

Created attachment 415838 [details] Patch

EWS

Comment 7 2020-12-10 00:42:08 PST

Committed r270616: <https://trac.webkit.org/changeset/270616> All reviewed patches have been landed. Closing bug and clearing flags on attachment 415838 [details].

Jiewen Tan

Comment 8 2020-12-10 03:27:28 PST

A build fix: Committed r270619: <https://trac.webkit.org/changeset/270619>

Note You need to log in before you can comment on or make changes to this bug.