Changes on GCPW to trigger ToS vs non-ToS google login page based on

previous state of user tos acceptance.

Bug: 1045536
Change-Id: Ib87942148d1b7cd82309edb03c8eb8c6e4680f78
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2021332
Commit-Queue: Rakesh Soma <rakeshsoma@google.com>
Reviewed-by: Tien Mai <tienmai@chromium.org>
Auto-Submit: Rakesh Soma <rakeshsoma@google.com>
Cr-Commit-Position: refs/heads/master@{#735778}

chrome/credential_provider/common/gcp_strings.cc

@@ -75,14 +75,14 @@ const char kGaiaIdSwitch[] = "gaia-id";
 // for GCPW.
 const char kGcpwEndpointPathSwitch[] = "gcpw-endpoint-path";
 
-// The show_tos parameter is used to specify whether tos screen needs to be
-// shown as part of the login process or not.
-const char kShowTosSwitch[] = "show_tos";
-
 // Allows specifying additional oauth scopes for the access token being passed
 // to GCPW.
 const char kGcpwAdditionalOauthScopes[] = "gcpw-additional-oauth-scopes";
 
+// The show_tos parameter is used to specify whether tos screen needs to be
+// shown as part of the login process or not.
+const char kShowTosSwitch[] = "show_tos";
+
 // Parameter appended to sign in URL to pass valid signin domains to the inline
 // login handler. These domains are separated by ','.
 const char kEmailDomainsSigninPromoParameter[] = "emailDomains";

chrome/credential_provider/gaiacp/gaia_credential.cc

@@ -4,7 +4,10 @@
 
 #include "chrome/credential_provider/gaiacp/gaia_credential.h"
 
+#include "base/command_line.h"
+#include "chrome/credential_provider/common/gcp_strings.h"
 #include "chrome/credential_provider/gaiacp/logging.h"
+#include "chrome/credential_provider/gaiacp/mdm_utils.h"
 
 namespace credential_provider {
 
@@ -21,4 +24,16 @@ void CGaiaCredential::FinalRelease() {
   LOGFN(INFO);
 }
 
+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");
+  }
+
+  return S_OK;
+}
+
 }  // namespace credential_provider

chrome/credential_provider/gaiacp/gaia_credential.h

@@ -34,6 +34,11 @@ class ATL_NO_VTABLE CGaiaCredential
   END_COM_MAP()
 
   DECLARE_PROTECT_FINAL_CONSTRUCT()
+
+  // CGaiaCredentialBase
+
+  // Adds additional command line switches like showing ToS.
+  HRESULT GetUserGlsCommandline(base::CommandLine* command_line) override;
 };
 
 }  // namespace credential_provider

chrome/credential_provider/gaiacp/gaia_credential_base.cc

@@ -1132,7 +1132,7 @@ HRESULT CGaiaCredentialBase::GetGlsCommandline(
 
   hr = GetUserGlsCommandline(command_line);
   if (FAILED(hr)) {
-    LOGFN(ERROR) << "GetBaseGlsCommandline hr=" << putHR(hr);
+    LOGFN(ERROR) << "GetUserGlsCommandline hr=" << putHR(hr);
     return hr;
   }
 
@@ -2091,6 +2091,14 @@ HRESULT RegisterAssociation(const base::string16& sid,
     return hr;
   }
 
+  if (IsGemEnabled()) {
+    hr = SetUserProperty(sid, kKeyAcceptTos, 1u);
+    if (FAILED(hr)) {
+      LOGFN(ERROR) << "SetUserProperty(acceptTos) hr=" << putHR(hr);
+      return hr;
+    }
+  }
+
   return S_OK;
 }
 

chrome/credential_provider/gaiacp/gaia_credential_base_unittests.cc

@@ -134,7 +134,25 @@ TEST_F(GcpGaiaCredentialBaseTest, GetSerialization_Start) {
   ASSERT_EQ(S_OK, StartLogonProcessAndWait());
 }
 
