Unify the supervised user extension approval flow result handlers

All call sites now go through the SupervisedUserExtensionsDelegate and
handle all flow states in one callback method.

This change is to prepare for https://chromium-review.googlesource.com/c/chromium/src/+/4163439
and the introduction of a new dialog flow for extension approvals.
The existing enums and methods have been renamed to be implementation-
agnostic so that they can be used for any UI that invokes the approval
flow.

One side effect of this change is only allowing one dialog to be open
at a time. This was existing behavior for the ManagementAPI and
ExtensionEnableFlow, but has been extended to the WebstorePrivateAPI
as part of this change. Other than this, no behavior change is expected.

Tested: Manually checked the supervised user flows, testing the
approve/deny/blocked scenarios.

Bug: b:262450453
Change-Id: Id8852249968118ff46ccdafec0fbfb578468997e
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/4226049
Reviewed-by: Tim <tjudkins@chromium.org>
Reviewed-by: Aga Wronska <agawronska@chromium.org>
Commit-Queue: Courtney Wong <courtneywong@chromium.org>
Reviewed-by: Devlin Cronin <rdevlin.cronin@chromium.org>
Cr-Commit-Position: refs/heads/main@{#1107820}

chrome/browser/extensions/api/management/management_api_unittest.cc

@@ -52,6 +52,7 @@
 #include "chrome/browser/supervised_user/supervised_user_service_factory.h"
 #include "chrome/browser/supervised_user/supervised_user_test_util.h"
 #include "content/public/browser/gpu_data_manager.h"
+#include "extensions/browser/supervised_user_extensions_delegate.h"
 #endif  // BUILDFLAG(IS_CHROMEOS_ASH)
 
 using extensions::mojom::ManifestLocation;
@@ -953,23 +954,25 @@ class TestSupervisedUserExtensionsDelegate
       const extensions::Extension& extension,
       content::BrowserContext* context,
       content::WebContents* contents,
-      ParentPermissionDialogDoneCallback parent_permission_callback,
-      base::OnceClosure error_callback) override {
+      ExtensionApprovalDoneCallback extension_approval_callback) override {
     // Preconditions.
     DCHECK(IsChild(context));
     DCHECK(!IsExtensionAllowedByParent(extension, context));
 
     if (CanInstallExtensions(context)) {
       ShowParentPermissionDialogForExtension(
-          extension, context, contents, std::move(parent_permission_callback));
+          extension, context, contents, std::move(extension_approval_callback));
     } else {
       ShowExtensionEnableBlockedByParentDialogForExtension(
-          extension, contents, std::move(error_callback));
+          extension, contents,
+          base::BindOnce(std::move(extension_approval_callback),
+                         SupervisedUserExtensionsDelegate::
+                             ExtensionApprovalResult::kBlocked));
     }
   }
 
   void set_next_parent_permission_dialog_result(
-      ParentPermissionDialogResult result) {
+      ExtensionApprovalResult result) {
     dialog_result_ = result;
   }
 
@@ -991,7 +994,7 @@ class TestSupervisedUserExtensionsDelegate
       const extensions::Extension& extension,
       content::BrowserContext* context,
       content::WebContents* contents,
-      ParentPermissionDialogDoneCallback done_callback) {
+      ExtensionApprovalDoneCallback done_callback) {
     ++show_dialog_count_;
     std::move(done_callback).Run(dialog_result_);
   }
@@ -1009,8 +1012,7 @@ class TestSupervisedUserExtensionsDelegate
     std::move(done_callback).Run();
   }
 
-  ParentPermissionDialogResult dialog_result_ =
-      ParentPermissionDialogResult::kParentPermissionFailed;
+  ExtensionApprovalResult dialog_result_ = ExtensionApprovalResult::kFailed;
   int show_dialog_count_ = 0;
   int show_block_dialog_count_ = 0;
 };
