Invalid refresh token does not trigger user to go through online signin |
||||
Issue descriptionRepro: 1. Create a user on a device then sign out; 2. Revoke the refresh token of that device, e.g. through "My Account" and remove the device; 3. Sign in on device via offline login; 4. Sign-out-and-back-in notification should pop up; 5. Click on the notification to sign out After step 5, we should force user to go through online sign-in to restore the refresh token. But it does not happen. User can still use offline login and there is no UI to send the user to online login automatically. As a result, all services that depend on the token would not work (e.g. sync would stop working) and user would see the sign-out-and-back-in notification all the time. One possible fix is to defer marking valid token flag in OAuth2LoginManager::OnRefreshTokenAvailable [1] until the token is verified because the loaded token could be a revoked/invalid one. [1]: https://cs.chromium.org/chromium/src/chrome/browser/chromeos/login/signin/oauth2_login_manager.cc?rcl=f7e84fa480be67a1fae168d7a9be8fd3c4896614&l=154
,
Sep 8 2017
Turns out that my test account somehow get "sync.suppress_start" set and sync is not started because of that. Once that is fixed, AuthSyncObserver gets the error and correctly flag the user. It seems that we have SigninErrorController that can be used to detect auth error independently of sync. Think we should switch to that instead.
,
Sep 26 2017
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/b29b6884019c4c7b1bc0e074617d0550b75164fc commit b29b6884019c4c7b1bc0e074617d0550b75164fc Author: Xiyuan Xia <xiyuan@chromium.org> Date: Tue Sep 26 16:31:08 2017 cros: AuthSyncObserver observes SigninErrorController - Make AuthSyncObserver observe SigninErrorController in addition to sync service so that auth errors caused by other services are caught as well. - Fix an edge case that OAuth2LoginManager marks valid oauth2 token when it thinks /MergeSession succeeds regardless of whether there is any auth error; Bug: 760610 Test: OAuth2Test.SetInvalidTokenStatus Change-Id: Idbaee9f7faa1aa2aff939a1540bfcfdd9f49c96c Reviewed-on: https://chromium-review.googlesource.com/658378 Commit-Queue: Xiyuan Xia <xiyuan@chromium.org> Reviewed-by: Achuith Bhandarkar <achuith@chromium.org> Cr-Commit-Position: refs/heads/master@{#504391} [modify] https://crrev.com/b29b6884019c4c7b1bc0e074617d0550b75164fc/chrome/browser/chromeos/login/session/user_session_manager.cc [modify] https://crrev.com/b29b6884019c4c7b1bc0e074617d0550b75164fc/chrome/browser/chromeos/login/signin/auth_sync_observer.cc [modify] https://crrev.com/b29b6884019c4c7b1bc0e074617d0550b75164fc/chrome/browser/chromeos/login/signin/auth_sync_observer.h [modify] https://crrev.com/b29b6884019c4c7b1bc0e074617d0550b75164fc/chrome/browser/chromeos/login/signin/auth_sync_observer_factory.cc [modify] https://crrev.com/b29b6884019c4c7b1bc0e074617d0550b75164fc/chrome/browser/chromeos/login/signin/oauth2_browsertest.cc [modify] https://crrev.com/b29b6884019c4c7b1bc0e074617d0550b75164fc/chrome/browser/chromeos/login/users/chrome_user_manager_impl.cc
,
Sep 26 2017
,
Sep 26 2017
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/849942232cf7a1c3548d8fbb826d1bc0c317fad5 commit 849942232cf7a1c3548d8fbb826d1bc0c317fad5 Author: Michael Giuffrida <michaelpg@chromium.org> Date: Tue Sep 26 19:09:19 2017 Revert "cros: AuthSyncObserver observes SigninErrorController" This reverts commit b29b6884019c4c7b1bc0e074617d0550b75164fc. Reason for revert: CrOS browser_tests failing in debug builds Bug: 768949 Original change's description: > cros: AuthSyncObserver observes SigninErrorController > > - Make AuthSyncObserver observe SigninErrorController in addition > to sync service so that auth errors caused by other services > are caught as well. > - Fix an edge case that OAuth2LoginManager marks valid oauth2 > token when it thinks /MergeSession succeeds regardless of > whether there is any auth error; > > Bug: 760610 > Test: OAuth2Test.SetInvalidTokenStatus > > Change-Id: Idbaee9f7faa1aa2aff939a1540bfcfdd9f49c96c > Reviewed-on: https://chromium-review.googlesource.com/658378 > Commit-Queue: Xiyuan Xia <xiyuan@chromium.org> > Reviewed-by: Achuith Bhandarkar <achuith@chromium.org> > Cr-Commit-Position: refs/heads/master@{#504391} TBR=xiyuan@chromium.org,achuith@chromium.org Change-Id: If3421269787c16081e1fd00382e5d92f73d8f082 No-Presubmit: true No-Tree-Checks: true No-Try: true Bug: 760610 Reviewed-on: https://chromium-review.googlesource.com/685398 Reviewed-by: Michael Giuffrida <michaelpg@chromium.org> Commit-Queue: Michael Giuffrida <michaelpg@chromium.org> Cr-Commit-Position: refs/heads/master@{#504447} [modify] https://crrev.com/849942232cf7a1c3548d8fbb826d1bc0c317fad5/chrome/browser/chromeos/login/session/user_session_manager.cc [modify] https://crrev.com/849942232cf7a1c3548d8fbb826d1bc0c317fad5/chrome/browser/chromeos/login/signin/auth_sync_observer.cc [modify] https://crrev.com/849942232cf7a1c3548d8fbb826d1bc0c317fad5/chrome/browser/chromeos/login/signin/auth_sync_observer.h [modify] https://crrev.com/849942232cf7a1c3548d8fbb826d1bc0c317fad5/chrome/browser/chromeos/login/signin/auth_sync_observer_factory.cc [modify] https://crrev.com/849942232cf7a1c3548d8fbb826d1bc0c317fad5/chrome/browser/chromeos/login/signin/oauth2_browsertest.cc [modify] https://crrev.com/849942232cf7a1c3548d8fbb826d1bc0c317fad5/chrome/browser/chromeos/login/users/chrome_user_manager_impl.cc
,
Sep 26 2017
,
Sep 29 2017
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/17f47a1f6763a2e1af29c97f10832276f40abcce commit 17f47a1f6763a2e1af29c97f10832276f40abcce Author: Xiyuan Xia <xiyuan@chromium.org> Date: Fri Sep 29 16:06:36 2017 Reland "cros: AuthSyncObserver observes SigninErrorController" - Make AuthSyncObserver observe SigninErrorController in addition to sync service so that auth errors caused by other services are caught as well. - Fix an edge case that OAuth2LoginManager marks valid oauth2 token when it thinks /MergeSession succeeds regardless of whether there is any auth error; Additional changes of reland: - Update FakeGaia to mint tokens for any scopes when there is an AccessTokenInfo with no |scopes| defined; - Update the failed tests to use that so that all auth requests succeed and AuthSyncObserver does not set invalid token status unexpectedly; Bug: 760610 , 768949 Test: OAuth2Test.SetInvalidTokenStatus Change-Id: I8173ceec02ba036820439b7a66f09cac40f27d3d Reviewed-on: https://chromium-review.googlesource.com/685795 Reviewed-by: Mihai Sardarescu <msarda@chromium.org> Reviewed-by: Achuith Bhandarkar <achuith@chromium.org> Commit-Queue: Xiyuan Xia <xiyuan@chromium.org> Cr-Commit-Position: refs/heads/master@{#505381} [modify] https://crrev.com/17f47a1f6763a2e1af29c97f10832276f40abcce/chrome/browser/chromeos/login/saml/saml_browsertest.cc [modify] https://crrev.com/17f47a1f6763a2e1af29c97f10832276f40abcce/chrome/browser/chromeos/login/session/user_session_manager.cc [modify] https://crrev.com/17f47a1f6763a2e1af29c97f10832276f40abcce/chrome/browser/chromeos/login/signin/auth_sync_observer.cc [modify] https://crrev.com/17f47a1f6763a2e1af29c97f10832276f40abcce/chrome/browser/chromeos/login/signin/auth_sync_observer.h [modify] https://crrev.com/17f47a1f6763a2e1af29c97f10832276f40abcce/chrome/browser/chromeos/login/signin/auth_sync_observer_factory.cc [modify] https://crrev.com/17f47a1f6763a2e1af29c97f10832276f40abcce/chrome/browser/chromeos/login/signin/oauth2_browsertest.cc [modify] https://crrev.com/17f47a1f6763a2e1af29c97f10832276f40abcce/chrome/browser/chromeos/login/test/oobe_base_test.cc [modify] https://crrev.com/17f47a1f6763a2e1af29c97f10832276f40abcce/chrome/browser/chromeos/login/test/oobe_base_test.h [modify] https://crrev.com/17f47a1f6763a2e1af29c97f10832276f40abcce/chrome/browser/chromeos/login/users/chrome_user_manager_impl.cc [modify] https://crrev.com/17f47a1f6763a2e1af29c97f10832276f40abcce/chrome/browser/chromeos/policy/login_policy_test_base.cc [modify] https://crrev.com/17f47a1f6763a2e1af29c97f10832276f40abcce/google_apis/gaia/fake_gaia.cc [modify] https://crrev.com/17f47a1f6763a2e1af29c97f10832276f40abcce/google_apis/gaia/fake_gaia.h
,
Sep 29 2017
Edge case P2 but CL could have complications. Put to M63 to give it more time to bake. |
||||
►
Sign in to add a comment |
||||
Comment 1 by xiy...@chromium.org
, Aug 30 2017