Add a policy to disable mounting of external storage.

Add a policy to disable mounting of external storage.

BUG=chromium-os:32338
TEST=unit tests. Manual testing can be done by setting the new policy in a DM server to true and verifying that external storage is not mount and shown in the file manager in Chrome.


Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=148065

[pastarmovj, 2012-07-06 14:27:27]

Hi guys,
this is my proposed solution for a policy controlling external media. It will
prevent mounting anything as long as the policy is set and even try to unmount
drives if the policy gets active in the middle of a session.

I tested it on an Alex device with an USB stick and SD card and in both cases
instead of a file manager you get to see a neat message telling you to go kill
your admin for that restriction.

This is an user policy which means it can be configured differently for every
user too.

[Mattias Nissler (ping if slow), 2012-07-09 09:43:01]

http://codereview.chromium.org/10697097/diff/2001/chrome/app/policy/policy_te...
File chrome/app/policy/policy_templates.json (right):

http://codereview.chromium.org/10697097/diff/2001/chrome/app/policy/policy_te...
chrome/app/policy/policy_templates.json:2767: This policy affects all types of
storage media. For example: USB sticks, external hard drives, SD and other
memory cards, optical storage, perfocards etc.
s/USB sticks/USB flash drives/, which seems a more commonly used name for the
thing. What is a perfocard?

http://codereview.chromium.org/10697097/diff/2001/chrome/browser/chromeos/ext...
File chrome/browser/chromeos/extensions/file_browser_event_router.cc (right):

http://codereview.chromium.org/10697097/diff/2001/chrome/browser/chromeos/ext...
chrome/browser/chromeos/extensions/file_browser_event_router.cc:351: // If the
policy got just disabled we have to unmount every device currently
s/got just/just got/

http://codereview.chromium.org/10697097/diff/2001/chrome/browser/chromeos/ext...
chrome/browser/chromeos/extensions/file_browser_event_router.cc:352: // mounted.
The opposite is fine - we can let the user re-plug its device to
s/its/her/

http://codereview.chromium.org/10697097/diff/2001/chrome/browser/chromeos/ext...
chrome/browser/chromeos/extensions/file_browser_event_router.cc:353: // get it
available.
s/get/make/

