Skip to content

Commit

Permalink
iwa: Extract IsolatedWebAppResponseReader class
Browse files Browse the repository at this point in the history 
This class encapsulates the Isolated-Web-App-specific logic for reading
responses from Signed Web Bundles. The `IsolatedWebAppReaderRegistry`,
which, confusingly, stored `SignedWebBundleReader`s prior to this CL,
now stores `IsolatedWebAppResponseReader`s.

All of this is in preparation for extracting the validation and
verification logic for Isolated Web Apps from the
`IsolatedWebAppReaderRegistry`, so that the validity of an Isolated Web
App can also be checked without having to go through the registry.

In this intermediary commit, `IsolatedWebAppReaderRegistry::Cache` now
stores an `absl::variant`, which will be simplified again in the next
CL.

Bug: 1366309
Change-Id: I1649a9dd3a49b3f016e99a93e6c4597c622b5199
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/4173652
Commit-Queue: Christian Flach <cmfcmf@chromium.org>
Reviewed-by: Sonja Laurila <laurila@google.com>
Reviewed-by: Oleksandr Peletskyi <peletskyi@chromium.org>
Cr-Commit-Position: refs/heads/main@{#1095617}
  • Loading branch information
@cmfcmf
cmfcmf authored and Chromium LUCI CQ committed Jan 23, 2023
1 parent 4937e13 commit e5aa271
Show file tree
Hide file tree
Showing 8 changed files with 443 additions and 125 deletions.
3 changes: 3 additions & 0 deletions chrome/browser/web_applications/BUILD.gn
View file
Original file line number Diff line number Diff line change
Expand Up @@ -76,6 +76,8 @@ source_set("web_applications") {
"isolated_web_apps/isolated_web_app_reader_registry.h",
"isolated_web_apps/isolated_web_app_reader_registry_factory.cc",
"isolated_web_apps/isolated_web_app_reader_registry_factory.h",
"isolated_web_apps/isolated_web_app_response_reader.cc",
"isolated_web_apps/isolated_web_app_response_reader.h",
"isolated_web_apps/isolated_web_app_trust_checker.cc",
"isolated_web_apps/isolated_web_app_trust_checker.h",
"isolated_web_apps/isolated_web_app_url_info.cc",
Expand Down Expand Up @@ -595,6 +597,7 @@ source_set("web_applications_unit_tests") {
"isolated_web_apps/install_isolated_web_app_from_command_line_unittest.cc",
"isolated_web_apps/isolated_web_app_reader_registry_factory_unittest.cc",
"isolated_web_apps/isolated_web_app_reader_registry_unittest.cc",
"isolated_web_apps/isolated_web_app_response_reader_unittest.cc",
"isolated_web_apps/isolated_web_app_trust_checker_unittest.cc",
"isolated_web_apps/isolated_web_app_url_info_unittest.cc",
"isolated_web_apps/isolated_web_app_url_loader_factory_unittest.cc",
Expand Down
114 changes: 47 additions & 67 deletions chrome/browser/web_applications/isolated_web_apps/isolated_web_app_reader_registry.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,7 @@
#include "base/strings/strcat.h"
#include "base/strings/stringprintf.h"
#include "base/time/time.h"
#include "chrome/browser/web_applications/isolated_web_apps/isolated_web_app_response_reader.h"
#include "chrome/browser/web_applications/isolated_web_apps/signed_web_bundle_reader.h"
#include "chrome/common/url_constants.h"
#include "components/web_package/mojom/web_bundle_parser.mojom.h"
Expand Down Expand Up @@ -79,7 +80,7 @@ void IsolatedWebAppReaderRegistry::ReadResponse(
: ReaderCacheState::kNotCached);

if (found) {
switch (cache_entry_it->second.state) {
switch (cache_entry_it->second.state()) {
case Cache::Entry::State::kPending:
// If integrity block and metadata are still being read, then the
// `SignedWebBundleReader` is not yet ready to be used for serving
Expand All @@ -90,8 +91,10 @@ void IsolatedWebAppReaderRegistry::ReadResponse(
case Cache::Entry::State::kReady:
// If integrity block and metadata have already been read, read
// the response from the cached `SignedWebBundleReader`.
DoReadResponse(cache_entry_it->second.GetReader(), resource_request,
std::move(callback));
DoReadResponse(
*absl::get<std::unique_ptr<IsolatedWebAppResponseReader>>(
cache_entry_it->second.GetReader()),
resource_request, std::move(callback));
return;
}
}
Expand All @@ -112,15 +115,17 @@ void IsolatedWebAppReaderRegistry::ReadResponse(
cache_entry_it->second.pending_requests.emplace_back(resource_request,
std::move(callback));

cache_entry_it->second.GetReader().StartReading(
base::BindOnce(
&IsolatedWebAppReaderRegistry::OnIntegrityBlockRead,
// `base::Unretained` can be used here since `this` owns `reader`.
base::Unretained(this), web_bundle_path, web_bundle_id),
base::BindOnce(
&IsolatedWebAppReaderRegistry::OnIntegrityBlockAndMetadataRead,
// `base::Unretained` can be used here since `this` owns `reader`.
base::Unretained(this), web_bundle_path, web_bundle_id));
absl::get<std::unique_ptr<SignedWebBundleReader>>(
cache_entry_it->second.GetReader())
->StartReading(
base::BindOnce(
&IsolatedWebAppReaderRegistry::OnIntegrityBlockRead,
// `base::Unretained` can be used here since `this` owns `reader`.
base::Unretained(this), web_bundle_path, web_bundle_id),
base::BindOnce(
&IsolatedWebAppReaderRegistry::OnIntegrityBlockAndMetadataRead,
// `base::Unretained` can be used here since `this` owns `reader`.
base::Unretained(this), web_bundle_path, web_bundle_id));
}

void IsolatedWebAppReaderRegistry::OnIntegrityBlockRead(
Expand Down Expand Up @@ -177,7 +182,7 @@ void IsolatedWebAppReaderRegistry::OnIntegrityBlockAndMetadataRead(

auto cache_entry_it = reader_cache_.Find(web_bundle_path);
DCHECK(cache_entry_it != reader_cache_.End());
DCHECK_EQ(cache_entry_it->second.state, Cache::Entry::State::kPending);
DCHECK_EQ(cache_entry_it->second.state(), Cache::Entry::State::kPending);

absl::optional<
std::pair<ReadResponseError, ReadIntegrityBlockAndMetadataStatus>>
Expand All @@ -188,11 +193,12 @@ void IsolatedWebAppReaderRegistry::OnIntegrityBlockAndMetadataRead(
GetStatusFromError(*read_integrity_block_and_metadata_error));
}

SignedWebBundleReader& reader = cache_entry_it->second.GetReader();
auto reader = std::move(absl::get<std::unique_ptr<SignedWebBundleReader>>(
cache_entry_it->second.GetReader()));

if (!error_and_status.has_value()) {
if (auto error_message = validator_->ValidateMetadata(
web_bundle_id, reader.GetPrimaryURL(), reader.GetEntries());
web_bundle_id, reader->GetPrimaryURL(), reader->GetEntries());
error_message.has_value()) {
error_and_status = std::make_pair(
ReadResponseError::ForMetadataValidationError(*error_message),
Expand Down Expand Up @@ -222,14 +228,17 @@ void IsolatedWebAppReaderRegistry::OnIntegrityBlockAndMetadataRead(
// consumers that were waiting for this `SignedWebBundleReader` to become
// available.
verified_files_.insert(cache_entry_it->first);
cache_entry_it->second.state = Cache::Entry::State::kReady;
cache_entry_it->second.GetReader() =
std::make_unique<IsolatedWebAppResponseReader>(std::move(reader));
for (auto& [resource_request, callback] : pending_requests) {
DoReadResponse(reader, resource_request, std::move(callback));
DoReadResponse(*absl::get<std::unique_ptr<IsolatedWebAppResponseReader>>(
cache_entry_it->second.GetReader()),
resource_request, std::move(callback));
}
}

void IsolatedWebAppReaderRegistry::DoReadResponse(
SignedWebBundleReader& reader,
IsolatedWebAppResponseReader& reader,
network::ResourceRequest resource_request,
ReadResponseCallback callback) {
DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);
Expand Down Expand Up @@ -257,28 +266,24 @@ void IsolatedWebAppReaderRegistry::DoReadResponse(
base::BindOnce(
&IsolatedWebAppReaderRegistry::OnResponseRead,
// `base::Unretained` can be used here since `this` owns `reader`.
base::Unretained(this), reader.AsWeakPtr(), std::move(callback)));
base::Unretained(this), std::move(callback)));
}

void IsolatedWebAppReaderRegistry::OnResponseRead(
base::WeakPtr<SignedWebBundleReader> reader,
ReadResponseCallback callback,
base::expected<web_package::mojom::BundleResponsePtr,
SignedWebBundleReader::ReadResponseError> response_head) {
base::UmaHistogramEnumeration(
"WebApp.Isolated.ReadResponseHeadStatus",
response_head.has_value() ? ReadResponseHeadStatus::kSuccess
: GetStatusFromError(response_head.error()));

if (!response_head.has_value()) {
base::expected<IsolatedWebAppResponseReader::Response,
IsolatedWebAppResponseReader::Error> response) {
base::UmaHistogramEnumeration("WebApp.Isolated.ReadResponseHeadStatus",
response.has_value()
? ReadResponseHeadStatus::kSuccess
: GetStatusFromError(response.error()));

if (!response.has_value()) {
std::move(callback).Run(
base::unexpected(ReadResponseError::ForError(response_head.error())));
base::unexpected(ReadResponseError::ForError(response.error())));
return;
}
// Since `this` owns `reader`, we only pass a weak reference to it to the
// `Response` object. If `this` deletes `reader`, it makes sense that the
// reference contained in `Response` also becomes invalid.
std::move(callback).Run(Response(std::move(*response_head), reader));
std::move(callback).Run(std::move(*response));
}

IsolatedWebAppReaderRegistry::ReadIntegrityBlockAndMetadataStatus
Expand Down Expand Up @@ -329,42 +334,17 @@ IsolatedWebAppReaderRegistry::GetStatusFromError(

IsolatedWebAppReaderRegistry::ReadResponseHeadStatus
IsolatedWebAppReaderRegistry::GetStatusFromError(
const SignedWebBundleReader::ReadResponseError& error) {
const IsolatedWebAppResponseReader::Error& error) {
switch (error.type) {
case SignedWebBundleReader::ReadResponseError::Type::kParserInternalError:
case IsolatedWebAppResponseReader::Error::Type::kParserInternalError:
return ReadResponseHeadStatus::kResponseHeadParserInternalError;
case SignedWebBundleReader::ReadResponseError::Type::kFormatError:
case IsolatedWebAppResponseReader::Error::Type::kFormatError:
return ReadResponseHeadStatus::kResponseHeadParserFormatError;
case SignedWebBundleReader::ReadResponseError::Type::kResponseNotFound:
case IsolatedWebAppResponseReader::Error::Type::kResponseNotFound:
return ReadResponseHeadStatus::kResponseNotFoundError;
}
}

IsolatedWebAppReaderRegistry::Response::Response(
web_package::mojom::BundleResponsePtr head,
base::WeakPtr<SignedWebBundleReader> reader)
: head_(std::move(head)), reader_(std::move(reader)) {}

IsolatedWebAppReaderRegistry::Response::Response(Response&&) = default;

IsolatedWebAppReaderRegistry::Response&
IsolatedWebAppReaderRegistry::Response::operator=(Response&&) = default;

IsolatedWebAppReaderRegistry::Response::~Response() = default;

void IsolatedWebAppReaderRegistry::Response::ReadBody(
mojo::ScopedDataPipeProducerHandle producer_handle,
base::OnceCallback<void(net::Error net_error)> callback) {
if (!reader_) {
// The weak pointer to `reader_` might no longer be valid when this is
// called.
std::move(callback).Run(net::ERR_FAILED);
return;
}
reader_->ReadResponseBody(head_->Clone(), std::move(producer_handle),
std::move(callback));
}

// static
IsolatedWebAppReaderRegistry::ReadResponseError
IsolatedWebAppReaderRegistry::ReadResponseError::ForError(
Expand Down Expand Up @@ -403,15 +383,15 @@ IsolatedWebAppReaderRegistry::ReadResponseError::ForMetadataValidationError(
// static
IsolatedWebAppReaderRegistry::ReadResponseError
IsolatedWebAppReaderRegistry::ReadResponseError::ForError(
const SignedWebBundleReader::ReadResponseError& error) {
const IsolatedWebAppResponseReader::Error& error) {
switch (error.type) {
case SignedWebBundleReader::ReadResponseError::Type::kParserInternalError:
case IsolatedWebAppResponseReader::Error::Type::kParserInternalError:
return ForOtherError(base::StringPrintf(
"Failed to parse response head: %s", error.message.c_str()));
case SignedWebBundleReader::ReadResponseError::Type::kFormatError:
case IsolatedWebAppResponseReader::Error::Type::kFormatError:
return ForOtherError(base::StringPrintf(
"Failed to parse response head: %s", error.message.c_str()));
case SignedWebBundleReader::ReadResponseError::Type::kResponseNotFound:
case IsolatedWebAppResponseReader::Error::Type::kResponseNotFound:
return ForResponseNotFound(base::StringPrintf(
"Failed to read response: %s", error.message.c_str()));
}
Expand Down Expand Up @@ -490,7 +470,7 @@ void IsolatedWebAppReaderRegistry::Cache::CleanupOldEntries() {
// If a `SignedWebBundleReader` is ready to read responses and has
// not been used for at least `kCleanupInterval`, remove it from the
// cache.
return cache_entry.state == Entry::State::kReady &&
return cache_entry.state() == Entry::State::kReady &&
now - cache_entry.last_access() > kCleanupInterval;
},
[](const std::pair<base::FilePath, Entry>& entry) -> const Entry& {
Expand Down
78 changes: 28 additions & 50 deletions chrome/browser/web_applications/isolated_web_apps/isolated_web_app_reader_registry.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,7 @@
#include "base/time/time.h"
#include "base/timer/timer.h"
#include "base/types/expected.h"
#include "chrome/browser/web_applications/isolated_web_apps/isolated_web_app_response_reader.h"
#include "chrome/browser/web_applications/isolated_web_apps/isolated_web_app_validator.h"
#include "chrome/browser/web_applications/isolated_web_apps/signed_web_bundle_reader.h"
#include "components/keyed_service/core/keyed_service.h"
Expand All @@ -24,6 +25,7 @@
#include "components/web_package/signed_web_bundles/signed_web_bundle_signature_verifier.h"
#include "services/network/public/cpp/resource_request.h"
#include "third_party/abseil-cpp/absl/types/optional.h"
#include "third_party/abseil-cpp/absl/types/variant.h"

namespace web_package {
class SignedWebBundleId;
Expand Down Expand Up @@ -51,40 +53,6 @@ class IsolatedWebAppReaderRegistry : public KeyedService {
IsolatedWebAppReaderRegistry& operator=(const IsolatedWebAppReaderRegistry&) =
delete;

// A `Response` object contains the response head, as well as a `ReadBody`
// function to read the response's body. It holds weakly onto a
// `SignedWebBundleReader` for reading the response body. This reference will
// remain valid until the reader is evicted from the cache of the
// `IsolatedWebAppReaderRegistry`.
class Response {
public:
Response(web_package::mojom::BundleResponsePtr head,
base::WeakPtr<SignedWebBundleReader> reader);

Response(const Response&) = delete;
Response& operator=(const Response&) = delete;

Response(Response&&);
Response& operator=(Response&&);

~Response();

// Returns the head of the response, which includes status code and response
// headers.
const web_package::mojom::BundleResponsePtr& head() { return head_; }

// Reads the body of the response into `producer_handle`, calling `callback`
// with `net::OK` on success, and another error code on failure. A failure
// may also occur if the `SignedWebBundleReader` that was used to read the
// response head has since been evicted from the cache.
void ReadBody(mojo::ScopedDataPipeProducerHandle producer_handle,
base::OnceCallback<void(net::Error net_error)> callback);

private:
web_package::mojom::BundleResponsePtr head_;
base::WeakPtr<SignedWebBundleReader> reader_;
};

struct ReadResponseError {
enum class Type {
kOtherError,
Expand All @@ -98,7 +66,7 @@ class IsolatedWebAppReaderRegistry : public KeyedService {
const std::string& error);

static ReadResponseError ForError(
const SignedWebBundleReader::ReadResponseError& error);
const IsolatedWebAppResponseReader::Error& error);

Type type;
std::string message;
Expand All @@ -117,7 +85,8 @@ class IsolatedWebAppReaderRegistry : public KeyedService {
};

using ReadResponseCallback = base::OnceCallback<void(
base::expected<Response, ReadResponseError> response)>;
base::expected<IsolatedWebAppResponseReader::Response, ReadResponseError>
response)>;

// Given a path to a Signed Web Bundle, the expected Signed Web Bundle ID, and
// a request, read the corresponding response from it. The `callback` receives
Expand Down Expand Up @@ -191,21 +160,20 @@ class IsolatedWebAppReaderRegistry : public KeyedService {
absl::optional<SignedWebBundleReader::ReadIntegrityBlockAndMetadataError>
read_integrity_block_and_metadata_error);

void DoReadResponse(SignedWebBundleReader& reader,
void DoReadResponse(IsolatedWebAppResponseReader& reader,
network::ResourceRequest resource_request,
ReadResponseCallback callback);

void OnResponseRead(
base::WeakPtr<SignedWebBundleReader> reader,
ReadResponseCallback callback,
base::expected<web_package::mojom::BundleResponsePtr,
SignedWebBundleReader::ReadResponseError> response_head);
base::expected<IsolatedWebAppResponseReader::Response,
IsolatedWebAppResponseReader::Error> response);

ReadIntegrityBlockAndMetadataStatus GetStatusFromError(
const SignedWebBundleReader::ReadIntegrityBlockAndMetadataError& error);

ReadResponseHeadStatus GetStatusFromError(
const SignedWebBundleReader::ReadResponseError& error);
const IsolatedWebAppResponseReader::Error& error);

enum class ReaderCacheState;

Expand Down Expand Up @@ -236,10 +204,11 @@ class IsolatedWebAppReaderRegistry : public KeyedService {
void Erase(base::flat_map<base::FilePath, Entry>::iterator iterator);

// A cache `Entry` has two states: In its initial `kPending` state, it
// caches requests made to a Signed Web Bundle until the
// `SignedWebBundleReader` is ready. Once the `SignedWebBundleReader` is
// ready to serve responses, all queued requests are run and the state is
// updated to `kReady`.
// caches requests made to a Signed Web Bundle and holds a
// `SignedWebBundleReader` in `reader_`. Once the `SignedWebBundleReader` is
// ready to serve responses, it is converted into an
// `IsolatedWebAppResponseReader`, all queued requests are run, and the
// state is updated to `kReady`.
class Entry {
public:
explicit Entry(std::unique_ptr<SignedWebBundleReader> reader);
Expand All @@ -251,15 +220,17 @@ class IsolatedWebAppReaderRegistry : public KeyedService {
Entry(Entry&& other);
Entry& operator=(Entry&& other);

SignedWebBundleReader& GetReader() {
absl::variant<std::unique_ptr<SignedWebBundleReader>,
std::unique_ptr<IsolatedWebAppResponseReader>>&
GetReader() {
last_access_ = base::TimeTicks::Now();
return *reader_;
return reader_;
}

const base::TimeTicks last_access() const { return last_access_; }

ReaderCacheState AsReaderCacheState() {
switch (state) {
switch (state()) {
case State::kPending:
return ReaderCacheState::kCachedPending;
case State::kReady:
Expand All @@ -268,14 +239,21 @@ class IsolatedWebAppReaderRegistry : public KeyedService {
}

enum class State { kPending, kReady };
State state() const {
return absl::holds_alternative<
std::unique_ptr<IsolatedWebAppResponseReader>>(reader_)
? State::kReady
: State::kPending;
}

State state = State::kPending;
std::vector<std::pair<network::ResourceRequest,
IsolatedWebAppReaderRegistry::ReadResponseCallback>>
pending_requests;

private:
std::unique_ptr<SignedWebBundleReader> reader_;
absl::variant<std::unique_ptr<SignedWebBundleReader>,
std::unique_ptr<IsolatedWebAppResponseReader>>
reader_;
// The point in time when the `reader` was last accessed.
base::TimeTicks last_access_;
};
Expand Down

0 comments on commit e5aa271

Please sign in to comment.