Bug 218893 – [WebAuthn] Implement SPI for AuthenticationServices.Framework

• Component: WebKit Misc.
• Status: RESOLVED FIXED

[Jiewen Tan]

Implement SPI for AuthenticationServices.Framework.

[Radar WebKit Bug Importer]

<rdar://problem/71364731>

[Jiewen Tan]

Created attachment 414017 [details]
WIP.patch

[Alex Christensen]

Comment on attachment 414017 [details]
WIP.patch

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

> Source/WebKit/UIProcess/API/Cocoa/_WKWebAuthenticationPanel.h:85
> +    _WKPublicKeyCredentialTypePublicKey,

Why an enum with only one value?

> Source/WebKit/UIProcess/API/Cocoa/_WKWebAuthenticationPanel.h:209
> +@property (nullable, nonatomic, readonly, strong) _WKAuthenticationExtensionsClientOutputs* extensions;

* should be after space

> Source/WebKit/UIProcess/API/Cocoa/_WKWebAuthenticationPanel.h:246
> +@interface _WKWebAuthenticationPanel (WKDeprecated)

WK_API_DEPRECATED or WK_API_DEPRECATED_WITH_REPLACEMENT

> Source/WebKit/UIProcess/API/Cocoa/_WKWebAuthenticationPanel.mm:45
> +    if (!(self = [super init]))

Do you intend to add more here?  You could just return [super init] or omit this entirely.

[Jiewen Tan]

Comment on attachment 414017 [details]
WIP.patch

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

> Source/WebKit/ChangeLog:3
> +        [WebAuthn] Implement SPI for AuthenticationServices.Framework

This patch intends to get feedbacks from AuthenticationServices.Framework folks. And it's more or less a direct ObjC translations of the WebAuthn Web IDLs.

>> Source/WebKit/UIProcess/API/Cocoa/_WKWebAuthenticationPanel.h:85
>> +    _WKPublicKeyCredentialTypePublicKey,
> 
> Why an enum with only one value?

That's what's in the Web IDL...

> Source/WebKit/UIProcess/API/Cocoa/_WKWebAuthenticationPanel.h:206
> +@interface _WKAuthenticatorResponse : NSObject <NSSecureCoding, NSCopying>

Note: ClientData will not be returned here.

>> Source/WebKit/UIProcess/API/Cocoa/_WKWebAuthenticationPanel.h:209
>> +@property (nullable, nonatomic, readonly, strong) _WKAuthenticationExtensionsClientOutputs* extensions;
> 
> * should be after space

Fixed.

> Source/WebKit/UIProcess/API/Cocoa/_WKWebAuthenticationPanel.h:240
> +- (void)makeCredentialWithHash:(NSData *)hash options:(_WKPublicKeyCredentialCreationOptions *)options completionHandler:(void (^)(_WKAuthenticatorAttestationResponse *, NSError *))handler WK_API_AVAILABLE(macos(WK_MAC_TBA), ios(WK_IOS_TBA));

Note: The hash is the clientDataHash.

>> Source/WebKit/UIProcess/API/Cocoa/_WKWebAuthenticationPanel.h:246
>> +@interface _WKWebAuthenticationPanel (WKDeprecated)
> 
> WK_API_DEPRECATED or WK_API_DEPRECATED_WITH_REPLACEMENT

Is there a way to mark those API as deprecated for a future release such that it won't affect the current client even if they set -Werror,-Wdeprecated-declarations?

>> Source/WebKit/UIProcess/API/Cocoa/_WKWebAuthenticationPanel.mm:45
>> +    if (!(self = [super init]))
> 
> Do you intend to add more here?  You could just return [super init] or omit this entirely.

This is a WIP. If I don't need initialize anything. I will omit this. Thanks!

[Jiewen Tan]

Created attachment 414398 [details]
Patch

[Jiewen Tan]

Created attachment 414494 [details]
Patch

[Jiewen Tan]

Created attachment 414496 [details]
Patch

[Alex Christensen]

Comment on attachment 414496 [details]
Patch

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

> Source/WebKit/ChangeLog:15
> +        is the change of ownerships. Priori to this change, AuthenticatorManager owns the APIWebAuthenticationPanel.

Prior

> Source/WebKit/UIProcess/API/APIWebAuthenticationPanel.cpp:45
> +    : m_manager(WTF::makeUnique<AuthenticatorManager>())

I think WTF:: is unnecessary here.

> Source/WebKit/UIProcess/API/APIWebAuthenticationPanel.h:68
> +    // FIXME<rdar://problem/71509848>: Remove the folowing deprecated methods.

Space and colon before <
// FIXME: <rdar:...
Also, is there something that should be done before these should be removed?  Why don't we just remove them in this patch?

