Chromium Code Reviews
chromiumcodereview-hr@appspot.gserviceaccount.com (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out
(914)

Unified Diff: chrome/browser/safe_browsing/srt_fetcher_browsertest_win.cc

Issue 2780873002: Basic IPC communication between Chrome and the SW Reporter. (Closed)
Patch Set: RefCounted subclasses can't have public destructors Created 3 years, 8 months ago
Use n/p to move between diff chunks; N/P to move between comments. Draft comments are only viewable by you.
Jump to:
View side-by-side diff with in-line comments
Download patch
Index: chrome/browser/safe_browsing/srt_fetcher_browsertest_win.cc
diff --git a/chrome/browser/safe_browsing/srt_fetcher_browsertest_win.cc b/chrome/browser/safe_browsing/srt_fetcher_browsertest_win.cc
index ad157be9a251e05d48bbd32021cb0a5a5e615357..60053da7e6aeaf67233d9afea955a2f5db815d7d 100644
--- a/chrome/browser/safe_browsing/srt_fetcher_browsertest_win.cc
+++ b/chrome/browser/safe_browsing/srt_fetcher_browsertest_win.cc
@@ -5,18 +5,24 @@
#include "chrome/browser/safe_browsing/srt_fetcher_win.h"
#include <initializer_list>
-#include <iterator>
#include <set>
+#include <tuple>
+#include <utility>
#include <vector>
#include "base/bind.h"
#include "base/bind_helpers.h"
#include "base/callback.h"
+#include "base/callback_helpers.h"
#include "base/files/file_path.h"
#include "base/memory/ref_counted.h"
#include "base/message_loop/message_loop.h"
+#include "base/run_loop.h"
+#include "base/strings/string_number_conversions.h"
+#include "base/test/multiprocess_test.h"
#include "base/test/scoped_feature_list.h"
#include "base/test/test_mock_time_task_runner.h"
+#include "base/threading/sequenced_task_runner_handle.h"
#include "base/threading/thread_task_runner_handle.h"
#include "base/time/time.h"
#include "base/version.h"
@@ -30,23 +36,197 @@
#include "chrome/browser/ui/test/test_browser_dialog.h"
#include "chrome/common/pref_names.h"
#include "chrome/test/base/in_process_browser_test.h"
+#include "components/chrome_cleaner/public/interfaces/chrome_prompt.mojom.h"
#include "components/component_updater/pref_names.h"
#include "components/prefs/pref_service.h"
#include "components/safe_browsing_db/safe_browsing_prefs.h"
+#include "mojo/edk/embedder/embedder.h"
+#include "mojo/edk/embedder/scoped_ipc_support.h"
+#include "mojo/edk/system/core.h"
+#include "testing/multiprocess_func_list.h"
namespace safe_browsing {
namespace {
-const char* const kExpectedSwitches[] = {kExtendedSafeBrowsingEnabledSwitch,
- kChromeVersionSwitch,
- kChromeChannelSwitch};
+using chrome_cleaner::mojom::PromptAcceptance;
+
+// Special switches passed by the parent process (test case) to the reporter
+// child process to indicate the behavior that should be mocked.
+constexpr char kExitCodeToReturnSwitch[] = "exit-code-to-return";
+constexpr char kReportUwSFoundSwitch[] = "report-uws-found";
+constexpr char kReportElevationRequiredSwitch[] = "report-elevation-required";
+constexpr char kExpectedPromptAcceptanceSwitch[] = "expected-prompt-acceptance";
+
+// The exit code to be returned in case of failure in the child process.
+// This should never be passed as the expected exit code to be reported by
+// tests.
+constexpr int kFailureExitCode = -1;
+
+// Pass the |prompt_acceptance| to the mock child process in command line
+// switch kExpectedPromptAcceptanceSwitch.
+void AddPromptAcceptanceToCommandLine(PromptAcceptance prompt_acceptance,
+ base::CommandLine* command_line) {
+ command_line->AppendSwitchASCII(
+ kExpectedPromptAcceptanceSwitch,
+ base::IntToString(static_cast<int>(prompt_acceptance)));
+}
+
+// Parses and returns the prompt acceptance value passed by the parent process
+// in command line switch kExpectedPromptAcceptanceSwitch. Returns
+// PromptAcceptance::UNSPECIFIED if the switch doesn't exist or can't be
+// parsed to a valid PromptAcceptance enumerator.
+PromptAcceptance PromptAcceptanceFromCommandLine(
+ const base::CommandLine& command_line) {
+ const std::string& prompt_acceptance_str =
+ command_line.GetSwitchValueASCII(kExpectedPromptAcceptanceSwitch);
+ int val = -1;
+ if (base::StringToInt(prompt_acceptance_str, &val)) {
+ PromptAcceptance prompt_acceptance = static_cast<PromptAcceptance>(val);
+ if (chrome_cleaner::mojom::IsKnownEnumValue(prompt_acceptance))
+ return prompt_acceptance;
+ }
+ return PromptAcceptance::UNSPECIFIED;
+}
+
+// Pointer to ChromePromptPtr object to be used by the child process. The
+// object must be created, deleted, and accessed on the IPC thread only.
+chrome_cleaner::mojom::ChromePromptPtr* g_chrome_prompt_ptr = nullptr;
+
+// The callback function to be passed to ChromePrompt::PromptUser to check if
+// the prompt accepted returned by the parent process is equal to
+// |expected_prompt_acceptance|. Will set |expected_value_received| with the
+// comparison result, so that the main thread can report failure. Will invoke
+// |done| callback when done.
+void PromptUserCallback(const base::Closure& done,
+ PromptAcceptance expected_prompt_acceptance,
+ bool* expected_value_received,
+ PromptAcceptance prompt_acceptance) {
+ *expected_value_received = prompt_acceptance == expected_prompt_acceptance;
+ // It's safe to delete the ChromePromptPtr object here, since it will not be
+ // used anymore by the child process.
+ delete g_chrome_prompt_ptr;
+ g_chrome_prompt_ptr = nullptr;
+ done.Run();
+}
+
+// Mocks the sending of scan results from the child process to the parent
+// process. Obtains the behavior to be mocked from special switches in
+// |command_line|. Sets |expected_value_received| as true if the parent
+// process replies with the expected prompt acceptance value.
+void SendScanResults(const std::string& chrome_mojo_pipe_token,
+ const base::CommandLine& command_line,
+ const base::Closure& done,
+ bool* expected_value_received) {
+ constexpr int kDefaultUwSId = 10;
+ constexpr char kDefaultUwSName[] = "RemovedUwS";
+
+ mojo::ScopedMessagePipeHandle message_pipe_handle =
+ mojo::edk::CreateChildMessagePipe(chrome_mojo_pipe_token);
+ // This pointer will be deleted by PromptUserCallback.
+ g_chrome_prompt_ptr = new chrome_cleaner::mojom::ChromePromptPtr();
+ g_chrome_prompt_ptr->Bind(chrome_cleaner::mojom::ChromePromptPtrInfo(
+ std::move(message_pipe_handle), 0));
+
+ std::vector<chrome_cleaner::mojom::UwSPtr> removable_uws_found;
+ if (command_line.HasSwitch(kReportUwSFoundSwitch)) {
+ chrome_cleaner::mojom::UwSPtr uws = chrome_cleaner::mojom::UwS::New();
+ uws->id = kDefaultUwSId;
+ uws->name = kDefaultUwSName;
+ uws->observed_behaviours = chrome_cleaner::mojom::ObservedBehaviours::New();
+ removable_uws_found.push_back(std::move(uws));
+ }
+ const bool elevation_required =
+ command_line.HasSwitch(kReportElevationRequiredSwitch);
+ const PromptAcceptance expected_prompt_acceptance =
+ PromptAcceptanceFromCommandLine(command_line);
+
+ (*g_chrome_prompt_ptr)
+ ->PromptUser(
+ std::move(removable_uws_found), elevation_required,
+ base::Bind(&PromptUserCallback, done, expected_prompt_acceptance,
+ expected_value_received));
+}
+
+// Connects to the parent process and sends mocked scan results. Returns true
+// if connection was successful and the prompt acceptance results sent by the
+// parent process are the same as expected.
+bool ConnectAndSendDataToParentProcess(
+ const std::string& chrome_mojo_pipe_token,
+ const base::CommandLine& command_line) {
+ DCHECK(!chrome_mojo_pipe_token.empty());
+
+ mojo::edk::Init();
+ base::Thread::Options options(base::MessageLoop::TYPE_IO, 0);
+ base::Thread io_thread("IPCThread");
+ if (!io_thread.StartWithOptions(options))
+ return false;
+ mojo::edk::ScopedIPCSupport ipc_support(
+ io_thread.task_runner(),
+ mojo::edk::ScopedIPCSupport::ShutdownPolicy::CLEAN);
+ mojo::edk::SetParentPipeHandleFromCommandLine();
+ base::MessageLoop message_loop;
+ base::RunLoop run_loop;
+ // After the response from the parent process is received, this will post a
+ // task to unblock the child process's main thread.
+ auto done = base::Bind(
+ [](scoped_refptr<base::SequencedTaskRunner> main_runner,
+ base::Closure quit_closure) {
+ main_runner->PostTask(FROM_HERE, std::move(quit_closure));
+ },
+ base::SequencedTaskRunnerHandle::Get(),
+ base::Passed(run_loop.QuitClosure()));
+
+ bool expected_value_received = false;
+ io_thread.task_runner()->PostTask(
+ FROM_HERE, base::Bind(&SendScanResults, chrome_mojo_pipe_token,
+ command_line, done, &expected_value_received));
+ run_loop.Run();
+
+ return expected_value_received;
+}
+
+// Mocks a Software Reporter process that returns an exit code specified by
+// command line switch kExitCodeToReturnSwitch. If a Mojo IPC is available,
+// this will also connect to the parent process and send mocked scan results
+// to the parent process using data passed as command line switches.
+MULTIPROCESS_TEST_MAIN(MockSwReporterProcess) {
+ base::CommandLine* command_line = base::CommandLine::ForCurrentProcess();
+ const std::string& str =
+ command_line->GetSwitchValueASCII(kExitCodeToReturnSwitch);
+ const std::string& chrome_mojo_pipe_token =
+ command_line->GetSwitchValueASCII(kChromeMojoPipeTokenSwitch);
+ int exit_code_to_report = kFailureExitCode;
+ bool success = base::StringToInt(str, &exit_code_to_report) &&
+ (chrome_mojo_pipe_token.empty() ||
+ ConnectAndSendDataToParentProcess(chrome_mojo_pipe_token,
+ *command_line));
+ return success ? exit_code_to_report : kFailureExitCode;
+}
-class SRTFetcherTest : public InProcessBrowserTest,
- public SwReporterTestingDelegate {
+// Parameters for this test:
+// - bool in_browser_cleaner_ui: indicates if InBrowserCleanerUI experiment
+// is enabled; if so, the parent and the child processes will communicate
+// via a Mojo IPC;
+// - bool elevation_required: indicates if the scan results sent by the child
+// process should consider that elevation will be required for cleanup.
+class SRTFetcherTest
+ : public InProcessBrowserTest,
+ public SwReporterTestingDelegate,
+ public ::testing::WithParamInterface<std::tuple<bool, bool>> {
public:
void SetUpInProcessBrowserTestFixture() override {
SetSwReporterTestingDelegate(this);
+
+ std::tie(in_browser_cleaner_ui_, elevation_required_) = GetParam();
+ // The config should only accept elevation_required_ if InBrowserCleanerUI
+ // feature is enabled.
+ ASSERT_TRUE(!elevation_required_ || in_browser_cleaner_ui_);
+
+ if (in_browser_cleaner_ui_)
+ scoped_feature_list_.InitAndEnableFeature(kInBrowserCleanerUIFeature);
+ else
+ scoped_feature_list_.InitAndDisableFeature(kInBrowserCleanerUIFeature);
}
void SetUpOnMainThread() override {
@@ -80,14 +260,37 @@ class SRTFetcherTest : public InProcessBrowserTest,
prompt_trigger_called_ = true;
}
- // Records that the reporter was launched with the parameters given in
- // |invocation|.
- int LaunchReporter(const SwReporterInvocation& invocation) override {
+ // Spawns and returns a subprocess to mock an execution of the reporter with
+ // the parameters given in |invocation| that will return
+ // |exit_code_to_report_|. If IPC communication needs to be mocked, this will
+ // also provide values that define the expected behavior of the child
+ // process.
+ // Records the launch and parameters used for further verification.
+ base::Process LaunchReporter(
+ const SwReporterInvocation& invocation,
+ const base::LaunchOptions& launch_options) override {
++reporter_launch_count_;
reporter_launch_parameters_.push_back(invocation);
if (first_launch_callback_)
std::move(first_launch_callback_).Run();
- return exit_code_to_report_;
+
+ base::CommandLine command_line(
+ base::GetMultiProcessTestChildBaseCommandLine());
+ command_line.AppendArguments(invocation.command_line,
+ /*include_program=*/false);
+ command_line.AppendSwitchASCII(kExitCodeToReturnSwitch,
+ base::IntToString(exit_code_to_report_));
+ if (in_browser_cleaner_ui_) {
+ AddPromptAcceptanceToCommandLine(PromptAcceptance::DENIED, &command_line);
+ if (exit_code_to_report_ == kSwReporterCleanupNeeded) {
+ command_line.AppendSwitch(kReportUwSFoundSwitch);
+ if (elevation_required_)
+ command_line.AppendSwitch(kReportElevationRequiredSwitch);
+ }
+ }
+ base::SpawnChildResult result = base::SpawnMultiProcessTestChild(
+ "MockSwReporterProcess", command_line, launch_options);
+ return std::move(result.process);
}
// Returns the test's idea of the current time.
@@ -231,22 +434,20 @@ class SRTFetcherTest : public InProcessBrowserTest,
}
// Expects |invocation|'s command line to contain all the switches required
- // for reporter logging.
+ // for reporter logging if and only if |expect_switches| is true.
void ExpectLoggingSwitches(const SwReporterInvocation& invocation,
bool expect_switches) {
+ static const std::set<std::string> logging_switches{
+ kExtendedSafeBrowsingEnabledSwitch, kChromeVersionSwitch,
+ kChromeChannelSwitch};
+
const base::CommandLine::SwitchMap& invocation_switches =
invocation.command_line.GetSwitches();
- std::set<std::string> expected_switches;
- if (expect_switches)
- expected_switches = {std::begin(kExpectedSwitches),
- std::end(kExpectedSwitches)};
- EXPECT_EQ(expected_switches.size(), invocation_switches.size());
- // Checks if all expected switches are in the invocation switches. It's not
- // necessary to check if all invocation switches are expected, since we
- // checked if both sets should have the same size.
- for (const std::string& expected_switch : expected_switches) {
- EXPECT_NE(invocation_switches.end(),
- invocation_switches.find(expected_switch));
+ // Checks if switches that enable logging on the reporter are present on
+ // the invocation if and only if logging is allowed.
+ for (const std::string& logging_switch : logging_switches) {
+ EXPECT_EQ(expect_switches, invocation_switches.find(logging_switch) !=
+ invocation_switches.end());
}
}
@@ -257,6 +458,9 @@ class SRTFetcherTest : public InProcessBrowserTest,
// The task runner that was in use before installing |mock_time_task_runner_|.
scoped_refptr<base::SingleThreadTaskRunner> saved_task_runner_;
+ bool in_browser_cleaner_ui_;
+ bool elevation_required_;
+
bool prompt_trigger_called_ = false;
int reporter_launch_count_ = 0;
std::vector<SwReporterInvocation> reporter_launch_parameters_;
@@ -266,6 +470,8 @@ class SRTFetcherTest : public InProcessBrowserTest,
// can be used to perform actions in the middle of a queue of reporters which
// all launch on the same mock clock tick.
base::OnceClosure first_launch_callback_;
+
+ base::test::ScopedFeatureList scoped_feature_list_;
};
class SRTFetcherPromptTest : public DialogBrowserTest {
@@ -283,19 +489,19 @@ class SRTFetcherPromptTest : public DialogBrowserTest {
} // namespace
-IN_PROC_BROWSER_TEST_F(SRTFetcherTest, NothingFound) {
+IN_PROC_BROWSER_TEST_P(SRTFetcherTest, NothingFound) {
RunReporter(kSwReporterNothingFound);
ExpectReporterLaunches(0, 1, false);
ExpectToRunAgain(kDaysBetweenSuccessfulSwReporterRuns);
}
-IN_PROC_BROWSER_TEST_F(SRTFetcherTest, CleanupNeeded) {
+IN_PROC_BROWSER_TEST_P(SRTFetcherTest, CleanupNeeded) {
RunReporter(kSwReporterCleanupNeeded);
ExpectReporterLaunches(0, 1, true);
ExpectToRunAgain(kDaysBetweenSuccessfulSwReporterRuns);
}
-IN_PROC_BROWSER_TEST_F(SRTFetcherTest, RanRecently) {
+IN_PROC_BROWSER_TEST_P(SRTFetcherTest, RanRecently) {
constexpr int kDaysLeft = 1;
SetDaysSinceLastTriggered(kDaysBetweenSuccessfulSwReporterRuns - kDaysLeft);
RunReporter(kSwReporterNothingFound);
@@ -306,7 +512,7 @@ IN_PROC_BROWSER_TEST_F(SRTFetcherTest, RanRecently) {
}
// Test is flaky. crbug.com/705608
-IN_PROC_BROWSER_TEST_F(SRTFetcherTest, DISABLED_WaitForBrowser) {
+IN_PROC_BROWSER_TEST_P(SRTFetcherTest, DISABLED_WaitForBrowser) {
Profile* profile = browser()->profile();
// Ensure that even though we're closing the last browser, we don't enter the
@@ -346,13 +552,13 @@ IN_PROC_BROWSER_TEST_F(SRTFetcherTest, DISABLED_WaitForBrowser) {
ExpectToRunAgain(kDaysBetweenSuccessfulSwReporterRuns);
}
-IN_PROC_BROWSER_TEST_F(SRTFetcherTest, Failure) {
+IN_PROC_BROWSER_TEST_P(SRTFetcherTest, Failure) {
RunReporter(kReporterFailureExitCode);
ExpectReporterLaunches(0, 1, false);
ExpectToRunAgain(kDaysBetweenSuccessfulSwReporterRuns);
}
-IN_PROC_BROWSER_TEST_F(SRTFetcherTest, RunDaily) {
+IN_PROC_BROWSER_TEST_P(SRTFetcherTest, RunDaily) {
PrefService* local_state = g_browser_process->local_state();
local_state->SetBoolean(prefs::kSwReporterPendingPrompt, true);
SetDaysSinceLastTriggered(kDaysBetweenSuccessfulSwReporterRuns - 1);
@@ -381,7 +587,7 @@ IN_PROC_BROWSER_TEST_F(SRTFetcherTest, RunDaily) {
ExpectToRunAgain(kDaysBetweenSuccessfulSwReporterRuns);
}
-IN_PROC_BROWSER_TEST_F(SRTFetcherTest, ParameterChange) {
+IN_PROC_BROWSER_TEST_P(SRTFetcherTest, ParameterChange) {
// If the reporter is run several times with different parameters, it should
// only be launched once, with the last parameter set.
const base::FilePath path1(L"path1");
@@ -438,7 +644,7 @@ IN_PROC_BROWSER_TEST_F(SRTFetcherTest, ParameterChange) {
ExpectToRunAgain(kDaysBetweenSuccessfulSwReporterRuns);
}
-IN_PROC_BROWSER_TEST_F(SRTFetcherTest, MultipleLaunches) {
+IN_PROC_BROWSER_TEST_P(SRTFetcherTest, MultipleLaunches) {
const base::FilePath path1(L"path1");
const base::FilePath path2(L"path2");
const base::FilePath path3(L"path3");
@@ -506,8 +712,7 @@ IN_PROC_BROWSER_TEST_F(SRTFetcherTest, MultipleLaunches) {
}
}
-IN_PROC_BROWSER_TEST_F(SRTFetcherTest, ReporterLogging_NoSBExtendedReporting) {
- base::test::ScopedFeatureList scoped_feature_list;
+IN_PROC_BROWSER_TEST_P(SRTFetcherTest, ReporterLogging_NoSBExtendedReporting) {
RunReporter(kSwReporterNothingFound);
ExpectReporterLaunches(0, 1, false);
ExpectLoggingSwitches(reporter_launch_parameters_.front(), false);
@@ -515,8 +720,7 @@ IN_PROC_BROWSER_TEST_F(SRTFetcherTest, ReporterLogging_NoSBExtendedReporting) {
ExpectToRunAgain(kDaysBetweenSuccessfulSwReporterRuns);
}
-IN_PROC_BROWSER_TEST_F(SRTFetcherTest, ReporterLogging_EnabledFirstRun) {
- base::test::ScopedFeatureList scoped_feature_list;
+IN_PROC_BROWSER_TEST_P(SRTFetcherTest, ReporterLogging_EnabledFirstRun) {
EnableSBExtendedReporting();
// Note: don't set last time sent logs in the local state.
// SBER is enabled and there is no record in the local state of the last time
@@ -528,8 +732,7 @@ IN_PROC_BROWSER_TEST_F(SRTFetcherTest, ReporterLogging_EnabledFirstRun) {
ExpectToRunAgain(kDaysBetweenSuccessfulSwReporterRuns);
}
-IN_PROC_BROWSER_TEST_F(SRTFetcherTest, ReporterLogging_EnabledNoRecentLogging) {
- base::test::ScopedFeatureList scoped_feature_list;
+IN_PROC_BROWSER_TEST_P(SRTFetcherTest, ReporterLogging_EnabledNoRecentLogging) {
// SBER is enabled and last time logs were sent was more than
// |kDaysBetweenReporterLogsSent| day ago, so we should send logs in this run.
EnableSBExtendedReporting();
@@ -541,8 +744,7 @@ IN_PROC_BROWSER_TEST_F(SRTFetcherTest, ReporterLogging_EnabledNoRecentLogging) {
ExpectToRunAgain(kDaysBetweenSuccessfulSwReporterRuns);
}
-IN_PROC_BROWSER_TEST_F(SRTFetcherTest, ReporterLogging_EnabledRecentlyLogged) {
- base::test::ScopedFeatureList scoped_feature_list;
+IN_PROC_BROWSER_TEST_P(SRTFetcherTest, ReporterLogging_EnabledRecentlyLogged) {
// SBER is enabled, but logs have been sent less than
// |kDaysBetweenReporterLogsSent| day ago, so we shouldn't send any logs in
// this run.
@@ -556,8 +758,7 @@ IN_PROC_BROWSER_TEST_F(SRTFetcherTest, ReporterLogging_EnabledRecentlyLogged) {
ExpectToRunAgain(kDaysBetweenSuccessfulSwReporterRuns);
}
-IN_PROC_BROWSER_TEST_F(SRTFetcherTest, ReporterLogging_MultipleLaunches) {
- base::test::ScopedFeatureList scoped_feature_list;
+IN_PROC_BROWSER_TEST_P(SRTFetcherTest, ReporterLogging_MultipleLaunches) {
EnableSBExtendedReporting();
SetLastTimeSentReport(kDaysBetweenReporterLogsSent + 3);
@@ -593,6 +794,16 @@ IN_PROC_BROWSER_TEST_F(SRTFetcherTest, ReporterLogging_MultipleLaunches) {
ExpectToRunAgain(kDaysBetweenSuccessfulSwReporterRuns);
}
+INSTANTIATE_TEST_CASE_P(NoInBrowserCleanerUI,
+ SRTFetcherTest,
+ testing::Combine(testing::Values(false),
+ testing::Values(false)));
+
+INSTANTIATE_TEST_CASE_P(InBrowserCleanerUI,
+ SRTFetcherTest,
+ testing::Combine(testing::Values(true),
+ testing::Bool()));
+
// This provide tests which allows explicit invocation of the SRT Prompt
// useful for checking dialog layout or any other interactive functionality
// tests. See docs/testing/test_browser_dialog.md for description of the
« no previous file with comments | « chrome/browser/safe_browsing/srt_chrome_prompt_impl.cc ('k') | chrome/browser/safe_browsing/srt_fetcher_win.h » ('j') | no next file with comments »

Powered by Google App Engine
This is Rietveld 408576698