GAIA ID migration for Android

chrome/browser/signin/oauth2_token_service_delegate_android.cc

Index: chrome/browser/signin/oauth2_token_service_delegate_android.cc
diff --git a/chrome/browser/signin/oauth2_token_service_delegate_android.cc b/chrome/browser/signin/oauth2_token_service_delegate_android.cc
index 4c322a4eeb6f49c3fc75a59e7cfcadbe250f84f7..b0d4cf6947f22240c31772e5c80aa04a749a2443 100644
--- a/chrome/browser/signin/oauth2_token_service_delegate_android.cc
+++ b/chrome/browser/signin/oauth2_token_service_delegate_android.cc
@@ -131,7 +131,8 @@ std::string AndroidAccessTokenFetcher::CombineScopes(
 
 bool OAuth2TokenServiceDelegateAndroid::is_testing_profile_ = false;
 
-OAuth2TokenServiceDelegateAndroid::OAuth2TokenServiceDelegateAndroid() {
+OAuth2TokenServiceDelegateAndroid::OAuth2TokenServiceDelegateAndroid()
+    : account_tracker_service_(nullptr) {
   DVLOG(1) << "OAuth2TokenServiceDelegateAndroid::ctor";
   JNIEnv* env = AttachCurrentThread();
   base::android::ScopedJavaLocalRef<jobject> local_java_ref =
@@ -162,20 +163,29 @@ static jobject GetForProfile(JNIEnv* env,
                                                           j_profile_android);
 }
 
-void OAuth2TokenServiceDelegateAndroid::Initialize() {
+void OAuth2TokenServiceDelegateAndroid::Initialize(
+    AccountTrackerService* account_tracker_service) {
   DVLOG(1) << "OAuth2TokenServiceDelegateAndroid::Initialize";
+  account_tracker_service_ = account_tracker_service;

[Roger Tawa OOO till Jul 10th, 2015/08/12 15:28:47]

DCHECK(account_tracker_service); ?

[gogerald1, 2015/08/13 18:12:12]

Done.

   if (!is_testing_profile_) {
     Java_OAuth2TokenService_validateAccounts(
         AttachCurrentThread(), java_ref_.obj(),
         base::android::GetApplicationContext(), JNI_TRUE);
   }
+  if (account_tracker_service_->GetMigrationState() !=
+      AccountTrackerService::MIGRATION_NOT_STARTED) {
+    account_tracker_service_->SetMigrationDone();
+  }

[Roger Tawa OOO till Jul 10th, 2015/08/12 15:28:47]

Is it correct to mark migration done here?  Since
Java_OAuth2TokenService_validateAccounts() is now async, seems like we should
wait until it is finished.

Also, don't we need to wait for signin manager to finish its migration before we
mark as done?  Signin manager already marks the migration as complete.

Also we had discussed that migration cannot begin on android until we have a
mapping of all gaiaid<->username in place.  It's not clear in this CL that ATS
will make the right choice to begin the migration.

[gogerald1, 2015/08/13 18:12:12]

Done.

 }
 
 bool OAuth2TokenServiceDelegateAndroid::RefreshTokenIsAvailable(
     const std::string& account_id) const {
+  std::string accountid = MapUserAccountIdToOs(account_id);

[Roger Tawa OOO till Jul 10th, 2015/08/12 15:28:47]

local_account_id ?  Same below.

[gogerald1, 2015/08/13 18:12:12]

Done.

+  if (accountid.empty())
+    accountid = account_id;
   JNIEnv* env = AttachCurrentThread();
   ScopedJavaLocalRef<jstring> j_account_id =
-      ConvertUTF8ToJavaString(env, account_id);
+      ConvertUTF8ToJavaString(env, accountid);
   jboolean refresh_token_is_available =
       Java_OAuth2TokenService_hasOAuth2RefreshToken(
           env, base::android::GetApplicationContext(), j_account_id.obj());
@@ -188,7 +198,8 @@ void OAuth2TokenServiceDelegateAndroid::UpdateAuthError(
   // TODO(rogerta): do we need to update anything, or does the system handle it?
 }
 
-std::vector<std::string> OAuth2TokenServiceDelegateAndroid::GetAccounts() {
+std::vector<std::string>
+OAuth2TokenServiceDelegateAndroid::GetPersistentAccounts() {

[Roger Tawa OOO till Jul 10th, 2015/08/12 15:28:47]

Today accounts are persisted as usernames.  During the migration of O2TS on
android, this should be changed to account_ids.

[gogerald1, 2015/08/13 18:12:12]

Done.

   std::vector<std::string> accounts;
   JNIEnv* env = AttachCurrentThread();
   ScopedJavaLocalRef<jobjectArray> j_accounts =
@@ -200,6 +211,18 @@ std::vector<std::string> OAuth2TokenServiceDelegateAndroid::GetAccounts() {
   return accounts;
 }
 
+std::vector<std::string> OAuth2TokenServiceDelegateAndroid::GetAccounts() {
+  std::vector<std::string> accounts_name = GetPersistentAccounts();
+  std::vector<std::string> accounts_id;
+  for (std::vector<std::string>::iterator it = accounts_name.begin();
+       it != accounts_name.end(); ++it) {
+    std::string account_id = MapOsAccountIdToUser(*it);
+    if (!account_id.empty())
+      accounts_id.push_back(account_id);
+  }

[Roger Tawa OOO till Jul 10th, 2015/08/12 15:28:47]

When does this mapping need to be done here?  Before migration, accounts are
persisted as usernames, and we want to return usernames.  During and after
migration, accounts are persisted as account_ids and we want to return
account_ids.

[gogerald1, 2015/08/13 18:12:12]

Done.

+  return accounts_id;
+}
+
 std::vector<std::string>
 OAuth2TokenServiceDelegateAndroid::GetSystemAccounts() {

[Roger Tawa OOO till Jul 10th, 2015/08/12 15:28:47]

Rename this to GetSystemUsernames().  The system always deals with usernames, or
emails.  I think this makes it less confusing with account_ids.

[gogerald1, 2015/08/13 18:12:12]

Acknowledged.

   std::vector<std::string> accounts;
@@ -217,8 +240,11 @@ OAuth2TokenServiceDelegateAndroid::CreateAccessTokenFetcher(
     const std::string& account_id,
     net::URLRequestContextGetter* getter,
     OAuth2AccessTokenConsumer* consumer) {
-  ValidateAccountId(account_id);
-  return new AndroidAccessTokenFetcher(consumer, account_id);
+  std::string accountid = MapUserAccountIdToOs(account_id);
+  if (accountid.empty())
+    accountid = account_id;

[Roger Tawa OOO till Jul 10th, 2015/08/12 15:28:47]

When would MapUserAccountIdToOs() return an empty string? In these cases, I
suspect it would be better for it to return the argument it was passed.  That
way callers don't need to check for empty and use the arg passed to
MapUserAccountIdToOs().

[gogerald1, 2015/08/13 18:12:12]

Done.

+  ValidateAccountId(accountid);
+  return new AndroidAccessTokenFetcher(consumer, accountid);
 }
 
 void OAuth2TokenServiceDelegateAndroid::InvalidateAccessToken(
@@ -226,7 +252,7 @@ void OAuth2TokenServiceDelegateAndroid::InvalidateAccessToken(
     const std::string& client_id,
     const OAuth2TokenService::ScopeSet& scopes,
     const std::string& access_token) {
-  ValidateAccountId(account_id);
+  ValidateAccountId(MapUserAccountIdToOs(account_id));
   JNIEnv* env = AttachCurrentThread();
   ScopedJavaLocalRef<jstring> j_access_token =
       ConvertUTF8ToJavaString(env, access_token);
@@ -245,13 +271,21 @@ void OAuth2TokenServiceDelegateAndroid::ValidateAccounts(
     signed_in_account = ConvertJavaStringToUTF8(env, j_current_acc);
   if (!signed_in_account.empty())
     signed_in_account = gaia::CanonicalizeEmail(signed_in_account);
-  ValidateAccounts(signed_in_account, j_force_notifications != JNI_FALSE);
+  ValidateAccountsWithOsTypeIds(signed_in_account,
+                                j_force_notifications != JNI_FALSE);
 }
 
 void OAuth2TokenServiceDelegateAndroid::ValidateAccounts(
     const std::string& signed_in_account,
     bool force_notifications) {
-  std::vector<std::string> prev_ids = GetAccounts();
+  ValidateAccountsWithOsTypeIds(MapUserAccountIdToOs(signed_in_account),
+                                force_notifications);
+}
+
+void OAuth2TokenServiceDelegateAndroid::ValidateAccountsWithOsTypeIds(
+    const std::string& signed_in_account,
+    bool force_notifications) {

[Roger Tawa OOO till Jul 10th, 2015/08/12 15:28:47]

I don't think we should be validating accounts with usernames.  We should be
validating accounts with account_ids.  GetPersistentAccounts() should return
account_ids.  The usernames returned by GetSystemAccounts() should be mapped to
account_ids, and then validation can happen with account ids.

[gogerald1, 2015/08/13 18:12:12]

Done.

+  std::vector<std::string> prev_ids = GetPersistentAccounts();
   std::vector<std::string> curr_ids = GetSystemAccounts();
   std::vector<std::string> refreshed_ids;
   std::vector<std::string> revoked_ids;
@@ -376,7 +410,8 @@ void OAuth2TokenServiceDelegateAndroid::FireRefreshTokenAvailable(
       ConvertUTF8ToJavaString(env, account_id);
   Java_OAuth2TokenService_notifyRefreshTokenAvailable(env, java_ref_.obj(),
                                                       account_name.obj());
-  OAuth2TokenServiceDelegate::FireRefreshTokenAvailable(account_id);
+  OAuth2TokenServiceDelegate::FireRefreshTokenAvailable(
+      MapOsAccountIdToUser(account_id));
 }
 
 void OAuth2TokenServiceDelegateAndroid::FireRefreshTokenRevokedFromJava(
@@ -397,7 +432,8 @@ void OAuth2TokenServiceDelegateAndroid::FireRefreshTokenRevoked(
       ConvertUTF8ToJavaString(env, account_id);
   Java_OAuth2TokenService_notifyRefreshTokenRevoked(env, java_ref_.obj(),
                                                     account_name.obj());
-  OAuth2TokenServiceDelegate::FireRefreshTokenRevoked(account_id);
+  OAuth2TokenServiceDelegate::FireRefreshTokenRevoked(
+      MapOsAccountIdToUser(account_id));
 }
 
 void OAuth2TokenServiceDelegateAndroid::FireRefreshTokensLoadedFromJava(
@@ -417,7 +453,7 @@ void OAuth2TokenServiceDelegateAndroid::FireRefreshTokensLoaded() {
 void OAuth2TokenServiceDelegateAndroid::RevokeAllCredentials() {
   DVLOG(1) << "OAuth2TokenServiceDelegateAndroid::RevokeAllCredentials";
   ScopedBatchChange batch(this);
-  std::vector<std::string> accounts = GetAccounts();
+  std::vector<std::string> accounts = GetPersistentAccounts();

[Roger Tawa OOO till Jul 10th, 2015/08/12 15:28:47]

This is case where the code assumes GetPersistentAccounts() return account_ids. 
However, GetPersistentAccounts() returns usernames in this CL.  I think it's
correct for GetPersistentAccounts() to return accounts_ids, so
GetPersistentAccounts() should be changed to return them.

[gogerald1, 2015/08/13 18:12:12]

Done.

   for (std::vector<std::string>::iterator it = accounts.begin();
        it != accounts.end(); it++) {
     FireRefreshTokenRevoked(*it);
@@ -432,6 +468,41 @@ void OAuth2TokenServiceDelegateAndroid::RevokeAllCredentials() {
       env, base::android::GetApplicationContext(), java_accounts.obj());
 }
 
+std::string OAuth2TokenServiceDelegateAndroid::MapUserAccountIdToOs(

[Roger Tawa OOO till Jul 10th, 2015/08/12 15:28:47]

Maybe call this function: MapAccountIdToSystemUsername()

The function below could be called: MapSystemUsernameToAccountId()

[gogerald1, 2015/08/13 18:12:12]

Acknowledged.

+    const std::string& user_account_id) const {
+  if (account_tracker_service_->GetMigrationState() !=
+      AccountTrackerService::MIGRATION_NOT_STARTED) {
+    return account_tracker_service_->GetAccountInfo(user_account_id).email;
+  }
+  return user_account_id;
+}
+
+std::string OAuth2TokenServiceDelegateAndroid::MapOsAccountIdToUser(
+    const std::string& os_account_id) const {
+  if (account_tracker_service_->GetMigrationState() !=
+      AccountTrackerService::MIGRATION_NOT_STARTED) {
+    return account_tracker_service_->FindAccountInfoByEmail(os_account_id)
+        .account_id;
+  }
+
+  return os_account_id;
+}
+
+void OAuth2TokenServiceDelegateAndroid::SeedAccountsInfo(
+    JNIEnv* env,
+    jobject obj,
+    jobjectArray gaiaids,
+    jobjectArray usernames) {
+  std::vector<std::string> gaia_ids;
+  std::vector<std::string> account_names;
+  base::android::AppendJavaStringArrayToStringVector(env, gaiaids, &gaia_ids);
+  base::android::AppendJavaStringArrayToStringVector(env, usernames,
+                                                     &account_names);

[Roger Tawa OOO till Jul 10th, 2015/08/12 15:28:47]

DCHECK that arrays are same length.

[gogerald1, 2015/08/13 18:12:12]

Done.

+
+  for (unsigned int i = 0; i < gaia_ids.size(); i++) {
+    account_tracker_service_->SeedAccountInfo(gaia_ids[i], account_names[i]);
+  }
+}
 // 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,