> Source/WebKit/UIProcess/API/APIWebAuthenticationPanel.h:79
> +    std::unique_ptr<WebKit::AuthenticatorManager> m_manager; // FIXME(): Change to UniqueRef.

Why don't we just do this now?

> Source/WebKit/UIProcess/API/Cocoa/_WKWebAuthenticationPanel.h:85
> +    _WKPublicKeyCredentialTypePublicKey,

I don't think this one-value enum is useful unless there are plans to add to it.

> Source/WebKit/UIProcess/API/Cocoa/_WKWebAuthenticationPanel.h:117
> +- (instancetype)init NS_UNAVAILABLE;

new should also be NS_UNAVAILABLE

> Source/WebKit/UIProcess/API/Cocoa/_WKWebAuthenticationPanel.h:154
> +@property (nonatomic, copy) NSNumber *alg;

This should have a better name.
"algorithm"?

> Source/WebKit/UIProcess/API/Cocoa/_WKWebAuthenticationPanel.h:162
> +@property (nonatomic) BOOL requireResidentKey; // Default to NO.

If this comment is necessary, please use header doc style like in our public API headers.

> Source/WebKit/UIProcess/API/Cocoa/_WKWebAuthenticationPanel.h:173
> +@property (nonatomic) _WKPublicKeyCredentialType type;

I feel like a lot of these should be readonly.

> Source/WebKit/UIProcess/API/Cocoa/_WKWebAuthenticationPanel.h:182
> +@property (nullable, nonatomic, copy) NSString *appid;

What is this?

> Source/WebKit/UIProcess/API/Cocoa/_WKWebAuthenticationPanel.h:259
> +// FIXME: <rdar://problem/71509485>

Please add some comment here or remove this.

> Source/WebKit/UIProcess/API/Cocoa/_WKWebAuthenticationPanel.mm:328
> +    result.append((const uint8_t*)[data bytes], [data length]);

Dot syntax?

> Source/WebKit/UIProcess/API/Cocoa/_WKWebAuthenticationPanel.mm:496
> +    return [[_WKAuthenticatorAttestationResponse alloc] initWithRawId:[NSData dataWithBytes:data.rawId->data() length:data.rawId->byteLength()] extensions:nil attestationObject:[NSData dataWithBytes:data.attestationObject->data() length:data.attestationObject->byteLength()]];

This looks like a memory leak.

> Source/WebKit/UIProcess/API/Cocoa/_WKWebAuthenticationPanel.mm:543
> +    return [[_WKAuthenticatorAssertionResponse alloc] initWithRawId:[NSData dataWithBytes:data.rawId->data() length:data.rawId->byteLength()] extensions:nil authenticatorData:[NSData dataWithBytes:data.authenticatorData->data() length:data.authenticatorData->byteLength()] signature:[NSData dataWithBytes:data.signature->data() length:data.signature->byteLength()] userHandle:userHandle];

ditto

> Source/WebKit/UIProcess/API/Cocoa/_WKWebAuthenticationPanelForTesting.h:39
> +// For testing only.

The name of this header makes this comment unnecessary.

> Tools/TestWebKitAPI/Tests/WebKitCocoa/_WKWebAuthenticationPanel.mm:1724
> +    _WKAuthenticationExtensionsClientInputs *extensions = [[_WKAuthenticationExtensionsClientInputs alloc] init];

Let's not leak memory, even in tests.  This doesn't use ARC yet.

[Jiewen Tan]

Comment on attachment 414496 [details]
Patch

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

Thanks Alex for reviewing this patch.

>> Source/WebKit/ChangeLog:15
>> +        is the change of ownerships. Priori to this change, AuthenticatorManager owns the APIWebAuthenticationPanel.
> 
> Prior

Fixed.

>> Source/WebKit/UIProcess/API/APIWebAuthenticationPanel.cpp:45
>> +    : m_manager(WTF::makeUnique<AuthenticatorManager>())
> 
> I think WTF:: is unnecessary here.

Removed.

>> Source/WebKit/UIProcess/API/APIWebAuthenticationPanel.h:68
>> +    // FIXME<rdar://problem/71509848>: Remove the folowing deprecated methods.
> 
> Space and colon before <
> // FIXME: <rdar:...
> Also, is there something that should be done before these should be removed?  Why don't we just remove them in this patch?

Currently, Safari still needs those fields to function. Only after the new implementation is completed, we can safely remove those fields.

>> Source/WebKit/UIProcess/API/APIWebAuthenticationPanel.h:79
>> +    std::unique_ptr<WebKit::AuthenticatorManager> m_manager; // FIXME(): Change to UniqueRef.
> 
> Why don't we just do this now?

There is another create method which doesn't set this field.

