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

chrome/browser/extensions/permissions_updater.h

 // Copyright (c) 2012 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_EXTENSIONS_PERMISSIONS_UPDATER_H__
 #define CHROME_BROWSER_EXTENSIONS_PERMISSIONS_UPDATER_H__
 
 #include <memory>
 #include <string>
 
 #include "base/macros.h"
 #include "extensions/browser/extension_event_histogram_value.h"
 
 namespace content {
 class BrowserContext;
 }
 
 namespace extensions {
 
 class Extension;
 class PermissionSet;
 
 // Updates an Extension's active and granted permissions in persistent storage
 // 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,
   };
 
   enum RemoveType {
     REMOVE_SOFT,
     REMOVE_HARD,
   };
 
   explicit PermissionsUpdater(content::BrowserContext* browser_context);
   PermissionsUpdater(content::BrowserContext* browser_context,
                      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.
   void AddPermissions(const Extension* extension,
                       const PermissionSet& permissions);
 
   // Removes the set of |permissions| from the |extension|'s active permission
   // set and sends the relevant messages and notifications.
   // If |remove_type| is REMOVE_HARD, this removes the permissions from the
   // granted permissions in the prefs (meaning that the extension would have
@@ skipping 60 matching lines @@
   // Initialization flag that determines whether prefs is consulted about the
   // extension. Transient extensions should not have entries in prefs.
   InitFlag init_flag_;
 
   DISALLOW_COPY_AND_ASSIGN(PermissionsUpdater);
 };
 
 }  // namespace extensions
 
 #endif  // CHROME_BROWSER_EXTENSIONS_PERMISSIONS_UPDATER_H__