Bug 219709 – [WebAuthn] Adopt new UI for the Platform Authenticator makeCredential flow

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

[Jiewen Tan]

Adopt new UI for the Platform Authenticator makeCredential flow.

[Radar WebKit Bug Importer]

<rdar://problem/72154774>

[Jiewen Tan]

Created attachment 415966 [details]
Patch

[Jiewen Tan]

Created attachment 415970 [details]
Patch

[Brent Fulgham]

Comment on attachment 415970 [details]
Patch

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

> Source/WebKit/UIProcess/WebAuthentication/Cocoa/LocalAuthenticator.mm:237
> +    // This is for the new UI.

I don't think you need this comment since the next line is conditional on the new UI.

> Source/WebKit/UIProcess/WebAuthentication/Cocoa/LocalAuthenticator.mm:315
> +    SecAccessControlRef accessControlRef = accessControl.get();

Does accessControlRef need to be retained in some fashion? Will calling 'verifyUser' with this be safe?

> Source/WebKit/UIProcess/WebAuthentication/Cocoa/LocalConnection.mm:119
> +    [context evaluateAccessControl:accessControl operation:LAAccessControlOperationUseKeySign options:options.get() reply:reply.get()];

This method call seems to use 'accessControl', which is not guaranteed to still live since you moved the underlying object in the calling method.

[Jiewen Tan]

Comment on attachment 415970 [details]
Patch

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

Thanks Brent for reviewing this patch.

>> Source/WebKit/UIProcess/WebAuthentication/Cocoa/LocalAuthenticator.mm:237
>> +    // This is for the new UI.
> 
> I don't think you need this comment since the next line is conditional on the new UI.

Removed.

>> Source/WebKit/UIProcess/WebAuthentication/Cocoa/LocalAuthenticator.mm:315
>> +    SecAccessControlRef accessControlRef = accessControl.get();
> 
> Does accessControlRef need to be retained in some fashion? Will calling 'verifyUser' with this be safe?

Yes, it is retained in the callback.

>> Source/WebKit/UIProcess/WebAuthentication/Cocoa/LocalConnection.mm:119
>> +    [context evaluateAccessControl:accessControl operation:LAAccessControlOperationUseKeySign options:options.get() reply:reply.get()];
> 
> This method call seems to use 'accessControl', which is not guaranteed to still live since you moved the underlying object in the calling method.

accessControl is held by the completionHandler, which is the held by reply. As long as the callee doesn't destruct the reply before verifying the accessControl, it should be safe.

[Brent Fulgham]

Comment on attachment 415970 [details]
Patch

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

r=me

>>> Source/WebKit/UIProcess/WebAuthentication/Cocoa/LocalAuthenticator.mm:315
>>> +    SecAccessControlRef accessControlRef = accessControl.get();
>> 
>> Does accessControlRef need to be retained in some fashion? Will calling 'verifyUser' with this be safe?
> 
> Yes, it is retained in the callback.

Ah! Gotcha, My mistake!

>>> Source/WebKit/UIProcess/WebAuthentication/Cocoa/LocalConnection.mm:119
>>> +    [context evaluateAccessControl:accessControl operation:LAAccessControlOperationUseKeySign options:options.get() reply:reply.get()];
>> 
>> This method call seems to use 'accessControl', which is not guaranteed to still live since you moved the underlying object in the calling method.
> 
> accessControl is held by the completionHandler, which is the held by reply. As long as the callee doesn't destruct the reply before verifying the accessControl, it should be safe.

Got it.

[Jiewen Tan]

Comment on attachment 415970 [details]
Patch

Thanks Brent for r+ this patch.

[Jiewen Tan]

Committed r270694: <https://trac.webkit.org/changeset/270694>

[Jiewen Tan]

A build fix:
Committed r270721: <https://trac.webkit.org/changeset/270721>