[signin] Add DICe flow for account consistency requests.

components/signin/core/browser/signin_header_helper.cc

Index: components/signin/core/browser/signin_header_helper.cc
diff --git a/components/signin/core/browser/signin_header_helper.cc b/components/signin/core/browser/signin_header_helper.cc
index 8fdfcd682ac6cd6215f1aa14f9ea3bc45834b84c..6c8d5c2068d9d4cc19c0cd409360c5f3454e5167 100644
--- a/components/signin/core/browser/signin_header_helper.cc
+++ b/components/signin/core/browser/signin_header_helper.cc
@@ -6,6 +6,7 @@
 
 #include <stddef.h>
 
+#include "base/logging.h"
 #include "base/macros.h"
 #include "base/strings/string_number_conversions.h"
 #include "base/strings/string_split.h"
@@ -134,13 +135,45 @@ bool IsUrlEligibleForXChromeConnectedHeader(const GURL& url) {
          gaia::IsGaiaSignonRealm(origin);
 }
 
+// Checks if the url has the required properties to have a
+// X-Chrome-ID-Consistency-Request header.
+bool IsUrlEligibleForXChromeIDConsistencyRequestHeader(const GURL& url) {

[msarda, 2017/06/07 13:06:30]

IsUrlEligibleForDiceRequestHeadr

[droger, 2017/06/07 13:29:57]

I'm fine doing this but then I think we should rename
IsUrlEligibleForXChromeConnectedHeader() to IsUrlEligibleForMirrorHeader() for
consistency.

[msarda, 2017/06/08 02:53:38]

I think we should name this to IsUrlEligibleForDiceRequestHeader (but keep the
other one as IsUrlEligibleForXChromeConnectedHeader).

+  return gaia::IsGaiaSignonRealm(url.GetOrigin());
+}
+
 // Checks if the url has the required properties to have an account consistency
 // header.
 bool IsUrlEligibleForAccountConsistencyRequestHeader(const GURL& url) {
-  // TODO(droger): Support X-Chrome-ID-Consistency-Request.
-  return IsUrlEligibleForXChromeConnectedHeader(url);
+  switch (switches::GetAccountConsistencyMethod()) {
+    case switches::AccountConsistencyMethod::kDisabled:
+    // Mirror header can be added even if account consistency is disabled.

[msarda, 2017/06/07 13:06:30]

I think this should never be the case - we should never append the mirror header
when account consistency is disabled.

[droger, 2017/06/07 13:29:57]

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.

[msarda, 2017/06/08 02:53:38]

Agreed. Thank you for explaining this.

+    // Fall through.
+    case switches::AccountConsistencyMethod::kMirror:
+      return IsUrlEligibleForXChromeConnectedHeader(url);
+    case switches::AccountConsistencyMethod::kDice:
+      return IsUrlEligibleForXChromeIDConsistencyRequestHeader(url);
+  }
+
+  NOTREACHED();
+  return false;
 }
 
+// Builds the value of the header to be included in DICe requests.

[msarda, 2017/06/08 02:53:38]

s/DICe/Dice or DICE. What would you prefer?

+std::string BuildDiceRequestIfPossible(
+    const GURL& url,
+    const content_settings::CookieSettings* cookie_settings) {
+  // If signin cookies are not allowed, don't add the header.
+  if (!SettingsAllowSigninCookies(cookie_settings))
+    return std::string();
+
+  // Check if url is elligible for the header.
+  if (!IsUrlEligibleForXChromeIDConsistencyRequestHeader(url))
+    return std::string();
+
+  return "client_id=" + GaiaUrls::GetInstance()->oauth2_chrome_client_id();
+}
+
+// Builds the value of the header to be included in Mirror requests.
 std::string BuildMirrorRequestIfPossible(
     bool is_header_request,
     const GURL& url,
@@ -151,9 +184,8 @@ std::string BuildMirrorRequestIfPossible(
     return std::string();
 
   // If signin cookies are not allowed, don't add the header.
-  if (!SettingsAllowSigninCookies(cookie_settings)) {
+  if (!SettingsAllowSigninCookies(cookie_settings))
     return std::string();
-  }
 
   // Check if url is elligible for the header.
   if (!IsUrlEligibleForXChromeConnectedHeader(url))
@@ -177,6 +209,8 @@ std::string BuildMirrorRequestIfPossible(
 
 }  // namespace
 
+extern const char kChromeIDConsistencyRequestHeader[] =

[msarda, 2017/06/08 02:53:38]

nit: move this below kChromeConnectedHeader to keep the same header as in the
header file.

+    "X-Chrome-ID-Consistency-Request";
 extern const char kChromeConnectedHeader[] = "X-Chrome-Connected";
 
 ManageAccountsParams::ManageAccountsParams()
@@ -221,11 +255,23 @@ bool AppendOrRemoveAccountConsistentyRequestHeader(
     int profile_mode_mask) {
   const GURL& url = redirect_url.is_empty() ? request->url() : redirect_url;
 
-  // TODO(droger): Support X-Chrome-ID-Consistency-Request.
-  std::string header_name = kChromeConnectedHeader;
-  std::string header_value = BuildMirrorRequestIfPossible(
-      true /* is_header_request */, url, account_id, cookie_settings,
-      profile_mode_mask);
+  std::string header_name;
+  std::string header_value;
+  switch (switches::GetAccountConsistencyMethod()) {

[msarda, 2017/06/08 02:53:37]

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?

+    case switches::AccountConsistencyMethod::kDice:
+      header_name = kChromeIDConsistencyRequestHeader;
+      header_value = BuildDiceRequestIfPossible(url, cookie_settings);
+      break;
+    case switches::AccountConsistencyMethod::kDisabled:

[msarda, 2017/06/07 13:06:30]

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)

[droger, 2017/06/07 13:29:57]

Same as above, this currently happens (I did not try though, I'm saying that
only from looking at the code).

+    // The mirror header is added even if account consistency is disabled.
+    // Fall through.
+    case switches::AccountConsistencyMethod::kMirror:
+      header_name = kChromeConnectedHeader;
+      header_value = BuildMirrorRequestIfPossible(
+          true /* is_header_request */, url, account_id, cookie_settings,
+          profile_mode_mask);
+      break;
+  }
 
   if (!header_name.empty() && header_value.empty()) {
     // If the request is being redirected, and it has the account consistency