Public Sessions - prompt the user for audioCapture/videoCapture requests

chrome/browser/media/extension_media_access_handler.cc

Index: chrome/browser/media/extension_media_access_handler.cc
diff --git a/chrome/browser/media/extension_media_access_handler.cc b/chrome/browser/media/extension_media_access_handler.cc
index 681fe900f28a8ec754e681bfbdbf23e1aec5717c..59d6c81aa860072a13daece17d9b3cb05e3a19cc 100644
--- a/chrome/browser/media/extension_media_access_handler.cc
+++ b/chrome/browser/media/extension_media_access_handler.cc
@@ -11,9 +11,12 @@
 #include "chrome/browser/media/webrtc/media_stream_device_permissions.h"
 #include "chrome/browser/profiles/profile.h"
 #include "chrome/common/pref_names.h"
+#include "chromeos/login/login_state.h"
 #include "content/public/browser/web_contents.h"
 #include "extensions/common/extension.h"
+#include "extensions/common/permissions/manifest_permission_set.h"
 #include "extensions/common/permissions/permissions_data.h"
+#include "extensions/common/url_pattern_set.h"
 
 namespace {
 
@@ -38,6 +41,16 @@ bool IsMediaRequestWhitelistedForExtension(
          extension->id() == "gjaehgfemfahhmlgpdfknkhdnemmolop";
 }
 
+// Returns true if we're in a Public Session.
+bool IsPublicSession() {
+#if defined(OS_CHROMEOS)
+  if (chromeos::LoginState::IsInitialized()) {
+    return chromeos::LoginState::Get()->IsPublicSessionUser();
+  }
+#endif
+  return false;
+}
+
 }  // namespace
 
 ExtensionMediaAccessHandler::ExtensionMediaAccessHandler() {
@@ -66,6 +79,63 @@ bool ExtensionMediaAccessHandler::CheckMediaAccessPermission(
           : extensions::APIPermission::kVideoCapture);
 }
 
