Access Context Auditing: Store non-persistent cookie records

To support recording access for non-persistent cookies, the Access
Context Audit service and databases are extended to store non-
persistent cookies alongside persistent cookies. These records are
cleared on startup unless restoring the previous session.

Bug: 1101675
Change-Id: I7ee7a47530f8f0d6b05fc8890b94c605f839c2ab
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2273057Reviewed-by: Martin Šrámek <msramek@chromium.org>
Reviewed-by: Christian Dullweber <dullweber@chromium.org>
Commit-Queue: Theodore Olsauskas-Warren <sauski@google.com>
Cr-Commit-Position: refs/heads/master@{#785843}

chrome/browser/browsing_data/access_context_audit_database.cc

@@ -6,6 +6,7 @@
 
 #include "base/logging.h"
 #include "sql/database.h"
+#include "sql/meta_table.h"
 #include "sql/recovery.h"
 #include "sql/statement.h"
 #include "sql/transaction.h"
@@ -16,6 +17,7 @@ const base::FilePath::CharType kDatabaseName[] =
     FILE_PATH_LITERAL("AccessContextAudit");
 const char kCookieTableName[] = "cookies";
 const char kStorageAPITableName[] = "originStorageAPIs";
+static const int kVersionNumber = 1;
 
 // Callback that is fired upon an SQLite error, attempts to automatically
 // recover the database if it appears possible to do so.
@@ -46,6 +48,30 @@ void DatabaseErrorCallback(sql::Database* db,
     DLOG(FATAL) << db->GetErrorMessage();
 }
 
+// Returns true if a cookie table already exists in |db|, but is missing the
+// is_persistent field.
+bool CookieTableMissingIsPersistent(sql::Database* db) {
+  std::string select = "SELECT sql FROM sqlite_master WHERE name = '";
+  select.append(kCookieTableName);
+  select.append("' AND type = 'table'");
+  sql::Statement statement(db->GetUniqueStatement(select.c_str()));
+
+  // Unable to step implies cookies table does not exist.
+  if (!statement.Step())
+    return false;
+
+  std::string cookies_schema = statement.ColumnString(0);
+  return cookies_schema.find("is_persistent") == std::string::npos;
+}
+
+// Removes all cookie records in |db| with is_persistent = false.
+bool DeleteNonPersistentCookies(sql::Database* db) {
+  std::string remove = "DELETE FROM ";
+  remove.append(kCookieTableName);
+  remove.append(" WHERE is_persistent != 1");
+  return db->Execute(remove.c_str());
+}
+
 }  // namespace
 
 AccessContextAuditDatabase::AccessRecord::AccessRecord(
@@ -53,13 +79,15 @@ AccessContextAuditDatabase::AccessRecord::AccessRecord(
     const std::string& name,
     const std::string& domain,
     const std::string& path,
-    const base::Time& last_access_time)
+    const base::Time& last_access_time,
+    bool is_persistent)
     : top_frame_origin(top_frame_origin),
       type(StorageAPIType::kCookie),
       name(name),
       domain(domain),
       path(path),
