Allow enhanced bookmarks external component extensions to be disabled

Allow enhanced bookmarks external component extensions to be disabled

Allow enhanced bookmarks external component extensions to be
disabled/uninstalled. This facilitates ongoing development and testing
of multiple versions of extensions that are ultimately released as
external component extensions.

BUG=401315
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=289270

[Mike Wittman, 2014-08-07 00:29:16]

Hi Antony, please take a look.

I considered adding a new flag for this, but it since
--show-component-extension-options must be specified to see the extensions
anyway, it seems like we might as well just reuse that instead of having a
second flag conditional on it.

[asargent_no_longer_on_chrome, 2014-08-07 20:14:39]

Do you need this to work in official builds, or would it be sufficient to make
it only work in local (eg chromium) builds? We've become increasingly concerned
about malware abusing various chrome flags, and I can imagine some cases where
we have an external component extension installed that provides some security
benefits which malware authors would want to turn off via this mechanism.

[Mike Wittman, 2014-08-07 21:18:18]

On 2014/08/07 20:14:39, Antony Sargent wrote:
> Do you need this to work in official builds, or would it be sufficient to make
> it only work in local (eg chromium) builds? We've become increasingly
concerned
> about malware abusing various chrome flags, and I can imagine some cases where
> we have an external component extension installed that provides some security
> benefits which malware authors would want to turn off via this mechanism.

Local-only builds would not be sufficient for development of the enhanced
bookmarks extension since most developers/testers do not build Chromium. I'm
open to other methods to achieve the same end though: maybe only allow extension
disables generated by user interaction with chrome://extensions (if practical),
or whitelist the extension we care about?

[asargent_no_longer_on_chrome, 2014-08-07 22:26:17]

On 2014/08/07 21:18:18, Mike Wittman wrote:
> On 2014/08/07 20:14:39, Antony Sargent wrote:
> > Do you need this to work in official builds, or would it be sufficient to
make
> > it only work in local (eg chromium) builds? We've become increasingly
> concerned
> > about malware abusing various chrome flags, and I can imagine some cases
where
> > we have an external component extension installed that provides some
security
> > benefits which malware authors would want to turn off via this mechanism.
> 
> Local-only builds would not be sufficient for development of the enhanced
> bookmarks extension since most developers/testers do not build Chromium. I'm
> open to other methods to achieve the same end though: maybe only allow
extension
> disables generated by user interaction with chrome://extensions (if
practical),
> or whitelist the extension we care about?

Would you want the disable/uninstall to persist across restarts of the browser,
or would having it only last for the duration of the running chrome session be
sufficient? (ie you'd need to go to chrome://extensions and manually
disable/uninstall once for each run of the browser)

Also, it seems as if you can use the load unpacked feature to point at a local
folder containing a copy of the extension with the same id (as determined by
copying the "key" field that gets inserted into the manfiest at install time
into your profile dir / Preferences file) to temporarily load an updated version
of a component extension. Would this be sufficient for your needs, or is it
important that you're testing the crx install experience as well?

[Mike Wittman, 2014-08-07 23:22:18]

On 2014/08/07 22:26:17, Antony Sargent wrote:
> Would you want the disable/uninstall to persist across restarts of the
browser,
> or would having it only last for the duration of the running chrome session be
> sufficient? (ie you'd need to go to chrome://extensions and manually
> disable/uninstall once for each run of the browser)

We'd want it to be persistent across restarts of the browser. Developers and
testers are most often running a version of the extension other than the
external component one, so having to constantly disable the external component
extension would be very cumbersome.

> Also, it seems as if you can use the load unpacked feature to point at a local
> folder containing a copy of the extension with the same id (as determined by
> copying the "key" field that gets inserted into the manfiest at install time
> into your profile dir / Preferences file) to temporarily load an updated
version
> of a component extension. Would this be sufficient for your needs, or is it
> important that you're testing the crx install experience as well?

Testing the crx install experience is mildly important, but a more significant
issue is that we have multiple versions of the extension with different
keys/IDs: the (prod) external component extension, a nightly build, and
developer-built extensions. We distribute the first two through the CWS and I
don't see that this approach can work for that case. Also, it would require all
developers to have access to and build with the prod extension key, which we'd
like to avoid.

