diff options
author | atwilson@chromium.org <atwilson@chromium.org@0039d316-1c4b-4281-b951-d872f2087c98> | 2013-08-22 10:49:39 +0000 |
---|---|---|
committer | atwilson@chromium.org <atwilson@chromium.org@0039d316-1c4b-4281-b951-d872f2087c98> | 2013-08-22 10:49:39 +0000 |
commit | 9819fd0996f70706bc4efd21d2a3f4bd172deda8 (patch) | |
tree | 2fb1920ddd4c8623217cedc5e189fdd84bd31daa | |
parent | 9fec8755f9af45d998a729d655892fedc14eb3a7 (diff) | |
download | chromium_src-9819fd0996f70706bc4efd21d2a3f4bd172deda8.zip chromium_src-9819fd0996f70706bc4efd21d2a3f4bd172deda8.tar.gz chromium_src-9819fd0996f70706bc4efd21d2a3f4bd172deda8.tar.bz2 |
Convert UserPolicySigninService to use OAuth2TokenService
Updated UserPolicySigninService to use OAuth2TokenService and refactored the
Android code to allow the android implementation to be shared with desktop.
Added a FakeProfileOAuth2TokenService and added associated refactorings of
OAuth2TokenService to support it.
Updated various unit tests to use new FakeProfileOAuth2TokenService instead of
rolling their own mocks.
Updated TestingProfile::Builder to enable setting testing factories before any
services are created - this is required because UserPolicySigninService is
instantiated at profile creation time, and this means there is no way to inject
a custom instance.
Updated TestingProfile::Builder to support setting a TestingProfile as incognito
at construction time. This is required because otherwise you have a situation
where services will be created at profile creation time, that should not exist
for incognito profiles.
BUG=265831
TBR=estade,isherman
Review URL: https://chromiumcodereview.appspot.com/23068005
git-svn-id: svn://svn.chromium.org/chrome/trunk/src@218990 0039d316-1c4b-4281-b951-d872f2087c98
62 files changed, 1029 insertions, 646 deletions
diff --git a/chrome/browser/chromeos/policy/user_cloud_policy_manager_chromeos_unittest.cc b/chrome/browser/chromeos/policy/user_cloud_policy_manager_chromeos_unittest.cc index eb1f266..9d2056d 100644 --- a/chrome/browser/chromeos/policy/user_cloud_policy_manager_chromeos_unittest.cc +++ b/chrome/browser/chromeos/policy/user_cloud_policy_manager_chromeos_unittest.cc @@ -91,11 +91,11 @@ class UserCloudPolicyManagerChromeOSTest : public testing::Test { chrome::kInitialProfile, scoped_ptr<PrefServiceSyncable>(), UTF8ToUTF16("testing_profile"), 0, std::string()); signin_profile_ = profile_manager_->CreateTestingProfile(kSigninProfile); - signin_profile_->set_incognito(true); + signin_profile_->ForceIncognito(true); // Usually the signin Profile and the main Profile are separate, but since // the signin Profile is an OTR Profile then for this test it suffices to // attach it to the main Profile. - profile_->SetOffTheRecordProfile(signin_profile_); + profile_->SetOffTheRecordProfile(scoped_ptr<Profile>(signin_profile_)); signin_profile_->SetOriginalProfile(profile_); ASSERT_EQ(signin_profile_, chromeos::ProfileHelper::GetSigninProfile()); diff --git a/chrome/browser/chromeos/power/power_prefs_unittest.cc b/chrome/browser/chromeos/power/power_prefs_unittest.cc index 6b3cd2d..7a28e04 100644 --- a/chrome/browser/chromeos/power/power_prefs_unittest.cc +++ b/chrome/browser/chromeos/power/power_prefs_unittest.cc @@ -152,17 +152,19 @@ TEST_F(PowerPrefsTest, LoginScreen) { scoped_ptr<TestingPrefServiceSyncable> login_profile_prefs( new TestingPrefServiceSyncable); chrome::RegisterLoginProfilePrefs(login_profile_prefs->registry()); - scoped_ptr<TestingProfile> login_profile_owner(new TestingProfile( - profile_manager_.profiles_dir().AppendASCII(chrome::kInitialProfile), - NULL, - scoped_refptr<ExtensionSpecialStoragePolicy>(), - scoped_ptr<PrefServiceSyncable>(login_profile_prefs.release()))); + TestingProfile::Builder builder; + builder.SetPath( + profile_manager_.profiles_dir().AppendASCII(chrome::kInitialProfile)); + builder.SetPrefService(login_profile_prefs.PassAs<PrefServiceSyncable>()); + builder.SetIncognito(); + scoped_ptr<TestingProfile> login_profile_owner(builder.Build()); + TestingProfile* login_profile = login_profile_owner.get(); TestingProfile* login_profile_parent = profile_manager_.CreateTestingProfile( chrome::kInitialProfile); - login_profile_parent->SetOffTheRecordProfile(login_profile_owner.release()); + login_profile_parent->SetOffTheRecordProfile( + login_profile_owner.PassAs<Profile>()); login_profile->SetOriginalProfile(login_profile_parent); - login_profile->set_incognito(true); // Inform power_prefs_ that the login screen is being shown. power_prefs_->Observe(chrome::NOTIFICATION_LOGIN_OR_LOCK_WEBUI_VISIBLE, diff --git a/chrome/browser/chromeos/settings/device_oauth2_token_service.cc b/chrome/browser/chromeos/settings/device_oauth2_token_service.cc index adcf750..dbcf249 100644 --- a/chrome/browser/chromeos/settings/device_oauth2_token_service.cc +++ b/chrome/browser/chromeos/settings/device_oauth2_token_service.cc @@ -220,10 +220,6 @@ DeviceOAuth2TokenService::DeviceOAuth2TokenService( DeviceOAuth2TokenService::~DeviceOAuth2TokenService() { } -net::URLRequestContextGetter* DeviceOAuth2TokenService::GetRequestContext() { - return url_request_context_getter_.get(); -} - scoped_ptr<OAuth2TokenService::Request> DeviceOAuth2TokenService::StartRequest( const OAuth2TokenService::ScopeSet& scopes, OAuth2TokenService::Consumer* consumer) { @@ -283,4 +279,8 @@ std::string DeviceOAuth2TokenService::GetRobotAccountId() { return std::string(); } +net::URLRequestContextGetter* DeviceOAuth2TokenService::GetRequestContext() { + return url_request_context_getter_.get(); +} + } // namespace chromeos diff --git a/chrome/browser/chromeos/settings/device_oauth2_token_service.h b/chrome/browser/chromeos/settings/device_oauth2_token_service.h index ca6f41e..24f8529 100644 --- a/chrome/browser/chromeos/settings/device_oauth2_token_service.h +++ b/chrome/browser/chromeos/settings/device_oauth2_token_service.h @@ -54,6 +54,9 @@ class DeviceOAuth2TokenService : public OAuth2TokenService { // Pull the robot account ID from device policy. virtual std::string GetRobotAccountId(); + // Implementation of OAuth2TokenService. + virtual net::URLRequestContextGetter* GetRequestContext() OVERRIDE; + private: class ValidatingConsumer; friend class ValidatingConsumer; @@ -66,9 +69,6 @@ class DeviceOAuth2TokenService : public OAuth2TokenService { PrefService* local_state); virtual ~DeviceOAuth2TokenService(); - // Implementation of OAuth2TokenService. - virtual net::URLRequestContextGetter* GetRequestContext() OVERRIDE; - void OnValidationComplete(bool token_is_valid); bool refresh_token_is_valid_; diff --git a/chrome/browser/content_settings/content_settings_pref_provider_unittest.cc b/chrome/browser/content_settings/content_settings_pref_provider_unittest.cc index b1c8a62..3646541 100644 --- a/chrome/browser/content_settings/content_settings_pref_provider_unittest.cc +++ b/chrome/browser/content_settings/content_settings_pref_provider_unittest.cc @@ -154,11 +154,10 @@ TEST_F(PrefProviderTest, Incognito) { scoped_ptr<TestingProfile> profile = profile_builder.Build(); TestingProfile::Builder otr_profile_builder; + otr_profile_builder.SetIncognito(); otr_profile_builder.SetPrefService(make_scoped_ptr(otr_prefs)); - TestingProfile* otr_profile = otr_profile_builder.Build().release(); - - otr_profile->set_incognito(true); - profile->SetOffTheRecordProfile(otr_profile); + scoped_ptr<TestingProfile> otr_profile(otr_profile_builder.Build()); + profile->SetOffTheRecordProfile(otr_profile.PassAs<Profile>()); PrefProvider pref_content_settings_provider(regular_prefs, false); PrefProvider pref_content_settings_provider_incognito(otr_prefs, true); diff --git a/chrome/browser/extensions/api/cookies/cookies_unittest.cc b/chrome/browser/extensions/api/cookies/cookies_unittest.cc index 764e028..b5fda79 100644 --- a/chrome/browser/extensions/api/cookies/cookies_unittest.cc +++ b/chrome/browser/extensions/api/cookies/cookies_unittest.cc @@ -33,82 +33,50 @@ struct DomainMatchCase { const bool matches; }; -// A test profile that supports linking with another profile for incognito -// support. -class OtrTestingProfile : public TestingProfile { - public: - OtrTestingProfile() : linked_profile_(NULL) {} - virtual Profile* GetOriginalProfile() OVERRIDE { - if (IsOffTheRecord()) - return linked_profile_; - else - return this; - } - - virtual Profile* GetOffTheRecordProfile() OVERRIDE { - if (IsOffTheRecord()) - return this; - else - return linked_profile_; - } - - virtual bool HasOffTheRecordProfile() OVERRIDE { - return (!IsOffTheRecord() && linked_profile_); - } - - static void LinkProfiles(OtrTestingProfile* profile1, - OtrTestingProfile* profile2) { - profile1->set_linked_profile(profile2); - profile2->set_linked_profile(profile1); - } - - void set_linked_profile(OtrTestingProfile* profile) { - linked_profile_ = profile; - } - - private: - OtrTestingProfile* linked_profile_; -}; - } // namespace class ExtensionCookiesTest : public testing::Test { }; TEST_F(ExtensionCookiesTest, StoreIdProfileConversion) { - OtrTestingProfile profile, otrProfile; - otrProfile.set_incognito(true); - OtrTestingProfile::LinkProfiles(&profile, &otrProfile); + TestingProfile::Builder profile_builder; + TestingProfile::Builder otr_profile_builder; + otr_profile_builder.SetIncognito(); + scoped_ptr<TestingProfile> profile = profile_builder.Build(); + scoped_ptr<TestingProfile> otr_profile = otr_profile_builder.Build(); + otr_profile->SetOriginalProfile(profile.get()); + profile->SetOffTheRecordProfile(otr_profile.PassAs<Profile>()); EXPECT_EQ(std::string("0"), - cookies_helpers::GetStoreIdFromProfile(&profile)); - EXPECT_EQ(&profile, + cookies_helpers::GetStoreIdFromProfile(profile.get())); + EXPECT_EQ(profile.get(), cookies_helpers::ChooseProfileFromStoreId( - "0", &profile, true)); - EXPECT_EQ(&profile, + "0", profile.get(), true)); + EXPECT_EQ(profile.get(), cookies_helpers::ChooseProfileFromStoreId( - "0", &profile, false)); - EXPECT_EQ(&otrProfile, + "0", profile.get(), false)); + EXPECT_EQ(profile->GetOffTheRecordProfile(), cookies_helpers::ChooseProfileFromStoreId( - "1", &profile, true)); + "1", profile.get(), true)); EXPECT_EQ(NULL, cookies_helpers::ChooseProfileFromStoreId( - "1", &profile, false)); + "1", profile.get(), false)); EXPECT_EQ(std::string("1"), - cookies_helpers::GetStoreIdFromProfile(&otrProfile)); + cookies_helpers::GetStoreIdFromProfile( + profile->GetOffTheRecordProfile())); EXPECT_EQ(NULL, cookies_helpers::ChooseProfileFromStoreId( - "0", &otrProfile, true)); + "0", profile->GetOffTheRecordProfile(), true)); EXPECT_EQ(NULL, cookies_helpers::ChooseProfileFromStoreId( - "0", &otrProfile, false)); - EXPECT_EQ(&otrProfile, + "0", profile->GetOffTheRecordProfile(), false)); + EXPECT_EQ(profile->GetOffTheRecordProfile(), cookies_helpers::ChooseProfileFromStoreId( - "1", &otrProfile, true)); - EXPECT_EQ(&otrProfile, + "1", profile->GetOffTheRecordProfile(), true)); + EXPECT_EQ(profile->GetOffTheRecordProfile(), cookies_helpers::ChooseProfileFromStoreId( - "1", &otrProfile, false)); + "1", profile->GetOffTheRecordProfile(), false)); } TEST_F(ExtensionCookiesTest, ExtensionTypeCreation) { diff --git a/chrome/browser/extensions/event_router_forwarder_unittest.cc b/chrome/browser/extensions/event_router_forwarder_unittest.cc index 0e6619a..865d74b 100644 --- a/chrome/browser/extensions/event_router_forwarder_unittest.cc +++ b/chrome/browser/extensions/event_router_forwarder_unittest.cc @@ -111,10 +111,13 @@ class EventRouterForwarderTest : public testing::Test { } TestingProfile* CreateIncognitoProfile(TestingProfile* base) { - TestingProfile* incognito = new TestingProfile; // Owned by |base|. - incognito->set_incognito(true); - base->SetOffTheRecordProfile(incognito); - return incognito; + TestingProfile::Builder builder; + builder.SetIncognito(); + scoped_ptr<TestingProfile> incognito = builder.Build(); + TestingProfile* incognito_ptr = incognito.get(); + // Incognito profile now owned by |base| + base->SetOffTheRecordProfile(incognito.PassAs<Profile>()); + return incognito_ptr; } base::MessageLoopForUI message_loop_; diff --git a/chrome/browser/extensions/menu_manager_unittest.cc b/chrome/browser/extensions/menu_manager_unittest.cc index 80e7c60..5fef312 100644 --- a/chrome/browser/extensions/menu_manager_unittest.cc +++ b/chrome/browser/extensions/menu_manager_unittest.cc @@ -86,10 +86,10 @@ class MenuManagerTest : public testing::Test { } protected: - TestingProfile profile_; base::MessageLoopForUI message_loop_; content::TestBrowserThread ui_thread_; content::TestBrowserThread file_thread_; + TestingProfile profile_; MenuManager manager_; ExtensionList extensions_; diff --git a/chrome/browser/managed_mode/managed_user_refresh_token_fetcher_unittest.cc b/chrome/browser/managed_mode/managed_user_refresh_token_fetcher_unittest.cc index 75608a6..fb79edc 100644 --- a/chrome/browser/managed_mode/managed_user_refresh_token_fetcher_unittest.cc +++ b/chrome/browser/managed_mode/managed_user_refresh_token_fetcher_unittest.cc @@ -6,6 +6,7 @@ #include "base/memory/scoped_ptr.h" #include "base/strings/stringprintf.h" #include "chrome/browser/managed_mode/managed_user_refresh_token_fetcher.h" +#include "chrome/browser/signin/fake_profile_oauth2_token_service.h" #include "chrome/browser/signin/oauth2_token_service.h" #include "chrome/test/base/testing_profile.h" #include "content/public/test/test_browser_thread_bundle.h" @@ -28,6 +29,7 @@ const char kDeviceName[] = "Compy"; const char kAccessToken[] = "accesstoken"; const char kAuthorizationCode[] = "authorizationcode"; const char kManagedUserToken[] = "managedusertoken"; +const char kOAuth2RefreshToken[] = "refreshtoken"; const char kIssueTokenResponseFormat[] = "{" @@ -41,100 +43,6 @@ const char kGetRefreshTokenResponseFormat[] = " \"refresh_token\": \"%s\"" "}"; -// MockOAuth2TokenService --------------------------------------------- - -class MockOAuth2TokenService : public OAuth2TokenService { - public: - class Request : public OAuth2TokenService::Request { - public: - Request(const OAuth2TokenService::ScopeSet& scopes, - OAuth2TokenService::Consumer* consumer, - MockOAuth2TokenService* owner); - virtual ~Request(); - - void Succeed(); - void Fail(GoogleServiceAuthError::State error); - - const OAuth2TokenService::ScopeSet& scopes() const { return scopes_; } - - private: - OAuth2TokenService::ScopeSet scopes_; - - OAuth2TokenService::Consumer* consumer_; - - MockOAuth2TokenService* owner_; - }; - - MockOAuth2TokenService(); - virtual ~MockOAuth2TokenService(); - - Request* request() const { return request_; } - - void ClearRequest(Request* request); - - private: - // OAuth2TokenService overrides: - virtual scoped_ptr<OAuth2TokenService::Request> StartRequest( - const OAuth2TokenService::ScopeSet& scopes, - OAuth2TokenService::Consumer* consumer) OVERRIDE; - virtual std::string GetRefreshToken() OVERRIDE; - virtual net::URLRequestContextGetter* GetRequestContext() OVERRIDE { - return NULL; - } - - Request* request_; - - DISALLOW_COPY_AND_ASSIGN(MockOAuth2TokenService); -}; - -MockOAuth2TokenService::Request::Request( - const OAuth2TokenService::ScopeSet& scopes, - OAuth2TokenService::Consumer* consumer, - MockOAuth2TokenService* owner) - : scopes_(scopes), - consumer_(consumer), - owner_(owner) {} - -MockOAuth2TokenService::Request::~Request() { - owner_->ClearRequest(this); -} - -void MockOAuth2TokenService::Request::Succeed() { - base::Time expiration_date = base::Time::Now() + - base::TimeDelta::FromHours(1); - consumer_->OnGetTokenSuccess(this, kAccessToken, expiration_date); -} - -void MockOAuth2TokenService::Request::Fail( - GoogleServiceAuthError::State error) { - consumer_->OnGetTokenFailure(this, GoogleServiceAuthError(error)); -} - -MockOAuth2TokenService::MockOAuth2TokenService() : request_(NULL) {} - -MockOAuth2TokenService::~MockOAuth2TokenService() { - EXPECT_FALSE(request_); -} - -void MockOAuth2TokenService::ClearRequest( - MockOAuth2TokenService::Request* request) { - if (request_ == request) - request_ = NULL; -} - -scoped_ptr<OAuth2TokenService::Request> MockOAuth2TokenService::StartRequest( - const OAuth2TokenService::ScopeSet& scopes, - OAuth2TokenService::Consumer* consumer) { - scoped_ptr<Request> request(new Request(scopes, consumer, this)); - request_ = request.get(); - return request.PassAs<OAuth2TokenService::Request>(); -} - -std::string MockOAuth2TokenService::GetRefreshToken() { - NOTREACHED(); - return std::string(); -} - // Utility methods -------------------------------------------------- // Slightly hacky way to extract a value from a URL-encoded POST request body. @@ -166,6 +74,14 @@ void SetHttpError(net::TestURLFetcher* url_fetcher, int error) { url_fetcher->delegate()->OnURLFetchComplete(url_fetcher); } +void VerifyTokenRequest( + std::vector<FakeProfileOAuth2TokenService::PendingRequest> requests) { + ASSERT_EQ(1u, requests.size()); + EXPECT_EQ(1u, requests[0].scopes.size()); + EXPECT_EQ(1u, requests[0].scopes.count( + GaiaUrls::GetInstance()->oauth1_login_scope())); +} + } // namespace class ManagedUserRefreshTokenFetcherTest : public testing::Test { @@ -176,10 +92,11 @@ class ManagedUserRefreshTokenFetcherTest : public testing::Test { protected: void StartFetching(); - MockOAuth2TokenService::Request* GetOAuth2TokenServiceRequest(); net::TestURLFetcher* GetIssueTokenRequest(); net::TestURLFetcher* GetRefreshTokenRequest(); + void MakeOAuth2TokenServiceRequestSucceed(); + void MakeOAuth2TokenServiceRequestFail(GoogleServiceAuthError::State error); void MakeIssueTokenRequestSucceed(); void MakeRefreshTokenFetchSucceed(); @@ -194,7 +111,7 @@ class ManagedUserRefreshTokenFetcherTest : public testing::Test { content::TestBrowserThreadBundle thread_bundle_; TestingProfile profile_; - MockOAuth2TokenService oauth2_token_service_; + FakeProfileOAuth2TokenService oauth2_token_service_; net::TestURLFetcherFactory url_fetcher_factory_; scoped_ptr<ManagedUserRefreshTokenFetcher> token_fetcher_; @@ -211,22 +128,13 @@ ManagedUserRefreshTokenFetcherTest::ManagedUserRefreshTokenFetcherTest() weak_ptr_factory_(this) {} void ManagedUserRefreshTokenFetcherTest::StartFetching() { + oauth2_token_service_.IssueRefreshToken(kOAuth2RefreshToken); token_fetcher_->Start(kManagedUserId, kDeviceName, base::Bind( &ManagedUserRefreshTokenFetcherTest::OnTokenFetched, weak_ptr_factory_.GetWeakPtr())); } -MockOAuth2TokenService::Request* -ManagedUserRefreshTokenFetcherTest::GetOAuth2TokenServiceRequest() { - MockOAuth2TokenService::Request* request = oauth2_token_service_.request(); - - OAuth2TokenService::ScopeSet scopes = request->scopes(); - EXPECT_EQ(1u, scopes.size()); - EXPECT_EQ(1u, scopes.count(GaiaUrls::GetInstance()->oauth1_login_scope())); - return request; -} - net::TestURLFetcher* ManagedUserRefreshTokenFetcherTest::GetIssueTokenRequest() { net::TestURLFetcher* url_fetcher = url_fetcher_factory_.GetFetcherByID(1); @@ -265,6 +173,28 @@ ManagedUserRefreshTokenFetcherTest::GetRefreshTokenRequest() { return url_fetcher; } +void +ManagedUserRefreshTokenFetcherTest::MakeOAuth2TokenServiceRequestSucceed() { + std::vector<FakeProfileOAuth2TokenService::PendingRequest> requests = + oauth2_token_service_.GetPendingRequests(); + VerifyTokenRequest(requests); + base::Time expiration_date = base::Time::Now() + + base::TimeDelta::FromHours(1); + oauth2_token_service_.IssueTokenForScope(requests[0].scopes, + kAccessToken, + expiration_date); +} + +void +ManagedUserRefreshTokenFetcherTest::MakeOAuth2TokenServiceRequestFail( + GoogleServiceAuthError::State error) { + std::vector<FakeProfileOAuth2TokenService::PendingRequest> requests = + oauth2_token_service_.GetPendingRequests(); + VerifyTokenRequest(requests); + oauth2_token_service_.IssueErrorForScope(requests[0].scopes, + GoogleServiceAuthError(error)); +} + void ManagedUserRefreshTokenFetcherTest::MakeIssueTokenRequestSucceed() { SendResponse(GetIssueTokenRequest(), base::StringPrintf(kIssueTokenResponseFormat, @@ -292,7 +222,7 @@ void ManagedUserRefreshTokenFetcherTest::OnTokenFetched( TEST_F(ManagedUserRefreshTokenFetcherTest, Success) { StartFetching(); - GetOAuth2TokenServiceRequest()->Succeed(); + MakeOAuth2TokenServiceRequestSucceed(); MakeIssueTokenRequestSucceed(); MakeRefreshTokenFetchSucceed(); @@ -302,9 +232,9 @@ TEST_F(ManagedUserRefreshTokenFetcherTest, Success) { TEST_F(ManagedUserRefreshTokenFetcherTest, ExpiredAccessToken) { StartFetching(); - GetOAuth2TokenServiceRequest()->Succeed(); + MakeOAuth2TokenServiceRequestSucceed(); SetHttpError(GetIssueTokenRequest(), net::HTTP_UNAUTHORIZED); - GetOAuth2TokenServiceRequest()->Succeed(); + MakeOAuth2TokenServiceRequestSucceed(); MakeIssueTokenRequestSucceed(); MakeRefreshTokenFetchSucceed(); @@ -316,9 +246,9 @@ TEST_F(ManagedUserRefreshTokenFetcherTest, ExpiredAccessTokenRetry) { // If we get a 401 error for the second time, we should give up instead of // retrying again. StartFetching(); - GetOAuth2TokenServiceRequest()->Succeed(); + MakeOAuth2TokenServiceRequestSucceed(); SetHttpError(GetIssueTokenRequest(), net::HTTP_UNAUTHORIZED); - GetOAuth2TokenServiceRequest()->Succeed(); + MakeOAuth2TokenServiceRequestSucceed(); SetHttpError(GetIssueTokenRequest(), net::HTTP_UNAUTHORIZED); EXPECT_EQ(GoogleServiceAuthError::CONNECTION_FAILED, error().state()); @@ -328,7 +258,7 @@ TEST_F(ManagedUserRefreshTokenFetcherTest, ExpiredAccessTokenRetry) { TEST_F(ManagedUserRefreshTokenFetcherTest, MalformedIssueTokenResponse) { StartFetching(); - GetOAuth2TokenServiceRequest()->Succeed(); + MakeOAuth2TokenServiceRequestSucceed(); SendResponse(GetIssueTokenRequest(), "choke"); EXPECT_EQ(GoogleServiceAuthError::CONNECTION_FAILED, error().state()); @@ -338,7 +268,7 @@ TEST_F(ManagedUserRefreshTokenFetcherTest, MalformedIssueTokenResponse) { TEST_F(ManagedUserRefreshTokenFetcherTest, FetchAccessTokenFailure) { StartFetching(); - GetOAuth2TokenServiceRequest()->Fail( + MakeOAuth2TokenServiceRequestFail( GoogleServiceAuthError::INVALID_GAIA_CREDENTIALS); EXPECT_EQ(GoogleServiceAuthError::INVALID_GAIA_CREDENTIALS, error().state()); @@ -347,7 +277,7 @@ TEST_F(ManagedUserRefreshTokenFetcherTest, FetchAccessTokenFailure) { TEST_F(ManagedUserRefreshTokenFetcherTest, IssueTokenNetworkError) { StartFetching(); - GetOAuth2TokenServiceRequest()->Succeed(); + MakeOAuth2TokenServiceRequestSucceed(); SetNetworkError(GetIssueTokenRequest(), net::ERR_SSL_PROTOCOL_ERROR); EXPECT_EQ(GoogleServiceAuthError::CONNECTION_FAILED, error().state()); @@ -357,7 +287,7 @@ TEST_F(ManagedUserRefreshTokenFetcherTest, IssueTokenNetworkError) { TEST_F(ManagedUserRefreshTokenFetcherTest, FetchRefreshTokenNetworkError) { StartFetching(); - GetOAuth2TokenServiceRequest()->Succeed(); + MakeOAuth2TokenServiceRequestSucceed(); MakeIssueTokenRequestSucceed(); SetNetworkError(GetRefreshTokenRequest(), net::ERR_CONNECTION_REFUSED); EXPECT_EQ(GoogleServiceAuthError::NONE, error().state()); @@ -371,7 +301,7 @@ TEST_F(ManagedUserRefreshTokenFetcherTest, FetchRefreshTokenNetworkError) { TEST_F(ManagedUserRefreshTokenFetcherTest, FetchRefreshTokenTransientNetworkError) { StartFetching(); - GetOAuth2TokenServiceRequest()->Succeed(); + MakeOAuth2TokenServiceRequestSucceed(); MakeIssueTokenRequestSucceed(); SetNetworkError(GetRefreshTokenRequest(), net::ERR_CONNECTION_REFUSED); @@ -384,7 +314,7 @@ TEST_F(ManagedUserRefreshTokenFetcherTest, TEST_F(ManagedUserRefreshTokenFetcherTest, FetchRefreshTokenBadRequest) { StartFetching(); - GetOAuth2TokenServiceRequest()->Succeed(); + MakeOAuth2TokenServiceRequestSucceed(); MakeIssueTokenRequestSucceed(); SetHttpError(GetRefreshTokenRequest(), net::HTTP_BAD_REQUEST); @@ -403,7 +333,7 @@ TEST_F(ManagedUserRefreshTokenFetcherTest, CancelWhileFetchingAccessToken) { TEST_F(ManagedUserRefreshTokenFetcherTest, CancelWhileCallingIssueToken) { StartFetching(); - GetOAuth2TokenServiceRequest()->Succeed(); + MakeOAuth2TokenServiceRequestSucceed(); Reset(); EXPECT_EQ(GoogleServiceAuthError::NONE, error().state()); @@ -412,7 +342,7 @@ TEST_F(ManagedUserRefreshTokenFetcherTest, CancelWhileCallingIssueToken) { TEST_F(ManagedUserRefreshTokenFetcherTest, CancelWhileFetchingRefreshToken) { StartFetching(); - GetOAuth2TokenServiceRequest()->Succeed(); + MakeOAuth2TokenServiceRequestSucceed(); MakeIssueTokenRequestSucceed(); Reset(); diff --git a/chrome/browser/password_manager/password_generation_manager_unittest.cc b/chrome/browser/password_manager/password_generation_manager_unittest.cc index 22f2050..ad11d79 100644 --- a/chrome/browser/password_manager/password_generation_manager_unittest.cc +++ b/chrome/browser/password_manager/password_generation_manager_unittest.cc @@ -68,8 +68,8 @@ class IncognitoPasswordGenerationManagerTest : virtual content::BrowserContext* CreateBrowserContext() OVERRIDE { // Create an incognito profile. TestingProfile::Builder builder; + builder.SetIncognito(); scoped_ptr<TestingProfile> profile = builder.Build(); - profile->set_incognito(true); return profile.release(); } }; diff --git a/chrome/browser/policy/cloud/cloud_policy_client_registration_helper.cc b/chrome/browser/policy/cloud/cloud_policy_client_registration_helper.cc index 224977f..0f0e7be 100644 --- a/chrome/browser/policy/cloud/cloud_policy_client_registration_helper.cc +++ b/chrome/browser/policy/cloud/cloud_policy_client_registration_helper.cc @@ -11,13 +11,13 @@ #include "base/logging.h" #include "base/time/time.h" #include "base/values.h" +#include "chrome/browser/signin/oauth2_token_service.h" #include "google_apis/gaia/gaia_constants.h" #include "google_apis/gaia/gaia_urls.h" #include "google_apis/gaia/google_service_auth_error.h" #if defined(OS_ANDROID) #include "chrome/browser/signin/android_profile_oauth2_token_service.h" -#include "chrome/browser/signin/oauth2_token_service.h" #else #include "google_apis/gaia/oauth2_access_token_consumer.h" #include "google_apis/gaia/oauth2_access_token_fetcher.h" @@ -25,8 +25,6 @@ namespace policy { -namespace { - // OAuth2 scope for the userinfo service. const char kServiceScopeGetUserInfo[] = "https://www.googleapis.com/auth/userinfo.email"; @@ -37,22 +35,25 @@ const char kGetHostedDomainKey[] = "hd"; typedef base::Callback<void(const std::string&)> StringCallback; -} // namespace - -#if defined(OS_ANDROID) - // This class fetches an OAuth2 token scoped for the userinfo and DM services. -// The AccountManager is used to mint the token on the Java side, given the -// username of an account that is known to exist on the device. -// This allows fetching the token before the sign-in process is finished. -class CloudPolicyClientRegistrationHelper::TokenHelperAndroid +// On Android, we use a special API to allow us to fetch a token for an account +// that is not yet logged in to allow fetching the token before the sign-in +// process is finished. +class CloudPolicyClientRegistrationHelper::TokenServiceHelper : public OAuth2TokenService::Consumer { public: - TokenHelperAndroid(); + TokenServiceHelper(); - void FetchAccessToken(AndroidProfileOAuth2TokenService* token_service, - const std::string& username, - const StringCallback& callback); + void FetchAccessToken( +#if defined(OS_ANDROID) + // TODO(atwilson): Remove this when StartRequestForUsername() is merged + // into the base OAuth2TokenService class. + AndroidProfileOAuth2TokenService* token_service, +#else + OAuth2TokenService* token_service, +#endif + const std::string& username, + const StringCallback& callback); private: // OAuth2TokenService::Consumer implementation: @@ -66,23 +67,35 @@ class CloudPolicyClientRegistrationHelper::TokenHelperAndroid scoped_ptr<OAuth2TokenService::Request> token_request_; }; -CloudPolicyClientRegistrationHelper::TokenHelperAndroid::TokenHelperAndroid() {} +CloudPolicyClientRegistrationHelper::TokenServiceHelper::TokenServiceHelper() {} -void CloudPolicyClientRegistrationHelper::TokenHelperAndroid::FetchAccessToken( +void CloudPolicyClientRegistrationHelper::TokenServiceHelper::FetchAccessToken( +#if defined(OS_ANDROID) AndroidProfileOAuth2TokenService* token_service, +#else + OAuth2TokenService* token_service, +#endif const std::string& username, const StringCallback& callback) { + DCHECK(!token_request_); + // Either the caller must supply a username, or the user must be signed in + // already. + DCHECK(!username.empty() || token_service->RefreshTokenIsAvailable()); callback_ = callback; OAuth2TokenService::ScopeSet scopes; scopes.insert(GaiaConstants::kDeviceManagementServiceOAuth); scopes.insert(kServiceScopeGetUserInfo); +#if defined(OS_ANDROID) token_request_ = token_service->StartRequestForUsername(username, scopes, this); +#else + token_request_ = token_service->StartRequest(scopes, this); +#endif } -void CloudPolicyClientRegistrationHelper::TokenHelperAndroid::OnGetTokenSuccess( +void CloudPolicyClientRegistrationHelper::TokenServiceHelper::OnGetTokenSuccess( const OAuth2TokenService::Request* request, const std::string& access_token, const base::Time& expiration_time) { @@ -90,22 +103,23 @@ void CloudPolicyClientRegistrationHelper::TokenHelperAndroid::OnGetTokenSuccess( callback_.Run(access_token); } -void CloudPolicyClientRegistrationHelper::TokenHelperAndroid::OnGetTokenFailure( +void CloudPolicyClientRegistrationHelper::TokenServiceHelper::OnGetTokenFailure( const OAuth2TokenService::Request* request, const GoogleServiceAuthError& error) { DCHECK_EQ(token_request_.get(), request); callback_.Run(""); } -#else - +#if !defined(OS_ANDROID) // This class fetches the OAuth2 token scoped for the userinfo and DM services. // It uses an OAuth2AccessTokenFetcher to fetch it, given a login refresh token -// that can be used to authorize that request. -class CloudPolicyClientRegistrationHelper::TokenHelper +// that can be used to authorize that request. This class is not needed on +// Android because we can use OAuth2TokenService to fetch tokens for accounts +// even before they are signed in. +class CloudPolicyClientRegistrationHelper::LoginTokenHelper : public OAuth2AccessTokenConsumer { public: - TokenHelper(); + LoginTokenHelper(); void FetchAccessToken(const std::string& login_refresh_token, net::URLRequestContextGetter* context, @@ -122,12 +136,13 @@ class CloudPolicyClientRegistrationHelper::TokenHelper scoped_ptr<OAuth2AccessTokenFetcher> oauth2_access_token_fetcher_; }; -CloudPolicyClientRegistrationHelper::TokenHelper::TokenHelper() {} +CloudPolicyClientRegistrationHelper::LoginTokenHelper::LoginTokenHelper() {} -void CloudPolicyClientRegistrationHelper::TokenHelper::FetchAccessToken( +void CloudPolicyClientRegistrationHelper::LoginTokenHelper::FetchAccessToken( const std::string& login_refresh_token, net::URLRequestContextGetter* context, const StringCallback& callback) { + DCHECK(!oauth2_access_token_fetcher_); callback_ = callback; // Start fetching an OAuth2 access token for the device management and @@ -145,13 +160,13 @@ void CloudPolicyClientRegistrationHelper::TokenHelper::FetchAccessToken( scopes); } -void CloudPolicyClientRegistrationHelper::TokenHelper::OnGetTokenSuccess( +void CloudPolicyClientRegistrationHelper::LoginTokenHelper::OnGetTokenSuccess( const std::string& access_token, const base::Time& expiration_time) { callback_.Run(access_token); } -void CloudPolicyClientRegistrationHelper::TokenHelper::OnGetTokenFailure( +void CloudPolicyClientRegistrationHelper::LoginTokenHelper::OnGetTokenFailure( const GoogleServiceAuthError& error) { callback_.Run(""); } @@ -178,10 +193,13 @@ CloudPolicyClientRegistrationHelper::~CloudPolicyClientRegistrationHelper() { client_->RemoveObserver(this); } -#if defined(OS_ANDROID) void CloudPolicyClientRegistrationHelper::StartRegistration( +#if defined(OS_ANDROID) AndroidProfileOAuth2TokenService* token_service, +#else + OAuth2TokenService* token_service, +#endif const std::string& username, const base::Closure& callback) { DVLOG(1) << "Starting registration process with username"; @@ -189,16 +207,15 @@ void CloudPolicyClientRegistrationHelper::StartRegistration( callback_ = callback; client_->AddObserver(this); - token_helper_.reset(new TokenHelperAndroid()); - token_helper_->FetchAccessToken( + token_service_helper_.reset(new TokenServiceHelper()); + token_service_helper_->FetchAccessToken( token_service, username, base::Bind(&CloudPolicyClientRegistrationHelper::OnTokenFetched, base::Unretained(this))); } -#else - +#if !defined(OS_ANDROID) void CloudPolicyClientRegistrationHelper::StartRegistrationWithLoginToken( const std::string& login_refresh_token, const base::Closure& callback) { @@ -207,19 +224,22 @@ void CloudPolicyClientRegistrationHelper::StartRegistrationWithLoginToken( callback_ = callback; client_->AddObserver(this); - token_helper_.reset(new TokenHelper()); - token_helper_->FetchAccessToken( + login_token_helper_.reset( + new CloudPolicyClientRegistrationHelper::LoginTokenHelper()); + login_token_helper_->FetchAccessToken( login_refresh_token, context_, base::Bind(&CloudPolicyClientRegistrationHelper::OnTokenFetched, base::Unretained(this))); } - #endif void CloudPolicyClientRegistrationHelper::OnTokenFetched( const std::string& access_token) { - token_helper_.reset(); +#if !defined(OS_ANDROID) + login_token_helper_.reset(); +#endif + token_service_helper_.reset(); if (access_token.empty()) { DLOG(WARNING) << "Could not fetch access token for " diff --git a/chrome/browser/policy/cloud/cloud_policy_client_registration_helper.h b/chrome/browser/policy/cloud/cloud_policy_client_registration_helper.h index 330cfbd..cb26ed5 100644 --- a/chrome/browser/policy/cloud/cloud_policy_client_registration_helper.h +++ b/chrome/browser/policy/cloud/cloud_policy_client_registration_helper.h @@ -16,6 +16,7 @@ #include "chrome/browser/policy/proto/cloud/device_management_backend.pb.h" class AndroidProfileOAuth2TokenService; +class OAuth2TokenService; namespace net { class URLRequestContextGetter; @@ -40,15 +41,23 @@ class CloudPolicyClientRegistrationHelper : public UserInfoFetcher::Delegate, enterprise_management::DeviceRegisterRequest::Type registration_type); virtual ~CloudPolicyClientRegistrationHelper(); -#if defined(OS_ANDROID) // Starts the client registration process. This version uses the - // AndroidProfileOAuth2TokenService to mint the new token for the userinfo + // supplied OAuth2TokenService to mint the new token for the userinfo // and DM services, using the |username| account. // |callback| is invoked when the registration is complete. - void StartRegistration(AndroidProfileOAuth2TokenService* token_service, - const std::string& username, - const base::Closure& callback); + void StartRegistration( +#if defined(OS_ANDROID) + // TODO(atwilson): Remove this when the Android StartRequestForUsername() + // API is folded into the base OAuth2TokenService class (when that class + // is made multi-account aware). + AndroidProfileOAuth2TokenService* token_service, #else + OAuth2TokenService* token_service, +#endif + const std::string& username, + const base::Closure& callback); + +#if !defined(OS_ANDROID) // Starts the client registration process. The |login_refresh_token| is used // to mint a new token for the userinfo and DM services. // |callback| is invoked when the registration is complete. @@ -57,10 +66,9 @@ class CloudPolicyClientRegistrationHelper : public UserInfoFetcher::Delegate, #endif private: -#if defined(OS_ANDROID) - class TokenHelperAndroid; -#else - class TokenHelper; + class TokenServiceHelper; +#if !defined(OS_ANDROID) + class LoginTokenHelper; #endif void OnTokenFetched(const std::string& oauth_access_token); @@ -79,13 +87,17 @@ class CloudPolicyClientRegistrationHelper : public UserInfoFetcher::Delegate, // Invoked when the registration request has been completed. void RequestCompleted(); - // Internal helper used to fetch the access token. There is an OS_ANDROID - // implementation which uses the AccountManager and a known account name, - // and a desktop implementation which uses an OAuth2AccessTokenFetcher. -#if defined(OS_ANDROID) - scoped_ptr<TokenHelperAndroid> token_helper_; -#else - scoped_ptr<TokenHelper> token_helper_; + // Internal helper class that uses OAuth2TokenService to fetch an OAuth + // access token. On desktop, this is only used after the user has signed in - + // desktop platforms use LoginTokenHelper for policy fetches performed before + // signin is complete. + scoped_ptr<TokenServiceHelper> token_service_helper_; + +#if !defined(OS_ANDROID) + // Special desktop-only helper to fetch an OAuth access token prior to + // the completion of signin. Not used on Android since all token fetching + // is done via OAuth2TokenService. + scoped_ptr<LoginTokenHelper> login_token_helper_; #endif // Helper class for fetching information from GAIA about the currently diff --git a/chrome/browser/policy/cloud/user_policy_signin_service.cc b/chrome/browser/policy/cloud/user_policy_signin_service.cc index 56d712c..baef750 100644 --- a/chrome/browser/policy/cloud/user_policy_signin_service.cc +++ b/chrome/browser/policy/cloud/user_policy_signin_service.cc @@ -14,10 +14,9 @@ #include "chrome/browser/policy/cloud/user_cloud_policy_manager.h" #include "chrome/browser/profiles/profile.h" #include "chrome/browser/profiles/profile_manager.h" +#include "chrome/browser/signin/profile_oauth2_token_service.h" +#include "chrome/browser/signin/profile_oauth2_token_service_factory.h" #include "chrome/browser/signin/signin_manager.h" -#include "chrome/browser/signin/signin_manager_factory.h" -#include "chrome/browser/signin/token_service.h" -#include "chrome/browser/signin/token_service_factory.h" #include "chrome/common/pref_names.h" #include "content/public/browser/notification_details.h" #include "content/public/browser/notification_source.h" @@ -28,35 +27,41 @@ namespace policy { UserPolicySigninService::UserPolicySigninService( Profile* profile, PrefService* local_state, - DeviceManagementService* device_management_service) + DeviceManagementService* device_management_service, + ProfileOAuth2TokenService* token_service) : UserPolicySigninServiceBase(profile, local_state, - device_management_service) { + device_management_service), + oauth2_token_service_(token_service) { if (profile->GetPrefs()->GetBoolean(prefs::kDisableCloudPolicyOnSignin)) return; + // ProfileOAuth2TokenService should not yet have loaded its tokens since this + // happens in the background after PKS initialization - so this service + // should always be created before the oauth token is available. + DCHECK(!oauth2_token_service_->RefreshTokenIsAvailable()); + // Listen for an OAuth token to become available so we can register a client // if for some reason the client is not already registered (for example, if // the policy load failed during initial signin). - registrar()->Add(this, - chrome::NOTIFICATION_TOKEN_AVAILABLE, - content::Source<TokenService>( - TokenServiceFactory::GetForProfile(profile))); - - // TokenService should not yet have loaded its tokens since this happens in - // the background after PKS initialization - so this service should always be - // created before the oauth token is available. - DCHECK(!TokenServiceFactory::GetForProfile(profile)->HasOAuthLoginToken()); + oauth2_token_service_->AddObserver(this); } -UserPolicySigninService::~UserPolicySigninService() {} +UserPolicySigninService::~UserPolicySigninService() { +} -void UserPolicySigninService::Shutdown() { +void UserPolicySigninService::PrepareForUserCloudPolicyManagerShutdown() { // Stop any pending registration helper activity. We do this here instead of // in the destructor because we want to shutdown the registration helper // before UserCloudPolicyManager shuts down the CloudPolicyClient. registration_helper_.reset(); + + UserPolicySigninServiceBase::PrepareForUserCloudPolicyManagerShutdown(); +} + +void UserPolicySigninService::Shutdown() { UserPolicySigninServiceBase::Shutdown(); + oauth2_token_service_->RemoveObserver(this); } void UserPolicySigninService::RegisterPolicyClient( @@ -98,44 +103,21 @@ void UserPolicySigninService::CallPolicyRegistrationCallback( callback.Run(client.Pass()); } -void UserPolicySigninService::Observe( - int type, - const content::NotificationSource& source, - const content::NotificationDetails& details) { - - if (profile()->IsManaged()) { - registrar()->RemoveAll(); - return; - } - - // If using a TestingProfile with no SigninManager or UserCloudPolicyManager, - // skip initialization. - if (!GetManager() || !SigninManagerFactory::GetForProfile(profile())) { +void UserPolicySigninService::OnRefreshTokenAvailable( + const std::string& account_id) { + // If using a TestingProfile with no UserCloudPolicyManager, skip + // initialization. + if (!GetManager()) { DVLOG(1) << "Skipping initialization for tests due to missing components."; return; } - switch (type) { - case chrome::NOTIFICATION_TOKEN_AVAILABLE: { - const TokenService::TokenAvailableDetails& token_details = - *(content::Details<const TokenService::TokenAvailableDetails>( - details).ptr()); - if (token_details.service() == - GaiaConstants::kGaiaOAuth2LoginRefreshToken) { - SigninManager* signin_manager = - SigninManagerFactory::GetForProfile(profile()); - std::string username = signin_manager->GetAuthenticatedUsername(); - // Should not have GAIA tokens if the user isn't signed in. - DCHECK(!username.empty()); - // TokenService now has a refresh token (implying that the user is - // signed in) so initialize the UserCloudPolicyManager. - InitializeForSignedInUser(username); - } - break; - } - default: - UserPolicySigninServiceBase::Observe(type, source, details); - } + std::string username = GetSigninManager()->GetAuthenticatedUsername(); + // Should not have OAuth tokens if the user isn't signed in. + DCHECK(!username.empty()); + // ProfileOAuth2TokenService now has a refresh token so initialize the + // UserCloudPolicyManager. + InitializeForSignedInUser(username); } void UserPolicySigninService::InitializeUserCloudPolicyManager( @@ -146,10 +128,9 @@ void UserPolicySigninService::InitializeUserCloudPolicyManager( void UserPolicySigninService::ShutdownUserCloudPolicyManager() { UserCloudPolicyManager* manager = GetManager(); - if (manager) { - // Allow the user to signout again. - SigninManagerFactory::GetForProfile(profile())->ProhibitSignout(false); - } + // Allow the user to signout again. + if (manager) + GetSigninManager()->ProhibitSignout(false); UserPolicySigninServiceBase::ShutdownUserCloudPolicyManager(); } @@ -164,22 +145,19 @@ void UserPolicySigninService::OnInitializationCompleted( DVLOG_IF(1, manager->IsClientRegistered()) << "Client already registered - not fetching DMToken"; if (!manager->IsClientRegistered()) { - std::string token = TokenServiceFactory::GetForProfile(profile())-> - GetOAuth2LoginRefreshToken(); - if (token.empty()) { - // No token yet - this class listens for NOTIFICATION_TOKEN_AVAILABLE + if (!oauth2_token_service_->RefreshTokenIsAvailable()) { + // No token yet - this class listens for OnRefreshTokenAvailable() // and will re-attempt registration once the token is available. DLOG(WARNING) << "No OAuth Refresh Token - delaying policy download"; return; } - RegisterCloudPolicyService(token); + RegisterCloudPolicyService(); } // If client is registered now, prohibit signout. ProhibitSignoutIfNeeded(); } -void UserPolicySigninService::RegisterCloudPolicyService( - const std::string& login_token) { +void UserPolicySigninService::RegisterCloudPolicyService() { DCHECK(!GetManager()->IsClientRegistered()); DVLOG(1) << "Fetching new DM Token"; // Do nothing if already starting the registration process. @@ -193,8 +171,9 @@ void UserPolicySigninService::RegisterCloudPolicyService( GetManager()->core()->client(), ShouldForceLoadPolicy(), enterprise_management::DeviceRegisterRequest::BROWSER)); - registration_helper_->StartRegistrationWithLoginToken( - login_token, + registration_helper_->StartRegistration( + oauth2_token_service_, + GetSigninManager()->GetAuthenticatedUsername(), base::Bind(&UserPolicySigninService::OnRegistrationComplete, base::Unretained(this))); } @@ -207,9 +186,7 @@ void UserPolicySigninService::OnRegistrationComplete() { void UserPolicySigninService::ProhibitSignoutIfNeeded() { if (GetManager()->IsClientRegistered()) { DVLOG(1) << "User is registered for policy - prohibiting signout"; - SigninManager* signin_manager = - SigninManagerFactory::GetForProfile(profile()); - signin_manager->ProhibitSignout(true); + GetSigninManager()->ProhibitSignout(true); } } diff --git a/chrome/browser/policy/cloud/user_policy_signin_service.h b/chrome/browser/policy/cloud/user_policy_signin_service.h index b708f79..fccb980 100644 --- a/chrome/browser/policy/cloud/user_policy_signin_service.h +++ b/chrome/browser/policy/cloud/user_policy_signin_service.h @@ -11,6 +11,7 @@ #include "base/compiler_specific.h" #include "base/memory/scoped_ptr.h" #include "chrome/browser/policy/cloud/user_policy_signin_service_base.h" +#include "chrome/browser/signin/profile_oauth2_token_service.h" class Profile; @@ -20,26 +21,27 @@ class CloudPolicyClientRegistrationHelper; // A specialization of the UserPolicySigninServiceBase for the desktop // platforms (Windows, Mac and Linux). -class UserPolicySigninService : public UserPolicySigninServiceBase { +class UserPolicySigninService : public UserPolicySigninServiceBase, + public OAuth2TokenService::Observer { public: // Creates a UserPolicySigninService associated with the passed |profile|. UserPolicySigninService(Profile* profile, PrefService* local_state, - DeviceManagementService* device_management_service); + DeviceManagementService* device_management_service, + ProfileOAuth2TokenService* oauth2_token_service); virtual ~UserPolicySigninService(); // Registers a CloudPolicyClient for fetching policy for a user. The // |oauth2_login_token| and |username| are explicitly passed because - // the user is not signed in yet (TokenService does not have any tokens yet - // to prevent services from using it until after we've fetched policy). + // the user is not signed in yet (ProfileOAuth2TokenService does not have + // any tokens yet to prevent services from using it until after we've fetched + // policy). void RegisterPolicyClient(const std::string& username, const std::string& oauth2_login_token, const PolicyRegistrationCallback& callback); - // content::NotificationObserver implementation: - virtual void Observe(int type, - const content::NotificationSource& source, - const content::NotificationDetails& details) OVERRIDE; + // OAuth2TokenService::Observer implementation: + virtual void OnRefreshTokenAvailable(const std::string& account_id) OVERRIDE; // CloudPolicyService::Observer implementation: virtual void OnInitializationCompleted(CloudPolicyService* service) OVERRIDE; @@ -47,9 +49,12 @@ class UserPolicySigninService : public UserPolicySigninServiceBase { // BrowserContextKeyedService implementation: virtual void Shutdown() OVERRIDE; + protected: // UserPolicySigninServiceBase implementation: virtual void InitializeUserCloudPolicyManager( scoped_ptr<CloudPolicyClient> client) OVERRIDE; + + virtual void PrepareForUserCloudPolicyManagerShutdown() OVERRIDE; virtual void ShutdownUserCloudPolicyManager() OVERRIDE; private: @@ -57,7 +62,7 @@ class UserPolicySigninService : public UserPolicySigninServiceBase { // the cloud policy server. |oauth_login_token| should contain an OAuth login // refresh token that can be downscoped to get an access token for the // device_management service. - void RegisterCloudPolicyService(const std::string& oauth_login_token); + void RegisterCloudPolicyService(); // Callback invoked when policy registration has finished. void OnRegistrationComplete(); @@ -72,6 +77,10 @@ class UserPolicySigninService : public UserPolicySigninServiceBase { scoped_ptr<CloudPolicyClientRegistrationHelper> registration_helper_; + // Weak pointer to the token service we use to authenticate during + // CloudPolicyClient registration. + ProfileOAuth2TokenService* oauth2_token_service_; + DISALLOW_COPY_AND_ASSIGN(UserPolicySigninService); }; diff --git a/chrome/browser/policy/cloud/user_policy_signin_service_android.cc b/chrome/browser/policy/cloud/user_policy_signin_service_android.cc index 1a9faee..c630659 100644 --- a/chrome/browser/policy/cloud/user_policy_signin_service_android.cc +++ b/chrome/browser/policy/cloud/user_policy_signin_service_android.cc @@ -18,7 +18,6 @@ #include "chrome/browser/profiles/profile.h" #include "chrome/browser/signin/profile_oauth2_token_service_factory.h" #include "chrome/browser/signin/signin_manager.h" -#include "chrome/browser/signin/signin_manager_factory.h" #include "chrome/common/chrome_switches.h" #include "chrome/common/pref_names.h" #include "net/base/network_change_notifier.h" @@ -39,11 +38,13 @@ enterprise_management::DeviceRegisterRequest::Type GetRegistrationType() { UserPolicySigninService::UserPolicySigninService( Profile* profile, PrefService* local_state, - DeviceManagementService* device_management_service) + DeviceManagementService* device_management_service, + AndroidProfileOAuth2TokenService* token_service) : UserPolicySigninServiceBase(profile, local_state, device_management_service), - weak_factory_(this) {} + weak_factory_(this), + oauth2_token_service_(token_service) {} UserPolicySigninService::~UserPolicySigninService() {} @@ -68,7 +69,7 @@ void UserPolicySigninService::RegisterPolicyClient( force_load_policy, GetRegistrationType())); registration_helper_->StartRegistration( - ProfileOAuth2TokenServiceFactory::GetForProfile(profile()), + oauth2_token_service_, username, base::Bind(&UserPolicySigninService::CallPolicyRegistrationCallback, base::Unretained(this), @@ -137,9 +138,7 @@ void UserPolicySigninService::RegisterCloudPolicyService() { // If the user signed-out while this task was waiting then Shutdown() would // have been called, which would have invalidated this task. Since we're here // then the user must still be signed-in. - SigninManager* signin_manager = - SigninManagerFactory::GetForProfile(profile()); - const std::string& username = signin_manager->GetAuthenticatedUsername(); + const std::string& username = GetSigninManager()->GetAuthenticatedUsername(); DCHECK(!username.empty()); DCHECK(!GetManager()->IsClientRegistered()); DCHECK(GetManager()->core()->client()); @@ -155,7 +154,7 @@ void UserPolicySigninService::RegisterCloudPolicyService() { force_load_policy, GetRegistrationType())); registration_helper_->StartRegistration( - ProfileOAuth2TokenServiceFactory::GetForProfile(profile()), + oauth2_token_service_, username, base::Bind(&UserPolicySigninService::OnRegistrationDone, base::Unretained(this))); diff --git a/chrome/browser/policy/cloud/user_policy_signin_service_android.h b/chrome/browser/policy/cloud/user_policy_signin_service_android.h index 0c5a9c4..e990580 100644 --- a/chrome/browser/policy/cloud/user_policy_signin_service_android.h +++ b/chrome/browser/policy/cloud/user_policy_signin_service_android.h @@ -13,6 +13,7 @@ #include "base/memory/weak_ptr.h" #include "chrome/browser/policy/cloud/user_policy_signin_service_base.h" +class AndroidProfileOAuth2TokenService; class Profile; namespace policy { @@ -25,7 +26,8 @@ class UserPolicySigninService : public UserPolicySigninServiceBase { // Creates a UserPolicySigninService associated with the passed |profile|. UserPolicySigninService(Profile* profile, PrefService* local_state, - DeviceManagementService* device_management_service); + DeviceManagementService* device_management_service, + AndroidProfileOAuth2TokenService* token_service); virtual ~UserPolicySigninService(); // Registers a CloudPolicyClient for fetching policy for |username|. @@ -57,6 +59,10 @@ class UserPolicySigninService : public UserPolicySigninServiceBase { scoped_ptr<CloudPolicyClientRegistrationHelper> registration_helper_; base::WeakPtrFactory<UserPolicySigninService> weak_factory_; + // Weak pointer to the token service used to authenticate the + // CloudPolicyClient during registration. + AndroidProfileOAuth2TokenService* oauth2_token_service_; + DISALLOW_COPY_AND_ASSIGN(UserPolicySigninService); }; diff --git a/chrome/browser/policy/cloud/user_policy_signin_service_base.cc b/chrome/browser/policy/cloud/user_policy_signin_service_base.cc index d50902e..1baf659 100644 --- a/chrome/browser/policy/cloud/user_policy_signin_service_base.cc +++ b/chrome/browser/policy/cloud/user_policy_signin_service_base.cc @@ -72,7 +72,7 @@ void UserPolicySigninServiceBase::Observe( const content::NotificationDetails& details) { // If using a TestingProfile with no SigninManager or UserCloudPolicyManager, // skip initialization. - if (!GetManager() || !SigninManagerFactory::GetForProfile(profile_)) { + if (!GetManager() || !GetSigninManager()) { DVLOG(1) << "Skipping initialization for tests due to missing components."; return; } @@ -133,6 +133,10 @@ void UserPolicySigninServiceBase::OnClientError(CloudPolicyClient* client) { } void UserPolicySigninServiceBase::Shutdown() { + PrepareForUserCloudPolicyManagerShutdown(); +} + +void UserPolicySigninServiceBase::PrepareForUserCloudPolicyManagerShutdown() { UserCloudPolicyManager* manager = GetManager(); if (manager && manager->core()->client()) manager->core()->client()->RemoveObserver(this); @@ -181,9 +185,7 @@ bool UserPolicySigninServiceBase::ShouldLoadPolicyForUser( } void UserPolicySigninServiceBase::InitializeOnProfileReady() { - SigninManager* signin_manager = - SigninManagerFactory::GetForProfile(profile_); - std::string username = signin_manager->GetAuthenticatedUsername(); + std::string username = GetSigninManager()->GetAuthenticatedUsername(); if (username.empty()) ShutdownUserCloudPolicyManager(); else @@ -230,7 +232,7 @@ void UserPolicySigninServiceBase::InitializeUserCloudPolicyManager( } void UserPolicySigninServiceBase::ShutdownUserCloudPolicyManager() { - Shutdown(); + PrepareForUserCloudPolicyManagerShutdown(); UserCloudPolicyManager* manager = GetManager(); if (manager) manager->DisconnectAndRemovePolicy(); @@ -240,4 +242,8 @@ UserCloudPolicyManager* UserPolicySigninServiceBase::GetManager() { return UserCloudPolicyManagerFactory::GetForProfile(profile_); } +SigninManager* UserPolicySigninServiceBase::GetSigninManager() { + return SigninManagerFactory::GetForProfile(profile_); +} + } // namespace policy diff --git a/chrome/browser/policy/cloud/user_policy_signin_service_base.h b/chrome/browser/policy/cloud/user_policy_signin_service_base.h index fc000aa..a551c35 100644 --- a/chrome/browser/policy/cloud/user_policy_signin_service_base.h +++ b/chrome/browser/policy/cloud/user_policy_signin_service_base.h @@ -20,6 +20,7 @@ class PrefService; class Profile; +class SigninManager; namespace policy { @@ -116,6 +117,10 @@ class UserPolicySigninServiceBase : public BrowserContextKeyedService, virtual void InitializeUserCloudPolicyManager( scoped_ptr<CloudPolicyClient> client); + // Prepares for the UserCloudPolicyManager to be shutdown due to + // user signout or profile destruction. + virtual void PrepareForUserCloudPolicyManagerShutdown(); + // Shuts down the UserCloudPolicyManager (for example, after the user signs // out) and deletes any cached policy. virtual void ShutdownUserCloudPolicyManager(); @@ -125,6 +130,7 @@ class UserPolicySigninServiceBase : public BrowserContextKeyedService, Profile* profile() { return profile_; } content::NotificationRegistrar* registrar() { return ®istrar_; } + SigninManager* GetSigninManager(); private: // Weak pointer to the profile this service is associated with. diff --git a/chrome/browser/policy/cloud/user_policy_signin_service_factory.cc b/chrome/browser/policy/cloud/user_policy_signin_service_factory.cc index 29394fd..fbeafd1 100644 --- a/chrome/browser/policy/cloud/user_policy_signin_service_factory.cc +++ b/chrome/browser/policy/cloud/user_policy_signin_service_factory.cc @@ -9,6 +9,7 @@ #include "chrome/browser/policy/browser_policy_connector.h" #include "chrome/browser/policy/cloud/user_cloud_policy_manager_factory.h" #include "chrome/browser/profiles/profile.h" +#include "chrome/browser/signin/profile_oauth2_token_service_factory.h" #include "chrome/browser/signin/signin_manager_factory.h" #include "chrome/common/pref_names.h" #include "components/browser_context_keyed_service/browser_context_dependency_manager.h" @@ -16,10 +17,8 @@ #if defined(OS_ANDROID) #include "chrome/browser/policy/cloud/user_policy_signin_service_android.h" -#include "chrome/browser/signin/profile_oauth2_token_service_factory.h" #else #include "chrome/browser/policy/cloud/user_policy_signin_service.h" -#include "chrome/browser/signin/token_service_factory.h" #endif namespace policy { @@ -35,11 +34,7 @@ UserPolicySigninServiceFactory::UserPolicySigninServiceFactory() : BrowserContextKeyedServiceFactory( "UserPolicySigninService", BrowserContextDependencyManager::GetInstance()) { -#if defined(OS_ANDROID) DependsOn(ProfileOAuth2TokenServiceFactory::GetInstance()); -#else - DependsOn(TokenServiceFactory::GetInstance()); -#endif DependsOn(SigninManagerFactory::GetInstance()); DependsOn(UserCloudPolicyManagerFactory::GetInstance()); } @@ -66,15 +61,20 @@ void UserPolicySigninServiceFactory::SetDeviceManagementServiceForTesting( BrowserContextKeyedService* UserPolicySigninServiceFactory::BuildServiceInstanceFor( - content::BrowserContext* profile) const { + content::BrowserContext* context) const { + Profile* profile = static_cast<Profile*>(context); BrowserPolicyConnector* connector = g_browser_process->browser_policy_connector(); DeviceManagementService* device_management_service = g_device_management_service ? g_device_management_service : connector->device_management_service(); - return new UserPolicySigninService(static_cast<Profile*>(profile), - g_browser_process->local_state(), - device_management_service); + // TODO(atwilson): Inject SigninManager here or remove the dependency + // entirely. http://crbug.com/276270. + return new UserPolicySigninService( + profile, + g_browser_process->local_state(), + device_management_service, + ProfileOAuth2TokenServiceFactory::GetForProfile(profile)); } bool diff --git a/chrome/browser/policy/cloud/user_policy_signin_service_unittest.cc b/chrome/browser/policy/cloud/user_policy_signin_service_unittest.cc index 5094030..8da3550 100644 --- a/chrome/browser/policy/cloud/user_policy_signin_service_unittest.cc +++ b/chrome/browser/policy/cloud/user_policy_signin_service_unittest.cc @@ -19,6 +19,7 @@ #include "chrome/browser/prefs/browser_prefs.h" #include "chrome/browser/profiles/profile.h" #include "chrome/browser/signin/fake_signin_manager.h" +#include "chrome/browser/signin/profile_oauth2_token_service_factory.h" #include "chrome/browser/signin/signin_manager.h" #include "chrome/browser/signin/signin_manager_factory.h" #include "chrome/test/base/testing_browser_process.h" @@ -41,11 +42,9 @@ #if defined(OS_ANDROID) #include "chrome/browser/policy/cloud/user_policy_signin_service_android.h" #include "chrome/browser/signin/android_profile_oauth2_token_service.h" -#include "chrome/browser/signin/profile_oauth2_token_service_factory.h" #else #include "chrome/browser/policy/cloud/user_policy_signin_service.h" -#include "chrome/browser/signin/token_service.h" -#include "chrome/browser/signin/token_service_factory.h" +#include "chrome/browser/signin/fake_profile_oauth2_token_service.h" #endif namespace em = enterprise_management; @@ -72,10 +71,6 @@ const char kHostedDomainResponse[] = " \"hd\": \"test.com\"" "}"; -const char kCombinedScopes[] = - "https://www.googleapis.com/auth/chromeosdevicemanagement " - "https://www.googleapis.com/auth/userinfo.email"; - class SigninManagerFake : public FakeSigninManager { public: explicit SigninManagerFake(Profile* profile) @@ -95,26 +90,32 @@ class SigninManagerFake : public FakeSigninManager { }; #if defined(OS_ANDROID) - -class FakeProfileOAuth2TokenService : public AndroidProfileOAuth2TokenService { +// TODO(atwilson): Remove this when ProfileOAuth2TokenService supports +// usernames. +class FakeAndroidProfileOAuth2TokenService + : public AndroidProfileOAuth2TokenService { public: - explicit FakeProfileOAuth2TokenService(Profile* profile) { + explicit FakeAndroidProfileOAuth2TokenService(Profile* profile) { Initialize(profile); } static BrowserContextKeyedService* Build(content::BrowserContext* profile) { - return new FakeProfileOAuth2TokenService(static_cast<Profile*>(profile)); + return new FakeAndroidProfileOAuth2TokenService( + static_cast<Profile*>(profile)); } // AndroidProfileOAuth2TokenService overrides: - virtual void FetchOAuth2Token( + virtual void FetchOAuth2TokenWithUsername( + RequestImpl* request, const std::string& username, - const std::string& scope, - const FetchOAuth2TokenCallback& callback) OVERRIDE { + const OAuth2TokenService::ScopeSet& scope) OVERRIDE { ASSERT_TRUE(!HasPendingRequest()); ASSERT_EQ(kTestUser, username); - ASSERT_EQ(kCombinedScopes, scope); - pending_callback_ = callback; + ASSERT_EQ(2U, scope.size()); + EXPECT_EQ(1U, scope.count(GaiaConstants::kDeviceManagementServiceOAuth)); + EXPECT_EQ(1U, scope.count( + "https://www.googleapis.com/auth/userinfo.email")); + pending_request_ = request->AsWeakPtr(); } void IssueToken(const std::string& token) { @@ -122,17 +123,21 @@ class FakeProfileOAuth2TokenService : public AndroidProfileOAuth2TokenService { GoogleServiceAuthError error = GoogleServiceAuthError::AuthErrorNone(); if (token.empty()) error = GoogleServiceAuthError::FromServiceError("fail"); - pending_callback_.Run( - error, token, base::Time::Now() + base::TimeDelta::FromDays(1)); - pending_callback_.Reset(); + if (pending_request_) { + pending_request_->InformConsumer( + error, + token, + base::Time::Now() + base::TimeDelta::FromDays(1)); + } + pending_request_.reset(); } bool HasPendingRequest() const { - return !pending_callback_.is_null(); + return pending_request_; } private: - FetchOAuth2TokenCallback pending_callback_; + base::WeakPtr<RequestImpl> pending_request_; }; #endif @@ -180,26 +185,25 @@ class UserPolicySigninServiceTest : public testing::Test { chrome::RegisterUserProfilePrefs(prefs->registry()); TestingProfile::Builder builder; builder.SetPrefService(scoped_ptr<PrefServiceSyncable>(prefs.Pass())); + builder.AddTestingFactory(SigninManagerFactory::GetInstance(), + SigninManagerFake::Build); +#if defined(OS_ANDROID) + builder.AddTestingFactory(ProfileOAuth2TokenServiceFactory::GetInstance(), + FakeAndroidProfileOAuth2TokenService::Build); +#else + builder.AddTestingFactory(ProfileOAuth2TokenServiceFactory::GetInstance(), + FakeProfileOAuth2TokenService::Build); +#endif + profile_ = builder.Build().Pass(); + signin_manager_ = static_cast<SigninManagerFake*>( + SigninManagerFactory::GetForProfile(profile_.get())); mock_store_ = new MockUserCloudPolicyStore(); EXPECT_CALL(*mock_store_, Load()).Times(AnyNumber()); manager_.reset(new UserCloudPolicyManager( profile_.get(), scoped_ptr<UserCloudPolicyStore>(mock_store_))); - signin_manager_ = static_cast<SigninManagerFake*>( - SigninManagerFactory::GetInstance()->SetTestingFactoryAndUse( - profile_.get(), SigninManagerFake::Build)); - -#if defined(OS_ANDROID) - ProfileOAuth2TokenServiceFactory* factory = - ProfileOAuth2TokenServiceFactory::GetInstance(); - token_service_ = static_cast<FakeProfileOAuth2TokenService*>( - factory->SetTestingFactoryAndUse(profile_.get(), - FakeProfileOAuth2TokenService::Build)); -#endif - // Make sure the UserPolicySigninService is created. - UserPolicySigninServiceFactory::GetForProfile(profile_.get()); Mock::VerifyAndClearExpectations(mock_store_); url_factory_.set_remove_fetcher_on_delete(true); } @@ -217,9 +221,26 @@ class UserPolicySigninServiceTest : public testing::Test { run_loop.RunUntilIdle(); } +#if defined(OS_ANDROID) + FakeAndroidProfileOAuth2TokenService* GetTokenService() { + ProfileOAuth2TokenService* service = + ProfileOAuth2TokenServiceFactory::GetForProfile(profile_.get()); + return static_cast<FakeAndroidProfileOAuth2TokenService*>(service); + } +#else + FakeProfileOAuth2TokenService* GetTokenService() { + ProfileOAuth2TokenService* service = + ProfileOAuth2TokenServiceFactory::GetForProfile(profile_.get()); + return static_cast<FakeProfileOAuth2TokenService*>(service); + } +#endif + bool IsRequestActive() { #if defined(OS_ANDROID) - if (token_service_->HasPendingRequest()) + if (GetTokenService()->HasPendingRequest()) + return true; +#else + if (!GetTokenService()->GetPendingRequests().empty()) return true; #endif return url_factory_.GetFetcherByID(0); @@ -227,8 +248,8 @@ class UserPolicySigninServiceTest : public testing::Test { void MakeOAuthTokenFetchSucceed() { #if defined(OS_ANDROID) - ASSERT_TRUE(token_service_->HasPendingRequest()); - token_service_->IssueToken("fake_token"); + ASSERT_TRUE(GetTokenService()->HasPendingRequest()); + GetTokenService()->IssueToken("fake_token"); #else ASSERT_TRUE(IsRequestActive()); net::TestURLFetcher* fetcher = url_factory_.GetFetcherByID(0); @@ -340,9 +361,6 @@ class UserPolicySigninServiceTest : public testing::Test { net::TestURLFetcherFactory url_factory_; SigninManagerFake* signin_manager_; -#if defined(OS_ANDROID) - FakeProfileOAuth2TokenService* token_service_; // Not owned. -#endif // Used in conjunction with OnRegisterCompleted() to test client registration // callbacks. @@ -400,11 +418,43 @@ TEST_F(UserPolicySigninServiceTest, InitWhileSignedIn) { ASSERT_FALSE(IsRequestActive()); // Make oauth token available. - TokenServiceFactory::GetForProfile(profile_.get())->IssueAuthTokenForTest( - GaiaConstants::kGaiaOAuth2LoginRefreshToken, "oauth_login_refresh_token"); + GetTokenService()->IssueRefreshToken("oauth_login_refresh_token"); + + // Client registration should be in progress since we now have an oauth token. + ASSERT_TRUE(IsRequestActive()); +} + +TEST_F(UserPolicySigninServiceTest, InitWhileSignedInOAuthError) { + // Set the user as signed in. + SigninManagerFactory::GetForProfile(profile_.get())->SetAuthenticatedUsername( + kTestUser); + + // Let the SigninService know that the profile has been created. + content::NotificationService::current()->Notify( + chrome::NOTIFICATION_PROFILE_ADDED, + content::Source<Profile>(profile_.get()), + content::NotificationService::NoDetails()); + + // UserCloudPolicyManager should be initialized. + ASSERT_TRUE(manager_->core()->service()); + + // Complete initialization of the store. + mock_store_->NotifyStoreLoaded(); + + // No oauth access token yet, so client registration should be deferred. + ASSERT_FALSE(IsRequestActive()); + + // Make oauth token available. + GetTokenService()->IssueRefreshToken("oauth_login_refresh_token"); // Client registration should be in progress since we now have an oauth token. ASSERT_TRUE(IsRequestActive()); + + // Now fail the access token fetch. + GoogleServiceAuthError error( + GoogleServiceAuthError::INVALID_GAIA_CREDENTIALS); + GetTokenService()->IssueErrorForAllPendingRequests(error); + ASSERT_FALSE(IsRequestActive()); } TEST_F(UserPolicySigninServiceTest, SignInAfterInit) { @@ -427,8 +477,7 @@ TEST_F(UserPolicySigninServiceTest, SignInAfterInit) { mock_store_->NotifyStoreLoaded(); // Make oauth token available. - TokenServiceFactory::GetForProfile(profile_.get())->IssueAuthTokenForTest( - GaiaConstants::kGaiaOAuth2LoginRefreshToken, "oauth_login_refresh_token"); + GetTokenService()->IssueRefreshToken("oauth_login_refresh_token"); // UserCloudPolicyManager should be initialized. ASSERT_TRUE(manager_->core()->service()); @@ -457,8 +506,7 @@ TEST_F(UserPolicySigninServiceTest, SignInWithNonEnterpriseUser) { mock_store_->NotifyStoreLoaded(); // Make oauth token available. - TokenServiceFactory::GetForProfile(profile_.get())->IssueAuthTokenForTest( - GaiaConstants::kGaiaOAuth2LoginRefreshToken, "oauth_login_refresh_token"); + GetTokenService()->IssueRefreshToken("oauth_login_refresh_token"); // UserCloudPolicyManager should not be initialized and there should be no // DMToken request active. @@ -483,8 +531,7 @@ TEST_F(UserPolicySigninServiceTest, UnregisteredClient) { kTestUser); // Make oauth token available. - TokenServiceFactory::GetForProfile(profile_.get())->IssueAuthTokenForTest( - GaiaConstants::kGaiaOAuth2LoginRefreshToken, "oauth_login_refresh_token"); + GetTokenService()->IssueRefreshToken("oauth_login_refresh_token"); // UserCloudPolicyManager should be initialized. ASSERT_TRUE(manager_->core()->service()); @@ -517,8 +564,7 @@ TEST_F(UserPolicySigninServiceTest, RegisteredClient) { kTestUser); // Make oauth token available. - TokenServiceFactory::GetForProfile(profile_.get())->IssueAuthTokenForTest( - GaiaConstants::kGaiaOAuth2LoginRefreshToken, "oauth_login_refresh_token"); + GetTokenService()->IssueRefreshToken("oauth_login_refresh_token"); // UserCloudPolicyManager should be initialized. ASSERT_TRUE(manager_->core()->service()); @@ -578,8 +624,8 @@ TEST_F(UserPolicySigninServiceTest, RegisterPolicyClientOAuthFailure) { // Cause the access token fetch to fail - callback should be invoked. #if defined(OS_ANDROID) - ASSERT_TRUE(token_service_->HasPendingRequest()); - token_service_->IssueToken(""); + ASSERT_TRUE(GetTokenService()->HasPendingRequest()); + GetTokenService()->IssueToken(""); #else net::TestURLFetcher* fetcher = url_factory_.GetFetcherByID(0); fetcher->set_status(net::URLRequestStatus(net::URLRequestStatus::FAILED, -1)); diff --git a/chrome/browser/profiles/off_the_record_profile_impl.cc b/chrome/browser/profiles/off_the_record_profile_impl.cc index 7f42c5d..4aacf82 100644 --- a/chrome/browser/profiles/off_the_record_profile_impl.cc +++ b/chrome/browser/profiles/off_the_record_profile_impl.cc @@ -99,7 +99,7 @@ OffTheRecordProfileImpl::OffTheRecordProfileImpl(Profile* real_profile) void OffTheRecordProfileImpl::Init() { BrowserContextDependencyManager::GetInstance()->CreateBrowserContextServices( - this, false); + this); DCHECK_NE(IncognitoModePrefs::DISABLED, IncognitoModePrefs::GetAvailability(profile_->GetPrefs())); @@ -496,4 +496,3 @@ PrefProxyConfigTracker* OffTheRecordProfileImpl::CreateProxyConfigTracker() { return ProxyServiceFactory::CreatePrefProxyConfigTrackerOfProfile( GetPrefs(), g_browser_process->local_state()); } - diff --git a/chrome/browser/profiles/off_the_record_profile_impl_unittest.cc b/chrome/browser/profiles/off_the_record_profile_impl_unittest.cc index 630fc6e..09b6015 100644 --- a/chrome/browser/profiles/off_the_record_profile_impl_unittest.cc +++ b/chrome/browser/profiles/off_the_record_profile_impl_unittest.cc @@ -151,9 +151,8 @@ TEST_F(OffTheRecordProfileImplTest, GetHostZoomMap) { new OffTheRecordProfileImpl(parent_profile.get())); child_profile->InitHostZoomMap(); - BrowserContextDependencyManager::GetInstance()->CreateBrowserContextServices( - child_profile.get(), - true); // For testing. + BrowserContextDependencyManager::GetInstance()-> + CreateBrowserContextServicesForTest(child_profile.get(), false); // Prepare child host zoom map. HostZoomMap* child_zoom_map = diff --git a/chrome/browser/profiles/profile_destroyer_unittest.cc b/chrome/browser/profiles/profile_destroyer_unittest.cc index 3c04289..4976584 100644 --- a/chrome/browser/profiles/profile_destroyer_unittest.cc +++ b/chrome/browser/profiles/profile_destroyer_unittest.cc @@ -11,7 +11,14 @@ class TestingOffTheRecordDestructionProfile : public TestingProfile { public: - TestingOffTheRecordDestructionProfile() : destroyed_otr_profile_(false) { + TestingOffTheRecordDestructionProfile() + : TestingProfile(base::FilePath(), + NULL, + scoped_refptr<ExtensionSpecialStoragePolicy>() + scoped_ptr<PrefServiceSyncable>(), + true, + TestingFactories()), + destroyed_otr_profile_(false) { set_incognito(true); } virtual void DestroyOffTheRecordProfile() OVERRIDE { diff --git a/chrome/browser/profiles/profile_impl.cc b/chrome/browser/profiles/profile_impl.cc index 95f2be1..1ad2bcf 100644 --- a/chrome/browser/profiles/profile_impl.cc +++ b/chrome/browser/profiles/profile_impl.cc @@ -755,7 +755,7 @@ void ProfileImpl::OnPrefsLoaded(bool success) { prefs_->SetBoolean(prefs::kSessionExitedCleanly, true); BrowserContextDependencyManager::GetInstance()->CreateBrowserContextServices( - this, false); + this); DCHECK(!net_pref_observer_); { diff --git a/chrome/browser/profiles/profile_manager_unittest.cc b/chrome/browser/profiles/profile_manager_unittest.cc index dd2417b..1181443 100644 --- a/chrome/browser/profiles/profile_manager_unittest.cc +++ b/chrome/browser/profiles/profile_manager_unittest.cc @@ -54,13 +54,9 @@ namespace { // observers is the same. Profile* g_created_profile; -} // namespace - -namespace testing { - -class ProfileManager : public ::ProfileManagerWithoutInit { +class UnittestProfileManager : public ::ProfileManagerWithoutInit { public: - explicit ProfileManager(const base::FilePath& user_data_dir) + explicit UnittestProfileManager(const base::FilePath& user_data_dir) : ::ProfileManagerWithoutInit(user_data_dir) {} protected: @@ -84,7 +80,7 @@ class ProfileManager : public ::ProfileManagerWithoutInit { } }; -} // namespace testing +} // namespace class ProfileManagerTest : public testing::Test { protected: @@ -102,7 +98,7 @@ class ProfileManagerTest : public testing::Test { // Create a new temporary directory, and store the path ASSERT_TRUE(temp_dir_.CreateUniqueTempDir()); TestingBrowserProcess::GetGlobal()->SetProfileManager( - new testing::ProfileManager(temp_dir_.path())); + new UnittestProfileManager(temp_dir_.path())); #if defined(OS_CHROMEOS) CommandLine* cl = CommandLine::ForCurrentProcess(); @@ -417,11 +413,12 @@ TEST_F(ProfileManagerTest, GetLastUsedProfileAllowedByPolicy) { // Attach an incognito Profile to the TestingProfile. ASSERT_FALSE(profile->GetOffTheRecordProfile()); - TestingProfile* incognito_profile = new TestingProfile(); - incognito_profile->set_incognito(true); + TestingProfile::Builder builder; + builder.SetIncognito(); + scoped_ptr<TestingProfile> incognito_profile = builder.Build(); EXPECT_TRUE(incognito_profile->IsOffTheRecord()); TestingProfile* testing_profile = static_cast<TestingProfile*>(profile); - testing_profile->SetOffTheRecordProfile(incognito_profile); + testing_profile->SetOffTheRecordProfile(incognito_profile.PassAs<Profile>()); ASSERT_TRUE(profile->GetOffTheRecordProfile()); IncognitoModePrefs::SetAvailability(prefs, IncognitoModePrefs::DISABLED); @@ -569,10 +566,10 @@ TEST_F(ProfileManagerTest, LastOpenedProfilesDoesNotContainIncognito) { // incognito profiles should not be managed by the profile manager but by the // original profile. - TestingProfile* profile2 = new TestingProfile(); - ASSERT_TRUE(profile2); - profile2->set_incognito(true); - profile1->SetOffTheRecordProfile(profile2); + TestingProfile::Builder builder; + builder.SetIncognito(); + scoped_ptr<TestingProfile> profile2 = builder.Build(); + profile1->SetOffTheRecordProfile(profile2.PassAs<Profile>()); std::vector<Profile*> last_opened_profiles = profile_manager->GetLastOpenedProfiles(); @@ -588,7 +585,8 @@ TEST_F(ProfileManagerTest, LastOpenedProfilesDoesNotContainIncognito) { EXPECT_EQ(profile1, last_opened_profiles[0]); // And for profile2. - Browser::CreateParams profile2_params(profile2, chrome::GetActiveDesktop()); + Browser::CreateParams profile2_params(profile1->GetOffTheRecordProfile(), + chrome::GetActiveDesktop()); scoped_ptr<Browser> browser2a( chrome::CreateBrowserWithTestWindowForParams(&profile2_params)); diff --git a/chrome/browser/signin/android_profile_oauth2_token_service.cc b/chrome/browser/signin/android_profile_oauth2_token_service.cc index f79eb63..42a6516 100644 --- a/chrome/browser/signin/android_profile_oauth2_token_service.cc +++ b/chrome/browser/signin/android_profile_oauth2_token_service.cc @@ -36,6 +36,16 @@ std::string CombineScopes(const OAuth2TokenService::ScopeSet& scopes) { return scope; } +// Callback from FetchOAuth2TokenWithUsername(). +// Arguments: +// - the error, or NONE if the token fetch was successful. +// - the OAuth2 access token. +// - the expiry time of the token (may be null, indicating that the expiry +// time is unknown. +typedef base::Callback<void( + const GoogleServiceAuthError&, const std::string&, const base::Time&)> + FetchOAuth2TokenCallback; + } // namespace AndroidProfileOAuth2TokenService::AndroidProfileOAuth2TokenService() { @@ -45,18 +55,6 @@ AndroidProfileOAuth2TokenService::~AndroidProfileOAuth2TokenService() { } scoped_ptr<OAuth2TokenService::Request> - AndroidProfileOAuth2TokenService::StartRequest( - const OAuth2TokenService::ScopeSet& scopes, - OAuth2TokenService::Consumer* consumer) { - const std::string& sync_username = - SigninManagerFactory::GetForProfile(profile())-> - GetAuthenticatedUsername(); - DCHECK(content::BrowserThread::CurrentlyOn(content::BrowserThread::UI)); - DCHECK(!sync_username.empty()); - return StartRequestForUsername(sync_username, scopes, consumer); -} - -scoped_ptr<OAuth2TokenService::Request> AndroidProfileOAuth2TokenService::StartRequestForUsername( const std::string& username, const OAuth2TokenService::ScopeSet& scopes, @@ -64,10 +62,7 @@ scoped_ptr<OAuth2TokenService::Request> DCHECK(content::BrowserThread::CurrentlyOn(content::BrowserThread::UI)); scoped_ptr<RequestImpl> request(new RequestImpl(consumer)); - FetchOAuth2Token( - username, - CombineScopes(scopes), - base::Bind(&RequestImpl::InformConsumer, request->AsWeakPtr())); + FetchOAuth2TokenWithUsername(request.get(), username, scopes); return request.PassAs<Request>(); } @@ -91,20 +86,36 @@ void AndroidProfileOAuth2TokenService::InvalidateToken( } void AndroidProfileOAuth2TokenService::FetchOAuth2Token( + RequestImpl* request, + net::URLRequestContextGetter* getter, + const std::string& client_id, + const std::string& client_secret, + const OAuth2TokenService::ScopeSet& scopes) { + DCHECK(content::BrowserThread::CurrentlyOn(content::BrowserThread::UI)); + std::string username = SigninManagerFactory::GetForProfile(profile())-> + GetAuthenticatedUsername(); + DCHECK(!username.empty()); + // Just ignore client_id, getter, etc since we don't use them on Android. + FetchOAuth2TokenWithUsername(request, username, scopes); +} + +void AndroidProfileOAuth2TokenService::FetchOAuth2TokenWithUsername( + RequestImpl* request, const std::string& username, - const std::string& scope, - const FetchOAuth2TokenCallback& callback) { + const OAuth2TokenService::ScopeSet& scopes) { JNIEnv* env = AttachCurrentThread(); + std::string scope = CombineScopes(scopes); ScopedJavaLocalRef<jstring> j_username = ConvertUTF8ToJavaString(env, username); ScopedJavaLocalRef<jstring> j_scope = ConvertUTF8ToJavaString(env, scope); - // Allocate a copy of the callback on the heap, because the callback + // Allocate a copy of the request WeakPtr on the heap, because the object // needs to be passed through JNI as an int. // It will be passed back to OAuth2TokenFetched(), where it will be freed. scoped_ptr<FetchOAuth2TokenCallback> heap_callback( - new FetchOAuth2TokenCallback(callback)); + new FetchOAuth2TokenCallback(base::Bind(&RequestImpl::InformConsumer, + request->AsWeakPtr()))); // Call into Java to get a new token. Java_AndroidProfileOAuth2TokenServiceHelper_getOAuth2AuthToken( @@ -121,11 +132,8 @@ void OAuth2TokenFetched(JNIEnv* env, jclass clazz, jboolean result, jint nativeCallback) { std::string token = ConvertJavaStringToUTF8(env, authToken); - scoped_ptr<AndroidProfileOAuth2TokenService::FetchOAuth2TokenCallback> - heap_callback( - reinterpret_cast< - AndroidProfileOAuth2TokenService::FetchOAuth2TokenCallback*>( - nativeCallback)); + scoped_ptr<FetchOAuth2TokenCallback> heap_callback( + reinterpret_cast<FetchOAuth2TokenCallback*>(nativeCallback)); GoogleServiceAuthError err(result ? GoogleServiceAuthError::NONE : GoogleServiceAuthError::INVALID_GAIA_CREDENTIALS); diff --git a/chrome/browser/signin/android_profile_oauth2_token_service.h b/chrome/browser/signin/android_profile_oauth2_token_service.h index 39680fe..0057540 100644 --- a/chrome/browser/signin/android_profile_oauth2_token_service.h +++ b/chrome/browser/signin/android_profile_oauth2_token_service.h @@ -28,23 +28,6 @@ class TokenService; // request from other thread, please use ProfileOAuth2TokenServiceRequest. class AndroidProfileOAuth2TokenService : public ProfileOAuth2TokenService { public: - - // Callback from FetchOAuth2Token. - // Arguments: - // - the error, or NONE if the token fetch was successful. - // - the OAuth2 access token. - // - the expiry time of the token (may be null, indicating that the expiry - // time is unknown. - typedef base::Callback<void( - const GoogleServiceAuthError&, const std::string&, const base::Time&)> - FetchOAuth2TokenCallback; - - // Start the OAuth2 access token for the given scopes using - // ProfileSyncServiceAndroid. - virtual scoped_ptr<OAuth2TokenService::Request> StartRequest( - const OAuth2TokenService::ScopeSet& scopes, - OAuth2TokenService::Consumer* consumer) OVERRIDE; - // StartRequest() fetches a token for the currently signed-in account; this // version uses the account corresponding to |username|. This allows fetching // tokens before a user is signed-in (e.g. during the sign-in flow). @@ -66,10 +49,20 @@ class AndroidProfileOAuth2TokenService : public ProfileOAuth2TokenService { AndroidProfileOAuth2TokenService(); virtual ~AndroidProfileOAuth2TokenService(); - // virtual for testing. - virtual void FetchOAuth2Token(const std::string& username, - const std::string& scope, - const FetchOAuth2TokenCallback& callback); + // Overridden from OAuth2TokenService to intercept token fetch requests and + // redirect them to the Account Manager. + virtual void FetchOAuth2Token(RequestImpl* request, + net::URLRequestContextGetter* getter, + const std::string& client_id, + const std::string& client_secret, + const ScopeSet& scopes) OVERRIDE; + + // Low-level helper function used by both FetchOAuth2Token and + // StartRequestForUsername to fetch tokens. virtual to enable mocks. + virtual void FetchOAuth2TokenWithUsername( + RequestImpl* request, + const std::string& username, + const ScopeSet& scope); private: DISALLOW_COPY_AND_ASSIGN(AndroidProfileOAuth2TokenService); diff --git a/chrome/browser/signin/fake_profile_oauth2_token_service.cc b/chrome/browser/signin/fake_profile_oauth2_token_service.cc new file mode 100644 index 0000000..04d4fb9 --- /dev/null +++ b/chrome/browser/signin/fake_profile_oauth2_token_service.cc @@ -0,0 +1,120 @@ +// Copyright 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 "chrome/browser/signin/fake_profile_oauth2_token_service.h" + +FakeProfileOAuth2TokenService::PendingRequest::PendingRequest() { +} + +FakeProfileOAuth2TokenService::PendingRequest::~PendingRequest() { +} + +// static +BrowserContextKeyedService* FakeProfileOAuth2TokenService::Build( + content::BrowserContext* profile) { + return new FakeProfileOAuth2TokenService(); +} + +FakeProfileOAuth2TokenService::FakeProfileOAuth2TokenService() { +} + +FakeProfileOAuth2TokenService::~FakeProfileOAuth2TokenService() { +} + +void FakeProfileOAuth2TokenService::Shutdown() { + // Do not call the base class handler because it assumes that Initialize() + // is always called before Shutdown() and that's not the case for this mock. +} + +void FakeProfileOAuth2TokenService::IssueRefreshToken( + const std::string& token) { + refresh_token_ = token; + if (refresh_token_.empty()) + FireRefreshTokenRevoked("account_id"); + else + FireRefreshTokenAvailable("account_id"); + // TODO(atwilson): Maybe we should also call FireRefreshTokensLoaded() here? +} + +void FakeProfileOAuth2TokenService::IssueTokenForScope( + const ScopeSet& scope, + const std::string& access_token, + const base::Time& expiration) { + CompleteRequests(false, + scope, + GoogleServiceAuthError::AuthErrorNone(), + access_token, + expiration); +} + +void FakeProfileOAuth2TokenService::IssueErrorForScope( + const ScopeSet& scope, + const GoogleServiceAuthError& error) { + CompleteRequests(false, scope, error, std::string(), base::Time()); +} + +void FakeProfileOAuth2TokenService::IssueErrorForAllPendingRequests( + const GoogleServiceAuthError& error) { + CompleteRequests(true, ScopeSet(), error, std::string(), base::Time()); +} + +void FakeProfileOAuth2TokenService::IssueTokenForAllPendingRequests( + const std::string& access_token, + const base::Time& expiration) { + CompleteRequests(true, + ScopeSet(), + GoogleServiceAuthError::AuthErrorNone(), + access_token, + expiration); +} + +void FakeProfileOAuth2TokenService::CompleteRequests( + bool all_scopes, + const ScopeSet& scope, + const GoogleServiceAuthError& error, + const std::string& access_token, + const base::Time& expiration) { + std::vector<FakeProfileOAuth2TokenService::PendingRequest> requests = + GetPendingRequests(); + // Walk the requests and notify the callbacks. + for (std::vector<PendingRequest>::iterator it = pending_requests_.begin(); + it != pending_requests_.end(); ++it) { + if (it->request && (all_scopes || it->scopes == scope)) + it->request->InformConsumer(error, access_token, expiration); + } +} + +std::string FakeProfileOAuth2TokenService::GetRefreshToken() { + return refresh_token_; +} + +net::URLRequestContextGetter* +FakeProfileOAuth2TokenService::GetRequestContext() { + return NULL; +} + +std::vector<FakeProfileOAuth2TokenService::PendingRequest> +FakeProfileOAuth2TokenService::GetPendingRequests() { + std::vector<PendingRequest> valid_requests; + for (std::vector<PendingRequest>::iterator it = pending_requests_.begin(); + it != pending_requests_.end(); ++it) { + if (it->request) + valid_requests.push_back(*it); + } + return valid_requests; +} + +void FakeProfileOAuth2TokenService::FetchOAuth2Token( + RequestImpl* request, + net::URLRequestContextGetter* getter, + const std::string& client_id, + const std::string& client_secret, + const ScopeSet& scopes) { + PendingRequest pending_request; + pending_request.client_id = client_id; + pending_request.client_secret = client_secret; + pending_request.scopes = scopes; + pending_request.request = request->AsWeakPtr(); + pending_requests_.push_back(pending_request); +} diff --git a/chrome/browser/signin/fake_profile_oauth2_token_service.h b/chrome/browser/signin/fake_profile_oauth2_token_service.h new file mode 100644 index 0000000..cd5bc8b --- /dev/null +++ b/chrome/browser/signin/fake_profile_oauth2_token_service.h @@ -0,0 +1,111 @@ +// Copyright 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. + +#ifndef CHROME_BROWSER_SIGNIN_FAKE_PROFILE_OAUTH2_TOKEN_SERVICE_H_ +#define CHROME_BROWSER_SIGNIN_FAKE_PROFILE_OAUTH2_TOKEN_SERVICE_H_ + +#include "chrome/browser/signin/profile_oauth2_token_service.h" + +#include <string> +#include <vector> + +#include "base/compiler_specific.h" +#include "base/memory/weak_ptr.h" + +namespace content { +class BrowserContext; +} + +// Helper class to simplify writing unittests that depend on an instance of +// ProfileOAuth2TokenService. +// +// Tests would typically do something like the following: +// +// FakeProfileOAuth2TokenService service; +// ... +// service.IssueRefreshToken("token"); // Issue refresh token/notify observers +// ... +// // Confirm that there is at least one active request. +// EXPECT_GT(0U, service.GetPendingRequests().size()); +// ... +// // Make any pending token fetches for a given scope succeed. +// ScopeSet scopes; +// scopes.insert(GaiaConstants::kYourServiceScope); +// IssueTokenForScope(scopes, "access_token", base::Time()::Max()); +// ... +// // ...or make them fail... +// IssueErrorForScope(scopes, GoogleServiceAuthError(INVALID_GAIA_CREDENTIALS)); +// +class FakeProfileOAuth2TokenService : public ProfileOAuth2TokenService { + public: + struct PendingRequest { + PendingRequest(); + ~PendingRequest(); + + std::string client_id; + std::string client_secret; + ScopeSet scopes; + base::WeakPtr<RequestImpl> request; + }; + + FakeProfileOAuth2TokenService(); + virtual ~FakeProfileOAuth2TokenService(); + + // Sets the current refresh token. If |token| is non-empty, this will invoke + // OnRefreshTokenAvailable() on all Observers, otherwise this will invoke + // OnRefreshTokenRevoked(). + void IssueRefreshToken(const std::string& token); + + // Gets a list of active requests (can be used by tests to validate that the + // correct request has been issued). + std::vector<PendingRequest> GetPendingRequests(); + + // Helper routines to issue tokens for pending requests. + void IssueTokenForScope(const ScopeSet& scopes, + const std::string& access_token, + const base::Time& expiration); + + void IssueErrorForScope(const ScopeSet& scopes, + const GoogleServiceAuthError& error); + + void IssueTokenForAllPendingRequests(const std::string& access_token, + const base::Time& expiration); + + void IssueErrorForAllPendingRequests(const GoogleServiceAuthError& error); + + virtual void Shutdown() OVERRIDE; + + // Helper function to be used with + // BrowserContextKeyedService::SetTestingFactory(). + static BrowserContextKeyedService* Build(content::BrowserContext* profile); + + protected: + // OAuth2TokenService overrides. + virtual void FetchOAuth2Token(RequestImpl* request, + net::URLRequestContextGetter* getter, + const std::string& client_id, + const std::string& client_secret, + const ScopeSet& scopes) OVERRIDE; + + virtual std::string GetRefreshToken() OVERRIDE; + + virtual net::URLRequestContextGetter* GetRequestContext() OVERRIDE; + + private: + // Helper function to complete pending requests - if |all_scopes| is true, + // then all pending requests are completed, otherwise, only those requests + // matching |scopes| are completed. + void CompleteRequests(bool all_scopes, + const ScopeSet& scopes, + const GoogleServiceAuthError& error, + const std::string& access_token, + const base::Time& expiration); + + std::vector<PendingRequest> pending_requests_; + std::string refresh_token_; + + DISALLOW_COPY_AND_ASSIGN(FakeProfileOAuth2TokenService); +}; + +#endif // CHROME_BROWSER_SIGNIN_FAKE_PROFILE_OAUTH2_TOKEN_SERVICE_H_ diff --git a/chrome/browser/signin/oauth2_token_service.cc b/chrome/browser/signin/oauth2_token_service.cc index 1891244..1773871 100644 --- a/chrome/browser/signin/oauth2_token_service.cc +++ b/chrome/browser/signin/oauth2_token_service.cc @@ -315,7 +315,6 @@ OAuth2TokenService::OAuth2TokenService() { } OAuth2TokenService::~OAuth2TokenService() { - DCHECK(content::BrowserThread::CurrentlyOn(content::BrowserThread::UI)); // Release all the pending fetchers. STLDeleteContainerPairSecondPointers( pending_fetchers_.begin(), pending_fetchers_.end()); @@ -330,7 +329,6 @@ void OAuth2TokenService::RemoveObserver(Observer* observer) { } bool OAuth2TokenService::RefreshTokenIsAvailable() { - DCHECK(content::BrowserThread::CurrentlyOn(content::BrowserThread::UI)); return !GetRefreshToken().empty(); } @@ -383,20 +381,34 @@ OAuth2TokenService::StartRequestForClientWithContext( scoped_ptr<RequestImpl> request(new RequestImpl(consumer)); - std::string refresh_token = GetRefreshToken(); - if (refresh_token.empty()) { + if (!RefreshTokenIsAvailable()) { base::MessageLoop::current()->PostTask(FROM_HERE, base::Bind( &RequestImpl::InformConsumer, request->AsWeakPtr(), - GoogleServiceAuthError( - GoogleServiceAuthError::USER_NOT_SIGNED_UP), + GoogleServiceAuthError(GoogleServiceAuthError::USER_NOT_SIGNED_UP), std::string(), base::Time())); return request.PassAs<Request>(); } - if (HasCacheEntry(scopes)) - return StartCacheLookupRequest(scopes, consumer); + if (HasCacheEntry(scopes)) { + StartCacheLookupRequest(request.get(), scopes, consumer); + } else { + FetchOAuth2Token(request.get(), + getter, + client_id, + client_secret, + scopes); + } + return request.PassAs<Request>(); +} + +void OAuth2TokenService::FetchOAuth2Token(RequestImpl* request, + net::URLRequestContextGetter* getter, + const std::string& client_id, + const std::string& client_secret, + const ScopeSet& scopes) { + std::string refresh_token = GetRefreshToken(); // If there is already a pending fetcher for |scopes| and |refresh_token|, // simply register this |request| for those results rather than starting @@ -406,7 +418,7 @@ OAuth2TokenService::StartRequestForClientWithContext( pending_fetchers_.find(fetch_parameters); if (iter != pending_fetchers_.end()) { iter->second->AddWaitingRequest(request->AsWeakPtr()); - return request.PassAs<Request>(); + return; } pending_fetchers_[fetch_parameters] = @@ -417,23 +429,20 @@ OAuth2TokenService::StartRequestForClientWithContext( refresh_token, scopes, request->AsWeakPtr()); - return request.PassAs<Request>(); } -scoped_ptr<OAuth2TokenService::Request> - OAuth2TokenService::StartCacheLookupRequest( - const OAuth2TokenService::ScopeSet& scopes, - OAuth2TokenService::Consumer* consumer) { +void OAuth2TokenService::StartCacheLookupRequest( + RequestImpl* request, + const OAuth2TokenService::ScopeSet& scopes, + OAuth2TokenService::Consumer* consumer) { CHECK(HasCacheEntry(scopes)); const CacheEntry* cache_entry = GetCacheEntry(scopes); - scoped_ptr<RequestImpl> request(new RequestImpl(consumer)); base::MessageLoop::current()->PostTask(FROM_HERE, base::Bind( &RequestImpl::InformConsumer, request->AsWeakPtr(), GoogleServiceAuthError(GoogleServiceAuthError::NONE), cache_entry->access_token, cache_entry->expiration_date)); - return request.PassAs<Request>(); } void OAuth2TokenService::InvalidateToken(const ScopeSet& scopes, diff --git a/chrome/browser/signin/oauth2_token_service.h b/chrome/browser/signin/oauth2_token_service.h index 1d335cc..ac993de 100644 --- a/chrome/browser/signin/oauth2_token_service.h +++ b/chrome/browser/signin/oauth2_token_service.h @@ -14,6 +14,7 @@ #include "base/memory/scoped_ptr.h" #include "base/memory/weak_ptr.h" #include "base/observer_list.h" +#include "base/threading/non_thread_safe.h" #include "base/time/time.h" #include "google_apis/gaia/google_service_auth_error.h" @@ -49,7 +50,7 @@ class GoogleServiceAuthError; // // The caller of StartRequest() owns the returned request and is responsible to // delete the request even once the callback has been invoked. -class OAuth2TokenService { +class OAuth2TokenService : public base::NonThreadSafe { public: // Class representing a request that fetches an OAuth2 access token. class Request { @@ -113,13 +114,15 @@ class OAuth2TokenService { // |scopes| is the set of scopes to get an access token for, |consumer| is // the object that will be called back with results if the returned request // is not deleted. + // TODO(atwilson): Make this non-virtual when we change + // ProfileOAuth2TokenServiceRequestTest to use FakeProfileOAuth2TokenService. virtual scoped_ptr<Request> StartRequest(const ScopeSet& scopes, Consumer* consumer); // This method does the same as |StartRequest| except it uses |client_id| and // |client_secret| to identify OAuth client app instead of using // Chrome's default values. - virtual scoped_ptr<Request> StartRequestForClient( + scoped_ptr<Request> StartRequestForClient( const std::string& client_id, const std::string& client_secret, const ScopeSet& scopes, @@ -128,7 +131,7 @@ class OAuth2TokenService { // This method does the same as |StartRequest| except it uses the request // context given by |getter| instead of using the one returned by // |GetRequestContext| implemented by derived classes. - virtual scoped_ptr<Request> StartRequestWithContext( + scoped_ptr<Request> StartRequestWithContext( net::URLRequestContextGetter* getter, const ScopeSet& scopes, Consumer* consumer); @@ -190,8 +193,9 @@ class OAuth2TokenService { // Posts a task to fire the Consumer callback with the cached token. Must // Must only be called if HasCacheEntry() returns true. - scoped_ptr<Request> StartCacheLookupRequest(const ScopeSet& scopes, - Consumer* consumer); + void StartCacheLookupRequest(RequestImpl* request, + const ScopeSet& scopes, + Consumer* consumer); // Clears the internal token cache. void ClearCache(); @@ -208,11 +212,19 @@ class OAuth2TokenService { void FireRefreshTokensLoaded(); void FireRefreshTokensCleared(); - private: // Derived classes must provide a request context used for fetching access // tokens with the |StartRequest| method. virtual net::URLRequestContextGetter* GetRequestContext() = 0; + // Fetches an OAuth token for the specified client/scopes. Virtual so it can + // be overridden for tests and for platform-specific behavior on Android. + virtual void FetchOAuth2Token(RequestImpl* request, + net::URLRequestContextGetter* getter, + const std::string& client_id, + const std::string& client_secret, + const ScopeSet& scopes); + + private: // Class that fetches an OAuth2 access token for a given set of scopes and // OAuth2 refresh token. class Fetcher; diff --git a/chrome/browser/signin/profile_oauth2_token_service.cc b/chrome/browser/signin/profile_oauth2_token_service.cc index 6c00938..7dbab23 100644 --- a/chrome/browser/signin/profile_oauth2_token_service.cc +++ b/chrome/browser/signin/profile_oauth2_token_service.cc @@ -68,8 +68,6 @@ ProfileOAuth2TokenService::~ProfileOAuth2TokenService() { } void ProfileOAuth2TokenService::Initialize(Profile* profile) { - DCHECK(content::BrowserThread::CurrentlyOn(content::BrowserThread::UI)); - DCHECK(profile); DCHECK(!profile_); profile_ = profile; @@ -93,6 +91,7 @@ void ProfileOAuth2TokenService::Initialize(Profile* profile) { } void ProfileOAuth2TokenService::Shutdown() { + DCHECK(profile_) << "Shutdown() called without matching call to Initialize()"; CancelAllRequests(); signin_global_error_->RemoveProvider(this); GlobalErrorServiceFactory::GetForProfile(profile_)->RemoveGlobalError( @@ -108,6 +107,10 @@ std::string ProfileOAuth2TokenService::GetRefreshToken() { return token_service->GetOAuth2LoginRefreshToken(); } +net::URLRequestContextGetter* ProfileOAuth2TokenService::GetRequestContext() { + return profile_->GetRequestContext(); +} + void ProfileOAuth2TokenService::UpdateAuthError( const GoogleServiceAuthError& error) { // Do not report connection errors as these are not actually auth errors. @@ -164,10 +167,6 @@ GoogleServiceAuthError ProfileOAuth2TokenService::GetAuthStatus() const { return last_auth_error_; } -net::URLRequestContextGetter* ProfileOAuth2TokenService::GetRequestContext() { - return profile_->GetRequestContext(); -} - void ProfileOAuth2TokenService::RegisterCacheEntry( const std::string& refresh_token, const ScopeSet& scopes, diff --git a/chrome/browser/signin/profile_oauth2_token_service.h b/chrome/browser/signin/profile_oauth2_token_service.h index 013cc06..a920e5e 100644 --- a/chrome/browser/signin/profile_oauth2_token_service.h +++ b/chrome/browser/signin/profile_oauth2_token_service.h @@ -58,9 +58,6 @@ class ProfileOAuth2TokenService : public OAuth2TokenService, // SigninGlobalError::AuthStatusProvider implementation. virtual GoogleServiceAuthError GetAuthStatus() const OVERRIDE; - // OAuth2TokenService implementation. - virtual net::URLRequestContextGetter* GetRequestContext() OVERRIDE; - // Takes injected TokenService for testing. bool ShouldCacheForRefreshToken(TokenService *token_service, const std::string& refresh_token); @@ -94,6 +91,9 @@ class ProfileOAuth2TokenService : public OAuth2TokenService, // OAuth2TokenService overrides. virtual std::string GetRefreshToken() OVERRIDE; + // OAuth2TokenService implementation. + virtual net::URLRequestContextGetter* GetRequestContext() OVERRIDE; + // Updates the internal cache of the result from the most-recently-completed // auth request (used for reporting errors to the user). virtual void UpdateAuthError(const GoogleServiceAuthError& error) OVERRIDE; diff --git a/chrome/browser/signin/profile_oauth2_token_service_factory.cc b/chrome/browser/signin/profile_oauth2_token_service_factory.cc index e40433b..23dbcae 100644 --- a/chrome/browser/signin/profile_oauth2_token_service_factory.cc +++ b/chrome/browser/signin/profile_oauth2_token_service_factory.cc @@ -6,6 +6,7 @@ #include "chrome/browser/profiles/profile.h" #include "chrome/browser/signin/profile_oauth2_token_service.h" +#include "chrome/browser/signin/signin_manager_factory.h" #include "chrome/browser/signin/token_service_factory.h" #include "chrome/browser/ui/global_error/global_error_service_factory.h" #include "components/browser_context_keyed_service/browser_context_dependency_manager.h" @@ -15,6 +16,7 @@ ProfileOAuth2TokenServiceFactory::ProfileOAuth2TokenServiceFactory() "ProfileOAuth2TokenService", BrowserContextDependencyManager::GetInstance()) { DependsOn(GlobalErrorServiceFactory::GetInstance()); + DependsOn(SigninManagerFactory::GetInstance()); DependsOn(TokenServiceFactory::GetInstance()); } diff --git a/chrome/browser/signin/profile_oauth2_token_service_request_unittest.cc b/chrome/browser/signin/profile_oauth2_token_service_request_unittest.cc index ef1c18b..6fb6e5b 100644 --- a/chrome/browser/signin/profile_oauth2_token_service_request_unittest.cc +++ b/chrome/browser/signin/profile_oauth2_token_service_request_unittest.cc @@ -181,9 +181,11 @@ class ProfileOAuth2TokenServiceRequestTest : public testing::Test { void ProfileOAuth2TokenServiceRequestTest::SetUp() { ui_thread_.reset(new content::TestBrowserThread(content::BrowserThread::UI, &ui_loop_)); - profile_.reset(new TestingProfile()); - ProfileOAuth2TokenServiceFactory::GetInstance()->SetTestingFactory( - profile_.get(), &CreateOAuth2TokenService); + TestingProfile::Builder builder; + builder.AddTestingFactory(ProfileOAuth2TokenServiceFactory::GetInstance(), + &CreateOAuth2TokenService); + profile_ = builder.Build(); + oauth2_service_ = (MockProfileOAuth2TokenService*) ProfileOAuth2TokenServiceFactory::GetForProfile(profile_.get()); } diff --git a/chrome/browser/signin/token_service_unittest.cc b/chrome/browser/signin/token_service_unittest.cc index 0617ae5..f934764 100644 --- a/chrome/browser/signin/token_service_unittest.cc +++ b/chrome/browser/signin/token_service_unittest.cc @@ -64,7 +64,8 @@ void TokenServiceTestHarness::SetUp() { oauth_token_ = "oauth"; oauth_secret_ = "secret"; - profile_.reset(new TestingProfile()); + profile_ = CreateProfile().Pass(); + profile_->CreateWebDataService(); // Force the loading of the WebDataService. @@ -81,6 +82,10 @@ void TokenServiceTestHarness::SetUp() { service()->Initialize("test", profile_.get()); } +scoped_ptr<TestingProfile> TokenServiceTestHarness::CreateProfile() { + return make_scoped_ptr(new TestingProfile()); +} + void TokenServiceTestHarness::TearDown() { // You have to destroy the profile before the threads are shut down. profile_.reset(); diff --git a/chrome/browser/signin/token_service_unittest.h b/chrome/browser/signin/token_service_unittest.h index 34aa043..3039be3 100644 --- a/chrome/browser/signin/token_service_unittest.h +++ b/chrome/browser/signin/token_service_unittest.h @@ -63,6 +63,7 @@ class TokenServiceTestHarness : public testing::Test { virtual void SetUp() OVERRIDE; virtual void TearDown() OVERRIDE; + virtual scoped_ptr<TestingProfile> CreateProfile(); void UpdateCredentialsOnService(); TestingProfile* profile() const { return profile_.get(); } TokenService* service() const { return service_; } diff --git a/chrome/browser/signin/ubertoken_fetcher_unittest.cc b/chrome/browser/signin/ubertoken_fetcher_unittest.cc index 569025a..1592d4d 100644 --- a/chrome/browser/signin/ubertoken_fetcher_unittest.cc +++ b/chrome/browser/signin/ubertoken_fetcher_unittest.cc @@ -64,13 +64,17 @@ class UbertokenFetcherTest : public TokenServiceTestHarness { public: virtual void SetUp() OVERRIDE { TokenServiceTestHarness::SetUp(); - - ProfileOAuth2TokenServiceFactory::GetInstance()-> - SetTestingFactoryAndUse(profile(), Build); UpdateCredentialsOnService(); fetcher_.reset(new UbertokenFetcher(profile(), &consumer_)); } + virtual scoped_ptr<TestingProfile> CreateProfile() OVERRIDE { + TestingProfile::Builder builder; + builder.AddTestingFactory(ProfileOAuth2TokenServiceFactory::GetInstance(), + Build); + return builder.Build().Pass(); + } + virtual void TearDown() OVERRIDE { fetcher_.reset(); TokenServiceTestHarness::TearDown(); diff --git a/chrome/browser/sync/profile_sync_service_autofill_unittest.cc b/chrome/browser/sync/profile_sync_service_autofill_unittest.cc index 1af7644..4f03967 100644 --- a/chrome/browser/sync/profile_sync_service_autofill_unittest.cc +++ b/chrome/browser/sync/profile_sync_service_autofill_unittest.cc @@ -377,7 +377,7 @@ class AbstractAutofillFactory { public: virtual DataTypeController* CreateDataTypeController( ProfileSyncComponentsFactory* factory, - ProfileMock* profile, + TestingProfile* profile, ProfileSyncService* service) = 0; virtual void SetExpectation(ProfileSyncComponentsFactoryMock* factory, ProfileSyncService* service, @@ -390,7 +390,7 @@ class AutofillEntryFactory : public AbstractAutofillFactory { public: virtual browser_sync::DataTypeController* CreateDataTypeController( ProfileSyncComponentsFactory* factory, - ProfileMock* profile, + TestingProfile* profile, ProfileSyncService* service) OVERRIDE { return new AutofillDataTypeController(factory, profile, service); } @@ -412,7 +412,7 @@ class AutofillProfileFactory : public AbstractAutofillFactory { public: virtual browser_sync::DataTypeController* CreateDataTypeController( ProfileSyncComponentsFactory* factory, - ProfileMock* profile, + TestingProfile* profile, ProfileSyncService* service) OVERRIDE { return new AutofillProfileDataTypeController(factory, profile, service); } @@ -503,7 +503,10 @@ class ProfileSyncServiceAutofillTest virtual void SetUp() OVERRIDE { AbstractProfileSyncServiceTest::SetUp(); - profile_.reset(new ProfileMock()); + TestingProfile::Builder builder; + builder.AddTestingFactory(ProfileOAuth2TokenServiceFactory::GetInstance(), + FakeOAuth2TokenService::BuildTokenService); + profile_ = builder.Build().Pass(); web_database_.reset(new WebDatabaseFake(&autofill_table_)); MockWebDataServiceWrapper* wrapper = static_cast<MockWebDataServiceWrapper*>( @@ -524,8 +527,6 @@ class ProfileSyncServiceAutofillTest token_service_ = static_cast<TokenService*>( TokenServiceFactory::GetInstance()->SetTestingFactoryAndUse( profile_.get(), BuildTokenService)); - ProfileOAuth2TokenServiceFactory::GetInstance()->SetTestingFactory( - profile_.get(), FakeOAuth2TokenService::BuildTokenService); EXPECT_CALL(*personal_data_manager_, LoadProfiles()).Times(1); EXPECT_CALL(*personal_data_manager_, LoadCreditCards()).Times(1); @@ -757,7 +758,7 @@ class ProfileSyncServiceAutofillTest friend class AddAutofillHelper<AutofillProfile>; friend class FakeServerUpdater; - scoped_ptr<ProfileMock> profile_; + scoped_ptr<TestingProfile> profile_; AutofillTableMock autofill_table_; scoped_ptr<WebDatabaseFake> web_database_; scoped_refptr<WebDataServiceFake> web_data_service_; diff --git a/chrome/browser/sync/profile_sync_service_password_unittest.cc b/chrome/browser/sync/profile_sync_service_password_unittest.cc index 17b9523..9d3cdd6 100644 --- a/chrome/browser/sync/profile_sync_service_password_unittest.cc +++ b/chrome/browser/sync/profile_sync_service_password_unittest.cc @@ -32,7 +32,6 @@ #include "chrome/browser/sync/profile_sync_test_util.h" #include "chrome/browser/sync/test_profile_sync_service.h" #include "chrome/common/pref_names.h" -#include "chrome/test/base/profile_mock.h" #include "content/public/browser/notification_source.h" #include "content/public/common/password_form.h" #include "content/public/test/mock_notification_observer.h" @@ -154,7 +153,10 @@ class ProfileSyncServicePasswordTest : public AbstractProfileSyncServiceTest { virtual void SetUp() { AbstractProfileSyncServiceTest::SetUp(); - profile_.reset(new ProfileMock()); + TestingProfile::Builder builder; + builder.AddTestingFactory(ProfileOAuth2TokenServiceFactory::GetInstance(), + FakeOAuth2TokenService::BuildTokenService); + profile_ = builder.Build().Pass(); invalidation::InvalidationServiceFactory::GetInstance()-> SetBuildOnlyFakeInvalidatorsForTest(true); password_store_ = static_cast<MockPasswordStore*>( @@ -192,8 +194,6 @@ class ProfileSyncServicePasswordTest : public AbstractProfileSyncServiceTest { token_service_ = static_cast<TokenService*>( TokenServiceFactory::GetInstance()->SetTestingFactoryAndUse( profile_.get(), BuildTokenService)); - ProfileOAuth2TokenServiceFactory::GetInstance()->SetTestingFactory( - profile_.get(), FakeOAuth2TokenService::BuildTokenService); PasswordTestProfileSyncService* sync = static_cast<PasswordTestProfileSyncService*>( @@ -314,7 +314,7 @@ class ProfileSyncServicePasswordTest : public AbstractProfileSyncServiceTest { } content::MockNotificationObserver observer_; - scoped_ptr<ProfileMock> profile_; + scoped_ptr<TestingProfile> profile_; scoped_refptr<MockPasswordStore> password_store_; content::NotificationRegistrar registrar_; }; diff --git a/chrome/browser/sync/profile_sync_service_preference_unittest.cc b/chrome/browser/sync/profile_sync_service_preference_unittest.cc index 4d57c21..03b3e5d5 100644 --- a/chrome/browser/sync/profile_sync_service_preference_unittest.cc +++ b/chrome/browser/sync/profile_sync_service_preference_unittest.cc @@ -123,7 +123,10 @@ class ProfileSyncServicePreferenceTest virtual void SetUp() { AbstractProfileSyncServiceTest::SetUp(); - profile_.reset(new TestingProfile()); + TestingProfile::Builder builder; + builder.AddTestingFactory(ProfileOAuth2TokenServiceFactory::GetInstance(), + FakeOAuth2TokenService::BuildTokenService); + profile_ = builder.Build().Pass(); invalidation::InvalidationServiceFactory::GetInstance()-> SetBuildOnlyFakeInvalidatorsForTest(true); prefs_ = profile_->GetTestingPrefService(); @@ -156,8 +159,6 @@ class ProfileSyncServicePreferenceTest SigninManagerBase* signin = SigninManagerFactory::GetForProfile(profile_.get()); signin->SetAuthenticatedUsername("test"); - ProfileOAuth2TokenServiceFactory::GetInstance()->SetTestingFactory( - profile_.get(), FakeOAuth2TokenService::BuildTokenService); sync_service_ = static_cast<TestProfileSyncService*>( ProfileSyncServiceFactory::GetInstance()->SetTestingFactoryAndUse( profile_.get(), &TestProfileSyncService::BuildAutoStartAsyncInit)); diff --git a/chrome/browser/sync/profile_sync_service_session_unittest.cc b/chrome/browser/sync/profile_sync_service_session_unittest.cc index 592a826..1b4e173 100644 --- a/chrome/browser/sync/profile_sync_service_session_unittest.cc +++ b/chrome/browser/sync/profile_sync_service_session_unittest.cc @@ -212,13 +212,15 @@ class ProfileSyncServiceSessionTest protected: virtual TestingProfile* CreateProfile() OVERRIDE { - TestingProfile* profile = new TestingProfile(); + TestingProfile::Builder builder; + builder.AddTestingFactory(ProfileOAuth2TokenServiceFactory::GetInstance(), + FakeOAuth2TokenService::BuildTokenService); // Don't want the profile to create a real ProfileSyncService. - ProfileSyncServiceFactory::GetInstance()->SetTestingFactory(profile, - NULL); + builder.AddTestingFactory(ProfileSyncServiceFactory::GetInstance(), NULL); + scoped_ptr<TestingProfile> profile(builder.Build()); invalidation::InvalidationServiceFactory::GetInstance()-> SetBuildOnlyFakeInvalidatorsForTest(true); - return profile; + return profile.release(); } virtual void SetUp() { @@ -271,8 +273,6 @@ class ProfileSyncServiceSessionTest SigninManagerBase* signin = SigninManagerFactory::GetForProfile(profile()); signin->SetAuthenticatedUsername("test_user"); - ProfileOAuth2TokenServiceFactory::GetInstance()->SetTestingFactory( - profile(), FakeOAuth2TokenService::BuildTokenService); ProfileSyncComponentsFactoryMock* factory = new ProfileSyncComponentsFactoryMock(); sync_service_.reset(new FakeProfileSyncService( diff --git a/chrome/browser/sync/profile_sync_service_startup_unittest.cc b/chrome/browser/sync/profile_sync_service_startup_unittest.cc index f1bfd96..a1bdf31 100644 --- a/chrome/browser/sync/profile_sync_service_startup_unittest.cc +++ b/chrome/browser/sync/profile_sync_service_startup_unittest.cc @@ -68,6 +68,11 @@ class FakeTokenService : public TokenService { content::Source<TokenService>(this), content::NotificationService::NoDetails()); } + + static BrowserContextKeyedService* BuildFakeTokenService( + content::BrowserContext* profile) { + return new FakeTokenService(); + } }; class ProfileSyncServiceStartupTest : public testing::Test { @@ -76,28 +81,35 @@ class ProfileSyncServiceStartupTest : public testing::Test { : thread_bundle_(content::TestBrowserThreadBundle::REAL_DB_THREAD | content::TestBrowserThreadBundle::REAL_FILE_THREAD | content::TestBrowserThreadBundle::REAL_IO_THREAD), - profile_(new TestingProfile), sync_(NULL) {} virtual ~ProfileSyncServiceStartupTest() { } virtual void SetUp() { + profile_ = CreateProfile(); + } + + virtual scoped_ptr<TestingProfile> CreateProfile() { + TestingProfile::Builder builder; #if defined(OS_CHROMEOS) - SigninManagerFactory::GetInstance()->SetTestingFactory( - profile_.get(), FakeSigninManagerBase::Build); + builder.AddTestingFactory(SigninManagerFactory::GetInstance(), + FakeSigninManagerBase::Build); #else - SigninManagerFactory::GetInstance()->SetTestingFactory( - profile_.get(), FakeSigninManager::Build); + builder.AddTestingFactory(SigninManagerFactory::GetInstance(), + FakeSigninManager::Build); #endif + builder.AddTestingFactory(ProfileOAuth2TokenServiceFactory::GetInstance(), + FakeOAuth2TokenService::BuildTokenService); + builder.AddTestingFactory(ProfileSyncServiceFactory::GetInstance(), + BuildService); + builder.AddTestingFactory(TokenServiceFactory::GetInstance(), + FakeTokenService::BuildFakeTokenService); + return builder.Build(); } virtual void TearDown() { sync_->RemoveObserver(&observer_); - ProfileSyncServiceFactory::GetInstance()->SetTestingFactory( - profile_.get(), NULL); - ProfileOAuth2TokenServiceFactory::GetInstance()->SetTestingFactory( - profile_.get(), NULL); profile_.reset(); // Pump messages posted by the sync core thread (which may end up @@ -118,11 +130,8 @@ class ProfileSyncServiceStartupTest : public testing::Test { } void CreateSyncService() { - ProfileOAuth2TokenServiceFactory::GetInstance()->SetTestingFactory( - profile_.get(), FakeOAuth2TokenService::BuildTokenService); sync_ = static_cast<TestProfileSyncService*>( - ProfileSyncServiceFactory::GetInstance()->SetTestingFactoryAndUse( - profile_.get(), BuildService)); + ProfileSyncServiceFactory::GetForProfile(profile_.get())); sync_->AddObserver(&observer_); sync_->set_synchronous_sync_configuration(); } @@ -146,8 +155,6 @@ class ProfileSyncServiceStartupCrosTest : public ProfileSyncServiceStartupTest { public: virtual void SetUp() { ProfileSyncServiceStartupTest::SetUp(); - ProfileOAuth2TokenServiceFactory::GetInstance()->SetTestingFactory( - profile_.get(), FakeOAuth2TokenService::BuildTokenService); sync_ = static_cast<TestProfileSyncService*>( ProfileSyncServiceFactory::GetInstance()->SetTestingFactoryAndUse( profile_.get(), BuildCrosService)); @@ -173,11 +180,6 @@ class ProfileSyncServiceStartupCrosTest : public ProfileSyncServiceStartupTest { } }; -BrowserContextKeyedService* BuildFakeTokenService( - content::BrowserContext* profile) { - return new FakeTokenService(); -} - TEST_F(ProfileSyncServiceStartupTest, StartFirstTime) { // We've never completed startup. profile_->GetPrefs()->ClearPref(prefs::kSyncHasSetupCompleted); @@ -235,8 +237,7 @@ TEST_F(ProfileSyncServiceStartupTest, DISABLED_StartNoCredentials) { SigninManagerFactory::GetForProfile( profile_.get())->Initialize(profile_.get(), NULL); TokenService* token_service = static_cast<TokenService*>( - TokenServiceFactory::GetInstance()->SetTestingFactoryAndUse( - profile_.get(), BuildFakeTokenService)); + TokenServiceFactory::GetForProfile(profile_.get())); CreateSyncService(); // Should not actually start, rather just clean things up and wait @@ -327,8 +328,7 @@ TEST_F(ProfileSyncServiceStartupCrosTest, StartCrosNoCredentials) { profile_->GetPrefs()->ClearPref(prefs::kSyncHasSetupCompleted); EXPECT_CALL(observer_, OnStateChanged()).Times(AnyNumber()); TokenService* token_service = static_cast<TokenService*>( - TokenServiceFactory::GetInstance()->SetTestingFactoryAndUse( - profile_.get(), BuildFakeTokenService)); + TokenServiceFactory::GetForProfile(profile_.get())); sync_->Initialize(); // Sync should not start because there are no tokens yet. diff --git a/chrome/browser/sync/profile_sync_service_typed_url_unittest.cc b/chrome/browser/sync/profile_sync_service_typed_url_unittest.cc index 23c6edd..c1f2fd9 100644 --- a/chrome/browser/sync/profile_sync_service_typed_url_unittest.cc +++ b/chrome/browser/sync/profile_sync_service_typed_url_unittest.cc @@ -38,7 +38,6 @@ #include "chrome/browser/sync/profile_sync_service_factory.h" #include "chrome/browser/sync/profile_sync_test_util.h" #include "chrome/browser/sync/test_profile_sync_service.h" -#include "chrome/test/base/profile_mock.h" #include "chrome/test/base/testing_profile.h" #include "components/browser_context_keyed_service/refcounted_browser_context_keyed_service.h" #include "content/public/browser/notification_service.h" @@ -184,7 +183,10 @@ class ProfileSyncServiceTypedUrlTest : public AbstractProfileSyncServiceTest { virtual void SetUp() { AbstractProfileSyncServiceTest::SetUp(); - profile_.reset(new ProfileMock()); + TestingProfile::Builder builder; + builder.AddTestingFactory(ProfileOAuth2TokenServiceFactory::GetInstance(), + FakeOAuth2TokenService::BuildTokenService); + profile_ = builder.Build().Pass(); invalidation::InvalidationServiceFactory::GetInstance()-> SetBuildOnlyFakeInvalidatorsForTest(true); history_backend_ = new HistoryBackendMock(); @@ -214,8 +216,6 @@ class ProfileSyncServiceTypedUrlTest : public AbstractProfileSyncServiceTest { token_service_ = static_cast<TokenService*>( TokenServiceFactory::GetInstance()->SetTestingFactoryAndUse( profile_.get(), BuildTokenService)); - ProfileOAuth2TokenServiceFactory::GetInstance()->SetTestingFactory( - profile_.get(), FakeOAuth2TokenService::BuildTokenService); sync_service_ = static_cast<TestProfileSyncService*>( ProfileSyncServiceFactory::GetInstance()->SetTestingFactoryAndUse( profile_.get(), @@ -322,7 +322,7 @@ class ProfileSyncServiceTypedUrlTest : public AbstractProfileSyncServiceTest { scoped_ptr<Thread> history_thread_; - scoped_ptr<ProfileMock> profile_; + scoped_ptr<TestingProfile> profile_; scoped_refptr<HistoryBackendMock> history_backend_; HistoryServiceMock* history_service_; browser_sync::DataTypeErrorHandlerMock error_handler_; diff --git a/chrome/browser/sync/profile_sync_service_unittest.cc b/chrome/browser/sync/profile_sync_service_unittest.cc index ff213c2..b2d1a53 100644 --- a/chrome/browser/sync/profile_sync_service_unittest.cc +++ b/chrome/browser/sync/profile_sync_service_unittest.cc @@ -59,11 +59,12 @@ class ProfileSyncServiceTestHarness { } void SetUp() { - profile.reset(new TestingProfile()); + TestingProfile::Builder builder; + builder.AddTestingFactory(ProfileOAuth2TokenServiceFactory::GetInstance(), + FakeOAuth2TokenService::BuildTokenService); + profile = builder.Build().Pass(); invalidation::InvalidationServiceFactory::GetInstance()-> SetBuildOnlyFakeInvalidatorsForTest(true); - ProfileOAuth2TokenServiceFactory::GetInstance()->SetTestingFactory( - profile.get(), FakeOAuth2TokenService::BuildTokenService); } void TearDown() { diff --git a/chrome/browser/tab_contents/spelling_menu_observer_browsertest.cc b/chrome/browser/tab_contents/spelling_menu_observer_browsertest.cc index ae5c0a0..ad3e7fa 100644 --- a/chrome/browser/tab_contents/spelling_menu_observer_browsertest.cc +++ b/chrome/browser/tab_contents/spelling_menu_observer_browsertest.cc @@ -44,7 +44,7 @@ class MockRenderViewContextMenu : public RenderViewContextMenuProxy { string16 title; }; - MockRenderViewContextMenu(); + explicit MockRenderViewContextMenu(bool incognito); virtual ~MockRenderViewContextMenu(); // RenderViewContextMenuProxy implementation. @@ -89,9 +89,12 @@ class MockRenderViewContextMenu : public RenderViewContextMenuProxy { DISALLOW_COPY_AND_ASSIGN(MockRenderViewContextMenu); }; -MockRenderViewContextMenu::MockRenderViewContextMenu() - : observer_(NULL), - profile_(new TestingProfile) { +MockRenderViewContextMenu::MockRenderViewContextMenu(bool incognito) + : observer_(NULL) { + TestingProfile::Builder builder; + if (incognito) + builder.SetIncognito(); + profile_ = builder.Build(); } MockRenderViewContextMenu::~MockRenderViewContextMenu() { @@ -202,7 +205,7 @@ class SpellingMenuObserverTest : public InProcessBrowserTest { SpellingMenuObserverTest(); virtual void SetUpOnMainThread() OVERRIDE { - Reset(); + Reset(false); } virtual void CleanUpOnMainThread() OVERRIDE { @@ -210,9 +213,9 @@ class SpellingMenuObserverTest : public InProcessBrowserTest { menu_.reset(); } - void Reset() { + void Reset(bool incognito) { observer_.reset(); - menu_.reset(new MockRenderViewContextMenu); + menu_.reset(new MockRenderViewContextMenu(incognito)); observer_.reset(new SpellingMenuObserver(menu_.get())); menu_->SetObserver(observer_.get()); } @@ -395,7 +398,8 @@ IN_PROC_BROWSER_TEST_F(SpellingMenuObserverTest, // is functional. IN_PROC_BROWSER_TEST_F(SpellingMenuObserverTest, NoSpellingServiceWhenOffTheRecord) { - menu()->GetProfile()->AsTestingProfile()->set_incognito(true); + // Create a menu in an incognito profile. + Reset(true); // This means spellchecking is allowed. Default is that the service is // contacted but this test makes sure that if profile is incognito, that @@ -433,9 +437,6 @@ IN_PROC_BROWSER_TEST_F(SpellingMenuObserverTest, EXPECT_EQ(IDC_CONTENT_CONTEXT_SPELLING_TOGGLE, item.command_id); EXPECT_FALSE(item.enabled); EXPECT_FALSE(item.hidden); - - // Set incognito back to false to allow appropriate test cleanup. - menu()->GetProfile()->AsTestingProfile()->set_incognito(false); } // Test that the menu is preceeded by a separator if there are any suggestions, @@ -452,7 +453,7 @@ IN_PROC_BROWSER_TEST_F(SpellingMenuObserverTest, SuggestionsForceTopSeparator) { EXPECT_NE(-1, item.command_id); // Case #2. Misspelled word, suggestions, no spellcheck service. - Reset(); + Reset(false); menu()->GetPrefs()->SetBoolean(prefs::kSpellCheckUseSpellingService, false); InitMenu("asdfkj", "asdf"); @@ -463,7 +464,7 @@ IN_PROC_BROWSER_TEST_F(SpellingMenuObserverTest, SuggestionsForceTopSeparator) { EXPECT_EQ(-1, item.command_id); // Case #3. Misspelled word, suggestion service is on. - Reset(); + Reset(false); menu()->GetPrefs()->SetBoolean(prefs::kSpellCheckUseSpellingService, true); CommandLine* command_line = CommandLine::ForCurrentProcess(); command_line->AppendSwitch(switches::kUseSpellingSuggestions); diff --git a/chrome/browser/translate/translate_manager_browsertest.cc b/chrome/browser/translate/translate_manager_browsertest.cc index 4bfe4e0..f10bb02 100644 --- a/chrome/browser/translate/translate_manager_browsertest.cc +++ b/chrome/browser/translate/translate_manager_browsertest.cc @@ -1146,12 +1146,12 @@ TEST_F(TranslateManagerBrowserTest, AlwaysTranslateLanguagePref) { // case either. TestingProfile* test_profile = static_cast<TestingProfile*>(web_contents()->GetBrowserContext()); - test_profile->set_incognito(true); + test_profile->ForceIncognito(true); SimulateNavigation(GURL("http://www.youtube.fr"), "fr", true); EXPECT_FALSE(GetTranslateMessage(&page_id, &original_lang, &target_lang)); EXPECT_TRUE(GetTranslateInfoBar() != NULL); EXPECT_TRUE(CloseTranslateInfoBar()); - test_profile->set_incognito(false); // Get back to non incognito. + test_profile->ForceIncognito(false); // Get back to non incognito. // Now revert the always translate pref and make sure we go back to expected // behavior, which is show a "before translate" infobar. @@ -1291,7 +1291,7 @@ TEST_F(TranslateManagerBrowserTest, BeforeTranslateExtraButtons) { static_cast<extensions::TestExtensionSystem*>( extensions::ExtensionSystem::Get(test_profile))-> CreateExtensionProcessManager(); - test_profile->set_incognito(true); + test_profile->ForceIncognito(true); for (int i = 0; i < 8; ++i) { SCOPED_TRACE(::testing::Message() << "Iteration " << i << " incognito mode=" << test_profile->IsOffTheRecord()); @@ -1308,7 +1308,7 @@ TEST_F(TranslateManagerBrowserTest, BeforeTranslateExtraButtons) { EXPECT_TRUE(infobar->ShouldShowAlwaysTranslateShortcut()); } if (i == 3) - test_profile->set_incognito(false); + test_profile->ForceIncognito(false); } // Simulate the user pressing "Always translate French". infobar->AlwaysTranslatePageLanguage(); @@ -1324,7 +1324,7 @@ TEST_F(TranslateManagerBrowserTest, BeforeTranslateExtraButtons) { // Now test that declining the translation causes a "never translate" button // to be shown (in non incognito mode only). - test_profile->set_incognito(true); + test_profile->ForceIncognito(true); for (int i = 0; i < 8; ++i) { SCOPED_TRACE(::testing::Message() << "Iteration " << i << " incognito mode=" << test_profile->IsOffTheRecord()); @@ -1340,7 +1340,7 @@ TEST_F(TranslateManagerBrowserTest, BeforeTranslateExtraButtons) { EXPECT_TRUE(infobar->ShouldShowNeverTranslateShortcut()); } if (i == 3) - test_profile->set_incognito(false); + test_profile->ForceIncognito(false); } // Simulate the user pressing "Never translate French". infobar->NeverTranslatePageLanguage(); diff --git a/chrome/browser/ui/autofill/autofill_dialog_controller_unittest.cc b/chrome/browser/ui/autofill/autofill_dialog_controller_unittest.cc index 73aa5eb..f7d8798 100644 --- a/chrome/browser/ui/autofill/autofill_dialog_controller_unittest.cc +++ b/chrome/browser/ui/autofill/autofill_dialog_controller_unittest.cc @@ -2139,7 +2139,7 @@ TEST_F(AutofillDialogControllerTest, SaveDetailsInChrome) { controller()->MenuModelForSection(SECTION_EMAIL)->ActivatedAt(1); EXPECT_TRUE(controller()->ShouldOfferToSaveInChrome()); - profile()->set_incognito(true); + profile()->ForceIncognito(true); EXPECT_FALSE(controller()->ShouldOfferToSaveInChrome()); } diff --git a/chrome/browser/ui/bookmarks/bookmark_context_menu_controller_unittest.cc b/chrome/browser/ui/bookmarks/bookmark_context_menu_controller_unittest.cc index ef95d4b..4d19024 100644 --- a/chrome/browser/ui/bookmarks/bookmark_context_menu_controller_unittest.cc +++ b/chrome/browser/ui/bookmarks/bookmark_context_menu_controller_unittest.cc @@ -46,7 +46,13 @@ class BookmarkContextMenuControllerTest : public testing::Test { } virtual void SetUp() OVERRIDE { - profile_.reset(new TestingProfile()); + Reset(false); + } + void Reset(bool incognito) { + TestingProfile::Builder builder; + if (incognito) + builder.SetIncognito(); + profile_ = builder.Build(); profile_->CreateBookmarkModel(true); model_ = BookmarkModelFactory::GetForProfile(profile_.get()); @@ -237,11 +243,12 @@ TEST_F(BookmarkContextMenuControllerTest, MultipleFoldersWithURLs) { // Tests the enabled state of open incognito. TEST_F(BookmarkContextMenuControllerTest, DisableIncognito) { + // Create a new incognito profile. + Reset(true); std::vector<const BookmarkNode*> nodes; nodes.push_back(model_->bookmark_bar_node()->GetChild(0)); BookmarkContextMenuController controller( NULL, NULL, NULL, profile_.get(), NULL, nodes[0]->parent(), nodes); - profile_->set_incognito(true); EXPECT_FALSE(controller.IsCommandIdEnabled(IDC_BOOKMARK_BAR_OPEN_INCOGNITO)); EXPECT_FALSE( controller.IsCommandIdEnabled(IDC_BOOKMARK_BAR_OPEN_ALL_INCOGNITO)); diff --git a/chrome/browser/ui/browser_command_controller_unittest.cc b/chrome/browser/ui/browser_command_controller_unittest.cc index d37dade..ccd98cf 100644 --- a/chrome/browser/ui/browser_command_controller_unittest.cc +++ b/chrome/browser/ui/browser_command_controller_unittest.cc @@ -293,16 +293,17 @@ TEST_F(BrowserCommandControllerTest, // Set up a profile with an off the record profile. TestingProfile::Builder builder; - TestingProfile* profile2 = builder.Build().release(); - profile2->set_incognito(true); + builder.SetIncognito(); + scoped_ptr<TestingProfile> profile2(builder.Build()); TestingProfile::Builder builder2; TestingProfile* profile1 = builder2.Build().release(); profile2->SetOriginalProfile(profile1); EXPECT_EQ(profile2->GetOriginalProfile(), profile1); - profile1->SetOffTheRecordProfile(profile2); + profile1->SetOffTheRecordProfile(profile2.PassAs<Profile>()); // Create a new browser based on the off the record profile. - Browser::CreateParams profile_params(profile2, chrome::GetActiveDesktop()); + Browser::CreateParams profile_params(profile1->GetOffTheRecordProfile(), + chrome::GetActiveDesktop()); scoped_ptr<Browser> browser2( chrome::CreateBrowserWithTestWindowForParams(&profile_params)); diff --git a/chrome/browser/ui/sync/one_click_signin_helper.h b/chrome/browser/ui/sync/one_click_signin_helper.h index 5ab7595..da9a138 100644 --- a/chrome/browser/ui/sync/one_click_signin_helper.h +++ b/chrome/browser/ui/sync/one_click_signin_helper.h @@ -130,7 +130,7 @@ class OneClickSigninHelper private: friend class content::WebContentsUserData<OneClickSigninHelper>; friend class OneClickSigninHelperTest; - FRIEND_TEST_ALL_PREFIXES(OneClickSigninHelperTest, + FRIEND_TEST_ALL_PREFIXES(OneClickSigninHelperIncognitoTest, ShowInfoBarUIThreadIncognito); FRIEND_TEST_ALL_PREFIXES(OneClickSigninHelperTest, SigninFromWebstoreWithConfigSyncfirst); diff --git a/chrome/browser/ui/sync/one_click_signin_helper_unittest.cc b/chrome/browser/ui/sync/one_click_signin_helper_unittest.cc index c61d806..d8ceb51 100644 --- a/chrome/browser/ui/sync/one_click_signin_helper_unittest.cc +++ b/chrome/browser/ui/sync/one_click_signin_helper_unittest.cc @@ -205,11 +205,10 @@ class OneClickSigninHelperTest : public ChromeRenderViewHostTestHarness { virtual void SetUp() OVERRIDE; virtual void TearDown() OVERRIDE; - // Creates the sign-in manager for tests. If |use_incognito| is true then - // a WebContents for an incognito profile is created. If |username| is + // Creates the sign-in manager for tests. If |username| is // is not empty, the profile of the mock WebContents will be connected to // the given account. - void CreateSigninManager(bool use_incognito, const std::string& username); + void CreateSigninManager(const std::string& username); // Set the ID of the signin process that the test will assume to be the // only process allowed to sign the user in to Chrome. @@ -258,9 +257,7 @@ void OneClickSigninHelperTest::SetTrustedSigninProcessID(int id) { } void OneClickSigninHelperTest::CreateSigninManager( - bool use_incognito, const std::string& username) { - profile()->set_incognito(use_incognito); signin_manager_ = static_cast<SigninManagerMock*>( SigninManagerFactory::GetInstance()->SetTestingFactoryAndUse( profile(), BuildSigninManagerMock)); @@ -368,6 +365,20 @@ TestProfileIOData* OneClickSigninHelperIOTest::CreateTestProfileIOData( return io_data; } +class OneClickSigninHelperIncognitoTest : public OneClickSigninHelperTest { + protected: + // content::RenderViewHostTestHarness. + virtual content::BrowserContext* CreateBrowserContext() OVERRIDE; +}; + +content::BrowserContext* +OneClickSigninHelperIncognitoTest::CreateBrowserContext() { + // Builds an incognito profile to run this test. + TestingProfile::Builder builder; + builder.SetIncognito(); + return builder.Build().release(); +} + TEST_F(OneClickSigninHelperTest, CanOfferNoContents) { std::string error_message; EXPECT_FALSE(OneClickSigninHelper::CanOffer( @@ -387,7 +398,7 @@ TEST_F(OneClickSigninHelperTest, CanOfferNoContents) { } TEST_F(OneClickSigninHelperTest, CanOffer) { - CreateSigninManager(false, std::string()); + CreateSigninManager(std::string()); EXPECT_CALL(*signin_manager_, IsAllowedUsername(_)). WillRepeatedly(Return(true)); @@ -425,7 +436,7 @@ TEST_F(OneClickSigninHelperTest, CanOffer) { } TEST_F(OneClickSigninHelperTest, CanOfferFirstSetup) { - CreateSigninManager(false, std::string()); + CreateSigninManager(std::string()); EXPECT_CALL(*signin_manager_, IsAllowedUsername(_)). WillRepeatedly(Return(true)); @@ -455,7 +466,7 @@ TEST_F(OneClickSigninHelperTest, CanOfferFirstSetup) { } TEST_F(OneClickSigninHelperTest, CanOfferProfileConnected) { - CreateSigninManager(false, "foo@gmail.com"); + CreateSigninManager("foo@gmail.com"); EXPECT_CALL(*signin_manager_, IsAllowedUsername(_)). WillRepeatedly(Return(true)); @@ -495,7 +506,7 @@ TEST_F(OneClickSigninHelperTest, CanOfferProfileConnected) { } TEST_F(OneClickSigninHelperTest, CanOfferUsernameNotAllowed) { - CreateSigninManager(false, std::string()); + CreateSigninManager(std::string()); EXPECT_CALL(*signin_manager_, IsAllowedUsername(_)). WillRepeatedly(Return(false)); @@ -519,7 +530,7 @@ TEST_F(OneClickSigninHelperTest, CanOfferUsernameNotAllowed) { } TEST_F(OneClickSigninHelperTest, CanOfferWithRejectedEmail) { - CreateSigninManager(false, std::string()); + CreateSigninManager(std::string()); EXPECT_CALL(*signin_manager_, IsAllowedUsername(_)). WillRepeatedly(Return(true)); @@ -547,8 +558,8 @@ TEST_F(OneClickSigninHelperTest, CanOfferWithRejectedEmail) { "john@gmail.com", &error_message)); } -TEST_F(OneClickSigninHelperTest, CanOfferIncognito) { - CreateSigninManager(true, std::string()); +TEST_F(OneClickSigninHelperIncognitoTest, CanOfferIncognito) { + CreateSigninManager(std::string()); std::string error_message; EXPECT_FALSE(OneClickSigninHelper::CanOffer( @@ -568,7 +579,7 @@ TEST_F(OneClickSigninHelperTest, CanOfferIncognito) { } TEST_F(OneClickSigninHelperTest, CanOfferNoSigninCookies) { - CreateSigninManager(false, std::string()); + CreateSigninManager(std::string()); AllowSigninCookies(false); EXPECT_CALL(*signin_manager_, IsAllowedUsername(_)). @@ -592,7 +603,7 @@ TEST_F(OneClickSigninHelperTest, CanOfferNoSigninCookies) { } TEST_F(OneClickSigninHelperTest, CanOfferDisabledByPolicy) { - CreateSigninManager(false, std::string()); + CreateSigninManager(std::string()); EXPECT_CALL(*signin_manager_, IsAllowedUsername(_)). WillRepeatedly(Return(true)); @@ -626,8 +637,8 @@ TEST_F(OneClickSigninHelperTest, CanOfferDisabledByPolicy) { // Should not crash if a helper instance is not associated with an incognito // web contents. -TEST_F(OneClickSigninHelperTest, ShowInfoBarUIThreadIncognito) { - CreateSigninManager(true, std::string()); +TEST_F(OneClickSigninHelperIncognitoTest, ShowInfoBarUIThreadIncognito) { + CreateSigninManager(std::string()); OneClickSigninHelper* helper = OneClickSigninHelper::FromWebContents(web_contents()); EXPECT_EQ(NULL, helper); @@ -642,7 +653,7 @@ TEST_F(OneClickSigninHelperTest, ShowInfoBarUIThreadIncognito) { // config sync, then Chrome should redirect immediately to sync settings page, // and upon successful setup, redirect back to webstore. TEST_F(OneClickSigninHelperTest, SigninFromWebstoreWithConfigSyncfirst) { - CreateSigninManager(false, std::string()); + CreateSigninManager(std::string()); EXPECT_CALL(*signin_manager_, IsAllowedUsername(_)) .WillRepeatedly(Return(true)); diff --git a/chrome/browser/ui/views/bookmarks/bookmark_context_menu_test.cc b/chrome/browser/ui/views/bookmarks/bookmark_context_menu_test.cc index 2b83def..70b00e1 100644 --- a/chrome/browser/ui/views/bookmarks/bookmark_context_menu_test.cc +++ b/chrome/browser/ui/views/bookmarks/bookmark_context_menu_test.cc @@ -260,9 +260,12 @@ TEST_F(BookmarkContextMenuTest, MultipleFoldersWithURLs) { TEST_F(BookmarkContextMenuTest, DisableIncognito) { std::vector<const BookmarkNode*> nodes; nodes.push_back(model_->bookmark_bar_node()->GetChild(0)); + TestingProfile::Builder builder; + builder.SetIncognito(); + scoped_ptr<TestingProfile> incognito_ = builder.Build().Pass(); + incognito_->SetOriginalProfile(profile_.get()); BookmarkContextMenu controller( - NULL, NULL, profile_.get(), NULL, nodes[0]->parent(), nodes, false); - profile_->set_incognito(true); + NULL, NULL, incognito_.get(), NULL, nodes[0]->parent(), nodes, false); EXPECT_FALSE(controller.IsCommandEnabled(IDC_BOOKMARK_BAR_OPEN_INCOGNITO)); EXPECT_FALSE( controller.IsCommandEnabled(IDC_BOOKMARK_BAR_OPEN_ALL_INCOGNITO)); diff --git a/chrome/browser/ui/views/frame/browser_view_unittest.cc b/chrome/browser/ui/views/frame/browser_view_unittest.cc index d3ad284..38e6ae2 100644 --- a/chrome/browser/ui/views/frame/browser_view_unittest.cc +++ b/chrome/browser/ui/views/frame/browser_view_unittest.cc @@ -324,9 +324,9 @@ class BrowserViewIncognitoSwitcherTest : public BrowserViewTest { // visible. // This profile instance is owned by the TestingProfile instance within the // BrowserWithTestWindowTest class. - TestingProfile* incognito_profile = new TestingProfile(); - incognito_profile->set_incognito(true); - GetProfile()->SetOffTheRecordProfile(incognito_profile); + TestingProfile::Builder builder; + builder.SetIncognito(); + GetProfile()->SetOffTheRecordProfile(builder.Build()); browser_view_ = new TestBrowserView(); browser_view_->SetWindowSwitcherButton( diff --git a/chrome/chrome_tests_unit.gypi b/chrome/chrome_tests_unit.gypi index 84e565b..1ff276a 100644 --- a/chrome/chrome_tests_unit.gypi +++ b/chrome/chrome_tests_unit.gypi @@ -163,6 +163,8 @@ 'browser/search_engines/template_url_service_test_util.h', 'browser/sessions/session_service_test_helper.cc', 'browser/sessions/session_service_test_helper.h', + 'browser/signin/fake_profile_oauth2_token_service.cc', + 'browser/signin/fake_profile_oauth2_token_service.h', 'browser/signin/fake_signin_manager.cc', 'browser/signin/fake_signin_manager.h', 'browser/ssl/ssl_client_auth_requestor_mock.cc', diff --git a/chrome/test/base/testing_profile.cc b/chrome/test/base/testing_profile.cc index 0a0833e..f53c423 100644 --- a/chrome/test/base/testing_profile.cc +++ b/chrome/test/base/testing_profile.cc @@ -167,6 +167,7 @@ TestingProfile::TestingProfile() : start_time_(Time::Now()), testing_prefs_(NULL), incognito_(false), + force_incognito_(false), original_profile_(NULL), last_session_exited_cleanly_(true), browser_context_dependency_manager_( @@ -184,6 +185,7 @@ TestingProfile::TestingProfile(const base::FilePath& path) : start_time_(Time::Now()), testing_prefs_(NULL), incognito_(false), + force_incognito_(false), original_profile_(NULL), last_session_exited_cleanly_(true), profile_path_(path), @@ -200,6 +202,7 @@ TestingProfile::TestingProfile(const base::FilePath& path, : start_time_(Time::Now()), testing_prefs_(NULL), incognito_(false), + force_incognito_(false), original_profile_(NULL), last_session_exited_cleanly_(true), profile_path_(path), @@ -221,11 +224,14 @@ TestingProfile::TestingProfile( const base::FilePath& path, Delegate* delegate, scoped_refptr<ExtensionSpecialStoragePolicy> extension_policy, - scoped_ptr<PrefServiceSyncable> prefs) + scoped_ptr<PrefServiceSyncable> prefs, + bool incognito, + const TestingFactories& factories) : start_time_(Time::Now()), prefs_(prefs.release()), testing_prefs_(NULL), - incognito_(false), + incognito_(incognito), + force_incognito_(false), original_profile_(NULL), last_session_exited_cleanly_(true), extension_special_storage_policy_(extension_policy), @@ -241,6 +247,12 @@ TestingProfile::TestingProfile( profile_path_ = temp_dir_.path(); } + // Set any testing factories prior to initializing the services. + for (TestingFactories::const_iterator it = factories.begin(); + it != factories.end(); ++it) { + it->first->SetTestingFactory(this, it->second); + } + Init(); // If caller supplied a delegate, delay the FinishInit invocation until other // tasks have run. @@ -309,7 +321,11 @@ void TestingProfile::Init() { extensions::ExtensionSystemFactory::GetInstance()->SetTestingFactory( this, extensions::TestExtensionSystem::Build); - browser_context_dependency_manager_->CreateBrowserContextServices(this, true); + // If there is no separate original profile specified for this profile, then + // force preferences to be registered - this allows tests to create a + // standalone incognito profile while still having prefs registered. + browser_context_dependency_manager_->CreateBrowserContextServicesForTest( + this, !original_profile_); #if defined(ENABLE_NOTIFICATIONS) // Install profile keyed service factory hooks for dummy/test services @@ -330,6 +346,9 @@ void TestingProfile::FinishInit() { } TestingProfile::~TestingProfile() { + // Revert to non-incognito mode before shutdown. + force_incognito_ = false; + // Any objects holding live URLFetchers should be deleted before teardown. TemplateURLFetcherFactory::ShutdownForProfile(this); @@ -524,18 +543,22 @@ std::string TestingProfile::GetProfileName() { } bool TestingProfile::IsOffTheRecord() const { - return incognito_; + return force_incognito_ || incognito_; } -void TestingProfile::SetOffTheRecordProfile(Profile* profile) { - incognito_profile_.reset(profile); +void TestingProfile::SetOffTheRecordProfile(scoped_ptr<Profile> profile) { + DCHECK(!IsOffTheRecord()); + incognito_profile_ = profile.Pass(); } void TestingProfile::SetOriginalProfile(Profile* profile) { + DCHECK(IsOffTheRecord()); original_profile_ = profile; } Profile* TestingProfile::GetOffTheRecordProfile() { + if (IsOffTheRecord()) + return this; return incognito_profile_.get(); } @@ -794,7 +817,8 @@ Profile::ExitType TestingProfile::GetLastSessionExitType() { TestingProfile::Builder::Builder() : build_called_(false), - delegate_(NULL) { + delegate_(NULL), + incognito_(false) { } TestingProfile::Builder::~Builder() { @@ -818,6 +842,16 @@ void TestingProfile::Builder::SetPrefService( pref_service_ = prefs.Pass(); } +void TestingProfile::Builder::SetIncognito() { + incognito_ = true; +} + +void TestingProfile::Builder::AddTestingFactory( + BrowserContextKeyedServiceFactory* service_factory, + BrowserContextKeyedServiceFactory::FactoryFunction callback) { + testing_factories_.push_back(std::make_pair(service_factory, callback)); +} + scoped_ptr<TestingProfile> TestingProfile::Builder::Build() { DCHECK(!build_called_); build_called_ = true; @@ -825,5 +859,7 @@ scoped_ptr<TestingProfile> TestingProfile::Builder::Build() { path_, delegate_, extension_policy_, - pref_service_.Pass())); + pref_service_.Pass(), + incognito_, + testing_factories_)); } diff --git a/chrome/test/base/testing_profile.h b/chrome/test/base/testing_profile.h index 8e63e5a..03c95d4 100644 --- a/chrome/test/base/testing_profile.h +++ b/chrome/test/base/testing_profile.h @@ -11,6 +11,7 @@ #include "base/memory/ref_counted.h" #include "base/memory/scoped_ptr.h" #include "chrome/browser/profiles/profile.h" +#include "components/browser_context_keyed_service/browser_context_keyed_service_factory.h" namespace content { class MockResourceContext; @@ -59,6 +60,10 @@ class TestingProfile : public Profile { // Default constructor that cannot be used with multi-profiles. TestingProfile(); + typedef std::vector<std::pair< + BrowserContextKeyedServiceFactory*, + BrowserContextKeyedServiceFactory::FactoryFunction> > TestingFactories; + // Helper class for building an instance of TestingProfile (allows injecting // mocks for various services prior to profile initialization). // TODO(atwilson): Remove non-default constructors and various setters in @@ -68,13 +73,19 @@ class TestingProfile : public Profile { Builder(); ~Builder(); - // Sets a Delegate to be called back when the Profile is fully initialized. - // This causes the final initialization to be performed via a task so the - // caller must run a MessageLoop. Caller maintains ownership of the Delegate + // Sets a Delegate to be called back during profile init. This causes the + // final initialization to be performed via a task so the caller must run + // a MessageLoop. Caller maintains ownership of the Delegate // and must manage its lifetime so it continues to exist until profile // initialization is complete. void SetDelegate(Delegate* delegate); + // Adds a testing factory to the TestingProfile. These testing factories + // are applied before the ProfileKeyedServices are created. + void AddTestingFactory( + BrowserContextKeyedServiceFactory* service_factory, + BrowserContextKeyedServiceFactory::FactoryFunction callback); + // Sets the ExtensionSpecialStoragePolicy to be returned by // GetExtensionSpecialStoragePolicy(). void SetExtensionSpecialStoragePolicy( @@ -86,6 +97,9 @@ class TestingProfile : public Profile { // Sets the PrefService to be used by this profile. void SetPrefService(scoped_ptr<PrefServiceSyncable> prefs); + // Makes the Profile being built an incognito profile. + void SetIncognito(); + // Creates the TestingProfile using previously-set settings. scoped_ptr<TestingProfile> Build(); @@ -98,6 +112,8 @@ class TestingProfile : public Profile { scoped_refptr<ExtensionSpecialStoragePolicy> extension_policy_; base::FilePath path_; Delegate* delegate_; + bool incognito_; + TestingFactories testing_factories_; DISALLOW_COPY_AND_ASSIGN(Builder); }; @@ -120,7 +136,9 @@ class TestingProfile : public Profile { TestingProfile(const base::FilePath& path, Delegate* delegate, scoped_refptr<ExtensionSpecialStoragePolicy> extension_policy, - scoped_ptr<PrefServiceSyncable> prefs); + scoped_ptr<PrefServiceSyncable> prefs, + bool incognito, + const TestingFactories& factories); virtual ~TestingProfile(); @@ -186,9 +204,26 @@ class TestingProfile : public Profile { virtual TestingProfile* AsTestingProfile() OVERRIDE; virtual std::string GetProfileName() OVERRIDE; - void set_incognito(bool incognito) { incognito_ = incognito; } + + // DEPRECATED, because it's fragile to change a profile from non-incognito + // to incognito after the ProfileKeyedServices have been created (some + // ProfileKeyedServices either should not exist in incognito mode, or will + // crash when they try to get references to other services they depend on, + // but do not exist in incognito mode). + // TODO(atwilson): Remove this API (http://crbug.com/277296). + // + // Changes a profile's to/from incognito mode temporarily - profile will be + // returned to non-incognito before destruction to allow services to + // properly shutdown. This is only supported for legacy tests - new tests + // should create a true incognito profile using Builder::SetIncognito() or + // by using the TestingProfile constructor that allows setting the incognito + // flag. + void ForceIncognito(bool force_incognito) { + force_incognito_ = force_incognito; + } + // Assumes ownership. - virtual void SetOffTheRecordProfile(Profile* profile); + virtual void SetOffTheRecordProfile(scoped_ptr<Profile> profile); virtual void SetOriginalProfile(Profile* profile); virtual Profile* GetOffTheRecordProfile() OVERRIDE; virtual void DestroyOffTheRecordProfile() OVERRIDE {} @@ -306,6 +341,7 @@ class TestingProfile : public Profile { std::wstring id_; bool incognito_; + bool force_incognito_; scoped_ptr<Profile> incognito_profile_; Profile* original_profile_; diff --git a/chrome/test/base/testing_profile_manager.cc b/chrome/test/base/testing_profile_manager.cc index e6f4748..644760c 100644 --- a/chrome/test/base/testing_profile_manager.cc +++ b/chrome/test/base/testing_profile_manager.cc @@ -16,10 +16,10 @@ namespace testing { -class ProfileManager : public ::ProfileManager { +class ProfileManager : public ::ProfileManagerWithoutInit { public: explicit ProfileManager(const base::FilePath& user_data_dir) - : ::ProfileManager(user_data_dir) {} + : ::ProfileManagerWithoutInit(user_data_dir) {} protected: virtual Profile* CreateProfileHelper( @@ -57,11 +57,10 @@ TestingProfile* TestingProfileManager::CreateTestingProfile( profile_path = profile_path.AppendASCII(profile_name); // Create the profile and register it. - TestingProfile* profile = new TestingProfile( - profile_path, - NULL, - scoped_refptr<ExtensionSpecialStoragePolicy>(), - prefs.Pass()); + TestingProfile::Builder builder; + builder.SetPath(profile_path); + builder.SetPrefService(prefs.Pass()); + TestingProfile* profile = builder.Build().release(); profile_manager_->AddProfile(profile); // Takes ownership. // Update the user metadata. diff --git a/components/autofill/core/browser/personal_data_manager_unittest.cc b/components/autofill/core/browser/personal_data_manager_unittest.cc index 7c08e5f..0a2b582 100644 --- a/components/autofill/core/browser/personal_data_manager_unittest.cc +++ b/components/autofill/core/browser/personal_data_manager_unittest.cc @@ -99,7 +99,9 @@ class PersonalDataManagerTest : public testing::Test { } void MakeProfileIncognito() { - profile_->set_incognito(true); + // Switch to an incognito profile. + profile_->ForceIncognito(true); + DCHECK(profile_->IsOffTheRecord()); } base::MessageLoopForUI message_loop_; diff --git a/components/browser_context_keyed_service/browser_context_dependency_manager.cc b/components/browser_context_keyed_service/browser_context_dependency_manager.cc index 1031f4e..66134233 100644 --- a/components/browser_context_keyed_service/browser_context_dependency_manager.cc +++ b/components/browser_context_keyed_service/browser_context_dependency_manager.cc @@ -40,9 +40,22 @@ void BrowserContextDependencyManager::AddEdge( } void BrowserContextDependencyManager::CreateBrowserContextServices( - content::BrowserContext* context, bool is_testing_context) { + content::BrowserContext* context) { + DoCreateBrowserContextServices(context, false, false); +} + +void BrowserContextDependencyManager::CreateBrowserContextServicesForTest( + content::BrowserContext* context, + bool force_register_prefs) { + DoCreateBrowserContextServices(context, true, force_register_prefs); +} + +void BrowserContextDependencyManager::DoCreateBrowserContextServices( + content::BrowserContext* context, + bool is_testing_context, + bool force_register_prefs) { TRACE_EVENT0("browser", - "BrowserContextDependencyManager::CreateBrowserContextServices") + "BrowserContextDependencyManager::DoCreateBrowserContextServices") #ifndef NDEBUG // Unmark |context| as dead. This exists because of unit tests, which will // often have similar stack structures. 0xWhatever might be created, go out @@ -64,9 +77,12 @@ void BrowserContextDependencyManager::CreateBrowserContextServices( BrowserContextKeyedBaseFactory* factory = static_cast<BrowserContextKeyedBaseFactory*>(construction_order[i]); - if (!context->IsOffTheRecord()) { + if (!context->IsOffTheRecord() || force_register_prefs) { // We only register preferences on normal contexts because the incognito - // context shares the pref service with the normal one. + // context shares the pref service with the normal one. Always register + // for standalone testing contexts (testing contexts that don't have an + // "original" profile set) as otherwise the preferences won't be + // registered. factory->RegisterUserPrefsOnBrowserContext(context); } diff --git a/components/browser_context_keyed_service/browser_context_dependency_manager.h b/components/browser_context_keyed_service/browser_context_dependency_manager.h index 3e96eff..5482aaa 100644 --- a/components/browser_context_keyed_service/browser_context_dependency_manager.h +++ b/components/browser_context_keyed_service/browser_context_dependency_manager.h @@ -34,12 +34,22 @@ class BROWSER_CONTEXT_KEYED_SERVICE_EXPORT BrowserContextDependencyManager { BrowserContextKeyedBaseFactory* dependee); // Called by each BrowserContext to alert us of its creation. Several services - // want to be started when a context is created. Testing configuration is also - // done at this time. (If you want your BrowserContextKeyedService to be - // started with the BrowserContext, override BrowserContextKeyedBaseFactory:: - // ServiceIsCreatedWithBrowserContext() to return true.) - void CreateBrowserContextServices(content::BrowserContext* context, - bool is_testing_context); + // want to be started when a context is created. If you want your + // BrowserContextKeyedService to be started with the BrowserContext, override + // BrowserContextKeyedBaseFactory::ServiceIsCreatedWithBrowserContext() to + // return true. This method also registers any service-related preferences + // for non-incognito profiles. + void CreateBrowserContextServices(content::BrowserContext* context); + + // Similar to CreateBrowserContextServices(), except this is used for creating + // test BrowserContexts - these contexts will not create services for any + // BrowserContextKeyedBaseFactories that return true from + // ServiceIsNULLWhileTesting(). Callers can pass |force_register_prefs| as + // true to have preferences registered even for incognito profiles - this + // allows tests to specify a standalone incognito profile without an + // associated normal profile. + void CreateBrowserContextServicesForTest(content::BrowserContext* context, + bool force_register_prefs); // Called by each BrowserContext to alert us that we should destroy services // associated with it. @@ -58,6 +68,11 @@ class BROWSER_CONTEXT_KEYED_SERVICE_EXPORT BrowserContextDependencyManager { friend class BrowserContextDependencyManagerUnittests; friend struct DefaultSingletonTraits<BrowserContextDependencyManager>; + // Helper function used by CreateBrowserContextServices[ForTest]. + void DoCreateBrowserContextServices(content::BrowserContext* context, + bool is_testing_context, + bool force_register_prefs); + BrowserContextDependencyManager(); virtual ~BrowserContextDependencyManager(); diff --git a/components/browser_context_keyed_service/browser_context_keyed_base_factory.cc b/components/browser_context_keyed_service/browser_context_keyed_base_factory.cc index e6caa69..cc3f487 100644 --- a/components/browser_context_keyed_service/browser_context_keyed_base_factory.cc +++ b/components/browser_context_keyed_service/browser_context_keyed_base_factory.cc @@ -68,7 +68,6 @@ void BrowserContextKeyedBaseFactory::RegisterUserPrefsOnBrowserContext( // to enforce a uniquenes check here because some tests create one context and // multiple services of the same type attached to that context (serially, not // parallel) and we don't want to register multiple times on the same context. - DCHECK(!context->IsOffTheRecord()); std::set<content::BrowserContext*>::iterator it = registered_preferences_.find(context); |