Reland "[signin] Don't clear unconsented primary account on cancelling Sync dialog"
This is a reland of f21f4c91601bb2cf0af8527bc3b0cb3c6526f3b5
The CL is the same as the one that was reverted.
The failing Sync test was modified in the meantime by CL
https://chromium-review.googlesource.com/c/chromium/src/+/1948890
and should now pass.
Original change's description:
> [signin] Don't clear unconsented primary account on cancelling Sync dialog
>
> The unconsented primary account doesn't necessarily change when the Sync
> confirmation dialog is cancelled.
> In particular this is the case when PrimaryAccountMutator::ClearPrimaryAccount
> is called with RemoveAccountOptions::kKeepAll.
>
> This CL stops resetting the unconsented primary account as part of the signout
> process.
> The unconsented primary account is recomputed anyway when GoogleSignedOut is
> called.
>
>
> Fixed: 1029701
> Change-Id: Id46bb468952efe63170a13872519fcaded076cb1
> Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1944862
> Commit-Queue: David Roger <droger@chromium.org>
> Reviewed-by: Monica Basta <msalama@chromium.org>
> Cr-Commit-Position: refs/heads/master@{#720502}
TBR=msalama
Fixed: 1029701
Change-Id: I26e872f140db99d723037c16dc55219df5a0c706
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1947647
Reviewed-by: David Roger <droger@chromium.org>
Commit-Queue: David Roger <droger@chromium.org>
Cr-Commit-Position: refs/heads/master@{#721027}
diff --git a/components/signin/internal/identity_manager/primary_account_manager.cc b/components/signin/internal/identity_manager/primary_account_manager.cc
index 0156e40..aebcdb1 100644
--- a/components/signin/internal/identity_manager/primary_account_manager.cc
+++ b/components/signin/internal/identity_manager/primary_account_manager.cc
@@ -327,7 +327,7 @@
const CoreAccountInfo account_info = GetAuthenticatedAccountInfo();
client_->GetPrefs()->ClearPref(prefs::kGoogleServicesHostedDomain);
- SetPrimaryAccountInternal(CoreAccountInfo(), /*consented_to_sync=*/false);
+ SetPrimaryAccountInternal(account_info, /*consented_to_sync=*/false);
// Revoke all tokens before sending signed_out notification, because there
// may be components that don't listen for token service events when the
@@ -351,10 +351,8 @@
break;
}
- for (Observer& observer : observers_) {
+ for (Observer& observer : observers_)
observer.GoogleSignedOut(account_info);
- observer.UnconsentedPrimaryAccountChanged(primary_account_info());
- }
}
void PrimaryAccountManager::OnRefreshTokensLoaded() {
diff --git a/components/signin/internal/identity_manager/primary_account_manager_unittest.cc b/components/signin/internal/identity_manager/primary_account_manager_unittest.cc
index 077d919d..62544a76 100644
--- a/components/signin/internal/identity_manager/primary_account_manager_unittest.cc
+++ b/components/signin/internal/identity_manager/primary_account_manager_unittest.cc
@@ -154,13 +154,16 @@
EXPECT_FALSE(manager_->IsAuthenticated());
EXPECT_TRUE(manager_->GetAuthenticatedAccountInfo().email.empty());
EXPECT_TRUE(manager_->GetAuthenticatedAccountId().empty());
+ EXPECT_EQ(main_account_id,
+ manager_->GetUnconsentedPrimaryAccountInfo().account_id);
// Should not be persisted anymore
ShutDownManager();
CreatePrimaryAccountManager();
EXPECT_FALSE(manager_->IsAuthenticated());
EXPECT_TRUE(manager_->GetAuthenticatedAccountInfo().email.empty());
EXPECT_TRUE(manager_->GetAuthenticatedAccountId().empty());
- EXPECT_EQ(CoreAccountInfo(), manager_->GetUnconsentedPrimaryAccountInfo());
+ EXPECT_EQ(main_account_id,
+ manager_->GetUnconsentedPrimaryAccountInfo().account_id);
}
TEST_F(PrimaryAccountManagerTest, SignOutRevoke) {
@@ -204,8 +207,9 @@
std::vector<CoreAccountId> expected_tokens = {main_account_id,
other_account_id};
EXPECT_EQ(expected_tokens, token_service_.GetAccounts());
- EXPECT_EQ(manager_->GetUnconsentedPrimaryAccountInfo(),
- manager_->GetAuthenticatedAccountInfo());
+ // Unconsented primary account is not reset.
+ EXPECT_EQ(main_account_id,
+ manager_->GetUnconsentedPrimaryAccountInfo().account_id);
}
TEST_F(PrimaryAccountManagerTest, SignOutDiceWithError) {
diff --git a/components/signin/public/identity_manager/identity_manager.cc b/components/signin/public/identity_manager/identity_manager.cc
index 5447ed0..2f68233f 100644
--- a/components/signin/public/identity_manager/identity_manager.cc
+++ b/components/signin/public/identity_manager/identity_manager.cc
@@ -505,7 +505,7 @@
// On ChromeOS and on mobile platforms, we support only the primary account as
// the unconsented primary account. By this early return, we avoid an extra
// request to GAIA that lists cookie accounts.
- return base::nullopt;
+ return CoreAccountInfo();
#else
AccountsInCookieJarInfo cookie_info = GetAccountsInCookieJar();
diff --git a/components/signin/public/identity_manager/identity_manager_unittest.cc b/components/signin/public/identity_manager/identity_manager_unittest.cc
index 1bc58a28..a7c1c596 100644
--- a/components/signin/public/identity_manager/identity_manager_unittest.cc
+++ b/components/signin/public/identity_manager/identity_manager_unittest.cc
@@ -12,6 +12,7 @@
#include "base/command_line.h"
#include "base/containers/flat_set.h"
#include "base/run_loop.h"
+#include "base/scoped_observer.h"
#include "base/stl_util.h"
#include "base/test/bind_test_util.h"
#include "base/test/task_environment.h"
@@ -1558,6 +1559,47 @@
EXPECT_EQ(identity_manager()->GetUnconsentedPrimaryAccountInfo(), empty_info);
}
+TEST_F(IdentityManagerTest, UnconsentedPrimaryAccountNotChangedOnSignout) {
+ // Setup cookies and token for the main account.
+ CoreAccountInfo account_info;
+ account_info.email = kTestEmail;
+ account_info.gaia = kTestGaiaId;
+ account_info.account_id =
+ identity_manager()->PickAccountIdForAccount(kTestGaiaId, kTestEmail);
+ SetCookieAccounts(identity_manager(), test_url_loader_factory(),
+ {{account_info.email, account_info.gaia}});
+ SetRefreshTokenForAccount(identity_manager(), account_info.account_id,
+ "refresh-token");
+ EXPECT_EQ(account_info,
+ identity_manager()->GetUnconsentedPrimaryAccountInfo());
+ EXPECT_EQ(account_info, identity_manager()->GetPrimaryAccountInfo());
+ EXPECT_TRUE(identity_manager()->HasPrimaryAccountWithRefreshToken());
+
+ // Tests that OnUnconsentedPrimaryAccountChanged is never called.
+ class UnconsentedPrimaryAccountObserver : public IdentityManager::Observer {
+ public:
+ void OnUnconsentedPrimaryAccountChanged(
+ const CoreAccountInfo& unconsented_primary_account_info) override {
+ NOTREACHED();
+ called_ = true;
+ }
+ bool called_ = false;
+ };
+ UnconsentedPrimaryAccountObserver observer;
+ ScopedObserver<IdentityManager, IdentityManager::Observer> scoped_observer(
+ &observer);
+ scoped_observer.Add(identity_manager());
+ // Clear primary account but do not delete the account.
+ ClearPrimaryAccount(identity_manager(),
+ ClearPrimaryAccountPolicy::KEEP_ALL_ACCOUNTS);
+ // Primary account is cleared, but unconsented account is not.
+ EXPECT_FALSE(identity_manager()->HasPrimaryAccount());
+ EXPECT_EQ(account_info,
+ identity_manager()->GetUnconsentedPrimaryAccountInfo());
+ // OnUnconsentedPrimaryAccountChanged was not fired.
+ EXPECT_FALSE(observer.called_);
+}
+
TEST_F(IdentityManagerTest,
UnconsentedPrimaryAccountTokenRevokedWithStaleCookies) {
ClearPrimaryAccount(identity_manager(), ClearPrimaryAccountPolicy::DEFAULT);