@@ -1349,8 +1351,7 @@ TEST_F(ManagementApiSupervisedUserTest,
   // Now try again with parent approval, and this should succeed.
   {
     supervised_user_delegate_->set_next_parent_permission_dialog_result(
-        SupervisedUserExtensionsDelegate::ParentPermissionDialogResult::
-            kParentPermissionReceived);
+        SupervisedUserExtensionsDelegate::ExtensionApprovalResult::kApproved);
     std::string error;
     bool success = RunSetEnabledFunction(web_contents_.get(), extension_id,
                                          /*use_user_gesture=*/true,
@@ -1401,8 +1402,7 @@ TEST_F(ManagementApiSupervisedUserTest, SetEnabled_UnsupportedRequirement) {
   // Parent approval should fail because of the unsupported requirements.
   {
     supervised_user_delegate_->set_next_parent_permission_dialog_result(
-        SupervisedUserExtensionsDelegate::ParentPermissionDialogResult::
-            kParentPermissionReceived);
+        SupervisedUserExtensionsDelegate::ExtensionApprovalResult::kApproved);
     std::string error;
     bool success = RunSetEnabledFunction(web_contents_.get(), extension->id(),
                                          /*user_user_gesture=*/true,
@@ -1436,8 +1436,7 @@ TEST_F(ManagementApiSupervisedUserTest, SetEnabledDisabled_UmaMetrics) {
 
   // The parent will approve.
   supervised_user_delegate_->set_next_parent_permission_dialog_result(
-      SupervisedUserExtensionsDelegate::ParentPermissionDialogResult::
-          kParentPermissionReceived);
+      SupervisedUserExtensionsDelegate::ExtensionApprovalResult::kApproved);
 
   RunSetEnabledFunction(web_contents_.get(), extension->id(),
                         /*use_user_gesture=*/true, /*accept_dialog=*/true,
@@ -1523,8 +1522,7 @@ TEST_F(ManagementApiSupervisedUserTestWithSetup, SetEnabled_ParentApproves) {
 
   // The parent will approve.
   supervised_user_delegate_->set_next_parent_permission_dialog_result(
-      SupervisedUserExtensionsDelegate::ParentPermissionDialogResult::
-          kParentPermissionReceived);
+      SupervisedUserExtensionsDelegate::ExtensionApprovalResult::kApproved);
 
   // Simulate a call to chrome.management.setEnabled(). It should succeed.
   std::string error;
@@ -1548,8 +1546,7 @@ TEST_F(ManagementApiSupervisedUserTestWithSetup, SetEnabled_ParentDenies) {
 
   // The parent will deny the next dialog.
   supervised_user_delegate_->set_next_parent_permission_dialog_result(
-      SupervisedUserExtensionsDelegate::ParentPermissionDialogResult::
-          kParentPermissionCanceled);
+      SupervisedUserExtensionsDelegate::ExtensionApprovalResult::kCanceled);
 
   // Simulate a call to chrome.management.setEnabled(). It should not succeed.
   std::string error;
@@ -1574,8 +1571,7 @@ TEST_F(ManagementApiSupervisedUserTestWithSetup, SetEnabled_DialogFails) {
   // The next dialog will close due to a failure (e.g. network failure while
   // looking up parent information).
   supervised_user_delegate_->set_next_parent_permission_dialog_result(
-      SupervisedUserExtensionsDelegate::ParentPermissionDialogResult::
-          kParentPermissionFailed);
+      SupervisedUserExtensionsDelegate::ExtensionApprovalResult::kFailed);
 
   // Simulate a call to chrome.management.setEnabled(). It should not succeed.
   std::string error;
@@ -1622,8 +1618,7 @@ TEST_F(ManagementApiSupervisedUserTestWithSetup,
 
   // The parent will approve.
   supervised_user_delegate_->set_next_parent_permission_dialog_result(
-      SupervisedUserExtensionsDelegate::ParentPermissionDialogResult::
-          kParentPermissionReceived);
+      SupervisedUserExtensionsDelegate::ExtensionApprovalResult::kApproved);
 
   // Simulate a call to chrome.management.setEnabled(). It should succeed
   // despite a lack of web contents.
@@ -1655,8 +1650,7 @@ TEST_F(ManagementApiSupervisedUserTestWithSetup,
 
   // The parent will cancel.
   supervised_user_delegate_->set_next_parent_permission_dialog_result(
-      SupervisedUserExtensionsDelegate::ParentPermissionDialogResult::
-          kParentPermissionCanceled);
+      SupervisedUserExtensionsDelegate::ExtensionApprovalResult::kCanceled);
 
   // Simulate a call to chrome.management.setEnabled() with no web contents.
   std::string error;
@@ -1689,8 +1683,7 @@ TEST_F(ManagementApiSupervisedUserTestWithSetup,
 
   // The request will fail.
   supervised_user_delegate_->set_next_parent_permission_dialog_result(
-      SupervisedUserExtensionsDelegate::ParentPermissionDialogResult::
-          kParentPermissionFailed);
+      SupervisedUserExtensionsDelegate::ExtensionApprovalResult::kFailed);
 
   // Simulate a call to chrome.management.setEnabled() with no web contents.
   std::string error;

chrome/browser/extensions/api/webstore_private/webstore_private_api.cc

@@ -72,6 +72,7 @@
 // flag to #if BUILDFLAG(IS_CHROMEOS)
 #include "chrome/browser/supervised_user/supervised_user_service.h"
 #include "chrome/browser/supervised_user/supervised_user_service_factory.h"
+#include "extensions/browser/api/management/management_api.h"
 #endif  // BUILDFLAG(ENABLE_SUPERVISED_USERS)
 
 using safe_browsing::SafeBrowsingNavigationObserverManager;
@@ -615,45 +616,47 @@ void WebstorePrivateBeginInstallWithManifest3Function::OnWebstoreParseFailure(
 
 #if BUILDFLAG(ENABLE_SUPERVISED_USERS)
 
-void WebstorePrivateBeginInstallWithManifest3Function::OnParentPermissionDone(
-    ParentPermissionDialog::Result result) {
+void WebstorePrivateBeginInstallWithManifest3Function::OnExtensionApprovalDone(
+    SupervisedUserExtensionsDelegate::ExtensionApprovalResult result) {
   switch (result) {
-    case ParentPermissionDialog::Result::kParentPermissionReceived:
-      OnParentPermissionReceived();
+    case SupervisedUserExtensionsDelegate::ExtensionApprovalResult::kApproved:
+      OnExtensionApprovalApproved();
       break;
-    case ParentPermissionDialog::Result::kParentPermissionCanceled:
-      OnParentPermissionCanceled();
+    case SupervisedUserExtensionsDelegate::ExtensionApprovalResult::kCanceled:
+      OnExtensionApprovalCanceled();
       break;
-    case ParentPermissionDialog::Result::kParentPermissionFailed:
-      OnParentPermissionFailed();
+    case SupervisedUserExtensionsDelegate::ExtensionApprovalResult::kFailed:
+      OnExtensionApprovalFailed();
+      break;
+    case SupervisedUserExtensionsDelegate::ExtensionApprovalResult::kBlocked:
+      OnExtensionApprovalBlocked();
       break;
   }
+  Release();  // Matches the AddRef in Run().
 }
 
 void WebstorePrivateBeginInstallWithManifest3Function::
-    OnParentPermissionReceived() {
+    OnExtensionApprovalApproved() {
   SupervisedUserService* service =
       SupervisedUserServiceFactory::GetForProfile(profile_);
   service->AddExtensionApproval(*dummy_extension_);
 
   HandleInstallProceed();
-  Release();  // Matches the AddRef in Run().
 }
 
 void WebstorePrivateBeginInstallWithManifest3Function::
-    OnParentPermissionCanceled() {
+    OnExtensionApprovalCanceled() {
   if (test_webstore_installer_delegate) {
     test_webstore_installer_delegate->OnExtensionInstallFailure(
         dummy_extension_->id(), kWebstoreParentPermissionFailedError,
         WebstoreInstaller::FailureReason::FAILURE_REASON_CANCELLED);
   }
 
   HandleInstallAbort(true /* user_initiated */);
-  Release();  // Matches the AddRef in Run().
 }
 
 void WebstorePrivateBeginInstallWithManifest3Function::
-    OnParentPermissionFailed() {
+    OnExtensionApprovalFailed() {
   if (test_webstore_installer_delegate) {
     test_webstore_installer_delegate->OnExtensionInstallFailure(
         dummy_extension_->id(), kWebstoreParentPermissionFailedError,
@@ -662,8 +665,12 @@ void WebstorePrivateBeginInstallWithManifest3Function::
 
   Respond(BuildResponse(api::webstore_private::RESULT_UNKNOWN_ERROR,
                         kWebstoreParentPermissionFailedError));
+}
 
-  Release();  // Matches the AddRef in Run().
+void WebstorePrivateBeginInstallWithManifest3Function::
+    OnExtensionApprovalBlocked() {
+  Respond(BuildResponse(api::webstore_private::RESULT_BLOCKED_FOR_CHILD_ACCOUNT,
+                        kParentBlockedExtensionInstallError));
 }
 
 bool WebstorePrivateBeginInstallWithManifest3Function::
@@ -677,26 +684,32 @@ bool WebstorePrivateBeginInstallWithManifest3Function::
     return false;
   }
 
-  ParentPermissionDialog::DoneCallback done_callback = base::BindOnce(
-      &WebstorePrivateBeginInstallWithManifest3Function::OnParentPermissionDone,
-      this);
-
-  parent_permission_dialog_ =
-      ParentPermissionDialog::CreateParentPermissionDialogForExtension(
-          profile_, web_contents->GetTopLevelNativeWindow(),
-          gfx::ImageSkia::CreateFrom1xBitmap(icon_), dummy_extension_.get(),
-          std::move(done_callback));
-  parent_permission_dialog_->ShowDialog();
+  auto extension_approval_callback =
+      base::BindOnce(&WebstorePrivateBeginInstallWithManifest3Function::
+                         OnExtensionApprovalDone,
+                     this);
+
+  SupervisedUserExtensionsDelegate* supervised_user_extensions_delegate =
+      ManagementAPI::GetFactoryInstance()
+          ->Get(profile_)
+          ->GetSupervisedUserExtensionsDelegate();
+  DCHECK(supervised_user_extensions_delegate);
+
+  // Assume that the block dialog will not be shown by the
+  // SupervisedUserExtensionsDelegate, because if permissions for extensions
+  // were disabled, the block dialog would have been shown at the install prompt
+  // step.
+  supervised_user_extensions_delegate->PromptForParentPermissionOrShowError(
+      *dummy_extension_, profile_, web_contents,
+      std::move(extension_approval_callback));
 
   return true;
 }
 
 void WebstorePrivateBeginInstallWithManifest3Function::
     OnBlockedByParentDialogDone() {
-  Respond(BuildResponse(api::webstore_private::RESULT_BLOCKED_FOR_CHILD_ACCOUNT,
-                        kParentBlockedExtensionInstallError));
-  // Matches the AddRef in Run().
-  Release();
+  OnExtensionApprovalDone(
+      SupervisedUserExtensionsDelegate::ExtensionApprovalResult::kBlocked);
 }
 
 #endif  // BUILDFLAG(ENABLE_SUPERVISED_USERS)

chrome/browser/extensions/api/webstore_private/webstore_private_api.h

@@ -27,7 +27,7 @@
 // TODO(https://crbug.com/1060801): Here and elsewhere, possibly switch build
 // flag to #if BUILDFLAG(IS_CHROMEOS)
 #include "chrome/browser/supervised_user/supervised_user_extensions_metrics_recorder.h"
-#include "chrome/browser/ui/supervised_user/parent_permission_dialog.h"
+#include "extensions/browser/supervised_user_extensions_delegate.h"
 #endif  // BUILDFLAG(ENABLE_SUPERVISED_USERS)
 
 class Profile;
@@ -92,13 +92,17 @@ class WebstorePrivateBeginInstallWithManifest3Function
                               const std::string& error_message) override;
 
 #if BUILDFLAG(ENABLE_SUPERVISED_USERS)
-  void OnParentPermissionDone(ParentPermissionDialog::Result result);
+  // Handles the result of the extension approval flow.
+  void OnExtensionApprovalDone(
+      SupervisedUserExtensionsDelegate::ExtensionApprovalResult result);
 
-  void OnParentPermissionReceived();
+  void OnExtensionApprovalApproved();
 
-  void OnParentPermissionCanceled();
+  void OnExtensionApprovalCanceled();
 
-  void OnParentPermissionFailed();
+  void OnExtensionApprovalFailed();
+
+  void OnExtensionApprovalBlocked();
 
   // Returns true if the parental approval prompt was shown, false if there was
   // an error showing it.
@@ -156,7 +160,6 @@ class WebstorePrivateBeginInstallWithManifest3Function
   std::u16string blocked_by_policy_error_message_;
 
 #if BUILDFLAG(ENABLE_SUPERVISED_USERS)
-  std::unique_ptr<ParentPermissionDialog> parent_permission_dialog_;
   SupervisedUserExtensionsMetricsRecorder
       supervised_user_extensions_metrics_recorder_;
 #endif  // BUILDFLAG(ENABLE_SUPERVISED_USERS)

chrome/browser/supervised_user/supervised_user_extensions_delegate_impl.cc

@@ -20,24 +20,24 @@
 namespace {
 
 void OnParentPermissionDialogComplete(
-    extensions::SupervisedUserExtensionsDelegate::
-        ParentPermissionDialogDoneCallback delegate_done_callback,
+    extensions::SupervisedUserExtensionsDelegate::ExtensionApprovalDoneCallback
+        delegate_done_callback,
     ParentPermissionDialog::Result result) {
   switch (result) {
     case ParentPermissionDialog::Result::kParentPermissionReceived:
       std::move(delegate_done_callback)
           .Run(extensions::SupervisedUserExtensionsDelegate::
-                   ParentPermissionDialogResult::kParentPermissionReceived);
+                   ExtensionApprovalResult::kApproved);
       break;
     case ParentPermissionDialog::Result::kParentPermissionCanceled:
       std::move(delegate_done_callback)
           .Run(extensions::SupervisedUserExtensionsDelegate::
-                   ParentPermissionDialogResult::kParentPermissionCanceled);
+                   ExtensionApprovalResult::kCanceled);
       break;
     case ParentPermissionDialog::Result::kParentPermissionFailed:
       std::move(delegate_done_callback)
           .Run(extensions::SupervisedUserExtensionsDelegate::
-                   ParentPermissionDialogResult::kParentPermissionFailed);
+                   ExtensionApprovalResult::kFailed);
       break;
   }
 }
@@ -71,8 +71,7 @@ void SupervisedUserExtensionsDelegateImpl::PromptForParentPermissionOrShowError(
     const extensions::Extension& extension,
     content::BrowserContext* browser_context,
     content::WebContents* web_contents,
-    ParentPermissionDialogDoneCallback parent_permission_callback,
-    base::OnceClosure error_callback) {
+    ExtensionApprovalDoneCallback extension_approval_callback) {
   DCHECK(IsChild(browser_context));
   DCHECK(!IsExtensionAllowedByParent(extension, browser_context));
 
@@ -82,10 +81,13 @@ void SupervisedUserExtensionsDelegateImpl::PromptForParentPermissionOrShowError(
   if (CanInstallExtensions(browser_context)) {
     ShowParentPermissionDialogForExtension(
         extension, browser_context, web_contents,
-        std::move(parent_permission_callback));
+        std::move(extension_approval_callback));
   } else {
     ShowExtensionEnableBlockedByParentDialogForExtension(
-        extension, web_contents, std::move(error_callback));
+        extension, web_contents,
+        base::BindOnce(std::move(extension_approval_callback),
+                       SupervisedUserExtensionsDelegate::
+                           ExtensionApprovalResult::kBlocked));
   }
 }
 
@@ -101,7 +103,7 @@ void SupervisedUserExtensionsDelegateImpl::
         const extensions::Extension& extension,
         content::BrowserContext* context,
         content::WebContents* contents,
-        ParentPermissionDialogDoneCallback done_callback) {
+        ExtensionApprovalDoneCallback done_callback) {
   ParentPermissionDialog::DoneCallback inner_done_callback = base::BindOnce(
       &::OnParentPermissionDialogComplete, std::move(done_callback));