Add ExternalStorageReadOnly profile value.

Add ExternalStorageReadOnly profile value.

TEST=policy_test_cases
BUG=629945
TBR=isherman@chromium.org

Committed: https://crrev.com/36aeef7a0a017f4fbaafba82578a20aaf4bd3e00
Cr-Commit-Position: refs/heads/master@{#413998}

[yamaguchi, 2016-08-22 07:18:57]

yamaguchi@chromium.org changed reviewers:
+ atwilson@chromium.org, bartfab@chromium.org

[yamaguchi, 2016-08-22 07:18:58]

PTAL.
The change that uses the policy was submitted by:
https://codereview.chromium.org/2201023002/patch/190001/200008

[Andrew T Wilson (Slow), 2016-08-22 13:21:40]

LGTM with a few nits.

https://codereview.chromium.org/2267653002/diff/1/components/policy/resources...
File components/policy/resources/policy_templates.json (right):

https://codereview.chromium.org/2267653002/diff/1/components/policy/resources...
components/policy/resources/policy_templates.json:5634: 'caption': '''Limit
mounting of external storage to read-only mode.''',
I don't think admins necessarily know what "mounting to read-only mode" is.
Perhaps change this (and the description) to be something like "Treat external
storage devices as read-only" or "Do not allow writing data to external storage
devices".

https://codereview.chromium.org/2267653002/diff/1/components/policy/resources...
components/policy/resources/policy_templates.json:5637: 'dynamic_refresh': True,
Can you actually dynamically accept changes to this value (will you unmount and
remount external storage on the fly when this value changes)? If you require a
restart of the session, then this should be False (not sure how your
implementation works).

[yamaguchi, 2016-08-23 02:47:16]

https://codereview.chromium.org/2267653002/diff/1/components/policy/resources...
File components/policy/resources/policy_templates.json (right):

https://codereview.chromium.org/2267653002/diff/1/components/policy/resources...
components/policy/resources/policy_templates.json:5634: 'caption': '''Limit
mounting of external storage to read-only mode.''',
On 2016/08/22 13:21:40, Andrew T Wilson (Slow) wrote:
> I don't think admins necessarily know what "mounting to read-only mode" is.
> Perhaps change this (and the description) to be something like "Treat external
> storage devices as read-only" or "Do not allow writing data to external
storage
> devices".

Done.

https://codereview.chromium.org/2267653002/diff/1/components/policy/resources...
components/policy/resources/policy_templates.json:5637: 'dynamic_refresh': True,
On 2016/08/22 13:21:40, Andrew T Wilson (Slow) wrote:
> Can you actually dynamically accept changes to this value (will you unmount
and
> remount external storage on the fly when this value changes)? If you require a
> restart of the session, then this should be False (not sure how your
> implementation works).

Changed it to False because that pattern is not covered by our current design of
the feature at the moment.

The value change will be reflected without session restart when other event
(e.g. the system recovered from suspend, new device connected, ...) occurs after
pref change, but we don't handle the preference change event for this value.

[yamaguchi, 2016-08-23 02:48:38]

The CQ bit was checked by yamaguchi@chromium.org

[yamaguchi, 2016-08-23 02:48:39]

The patchset sent to the CQ was uploaded after l-g-t-m from
atwilson@chromium.org
Link to the patchset: https://codereview.chromium.org/2267653002/#ps20001
(title: "Change description text to be more user-friendly.")

[commit-bot: I haz the power, 2016-08-23 02:49:02]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-08-23 02:52:20]

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

[commit-bot: I haz the power, 2016-08-23 02:52:21]

Try jobs failed on following builders:
  ios-simulator on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)

[yamaguchi, 2016-08-23 03:02:10]

The CQ bit was checked by yamaguchi@chromium.org

[yamaguchi, 2016-08-23 03:02:10]

The patchset sent to the CQ was uploaded after l-g-t-m from
atwilson@chromium.org
Link to the patchset: https://codereview.chromium.org/2267653002/#ps40001
(title: "Sync to head.")

[commit-bot: I haz the power, 2016-08-23 03:02:36]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-08-23 03:09:19]

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

[commit-bot: I haz the power, 2016-08-23 03:09:20]

Try jobs failed on following builders:
  chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)

[yamaguchi, 2016-08-23 03:24:34]

yamaguchi@chromium.org changed reviewers:
+ rkaplow@chromium.org

[yamaguchi, 2016-08-23 03:24:34]

+rkaplow for tools/metrics/histograms/histograms.xml

[satorux1, 2016-08-23 22:16:03]

satorux@chromium.org changed reviewers:
+ isherman@chromium.org, satorux@chromium.org

[satorux1, 2016-08-23 22:16:03]

contacted isherman@ as rkaplow@ is now traveling. isherman gave LGTM via chat,
but could not log in to riteveld from the phone. he suggested we submit with
TBR=isherman@

[satorux1, 2016-08-23 23:26:27]

Description was changed from

==========
Add ExternalStorageReadOnly profile value.

TEST=policy_test_cases
BUG=629945
==========

to

==========
Add ExternalStorageReadOnly profile value.

TEST=policy_test_cases
BUG=629945
TBR=isherman@chromium.org
==========

[satorux1, 2016-08-23 23:26:32]

The CQ bit was checked by satorux@chromium.org

[commit-bot: I haz the power, 2016-08-23 23:27:30]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-08-24 01:13:52]

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

[commit-bot: I haz the power, 2016-08-24 01:13:53]

Try jobs failed on following builders:
  linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[Ilya Sherman, 2016-08-24 01:47:58]

