Bug 236820 – [WebAuthn] Support for conditional mediation

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

[[e-mail]]

Conditional mediation was recently merged into the Credentials Management specification (https://w3c.github.io/webappsec-credential-management/#dom-credentialmediationrequirement-conditional). This bug is to add support for this mediation requirement.

[[e-mail]]

rdar://84821947

[[e-mail]]

Created attachment 452463 [details]
Patch

[[e-mail]]

Created attachment 452466 [details]
Patch

[[e-mail]]

Created attachment 452479 [details]
Patch

[Chris Dumez]

Comment on attachment 452479 [details]
Patch

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

> Source/WebCore/Modules/credentialmanagement/BasicCredential.cpp:57
> +    document.page()->authenticatorCoordinator().isConditionalMediationAvailable(WTFMove(promise));

Why should probably null check there page, there is a reason Document::page() returns a raw pointer.

> Source/WebCore/Modules/credentialmanagement/CredentialRequestOptions.h:38
> +enum class MediationRequirement { Silent, Optional, Required, Conditional };

enum class MediationRequirement : uint8_t { ... }

> Source/WebCore/Modules/webauthn/AuthenticatorCoordinator.cpp:282
> +    // Async operation are dispatched and handled in the messenger.

operations

> Source/WebCore/Modules/webauthn/AuthenticatorCoordinatorClient.h:59
> +    virtual void isConditionalMediationAvailable(QueryCompletionHandler&&) { };

Given that this takes a CompletionHandler as parameter, I think this virtual function should either be:
1. Pure virtual (= 0).
or
2. The default implementation should call the completion handler.

Otherwise, not overriding this (seemingly optional) virtual function will results in crashes in debug builds because completion handlers aren't called.

This comment seems to apply to other virtual functions in this file too.

> Source/WebKit/UIProcess/WebAuthentication/Cocoa/WebAuthenticatorCoordinatorProxy.mm:263
> +    if (mediation && *mediation == MediationRequirement::Conditional && [requestContext respondsToSelector:@selector(setRequestStyle:)])

I believe you can you replace `mediation && *mediation == MediationRequirement::Conditional` with `mediation == MediationRequirement::Conditional`. std::optional's operator==() deals with this for you.

> Source/WebKit/UIProcess/WebAuthentication/Cocoa/WebAuthenticatorCoordinatorProxy.mm:315
> +        callOnMainRunLoop([handler = WTFMove(handler), credential = retainPtr(credential), error = retainPtr(error)] () mutable {

May want to call ensureOnMainRunLoop() if there is any chance this gets called on the main thread.

> Source/WebKit/UIProcess/WebAuthentication/Cocoa/WebAuthenticatorCoordinatorProxy.mm:316
> +            AuthenticatorResponseData response = { };

= { }; should not be necessary?

> Source/WebKit/UIProcess/WebAuthentication/Cocoa/WebAuthenticatorCoordinatorProxy.mm:318
> +            ExceptionData exceptionData = { };

ditto.

> Source/WebKit/UIProcess/WebAuthentication/WebAuthenticatorCoordinatorProxy.messages.in:30
> +    GetAssertion(WebCore::FrameIdentifier frameID, struct WebKit::FrameInfoData frameInfo, Vector<uint8_t> hash, struct WebCore::PublicKeyCredentialRequestOptions options, enum:int WebCore::CredentialRequestOptions::MediationRequirement mediation, bool processingUserGesture) -> (struct WebCore::AuthenticatorResponseData data, enum:int WebCore::AuthenticatorAttachment attachment, struct WebCore::ExceptionData exception) Async

`enum:uint8_t WebCore::CredentialRequestOptions::MediationRequirement`

> Source/WebKit/WebAuthnProcess/WebAuthnConnectionToWebProcess.cpp:104
> +    handleRequest({ WTFMove(hash), WTFMove(options), nullptr, WebAuthenticationPanelResult::Unavailable, nullptr, std::nullopt, { }, processingUserGesture, String(), nullptr, std::nullopt }, WTFMove(handler));

This doesn't use mediation ?

[[e-mail]]

Created attachment 452551 [details]
Patch

[Brent Fulgham]

Comment on attachment 452551 [details]
Patch

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

r=me

> Source/WebCore/Modules/credentialmanagement/BasicCredential.cpp:60
> +    page->authenticatorCoordinator().isConditionalMediationAvailable(WTFMove(promise));

I think this might be simpler using the old:
if (auto* page = document.page())
    page->authenticator...<<ETC>>

Since nothing else happens in this method if page is nullptr.

[Chris Dumez]

Comment on attachment 452551 [details]
Patch

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

> Source/WebCore/Modules/credentialmanagement/BasicCredential.cpp:59
> +        return;

Well, I would think we should reject the promise here..

[[e-mail]]

Created attachment 452555 [details]
Patch for landing

[Chris Dumez]

Comment on attachment 452555 [details]
Patch for landing

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

> Source/WebCore/Modules/credentialmanagement/BasicCredential.cpp:60
> +        promise.reject(Exception { NotSupportedError });

No big deal but I think InvalidStateError applies better for such cases.

[[e-mail]]

Created attachment 452557 [details]
Patch for landing

[[e-mail]]

Created attachment 452561 [details]
[fast-cq]Patch for landing

[EWS]

Committed r290184 (247512@main): <https://commits.webkit.org/247512@main>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 452561 [details].

[Aakash Jain]

Please ignore the red color of commit-queue bubble, it was landed successfully, the color is incorrect (Bug 234644).