Fix #11444: Google accounts should not show in the profile menu #6516

jumde · 2020-08-26T23:33:18Z

Resolves brave/brave-browser#11444

Description

This PR moves the toggle to enable Google Login for extensions to brave://settings/extensions which is disabled by default.
ProfileViewMenu should not display google profile info when the user has enabled Google Login for extensions.

Submitter Checklist:

Test Plan:

Launch browser with a clean profile. Login to gmail.com.
Navigate to brave://signin-internals and verify there are no Accounts in Token Service or Refresh token events
Navigate to brave://settings/extensions - Toggle Google Login for extensions and Relaunch
Navigate to https://chrome.google.com/webstore/detail/google-keep-chrome-extens/lpcaedmchfhocbbapmcbpinfpgnhiddi - Install and verify login works
Navigate to https://chrome.google.com/webstore/detail/google-calendar/gmbgaklkmjakoegficnlkhebmhkjfich - Install and verify that login works
Add a new profile Profiles > Add Profile and verify that the Profile Menu next to the hamburger menu doesn't show your google login information.
Navigate to brave://settings/siteData and delete all site data
Navigate to brave://settings/extensions - Toggle Google Login for extensions and Relaunch
Navigate to https://chrome.google.com/webstore/detail/google-keep-chrome-extens/lpcaedmchfhocbbapmcbpinfpgnhiddi - Install and Sign In will not work.

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.

mkarolin · 2020-08-27T20:17:11Z

chromium_src/chrome/browser/profiles/gaia_info_update_service_factory.cc

+
+#include "brave/browser/profiles/brave_gaia_info_update_service.h"
+
+#define  GAIAInfoUpdateService BraveGAIAInfoUpdateService


I am getting an error in Debug build on Windows:

lld-link: error: undefined symbol: public: static class GAIAInfoUpdateService * __cdecl GAIAInfoUpdateServiceFactory::GetForProfile(class Profile *) >>> referenced by C:\bb4\brave-browser\src\chrome\browser\profiles\profile_impl.cc:671

This changes the return type of GAIAInfoUpdateService* GAIAInfoUpdateServiceFactory::GetForProfile as well.

It might be easier to override ShouldUseGAIAProfileInfo directly by adding chromium_src override of gaia_info_update_service.h like so:

#ifndef BRAVE_CHROMIUM_SRC_CHROME_BROWSER_PROFILES_GAIA_INFO_UPDATE_SERVICE_H_ #define BRAVE_CHROMIUM_SRC_CHROME_BROWSER_PROFILES_GAIA_INFO_UPDATE_SERVICE_H_ #define ShouldUseGAIAProfileInfo \ ShouldUseGAIAProfileInfo_ChromiumImpl(Profile* profile); \ static bool ShouldUseGAIAProfileInfo #include "../../../../../chrome/browser/profiles/gaia_info_update_service.h" #undef ShouldUseGAIAProfileInfo #endif // BRAVE_CHROMIUM_SRC_CHROME_BROWSER_PROFILES_GAIA_INFO_UPDATE_SERVICE_H_

and changing the gaia_info_update_service.cc override to be:

#include "chrome/browser/profiles/gaia_info_update_service.h" #define ShouldUseGAIAProfileInfo ShouldUseGAIAProfileInfo_ChromiumImpl #include "../../../../../chrome/browser/profiles/gaia_info_update_service.cc" #undef ShouldUseGAIAProfileInfo // static bool GAIAInfoUpdateService::ShouldUseGAIAProfileInfo(Profile* profile) { return false; }

