Initial RemoteCommandService

chrome/browser/policy/remote_commands/remote_commands_browsertest.cc

+// Copyright 2015 The Chromium Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+#include <string>
+
+#include "base/command_line.h"
+#include "base/logging.h"
+#include "base/macros.h"
+#include "base/memory/scoped_ptr.h"
+#include "base/run_loop.h"
+#include "base/time/time.h"
+#include "chrome/browser/browser_process.h"
+#include "chrome/browser/policy/cloud/test_request_interceptor.h"
+#include "chrome/browser/policy/remote_commands/testing_remote_commands_server.h"
+#include "chrome/browser/profiles/profile.h"
+#include "chrome/browser/ui/browser.h"
+#include "chrome/test/base/in_process_browser_test.h"
+#include "components/policy/core/browser/browser_policy_connector.h"
+#include "components/policy/core/common/policy_switches.h"
+#include "components/policy/core/common/remote_commands/remote_command_job.h"
+#include "components/policy/core/common/remote_commands/remote_commands_service.h"
+#include "components/policy/core/common/remote_commands/test_remote_command_job.h"
+#include "content/public/browser/browser_thread.h"
+#include "net/url_request/url_request_context_getter.h"
+#include "policy/proto/device_management_backend.pb.h"
+#include "testing/gmock/include/gmock/gmock.h"
+#include "testing/gtest/include/gtest/gtest.h"
+
+#if defined(OS_CHROMEOS)
+#include "chrome/browser/chromeos/policy/user_cloud_policy_manager_chromeos.h"
+#include "chrome/browser/chromeos/policy/user_cloud_policy_manager_factory_chromeos.h"
+#else
+#include "chrome/browser/policy/cloud/user_cloud_policy_manager_factory.h"
+#include "components/policy/core/common/cloud/user_cloud_policy_manager.h"
+#endif
+
+using testing::AnyNumber;
+using testing::InvokeWithoutArgs;
+using testing::ReturnNew;
+using testing::_;
+
+namespace policy {
+
+namespace {
+const char kTestToken[] = "secret_token";
+const char kTestClientID[] = "testing_client_id";
+const char kTestPayload[] = "_testing_payload_";
+}  // namespace
+
+namespace em = enterprise_management;
+
+class MockTestRemoteCommandFactory : public RemoteCommandJob::Factory {
+ public:
+  MockTestRemoteCommandFactory() {}
+  ~MockTestRemoteCommandFactory() override {}
+
+  MOCK_METHOD0(BuildTestCommand, TestRemoteCommandJob*());
+
+ private:
+  // RemoteCommandJob::Factory:
+  scoped_ptr<RemoteCommandJob> BuildJobForType(
+      em::RemoteCommand_Type type) override {
+    if (type != em::RemoteCommand_Type_COMMAND_ECHO_TEST)
+      return nullptr;

[bartfab (slow), 2015/02/12 14:29:18]

Nit: Add NOTREACHED().

[binjin, 2015/02/16 22:46:22]

Done.

+    return make_scoped_ptr<RemoteCommandJob>(BuildTestCommand());
+  }
+
+  DISALLOW_COPY_AND_ASSIGN(MockTestRemoteCommandFactory);
+};
+
+class MockRemoteCommandsServer : public TestingRemoteCommandsServer {
+ public:
+  MockRemoteCommandsServer() {}
+  ~MockRemoteCommandsServer() override {}
+
+  MOCK_CONST_METHOD2(SucceededJobReported,
+                     void(const std::string&, base::Time));
+  MOCK_CONST_METHOD1(FailedJobReported, void(base::Time));
+  MOCK_CONST_METHOD1(IgnoredJobReported, void(base::Time));
+
+ private:
+  // TestingRemoteCommandsServer:
+  void JobResultReported(const enterprise_management::RemoteCommandResult&
+                             job_result) const override {
+    base::Time timestamp;
+    if (job_result.has_timestamp()) {

[bartfab (slow), 2015/02/12 14:29:18]

It would be an error for the job not to have a timestamp. Rather than testing
for this, always use job_result.timestamp() (this will not crash even if no
timestamp was set by the way, courtesy of protobuf).

[binjin, 2015/02/16 22:46:22]

Done.

+      timestamp = base::TimeDelta::FromMilliseconds(job_result.timestamp()) +
+                  base::Time::UnixEpoch();
+    }
+    switch (job_result.result()) {
+      case em::RemoteCommandResult_ResultType_RESULT_SUCCESS:
+        SucceededJobReported(job_result.payload(), timestamp);
+        break;
+      case em::RemoteCommandResult_ResultType_RESULT_FAILURE:
+        FailedJobReported(timestamp);
+        break;
+      case em::RemoteCommandResult_ResultType_RESULT_IGNORED:
+        IgnoredJobReported(timestamp);
+        break;
+      default:
+        NOTREACHED();
+    }
+  }
+
+  DISALLOW_COPY_AND_ASSIGN(MockRemoteCommandsServer);
+};
+
+// XXX
+class RemoteCommandsBrowserTest : public InProcessBrowserTest {

[bartfab (slow), 2015/02/12 14:29:18]

Rather than being a browser test, I think this should be a unit test for the
RemoteCommandsService class. This would also avoid the need for all the setup
overhead as you can just create a fake/mock CloudPolicyClient and use that in
the test.

[binjin, 2015/02/16 22:46:22]

I will change this into a unit test

+ protected:
+  RemoteCommandsBrowserTest() {}
+  ~RemoteCommandsBrowserTest() override {}
+
+  // XXX
+  void Register() {
+    ASSERT_TRUE(policy_manager());
+    ASSERT_TRUE(policy_manager()->core()->client());
+
+    EXPECT_FALSE(policy_manager()->core()->client()->is_registered());
+    policy_manager()->core()->client()->SetupRegistration(kTestToken,
+                                                          kTestClientID);
+    EXPECT_TRUE(policy_manager()->core()->client()->is_registered());
+  }
+
+  void SetUpInProcessBrowserTestFixture() override {
+    base::CommandLine* command_line = base::CommandLine::ForCurrentProcess();
+    command_line->AppendSwitchASCII(switches::kDeviceManagementUrl,
+                                    "http://localhost");
+  }
+
+  void SetUpOnMainThread() override {
+    interceptor_.reset(new TestRequestInterceptor(
+        "localhost", content::BrowserThread::GetMessageLoopProxyForThread(
+                         content::BrowserThread::IO)));
+    server_.reset(new MockRemoteCommandsServer());
+
+    BrowserPolicyConnector* connector =

[bartfab (slow), 2015/02/12 14:29:18]

Nit: s/BrowserPolicyConnector*/BrowserPolicyConnector* const/

[binjin, 2015/02/16 22:46:22]

Done.

+        g_browser_process->browser_policy_connector();
+    connector->ScheduleServiceInitialization(0);
+
+    ASSERT_TRUE(policy_manager());
+
+#if !defined(OS_CHROMEOS)

[bartfab (slow), 2015/02/12 14:29:18]

I think it is OK to limit ourselves to Chrome OS now. We are only implementing
device commands at the moment, which means we are limited to Chrome OS.

[binjin, 2015/02/16 22:46:22]

Acknowledged.

+    policy_manager()->Connect(
+        g_browser_process->local_state(),
+        g_browser_process->system_request_context(),
+        UserCloudPolicyManager::CreateCloudPolicyClient(
+            connector->device_management_service(),
+            g_browser_process->system_request_context()).Pass());
+#endif
+
+    scoped_ptr<MockTestRemoteCommandFactory> factory(
+        new MockTestRemoteCommandFactory());
+    mock_factory_ = factory.get();
+    policy_manager()->core()->StartRemoteCommandsService(factory.Pass());
+  }
+
+  void TearDownOnMainThread() override {
+    EXPECT_EQ(0u, interceptor_->GetPendingSize());
+    mock_factory_ = nullptr;

[bartfab (slow), 2015/02/12 14:29:18]

No need for this, really. It is a non-owner pointer.

[binjin, 2015/02/16 22:46:22]

Done.

+    server_.reset();
+    interceptor_.reset();
+  }
+
+#if defined(OS_CHROMEOS)
+  UserCloudPolicyManagerChromeOS* policy_manager() {
+    return UserCloudPolicyManagerFactoryChromeOS::GetForProfile(

[bartfab (slow), 2015/02/12 14:29:18]

We are only implementing device commands for now. Using the user policy
infrastructure for this is weird.

[binjin, 2015/02/16 22:46:22]

Acknowledged.

+        browser()->profile());
+  }
+#else
+  UserCloudPolicyManager* policy_manager() {
+    return UserCloudPolicyManagerFactory::GetForBrowserContext(
+        browser()->profile());
+  }
+#endif  // defined(OS_CHROMEOS)
+
+  scoped_ptr<TestRequestInterceptor> interceptor_;
+  scoped_ptr<MockRemoteCommandsServer> server_;
+  MockTestRemoteCommandFactory* mock_factory_;

[bartfab (slow), 2015/02/12 14:29:18]

Nit: Initialize with = nullptr.

[binjin, 2015/02/16 22:46:22]

Done.

+
+ private:
+  DISALLOW_COPY_AND_ASSIGN(RemoteCommandsBrowserTest);
+};
+
+IN_PROC_BROWSER_TEST_F(RemoteCommandsBrowserTest, SingleCommand) {
+  Register();
+
+  interceptor_->PushJobCallback(
+      TestRequestInterceptor::FetchRemoteCommandsJob(server_.get(), true));
+  interceptor_->PushJobCallback(
+      TestRequestInterceptor::FetchRemoteCommandsJob(server_.get(), true));

[bartfab (slow), 2015/02/12 14:29:18]

Why do we expect two callbacks?

[binjin, 2015/02/16 22:46:22]

First one is for commands fetching, second one is for commands result return
back.

+
+  base::RunLoop run_loop;
+  EXPECT_CALL(*mock_factory_, BuildTestCommand())
+      .Times(1)
+      .WillOnce(ReturnNew<TestRemoteCommandJob>(
+          true, base::TimeDelta::FromSeconds(1)));
+  EXPECT_CALL(*server_, SucceededJobReported(kTestPayload, _))
+      .Times(1)
+      .WillOnce(InvokeWithoutArgs(&run_loop, &base::RunLoop::Quit));
+  server_->IssueCommand(em::RemoteCommand_Type_COMMAND_ECHO_TEST, kTestPayload);
+
+  policy_manager()->core()->remote_commands_service()->FetchRemoteCommands();

[bartfab (slow), 2015/02/12 14:29:18]

Will connecting the client not issue a remote command fetch? If so, we could
enqueue a command, *then* connect the client and wait for it to issue a command
fetch.

[binjin, 2015/02/16 22:46:22]

Acknowledged.

+  run_loop.Run();
+
+  EXPECT_EQ(0u, server_->GetUnreportedCommandCount());
+}
+
+}  // namespace policy