Fix cloud policy duplicate registrations issue.

chrome/browser/policy/cloud_policy_browsertest.cc

Index: chrome/browser/policy/cloud_policy_browsertest.cc
diff --git a/chrome/browser/policy/cloud_policy_browsertest.cc b/chrome/browser/policy/cloud_policy_browsertest.cc
index 632ca1b3fcc0a62dd40bc2858d9e5aecd0b66739..1387bc5dafbba97b5d0bef13cf0d3f404ab227e4 100644
--- a/chrome/browser/policy/cloud_policy_browsertest.cc
+++ b/chrome/browser/policy/cloud_policy_browsertest.cc
@@ -29,7 +29,11 @@
 #include "content/public/browser/notification_source.h"
 #include "content/public/test/test_utils.h"
 #include "googleurl/src/gurl.h"
+#include "net/base/net_errors.h"
 #include "net/test/test_server.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 "policy/policy_constants.h"
 #include "testing/gmock/include/gmock/gmock.h"
 #include "testing/gtest/include/gtest/gtest.h"
@@ -44,6 +48,7 @@
 #include "chrome/browser/signin/signin_manager_factory.h"
 #endif
 
+using testing::AnyNumber;
 using testing::InvokeWithoutArgs;
 using testing::Mock;
 using testing::_;
