Fix 5869: Google OAuth should work by default #3784

jumde · 2019-10-23T16:28:27Z

Fix brave/brave-browser#5869

Submitter Checklist:

Test Plan:

Navigate to expensify.com
Verify that Cross-Site cookie blocking is enabled
Click login using Google
Login should succeed
Navigate to brave://settings/socialBlocking and disable Google login
Navigate to brave://settings/clearBrowserData and clear all data
Try logging in again, login should fail

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.

bridiver · 2019-10-30T00:02:06Z

components/brave_shields/browser/brave_shields_util.cc

@@ -205,6 +205,20 @@ ControlType GetCookieControlType(Profile* profile, const GURL& url) {
      HostContentSettingsMapFactory::GetForProfile(profile), url);
 }

+void SetCookieWhitelist(HostContentSettingsMap* map, ControlType type,


it's confusing to me that we have a method called SetCookieWhitelist that takes a ControlType. It seems to me that this is just an overloaded SetCookieControlType with a string pattern instead of a GURL, correct?

I'd say SetCookieControlTypeForPattern? Overloads are often confusing and also not encouraged by CC.

bridiver · 2019-10-30T00:03:07Z

components/brave_shields/browser/brave_shields_util.cc

+                                    CONTENT_SETTINGS_TYPE_PLUGINS, kCookies,
+                                    GetDefaultBlockFromControlType(type));
+
+  // Allow cookies for social media sites when used in first party context


why does this method care whether it's social media or not?

bridiver · 2019-10-30T00:03:40Z

components/brave_shields/browser/brave_shields_util.cc

+  map->SetContentSettingCustomScope(
+      ContentSettingsPattern::FromString(pattern),
+      ContentSettingsPattern::FromString("https://firstParty/*"),
+      CONTENT_SETTINGS_TYPE_PLUGINS, kCookies, CONTENT_SETTING_ALLOW);


why is this always ALLOW if we're passing a ControlType?

bridiver · 2019-10-30T00:05:33Z

components/brave_shields/browser/cookie_pref_service.cc

+    ControlType type = prefs->GetBoolean(kGoogleLoginControlType)
+                           ? ControlType::ALLOW
+                           : ControlType::BLOCK;
+    SetCookieWhitelist(map, type, kGoogleOAuthPattern);


I think it's problematic to set this as a brave cookie plugin type because it will always end up with an override one direction or the other which could potentially conflict with user overrides. It seems better to convert this directly to cookie settings in BravePrefProvider so we only add an override when it's enabled

it's especially problematic because of this https://github.com/brave/brave-core/pull/3784/files#diff-6c160ccdf43d9f23e468a95ec08b4d12R219 which adds an allow override no matter what the setting is

bridiver

as per comment we should not be adding any content settings when kGoogleLoginControlType is disabled and the current code just changes what the override is. We should convert it to non-persistent chrome cookie settings instead when we convert everything else in BravePrefProvider

bridiver · 2019-10-30T00:11:03Z

components/brave_shields/browser/cookie_pref_service.cc