[asargent_no_longer_on_chrome, 2014-08-07 23:50:18]

+cc mek in case he has any thoughts, since he wrote the external component code

Ok, in thinking about this some more, we probably already need to tolerate the
absence of an external component extension, since some (all?) of them start out
not installed until we can talk to the webstore, so I think supporting uninstall
probably works out ok. I'm not as sure about disable - there may be some places
in the code that assume that a component extension cannot be disabled, but you
can see if it just happens to work. 

To be conservative though, maybe we should modify things so that each consumer
gets to decide if their component extension can be disabled if the command line
flag is present. For a concrete implementation, we can add a pref key name
something like "external_component_allows_disable", which, if present, allows
external component extensions to be disabled/uninstalled. Then that can be set
in  ExternalComponentLoader::StartLoading() for hotwording (this is for
hotwording right?).

[Mike Wittman, 2014-08-08 00:42:59]

On 2014/08/07 23:50:18, Antony Sargent wrote:
> Ok, in thinking about this some more, we probably already need to tolerate the
> absence of an external component extension, since some (all?) of them start
out
> not installed until we can talk to the webstore, so I think supporting
uninstall
> probably works out ok. I'm not as sure about disable - there may be some
places
> in the code that assume that a component extension cannot be disabled, but you
> can see if it just happens to work. 

Until r281251 it was possible to disable external component extensions, and our
developers were doing so on our extension with no apparent ill effects.

> To be conservative though, maybe we should modify things so that each consumer
> gets to decide if their component extension can be disabled if the command
line
> flag is present. For a concrete implementation, we can add a pref key name
> something like "external_component_allows_disable", which, if present, allows
> external component extensions to be disabled/uninstalled. Then that can be set
> in  ExternalComponentLoader::StartLoading() for hotwording (this is for
> hotwording right?).

This is for the enhanced bookmarks extension (aka "BookmarksExpermiment" in
ExternalComponentLoader::StartLoading()). Setting a pref per-extension sounds
like a reasonable approach to me.

[Mike Wittman, 2014-08-08 23:47:31]

On 2014/08/08 00:42:59, Mike Wittman wrote:
> On 2014/08/07 23:50:18, Antony Sargent wrote:
> > To be conservative though, maybe we should modify things so that each
consumer
> > gets to decide if their component extension can be disabled if the command
> line
> > flag is present. For a concrete implementation, we can add a pref key name
> > something like "external_component_allows_disable", which, if present,
allows
> > external component extensions to be disabled/uninstalled. Then that can be
set
> > in  ExternalComponentLoader::StartLoading() for hotwording (this is for
> > hotwording right?).
> 
> This is for the enhanced bookmarks extension (aka "BookmarksExpermiment" in
> ExternalComponentLoader::StartLoading()). Setting a pref per-extension sounds
> like a reasonable approach to me.

This approach turns out to be not quite as simple as hoped; the "prefs" that are
set in ExternalComponentLoader::StartLoading() are nothing more than a means of
communicating selected information to extensions::ExternalProviderImpl. They're
never actually stored.

Would it be acceptable to (ab)use the existing "install_parameter" preference
for this case? If not, it looks like I'll need to thread another parameter
through the extension installation call stack including ExternalProviderImpl,
ExtensionService, PendingExtensionManager, PendingExtensionInfo, ExtensionPrefs,
and associated tests.

[asargent_no_longer_on_chrome, 2014-08-11 21:24:13]