@@ -54,6 +59,13 @@ namespace policy {
 
 namespace {
 
+// Bogus OAuth tokens to authenticate register requests at the testserver.
+// The testserver accepts any token, but does additional checks for
+// |kReRegisterAuthToken|: it verifies that the register request has the
+// re-register flag set.
+const char kAuthToken[] = "bogustoken";
+const char kReRegisterAuthToken[] = "reregistertoken";
+
 class MockCloudPolicyClientObserver : public CloudPolicyClient::Observer {
  public:
   MockCloudPolicyClientObserver() {}
@@ -64,6 +76,50 @@ class MockCloudPolicyClientObserver : public CloudPolicyClient::Observer {
   MOCK_METHOD1(OnClientError, void(CloudPolicyClient*));
 };
 
+// Intercepts requests to the testserver and makes them fail. This is used to
+// test the error recovery mechanisms.
+class ErrorInjector : public net::URLRequestJobFactory::ProtocolHandler {
+ public:
+  // Fails only the first request sent if |once| is true, otherwise makes all
+  // requests fail.
+  explicit ErrorInjector(bool once) : once_(once) {}
+  ~ErrorInjector() {}
+
+  // Registers this object as a request interceptor. It becomes owned by
+  // URLRequestFilter.
+  void Register() {
+    scoped_ptr<net::URLRequestJobFactory::ProtocolHandler> self(this);
+    content::BrowserThread::PostTask(
+        content::BrowserThread::IO, FROM_HERE,
+        base::Bind(&net::URLRequestFilter::AddHostnameProtocolHandler,
+                   base::Unretained(net::URLRequestFilter::GetInstance()),
+                   "http",
+                   net::TestServer::kLocalhost,
+                   base::Passed(&self)));
+  }
+
+  // Cleans up this object's registration at the URLRequestFilter. This must be
+  // posted to IO after all expected requests are complete, if |once_| is false.

[Mattias Nissler (ping if slow), 2013/02/11 18:23:50]

That's a weird protocol. Let's have only one way of unregistration.

[Joao da Silva, 2013/02/12 16:33:41]

Obsolete.

+  static void Unregister() {
+    net::URLRequestFilter::GetInstance()->RemoveHostnameHandler(
+        "http",
+        net::TestServer::kLocalhost);
+  }
+
+  virtual net::URLRequestJob* MaybeCreateJob(
+      net::URLRequest* request,
+      net::NetworkDelegate* network_delegate) const OVERRIDE {
+    if (once_)
+      Unregister();
+    return new net::URLRequestErrorJob(
+        request, network_delegate, net::ERR_NETWORK_CHANGED);
+  }
+
+ private:
+  bool once_;
+  DISALLOW_COPY_AND_ASSIGN(ErrorInjector);
+};
+
 const char* GetTestUser() {
 #if defined(OS_CHROMEOS)
   return chromeos::UserManager::kStubUser;
@@ -110,7 +166,7 @@ std::string GetTestPolicy() {
 
 }  // namespace
 
-// Tests the cloud policy stack(s).
+// Tests the cloud policy stack.
 class CloudPolicyTest : public InProcessBrowserTest {
  protected:
   CloudPolicyTest() {}
@@ -150,7 +206,18 @@ class CloudPolicyTest : public InProcessBrowserTest {
           << "There are pre-existing policies in this machine that will "
           << "interfere with these tests. Policies found: " << dict;
     }
+  }
+
+  CloudPolicyManager* GetUserCloudPolicyManager() {
+#if defined(OS_CHROMEOS)
+    return g_browser_process->browser_policy_connector()->
+        GetUserCloudPolicyManager();
+#else
+    return UserCloudPolicyManagerFactory::GetForProfile(browser()->profile());
+#endif  // defined(OS_CHROMEOS)
+  }
 
+  void Register(const std::string& auth_token) {
     BrowserPolicyConnector* connector =
         g_browser_process->browser_policy_connector();
     connector->ScheduleServiceInitialization(0);
@@ -178,13 +245,17 @@ class CloudPolicyTest : public InProcessBrowserTest {
     ASSERT_TRUE(policy_manager->core()->client());
     base::RunLoop run_loop;
     MockCloudPolicyClientObserver observer;
-    EXPECT_CALL(observer, OnRegistrationStateChanged(_)).WillOnce(
-        InvokeWithoutArgs(&run_loop, &base::RunLoop::Quit));
+    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");
+    policy_manager->RegisterClient(auth_token);
     run_loop.Run();
     Mock::VerifyAndClearExpectations(&observer);
     policy_manager->core()->client()->RemoveObserver(&observer);
@@ -201,7 +272,49 @@ class CloudPolicyTest : public InProcessBrowserTest {
   scoped_ptr<net::TestServer> test_server_;
 };
 
+IN_PROC_BROWSER_TEST_F(CloudPolicyTest, Register) {
+  EXPECT_FALSE(GetUserCloudPolicyManager()->core()->client()->is_registered());
+  ASSERT_NO_FATAL_FAILURE(Register(kAuthToken));
+  EXPECT_TRUE(GetUserCloudPolicyManager()->core()->client()->is_registered());
+}
+
+IN_PROC_BROWSER_TEST_F(CloudPolicyTest, RetryRegister) {
+  // Verifies that if the first register request fails and is retried then the
+  // second request will set the re-register flag.
+
+  // Make the first request fail.
+  const bool fail_once = true;
+  ErrorInjector* injector = new ErrorInjector(fail_once);
+  injector->Register();
+
+  // Register with |kReRegisterAuthToken|. The testserver only accepts the
+  // registration with that token if the re-register flag is set.

[Mattias Nissler (ping if slow), 2013/02/11 18:23:50]

This is odd, because the error handling is now in two places: In the
ErrorInjector _and_ the test server. I think we should implement that in one
place. It may make sense to run the failure tests not against the test server
but against a fake server implementation based on the injector stuff. Note that
device_management_service_browsertest.cc has almost-suitable code already.

[Joao da Silva, 2013/02/12 16:33:41]

Fair. I think there's value in having both kinds of test: against the
testserver, and with canned responses.

Let's leave this file with tests against the testserver; these changes will be
reverted because it's hard to verify the register request in this way.

I'll add a new browsertest that is similar to this one but runs with canned
responses, similar to device_management_service_browsertest. I'm not adding
these tests there because those tests run with their own
device_management_service instead of the connector's, and there's a lot of setup
for the cloud policy stack that doesn't belong there.

+  ASSERT_NO_FATAL_FAILURE(Register(kReRegisterAuthToken));
+
+  // Verify that the registration was accepted.
+  EXPECT_TRUE(GetUserCloudPolicyManager()->core()->client()->is_registered());
+}
+
+IN_PROC_BROWSER_TEST_F(CloudPolicyTest, RetryRegisterAndFail) {
+  // Make all the registration retries fail.
+  const bool always_fail = false;
+  ErrorInjector* injector = new ErrorInjector(always_fail);
+  injector->Register();
+
+  // Registering should fail and give up after reaching the retry limit.
+  ASSERT_NO_FATAL_FAILURE(Register(kAuthToken));
+  EXPECT_FALSE(GetUserCloudPolicyManager()->core()->client()->is_registered());
+
+  // Cleanup.
+  content::BrowserThread::PostTask(
+      content::BrowserThread::IO, FROM_HERE,
+      base::Bind(&ErrorInjector::Unregister));
+}
+
 IN_PROC_BROWSER_TEST_F(CloudPolicyTest, FetchPolicy) {
+  ASSERT_NO_FATAL_FAILURE(Register(kAuthToken));
+  ASSERT_TRUE(GetUserCloudPolicyManager()->core()->client()->is_registered());
+
   PolicyService* policy_service = browser()->profile()->GetPolicyService();
   {
     base::RunLoop run_loop;