Commit 01fd5c91 authored by pneubeck@chromium.org's avatar pneubeck@chromium.org

policy: Remove SigninManager from UserCloudPolicyStore.

Instead of accessing the SigninManager singleton from UserCloudPolicyStore, the username from signin is injected to UserCloudPolicyStore.

BUG=271392
(For change on Android:)

R=atwilson@chromium.org, dconnelly@chromium.org, joaodasilva@chromium.org
TBR=tedchoc@chromium.org

Review URL: https://codereview.chromium.org/49783006

git-svn-id: svn://svn.chromium.org/chrome/trunk/src@233065 0039d316-1c4b-4281-b951-d872f2087c98
parent 25e35c3f
......@@ -106,6 +106,7 @@ void SigninManagerAndroid::FetchPolicyBeforeSignIn(JNIEnv* env, jobject obj) {
policy::UserPolicySigninService* service =
policy::UserPolicySigninServiceFactory::GetForProfile(profile_);
service->FetchPolicyForSignedInUser(
username_,
cloud_policy_client_.Pass(),
base::Bind(&SigninManagerAndroid::OnPolicyFetchDone,
weak_factory_.GetWeakPtr()));
......
......@@ -7,8 +7,7 @@
namespace policy {
MockUserCloudPolicyStore::MockUserCloudPolicyStore()
: UserCloudPolicyStore(NULL,
base::FilePath(),
: UserCloudPolicyStore(base::FilePath(),
scoped_refptr<base::SequencedTaskRunner>()) {}
MockUserCloudPolicyStore::~MockUserCloudPolicyStore() {}
......
......@@ -27,6 +27,7 @@ class MockUserCloudPolicyStore : public UserCloudPolicyStore {
using CloudPolicyStore::policy_map_;
using CloudPolicyStore::policy_;
using CloudPolicyStore::status_;
using UserCloudPolicyStore::signin_username_;
private:
DISALLOW_COPY_AND_ASSIGN(MockUserCloudPolicyStore);
......
......@@ -46,6 +46,10 @@ void UserCloudPolicyManager::Shutdown() {
BrowserContextKeyedService::Shutdown();
}
void UserCloudPolicyManager::SetSigninUsername(const std::string& username) {
store_->SetSigninUsername(username);
}
void UserCloudPolicyManager::Connect(
PrefService* local_state,
scoped_refptr<net::URLRequestContextGetter> request_context,
......
......@@ -46,6 +46,8 @@ class UserCloudPolicyManager : public CloudPolicyManager,
virtual void Shutdown() OVERRIDE;
void SetSigninUsername(const std::string& username);
// Initializes the cloud connection. |local_state| must stay valid until this
// object is deleted or DisconnectAndRemovePolicy() gets called. Virtual for
// mocking.
......
......@@ -67,7 +67,7 @@ UserCloudPolicyManagerFactory::CreateManagerForOriginalProfile(
bool force_immediate_load,
scoped_refptr<base::SequencedTaskRunner> background_task_runner) {
scoped_ptr<UserCloudPolicyStore> store(
UserCloudPolicyStore::Create(profile, background_task_runner));
UserCloudPolicyStore::Create(profile->GetPath(), background_task_runner));
if (force_immediate_load)
store->LoadImmediately();
scoped_ptr<UserCloudPolicyManager> manager(
......
......@@ -10,7 +10,6 @@
#include "base/task_runner_util.h"
#include "chrome/browser/policy/proto/cloud/device_management_backend.pb.h"
#include "chrome/browser/policy/proto/cloud/device_management_local.pb.h"
#include "chrome/browser/profiles/profile.h"
#include "chrome/browser/signin/signin_manager.h"
#include "chrome/browser/signin/signin_manager_factory.h"
#include "policy/policy_constants.h"
......@@ -94,24 +93,26 @@ void StorePolicyToDiskOnBackgroundThread(
} // namespace
UserCloudPolicyStore::UserCloudPolicyStore(
Profile* profile,
const base::FilePath& path,
scoped_refptr<base::SequencedTaskRunner> background_task_runner)
: UserCloudPolicyStoreBase(background_task_runner),
weak_factory_(this),
profile_(profile),
backing_file_path_(path) {}
UserCloudPolicyStore::~UserCloudPolicyStore() {}
// static
scoped_ptr<UserCloudPolicyStore> UserCloudPolicyStore::Create(
Profile* profile,
const base::FilePath& profile_path,
scoped_refptr<base::SequencedTaskRunner> background_task_runner) {
base::FilePath path =
profile->GetPath().Append(kPolicyDir).Append(kPolicyCacheFile);
profile_path.Append(kPolicyDir).Append(kPolicyCacheFile);
return make_scoped_ptr(
new UserCloudPolicyStore(profile, path, background_task_runner));
new UserCloudPolicyStore(path, background_task_runner));
}
void UserCloudPolicyStore::SetSigninUsername(const std::string& username) {
signin_username_ = username;
}
void UserCloudPolicyStore::LoadImmediately() {
......@@ -217,17 +218,10 @@ void UserCloudPolicyStore::Validate(
scoped_ptr<UserCloudPolicyValidator> validator = CreateValidator(
policy.Pass(),
CloudPolicyValidatorBase::TIMESTAMP_NOT_BEFORE);
SigninManager* signin = SigninManagerFactory::GetForProfileIfExists(profile_);
if (signin) {
std::string username = signin->GetAuthenticatedUsername();
if (username.empty())
username = signin->GetUsernameForAuthInProgress();
// Validate the username if the user is signed in (or in the process of
// signing in).
if (!username.empty())
validator->ValidateUsername(username);
}
// Validate the username if the user is signed in.
if (!signin_username_.empty())
validator->ValidateUsername(signin_username_);
if (validate_in_background) {
// Start validation in the background. The Validator will free itself once
......
......@@ -13,8 +13,6 @@
#include "base/memory/weak_ptr.h"
#include "chrome/browser/policy/cloud/user_cloud_policy_store_base.h"
class Profile;
namespace base {
class SequencedTaskRunner;
}
......@@ -26,19 +24,22 @@ namespace policy {
// a secure storage implementation.
class UserCloudPolicyStore : public UserCloudPolicyStoreBase {
public:
// Creates a policy store associated with the user signed in to this
// |profile|.
// Creates a policy store associated with a signed-in (or in the progress of
// it) user.
UserCloudPolicyStore(
Profile* profile,
const base::FilePath& policy_file,
scoped_refptr<base::SequencedTaskRunner> background_task_runner);
virtual ~UserCloudPolicyStore();
// Factory method for creating a UserCloudPolicyStore for |profile|.
// Factory method for creating a UserCloudPolicyStore for a profile with path
// |profile_path|.
static scoped_ptr<UserCloudPolicyStore> Create(
Profile* profile,
const base::FilePath& profile_path,
scoped_refptr<base::SequencedTaskRunner> background_task_runner);
// Sets the username from signin for validation of the policy.
void SetSigninUsername(const std::string& username);
// Loads policy immediately on the current thread. Virtual for mocks.
virtual void LoadImmediately();
......@@ -51,6 +52,9 @@ class UserCloudPolicyStore : public UserCloudPolicyStoreBase {
virtual void Store(
const enterprise_management::PolicyFetchResponse& policy) OVERRIDE;
protected:
std::string signin_username_;
private:
// Callback invoked when a new policy has been loaded from disk. If
// |validate_in_background| is true, then policy is validated via a background
......@@ -77,9 +81,6 @@ class UserCloudPolicyStore : public UserCloudPolicyStoreBase {
// WeakPtrFactory used to create callbacks for validating and storing policy.
base::WeakPtrFactory<UserCloudPolicyStore> weak_factory_;
// Weak pointer to the profile associated with this store.
Profile* profile_;
// Path to file where we store persisted policy.
base::FilePath backing_file_path_;
......
......@@ -8,16 +8,10 @@
#include "base/files/scoped_temp_dir.h"
#include "base/message_loop/message_loop.h"
#include "base/message_loop/message_loop_proxy.h"
#include "base/prefs/pref_service.h"
#include "base/run_loop.h"
#include "chrome/browser/policy/cloud/mock_cloud_external_data_manager.h"
#include "chrome/browser/policy/cloud/mock_cloud_policy_store.h"
#include "chrome/browser/policy/cloud/policy_builder.h"
#include "chrome/browser/signin/fake_signin_manager.h"
#include "chrome/browser/signin/signin_manager.h"
#include "chrome/browser/signin/signin_manager_factory.h"
#include "chrome/common/pref_names.h"
#include "chrome/test/base/testing_profile.h"
#include "net/url_request/url_request_context_getter.h"
#include "policy/policy_constants.h"
#include "testing/gmock/include/gmock/gmock.h"
......@@ -40,22 +34,15 @@ void RunUntilIdle() {
class UserCloudPolicyStoreTest : public testing::Test {
public:
UserCloudPolicyStoreTest()
: loop_(base::MessageLoop::TYPE_UI),
profile_(new TestingProfile()) {}
UserCloudPolicyStoreTest() : loop_(base::MessageLoop::TYPE_UI) {}
virtual void SetUp() OVERRIDE {
ASSERT_TRUE(tmp_dir_.CreateUniqueTempDir());
SigninManager* signin = static_cast<SigninManager*>(
SigninManagerFactory::GetInstance()->SetTestingFactoryAndUse(
profile_.get(), FakeSigninManager::Build));
profile_->GetPrefs()->SetString(prefs::kGoogleServicesUsername,
PolicyBuilder::kFakeUsername);
signin->Initialize(profile_.get(), NULL);
store_.reset(new UserCloudPolicyStore(
profile_.get(), policy_file(), loop_.message_loop_proxy()));
store_.reset(
new UserCloudPolicyStore(policy_file(), loop_.message_loop_proxy()));
external_data_manager_.reset(new MockCloudExternalDataManager);
external_data_manager_->SetPolicyStore(store_.get());
store_->SetSigninUsername(PolicyBuilder::kFakeUsername);
store_->AddObserver(&observer_);
policy_.payload().mutable_passwordmanagerenabled()->set_value(true);
......@@ -104,7 +91,6 @@ class UserCloudPolicyStoreTest : public testing::Test {
// callers can use RunLoop to manage both virtual threads.
base::MessageLoop loop_;
scoped_ptr<TestingProfile> profile_;
base::ScopedTempDir tmp_dir_;
DISALLOW_COPY_AND_ASSIGN(UserCloudPolicyStoreTest);
......@@ -277,8 +263,9 @@ TEST_F(UserCloudPolicyStoreTest, StoreThenLoad) {
RunUntilIdle();
// Now, make sure the policy can be read back in from a second store.
scoped_ptr<UserCloudPolicyStore> store2(new UserCloudPolicyStore(
profile_.get(), policy_file(), loop_.message_loop_proxy()));
scoped_ptr<UserCloudPolicyStore> store2(
new UserCloudPolicyStore(policy_file(), loop_.message_loop_proxy()));
store2->SetSigninUsername(PolicyBuilder::kFakeUsername);
store2->AddObserver(&observer_);
EXPECT_CALL(observer_, OnStoreLoaded(store2.get()));
store2->Load();
......@@ -302,8 +289,9 @@ TEST_F(UserCloudPolicyStoreTest, StoreThenLoadImmediately) {
RunUntilIdle();
// Now, make sure the policy can be read back in from a second store.
scoped_ptr<UserCloudPolicyStore> store2(new UserCloudPolicyStore(
profile_.get(), policy_file(), loop_.message_loop_proxy()));
scoped_ptr<UserCloudPolicyStore> store2(
new UserCloudPolicyStore(policy_file(), loop_.message_loop_proxy()));
store2->SetSigninUsername(PolicyBuilder::kFakeUsername);
store2->AddObserver(&observer_);
EXPECT_CALL(observer_, OnStoreLoaded(store2.get()));
store2->LoadImmediately(); // Should load without running the message loop.
......@@ -338,12 +326,9 @@ TEST_F(UserCloudPolicyStoreTest, LoadValidationError) {
// Sign out, and sign back in as a different user, and try to load the profile
// data (should fail due to mismatched username).
SigninManagerFactory::GetForProfile(profile_.get())->SignOut();
SigninManagerFactory::GetForProfile(profile_.get())->SetAuthenticatedUsername(
"foobar@foobar.com");
scoped_ptr<UserCloudPolicyStore> store2(new UserCloudPolicyStore(
profile_.get(), policy_file(), loop_.message_loop_proxy()));
scoped_ptr<UserCloudPolicyStore> store2(
new UserCloudPolicyStore(policy_file(), loop_.message_loop_proxy()));
store2->SetSigninUsername("foobar@foobar.com");
store2->AddObserver(&observer_);
ExpectError(store2.get(), CloudPolicyStore::STATUS_VALIDATION_ERROR);
store2->Load();
......@@ -354,9 +339,8 @@ TEST_F(UserCloudPolicyStoreTest, LoadValidationError) {
// Sign out - we should be able to load the policy (don't check usernames
// when signed out).
SigninManagerFactory::GetForProfile(profile_.get())->SignOut();
scoped_ptr<UserCloudPolicyStore> store3(new UserCloudPolicyStore(
profile_.get(), policy_file(), loop_.message_loop_proxy()));
scoped_ptr<UserCloudPolicyStore> store3(
new UserCloudPolicyStore(policy_file(), loop_.message_loop_proxy()));
store3->AddObserver(&observer_);
EXPECT_CALL(observer_, OnStoreLoaded(store3.get()));
store3->Load();
......@@ -366,12 +350,9 @@ TEST_F(UserCloudPolicyStoreTest, LoadValidationError) {
store3->RemoveObserver(&observer_);
// Now start a signin as a different user - this should fail validation.
FakeSigninManager* signin = static_cast<FakeSigninManager*>(
SigninManagerFactory::GetForProfile(profile_.get()));
signin->set_auth_in_progress("foobar@foobar.com");
scoped_ptr<UserCloudPolicyStore> store4(new UserCloudPolicyStore(
profile_.get(), policy_file(), loop_.message_loop_proxy()));
scoped_ptr<UserCloudPolicyStore> store4(
new UserCloudPolicyStore(policy_file(), loop_.message_loop_proxy()));
store4->SetSigninUsername("foobar@foobar.com");
store4->AddObserver(&observer_);
ExpectError(store4.get(), CloudPolicyStore::STATUS_VALIDATION_ERROR);
store4->Load();
......
......@@ -120,8 +120,10 @@ void UserPolicySigninService::OnRefreshTokenAvailable(
}
void UserPolicySigninService::InitializeUserCloudPolicyManager(
const std::string& username,
scoped_ptr<CloudPolicyClient> client) {
UserPolicySigninServiceBase::InitializeUserCloudPolicyManager(client.Pass());
UserPolicySigninServiceBase::InitializeUserCloudPolicyManager(username,
client.Pass());
ProhibitSignoutIfNeeded();
}
......
......@@ -59,6 +59,7 @@ class UserPolicySigninService : public UserPolicySigninServiceBase,
protected:
// UserPolicySigninServiceBase implementation:
virtual void InitializeUserCloudPolicyManager(
const std::string& username,
scoped_ptr<CloudPolicyClient> client) OVERRIDE;
virtual void PrepareForUserCloudPolicyManagerShutdown() OVERRIDE;
......
......@@ -45,6 +45,7 @@ UserPolicySigninServiceBase::UserPolicySigninServiceBase(
UserPolicySigninServiceBase::~UserPolicySigninServiceBase() {}
void UserPolicySigninServiceBase::FetchPolicyForSignedInUser(
const std::string& username,
scoped_ptr<CloudPolicyClient> client,
const PolicyFetchCallback& callback) {
DCHECK(client);
......@@ -56,7 +57,7 @@ void UserPolicySigninServiceBase::FetchPolicyForSignedInUser(
UserCloudPolicyManager* manager = GetManager();
DCHECK(manager);
DCHECK(!manager->core()->client());
InitializeUserCloudPolicyManager(client.Pass());
InitializeUserCloudPolicyManager(username, client.Pass());
DCHECK(manager->IsClientRegistered());
// Now initiate a policy fetch.
......@@ -159,6 +160,8 @@ scoped_ptr<CloudPolicyClient> UserPolicySigninServiceBase::PrepareToRegister(
return scoped_ptr<CloudPolicyClient>();
}
GetManager()->SetSigninUsername(username);
// If the DeviceManagementService is not yet initialized, start it up now.
device_management_service_->ScheduleInitialization(0);
......@@ -201,8 +204,11 @@ void UserPolicySigninServiceBase::InitializeForSignedInUser(
// OnInitializationCompleted() callback is invoked and this will
// initiate a policy fetch.
InitializeUserCloudPolicyManager(
username,
UserCloudPolicyManager::CreateCloudPolicyClient(
device_management_service_).Pass());
} else {
manager->SetSigninUsername(username);
}
// If the CloudPolicyService is initialized, kick off registration.
......@@ -213,8 +219,10 @@ void UserPolicySigninServiceBase::InitializeForSignedInUser(
}
void UserPolicySigninServiceBase::InitializeUserCloudPolicyManager(
const std::string& username,
scoped_ptr<CloudPolicyClient> client) {
UserCloudPolicyManager* manager = GetManager();
manager->SetSigninUsername(username);
DCHECK(!manager->core()->client());
manager->Connect(local_state_, request_context_, client.Pass());
DCHECK(manager->core()->service());
......
......@@ -70,7 +70,8 @@ class UserPolicySigninServiceBase : public BrowserContextKeyedService,
// previously initialized via RegisterPolicyClient. |callback| is invoked
// once the policy fetch is complete, passing true if the policy fetch
// succeeded.
void FetchPolicyForSignedInUser(scoped_ptr<CloudPolicyClient> client,
void FetchPolicyForSignedInUser(const std::string& username,
scoped_ptr<CloudPolicyClient> client,
const PolicyFetchCallback& callback);
// content::NotificationObserver implementation:
......@@ -121,6 +122,7 @@ class UserPolicySigninServiceBase : public BrowserContextKeyedService,
// signed in account at startup, and from FetchPolicyForSignedInUser() during
// the initial policy fetch after signing in.
virtual void InitializeUserCloudPolicyManager(
const std::string& username,
scoped_ptr<CloudPolicyClient> client);
// Prepares for the UserCloudPolicyManager to be shutdown due to
......
......@@ -256,7 +256,9 @@ class UserPolicySigninServiceTest : public testing::Test {
.WillOnce(device_management_service_.CreateAsyncJob(&fetch_request));
EXPECT_CALL(device_management_service_, StartJob(_, _, _, _, _, _, _))
.Times(1);
signin_service->FetchPolicyForSignedInUser(
kTestUser,
created_client_.Pass(),
base::Bind(&UserPolicySigninServiceTest::OnPolicyRefresh,
base::Unretained(this)));
......@@ -265,6 +267,7 @@ class UserPolicySigninServiceTest : public testing::Test {
ASSERT_TRUE(fetch_request);
// UserCloudPolicyManager should now be initialized.
EXPECT_EQ(mock_store_->signin_username_, kTestUser);
ASSERT_TRUE(manager_->core()->service());
// Make the policy fetch succeed - this should result in a write to the
......@@ -359,6 +362,7 @@ TEST_F(UserPolicySigninServiceTest, InitWhileSignedIn) {
GetTokenService()->IssueRefreshToken("oauth_login_refresh_token");
// Client registration should be in progress since we now have an oauth token.
EXPECT_EQ(mock_store_->signin_username_, kTestUser);
ASSERT_TRUE(IsRequestActive());
}
......@@ -418,6 +422,7 @@ TEST_F(UserPolicySigninServiceTest, SignInAfterInit) {
GetTokenService()->IssueRefreshToken("oauth_login_refresh_token");
// UserCloudPolicyManager should be initialized.
EXPECT_EQ(mock_store_->signin_username_, kTestUser);
ASSERT_TRUE(manager_->core()->service());
// Client registration should be in progress since we have an oauth token.
......@@ -472,6 +477,7 @@ TEST_F(UserPolicySigninServiceTest, UnregisteredClient) {
GetTokenService()->IssueRefreshToken("oauth_login_refresh_token");
// UserCloudPolicyManager should be initialized.
EXPECT_EQ(mock_store_->signin_username_, kTestUser);
ASSERT_TRUE(manager_->core()->service());
// Client registration should not be in progress since the store is not
......@@ -505,6 +511,7 @@ TEST_F(UserPolicySigninServiceTest, RegisteredClient) {
GetTokenService()->IssueRefreshToken("oauth_login_refresh_token");
// UserCloudPolicyManager should be initialized.
EXPECT_EQ(mock_store_->signin_username_, kTestUser);
ASSERT_TRUE(manager_->core()->service());
// Client registration should not be in progress since the store is not
......@@ -529,6 +536,7 @@ TEST_F(UserPolicySigninServiceTest, RegisteredClient) {
TEST_F(UserPolicySigninServiceTest, SignOutAfterInit) {
EXPECT_CALL(*mock_store_, Clear());
// Set the user as signed in.
SigninManagerFactory::GetForProfile(profile_.get())->SetAuthenticatedUsername(
kTestUser);
......@@ -540,6 +548,7 @@ TEST_F(UserPolicySigninServiceTest, SignOutAfterInit) {
content::NotificationService::NoDetails());
// UserCloudPolicyManager should be initialized.
EXPECT_EQ(mock_store_->signin_username_, kTestUser);
ASSERT_TRUE(manager_->core()->service());
// Now sign out.
......@@ -700,6 +709,7 @@ TEST_F(UserPolicySigninServiceTest, FetchPolicyFailed) {
UserPolicySigninService* signin_service =
UserPolicySigninServiceFactory::GetForProfile(profile_.get());
signin_service->FetchPolicyForSignedInUser(
kTestUser,
client.Pass(),
base::Bind(&UserPolicySigninServiceTest::OnPolicyRefresh,
base::Unretained(this)));
......@@ -709,7 +719,9 @@ TEST_F(UserPolicySigninServiceTest, FetchPolicyFailed) {
EXPECT_CALL(*this, OnPolicyRefresh(false)).Times(1);
fetch_request->SendResponse(DM_STATUS_REQUEST_FAILED,
em::DeviceManagementResponse());
// UserCloudPolicyManager should be initialized.
EXPECT_EQ(mock_store_->signin_username_, kTestUser);
ASSERT_TRUE(manager_->core()->service());
}
......
......@@ -201,9 +201,11 @@ void OneClickSigninSyncStarter::OnRegisteredForPolicy(
void OneClickSigninSyncStarter::LoadPolicyWithCachedClient() {
DCHECK(policy_client_);
SigninManager* signin = SigninManagerFactory::GetForProfile(profile_);
policy::UserPolicySigninService* policy_service =
policy::UserPolicySigninServiceFactory::GetForProfile(profile_);
policy_service->FetchPolicyForSignedInUser(
signin->GetUsernameForAuthInProgress(),
policy_client_.Pass(),
base::Bind(&OneClickSigninSyncStarter::OnPolicyFetchComplete,
weak_pointer_factory_.GetWeakPtr()));
......
Markdown is supported
0%
or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment