diff options
author | acleung@chromium.org <acleung@chromium.org@0039d316-1c4b-4281-b951-d872f2087c98> | 2014-04-25 03:09:04 +0000 |
---|---|---|
committer | acleung@chromium.org <acleung@chromium.org@0039d316-1c4b-4281-b951-d872f2087c98> | 2014-04-25 03:09:04 +0000 |
commit | 26f9ed0135aca70e48ea8107310a144af0541898 (patch) | |
tree | 61d5d198bf0d2688450b304aec2b35ec86f1926a | |
parent | 0ea3c436c962779674fb0627879cd21addd03113 (diff) | |
download | chromium_src-26f9ed0135aca70e48ea8107310a144af0541898.zip chromium_src-26f9ed0135aca70e48ea8107310a144af0541898.tar.gz chromium_src-26f9ed0135aca70e48ea8107310a144af0541898.tar.bz2 |
Calls FireRefreshTokenRevoked if an account is removed.
We cannot reply on periodic reconcile actions to remove accounts from cookies. The reason is that we actually *don't* do it at that stage for Android. Instead, we are going to keep a list of accounts last seen by Chrome and store that in a shared pref. We will come to the old list to the new list and call FireRefreshTokenRevoked accordingingly.
BUG=305086
Review URL: https://codereview.chromium.org/213823004
git-svn-id: svn://svn.chromium.org/chrome/trunk/src@266108 0039d316-1c4b-4281-b951-d872f2087c98
7 files changed, 259 insertions, 39 deletions
diff --git a/chrome/android/java/src/org/chromium/chrome/browser/signin/OAuth2TokenService.java b/chrome/android/java/src/org/chromium/chrome/browser/signin/OAuth2TokenService.java index 2e5daa8..9adf0cc 100644 --- a/chrome/android/java/src/org/chromium/chrome/browser/signin/OAuth2TokenService.java +++ b/chrome/android/java/src/org/chromium/chrome/browser/signin/OAuth2TokenService.java @@ -7,8 +7,11 @@ package org.chromium.chrome.browser.signin; import android.accounts.Account; import android.app.Activity; import android.content.Context; +import android.preference.PreferenceManager; import android.util.Log; +import com.google.common.annotations.VisibleForTesting; + import org.chromium.base.CalledByNative; import org.chromium.base.ObserverList; import org.chromium.base.ThreadUtils; @@ -16,9 +19,12 @@ import org.chromium.chrome.browser.profiles.Profile; import org.chromium.sync.signin.AccountManagerHelper; import org.chromium.sync.signin.ChromeSigninController; +import java.util.Arrays; import java.util.concurrent.Semaphore; import java.util.concurrent.TimeUnit; import java.util.concurrent.atomic.AtomicReference; +import java.util.HashSet; +import java.util.Set; import javax.annotation.Nullable; @@ -33,6 +39,9 @@ public final class OAuth2TokenService { private static final String TAG = "OAuth2TokenService"; + @VisibleForTesting + public static final String STORED_ACCOUNTS_KEY = "google.services.stored_accounts"; + public interface OAuth2TokenServiceObserver { void onRefreshTokenAvailable(Account account); void onRefreshTokenRevoked(Account account); @@ -86,16 +95,28 @@ public final class OAuth2TokenService { } /** - * Called by native to list the accounts with OAuth2 refresh tokens. + * Called by native to list the activite accounts in the OS. */ + @VisibleForTesting @CalledByNative - public static String[] getAccounts(Context context) { + public static String[] getSystemAccounts(Context context) { AccountManagerHelper accountManagerHelper = AccountManagerHelper.get(context); java.util.List<String> accountNames = accountManagerHelper.getGoogleAccountNames(); return accountNames.toArray(new String[accountNames.size()]); } /** + * Called by native to list the accounts with OAuth2 refresh tokens. + * This can differ from getSystemAccounts as the user add/remove accounts + * from the OS. validateAccounts should be called to keep these two + * in sync. + */ + @CalledByNative + public static String[] getAccounts(Context context) { + return getStoredAccounts(context); + } + + /** * Called by native to retrieve OAuth2 tokens. * * @param username The native username (full address). @@ -202,9 +223,7 @@ public final class OAuth2TokenService { ThreadUtils.assertOnUiThread(); String currentlySignedInAccount = ChromeSigninController.get(context).getSignedInAccountName(); - String[] accounts = getAccounts(context); - nativeValidateAccounts( - mNativeProfileOAuth2TokenService, accounts, currentlySignedInAccount); + nativeValidateAccounts(mNativeProfileOAuth2TokenService, currentlySignedInAccount); } /** @@ -262,12 +281,26 @@ public final class OAuth2TokenService { } } + private static String[] getStoredAccounts(Context context) { + Set<String> accounts = + PreferenceManager.getDefaultSharedPreferences(context) + .getStringSet(STORED_ACCOUNTS_KEY, null); + return accounts == null ? new String[]{} : accounts.toArray(new String[accounts.size()]); + } + + @CalledByNative + private static void saveStoredAccounts(Context context, String[] accounts) { + Set<String> set = new HashSet<String>(Arrays.asList(accounts)); + PreferenceManager.getDefaultSharedPreferences(context).edit(). + putStringSet(STORED_ACCOUNTS_KEY, set).apply(); + } + private static native Object nativeGetForProfile(Profile profile); private static native void nativeOAuth2TokenFetched( String authToken, boolean result, long nativeCallback); private native void nativeValidateAccounts( long nativeAndroidProfileOAuth2TokenService, - String[] accounts, String currentlySignedInAccount); + String currentlySignedInAccount); private native void nativeFireRefreshTokenAvailableFromJava( long nativeAndroidProfileOAuth2TokenService, String accountName); private native void nativeFireRefreshTokenRevokedFromJava( diff --git a/chrome/android/javatests/src/org/chromium/chrome/browser/signin/OAuth2TokenServiceIntegrationTest.java b/chrome/android/javatests/src/org/chromium/chrome/browser/signin/OAuth2TokenServiceIntegrationTest.java index c4067ce..1ba655e 100644 --- a/chrome/android/javatests/src/org/chromium/chrome/browser/signin/OAuth2TokenServiceIntegrationTest.java +++ b/chrome/android/javatests/src/org/chromium/chrome/browser/signin/OAuth2TokenServiceIntegrationTest.java @@ -201,6 +201,122 @@ public class OAuth2TokenServiceIntegrationTest extends ChromeShellTestBase { @MediumTest @UiThreadTest + public void testValidateAccountsSingleAccountWithoutChanges() { + // Add account. + mAccountManager.addAccountHolderExplicitly(TEST_ACCOUNT_HOLDER_1); + + // Mark user as signed in. + mChromeSigninController.setSignedInAccountName(TEST_ACCOUNT1.name); + + // Run one validation. + mOAuth2TokenService.validateAccounts(mContext); + assertEquals(1, mObserver.getAvailableCallCount()); + assertEquals(0, mObserver.getRevokedCallCount()); + assertEquals(0, mObserver.getLoadedCallCount()); + + // Re-run validation. + mOAuth2TokenService.validateAccounts(mContext); + assertEquals(2, mObserver.getAvailableCallCount()); + assertEquals(0, mObserver.getRevokedCallCount()); + assertEquals(0, mObserver.getLoadedCallCount()); + } + + @MediumTest + @UiThreadTest + public void testValidateAccountsSingleAccountThenAddOne() { + // Add account. + mAccountManager.addAccountHolderExplicitly(TEST_ACCOUNT_HOLDER_1); + + // Mark user as signed in. + mChromeSigninController.setSignedInAccountName(TEST_ACCOUNT1.name); + + // Run one validation. + mOAuth2TokenService.validateAccounts(mContext); + assertEquals(1, mObserver.getAvailableCallCount()); + assertEquals(0, mObserver.getRevokedCallCount()); + assertEquals(0, mObserver.getLoadedCallCount()); + + // Add another account. + mAccountManager.addAccountHolderExplicitly(TEST_ACCOUNT_HOLDER_2); + + // Re-run validation. + mOAuth2TokenService.validateAccounts(mContext); + assertEquals(3, mObserver.getAvailableCallCount()); + assertEquals(0, mObserver.getRevokedCallCount()); + assertEquals(0, mObserver.getLoadedCallCount()); + } + + @MediumTest + @UiThreadTest + public void testValidateAccountsTwoAccountsThenRemoveOne() { + // Add accounts. + mAccountManager.addAccountHolderExplicitly(TEST_ACCOUNT_HOLDER_1); + mAccountManager.addAccountHolderExplicitly(TEST_ACCOUNT_HOLDER_2); + + // Mark user as signed in. + mChromeSigninController.setSignedInAccountName(TEST_ACCOUNT1.name); + + // Run one validation. + mOAuth2TokenService.validateAccounts(mContext); + assertEquals(2, mObserver.getAvailableCallCount()); + + mAccountManager.removeAccountHolderExplicitly(TEST_ACCOUNT_HOLDER_2); + mOAuth2TokenService.validateAccounts(mContext); + + assertEquals(3, mObserver.getAvailableCallCount()); + assertEquals(1, mObserver.getRevokedCallCount()); + assertEquals(0, mObserver.getLoadedCallCount()); + } + + @MediumTest + @UiThreadTest + public void testValidateAccountsTwoAccountsThenRemoveAll() { + // Add accounts. + mAccountManager.addAccountHolderExplicitly(TEST_ACCOUNT_HOLDER_1); + mAccountManager.addAccountHolderExplicitly(TEST_ACCOUNT_HOLDER_2); + + // Mark user as signed in. + mChromeSigninController.setSignedInAccountName(TEST_ACCOUNT1.name); + + mOAuth2TokenService.validateAccounts(mContext); + assertEquals(2, mObserver.getAvailableCallCount()); + + // Remove all. + mAccountManager.removeAccountHolderExplicitly(TEST_ACCOUNT_HOLDER_1); + mAccountManager.removeAccountHolderExplicitly(TEST_ACCOUNT_HOLDER_2); + + // Re-validate and run checks. + mOAuth2TokenService.validateAccounts(mContext); + assertEquals(2, mObserver.getRevokedCallCount()); + assertEquals(0, mObserver.getLoadedCallCount()); + } + + @MediumTest + @UiThreadTest + public void testValidateAccountsTwoAccountsThenRemoveAllSignOut() { + // Add accounts. + mAccountManager.addAccountHolderExplicitly(TEST_ACCOUNT_HOLDER_1); + mAccountManager.addAccountHolderExplicitly(TEST_ACCOUNT_HOLDER_2); + + // Mark user as signed in. + mChromeSigninController.setSignedInAccountName(TEST_ACCOUNT1.name); + + mOAuth2TokenService.validateAccounts(mContext); + assertEquals(2, mObserver.getAvailableCallCount()); + + // Remove all. + mChromeSigninController.clearSignedInUser(); + mAccountManager.removeAccountHolderExplicitly(TEST_ACCOUNT_HOLDER_1); + mAccountManager.removeAccountHolderExplicitly(TEST_ACCOUNT_HOLDER_2); + + // Re-validate and run checks. + mOAuth2TokenService.validateAccounts(mContext); + assertEquals(2, mObserver.getRevokedCallCount()); + assertEquals(0, mObserver.getLoadedCallCount()); + } + + @MediumTest + @UiThreadTest public void testValidateAccountsTwoAccountsRegisteredAndOneSignedIn() { // Add accounts. mAccountManager.addAccountHolderExplicitly(TEST_ACCOUNT_HOLDER_1); diff --git a/chrome/android/javatests/src/org/chromium/chrome/browser/signin/OAuth2TokenServiceTest.java b/chrome/android/javatests/src/org/chromium/chrome/browser/signin/OAuth2TokenServiceTest.java index 78baa84..c80ffd7 100644 --- a/chrome/android/javatests/src/org/chromium/chrome/browser/signin/OAuth2TokenServiceTest.java +++ b/chrome/android/javatests/src/org/chromium/chrome/browser/signin/OAuth2TokenServiceTest.java @@ -46,9 +46,12 @@ public class OAuth2TokenServiceTest extends InstrumentationTestCase { AccountHolder accountHolder1 = AccountHolder.create().account(account1).build(); mAccountManager.addAccountHolderExplicitly(accountHolder1); + String[] sysAccounts = OAuth2TokenService.getSystemAccounts(mContext); + assertEquals("There should be one registered account", 1, sysAccounts.length); + assertEquals("The account should be " + account1, account1.name, sysAccounts[0]); + String[] accounts = OAuth2TokenService.getAccounts(mContext); - assertEquals("There should be one registered account", 1, accounts.length); - assertEquals("The account should be " + account1, account1.name, accounts[0]); + assertEquals("There should be zero registered account", 0, accounts.length); } @SmallTest @@ -61,12 +64,15 @@ public class OAuth2TokenServiceTest extends InstrumentationTestCase { AccountHolder accountHolder2 = AccountHolder.create().account(account2).build(); mAccountManager.addAccountHolderExplicitly(accountHolder2); - String[] accounts = OAuth2TokenService.getAccounts(mContext); - assertEquals("There should be one registered account", 2, accounts.length); + String[] sysAccounts = OAuth2TokenService.getSystemAccounts(mContext); + assertEquals("There should be one registered account", 2, sysAccounts.length); assertTrue("The list should contain " + account1, - Arrays.asList(accounts).contains(account1.name)); + Arrays.asList(sysAccounts).contains(account1.name)); assertTrue("The list should contain " + account2, - Arrays.asList(accounts).contains(account2.name)); + Arrays.asList(sysAccounts).contains(account2.name)); + + String[] accounts = OAuth2TokenService.getAccounts(mContext); + assertEquals("There should be zero registered account", 0, accounts.length); } @SmallTest diff --git a/chrome/browser/android/signin/signin_manager_android.cc b/chrome/browser/android/signin/signin_manager_android.cc index 43af5fa..153934a 100644 --- a/chrome/browser/android/signin/signin_manager_android.cc +++ b/chrome/browser/android/signin/signin_manager_android.cc @@ -225,8 +225,7 @@ void SigninManagerAndroid::LogInSignedInUser(JNIEnv* env, jobject obj) { profile_); const std::string& primary_acct = signin_manager->GetAuthenticatedAccountId(); - const std::vector<std::string>& ids = token_service->GetAccounts(); - token_service->ValidateAccounts(primary_acct, ids); + token_service->ValidateAccounts(primary_acct); } else { DVLOG(1) << "SigninManagerAndroid::LogInSignedInUser " diff --git a/chrome/browser/signin/android_profile_oauth2_token_service.cc b/chrome/browser/signin/android_profile_oauth2_token_service.cc index 71fe1c3..c88af81 100644 --- a/chrome/browser/signin/android_profile_oauth2_token_service.cc +++ b/chrome/browser/signin/android_profile_oauth2_token_service.cc @@ -98,6 +98,18 @@ std::vector<std::string> AndroidProfileOAuth2TokenService::GetAccounts() { return accounts; } +std::vector<std::string> AndroidProfileOAuth2TokenService::GetSystemAccounts() { + std::vector<std::string> accounts; + JNIEnv* env = AttachCurrentThread(); + ScopedJavaLocalRef<jobjectArray> j_accounts = + Java_OAuth2TokenService_getSystemAccounts( + env, base::android::GetApplicationContext()); + base::android::AppendJavaStringArrayToStringVector(env, + j_accounts.obj(), + &accounts); + return accounts; +} + void AndroidProfileOAuth2TokenService::FetchOAuth2Token( RequestImpl* request, const std::string& account_id, @@ -157,43 +169,74 @@ void AndroidProfileOAuth2TokenService::InvalidateOAuth2Token( j_access_token.obj()); } -void AndroidProfileOAuth2TokenService::ValidateAccounts(JNIEnv* env, - jobject obj, - jobjectArray accounts, - jstring j_current_acc) { - std::vector<std::string> account_ids; - base::android::AppendJavaStringArrayToStringVector(env, - accounts, - &account_ids); +void AndroidProfileOAuth2TokenService::ValidateAccounts( + JNIEnv* env, + jobject obj, + jstring j_current_acc) { std::string signed_in_account = ConvertJavaStringToUTF8(env, j_current_acc); - ValidateAccounts(signed_in_account, account_ids); + ValidateAccounts(signed_in_account); } void AndroidProfileOAuth2TokenService::ValidateAccounts( - const std::string& signed_in_account, - const std::vector<std::string>& account_ids) { - if (signed_in_account.empty()) - return; + const std::string& signed_in_account) { + std::vector<std::string> prev_ids = GetAccounts(); + std::vector<std::string> curr_ids = GetSystemAccounts(); + + if (!ValidateAccounts(signed_in_account, prev_ids, curr_ids)) { + curr_ids.clear(); + } - if (std::find(account_ids.begin(), - account_ids.end(), - signed_in_account) != account_ids.end()) { - // Currently signed in account still exists among accounts on system. - std::vector<std::string> ids = GetAccounts(); + JNIEnv* env = AttachCurrentThread(); + ScopedJavaLocalRef<jobjectArray> java_accounts( + base::android::ToJavaArrayOfStrings(env, curr_ids)); + Java_OAuth2TokenService_saveStoredAccounts( + env, base::android::GetApplicationContext(), java_accounts.obj()); +} + +bool AndroidProfileOAuth2TokenService::ValidateAccounts( + const std::string& signed_in_account, + const std::vector<std::string>& prev_account_ids, + const std::vector<std::string>& curr_account_ids) { + if (std::find(curr_account_ids.begin(), + curr_account_ids.end(), + signed_in_account) != curr_account_ids.end()) { + // Test to see if an account is removed from the Android AccountManager. + // If so, invoke FireRefreshTokenRevoked to notify the reconcilor. + for (std::vector<std::string>::const_iterator it = prev_account_ids.begin(); + it != prev_account_ids.end(); it++) { + if (*it == signed_in_account) + continue; + + if (std::find(curr_account_ids.begin(), + curr_account_ids.end(), + *it) == curr_account_ids.end()) { + FireRefreshTokenRevoked(*it); + } + } // Always fire the primary signed in account first. FireRefreshTokenAvailable(signed_in_account); - for (std::vector<std::string>::iterator it = ids.begin(); - it != ids.end(); it++) { + for (std::vector<std::string>::const_iterator it = curr_account_ids.begin(); + it != curr_account_ids.end(); it++) { if (*it != signed_in_account) { FireRefreshTokenAvailable(*it); } } + return true; } else { // Currently signed in account does not any longer exist among accounts on - // system. - FireRefreshTokenRevoked(signed_in_account); + // system together with all other accounts. + if (!signed_in_account.empty()) { + FireRefreshTokenRevoked(signed_in_account); + } + for (std::vector<std::string>::const_iterator it = prev_account_ids.begin(); + it != prev_account_ids.end(); it++) { + if (*it == signed_in_account) + continue; + FireRefreshTokenRevoked(*it); + } + return false; } } @@ -252,6 +295,14 @@ void AndroidProfileOAuth2TokenService::FireRefreshTokensLoaded() { env, java_ref_.obj()); } +void AndroidProfileOAuth2TokenService::RevokeAllCredentials() { + std::vector<std::string> accounts = GetAccounts(); + for (std::vector<std::string>::iterator it = accounts.begin(); + it != accounts.end(); it++) { + FireRefreshTokenRevoked(*it); + } +} + // Called from Java when fetching of an OAuth2 token is finished. The // |authToken| param is only valid when |result| is true. void OAuth2TokenFetched(JNIEnv* env, jclass clazz, diff --git a/chrome/browser/signin/android_profile_oauth2_token_service.h b/chrome/browser/signin/android_profile_oauth2_token_service.h index a641e3e..d1de5a6 100644 --- a/chrome/browser/signin/android_profile_oauth2_token_service.h +++ b/chrome/browser/signin/android_profile_oauth2_token_service.h @@ -42,15 +42,20 @@ class AndroidProfileOAuth2TokenService : public ProfileOAuth2TokenService { // Lists account IDs of all accounts with a refresh token. virtual std::vector<std::string> GetAccounts() OVERRIDE; + // Lists account at the OS level. + std::vector<std::string> GetSystemAccounts(); + void ValidateAccounts(JNIEnv* env, jobject obj, - jobjectArray accounts, jstring current_account); // Takes a the signed in sync account as well as all the other // android account ids and check the token status of each. - void ValidateAccounts(const std::string& signed_in_account, - const std::vector<std::string>& account_ids); + void ValidateAccounts(const std::string& signed_in_account); + + bool ValidateAccounts(const std::string& signed_in_account, + const std::vector<std::string>& prev_account_ids, + const std::vector<std::string>& curr_account_ids); // Triggers a notification to all observers of the OAuth2TokenService that a // refresh token is now available. This may cause observers to retry @@ -67,6 +72,10 @@ class AndroidProfileOAuth2TokenService : public ProfileOAuth2TokenService { // refresh tokens have now been loaded. virtual void FireRefreshTokensLoadedFromJava(JNIEnv* env, jobject obj); + // Overridden from OAuth2TokenService to complete signout of all + // OA2TService aware accounts. + virtual void RevokeAllCredentials() OVERRIDE; + protected: friend class ProfileOAuth2TokenServiceFactory; AndroidProfileOAuth2TokenService(); diff --git a/sync/test/android/javatests/src/org/chromium/sync/test/util/MockAccountManager.java b/sync/test/android/javatests/src/org/chromium/sync/test/util/MockAccountManager.java index 6f575e2..89410cf 100644 --- a/sync/test/android/javatests/src/org/chromium/sync/test/util/MockAccountManager.java +++ b/sync/test/android/javatests/src/org/chromium/sync/test/util/MockAccountManager.java @@ -141,6 +141,12 @@ public class MockAccountManager implements AccountManagerDelegate { return result; } + public boolean removeAccountHolderExplicitly(AccountHolder accountHolder) { + boolean result = mAccounts.remove(accountHolder); + postAsyncAccountChangedEvent(); + return result; + } + @Override public AccountManagerFuture<Boolean> removeAccount(Account account, AccountManagerCallback<Boolean> callback, Handler handler) { |