Skip to content

GitLab

T
tangled
Commit 3c5cfe2c authored by Andrei Salavei's avatar Andrei Salavei Committed by Commit Bot
Browse files
Options

Update primary account setup flow during migration 

Update IdentityManager initialization flow during the AccountManager
migration. After the offline login the |user_context| has no token,
hence the Primary account could not be setup for the IdentityManager.
Add login account to the AccountManager in case it is not presented
there to properly setup primary account for the IdentityManager.
The primary account with token will be configured later during
the AccountManager migration process.
Remove |DeprecatedSetPrimaryAccountAndUpdateAccountInfo|.

Bug: 987955
Change-Id: Ia1665e8337e26ca05af9a9abc43ab23816fce14d
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2131912
Commit-Queue: Andrei Salavei <solovey@google.com>
Reviewed-by: default avatarDavid Roger <droger@chromium.org>
Reviewed-by: default avatarKush Sinha <sinhak@chromium.org>
Cr-Commit-Position: refs/heads/master@{#758775}
parent c5cbec6b
Hide whitespace changes
Inline Side-by-side
Showing with 84 additions and 125 deletions
chrome/browser/chromeos/account_manager/account_manager_migrator.cc
View file @ 3c5cfe2c
...@@ -96,9 +96,10 @@ class AccountMigrationBaseStep : public AccountMigrationRunner::Step { ...@@ -96,9 +96,10 @@ class AccountMigrationBaseStep : public AccountMigrationRunner::Step {
~AccountMigrationBaseStep() override = default; ~AccountMigrationBaseStep() override = default;
protected: protected:
bool IsAccountPresentInAccountManager( bool IsAccountWithNonDummyTokenPresentInAccountManager(
const AccountManager::AccountKey& account) const { const AccountManager::AccountKey& account) const {
return base::Contains(account_manager_accounts_, account); return base::Contains(account_manager_accounts_, account) &&
!account_manager_->HasDummyGaiaToken(account);
} }
bool IsAccountManagerEmpty() const { bool IsAccountManagerEmpty() const {
...@@ -184,7 +185,7 @@ class DeviceAccountMigration : public AccountMigrationBaseStep, ...@@ -184,7 +185,7 @@ class DeviceAccountMigration : public AccountMigrationBaseStep,
private: private:
void StartMigration() override { void StartMigration() override {
if (IsAccountPresentInAccountManager(device_account_)) { if (IsAccountWithNonDummyTokenPresentInAccountManager(device_account_)) {
FinishWithSuccess(); FinishWithSuccess();
return; return;
} }
......
chrome/browser/chromeos/login/session/user_session_manager.cc
View file @ 3c5cfe2c
...@@ -1282,109 +1282,90 @@ void UserSessionManager::InitProfilePreferences( ...@@ -1282,109 +1282,90 @@ void UserSessionManager::InitProfilePreferences(
DCHECK(!gaia_id.empty()); DCHECK(!gaia_id.empty());
} }
bool should_use_legacy_flow = false; // We need to set the Primary Account. This is handled by
if (!identity_manager // |IdentityManager|, which enforces the invariant that only an account
->FindExtendedAccountInfoForAccountWithRefreshTokenByGaiaId( // previously known to |IdentityManager| can be set as the Primary
gaia_id) // Account. |IdentityManager| gets its knowledge of accounts from
.has_value() && // |AccountManager| and hence, before we set the Primary Account, we need
user_context.GetRefreshToken().empty()) { // to make sure that:
// Edge case: |AccountManager| is enabled but neither |IdentityManager| // 1. The account is present in |AccountManager|, and
// nor |user_context| has the refresh token. This means that an existing // 2. |IdentityManager| has been notified about it.
// user has switched on Account Manager for the first time and has not
// undergone the migration flow yet. This migration will be done shorty AccountManager* account_manager =
// in-session. g_browser_process->platform_part()
// TODO(https://crbug.com/987955): Remove this. ->GetAccountManagerFactory()
should_use_legacy_flow = true; ->GetAccountManager(profile->GetPath().value());
// |AccountManager| MUST have been fully initialized at this point (via
// |UserSessionManager::InitializeAccountManager|), otherwise we cannot
// guarantee that |IdentityManager| will have this account in Step (2).
// Reason: |AccountManager::UpsertAccount| is an async API that can
// technically take an arbitrarily long amount of time to complete and
// notify |AccountManager|'s observers. However, if |AccountManager| has
// been fully initialized, |AccountManager::UpsertAccount| and the
// associated notifications happen synchronously. We are relying on that
// (undocumented) behaviour here.
// TODO(sinhak): This is a leaky abstraction. Explore if
// |UserSessionManager::InitProfilePreferences| can handle an asynchronous
// callback and continue.
DCHECK(account_manager->IsInitialized());
const AccountManager::AccountKey account_key{
gaia_id, account_manager::AccountType::ACCOUNT_TYPE_GAIA};
// 1. Make sure that the account is present in |AccountManager|.
if (!user_context.GetRefreshToken().empty()) {
// |AccountManager::UpsertAccount| is idempotent. We can safely call it
// without checking for re-auth cases.
// We MUST NOT revoke old Device Account tokens (|revoke_old_token| =
// |false|), otherwise Gaia will revoke all tokens associated to this
// user's device id, including |refresh_token_| and the user will be
// stuck performing an online auth with Gaia at every login. See
// https://crbug.com/952570 and https://crbug.com/865189 for context.
account_manager->UpsertAccount(account_key,
user->GetDisplayEmail() /* raw_email */,
user_context.GetRefreshToken());
} else if (!account_manager->IsTokenAvailable(account_key)) {
// When |user_context| does not contain a refresh token and account is not
// present in the AccountManager it means the migration to the
// AccountManager didn't happen.
// Set account with dummy token to let IdentitManager know that account
// exists and we can safely configure the primary account at the step 2.
// The real token will be set later during the migration.
account_manager->UpsertAccount(account_key,
user->GetDisplayEmail() /* raw_email */,
AccountManager::kInvalidToken);
} }
base::UmaHistogramBoolean( DCHECK(account_manager->IsTokenAvailable(account_key));
"AccountManager.LegacySetPrimaryAccountAndUpdateAccountInfo",
should_use_legacy_flow); // 2. Make sure that IdentityManager has been notified about it.
base::Optional<AccountInfo> account_info =
if (!should_use_legacy_flow) { identity_manager
// We need to set the Primary Account. This is handled by ->FindExtendedAccountInfoForAccountWithRefreshTokenByGaiaId(
// |IdentityManager|, which enforces the invariant that only an account gaia_id);
// previously known to |IdentityManager| can be set as the Primary
// Account. |IdentityManager| gets its knowledge of accounts from DCHECK(account_info.has_value());
// |AccountManager| and hence, before we set the Primary Account, we need if (features::IsSplitSyncConsentEnabled()) {
// to make sure that: if (is_new_profile) {
// 1. The account is present in |AccountManager|, and if (!identity_manager->HasPrimaryAccount(ConsentLevel::kSync)) {
// 2. |IdentityManager| has been notified about it. // Set the account without recording browser sync consent.
identity_manager->GetPrimaryAccountMutator()
AccountManager* account_manager = ->SetUnconsentedPrimaryAccount(account_info->account_id);
g_browser_process->platform_part()
->GetAccountManagerFactory()
->GetAccountManager(profile->GetPath().value());
// |AccountManager| MUST have been fully initialized at this point (via
// |UserSessionManager::InitializeAccountManager|), otherwise we cannot
// guarantee that |IdentityManager| will have this account in Step (2).
// Reason: |AccountManager::UpsertAccount| is an async API that can
// technically take an arbitrarily long amount of time to complete and
// notify |AccountManager|'s observers. However, if |AccountManager| has
// been fully initialized, |AccountManager::UpsertAccount| and the
// associated notifications happen synchronously. We are relying on that
// (undocumented) behaviour here.
// TODO(sinhak): This is a leaky abstraction. Explore if
// |UserSessionManager::InitProfilePreferences| can handle an asynchronous
// callback and continue.
DCHECK(account_manager->IsInitialized());
// 1. Make sure that the account is present in |AccountManager|.
if (!user_context.GetRefreshToken().empty()) {
// |AccountManager::UpsertAccount| is idempotent. We can safely call it
// without checking for re-auth cases.
// We MUST NOT revoke old Device Account tokens (|revoke_old_token| =
// |false|), otherwise Gaia will revoke all tokens associated to this
// user's device id, including |refresh_token_| and the user will be
// stuck performing an online auth with Gaia at every login. See
// https://crbug.com/952570 and https://crbug.com/865189 for context.
account_manager->UpsertAccount(
AccountManager::AccountKey{
gaia_id, account_manager::AccountType::ACCOUNT_TYPE_GAIA},
user->GetDisplayEmail() /* raw_email */,
user_context.GetRefreshToken());
}
// else: If |user_context| does not contain a refresh token, then we are
// restoring an existing Profile, in which case the account will be
// already present in |AccountManager|.
// 2. Make sure that IdentityManager has been notified about it.
base::Optional<AccountInfo> account_info =
identity_manager
->FindExtendedAccountInfoForAccountWithRefreshTokenByGaiaId(
gaia_id);
DCHECK(account_info.has_value());
if (features::IsSplitSyncConsentEnabled()) {
if (is_new_profile) {
if (!identity_manager->HasPrimaryAccount(ConsentLevel::kSync)) {
// Set the account without recording browser sync consent.
identity_manager->GetPrimaryAccountMutator()
->SetUnconsentedPrimaryAccount(account_info->account_id);
}
} }
CHECK(identity_manager->HasPrimaryAccount(ConsentLevel::kNotRequired));
CHECK_EQ(
identity_manager->GetPrimaryAccountInfo(ConsentLevel::kNotRequired)
.gaia,
gaia_id);
} else {
// Set a primary account here because the profile might have been
// created with the feature SplitSyncConsent enabled. Then the
// profile might only have an unconsented primary account.
identity_manager->GetPrimaryAccountMutator()->SetPrimaryAccount(
account_info->account_id);
CHECK(identity_manager->HasPrimaryAccount(ConsentLevel::kSync));
CHECK_EQ(identity_manager->GetPrimaryAccountInfo().gaia, gaia_id);
} }
CHECK(identity_manager->HasPrimaryAccount(ConsentLevel::kNotRequired));
CHECK_EQ(
identity_manager->GetPrimaryAccountInfo(ConsentLevel::kNotRequired)
.gaia,
gaia_id);
} else { } else {
// Make sure that the google service username is properly set (we do this // Set a primary account here because the profile might have been
// on every sign in, not just the first login, to deal with existing // created with the feature SplitSyncConsent enabled. Then the
// profiles that might not have it set yet). // profile might only have an unconsented primary account.
// TODO(https://crbug.com/987955): Check the UMA stat and remove it when identity_manager->GetPrimaryAccountMutator()->SetPrimaryAccount(
// all users have been migrated to Account Manager. account_info->account_id);
identity_manager->GetPrimaryAccountMutator() CHECK(identity_manager->HasPrimaryAccount(ConsentLevel::kSync));
->DeprecatedSetPrimaryAccountAndUpdateAccountInfo( CHECK_EQ(identity_manager->GetPrimaryAccountInfo().gaia, gaia_id);
gaia_id, user_context.GetAccountId().GetUserEmail());
} }
CoreAccountId account_id = CoreAccountId account_id =
......
components/signin/internal/identity_manager/primary_account_mutator_impl.cc
View file @ 3c5cfe2c
...@@ -64,14 +64,6 @@ void PrimaryAccountMutatorImpl::SetUnconsentedPrimaryAccount( ...@@ -64,14 +64,6 @@ void PrimaryAccountMutatorImpl::SetUnconsentedPrimaryAccount(
AccountInfo account_info = account_tracker_->GetAccountInfo(account_id); AccountInfo account_info = account_tracker_->GetAccountInfo(account_id);
primary_account_manager_->SetUnconsentedPrimaryAccountInfo(account_info); primary_account_manager_->SetUnconsentedPrimaryAccountInfo(account_info);
} }
bool PrimaryAccountMutatorImpl::DeprecatedSetPrimaryAccountAndUpdateAccountInfo(
const std::string& gaia_id,
const std::string& email) {
CoreAccountId account_id = account_tracker_->SeedAccountInfo(gaia_id, email);
SetPrimaryAccount(account_id);
return true;
}
#endif #endif
#if !defined(OS_CHROMEOS) #if !defined(OS_CHROMEOS)
......
components/signin/internal/identity_manager/primary_account_mutator_impl.h
View file @ 3c5cfe2c
...@@ -29,9 +29,6 @@ class PrimaryAccountMutatorImpl : public PrimaryAccountMutator { ...@@ -29,9 +29,6 @@ class PrimaryAccountMutatorImpl : public PrimaryAccountMutator {
#if defined(OS_CHROMEOS) #if defined(OS_CHROMEOS)
void RevokeSyncConsent() override; void RevokeSyncConsent() override;
void SetUnconsentedPrimaryAccount(const CoreAccountId& account_id) override; void SetUnconsentedPrimaryAccount(const CoreAccountId& account_id) override;
bool DeprecatedSetPrimaryAccountAndUpdateAccountInfo(
const std::string& gaia_id,
const std::string& email) override;
#endif #endif
#if !defined(OS_CHROMEOS) #if !defined(OS_CHROMEOS)
bool ClearPrimaryAccount( bool ClearPrimaryAccount(
......
components/signin/public/identity_manager/primary_account_mutator.h
View file @ 3c5cfe2c
...@@ -72,18 +72,6 @@ class PrimaryAccountMutator { ...@@ -72,18 +72,6 @@ class PrimaryAccountMutator {
// of "unconsented". // of "unconsented".
virtual void SetUnconsentedPrimaryAccount( virtual void SetUnconsentedPrimaryAccount(
const CoreAccountId& account_id) = 0; const CoreAccountId& account_id) = 0;
// Updates the info of the account corresponding to (|gaia_id|, |email|),
// marks it as the primary account, and returns whether the operation
// succeeded or not. Currently, this method is guaranteed to succeed.
// NOTE: Unlike SetPrimaryAccount(), this method does not require that the
// account is known by IdentityManager. The reason is that there are
// contexts on ChromeOS where the primary account is not guaranteed to be
// known by IdentityManager when it is set.
// TODO(https://crbug.com/987955): Remove this API.
virtual bool DeprecatedSetPrimaryAccountAndUpdateAccountInfo(
const std::string& gaia_id,
const std::string& email) = 0;
#endif #endif
#if !defined(OS_CHROMEOS) #if !defined(OS_CHROMEOS)
......
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