Fix two cases in IsPingingEnabled(..)

This Cl fixes two problems of ChromePasswordProtectionService::
IsPingingEnabled().

1. We should specifically check if user signed in for sync password
reuse case before sending out the ping.
2. For saved password reuse pings, we don't need to check for
sync status.

Bug: 876788
Change-Id: Iec7ffdc1d1b1af5ae73c9c4b6e8a78930c75aad6
Reviewed-on: https://chromium-review.googlesource.com/1194827
Commit-Queue: Jialiu Lin <jialiul@chromium.org>
Reviewed-by: Daniel Rubery <drubery@chromium.org>
Cr-Commit-Position: refs/heads/master@{#589285}

chrome/browser/safe_browsing/chrome_password_protection_service.cc

@@ -576,6 +576,7 @@ bool ChromePasswordProtectionService::IsIncognito() {
 
 bool ChromePasswordProtectionService::IsPingingEnabled(
     LoginReputationClientRequest::TriggerType trigger_type,
+    ReusedPasswordType password_type,
     RequestOutcome* reason) {
   if (!IsSafeBrowsingEnabled()) {
     *reason = RequestOutcome::SAFE_BROWSING_DISABLED;
@@ -583,6 +584,15 @@ bool ChromePasswordProtectionService::IsPingingEnabled(
   }
 
   if (trigger_type == LoginReputationClientRequest::PASSWORD_REUSE_EVENT) {
+    if (password_type == PasswordReuseEvent::SAVED_PASSWORD)
+      return true;
+
+    if (password_type == PasswordReuseEvent::SIGN_IN_PASSWORD &&
+        GetSyncAccountType() == PasswordReuseEvent::NOT_SIGNED_IN) {
+      *reason = RequestOutcome::USER_NOT_SIGNED_IN;
+      return false;
+    }
+
     PasswordProtectionTrigger trigger_level =
         GetPasswordProtectionWarningTriggerPref();
     if (trigger_level == PASSWORD_REUSE) {
@@ -799,6 +809,7 @@ void ChromePasswordProtectionService::MaybeLogPasswordReuseLookupEvent(
     case RequestOutcome::DISABLED_DUE_TO_FEATURE_DISABLED:
     case RequestOutcome::DISABLED_DUE_TO_USER_POPULATION:
     case RequestOutcome::SAFE_BROWSING_DISABLED:
+    case RequestOutcome::USER_NOT_SIGNED_IN:
       MaybeLogPasswordReuseLookupResult(web_contents,
                                         PasswordReuseLookup::REQUEST_FAILURE);
       break;

chrome/browser/safe_browsing/chrome_password_protection_service.h

@@ -202,9 +202,10 @@ class ChromePasswordProtectionService : public PasswordProtectionService {
 
   bool IsIncognito() override;
 
-  // Checks if pinging should be enabled based on the |trigger_type| and user
-  // state, updates |reason| accordingly.
+  // Checks if pinging should be enabled based on the |trigger_type|,
+  // |password_type| and user state, updates |reason| accordingly.
   bool IsPingingEnabled(LoginReputationClientRequest::TriggerType trigger_type,
+                        ReusedPasswordType password_type,
                         RequestOutcome* reason) override;
 
   // If user enabled history syncing.
@@ -267,7 +268,9 @@ class ChromePasswordProtectionService : public PasswordProtectionService {
   FRIEND_TEST_ALL_PREFIXES(ChromePasswordProtectionServiceTest,
                            VerifyUserPopulationForPasswordOnFocusPing);
   FRIEND_TEST_ALL_PREFIXES(ChromePasswordProtectionServiceTest,
-                           VerifyUserPopulationForProtectedPasswordEntryPing);
+                           VerifyUserPopulationForSyncPasswordEntryPing);
+  FRIEND_TEST_ALL_PREFIXES(ChromePasswordProtectionServiceTest,
+                           VerifyUserPopulationForSavedPasswordEntryPing);
   FRIEND_TEST_ALL_PREFIXES(ChromePasswordProtectionServiceTest,
                            VerifyPasswordReuseUserEventNotRecorded);
   FRIEND_TEST_ALL_PREFIXES(ChromePasswordProtectionServiceTest,

chrome/browser/safe_browsing/chrome_password_protection_service_unittest.cc

@@ -303,62 +303,121 @@ TEST_F(ChromePasswordProtectionServiceTest,
   RequestOutcome reason;
   service_->ConfigService(false /*incognito*/, false /*SBER*/);
   EXPECT_FALSE(service_->IsPingingEnabled(
-      LoginReputationClientRequest::UNFAMILIAR_LOGIN_PAGE, &reason));
+      LoginReputationClientRequest::UNFAMILIAR_LOGIN_PAGE,
+      PasswordReuseEvent::REUSED_PASSWORD_TYPE_UNKNOWN, &reason));
   EXPECT_EQ(RequestOutcome::DISABLED_DUE_TO_USER_POPULATION, reason);
 
   service_->ConfigService(false /*incognito*/, true /*SBER*/);
   EXPECT_TRUE(service_->IsPingingEnabled(
-      LoginReputationClientRequest::UNFAMILIAR_LOGIN_PAGE, &reason));
+      LoginReputationClientRequest::UNFAMILIAR_LOGIN_PAGE,
+      PasswordReuseEvent::REUSED_PASSWORD_TYPE_UNKNOWN, &reason));
 
   service_->ConfigService(true /*incognito*/, false /*SBER*/);
   EXPECT_FALSE(service_->IsPingingEnabled(
-      LoginReputationClientRequest::UNFAMILIAR_LOGIN_PAGE, &reason));
+      LoginReputationClientRequest::UNFAMILIAR_LOGIN_PAGE,
+      PasswordReuseEvent::REUSED_PASSWORD_TYPE_UNKNOWN, &reason));
   EXPECT_EQ(RequestOutcome::DISABLED_DUE_TO_INCOGNITO, reason);
 
   service_->ConfigService(true /*incognito*/, true /*SBER*/);
   EXPECT_FALSE(service_->IsPingingEnabled(
-      LoginReputationClientRequest::UNFAMILIAR_LOGIN_PAGE, &reason));
+      LoginReputationClientRequest::UNFAMILIAR_LOGIN_PAGE,
+      PasswordReuseEvent::REUSED_PASSWORD_TYPE_UNKNOWN, &reason));
   EXPECT_EQ(RequestOutcome::DISABLED_DUE_TO_INCOGNITO, reason);
 }
 
 TEST_F(ChromePasswordProtectionServiceTest,
-       VerifyUserPopulationForProtectedPasswordEntryPing) {
+       VerifyUserPopulationForSavedPasswordEntryPing) {
+  RequestOutcome reason;
+  service_->ConfigService(false /*incognito*/, false /*SBER*/);
+  EXPECT_TRUE(service_->IsPingingEnabled(
+      LoginReputationClientRequest::PASSWORD_REUSE_EVENT,
+      PasswordReuseEvent::SAVED_PASSWORD, &reason));
+
+  service_->ConfigService(false /*incognito*/, true /*SBER*/);
+  EXPECT_TRUE(service_->IsPingingEnabled(
+      LoginReputationClientRequest::PASSWORD_REUSE_EVENT,
+      PasswordReuseEvent::SAVED_PASSWORD, &reason));
+
+  service_->ConfigService(true /*incognito*/, false /*SBER*/);
+  EXPECT_TRUE(service_->IsPingingEnabled(
+      LoginReputationClientRequest::PASSWORD_REUSE_EVENT,
+      PasswordReuseEvent::SAVED_PASSWORD, &reason));
+
+  service_->ConfigService(true /*incognito*/, true /*SBER*/);
+  EXPECT_TRUE(service_->IsPingingEnabled(
+      LoginReputationClientRequest::PASSWORD_REUSE_EVENT,
+      PasswordReuseEvent::SAVED_PASSWORD, &reason));
+}
+
+TEST_F(ChromePasswordProtectionServiceTest,
+       VerifyUserPopulationForSyncPasswordEntryPing) {
+  // If user is not signed in, no ping should be sent.
+  RequestOutcome reason;
+  service_->ConfigService(false /*incognito*/, false /*SBER*/);
+  EXPECT_FALSE(service_->IsPingingEnabled(
+      LoginReputationClientRequest::PASSWORD_REUSE_EVENT,
+      PasswordReuseEvent::SIGN_IN_PASSWORD, &reason));
+  EXPECT_EQ(RequestOutcome::USER_NOT_SIGNED_IN, reason);
+
+  service_->ConfigService(false /*incognito*/, true /*SBER*/);
+  EXPECT_FALSE(service_->IsPingingEnabled(
+      LoginReputationClientRequest::PASSWORD_REUSE_EVENT,
+      PasswordReuseEvent::SIGN_IN_PASSWORD, &reason));
+  EXPECT_EQ(RequestOutcome::USER_NOT_SIGNED_IN, reason);
+
+  service_->ConfigService(true /*incognito*/, false /*SBER*/);
+  EXPECT_FALSE(service_->IsPingingEnabled(
+      LoginReputationClientRequest::PASSWORD_REUSE_EVENT,
+      PasswordReuseEvent::SIGN_IN_PASSWORD, &reason));
+  EXPECT_EQ(RequestOutcome::USER_NOT_SIGNED_IN, reason);
+
+  service_->ConfigService(true /*incognito*/, true /*SBER*/);
+  EXPECT_FALSE(service_->IsPingingEnabled(
+      LoginReputationClientRequest::PASSWORD_REUSE_EVENT,
+      PasswordReuseEvent::SIGN_IN_PASSWORD, &reason));
+  EXPECT_EQ(RequestOutcome::USER_NOT_SIGNED_IN, reason);
+
   SigninManagerFactory::GetForProfile(profile())->SetAuthenticatedAccountInfo(
       kTestGaiaID, kTestEmail);
   SetUpSyncAccount(std::string(AccountTrackerService::kNoHostedDomainFound),
                    std::string(kTestGaiaID), std::string(kTestEmail));
 
-  // Protected password entry pinging is enabled by default.
-  RequestOutcome reason;
+  // Sync password entry pinging is enabled by default.
   service_->ConfigService(false /*incognito*/, false /*SBER*/);
   EXPECT_TRUE(service_->IsPingingEnabled(
-      LoginReputationClientRequest::PASSWORD_REUSE_EVENT, &reason));
+      LoginReputationClientRequest::PASSWORD_REUSE_EVENT,
+      PasswordReuseEvent::SIGN_IN_PASSWORD, &reason));
 
   service_->ConfigService(false /*incognito*/, true /*SBER*/);
   EXPECT_TRUE(service_->IsPingingEnabled(
-      LoginReputationClientRequest::PASSWORD_REUSE_EVENT, &reason));
+      LoginReputationClientRequest::PASSWORD_REUSE_EVENT,
+      PasswordReuseEvent::SIGN_IN_PASSWORD, &reason));
 
   service_->ConfigService(true /*incognito*/, false /*SBER*/);
   EXPECT_TRUE(service_->IsPingingEnabled(
-      LoginReputationClientRequest::PASSWORD_REUSE_EVENT, &reason));
+      LoginReputationClientRequest::PASSWORD_REUSE_EVENT,
+      PasswordReuseEvent::SIGN_IN_PASSWORD, &reason));
 
   service_->ConfigService(true /*incognito*/, true /*SBER*/);
   EXPECT_TRUE(service_->IsPingingEnabled(
-      LoginReputationClientRequest::PASSWORD_REUSE_EVENT, &reason));
+      LoginReputationClientRequest::PASSWORD_REUSE_EVENT,
+      PasswordReuseEvent::SIGN_IN_PASSWORD, &reason));
 