@@ -21,6 +23,21 @@ namespace brave_shields {

 namespace {

+void SetCookieWhitelistForSocialMedia(HostContentSettingsMap* map,


this method makes more sense as SetCookieWhitelist because it will potentially add other pref based overrides, but ForSocialMedia seems irrelevant to the method and google auth isn't social media anyway. This is just setting a whitelist based on prefs

bridiver · 2019-10-30T00:13:01Z

please add tests for this including a test to ensure that no overrides are added when the pref is false

iefremov · 2019-10-30T11:14:18Z

components/brave_shields/browser/brave_shields_util.h

+
+void SetCookieWhitelist(HostContentSettingsMap* map,
+                        ControlType type,
+                        std::string pattern);


const std::string& or base::StringPiece

iefremov · 2019-11-04T10:16:43Z

browser/net/brave_network_delegate_browsertest.cc

+  }
+
+  void NavigateToPageWithFrame(const std::string& host) {
+      GURL main_url(https_server_.GetURL(host, "/iframe.html"));


pls correct spaces everywhere (or use clang-format)

iefremov · 2019-11-04T10:31:47Z

components/brave_shields/common/brave_shield_constants.h

@@ -17,6 +17,7 @@ const char kFingerprinting[] = "fingerprinting";
 const char kBraveShields[] = "braveShields";
 const char kReferrers[] = "referrers";
 const char kCookies[] = "cookies";
+const char kGoogleLogin[] = "google-login";


is it used anywhere?

iefremov · 2019-11-04T10:35:41Z

components/content_settings/core/browser/brave_content_settings_pref_provider.h

@@ -56,6 +56,8 @@ class BravePrefProvider : public PrefProvider,
                               const ContentSettingsPattern& secondary_pattern,
                               ContentSettingsType content_type,
                               const std::string& resource_identifier) override;
+  void OnCookiePrefsChanged(const std::string& pref);
+  void SetOAuthCookieException(ContentSettingsType type);


iefremov · 2019-11-04T10:51:55Z

browser/net/brave_network_delegate_browsertest.cc

+
+IN_PROC_BROWSER_TEST_F(BraveNetworkDelegateBrowserTest,
+                       IframeGoogleAccountsCookieBlocked) {
+  BlockGoogleOAuthCookies();


we need a test, showing that the new preference doesn't override user choice in per-site shields settings

I actually requested this in the last review and should verify your comment below

Added here: https://github.com/brave/brave-core/pull/3784/files#diff-a3b1f2bffde11cef192a185a65955508R350

iefremov · 2019-11-04T10:54:14Z

components/content_settings/core/browser/brave_content_settings_pref_provider.cc

@@ -254,6 +261,14 @@ void BravePrefProvider::UpdateCookieRules(ContentSettingsType content_type,
    }
  }

+  if (prefs_->GetBoolean(kGoogleLoginControlType)) {


not sure if this overrides user shields settings or not.
Anyway, this method becomes too fat, so i'd prefer to put it into a separate method (or at least add a comment).

it shouldn't override shields settings as long as it comes last, but it should have a test to verify

Test added here: https://github.com/brave/brave-core/pull/3784/files#diff-a3b1f2bffde11cef192a185a65955508R350

although actually we want it to override site shield settings because it should work the same whether the default is block 3p cookies or whether the default is something else and site shield settings are block 3p cookies. In the case where site settings are block all you won't be able to login to anything anyway

bridiver · 2019-11-04T16:27:19Z

components/content_settings/core/browser/brave_content_settings_pref_provider.cc

+
+void BravePrefProvider::OnCookiePrefsChanged(
+    const std::string& pref) {
+    OnCookieSettingsChanged(CONTENT_SETTINGS_TYPE_PLUGINS);


lint - 2 spaces

jumde · 2019-11-12T00:01:05Z

browser/net/brave_network_delegate_browsertest.cc

@@ -188,7 +212,7 @@ IN_PROC_BROWSER_TEST_F(BraveNetworkDelegateBrowserTest, DefaultCookiesBlocked) {
 IN_PROC_BROWSER_TEST_F(BraveNetworkDelegateBrowserTest,
                       PrefToggleBlockAllToBlockThirdParty) {
  DefaultBlockAllCookies();
-  BlockThirdPartyCookies();
+  BlockAllThirdPartyCookies();


Renamed so that it's consistent with other helper functions like DefaultBlockAllCookies

iefremov · 2019-11-12T23:31:05Z

components/content_settings/core/browser/brave_content_settings_pref_provider.cc

  rules.clear();

+  if (prefs_->GetBoolean(kGoogleLoginControlType)) {


I desperately desire a comment here, describing the whole situation. So an occasional stranger could have a little chance to understand why and what happens here :)

bridiver · 2019-11-13T16:51:49Z

browser/net/brave_network_delegate_browsertest.cc

+                                        server->GetURL("a.com", "/"));
+  }
+
+  void BlockThirdPartyCookies(const EmbeddedTestServer* server) {


I think this method name is confusing because it's not obvious in the tests that this is a site override and not the default. I think it's better to pass in the URL instead of the server

bridiver · 2019-11-13T16:51:54Z

browser/net/brave_network_delegate_browsertest.cc

@@ -67,21 +82,28 @@ class BraveNetworkDelegateBrowserTest : public InProcessBrowserTest {
        browser()->profile(), brave_shields::ControlType::ALLOW, GURL());
  }

-  void AllowCookies() {
+  void AllowCookies(const EmbeddedTestServer* server) {


same as below

bridiver · 2019-11-13T16:54:16Z

browser/net/brave_network_delegate_browsertest.cc

  }

-  void BlockCookies() {
+  void BlockCookies(const EmbeddedTestServer* server) {


bridiver · 2019-11-13T17:00:07Z

browser/net/brave_network_delegate_browsertest.cc

  void DefaultBlockAllCookies() {
    brave_shields::SetCookieControlType(
        browser()->profile(), brave_shields::ControlType::BLOCK, GURL());
  }

-  void BlockThirdPartyCookies() {
+  void BlockAllThirdPartyCookies() {


BlockAllThirdPartyCookies doesn't mean anything different to me than BlockThirdPartyCookies. DefaultBlockThirdPartyCookies maybe?

bridiver · 2019-11-13T17:01:53Z

browser/net/brave_network_delegate_browsertest.cc

@@ -298,3 +329,58 @@ IN_PROC_BROWSER_TEST_F(BraveNetworkDelegateBrowserTest,
  ExpectCookiesOnHost(embedded_test_server(), "a.com", "");
  ExpectCookiesOnHost(embedded_test_server(), "b.com", "");
 }
+
+IN_PROC_BROWSER_TEST_F(BraveNetworkDelegateBrowserTest,
+                       IframeGoogleAccountsCookieAllowed) {


should these maybe be ThirdPartyGoogleAccountsCookiesAllowed? Instead of Iframe? I know it's basically equivalent, but I think ThirdParty is a better description

bsclifton · 2019-11-19T00:11:33Z

🎉 !!

jumde requested a review from bridiver as a code owner October 23, 2019 16:28

jumde force-pushed the google_oauth branch 2 times, most recently from 4a6a17a to e0c3923 Compare October 29, 2019 22:21

jumde changed the title ~~[WIP] Fix Google OAuth~~ Fix 5869: Fix Google OAuth Oct 29, 2019

jumde changed the title ~~Fix 5869: Fix Google OAuth~~ Fix 5869: Google OAuth should work by default Oct 29, 2019

jumde self-assigned this Oct 29, 2019

jumde force-pushed the google_oauth branch from e0c3923 to e258833 Compare October 29, 2019 22:38

jumde requested review from iefremov and bsclifton October 29, 2019 22:40

bridiver reviewed Oct 30, 2019

View reviewed changes

bridiver requested changes Oct 30, 2019

View reviewed changes

bridiver reviewed Oct 30, 2019

View reviewed changes

iefremov reviewed Oct 30, 2019

View reviewed changes

jumde force-pushed the google_oauth branch 3 times, most recently from 0977c74 to a7be6ec Compare November 3, 2019 19:01

iefremov reviewed Nov 4, 2019

View reviewed changes

bridiver reviewed Nov 4, 2019

View reviewed changes

This was referenced Nov 4, 2019

Add a setting for blocking Disqus comments brave/brave-browser#5059

Open

Add social blocking toggle for YouTube embeds brave/brave-browser#4986

Open

jumde changed the title ~~Fix 5869: Google OAuth should work by default~~ [WIP] Fix 5869: Google OAuth should work by default Nov 7, 2019

jumde added the CI/skip Do not run CI builds (except noplatform) label Nov 7, 2019

jumde commented Nov 12, 2019

View reviewed changes

jumde changed the title ~~[WIP] Fix 5869: Google OAuth should work by default~~ Fix 5869: Google OAuth should work by default Nov 12, 2019

jumde removed the CI/skip Do not run CI builds (except noplatform) label Nov 12, 2019

jumde force-pushed the google_oauth branch from 55d09e0 to 31398ec Compare November 12, 2019 04:18

iefremov reviewed Nov 12, 2019

View reviewed changes

jumde force-pushed the google_oauth branch from 31398ec to 1f223e6 Compare November 13, 2019 00:31

iefremov self-requested a review November 13, 2019 01:19

iefremov approved these changes Nov 13, 2019

View reviewed changes

jumde force-pushed the google_oauth branch from 1f223e6 to de10737 Compare November 13, 2019 14:28

jumde requested a review from bridiver November 13, 2019 14:29

jumde force-pushed the google_oauth branch from de10737 to 622eb31 Compare November 13, 2019 16:37

bridiver reviewed Nov 13, 2019

View reviewed changes

jumde force-pushed the google_oauth branch from 622eb31 to b9f1c3e Compare November 13, 2019 22:10

bridiver approved these changes Nov 13, 2019

View reviewed changes

jumde force-pushed the google_oauth branch 2 times, most recently from 29563ff to 0728dde Compare November 14, 2019 21:28

Fix 5869: Google OAuth should work by default

307ecb2

jumde force-pushed the google_oauth branch from 0728dde to 307ecb2 Compare November 15, 2019 16:50

jumde merged commit cac5b82 into master Nov 15, 2019

jumde deleted the google_oauth branch November 15, 2019 20:49

jumde added this to the 1.3.x - Nightly milestone Nov 15, 2019

This was referenced Nov 19, 2019

Google Calendar Extension won't authenticate brave/brave-browser#4672

Closed

Login with Google OAuth not working brave/brave-browser#6408

Closed

LaurenWags mentioned this pull request Jan 10, 2020

Cannot login to expensify (Google Auth) brave/brave-browser#5869

Closed

rebron mentioned this pull request Mar 26, 2020

Can't remove the url 'https://accounts.google.com' from brave://settings/content/cookies page brave/brave-browser#8842

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix 5869: Google OAuth should work by default #3784

Fix 5869: Google OAuth should work by default #3784

jumde commented Oct 23, 2019 •

edited

Loading

bridiver Oct 30, 2019

iefremov Oct 30, 2019

bridiver Oct 30, 2019

bridiver Oct 30, 2019

bridiver Oct 30, 2019

bridiver Oct 30, 2019

bridiver left a comment

bridiver Oct 30, 2019

bridiver commented Oct 30, 2019

iefremov Oct 30, 2019

iefremov Nov 4, 2019

iefremov Nov 4, 2019

iefremov Nov 4, 2019

iefremov Nov 4, 2019

bridiver Nov 4, 2019

jumde Nov 11, 2019

iefremov Nov 4, 2019

bridiver Nov 4, 2019

jumde Nov 12, 2019

bridiver Nov 12, 2019

bridiver Nov 4, 2019

jumde Nov 12, 2019

iefremov Nov 12, 2019

bridiver Nov 13, 2019

bridiver Nov 13, 2019

bridiver Nov 13, 2019

bridiver Nov 13, 2019

bridiver Nov 13, 2019

bsclifton commented Nov 19, 2019

		@@ -21,6 +23,21 @@ namespace brave_shields {

		namespace {

		void SetCookieWhitelistForSocialMedia(HostContentSettingsMap* map,

		rules.clear();

		if (prefs_->GetBoolean(kGoogleLoginControlType)) {

Fix 5869: Google OAuth should work by default #3784

Fix 5869: Google OAuth should work by default #3784

Conversation

jumde commented Oct 23, 2019 • edited Loading

Submitter Checklist:

Test Plan:

Reviewer Checklist:

After-merge Checklist:

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bridiver left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bridiver commented Oct 30, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bsclifton commented Nov 19, 2019

jumde commented Oct 23, 2019 •

edited

Loading