Fix 3650: Enable google sign in for extensions #5616

jumde · 2020-05-21T23:59:56Z

Resolves brave/brave-browser#3650
Resolves brave/brave-browser#4672

Security Review: https://github.com/brave/security/issues/157 - (approved)

Submitter Checklist:

Test Plan:

Some follow-up changes were made to the workflow in https://github.com/brave/brave-core/pull/6516 -

To enable `Google Login for extensions` :

1. Navigate to brave://settings/extensions 
2. Toggle Google Login for extensions and Relaunch.

Open Brave with a new profile and verify no connections are made to accounts.google.com
Navigate to https://chrome.google.com/webstore/detail/google-keep-chrome-extens/lpcaedmchfhocbbapmcbpinfpgnhiddi to install google-keep extension
Verify that login to google works with google keep.
Navigate to https://chrome.google.com/webstore/detail/google-calendar/gmbgaklkmjakoegficnlkhebmhkjfich to install google-calendar extension
Verify that login works with google calendar
Remove the extensions from Brave
Navigate to brave://settings/siteData and clear all site data
Navigate to brave://settings/socialBlocking
Disable Allow Google login buttons on third party sites
Click Relaunch
Install Google Keep extension and navigate to brave.com
Try sign-in with Google Keep extension. It 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.

jumde · 2020-05-26T06:27:21Z

patches/components-signin-internal-identity_manager-gaia_cookie_manager_service.cc.patch

