Bug 206112 - [WebAuthn] Implement SPI to tell UI clients to select assertion responses
Summary: [WebAuthn] Implement SPI to tell UI clients to select assertion responses
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: 206263
Blocks: 181943
  Show dependency treegraph
 
Reported: 2020-01-10 16:39 PST by Jiewen Tan
Modified: 2021-12-24 16:55 PST (History)
9 users (show)

See Also:


Attachments
Patch (44.73 KB, patch)
2020-01-10 17:14 PST, Jiewen Tan
no flags Details | Formatted Diff | Diff
Patch (47.62 KB, patch)
2020-01-13 11:59 PST, Alex Christensen
no flags Details | Formatted Diff | Diff
Patch (50.22 KB, patch)
2020-01-13 12:23 PST, Jiewen Tan
no flags Details | Formatted Diff | Diff
Patch (57.49 KB, patch)
2020-01-13 14:57 PST, Jiewen Tan
achristensen: review+
Details | Formatted Diff | Diff
Patch for landing (59.71 KB, patch)
2020-01-14 12:22 PST, Jiewen Tan
commit-queue: commit-queue-
Details | Formatted Diff | Diff
Patch for Landing (60.77 KB, patch)
2020-01-14 16:25 PST, Jiewen Tan
no flags Details | Formatted Diff | Diff
Patch for Landing (58.86 KB, patch)
2020-01-14 16:29 PST, Jiewen Tan
no flags Details | Formatted Diff | Diff
Patch for Landing (59.44 KB, patch)
2020-01-14 16:49 PST, Jiewen Tan
no flags Details | Formatted Diff | Diff
Patch for Landing (68.11 KB, patch)
2020-01-14 17:16 PST, Jiewen Tan
no flags 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-01-10 16:39:07 PST
Implement SPI to tell UI clients to select assertion responses.
Comment 1 Radar WebKit Bug Importer 2020-01-10 16:40:04 PST
<rdar://problem/58495733>
Comment 2 Jiewen Tan 2020-01-10 17:14:25 PST
Created attachment 387403 [details]
Patch
Comment 3 Jiewen Tan 2020-01-13 11:02:43 PST
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.
Comment 4 Jiewen Tan 2020-01-13 11:08:11 PST
The build error seems to be related to unified build. I'm building locally to see what files are missing.
Comment 5 Alex Christensen 2020-01-13 11:59:51 PST
Created attachment 387552 [details]
Patch
Comment 6 Jiewen Tan 2020-01-13 12:23:47 PST
Created attachment 387555 [details]
Patch
Comment 7 Alex Christensen 2020-01-13 12:59:31 PST
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.
Comment 8 Jiewen Tan 2020-01-13 14:39:34 PST
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.
Comment 9 Jiewen Tan 2020-01-13 14:57:48 PST
Created attachment 387572 [details]
Patch
Comment 10 Alex Christensen 2020-01-13 16:41:07 PST
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.
Comment 11 Jiewen Tan 2020-01-14 12:14:09 PST
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.
Comment 12 Jiewen Tan 2020-01-14 12:22:41 PST
Created attachment 387689 [details]
Patch for landing
Comment 13 WebKit Commit Bot 2020-01-14 12:52:08 PST
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 commit-queue@webkit.org...
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
Comment 14 Jiewen Tan 2020-01-14 13:37:31 PST
Committed r254533: <https://trac.webkit.org/changeset/254533>
Comment 15 Aakash Jain 2020-01-14 14:22:00 PST
(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
Comment 16 Jiewen Tan 2020-01-14 14:31:08 PST
A build fix:
Committed r254535: <https://trac.webkit.org/changeset/254535>
Comment 17 WebKit Commit Bot 2020-01-14 15:08:25 PST
Re-opened since this is blocked by bug 206263
Comment 18 Ryan Haddad 2020-01-14 15:12:15 PST
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
Comment 19 Jiewen Tan 2020-01-14 16:25:08 PST
Created attachment 387724 [details]
Patch for Landing
Comment 20 Jiewen Tan 2020-01-14 16:29:36 PST
Created attachment 387727 [details]
Patch for Landing
Comment 21 Jiewen Tan 2020-01-14 16:49:59 PST
Created attachment 387734 [details]
Patch for Landing
Comment 22 Jiewen Tan 2020-01-14 17:16:00 PST
Created attachment 387737 [details]
Patch for Landing
Comment 23 Jiewen Tan 2020-01-14 18:50:18 PST
Committed r254554: <https://trac.webkit.org/changeset/254554>