Skip to content

GitLab

T
tangled
Commit d6420b4e authored by David Roger's avatar David Roger Committed by Commit Bot
Browse files
Options

[Signin] When using Dice, AccountReconcilor reorders accounts on restart 

Bug: 767004
Change-Id: I809d651345d4c5ae376c2967c58a11c5c1dd94b4
Reviewed-on: https://chromium-review.googlesource.com/674870
Commit-Queue: David Roger <droger@chromium.org>
Reviewed-by: default avatarMihai Sardarescu <msarda@chromium.org>
Cr-Commit-Position: refs/heads/master@{#504341}
parent 3f176c1c
Hide whitespace changes
Inline Side-by-side
Showing with 177 additions and 38 deletions
chrome/browser/signin/account_reconcilor_unittest.cc
View file @ d6420b4e
......@@ -328,6 +328,12 @@ struct AccountReconcilorTestDiceParam {
const char* cookies_after_reconcile;
};
// Pretty prints a AccountReconcilorTestDiceParam. Used by gtest.
void PrintTo(const AccountReconcilorTestDiceParam& param, ::std::ostream* os) {
*os << "Tokens: " << param.tokens << ". Cookies: " << param.cookies
<< ". First reconcile: " << param.is_first_reconcile;
}
// clang-format off
const AccountReconcilorTestDiceParam kDiceParams[] = {
// This table encodes the initial state and expectations of a reconcile.
......@@ -345,56 +351,120 @@ const AccountReconcilorTestDiceParam kDiceParams[] = {
// -------------------------------------------------------------------
// Tokens | Cookies | First Run | Gaia calls | Cookies after reconcile
// -------------------------------------------------------------------
// Sync enabled, first reconcile.
// First reconcile (Chrome restart): Rebuild the Gaia cookie to match the
// tokens. Make the Sync account the default account in the Gaia cookie.
// Sync enabled.
{ "*AB", "AB", true, "", "AB"},
{ "*AB", "BA", true, "XAB", "AB"},
{ "*AB", "A", true, "B", "AB"},
{ "*AB", "B", true, "XAB", "AB"},
{ "*AB", "", true, "AB", "AB"},
// Sync enabled, first reconcile, token error on primary.
// Sync enabled, token error on primary.
{ "*xAB", "AB", true, "X", ""},
{ "*xAB", "BA", true, "X", ""},
{ "*xAB", "BA", true, "XB", "B"},
{ "*xAB", "A", true, "X", ""},
{ "*xAB", "B", true, "X", ""},
{ "*xAB", "", true, "", ""},
// Sync enabled, first reconcile, token error on secondary.
{ "*xAB", "B", true, "", "B"},
{ "*xAB", "", true, "B", "B"},
// Sync enabled, token error on secondary.
{ "*AxB", "AB", true, "XA", "A"},
{ "*AxB", "BA", true, "XA", "A"},
{ "*AxB", "A", true, "", "A"},
{ "*AxB", "B", true, "XA", "A"},
{ "*AxB", "", true, "A", "A"},
// Sync enabled, first reconcile, token error on both accounts.
// Sync enabled, token error on both accounts.
{ "*xAxB", "AB", true, "X", ""},
{ "*xAxB", "BA", true, "X", ""},
{ "*xAxB", "A", true, "X", ""},
{ "*xAxB", "B", true, "X", ""},
{ "*xAxB", "", true, "", ""},
// Sync disabled, first reconcile.
// Sync disabled.
{ "AB", "AB", true, "", "AB"},
{ "AB", "BA", true, "", "BA"},
{ "AB", "A", true, "B", "AB"},
{ "AB", "B", true, "A", "BA"},
{ "AB", "", true, "AB", "AB"},
// Sync disabled, first reconcile, token error on first account.
// Sync disabled, token error on first account.
{ "xAB", "AB", true, "XB", "B"},
{ "xAB", "BA", true, "XB", "B"},
{ "xAB", "A", true, "XB", "B"},
{ "xAB", "B", true, "", "B"},
{ "xAB", "", true, "B", "B"},
// Sync disabled, first reconcile, token error on second account.
// Sync disabled, token error on second account.
{ "AxB", "AB", true, "XA", "A"},
{ "AxB", "BA", true, "XA", "A"},
{ "AxB", "A", true, "", "A"},
{ "AxB", "B", true, "XA", "A"},
{ "AxB", "", true, "A", "A"},
// Sync disabled, first reconcile, token error on both accounts.
// Sync disabled, token error on both accounts.
{ "xAxB", "AB", true, "X", ""},
{ "xAxB", "BA", true, "X", ""},
{ "xAxB", "A", true, "X", ""},
{ "xAxB", "B", true, "X", ""},
{ "xAxB", "", true, "", ""},
// Check that Gaia default account is kept in case of mismatch.
// Chrome is running: Do not change the order of accounts already present in
// the Gaia cookies.
// Sync enabled.
{ "*AB", "AB", false, "", "AB"},
{ "*AB", "BA", false, "", "BA"},
{ "*AB", "A", false, "B", "AB"},
{ "*AB", "B", false, "A", "BA"},
{ "*AB", "", false, "AB", "AB"},
// Sync enabled, token error on primary.
{ "*xAB", "AB", false, "X", ""},
{ "*xAB", "BA", false, "XB", "B"},
{ "*xAB", "A", false, "X", ""},
{ "*xAB", "B", false, "", "B"},
{ "*xAB", "", false, "B", "B"},
// Sync enabled, token error on secondary.
{ "*AxB", "AB", false, "XA", "A"},
// TODO(droger): consider doing XA:
{ "*AxB", "BA", false, "X", ""},
{ "*AxB", "A", false, "", "A"},
// TODO(droger): consider doing XA:
{ "*AxB", "B", false, "X", ""},
{ "*AxB", "", false, "A", "A"},
// Sync enabled, token error on both accounts.
{ "*xAxB", "AB", false, "X", ""},
{ "*xAxB", "BA", false, "X", ""},
{ "*xAxB", "A", false, "X", ""},
{ "*xAxB", "B", false, "X", ""},
{ "*xAxB", "", false, "", ""},
// Sync disabled.
{ "AB", "AB", false, "", "AB"},
{ "AB", "BA", false, "", "BA"},
{ "AB", "A", false, "B", "AB"},
{ "AB", "B", false, "A", "BA"},
{ "AB", "", false, "AB", "AB"},
// Sync disabled, token error on first account.
{ "xAB", "AB", false, "X", ""},
{ "xAB", "BA", false, "XB", "B"},
{ "xAB", "A", false, "X", ""},
{ "xAB", "B", false, "", "B"},
{ "xAB", "", false, "B", "B"},
// Sync disabled, token error on second account.
{ "AxB", "AB", false, "XA", "A"},
{ "AxB", "BA", false, "X", ""},
{ "AxB", "A", false, "", "A"},
{ "AxB", "B", false, "X", ""},
{ "AxB", "", false, "A", "A"},
// Sync disabled, token error on both accounts.
{ "xAxB", "AB", false, "X", ""},
{ "xAxB", "BA", false, "X", ""},
{ "xAxB", "A", false, "X", ""},
{ "xAxB", "B", false, "X", ""},
{ "xAxB", "", false, "", ""},
// Miscellaneous cases.
// Check that unknown Gaia accounts are signed out.
{ "", "A", true, "X", ""},
{ "", "A", false, "X", ""},
{ "*A", "AB", true, "XA", "A"},
{ "*A", "AB", false, "XA", "A"},
// Check that Gaia default account is kept in first position.
{ "AB", "BC", true, "XBA", "BA"},
{ "AB", "BC", false, "XBA", "BA"},
};
// clang-format on
......@@ -470,11 +540,9 @@ TEST_P(AccountReconcilorTestDice, TableRowTest) {
accounts_[GetParam().cookies[1]].gaia_id.c_str());
}
// Dummy reconcile if needed.
if (!GetParam().is_first_reconcile) {
// TODO: dummy reconcile.
NOTREACHED();
}
// Call list accounts now so that the next call completes synchronously.
cookie_manager_service()->ListAccounts(nullptr, nullptr, "foo");
base::RunLoop().RunUntilIdle();
// Setup expectations.
testing::InSequence mock_sequence;
......@@ -501,9 +569,9 @@ TEST_P(AccountReconcilorTestDice, TableRowTest) {
// Reconcile.
AccountReconcilor* reconcilor = GetMockReconcilor();
ASSERT_TRUE(reconcilor->first_execution_);
reconcilor->first_execution_ = GetParam().is_first_reconcile;
reconcilor->StartReconcile();
ASSERT_TRUE(reconcilor->is_reconcile_started_);
base::RunLoop().RunUntilIdle();
for (int i = 0; GetParam().gaia_api_calls[i] != '\0'; ++i) {
if (GetParam().gaia_api_calls[i] == 'X')
continue;
......@@ -515,6 +583,11 @@ TEST_P(AccountReconcilorTestDice, TableRowTest) {
}
ASSERT_FALSE(reconcilor->is_reconcile_started_);
ASSERT_EQ(signin_metrics::ACCOUNT_RECONCILOR_OK, reconcilor->GetState());
// Another reconcile is sometimes triggered if Chrome accounts have changed.
// Allow it to finish.
cookie_manager_service()->SetListAccountsResponseNoAccounts();
cookie_manager_service()->set_list_accounts_stale_for_testing(true);
base::RunLoop().RunUntilIdle();
}
INSTANTIATE_TEST_CASE_P(DiceTable,
......
components/signin/core/browser/account_reconcilor.cc
View file @ d6420b4e
......@@ -375,7 +375,8 @@ void AccountReconcilor::ValidateAccountsFromTokenService() {
// accounts.
for (auto i = chrome_accounts_.begin(); i != chrome_accounts_.end(); ++i) {
if (token_service_->GetDelegate()->RefreshTokenHasError(*i)) {
if (primary_account_ == *i) {
if ((primary_account_ == *i) &&
!signin::IsAccountConsistencyDiceEnabled()) {
primary_account_.clear();
chrome_accounts_.clear();
break;
......@@ -416,19 +417,64 @@ void AccountReconcilor::OnReceivedManageAccountsResponse(
}
}
// There are several cases, depending on the account consistency method and
// whether this is the first execution. The logic can be summarized below:
// * With Mirror, always use the primary account as first Gaia account.
// * With Dice,
// - On first execution, the candidates are examined in this order:
// 1. The primary account
// 2. The current first Gaia account
// 3. The last known first Gaia account
// 4. The first account in the token service
// - On subsequent executions, the order is:
// 1. The current first Gaia account
// * If the Gaia account has a token error, logout everything.
// 2. The primary account
// 3. The last known first Gaia account
// 4. The first account in the token service
std::string AccountReconcilor::GetFirstGaiaAccountForReconcile() const {
// The first account in the cookie should be the primary account if there is
// one.
if (!primary_account_.empty())
if (!signin::IsAccountConsistencyDiceEnabled()) {
// Mirror only uses the primary account, and it is never empty.
DCHECK(!primary_account_.empty());
DCHECK(base::ContainsValue(chrome_accounts_, primary_account_));
return primary_account_;
}
DCHECK(signin::IsAccountConsistencyDiceEnabled());
if (chrome_accounts_.empty())
return std::string(); // No Chrome account, log out.
// Use the current first Gaia account, if we have a token for it.
if (!gaia_accounts_.empty() &&
base::ContainsValue(chrome_accounts_, gaia_accounts_[0].id)) {
return gaia_accounts_[0].id;
// Dice can only change the first Gaia account on the first execution.
if (first_execution_) {
if (!primary_account_.empty()) {
if (base::ContainsValue(chrome_accounts_, primary_account_)) {
return primary_account_;
} else if (!gaia_accounts_.empty() && gaia_accounts_[0].valid &&
(primary_account_ == gaia_accounts_[0].id)) {
// Currently signed-in on Gaia with the primary account, but the token
// is lost. Signout.
// Note: if there is a Sync account without a valid token, and the user
// is currently signed into Gaia with a different account, we don't log
// the user out of Gaia.
return std::string();
}
}
if (!gaia_accounts_.empty() && gaia_accounts_[0].valid &&
base::ContainsValue(chrome_accounts_, gaia_accounts_[0].id)) {
return gaia_accounts_[0].id;
}
} else {
// When this is not the first execution, and there is a valid Gaia account,
// it must be used. If its token is invalid, perform full logout.
if (!gaia_accounts_.empty() && gaia_accounts_[0].valid) {
return base::ContainsValue(chrome_accounts_, gaia_accounts_[0].id)
? gaia_accounts_[0].id
: std::string(); // Main token lost: log out.
}
if (!primary_account_.empty() &&
base::ContainsValue(chrome_accounts_, primary_account_)) {
return primary_account_;
}
}
// If Sync is disabled, and there is no Gaia cookie, try the last known
......@@ -444,20 +490,20 @@ std::string AccountReconcilor::GetFirstGaiaAccountForReconcile() const {
void AccountReconcilor::FinishReconcile() {
VLOG(1) << "AccountReconcilor::FinishReconcile";
DCHECK(add_to_cookie_.empty());
int number_gaia_accounts = gaia_accounts_.size();
std::string first_account = GetFirstGaiaAccountForReconcile();
// |first_account| is either empty or an element of |chrome_accounts_|.
// |first_account| must be in |chrome_accounts_|.
DCHECK(first_account.empty() ||
(std::find(chrome_accounts_.begin(), chrome_accounts_.end(),
first_account) != chrome_accounts_.end()));
bool primary_account_mismatch =
size_t number_gaia_accounts = gaia_accounts_.size();
bool first_account_mismatch =
(number_gaia_accounts > 0) && (first_account != gaia_accounts_[0].id);
// If there are any accounts in the gaia cookie but not in chrome, then
// those accounts need to be removed from the cookie. This means we need
// to blow the cookie away.
int removed_from_cookie = 0;
for (size_t i = 0; i < gaia_accounts_.size(); ++i) {
for (size_t i = 0; i < number_gaia_accounts; ++i) {
if (gaia_accounts_[i].valid &&
chrome_accounts_.end() == std::find(chrome_accounts_.begin(),
chrome_accounts_.end(),
......@@ -466,7 +512,7 @@ void AccountReconcilor::FinishReconcile() {
}
}
bool rebuild_cookie = primary_account_mismatch || removed_from_cookie > 0;
bool rebuild_cookie = first_account_mismatch || (removed_from_cookie > 0);
std::vector<gaia::ListedAccount> original_gaia_accounts =
gaia_accounts_;
if (rebuild_cookie) {
......@@ -478,12 +524,19 @@ void AccountReconcilor::FinishReconcile() {
gaia_accounts_.clear();
}
// Create a list of accounts that need to be added to the gaia cookie.
if (!first_account.empty())
if (first_account.empty()) {
DCHECK(signin::IsAccountConsistencyDiceEnabled());
// Gaia cookie has been cleared or was already empty.
DCHECK((first_account_mismatch && rebuild_cookie) ||
(number_gaia_accounts == 0));
RevokeAllSecondaryTokens();
} else {
// Create a list of accounts that need to be added to the Gaia cookie.
add_to_cookie_.push_back(first_account);
for (size_t i = 0; i < chrome_accounts_.size(); ++i) {
if (chrome_accounts_[i] != first_account)
add_to_cookie_.push_back(chrome_accounts_[i]);
for (size_t i = 0; i < chrome_accounts_.size(); ++i) {
if (chrome_accounts_[i] != first_account)
add_to_cookie_.push_back(chrome_accounts_[i]);
}
}
// For each account known to chrome, PerformMergeAction() if the account is
......@@ -512,7 +565,7 @@ void AccountReconcilor::FinishReconcile() {
signin_metrics::LogSigninAccountReconciliation(
chrome_accounts_.size(), added_to_cookie, removed_from_cookie,
!primary_account_mismatch, first_execution_, number_gaia_accounts);
!first_account_mismatch, first_execution_, number_gaia_accounts);
first_execution_ = false;
CalculateIfReconcileIsDone();
if (!is_reconcile_started_)
......@@ -553,6 +606,15 @@ void AccountReconcilor::ScheduleStartReconcileIfChromeAccountsChanged() {
}
}
void AccountReconcilor::RevokeAllSecondaryTokens() {
for (const std::string& account : chrome_accounts_) {
if (account != primary_account_) {
VLOG(1) << "Revoking token for " << account;
token_service_->RevokeCredentials(account);
}
}
}
// Remove the account from the list that is being merged.
bool AccountReconcilor::MarkAccountAsAddedToCookie(
const std::string& account_id) {
......
components/signin/core/browser/account_reconcilor.h
View file @ d6420b4e
......@@ -180,6 +180,8 @@ class AccountReconcilor : public KeyedService,
void AbortReconcile();
void CalculateIfReconcileIsDone();
void ScheduleStartReconcileIfChromeAccountsChanged();
// Revokes tokens for all accounts in chrome_accounts_ but primary_account_.
void RevokeAllSecondaryTokens();
void ValidateAccountsFromTokenService();
// Note internally that this |account_id| is added to the cookie jar.
......@@ -189,6 +191,8 @@ class AccountReconcilor : public KeyedService,
bool IsTokenServiceReady();
// Returns the first account to add in the Gaia cookie.
// If this returns an empty string, the user must be logged out of all
// accounts.
std::string GetFirstGaiaAccountForReconcile() const;
// Overriden from content_settings::Observer.
......
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