Handling of CREDENTIALS_REJECTED_BY_CLIENT in Sync |
|||||
Issue descriptionfrom treib@ "I've been looking over Sync's auth code, and was wondering whether the fix for crbug.com/824791 actually works correctly. Right now, the check for CREDENTIALS_REJECTED_BY_CLIENT happens in OnRefreshTokensLoaded, which (outside of tests) only gets called during startup. So if Sync is running and I sign out of Gmail, then the access token will *not* get cleared immediately. Rather, that'll only happen during the next startup, where we don't have an access token anyway (because we don't persist it)."
,
May 22 2018
I think the fix will be to move this code from SyncAuthManager::OnRefreshTokensLoaded to SyncAuthManager::OnRefreshTokenAvailable. I can handle that if you want (since I'm the one who broke it :).)
,
May 22 2018
Yes, that probably is a good fix. Feel free to take the bug!
,
May 22 2018
,
May 22 2018
,
May 22 2018
Pending CL: https://crrev.com/c/1068178 I hope to get it in before BP on Thursday.
,
May 23 2018
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/3a3e7093861f3d55dba3d077cf5d3b825ad5d50d commit 3a3e7093861f3d55dba3d077cf5d3b825ad5d50d Author: Marc Treib <treib@chromium.org> Date: Wed May 23 18:06:49 2018 SyncAuthManager: fix handling of CREDENTIALS_REJECTED_BY_CLIENT error On a CREDENTIALS_REJECTED_BY_CLIENT auth error, any access token must be dropped immediately. The original implementation of this (https://crrev.com/c/1007277) relied on the fact that ProfileSyncService::OnRefreshTokensLoaded was called from ProfileSyncService::OnRefreshTokenAvailable. This was changed by a recent refactoring, hence breaking the fix. This CL fixes it again by moving the CREDENTIALS_REJECTED_BY_CLIENT handling into OnRefreshTokenAvailable, and also updates the test to better resemble the real code, so that it would have caught this regression. Bug: 845502 Change-Id: I991c15fbfea01053a92fcba6b9694bea8be72e3b Reviewed-on: https://chromium-review.googlesource.com/1068178 Reviewed-by: Mihai Sardarescu <msarda@chromium.org> Reviewed-by: David Roger <droger@chromium.org> Commit-Queue: Marc Treib <treib@chromium.org> Cr-Commit-Position: refs/heads/master@{#561155} [modify] https://crrev.com/3a3e7093861f3d55dba3d077cf5d3b825ad5d50d/chrome/browser/signin/mutable_profile_oauth2_token_service_delegate.cc [modify] https://crrev.com/3a3e7093861f3d55dba3d077cf5d3b825ad5d50d/chrome/browser/signin/mutable_profile_oauth2_token_service_delegate.h [modify] https://crrev.com/3a3e7093861f3d55dba3d077cf5d3b825ad5d50d/components/browser_sync/profile_sync_service_unittest.cc [modify] https://crrev.com/3a3e7093861f3d55dba3d077cf5d3b825ad5d50d/components/browser_sync/sync_auth_manager.cc [modify] https://crrev.com/3a3e7093861f3d55dba3d077cf5d3b825ad5d50d/google_apis/gaia/fake_oauth2_token_service_delegate.cc [modify] https://crrev.com/3a3e7093861f3d55dba3d077cf5d3b825ad5d50d/google_apis/gaia/oauth2_token_service_delegate.cc [modify] https://crrev.com/3a3e7093861f3d55dba3d077cf5d3b825ad5d50d/google_apis/gaia/oauth2_token_service_delegate.h
,
May 23 2018
,
May 23 2018
M در تاریخ ۲۳ مهٔ ۲۰۱۸ ۱۰:۴۰ ب.ظ، "tr… via monorail" < monorail+v2.3203352690@chromium.org> نوشت:
,
May 28 2018
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/820a8547a9bfce2b145be941e1130a419bc667a1 commit 820a8547a9bfce2b145be941e1130a419bc667a1 Author: Marc Treib <treib@chromium.org> Date: Mon May 28 10:48:33 2018 SyncAuthManager: set auth error on CREDENTIALS_REJECTED_BY_CLIENT Before this CL, the CREDENTIALS_REJECTED_BY_CLIENT error was not set as the last auth error in SyncService/SyncAuthManager, so clients that check SyncService::GetAuthError() would not know of it, and might wrongly think that Sync was still active. In practice, Sync would usually try to fetch a new access token in this situation, which would of course fail, so the error would make it there eventually. But it seems better to explicitly set it immediately. Bug: 842697 , 845502 Change-Id: I25386959647e57a619ef2207098212c0f7c1af67 Reviewed-on: https://chromium-review.googlesource.com/1071747 Reviewed-by: David Roger <droger@chromium.org> Commit-Queue: Marc Treib <treib@chromium.org> Cr-Commit-Position: refs/heads/master@{#562229} [modify] https://crrev.com/820a8547a9bfce2b145be941e1130a419bc667a1/components/browser_sync/profile_sync_service_unittest.cc [modify] https://crrev.com/820a8547a9bfce2b145be941e1130a419bc667a1/components/browser_sync/sync_auth_manager.cc |
|||||
►
Sign in to add a comment |
|||||
Comment 1 by droger@chromium.org
, May 22 2018