On 2014/08/08 23:47:31, Mike Wittman wrote:
> On 2014/08/08 00:42:59, Mike Wittman wrote:
> > On 2014/08/07 23:50:18, Antony Sargent wrote:
> > > To be conservative though, maybe we should modify things so that each
> consumer
> > > gets to decide if their component extension can be disabled if the command
> > line
> > > flag is present. For a concrete implementation, we can add a pref key name
> > > something like "external_component_allows_disable", which, if present,
> allows
> > > external component extensions to be disabled/uninstalled. Then that can be
> set
> > > in  ExternalComponentLoader::StartLoading() for hotwording (this is for
> > > hotwording right?).
> > 
> > This is for the enhanced bookmarks extension (aka "BookmarksExpermiment" in
> > ExternalComponentLoader::StartLoading()). Setting a pref per-extension
sounds
> > like a reasonable approach to me.
> 
> This approach turns out to be not quite as simple as hoped; the "prefs" that
are
> set in ExternalComponentLoader::StartLoading() are nothing more than a means
of
> communicating selected information to extensions::ExternalProviderImpl.
They're
> never actually stored.
> 
> Would it be acceptable to (ab)use the existing "install_parameter" preference
> for this case? If not, it looks like I'll need to thread another parameter
> through the extension installation call stack including ExternalProviderImpl,
> ExtensionService, PendingExtensionManager, PendingExtensionInfo,
ExtensionPrefs,
> and associated tests.

I'd rather we not overload the install_paramter - I just looked traced through
the code and had a hard time even understanding what that is used for in the
first place. 

Maybe a simpler approach is to just add methods on ExternalComponentLoader like
"canBeDisabled" and "canBeUninstalled" that take a std::string? Then you can
call that from the same place in admin_policy.cc as your current patch?

[rpetterson, 2014-08-11 22:11:12]

On 2014/08/11 21:24:13, Antony Sargent wrote:
> On 2014/08/08 23:47:31, Mike Wittman wrote:
> > On 2014/08/08 00:42:59, Mike Wittman wrote:
> > > On 2014/08/07 23:50:18, Antony Sargent wrote:
> > > > To be conservative though, maybe we should modify things so that each
> > consumer
> > > > gets to decide if their component extension can be disabled if the
command
> > > line
> > > > flag is present. For a concrete implementation, we can add a pref key
name
> > > > something like "external_component_allows_disable", which, if present,
> > allows
> > > > external component extensions to be disabled/uninstalled. Then that can
be
> > set
> > > > in  ExternalComponentLoader::StartLoading() for hotwording (this is for
> > > > hotwording right?).
> > > 
> > > This is for the enhanced bookmarks extension (aka "BookmarksExpermiment"
in
> > > ExternalComponentLoader::StartLoading()). Setting a pref per-extension
> sounds
> > > like a reasonable approach to me.
> > 
> > This approach turns out to be not quite as simple as hoped; the "prefs" that
> are
> > set in ExternalComponentLoader::StartLoading() are nothing more than a means
> of
> > communicating selected information to extensions::ExternalProviderImpl.
> They're
> > never actually stored.
> > 
> > Would it be acceptable to (ab)use the existing "install_parameter"
preference
> > for this case? If not, it looks like I'll need to thread another parameter
> > through the extension installation call stack including
ExternalProviderImpl,
> > ExtensionService, PendingExtensionManager, PendingExtensionInfo,
> ExtensionPrefs,
> > and associated tests.
> 
> I'd rather we not overload the install_paramter - I just looked traced through
> the code and had a hard time even understanding what that is used for in the
> first place. 
> 
> Maybe a simpler approach is to just add methods on ExternalComponentLoader
like
> "canBeDisabled" and "canBeUninstalled" that take a std::string? Then you can
> call that from the same place in admin_policy.cc as your current patch?

Just to clarify, this is for bookmarks, not hotwording. Hotwording had
previously introduced a bug which made this possible so now Mike is trying to do
it the Right Way.

Would the canBeDisabled/canBeUninstalled be for all external component
extensions or are you suggesting keeping a map to specific extensions?

[asargent_no_longer_on_chrome, 2014-08-11 22:24:10]

I was thinking those methods would take in an id, and the code inside
of ExternalComponentLoader
would return yes/no depending on whether we wanted to support it for that
extension or not.



On Mon, Aug 11, 2014 at 3:11 PM, <rlp@chromium.org> wrote:

