Define flags to manipulate the behavior of GCPW SSO

Bug: 1038368
Change-Id: If6d1831cbda98084e9dfc7ee9387f0b77c96208f
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1984474
Commit-Queue: Yusuf Sengul <yusufsn@google.com>
Reviewed-by: Mihai Sardarescu <msarda@chromium.org>
Reviewed-by: Roger Tawa <rogerta@chromium.org>
Reviewed-by: Tien Mai <tienmai@chromium.org>
Cr-Commit-Position: refs/heads/master@{#736190}

chrome/browser/signin/signin_util_win.cc

@@ -89,9 +89,6 @@ void FinishImportCredentialsFromProvider(const CoreAccountId& account_id,
         signin_metrics::Reason::REASON_SIGNIN_PRIMARY_ACCOUNT, account_id,
         DiceTurnSyncOnHelper::SigninAbortedMode::KEEP_ACCOUNT);
   }
-
-  // Mark this profile as having been signed in with the credential provider.
-  profile->GetPrefs()->SetBoolean(prefs::kSignedInWithCredentialProvider, true);
 }
 
 // Start the process of importing credentials from the credential provider given
@@ -134,6 +131,65 @@ void ImportCredentialsFromProvider(Profile* profile,
                                        account_id, nullptr));
     }
   }
+
+  // Mark this profile as having been signed in with the credential provider.
+  profile->GetPrefs()->SetBoolean(prefs::kSignedInWithCredentialProvider, true);
+}
+
+// Extracts preferences to consider while signing in through credential
+// provider. The preferences are set by credential provider after a successful
+// login. They manipulate the behavior of Chrome when importing refresh_token
+// provided by credential provider. When |allow_import_only_on_first_run| is
+// set to true, importing refresh_token is only allowed during Chrome
+// first time run. If it is false, refresh_token is allowed to be imported in
+// subsequent runs. When |allow_import_when_primary_account_exists| is set to
+// false, importing refresh_token is only allowed when profile doesn't have a
+// primary account. If |allow_import_when_primary_account_exists| is set to
+// true, importing refresh_token is allowed even if the profile has primary
+// account for the user authenticated through credential provider.
+void ExtractCredentialImportPreferences(
+    base::string16* cred_provider_user_gaia_id,
+    bool* allow_import_only_on_first_run,
+    bool* allow_import_when_primary_account_exists) {
+  DCHECK(cred_provider_user_gaia_id);
+  DCHECK(allow_import_only_on_first_run);
+  DCHECK(allow_import_when_primary_account_exists);
+
+  // Initialize to more restricted configuration.
+  *allow_import_only_on_first_run = true;
+  *allow_import_when_primary_account_exists = false;
+  cred_provider_user_gaia_id->clear();
+
+  base::win::RegKey key;
+  if (key.Open(HKEY_CURRENT_USER, credential_provider::kRegHkcuAccountsPath,
+               KEY_READ) != ERROR_SUCCESS) {
+    return;
+  }
+
+  base::win::RegistryKeyIterator it(key.Handle(), L"");
+  if (!it.Valid() || it.SubkeyCount() != 1)
+    return;
+
+  base::win::RegKey key_account(key.Handle(), it.Name(), KEY_QUERY_VALUE);
+  if (!key_account.Valid())
+    return;
+
+  // No need to return immediately if reading following registries fail. They
+  // will set to be stricter by default. cred_provider_user_gaia_id will be
+  // correctly set, though.
+  DWORD reg_import_only_on_first_run = 1;
+  key_account.ReadValueDW(credential_provider::kAllowImportOnlyOnFirstRun,
+                          &reg_import_only_on_first_run);
+
+  DWORD reg_import_when_primary_account_exists = 0;
+  key_account.ReadValueDW(
+      credential_provider::kAllowImportWhenPrimaryAccountExists,
+      &reg_import_when_primary_account_exists);
+
+  *cred_provider_user_gaia_id = it.Name();
+  *allow_import_only_on_first_run = (reg_import_only_on_first_run == 1);
+  *allow_import_when_primary_account_exists =
+      (reg_import_when_primary_account_exists == 1);
 }
 
 // Attempt to sign in with a credentials from a system installed credential