-      last_access_time(last_access_time) {}
+      last_access_time(last_access_time),
+      is_persistent(is_persistent) {}
 
 AccessContextAuditDatabase::AccessRecord::AccessRecord(
     const GURL& top_frame_origin,
@@ -73,16 +101,20 @@ AccessContextAuditDatabase::AccessRecord::AccessRecord(
   DCHECK(type != StorageAPIType::kCookie);
 }
 
+AccessContextAuditDatabase::AccessRecord::~AccessRecord() = default;
+
 AccessContextAuditDatabase::AccessRecord::AccessRecord(
     const AccessRecord& other) = default;
 
-AccessContextAuditDatabase::AccessRecord::~AccessRecord() = default;
+AccessContextAuditDatabase::AccessRecord&
+AccessContextAuditDatabase::AccessRecord::operator=(const AccessRecord& other) =
+    default;
 
 AccessContextAuditDatabase::AccessContextAuditDatabase(
     const base::FilePath& path_to_database_dir)
     : db_file_path_(path_to_database_dir.Append(kDatabaseName)) {}
 
-void AccessContextAuditDatabase::Init() {
+void AccessContextAuditDatabase::Init(bool restore_non_persistent_cookies) {
   db_.set_histogram_tag("Access Context Audit");
 
   db_.set_error_callback(
@@ -90,19 +122,56 @@ void AccessContextAuditDatabase::Init() {
 
   // Cache values generated assuming ~5000 individual pieces of client storage
   // API data, each accessed in an average of 3 different contexts (complete
-  // speculation, most will be 1, some will be >50), with an average of 40bytes
-  // per audit entry.
+  // speculation, most will be 1, some will be >50), with an average of
+  // 40bytes per audit entry.
   // TODO(crbug.com/1083384): Revist these numbers.
   db_.set_page_size(4096);
   db_.set_cache_size(128);
 
   db_.set_exclusive_locking();
 
-  if (db_.Open(db_file_path_))
-    InitializeSchema();
+  if (!db_.Open(db_file_path_))
+    return;
+
+  // Scope database initialisation in a transaction.
+  sql::Transaction transaction(&db_);
+  if (!transaction.Begin())
+    return;
+
+  if (!meta_table_.Init(&db_, kVersionNumber, kVersionNumber))
+    return;
+
+  if (meta_table_.GetCompatibleVersionNumber() > kVersionNumber) {
+    LOG(ERROR) << "Access Context Audit database is too new, kVersionNumber"
+               << kVersionNumber << ", GetCompatibleVersionNumber="
+               << meta_table_.GetCompatibleVersionNumber();
+    // No error will have been caught by the SQLite error handler, manually
+    // shut the the database.
+    transaction.Rollback();
+    db_.Close();
+    return;
+  }
+
+  if (!InitializeSchema())
+    return;
+
+  if (!restore_non_persistent_cookies)
+    DeleteNonPersistentCookies(&db_);
+
+  transaction.Commit();
 }
 
 bool AccessContextAuditDatabase::InitializeSchema() {
+  if (CookieTableMissingIsPersistent(&db_)) {
+    // Simply remove the table in this case. Due to a flag misconfiguration this
+    // version of the table was pushed to all canary users for a short period.
+    // TODO(crbug.com/1102006): Remove this code before M86 branch point.
+    std::string drop_table = "DROP TABLE ";
+    drop_table.append(kCookieTableName);
+    if (!db_.Execute(drop_table.c_str()))
+      return false;
+  }
+
   std::string create_table;
   create_table.append("CREATE TABLE IF NOT EXISTS ");
   create_table.append(kCookieTableName);
@@ -112,6 +181,7 @@ bool AccessContextAuditDatabase::InitializeSchema() {
       "domain TEXT NOT NULL,"
       "path TEXT NOT NULL,"
       "access_utc INTEGER NOT NULL,"
+      "is_persistent INTEGER NOT NULL,"
       "PRIMARY KEY (top_frame_origin, name, domain, path))");
 
   if (!db_.Execute(create_table.c_str()))
@@ -142,8 +212,8 @@ void AccessContextAuditDatabase::AddRecords(
   insert.append("INSERT OR REPLACE INTO ");
   insert.append(kCookieTableName);
   insert.append(
-      "(top_frame_origin, name, domain, path, access_utc) "
-      "VALUES (?, ?, ?, ?, ?)");
+      "(top_frame_origin, name, domain, path, access_utc, is_persistent) "
+      "VALUES (?, ?, ?, ?, ?, ?)");
   sql::Statement insert_cookie(
       db_.GetCachedStatement(SQL_FROM_HERE, insert.c_str()));
 
@@ -165,6 +235,7 @@ void AccessContextAuditDatabase::AddRecords(
       insert_cookie.BindInt64(
           4,
           record.last_access_time.ToDeltaSinceWindowsEpoch().InMicroseconds());
+      insert_cookie.BindBool(5, record.is_persistent);
 
       if (!insert_cookie.Run())
         return;
@@ -324,7 +395,8 @@ AccessContextAuditDatabase::GetAllRecords() {
 
   std::string select;
   select.append(
-      "SELECT top_frame_origin, name, domain, path, access_utc FROM ");
+      "SELECT top_frame_origin, name, domain, path, access_utc, is_persistent "
+      "FROM ");
   select.append(kCookieTableName);
   sql::Statement select_cookies(
       db_.GetCachedStatement(SQL_FROM_HERE, select.c_str()));
@@ -334,7 +406,8 @@ AccessContextAuditDatabase::GetAllRecords() {
         GURL(select_cookies.ColumnString(0)), select_cookies.ColumnString(1),
         select_cookies.ColumnString(2), select_cookies.ColumnString(3),
         base::Time::FromDeltaSinceWindowsEpoch(
-            base::TimeDelta::FromMicroseconds(select_cookies.ColumnInt64(4))));
+            base::TimeDelta::FromMicroseconds(select_cookies.ColumnInt64(4))),
+        select_cookies.ColumnBool(5));
   }
 
   select.clear();

chrome/browser/browsing_data/access_context_audit_database.h

@@ -11,6 +11,7 @@
 #include "net/cookies/canonical_cookie.h"
 #include "sql/database.h"
 #include "sql/init_status.h"
+#include "sql/meta_table.h"
 #include "sql/test/test_helpers.h"
 #include "url/gurl.h"
 
@@ -41,13 +42,16 @@ class AccessContextAuditDatabase
                  const std::string& name,
                  const std::string& domain,
                  const std::string& path,
-                 const base::Time& last_access_time);
+                 const base::Time& last_access_time,
+                 bool is_persistent);
     AccessRecord(const GURL& top_frame_origin,
                  const StorageAPIType& type,
                  const GURL& origin,
                  const base::Time& last_access_time);
-    AccessRecord(const AccessRecord& other);
     ~AccessRecord();
+    AccessRecord(const AccessRecord& other);
+    AccessRecord& operator=(const AccessRecord& other);
+
     GURL top_frame_origin;
     StorageAPIType type;
 
@@ -60,13 +64,17 @@ class AccessContextAuditDatabase
     GURL origin;
 
     base::Time last_access_time;
+
+    // When |type| is kCookie, indicates the record will be cleared on startup
+    // unless the database is started with restore_non_persistent_cookies.
+    bool is_persistent;
   };
 
   explicit AccessContextAuditDatabase(
       const base::FilePath& path_to_database_dir);
 
   // Initialises internal database. Must be called prior to any other usage.
