Add show_tos param to gaia endpoint URL instead of command_line switch

to support TOS acceptance feature in previous versions of chrome as well.

Bug: 1045536
Change-Id: I7ba5fd35549acf10d5335960bb77c61a7470d484
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2082111
Commit-Queue: Yusuf Sengul <yusufsn@google.com>
Reviewed-by: Yusuf Sengul <yusufsn@google.com>
Auto-Submit: Rakesh Soma <rakeshsoma@google.com>
Cr-Commit-Position: refs/heads/master@{#746118}

chrome/credential_provider/gaiacp/gaia_credential.cc

@@ -31,15 +31,11 @@ void CGaiaCredential::FinalRelease() {
 
 HRESULT CGaiaCredential::GetUserGlsCommandline(
     base::CommandLine* command_line) {
-  // Don't show tos when GEM isn't enabled.
-  if (IsGemEnabled()) {
-    // In default add user flow, the user has to accept tos
-    // every time. So we need to set the show_tos switch to 1.
-    command_line->AppendSwitchASCII(kShowTosSwitch, "1");
-  }
-
+  // In default add user flow, the user has to accept tos
+  // every time. So we need to set the show_tos switch to 1.
+  bool show_tos = IsGemEnabled();
   HRESULT hr = SetGaiaEndpointCommandLineIfNeeded(
-      L"ep_setup_url", kGaiaSetupPath, IsGemEnabled(), command_line);
+      L"ep_setup_url", kGaiaSetupPath, IsGemEnabled(), show_tos, command_line);
   if (FAILED(hr)) {
     LOGFN(ERROR) << "Setting gaia url for gaia credential failed";
     return E_FAIL;

chrome/credential_provider/gaiacp/gaia_credential_base_unittests.cc

@@ -187,13 +187,9 @@ TEST_P(GcpGaiaCredentialGetSerializationBaseTest, Finish) {
   if (is_gem_features_enabled) {
     ASSERT_EQ(S_OK, hr);
     ASSERT_EQ(1u, accept_tos);
-    // Verify command line switch for show_tos.
-    ASSERT_EQ("1", test->GetShowTosFromCmdLine());
   } else {
     ASSERT_TRUE(FAILED(hr));
     ASSERT_EQ(0u, accept_tos);
-    // Verify command line switch for show_tos.
-    ASSERT_EQ("0", test->GetShowTosFromCmdLine());
   }
 }
 

chrome/credential_provider/gaiacp/gaia_credential_other_user.cc

@@ -27,19 +27,16 @@ void COtherUserGaiaCredential::FinalRelease() {
 
 HRESULT COtherUserGaiaCredential::GetUserGlsCommandline(
     base::CommandLine* command_line) {
-  // Don't show tos when GEM isn't enabled.
-  if (IsGemEnabled()) {
     // In default other user flow, the user has to accept tos
     // every time. So we need to set the show_tos switch to 1.
-    command_line->AppendSwitchASCII(kShowTosSwitch, "1");
-  }
-
-  HRESULT hr = SetGaiaEndpointCommandLineIfNeeded(
-      L"ep_setup_url", kGaiaSetupPath, IsGemEnabled(), command_line);
-  if (FAILED(hr)) {
-    LOGFN(ERROR) << "Setting gaia url for gaia credential failed";
-    return E_FAIL;
-  }
+    bool show_tos = IsGemEnabled();
+    HRESULT hr = SetGaiaEndpointCommandLineIfNeeded(
+        L"ep_setup_url", kGaiaSetupPath, IsGemEnabled(), show_tos,
+        command_line);
+    if (FAILED(hr)) {
+      LOGFN(ERROR) << "Setting gaia url for gaia credential failed";
+      return E_FAIL;
+    }
   return S_OK;
 }
 }  // namespace credential_provider

chrome/credential_provider/gaiacp/gaia_credential_other_user_unittests.cc

@@ -197,17 +197,15 @@ TEST_P(GcpOtherUserCredentialGlsTest, GetUserGlsCommandLine) {
 
   EXPECT_TRUE(command_line.HasSwitch(kGcpwSigninSwitch));
   EXPECT_TRUE(command_line.HasSwitch(switches::kDisableExtensions));
-  ASSERT_EQ(is_gem_features_enabled ? "1" : "",
-            command_line.GetSwitchValueASCII(kShowTosSwitch));
 
   if (is_ep_url_set) {
     ASSERT_EQ("http://login.com/",
               command_line.GetSwitchValueASCII(switches::kGaiaUrl));
     ASSERT_TRUE(gcpw_path.empty());
   } else if (is_gem_features_enabled) {
-    ASSERT_EQ(gcpw_path,
-              base::StringPrintf("embedded/setup/windows?device_id=%s",
-                                 device_id.c_str()));
+    ASSERT_EQ(gcpw_path, base::StringPrintf(
+                             "embedded/setup/windows?device_id=%s&show_tos=1",
+                             device_id.c_str()));
     ASSERT_TRUE(command_line.GetSwitchValueASCII(switches::kGaiaUrl).empty());
   } else {
     ASSERT_TRUE(command_line.GetSwitchValueASCII(switches::kGaiaUrl).empty());

chrome/credential_provider/gaiacp/gaia_credential_unittests.cc

@@ -199,9 +199,9 @@ TEST_P(GcpGaiaCredentialGlsTest, GetUserGlsCommandLine) {
               command_line.GetSwitchValueASCII(switches::kGaiaUrl));
     ASSERT_TRUE(gcpw_path.empty());
   } else if (is_gem_features_enabled) {
-    ASSERT_EQ(gcpw_path,
-              base::StringPrintf("embedded/setup/windows?device_id=%s",
-                                 device_id.c_str()));
+    ASSERT_EQ(gcpw_path, base::StringPrintf(
+                             "embedded/setup/windows?device_id=%s&show_tos=1",
+                             device_id.c_str()));
     ASSERT_TRUE(command_line.GetSwitchValueASCII(switches::kGaiaUrl).empty());
   } else {
     ASSERT_TRUE(command_line.GetSwitchValueASCII(switches::kGaiaUrl).empty());

chrome/credential_provider/gaiacp/gcp_utils.cc

@@ -952,13 +952,14 @@ HRESULT GenerateDeviceId(std::string* device_id) {
 HRESULT SetGaiaEndpointCommandLineIfNeeded(const wchar_t* override_registry_key,
                                            const std::string& default_endpoint,
                                            bool provide_deviceid,
+                                           bool show_tos,
                                            base::CommandLine* command_line) {
   // Registry specified endpoint.
   wchar_t endpoint_url_setting[256];
   ULONG endpoint_url_length = base::size(endpoint_url_setting);
-  if (SUCCEEDED(GetGlobalFlag(override_registry_key, endpoint_url_setting,
-                              &endpoint_url_length)) &&
-      endpoint_url_setting[0]) {
+  HRESULT hr = GetGlobalFlag(override_registry_key, endpoint_url_setting,
+                             &endpoint_url_length);
+  if (SUCCEEDED(hr) && endpoint_url_setting[0]) {
     GURL endpoint_url(endpoint_url_setting);
     if (endpoint_url.is_valid()) {
       command_line->AppendSwitchASCII(switches::kGaiaUrl,
@@ -966,15 +967,22 @@ HRESULT SetGaiaEndpointCommandLineIfNeeded(const wchar_t* override_registry_key,
       command_line->AppendSwitchASCII(kGcpwEndpointPathSwitch,
                                       endpoint_url.path().substr(1));
     }
-  } else if (provide_deviceid) {
+    return S_OK;
+  }
+
+  if (provide_deviceid || show_tos) {
     std::string device_id;
-    HRESULT hr = GenerateDeviceId(&device_id);
+    hr = GenerateDeviceId(&device_id);
     if (SUCCEEDED(hr)) {
       command_line->AppendSwitchASCII(
           kGcpwEndpointPathSwitch,
-          base::StringPrintf("%s?device_id=%s", default_endpoint.c_str(),
-                             device_id.c_str()));
-      return S_OK;
+          base::StringPrintf("%s?device_id=%s&show_tos=%d",
+                             default_endpoint.c_str(), device_id.c_str(),
+                             show_tos ? 1 : 0));
+    } else if (show_tos) {
+      command_line->AppendSwitchASCII(
+          kGcpwEndpointPathSwitch,
+          base::StringPrintf("%s?show_tos=1", default_endpoint.c_str()));
     }
   }
   return S_OK;

chrome/credential_provider/gaiacp/gcp_utils.h

@@ -341,6 +341,7 @@ HRESULT GenerateDeviceId(std::string* device_id);
 HRESULT SetGaiaEndpointCommandLineIfNeeded(const wchar_t* override_registry_key,
                                            const std::string& default_endpoint,
                                            bool provide_deviceid,
+                                           bool show_tos,
                                            base::CommandLine* command_line);
 
 // Returns the file path to installed chrome.exe.

chrome/credential_provider/gaiacp/reauth_credential.cc

@@ -57,8 +57,9 @@ HRESULT CReauthCredential::GetUserGlsCommandline(
   // 1. We need to append this switch irrespective of whether its a
   // reauth flow vs add user flow.
   // 2. We only show tos for GEM usecases.
+  bool show_tos = false;
   if (!CheckIfTosAccepted() && IsGemEnabled())
-    command_line->AppendSwitchASCII(kShowTosSwitch, "1");
+    show_tos = true;
 
   // If this is an existing user with an SID, try to get its gaia id and pass
   // it to the GLS for verification.
@@ -83,11 +84,13 @@ HRESULT CReauthCredential::GetUserGlsCommandline(
                                      OLE2CW(email_for_reauth_));
     // Use kGaiaReauthPath when there is no email_for_reauth_ field set.
     hr = SetGaiaEndpointCommandLineIfNeeded(L"ep_reauth_url", kGaiaReauthPath,
-                                            IsGemEnabled(), command_line);
+                                            IsGemEnabled(), show_tos,
+                                            command_line);
   } else {
     // Use kGaiaSetupPath when there is no email_for_reauth_ field set.
     hr = SetGaiaEndpointCommandLineIfNeeded(L"ep_reauth_url", kGaiaSetupPath,
-                                            IsGemEnabled(), command_line);
+                                            IsGemEnabled(), show_tos,
+                                            command_line);
   }
 
   if (FAILED(hr)) {

chrome/credential_provider/gaiacp/reauth_credential_unittests.cc

@@ -292,9 +292,11 @@ class GcpReauthCredentialGlsRunnerTest : public GlsRunnerTestBase {};
 // 1. Is gem features enabled / disabled.
 // 2. Is ep_url already set via registry.
 // 3. Does reauth email exist.
+// 4. Did user already accept TOS.
 class GcpReauthCredentialGlsTest
     : public GcpReauthCredentialGlsRunnerTest,
-      public ::testing::WithParamInterface<std::tuple<bool, bool, bool>> {};
+      public ::testing::WithParamInterface<std::tuple<bool, bool, bool, bool>> {
+};
 
 TEST_P(GcpReauthCredentialGlsTest, GetUserGlsCommandLine) {
   USES_CONVERSION;
@@ -339,6 +341,10 @@ TEST_P(GcpReauthCredentialGlsTest, GetUserGlsCommandLine) {
                         A2COLE(test_data_storage.GetSuccessEmail().c_str()))));
   }
 
+  const bool is_tos_accepted = std::get<3>(GetParam());
+  if (is_tos_accepted)
+    ASSERT_EQ(S_OK, SetUserProperty(OLE2CW(kSid), kKeyAcceptTos, 1));
+
   // Get user gls command line and extract the kGaiaUrl &
   // kGcpwEndpointPathSwitch switch from it.
   Microsoft::WRL::ComPtr<ITestCredential> test_cred;
@@ -367,13 +373,15 @@ TEST_P(GcpReauthCredentialGlsTest, GetUserGlsCommandLine) {
     ASSERT_TRUE(gcpw_path.empty());
   } else if (is_gem_features_enabled) {
     if (set_email_for_reauth) {
-      ASSERT_EQ(gcpw_path,
-                base::StringPrintf("embedded/reauth/windows?device_id=%s",
-                                   device_id.c_str()));
+      ASSERT_EQ(
+          gcpw_path,
+          base::StringPrintf("embedded/reauth/windows?device_id=%s&show_tos=%d",
+                             device_id.c_str(), is_tos_accepted ? 0 : 1));
     } else {
-      ASSERT_EQ(gcpw_path,
-                base::StringPrintf("embedded/setup/windows?device_id=%s",
-                                   device_id.c_str()));
+      ASSERT_EQ(
+          gcpw_path,
+          base::StringPrintf("embedded/setup/windows?device_id=%s&show_tos=%d",
+                             device_id.c_str(), is_tos_accepted ? 0 : 1));
     }
     ASSERT_TRUE(command_line.GetSwitchValueASCII(switches::kGaiaUrl).empty());
   } else {
@@ -385,6 +393,7 @@ TEST_P(GcpReauthCredentialGlsTest, GetUserGlsCommandLine) {
 INSTANTIATE_TEST_SUITE_P(All,
                          GcpReauthCredentialGlsTest,
                          ::testing::Combine(::testing::Bool(),
+                                            ::testing::Bool(),
                                             ::testing::Bool(),
                                             ::testing::Bool()));
 
@@ -622,18 +631,18 @@ INSTANTIATE_TEST_SUITE_P(All,
                          ::testing::Values(true, false));
 
 // Tests the normal reauth scenario.
-// 1. Is gem features enabled. If enabled, tos should be tested out.
-//    Otherwise, ToS shouldn't be set irrespective of the |kAcceptTos|
-//    registry entry.
+// 1. Is gem features enabled.
+// 2. Is tos already accepted.
 class GcpNormalReauthCredentialGlsRunnerTest
     : public GcpReauthCredentialGlsRunnerTest,
-      public ::testing::WithParamInterface<bool> {};
+      public ::testing::WithParamInterface<std::tuple<bool, bool>> {};
 
 TEST_P(GcpNormalReauthCredentialGlsRunnerTest, WithGemFeatures) {
   USES_CONVERSION;
   CredentialProviderSigninDialogTestDataStorage test_data_storage;
 
-  bool is_gem_features_enabled = GetParam();
+  bool is_gem_features_enabled = std::get<0>(GetParam());
+  bool is_tos_already_accepted = std::get<1>(GetParam());
 
   CComBSTR username = L"foo_bar";
   CComBSTR full_name = A2COLE(test_data_storage.GetSuccessFullName().c_str());
@@ -651,13 +660,14 @@ TEST_P(GcpNormalReauthCredentialGlsRunnerTest, WithGemFeatures) {
   if (is_gem_features_enabled) {
     // Set |kKeyEnableGemFeatures| registry entry to 1.
     ASSERT_EQ(S_OK, SetGlobalFlagForTesting(kKeyEnableGemFeatures, 1u));
-    // Set that ToS was already accepted by the user.
-    ASSERT_EQ(S_OK, SetUserProperty(OLE2CW(sid), kKeyAcceptTos, 1u));
   } else {
     // Set |kKeyEnableGemFeatures| registry entry to 0.
     ASSERT_EQ(S_OK, SetGlobalFlagForTesting(kKeyEnableGemFeatures, 0u));
   }
 
+  if (is_tos_already_accepted)
+    ASSERT_EQ(S_OK, SetUserProperty(OLE2CW(sid), kKeyAcceptTos, 1u));
+
   // Create provider and start logon.
   Microsoft::WRL::ComPtr<ICredentialProviderCredential> cred;
 
@@ -673,12 +683,16 @@ TEST_P(GcpNormalReauthCredentialGlsRunnerTest, WithGemFeatures) {
   ASSERT_EQ(S_OK, StartLogonProcessAndWait());
 
   // Verify command line switch for show_tos.
-  ASSERT_EQ("0", test->GetShowTosFromCmdLine());
+  if (is_gem_features_enabled && !is_tos_already_accepted)
+    ASSERT_EQ("1", test->GetShowTosFromCmdLine());
+  else
+    ASSERT_EQ("0", test->GetShowTosFromCmdLine());
 }
 
 INSTANTIATE_TEST_SUITE_P(All,
                          GcpNormalReauthCredentialGlsRunnerTest,
-                         ::testing::Values(true, false));
+                         ::testing::Combine(::testing::Bool(),
+                                            ::testing::Bool()));
 
 TEST_F(GcpReauthCredentialGlsRunnerTest, NormalReauthWithoutEmail) {
   USES_CONVERSION;

chrome/credential_provider/test/test_credential.h

@@ -338,10 +338,10 @@ HRESULT CTestCredentialBase<T>::ForkGaiaLogonStub(
     const base::CommandLine& command_line,
     CGaiaCredentialBase::UIProcessInfo* uiprocinfo) {
   // Record command_line parameter "show_tos" into global variable.
+  std::string gcpw_path =
+      command_line.GetSwitchValueASCII(kGcpwEndpointPathSwitch);
   show_tos_command_line_ =
-      command_line.HasSwitch(kShowTosSwitch)
-          ? command_line.GetSwitchValueASCII(kShowTosSwitch)
-          : "0";
+      (gcpw_path.find("show_tos=1") != std::string::npos) ? "1" : "0";
 
   if (fail_loading_gaia_logon_stub_)
     return E_FAIL;