Bug 219709

Summary: [WebAuthn] Adopt new UI for the Platform Authenticator makeCredential flow
Product: WebKit Reporter: Jiewen Tan <jiewen_tan>
Component: WebKit Misc.Assignee: Jiewen Tan <jiewen_tan>
Status: RESOLVED FIXED    
Severity: Normal CC: bfulgham, jiewen_tan, webkit-bug-importer
Priority: P2 Keywords: InRadar
Version: WebKit Nightly Build   
Hardware: Unspecified   
OS: Unspecified   
Bug Depends on:    
Bug Blocks: 181943    
Attachments:
Description Flags
Patch
ews-feeder: commit-queue-
Patch bfulgham: review+

Description Jiewen Tan 2020-12-09 14:57:13 PST
Adopt new UI for the Platform Authenticator makeCredential flow.
Comment 1 Radar WebKit Bug Importer 2020-12-09 14:57:49 PST
<rdar://problem/72154774>
Comment 2 Jiewen Tan 2020-12-10 21:49:49 PST
Created attachment 415966 [details]
Patch
Comment 3 Jiewen Tan 2020-12-10 22:26:33 PST
Created attachment 415970 [details]
Patch
Comment 4 Brent Fulgham 2020-12-11 11:23:02 PST
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.
Comment 5 Jiewen Tan 2020-12-11 11:29:45 PST
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.
Comment 6 Brent Fulgham 2020-12-11 11:41:25 PST
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.
Comment 7 Jiewen Tan 2020-12-11 11:43:42 PST
Comment on attachment 415970 [details]
Patch

Thanks Brent for r+ this patch.
Comment 8 Jiewen Tan 2020-12-11 11:47:14 PST
Committed r270694: <https://trac.webkit.org/changeset/270694>
Comment 9 Jiewen Tan 2020-12-11 17:07:01 PST
A build fix:
Committed r270721: <https://trac.webkit.org/changeset/270721>