Fix cloud policy duplicate registrations issue.

chrome/browser/policy/cloud_policy_manager_browsertest.cc

Index: chrome/browser/policy/cloud_policy_manager_browsertest.cc
diff --git a/chrome/browser/policy/cloud_policy_manager_browsertest.cc b/chrome/browser/policy/cloud_policy_manager_browsertest.cc
new file mode 100644
index 0000000000000000000000000000000000000000..2cb6a930e4e9357d6ec6dd8e66aaca1f17df6d41
--- /dev/null
+++ b/chrome/browser/policy/cloud_policy_manager_browsertest.cc
@@ -0,0 +1,427 @@
+// Copyright (c) 2013 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 "base/bind.h"
+#include "base/bind_helpers.h"
+#include "base/command_line.h"
+#include "base/memory/ref_counted.h"
+#include "base/memory/scoped_ptr.h"
+#include "base/run_loop.h"
+#include "base/values.h"
+#include "chrome/browser/browser_process.h"
+#include "chrome/browser/policy/browser_policy_connector.h"
+#include "chrome/browser/policy/cloud_policy_client.h"
+#include "chrome/browser/policy/cloud_policy_constants.h"
+#include "chrome/browser/policy/policy_map.h"
+#include "chrome/browser/policy/policy_service.h"
+#include "chrome/browser/policy/proto/device_management_backend.pb.h"
+#include "chrome/browser/profiles/profile.h"
+#include "chrome/browser/ui/browser.h"
+#include "chrome/common/chrome_switches.h"
+#include "chrome/test/base/in_process_browser_test.h"
+#include "content/public/browser/browser_thread.h"
+#include "googleurl/src/gurl.h"
+#include "net/base/io_buffer.h"
+#include "net/base/net_errors.h"
+#include "net/base/upload_data_stream.h"
+#include "net/url_request/url_request_error_job.h"
+#include "net/url_request/url_request_filter.h"
+#include "net/url_request/url_request_job_factory.h"
+#include "net/url_request/url_request_test_job.h"
+#include "policy/policy_constants.h"
+#include "testing/gmock/include/gmock/gmock.h"
+#include "testing/gtest/include/gtest/gtest.h"
+
+#if defined(OS_CHROMEOS)
+#include "chrome/browser/chromeos/login/user_manager.h"
+#include "chrome/browser/policy/user_cloud_policy_manager_chromeos.h"
+#else
+#include "chrome/browser/policy/user_cloud_policy_manager.h"
+#include "chrome/browser/policy/user_cloud_policy_manager_factory.h"
+#include "chrome/browser/signin/signin_manager.h"
+#include "chrome/browser/signin/signin_manager_factory.h"
+#endif
+
+using testing::AnyNumber;
+using testing::InvokeWithoutArgs;
+using testing::Mock;
+using testing::_;
+
+namespace em = enterprise_management;
+
+namespace policy {
+
+namespace {
+
+// Dummy service URL for testing with request interception enabled.
+const char kServiceUrl[] = "http://dmserver.com/device_management";
+
+// HTTP headers for replies simulating a bad request response.
+const char kBadHeaders[] =
+    "HTTP/1.1 400 Bad request\0"
+    "Content-type: application/protobuf\0"
+    "\0";
+
+// HTTP headers for replies containing good responses.
+const char kGoodHeaders[] =
+    "HTTP/1.1 200 OK\0"
+    "Content-type: application/protobuf\0"
+    "\0";
+
+// A callback that returns a new URLRequestJob given a URLRequest.
+// This is used to queue callbacks that will handle expected requests.
+typedef base::Callback<net::URLRequestJob*(net::URLRequest*,
+                                           net::NetworkDelegate*)> JobCallback;
+
+// Helper callback for jobs that should fail with a network |error|.
+net::URLRequestJob* ErrorJobCallback(int error,
+                                     net::URLRequest* request,
+                                     net::NetworkDelegate* network_delegate) {
+  return new net::URLRequestErrorJob(request, network_delegate, error);
+}
+
+// Helper callback for jobs that should fail with a 400 HTTP error.
+net::URLRequestJob* BadRequestJobCallback(
+    net::URLRequest* request,
+    net::NetworkDelegate* network_delegate) {

[Mattias Nissler (ping if slow), 2013/02/13 10:50:43]

move kBadHeaders definition here?

[Joao da Silva, 2013/02/13 15:57:15]

Done.

+  std::string headers(kBadHeaders, arraysize(kBadHeaders));
+  return new net::URLRequestTestJob(
+      request, network_delegate, headers, std::string(), true);
+}
+
+// Parses the upload data in |request| into |request_msg|, and validates the
+// request. The query string in the URL must contain the |expected_type| for
+// the "request" parameter. Returns true if all checks succeeded, and the
+// request data has been parsed into |request_msg|.
+bool ValidRequest(net::URLRequest* request,
+                  const std::string& expected_type,
+                  em::DeviceManagementRequest* request_msg) {
+  if (request->method() != "POST")
+    return false;
+  std::string spec = request->url().spec();
+  if (spec.find("request=" + expected_type) == std::string::npos)
+    return false;
+
+  // This destroys the UploadDataStream to read its data, but that's OK since
+  // the request isn't going anywhere.
+  net::UploadDataStream* stream =
+      const_cast<net::UploadDataStream*>(request->get_upload());

[Mattias Nissler (ping if slow), 2013/02/13 10:50:43]

so there's no way to read a const UploadDataStream without const_cast'ing it and
destroying it?

[Joao da Silva, 2013/02/13 15:57:15]

It's actually possible to read from the const object and without destroying it,
yes.

The fetchers from device_management_service.cc set the upload data as a single
string, to the UploadDataStream has a single UploadBytesElementsReader with the
data in memory, and it's possible to read it using only const objects.

This has been done in the new file.

+  if (!stream)
+    return false;
+  stream->Init(net::CompletionCallback());
+  scoped_refptr<net::IOBuffer> buffer(new net::IOBuffer(1024));
+  int size = stream->Read(buffer.get(), 1024, net::CompletionCallback());
+  std::string data(buffer->data(), size);
+  if (!request_msg->ParseFromString(data))
+    return false;
+
+  return true;
+}
+
+// Helper callback for register jobs that should suceed. Validates the request
+// parameters and returns an appropriate response job. If |expect_reregister|
+// is true then the reregister flag must be set in the DeviceRegisterRequest
+// protobuf.
+net::URLRequestJob* RegisterJobCallback(
+    bool expect_reregister,
+    net::URLRequest* request,
+    net::NetworkDelegate* network_delegate) {
+  em::DeviceManagementRequest request_msg;
+  if (!ValidRequest(request, "register", &request_msg))
+    return BadRequestJobCallback(request, network_delegate);
+
+  if (!request_msg.has_register_request() ||
+      request_msg.has_unregister_request() ||
+      request_msg.has_policy_request() ||
+      request_msg.has_device_status_report_request() ||
+      request_msg.has_session_status_report_request() ||
+      request_msg.has_auto_enrollment_request()) {
+    return BadRequestJobCallback(request, network_delegate);
+  }
+
+  const em::DeviceRegisterRequest& register_request =
+      request_msg.register_request();
+  if (expect_reregister &&
+      (!register_request.has_reregister() || !register_request.reregister())) {
+    return BadRequestJobCallback(request, network_delegate);
+  } else if (!expect_reregister &&
+             register_request.has_reregister() &&
+             register_request.reregister()) {
+    return BadRequestJobCallback(request, network_delegate);
+  }
+
+  em::DeviceRegisterRequest::Type expected_type =
+#if defined(OS_CHROMEOS)
+      em::DeviceRegisterRequest::USER;
+#else
+      em::DeviceRegisterRequest::BROWSER;
+#endif
+  if (!register_request.has_type() || register_request.type() != expected_type)
+    return BadRequestJobCallback(request, network_delegate);
+
+  em::DeviceManagementResponse response;
+  em::DeviceRegisterResponse* register_response =
+      response.mutable_register_response();
+  register_response->set_device_management_token("s3cr3t70k3n");
+  std::string data;
+  response.SerializeToString(&data);
+
+  std::string headers(kGoodHeaders, arraysize(kGoodHeaders));

[Mattias Nissler (ping if slow), 2013/02/13 10:50:43]

seems like kGoodHeaders could be a static constant defined here?

[Joao da Silva, 2013/02/13 15:57:15]

Done.

+  return new net::URLRequestTestJob(
+      request, network_delegate, headers, data, true);
+}
+
+class MockCloudPolicyClientObserver : public CloudPolicyClient::Observer {
+ public:
+  MockCloudPolicyClientObserver() {}
+  virtual ~MockCloudPolicyClientObserver() {}
+
+  MOCK_METHOD1(OnPolicyFetched, void(CloudPolicyClient*));
+  MOCK_METHOD1(OnRegistrationStateChanged, void(CloudPolicyClient*));
+  MOCK_METHOD1(OnClientError, void(CloudPolicyClient*));
+};
+
+// Intercepts all requests to "dmserver.com" while in scope. Must be created and

[Mattias Nissler (ping if slow), 2013/02/13 10:50:43]

Let's use localhost or example.com to be on the safe side.

[Joao da Silva, 2013/02/13 15:57:15]

Done.

+// destroyed while the IO thread is valid.
+class RequestInterceptor {

[Mattias Nissler (ping if slow), 2013/02/13 10:50:43]

It seems this both large and generic enough to move to a sepparte file.

[Joao da Silva, 2013/02/13 15:57:15]

Done. Also updated device_management_service_browsertest.cc to use the new
interceptor class.

+ public:
+  RequestInterceptor() {
+    delegate_ = new Delegate();
+    scoped_ptr<net::URLRequestJobFactory::ProtocolHandler> handler(delegate_);
+    PostToIOAndWait(
+        base::Bind(&net::URLRequestFilter::AddHostnameProtocolHandler,
+                   base::Unretained(net::URLRequestFilter::GetInstance()),
+                   "http", "dmserver.com", base::Passed(&handler)));
+  }
+
+  ~RequestInterceptor() {
+    // RemoveHostnameHandler() destroys the |delegate_|, which is owned by
+    // the URLRequestFilter.
+    delegate_ = NULL;
+    PostToIOAndWait(
+        base::Bind(&net::URLRequestFilter::RemoveHostnameHandler,
+                   base::Unretained(net::URLRequestFilter::GetInstance()),
+                   "http", "dmserver.com"));
+  }
+
+  // Returns the number of pending callback jobs that haven't been used yet.
+  size_t GetPendingSize() {
+    size_t pending_size = 0xffff;

[Mattias Nissler (ping if slow), 2013/02/13 10:50:43]

That's a weird choice. How about std::numeric_limits<size_t>::max()?

[Joao da Silva, 2013/02/13 15:57:15]

Done.

+    PostToIOAndWait(base::Bind(&Delegate::GetPendingSize,
+                               base::Unretained(delegate_),
+                               &pending_size));
+    return pending_size;
+  }
+
+  // Queues |callback| to handle a request to "dmserver.com". Each callback is
+  // used only once, and in the order that they're pushed.
+  void PushJobCallback(const JobCallback& callback) {
+    PostToIOAndWait(base::Bind(&Delegate::PushJobCallback,
+                               base::Unretained(delegate_),
+                               callback));
+  }
+
+ private:
+  class Delegate : public net::URLRequestJobFactory::ProtocolHandler {
+   public:
+    virtual net::URLRequestJob* MaybeCreateJob(
+        net::URLRequest* request,
+        net::NetworkDelegate* network_delegate) const OVERRIDE {
+      CHECK(content::BrowserThread::CurrentlyOn(content::BrowserThread::IO));
+
+      if (request->url().host() != "dmserver.com") {
+        // Reject requests to other servers.
+        return ErrorJobCallback(
+            net::ERR_CONNECTION_REFUSED, request, network_delegate);
+      }
+
+      if (pending_job_callbacks_.empty()) {
+        // Reject dmserver requests by default.
+        return BadRequestJobCallback(request, network_delegate);
+      }
+
+      JobCallback callback = pending_job_callbacks_.front();
+      pending_job_callbacks_.pop();
+      return callback.Run(request, network_delegate);
+    }
+
+    void GetPendingSize(size_t* pending_size) const {
+      CHECK(content::BrowserThread::CurrentlyOn(content::BrowserThread::IO));
+      *pending_size = pending_job_callbacks_.size();
+    }
+
+    void PushJobCallback(const JobCallback& callback) {
+      CHECK(content::BrowserThread::CurrentlyOn(content::BrowserThread::IO));
+      pending_job_callbacks_.push(callback);
+    }
+
+   private:
+    // The queue of pending callbacks. 'mutable' because MaybeCreateJob() is a
+    // const method; it can't reenter though, because it runs exclusively on
+    // the IO thread.
+    mutable std::queue<JobCallback> pending_job_callbacks_;
+  };
+
+  // Helper to execute a |task| on IO, and return only after it has completed.
+  void PostToIOAndWait(const base::Closure& task) {
+    base::RunLoop run_loop;
+    content::BrowserThread::PostTaskAndReply(
+        content::BrowserThread::IO, FROM_HERE, task, run_loop.QuitClosure());
+    run_loop.Run();
+  }
+
+  // Owned by URLRequestFilter. This handle is valid on IO and only while the
+  // interceptor is valid.
+  Delegate* delegate_;
+
+  DISALLOW_COPY_AND_ASSIGN(RequestInterceptor);

[Mattias Nissler (ping if slow), 2013/02/13 10:50:43]

#include "base/basictypes.h"

[Joao da Silva, 2013/02/13 15:57:15]

Done.

+};
+
+}  // namespace
+
+// Tests the cloud policy stack using a URLRequestJobFactory::ProtocolHandler
+// to intercept requests and produce canned responses.
+class CloudPolicyManagerTest : public InProcessBrowserTest {
+ protected:
+  CloudPolicyManagerTest() {}
+  virtual ~CloudPolicyManagerTest() {}
+
+  virtual void SetUpInProcessBrowserTestFixture() OVERRIDE {
+    CommandLine* command_line = CommandLine::ForCurrentProcess();
+    command_line->AppendSwitchASCII(switches::kDeviceManagementUrl,
+                                    kServiceUrl);
+  }
+
+  virtual void SetUpOnMainThread() OVERRIDE {
+    // Checks that no policies have been loaded by the other providers before
+    // setting up the cloud connection. Other policies configured in the test
+    // machine will interfere with these tests.
+    const PolicyMap& map = g_browser_process->policy_service()->GetPolicies(
+        PolicyNamespace(POLICY_DOMAIN_CHROME, std::string()));
+    if (!map.empty()) {
+      base::DictionaryValue dict;
+      for (PolicyMap::const_iterator it = map.begin(); it != map.end(); ++it)
+        dict.SetWithoutPathExpansion(it->first, it->second.value->DeepCopy());
+      ADD_FAILURE()
+          << "There are pre-existing policies in this machine that will "
+          << "interfere with these tests. Policies found: " << dict;
+    }

[Mattias Nissler (ping if slow), 2013/02/13 10:50:43]

This should probably be factored out now that we have the code in two places.

[Joao da Silva, 2013/02/13 15:57:15]

Done. Added policy/test_utils.h to contain helpers like this; it's a bit empty
for now, but we'll probably put more utilities there now that it exists.

+
+    interceptor_.reset(new RequestInterceptor());
+
+    BrowserPolicyConnector* connector =
+        g_browser_process->browser_policy_connector();
+    connector->ScheduleServiceInitialization(0);
+
+#if !defined(OS_CHROMEOS)
+    // Mock a signed-in user. This is used by the UserCloudPolicyStore to pass
+    // the username to the UserCloudPolicyValidator.
+    SigninManager* signin_manager =
+        SigninManagerFactory::GetForProfile(browser()->profile());
+    ASSERT_TRUE(signin_manager);
+    signin_manager->SetAuthenticatedUsername("user@example.com");
+
+    UserCloudPolicyManager* policy_manager =
+        UserCloudPolicyManagerFactory::GetForProfile(browser()->profile());
+    ASSERT_TRUE(policy_manager);
+    policy_manager->Connect(g_browser_process->local_state(),
+                            UserCloudPolicyManager::CreateCloudPolicyClient(
+                                connector->device_management_service()).Pass());
+#endif

[Mattias Nissler (ping if slow), 2013/02/13 10:50:43]

I'm wondering whether this test would be simpler to follow if you'd just create
a separate CloudPolicyManager (or CloudPolicyCore) instance for testing. Given
the extensive setup you do, the test doesn't really test the real thing as it
runs in the browser.

[Joao da Silva, 2013/02/13 15:57:15]

I'd prefer not to do this setup too. Unfortunately it's required to setup a
signed-in state on the desktop, which is besides what we want to test here.

I still think this is better than creating a manager directly. This covers the
connector creating the cloud policy stuff on startup, unblocking cache loading,
etc.

+  }
+
+  virtual void CleanUpOnMainThread() OVERRIDE {
+    // Verify that all the expected requests were handled.
+    EXPECT_EQ(0u, interceptor_->GetPendingSize());
+
+    interceptor_.reset();
+  }
+
+#if defined(OS_CHROMEOS)
+  UserCloudPolicyManagerChromeOS* policy_manager() {
+    return g_browser_process->browser_policy_connector()->
+        GetUserCloudPolicyManager();
+  }
+#else
+  UserCloudPolicyManager* policy_manager() {
+    return UserCloudPolicyManagerFactory::GetForProfile(browser()->profile());
+  }
+#endif  // defined(OS_CHROMEOS)
+
+  // Register the client of the policy_manager() using a bogus auth token, and
+  // returns once the registration gets a result back.
+  void Register() {
+    ASSERT_TRUE(policy_manager());
+    ASSERT_TRUE(policy_manager()->core()->client());
+
+    base::RunLoop run_loop;
+    MockCloudPolicyClientObserver observer;
+    EXPECT_CALL(observer, OnRegistrationStateChanged(_))
+        .Times(AnyNumber())
+        .WillRepeatedly(InvokeWithoutArgs(&run_loop, &base::RunLoop::Quit));
+    EXPECT_CALL(observer, OnClientError(_))
+        .Times(AnyNumber())
+        .WillRepeatedly(InvokeWithoutArgs(&run_loop, &base::RunLoop::Quit));
+    policy_manager()->core()->client()->AddObserver(&observer);
+
+    // Give a bogus OAuth token to the |policy_manager|. This should make its
+    // CloudPolicyClient fetch the DMToken.
+    policy_manager()->RegisterClient("bogus");
+    run_loop.Run();
+    Mock::VerifyAndClearExpectations(&observer);
+    policy_manager()->core()->client()->RemoveObserver(&observer);
+  }
+
+  scoped_ptr<RequestInterceptor> interceptor_;
+};
+
+IN_PROC_BROWSER_TEST_F(CloudPolicyManagerTest, Register) {
+  // Accept one register request. The initial request should not include the
+  // reregister flag.
+  const bool expect_reregister = false;
+  interceptor_->PushJobCallback(
+      base::Bind(&RegisterJobCallback, expect_reregister));
+
+  EXPECT_FALSE(policy_manager()->core()->client()->is_registered());
+  ASSERT_NO_FATAL_FAILURE(Register());
+  EXPECT_TRUE(policy_manager()->core()->client()->is_registered());
+}
+
+IN_PROC_BROWSER_TEST_F(CloudPolicyManagerTest, RegisterFails) {
+  // The interceptor makes all requests fail by default; this will trigger
+  // an OnClientError() call on the observer.
+  EXPECT_FALSE(policy_manager()->core()->client()->is_registered());
+  ASSERT_NO_FATAL_FAILURE(Register());
+  EXPECT_FALSE(policy_manager()->core()->client()->is_registered());
+}
+
+IN_PROC_BROWSER_TEST_F(CloudPolicyManagerTest, RegisterFailsWithRetries) {
+  // Fail 4 times with ERR_NETWORK_CHANGED; the first 3 will trigger a retry,
+  // the last one will forward the error to the client and unblock the
+  // register process.
+  for (int i = 0; i < 4; ++i) {
+    interceptor_->PushJobCallback(base::Bind(&ErrorJobCallback,
+                                             net::ERR_NETWORK_CHANGED));
+  }
+
+  EXPECT_FALSE(policy_manager()->core()->client()->is_registered());
+  ASSERT_NO_FATAL_FAILURE(Register());
+  EXPECT_FALSE(policy_manager()->core()->client()->is_registered());
+}
+
+IN_PROC_BROWSER_TEST_F(CloudPolicyManagerTest, RegisterWithRetry) {
+  // Accept one register request after failing once. The retry request should
+  // set the reregister flag.
+  interceptor_->PushJobCallback(base::Bind(&ErrorJobCallback,
+                                           net::ERR_NETWORK_CHANGED));
+  const bool expect_reregister = true;
+  interceptor_->PushJobCallback(
+      base::Bind(&RegisterJobCallback, expect_reregister));
+
+  EXPECT_FALSE(policy_manager()->core()->client()->is_registered());
+  ASSERT_NO_FATAL_FAILURE(Register());
+  EXPECT_TRUE(policy_manager()->core()->client()->is_registered());
+}
+
+}  // namespace policy