Add managed policies for Native Messaging

Add managed policies for Native Messaging

This adds two new policies that will allow to block all or some native
messaging hosts.

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

[Sergey Ulanov, 2013-12-28 03:39:01]

kalman@, please review chrome/browser/extensions
isherman@ - tools/metrics/histograms/histograms.xml
dconnelly@ - everything else

[not at google - send to devlin, 2013-12-28 04:55:50]

https://codereview.chromium.org/118253005/diff/30001/chrome/browser/extension...
File chrome/browser/extensions/api/messaging/native_message_process_host.cc
(right):

https://codereview.chromium.org/118253005/diff/30001/chrome/browser/extension...
chrome/browser/extensions/api/messaging/native_message_process_host.cc:59: if
(!pref_service->IsManagedPreference(prefs::kNativeMessagingBlacklist))
what is the difference between this check and the one on line 63?

https://codereview.chromium.org/118253005/diff/30001/chrome/browser/extension...
chrome/browser/extensions/api/messaging/native_message_process_host.cc:75: if
(pref_service->IsManagedPreference(prefs::kNativeMessagingWhitelist)) {
ditto

https://codereview.chromium.org/118253005/diff/30001/chrome/browser/extension...
File chrome/browser/extensions/api/messaging/native_messaging_policy_handler.cc
(right):

https://codereview.chromium.org/118253005/diff/30001/chrome/browser/extension...
chrome/browser/extensions/api/messaging/native_messaging_policy_handler.cc:40:
if (CheckAndGetList(policies, &errors, &list) && list)
If I'm reading this correctly... the "&& list" here means that if the policy is
cleared then the corresponding preferences won't be cleared? that seems
undesirable.

https://codereview.chromium.org/118253005/diff/30001/chrome/browser/extension...
chrome/browser/extensions/api/messaging/native_messaging_policy_handler.cc:51:
scoped_ptr<base::ListValue>* names) {
you could just pass a base::ListValue* here and then instead of using the
"*names = foo.Pass()" pattern use the "names->Swap(foo)" pattern.

https://codereview.chromium.org/118253005/diff/30001/chrome/browser/extension...
chrome/browser/extensions/api/messaging/native_messaging_policy_handler.cc:53:
names->reset();
this will always be a no-op I think.

https://codereview.chromium.org/118253005/diff/30001/chrome/browser/extension...
chrome/browser/extensions/api/messaging/native_messaging_policy_handler.cc:59:
if (!value)
what does not finding this value mean?

https://codereview.chromium.org/118253005/diff/30001/chrome/browser/extension...
chrome/browser/extensions/api/messaging/native_messaging_policy_handler.cc:87:
filtered_list->Append(base::Value::CreateStringValue(name));
the "CreateStringValue(...)" methods are deprecated, use "new StringValue(...)".

https://codereview.chromium.org/118253005/diff/30001/chrome/browser/extension...
chrome/browser/extensions/api/messaging/native_messaging_policy_handler.cc:91:
*names = filtered_list.Pass();
the way this is implemented there isn't any advantage in having |names| be
passed in as a scoped_ptr*. You might as well just pass in an always-not-null
ListValue* and fill that in (as in, rather than populating |filtered_list|
populate |names| instead).

that said, an optimisation you could do with the current code would be "if
(!names) return true" up on line 67. That is, return true if it actually exists
+ is a list. That would save you doing the same work twice and presuming how
this policy stuff is implemented just about the most efficient way to do this.

up to you whether you follow the 1st or 2nd paragraph there.

https://codereview.chromium.org/118253005/diff/30001/chrome/browser/extension...
File chrome/browser/extensions/api/messaging/native_messaging_policy_handler.h
(right):

https://codereview.chromium.org/118253005/diff/30001/chrome/browser/extension...
chrome/browser/extensions/api/messaging/native_messaging_policy_handler.h:26:
bool allow_wildcards);
nit: indentation

https://codereview.chromium.org/118253005/diff/30001/chrome/browser/extension...
File
chrome/browser/extensions/api/messaging/native_messaging_policy_handler_unittest.cc
(right):

