Bug 219708 – [WebAuthn] Adopt new UI for the Security Key makeCredential flow

• Status: RESOLVED
• Resolution: FIXED
• Component: WebKit Misc.

[Jiewen Tan]

Adopt new UI for the Security Key makeCredential flow.

[Radar WebKit Bug Importer]

<rdar://problem/72154735>

[Jiewen Tan]

Created attachment 415807 [details]
Patch

[Brent Fulgham]

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 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]

Created attachment 415825 [details]
Patch

[Jiewen Tan]

Created attachment 415838 [details]
Patch

[EWS]

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]

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