Show notification when user request an approved or rejected extension

Request extension is a multi-steps process. The policy may be updated
during the requests. If policy is updated to whitelist or blacklist an
extension right before a user confirms the request dialog, we will show
the request approved/rejected notification immediately.

Bug: 1006899
Change-Id: I0f41774b8c1a89dd9b34465f8237320bdddc6dce
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2089996
Commit-Queue: Owen Min <zmin@chromium.org>
Reviewed-by: Karan Bhatia <karandeepb@chromium.org>
Reviewed-by: Julian Pastarmov <pastarmovj@chromium.org>
Cr-Commit-Position: refs/heads/master@{#748354}

chrome/browser/enterprise_reporting/notification/extension_request_observer.cc

@@ -18,6 +18,11 @@ ExtensionRequestObserver::ExtensionRequestObserver(Profile* profile)
   extensions::ExtensionManagementFactory::GetForBrowserContext(profile_)
       ->AddObserver(this);
   OnExtensionManagementSettingsChanged();
+  pref_change_registrar_.Init(profile_->GetPrefs());
+  pref_change_registrar_.Add(
+      prefs::kCloudExtensionRequestIds,
+      base::BindRepeating(&ExtensionRequestObserver::OnPendingListChanged,
+                          weak_factory_.GetWeakPtr()));
 }
 
 ExtensionRequestObserver::~ExtensionRequestObserver() {
@@ -30,6 +35,21 @@ ExtensionRequestObserver::~ExtensionRequestObserver() {
 }
 
 void ExtensionRequestObserver::OnExtensionManagementSettingsChanged() {
+  ShowAllNotifications();
+}
+
+void ExtensionRequestObserver::OnPendingListChanged() {
+  // The pending list is updated when user confirm the notification and requests
+  // are removed from the list. There is no need to show new notification at
+  // this point.
+  if (closing_notification_and_deleting_requests_) {
+    closing_notification_and_deleting_requests_ = false;
+    return;
+  }
+  ShowAllNotifications();
+}
+
+void ExtensionRequestObserver::ShowAllNotifications() {
   if (!profile_->GetPrefs()->GetBoolean(prefs::kCloudExtensionRequestEnabled)) {
     CloseAllNotifications();
     return;
@@ -112,6 +132,8 @@ void ExtensionRequestObserver::RemoveExtensionsFromPendingList(
       prefs::kCloudExtensionRequestIds);
   for (auto& id : extension_ids)
     pending_requests_update->RemoveKey(id);
+
+  closing_notification_and_deleting_requests_ = true;
 }
 
 }  // namespace enterprise_reporting

chrome/browser/enterprise_reporting/notification/extension_request_observer.h

@@ -26,6 +26,13 @@ class ExtensionRequestObserver
   // extensions::ExtensionManagement::Observer
   void OnExtensionManagementSettingsChanged() override;
 
+  // Notifies when request pending list is updated.
+  void OnPendingListChanged();
+
+  // Shows notifications when requests are approved, rejected or
+  // force-installed. It also closes the notification that is no longer needed.
+  void ShowAllNotifications();
+
   void ShowNotification(ExtensionRequestNotification::NotifyType type);
   void CloseAllNotifications();
 
@@ -40,6 +47,9 @@ class ExtensionRequestObserver
 
   Profile* profile_;
 
+  PrefChangeRegistrar pref_change_registrar_;
+  bool closing_notification_and_deleting_requests_ = false;
+
   base::WeakPtrFactory<ExtensionRequestObserver> weak_factory_{this};
 };
 

chrome/browser/enterprise_reporting/notification/extension_request_observer_unittest.cc

@@ -256,4 +256,16 @@ TEST_F(ExtensionRequestObserverTest, ExtensionRequestPolicyToggle) {
                 ->DictSize());
 }
 
+TEST_F(ExtensionRequestObserverTest, PendingRequestAddedAfterPolicyUpdated) {
+  ExtensionRequestObserver observer(profile());
+  VerifyNotification(false);
+
+  SetExtensionSettings(kExtensionSettings);
+  VerifyNotification(false);
+
+  SetPendingList({kExtensionId1, kExtensionId2, kExtensionId3, kExtensionId4,
+                  kExtensionId5, kExtensionId6});
+  VerifyNotification(true);
+}
+
 }  // namespace enterprise_reporting

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