and then there will be no need for browser/profiles/brave_gaia_info_update_service.{h,cc} or chromium_src/chrome/browser/profiles/gaia_info_update_service_factory.cc` overrides.

this is helpful, thanks @mkarolin. I'll update

mkarolin · 2020-08-27T20:19:44Z

browser/ui/views/profiles/brave_profile_menu_view.cc

@@ -33,6 +33,9 @@ void BraveProfileMenuView::BuildAutofillButtons() {}
 // We don't want to show any Chromium sync info.
 void BraveProfileMenuView::BuildSyncInfo() {}

+// We don't want any feature buttons like MANAGE_GOOGLE_ACCOUNT
+void BraveProfileMenuView::BuildFeatureButtons() {}


I believe we want to keep the IDS_PROFILES_CLOSE_X_WINDOWS_BUTTON here. At least it's part of the intended design change here brave/brave-browser#8807

jumde · 2020-08-27T22:34:11Z

chromium_src/chrome/browser/extensions/api/webrtc_logging_private/webrtc_logging_private_api.cc

+#include "chrome/browser/extensions/api/webrtc_logging_private/webrtc_logging_private_api.h"
+
+#define BRAVE_DISABLE_RPH_FROM_REQUEST \
+    *error = "Unsupported"; return nullptr;


Will be reverted when this is merged: #6522

jumde · 2020-08-27T22:34:41Z

...hes/chrome-browser-extensions-api-webrtc_logging_private-webrtc_logging_private_api.cc.patch

 --- a/chrome/browser/extensions/api/webrtc_logging_private/webrtc_logging_private_api.cc
 +++ b/chrome/browser/extensions/api/webrtc_logging_private/webrtc_logging_private_api.cc
 @@ -89,6 +89,7 @@ content::RenderProcessHost* WebrtcLoggingPrivateFunction::RphFromRequest(
     const api::webrtc_logging_private::RequestInfo& request,
     const std::string& security_origin,
     std::string* error) {
-+  return nullptr; // feature disabled in Brave
+  BRAVE_DISABLE_RPH_FROM_REQUEST //feature disabled in Brave


Will be reverted when this is merged: #6522

bridiver · 2020-08-28T16:54:52Z

browser/ui/views/profiles/brave_profile_menu_view.cc

 #include "chrome/browser/profiles/profile_attributes_entry.h"
 #include "chrome/browser/profiles/profile_manager.h"
 #include "chrome/browser/profiles/profile_window.h"
+#include "chrome/browser/ui/browser_finder.h"
+#include "chrome/grit/generated_resources.h"
+#include "components/vector_icons/vector_icons.h"


are there existing deps for this and ui/base?

@bridiver - here: https://github.com/brave/brave-core/blob/master/browser/ui/BUILD.gn#L220

bridiver

chromium_src changes look ok, someone else should verify functionality

bsclifton

Changes LGTM - ran through the test plan on Windows and it works great 😄

Also ran unit tests + browser tests locally; every single one passed (no retries, etc). Looking perfect!

petemill

Didn't get a chance to run it, but code looks good. Only left an optional nit!

petemill · 2020-08-28T20:38:35Z

...r/resources/settings/brave_default_extensions_page/brave_default_extensions_browser_proxy.js

@@ -59,6 +60,9 @@ cr.define('settings', function() {
    getWeb3ProviderList() {
      return new Promise(resolve => chrome.braveWallet.getWeb3ProviderList(resolve))
    }
+    wasSignInEnabledAtStartup() {


nit (see related): doesn't need to be in proxy since not sending any data

petemill · 2020-08-28T20:40:11Z

browser/resources/settings/brave_default_extensions_page/brave_default_extensions_page.js

@@ -101,5 +102,10 @@ Polymer({
  openWebStoreUrl_: function() {
    window.open(loadTimeData.getString('getMoreExtensionsUrl'));
  },
+
+  shouldShowRestartForGoogleLogin_: function(value) {
+    return this.browserProxy_.wasSignInEnabledAtStartup() != value;


nit: this doesn't need to be either a function (since it's static) or something from browserProxy (since this component can access loadTimeData just fine). Can simply be a polymer property initialized with the value of loadTimeData.getBoolean('signInAllowedOnNextStartupInitialValue'). Still, just a nit and only mentioning to reduce code or for the future.

mkarolin

LGTM

LaurenWags · 2020-08-31T14:09:22Z

Verified using PR build provided by PJ using macOS x64 10.14.6:

Verified STR from PR description

Step 2: Confirmed no Accounts in Token Service or Refresh token events

Step 3: Confirmed Allow Google Login for extensions is off by default

Step 3: Confirmed toggling Allow Google Login for extensions prompts for restart

Step 3: Confirmed Allow Google Login for extensions is on after restart

Step 4: Able to use Google Keep extension

Step 5: Able to use Google Calendar extension

Step 6: Confirmed profile menu doesn't show any Google sign in data

Step 9: Confirmed after removing site data, disabling Allow Google Login for extensions and relaunching, when attempting to use the Google Keep extension the Sign in button does not work

Also verified the below scenarios:

Scenario 1

Installed build with fresh profile
Navigated to brave://settings/extensions, enabled Allow Google Login for extensions and relaunched
Confirmed setting retained after relaunch
Navigate to https://chrome.google.com/webstore/detail/google-keep-chrome-extens/lpcaedmchfhocbbapmcbpinfpgnhiddi and install this extension
Close Google login page that opens
Open a tab and navigate to brave.com
Open extension, see Sign in button as below
Click on it and login on resulting Google page
Confirmed able to login and use extension

Scenario 2:

Installed build with fresh profile
Navigated to brave://settings/extensions, enabled Allow Google Login for extensions and relaunched
Confirmed setting is on
Disabled Allow Google Login for extensions and relaunched
Confirmed setting is off
Enabled Allow Google Login for extensions and relaunched
Confirmed setting is on

Verification passed on Windows 10 x64 using local build given by @jumde

Verified the test plan mentioned in the PR

Step 2: Confirmed no Accounts in Token Service or `Refresh token events