>> Source/WebKit/UIProcess/API/Cocoa/_WKWebAuthenticationPanel.h:85
>> +    _WKPublicKeyCredentialTypePublicKey,
> 
> I don't think this one-value enum is useful unless there are plans to add to it.

Yeah, probably not a big deal. Let's remove that.

>> Source/WebKit/UIProcess/API/Cocoa/_WKWebAuthenticationPanel.h:117
>> +- (instancetype)init NS_UNAVAILABLE;
> 
> new should also be NS_UNAVAILABLE

Added.

>> Source/WebKit/UIProcess/API/Cocoa/_WKWebAuthenticationPanel.h:154
>> +@property (nonatomic, copy) NSNumber *alg;
> 
> This should have a better name.
> "algorithm"?

Fixed.

>> Source/WebKit/UIProcess/API/Cocoa/_WKWebAuthenticationPanel.h:162
>> +@property (nonatomic) BOOL requireResidentKey; // Default to NO.
> 
> If this comment is necessary, please use header doc style like in our public API headers.

Fixed.

>> Source/WebKit/UIProcess/API/Cocoa/_WKWebAuthenticationPanel.h:173
>> +@property (nonatomic) _WKPublicKeyCredentialType type;
> 
> I feel like a lot of these should be readonly.

Probably doesn't matter given this is input.

>> Source/WebKit/UIProcess/API/Cocoa/_WKWebAuthenticationPanel.h:182
>> +@property (nullable, nonatomic, copy) NSString *appid;
> 
> What is this?

https://www.w3.org/TR/webauthn/#sctn-appid-extension

>> Source/WebKit/UIProcess/API/Cocoa/_WKWebAuthenticationPanel.h:259
>> +// FIXME: <rdar://problem/71509485>
> 
> Please add some comment here or remove this.

Added.

>> Source/WebKit/UIProcess/API/Cocoa/_WKWebAuthenticationPanel.mm:328
>> +    result.append((const uint8_t*)[data bytes], [data length]);
> 
> Dot syntax?

Fixed.

>> Source/WebKit/UIProcess/API/Cocoa/_WKWebAuthenticationPanel.mm:496
>> +    return [[_WKAuthenticatorAttestationResponse alloc] initWithRawId:[NSData dataWithBytes:data.rawId->data() length:data.rawId->byteLength()] extensions:nil attestationObject:[NSData dataWithBytes:data.attestationObject->data() length:data.attestationObject->byteLength()]];
> 
> This looks like a memory leak.

Fixed.

>> Source/WebKit/UIProcess/API/Cocoa/_WKWebAuthenticationPanel.mm:543
>> +    return [[_WKAuthenticatorAssertionResponse alloc] initWithRawId:[NSData dataWithBytes:data.rawId->data() length:data.rawId->byteLength()] extensions:nil authenticatorData:[NSData dataWithBytes:data.authenticatorData->data() length:data.authenticatorData->byteLength()] signature:[NSData dataWithBytes:data.signature->data() length:data.signature->byteLength()] userHandle:userHandle];
> 
> ditto

Fixed.

>> Source/WebKit/UIProcess/API/Cocoa/_WKWebAuthenticationPanelForTesting.h:39
>> +// For testing only.
> 
> The name of this header makes this comment unnecessary.

Removed.

>> Tools/TestWebKitAPI/Tests/WebKitCocoa/_WKWebAuthenticationPanel.mm:1724
>> +    _WKAuthenticationExtensionsClientInputs *extensions = [[_WKAuthenticationExtensionsClientInputs alloc] init];
> 
> Let's not leak memory, even in tests.  This doesn't use ARC yet.

Fixed.

[Jiewen Tan]

Created attachment 414520 [details]
Patch

[Jiewen Tan]

Comment on attachment 414520 [details]
Patch

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

> Source/WebKit/UIProcess/API/APIWebAuthenticationPanel.h:68
> +    // FIXME: <rdar://problem/71509848> Remove the folowing deprecated methods.

...following...

> Source/WebKit/UIProcess/API/APIWebAuthenticationPanel.h:76
> +    // FIXME: <rdar://problem/71509848> Remove the folowing deprecated method.

...following...

> Source/WebKit/UIProcess/API/APIWebAuthenticationPanel.h:82
> +    // FIXME: <rdar://problem/71509848> Remove the folowing deprecated fields.

...following...

> Source/WebKit/UIProcess/WebAuthentication/WebAuthenticationRequestData.h:49
> +    // FIXME<rdar://problem/71509848>: Remove the folowing deprecated fields.

...following...

[Alex Christensen]

Comment on attachment 414520 [details]
Patch

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

> Source/WebKit/UIProcess/API/APIWebAuthenticationPanel.cpp:46
> +    , m_client(WTF::makeUniqueRef<WebAuthenticationPanelClient>())

