This is an issue with misbehaving extensions that are using the chrome.identity API. See also Issue 695963 for more background.

Mihai - did we change anything with how the chrome.identity API behaves recently? Maybe this relates to the first milestone of the Dice rollout (I think all the reports I've seen have had auth errors in 61)?

It seems like the reports we've been getting recently have been much more severe than what we were seeing before, where we continuously pop up a login page.

Actually, first milestone of Dice isn't rolling out until 62, so can't be that. Will follow-up over email

CC+ Colin

Colin has been moving the chrome.identity API to the Identity Service and the code landed in M61 and before. Maybe this change triggered some weird behavior if the user is in an auth error state.

Indeed the problem is a behavior change introduced by the usage of the Identity service. We have found the root cause and we have a potential fix (see CL https://chromium-review.googlesource.com/c/chromium/src/+/704743 ), however Colin would like to try and add an integration test for this change.

I'm reassigning this bug to Colin and marking it as RBS for M62 (I think we should not respin M61 for this though).

Removing the M-61 milestone label to make it clear that this is RBS for 62. Thanks for the quick work Colin.

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/86ee410de631c830b48440daca1d25bafd2d0a01

commit 86ee410de631c830b48440daca1d25bafd2d0a01
Author: Colin Blundell <blundell@chromium.org>
Date: Mon Oct 09 16:55:03 2017

Identity Service: Have GetPrimaryAccountWhenAvailable() check refresh token

https://chromium-review.googlesource.com/539637 introduced the
Identity Service GetPrimaryAccountWhenAvailable() method and changed the
identity extension API implementation to use it. However, that CL
inadvertently introduced a semantic change in the identity extension API
implementation in the case where the user was signed in with their refresh
token in an error state. Previously, the implementation would start a
signin flow and wait for a notification that a refresh token was available.
Crucially, this notification would come in only when a *new* refresh token
was available. GetPrimaryAccountWhenAvailable(), by contrast, returned
immediately if the user had a refresh token available (even if that
refresh token was in an error state). Due to a separate bug in the
identity API extension implementation itself, the net effect of this
semantic change is that the identity extension API implementation would loop
forever trying new requests and then going through the signin flow again
when those requests inevitably failed (because they were all being made
with the same bad refresh token).

This CL changes the semantics of the Identity Services'
GetPrimaryAccountWhenAvailable() method: it returns only when the user
has a primary account and that primary account has a refresh token *in a
non-error state.* This change, which better reflects the spirit of the
method, also restores the previous behavior of the identity extension API
implementation. Note that as the identity extension API implementation is
currently the only caller of this method in the codebase, this change
will have no other effects.

To test this change, do the following:
- Install an extension with the identity API in its manifest.
- Sign in to Chrome.
- Go to accounts.google.com and revoke your refresh token for Chrome there.
- Ensure that your account in Chrome goes into an error state (verify
  that a red exclamation point appears in the top right by your account name).
- Go to chrome://extensions, enable developer mode, and inspect the background
  page of the above app. At the JS console that that brings up, execute:
  chrome.identity.getAuthToken({interactive: true}, (token) => {console.log(token);} )
- Verify that you can close the signin tab that then opens and continue
  using the browser as normal.
- Sign in to Chrome.
- Verify that at the JS console referenced above a token gets printed.

This CL also adds a unittest that fails without the production change. As a
followup, I am going to investigate adding browsertests of the Chrome Identity
extension API that also would have failed before this change.

Bug:  772122 
Change-Id: I544406684de1945b51807acac303bb667363615a
Reviewed-on: https://chromium-review.googlesource.com/704697
Reviewed-by: Mike West <mkwst@chromium.org>
Reviewed-by: Mihai Sardarescu <msarda@chromium.org>
Commit-Queue: Colin Blundell (at a convergence, slow until Oct 16) <blundell@chromium.org>
Cr-Commit-Position: refs/heads/master@{#507394}
[modify] https://crrev.com/86ee410de631c830b48440daca1d25bafd2d0a01/google_apis/gaia/oauth2_token_service.cc
[modify] https://crrev.com/86ee410de631c830b48440daca1d25bafd2d0a01/google_apis/gaia/oauth2_token_service.h
[modify] https://crrev.com/86ee410de631c830b48440daca1d25bafd2d0a01/services/identity/DEPS
[modify] https://crrev.com/86ee410de631c830b48440daca1d25bafd2d0a01/services/identity/README.md
[modify] https://crrev.com/86ee410de631c830b48440daca1d25bafd2d0a01/services/identity/identity_manager.cc
[modify] https://crrev.com/86ee410de631c830b48440daca1d25bafd2d0a01/services/identity/identity_manager_unittest.cc
[modify] https://crrev.com/86ee410de631c830b48440daca1d25bafd2d0a01/services/identity/public/interfaces/identity_manager.mojom

[Auto-generated comment by a script] We noticed that this issue is targeted for M-62; it appears the fix may have landed after branch point, meaning a merge might be required. Please confirm if a merge is required here - if so add Merge-Request-62 label, otherwise remove Merge-TBD label. Thanks.

Let's test this on tomorrow's Canary and then request a merge if all looks good. Thanks Colin!

Last canary (version 63.0.3236.0) does not include the CL https://chromium-review.googlesource.com/704697, but we believe this fix is critical and it was also manually tested during development. If there is a canary that will be pushed later today, then we'll manually manually on Canary as well. Will another Canary be released today and if so at what time? 

If there is no Canary today, we think the fix is safe enough and we recommend merging it back today to ensure it is included in the next beta. The alternative seems to be that we would merge it directly to Stable after verifying on Canary.

I am also filing for a merge request to M62 as we're working from Munich, 9 hours ahead of MTV, so doing all the necessary round-trips to get the merge approved and then actually get the merge in today will be very tight timing-wise. Thank you!

This bug requires manual review: We are only 6 days from stable.
Please contact the milestone owner if you have questions.
Owners: amineer@(Android), cmasso@(iOS), bhthompson@(ChromeOS), abdulsyed@(Desktop)

For more details visit https://www.chromium.org/issue-tracking/autotriage - Your friendly Sheriffbot

This seems like a fairly critical regression. I am fine with plan in #13 if it's well tested locally and seems like there is full unit test coverage. Approving merge to M62: branch:3202.

Chatted with Abdul offline. This merge needs to happen before 3 PM PST today to make it into the cut for the last Beta tomorrow.

+Pavel has kindly offered to step in and merge the CL in c#9 to M62 branch 3202 for us, since the sign-in team is currently in Munich :) Thanks Pavel! Let us know if you run into any issues.

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/8ddd07507cfccb3518edea736f904869c08ad3d6

commit 8ddd07507cfccb3518edea736f904869c08ad3d6
Author: Pavel Yatsuk <pavely@chromium.org>
Date: Tue Oct 10 19:12:41 2017

Identity Service: Have GetPrimaryAccountWhenAvailable() check refresh token

https://chromium-review.googlesource.com/539637 introduced the
Identity Service GetPrimaryAccountWhenAvailable() method and changed the
identity extension API implementation to use it. However, that CL
inadvertently introduced a semantic change in the identity extension API
implementation in the case where the user was signed in with their refresh
token in an error state. Previously, the implementation would start a
signin flow and wait for a notification that a refresh token was available.
Crucially, this notification would come in only when a *new* refresh token
was available. GetPrimaryAccountWhenAvailable(), by contrast, returned
immediately if the user had a refresh token available (even if that
refresh token was in an error state). Due to a separate bug in the
identity API extension implementation itself, the net effect of this
semantic change is that the identity extension API implementation would loop
forever trying new requests and then going through the signin flow again
when those requests inevitably failed (because they were all being made
with the same bad refresh token).

This CL changes the semantics of the Identity Services'
GetPrimaryAccountWhenAvailable() method: it returns only when the user
has a primary account and that primary account has a refresh token *in a
non-error state.* This change, which better reflects the spirit of the
method, also restores the previous behavior of the identity extension API
implementation. Note that as the identity extension API implementation is
currently the only caller of this method in the codebase, this change
will have no other effects.

To test this change, do the following:
- Install an extension with the identity API in its manifest.
- Sign in to Chrome.
- Go to accounts.google.com and revoke your refresh token for Chrome there.
- Ensure that your account in Chrome goes into an error state (verify
  that a red exclamation point appears in the top right by your account name).
- Go to chrome://extensions, enable developer mode, and inspect the background
  page of the above app. At the JS console that that brings up, execute:
  chrome.identity.getAuthToken({interactive: true}, (token) => {console.log(token);} )
- Verify that you can close the signin tab that then opens and continue
  using the browser as normal.
- Sign in to Chrome.
- Verify that at the JS console referenced above a token gets printed.

This CL also adds a unittest that fails without the production change. As a
followup, I am going to investigate adding browsertests of the Chrome Identity
extension API that also would have failed before this change.

TBR=blundell@chromium.org

(cherry picked from commit 86ee410de631c830b48440daca1d25bafd2d0a01)

Bug:  772122 
Change-Id: I544406684de1945b51807acac303bb667363615a
Reviewed-on: https://chromium-review.googlesource.com/704697
Reviewed-by: Mike West <mkwst@chromium.org>
Reviewed-by: Mihai Sardarescu <msarda@chromium.org>
Commit-Queue: Colin Blundell (at a convergence, slow until Oct 16) <blundell@chromium.org>
Cr-Original-Commit-Position: refs/heads/master@{#507394}
Reviewed-on: https://chromium-review.googlesource.com/710161
Reviewed-by: Pavel Yatsuk <pavely@chromium.org>
Cr-Commit-Position: refs/branch-heads/3202@{#640}
Cr-Branched-From: fa6a5d87adff761bc16afc5498c3f5944c1daa68-refs/heads/master@{#499098}
[modify] https://crrev.com/8ddd07507cfccb3518edea736f904869c08ad3d6/google_apis/gaia/oauth2_token_service.cc
[modify] https://crrev.com/8ddd07507cfccb3518edea736f904869c08ad3d6/google_apis/gaia/oauth2_token_service.h
[modify] https://crrev.com/8ddd07507cfccb3518edea736f904869c08ad3d6/services/identity/DEPS
[modify] https://crrev.com/8ddd07507cfccb3518edea736f904869c08ad3d6/services/identity/README.md
[modify] https://crrev.com/8ddd07507cfccb3518edea736f904869c08ad3d6/services/identity/identity_manager.cc
[modify] https://crrev.com/8ddd07507cfccb3518edea736f904869c08ad3d6/services/identity/identity_manager_unittest.cc
[modify] https://crrev.com/8ddd07507cfccb3518edea736f904869c08ad3d6/services/identity/public/interfaces/identity_manager.mojom

 Issue 766840  has been merged into this issue.

 Issue 764735  has been merged into this issue.

I verified the fix on 62.0.3202.52 on Mac using the instructions given in the CL description.

I think I'm running into this bug now, and I'm running 62.0.3202.75 on linux.

Repro steps:
1) Start chrome with an empty user data dir.
2) Sign in to chrome (with jwd@google.com account)
3) Wait for things to sync.
4) Sign in loop at chrome://chrome-signin/?access_point=6&reason=0.

I can get out of the loop sometimes by completing that signin. This results in a red triangle icon in the profile chooser. If I try to re-sign in, it re-enters the sign in loop.

If I'm in the red triangle icon state I can then disable my extensions, re-sign in, and everything seems fine. Re-enabling some extensions will then cause the loop again.

This was after being OOO and not using this install for 4 weeks.

Hey Jesse, I'm not sure I understand your repro steps. What exactly does "Sign in loop at chrome://chrome-signin/?access_point=6&reason=0" mean? Does it mean that a tab is repeatedly opened with that URL, and Chrome is essentially unusable?

I'm also confused how signing in through the page at chrome://chrome-signin/?access_point=6&reason=0 causes you to get into an auth error state (red triangle in the profile chooser). When you sign-in through that page, it should be *fixing* your auth error, not causing a new one.

Would it be possible for you to take a video of the entire end-to-end flow with the repro steps you've listed above and attach it to this bug?

Also, Colin/Mihai - can you please investigate to make sure we didn't cause any regressions in M62? Let's treat this as P1, since M62 is ramping up this week. Taking off the RBS label for now until we investigate.

Investigating. I have the same questions as Eli in c#23; if you can sign in at all, it seems like this can't be exactly the same behavior as previous users were seeing?

I just confirmed that the original buggy behavior has not regressed on trunk by executing the steps described in c#9 above. Additionally:

- I have not landed any CLs on trunk since the one in c#9.
- Of the CLs that Mihai has landed, none of them look suspicious.

I think that we'll need answers to the questions from c#23 to narrow down where/how to investigate further.

Re: c#24, Mihai reminded me that in the original buggy behavior, users could sign in as well; the problem was that users didn't have the account information needed any longer. So you can ignore c#24.

More questions:
* Did you test on the internal network (Google-A / GoogleGuest)?
* Did you see the sync confirmation dialog after finishing the Chrome sign-in flow?

To investigate we need the following:
* screenshot with chrome://signin-internals
* list with the extensions you are using (if you could find the one that leads to this loop, that would be even better)
* screenshot with the triangle error (click the avatar button, then take a screenshot)

Answering c#23 questions:

Yeah, I meant that a tab is repeatedly open with that url and chrome is unusable.

I've attached repro_from_clean.mp4, and in drive https://goo.gl/kzQMer. It captures the repro steps I described in c#21.

https://goo.gl/A4WzbD is a video that shows the auth error caused by signing in to that page. It also shows a repro when fixing the auth error. https://goo.gl/uGkHv8 shows that sometimes I can't actually get to the auth error step, not sure why.


https://goo.gl/y8Vw9T shows a full workaround by disabling some extensions, restarting chrome, and fixing the auth error.

https://goo.gl/aMEe3e shows that I can repro by enabling a single extension and waiting/causing its background page to activate. 

All videos are uploaded to https://drive.google.com/drive/folders/1TR4D1OsO5UynDa_OihQWpy96ho9SsXCH?usp=sharing

Deleted: repro_from_clean.mp4 5.9 MB

	repro_from_clean.mp4 5.9 MB View Download

Mihai and I did a lot of code inspection today. It turns out that the fix from c#9 left open an edge case: if the user successfully generated an access token from the refresh token, and then the request to mint the extension's access token from the former access token failed, we could still enter this loop. The reason is that the failure to mint the extension's access token does not cause Chrome to mark the refresh token as being in a bad state.

Our strong suspicion is that this behavior is in fact what is occurring in Jesse's first video, as evidenced by the fact that at no point in that video is a signin error manifested.

We are planning to fix this edge case with a CL that makes two changes:
(1) Changes the identity extension API impl to only show the signin UI once and then fail the request if it re-enters an error state.
(2) Changes the identity extension API impl to only show the signin UI if the user's primary account is in a bad state according to Chrome -- i.e. the user isn't signed in, doesn't have a refresh token, or the refresh token has an error.

The combination of these changes will be to (a) completely shut the door on the possibility of login UI loops, and (b) avoid extensions trying to reauth the user when the user's primary account is in fact in a good state as far as Chrome goes.

My suggestion would be that we target M63 with this fix. The remaining edge case described above is a strict subset of the original bug, as it is limited to cases where generating an access token from the refresh token succeeds BUT minting the extension's access token from that "primary" access token fails.

At this time, we do not know what is causing the behavior seen in Jesse's other videos, where signin triggers an immediate auth error.

Here are the signin-internals screenshots.

Deleted: 01_before_signin.png 109 KB

	01_before_signin.png 109 KB View Download

Deleted: 02_during_signin.png 277 KB

	02_during_signin.png 277 KB View Download

Deleted: 03_during_signin_syncing.png 283 KB

	03_during_signin_syncing.png 283 KB View Download

Deleted: 04_hit_signin_loop.png 259 KB

	04_hit_signin_loop.png 259 KB View Download

Deleted: 05_in_auth_error_state.png 218 KB

	05_in_auth_error_state.png 218 KB View Download

Thanks all for the quick work. Colin, I agree with your assessment in c#29 - let's target M63 with this fix and treat it as a P1.

It would be good to follow-up after that fix to understand why signing in is causing an auth error. That seems very backwards to me (:

There are 3 things in here that are important:
1. Sign-in loop triggered by extension code - Colin will follow-up on this bug with a fix.
2. Chrome always fails to get an access token for a scope https://www.googleapis.com/auth/chromewebstore.readonly (defined here https://cs.chromium.org/chromium/src/extensions/browser/updater/extension_downloader.cc?rcl=7e9641fa23646607d3349d5386aceafefc805d6d&l=94 ). I'll follow-up on this on a different bug with the OWNERS for this code.
3. Jesse gets invalid credentials responses from Gaia and this is why Chrome enters this auth error state. I think this requires server-side investigations. I'll follow-up with the Google auth team on an internal bug.

For the sign-in loop problem, https://chromium-review.googlesource.com/c/chromium/src/+/758647 is out for review now.

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/1046cead660a6090203c1ac55209b735db40c0a3

commit 1046cead660a6090203c1ac55209b735db40c0a3
Author: Colin Blundell <blundell@chromium.org>
Date: Tue Nov 14 09:22:55 2017

[Identity extension API] Solidify/unify policy on launching signin flow

In several parts of the implementation of the
chrome.identity.getAuthToken() extension API, it can launch a signin
flow. This signin flow gets launched in response to discovering that the
user isn't signed in or in response to an error occurring that indicates
that the user needs to reauthenticate.

However, the policies on launching the signin flow are flawed in ways
that negatively impact users:
(1) Several parts of the flow do not gate the number of times that they
launch a signin UI. This can result in the browser becoming unusable
if there are bugs that result in the flow looping without user
intervention (see  crbug.com/772122 ).
(2) The implementation launches a signin UI even in cases where the
user's refresh token is in a valid state, i.e., when the error is not
one that can be fixed by the user reauthenticating. This is
unnecessarily intrusive to the user.

This CL solidies and unifies all of these policies into one:
- Start a signin flow at most once and only if the user's refresh token
is not in a valid state. This is a behavioral change that eliminates
the possibility of hammering the user with login pages and avoids
asking the user to reauth if we don't think that the problem is due to
reauth.

This CL also adds several tests that fail without the policy changes
made in the CL.

Note that this change *is* a behavioral change. To test that this change
has not broken the common case of the user needing to reauth due to their
refresh token being in a bad state, do the following:
- Install an extension with the identity API in its manifest.
- Sign in to Chrome.
- Go to accounts.google.com and revoke your refresh token for Chrome there.
- Ensure that your account in Chrome goes into an error state (verify
that a red exclamation point appears in the top right by your account name).
- Go to chrome://extensions, enable developer mode, and inspect the background
page of the above app. At the JS console that that brings up, execute:
chrome.identity.getAuthToken({interactive: true}, (token) => {console.log(token);} )
- Verify that a signin prompt then opens.
- Sign in to Chrome.
- Verify that at the JS console referenced above a token gets printed.

Bug:  772122 
Change-Id: I779134f706ca1e693612ae3352a5c5dd0a3e468f
Reviewed-on: https://chromium-review.googlesource.com/758647
Reviewed-by: Ken Rockot <rockot@chromium.org>
Reviewed-by: Mihai Sardarescu <msarda@chromium.org>
Commit-Queue: Colin Blundell <blundell@chromium.org>
Cr-Commit-Position: refs/heads/master@{#516242}
[modify] https://crrev.com/1046cead660a6090203c1ac55209b735db40c0a3/chrome/browser/extensions/api/identity/identity_apitest.cc
[modify] https://crrev.com/1046cead660a6090203c1ac55209b735db40c0a3/chrome/browser/extensions/api/identity/identity_get_auth_token_function.cc
[modify] https://crrev.com/1046cead660a6090203c1ac55209b735db40c0a3/chrome/browser/extensions/api/identity/identity_get_auth_token_function.h

Requesting merging of https://chromium-review.googlesource.com/758647 to 63.

This bug requires manual review: DEPS changes referenced in bugdroid comments.
Please contact the milestone owner if you have questions.
Owners: cmasso@(Android), cmasso@(iOS), gkihumba@(ChromeOS), govind@(Desktop)

For more details visit https://www.chromium.org/issue-tracking/autotriage - Your friendly Sheriffbot

This bug exists on M61 and M62 and we're very close to M63 stable promotion. Can this wait until M64? If not, could you pls justify the merge?

Please note we're ONLY taking absolutely critical and safe merges in for M63 at this point.

The broken behavior here is pretty terrible. Chrome is essentially unusable, since it just infinitely loops opening up sign-in tabs.

Even though it may only be affecting a small population of users, I believe it's worth merging back ASAP due to the severity of the issue.

Ok, is the change listed at #34 well baked/verified in Canary, having enough automation tests coverage and safe to merge to M63?

Colin, I'll let you answer that question. It landed two days ago, so it has been on Canary for a day or two at this point. Does it have automation test coverage, Colin?

Also latest dev #64.0.3269.3 includes this fix. How is the change looking in dev?

Hi,

The change has good automated test coverage (added in the same CL). However, we don't have any means of checking the impact of these changes in production other than the presence or absence of user reports that they're getting into this state.

My expectation is that at this point, the loophole for getting into this state is primarily restricted to enterprise users, who presumably aren't running dev or canary. The most likely reason that we've seen for getting into this loophole is a server error on minting an access token for an extension that is *not* related to the state of the user's refresh token. The only example that we've concretely seen where that occurs is where an enterprise user has previously granted consent to an extension but the enterprise has subsequently changed their policies such that that it is no longer allowed for the user to grant such consent. In this particular case, the user can solve the problem by going to their Google account on the web (e.g., in a different browser or on a different device) and revoking permissions for that extension.

The primary danger that I see if we don't merge back is that we could get into a firedrill situation if this impacts an important enterprise. That said, I wouldn't characterize this change as "absolutely safe" given that it is making a behavioral change.

I had thought that we were farther away from promoting 63 -- I had mistakenly gotten the information that 63 stable release was December 12.

Given that this loophole has been present in both 61 and 62 and we've seen only one report of this particular edge case being triggered, I'm now inclined to avoid the cherrypick given Govind's requirement that the merge be "absolutely critical and safe". Eli, what do you think? Recall that all of the user reports described in the original bug would have been fixed by the CL that was merged to 62 in c#17. Here we're talking about cases like that described in c#21.

Alright, that's fine with me, I trust your judgement Colin. If this is really scoped to some extremely rare edge cases (e.g. the enterprise use case above), I'm fine with punting this to M64. Updating labels. Should we just mark this as Fixed now then?

Yes.

Issue 800567 has been merged into this issue.

We are seeing a spike of these errors in M63 (see  Issue 800567 ). We are still evaluating whether we need to have this bug merged back in M63 (CL https://chromium-review.googlesource.com/c/chromium/src/+/758647 to be more precise).


Currently, M63 users have the following options. 

1. Install and use Chrome beta M64 (until M64 is released)

2. Disable all extensions using the command line argument "--disable-extensions":
2.a. on macOS:
* Open terminal.
* Run "open /Applications/Google\ Chrome.app/ --args --disable-extensions"

2.b. on Windows (also explained http://techdows.com/2013/05/how-to-disable-all-extensions-when-you-cant-open-the-chrome-browser.html ):
* Right click on the Chrome shortcut on desktop and select “properties”
* In the ‘Target’ field at the end of Chrome.exe after giving a space Add --disable-extensions (two single hyphens) and click ‘Apply’ and ‘OK’ to apply the switch.
* Now you’ll be able to open Chrome browser with all extensions turned off.

Another easier workaround that works from cmasso@:
* Turn off network (e.g. Disable WiFi)
* Disable extensions from chrome://extensions

Issue 792588 has been merged into this issue.