Fix cloud policy duplicate registrations issue.

chrome/browser/policy/cloud_policy_browsertest.cc

 // Copyright (c) 2012 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/base_paths.h"
 #include "base/command_line.h"
 #include "base/file_util.h"
 #include "base/files/scoped_temp_dir.h"
 #include "base/memory/scoped_ptr.h"
 #include "base/path_service.h"
@@ skipping 11 matching lines @@
 #include "chrome/browser/profiles/profile.h"
 #include "chrome/browser/ui/browser.h"
 #include "chrome/common/chrome_notification_types.h"
 #include "chrome/common/chrome_switches.h"
 #include "chrome/test/base/in_process_browser_test.h"
 #include "content/public/browser/browser_thread.h"
 #include "content/public/browser/notification_service.h"
 #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"
 
 #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 {
 
+// 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() {}
   virtual ~MockCloudPolicyClientObserver() {}
 
   MOCK_METHOD1(OnPolicyFetched, void(CloudPolicyClient*));
   MOCK_METHOD1(OnRegistrationStateChanged, void(CloudPolicyClient*));
   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;
 #else
   return "user@example.com";
 #endif
 }
 
 std::string GetEmptyPolicy() {
   const char kEmptyPolicy[] =
@@ skipping 26 matching lines @@
       "  \"managed_users\": [ \"*\" ],"
       "  \"policy_user\": \"%s\""
       "}";
 
   return base::StringPrintf(kTestPolicy, dm_protocol::kChromeUserPolicyType,
                             GetTestUser());
 }
 
 }  // namespace
 
-// Tests the cloud policy stack(s).
+// Tests the cloud policy stack.
 class CloudPolicyTest : public InProcessBrowserTest {
  protected:
   CloudPolicyTest() {}
   virtual ~CloudPolicyTest() {}
 
   virtual void SetUpInProcessBrowserTestFixture() OVERRIDE {
     // The TestServer wants the docroot as a path relative to the source dir.
     base::FilePath source;
     ASSERT_TRUE(PathService::Get(base::DIR_SOURCE_ROOT, &source));
     ASSERT_TRUE(temp_dir_.CreateUniqueTempDirUnderPath(source));
@@ skipping 19 matching lines @@
     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;
     }
+  }
 
+  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);
 
 #if defined(OS_CHROMEOS)
     UserCloudPolicyManagerChromeOS* policy_manager =
         connector->GetUserCloudPolicyManager();
     ASSERT_TRUE(policy_manager);
 #else
     // 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(GetTestUser());
 
     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  // defined(OS_CHROMEOS)
 
     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);
   }
 
   void SetServerPolicy(const std::string& policy) {
     int result = file_util::WriteFile(
         temp_dir_.path().AppendASCII("device_management"),
         policy.data(), policy.size());
     ASSERT_EQ(static_cast<int>(policy.size()), result);
   }
 
   base::ScopedTempDir temp_dir_;
   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;
     policy_service->RefreshPolicies(run_loop.QuitClosure());
     run_loop.Run();
   }
 
   PolicyMap empty;
   EXPECT_TRUE(empty.Equals(policy_service->GetPolicies(
       PolicyNamespace(POLICY_DOMAIN_CHROME, std::string()))));
@@ skipping 65 matching lines @@
 
   // They should now serialize to the same bytes.
   std::string chrome_settings_serialized;
   std::string cloud_policy_serialized;
   ASSERT_TRUE(chrome_settings.SerializeToString(&chrome_settings_serialized));
   ASSERT_TRUE(cloud_policy.SerializeToString(&cloud_policy_serialized));
   EXPECT_EQ(chrome_settings_serialized, cloud_policy_serialized);
 }
 
 }  // namespace policy