Calls FireRefreshTokenRevoked if an account is removed.

chrome/browser/signin/android_profile_oauth2_token_service.cc

 // Copyright 2013 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "chrome/browser/signin/android_profile_oauth2_token_service.h"
 
 #include "base/android/jni_android.h"
 #include "base/android/jni_array.h"
 #include "base/android/jni_string.h"
 #include "base/bind.h"
@@ skipping 153 matching lines @@
   std::vector<std::string> account_ids;
   base::android::AppendJavaStringArrayToStringVector(env,
                                                      accounts,
                                                      &account_ids);
   std::string signed_in_account = ConvertJavaStringToUTF8(env, j_current_acc);
   ValidateAccounts(signed_in_account, account_ids);
 }
 
 void AndroidProfileOAuth2TokenService::ValidateAccounts(
     const std::string& signed_in_account,
     const std::vector<std::string>& account_ids) {

[Roger Tawa OOO till Jul 10th, 2014/03/27 15:46:45]

This function is called when the user signs in and when clank receives
AccountManager.LOGIN_ACCOUNTS_CHANGED_ACTION.  I think it should be called from
SigninManagerAndroid::SignOut() too.  Yes/no?

What happens when you sign out?  Clank should fire a revoke for all accounts.

[acleung1, 2014/03/27 21:09:54]

Yes. Good point. I was under the impression that SigninMangerAndroid would call
the SigninManagerBase that the reconcilor would do the signing out.

Should AccountReconcilor::GoogleSignedOut be in charge of this?

Otherwise, we might need to add functions in O2TS to do complete signouts?

[Roger Tawa OOO till Jul 10th, 2014/03/28 13:21:59]

SigninManager already calls RevokeAllCredentials() on the token service.  The
base class ProfileOAuth2TokenService has an empty implementation of this
function, which AndroidProfileOAuth2TokenService inherits.  Might be best to
fire the revoke events there.

Note that RevokeAllCredentials() should not actually try to revoke the tokens at
the OS level, just fire the events and forget them internally.  It seems like
the right behaviour of the token service on android should be:

0/ I think it would be better to remove the |account_ids| arg from this
function.  Today code passes either the result of O2TS::GetAccounts() or the
list of the underlying OS accounts, both of which the token service can get for
itself.  Also, I think this should always be the list of the underlying OS
accounts anyway, so it would be better to not have the caller pass that in
explicitly.

1/ When ValidateAccounts() is called with a non-empty |signed_in_account|, the
token service should remember all underlying OS accounts, as is done by your CL
here.

2/ When ValidateAccounts() is called with an empty |signed_in_account|, no
accounts should be remembered.  Maybe good to clear |current_accounts_| at this
point.

3/ GetAccounts() should return the |current_accounts_| instead of the OS
accounts directly.

4/ RevokeAllCredentials() should clear |current_accounts_| after firing revoke
events.

What do you think?

[Roger Tawa OOO till Jul 10th, 2014/04/02 14:45:58]

I don't think we need to change AccountReconcilor or SigninManagerAndroid to
handle this.  I think we just need RevokeAllCredentials() to be implemented in
this class to send out revoke events to everyone, and essentially "forget" that
there are any accounts.

[acleung1, 2014/04/08 02:01:02]

Yes, you are correct. My reply was oudated and was intended for the first reply.

   if (signed_in_account.empty())
     return;
 
   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();

[Roger Tawa OOO till Jul 10th, 2014/03/27 15:46:45]

In what cases is |ids| going to be different from |accounts_ids|?

[acleung1, 2014/03/27 21:09:54]

Good catch. It's a mistake from previous CL. Was going to fix it in a separate
CL. Mind as well fix it now that I am caught :)

 
+    // Test to see if we an account is removed from the Android AccountManager.

[Roger Tawa OOO till Jul 10th, 2014/03/27 15:46:45]

Remove "if we" ?

[acleung1, 2014/03/27 21:09:54]

Done.

+    // If so, invoke FireRefreshTokenRevoked to notify the reconcilor.
+    for (std::vector<std::string>::iterator it = current_accounts_.begin();
+        it != current_accounts_.end(); it++) {

[Roger Tawa OOO till Jul 10th, 2014/03/27 15:46:45]

Indent one more.

[acleung1, 2014/03/27 21:09:54]

Done.

+      if (*it == signed_in_account)
+        continue;
+
+      if (std::find(account_ids.begin(),
+                    account_ids.end(),
+                    *it) == account_ids.end()) {
+        FireRefreshTokenRevoked(*it);
+      }
+    }
+
+    current_accounts_.clear();
+    current_accounts_.push_back(signed_in_account);
+
     // 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++) {
       if (*it != signed_in_account) {
         FireRefreshTokenAvailable(*it);
+        current_accounts_.push_back(*it);
       }
     }
   } else {
     // Currently signed in account does not any longer exist among accounts on
     // system.
     FireRefreshTokenRevoked(signed_in_account);
   }
 }
 
 void AndroidProfileOAuth2TokenService::FireRefreshTokenAvailableFromJava(
@@ skipping 65 matching lines @@
   GoogleServiceAuthError err(result ?
                              GoogleServiceAuthError::NONE :
                              GoogleServiceAuthError::CONNECTION_FAILED);
   heap_callback->Run(err, token, base::Time());
 }
 
 // static
 bool AndroidProfileOAuth2TokenService::Register(JNIEnv* env) {
   return RegisterNativesImpl(env);
 }