[Webkit-unassigned] [Bug 230906] Adopt platform UI for WebAuthn

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=230906

David Kilzer (:ddkilzer) <ddkilzer at webkit.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |ddkilzer at webkit.org

--- Comment #5 from David Kilzer (:ddkilzer) <ddkilzer at webkit.org> ---
Comment on attachment 439610
  --> https://bugs.webkit.org/attachment.cgi?id=439610
Patch

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

r- for the reasons Brent mentioned, plus a few others.

> Source/WebKit/UIProcess/WebAuthentication/Cocoa/WebAuthenticatorCoordinatorProxy.mm:73
> +static inline RetainPtr<NSString> toNSString(UserVerificationRequirement userVerificationRequirement)
> +{
> +    switch (userVerificationRequirement) {
> +    case UserVerificationRequirement::Required:
> +        return adoptNS(@"required");
> +    case UserVerificationRequirement::Discouraged:
> +        return adoptNS(@"discouraged");
> +    case UserVerificationRequirement::Preferred:
> +        return adoptNS(@"preferred");
> +    }
> +
> +    return adoptNS(@"preferred");

There is no need to adoptNS() here as these are immortal NSString constants.

> Source/WebKit/UIProcess/WebAuthentication/Cocoa/WebAuthenticatorCoordinatorProxy.mm:81
> +    NSMutableArray<NSString *> *transports = nil;
> +    size_t transportCount = descriptor.transports.size();
> +    if (transportCount) {
> +        transports = [[NSMutableArray alloc] initWithCapacity:transportCount];

The `transports` variable here should be a RetainPtr<> (no need to initialize as C++ does that for you):

    RetainPtr<NSMutableArray<NSString *> *> transports;

And assignment should use adoptNS():

         transports = adoptNS([[NSMutableArray alloc] initWithCapacity:transportCount]);

> Source/WebKit/UIProcess/WebAuthentication/Cocoa/WebAuthenticatorCoordinatorProxy.mm:105
> +    return adoptNS([allocASCPublicKeyCredentialDescriptorInstance() initWithCredentialID:toNSData(descriptor.idVector).get() transports:WTFMove(transports)]);

The `WTFMove(transports)` expression is wrong; I'm surprised it even compiles!

This should change to `transports.get()` once you use RetainPtr<>/adopNS() above.

> Source/WebKit/UIProcess/WebAuthentication/Cocoa/WebAuthenticatorCoordinatorProxy.mm:120
> +    std::optional<PublicKeyCredentialCreationOptions::AuthenticatorSelectionCriteria> authenticatorSelection = options.authenticatorSelection;
> +    if (authenticatorSelection) {
> +        std::optional<AuthenticatorAttachment> attachment = authenticatorSelection->authenticatorAttachment;
> +        if (attachment == AuthenticatorAttachment::Platform)
> +            requestTypes = ASCCredentialRequestTypePlatformPublicKeyRegistration;
> +        else if (attachment == AuthenticatorAttachment::CrossPlatform)
> +            requestTypes = ASCCredentialRequestTypeSecurityKeyPublicKeyRegistration;

What if `attachment` is not set?  I don't see a check to ensure that `attachment` is set before comparing it.

Also, shouldn't these comparisons use`*attachment` instead of `attachment` to get the value out of the std::optional<>?

> Source/WebKit/UIProcess/WebAuthentication/Cocoa/WebAuthenticatorCoordinatorProxy.mm:122
> +        userVerification = toNSString(authenticatorSelection->userVerification).get();

If you ever returned non-immortal NSString objects from toNSString() in the future, the NSString returned by get() here would be over-released once this line of code completes.

It's better to change `userVerification` to be `RetainPtr<NSString>` instead, and drop the `.get()` here, and move the `.get()` below to where it's needed.

>> Source/WebKit/UIProcess/WebAuthentication/Cocoa/WebAuthenticatorCoordinatorProxy.mm:127
>> +    ASCCredentialRequestContext *requestContext = [allocASCCredentialRequestContextInstance() initWithRequestTypes:requestTypes];
> 
> You should adoptNS here to avoid the possibility of leaking this (e.g., if we add new code someday that does an early return or something).

Correct.  You can also use `auto` to reduce typing:

    auto requestContext = adoptNS([allocASCCredentialRequestContextInstance() initWithRequestTypes:requestTypes]);

>> Source/WebKit/UIProcess/WebAuthentication/Cocoa/WebAuthenticatorCoordinatorProxy.mm:130
>> +    ASCPublicKeyCredentialCreationOptions *credentialCreationOptions = [allocASCPublicKeyCredentialCreationOptionsInstance() init];
> 
> This should be wrapped in 'adoptNS', since the refcount is increased when you assign to the requestContext member, and we will leak if this isn't cleaned up.

Correct.  Same as previous comment using `auto` and `adoptNS()` here.

>> Source/WebKit/UIProcess/WebAuthentication/Cocoa/WebAuthenticatorCoordinatorProxy.mm:144
>> +    credentialCreationOptions.supportedAlgorithmIdentifiers = WTFMove(supportedAlgorithmIdentifiers);
> 
> I don't think this will do what you want. You are just moving a pointer, and the refcount will now be +1  (assuming supportedAlgorithmIdentifiers is refcounted).
> 
> I think you need to adoptNS the NSMutableArray, so it gets cleaned up.

Correct.  Please switch to adoptNS() and RetainPtr<> for `supportedAlgorithmIdentifiers`.

>> Source/WebKit/UIProcess/WebAuthentication/Cocoa/WebAuthenticatorCoordinatorProxy.mm:153
>> +        credentialCreationOptions.excludedCredentials = WTFMove(excludedCredentials);
> 
> Ditto the above.

Correct.  Please use adoptNS() and RetainPtr<> for `excludedCredentials`.

> Source/WebKit/UIProcess/WebAuthentication/Cocoa/WebAuthenticatorCoordinatorProxy.mm:174
> +    std::optional<AuthenticatorAttachment> attachment = options.authenticatorAttachment;
> +    if (attachment == AuthenticatorAttachment::Platform)
> +        requestTypes = ASCCredentialRequestTypePlatformPublicKeyAssertion;
> +    else if (attachment == AuthenticatorAttachment::CrossPlatform)
> +        requestTypes = ASCCredentialRequestTypeSecurityKeyPublicKeyAssertion;

Ditto from comment about about checking whether `attachment` is set before using it, and using `*attachment` instead of `attachment` here.

> Source/WebKit/UIProcess/WebAuthentication/Cocoa/WebAuthenticatorCoordinatorProxy.mm:176
> +    userVerification = toNSString(options.userVerification).get();

Change `userVerification` to be `RetainPtr<NSString>` in this method as well.

> Source/WebKit/UIProcess/WebAuthentication/Cocoa/WebAuthenticatorCoordinatorProxy.mm:181
> +    NSMutableArray<ASCPublicKeyCredentialDescriptor *> *allowedCredentials = nil;
> +    if (allowedCredentialCount) {
> +        allowedCredentials = [[NSMutableArray alloc] initWithCapacity:allowedCredentialCount];

Should use RetainPtr<> and adoptNS() here.

>> Source/WebKit/UIProcess/WebAuthentication/Cocoa/WebAuthenticatorCoordinatorProxy.mm:187
>> +    ASCCredentialRequestContext *requestContext = [allocASCCredentialRequestContextInstance() initWithRequestTypes:requestTypes];
> 
> I recommend adoptNS here to avoid future mistakes.

Correct.

>> Source/WebKit/UIProcess/WebAuthentication/Cocoa/WebAuthenticatorCoordinatorProxy.mm:193
>> +        requestContext.platformKeyCredentialAssertionOptions = [allocASCPublicKeyCredentialAssertionOptionsInstance() initWithKind:ASCPublicKeyCredentialKindPlatform relyingPartyIdentifier:options.rpId challenge:challenge userVerificationPreference:userVerification allowedCredentials:allowedCredentials];
> 
> I think this will leak.

Yes, both `allowedCredentials` and `requestContext` need to use RetainPtr<> types with adoptNS().

> Source/WebKit/UIProcess/WebAuthentication/Cocoa/WebAuthenticatorCoordinatorProxy.mm:198
> +    return adoptNS(requestContext);

As a bonus, you don't have to use adoptNS() here if you use it above.

This is one place where you could use WTFMove(requestContext) if the variable is already a RetainPtr<>, though.

> Source/WebKit/UIProcess/WebAuthentication/Cocoa/WebAuthenticatorCoordinatorProxy.mm:203
> +    RetainPtr<ASCCredentialRequestContext> requestContext = configureRegistrationRequestContext(WTFMove(options));

It's a bit strange to use `WTFMove(options)` here, but configureRegistrationRequestContext() doesn't take a rvalue reference.

I don't think this is wrong, but I'm not 100% sure.

> Source/WebKit/UIProcess/WebAuthentication/Cocoa/WebAuthenticatorCoordinatorProxy.mm:215
> +    ASCAgentProxy *proxy = [allocASCAgentProxyInstance() init];

Please use RetainPtr<>/adoptNS().

> Source/WebKit/UIProcess/WebAuthentication/Cocoa/WebAuthenticatorCoordinatorProxy.mm:218
> +    [proxy performAuthorizationRequestsForContext:requestContext.get() withClearanceHandler:makeBlockPtr([this, handler = WTFMove(handler), window = WTFMove(window)](NSXPCListenerEndpoint *daemonEnpoint, NSError *error) mutable {

What happens when the `this` object is deallocated?  Is this call async, or is guaranteed to finish before the method returns?

If it's async (or `this` can be deleted from another thread), we may need to look into using a WeakPtr for `this`.  If this all happens on the main thread, you don't need WeakPtr.

> Source/WebKit/UIProcess/WebAuthentication/Cocoa/WebAuthenticatorCoordinatorProxy.mm:226
> +        m_presenter = [allocASCAuthorizationRemotePresenterInstance() init];

This needs adoptNS() or it will leak since the assignment to the instance variable will +1 retain it again.

> Source/WebKit/UIProcess/WebAuthentication/Cocoa/WebAuthenticatorCoordinatorProxy.mm:269
> +                    exceptionCode = (ExceptionCode) error.code;

It's a bit dodgy to use a C-style cast here.  Usually we create a helper function to map from one type to the other, then provide a default value if an unknown value is seen.

Can an `error.code` be returned that doesn't map to `ExceptionCode`?

-- 
You are receiving this mail because:
You are the assignee for the bug.
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.webkit.org/pipermail/webkit-unassigned/attachments/20211004/b6a04251/attachment-0001.htm>