[SafetyCheck] Add undo for 'Got it' button click

When clicking 'Got it' the list of unused site permissions is cleared and not shown again. On Undo the list is restored. This change adds the CPP side. The TS/UI side already landed.

Bug: 1345920
Change-Id: Id18f531e8a7901e1c7348e21aad0d5435fe224f8
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/4166069
Reviewed-by: Side YILMAZ <sideyilmaz@chromium.org>
Commit-Queue: Eduard Hez <ehez@chromium.org>
Reviewed-by: Christian Dullweber <dullweber@chromium.org>
Reviewed-by: Kamila Hasanbega <hkamila@chromium.org>
Cr-Commit-Position: refs/heads/main@{#1098372}

chrome/browser/resources/settings/site_settings/site_settings_permissions_browser_proxy.ts

@@ -16,6 +16,7 @@ import {ContentSettingsTypes} from './constants.js';
 export interface UnusedSitePermissions {
   origin: string;
   permissions: ContentSettingsTypes[];
+  expiration: string;
 }
 
 /**

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

@@ -5,6 +5,7 @@
 #include "chrome/browser/ui/webui/settings/site_settings_permissions_handler.h"
 
 #include "base/check.h"
+#include "base/json/values_util.h"
 #include "base/metrics/histogram_functions.h"
 #include "base/threading/thread_checker.h"
 #include "base/time/clock.h"
@@ -33,6 +34,10 @@ size_t kAllowAgainMetricsExclusiveMaxCount = 31;
 // Using a single bucket per day, following the value of
 // |kAllowAgainMetricsExclusiveMaxCount|.
 size_t kAllowAgainMetricsBuckets = 31;
+// Key of the expiration time in the |UnusedSitePermissions| object. Indicates
+// the time after which the associated origin and permissions are no longer
+// shown in the UI.
+constexpr char kExpirationKey[] = "expiration";
 }  // namespace
 
 SiteSettingsPermissionsHandler::SiteSettingsPermissionsHandler(Profile* profile)
@@ -88,6 +93,50 @@ void SiteSettingsPermissionsHandler::
   SendUnusedSitePermissionsReviewList();
 }
 
+void SiteSettingsPermissionsHandler::
+    HandleUndoAcknowledgeRevokedUnusedSitePermissionsList(
+        const base::Value::List& args) {
+  CHECK_EQ(1U, args.size());
+  CHECK(args[0].is_list());
+
+  const base::Value::List& unused_site_permissions_list = args[0].GetList();
+  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));
+    }
+
+    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));
+  }
+
+  SendUnusedSitePermissionsReviewList();
+}
+
 base::Value::List
 SiteSettingsPermissionsHandler::PopulateUnusedSitePermissionsData() {
   base::Value::List result;
@@ -126,6 +175,10 @@ SiteSettingsPermissionsHandler::PopulateUnusedSitePermissionsData() {
         site_settings::kPermissions,
         base::Value(std::move(permissions_value_list)));
 
+    revoked_permission_value.Set(
+        kExpirationKey,
+        base::TimeToValue(revoked_permissions.metadata.expiration));
+
     result.Append(std::move(revoked_permission_value));
   }
   return result;
@@ -149,6 +202,12 @@ void SiteSettingsPermissionsHandler::RegisterMessages() {
       base::BindRepeating(&SiteSettingsPermissionsHandler::
                               HandleAcknowledgeRevokedUnusedSitePermissionsList,
                           base::Unretained(this)));
+  web_ui()->RegisterMessageCallback(
+      "undoAcknowledgeRevokedUnusedSitePermissionsList",
+      base::BindRepeating(
+          &SiteSettingsPermissionsHandler::
+              HandleUndoAcknowledgeRevokedUnusedSitePermissionsList,
+          base::Unretained(this)));
 }
 
 void SiteSettingsPermissionsHandler::SendUnusedSitePermissionsReviewList() {

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

@@ -50,6 +50,12 @@ class SiteSettingsPermissionsHandler : public settings::SettingsPageUIHandler {
   void HandleAcknowledgeRevokedUnusedSitePermissionsList(
       const base::Value::List& args);
 
+  // Reverse the changes made by
+  // |HandleAcknowledgeRevokedUnusedSitePermissionsList|. List of revoked
+  // permissions is repopulated. Permission settings are not changed.
+  void HandleUndoAcknowledgeRevokedUnusedSitePermissionsList(
+      const base::Value::List& args);
+
   // Returns the list of revoked permissions that belongs to origins which
   // haven't been visited recently.
   base::Value::List PopulateUnusedSitePermissionsData();

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

@@ -171,4 +171,11 @@ TEST_F(SiteSettingsPermissionsHandlerTest,
   const auto& revoked_permissions_after =
       handler()->PopulateUnusedSitePermissionsData();
   EXPECT_EQ(revoked_permissions_after.size(), 0U);
+
+  // Undo reverts the list to its initial state.
+  base::Value::List undo_args;
+  undo_args.Append(revoked_permissions_before.Clone());
+  handler()->HandleUndoAcknowledgeRevokedUnusedSitePermissionsList(undo_args);
+  EXPECT_EQ(revoked_permissions_before,
+            handler()->PopulateUnusedSitePermissionsData());
 }

chrome/test/data/webui/settings/safety_check_permissions_test.ts

@@ -64,11 +64,13 @@ suite('SafetyCheckUnusedSitePermissionsUiTests', function() {
           ContentSettingsTypes.GEOLOCATION,
           ContentSettingsTypes.CAMERA,
         ],
+        expiration: '13317004800000000',  // Represents 2023-01-01T00:00:00.
       },
       {
         origin: origin2,
         permissions:
             [ContentSettingsTypes.POPUPS, ContentSettingsTypes.SENSORS],
+        expiration: '13348540800000000',  // Represents 2024-01-01T00:00:00.
       },
     ];
     createPage(mockData);
@@ -234,6 +236,7 @@ suite('SafetyCheckPagePermissionModulesTest', function() {
       origin: 'www.example1.com',
       permissions:
           [ContentSettingsTypes.GEOLOCATION, ContentSettingsTypes.CAMERA],
+      expiration: '13317004800000000',  // Represents 2023-01-01T00:00:00.
     },
   ];
 

chrome/test/data/webui/settings/site_settings_page_test.ts

@@ -248,6 +248,7 @@ suite('PrivacySandboxSettings4Disabled', function() {
 const unusedSitePermissionMockData = [{
   origin: 'www.example.com',
   permissions: [ContentSettingsTypes.CAMERA],
+  expiration: '13317004800000000',  // Represents 2023-01-01T00:00:00.
 }];
 
 suite('UnusedSitePermissionsReview', function() {

chrome/test/data/webui/settings/unused_site_permissions_test.ts

@@ -29,10 +29,12 @@ suite('CrSettingsUnusedSitePermissionsTest', function() {
     ContentSettingsTypes.NOTIFICATIONS,
   ];
 
-  const mockData = [1, 2, 3, 4].map(i => ({
-                                      origin: `https://www.example${i}.com:443`,
-                                      permissions: permissions.slice(0, i),
-                                    }));
+  const mockData = [1, 2, 3, 4].map(
+      i => ({
+        origin: `https://www.example${i}.com:443`,
+        permissions: permissions.slice(0, i),
+        expiration: '13317004800000000',  // Represents 2023-01-01T00:00:00.
+      }));
 
   /* Asserts for each row whether or not it is animating. */
   function assertAnimation(expectedAnimation: boolean[]) {

components/permissions/unused_site_permissions_service.cc

@@ -285,7 +285,15 @@ void UnusedSitePermissionsService::RevokeUnusedPermissions() {
        itr != recently_unused_permissions_.end();) {
     std::list<ContentSettingEntry>& unused_site_permissions = itr->second;
 
-    std::list<ContentSettingEntry> revoked_permissions;
+    // All |primary_pattern|s are equal across list items, the same is true for
+    // |secondary_pattern|s. This property is needed later and checked in the
+    // loop.
+    ContentSettingsPattern primary_pattern =
+        unused_site_permissions.front().source.primary_pattern;
+    ContentSettingsPattern secondary_pattern =
+        unused_site_permissions.front().source.secondary_pattern;
+
+    std::list<ContentSettingsType> revoked_permissions;
     for (auto permission_itr = unused_site_permissions.begin();
          permission_itr != unused_site_permissions.end();) {
       const ContentSettingEntry& entry = *permission_itr;
@@ -297,14 +305,17 @@ void UnusedSitePermissionsService::RevokeUnusedPermissions() {
         continue;
       }
 
+      DCHECK_EQ(entry.source.primary_pattern, primary_pattern);
+      DCHECK_EQ(entry.source.secondary_pattern, secondary_pattern);
+
       // Reset the permission to default if the site is visited before
       // threshold. Also, the secondary pattern should be wildcard.
       DCHECK(entry.source.metadata.last_visited != base::Time());
       DCHECK(entry.type != ContentSettingsType::NOTIFICATIONS);
       if (entry.source.metadata.last_visited < threshold &&
           entry.source.secondary_pattern ==
               ContentSettingsPattern::Wildcard()) {
-        revoked_permissions.push_back(entry);
+        revoked_permissions.push_back(entry.type);
         hcsm_->SetContentSettingCustomScope(
             entry.source.primary_pattern, entry.source.secondary_pattern,
             entry.type, ContentSetting::CONTENT_SETTING_DEFAULT);
@@ -316,7 +327,9 @@ void UnusedSitePermissionsService::RevokeUnusedPermissions() {
 
     // Store revoked permissions on HCSM.
     if (!revoked_permissions.empty()) {
-      StorePermissionInRevokedPermissionSetting(revoked_permissions);
+      StorePermissionInRevokedPermissionSetting(revoked_permissions,
+                                                absl::nullopt, primary_pattern,
+                                                secondary_pattern);
     }
 
     // Handle clean up of recently_unused_permissions_ map after revocation.
@@ -338,14 +351,24 @@ void UnusedSitePermissionsService::RevokeUnusedPermissions() {
 }
 
 void UnusedSitePermissionsService::StorePermissionInRevokedPermissionSetting(
-    const std::list<UnusedSitePermissionsService::ContentSettingEntry>&
-        recently_revoked_permissions) {
-  DCHECK(!recently_revoked_permissions.empty());
-  const ContentSettingsPattern& primary_pattern =
-      recently_revoked_permissions.front().source.primary_pattern;
-  const ContentSettingsPattern& secondary_pattern =
-      recently_revoked_permissions.front().source.secondary_pattern;
+    const std::list<ContentSettingsType> permissions,
+    const absl::optional<content_settings::ContentSettingConstraints>
+        constraint,
+    const url::Origin origin) {
+  // The |secondary_pattern| for
+  // |ContentSettingsType::REVOKED_UNUSED_SITE_PERMISSIONS| is always wildcard.
+  StorePermissionInRevokedPermissionSetting(
+      permissions, constraint,
+      ContentSettingsPattern::FromURLNoWildcard(origin.GetURL()),
+      ContentSettingsPattern::Wildcard());
+}
 
+void UnusedSitePermissionsService::StorePermissionInRevokedPermissionSetting(
+    const std::list<ContentSettingsType> permissions,
+    const absl::optional<content_settings::ContentSettingConstraints>
+        constraint,
+    const ContentSettingsPattern& primary_pattern,
+    const ContentSettingsPattern& secondary_pattern) {
   GURL url = GURL(primary_pattern.ToString());
   // The url should be valid as it is checked that the pattern represents a
   // single origin.
@@ -362,21 +385,22 @@ void UnusedSitePermissionsService::StorePermissionInRevokedPermissionSetting(
           ? std::move(*dict.FindList(permissions::kRevokedKey))
           : base::Value::List();
 
-  for (const auto& permission : recently_revoked_permissions) {
-    permission_type_list.Append(static_cast<int32_t>(permission.type));
+  for (const auto& permission : permissions) {
+    permission_type_list.Append(static_cast<int32_t>(permission));
   }
 
   dict.Set(kRevokedKey, base::Value::List(std::move(permission_type_list)));
 
-  const content_settings::ContentSettingConstraints constraint{
+  const content_settings::ContentSettingConstraints default_constraint = {
       .expiration = clock_->Now() + GetCleanUpThreshold()};
 
   // Set website setting for the list of recently revoked permissions and
   // previously revoked permissions, if exists.
   hcsm_->SetWebsiteSettingCustomScope(
       primary_pattern, secondary_pattern,
       ContentSettingsType::REVOKED_UNUSED_SITE_PERMISSIONS,
-      base::Value(std::move(dict)), constraint);
+      base::Value(std::move(dict)),
+      constraint.has_value() ? constraint.value() : default_constraint);
 }
 
 void UnusedSitePermissionsService::UpdateUnusedPermissionsForTesting() {

components/permissions/unused_site_permissions_service.h

@@ -14,6 +14,7 @@
 #include "base/time/clock.h"
 #include "base/timer/timer.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/keyed_service/core/keyed_service.h"
 #include "content/public/browser/web_contents_observer.h"
@@ -90,6 +91,13 @@ class UnusedSitePermissionsService
   // the user. Does not change permissions themselves.
   void ClearRevokedPermissionsList();
 
+  // Stores revoked permissions data on HCSM.
+  void StorePermissionInRevokedPermissionSetting(
+      const std::list<ContentSettingsType> permissions,
+      const absl::optional<content_settings::ContentSettingConstraints>
+          constraint,
+      const url::Origin origin);
+
   // Test support:
   void SetClockForTesting(base::Clock* clock);
   std::vector<ContentSettingEntry> GetTrackedUnusedPermissionsForTesting();
@@ -115,8 +123,11 @@ class UnusedSitePermissionsService
 
   // Stores revoked permissions data on HCSM.
   void StorePermissionInRevokedPermissionSetting(
-      const std::list<UnusedSitePermissionsService::ContentSettingEntry>&
-          recently_revoked_permissions);
+      const std::list<ContentSettingsType> permissions,
+      const absl::optional<content_settings::ContentSettingConstraints>
+          constraint,
+      const ContentSettingsPattern& primary_pattern,
+      const ContentSettingsPattern& secondary_pattern);
 
   // Set of permissions that haven't been used for at least a week.
   UnusedPermissionMap recently_unused_permissions_;

components/permissions/unused_site_permissions_service_unittest.cc

@@ -223,6 +223,49 @@ TEST_F(UnusedSitePermissionsServiceTest, TrackUnusedButDontRevoke) {
   EXPECT_EQ(GetRevokedPermissionsForOneOrigin(hcsm(), url).size(), 0u);
 }
 
+TEST_F(UnusedSitePermissionsServiceTest, SecondaryPatternAlwaysWildcard) {
+  base::test::ScopedFeatureList scoped_feature;
+  scoped_feature.InitAndEnableFeature(
+      content_settings::features::kSafetyCheckUnusedSitePermissions);
+
+  const ContentSettingsType types[] = {
+      ContentSettingsType::GEOLOCATION,
+      ContentSettingsType::AUTOMATIC_DOWNLOADS};
+  const content_settings::ContentSettingConstraints constraint{
+      .track_last_visit_for_autoexpiration = true};
+
+  // Test combinations of a single origin |primary_pattern| and different
+  // |secondary_pattern|s: equal to primary pattern, different single origin
+  // pattern, with domain with wildcard, wildcard.
+  for (const auto type : types) {
+    hcsm()->SetContentSettingDefaultScope(
+        GURL("https://example1.com"), GURL("https://example1.com"), type,
+        ContentSetting::CONTENT_SETTING_ALLOW, constraint);
+    hcsm()->SetContentSettingDefaultScope(
+        GURL("https://example2.com"), GURL("https://example3.com"), type,
+        ContentSetting::CONTENT_SETTING_ALLOW, constraint);
+    hcsm()->SetContentSettingDefaultScope(
+        GURL("https://example3.com"), GURL("https://[*.]example1.com"), type,
+        ContentSetting::CONTENT_SETTING_ALLOW, constraint);
+    hcsm()->SetContentSettingDefaultScope(
+        GURL("https://example4.com"), GURL("*"), type,
+        ContentSetting::CONTENT_SETTING_ALLOW, constraint);
+  }
+
+  service()->UpdateUnusedPermissionsForTesting();
+  EXPECT_EQ(GetRevokedUnusedPermissions(hcsm()).size(), 0u);
+
+  // Travel through time for 70 days so that permissions are revoked.
+  clock()->Advance(base::Days(70));
+  service()->UpdateUnusedPermissionsForTesting();
+
+  EXPECT_EQ(GetRevokedUnusedPermissions(hcsm()).size(), 4u);
+  for (auto unused_permission : GetRevokedUnusedPermissions(hcsm())) {
+    EXPECT_EQ(unused_permission.secondary_pattern,
+              ContentSettingsPattern::Wildcard());
+  }
+}
+
 TEST_F(UnusedSitePermissionsServiceTest, MultipleRevocationsForSameOrigin) {
   base::test::ScopedFeatureList scoped_feature;
   scoped_feature.InitAndEnableFeature(