[SafetyCheck] Add undo for 'Allow again' action for unused sites module

When clicking 'Allow again' for an origin's revoked permissions in the unused sites module the permission(s) is(are) granted again, the item is removed from the list of revoked permissions, and the origin is added to an ignore list for the future. This CL adds the undo functionality on the CPP side (TS side already landed).

Bug: 1345920, 1409416
Change-Id: If88b1a7d5a988dabfa0260c1725b4d1998523fed
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/4176863
Reviewed-by: Christian Dullweber <dullweber@chromium.org>
Reviewed-by: Illia Klimov <elklm@chromium.org>
Commit-Queue: Eduard Hez <ehez@chromium.org>
Reviewed-by: Rainhard Findling <rainhard@chromium.org>
Cr-Commit-Position: refs/heads/main@{#1098533}

chrome/browser/ui/webui/settings/site_settings_permissions_handler.cc

@@ -18,12 +18,10 @@
 #include "components/content_settings/core/browser/host_content_settings_map.h"
 #include "components/content_settings/core/common/content_settings.h"
 #include "components/content_settings/core/common/content_settings_pattern.h"
-#include "components/content_settings/core/common/content_settings_types.h"
 #include "components/content_settings/core/common/features.h"
 #include "components/permissions/constants.h"
 #include "components/permissions/unused_site_permissions_service.h"
 #include "url/gurl.h"
-#include "url/origin.h"
 
 namespace {
 // Reflects the maximum number of days between a permissions being revoked and
@@ -59,6 +57,7 @@ void SiteSettingsPermissionsHandler::HandleGetRevokedUnusedSitePermissionsList(
 void SiteSettingsPermissionsHandler::HandleAllowPermissionsAgainForUnusedSite(
     const base::Value::List& args) {
   CHECK_EQ(1U, args.size());
+  CHECK(args[0].is_string());
   const std::string& origin_str = args[0].GetString();
 
   permissions::UnusedSitePermissionsService* service =
@@ -83,6 +82,22 @@ void SiteSettingsPermissionsHandler::HandleAllowPermissionsAgainForUnusedSite(
   SendUnusedSitePermissionsReviewList();
 }
 
+void SiteSettingsPermissionsHandler::
+    HandleUndoAllowPermissionsAgainForUnusedSite(
+        const base::Value::List& args) {
+  CHECK_EQ(1U, args.size());
+  CHECK(args[0].is_dict());
+
+  auto [origin, permissions, constraints] =
+      GetUnusedSitePermissionsFromDict(args[0].GetDict());
+  permissions::UnusedSitePermissionsService* service =
+      UnusedSitePermissionsServiceFactory::GetForProfile(profile_);
+
+  service->UndoRegrantPermissionsForOrigin(permissions, constraints, origin);
+
+  SendUnusedSitePermissionsReviewList();
+}
+
 void SiteSettingsPermissionsHandler::
     HandleAcknowledgeRevokedUnusedSitePermissionsList(
         const base::Value::List& args) {
@@ -103,35 +118,13 @@ void SiteSettingsPermissionsHandler::
   permissions::UnusedSitePermissionsService* service =
       UnusedSitePermissionsServiceFactory::GetForProfile(profile_);
 
-  for (const auto& unused_site_permissions : unused_site_permissions_list) {
-    CHECK(unused_site_permissions.is_dict());
-    const std::string* origin_str =
-        unused_site_permissions.GetDict().FindString(site_settings::kOrigin);
-    CHECK(origin_str);
-    const auto url = GURL(*origin_str);
-    CHECK(url.is_valid());
-
-    const base::Value::List* permissions =
-        unused_site_permissions.GetDict().FindList(site_settings::kPermissions);
-    CHECK(permissions);
-    std::list<ContentSettingsType> permission_types;
-    for (const auto& permission : *permissions) {
-      CHECK(permission.is_string());
-      const std::string& type = permission.GetString();
-      permission_types.push_back(
-          site_settings::ContentSettingsTypeFromGroupName(type));
-    }
+  for (const auto& unused_site_permissions_js : unused_site_permissions_list) {
+    CHECK(unused_site_permissions_js.is_dict());
+    auto [origin, permissions, constraints] =
+        GetUnusedSitePermissionsFromDict(unused_site_permissions_js.GetDict());
 
-    const base::Value* js_expiration =
-        unused_site_permissions.GetDict().Find(kExpirationKey);
-    CHECK(js_expiration);
-    auto expiration = base::ValueToTime(js_expiration);
-
-    const content_settings::ContentSettingConstraints constraint{
-        .expiration = *expiration};
-
-    service->StorePermissionInRevokedPermissionSetting(
-        permission_types, constraint, url::Origin::Create(url));
+    service->StorePermissionInRevokedPermissionSetting(permissions, constraints,
+                                                       origin);
   }
 
   SendUnusedSitePermissionsReviewList();
@@ -184,6 +177,40 @@ SiteSettingsPermissionsHandler::PopulateUnusedSitePermissionsData() {
   return result;
 }
 
+std::tuple<url::Origin,
+           std::set<ContentSettingsType>,
+           content_settings::ContentSettingConstraints>
+SiteSettingsPermissionsHandler::GetUnusedSitePermissionsFromDict(
+    const base::Value::Dict& unused_site_permissions) {
+  const std::string* origin_str =
+      unused_site_permissions.FindString(site_settings::kOrigin);
+  CHECK(origin_str);
+  const auto url = GURL(*origin_str);
+  CHECK(url.is_valid());
+  const url::Origin origin = url::Origin::Create(url);
+
+  const base::Value::List* permissions =
+      unused_site_permissions.FindList(site_settings::kPermissions);
+  CHECK(permissions);
+  std::set<ContentSettingsType> permission_types;
+  for (const auto& permission : *permissions) {
+    CHECK(permission.is_string());
+    const std::string& type = permission.GetString();
+    permission_types.insert(
+        site_settings::ContentSettingsTypeFromGroupName(type));
+  }
+
+  const base::Value* js_expiration =
+      unused_site_permissions.Find(kExpirationKey);
+  CHECK(js_expiration);
+  auto expiration = base::ValueToTime(js_expiration);
+
+  const content_settings::ContentSettingConstraints constraints{
+      .expiration = *expiration};
+
+  return std::make_tuple(origin, permission_types, constraints);
+}
+
 void SiteSettingsPermissionsHandler::RegisterMessages() {
   // Usage of base::Unretained(this) is safe, because web_ui() owns `this` and
   // won't release ownership until destruction.
@@ -197,6 +224,11 @@ void SiteSettingsPermissionsHandler::RegisterMessages() {
       base::BindRepeating(&SiteSettingsPermissionsHandler::
                               HandleAllowPermissionsAgainForUnusedSite,
                           base::Unretained(this)));
+  web_ui()->RegisterMessageCallback(
+      "undoAllowPermissionsAgainForUnusedSite",
+      base::BindRepeating(&SiteSettingsPermissionsHandler::
+                              HandleUndoAllowPermissionsAgainForUnusedSite,
+                          base::Unretained(this)));
   web_ui()->RegisterMessageCallback(
       "acknowledgeRevokedUnusedSitePermissionsList",
       base::BindRepeating(&SiteSettingsPermissionsHandler::

chrome/browser/ui/webui/settings/site_settings_permissions_handler.h

@@ -9,6 +9,9 @@
 #include "base/time/clock.h"
 #include "chrome/browser/profiles/profile.h"
 #include "chrome/browser/ui/webui/settings/settings_page_ui_handler.h"
+#include "components/content_settings/core/common/content_settings_constraints.h"
+#include "components/content_settings/core/common/content_settings_types.h"
+#include "url/origin.h"
 
 /**
  * This handler deals with the permission-related operations on the site
@@ -41,17 +44,23 @@ class SiteSettingsPermissionsHandler : public settings::SettingsPageUIHandler {
   // "Unused site permissions" module.
   void HandleGetRevokedUnusedSitePermissionsList(const base::Value::List& args);
 
-  // Re-grant the revoked permissions and remove the origin from the revoked
-  // permissions list.
+  // Re-grant the revoked permissions and remove the given origin from the
+  // revoked permissions list.
   void HandleAllowPermissionsAgainForUnusedSite(const base::Value::List& args);
 
+  // Reverse the changes made by |HandleAllowPermissionsAgainForUnusedSite| for
+  // the given |UnusedSitePermission| object.
+  void HandleUndoAllowPermissionsAgainForUnusedSite(
+      const base::Value::List& args);
+
   // Clear the list of revoked permissions so they are not shown again.
   // Permission settings themselves are not affected by this.
   void HandleAcknowledgeRevokedUnusedSitePermissionsList(
       const base::Value::List& args);
 
   // Reverse the changes made by
-  // |HandleAcknowledgeRevokedUnusedSitePermissionsList|. List of revoked
+  // |HandleAcknowledgeRevokedUnusedSitePermissionsList| for the given list of
+  // |UnusedSitePermission| objects. List of revoked
   // permissions is repopulated. Permission settings are not changed.
   void HandleUndoAcknowledgeRevokedUnusedSitePermissionsList(
       const base::Value::List& args);
@@ -63,6 +72,14 @@ class SiteSettingsPermissionsHandler : public settings::SettingsPageUIHandler {
   // Sends the list of unused site permissions to review to the WebUI.
   void SendUnusedSitePermissionsReviewList();
 
+  // Get values from |UnusedSitePermission| object in
+  // site_settings_permissions_browser_proxy.ts.
+  std::tuple<url::Origin,
+             std::set<ContentSettingsType>,
+             content_settings::ContentSettingConstraints>
+  GetUnusedSitePermissionsFromDict(
+      const base::Value::Dict& unused_site_permissions);
+
   const raw_ptr<Profile> profile_;
 
   base::Clock* clock_;

chrome/browser/ui/webui/settings/site_settings_permissions_handler_unittest.cc

@@ -30,6 +30,8 @@
 
 constexpr char kUnusedTestSite[] = "https://example1.com";
 constexpr char kUsedTestSite[] = "https://example2.com";
+constexpr ContentSettingsType kUnusedPermission =
+    ContentSettingsType::GEOLOCATION;
 
 class SiteSettingsPermissionsHandlerTest : public testing::Test {
  public:
@@ -93,6 +95,18 @@ class SiteSettingsPermissionsHandlerTest : public testing::Test {
     }
   }
 
+  void ExpectRevokedPermission() {
+    ContentSettingsForOneType revoked_permissions_list;
+    hcsm()->GetSettingsForOneType(
+        ContentSettingsType::REVOKED_UNUSED_SITE_PERMISSIONS,
+        &revoked_permissions_list);
+    EXPECT_EQ(1U, revoked_permissions_list.size());
+    EXPECT_EQ(
+        ContentSetting::CONTENT_SETTING_ASK,
+        hcsm()->GetContentSetting(GURL(kUnusedTestSite), GURL(kUnusedTestSite),
+                                  kUnusedPermission));
+  }
+
   TestingProfile* profile() { return profile_.get(); }
   content::TestWebUI* web_ui() { return &web_ui_; }
   SiteSettingsPermissionsHandler* handler() { return handler_.get(); }
@@ -133,6 +147,9 @@ TEST_F(SiteSettingsPermissionsHandlerTest,
   // Advance 14 days; this will be the expected histogram sample.
   clock()->Advance(base::Days(14));
   base::HistogramTester histogram_tester;
+  base::Value::List initial_unused_site_permissions =
+      handler()->PopulateUnusedSitePermissionsData();
+  ExpectRevokedPermission();
 
   // Allow the revoked permission for the unused site again.
   base::Value::List args;
@@ -157,7 +174,12 @@ TEST_F(SiteSettingsPermissionsHandlerTest,
   EXPECT_EQ(
       ContentSetting::CONTENT_SETTING_ALLOW,
       hcsm()->GetContentSetting(GURL(kUnusedTestSite), GURL(kUnusedTestSite),
-                                ContentSettingsType::GEOLOCATION));
+                                kUnusedPermission));
+
+  // Undoing restores the initial state.
+  handler()->HandleUndoAllowPermissionsAgainForUnusedSite(
+      std::move(initial_unused_site_permissions));
+  ExpectRevokedPermission();
 }
 
 TEST_F(SiteSettingsPermissionsHandlerTest,

components/permissions/unused_site_permissions_service.cc

@@ -191,6 +191,21 @@ void UnusedSitePermissionsService::RegrantPermissionsForOrigin(
       ContentSettingsType::REVOKED_UNUSED_SITE_PERMISSIONS, {});
 }
 
+void UnusedSitePermissionsService::UndoRegrantPermissionsForOrigin(
+    const std::set<ContentSettingsType> permissions,
+    const absl::optional<content_settings::ContentSettingConstraints>
+        constraint,
+    const url::Origin origin) {
+  for (const auto& permission : permissions) {
+    hcsm_->SetContentSettingCustomScope(
+        ContentSettingsPattern::FromURLNoWildcard(origin.GetURL()),
+        ContentSettingsPattern::Wildcard(), permission,
+        ContentSetting::CONTENT_SETTING_DEFAULT);
+  }
+
+  StorePermissionInRevokedPermissionSetting(permissions, constraint, origin);
+}
+
 void UnusedSitePermissionsService::ClearRevokedPermissionsList() {
   ContentSettingsForOneType settings;
   hcsm_->GetSettingsForOneType(
@@ -293,7 +308,7 @@ void UnusedSitePermissionsService::RevokeUnusedPermissions() {
     ContentSettingsPattern secondary_pattern =
         unused_site_permissions.front().source.secondary_pattern;
 
-    std::list<ContentSettingsType> revoked_permissions;
+    std::set<ContentSettingsType> revoked_permissions;
     for (auto permission_itr = unused_site_permissions.begin();
          permission_itr != unused_site_permissions.end();) {
       const ContentSettingEntry& entry = *permission_itr;
@@ -315,7 +330,7 @@ void UnusedSitePermissionsService::RevokeUnusedPermissions() {
       if (entry.source.metadata.last_visited < threshold &&
           entry.source.secondary_pattern ==
               ContentSettingsPattern::Wildcard()) {
-        revoked_permissions.push_back(entry.type);
+        revoked_permissions.insert(entry.type);
         hcsm_->SetContentSettingCustomScope(
             entry.source.primary_pattern, entry.source.secondary_pattern,
             entry.type, ContentSetting::CONTENT_SETTING_DEFAULT);
@@ -351,7 +366,7 @@ void UnusedSitePermissionsService::RevokeUnusedPermissions() {
 }
 
 void UnusedSitePermissionsService::StorePermissionInRevokedPermissionSetting(
-    const std::list<ContentSettingsType> permissions,
+    const std::set<ContentSettingsType> permissions,
     const absl::optional<content_settings::ContentSettingConstraints>
         constraint,
     const url::Origin origin) {
@@ -364,7 +379,7 @@ void UnusedSitePermissionsService::StorePermissionInRevokedPermissionSetting(
 }
 
 void UnusedSitePermissionsService::StorePermissionInRevokedPermissionSetting(
-    const std::list<ContentSettingsType> permissions,
+    const std::set<ContentSettingsType> permissions,
     const absl::optional<content_settings::ContentSettingConstraints>
         constraint,
     const ContentSettingsPattern& primary_pattern,

components/permissions/unused_site_permissions_service.h

@@ -87,13 +87,21 @@ class UnusedSitePermissionsService
   // from revoked permissions list.
   void RegrantPermissionsForOrigin(const url::Origin& origin);
 
+  // Reverse changes made by |RegrantPermissionsForOrigin|. Adds this origin to
+  // the removed permissions list and resets its permissions.
+  void UndoRegrantPermissionsForOrigin(
+      const std::set<ContentSettingsType> permissions,
+      const absl::optional<content_settings::ContentSettingConstraints>
+          constraint,
+      const url::Origin origin);
+
   // Clear the list of revoked permissions so they will no longer be shown to
   // the user. Does not change permissions themselves.
   void ClearRevokedPermissionsList();
 
   // Stores revoked permissions data on HCSM.
   void StorePermissionInRevokedPermissionSetting(
-      const std::list<ContentSettingsType> permissions,
+      const std::set<ContentSettingsType> permissions,
       const absl::optional<content_settings::ContentSettingConstraints>
           constraint,
       const url::Origin origin);
@@ -123,7 +131,7 @@ class UnusedSitePermissionsService
 
   // Stores revoked permissions data on HCSM.
   void StorePermissionInRevokedPermissionSetting(
-      const std::list<ContentSettingsType> permissions,
+      const std::set<ContentSettingsType> permissions,
       const absl::optional<content_settings::ContentSettingConstraints>
           constraint,
       const ContentSettingsPattern& primary_pattern,