0

Remove remove_account_mappings_with_email_key from GCM code

For >99% of users, the account ID was migrated from email to Gaia ID.
The GCM code that deleted all account mappings that have an email as a
key can now be deleted (especially to avoid mistakes that could delete
this data due to the wrong value of a boolean).

I think the risk is that for a handful of users, data mapped to the
wrong account may be left on machine even after they are migrated.

Bug: 1445535

Change-Id: I0a28e9bc9ae8db3a4d6f10435fdddbdda904ee6c
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/4528244
Reviewed-by: Peter Beverloo <peter@chromium.org>
Auto-Submit: Mihai Sardarescu <msarda@chromium.org>
Reviewed-by: David Roger <droger@chromium.org>
Commit-Queue: Mihai Sardarescu <msarda@chromium.org>
Cr-Commit-Position: refs/heads/main@{#1160685}
This commit is contained in:
Mihai Sardarescu
2023-06-21 16:09:27 +00:00
committed by Chromium LUCI CQ
parent f6b611d980
commit 65950e8c87
20 changed files with 15 additions and 73 deletions

@ -1347,7 +1347,6 @@ void BrowserProcessImpl::CreateGCMDriver() {
gcm_driver_ = gcm::CreateGCMDriverDesktop(
base::WrapUnique(new gcm::GCMClientFactory), local_state(), store_path,
/*remove_account_mappings_with_email_key=*/false,
base::BindRepeating(&RequestProxyResolvingSocketFactory),
system_network_context_manager()->GetSharedURLLoaderFactory(),
content::GetNetworkConnectionTracker(), chrome::GetChannel(),

@ -72,7 +72,6 @@ FakeGCMClient::~FakeGCMClient() {
void FakeGCMClient::Initialize(
const ChromeBuildInfo& chrome_build_info,
const base::FilePath& store_path,
bool remove_account_mappings_with_email_key,
const scoped_refptr<base::SequencedTaskRunner>& blocking_task_runner,
scoped_refptr<base::SequencedTaskRunner> io_task_runner,
base::RepeatingCallback<void(

@ -51,7 +51,6 @@ class FakeGCMClient : public GCMClient {
void Initialize(
const ChromeBuildInfo& chrome_build_info,
const base::FilePath& store_path,
bool remove_account_mappings_with_email_key,
const scoped_refptr<base::SequencedTaskRunner>& blocking_task_runner,
scoped_refptr<base::SequencedTaskRunner> io_task_runner,
base::RepeatingCallback<void(

@ -236,9 +236,6 @@ class GCMClient {
// connection. Must be called on |io_task_runner|.
// |chrome_build_info|: chrome info, i.e., version, channel and etc.
// |store_path|: path to the GCM store.
// |remove_account_mappings_with_email_key|: Whether account mappings having
// email as account key should be removed while loading. Required
// during the migration of account identifier from email to Gaia ID.
// |blocking_task_runner|: for running blocking file tasks.
// |io_task_runner|: for running IO tasks. When provided, it could be a
// wrapper on top of base::SingleThreadTaskRunner::GetCurrentDefault() to
@ -252,7 +249,6 @@ class GCMClient {
virtual void Initialize(
const ChromeBuildInfo& chrome_build_info,
const base::FilePath& store_path,
bool remove_account_mappings_with_email_key,
const scoped_refptr<base::SequencedTaskRunner>& blocking_task_runner,
scoped_refptr<base::SequencedTaskRunner> io_task_runner,
base::RepeatingCallback<void(

@ -279,7 +279,6 @@ GCMClientImpl::~GCMClientImpl() = default;
void GCMClientImpl::Initialize(
const ChromeBuildInfo& chrome_build_info,
const base::FilePath& path,
bool remove_account_mappings_with_email_key,
const scoped_refptr<base::SequencedTaskRunner>& blocking_task_runner,
scoped_refptr<base::SequencedTaskRunner> io_task_runner,
base::RepeatingCallback<void(
@ -298,9 +297,8 @@ void GCMClientImpl::Initialize(
url_loader_factory_ = url_loader_factory;
network_connection_tracker_ = network_connection_tracker;
chrome_build_info_ = chrome_build_info;
gcm_store_ = std::make_unique<GCMStoreImpl>(
path, remove_account_mappings_with_email_key, blocking_task_runner,
std::move(encryptor));
gcm_store_ = std::make_unique<GCMStoreImpl>(path, blocking_task_runner,
std::move(encryptor));
delegate_ = delegate;
io_task_runner_ = std::move(io_task_runner);
recorder_.SetDelegate(this);

@ -121,7 +121,6 @@ class GCMClientImpl
void Initialize(
const ChromeBuildInfo& chrome_build_info,
const base::FilePath& store_path,
bool remove_account_mappings_with_email_key,
const scoped_refptr<base::SequencedTaskRunner>& blocking_task_runner,
scoped_refptr<base::SequencedTaskRunner> io_task_runner,
base::RepeatingCallback<void(

@ -612,7 +612,6 @@ void GCMClientImplTest::InitializeGCMClient() {
chrome_build_info.product_category_for_subtypes = kProductCategoryForSubtypes;
gcm_client_->Initialize(
chrome_build_info, gcm_store_path(),
/*remove_account_mappings_with_email_key=*/true,
task_environment_.GetMainThreadTaskRunner(),
base::SingleThreadTaskRunner::GetCurrentDefault(), base::DoNothing(),
base::MakeRefCounted<network::WeakWrapperSharedURLLoaderFactory>(

@ -78,7 +78,6 @@ std::unique_ptr<GCMDriver> CreateGCMDriverDesktop(
std::unique_ptr<GCMClientFactory> gcm_client_factory,
PrefService* prefs,
const base::FilePath& store_path,
bool remove_account_mappings_with_email_key,
base::RepeatingCallback<void(
mojo::PendingReceiver<network::mojom::ProxyResolvingSocketFactory>)>
get_socket_factory_callback,
@ -92,8 +91,7 @@ std::unique_ptr<GCMDriver> CreateGCMDriverDesktop(
return std::unique_ptr<GCMDriver>(new GCMDriverDesktop(
std::move(gcm_client_factory),
GetChromeBuildInfo(channel, product_category_for_subtypes), prefs,
store_path, remove_account_mappings_with_email_key,
get_socket_factory_callback, std::move(url_loader_factory),
store_path, get_socket_factory_callback, std::move(url_loader_factory),
network_connection_tracker, ui_task_runner, io_task_runner,
blocking_task_runner));
}

@ -32,7 +32,6 @@ std::unique_ptr<GCMDriver> CreateGCMDriverDesktop(
std::unique_ptr<GCMClientFactory> gcm_client_factory,
PrefService* prefs,
const base::FilePath& store_path,
bool remove_account_mappings_with_email_key,
base::RepeatingCallback<void(
mojo::PendingReceiver<network::mojom::ProxyResolvingSocketFactory>)>
get_socket_factory_callback,

@ -73,7 +73,6 @@ class GCMDriverDesktop::IOWorker : public GCMClient::Delegate {
std::unique_ptr<GCMClientFactory> gcm_client_factory,
const GCMClient::ChromeBuildInfo& chrome_build_info,
const base::FilePath& store_path,
bool remove_account_mappings_with_email_key,
base::RepeatingCallback<void(
mojo::PendingReceiver<network::mojom::ProxyResolvingSocketFactory>)>
get_socket_factory_callback,
@ -147,7 +146,6 @@ void GCMDriverDesktop::IOWorker::Initialize(
std::unique_ptr<GCMClientFactory> gcm_client_factory,
const GCMClient::ChromeBuildInfo& chrome_build_info,
const base::FilePath& store_path,
bool remove_account_mappings_with_email_key,
base::RepeatingCallback<void(
mojo::PendingReceiver<network::mojom::ProxyResolvingSocketFactory>)>
get_socket_factory_callback,
@ -163,11 +161,10 @@ void GCMDriverDesktop::IOWorker::Initialize(
network::SharedURLLoaderFactory::Create(
std::move(pending_loader_factory));
gcm_client_->Initialize(
chrome_build_info, store_path, remove_account_mappings_with_email_key,
blocking_task_runner, io_thread_, std::move(get_socket_factory_callback),
url_loader_factory_for_io, network_connection_tracker,
std::make_unique<SystemEncryptor>(), this);
gcm_client_->Initialize(chrome_build_info, store_path, blocking_task_runner,
io_thread_, std::move(get_socket_factory_callback),
url_loader_factory_for_io, network_connection_tracker,
std::make_unique<SystemEncryptor>(), this);
}
void GCMDriverDesktop::IOWorker::OnRegisterFinished(
@ -499,7 +496,6 @@ GCMDriverDesktop::GCMDriverDesktop(
const GCMClient::ChromeBuildInfo& chrome_build_info,
PrefService* prefs,
const base::FilePath& store_path,
bool remove_account_mappings_with_email_key,
base::RepeatingCallback<void(
mojo::PendingReceiver<network::mojom::ProxyResolvingSocketFactory>)>
get_socket_factory_callback,
@ -527,8 +523,7 @@ GCMDriverDesktop::GCMDriverDesktop(
base::BindOnce(
&GCMDriverDesktop::IOWorker::Initialize,
base::Unretained(io_worker_.get()), std::move(gcm_client_factory),
chrome_build_info, store_path, remove_account_mappings_with_email_key,
std::move(get_socket_factory_callback),
chrome_build_info, store_path, std::move(get_socket_factory_callback),
// ->Clone() permits creation of an equivalent
// SharedURLLoaderFactory on IO thread.
url_loader_factory_for_ui->Clone(),

@ -49,15 +49,11 @@ class GCMDelayedTaskController;
class GCMDriverDesktop : public GCMDriver,
protected InstanceIDHandler {
public:
// |remove_account_mappings_with_email_key| indicates whether account mappings
// having email as account key should be removed while loading. This is
// required during the migration of account identifier from email to Gaia ID.
GCMDriverDesktop(
std::unique_ptr<GCMClientFactory> gcm_client_factory,
const GCMClient::ChromeBuildInfo& chrome_build_info,
PrefService* prefs,
const base::FilePath& store_path,
bool remove_account_mappings_with_email_key,
base::RepeatingCallback<void(
mojo::PendingReceiver<network::mojom::ProxyResolvingSocketFactory>)>
get_socket_factory_callback,

@ -247,8 +247,7 @@ void GCMDriverTest::CreateDriver() {
std::unique_ptr<GCMClientFactory>(new FakeGCMClientFactory(
base::SingleThreadTaskRunner::GetCurrentDefault(),
io_thread_.task_runner())),
chrome_build_info, &prefs_, temp_dir_.GetPath(),
/*remove_account_mappings_with_email_key=*/true, base::DoNothing(),
chrome_build_info, &prefs_, temp_dir_.GetPath(), base::DoNothing(),
base::MakeRefCounted<network::WeakWrapperSharedURLLoaderFactory>(
&test_url_loader_factory_),
network::TestNetworkConnectionTracker::GetInstance(),

@ -153,8 +153,7 @@ void GCMDriverBaseTest::CreateDriver() {
std::make_unique<FakeGCMClientFactory>(
base::SingleThreadTaskRunner::GetCurrentDefault(),
io_thread_.task_runner()),
chrome_build_info, &prefs_, temp_dir_.GetPath(),
/*remove_account_mappings_with_email_key=*/true, base::DoNothing(),
chrome_build_info, &prefs_, temp_dir_.GetPath(), base::DoNothing(),
base::MakeRefCounted<network::WeakWrapperSharedURLLoaderFactory>(
&test_url_loader_factory_),
network::TestNetworkConnectionTracker::GetInstance(),

@ -158,21 +158,9 @@ GCMProfileService::GCMProfileService(
scoped_refptr<base::SequencedTaskRunner>& blocking_task_runner)
: identity_manager_(identity_manager),
url_loader_factory_(std::move(url_loader_factory)) {
#if BUILDFLAG(IS_CHROMEOS_ASH)
signin::IdentityManager::AccountIdMigrationState id_migration =
identity_manager_->GetAccountIdMigrationState();
bool remove_account_mappings_with_email_key =
(id_migration == signin::IdentityManager::MIGRATION_IN_PROGRESS) ||
(id_migration == signin::IdentityManager::MIGRATION_DONE);
#else
// Migration is done on non-ChromeOS platforms.
bool remove_account_mappings_with_email_key = false;
#endif
driver_ = CreateGCMDriverDesktop(
std::move(gcm_client_factory), prefs,
path.Append(gcm_driver::kGCMStoreDirname),
remove_account_mappings_with_email_key,
base::BindRepeating(get_socket_factory_callback,
weak_ptr_factory_.GetWeakPtr()),
url_loader_factory_, network_connection_tracker, channel,

@ -198,7 +198,6 @@ class GCMStoreImpl::Backend
: public base::RefCountedThreadSafe<GCMStoreImpl::Backend> {
public:
Backend(const base::FilePath& path,
bool remove_account_mappings_with_email_key,
scoped_refptr<base::SequencedTaskRunner> foreground_runner,
std::unique_ptr<Encryptor> encryptor);
@ -272,7 +271,6 @@ class GCMStoreImpl::Backend
bool LoadInstanceIDData(std::map<std::string, std::string>* instance_id_data);
const base::FilePath path_;
bool remove_account_mappings_with_email_key_;
scoped_refptr<base::SequencedTaskRunner> foreground_task_runner_;
std::unique_ptr<Encryptor> encryptor_;
@ -281,12 +279,9 @@ class GCMStoreImpl::Backend
GCMStoreImpl::Backend::Backend(
const base::FilePath& path,
bool remove_account_mappings_with_email_key,
scoped_refptr<base::SequencedTaskRunner> foreground_task_runner,
std::unique_ptr<Encryptor> encryptor)
: path_(path),
remove_account_mappings_with_email_key_(
remove_account_mappings_with_email_key),
foreground_task_runner_(foreground_task_runner),
encryptor_(std::move(encryptor)) {}
@ -1146,13 +1141,7 @@ bool GCMStoreImpl::Backend::LoadAccountMappingInfo(
}
for (const auto& account_mapping : loaded_account_mappings) {
bool remove = remove_account_mappings_with_email_key_ &&
account_mapping.account_id.IsEmail();
if (remove) {
RemoveAccountMapping(account_mapping.account_id, base::DoNothing());
} else {
account_mappings->push_back(account_mapping);
}
account_mappings->push_back(account_mapping);
}
return true;
@ -1227,11 +1216,9 @@ bool GCMStoreImpl::Backend::LoadInstanceIDData(
GCMStoreImpl::GCMStoreImpl(
const base::FilePath& path,
bool remove_account_mappings_with_email_key,
scoped_refptr<base::SequencedTaskRunner> blocking_task_runner,
std::unique_ptr<Encryptor> encryptor)
: backend_(new Backend(path,
remove_account_mappings_with_email_key,
base::SingleThreadTaskRunner::GetCurrentDefault(),
std::move(encryptor))),
blocking_task_runner_(blocking_task_runner) {}

@ -26,11 +26,7 @@ class Encryptor;
// all callbacks to the thread on which the GCMStoreImpl is created.
class GCM_EXPORT GCMStoreImpl : public GCMStore {
public:
// |remove_account_mappings_with_email_key| indicates whether account mappings
// having email as account key should be removed while loading. This is
// required during the migration of account identifier from email to Gaia ID.
GCMStoreImpl(const base::FilePath& path,
bool remove_account_mappings_with_email_key,
scoped_refptr<base::SequencedTaskRunner> blocking_task_runner,
std::unique_ptr<Encryptor> encryptor);

@ -97,8 +97,7 @@ std::unique_ptr<GCMStoreImpl> GCMStoreImplTest::BuildGCMStore() {
store_path_ =
temp_directory_.GetPath().Append(FILE_PATH_LITERAL("GCM Store"));
return std::make_unique<GCMStoreImpl>(
store_path_, /*remove_account_mappings_with_email_key=*/true,
task_environment_.GetMainThreadTaskRunner(),
store_path_, task_environment_.GetMainThreadTaskRunner(),
std::make_unique<FakeEncryptor>());
}

@ -219,9 +219,7 @@ void MCSClientTest::TearDown() {
void MCSClientTest::BuildMCSClient() {
gcm_store_ = std::make_unique<GCMStoreImpl>(
temp_directory_.GetPath(),
/*remove_account_mappings_with_email_key=*/true,
task_environment_.GetMainThreadTaskRunner(),
temp_directory_.GetPath(), task_environment_.GetMainThreadTaskRunner(),
base::WrapUnique<Encryptor>(new FakeEncryptor));
mcs_client_ = std::make_unique<TestMCSClient>(
&clock_, &connection_factory_, gcm_store_.get(),

@ -298,8 +298,8 @@ void MCSProbe::Start() {
base::SingleThreadTaskRunner::GetCurrentDefault(), &recorder_,
network_connection_tracker_.get());
gcm_store_ = std::make_unique<GCMStoreImpl>(
gcm_store_path_, /*remove_account_mappings_with_email_key=*/true,
file_thread_.task_runner(), std::make_unique<FakeEncryptor>());
gcm_store_path_, file_thread_.task_runner(),
std::make_unique<FakeEncryptor>());
mcs_client_ = std::make_unique<MCSClient>(
"probe", &clock_, connection_factory_.get(), gcm_store_.get(),

@ -589,7 +589,6 @@ void ApplicationContextImpl::CreateGCMDriver() {
gcm_driver_ = gcm::CreateGCMDriverDesktop(
base::WrapUnique(new gcm::GCMClientFactory), GetLocalState(), store_path,
/*remove_account_mappings_with_email_key=*/true,
// Because ApplicationContextImpl is destroyed after all WebThreads have
// been shut down, base::Unretained() is safe here.
base::BindRepeating(&RequestProxyResolvingSocketFactory,