-  void Init();
+  void Init(bool restore_non_persistent_cookies);
 
   // Persists the provided list of |records| in the database.
   void AddRecords(const std::vector<AccessRecord>& records);
@@ -100,6 +108,7 @@ class AccessContextAuditDatabase
   bool InitializeSchema();
 
   sql::Database db_;
+  sql::MetaTable meta_table_;
   base::FilePath db_file_path_;
 };
 

chrome/browser/browsing_data/access_context_audit_database_unittest.cc

@@ -35,6 +35,7 @@ void ExpectAccessRecordsEqual(
     EXPECT_EQ(a.name, b.name);
     EXPECT_EQ(a.domain, b.domain);
     EXPECT_EQ(a.path, b.path);
+    EXPECT_EQ(a.is_persistent, b.is_persistent);
   } else {
     EXPECT_EQ(a.origin, b.origin);
   }
@@ -76,11 +77,11 @@ class AccessContextAuditDatabaseTest : public testing::Test {
 
   void SetUp() override { ASSERT_TRUE(temp_directory_.CreateUniqueTempDir()); }
 
-  void OpenDatabase() {
+  void OpenDatabase(bool restore_non_persistent_cookies = false) {
     database_.reset();
     database_ = base::MakeRefCounted<AccessContextAuditDatabase>(
         temp_directory_.GetPath());
-    database_->Init();
+    database_->Init(restore_non_persistent_cookies);
   }
 
   void CloseDatabase() { database_.reset(); }
@@ -109,17 +110,20 @@ class AccessContextAuditDatabaseTest : public testing::Test {
         AccessContextAuditDatabase::AccessRecord(
             GURL("https://test2.com"), "cookie1", "test.com", "/",
             base::Time::FromDeltaSinceWindowsEpoch(
-                base::TimeDelta::FromHours(3))),
+                base::TimeDelta::FromHours(3)),
+            /* is_persistent */ true),
         AccessContextAuditDatabase::AccessRecord(
             GURL("https://test2.com"), kManyContextsCookieName,
             kManyContextsCookieDomain, kManyContextsCookiePath,
             base::Time::FromDeltaSinceWindowsEpoch(
-                base::TimeDelta::FromHours(4))),
+                base::TimeDelta::FromHours(4)),
+            /* is_persistent */ true),
         AccessContextAuditDatabase::AccessRecord(
             GURL("https://test3.com"), kManyContextsCookieName,
             kManyContextsCookieDomain, kManyContextsCookiePath,
             base::Time::FromDeltaSinceWindowsEpoch(
-                base::TimeDelta::FromHours(4))),
+                base::TimeDelta::FromHours(4)),
+            /* is_persistent */ true),
         AccessContextAuditDatabase::AccessRecord(
             GURL("https://test4.com:8000"), kManyContextsStorageAPIType,
             GURL(kManyContextsStorageAPIOrigin),
@@ -135,6 +139,12 @@ class AccessContextAuditDatabaseTest : public testing::Test {
             GURL(kManyContextsStorageAPIOrigin),
             base::Time::FromDeltaSinceWindowsEpoch(
                 base::TimeDelta::FromHours(7))),
+        AccessContextAuditDatabase::AccessRecord(
+            GURL("https://test6.com"), "non-persistent-cookie",
+            "non-persistent-domain", "/",
+            base::Time::FromDeltaSinceWindowsEpoch(
+                base::TimeDelta::FromHours(8)),
+            /* is_persistent */ false),
     };
   }
 