> On 2014/08/11 21:24:13, Antony Sargent wrote:
>
>> On 2014/08/08 23:47:31, Mike Wittman wrote:
>> > On 2014/08/08 00:42:59, Mike Wittman wrote:
>> > > On 2014/08/07 23:50:18, Antony Sargent wrote:
>> > > > To be conservative though, maybe we should modify things so that
>> each
>> > consumer
>> > > > gets to decide if their component extension can be disabled if the
>>
> command
>
>> > > line
>> > > > flag is present. For a concrete implementation, we can add a pref
>> key
>>
> name
>
>> > > > something like "external_component_allows_disable", which, if
>> present,
>> > allows
>> > > > external component extensions to be disabled/uninstalled. Then that
>> can
>>
> be
>
>> > set
>> > > > in  ExternalComponentLoader::StartLoading() for hotwording (this
>> is for
>> > > > hotwording right?).
>> > >
>> > > This is for the enhanced bookmarks extension (aka
>> "BookmarksExpermiment"
>>
> in
>
>> > > ExternalComponentLoader::StartLoading()). Setting a pref
>> per-extension
>> sounds
>> > > like a reasonable approach to me.
>> >
>> > This approach turns out to be not quite as simple as hoped; the "prefs"
>> that
>> are
>> > set in ExternalComponentLoader::StartLoading() are nothing more than a
>> means
>> of
>> > communicating selected information to extensions::ExternalProviderImpl.
>> They're
>> > never actually stored.
>> >
>> > Would it be acceptable to (ab)use the existing "install_parameter"
>>
> preference
>
>> > for this case? If not, it looks like I'll need to thread another
>> parameter
>> > through the extension installation call stack including
>>
> ExternalProviderImpl,
>
>> > ExtensionService, PendingExtensionManager, PendingExtensionInfo,
>> ExtensionPrefs,
>> > and associated tests.
>>
>
>  I'd rather we not overload the install_paramter - I just looked traced
>> through
>> the code and had a hard time even understanding what that is used for in
>> the
>> first place.
>>
>
>  Maybe a simpler approach is to just add methods on ExternalComponentLoader
>>
> like
>
>> "canBeDisabled" and "canBeUninstalled" that take a std::string? Then you
>> can
>> call that from the same place in admin_policy.cc as your current patch?
>>
>
> Just to clarify, this is for bookmarks, not hotwording. Hotwording had
> previously introduced a bug which made this possible so now Mike is trying
> to do
> it the Right Way.
>
> Would the canBeDisabled/canBeUninstalled be for all external component
> extensions or are you suggesting keeping a map to specific extensions?
>
> https://codereview.chromium.org/445233002/
>

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[Mike Wittman, 2014-08-11 22:45:53]

On 2014/08/11 22:24:10, Antony Sargent wrote:
> I was thinking those methods would take in an id, and the code inside
> of ExternalComponentLoader
> would return yes/no depending on whether we wanted to support it for that
> extension or not.

That would work except that it's a DEPS violation (including chrome/ from
extensions/). If desirable, I can investigate configuring the existing
ManagementPolicyImpl from chrome code. Otherwise we could just check the IDs
directly in ManagementPolicyImpl.

[Mike Wittman, 2014-08-12 19:25:44]

Uploaded a provisional patch for whitelisting directly in ManagementPolicyImpl,
looking into what's required to place this code in chrome/.

[Mike Wittman, 2014-08-12 20:02:28]

On 2014/08/12 19:25:44, Mike Wittman wrote:
> Uploaded a provisional patch for whitelisting directly in
ManagementPolicyImpl,
> looking into what's required to place this code in chrome/.

Moved whitelist into ExternalComponentLoader, with check alongside admin_policy.
This seemed a reasonable way to do this within chrome/. Please take a look.

[asargent_no_longer_on_chrome, 2014-08-12 23:27:35]

lgtm (and sorry for all the back and forth)

https://codereview.chromium.org/445233002/diff/80001/chrome/browser/extension...
File chrome/browser/extensions/standard_management_policy_provider.cc (right):

https://codereview.chromium.org/445233002/diff/80001/chrome/browser/extension...
chrome/browser/extensions/standard_management_policy_provider.cc:67: }
nit: can you add the check for extension->location() ==
Manifest::EXTERNAL_COMPONENT here as well?

[Mike Wittman, 2014-08-13 01:25:13]

https://codereview.chromium.org/445233002/diff/80001/chrome/browser/extension...
File chrome/browser/extensions/standard_management_policy_provider.cc (right):

