Skip to content

Commit

Permalink
Converted OAuth2AccessTokenFetcher to using SimplerURLLoader.
Browse files Browse the repository at this point in the history
This impacted a lot of APIs that now need to be provided with a
SharedURLLoaderFactory. Note that in many of these APIs we still need
also the net::URLRequestContextGetter as other GAIA requests still use
the networ API directly. Once all GAIA code uses the network service,
all APIs will be changed to only take the SharedURLLoaderFactory.

Many tests that simulate URL responses had to be changed and use a
TestURLLoaderFactory. In some instances, we need the
TestURLLoaderFactory for the OAuth token but still mock the URLRequests
directly for other operations.

Also:
- split up OAuth2AccessTokenFetcherImplTest.MakeGetAccessTokenBody
  into smaller tests
- exposing ways to simulate URL responses directly in
  TestURLLoaderFactory as it was required by some tests

Bug: 840396
Cq-Include-Trybots: luci.chromium.try:ios-simulator-full-configs;luci.chromium.try:linux_mojo;master.tryserver.chromium.mac:ios-simulator-cronet
Change-Id: I6d1d1081238aa5e1d8dc5f1469e4c752c93729fc
Reviewed-on: https://chromium-review.googlesource.com/1096309
Commit-Queue: John Abd-El-Malek <jam@chromium.org>
Reviewed-by: Maksim Ivanov <emaxx@chromium.org>
Reviewed-by: John Abd-El-Malek <jam@chromium.org>
Reviewed-by: Mattias Nissler <mnissler@chromium.org>
Reviewed-by: Roger Tawa <rogerta@chromium.org>
Cr-Commit-Position: refs/heads/master@{#569408}
  • Loading branch information
jcivelli-google authored and Commit Bot committed Jun 21, 2018
1 parent 4d25127 commit 6ab180d
Show file tree
Hide file tree
Showing 124 changed files with 1,558 additions and 888 deletions.
5 changes: 5 additions & 0 deletions chrome/browser/android/signin/signin_manager_android.cc
Original file line number Diff line number Diff line change
Expand Up @@ -44,6 +44,7 @@
#include "components/signin/core/browser/signin_pref_names.h"
#include "content/public/browser/browsing_data_filter_builder.h"
#include "content/public/browser/browsing_data_remover.h"
#include "content/public/browser/storage_partition.h"
#include "google_apis/gaia/gaia_auth_util.h"
#include "google_apis/gaia/gaia_constants.h"
#include "jni/SigninManager_jni.h"
Expand Down Expand Up @@ -172,11 +173,15 @@ void SigninManagerAndroid::FetchPolicyBeforeSignIn(
if (!dm_token_.empty()) {
policy::UserPolicySigninService* service =
policy::UserPolicySigninServiceFactory::GetForProfile(profile_);
scoped_refptr<network::SharedURLLoaderFactory> url_loader_factory =
content::BrowserContext::GetDefaultStoragePartition(profile_)
->GetURLLoaderFactoryForBrowserProcess();
service->FetchPolicyForSignedInUser(
AccountTrackerServiceFactory::GetForProfile(profile_)
->FindAccountInfoByEmail(username_)
.GetAccountId(),
dm_token_, client_id_, profile_->GetRequestContext(),
url_loader_factory,
base::Bind(&SigninManagerAndroid::OnPolicyFetchDone,
weak_factory_.GetWeakPtr()));
dm_token_.clear();
Expand Down
4 changes: 3 additions & 1 deletion chrome/browser/browser_process_impl.cc
Original file line number Diff line number Diff line change
Expand Up @@ -1130,7 +1130,9 @@ void BrowserProcessImpl::PreMainMessageLoopRun() {
// requires that threads are running; this Init() call lets the connector
// resume its initialization now that the loops are spinning and the
// system request context is available for the fetchers.
browser_policy_connector()->Init(local_state(), system_request_context());
browser_policy_connector()->Init(
local_state(), system_request_context(),
system_network_context_manager()->GetSharedURLLoaderFactory());

if (local_state_->IsManagedPreference(prefs::kDefaultBrowserSettingEnabled))
ApplyDefaultBrowserPolicy();
Expand Down
10 changes: 9 additions & 1 deletion chrome/browser/chromeos/drive/drive_integration_service.cc
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,7 @@
#include "chrome/browser/download/download_core_service_factory.h"
#include "chrome/browser/download/download_prefs.h"
#include "chrome/browser/drive/drive_notification_manager_factory.h"
#include "chrome/browser/net/system_network_context_manager.h"
#include "chrome/browser/profiles/incognito_helpers.h"
#include "chrome/browser/profiles/profile.h"
#include "chrome/browser/signin/profile_oauth2_token_service_factory.h"
Expand Down Expand Up @@ -56,6 +57,7 @@
#include "net/traffic_annotation/network_traffic_annotation.h"
#include "services/device/public/mojom/constants.mojom.h"
#include "services/device/public/mojom/wake_lock_provider.mojom.h"
#include "services/network/public/cpp/shared_url_loader_factory.h"
#include "services/service_manager/public/cpp/connector.h"
#include "storage/browser/fileapi/external_mount_points.h"
#include "ui/base/l10n/l10n_util.h"
Expand Down Expand Up @@ -362,9 +364,15 @@ DriveIntegrationService::DriveIntegrationService(
if (test_drive_service) {
drive_service_.reset(test_drive_service);
} else {
scoped_refptr<network::SharedURLLoaderFactory> url_loader_factory;
if (g_browser_process->system_network_context_manager()) {
// system_network_context_manager() returns nullptr in unit-tests.
url_loader_factory = g_browser_process->system_network_context_manager()
->GetSharedURLLoaderFactory();
}
drive_service_.reset(new DriveAPIService(
oauth_service, g_browser_process->system_request_context(),
blocking_task_runner_.get(),
url_loader_factory, blocking_task_runner_.get(),
GURL(google_apis::DriveApiUrlGenerator::kBaseUrlForProduction),
GURL(google_apis::DriveApiUrlGenerator::kBaseThumbnailUrlForProduction),
GetDriveUserAgent(), NO_TRAFFIC_ANNOTATION_YET));
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -26,6 +26,7 @@
#include "chrome/browser/chromeos/fileapi/external_file_url_util.h"
#include "chrome/browser/chromeos/fileapi/file_system_backend.h"
#include "chrome/browser/chromeos/profiles/profile_helper.h"
#include "chrome/browser/net/system_network_context_manager.h"
#include "chrome/browser/profiles/profile.h"
#include "chrome/browser/profiles/profile_manager.h"
#include "chrome/browser/signin/profile_oauth2_token_service_factory.h"
Expand All @@ -39,10 +40,12 @@
#include "components/signin/core/browser/profile_oauth2_token_service.h"
#include "components/signin/core/browser/signin_manager.h"
#include "content/public/browser/browser_thread.h"
#include "content/public/browser/storage_partition.h"
#include "google_apis/drive/auth_service.h"
#include "google_apis/drive/drive_api_url_generator.h"
#include "google_apis/drive/drive_switches.h"
#include "mojo/public/cpp/bindings/callback_helpers.h"
#include "services/network/public/cpp/shared_url_loader_factory.h"
#include "storage/common/fileapi/file_system_info.h"
#include "storage/common/fileapi/file_system_util.h"
#include "url/gurl.h"
Expand Down Expand Up @@ -1522,9 +1525,12 @@ void FileManagerPrivateInternalGetDownloadUrlFunction::OnGotDownloadUrl(
std::vector<std::string> scopes;
scopes.emplace_back("https://www.googleapis.com/auth/drive.readonly");

scoped_refptr<network::SharedURLLoaderFactory> url_loader_factory =
content::BrowserContext::GetDefaultStoragePartition(GetProfile())
->GetURLLoaderFactoryForBrowserProcess();
auth_service_ = std::make_unique<google_apis::AuthService>(
oauth2_token_service, account_id, GetProfile()->GetRequestContext(),
scopes);
url_loader_factory, scopes);
auth_service_->StartAuthentication(base::Bind(
&FileManagerPrivateInternalGetDownloadUrlFunction::OnTokenFetched, this));
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -35,6 +35,7 @@
#include "chrome/browser/devtools/devtools_window.h"
#include "chrome/browser/extensions/devtools_util.h"
#include "chrome/browser/lifetime/application_lifetime.h"
#include "chrome/browser/net/system_network_context_manager.h"
#include "chrome/browser/profiles/profile.h"
#include "chrome/browser/profiles/profile_manager.h"
#include "chrome/browser/profiles/profiles_state.h"
Expand Down Expand Up @@ -63,6 +64,7 @@
#include "extensions/browser/app_window/app_window.h"
#include "extensions/browser/app_window/app_window_registry.h"
#include "google_apis/drive/auth_service.h"
#include "services/network/public/cpp/shared_url_loader_factory.h"
#include "storage/browser/fileapi/external_mount_points.h"
#include "storage/common/fileapi/file_system_types.h"
#include "ui/base/webui/web_ui_util.h"
Expand Down Expand Up @@ -374,7 +376,10 @@ bool FileManagerPrivateRequestWebStoreAccessTokenFunction::RunAsync() {
SigninManagerFactory::GetForProfile(GetProfile());
auth_service_ = std::make_unique<google_apis::AuthService>(
oauth_service, signin_manager->GetAuthenticatedAccountId(),
url_request_context_getter, scopes);
url_request_context_getter,
g_browser_process->system_network_context_manager()
->GetSharedURLLoaderFactory(),
scopes);
auth_service_->StartAuthentication(base::Bind(
&FileManagerPrivateRequestWebStoreAccessTokenFunction::
OnAccessTokenFetched,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -20,12 +20,14 @@
#include "chrome/browser/chromeos/policy/enrollment_status_chromeos.h"
#include "chrome/browser/chromeos/policy/policy_oauth2_token_fetcher.h"
#include "chrome/browser/chromeos/profiles/profile_helper.h"
#include "chrome/browser/net/system_network_context_manager.h"
#include "chromeos/chromeos_switches.h"
#include "chromeos/dbus/dbus_thread_manager.h"
#include "components/policy/core/common/cloud/cloud_policy_constants.h"
#include "google_apis/gaia/gaia_auth_consumer.h"
#include "google_apis/gaia/gaia_auth_fetcher.h"
#include "google_apis/gaia/gaia_constants.h"
#include "services/network/public/cpp/shared_url_loader_factory.h"

namespace {

Expand Down Expand Up @@ -98,6 +100,8 @@ void EnterpriseEnrollmentHelperImpl::EnrollUsingAuthCode(
oauth_fetcher_.reset(policy::PolicyOAuth2TokenFetcher::CreateInstance());
oauth_fetcher_->StartWithAuthCode(
auth_code, g_browser_process->system_request_context(),
g_browser_process->system_network_context_manager()
->GetSharedURLLoaderFactory(),
base::Bind(&EnterpriseEnrollmentHelperImpl::OnTokenFetched,
weak_ptr_factory_.GetWeakPtr(),
fetch_additional_token /* is_additional_token */));
Expand Down Expand Up @@ -267,6 +271,8 @@ void EnterpriseEnrollmentHelperImpl::OnTokenFetched(
oauth_fetcher_.reset(policy::PolicyOAuth2TokenFetcher::CreateInstance());
oauth_fetcher_->StartWithRefreshToken(
refresh_token, g_browser_process->system_request_context(),
g_browser_process->system_network_context_manager()
->GetSharedURLLoaderFactory(),
base::Bind(&EnterpriseEnrollmentHelperImpl::OnTokenFetched,
weak_ptr_factory_.GetWeakPtr(),
false /* is_additional_token */));
Expand Down
11 changes: 10 additions & 1 deletion chrome/browser/chromeos/login/existing_user_controller.cc
Original file line number Diff line number Diff line change
Expand Up @@ -51,6 +51,7 @@
#include "chrome/browser/chromeos/profiles/profile_helper.h"
#include "chrome/browser/chromeos/settings/cros_settings.h"
#include "chrome/browser/chromeos/system/device_disabling_manager.h"
#include "chrome/browser/net/system_network_context_manager.h"
#include "chrome/browser/profiles/profile.h"
#include "chrome/browser/ui/aura/accessibility/automation_manager_aura.h"
#include "chrome/browser/ui/webui/chromeos/login/l10n_util.h"
Expand Down Expand Up @@ -98,6 +99,7 @@
#include "net/http/http_transaction_factory.h"
#include "net/url_request/url_request_context.h"
#include "net/url_request/url_request_context_getter.h"
#include "services/network/public/cpp/shared_url_loader_factory.h"
#include "ui/accessibility/ax_enums.mojom.h"
#include "ui/base/l10n/l10n_util.h"
#include "ui/views/widget/widget.h"
Expand Down Expand Up @@ -1067,10 +1069,17 @@ void ExistingUserController::OnOldEncryptionDetected(
// Use signin profile request context
net::URLRequestContextGetter* const signin_profile_context =
ProfileHelper::GetSigninProfile()->GetRequestContext();
scoped_refptr<network::SharedURLLoaderFactory>
sigin_profile_url_loader_factory =
content::BrowserContext::GetDefaultStoragePartition(
ProfileHelper::GetSigninProfile())
->GetURLLoaderFactoryForBrowserProcess();

auto cloud_policy_client = std::make_unique<policy::CloudPolicyClient>(
std::string() /* machine_id */, std::string() /* machine_model */,
std::string() /* brand_code */, device_management_service,
signin_profile_context, nullptr /* signing_service */,
signin_profile_context, sigin_profile_url_loader_factory,
nullptr /* signing_service */,
chromeos::GetDeviceDMTokenForUserPolicyGetter(
user_context.GetAccountId()));
pre_signin_policy_fetcher_ = std::make_unique<policy::PreSigninPolicyFetcher>(
Expand Down
4 changes: 3 additions & 1 deletion chrome/browser/chromeos/oauth2_token_service_delegate.cc
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,7 @@
#include "base/logging.h"
#include "chromeos/account_manager/account_manager.h"
#include "components/signin/core/browser/account_tracker_service.h"
#include "services/network/public/cpp/shared_url_loader_factory.h"

namespace chromeos {

Expand All @@ -30,6 +31,7 @@ OAuth2AccessTokenFetcher*
ChromeOSOAuth2TokenServiceDelegate::CreateAccessTokenFetcher(
const std::string& account_id,
net::URLRequestContextGetter* getter,
scoped_refptr<network::SharedURLLoaderFactory> url_loader_factory,
OAuth2AccessTokenConsumer* consumer) {
DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);
DCHECK_EQ(LOAD_CREDENTIALS_FINISHED_WITH_SUCCESS, load_credentials_state_);
Expand All @@ -41,7 +43,7 @@ ChromeOSOAuth2TokenServiceDelegate::CreateAccessTokenFetcher(

// |OAuth2TokenService| will manage the lifetime of the released pointer.
return account_manager_
->CreateAccessTokenFetcher(account_key, getter, consumer)
->CreateAccessTokenFetcher(account_key, url_loader_factory, consumer)
.release();
}

Expand Down
1 change: 1 addition & 0 deletions chrome/browser/chromeos/oauth2_token_service_delegate.h
Original file line number Diff line number Diff line change
Expand Up @@ -36,6 +36,7 @@ class ChromeOSOAuth2TokenServiceDelegate : public OAuth2TokenServiceDelegate,
OAuth2AccessTokenFetcher* CreateAccessTokenFetcher(
const std::string& account_id,
net::URLRequestContextGetter* getter,
scoped_refptr<network::SharedURLLoaderFactory> url_loader_factory,
OAuth2AccessTokenConsumer* consumer) override;
bool RefreshTokenIsAvailable(const std::string& account_id) const override;
void UpdateAuthError(const std::string& account_id,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,7 @@

#include "base/macros.h"
#include "base/memory/ptr_util.h"
#include "base/memory/ref_counted.h"
#include "chrome/browser/chrome_notification_types.h"
#include "chrome/browser/chromeos/login/users/fake_chrome_user_manager.h"
#include "chrome/browser/chromeos/settings/cros_settings.h"
Expand All @@ -35,6 +36,8 @@
#include "content/public/browser/notification_details.h"
#include "content/public/browser/notification_service.h"
#include "content/public/test/test_browser_thread_bundle.h"
#include "services/network/public/cpp/weak_wrapper_shared_url_loader_factory.h"
#include "services/network/test/test_url_loader_factory.h"
#include "testing/gtest/include/gtest/gtest.h"

namespace policy {
Expand Down Expand Up @@ -137,6 +140,9 @@ class AffiliatedInvalidationServiceProviderImplTest : public testing::Test {
std::unique_ptr<chromeos::ScopedTestDeviceSettingsService>
test_device_settings_service_;
std::unique_ptr<chromeos::ScopedTestCrosSettings> test_cros_settings_;
network::TestURLLoaderFactory test_url_loader_factory_;
scoped_refptr<network::WeakWrapperSharedURLLoaderFactory>
test_shared_loader_factory_;
TestingProfileManager profile_manager_;
};

Expand Down Expand Up @@ -197,6 +203,9 @@ AffiliatedInvalidationServiceProviderImplTest::
chromeos::ScopedStubInstallAttributes::CreateCloudManaged(
"example.com",
"device_id")),
test_shared_loader_factory_(
base::MakeRefCounted<network::WeakWrapperSharedURLLoaderFactory>(
&test_url_loader_factory_)),
profile_manager_(TestingBrowserProcess::GetGlobal()) {}

void AffiliatedInvalidationServiceProviderImplTest::SetUp() {
Expand All @@ -207,7 +216,8 @@ void AffiliatedInvalidationServiceProviderImplTest::SetUp() {
test_device_settings_service_.reset(new
chromeos::ScopedTestDeviceSettingsService);
test_cros_settings_.reset(new chromeos::ScopedTestCrosSettings);
chromeos::DeviceOAuth2TokenServiceFactory::Initialize();
chromeos::DeviceOAuth2TokenServiceFactory::Initialize(
test_shared_loader_factory_);

invalidation::ProfileInvalidationProviderFactory::GetInstance()->
RegisterTestingFactory(BuildProfileInvalidationProvider);
Expand All @@ -219,6 +229,7 @@ void AffiliatedInvalidationServiceProviderImplTest::TearDown() {
consumer_.reset();
provider_->Shutdown();
provider_.reset();
test_shared_loader_factory_->Detach();

invalidation::ProfileInvalidationProviderFactory::GetInstance()->
RegisterTestingFactory(nullptr);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -63,6 +63,7 @@
#include "content/public/browser/browser_thread.h"
#include "google_apis/gaia/gaia_auth_util.h"
#include "net/url_request/url_request_context_getter.h"
#include "services/network/public/cpp/shared_url_loader_factory.h"

using content::BrowserThread;

Expand Down Expand Up @@ -147,9 +148,11 @@ BrowserPolicyConnectorChromeOS::~BrowserPolicyConnectorChromeOS() {}

void BrowserPolicyConnectorChromeOS::Init(
PrefService* local_state,
scoped_refptr<net::URLRequestContextGetter> request_context) {
scoped_refptr<net::URLRequestContextGetter> request_context,
scoped_refptr<network::SharedURLLoaderFactory> url_loader_factory) {
local_state_ = local_state;
ChromeBrowserPolicyConnector::Init(local_state, request_context);
ChromeBrowserPolicyConnector::Init(local_state, request_context,
url_loader_factory);

affiliated_invalidation_service_provider_ =
std::make_unique<AffiliatedInvalidationServiceProviderImpl>();
Expand Down Expand Up @@ -177,7 +180,7 @@ void BrowserPolicyConnectorChromeOS::Init(
GetBackgroundTaskRunner(),
content::BrowserThread::GetTaskRunnerForThread(
content::BrowserThread::IO),
request_context);
request_context, url_loader_factory);
device_local_account_policy_service_->Connect(device_management_service());
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -30,10 +30,6 @@ class AttestationFlow;

} // namespace chromeos

namespace net {
class URLRequestContextGetter;
}

namespace policy {

class AffiliatedCloudPolicyInvalidator;
Expand All @@ -60,9 +56,10 @@ class BrowserPolicyConnectorChromeOS
~BrowserPolicyConnectorChromeOS() override;

// ChromeBrowserPolicyConnector:
void Init(
PrefService* local_state,
scoped_refptr<net::URLRequestContextGetter> request_context) override;
void Init(PrefService* local_state,
scoped_refptr<net::URLRequestContextGetter> request_context,
scoped_refptr<network::SharedURLLoaderFactory> url_loader_factory)
override;

// Checks whether this devices is under any kind of enterprise management.
bool IsEnterpriseManaged() const override;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -191,7 +191,8 @@ void CloudExternalDataPolicyObserverTest::SetUp() {
base::ThreadTaskRunnerHandle::Get(),
base::ThreadTaskRunnerHandle::Get(),
base::ThreadTaskRunnerHandle::Get(),
base::ThreadTaskRunnerHandle::Get(), nullptr));
base::ThreadTaskRunnerHandle::Get(), /*request_context=*/nullptr,
/*url_loader_factory=*/nullptr));
url_fetcher_factory_.set_remove_fetcher_on_delete(true);

EXPECT_CALL(user_policy_provider_, IsInitializationComplete(_))
Expand Down
10 changes: 10 additions & 0 deletions chrome/browser/chromeos/policy/device_cloud_policy_initializer.cc
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,7 @@
#include "chrome/browser/chromeos/policy/enrollment_status_chromeos.h"
#include "chrome/browser/chromeos/policy/server_backed_device_state.h"
#include "chrome/browser/chromeos/settings/install_attributes.h"
#include "chrome/browser/net/system_network_context_manager.h"
#include "chrome/common/chrome_content_client.h"
#include "chrome/common/pref_names.h"
#include "chromeos/attestation/attestation.pb.h"
Expand Down Expand Up @@ -70,6 +71,11 @@ void DeviceCloudPolicyInitializer::SetSigningServiceForTesting(
signing_service_ = std::move(signing_service);
}

void DeviceCloudPolicyInitializer::SetSystemURLLoaderFactoryForTesting(
scoped_refptr<network::SharedURLLoaderFactory> system_url_loader_factory) {
system_url_loader_factory_for_testing_ = system_url_loader_factory;
}

DeviceCloudPolicyInitializer::~DeviceCloudPolicyInitializer() {
DCHECK(!is_initialized_);
}
Expand Down Expand Up @@ -297,6 +303,10 @@ std::unique_ptr<CloudPolicyClient> DeviceCloudPolicyInitializer::CreateClient(
return std::make_unique<CloudPolicyClient>(
statistics_provider_->GetEnterpriseMachineID(), machine_model, brand_code,
device_management_service, g_browser_process->system_request_context(),
system_url_loader_factory_for_testing_
? system_url_loader_factory_for_testing_
: g_browser_process->system_network_context_manager()
->GetSharedURLLoaderFactory(),
signing_service_.get(), CloudPolicyClient::DeviceDMTokenCallback());
}

Expand Down
Loading

0 comments on commit 6ab180d

Please sign in to comment.