From 80d1dd0f57342325f321b1809dc6e3a3f59d4aa0 Mon Sep 17 00:00:00 2001 From: Eduard Hez Date: Mon, 30 Jan 2023 11:54:11 +0000 Subject: [PATCH] [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 Reviewed-by: Illia Klimov Commit-Queue: Eduard Hez Reviewed-by: Rainhard Findling Cr-Commit-Position: refs/heads/main@{#1098533} --- .../site_settings_permissions_handler.cc | 92 +++++++++++++------ .../site_settings_permissions_handler.h | 23 ++++- ...e_settings_permissions_handler_unittest.cc | 24 ++++- .../unused_site_permissions_service.cc | 23 ++++- .../unused_site_permissions_service.h | 12 ++- ...nused_site_permissions_service_unittest.cc | 83 +++++++++++++++++ 6 files changed, 217 insertions(+), 40 deletions(-) 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) {