-  // If password protection pinging is disabled by policy,
+  // If sync password entry pinging is disabled by policy,
   // |IsPingingEnabled(..)| should return false.
   profile()->GetPrefs()->SetInteger(prefs::kPasswordProtectionWarningTrigger,
                                     PASSWORD_PROTECTION_OFF);
   service_->ConfigService(false /*incognito*/, false /*SBER*/);
   EXPECT_FALSE(service_->IsPingingEnabled(
-      LoginReputationClientRequest::PASSWORD_REUSE_EVENT, &reason));
+      LoginReputationClientRequest::PASSWORD_REUSE_EVENT,
+      PasswordReuseEvent::SIGN_IN_PASSWORD, &reason));
   EXPECT_EQ(RequestOutcome::TURNED_OFF_BY_ADMIN, reason);
 
   profile()->GetPrefs()->SetInteger(prefs::kPasswordProtectionWarningTrigger,
                                     PASSWORD_REUSE);
   EXPECT_FALSE(service_->IsPingingEnabled(
-      LoginReputationClientRequest::PASSWORD_REUSE_EVENT, &reason));
+      LoginReputationClientRequest::PASSWORD_REUSE_EVENT,
+      PasswordReuseEvent::SIGN_IN_PASSWORD, &reason));
   EXPECT_EQ(RequestOutcome::PASSWORD_ALERT_MODE, reason);
 }
 
