Commit b234806b authored by Kush Sinha's avatar Kush Sinha Committed by Commit Bot

Return raw email ids in crOS Account Manager notifications

Chrome OS Account Manager's observers need to map
|AccountManager::AccountKey| to email ids. This mapping was being
provided by |AccountMapperUtil|, but we want to eliminate that class and
maintain the mapping directly in AccountManager.

https://crrev.com/c/1452003 ensured that all new account additions in
Chrome OS Account Manager have an email id associated with them. This
patch makes the email id available for reading in observer
notifications and |AccountManager::GetAccounts|.

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: I0e130ce968a02a37f2380460f6936b5cf169ae02
Reviewed-on: https://chromium-review.googlesource.com/c/1477691Reviewed-by: default avatarColin Blundell <blundell@chromium.org>
Reviewed-by: default avatarXiyuan Xia <xiyuan@chromium.org>
Commit-Queue: Kush Sinha <sinhak@chromium.org>
Cr-Commit-Position: refs/heads/master@{#635908}
parent a17379ce
......@@ -150,8 +150,12 @@ class AccountMigrationBaseStep : public AccountMigrationRunner::Step {
&AccountMigrationBaseStep::OnGetAccounts, weak_factory_.GetWeakPtr()));
}
void OnGetAccounts(std::vector<AccountManager::AccountKey> accounts) {
account_manager_accounts_ = std::move(accounts);
void OnGetAccounts(const std::vector<AccountManager::Account>& accounts) {
account_manager_accounts_.clear();
account_manager_accounts_.reserve(accounts.size());
for (const AccountManager::Account& account : accounts) {
account_manager_accounts_.emplace_back(account.key);
}
StartMigration();
}
......
......@@ -200,7 +200,7 @@ void ChromeOSOAuth2TokenServiceDelegate::LoadCredentials(
DCHECK(account_manager_);
account_manager_->AddObserver(this);
account_manager_->GetAccounts(
base::BindOnce(&ChromeOSOAuth2TokenServiceDelegate::GetAccountsCallback,
base::BindOnce(&ChromeOSOAuth2TokenServiceDelegate::OnGetAccounts,
weak_factory_.GetWeakPtr()));
}
......@@ -259,8 +259,8 @@ ChromeOSOAuth2TokenServiceDelegate::GetURLLoaderFactory() const {
return account_manager_->GetUrlLoaderFactory();
}
void ChromeOSOAuth2TokenServiceDelegate::GetAccountsCallback(
std::vector<AccountManager::AccountKey> account_keys) {
void ChromeOSOAuth2TokenServiceDelegate::OnGetAccounts(
const std::vector<AccountManager::Account>& accounts) {
DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);
// This callback should only be triggered during |LoadCredentials|, which
......@@ -269,27 +269,26 @@ void ChromeOSOAuth2TokenServiceDelegate::GetAccountsCallback(
DCHECK_EQ(LOAD_CREDENTIALS_IN_PROGRESS, load_credentials_state());
set_load_credentials_state(LOAD_CREDENTIALS_FINISHED_WITH_SUCCESS);
// The typical order of |OAuth2TokenService::Observer| callbacks is:
// 1. OnRefreshTokenAvailable
// 2. OnEndBatchChanges
// 3. OnRefreshTokensLoaded
{
ScopedBatchChange batch(this);
for (const auto& account_key : account_keys) {
OnTokenUpserted(account_key);
for (const auto& account : accounts) {
OnTokenUpserted(account);
}
}
FireRefreshTokensLoaded();
}
void ChromeOSOAuth2TokenServiceDelegate::OnTokenUpserted(
const AccountManager::AccountKey& account_key) {
const AccountManager::Account& account) {
DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);
account_keys_.insert(account_key);
account_keys_.insert(account.key);
std::string account_id =
account_mapper_util_->AccountKeyToOAuthAccountId(account_key);
account_mapper_util_->AccountKeyToOAuthAccountId(account.key);
if (account_id.empty()) {
return;
}
......@@ -306,7 +305,7 @@ void ChromeOSOAuth2TokenServiceDelegate::OnTokenUpserted(
// However, if we know that |account_key| has a dummy token, store a
// persistent error against it, so that we can pre-emptively reject access
// token requests for it.
if (account_manager_->HasDummyGaiaToken(account_key)) {
if (account_manager_->HasDummyGaiaToken(account.key)) {
error = GoogleServiceAuthError::FromInvalidGaiaCredentialsReason(
GoogleServiceAuthError::InvalidGaiaCredentialsReason::
CREDENTIALS_REJECTED_BY_CLIENT);
......@@ -322,18 +321,18 @@ void ChromeOSOAuth2TokenServiceDelegate::OnTokenUpserted(
}
void ChromeOSOAuth2TokenServiceDelegate::OnAccountRemoved(
const AccountManager::AccountKey& account_key) {
const AccountManager::Account& account) {
DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);
DCHECK_EQ(LOAD_CREDENTIALS_FINISHED_WITH_SUCCESS, load_credentials_state());
auto it = account_keys_.find(account_key);
auto it = account_keys_.find(account.key);
if (it == account_keys_.end()) {
return;
}
account_keys_.erase(it);
std::string account_id =
account_mapper_util_->AccountKeyToOAuthAccountId(account_key);
account_mapper_util_->AccountKeyToOAuthAccountId(account.key);
if (account_id.empty()) {
return;
}
......
......@@ -59,8 +59,8 @@ class ChromeOSOAuth2TokenServiceDelegate
const net::BackoffEntry* BackoffEntry() const override;
// |AccountManager::Observer| overrides.
void OnTokenUpserted(const AccountManager::AccountKey& account_key) override;
void OnAccountRemoved(const AccountManager::AccountKey& account_key) override;
void OnTokenUpserted(const AccountManager::Account& account) override;
void OnAccountRemoved(const AccountManager::Account& account) override;
// |NetworkConnectionTracker::NetworkConnectionObserver| overrides.
void OnConnectionChanged(network::mojom::ConnectionType type) override;
......@@ -78,8 +78,7 @@ class ChromeOSOAuth2TokenServiceDelegate
};
// Callback handler for |AccountManager::GetAccounts|.
void GetAccountsCallback(
std::vector<AccountManager::AccountKey> account_keys);
void OnGetAccounts(const std::vector<AccountManager::Account>& accounts);
// TODO(sinhak): Either remove |AccountMapperUtil| or move it to an anonymous
// namespace.
......
......@@ -101,13 +101,13 @@ void AccountManagerUIHandler::HandleGetAccounts(const base::ListValue* args) {
base::Value callback_id = args->GetList()[0].Clone();
account_manager_->GetAccounts(
base::BindOnce(&AccountManagerUIHandler::GetAccountsCallbackHandler,
base::BindOnce(&AccountManagerUIHandler::OnGetAccounts,
weak_factory_.GetWeakPtr(), std::move(callback_id)));
}
void AccountManagerUIHandler::GetAccountsCallbackHandler(
void AccountManagerUIHandler::OnGetAccounts(
base::Value callback_id,
std::vector<AccountManager::AccountKey> account_keys) {
const std::vector<AccountManager::Account>& stored_accounts) {
base::ListValue accounts;
const AccountId device_account_id =
......@@ -116,7 +116,8 @@ void AccountManagerUIHandler::GetAccountsCallbackHandler(
->GetAccountId();
base::DictionaryValue device_account;
for (const auto& account_key : account_keys) {
for (const auto& stored_account : stored_accounts) {
const AccountManager::AccountKey& account_key = stored_account.key;
// We are only interested in listing GAIA accounts.
if (account_key.account_type !=
account_manager::AccountType::ACCOUNT_TYPE_GAIA) {
......@@ -230,12 +231,12 @@ void AccountManagerUIHandler::OnJavascriptDisallowed() {
// guarantee that |AccountManager| (our source of truth) will have a newly added
// account by the time |IdentityManager| has it.
void AccountManagerUIHandler::OnTokenUpserted(
const AccountManager::AccountKey& account_key) {
const AccountManager::Account& account) {
RefreshUI();
}
void AccountManagerUIHandler::OnAccountRemoved(
const AccountManager::AccountKey& account_key) {
const AccountManager::Account& account) {
RefreshUI();
}
......
......@@ -40,8 +40,8 @@ class AccountManagerUIHandler : public ::settings::SettingsPageUIHandler,
// |AccountManager::Observer| overrides.
// |AccountManager| is considered to be the source of truth for account
// information.
void OnTokenUpserted(const AccountManager::AccountKey& account_key) override;
void OnAccountRemoved(const AccountManager::AccountKey& account_key) override;
void OnTokenUpserted(const AccountManager::Account& account) override;
void OnAccountRemoved(const AccountManager::Account& account) override;
// |identity::IdentityManager::Observer| overrides.
void OnExtendedAccountInfoUpdated(const AccountInfo& info) override;
......@@ -63,9 +63,9 @@ class AccountManagerUIHandler : public ::settings::SettingsPageUIHandler,
void HandleShowWelcomeDialogIfRequired(const base::ListValue* args);
// |AccountManager::GetAccounts| callback.
void GetAccountsCallbackHandler(
void OnGetAccounts(
base::Value callback_id,
std::vector<AccountManager::AccountKey> account_keys);
const std::vector<AccountManager::Account>& stored_accounts);
// Refreshes the UI.
void RefreshUI();
......
......@@ -221,8 +221,9 @@ void AccountManager::InsertAccountsAndRunInitializationCallbacks(
}
initialization_callbacks_.clear();
for (const auto& token : accounts_) {
NotifyTokenObservers(token.first);
for (const auto& account : accounts_) {
NotifyTokenObservers(
Account{account.first /* key */, account.second.raw_email});
}
RecordNumAccountsMetric(accounts_.size());
......@@ -255,7 +256,7 @@ void AccountManager::GetAccountsInternal(AccountListCallback callback) {
DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);
DCHECK_EQ(init_state_, InitializationState::kInitialized);
std::move(callback).Run(GetAccountKeys());
std::move(callback).Run(GetAccounts());
}
void AccountManager::GetAccountEmail(
......@@ -301,10 +302,11 @@ void AccountManager::RemoveAccountInternal(const AccountKey& account_key) {
return;
}
const std::string raw_email = it->second.raw_email;
MaybeRevokeTokenOnServer(account_key);
accounts_.erase(it);
PersistAccountsAsync();
NotifyAccountRemovalObservers(account_key);
NotifyAccountRemovalObservers(Account{account_key, raw_email});
}
void AccountManager::UpsertAccount(const AccountKey& account_key,
......@@ -362,7 +364,7 @@ void AccountManager::UpsertAccountInternal(const AccountKey& account_key,
// New account. Insert it.
accounts_.emplace(account_key, account);
PersistAccountsAsync();
NotifyTokenObservers(account_key);
NotifyTokenObservers(Account{account_key, account.raw_email});
return;
}
......@@ -376,7 +378,7 @@ void AccountManager::UpsertAccountInternal(const AccountKey& account_key,
PersistAccountsAsync();
if (did_token_change) {
NotifyTokenObservers(account_key);
NotifyTokenObservers(Account{account_key, account.raw_email});
}
}
......@@ -399,31 +401,30 @@ std::string AccountManager::GetSerializedAccounts() {
return accounts_proto.SerializeAsString();
}
std::vector<AccountManager::AccountKey> AccountManager::GetAccountKeys() {
std::vector<AccountManager::AccountKey> accounts;
std::vector<AccountManager::Account> AccountManager::GetAccounts() {
std::vector<Account> accounts;
accounts.reserve(accounts_.size());
for (const auto& key_val : accounts_) {
accounts.emplace_back(key_val.first);
accounts.emplace_back(Account{key_val.first, key_val.second.raw_email});
}
return accounts;
}
void AccountManager::NotifyTokenObservers(const AccountKey& account_key) {
void AccountManager::NotifyTokenObservers(const Account& account) {
DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);
for (auto& observer : observers_) {
observer.OnTokenUpserted(account_key);
observer.OnTokenUpserted(account);
}
}
void AccountManager::NotifyAccountRemovalObservers(
const AccountKey& account_key) {
void AccountManager::NotifyAccountRemovalObservers(const Account& account) {
DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);
for (auto& observer : observers_) {
observer.OnAccountRemoved(account_key);
observer.OnAccountRemoved(account);
}
}
......
......@@ -72,8 +72,18 @@ class CHROMEOS_EXPORT AccountManager {
bool operator!=(const AccountKey& other) const;
};
// Publicly viewable information about an account.
struct Account {
// A unique identifier for this account.
AccountKey key;
// The raw, un-canonicalized email id for this account.
std::string raw_email;
};
// Callback used for the (asynchronous) GetAccounts() call.
using AccountListCallback = base::OnceCallback<void(std::vector<AccountKey>)>;
using AccountListCallback =
base::OnceCallback<void(const std::vector<Account>&)>;
using DelayNetworkCallRunner =
base::RepeatingCallback<void(base::OnceClosure)>;
......@@ -83,7 +93,7 @@ class CHROMEOS_EXPORT AccountManager {
Observer();
virtual ~Observer();
// Called when the token for |account_key| is updated/inserted.
// Called when the token for |account| is updated/inserted.
// Use |AccountManager::AddObserver| to add an |Observer|.
// Note: |Observer|s which register with |AccountManager| before its
// initialization is complete will get notified when |AccountManager| is
......@@ -91,13 +101,13 @@ class CHROMEOS_EXPORT AccountManager {
// Note: |Observer|s which register with |AccountManager| after its
// initialization is complete will not get an immediate
// notification-on-registration.
virtual void OnTokenUpserted(const AccountKey& account_key) = 0;
virtual void OnTokenUpserted(const Account& account) = 0;
// Called when an account has been removed from AccountManager.
// Observers that may have cached access tokens (fetched via
// |AccountManager::CreateAccessTokenFetcher|), must clear their cache entry
// for this |account_key| on receiving this callback.
virtual void OnAccountRemoved(const AccountKey& account_key) = 0;
// for this |account| on receiving this callback.
virtual void OnAccountRemoved(const Account& account) = 0;
private:
DISALLOW_COPY_AND_ASSIGN(Observer);
......@@ -272,14 +282,14 @@ class CHROMEOS_EXPORT AccountManager {
// Gets a serialized representation of accounts.
std::string GetSerializedAccounts();
// Gets the |AccountKey|s stored in |accounts_|.
std::vector<AccountManager::AccountKey> GetAccountKeys();
// Gets the publicly viewable information stored in |accounts_|.
std::vector<Account> GetAccounts();
// Notify |Observer|s about a token update.
void NotifyTokenObservers(const AccountKey& account_key);
// Notifies |Observer|s about a token update for |account|.
void NotifyTokenObservers(const Account& account);
// Notify |Observer|s about an account removal.
void NotifyAccountRemovalObservers(const AccountKey& account_key);
// Notifies |Observer|s about an |account| removal.
void NotifyAccountRemovalObservers(const Account& account);
// Revokes |account_key|'s token on the relevant backend.
// Note: Does not do anything if the |account_manager::AccountType|
......
......@@ -33,6 +33,17 @@ constexpr char kGaiaToken[] = "gaia_token";
constexpr char kNewGaiaToken[] = "new_gaia_token";
constexpr char kRawUserEmail[] = "user@example.com";
bool IsAccountKeyPresent(const std::vector<AccountManager::Account>& accounts,
const AccountManager::AccountKey& account_key) {
for (const auto& account : accounts) {
if (account.key == account_key) {
return true;
}
}
return false;
}
} // namespace
class AccountManagerSpy : public AccountManager {
......@@ -58,18 +69,16 @@ class AccountManagerTest : public testing::Test {
}
// Gets the list of accounts stored in |account_manager_|.
std::vector<AccountManager::AccountKey> GetAccountsBlocking() {
std::vector<AccountManager::AccountKey> accounts;
std::vector<AccountManager::Account> GetAccountsBlocking() {
std::vector<AccountManager::Account> accounts;
base::RunLoop run_loop;
account_manager_->GetAccounts(base::BindOnce(
[](std::vector<AccountManager::AccountKey>* accounts,
base::OnceClosure quit_closure,
std::vector<AccountManager::AccountKey> stored_accounts) -> void {
*accounts = std::move(stored_accounts);
std::move(quit_closure).Run();
},
base::Unretained(&accounts), run_loop.QuitClosure()));
account_manager_->GetAccounts(base::BindLambdaForTesting(
[&accounts, &run_loop](
const std::vector<AccountManager::Account>& stored_accounts) {
accounts = stored_accounts;
run_loop.Quit();
}));
run_loop.Run();
return accounts;
......@@ -129,19 +138,26 @@ class AccountManagerObserver : public AccountManager::Observer {
AccountManagerObserver() = default;
~AccountManagerObserver() override = default;
void OnTokenUpserted(const AccountManager::AccountKey& account_key) override {
void OnTokenUpserted(const AccountManager::Account& account) override {
is_token_upserted_callback_called_ = true;
accounts_.insert(account_key);
accounts_.insert(account.key);
last_upserted_account_key_ = account.key;
last_upserted_account_email_ = account.raw_email;
}
void OnAccountRemoved(
const AccountManager::AccountKey& account_key) override {
void OnAccountRemoved(const AccountManager::Account& account) override {
is_account_removed_callback_called_ = true;
accounts_.erase(account_key);
accounts_.erase(account.key);
last_removed_account_key_ = account.key;
last_removed_account_email_ = account.raw_email;
}
bool is_token_upserted_callback_called_ = false;
bool is_account_removed_callback_called_ = false;
AccountManager::AccountKey last_upserted_account_key_;
std::string last_upserted_account_email_;
AccountManager::AccountKey last_removed_account_key_;
std::string last_removed_account_email_;
std::set<AccountManager::AccountKey> accounts_;
private:
......@@ -178,10 +194,11 @@ TEST_F(AccountManagerTest, TestInitialization) {
TEST_F(AccountManagerTest, TestUpsert) {
account_manager_->UpsertAccount(kGaiaAccountKey_, kRawUserEmail, kGaiaToken);
std::vector<AccountManager::AccountKey> accounts = GetAccountsBlocking();
std::vector<AccountManager::Account> accounts = GetAccountsBlocking();
EXPECT_EQ(1UL, accounts.size());
EXPECT_EQ(kGaiaAccountKey_, accounts[0]);
EXPECT_EQ(kGaiaAccountKey_, accounts[0].key);
EXPECT_EQ(kRawUserEmail, accounts[0].raw_email);
}
TEST_F(AccountManagerTest, TestTokenPersistence) {
......@@ -189,10 +206,11 @@ TEST_F(AccountManagerTest, TestTokenPersistence) {
scoped_task_environment_.RunUntilIdle();
ResetAndInitializeAccountManager();
std::vector<AccountManager::AccountKey> accounts = GetAccountsBlocking();
std::vector<AccountManager::Account> accounts = GetAccountsBlocking();
EXPECT_EQ(1UL, accounts.size());
EXPECT_EQ(kGaiaAccountKey_, accounts[0]);
EXPECT_EQ(kGaiaAccountKey_, accounts[0].key);
EXPECT_EQ(kRawUserEmail, accounts[0].raw_email);
EXPECT_EQ(kGaiaToken, account_manager_->accounts_[kGaiaAccountKey_].token);
}
......@@ -239,6 +257,8 @@ TEST_F(AccountManagerTest, ObserversAreNotifiedOnTokenInsertion) {
EXPECT_TRUE(observer->is_token_upserted_callback_called_);
EXPECT_EQ(1UL, observer->accounts_.size());
EXPECT_EQ(kGaiaAccountKey_, *observer->accounts_.begin());
EXPECT_EQ(kGaiaAccountKey_, observer->last_upserted_account_key_);
EXPECT_EQ(kRawUserEmail, observer->last_upserted_account_email_);
account_manager_->RemoveObserver(observer.get());
}
......@@ -258,6 +278,8 @@ TEST_F(AccountManagerTest, ObserversAreNotifiedOnTokenUpdate) {
EXPECT_TRUE(observer->is_token_upserted_callback_called_);
EXPECT_EQ(1UL, observer->accounts_.size());
EXPECT_EQ(kGaiaAccountKey_, *observer->accounts_.begin());
EXPECT_EQ(kGaiaAccountKey_, observer->last_upserted_account_key_);
EXPECT_EQ(kRawUserEmail, observer->last_upserted_account_email_);
account_manager_->RemoveObserver(observer.get());
}
......@@ -283,7 +305,7 @@ TEST_F(AccountManagerTest, RemovedAccountsAreImmediatelyUnavailable) {
account_manager_->UpsertAccount(kGaiaAccountKey_, kRawUserEmail, kGaiaToken);
account_manager_->RemoveAccount(kGaiaAccountKey_);
std::vector<AccountManager::AccountKey> accounts = GetAccountsBlocking();
std::vector<AccountManager::Account> accounts = GetAccountsBlocking();
EXPECT_TRUE(accounts.empty());
}
......@@ -295,7 +317,7 @@ TEST_F(AccountManagerTest, AccountRemovalIsPersistedToDisk) {
ResetAndInitializeAccountManager();
std::vector<AccountManager::AccountKey> accounts = GetAccountsBlocking();
std::vector<AccountManager::Account> accounts = GetAccountsBlocking();
EXPECT_TRUE(accounts.empty());
}
......@@ -310,6 +332,8 @@ TEST_F(AccountManagerTest, ObserversAreNotifiedOnAccountRemoval) {
account_manager_->RemoveAccount(kGaiaAccountKey_);
EXPECT_TRUE(observer->is_account_removed_callback_called_);
EXPECT_TRUE(observer->accounts_.empty());
EXPECT_EQ(kGaiaAccountKey_, observer->last_removed_account_key_);
EXPECT_EQ(kRawUserEmail, observer->last_removed_account_email_);
account_manager_->RemoveObserver(observer.get());
}
......@@ -374,8 +398,8 @@ TEST_F(AccountManagerTest,
AccountManager::kActiveDirectoryDummyToken);
scoped_task_environment_.RunUntilIdle();
EXPECT_FALSE(account_manager_->IsTokenAvailable(kActiveDirectoryAccountKey_));
std::vector<AccountManager::AccountKey> accounts = GetAccountsBlocking();
EXPECT_TRUE(base::ContainsValue(accounts, kActiveDirectoryAccountKey_));
std::vector<AccountManager::Account> accounts = GetAccountsBlocking();
EXPECT_TRUE(IsAccountKeyPresent(accounts, kActiveDirectoryAccountKey_));
}
TEST_F(AccountManagerTest, IsTokenAvailableReturnsTrueForInvalidTokens) {
......@@ -384,8 +408,8 @@ TEST_F(AccountManagerTest, IsTokenAvailableReturnsTrueForInvalidTokens) {
AccountManager::kInvalidToken);
scoped_task_environment_.RunUntilIdle();
EXPECT_TRUE(account_manager_->IsTokenAvailable(kGaiaAccountKey_));
std::vector<AccountManager::AccountKey> accounts = GetAccountsBlocking();
EXPECT_TRUE(base::ContainsValue(accounts, kGaiaAccountKey_));
std::vector<AccountManager::Account> accounts = GetAccountsBlocking();
EXPECT_TRUE(IsAccountKeyPresent(accounts, kGaiaAccountKey_));
}
} // namespace 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