Step 3: Confirmed Allow Google Login for extensions is off by default

Confirmed Allow Google Login for extensions is on after restart

Step 4: Able to use Google Keep extension

Step 5: Able to use Google Calendar extension

Step 6: Confirmed profile menu doesn't show any Google sign-in data

Step 9: Confirmed after removing site data, disabling Allow Google Login for extensions and relaunching, when attempting to use the Google Keep extension the Sign in button does not work

Also verified the below scenarios:

Scenario 1 (This PR fixed the issue #11493)

Installed build with fresh profile
Navigated to brave://settings/extensions, enabled Allow Google Login for extensions and relaunched
Confirmed setting retained after relaunch
Navigate to https://chrome.google.com/webstore/detail/google-keep-chrome-extens/lpcaedmchfhocbbapmcbpinfpgnhiddi and install this extension
Close Google login page that opens
Open a tab and navigate to brave.com
Open extension, see Sign in button as below
Click on it and login on resulting Google page
Confirmed able to login and use extension

Scenario 2:

Installed build with fresh profile
Navigated to brave://settings/extensions, enabled Allow Google Login for extensions and relaunched
Confirmed setting is on
Disabled Allow Google Login for extensions and relaunched
Confirmed setting is off
Enabled Allow Google Login for extensions and relaunched
Confirmed setting is on
Restart the browser
Confirmed setting is on

jumde · 2020-08-31T17:23:40Z

CI failure for test-install on windows is known: https://ci.brave.com/job/pr-brave-browser-profile_manager-windows/15/

Fix #11444: Google accounts should not show in the profile menu

LaurenWags · 2020-09-01T13:29:46Z

Verified using

Brave | 1.15.25 Chromium: 85.0.4183.83 (Official Build) nightly (64-bit)
-- | --
Revision | 94abc2237ae0c9a4cb5f035431c8adfb94324633-refs/branch-heads/4183@{#1658}
OS | macOS Version 10.14.6 (Build 18G3020)

Verified STR from PR description

Step 2: Confirmed no Accounts in Token Service or Refresh token events

Step 3: Confirmed Allow Google Login for extensions is off by default

Step 3: Confirmed toggling Allow Google Login for extensions prompts for restart

Step 3: Confirmed Allow Google Login for extensions is on after restart

Step 4: Able to use Google Keep extension

Step 5: Able to use Google Calendar extension

Step 6: Confirmed profile menu doesn't show any Google sign in data for either profile

Step 9: Confirmed after removing site data, disabling Allow Google Login for extensions and relaunching, when attempting to use the Google Keep extension the Sign in button does not work

Also verified the below scenarios:

Scenario 1 - install extension before signing into Google

Installed build with fresh profile
Navigated to brave://settings/extensions, enabled Allow Google Login for extensions and relaunched
Confirmed setting retained after relaunch
Navigate to https://chrome.google.com/webstore/detail/google-keep-chrome-extens/lpcaedmchfhocbbapmcbpinfpgnhiddi and install this extension
Close Google login page that opens
Open a tab and navigate to brave.com
Open extension, see Sign in button as below
Click on it and login on resulting Google page
Confirmed able to login and use extension

NOTE - if you leave sign in page open in step 5, you will encounter brave/brave-browser#11493. Discussed with @jumde and #11493 has been labeled Chromium/waiting upstream.

Scenario 2 - toggle setting multiple times

Installed build with fresh profile
Navigated to brave://settings/extensions, enabled Allow Google Login for extensions and relaunched
Confirmed setting is on
Disabled Allow Google Login for extensions and relaunched
Confirmed setting is off
Enabled Allow Google Login for extensions and relaunched
Confirmed setting is on

Scenario 3 - STR from brave/brave-browser/issues/11444

Clean profile
Create a second profile
In one of the windows, sign into mail.google.com (this is Window A)
Click on the profile icon of Window A
Confirm you do not see "Manage your Brave sync chain" and "Sign out" options.
Close and Relaunch.
Click on profile icon of Window A
Confirm profile icon has not changed to reflect the google account's image.
Click on profile icon of Window B.
Confirm you do not see "Manage your Brave sync chain" and "Sign out" options. Confirm profile icon has not changed.

Also tried signing in/out various times, enabling/disabling the Allow Google login for extensions setting, having multiple gmail accounts logged in under one profile or under 2 profiles. In all cases, no google account information was observed in the profile menu.

mkarolin · 2020-10-22T16:01:01Z

browser/resources/settings/social_blocking_page/social_blocking_page.js

-    this.restartBrowser_ = this.restartBrowser_.bind(this)
-  },
-
-  shouldShowRestart_: function(value) {


@jumde this function and setSignInEnabledAtNextStartup_ below have been removed from this .js but they are still referenced in the HTML. Were they meant to stay and just have their functionality moved here from the proxy?

jumde requested a review from bridiver as a code owner August 26, 2020 23:33

jumde self-assigned this Aug 26, 2020

jumde force-pushed the profile_manager branch from 6877907 to fef6c87 Compare August 26, 2020 23:35

jumde mentioned this pull request Aug 27, 2020

[WIP] Fix #11404: Remove account info in profile menu #6507

Closed

32 tasks

jumde force-pushed the profile_manager branch 3 times, most recently from bbb7201 to 0b2b2f2 Compare August 27, 2020 19:11

jumde added the CI/run-network-audit Run network-audit label Aug 27, 2020

LaurenWags mentioned this pull request Aug 27, 2020

[Desktop] google account info showing in profile manager brave/brave-browser#11444

Closed

jumde requested review from mkarolin, bsclifton and petemill August 27, 2020 19:23

mkarolin reviewed Aug 27, 2020

View reviewed changes

jumde requested a review from mkarolin August 27, 2020 21:21

jumde force-pushed the profile_manager branch from 0b2b2f2 to 28ac720 Compare August 27, 2020 22:28

jumde commented Aug 27, 2020

View reviewed changes

jumde force-pushed the profile_manager branch 5 times, most recently from 8ac3807 to 0414d4e Compare August 28, 2020 16:39

bridiver reviewed Aug 28, 2020

View reviewed changes

bridiver approved these changes Aug 28, 2020

View reviewed changes

jumde force-pushed the profile_manager branch from 0414d4e to 3b2318a Compare August 28, 2020 19:37

bsclifton approved these changes Aug 28, 2020

View reviewed changes

petemill approved these changes Aug 28, 2020

View reviewed changes

mkarolin approved these changes Aug 28, 2020

View reviewed changes

Fix #11444: Google accounts should not show in the profile menu

762d135

jumde force-pushed the profile_manager branch from 3b2318a to 762d135 Compare August 30, 2020 19:56

jumde mentioned this pull request Aug 31, 2020

Fix 3650: Enable google sign in for extensions #5616

Merged

32 tasks

jumde merged commit 239fcb8 into master Aug 31, 2020

jumde deleted the profile_manager branch August 31, 2020 20:56

jumde added a commit that referenced this pull request Aug 31, 2020

Merge pull request #6516 from brave/profile_manager

f163390

Fix #11444: Google accounts should not show in the profile menu

jumde added this to the 1.15.x - Nightly milestone Aug 31, 2020

jumde mentioned this pull request Aug 31, 2020

Fix #11444: Google accounts should not show in the profile menu (uplift to 1.14.x) #6552

Merged

4 tasks

jumde added the l10n label Sep 21, 2020

mkarolin reviewed Oct 22, 2020

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix #11444: Google accounts should not show in the profile menu #6516

Fix #11444: Google accounts should not show in the profile menu #6516

jumde commented Aug 26, 2020 •

edited

Loading

mkarolin Aug 27, 2020

mkarolin Aug 27, 2020

jumde Aug 27, 2020

mkarolin Aug 27, 2020

jumde Aug 27, 2020

jumde Aug 27, 2020

bridiver Aug 28, 2020

jumde Aug 28, 2020

bridiver left a comment

bsclifton left a comment

petemill left a comment

petemill Aug 28, 2020

petemill Aug 28, 2020

mkarolin left a comment

LaurenWags commented Aug 31, 2020 •

edited by GeetaSarvadnya

Loading

jumde commented Aug 31, 2020 •

edited

Loading

LaurenWags commented Sep 1, 2020

mkarolin Oct 22, 2020

jumde Oct 22, 2020


		#include "brave/browser/profiles/brave_gaia_info_update_service.h"

		#define GAIAInfoUpdateService BraveGAIAInfoUpdateService

Fix #11444: Google accounts should not show in the profile menu #6516

Fix #11444: Google accounts should not show in the profile menu #6516

Conversation

jumde commented Aug 26, 2020 • edited Loading

Description

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

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

bsclifton left a comment

Choose a reason for hiding this comment

petemill left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mkarolin left a comment

Choose a reason for hiding this comment

LaurenWags commented Aug 31, 2020 • edited by GeetaSarvadnya Loading

jumde commented Aug 31, 2020 • edited Loading

LaurenWags commented Sep 1, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jumde commented Aug 26, 2020 •

edited

Loading

LaurenWags commented Aug 31, 2020 •

edited by GeetaSarvadnya

Loading

jumde commented Aug 31, 2020 •

edited

Loading