Refactor HasTrustTokensAnswerer to use SuitableTrustTokenOrigin

Previously, HasTrustTokensAnswerer enforced preconditions on the
url::Origin passed to its constructor by making the constructor private
and exposing a static factory method with the potential for failure. We
can now enforce this in a cleaner way with the type-safety provided by
SuitableTrustTokenOrigin, which allows removing some tests and
deduplicating the precondition-checking logic.

R=csharrison

Bug: 1061116
Change-Id: Ief3776d605d7e64ea70c1fe738cf9725205489a3
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2132598
Commit-Queue: Charlie Harrison <csharrison@chromium.org>
Reviewed-by: Charlie Harrison <csharrison@chromium.org>
Auto-Submit: David Van Cleve <davidvc@chromium.org>
Cr-Commit-Position: refs/heads/master@{#756112}

services/network/trust_tokens/has_trust_tokens_answerer.cc

@@ -8,75 +8,55 @@
 #include "services/network/public/cpp/is_potentially_trustworthy.h"
 #include "services/network/public/mojom/trust_tokens.mojom.h"
 #include "services/network/trust_tokens/pending_trust_token_store.h"
+#include "services/network/trust_tokens/suitable_trust_token_origin.h"
 #include "url/url_constants.h"
 
 namespace network {
 
-std::unique_ptr<HasTrustTokensAnswerer> HasTrustTokensAnswerer::Create(
-    const url::Origin& top_frame_origin,
-    PendingTrustTokenStore* pending_trust_token_store) {
+HasTrustTokensAnswerer::HasTrustTokensAnswerer(
+    SuitableTrustTokenOrigin top_frame_origin,
+    PendingTrustTokenStore* pending_trust_token_store)
+    : top_frame_origin_(std::move(top_frame_origin)),
+      pending_trust_token_store_(pending_trust_token_store) {
   DCHECK(pending_trust_token_store);
-
-  if (!IsOriginPotentiallyTrustworthy(top_frame_origin))
-    return nullptr;
-
-  if (top_frame_origin.scheme() != url::kHttpsScheme &&
-      top_frame_origin.scheme() != url::kHttpScheme) {
-    return nullptr;
-  }
-
-  return base::WrapUnique(
-      new HasTrustTokensAnswerer(top_frame_origin, pending_trust_token_store));
 }
 
 HasTrustTokensAnswerer::~HasTrustTokensAnswerer() = default;
 
 void HasTrustTokensAnswerer::HasTrustTokens(const url::Origin& issuer,
                                             HasTrustTokensCallback callback) {
-  if (!IsOriginPotentiallyTrustworthy(issuer)) {
-    std::move(callback).Run(mojom::HasTrustTokensResult::New(
-        mojom::TrustTokenOperationStatus::kInvalidArgument,
-        /*has_trust_tokens=*/false));
-    return;
-  }
+  base::Optional<SuitableTrustTokenOrigin> maybe_suitable_issuer =
+      SuitableTrustTokenOrigin::Create(issuer);
 
-  if (issuer.scheme() != url::kHttpsScheme &&
-      issuer.scheme() != url::kHttpScheme) {
+  if (!maybe_suitable_issuer) {
     std::move(callback).Run(mojom::HasTrustTokensResult::New(
         mojom::TrustTokenOperationStatus::kInvalidArgument,
         /*has_trust_tokens=*/false));
     return;
   }
 
-  pending_trust_token_store_->ExecuteOrEnqueue(
-      base::BindOnce(&HasTrustTokensAnswerer::AnswerQueryWithStore,
-                     weak_factory_.GetWeakPtr(), issuer, std::move(callback)));
+  pending_trust_token_store_->ExecuteOrEnqueue(base::BindOnce(
+      &HasTrustTokensAnswerer::AnswerQueryWithStore, weak_factory_.GetWeakPtr(),
+      std::move(*maybe_suitable_issuer), std::move(callback)));
 }
 
 void HasTrustTokensAnswerer::AnswerQueryWithStore(
-    const url::Origin& issuer,
+    const SuitableTrustTokenOrigin& issuer,
     HasTrustTokensCallback callback,
     TrustTokenStore* trust_token_store) {
   DCHECK(trust_token_store);
 
-  if (!trust_token_store->SetAssociation(issuer, top_frame_origin_)) {
+  if (!trust_token_store->SetAssociation(issuer.origin(),
+                                         top_frame_origin_.origin())) {
     std::move(callback).Run(mojom::HasTrustTokensResult::New(
         mojom::TrustTokenOperationStatus::kResourceExhausted,
         /*has_trust_tokens=*/false));
     return;
   }
 
-  bool has_trust_tokens = trust_token_store->CountTokens(issuer);
+  bool has_trust_tokens = trust_token_store->CountTokens(issuer.origin());
   std::move(callback).Run(mojom::HasTrustTokensResult::New(
       mojom::TrustTokenOperationStatus::kOk, has_trust_tokens));
 }
 
-HasTrustTokensAnswerer::HasTrustTokensAnswerer(
-    const url::Origin& top_frame_origin,
-    PendingTrustTokenStore* pending_trust_token_store)
-    : top_frame_origin_(top_frame_origin),
-      pending_trust_token_store_(pending_trust_token_store) {
-  DCHECK(pending_trust_token_store);
-}
-
 }  // namespace network

services/network/trust_tokens/has_trust_tokens_answerer.h

@@ -10,6 +10,7 @@
 #include "base/callback.h"
 #include "services/network/public/mojom/trust_tokens.mojom.h"
 #include "services/network/trust_tokens/pending_trust_token_store.h"
+#include "services/network/trust_tokens/suitable_trust_token_origin.h"
 #include "services/network/trust_tokens/trust_token_store.h"
 #include "url/origin.h"
 
@@ -26,12 +27,8 @@ namespace network {
 class HasTrustTokensAnswerer : public mojom::HasTrustTokensAnswerer {
  public:
   // Constructs a new answerer bound to the given top frame origin.
-  //
-  // If |top_frame_origin| is not both (1) potentially trustworthy and
-  // (2) either HTTP or HTTPS, returns nullptr.
-  static std::unique_ptr<HasTrustTokensAnswerer> Create(
-      const url::Origin& top_frame_origin,
-      PendingTrustTokenStore* pending_trust_token_store);
+  HasTrustTokensAnswerer(SuitableTrustTokenOrigin top_frame_origin,
+                         PendingTrustTokenStore* pending_trust_token_store);
 
   ~HasTrustTokensAnswerer() override;
 
@@ -43,18 +40,15 @@ class HasTrustTokensAnswerer : public mojom::HasTrustTokensAnswerer {
                       HasTrustTokensCallback callback) override;
 
  private:
-  HasTrustTokensAnswerer(const url::Origin& top_frame_origin,
-                         PendingTrustTokenStore* pending_trust_token_store);
-
   // Continuation of HasTrustTokens: uses |trust_token_store| to answer a
   // HasTrusttokens query against |issuer|.
   //
   // Requires that |issuer| is potentially trustworthy and HTTP or HTTPS.
-  void AnswerQueryWithStore(const url::Origin& issuer,
+  void AnswerQueryWithStore(const SuitableTrustTokenOrigin& issuer,
                             HasTrustTokensCallback callback,
                             TrustTokenStore* trust_token_store);
 
-  const url::Origin top_frame_origin_;
+  const SuitableTrustTokenOrigin top_frame_origin_;
   PendingTrustTokenStore* pending_trust_token_store_;
 
   base::WeakPtrFactory<HasTrustTokensAnswerer> weak_factory_{this};

services/network/trust_tokens/has_trust_tokens_answerer_unittest.cc

@@ -16,24 +16,11 @@
 
 namespace network {
 
-TEST(HasTrustTokensAnswerer, CreatesOnlyForGoodToplevelOrigin) {
-  PendingTrustTokenStore pending_store;
-  // Opaque origin (not potentially trustworthy):
-  EXPECT_FALSE(HasTrustTokensAnswerer::Create(url::Origin(), &pending_store));
-
-  // Potentially trustworthy, but non-HTTP/HTTPS origin:
-  EXPECT_FALSE(HasTrustTokensAnswerer::Create(
-      url::Origin::Create(GURL("file:///hello.txt")), &pending_store));
-
-  // Potentially trustworthy, HTTP/HTTPS origin:
-  EXPECT_TRUE(HasTrustTokensAnswerer::Create(
-      url::Origin::Create(GURL("http://localhost")), &pending_store));
-}
-
 TEST(HasTrustTokensAnswerer, HandlesInsecureIssuerOrigin) {
   PendingTrustTokenStore pending_store;
-  auto answerer = HasTrustTokensAnswerer::Create(
-      url::Origin::Create(GURL("https://toplevel.com")), &pending_store);
+  auto answerer = std::make_unique<HasTrustTokensAnswerer>(
+      *SuitableTrustTokenOrigin::Create(GURL("https://toplevel.com")),
+      &pending_store);
 
   mojom::HasTrustTokensResultPtr result;
 
@@ -51,8 +38,9 @@ TEST(HasTrustTokensAnswerer, HandlesInsecureIssuerOrigin) {
 
 TEST(HasTrustTokensAnswerer, HandlesNonHttpNonHttpsIssuerOrigin) {
   PendingTrustTokenStore pending_store;
-  auto answerer = HasTrustTokensAnswerer::Create(
-      url::Origin::Create(GURL("https://toplevel.com")), &pending_store);
+  auto answerer = std::make_unique<HasTrustTokensAnswerer>(
+      *SuitableTrustTokenOrigin::Create(GURL("https://toplevel.com")),
+      &pending_store);
 
   mojom::HasTrustTokensResultPtr result;
 
@@ -72,8 +60,8 @@ TEST(HasTrustTokensAnswerer, HandlesNonHttpNonHttpsIssuerOrigin) {
 TEST(HasTrustTokensAnswerer, HandlesFailureToAssociateIssuer) {
   std::unique_ptr<TrustTokenStore> store = TrustTokenStore::CreateInMemory();
 
-  const url::Origin kToplevel =
-      url::Origin::Create(GURL("https://toplevel.com"));
+  const SuitableTrustTokenOrigin kToplevel =
+      *SuitableTrustTokenOrigin::Create(GURL("https://toplevel.com"));
 
   // This max-number-of-issuer limit is expected to be quite small (as of
   // writing, 2).
@@ -81,12 +69,13 @@ TEST(HasTrustTokensAnswerer, HandlesFailureToAssociateIssuer) {
     ASSERT_TRUE(store->SetAssociation(
         url::Origin::Create(
             GURL(base::StringPrintf("https://issuer%d.com", i))),
-        kToplevel));
+        kToplevel.origin()));
   }
 
   PendingTrustTokenStore pending_store;
   pending_store.OnStoreReady(std::move(store));
-  auto answerer = HasTrustTokensAnswerer::Create(kToplevel, &pending_store);
+  auto answerer =
+      std::make_unique<HasTrustTokensAnswerer>(kToplevel, &pending_store);
 
   mojom::HasTrustTokensResultPtr result;
 
@@ -109,13 +98,14 @@ TEST(HasTrustTokensAnswerer, SuccessWithNoTokens) {
   TrustTokenStore* raw_store = store.get();
 
   const url::Origin kIssuer = url::Origin::Create(GURL("https://issuer.com"));
-  const url::Origin kToplevel =
-      url::Origin::Create(GURL("https://toplevel.com"));
+  const SuitableTrustTokenOrigin kToplevel =
+      *SuitableTrustTokenOrigin::Create(GURL("https://toplevel.com"));
 
   PendingTrustTokenStore pending_store;
   pending_store.OnStoreReady(std::move(store));
 
-  auto answerer = HasTrustTokensAnswerer::Create(kToplevel, &pending_store);
+  auto answerer =
+      std::make_unique<HasTrustTokensAnswerer>(kToplevel, &pending_store);
 
   mojom::HasTrustTokensResultPtr result;
 
@@ -132,7 +122,7 @@ TEST(HasTrustTokensAnswerer, SuccessWithNoTokens) {
   EXPECT_FALSE(result->has_trust_tokens);
 
   // The query should have associated the issuer with the top-level origin.
-  EXPECT_TRUE(raw_store->IsAssociated(kIssuer, kToplevel));
+  EXPECT_TRUE(raw_store->IsAssociated(kIssuer, kToplevel.origin()));
 }
 
 TEST(HasTrustTokensAnswerer, SuccessWithTokens) {
@@ -140,12 +130,13 @@ TEST(HasTrustTokensAnswerer, SuccessWithTokens) {
   TrustTokenStore* raw_store = store.get();
 
   const url::Origin kIssuer = url::Origin::Create(GURL("https://issuer.com"));
-  const url::Origin kToplevel =
-      url::Origin::Create(GURL("https://toplevel.com"));
+  const SuitableTrustTokenOrigin kToplevel =
+      *SuitableTrustTokenOrigin::Create(GURL("https://toplevel.com"));
 
   PendingTrustTokenStore pending_store;
   pending_store.OnStoreReady(std::move(store));
-  auto answerer = HasTrustTokensAnswerer::Create(kToplevel, &pending_store);
+  auto answerer =
+      std::make_unique<HasTrustTokensAnswerer>(kToplevel, &pending_store);
 
   // Populate the store, giving the issuer a key commitment for the key "issuing
   // key" and a token issued with that key.