WTF:: unnecessary

> Source/WebKit/UIProcess/API/APIWebAuthenticationPanel.cpp:51
> +    : m_client(WTF::makeUniqueRef<WebAuthenticationPanelClient>())

ditto

> Source/WebKit/UIProcess/API/APIWebAuthenticationPanel.h:56
> +    using Respond = Variant<Ref<WebCore::AuthenticatorResponse>, WebCore::ExceptionData>;

Response

> Source/WebKit/UIProcess/API/APIWebAuthenticationPanel.h:69
> +    using TransportSet = HashSet<WebCore::AuthenticatorTransport, WTF::IntHash<WebCore::AuthenticatorTransport>, WTF::StrongEnumHashTraits<WebCore::AuthenticatorTransport>>;

Could this be an OptionSet?  Ditto with all the Vector<WebCore::AuthenticatorTransport>

> Source/WebKit/UIProcess/API/Cocoa/_WKWebAuthenticationPanel.h:199
> +@property (nullable, nonatomic, copy) NSNumber *timeout;

I would say this should be NSTimeInterval but there can be no timeout, hence the nullable.  The value backing this should probably be Optional<Seconds> instead of Optional<unsigned> so that one can specify a fraction of a second.

> Source/WebKit/UIProcess/API/Cocoa/_WKWebAuthenticationPanelForTesting.h:39
> ++ (WebCore::PublicKeyCredentialCreationOptions)convertToCoreCreationOptionsWithOptions:(_WKPublicKeyCredentialCreationOptions *)options WK_API_AVAILABLE(macos(WK_MAC_TBA), ios(WK_IOS_TBA));

I think it would be better if we just exposed the things we want to query in an ObjC API, like exposing the icon instead of a WebCore C++ structure through an ObjC API.

> Tools/TestWebKitAPI/Tests/WebKitCocoa/_WKWebAuthenticationPanel.mm:1449
> +    RetainPtr<_WKPublicKeyCredentialCreationOptions> options = [[_WKPublicKeyCredentialCreationOptions alloc] initWithRp:rp.get() user:user.get() pubKeyCredParams:pubKeyCredParams];

adoptNS or this is a memory leak.  Same everywhere else in this test and other tests where we alloc init... without an autorelease or release.

> Tools/TestWebKitAPI/Tests/WebKitCocoa/_WKWebAuthenticationPanel.mm:1775
> +    RetainPtr<NSData> nsHash = [NSData dataWithBytes:hash length:sizeof(hash)];

This is actually not a memory leak because dataWithBytes:length: returns an autoreleased NSData*

[Jiewen Tan]

Comment on attachment 414520 [details]
Patch

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

>> Source/WebKit/UIProcess/API/APIWebAuthenticationPanel.cpp:46
>> +    , m_client(WTF::makeUniqueRef<WebAuthenticationPanelClient>())
> 
> WTF:: unnecessary

Fixed.

>> Source/WebKit/UIProcess/API/APIWebAuthenticationPanel.cpp:51
>> +    : m_client(WTF::makeUniqueRef<WebAuthenticationPanelClient>())
> 
> ditto

Fixed.

>> Source/WebKit/UIProcess/API/APIWebAuthenticationPanel.h:56
>> +    using Respond = Variant<Ref<WebCore::AuthenticatorResponse>, WebCore::ExceptionData>;
> 
> Response

Fixed.

>> Source/WebKit/UIProcess/API/APIWebAuthenticationPanel.h:69
>> +    using TransportSet = HashSet<WebCore::AuthenticatorTransport, WTF::IntHash<WebCore::AuthenticatorTransport>, WTF::StrongEnumHashTraits<WebCore::AuthenticatorTransport>>;
> 
> Could this be an OptionSet?  Ditto with all the Vector<WebCore::AuthenticatorTransport>

It really doesn't matter at this point given the field will be removed.

>> Source/WebKit/UIProcess/API/Cocoa/_WKWebAuthenticationPanel.h:199
>> +@property (nullable, nonatomic, copy) NSNumber *timeout;
> 
> I would say this should be NSTimeInterval but there can be no timeout, hence the nullable.  The value backing this should probably be Optional<Seconds> instead of Optional<unsigned> so that one can specify a fraction of a second.

That's what is suggested in the Web IDL.

>> Source/WebKit/UIProcess/API/Cocoa/_WKWebAuthenticationPanelForTesting.h:39
>> ++ (WebCore::PublicKeyCredentialCreationOptions)convertToCoreCreationOptionsWithOptions:(_WKPublicKeyCredentialCreationOptions *)options WK_API_AVAILABLE(macos(WK_MAC_TBA), ios(WK_IOS_TBA));
> 
> I think it would be better if we just exposed the things we want to query in an ObjC API, like exposing the icon instead of a WebCore C++ structure through an ObjC API.

