Skip to content

Commit

Permalink
Drop psm::FakeRlweClient
Browse files Browse the repository at this point in the history
FakeRlweClient is anything but simple. It partially reimplements the
PIR logic from the
private_membership::rlwe::PrivateMembershipRlweClient, which forces
the policy::RequestHandlerForPsmAutoEnrollment to reply with
corresponding data. However, all of this should be implementation
details of the PSM library. It contradicts the general idea that
tests should be easy to understand.

This change drops the FakeRlweClient and the
RequestHandlerForPsmAutoEnrollment.

Instead, tests can now use psm::RlweTestSupport to construct an
RlweClientImpl from a test case from the PSM library's test database.
The EmbeddedPolicyTestServer is also using the test cases from the
test database to validate requests and send appropriate responses.

This makes the test logic less complex, requiring less understanding
of implementation details.

Bug: b/241911665
Change-Id: Ifae396257638433503ff82e19945e62d38df60c1
Tests: Refactored browser tests
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/4155468
Reviewed-by: Danila Kuzmin <dkuzmin@google.com>
Commit-Queue: Roland Bock <rbock@google.com>
Reviewed-by: Igor <igorcov@chromium.org>
Reviewed-by: Amr Aboelkher <amraboelkher@chromium.org>
Cr-Commit-Position: refs/heads/main@{#1097319}
  • Loading branch information
Roland Bock authored and Chromium LUCI CQ committed Jan 26, 2023
1 parent 896fbdb commit b53d286
Show file tree
Hide file tree
Showing 14 changed files with 362 additions and 281 deletions.
12 changes: 10 additions & 2 deletions chrome/browser/ash/BUILD.gn
Expand Up @@ -4453,8 +4453,8 @@ static_library("test_support") {
"policy/core/fake_device_cloud_policy_manager.h",
"policy/core/user_policy_test_helper.cc",
"policy/core/user_policy_test_helper.h",
"policy/enrollment/psm/fake_rlwe_client.cc",
"policy/enrollment/psm/fake_rlwe_client.h",
"policy/enrollment/psm/rlwe_test_support.cc",
"policy/enrollment/psm/rlwe_test_support.h",
"policy/external_data/cloud_external_data_manager_base_test_util.cc",
"policy/external_data/cloud_external_data_manager_base_test_util.h",
"policy/handlers/fake_device_name_policy_handler.cc",
Expand Down Expand Up @@ -4627,6 +4627,7 @@ static_library("test_support") {
"//extensions/browser",
"//extensions/common:common_constants",
"//skia",
"//third_party/private_membership",
"//third_party/re2",
"//ui/base",
"//ui/chromeos/resources",
Expand All @@ -4637,6 +4638,12 @@ static_library("test_support") {
"//ui/views/controls/webview",
"//ui/wm",
]

data = [
# TODO(crbug/1393862): Script should create binarypb from textpb.
# Required for policy/enrollment/psm/rlwe_test_support.cc
"//third_party/private_membership/src/internal/testing/regression_test_data/test_data.binarypb",
]
}

source_set("unit_tests") {
Expand Down Expand Up @@ -5360,6 +5367,7 @@ source_set("unit_tests") {
"policy/enrollment/enrollment_config_unittest.cc",
"policy/enrollment/psm/construct_rlwe_id_unittest.cc",
"policy/enrollment/psm/rlwe_dmserver_client_impl_unittest.cc",
"policy/enrollment/psm/rlwe_test_support_unittest.cc",
"policy/enrollment/tpm_enrollment_key_signing_service_unittest.cc",
"policy/external_data/cloud_external_data_manager_base_unittest.cc",
"policy/external_data/cloud_external_data_policy_observer_unittest.cc",
Expand Down
Expand Up @@ -9,7 +9,6 @@
#include "ash/public/cpp/login_screen_test_api.h"
#include "base/check.h"
#include "base/command_line.h"
#include "base/functional/bind.h"
#include "base/test/gtest_util.h"
#include "base/values.h"
#include "build/build_config.h"
Expand Down Expand Up @@ -38,7 +37,7 @@
#include "chrome/browser/ash/policy/core/browser_policy_connector_ash.h"
#include "chrome/browser/ash/policy/enrollment/auto_enrollment_type_checker.h"
#include "chrome/browser/ash/policy/enrollment/enrollment_requisition_manager.h"
#include "chrome/browser/ash/policy/enrollment/psm/fake_rlwe_client.h"
#include "chrome/browser/ash/policy/enrollment/psm/rlwe_test_support.h"
#include "chrome/browser/ash/policy/server_backed_state/server_backed_state_keys_broker.h"
#include "chrome/browser/browser_process.h"
#include "chrome/browser/browser_process_platform_part.h"
Expand Down Expand Up @@ -1081,7 +1080,7 @@ IN_PROC_BROWSER_TEST_F(InitialEnrollmentTest, EnrollmentForced) {
WizardController::default_controller()
->GetAutoEnrollmentControllerForTesting()
->SetRlweClientFactoryForTesting(
base::BindRepeating(&policy::psm::FakeRlweClient::Create));
policy::psm::testing::CreateClientFactory(/*is_member=*/true));

OobeScreenWaiter(EnrollmentScreenView::kScreenId).Wait();

Expand Down Expand Up @@ -1123,7 +1122,7 @@ IN_PROC_BROWSER_TEST_F(InitialEnrollmentTest,
WizardController::default_controller()
->GetAutoEnrollmentControllerForTesting()
->SetRlweClientFactoryForTesting(
base::BindRepeating(&policy::psm::FakeRlweClient::Create));
policy::psm::testing::CreateClientFactory(/*is_member=*/true));

OobeScreenWaiter(EnrollmentScreenView::kScreenId).Wait();

Expand Down Expand Up @@ -1163,7 +1162,7 @@ IN_PROC_BROWSER_TEST_F(InitialEnrollmentTest,
WizardController::default_controller()
->GetAutoEnrollmentControllerForTesting()
->SetRlweClientFactoryForTesting(
base::BindRepeating(&policy::psm::FakeRlweClient::Create));
policy::psm::testing::CreateClientFactory(/*is_member=*/true));

enrollment_ui_.WaitForStep(test::ui::kEnrollmentStepDeviceAttributes);
enrollment_ui_.SubmitDeviceAttributes(test::values::kAssetId,
Expand Down
4 changes: 2 additions & 2 deletions chrome/browser/ash/login/oobe_interactive_ui_test.cc
Expand Up @@ -45,7 +45,7 @@
#include "chrome/browser/ash/login/ui/login_display_host.h"
#include "chrome/browser/ash/login/wizard_controller.h"
#include "chrome/browser/ash/policy/enrollment/auto_enrollment_type_checker.h"
#include "chrome/browser/ash/policy/enrollment/psm/fake_rlwe_client.h"
#include "chrome/browser/ash/policy/enrollment/psm/rlwe_test_support.h"
#include "chrome/browser/chrome_browser_main.h"
#include "chrome/browser/chrome_browser_main_extra_parts.h"
#include "chrome/browser/extensions/api/quick_unlock_private/quick_unlock_private_api.h"
Expand Down Expand Up @@ -888,7 +888,7 @@ void OobeZeroTouchInteractiveUITest::ZeroTouchEndToEnd() {
WizardController::default_controller()
->GetAutoEnrollmentControllerForTesting()
->SetRlweClientFactoryForTesting(
base::BindRepeating(&policy::psm::FakeRlweClient::Create));
policy::psm::testing::CreateClientFactory());

PerformStepsBeforeEnrollmentCheck();

Expand Down
113 changes: 0 additions & 113 deletions chrome/browser/ash/policy/enrollment/psm/fake_rlwe_client.cc

This file was deleted.

43 changes: 0 additions & 43 deletions chrome/browser/ash/policy/enrollment/psm/fake_rlwe_client.h

This file was deleted.

86 changes: 86 additions & 0 deletions chrome/browser/ash/policy/enrollment/psm/rlwe_test_support.cc
@@ -0,0 +1,86 @@
// Copyright 2023 The Chromium Authors
// Use of this source code is governed by a BSD-style license that can be
// found in the LICENSE file.

#include "chrome/browser/ash/policy/enrollment/psm/rlwe_test_support.h"

#include <memory>
#include <string>

#include "base/check.h"
#include "base/files/file_path.h"
#include "base/files/file_util.h"
#include "base/path_service.h"
#include "base/threading/thread_restrictions.h"
#include "chrome/browser/ash/policy/enrollment/psm/rlwe_client_impl.h"
#include "components/policy/proto/device_management_backend.pb.h"
#include "third_party/private_membership/src/internal/testing/regression_test_data/regression_test_data.pb.h"
#include "third_party/private_membership/src/private_membership_rlwe_client.h"

namespace em = enterprise_management;
using RlweTestData =
private_membership::rlwe::PrivateMembershipRlweClientRegressionTestData;

namespace policy::psm::testing {

namespace {

std::unique_ptr<RlweClient> CreateRlweClient(
const RlweTestCase& test_case,
const private_membership::rlwe::RlwePlaintextId& /* unused*/) {
auto status_or_client =
private_membership::rlwe::PrivateMembershipRlweClient::CreateForTesting(
private_membership::rlwe::RlweUseCase::CROS_DEVICE_STATE,
{test_case.plaintext_id()}, test_case.ec_cipher_key(),
test_case.seed()); // IN-TEST
CHECK(status_or_client.ok()) << status_or_client.status().message();

return std::make_unique<RlweClientImpl>(std::move(status_or_client).value(),
test_case.plaintext_id());
}

RlweTestData ReadTestData() {
base::FilePath src_root_dir;
CHECK(base::PathService::Get(base::DIR_SOURCE_ROOT, &src_root_dir));
const base::FilePath path_to_test_data =
src_root_dir.AppendASCII("third_party")
.AppendASCII("private_membership")
.AppendASCII("src")
.AppendASCII("internal")
.AppendASCII("testing")
.AppendASCII("regression_test_data")
.AppendASCII("test_data.binarypb");

base::ScopedAllowBlockingForTesting allow_blocking;
CHECK(base::PathExists(path_to_test_data))
<< " path_to_test_data: " << path_to_test_data;

std::string serialized_test_data;
CHECK(base::ReadFileToString(path_to_test_data, &serialized_test_data));

RlweTestData test_data;
CHECK(test_data.ParseFromString(serialized_test_data));

return test_data;
}

} // namespace

RlweTestCase LoadTestCase(bool is_member) {
const auto test_data = ReadTestData();

for (const auto& test_case : test_data.test_cases()) {
if (test_case.is_positive_membership_expected() == is_member) {
return test_case;
}
}

CHECK(false) << "Could not find psm test data for is_member == " << is_member;
return {};
}

RlweClientFactory CreateClientFactory(bool is_member) {
return base::BindRepeating(&CreateRlweClient, LoadTestCase(is_member));
}

} // namespace policy::psm::testing
44 changes: 44 additions & 0 deletions chrome/browser/ash/policy/enrollment/psm/rlwe_test_support.h
@@ -0,0 +1,44 @@
// Copyright 2023 The Chromium Authors
// Use of this source code is governed by a BSD-style license that can be
// found in the LICENSE file.

#ifndef CHROME_BROWSER_ASH_POLICY_ENROLLMENT_PSM_RLWE_TEST_SUPPORT_H_
#define CHROME_BROWSER_ASH_POLICY_ENROLLMENT_PSM_RLWE_TEST_SUPPORT_H_

#include "chrome/browser/ash/policy/enrollment/psm/rlwe_client.h"

#include <memory>

#include "base/functional/callback.h"
#include "third_party/private_membership/src/internal/testing/regression_test_data/regression_test_data.pb.h"

namespace private_membership::rlwe {
class RlwePlaintextId;
} // namespace private_membership::rlwe

namespace policy::psm::testing {

using RlweTestCase = private_membership::rlwe::
PrivateMembershipRlweClientRegressionTestData::TestCase;

using RlweClientFactory = base::RepeatingCallback<std::unique_ptr<RlweClient>(
const private_membership::rlwe::RlwePlaintextId&)>;

// Load a test case from the test database provided by the
// third_party/private_membership library. These can be used to simulate PSM
// requests for members or non-members of the set.
//
// Note that this function performs a blocking file read.
RlweTestCase LoadTestCase(bool is_member);

// Load a test case from the test database provided by the
// third_party/private_membership library. Based on that test case,
// create a callback as needed by the `AutoEnrollmentController` to create an
// `RlweClient`. The returned callback will ignore the plaintext id and
// instead create an `RlweClient` that will create requests as expected by the
// EmbeddedPolicyTestServer.
RlweClientFactory CreateClientFactory(bool is_member = true);

} // namespace policy::psm::testing

#endif // CHROME_BROWSER_ASH_POLICY_ENROLLMENT_PSM_RLWE_TEST_SUPPORT_H_

0 comments on commit b53d286

Please sign in to comment.