Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2794803003:
PS - Remove Clipboard Read permission from extensions in Public Sessions (except for whitelisted on… (Closed)
|
Created:
3 years, 8 months ago by Ivan Šandrk Modified:
3 years, 8 months ago CC:
chromium-reviews, chromium-apps-reviews_chromium.org, oshima+watch_chromium.org, davemoore+watch_chromium.org, extensions-reviews_chromium.org Target Ref:
refs/heads/master Project:
chromium Visibility:
Public. |
DescriptionPS - Remove Clipboard Read permission from extensions in Public Sessions (except for whitelisted ones)
In Public Sessions, apps and extensions are force-installed by admin policy so the user does not get a chance to review the permissions for these apps. This is not acceptable from a security standpoint, so we remove ClipboardRead permission from them (except for whitelisted ones - eg. remote desktop clients). This forceful removal of permission is safe since the clipboard pasting code checks for this permission before doing the paste.
TEST=
unit_tests --gtest_filter=DeviceLocalAccountManagementPolicyProviderTest.IsWhitelisted
unit_tests --gtest_filter=PermissionsUpdaterDelegateChromeOSTest.*
unit_tests --gtest_filter=PermissionsUpdaterTest.Delegate
BUG=707864
Review-Url: https://codereview.chromium.org/2794803003
Cr-Commit-Position: refs/heads/master@{#464031}
Committed: https://chromium.googlesource.com/chromium/src/+/80e3eb904ef28e2491b831099597fc05347845ae
Patch Set 1 #
Total comments: 2
Patch Set 2 : Using a platform delegate #
Total comments: 14
Patch Set 3 : Set delegate from Public Session specific code #Patch Set 4 : Added tests #
Total comments: 6
Patch Set 5 : Updated comments #
Total comments: 13
Patch Set 6 : Drew's comments #Patch Set 7 : Fixed test crash #
Total comments: 4
Patch Set 8 : Updated comment #Messages
Total messages: 67 (47 generated)
The CQ bit was checked by isandrk@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
isandrk@chromium.org changed reviewers: + rdevlin.cronin@chromium.org
Hey Devlin, take a preliminary stab at this just to make sure I'm not doing anything too wrong. Tomorrow I'll see if I can add some tests. Ivan
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2794803003/diff/1/chrome/browser/extensions/p... File chrome/browser/extensions/permissions_updater.cc (right): https://codereview.chromium.org/2794803003/diff/1/chrome/browser/extensions/p... chrome/browser/extensions/permissions_updater.cc:208: #if defined(OS_CHROMEOS) I'd like to avoid adding so many #ifdefs and chromeos + public sessions specific logic here. Could we instead do something like: namespace { PermissionsUpdater* g_delegate = nullptr; } void PermissionsUpdater::SetPlatformDelegate(Delegate* delegate) { g_delegate = delegate; } void InitializePermissions() { .... if (g_delegate) g_delegate->InitializePermissions(...); } and then in CrOS-specific code, have a PermissionsUpdaterDelegate that implements an InitializePermissions function that withholds this? It's a bit more code, but I think it's a little cleaner in the sense that it keeps core extensions code from needing to know (more) about public sessions, etc. https://codereview.chromium.org/2794803003/diff/1/chrome/browser/extensions/s... File chrome/browser/extensions/scripting_permissions_modifier.h (right): https://codereview.chromium.org/2794803003/diff/1/chrome/browser/extensions/s... chrome/browser/extensions/scripting_permissions_modifier.h:94: void WithholdClipboardRead(); This isn't a script permission, so doesn't really belong here. :) See other comment in permissions updater for an alternative suggestion.
The CQ bit was checked by isandrk@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Moved out platform specific code to a delegate. Ptal again! https://codereview.chromium.org/2794803003/diff/20001/chrome/browser/extensio... File chrome/browser/extensions/permissions_updater.cc (right): https://codereview.chromium.org/2794803003/diff/20001/chrome/browser/extensio... chrome/browser/extensions/permissions_updater.cc:80: if (!g_delegate) I'm not sure if the SetPlatformDelegate function is needed at all. And I'm also not sure if doing it like this is ok. https://codereview.chromium.org/2794803003/diff/20001/chrome/browser/extensio... chrome/browser/extensions/permissions_updater.cc:261: DCHECK_EQ(init_flag_ & INIT_FLAG_TRANSIENT, 0); Fixed linter error.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
isandrk@chromium.org changed reviewers: + atwilson@chromium.org
Hey Drew, ptal!
https://codereview.chromium.org/2794803003/diff/20001/chrome/browser/extensio... File chrome/browser/extensions/permissions_updater.cc (right): https://codereview.chromium.org/2794803003/diff/20001/chrome/browser/extensio... chrome/browser/extensions/permissions_updater.cc:80: if (!g_delegate) On 2017/04/04 15:41:56, Ivan Šandrk wrote: > I'm not sure if the SetPlatformDelegate function is needed at all. And I'm also > not sure if doing it like this is ok. I think what would be better would be instead moving this call to SetDelegate to chromeos session code. I don't know that code well, but I'm assuming there's some point at which we know that we're in a public session. From there, we set the delegate, since that's the only case we need. Then, we don't need any #ifdefs in this file! :) https://codereview.chromium.org/2794803003/diff/20001/chrome/browser/extensio... chrome/browser/extensions/permissions_updater.cc:261: DCHECK_EQ(init_flag_ & INIT_FLAG_TRANSIENT, 0); On 2017/04/04 15:41:57, Ivan Šandrk wrote: > Fixed linter error. Thanks! But prefer DCHECK_EQ(expected, actual), so DCHECK_EQ(0, init_flag & INIT_FLAG_TRANSIENT); https://codereview.chromium.org/2794803003/diff/20001/chrome/browser/extensio... File chrome/browser/extensions/permissions_updater_delegate_chromeos.cc (right): https://codereview.chromium.org/2794803003/diff/20001/chrome/browser/extensio... chrome/browser/extensions/permissions_updater_delegate_chromeos.cc:36: permissions_updater->RemovePermissionsUnsafe(extension, permissions); this does a lot of extra work (e.g. notifying subsystems that permissions were removed, etc). Since this is for initialization, we can instead just avoid ever adding the permissions. To do this, we could pass in a mutable pointer to the granted permissions from InitializePermissions. https://codereview.chromium.org/2794803003/diff/20001/chrome/browser/extensio... File chrome/browser/extensions/permissions_updater_delegate_chromeos.h (right): https://codereview.chromium.org/2794803003/diff/20001/chrome/browser/extensio... chrome/browser/extensions/permissions_updater_delegate_chromeos.h:10: #include "content/public/browser/browser_context.h" unneeded https://codereview.chromium.org/2794803003/diff/20001/chrome/browser/extensio... chrome/browser/extensions/permissions_updater_delegate_chromeos.h:11: #include "extensions/common/extension.h" prefer forward declaration https://codereview.chromium.org/2794803003/diff/20001/chrome/browser/extensio... chrome/browser/extensions/permissions_updater_delegate_chromeos.h:22: class PermissionsUpdaterDelegateChromeOS : public PermissionsUpdater::Delegate { as hinted in permissions updater, it'd be great for this to be in CrOS public sessions code.
+1 to existing comments, also is it possible to add tests? At least testing the delegate impl makes sense.
The CQ bit was checked by isandrk@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Patchset #3 (id:40001) has been deleted
The CQ bit was checked by isandrk@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Patchset #3 (id:60001) has been deleted
Dry run: Try jobs failed on following builders: ios-device on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...) ios-simulator on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...) ios-simulator-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator-xco...)
The CQ bit was checked by isandrk@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by isandrk@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Patchset #4 (id:100001) has been deleted
Dry run: Try jobs failed on following builders: android_clang_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_clan...) chromeos_daisy_chromium_compile_only_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_daisy_...) linux_chromium_compile_dbg_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...) mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
The CQ bit was checked by isandrk@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
isandrk@chromium.org changed reviewers: + alemate@chromium.org
Hey guys, ptal again! Alexander, ptal at chrome/browser/chromeos/login/users/chrome_user_manager_impl.cc https://codereview.chromium.org/2794803003/diff/20001/chrome/browser/extensio... File chrome/browser/extensions/permissions_updater.cc (right): https://codereview.chromium.org/2794803003/diff/20001/chrome/browser/extensio... chrome/browser/extensions/permissions_updater.cc:80: if (!g_delegate) On 2017/04/05 14:35:20, Devlin wrote: > On 2017/04/04 15:41:56, Ivan Šandrk wrote: > > I'm not sure if the SetPlatformDelegate function is needed at all. And I'm > also > > not sure if doing it like this is ok. > > I think what would be better would be instead moving this call to SetDelegate to > chromeos session code. I don't know that code well, but I'm assuming there's > some point at which we know that we're in a public session. From there, we set > the delegate, since that's the only case we need. Then, we don't need any > #ifdefs in this file! :) Right, done. https://codereview.chromium.org/2794803003/diff/20001/chrome/browser/extensio... chrome/browser/extensions/permissions_updater.cc:261: DCHECK_EQ(init_flag_ & INIT_FLAG_TRANSIENT, 0); On 2017/04/05 14:35:20, Devlin wrote: > On 2017/04/04 15:41:57, Ivan Šandrk wrote: > > Fixed linter error. > > Thanks! But prefer DCHECK_EQ(expected, actual), so > DCHECK_EQ(0, init_flag & INIT_FLAG_TRANSIENT); Done. https://codereview.chromium.org/2794803003/diff/20001/chrome/browser/extensio... File chrome/browser/extensions/permissions_updater_delegate_chromeos.cc (right): https://codereview.chromium.org/2794803003/diff/20001/chrome/browser/extensio... chrome/browser/extensions/permissions_updater_delegate_chromeos.cc:36: permissions_updater->RemovePermissionsUnsafe(extension, permissions); On 2017/04/05 14:35:20, Devlin wrote: > this does a lot of extra work (e.g. notifying subsystems that permissions were > removed, etc). Since this is for initialization, we can instead just avoid ever > adding the permissions. To do this, we could pass in a mutable pointer to the > granted permissions from InitializePermissions. Thanks for the explanation, done. https://codereview.chromium.org/2794803003/diff/20001/chrome/browser/extensio... File chrome/browser/extensions/permissions_updater_delegate_chromeos.h (right): https://codereview.chromium.org/2794803003/diff/20001/chrome/browser/extensio... chrome/browser/extensions/permissions_updater_delegate_chromeos.h:10: #include "content/public/browser/browser_context.h" On 2017/04/05 14:35:20, Devlin wrote: > unneeded Done. https://codereview.chromium.org/2794803003/diff/20001/chrome/browser/extensio... chrome/browser/extensions/permissions_updater_delegate_chromeos.h:11: #include "extensions/common/extension.h" On 2017/04/05 14:35:20, Devlin wrote: > prefer forward declaration Done. https://codereview.chromium.org/2794803003/diff/20001/chrome/browser/extensio... chrome/browser/extensions/permissions_updater_delegate_chromeos.h:22: class PermissionsUpdaterDelegateChromeOS : public PermissionsUpdater::Delegate { On 2017/04/05 14:35:20, Devlin wrote: > as hinted in permissions updater, it'd be great for this to be in CrOS public > sessions code. Done.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Description was changed from ========== PS - Remove Clipboard Read permission from extensions in Public Sessions (except for whitelisted ones) In Public Sessions, apps and extensions are force-installed by admin policy so the user does not get a chance to review the permissions for these apps. This is not acceptable from a security standpoint, so we remove ClipboardRead permission from them (except for whitelisted ones - eg. remote desktop clients). This forceful removal of permission is safe since the clipboard pasting code checks for this permission before doing the paste. TEST=manual BUG=707864 ========== to ========== PS - Remove Clipboard Read permission from extensions in Public Sessions (except for whitelisted ones) In Public Sessions, apps and extensions are force-installed by admin policy so the user does not get a chance to review the permissions for these apps. This is not acceptable from a security standpoint, so we remove ClipboardRead permission from them (except for whitelisted ones - eg. remote desktop clients). This forceful removal of permission is safe since the clipboard pasting code checks for this permission before doing the paste. TEST= unit_tests --gtest_filter=DeviceLocalAccountManagementPolicyProviderTest.IsWhitelisted unit_tests --gtest_filter=PermissionsUpdaterDelegateChromeOSTest.* unit_tests --gtest_filter=PermissionsUpdaterTest.Delegate BUG=707864 ==========
chrome_user_manager_impl.cc lgtm
extensions lgtm A few other drive-by nits, but in general, this looks great. :) https://codereview.chromium.org/2794803003/diff/120001/chrome/browser/chromeo... File chrome/browser/chromeos/extensions/permissions_updater_delegate_chromeos.cc (right): https://codereview.chromium.org/2794803003/diff/120001/chrome/browser/chromeo... chrome/browser/chromeos/extensions/permissions_updater_delegate_chromeos.cc:26: extension)) { nit: || !granted_permissions->HasAPIPermission(APIPermission::kClipboardRead) https://codereview.chromium.org/2794803003/diff/120001/chrome/browser/chromeo... File chrome/browser/chromeos/extensions/permissions_updater_delegate_chromeos.h (right): https://codereview.chromium.org/2794803003/diff/120001/chrome/browser/chromeo... chrome/browser/chromeos/extensions/permissions_updater_delegate_chromeos.h:20: // This is not acceptable from a security standpoint, so we remove ClipboardRead This comment starts to get very specific, and is probably better placed where we remove the clipboardRead permission. We could keep this class comment a bit more general by saying something like "remove the permissions that violate security or privacy in public sessions". https://codereview.chromium.org/2794803003/diff/120001/chrome/browser/chromeo... File chrome/browser/chromeos/login/users/chrome_user_manager_impl.cc (right): https://codereview.chromium.org/2794803003/diff/120001/chrome/browser/chromeo... chrome/browser/chromeos/login/users/chrome_user_manager_impl.cc:842: new extensions::PermissionsUpdaterDelegateChromeOS); This will leak - is that intentional?
The CQ bit was checked by isandrk@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Thanks for the explanations Devlin, learned something new :-) https://codereview.chromium.org/2794803003/diff/120001/chrome/browser/chromeo... File chrome/browser/chromeos/extensions/permissions_updater_delegate_chromeos.cc (right): https://codereview.chromium.org/2794803003/diff/120001/chrome/browser/chromeo... chrome/browser/chromeos/extensions/permissions_updater_delegate_chromeos.cc:26: extension)) { On 2017/04/08 00:30:51, Devlin wrote: > nit: || !granted_permissions->HasAPIPermission(APIPermission::kClipboardRead) Done. https://codereview.chromium.org/2794803003/diff/120001/chrome/browser/chromeo... File chrome/browser/chromeos/extensions/permissions_updater_delegate_chromeos.h (right): https://codereview.chromium.org/2794803003/diff/120001/chrome/browser/chromeo... chrome/browser/chromeos/extensions/permissions_updater_delegate_chromeos.h:20: // This is not acceptable from a security standpoint, so we remove ClipboardRead On 2017/04/08 00:30:51, Devlin wrote: > This comment starts to get very specific, and is probably better placed where we > remove the clipboardRead permission. We could keep this class comment a bit > more general by saying something like "remove the permissions that violate > security or privacy in public sessions". Done. https://codereview.chromium.org/2794803003/diff/120001/chrome/browser/chromeo... File chrome/browser/chromeos/login/users/chrome_user_manager_impl.cc (right): https://codereview.chromium.org/2794803003/diff/120001/chrome/browser/chromeo... chrome/browser/chromeos/login/users/chrome_user_manager_impl.cc:842: new extensions::PermissionsUpdaterDelegateChromeOS); On 2017/04/08 00:30:51, Devlin wrote: > This will leak - is that intentional? Yes. It's set once during login into Public Session, and it's leaked (gets reclaimed during logout when the whole Chrome process is shut down).
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2794803003/diff/140001/chrome/browser/chromeo... File chrome/browser/chromeos/extensions/permissions_updater_delegate_chromeos.cc (right): https://codereview.chromium.org/2794803003/diff/140001/chrome/browser/chromeo... chrome/browser/chromeos/extensions/permissions_updater_delegate_chromeos.cc:27: !(*granted_permissions) Why check for this here? If kClipboardRead isn't set, then the code below is a no-op? I'm just thinking that if we ever want to extend this code to remove other permissions, it's gonna make this logic more convoluted. https://codereview.chromium.org/2794803003/diff/140001/chrome/browser/chromeo... File chrome/browser/chromeos/extensions/permissions_updater_delegate_chromeos_unittest.cc (right): https://codereview.chromium.org/2794803003/diff/140001/chrome/browser/chromeo... chrome/browser/chromeos/extensions/permissions_updater_delegate_chromeos_unittest.cc:62: EXPECT_EQ(*CreatePermissions(), *granted_permissions); This is fine, but I probably would've created separate tests for each case (non-ps-whitelisted + non-ps-nonwhitelisted) because then it's easy to tell what broke if a test failed just from seeing which test failed. Especially since the setup code is pretty minimal (just need to create a delegate). https://codereview.chromium.org/2794803003/diff/140001/chrome/browser/extensi... File chrome/browser/extensions/permissions_updater.cc (right): https://codereview.chromium.org/2794803003/diff/140001/chrome/browser/extensi... chrome/browser/extensions/permissions_updater.cc:89: g_delegate = delegate; I think we should CHECK(!g_delegate) here to make sure we never set a delegate multiple times. https://codereview.chromium.org/2794803003/diff/140001/chrome/browser/extensi... File chrome/browser/extensions/permissions_updater.h (right): https://codereview.chromium.org/2794803003/diff/140001/chrome/browser/extensi... chrome/browser/extensions/permissions_updater.h:31: virtual void InitializePermissions( 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); https://codereview.chromium.org/2794803003/diff/140001/chrome/browser/extensi... chrome/browser/extensions/permissions_updater.h:52: static void SetPlatformDelegate(Delegate* delegate); 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.
The CQ bit was checked by isandrk@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Drew, I've adressed your comments. Ptal https://codereview.chromium.org/2794803003/diff/140001/chrome/browser/chromeo... File chrome/browser/chromeos/extensions/permissions_updater_delegate_chromeos.cc (right): https://codereview.chromium.org/2794803003/diff/140001/chrome/browser/chromeo... chrome/browser/chromeos/extensions/permissions_updater_delegate_chromeos.cc:27: !(*granted_permissions) On 2017/04/11 11:32:54, Andrew T Wilson (Slow) wrote: > Why check for this here? If kClipboardRead isn't set, then the code below is a > no-op? I'm just thinking that if we ever want to extend this code to remove > other permissions, it's gonna make this logic more convoluted. Devlin told me to add this part. https://codereview.chromium.org/2794803003/diff/140001/chrome/browser/chromeo... File chrome/browser/chromeos/extensions/permissions_updater_delegate_chromeos_unittest.cc (right): https://codereview.chromium.org/2794803003/diff/140001/chrome/browser/chromeo... chrome/browser/chromeos/extensions/permissions_updater_delegate_chromeos_unittest.cc:62: EXPECT_EQ(*CreatePermissions(), *granted_permissions); On 2017/04/11 11:32:54, Andrew T Wilson (Slow) wrote: > This is fine, but I probably would've created separate tests for each case > (non-ps-whitelisted + non-ps-nonwhitelisted) because then it's easy to tell what > broke if a test failed just from seeing which test failed. Especially since the > setup code is pretty minimal (just need to create a delegate). Acknowledged. Was thinking about this myself but then decided to not split the tests completely. I'll keep this in mind for next time (lightweight tests). https://codereview.chromium.org/2794803003/diff/140001/chrome/browser/extensi... File chrome/browser/extensions/permissions_updater.cc (right): https://codereview.chromium.org/2794803003/diff/140001/chrome/browser/extensi... chrome/browser/extensions/permissions_updater.cc:89: g_delegate = delegate; On 2017/04/11 11:32:54, Andrew T Wilson (Slow) wrote: > I think we should CHECK(!g_delegate) here to make sure we never set a delegate > multiple times. Done. https://codereview.chromium.org/2794803003/diff/140001/chrome/browser/extensi... File chrome/browser/extensions/permissions_updater.h (right): https://codereview.chromium.org/2794803003/diff/140001/chrome/browser/extensi... chrome/browser/extensions/permissions_updater.h:31: virtual void InitializePermissions( On 2017/04/11 11:32:54, Andrew T Wilson (Slow) wrote: > 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()? Devlin gave me the suggestion to use InitializePermissions, and it seemed fitting to me as just being the platform specific specialization for the function. > 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); 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. https://codereview.chromium.org/2794803003/diff/140001/chrome/browser/extensi... chrome/browser/extensions/permissions_updater.h:52: static void SetPlatformDelegate(Delegate* delegate); On 2017/04/11 11:32:54, Andrew T Wilson (Slow) wrote: > 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. 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.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
The CQ bit was checked by isandrk@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
(just responding to some questions) https://codereview.chromium.org/2794803003/diff/140001/chrome/browser/chromeo... File chrome/browser/chromeos/extensions/permissions_updater_delegate_chromeos.cc (right): https://codereview.chromium.org/2794803003/diff/140001/chrome/browser/chromeo... chrome/browser/chromeos/extensions/permissions_updater_delegate_chromeos.cc:27: !(*granted_permissions) On 2017/04/11 13:37:27, Ivan Šandrk wrote: > On 2017/04/11 11:32:54, Andrew T Wilson (Slow) wrote: > > Why check for this here? If kClipboardRead isn't set, then the code below is a > > no-op? I'm just thinking that if we ever want to extend this code to remove > > other permissions, it's gonna make this logic more convoluted. > > Devlin told me to add this part. PermissionSets are designed to be (mostly) immutable, so when we remove a permission, we do so by constructing a new PermissionSet, which isn't terribly expensive, but isn't free. I'd prefer we only do that work if necessary. https://codereview.chromium.org/2794803003/diff/140001/chrome/browser/extensi... File chrome/browser/extensions/permissions_updater.h (right): https://codereview.chromium.org/2794803003/diff/140001/chrome/browser/extensi... chrome/browser/extensions/permissions_updater.h:31: virtual void InitializePermissions( On 2017/04/11 13:37:27, Ivan Šandrk wrote: > On 2017/04/11 11:32:54, Andrew T Wilson (Slow) wrote: > > 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()? > > Devlin gave me the suggestion to use InitializePermissions, and it seemed > fitting to me as just being the platform specific specialization for the > function. > > > 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); > > 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. 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). https://codereview.chromium.org/2794803003/diff/140001/chrome/browser/extensi... chrome/browser/extensions/permissions_updater.h:52: static void SetPlatformDelegate(Delegate* delegate); On 2017/04/11 13:37:27, Ivan Šandrk wrote: > On 2017/04/11 11:32:54, Andrew T Wilson (Slow) wrote: > > 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. > > 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. 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)
https://codereview.chromium.org/2794803003/diff/180001/chrome/browser/extensi... File chrome/browser/extensions/permissions_updater.h (right): https://codereview.chromium.org/2794803003/diff/180001/chrome/browser/extensi... chrome/browser/extensions/permissions_updater.h:56: // freed but this is fine since it will go away together with the browser I'm not quite sure I follow the "this is fine since it will go away together with the browser process". Isn't this just leaked? Doesn't any allocated object go away with the process its in (unless its e.g. persisted to disk, or we're talking about very complex handles?) I'd just summarize this as: // |delegate| is leaked.
https://codereview.chromium.org/2794803003/diff/180001/chrome/browser/extensi... File chrome/browser/extensions/permissions_updater.h (right): https://codereview.chromium.org/2794803003/diff/180001/chrome/browser/extensi... chrome/browser/extensions/permissions_updater.h:56: // freed but this is fine since it will go away together with the browser On 2017/04/11 15:06:22, Devlin wrote: > I'm not quite sure I follow the "this is fine since it will go away together > with the browser process". Isn't this just leaked? Doesn't any allocated > object go away with the process its in (unless its e.g. persisted to disk, or > we're talking about very complex handles?) > > I'd just summarize this as: > // |delegate| is leaked. You would summarize the whole "|delegate| is created ..." part?
https://codereview.chromium.org/2794803003/diff/180001/chrome/browser/extensi... File chrome/browser/extensions/permissions_updater.h (right): https://codereview.chromium.org/2794803003/diff/180001/chrome/browser/extensi... chrome/browser/extensions/permissions_updater.h:56: // freed but this is fine since it will go away together with the browser On 2017/04/11 15:11:16, Ivan Šandrk wrote: > On 2017/04/11 15:06:22, Devlin wrote: > > I'm not quite sure I follow the "this is fine since it will go away together > > with the browser process". Isn't this just leaked? Doesn't any allocated > > object go away with the process its in (unless its e.g. persisted to disk, or > > we're talking about very complex handles?) > > > > I'd just summarize this as: > > // |delegate| is leaked. > > You would summarize the whole "|delegate| is created ..." part? Pretty much... suggested alternative: // Sets a delegate to provide platform-specific logic. This should be // set during startup (to ensure all extensions are initialized through // the delegate). // |delegate| is a singleton instance and is leaked. Feel free to wordsmith, though.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
lgtm
https://codereview.chromium.org/2794803003/diff/180001/chrome/browser/extensi... File chrome/browser/extensions/permissions_updater.h (right): https://codereview.chromium.org/2794803003/diff/180001/chrome/browser/extensi... chrome/browser/extensions/permissions_updater.h:56: // freed but this is fine since it will go away together with the browser On 2017/04/11 15:15:02, Devlin wrote: > On 2017/04/11 15:11:16, Ivan Šandrk wrote: > > On 2017/04/11 15:06:22, Devlin wrote: > > > I'm not quite sure I follow the "this is fine since it will go away together > > > with the browser process". Isn't this just leaked? Doesn't any allocated > > > object go away with the process its in (unless its e.g. persisted to disk, > or > > > we're talking about very complex handles?) > > > > > > I'd just summarize this as: > > > // |delegate| is leaked. > > > > You would summarize the whole "|delegate| is created ..." part? > > Pretty much... suggested alternative: > // Sets a delegate to provide platform-specific logic. This should be > // set during startup (to ensure all extensions are initialized through > // the delegate). > // |delegate| is a singleton instance and is leaked. > > Feel free to wordsmith, though. Done.
The CQ bit was checked by isandrk@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from alemate@chromium.org, rdevlin.cronin@chromium.org, atwilson@chromium.org Link to the patchset: https://codereview.chromium.org/2794803003/#ps200001 (title: "Updated comment")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 200001, "attempt_start_ts": 1492005497050460,
"parent_rev": "49766ea841fd7c75647e4ffb0659dc344d8d8947", "commit_rev":
"80e3eb904ef28e2491b831099597fc05347845ae"}
Message was sent while issue was closed.
Description was changed from ========== PS - Remove Clipboard Read permission from extensions in Public Sessions (except for whitelisted ones) In Public Sessions, apps and extensions are force-installed by admin policy so the user does not get a chance to review the permissions for these apps. This is not acceptable from a security standpoint, so we remove ClipboardRead permission from them (except for whitelisted ones - eg. remote desktop clients). This forceful removal of permission is safe since the clipboard pasting code checks for this permission before doing the paste. TEST= unit_tests --gtest_filter=DeviceLocalAccountManagementPolicyProviderTest.IsWhitelisted unit_tests --gtest_filter=PermissionsUpdaterDelegateChromeOSTest.* unit_tests --gtest_filter=PermissionsUpdaterTest.Delegate BUG=707864 ========== to ========== PS - Remove Clipboard Read permission from extensions in Public Sessions (except for whitelisted ones) In Public Sessions, apps and extensions are force-installed by admin policy so the user does not get a chance to review the permissions for these apps. This is not acceptable from a security standpoint, so we remove ClipboardRead permission from them (except for whitelisted ones - eg. remote desktop clients). This forceful removal of permission is safe since the clipboard pasting code checks for this permission before doing the paste. TEST= unit_tests --gtest_filter=DeviceLocalAccountManagementPolicyProviderTest.IsWhitelisted unit_tests --gtest_filter=PermissionsUpdaterDelegateChromeOSTest.* unit_tests --gtest_filter=PermissionsUpdaterTest.Delegate BUG=707864 Review-Url: https://codereview.chromium.org/2794803003 Cr-Commit-Position: refs/heads/master@{#464031} Committed: https://chromium.googlesource.com/chromium/src/+/80e3eb904ef28e2491b831099597... ==========
Message was sent while issue was closed.
Committed patchset #8 (id:200001) as https://chromium.googlesource.com/chromium/src/+/80e3eb904ef28e2491b831099597... |