http://codereview.chromium.org/10697097/diff/2001/chrome/browser/chromeos/ext...
chrome/browser/chromeos/extensions/file_browser_event_router.cc:358: while
(!mounts.empty()) {
seems like a simple loop instead of a while(!empty) { erase() } would work as
well and is less confusing.

http://codereview.chromium.org/10697097/diff/2001/chrome/browser/chromeos/ext...
chrome/browser/chromeos/extensions/file_browser_event_router.cc:367:
profile_->GetExtensionEventRouter()->DispatchEventToRenderers(
This should probably be guarded in a type/pref name check, now that you listen
for multiple notifications.

http://codereview.chromium.org/10697097/diff/2001/chrome/common/pref_names.cc
File chrome/common/pref_names.cc (right):

http://codereview.chromium.org/10697097/diff/2001/chrome/common/pref_names.cc...
chrome/common/pref_names.cc:1771: // A pref holding the value of the policy used
to disable moutning of external
s/moutning/mounting/

http://codereview.chromium.org/10697097/diff/2001/chrome/common/pref_names.cc...
chrome/common/pref_names.cc:1773: const char kExternalStorageDisabled[] =
"hardware.extrenal_storage_disabled";
s/extrenal/external/

http://codereview.chromium.org/10697097/diff/2001/chrome/test/functional/poli...
File chrome/test/functional/policy_test_cases.py (right):

http://codereview.chromium.org/10697097/diff/2001/chrome/test/functional/poli...
chrome/test/functional/policy_test_cases.py:260: 'ExternalStorageDisabled':
(None, [], [], ['chromeos']),
the second parameter is the value, which should be a boolean in this case.

[pastarmovj, 2012-07-11 01:13:56]

Thanks for reviewing this one! PTAL.

http://codereview.chromium.org/10697097/diff/2001/chrome/app/policy/policy_te...
File chrome/app/policy/policy_templates.json (right):

http://codereview.chromium.org/10697097/diff/2001/chrome/app/policy/policy_te...
chrome/app/policy/policy_templates.json:2767: This policy affects all types of
storage media. For example: USB sticks, external hard drives, SD and other
memory cards, optical storage, perfocards etc.
On 2012/07/09 09:43:01, Mattias Nissler wrote:
> s/USB sticks/USB flash drives/, which seems a more commonly used name for the
> thing. What is a perfocard?

Sorry i meant to write punch cards but ended up with the less common name :).

http://codereview.chromium.org/10697097/diff/2001/chrome/browser/chromeos/ext...
File chrome/browser/chromeos/extensions/file_browser_event_router.cc (right):

http://codereview.chromium.org/10697097/diff/2001/chrome/browser/chromeos/ext...
chrome/browser/chromeos/extensions/file_browser_event_router.cc:351: // If the
policy got just disabled we have to unmount every device currently
On 2012/07/09 09:43:01, Mattias Nissler wrote:
> s/got just/just got/

Done.

http://codereview.chromium.org/10697097/diff/2001/chrome/browser/chromeos/ext...
chrome/browser/chromeos/extensions/file_browser_event_router.cc:352: // mounted.
The opposite is fine - we can let the user re-plug its device to
On 2012/07/09 09:43:01, Mattias Nissler wrote:
> s/its/her/

Done.

http://codereview.chromium.org/10697097/diff/2001/chrome/browser/chromeos/ext...
chrome/browser/chromeos/extensions/file_browser_event_router.cc:353: // get it
available.
On 2012/07/09 09:43:01, Mattias Nissler wrote:
> s/get/make/

Done.

http://codereview.chromium.org/10697097/diff/2001/chrome/browser/chromeos/ext...
chrome/browser/chromeos/extensions/file_browser_event_router.cc:358: while
(!mounts.empty()) {
On 2012/07/09 09:43:01, Mattias Nissler wrote:
> seems like a simple loop instead of a while(!empty) { erase() } would work as
> well and is less confusing.

Done.

http://codereview.chromium.org/10697097/diff/2001/chrome/browser/chromeos/ext...
chrome/browser/chromeos/extensions/file_browser_event_router.cc:367:
profile_->GetExtensionEventRouter()->DispatchEventToRenderers(
On 2012/07/09 09:43:01, Mattias Nissler wrote:
> This should probably be guarded in a type/pref name check, now that you listen
> for multiple notifications.

I don't think so it used to catch all notifications before and I added one which
I handle above and return so I think the catch all the rest semantics is fine
here.

http://codereview.chromium.org/10697097/diff/2001/chrome/common/pref_names.cc
File chrome/common/pref_names.cc (right):

http://codereview.chromium.org/10697097/diff/2001/chrome/common/pref_names.cc...
chrome/common/pref_names.cc:1771: // A pref holding the value of the policy used
to disable moutning of external
On 2012/07/09 09:43:01, Mattias Nissler wrote:
> s/moutning/mounting/

Done.

http://codereview.chromium.org/10697097/diff/2001/chrome/common/pref_names.cc...
chrome/common/pref_names.cc:1773: const char kExternalStorageDisabled[] =
"hardware.extrenal_storage_disabled";
On 2012/07/09 09:43:01, Mattias Nissler wrote:
> s/extrenal/external/

Done.

http://codereview.chromium.org/10697097/diff/2001/chrome/test/functional/poli...
File chrome/test/functional/policy_test_cases.py (right):

http://codereview.chromium.org/10697097/diff/2001/chrome/test/functional/poli...
chrome/test/functional/policy_test_cases.py:260: 'ExternalStorageDisabled':
(None, [], [], ['chromeos']),
On 2012/07/09 09:43:01, Mattias Nissler wrote:
> the second parameter is the value, which should be a boolean in this case.

Done.

[Mattias Nissler (ping if slow), 2012-07-11 09:48:12]

Cyrus, please take a look at the first comment and let us know whether the
wording of the error message is fine with you.

http://codereview.chromium.org/10697097/diff/2002/chrome/app/generated_resour...
File chrome/app/generated_resources.grd (right):

http://codereview.chromium.org/10697097/diff/2002/chrome/app/generated_resour...
chrome/app/generated_resources.grd:15186: +       Sorry, your administrator has
disabled external storage on your account.
Cyrus, please let us know if you're happy with this wording or if you'd like
something else.

http://codereview.chromium.org/10697097/diff/2002/chrome/app/policy/policy_te...
File chrome/app/policy/policy_templates.json (right):

http://codereview.chromium.org/10697097/diff/2002/chrome/app/policy/policy_te...
chrome/app/policy/policy_templates.json:2767: This policy affects all types of
storage media. For example: USB flash drives, external hard drives, SD and other
memory cards, optical storage, punch cards etc.
I don't think admins will get joke here and some have already been picky about
the description strings in the past, so if I were you, I'd remove the punch
cards here and put them into a code comment if you like to keep them.

http://codereview.chromium.org/10697097/diff/2002/chrome/browser/chromeos/ext...
File chrome/browser/chromeos/extensions/file_browser_event_router.cc (right):

http://codereview.chromium.org/10697097/diff/2002/chrome/browser/chromeos/ext...
chrome/browser/chromeos/extensions/file_browser_event_router.cc:366: }
I'd still list the preferences that are supposed to trigger the code below
explicitly, so people coming after you have an easier time figuring out under
which conditions this code runs.

[cyrusm_google.com, 2012-07-11 18:33:18]

LGTM

On Wed, Jul 11, 2012 at 5:48 AM, <mnissler@chromium.org> wrote:

> Cyrus, please take a look at the first comment and let us know whether the
> wording of the error message is fine with you.
>
>
> http://codereview.chromium.**org/10697097/diff/2002/chrome/**
>
app/generated_resources.grd<http://codereview.chromium.org/10697097/diff/2002/chrome/app/generated_resources.grd>
> File chrome/app/generated_**resources.grd (right):
>
> http://codereview.chromium.**org/10697097/diff/2002/chrome/**
>
app/generated_resources.grd#**newcode15186<http://codereview.chromium.org/10697097/diff/2002/chrome/app/generated_resources.grd#newcode15186>
> chrome/app/generated_**resources.grd:15186: +       Sorry, your
> administrator has disabled external storage on your account.
> Cyrus, please let us know if you're happy with this wording or if you'd
> like something else.
>
> http://codereview.chromium.**org/10697097/diff/2002/chrome/**
>
app/policy/policy_templates.**json<http://codereview.chromium.org/10697097/diff/2002/chrome/app/policy/policy_templates.json>
> File chrome/app/policy/policy_**templates.json (right):
>
> http://codereview.chromium.**org/10697097/diff/2002/chrome/**
>
app/policy/policy_templates.**json#newcode2767<http://codereview.chromium.org/10697097/diff/2002/chrome/app/policy/policy_templates.json#newcode2767>
> chrome/app/policy/policy_**templates.json:2767: This policy affects all
> types of storage media. For example: USB flash drives, external hard
> drives, SD and other memory cards, optical storage, punch cards etc.
> I don't think admins will get joke here and some have already been picky
> about the description strings in the past, so if I were you, I'd remove
> the punch cards here and put them into a code comment if you like to
> keep them.
>
> http://codereview.chromium.**org/10697097/diff/2002/chrome/**
>
browser/chromeos/extensions/**file_browser_event_router.cc<http://codereview.chromium.org/10697097/diff/2002/chrome/browser/chromeos/extensions/file_browser_event_router.cc>
> File chrome/browser/chromeos/**extensions/file_browser_event_**router.cc
> (right):
>
> http://codereview.chromium.**org/10697097/diff/2002/chrome/**
>
browser/chromeos/extensions/**file_browser_event_router.cc#**newcode366<http://codereview.chromium.org/10697097/diff/2002/chrome/browser/chromeos/extensions/file_browser_event_router.cc#newcode366>
> chrome/browser/chromeos/**extensions/file_browser_event_**router.cc:366: }
> I'd still list the preferences that are supposed to trigger the code
> below explicitly, so people coming after you have an easier time
> figuring out under which conditions this code runs.
>
>
http://codereview.chromium.**org/10697097/<http://codereview.chromium.org/106...
>

[pastarmovj, 2012-07-13 15:06:06]

@Mattias: PTAL. :)

@Zel: Ping?

http://codereview.chromium.org/10697097/diff/2002/chrome/app/policy/policy_te...
File chrome/app/policy/policy_templates.json (right):

http://codereview.chromium.org/10697097/diff/2002/chrome/app/policy/policy_te...
chrome/app/policy/policy_templates.json:2767: This policy affects all types of
storage media. For example: USB flash drives, external hard drives, SD and other
memory cards, optical storage, punch cards etc.
On 2012/07/11 09:48:13, Mattias Nissler wrote:
> I don't think admins will get joke here and some have already been picky about
> the description strings in the past, so if I were you, I'd remove the punch
> cards here and put them into a code comment if you like to keep them.

Done.

http://codereview.chromium.org/10697097/diff/2002/chrome/browser/chromeos/ext...
File chrome/browser/chromeos/extensions/file_browser_event_router.cc (right):

http://codereview.chromium.org/10697097/diff/2002/chrome/browser/chromeos/ext...
chrome/browser/chromeos/extensions/file_browser_event_router.cc:366: }
On 2012/07/11 09:48:13, Mattias Nissler wrote:
> I'd still list the preferences that are supposed to trigger the code below
> explicitly, so people coming after you have an easier time figuring out under
> which conditions this code runs.

Done.

[zel, 2012-07-16 20:11:39]

lgtm

[Mattias Nissler (ping if slow), 2012-07-17 08:26:30]

LGTM w/ a nit

http://codereview.chromium.org/10697097/diff/1012/chrome/app/policy/policy_te...
File chrome/app/policy/policy_templates.json (right):

http://codereview.chromium.org/10697097/diff/1012/chrome/app/policy/policy_te...
chrome/app/policy/policy_templates.json:2767: This policy affects all types of
storage media. For example: USB flash drives, external hard drives, SD and other
memory cards, optical storage etc.
nit: either join the lines are put a blank line for paragraph breaking (which is
the de-facto style found in this file).

[commit-bot: I haz the power, 2012-07-24 06:20:32]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/pastarmovj@chromium.org/10697097/20001

[commit-bot: I haz the power, 2012-07-24 06:20:34]

Failed to apply patch for chrome/app/policy/policy_templates.json:
While running patch -p1 --forward --force;
patching file chrome/app/policy/policy_templates.json
Hunk #1 FAILED at 112.
Hunk #2 succeeded at 2756 with fuzz 2 (offset 4 lines).
1 out of 2 hunks FAILED -- saving rejects to file
chrome/app/policy/policy_templates.json.rej

[commit-bot: I haz the power, 2012-07-24 07:19:44]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/pastarmovj@chromium.org/10697097/24001

[commit-bot: I haz the power, 2012-07-24 08:46:15]

Change committed as 148065