Restart the host when the third party auth certificate changes

remoting/host/linux/certificate_watcher.cc

+// Copyright 2016 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 "certificate_watcher.h"
+
+#include "base/bind.h"
+#include "base/bind_helpers.h"
+#include "base/location.h"
+#include "base/logging.h"
+#include "base/message_loop/message_loop.h"
+
+namespace remoting {
+
+// Full Path: $HOME/.pki/nssdb
+const char kNSSEnvironmentPrefix[] = "HOME";

[Sergey Ulanov, 2016/03/30 21:02:44]

Please use base::PathService with base::DIR_HOME to get home directory path.

[Yuwei, 2016/03/31 17:40:03]

Done.

+const char kNSSWatchPathToHome[] = "/.pki/nssdb";

[Sergey Ulanov, 2016/03/30 21:02:44]

Maybe call this kNssCertDirectoryPath - notice lower-case 'ss'

[Yuwei, 2016/03/31 17:40:03]

Done.

+
+CertificateWatcher::CertificateWatcher(int delay,
+                                       const base::Closure& restart_action)
+    : CertificateWatcher(delay, restart_action,
+                         base::Bind(&base::DoNothing),
+                         base::FilePath(getenv(kNSSEnvironmentPrefix) +
+                                        std::string(kNSSWatchPathToHome))) {}
+
+CertificateWatcher::CertificateWatcher(
+    int delay,
+    const base::Closure& restart_action,
+    const base::Closure& restart_deferred_action, const base::FilePath& path)

[Sergey Ulanov, 2016/03/30 21:02:44]

one argument per line please (clang-format?)

[Yuwei, 2016/03/30 21:37:00]

Okay... In fact this file has been processed by clang-format...

+    : delay_(delay),
+      restart_action_(restart_action),
+      restart_deferred_action_(restart_deferred_action),
+      nss_watch_path_(path) {
+  if (monitor_) {
+    monitor_->AddStatusObserver(this);
+  }
+}
+
+void CertificateWatcher::OnNSSUpdate(const base::FilePath& path, bool error) {
+  if (error || path != nss_watch_path_) {
+    LOG(WARNING) << "Unexpected file update callback. "
+                 << "Path: " << path.MaybeAsASCII() << "; "
+                 << "Error: " << error;
+    return;
+  }
+
+  if (inhibit_mode_) {

[Sergey Ulanov, 2016/03/30 21:02:44]

I think we want to always start the timer here. Then OnTimer() should check if
there is an active client and defer restart until that client is disconnected.
E.g. currently this code doesn't handle the case when a new client is connected
between OnNssUpdate() and OnTimer().

[Yuwei, 2016/03/30 21:37:00]

This sounds better although I don't think the client can still successfully
connect to the host after OnNSSUpdate happens...

[Yuwei, 2016/03/31 17:40:03]

Done.

+    inhibit_restart_scheduled_ = true;
+    restart_deferred_action_.Run();
+    return;
+  }
+
+  restart_timer_->Reset();
+}
+
+void CertificateWatcher::OnTimer() {
+  LOG(INFO) << "NSS certificate was updated. Calling restart...";
+  restart_action_.Run();
+}
+
+CertificateWatcher::~CertificateWatcher() {
+  Stop();
+}
+
+void CertificateWatcher::StartOn(scoped_refptr<AutoThreadTaskRunner> runner,
+                               base::WeakPtr<HostStatusMonitor> monitor) {
+  runner->PostTask(FROM_HERE, base::Bind(&CertificateWatcher::Start,
+                                         base::Unretained(this),
+                                         monitor));
+}
+
+void CertificateWatcher::Start(base::WeakPtr<HostStatusMonitor> monitor) {
+  monitor_ = monitor;
+  file_watcher_.reset(new base::FilePathWatcher());
+  file_watcher_->Watch(
+      nss_watch_path_, true,
+      base::Bind(&CertificateWatcher::OnNSSUpdate, base::Unretained(this)));
+
+  auto raw_timer =
+      new base::DelayTimer(FROM_HERE, base::TimeDelta::FromSeconds(delay_),
+                           this, &CertificateWatcher::OnTimer);
+  restart_timer_.reset(raw_timer);
+
+  LOG(INFO) << "Started watching certificate changes.";

[Sergey Ulanov, 2016/03/30 21:02:44]

I don't think you need this comment. At least not as LOG(INFO). Maybe replace it
with VLOG(1)

[Yuwei, 2016/03/31 17:40:04]

Done.

+}
+
+void CertificateWatcher::Stop() {

[Sergey Ulanov, 2016/03/30 21:02:44]

This method is not used anywhere except the destructor. Just move this code to
the destructor. 

[Yuwei, 2016/03/31 17:40:03]

Done.

+  file_watcher_.reset();
+  restart_timer_.reset();
+
+  if (monitor_) {
+    monitor_->RemoveStatusObserver(this);
+  }
+
+  LOG(INFO) << "Stopped watching certificate changes.";

[Sergey Ulanov, 2016/03/30 21:02:44]

same as above.

[Yuwei, 2016/03/31 17:40:04]

Done.

+}
+
+void CertificateWatcher::Inhibit() { inhibit_mode_ = true; }
+
+void CertificateWatcher::Uninhibit() {
+  inhibit_mode_ = false;
+  if (inhibit_restart_scheduled_) {
+    restart_timer_->Reset();

[Sergey Ulanov, 2016/03/30 21:02:44]

I don't think you really want to activate the timer in this case. The timer is
needed because the certs directory may keep changing. But that doesn't apply to
the case when the client disconnects. I think if the certs were updated while a
client was connected then you want to restart the host right after the client
disconnects (provided that 2 seconds interval has passed since the last change)

[Yuwei, 2016/03/31 17:40:03]

Logic changed so now we don't need to call restart when inhibit turns off...

+    LOG(INFO) << "Certificate was updated in inhibit mode. "
+                 "Scheduled a restart.";
+  }
+}
+
+// HostStatusObserver Impl:

[Sergey Ulanov, 2016/03/30 21:02:44]

remove this comment

[Yuwei, 2016/03/31 17:40:04]

Done.

+void CertificateWatcher::OnClientConnected(const std::string& jid) {
+  Inhibit();
+}
+
+void CertificateWatcher::OnClientDisconnected(const std::string& jid) {
+  Uninhibit();
+}
+
+}  // namespace remoting