Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2582573002:
Signin/OAuth: Create an AccessTokenFetcher helper class (Closed)
|
Created:
4 years ago by Marc Treib Modified:
3 years, 10 months ago CC:
chromium-reviews Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionSignin/OAuth: Create an AccessTokenFetcher helper class
Reliably obtaining an OAuth2 access token from OAuth2TokenService is
surprisingly complicated, since there are many special cases to handle
(e.g. auth currently in progress, or auth completed but refresh token
not loaded yet, ...). Various classes implement some subset of these
edge cases.
This CL introduces a new helper class that encapsulates all this, and
adds a first usage as an example (in SuggestionsService).
Partial list of additional classes that can be made simpler and more
robust by using the new helper:
- NTPSnippetsFetcher
- FamilyInfoFetcher
- SafeSearchURLReporter
- PermissionRequestCreatorApiary
BUG=609084
Review-Url: https://codereview.chromium.org/2582573002
Cr-Commit-Position: refs/heads/master@{#446678}
Committed: https://chromium.googlesource.com/chromium/src/+/4c8c6cfe37c424387e26278371125cccbd16d5f3
Patch Set 1 #Patch Set 2 : Tests! #Patch Set 3 : Use in SuggestionsService; fixes #Patch Set 4 : AuthInProgress test #
Total comments: 32
Patch Set 5 : review #Patch Set 6 : ChromeOS, try2 #Patch Set 7 : ChromeOS, try3 #Patch Set 8 : ChromeOS, for realz now #
Total comments: 3
Patch Set 9 : review2 #
Total comments: 8
Patch Set 10 : review3 #Patch Set 11 : ChromeOS... #Patch Set 12 : rebase #Patch Set 13 : return AuthErrors #
Total comments: 20
Patch Set 14 : review4 #
Total comments: 2
Patch Set 15 : review5 #
Total comments: 1
Messages
Total messages: 67 (41 generated)
The CQ bit was checked by treib@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by treib@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
Description was changed from ========== Signin/OAuth: Create an AccessTokenFetcher helper class Reliably obtaining an OAuth2 access token from OAuth2TokenService is surprisingly complicated, since there are many special cases to handle (e.g. auth currently in progress, or auth completed but refresh token not loaded yet, ...). Various classes implement some subset of these edge cases. This CL introduces a new helper class that encapsulates all this. Partial list of classes that can be simplified (and made more robust) by using the new helper: - SuggestionsService - NTPSnippetsFetcher - FamilyInfoFetcher - SafeSearchURLReporter - PermissionRequestCreatorApiary BUG=609084 ========== to ========== Signin/OAuth: Create an AccessTokenFetcher helper class Reliably obtaining an OAuth2 access token from OAuth2TokenService is surprisingly complicated, since there are many special cases to handle (e.g. auth currently in progress, or auth completed but refresh token not loaded yet, ...). Various classes implement some subset of these edge cases. This CL introduces a new helper class that encapsulates all this, and adds a first usage as an example (in SuggestionsService). Partial list of additional classes that can be simplified (and made more robust) by using the new helper: - NTPSnippetsFetcher - FamilyInfoFetcher - SafeSearchURLReporter - PermissionRequestCreatorApiary BUG=609084 ==========
Patchset #3 (id:40001) has been deleted
Patchset #3 (id:60001) has been deleted
The CQ bit was checked by treib@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Description was changed from ========== Signin/OAuth: Create an AccessTokenFetcher helper class Reliably obtaining an OAuth2 access token from OAuth2TokenService is surprisingly complicated, since there are many special cases to handle (e.g. auth currently in progress, or auth completed but refresh token not loaded yet, ...). Various classes implement some subset of these edge cases. This CL introduces a new helper class that encapsulates all this, and adds a first usage as an example (in SuggestionsService). Partial list of additional classes that can be simplified (and made more robust) by using the new helper: - NTPSnippetsFetcher - FamilyInfoFetcher - SafeSearchURLReporter - PermissionRequestCreatorApiary BUG=609084 ========== to ========== Signin/OAuth: Create an AccessTokenFetcher helper class Reliably obtaining an OAuth2 access token from OAuth2TokenService is surprisingly complicated, since there are many special cases to handle (e.g. auth currently in progress, or auth completed but refresh token not loaded yet, ...). Various classes implement some subset of these edge cases. This CL introduces a new helper class that encapsulates all this, and adds a first usage as an example (in SuggestionsService). Partial list of additional classes that can be made simpler and more robust by using the new helper: - NTPSnippetsFetcher - FamilyInfoFetcher - SafeSearchURLReporter - PermissionRequestCreatorApiary BUG=609084 ==========
treib@chromium.org changed reviewers: + rogerta@chromium.org
Hi Roger! Could you take a first look and check: - Do you agree that this helper class makes sense? - Is this a good place and name for it? Thanks!
rogerta@chromium.org changed reviewers: + msarda@chromium.org
Hi Marc, I'm no longer working on signin, so passing off review to new owner Mihai.
msarda@chromium.org changed reviewers: + anthonyvd@chromium.org
CC+ Anthony Hi Marc, I personally think the fetcher that you are proposing makes a lot of sense. I am attending a conference today and tomorrow so I did not have a change to do a thorough review. I think overall this class makes sense. The name and the directory seem correct to me. Thank you, Mihai https://codereview.chromium.org/2582573002/diff/100001/components/signin/core... File components/signin/core/browser/access_token_fetcher.cc (right): https://codereview.chromium.org/2582573002/diff/100001/components/signin/core... components/signin/core/browser/access_token_fetcher.cc:47: // GoogleSigninSucceeded/Failed (from SigninManagerBase::Observer) first? I agree this is a tricky question: currently the refresh token is updated after the authenticated account id is set - this means that GoogleSigninSucceeded is always fired before a OnRefreshTokenAvailable. So at the current state listening for GoogleSigninSucceded may not be needed. However, if the sign-in fails, then the OnRefreshTokenAvailable will not be called, which in turn means that you will not call the callback if you are not listening for GoogleSigninFailed. So I would say you would probably need to wait for GoogleSigninSucceeded/Failed as well. https://codereview.chromium.org/2582573002/diff/100001/components/signin/core... components/signin/core/browser/access_token_fetcher.cc:54: base::ThreadTaskRunnerHandle::Get()->PostTask( It is not clear to me how you would use this API: would you expect the caller to create an AccessTokenFetcher when the user is not authenticated (even when auth is not even in progress?)? I mean in that case (which is a synchronous call by the way that always ends in error), wouldn't it be easy to say the caller should never create an AccessTokenFetcher if the profiles is not authenticated and remove this if branch? https://codereview.chromium.org/2582573002/diff/100001/components/signin/core... components/signin/core/browser/access_token_fetcher.cc:61: callback_.Run(std::string()); We should to pass an error to the callback here as otherwise it will be impossible to figure out on the caller what went wrong when requesting an access token. https://codereview.chromium.org/2582573002/diff/100001/components/signin/core... components/signin/core/browser/access_token_fetcher.cc:61: callback_.Run(std::string()); We should use a callback that may be called only once (see https://cs.chromium.org/chromium/src/base/callback_forward.h?rcl=0&l=36 ) https://codereview.chromium.org/2582573002/diff/100001/components/signin/core... components/signin/core/browser/access_token_fetcher.cc:66: DCHECK(waiting_for_refresh_token_); Just to make sure what I said in my previous comment stays true: please add a DCHECK(signin_manager_->IsAuthenticated()) here. https://codereview.chromium.org/2582573002/diff/100001/components/signin/core... components/signin/core/browser/access_token_fetcher.cc:72: Nit: I would set waiting_for_refresh_token_ before calling token_service_->RemoveObserver(). https://codereview.chromium.org/2582573002/diff/100001/components/signin/core... components/signin/core/browser/access_token_fetcher.cc:116: StartAccessTokenRequest(); It is not clear to me yet why we need to retry. I have not seen a similar usecase of the access token API. I think the fact that we need to always retry on a legitimate error response a bad practice and we should fix the API to avoid such hacks in the code. https://codereview.chromium.org/2582573002/diff/100001/components/signin/core... components/signin/core/browser/access_token_fetcher.cc:121: callback_.Run(std::string()); We need an API that would allow us to pass the error to the caller - the caller should decide whether it needs to retry, use backoff or something else. https://codereview.chromium.org/2582573002/diff/100001/components/signin/core... File components/signin/core/browser/access_token_fetcher.h (right): https://codereview.chromium.org/2582573002/diff/100001/components/signin/core... components/signin/core/browser/access_token_fetcher.h:36: const TokenCallback& callback); I would expect 2 things: * either the token callback takes 2 arguments (std:sting token, GaiaAuthError error). or * we have 2 callbacks (one for the success and one for the error cases). I think for simplicity, I would prefer the case with a callback with 2 arguments. https://codereview.chromium.org/2582573002/diff/100001/components/signin/core... components/signin/core/browser/access_token_fetcher.h:43: void NotSignedInCallback(); This method names is very unclear. https://codereview.chromium.org/2582573002/diff/100001/components/signin/core... components/signin/core/browser/access_token_fetcher.h:49: void StartAccessTokenRequest(); Move this method next to the NotSignedInCallback as it is a private method that is not overridden from a superclass. https://codereview.chromium.org/2582573002/diff/100001/components/signin/core... components/signin/core/browser/access_token_fetcher.h:64: bool waiting_for_refresh_token_ = false; I think we prefer values to be initialized on the constructor.
The CQ bit was checked by treib@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
treib@chromium.org changed reviewers: + jkrcal@chromium.org
PTAL again! Jan, there's one specific question there for you. https://codereview.chromium.org/2582573002/diff/100001/components/signin/core... File components/signin/core/browser/access_token_fetcher.cc (right): https://codereview.chromium.org/2582573002/diff/100001/components/signin/core... components/signin/core/browser/access_token_fetcher.cc:47: // GoogleSigninSucceeded/Failed (from SigninManagerBase::Observer) first? On 2017/01/18 21:55:19, msarda wrote: > I agree this is a tricky question: currently the refresh token is updated after > the authenticated account id is set - this means that GoogleSigninSucceeded is > always fired before a OnRefreshTokenAvailable. So at the current state listening > for GoogleSigninSucceded may not be needed. > > However, if the sign-in fails, then the OnRefreshTokenAvailable will not be > called, which in turn means that you will not call the callback if you are not > listening for GoogleSigninFailed. > > So I would say you would probably need to wait for GoogleSigninSucceeded/Failed > as well. Done. However, I'm still not super confident that this reliably handles all cases now. I guess the only "safe" option would be to also add a timeout to the whole thing... not sure that's worth the hassle. https://codereview.chromium.org/2582573002/diff/100001/components/signin/core... components/signin/core/browser/access_token_fetcher.cc:54: base::ThreadTaskRunnerHandle::Get()->PostTask( On 2017/01/18 21:55:19, msarda wrote: > It is not clear to me how you would use this API: would you expect the caller to > create an AccessTokenFetcher when the user is not authenticated (even when auth > is not even in progress?)? I mean in that case (which is a synchronous call by > the way that always ends in error), wouldn't it be easy to say the caller should > never create an AccessTokenFetcher if the profiles is not authenticated and > remove this if branch? There are some (future) clients of this class that can work in authenticated or non-authenticated mode (but prefer authenticated of course). IMO it's convenient for them to just say "get me an access token", and see if one comes back, rather than checking IsAuthenticated/AuthInProgress/whatever else manually. I could probably be convinced otherwise though :) https://codereview.chromium.org/2582573002/diff/100001/components/signin/core... components/signin/core/browser/access_token_fetcher.cc:61: callback_.Run(std::string()); On 2017/01/18 21:55:19, msarda wrote: > We should use a callback that may be called only once (see > https://cs.chromium.org/chromium/src/base/callback_forward.h?rcl=0&l=36 ) I actually did use OnceCallback in a previous iteration. The issue is that it's very new and not widely supported yet, e.g. all the PostTask methods only take RepeatingCallbacks. If it turns out that we can get away without PostTask etc, then I'm happy to switch back to OnceCallback. https://codereview.chromium.org/2582573002/diff/100001/components/signin/core... components/signin/core/browser/access_token_fetcher.cc:61: callback_.Run(std::string()); On 2017/01/18 21:55:19, msarda wrote: > We should to pass an error to the callback here as otherwise it will be > impossible to figure out on the caller what went wrong when requesting an access > token. So far, none of the callers I'm aware of care about what exactly went wrong, and I don't see what they would do about it anyway (log an error message somewhere?). So I went the lazy route :) But maybe it's the nicer API anyway to return some sort of error code? https://codereview.chromium.org/2582573002/diff/100001/components/signin/core... components/signin/core/browser/access_token_fetcher.cc:66: DCHECK(waiting_for_refresh_token_); On 2017/01/18 21:55:19, msarda wrote: > Just to make sure what I said in my previous comment stays true: please add a > DCHECK(signin_manager_->IsAuthenticated()) here. Done. https://codereview.chromium.org/2582573002/diff/100001/components/signin/core... components/signin/core/browser/access_token_fetcher.cc:72: On 2017/01/18 21:55:18, msarda wrote: > Nit: I would set waiting_for_refresh_token_ before calling > token_service_->RemoveObserver(). Done. https://codereview.chromium.org/2582573002/diff/100001/components/signin/core... components/signin/core/browser/access_token_fetcher.cc:116: StartAccessTokenRequest(); On 2017/01/18 21:55:19, msarda wrote: > It is not clear to me yet why we need to retry. I have not seen a similar > usecase of the access token API. > > I think the fact that we need to always retry on a legitimate error response a > bad practice and we should fix the API to avoid such hacks in the code. We had a case on Android where sometimes during startup, the first request would get canceled. IIRC, it had something to do with the refresh token getting reloaded. Agreed it's a hack, but at the time, nobody could figure out a nicer solution. +jrkcal, do you remember more details? https://codereview.chromium.org/2582573002/diff/100001/components/signin/core... File components/signin/core/browser/access_token_fetcher.h (right): https://codereview.chromium.org/2582573002/diff/100001/components/signin/core... components/signin/core/browser/access_token_fetcher.h:36: const TokenCallback& callback); On 2017/01/18 21:55:19, msarda wrote: > I would expect 2 things: > * either the token callback takes 2 arguments (std:sting token, GaiaAuthError > error). > or > * we have 2 callbacks (one for the success and one for the error cases). > > I think for simplicity, I would prefer the case with a callback with 2 > arguments. I assume you mean GoogleServiceAuthError? The issue is that not all the error cases here can be properly expressed by that, e.g. the "you're not signed in" case (if we're keeping that), or the "we didn't get a refresh token" case. https://codereview.chromium.org/2582573002/diff/100001/components/signin/core... components/signin/core/browser/access_token_fetcher.h:43: void NotSignedInCallback(); On 2017/01/18 21:55:19, msarda wrote: > This method names is very unclear. Renamed to "RunCallbackNotSignedIn" - better? https://codereview.chromium.org/2582573002/diff/100001/components/signin/core... components/signin/core/browser/access_token_fetcher.h:49: void StartAccessTokenRequest(); On 2017/01/18 21:55:19, msarda wrote: > Move this method next to the NotSignedInCallback as it is a private method that > is not overridden from a superclass. Done. https://codereview.chromium.org/2582573002/diff/100001/components/signin/core... components/signin/core/browser/access_token_fetcher.h:64: bool waiting_for_refresh_token_ = false; On 2017/01/18 21:55:19, msarda wrote: > I think we prefer values to be initialized on the constructor. AFAIK there's no general preference for one over the other. But I don't mind either way, so done.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
https://codereview.chromium.org/2582573002/diff/100001/components/signin/core... File components/signin/core/browser/access_token_fetcher.cc (right): https://codereview.chromium.org/2582573002/diff/100001/components/signin/core... components/signin/core/browser/access_token_fetcher.cc:116: StartAccessTokenRequest(); On 2017/01/19 15:03:55, Marc Treib wrote: > On 2017/01/18 21:55:19, msarda wrote: > > It is not clear to me yet why we need to retry. I have not seen a similar > > usecase of the access token API. > > > > I think the fact that we need to always retry on a legitimate error response a > > bad practice and we should fix the API to avoid such hacks in the code. > > We had a case on Android where sometimes during startup, the first request would > get canceled. IIRC, it had something to do with the refresh token getting > reloaded. > Agreed it's a hack, but at the time, nobody could figure out a nicer solution. > +jrkcal, do you remember more details? I agree it is ugly. Reasoning for the retry: https://groups.google.com/a/google.com/d/msg/zine-eng/TqMf2u1Pn9Y/1EelPl3XAQAJ
Hi! I'm also at a conference so I might not be able to thoroughly review this until next week. Please let me know if there's a rush.
On 2017/01/19 20:32:03, anthonyvd wrote: > Hi! I'm also at a conference so I might not be able to thoroughly review this > until next week. Please let me know if there's a rush. No rush at all! Also, it's probably not necessary for both of you to review this in detail :)
Overall this looks like a great change to me, plus it adds more tests that exercise that code flow. I'll let Mihai do the more thorough review as the new owner of this code. Quick note that this only handles getting tokens for the primary account, which I think is the correct thing to do now but might need revisiting in the future when Mihai's Account Consistency proposal is implemented. Nothing important now, just throwing that out there. Are there any plans to change the other classes listed in the CL description? Thanks a ton, lgtm!
On 2017/01/23 14:31:00, anthonyvd wrote: > Overall this looks like a great change to me, plus it adds more tests that > exercise that code flow. I'll let Mihai do the more thorough review as the new > owner of this code. > > Quick note that this only handles getting tokens for the primary account, which > I think is the correct thing to do now but might need revisiting in the future > when Mihai's Account Consistency proposal is implemented. Nothing important now, > just throwing that out there. > > Are there any plans to change the other classes listed in the CL description? > > Thanks a ton, lgtm! Thanks! I'm not very familiar with the plans around Account Consistency. Would there be any case where you'd want OAuth tokens for secondary accounts? Either way, I'd say we address that once it comes up :) I do have plans to change the other classes, but I would've done that in separate CLs - IMO this one is large enough, and one example seems sufficient for the initial CL.
I have actually done this review on Friday and forgot to push the publish button (sorry). I agree this API makes a lot of sense, however as we want to use this extensively it is worth spending the time to get it right from the begining. Thank you so much for doing this. https://codereview.chromium.org/2582573002/diff/100001/components/signin/core... File components/signin/core/browser/access_token_fetcher.cc (right): https://codereview.chromium.org/2582573002/diff/100001/components/signin/core... components/signin/core/browser/access_token_fetcher.cc:47: // GoogleSigninSucceeded/Failed (from SigninManagerBase::Observer) first? On 2017/01/19 15:03:55, Marc Treib wrote: > On 2017/01/18 21:55:19, msarda wrote: > > I agree this is a tricky question: currently the refresh token is updated > after > > the authenticated account id is set - this means that GoogleSigninSucceeded is > > always fired before a OnRefreshTokenAvailable. So at the current state > listening > > for GoogleSigninSucceded may not be needed. > > > > However, if the sign-in fails, then the OnRefreshTokenAvailable will not be > > called, which in turn means that you will not call the callback if you are not > > listening for GoogleSigninFailed. > > > > So I would say you would probably need to wait for > GoogleSigninSucceeded/Failed > > as well. > > Done. > However, I'm still not super confident that this reliably handles all cases now. > I guess the only "safe" option would be to also add a timeout to the whole > thing... not sure that's worth the hassle. I think using a timeout is not a good API. This comment made me think that maybe we need a way to cancel the request (in the current implementation that is actually handled by the constructor as it unregisters from the token service, but the callback is not called in that case). We need to be explicit about this (explain this in the comments on the header file). https://codereview.chromium.org/2582573002/diff/100001/components/signin/core... components/signin/core/browser/access_token_fetcher.cc:61: callback_.Run(std::string()); On 2017/01/19 15:03:55, Marc Treib wrote: > On 2017/01/18 21:55:19, msarda wrote: > > We should to pass an error to the callback here as otherwise it will be > > impossible to figure out on the caller what went wrong when requesting an > access > > token. > > So far, none of the callers I'm aware of care about what exactly went wrong, and > I don't see what they would do about it anyway (log an error message > somewhere?). So I went the lazy route :) > But maybe it's the nicer API anyway to return some sort of error code? I think the current API is fine for discussion, but we need to add support for errors before this CL can be landed. https://codereview.chromium.org/2582573002/diff/100001/components/signin/core... components/signin/core/browser/access_token_fetcher.cc:116: StartAccessTokenRequest(); On 2017/01/19 15:48:45, jkrcal wrote: > On 2017/01/19 15:03:55, Marc Treib wrote: > > On 2017/01/18 21:55:19, msarda wrote: > > > It is not clear to me yet why we need to retry. I have not seen a similar > > > usecase of the access token API. > > > > > > I think the fact that we need to always retry on a legitimate error response > a > > > bad practice and we should fix the API to avoid such hacks in the code. > > > > We had a case on Android where sometimes during startup, the first request > would > > get canceled. IIRC, it had something to do with the refresh token getting > > reloaded. > > Agreed it's a hack, but at the time, nobody could figure out a nicer solution. > > +jrkcal, do you remember more details? > > I agree it is ugly. Reasoning for the retry: > https://groups.google.com/a/google.com/d/msg/zine-eng/TqMf2u1Pn9Y/1EelPl3XAQAJ Thank you for the pointer. I think we should not do the retry for all fetchers on all platforms, just because we have a limited use case on Android. I think this should at least be scoped to Android OS, with a comment and a link to this thread. https://codereview.chromium.org/2582573002/diff/100001/components/signin/core... File components/signin/core/browser/access_token_fetcher.h (right): https://codereview.chromium.org/2582573002/diff/100001/components/signin/core... components/signin/core/browser/access_token_fetcher.h:36: const TokenCallback& callback); On 2017/01/19 15:03:56, Marc Treib wrote: > On 2017/01/18 21:55:19, msarda wrote: > > I would expect 2 things: > > * either the token callback takes 2 arguments (std:sting token, GaiaAuthError > > error). > > or > > * we have 2 callbacks (one for the success and one for the error cases). > > > > I think for simplicity, I would prefer the case with a callback with 2 > > arguments. > > I assume you mean GoogleServiceAuthError? The issue is that not all the error > cases here can be properly expressed by that, e.g. the "you're not signed in" > case (if we're keeping that), or the "we didn't get a refresh token" case. Yes, this is what I had in mind. I still think we need some API to pass the error to the caller here. https://codereview.chromium.org/2582573002/diff/180001/components/signin/core... File components/signin/core/browser/access_token_fetcher.cc (right): https://codereview.chromium.org/2582573002/diff/180001/components/signin/core... components/signin/core/browser/access_token_fetcher.cc:45: WaitForRefreshToken(); AuthInProgress is a state that I think we should ignore when we plan to fetch access tokens: * On mobile (iOS and Android), this does not exist * On desktop, this is basically the state between the user finishing signing in with Gaia and the state that sync is started. I would basically avoid treating the auth in progress state different than unauthenticated case for the following reason: if the service starts fetching access tokens at random moments, than it basically relies on chance to start when auth is in progress. I see 2 options to use the AccessTokenFetcher: 1. A service would listen for GoogleSignedIn events and then would start fetching access tokens using this fetcher. 2. A service wants to get an access token soon after the user signs in. In this case, this fetcher should listen for GoogleSignedIn and then start fetching an access token (this means removing RunCallbackNotSignedIn).
PTAL again! Returning an error code is still TBD, but the rest should be addressed. If you're otherwise okay with the API now, I'll add the error codes. Thanks! https://codereview.chromium.org/2582573002/diff/100001/components/signin/core... File components/signin/core/browser/access_token_fetcher.cc (right): https://codereview.chromium.org/2582573002/diff/100001/components/signin/core... components/signin/core/browser/access_token_fetcher.cc:47: // GoogleSigninSucceeded/Failed (from SigninManagerBase::Observer) first? On 2017/01/23 17:08:09, msarda wrote: > On 2017/01/19 15:03:55, Marc Treib wrote: > > On 2017/01/18 21:55:19, msarda wrote: > > > I agree this is a tricky question: currently the refresh token is updated > > after > > > the authenticated account id is set - this means that GoogleSigninSucceeded > is > > > always fired before a OnRefreshTokenAvailable. So at the current state > > listening > > > for GoogleSigninSucceded may not be needed. > > > > > > However, if the sign-in fails, then the OnRefreshTokenAvailable will not be > > > called, which in turn means that you will not call the callback if you are > not > > > listening for GoogleSigninFailed. > > > > > > So I would say you would probably need to wait for > > GoogleSigninSucceeded/Failed > > > as well. > > > > Done. > > However, I'm still not super confident that this reliably handles all cases > now. > > I guess the only "safe" option would be to also add a timeout to the whole > > thing... not sure that's worth the hassle. > > I think using a timeout is not a good API. > > This comment made me think that maybe we need a way to cancel the request (in > the current implementation that is actually handled by the constructor as it > unregisters from the token service, but the callback is not called in that > case). We need to be explicit about this (explain this in the comments on the > header file). Not calling the callback after we're destroyed is on purpose: The Fetcher *is* the request, you cancel by destroying the fetcher. I find that convenient for clients: They can have the Fetcher as a member (and bind the callback to a method), and if they're destroyed, they don't have to worry about canceling. The comment on the ctor in the header already mentions that the callback won't be called after destroying the instance. Any suggestions on how to make that clearer? https://codereview.chromium.org/2582573002/diff/100001/components/signin/core... components/signin/core/browser/access_token_fetcher.cc:116: StartAccessTokenRequest(); On 2017/01/23 17:08:09, msarda wrote: > On 2017/01/19 15:48:45, jkrcal wrote: > > On 2017/01/19 15:03:55, Marc Treib wrote: > > > On 2017/01/18 21:55:19, msarda wrote: > > > > It is not clear to me yet why we need to retry. I have not seen a similar > > > > usecase of the access token API. > > > > > > > > I think the fact that we need to always retry on a legitimate error > response > > a > > > > bad practice and we should fix the API to avoid such hacks in the code. > > > > > > We had a case on Android where sometimes during startup, the first request > > would > > > get canceled. IIRC, it had something to do with the refresh token getting > > > reloaded. > > > Agreed it's a hack, but at the time, nobody could figure out a nicer > solution. > > > +jrkcal, do you remember more details? > > > > I agree it is ugly. Reasoning for the retry: > > https://groups.google.com/a/google.com/d/msg/zine-eng/TqMf2u1Pn9Y/1EelPl3XAQAJ > > Thank you for the pointer. I think we should not do the retry for all fetchers > on all platforms, just because we have a limited use case on Android. > > I think this should at least be scoped to Android OS, with a comment and a link > to this thread. > The thread is not public unfortunately, so we can't link to it from Chromium. But I've tried to copy the relevant parts into a comment here. It sounds to me like one of the cases where a retry is required can happen on non-Android too (credentials updated), so I haven't put in "#ifdef ANDROID"s. https://codereview.chromium.org/2582573002/diff/100001/components/signin/core... components/signin/core/browser/access_token_fetcher.cc:121: callback_.Run(std::string()); On 2017/01/18 21:55:18, msarda wrote: > We need an API that would allow us to pass the error to the caller - the caller > should decide whether it needs to retry, use backoff or something else. I'd actually prefer if any retrying were handled within this class, so that clients don't have to worry about that. In the past, there were multiple instances where clients implemented retrying badly and caused issues. (I know because I wrote one of those bad clients :D) Passing an error might still make sense, but retry is IMO not a valid reason for it. https://codereview.chromium.org/2582573002/diff/180001/components/signin/core... File components/signin/core/browser/access_token_fetcher.cc (right): https://codereview.chromium.org/2582573002/diff/180001/components/signin/core... components/signin/core/browser/access_token_fetcher.cc:45: WaitForRefreshToken(); On 2017/01/23 17:08:09, msarda wrote: > AuthInProgress is a state that I think we should ignore when we plan to fetch > access tokens: > * On mobile (iOS and Android), this does not exist Ah, I didn't know that! I only knew about ChromeOS. > * On desktop, this is basically the state between the user finishing signing in > with Gaia and the state that sync is started. > > I would basically avoid treating the auth in progress state different than > unauthenticated case for the following reason: if the service starts fetching > access tokens at random moments, than it basically relies on chance to start > when auth is in progress. Okay, I see your reasoning. I removed the AuthInProgress handling. > I see 2 options to use the AccessTokenFetcher: > 1. A service would listen for GoogleSignedIn events and then would start > fetching access tokens using this fetcher. > 2. A service wants to get an access token soon after the user signs in. In this > case, this fetcher should listen for GoogleSignedIn and then start fetching an > access token (this means removing RunCallbackNotSignedIn). Hm. I was hoping that clients wouldn't have to worry about sign-in state, and could just rely on the Fetcher to return an empty token in the not-signed-in case. But I guess that makes the API for the Fetcher too weird. And it's not that much of a hassle to listen for signed-in events anyway. So, done.
https://codereview.chromium.org/2582573002/diff/100001/components/signin/core... File components/signin/core/browser/access_token_fetcher.cc (right): https://codereview.chromium.org/2582573002/diff/100001/components/signin/core... components/signin/core/browser/access_token_fetcher.cc:121: callback_.Run(std::string()); On 2017/01/24 10:17:32, Marc Treib wrote: > On 2017/01/18 21:55:18, msarda wrote: > > We need an API that would allow us to pass the error to the caller - the > caller > > should decide whether it needs to retry, use backoff or something else. > > I'd actually prefer if any retrying were handled within this class, so that > clients don't have to worry about that. > In the past, there were multiple instances where clients implemented retrying > badly and caused issues. (I know because I wrote one of those bad clients :D) > > Passing an error might still make sense, but retry is IMO not a valid reason for > it. I agree with this - we should hide the nasty things from the clients of this class. https://codereview.chromium.org/2582573002/diff/200001/components/signin/core... File components/signin/core/browser/access_token_fetcher.cc (right): https://codereview.chromium.org/2582573002/diff/200001/components/signin/core... components/signin/core/browser/access_token_fetcher.cc:59: StartAccessTokenRequest(); Optional nit: Let's use an early return here and remove the else branch. https://codereview.chromium.org/2582573002/diff/200001/components/signin/core... components/signin/core/browser/access_token_fetcher.cc:68: void AccessTokenFetcher::StartAccessTokenRequest() { Let's add a DCHECK that this method is called only when the token service has a refresh token for the authenticated account id. Let will allow us to catch cases when this method is called in an unexpected state. WDYT? https://codereview.chromium.org/2582573002/diff/200001/components/signin/core... components/signin/core/browser/access_token_fetcher.cc:81: DCHECK(signin_manager_->IsAuthenticated()); Nit: Move this DCHECK in the group of DCHECKs above. https://codereview.chromium.org/2582573002/diff/200001/components/signin/core... components/signin/core/browser/access_token_fetcher.cc:141: error.state() == GoogleServiceAuthError::State::REQUEST_CANCELED) { I think this case is a bit more tricky: 1. If the user signs out when we're waiting for an access token (the token was revoked), then all the existing access token requests will be cancelled (see https://cs.chromium.org/chromium/src/components/signin/core/browser/profile_o...). In that case, I do not think it is a good idea to start a new access token request. 2. If the user re authenticates (the OnRefreshTokenAvailable case), then all the existing access token requests will be cancelled (this matches your comment). In that case we would like to start a new access token request. The problem is how to distinguish between the 2 cases. I would say that what we could do here would be to start an access token request ONLY IF RefreshTokenAvailable(authenticated acount id) return true (this basically matches the case when the requests were cancelled after a OnRefreshTokenAvailable method was called).
Thanks for the review! Comments addressed, now hopefully the only remaining thing is returning an error code. https://codereview.chromium.org/2582573002/diff/200001/components/signin/core... File components/signin/core/browser/access_token_fetcher.cc (right): https://codereview.chromium.org/2582573002/diff/200001/components/signin/core... components/signin/core/browser/access_token_fetcher.cc:59: StartAccessTokenRequest(); On 2017/01/25 09:54:29, msarda wrote: > Optional nit: Let's use an early return here and remove the else branch. Done. Also done similarly in Start() above. https://codereview.chromium.org/2582573002/diff/200001/components/signin/core... components/signin/core/browser/access_token_fetcher.cc:68: void AccessTokenFetcher::StartAccessTokenRequest() { On 2017/01/25 09:54:29, msarda wrote: > Let's add a DCHECK that this method is called only when the token service has a > refresh token for the authenticated account id. Let will allow us to catch cases > when this method is called in an unexpected state. > WDYT? Yup, sounds reasonable. Done. https://codereview.chromium.org/2582573002/diff/200001/components/signin/core... components/signin/core/browser/access_token_fetcher.cc:81: DCHECK(signin_manager_->IsAuthenticated()); On 2017/01/25 09:54:29, msarda wrote: > Nit: Move this DCHECK in the group of DCHECKs above. Done. https://codereview.chromium.org/2582573002/diff/200001/components/signin/core... components/signin/core/browser/access_token_fetcher.cc:141: error.state() == GoogleServiceAuthError::State::REQUEST_CANCELED) { On 2017/01/25 09:54:29, msarda wrote: > I think this case is a bit more tricky: > 1. If the user signs out when we're waiting for an access token (the token was > revoked), then all the existing access token requests will be cancelled (see > https://cs.chromium.org/chromium/src/components/signin/core/browser/profile_o...). > In that case, I do not think it is a good idea to start a new access token > request. > 2. If the user re authenticates (the OnRefreshTokenAvailable case), then all the > existing access token requests will be cancelled (this matches your comment). In > that case we would like to start a new access token request. > > The problem is how to distinguish between the 2 cases. I would say that what we > could do here would be to start an access token request ONLY IF > RefreshTokenAvailable(authenticated acount id) return true (this basically > matches the case when the requests were cancelled after a > OnRefreshTokenAvailable method was called). Yup, that sounds very reasonable! I guess it doesn't cause any big harm to retry if the refresh token isn't there anymore, but let's try to keep the hacky workaround as contained as possible. I've updated the comment to explain this, please sanity check :) I've also added tests to make sure we don't retry in those cases.
https://codereview.chromium.org/2582573002/diff/180001/components/signin/core... File components/signin/core/browser/access_token_fetcher.cc (right): https://codereview.chromium.org/2582573002/diff/180001/components/signin/core... components/signin/core/browser/access_token_fetcher.cc:45: WaitForRefreshToken(); On 2017/01/24 10:17:33, Marc Treib wrote: > On 2017/01/23 17:08:09, msarda wrote: > > AuthInProgress is a state that I think we should ignore when we plan to fetch > > access tokens: > > * On mobile (iOS and Android), this does not exist > > Ah, I didn't know that! I only knew about ChromeOS. > > > * On desktop, this is basically the state between the user finishing signing > in > > with Gaia and the state that sync is started. > > > > I would basically avoid treating the auth in progress state different than > > unauthenticated case for the following reason: if the service starts fetching > > access tokens at random moments, than it basically relies on chance to start > > when auth is in progress. > > Okay, I see your reasoning. I removed the AuthInProgress handling. > > > I see 2 options to use the AccessTokenFetcher: > > 1. A service would listen for GoogleSignedIn events and then would start > > fetching access tokens using this fetcher. > > 2. A service wants to get an access token soon after the user signs in. In > this > > case, this fetcher should listen for GoogleSignedIn and then start fetching an > > access token (this means removing RunCallbackNotSignedIn). > > Hm. I was hoping that clients wouldn't have to worry about sign-in state, and > could just rely on the Fetcher to return an empty token in the not-signed-in > case. But I guess that makes the API for the Fetcher too weird. And it's not > that much of a hassle to listen for signed-in events anyway. So, done. Reviving an old comment thread: Are you positive that AuthInProgress doesn't exist on Android? The reason I'm asking is: One of the future clients of this class (which is used on Android only) does handle the AuthInProgress case, and I'd like to believe there was some reason for adding that code ;) Link: https://cs.chromium.org/chromium/src/components/ntp_snippets/remote/remote_su...
msarda@chromium.org changed reviewers: + gogerald@chromium.org
I have not really worked on sign-in on Chrome Android - I think Ganggui would know better than I do. Here is how I read the existing code: 1. AuthInProgress is true when !possibly_invalid_account_id_.empty() - see https://cs.chromium.org/chromium/src/components/signin/core/browser/signin_ma... 2. So the question is when possibly_invalid_account_id_ is non-empty 3. possibly_invalid_account_id_ is set in PrepareForSignin, CopyCredentialsFrom and in OnExternalSigninCompleted (See https://cs.chromium.org/chromium/src/components/signin/core/browser/signin_ma...) 4. On Android, we never call PrepareForSignin or CopyCrendetialsFrom (this is used when we create a new profile during sign-in). 5. This basically means that we only call OnExternalSigninCompleted when the user signs in https://cs.chromium.org/chromium/src/components/signin/core/browser/signin_ma... and https://cs.chromium.org/chromium/src/chrome/browser/android/signin/signin_man... 6. OnExternalSignin sets possibly_invalid_account_id_ and then synchronously calls OnSignIn and clears it before calling its observers with GoogleSigninSucceeded So I personally do not see when AuthInProgress would return true on Android for a caller. However, I do not think you need to go and change the users of this method.
On 2017/01/25 14:51:24, msarda wrote: > I have not really worked on sign-in on Chrome Android - I think Ganggui would > know better than I do. > > Here is how I read the existing code: > 1. AuthInProgress is true when !possibly_invalid_account_id_.empty() - see > https://cs.chromium.org/chromium/src/components/signin/core/browser/signin_ma... > 2. So the question is when possibly_invalid_account_id_ is non-empty > 3. possibly_invalid_account_id_ is set in PrepareForSignin, CopyCredentialsFrom > and in OnExternalSigninCompleted (See > https://cs.chromium.org/chromium/src/components/signin/core/browser/signin_ma...) > 4. On Android, we never call PrepareForSignin or CopyCrendetialsFrom (this is > used when we create a new profile during sign-in). > 5. This basically means that we only call OnExternalSigninCompleted when the > user signs in > https://cs.chromium.org/chromium/src/components/signin/core/browser/signin_ma... > and > https://cs.chromium.org/chromium/src/chrome/browser/android/signin/signin_man... > 6. OnExternalSignin sets possibly_invalid_account_id_ and then synchronously > calls OnSignIn and clears it before calling its observers with > GoogleSigninSucceeded > > So I personally do not see when AuthInProgress would return true on Android for > a caller. > > However, I do not think you need to go and change the users of this method. Hi msarda@, my understanding of AuthInProgress on Android is the same as you (assuming all the functions are called in the same thread). SigninManager (signin_manager.cc) is a very thin layer on Android. We just use it to record signed in account after all the verifications have been done. So the operations in SigninManger (signin_manager) for Android should could be done synchronously.
On 2017/01/25 15:25:14, gogerald1 wrote: > On 2017/01/25 14:51:24, msarda wrote: > > I have not really worked on sign-in on Chrome Android - I think Ganggui would > > know better than I do. > > > > Here is how I read the existing code: > > 1. AuthInProgress is true when !possibly_invalid_account_id_.empty() - see > > > https://cs.chromium.org/chromium/src/components/signin/core/browser/signin_ma... > > 2. So the question is when possibly_invalid_account_id_ is non-empty > > 3. possibly_invalid_account_id_ is set in PrepareForSignin, > CopyCredentialsFrom > > and in OnExternalSigninCompleted (See > > > https://cs.chromium.org/chromium/src/components/signin/core/browser/signin_ma...) > > 4. On Android, we never call PrepareForSignin or CopyCrendetialsFrom (this is > > used when we create a new profile during sign-in). > > 5. This basically means that we only call OnExternalSigninCompleted when the > > user signs in > > > https://cs.chromium.org/chromium/src/components/signin/core/browser/signin_ma... > > and > > > https://cs.chromium.org/chromium/src/chrome/browser/android/signin/signin_man... > > 6. OnExternalSignin sets possibly_invalid_account_id_ and then synchronously > > calls OnSignIn and clears it before calling its observers with > > GoogleSigninSucceeded > > > > So I personally do not see when AuthInProgress would return true on Android > for > > a caller. > > > > However, I do not think you need to go and change the users of this method. > > Hi msarda@, my understanding of AuthInProgress on Android is the same as you > (assuming all the functions are called in the same thread). SigninManager > (signin_manager.cc) is a very thin layer on Android. We just use it to record > signed in account after all the verifications have been done. So the operations > in SigninManger (signin_manager) for Android should could be done synchronously. Thanks both for the detailed investigation and explanations! (The code I linked to is in fact my team's code, and I was worried that when I switch it to the new fetcher, we'd lose handling of some edge case. But thanks to your explanations, I'm now confident that that edge case doesn't actually exist, so it's all good.)
The CQ bit was checked by treib@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
msarda@, PTAL again - I've now added the error code to the callback, so the API should be complete. Thanks!
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)
I have done a pass over the entire CL now. I still have a couple of comments/questions. I think the API looks good. The tests look great. https://codereview.chromium.org/2582573002/diff/280001/components/signin/core... File components/signin/core/browser/access_token_fetcher.cc (right): https://codereview.chromium.org/2582573002/diff/280001/components/signin/core... components/signin/core/browser/access_token_fetcher.cc:69: void AccessTokenFetcher::StartAccessTokenRequest() { Please add a DCHECK(!access_token_request_) to catch cases when we call StartAccessTokenRequest multiple times without waiting for previous calls to end. https://codereview.chromium.org/2582573002/diff/280001/components/signin/core... components/signin/core/browser/access_token_fetcher.cc:96: GoogleServiceAuthError(GoogleServiceAuthError::INVALID_GAIA_CREDENTIALS), I suppose here we should pass the error |error| to the callback (and not INVALID_GAIA_CREDENTIALS). https://codereview.chromium.org/2582573002/diff/280001/components/signin/core... components/signin/core/browser/access_token_fetcher.cc:123: std::move(callback_).Run( I think calling here the callback with INVALID_GAIA_CRENDENTIALS is not correct. If there is a refresh token for the authenticated account id, then OnRefreshTokenAvailable(authenticated_account_id) was already called. This means that a StartAccessTokenRequest was fired, but it may not have returned with an access token. Is at this point OnRefreshTokensLoaded is called, then the callback will be called with INVALID_GAIA_CREDENTIALS which is incorrect. I see 2 options here: 1. As long as the tokens were not loaded, we ignore the calls to OnRefreshTokenAvailable. We only call StartAccessTokenRequest when the tokens are loaded if there is refresh token available for the authenticated account id. or 2. OnRefreshTokensLoaded we call the callback with INVALID_GAIA_CREDENTIALS only if there is no refresh token available for the authenticated account id. https://codereview.chromium.org/2582573002/diff/280001/components/signin/core... components/signin/core/browser/access_token_fetcher.cc:156: // TODO(treib): Should we check error.IsTransientError() instead of explicit I think we're defending against a corner case when requests are cancelled. Thus I would say that we should not use error.IsTransientError() here. Consider removing this TODO. https://codereview.chromium.org/2582573002/diff/280001/components/signin/core... components/signin/core/browser/access_token_fetcher.cc:160: token_service_->RefreshTokenIsAvailable( I was thinking that here we may want to always retry when the existing requests were cancelled, but there is a refresh token is still available. Why should we only retry once? I think that would simplify the code a bit. https://codereview.chromium.org/2582573002/diff/280001/components/signin/core... File components/signin/core/browser/access_token_fetcher.h (right): https://codereview.chromium.org/2582573002/diff/280001/components/signin/core... components/signin/core/browser/access_token_fetcher.h:17: // Helper class to ease the task of obtaining an OAuth2 access token. This ss/access token/access token for the authenticated account id https://codereview.chromium.org/2582573002/diff/280001/components/signin/core... components/signin/core/browser/access_token_fetcher.h:71: Nit: Kill empty line. https://codereview.chromium.org/2582573002/diff/280001/components/signin/core... File components/signin/core/browser/access_token_fetcher_unittest.cc (right): https://codereview.chromium.org/2582573002/diff/280001/components/signin/core... components/signin/core/browser/access_token_fetcher_unittest.cc:199: GoogleServiceAuthError(GoogleServiceAuthError::INVALID_GAIA_CREDENTIALS)); This test will fail, if you pass a different error auth error here.
Thanks! Comments addressed, PTAL again. https://codereview.chromium.org/2582573002/diff/280001/components/signin/core... File components/signin/core/browser/access_token_fetcher.cc (right): https://codereview.chromium.org/2582573002/diff/280001/components/signin/core... components/signin/core/browser/access_token_fetcher.cc:69: void AccessTokenFetcher::StartAccessTokenRequest() { On 2017/01/27 10:38:58, msarda wrote: > Please add a DCHECK(!access_token_request_) to catch cases when we call > StartAccessTokenRequest multiple times without waiting for previous calls to > end. Done. https://codereview.chromium.org/2582573002/diff/280001/components/signin/core... components/signin/core/browser/access_token_fetcher.cc:96: GoogleServiceAuthError(GoogleServiceAuthError::INVALID_GAIA_CREDENTIALS), On 2017/01/27 10:38:58, msarda wrote: > I suppose here we should pass the error |error| to the callback (and not > INVALID_GAIA_CREDENTIALS). D'oh. Done. https://codereview.chromium.org/2582573002/diff/280001/components/signin/core... components/signin/core/browser/access_token_fetcher.cc:123: std::move(callback_).Run( On 2017/01/27 10:38:58, msarda wrote: > I think calling here the callback with INVALID_GAIA_CRENDENTIALS is not correct. > If there is a refresh token for the authenticated account id, then > OnRefreshTokenAvailable(authenticated_account_id) was already called. This means > that a StartAccessTokenRequest was fired, but it may not have returned with an > access token. Is at this point OnRefreshTokensLoaded is called, then the > callback will be called with INVALID_GAIA_CREDENTIALS which is incorrect. But if OnRefreshTokenAvailable(authenticated_account_id) was already called, then we have unregistered ourselves as an observer already, so we should never get the OnRefreshTokensLoaded call. I've added a "DCHECK(!access_token_request_)" to make sure. > I see 2 options here: > 1. As long as the tokens were not loaded, we ignore the calls to > OnRefreshTokenAvailable. We only call StartAccessTokenRequest when the tokens > are loaded if there is refresh token available for the authenticated account id. IIRC, there isn't any way to know if the tokens were already loaded earlier, so we don't know if we should wait for that call or not. > or > 2. OnRefreshTokensLoaded we call the callback with INVALID_GAIA_CREDENTIALS only > if there is no refresh token available for the authenticated account id. Per the explanation above, I think this is what already happens. https://codereview.chromium.org/2582573002/diff/280001/components/signin/core... components/signin/core/browser/access_token_fetcher.cc:156: // TODO(treib): Should we check error.IsTransientError() instead of explicit On 2017/01/27 10:38:58, msarda wrote: > I think we're defending against a corner case when requests are cancelled. Thus > I would say that we should not use error.IsTransientError() here. Consider > removing this TODO. A comment in google_service_auth_error.cc says "These are failures that are likely to succeed if tried again". That sounds like a thing we might want to retry here - I definitely want to avoid a situations where clients of this class have to retry. All that ugliness should be handled in here. OTOH, it feels wrong to add handling code for edge cases that we're not even sure exist. Hence the TODO as a trade-off. WDYT? https://codereview.chromium.org/2582573002/diff/280001/components/signin/core... components/signin/core/browser/access_token_fetcher.cc:160: token_service_->RefreshTokenIsAvailable( On 2017/01/27 10:38:58, msarda wrote: > I was thinking that here we may want to always retry when the existing requests > were cancelled, but there is a refresh token is still available. Why should we > only retry once? > > I think that would simplify the code a bit. An unbounded retry loop is a recipe for DDOSing ourselves. I'm speaking from experience here ;) IMO if we retry multiple times, then we have to add backoff, which would make the code more complicated. I'd say, as long as we don't know of any situation that require multiple retries, we should stay with this. https://codereview.chromium.org/2582573002/diff/280001/components/signin/core... File components/signin/core/browser/access_token_fetcher.h (right): https://codereview.chromium.org/2582573002/diff/280001/components/signin/core... components/signin/core/browser/access_token_fetcher.h:17: // Helper class to ease the task of obtaining an OAuth2 access token. This On 2017/01/27 10:38:58, msarda wrote: > ss/access token/access token for the authenticated account id Done. https://codereview.chromium.org/2582573002/diff/280001/components/signin/core... components/signin/core/browser/access_token_fetcher.h:71: On 2017/01/27 10:38:58, msarda wrote: > Nit: Kill empty line. Done. https://codereview.chromium.org/2582573002/diff/280001/components/signin/core... File components/signin/core/browser/access_token_fetcher_unittest.cc (right): https://codereview.chromium.org/2582573002/diff/280001/components/signin/core... components/signin/core/browser/access_token_fetcher_unittest.cc:199: GoogleServiceAuthError(GoogleServiceAuthError::INVALID_GAIA_CREDENTIALS)); On 2017/01/27 10:38:58, msarda wrote: > This test will fail, if you pass a different error auth error here. Good catch! Per your other comment, I fixed the actual problem in the class. I also used a different error code here.
The CQ bit was checked by treib@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Roger / Anthony - Advice needed. Please take a quick look at the question below: Code looks good. I have one final question and I think we need advice from rogerta@ and anthonyvd@ to find the right solution there. I find that the TokenService is not very consistent with the auth errors in case there is no local refresh token for the authenticated account: * It looks like the OAuth2TokenService will respond with a GoogleServiceAuthError::USER_NOT_SIGNED_UP when there is no refresh token available for that accounts - see https://cs.chromium.org/chromium/src/google_apis/gaia/oauth2_token_service.cc... * In the MutableTokenServiceDelegate, we set the error INVALID_CREDENTIALS when the refresh token is missing - see https://cs.chromium.org/chromium/src/chrome/browser/signin/mutable_profile_oa... I somehow think these 2 cases should lead to the same auth error. Am I wrong? Do we need to change them to make them consistent? https://codereview.chromium.org/2582573002/diff/280001/components/signin/core... File components/signin/core/browser/access_token_fetcher.cc (right): https://codereview.chromium.org/2582573002/diff/280001/components/signin/core... components/signin/core/browser/access_token_fetcher.cc:123: std::move(callback_).Run( On 2017/01/27 11:16:55, Marc Treib wrote: > On 2017/01/27 10:38:58, msarda wrote: > > I think calling here the callback with INVALID_GAIA_CRENDENTIALS is not > correct. > > If there is a refresh token for the authenticated account id, then > > OnRefreshTokenAvailable(authenticated_account_id) was already called. This > means > > that a StartAccessTokenRequest was fired, but it may not have returned with > an > > access token. Is at this point OnRefreshTokensLoaded is called, then the > > callback will be called with INVALID_GAIA_CREDENTIALS which is incorrect. > > But if OnRefreshTokenAvailable(authenticated_account_id) was already called, > then we have unregistered ourselves as an observer already, so we should never > get the OnRefreshTokensLoaded call. I've added a > "DCHECK(!access_token_request_)" to make sure. > > > I see 2 options here: > > 1. As long as the tokens were not loaded, we ignore the calls to > > OnRefreshTokenAvailable. We only call StartAccessTokenRequest when the tokens > > are loaded if there is refresh token available for the authenticated account > id. > > IIRC, there isn't any way to know if the tokens were already loaded earlier, so > we don't know if we should wait for that call or not. > > > or > > 2. OnRefreshTokensLoaded we call the callback with INVALID_GAIA_CREDENTIALS > only > > if there is no refresh token available for the authenticated account id. > > Per the explanation above, I think this is what already happens. I missed that part where the this object removes itself from the list of observers of the token service. Thank you for explaining that. https://codereview.chromium.org/2582573002/diff/280001/components/signin/core... components/signin/core/browser/access_token_fetcher.cc:156: // TODO(treib): Should we check error.IsTransientError() instead of explicit On 2017/01/27 11:16:55, Marc Treib wrote: > On 2017/01/27 10:38:58, msarda wrote: > > I think we're defending against a corner case when requests are cancelled. > Thus > > I would say that we should not use error.IsTransientError() here. Consider > > removing this TODO. > > A comment in google_service_auth_error.cc says "These are failures that are > likely to succeed if tried again". That sounds like a thing we might want to > retry here - I definitely want to avoid a situations where clients of this class > have to retry. All that ugliness should be handled in here. > > OTOH, it feels wrong to add handling code for edge cases that we're not even > sure exist. Hence the TODO as a trade-off. WDYT? Do you plan to do something towards fixing this TODO? If so, what? If not, then I'd say it should be removed (we have already too many TODOs in the code). https://codereview.chromium.org/2582573002/diff/280001/components/signin/core... components/signin/core/browser/access_token_fetcher.cc:160: token_service_->RefreshTokenIsAvailable( On 2017/01/27 11:16:55, Marc Treib wrote: > On 2017/01/27 10:38:58, msarda wrote: > > I was thinking that here we may want to always retry when the existing > requests > > were cancelled, but there is a refresh token is still available. Why should we > > only retry once? > > > > I think that would simplify the code a bit. > > An unbounded retry loop is a recipe for DDOSing ourselves. I'm speaking from > experience here ;) > > IMO if we retry multiple times, then we have to add backoff, which would make > the code more complicated. I'd say, as long as we don't know of any situation > that require multiple retries, we should stay with this. Ok, I am convinced now that your approach is the right one - we should avoid DDOsing GAIA :) https://codereview.chromium.org/2582573002/diff/300001/components/signin/core... File components/signin/core/browser/access_token_fetcher.cc (right): https://codereview.chromium.org/2582573002/diff/300001/components/signin/core... components/signin/core/browser/access_token_fetcher.cc:123: std::move(callback_).Run( I am a bit afraid of generating our own auth error here as I fear it may get lead to a difference in the long run in the error code with the OAuth2TokenService. I find that the TokenService is not very consistent with the auth errors: * It looks like the OAuth2TokenService will respond with a GoogleServiceAuthError::USER_NOT_SIGNED_UP when there is no refresh token available for that accounts - see https://cs.chromium.org/chromium/src/google_apis/gaia/oauth2_token_service.cc... . * In the MutableTokenServiceDelegate, we set the error INVALID_CREDENTIALS when the refresh token is missing - see https://cs.chromium.org/chromium/src/chrome/browser/signin/mutable_profile_oa... I think it would make sense for us to call StartAccessTokenRequest here as well. Note that in that case we also get some logging done in chrome://signin-internals that may be useful in some cases. But I agree that in that case you'll have to remove the DCHECK I asked you to add a a previous review DCHECK(token_service_->RefreshTokenIsAvailable(signin_manager_->GetAuthenticatedAccountId()).
Thanks! PTAL again. I think the question to Roger/Anthony is really unrelated to this CL: Now, the AccessTokenFetcher only passes on whatever error it receives from the service. https://codereview.chromium.org/2582573002/diff/280001/components/signin/core... File components/signin/core/browser/access_token_fetcher.cc (right): https://codereview.chromium.org/2582573002/diff/280001/components/signin/core... components/signin/core/browser/access_token_fetcher.cc:156: // TODO(treib): Should we check error.IsTransientError() instead of explicit On 2017/01/27 12:35:37, msarda wrote: > On 2017/01/27 11:16:55, Marc Treib wrote: > > On 2017/01/27 10:38:58, msarda wrote: > > > I think we're defending against a corner case when requests are cancelled. > > Thus > > > I would say that we should not use error.IsTransientError() here. Consider > > > removing this TODO. > > > > A comment in google_service_auth_error.cc says "These are failures that are > > likely to succeed if tried again". That sounds like a thing we might want to > > retry here - I definitely want to avoid a situations where clients of this > class > > have to retry. All that ugliness should be handled in here. > > > > OTOH, it feels wrong to add handling code for edge cases that we're not even > > sure exist. Hence the TODO as a trade-off. WDYT? > > Do you plan to do something towards fixing this TODO? If so, what? If not, then > I'd say it should be removed (we have already too many TODOs in the code). That's a good point - it's a TODO without concrete plans to do anything about it. While have plenty of precedent for that, let's try not to add more :) But I do want to document that I'm not confident we're doing the right thing here. It seems reasonable to retry on a transient error, and I definitely want to keep any retrying in here, and not leave it to clients who will get it wrong. Would it be better to leave the comment in, just without the TODO tag? https://codereview.chromium.org/2582573002/diff/300001/components/signin/core... File components/signin/core/browser/access_token_fetcher.cc (right): https://codereview.chromium.org/2582573002/diff/300001/components/signin/core... components/signin/core/browser/access_token_fetcher.cc:123: std::move(callback_).Run( On 2017/01/27 12:35:37, msarda wrote: > I am a bit afraid of generating our own auth error here as I fear it may get > lead to a difference in the long run in the error code with the > OAuth2TokenService. > > I find that the TokenService is not very consistent with the auth errors: > * It looks like the OAuth2TokenService will respond with a > GoogleServiceAuthError::USER_NOT_SIGNED_UP when there is no refresh token > available for that accounts - see > https://cs.chromium.org/chromium/src/google_apis/gaia/oauth2_token_service.cc... > . > * In the MutableTokenServiceDelegate, we set the error INVALID_CREDENTIALS when > the refresh token is missing - see > https://cs.chromium.org/chromium/src/chrome/browser/signin/mutable_profile_oa... > > I think it would make sense for us to call StartAccessTokenRequest here as well. > Note that in that case we also get some logging done in > chrome://signin-internals that may be useful in some cases. But I agree that in > that case you'll have to remove the DCHECK I asked you to add a a previous > review > DCHECK(token_service_->RefreshTokenIsAvailable(signin_manager_->GetAuthenticatedAccountId()). > I wasn't happy about generating our own auth error here, and I like your proposal much more. Done! https://codereview.chromium.org/2582573002/diff/320001/components/signin/core... File components/signin/core/browser/access_token_fetcher_unittest.cc (right): https://codereview.chromium.org/2582573002/diff/320001/components/signin/core... components/signin/core/browser/access_token_fetcher_unittest.cc:258: base::MessageLoop message_loop; This is unfortunate :( But I don't see any other solution.
On 2017/01/27 13:16:39, Marc Treib wrote: > Thanks! PTAL again. > > I think the question to Roger/Anthony is really unrelated to this CL: Now, the > AccessTokenFetcher only passes on whatever error it receives from the service. > > https://codereview.chromium.org/2582573002/diff/280001/components/signin/core... > File components/signin/core/browser/access_token_fetcher.cc (right): > > https://codereview.chromium.org/2582573002/diff/280001/components/signin/core... > components/signin/core/browser/access_token_fetcher.cc:156: // TODO(treib): > Should we check error.IsTransientError() instead of explicit > On 2017/01/27 12:35:37, msarda wrote: > > On 2017/01/27 11:16:55, Marc Treib wrote: > > > On 2017/01/27 10:38:58, msarda wrote: > > > > I think we're defending against a corner case when requests are cancelled. > > > Thus > > > > I would say that we should not use error.IsTransientError() here. Consider > > > > removing this TODO. > > > > > > A comment in google_service_auth_error.cc says "These are failures that are > > > likely to succeed if tried again". That sounds like a thing we might want to > > > retry here - I definitely want to avoid a situations where clients of this > > class > > > have to retry. All that ugliness should be handled in here. > > > > > > OTOH, it feels wrong to add handling code for edge cases that we're not even > > > sure exist. Hence the TODO as a trade-off. WDYT? > > > > Do you plan to do something towards fixing this TODO? If so, what? If not, > then > > I'd say it should be removed (we have already too many TODOs in the code). > > That's a good point - it's a TODO without concrete plans to do anything about > it. While have plenty of precedent for that, let's try not to add more :) > > But I do want to document that I'm not confident we're doing the right thing > here. It seems reasonable to retry on a transient error, and I definitely want > to keep any retrying in here, and not leave it to clients who will get it wrong. > Would it be better to leave the comment in, just without the TODO tag? > > https://codereview.chromium.org/2582573002/diff/300001/components/signin/core... > File components/signin/core/browser/access_token_fetcher.cc (right): > > https://codereview.chromium.org/2582573002/diff/300001/components/signin/core... > components/signin/core/browser/access_token_fetcher.cc:123: > std::move(callback_).Run( > On 2017/01/27 12:35:37, msarda wrote: > > I am a bit afraid of generating our own auth error here as I fear it may get > > lead to a difference in the long run in the error code with the > > OAuth2TokenService. > > > > I find that the TokenService is not very consistent with the auth errors: > > * It looks like the OAuth2TokenService will respond with a > > GoogleServiceAuthError::USER_NOT_SIGNED_UP when there is no refresh token > > available for that accounts - see > > > https://cs.chromium.org/chromium/src/google_apis/gaia/oauth2_token_service.cc... > > . > > * In the MutableTokenServiceDelegate, we set the error INVALID_CREDENTIALS > when > > the refresh token is missing - see > > > https://cs.chromium.org/chromium/src/chrome/browser/signin/mutable_profile_oa... > > > > I think it would make sense for us to call StartAccessTokenRequest here as > well. > > Note that in that case we also get some logging done in > > chrome://signin-internals that may be useful in some cases. But I agree that > in > > that case you'll have to remove the DCHECK I asked you to add a a previous > > review > > > DCHECK(token_service_->RefreshTokenIsAvailable(signin_manager_->GetAuthenticatedAccountId()). > > > > I wasn't happy about generating our own auth error here, and I like your > proposal much more. Done! > > https://codereview.chromium.org/2582573002/diff/320001/components/signin/core... > File components/signin/core/browser/access_token_fetcher_unittest.cc (right): > > https://codereview.chromium.org/2582573002/diff/320001/components/signin/core... > components/signin/core/browser/access_token_fetcher_unittest.cc:258: > base::MessageLoop message_loop; > This is unfortunate :( > But I don't see any other solution. Hi Mihai, WRT the different errors, looks like INVALID_CREDENTIALS would work in both cases. I can do a quick pass over the entire CL shortly too.
LGTM. Thank you so much for doing this fetcher. I think it will simplify the way we fetch access tokens.
The CQ bit was checked by treib@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by treib@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from anthonyvd@chromium.org Link to the patchset: https://codereview.chromium.org/2582573002/#ps320001 (title: "review5")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 320001, "attempt_start_ts": 1485533211912540,
"parent_rev": "725d57944395b995089542d44b2405df7038a7eb", "commit_rev":
"4c8c6cfe37c424387e26278371125cccbd16d5f3"}
Message was sent while issue was closed.
Description was changed from ========== Signin/OAuth: Create an AccessTokenFetcher helper class Reliably obtaining an OAuth2 access token from OAuth2TokenService is surprisingly complicated, since there are many special cases to handle (e.g. auth currently in progress, or auth completed but refresh token not loaded yet, ...). Various classes implement some subset of these edge cases. This CL introduces a new helper class that encapsulates all this, and adds a first usage as an example (in SuggestionsService). Partial list of additional classes that can be made simpler and more robust by using the new helper: - NTPSnippetsFetcher - FamilyInfoFetcher - SafeSearchURLReporter - PermissionRequestCreatorApiary BUG=609084 ========== to ========== Signin/OAuth: Create an AccessTokenFetcher helper class Reliably obtaining an OAuth2 access token from OAuth2TokenService is surprisingly complicated, since there are many special cases to handle (e.g. auth currently in progress, or auth completed but refresh token not loaded yet, ...). Various classes implement some subset of these edge cases. This CL introduces a new helper class that encapsulates all this, and adds a first usage as an example (in SuggestionsService). Partial list of additional classes that can be made simpler and more robust by using the new helper: - NTPSnippetsFetcher - FamilyInfoFetcher - SafeSearchURLReporter - PermissionRequestCreatorApiary BUG=609084 Review-Url: https://codereview.chromium.org/2582573002 Cr-Commit-Position: refs/heads/master@{#446678} Committed: https://chromium.googlesource.com/chromium/src/+/4c8c6cfe37c424387e2627837112... ==========
Message was sent while issue was closed.
Committed patchset #15 (id:320001) as https://chromium.googlesource.com/chromium/src/+/4c8c6cfe37c424387e2627837112... |