https://codereview.chromium.org/445233002/diff/80001/chrome/browser/extension...
chrome/browser/extensions/standard_management_policy_provider.cc:67: }
On 2014/08/12 23:27:35, Antony Sargent wrote:
> nit: can you add the check for extension->location() ==
> Manifest::EXTERNAL_COMPONENT here as well?

Done.

[Mike Wittman, 2014-08-13 01:25:23]

The CQ bit was checked by wittman@chromium.org

[commit-bot: I haz the power, 2014-08-13 01:32:58]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/wittman@chromium.org/445233002/100001

[commit-bot: I haz the power, 2014-08-13 01:52:45]

FYI, CQ is re-trying this CL (attempt #1).
The failing builders are:
  linux_gpu on tryserver.chromium.gpu
(http://build.chromium.org/p/tryserver.chromium.gpu/builders/linux_gpu/builds/...)
  mac_gpu on tryserver.chromium.gpu
(http://build.chromium.org/p/tryserver.chromium.gpu/builders/mac_gpu/builds/40884)
  win_gpu on tryserver.chromium.gpu
(http://build.chromium.org/p/tryserver.chromium.gpu/builders/win_gpu/builds/46109)
  android_aosp on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/android_aosp/bu...)
  android_chromium_gn_compile_rel on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/android_chromiu...)
  android_clang_dbg on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/android_clang_d...)
  android_dbg on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/android_dbg/bui...)
  chromium_presubmit on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
  linux_chromium_chromeos_rel_swarming on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_gn_rel on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_rel_swarming on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  ios_dbg_simulator on tryserver.chromium.mac
(http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_dbg_simulator...)
  ios_rel_device on tryserver.chromium.mac
(http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_rel_device/bu...)
  ios_rel_device_ninja on tryserver.chromium.mac
(http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_rel_device_ni...)
  mac_chromium_compile_dbg on tryserver.chromium.mac
(http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
  mac_chromium_rel_swarming on tryserver.chromium.mac
(http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
  win8_chromium_rel on tryserver.chromium.win
(http://build.chromium.org/p/tryserver.chromium.win/builders/win8_chromium_rel...)
  win_chromium_compile_dbg on tryserver.chromium.win
(http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_comp...)
  win_chromium_rel_swarming on tryserver.chromium.win
(http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)
  win_chromium_x64_rel on tryserver.chromium.win
(http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)

[commit-bot: I haz the power, 2014-08-13 01:56:39]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-08-13 01:56:44]

Try jobs failed on following builders:
  linux_gpu on tryserver.chromium.gpu
(http://build.chromium.org/p/tryserver.chromium.gpu/builders/linux_gpu/builds/...)
  mac_gpu on tryserver.chromium.gpu
(http://build.chromium.org/p/tryserver.chromium.gpu/builders/mac_gpu/builds/40892)
  win_gpu on tryserver.chromium.gpu
(http://build.chromium.org/p/tryserver.chromium.gpu/builders/win_gpu/builds/46117)
  android_aosp on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/android_aosp/bu...)
  android_chromium_gn_compile_rel on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/android_chromiu...)
  android_clang_dbg on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/android_clang_d...)
  android_dbg on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/android_dbg/bui...)
  chromium_presubmit on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
  linux_chromium_chromeos_rel_swarming on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_gn_rel on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_rel_swarming on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  ios_dbg_simulator on tryserver.chromium.mac
(http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_dbg_simulator...)
  ios_rel_device on tryserver.chromium.mac
(http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_rel_device/bu...)
  ios_rel_device_ninja on tryserver.chromium.mac
(http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_rel_device_ni...)
  mac_chromium_compile_dbg on tryserver.chromium.mac
(http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
  mac_chromium_rel_swarming on tryserver.chromium.mac
(http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
  win8_chromium_rel on tryserver.chromium.win
(http://build.chromium.org/p/tryserver.chromium.win/builders/win8_chromium_rel...)

[Mike Wittman, 2014-08-13 07:28:22]

The CQ bit was checked by wittman@chromium.org

[commit-bot: I haz the power, 2014-08-13 07:30:30]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/wittman@chromium.org/445233002/120001

[commit-bot: I haz the power, 2014-08-13 12:15:08]

Change committed as 289270