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

Jump to bottom

Implement Global Privacy Control #6743

Merged
merged 3 commits into from
Oct 6, 2020
Merged

Implement Global Privacy Control #6743

merged 3 commits into from
Oct 6, 2020

Conversation

antonok-edm
Copy link
Collaborator

@antonok-edm antonok-edm commented Oct 1, 2020 •
edited
Loading

Resolves brave/brave-browser#11917

Submitter Checklist:

Test Plan:

This feature is gated in Nightly, so these tests will not work in Release or Beta.

Sec-GPC header

With the Network tab of the Developer Tools open, navigate to an HTTPS webpage of your choice. Choose a few requests at random and verify that the "Sec-GPC" header is set and has a value of "1".

globalPrivacyControl API

Open the Console in the Developer Tools, and verify that the following Javascript evaluates to true:

navigator.globalPrivacyControl

Reviewer Checklist:

  • New files have MPL-2.0 license header.
  • Request a security/privacy review as needed.
  • Adequate test coverage exists to prevent regressions
  • Verify test plan is specified in PR before merging to source

After-merge Checklist:

  • The associated issue milestone is set to the smallest version that the
    changes has landed on.
  • All relevant documentation has been updated.

@antonok-edm antonok-edm self-assigned this Oct 1, 2020
@antonok-edm antonok-edm force-pushed the global-privacy-control branch from 28786c4 to 979f35b Compare October 1, 2020 16:35
@antonok-edm antonok-edm marked this pull request as ready for review October 1, 2020 17:38
@antonok-edm antonok-edm requested a review from bridiver as a code owner October 1, 2020 17:38
@antonok-edm antonok-edm force-pushed the global-privacy-control branch from e1ec07f to 3231ede Compare October 1, 2020 18:33
bridiver
bridiver reviewed Oct 1, 2020
View reviewed changes
third_party/blink/renderer/modules/global_privacy_control/navigator_global_privacy_control.h Outdated
static const char kSupplementName[];

static NavigatorGlobalPrivacyControl& From(Navigator&);
static bool globalPrivacyControl(blink::Navigator&) { return true; }
Copy link
Collaborator

Choose a reason for hiding this comment

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

avoid inline methods