-TEST_F(GcpGaiaCredentialBaseTest, GetSerialization_Finish) {
+// Tests the GetSerialization Finish 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.
+class GcpGaiaCredentialGetSerializationBaseTest
+    : public GcpGaiaCredentialBaseTest,
+      public ::testing::WithParamInterface<bool> {};
+
+TEST_P(GcpGaiaCredentialGetSerializationBaseTest, Finish) {
+  bool is_gem_features_enabled = GetParam();
+
+  if (is_gem_features_enabled) {
+    // Set |kKeyEnableGemFeatures| registry entry to 1.
+    ASSERT_EQ(S_OK, SetGlobalFlagForTesting(kKeyEnableGemFeatures, 1u));
+  } else {
+    // Set |kKeyEnableGemFeatures| registry entry to 0.
+    ASSERT_EQ(S_OK, SetGlobalFlagForTesting(kKeyEnableGemFeatures, 0u));
+  }
+
   // Create provider and start logon.
   Microsoft::WRL::ComPtr<ICredentialProviderCredential> cred;
 
@@ -152,12 +170,36 @@ TEST_F(GcpGaiaCredentialBaseTest, GetSerialization_Finish) {
   EXPECT_EQ(S_OK, fake_os_user_manager()->GetUserSID(
                       OSUserManager::GetLocalDomain().c_str(), kDefaultUsername,
                       &sid));
-  ::LocalFree(sid);
 
   // New user should be created.
   EXPECT_EQ(2ul, fake_os_user_manager()->GetUserCount());
+
+  // Finishing logon process should trigger credential changed and trigger
+  // GetSerialization.
+  ASSERT_EQ(S_OK, FinishLogonProcess(true, true, 0));
+
+  // Make sure ToS acceptance when is_gem_features_enabled isn't enabled.
+  DWORD accept_tos = 0u;
+  wchar_t* user_sid_string = nullptr;
+  ASSERT_TRUE(ConvertSidToStringSid(sid, &user_sid_string));
+  HRESULT hr = GetUserProperty(user_sid_string, kKeyAcceptTos, &accept_tos);
+  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());
+  }
 }
 
+INSTANTIATE_TEST_SUITE_P(All,
+                         GcpGaiaCredentialGetSerializationBaseTest,
+                         ::testing::Values(true, false));
+
 // This test emulates the scenario where SetDeselected is triggered by the
 // Windows Login UI process after GetSerialization prior to invocation of
 // ReportResult. Note: This currently happens only for OtherUser credential
@@ -1360,6 +1402,8 @@ void GcpGaiaCredentialBaseAdScenariosTest::SetUp() {
   // Override registry to enable cloud association with google.
   constexpr wchar_t kRegCloudAssociation[] = L"enable_cloud_association";
   ASSERT_EQ(S_OK, SetGlobalFlagForTesting(kRegCloudAssociation, 1));
+  // Set |kKeyEnableGemFeatures| registry entry
+  ASSERT_EQ(S_OK, SetGlobalFlagForTesting(kKeyEnableGemFeatures, 1u));
 
   ASSERT_EQ(S_OK, InitializeProviderAndGetCredential(0, &cred_));
 }
@@ -1481,6 +1525,12 @@ TEST_F(GcpGaiaCredentialBaseAdScenariosTest,
   ASSERT_EQ(S_OK, gaia_id_hr);
   ASSERT_TRUE(gaia_id[0]);
 
+  // Make sure ToS acceptance was recorded.
+  DWORD accept_tos;
+  HRESULT hr = GetUserProperty(sid_str.c_str(), kKeyAcceptTos, &accept_tos);
+  ASSERT_EQ(S_OK, hr);
+  ASSERT_EQ(1u, accept_tos);
+
   // Verify that the authentication results dictionary is now empty.
   ASSERT_TRUE(test->IsAuthenticationResultsEmpty());
 }

chrome/credential_provider/gaiacp/gcpw_strings.cc

@@ -9,4 +9,7 @@ namespace credential_provider {
 const char kKeyLastSuccessfulOnlineLoginMillis[] =
     "last_successful_online_login_millis";
 const char kKeyValidityPeriodInDays[] = "validity_period_in_days";
+
+const wchar_t kKeyAcceptTos[] = L"accept_tos";
+const wchar_t kKeyEnableGemFeatures[] = L"enable_gem_features";
 }  // namespace credential_provider

chrome/credential_provider/gaiacp/gcpw_strings.h

@@ -12,6 +12,12 @@ namespace credential_provider {
 // Time parameters to control validity of the offline session.
 extern const char kKeyLastSuccessfulOnlineLoginMillis[];
 extern const char kKeyValidityPeriodInDays[];
+
+// Registry parameters for gcpw.
+extern const wchar_t kKeyAcceptTos[];
+// Registry parameter controlling whether features related to GEM
+// should be enabled / disabled.
+extern const wchar_t kKeyEnableGemFeatures[];
 }  // namespace credential_provider
 
 #endif  // CHROME_CREDENTIAL_PROVIDER_GAIACP_GCPW_STRINGS_H_

chrome/credential_provider/gaiacp/mdm_utils.cc

@@ -25,6 +25,7 @@
 #include "build/branding_buildflags.h"
 #include "chrome/credential_provider/common/gcp_strings.h"
 #include "chrome/credential_provider/gaiacp/gcp_utils.h"
+#include "chrome/credential_provider/gaiacp/gcpw_strings.h"
 #include "chrome/credential_provider/gaiacp/logging.h"
 #include "chrome/credential_provider/gaiacp/reg_utils.h"
 
