|
|
Description[signin] Generate OAuth token on Dice Signin responses
Introduces the DiceResponseHandler class to handle the Dice responses, which
is a new profile keyed service.
In this CL, only the SIGNIN response is implemented, and triggers a OAuth2
token request.
BUG=730589
Review-Url: https://codereview.chromium.org/2942193002
Cr-Commit-Position: refs/heads/master@{#481518}
Committed: https://chromium.googlesource.com/chromium/src/+/7c51029f7f3e25be2a0d2a9c54263c43660990be
Patch Set 1 #Patch Set 2 : cleanup #Patch Set 3 : fix fetcher lifetime #Patch Set 4 : Standalone keyed service #Patch Set 5 : Minor cleanup #Patch Set 6 : Pass OAuth2TokenServiceDelegate directly #
Total comments: 9
Patch Set 7 : Review comments #
Total comments: 8
Patch Set 8 : Review comments #
Messages
Total messages: 34 (25 generated)
Description was changed from ========== [signin] Generate OAuth token on Dice Signin responses BUG=730589 ========== to ========== [signin] Generate OAuth token on Dice Signin responses Introduces the DiceResponseHandler class, owned by ChromeSigninClient. This class handles the Dice response. In this CL, only the SIGNIN response is implemented, and triggers a OAuth2 token request. BUG=730589 ==========
Description was changed from ========== [signin] Generate OAuth token on Dice Signin responses Introduces the DiceResponseHandler class, owned by ChromeSigninClient. This class handles the Dice response. In this CL, only the SIGNIN response is implemented, and triggers a OAuth2 token request. BUG=730589 ========== to ========== [signin] Generate OAuth token on Dice Signin responses Introduces the DiceResponseHandler class, owned by ChromeSigninClient, to handle the Dice response. In this CL, only the SIGNIN response is implemented, and triggers a OAuth2 token request. BUG=730589 ==========
Description was changed from ========== [signin] Generate OAuth token on Dice Signin responses Introduces the DiceResponseHandler class, owned by ChromeSigninClient, to handle the Dice response. In this CL, only the SIGNIN response is implemented, and triggers a OAuth2 token request. BUG=730589 ========== to ========== [signin] Generate OAuth token on Dice Signin responses Introduces the DiceResponseHandler class, owned by ChromeSigninClient, to handle the Dice responses. In this CL, only the SIGNIN response is implemented, and triggers a OAuth2 token request. BUG=730589 ==========
The CQ bit was checked by droger@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...
droger@chromium.org changed reviewers: + msarda@chromium.org
Not sure how to test this though, I guess this would have to be a browser test, but we'd need to mock gaia and I don't know how that works.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_tsan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
Oops, looks like bots are failing, CL is not ready yet for review...
The CQ bit was checked by droger@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...
Description was changed from ========== [signin] Generate OAuth token on Dice Signin responses Introduces the DiceResponseHandler class, owned by ChromeSigninClient, to handle the Dice responses. In this CL, only the SIGNIN response is implemented, and triggers a OAuth2 token request. BUG=730589 ========== to ========== [signin] Generate OAuth token on Dice Signin responses Introduces the DiceResponseHandler class to handle the Dice responses, which is a new profile keyed service. In this CL, only the SIGNIN response is implemented, and triggers a OAuth2 token request. BUG=730589 ==========
This should be ready for review now.
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 droger@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.
https://codereview.chromium.org/2942193002/diff/100001/chrome/browser/signin/... File chrome/browser/signin/dice_response_handler.cc (right): https://codereview.chromium.org/2942193002/diff/100001/chrome/browser/signin/... chrome/browser/signin/dice_response_handler.cc:87: case signin::DiceAction::SIGNIN: In Chrome there are multiple notions of account id: * account_id - corresponds to the account id used in Chrome. Depending on the platform (and on the migration state), this may be either the email or the gaia ID * gaia_id - the GAIA ID of the account The service that maps the Gaia ID to the Chrome account ID is here: https://cs.chromium.org/chromium/src/components/signin/core/browser/account_t... On Desktop, the Chrome account id is in general equal to the Gaia ID, however from an API perspective this is not true. To get the Chrome account ID, consider using https://cs.chromium.org/chromium/src/components/signin/core/browser/account_t... https://codereview.chromium.org/2942193002/diff/100001/chrome/browser/signin/... chrome/browser/signin/dice_response_handler.cc:93: NOTIMPLEMENTED() << "Signout through Dice is not implemented."; I think this should not be NOTIMPLEMENTED as the server may send this reply even if we did no implement it. Maybe change this to a LOG(ERROR). https://codereview.chromium.org/2942193002/diff/100001/chrome/browser/signin/... chrome/browser/signin/dice_response_handler.cc:115: // TODO(droger): The token exchange must complete quicly or be cancelled. Add s/quicly/quickly https://codereview.chromium.org/2942193002/diff/100001/chrome/browser/signin/... chrome/browser/signin/dice_response_handler.cc:121: oauth2_token_service_delegate_->UpdateCredentials(account_id_for_signin_, Use the token service (not its delegate) to update credentials. The delegate is in general ment to be hidden. The API for TokenService::UpdateCredentials is here https://cs.chromium.org/chromium/src/components/signin/core/browser/profile_o... https://codereview.chromium.org/2942193002/diff/100001/chrome/browser/signin/... chrome/browser/signin/dice_response_handler.cc:121: oauth2_token_service_delegate_->UpdateCredentials(account_id_for_signin_, Before calling UpdateCredentials, please also seed the account by calling https://cs.chromium.org/chromium/src/components/signin/core/browser/account_t...
The CQ bit was checked by droger@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...
Thanks. https://codereview.chromium.org/2942193002/diff/100001/chrome/browser/signin/... File chrome/browser/signin/dice_response_handler.cc (right): https://codereview.chromium.org/2942193002/diff/100001/chrome/browser/signin/... chrome/browser/signin/dice_response_handler.cc:87: case signin::DiceAction::SIGNIN: On 2017/06/20 08:45:22, msarda wrote: > In Chrome there are multiple notions of account id: > * account_id - corresponds to the account id used in Chrome. Depending on the > platform (and on the migration state), this may be either the email or the gaia > ID > * gaia_id - the GAIA ID of the account > The service that maps the Gaia ID to the Chrome account ID is here: > https://cs.chromium.org/chromium/src/components/signin/core/browser/account_t... > > On Desktop, the Chrome account id is in general equal to the Gaia ID, however > from an API perspective this is not true. > > To get the Chrome account ID, consider using > https://cs.chromium.org/chromium/src/components/signin/core/browser/account_t... As discussed offline, I did not use PickAccountIdForAccount(), but SeedAccountInfo() instead. https://codereview.chromium.org/2942193002/diff/100001/chrome/browser/signin/... chrome/browser/signin/dice_response_handler.cc:93: NOTIMPLEMENTED() << "Signout through Dice is not implemented."; On 2017/06/20 08:45:22, msarda wrote: > I think this should not be NOTIMPLEMENTED as the server may send this reply even > if we did no implement it. Maybe change this to a LOG(ERROR). Done. https://codereview.chromium.org/2942193002/diff/100001/chrome/browser/signin/... chrome/browser/signin/dice_response_handler.cc:115: // TODO(droger): The token exchange must complete quicly or be cancelled. Add On 2017/06/20 08:45:23, msarda wrote: > s/quicly/quickly Done. https://codereview.chromium.org/2942193002/diff/100001/chrome/browser/signin/... chrome/browser/signin/dice_response_handler.cc:121: oauth2_token_service_delegate_->UpdateCredentials(account_id_for_signin_, On 2017/06/20 08:45:22, msarda wrote: > Before calling UpdateCredentials, please also seed the account by calling > https://cs.chromium.org/chromium/src/components/signin/core/browser/account_t... Done.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
lgtm https://codereview.chromium.org/2942193002/diff/120001/chrome/browser/signin/... File chrome/browser/signin/dice_response_handler.cc (right): https://codereview.chromium.org/2942193002/diff/120001/chrome/browser/signin/... chrome/browser/signin/dice_response_handler.cc:112: const std::string& authorization_code) { Let's DCHECK that gaia_id, email and authorization_code are not empty here. https://codereview.chromium.org/2942193002/diff/120001/chrome/browser/signin/... chrome/browser/signin/dice_response_handler.cc:128: const ClientOAuthResult& result) { I would also add a VLOG(1) here. https://codereview.chromium.org/2942193002/diff/120001/chrome/browser/signin/... chrome/browser/signin/dice_response_handler.cc:141: DLOG(ERROR) << "Dice OAuth failed: " << error.error_message(); Let's use VLOG in this case as it may be useful to be able to check this even when not built in debug mode (this will be useful especially for folks that work on Gaia). https://codereview.chromium.org/2942193002/diff/120001/chrome/browser/signin/... File chrome/browser/signin/dice_response_handler.h (right): https://codereview.chromium.org/2942193002/diff/120001/chrome/browser/signin/... chrome/browser/signin/dice_response_handler.h:53: ProfileOAuth2TokenService* profile_oauth2_token_service_; There is only one token_service. Let's rename: s/profile_oauth2_token_service_/token_service_ Also, I think the API for UpdateCredentials is available in the OAUth2TokenService. May I ask you to use Oauth2TokenService in place of ProfileOAuth2TokenService to keep it as generic as possible?
Thanks! https://codereview.chromium.org/2942193002/diff/120001/chrome/browser/signin/... File chrome/browser/signin/dice_response_handler.cc (right): https://codereview.chromium.org/2942193002/diff/120001/chrome/browser/signin/... chrome/browser/signin/dice_response_handler.cc:112: const std::string& authorization_code) { On 2017/06/22 09:11:31, msarda wrote: > Let's DCHECK that gaia_id, email and authorization_code are not empty here. Done. https://codereview.chromium.org/2942193002/diff/120001/chrome/browser/signin/... chrome/browser/signin/dice_response_handler.cc:128: const ClientOAuthResult& result) { On 2017/06/22 09:11:31, msarda wrote: > I would also add a VLOG(1) here. Done. https://codereview.chromium.org/2942193002/diff/120001/chrome/browser/signin/... chrome/browser/signin/dice_response_handler.cc:141: DLOG(ERROR) << "Dice OAuth failed: " << error.error_message(); On 2017/06/22 09:11:31, msarda wrote: > Let's use VLOG in this case as it may be useful to be able to check this even > when not built in debug mode (this will be useful especially for folks that work > on Gaia). Done. https://codereview.chromium.org/2942193002/diff/120001/chrome/browser/signin/... File chrome/browser/signin/dice_response_handler.h (right): https://codereview.chromium.org/2942193002/diff/120001/chrome/browser/signin/... chrome/browser/signin/dice_response_handler.h:53: ProfileOAuth2TokenService* profile_oauth2_token_service_; On 2017/06/22 09:11:31, msarda wrote: > There is only one token_service. Let's rename: > s/profile_oauth2_token_service_/token_service_ > > Also, I think the API for UpdateCredentials is available in the > OAUth2TokenService. May I ask you to use Oauth2TokenService in place of > ProfileOAuth2TokenService to keep it as generic as possible? No, UpdateCredentials is specific to ProfileOAuth2TokenService.
The CQ bit was checked by droger@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from msarda@chromium.org Link to the patchset: https://codereview.chromium.org/2942193002/#ps140001 (title: "Review comments")
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": 140001, "attempt_start_ts": 1498135169343560, "parent_rev": "fbccda44a34bf3ddaf0aa3119afbba21fa9ab5a9", "commit_rev": "7c51029f7f3e25be2a0d2a9c54263c43660990be"}
Message was sent while issue was closed.
Description was changed from ========== [signin] Generate OAuth token on Dice Signin responses Introduces the DiceResponseHandler class to handle the Dice responses, which is a new profile keyed service. In this CL, only the SIGNIN response is implemented, and triggers a OAuth2 token request. BUG=730589 ========== to ========== [signin] Generate OAuth token on Dice Signin responses Introduces the DiceResponseHandler class to handle the Dice responses, which is a new profile keyed service. In this CL, only the SIGNIN response is implemented, and triggers a OAuth2 token request. BUG=730589 Review-Url: https://codereview.chromium.org/2942193002 Cr-Commit-Position: refs/heads/master@{#481518} Committed: https://chromium.googlesource.com/chromium/src/+/7c51029f7f3e25be2a0d2a9c5426... ==========
Message was sent while issue was closed.
Committed patchset #8 (id:140001) as https://chromium.googlesource.com/chromium/src/+/7c51029f7f3e25be2a0d2a9c5426... |