Reporting a policy error after detecting mistyped policies.

Reporting a policy error after detecting mistyped policies.

After this changelist policy::Schema is used to validate policy contents.
Mistyped values trigger PolicyWatcher::PolicyErrorCallback.
Misspelled names trigger just a LOG(WARNING) message.

As a follow-up to using policy::Schema for validation, we can remove ad-hoc
fallback values for 2 out of 15 or our policies that used to be stored in
PolicyWatcher::bad_type_values_.  This can be replaced with a CHECK that
verifies that the Schema indeed didn't pass through any mistyped values.

As another follow-up we can remove the TODO comment asking to start using
PolicyWatcher::PolicyErrorCallback again.  We can also remove
SignalTransientPolicyError which is not used anymore at this point (we can
hesitantly consider reintroducing it if needed to handle detection of
unparseable files;  this is still some time in the future though).

BUG=427513
TEST=remoting_unittests + manual verification on Win and Linux via TestRealChromotingPolicy

Committed: https://crrev.com/e5d69ea2ccfe129eccd06d3b1de43a7092ee201d
Cr-Commit-Position: refs/heads/master@{#317646}

[Łukasz Anforowicz, 2015-02-03 18:39:53]

Patchset #1 (id:1) has been deleted

[Łukasz Anforowicz, 2015-02-04 19:43:38]

Patchset #1 (id:20001) has been deleted

[Łukasz Anforowicz, 2015-02-04 20:14:55]

Patchset #1 (id:40001) has been deleted

[Łukasz Anforowicz, 2015-02-12 00:19:45]

Patchset #2 (id:80001) has been deleted

[Łukasz Anforowicz, 2015-02-12 00:19:55]

Patchset #1 (id:60001) has been deleted

[Łukasz Anforowicz, 2015-02-12 00:20:59]

lukasza@chromium.org changed reviewers:
+ sergeyu@chromium.org

[Łukasz Anforowicz, 2015-02-12 00:21:00]

Sergey, could you please take a look?

[Sergey Ulanov, 2015-02-12 19:22:59]

lgtm with some nits.

https://codereview.chromium.org/891053003/diff/100001/remoting/host/policy_wa...
File remoting/host/policy_watcher.cc (right):

https://codereview.chromium.org/891053003/diff/100001/remoting/host/policy_wa...
remoting/host/policy_watcher.cc:59: CHECK(value->IsType(i.value().GetType()));
Does this need to be a CHECK() instead of DCHECK()?
Do we have unittests to verify that |default_values| matches schema?

https://codereview.chromium.org/891053003/diff/100001/remoting/host/policy_wa...
remoting/host/policy_watcher.cc:204: const policy::Schema* schema =
schema_map->GetSchema(ns);
nit: You can write this whole function in one line:

  return owned_schema_registry_->schema_map()->GetSchema(GetPolicyNamespace());

Even if you'd prefer keeping |schema_map|, you don't need |schema|

https://codereview.chromium.org/891053003/diff/100001/remoting/host/policy_wa...
remoting/host/policy_watcher.cc:212: const std::string policy_name_prefix =
"RemoteAccessHost";
This doesn't need to be std::string. Style guide asks to use kConstName for
consts.
  const char kPolicyNamePrefix[] = "RemoteAccessHost";

https://codereview.chromium.org/891053003/diff/100001/remoting/host/policy_wa...
remoting/host/policy_watcher.cc:228: // 1) comments,
Comments should be dropped by the parser, no?

[Łukasz Anforowicz, 2015-02-12 20:31:46]

New patchsets have been uploaded after l-g-t-m from sergeyu@chromium.org

[Łukasz Anforowicz, 2015-02-12 20:34:14]

Thanks for the feedback.  I tried addressing it in patchset #2.

-Lukasz

https://codereview.chromium.org/891053003/diff/100001/remoting/host/policy_wa...
File remoting/host/policy_watcher.cc (right):

https://codereview.chromium.org/891053003/diff/100001/remoting/host/policy_wa...
remoting/host/policy_watcher.cc:59: CHECK(value->IsType(i.value().GetType()));
On 2015/02/12 19:22:58, Sergey Ulanov wrote:
> Does this need to be a CHECK() instead of DCHECK()?

Good question.  I was wondering about it myself.

Arguments for leaving this as a CHECK:
- It was a CHECK before, even though in theory bad_type_values_ should cover all
the policies.
- Continuing with bad policies is so bad, that it should be fatal to the
process.

Arguments for changing to DCHECK:
- This should already be enforced by other parts of the code + should be already
verified via unit tests (well... after I add more tests as you suggested below).

> Do we have unittests to verify that |default_values| matches schema?

We have PolicySchemaAndPolicyWatcherShouldBeInSync unit test which landed in
crrev.com/903883005.  Currently this only verifies that |default_values| matches
the schema wrt policy names.  I should have added type match verfication as
well.  On it! :-)

