Extending known misconfiguration failures occurred in case of

force installed extensions

When force installed extensions failed to install with failure reason
CRX_INSTALL_ERROR_DECLINED and CrxInstallErrorDetail is
DISALLOWED_BY_POLICY and installation fails because extension type
is not allowed as per the policy ExtensionAllowedTypes, we consider it
as one of the misconfigurations.


Bug: 981891
Change-Id: If4c23d05ab8ba9f29189179e244eaa715733836d
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2007646Reviewed-by: Oleg Davydov <burunduk@chromium.org>
Reviewed-by: Karan Bhatia <karandeepb@chromium.org>
Commit-Queue: Swapnil Gupta <swapnilgupta@google.com>
Cr-Commit-Position: refs/heads/master@{#748236}

chrome/browser/extensions/crx_installer.cc

@@ -1005,6 +1005,11 @@ void CrxInstaller::NotifyCrxInstallComplete(
   if (!success && (!expected_id_.empty() || extension())) {
     switch (error->type()) {
       case CrxInstallErrorType::DECLINED:
+        if (error->detail() == CrxInstallErrorDetail::DISALLOWED_BY_POLICY) {
+          installation_reporter
+              ->ReportExtensionTypeForPolicyDisallowedExtension(
+                  extension_id, extension()->GetType());
+        }
         installation_reporter->ReportCrxInstallError(
             extension_id,
             InstallationReporter::FailureReason::CRX_INSTALL_ERROR_DECLINED,

chrome/browser/extensions/forced_extensions/installation_reporter.cc

@@ -110,13 +110,18 @@ void InstallationReporter::ReportFailure(const ExtensionId& id,
   NotifyObserversOfFailure(id, reason, data);
 }
 
+void InstallationReporter::ReportExtensionTypeForPolicyDisallowedExtension(
+    const ExtensionId& id,
+    Manifest::Type extension_type) {
+  InstallationData& data = installation_data_map_[id];
+  data.extension_type = extension_type;
+}
+
 void InstallationReporter::ReportCrxInstallError(
     const ExtensionId& id,
     FailureReason reason,
     CrxInstallErrorDetail crx_install_error) {
   DCHECK(reason == FailureReason::CRX_INSTALL_ERROR_DECLINED ||
-         reason ==
-             FailureReason::CRX_INSTALL_ERROR_SANDBOXED_UNPACKER_FAILURE ||
          reason == FailureReason::CRX_INSTALL_ERROR_OTHER);
   InstallationData& data = installation_data_map_[id];
   data.failure_reason = reason;

chrome/browser/extensions/forced_extensions/installation_reporter.h

@@ -199,6 +199,9 @@ class InstallationReporter : public KeyedService {
     // CRX_INSTALL_ERROR_SANDBOXED_UNPACKER_FAILURE.
     base::Optional<extensions::SandboxedUnpackerFailureReason>
         unpacker_failure_reason;
+    // Type of extension, assigned when CRX installation error detail is
+    // DISALLOWED_BY_POLICY.
+    base::Optional<Manifest::Type> extension_type;
   };
 
   class Observer : public base::CheckedObserver {
@@ -235,6 +238,11 @@ class InstallationReporter : public KeyedService {
   void ReportDownloadingCacheStatus(
       const ExtensionId& id,
       ExtensionDownloaderDelegate::CacheStatus cache_status);
+  // Assigns the extension type. See InstallationData::extension_type for more
+  // details.
+  void ReportExtensionTypeForPolicyDisallowedExtension(
+      const ExtensionId& id,
+      Manifest::Type extension_type);
   void ReportCrxInstallError(const ExtensionId& id,
                              FailureReason reason,
                              CrxInstallErrorDetail crx_install_error);

chrome/browser/extensions/forced_extensions/installation_tracker.cc

@@ -10,6 +10,8 @@
 #include "base/metrics/histogram_functions.h"
 #include "base/metrics/histogram_macros.h"
 #include "base/values.h"
+#include "chrome/browser/extensions/extension_management.h"
+#include "chrome/browser/extensions/extension_management_constants.h"
 #include "chrome/browser/extensions/external_provider_impl.h"
 #include "chrome/browser/extensions/forced_extensions/installation_reporter.h"
 #include "chrome/browser/profiles/profile.h"
@@ -198,6 +200,24 @@ void InstallationTracker::OnExtensionInstallationFailed(
     ReportResults();
 }
 
+bool InstallationTracker::IsMisconfiguration(
+    const InstallationReporter::InstallationData& installation_data,
+    const ExtensionId& id) {
+  ExtensionManagement* management =
+      ExtensionManagementFactory::GetForBrowserContext(profile_);
+  CrxInstallErrorDetail detail = installation_data.install_error_detail.value();
+  if (detail == CrxInstallErrorDetail::KIOSK_MODE_ONLY)
+    return true;
+
+  if (detail == CrxInstallErrorDetail::DISALLOWED_BY_POLICY &&
+      !management->IsAllowedManifestType(
+          installation_data.extension_type.value(), id)) {
+    return true;
+  }
+
+  return false;
+}
+
 #if defined(OS_CHROMEOS)
 // Returns the type of session in case extension fails to install.
 InstallationTracker::SessionType InstallationTracker::GetSessionType() {
@@ -329,8 +349,7 @@ void InstallationTracker::ReportMetrics() {
         if (installation.install_error_detail) {
           CrxInstallErrorDetail detail =
               installation.install_error_detail.value();
-          // KIOSK_MODE_ONLY is a type of misconfiguration failure.
-          if (detail == CrxInstallErrorDetail::KIOSK_MODE_ONLY)
+          if (IsMisconfiguration(installation, extension_id))
             misconfigured_extensions++;
           UMA_HISTOGRAM_ENUMERATION(
               "Extensions.ForceInstalledFailureCrxInstallError", detail);
@@ -349,6 +368,7 @@ void InstallationTracker::ReportMetrics() {
       "ForceInstalledSessionsWithNonMisconfigurationFailureOccured",
       non_misconfigured_failure_occurred);
 }
+
 void InstallationTracker::ReportResults() {
   DCHECK(!reported_);
   // Report only if there was non-empty list of force-installed extensions.

chrome/browser/extensions/forced_extensions/installation_tracker.h

@@ -114,6 +114,13 @@ class InstallationTracker : public ExtensionRegistryObserver,
   // Loads list of force-installed extensions if available.
   void OnForcedExtensionsPrefChanged();
 
+  // Returns true only in case of some well-known misconfigurations which are
+  // easy to detect. Can return false for misconfigurations which are hard to
+  // distinguish with other errors.
+  bool IsMisconfiguration(
+      const InstallationReporter::InstallationData& installation_data,
+      const ExtensionId& id);
+
 #if defined(OS_CHROMEOS)
   // Returns Session Type in case extension fails to install.
   SessionType GetSessionType();

chrome/browser/extensions/forced_extensions/installation_tracker_unittest.cc

@@ -19,6 +19,7 @@
 #include "extensions/browser/pref_names.h"
 #include "extensions/common/extension.h"
 #include "extensions/common/extension_builder.h"
+#include "extensions/common/manifest.h"
 #include "extensions/common/value_builder.h"
 #include "net/base/net_errors.h"
 #include "testing/gtest/include/gtest/gtest.h"
@@ -30,8 +31,10 @@
 #endif  // defined(OS_CHROMEOS)
 
 namespace {
-constexpr char kExtensionId1[] = "id1";
-constexpr char kExtensionId2[] = "id2";
+
+// The extension ids used here should be valid extension ids.
+constexpr char kExtensionId1[] = "abcdefghijklmnopabcdefghijklmnop";
+constexpr char kExtensionId2[] = "bcdefghijklmnopabcdefghijklmnopa";
 constexpr char kExtensionName1[] = "name1";
 constexpr char kExtensionName2[] = "name2";
 constexpr char kExtensionUpdateUrl[] =
@@ -396,11 +399,11 @@ TEST_F(ForcedExtensionsInstallationTrackerTest, ExtensionManifestFetchFailed) {
 }
 
 // Session in which either all the extensions installed successfully, or all
-// failures are admin-side misconfigurations. Misconfiguration failure includes
-// error KIOSK_MODE_ONLY, when force installed extension fails to install with
-// failure reason CRX_INSTALL_ERROR.
+// failures are admin-side misconfigurations. This test verifies that failure
+// CRX_INSTALL_ERROR with detailed error KIOSK_MODE_ONLY is considered as
+// misconfiguration.
 TEST_F(ForcedExtensionsInstallationTrackerTest,
-       NonMisconfigurationFailureNotPresent) {
+       NonMisconfigurationFailureNotPresentKioskModeOnlyError) {
   SetupForceList();
   auto extension =
       ExtensionBuilder(kExtensionName1).SetID(kExtensionId1).Build();
@@ -416,6 +419,68 @@ TEST_F(ForcedExtensionsInstallationTrackerTest,
                                       1);
 }
 
+// Session in which either all the extensions installed successfully, or all
+// failures are admin-side misconfigurations. This test verifies that failure
+// CRX_INSTALL_ERROR with detailed error DISALLOWED_BY_POLICY and when extension
+// type which is not allowed to install according to policy
+// kExtensionAllowedTypes is considered as misconfiguration.
+TEST_F(ForcedExtensionsInstallationTrackerTest,
+       NonMisconfigurationFailureNotPresentDisallowedByPolicyTypeError) {
+  SetupForceList();
+  // Set TYPE_EXTENSION and TYPE_THEME as the allowed extension types.
+  std::unique_ptr<base::Value> list =
+      ListBuilder().Append("extension").Append("theme").Build();
+  prefs_->SetManagedPref(pref_names::kAllowedTypes, std::move(list));
+
+  auto extension =
+      ExtensionBuilder(kExtensionName1).SetID(kExtensionId1).Build();
+  tracker_->OnExtensionLoaded(&profile_, extension.get());
+  // Hosted app is not a valid extension type, so this should report an error.
+  installation_reporter_->ReportExtensionTypeForPolicyDisallowedExtension(
+      kExtensionId2, Manifest::Type::TYPE_HOSTED_APP);
+  installation_reporter_->ReportCrxInstallError(
+      kExtensionId2,
+      InstallationReporter::FailureReason::CRX_INSTALL_ERROR_DECLINED,
+      CrxInstallErrorDetail::DISALLOWED_BY_POLICY);
+  // InstallationTracker shuts down timer because all extension are either
+  // loaded or failed.
+  EXPECT_FALSE(fake_timer_->IsRunning());
+  histogram_tester_.ExpectBucketCount(
+      kPossibleNonMisconfigurationFailures,
+      0 /*Misconfiguration failure not present*/, 1 /*Count of the sample*/);
+}
+
+// Session in which at least one non misconfiguration failure occurred. One of
+// the extension fails to install with DISALLOWED_BY_POLICY error but has
+// extension type which is allowed by policy ExtensionAllowedTypes. This is not
+// a misconfiguration failure.
+TEST_F(ForcedExtensionsInstallationTrackerTest,
+       NonMisconfigurationFailurePresentDisallowedByPolicyError) {
+  SetupForceList();
+
+  // Set TYPE_EXTENSION and TYPE_THEME as the allowed extension types.
+  std::unique_ptr<base::Value> list =
+      ListBuilder().Append("extension").Append("theme").Build();
+  prefs_->SetManagedPref(pref_names::kAllowedTypes, std::move(list));
+
+  auto extension =
+      ExtensionBuilder(kExtensionName1).SetID(kExtensionId1).Build();
+  tracker_->OnExtensionLoaded(&profile_, extension.get());
+  installation_reporter_->ReportExtensionTypeForPolicyDisallowedExtension(
+      kExtensionId2, Manifest::Type::TYPE_EXTENSION);
+  installation_reporter_->ReportCrxInstallError(
+      kExtensionId2,
+      InstallationReporter::FailureReason::CRX_INSTALL_ERROR_DECLINED,
+      CrxInstallErrorDetail::DISALLOWED_BY_POLICY);
+
+  // InstallationTracker shuts down timer because all extension are either
+  // loaded or failed.
+  EXPECT_FALSE(fake_timer_->IsRunning());
+  histogram_tester_.ExpectBucketCount(kPossibleNonMisconfigurationFailures,
+                                      1 /*Misconfiguration failure present*/,
+                                      1 /*Count of the sample*/);
+}
+
 // Session in which at least one non misconfiguration failure occurred.
 // Misconfiguration failure includes error KIOSK_MODE_ONLY, when force installed
 // extension fails to install with failure reason CRX_INSTALL_ERROR.