Commit db21f388 authored by James Cook's avatar James Cook Committed by Commit Bot

cros: Migrate ArcAuthService to unconsented primary account

SplitSettingsSync will allow the user to opt-out of browser sync.
However, IdentityAccessor::GetPrimaryAccountInfo() assumes the user has
consented to browser sync. ARC has its own consent, unrelated to
chrome browser sync consent. Switch to using the "unconsented"
primary account. This account is always the main GAIA account for the
signed-in user, and exists whether or not the user has consented
to browser sync.

This requires updating:
* ArcAuthService
* ArcSupportHost
* ArcAndroidManagementChecker

See go/cros-primary-account and go/cros-sync-mock for details.

TBR=droger@chromium.org

Bug: 1042400
Test: updated existing unit tests and browser tests
Test: Set --use-unconsented-primary-account and
  --enable-features=SplitSettingsSync and verify Play Store runs
  and is signed in after:
  * Login with consumer @gmail.com
  * Enroll to @google.com and signin with @google.com
  * Enroll to @managedchrome.com and signin with managed guest

Change-Id: I48abe048a95d6aa234d4b87e76b2d70c5e88aebd
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2029307
Commit-Queue: James Cook <jamescook@chromium.org>
Reviewed-by: default avatarJames Cook <jamescook@chromium.org>
Reviewed-by: default avatarYury Khmel <khmel@chromium.org>
Cr-Commit-Position: refs/heads/master@{#737384}
parent 53f4a720
......@@ -678,8 +678,10 @@ void ArcSupportHost::OnMessage(const base::DictionaryValue& message) {
}
auto* identity_manager = IdentityManagerFactory::GetForProfile(profile_);
DCHECK(identity_manager->HasPrimaryAccount());
CoreAccountId account_id = identity_manager->GetPrimaryAccountId();
// This class doesn't care about browser sync consent.
DCHECK(identity_manager->HasUnconsentedPrimaryAccount());
CoreAccountId account_id =
identity_manager->GetUnconsentedPrimaryAccountId();
bool is_child = user_manager::UserManager::Get()->IsLoggedInAsChildUser();
// Record acceptance of ToS if it was shown to the user, otherwise simply
......
......@@ -11,12 +11,12 @@
#include "chrome/browser/chromeos/login/users/fake_chrome_user_manager.h"
#include "chrome/browser/consent_auditor/consent_auditor_factory.h"
#include "chrome/browser/consent_auditor/consent_auditor_test_utils.h"
#include "chrome/browser/signin/identity_manager_factory.h"
#include "chrome/browser/signin/identity_test_environment_profile_adaptor.h"
#include "chrome/test/base/browser_with_test_window_test.h"
#include "chrome/test/base/testing_profile.h"
#include "components/consent_auditor/fake_consent_auditor.h"
#include "components/signin/public/identity_manager/identity_manager.h"
#include "components/signin/public/identity_manager/identity_test_utils.h"
#include "components/signin/public/identity_manager/identity_test_environment.h"
#include "components/user_manager/scoped_user_manager.h"
#include "content/public/test/browser_task_environment.h"
#include "testing/gmock/include/gmock/gmock.h"
......@@ -75,9 +75,11 @@ class ArcSupportHostTest : public BrowserWithTestWindowTest {
BrowserWithTestWindowTest::SetUp();
user_manager_enabler_ = std::make_unique<user_manager::ScopedUserManager>(
std::make_unique<chromeos::FakeChromeUserManager>());
signin::MakePrimaryAccountAvailable(
IdentityManagerFactory::GetForProfile(profile()),
"testing@account.com");
identity_test_env_adaptor_ =
std::make_unique<IdentityTestEnvironmentProfileAdaptor>(profile());
// The code under test should not be tied to browser sync consent.
identity_test_env_adaptor_->identity_test_env()
->MakeUnconsentedPrimaryAccountAvailable("testing@account.com");
support_host_ = std::make_unique<ArcSupportHost>(profile());
fake_arc_support_ = std::make_unique<FakeArcSupport>(support_host_.get());
......@@ -90,6 +92,7 @@ class ArcSupportHostTest : public BrowserWithTestWindowTest {
fake_arc_support_.reset();
support_host_.reset();
identity_test_env_adaptor_.reset();
user_manager_enabler_.reset();
BrowserWithTestWindowTest::TearDown();
......@@ -130,14 +133,20 @@ class ArcSupportHostTest : public BrowserWithTestWindowTest {
// BrowserWithTestWindowTest:
TestingProfile::TestingFactories GetTestingFactories() override {
return {{ConsentAuditorFactory::GetInstance(),
base::BindRepeating(&BuildFakeConsentAuditor)}};
TestingProfile::TestingFactories factories = {
{ConsentAuditorFactory::GetInstance(),
base::BindRepeating(&BuildFakeConsentAuditor)}};
IdentityTestEnvironmentProfileAdaptor::
AppendIdentityTestEnvironmentFactories(&factories);
return factories;
}
private:
std::unique_ptr<ArcSupportHost> support_host_;
std::unique_ptr<FakeArcSupport> fake_arc_support_;
std::unique_ptr<user_manager::ScopedUserManager> user_manager_enabler_;
std::unique_ptr<IdentityTestEnvironmentProfileAdaptor>
identity_test_env_adaptor_;
std::unique_ptr<MockAuthDelegate> auth_delegate_;
std::unique_ptr<MockTermsOfServiceDelegate> tos_delegate_;
......
......@@ -224,8 +224,25 @@ void TriggerAccountManagerMigrationsIfRequired(Profile* profile) {
migrator->Start();
}
std::string GetAuthenticatedUsernameUTF8(Profile* profile) {
return base::UTF16ToUTF8(signin_ui_util::GetAuthenticatedUsername(profile));
// See //components/arc/mojom/auth.mojom RequestPrimaryAccount() for the spec.
// See also go/arc-primary-account.
std::string GetAccountName(Profile* profile) {
switch (GetAccountType(profile)) {
case mojom::ChromeAccountType::USER_ACCOUNT:
case mojom::ChromeAccountType::CHILD_ACCOUNT:
// IdentityManager::GetUnconsentedPrimaryAccountInfo().email might be more
// appropriate here, but this is what we have done historically.
return chromeos::ProfileHelper::Get()
->GetUserByProfile(profile)
->GetDisplayEmail();
case mojom::ChromeAccountType::ROBOT_ACCOUNT:
case mojom::ChromeAccountType::ACTIVE_DIRECTORY_ACCOUNT:
case mojom::ChromeAccountType::OFFLINE_DEMO_ACCOUNT:
return std::string();
case mojom::ChromeAccountType::UNKNOWN:
NOTREACHED();
return std::string();
}
}
} // namespace
......@@ -282,8 +299,7 @@ void ArcAuthService::GetGoogleAccountsInArc(
void ArcAuthService::RequestPrimaryAccount(
RequestPrimaryAccountCallback callback) {
std::move(callback).Run(GetAuthenticatedUsernameUTF8(profile_),
GetAccountType(profile_));
std::move(callback).Run(GetAccountName(profile_), GetAccountType(profile_));
}
void ArcAuthService::OnConnectionReady() {
......@@ -517,9 +533,12 @@ void ArcAuthService::FetchPrimaryAccountInfo(
->SetURLLoaderFactoryForTesting(url_loader_factory_);
}
} else {
// Optionally retrieve auth code in silent mode.
// Optionally retrieve auth code in silent mode. Use the "unconsented"
// primary account because this class doesn't care about browser sync
// consent.
DCHECK(identity_manager_->HasUnconsentedPrimaryAccount());
auth_code_fetcher = CreateArcBackgroundAuthCodeFetcher(
identity_manager_->GetPrimaryAccountId(), initial_signin);
identity_manager_->GetUnconsentedPrimaryAccountId(), initial_signin);
}
// Add the request to |pending_token_requests_| first, before starting a token
......@@ -672,7 +691,7 @@ void ArcAuthService::OnPrimaryAccountAuthCodeFetched(
DeletePendingTokenRequest(fetcher);
if (success) {
const std::string& full_account_id = GetAuthenticatedUsernameUTF8(profile_);
const std::string& full_account_id = GetAccountName(profile_);
std::move(callback).Run(
mojom::ArcSignInStatus::SUCCESS,
CreateAccountInfo(!IsArcOptInVerificationDisabled(), auth_code,
......
......@@ -93,7 +93,7 @@ namespace arc {
class FakeAuthInstance : public mojom::AuthInstance {
public:
FakeAuthInstance() {}
FakeAuthInstance() = default;
~FakeAuthInstance() override = default;
// mojom::AuthInstance:
......@@ -311,8 +311,9 @@ class ArcAuthServiceTest : public InProcessBrowserTest {
identity_test_env->SetAutomaticIssueOfAccessTokens(true);
if (user_type != user_manager::USER_TYPE_ACTIVE_DIRECTORY) {
// Identity service doesn't have a primary account for Active Directory
// sessions.
identity_test_env->MakePrimaryAccountAvailable(kFakeUserName);
// sessions. Use "unconsented" because ARC doesn't care about browser
// sync consent.
identity_test_env->MakeUnconsentedPrimaryAccountAvailable(kFakeUserName);
}
profile()->GetPrefs()->SetBoolean(prefs::kArcSignedIn, true);
......
......@@ -39,7 +39,8 @@ CoreAccountId GetDeviceAccountId(Profile* profile) {
const auto* const identity_manager =
IdentityManagerFactory::GetForProfile(profile);
return identity_manager->GetPrimaryAccountId();
// The account is the same whether or not the user consented to browser sync.
return identity_manager->GetUnconsentedPrimaryAccountId();
}
} // namespace
......
......@@ -108,11 +108,10 @@ class ArcPlayStoreDisabledWaiter : public ArcSessionManager::Observer {
class ArcSessionManagerTest : public MixinBasedInProcessBrowserTest {
protected:
ArcSessionManagerTest() {}
ArcSessionManagerTest() = default;
~ArcSessionManagerTest() override = default;
// MixinBasedInProcessBrowserTest:
~ArcSessionManagerTest() override {}
void SetUpCommandLine(base::CommandLine* command_line) override {
MixinBasedInProcessBrowserTest::SetUpCommandLine(command_line);
arc::SetArcAvailableCommandLineForTesting(command_line);
......@@ -150,7 +149,7 @@ class ArcSessionManagerTest : public MixinBasedInProcessBrowserTest {
std::make_unique<IdentityTestEnvironmentProfileAdaptor>(profile_.get());
// Seed account info properly.
identity_test_env()->MakePrimaryAccountAvailable(kFakeUserName);
identity_test_env()->MakeUnconsentedPrimaryAccountAvailable(kFakeUserName);
profile()->GetPrefs()->SetBoolean(prefs::kArcSignedIn, true);
profile()->GetPrefs()->SetBoolean(prefs::kArcTermsAccepted, true);
......@@ -216,6 +215,10 @@ class ArcSessionManagerTest : public MixinBasedInProcessBrowserTest {
return identity_test_environment_adaptor_->identity_test_env();
}
signin::IdentityManager* identity_manager() {
return identity_test_env()->identity_manager();
}
private:
chromeos::LocalPolicyTestServerMixin local_policy_mixin_{&mixin_host_};
std::unique_ptr<user_manager::ScopedUserManager> user_manager_enabler_;
......@@ -230,7 +233,8 @@ class ArcSessionManagerTest : public MixinBasedInProcessBrowserTest {
IN_PROC_BROWSER_TEST_F(ArcSessionManagerTest, ConsumerAccount) {
EnableArc();
identity_test_env()->WaitForAccessTokenRequestIfNecessaryAndRespondWithToken(
kUnmanagedAuthToken, base::Time::Max());
identity_manager()->GetUnconsentedPrimaryAccountId(), kUnmanagedAuthToken,
base::Time::Max());
ASSERT_EQ(ArcSessionManager::State::ACTIVE,
ArcSessionManager::Get()->state());
}
......@@ -255,7 +259,8 @@ IN_PROC_BROWSER_TEST_F(ArcSessionManagerTest, ManagedChromeAccount) {
IN_PROC_BROWSER_TEST_F(ArcSessionManagerTest, ManagedAndroidAccount) {
EnableArc();
identity_test_env()->WaitForAccessTokenRequestIfNecessaryAndRespondWithToken(
kManagedAuthToken, base::Time::Max());
identity_manager()->GetUnconsentedPrimaryAccountId(), kManagedAuthToken,
base::Time::Max());
ArcPlayStoreDisabledWaiter().Wait();
EXPECT_FALSE(IsArcPlayStoreEnabledForProfile(profile()));
}
......
......@@ -28,8 +28,8 @@ namespace signin_ui_util {
// The maximum number of times to show the welcome tutorial for an upgrade user.
const int kUpgradeWelcomeTutorialShowMax = 1;
// Returns the username of the authenticated user or an empty string if there is
// no authenticated user.
// Returns the username of the primary account or an empty string if there is
// no primary account or the account has not consented to browser sync.
base::string16 GetAuthenticatedUsername(Profile* profile);
// Initializes signin-related preferences.
......
Markdown is supported
0%
or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment