Wait for pref/bgpage in ExtensionForceInstallMixin

Add ability to wait for the given events into
ExtensionForceInstallMixin:
* the pref gets updated;
* the extension's background page is loaded for the first time.

Migrate the tests in
challenge_response_auth_keys_loader_browsertest.cc onto
ExtensionForceInstallMixin (instead of
TestCertificateProviderExtensionLoginScreenMixin).

Bug: 1090941
Change-Id: I2b71546564b99a682a74fef70abc243d3e198c8c
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2312640
Commit-Queue: Maksim Ivanov <emaxx@chromium.org>
Reviewed-by: Omar Morsi <omorsi@google.com>
Reviewed-by: Fabian Sommer <fabiansommer@chromium.org>
Cr-Commit-Position: refs/heads/master@{#791215}

chrome/browser/chromeos/certificate_provider/test_certificate_provider_extension.cc

@@ -14,9 +14,11 @@
 #include "base/json/json_writer.h"
 #include "base/logging.h"
 #include "base/numerics/safe_conversions.h"
+#include "base/path_service.h"
 #include "base/strings/string_piece.h"
 #include "base/strings/string_util.h"
 #include "base/threading/thread_restrictions.h"
+#include "chrome/common/chrome_paths.h"
 #include "chrome/common/extensions/api/certificate_provider.h"
 #include "content/public/browser/browser_context.h"
 #include "content/public/browser/notification_details.h"
@@ -37,6 +39,11 @@
 namespace {
 
 constexpr char kExtensionId[] = "ecmhnokcdiianioonpgakiooenfnonid";
+// Paths relative to |chrome::DIR_TEST_DATA|:
+constexpr base::FilePath::CharType kExtensionPath[] =
+    FILE_PATH_LITERAL("extensions/test_certificate_provider/extension/");
+constexpr base::FilePath::CharType kExtensionPemPath[] =
+    FILE_PATH_LITERAL("extensions/test_certificate_provider/extension.pem");
 
 // List of algorithms that the extension claims to support for the returned
 // certificates.
@@ -123,6 +130,18 @@ extensions::ExtensionId TestCertificateProviderExtension::extension_id() {
   return kExtensionId;
 }
 
+// static
+base::FilePath TestCertificateProviderExtension::GetExtensionSourcePath() {
+  return base::PathService::CheckedGet(chrome::DIR_TEST_DATA)
+      .Append(kExtensionPath);
+}
+
+// static
+base::FilePath TestCertificateProviderExtension::GetExtensionPemPath() {
+  return base::PathService::CheckedGet(chrome::DIR_TEST_DATA)
+      .Append(kExtensionPemPath);
+}
+
 // static
 scoped_refptr<net::X509Certificate>
 TestCertificateProviderExtension::GetCertificate() {

chrome/browser/chromeos/certificate_provider/test_certificate_provider_extension.h

@@ -21,6 +21,7 @@
 #include "third_party/boringssl/src/include/openssl/evp.h"
 
 namespace base {
+class FilePath;
 class Value;
 }
 
@@ -41,6 +42,8 @@ class TestCertificateProviderExtension final
     : public content::NotificationObserver {
  public:
   static extensions::ExtensionId extension_id();
+  static base::FilePath GetExtensionSourcePath();
+  static base::FilePath GetExtensionPemPath();
   // Returns the certificate provided by the extension.
   static scoped_refptr<net::X509Certificate> GetCertificate();
   static std::string GetCertificateSpki();

chrome/browser/chromeos/login/challenge_response_auth_keys_loader_browsertest.cc

@@ -10,23 +10,21 @@
 #include "base/scoped_observer.h"
 #include "base/test/bind_test_util.h"
 #include "chrome/browser/chromeos/certificate_provider/test_certificate_provider_extension.h"
-#include "chrome/browser/chromeos/certificate_provider/test_certificate_provider_extension_login_screen_mixin.h"
 #include "chrome/browser/chromeos/login/test/device_state_mixin.h"
 #include "chrome/browser/chromeos/login/test/oobe_base_test.h"
 #include "chrome/browser/chromeos/profiles/profile_helper.h"
+#include "chrome/browser/policy/extension_force_install_mixin.h"
 #include "chrome/browser/profiles/profile.h"
 #include "chromeos/login/auth/challenge_response/known_user_pref_utils.h"
 #include "chromeos/login/auth/challenge_response_key.h"
 #include "components/account_id/account_id.h"
-#include "components/prefs/pref_change_registrar.h"
-#include "components/prefs/pref_service.h"
 #include "components/user_manager/known_user.h"
 #include "content/public/test/browser_test.h"
 #include "extensions/browser/extension_host.h"
-#include "extensions/browser/pref_names.h"
 #include "extensions/browser/process_manager.h"
 #include "extensions/browser/process_manager_observer.h"
 #include "extensions/common/extension_id.h"
+#include "extensions/common/features/simple_feature.h"
 
 namespace chromeos {
 
@@ -34,12 +32,12 @@ namespace {
 
 constexpr char kUserEmail[] = "testuser@example.com";
 
-content::BrowserContext* GetBrowserContext() {
+Profile* GetProfile() {
   return ProfileHelper::GetSigninProfile()->GetOriginalProfile();
 }
 
 extensions::ProcessManager* GetProcessManager() {
-  return extensions::ProcessManager::Get(GetBrowserContext());
+  return extensions::ProcessManager::Get(GetProfile());
 }
 
 }  // namespace
@@ -47,7 +45,7 @@ extensions::ProcessManager* GetProcessManager() {
 class ChallengeResponseAuthKeysLoaderBrowserTest : public OobeBaseTest {
  public:
   ChallengeResponseAuthKeysLoaderBrowserTest() {
-    // Required for TestCertificateProviderExtensionLoginScreenMixin
+    // Allow the forced installation of extensions in the background.
     needs_background_networking_ = true;
   }
   ChallengeResponseAuthKeysLoaderBrowserTest(
@@ -63,11 +61,17 @@ class ChallengeResponseAuthKeysLoaderBrowserTest : public OobeBaseTest {
     challenge_response_auth_keys_loader_->SetMaxWaitTimeForTesting(
         base::TimeDelta::Max());
 
+    cert_provider_extension_ =
+        std::make_unique<TestCertificateProviderExtension>(GetProfile());
+    extension_force_install_mixin_.InitWithDeviceStateMixin(
+        GetProfile(), &device_state_mixin_);
+
     // Register the ChallengeResponseKey for the user.
     user_manager::known_user::SaveKnownUser(account_id_);
   }
 
   void TearDownOnMainThread() override {
+    cert_provider_extension_.reset();
     if (!should_delete_loader_after_shutdown_)
       challenge_response_auth_keys_loader_.reset();
     OobeBaseTest::TearDownOnMainThread();
@@ -105,41 +109,12 @@ class ChallengeResponseAuthKeysLoaderBrowserTest : public OobeBaseTest {
   }
 
   void InstallExtension(bool wait_on_extension_loaded) {
-    cert_provider_extension_mixin_.AddExtensionForForceInstallation();
-    if (wait_on_extension_loaded) {
-      cert_provider_extension_mixin_.WaitUntilExtensionLoaded();
-    } else {
-      // Even though we do not want to wait until the extension is fully ready,
-      // wait until the extension has been registered as a force-installed
-      // login-screen extension in profile preferences.
-      WaitUntilPrefUpdated();
-    }
-  }
-
-  void PrefChangedCallback() {
-    const PrefService* prefs = ProfileHelper::GetSigninProfile()->GetPrefs();
-    const PrefService::Preference* pref =
-        prefs->FindPreference(extensions::pref_names::kLoginScreenExtensions);
-    if (pref->IsManaged() && wait_for_pref_change_run_loop_) {
-      wait_for_pref_change_run_loop_->Quit();
-    }
-  }
-
-  void CheckExtensionInstallPolicyApplied() {
-    // Check that the extension is registered as a force-installed login-screen
-    // extension.
-    const PrefService* const prefs =
-        ProfileHelper::GetSigninProfile()->GetPrefs();
-    const PrefService::Preference* const pref =
-        prefs->FindPreference(extensions::pref_names::kLoginScreenExtensions);
-    EXPECT_TRUE(pref);
-    EXPECT_TRUE(pref->IsManaged());
-    EXPECT_EQ(pref->GetType(), base::Value::Type::DICTIONARY);
-    EXPECT_EQ(pref->GetValue()->DictSize(), static_cast<size_t>(1));
-
-    for (const auto& item : pref->GetValue()->DictItems()) {
-      EXPECT_EQ(item.first, extension_id());
-    }
+    EXPECT_TRUE(extension_force_install_mixin_.ForceInstallFromSourceDir(
+        TestCertificateProviderExtension::GetExtensionSourcePath(),
+        TestCertificateProviderExtension::GetExtensionPemPath(),
+        wait_on_extension_loaded
+            ? ExtensionForceInstallMixin::WaitMode::kBackgroundPageFirstLoad
+            : ExtensionForceInstallMixin::WaitMode::kPrefSet));
   }
 
   std::vector<ChallengeResponseKey> LoadChallengeResponseKeys() {
@@ -152,9 +127,8 @@ class ChallengeResponseAuthKeysLoaderBrowserTest : public OobeBaseTest {
     return challenge_response_keys;
   }
 
-  std::string GetSpki() const {
-    return cert_provider_extension_mixin_.test_certificate_provider_extension()
-        ->GetCertificateSpki();
+  static std::string GetSpki() {
+    return TestCertificateProviderExtension::GetCertificateSpki();
   }
 
   static extensions::ExtensionId extension_id() {
@@ -176,32 +150,16 @@ class ChallengeResponseAuthKeysLoaderBrowserTest : public OobeBaseTest {
   }
 
  private:
-  void WaitUntilPrefUpdated() {
-    PrefChangeRegistrar pref_change_registrar;
-    pref_change_registrar.Init(ProfileHelper::GetSigninProfile()->GetPrefs());
-    pref_change_registrar.Add(
-        extensions::pref_names::kLoginScreenExtensions,
-        base::BindRepeating(
-            &ChallengeResponseAuthKeysLoaderBrowserTest::PrefChangedCallback,
-            weak_ptr_factory_.GetWeakPtr()));
-    const PrefService* prefs = ProfileHelper::GetSigninProfile()->GetPrefs();
-    const PrefService::Preference* pref =
-        prefs->FindPreference(extensions::pref_names::kLoginScreenExtensions);
-    if (!pref->IsManaged()) {
-      base::RunLoop wait_for_pref_change_run_loop;
-      wait_for_pref_change_run_loop_ = &wait_for_pref_change_run_loop;
-      wait_for_pref_change_run_loop.Run();
-    }
-  }
-
   const AccountId account_id_{AccountId::FromUserEmail(kUserEmail)};
 
+  // Bypass "signin_screen" feature only enabled for allowlisted extensions.
+  extensions::SimpleFeature::ScopedThreadUnsafeAllowlistForTest
+      feature_allowlist_{extension_id()};
+
   DeviceStateMixin device_state_mixin_{
       &mixin_host_, DeviceStateMixin::State::OOBE_COMPLETED_CLOUD_ENROLLED};
-  TestCertificateProviderExtensionLoginScreenMixin
-      cert_provider_extension_mixin_{&mixin_host_, &device_state_mixin_,
-                                     /*load_extension_immediately=*/false};
-  base::RunLoop* wait_for_pref_change_run_loop_ = nullptr;
+  std::unique_ptr<TestCertificateProviderExtension> cert_provider_extension_;
+  ExtensionForceInstallMixin extension_force_install_mixin_{&mixin_host_};
 
   std::unique_ptr<ChallengeResponseAuthKeysLoader>
       challenge_response_auth_keys_loader_;
@@ -218,7 +176,6 @@ class ChallengeResponseAuthKeysLoaderBrowserTest : public OobeBaseTest {
 IN_PROC_BROWSER_TEST_F(ChallengeResponseAuthKeysLoaderBrowserTest,
                        NoKeyRegistered) {
   InstallExtension(/*wait_on_extension_loaded=*/true);
-  CheckExtensionInstallPolicyApplied();
 
   // Challenge Response Auth Keys cannot be loaded.
   EXPECT_FALSE(
@@ -245,7 +202,6 @@ IN_PROC_BROWSER_TEST_F(ChallengeResponseAuthKeysLoaderBrowserTest,
                        LoadingKeysAfterExtensionIsInstalled) {
   RegisterChallengeResponseKey(/*with_extension_id=*/true);
   InstallExtension(/*wait_on_extension_loaded=*/true);
-  CheckExtensionInstallPolicyApplied();
 
   // Challenge Response Auth Keys can be loaded.
   EXPECT_TRUE(
@@ -267,7 +223,6 @@ IN_PROC_BROWSER_TEST_F(ChallengeResponseAuthKeysLoaderBrowserTest,
                        LoadingKeysWhileExtensionIsBeingInstalled) {
   RegisterChallengeResponseKey(/*with_extension_id=*/true);
   InstallExtension(/*wait_on_extension_loaded=*/false);
-  CheckExtensionInstallPolicyApplied();
 
   // Challenge Response Auth Keys can be loaded.
   EXPECT_TRUE(
@@ -304,7 +259,6 @@ IN_PROC_BROWSER_TEST_F(ChallengeResponseAuthKeysLoaderBrowserTest,
                        LoadingKeysWithoutExtensionId) {
   RegisterChallengeResponseKey(/*with_extension_id=*/false);
   InstallExtension(/*wait_on_extension_loaded=*/true);
-  CheckExtensionInstallPolicyApplied();
 
   // Challenge Response Auth Keys can be loaded.
   EXPECT_TRUE(
@@ -323,7 +277,6 @@ IN_PROC_BROWSER_TEST_F(ChallengeResponseAuthKeysLoaderBrowserTest,
                        DestroyedBeforeCompletion) {
   RegisterChallengeResponseKey(/*with_extension_id=*/true);
   InstallExtension(/*wait_on_extension_loaded=*/false);
-  CheckExtensionInstallPolicyApplied();
 
   // Challenge Response Auth Keys can be loaded.
   EXPECT_TRUE(
@@ -347,7 +300,6 @@ IN_PROC_BROWSER_TEST_F(ChallengeResponseAuthKeysLoaderBrowserTest,
                        AfterShutdown) {
   RegisterChallengeResponseKey(/*with_extension_id=*/true);
   InstallExtension(/*wait_on_extension_loaded=*/false);
-  CheckExtensionInstallPolicyApplied();
 
   // Challenge Response Auth Keys can be loaded.
   EXPECT_TRUE(
@@ -436,7 +388,6 @@ IN_PROC_BROWSER_TEST_F(ChallengeResponseExtensionLoadObserverTest,
 
   RegisterChallengeResponseKey(/*with_extension_id=*/true);
   InstallExtension(/*wait_on_extension_loaded=*/false);
-  CheckExtensionInstallPolicyApplied();
 
   // Challenge Response Auth Keys can be loaded.
   EXPECT_TRUE(

chrome/browser/policy/extension_force_install_mixin.cc

@@ -10,13 +10,17 @@
 #include <string>
 #include <vector>
 
+#include "base/bind.h"
 #include "base/check.h"
+#include "base/check_op.h"
 #include "base/files/file_path.h"
 #include "base/files/file_util.h"
 #include "base/files/important_file_writer.h"
 #include "base/files/scoped_temp_dir.h"
+#include "base/memory/weak_ptr.h"
 #include "base/notreached.h"
 #include "base/optional.h"
+#include "base/run_loop.h"
 #include "base/strings/string_util.h"
 #include "base/strings/stringprintf.h"
 #include "base/threading/thread_restrictions.h"
@@ -25,15 +29,19 @@
 #include "chrome/browser/profiles/profile.h"
 #include "components/crx_file/crx_verifier.h"
 #include "components/crx_file/id_util.h"
+#include "components/prefs/pref_change_registrar.h"
+#include "components/prefs/pref_service.h"
 #include "extensions/browser/extension_creator.h"
 #include "extensions/browser/extension_registry.h"
 #include "extensions/browser/extension_system.h"
+#include "extensions/browser/pref_names.h"
 #include "extensions/browser/runtime_data.h"
 #include "extensions/browser/test_extension_registry_observer.h"
 #include "extensions/common/extension.h"
 #include "extensions/common/extension_id.h"
 #include "extensions/common/file_util.h"
 #include "extensions/common/manifest_constants.h"
+#include "extensions/test/test_background_page_first_load_observer.h"
 #include "extensions/test/test_background_page_ready_observer.h"
 #include "net/test/embedded_test_server/embedded_test_server.h"
 #include "testing/gtest/include/gtest/gtest.h"
@@ -43,6 +51,7 @@
 #if defined(OS_CHROMEOS)
 #include "chrome/browser/chromeos/login/test/device_state_mixin.h"
 #include "chrome/browser/chromeos/policy/device_policy_cros_browser_test.h"
+#include "chrome/browser/chromeos/profiles/profile_helper.h"
 #include "components/policy/proto/chrome_device_policy.pb.h"
 #endif
 
@@ -64,6 +73,78 @@ constexpr char kUpdateManifestTemplate[] =
          </app>
        </gupdate>)";
 
+// Implements waiting until the given extension appears in the
+// force-installation pref.
+class ForceInstallPrefObserver final {
+ public:
+  ForceInstallPrefObserver(Profile* profile,
+                           const extensions::ExtensionId& extension_id);
+  ForceInstallPrefObserver(const ForceInstallPrefObserver&) = delete;
+  ForceInstallPrefObserver& operator=(const ForceInstallPrefObserver&) = delete;
+  ~ForceInstallPrefObserver();
+
+  void Wait();
+
+ private:
+  void OnPrefChanged();
+  bool IsForceInstallPrefSet() const;
+
+  PrefService* const pref_service_;
+  const std::string pref_name_;
+  const extensions::ExtensionId extension_id_;
+  PrefChangeRegistrar pref_change_registrar_;
+  base::RunLoop run_loop_;
+  base::WeakPtrFactory<ForceInstallPrefObserver> weak_ptr_factory_{this};
+};
+
+std::string GetForceInstallPrefName(Profile* profile) {
+#if defined(OS_CHROMEOS)
+  if (chromeos::ProfileHelper::IsSigninProfile(profile))
+    return extensions::pref_names::kLoginScreenExtensions;
+#endif  // OS_CHROMEOS
+  return extensions::pref_names::kInstallForceList;
+}
+
+ForceInstallPrefObserver::ForceInstallPrefObserver(
+    Profile* profile,
+    const extensions::ExtensionId& extension_id)
+    : pref_service_(profile->GetPrefs()),
+      pref_name_(GetForceInstallPrefName(profile)),
+      extension_id_(extension_id) {
+  pref_change_registrar_.Init(pref_service_);
+  pref_change_registrar_.Add(
+      pref_name_, base::BindRepeating(&ForceInstallPrefObserver::OnPrefChanged,
+                                      weak_ptr_factory_.GetWeakPtr()));
+}
+
+ForceInstallPrefObserver::~ForceInstallPrefObserver() = default;
+
+void ForceInstallPrefObserver::Wait() {
+  if (IsForceInstallPrefSet())
+    return;
+  run_loop_.Run();
+}
+
+void ForceInstallPrefObserver::OnPrefChanged() {
+  if (IsForceInstallPrefSet())
+    run_loop_.Quit();
+}
+
+bool ForceInstallPrefObserver::IsForceInstallPrefSet() const {
+  const PrefService::Preference* const pref =
+      pref_service_->FindPreference(pref_name_);
+  if (!pref || !pref->IsManaged()) {
+    // Note that we intentionally ignore the pref if it's set but isn't managed,
+    // mimicking the real behavior of the Extensions system that only respects
+    // trusted (policy-set) values. Normally there's no "untrusted" value in
+    // these prefs, but in theory this is an attack that the Extensions system
+    // protects against, and there might be tests that simulate this scenario.
+    return false;
+  }
+  DCHECK_EQ(pref->GetType(), base::Value::Type::DICTIONARY);
+  return pref->GetValue()->FindKey(extension_id_) != nullptr;
+}
+
 // Implements waiting for the mixin's specified event.
 class ForceInstallWaiter final {
  public:
@@ -86,9 +167,12 @@ class ForceInstallWaiter final {
   const ExtensionForceInstallMixin::WaitMode wait_mode_;
   const extensions::ExtensionId extension_id_;
   Profile* const profile_;
+  std::unique_ptr<ForceInstallPrefObserver> force_install_pref_observer_;
   std::unique_ptr<extensions::TestExtensionRegistryObserver> registry_observer_;
   std::unique_ptr<extensions::ExtensionBackgroundPageReadyObserver>
       background_page_ready_observer_;
+  std::unique_ptr<extensions::TestBackgroundPageFirstLoadObserver>
+      background_page_first_load_observer_;
 };
 
 ForceInstallWaiter::ForceInstallWaiter(
@@ -101,15 +185,24 @@ ForceInstallWaiter::ForceInstallWaiter(
   switch (wait_mode_) {
     case ExtensionForceInstallMixin::WaitMode::kNone:
       break;
+    case ExtensionForceInstallMixin::WaitMode::kPrefSet:
+      force_install_pref_observer_ =
+          std::make_unique<ForceInstallPrefObserver>(profile_, extension_id_);
+      break;
     case ExtensionForceInstallMixin::WaitMode::kLoad:
       registry_observer_ =
           std::make_unique<extensions::TestExtensionRegistryObserver>(
-              extensions::ExtensionRegistry::Get(profile_), extension_id);
+              extensions::ExtensionRegistry::Get(profile_), extension_id_);
       break;
     case ExtensionForceInstallMixin::WaitMode::kBackgroundPageReady:
       background_page_ready_observer_ =
           std::make_unique<extensions::ExtensionBackgroundPageReadyObserver>(
-              profile_, extension_id);
+              profile_, extension_id_);
+      break;
+    case ExtensionForceInstallMixin::WaitMode::kBackgroundPageFirstLoad:
+      background_page_first_load_observer_ =
+          std::make_unique<extensions::TestBackgroundPageFirstLoadObserver>(
+              profile_, extension_id_);
       break;
   }
 }
@@ -128,14 +221,24 @@ void ForceInstallWaiter::WaitImpl(bool* success) {
       // No waiting needed.
       *success = true;
       break;
+    case ExtensionForceInstallMixin::WaitMode::kPrefSet:
+      // Wait and assert that the waiting run loop didn't time out.
+      ASSERT_NO_FATAL_FAILURE(force_install_pref_observer_->Wait());
+      *success = true;
+      break;
     case ExtensionForceInstallMixin::WaitMode::kLoad:
       *success = registry_observer_->WaitForExtensionLoaded() != nullptr;
       break;
-    case ExtensionForceInstallMixin::WaitMode::kBackgroundPageReady: {
+    case ExtensionForceInstallMixin::WaitMode::kBackgroundPageReady:
+      // Wait and assert that the waiting run loop didn't time out.
       ASSERT_NO_FATAL_FAILURE(background_page_ready_observer_->Wait());
       *success = true;
       break;
-    }
+    case ExtensionForceInstallMixin::WaitMode::kBackgroundPageFirstLoad:
+      // Wait and assert that the waiting run loop didn't time out.
+      ASSERT_NO_FATAL_FAILURE(background_page_first_load_observer_->Wait());
+      *success = true;
+      break;
   }
 }
 

chrome/browser/policy/extension_force_install_mixin.h

@@ -73,10 +73,14 @@ class ExtensionForceInstallMixin final : public InProcessBrowserTestMixin {
   enum class WaitMode {
     // Don't wait, and return immediately.
     kNone,
+    // Wait until the force-installation pref is updated.
+    kPrefSet,
     // Wait until the extension is loaded.
     kLoad,
     // Wait until the extension's background page is ready.
     kBackgroundPageReady,
+    // Wait until the extension's background page is loaded for the first time.
+    kBackgroundPageFirstLoad,
   };
 
   explicit ExtensionForceInstallMixin(InProcessBrowserTestMixinHost* host);