Public Sessions - prompt the user for audioCapture/videoCapture requests

chrome/browser/media/public_session_media_access_handler.cc

+// Copyright 2016 The Chromium Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+#include "chrome/browser/media/public_session_media_access_handler.h"
+
+#include <utility>
+
+#include "base/bind.h"
+#include "base/bind_helpers.h"
+#include "base/memory/ptr_util.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/permission_set.h"
+#include "extensions/common/url_pattern_set.h"
+
+namespace {
+
+// Returns true if we're in a Public Session.
+bool IsPublicSession() {
+#if defined(OS_CHROMEOS)

[Devlin, 2016/12/02 20:40:28]

This should only be compiled on CrOS; remove ifdef

[Ivan Šandrk, 2016/12/05 13:10:51]

Done.

+  if (chromeos::LoginState::IsInitialized()) {

[Devlin, 2016/12/02 20:40:28]

simplify to
return chromeos::LoginState::IsInitialized() &&
       chromeos::LoginState::Get()->IsPublicSessionUser();

[Ivan Šandrk, 2016/12/05 13:10:50]

Done.

+    return chromeos::LoginState::Get()->IsPublicSessionUser();
+  }
+#endif
+  return false;
+}
+
+}  // namespace
+
+PublicSessionMediaAccessHandler::PublicSessionMediaAccessHandler() {}
+
+PublicSessionMediaAccessHandler::~PublicSessionMediaAccessHandler() {}
+
+bool PublicSessionMediaAccessHandler::SupportsStreamType(
+    const content::MediaStreamType type,
+    const extensions::Extension* extension) {
+  return ExtensionMediaAccessHandler::SupportsStreamType(type, extension);

[Devlin, 2016/12/02 20:40:28]

No need to override this if we just return the parent class's impl.

[Ivan Šandrk, 2016/12/05 13:10:51]

Acknowledged.

+}
+
+bool PublicSessionMediaAccessHandler::CheckMediaAccessPermission(
+    content::WebContents* web_contents,
+    const GURL& security_origin,
+    content::MediaStreamType type,
+    const extensions::Extension* extension) {
+  return ExtensionMediaAccessHandler::CheckMediaAccessPermission(

[Devlin, 2016/12/02 20:40:28]

ditto

[Ivan Šandrk, 2016/12/05 13:10:51]

Acknowledged.

+      web_contents, security_origin, type, extension);
+}
+
+void PublicSessionMediaAccessHandler::HandleRequest(
+    content::WebContents* web_contents,
+    const content::MediaStreamRequest& request,
+    const content::MediaResponseCallback& callback,
+    const extensions::Extension* extension) {
+  // This class handles requests for Public Sessions only, outside of them just
+  // pass the request through to the original class.
+  if (!(IsPublicSession() && extension && extension->is_platform_app()))

[Sergey Ulanov, 2016/12/02 20:29:47]

don't need to check extension here. This function should never be called with
extesnion==nullptr, because SupportsStreamType() would return false otherwise.
Maybe add DCHECK(extension).
Also it would be more readable as (!IsPublicSession() ||
!extension->is_platform_app()).

[Ivan Šandrk, 2016/12/05 13:10:51]

Done.

+    return ExtensionMediaAccessHandler::HandleRequest(web_contents, request,

[Sergey Ulanov, 2016/12/02 20:29:47]

need {} for multi-line if blocks:
https://google.github.io/styleguide/cppguide.html#Conditionals

[Ivan Šandrk, 2016/12/05 13:10:51]

Done.

+                                                      callback, extension);
+
+  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) {

[Sergey Ulanov, 2016/12/02 20:29:47]

The code would be more readable if you handle !needs_prompt first:

if (!needs_prompt) {
  ChainHandleRequest(web_contents, request, callback, extension);
  return;
}

.. show prompt.

[Devlin, 2016/12/02 20:40:28]

(+1)

[Ivan Šandrk, 2016/12/05 13:10:51]

Good point!

+    auto permission_set = base::MakeUnique<extensions::PermissionSet>(
+        new_apis, extensions::ManifestPermissionSet(),
+        extensions::URLPatternSet(), extensions::URLPatternSet());
+
+    auto prompt = base::MakeUnique<ExtensionInstallPrompt>(web_contents);
+    // Raw copy used *only* for calling ShowDialog (to avoid the awkward
+    // situation where the same unique_ptr is used as this and is passed through
+    // inside a callback).
+    auto prompt_raw = prompt.get();
+
+    // "Dialog closed" callback owns the prompt, hence the prompt gets destroyed
+    // together with the callback. The callback is guaranteed to be destroyed

[Andrew T Wilson (Slow), 2016/12/04 19:57:01]

how is the destruction of the callback guaranteed? This is OK, but these types
of mechanisms tend to be fragile - why not store a unique_ptr to the dialog
along with the UserChoice for this extension (we can only have one prompt per
extension, right?) - that way you know it gets freed when this object is
destroyed, and you can free the dialog when ResolvePermissionPrompt is invoked
and the user choice is updated.

[Ivan Šandrk, 2016/12/05 17:04:46]

Changed the approach.

+    // even if it's not run.
+    prompt_raw->ShowDialog(
+        base::Bind(&PublicSessionMediaAccessHandler::ResolvePermissionPrompt,
+                   base::Unretained(this), web_contents, request, callback,
+                   extension, std::move(prompt)),
+        extension,
+        nullptr,  // Uses the extension icon.
+        base::MakeUnique<ExtensionInstallPrompt::Prompt>(
+            ExtensionInstallPrompt::PERMISSIONS_PROMPT),
+        std::move(permission_set),
+        ExtensionInstallPrompt::GetDefaultShowDialogCallback());
+    return;
+  }
+
+  ChainHandleRequest(web_contents, request, callback, extension);
+}
+
+void PublicSessionMediaAccessHandler::ChainHandleRequest(
+    content::WebContents* web_contents,
+    const content::MediaStreamRequest& request,
+    const content::MediaResponseCallback& callback,
+    const extensions::Extension* extension) {
+  DCHECK(IsPublicSession() && extension && extension->is_platform_app());
+  const UserChoice& user_choice = user_choice_cache_[extension->id()];
+  content::MediaStreamRequest request_copy(request);
+
+  if (!user_choice.IsAllowed(content::MEDIA_DEVICE_AUDIO_CAPTURE))

[Andrew T Wilson (Slow), 2016/12/04 19:57:01]

Can you comment about what this code is doing?

[Ivan Šandrk, 2016/12/05 17:04:46]

Done.

+    request_copy.audio_type = content::MEDIA_NO_SERVICE;
+  if (!user_choice.IsAllowed(content::MEDIA_DEVICE_VIDEO_CAPTURE))
+    request_copy.video_type = content::MEDIA_NO_SERVICE;
+
+  // Pass the request through to the original class.
+  ExtensionMediaAccessHandler::HandleRequest(web_contents, request_copy,
+                                             callback, extension);
+}
+
+void PublicSessionMediaAccessHandler::ResolvePermissionPrompt(
+    content::WebContents* web_contents,
+    const content::MediaStreamRequest& request,
+    const content::MediaResponseCallback& callback,
+    const extensions::Extension* extension,
+    const std::unique_ptr<ExtensionInstallPrompt>& prompt,
+    ExtensionInstallPrompt::Result prompt_result) {
+  bool allowed = (prompt_result == ExtensionInstallPrompt::Result::ACCEPTED);

[Devlin, 2016/12/02 20:40:28]

nit: parens unnecessary

[Ivan Šandrk, 2016/12/05 13:10:51]

Done.

+  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);
+
+  ChainHandleRequest(web_contents, request, callback, extension);
+}
+
+bool PublicSessionMediaAccessHandler::UserChoice::IsAllowed(
+    content::MediaStreamType type) const {
+  if (type == content::MEDIA_DEVICE_AUDIO_CAPTURE) {

[Sergey Ulanov, 2016/12/02 20:29:47]

nit: Maybe replace this with a switch statement
Same for the functions below

[Ivan Šandrk, 2016/12/05 13:10:51]

Done.

+    return !audio_prompted_ || audio_allowed_;
+  } else {

[Devlin, 2016/12/02 20:40:28]

I like Sergey's idea about the switch, so probably moot, but for future
reference no "else" after a return.

[Ivan Šandrk, 2016/12/05 13:10:51]

Good point.

+    DCHECK(type == content::MEDIA_DEVICE_VIDEO_CAPTURE);
+    return !video_prompted_ || video_allowed_;
+  }
+}
+
+bool PublicSessionMediaAccessHandler::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;

[Sergey Ulanov, 2016/12/02 20:29:47]

add NOTREACHED() here?

[Ivan Šandrk, 2016/12/05 13:10:51]

This part is hit sometimes (when type is content::MEDIA_NO_SERVICE, ie. for
example video wasn't requested, only audio). I can also replace this function
with:
  return type == content::MEDIA_DEVICE_AUDIO_CAPTURE && !audio_prompted_ ||
         type == content::MEDIA_DEVICE_VIDEO_CAPTURE && !video_prompted_;

Does that seem more readable?

+}
+
+void PublicSessionMediaAccessHandler::UserChoice::Set(
+    content::MediaStreamType type, bool allowed) {
+  if (type == content::MEDIA_DEVICE_AUDIO_CAPTURE) {
+    DCHECK(!audio_prompted_);
+    audio_prompted_ = true;
+    audio_allowed_ = allowed;
+  } else {
+    DCHECK(type == content::MEDIA_DEVICE_VIDEO_CAPTURE);

[Devlin, 2016/12/02 20:40:28]

nit: DCHECK_EQ

[Ivan Šandrk, 2016/12/05 13:10:51]

Done.

+    DCHECK(!video_prompted_);
+    video_prompted_ = true;
+    video_allowed_ = allowed;
+  }
+}