Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1504283002:
[Sync] Switch to using GoogleAuthUtil.clearToken(). (Closed)
|
Created:
5 years ago by maxbogue Modified:
5 years ago CC:
chromium-reviews, tim+watch_chromium.org, pvalenzuela+watch_chromium.org, maxbogue+watch_chromium.org, plaree+watch_chromium.org, zea+watch_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
Description[Sync] Switch to using GoogleAuthUtil.clearToken().
GoogleAuthUtil has much better failure semantics, so this should
clear up some of the runtime exceptions AccountManager.invalidateAuthToken()
was producing.
Since invalidateToken() now throws exceptions when things go wrong,
this change isolates the network retry logic already existing in
AccountManagerHelper and shares it between getToken and invalidateToken().
BUG=418809, 535320
Committed: https://crrev.com/cbac98d0772350fe474971c09fd3332db7efebfa
Cr-Commit-Position: refs/heads/master@{#364474}
Patch Set 1 #Patch Set 2 : Actually attempt the AuthTask. #Patch Set 3 : Comment all the things. #Patch Set 4 : Use Void instead of Boolean. #
Total comments: 16
Patch Set 5 : Address comments. #
Total comments: 5
Patch Set 6 : Re-add hasUseCredentialsPermission() #Patch Set 7 : Remove USE_CREDENTIALS check again :) #
Messages
Total messages: 21 (8 generated)
Description was changed from ========== [Sync] Switch to using GoogleAuthUtil.clearToken(). BUG=418809 ========== to ========== [Sync] Switch to using GoogleAuthUtil.clearToken(). GoogleAuthUtil has much better failure semantics, so this should clear up some of the exceptions AccountManager.invalidateAuthToken() was producing. Since invalidateToken() now throws exceptions when things go wrong, this change isolates the network retry logic already existing in AccountManagerHelper and shares it between getToken and invalidateToken(). BUG=418809 ==========
Description was changed from ========== [Sync] Switch to using GoogleAuthUtil.clearToken(). GoogleAuthUtil has much better failure semantics, so this should clear up some of the exceptions AccountManager.invalidateAuthToken() was producing. Since invalidateToken() now throws exceptions when things go wrong, this change isolates the network retry logic already existing in AccountManagerHelper and shares it between getToken and invalidateToken(). BUG=418809 ========== to ========== [Sync] Switch to using GoogleAuthUtil.clearToken(). GoogleAuthUtil has much better failure semantics, so this should clear up some of the runtime exceptions AccountManager.invalidateAuthToken() was producing. Since invalidateToken() now throws exceptions when things go wrong, this change isolates the network retry logic already existing in AccountManagerHelper and shares it between getToken and invalidateToken(). BUG=418809 ==========
maxbogue@chromium.org changed reviewers: + bauerb@chromium.org, knn@chromium.org, nyquist@chromium.org
Tommy and (Bernhard and/or Khannan), could you review this change?
https://codereview.chromium.org/1504283002/diff/60001/sync/android/java/src/o... File sync/android/java/src/org/chromium/sync/signin/AccountManagerDelegate.java (right): https://codereview.chromium.org/1504283002/diff/60001/sync/android/java/src/o... sync/android/java/src/org/chromium/sync/signin/AccountManagerDelegate.java:43: * to a network connectivity issue, for example. I'm not sure this comment makes sense. Does invalidateAuthToken ever access the network? Though it's fine for you to mention that it's transient, I think I'd just remove the example part. https://codereview.chromium.org/1504283002/diff/60001/sync/android/java/src/o... File sync/android/java/src/org/chromium/sync/signin/AccountManagerHelper.java (right): https://codereview.chromium.org/1504283002/diff/60001/sync/android/java/src/o... sync/android/java/src/org/chromium/sync/signin/AccountManagerHelper.java:351: return null; Wouldn't this trigger the !success path of ConnectionRetry#attempt().AsyncTask#onPostExecute(T)? https://codereview.chromium.org/1504283002/diff/60001/sync/android/java/src/o... sync/android/java/src/org/chromium/sync/signin/AccountManagerHelper.java:420: || mNumTries.incrementAndGet() == MAX_TRIES I know it shouldn't be like that, but since we've had issues with GAIA before, as a safety measure, could we use >= here?
Yay! You also took care of https://code.google.com/p/chromium/issues/detail?id=535320 as well :) https://codereview.chromium.org/1504283002/diff/60001/sync/android/java/src/o... File sync/android/java/src/org/chromium/sync/signin/AccountManagerHelper.java (right): https://codereview.chromium.org/1504283002/diff/60001/sync/android/java/src/o... sync/android/java/src/org/chromium/sync/signin/AccountManagerHelper.java:340: * Removes an auth token from the AccountManager's cache. Clear the auth token in local cache with respect to the Chrome's Application Context. https://codereview.chromium.org/1504283002/diff/60001/sync/android/java/src/o... sync/android/java/src/org/chromium/sync/signin/AccountManagerHelper.java:344: if (!hasUseCredentialsPermission()) { This is no longer required now :) https://codereview.chromium.org/1504283002/diff/60001/sync/android/java/src/o... sync/android/java/src/org/chromium/sync/signin/AccountManagerHelper.java:347: if (authToken != null && !authToken.isEmpty()) { Early return or throw IllegalArgumentException here?
Nice, thanks! https://codereview.chromium.org/1504283002/diff/60001/sync/android/java/src/o... File sync/android/java/src/org/chromium/sync/signin/AccountManagerDelegate.java (right): https://codereview.chromium.org/1504283002/diff/60001/sync/android/java/src/o... sync/android/java/src/org/chromium/sync/signin/AccountManagerDelegate.java:34: * @exception AuthException Indicates a failure in fetching the auth token perhaps due to a Nit: Prefer @throws over @exception (they are equivalent, but the former seems to be what is used exclusively in our Java code). https://codereview.chromium.org/1504283002/diff/60001/sync/android/java/src/o... File sync/android/java/src/org/chromium/sync/signin/AccountManagerHelper.java (right): https://codereview.chromium.org/1504283002/diff/60001/sync/android/java/src/o... sync/android/java/src/org/chromium/sync/signin/AccountManagerHelper.java:368: void onSuccess(T result); Document these methods if they're public.
Description was changed from ========== [Sync] Switch to using GoogleAuthUtil.clearToken(). GoogleAuthUtil has much better failure semantics, so this should clear up some of the runtime exceptions AccountManager.invalidateAuthToken() was producing. Since invalidateToken() now throws exceptions when things go wrong, this change isolates the network retry logic already existing in AccountManagerHelper and shares it between getToken and invalidateToken(). BUG=418809 ========== to ========== [Sync] Switch to using GoogleAuthUtil.clearToken(). GoogleAuthUtil has much better failure semantics, so this should clear up some of the runtime exceptions AccountManager.invalidateAuthToken() was producing. Since invalidateToken() now throws exceptions when things go wrong, this change isolates the network retry logic already existing in AccountManagerHelper and shares it between getToken and invalidateToken(). BUG=418809,535320 ==========
https://codereview.chromium.org/1504283002/diff/60001/sync/android/java/src/o... File sync/android/java/src/org/chromium/sync/signin/AccountManagerDelegate.java (right): https://codereview.chromium.org/1504283002/diff/60001/sync/android/java/src/o... sync/android/java/src/org/chromium/sync/signin/AccountManagerDelegate.java:34: * @exception AuthException Indicates a failure in fetching the auth token perhaps due to a On 2015/12/09 11:45:56, Bernhard Bauer wrote: > Nit: Prefer @throws over @exception (they are equivalent, but the former seems > to be what is used exclusively in our Java code). Done. https://codereview.chromium.org/1504283002/diff/60001/sync/android/java/src/o... sync/android/java/src/org/chromium/sync/signin/AccountManagerDelegate.java:43: * to a network connectivity issue, for example. On 2015/12/09 02:50:27, nyquist wrote: > I'm not sure this comment makes sense. Does invalidateAuthToken ever access the > network? Though it's fine for you to mention that it's transient, I think I'd > just remove the example part. Done. https://codereview.chromium.org/1504283002/diff/60001/sync/android/java/src/o... File sync/android/java/src/org/chromium/sync/signin/AccountManagerHelper.java (right): https://codereview.chromium.org/1504283002/diff/60001/sync/android/java/src/o... sync/android/java/src/org/chromium/sync/signin/AccountManagerHelper.java:340: * Removes an auth token from the AccountManager's cache. On 2015/12/09 04:45:34, knn wrote: > Clear the auth token in local cache with respect to the Chrome's Application > Context. Done. https://codereview.chromium.org/1504283002/diff/60001/sync/android/java/src/o... sync/android/java/src/org/chromium/sync/signin/AccountManagerHelper.java:344: if (!hasUseCredentialsPermission()) { On 2015/12/09 04:45:34, knn wrote: > This is no longer required now :) Done. https://codereview.chromium.org/1504283002/diff/60001/sync/android/java/src/o... sync/android/java/src/org/chromium/sync/signin/AccountManagerHelper.java:347: if (authToken != null && !authToken.isEmpty()) { On 2015/12/09 04:45:34, knn wrote: > Early return or throw IllegalArgumentException here? Done. https://codereview.chromium.org/1504283002/diff/60001/sync/android/java/src/o... sync/android/java/src/org/chromium/sync/signin/AccountManagerHelper.java:351: return null; On 2015/12/09 02:50:27, nyquist wrote: > Wouldn't this trigger the !success path of > ConnectionRetry#attempt().AsyncTask#onPostExecute(T)? Aaaaah crap you're right. I changed this from a Boolean as an afterthought and didn't notice :( Thank you! https://codereview.chromium.org/1504283002/diff/60001/sync/android/java/src/o... sync/android/java/src/org/chromium/sync/signin/AccountManagerHelper.java:368: void onSuccess(T result); On 2015/12/09 11:45:56, Bernhard Bauer wrote: > Document these methods if they're public. It's a private interface; they aren't public (only accessible within AccountManagerHelper). Also they're very straightforward. https://codereview.chromium.org/1504283002/diff/60001/sync/android/java/src/o... sync/android/java/src/org/chromium/sync/signin/AccountManagerHelper.java:420: || mNumTries.incrementAndGet() == MAX_TRIES On 2015/12/09 02:50:27, nyquist wrote: > I know it shouldn't be like that, but since we've had issues with GAIA before, > as a safety measure, could we use >= here? Done.
https://codereview.chromium.org/1504283002/diff/80001/sync/android/java/src/o... File sync/android/java/src/org/chromium/sync/signin/AccountManagerHelper.java (left): https://codereview.chromium.org/1504283002/diff/80001/sync/android/java/src/o... sync/android/java/src/org/chromium/sync/signin/AccountManagerHelper.java:342: private boolean hasUseCredentialsPermission() { The official webview and chrome APK targets do have 23 as their android:targetSdkVersion="23" in their manifest, but there is code in chromium that does not yet. I'm a little bit worried about removing this check already. How about just adding a TODO to remove it in the future instead? $ git grep targetSdkVersion FTW...
https://codereview.chromium.org/1504283002/diff/80001/sync/android/java/src/o... File sync/android/java/src/org/chromium/sync/signin/AccountManagerHelper.java (left): https://codereview.chromium.org/1504283002/diff/80001/sync/android/java/src/o... sync/android/java/src/org/chromium/sync/signin/AccountManagerHelper.java:342: private boolean hasUseCredentialsPermission() { On 2015/12/09 20:12:20, nyquist wrote: > The official webview and chrome APK targets do have 23 as their > android:targetSdkVersion="23" in their manifest, but there is code in chromium > that does not yet. > > I'm a little bit worried about removing this check already. How about just > adding a TODO to remove it in the future instead? > > $ git grep targetSdkVersion > FTW... Done.
Thanks! lgtm % question https://codereview.chromium.org/1504283002/diff/80001/sync/android/java/src/o... File sync/android/java/src/org/chromium/sync/signin/AccountManagerHelper.java (left): https://codereview.chromium.org/1504283002/diff/80001/sync/android/java/src/o... sync/android/java/src/org/chromium/sync/signin/AccountManagerHelper.java:342: private boolean hasUseCredentialsPermission() { On 2015/12/09 20:12:20, nyquist wrote: > The official webview and chrome APK targets do have 23 as their > android:targetSdkVersion="23" in their manifest, but there is code in chromium > that does not yet. > > I'm a little bit worried about removing this check already. How about just > adding a TODO to remove it in the future instead? > > $ git grep targetSdkVersion > FTW... What has the targetSdkVersion to do with this, we are removing this because GoogleAuthUtil.clearToken() does not require that permission right? https://developers.google.com/android/reference/com/google/android/gms/auth/G..., java.lang.String)
lgtm. sorry for the USE_CREDENTIALS stuff :-/ https://codereview.chromium.org/1504283002/diff/80001/sync/android/java/src/o... File sync/android/java/src/org/chromium/sync/signin/AccountManagerHelper.java (left): https://codereview.chromium.org/1504283002/diff/80001/sync/android/java/src/o... sync/android/java/src/org/chromium/sync/signin/AccountManagerHelper.java:342: private boolean hasUseCredentialsPermission() { On 2015/12/10 07:32:43, knn wrote: > On 2015/12/09 20:12:20, nyquist wrote: > > The official webview and chrome APK targets do have 23 as their > > android:targetSdkVersion="23" in their manifest, but there is code in chromium > > that does not yet. > > > > I'm a little bit worried about removing this check already. How about just > > adding a TODO to remove it in the future instead? > > > > $ git grep targetSdkVersion > > FTW... > > What has the targetSdkVersion to do with this, we are removing this because > GoogleAuthUtil.clearToken() does not require that permission right? > https://developers.google.com/android/reference/com/google/android/gms/auth/G..., > java.lang.String) Doh! :-/
https://codereview.chromium.org/1504283002/diff/80001/sync/android/java/src/o... File sync/android/java/src/org/chromium/sync/signin/AccountManagerHelper.java (left): https://codereview.chromium.org/1504283002/diff/80001/sync/android/java/src/o... sync/android/java/src/org/chromium/sync/signin/AccountManagerHelper.java:342: private boolean hasUseCredentialsPermission() { On 2015/12/10 18:44:40, nyquist wrote: > On 2015/12/10 07:32:43, knn wrote: > > On 2015/12/09 20:12:20, nyquist wrote: > > > The official webview and chrome APK targets do have 23 as their > > > android:targetSdkVersion="23" in their manifest, but there is code in > chromium > > > that does not yet. > > > > > > I'm a little bit worried about removing this check already. How about just > > > adding a TODO to remove it in the future instead? > > > > > > $ git grep targetSdkVersion > > > FTW... > > > > What has the targetSdkVersion to do with this, we are removing this because > > GoogleAuthUtil.clearToken() does not require that permission right? > > > https://developers.google.com/android/reference/com/google/android/gms/auth/G..., > > java.lang.String) > > Doh! :-/ Thanks for clearing this up Khannan!
The CQ bit was checked by maxbogue@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from nyquist@chromium.org, knn@chromium.org Link to the patchset: https://codereview.chromium.org/1504283002/#ps120001 (title: "Remove USE_CREDENTIALS check again :)")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1504283002/120001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1504283002/120001
Message was sent while issue was closed.
Description was changed from ========== [Sync] Switch to using GoogleAuthUtil.clearToken(). GoogleAuthUtil has much better failure semantics, so this should clear up some of the runtime exceptions AccountManager.invalidateAuthToken() was producing. Since invalidateToken() now throws exceptions when things go wrong, this change isolates the network retry logic already existing in AccountManagerHelper and shares it between getToken and invalidateToken(). BUG=418809,535320 ========== to ========== [Sync] Switch to using GoogleAuthUtil.clearToken(). GoogleAuthUtil has much better failure semantics, so this should clear up some of the runtime exceptions AccountManager.invalidateAuthToken() was producing. Since invalidateToken() now throws exceptions when things go wrong, this change isolates the network retry logic already existing in AccountManagerHelper and shares it between getToken and invalidateToken(). BUG=418809,535320 ==========
Message was sent while issue was closed.
Committed patchset #7 (id:120001)
Message was sent while issue was closed.
Description was changed from ========== [Sync] Switch to using GoogleAuthUtil.clearToken(). GoogleAuthUtil has much better failure semantics, so this should clear up some of the runtime exceptions AccountManager.invalidateAuthToken() was producing. Since invalidateToken() now throws exceptions when things go wrong, this change isolates the network retry logic already existing in AccountManagerHelper and shares it between getToken and invalidateToken(). BUG=418809,535320 ========== to ========== [Sync] Switch to using GoogleAuthUtil.clearToken(). GoogleAuthUtil has much better failure semantics, so this should clear up some of the runtime exceptions AccountManager.invalidateAuthToken() was producing. Since invalidateToken() now throws exceptions when things go wrong, this change isolates the network retry logic already existing in AccountManagerHelper and shares it between getToken and invalidateToken(). BUG=418809,535320 Committed: https://crrev.com/cbac98d0772350fe474971c09fd3332db7efebfa Cr-Commit-Position: refs/heads/master@{#364474} ==========
Message was sent while issue was closed.
Patchset 7 (id:??) landed as https://crrev.com/cbac98d0772350fe474971c09fd3332db7efebfa Cr-Commit-Position: refs/heads/master@{#364474} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