@@ -151,18 +161,61 @@ TEST_F(AccessContextAuditDatabaseTest, DatabaseInitialization) {
   sql::Database raw_db;
   EXPECT_TRUE(raw_db.Open(db_path()));
 
-  // [cookies] and [storageapi].
-  EXPECT_EQ(2u, sql::test::CountSQLTables(&raw_db));
+  // Database is currently at version 1.
+  sql::MetaTable meta_table;
+  EXPECT_TRUE(meta_table.Init(&raw_db, 1, 1));
 
-  // [top_frame_origin, name, domain, path, access_utc]
-  EXPECT_EQ(5u, sql::test::CountTableColumns(&raw_db, "cookies"));
+  // [cookies], [storageapi] and [meta]
+  EXPECT_EQ(3u, sql::test::CountSQLTables(&raw_db));
+
+  // [top_frame_origin, name, domain, path, access_utc, is_persistent]
+  EXPECT_EQ(6u, sql::test::CountTableColumns(&raw_db, "cookies"));
 
   // [top_frame_origin, type, origin, access_utc]
   EXPECT_EQ(4u, sql::test::CountTableColumns(&raw_db, "originStorageAPIs"));
 }
 
-TEST_F(AccessContextAuditDatabaseTest, DataPersisted) {
-  // Check that data is retrievable both before and after a database reopening.
+TEST_F(AccessContextAuditDatabaseTest, IsPersistentSchemaMigration) {
+  // Check that the database opens and functions correctly when a database
+  // with a cookies table, but without an is_persistent field, is present.
+  auto test_records = GetTestRecords();
+  sql::Database raw_db;
+  EXPECT_TRUE(raw_db.Open(db_path()));
+
+  // Create a cookies table without is_persistent.
+  EXPECT_TRUE(
+      raw_db.Execute("CREATE TABLE cookies "
+                     "(top_frame_origin TEXT NOT NULL,"
+                     "name TEXT NOT NULL,"
+                     "domain TEXT NOT NULL,"
+                     "path TEXT NOT NULL,"
+                     "access_utc INTEGER NOT NULL,"
+                     "PRIMARY KEY (top_frame_origin, name, domain, path))"));
+
+  OpenDatabase();
+  database()->AddRecords(test_records);
+  ValidateDatabaseRecords(database(), test_records);
+}
+
+TEST_F(AccessContextAuditDatabaseTest, RestoreNonPersistentCookies) {
+  // Check that non-persistent records are preserved with all other records
+  // when the database is opened with the restore_non_persistent_cookies flag.
+  auto test_records = GetTestRecords();
+  OpenDatabase();
+  database()->AddRecords(test_records);
+  ValidateDatabaseRecords(database(), test_records);
+
+  CloseDatabase();
+  OpenDatabase(/* restore_non_persistent_cookies */ true);
+
+  ValidateDatabaseRecords(database(), test_records);
+  CloseDatabase();
+}
+
+TEST_F(AccessContextAuditDatabaseTest, NonPersistentCookiesRemoved) {
+  // Check that non-persistent records are removed, but all other records are
+  // retained when the database is opened without the
+  // restore_non_persistent_cookies flag.
   auto test_records = GetTestRecords();
   OpenDatabase();
   database()->AddRecords(test_records);
@@ -171,6 +224,16 @@ TEST_F(AccessContextAuditDatabaseTest, DataPersisted) {
   CloseDatabase();
   OpenDatabase();
 
+  test_records.erase(
+      std::remove_if(
+          test_records.begin(), test_records.end(),
+          [=](const AccessContextAuditDatabase::AccessRecord& record) {
+            return record.type ==
+                       AccessContextAuditDatabase::StorageAPIType::kCookie &&
+                   record.is_persistent == false;
+          }),
+      test_records.end());
+
   ValidateDatabaseRecords(database(), test_records);
   CloseDatabase();
 }
@@ -190,7 +253,8 @@ TEST_F(AccessContextAuditDatabaseTest, RecoveredOnOpen) {
   expecter.ExpectError(SQLITE_CORRUPT);
 
   // Open that database and ensure that it does not fail.
-  EXPECT_NO_FATAL_FAILURE(OpenDatabase());
+  EXPECT_NO_FATAL_FAILURE(
+      OpenDatabase(/* restore_non_persistent_cookies */ true));
 
   // Data should be recovered.
   ValidateDatabaseRecords(database(), test_records);

chrome/browser/browsing_data/access_context_audit_service.cc

@@ -34,8 +34,10 @@ bool AccessContextAuditService::Init(
 
   if (!database_task_runner_->PostTask(
           FROM_HERE,
-          base::BindOnce(&AccessContextAuditDatabase::Init, database_)))
+          base::BindOnce(&AccessContextAuditDatabase::Init, database_,
+                         profile_->ShouldRestoreOldSessionCookies()))) {
     return false;
+  }
 
   cookie_manager->AddGlobalChangeListener(
       cookie_listener_receiver_.BindNewPipeAndPassRemote());
@@ -49,14 +51,14 @@ void AccessContextAuditService::RecordCookieAccess(
   auto now = base::Time::Now();
   std::vector<AccessContextAuditDatabase::AccessRecord> access_records;
   for (const auto& cookie : accessed_cookies) {
-    // Do not record access for already expired or non-persistent cookies. This
-    // is more than an optimisation, deletion events will not be fired for them.
-    if (cookie.ExpiryDate() < now || !cookie.IsPersistent())
+    // Do not record accesses to already expired cookies. This service is
+    // informed of deletion via OnCookieChange.
+    if (cookie.ExpiryDate() < now && cookie.IsPersistent())
       continue;
 
     access_records.emplace_back(top_frame_origin, cookie.Name(),
                                 cookie.Domain(), cookie.Path(),
-                                cookie.LastAccessDate());
+                                cookie.LastAccessDate(), cookie.IsPersistent());
   }
   database_task_runner_->PostTask(
       FROM_HERE, base::BindOnce(&AccessContextAuditDatabase::AddRecords,

chrome/browser/browsing_data/access_context_audit_service_factory.cc

@@ -38,6 +38,11 @@ KeyedService* AccessContextAuditServiceFactory::BuildServiceInstanceFor(
           features::kClientStorageAccessContextAuditing))
     return nullptr;
 
+  // The service implementation will persist session cookies until next startup.
+  // It is only used with regular profiles, which always persist session
+  // cookies.
+  DCHECK(static_cast<Profile*>(context)->ShouldPersistSessionCookies());
+
   std::unique_ptr<AccessContextAuditService> context_audit_service(
       new AccessContextAuditService(static_cast<Profile*>(context)));
   if (!context_audit_service->Init(