Bug 219708 - [WebAuthn] Adopt new UI for the Security Key makeCredential flow
Summary: [WebAuthn] Adopt new UI for the Security Key makeCredential flow
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKit Misc. (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Jiewen Tan
URL:
Keywords: InRadar
Depends on:
Blocks: 181943
  Show dependency treegraph
 
Reported: 2020-12-09 14:56 PST by Jiewen Tan
Modified: 2020-12-10 03:33 PST (History)
3 users (show)

See Also:


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

Note You need to log in before you can comment on or make changes to this bug.
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>