[Signin] Handle multiple concurrent Dice responses

[Signin] Handle multiple concurrent Dice responses

This CL adds support for handling multiple concurrent Dice responses.

BUG=730589
Review-Url: https://codereview.chromium.org/2950073002
Cr-Commit-Position: refs/heads/master@{#481882}
Committed: https://chromium.googlesource.com/chromium/src/+/83cad2ae0c47202e124c910ce8f710fda13cf838

[droger, 2017-06-21 11:44:40]

Description was changed from

==========
multiple tokens

BUG=
==========

to

==========
[Signin] Handle multiple concurrent Dice responses

This CL adds support for handling multiple concurrent Dice responses.

BUG=730589
==========

[droger, 2017-06-21 11:45:38]

droger@chromium.org changed reviewers:
+ msarda@chromium.org

[msarda, 2017-06-22 09:51:32]

https://codereview.chromium.org/2950073002/diff/20001/chrome/browser/signin/d...
File chrome/browser/signin/dice_response_handler.cc (right):

https://codereview.chromium.org/2950073002/diff/20001/chrome/browser/signin/d...
chrome/browser/signin/dice_response_handler.cc:154: if ((it->get()->gaia_id() ==
gaia_id) && (it->get()->email() == email))
This is a tricky case - in this case the Gaia cookie contains an account that
was reauthenticated while a token request was in progress. It is not clear to me
if the old authorization code can still be used. Also, it is not clear to me how
this would work once the cookie and the refresh tokens are bound one to another.

I think it should be safer to remove the old token fetcher and start a new one
with a fresher authorization code.

WDYT?

https://codereview.chromium.org/2950073002/diff/20001/chrome/browser/signin/d...
File chrome/browser/signin/dice_response_handler.h (right):

https://codereview.chromium.org/2950073002/diff/20001/chrome/browser/signin/d...
chrome/browser/signin/dice_response_handler.h:64:
std::unique_ptr<GaiaAuthFetcher> gaia_auth_fetcher_;
Add DISALLOW_COPY_AND_ASSIGN(DiceTokenFetcher);

https://codereview.chromium.org/2950073002/diff/20001/chrome/browser/signin/d...
chrome/browser/signin/dice_response_handler.h:75: // Called after token exchange
on SIGNIN.
I do not understand what SIGNIN means here. Maybe change this to "Called after
exchanging an OAuth 2.0 authorization code for a refresh token."

https://codereview.chromium.org/2950073002/diff/20001/chrome/browser/signin/d...
chrome/browser/signin/dice_response_handler.h:87:
std::vector<std::unique_ptr<DiceTokenFetcher>> token_fetchers_;
Add Add DISALLOW_COPY_AND_ASSIGN( DiceResponseHandler)

https://codereview.chromium.org/2950073002/diff/20001/chrome/browser/signin/d...
File chrome/browser/signin/dice_response_handler_unittest.cc (right):

https://codereview.chromium.org/2950073002/diff/20001/chrome/browser/signin/d...
chrome/browser/signin/dice_response_handler_unittest.cc:114:
TEST_F(DiceResponseHandlerTest, SigninRepeatedWithSameAccount) {
This test will need to be updated if you implement my previous suggestion.

[droger, 2017-06-22 11:54:20]

Thanks.

https://codereview.chromium.org/2950073002/diff/20001/chrome/browser/signin/d...
File chrome/browser/signin/dice_response_handler.cc (right):

https://codereview.chromium.org/2950073002/diff/20001/chrome/browser/signin/d...
chrome/browser/signin/dice_response_handler.cc:154: if ((it->get()->gaia_id() ==
gaia_id) && (it->get()->email() == email))
On 2017/06/22 09:51:31, msarda wrote:
> This is a tricky case - in this case the Gaia cookie contains an account that
> was reauthenticated while a token request was in progress. It is not clear to
me
> if the old authorization code can still be used. Also, it is not clear to me
how
> this would work once the cookie and the refresh tokens are bound one to
another.
> 
> I think it should be safer to remove the old token fetcher and start a new one
> with a fresher authorization code.
> 
> WDYT?

Thanks fro bringing that up.

Summary of offline discussion:
1) we can cancel the request if the authorization code is the same
2) if requests are different and we receive a different token, then we should
probably revoke the old token. Note that this should be done even if the
requests don't happen at the same time, and thus belongs to another layer
(probably the token service: UpdateCredentials should revoke the old
credentials). Filed https://crbug.com/735898

https://codereview.chromium.org/2950073002/diff/20001/chrome/browser/signin/d...
File chrome/browser/signin/dice_response_handler.h (right):

https://codereview.chromium.org/2950073002/diff/20001/chrome/browser/signin/d...
chrome/browser/signin/dice_response_handler.h:75: // Called after token exchange
on SIGNIN.
On 2017/06/22 09:51:32, msarda wrote:
> I do not understand what SIGNIN means here. Maybe change this to "Called after
> exchanging an OAuth 2.0 authorization code for a refresh token."

I meant DiceAction::SIGNIN. Clarified comment.

https://codereview.chromium.org/2950073002/diff/20001/chrome/browser/signin/d...
chrome/browser/signin/dice_response_handler.h:87:
std::vector<std::unique_ptr<DiceTokenFetcher>> token_fetchers_;
On 2017/06/22 09:51:32, msarda wrote:
> Add Add DISALLOW_COPY_AND_ASSIGN( DiceResponseHandler)

Done.

[msarda, 2017-06-22 12:20:47]

lgtm

[droger, 2017-06-23 13:26:57]

The CQ bit was checked by droger@chromium.org

[droger, 2017-06-23 13:26:58]

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/2950073002/#ps60001
(title: "Rebase")

[commit-bot: I haz the power, 2017-06-23 13:27:06]

CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-06-23 14:40:23]

CQ is committing da patch.

Bot data: {"patchset_id": 60001, "attempt_start_ts": 1498224417708720,
"parent_rev": "11caa3d74cd3178ba7c721f2c2053543f75fce1f", "commit_rev":
"83cad2ae0c47202e124c910ce8f710fda13cf838"}

[commit-bot: I haz the power, 2017-06-23 14:40:35]

Description was changed from

==========
[Signin] Handle multiple concurrent Dice responses

This CL adds support for handling multiple concurrent Dice responses.

BUG=730589
==========

to

==========
[Signin] Handle multiple concurrent Dice responses

This CL adds support for handling multiple concurrent Dice responses.

BUG=730589

Review-Url: https://codereview.chromium.org/2950073002
Cr-Commit-Position: refs/heads/master@{#481882}
Committed:
https://chromium.googlesource.com/chromium/src/+/83cad2ae0c47202e124c910ce8f7...
==========

[commit-bot: I haz the power, 2017-06-23 14:40:37]

Committed patchset #4 (id:60001) as
https://chromium.googlesource.com/chromium/src/+/83cad2ae0c47202e124c910ce8f7...