https://codereview.chromium.org/118253005/diff/30001/chrome/browser/extension...
chrome/browser/extensions/api/messaging/native_messaging_policy_handler_unittest.cc:33:
EXPECT_TRUE(errors.empty());
fwiw, rather than this pattern of constantly clearing the errors, I usually
prefer a safer and more obviously correct sequence of assertions in the form:

// setup...
{
  policy::PolicyErrorMap errors;
  CheckPolicySettings(&errors);
  EXPECT_TRUE(errors.empty());
}

etc. but, up to you.

https://codereview.chromium.org/118253005/diff/30001/chrome/browser/extension...
chrome/browser/extensions/api/messaging/native_messaging_policy_handler_unittest.cc:66:
errors.GetErrors(policy::key::kNativeMessagingBlacklist).empty());
can you actually assert that the content here is what you expect? something
along the lines of

{
  PolcyErrorMap errors;
  CheckPolicySerttings(&errors);
  EXPECT_EQ(kTheErrorThatYouExpect, errors.GetErrors(..));
}

https://codereview.chromium.org/118253005/diff/30001/chrome/browser/extension...
chrome/browser/extensions/api/messaging/native_messaging_policy_handler_unittest.cc:81:
base::Value::CreateStringValue("com.example.test"));
new StringValue

https://codereview.chromium.org/118253005/diff/30001/chrome/browser/extension...
chrome/browser/extensions/api/messaging/native_messaging_policy_handler_unittest.cc:91:

would it be useful to test the wildcard in here somewhere?

[Ilya Sherman, 2013-12-29 01:50:17]

histograms.xml lgtm

[Joao da Silva, 2013-12-29 23:25:26]

(Daniel is OOO, drive-by look at the policy bits)

https://codereview.chromium.org/118253005/diff/30001/chrome/browser/extension...
File chrome/browser/extensions/api/messaging/message_service.cc (right):

https://codereview.chromium.org/118253005/diff/30001/chrome/browser/extension...
chrome/browser/extensions/api/messaging/message_service.cc:67: "Access to then
native messaging host is prohibited by system policies.";
*the

We often blame these restrictions on the system administrator :-)

Suggestion: "Access to the native messaging host was disabled by the system
administrator."

https://codereview.chromium.org/118253005/diff/30001/chrome/browser/extension...
File chrome/browser/extensions/api/messaging/native_messaging_policy_handler.cc
(right):

https://codereview.chromium.org/118253005/diff/30001/chrome/browser/extension...
chrome/browser/extensions/api/messaging/native_messaging_policy_handler.cc:68:
// Filter the list, rejecting any invalid extension IDs.
IIUC there are no extension IDs in these values.

https://codereview.chromium.org/118253005/diff/30001/chrome/chrome_browser_ex...
File chrome/chrome_browser_extensions.gypi (right):

https://codereview.chromium.org/118253005/diff/30001/chrome/chrome_browser_ex...
chrome/chrome_browser_extensions.gypi:377:
'browser/extensions/api/messaging/native_messaging_policy_handler.h',
nit: order

https://codereview.chromium.org/118253005/diff/30001/chrome/common/pref_names.cc
File chrome/common/pref_names.cc (right):

https://codereview.chromium.org/118253005/diff/30001/chrome/common/pref_names...
chrome/common/pref_names.cc:1710: // type is on this whitelisted or
alternatively on kExtensionInstallAllowList or
The previous sentence looks better to me

https://codereview.chromium.org/118253005/diff/30001/chrome/common/pref_names...
chrome/common/pref_names.cc:2685: const char kNativeMessagingWhitelist[] =
"native_messaging.whitelist";
Same about the order; this should be above line 1297.

https://codereview.chromium.org/118253005/diff/30001/chrome/common/pref_names.h
File chrome/common/pref_names.h (right):

https://codereview.chromium.org/118253005/diff/30001/chrome/common/pref_names...
chrome/common/pref_names.h:958: extern const char kNativeMessagingWhitelist[];
Note that profile prefs are listed before local state prefs. Since these 2
belong to the profile they should be moved above line 412.

https://codereview.chromium.org/118253005/diff/30001/components/policy/resour...
File components/policy/resources/policy_templates.json (right):

https://codereview.chromium.org/118253005/diff/30001/components/policy/resour...
components/policy/resources/policy_templates.json:2552:
'com.native.messaging.host.name1',
name2?

