Bug 218870

Summary: [macOS] Perform AX TCC check in the UI process
Product: WebKit Reporter: Per Arne Vollan <pvollan>
Component: WebKit Misc.Assignee: Per Arne Vollan <pvollan>
Status: RESOLVED FIXED    
Severity: Normal CC: bfulgham, webkit-bug-importer
Priority: P2 Keywords: InRadar
Version: WebKit Nightly Build   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
Patch
none
Patch
none
Patch
bfulgham: review+
Patch
ews-feeder: commit-queue-
Patch none

Description Per Arne Vollan 2020-11-12 13:04:13 PST
On behalf of the WebContent process, perform AX TCC check in the Networking process on macOS. This is in preparation of blocking tccd in the WebContent process.
Comment 1 Radar WebKit Bug Importer 2020-11-12 13:04:42 PST
<rdar://problem/71339830>
Comment 2 Per Arne Vollan 2020-11-12 13:48:22 PST
Created attachment 413973 [details]
Patch
Comment 3 Per Arne Vollan 2020-11-12 14:06:16 PST
Created attachment 413974 [details]
Patch
Comment 4 Alexey Proskuryakov 2020-11-12 18:47:08 PST
Comment on attachment 413974 [details]
Patch

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

> Source/WebCore/PAL/ChangeLog:3
> +        [macOS] Perform AX TCC check in the Networking process

I know that Networking process does more than networking, but making it responsible for accessibility in any way goes beyond reasonable in my opinion.
Comment 5 Per Arne Vollan 2020-11-13 08:17:22 PST
Created attachment 414045 [details]
Patch
Comment 6 Per Arne Vollan 2020-11-13 08:21:03 PST
(In reply to Alexey Proskuryakov from comment #4)
> Comment on attachment 413974 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=413974&action=review
> 
> > Source/WebCore/PAL/ChangeLog:3
> > +        [macOS] Perform AX TCC check in the Networking process
> 
> I know that Networking process does more than networking, but making it
> responsible for accessibility in any way goes beyond reasonable in my
> opinion.

Yes, that is a very good point. I have moved this to the UI process in the latest patch.

Thanks for reviewing!
Comment 7 Brent Fulgham 2020-11-16 13:04:31 PST
Comment on attachment 414045 [details]
Patch

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

> Source/WebCore/PAL/pal/spi/mac/HIServicesSPI.h:161
> +typedef Boolean (*AXAuditTokenIsAuthenticatedCallback)(audit_token_t auditToken);

We might not need the 'auditToken' name for the argument here.

> Source/WebKit/WebProcess/cocoa/WebProcessCocoa.mm:184
> +static Boolean isAXAuthenticatedCallback(audit_token_t auditToken)

Do we know how frequently this is called? Is it just once at process launch, or does it happen frequently in UIKit/AppKit code?

Would it be better to perform the message send asynchronously as part of the launch process, and store the result so that this callback is very quick?
Comment 8 Per Arne Vollan 2020-11-16 13:24:18 PST
(In reply to Brent Fulgham from comment #7)
> Comment on attachment 414045 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=414045&action=review
> 
> > Source/WebCore/PAL/pal/spi/mac/HIServicesSPI.h:161
> > +typedef Boolean (*AXAuditTokenIsAuthenticatedCallback)(audit_token_t auditToken);
> 
> We might not need the 'auditToken' name for the argument here.
> 

Will fix!

> > Source/WebKit/WebProcess/cocoa/WebProcessCocoa.mm:184
> > +static Boolean isAXAuthenticatedCallback(audit_token_t auditToken)
> 
> Do we know how frequently this is called? Is it just once at process launch,
> or does it happen frequently in UIKit/AppKit code?
> 

This will only be called when an Accessibility client (e.g. VoiceOver) makes the first request to the WebContent process, so it is not called frequently.

> Would it be better to perform the message send asynchronously as part of the
> launch process, and store the result so that this callback is very quick?

That's a good point, although in this case I don't believe we can cache the result, since the result of this function call depends on the provided audit token, which is the audit token of the client making the accessibility request.

Thanks for reviewing!
Comment 9 Brent Fulgham 2020-11-16 13:38:28 PST
Comment on attachment 414045 [details]
Patch

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

r=me. Please monitor perf bots (as you always do!) :-)

> Source/WebCore/PAL/pal/spi/mac/HIServicesSPI.h:161
> +typedef Boolean (*AXAuditTokenIsAuthenticatedCallback)(audit_token_t auditToken);

We might not need the 'auditToken' name for the argument here.
Comment 10 Per Arne Vollan 2020-11-17 07:37:41 PST
Created attachment 414338 [details]
Patch
Comment 11 Per Arne Vollan 2020-11-17 07:38:45 PST
(In reply to Brent Fulgham from comment #9)
> Comment on attachment 414045 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=414045&action=review
> 
> r=me. Please monitor perf bots (as you always do!) :-)
> 

Will do!

> > Source/WebCore/PAL/pal/spi/mac/HIServicesSPI.h:161
> > +typedef Boolean (*AXAuditTokenIsAuthenticatedCallback)(audit_token_t auditToken);
> 
> We might not need the 'auditToken' name for the argument here.

Fixed in latest patch.

Thanks for reviewing!
Comment 12 EWS 2020-11-17 08:26:14 PST
Patch 414338 does not build
Comment 13 Per Arne Vollan 2020-11-17 08:43:00 PST
Created attachment 414344 [details]
Patch
Comment 14 EWS 2020-11-17 09:22:11 PST
Committed r269906: <https://trac.webkit.org/changeset/269906>

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