Bug 206112

<rdar://problem/58495733>

Created attachment 387403 [details] Patch

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.

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

Created attachment 387552 [details] Patch

Created attachment 387555 [details] Patch

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

Created attachment 387572 [details] Patch

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

Created attachment 387689 [details] Patch for landing

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

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

(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

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

Re-opened since this is blocked by bug 206263

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

Created attachment 387724 [details] Patch for Landing

Created attachment 387727 [details] Patch for Landing

Created attachment 387734 [details] Patch for Landing

Created attachment 387737 [details] Patch for Landing

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