@@ -386,6 +387,11 @@ bool PasswordRecoveryEnabled() {
   return true;
 }
 
+bool IsGemEnabled() {
+  // The gem features are enabled by default.
+  return GetGlobalFlagOrDefault(kKeyEnableGemFeatures, 1);
+}
+
 HRESULT EnrollToGoogleMdmIfNeeded(const base::Value& properties) {
   LOGFN(INFO);
 

chrome/credential_provider/gaiacp/mdm_utils.h

@@ -86,6 +86,9 @@ bool MdmEnrollmentEnabled();
 // machine.
 bool PasswordRecoveryEnabled();
 
+// Returns true if the |kKeyEnableGemFeatures| is set to 1.
+bool IsGemEnabled();
+
 // Gets the escrow service URL as defined in the registry or a default value if
 // nothing is set.
 GURL EscrowServiceUrl();

chrome/credential_provider/gaiacp/reauth_credential.cc

@@ -7,7 +7,9 @@
 #include "base/command_line.h"
 #include "chrome/credential_provider/common/gcp_strings.h"
 #include "chrome/credential_provider/gaiacp/gaia_resources.h"
+#include "chrome/credential_provider/gaiacp/gcpw_strings.h"
 #include "chrome/credential_provider/gaiacp/logging.h"
+#include "chrome/credential_provider/gaiacp/mdm_utils.h"
 #include "chrome/credential_provider/gaiacp/os_user_manager.h"
 #include "chrome/credential_provider/gaiacp/reg_utils.h"
 
@@ -27,6 +29,15 @@ void CReauthCredential::FinalRelease() {
 }
 
 // CGaiaCredentialBase /////////////////////////////////////////////////////////
+bool CReauthCredential::CheckIfTosAccepted() {
+  DCHECK(os_user_sid_.Length());
+
+  DWORD acceptTos = 0;
+  HRESULT hr = GetUserProperty(OLE2W(os_user_sid_), kKeyAcceptTos, &acceptTos);
+  if (FAILED(hr))
+    LOGFN(ERROR) << "Failed getting accept_tos. hr = " << putHR(hr);
+  return acceptTos == 1;
+}
 
 HRESULT CReauthCredential::GetUserGlsCommandline(
     base::CommandLine* command_line) {
@@ -37,6 +48,15 @@ HRESULT CReauthCredential::GetUserGlsCommandline(
   // is E_UNEXPECTED.
   bool get_cmd_line_status = false;
 
+  // Check if tos is accepted. If not, we need to load gaia login page
+  // with ToS acceptance screen.
+  // Note:
+  // 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.
+  if (!CheckIfTosAccepted() && IsGemEnabled())
+    command_line->AppendSwitchASCII(kShowTosSwitch, "1");
+
   // If this is an existing user with an SID, try to get its gaia id and pass
   // it to the GLS for verification.
   base::string16 gaia_id;

chrome/credential_provider/gaiacp/reauth_credential.h

@@ -66,6 +66,10 @@ class ATL_NO_VTABLE CReauthCredential
                                BSTR* error_text) override;
   HRESULT GetStringValueImpl(DWORD field_id, wchar_t** value) override;
 
+  // Check if tos has been accepted by this user at least once prior to this
+  // login attempt.
+  bool CheckIfTosAccepted();
+
   // Information about the OS user.
   CComBSTR os_user_domain_;
   CComBSTR os_username_;

chrome/credential_provider/gaiacp/reauth_credential_unittests.cc

@@ -160,7 +160,6 @@ INSTANTIATE_TEST_SUITE_P(All,
                                             ::testing::Bool(),
                                             ::testing::Bool()));
 
-// Tests the GetStringValue method specific to FID_DESCRIPTION label for reasons
 // Tests the GetStringValue method specific to FID_DESCRIPTION label for reasons
 // to enforce GLS. Parameters are:
 // 1. Is enrolled with mdm.
@@ -427,10 +426,20 @@ TEST_F(GcpReauthCredentialGlsRunnerTest, UserGaiaIdMismatch) {
   ASSERT_EQ(S_OK, FinishLogonProcess(false, false, IDS_ACCOUNT_IN_USE_BASE));
 }
 
-TEST_F(GcpReauthCredentialGlsRunnerTest, NormalReauth) {
+// 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.
+class GcpNormalReauthCredentialGlsRunnerTest
+    : public GcpReauthCredentialGlsRunnerTest,
+      public ::testing::WithParamInterface<bool> {};
+
+TEST_P(GcpNormalReauthCredentialGlsRunnerTest, WithGemFeatures) {
   USES_CONVERSION;
   CredentialProviderSigninDialogTestDataStorage test_data_storage;
 
+  bool is_gem_features_enabled = GetParam();
+
   CComBSTR username = L"foo_bar";
   CComBSTR full_name = A2COLE(test_data_storage.GetSuccessFullName().c_str());
   CComBSTR password = A2COLE(test_data_storage.GetSuccessPassword().c_str());
@@ -444,6 +453,16 @@ TEST_F(GcpReauthCredentialGlsRunnerTest, NormalReauth) {
                 L"comment", base::UTF8ToUTF16(test_data_storage.GetSuccessId()),
                 OLE2CW(email), &sid));
 
+  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));
+  }
+
   // Create provider and start logon.
   Microsoft::WRL::ComPtr<ICredentialProviderCredential> cred;
 
