Chromium Code Reviews
chromiumcodereview-hr@appspot.gserviceaccount.com (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out
(41)

Issue 10830239: [sync] Auto-create credential cache for users who are already signed in and go on to upgrade Chrome (Closed)

Created:
8 years, 4 months ago by Raghu Simha
Modified:
8 years, 4 months ago
CC:
chromium-reviews, ncarter (slow), akalin, tim (not reviewing)
Visibility:
Public.

Description

[sync] Auto-create credential cache for users who are already signed in and go on to upgrade Chrome CredentialCacheService on Windows 8 writes to the local credential cache when a user actively signs in / reconfigures sync. Existing cached credentials are not updated when Chrome is merely restarted after the user is signed in. This causes a problem when a user is already signed in, and then upgrades (and restarts) chrome from a version that didn't originally support credential caching. In such cases, we never end up caching credentials, and therefore, the user will have to sign in separately to Metro and Desktop. This patch contains the following changes: 1) Adds logic to auto-heal already-signed-in users who upgrade from older versions, by writing existing credentials to the local cache if during restart, we notice that there is no local cache file, and the user is already signed in to sync. 2) Simplifies the logic around checking if an alternate credential cache file exists, and only then initializing |alternate_store_|. It turns out that JsonPrefStore returns a useful PrefReadError field, and there is no need for CCS to do funky stuff on the FILE thread. 3) Simplifies OnInitialzationCompleted, which was being used to observe two separate JsonPrefStores. Instead of having CCS be a PrefStore::Observer, we now use two helper classes -- LocalStoreObserver and AlternateStoreObserver to cleanly divide what is done when each pref store is initialized. 4) Updates prefs::kGoogleServicesUsername by listening to the notifications NOTIFICATION_GOOGLE_SIGNED_OUT and NOTIFICATION_GOOGLE_SIGNIN_SUCCESSFUL instead of directly listening to the pref change. 5) Fixes a stray instance where we were accessing the gaia username pref via SyncPrefs instead of via the SigninManager. BUG=141555 TEST=Sign in to chrome, exit the browser, and delete "Sync Credentials" from the default profile directory. Restart Chrome and make sure that the credential cache file is freshly written using existing sync credentials. Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=151364

Patch Set 1 #

Total comments: 1

Patch Set 2 : Fix unit test, etc. #

Total comments: 10

Patch Set 3 : CR Feedback; alphabetize notifications #

Unified diffs Side-by-side diffs Delta from patch set Stats (+337 lines, -236 lines) Patch
M chrome/browser/signin/token_service.h View 1 2 1 chunk +4 lines, -0 lines 0 comments Download
M chrome/browser/sync/credential_cache_service_win.h View 7 chunks +66 lines, -25 lines 0 comments Download
M chrome/browser/sync/credential_cache_service_win.cc View 1 2 12 chunks +267 lines, -208 lines 0 comments Download
M chrome/browser/sync/credential_cache_service_win_unittest.cc View 1 1 chunk +0 lines, -3 lines 0 comments Download

Messages

Total messages: 6 (0 generated)
Raghu Simha
Drew, please review. Thanks. http://codereview.chromium.org/10830239/diff/1/chrome/browser/sync/credential_cache_service_win.cc File chrome/browser/sync/credential_cache_service_win.cc (right): http://codereview.chromium.org/10830239/diff/1/chrome/browser/sync/credential_cache_service_win.cc#newcode153 chrome/browser/sync/credential_cache_service_win.cc:153: void CredentialCacheService::ReadCachedCredentialsFromAlternateProfile() { ReadCachedCredentialsFromAlternateProfile() was ...
8 years, 4 months ago (2012-08-09 06:10:40 UTC) #1
Raghu Simha
Ping :)
8 years, 4 months ago (2012-08-10 17:57:55 UTC) #2
Raghu Simha
Ping :)
8 years, 4 months ago (2012-08-10 17:57:56 UTC) #3
Andrew T Wilson (Slow)
Sorry for the delay. http://codereview.chromium.org/10830239/diff/4002/chrome/browser/signin/token_service.h File chrome/browser/signin/token_service.h (right): http://codereview.chromium.org/10830239/diff/4002/chrome/browser/signin/token_service.h#newcode171 chrome/browser/signin/token_service.h:171: GaiaAuthConsumer::ClientLoginResult credentials() const { Should ...
8 years, 4 months ago (2012-08-11 01:40:20 UTC) #4
Raghu Simha
Drew, thanks for the feedback. PTAL. http://codereview.chromium.org/10830239/diff/4002/chrome/browser/signin/token_service.h File chrome/browser/signin/token_service.h (right): http://codereview.chromium.org/10830239/diff/4002/chrome/browser/signin/token_service.h#newcode171 chrome/browser/signin/token_service.h:171: GaiaAuthConsumer::ClientLoginResult credentials() const ...
8 years, 4 months ago (2012-08-13 19:48:29 UTC) #5
Andrew T Wilson (Slow)
8 years, 4 months ago (2012-08-13 21:33:26 UTC) #6
lgtm

Powered by Google App Engine
This is Rietveld 408576698