https://codereview.chromium.org/118253005/diff/30001/components/policy/resour...
components/policy/resources/policy_templates.json:2556: 'desc': '''Allows you to
specify which native messaging hosts the users are not alloed.
*allowed

Seems like a bit of the sentence is missing at the end:

".. the users are not allowed to use"?

https://codereview.chromium.org/118253005/diff/30001/components/policy/resour...
components/policy/resources/policy_templates.json:2577:
'com.native.messaging.host.name1',
name2?

[Sergey Ulanov, 2014-01-06 23:21:54]

https://codereview.chromium.org/118253005/diff/30001/chrome/browser/extension...
File chrome/browser/extensions/api/messaging/message_service.cc (right):

https://codereview.chromium.org/118253005/diff/30001/chrome/browser/extension...
chrome/browser/extensions/api/messaging/message_service.cc:67: "Access to then
native messaging host is prohibited by system policies.";
On 2013/12/29 23:25:28, Joao da Silva wrote:
> *the
> 
> We often blame these restrictions on the system administrator :-)
> 
> Suggestion: "Access to the native messaging host was disabled by the system
> administrator."

Done.

https://codereview.chromium.org/118253005/diff/30001/chrome/browser/extension...
File chrome/browser/extensions/api/messaging/native_message_process_host.cc
(right):

https://codereview.chromium.org/118253005/diff/30001/chrome/browser/extension...
chrome/browser/extensions/api/messaging/native_message_process_host.cc:59: if
(!pref_service->IsManagedPreference(prefs::kNativeMessagingBlacklist))
On 2013/12/28 04:55:51, kalman wrote:
> what is the difference between this check and the one on line 63?

Normally the preference will always be present in the prefs dictionary (because
it's registered in chrome/browser/extensions/extension_prefs.cc). This check
verifies that it's actually managed.

https://codereview.chromium.org/118253005/diff/30001/chrome/browser/extension...
chrome/browser/extensions/api/messaging/native_message_process_host.cc:75: if
(pref_service->IsManagedPreference(prefs::kNativeMessagingWhitelist)) {
On 2013/12/28 04:55:51, kalman wrote:
> ditto

Same here.

https://codereview.chromium.org/118253005/diff/30001/chrome/browser/extension...
File chrome/browser/extensions/api/messaging/native_messaging_policy_handler.cc
(right):

https://codereview.chromium.org/118253005/diff/30001/chrome/browser/extension...
chrome/browser/extensions/api/messaging/native_messaging_policy_handler.cc:40:
if (CheckAndGetList(policies, &errors, &list) && list)
On 2013/12/28 04:55:51, kalman wrote:
> If I'm reading this correctly... the "&& list" here means that if the policy
is
> cleared then the corresponding preferences won't be cleared? that seems
> undesirable.

That's the way it works for all other policies, e.g.
https://code.google.com/p/chromium/codesearch#chromium/src/components/policy/...
https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/ext...

I'm not sure what's the reason for not cleaning the policy, but it shouldn't
make any difference because IsManagedPreference() would be returning false for
the policies that have been removed.

https://codereview.chromium.org/118253005/diff/30001/chrome/browser/extension...
chrome/browser/extensions/api/messaging/native_messaging_policy_handler.cc:51:
scoped_ptr<base::ListValue>* names) {
On 2013/12/28 04:55:51, kalman wrote:
> you could just pass a base::ListValue* here and then instead of using the
> "*names = foo.Pass()" pattern use the "names->Swap(foo)" pattern.

We want to distinguish between the case when the policy is not set and when it's
set to an empty list.

https://codereview.chromium.org/118253005/diff/30001/chrome/browser/extension...
chrome/browser/extensions/api/messaging/native_messaging_policy_handler.cc:53:
names->reset();
On 2013/12/28 04:55:51, kalman wrote:
> this will always be a no-op I think.

Done.

https://codereview.chromium.org/118253005/diff/30001/chrome/browser/extension...
chrome/browser/extensions/api/messaging/native_messaging_policy_handler.cc:59:
if (!value)
On 2013/12/28 04:55:51, kalman wrote:
> what does not finding this value mean?

It means that the policy isn't set.

https://codereview.chromium.org/118253005/diff/30001/chrome/browser/extension...
chrome/browser/extensions/api/messaging/native_messaging_policy_handler.cc:68:
// Filter the list, rejecting any invalid extension IDs.
On 2013/12/29 23:25:28, Joao da Silva wrote:
> IIUC there are no extension IDs in these values.

Copied this code from chrome/browser/extensions/policy_handlers.cc and forgot to
update the comment.

https://codereview.chromium.org/118253005/diff/30001/chrome/browser/extension...
chrome/browser/extensions/api/messaging/native_messaging_policy_handler.cc:87:
filtered_list->Append(base::Value::CreateStringValue(name));
On 2013/12/28 04:55:51, kalman wrote:
> the "CreateStringValue(...)" methods are deprecated, use "new
StringValue(...)".

Done.

https://codereview.chromium.org/118253005/diff/30001/chrome/browser/extension...
chrome/browser/extensions/api/messaging/native_messaging_policy_handler.cc:91:
*names = filtered_list.Pass();
On 2013/12/28 04:55:51, kalman wrote:
> the way this is implemented there isn't any advantage in having |names| be
> passed in as a scoped_ptr*. You might as well just pass in an always-not-null
> ListValue* and fill that in (as in, rather than populating |filtered_list|
> populate |names| instead).

We want to distinguish between the case when the policy is not set and when it's
set to an empty list, so I don't think names can be ListValue*.

> 
> that said, an optimisation you could do with the current code would be "if
> (!names) return true" up on line 67. That is, return true if it actually
exists
> + is a list. That would save you doing the same work twice and presuming how
> this policy stuff is implemented just about the most efficient way to do this.
> 

We still want to generate the errors in lines 74 and 82, even when names=NULL.

> up to you whether you follow the 1st or 2nd paragraph there.

https://codereview.chromium.org/118253005/diff/30001/chrome/browser/extension...
File chrome/browser/extensions/api/messaging/native_messaging_policy_handler.h
(right):

https://codereview.chromium.org/118253005/diff/30001/chrome/browser/extension...
chrome/browser/extensions/api/messaging/native_messaging_policy_handler.h:26:
bool allow_wildcards);
On 2013/12/28 04:55:51, kalman wrote:
> nit: indentation

Done.

https://codereview.chromium.org/118253005/diff/30001/chrome/browser/extension...
File
chrome/browser/extensions/api/messaging/native_messaging_policy_handler_unittest.cc
(right):

https://codereview.chromium.org/118253005/diff/30001/chrome/browser/extension...
chrome/browser/extensions/api/messaging/native_messaging_policy_handler_unittest.cc:33:
EXPECT_TRUE(errors.empty());
On 2013/12/28 04:55:51, kalman wrote:
> fwiw, rather than this pattern of constantly clearing the errors, I usually
> prefer a safer and more obviously correct sequence of assertions in the form:
> 
> // setup...
> {
>   policy::PolicyErrorMap errors;
>   CheckPolicySettings(&errors);
>   EXPECT_TRUE(errors.empty());
> }
> 
> etc. but, up to you.

Done.

https://codereview.chromium.org/118253005/diff/30001/chrome/browser/extension...
chrome/browser/extensions/api/messaging/native_messaging_policy_handler_unittest.cc:66:
errors.GetErrors(policy::key::kNativeMessagingBlacklist).empty());
On 2013/12/28 04:55:51, kalman wrote:
> can you actually assert that the content here is what you expect? something
> along the lines of
> 
> {
>   PolcyErrorMap errors;
>   CheckPolicySerttings(&errors);
>   EXPECT_EQ(kTheErrorThatYouExpect, errors.GetErrors(..));
> }

The error message is localized, so I don't think it would be right to check the
message here.

https://codereview.chromium.org/118253005/diff/30001/chrome/browser/extension...
chrome/browser/extensions/api/messaging/native_messaging_policy_handler_unittest.cc:81:
base::Value::CreateStringValue("com.example.test"));
On 2013/12/28 04:55:51, kalman wrote:
> new StringValue

Done.

https://codereview.chromium.org/118253005/diff/30001/chrome/browser/extension...
chrome/browser/extensions/api/messaging/native_messaging_policy_handler_unittest.cc:91:

On 2013/12/28 04:55:51, kalman wrote:
> would it be useful to test the wildcard in here somewhere?

Done.

https://codereview.chromium.org/118253005/diff/30001/chrome/chrome_browser_ex...
File chrome/chrome_browser_extensions.gypi (right):

https://codereview.chromium.org/118253005/diff/30001/chrome/chrome_browser_ex...
chrome/chrome_browser_extensions.gypi:377:
'browser/extensions/api/messaging/native_messaging_policy_handler.h',
On 2013/12/29 23:25:28, Joao da Silva wrote:
> nit: order

Done.

https://codereview.chromium.org/118253005/diff/30001/chrome/common/pref_names.cc
File chrome/common/pref_names.cc (right):

https://codereview.chromium.org/118253005/diff/30001/chrome/common/pref_names...
chrome/common/pref_names.cc:1710: // type is on this whitelisted or
alternatively on kExtensionInstallAllowList or
On 2013/12/29 23:25:28, Joao da Silva wrote:
> The previous sentence looks better to me

Done.

https://codereview.chromium.org/118253005/diff/30001/chrome/common/pref_names...
chrome/common/pref_names.cc:2685: const char kNativeMessagingWhitelist[] =
"native_messaging.whitelist";
On 2013/12/29 23:25:28, Joao da Silva wrote:
> Same about the order; this should be above line 1297.

Done.

https://codereview.chromium.org/118253005/diff/30001/chrome/common/pref_names.h
File chrome/common/pref_names.h (right):

https://codereview.chromium.org/118253005/diff/30001/chrome/common/pref_names...
chrome/common/pref_names.h:958: extern const char kNativeMessagingWhitelist[];
On 2013/12/29 23:25:28, Joao da Silva wrote:
> Note that profile prefs are listed before local state prefs. Since these 2
> belong to the profile they should be moved above line 412.

Done.

https://codereview.chromium.org/118253005/diff/30001/components/policy/resour...
File components/policy/resources/policy_templates.json (right):

https://codereview.chromium.org/118253005/diff/30001/components/policy/resour...
components/policy/resources/policy_templates.json:2552:
'com.native.messaging.host.name1',
On 2013/12/29 23:25:28, Joao da Silva wrote:
> name2?

Done.

https://codereview.chromium.org/118253005/diff/30001/components/policy/resour...
components/policy/resources/policy_templates.json:2556: 'desc': '''Allows you to
specify which native messaging hosts the users are not alloed.
On 2013/12/29 23:25:28, Joao da Silva wrote:
> *allowed
> 
> Seems like a bit of the sentence is missing at the end:
> 
> ".. the users are not allowed to use"?

