Reporting of policy errors via host-offline-reason: part 1

remoting/host/remoting_me2me_host.cc

 // Copyright (c) 2012 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.
 //
 // This file implements a standalone host process for Me2Me.
 
 #include <string>
 
 #include "base/at_exit.h"
 #include "base/bind.h"
@@ skipping 216 matching lines @@
   void CreateAuthenticatorFactory();
 
   // Tear down resources that run on the UI thread.
   void ShutdownOnUiThread();
 
   // Applies the host config, returning true if successful.
   bool ApplyConfig(scoped_ptr<JsonHostConfig> config);
 
   // Handles policy updates, by calling On*PolicyUpdate methods.
   void OnPolicyUpdate(scoped_ptr<base::DictionaryValue> policies);
+  void OnPolicyError();
   void ApplyHostDomainPolicy();
   void ApplyUsernamePolicy();
   bool OnHostDomainPolicyUpdate(base::DictionaryValue* policies);
   bool OnUsernamePolicyUpdate(base::DictionaryValue* policies);
   bool OnNatPolicyUpdate(base::DictionaryValue* policies);
   bool OnRelayPolicyUpdate(base::DictionaryValue* policies);
   bool OnUdpPortPolicyUpdate(base::DictionaryValue* policies);
   bool OnCurtainPolicyUpdate(base::DictionaryValue* policies);
   bool OnHostTalkGadgetPrefixPolicyUpdate(base::DictionaryValue* policies);
   bool OnHostTokenUrlPolicyUpdate(base::DictionaryValue* policies);
@@ skipping 273 matching lines @@
   }
 
   if (state_ == HOST_INITIALIZING) {
     // TODO(sergeyu): Currently OnPolicyUpdate() assumes that host config is
     // already loaded so PolicyWatcher has to be started here. Separate policy
     // loading from policy verifications and move |policy_watcher_|
     // initialization to StartOnNetworkThread().
     policy_watcher_.reset(
         policy_hack::PolicyWatcher::Create(context_->file_task_runner()));
     policy_watcher_->StartWatching(
-        base::Bind(&HostProcess::OnPolicyUpdate, base::Unretained(this)));
+        base::Bind(&HostProcess::OnPolicyUpdate, base::Unretained(this)),
+        base::Bind(&HostProcess::OnPolicyError, base::Unretained(this)));
   } else {
     // Reapply policies that could be affected by a new config.
     ApplyHostDomainPolicy();
     ApplyUsernamePolicy();
 
     if (state_ == HOST_STARTED) {
       // TODO(sergeyu): Here we assume that PIN is the only part of the config
       // that may change while the service is running. Change ApplyConfig() to
       // detect other changes in the config and restart host if necessary here.
       CreateAuthenticatorFactory();
@@ skipping 75 matching lines @@
             third_party_auth_config_,
             key_pair_, context_->url_request_context_getter()));
     factory = protocol::Me2MeHostAuthenticatorFactory::CreateWithThirdPartyAuth(
         use_service_account_, host_owner_, local_certificate, key_pair_,
         token_validator_factory.Pass());
 
   } else {
     // TODO(rmsousa): If the policy is bad the host should not go online. It
     // should keep running, but not connected, until the policies are fixed.
     // Having it show up as online and then reject all clients is misleading.
+    // TODO(lukasza): code review please: the new ShutdownHost call below seems

[Lambros, 2014/11/13 00:30:27]

Please remove this. When requesting a code review, you can add your own inline
comments using the Rietveld web UI, and reviewers will see these comments in the
request.

[Łukasz Anforowicz, 2014/11/13 17:48:09]

Done.

+    //                to match the TODO from rmsousa above (at least in spirit)
+    //                If this makes sense, then I can add this call in the next
+    //                check-in.
     LOG(ERROR) << "One of the third-party token URLs is empty or invalid. "
                << "Host will reject all clients until policies are corrected. "
                << "TokenUrl: " << third_party_auth_config_.token_url << ", "
                << "TokenValidationUrl: "
                << third_party_auth_config_.token_validation_url;
     factory = protocol::Me2MeHostAuthenticatorFactory::CreateRejecting();
   }
 
 #if defined(OS_POSIX)
   // On Linux and Mac, perform a PAM authorization step after authentication.
@@ skipping 289 matching lines @@
   restart_required |= OnPairingPolicyUpdate(policies.get());
   restart_required |= OnGnubbyAuthPolicyUpdate(policies.get());
 
   if (state_ == HOST_INITIALIZING) {
     StartHost();
   } else if (state_ == HOST_STARTED && restart_required) {
     RestartHost();
   }
 }
 
+void HostProcess::OnPolicyError() {
+  context_->network_task_runner()->PostTask(
+      FROM_HERE,
+      base::Bind(

[Lambros, 2014/11/13 00:30:27]

Formatting is a bit odd - see examples elsewhere, or use clang-format. Lines
below should be +4 indented, not +2. And some stuff could be collapsed onto the
previous line.

[Łukasz Anforowicz, 2014/11/13 17:48:09]

Ok to +4 rather than +2.  Let's discuss the other things - I think single value
or name-of-called-method per line is very readable, but if there is some kind of
policy for keeping things short, I can collapse into less lines.

+        &HostProcess::ShutdownHost,
+        this,
+        kInvalidHostConfigurationExitCode));
+}
+
 void HostProcess::ApplyHostDomainPolicy() {
   HOST_LOG << "Policy sets host domain: " << host_domain_;
 
   // If the user does not have a Google email, their client JID will not be
   // based on their email. In that case, the username/host domain policies would
   // be meaningless, since there is no way to check that the JID attempting to
   // connect actually corresponds to the owner email in question.
   if (host_owner_ != host_owner_email_) {
     LOG(ERROR) << "The username and host domain policies cannot be enabled for "
                << "accounts with a non-Google email.";
@@ skipping 529 matching lines @@
   int exit_code = kSuccessExitCode;
   new HostProcess(context.Pass(), &exit_code);
 
   // Run the main (also UI) message loop until the host no longer needs it.
   message_loop.Run();
 
   return exit_code;
 }
 
 }  // namespace remoting