https://codereview.chromium.org/891053003/diff/100001/remoting/host/policy_wa...
remoting/host/policy_watcher.cc:204: const policy::Schema* schema =
schema_map->GetSchema(ns);
On 2015/02/12 19:22:59, Sergey Ulanov wrote:
> nit: You can write this whole function in one line:
> 
>   return
owned_schema_registry_->schema_map()->GetSchema(GetPolicyNamespace());
> 
> Even if you'd prefer keeping |schema_map|, you don't need |schema|

Thanks.  I don't know why I did it this way...

https://codereview.chromium.org/891053003/diff/100001/remoting/host/policy_wa...
remoting/host/policy_watcher.cc:212: const std::string policy_name_prefix =
"RemoteAccessHost";
On 2015/02/12 19:22:59, Sergey Ulanov wrote:
> This doesn't need to be std::string. Style guide asks to use kConstName for
> consts.
>   const char kPolicyNamePrefix[] = "RemoteAccessHost";
>   

Done.

BTW: I initially started replying that your suggestion would repeatedly
construct std::string inside the loop, but didn't notice that std::string::find
has an overload that accepts const char* arguments.  And maybe the repeated
std::string construction would not be that bad either in this particular case
(AFAIK std::string doesn't do heap allocation for strings smaller than 32
bytes).

https://codereview.chromium.org/891053003/diff/100001/remoting/host/policy_wa...
remoting/host/policy_watcher.cc:228: // 1) comments,
On 2015/02/12 19:22:58, Sergey Ulanov wrote:
> Comments should be dropped by the parser, no?

By "comments" I meant JSON entries like these:

{
  "_comment": "$Id:
//depot/google3/googledata/corp/puppet/goobuntu/common/modules/chrome/files/chromoting.json#2
$",
  "RemoteAccessClientFirewallTraversal": false,
...
}

I've added the example to the comment.

[Łukasz Anforowicz, 2015-02-23 19:29:32]

The CQ bit was checked by lukasza@chromium.org

[commit-bot: I haz the power, 2015-02-23 19:30:09]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/891053003/140001

[commit-bot: I haz the power, 2015-02-23 19:36:14]

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

[commit-bot: I haz the power, 2015-02-23 19:36:15]

Try jobs failed on following builders:
  ios_dbg_simulator on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_dbg_simulator...)

[Łukasz Anforowicz, 2015-02-23 20:58:58]

The CQ bit was checked by lukasza@chromium.org

[commit-bot: I haz the power, 2015-02-23 20:59:46]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/891053003/140001

[commit-bot: I haz the power, 2015-02-23 21:00:43]

Committed patchset #3 (id:140001)

[commit-bot: I haz the power, 2015-02-23 21:01:16]

Patchset 3 (id:??) landed as
https://crrev.com/e5d69ea2ccfe129eccd06d3b1de43a7092ee201d
Cr-Commit-Position: refs/heads/master@{#317646}