@@ -208,15 +264,38 @@ void SetDiceTurnSyncOnHelperDelegateForTesting(
 }
 
 void SigninWithCredentialProviderIfPossible(Profile* profile) {
-  // Check to see if auto signin information is available.  Only applies if:
-  //
-  //  - This is first run.
-  //  - Opening the initial profile.
-  //  - Not already signed in.
-  if (!(first_run::IsChromeFirstRun() &&
-        g_browser_process->profile_manager()->GetInitialProfileDir() ==
-            profile->GetPath().BaseName() &&
-        !IdentityManagerFactory::GetForProfile(profile)->HasPrimaryAccount())) {
+  bool import_only_on_first_run = true;
+  bool import_when_primary_account_exists = false;
+  base::string16 cred_provider_user_gaia_id;
+
+  ExtractCredentialImportPreferences(&cred_provider_user_gaia_id,
+                                     &import_only_on_first_run,
+                                     &import_when_primary_account_exists);
+
+  if (cred_provider_user_gaia_id.empty())
+    return;
+
+  if (import_only_on_first_run && !first_run::IsChromeFirstRun())
+    return;
+
+  if (g_browser_process->profile_manager()->GetInitialProfileDir() !=
+      profile->GetPath().BaseName()) {
+    return;
+  }
+
+  auto* identity_manager = IdentityManagerFactory::GetForProfile(profile);
+  if (identity_manager->HasPrimaryAccount()) {
+    base::string16 gaia_id =
+        base::UTF8ToUTF16(identity_manager->GetPrimaryAccountInfo().gaia);
+
+    if (!import_when_primary_account_exists ||
+        (gaia_id != cred_provider_user_gaia_id)) {
+      return;
+    }
+
+    // If there is already a primary account in the profile, this means sync was
+    // turned on. So it shouldn't be turned on again.
+    TrySigninWithCredentialProvider(profile, gaia_id, false);
     return;
   }
 

chrome/browser/signin/signin_util_win_browsertest.cc

@@ -85,50 +85,27 @@ void AssertSigninStarted(bool expect_is_started, Profile* profile) {
 
 }  // namespace
 
-class SigninUtilWinBrowserTest
-    : public InProcessBrowserTest,
-      public testing::WithParamInterface<SigninUtilWinBrowserTestParams> {
- protected:
-  void SetUpCommandLine(base::CommandLine* command_line) override {
-    command_line->AppendSwitch(GetParam().is_first_run
-                                   ? switches::kForceFirstRun
-                                   : switches::kNoFirstRun);
-  }
-
-  bool SetUpUserDataDirectory() override {
-    registry_override_.OverrideRegistry(HKEY_CURRENT_USER);
-
-    base::win::RegKey key;
-    CreateRegKey(&key);
-
-    if (!GetParam().email.empty()) {
-      EXPECT_TRUE(key.Valid());
-      EXPECT_EQ(ERROR_SUCCESS,
-                key.WriteValue(
-                    base::ASCIIToUTF16(credential_provider::kKeyEmail).c_str(),
-                    GetParam().email.c_str()));
-    }
-
-    if (!GetParam().refresh_token.empty())
-      WriteRefreshToken(&key, GetParam().refresh_token);
-
-    if (GetParam().expect_is_started) {
-      signin_util::SetDiceTurnSyncOnHelperDelegateForTesting(
-          std::unique_ptr<DiceTurnSyncOnHelper::Delegate>(
-              new TestDiceTurnSyncOnHelperDelegate()));
-    }
-
-    return InProcessBrowserTest::SetUpUserDataDirectory();
-  }
+class BrowserTestHelper {
+ public:
+  BrowserTestHelper(const base::string16& gaia_id,
+                    const base::string16& email,
+                    const std::string& refresh_token,
+                    int import_only_on_first_run,
+                    int import_on_primary_account)
+      : gaia_id_(gaia_id),
+        email_(email),
+        refresh_token_(refresh_token),
+        import_only_on_first_run_(import_only_on_first_run),
+        import_on_primary_account_(import_on_primary_account) {}
 
+ protected:
   void CreateRegKey(base::win::RegKey* key) {
-    if (!GetParam().gaia_id.empty()) {
+    if (!gaia_id_.empty()) {
       EXPECT_EQ(
           ERROR_SUCCESS,
           key->Create(HKEY_CURRENT_USER,
                       credential_provider::kRegHkcuAccountsPath, KEY_WRITE));
-      EXPECT_EQ(ERROR_SUCCESS,
-                key->CreateKey(GetParam().gaia_id.c_str(), KEY_WRITE));
+      EXPECT_EQ(ERROR_SUCCESS, key->CreateKey(gaia_id_.c_str(), KEY_WRITE));
     }
   }
 
@@ -159,13 +136,90 @@ class SigninUtilWinBrowserTest
     EXPECT_EQ(ERROR_SUCCESS,
               key.Open(HKEY_CURRENT_USER,
                        credential_provider::kRegHkcuAccountsPath, KEY_READ));
-    EXPECT_EQ(ERROR_SUCCESS, key.OpenKey(GetParam().gaia_id.c_str(), KEY_READ));
+    EXPECT_EQ(ERROR_SUCCESS, key.OpenKey(gaia_id_.c_str(), KEY_READ));
     EXPECT_EQ(
         exists,
         key.HasValue(
             base::ASCIIToUTF16(credential_provider::kKeyRefreshToken).c_str()));
   }
 
+  void SetSigninUtilRegistry() {
+    base::win::RegKey key;
+    CreateRegKey(&key);
+
+    if (import_only_on_first_run_ != 2) {
+      EXPECT_TRUE(key.Valid());
+      EXPECT_EQ(ERROR_SUCCESS,
+                key.WriteValue(credential_provider::kAllowImportOnlyOnFirstRun,
+                               import_only_on_first_run_));
+    }
+
+    if (import_on_primary_account_ != 2) {
+      EXPECT_TRUE(key.Valid());
+      EXPECT_EQ(ERROR_SUCCESS,
+                key.WriteValue(
+                    credential_provider::kAllowImportWhenPrimaryAccountExists,
+                    import_on_primary_account_));
+    }
+
+    if (!email_.empty()) {
+      EXPECT_TRUE(key.Valid());
+      EXPECT_EQ(ERROR_SUCCESS,
+                key.WriteValue(
+                    base::ASCIIToUTF16(credential_provider::kKeyEmail).c_str(),
+                    email_.c_str()));
+    }
+
+    if (!refresh_token_.empty())
+      WriteRefreshToken(&key, refresh_token_);
+  }
+
+  bool IsPreTest() {
+    std::string test_name =
+        ::testing::UnitTest::GetInstance()->current_test_info()->name();
+    LOG(WARNING) << "test_name " << test_name;
+    return test_name.find("PRE_") != std::string::npos;
+  }
+
+ private:
+  base::string16 gaia_id_;
+  base::string16 email_;
+  std::string refresh_token_;
+  int import_only_on_first_run_;
+  int import_on_primary_account_;
+};
+
+class SigninUtilWinBrowserTest
+    : public BrowserTestHelper,
+      public InProcessBrowserTest,
+      public testing::WithParamInterface<SigninUtilWinBrowserTestParams> {
+ public:
+  SigninUtilWinBrowserTest()
+      : BrowserTestHelper(GetParam().gaia_id,
+                          GetParam().email,
+                          GetParam().refresh_token,
+                          2,
+                          2) {}
+
+ protected:
+  void SetUpCommandLine(base::CommandLine* command_line) override {
+    command_line->AppendSwitch(GetParam().is_first_run
+                                   ? switches::kForceFirstRun
+                                   : switches::kNoFirstRun);
+  }
+
+  bool SetUpUserDataDirectory() override {
+    registry_override_.OverrideRegistry(HKEY_CURRENT_USER);
+
+    signin_util::SetDiceTurnSyncOnHelperDelegateForTesting(
+        std::unique_ptr<DiceTurnSyncOnHelper::Delegate>(
+            new TestDiceTurnSyncOnHelperDelegate()));
+
+    SetSigninUtilRegistry();
+
+    return InProcessBrowserTest::SetUpUserDataDirectory();
+  }
+
  private:
   registry_util::RegistryOverrideManager registry_override_;
 };
@@ -235,6 +289,7 @@ IN_PROC_BROWSER_TEST_P(SigninUtilWinBrowserTest, NoReauthAfterSignout) {
 
 IN_PROC_BROWSER_TEST_P(SigninUtilWinBrowserTest, FixReauth) {
   ProfileManager* profile_manager = g_browser_process->profile_manager();
+
   ASSERT_EQ(1u, profile_manager->GetNumberOfProfiles());
 
   Profile* profile =
@@ -305,3 +360,169 @@ INSTANTIATE_TEST_SUITE_P(SigninUtilWinBrowserTest5,
                              /*email=*/L"foo@gmail.com",
                              /*refresh_token=*/"lst-123456",
                              /*expect_is_started=*/true)));
+
+INSTANTIATE_TEST_SUITE_P(SigninUtilWinBrowserTest6,
+                         SigninUtilWinBrowserTest,
+                         testing::Values(SigninUtilWinBrowserTestParams(
+                             /*is_first_run=*/false,
+                             /*gaia_id=*/L"gaia-123456",
+                             /*email=*/L"foo@gmail.com",
+                             /*refresh_token=*/"lst-123456",
+                             /*expect_is_started=*/false)));
+
+struct ExistingWinBrowserSigninUtilTestParams : SigninUtilWinBrowserTestParams {
+  ExistingWinBrowserSigninUtilTestParams(
+      const base::string16& gaia_id,
+      const base::string16& email,
+      const std::string& refresh_token,
+      const int allow_import_only_on_first_run,
+      const int allow_import_on_primary_account,
+      const base::string16& existing_email,
+      bool expect_is_started)
+      : SigninUtilWinBrowserTestParams(false,
+                                       gaia_id,
+                                       email,
+                                       refresh_token,
+                                       expect_is_started),
+        import_only_on_first_run(allow_import_only_on_first_run),
+        import_on_primary_account(allow_import_on_primary_account),
+        existing_email(existing_email) {}
+
+  int import_only_on_first_run;
+  int import_on_primary_account;
+  base::string16 existing_email;
+};
+
+class ExistingWinBrowserSigninUtilTest
+    : public BrowserTestHelper,
+      public InProcessBrowserTest,
+      public testing::WithParamInterface<
+          ExistingWinBrowserSigninUtilTestParams> {
+ public:
+  ExistingWinBrowserSigninUtilTest()
+      : BrowserTestHelper(GetParam().gaia_id,
+                          GetParam().email,
+                          GetParam().refresh_token,
+                          GetParam().import_only_on_first_run,
+                          GetParam().import_on_primary_account) {}
+
+ protected:
+  bool SetUpUserDataDirectory() override {
+    registry_override_.OverrideRegistry(HKEY_CURRENT_USER);
+
+    signin_util::SetDiceTurnSyncOnHelperDelegateForTesting(
+        std::unique_ptr<DiceTurnSyncOnHelper::Delegate>(
+            new TestDiceTurnSyncOnHelperDelegate()));
+    if (!IsPreTest())
+      SetSigninUtilRegistry();
+
+    return InProcessBrowserTest::SetUpUserDataDirectory();
+  }
+
+ private:
+  registry_util::RegistryOverrideManager registry_override_;
+};
+
+IN_PROC_BROWSER_TEST_P(ExistingWinBrowserSigninUtilTest,
+                       PRE_ExistingWinBrowser) {
+  ProfileManager* profile_manager = g_browser_process->profile_manager();
+  Profile* profile =
+      profile_manager->GetLastUsedProfile(profile_manager->user_data_dir());
+
+  ASSERT_EQ(profile_manager->GetInitialProfileDir(),
+            profile->GetPath().BaseName());
+
+  if (!GetParam().existing_email.empty()) {
+    auto* identity_manager = IdentityManagerFactory::GetForProfile(profile);
+
+    ASSERT_TRUE(identity_manager);
+
+    signin::MakePrimaryAccountAvailable(
+        identity_manager, base::UTF16ToUTF8(GetParam().existing_email));
+
+    ASSERT_TRUE(identity_manager->HasPrimaryAccount());
+  }
+}
+
+IN_PROC_BROWSER_TEST_P(ExistingWinBrowserSigninUtilTest, ExistingWinBrowser) {
+  ProfileManager* profile_manager = g_browser_process->profile_manager();
+  ASSERT_EQ(1u, profile_manager->GetNumberOfProfiles());
+
+  Profile* profile =
+      profile_manager->GetLastUsedProfile(profile_manager->user_data_dir());
+  ASSERT_EQ(profile_manager->GetInitialProfileDir(),
+            profile->GetPath().BaseName());
+
+  AssertSigninStarted(GetParam().expect_is_started, profile);
+
+  // If a refresh token was specified and a sign in attempt was expected, make
+  // sure the refresh token was removed from the registry.
+  if (!GetParam().refresh_token.empty() && GetParam().expect_is_started)
+    ExpectRefreshTokenExists(false);
+}
+
+INSTANTIATE_TEST_SUITE_P(OnlyAllowFirstRun,
+                         ExistingWinBrowserSigninUtilTest,
+                         testing::Values(ExistingWinBrowserSigninUtilTestParams(
+                             /*gaia_id=*/L"gaia-123456",
+                             /*email=*/L"foo@gmail.com",
+                             /*refresh_token=*/"lst-123456",
+                             /*import_only_on_first_run=*/1,
+                             /*import_on_primary_account=*/0,
+                             /*existing_email=*/base::string16(),
+                             /*expect_is_started=*/false)));
+
+INSTANTIATE_TEST_SUITE_P(AllowSubsequentRun,
+                         ExistingWinBrowserSigninUtilTest,
+                         testing::Values(ExistingWinBrowserSigninUtilTestParams(
+                             /*gaia_id=*/L"gaia-123456",
+                             /*email=*/L"foo@gmail.com",
+                             /*refresh_token=*/"lst-123456",
+                             /*import_only_on_first_run=*/0,
+                             /*import_on_primary_account=*/0,
+                             /*existing_email=*/base::string16(),
+                             /*expect_is_started=*/true)));
+
+INSTANTIATE_TEST_SUITE_P(OnlyAllowProfileWithNoPrimaryAccount,
+                         ExistingWinBrowserSigninUtilTest,
+                         testing::Values(ExistingWinBrowserSigninUtilTestParams(
+                             /*gaia_id=*/L"gaia_id_for_foo_gmail.com",
+                             /*email=*/L"foo@gmail.com",
+                             /*refresh_token=*/"lst-123456",
+                             /*import_only_on_first_run=*/0,
+                             /*import_on_primary_account=*/0,
+                             /*existing_email=*/L"bar@gmail.com",
+                             /*expect_is_started=*/false)));
+
+INSTANTIATE_TEST_SUITE_P(AllowProfileWithPrimaryAccount_DifferentUser,
+                         ExistingWinBrowserSigninUtilTest,
+                         testing::Values(ExistingWinBrowserSigninUtilTestParams(
+                             /*gaia_id=*/L"gaia_id_for_foo_gmail.com",
+                             /*email=*/L"foo@gmail.com",
+                             /*refresh_token=*/"lst-123456",
+                             /*import_only_on_first_run=*/0,
+                             /*import_on_primary_account=*/1,
+                             /*existing_email=*/L"bar@gmail.com",
+                             /*expect_is_started=*/false)));
+
+INSTANTIATE_TEST_SUITE_P(AllowProfileWithPrimaryAccount_DisabledImport,
+                         ExistingWinBrowserSigninUtilTest,
+                         testing::Values(ExistingWinBrowserSigninUtilTestParams(
+                             /*gaia_id=*/L"gaia_id_for_foo_gmail.com",
+                             /*email=*/L"foo@gmail.com",
+                             /*refresh_token=*/"lst-123456",
+                             /*import_only_on_first_run=*/0,
+                             /*import_on_primary_account=*/0,
+                             /*existing_email=*/L"foo@gmail.com",
+                             /*expect_is_started=*/false)));
+
+INSTANTIATE_TEST_SUITE_P(AllowProfileWithPrimaryAccount_SameUser,
+                         ExistingWinBrowserSigninUtilTest,
+                         testing::Values(ExistingWinBrowserSigninUtilTestParams(
+                             /*gaia_id=*/L"gaia_id_for_foo_gmail.com",
+                             /*email=*/L"foo@gmail.com",
+                             /*refresh_token=*/"lst-123456",
+                             /*import_only_on_first_run=*/0,
+                             /*import_on_primary_account=*/1,
+                             /*existing_email=*/L"foo@gmail.com",
+                             /*expect_is_started=*/true)));

chrome/credential_provider/common/gcp_strings.cc

@@ -92,6 +92,12 @@ const char kGcpwEndpointPathPromoParameter[] = "gcpw_endpoint_path";
 
 const wchar_t kRunAsCrashpadHandlerEntryPoint[] = L"RunAsCrashpadHandler";
 
+// Flags to manipulate behavior of Chrome when importing credentials for the
+// account signs in through GCPW.
+const wchar_t kAllowImportOnlyOnFirstRun[] = L"allow_import_only_on_first_run";
+const wchar_t kAllowImportWhenPrimaryAccountExists[] =
+    L"allow_import_when_primary_exists";
+
 #if BUILDFLAG(GOOGLE_CHROME_BRANDING)
 const wchar_t kRegHkcuAccountsPath[] = L"Software\\Google\\Accounts";
 #else

chrome/credential_provider/common/gcp_strings.h

@@ -96,9 +96,14 @@ extern const char kEmailDomainsSeparator[];
 extern const char kValidateGaiaIdSigninPromoParameter[];
 extern const char kGcpwEndpointPathPromoParameter[];
 
-// Crashpad related constants
+// Crashpad related constants.
 extern const wchar_t kRunAsCrashpadHandlerEntryPoint[];
 
+// Flags to manipulate behavior of Chrome when importing credentials for the
+// account signs in through GCPW.
+extern const wchar_t kAllowImportOnlyOnFirstRun[];
+extern const wchar_t kAllowImportWhenPrimaryAccountExists[];
+
 // HKCU account information path in the hive of the OS user.
 extern const wchar_t kRegHkcuAccountsPath[];
 

chrome/credential_provider/gaiacp/scoped_user_profile.cc

@@ -571,6 +571,26 @@ HRESULT ScopedUserProfile::SaveAccountInfo(const base::Value& properties) {
       LOGFN(ERROR) << "key.WriteValue(" << sid << ", RT) hr=" << putHR(hr);
       return hr;
     }
+
+    // Set both of the settings to stricter defaults.
+    sts = key.WriteValue(kAllowImportOnlyOnFirstRun,
+                         GetGlobalFlagOrDefault(kAllowImportOnlyOnFirstRun, 1));
+    if (sts != ERROR_SUCCESS) {
+      HRESULT hr = HRESULT_FROM_WIN32(sts);
+      LOGFN(ERROR) << "key.WriteValue(" << sid
+                   << ", import_on_first_run) hr=" << putHR(hr);
+      return hr;
+    }
+
+    sts = key.WriteValue(
+        kAllowImportWhenPrimaryAccountExists,
+        GetGlobalFlagOrDefault(kAllowImportWhenPrimaryAccountExists, 0));
+    if (sts != ERROR_SUCCESS) {
+      HRESULT hr = HRESULT_FROM_WIN32(sts);
+      LOGFN(ERROR) << "key.WriteValue(" << sid
+                   << ", import_on_no_primary_account) hr=" << putHR(hr);
+      return hr;
+    }
   }
 
   // This code for setting profile pictures is specific for windows 8+.