PS - Remove Clipboard Read permission from extensions in Public Sessions (except for whitelisted on…

chrome/browser/extensions/permissions_updater.h

Index: chrome/browser/extensions/permissions_updater.h
diff --git a/chrome/browser/extensions/permissions_updater.h b/chrome/browser/extensions/permissions_updater.h
index e31d52a45b0d00a6e0b902b2727139cf01e13605..7547ff3da644b01eafdfed776c3282cd65e0e798 100644
--- a/chrome/browser/extensions/permissions_updater.h
+++ b/chrome/browser/extensions/permissions_updater.h
@@ -24,6 +24,15 @@ class PermissionSet;
 // and notifies interested parties of the changes.
 class PermissionsUpdater {
  public:
+  // Platform specific delegate.
+  class Delegate {
+   public:
+    virtual ~Delegate() {}
+    virtual void InitializePermissions(

[Andrew T Wilson (Slow), 2017/04/11 11:32:54]

Document what exactly InitialPermissions is supposed to do (what's the
contract?) - I don't think it's documented anywhere. Why is it called
InitializePermissions() and not FilterPermissions()?

Finally, is this the right API, rather than maybe passing a pointer to the
PermissionSet that can be modified directly (i.e. why isn't this defined as):

virtual void InitializePermissions(const Extension* extension, PermissionSet*
granted);

[Ivan Šandrk, 2017/04/11 13:37:27]

Devlin gave me the suggestion to use InitializePermissions, and it seemed
fitting to me as just being the platform specific specialization for the
function.
The code it's hooked into exposes just a std::unique_ptr<const PermissionSet> to
be modified, and the preceding code does the same thing I did here.

[Devlin, 2017/04/11 15:03:38]

I recommended InitializePermissions to keep symmetry with
PermissionsUpdater::InitializePermissions().  And there's no guarantee that each
delegate implementation would want to filter (rather than add or otherwise
modify).

+        const Extension* extension,
+        std::unique_ptr<const PermissionSet>* granted_permissions) = 0;
+  };
+
   enum InitFlag {
     INIT_FLAG_NONE = 0,
     INIT_FLAG_TRANSIENT = 1 << 0,
@@ -39,6 +48,9 @@ class PermissionsUpdater {
                      InitFlag init_flag);
   ~PermissionsUpdater();
 
+  // Used for platform specific logic as to not litter the code with #ifdefs.
+  static void SetPlatformDelegate(Delegate* delegate);

[Andrew T Wilson (Slow), 2017/04/11 11:32:54]

I'm surprised to see this as a static function/static member. What's the
lifetime of PermissionsUpdater - how many of them exist? Why isn't the delegate
stored as a member of PermissionsUpdater (maybe it isn't created yet at the time
we call SetPlatformDelegate()?). What is the lifetime of PermissionsUpdater (how
long does |delegate| have to stay alive - when can the owner free it? I guess
it's OK to free it as long as someone calls SetPlatformDelegate(nullptr) first?
We should document that.

[Ivan Šandrk, 2017/04/11 13:37:27]

PermissionsUpdater has a short lifetime - just a helper object used to change
permissions on extensions. Many of them are created, but I'd say only one exists
at a given time.

SetPlatformDelegate is called during login into public session, before any of
the PermissionsUpdater are created. I've made it a static function because it
didn't really make sense otherwise to me, too many calls to PermissionsUpdater
in too many locations.

The delegate should stay alive for the whole user-session - it nevers gets
free'd, it's leaked, and it gets reclaimed during logout when the whole chrome
process is shut down.

[Devlin, 2017/04/11 15:03:38]

PermissionsUpdater is designed to be a (mostly) single use class that's just
allocated on the stack as-needed, so there's no common place to hook in. 
Additionally, it seems like delegate implementations would want to be applied to
each PermissionsUpdater, rather than only to a subset.  (Of course, if we ever
have more than one, we'll need to tweak this a bit, e.g. adding to a vector to
iterate over or similar)

+
   // Adds the set of |permissions| to the |extension|'s active permission set
   // and sends the relevant messages and notifications. This method assumes the
   // user has already been prompted, if necessary, for the extra permissions.