The purpose is to test if the objc options are translated well into the C++ options. So, that's why. The underneath functionality once the options are set correctly is well tested with layout tests. So it will be redundant to do a full set of integrated tests to test the correctness of the options. That's why I think unit tests like this will make more sense.

>> Tools/TestWebKitAPI/Tests/WebKitCocoa/_WKWebAuthenticationPanel.mm:1449
>> +    RetainPtr<_WKPublicKeyCredentialCreationOptions> options = [[_WKPublicKeyCredentialCreationOptions alloc] initWithRp:rp.get() user:user.get() pubKeyCredParams:pubKeyCredParams];
> 
> adoptNS or this is a memory leak.  Same everywhere else in this test and other tests where we alloc init... without an autorelease or release.

Fixed.

>> Tools/TestWebKitAPI/Tests/WebKitCocoa/_WKWebAuthenticationPanel.mm:1775
>> +    RetainPtr<NSData> nsHash = [NSData dataWithBytes:hash length:sizeof(hash)];
> 
> This is actually not a memory leak because dataWithBytes:length: returns an autoreleased NSData*

Fixed.

[Jiewen Tan]

Created attachment 414613 [details]
Patch

[Alex Christensen]

Comment on attachment 414613 [details]
Patch

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

> Source/WebKit/UIProcess/API/Cocoa/_WKWebAuthenticationPanel.h:270
> +@interface _WKWebAuthenticationPanel (WKDeprecated)

Instead of this, use WK_API_DEPRECATED or WK_API_DEPRECATED_WITH_REPLACEMENT.

> Source/WebKit/UIProcess/API/Cocoa/_WKWebAuthenticationPanel.mm:51
> +    _appid = appid;

All the other objects in our ObjC API (with a few exceptions that we should fix) are implemented by C++ objects.  This is implemented by copying all the C++ properties into ivars.  We should do the former instead.
Initializers should call API::Object::constructInWrapper
dealloc should call C++ destructor.
You'll need new API object types in APIObject.h and new calls to alloc in APIObject.mm
You'll need an API::ObjectStorage "ivar" with @package.
You'll need to define WrapperTraits for the API object types.
You'll probably need an *Internal.h header.  Also, it's a good pattern to have one class per header instead of cramming them all into one like this is now.

WKNavigationAction is a good example of this pattern.

> Source/WebKit/UIProcess/API/Cocoa/_WKWebAuthenticationPanel.mm:494
> +    _WKAuthenticatorAttestationResponse* result = [[_WKAuthenticatorAttestationResponse alloc] initWithRawId:[NSData dataWithBytes:data.rawId->data() length:data.rawId->byteLength()] extensions:nil attestationObject:[NSData dataWithBytes:data.attestationObject->data() length:data.attestationObject->byteLength()]];

Memory leak.

> Source/WebKit/UIProcess/API/Cocoa/_WKWebAuthenticationPanel.mm:536
> +        extensions = [[_WKAuthenticationExtensionsClientOutputs alloc] initWithAppid:data.appid.value()];

Memory leak.

> Source/WebKit/UIProcess/API/Cocoa/_WKWebAuthenticationPanel.mm:542
> +    _WKAuthenticatorAssertionResponse *result = [[_WKAuthenticatorAssertionResponse alloc] initWithRawId:[NSData dataWithBytes:data.rawId->data() length:data.rawId->byteLength()] extensions:nil authenticatorData:[NSData dataWithBytes:data.authenticatorData->data() length:data.authenticatorData->byteLength()] signature:[NSData dataWithBytes:data.signature->data() length:data.signature->byteLength()] userHandle:userHandle];

Memory leak.

[Jiewen Tan]

Comment on attachment 414613 [details]
Patch

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

>> Source/WebKit/UIProcess/API/Cocoa/_WKWebAuthenticationPanel.h:270
>> +@interface _WKWebAuthenticationPanel (WKDeprecated)
> 
> Instead of this, use WK_API_DEPRECATED or WK_API_DEPRECATED_WITH_REPLACEMENT.

Fixed.

>> Source/WebKit/UIProcess/API/Cocoa/_WKWebAuthenticationPanel.mm:51
>> +    _appid = appid;
> 
> All the other objects in our ObjC API (with a few exceptions that we should fix) are implemented by C++ objects.  This is implemented by copying all the C++ properties into ivars.  We should do the former instead.
> Initializers should call API::Object::constructInWrapper
> dealloc should call C++ destructor.
> You'll need new API object types in APIObject.h and new calls to alloc in APIObject.mm
> You'll need an API::ObjectStorage "ivar" with @package.
> You'll need to define WrapperTraits for the API object types.
> You'll probably need an *Internal.h header.  Also, it's a good pattern to have one class per header instead of cramming them all into one like this is now.
> 
> WKNavigationAction is a good example of this pattern.

