Need to invalidate auth token after receiving 401 error code |
||||
Issue descriptionFollow up to b/110533555. It appears that I have a chrome canary client that is always using the same ntp_snippets oauth access token. Even across restarts, the client uses the same access token, which always results in the server returning a 401 due to the token being expired. From https://developer.android.com/reference/android/accounts/AccountManager it looks like perhaps we need to be forcing the android auth manager to invalidate the access token. Looking at the sync code, it appears we do just that every time we fetch a new token: https://cs.chromium.org/chromium/src/components/browser_sync/sync_auth_manager.cc?sq=package:chromium&g=0&l=267 The Zine code likely needs to do the same. Mihai, does that sound right to you? This makes me wonder how this ever worked in the first place. When does the Chrome identity manager automatically invalidate tokens?
,
Jul 2
,
Jul 3
On non-Android platforms token service caches token expiration time along with the token and uses it to decide when to request new one (https://cs.chromium.org/chromium/src/google_apis/gaia/oauth2_token_service.cc?sq=package:chromium&dr&g=0&l=681).
,
Jul 6
I don't think there is any auto-expire built into the Android Account Manager setup. It might be in the Google Play services authenticator (that backs the AccountManager for the "com.google" authority), but from the symptoms shown here, I don't think that's the case. I.e. we'd need to manually invoke invalidateToken(...) or use the getNewToken(...) functionality. It is common (required?) to OAuth2 to provide an expiration timestamp, but it's not made available from the Android auth APIs.
,
Jul 10
Hey Patrick, anh update on this bug?
,
Jul 10
,
Jul 11
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/bbc62fce2681c25cf09a88982ca8b50202ce7e14 commit bbc62fce2681c25cf09a88982ca8b50202ce7e14 Author: Patrick Noland <pnoland@google.com> Date: Wed Jul 11 18:34:38 2018 [Zine]Invalidate auth token on 401 Bug: 859684 Change-Id: Ia97912a688af2080aa139138a2195bdbce949932 Reviewed-on: https://chromium-review.googlesource.com/1133428 Reviewed-by: Filip Gorski <fgorski@chromium.org> Commit-Queue: Patrick Noland <pnoland@chromium.org> Cr-Commit-Position: refs/heads/master@{#574255} [modify] https://crrev.com/bbc62fce2681c25cf09a88982ca8b50202ce7e14/components/ntp_snippets/remote/json_request.cc [modify] https://crrev.com/bbc62fce2681c25cf09a88982ca8b50202ce7e14/components/ntp_snippets/remote/json_request.h [modify] https://crrev.com/bbc62fce2681c25cf09a88982ca8b50202ce7e14/components/ntp_snippets/remote/remote_suggestions_fetcher_impl.cc [modify] https://crrev.com/bbc62fce2681c25cf09a88982ca8b50202ce7e14/components/ntp_snippets/remote/remote_suggestions_fetcher_impl.h [modify] https://crrev.com/bbc62fce2681c25cf09a88982ca8b50202ce7e14/components/ntp_snippets/remote/remote_suggestions_fetcher_impl_unittest.cc [modify] https://crrev.com/bbc62fce2681c25cf09a88982ca8b50202ce7e14/tools/metrics/histograms/enums.xml
,
Jul 12
Fixed for Zine. Per https://crbug.com/609084 we should consider putting invalidation and retry logic into a base class so that this isn't a gotcha for everyone doing a similar thing.
,
Jul 12
Great, thanks Patrick! And agreed about using a base class. Is it worth filing a bug about that? It probably affects the feed migration work too, right?
,
Jul 12
I think https://crbug.com/609084 actually captures the context and remaining work fairly well. I'll clean it up a little and assign it to myself.
,
Aug 7
It looks like this landed in 69.0.3489.0. There are a number of user reports from 67 and 68. We should keep an eye on this in the feedback reports while we wait for M69 to push to stable. |
||||
►
Sign in to add a comment |
||||
Comment 1 by zea@chromium.org
, Jul 2