Commit 2ac90d1e authored by Colin Blundell's avatar Colin Blundell Committed by Commit Bot

Fix IdentityManager::HasPrimaryAccount()

"Signing out" always happens via SigninManager and causes the account to
be removed from SigninManager as well as AccountTrackerService. However,
the primary account can be removed from AccountTrackerService in
circumstances *other than* the user signing out of the primary account;
e.g., if the refresh token for the account is revoked from
ProfileOAuth2TokenService. In these circumstances, there would still be
a primary account (i.e., SigninManagerBase::GetAuthenticatedAccountId()
returns a valid account ID), but there would be no AccountInfo for the
account (i.e., SigninManagerBase::GetAuthenticatedAccountInfo() returns
an empty AccountInfo).

IdentityManager::HasPrimaryAccount() currently internally uses
SigninManagerBase::GetAuthenticatedAccountInfo(). This will result in it
giving an incorrect result in these edge cases. This CL changes it to
use SigninManagerBase::IsAuthenticated() and adds a test that fails
before the change.

Bug: 892553
Change-Id: I227ad6767089b259c0d54e33f5f88f6789f2de9a
Reviewed-on: https://chromium-review.googlesource.com/c/1264203Reviewed-by: default avatarDavid Roger <droger@chromium.org>
Commit-Queue: Colin Blundell <blundell@chromium.org>
Cr-Commit-Position: refs/heads/master@{#597064}
parent e6721964
...@@ -69,7 +69,7 @@ AccountInfo IdentityManager::GetPrimaryAccountInfo() const { ...@@ -69,7 +69,7 @@ AccountInfo IdentityManager::GetPrimaryAccountInfo() const {
} }
bool IdentityManager::HasPrimaryAccount() const { bool IdentityManager::HasPrimaryAccount() const {
return !GetPrimaryAccountInfo().account_id.empty(); return signin_manager_->IsAuthenticated();
} }
#if !defined(OS_CHROMEOS) #if !defined(OS_CHROMEOS)
......
...@@ -750,7 +750,15 @@ TEST_F(IdentityManagerTest, PrimaryAccountInfoAfterSigninAndSignout) { ...@@ -750,7 +750,15 @@ TEST_F(IdentityManagerTest, PrimaryAccountInfoAfterSigninAndSignout) {
TEST_F(IdentityManagerTest, HasPrimaryAccount) { TEST_F(IdentityManagerTest, HasPrimaryAccount) {
EXPECT_TRUE(identity_manager()->HasPrimaryAccount()); EXPECT_TRUE(identity_manager()->HasPrimaryAccount());
// Removing the account from the AccountTrackerService should not cause
// IdentityManager to think that there is no longer a primary account.
account_tracker()->RemoveAccount(
identity_manager()->GetPrimaryAccountInfo().account_id);
EXPECT_TRUE(identity_manager()->HasPrimaryAccount());
#if !defined(OS_CHROMEOS) #if !defined(OS_CHROMEOS)
// Signing out should cause IdentityManager to recognize that there is no
// longer a primary account.
base::RunLoop run_loop; base::RunLoop run_loop;
identity_manager_observer()->set_on_primary_account_cleared_callback( identity_manager_observer()->set_on_primary_account_cleared_callback(
run_loop.QuitClosure()); run_loop.QuitClosure());
......
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