I can see that being useful when there is a need to keep the binding between the C++ objects and the objc objects. But for what this API does, it's not necessary to do that. Also, all the fields in the response are needed to be copied and convert into NS types given they are all binaries.

I can do a follow up to separate this file into different ones.

>> Source/WebKit/UIProcess/API/Cocoa/_WKWebAuthenticationPanel.mm:494
>> +    _WKAuthenticatorAttestationResponse* result = [[_WKAuthenticatorAttestationResponse alloc] initWithRawId:[NSData dataWithBytes:data.rawId->data() length:data.rawId->byteLength()] extensions:nil attestationObject:[NSData dataWithBytes:data.attestationObject->data() length:data.attestationObject->byteLength()]];
> 
> Memory leak.

Fixed.

>> Source/WebKit/UIProcess/API/Cocoa/_WKWebAuthenticationPanel.mm:536
>> +        extensions = [[_WKAuthenticationExtensionsClientOutputs alloc] initWithAppid:data.appid.value()];
> 
> Memory leak.

Fixed.

>> Source/WebKit/UIProcess/API/Cocoa/_WKWebAuthenticationPanel.mm:542
>> +    _WKAuthenticatorAssertionResponse *result = [[_WKAuthenticatorAssertionResponse alloc] initWithRawId:[NSData dataWithBytes:data.rawId->data() length:data.rawId->byteLength()] extensions:nil authenticatorData:[NSData dataWithBytes:data.authenticatorData->data() length:data.authenticatorData->byteLength()] signature:[NSData dataWithBytes:data.signature->data() length:data.signature->byteLength()] userHandle:userHandle];
> 
> Memory leak.

Fixed.

[Jiewen Tan]

Created attachment 414619 [details]
Patch

[Darin Adler]

Comment on attachment 414619 [details]
Patch

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

> Source/WebKit/UIProcess/API/Cocoa/_WKWebAuthenticationPanel.mm:495
> +    return result.get();

This is an overrelease. We are returning an object after destroying the RetainPtr keeping it alive.

There are two possible solutions:

1) Change the return type to RetainPtr.
2) Use autorelease() instead of ptr().

I prefer solution (1).

> Source/WebKit/UIProcess/API/Cocoa/_WKWebAuthenticationPanel.mm:543
> +    return result.get();

Same mistake again.

[Darin Adler]

Comment on attachment 414619 [details]
Patch

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

> Source/WebKit/UIProcess/API/Cocoa/_WKWebAuthenticationPanel.mm:495
> +    return result.get();

This is an overrelease. We are returning an object after destroying the RetainPtr keeping it alive.

There are two possible solutions:

1) Change the return type to RetainPtr.
2) Use autorelease() instead of ptr().

I prefer solution (1).

> Source/WebKit/UIProcess/API/Cocoa/_WKWebAuthenticationPanel.mm:543
> +    return result.get();

Same mistake again.

[Jiewen Tan]

Comment on attachment 414619 [details]
Patch

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

Thanks Darin for reviewing this patch.

>>> Source/WebKit/UIProcess/API/Cocoa/_WKWebAuthenticationPanel.mm:495
>>> +    return result.get();
>> 
>> This is an overrelease. We are returning an object after destroying the RetainPtr keeping it alive.
>> 
>> There are two possible solutions:
>> 
>> 1) Change the return type to RetainPtr.
>> 2) Use autorelease() instead of ptr().
>> 
>> I prefer solution (1).
> 
> This is an overrelease. We are returning an object after destroying the RetainPtr keeping it alive.
> 
> There are two possible solutions:
> 
> 1) Change the return type to RetainPtr.
> 2) Use autorelease() instead of ptr().
> 
> I prefer solution (1).

Fixed.

>>> Source/WebKit/UIProcess/API/Cocoa/_WKWebAuthenticationPanel.mm:543
>>> +    return result.get();
>> 
>> Same mistake again.
> 
> Same mistake again.

Fixed.

[Jiewen Tan]

Created attachment 414632 [details]
Patch

[Darin Adler]

Comment on attachment 414632 [details]
Patch

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

RetainPtr idioms look good. (This is not a review of the rest of the patch.)

> Source/WebKit/UIProcess/API/Cocoa/_WKWebAuthenticationPanel.mm:533
> +    RetainPtr<_WKAuthenticationExtensionsClientOutputs> extensions = nil;

No need for "= nil" with a RetainPtr; they get initialized to nil no matter what.

[Jiewen Tan]

Comment on attachment 414632 [details]
Patch

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

