Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2923733003:
[signin] Add DICe flow for account consistency requests. (Closed)
Description[signin] Add DICe flow for account consistency requests.
BUG=730029
Review-Url: https://codereview.chromium.org/2923733003
Cr-Commit-Position: refs/heads/master@{#478584}
Committed: https://chromium.googlesource.com/chromium/src/+/4fa459890bf3fa250d5a57a5670fe677ed3a075d
Patch Set 1 #
Total comments: 1
Patch Set 2 #
Total comments: 20
Patch Set 3 : move more stuff #Patch Set 4 : Split into multiple classes, to support both headers concurrently #Patch Set 5 : Add test for Dice disabled #
Total comments: 23
Patch Set 6 : review comments #Patch Set 7 : fix style #Dependent Patchsets: Messages
Total messages: 52 (41 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...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Patchset #1 (id:1) has been deleted
The CQ bit was checked by droger@chromium.org to run a CQ dry run
Patchset #1 (id:20001) has been deleted
Patchset #1 (id:40001) has been deleted
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Patchset #2 (id:80001) has been deleted
Patchset #2 (id:100001) has been deleted
Patchset #1 (id:60001) has been deleted
droger@chromium.org changed reviewers: + msarda@chromium.org
Description was changed from ========== [signin] Add DICe flow for account consistency requests. ========== to ========== [signin] Add DICe flow for account consistency requests. BUG=730029 ==========
https://codereview.chromium.org/2923733003/diff/120001/components/signin/core... File components/signin/core/browser/signin_header_helper.cc (right): https://codereview.chromium.org/2923733003/diff/120001/components/signin/core... components/signin/core/browser/signin_header_helper.cc:267: // Fall through. Sanity check: we never want to add the mirror header when DICe is enabled (even for drive), is this correct?
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: ios-simulator on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)
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 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 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: Try jobs failed on following builders: ios-simulator on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)
Patchset #2 (id:140001) has been deleted
Patchset #2 (id:20002) has been deleted
https://codereview.chromium.org/2923733003/diff/170001/components/signin/core... File components/signin/core/browser/signin_header_helper.cc (right): https://codereview.chromium.org/2923733003/diff/170001/components/signin/core... components/signin/core/browser/signin_header_helper.cc:140: bool IsUrlEligibleForXChromeIDConsistencyRequestHeader(const GURL& url) { IsUrlEligibleForDiceRequestHeadr https://codereview.chromium.org/2923733003/diff/170001/components/signin/core... components/signin/core/browser/signin_header_helper.cc:149: // Mirror header can be added even if account consistency is disabled. I think this should never be the case - we should never append the mirror header when account consistency is disabled. https://codereview.chromium.org/2923733003/diff/170001/components/signin/core... components/signin/core/browser/signin_header_helper.cc:265: case switches::AccountConsistencyMethod::kDisabled: No, we must not append the mirror header if account consistency is disabled (we may remove it in that case, but that would be strange). There is a possibility that this code is never called when account consistency is enabled. I think this should maybe be NOTREACHED. May I ask you to test as follows: * run chrome without mirror enabled * sign in to chrome * go to google.com * check if we pass the header on this request (we *must* not pass that header in this case) https://codereview.chromium.org/2923733003/diff/170001/components/signin/core... File components/signin/core/browser/signin_header_helper.h (right): https://codereview.chromium.org/2923733003/diff/170001/components/signin/core... components/signin/core/browser/signin_header_helper.h:33: extern const char kChromeConnectedHeader[]; I think these headers are public only for test. I find Identity Consistency and Account Consistency are very confusing. What do you think about renaming them to kMirrorRequestHeader and kDiceRequestHeader? https://codereview.chromium.org/2923733003/diff/170001/components/signin/core... File components/signin/core/browser/signin_header_helper_unittest.cc (right): https://codereview.chromium.org/2923733003/diff/170001/components/signin/core... components/signin/core/browser/signin_header_helper_unittest.cc:146: TEST_F(SigninHeaderHelperTest, TestMirrorRequestGaiaURL) { We must not pass the mirror header when mirror is disabled. https://codereview.chromium.org/2923733003/diff/170001/components/signin/core... components/signin/core/browser/signin_header_helper_unittest.cc:155: // Tests that no DICe request is returned when the target is not a Gaia URL. Let's use Dice (or dice) and not DICe when referring to the project. https://codereview.chromium.org/2923733003/diff/170001/components/signin/core... File components/signin/core/common/profile_management_switches.cc (right): https://codereview.chromium.org/2923733003/diff/170001/components/signin/core... components/signin/core/common/profile_management_switches.cc:45: GetAccountConsistencyMethod() == AccountConsistencyMethod::kDice; Let's not enable yet multi-account extension for Dice.
Thanks. No code changes, only some replies. I will ping again when the comments are addressed. https://codereview.chromium.org/2923733003/diff/170001/components/signin/core... File components/signin/core/browser/signin_header_helper.cc (right): https://codereview.chromium.org/2923733003/diff/170001/components/signin/core... components/signin/core/browser/signin_header_helper.cc:140: bool IsUrlEligibleForXChromeIDConsistencyRequestHeader(const GURL& url) { On 2017/06/07 13:06:30, msarda wrote: > IsUrlEligibleForDiceRequestHeadr I'm fine doing this but then I think we should rename IsUrlEligibleForXChromeConnectedHeader() to IsUrlEligibleForMirrorHeader() for consistency. https://codereview.chromium.org/2923733003/diff/170001/components/signin/core... components/signin/core/browser/signin_header_helper.cc:149: // Mirror header can be added even if account consistency is disabled. On 2017/06/07 13:06:30, msarda wrote: > I think this should never be the case - we should never append the mirror header > when account consistency is disabled. As discussed offline, this currently happens: see line 134 just above. If (IsDriveOrigin(origin) || gaia::IsGaiaSignonRealm(origin)), the header is appended, even if mirror is disabled. https://codereview.chromium.org/2923733003/diff/170001/components/signin/core... components/signin/core/browser/signin_header_helper.cc:265: case switches::AccountConsistencyMethod::kDisabled: On 2017/06/07 13:06:30, msarda wrote: > No, we must not append the mirror header if account consistency is disabled (we > may remove it in that case, but that would be strange). There is a possibility > that this code is never called when account consistency is enabled. > > I think this should maybe be NOTREACHED. > > May I ask you to test as follows: > * run chrome without mirror enabled > * sign in to chrome > * go to http://google.com > * check if we pass the header on this request (we *must* not pass that header in > this case) > Same as above, this currently happens (I did not try though, I'm saying that only from looking at the code). https://codereview.chromium.org/2923733003/diff/170001/components/signin/core... File components/signin/core/browser/signin_header_helper.h (right): https://codereview.chromium.org/2923733003/diff/170001/components/signin/core... components/signin/core/browser/signin_header_helper.h:33: extern const char kChromeConnectedHeader[]; On 2017/06/07 13:06:30, msarda wrote: > I think these headers are public only for test. > I find Identity Consistency and Account Consistency are very confusing. > > What do you think about renaming them to kMirrorRequestHeader and > kDiceRequestHeader? > Yes, that's fine. https://codereview.chromium.org/2923733003/diff/170001/components/signin/core... File components/signin/core/browser/signin_header_helper_unittest.cc (right): https://codereview.chromium.org/2923733003/diff/170001/components/signin/core... components/signin/core/browser/signin_header_helper_unittest.cc:146: TEST_F(SigninHeaderHelperTest, TestMirrorRequestGaiaURL) { On 2017/06/07 13:06:30, msarda wrote: > We must not pass the mirror header when mirror is disabled. As said in other comments, this is the current behavior. Let me know if this is a bug, and I can change it.
https://codereview.chromium.org/2923733003/diff/170001/components/signin/core... File components/signin/core/browser/signin_header_helper.cc (right): https://codereview.chromium.org/2923733003/diff/170001/components/signin/core... components/signin/core/browser/signin_header_helper.cc:140: bool IsUrlEligibleForXChromeIDConsistencyRequestHeader(const GURL& url) { On 2017/06/07 13:29:57, droger wrote: > On 2017/06/07 13:06:30, msarda wrote: > > IsUrlEligibleForDiceRequestHeadr > > I'm fine doing this but then I think we should rename > IsUrlEligibleForXChromeConnectedHeader() to IsUrlEligibleForMirrorHeader() for > consistency. I think we should name this to IsUrlEligibleForDiceRequestHeader (but keep the other one as IsUrlEligibleForXChromeConnectedHeader). https://codereview.chromium.org/2923733003/diff/170001/components/signin/core... components/signin/core/browser/signin_header_helper.cc:149: // Mirror header can be added even if account consistency is disabled. On 2017/06/07 13:29:57, droger wrote: > On 2017/06/07 13:06:30, msarda wrote: > > I think this should never be the case - we should never append the mirror > header > > when account consistency is disabled. > > As discussed offline, this currently happens: see line 134 just above. > If (IsDriveOrigin(origin) || gaia::IsGaiaSignonRealm(origin)), the header is > appended, even if mirror is disabled. Agreed. Thank you for explaining this. https://codereview.chromium.org/2923733003/diff/170001/components/signin/core... components/signin/core/browser/signin_header_helper.cc:161: // Builds the value of the header to be included in DICe requests. s/DICe/Dice or DICE. What would you prefer? https://codereview.chromium.org/2923733003/diff/170001/components/signin/core... components/signin/core/browser/signin_header_helper.cc:212: extern const char kChromeIDConsistencyRequestHeader[] = nit: move this below kChromeConnectedHeader to keep the same header as in the header file. https://codereview.chromium.org/2923733003/diff/170001/components/signin/core... components/signin/core/browser/signin_header_helper.cc:260: switch (switches::GetAccountConsistencyMethod()) { I think after our discussion today, we need to do the following: * we continue to send the X-Chrome-Connected header in the same cases as we do today (independently if Dice is enabled or not) * we send the new Dice header only when Dice is enabled. WDYT? https://codereview.chromium.org/2923733003/diff/170001/components/signin/core... File components/signin/core/browser/signin_header_helper.h (right): https://codereview.chromium.org/2923733003/diff/170001/components/signin/core... components/signin/core/browser/signin_header_helper.h:33: extern const char kChromeConnectedHeader[]; On 2017/06/07 13:29:57, droger wrote: > On 2017/06/07 13:06:30, msarda wrote: > > I think these headers are public only for test. > > I find Identity Consistency and Account Consistency are very confusing. > > > > What do you think about renaming them to kMirrorRequestHeader and > > kDiceRequestHeader? > > > > Yes, that's fine. After today's discussion ,let's keep the current name kChromeConnectedHeader. https://codereview.chromium.org/2923733003/diff/170001/components/signin/core... File components/signin/core/browser/signin_header_helper_unittest.cc (right): https://codereview.chromium.org/2923733003/diff/170001/components/signin/core... components/signin/core/browser/signin_header_helper_unittest.cc:146: TEST_F(SigninHeaderHelperTest, TestMirrorRequestGaiaURL) { On 2017/06/07 13:29:57, droger wrote: > On 2017/06/07 13:06:30, msarda wrote: > > We must not pass the mirror header when mirror is disabled. > > As said in other comments, this is the current behavior. Let me know if this is > a bug, and I can change it. You are correct. See my comments in the previous file.
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...
Patchset #7 (id:270001) has been deleted
Patchset #5 (id:230001) has been deleted
Patchset #4 (id:210001) has been deleted
Patchset #3 (id:190001) has been deleted
Patchset #4 (id:290001) has been deleted
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...
Following the discussions, we need to support sending both headers, and we can't just get away with a simple if(). I took this opportunity to refactor the code to use classes and split Dice and ChromeConnected into their own files. The CL became kindof large though, sorry about that. https://codereview.chromium.org/2923733003/diff/170001/components/signin/core... File components/signin/core/common/profile_management_switches.cc (right): https://codereview.chromium.org/2923733003/diff/170001/components/signin/core... components/signin/core/common/profile_management_switches.cc:45: GetAccountConsistencyMethod() == AccountConsistencyMethod::kDice; On 2017/06/07 13:06:31, msarda wrote: > Let's not enable yet multi-account extension for Dice. Ok. Filed crbug.com/731066 to track this.
This is a great refactoring. I think it looks really great. I have a couple of comments. https://codereview.chromium.org/2923733003/diff/330001/components/signin/core... File components/signin/core/browser/chrome_connected_header_helper.cc (right): https://codereview.chromium.org/2923733003/diff/330001/components/signin/core... components/signin/core/browser/chrome_connected_header_helper.cc:46: // GAIA Id is only necessary for Drive. Don't set it otherwise. Let's be consistent and use "Gaia ID" everywhere. https://codereview.chromium.org/2923733003/diff/330001/components/signin/core... components/signin/core/browser/chrome_connected_header_helper.cc:77: // workaround until the more generic chrome.principals API is available. You may remove the comment about the "chrome.principals API" as this is dead. https://codereview.chromium.org/2923733003/diff/330001/components/signin/core... components/signin/core/browser/chrome_connected_header_helper.cc:79: // Consider the account id sensitive and limit it to secure domains. Consistency: "id" or "ID" everywhere. https://codereview.chromium.org/2923733003/diff/330001/components/signin/core... File components/signin/core/browser/chrome_connected_header_helper.h (right): https://codereview.chromium.org/2923733003/diff/330001/components/signin/core... components/signin/core/browser/chrome_connected_header_helper.h:19: class ChromeConnectedHeaderHelper : public SigninHeaderHelper { I think in general we add the constructor and the destructor even when they are empty. Not sure what the styleguide says about this though. https://codereview.chromium.org/2923733003/diff/330001/components/signin/core... components/signin/core/browser/chrome_connected_header_helper.h:30: // Returns whether the URL is eligible for the GaiaID parameter. s/GaiaID/Gaia ID https://codereview.chromium.org/2923733003/diff/330001/components/signin/core... File components/signin/core/browser/signin_header_helper.cc (right): https://codereview.chromium.org/2923733003/diff/330001/components/signin/core... components/signin/core/browser/signin_header_helper.cc:160: // Dice is not enabled on mobile. Indent https://codereview.chromium.org/2923733003/diff/330001/components/signin/core... File components/signin/core/browser/signin_header_helper.h (right): https://codereview.chromium.org/2923733003/diff/330001/components/signin/core... components/signin/core/browser/signin_header_helper.h:111: I think it would be good to continue to have the function BuildMirrorRequestCookieIfPossible here and avoid changing the public API (basically, I'd like to avoid other parts of the code to import the chrome_connected_signin_header_helper.h). For me the public API should be in //components/signin/core/browser/signin_header_helper.h and the classes chrome_connected_signin_header_helper and dice_header_helper.h should not be used by other part of the code. https://codereview.chromium.org/2923733003/diff/330001/components/signin/core... File components/signin/core/browser/signin_header_helper_unittest.cc (right): https://codereview.chromium.org/2923733003/diff/330001/components/signin/core... components/signin/core/browser/signin_header_helper_unittest.cc:45: EXPECT_EQ(signin::ChromeConnectedHeaderHelper::BuildRequestCookieIfPossible( I'd like to avoid this call from the test and just call signin::BuildMirrorRequestCookieIfPossible. https://codereview.chromium.org/2923733003/diff/330001/components/signin/core... components/signin/core/browser/signin_header_helper_unittest.cc:154: CheckMirrorHeaderRequest(GURL("https://accounts.google.com"), "0123456789", This actually end up calling |signin::AppendOrRemoveAccountConsistentyRequestHeader| twice. I'd like a test where we call |signin::AppendOrRemoveAccountConsistentyRequestHeader| only once and then check that both headers are present. https://codereview.chromium.org/2923733003/diff/330001/components/signin/core... File components/signin/core/common/profile_management_switches.h (right): https://codereview.chromium.org/2923733003/diff/330001/components/signin/core... components/signin/core/common/profile_management_switches.h:40: // DICe is only supported on desktop. Dice https://codereview.chromium.org/2923733003/diff/330001/components/signin/core... components/signin/core/common/profile_management_switches.h:42: // Called in tests to force enable DICe account consistency. Dice https://codereview.chromium.org/2923733003/diff/330001/components/signin/ios/... File components/signin/ios/browser/account_consistency_service.mm (right): https://codereview.chromium.org/2923733003/diff/330001/components/signin/ios/... components/signin/ios/browser/account_consistency_service.mm:335: cookie_value = Let's avoid this change as explained previously.
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...
Patchset #6 (id:350001) has been deleted
Thanks! https://codereview.chromium.org/2923733003/diff/330001/components/signin/core... File components/signin/core/browser/chrome_connected_header_helper.cc (right): https://codereview.chromium.org/2923733003/diff/330001/components/signin/core... components/signin/core/browser/chrome_connected_header_helper.cc:46: // GAIA Id is only necessary for Drive. Don't set it otherwise. On 2017/06/08 23:43:04, msarda wrote: > Let's be consistent and use "Gaia ID" everywhere. Done. https://codereview.chromium.org/2923733003/diff/330001/components/signin/core... components/signin/core/browser/chrome_connected_header_helper.cc:77: // workaround until the more generic chrome.principals API is available. On 2017/06/08 23:43:04, msarda wrote: > You may remove the comment about the "chrome.principals API" as this is dead. Done. https://codereview.chromium.org/2923733003/diff/330001/components/signin/core... components/signin/core/browser/chrome_connected_header_helper.cc:79: // Consider the account id sensitive and limit it to secure domains. On 2017/06/08 23:43:04, msarda wrote: > Consistency: "id" or "ID" everywhere. Done. https://codereview.chromium.org/2923733003/diff/330001/components/signin/core... File components/signin/core/browser/chrome_connected_header_helper.h (right): https://codereview.chromium.org/2923733003/diff/330001/components/signin/core... components/signin/core/browser/chrome_connected_header_helper.h:19: class ChromeConnectedHeaderHelper : public SigninHeaderHelper { On 2017/06/08 23:43:04, msarda wrote: > I think in general we add the constructor and the destructor even when they are > empty. Not sure what the styleguide says about this though. Done. https://codereview.chromium.org/2923733003/diff/330001/components/signin/core... components/signin/core/browser/chrome_connected_header_helper.h:30: // Returns whether the URL is eligible for the GaiaID parameter. On 2017/06/08 23:43:04, msarda wrote: > s/GaiaID/Gaia ID Done. https://codereview.chromium.org/2923733003/diff/330001/components/signin/core... File components/signin/core/browser/signin_header_helper.cc (right): https://codereview.chromium.org/2923733003/diff/330001/components/signin/core... components/signin/core/browser/signin_header_helper.cc:160: // Dice is not enabled on mobile. On 2017/06/08 23:43:05, msarda wrote: > Indent Not done. Indentation is correct. https://codereview.chromium.org/2923733003/diff/330001/components/signin/core... File components/signin/core/browser/signin_header_helper.h (right): https://codereview.chromium.org/2923733003/diff/330001/components/signin/core... components/signin/core/browser/signin_header_helper.h:111: On 2017/06/08 23:43:05, msarda wrote: > I think it would be good to continue to have the function > BuildMirrorRequestCookieIfPossible here and avoid changing the public API > (basically, I'd like to avoid other parts of the code to import the > chrome_connected_signin_header_helper.h). For me the public API should be in > //components/signin/core/browser/signin_header_helper.h and the classes > chrome_connected_signin_header_helper and dice_header_helper.h should not be > used by other part of the code. Done. https://codereview.chromium.org/2923733003/diff/330001/components/signin/core... File components/signin/core/browser/signin_header_helper_unittest.cc (right): https://codereview.chromium.org/2923733003/diff/330001/components/signin/core... components/signin/core/browser/signin_header_helper_unittest.cc:45: EXPECT_EQ(signin::ChromeConnectedHeaderHelper::BuildRequestCookieIfPossible( On 2017/06/08 23:43:05, msarda wrote: > I'd like to avoid this call from the test and just call > signin::BuildMirrorRequestCookieIfPossible. Done. https://codereview.chromium.org/2923733003/diff/330001/components/signin/core... components/signin/core/browser/signin_header_helper_unittest.cc:154: CheckMirrorHeaderRequest(GURL("https://accounts.google.com"), "0123456789", On 2017/06/08 23:43:05, msarda wrote: > This actually end up calling > |signin::AppendOrRemoveAccountConsistentyRequestHeader| twice. I'd like a test > where we call |signin::AppendOrRemoveAccountConsistentyRequestHeader| only once > and then check that both headers are present. Good point, added a test. https://codereview.chromium.org/2923733003/diff/330001/components/signin/core... File components/signin/core/common/profile_management_switches.h (right): https://codereview.chromium.org/2923733003/diff/330001/components/signin/core... components/signin/core/common/profile_management_switches.h:40: // DICe is only supported on desktop. On 2017/06/08 23:43:05, msarda wrote: > Dice Done. https://codereview.chromium.org/2923733003/diff/330001/components/signin/core... components/signin/core/common/profile_management_switches.h:42: // Called in tests to force enable DICe account consistency. On 2017/06/08 23:43:05, msarda wrote: > Dice Done.
lgtm
The CQ bit was checked by droger@chromium.org
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": 390001, "attempt_start_ts": 1497257740081270,
"parent_rev": "a9ef138c0e921e8d0986228a15e4c282e113e1f8", "commit_rev":
"4fa459890bf3fa250d5a57a5670fe677ed3a075d"}
Message was sent while issue was closed.
Description was changed from ========== [signin] Add DICe flow for account consistency requests. BUG=730029 ========== to ========== [signin] Add DICe flow for account consistency requests. BUG=730029 Review-Url: https://codereview.chromium.org/2923733003 Cr-Commit-Position: refs/heads/master@{#478584} Committed: https://chromium.googlesource.com/chromium/src/+/4fa459890bf3fa250d5a57a5670f... ==========
Message was sent while issue was closed.
Committed patchset #7 (id:390001) as https://chromium.googlesource.com/chromium/src/+/4fa459890bf3fa250d5a57a5670f... |
