Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
213823004:
Calls FireRefreshTokenRevoked if an account is removed. (Closed)
|
Created:
6 years, 9 months ago by acleung1 Modified:
6 years, 6 months ago CC:
chromium-reviews Base URL:
https://chromium.googlesource.com/chromium/src.git@master Visibility:
Public. |
DescriptionCalls 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
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=266108
Patch Set 1 #
Total comments: 11
Patch Set 2 : address comments #Patch Set 3 : Address Issues + Use Shared Pref #Patch Set 4 : Remove unused varible. #
Total comments: 1
Patch Set 5 : Change GetAccount to return the cache instead of system call. #Patch Set 6 : Fix some issues. #
Total comments: 4
Patch Set 7 : Added Tests / Addressed Comments #Patch Set 8 : Revert getAccounts to original behavior #
Total comments: 11
Patch Set 9 : #Patch Set 10 : clean up #
Total comments: 8
Patch Set 11 : comments + tests #
Total comments: 16
Messages
Total messages: 25 (0 generated)
Thanks Alan. A few questions below. https://codereview.chromium.org/213823004/diff/1/chrome/browser/signin/androi... File chrome/browser/signin/android_profile_oauth2_token_service.cc (right): https://codereview.chromium.org/213823004/diff/1/chrome/browser/signin/androi... chrome/browser/signin/android_profile_oauth2_token_service.cc:174: const std::vector<std::string>& account_ids) { 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. https://codereview.chromium.org/213823004/diff/1/chrome/browser/signin/androi... chrome/browser/signin/android_profile_oauth2_token_service.cc:182: std::vector<std::string> ids = GetAccounts(); In what cases is |ids| going to be different from |accounts_ids|? https://codereview.chromium.org/213823004/diff/1/chrome/browser/signin/androi... chrome/browser/signin/android_profile_oauth2_token_service.cc:184: // Test to see if we an account is removed from the Android AccountManager. Remove "if we" ? https://codereview.chromium.org/213823004/diff/1/chrome/browser/signin/androi... chrome/browser/signin/android_profile_oauth2_token_service.cc:187: it != current_accounts_.end(); it++) { Indent one more.
Can you please take another look, thanks! https://codereview.chromium.org/213823004/diff/1/chrome/browser/signin/androi... File chrome/browser/signin/android_profile_oauth2_token_service.cc (right): https://codereview.chromium.org/213823004/diff/1/chrome/browser/signin/androi... chrome/browser/signin/android_profile_oauth2_token_service.cc:174: const std::vector<std::string>& account_ids) { On 2014/03/27 15:46:45, Roger Tawa wrote: > 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. 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? https://codereview.chromium.org/213823004/diff/1/chrome/browser/signin/androi... chrome/browser/signin/android_profile_oauth2_token_service.cc:182: std::vector<std::string> ids = GetAccounts(); On 2014/03/27 15:46:45, Roger Tawa wrote: > In what cases is |ids| going to be different from |accounts_ids|? 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 :) https://codereview.chromium.org/213823004/diff/1/chrome/browser/signin/androi... chrome/browser/signin/android_profile_oauth2_token_service.cc:184: // Test to see if we an account is removed from the Android AccountManager. On 2014/03/27 15:46:45, Roger Tawa wrote: > Remove "if we" ? Done. https://codereview.chromium.org/213823004/diff/1/chrome/browser/signin/androi... chrome/browser/signin/android_profile_oauth2_token_service.cc:187: it != current_accounts_.end(); it++) { On 2014/03/27 15:46:45, Roger Tawa wrote: > Indent one more. Done.
https://codereview.chromium.org/213823004/diff/1/chrome/browser/signin/androi... File chrome/browser/signin/android_profile_oauth2_token_service.cc (right): https://codereview.chromium.org/213823004/diff/1/chrome/browser/signin/androi... chrome/browser/signin/android_profile_oauth2_token_service.cc:174: const std::vector<std::string>& account_ids) { On 2014/03/27 21:09:54, acleung1 wrote: > On 2014/03/27 15:46:45, Roger Tawa wrote: > > 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. > > 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? 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?
On 2014/03/28 13:21:59, Roger Tawa wrote: > https://codereview.chromium.org/213823004/diff/1/chrome/browser/signin/androi... > File chrome/browser/signin/android_profile_oauth2_token_service.cc (right): > > https://codereview.chromium.org/213823004/diff/1/chrome/browser/signin/androi... > chrome/browser/signin/android_profile_oauth2_token_service.cc:174: const > std::vector<std::string>& account_ids) { > On 2014/03/27 21:09:54, acleung1 wrote: > > On 2014/03/27 15:46:45, Roger Tawa wrote: > > > 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. > > > > 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? > > 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. > I just realized that if we must store the last seen accounts some where, it should probably be in the Java layer. The reason is that we Chrome gets killed all the time on Android. The common thing to do is save it in a share pref. Let still pass that in as the last seen accounts. WDYT? > 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. > Done. Still there. > 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. > Done. It will just copy |signed_in_account| into the new |current_accounts_| preference. > 3/ GetAccounts() should return the |current_accounts_| instead of the OS > accounts directly. > I am not sure about that one. Right now it remains the system's list. ValidateAccounts will compare that with the last seen one. > 4/ RevokeAllCredentials() should clear |current_accounts_| after firing revoke > events. > Done. > What do you think?
This was sitting in pending list, forgot to send yesterday. https://codereview.chromium.org/213823004/diff/1/chrome/browser/signin/androi... File chrome/browser/signin/android_profile_oauth2_token_service.cc (right): https://codereview.chromium.org/213823004/diff/1/chrome/browser/signin/androi... chrome/browser/signin/android_profile_oauth2_token_service.cc:174: const std::vector<std::string>& account_ids) { On 2014/03/27 21:09:54, acleung1 wrote: > On 2014/03/27 15:46:45, Roger Tawa wrote: > > 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. > > 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? 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.
On 2014/03/31 21:30:50, acleung1 wrote: > On 2014/03/28 13:21:59, Roger Tawa wrote: > > > https://codereview.chromium.org/213823004/diff/1/chrome/browser/signin/androi... > > File chrome/browser/signin/android_profile_oauth2_token_service.cc (right): > > > > > https://codereview.chromium.org/213823004/diff/1/chrome/browser/signin/androi... > > chrome/browser/signin/android_profile_oauth2_token_service.cc:174: const > > std::vector<std::string>& account_ids) { > > On 2014/03/27 21:09:54, acleung1 wrote: > > > On 2014/03/27 15:46:45, Roger Tawa wrote: > > > > 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. > > > > > > 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? > > > > 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. > > > > I just realized that if we must store the last seen accounts some where, it > should > probably be in the Java layer. The reason is that we Chrome gets killed all the > time > on Android. The common thing to do is save it in a share pref. Let still pass > that in as the last seen accounts. > > WDYT? > > > 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. > > > Done. Still there. > > > 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. > > > > Done. It will just copy |signed_in_account| into the new |current_accounts_| > preference. > > > 3/ GetAccounts() should return the |current_accounts_| instead of the OS > > accounts directly. > > > > I am not sure about that one. Right now it remains the system's list. > ValidateAccounts will > compare that with the last seen one. > > > 4/ RevokeAllCredentials() should clear |current_accounts_| after firing revoke > > events. > > > Done. > > > What do you think? If it makes sense to store last seen in the java layer, that's cool. Wrt GetAccounts(), that is a public api. If some code calls it after RevokeAllCredentials() is called, then it should return an empty list. If GetAccounts() always returns the system list, then this will not be the case. See comments in GetAccounts(); the intention was that it would return a "filtered" list based on state of token service. I think GetAccounts() should return the "last seen" accounts. I guess this is equivalent to the old |current_accounts_| member you had. Since ValidateAccounts() needs the list of system accounts, either this can be passed in as arg, or the current implementation of GetAccounts() could be renamed to something like GetSystemAccounts() and used by ValidateAccounts(). Does that make sense?
https://codereview.chromium.org/213823004/diff/60001/chrome/browser/signin/an... File chrome/browser/signin/android_profile_oauth2_token_service.cc (right): https://codereview.chromium.org/213823004/diff/60001/chrome/browser/signin/an... chrome/browser/signin/android_profile_oauth2_token_service.cc:208: FireRefreshTokenRevoked(signed_in_account); Should also fire revoke for all last seen accounts too.
Hi Roger, this is ready to go for another look. I added Tommy for chrome/android/java/src/org/chromium/chrome/browser/signin/OAuth2TokenService.java OWNER approval. Would you mind taking a look? Thanks! https://codereview.chromium.org/213823004/diff/1/chrome/browser/signin/androi... File chrome/browser/signin/android_profile_oauth2_token_service.cc (right): https://codereview.chromium.org/213823004/diff/1/chrome/browser/signin/androi... chrome/browser/signin/android_profile_oauth2_token_service.cc:174: const std::vector<std::string>& account_ids) { On 2014/04/02 14:45:58, Roger Tawa wrote: > On 2014/03/27 21:09:54, acleung1 wrote: > > On 2014/03/27 15:46:45, Roger Tawa wrote: > > > 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. > > > > 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? > > 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. Yes, you are correct. My reply was oudated and was intended for the first reply.
Thanks Alan. lgtm with some nits below. Can we also have a unit test for java code? https://codereview.chromium.org/213823004/diff/100001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/signin/OAuth2TokenService.java (right): https://codereview.chromium.org/213823004/diff/100001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/signin/OAuth2TokenService.java:217: mNativeProfileOAuth2TokenService, prevAccounts, currentlySignedInAccount); In the case where |currentlySignedInAccount| is not in the array returned by getSystemAccounts(), it seems we should call saveStoredAccounts() with an empty list. Maybe nativeValidateAccounts() needs to return a bool so we can write something like: if (!nativeValidateAccounts(mNativeProfileOAuth2TokenService, prevAccounts, currentlySignedInAccount)) { saveStoredAccounts(new String[]{}); } https://codereview.chromium.org/213823004/diff/100001/chrome/browser/signin/a... File chrome/browser/signin/android_profile_oauth2_token_service.cc (right): https://codereview.chromium.org/213823004/diff/100001/chrome/browser/signin/a... chrome/browser/signin/android_profile_oauth2_token_service.cc:210: it != account_ids.end(); it++) { Should this be |prev_account_ids| instead of |account_ids| ?
https://codereview.chromium.org/213823004/diff/100001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/signin/OAuth2TokenService.java (right): https://codereview.chromium.org/213823004/diff/100001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/signin/OAuth2TokenService.java:217: mNativeProfileOAuth2TokenService, prevAccounts, currentlySignedInAccount); On 2014/04/08 20:40:53, Roger Tawa wrote: > In the case where |currentlySignedInAccount| is not in the array returned by > getSystemAccounts(), it seems we should call saveStoredAccounts() with an empty > list. Maybe nativeValidateAccounts() needs to return a bool so we can write > something like: > > if (!nativeValidateAccounts(mNativeProfileOAuth2TokenService, > prevAccounts, > currentlySignedInAccount)) { > saveStoredAccounts(new String[]{}); > } Done. https://codereview.chromium.org/213823004/diff/100001/chrome/browser/signin/a... File chrome/browser/signin/android_profile_oauth2_token_service.cc (right): https://codereview.chromium.org/213823004/diff/100001/chrome/browser/signin/a... chrome/browser/signin/android_profile_oauth2_token_service.cc:210: it != account_ids.end(); it++) { On 2014/04/08 20:40:53, Roger Tawa wrote: > Should this be |prev_account_ids| instead of |account_ids| ? Yes. It will cover the case where user deletes another account then his sync account before going back to chrome.
Looking good Alan, but I think you undid a change from patchset #6 that should remain. See below. https://codereview.chromium.org/213823004/diff/140001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/signin/OAuth2TokenService.java (right): https://codereview.chromium.org/213823004/diff/140001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/signin/OAuth2TokenService.java:108: return getSystemAccounts(context); This needs to remain getStoredAccounts() right? Otherwise, after revoking all tokens, callers of O2TS::GetAccounts() will still see all accounts on the system. I think you need to annotate getSystemAccounts() with @CalledByNative, and then have getSystemAccounts() called at line 179 in android_profile_oauth2_token_service.cc. Yes/no? Alternatively, pass in system accounts to nativeValidateAccounts() so it does not need to fetch it explicitly? Might make it easier to test that function too? https://codereview.chromium.org/213823004/diff/140001/chrome/android/javatest... File chrome/android/javatests/src/org/chromium/chrome/browser/signin/OAuth2TokenServiceTest.java (right): https://codereview.chromium.org/213823004/diff/140001/chrome/android/javatest... chrome/android/javatests/src/org/chromium/chrome/browser/signin/OAuth2TokenServiceTest.java:121: assertEquals("We should have zero accounts", 2, accounts.length); Comment should say two accounts? https://codereview.chromium.org/213823004/diff/140001/chrome/android/javatest... chrome/android/javatests/src/org/chromium/chrome/browser/signin/OAuth2TokenServiceTest.java:123: Is this test complete? Seems it does the same as the previous one? https://codereview.chromium.org/213823004/diff/140001/chrome/browser/signin/a... File chrome/browser/signin/android_profile_oauth2_token_service.cc (right): https://codereview.chromium.org/213823004/diff/140001/chrome/browser/signin/a... chrome/browser/signin/android_profile_oauth2_token_service.cc:177: return false; Should this return here, or should we still fire the revokes if |prev_account_ids| is not empty? Seems like you might be able to just remove lines 176-177. If |signed_in_accounts| is empty, it will fail the find() at line 180, and hit the code at line 210. What do you think? https://codereview.chromium.org/213823004/diff/140001/chrome/browser/signin/a... chrome/browser/signin/android_profile_oauth2_token_service.cc:219: } I think it would be good to write tests for this function, since it does a lot of things. Tests I could see: 1/ |prev_account_ids| empty, system accounts non-empty 2/ |prev_account_ids| == non-empty system accounts 3/ |prev_account_ids| non-empty but not equal to non-empty system accounts 4/ 1-3 above, with empty |signed_in_account| 5/ 1-3 above, with non-empty |signed_in_account| and its in system accounts 6/ 1-3 above, with non-empty |signed_in_account| and its not in system accounts After each case, make sure stored accounts is what you expect and that all available and revoked events were fired.
Not trying to ignore you by the way... I mostly agree with Roger so far, so I'll wait and do a round when those comments have been addressed. https://codereview.chromium.org/213823004/diff/140001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/signin/OAuth2TokenService.java (right): https://codereview.chromium.org/213823004/diff/140001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/signin/OAuth2TokenService.java:97: public static String[] getSystemAccounts(Context context) { Does this need to be public?
https://codereview.chromium.org/213823004/diff/140001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/signin/OAuth2TokenService.java (right): https://codereview.chromium.org/213823004/diff/140001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/signin/OAuth2TokenService.java:97: public static String[] getSystemAccounts(Context context) { On 2014/04/10 19:07:40, nyquist wrote: > Does this need to be public? No. Removed public visibility. https://codereview.chromium.org/213823004/diff/140001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/signin/OAuth2TokenService.java:108: return getSystemAccounts(context); On 2014/04/10 15:30:54, Roger Tawa wrote: > This needs to remain getStoredAccounts() right? Otherwise, after revoking all > tokens, callers of O2TS::GetAccounts() will still see all accounts on the > system. > > I think you need to annotate getSystemAccounts() with @CalledByNative, and then > have getSystemAccounts() called at line 179 in > android_profile_oauth2_token_service.cc. Yes/no? > > Alternatively, pass in system accounts to nativeValidateAccounts() so it does > not need to fetch it explicitly? Might make it easier to test that function > too? I have been trying to find a way to get this to work (I think you might have missed a PM I sent you?) Anyways, here are the issues. 1) GetAccounts is called in more than just one place as this is an PO2TS method. This mean it is possible that someone might want to list accounts before ValidateAccounts gets a chance to update the pref. 2) GetAccounts need to memorize the result of the last system to list account. This means that we are make GetAccount non-sideeffect-free. Which is gets into all kinds of weird race condition with other people calling it from #1. For @CalledByNative: I don't think so. Doesn't look like I am making any direct calls. I haven't gotten any dynamic linking error when I test it. https://codereview.chromium.org/213823004/diff/140001/chrome/android/javatest... File chrome/android/javatests/src/org/chromium/chrome/browser/signin/OAuth2TokenServiceTest.java (right): https://codereview.chromium.org/213823004/diff/140001/chrome/android/javatest... chrome/android/javatests/src/org/chromium/chrome/browser/signin/OAuth2TokenServiceTest.java:121: assertEquals("We should have zero accounts", 2, accounts.length); On 2014/04/10 15:30:54, Roger Tawa wrote: > Comment should say two accounts? I intent to revert this. (see comments on why in the Java file.) https://codereview.chromium.org/213823004/diff/140001/chrome/android/javatest... chrome/android/javatests/src/org/chromium/chrome/browser/signin/OAuth2TokenServiceTest.java:123: On 2014/04/10 15:30:54, Roger Tawa wrote: > Is this test complete? Seems it does the same as the previous one? Ditto.
Sorry for the delay Alan. See below for some comments. https://codereview.chromium.org/213823004/diff/140001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/signin/OAuth2TokenService.java (right): https://codereview.chromium.org/213823004/diff/140001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/signin/OAuth2TokenService.java:108: return getSystemAccounts(context); On 2014/04/10 21:36:01, acleung1 wrote: > On 2014/04/10 15:30:54, Roger Tawa wrote: > > This needs to remain getStoredAccounts() right? Otherwise, after revoking all > > tokens, callers of O2TS::GetAccounts() will still see all accounts on the > > system. > > > > I think you need to annotate getSystemAccounts() with @CalledByNative, and > then > > have getSystemAccounts() called at line 179 in > > android_profile_oauth2_token_service.cc. Yes/no? > > > > Alternatively, pass in system accounts to nativeValidateAccounts() so it does > > not need to fetch it explicitly? Might make it easier to test that function > > too? > > > I have been trying to find a way to get this to work (I think you might have > missed a PM I sent you?) > > Anyways, here are the issues. > > 1) GetAccounts is called in more than just one place as this is an PO2TS method. > This mean it is possible that someone might want to list accounts before > ValidateAccounts gets a chance to update the pref. > > 2) GetAccounts need to memorize the result of the last system to list account. > This means that we are make GetAccount non-sideeffect-free. Which is gets into > all kinds of weird race condition with other people calling it from #1. > > > For @CalledByNative: I don't think so. Doesn't look like I am making any direct > calls. I haven't gotten any dynamic linking error when I test it. GetAccounts() is a public api of OAuth2TokenService, so its behaviour needs to be well defined. Its overridden in AndroidProfileOAuth2TokenService to return the accounts known to chrome. The behaviour of GetAccounts() on android should be: - after initial creation of APO2TS, it should return an empty list - after a call to nativeValidateAccounts() where currentlySignedInAccount is empty, it should continue to return an empty list - after a call to nativeValidateAccounts() where currentlySignedInAccount is non-empty, it should return the system accounts - after a call to RevokeAllCredentials(), it should return an empty list If there is a customer of APO2TS that wants to get the list of "system" accounts before validateAccounts() has run, there seems to be a layering violation going on. What is the scenario? All calls to GetAccounts() internally (i.e. from this class or the sibling C++ class) should be changed if they need different behaviour. So my suggestion would be: - stored accounts starts out empty - GetAccounts() returns the stored accounts. This is side-effect free - stored accounts is updated in validateAccounts(), as is done already below - no code called directly or indirectly from validateAccounts() should call GetAccounts(), since the list is rather undefined at this point. Code should call getStoredAccounts() and getSystemAccounts() to get a full picture of the current state and validate it. In fact, it might be best to have validateAccounts() pass all required info as args to nativeValidateAccounts() so that there is no need to call from C++ code to java code. Does that seem reasonable? What situations are not covered by this?
> GetAccounts() is a public api of OAuth2TokenService, so its behaviour needs to > be well defined. Its overridden in AndroidProfileOAuth2TokenService to return > the accounts known to chrome. The behaviour of GetAccounts() on android should > be: I think the biggest issue I found is this. We can't rely on ValidateAccounts to 'update' the cache. As you have mentioned, GetAccounts is a public API. Everyone calls it at various time. > - after initial creation of APO2TS, it should return an empty list This make sense but I am not sure how to get that to work. Here is the situation. On a complete new run, we don't know if we called ValidateAccount or not. We are in the situation where we don't know if we have just initialized with already signed in accounts or a bunch of account just got added. > - after a call to nativeValidateAccounts() where currentlySignedInAccount is > empty, it should continue to return an empty list Agree. > - after a call to nativeValidateAccounts() where currentlySignedInAccount is > non-empty, it should return the system accounts Agree. > - after a call to RevokeAllCredentials(), it should return an empty list Agree. > If there is a customer of APO2TS that wants to get the list of "system" accounts > before validateAccounts() has run, there seems to be a layering violation going > on. What is the scenario? I have seen this occasionally but wasn't able to get a stacktrace. Could the reconciler call this on a periodic run? > All calls to GetAccounts() internally (i.e. from this class or the sibling C++ > class) should be changed if they need different behaviour. So my suggestion > would be: > > - stored accounts starts out empty > - GetAccounts() returns the stored accounts. This is side-effect free > - stored accounts is updated in validateAccounts(), as is done already below > - no code called directly or indirectly from validateAccounts() should call > GetAccounts(), since the list is rather undefined at this point. Code should > call getStoredAccounts() and getSystemAccounts() to get a full picture of the > current state and validate it. In fact, it might be best to have > validateAccounts() pass all required info as args to nativeValidateAccounts() so > that there is no need to call from C++ code to java code. > > Does that seem reasonable? What situations are not covered by this? Good idea. I think the best way to move forward is to do exactly that. I think I will try to correct all the GetAccount calls if their expectation isn't correct.
On 2014/04/16 20:58:27, acleung1 wrote: > > GetAccounts() is a public api of OAuth2TokenService, so its behaviour needs to > > be well defined. Its overridden in AndroidProfileOAuth2TokenService to return > > the accounts known to chrome. The behaviour of GetAccounts() on android > should > > be: > > I think the biggest issue I found is this. We can't rely on ValidateAccounts to > 'update' the cache. As you have mentioned, GetAccounts is a public API. Everyone > calls it at various time. There are no concurrency problem between these two methods. Do you have an example where this does not work? > > - after initial creation of APO2TS, it should return an empty list > > This make sense but I am not sure how to get that to work. Here is the > situation. On a complete new run, we don't know if we called ValidateAccount or > not. We are in the situation where we don't know if we have just initialized > with already signed in accounts or a bunch of account just got added. If any code calls GetAccounts() "too early", then it will see no accounts. But then it should receive notification of the accounts when validateAccounts() runs. Are there cases where you think this won't work?
Ok. Please take another look. I am much more happy with this patch. > If any code calls GetAccounts() "too early", then it will see no accounts. But > then it should receive notification of the accounts when validateAccounts() > runs. Are there cases where you think this won't work?
Looks great Alan. Mostly nits, but one comment in android_profile_oauth2_token_service.cc, let me know what you think. Do you think we can have tests like I suggested in message #12? https://codereview.chromium.org/213823004/diff/180001/chrome/android/javatest... File chrome/android/javatests/src/org/chromium/chrome/browser/signin/OAuth2TokenServiceTest.java (right): https://codereview.chromium.org/213823004/diff/180001/chrome/android/javatest... chrome/android/javatests/src/org/chromium/chrome/browser/signin/OAuth2TokenServiceTest.java:54: assertEquals("There should be one registered account", 0, accounts.length); nit: assert string should zero, not one. https://codereview.chromium.org/213823004/diff/180001/chrome/android/javatest... chrome/android/javatests/src/org/chromium/chrome/browser/signin/OAuth2TokenServiceTest.java:75: assertEquals("There should be one registered account", 0, accounts.length); nit: assert string should zero, not one. https://codereview.chromium.org/213823004/diff/180001/chrome/browser/signin/a... File chrome/browser/signin/android_profile_oauth2_token_service.cc (right): https://codereview.chromium.org/213823004/diff/180001/chrome/browser/signin/a... chrome/browser/signin/android_profile_oauth2_token_service.cc:198: } Nit: maybe remove dupe code: std::vector<std::string> prev_ids = ...; std::vector<std::string> curr_ids = ...; if (!ValidateAccounts(signed_in_account, prev_ids, curr_ids)) curr_ids.clear(); JNIEnv* env = AttachCurrentThread(); ScopedJavaLocalRef<jobjectArray> java_accounts( base::android::ToJavaArrayOfStrings(env, curr_ids)); Java_OAuth2TokenService_saveStoredAccounts( env, base::android::GetApplicationContext(), java_accounts.obj()); https://codereview.chromium.org/213823004/diff/180001/chrome/browser/signin/a... chrome/browser/signin/android_profile_oauth2_token_service.cc:205: return false; I think lines 204-205 should be removed. That way, if |prev_account_ids| happen to be not empty, we hit lines 235-244 below and send notifs.
Thanks! There is already tests that cover most of those. I added a few more. PTAL? https://codereview.chromium.org/213823004/diff/180001/chrome/android/javatest... File chrome/android/javatests/src/org/chromium/chrome/browser/signin/OAuth2TokenServiceTest.java (right): https://codereview.chromium.org/213823004/diff/180001/chrome/android/javatest... chrome/android/javatests/src/org/chromium/chrome/browser/signin/OAuth2TokenServiceTest.java:54: assertEquals("There should be one registered account", 0, accounts.length); On 2014/04/23 15:03:59, Roger Tawa wrote: > nit: assert string should zero, not one. Done. https://codereview.chromium.org/213823004/diff/180001/chrome/android/javatest... chrome/android/javatests/src/org/chromium/chrome/browser/signin/OAuth2TokenServiceTest.java:75: assertEquals("There should be one registered account", 0, accounts.length); On 2014/04/23 15:03:59, Roger Tawa wrote: > nit: assert string should zero, not one. Done. https://codereview.chromium.org/213823004/diff/180001/chrome/browser/signin/a... File chrome/browser/signin/android_profile_oauth2_token_service.cc (right): https://codereview.chromium.org/213823004/diff/180001/chrome/browser/signin/a... chrome/browser/signin/android_profile_oauth2_token_service.cc:198: } On 2014/04/23 15:03:59, Roger Tawa wrote: > Nit: maybe remove dupe code: > > std::vector<std::string> prev_ids = ...; > std::vector<std::string> curr_ids = ...; > if (!ValidateAccounts(signed_in_account, prev_ids, curr_ids)) > curr_ids.clear(); > > JNIEnv* env = AttachCurrentThread(); > ScopedJavaLocalRef<jobjectArray> java_accounts( > base::android::ToJavaArrayOfStrings(env, curr_ids)); > Java_OAuth2TokenService_saveStoredAccounts( > env, > base::android::GetApplicationContext(), > java_accounts.obj()); Done. https://codereview.chromium.org/213823004/diff/180001/chrome/browser/signin/a... chrome/browser/signin/android_profile_oauth2_token_service.cc:205: return false; On 2014/04/23 15:03:59, Roger Tawa wrote: > I think lines 204-205 should be removed. That way, if |prev_account_ids| happen > to be not empty, we hit lines 235-244 below and send notifs. Done.
lgtm Thanks Alan.
The CQ bit was checked by acleung@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/acleung@chromium.org/213823004/200001
https://codereview.chromium.org/213823004/diff/200001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/signin/OAuth2TokenService.java (right): https://codereview.chromium.org/213823004/diff/200001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/signin/OAuth2TokenService.java:43: public static final String STORED_ACCOUNTS_KEY = "google.services.stored_accounts"; Nit: This is in a shared namespace. It lgtm, but keep in mind it is hard to ever change it. Oftentimes we use the package name for these kinds of things (but then again, that can naturally also change over time). https://codereview.chromium.org/213823004/diff/200001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/signin/OAuth2TokenService.java:98: * Called by native to list the activite accounts in the OS. Add a comment along the lines "This might not be what you want", and point to {@link #getAccounts}. Also, typo: active https://codereview.chromium.org/213823004/diff/200001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/signin/OAuth2TokenService.java:110: * This can differ from getSystemAccounts as the user add/remove accounts Nit: Use JavaDoc style (ie. {@link #getSystemAccounts}) for code links. https://codereview.chromium.org/213823004/diff/200001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/signin/OAuth2TokenService.java:111: * from the OS. validateAccounts should be called to keep these two Nit: Same as above. https://codereview.chromium.org/213823004/diff/200001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/signin/OAuth2TokenService.java:295: putStringSet(STORED_ACCOUNTS_KEY, set).apply(); .apply() does a best-effort attempt to persist the data, but exits immediately. I think this is the right way to go, but will we recover from this if the persistance fails? Maybe on a cold start? https://codereview.chromium.org/213823004/diff/200001/chrome/android/javatest... File chrome/android/javatests/src/org/chromium/chrome/browser/signin/OAuth2TokenServiceIntegrationTest.java (right): https://codereview.chromium.org/213823004/diff/200001/chrome/android/javatest... chrome/android/javatests/src/org/chromium/chrome/browser/signin/OAuth2TokenServiceIntegrationTest.java:290: assertEquals(2, mObserver.getRevokedCallCount()); Nit: Add assertEquals(2, mObserver.getAvailableCallCount()); here too? https://codereview.chromium.org/213823004/diff/200001/chrome/android/javatest... chrome/android/javatests/src/org/chromium/chrome/browser/signin/OAuth2TokenServiceIntegrationTest.java:308: mChromeSigninController.clearSignedInUser(); From the code, it seems like just clearing the signed in user should be enough to get the call to revoke for all the accounts. Is that intentional? It is unclear from the test. https://codereview.chromium.org/213823004/diff/200001/chrome/android/javatest... chrome/android/javatests/src/org/chromium/chrome/browser/signin/OAuth2TokenServiceIntegrationTest.java:313: mOAuth2TokenService.validateAccounts(mContext); Nit: Also assertEquals(2, mObserver.getAvailableCallCount()); here? https://codereview.chromium.org/213823004/diff/200001/chrome/android/javatest... File chrome/android/javatests/src/org/chromium/chrome/browser/signin/OAuth2TokenServiceTest.java (right): https://codereview.chromium.org/213823004/diff/200001/chrome/android/javatest... chrome/android/javatests/src/org/chromium/chrome/browser/signin/OAuth2TokenServiceTest.java:68: assertEquals("There should be one registered account", 2, sysAccounts.length); s/one/two https://codereview.chromium.org/213823004/diff/200001/chrome/browser/signin/a... File chrome/browser/signin/android_profile_oauth2_token_service.cc (right): https://codereview.chromium.org/213823004/diff/200001/chrome/browser/signin/a... chrome/browser/signin/android_profile_oauth2_token_service.cc:185: if (!ValidateAccounts(signed_in_account, prev_ids, curr_ids)) { Nit: Remove { } to be consistent? https://codereview.chromium.org/213823004/diff/200001/chrome/browser/signin/a... chrome/browser/signin/android_profile_oauth2_token_service.cc:217: // Always fire the primary signed in account first. How would you feel about extracting the rest of this block into its own method, something like: void FireRefreshTokenAvailableForAllAccounts(const std::string& first_account_id, const std::vector<std::string>& all_account_ids) ? https://codereview.chromium.org/213823004/diff/200001/chrome/browser/signin/a... chrome/browser/signin/android_profile_oauth2_token_service.cc:222: if (*it != signed_in_account) { Nit: Remove { } to match the rest of this function? https://codereview.chromium.org/213823004/diff/200001/chrome/browser/signin/a... chrome/browser/signin/android_profile_oauth2_token_service.cc:228: // Currently signed in account does not any longer exist among accounts on How would you feel about extracting the rest of this block into its own method, something like: void FireRefreshTokenRevokedForAllAccounts(const std::string& first_account_id, const std::vector<std::string>& all_account_ids) ? https://codereview.chromium.org/213823004/diff/200001/chrome/browser/signin/a... chrome/browser/signin/android_profile_oauth2_token_service.cc:229: // system together with all other accounts. Could you add a comment for why we revoked all tokens (even for the non-signed in accounts) when the currently signed in account is removed? https://codereview.chromium.org/213823004/diff/200001/chrome/browser/signin/a... chrome/browser/signin/android_profile_oauth2_token_service.cc:230: if (!signed_in_account.empty()) { Nit: Could you add a comment as to why we need to call FireRefreshTokenRevoked for the currently signed in account first? https://codereview.chromium.org/213823004/diff/200001/chrome/browser/signin/a... File chrome/browser/signin/android_profile_oauth2_token_service.h (right): https://codereview.chromium.org/213823004/diff/200001/chrome/browser/signin/a... chrome/browser/signin/android_profile_oauth2_token_service.h:56: bool ValidateAccounts(const std::string& signed_in_account, Why is this function public again? It seems to only be called from within APO2TS? Also, could you add a comment as to what the effects of calling this method is? i.e. that refresh tokens are considered available or revoked. Also, what does the bool return value describe?
Message was sent while issue was closed.
Change committed as 266108 |
