From 24e27a4803024939f1dab36407bb6c57641fc5f4 Mon Sep 17 00:00:00 2001 From: David Van Cleve Date: Thu, 9 Apr 2020 01:46:39 +0000 Subject: [PATCH] Make TrustTokenPersister origins type-safe At a fundamental level, TrustTokenPersister requires that all of its url::Origin arguments have meaningful serializations, in order to key persistent state by their serializations. It currently enforces this by requiring that all of its operations' origin parameters be non-opaque and either HTTP or HTTPS. SuitableTrustTokenOrigin is a url::Origin wrapper encoding closely related preconditions (IsPotentiallyTrustworthy and HTTP or HTTPS) under which an origin is suitable for use in a Trust Tokens operation. This change modifies TrustTokenPersister's interface to take SuitableTrustTokenOrigins rather than url::Origins. SuitableTrustTokenOrigin's properties are somewhat stricter than what TrustTokenPersister needs to require in order to satisfy its contract (put in state keyed to an origin, get out state corresponding to the same origin), but all use to of the persister is expected to be of origins that satisfy SuitableTrustTokenOrigin's requirements in any case, so the difference between the preconditions it enforces and TrustTokenPersister's current interface is mostly academic. This change makes the interface much simpler (not to mention safer) as preconditions can now be enforced implicitly rather than through method comments and DCHECKs. R=csharrison Bug: 1042962 Change-Id: I5b8db80546b8b2a276d0bebd88d24e18e42581ab Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2134600 Commit-Queue: David Van Cleve Reviewed-by: Charlie Harrison Auto-Submit: David Van Cleve Cr-Commit-Position: refs/heads/master@{#757683} --- .../in_memory_trust_token_persister.cc | 18 +++++---- .../in_memory_trust_token_persister.h | 25 ++++++------ .../sqlite_trust_token_persister.cc | 38 ++++++------------- .../sqlite_trust_token_persister.h | 24 +++++------- .../sqlite_trust_token_persister_unittest.cc | 2 +- .../trust_tokens/trust_token_persister.h | 18 ++++----- .../trust_token_persister_unittest.cc | 10 ++--- 7 files changed, 60 insertions(+), 75 deletions(-) diff --git a/services/network/trust_tokens/in_memory_trust_token_persister.cc b/services/network/trust_tokens/in_memory_trust_token_persister.cc index 10c66dce52f81a..b2a241dab490a4 100644 --- a/services/network/trust_tokens/in_memory_trust_token_persister.cc +++ b/services/network/trust_tokens/in_memory_trust_token_persister.cc @@ -10,7 +10,8 @@ InMemoryTrustTokenPersister::InMemoryTrustTokenPersister() = default; InMemoryTrustTokenPersister::~InMemoryTrustTokenPersister() = default; std::unique_ptr -InMemoryTrustTokenPersister::GetToplevelConfig(const url::Origin& toplevel) { +InMemoryTrustTokenPersister::GetToplevelConfig( + const SuitableTrustTokenOrigin& toplevel) { auto it = toplevel_configs_.find(toplevel); if (it == toplevel_configs_.end()) return nullptr; @@ -18,7 +19,8 @@ InMemoryTrustTokenPersister::GetToplevelConfig(const url::Origin& toplevel) { } std::unique_ptr -InMemoryTrustTokenPersister::GetIssuerConfig(const url::Origin& issuer) { +InMemoryTrustTokenPersister::GetIssuerConfig( + const SuitableTrustTokenOrigin& issuer) { auto it = issuer_configs_.find(issuer); if (it == issuer_configs_.end()) return nullptr; @@ -27,8 +29,8 @@ InMemoryTrustTokenPersister::GetIssuerConfig(const url::Origin& issuer) { std::unique_ptr InMemoryTrustTokenPersister::GetIssuerToplevelPairConfig( - const url::Origin& issuer, - const url::Origin& toplevel) { + const SuitableTrustTokenOrigin& issuer, + const SuitableTrustTokenOrigin& toplevel) { auto it = issuer_toplevel_pair_configs_.find(std::make_pair(issuer, toplevel)); if (it == issuer_toplevel_pair_configs_.end()) @@ -37,20 +39,20 @@ InMemoryTrustTokenPersister::GetIssuerToplevelPairConfig( } void InMemoryTrustTokenPersister::SetToplevelConfig( - const url::Origin& toplevel, + const SuitableTrustTokenOrigin& toplevel, std::unique_ptr config) { toplevel_configs_[toplevel] = std::move(config); } void InMemoryTrustTokenPersister::SetIssuerConfig( - const url::Origin& issuer, + const SuitableTrustTokenOrigin& issuer, std::unique_ptr config) { issuer_configs_[issuer] = std::move(config); } void InMemoryTrustTokenPersister::SetIssuerToplevelPairConfig( - const url::Origin& issuer, - const url::Origin& toplevel, + const SuitableTrustTokenOrigin& issuer, + const SuitableTrustTokenOrigin& toplevel, std::unique_ptr config) { issuer_toplevel_pair_configs_[std::make_pair(issuer, toplevel)] = std::move(config); diff --git a/services/network/trust_tokens/in_memory_trust_token_persister.h b/services/network/trust_tokens/in_memory_trust_token_persister.h index a279a53a928834..6e57c4865b25e6 100644 --- a/services/network/trust_tokens/in_memory_trust_token_persister.h +++ b/services/network/trust_tokens/in_memory_trust_token_persister.h @@ -11,8 +11,8 @@ #include "services/network/trust_tokens/proto/public.pb.h" #include "services/network/trust_tokens/proto/storage.pb.h" +#include "services/network/trust_tokens/suitable_trust_token_origin.h" #include "services/network/trust_tokens/trust_token_persister.h" -#include "url/origin.h" namespace network { @@ -26,29 +26,30 @@ class InMemoryTrustTokenPersister : public TrustTokenPersister { // TrustTokenPersister implementation: std::unique_ptr GetIssuerConfig( - const url::Origin& issuer) override; + const SuitableTrustTokenOrigin& issuer) override; std::unique_ptr GetToplevelConfig( - const url::Origin& toplevel) override; + const SuitableTrustTokenOrigin& toplevel) override; std::unique_ptr - GetIssuerToplevelPairConfig(const url::Origin& issuer, - const url::Origin& toplevel) override; + GetIssuerToplevelPairConfig( + const SuitableTrustTokenOrigin& issuer, + const SuitableTrustTokenOrigin& toplevel) override; - void SetIssuerConfig(const url::Origin& issuer, + void SetIssuerConfig(const SuitableTrustTokenOrigin& issuer, std::unique_ptr config) override; void SetToplevelConfig( - const url::Origin& toplevel, + const SuitableTrustTokenOrigin& toplevel, std::unique_ptr config) override; void SetIssuerToplevelPairConfig( - const url::Origin& issuer, - const url::Origin& toplevel, + const SuitableTrustTokenOrigin& issuer, + const SuitableTrustTokenOrigin& toplevel, std::unique_ptr config) override; private: - std::map> + std::map> toplevel_configs_; - std::map> + std::map> issuer_configs_; - std::map, + std::map, std::unique_ptr> issuer_toplevel_pair_configs_; }; diff --git a/services/network/trust_tokens/sqlite_trust_token_persister.cc b/services/network/trust_tokens/sqlite_trust_token_persister.cc index f22b050d2638e6..0492e4ab639e74 100644 --- a/services/network/trust_tokens/sqlite_trust_token_persister.cc +++ b/services/network/trust_tokens/sqlite_trust_token_persister.cc @@ -16,10 +16,8 @@ namespace network { namespace { -std::string ToKey(const url::Origin& issuer, const url::Origin& toplevel) { - DCHECK(issuer.GetURL().SchemeIsHTTPOrHTTPS()); - DCHECK(toplevel.GetURL().SchemeIsHTTPOrHTTPS()); - +std::string ToKey(const SuitableTrustTokenOrigin& issuer, + const SuitableTrustTokenOrigin& toplevel) { // U+0020 space is a character forbidden in schemes/hosts/ports, so it // shouldn't appear in the serialization of either origin, preventing // collisions. @@ -60,9 +58,8 @@ void SQLiteTrustTokenPersister::CreateForFilePath( } std::unique_ptr -SQLiteTrustTokenPersister::GetIssuerConfig(const url::Origin& issuer) { - DCHECK(issuer.GetURL().SchemeIsHTTPOrHTTPS()); - +SQLiteTrustTokenPersister::GetIssuerConfig( + const SuitableTrustTokenOrigin& issuer) { auto* data = database_owner_->IssuerData(); CHECK(data); @@ -72,9 +69,8 @@ SQLiteTrustTokenPersister::GetIssuerConfig(const url::Origin& issuer) { } std::unique_ptr -SQLiteTrustTokenPersister::GetToplevelConfig(const url::Origin& toplevel) { - DCHECK(toplevel.GetURL().SchemeIsHTTPOrHTTPS()); - +SQLiteTrustTokenPersister::GetToplevelConfig( + const SuitableTrustTokenOrigin& toplevel) { auto* data = database_owner_->ToplevelData(); CHECK(data); @@ -85,11 +81,8 @@ SQLiteTrustTokenPersister::GetToplevelConfig(const url::Origin& toplevel) { std::unique_ptr SQLiteTrustTokenPersister::GetIssuerToplevelPairConfig( - const url::Origin& issuer, - const url::Origin& toplevel) { - DCHECK(issuer.GetURL().SchemeIsHTTPOrHTTPS()); - DCHECK(toplevel.GetURL().SchemeIsHTTPOrHTTPS()); - + const SuitableTrustTokenOrigin& issuer, + const SuitableTrustTokenOrigin& toplevel) { auto* data = database_owner_->IssuerToplevelPairData(); CHECK(data); @@ -99,10 +92,8 @@ SQLiteTrustTokenPersister::GetIssuerToplevelPairConfig( } void SQLiteTrustTokenPersister::SetIssuerConfig( - const url::Origin& issuer, + const SuitableTrustTokenOrigin& issuer, std::unique_ptr config) { - DCHECK(issuer.GetURL().SchemeIsHTTPOrHTTPS()); - sqlite_proto::KeyValueData* data = database_owner_->IssuerData(); CHECK(data); @@ -110,10 +101,8 @@ void SQLiteTrustTokenPersister::SetIssuerConfig( } void SQLiteTrustTokenPersister::SetToplevelConfig( - const url::Origin& toplevel, + const SuitableTrustTokenOrigin& toplevel, std::unique_ptr config) { - DCHECK(toplevel.GetURL().SchemeIsHTTPOrHTTPS()); - sqlite_proto::KeyValueData* data = database_owner_->ToplevelData(); CHECK(data); @@ -121,12 +110,9 @@ void SQLiteTrustTokenPersister::SetToplevelConfig( } void SQLiteTrustTokenPersister::SetIssuerToplevelPairConfig( - const url::Origin& issuer, - const url::Origin& toplevel, + const SuitableTrustTokenOrigin& issuer, + const SuitableTrustTokenOrigin& toplevel, std::unique_ptr config) { - DCHECK(issuer.GetURL().SchemeIsHTTPOrHTTPS()); - DCHECK(toplevel.GetURL().SchemeIsHTTPOrHTTPS()); - sqlite_proto::KeyValueData* data = database_owner_->IssuerToplevelPairData(); CHECK(data); diff --git a/services/network/trust_tokens/sqlite_trust_token_persister.h b/services/network/trust_tokens/sqlite_trust_token_persister.h index 7a852eb0ab7582..542309eb418477 100644 --- a/services/network/trust_tokens/sqlite_trust_token_persister.h +++ b/services/network/trust_tokens/sqlite_trust_token_persister.h @@ -57,28 +57,24 @@ class SQLiteTrustTokenPersister : public TrustTokenPersister { // TrustTokenPersister implementation: - // Preconditions: - // - All of these methods require that ther Origin inputs' schemes must be - // HTTP or HTTPS. - // - // Postconditions: - // - Each getter returns nullptr when the requested record was not found. + // Each getter returns nullptr when the requested record was not found. std::unique_ptr GetIssuerConfig( - const url::Origin& issuer) override; + const SuitableTrustTokenOrigin& issuer) override; std::unique_ptr GetToplevelConfig( - const url::Origin& toplevel) override; + const SuitableTrustTokenOrigin& toplevel) override; std::unique_ptr - GetIssuerToplevelPairConfig(const url::Origin& issuer, - const url::Origin& toplevel) override; + GetIssuerToplevelPairConfig( + const SuitableTrustTokenOrigin& issuer, + const SuitableTrustTokenOrigin& toplevel) override; - void SetIssuerConfig(const url::Origin& issuer, + void SetIssuerConfig(const SuitableTrustTokenOrigin& issuer, std::unique_ptr config) override; void SetToplevelConfig( - const url::Origin& toplevel, + const SuitableTrustTokenOrigin& toplevel, std::unique_ptr config) override; void SetIssuerToplevelPairConfig( - const url::Origin& issuer, - const url::Origin& toplevel, + const SuitableTrustTokenOrigin& issuer, + const SuitableTrustTokenOrigin& toplevel, std::unique_ptr config) override; private: diff --git a/services/network/trust_tokens/sqlite_trust_token_persister_unittest.cc b/services/network/trust_tokens/sqlite_trust_token_persister_unittest.cc index 7a18971a057c39..a441635835441a 100644 --- a/services/network/trust_tokens/sqlite_trust_token_persister_unittest.cc +++ b/services/network/trust_tokens/sqlite_trust_token_persister_unittest.cc @@ -37,7 +37,7 @@ TEST(SQLiteTrustTokenPersister, PutReinitializeAndGet) { base::File::FLAG_CREATE_ALWAYS | base::File::FLAG_WRITE); ASSERT_TRUE(temp_file.IsValid()); - auto origin = url::Origin::Create(GURL("https://a.com/")); + auto origin = *SuitableTrustTokenOrigin::Create(GURL("https://a.com/")); { std::unique_ptr persister; SQLiteTrustTokenPersister::CreateForFilePath( diff --git a/services/network/trust_tokens/trust_token_persister.h b/services/network/trust_tokens/trust_token_persister.h index 9d9dea9561fb0c..bcc2be20b46819 100644 --- a/services/network/trust_tokens/trust_token_persister.h +++ b/services/network/trust_tokens/trust_token_persister.h @@ -7,7 +7,7 @@ #include -#include "url/origin.h" +#include "services/network/trust_tokens/suitable_trust_token_origin.h" namespace network { @@ -28,22 +28,22 @@ class TrustTokenPersister { TrustTokenPersister& operator=(const TrustTokenPersister&) = delete; virtual std::unique_ptr GetIssuerConfig( - const url::Origin& issuer) = 0; + const SuitableTrustTokenOrigin& issuer) = 0; virtual std::unique_ptr GetToplevelConfig( - const url::Origin& toplevel) = 0; + const SuitableTrustTokenOrigin& toplevel) = 0; virtual std::unique_ptr - GetIssuerToplevelPairConfig(const url::Origin& issuer, - const url::Origin& toplevel) = 0; + GetIssuerToplevelPairConfig(const SuitableTrustTokenOrigin& issuer, + const SuitableTrustTokenOrigin& toplevel) = 0; virtual void SetIssuerConfig( - const url::Origin& issuer, + const SuitableTrustTokenOrigin& issuer, std::unique_ptr config) = 0; virtual void SetToplevelConfig( - const url::Origin& toplevel, + const SuitableTrustTokenOrigin& toplevel, std::unique_ptr config) = 0; virtual void SetIssuerToplevelPairConfig( - const url::Origin& issuer, - const url::Origin& toplevel, + const SuitableTrustTokenOrigin& issuer, + const SuitableTrustTokenOrigin& toplevel, std::unique_ptr config) = 0; }; diff --git a/services/network/trust_tokens/trust_token_persister_unittest.cc b/services/network/trust_tokens/trust_token_persister_unittest.cc index b251919d57f830..5d8ac61f10d728 100644 --- a/services/network/trust_tokens/trust_token_persister_unittest.cc +++ b/services/network/trust_tokens/trust_token_persister_unittest.cc @@ -98,7 +98,7 @@ TYPED_TEST(TrustTokenPersisterTest, NegativeResults) { env.RunUntilIdle(); // Give implementations with asynchronous initialization // time to initialize. - auto origin = url::Origin::Create(GURL("https://a.com/")); + auto origin = *SuitableTrustTokenOrigin::Create(GURL("https://a.com/")); EXPECT_THAT(persister->GetIssuerConfig(origin), IsNull()); EXPECT_THAT(persister->GetToplevelConfig(origin), IsNull()); EXPECT_THAT(persister->GetIssuerToplevelPairConfig(origin, origin), IsNull()); @@ -122,7 +122,7 @@ TYPED_TEST(TrustTokenPersisterTest, StoresIssuerConfigs) { *config.add_tokens() = my_token; auto config_to_store = std::make_unique(config); - auto origin = url::Origin::Create(GURL("https://a.com/")); + auto origin = *SuitableTrustTokenOrigin::Create(GURL("https://a.com/")); persister->SetIssuerConfig(origin, std::move(config_to_store)); env.RunUntilIdle(); // Give implementations with asynchronous write @@ -149,7 +149,7 @@ TYPED_TEST(TrustTokenPersisterTest, StoresToplevelConfigs) { *config.add_associated_issuers() = "an issuer"; auto config_to_store = std::make_unique(config); - auto origin = url::Origin::Create(GURL("https://a.com/")); + auto origin = *SuitableTrustTokenOrigin::Create(GURL("https://a.com/")); persister->SetToplevelConfig(origin, std::move(config_to_store)); env.RunUntilIdle(); // Give implementations with asynchronous write // operations time to complete the operation. @@ -176,8 +176,8 @@ TYPED_TEST(TrustTokenPersisterTest, StoresIssuerToplevelPairConfigs) { auto config_to_store = std::make_unique(config); - auto toplevel = url::Origin::Create(GURL("https://a.com/")); - auto issuer = url::Origin::Create(GURL("https://issuer.com/")); + auto toplevel = *SuitableTrustTokenOrigin::Create(GURL("https://a.com/")); + auto issuer = *SuitableTrustTokenOrigin::Create(GURL("https://issuer.com/")); persister->SetIssuerToplevelPairConfig(issuer, toplevel, std::move(config_to_store)); env.RunUntilIdle(); // Give implementations with asynchronous write