Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add origin-based permission lifetime support #8378

Merged
merged 9 commits into from
Apr 13, 2021
Merged

Add origin-based permission lifetime support #8378

Show file tree
Hide file tree
Changes from 8 commits
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
dbcb6bd
Support domains as PermissionExpiration key.
goodov Mar 19, 2021
03074aa
Add ephemeral TLD destroy observer.
goodov Mar 24, 2021
af8a171
Fix linter issues.
goodov Apr 2, 2021
1d36061
Rename permission origin lifetime monitor.
goodov Apr 2, 2021
ea10453
Add comments.
goodov Apr 2, 2021
ae5e8a5
Move permission origin lifetime monitor to components.
goodov Apr 5, 2021
1a0dc91
Add @akhoroshilov to permissions codeowners.
goodov Apr 9, 2021
076f42b
Pass some args as const ref.
goodov Apr 9, 2021
58370b1
Narrow down CODEOWNERS to lifetime/expiration files.
goodov Apr 13, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 4 additions & 0 deletions .github/CODEOWNERS
View file
Open in desktop
Validating CODEOWNERS rules …
Original file line number Diff line number Diff line change
Expand Up @@ -69,6 +69,10 @@ components/weekly_storage/ @iefremov
# Perf predictor
components/brave_perf_predictor/ @iefremov

# Permissions
browser/permissions/ @akhoroshilov
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we really need to avoid having entire components blocked on one person, can you restrict this to just critical parts of the code and/or add more people?

components/permissions/ @akhoroshilov

