Add managed policies for Native Messaging

chrome/browser/extensions/api/messaging/native_messaging_policy_handler.cc

+// Copyright 2013 The Chromium Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+#include "chrome/browser/extensions/api/messaging/native_messaging_policy_handler.h"
+
+#include "base/logging.h"
+#include "base/prefs/pref_value_map.h"
+#include "chrome/browser/extensions/api/messaging/native_messaging_host_manifest.h"
+#include "chrome/browser/extensions/external_policy_loader.h"
+#include "chrome/common/pref_names.h"
+#include "components/policy/core/browser/policy_error_map.h"
+#include "components/policy/core/common/policy_map.h"
+#include "grit/component_strings.h"
+#include "policy/policy_constants.h"
+
+namespace extensions {
+
+NativeMessagingHostListPolicyHandler::NativeMessagingHostListPolicyHandler(
+    const char* policy_name,
+    const char* pref_path,
+    bool allow_wildcards)
+    : policy::TypeCheckingPolicyHandler(policy_name, base::Value::TYPE_LIST),
+      pref_path_(pref_path),
+      allow_wildcards_(allow_wildcards) {}
+
+NativeMessagingHostListPolicyHandler::~NativeMessagingHostListPolicyHandler() {}
+
+bool NativeMessagingHostListPolicyHandler::CheckPolicySettings(
+    const policy::PolicyMap& policies,
+    policy::PolicyErrorMap* errors) {
+  return CheckAndGetList(policies, errors, NULL);
+}
+
+void NativeMessagingHostListPolicyHandler::ApplyPolicySettings(
+    const policy::PolicyMap& policies,
+    PrefValueMap* prefs) {
+  scoped_ptr<base::ListValue> list;
+  policy::PolicyErrorMap errors;
+  if (CheckAndGetList(policies, &errors, &list) && list)

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

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.

[Sergey Ulanov, 2014/01/06 23:21:55]

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.

+    prefs->SetValue(pref_path(), list.release());
+}
+
+const char* NativeMessagingHostListPolicyHandler::pref_path() const {
+  return pref_path_;
+}
+
+bool NativeMessagingHostListPolicyHandler::CheckAndGetList(
+    const policy::PolicyMap& policies,
+    policy::PolicyErrorMap* errors,
+    scoped_ptr<base::ListValue>* names) {

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

you could just pass a base::ListValue* here and then instead of using the
"*names = foo.Pass()" pattern use the "names->Swap(foo)" pattern.

[Sergey Ulanov, 2014/01/06 23:21:55]

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

+  if (names)
+    names->reset();

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

this will always be a no-op I think.

[Sergey Ulanov, 2014/01/06 23:21:55]

Done.

+
+  const base::Value* value = NULL;
+  if (!CheckAndGetValue(policies, errors, &value))
+    return false;
+
+  if (!value)

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

what does not finding this value mean?

[Sergey Ulanov, 2014/01/06 23:21:55]

It means that the policy isn't set.

+    return true;
+
+  const base::ListValue* list_value = NULL;
+  if (!value->GetAsList(&list_value)) {
+    NOTREACHED();
+    return false;
+  }
+
+  // Filter the list, rejecting any invalid extension IDs.

[Joao da Silva, 2013/12/29 23:25:28]

IIUC there are no extension IDs in these values.

[Sergey Ulanov, 2014/01/06 23:21:55]

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

+  scoped_ptr<base::ListValue> filtered_list(new base::ListValue());
+  for (base::ListValue::const_iterator entry(list_value->begin());
+       entry != list_value->end(); ++entry) {
+    std::string name;
+    if (!(*entry)->GetAsString(&name)) {
+      errors->AddError(policy_name(),
+                       entry - list_value->begin(),
+                       IDS_POLICY_TYPE_ERROR,
+                       ValueTypeToString(base::Value::TYPE_STRING));
+      continue;
+    }
+    if (!(allow_wildcards_ && name == "*") &&
+        !NativeMessagingHostManifest::IsValidName(name)) {
+      errors->AddError(policy_name(),
+                       entry - list_value->begin(),
+                       IDS_POLICY_VALUE_FORMAT_ERROR);
+      continue;
+    }
+    filtered_list->Append(base::Value::CreateStringValue(name));

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

the "CreateStringValue(...)" methods are deprecated, use "new StringValue(...)".

[Sergey Ulanov, 2014/01/06 23:21:55]

Done.

+  }
+
+  if (names)
+    *names = filtered_list.Pass();

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

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.

[Sergey Ulanov, 2014/01/06 23:21:55]

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*.
We still want to generate the errors in lines 74 and 82, even when names=NULL.

+
+  return true;
+}
+
+}  // namespace extensions