Refactoring: Move ownership of |ArcAuthContext|

Move ownership of |ArcAuthContext| from |ArcSessionManager| to
|ArcBackgroundAuthCodeFetcher|.

ARC++ assumed that only one account will be synced between Chrome OS and
ARC++, i.e. the Device Account. This assumption is no longer true
because of the introduction of Chrome OS Account Manager (Design Doc is
linked in the bug id).
Syncing multiple accounts requires the ability to create multiple auth
code fetchers which in turn require multiple |ArcAuthContext|s. This was
not possible currently because of the ownership of a single
|ArcAuthContext| by |ArcSessionManager|.

|ArcSessionManager| does not really need an |ArcAuthContext|. It was
needed only for:

  - Passing the context to |ArcBackgroundAuthCodeFetcher|: Not required
  if |ArcBackgroundAuthCodeFetcher| creates and owns the context.
  - Getting an instance of Token Service: Can be retrieved directly via
  |ProfileOAuth2TokenServiceFactory|.
  - |account_id| and |full_account_id|: Can be retrieved directly via
  |SigninManagerBase|.

Bug: 871690
Test: browser_tests --gtest_filter="*ArcAuthServiceTest*"
Change-Id: I78a6d2aa6693529f67f480967258c9fb00469e02
Reviewed-on: https://chromium-review.googlesource.com/1171237
Commit-Queue: Kush Sinha <sinhak@chromium.org>
Reviewed-by: Hidehiko Abe <hidehiko@chromium.org>
Reviewed-by: Xiyuan Xia <xiyuan@chromium.org>
Reviewed-by: Lutz Justen <ljusten@chromium.org>
Cr-Commit-Position: refs/heads/master@{#585456}

chrome/browser/chromeos/arc/arc_session_manager.cc

@@ -4,6 +4,7 @@
 
 #include "chrome/browser/chromeos/arc/arc_session_manager.h"
 
+#include <string>
 #include <utility>
 
 #include "base/bind.h"
@@ -18,7 +19,6 @@
 #include "chrome/browser/chromeos/arc/arc_optin_uma.h"
 #include "chrome/browser/chromeos/arc/arc_support_host.h"
 #include "chrome/browser/chromeos/arc/arc_util.h"
-#include "chrome/browser/chromeos/arc/auth/arc_auth_context.h"
 #include "chrome/browser/chromeos/arc/auth/arc_auth_service.h"
 #include "chrome/browser/chromeos/arc/optin/arc_terms_of_service_default_negotiator.h"
 #include "chrome/browser/chromeos/arc/optin/arc_terms_of_service_oobe_negotiator.h"
@@ -456,8 +456,6 @@ void ArcSessionManager::Initialize() {
       cryptohome::Identification(
           multi_user_util::GetAccountIdFromProfile(profile_)));
 
-  context_ = std::make_unique<ArcAuthContext>(profile_);
-
   if (g_enable_check_android_management_in_tests.value_or(g_ui_enabled))
     ArcAndroidManagementChecker::StartClient();
 
@@ -488,7 +486,6 @@ void ArcSessionManager::Shutdown() {
     support_host_->Close();
     support_host_.reset();
   }
-  context_.reset();
   pai_starter_.reset();
   fast_app_reinstall_starter_.reset();
   profile_ = nullptr;
@@ -876,8 +873,7 @@ void ArcSessionManager::StartAndroidManagementCheck() {
     return;
 
   android_management_checker_ = std::make_unique<ArcAndroidManagementChecker>(
-      profile_, context_->token_service(), context_->account_id(),
-      false /* retry_on_error */);
+      profile_, false /* retry_on_error */);
   android_management_checker_->StartCheck(
       base::Bind(&ArcSessionManager::OnAndroidManagementChecked,
                  weak_ptr_factory_.GetWeakPtr()));
@@ -931,8 +927,7 @@ void ArcSessionManager::StartBackgroundAndroidManagementCheck() {
   }
 
   android_management_checker_ = std::make_unique<ArcAndroidManagementChecker>(
-      profile_, context_->token_service(), context_->account_id(),
-      true /* retry_on_error */);
+      profile_, true /* retry_on_error */);
   android_management_checker_->StartCheck(
       base::Bind(&ArcSessionManager::OnBackgroundAndroidManagementChecked,
                  weak_ptr_factory_.GetWeakPtr()));