@@ -1114,7 +1173,7 @@ TEST_F(ChromePasswordProtectionServiceTest, VerifySendsPingForAboutBlank) {
   RequestOutcome reason;
   EXPECT_TRUE(service_->CanSendPing(
       LoginReputationClientRequest::PASSWORD_REUSE_EVENT, GURL("about:blank"),
-      PasswordReuseEvent::SIGN_IN_PASSWORD, &reason));
+      PasswordReuseEvent::SAVED_PASSWORD, &reason));
 }
 
 }  // namespace safe_browsing

components/safe_browsing/password_protection/metrics_util.h

@@ -91,11 +91,13 @@ enum class RequestOutcome {
   // No request sent if the admin configures password protection to
   // warn on ALL password reuses (rather than just phishing sites).
   PASSWORD_ALERT_MODE = 18,
-  // No request sent sent if the admin turns off password protection.
+  // No request sent if the admin turns off password protection.
   TURNED_OFF_BY_ADMIN = 19,
   // No request sent because Safe Browsing is disabled.
   SAFE_BROWSING_DISABLED = 20,
-  kMaxValue = SAFE_BROWSING_DISABLED,
+  // No request sent because user is not signed-in.
+  USER_NOT_SIGNED_IN = 21,
+  kMaxValue = USER_NOT_SIGNED_IN,
 };
 
 // Enum values indicates if a password protection warning is shown or