+@@ -553,6 +553,7 @@ void GaiaCookieManagerService::AddAccountToCookieWithToken(
+ bool GaiaCookieManagerService::ListAccounts(
+     std::vector<gaia::ListedAccount>* accounts,
+     std::vector<gaia::ListedAccount>* signed_out_accounts) {


This removes request to accounts.google.com on start, it is not required for enabling google sign in.

diracdeltas · 2020-05-27T18:26:45Z

app/brave_generated_resources.grd

@@ -403,7 +403,7 @@ By installing this extension, you are agreeing to the Google Widevine Terms of U
        Show the number of blocked items on the Shields icon
      </message>
      <message name="IDS_SETTINGS_BRAVE_SHIELDS_GOOGLE_LOGIN_LABEL" desc="Label for a switch control which allows Google social buttons to be enabled/disabled">
-        Allow Google login buttons on third party sites
+        Allow Google login in extensions and third party sites


suggest adding a link to a page (wiki?) explaining what google login in extensions entails

@diracdeltas - This is the wiki: https://developer.chrome.com/apps/identity - I can add it as a comment in the code. Or do you want me to create a separate brave wiki for it?

@jumde i mean add a page that explains to the user what this option means and what data gets sent to Google. i think most people understand what Google login in 3p sites means but it's not clear what gets sent to google when you enable google login in extensionos.

@diracdeltas - Here it is: https://github.com/brave/brave-browser/wiki/Social-Blocking:-Allow-Google-login

wiki page looks good, can you add an info link to it next to the setting?

@diracdeltas - Set here: https://github.com/brave/brave-core/pull/5616/files#diff-2ff48ebe156c663642e6a503ed3d3913R17

pilgrim-brave · 2020-06-02T15:57:39Z

chromium_src/components/signin/internal/identity_manager/primary_account_manager.cc

+
+// Ensures that requests to accounts.google.com is not initiated at startup if
+// google login for extensions is enabled
+#define BRAVE_NO_GAIA_REQUEST_ON_STARTUP                  \


Pretty sure that these defines should be named based on the class/function they're in, instead of by their use, but @bridiver can verify.

pilgrim-brave

LGTM modulo minor comments.

browser/ui/webui/settings/brave_social_blocking_handler.cc

app/brave_main_delegate.cc

chromium_src/components/sync/driver/sync_session_durations_metrics_recorder.cc

bwims · 2020-08-06T12:03:04Z

Does anyone know if this will resolve #5197 ? If so, when it is likely to be released?

Many thanks!

bridiver

see comments

petemill · 2020-08-18T04:25:58Z

ui/webui/resources/polymer_overriding.js

+    component.properties = component.properties || {}
+    Object.assign(component.properties, allPropertiesMap[moduleName])


I think this would be cleaner to understand with

component.properties = { ...component.properties, ...allPropertiesMap[moduleName] }

petemill · 2020-08-18T19:13:21Z

ui/webui/resources/polymer_overriding.js

@@ -73,6 +88,18 @@ export function RegisterPolymerComponentBehaviors(behaviorsMap) {
  Object.assign(allBehaviorsMap, behaviorsMap)
 }

+export function RegisterPolymerComponentProperties(propertiesMap) {
+  if (debug) {
+    console.error('RegisterPolymerComponentProperties', ...Object.keys(propertiesMap))


IMO would be better to have console.debug (or console.log) here so that we don't have things that look like errors in the long list of log entries that occur when debug flag is on

@petemill - updated

petemill · 2020-08-18T19:21:28Z

ui/webui/resources/polymer_overriding.js

@@ -18,6 +18,8 @@ if (debug) {
 }

 const allBehaviorsMap = {}
+const allPropertiesMap = {}
+const missingComponentsMap = {}


I think this is hard to understand and follow what it's used for.

How about calling it something explicit like componentPropertyModifications or propertyModificationComponentBuffer and a comment stating that components are held there in case RegisterPolymerComponentProperties is called after the component is registered?

petemill · 2020-08-18T19:21:56Z

ui/webui/resources/polymer_overriding.js

+RegisterStyleOverride('cr-button', CrButtonStyleTemplate)
+


did you mean to add a blank line?

simonhong · 2020-08-19T01:17:56Z

chromium_src/BUILD.gn

@@ -5,6 +5,9 @@

 source_set("browser") {
  sources = []
+  deps = [
+    "//brave/chromium_src/chrome/app"


I think //brave/chromium_src/chrome/app should not be added here because it is higher layer than chrome/browser, chrome/common or chrome/renderer.
Instead, how about adding //brave/chromium_src/chrome/app to //brave:browser_dependencies? @jumde @bridiver

When I needed to add new dependencies to brave_main_delegate.cc, I added them directly to //brave:browser_dependencies. but I think I should move them into //brave/chromium_src/chrome/app later.

simonhong · 2020-08-19T02:09:50Z

BUILD.gn

@@ -38,6 +38,7 @@ if (!is_ios) {
      "browser",
      "chromium_src:browser",
      "common",
+      "//brave/chromium_src/chrome/app"


nit: to align with others, "chromium_src/chrome/app would be better?

Hmm, we've used absolute/relative path in a mix :)

jumde · 2020-08-19T18:46:30Z

CI fails with known issues being fixed here:

#6455

bwims · 2020-09-23T11:46:00Z

@bwims - The latest release of Tab Outliner in paid mode is not working on Chrome as well. Are you able to login to your account with Chrome?
Here is what I tried:

Navigate to chrome-extension://eggkanocgddhmamlbiijnphhppkpkmkl/options.html

Click Sign Into Chrome - Nothing happens

Let me know if you're seeing the same behvior.

No, it works fine. You have to call up the extension, click the settings, cogwheel and enable access to Google Drive.
Let me know if you want screenshots!
Thanks!
First picture shows extension button

Next the settings cogwheel (you have to hover in the bottom left corner to see these options)

This is the backup tab on Brave which shows it cannot be authorized.

This shows the same tab on Google after clicking the authorize button

Hi!

I have been away for a while and I was wondering if (a) Tabs Outliner now works (b) if so, which version of Brave can I use with it?

Many thanks!

jumde · 2020-09-23T19:32:21Z

@bwims - Can you try using Tab Outliner with the latest version. You'll have to enable Google Login for extensions in brave://settings/extensions. 1.14.81 Chromium: 85.0.4183.102 (Official Build) (64-bit)

There is some additional work required to fix all Paid/Premium extensions, its being tracked here: brave/brave-browser#10622

bwims · 2020-09-24T11:34:30Z

@bwims - Can you try using Tab Outliner with the latest version. You'll have to enable Google Login for extensions in brave://settings/extensions. 1.14.81 Chromium: 85.0.4183.102 (Official Build) (64-bit)

There is some additional work required to fix all Paid/Premium extensions, its being tracked here: brave/brave-browser#10622

It half works. It lets me use the automatic backup paid feature but not others.

The main problem is still as described on
brave/brave-browser#5197 . It wants access to my Chrome Profile email, so I guess that is totally different.

I don't think it is related to brave/brave-browser#10622

Oh well!

jumde requested a review from bridiver as a code owner May 21, 2020 23:59

jumde self-assigned this May 21, 2020

jumde force-pushed the keep_extension branch 2 times, most recently from 8fc7ff4 to 407cbcb Compare May 26, 2020 06:18

jumde commented May 26, 2020

View reviewed changes

jumde force-pushed the keep_extension branch 2 times, most recently from 04c3b7f to 2c5d065 Compare May 26, 2020 18:14

diracdeltas reviewed May 27, 2020

View reviewed changes

jumde force-pushed the keep_extension branch from b481a3c to 51bab87 Compare May 28, 2020 19:20

jumde added the CI/run-network-audit Run network-audit label May 28, 2020

jumde force-pushed the keep_extension branch 2 times, most recently from 91ee1bf to 984be74 Compare May 31, 2020 23:37

jumde changed the title ~~[WIP] Fix 3650: Enable google sign in for extensions~~ Fix 3650: Enable google sign in for extensions May 31, 2020

jumde requested review from iefremov and pilgrim-brave May 31, 2020 23:41

jumde force-pushed the keep_extension branch 2 times, most recently from 9d2f11d to a5753f0 Compare June 1, 2020 02:22

jumde requested a review from bsclifton June 1, 2020 03:48

pilgrim-brave reviewed Jun 2, 2020

View reviewed changes

darkdh reviewed Jun 3, 2020

View reviewed changes

This was referenced Jul 20, 2020

Unable to authenticate or use Google Keep extension in Brave brave/brave-browser#3650

Closed

Tabs Outliner extension doesn't work in Brave brave/brave-browser#5197

Open

jumde force-pushed the keep_extension branch 5 times, most recently from 6825e6c to 80c9755 Compare August 5, 2020 23:03

jumde force-pushed the keep_extension branch from 80c9755 to f514edb Compare August 7, 2020 00:07

bridiver requested changes Aug 17, 2020

View reviewed changes

petemill reviewed Aug 18, 2020

View reviewed changes

jumde force-pushed the keep_extension branch 3 times, most recently from 28ed4f8 to 5d0f022 Compare August 18, 2020 18:57

petemill reviewed Aug 18, 2020

View reviewed changes

jumde requested review from petemill, bridiver and diracdeltas August 18, 2020 20:11

bridiver approved these changes Aug 18, 2020

View reviewed changes

jumde force-pushed the keep_extension branch 2 times, most recently from 334b13d to b07893f Compare August 19, 2020 01:02

simonhong reviewed Aug 19, 2020

View reviewed changes

jumde force-pushed the keep_extension branch from b07893f to 5196b73 Compare August 19, 2020 01:59

simonhong reviewed Aug 19, 2020

View reviewed changes

jumde force-pushed the keep_extension branch from 5196b73 to eca7a47 Compare August 19, 2020 04:15

Fix 3650: Enable google sign in for extensions

4317137

jumde force-pushed the keep_extension branch from eca7a47 to 4317137 Compare August 19, 2020 16:10

jumde merged commit 50b1ac4 into master Aug 19, 2020

jumde deleted the keep_extension branch August 19, 2020 18:46

jumde added this to the 1.14.x - Nightly milestone Aug 19, 2020

Brave-Matt mentioned this pull request Aug 21, 2020

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

Closed

jumde mentioned this pull request Sep 1, 2020

Setup oauth client and secret for extension login brave/brave-browser#9882

Closed

mariospr mentioned this pull request May 11, 2021

Setting "Allow Google login for extensions" does not work brave/brave-browser#15754

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix 3650: Enable google sign in for extensions #5616

Fix 3650: Enable google sign in for extensions #5616

jumde commented May 21, 2020 •

edited

Loading

jumde May 26, 2020 •

edited

Loading

diracdeltas May 27, 2020

jumde May 27, 2020

diracdeltas May 27, 2020

jumde May 29, 2020

diracdeltas Jun 2, 2020

jumde Aug 9, 2020

pilgrim-brave Jun 2, 2020

pilgrim-brave left a comment

bwims commented Aug 6, 2020

bridiver left a comment

petemill Aug 18, 2020

petemill Aug 18, 2020

jumde Aug 18, 2020

petemill Aug 18, 2020

petemill Aug 18, 2020

simonhong Aug 19, 2020 •

edited

Loading

simonhong Aug 19, 2020 •

edited

Loading

jumde commented Aug 19, 2020

bwims commented Sep 23, 2020

jumde commented Sep 23, 2020

bwims commented Sep 24, 2020

		component.properties = component.properties \|\| {}
		Object.assign(component.properties, allPropertiesMap[moduleName])

		RegisterStyleOverride('cr-button', CrButtonStyleTemplate)

Fix 3650: Enable google sign in for extensions #5616

Fix 3650: Enable google sign in for extensions #5616

Conversation

jumde commented May 21, 2020 • edited Loading

Submitter Checklist:

Test Plan:

Reviewer Checklist:

After-merge Checklist:

jumde May 26, 2020 • edited Loading

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

pilgrim-brave left a comment

Choose a reason for hiding this comment

bwims commented Aug 6, 2020

bridiver left a 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

simonhong Aug 19, 2020 • edited Loading

Choose a reason for hiding this comment

simonhong Aug 19, 2020 • edited Loading

Choose a reason for hiding this comment

jumde commented Aug 19, 2020

bwims commented Sep 23, 2020

jumde commented Sep 23, 2020

bwims commented Sep 24, 2020

jumde commented May 21, 2020 •

edited

Loading

jumde May 26, 2020 •

edited

Loading

simonhong Aug 19, 2020 •

edited

Loading

simonhong Aug 19, 2020 •

edited

Loading