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"

[Sergey Ulanov, 2016/04/04 19:29:37]

Specify pull path here please.

[Yuwei, 2016/04/04 21:40:24]

full path I guess?

[Sergey Ulanov, 2016/04/04 21:44:38]

yes

[Yuwei, 2016/04/04 23:42:24]

Done.

+
+#include "base/bind.h"
+#include "base/bind_helpers.h"
+#include "base/location.h"
+#include "base/logging.h"
+#include "base/message_loop/message_loop.h"
+#include "base/path_service.h"
+
+namespace remoting {
+
+// Delay time to restart the host when a change of certificate is detected.
+// This is to avoid repeating restarts when continuous writes to the database
+// occur.
+const int kRestartDelayInSecond = 2;
+
+// Full Path: $HOME/.pki/nssdb
+const char kCertDirectoryPath[] = ".pki/nssdb";
+
+CertificateWatcher::CertificateWatcher(
+    const base::Closure& restart_action,
+    scoped_refptr<base::SingleThreadTaskRunner> network_runner,
+    scoped_refptr<base::SingleThreadTaskRunner> io_runner)
+    : restart_action_(restart_action),
+      network_runner_(network_runner),

[Sergey Ulanov, 2016/04/04 19:29:37]

Don't need to pass it as parameter explicitly. Instead use
base::ThreadTaskRunnerHandle::Get() to get task runner for the current thread.

[Yuwei, 2016/04/04 23:42:24]

Done.

+      io_runner_(io_runner),

[Sergey Ulanov, 2016/04/04 19:29:37]

Call this io_task_runner_.

[Yuwei, 2016/04/04 23:42:24]

Done.

+      delay_(base::TimeDelta::FromSeconds(kRestartDelayInSecond)) {
+  if (!base::PathService::Get(base::DIR_HOME, &cert_watch_path_)) {
+    LOG(ERROR) << "Failed to get path of the home directory. ";
+    return;
+  }
+

[Sergey Ulanov, 2016/04/04 19:29:37]

nit: remove this empty line. Otherwise the following line looks as if it's not
related to the code above.

[Yuwei, 2016/04/04 23:42:24]

Done.

+   cert_watch_path_ = cert_watch_path_.AppendASCII(kCertDirectoryPath);

[Sergey Ulanov, 2016/04/04 19:29:37]

nit: incorrect indentation.

[Yuwei, 2016/04/04 23:42:24]

Done.

+}
+
+void CertificateWatcher::OnCertDirectoryChanged(const base::FilePath& path,
+                                                bool error) {
+  DCHECK(io_runner_->BelongsToCurrentThread());
+
+  if (error || path != cert_watch_path_) {
+    LOG(WARNING) << "Unexpected file update callback. "
+                 << "Path: " << path.MaybeAsASCII() << "; "
+                 << "Error: " << error;
+    return;
+  }
+
+  network_runner_->PostTask(
+      FROM_HERE, base::Bind(&base::DelayTimer::Reset,
+                            base::Unretained(restart_timer_.get())));
+}
+
+void CertificateWatcher::OnTimer() {
+  DCHECK(network_runner_->BelongsToCurrentThread());
+
+  if (inhibit_mode_) {
+    restart_pending_ = true;
+    return;
+  }
+
+  VLOG(1) << "Certificate was updated. Calling restart...";
+  restart_action_.Run();
+}
+
+CertificateWatcher::~CertificateWatcher() {
+  if (monitor_) {
+    monitor_->RemoveStatusObserver(this);
+  }
+
+  VLOG(1) << "Stopped watching certificate changes.";
+}
+
+void CertificateWatcher::Start() {

[Sergey Ulanov, 2016/04/04 19:29:37]

Do you really need this separate from the constructor? I suggest moving all this
code to the constructor.

[Yuwei, 2016/04/04 21:45:20]

It's basically to allow unit tests to override parameters before starting the
test. If we move this to the constructor then we need to have separate
constructor for testing...

[Sergey Ulanov, 2016/04/04 21:47:15]

I see. Then maybe make it return a bool value (false in case it fails). Then you
can make is_started() private or remove it completely.

[Yuwei, 2016/04/04 23:42:24]

Done.

+  if (cert_watch_path_.empty()) {
+    LOG(ERROR) << "Failed to start watching. "

[Sergey Ulanov, 2016/04/04 19:29:37]

This essentially duplicates the error you have in the constructor.  You can
avoid this by moving all code to the constructor.

+               << "The certificate path is not correctly set up.";
+    return;
+  }
+
+  io_runner_->PostTask(FROM_HERE, base::Bind(
+      &CertificateWatcher::StartWatcherOnIo, base::Unretained(this)));

[Sergey Ulanov, 2016/04/04 19:29:37]

base::Unretained() is not safe here. CertificateWatcher may be destroyed by the
time this task gets executed on the IO thread. Instead you can create
FilePathWatcher here and post a task directly for the Watch() method:

file_watcher_.reset(new base::FilePathWatcher());
io_runner_->PostTask(FROM_HERE, base::Bind(
       &FilePathWatcher::Watch, base::Unretained(file_watcher.get()),
       cert_watch_path_, true, base::Bind(&OnCertDirectoryChanged));

Then from the destructor post a task on io thread to delete the watcher:
  io_runner_->DeleteSoon(FROM_HERE, file_watcher_.release());

[Yuwei, 2016/04/04 23:42:24]

Done.

+
+  network_runner_->PostTask(FROM_HERE, base::Bind(
+      &CertificateWatcher::StartTimerOnNetwork, base::Unretained(this)));

[Sergey Ulanov, 2016/04/04 19:29:37]

Why do you need to post a separate task here? network_runner_ must be the same
thread on which this code is executed.

[Yuwei, 2016/04/04 21:40:24]

I was thinking it's the main thread calling this function... Looks like I can
clean up the code a little bit.

[Yuwei, 2016/04/04 23:42:24]

Not posting task now

+
+  VLOG(1) << "Started watching certificate changes.";
+}
+
+void CertificateWatcher::StartWatcherOnIo() {
+  DCHECK(io_runner_->BelongsToCurrentThread());
+
+  file_watcher_.reset(new base::FilePathWatcher());
+  file_watcher_->Watch(
+      cert_watch_path_, true,
+      base::Bind(&CertificateWatcher::OnCertDirectoryChanged,
+                 base::Unretained(this)));

[Sergey Ulanov, 2016/04/04 19:29:37]

It's not safe to use base::Unretained() here. OnCertDirectoryChanged() is called
on the IO thread, while CertificateWatcher is destroyed on the network thread. I
suggest you make OnCertDirectoryChanged a static method that gets a weak pointer
for the CertficateWatcher
to post the task back to the network thread:

OnCertDirectoryChanged(scoped_refptr<TaskRunner> network_task_runner,
                       base::WeakPtr<CertificateWatcher> cert_watcher,
                       const base::FilePath& path,
                       bool error) {
  network_task_runner->PostTask(
        FROM_HERE, 
        base::Bind(&CertificateWatcher::DirectoryChanged, cert_watcher,
                   path, error));
}

You will need to bind the first two parameters for the callback passed to
FilePathWatcher::Watch().

[Yuwei, 2016/04/04 23:42:24]

Done.

+}
+
+void CertificateWatcher::StartTimerOnNetwork() {
+  DCHECK(network_runner_->BelongsToCurrentThread());
+
+  restart_timer_.reset(new base::DelayTimer(FROM_HERE, delay_, this,
+                                            &CertificateWatcher::OnTimer));
+}
+
+bool CertificateWatcher::Started() const {
+  return file_watcher_ != nullptr;
+}
+
+void CertificateWatcher::SetMonitor(base::WeakPtr<HostStatusMonitor> monitor) {
+  if (monitor_) {
+    monitor_->RemoveStatusObserver(this);
+  }
+  if (monitor) {

[Sergey Ulanov, 2016/04/04 19:29:37]

I don't think this you need this condition: monitor must exist when SetMonitor()
is called.

[Yuwei, 2016/04/04 23:42:24]

Changed to DCHECK

+    monitor->AddStatusObserver(this);
+  }
+  monitor_ = monitor;
+}
+
+void CertificateWatcher::OnClientConnected(const std::string& jid) {
+  if (!Started()) {
+    return;
+  }
+  DCHECK(network_runner_->BelongsToCurrentThread());
+  inhibit_mode_ = true;
+}
+
+void CertificateWatcher::OnClientDisconnected(const std::string& jid) {
+  if (!Started()) {
+    return;
+  }
+  DCHECK(network_runner_->BelongsToCurrentThread());
+  inhibit_mode_ = false;
+  if (restart_pending_) {
+    restart_pending_ = false;
+    restart_action_.Run();
+  }
+}
+
+void CertificateWatcher::SetDelayForTests(const base::TimeDelta& delay) {
+  DCHECK(!Started());
+  delay_ = delay;
+}
+
+void CertificateWatcher::SetWatchPathForTests(
+    const base::FilePath& watch_path) {
+  DCHECK(!Started());
+  cert_watch_path_ = watch_path;
+}
+
+}  // namespace remoting