Bug 206112 – [WebAuthn] Implement SPI to tell UI clients to select assertion responses

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

[Jiewen Tan]

Implement SPI to tell UI clients to select assertion responses.

[Radar WebKit Bug Importer]

<rdar://problem/58495733>

[Jiewen Tan]

Created attachment 387403 [details]
Patch

[Jiewen Tan]

Comment on attachment 387403 [details]
Patch

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

> Source/WebKit/UIProcess/WebAuthentication/Cocoa/WebAuthenticationPanelClient.mm:33
> +#import "WKSharedAPICast.h"

Removed.

> Tools/TestWebKitAPI/Tests/WebKitCocoa/_WKWebAuthenticationPanel.mm:42
> +#import <wtf/text/Base64.h>

Removed.

> Tools/TestWebKitAPI/Tests/WebKitCocoa/_WKWebAuthenticationPanel.mm:95
> +        EXPECT_TRUE([response.displayName isEqual:@"John P. Smith"] || [response.displayName isEqual:@""]);

Need to check userHandler as well.

[Jiewen Tan]

The build error seems to be related to unified build. I'm building locally to see what files are missing.

[Alex Christensen]

Created attachment 387552 [details]
Patch

[Jiewen Tan]

Created attachment 387555 [details]
Patch

[Alex Christensen]

Comment on attachment 387555 [details]
Patch

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

> Source/WebKit/UIProcess/API/APIWebAuthenticationAssertionResponse.h:40
> +    WTF::String name() const { return m_response->name(); }

const WTF::String&
This requires a change in WebCore, too.

> Source/WebKit/UIProcess/API/APIWebAuthenticationAssertionResponse.h:44
> +    const Ref<WebCore::AuthenticatorAssertionResponse>& response() { return m_response; }

Can you make this a const WebCore::AuthenticatorAssertionResponse& ?

> Source/WebKit/UIProcess/API/Cocoa/_WKWebAuthenticationAssertionResponse.mm:30
> +    RetainPtr<NSData> _userHandle;

This shouldn't be here.  If you make WebAuthenticationAssertionResponse::userHandle return an API::Data, you can just return wrapper of that.