@antonok-edm antonok-edm force-pushed the global-privacy-control branch from 3231ede to 73b3f84 Compare October 1, 2020 20:07
@antonok-edm antonok-edm requested a review from iefremov as a code owner October 1, 2020 20:07
@antonok-edm antonok-edm force-pushed the global-privacy-control branch from 73b3f84 to c88878f Compare October 1, 2020 20:17
@antonok-edm antonok-edm requested a review from bridiver October 1, 2020 20:18
bridiver
bridiver reviewed Oct 1, 2020
View reviewed changes
browser/net/brave_request_handler.cc Outdated
@@ -100,6 +103,12 @@ void BraveRequestHandler::SetupCallbacks() {
base::Bind(brave::OnBeforeStartTransaction_SiteHacksWork);
before_start_transaction_callbacks_.push_back(start_transaction_callback);

if (base::FeatureList::IsEnabled(features::kGlobalPrivacyControl)) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

it's better to encapsulate feature checks inside the relevant code, so rather than conditionally including the helper, you should always include it and conditionally add the header inside it

bridiver
bridiver reviewed Oct 1, 2020
View reviewed changes
common/brave_features.cc Outdated
@@ -19,4 +19,7 @@ const base::Feature kBraveRewards{"BraveRewards",
#endif
#endif // defined(OS_ANDROID)

const base::Feature kGlobalPrivacyControl{"GlobalPrivacyControl",
base::FEATURE_ENABLED_BY_DEFAULT};
Copy link
Collaborator

Choose a reason for hiding this comment

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

we want this enabled by default in nightly, right? You can check the release channel in BraveMainDelegate::BasicStartupComplete and conditionally add it to the list of enabled features

bridiver
bridiver reviewed Oct 1, 2020
View reviewed changes
third_party/blink/renderer/modules/global_privacy_control/navigator_global_privacy_control.h
static const char kSupplementName[];

static NavigatorGlobalPrivacyControl& From(Navigator&);
static bool globalPrivacyControl(blink::Navigator&);
Copy link
Collaborator

Choose a reason for hiding this comment

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

we should have tests for the api and also for the header

@antonok-edm antonok-edm force-pushed the global-privacy-control branch 4 times, most recently from 63840c0 to 634bea3 Compare October 2, 2020 00:29
iefremov
iefremov reviewed Oct 2, 2020
View reviewed changes
browser/net/global_privacy_control_network_delegate_helper.cc
const ResponseCallback& next_callback,
std::shared_ptr<BraveRequestInfo> ctx) {
if (base::FeatureList::IsEnabled(features::kGlobalPrivacyControl)) {
headers->SetHeader(kSecGpcHeader, "1");
Copy link
Contributor

Choose a reason for hiding this comment

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

do we need it for all requests, or just for navigations/downloads?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Talked to @pes10k about this as it's not currently described anywhere - he will try to get that clarified in the spec but there shouldn't be any harm in doing it this way.

Copy link
Contributor

Choose a reason for hiding this comment

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

just heard back, its intended to be on every request

iefremov
iefremov reviewed Oct 2, 2020
View reviewed changes
common/brave_features.cc Outdated
@@ -19,4 +19,7 @@ const base::Feature kBraveRewards{"BraveRewards",
#endif
#endif // defined(OS_ANDROID)

const base::Feature kGlobalPrivacyControl{"GlobalPrivacyControl",
base::FEATURE_DISABLED_BY_DEFAULT};
Copy link
Contributor

Choose a reason for hiding this comment

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

clang-format

iefremov
iefremov reviewed Oct 2, 2020
View reviewed changes
common/brave_features.cc
@@ -19,4 +19,7 @@ const base::Feature kBraveRewards{"BraveRewards",
#endif
#endif // defined(OS_ANDROID)

const base::Feature kGlobalPrivacyControl{"GlobalPrivacyControl",
Copy link
Contributor

Choose a reason for hiding this comment

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

can we add a comment, link to the spec?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Spec is currently not public, so there isn't a stable URL for it at the moment unfortunately

Copy link
Member

Choose a reason for hiding this comment

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

Perhaps just a comment or link to a wiki on brave-browser

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I added a comment from the spec

iefremov
iefremov reviewed Oct 2, 2020
View reviewed changes
third_party/blink/renderer/modules/global_privacy_control/navigator_global_privacy_control.cc
namespace blink {

NavigatorGlobalPrivacyControl::
NavigatorGlobalPrivacyControl(Navigator& navigator) // NOLINT
Copy link
Contributor

Choose a reason for hiding this comment

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

just curious - why this is NOLINT

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Lint asks for const Navigator& but then it doesn't match the IDL generated header.

iefremov
iefremov reviewed Oct 2, 2020
View reviewed changes
browser/net/global_privacy_control_network_delegate_helper_browsertest.cc Outdated

std::unique_ptr<net::test_server::HttpResponse> HandleRequest(
const net::test_server::HttpRequest& request) {
auto http_response =
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't think you really need to craft responses, the same can be achieved with RegisterRequestMonitor - e.g. see BraveContentSettingsAgentImplBrowserTest. Also this callback is called on IO thread, so acess to header_result_ should be guarded

iefremov
iefremov reviewed Oct 2, 2020
View reviewed changes
browser/net/global_privacy_control_network_delegate_helper_browsertest.cc Outdated
IN_PROC_BROWSER_TEST_F(GlobalPrivacyControlNetworkDelegateBrowserTest,
IncludesSecGPCHeader) {
const GURL target = https_server().GetURL("example.com", "/index.html");
ui_test_utils::UrlLoadObserver load_complete(
Copy link
Contributor

Choose a reason for hiding this comment

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

no need for observers, just ui_test_utils::NavigateToURL

iefremov
iefremov reviewed Oct 2, 2020
View reviewed changes
browser/net/global_privacy_control_network_delegate_helper_browsertest.cc Outdated
load_complete.Wait();

bool as_expected = false;
ASSERT_TRUE(ExecuteScriptAndExtractBool(
Copy link
Contributor

Choose a reason for hiding this comment

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

AFAIK this is deprecated in favor of EvalJs

iefremov
iefremov reviewed Oct 2, 2020
View reviewed changes
browser/net/global_privacy_control_network_delegate_helper_browsertest.cc Outdated
browser()->tab_strip_model()->GetActiveWebContents(),
"window.domAutomationController.send(navigator.globalPrivacyControl)",
&as_expected));
EXPECT_EQ(as_expected, true);
Copy link
Contributor

Choose a reason for hiding this comment

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

EXPECT_TRUE

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Can't use EXPECT_TRUE with EvalJs, so I'm leaving EXPECT_EQ here

iefremov
iefremov reviewed Oct 2, 2020
View reviewed changes
browser/net/global_privacy_control_network_delegate_helper_browsertest.cc
EXPECT_EQ(as_expected, true);
}

class DisabledGlobalPrivacyControlNetworkDelegateBrowserTest
Copy link
Contributor

Choose a reason for hiding this comment

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

mb just disable feature in the test itself?

@antonok-edm antonok-edm force-pushed the global-privacy-control branch from 634bea3 to 4d179b3 Compare October 2, 2020 15:29
@antonok-edm antonok-edm force-pushed the global-privacy-control branch from 4d179b3 to 4eabf46 Compare October 2, 2020 15:47
iefremov
iefremov reviewed Oct 2, 2020
View reviewed changes
browser/net/global_privacy_control_network_delegate_helper_browsertest.cc Outdated
* License, v. 2.0. If a copy of the MPL was not distributed with this file,
* You can obtain one at https://mozilla.org/MPL/2.0/. */

#include <map>
Copy link
Contributor

Choose a reason for hiding this comment

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

not needed now

iefremov
iefremov reviewed Oct 2, 2020
View reviewed changes
browser/net/global_privacy_control_network_delegate_helper_browsertest.cc Outdated

if (request.headers.find("Sec-GPC") == request.headers.end()) {
header_result_ = GPCHeaderResult::NO_HEADER;
} else if (request.headers.at("Sec-GPC") != "1") {
Copy link
Contributor

Choose a reason for hiding this comment

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

we probably should use kSecGpcHeader from network_constants.h

iefremov
iefremov reviewed Oct 2, 2020
View reviewed changes
browser/net/global_privacy_control_network_delegate_helper_browsertest.cc Outdated
void HandleRequest(const net::test_server::HttpRequest& request) {
base::AutoLock auto_lock(header_result_lock_);

if (request.headers.find("Sec-GPC") == request.headers.end()) {
Copy link
Contributor

Choose a reason for hiding this comment

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

super small nit:

auto it = request.headers.find("Sec-GPC");
if (it == request.headers.end()) { 
... }
else if (it->second != "1") {
... }
else {
... }

iefremov
iefremov reviewed Oct 2, 2020
View reviewed changes
browser/net/global_privacy_control_network_delegate_helper_browsertest.cc Outdated
#include "net/test/embedded_test_server/http_request.h"
#include "net/test/embedded_test_server/http_response.h"

enum GPCHeaderResult {
Copy link
Contributor

Choose a reason for hiding this comment

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

enum class, also I think preferable naming for constants is kOk, kNoRequest, etc

@antonok-edm antonok-edm force-pushed the global-privacy-control branch from 4eabf46 to 4e06a9f Compare October 2, 2020 21:45
@bbondy bbondy removed their request for review October 5, 2020 13:56
bbondy
bbondy reviewed Oct 5, 2020
View reviewed changes
third_party/blink/renderer/modules/global_privacy_control/navigator_global_privacy_control.cc

bool NavigatorGlobalPrivacyControl::
globalPrivacyControl(blink::Navigator& navigator) { // NOLINT
return true;
Copy link
Member

Choose a reason for hiding this comment

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

I believe we want to check base::FeatureList::IsEnabled here

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@bbondy I discussed this with @iefremov and @bridiver in DMs - there isn't a way to gate the presence of the IDL-generated API by a feature flag. We've decided it's fine since this is purely a new API and shouldn't break webcompat anywhere. And if it has to exist, better to just always return true rather than false.

Copy link
Member

Choose a reason for hiding this comment

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

ok

@antonok-edm antonok-edm force-pushed the global-privacy-control branch from 4e06a9f to 798d7f4 Compare October 5, 2020 15:47
pes10k
pes10k approved these changes Oct 5, 2020
View reviewed changes
Copy link
Contributor

@pes10k pes10k left a comment

Choose a reason for hiding this comment

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

lgtm

@antonok-edm antonok-edm merged commit f24bb8e into master Oct 6, 2020
@antonok-edm antonok-edm deleted the global-privacy-control branch October 6, 2020 15:17
@emerick emerick mentioned this pull request Oct 6, 2020
Merged
33 tasks
@antonok-edm antonok-edm added this to the 1.17.x - Nightly milestone Oct 7, 2020
@LaurenWags LaurenWags mentioned this pull request Nov 4, 2020
Closed
@Panther7170 Panther7170 mentioned this pull request Aug 25, 2023
Closed
4 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@bbondy bbondy bbondy left review comments

@iefremov iefremov iefremov approved these changes

@pes10k pes10k pes10k approved these changes

@bridiver bridiver bridiver approved these changes

@mkarolin mkarolin Awaiting requested review from mkarolin

@yrliou yrliou Awaiting requested review from yrliou

Assignees

@antonok-edm antonok-edm

Labels
None yet
Projects
None yet
Milestone
1.17.x - Release #1
Development

Successfully merging this pull request may close these issues.

Implement Global Privacy Control (only in nightly)
5 participants
@antonok-edm @bridiver @pes10k @bbondy @iefremov