Commit 3df86946 authored by Marc Treib's avatar Marc Treib Committed by Commit Bot

Fix Password Sync in Local Sync mode (i.e. roaming profiles)

When PasswordModelTypeController was updated to support the
account-scoped password storage, a condition was added to its
GetPreconditionState() method to make sure Password Sync only runs if
either Sync-the-feature is enabled, or the user has opted in to the
account-scoped storage.

However, for the case of local Sync, Sync-the-feature is *not*
considered enabled (and of course no account-storage-opt-in exists).
So this broke local Sync for Passwords.

Bug: 1108609
Change-Id: I2bb2edb7247d3b2e95dba306718488352329840b
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2316214
Commit-Queue: Marc Treib <treib@chromium.org>
Reviewed-by: default avatarMikel Astiz <mastiz@chromium.org>
Cr-Commit-Position: refs/heads/master@{#791782}
parent 21f77aaa
...@@ -16,6 +16,7 @@ ...@@ -16,6 +16,7 @@
#include "chrome/browser/ui/browser.h" #include "chrome/browser/ui/browser.h"
#include "chrome/test/base/in_process_browser_test.h" #include "chrome/test/base/in_process_browser_test.h"
#include "components/browser_sync/browser_sync_switches.h" #include "components/browser_sync/browser_sync_switches.h"
#include "components/sync/base/model_type.h"
#include "components/sync/driver/profile_sync_service.h" #include "components/sync/driver/profile_sync_service.h"
#include "components/sync/driver/sync_driver_switches.h" #include "components/sync/driver/sync_driver_switches.h"
#include "content/public/test/browser_test.h" #include "content/public/test/browser_test.h"
...@@ -77,7 +78,30 @@ IN_PROC_BROWSER_TEST_F(LocalSyncTest, ShouldStart) { ...@@ -77,7 +78,30 @@ IN_PROC_BROWSER_TEST_F(LocalSyncTest, ShouldStart) {
EXPECT_TRUE(service->IsLocalSyncEnabled()); EXPECT_TRUE(service->IsLocalSyncEnabled());
// Verify that the expected set of data types successfully started up.
// If this test fails after adding a new data type, carefully consider whether
// the type should be enabled in Local Sync mode, i.e. for roaming profiles on
// Windows.
// TODO(crbug.com/1109640): Consider whether all of these types should really
// be enabled in Local Sync mode.
EXPECT_EQ(service->GetActiveDataTypes(),
syncer::ModelTypeSet(
syncer::BOOKMARKS, syncer::PREFERENCES, syncer::PASSWORDS,
syncer::AUTOFILL_PROFILE, syncer::AUTOFILL,
syncer::AUTOFILL_WALLET_DATA, syncer::AUTOFILL_WALLET_METADATA,
syncer::THEMES, syncer::TYPED_URLS, syncer::EXTENSIONS,
syncer::SEARCH_ENGINES, syncer::SESSIONS, syncer::APPS,
syncer::APP_SETTINGS, syncer::EXTENSION_SETTINGS,
syncer::HISTORY_DELETE_DIRECTIVES, syncer::DICTIONARY,
syncer::DEVICE_INFO, syncer::PRIORITY_PREFERENCES,
syncer::WEB_APPS, syncer::PROXY_TABS, syncer::NIGORI));
// Verify certain features are disabled. // Verify certain features are disabled.
EXPECT_FALSE(service->GetActiveDataTypes().Has(syncer::USER_CONSENTS));
EXPECT_FALSE(service->GetActiveDataTypes().Has(syncer::USER_EVENTS));
EXPECT_FALSE(service->GetActiveDataTypes().Has(syncer::SECURITY_EVENTS));
EXPECT_FALSE(service->GetActiveDataTypes().Has(syncer::SEND_TAB_TO_SELF));
EXPECT_FALSE(service->GetActiveDataTypes().Has(syncer::SHARING_MESSAGE));
EXPECT_FALSE(send_tab_to_self::IsUserSyncTypeActive(browser()->profile())); EXPECT_FALSE(send_tab_to_self::IsUserSyncTypeActive(browser()->profile()));
} }
#endif // defined(OS_WIN) #endif // defined(OS_WIN)
......
...@@ -129,8 +129,10 @@ syncer::DataTypeController::PreconditionState ...@@ -129,8 +129,10 @@ syncer::DataTypeController::PreconditionState
PasswordModelTypeController::GetPreconditionState() const { PasswordModelTypeController::GetPreconditionState() const {
// If Sync-the-feature is enabled, then the user has opted in to that, and no // If Sync-the-feature is enabled, then the user has opted in to that, and no
// additional opt-in is required here. // additional opt-in is required here.
if (sync_service_->IsSyncFeatureEnabled()) if (sync_service_->IsSyncFeatureEnabled() ||
sync_service_->IsLocalSyncEnabled()) {
return PreconditionState::kPreconditionsMet; return PreconditionState::kPreconditionsMet;
}
// If Sync-the-feature is *not* enabled, then password sync should only be // If Sync-the-feature is *not* enabled, then password sync should only be
// turned on if the user has opted in to the account-scoped storage. // turned on if the user has opted in to the account-scoped storage.
return features_util::IsOptedInForAccountStorage(pref_service_, sync_service_) return features_util::IsOptedInForAccountStorage(pref_service_, sync_service_)
......
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