[Passwords] Fix for recycling primary keys in login database

Primary keys of the login database are used as storage key for
Password USS Sync.
Currently, they are recycled and hence are causing Sync to commit
entities with wrong client tags.

This CL fixes that by adding the AUTOINCREMENT flag to the primary key
column.

Since the SQLTableBuilder is currenly only used for creating
PasswordManager databases, and for simplicity, this CL is restricting
the primary key to be a single INTEGER column.

Bug: 1016718,902349
Change-Id: I931826547b92566f631b4ca12b53039be3ceb491
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1872099
Commit-Queue: Mohamed Amir Yosef <mamir@chromium.org>
Reviewed-by: Mikel Astiz <mastiz@chromium.org>
Reviewed-by: Vasilii Sukhanov <vasilii@chromium.org>
Cr-Commit-Position: refs/heads/master@{#708273}

chrome/browser/sync/test/integration/passwords_helper.cc

@@ -164,6 +164,18 @@ void UpdateLogin(PasswordStore* store, const PasswordForm& form) {
   wait_event.Wait();
 }
 
+void UpdateLoginWithPrimaryKey(PasswordStore* store,
+                               const PasswordForm& new_form,
+                               const PasswordForm& old_form) {
+  ASSERT_TRUE(store);
+  base::WaitableEvent wait_event(
+      base::WaitableEvent::ResetPolicy::MANUAL,
+      base::WaitableEvent::InitialState::NOT_SIGNALED);
+  store->UpdateLoginWithPrimaryKey(new_form, old_form);
+  store->ScheduleTask(base::BindOnce(&PasswordStoreCallback, &wait_event));
+  wait_event.Wait();
+}
+
 std::vector<std::unique_ptr<PasswordForm>> GetLogins(PasswordStore* store) {
   EXPECT_TRUE(store);
   password_manager::PasswordStore::FormDigest matcher_form = {

chrome/browser/sync/test/integration/passwords_helper.h

@@ -36,6 +36,12 @@ void AddLogin(password_manager::PasswordStore* store,
 void UpdateLogin(password_manager::PasswordStore* store,
                  const autofill::PasswordForm& form);
 
+// Removes |old_form| from password store |store| and immediately adds
+// |new_form|. This method blocks until the operation is complete.
+void UpdateLoginWithPrimaryKey(password_manager::PasswordStore* store,
+                               const autofill::PasswordForm& new_form,
+                               const autofill::PasswordForm& old_form);
+
 // Returns all logins from |store| matching a fake signon realm (see
 // CreateTestPasswordForm()).
 // TODO(treib): Rename this to make clear how specific it is.

chrome/browser/sync/test/integration/two_client_passwords_sync_test.cc

@@ -35,6 +35,7 @@ using passwords_helper::GetVerifierPasswordStore;
 using passwords_helper::RemoveLogin;
 using passwords_helper::RemoveLogins;
 using passwords_helper::UpdateLogin;
+using passwords_helper::UpdateLoginWithPrimaryKey;
 
 using autofill::PasswordForm;
 
@@ -335,6 +336,25 @@ IN_PROC_BROWSER_TEST_P(TwoClientPasswordsSyncTest, E2E_ONLY(TwoClientAddPass)) {
   }
 }
 
+IN_PROC_BROWSER_TEST_P(TwoClientPasswordsSyncTest, AddImmediatelyAfterDelete) {
+  ASSERT_TRUE(SetupSync()) << "SetupSync() failed.";
+  ASSERT_TRUE(AllProfilesContainSamePasswordFormsAsVerifier());
+
+  PasswordForm form0 = CreateTestPasswordForm(0);
+  AddLogin(GetVerifierPasswordStore(), form0);
+  AddLogin(GetPasswordStore(0), form0);
+
+  ASSERT_TRUE(SamePasswordFormsAsVerifierChecker(1).Wait());
+  ASSERT_TRUE(AllProfilesContainSamePasswordFormsAsVerifier());
+
+  PasswordForm form1 = CreateTestPasswordForm(1);
+  UpdateLoginWithPrimaryKey(GetVerifierPasswordStore(), form1, form0);
+  UpdateLoginWithPrimaryKey(GetPasswordStore(0), form1, form0);
+
+  ASSERT_TRUE(SamePasswordFormsAsVerifierChecker(1).Wait());
+  ASSERT_TRUE(AllProfilesContainSamePasswordFormsAsVerifier());
+}
+
 INSTANTIATE_TEST_SUITE_P(USS,
                          TwoClientPasswordsSyncTest,
                          ::testing::Values(false, true));

components/password_manager/core/browser/BUILD.gn

@@ -470,6 +470,7 @@ bundle_data("unit_tests_bundle_data") {
     "//components/test/data/password_manager/login_db_v23.sql",
     "//components/test/data/password_manager/login_db_v24.sql",
     "//components/test/data/password_manager/login_db_v25.sql",
+    "//components/test/data/password_manager/login_db_v26.sql",
     "//components/test/data/password_manager/login_db_v2_broken.sql",
     "//components/test/data/password_manager/login_db_v3.sql",
     "//components/test/data/password_manager/login_db_v3_broken.sql",

components/password_manager/core/browser/android_affiliation/affiliation_database.cc

@@ -260,7 +260,7 @@ void AffiliationDatabase::InitializeTableBuilders(
     SQLTableBuilder* eq_classes_builder,
     SQLTableBuilder* eq_class_members_builder) {
   // Version 1 of the affiliation database.
-  eq_classes_builder->AddColumnToPrimaryKey("id", "INTEGER");
+  eq_classes_builder->AddPrimaryKeyColumn("id");
   eq_classes_builder->AddColumn("last_update_time", "INTEGER");
   // The first call to |SealVersion| sets the version to 0, that's why it is
   // repeated.
@@ -268,7 +268,7 @@ void AffiliationDatabase::InitializeTableBuilders(
   unsigned eq_classes_version = eq_classes_builder->SealVersion();
   DCHECK_EQ(1u, eq_classes_version);
 
-  eq_class_members_builder->AddColumnToPrimaryKey("id", "INTEGER");
+  eq_class_members_builder->AddPrimaryKeyColumn("id");
   eq_class_members_builder->AddColumnToUniqueKey("facet_uri",
                                                  "LONGVARCHAR NOT NULL");
   eq_class_members_builder->AddColumn(

components/password_manager/core/browser/login_database.cc

@@ -55,7 +55,7 @@ using autofill::PasswordForm;
 namespace password_manager {
 
 // The current version number of the login database schema.
-const int kCurrentVersionNumber = 25;
+const int kCurrentVersionNumber = 26;
 // The oldest version of the schema such that a legacy Chrome client using that
 // version can still read/write the current database.
 const int kCompatibleVersionNumber = 19;
@@ -491,14 +491,13 @@ void InitializeBuilders(SQLTableBuilders builders) {
   SealVersion(builders, /*expected_version=*/19u);
 
   // Version 20.
-  builders.logins->AddColumnToPrimaryKey("id", "INTEGER");
+  builders.logins->AddPrimaryKeyColumn("id");
   SealVersion(builders, /*expected_version=*/20u);
 
   // Version 21.
-  builders.sync_entities_metadata->AddColumnToPrimaryKey("storage_key",
-                                                         "INTEGER");
+  builders.sync_entities_metadata->AddPrimaryKeyColumn("storage_key");
   builders.sync_entities_metadata->AddColumn("metadata", "VARCHAR NOT NULL");
-  builders.sync_model_metadata->AddColumnToPrimaryKey("id", "INTEGER");
+  builders.sync_model_metadata->AddPrimaryKeyColumn("id");
   builders.sync_model_metadata->AddColumn("model_metadata", "VARCHAR NOT NULL");
   SealVersion(builders, /*expected_version=*/21u);
 
@@ -520,6 +519,9 @@ void InitializeBuilders(SQLTableBuilders builders) {
   builders.logins->AddColumn("date_last_used", "INTEGER NOT NULL DEFAULT 0");
   SealVersion(builders, /*expected_version=*/25u);
 
+  // Version 26 is the first version where the id is AUTOINCREMENT.
+  SealVersion(builders, /*expected_version=*/26u);
+
   DCHECK_EQ(static_cast<size_t>(COLUMN_NUM), builders.logins->NumberOfColumns())
       << "Adjust LoginDatabaseTableColumns if you change column definitions "
          "here.";
@@ -561,7 +563,7 @@ bool MigrateLogins(unsigned current_version,
 
   // Sync Metadata tables have been introduced in version 21. It is enough to
   // drop all data because Sync would populate the tables properly at startup.
-  if (current_version == 21 || current_version == 22 || current_version == 23) {
+  if (current_version >= 21 && current_version < 26) {
     if (!ClearAllSyncMetadata(db))
       return false;
   }
@@ -586,6 +588,45 @@ bool MigrateLogins(unsigned current_version,
       return false;
   }
 
+  // In version 26, the primary key of the logins table became an AUTOINCREMENT
+  // field. Since SQLite doesn't allow changing the column type, the only way is
+  // to actually create a temp table with the primary key propely set as an
+  // AUTOINCREMENT field, and move the data there. The code has been adjusted
+  // such that newly created tables have the primary key properly set as
+  // AUTOINCREMENT.
+  if (current_version < 26) {
+    // This statement creates the logins database similar to version 26 with the
+    // primary key column set to AUTOINCREMENT.
+    std::string temp_table_create_statement_version_26 =
+        "CREATE TABLE logins_temp (origin_url VARCHAR NOT NULL,action_url "
+        "VARCHAR,username_element VARCHAR,username_value "
+        "VARCHAR,password_element VARCHAR,password_value BLOB,submit_element "
+        "VARCHAR,signon_realm VARCHAR NOT NULL,preferred INTEGER NOT "
+        "NULL,date_created INTEGER NOT NULL,blacklisted_by_user INTEGER NOT "
+        "NULL,scheme INTEGER NOT NULL,password_type INTEGER,times_used "
+        "INTEGER,form_data BLOB,date_synced INTEGER,display_name "
+        "VARCHAR,icon_url VARCHAR,federation_url VARCHAR,skip_zero_click "
+        "INTEGER,generation_upload_status INTEGER,possible_username_pairs "
+        "BLOB,id INTEGER PRIMARY KEY AUTOINCREMENT,date_last_used "
+        "INTEGER,UNIQUE (origin_url, username_element, username_value, "
+        "password_element, signon_realm))";
+    std::string move_data_statement =
+        "INSERT INTO logins_temp SELECT * from logins";
+    std::string drop_table_statement = "DROP TABLE logins";
+    std::string rename_table_statement =
+        "ALTER TABLE logins_temp RENAME TO logins";
+
+    sql::Transaction transaction(db);
+    if (!(transaction.Begin() &&
+          db->Execute(temp_table_create_statement_version_26.c_str()) &&
+          db->Execute(move_data_statement.c_str()) &&
+          db->Execute(drop_table_statement.c_str()) &&
+          db->Execute(rename_table_statement.c_str()) &&
+          transaction.Commit())) {
+      return false;
+    }
+  }
+
   return true;
 }
 

components/password_manager/core/browser/login_database_unittest.cc

@@ -452,6 +452,27 @@ TEST_F(LoginDatabaseTest, RemoveLoginsByPrimaryKey) {
   EXPECT_EQ(0U, result.size());
 }
 
+TEST_F(LoginDatabaseTest, ShouldNotRecyclePrimaryKeys) {
+  std::vector<std::unique_ptr<PasswordForm>> result;
+
+  // Example password form.
+  PasswordForm form;
+  GenerateExamplePasswordForm(&form);
+
+  // Add the form.
+  PasswordStoreChangeList change_list = db().AddLogin(form);
+  ASSERT_EQ(1U, change_list.size());
+  int primary_key1 = change_list[0].primary_key();
+  change_list.clear();
+  // Delete the form
+  EXPECT_TRUE(db().RemoveLoginByPrimaryKey(primary_key1, &change_list));
+  ASSERT_EQ(1U, change_list.size());
+  // Add it again.
+  change_list = db().AddLogin(form);
+  ASSERT_EQ(1U, change_list.size());
+  EXPECT_NE(primary_key1, change_list[0].primary_key());
+}
+
 TEST_F(LoginDatabaseTest, TestPublicSuffixDomainMatching) {
   std::vector<std::unique_ptr<PasswordForm>> result;
 

components/password_manager/core/browser/sql_table_builder.cc

@@ -35,8 +35,8 @@ unsigned constexpr SQLTableBuilder::kInvalidVersion;
 struct SQLTableBuilder::Column {
   std::string name;
   std::string type;
-  // Whether this column is part of the table's PRIMARY KEY constraint.
-  bool part_of_primary_key;
+  // Whether this column is the table's PRIMARY KEY.
+  bool is_primary_key;
   // Whether this column is part of the table's UNIQUE constraint.
   bool part_of_unique_key;
   // The first version this column is part of.
@@ -76,10 +76,12 @@ void SQLTableBuilder::AddColumn(std::string name, std::string type) {
                       sealed_version_ + 1, kInvalidVersion, false});
 }
 
-void SQLTableBuilder::AddColumnToPrimaryKey(std::string name,
-                                            std::string type) {
-  AddColumn(std::move(name), std::move(type));
-  columns_.back().part_of_primary_key = true;
+void SQLTableBuilder::AddPrimaryKeyColumn(std::string name) {
+  for (const Column& column : columns_) {
+    DCHECK(!column.is_primary_key);
+  }
+  AddColumn(std::move(name), "INTEGER");
+  columns_.back().is_primary_key = true;
 }
 
 void SQLTableBuilder::AddColumnToUniqueKey(std::string name, std::string type) {
@@ -108,7 +110,7 @@ void SQLTableBuilder::RenameColumn(const std::string& old_name,
     // just replaced, it needs to be kept for generating the migration code.
     Column new_column = {new_name,
                          old_column->type,
-                         old_column->part_of_primary_key,
+                         old_column->is_primary_key,
                          old_column->part_of_unique_key,
                          sealed_version_ + 1,
                          kInvalidVersion,
@@ -173,7 +175,6 @@ void SQLTableBuilder::AddIndex(std::string name,
 }
 
 std::string SQLTableBuilder::ComputeConstraints(unsigned version) const {
-  std::string primary_key;
   std::string unique_key;
   for (const Column& column : columns_) {
     // Ignore dropped columns.
@@ -182,14 +183,10 @@ std::string SQLTableBuilder::ComputeConstraints(unsigned version) const {
     // Ignore columns columns from future versions.
     if (column.min_version > version)
       continue;
-    if (column.part_of_primary_key)
-      Append(column.name, &primary_key);
     if (column.part_of_unique_key)
       Append(column.name, &unique_key);
   }
   std::string constraints;
-  if (!primary_key.empty())
-    Append("PRIMARY KEY (" + primary_key + ")", &constraints);
   if (!unique_key.empty())
     Append("UNIQUE (" + unique_key + ")", &constraints);
   return constraints;
@@ -213,13 +210,21 @@ bool SQLTableBuilder::CreateTable(sql::Database* db) const {
   DCHECK(IsVersionLastAndSealed(sealed_version_));
   if (db->DoesTableExist(table_name_.c_str()))
     return true;
+
   std::string constraints = ComputeConstraints(sealed_version_);
-  DCHECK(!constraints.empty());
+  DCHECK(!constraints.empty() || std::any_of(columns_.begin(), columns_.end(),
+                                             [](const Column& column) {
+                                               return column.is_primary_key;
+                                             }));
 
   std::string names;  // Names and types of the current columns.
   for (const Column& column : columns_) {
-    if (IsColumnInLastVersion(column))
-      Append(column.name + " " + column.type, &names);
+    if (IsColumnInLastVersion(column)) {
+      std::string suffix;
+      if (column.is_primary_key)
+        suffix = " PRIMARY KEY AUTOINCREMENT";
+      Append(column.name + " " + column.type + suffix, &names);
+    }
   }
 
   std::vector<std::string>
@@ -231,12 +236,16 @@ bool SQLTableBuilder::CreateTable(sql::Database* db) const {
           base::JoinString(index.columns, ", ").c_str()));
     }
   }
+
+  std::string create_table_statement =
+      constraints.empty()
+          ? base::StringPrintf("CREATE TABLE %s (%s)", table_name_.c_str(),
+                               names.c_str())
+          : base::StringPrintf("CREATE TABLE %s (%s, %s)", table_name_.c_str(),
+                               names.c_str(), constraints.c_str());
+
   sql::Transaction transaction(db);
-  return transaction.Begin() &&
-         db->Execute(base::StringPrintf("CREATE TABLE %s (%s, %s)",
-                                        table_name_.c_str(), names.c_str(),
-                                        constraints.c_str())
-                         .c_str()) &&
+  return transaction.Begin() && db->Execute(create_table_statement.c_str()) &&
          std::all_of(create_index_sqls.begin(), create_index_sqls.end(),
                      [&db](const std::string& sql) {
                        return db->Execute(sql.c_str());
@@ -259,7 +268,7 @@ std::string SQLTableBuilder::ListAllNonuniqueKeyNames() const {
   std::string result;
   for (const Column& column : columns_) {
     if (IsColumnInLastVersion(column) &&
-        !(column.part_of_primary_key || column.part_of_unique_key))
+        !(column.is_primary_key || column.part_of_unique_key))
       Append(column.name + "=?", &result);
   }
   return result;
@@ -283,10 +292,11 @@ std::vector<base::StringPiece> SQLTableBuilder::AllPrimaryKeyNames() const {
   std::vector<base::StringPiece> result;
   result.reserve(columns_.size());
   for (const Column& column : columns_) {
-    if (IsColumnInLastVersion(column) && column.part_of_primary_key) {
+    if (IsColumnInLastVersion(column) && column.is_primary_key) {
       result.emplace_back(column.name);
     }
   }
+  DCHECK(result.size() < 2);
   return result;
 }
 
@@ -318,6 +328,8 @@ bool SQLTableBuilder::MigrateToNextFrom(unsigned old_version,
   // because that is not supported by a single SQLite command.
   bool needs_temp_table = false;
 
+  bool has_primary_key = false;
+
   for (auto column = columns_.begin(); column != columns_.end(); ++column) {
     if (column->max_version == old_version) {
       // This column was deleted after |old_version|. It can have two reasons:
@@ -337,15 +349,27 @@ bool SQLTableBuilder::MigrateToNextFrom(unsigned old_version,
       }
     } else if (column->min_version == old_version + 1) {
       // This column was added after old_version.
-      if (column->part_of_primary_key || column->part_of_unique_key)
+      if (column->is_primary_key || column->part_of_unique_key)
         needs_temp_table = true;
-      names_of_new_columns_list.push_back(column->name + " " + column->type);
+      std::string suffix;
+      if (column->is_primary_key) {
+        suffix = " PRIMARY KEY AUTOINCREMENT";
+        has_primary_key = true;
+      }
+      names_of_new_columns_list.push_back(column->name + " " + column->type +
+                                          suffix);
     } else if (column->min_version <= old_version &&
                (column->max_version == kInvalidVersion ||
                 column->max_version > old_version)) {
+      std::string suffix;
+      if (column->is_primary_key) {
+        suffix = " PRIMARY KEY AUTOINCREMENT";
+        has_primary_key = true;
+      }
       // This column stays.
       Append(column->name, &old_names_of_existing_columns_without_types);
-      Append(column->name + " " + column->type, &new_names_of_existing_columns);
+      Append(column->name + " " + column->type + suffix,
+             &new_names_of_existing_columns);
       Append(column->name, &new_names_of_existing_columns_without_types);
     }
   }
@@ -365,7 +389,7 @@ bool SQLTableBuilder::MigrateToNextFrom(unsigned old_version,
     // one.
 
     std::string constraints = ComputeConstraints(old_version + 1);
-    DCHECK(!constraints.empty());
+    DCHECK(has_primary_key || !constraints.empty());
 
     // Foreign key constraints are not enabled for the login database, so no
     // PRAGMA foreign_keys=off needed.
@@ -376,12 +400,17 @@ bool SQLTableBuilder::MigrateToNextFrom(unsigned old_version,
       Append(new_column, &names_of_all_columns);
     }
 
+    std::string create_table_statement =
+        constraints.empty()
+            ? base::StringPrintf("CREATE TABLE %s (%s)",
+                                 temp_table_name.c_str(),
+                                 names_of_all_columns.c_str())
+            : base::StringPrintf(
+                  "CREATE TABLE %s (%s, %s)", temp_table_name.c_str(),
+                  names_of_all_columns.c_str(), constraints.c_str());
+
     sql::Transaction transaction(db);
-    if (!(transaction.Begin() &&
-          db->Execute(base::StringPrintf(
-                          "CREATE TABLE %s (%s, %s)", temp_table_name.c_str(),
-                          names_of_all_columns.c_str(), constraints.c_str())
-                          .c_str()) &&
+    if (!(transaction.Begin() && db->Execute(create_table_statement.c_str()) &&
           db->Execute(base::StringPrintf(
                           "INSERT OR REPLACE INTO %s (%s) SELECT %s FROM %s",
                           temp_table_name.c_str(),

components/password_manager/core/browser/sql_table_builder.h

@@ -56,7 +56,9 @@ class SQLTableBuilder {
   void AddColumn(std::string name, std::string type);
 
   // As AddColumn but also adds column |name| to the primary key of the table.
-  void AddColumnToPrimaryKey(std::string name, std::string type);
+  // The column must be of type "INTEGER" and will be AUTO INCREMENT. This
+  // method can be called only once.
+  void AddPrimaryKeyColumn(std::string name);
 
   // As AddColumn but also adds column |name| to the unique key of the table.
   void AddColumnToUniqueKey(std::string name, std::string type);

components/password_manager/core/browser/sql_table_builder_unittest.cc

@@ -229,7 +229,7 @@ TEST_F(SQLTableBuilderTest, MigrateFrom) {
 }
 
 TEST_F(SQLTableBuilderTest, MigrateFrom_RenameAndAddColumns) {
-  builder()->AddColumnToPrimaryKey("id", "INTEGER");
+  builder()->AddPrimaryKeyColumn("id");
   builder()->AddColumn("old_name", "INTEGER");
   EXPECT_EQ(0u, builder()->SealVersion());
 
@@ -258,8 +258,7 @@ TEST_F(SQLTableBuilderTest, MigrateFrom_RenameAndAddColumns) {
 }
 
 TEST_F(SQLTableBuilderTest, MigrateFrom_RenameAndAddAndDropColumns) {
-  builder()->AddColumnToPrimaryKey("pk_1", "VARCHAR NOT NULL");
-  builder()->AddColumnToPrimaryKey("pk_2", "VARCHAR NOT NULL");
+  builder()->AddPrimaryKeyColumn("pk_1");
   builder()->AddColumnToUniqueKey("uni", "VARCHAR NOT NULL");
   builder()->AddColumn("old_name", "INTEGER");
   EXPECT_EQ(0u, builder()->SealVersion());
@@ -279,18 +278,16 @@ TEST_F(SQLTableBuilderTest, MigrateFrom_RenameAndAddAndDropColumns) {
   EXPECT_FALSE(db()->DoesColumnExist("my_logins_table", "old_name"));
   EXPECT_FALSE(db()->DoesColumnExist("my_logins_table", "added"));
   EXPECT_TRUE(db()->DoesColumnExist("my_logins_table", "pk_1"));
-  EXPECT_TRUE(db()->DoesColumnExist("my_logins_table", "pk_2"));
   EXPECT_TRUE(db()->DoesColumnExist("my_logins_table", "uni"));
   EXPECT_TRUE(db()->DoesColumnExist("my_logins_table", "new_name"));
   EXPECT_TRUE(IsColumnOfType("new_name", "INTEGER"));
-  EXPECT_EQ(5u, builder()->NumberOfColumns());
-  EXPECT_EQ("signon_realm, pk_1, pk_2, uni, new_name",
+  EXPECT_EQ(4u, builder()->NumberOfColumns());
+  EXPECT_EQ("signon_realm, pk_1, uni, new_name",
             builder()->ListAllColumnNames());
   EXPECT_EQ("new_name=?", builder()->ListAllNonuniqueKeyNames());
   EXPECT_EQ("signon_realm=? AND uni=?", builder()->ListAllUniqueKeyNames());
 
-  EXPECT_THAT(builder()->AllPrimaryKeyNames(),
-              UnorderedElementsAre("pk_1", "pk_2"));
+  EXPECT_THAT(builder()->AllPrimaryKeyNames(), UnorderedElementsAre("pk_1"));
 }
 
 TEST_F(SQLTableBuilderTest, MigrateFrom_AddPrimaryKey) {
@@ -298,7 +295,7 @@ TEST_F(SQLTableBuilderTest, MigrateFrom_AddPrimaryKey) {
   EXPECT_EQ(0u, builder()->SealVersion());
   EXPECT_TRUE(builder()->CreateTable(db()));
 
-  builder()->AddColumnToPrimaryKey("pk_1", "VARCHAR NOT NULL");
+  builder()->AddPrimaryKeyColumn("pk_1");
   EXPECT_EQ(1u, builder()->SealVersion());
 
   EXPECT_FALSE(db()->DoesColumnExist("my_logins_table", "pk_1"));
@@ -308,8 +305,9 @@ TEST_F(SQLTableBuilderTest, MigrateFrom_AddPrimaryKey) {
   EXPECT_TRUE(builder()->MigrateFrom(0, db()));
 
   EXPECT_TRUE(db()->DoesColumnExist("my_logins_table", "pk_1"));
-  EXPECT_TRUE(db()->GetSchema().find("PRIMARY KEY (pk_1)") !=
-              std::string::npos);
+  EXPECT_TRUE(
+      db()->GetSchema().find("pk_1 INTEGER PRIMARY KEY AUTOINCREMENT") !=
+      std::string::npos);
 }
 
 }  // namespace password_manager

components/test/data/password_manager/login_db_v26.sql

+PRAGMA foreign_keys=OFF;
+BEGIN TRANSACTION;
+CREATE TABLE meta(key LONGVARCHAR NOT NULL UNIQUE PRIMARY KEY, value LONGVARCHAR);
+INSERT INTO "meta" VALUES('last_compatible_version','19');
+INSERT INTO "meta" VALUES('version','25');
+CREATE TABLE logins (
+origin_url VARCHAR NOT NULL,
+action_url VARCHAR,
+username_element VARCHAR,
+username_value VARCHAR,
+password_element VARCHAR,
+password_value BLOB,
+submit_element VARCHAR,
+signon_realm VARCHAR NOT NULL,
+preferred INTEGER NOT NULL,
+date_created INTEGER NOT NULL,
+blacklisted_by_user INTEGER NOT NULL,
+scheme INTEGER NOT NULL,
+password_type INTEGER,
+times_used INTEGER,
+form_data BLOB,
+date_synced INTEGER,
+display_name VARCHAR,
+icon_url VARCHAR,
+federation_url VARCHAR,
+skip_zero_click INTEGER,
+generation_upload_status INTEGER,
+possible_username_pairs BLOB,
+id INTEGER PRIMARY KEY AUTOINCREMENT,
+date_last_used INTEGER,
+UNIQUE (origin_url, username_element, username_value, password_element, signon_realm));
+INSERT INTO "logins" (origin_url,action_url,username_element,username_value,password_element,password_value,submit_element,signon_realm,preferred,date_created,blacklisted_by_user,scheme,password_type,times_used,form_data,date_synced,display_name,icon_url,federation_url,skip_zero_click,generation_upload_status,possible_username_pairs,date_last_used) VALUES(
+'https://accounts.google.com/ServiceLogin', /* origin_url */
+'https://accounts.google.com/ServiceLoginAuth', /* action_url */
+'Email', /* username_element */
+'theerikchen', /* username_value */
+'Passwd', /* password_element */
+X'', /* password_value */
+'', /* submit_element */
+'https://accounts.google.com/', /* signon_realm */
+1, /* preferred */
+13047429345000000, /* date_created */
+0, /* blacklisted_by_user */
+0, /* scheme */
+0, /* password_type */
+1, /* times_used */
+X'18000000020000000000000000000000000000000000000000000000', /* form_data */
+0, /* date_synced */
+'', /* display_name */
+'', /* icon_url */
+'', /* federation_url */
+1,  /* skip_zero_click */
+0,  /* generation_upload_status */
+X'00000000', /* possible_username_pairs */
+0 /* date_last_used */
+);
+INSERT INTO "logins" (origin_url,action_url,username_element,username_value,password_element,password_value,submit_element,signon_realm,preferred,date_created,blacklisted_by_user,scheme,password_type,times_used,form_data,date_synced,display_name,icon_url,federation_url,skip_zero_click,generation_upload_status,possible_username_pairs,date_last_used) VALUES(
+'https://accounts.google.com/ServiceLogin', /* origin_url */
+'https://accounts.google.com/ServiceLoginAuth', /* action_url */
+'Email', /* username_element */
+'theerikchen2', /* username_value */
+'Passwd', /* password_element */
+X'', /* password_value */
+'non-empty', /* submit_element */
+'https://accounts.google.com/', /* signon_realm */
+1, /* preferred */
+13047423600000000, /* date_created */
+0, /* blacklisted_by_user */
+0, /* scheme */
+0, /* password_type */
+1, /* times_used */
+X'18000000020000000000000000000000000000000000000000000000', /* form_data */
+0, /* date_synced */
+'', /* display_name */
+'https://www.google.com/icon', /* icon_url */
+'', /* federation_url */
+1,  /* skip_zero_click */
+0,  /* generation_upload_status */
+X'00000000', /* possible_username_pairs */
+0 /* date_last_used */
+);
+INSERT INTO "logins" (origin_url,action_url,username_element,username_value,password_element,password_value,submit_element,signon_realm,preferred,date_created,blacklisted_by_user,scheme,password_type,times_used,form_data,date_synced,display_name,icon_url,federation_url,skip_zero_click,generation_upload_status,possible_username_pairs,date_last_used) VALUES(
+'http://example.com', /* origin_url */
+'http://example.com/landing', /* action_url */
+'', /* username_element */
+'user', /* username_value */
+'', /* password_element */
+X'', /* password_value */
+'non-empty', /* submit_element */
+'http://example.com', /* signon_realm */
+1, /* preferred */
+13047423600000000, /* date_created */
+0, /* blacklisted_by_user */
+1, /* scheme */
+0, /* password_type */
+1, /* times_used */
+X'18000000020000000000000000000000000000000000000000000000', /* form_data */
+0, /* date_synced */
+'', /* display_name */
+'https://www.google.com/icon', /* icon_url */
+'', /* federation_url */
+1,  /* skip_zero_click */
+0,  /* generation_upload_status */
+X'00000000', /* possible_username_pairs */
+0 /* date_last_used */
+);
+CREATE INDEX logins_signon ON logins (signon_realm);
+CREATE TABLE stats (
+origin_domain VARCHAR NOT NULL,
+username_value VARCHAR,
+dismissal_count INTEGER,
+update_time INTEGER NOT NULL,
+UNIQUE(origin_domain, username_value));
+CREATE INDEX stats_origin ON stats(origin_domain);
+CREATE TABLE sync_entities_metadata (
+  storage_key INTEGER PRIMARY KEY AUTOINCREMENT,
+  metadata VARCHAR NOT NULL
+);
+CREATE TABLE sync_model_metadata (
+  id INTEGER PRIMARY KEY AUTOINCREMENT,
+  metadata VARCHAR NOT NULL
+);
+COMMIT;