Bug 217319 - Disable media capture if an app's entitlements won't allow access to capture devices
Summary: Disable media capture if an app's entitlements won't allow access to capture ...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebRTC (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Eric Carlson
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2020-10-05 08:56 PDT by Eric Carlson
Modified: 2020-10-05 18:21 PDT (History)
8 users (show)

See Also:


Attachments
Patch (29.97 KB, patch)
2020-10-05 09:37 PDT, Eric Carlson
no flags Details | Formatted Diff | Diff
Patch for landing (9.71 KB, patch)
2020-10-05 14:35 PDT, Eric Carlson
ews-feeder: commit-queue-
Details | Formatted Diff | Diff
Patch for landing (10.51 KB, patch)
2020-10-05 14:50 PDT, Eric Carlson
no flags Details | Formatted Diff | Diff
Fix usage strings (2.07 KB, patch)
2020-10-05 17:08 PDT, Eric Carlson
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Eric Carlson 2020-10-05 08:56:52 PDT
Disable media capture if an app's entitlements won't allow access to capture devices
Comment 1 Radar WebKit Bug Importer 2020-10-05 08:57:09 PDT
<rdar://problem/69956112>
Comment 2 Eric Carlson 2020-10-05 09:37:53 PDT
Created attachment 410528 [details]
Patch
Comment 3 youenn fablet 2020-10-05 10:45:48 PDT
Comment on attachment 410528 [details]
Patch

I think we can keep the patch much smaller by keeping _mediaCaptureEnabled.
WKWebViewConfiguration._mediaCaptureEnabled is better than WKPReferences._mediaDevicesEnabled as we want a page to have a consistent exposure of getUserMedia throughout its lifetime.
Also, not removing it will make the patch much smaller :)

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

> Source/WebCore/page/SettingsBase.cpp:343
> +        m_page->settings().setMediaDevicesEnabled(true);

mock capture devices can be changed during the lifetime of the page.
MediaDevicesEnabled should be set once and for all for the page so, it might be better to have two toggles for now.
If we really want to have just one toggle, we should compute it in UIProcess.

> Source/WebKit/UIProcess/API/Cocoa/WKWebViewConfigurationPrivate.h:-123
> -@property (nonatomic, setter=_setMediaCaptureEnabled:) BOOL _mediaCaptureEnabled WK_API_AVAILABLE(macos(WK_MAC_TBA), ios(WK_IOS_TBA));

Why removing this one?
This does not seem necessary for this patch and we should instead be removing _mediaDevicesEnabled from being exposed in WebPreferences.
Comment 4 Eric Carlson 2020-10-05 14:35:25 PDT
Created attachment 410561 [details]
Patch for landing
Comment 5 Eric Carlson 2020-10-05 14:50:59 PDT
Created attachment 410565 [details]
Patch for landing
Comment 6 Eric Carlson 2020-10-05 15:57:21 PDT
Comment on attachment 410565 [details]
Patch for landing

The Windows test failures are unrelated
Comment 7 EWS 2020-10-05 16:15:23 PDT
Committed r268012: <https://trac.webkit.org/changeset/268012>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 410565 [details].
Comment 8 Darin Adler 2020-10-05 16:56:40 PDT
Comment on attachment 410565 [details]
Patch for landing

Not a big fan of the word "random" in those usage messages, but I guess it’s just MiniBrowser. I would have probably written "Recording videos on behalf of websites"
Comment 9 Eric Carlson 2020-10-05 16:59:48 PDT
(In reply to Darin Adler from comment #8)
> Comment on attachment 410565 [details]
> Patch for landing
> 
> Not a big fan of the word "random" in those usage messages, but I guess it’s
> just MiniBrowser. I would have probably written "Recording videos on behalf
> of websites"

Good point, I'll fix that in a followup.
Comment 10 Eric Carlson 2020-10-05 17:08:36 PDT
Reopening to attach new patch.
Comment 11 Eric Carlson 2020-10-05 17:08:37 PDT
Created attachment 410594 [details]
Fix usage strings
Comment 12 EWS 2020-10-05 18:21:15 PDT
Committed r268023: <https://trac.webkit.org/changeset/268023>

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