>> Source/WebKit/UIProcess/API/Cocoa/_WKWebAuthenticationPanel.mm:533
>> +    RetainPtr<_WKAuthenticationExtensionsClientOutputs> extensions = nil;
> 
> No need for "= nil" with a RetainPtr; they get initialized to nil no matter what.

Fixed.

[Jiewen Tan]

Created attachment 414639 [details]
Patch

[Jiewen Tan]

Created attachment 414640 [details]
Patch

[Alex Christensen]

Comment on attachment 414640 [details]
Patch

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

> Source/WebKit/UIProcess/API/Cocoa/_WKWebAuthenticationPanel.h:31
> +// FIXME: <rdar://problem/71509848> Separate the following different interfaces into separate files.

I'm still not crazy about having a bunch of little objects that don't have their own header land in the repository.  Especially little objects that use ObjC-generated ivars unlike the rest of the API objects in WebKit.

> Source/WebKit/UIProcess/API/Cocoa/_WKWebAuthenticationPanel.h:192
> +- (instancetype)init NS_UNAVAILABLE;

new should also be NS_UNAVAILABLE here.

> Source/WebKit/UIProcess/API/Cocoa/_WKWebAuthenticationPanel.h:270
> +// FIXME: <rdar://problem/71509848> Remove the following deprecated method.
> +@property (nonatomic, readonly, copy) NSString *relyingPartyID;

I think these should be annotated as deprecated with this change.

> Source/WebKit/UIProcess/API/Cocoa/_WKWebAuthenticationPanel.mm:337
> +static void encodeEntity(WebCore::PublicKeyCredentialCreationOptions::Entity& output, _WKPublicKeyCredentialEntity* input)

This looks a lot like returning by reference, but it's not.  I think it would be better to just have these two lines of code duplicated for the two different types of entities here.

> Source/WebKit/UIProcess/API/Cocoa/_WKWebAuthenticationPanel.mm:447
> +    return result;

Would it be possible to return { attachment, key, verification }; instead of constructing an object, filling it in, and then returning it? Making this change would cause a compiler failure if we add something in the future and forget to update, and it also doesn't require default constructor to be called.

