Skip to content

Commit

Permalink
Attribution-scoped rate limit data should be deleted if attribution t…
Browse files Browse the repository at this point in the history 
…ime within time range

Time column in attribution-rate limit data changed from attribution
time to source time in crrev.com/c/4184678.

However, the time-ranged data deletion should be based on attribution
time as well for proper clean up.

(cherry picked from commit 5b67d4e)

Bug: 1410756
Change-Id: Iaffda79594fb2a55920420809278ae92c6471e8f
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/4197849
Reviewed-by: Andrew Paseltiner <apaseltiner@chromium.org>
Commit-Queue: Nan Lin <linnan@chromium.org>
Cr-Original-Commit-Position: refs/heads/main@{#1098168}
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/4206030
Bot-Commit: Rubber Stamper <rubber-stamper@appspot.gserviceaccount.com>
Cr-Commit-Position: refs/branch-heads/5563@{#38}
Cr-Branched-From: 3ac59a6-refs/heads/main@{#1097615}
  • Loading branch information
@linnan-github
linnan-github authored and Chromium LUCI CQ committed Jan 30, 2023
1 parent b96031b commit 4c42733
Show file tree
Hide file tree
Showing 10 changed files with 257 additions and 62 deletions.
4 changes: 2 additions & 2 deletions content/browser/attribution_reporting/attribution_storage_sql.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -68,11 +68,11 @@ namespace content {
// Version number of the database.
// TODO: remove the active_unattributed_sources_by_site_reporting_origin index
// during the next DB migration.
const int AttributionStorageSql::kCurrentVersionNumber = 42;
const int AttributionStorageSql::kCurrentVersionNumber = 43;

// Earliest version which can use a |kCurrentVersionNumber| database
// without failing.
const int AttributionStorageSql::kCompatibleVersionNumber = 42;
const int AttributionStorageSql::kCompatibleVersionNumber = 43;

// Latest version of the database that cannot be upgraded to
// |kCurrentVersionNumber| without razing the database.
Expand Down
33 changes: 33 additions & 0 deletions content/browser/attribution_reporting/attribution_storage_sql_migrations.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -583,6 +583,34 @@ bool MigrateToVersion42(sql::Database* db, sql::MetaTable* meta_table) {
return transaction.Commit();
}

bool MigrateToVersion43(sql::Database* db, sql::MetaTable* meta_table) {
// Wrap each migration in its own transaction. See comment in
// `MigrateToVersion34`.
sql::Transaction transaction(db);
if (!transaction.Begin()) {
return false;
}

static constexpr char kRenameExpiryTimeSql[] =
"ALTER TABLE rate_limits "
"RENAME COLUMN expiry_time TO source_expiry_or_attribution_time";
if (!db->Execute(kRenameExpiryTimeSql)) {
return false;
}

static_assert(static_cast<int>(RateLimitTable::Scope::kAttribution) == 1);

static constexpr char kSetAttributionTimeSql[] =
"UPDATE rate_limits "
"SET source_expiry_or_attribution_time=time WHERE scope=1";
if (!db->Execute(kSetAttributionTimeSql)) {
return false;
}

meta_table->SetVersionNumber(43);
return transaction.Commit();
}

} // namespace

bool UpgradeAttributionStorageSqlSchema(sql::Database* db,
Expand Down Expand Up @@ -640,6 +668,11 @@ bool UpgradeAttributionStorageSqlSchema(sql::Database* db,
return false;
}
}
if (meta_table->GetVersionNumber() == 42) {
if (!MigrateToVersion43(db, meta_table)) {
return false;
}
}
// Add similar if () blocks for new versions here.

if (base::ThreadTicks::IsSupported()) {
Expand Down
59 changes: 56 additions & 3 deletions content/browser/attribution_reporting/attribution_storage_sql_migrations_unittest.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -280,16 +280,16 @@ TEST_F(AttributionStorageSqlMigrationsTest, MigrateVersion34ToCurrent) {
NormalizeSchema(db.GetSchema()));

// Verify that data is preserved across the migration.
sql::Statement s(
db.GetUniqueStatement("SELECT expiry_time FROM rate_limits"));
sql::Statement s(db.GetUniqueStatement(
"SELECT source_expiry_or_attribution_time FROM rate_limits"));

ASSERT_TRUE(s.Step());
ASSERT_EQ(7, s.ColumnInt64(0)); // with matching source
ASSERT_TRUE(s.Step());
EXPECT_EQ(9 + base::Days(30).InMicroseconds(),
s.ColumnInt64(0)); // without matching source
ASSERT_TRUE(s.Step());
EXPECT_EQ(0, s.ColumnInt64(0)); // for attribution
EXPECT_EQ(9, s.ColumnInt64(0)); // for attribution
ASSERT_FALSE(s.Step());
}

Expand Down Expand Up @@ -655,4 +655,57 @@ TEST_F(AttributionStorageSqlMigrationsTest, MigrateVersion41ToCurrent) {
histograms.ExpectTotalCount("Conversions.Storage.MigrationTime", 1);
}

TEST_F(AttributionStorageSqlMigrationsTest, MigrateVersion42ToCurrent) {
base::HistogramTester histograms;
LoadDatabase(GetVersionFilePath(42), DbPath());

// Verify pre-conditions.
{
sql::Database db;
ASSERT_TRUE(db.Open(DbPath()));
ASSERT_TRUE(db.DoesColumnExist("rate_limits", "expiry_time"));
ASSERT_FALSE(
db.DoesColumnExist("rate_limits", "source_expiry_or_attribution_time"));

static constexpr char kSql[] = "SELECT expiry_time FROM rate_limits";
sql::Statement s(db.GetUniqueStatement(kSql));

ASSERT_TRUE(s.Step());
ASSERT_EQ(7, s.ColumnInt64(0));
ASSERT_TRUE(s.Step());
ASSERT_EQ(10, s.ColumnInt64(0));
ASSERT_FALSE(s.Step());
}

MigrateDatabase();

// Verify schema is current.
{
sql::Database db;
ASSERT_TRUE(db.Open(DbPath()));

// Check version.
EXPECT_EQ(AttributionStorageSql::kCurrentVersionNumber,
VersionFromDatabase(&db));

// Compare normalized schemas
EXPECT_EQ(NormalizeSchema(GetCurrentSchema()),
NormalizeSchema(db.GetSchema()));

// Verify that data is preserved across the migration.
sql::Statement s(db.GetUniqueStatement(
"SELECT source_expiry_or_attribution_time FROM rate_limits"));

ASSERT_TRUE(s.Step());
ASSERT_EQ(7, s.ColumnInt64(0)); // unchanged
ASSERT_TRUE(s.Step());
ASSERT_EQ(9, s.ColumnInt64(0)); // from time
ASSERT_FALSE(s.Step());
}

// DB creation histograms should be recorded.
histograms.ExpectTotalCount("Conversions.Storage.CreationTime", 0);
histograms.ExpectTotalCount("Conversions.Storage.MigrationTime", 1);
}

} // namespace content
6 changes: 4 additions & 2 deletions content/browser/attribution_reporting/attribution_storage_sql_query_plans_unittest.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -213,12 +213,14 @@ TEST_F(AttributionSqlQueryPlanTest, kRateLimitSelectReportingOriginsSql) {

TEST_F(AttributionSqlQueryPlanTest, kDeleteRateLimitRangeSql) {
EXPECT_THAT(GetPlan(attribution_queries::kDeleteRateLimitRangeSql),
UsesIndex("rate_limit_time_idx"));
AllOf(UsesIndex("rate_limit_time_idx"),
UsesIndex("rate_limit_reporting_origin_idx")));
}

TEST_F(AttributionSqlQueryPlanTest, kSelectRateLimitsForDeletionSql) {
EXPECT_THAT(GetPlan(attribution_queries::kSelectRateLimitsForDeletionSql),
UsesIndex("rate_limit_time_idx"));
AllOf(UsesIndex("rate_limit_time_idx"),
UsesIndex("rate_limit_reporting_origin_idx")));
}

TEST_F(AttributionSqlQueryPlanTest, kDeleteExpiredRateLimitsSql) {
Expand Down
49 changes: 25 additions & 24 deletions content/browser/attribution_reporting/rate_limit_table.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -48,11 +48,9 @@ bool RateLimitTable::CreateTable(sql::Database* db) {
// |context_origin| is the source origin for `kSource` or the destination
// origin for `kAttribution`.
// |reporting_origin| is the reporting origin of the impression/conversion.
// |time| is the time of either the source registration or the attribution
// trigger, depending on |scope|.
// |expiry_time| is only meaningful when |scope| is
// `RateLimitTable::Scope::kSource` and contains the source's expiry time,
// otherwise it is set to `base::Time()`.
// |time| is the time of the source registration.
// |source_expiry_or_attribution_time| is either the source's expiry time or
// the attribution time, depending on |scope|.
static constexpr char kRateLimitTableSql[] =
"CREATE TABLE rate_limits("
"id INTEGER PRIMARY KEY NOT NULL,"
Expand All @@ -63,7 +61,7 @@ bool RateLimitTable::CreateTable(sql::Database* db) {
"context_origin TEXT NOT NULL,"
"reporting_origin TEXT NOT NULL,"
"time INTEGER NOT NULL,"
"expiry_time INTEGER NOT NULL)";
"source_expiry_or_attribution_time INTEGER NOT NULL)";
if (!db->Execute(kRateLimitTableSql)) {
return false;
}
Expand Down Expand Up @@ -106,19 +104,19 @@ bool RateLimitTable::CreateTable(sql::Database* db) {
bool RateLimitTable::AddRateLimitForSource(sql::Database* db,
const StoredSource& source) {
DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);
return AddRateLimit(db, Scope::kSource, source);
return AddRateLimit(db, source, /*trigger_time=*/absl::nullopt);
}

bool RateLimitTable::AddRateLimitForAttribution(
sql::Database* db,
const AttributionInfo& attribution_info) {
DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);
return AddRateLimit(db, Scope::kAttribution, attribution_info.source);
return AddRateLimit(db, attribution_info.source, attribution_info.time);
}

bool RateLimitTable::AddRateLimit(sql::Database* db,
Scope scope,
const StoredSource& source) {
const StoredSource& source,
absl::optional<base::Time> trigger_time) {
const CommonSourceInfo& common_info = source.common_info();

// Only delete expired rate limits periodically to avoid excessive DB
Expand All @@ -134,23 +132,23 @@ bool RateLimitTable::AddRateLimit(sql::Database* db,
last_cleared_ = now;
}

Scope scope;
const attribution_reporting::SuitableOrigin* context_origin;
base::Time expiry_time;
switch (scope) {
case Scope::kSource:
context_origin = &common_info.source_origin();
expiry_time = common_info.expiry_time();
break;
case Scope::kAttribution:
context_origin = &common_info.destination_origin();
expiry_time = base::Time();
break;
base::Time source_expiry_or_attribution_time;
if (trigger_time.has_value()) {
scope = Scope::kAttribution;
context_origin = &common_info.destination_origin();
source_expiry_or_attribution_time = *trigger_time;
} else {
scope = Scope::kSource;
context_origin = &common_info.source_origin();
source_expiry_or_attribution_time = common_info.expiry_time();
}

static constexpr char kStoreRateLimitSql[] =
"INSERT INTO rate_limits"
"(scope,source_id,source_site,"
"destination_site,context_origin,reporting_origin,time,expiry_time)"
"(scope,source_id,source_site,destination_site,context_origin,"
"reporting_origin,time,source_expiry_or_attribution_time)"
"VALUES(?,?,?,?,?,?,?,?)";
sql::Statement statement(
db->GetCachedStatement(SQL_FROM_HERE, kStoreRateLimitSql));
Expand All @@ -161,7 +159,7 @@ bool RateLimitTable::AddRateLimit(sql::Database* db,
statement.BindString(4, context_origin->Serialize());
statement.BindString(5, common_info.reporting_origin().Serialize());
statement.BindTime(6, common_info.source_time());
statement.BindTime(7, expiry_time);
statement.BindTime(7, source_expiry_or_attribution_time);

return statement.Run();
}
Expand Down Expand Up @@ -218,7 +216,8 @@ RateLimitResult RateLimitTable::SourceAllowedForDestinationLimit(
"update `scope=0` query below");

// Check the number of unique destinations covered by all source registrations
// whose [source_time, expiry_time] intersect with the current source_time.
// whose [source_time, source_expiry_or_attribution_time] intersect with the
// current source_time.
sql::Statement statement(db->GetCachedStatement(
SQL_FROM_HERE, attribution_queries::kRateLimitSourceAllowedSql));

Expand Down Expand Up @@ -324,6 +323,7 @@ bool RateLimitTable::ClearAllDataInRange(sql::Database* db,
DCHECK(!((delete_begin.is_null() || delete_begin.is_min()) &&
delete_end.is_max()));

// TODO(linnan): Optimize using a more appropriate index.
sql::Statement statement(db->GetCachedStatement(
SQL_FROM_HERE, attribution_queries::kDeleteRateLimitRangeSql));
statement.BindTime(0, delete_begin);
Expand Down Expand Up @@ -359,6 +359,7 @@ bool RateLimitTable::ClearDataForOriginsInRange(
return false;
}

// TODO(linnan): Optimize using a more appropriate index.
sql::Statement select_statement(db->GetCachedStatement(
SQL_FROM_HERE, attribution_queries::kSelectRateLimitsForDeletionSql));
select_statement.BindTime(0, delete_begin);
Expand Down
5 changes: 3 additions & 2 deletions content/browser/attribution_reporting/rate_limit_table.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,7 @@
#include "content/browser/attribution_reporting/stored_source.h"
#include "content/common/content_export.h"
#include "content/public/browser/storage_partition.h"
#include "third_party/abseil-cpp/absl/types/optional.h"

namespace sql {
class Database;
Expand Down Expand Up @@ -98,8 +99,8 @@ class CONTENT_EXPORT RateLimitTable {

private:
[[nodiscard]] bool AddRateLimit(sql::Database* db,
Scope scope,
const StoredSource& source)
const StoredSource& source,
absl::optional<base::Time> trigger_time)
VALID_CONTEXT_REQUIRED(sequence_checker_);

[[nodiscard]] RateLimitResult AllowedForReportingOriginLimit(
Expand Down
Loading

0 comments on commit 4c42733

Please sign in to comment.