components/safe_browsing/password_protection/mock_password_protection_service.h

@@ -41,8 +41,9 @@ class MockPasswordProtectionService : public PasswordProtectionService {
   MOCK_METHOD1(UserClickedThroughSBInterstitial, bool(content::WebContents*));
   MOCK_METHOD2(ShowInterstitial,
                void(content::WebContents*, ReusedPasswordType));
-  MOCK_METHOD2(IsPingingEnabled,
+  MOCK_METHOD3(IsPingingEnabled,
                bool(LoginReputationClientRequest::TriggerType,
+                    ReusedPasswordType,
                     RequestOutcome*));
   MOCK_METHOD3(ShowModalWarning,
                void(content::WebContents*,

components/safe_browsing/password_protection/password_protection_service.cc

@@ -399,7 +399,7 @@ bool PasswordProtectionService::CanSendPing(
     ReusedPasswordType password_type,
     RequestOutcome* reason) {
   *reason = RequestOutcome::UNKNOWN;
-  if (IsPingingEnabled(trigger_type, reason) &&
+  if (IsPingingEnabled(trigger_type, password_type, reason) &&
       !IsURLWhitelistedForPasswordEntry(main_frame_url, reason)) {
     return true;
   }

components/safe_browsing/password_protection/password_protection_service.h

@@ -267,6 +267,7 @@ class PasswordProtectionService : public history::HistoryServiceObserver {
 
   virtual bool IsPingingEnabled(
       LoginReputationClientRequest::TriggerType trigger_type,
+      ReusedPasswordType password_type,
       RequestOutcome* reason) = 0;
 
   virtual bool IsHistorySyncEnabled() = 0;

tools/metrics/histograms/enums.xml

@@ -38131,6 +38131,7 @@ Called by update_net_trust_anchors.py.-->
   <int value="18" label="Password Alert mode: no need to send ping"/>
   <int value="19" label="Turned off by enterprise admin"/>
   <int value="20" label="SafeBrowsing disabled"/>
+  <int value="21" label="User not signed in"/>
 </enum>
 
 <enum name="PasswordProtectionSyncAccountType">