Refactor the recovery component action handler for Windows.

This CL refactors the implementation of the recovery component action
handler for Windows so that most of the code can be reused in the
future for macOS recovery component. It also adds a unit test.

This is trying to reland
https://chromium-review.googlesource.com/c/chromium/src/+/1946020
which failed on Linux with:

recovery_improved_component_installer.cc:94:11: error: no member named 'start_hidden' in 'base::LaunchOptions'
options.start_hidden = true;
~~~~~~~ ^

Bug: 1029953
Change-Id: I685963ed8b281d5709867c55b73617a121e7b78b
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1949224Reviewed-by: Joshua Pawlicki <waffles@chromium.org>
Commit-Queue: Sorin Jianu <sorin@chromium.org>
Cr-Commit-Position: refs/heads/master@{#721257}

chrome/browser/BUILD.gn

@@ -330,6 +330,7 @@ jumbo_static_library("browser") {
     "component_updater/recovery_component_installer.h",
     "component_updater/recovery_improved_component_installer.cc",
     "component_updater/recovery_improved_component_installer.h",
+    "component_updater/recovery_improved_component_installer_win.cc",
     "component_updater/safety_tips_component_installer.cc",
     "component_updater/safety_tips_component_installer.h",
     "component_updater/ssl_error_assistant_component_installer.cc",

chrome/browser/component_updater/recovery_improved_component_installer.h

@@ -11,22 +11,140 @@
 #include <vector>
 
 #include "base/feature_list.h"
+#include "base/files/file_path.h"
+#include "base/memory/ref_counted.h"
+#include "base/task/task_traits.h"
+#include "base/threading/sequenced_task_runner_handle.h"
 #include "components/component_updater/component_installer.h"
+#include "components/crx_file/crx_verifier.h"
+#include "components/update_client/component_unpacker.h"
+#include "components/update_client/update_client.h"
 
 namespace base {
-class FilePath;
+class CommandLine;
+class Process;
 }  // namespace base
 
 class PrefService;
 
 namespace component_updater {
 
+// Handles the |run| action for the recovery component.
+//
+// The recovery component consists of a executable program (the name of the
+// program is known at build time but it depends on the operating system),
+// wrapped inside a CRX container, which CRX is itself contained inside a
+// component updater CRX payload. In other words, looking from outside, the
+// component updater installs and updates a CRX, containing a CRX, containing
+// a program. When this program is run, it can restore the functionality of
+// the Chrome updater.
+
+// The |RecoveryComponentActionHandler| is responsible for unpacking the inner
+// CRX described above, and running the executable program inside it.
+//
+// The |RecoveryComponentActionHandler::Handle| function is invoked as a result
+// of an |action| element present in the update response for the recovery
+// component. Note that the |action| element can be present in the update
+// response even if there are no updates for the recovery component.
+//
+// When Chrome is installed per-system, the CRX is being handed over to
+// a system elevator, which unpacks the CRX in a secure location of the
+// file system, and runs the recovery program with system privileges.
+//
+// When Chrome is installed per-user, the CRX is unpacked in a temporary
+// directory for the user, and the recovery program runs with normal user
+// privileges.
+class RecoveryComponentActionHandler : public update_client::ActionHandler {
+ public:
+  static scoped_refptr<update_client::ActionHandler> MakeActionHandler();
+
+  // Overrides for update_client::RecoveryComponentActionHandler. |action| is an
+  // absolute file path to a CRX to be unpacked. |session_id| contains the
+  // session id corresponding to the current update transaction. The session id
+  // is passed as a command line argument to the recovery program, and sent as
+  // part of the completion pings during the actual recovery.
+  void Handle(const base::FilePath& action,
+              const std::string& session_id,
+              Callback callback) override;
+
+ protected:
+  static constexpr base::TaskTraits kTaskTraits = {
+      base::ThreadPool(), base::MayBlock(), base::TaskPriority::BEST_EFFORT,
+      base::TaskShutdownBehavior::SKIP_ON_SHUTDOWN};
+
+  // This task joins a process, hence .WithBaseSyncPrimitives().
+  static constexpr base::TaskTraits kTaskTraitsRunCommand = {
+      base::ThreadPool(), base::MayBlock(), base::WithBaseSyncPrimitives(),
+      base::TaskPriority::BEST_EFFORT,
+      base::TaskShutdownBehavior::CONTINUE_ON_SHUTDOWN};
+
+  // Accepts only the production key hash and the production proof by default.
+  RecoveryComponentActionHandler(
+      const std::vector<uint8_t>& key_hash = {std::begin(kKeyHash),
+                                              std::end(kKeyHash)},
+      crx_file::VerifierFormat verifier_format =
+          crx_file::VerifierFormat::CRX3_WITH_PUBLISHER_PROOF);
+  ~RecoveryComponentActionHandler() override;
+
+  base::FilePath crx_path() const { return crx_path_; }
+
+  std::string session_id() const { return session_id_; }
+
+  scoped_refptr<base::SequencedTaskRunner> main_task_runner() {
+    return main_task_runner_;
+  }
+
+ private:
+  // The production public key hash for the inner CRX.
+  static constexpr uint8_t kKeyHash[] = {
+      0x5f, 0x94, 0xe0, 0x3c, 0x64, 0x30, 0x9f, 0xbc, 0xfe, 0x00, 0x9a,
+      0x27, 0x3e, 0x52, 0xbf, 0xa5, 0x84, 0xb9, 0xb3, 0x75, 0x07, 0x29,
+      0xde, 0xfa, 0x32, 0x76, 0xd9, 0x93, 0xb5, 0xa3, 0xce, 0x02};
+
+  virtual base::CommandLine MakeCommandLine(
+      const base::FilePath& unpack_path) const = 0;
+  virtual void Elevate(Callback callback) = 0;
+
+  void Unpack();
+  void UnpackComplete(const update_client::ComponentUnpacker::Result& result);
+  void RunCommand(const base::CommandLine& cmdline);
+  void WaitForCommand(base::Process process);
+
+  SEQUENCE_CHECKER(sequence_checker_);
+
+  // Executes tasks in the context of the sequence which created this object.
+  scoped_refptr<base::SequencedTaskRunner> main_task_runner_ =
+      base::SequencedTaskRunnerHandle::Get();
+
+  // The key hash and its proof for the inner CRX to be unpacked and run.
+  const std::vector<uint8_t> key_hash_;
+  const crx_file::VerifierFormat verifier_format_;
+
+  // Contains the CRX specified as a run action.
+  base::FilePath crx_path_;
+
+  // The session id of the update transaction, as defined by |update_client|.
+  std::string session_id_;
+
+  // Called when the action is handled.
+  Callback callback_;
+
+  // Contains the path where the action CRX is unpacked in the per-user case.
+  base::FilePath unpack_path_;
+
+  RecoveryComponentActionHandler(const RecoveryComponentActionHandler&) =
+      delete;
+  RecoveryComponentActionHandler& operator=(
+      const RecoveryComponentActionHandler&) = delete;
+};
+
 class ComponentUpdateService;
 
 class RecoveryImprovedInstallerPolicy : public ComponentInstallerPolicy {
  public:
-  explicit RecoveryImprovedInstallerPolicy(PrefService* prefs);
-  ~RecoveryImprovedInstallerPolicy() override;
+  explicit RecoveryImprovedInstallerPolicy(PrefService* prefs)
+      : prefs_(prefs) {}
+  ~RecoveryImprovedInstallerPolicy() override = default;
 
  private:
   friend class RecoveryImprovedInstallerTest;

chrome/browser/component_updater/recovery_improved_component_installer_win.cc

+// Copyright (c) 2019 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 "build/branding_buildflags.h"
+#if BUILDFLAG(GOOGLE_CHROME_BRANDING)
+
+#include <windows.h>
+#include <wrl/client.h>
+
+#include <tuple>
+#include <utility>
+
+#include "base/bind.h"
+#include "base/callback.h"
+#include "base/command_line.h"
+#include "base/files/file_path.h"
+#include "base/logging.h"
+#include "base/memory/ref_counted.h"
+#include "base/process/process.h"
+#include "base/single_thread_task_runner.h"
+#include "base/strings/utf_string_conversions.h"
+#include "base/task/post_task.h"
+#include "chrome/browser/component_updater/component_updater_utils.h"
+#include "chrome/browser/component_updater/recovery_improved_component_installer.h"
+#include "chrome/elevation_service/elevation_service_idl.h"
+#include "chrome/install_static/install_util.h"
+#include "components/version_info/version_info.h"
+
+namespace component_updater {
+
+namespace {
+
+const base::FilePath::CharType kRecoveryFileName[] =
+    FILE_PATH_LITERAL("ChromeRecovery.exe");
+
+// Returns the Chrome's appid registered with Google Update for updates.
+std::string GetBrowserAppId() {
+  return base::UTF16ToUTF8(install_static::GetAppGuid());
+}
+
+// Returns the current browser version.
+std::string GetBrowserVersion() {
+  return version_info::GetVersion().GetString();
+}
+
+// Instantiates the elevator service, calls its elevator interface, then
+// blocks waiting for the recovery processes to exit. Returns the result
+// of the recovery as a tuple.
+std::tuple<bool, int, int> RunRecoveryCRXElevated(
+    const base::FilePath& crx_path,
+    const std::string& browser_appid,
+    const std::string& browser_version,
+    const std::string& session_id) {
+  Microsoft::WRL::ComPtr<IElevator> elevator;
+  HRESULT hr = CoCreateInstance(
+      install_static::GetElevatorClsid(), nullptr, CLSCTX_LOCAL_SERVER,
+      install_static::GetElevatorIid(), IID_PPV_ARGS_Helper(&elevator));
+  if (FAILED(hr))
+    return {false, static_cast<int>(hr), 0};
+
+  hr = CoSetProxyBlanket(
+      elevator.Get(), RPC_C_AUTHN_DEFAULT, RPC_C_AUTHZ_DEFAULT,
+      COLE_DEFAULT_PRINCIPAL, RPC_C_AUTHN_LEVEL_PKT_PRIVACY,
+      RPC_C_IMP_LEVEL_IMPERSONATE, nullptr, EOAC_DYNAMIC_CLOAKING);
+  if (FAILED(hr))
+    return {false, static_cast<int>(hr), 0};
+
+  ULONG_PTR proc_handle = 0;
+  hr = elevator->RunRecoveryCRXElevated(
+      crx_path.value().c_str(), base::UTF8ToWide(browser_appid).c_str(),
+      base::UTF8ToWide(browser_version).c_str(),
+      base::UTF8ToWide(session_id).c_str(), base::Process::Current().Pid(),
+      &proc_handle);
+  if (FAILED(hr))
+    return {false, static_cast<int>(hr), 0};
+
+  int exit_code = 0;
+  const base::TimeDelta kMaxWaitTime = base::TimeDelta::FromSeconds(600);
+  base::Process process(reinterpret_cast<base::ProcessHandle>(proc_handle));
+  const bool succeeded =
+      process.WaitForExitWithTimeout(kMaxWaitTime, &exit_code);
+  return {succeeded, exit_code, 0};
+}
+
+// Handles the |run| action for the recovery component for Windows.
+class RecoveryComponentActionHandlerWin
+    : public RecoveryComponentActionHandler {
+ public:
+  RecoveryComponentActionHandlerWin() = default;
+
+ private:
+  ~RecoveryComponentActionHandlerWin() override = default;
+
+  // Overrides for RecoveryComponentActionHandler.
+  base::CommandLine MakeCommandLine(
+      const base::FilePath& unpack_path) const override;
+  void Elevate(Callback callback) override;
+
+  // Calls the elevator service to handle the CRX. Since the invocation of
+  // the elevator service consists of several Windows COM IPC calls, a
+  // certain type of task runner is necessary to initialize a COM apartment.
+  void RunElevatedInSTA(Callback callback);
+
+  RecoveryComponentActionHandlerWin(const RecoveryComponentActionHandlerWin&) =
+      delete;
+  RecoveryComponentActionHandlerWin& operator=(
+      const RecoveryComponentActionHandlerWin&) = delete;
+};
+
+base::CommandLine RecoveryComponentActionHandlerWin::MakeCommandLine(
+    const base::FilePath& unpack_path) const {
+  base::CommandLine command_line(unpack_path.Append(kRecoveryFileName));
+  command_line.AppendSwitchASCII("browser-version", GetBrowserVersion());
+  command_line.AppendSwitchASCII("sessionid", session_id());
+  const auto app_guid = GetBrowserAppId();
+  if (!app_guid.empty())
+    command_line.AppendSwitchASCII("appguid", app_guid);
+  return command_line;
+}
+
+void RecoveryComponentActionHandlerWin::Elevate(Callback callback) {
+  base::CreateCOMSTATaskRunner(
+      kTaskTraitsRunCommand, base::SingleThreadTaskRunnerThreadMode::DEDICATED)
+      ->PostTask(
+          FROM_HERE,
+          base::BindOnce(&RecoveryComponentActionHandlerWin::RunElevatedInSTA,
+                         this, std::move(callback)));
+}
+
+void RecoveryComponentActionHandlerWin::RunElevatedInSTA(Callback callback) {
+  bool succeeded = false;
+  int error_code = 0;
+  int extra_code = 0;
+  std::tie(succeeded, error_code, extra_code) = RunRecoveryCRXElevated(
+      crx_path(), GetBrowserAppId(), GetBrowserVersion(), session_id());
+  main_task_runner()->PostTask(
+      FROM_HERE,
+      base::BindOnce(std::move(callback), succeeded, error_code, extra_code));
+}
+
+}  // namespace
+
+scoped_refptr<update_client::ActionHandler>
+RecoveryComponentActionHandler::MakeActionHandler() {
+  return base::MakeRefCounted<RecoveryComponentActionHandlerWin>();
+}
+
+}  // namespace component_updater
+
+#endif  // GOOGLE_CHROME_BRANDING

chrome/browser/component_updater/recovery_improved_component_unittest.cc

+// Copyright 2019 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 <cstdint>
+#include <iterator>
+#include <string>
+#include <utility>
+
+#include "base/bind.h"
+#include "base/callback.h"
+#include "base/command_line.h"
+#include "base/files/file_path.h"
+#include "base/files/file_util.h"
+#include "base/files/scoped_temp_dir.h"
+#include "base/memory/ref_counted.h"
+#include "base/path_service.h"
+#include "base/test/task_environment.h"
+#include "build/build_config.h"
+#include "chrome/browser/component_updater/recovery_improved_component_installer.h"
+#include "components/crx_file/crx_verifier.h"
+#include "components/services/unzip/content/unzip_service.h"
+#include "components/services/unzip/in_process_unzipper.h"
+#include "components/update_client/update_client_errors.h"
+#include "testing/gtest/include/gtest/gtest.h"
+#include "testing/platform_test.h"
+
+namespace component_updater {
+
+namespace {
+
+// The public key hash for the test CRX.
+constexpr uint8_t kKeyHashTest[] = {
+    0x69, 0xfc, 0x41, 0xf6, 0x17, 0x20, 0xc6, 0x36, 0x92, 0xcd, 0x95,
+    0x76, 0x69, 0xf6, 0x28, 0xcc, 0xbe, 0x98, 0x4b, 0x93, 0x17, 0xd6,
+    0x9c, 0xb3, 0x64, 0x0c, 0x0d, 0x25, 0x61, 0xc5, 0x80, 0x1d};
+
+class RecoveryImprovedActionHandlerTest : public PlatformTest {
+ public:
+  RecoveryImprovedActionHandlerTest() = default;
+
+ protected:
+  base::ScopedTempDir temp_dir_;
+
+ private:
+  base::test::TaskEnvironment task_environment_;
+};
+
+class TestActionHandler : public RecoveryComponentActionHandler {
+ public:
+  // Accepts a test CRX without a production publisher proof.
+  TestActionHandler()
+      : RecoveryComponentActionHandler(
+            {std::begin(kKeyHashTest), std::end(kKeyHashTest)},
+            crx_file::VerifierFormat::CRX3) {}
+
+ protected:
+  ~TestActionHandler() override = default;
+
+ private:
+  // Overrides for RecoveryComponentActionHandler.
+  base::CommandLine MakeCommandLine(
+      const base::FilePath& unpack_path) const override;
+  void Elevate(Callback callback) override;
+
+  TestActionHandler(const TestActionHandler&) = delete;
+  TestActionHandler& operator=(const TestActionHandler&) = delete;
+};
+
+base::CommandLine TestActionHandler::MakeCommandLine(
+    const base::FilePath& unpack_path) const {
+  base::CommandLine command_line(
+      unpack_path.Append(FILE_PATH_LITERAL("ChromeRecovery.exe")));
+  return command_line;
+}
+
+// This test fixture only tests the per-user execution flow.
+void TestActionHandler::Elevate(Callback callback) {
+  NOTREACHED();
+}
+
+}  // namespace
+
+#if defined(OS_WIN)
+TEST_F(RecoveryImprovedActionHandlerTest, Handle) {
+  unzip::SetUnzipperLaunchOverrideForTesting(
+      base::BindRepeating(&unzip::LaunchInProcessUnzipper));
+
+  // Tests the error is propagated through the callback in the error case.
+  {
+    base::RunLoop runloop;
+    base::MakeRefCounted<TestActionHandler>()->Handle(
+        base::FilePath{FILE_PATH_LITERAL("not-found")}, "some-session-id",
+        base::BindOnce(
+            [](base::OnceClosure quit_closure, bool succeeded, int error_code,
+               int extra_code1) {
+              EXPECT_FALSE(succeeded);
+              EXPECT_EQ(update_client::UnpackerError::kInvalidFile,
+                        static_cast<update_client::UnpackerError>(error_code));
+              EXPECT_EQ(2, extra_code1);
+              std::move(quit_closure).Run();
+            },
+            runloop.QuitClosure()));
+    runloop.Run();
+  }
+
+  // Tests that the recovery program runs and it returns an expected value.
+  {
+    constexpr char kActionRunFileName[] = "ChromeRecovery.crx3";
+    base::FilePath from_path;
+    base::PathService::Get(base::DIR_SOURCE_ROOT, &from_path);
+    from_path = from_path.AppendASCII("components")
+                    .AppendASCII("test")
+                    .AppendASCII("data")
+                    .AppendASCII("update_client")
+                    .AppendASCII(kActionRunFileName);
+    ASSERT_TRUE(temp_dir_.CreateUniqueTempDir());
+    const base::FilePath to_path =
+        temp_dir_.GetPath().AppendASCII(kActionRunFileName);
+    ASSERT_TRUE(base::CopyFile(from_path, to_path));
+
+    base::RunLoop runloop;
+    base::MakeRefCounted<TestActionHandler>()->Handle(
+        to_path, "some-session-id",
+        base::BindOnce(
+            [](base::OnceClosure quit_closure, bool succeeded, int error_code,
+               int extra_code1) {
+              EXPECT_TRUE(succeeded);
+              EXPECT_EQ(1877345072, error_code);
+              EXPECT_EQ(0, extra_code1);
+              std::move(quit_closure).Run();
+            },
+            runloop.QuitClosure()));
+    runloop.Run();
+  }
+}
+#endif  //  OS_WIN
+
+}  // namespace component_updater

chrome/test/BUILD.gn

@@ -5239,6 +5239,14 @@ test("unit_tests") {
       "../browser/ui/webui/tab_strip/thumbnail_tracker_unittest.cc",
     ]
   }
+
+  if (is_win && is_chrome_branded) {
+    sources += [
+      "../browser/component_updater/recovery_improved_component_unittest.cc",
+    ]
+    data_deps +=
+        [ "//components/update_client:recovery_component_tests_bundle_data" ]
+  }
 }
 
 static_library("test_support_unit") {

components/update_client/BUILD.gn

@@ -194,6 +194,18 @@ bundle_data("unit_tests_bundle_data") {
   ]
 }
 
+bundle_data("recovery_component_tests_bundle_data") {
+  visibility = [ "//chrome/test:unit_tests" ]
+  testonly = true
+  sources = [
+    "//components/test/data/update_client/ChromeRecovery.crx3",
+  ]
+  outputs = [
+    "{{bundle_resources_dir}}/" +
+        "{{source_root_relative_dir}}/{{source_file_part}}",
+  ]
+}
+
 source_set("unit_tests") {
   testonly = true
   sources = [