Public Sessions - prompt the user for pageCapture requests

chrome/browser/chromeos/extensions/public_session_permission_helper.h

+// Copyright 2017 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.
+
+#ifndef CHROME_BROWSER_CHROMEOS_EXTENSIONS_PUBLIC_SESSION_PERMISSION_HELPER_H_
+#define CHROME_BROWSER_CHROMEOS_EXTENSIONS_PUBLIC_SESSION_PERMISSION_HELPER_H_
+
+#include <memory>
+#include <set>
+#include <vector>
+
+#include "base/callback_forward.h"
+#include "base/macros.h"
+#include "chrome/browser/extensions/extension_install_prompt.h"
+#include "extensions/common/extension_id.h"
+#include "extensions/common/permissions/api_permission.h"
+#include "extensions/common/permissions/api_permission_set.h"
+
+namespace content {
+class WebContents;
+}
+
+namespace extensions {
+
+class Extension;
+
+// In Public Sessions, extensions (and apps) are force-installed by admin policy
+// so the user does not get a chance to review the permissions for these
+// extensions. This is not acceptable from a security/privacy standpoint, so
+// when an extension uses one of the sensitive APIs for the first time, we show
+// the user a dialog where they can choose whether to allow the extension access
+// to the API.
+//
+// This class encapsulates the common functionality needed to show permission
+// requests to the user and to cache the user choices. The interface exposes two
+// functions which are used to request additional permissions, or to query the
+// currently granted permissions.
+class PublicSessionPermissionHelper {
+ private:

[Andrew T Wilson (Slow), 2017/01/31 16:22:24]

Why is this private section up here at the top?

[Ivan Šandrk, 2017/02/01 18:11:29]

I wanted to define PermissionHelperSet as private to the class, and had to have
it before HandlePermissionRequest because it uses it as an argument type.

+  // PermissionIDSet::ContainsAnyID() function accepts only
+  // std::set<APIPermission::ID> argument, therefore PermissionHelperSet is
+  // used in this class.

[Andrew T Wilson (Slow), 2017/01/31 16:22:25]

nit: this naming is weird - PermissionHelperSet is not a set of
PermissionHelpers, it's a set of permissions. Maybe you could call it something
like "APIPermissionSet" or something? Generally a "FooSet" is a set of Foo
objects.

[Ivan Šandrk, 2017/02/01 18:11:29]

APIPermissionSet already exists. So does PermissionIDSet (best name), and
PermissionSet. I wasn't too happy with any of the existing Permission
collections, that is why I went with this.

How does ExtensionPermissionSet sound?

I could use PermissionIDSet tho, but I would need to make some modifications to
it and that depends on whether Devlin will be fine with them.

+  using PermissionHelperSet = std::set<APIPermission::ID>;
+
+ public:
+  // Sets up the prompt asking the user for additional permission(s), handles
+  // the result, caches it, and then runs either success_callback or
+  // failure_callback depending on all permissions being allowed.

[Andrew T Wilson (Slow), 2017/01/31 16:22:24]

So failure_callback is invoked if *any* permission is denied? Maybe make that
clear either here or in the last paragraph of this documentation.

[Ivan Šandrk, 2017/02/01 18:11:29]

Done.

+  //
+  // Supports handling multiple requests for the same permission(s). Only the
+  // first request causes the prompt to be shown, subsequent ones are just
+  // enqueued to be called when the permission(s) is resolved.

[Andrew T Wilson (Slow), 2017/01/31 16:22:24]

Are you saying that the user will be prompted multiple times? Or just that you
will queue up the success/failure callback and invoke it after the current
prompt is answered? Can you clarify the description?

[Ivan Šandrk, 2017/02/01 18:11:29]

User will be prompted only once per permission; the callback will be queued up
to be invoked when the current prompt is answered.

Done.

+  //
+  // Caller must ensure that web_contents is valid. Must be called on UI thread.
+  //
+  // If finer resolving is needed, pass the same function in both callbacks and
+  // check the individual permissions by calling PermissionAllowed inside your
+  // function.
+  static void HandlePermissionRequest(const Extension& extension,
+                                      PermissionHelperSet requested_permissions,
+                                      content::WebContents* web_contents,
+                                      const base::Closure& success_callback,
+                                      const base::Closure& failure_callback);
+
+  // Used to check whether a certain permission is allowed. Useful only if
+  // called inside success/failure callbacks.

[Andrew T Wilson (Slow), 2017/01/31 16:22:24]

Why is it only useful inside a callback? Why don't we pass in the permissions
granted to the extension to the callback, and remove this funcion?

[Ivan Šandrk, 2017/02/01 18:11:29]

The function should be called only after the permission has been resolved.

You are right, I should remove it.

+  static bool PermissionAllowed(ExtensionId extension_id,
+                                APIPermission::ID permission_id);
+
+  PublicSessionPermissionHelper();

[Andrew T Wilson (Slow), 2017/01/31 16:22:24]

Why do we have public constructors when all APIs are static? Feels like you
should just define 1-2 static functions in a namespace, and move this entire
class into a .cc file (no need to expose in a header file). The fact that you
implement the static method with a class instance is really an implementation
detail that should be hidden.

[Ivan Šandrk, 2017/02/01 18:11:29]

Good point. Done.

+  PublicSessionPermissionHelper(PublicSessionPermissionHelper&& other);
+  ~PublicSessionPermissionHelper();
+
+ private:
+  void HandlePermissionRequestImpl(const Extension& extension,
+                                   PermissionHelperSet requested_permissions,
+                                   content::WebContents* web_contents,
+                                   const base::Closure& success_callback,
+                                   const base::Closure& failure_callback);
+
+  bool PermissionAllowedImpl(APIPermission::ID permission_id);
+
+  void ResolvePermissionPrompt(
+      const std::unique_ptr<ExtensionInstallPrompt>* prompt,
+      const PermissionIDSet& unprompted_permissions,
+      ExtensionInstallPrompt::Result prompt_result);
+
+  struct RequestCallback {
+    RequestCallback(const base::Closure& success_callback,
+                    const base::Closure& failure_callback,
+                    const PermissionHelperSet& permission_list);
+    RequestCallback(const RequestCallback& other);
+    ~RequestCallback();
+    base::Closure success_callback;
+    base::Closure failure_callback;
+    PermissionHelperSet permission_list;
+  };
+  using RequestCallbackList = std::vector<RequestCallback>;
+
+  std::set<std::unique_ptr<ExtensionInstallPrompt>> prompts_;
+  PermissionIDSet prompted_permission_set_;
+  PermissionIDSet allowed_permission_set_;
+  PermissionIDSet denied_permission_set_;
+  RequestCallbackList callbacks_;
+
+  DISALLOW_COPY_AND_ASSIGN(PublicSessionPermissionHelper);
+};
+
+}  // namespace extensions
+
+#endif  // CHROME_BROWSER_CHROMEOS_EXTENSIONS_PUBLIC_SESSION_PERMISSION_HELPER_H_