diff --git a/chrome/browser/ui/webui/settings/site_settings_permissions_handler.cc b/chrome/browser/ui/webui/settings/site_settings_permissions_handler.cc index 97d764098bb68f..aa31ce5de2f107 100644 --- a/chrome/browser/ui/webui/settings/site_settings_permissions_handler.cc +++ b/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 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, + 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 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:: diff --git a/chrome/browser/ui/webui/settings/site_settings_permissions_handler.h b/chrome/browser/ui/webui/settings/site_settings_permissions_handler.h index 6f122dcc1f6316..d9b571112c94a0 100644 --- a/chrome/browser/ui/webui/settings/site_settings_permissions_handler.h +++ b/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, + content_settings::ContentSettingConstraints> + GetUnusedSitePermissionsFromDict( + const base::Value::Dict& unused_site_permissions); + const raw_ptr profile_; base::Clock* clock_; diff --git a/chrome/browser/ui/webui/settings/site_settings_permissions_handler_unittest.cc b/chrome/browser/ui/webui/settings/site_settings_permissions_handler_unittest.cc index f0a1ebff08286a..3fbd56e3dfa727 100644 --- a/chrome/browser/ui/webui/settings/site_settings_permissions_handler_unittest.cc +++ b/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, diff --git a/components/permissions/unused_site_permissions_service.cc b/components/permissions/unused_site_permissions_service.cc index c57a53305d5548..0bdcefc9527acd 100644 --- a/components/permissions/unused_site_permissions_service.cc +++ b/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 permissions, + const absl::optional + 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 revoked_permissions; + std::set 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 permissions, + const std::set permissions, const absl::optional constraint, const url::Origin origin) { @@ -364,7 +379,7 @@ void UnusedSitePermissionsService::StorePermissionInRevokedPermissionSetting( } void UnusedSitePermissionsService::StorePermissionInRevokedPermissionSetting( - const std::list permissions, + const std::set permissions, const absl::optional constraint, const ContentSettingsPattern& primary_pattern, diff --git a/components/permissions/unused_site_permissions_service.h b/components/permissions/unused_site_permissions_service.h index dadd2a45b34fd9..ec5a6917faeed4 100644 --- a/components/permissions/unused_site_permissions_service.h +++ b/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 permissions, + const absl::optional + 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 permissions, + const std::set permissions, const absl::optional constraint, const url::Origin origin); @@ -123,7 +131,7 @@ class UnusedSitePermissionsService // Stores revoked permissions data on HCSM. void StorePermissionInRevokedPermissionSetting( - const std::list permissions, + const std::set permissions, const absl::optional constraint, const ContentSettingsPattern& primary_pattern, diff --git a/components/permissions/unused_site_permissions_service_unittest.cc b/components/permissions/unused_site_permissions_service_unittest.cc index 85ec54e51aa67a..d89b16ea57bda6 100644 --- a/components/permissions/unused_site_permissions_service_unittest.cc +++ b/components/permissions/unused_site_permissions_service_unittest.cc @@ -372,6 +372,89 @@ TEST_F(UnusedSitePermissionsServiceTest, RegrantPermissionsForOrigin) { // Check if the permissions of url1 is regranted. EXPECT_EQ(ContentSetting::CONTENT_SETTING_ALLOW, hcsm()->GetContentSetting(GURL(url1), GURL(url1), type)); + + // Undoing the changes should add url1 back to the list of revoked permissions + // and reset its permissions. + service()->UndoRegrantPermissionsForOrigin({type}, absl::nullopt, + url::Origin::Create(GURL(url1))); + + hcsm()->GetSettingsForOneType( + ContentSettingsType::REVOKED_UNUSED_SITE_PERMISSIONS, + &revoked_permissions_list); + EXPECT_EQ(2U, revoked_permissions_list.size()); + EXPECT_EQ(ContentSetting::CONTENT_SETTING_ASK, + hcsm()->GetContentSetting(GURL(url1), GURL(url1), type)); +} + +TEST_F(UnusedSitePermissionsServiceTest, RegrantPreventsAutorevoke) { + base::test::ScopedFeatureList scoped_feature; + scoped_feature.InitAndEnableFeature( + content_settings::features::kSafetyCheckUnusedSitePermissions); + + const GURL url1 = GURL("https://example1.com:443"); + const content_settings::ContentSettingConstraints constraint{ + .track_last_visit_for_autoexpiration = true}; + + hcsm()->SetContentSettingDefaultScope( + url1, url1, ContentSettingsType::GEOLOCATION, + ContentSetting::CONTENT_SETTING_ALLOW, constraint); + EXPECT_EQ(GetRevokedUnusedPermissions(hcsm()).size(), 0u); + + // Travel 70 days through time so that the granted permission is revoked. + clock()->Advance(base::Days(70)); + service()->UpdateUnusedPermissionsForTesting(); + EXPECT_EQ(GetRevokedUnusedPermissions(hcsm()).size(), 1u); + + // After regranting permissions they are not revoked again even after >60 days + // pass. + service()->RegrantPermissionsForOrigin(url::Origin::Create(url1)); + EXPECT_EQ(GetRevokedUnusedPermissions(hcsm()).size(), 0u); + + clock()->Advance(base::Days(70)); + service()->UpdateUnusedPermissionsForTesting(); + EXPECT_EQ(GetRevokedUnusedPermissions(hcsm()).size(), 0u); +} + +TEST_F(UnusedSitePermissionsServiceTest, UndoRegrantPermissionsForOrigin) { + base::test::ScopedFeatureList scoped_feature; + scoped_feature.InitAndEnableFeature( + content_settings::features::kSafetyCheckUnusedSitePermissions); + + const GURL url1 = GURL("https://example1.com:443"); + const ContentSettingsType type = ContentSettingsType::GEOLOCATION; + const content_settings::ContentSettingConstraints constraint{ + .track_last_visit_for_autoexpiration = true}; + + hcsm()->SetContentSettingDefaultScope( + url1, url1, type, ContentSetting::CONTENT_SETTING_ALLOW, constraint); + EXPECT_EQ(GetRevokedUnusedPermissions(hcsm()).size(), 0u); + + // Travel 70 days through time so that the granted permission is revoked. + clock()->Advance(base::Days(70)); + service()->UpdateUnusedPermissionsForTesting(); + EXPECT_EQ(GetRevokedUnusedPermissions(hcsm()).size(), 1u); + const ContentSettingPatternSource revoked_permission = + GetRevokedUnusedPermissions(hcsm())[0]; + + // Permission remains revoked after regrant and undo. + content_settings::ContentSettingConstraints expiration_constraint = { + .expiration = revoked_permission.metadata.expiration}; + service()->RegrantPermissionsForOrigin(url::Origin::Create(url1)); + service()->UndoRegrantPermissionsForOrigin({type}, expiration_constraint, + url::Origin::Create(url1)); + EXPECT_EQ(GetRevokedUnusedPermissions(hcsm()).size(), 1u); + + // Revoked permission is cleaned up after >30 days. + clock()->Advance(base::Days(40)); + service()->UpdateUnusedPermissionsForTesting(); + EXPECT_EQ(GetRevokedUnusedPermissions(hcsm()).size(), 0u); + + // If that permission is granted again, it will still be autorevoked. + hcsm()->SetContentSettingDefaultScope( + url1, url1, type, ContentSetting::CONTENT_SETTING_ALLOW, constraint); + clock()->Advance(base::Days(70)); + service()->UpdateUnusedPermissionsForTesting(); + EXPECT_EQ(GetRevokedUnusedPermissions(hcsm()).size(), 1u); } TEST_F(UnusedSitePermissionsServiceTest, NotRevokeNotificationPermission) {