Bug 202561 – [WebAuthn] Add more information to _WKWebAuthenticationPanel

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

[Jiewen Tan]

Needs a way to inform clients what transports are being used currently.

[Radar WebKit Bug Importer]

<rdar://problem/55973910>

[Jiewen Tan]

Created attachment 381658 [details]
Patch

[Jiewen Tan]

This patch here conflicts with Bug 202563 on some minor ways. Will rebase it depending on which one get landed first.

[youenn fablet]

Comment on attachment 381658 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=381658&action=review

> Source/WebKit/UIProcess/API/APIWebAuthenticationPanel.cpp:53
> +        m_transports.append(AuthenticatorTransport::Usb);

uncheckedAppend if we have m_transports.reserveInitialCapacity

> Source/WebKit/UIProcess/WebAuthentication/AuthenticatorManager.cpp:321
> +    page->uiClient().runWebAuthenticationPanel(*page, panel, [transports = WTFMove(transports), weakPanel = makeWeakPtr(panel), weakThis = makeWeakPtr(*this), this] (WebAuthenticationPanelResult result) {

Do we need to capture transports since we have weakPanel that stores roughly the same information?

> Source/WebKit/UIProcess/WebAuthentication/AuthenticatorManager.cpp:358
> +        processGoogleLegacyAppIdSupportExtension(options.extensions, transports);
> +    }, [&](const  PublicKeyCredentialRequestOptions& options) {

s/  / /

> Source/WebKit/UIProcess/WebAuthentication/AuthenticatorManager.h:87
> +    virtual void respondReceivedInternal(Respond&&) { };

s/;//

[Jiewen Tan]

Comment on attachment 381658 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=381658&action=review

Thanks Youenn for r+ this patch.

>> Source/WebKit/UIProcess/API/APIWebAuthenticationPanel.cpp:53
>> +        m_transports.append(AuthenticatorTransport::Usb);
> 
> uncheckedAppend if we have m_transports.reserveInitialCapacity

Thanks for catching me every time for this mistakes. Now, it is in my head.

>> Source/WebKit/UIProcess/WebAuthentication/AuthenticatorManager.cpp:321
>> +    page->uiClient().runWebAuthenticationPanel(*page, panel, [transports = WTFMove(transports), weakPanel = makeWeakPtr(panel), weakThis = makeWeakPtr(*this), this] (WebAuthenticationPanelResult result) {
> 
> Do we need to capture transports since we have weakPanel that stores roughly the same information?

No, probably not. The Panel only capture transports for external authenticators, i.e., USB and NFC. And we do have experimental support for Internal authenticators which are not going to expose to clients at this moment.

>> Source/WebKit/UIProcess/WebAuthentication/AuthenticatorManager.cpp:358
>> +    }, [&](const  PublicKeyCredentialRequestOptions& options) {
> 
> s/  / /

Fixed.

>> Source/WebKit/UIProcess/WebAuthentication/AuthenticatorManager.h:87
>> +    virtual void respondReceivedInternal(Respond&&) { };
> 
> s/;//

Fixed.

[Jiewen Tan]

Created attachment 381714 [details]
Patch for landing

[WebKit Commit Bot]

Comment on attachment 381714 [details]
Patch for landing

Rejecting attachment 381714 [details] from commit-queue.

Failed to run "['/Volumes/Data/EWS/WebKit/Tools/Scripts/webkit-patch', '--status-host=webkit-queues.webkit.org', '--bot-id=webkit-cq-03', 'apply-attachment', '--no-update', '--non-interactive', 381714, '--port=mac']" exit_code: 2 cwd: /Volumes/Data/EWS/WebKit

Logging in as [e-mail]...
Fetching: https://bugs.webkit.org/attachment.cgi?id=381714&action=edit
Fetching: https://bugs.webkit.org/show_bug.cgi?id=202561&ctype=xml&excludefield=attachmentdata
Processing 1 patch from 1 bug.
Processing patch 381714 from bug 202561.
Fetching: https://bugs.webkit.org/attachment.cgi?id=381714
Failed to run "[u'/Volumes/Data/EWS/WebKit/Tools/Scripts/svn-apply', '--force']" exit_code: 1 cwd: /Volumes/Data/EWS/WebKit

Parsed 18 diffs from patch file(s).
patching file Source/WebCore/ChangeLog
Hunk #1 succeeded at 1 with fuzz 3.
patching file Source/WebKit/ChangeLog
Hunk #1 succeeded at 1 with fuzz 3.
patching file Source/WebCore/Modules/webauthn/AuthenticatorCoordinator.cpp
patching file Source/WebCore/Modules/webauthn/WebAuthenticationConstants.h
patching file Source/WebKit/UIProcess/API/APIWebAuthenticationPanel.cpp
Hunk #1 FAILED at 30.
1 out of 1 hunk FAILED -- saving rejects to file Source/WebKit/UIProcess/API/APIWebAuthenticationPanel.cpp.rej
patching file Source/WebKit/UIProcess/API/APIWebAuthenticationPanel.h
Hunk #2 FAILED at 47.
1 out of 2 hunks FAILED -- saving rejects to file Source/WebKit/UIProcess/API/APIWebAuthenticationPanel.h.rej
patching file Source/WebKit/UIProcess/API/Cocoa/_WKWebAuthenticationPanel.h
patching file Source/WebKit/UIProcess/API/Cocoa/_WKWebAuthenticationPanel.mm
patching file Source/WebKit/UIProcess/WebAuthentication/AuthenticatorManager.cpp
patching file Source/WebKit/UIProcess/WebAuthentication/AuthenticatorManager.h
patching file Source/WebKit/UIProcess/WebAuthentication/Cocoa/NfcService.h
patching file Source/WebKit/UIProcess/WebAuthentication/Cocoa/NfcService.mm
patching file Source/WebKit/UIProcess/WebAuthentication/Mock/MockAuthenticatorManager.cpp
patching file Source/WebKit/UIProcess/WebAuthentication/Mock/MockAuthenticatorManager.h
patching file Tools/ChangeLog
Hunk #1 succeeded at 1 with fuzz 3.
patching file Tools/TestWebKitAPI/TestWebKitAPI.xcodeproj/project.pbxproj
patching file Tools/TestWebKitAPI/Tests/WebKitCocoa/_WKWebAuthenticationPanel.mm
patching file Tools/TestWebKitAPI/Tests/WebKitCocoa/web-authentication-make-credential-hid.html

Failed to run "[u'/Volumes/Data/EWS/WebKit/Tools/Scripts/svn-apply', '--force']" exit_code: 1 cwd: /Volumes/Data/EWS/WebKit

Full output: https://webkit-queues.webkit.org/results/13170214

[Jiewen Tan]

Committed r251498: <https://trac.webkit.org/changeset/251498>

[Jiewen Tan]

Committed r251512: <https://trac.webkit.org/changeset/251512>

[Jiewen Tan]

(In reply to Jiewen Tan from comment #9)
> Committed r251512: <https://trac.webkit.org/changeset/251512>

This is a speculative build fix.

[Alex Christensen]

Comment on attachment 381714 [details]
Patch for landing

View in context: https://bugs.webkit.org/attachment.cgi?id=381714&action=review

> Tools/TestWebKitAPI/Tests/WebKitCocoa/_WKWebAuthenticationPanel.mm:284
> +TEST(WebAuthenticationPanel, PanelHidSuccess2)

This test times out a lot on the bots.
https://results.webkit.org/?suite=api-tests&test=TestWebKitAPI.WebAuthenticationPanel.PanelHidSuccess2

[Jiewen Tan]

(In reply to Alex Christensen from comment #11)
> Comment on attachment 381714 [details]
> Patch for landing
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=381714&action=review
> 
> > Tools/TestWebKitAPI/Tests/WebKitCocoa/_WKWebAuthenticationPanel.mm:284
> > +TEST(WebAuthenticationPanel, PanelHidSuccess2)
> 
> This test times out a lot on the bots.
> https://results.webkit.org/?suite=api-tests&test=TestWebKitAPI.
> WebAuthenticationPanel.PanelHidSuccess2

Thanks, Alex. Will investigate.

[Jiewen Tan]

*** Bug 203380 has been marked as a duplicate of this bug. ***

[Jiewen Tan]

A test fix:
Committed r251562: <https://trac.webkit.org/changeset/251562>

[Jiewen Tan]

Another attempt to fix the test:
Committed r251579: <https://trac.webkit.org/changeset/251579>

[Aakash Jain]

TestWebKitAPI.WebAuthenticationPanel.PanelHidSuccess2 is still failing, both on iOS and mac. It is slowing down EWS (since in case of any test failure, EWS has to retry the tests to check for flakiness, and and re-run the tests on clean tree to verify if the test is pre-existing). 

https://ews-build.webkit.org/#/builders/3
https://ews-build.webkit.org/#/builders/9

[Matt Lewis]

All commits were rolled out in https://trac.webkit.org/changeset/251602/webkit

[Jiewen Tan]

Created attachment 382041 [details]
Patch for relanding

[Brent Fulgham]

Comment on attachment 382041 [details]
Patch for relanding

r=me. Please keep an eye on the Windows build failure -- I'm not sure why this change would impact it.

[Jiewen Tan]

Created attachment 382242 [details]
Patch for relanding

[Jiewen Tan]

Created attachment 382245 [details]
Patch for relanding

[Jiewen Tan]

Comment on attachment 382245 [details]
Patch for relanding

Thanks Brent for r+ the patch too. I borrowed an EWS bot (159) from Aakash. And this patch passes consistently on that bot with both macOS and iOS simulator for 1000 times. cq+. Will watch out for other bots' behavior.

[Jiewen Tan]

Created attachment 382255 [details]
EWS 159 iOS simulator debug results

[WebKit Commit Bot]

Comment on attachment 382245 [details]
Patch for relanding

Clearing flags on attachment: 382245

Committed r251762: <https://trac.webkit.org/changeset/251762>

[Jiewen Tan]

*** Bug 204187 has been marked as a duplicate of this bug. ***