> Source/WebKit/UIProcess/API/Cocoa/_WKWebAuthenticationPanel.mm:512
> +        }, [&](const WebCore::ExceptionData& exception) {
> +            handler(nil, [NSError errorWithDomain:WKErrorDomain code:WKErrorUnknown userInfo:nil]);

Can we use the exception data instead of just reporting an unknown error?

[Jiewen Tan]

Comment on attachment 414640 [details]
Patch

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

>> Source/WebKit/UIProcess/API/Cocoa/_WKWebAuthenticationPanel.h:31
>> +// FIXME: <rdar://problem/71509848> Separate the following different interfaces into separate files.
> 
> I'm still not crazy about having a bunch of little objects that don't have their own header land in the repository.  Especially little objects that use ObjC-generated ivars unlike the rest of the API objects in WebKit.

Alright, I have separated those interfaces into separate files. However, I'm not going to introduce APIObject for those interfaces given they are not practically useful here.

>> Source/WebKit/UIProcess/API/Cocoa/_WKWebAuthenticationPanel.h:192
>> +- (instancetype)init NS_UNAVAILABLE;
> 
> new should also be NS_UNAVAILABLE here.

Fixed.

>> Source/WebKit/UIProcess/API/Cocoa/_WKWebAuthenticationPanel.h:270
>> +@property (nonatomic, readonly, copy) NSString *relyingPartyID;
> 
> I think these should be annotated as deprecated with this change.

I don't want to break Safari now. Let's do that later.

>> Source/WebKit/UIProcess/API/Cocoa/_WKWebAuthenticationPanel.mm:337
>> +static void encodeEntity(WebCore::PublicKeyCredentialCreationOptions::Entity& output, _WKPublicKeyCredentialEntity* input)
> 
> This looks a lot like returning by reference, but it's not.  I think it would be better to just have these two lines of code duplicated for the two different types of entities here.

Fixed.

>> Source/WebKit/UIProcess/API/Cocoa/_WKWebAuthenticationPanel.mm:447
>> +    return result;
> 
> Would it be possible to return { attachment, key, verification }; instead of constructing an object, filling it in, and then returning it? Making this change would cause a compiler failure if we add something in the future and forget to update, and it also doesn't require default constructor to be called.

I think writing it in this way is more clear than having a helper that just has one line of return code.

>> Source/WebKit/UIProcess/API/Cocoa/_WKWebAuthenticationPanel.mm:512
>> +            handler(nil, [NSError errorWithDomain:WKErrorDomain code:WKErrorUnknown userInfo:nil]);
> 
> Can we use the exception data instead of just reporting an unknown error?

This patch is already pretty big. I would like to address that in <rdar://problem/71509485> given I might need to introduce some objc enum to do the translation.

[Jiewen Tan]

Created attachment 414661 [details]
Patch

[Alex Christensen]

Comment on attachment 414661 [details]
Patch

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

Getting closer.

> Source/WebKit/UIProcess/API/Cocoa/_WKAuthenticatorSelectionCriteria.h:46
> +/*!@discussion The default value is NO.*/

I think an empty line before this would be good.

> Source/WebKit/UIProcess/API/Cocoa/_WKAuthenticatorSelectionCriteria.h:48
> +/*!@discussion The default value is _WKUserVerificationRequirementPreferred.*/

ditto

> Source/WebKit/UIProcess/API/Cocoa/_WKPublicKeyCredentialCreationOptions.h:52
> +- (instancetype)initWithRp:(_WKPublicKeyCredentialRpEntity *)rp user:(_WKPublicKeyCredentialUserEntity *)user pubKeyCredParams:(NSArray<_WKPublicKeyCredentialParameters *> *)pubKeyCredParams;

pubKeyCredParams -> publicKeyCredentialParamaters
What is Rp?  I think that should be spelled out also.  Here and everywhere.

> Source/WebKit/UIProcess/API/Cocoa/_WKPublicKeyCredentialCreationOptions.h:62
> +/*!@discussion The default value is _WKAttestationConveyancePreferenceNone.*/

empty line before

> Source/WebKit/UIProcess/API/Cocoa/_WKPublicKeyCredentialRequestOptions.h:42
> +@property (nullable, nonatomic, copy) NSString *rpId;

Whatever rp is, then identifier.

[Jiewen Tan]

Comment on attachment 414661 [details]
Patch

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

>> Source/WebKit/UIProcess/API/Cocoa/_WKAuthenticatorSelectionCriteria.h:46
>> +/*!@discussion The default value is NO.*/
> 
> I think an empty line before this would be good.

Fixed.

>> Source/WebKit/UIProcess/API/Cocoa/_WKAuthenticatorSelectionCriteria.h:48
>> +/*!@discussion The default value is _WKUserVerificationRequirementPreferred.*/
> 
> ditto

Fixed.

>> Source/WebKit/UIProcess/API/Cocoa/_WKPublicKeyCredentialCreationOptions.h:52
>> +- (instancetype)initWithRp:(_WKPublicKeyCredentialRpEntity *)rp user:(_WKPublicKeyCredentialUserEntity *)user pubKeyCredParams:(NSArray<_WKPublicKeyCredentialParameters *> *)pubKeyCredParams;
> 
> pubKeyCredParams -> publicKeyCredentialParamaters
> What is Rp?  I think that should be spelled out also.  Here and everywhere.

I'm following the naming conversions from the spec. https://www.w3.org/TR/webauthn/#dictdef-publickeycredentialcreationoptions.

>> Source/WebKit/UIProcess/API/Cocoa/_WKPublicKeyCredentialCreationOptions.h:62
>> +/*!@discussion The default value is _WKAttestationConveyancePreferenceNone.*/
> 
> empty line before

Fixed.

>> Source/WebKit/UIProcess/API/Cocoa/_WKPublicKeyCredentialRequestOptions.h:42
>> +@property (nullable, nonatomic, copy) NSString *rpId;
> 
> Whatever rp is, then identifier.

Again, this is the naming conversion from the spec: https://www.w3.org/TR/webauthn/#dictdef-publickeycredentialrequestoptions.

[Jiewen Tan]

Created attachment 414703 [details]
Patch

[Alex Christensen]

Comment on attachment 414703 [details]
Patch

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

> Source/WebKit/UIProcess/API/Cocoa/_WKPublicKeyCredentialRequestOptions.h:42
> +@property (nullable, nonatomic, copy) NSString *rpId;

I still think this would be better as relyingPartyIdentifier, which is also mentioned in the spec in https://www.w3.org/TR/webauthn/#relying-party-identifier
Crypto has too many terms of art that are two or three letter abbreviations.

[Jiewen Tan]

Comment on attachment 414703 [details]
Patch

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

Thanks Alex for r+ this patch.

>> Source/WebKit/UIProcess/API/Cocoa/_WKPublicKeyCredentialRequestOptions.h:42
>> +@property (nullable, nonatomic, copy) NSString *rpId;
> 
> I still think this would be better as relyingPartyIdentifier, which is also mentioned in the spec in https://www.w3.org/TR/webauthn/#relying-party-identifier
> Crypto has too many terms of art that are two or three letter abbreviations.

Let me fix that then.

[Jiewen Tan]

Created attachment 414757 [details]
Patch for landing

[EWS]

Committed r270142: <https://trac.webkit.org/changeset/270142>

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