[M116] Fix crash when group name is not defined for permissions

This Cl fixes the crash that occurs whenever safety hub revokes a
permission whose ContentSettingsTypeFromGroupName is undefined by adding
null checks.

(cherry picked from commit 821cc1a)

Bug: 1459305
Change-Id: I2d0a4fd6ec1dd5e3fa2ba697739e4999a06f6613
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/4793613
Commit-Queue: Martin Šrámek <msramek@chromium.org>
Auto-Submit: Side YILMAZ <sideyilmaz@chromium.org>
Reviewed-by: Martin Šrámek <msramek@chromium.org>
Commit-Queue: Side YILMAZ <sideyilmaz@chromium.org>
Cr-Original-Commit-Position: refs/heads/main@{#1186942}
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/4804863
Reviewed-by: Elias Klim <elklm@chromium.org>
Cr-Commit-Position: refs/branch-heads/5845@{#1590}
Cr-Branched-From: 5a5dff6-refs/heads/main@{#1160321}

chrome/browser/permissions/unused_site_permissions_service_browsertest.cc

@@ -13,6 +13,7 @@
 #include "chrome/browser/ui/browser.h"
 #include "chrome/test/base/in_process_browser_test.h"
 #include "chrome/test/base/ui_test_utils.h"
+#include "components/content_settings/core/browser/content_settings_registry.h"
 #include "components/content_settings/core/browser/content_settings_utils.h"
 #include "components/content_settings/core/browser/host_content_settings_map.h"
 #include "components/content_settings/core/common/content_settings.h"
@@ -141,3 +142,52 @@ IN_PROC_BROWSER_TEST_F(UnusedSitePermissionsServiceBrowserTest,
   EXPECT_EQ(CONTENT_SETTING_ASK,
             map->GetContentSetting(url, url, ContentSettingsType::GEOLOCATION));
 }
+
+// Test that revocation happens correctly for all content setting types.
+IN_PROC_BROWSER_TEST_F(UnusedSitePermissionsServiceBrowserTest,
+                       RevokeAllContentSettingTypes) {
+  auto* map =
+      HostContentSettingsMapFactory::GetForProfile(browser()->profile());
+  auto* service =
+      UnusedSitePermissionsServiceFactory::GetForProfile(browser()->profile());
+
+  base::Time time;
+  ASSERT_TRUE(base::Time::FromString("2022-09-07 13:00", &time));
+  base::SimpleTestClock clock;
+  clock.SetNow(time);
+  map->SetClockForTesting(&clock);
+  service->SetClockForTesting(&clock);
+
+  // Allow all content settings in the content setting registry.
+  auto* content_settings_registry =
+      content_settings::ContentSettingsRegistry::GetInstance();
+  for (const content_settings::ContentSettingsInfo* info :
+       *content_settings_registry) {
+    ContentSettingsType type = info->website_settings_info()->type();
+
+    // Add last visited timestamp if the setting can be tracked.
+    content_settings::ContentSettingConstraints constraint;
+    if (content_settings::CanTrackLastVisit(type)) {
+      constraint.set_track_last_visit_for_autoexpiration(true);
+    }
+
+    if (!content_settings_registry->Get(type)->IsSettingValid(
+            ContentSetting::CONTENT_SETTING_ALLOW)) {
+      continue;
+    }
+
+    const GURL url("https://example1.com");
+    map->SetContentSettingDefaultScope(GURL(url), GURL(url), type,
+                                       ContentSetting::CONTENT_SETTING_ALLOW,
+                                       constraint);
+  }
+
+  // Travel through time for 70 days to make permissions be revoked.
+  clock.Advance(base::Days(70));
+  service->UpdateUnusedPermissionsForTesting();
+  ASSERT_EQ(GetRevokedUnusedPermissions(map).size(), 1u);
+
+  // Navigate to content settings page.
+  ASSERT_TRUE(ui_test_utils::NavigateToURL(browser(),
+                                           GURL("chrome://settings/content")));
+}

chrome/browser/resources/settings/site_settings_page/unused_site_permissions.ts

@@ -213,8 +213,7 @@ export class SettingsUnusedSitePermissionsElement extends
     const permissionsI18n = permissions.map(permission => {
       const localizationString =
           getLocalizationStringForContentType(permission);
-      assert(localizationString !== null);
-      return this.i18n(localizationString);
+      return localizationString ? this.i18n(localizationString) : '';
     });
 
     if (permissionsI18n.length === 1) {

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

@@ -200,9 +200,21 @@ base::Value::List SafetyHubHandler::PopulateUnusedSitePermissionsData() {
         stored_value.GetDict().FindList(permissions::kRevokedKey)->Clone();
     base::Value::List permissions_value_list;
     for (base::Value& type : type_list) {
-      permissions_value_list.Append(
+      base::StringPiece permission_str =
           site_settings::ContentSettingsTypeToGroupName(
-              static_cast<ContentSettingsType>(type.GetInt())));
+              static_cast<ContentSettingsType>(type.GetInt()));
+      if (!permission_str.empty()) {
+        permissions_value_list.Append(permission_str);
+      }
+    }
+
+    // Some permissions have no readable name, although Safety Hub revokes them.
+    // To prevent crashes, if there is no permission to be shown in the UI, the
+    // origin will not be added to the revoked permissions list.
+    // TODO(crbug.com/1459305): Remove this after adding check for
+    // ContentSettingsTypeToGroupName.
+    if (permissions_value_list.empty()) {
+      continue;
     }
 
     revoked_permission_value.Set(

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

@@ -53,6 +53,7 @@ class SafetyHubHandler : public settings::SettingsPageUIHandler {
   FRIEND_TEST_ALL_PREFIXES(
       SafetyHubHandlerTest,
       SendNotificationPermissionReviewList_FeatureDisabled);
+  FRIEND_TEST_ALL_PREFIXES(SafetyHubHandlerTest, RevokeAllContentSettingTypes);
 
   // SettingsPageUIHandler implementation.
   void OnJavascriptAllowed() override;

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

@@ -15,6 +15,7 @@
 #include "chrome/browser/ui/webui/settings/site_settings_helper.h"
 #include "chrome/common/chrome_features.h"
 #include "chrome/test/base/testing_profile.h"
+#include "components/content_settings/core/browser/content_settings_registry.h"
 #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"
@@ -332,3 +333,58 @@ TEST_F(SafetyHubHandlerTest, HandleResetNotificationPermissionForOrigins) {
 
   ValidateNotificationPermissionUpdate();
 }
+
+// Test that revocation is happen correctly for all content setting types.
+TEST_F(SafetyHubHandlerTest, RevokeAllContentSettingTypes) {
+  // TODO(crbug.com/1459305): Remove this after adding names for those
+  // types.
+  std::list<ContentSettingsType> no_name_types = {
+      ContentSettingsType::MIDI,
+      ContentSettingsType::DURABLE_STORAGE,
+      ContentSettingsType::ACCESSIBILITY_EVENTS,
+      ContentSettingsType::NFC,
+      ContentSettingsType::FILE_SYSTEM_READ_GUARD,
+      ContentSettingsType::CAMERA_PAN_TILT_ZOOM,
+      ContentSettingsType::TOP_LEVEL_STORAGE_ACCESS};
+
+  // Add all content settings in the content setting registry to revoked
+  // permissions list.
+  auto* content_settings_registry =
+      content_settings::ContentSettingsRegistry::GetInstance();
+  for (const content_settings::ContentSettingsInfo* info :
+       *content_settings_registry) {
+    ContentSettingsType type = info->website_settings_info()->type();
+
+    // If the permission can not be tracked, then also can not be revoked.
+    if (!content_settings::CanTrackLastVisit(type)) {
+      continue;
+    }
+
+    // If the permission can not set to ALLOW, then also can not be revoked.
+    if (!content_settings_registry->Get(type)->IsSettingValid(
+            ContentSetting::CONTENT_SETTING_ALLOW)) {
+      continue;
+    }
+
+    // Add the permission to revoked permission list.
+    auto dict = base::Value::Dict().Set(
+        permissions::kRevokedKey,
+        base::Value::List().Append(static_cast<int32_t>(type)));
+    hcsm()->SetWebsiteSettingDefaultScope(
+        GURL(kUnusedTestSite), GURL(kUnusedTestSite),
+        ContentSettingsType::REVOKED_UNUSED_SITE_PERMISSIONS,
+        base::Value(dict.Clone()));
+
+    // Unless the permission in no_name_types, it should be shown on the UI.
+    const auto& revoked_permissions =
+        handler()->PopulateUnusedSitePermissionsData();
+    bool is_no_name_type =
+        (std::find(no_name_types.begin(), no_name_types.end(), type) !=
+         no_name_types.end());
+    if (is_no_name_type) {
+      EXPECT_EQ(revoked_permissions.size(), 0U);
+    } else {
+      EXPECT_EQ(revoked_permissions.size(), 1U);
+    }
+  }
+}

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

@@ -499,12 +499,18 @@ ContentSettingsType ContentSettingsTypeFromGroupName(base::StringPiece name) {
 }
 
 base::StringPiece ContentSettingsTypeToGroupName(ContentSettingsType type) {
-  for (size_t i = 0; i < std::size(kContentSettingsTypeGroupNames); ++i) {
-    if (type == kContentSettingsTypeGroupNames[i].type) {
-      const char* name = kContentSettingsTypeGroupNames[i].name;
-      if (name)
-        return name;
-      break;
+  for (const auto& entry : kContentSettingsTypeGroupNames) {
+    if (type == entry.type) {
+      // Content setting types that aren't represented in the settings UI
+      // will have `nullptr` as their `name`. Although they are valid content
+      // settings types, they don't have a readable name.
+      // TODO(crbug.com/1459305): Replace LOG with CHECK.
+      if (!entry.name) {
+        LOG(ERROR) << static_cast<int32_t>(type)
+                   << " does not have a readable name.";
+      }
+
+      return entry.name ? entry.name : base::StringPiece();
     }
   }
 

components/permissions/unused_site_permissions_service.cc

@@ -370,7 +370,9 @@ void UnusedSitePermissionsService::RevokeUnusedPermissions() {
       }
 
       DCHECK_EQ(entry.source.primary_pattern, primary_pattern);
-      DCHECK_EQ(entry.source.secondary_pattern, secondary_pattern);
+      DCHECK(entry.source.secondary_pattern ==
+                 ContentSettingsPattern::Wildcard() ||
+             entry.source.secondary_pattern == entry.source.primary_pattern);
 
       // Reset the permission to default if the site is visited before
       // threshold. Also, the secondary pattern should be wildcard.