Consolidate data storage and notifications in the cloud policy subsystem

chrome/browser/policy/cloud_policy_subsystem_unittest.cc

 // Copyright (c) 2011 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 <vector>
 
+#include "base/memory/scoped_ptr.h"
 #include "base/message_loop.h"
 #include "base/scoped_temp_dir.h"
 #include "base/values.h"
 #include "chrome/browser/browser_process.h"
-#include "chrome/browser/policy/cloud_policy_identity_strategy.h"
+#include "chrome/browser/policy/cloud_policy_data.h"
 #include "chrome/browser/policy/logging_work_scheduler.h"
 #include "chrome/browser/policy/mock_configuration_policy_store.h"

[Joao da Silva, 2011/07/06 16:45:14]

Nit: mock_configuration_policy_store.h not used

[gfeher, 2011/07/07 13:51:00]

Done.

 #include "chrome/browser/policy/proto/cloud_policy.pb.h"
 #include "chrome/browser/policy/proto/device_management_backend.pb.h"
 #include "chrome/browser/policy/testing_cloud_policy_subsystem.h"
 #include "chrome/browser/policy/testing_policy_url_fetcher_factory.h"
 #include "chrome/browser/policy/user_policy_cache.h"
 #include "chrome/common/pref_names.h"
 #include "chrome/test/testing_browser_process.h"
 #include "chrome/test/testing_pref_service.h"
 #include "content/browser/browser_thread.h"
 #include "content/common/url_fetcher.h"
 #include "policy/policy_constants.h"
 #include "testing/gmock/include/gmock/gmock.h"

[Joao da Silva, 2011/07/06 16:45:14]

Nit: gmock.h not used

[gfeher, 2011/07/07 13:51:00]

Done.

 #include "testing/gtest/include/gtest/gtest.h"
 
 namespace policy {
 
 using ::testing::_;
 using ::testing::AtLeast;

[Joao da Silva, 2011/07/06 16:45:14]

Nit: AtLeast not used

[gfeher, 2011/07/07 13:51:00]

Done.

 using ::testing::AtMost;
 using ::testing::DoAll;

[Joao da Silva, 2011/07/06 16:45:14]

Nit: DoAll not used

[gfeher, 2011/07/07 13:51:00]

Done.

 using ::testing::InSequence;
 using ::testing::Invoke;
 using ::testing::Return;

[Joao da Silva, 2011/07/06 16:45:14]

Nit: Invoke and Return not used

[gfeher, 2011/07/07 13:51:00]

Done.

 
 namespace em = enterprise_management;
 
 class CloudPolicySubsystemTest;

[Joao da Silva, 2011/07/06 16:45:14]

Nit: not used

[gfeher, 2011/07/07 13:51:00]

Done.

 
 namespace {
 
 const char kGaiaAuthHeader[] = "GoogleLogin auth=secret123";
 const char kDMAuthHeader[] = "GoogleDMToken token=token123456";
 const char kDMToken[] = "token123456";
 
 const char kDeviceManagementUrl[] =
     "http://localhost:12345/device_management_test";
 
-// Constant responses of the identity strategy.
-const char kMachineId[] = "dummy-cros-machine-123";
-const char kMachineModel[] = "Pony";
-const char kDeviceId[] = "abc-xyz-123";
+// Fake data to be included in requests.
 const char kUsername[] = "john@smith.com";
 const char kAuthToken[] = "secret123";
 const char kPolicyType[] = "google/chrome/test";
 
 }  // namespace
 
-// A stripped-down identity strategy for the tests.
-class TestingIdentityStrategy : public CloudPolicyIdentityStrategy {
- public:
-  TestingIdentityStrategy() {
-  }
-
-  virtual std::string GetDeviceToken() OVERRIDE {
-    return device_token_;
-  }
-
-  virtual std::string GetDeviceID() OVERRIDE {
-    return kDeviceId;
-  }
-
-  virtual std::string GetMachineID() OVERRIDE {
-    return kMachineId;
-  }
-
-  virtual std::string GetMachineModel() OVERRIDE {
-    return kMachineModel;
-  }
-
-  virtual em::DeviceRegisterRequest_Type GetPolicyRegisterType() OVERRIDE {
-    return em::DeviceRegisterRequest::USER;
-  }
-
-  virtual std::string GetPolicyType() OVERRIDE {
-    return kPolicyType;
-  }
-
-  virtual bool GetCredentials(std::string* username,
-                              std::string* auth_token) OVERRIDE {
-    *username = kUsername;
-    *auth_token = kAuthToken;;
-    return !username->empty() && !auth_token->empty();
-  }
-
-  virtual void OnDeviceTokenAvailable(const std::string& token) OVERRIDE {
-    device_token_ = token;
-    NotifyDeviceTokenChanged();
-  }
-
- private:
-  std::string device_token_;
-
-  DISALLOW_COPY_AND_ASSIGN(TestingIdentityStrategy);
-};
-
 // An action that returns an URLRequestJob with an HTTP error code.
 ACTION_P(CreateFailedResponse, http_error_code) {
   DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
   em::DeviceManagementResponse response_data;
 
   arg2->response_data = response_data.SerializeAsString();
   arg2->response_code = http_error_code;
 }
 
 // An action that returns an URLRequestJob with a successful device
@@ skipping 47 matching lines @@
   }
 
   virtual void SetUp() {
     prefs_.reset(new TestingPrefService);
     CloudPolicySubsystem::RegisterPrefs(prefs_.get());
     ((TestingBrowserProcess*) g_browser_process)->SetLocalState(prefs_.get());
 
     logger_.reset(new EventLogger);
     factory_.reset(new TestingPolicyURLFetcherFactory(logger_.get()));
     URLFetcher::set_factory(factory_.get());
-    identity_strategy_.reset(new TestingIdentityStrategy);
     ASSERT_TRUE(temp_user_data_dir_.CreateUniqueTempDir());
+    cloud_policy_data_.reset(CloudPolicyData::CreateForUserPolicies());
     cache_ = new UserPolicyCache(
         temp_user_data_dir_.path().AppendASCII("CloudPolicyControllerTest"));
     cloud_policy_subsystem_.reset(new TestingCloudPolicySubsystem(
-        identity_strategy_.get(), cache_, kDeviceManagementUrl,
-        logger_.get()));
+        cloud_policy_data_.get(), cache_,
+        kDeviceManagementUrl, logger_.get()));
     cloud_policy_subsystem_->CompleteInitialization(
         prefs::kDevicePolicyRefreshRate, 0);
   }
 
   virtual void TearDown() {
     ((TestingBrowserProcess*) g_browser_process)->SetLocalState(NULL);
     cloud_policy_subsystem_->Shutdown();
     cloud_policy_subsystem_.reset();
+    cloud_policy_data_.reset();
     factory_.reset();
     logger_.reset();
     prefs_.reset();
   }
 
   void ExecuteTest() {
     DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
     // The first unexpected request of the policy subsystem will stop the
     // message loop.
     // This code relies on the fact that an InSequence object is active.
     EXPECT_CALL(*(factory_.get()), Intercept(_, _, _))
        .Times(AtMost(1))
        .WillOnce(InvokeWithoutArgs(
            this,
            &CloudPolicySubsystemTestBase::StopMessageLoop));
+    cloud_policy_data_->set_user_name(kUsername);
+    cloud_policy_data_->SetGaiaToken(kAuthToken);
+    cloud_policy_data_->SetDeviceToken("", true);
     loop_.RunAllPending();
   }
 
   void VerifyTest(const std::string& homepage_location) {
     // Test conditions.
     EXPECT_EQ(CloudPolicySubsystem::SUCCESS, cloud_policy_subsystem_->state());
     StringValue homepage_value(homepage_location);
     VerifyPolicy(kPolicyHomepageLocation, &homepage_value);
     VerifyServerLoad();
   }