# Java patching
build/android/bytecode/ @samartnik
patches/*.java.patch @samartnik
Expand Down
2 changes: 1 addition & 1 deletion browser/ephemeral_storage/ephemeral_storage_tab_helper.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -86,7 +86,7 @@ void EphemeralStorageTabHelper::ReadyToCommitNavigation(
}

void EphemeralStorageTabHelper::CreateEphemeralStorageAreasForDomainAndURL(
std::string new_domain,
const std::string& new_domain,
const GURL& new_url) {
if (new_url.is_empty())
return;
Expand Down
2 changes: 1 addition & 1 deletion browser/ephemeral_storage/ephemeral_storage_tab_helper.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -40,7 +40,7 @@ class EphemeralStorageTabHelper
content::NavigationHandle* navigation_handle) override;

private:
void CreateEphemeralStorageAreasForDomainAndURL(std::string new_domain,
void CreateEphemeralStorageAreasForDomainAndURL(const std::string& new_domain,
const GURL& new_url);

friend class content::WebContentsUserData<EphemeralStorageTabHelper>;
Expand Down
195 changes: 189 additions & 6 deletions browser/permissions/permission_lifetime_manager_browsertest.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -27,13 +27,15 @@
#include "chrome/test/base/ui_test_utils.h"
#include "components/content_settings/core/browser/host_content_settings_map.h"
#include "components/content_settings/core/common/content_settings_types.h"
#include "components/network_session_configurator/common/network_switches.h"
goodov marked this conversation as resolved.
Show resolved Hide resolved
#include "components/permissions/features.h"
#include "components/permissions/request_type.h"
#include "components/prefs/pref_service.h"
#include "content/public/browser/render_frame_host.h"
#include "content/public/browser/web_contents.h"
#include "content/public/test/browser_test.h"
#include "content/public/test/browser_test_utils.h"
#include "net/base/features.h"
#include "net/dns/mock_host_resolver.h"
#include "net/test/embedded_test_server/embedded_test_server.h"
#include "url/gurl.h"
Expand All @@ -49,22 +51,31 @@ const char kPreTestDataFileName[] = "pre_test_data";

class PermissionLifetimeManagerBrowserTest : public InProcessBrowserTest {
public:
PermissionLifetimeManagerBrowserTest() {
PermissionLifetimeManagerBrowserTest()
: https_server_(net::EmbeddedTestServer::TYPE_HTTPS) {
scoped_feature_list_.InitAndEnableFeature(features::kPermissionLifetime);
}

~PermissionLifetimeManagerBrowserTest() override = default;

void SetUpCommandLine(base::CommandLine* command_line) override {
InProcessBrowserTest::SetUpCommandLine(command_line);
command_line->AppendSwitch(switches::kIgnoreCertificateErrors);
}

void SetUpOnMainThread() override {
PermissionRequestManager* manager = GetPermissionRequestManager();
prompt_factory_.reset(new MockPermissionLifetimePromptFactory(manager));

host_resolver()->AddRule("*", "127.0.0.1");
ASSERT_TRUE(embedded_test_server()->Start());
https_server()->ServeFilesFromDirectory(GetChromeTestDataDir());
ASSERT_TRUE(https_server()->Start());
}

void TearDownOnMainThread() override { prompt_factory_.reset(); }

net::EmbeddedTestServer* https_server() { return &https_server_; }

PermissionRequestManager* GetPermissionRequestManager() {
return PermissionRequestManager::FromWebContents(
browser()->tab_strip_model()->GetActiveWebContents());
Expand Down Expand Up @@ -120,12 +131,13 @@ class PermissionLifetimeManagerBrowserTest : public InProcessBrowserTest {

protected:
base::test::ScopedFeatureList scoped_feature_list_;
net::test_server::EmbeddedTestServer https_server_;
std::unique_ptr<MockPermissionLifetimePromptFactory> prompt_factory_;
base::Value pre_test_data_{base::Value::Type::DICTIONARY};
};

IN_PROC_BROWSER_TEST_F(PermissionLifetimeManagerBrowserTest, ExpirationSmoke) {
const GURL& url = embedded_test_server()->GetURL("/empty.html");
const GURL& url = https_server()->GetURL("/empty.html");
ui_test_utils::NavigateToURL(browser(), url);
prompt_factory_->set_response_type(
PermissionRequestManager::AutoResponseType::ACCEPT_ALL);
Expand Down Expand Up @@ -169,7 +181,7 @@ IN_PROC_BROWSER_TEST_F(PermissionLifetimeManagerBrowserTest, ExpirationSmoke) {

IN_PROC_BROWSER_TEST_F(PermissionLifetimeManagerBrowserTest,
PRE_PermissionExpiredAfterRestart) {
const GURL& url = embedded_test_server()->GetURL("/empty.html");
const GURL& url = https_server()->GetURL("/empty.html");
ui_test_utils::NavigateToURL(browser(), url);
prompt_factory_->set_response_type(
PermissionRequestManager::AutoResponseType::ACCEPT_ALL);
Expand Down Expand Up @@ -227,7 +239,7 @@ IN_PROC_BROWSER_TEST_F(PermissionLifetimeManagerBrowserTest,

IN_PROC_BROWSER_TEST_F(PermissionLifetimeManagerBrowserTest,
ExpirationRemovedAfterManualReset) {
const GURL& url = embedded_test_server()->GetURL("/empty.html");
const GURL& url = https_server()->GetURL("/empty.html");
ui_test_utils::NavigateToURL(browser(), url);
prompt_factory_->set_response_type(
PermissionRequestManager::AutoResponseType::ACCEPT_ALL);
Expand Down Expand Up @@ -257,4 +269,175 @@ IN_PROC_BROWSER_TEST_F(PermissionLifetimeManagerBrowserTest,
EXPECT_TRUE(GetExpirationsPrefValue()->DictEmpty());
}

} // namespace permissions
class PermissionLifetimeManagerWithOriginMonitorBrowserTest
: public PermissionLifetimeManagerBrowserTest {
public:
PermissionLifetimeManagerWithOriginMonitorBrowserTest() {
scoped_feature_list_.InitAndEnableFeature(
net::features::kBraveEphemeralStorage);
}

protected:
base::test::ScopedFeatureList scoped_feature_list_;
};

IN_PROC_BROWSER_TEST_F(PermissionLifetimeManagerWithOriginMonitorBrowserTest,
DomainPermissionReset) {
const GURL& url = https_server()->GetURL("host.com", "/empty.html");
ui_test_utils::NavigateToURL(browser(), url);
prompt_factory_->set_response_type(
PermissionRequestManager::AutoResponseType::ACCEPT_ALL);

EXPECT_CALL(*prompt_factory_, OnPermissionPromptCreated(_))
.WillOnce(testing::Invoke([](MockPermissionLifetimePrompt* prompt) {
prompt->delegate()->Requests()[0]->SetLifetime(base::TimeDelta());
}));
content::ExecuteScriptAsync(
GetActiveMainFrame(),
"navigator.geolocation.getCurrentPosition(function(){});");
prompt_factory_->WaitForPermissionBubble();

EXPECT_EQ(1, prompt_factory_->show_count());
EXPECT_FALSE(permission_lifetime_timer().IsRunning());
EXPECT_FALSE(GetExpirationsPrefValue()->DictEmpty());

EXPECT_EQ(host_content_settings_map()->GetContentSetting(
url, url, ContentSettingsType::GEOLOCATION),
ContentSetting::CONTENT_SETTING_ALLOW);

// Navigate to another domain. It should reset the permission.
const GURL& other_url =
https_server()->GetURL("other_host.com", "/empty.html");
ui_test_utils::NavigateToURL(browser(), other_url);
EXPECT_EQ(host_content_settings_map()->GetContentSetting(
url, url, ContentSettingsType::GEOLOCATION),
ContentSetting::CONTENT_SETTING_ASK);
EXPECT_TRUE(GetExpirationsPrefValue()->DictEmpty());
}

IN_PROC_BROWSER_TEST_F(PermissionLifetimeManagerWithOriginMonitorBrowserTest,
FriendlyDomainPermissionKept) {
const GURL& url = https_server()->GetURL("example.com", "/empty.html");
ui_test_utils::NavigateToURL(browser(), url);
prompt_factory_->set_response_type(
PermissionRequestManager::AutoResponseType::ACCEPT_ALL);

EXPECT_CALL(*prompt_factory_, OnPermissionPromptCreated(_))
.WillOnce(testing::Invoke([](MockPermissionLifetimePrompt* prompt) {
prompt->delegate()->Requests()[0]->SetLifetime(base::TimeDelta());
}));
content::ExecuteScriptAsync(
GetActiveMainFrame(),
"navigator.geolocation.getCurrentPosition(function(){});");
prompt_factory_->WaitForPermissionBubble();

EXPECT_EQ(1, prompt_factory_->show_count());
EXPECT_FALSE(permission_lifetime_timer().IsRunning());
EXPECT_FALSE(GetExpirationsPrefValue()->DictEmpty());

EXPECT_EQ(host_content_settings_map()->GetContentSetting(
url, url, ContentSettingsType::GEOLOCATION),
ContentSetting::CONTENT_SETTING_ALLOW);

// Navigate to a subdomain, permission should be kept.
const GURL& sub_url =
https_server()->GetURL("sub.example.com", "/empty.html");
ui_test_utils::NavigateToURL(browser(), sub_url);
EXPECT_EQ(host_content_settings_map()->GetContentSetting(
url, url, ContentSettingsType::GEOLOCATION),
ContentSetting::CONTENT_SETTING_ALLOW);
EXPECT_FALSE(GetExpirationsPrefValue()->DictEmpty());

// Navigate to another domain. It should reset the permission.
const GURL& other_url =
https_server()->GetURL("other_host.com", "/empty.html");
ui_test_utils::NavigateToURL(browser(), other_url);
EXPECT_EQ(host_content_settings_map()->GetContentSetting(
url, url, ContentSettingsType::GEOLOCATION),
ContentSetting::CONTENT_SETTING_ASK);
EXPECT_TRUE(GetExpirationsPrefValue()->DictEmpty());
}

IN_PROC_BROWSER_TEST_F(PermissionLifetimeManagerWithOriginMonitorBrowserTest,
PublicSuffixListDomainPermissionReset) {
const GURL& url = https_server()->GetURL("user.github.io", "/empty.html");
ui_test_utils::NavigateToURL(browser(), url);
prompt_factory_->set_response_type(
PermissionRequestManager::AutoResponseType::ACCEPT_ALL);

EXPECT_CALL(*prompt_factory_, OnPermissionPromptCreated(_))
.WillOnce(testing::Invoke([](MockPermissionLifetimePrompt* prompt) {
prompt->delegate()->Requests()[0]->SetLifetime(base::TimeDelta());
}));
content::ExecuteScriptAsync(
GetActiveMainFrame(),
"navigator.geolocation.getCurrentPosition(function(){});");
prompt_factory_->WaitForPermissionBubble();

EXPECT_EQ(1, prompt_factory_->show_count());
EXPECT_FALSE(permission_lifetime_timer().IsRunning());
EXPECT_FALSE(GetExpirationsPrefValue()->DictEmpty());

EXPECT_EQ(host_content_settings_map()->GetContentSetting(
url, url, ContentSettingsType::GEOLOCATION),
ContentSetting::CONTENT_SETTING_ALLOW);

// Navigate to a subdomain, permission should be kept.
const GURL& sub_url =
https_server()->GetURL("sub.user.github.io", "/empty.html");
ui_test_utils::NavigateToURL(browser(), sub_url);
EXPECT_EQ(host_content_settings_map()->GetContentSetting(
url, url, ContentSettingsType::GEOLOCATION),
ContentSetting::CONTENT_SETTING_ALLOW);
EXPECT_FALSE(GetExpirationsPrefValue()->DictEmpty());

// Navigate to another domain in PSL. It should reset the permission.
const GURL& other_url =
https_server()->GetURL("user2.github.io", "/empty.html");
ui_test_utils::NavigateToURL(browser(), other_url);
EXPECT_EQ(host_content_settings_map()->GetContentSetting(
url, url, ContentSettingsType::GEOLOCATION),
ContentSetting::CONTENT_SETTING_ASK);
EXPECT_TRUE(GetExpirationsPrefValue()->DictEmpty());
}

IN_PROC_BROWSER_TEST_F(PermissionLifetimeManagerWithOriginMonitorBrowserTest,
PRE_DomainPermissionResetAfterRestart) {
const GURL& url = https_server()->GetURL("example.com", "/empty.html");
ui_test_utils::NavigateToURL(browser(), url);
prompt_factory_->set_response_type(
PermissionRequestManager::AutoResponseType::ACCEPT_ALL);

EXPECT_CALL(*prompt_factory_, OnPermissionPromptCreated(_))
.WillOnce(testing::Invoke([](MockPermissionLifetimePrompt* prompt) {
prompt->delegate()->Requests()[0]->SetLifetime(base::TimeDelta());
}));
content::ExecuteScriptAsync(
GetActiveMainFrame(),
"navigator.geolocation.getCurrentPosition(function(){});");
prompt_factory_->WaitForPermissionBubble();

EXPECT_EQ(1, prompt_factory_->show_count());
EXPECT_FALSE(permission_lifetime_timer().IsRunning());
EXPECT_FALSE(GetExpirationsPrefValue()->DictEmpty());

EXPECT_EQ(host_content_settings_map()->GetContentSetting(
url, url, ContentSettingsType::GEOLOCATION),
ContentSetting::CONTENT_SETTING_ALLOW);

pre_test_data_.SetStringKey("url", url.spec());
WritePreTestData();
}

IN_PROC_BROWSER_TEST_F(PermissionLifetimeManagerWithOriginMonitorBrowserTest,
DomainPermissionResetAfterRestart) {
ReadPreTestData();
const GURL url(*pre_test_data_.FindStringKey("url"));

EXPECT_EQ(host_content_settings_map()->GetContentSetting(
url, url, ContentSettingsType::GEOLOCATION),
ContentSetting::CONTENT_SETTING_ASK);
EXPECT_TRUE(GetExpirationsPrefValue()->DictEmpty());
}

} // namespace permissions
15 changes: 14 additions & 1 deletion browser/permissions/permission_lifetime_manager_factory.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,13 +5,18 @@

#include "brave/browser/permissions/permission_lifetime_manager_factory.h"

#include <memory>
#include <utility>

#include "brave/components/permissions/permission_lifetime_manager.h"
#include "brave/components/permissions/permission_origin_lifetime_monitor_impl.h"
#include "chrome/browser/content_settings/host_content_settings_map_factory.h"
#include "chrome/browser/permissions/permission_manager_factory.h"
#include "chrome/browser/profiles/incognito_helpers.h"
#include "chrome/browser/profiles/profile.h"
#include "components/keyed_service/content/browser_context_dependency_manager.h"
#include "components/permissions/features.h"
#include "net/base/features.h"

// static
permissions::PermissionLifetimeManager*
Expand Down Expand Up @@ -43,10 +48,18 @@ KeyedService* PermissionLifetimeManagerFactory::BuildServiceInstanceFor(
permissions::features::kPermissionLifetime)) {
return nullptr;
}
std::unique_ptr<permissions::PermissionOriginLifetimeMonitor>
permission_origin_lifetime_monitor;
if (base::FeatureList::IsEnabled(net::features::kBraveEphemeralStorage)) {
permission_origin_lifetime_monitor =
std::make_unique<permissions::PermissionOriginLifetimeMonitorImpl>(
context);
}
auto* profile = Profile::FromBrowserContext(context);
return new permissions::PermissionLifetimeManager(
HostContentSettingsMapFactory::GetForProfile(context),
profile->IsOffTheRecord() ? nullptr : profile->GetPrefs());
profile->IsOffTheRecord() ? nullptr : profile->GetPrefs(),
std::move(permission_origin_lifetime_monitor));
}

bool PermissionLifetimeManagerFactory::ServiceIsCreatedWithBrowserContext()
Expand Down
Loading