+bool ExtensionMediaAccessHandler::UserChoice::Disallowed(

[Sergey Ulanov, 2016/11/29 19:12:06]

IsAllowed()?
That would make code that uses this function cleaner.

[Ivan Šandrk, 2016/11/29 23:33:46]

I believe there's a subtle semantic difference between IsAllowed and
!Disallowed, but I may be wrong. It seems to me that the first implies that we
need explicit permission to do it, while the second implies that it just mustn't
be explicitly forbidden (the third option here is status quo outside of Public
Sessions where it's neither explicitly allowed or forbidden in which case we
allow it).

What do you think?

[Ivan Šandrk, 2016/11/30 17:00:50]

Decided to go with the IsAllowed in the end.

+    content::MediaStreamType type) const {
+  if (IsPublicSession()) {
+    CHECK(type == content::MEDIA_DEVICE_AUDIO_CAPTURE ||

[Sergey Ulanov, 2016/11/29 19:12:06]

Why is this CHECK instead of DCHECK?
https://chromium.googlesource.com/chromium/src/+/master/styleguide/c++/c++.md...
Also it would be cleaner to put a check for type ==
content::MEDIA_DEVICE_VIDEO_CAPTURE in the else case below.

[Ivan Šandrk, 2016/11/29 23:33:46]

Good point, done. And changed the other CHECK as well.

+          type == content::MEDIA_DEVICE_VIDEO_CAPTURE);
+    if (type == content::MEDIA_DEVICE_AUDIO_CAPTURE) {
+      return audio_prompted_ && !audio_allowed_;
+    } else {
+      return video_prompted_ && !video_allowed_;
+    }
+  }
+  return false;
+}
+
+bool ExtensionMediaAccessHandler::UserChoice::NeedsPrompting(
+    content::MediaStreamType type) const {
+  if (type == content::MEDIA_DEVICE_AUDIO_CAPTURE) {
+    return !audio_prompted_;
+  }
+  if (type == content::MEDIA_DEVICE_VIDEO_CAPTURE) {
+    return !video_prompted_;
+  }
+  return false;
+}
+
+void ExtensionMediaAccessHandler::UserChoice::Set(
+    content::MediaStreamType type, bool allowed) {
+  CHECK(type == content::MEDIA_DEVICE_AUDIO_CAPTURE ||
+        type == content::MEDIA_DEVICE_VIDEO_CAPTURE);
+  if (type == content::MEDIA_DEVICE_AUDIO_CAPTURE) {
+    DCHECK(!audio_prompted_);
+    audio_prompted_ = true;
+    audio_allowed_ = allowed;
+  } else {
+    DCHECK(!video_prompted_);
+    video_prompted_ = true;
+    video_allowed_ = allowed;
+  }
+}
+
+void ExtensionMediaAccessHandler::ResolvePermissionPrompt(
+    content::WebContents* web_contents,
+    const content::MediaStreamRequest& request,
+    const content::MediaResponseCallback& callback,
+    const extensions::Extension* extension,
+    ExtensionInstallPrompt::Result prompt_result) {
+  bool allowed = (prompt_result == ExtensionInstallPrompt::Result::ACCEPTED);
+  UserChoice& user_choice = user_choice_cache_[extension->id()];
+
+  if (user_choice.NeedsPrompting(request.audio_type))
+    user_choice.Set(content::MEDIA_DEVICE_AUDIO_CAPTURE, allowed);
+  if (user_choice.NeedsPrompting(request.video_type))
+    user_choice.Set(content::MEDIA_DEVICE_VIDEO_CAPTURE, allowed);
+
+  HandleRequestContinuation(web_contents, request, callback, extension);
+}
+
 void ExtensionMediaAccessHandler::HandleRequest(
     content::WebContents* web_contents,
     const content::MediaStreamRequest& request,
@@ -75,21 +145,72 @@ void ExtensionMediaAccessHandler::HandleRequest(
   // MediaStreamDevicesController::Accept(). Move this code into a shared method
   // between the two classes.
 
+  // In Public Sessions extensions are installed by admin policy hence they can
+  // freely use the camera and microphone without the user knowing. This is not
+  // acceptable from a security/privacy standpoint so we show the user a dialog
+  // where he can choose whether to allow the extension access to camera and/or
+  // microphone.
+  if (IsPublicSession()) {

[Sergey Ulanov, 2016/11/29 19:12:05]

Did you consider adding a separate MediaAccessHandler for public sessions? It
would allow to avoid mixing all the new logic that's applicable to public
session with everything else.
Potentially it could be implemented as a wrapper for ExtensionMediaAccessHandler
or as a separate MediaAccessHandler that's used before
ExtensionMediaAccessHandler (access handlers priority is determined by their
order here:
https://codesearch.chromium.org/chromium/src/chrome/browser/media/webrtc/medi...
)

[Ivan Šandrk, 2016/11/29 23:33:46]

This is a good idea :) I will investigate it more tomorrow.

[Ivan Šandrk, 2016/11/30 17:00:50]

Implemented a separate MediaAccessHandler :)

+    bool needs_prompt = false;
+    extensions::APIPermissionSet new_apis;
+    const UserChoice& user_choice = user_choice_cache_[extension->id()];
+
+    if (user_choice.NeedsPrompting(request.audio_type)) {
+      new_apis.insert(extensions::APIPermission::kAudioCapture);
+      needs_prompt = true;
+    }
+    if (user_choice.NeedsPrompting(request.video_type)) {
+      new_apis.insert(extensions::APIPermission::kVideoCapture);
+      needs_prompt = true;
+    }
+
+    if (needs_prompt) {
+      std::unique_ptr<const extensions::PermissionSet> permission_set(
+          new extensions::PermissionSet(
+              new_apis, extensions::ManifestPermissionSet(),
+              extensions::URLPatternSet(), extensions::URLPatternSet()));
+
+      prompt_.reset(new ExtensionInstallPrompt(web_contents));

[Sergey Ulanov, 2016/11/29 19:12:06]

There may be multiple requests that will need to be handled in parallel, so I
think you need to queue requests here instead of replacing the old |prompt_|.

[Sergey Ulanov, 2016/11/29 19:12:06]

Does ExtensionInstallPrompt() work when the web_contents is a background page or
a platform app window?

[Ivan Šandrk, 2016/11/29 23:33:46]

I believe it should always work when there's web_contents available. I've tested
this functionality with a couple of apps and it works in each case.

[Ivan Šandrk, 2016/11/29 23:33:46]

Now I've prompted to save the ExtensionInstallPrompt inside the callback itself,
I think it should be fine. Do you think there's something inherently wrong with
this approach?

+      prompt_->ShowDialog(
+          base::Bind(&ExtensionMediaAccessHandler::ResolvePermissionPrompt,
+              base::Unretained(this), web_contents, request, callback,
+              extension),
+          extension,
+          nullptr,
+          base::MakeUnique<ExtensionInstallPrompt::Prompt>(
+              ExtensionInstallPrompt::PERMISSIONS_PROMPT),
+          std::move(permission_set),
+          ExtensionInstallPrompt::GetDefaultShowDialogCallback());
+      return;
+    }
+  }
+
+  HandleRequestContinuation(web_contents, request, callback, extension);
+}
+
+void ExtensionMediaAccessHandler::HandleRequestContinuation(
+    content::WebContents* web_contents,
+    const content::MediaStreamRequest& request,
+    const content::MediaResponseCallback& callback,
+    const extensions::Extension* extension) {
   Profile* profile =
       Profile::FromBrowserContext(web_contents->GetBrowserContext());
+  const UserChoice& user_choice = user_choice_cache_[extension->id()];
 
   bool audio_allowed =
       request.audio_type == content::MEDIA_DEVICE_AUDIO_CAPTURE &&
       extension->permissions_data()->HasAPIPermission(
           extensions::APIPermission::kAudioCapture) &&
       GetDevicePolicy(profile, extension->url(), prefs::kAudioCaptureAllowed,
-                      prefs::kAudioCaptureAllowedUrls) != ALWAYS_DENY;
+                      prefs::kAudioCaptureAllowedUrls) != ALWAYS_DENY &&
+      !user_choice.Disallowed(content::MEDIA_DEVICE_AUDIO_CAPTURE);
   bool video_allowed =
       request.video_type == content::MEDIA_DEVICE_VIDEO_CAPTURE &&
       extension->permissions_data()->HasAPIPermission(
           extensions::APIPermission::kVideoCapture) &&
       GetDevicePolicy(profile, extension->url(), prefs::kVideoCaptureAllowed,
-                      prefs::kVideoCaptureAllowedUrls) != ALWAYS_DENY;
+                      prefs::kVideoCaptureAllowedUrls) != ALWAYS_DENY &&
+      !user_choice.Disallowed(content::MEDIA_DEVICE_VIDEO_CAPTURE);
 
   bool get_default_audio_device = audio_allowed;
   bool get_default_video_device = video_allowed;