@@ -458,9 +477,14 @@ TEST_F(GcpReauthCredentialGlsRunnerTest, NormalReauth) {
 
   ASSERT_EQ(S_OK, StartLogonProcessAndWait());
 
-  // Teardown of the test should confirm that the logon was successful.
+  // Verify command line switch for show_tos.
+  ASSERT_EQ("0", test->GetShowTosFromCmdLine());
 }
 
+INSTANTIATE_TEST_SUITE_P(All,
+                         GcpNormalReauthCredentialGlsRunnerTest,
+                         ::testing::Values(true, false));
+
 TEST_F(GcpReauthCredentialGlsRunnerTest, NormalReauthWithoutEmail) {
   USES_CONVERSION;
   CredentialProviderSigninDialogTestDataStorage test_data_storage;

chrome/credential_provider/test/test_credential.h

@@ -5,14 +5,15 @@
 #ifndef CHROME_CREDENTIAL_PROVIDER_TEST_TEST_CREDENTIAL_H_
 #define CHROME_CREDENTIAL_PROVIDER_TEST_TEST_CREDENTIAL_H_
 
-#include <memory>
-#include <string>
-
 #include <atlbase.h>
 #include <atlcom.h>
 #include <atlcomcli.h>
 #include <credentialprovider.h>
 
+#include <memory>
+#include <string>
+
+#include "base/command_line.h"
 #include "base/strings/string16.h"
 #include "base/strings/string_util.h"
 #include "base/synchronization/waitable_event.h"
@@ -20,10 +21,6 @@
 #include "chrome/credential_provider/gaiacp/gaia_credential_base.h"
 #include "chrome/credential_provider/test/gls_runner_test_base.h"
 
-namespace base {
-class CommandLine;
-}
-
 namespace credential_provider {
 
 namespace testing {
@@ -56,6 +53,7 @@ class DECLSPEC_UUID("3710aa3a-13c7-44c2-bc38-09ba137804d8") ITestCredential
   virtual bool STDMETHODCALLTYPE IsGlsRunning() = 0;
   virtual bool STDMETHODCALLTYPE IsAdJoinedUser() = 0;
   virtual bool STDMETHODCALLTYPE ContainsIsAdJoinedUser() = 0;
+  virtual std::string STDMETHODCALLTYPE GetShowTosFromCmdLine() = 0;
 };
 
 // Test implementation of an ICredentialProviderCredential backed by a Gaia
@@ -97,6 +95,7 @@ class ATL_NO_VTABLE CTestCredentialBase : public T, public ITestCredential {
   bool STDMETHODCALLTYPE IsGlsRunning() override;
   bool STDMETHODCALLTYPE IsAdJoinedUser() override;
   bool STDMETHODCALLTYPE ContainsIsAdJoinedUser() override;
+  std::string STDMETHODCALLTYPE GetShowTosFromCmdLine() override;
 
   void SignalGlsCompletion();
 
@@ -137,6 +136,7 @@ class ATL_NO_VTABLE CTestCredentialBase : public T, public ITestCredential {
   bool gls_process_started_ = false;
   bool ignore_expected_gaia_id_ = false;
   bool fail_loading_gaia_logon_stub_ = false;
+  std::string show_tos_command_line_;
 };
 
 template <class T>
@@ -263,6 +263,11 @@ bool CTestCredentialBase<T>::ContainsIsAdJoinedUser() {
   return true;
 }
 
+template <class T>
+std::string CTestCredentialBase<T>::GetShowTosFromCmdLine() {
+  return show_tos_command_line_;
+}
+
 template <class T>
 BSTR CTestCredentialBase<T>::GetErrorText() {
   return error_text_;
@@ -308,6 +313,12 @@ HRESULT CTestCredentialBase<T>::ForkGaiaLogonStub(
     OSProcessManager* process_manager,
     const base::CommandLine& command_line,
     CGaiaCredentialBase::UIProcessInfo* uiprocinfo) {
+  // Record command_line parameter "show_tos" into global variable.
+  show_tos_command_line_ =
+      command_line.HasSwitch(kShowTosSwitch)
+          ? command_line.GetSwitchValueASCII(kShowTosSwitch)
+          : "0";
+
   if (fail_loading_gaia_logon_stub_)
     return E_FAIL;