@@ skipping 67 matching lines @@
         << "No enough requests were fired during the test run.";
   }
 
   ScopedTempDir temp_user_data_dir_;
 
   MessageLoop loop_;
   BrowserThread ui_thread_;
   BrowserThread io_thread_;
 
   scoped_ptr<EventLogger> logger_;
-  scoped_ptr<TestingIdentityStrategy> identity_strategy_;
+  scoped_ptr<CloudPolicyData> cloud_policy_data_;
   scoped_ptr<CloudPolicySubsystem> cloud_policy_subsystem_;
   scoped_ptr<PrefService> prefs_;
   CloudPolicyCacheBase* cache_;
 
   scoped_ptr<TestingPolicyURLFetcherFactory> factory_;
 
   DISALLOW_COPY_AND_ASSIGN(CloudPolicySubsystemTestBase);
 };
 
 // A parameterized test case that simulates 100 failed registration attempts,
@@ skipping 34 matching lines @@
   VerifyTest("http://www.chromium.org");
 }
 
 // A random sample of error code pairs. Note that the following policy error
 // codes (401, 403, 410) make the policy subsystem to try and reregister, and
 // that is not expected in these tests.
 INSTANTIATE_TEST_CASE_P(
     CloudPolicySubsystemCombinedTestInstance,
     CloudPolicySubsystemCombinedTest,
     testing::Values(
         CombinedTestDesc(403, 400),

[pastarmovj, 2011/07/06 12:11:57]

I think you can use the constants from the policy backend here instead of
hardcoding the numbers.

[gfeher, 2011/07/06 15:14:20]

Those are actually private variables there, so this change seems to be more
difficult. Anyways, I understand your point, but I am not sure that this would
be a good idea, so let's discuss this sometimes later.

[pastarmovj, 2011/07/06 15:36:01]

My only reasoning is that these values are known to change (seldom) so it will
be great to make sure tests change along the code. But certainly this can be a
topic of another CL.

On 2011/07/06 15:14:20, gfeher wrote:

[gfeher, 2011/07/07 13:51:00]

Discussed off-line, I'll create another CL for this issue.

         CombinedTestDesc(403, 404),
         CombinedTestDesc(403, 412),
         CombinedTestDesc(403, 500),
         CombinedTestDesc(403, 503),
         CombinedTestDesc(403, 902),
         CombinedTestDesc(902, 400),
         CombinedTestDesc(503, 404),
         CombinedTestDesc(500, 412),
         CombinedTestDesc(412, 500),
         CombinedTestDesc(404, 503),
@@ skipping 77 matching lines @@
   ExecuteTest();
   VerifyTest("http://www.youtube.com");
 }
 
 INSTANTIATE_TEST_CASE_P(
     CloudPolicySubsystemPolicyReregisterTestInstance,
     CloudPolicySubsystemPolicyReregisterTest,
     testing::Values(401, 403, 410));
 
 }  // policy