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: 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 ews-feeder: commit-queue-

Description Jiewen Tan 2020-12-09 14:56:20 PST
Adopt new UI for the Security Key makeCredential flow.
Comment 1 Radar WebKit Bug Importer 2020-12-09 14:56:40 PST
<rdar://problem/72154735>
Comment 2 Jiewen Tan 2020-12-09 16:07:37 PST
Created attachment 415807 [details]
Patch
Comment 3 Brent Fulgham 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?
Comment 4 Jiewen Tan 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!
Comment 5 Jiewen Tan 2020-12-09 19:49:26 PST
Created attachment 415825 [details]
Patch
Comment 6 Jiewen Tan 2020-12-09 23:11:29 PST
Created attachment 415838 [details]
Patch
Comment 7 EWS 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].
Comment 8 Jiewen Tan 2020-12-10 03:27:28 PST
A build fix:
Committed r270619: <https://trac.webkit.org/changeset/270619>