Skip to content

Commit

Permalink
No more filter on syncing accounts when adding a new profile.
Browse files Browse the repository at this point in the history 
Fix filtering of syncing accounts when trying to add a new profile on Lacros.
GetAllAvailableAccounts to return all accounts regardless of Syncing status.

Bug: 1260291
Change-Id: I319d5d62c51639051f4e9a95bdc4158819ed7e07
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/3640954
Reviewed-by: David Roger <droger@chromium.org>
Commit-Queue: Ryan Sultanem <rsult@google.com>
Cr-Commit-Position: refs/heads/main@{#1002532}
  • Loading branch information
Ryan Sultanem authored and Chromium LUCI CQ committed May 12, 2022
1 parent 045be0e commit 75ce54b
Show file tree
Hide file tree
Showing 6 changed files with 33 additions and 120 deletions.
38 changes: 3 additions & 35 deletions chrome/browser/lacros/account_manager/account_manager_util.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -36,28 +36,7 @@ void GetAccountsNotDenylisted(
std::move(callback).Run(result);
}

void GetAccountsAvailableAsPrimaryImpl(
ProfileAttributesStorage* storage,
AccountProfileMapper::ListAccountsCallback callback,
const std::map<base::FilePath, std::vector<account_manager::Account>>&
accounts_map) {
// Collect all primary syncing accounts.
std::vector<std::string> syncing_gaia_ids_temp;
for (ProfileAttributesEntry* entry : storage->GetAllProfilesAttributes()) {
// Skip if not syncing.
if (!entry->IsAuthenticated())
continue;
DCHECK(!entry->GetGAIAId().empty());
syncing_gaia_ids_temp.push_back(entry->GetGAIAId());
}
// Insert them all at once to avoid O(N^2) complexity.
base::flat_set<std::string> syncing_gaia_ids(
std::move(syncing_gaia_ids_temp));

GetAccountsNotDenylisted(accounts_map, syncing_gaia_ids, std::move(callback));
}

void GetAccountsAvailableAsSecondaryImpl(
void GetAllAvailableAccountsImpl(
const base::FilePath& profile_path,
AccountProfileMapper::ListAccountsCallback callback,
const std::map<base::FilePath, std::vector<account_manager::Account>>&
Expand All @@ -80,23 +59,12 @@ void GetAccountsAvailableAsSecondaryImpl(

} // namespace

void GetAccountsAvailableAsPrimary(
AccountProfileMapper* mapper,
ProfileAttributesStorage* storage,
AccountProfileMapper::ListAccountsCallback callback) {
DCHECK(mapper);
DCHECK(storage);
DCHECK(callback);
mapper->GetAccountsMap(base::BindOnce(&GetAccountsAvailableAsPrimaryImpl,
storage, std::move(callback)));
}

void GetAccountsAvailableAsSecondary(
void GetAllAvailableAccounts(
AccountProfileMapper* mapper,
const base::FilePath& profile_path,
AccountProfileMapper::ListAccountsCallback callback) {
DCHECK(mapper);
DCHECK(callback);
mapper->GetAccountsMap(base::BindOnce(&GetAccountsAvailableAsSecondaryImpl,
mapper->GetAccountsMap(base::BindOnce(&GetAllAvailableAccountsImpl,
profile_path, std::move(callback)));
}
15 changes: 1 addition & 14 deletions chrome/browser/lacros/account_manager/account_manager_util.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -7,19 +7,6 @@

#include "chrome/browser/lacros/account_manager/account_profile_mapper.h"

class ProfileAttributesStorage;

// Lists accounts that are available as primary accounts for a new profile. This
// passes back all accounts in the OS that are not used as syncing accounts in
// some other profile. The accounts are returned in an arbitrary order. The only
// async part of this function is retrieving accounts from `mapper`, i.e.
// `callback` gets called iff `mapper` does not get deleted before completion of
// the task.
void GetAccountsAvailableAsPrimary(
AccountProfileMapper* mapper,
ProfileAttributesStorage* storage,
AccountProfileMapper::ListAccountsCallback callback);

// Lists accounts that are available as secondary accounts for profile with
// `profile_path`. This passes back all accounts in the OS, excluding the
// accounts that are already present in the given profile. The accounts are
Expand All @@ -28,7 +15,7 @@ void GetAccountsAvailableAsPrimary(
// async part of this function is retrieving accounts from `mapper`, i.e.
// `callback` gets called iff `mapper` does not get deleted before completion of
// the task.
void GetAccountsAvailableAsSecondary(
void GetAllAvailableAccounts(
AccountProfileMapper* mapper,
const base::FilePath& profile_path,
AccountProfileMapper::ListAccountsCallback callback);
Expand Down
66 changes: 11 additions & 55 deletions chrome/browser/lacros/account_manager/account_manager_util_unittest.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -145,41 +145,7 @@ class AccountManagerUtilTest : public testing::Test {
base::FilePath main_path_;
};

TEST_F(AccountManagerUtilTest, GetAccountsAvailableAsPrimary) {
base::FilePath other_path = GetProfilePath("Other");
std::unique_ptr<AccountProfileMapper> mapper =
CreateMapper({{main_path(), {"A"}}, {other_path, {"B", "C"}}});

base::MockRepeatingCallback<void(const std::vector<Account>&)> mock_callback;

// All the non-syncing accounts are returned.
EXPECT_CALL(mock_callback,
Run(testing::UnorderedElementsAre(
Field(&Account::key, AccountKey{"A", kGaiaType}),
Field(&Account::key, AccountKey{"B", kGaiaType}),
Field(&Account::key, AccountKey{"C", kGaiaType}))));
GetAccountsAvailableAsPrimary(mapper.get(), attributes_storage(),
mock_callback.Get());
testing::Mock::VerifyAndClearExpectations(&mock_callback);

// Check that only the non-syncing accounts are returned.
SetPrimaryAccountForProfile(other_path, "B");
EXPECT_CALL(mock_callback,
Run(testing::UnorderedElementsAre(
Field(&Account::key, AccountKey{"A", kGaiaType}),
Field(&Account::key, AccountKey{"C", kGaiaType}))));
GetAccountsAvailableAsPrimary(mapper.get(), attributes_storage(),
mock_callback.Get());
testing::Mock::VerifyAndClearExpectations(&mock_callback);

SetPrimaryAccountForProfile(main_path(), "A");
EXPECT_CALL(mock_callback, Run(testing::UnorderedElementsAre(Field(
&Account::key, AccountKey{"C", kGaiaType}))));
GetAccountsAvailableAsPrimary(mapper.get(), attributes_storage(),
mock_callback.Get());
}

TEST_F(AccountManagerUtilTest, GetAccountsAvailableAsSecondary) {
TEST_F(AccountManagerUtilTest, GetAllAvailableAccounts) {
base::FilePath second_path = GetProfilePath("Second");
base::FilePath third_path = GetProfilePath("Third");
base::FilePath unassigned = base::FilePath();
Expand All @@ -198,17 +164,15 @@ TEST_F(AccountManagerUtilTest, GetAccountsAvailableAsSecondary) {
Field(&Account::key, AccountKey{"C", kGaiaType}),
Field(&Account::key, AccountKey{"D", kGaiaType}),
Field(&Account::key, AccountKey{"E", kGaiaType}))));
GetAccountsAvailableAsSecondary(mapper.get(), main_path(),
mock_callback.Get());
GetAllAvailableAccounts(mapper.get(), main_path(), mock_callback.Get());
testing::Mock::VerifyAndClearExpectations(&mock_callback);

EXPECT_CALL(mock_callback,
Run(testing::UnorderedElementsAre(
Field(&Account::key, AccountKey{"A", kGaiaType}),
Field(&Account::key, AccountKey{"D", kGaiaType}),
Field(&Account::key, AccountKey{"E", kGaiaType}))));
GetAccountsAvailableAsSecondary(mapper.get(), second_path,
mock_callback.Get());
GetAllAvailableAccounts(mapper.get(), second_path, mock_callback.Get());
testing::Mock::VerifyAndClearExpectations(&mock_callback);

// Syncing status does not change anything here.
Expand All @@ -220,17 +184,15 @@ TEST_F(AccountManagerUtilTest, GetAccountsAvailableAsSecondary) {
Field(&Account::key, AccountKey{"C", kGaiaType}),
Field(&Account::key, AccountKey{"D", kGaiaType}),
Field(&Account::key, AccountKey{"E", kGaiaType}))));
GetAccountsAvailableAsSecondary(mapper.get(), main_path(),
mock_callback.Get());
GetAllAvailableAccounts(mapper.get(), main_path(), mock_callback.Get());
testing::Mock::VerifyAndClearExpectations(&mock_callback);

EXPECT_CALL(mock_callback,
Run(testing::UnorderedElementsAre(
Field(&Account::key, AccountKey{"A", kGaiaType}),
Field(&Account::key, AccountKey{"D", kGaiaType}),
Field(&Account::key, AccountKey{"E", kGaiaType}))));
GetAccountsAvailableAsSecondary(mapper.get(), second_path,
mock_callback.Get());
GetAllAvailableAccounts(mapper.get(), second_path, mock_callback.Get());
testing::Mock::VerifyAndClearExpectations(&mock_callback);

// Non existing profile path or empty profile path returns all accounts.
Expand All @@ -243,10 +205,8 @@ TEST_F(AccountManagerUtilTest, GetAccountsAvailableAsSecondary) {
Field(&Account::key, AccountKey{"D", kGaiaType}),
Field(&Account::key, AccountKey{"E", kGaiaType}))))
.Times(2);
GetAccountsAvailableAsSecondary(mapper.get(), non_existing_path,
mock_callback.Get());
GetAccountsAvailableAsSecondary(mapper.get(), base::FilePath(),
mock_callback.Get());
GetAllAvailableAccounts(mapper.get(), non_existing_path, mock_callback.Get());
GetAllAvailableAccounts(mapper.get(), base::FilePath(), mock_callback.Get());
testing::Mock::VerifyAndClearExpectations(&mock_callback);
}

Expand All @@ -265,16 +225,14 @@ TEST_F(AccountManagerUtilTest, GetAccountsAvailableAsSecondary_Overlapping) {
Run(testing::UnorderedElementsAre(
Field(&Account::key, AccountKey{"C", kGaiaType}),
Field(&Account::key, AccountKey{"E", kGaiaType}))));
GetAccountsAvailableAsSecondary(mapper.get(), main_path(),
mock_callback.Get());
GetAllAvailableAccounts(mapper.get(), main_path(), mock_callback.Get());
testing::Mock::VerifyAndClearExpectations(&mock_callback);

EXPECT_CALL(mock_callback,
Run(testing::UnorderedElementsAre(
Field(&Account::key, AccountKey{"A", kGaiaType}),
Field(&Account::key, AccountKey{"E", kGaiaType}))));
GetAccountsAvailableAsSecondary(mapper.get(), second_path,
mock_callback.Get());
GetAllAvailableAccounts(mapper.get(), second_path, mock_callback.Get());
testing::Mock::VerifyAndClearExpectations(&mock_callback);

// Syncing status does not change anything here.
Expand All @@ -284,15 +242,13 @@ TEST_F(AccountManagerUtilTest, GetAccountsAvailableAsSecondary_Overlapping) {
Run(testing::UnorderedElementsAre(
Field(&Account::key, AccountKey{"C", kGaiaType}),
Field(&Account::key, AccountKey{"E", kGaiaType}))));
GetAccountsAvailableAsSecondary(mapper.get(), main_path(),
mock_callback.Get());
GetAllAvailableAccounts(mapper.get(), main_path(), mock_callback.Get());
testing::Mock::VerifyAndClearExpectations(&mock_callback);

EXPECT_CALL(mock_callback,
Run(testing::UnorderedElementsAre(
Field(&Account::key, AccountKey{"A", kGaiaType}),
Field(&Account::key, AccountKey{"E", kGaiaType}))));
GetAccountsAvailableAsSecondary(mapper.get(), second_path,
mock_callback.Get());
GetAllAvailableAccounts(mapper.get(), second_path, mock_callback.Get());
testing::Mock::VerifyAndClearExpectations(&mock_callback);
}
2 changes: 1 addition & 1 deletion chrome/browser/lacros/account_manager/signin_helper_lacros.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -32,7 +32,7 @@ SigninHelperLacros::SigninHelperLacros(
consistency_cookie_manager->CreateScopedAccountUpdate());
}

GetAccountsAvailableAsSecondary(
GetAllAvailableAccounts(
account_profile_mapper, profile_path,
base::BindOnce(&SigninHelperLacros::OnAccountsAvailableAsSecondaryFetched,
weak_factory_.GetWeakPtr()));
Expand Down
12 changes: 2 additions & 10 deletions chrome/browser/ui/webui/signin/profile_picker_handler.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1193,16 +1193,8 @@ void ProfilePickerHandler::UpdateAvailableAccounts() {
AccountProfileMapper* mapper =
g_browser_process->profile_manager()->GetAccountProfileMapper();

if (IsSelectingSecondaryAccount(web_ui())) {
GetAccountsAvailableAsSecondary(
mapper, GetCurrentProfilePath(web_ui()),
base::BindOnce(&ProfilePickerHandler::GetAvailableAccountsInfo,
weak_factory_.GetWeakPtr()));
return;
}
GetAccountsAvailableAsPrimary(
mapper,
&g_browser_process->profile_manager()->GetProfileAttributesStorage(),
GetAllAvailableAccounts(
mapper, GetCurrentProfilePath(web_ui()),
base::BindOnce(&ProfilePickerHandler::GetAvailableAccountsInfo,
weak_factory_.GetWeakPtr()));
}
Expand Down
20 changes: 15 additions & 5 deletions chrome/browser/ui/webui/signin/profile_picker_handler_unittest.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -355,7 +355,8 @@ TEST_F(ProfilePickerHandlerTest, HandleGetAvailableAccounts_Available) {
EXPECT_EQ("available-accounts-changed", data1.arg1()->GetString());
EXPECT_EQ(data1.arg2()->GetListDeprecated().size(), 2u);

// ****** Account 1 syncing in Secondary profile: return account 2.
// ****** Account 1 syncing in Secondary profile: return account 1 and 2
// regardless of syncing status.
secondary->SetAuthInfo(kGaiaId1, u"example1@gmail.com",
/*is_consented_primary_account=*/true);
// Send message to the handler.
Expand All @@ -366,11 +367,20 @@ TEST_F(ProfilePickerHandlerTest, HandleGetAvailableAccounts_Available) {
const content::TestWebUI::CallData& data2 = *web_ui()->call_data().back();
EXPECT_EQ("cr.webUIListenerCallback", data2.function_name());
EXPECT_EQ("available-accounts-changed", data2.arg1()->GetString());
EXPECT_EQ(data2.arg2()->GetListDeprecated().size(), 1u);
const std::string* gaia_id =
EXPECT_EQ(data2.arg2()->GetListDeprecated().size(), 2u);
// Arbitrary order of results; using a set to perform the search without
// order.
base::flat_set<std::string> gaia_id_results;
const std::string* gaia_id1 =
data2.arg2()->GetListDeprecated()[0].FindStringPath("gaiaId");
EXPECT_NE(gaia_id, nullptr);
EXPECT_EQ(*gaia_id, kGaiaId2);
EXPECT_NE(gaia_id1, nullptr);
gaia_id_results.insert(*gaia_id1);
const std::string* gaia_id2 =
data2.arg2()->GetListDeprecated()[1].FindStringPath("gaiaId");
EXPECT_NE(gaia_id2, nullptr);
gaia_id_results.insert(*gaia_id2);
EXPECT_TRUE(gaia_id_results.contains(kGaiaId1));
EXPECT_TRUE(gaia_id_results.contains(kGaiaId2));
// TODO(https://crbug/1226050): Test all other fields.
}

Expand Down

0 comments on commit 75ce54b

Please sign in to comment.