histograms.xml lgtm

[yamaguchi, 2016-08-24 02:29:42]

The CQ bit was checked by yamaguchi@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-08-24 02:30:36]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[yamaguchi, 2016-08-24 03:22:22]

I added file configuration_policy_handler_list_factory.cc as it's necessary.
Would I need LGTM by the owners again? (bartfab, atwilson)

[satorux1, 2016-08-24 03:30:48]

On 2016/08/24 03:22:22, yamaguchi wrote:
> I added file configuration_policy_handler_list_factory.cc as it's necessary.
> Would I need LGTM by the owners again? (bartfab, atwilson)

looks like a mechanical change. should be alright to go submit now. 

bartfab, atwilson: please blame me if that's not the case.

[commit-bot: I haz the power, 2016-08-24 05:11:04]

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

[commit-bot: I haz the power, 2016-08-24 05:11:06]

Dry run: Try jobs failed on following builders:
  win_chromium_x64_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)

[satorux1, 2016-08-24 05:32:31]

The CQ bit was checked by satorux@chromium.org

[satorux1, 2016-08-24 05:32:31]

The patchset sent to the CQ was uploaded after l-g-t-m from
isherman@chromium.org, satorux@chromium.org, atwilson@chromium.org
Link to the patchset: https://codereview.chromium.org/2267653002/#ps60001
(title: "Add to policy-to-preference map.")

[commit-bot: I haz the power, 2016-08-24 05:32:48]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[yamaguchi, 2016-08-24 05:41:45]

CQ dry run had error with win_chromium_x64_rel_ng. I am investigating.
https://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64...

[satorux1, 2016-08-24 05:54:13]

On 2016/08/24 05:41:45, yamaguchi wrote:
> CQ dry run had error with win_chromium_x64_rel_ng. I am investigating.
>
https://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64...

looks unrelated to this change?

[fukino, 2016-08-24 06:02:19]

On 2016/08/24 05:54:13, satorux1 wrote:
> On 2016/08/24 05:41:45, yamaguchi wrote:
> > CQ dry run had error with win_chromium_x64_rel_ng. I am investigating.
> >
>
https://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64...
> 
> looks unrelated to this change?

Looks unrelated.
https://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64...
also get the same error. (CL by other person).

[commit-bot: I haz the power, 2016-08-24 06:40:06]

Description was changed from

==========
Add ExternalStorageReadOnly profile value.

TEST=policy_test_cases
BUG=629945
TBR=isherman@chromium.org
==========

to

==========
Add ExternalStorageReadOnly profile value.

TEST=policy_test_cases
BUG=629945
TBR=isherman@chromium.org
==========

[commit-bot: I haz the power, 2016-08-24 06:40:07]

Committed patchset #4 (id:60001)

[commit-bot: I haz the power, 2016-08-24 06:42:25]

Description was changed from

==========
Add ExternalStorageReadOnly profile value.

TEST=policy_test_cases
BUG=629945
TBR=isherman@chromium.org
==========

to

==========
Add ExternalStorageReadOnly profile value.

TEST=policy_test_cases
BUG=629945
TBR=isherman@chromium.org

Committed: https://crrev.com/36aeef7a0a017f4fbaafba82578a20aaf4bd3e00
Cr-Commit-Position: refs/heads/master@{#413998}
==========

[commit-bot: I haz the power, 2016-08-24 06:42:27]

Patchset 4 (id:??) landed as
https://crrev.com/36aeef7a0a017f4fbaafba82578a20aaf4bd3e00
Cr-Commit-Position: refs/heads/master@{#413998}

[bartfab (slow), 2016-08-29 14:50:40]

https://codereview.chromium.org/2267653002/diff/60001/components/policy/resou...
File components/policy/resources/policy_templates.json (right):

https://codereview.chromium.org/2267653002/diff/60001/components/policy/resou...
components/policy/resources/policy_templates.json:5637: 'dynamic_refresh':
False,
Why can we not change this at runtime? We try really hard to support dynamic
refresh for all policies.

https://codereview.chromium.org/2267653002/diff/60001/tools/metrics/histogram...
File tools/metrics/histograms/histograms.xml (right):

https://codereview.chromium.org/2267653002/diff/60001/tools/metrics/histogram...
tools/metrics/histograms/histograms.xml:75542: +  <int value="343" label="Mount
external storage only in read-only mode"/>
This should match the policy caption in policy_templates.json

[yamaguchi, 2016-08-30 05:35:13]

https://codereview.chromium.org/2267653002/diff/60001/components/policy/resou...
File components/policy/resources/policy_templates.json (right):

https://codereview.chromium.org/2267653002/diff/60001/components/policy/resou...
components/policy/resources/policy_templates.json:5637: 'dynamic_refresh':
False,
On 2016/08/29 14:50:40, bartfab (slow) wrote:
> Why can we not change this at runtime? We try really hard to support dynamic
> refresh for all policies.

We will do this by the next milestone.
We did not have the exact requirement and behavior specs about the dynamic
refresh for this feature.
Filed crbug.com/642247 .

https://codereview.chromium.org/2267653002/diff/60001/tools/metrics/histogram...
File tools/metrics/histograms/histograms.xml (right):

https://codereview.chromium.org/2267653002/diff/60001/tools/metrics/histogram...
tools/metrics/histograms/histograms.xml:75542: +  <int value="343" label="Mount
external storage only in read-only mode"/>
On 2016/08/29 14:50:40, bartfab (slow) wrote:
> This should match the policy caption in policy_templates.json

Acknowledged.