> Source/WebKit/UIProcess/WebAuthentication/Cocoa/WebAuthenticationPanelClient.mm:155
> +    // Call delegates in the next run loop to prevent clients' reentrance that would potentially modify the state
> +    // of the current run loop in unexpected ways.
> +    RunLoop::main().dispatch([weakThis = makeWeakPtr(*this), this, nsResponses = WTFMove(nsResponses), completionHandler = WTFMove(completionHandler)] () mutable {

This shouldn't be necessary, and if it is then it shouldn't be in the API layer.

> Source/WebKit/UIProcess/WebAuthentication/Cocoa/WebAuthenticationPanelClient.mm:157
> +            completionHandler(static_cast<API::WebAuthenticationAssertionResponse&>([[nsResponses firstObject] _apiObject]).response());

Will the set of responses ever be empty?  I see no such guarantee.

[Jiewen Tan]

Comment on attachment 387555 [details]
Patch

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

Thanks Alex for reviewing the patch.

>> Source/WebKit/UIProcess/API/APIWebAuthenticationAssertionResponse.h:40
>> +    WTF::String name() const { return m_response->name(); }
> 
> const WTF::String&
> This requires a change in WebCore, too.

Right. Fixed.

>> Source/WebKit/UIProcess/API/APIWebAuthenticationAssertionResponse.h:44
>> +    const Ref<WebCore::AuthenticatorAssertionResponse>& response() { return m_response; }
> 
> Can you make this a const WebCore::AuthenticatorAssertionResponse& ?

It is doable. However, at CtapAuthenticator::continueGetNextAssertionAfterResponseReceived where the value is going to be used, I have to use const_cast<AuthenticatorAssertionResponse*>(&response) to take a value out from a hashset. Fixed.

>> Source/WebKit/UIProcess/API/Cocoa/_WKWebAuthenticationAssertionResponse.mm:30
>> +    RetainPtr<NSData> _userHandle;
> 
> This shouldn't be here.  If you make WebAuthenticationAssertionResponse::userHandle return an API::Data, you can just return wrapper of that.

Fixed.

>> Source/WebKit/UIProcess/WebAuthentication/Cocoa/WebAuthenticationPanelClient.mm:155
>> +    RunLoop::main().dispatch([weakThis = makeWeakPtr(*this), this, nsResponses = WTFMove(nsResponses), completionHandler = WTFMove(completionHandler)] () mutable {
> 
> This shouldn't be necessary, and if it is then it shouldn't be in the API layer.

It is necessary. Every delegate in this class has it as a result of a bug fix. Why not? What's the difference of doing it here v.s. the implementation layer?

>> Source/WebKit/UIProcess/WebAuthentication/Cocoa/WebAuthenticationPanelClient.mm:157
>> +            completionHandler(static_cast<API::WebAuthenticationAssertionResponse&>([[nsResponses firstObject] _apiObject]).response());
> 
> Will the set of responses ever be empty?  I see no such guarantee.

No, it shouldn't. I should assert it above.

[Jiewen Tan]

Created attachment 387572 [details]
Patch

[Alex Christensen]

Comment on attachment 387572 [details]
Patch

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

r=me with a bunch of nits.

> Source/WebKit/UIProcess/API/APIWebAuthenticationAssertionResponse.cpp:48
> +static void releaseUserHandle(unsigned char*, const void* data)

This can be inline by making it a lambda that doesn't capture anything.

> Source/WebKit/UIProcess/API/APIWebAuthenticationAssertionResponse.cpp:51
> +    static_cast<ArrayBuffer*>(const_cast<void*>(data))->deref();

Can we change the const void* to just void* in FreeDataFunction instead of this?  A callback that gives you something to destroy should not give you a const pointer.

> Source/WebKit/UIProcess/API/APIWebAuthenticationAssertionResponse.cpp:59
> +        data = API::Data::createWithoutCopying((const unsigned char*)userHandle->data(), userHandle->byteLength(), releaseUserHandle, userHandle);

reinterpret_cast rather than c-style cast.

> Source/WebKit/UIProcess/API/Cocoa/_WKWebAuthenticationPanel.h:73
> +- (void)panel:(_WKWebAuthenticationPanel *)panel selectAssertionResponses:(NSArray < _WKWebAuthenticationAssertionResponse *> *)responses completionHandler:(void (^)(_WKWebAuthenticationAssertionResponse *))completionHandler WK_API_AVAILABLE(macos(WK_MAC_TBA), ios(WK_IOS_TBA));

selectAssertionResponse
Shouldn't be plural.  You're supposed to select one.

> Source/WebKit/UIProcess/WebAuthentication/Cocoa/WebAuthenticationPanelClient.mm:156
> +    RunLoop::main().dispatch([weakThis = makeWeakPtr(*this), this, nsResponses = WTFMove(nsResponses), completionHandler = WTFMove(completionHandler)] () mutable {

The original bug fix should've gone in the C++ caller rather than the API layer, which shouldn't contain any behavior-modifying code.  For example, if someone were to make another API for this behavior, like expanding the C API, they could create an API that doesn't have this.  All APIs should have this, so it needs to be in the implementation code, not the API code.

> Source/WebKit/UIProcess/WebAuthentication/fido/CtapAuthenticator.cpp:158
> +                auto returnResponse = m_assertionResponses.take(const_cast<AuthenticatorAssertionResponse*>(&response));

I think it would be better to pass a non-const AuthenticatorAssertionResponse& than this.

[Jiewen Tan]

Comment on attachment 387572 [details]
Patch

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

Thanks Alex for r+ the patch.

>> Source/WebKit/UIProcess/API/APIWebAuthenticationAssertionResponse.cpp:48
>> +static void releaseUserHandle(unsigned char*, const void* data)
> 
> This can be inline by making it a lambda that doesn't capture anything.

Fixed. My bad. I followed those old ways straightly.

>> Source/WebKit/UIProcess/API/APIWebAuthenticationAssertionResponse.cpp:51
>> +    static_cast<ArrayBuffer*>(const_cast<void*>(data))->deref();
> 
> Can we change the const void* to just void* in FreeDataFunction instead of this?  A callback that gives you something to destroy should not give you a const pointer.

Maybe it is true, but I don't think this patch should do the modification.

>> Source/WebKit/UIProcess/API/APIWebAuthenticationAssertionResponse.cpp:59
>> +        data = API::Data::createWithoutCopying((const unsigned char*)userHandle->data(), userHandle->byteLength(), releaseUserHandle, userHandle);
> 
> reinterpret_cast rather than c-style cast.

Fixed.

>> Source/WebKit/UIProcess/API/Cocoa/_WKWebAuthenticationPanel.h:73
>> +- (void)panel:(_WKWebAuthenticationPanel *)panel selectAssertionResponses:(NSArray < _WKWebAuthenticationAssertionResponse *> *)responses completionHandler:(void (^)(_WKWebAuthenticationAssertionResponse *))completionHandler WK_API_AVAILABLE(macos(WK_MAC_TBA), ios(WK_IOS_TBA));
> 
> selectAssertionResponse
> Shouldn't be plural.  You're supposed to select one.

Good point!

>> Source/WebKit/UIProcess/WebAuthentication/Cocoa/WebAuthenticationPanelClient.mm:156
>> +    RunLoop::main().dispatch([weakThis = makeWeakPtr(*this), this, nsResponses = WTFMove(nsResponses), completionHandler = WTFMove(completionHandler)] () mutable {
> 
> The original bug fix should've gone in the C++ caller rather than the API layer, which shouldn't contain any behavior-modifying code.  For example, if someone were to make another API for this behavior, like expanding the C API, they could create an API that doesn't have this.  All APIs should have this, so it needs to be in the implementation code, not the API code.

I have filed Bug 206248 as a follow up.

>> Source/WebKit/UIProcess/WebAuthentication/fido/CtapAuthenticator.cpp:158
>> +                auto returnResponse = m_assertionResponses.take(const_cast<AuthenticatorAssertionResponse*>(&response));
> 
> I think it would be better to pass a non-const AuthenticatorAssertionResponse& than this.

I don't think so. I think it might be good to add another overload method to take to accept a const pointer. This is a compromise over the existing HashSet API.

[Jiewen Tan]

Created attachment 387689 [details]
Patch for landing

[WebKit Commit Bot]

Comment on attachment 387689 [details]
Patch for landing

Rejecting attachment 387689 [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', 387689, '--port=mac']" exit_code: 2 cwd: /Volumes/Data/EWS/WebKit

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

Parsed 30 diffs from patch file(s).
patching file Source/WebKit/ChangeLog
Hunk #1 succeeded at 1 with fuzz 3.
patching file Source/WebCore/Modules/webauthn/AuthenticatorAssertionResponse.h
patching file Source/WebKit/Shared/API/APIObject.h
patching file Source/WebKit/Shared/Cocoa/APIObject.mm
patching file Source/WebKit/Sources.txt
Hunk #1 succeeded at 341 (offset 3 lines).
patching file Source/WebKit/SourcesCocoa.txt
patching file Source/WebKit/UIProcess/API/APIResourceLoadClient.h
patching file Source/WebKit/UIProcess/API/APIWebAuthenticationAssertionResponse.cpp
patching file Source/WebKit/UIProcess/API/APIWebAuthenticationAssertionResponse.h
patching file Source/WebKit/UIProcess/API/APIWebAuthenticationPanelClient.h
patching file Source/WebKit/UIProcess/API/Cocoa/_WKWebAuthenticationAssertionResponse.h
patching file Source/WebKit/UIProcess/API/Cocoa/_WKWebAuthenticationAssertionResponse.mm
patching file Source/WebKit/UIProcess/API/Cocoa/_WKWebAuthenticationAssertionResponseInternal.h
patching file Source/WebKit/UIProcess/API/Cocoa/_WKWebAuthenticationPanel.h
patching file Source/WebKit/UIProcess/API/Cocoa/_WKWebAuthenticationPanel.mm
patching file Source/WebKit/UIProcess/Cocoa/ResourceLoadDelegate.h
patching file Source/WebKit/UIProcess/Cocoa/ResourceLoadDelegate.mm
patching file Source/WebKit/UIProcess/WebAuthentication/Authenticator.h
patching file Source/WebKit/UIProcess/WebAuthentication/AuthenticatorManager.cpp
patching file Source/WebKit/UIProcess/WebAuthentication/AuthenticatorManager.h
patching file Source/WebKit/UIProcess/WebAuthentication/Cocoa/WebAuthenticationPanelClient.h
patching file Source/WebKit/UIProcess/WebAuthentication/Cocoa/WebAuthenticationPanelClient.mm
patching file Source/WebKit/UIProcess/WebAuthentication/fido/CtapAuthenticator.cpp
patching file Source/WebKit/UIProcess/WebAuthentication/fido/CtapAuthenticator.h
Hunk #1 FAILED at 63.
1 out of 1 hunk FAILED -- saving rejects to file Source/WebKit/UIProcess/WebAuthentication/fido/CtapAuthenticator.h.rej
patching file Source/WebKit/WebKit.xcodeproj/project.pbxproj
Hunk #1 succeeded at 1098 (offset 3 lines).
Hunk #2 succeeded at 3776 (offset 18 lines).
Hunk #3 succeeded at 6883 (offset 30 lines).
Hunk #4 succeeded at 8568 (offset 31 lines).
Hunk #5 succeeded at 9942 (offset 33 lines).
Hunk #6 succeeded at 10026 (offset 33 lines).
patching file Tools/ChangeLog
Hunk #1 succeeded at 1 with fuzz 3.
patching file Tools/TestWebKitAPI/TestWebKitAPI.xcodeproj/project.pbxproj
Hunk #5 FAILED at 4874.
Hunk #6 FAILED at 5055.
2 out of 6 hunks FAILED -- saving rejects to file Tools/TestWebKitAPI/TestWebKitAPI.xcodeproj/project.pbxproj.rej
patching file Tools/TestWebKitAPI/Tests/WebKitCocoa/_WKWebAuthenticationPanel.mm
patching file Tools/TestWebKitAPI/Tests/WebKitCocoa/web-authentication-get-assertion-hid-multiple-accounts.html
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/13304288

[Jiewen Tan]

Committed r254533: <https://trac.webkit.org/changeset/254533>

[Aakash Jain]

(In reply to Jiewen Tan from comment #14)
> Committed r254533: <https://trac.webkit.org/changeset/254533>
This seems to have broken iOS and macOS builds, tracked in https://bugs.webkit.org/show_bug.cgi?id=206259

[Jiewen Tan]

A build fix:
Committed r254535: <https://trac.webkit.org/changeset/254535>

[WebKit Commit Bot]

Re-opened since this is blocked by bug 206263

[Ryan Haddad]

Builds were still broken after the build fix: https://build.webkit.org/builders/Apple-Catalina-Release-Build/builds/2446

Reverted both changes in https://trac.webkit.org/changeset/254539

[Jiewen Tan]

Created attachment 387724 [details]
Patch for Landing

[Jiewen Tan]

Created attachment 387727 [details]
Patch for Landing

[Jiewen Tan]

Created attachment 387734 [details]
Patch for Landing

[Jiewen Tan]

Created attachment 387737 [details]
Patch for Landing

[Jiewen Tan]

Committed r254554: <https://trac.webkit.org/changeset/254554>