@@ -220,8 +220,22 @@ ExtensionInstallStatus AddExtensionToPendingList(const ExtensionId& id,
                                                  Profile* profile) {
   ExtensionInstallStatus status =
       GetWebstoreExtensionInstallStatus(id, profile);
-  if (status != kCanRequest)
+  // We put the |id| into the pending request list if it can be requested.
+  // Ideally we should not get here if the status is not |kCanRequest|. However
+  // policy might be updated between the client calling |requestExtension| or
+  // |beginInstallWithManifest3| and us checking the status here. Handle
+  // approvals and rejections for this case by adding the |id| into the pending
+  // list. ExtensionRequestObserver will observe this update and show the
+  // notificaion immediately.
+  // Please note that only the |id| that can be requested will be uploaded to
+  // the server and ExtensionRequestObserver will also show notifications once
+  // it's approved or rejected.
+  // |id| will be removed from the pending list once the notification is
+  // confirmed or closed by the user.
+  if (status != kCanRequest && status != kInstallable &&
+      status != kBlockedByPolicy) {
     return status;
+  }
 
   DictionaryPrefUpdate pending_requests_update(
       profile->GetPrefs(), prefs::kCloudExtensionRequestIds);
@@ -231,11 +245,17 @@ ExtensionInstallStatus AddExtensionToPendingList(const ExtensionId& id,
                       ::util::TimeToValue(base::Time::Now()));
   pending_requests_update->SetKey(id, std::move(request_data));
   // Query the new extension install status again. It should be changed from
-  // kCanRequest to kRequestPending if the id has been added into pending list
-  // successfully.
-  status = GetWebstoreExtensionInstallStatus(id, profile);
-  DCHECK_EQ(kRequestPending, status);
-  return status;
+  // |kCanRequest| to |kRequestPending| if the id has been added into pending
+  // list successfully. Otherwise, it shouldn't be changed.
+  ExtensionInstallStatus new_status =
+      GetWebstoreExtensionInstallStatus(id, profile);
+#if DCHECK_IS_ON()
+  if (status == kCanRequest)
+    DCHECK_EQ(kRequestPending, new_status);
+  else
+    DCHECK_EQ(status, new_status);
+#endif  // DCHECK_IS_ON()
+  return new_status;
 }
 
 }  // namespace

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

@@ -6,6 +6,7 @@
 
 #include <vector>
 
+#include "base/json/json_reader.h"
 #include "base/macros.h"
 #include "base/strings/stringprintf.h"
 #include "base/util/values/values_util.h"
@@ -19,6 +20,7 @@
 #include "extensions/browser/api_test_utils.h"
 #include "extensions/browser/extension_dialog_auto_confirm.h"
 #include "extensions/browser/extension_registry.h"
+#include "extensions/browser/pref_names.h"
 #include "extensions/common/extension_builder.h"
 
 namespace extensions {
@@ -31,6 +33,18 @@ constexpr char kExtensionManifest[] = R"({
   \"manifest_version\": 3,
   \"version\": \"0.1\"})";
 
+constexpr char kAllowedExtensionSettings[] = R"({
+  "abcdefghijklmnopabcdefghijklmnop" : {
+    "installation_mode": "allowed"
+  }
+})";
+
+constexpr char kBlockedExtensionSettings[] = R"({
+  "abcdefghijklmnopabcdefghijklmnop" : {
+    "installation_mode": "blocked"
+  }
+})";
+
 constexpr char kWebstoreUserCancelledError[] = "User cancelled install";
 
 base::Time GetFaketime() {
@@ -52,6 +66,16 @@ void VerifyPendingList(
   }
 }
 
+void SetExtensionSettings(const std::string& settings_string,
+                          TestingProfile* profile) {
+  base::Optional<base::Value> settings =
+      base::JSONReader::Read(settings_string);
+  ASSERT_TRUE(settings.has_value());
+  profile->GetTestingPrefService()->SetManagedPref(
+      pref_names::kExtensionManagement,
+      base::Value::ToUniquePtrValue(std::move(*settings)));
+}
+
 }  // namespace
 
 class WebstorePrivateExtensionInstallRequestBase : public ExtensionApiUnittest {
@@ -151,6 +175,29 @@ TEST_F(WebstorePrivateRequestExtensionTest, UnrequestableExtension) {
   VerifyPendingList({}, profile());
 }
 
+TEST_F(WebstorePrivateRequestExtensionTest, AlreadyApprovedExtension) {
+  SetExtensionSettings(kAllowedExtensionSettings, profile());
+  auto function =
+      base::MakeRefCounted<WebstorePrivateRequestExtensionFunction>();
+  std::unique_ptr<base::Value> response =
+      RunFunctionAndReturnValue(function.get(), GenerateArgs(kExtensionId));
+  VerifyResponse(ExtensionInstallStatus::EXTENSION_INSTALL_STATUS_INSTALLABLE,
+                 response.get());
+  VerifyPendingList({{kExtensionId, base::Time::Now()}}, profile());
+}
+
+TEST_F(WebstorePrivateRequestExtensionTest, AlreadyRejectedExtension) {
+  SetExtensionSettings(kBlockedExtensionSettings, profile());
+  auto function =
+      base::MakeRefCounted<WebstorePrivateRequestExtensionFunction>();
+  std::unique_ptr<base::Value> response =
+      RunFunctionAndReturnValue(function.get(), GenerateArgs(kExtensionId));
+  VerifyResponse(
+      ExtensionInstallStatus::EXTENSION_INSTALL_STATUS_BLOCKED_BY_POLICY,
+      response.get());
+  VerifyPendingList({{kExtensionId, base::Time::Now()}}, profile());
+}
+
 TEST_F(WebstorePrivateRequestExtensionTest, AlreadyPendingExtension) {
   SetPendingList({kExtensionId});
   VerifyPendingList({{kExtensionId, GetFaketime()}}, profile());