[Password Manager] [OSCrypt] Enable deleting corrupted passwords on mac

This CL includes:
- Enable feature for deleting corrupted passwords
- Enable feature for preventing overwriting the encryption key
- Disable feature that recovers passwords for Sync users (covered
    with the new feature)
- Update tests

Bug: 791541
Change-Id: I1726515dbd74ad2fa64ef1abf419e88c65d48bf0
Reviewed-on: https://chromium-review.googlesource.com/1240300
Commit-Queue: Tonko Sabolčec <tsabolcec@google.com>
Reviewed-by: Christos Froussios <cfroussios@chromium.org>
Reviewed-by: Vasilii Sukhanov <vasilii@chromium.org>
Reviewed-by: Dominic Battré <battre@chromium.org>
Cr-Commit-Position: refs/heads/master@{#595168}

components/os_crypt/os_crypt_features_mac.cc

@@ -11,7 +11,7 @@ namespace features {
 // unavailable, but there is a preference set that the key was created in the
 // past.
 const base::Feature kPreventEncryptionKeyOverwrites = {
-    "PreventEncryptionKeyOverwrites", base::FEATURE_DISABLED_BY_DEFAULT};
+    "PreventEncryptionKeyOverwrites", base::FEATURE_ENABLED_BY_DEFAULT};
 
 }  // namespace features
 }  // namespace os_crypt

components/password_manager/core/browser/login_database_unittest.cc

@@ -2111,6 +2111,12 @@ PasswordForm LoginDatabaseUndecryptableLoginsTest::AddDummyLogin(
 }
 
 TEST_F(LoginDatabaseUndecryptableLoginsTest, DeleteUndecryptableLoginsTest) {
+  // Disable feature for deleting corrupted passwords, so GetAutofillableLogins
+  // doesn't remove any passwords.
+  base::test::ScopedFeatureList scoped_feature_list;
+  scoped_feature_list.InitAndDisableFeature(
+      features::kDeleteCorruptedPasswords);
+
   auto form1 = AddDummyLogin("foo1", GURL("https://foo1.com/"), false);
   auto form2 = AddDummyLogin("foo2", GURL("https://foo2.com/"), true);
   auto form3 = AddDummyLogin("foo3", GURL("https://foo3.com/"), false);

components/password_manager/core/browser/password_syncable_service_unittest.cc

@@ -495,11 +495,8 @@ TEST_F(PasswordSyncableServiceTest, FailedReadFromPasswordStore) {
   syncer::SyncError error(FROM_HERE, syncer::SyncError::DATATYPE_ERROR,
                           "Failed to get passwords from store.",
                           syncer::PASSWORDS);
-  EXPECT_CALL(*password_store(), DeleteUndecryptableLogins())
-      .WillOnce(Return(DatabaseCleanupResult::kSuccess));
   EXPECT_CALL(*password_store(), FillAutofillableLogins(_))
-      .Times(2)
-      .WillRepeatedly(Return(false));
+      .WillOnce(Return(false));
   EXPECT_CALL(*error_factory, CreateAndUploadError(_, _))
       .WillOnce(Return(error));
   // ActOnPasswordStoreChanges() below shouldn't generate any changes for Sync.
@@ -539,11 +536,15 @@ TEST_F(PasswordSyncableServiceTest, RecoverPasswordsForSyncUsersDisabled) {
   EXPECT_TRUE(result.error().IsSet());
 }
 
-// Enable feature for deleting undecryptable logins.
+// Test that passwords are recovered for Sync users using the older feature
+// (kRecoverPasswordsForSyncUsers) when feature for recovering passwords for
+// Sync users is enabled, while feature for deleting corrupted passwords for
+// all users is disabled.
 TEST_F(PasswordSyncableServiceTest, RecoverPasswordsForSyncUsersEnabled) {
   base::test::ScopedFeatureList scoped_feature_list;
-  scoped_feature_list.InitAndEnableFeature(
-      features::kRecoverPasswordsForSyncUsers);
+  scoped_feature_list.InitWithFeatures(
+      {features::kRecoverPasswordsForSyncUsers},
+      {features::kDeleteCorruptedPasswords});
 
   EXPECT_CALL(*processor_, ProcessSyncChanges(_, IsEmpty()));
   EXPECT_CALL(*password_store(), FillAutofillableLogins(_))
@@ -560,8 +561,10 @@ TEST_F(PasswordSyncableServiceTest, RecoverPasswordsForSyncUsersEnabled) {
   EXPECT_FALSE(result.error().IsSet());
 }
 
-// Test that corrupted logins are not removed when merging data if features
-// for recovering passwords for both Sync and non-Sync users are enabled.
+// Test that passwords are not recovered using the older feature
+// (kRecoverPasswordForSyncUsers) when merging data if both features for
+// recovering passwords for Sync users and deleting passwords for all users
+// are enabled.
 TEST_F(PasswordSyncableServiceTest, PasswordRecoveryForAllUsersEnabled) {
   base::test::ScopedFeatureList scoped_feature_list;
   scoped_feature_list.InitWithFeatures({features::kRecoverPasswordsForSyncUsers,
@@ -588,8 +591,9 @@ TEST_F(PasswordSyncableServiceTest, PasswordRecoveryForAllUsersEnabled) {
 // Database cleanup fails because encryption is unavailable.
 TEST_F(PasswordSyncableServiceTest, FailedDeleteUndecryptableLogins) {
   base::test::ScopedFeatureList scoped_feature_list;
-  scoped_feature_list.InitAndEnableFeature(
-      features::kRecoverPasswordsForSyncUsers);
+  scoped_feature_list.InitWithFeatures(
+      {features::kRecoverPasswordsForSyncUsers},
+      {features::kDeleteCorruptedPasswords});
   auto error_factory = std::make_unique<syncer::SyncErrorFactoryMock>();
   syncer::SyncError error(
       FROM_HERE, syncer::SyncError::DATATYPE_ERROR,

components/password_manager/core/common/password_manager_features.cc

@@ -19,10 +19,10 @@ const base::Feature kAffiliationBasedMatching = {
 const base::Feature kAutofillHome = {"AutofillHome",
                                      base::FEATURE_ENABLED_BY_DEFAULT};
 
-// Recovers lost passwords on Mac by deleting ones that cannot be decrypted
+// Recovers lost passwords on Mac by deleting the ones that cannot be decrypted
 // with the present encryption key from the Keychain.
 const base::Feature kDeleteCorruptedPasswords = {
-    "DeleteCorruptedPasswords", base::FEATURE_DISABLED_BY_DEFAULT};
+    "DeleteCorruptedPasswords", base::FEATURE_ENABLED_BY_DEFAULT};
 
 // Use HTML based username detector.
 const base::Feature kHtmlBasedUsernameDetector = {
@@ -58,7 +58,7 @@ const base::Feature kPasswordsKeyboardAccessory = {
 // Deletes entries from local database on Mac which cannot be decrypted when
 // merging data with Sync.
 const base::Feature kRecoverPasswordsForSyncUsers = {
-    "RecoverPasswordsForSyncUsers", base::FEATURE_ENABLED_BY_DEFAULT};
+    "RecoverPasswordsForSyncUsers", base::FEATURE_DISABLED_BY_DEFAULT};
 
 // Enables the experiment for the password manager to only fill on account
 // selection, rather than autofilling on page load, with highlighting of fields.