Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2918403009:
[Dice] Parse the Dice response header (Closed)
Description[Dice] Parse the Dice response header
BUG=730589
Review-Url: https://codereview.chromium.org/2918403009
Cr-Commit-Position: refs/heads/master@{#478990}
Committed: https://chromium.googlesource.com/chromium/src/+/c0bf70fdccf4a551f4df5241ccd52b3370f3727b
Patch Set 1 #Patch Set 2 : Unit test #Patch Set 3 : mobile #Patch Set 4 : rebase #
Total comments: 31
Patch Set 5 : Fix android compile #Patch Set 6 : Review comments #Patch Set 7 : review comments #Patch Set 8 : Rebase #Patch Set 9 : Remove logout url #Messages
Total messages: 36 (25 generated)
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
https://codereview.chromium.org/2918403009/diff/60001/components/signin/core/... File components/signin/core/browser/signin_header_helper.h (right): https://codereview.chromium.org/2918403009/diff/60001/components/signin/core/... components/signin/core/browser/signin_header_helper.h:100: std::string session_index; I'm not quite sure what it is, but that should probably be an int?
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: android_clang_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_clan...)
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...
I started my code review on a previous patch. https://codereview.chromium.org/2918403009/diff/60001/chrome/browser/loader/c... File chrome/browser/loader/chrome_resource_dispatcher_host_delegate.cc (right): https://codereview.chromium.org/2918403009/diff/60001/chrome/browser/loader/c... chrome/browser/loader/chrome_resource_dispatcher_host_delegate.cc:848: signin::ProcessDiceResponseHeaderIfExists(request, io_data); It looks like the Mirror response header (ManageAccounts) is processed when a response is received - see https://cs.chromium.org/chromium/src/chrome/browser/loader/chrome_resource_di... Why do we need to process the Dice response header on a redirect? Do we also need to process it on a response? https://codereview.chromium.org/2918403009/diff/60001/chrome/browser/signin/c... File chrome/browser/signin/chrome_signin_helper.cc (right): https://codereview.chromium.org/2918403009/diff/60001/chrome/browser/signin/c... chrome/browser/signin/chrome_signin_helper.cc:146: void ProcessMirrorResponseHeaderIfExists(net::URLRequest* request, Could we replace this with a similar strategy as for FixAccountConsistencyRequestHeader - add a public method ProcessAccountConsistencyResponseHeader that either calls ProcessMirrorResponseHeaderIfExists or ProcessDiceResponseHeaderIfExists. Or do these calls needs to be made at different moments? https://codereview.chromium.org/2918403009/diff/60001/chrome/browser/signin/c... chrome/browser/signin/chrome_signin_helper.cc:223: // TODO(droger): Start the Chrome authentication flow. This TODO is wrong as we do not plan to start an authentication flow here. I would change this to TODO(droger): Process the Dice header: on sign-in, exchange the authorization code for an refresh token, on sign-out just follow the sign-out URL. Note that we did not yet decide what to do for sign-out (the thought was to leave all the UI on Gaia - then on Chrome we can simply revoke the token when an account is signed out). https://codereview.chromium.org/2918403009/diff/60001/components/signin/core/... File components/signin/core/browser/signin_header_helper.h (right): https://codereview.chromium.org/2918403009/diff/60001/components/signin/core/... components/signin/core/browser/signin_header_helper.h:53: SINGLE_SESSION_SIGNOUT // Sign out of a single session. I think I would not implement the single sign-out session yet (this does not exist on the web). Let's only implement this once Gaia actually supports it. Or do you want to fully support the API in the document? https://codereview.chromium.org/2918403009/diff/60001/components/signin/core/... components/signin/core/browser/signin_header_helper.h:82: // Struct describing the paramters received in the Dice response header. s/paramters/parameters https://codereview.chromium.org/2918403009/diff/60001/components/signin/core/... components/signin/core/browser/signin_header_helper.h:91: // SIGNOUT, this is the ID of the primary account. "this is the Gaia ID of the account that was signed in" (this may be a secondary account). https://codereview.chromium.org/2918403009/diff/60001/components/signin/core/... components/signin/core/browser/signin_header_helper.h:100: std::string session_index; On 2017/06/09 14:51:07, droger wrote: > I'm not quite sure what it is, but that should probably be an int? This is the index of the account in the Gaia authentication cookies. It should be an int. https://codereview.chromium.org/2918403009/diff/60001/components/signin/core/... components/signin/core/browser/signin_header_helper.h:103: std::string authorization_code; This should be set when action is SIGNIN (otherwise Dice cannot work). https://codereview.chromium.org/2918403009/diff/60001/components/signin/core/... components/signin/core/browser/signin_header_helper.h:107: GURL logout_url; Should be set when action is SIGNOUT. https://codereview.chromium.org/2918403009/diff/60001/components/signin/core/... File components/signin/core/browser/signin_header_helper_unittest.cc (right): https://codereview.chromium.org/2918403009/diff/60001/components/signin/core/... components/signin/core/browser/signin_header_helper_unittest.cc:28: namespace signin { I think that tests can be in an anonymous namespace. What is the reason for having them in the |signin| namespace (is it just for convenience)? https://codereview.chromium.org/2918403009/diff/60001/components/signin/core/... components/signin/core/browser/signin_header_helper_unittest.cc:243: EXPECT_EQ(DiceAction::SIGNIN, params.user_intention); I think we should test the following cases: * valid sign-in: "action=SIGNIN,id=..,authuser=..,authorization_code=.." * invalid sign-in (missing authorization code, which would match a bug on Gaia): "action=..,id=..,authuser=.." * valid logout: "action=SIGNOUT,id=..,authuser=..,session_index=..,logout_url=.."
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...
Thanks. Mostly done, but a couple questions. https://codereview.chromium.org/2918403009/diff/60001/chrome/browser/loader/c... File chrome/browser/loader/chrome_resource_dispatcher_host_delegate.cc (right): https://codereview.chromium.org/2918403009/diff/60001/chrome/browser/loader/c... chrome/browser/loader/chrome_resource_dispatcher_host_delegate.cc:848: signin::ProcessDiceResponseHeaderIfExists(request, io_data); On 2017/06/12 11:52:21, msarda wrote: > It looks like the Mirror response header (ManageAccounts) is processed when a > response is received - see > https://cs.chromium.org/chromium/src/chrome/browser/loader/chrome_resource_di... > > Why do we need to process the Dice response header on a redirect? Do we also > need to process it on a response? The Dice header is only seen on redirects currently. We could call the function on responses as well, if we want to catch it if Gaia changes the flow on their end, but I don't know if this is needed. https://codereview.chromium.org/2918403009/diff/60001/chrome/browser/signin/c... File chrome/browser/signin/chrome_signin_helper.cc (right): https://codereview.chromium.org/2918403009/diff/60001/chrome/browser/signin/c... chrome/browser/signin/chrome_signin_helper.cc:146: void ProcessMirrorResponseHeaderIfExists(net::URLRequest* request, On 2017/06/12 11:52:21, msarda wrote: > Could we replace this with a similar strategy as for > FixAccountConsistencyRequestHeader - add a public method > ProcessAccountConsistencyResponseHeader that either calls > ProcessMirrorResponseHeaderIfExists or ProcessDiceResponseHeaderIfExists. Or do > these calls needs to be made at different moments? They are done at different times currently. It probably would not hurt much to call them at the same time (it would just add some unneeded extra header parsing). https://codereview.chromium.org/2918403009/diff/60001/chrome/browser/signin/c... chrome/browser/signin/chrome_signin_helper.cc:223: // TODO(droger): Start the Chrome authentication flow. On 2017/06/12 11:52:21, msarda wrote: > This TODO is wrong as we do not plan to start an authentication flow here. I > would change this to TODO(droger): Process the Dice header: on sign-in, exchange > the authorization code for an refresh token, on sign-out just follow the > sign-out URL. > > Note that we did not yet decide what to do for sign-out (the thought was to > leave all the UI on Gaia - then on Chrome we can simply revoke the token when an > account is signed out). Done. https://codereview.chromium.org/2918403009/diff/60001/components/signin/core/... File components/signin/core/browser/signin_header_helper.h (right): https://codereview.chromium.org/2918403009/diff/60001/components/signin/core/... components/signin/core/browser/signin_header_helper.h:53: SINGLE_SESSION_SIGNOUT // Sign out of a single session. On 2017/06/12 11:52:21, msarda wrote: > I think I would not implement the single sign-out session yet (this does not > exist on the web). Let's only implement this once Gaia actually supports it. > > Or do you want to fully support the API in the document? Currently, all I'm doing is parsing the header. None of the actions are implemented. I'm ok to wait for Gaia support before implementing the single session signout. Were you thinking that we should not even try to parse it? https://codereview.chromium.org/2918403009/diff/60001/components/signin/core/... components/signin/core/browser/signin_header_helper.h:82: // Struct describing the paramters received in the Dice response header. On 2017/06/12 11:52:21, msarda wrote: > s/paramters/parameters Done. https://codereview.chromium.org/2918403009/diff/60001/components/signin/core/... components/signin/core/browser/signin_header_helper.h:91: // SIGNOUT, this is the ID of the primary account. On 2017/06/12 11:52:21, msarda wrote: > "this is the Gaia ID of the account that was signed in" (this may be a secondary > account). Added the clarifications. FYI: My comments were copied from the server-side doc. https://codereview.chromium.org/2918403009/diff/60001/components/signin/core/... components/signin/core/browser/signin_header_helper.h:100: std::string session_index; On 2017/06/12 11:52:21, msarda wrote: > On 2017/06/09 14:51:07, droger wrote: > > I'm not quite sure what it is, but that should probably be an int? > > This is the index of the account in the Gaia authentication cookies. It should > be an int. Done. https://codereview.chromium.org/2918403009/diff/60001/components/signin/core/... components/signin/core/browser/signin_header_helper.h:103: std::string authorization_code; On 2017/06/12 11:52:21, msarda wrote: > This should be set when action is SIGNIN (otherwise Dice cannot work). Done. https://codereview.chromium.org/2918403009/diff/60001/components/signin/core/... components/signin/core/browser/signin_header_helper.h:107: GURL logout_url; On 2017/06/12 11:52:21, msarda wrote: > Should be set when action is SIGNOUT. Done. https://codereview.chromium.org/2918403009/diff/60001/components/signin/core/... File components/signin/core/browser/signin_header_helper_unittest.cc (right): https://codereview.chromium.org/2918403009/diff/60001/components/signin/core/... components/signin/core/browser/signin_header_helper_unittest.cc:28: namespace signin { On 2017/06/12 11:52:21, msarda wrote: > I think that tests can be in an anonymous namespace. What is the reason for > having them in the |signin| namespace (is it just for convenience)? Yes, this is for convenience (removing the signin:: prefix everywhere). https://codereview.chromium.org/2918403009/diff/60001/components/signin/core/... components/signin/core/browser/signin_header_helper_unittest.cc:243: EXPECT_EQ(DiceAction::SIGNIN, params.user_intention); On 2017/06/12 11:52:21, msarda wrote: > I think we should test the following cases: > * valid sign-in: "action=SIGNIN,id=..,authuser=..,authorization_code=.." > * invalid sign-in (missing authorization code, which would match a bug on Gaia): > "action=..,id=..,authuser=.." > * valid logout: > "action=SIGNOUT,id=..,authuser=..,session_index=..,logout_url=.." SIGNIN and SIGNOUT done. However: what would be the expected behavior in the "invalid signin"? Were you thinking that we should also do some validation in this function and for example return DiceAction::NONE if some fields are missing?
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Code looks good. One last question about the API. https://codereview.chromium.org/2918403009/diff/60001/chrome/browser/loader/c... File chrome/browser/loader/chrome_resource_dispatcher_host_delegate.cc (right): https://codereview.chromium.org/2918403009/diff/60001/chrome/browser/loader/c... chrome/browser/loader/chrome_resource_dispatcher_host_delegate.cc:848: signin::ProcessDiceResponseHeaderIfExists(request, io_data); On 2017/06/12 12:39:14, droger wrote: > On 2017/06/12 11:52:21, msarda wrote: > > It looks like the Mirror response header (ManageAccounts) is processed when a > > response is received - see > > > https://cs.chromium.org/chromium/src/chrome/browser/loader/chrome_resource_di... > > > > Why do we need to process the Dice response header on a redirect? Do we also > > need to process it on a response? > > The Dice header is only seen on redirects currently. > > We could call the function on responses as well, if we want to catch it if Gaia > changes the flow on their end, but I don't know if this is needed. I think we're basically adding too much logic for sign-in header processing in ChromeResourceDispatcherHostDelegate, but I am not an owner of ChromeResourceDispatcherHostDelegate, so I do not know how much they care about this. It may be good to just call the sign-in header helper once from here and the header helper to do whatever it needs to do when a redirect occurs. WDYT? https://codereview.chromium.org/2918403009/diff/60001/chrome/browser/signin/c... File chrome/browser/signin/chrome_signin_helper.cc (right): https://codereview.chromium.org/2918403009/diff/60001/chrome/browser/signin/c... chrome/browser/signin/chrome_signin_helper.cc:146: void ProcessMirrorResponseHeaderIfExists(net::URLRequest* request, On 2017/06/12 12:39:14, droger wrote: > On 2017/06/12 11:52:21, msarda wrote: > > Could we replace this with a similar strategy as for > > FixAccountConsistencyRequestHeader - add a public method > > ProcessAccountConsistencyResponseHeader that either calls > > ProcessMirrorResponseHeaderIfExists or ProcessDiceResponseHeaderIfExists. Or > do > > these calls needs to be made at different moments? > > They are done at different times currently. > It probably would not hurt much to call them at the same time (it would just add > some unneeded extra header parsing). This was one of the reasons why I wanted to get a clear understanding on the various messages SIGNIN vs SIGNOUT vs SIGNOUT_SINGLE_SESSION before implementing them. It may be that we get the header on SIGNIN header on redirect, but SIGNOUT on final response. We may need to ask this of GAIA to understand when we should expect the response to have the Dice response header. https://codereview.chromium.org/2918403009/diff/60001/components/signin/core/... File components/signin/core/browser/signin_header_helper.h (right): https://codereview.chromium.org/2918403009/diff/60001/components/signin/core/... components/signin/core/browser/signin_header_helper.h:53: SINGLE_SESSION_SIGNOUT // Sign out of a single session. On 2017/06/12 12:39:14, droger wrote: > On 2017/06/12 11:52:21, msarda wrote: > > I think I would not implement the single sign-out session yet (this does not > > exist on the web). Let's only implement this once Gaia actually supports it. > > > > Or do you want to fully support the API in the document? > > Currently, all I'm doing is parsing the header. None of the actions are > implemented. > I'm ok to wait for Gaia support before implementing the single session signout. > > Were you thinking that we should not even try to parse it? That's why I thought. But I do not have a strong preference. https://codereview.chromium.org/2918403009/diff/60001/components/signin/core/... File components/signin/core/browser/signin_header_helper_unittest.cc (right): https://codereview.chromium.org/2918403009/diff/60001/components/signin/core/... components/signin/core/browser/signin_header_helper_unittest.cc:28: namespace signin { On 2017/06/12 12:39:16, droger wrote: > On 2017/06/12 11:52:21, msarda wrote: > > I think that tests can be in an anonymous namespace. What is the reason for > > having them in the |signin| namespace (is it just for convenience)? > > Yes, this is for convenience (removing the signin:: prefix everywhere). Acknowledged. https://codereview.chromium.org/2918403009/diff/60001/components/signin/core/... components/signin/core/browser/signin_header_helper_unittest.cc:243: EXPECT_EQ(DiceAction::SIGNIN, params.user_intention); On 2017/06/12 12:39:16, droger wrote: > On 2017/06/12 11:52:21, msarda wrote: > > I think we should test the following cases: > > * valid sign-in: "action=SIGNIN,id=..,authuser=..,authorization_code=.." > > * invalid sign-in (missing authorization code, which would match a bug on > Gaia): > > "action=..,id=..,authuser=.." > > * valid logout: > > "action=SIGNOUT,id=..,authuser=..,session_index=..,logout_url=.." > > SIGNIN and SIGNOUT done. > > However: what would be the expected behavior in the "invalid signin"? Were you > thinking that we should also do some validation in this function and for example > return DiceAction::NONE if some fields are missing? I wanted a test that shows how the parse is done in case where an argument is missing. I think we just need a test (it is up to you what the right behavior would be in this case).
Patchset #7 (id:120001) has been deleted
Patchset #8 (id:150001) has been deleted
https://codereview.chromium.org/2918403009/diff/60001/chrome/browser/loader/c... File chrome/browser/loader/chrome_resource_dispatcher_host_delegate.cc (right): https://codereview.chromium.org/2918403009/diff/60001/chrome/browser/loader/c... chrome/browser/loader/chrome_resource_dispatcher_host_delegate.cc:848: signin::ProcessDiceResponseHeaderIfExists(request, io_data); On 2017/06/12 14:03:42, msarda wrote: > On 2017/06/12 12:39:14, droger wrote: > > On 2017/06/12 11:52:21, msarda wrote: > > > It looks like the Mirror response header (ManageAccounts) is processed when > a > > > response is received - see > > > > > > https://cs.chromium.org/chromium/src/chrome/browser/loader/chrome_resource_di... > > > > > > Why do we need to process the Dice response header on a redirect? Do we also > > > need to process it on a response? > > > > The Dice header is only seen on redirects currently. > > > > We could call the function on responses as well, if we want to catch it if > Gaia > > changes the flow on their end, but I don't know if this is needed. > > I think we're basically adding too much logic for sign-in header processing in > ChromeResourceDispatcherHostDelegate, but I am not an owner of > ChromeResourceDispatcherHostDelegate, so I do not know how much they care about > this. It may be good to just call the sign-in header helper once from here and > the header helper to do whatever it needs to do when a redirect occurs. WDYT? I understand. I merged the Dice and Mirror function. That way, if we need to change the behavior in the future, we should not need to touch this file again. https://codereview.chromium.org/2918403009/diff/60001/chrome/browser/signin/c... File chrome/browser/signin/chrome_signin_helper.cc (right): https://codereview.chromium.org/2918403009/diff/60001/chrome/browser/signin/c... chrome/browser/signin/chrome_signin_helper.cc:146: void ProcessMirrorResponseHeaderIfExists(net::URLRequest* request, On 2017/06/12 14:03:42, msarda wrote: > On 2017/06/12 12:39:14, droger wrote: > > On 2017/06/12 11:52:21, msarda wrote: > > > Could we replace this with a similar strategy as for > > > FixAccountConsistencyRequestHeader - add a public method > > > ProcessAccountConsistencyResponseHeader that either calls > > > ProcessMirrorResponseHeaderIfExists or ProcessDiceResponseHeaderIfExists. Or > > do > > > these calls needs to be made at different moments? > > > > They are done at different times currently. > > It probably would not hurt much to call them at the same time (it would just > add > > some unneeded extra header parsing). > > This was one of the reasons why I wanted to get a clear understanding on the > various messages SIGNIN vs SIGNOUT vs SIGNOUT_SINGLE_SESSION before implementing > them. It may be that we get the header on SIGNIN header on redirect, but SIGNOUT > on final response. > > We may need to ask this of GAIA to understand when we should expect the response > to have the Dice response header. Ok. I'm going to ask them. I think in the meantime we can probably go forward with this CL. https://codereview.chromium.org/2918403009/diff/60001/components/signin/core/... File components/signin/core/browser/signin_header_helper_unittest.cc (right): https://codereview.chromium.org/2918403009/diff/60001/components/signin/core/... components/signin/core/browser/signin_header_helper_unittest.cc:243: EXPECT_EQ(DiceAction::SIGNIN, params.user_intention); On 2017/06/12 14:03:42, msarda wrote: > On 2017/06/12 12:39:16, droger wrote: > > On 2017/06/12 11:52:21, msarda wrote: > > > I think we should test the following cases: > > > * valid sign-in: "action=SIGNIN,id=..,authuser=..,authorization_code=.." > > > * invalid sign-in (missing authorization code, which would match a bug on > > Gaia): > > > "action=..,id=..,authuser=.." > > > * valid logout: > > > "action=SIGNOUT,id=..,authuser=..,session_index=..,logout_url=.." > > > > SIGNIN and SIGNOUT done. > > > > However: what would be the expected behavior in the "invalid signin"? Were you > > thinking that we should also do some validation in this function and for > example > > return DiceAction::NONE if some fields are missing? > > I wanted a test that shows how the parse is done in case where an argument is > missing. I think we just need a test (it is up to you what the right behavior > would be in this case). Done. I'm now doing validation and return NONE if headers are missing. Added test cases for this.
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...
lgtm
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Patchset #9 (id:190001) has been deleted
droger@chromium.org changed reviewers: + rdsmith@chromium.org
+rdsmith for chrome_resource_dispatcher_host_delegate.cc
LGTM stamp based on msarda@'s review.
The CQ bit was checked by droger@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from rdsmith@chromium.org, msarda@chromium.org Link to the patchset: https://codereview.chromium.org/2918403009/#ps210001 (title: "Remove logout url")
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Thanks for the reviews. msarda: I did not add the logout_url parameter, because it was just removed from the spec
CQ is committing da patch.
Bot data: {"patchset_id": 210001, "attempt_start_ts": 1497354966833750,
"parent_rev": "42b293e0d826de47c6ecd71774a41f1be37143eb", "commit_rev":
"c0bf70fdccf4a551f4df5241ccd52b3370f3727b"}
Message was sent while issue was closed.
Description was changed from ========== [Dice] Parse the Dice response header BUG=730589 ========== to ========== [Dice] Parse the Dice response header BUG=730589 Review-Url: https://codereview.chromium.org/2918403009 Cr-Commit-Position: refs/heads/master@{#478990} Committed: https://chromium.googlesource.com/chromium/src/+/c0bf70fdccf4a551f4df5241ccd5... ==========
Message was sent while issue was closed.
Committed patchset #9 (id:210001) as https://chromium.googlesource.com/chromium/src/+/c0bf70fdccf4a551f4df5241ccd5... |