@@ -975,8 +970,8 @@ void ArcSessionManager::StartArc() {
   provisioning_reported_ = false;
 
   std::string locale;
-  std::string preferred_lanaguages;
-  GetLocaleAndPreferredLanguages(profile_, &locale, &preferred_lanaguages);
+  std::string preferred_languages;
+  GetLocaleAndPreferredLanguages(profile_, &locale, &preferred_languages);
 
   ArcSession::UpgradeParams params;
 
@@ -991,9 +986,9 @@ void ArcSessionManager::StartArc() {
   params.is_child = profile_->IsChild();
   params.supervision_transition = GetSupervisionTransition(profile_);
   params.locale = locale;
-  // Empty |preferred_lanaguages| is converted to empty array.
+  // Empty |preferred_languages| is converted to empty array.
   params.preferred_languages = base::SplitString(
-      preferred_lanaguages, ",", base::TRIM_WHITESPACE, base::SPLIT_WANT_ALL);
+      preferred_languages, ",", base::TRIM_WHITESPACE, base::SPLIT_WANT_ALL);
 
   arc_session_runner_->RequestUpgrade(std::move(params));
 }
@@ -1056,7 +1051,7 @@ void ArcSessionManager::MaybeReenableArc() {
   DCHECK_EQ(state_, State::STOPPED);
 
   if (!reenable_arc_) {
-    // Reenabling is not triggered. Do nothing.
+    // Re-enabling is not triggered. Do nothing.
     return;
   }
   DCHECK(enable_requested_);

chrome/browser/chromeos/arc/arc_session_manager.h

@@ -7,7 +7,6 @@
 
 #include <memory>
 #include <ostream>
-#include <string>
 
 #include "base/macros.h"
 #include "base/memory/weak_ptr.h"
@@ -25,7 +24,6 @@ class Profile;
 namespace arc {
 
 class ArcAndroidManagementChecker;
-class ArcAuthContext;
 class ArcDataRemover;
 class ArcFastAppReinstallStarter;
 class ArcPaiStarter;
@@ -219,10 +217,6 @@ class ArcSessionManager : public ArcSessionRunner::Observer,
 
   ArcSupportHost* support_host() { return support_host_.get(); }
 
-  // TODO(hidehiko): Get rid of the getter by migration between ArcAuthContext
-  // and ArcAuthCodeFetcher.
-  ArcAuthContext* auth_context() { return context_.get(); }
-
   // On provisioning completion (regardless of whether successfully done or
   // not), this is called with its status. On success, called with
   // ProvisioningResult::SUCCESS, otherwise |result| is the error reason.
@@ -386,7 +380,6 @@ class ArcSessionManager : public ArcSessionRunner::Observer,
 
   std::unique_ptr<ArcTermsOfServiceNegotiator> terms_of_service_negotiator_;
 
-  std::unique_ptr<ArcAuthContext> context_;
   std::unique_ptr<ArcAndroidManagementChecker> android_management_checker_;
 
   std::unique_ptr<ScopedOptInFlowTracker> scoped_opt_in_tracker_;

chrome/browser/chromeos/arc/auth/arc_auth_context.cc

@@ -7,12 +7,10 @@
 #include <utility>
 
 #include "base/strings/stringprintf.h"
-#include "base/strings/utf_string_conversions.h"
 #include "chrome/browser/chromeos/arc/arc_support_host.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/signin/signin_ui_util.h"
 #include "chrome/browser/ui/app_list/arc/arc_app_utils.h"
 #include "components/signin/core/browser/profile_oauth2_token_service.h"
 #include "components/signin/core/browser/signin_manager_base.h"
@@ -66,9 +64,6 @@ ArcAuthContext::ArcAuthContext(Profile* profile)
       SigninManagerFactory::GetForProfile(profile);
   CHECK(token_service_ && signin_manager);
   account_id_ = signin_manager->GetAuthenticatedAccountId();
-
-  full_account_id_ = base::UTF16ToUTF8(
-      signin_ui_util::GetAuthenticatedUsername(signin_manager));
 }
 
 ArcAuthContext::~ArcAuthContext() {

chrome/browser/chromeos/arc/auth/arc_auth_context.h

@@ -31,11 +31,9 @@ class ArcAuthContext : public UbertokenConsumer,
   ~ArcAuthContext() override;
 
   ProfileOAuth2TokenService* token_service() { return token_service_; }
-  const std::string& account_id() const { return account_id_; }
 
-  // Returns full account id, including dots that are removed in CrOS for
-  // the default account id.
-  const std::string& full_account_id() const { return full_account_id_; }
+  // TODO(sinhak): Check usages of |account_id()| and see if we can remove it.
+  const std::string& account_id() const { return account_id_; }
 
   // Prepares the context. Calling while an inflight operation exists will
   // cancel the inflight operation.
@@ -73,7 +71,6 @@ class ArcAuthContext : public UbertokenConsumer,
   ProfileOAuth2TokenService* token_service_;
 
   std::string account_id_;
-  std::string full_account_id_;
 
   // Whether the merge session should be skipped. Set to true only in testing.
   bool skip_merge_session_for_testing_ = false;

chrome/browser/chromeos/arc/auth/arc_auth_service.cc

@@ -9,6 +9,7 @@
 #include "base/bind.h"
 #include "base/command_line.h"
 #include "base/memory/singleton.h"
+#include "base/strings/utf_string_conversions.h"
 #include "base/time/time.h"
 #include "chrome/browser/browser_process.h"
 #include "chrome/browser/chromeos/arc/arc_optin_uma.h"
@@ -22,6 +23,8 @@
 #include "chrome/browser/lifetime/application_lifetime.h"
 #include "chrome/browser/profiles/profile.h"
 #include "chrome/browser/signin/account_tracker_service_factory.h"
+#include "chrome/browser/signin/signin_manager_factory.h"
+#include "chrome/browser/signin/signin_ui_util.h"
 #include "chrome/browser/ui/app_list/arc/arc_data_removal_dialog.h"
 #include "chromeos/account_manager/account_manager_factory.h"
 #include "chromeos/chromeos_switches.h"
@@ -340,9 +343,13 @@ void ArcAuthService::RequestAccountInfo(bool initial_signin) {
     auth_code_fetcher = std::make_unique<ArcRobotAuthCodeFetcher>();
   } else {
     // Optionally retrieve auth code in silent mode.
-    auth_code_fetcher = std::make_unique<ArcBackgroundAuthCodeFetcher>(
-        url_loader_factory_, profile_, ArcSessionManager::Get()->auth_context(),
-        initial_signin);
+    auto background_auth_code_fetcher =
+        std::make_unique<ArcBackgroundAuthCodeFetcher>(
+            url_loader_factory_, profile_, initial_signin);
+    if (skip_merge_session_for_testing_)
+      background_auth_code_fetcher->SkipMergeSessionForTesting();
+
+    auth_code_fetcher = std::move(background_auth_code_fetcher);
   }
   auth_code_fetcher->Fetch(base::Bind(&ArcAuthService::OnAuthCodeFetched,
                                       weak_ptr_factory_.GetWeakPtr()));
@@ -433,11 +440,14 @@ void ArcAuthService::OnAuthCodeFetched(bool success,
   fetcher_.reset();
 
   if (success) {
+    const SigninManagerBase* const signin_manager =
+        SigninManagerFactory::GetForProfile(profile_);
+    const std::string& full_account_id = base::UTF16ToUTF8(
+        signin_ui_util::GetAuthenticatedUsername(signin_manager));
     OnAccountInfoReady(
-        CreateAccountInfo(
-            !IsArcOptInVerificationDisabled(), auth_code,
-            ArcSessionManager::Get()->auth_context()->full_account_id(),
-            GetAccountType(profile_), policy_util::IsAccountManaged(profile_)),
+        CreateAccountInfo(!IsArcOptInVerificationDisabled(), auth_code,
+                          full_account_id, GetAccountType(profile_),
+                          policy_util::IsAccountManaged(profile_)),
         mojom::ArcSignInStatus::SUCCESS);
   } else if (chromeos::DemoSession::Get() &&
              chromeos::DemoSession::Get()->started()) {
@@ -488,4 +498,8 @@ bool ArcAuthService::IsDeviceAccount(
   return account_mapper_util_.IsEqual(account_key, device_account_id);
 }
 
+void ArcAuthService::SkipMergeSessionForTesting() {
+  skip_merge_session_for_testing_ = true;
+}
+
 }  // namespace arc

chrome/browser/chromeos/arc/auth/arc_auth_service.h

@@ -75,6 +75,8 @@ class ArcAuthService : public KeyedService,
   void OnAccountRemoved(
       const chromeos::AccountManager::AccountKey& account_key) override;
 
+  void SkipMergeSessionForTesting();
+
  private:
   // Callbacks when auth info is fetched.
   void OnEnrollmentTokenFetched(
@@ -108,6 +110,8 @@ class ArcAuthService : public KeyedService,
 
   std::unique_ptr<ArcFetcherBase> fetcher_;
 
+  bool skip_merge_session_for_testing_ = false;
+
   base::WeakPtrFactory<ArcAuthService> weak_ptr_factory_;
 
   DISALLOW_COPY_AND_ASSIGN(ArcAuthService);

chrome/browser/chromeos/arc/auth/arc_auth_service_browsertest.cc

@@ -240,11 +240,6 @@ class ArcAuthServiceTest : public InProcessBrowserTest {
 
     ArcServiceLauncher::Get()->OnPrimaryUserProfilePrepared(profile());
 
-    // It is non-trivial to navigate through the merge session in a testing
-    // context; currently we just skip it.
-    // TODO(blundell): Figure out how to enable this flow.
-    ArcSessionManager::Get()->auth_context()->SkipMergeSessionForTesting();
-
     auth_service_ = ArcAuthService::GetForBrowserContext(profile());
     DCHECK(auth_service_);
 
@@ -252,6 +247,10 @@ class ArcAuthServiceTest : public InProcessBrowserTest {
         base::MakeRefCounted<network::WeakWrapperSharedURLLoaderFactory>(
             &test_url_loader_factory_);
     auth_service_->SetURLLoaderFactoryForTesting(test_shared_loader_factory_);
+    // It is non-trivial to navigate through the merge session in a testing
+    // context; currently we just skip it.
+    // TODO(blundell): Figure out how to enable this flow.
+    auth_service_->SkipMergeSessionForTesting();
 
     arc_bridge_service_ = ArcServiceManager::Get()->arc_bridge_service();
     DCHECK(arc_bridge_service_);

chrome/browser/chromeos/arc/auth/arc_background_auth_code_fetcher.cc

@@ -51,12 +51,11 @@ const char kAuthTokenExchangeEndPoint[] =
 ArcBackgroundAuthCodeFetcher::ArcBackgroundAuthCodeFetcher(
     scoped_refptr<network::SharedURLLoaderFactory> url_loader_factory,
     Profile* profile,
-    ArcAuthContext* context,
     bool initial_signin)
     : OAuth2TokenService::Consumer(kConsumerName),
       url_loader_factory_(std::move(url_loader_factory)),
       profile_(profile),
-      context_(context),
+      context_(profile_),
       initial_signin_(initial_signin),
       weak_ptr_factory_(this) {}
 
@@ -65,8 +64,8 @@ ArcBackgroundAuthCodeFetcher::~ArcBackgroundAuthCodeFetcher() = default;
 void ArcBackgroundAuthCodeFetcher::Fetch(const FetchCallback& callback) {
   DCHECK(callback_.is_null());
   callback_ = callback;
-  context_->Prepare(base::Bind(&ArcBackgroundAuthCodeFetcher::OnPrepared,
-                               weak_ptr_factory_.GetWeakPtr()));
+  context_.Prepare(base::Bind(&ArcBackgroundAuthCodeFetcher::OnPrepared,
+                              weak_ptr_factory_.GetWeakPtr()));
 }
 
 void ArcBackgroundAuthCodeFetcher::OnPrepared(
@@ -77,8 +76,8 @@ void ArcBackgroundAuthCodeFetcher::OnPrepared(
   }
 
   // Get token service and account ID to fetch auth tokens.
-  ProfileOAuth2TokenService* const token_service = context_->token_service();
-  const std::string& account_id = context_->account_id();
+  ProfileOAuth2TokenService* const token_service = context_.token_service();
+  const std::string& account_id = context_.account_id();
   DCHECK(token_service->RefreshTokenIsAvailable(account_id));
 
   OAuth2TokenService::ScopeSet scopes;
@@ -237,4 +236,8 @@ void ArcBackgroundAuthCodeFetcher::ReportResult(
   std::move(callback_).Run(!auth_code.empty(), auth_code);
 }
 
+void ArcBackgroundAuthCodeFetcher::SkipMergeSessionForTesting() {
+  context_.SkipMergeSessionForTesting();
+}
+
 }  // namespace arc

chrome/browser/chromeos/arc/auth/arc_background_auth_code_fetcher.h

@@ -41,13 +41,14 @@ class ArcBackgroundAuthCodeFetcher : public ArcAuthCodeFetcher,
   ArcBackgroundAuthCodeFetcher(
       scoped_refptr<network::SharedURLLoaderFactory> url_loader_factory,
       Profile* profile,
-      ArcAuthContext* context,
       bool initial_signin);
   ~ArcBackgroundAuthCodeFetcher() override;
 
   // ArcAuthCodeFetcher:
   void Fetch(const FetchCallback& callback) override;
 
+  void SkipMergeSessionForTesting();
+
  private:
   void ResetFetchers();
   void OnPrepared(net::URLRequestContextGetter* request_context_getter);
@@ -65,9 +66,9 @@ class ArcBackgroundAuthCodeFetcher : public ArcAuthCodeFetcher,
                     OptInSilentAuthCode uma_status);
 
   scoped_refptr<network::SharedURLLoaderFactory> url_loader_factory_;
-  // Unowned pointers.
+  // Unowned pointer.
   Profile* const profile_;
-  ArcAuthContext* const context_;
+  ArcAuthContext context_;
   FetchCallback callback_;
 
   std::unique_ptr<OAuth2TokenService::Request> login_token_request_;

chrome/browser/chromeos/arc/policy/arc_android_management_checker.cc

@@ -15,9 +15,12 @@
 #include "chrome/browser/chromeos/policy/browser_policy_connector_chromeos.h"
 #include "chrome/browser/net/system_network_context_manager.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 "components/policy/core/browser/browser_policy_connector.h"
 #include "components/policy/core/common/cloud/device_management_service.h"
 #include "components/signin/core/browser/profile_oauth2_token_service.h"
+#include "components/signin/core/browser/signin_manager_base.h"
 #include "services/network/public/cpp/shared_url_loader_factory.h"
 
 namespace arc {
@@ -33,24 +36,30 @@ policy::DeviceManagementService* GetDeviceManagementService() {
   return connector->device_management_service();
 }
 
+// Returns the Device Account Id. Assumes that |profile| is the only Profile
+// on Chrome OS.
+std::string GetDeviceAccountId(Profile* profile) {
+  const SigninManagerBase* const signin_manager =
+      SigninManagerFactory::GetForProfile(profile);
+
+  return signin_manager->GetAuthenticatedAccountId();
+}
+
 }  // namespace
 
-ArcAndroidManagementChecker::ArcAndroidManagementChecker(
-    Profile* profile,
-    ProfileOAuth2TokenService* token_service,
-    const std::string& account_id,
-    bool retry_on_error)
+ArcAndroidManagementChecker::ArcAndroidManagementChecker(Profile* profile,
+                                                         bool retry_on_error)
     : profile_(profile),
-      token_service_(token_service),
-      account_id_(account_id),
+      token_service_(ProfileOAuth2TokenServiceFactory::GetForProfile(profile_)),
+      device_account_id_(GetDeviceAccountId(profile_)),
       retry_on_error_(retry_on_error),
       retry_delay_(kRetryDelayMin),
       android_management_client_(
           GetDeviceManagementService(),
           g_browser_process->system_network_context_manager()
               ->GetSharedURLLoaderFactory(),
-          account_id,
-          token_service),
+          device_account_id_,
+          token_service_),
       weak_ptr_factory_(this) {}
 
 ArcAndroidManagementChecker::~ArcAndroidManagementChecker() {
@@ -79,7 +88,7 @@ void ArcAndroidManagementChecker::StartCheck(const CheckCallback& callback) {
 }
 
 void ArcAndroidManagementChecker::EnsureRefreshTokenLoaded() {
-  if (token_service_->RefreshTokenIsAvailable(account_id_)) {
+  if (token_service_->RefreshTokenIsAvailable(device_account_id_)) {
     // If the refresh token is already available, just start the management
     // check immediately.
     StartCheckInternal();
@@ -93,7 +102,7 @@ void ArcAndroidManagementChecker::EnsureRefreshTokenLoaded() {
 
 void ArcAndroidManagementChecker::OnRefreshTokenAvailable(
     const std::string& account_id) {
-  if (account_id != account_id_)
+  if (account_id != device_account_id_)
     return;
   OnRefreshTokensLoaded();
 }
@@ -106,7 +115,7 @@ void ArcAndroidManagementChecker::OnRefreshTokensLoaded() {
 void ArcAndroidManagementChecker::StartCheckInternal() {
   DCHECK(!callback_.is_null());
 
-  if (!token_service_->RefreshTokenIsAvailable(account_id_)) {
+  if (!token_service_->RefreshTokenIsAvailable(device_account_id_)) {
     VLOG(2) << "No refresh token is available for android management check.";
     std::move(callback_).Run(policy::AndroidManagementClient::Result::ERROR);
     return;

chrome/browser/chromeos/arc/policy/arc_android_management_checker.h

@@ -21,10 +21,7 @@ namespace arc {
 
 class ArcAndroidManagementChecker : public OAuth2TokenService::Observer {
  public:
-  ArcAndroidManagementChecker(Profile* profile,
-                              ProfileOAuth2TokenService* token_service,
-                              const std::string& account_id,
-                              bool retry_on_error);
+  ArcAndroidManagementChecker(Profile* profile, bool retry_on_error);
   ~ArcAndroidManagementChecker() override;
 
   static void StartClient();
@@ -54,7 +51,7 @@ class ArcAndroidManagementChecker : public OAuth2TokenService::Observer {
   Profile* profile_;
   ProfileOAuth2TokenService* const token_service_;
 
-  const std::string account_id_;
+  const std::string device_account_id_;
 
   // If true, on error, instead of reporting the error to the caller, schedule
   // the retry with delay.