[signin] Add strike system to the profile switch bubble

This CL adds an experiment parameter that allows to control after how
many times user declined the profile switch bubble we stop showing the
profile switch bubble.

The parameter is called "max_profile_switch_declined_count". It can have
the following values:
- N < 0 - we always show the bubble when eligible
- N == 0 - we never show the bubble
- N > 0 - we stop showing the bubble after the user declines it N times

Bug: 1166851
Change-Id: Ic165216a338a11d5730e582cd3c2045d107266f9
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2630805Reviewed-by: David Roger <droger@chromium.org>
Commit-Queue: Alex Ilin <alexilin@chromium.org>
Cr-Commit-Position: refs/heads/master@{#844020}

chrome/browser/signin/dice_web_signin_interceptor.cc

@@ -9,6 +9,7 @@
 #include "base/check.h"
 #include "base/hash/hash.h"
 #include "base/i18n/case_conversion.h"
+#include "base/metrics/field_trial_params.h"
 #include "base/metrics/histogram_functions.h"
 #include "base/optional.h"
 #include "base/strings/utf_string_conversions.h"
@@ -48,6 +49,8 @@ namespace {
 
 constexpr char kProfileCreationInterceptionDeclinedPref[] =
     "signin.ProfileCreationInterceptionDeclinedPref";
+constexpr char kProfileSwitchInterceptionDeclinedPref[] =
+    "signin.ProfileSwitchInterceptionDeclinedPref";
 
 void RecordSigninInterceptionHeuristicOutcome(
     SigninInterceptionHeuristicOutcome outcome) {
@@ -148,6 +151,7 @@ DiceWebSigninInterceptor::~DiceWebSigninInterceptor() = default;
 void DiceWebSigninInterceptor::RegisterProfilePrefs(
     user_prefs::PrefRegistrySyncable* registry) {
   registry->RegisterDictionaryPref(kProfileCreationInterceptionDeclinedPref);
+  registry->RegisterDictionaryPref(kProfileSwitchInterceptionDeclinedPref);
   registry->RegisterBooleanPref(prefs::kSigninInterceptionEnabled, true);
 }
 
@@ -176,6 +180,10 @@ DiceWebSigninInterceptor::GetHeuristicOutcome(
       email,
       &g_browser_process->profile_manager()->GetProfileAttributesStorage());
   if (switch_to_entry) {
+    if (HasUserDeclinedProfileSwitch(email)) {
+      return SigninInterceptionHeuristicOutcome::
+          kAbortUserDeclinedProfileForAccount;
+    }
     if (entry)
       *entry = switch_to_entry;
     return SigninInterceptionHeuristicOutcome::kInterceptProfileSwitch;
@@ -275,7 +283,8 @@ void DiceWebSigninInterceptor::MaybeInterceptWebSignin(
       interception_bubble_handle_ = delegate_->ShowSigninInterceptionBubble(
           web_contents, bubble_parameters,
           base::BindOnce(&DiceWebSigninInterceptor::OnProfileSwitchChoice,
-                         base::Unretained(this), entry->GetPath()));
+                         base::Unretained(this), account_info->email,
+                         entry->GetPath()));
       was_interception_ui_displayed_ = true;
     } else {
       // Interception is aborted.
@@ -484,9 +493,12 @@ void DiceWebSigninInterceptor::OnProfileCreationChoice(
 }
 
 void DiceWebSigninInterceptor::OnProfileSwitchChoice(
+    const std::string& email,
     const base::FilePath& profile_path,
     SigninInterceptionResult switch_profile) {
   if (switch_profile != SigninInterceptionResult::kAccepted) {
+    if (switch_profile == SigninInterceptionResult::kDeclined)
+      RecordProfileSwitchDeclined(email);
     Reset();
     return;
   }
@@ -570,8 +582,7 @@ void DiceWebSigninInterceptor::RecordProfileCreationDeclined(
                               kProfileCreationInterceptionDeclinedPref);
   std::string key = GetPersistentEmailHash(email);
   base::Optional<int> declined_count = update->FindIntKey(key);
-  update->SetIntKey(
-      key, declined_count.has_value() ? declined_count.value() + 1 : 1);
+  update->SetIntKey(key, declined_count.value_or(0) + 1);
 }
 
 bool DiceWebSigninInterceptor::HasUserDeclinedProfileCreation(
@@ -585,3 +596,31 @@ bool DiceWebSigninInterceptor::HasUserDeclinedProfileCreation(
   return declined_count &&
          declined_count.value() >= kMaxProfileCreationDeclinedCount;
 }
+
+void DiceWebSigninInterceptor::RecordProfileSwitchDeclined(
+    const std::string& email) {
+  DictionaryPrefUpdate update(profile_->GetPrefs(),
+                              kProfileSwitchInterceptionDeclinedPref);
+  std::string key = GetPersistentEmailHash(email);
+  base::Optional<int> declined_count = update->FindIntKey(key);
+  update->SetIntKey(key, declined_count.value_or(0) + 1);
+}
+
+bool DiceWebSigninInterceptor::HasUserDeclinedProfileSwitch(
+    const std::string& email) const {
+  const base::DictionaryValue* pref_data = profile_->GetPrefs()->GetDictionary(
+      kProfileSwitchInterceptionDeclinedPref);
+  base::Optional<int> declined_count =
+      pref_data->FindIntKey(GetPersistentEmailHash(email));
+
+  // The limit is controlled by an experiment. Zero value completely turns off
+  // the profile switch bubble. Negative values mean there is no limit. By
+  // default, there is no limit.
+  int max_profile_switch_declined_count =
+      base::GetFieldTrialParamByFeatureAsInt(
+          kDiceWebSigninInterceptionFeature,
+          "max_profile_switch_declined_count", -1);
+
+  return max_profile_switch_declined_count >= 0 &&
+         declined_count.value_or(0) >= max_profile_switch_declined_count;
+}

chrome/browser/signin/dice_web_signin_interceptor.h

@@ -271,7 +271,8 @@ class DiceWebSigninInterceptor : public KeyedService,
                                SigninInterceptionResult create);
   // Called after the user chose whether the session should continue in a new
   // profile.
-  void OnProfileSwitchChoice(const base::FilePath& profile_path,
+  void OnProfileSwitchChoice(const std::string& email,
+                             const base::FilePath& profile_path,
                              SigninInterceptionResult switch_profile);
 
   // Called when the new profile is created or loaded from disk.
@@ -287,17 +288,23 @@ class DiceWebSigninInterceptor : public KeyedService,
   // Returns a 8-bit hash of the email that can be persisted.
   static std::string GetPersistentEmailHash(const std::string& email);
 
-  // Should be called when the user declines profile creation, in order to
-  // remember their decision. This information is stored in prefs. Only a hash
-  // of the email is saved, as Chrome does not need to store the actual email,
-  // but only need to compare emails. The hash has low entropy to ensure it
-  // cannot be reversed.
+  // Should be called when the user declines profile creation or profile switch,
+  // in order to remember their decision. This information is stored in prefs.
+  // Only a hash of the email is saved, as Chrome does not need to store the
+  // actual email, but only need to compare emails. The hash has low entropy to
+  // ensure it cannot be reversed.
   void RecordProfileCreationDeclined(const std::string& email);
+  void RecordProfileSwitchDeclined(const std::string& email);
 
   // Checks if the user previously declined 2 times creating a new profile for
   // this account.
   bool HasUserDeclinedProfileCreation(const std::string& email) const;
 
+  // Checks if the user previously declined more than a threshold number of
+  // times switching to a new profile for this account. The limit is set up
+  // via an experiment parameter.
+  bool HasUserDeclinedProfileSwitch(const std::string& email) const;
+
   Profile* const profile_;
   signin::IdentityManager* const identity_manager_;
   std::unique_ptr<Delegate> delegate_;

chrome/browser/signin/dice_web_signin_interceptor_unittest.cc

@@ -7,6 +7,7 @@
 #include <memory>
 
 #include "base/callback.h"
+#include "base/strings/string_number_conversions.h"
 #include "base/strings/utf_string_conversions.h"
 #include "base/test/metrics/histogram_tester.h"
 #include "base/test/scoped_feature_list.h"
@@ -482,7 +483,7 @@ TEST_F(DiceWebSigninInterceptorTest, InterceptionInProgress) {
   MaybeIntercept(account_info.account_id);
 }
 
-TEST_F(DiceWebSigninInterceptorTest, DeclineRepeatedly) {
+TEST_F(DiceWebSigninInterceptorTest, DeclineCreationRepeatedly) {
   base::HistogramTester histogram_tester;
   AccountInfo primary_account_info =
       identity_test_env()->MakeUnconsentedPrimaryAccountAvailable(
@@ -523,21 +524,120 @@ TEST_F(DiceWebSigninInterceptorTest, DeclineRepeatedly) {
       SigninInterceptionHeuristicOutcome::kAbortUserDeclinedProfileForAccount,
       1);
 
-  // Even with a slightly different email.
+  // Another account can still be intercepted.
+  account_info.email = "oscar@example.com";
+  identity_test_env()->UpdateAccountInfoForAccount(account_info);
+  expected_parameters = {
+      DiceWebSigninInterceptor::SigninInterceptionType::kEnterprise,
+      account_info, primary_account_info, SkColor()};
+  EXPECT_CALL(*mock_delegate(),
+              ShowSigninInterceptionBubble(
+                  web_contents(), MatchBubbleParameters(expected_parameters),
+                  testing::_));
+  MaybeIntercept(account_info.account_id);
+  histogram_tester.ExpectBucketCount(
+      "Signin.Intercept.HeuristicOutcome",
+      SigninInterceptionHeuristicOutcome::kInterceptEnterprise,
+      kMaxProfileCreationDeclinedCount + 1);
+  EXPECT_EQ(interceptor()->is_interception_in_progress(), true);
+}
+
+TEST_F(DiceWebSigninInterceptorTest, DeclineSwitchRepeatedly_NoLimit) {
+  base::HistogramTester histogram_tester;
+  // Setup for profile switch interception.
+  std::string email = "bob@example.com";
+  AccountInfo account_info = identity_test_env()->MakeAccountAvailable(email);
+  Profile* profile_2 = CreateTestingProfile("Profile 2");
+  ProfileAttributesEntry* entry = nullptr;
+  ASSERT_TRUE(profile_attributes_storage()->GetProfileAttributesWithPath(
+      profile_2->GetPath(), &entry));
+  entry->SetAuthInfo(account_info.gaia, base::UTF8ToUTF16(email),
+                     /*is_consented_primary_account=*/false);
+
+  // Test that the profile switch can be declined multiple times.
+  DiceWebSigninInterceptor::Delegate::BubbleParameters expected_parameters = {
+      DiceWebSigninInterceptor::SigninInterceptionType::kProfileSwitch,
+      account_info, AccountInfo(), SkColor()};
+  for (int i = 0; i < 10; ++i) {
+    EXPECT_CALL(*mock_delegate(),
+                ShowSigninInterceptionBubble(
+                    web_contents(), MatchBubbleParameters(expected_parameters),
+                    testing::_))
+        .WillOnce(testing::WithArg<2>(testing::Invoke(
+            [](base::OnceCallback<void(SigninInterceptionResult)> callback) {
+              std::move(callback).Run(SigninInterceptionResult::kDeclined);
+              return nullptr;
+            })));
+    MaybeIntercept(account_info.account_id);
+    EXPECT_EQ(interceptor()->is_interception_in_progress(), false);
+    histogram_tester.ExpectUniqueSample(
+        "Signin.Intercept.HeuristicOutcome",
+        SigninInterceptionHeuristicOutcome::kInterceptProfileSwitch, i + 1);
+  }
+}
+
+TEST_F(DiceWebSigninInterceptorTest,
+       DeclineSwitchRepeatedly_LimitedByExperiment) {
+  const int kMaxProfileSwitchDeclinedCount = 3;
+  base::test::ScopedFeatureList feature_list;
+  feature_list.InitAndEnableFeatureWithParameters(
+      kDiceWebSigninInterceptionFeature,
+      {{"max_profile_switch_declined_count",
+        base::NumberToString(kMaxProfileSwitchDeclinedCount)}});
+
+  base::HistogramTester histogram_tester;
+  // Setup for profile switch interception.
+  std::string email = "bob@example.com";
+  AccountInfo account_info = identity_test_env()->MakeAccountAvailable(email);
+  Profile* profile_2 = CreateTestingProfile("Profile 2");
+  ProfileAttributesEntry* entry = nullptr;
+  ASSERT_TRUE(profile_attributes_storage()->GetProfileAttributesWithPath(
+      profile_2->GetPath(), &entry));
+  entry->SetAuthInfo(account_info.gaia, base::UTF8ToUTF16(email),
+                     /*is_consented_primary_account=*/false);
+
+  // Decline the interception kMaxProfileSwitchDeclinedCount times.
+  DiceWebSigninInterceptor::Delegate::BubbleParameters expected_parameters = {
+      DiceWebSigninInterceptor::SigninInterceptionType::kProfileSwitch,
+      account_info, AccountInfo(), SkColor()};
+  for (int i = 0; i < kMaxProfileSwitchDeclinedCount; ++i) {
+    EXPECT_CALL(*mock_delegate(),
+                ShowSigninInterceptionBubble(
+                    web_contents(), MatchBubbleParameters(expected_parameters),
+                    testing::_))
+        .WillOnce(testing::WithArg<2>(testing::Invoke(
+            [](base::OnceCallback<void(SigninInterceptionResult)> callback) {
+              std::move(callback).Run(SigninInterceptionResult::kDeclined);
+              return nullptr;
+            })));
+    MaybeIntercept(account_info.account_id);
+    EXPECT_EQ(interceptor()->is_interception_in_progress(), false);
+    histogram_tester.ExpectUniqueSample(
+        "Signin.Intercept.HeuristicOutcome",
+        SigninInterceptionHeuristicOutcome::kInterceptProfileSwitch, i + 1);
+  }
+
+  // Next time the interception is not shown again.
   MaybeIntercept(account_info.account_id);
-  account_info.email = "al.ice@example.com";
   EXPECT_EQ(interceptor()->is_interception_in_progress(), false);
   histogram_tester.ExpectBucketCount(
       "Signin.Intercept.HeuristicOutcome",
       SigninInterceptionHeuristicOutcome::kAbortUserDeclinedProfileForAccount,
-      2);
+      1);
 
   // Another account can still be intercepted.
   account_info.email = "oscar@example.com";
   identity_test_env()->UpdateAccountInfoForAccount(account_info);
+  Profile* profile_3 = CreateTestingProfile("Profile 3");
+  ProfileAttributesEntry* entry_2 = nullptr;
+  ASSERT_TRUE(profile_attributes_storage()->GetProfileAttributesWithPath(
+      profile_3->GetPath(), &entry_2));
+  entry_2->SetAuthInfo(account_info.gaia, base::UTF8ToUTF16(account_info.email),
+                       /*is_consented_primary_account=*/false);
+
   expected_parameters = {
-      DiceWebSigninInterceptor::SigninInterceptionType::kEnterprise,
-      account_info, primary_account_info, SkColor()};
+      DiceWebSigninInterceptor::SigninInterceptionType::kProfileSwitch,
+      account_info, AccountInfo(), SkColor()};
   EXPECT_CALL(*mock_delegate(),
               ShowSigninInterceptionBubble(
                   web_contents(), MatchBubbleParameters(expected_parameters),
@@ -545,11 +645,38 @@ TEST_F(DiceWebSigninInterceptorTest, DeclineRepeatedly) {
   MaybeIntercept(account_info.account_id);
   histogram_tester.ExpectBucketCount(
       "Signin.Intercept.HeuristicOutcome",
-      SigninInterceptionHeuristicOutcome::kInterceptEnterprise,
-      kMaxProfileCreationDeclinedCount + 1);
+      SigninInterceptionHeuristicOutcome::kInterceptProfileSwitch,
+      kMaxProfileSwitchDeclinedCount + 1);
   EXPECT_EQ(interceptor()->is_interception_in_progress(), true);
 }
 
+TEST_F(DiceWebSigninInterceptorTest,
+       DeclineSwitchRepeatedly_DisabledByExperiment) {
+  base::test::ScopedFeatureList feature_list;
+  feature_list.InitAndEnableFeatureWithParameters(
+      kDiceWebSigninInterceptionFeature,
+      {{"max_profile_switch_declined_count", "0"}});
+
+  base::HistogramTester histogram_tester;
+  // Setup for profile switch interception.
+  std::string email = "bob@example.com";
+  AccountInfo account_info = identity_test_env()->MakeAccountAvailable(email);
+  Profile* profile_2 = CreateTestingProfile("Profile 2");
+  ProfileAttributesEntry* entry = nullptr;
+  ASSERT_TRUE(profile_attributes_storage()->GetProfileAttributesWithPath(
+      profile_2->GetPath(), &entry));
+  entry->SetAuthInfo(account_info.gaia, base::UTF8ToUTF16(email),
+                     /*is_consented_primary_account=*/false);
+
+  // The interception is not shown even at first attempt.
+  MaybeIntercept(account_info.account_id);
+  EXPECT_EQ(interceptor()->is_interception_in_progress(), false);
+  histogram_tester.ExpectBucketCount(
+      "Signin.Intercept.HeuristicOutcome",
+      SigninInterceptionHeuristicOutcome::kAbortUserDeclinedProfileForAccount,
+      1);
+}
+
 TEST_F(DiceWebSigninInterceptorTest, PersistentHash) {
   // The hash is persistent (the value should never change).
   EXPECT_EQ("email_174",
@@ -558,10 +685,13 @@ TEST_F(DiceWebSigninInterceptorTest, PersistentHash) {
   EXPECT_NE(interceptor()->GetPersistentEmailHash("bob@gmail.com"),
             interceptor()->GetPersistentEmailHash("alice@example.com"));
   // Equivalent emails get the same hash.
-  EXPECT_EQ(interceptor()->GetPersistentEmailHash("bo.b@gmail.com"),
-            interceptor()->GetPersistentEmailHash("bob@gmail.com"));
   EXPECT_EQ(interceptor()->GetPersistentEmailHash("bob"),
             interceptor()->GetPersistentEmailHash("bob@gmail.com"));
+  EXPECT_EQ(interceptor()->GetPersistentEmailHash("bo.b@gmail.com"),
+            interceptor()->GetPersistentEmailHash("bob@gmail.com"));
+  // Dots are removed only for gmail accounts.
+  EXPECT_NE(interceptor()->GetPersistentEmailHash("alice@example.com"),
+            interceptor()->GetPersistentEmailHash("al.ice@example.com"));
 }
 
 // Interception other than the profile switch require at least 2 accounts.