Done.

https://codereview.chromium.org/118253005/diff/30001/components/policy/resour...
components/policy/resources/policy_templates.json:2577:
'com.native.messaging.host.name1',
On 2013/12/29 23:25:28, Joao da Silva wrote:
> name2?

Done.

[dconnelly, 2014-01-07 08:54:28]

Thanks joao for the review. Sorry for not adding OOO to my username.

lgtm

[not at google - send to devlin, 2014-01-07 23:52:39]

lgtm

[commit-bot: I haz the power, 2014-01-07 23:57:39]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/sergeyu@chromium.org/118253005/440001

[commit-bot: I haz the power, 2014-01-08 01:05:42]

Retried try job too often on android_clang_dbg for step(s) slave_steps
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=android_cl...

[commit-bot: I haz the power, 2014-01-08 02:19:50]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/sergeyu@chromium.org/118253005/730001

[commit-bot: I haz the power, 2014-01-08 04:15:37]

Retried try job too often on win_rel for step(s) nacl_integration
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_rel&nu...

[commit-bot: I haz the power, 2014-01-08 19:48:35]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/sergeyu@chromium.org/118253005/970001

[commit-bot: I haz the power, 2014-01-09 00:06:06]

Retried try job too often on linux_chromeos for step(s) browser_tests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_chro...

[commit-bot: I haz the power, 2014-01-09 01:51:04]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/sergeyu@chromium.org/118253005/1280001

[commit-bot: I haz the power, 2014-01-09 04:48:32]

Change committed as 243774