Commit 722e5748 authored by Kush Sinha's avatar Kush Sinha Committed by Commit Bot

Store account emails in Chrome OS Account Manager

Chrome OS Account Manager's clients relied on |AccountTrackerService| to
map |AccountManager::AccountKey| to other representations (namely raw
email and canonical email). Due to the relation between one such client,
|ChromeOSOAuth2TokenServiceDelegate|, and |AccountTrackerService|, it
was required that |AccountTrackerService| should be able to store
account mappings for which a refresh token was not yet known. This
usage model is no longer supported. See the attached bug for context.

Fix this by adding a |raw_email| field in Chrome OS Account Manager's
Account proto definition. Future patches will change clients to read
this value.

Bug: 925827
Test: chromeos_unittests --gtest_filter="*AccountManager*Test*"
Test: unit_tests --gtest_filter="*CrOSOAuthDelegateTest*"
Test: browser_tests --gtest_filter="*Arc*AuthService*Test*"
Change-Id: I76ef741ec0a9ea38ae8757ff234c8be6dd7a3155
Reviewed-on: https://chromium-review.googlesource.com/c/1452003Reviewed-by: default avatarXiyuan Xia <xiyuan@chromium.org>
Reviewed-by: default avatarColin Blundell <blundell@chromium.org>
Reviewed-by: default avatarLutz Justen <ljusten@chromium.org>
Commit-Queue: Kush Sinha <sinhak@chromium.org>
Cr-Commit-Position: refs/heads/master@{#635493}
parent b7ac0d80
......@@ -125,15 +125,17 @@ class AccountMigrationBaseStep : public AccountMigrationRunner::Step {
account_info.email = email;
account_info.gaia = gaia_id;
identity_manager_->LegacySeedAccountInfo(account_info);
account_manager_->UpsertToken(
account_manager_->UpsertAccount(
AccountManager::AccountKey{
gaia_id, account_manager::AccountType::ACCOUNT_TYPE_GAIA},
AccountManager::kInvalidToken);
email, AccountManager::kInvalidToken);
VLOG(1) << "Successfully migrated: " << email;
}
AccountManager* account_manager() { return account_manager_; }
identity::IdentityManager* identity_manager() { return identity_manager_; }
private:
// Implementations should use this to start their migration flow, instead of
// overriding |Run|.
......@@ -239,7 +241,9 @@ class DeviceAccountMigration : public AccountMigrationBaseStep,
continue;
}
account_manager()->UpsertToken(device_account_, it->second /* token */);
account_manager()->UpsertAccount(
device_account_, identity_manager()->GetPrimaryAccountInfo().email,
it->second /* token */);
is_success = true;
break;
}
......
......@@ -95,8 +95,8 @@ std::string AdjustConfig(const std::string& config, bool is_dns_cname_enabled) {
// Sets up Chrome OS Account Manager.
// |profile| is a non-owning pointer to |Profile|.
// |object_guid| is the Active Directory Object GUID for the Device Account.
void SetupAccountManager(Profile* profile, const std::string& object_guid) {
// |account_id| is the |AccountId| for the Device Account.
void SetupAccountManager(Profile* profile, const AccountId& account_id) {
if (!switches::IsAccountManagerEnabled())
return;
......@@ -106,13 +106,13 @@ void SetupAccountManager(Profile* profile, const std::string& object_guid) {
AccountManager* account_manager =
factory->GetAccountManager(profile->GetPath().value());
DCHECK(account_manager);
// |AccountManager::UpsertToken| is idempotent and safe to call multiple
// |AccountManager::UpsertAccount| is idempotent and safe to call multiple
// times.
account_manager->UpsertToken(
account_manager->UpsertAccount(
AccountManager::AccountKey{
object_guid,
account_id.GetObjGuid(),
account_manager::AccountType::ACCOUNT_TYPE_ACTIVE_DIRECTORY},
AccountManager::kActiveDirectoryDummyToken);
account_id.GetUserEmail(), AccountManager::kActiveDirectoryDummyToken);
}
} // namespace
......@@ -157,7 +157,7 @@ AuthPolicyCredentialsManager::AuthPolicyCredentialsManager(Profile* profile)
base::Bind(&AuthPolicyCredentialsManager::OnSignalConnectedCallback,
weak_factory_.GetWeakPtr()));
SetupAccountManager(profile, user->GetAccountId().GetObjGuid());
SetupAccountManager(profile, user->GetAccountId());
}
AuthPolicyCredentialsManager::~AuthPolicyCredentialsManager() {}
......
......@@ -13,6 +13,7 @@
#include "base/stl_util.h"
#include "chrome/browser/chromeos/account_mapper_util.h"
#include "chromeos/account_manager/account_manager.h"
#include "components/signin/core/browser/account_tracker_service.h"
#include "content/public/browser/network_service_instance.h"
#include "google_apis/gaia/oauth2_access_token_fetcher_immediate_error.h"
#include "net/base/backoff_entry.h"
......@@ -66,6 +67,7 @@ ChromeOSOAuth2TokenServiceDelegate::ChromeOSOAuth2TokenServiceDelegate(
chromeos::AccountManager* account_manager)
: account_mapper_util_(
std::make_unique<AccountMapperUtil>(account_tracker_service)),
account_tracker_service_(account_tracker_service),
account_manager_(account_manager),
backoff_entry_(&kBackoffPolicy),
backoff_error_(GoogleServiceAuthError::NONE),
......@@ -205,6 +207,31 @@ void ChromeOSOAuth2TokenServiceDelegate::LoadCredentials(
void ChromeOSOAuth2TokenServiceDelegate::UpdateCredentials(
const std::string& account_id,
const std::string& refresh_token) {
// This API could have been called for upserting the Device/Primary
// |account_id| or a Secondary |account_id|.
// Account insertion:
// Device Account insertion on Chrome OS happens as a 2 step process:
// 1. The account is inserted into SigninManager / AccountTrackerService, via
// IdentityManager, with a valid Gaia id and email but an invalid refresh
// token.
// 2. This API is called to update the aforementioned account with a valid
// refresh token.
// Secondary Account insertion on Chrome OS happens atomically in
// |InlineLoginHandlerChromeOS::<anon>::SigninHelper::OnClientOAuthSuccess|.
// In both of the aforementioned cases, we can be sure that when this API is
// called, |account_id| is guaranteed to be present in
// |AccountTrackerService|. This guarantee is important because
// |ChromeOSOAuth2TokenServiceDelegate| relies on |AccountTrackerService| to
// convert |account_id| to an email id.
// Account update:
// If an account is being updated, it must be present in
// |AccountTrackerService|.
// Hence for all cases (insertion and updates for Device and Secondary
// Accounts) we can be sure that |account_id| is present in
// |AccountTrackerService|.
DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);
DCHECK_EQ(LOAD_CREDENTIALS_FINISHED_WITH_SUCCESS, load_credentials_state());
DCHECK(!account_id.empty());
......@@ -212,12 +239,19 @@ void ChromeOSOAuth2TokenServiceDelegate::UpdateCredentials(
ValidateAccountId(account_id);
const AccountManager::AccountKey& account_key =
account_mapper_util_->OAuthAccountIdToAccountKey(account_id);
const AccountInfo& account_info =
account_tracker_service_->GetAccountInfo(account_id);
LOG_IF(FATAL, account_info.gaia.empty())
<< "account_id must be present in AccountTrackerService before "
"UpdateCredentials is called";
// Will result in AccountManager calling
// |ChromeOSOAuth2TokenServiceDelegate::OnTokenUpserted|.
account_manager_->UpsertToken(account_key, refresh_token);
account_manager_->UpsertAccount(
AccountManager::AccountKey{
account_info.gaia,
account_manager::AccountType::ACCOUNT_TYPE_GAIA} /* account_key */,
account_info.email /* email */, refresh_token);
}
scoped_refptr<network::SharedURLLoaderFactory>
......
......@@ -81,11 +81,16 @@ class ChromeOSOAuth2TokenServiceDelegate
void GetAccountsCallback(
std::vector<AccountManager::AccountKey> account_keys);
// TODO(sinhak): Either remove |AccountMapperUtil| or move it to an anonymous
// namespace.
std::unique_ptr<AccountMapperUtil> account_mapper_util_;
// A non-owning pointer to |AccountManager|. |AccountManager| is available
// A non-owning pointer.
AccountTrackerService* const account_tracker_service_;
// A non-owning pointer. |AccountManager| is available
// throughout the lifetime of a user session.
AccountManager* account_manager_;
AccountManager* const account_manager_;
// A cache of AccountKeys.
std::set<AccountManager::AccountKey> account_keys_;
......
......@@ -224,7 +224,7 @@ TEST_F(CrOSOAuthDelegateTest,
EXPECT_FALSE(
base::ContainsValue(delegate_->GetAccounts(), account_info_.account_id));
account_manager_.UpsertToken(gaia_account_key_, kGaiaToken);
account_manager_.UpsertAccount(gaia_account_key_, kUserEmail, kGaiaToken);
EXPECT_TRUE(delegate_->RefreshTokenIsAvailable(account_info_.account_id));
EXPECT_TRUE(
......@@ -241,8 +241,8 @@ TEST_F(CrOSOAuthDelegateTest,
EXPECT_FALSE(
base::ContainsValue(delegate_->GetAccounts(), account_info_.account_id));
account_manager_.UpsertToken(gaia_account_key_,
AccountManager::kInvalidToken);
account_manager_.UpsertAccount(gaia_account_key_, kUserEmail,
AccountManager::kInvalidToken);
EXPECT_TRUE(delegate_->RefreshTokenIsAvailable(account_info_.account_id));
EXPECT_TRUE(
......@@ -340,16 +340,18 @@ TEST_F(CrOSOAuthDelegateTest,
TEST_F(CrOSOAuthDelegateTest, BatchChangeObserversAreNotifiedOncePerBatch) {
// Setup
AccountInfo account1 = CreateAccountInfoTestFixture(
"1" /* gaia_id */, "test1@gmail.com" /* email */);
"1" /* gaia_id */, "user1@example.com" /* email */);
AccountInfo account2 = CreateAccountInfoTestFixture(
"2" /* gaia_id */, "test2@gmail.com" /* email */);
"2" /* gaia_id */, "user2@example.com" /* email */);
account_tracker_service_.SeedAccountInfo(account1);
account_tracker_service_.SeedAccountInfo(account2);
account_manager_.UpsertToken(
AccountManager::AccountKey{account1.gaia, ACCOUNT_TYPE_GAIA}, "token1");
account_manager_.UpsertToken(
AccountManager::AccountKey{account2.gaia, ACCOUNT_TYPE_GAIA}, "token2");
account_manager_.UpsertAccount(
AccountManager::AccountKey{account1.gaia, ACCOUNT_TYPE_GAIA},
"user1@example.com", "token1");
account_manager_.UpsertAccount(
AccountManager::AccountKey{account2.gaia, ACCOUNT_TYPE_GAIA},
"user2@example.com", "token2");
thread_bundle_.RunUntilIdle();
AccountManager account_manager;
......@@ -388,8 +390,8 @@ TEST_F(CrOSOAuthDelegateTest, GetAccountsShouldNotReturnAdAccounts) {
EXPECT_TRUE(delegate_->GetAccounts().empty());
// Insert an Active Directory account into AccountManager.
account_manager_.UpsertToken(ad_account_key_,
AccountManager::kActiveDirectoryDummyToken);
account_manager_.UpsertAccount(ad_account_key_, kUserEmail,
AccountManager::kActiveDirectoryDummyToken);
// OAuth delegate should not return Active Directory accounts.
EXPECT_TRUE(delegate_->GetAccounts().empty());
......@@ -398,7 +400,7 @@ TEST_F(CrOSOAuthDelegateTest, GetAccountsShouldNotReturnAdAccounts) {
TEST_F(CrOSOAuthDelegateTest, GetAccountsReturnsGaiaAccounts) {
EXPECT_TRUE(delegate_->GetAccounts().empty());
account_manager_.UpsertToken(gaia_account_key_, kGaiaToken);
account_manager_.UpsertAccount(gaia_account_key_, kUserEmail, kGaiaToken);
std::vector<std::string> accounts = delegate_->GetAccounts();
EXPECT_EQ(1UL, accounts.size());
......@@ -410,8 +412,8 @@ TEST_F(CrOSOAuthDelegateTest, GetAccountsReturnsGaiaAccounts) {
TEST_F(CrOSOAuthDelegateTest, GetAccountsReturnsGaiaAccountsWithInvalidTokens) {
EXPECT_TRUE(delegate_->GetAccounts().empty());
account_manager_.UpsertToken(gaia_account_key_,
AccountManager::kInvalidToken);
account_manager_.UpsertAccount(gaia_account_key_, kUserEmail,
AccountManager::kInvalidToken);
std::vector<std::string> accounts = delegate_->GetAccounts();
EXPECT_EQ(1UL, accounts.size());
......@@ -424,20 +426,22 @@ TEST_F(CrOSOAuthDelegateTest,
LOAD_CREDENTIALS_FINISHED_WITH_SUCCESS,
delegate_->load_credentials_state());
EXPECT_TRUE(delegate_->GetAccounts().empty());
const std::string kUserEmail2 = "random-email2@example.com";
const std::string kUserEmail3 = "random-email3@example.com";
// Insert 2 Gaia accounts and 1 Active Directory Account. Of the 2 Gaia
// accounts, 1 has a valid refresh token and 1 has a dummy token.
account_manager_.UpsertToken(gaia_account_key_, kGaiaToken);
account_manager_.UpsertAccount(gaia_account_key_, kUserEmail, kGaiaToken);
AccountManager::AccountKey gaia_account_key2{"random-gaia-id",
ACCOUNT_TYPE_GAIA};
account_tracker_service_.SeedAccountInfo(CreateAccountInfoTestFixture(
gaia_account_key2.id, "random-email@domain.com"));
account_manager_.UpsertToken(gaia_account_key2,
AccountManager::kInvalidToken);
account_tracker_service_.SeedAccountInfo(
CreateAccountInfoTestFixture(gaia_account_key2.id, kUserEmail2));
account_manager_.UpsertAccount(gaia_account_key2, kUserEmail2,
AccountManager::kInvalidToken);
account_manager_.UpsertToken(ad_account_key_,
AccountManager::kActiveDirectoryDummyToken);
account_manager_.UpsertAccount(ad_account_key_, kUserEmail3,
AccountManager::kActiveDirectoryDummyToken);
// Verify.
const std::vector<std::string> accounts = delegate_->GetAccounts();
......
......@@ -68,11 +68,12 @@ class SigninHelper : public GaiaAuthConsumer {
account_info.gaia = account_key_.id;
account_info.email = email_;
// TODO(crbug.com/922026): SigninHelper and InlineLoginHandlerChromeOS
// must be refactored to remove this use of LegacySeedAccountInfo.
// must be refactored to remove this use of LegacySeedAccountInfo. It should
// be possible to replace these two calls with a call to
// |AccountsMutator::AddOrUpdateAccount()|.
IdentityManagerFactory::GetForProfile(profile_)->LegacySeedAccountInfo(
account_info);
account_manager_->UpsertToken(account_key_, result.refresh_token);
account_manager_->UpsertAccount(account_key_, email_, result.refresh_token);
close_dialog_closure_.Run();
base::SequencedTaskRunnerHandle::Get()->DeleteSoon(FROM_HERE, this);
......
This diff is collapsed.
......@@ -16,7 +16,6 @@
#include "base/files/file_path.h"
#include "base/gtest_prod_util.h"
#include "base/macros.h"
#include "base/memory/ref_counted.h"
#include "base/memory/weak_ptr.h"
#include "base/observer_list.h"
#include "base/sequence_checker.h"
......@@ -73,10 +72,7 @@ class CHROMEOS_EXPORT AccountManager {
bool operator!=(const AccountKey& other) const;
};
// A map from |AccountKey| to a raw token.
using TokenMap = std::map<AccountKey, std::string>;
// A callback for list of |AccountKey|s.
// Callback used for the (asynchronous) GetAccounts() call.
using AccountListCallback = base::OnceCallback<void(std::vector<AccountKey>)>;
using DelayNetworkCallRunner =
......@@ -131,6 +127,12 @@ class CHROMEOS_EXPORT AccountManager {
// constructed object.
void GetAccounts(AccountListCallback callback);
// Gets (async) the raw, un-canonicalized email id corresponding to
// |account_key|. |callback| is called with an empty string if |account_key|
// is not known to Account Manager.
void GetAccountEmail(const AccountKey& account_key,
base::OnceCallback<void(const std::string&)> callback);
// Removes an account. Does not do anything if |account_key| is not known by
// |AccountManager|.
// Observers are notified about an account removal through
......@@ -140,13 +142,20 @@ class CHROMEOS_EXPORT AccountManager {
// with GAIA fails, AccountManager will forget the account.
void RemoveAccount(const AccountKey& account_key);
// Updates or inserts a token, for the account corresponding to the given
// |account_key|. Use |AccountManager::kActiveDirectoryDummyToken| as the
// |token| for Active Directory accounts, and |AccountManager::kInvalidToken|
// for accounts with unkown tokens.
// Note: |account_key| must be valid (|AccountKey::IsValid|).
// Updates or inserts an account. |raw_email| is the raw, un-canonicalized
// email id for |account_key|. |raw_email| must not be empty. Use
// |AccountManager::kActiveDirectoryDummyToken| as the |token| for Active
// Directory accounts, and |AccountManager::kInvalidToken| for Gaia accounts
// with unknown tokens.
void UpsertAccount(const AccountKey& account_key,
const std::string& raw_email,
const std::string& token);
// Updates the token for the account corresponding to the given |account_key|.
// The account must be known to Account Manager. See |UpsertAccount| for
// information about adding an account.
// Note: This API is idempotent.
void UpsertToken(const AccountKey& account_key, const std::string& token);
void UpdateToken(const AccountKey& account_key, const std::string& token);
// Add a non owning pointer to an |AccountManager::Observer|.
void AddObserver(Observer* observer);
......@@ -191,12 +200,25 @@ class CHROMEOS_EXPORT AccountManager {
kInitialized, // Initialization was successfully completed
};
// Account Manager's internal information about an account.
struct AccountInfo {
std::string raw_email;
std::string token;
};
// A util class to revoke Gaia tokens on server. This class is meant to be
// used for a single request.
class GaiaTokenRevocationRequest;
friend class AccountManagerTest;
FRIEND_TEST_ALL_PREFIXES(AccountManagerTest, TestInitialization);
FRIEND_TEST_ALL_PREFIXES(AccountManagerTest, TestTokenPersistence);
FRIEND_TEST_ALL_PREFIXES(AccountManagerTest,
UpdatingAccountEmailShouldNotOverwriteTokens);
FRIEND_TEST_ALL_PREFIXES(AccountManagerTest,
UpdatingTokensShouldNotOverwriteAccountEmail);
using AccountMap = std::map<AccountKey, AccountInfo>;
// Same as the public |Initialize| except for a |task_runner|.
void Initialize(
......@@ -205,9 +227,13 @@ class CHROMEOS_EXPORT AccountManager {
DelayNetworkCallRunner delay_network_call_runner,
scoped_refptr<base::SequencedTaskRunner> task_runner);
// Reads tokens from |tokens| and inserts them in |tokens_| and runs all
// Loads accounts from disk and returns the result.
static AccountMap LoadAccountsFromDisk(
const base::FilePath& tokens_file_path);
// Reads accounts from |accounts| and inserts them in |accounts_| and runs all
// callbacks waiting on |AccountManager| initialization.
void InsertTokensAndRunInitializationCallbacks(const TokenMap& tokens);
void InsertAccountsAndRunInitializationCallbacks(const AccountMap& accounts);
// Accepts a closure and runs it immediately if |AccountManager| has already
// been initialized, otherwise saves the |closure| for running later, when the
......@@ -218,18 +244,36 @@ class CHROMEOS_EXPORT AccountManager {
// |AccountManager| initialization (|init_state_|) is complete.
void GetAccountsInternal(AccountListCallback callback);
// Does the actual work of fetching the email for |account_key|. Assumes that
// |AccountManager| initialization (|init_state_|) is complete.
void GetAccountEmailInternal(
const AccountKey& account_key,
base::OnceCallback<void(const std::string&)> callback);
// Does the actual work of removing an account. Assumes that
// |AccountManager| initialization (|init_state_|) is complete.
void RemoveAccountInternal(const AccountKey& account_key);
// Does the actual work of updating or inserting tokens. Assumes that
// |AccountManager| initialization (|init_state_|) is complete.
void UpsertTokenInternal(const AccountKey& account_key,
// Assumes that |AccountManager| initialization (|init_state_|) is complete.
void UpdateTokenInternal(const AccountKey& account_key,
const std::string& token);
// Does the actual work of upserting an account and performing related tasks
// like revoking old tokens and informing observers. All account updates
// funnel through to this method. Assumes that |AccountManager| initialization
// (|init_state_|) is complete.
void UpsertAccountInternal(const AccountKey& account_key,
const AccountInfo& account);
// Posts a task on |task_runner_|, which is usually a background thread, to
// persist the current state of |tokens_|.
void PersistTokensAsync();
// persist the current state of |accounts_|.
void PersistAccountsAsync();
// Gets a serialized representation of accounts.
std::string GetSerializedAccounts();
// Gets the |AccountKey|s stored in |accounts_|.
std::vector<AccountManager::AccountKey> GetAccountKeys();
// Notify |Observer|s about a token update.
void NotifyTokenObservers(const AccountKey& account_key);
......@@ -240,9 +284,9 @@ class CHROMEOS_EXPORT AccountManager {
// Revokes |account_key|'s token on the relevant backend.
// Note: Does not do anything if the |account_manager::AccountType|
// of |account_key| does not support server token revocation.
// Note: Does not do anything if |account_key| is not present in |tokens_|.
// Note: Does not do anything if |account_key| is not present in |accounts_|.
// Hence, call this method before actually modifying or deleting old tokens
// from |tokens_|.
// from |accounts_|.
void MaybeRevokeTokenOnServer(const AccountKey& account_key);
// Revokes |refresh_token| with GAIA. Virtual for testing.
......@@ -267,8 +311,8 @@ class CHROMEOS_EXPORT AccountManager {
scoped_refptr<base::SequencedTaskRunner> task_runner_;
std::unique_ptr<base::ImportantFileWriter> writer_;
// A map of account keys to tokens.
TokenMap tokens_;
// A map from |AccountKey|s to |AccountInfo|.
AccountMap accounts_;
// Callbacks waiting on class initialization (|init_state_|).
std::vector<base::OnceClosure> initialization_callbacks_;
......
......@@ -21,7 +21,13 @@ message Account {
optional string id = 1;
optional AccountType account_type = 2;
// An authentication token for this |Account|. It is a login scoped refresh
// token for Gaia accounts and a dummy token for Active Directory accounts.
optional string token = 3;
// Raw, un-canonicalized email id for this |Account|. This is guaranteed to be
// present for Gaia accounts.
optional string raw_email = 4;
}
message Accounts {
......
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