Port ExtensionDownloader away from IdentityProvider

IdentityProvider is deprecated as a general-purpose means of interacting
with the user's Google accounts. This CL ports ExtensionDownloader
away from it, having it instead take in the OAuth2TokenService directly
as well as a callback that returns the account to use with the webstore.
It also clarifies the lifetime relationship between ExtensionDownloader
and the ProfileOAuth2TokenService/SigninManager instances on which it
depends for authentication.

In the long term ExtensionDownloader will be ported to interact with the
Identity Service for its use case. This CL is an incremental step on the
path that also fills the more near-term goal of eliminating usages of
IdentityProvider.

Bug: 809966, 809927
Change-Id: I6b82cc318bd807d559e1701391ed3e02fcb489aa
Reviewed-on: https://chromium-review.googlesource.com/1051810
Commit-Queue: Colin Blundell <blundell@chromium.org>
Reviewed-by: Devlin <rdevlin.cronin@chromium.org>
Cr-Commit-Position: refs/heads/master@{#561024}

chrome/browser/extensions/extension_system_factory.cc

@@ -9,6 +9,7 @@
 #include "chrome/browser/extensions/install_verifier_factory.h"
 #include "chrome/browser/policy/profile_policy_connector_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/browser/ui/global_error/global_error_service_factory.h"
 #include "components/keyed_service/content/browser_context_dependency_manager.h"
@@ -44,7 +45,7 @@ ExtensionSystemSharedFactory::ExtensionSystemSharedFactory()
         BrowserContextDependencyManager::GetInstance()) {
   DependsOn(ExtensionPrefsFactory::GetInstance());
   DependsOn(ExtensionManagementFactory::GetInstance());
-  // This depends on ExtensionService which depends on ExtensionRegistry.
+  // This depends on ExtensionService, which depends on ExtensionRegistry.
   DependsOn(ExtensionRegistryFactory::GetInstance());
   DependsOn(GlobalErrorServiceFactory::GetInstance());
   DependsOn(InstallVerifierFactory::GetInstance());
@@ -54,8 +55,9 @@ ExtensionSystemSharedFactory::ExtensionSystemSharedFactory()
   DependsOn(BlacklistFactory::GetInstance());
   DependsOn(DeclarativeUserScriptManagerFactory::GetInstance());
   DependsOn(EventRouterFactory::GetInstance());
-  // This depends on ExtensionDownloader which depends on
-  // ProfileIdentityProvider which depends on SigninManager.
+  // This depends on ExtensionDownloader, which depends on
+  // ProfileOAuth2TokenService and SigninManager for webstore authentication.
+  DependsOn(ProfileOAuth2TokenServiceFactory::GetInstance());
   DependsOn(SigninManagerFactory::GetInstance());
 }
 

chrome/browser/extensions/updater/chrome_extension_downloader_factory.cc

@@ -13,12 +13,11 @@
 #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 "components/signin/core/browser/profile_identity_provider.h"
+#include "components/signin/core/browser/profile_oauth2_token_service.h"
 #include "components/signin/core/browser/signin_manager.h"
 #include "components/update_client/update_query_params.h"
 #include "content/public/common/service_manager_connection.h"
 #include "extensions/browser/updater/extension_downloader.h"
-#include "google_apis/gaia/identity_provider.h"
 
 using extensions::ExtensionDownloader;
 using extensions::ExtensionDownloaderDelegate;
@@ -53,15 +52,27 @@ std::unique_ptr<ExtensionDownloader>
 ChromeExtensionDownloaderFactory::CreateForProfile(
     Profile* profile,
     ExtensionDownloaderDelegate* delegate) {
-  std::unique_ptr<IdentityProvider> identity_provider(
-      new ProfileIdentityProvider(
-          SigninManagerFactory::GetForProfile(profile),
-          ProfileOAuth2TokenServiceFactory::GetForProfile(profile)));
   service_manager::Connector* connector =
       content::ServiceManagerConnection::GetForProcess()->GetConnector();
   std::unique_ptr<ExtensionDownloader> downloader =
       CreateForRequestContext(profile->GetRequestContext(), delegate,
         connector);
-  downloader->SetWebstoreIdentityProvider(std::move(identity_provider));
+
+  // NOTE: It is not obvious why it is OK to pass raw pointers to the token
+  // service and signin manager here. The logic is as follows:
+  // ExtensionDownloader is owned by ExtensionUpdater.
+  // ExtensionUpdater is owned by ExtensionService.
+  // ExtensionService is owned by ExtensionSystemImpl::Shared.
+  // ExtensionSystemImpl::Shared is a KeyedService. Its factory
+  // (ExtensionSystemSharedFactory) specifies that it depends on SigninManager
+  // and ProfileOAuth2TokenService.
+  // Hence, the SigninManager and ProfileOAuth2TokenService instances are
+  // guaranteed to outlive |downloader|.
+  // TODO(843519): Make this lifetime relationship more explicit/cleaner.
+  downloader->SetWebstoreAuthenticationCapabilities(
+      base::BindRepeating(
+          &SigninManagerBase::GetAuthenticatedAccountId,
+          base::Unretained(SigninManagerFactory::GetForProfile(profile))),
+      ProfileOAuth2TokenServiceFactory::GetForProfile(profile));
   return downloader;
 }

