Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1129463004:
Let Ubertoken Fetch be primed with an access token. (Closed)
|
Created:
5 years, 7 months ago by Mike Lerman Modified:
5 years, 7 months ago CC:
chromium-reviews Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionLet Ubertoken Fetch be primed with an access token.
BUG=483596
TEST=Logging in with ChromeOS still results in the user logging into the content area.
Committed: https://crrev.com/bf4915494a7ccd674dba94dd6d10cce161258cec
Cr-Commit-Position: refs/heads/master@{#330752}
Patch Set 1 #Patch Set 2 : unit tests needs the extra parameter too #Patch Set 3 : ChromeOS sends access token to GCMS #
Total comments: 2
Patch Set 4 : TODO-- #
Total comments: 4
Patch Set 5 : rogerta's comments #
Total comments: 13
Patch Set 6 : split access token methods into new methods. #
Total comments: 6
Patch Set 7 : roger's nits and refactor AddAccountToCookie common code #Patch Set 8 : Rebase #
Total comments: 2
Patch Set 9 : ChromeOS doesn't always have an access token #
Messages
Total messages: 34 (9 generated)
mlerman@chromium.org changed reviewers: + dpolukhin@chromium.org, rogerta@chromium.org, xiyuan@chromium.org
Hi reviewers, This CL helps ChromeOS use the existing Access Token for use with the new GaiaCookieManagerService. Roger - can you review components/signin as owner? Xiyuan - can you review as the main ChromeOS owner of this integration? dpolukhin@ - can you review as owner of chrome/browser/chromeos/login? Thank you all! Mike xiyuan@chromium.org: Please review changes in rogerta@chromium.org: Please review changes in dpolukhin@chromium.org: Please review changes in
LGTM Thank you for following up on this. https://codereview.chromium.org/1129463004/diff/40001/chrome/browser/chromeos... File chrome/browser/chromeos/login/signin/oauth2_login_verifier.cc (right): https://codereview.chromium.org/1129463004/diff/40001/chrome/browser/chromeos... chrome/browser/chromeos/login/signin/oauth2_login_verifier.cc:57: // See http://crbug.com/483596 nit: Remove the TODO?
Thanks Xiyuan. https://codereview.chromium.org/1129463004/diff/40001/chrome/browser/chromeos... File chrome/browser/chromeos/login/signin/oauth2_login_verifier.cc (right): https://codereview.chromium.org/1129463004/diff/40001/chrome/browser/chromeos... chrome/browser/chromeos/login/signin/oauth2_login_verifier.cc:57: // See http://crbug.com/483596 On 2015/05/06 18:43:16, xiyuan wrote: > nit: Remove the TODO? Done.
lgtm
mlerman@chromium.org changed reviewers: + atwilson@chromium.org
Hi Drew, Can you help with a code review for components/signin while Roger's OOO? Thanks.
https://codereview.chromium.org/1129463004/diff/60001/components/signin/core/... File components/signin/core/browser/gaia_cookie_manager_service.cc (right): https://codereview.chromium.org/1129463004/diff/60001/components/signin/core/... components/signin/core/browser/gaia_cookie_manager_service.cc:316: const std::string& account_id, const std::string& oauth2_token) { oauth2_token is not very clear. Is this an access token or a refresh token? Let's rename this arg to |access_token|. https://codereview.chromium.org/1129463004/diff/60001/components/signin/core/... components/signin/core/browser/gaia_cookie_manager_service.cc:585: uber_token_fetcher_->OnGetTokenSuccess(nullptr, access_token, base::Time()); Would be better to add access_token arg to StartFethingToken(). As this it exposes the internal implementation of UbertokenFetcher.
https://codereview.chromium.org/1129463004/diff/80001/components/signin/core/... File components/signin/core/browser/gaia_cookie_manager_service.h (right): https://codereview.chromium.org/1129463004/diff/80001/components/signin/core/... components/signin/core/browser/gaia_cookie_manager_service.h:221: virtual void StartFetchingUbertoken(const std::string& access_token); This seems like a clunky API, to have this method do something special if access_token is empty. Consider breaking the functionality out into two separate routines, or else at least documenting that you do something special if access_token.empty() == true. https://codereview.chromium.org/1129463004/diff/80001/google_apis/gaia/uberto... File google_apis/gaia/ubertoken_fetcher.cc (right): https://codereview.chromium.org/1129463004/diff/80001/google_apis/gaia/uberto... google_apis/gaia/ubertoken_fetcher.cc:49: if (access_token.empty()) Adding a DCHECK() then handling this case seems bad. Just add your DCHECK() then fall through with an empty access_token (you'll end up getting an auth error, but that's OK - the access_token is invalid and there's no reason to treat access_token == "" any differently than access_token == "foobar_invalid_token" https://codereview.chromium.org/1129463004/diff/80001/google_apis/gaia/uberto... File google_apis/gaia/ubertoken_fetcher.h (right): https://codereview.chromium.org/1129463004/diff/80001/google_apis/gaia/uberto... google_apis/gaia/ubertoken_fetcher.h:53: virtual void StartFetchingTokenWithAccessToken(const std::string& account_id, This does special stuff if access_token.empty() == true. I think this is a poor API choice, so I'd either change the caller so they call StartFetchingToken() instead, or else document this behavior.
https://codereview.chromium.org/1129463004/diff/80001/google_apis/gaia/uberto... File google_apis/gaia/ubertoken_fetcher.cc (right): https://codereview.chromium.org/1129463004/diff/80001/google_apis/gaia/uberto... google_apis/gaia/ubertoken_fetcher.cc:50: StartFetchingToken(account_id); Should we return here to let access token fetching finish?
https://codereview.chromium.org/1129463004/diff/80001/components/signin/core/... File components/signin/core/browser/gaia_cookie_manager_service.cc (right): https://codereview.chromium.org/1129463004/diff/80001/components/signin/core/... components/signin/core/browser/gaia_cookie_manager_service.cc:328: } Nit: no need for { and } https://codereview.chromium.org/1129463004/diff/80001/components/signin/core/... File components/signin/core/browser/gaia_cookie_manager_service_unittest.cc (right): https://codereview.chromium.org/1129463004/diff/80001/components/signin/core/... components/signin/core/browser/gaia_cookie_manager_service_unittest.cc:673: } Should have a test for AddAccountToCookieWithToken(). https://codereview.chromium.org/1129463004/diff/80001/google_apis/gaia/uberto... File google_apis/gaia/ubertoken_fetcher.cc (right): https://codereview.chromium.org/1129463004/diff/80001/google_apis/gaia/uberto... google_apis/gaia/ubertoken_fetcher.cc:49: if (access_token.empty()) On 2015/05/11 10:06:21, Andrew T Wilson wrote: > Adding a DCHECK() then handling this case seems bad. Just add your DCHECK() then > fall through with an empty access_token (you'll end up getting an auth error, > but that's OK - the access_token is invalid and there's no reason to treat > access_token == "" any differently than access_token == "foobar_invalid_token" > This code is DCHECKing one thing and handling another, so seems OK from that point of view. However, seems that if the access token is empty, this should return after call to StartFetchingToken() as Xiyuan suggests. Alternatively, do as Drew suggests and collapse StartFetchingTokenWithAccessToken() into StartFetchingToken() and have only one method that does the right thing depending on if an access token is given or not.
https://codereview.chromium.org/1129463004/diff/80001/google_apis/gaia/uberto... File google_apis/gaia/ubertoken_fetcher.cc (right): https://codereview.chromium.org/1129463004/diff/80001/google_apis/gaia/uberto... google_apis/gaia/ubertoken_fetcher.cc:49: if (access_token.empty()) On 2015/05/11 16:56:16, Roger Tawa wrote: > On 2015/05/11 10:06:21, Andrew T Wilson wrote: > > Adding a DCHECK() then handling this case seems bad. Just add your DCHECK() > then > > fall through with an empty access_token (you'll end up getting an auth error, > > but that's OK - the access_token is invalid and there's no reason to treat > > access_token == "" any differently than access_token == "foobar_invalid_token" > > > > This code is DCHECKing one thing and handling another, so seems OK from that > point of view. However, seems that if the access token is empty, this should > return after call to StartFetchingToken() as Xiyuan suggests. > > Alternatively, do as Drew suggests and collapse > StartFetchingTokenWithAccessToken() into StartFetchingToken() and have only one > method that does the right thing depending on if an access token is given or > not. Ah, you are right, I missed that one was checking account_id and the other was checking access_token.
Thanks, everyone. Split the methods that take an access token into new methods. https://codereview.chromium.org/1129463004/diff/60001/components/signin/core/... File components/signin/core/browser/gaia_cookie_manager_service.cc (right): https://codereview.chromium.org/1129463004/diff/60001/components/signin/core/... components/signin/core/browser/gaia_cookie_manager_service.cc:316: const std::string& account_id, const std::string& oauth2_token) { On 2015/05/07 18:44:50, Roger Tawa wrote: > oauth2_token is not very clear. Is this an access token or a refresh token? > Let's rename this arg to |access_token|. Done. https://codereview.chromium.org/1129463004/diff/60001/components/signin/core/... components/signin/core/browser/gaia_cookie_manager_service.cc:585: uber_token_fetcher_->OnGetTokenSuccess(nullptr, access_token, base::Time()); On 2015/05/07 18:44:50, Roger Tawa wrote: > Would be better to add access_token arg to StartFethingToken(). As this it > exposes the internal implementation of UbertokenFetcher. Done. https://codereview.chromium.org/1129463004/diff/80001/components/signin/core/... File components/signin/core/browser/gaia_cookie_manager_service.cc (right): https://codereview.chromium.org/1129463004/diff/80001/components/signin/core/... components/signin/core/browser/gaia_cookie_manager_service.cc:328: } On 2015/05/11 16:56:15, Roger Tawa wrote: > Nit: no need for { and } Done. https://codereview.chromium.org/1129463004/diff/80001/components/signin/core/... File components/signin/core/browser/gaia_cookie_manager_service.h (right): https://codereview.chromium.org/1129463004/diff/80001/components/signin/core/... components/signin/core/browser/gaia_cookie_manager_service.h:221: virtual void StartFetchingUbertoken(const std::string& access_token); On 2015/05/11 10:06:21, Andrew T Wilson wrote: > This seems like a clunky API, to have this method do something special if > access_token is empty. Consider breaking the functionality out into two separate > routines, or else at least documenting that you do something special if > access_token.empty() == true. Broken out into two methods. https://codereview.chromium.org/1129463004/diff/80001/google_apis/gaia/uberto... File google_apis/gaia/ubertoken_fetcher.cc (right): https://codereview.chromium.org/1129463004/diff/80001/google_apis/gaia/uberto... google_apis/gaia/ubertoken_fetcher.cc:49: if (access_token.empty()) On 2015/05/12 15:08:08, Andrew T Wilson wrote: > On 2015/05/11 16:56:16, Roger Tawa wrote: > > On 2015/05/11 10:06:21, Andrew T Wilson wrote: > > > Adding a DCHECK() then handling this case seems bad. Just add your DCHECK() > > then > > > fall through with an empty access_token (you'll end up getting an auth > error, > > > but that's OK - the access_token is invalid and there's no reason to treat > > > access_token == "" any differently than access_token == > "foobar_invalid_token" > > > > > > > This code is DCHECKing one thing and handling another, so seems OK from that > > point of view. However, seems that if the access token is empty, this should > > return after call to StartFetchingToken() as Xiyuan suggests. > > > > Alternatively, do as Drew suggests and collapse > > StartFetchingTokenWithAccessToken() into StartFetchingToken() and have only > one > > method that does the right thing depending on if an access token is given or > > not. > > Ah, you are right, I missed that one was checking account_id and the other was > checking access_token. Changed this method so that it should always have both an access token and an account_id. https://codereview.chromium.org/1129463004/diff/80001/google_apis/gaia/uberto... google_apis/gaia/ubertoken_fetcher.cc:50: StartFetchingToken(account_id); On 2015/05/11 16:11:58, xiyuan wrote: > Should we return here to let access token fetching finish? I no longer permit access_token.empty(). The caller should call the other method instead. https://codereview.chromium.org/1129463004/diff/80001/google_apis/gaia/uberto... File google_apis/gaia/ubertoken_fetcher.h (right): https://codereview.chromium.org/1129463004/diff/80001/google_apis/gaia/uberto... google_apis/gaia/ubertoken_fetcher.h:53: virtual void StartFetchingTokenWithAccessToken(const std::string& account_id, On 2015/05/11 10:06:21, Andrew T Wilson wrote: > This does special stuff if access_token.empty() == true. I think this is a poor > API choice, so I'd either change the caller so they call StartFetchingToken() > instead, or else document this behavior. Changed, per your other comment, so that this no longer has special behaviour if access_token.empty(). We just DCHECK(!empty()).
*gentle nudge to reviewers*
Overall, I think it would be simpler to have one method called XXX() with an optional access token string rather than a pair of functions XXX() and XXXWithAccessToken(), but I'm OK as is. However, I don't think the code duplication is OK. Should at least refactor the code to use internal private methods that don't duplicate code. So lgtm with that refactoring. https://codereview.chromium.org/1129463004/diff/100001/components/signin/core... File components/signin/core/browser/gaia_cookie_manager_service.cc (right): https://codereview.chromium.org/1129463004/diff/100001/components/signin/core... components/signin/core/browser/gaia_cookie_manager_service.cc:326: const std::string& account_id, const std::string& access_token) { nit: two lines? https://codereview.chromium.org/1129463004/diff/100001/components/signin/core... components/signin/core/browser/gaia_cookie_manager_service.cc:335: VLOG(1) << "GaiaCookieManagerService::AddAccountToCookie: " << account_id; nit: wrong vlog function name. https://codereview.chromium.org/1129463004/diff/100001/components/signin/core... components/signin/core/browser/gaia_cookie_manager_service.cc:597: << requests_.front().account_id(); nit: wrong vlog function name.
Hi Roger, comments incorporated. I did some splitting of the method calls. Also, since I rebased in the change to include DelayNetworkCall (which means any such calls need to be Closures), I added a member which is the access_token_ optionally provided and rely on its presence to determine what to do. Let me know what you think. https://codereview.chromium.org/1129463004/diff/100001/components/signin/core... File components/signin/core/browser/gaia_cookie_manager_service.cc (right): https://codereview.chromium.org/1129463004/diff/100001/components/signin/core... components/signin/core/browser/gaia_cookie_manager_service.cc:326: const std::string& account_id, const std::string& access_token) { On 2015/05/14 20:51:35, Roger Tawa wrote: > nit: two lines? Done. https://codereview.chromium.org/1129463004/diff/100001/components/signin/core... components/signin/core/browser/gaia_cookie_manager_service.cc:335: VLOG(1) << "GaiaCookieManagerService::AddAccountToCookie: " << account_id; On 2015/05/14 20:51:35, Roger Tawa wrote: > nit: wrong vlog function name. Done. https://codereview.chromium.org/1129463004/diff/100001/components/signin/core... components/signin/core/browser/gaia_cookie_manager_service.cc:597: << requests_.front().account_id(); On 2015/05/14 20:51:35, Roger Tawa wrote: > nit: wrong vlog function name. Done.
The CQ bit was checked by mlerman@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from xiyuan@chromium.org, dpolukhin@chromium.org, rogerta@chromium.org Link to the patchset: https://codereview.chromium.org/1129463004/#ps140001 (title: "Rebase")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1129463004/140001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: linux_chromium_chromeos_rel_ng on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
https://codereview.chromium.org/1129463004/diff/140001/chrome/browser/chromeo... File chrome/browser/chromeos/login/signin/oauth2_login_verifier.cc (right): https://codereview.chromium.org/1129463004/diff/140001/chrome/browser/chromeo... chrome/browser/chromeos/login/signin/oauth2_login_verifier.cc:56: access_token_); Sorry, missed this. |access_token_| could be empty. So we need to selectively call AddAccountToCookie and AddAccountToCookieWithToken. :p
How's that, Xiyuan? https://codereview.chromium.org/1129463004/diff/140001/chrome/browser/chromeo... File chrome/browser/chromeos/login/signin/oauth2_login_verifier.cc (right): https://codereview.chromium.org/1129463004/diff/140001/chrome/browser/chromeo... chrome/browser/chromeos/login/signin/oauth2_login_verifier.cc:56: access_token_); On 2015/05/19 20:15:30, xiyuan wrote: > Sorry, missed this. > > |access_token_| could be empty. So we need to selectively call > AddAccountToCookie and AddAccountToCookieWithToken. :p Ah, I thought there always was one. Thanks! Done.
The CQ bit was checked by mlerman@chromium.org to run a CQ dry run
The patchset sent to the CQ was uploaded after l-g-t-m from dpolukhin@chromium.org, xiyuan@chromium.org, rogerta@chromium.org Link to the patchset: https://codereview.chromium.org/1129463004/#ps160001 (title: "ChromeOS doesn't always have an access token")
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1129463004/160001
On 2015/05/20 15:33:07, Mike Lerman wrote: > How's that, Xiyuan? > This code path is common for both Gaia login and offline login. AT is only available when Gaia is used. The offline case happens when we have valid RT but do not have proper cookies. It happens when user signs out on the google.com. When user signs in again, we don't force user go through Gaia because we have RT but we still re-create the cookies.
LGTM!
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 mlerman@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1129463004/160001
Message was sent while issue was closed.
Committed patchset #9 (id:160001)
Message was sent while issue was closed.
Patchset 9 (id:??) landed as https://crrev.com/bf4915494a7ccd674dba94dd6d10cce161258cec Cr-Commit-Position: refs/heads/master@{#330752} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
