Distinguish error types in LoginDatabase password encryption

There are two very distinct types of error in decrypting passwords:
item-specific errors, and service unavailability errors. When error
reporting was added in
https://chromiumcodereview.appspot.com/14354002
they were conflated, leading to issues where a single bad password
entry (e.g., from someone moving a login database file from one
machine/device to another) would cause operations that retrieve the
whole password list (sync, password list in settings, etc.) to fail.

This separates the two, and for item-specific errors simply skips
that item instead of bubbling failure to the higher level.
Service-level errors continue to be reported out, so that, e.g.,
password sync will not run while iOS keychain is unavailable due to
screen lock.

BUG=268361,273841

Review URL: https://chromiumcodereview.appspot.com/24195017

git-svn-id: svn://svn.chromium.org/chrome/trunk/src@224457 0039d316-1c4b-4281-b951-d872f2087c98

5 files changed, 77 insertions, 35 deletions

diff --git a/chrome/browser/password_manager/login_database.cc b/chrome/browser/password_manager/login_database.cc
index 929f7cd..1c0e21e 100644
--- a/chrome/browser/password_manager/login_database.cc
+++ b/chrome/browser/password_manager/login_database.cc
@@ -295,7 +295,8 @@ void LoginDatabase::ReportMetrics() {
 
 bool LoginDatabase::AddLogin(const PasswordForm& form) {
   std::string encrypted_password;
-  if (!EncryptedString(form.password_value, &encrypted_password))
+  if (EncryptedString(form.password_value, &encrypted_password) !=
+          ENCRYPTION_RESULT_SUCCESS)
     return false;
 
   // You *must* change LoginTableColumns if this query changes.
@@ -338,7 +339,8 @@ bool LoginDatabase::AddLogin(const PasswordForm& form) {
 
 bool LoginDatabase::UpdateLogin(const PasswordForm& form, int* items_changed) {
   std::string encrypted_password;
-  if (!EncryptedString(form.password_value, &encrypted_password))
+  if (EncryptedString(form.password_value, &encrypted_password) !=
+          ENCRYPTION_RESULT_SUCCESS)
     return false;
 
   sql::Statement s(db_.GetCachedStatement(SQL_FROM_HERE,
@@ -409,13 +411,16 @@ bool LoginDatabase::RemoveLoginsCreatedBetween(const base::Time delete_begin,
   return s.Run();
 }
 
-bool LoginDatabase::InitPasswordFormFromStatement(PasswordForm* form,
-                                                  sql::Statement& s) const {
+LoginDatabase::EncryptionResult LoginDatabase::InitPasswordFormFromStatement(
+    PasswordForm* form,
+    sql::Statement& s) const {
   std::string encrypted_password;
   s.ColumnBlobAsString(COLUMN_PASSWORD_VALUE, &encrypted_password);
   string16 decrypted_password;
-  if (!DecryptedString(encrypted_password, &decrypted_password))
-    return false;
+  EncryptionResult encryption_result =
+      DecryptedString(encrypted_password, &decrypted_password);
+  if (encryption_result != ENCRYPTION_RESULT_SUCCESS)
+    return encryption_result;
 
   std::string tmp = s.ColumnString(COLUMN_ORIGIN_URL);
   form->origin = GURL(tmp);
@@ -449,7 +454,7 @@ bool LoginDatabase::InitPasswordFormFromStatement(PasswordForm* form,
       s.ColumnByteLength(COLUMN_FORM_DATA));
   PickleIterator form_data_iter(form_data_pickle);
   autofill::DeserializeFormData(&form_data_iter, &form->form_data);
-  return true;
+  return ENCRYPTION_RESULT_SUCCESS;
 }
 
 bool LoginDatabase::GetLogins(const PasswordForm& form,
@@ -502,8 +507,12 @@ bool LoginDatabase::GetLogins(const PasswordForm& form,
 
   while (s.Step()) {
     scoped_ptr<PasswordForm> new_form(new PasswordForm());
-    if (!InitPasswordFormFromStatement(new_form.get(), s))
+    EncryptionResult result = InitPasswordFormFromStatement(new_form.get(), s);
+    if (result == ENCRYPTION_RESULT_SERVICE_FAILURE)
       return false;
+    if (result == ENCRYPTION_RESULT_ITEM_FAILURE)
+      continue;
+    DCHECK(result == ENCRYPTION_RESULT_SUCCESS);
     if (public_suffix_domain_matching_) {
       if (!SchemeMatches(new_form, form) ||
           !RegistryControlledDomainMatches(new_form, form) ||
@@ -548,8 +557,12 @@ bool LoginDatabase::GetLoginsCreatedBetween(
 
   while (s.Step()) {
     scoped_ptr<PasswordForm> new_form(new PasswordForm());
-    if (!InitPasswordFormFromStatement(new_form.get(), s))
+    EncryptionResult result = InitPasswordFormFromStatement(new_form.get(), s);
+    if (result == ENCRYPTION_RESULT_SERVICE_FAILURE)
       return false;
+    if (result == ENCRYPTION_RESULT_ITEM_FAILURE)
+      continue;
+    DCHECK(result == ENCRYPTION_RESULT_SUCCESS);
     forms->push_back(new_form.release());
   }
   return s.Succeeded();
@@ -581,8 +594,12 @@ bool LoginDatabase::GetAllLoginsWithBlacklistSetting(
 
   while (s.Step()) {
     scoped_ptr<PasswordForm> new_form(new PasswordForm());
-    if (!InitPasswordFormFromStatement(new_form.get(), s))
+    EncryptionResult result = InitPasswordFormFromStatement(new_form.get(), s);
+    if (result == ENCRYPTION_RESULT_SERVICE_FAILURE)
       return false;
+    if (result == ENCRYPTION_RESULT_ITEM_FAILURE)
+      continue;
+    DCHECK(result == ENCRYPTION_RESULT_SUCCESS);
     forms->push_back(new_form.release());
   }
   return s.Succeeded();
diff --git a/chrome/browser/password_manager/login_database.h b/chrome/browser/password_manager/login_database.h
index b62a5ec..792d6c3 100644
--- a/chrome/browser/password_manager/login_database.h
+++ b/chrome/browser/password_manager/login_database.h
@@ -79,28 +79,43 @@ class LoginDatabase {
  private:
   friend class LoginDatabaseTest;
 
+  // Result values for encryption/decryption actions.
+  enum EncryptionResult {
+    // Success.
+    ENCRYPTION_RESULT_SUCCESS,
+    // Failure for a specific item (e.g., the encrypted value was manually
+    // moved from another machine, and can't be decrypted on this machine).
+    // This is presumed to be a permanent failure.
+    ENCRYPTION_RESULT_ITEM_FAILURE,
+    // A service-level failure (e.g., on a platform using a keyring, the keyring
+    // is temporarily unavailable).
+    // This is presumed to be a temporary failure.
+    ENCRYPTION_RESULT_SERVICE_FAILURE,
+  };
+
   // Encrypts plain_text, setting the value of cipher_text and returning true if
   // successful, or returning false and leaving cipher_text unchanged if
   // encryption fails (e.g., if the underlying OS encryption system is
   // temporarily unavailable).
-  bool EncryptedString(const string16& plain_text,
-                       std::string* cipher_text) const;
+  EncryptionResult EncryptedString(const string16& plain_text,
+                                   std::string* cipher_text) const;
 
   // Decrypts cipher_text, setting the value of plain_text and returning true if
   // successful, or returning false and leaving plain_text unchanged if
   // decryption fails (e.g., if the underlying OS encryption system is
   // temporarily unavailable).
-  bool DecryptedString(const std::string& cipher_text,
-                       string16* plain_text) const;
+  EncryptionResult DecryptedString(const std::string& cipher_text,
+                                   string16* plain_text) const;
 
   bool InitLoginsTable();
   bool MigrateOldVersionsAsNeeded();
 
   // Fills |form| from the values in the given statement (which is assumed to
   // be of the form used by the Get*Logins methods).
-  // Returns true if |form| was successfully filled.
-  bool InitPasswordFormFromStatement(autofill::PasswordForm* form,
-                                     sql::Statement& s) const;
+  // Returns the EncryptionResult from decrypting the password in |s|; if not
+  // ENCRYPTION_RESULT_SUCCESS, |form| is not filled.
+  EncryptionResult InitPasswordFormFromStatement(autofill::PasswordForm* form,
+                                                 sql::Statement& s) const;
 
   // Loads all logins whose blacklist setting matches |blacklisted| into
   // |forms|.
diff --git a/chrome/browser/password_manager/login_database_mac.cc b/chrome/browser/password_manager/login_database_mac.cc
index fbf8f6e..d8bb994 100644
--- a/chrome/browser/password_manager/login_database_mac.cc
+++ b/chrome/browser/password_manager/login_database_mac.cc
@@ -8,14 +8,16 @@
 // rest of the database as a suplemental storage system to complement Keychain,
 // providing storage of fields Keychain doesn't allow.
 
-bool LoginDatabase::EncryptedString(const string16& plain_text,
-                                    std::string* cipher_text) const {
+LoginDatabase::EncryptionResult LoginDatabase::EncryptedString(
+    const string16& plain_text,
+    std::string* cipher_text) const {
   *cipher_text = std::string();
-  return true;
+  return ENCRYPTION_RESULT_SUCCESS;
 }
 
-bool LoginDatabase::DecryptedString(const std::string& cipher_text,
-                                    string16* plain_text) const {
+LoginDatabase::EncryptionResult LoginDatabase::DecryptedString(
+    const std::string& cipher_text,
+    string16* plain_text) const {
   *plain_text = string16();
-  return true;
+  return ENCRYPTION_RESULT_SUCCESS;
 }
diff --git a/chrome/browser/password_manager/login_database_posix.cc b/chrome/browser/password_manager/login_database_posix.cc
index 99eb621..a093576 100644
--- a/chrome/browser/password_manager/login_database_posix.cc
+++ b/chrome/browser/password_manager/login_database_posix.cc
@@ -8,14 +8,16 @@
 
 // TODO: Actually encrypt passwords on Linux.
 
-bool LoginDatabase::EncryptedString(const string16& plain_text,
-                                    std::string* cipher_text) const {
+LoginDatabase::EncryptionResult LoginDatabase::EncryptedString(
+    const string16& plain_text,
+    std::string* cipher_text) const {
   *cipher_text = UTF16ToUTF8(plain_text);
-  return true;
+  return ENCRYPTION_RESULT_SUCCESS;
 }
 
-bool LoginDatabase::DecryptedString(const std::string& cipher_text,
-                                    string16* plain_text) const {
+LoginDatabase::EncryptionResult LoginDatabase::DecryptedString(
+    const std::string& cipher_text,
+    string16* plain_text) const {
   *plain_text = UTF8ToUTF16(cipher_text);
-  return true;
+  return ENCRYPTION_RESULT_SUCCESS;
 }
diff --git a/chrome/browser/password_manager/login_database_win.cc b/chrome/browser/password_manager/login_database_win.cc
index 9862abc..13a48be 100644
--- a/chrome/browser/password_manager/login_database_win.cc
+++ b/chrome/browser/password_manager/login_database_win.cc
@@ -6,12 +6,18 @@
 #include "chrome/browser/password_manager/login_database.h"
 #include "components/webdata/encryptor/encryptor.h"
 
-bool LoginDatabase::EncryptedString(const string16& plain_text,
-                                    std::string* cipher_text) const {
-  return Encryptor::EncryptString16(plain_text, cipher_text);
+LoginDatabase::EncryptionResult LoginDatabase::EncryptedString(
+    const string16& plain_text,
+    std::string* cipher_text) const {
+  if (Encryptor::EncryptString16(plain_text, cipher_text))
+    return ENCRYPTION_RESULT_SUCCESS;
+  return ENCRYPTION_RESULT_ITEM_FAILURE;
 }
 
-bool LoginDatabase::DecryptedString(const std::string& cipher_text,
-                                    string16* plain_text) const {
-  return Encryptor::DecryptString16(cipher_text, plain_text);
+LoginDatabase::EncryptionResult LoginDatabase::DecryptedString(
+    const std::string& cipher_text,
+    string16* plain_text) const {
+  if (Encryptor::DecryptString16(cipher_text, plain_text))
+    return ENCRYPTION_RESULT_SUCCESS;
+  return ENCRYPTION_RESULT_ITEM_FAILURE;
 }