[Extensions] Account for policy-blocked extensions in ExtensionEnableFlow

ExtensionEnableFlow tries to re-enable a disabled extension, but did not
account for extensions that cannot be re-enabled by enterprise policy.
This would result in the ExtensionEnableFlow trying to re-enable the
extension (which would fail), and then notifying the delegate that the
re-enable was successful (which it wasn't).

This could lead to an infinite loop in the app list, where an
ExtensionAppItem would try to launch, see the extension needed to be
re-enabled, try to re-enable the extension through the
ExtensionEnableFlow, see that it was successful, and try to launch.

Fix this by checking if the extension must remain disabled in the
ExtensionEnableFlow and responding appropriately. Add a regression test
for the same.

Bug: 783831

Change-Id: I53cddecc895a9602f0884fa022b68ea7a6c2d667
Reviewed-on: https://chromium-review.googlesource.com/764851
Commit-Queue: Devlin <rdevlin.cronin@chromium.org>
Reviewed-by: Karan Bhatia <karandeepb@chromium.org>
Cr-Commit-Position: refs/heads/master@{#516309}

chrome/browser/ui/extensions/extension_enable_flow.cc

@@ -83,16 +83,20 @@ void ExtensionEnableFlow::Run() {
 }
 
 void ExtensionEnableFlow::CheckPermissionAndMaybePromptUser() {
-  ExtensionService* service =
-      extensions::ExtensionSystem::Get(profile_)->extension_service();
+  extensions::ExtensionSystem* system =
+      extensions::ExtensionSystem::Get(profile_);
+  ExtensionService* service = system->extension_service();
   const Extension* extension = service->GetExtensionById(extension_id_, true);
-  if (!extension) {
-    delegate_->ExtensionEnableFlowAborted(false);  // |delegate_| may delete us.
-    return;
-  }
 
-  // Supervised users can't re-enable custodian-installed extensions.
-  if (extensions::util::IsExtensionSupervised(extension, profile_)) {
+  bool abort =
+      !extension ||
+      // The extension might be force-disabled by policy.
+      system->management_policy()->MustRemainDisabled(extension, nullptr,
+                                                      nullptr) ||
+      // Supervised users can't re-enable custodian-installed extensions.
+      extensions::util::IsExtensionSupervised(extension, profile_);
+
+  if (abort) {
     delegate_->ExtensionEnableFlowAborted(false);  // |delegate_| may delete us.
     return;
   }
@@ -109,6 +113,7 @@ void ExtensionEnableFlow::CheckPermissionAndMaybePromptUser() {
     // This is a no-op if the extension was previously terminated.
     service->EnableExtension(extension_id_);
 
+    DCHECK(service->IsExtensionEnabled(extension_id_));
     delegate_->ExtensionEnableFlowFinished();  // |delegate_| may delete us.
     return;
   }
@@ -187,6 +192,8 @@ void ExtensionEnableFlow::InstallPromptDone(
     }
 
     service->GrantPermissionsAndEnableExtension(extension);
+
+    DCHECK(service->IsExtensionEnabled(extension_id_));
     delegate_->ExtensionEnableFlowFinished();  // |delegate_| may delete us.
   } else {
     delegate_->ExtensionEnableFlowAborted(

chrome/browser/ui/extensions/extension_enable_flow_browsertest.cc

+// Copyright 2017 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 "chrome/browser/ui/extensions/extension_enable_flow.h"
+
+#include "base/optional.h"
+#include "base/run_loop.h"
+#include "chrome/browser/extensions/extension_browsertest.h"
+#include "chrome/browser/extensions/extension_service.h"
+#include "chrome/browser/profiles/profile.h"
+#include "chrome/browser/ui/extensions/extension_enable_flow_delegate.h"
+#include "chrome/browser/ui/tabs/tab_strip_model.h"
+#include "content/public/browser/web_contents.h"
+#include "extensions/browser/extension_dialog_auto_confirm.h"
+#include "extensions/browser/extension_registry.h"
+#include "extensions/browser/extension_system.h"
+#include "extensions/browser/management_policy.h"
+#include "extensions/common/extension.h"
+#include "extensions/common/extension_builder.h"
+#include "extensions/common/extension_id.h"
+
+namespace {
+
+class TestManagementProvider : public extensions::ManagementPolicy::Provider {
+ public:
+  explicit TestManagementProvider(const extensions::ExtensionId& extension_id)
+      : extension_id_(extension_id) {}
+  ~TestManagementProvider() override {}
+
+  // MananagementPolicy::Provider:
+  std::string GetDebugPolicyProviderName() const override { return "test"; }
+  bool MustRemainDisabled(const extensions::Extension* extension,
+                          extensions::disable_reason::DisableReason* reason,
+                          base::string16* error) const override {
+    return extension->id() == extension_id_;
+  }
+
+ private:
+  const extensions::ExtensionId extension_id_;
+
+  DISALLOW_COPY_AND_ASSIGN(TestManagementProvider);
+};
+
+class TestDelegate : public ExtensionEnableFlowDelegate {
+ public:
+  TestDelegate() {}
+  ~TestDelegate() override {}
+
+  enum Result {
+    ABORTED,
+    FINISHED,
+  };
+
+  void ExtensionEnableFlowFinished() override {
+    result_ = FINISHED;
+    run_loop_.Quit();
+  }
+  void ExtensionEnableFlowAborted(bool user_initiated) override {
+    result_ = ABORTED;
+    run_loop_.Quit();
+  }
+
+  void Wait() { run_loop_.Run(); }
+
+  const base::Optional<Result>& result() const { return result_; }
+
+ private:
+  base::Optional<Result> result_;
+  base::RunLoop run_loop_;
+
+  DISALLOW_COPY_AND_ASSIGN(TestDelegate);
+};
+
+}  // namespace
+
+using ExtensionEnableFlowBrowserTest = ExtensionBrowserTest;
+
+// Test that trying to enable an extension that's blocked by policy fails
+// gracefully. See https://crbug.com/783831.
+IN_PROC_BROWSER_TEST_F(ExtensionEnableFlowBrowserTest,
+                       TryEnablingPolicyForbiddenExtension) {
+  scoped_refptr<const extensions::Extension> extension =
+      extensions::ExtensionBuilder("extension").Build();
+  extension_service()->AddExtension(extension.get());
+
+  extensions::ExtensionRegistry* registry =
+      extensions::ExtensionRegistry::Get(profile());
+  {
+    extensions::ScopedTestDialogAutoConfirm auto_confirm(
+        extensions::ScopedTestDialogAutoConfirm::ACCEPT);
+
+    extensions::ManagementPolicy* management_policy =
+        extensions::ExtensionSystem::Get(profile())->management_policy();
+    ASSERT_TRUE(management_policy);
+    TestManagementProvider test_provider(extension->id());
+    management_policy->RegisterProvider(&test_provider);
+    extension_service()->DisableExtension(
+        extension->id(), extensions::disable_reason::DISABLE_BLOCKED_BY_POLICY);
+    EXPECT_TRUE(registry->disabled_extensions().Contains(extension->id()));
+
+    TestDelegate delegate;
+
+    ExtensionEnableFlow enable_flow(profile(), extension->id(), &delegate);
+
+    content::WebContents* web_contents =
+        browser()->tab_strip_model()->GetActiveWebContents();
+    enable_flow.StartForWebContents(web_contents);
+    delegate.Wait();
+
+    ASSERT_TRUE(delegate.result());
+    EXPECT_EQ(TestDelegate::ABORTED, *delegate.result());
+
+    EXPECT_TRUE(registry->disabled_extensions().Contains(extension->id()));
+
+    management_policy->UnregisterProvider(&test_provider);
+  }
+}

chrome/test/BUILD.gn

@@ -726,6 +726,7 @@ test("browser_tests") {
       "../browser/ui/content_settings/content_setting_bubble_model_browsertest.cc",
       "../browser/ui/content_settings/content_setting_image_model_browsertest.cc",
       "../browser/ui/exclusive_access/fullscreen_controller_browsertest.cc",
+      "../browser/ui/extensions/extension_enable_flow_browsertest.cc",
       "../browser/ui/extensions/extension_installed_bubble_browsertest.cc",
       "../browser/ui/extensions/extension_message_bubble_browsertest.cc",
       "../browser/ui/extensions/extension_message_bubble_browsertest.h",