Skip to content

Commit

Permalink
Make TrustTokenPersister origins type-safe
Browse files Browse the repository at this point in the history 
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 <davidvc@chromium.org>
Reviewed-by: Charlie Harrison <csharrison@chromium.org>
Auto-Submit: David Van Cleve <davidvc@chromium.org>
Cr-Commit-Position: refs/heads/master@{#757683}
  • Loading branch information
David Van Cleve authored and Commit Bot committed Apr 9, 2020
1 parent d0b13ee commit 24e27a4
Show file tree
Hide file tree
Showing 7 changed files with 60 additions and 75 deletions.
18 changes: 10 additions & 8 deletions services/network/trust_tokens/in_memory_trust_token_persister.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -10,15 +10,17 @@ InMemoryTrustTokenPersister::InMemoryTrustTokenPersister() = default;
InMemoryTrustTokenPersister::~InMemoryTrustTokenPersister() = default;

std::unique_ptr<TrustTokenToplevelConfig>
InMemoryTrustTokenPersister::GetToplevelConfig(const url::Origin& toplevel) {
InMemoryTrustTokenPersister::GetToplevelConfig(
const SuitableTrustTokenOrigin& toplevel) {
auto it = toplevel_configs_.find(toplevel);
if (it == toplevel_configs_.end())
return nullptr;
return std::make_unique<TrustTokenToplevelConfig>(*it->second);
}

std::unique_ptr<TrustTokenIssuerConfig>
InMemoryTrustTokenPersister::GetIssuerConfig(const url::Origin& issuer) {
InMemoryTrustTokenPersister::GetIssuerConfig(
const SuitableTrustTokenOrigin& issuer) {
auto it = issuer_configs_.find(issuer);
if (it == issuer_configs_.end())
return nullptr;
Expand All @@ -27,8 +29,8 @@ InMemoryTrustTokenPersister::GetIssuerConfig(const url::Origin& issuer) {

std::unique_ptr<TrustTokenIssuerToplevelPairConfig>
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())
Expand All @@ -37,20 +39,20 @@ InMemoryTrustTokenPersister::GetIssuerToplevelPairConfig(
}

void InMemoryTrustTokenPersister::SetToplevelConfig(
const url::Origin& toplevel,
const SuitableTrustTokenOrigin& toplevel,
std::unique_ptr<TrustTokenToplevelConfig> config) {
toplevel_configs_[toplevel] = std::move(config);
}

void InMemoryTrustTokenPersister::SetIssuerConfig(
const url::Origin& issuer,
const SuitableTrustTokenOrigin& issuer,
std::unique_ptr<TrustTokenIssuerConfig> 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<TrustTokenIssuerToplevelPairConfig> config) {
issuer_toplevel_pair_configs_[std::make_pair(issuer, toplevel)] =
std::move(config);
Expand Down
25 changes: 13 additions & 12 deletions services/network/trust_tokens/in_memory_trust_token_persister.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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 {

Expand All @@ -26,29 +26,30 @@ class InMemoryTrustTokenPersister : public TrustTokenPersister {

// TrustTokenPersister implementation:
std::unique_ptr<TrustTokenIssuerConfig> GetIssuerConfig(
const url::Origin& issuer) override;
const SuitableTrustTokenOrigin& issuer) override;
std::unique_ptr<TrustTokenToplevelConfig> GetToplevelConfig(
const url::Origin& toplevel) override;
const SuitableTrustTokenOrigin& toplevel) override;
std::unique_ptr<TrustTokenIssuerToplevelPairConfig>
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<TrustTokenIssuerConfig> config) override;
void SetToplevelConfig(
const url::Origin& toplevel,
const SuitableTrustTokenOrigin& toplevel,
std::unique_ptr<TrustTokenToplevelConfig> config) override;
void SetIssuerToplevelPairConfig(
const url::Origin& issuer,
const url::Origin& toplevel,
const SuitableTrustTokenOrigin& issuer,
const SuitableTrustTokenOrigin& toplevel,
std::unique_ptr<TrustTokenIssuerToplevelPairConfig> config) override;

private:
std::map<url::Origin, std::unique_ptr<TrustTokenToplevelConfig>>
std::map<SuitableTrustTokenOrigin, std::unique_ptr<TrustTokenToplevelConfig>>
toplevel_configs_;
std::map<url::Origin, std::unique_ptr<TrustTokenIssuerConfig>>
std::map<SuitableTrustTokenOrigin, std::unique_ptr<TrustTokenIssuerConfig>>
issuer_configs_;
std::map<std::pair<url::Origin, url::Origin>,
std::map<std::pair<SuitableTrustTokenOrigin, SuitableTrustTokenOrigin>,
std::unique_ptr<TrustTokenIssuerToplevelPairConfig>>
issuer_toplevel_pair_configs_;
};
Expand Down
38 changes: 12 additions & 26 deletions services/network/trust_tokens/sqlite_trust_token_persister.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Expand Down Expand Up @@ -60,9 +58,8 @@ void SQLiteTrustTokenPersister::CreateForFilePath(
}

std::unique_ptr<TrustTokenIssuerConfig>
SQLiteTrustTokenPersister::GetIssuerConfig(const url::Origin& issuer) {
DCHECK(issuer.GetURL().SchemeIsHTTPOrHTTPS());

SQLiteTrustTokenPersister::GetIssuerConfig(
const SuitableTrustTokenOrigin& issuer) {
auto* data = database_owner_->IssuerData();
CHECK(data);

Expand All @@ -72,9 +69,8 @@ SQLiteTrustTokenPersister::GetIssuerConfig(const url::Origin& issuer) {
}

std::unique_ptr<TrustTokenToplevelConfig>
SQLiteTrustTokenPersister::GetToplevelConfig(const url::Origin& toplevel) {
DCHECK(toplevel.GetURL().SchemeIsHTTPOrHTTPS());

SQLiteTrustTokenPersister::GetToplevelConfig(
const SuitableTrustTokenOrigin& toplevel) {
auto* data = database_owner_->ToplevelData();
CHECK(data);

Expand All @@ -85,11 +81,8 @@ SQLiteTrustTokenPersister::GetToplevelConfig(const url::Origin& toplevel) {

std::unique_ptr<TrustTokenIssuerToplevelPairConfig>
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);

Expand All @@ -99,34 +92,27 @@ SQLiteTrustTokenPersister::GetIssuerToplevelPairConfig(
}

void SQLiteTrustTokenPersister::SetIssuerConfig(
const url::Origin& issuer,
const SuitableTrustTokenOrigin& issuer,
std::unique_ptr<TrustTokenIssuerConfig> config) {
DCHECK(issuer.GetURL().SchemeIsHTTPOrHTTPS());

sqlite_proto::KeyValueData<TrustTokenIssuerConfig>* data =
database_owner_->IssuerData();
CHECK(data);
data->UpdateData(issuer.Serialize(), *config);
}

void SQLiteTrustTokenPersister::SetToplevelConfig(
const url::Origin& toplevel,
const SuitableTrustTokenOrigin& toplevel,
std::unique_ptr<TrustTokenToplevelConfig> config) {
DCHECK(toplevel.GetURL().SchemeIsHTTPOrHTTPS());

sqlite_proto::KeyValueData<TrustTokenToplevelConfig>* data =
database_owner_->ToplevelData();
CHECK(data);
data->UpdateData(toplevel.Serialize(), *config);
}

void SQLiteTrustTokenPersister::SetIssuerToplevelPairConfig(
const url::Origin& issuer,
const url::Origin& toplevel,
const SuitableTrustTokenOrigin& issuer,
const SuitableTrustTokenOrigin& toplevel,
std::unique_ptr<TrustTokenIssuerToplevelPairConfig> config) {
DCHECK(issuer.GetURL().SchemeIsHTTPOrHTTPS());
DCHECK(toplevel.GetURL().SchemeIsHTTPOrHTTPS());

sqlite_proto::KeyValueData<TrustTokenIssuerToplevelPairConfig>* data =
database_owner_->IssuerToplevelPairData();
CHECK(data);
Expand Down
24 changes: 10 additions & 14 deletions services/network/trust_tokens/sqlite_trust_token_persister.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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<TrustTokenIssuerConfig> GetIssuerConfig(
const url::Origin& issuer) override;
const SuitableTrustTokenOrigin& issuer) override;
std::unique_ptr<TrustTokenToplevelConfig> GetToplevelConfig(
const url::Origin& toplevel) override;
const SuitableTrustTokenOrigin& toplevel) override;
std::unique_ptr<TrustTokenIssuerToplevelPairConfig>
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<TrustTokenIssuerConfig> config) override;
void SetToplevelConfig(
const url::Origin& toplevel,
const SuitableTrustTokenOrigin& toplevel,
std::unique_ptr<TrustTokenToplevelConfig> config) override;
void SetIssuerToplevelPairConfig(
const url::Origin& issuer,
const url::Origin& toplevel,
const SuitableTrustTokenOrigin& issuer,
const SuitableTrustTokenOrigin& toplevel,
std::unique_ptr<TrustTokenIssuerToplevelPairConfig> config) override;

private:
Expand Down
2 changes: 1 addition & 1 deletion services/network/trust_tokens/sqlite_trust_token_persister_unittest.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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<SQLiteTrustTokenPersister> persister;
SQLiteTrustTokenPersister::CreateForFilePath(
Expand Down
18 changes: 9 additions & 9 deletions services/network/trust_tokens/trust_token_persister.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,7 @@

#include <memory>

#include "url/origin.h"
#include "services/network/trust_tokens/suitable_trust_token_origin.h"

namespace network {

Expand All @@ -28,22 +28,22 @@ class TrustTokenPersister {
TrustTokenPersister& operator=(const TrustTokenPersister&) = delete;

virtual std::unique_ptr<TrustTokenIssuerConfig> GetIssuerConfig(
const url::Origin& issuer) = 0;
const SuitableTrustTokenOrigin& issuer) = 0;
virtual std::unique_ptr<TrustTokenToplevelConfig> GetToplevelConfig(
const url::Origin& toplevel) = 0;
const SuitableTrustTokenOrigin& toplevel) = 0;
virtual std::unique_ptr<TrustTokenIssuerToplevelPairConfig>
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<TrustTokenIssuerConfig> config) = 0;
virtual void SetToplevelConfig(
const url::Origin& toplevel,
const SuitableTrustTokenOrigin& toplevel,
std::unique_ptr<TrustTokenToplevelConfig> config) = 0;
virtual void SetIssuerToplevelPairConfig(
const url::Origin& issuer,
const url::Origin& toplevel,
const SuitableTrustTokenOrigin& issuer,
const SuitableTrustTokenOrigin& toplevel,
std::unique_ptr<TrustTokenIssuerToplevelPairConfig> config) = 0;
};

Expand Down
10 changes: 5 additions & 5 deletions services/network/trust_tokens/trust_token_persister_unittest.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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());
Expand All @@ -122,7 +122,7 @@ TYPED_TEST(TrustTokenPersisterTest, StoresIssuerConfigs) {
*config.add_tokens() = my_token;

auto config_to_store = std::make_unique<TrustTokenIssuerConfig>(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
Expand All @@ -149,7 +149,7 @@ TYPED_TEST(TrustTokenPersisterTest, StoresToplevelConfigs) {
*config.add_associated_issuers() = "an issuer";

auto config_to_store = std::make_unique<TrustTokenToplevelConfig>(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.
Expand All @@ -176,8 +176,8 @@ TYPED_TEST(TrustTokenPersisterTest, StoresIssuerToplevelPairConfigs) {

auto config_to_store =
std::make_unique<TrustTokenIssuerToplevelPairConfig>(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
Expand Down

0 comments on commit 24e27a4

Please sign in to comment.