chrome/browser/extensions/updater/extension_updater_unittest.cc

@@ -64,7 +64,6 @@
 #include "extensions/common/extension.h"
 #include "extensions/common/extension_urls.h"
 #include "extensions/common/manifest_constants.h"
-#include "google_apis/gaia/fake_identity_provider.h"
 #include "google_apis/gaia/fake_oauth2_token_service.h"
 #include "net/base/backoff_entry.h"
 #include "net/base/escape.h"
@@ -140,7 +139,6 @@ int kExpectedLoadFlags =
 int kExpectedLoadFlagsForDownloadWithCookies = net::LOAD_DISABLE_CACHE;
 
 // Fake authentication constants
-const char kFakeAccountId[] = "bobloblaw@lawblog.example.com";
 const char kFakeOAuth2Token[] = "ce n'est pas un jeton";
 
 const ManifestFetchData::PingData kNeverPingedData(
@@ -303,6 +301,7 @@ class MockService : public TestExtensionService {
   explicit MockService(TestExtensionPrefs* prefs)
       : prefs_(prefs),
         pending_extension_manager_(prefs->profile()),
+        fake_account_id_("bobloblaw@lawblog.example.com"),
         downloader_delegate_override_(NULL) {}
 
   ~MockService() override {}
@@ -327,6 +326,8 @@ class MockService : public TestExtensionService {
     return fake_token_service_.get();
   }
 
+  const std::string& fake_account_id() { return fake_account_id_; }
+
   // Creates test extensions and inserts them into list. The name and
   // version are all based on their index. If |update_url| is non-null, it
   // will be used as the update_url for each extension.
@@ -383,19 +384,19 @@ class MockService : public TestExtensionService {
 
   std::unique_ptr<ExtensionDownloader> CreateExtensionDownloaderWithIdentity(
       ExtensionDownloaderDelegate* delegate) {
-    std::unique_ptr<FakeIdentityProvider> fake_identity_provider;
     fake_token_service_.reset(new FakeOAuth2TokenService());
-    fake_identity_provider.reset(new FakeIdentityProvider(
-          fake_token_service_.get()));
-    fake_identity_provider->LogIn(kFakeAccountId);
-    fake_token_service_->AddAccount(kFakeAccountId);
+    fake_token_service_->AddAccount(fake_account_id_);
 
     std::unique_ptr<ExtensionDownloader> downloader(
         CreateExtensionDownloader(delegate));
-    downloader->SetWebstoreIdentityProvider(std::move(fake_identity_provider));
+    downloader->SetWebstoreAuthenticationCapabilities(
+        base::BindRepeating(&MockService::fake_account_id,
+                            base::Unretained(this)),
+        fake_token_service_.get());
     return downloader;
   }
 
+  std::string fake_account_id_;
   std::unique_ptr<FakeOAuth2TokenService> fake_token_service_;
 
   ExtensionDownloaderDelegate* downloader_delegate_override_;
@@ -1389,7 +1390,7 @@ class ExtensionUpdaterTest : public testing::Test {
 
     if (service->fake_token_service()) {
       service->fake_token_service()->IssueAllTokensForAccount(
-          kFakeAccountId, kFakeOAuth2Token, base::Time::Now());
+          service->fake_account_id(), kFakeOAuth2Token, base::Time::Now());
     }
     RunUntilIdle();
 

extensions/browser/updater/extension_downloader.cc

@@ -34,7 +34,6 @@
 #include "extensions/common/extension_updater_uma.h"
 #include "extensions/common/extension_urls.h"
 #include "extensions/common/manifest_url_handlers.h"
-#include "google_apis/gaia/identity_provider.h"
 #include "net/base/backoff_entry.h"
 #include "net/base/load_flags.h"
 #include "net/base/net_errors.h"
@@ -212,7 +211,8 @@ ExtensionDownloader::ExtensionDownloader(
           &kDefaultBackoffPolicy,
           base::BindRepeating(&ExtensionDownloader::CreateExtensionFetcher,
                               base::Unretained(this))),
-      extension_cache_(NULL),
+      extension_cache_(nullptr),
+      token_service_(nullptr),
       weak_ptr_factory_(this) {
   DCHECK(delegate_);
   DCHECK(request_context_.get());
@@ -309,9 +309,11 @@ void ExtensionDownloader::StartBlacklistUpdate(
   StartUpdateCheck(std::move(blacklist_fetch));
 }
 
-void ExtensionDownloader::SetWebstoreIdentityProvider(
-    std::unique_ptr<IdentityProvider> identity_provider) {
-  identity_provider_.swap(identity_provider);
+void ExtensionDownloader::SetWebstoreAuthenticationCapabilities(
+    const GetWebstoreAccountCallback& webstore_account_callback,
+    OAuth2TokenService* token_service) {
+  webstore_account_callback_ = webstore_account_callback;
+  token_service_ = token_service;
 }
 
 // static
@@ -884,12 +886,12 @@ void ExtensionDownloader::CreateExtensionFetcher() {
       // We should try OAuth2, but we have no token cached. This
       // ExtensionFetcher will be started once the token fetch is complete,
       // in either OnTokenFetchSuccess or OnTokenFetchFailure.
-      DCHECK(identity_provider_.get());
+      DCHECK(token_service_);
+      DCHECK(!webstore_account_callback_.is_null());
       OAuth2TokenService::ScopeSet webstore_scopes;
       webstore_scopes.insert(kWebstoreOAuth2Scope);
-      access_token_request_ =
-          identity_provider_->GetTokenService()->StartRequest(
-              identity_provider_->GetActiveAccountId(), webstore_scopes, this);
+      access_token_request_ = token_service_->StartRequest(
+          webstore_account_callback_.Run(), webstore_scopes, this);
       return;
     }
     extension_fetcher_->AddExtraRequestHeader(
@@ -1002,7 +1004,7 @@ bool ExtensionDownloader::IterateFetchCredentialsAfterFailure(
   // fetch.
   switch (fetch->credentials) {
     case ExtensionFetch::CREDENTIALS_NONE:
-      if (fetch->url.DomainIs(kGoogleDotCom) && identity_provider_) {
+      if (fetch->url.DomainIs(kGoogleDotCom) && token_service_) {
         fetch->credentials = ExtensionFetch::CREDENTIALS_OAUTH2_TOKEN;
       } else {
         fetch->credentials = ExtensionFetch::CREDENTIALS_COOKIES;
@@ -1014,12 +1016,12 @@ bool ExtensionDownloader::IterateFetchCredentialsAfterFailure(
       // should invalidate the token and try again.
       if (response_code == net::HTTP_UNAUTHORIZED &&
           fetch->oauth2_attempt_count <= kMaxOAuth2Attempts) {
-        DCHECK(identity_provider_.get());
+        DCHECK(token_service_);
+        DCHECK(!webstore_account_callback_.is_null());
         OAuth2TokenService::ScopeSet webstore_scopes;
         webstore_scopes.insert(kWebstoreOAuth2Scope);
-        identity_provider_->GetTokenService()->InvalidateAccessToken(
-            identity_provider_->GetActiveAccountId(), webstore_scopes,
-            access_token_);
+        token_service_->InvalidateAccessToken(webstore_account_callback_.Run(),
+                                              webstore_scopes, access_token_);
         access_token_.clear();
         return true;
       }

extensions/browser/updater/extension_downloader.h

@@ -25,8 +25,6 @@
 #include "net/url_request/url_fetcher_delegate.h"
 #include "url/gurl.h"
 
-class IdentityProvider;
-
 namespace net {
 class URLFetcher;
 class URLRequestContextGetter;
@@ -60,9 +58,13 @@ class ExtensionDownloader : public net::URLFetcherDelegate,
  public:
   // A closure which constructs a new ExtensionDownloader to be owned by the
   // caller.
-  typedef base::Callback<std::unique_ptr<ExtensionDownloader>(
-      ExtensionDownloaderDelegate* delegate)>
-      Factory;
+  using Factory = base::RepeatingCallback<std::unique_ptr<ExtensionDownloader>(
+      ExtensionDownloaderDelegate* delegate)>;
+
+  // A closure that returns the account to use for authentication to the
+  // webstore.
+  using GetWebstoreAccountCallback =
+      base::RepeatingCallback<const std::string&()>;
 
   // |delegate| is stored as a raw pointer and must outlive the
   // ExtensionDownloader.
@@ -109,10 +111,12 @@ class ExtensionDownloader : public net::URLFetcherDelegate,
                             const ManifestFetchData::PingData& ping_data,
                             int request_id);
 
-  // Sets an IdentityProvider to be used for OAuth2 authentication on protected
-  // Webstore downloads.
-  void SetWebstoreIdentityProvider(
-      std::unique_ptr<IdentityProvider> identity_provider);
+  // Sets GetWebstoreAccountCallback and TokenService instances to be used for
+  // OAuth2 authentication on protected Webstore downloads. Both objects must be
+  // valid to use for the lifetime of this object.
+  void SetWebstoreAuthenticationCapabilities(
+      const GetWebstoreAccountCallback& webstore_account_callback,
+      OAuth2TokenService* token_service);
 
   void set_brand_code(const std::string& brand_code) {
     brand_code_ = brand_code;
@@ -348,9 +352,13 @@ class ExtensionDownloader : public net::URLFetcherDelegate,
   // Cache for .crx files.
   ExtensionCache* extension_cache_;
 
-  // An IdentityProvider which may be used for authentication on protected
-  // download requests. May be NULL.
-  std::unique_ptr<IdentityProvider> identity_provider_;
+  // Gets the account to use for protected download requests. May be null. If
+  // non-null, valid to call for the lifetime of this object.
+  GetWebstoreAccountCallback webstore_account_callback_;
+
+  // May be used to fetch access tokens for protected download requests. May be
+  // null. If non-null, guaranteed to outlive this object.
+  OAuth2TokenService* token_service_;
 
   // A Webstore download-scoped access token for the |identity_provider_|'s
   // active account, if any.