Skip to content

Commit

Permalink
[UPM] Handle persistent auth errors in PasswordSettingsService.
Browse files Browse the repository at this point in the history 
If sync is broken due to a permanent auth error, setting service should
fallback to pre-UPM behaviour: old cross-platform setting prefs should
be used and no interaction with GMS Core should be made.

(cherry picked from commit 4841b63)

Bug: 1329151
Change-Id: I1a3ba4ec0af3cb7252fafa5390307cb519a335ec
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/3663625
Reviewed-by: Ioana Pandele <ioanap@chromium.org>
Commit-Queue: Maria Kazinova <kazinova@google.com>
Reviewed-by: Vasilii Sukhanov <vasilii@chromium.org>
Cr-Original-Commit-Position: refs/heads/main@{#1009064}
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/3687678
Cr-Commit-Position: refs/branch-heads/5060@{#511}
Cr-Branched-From: b83393d-refs/heads/main@{#1002911}
  • Loading branch information
Maria Kazinova authored and Chromium LUCI CQ committed Jun 3, 2022
1 parent 755b3b6 commit 922bc51
Show file tree
Hide file tree
Showing 7 changed files with 251 additions and 28 deletions.
55 changes: 43 additions & 12 deletions chrome/browser/password_manager/android/password_manager_settings_service_android_impl.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,7 @@

using password_manager::PasswordManagerSetting;
using password_manager::PasswordSettingsUpdaterAndroidBridge;
using password_manager::sync_util::CannotUseUPMDueToPersistentSyncError;
using password_manager::sync_util::IsPasswordSyncEnabled;

namespace {
Expand Down Expand Up @@ -133,6 +134,10 @@ bool PasswordManagerSettingsServiceAndroidImpl::IsSettingEnabled(
return regular_pref->GetValue()->GetBool();
}

if (CannotUseUPMDueToPersistentSyncError(sync_service_)) {
return regular_pref->GetValue()->GetBool();
}

if (!bridge_) {
return regular_pref->GetValue()->GetBool();
}
Expand All @@ -149,12 +154,15 @@ bool PasswordManagerSettingsServiceAndroidImpl::IsSettingEnabled(

void PasswordManagerSettingsServiceAndroidImpl::RequestSettingsFromBackend() {
// Backend has settings data only if passwords are synced.
if (bridge_ && IsPasswordSyncEnabled(sync_service_))
if (bridge_ && IsPasswordSyncEnabled(sync_service_) &&
!CannotUseUPMDueToPersistentSyncError(sync_service_)) {
FetchSettings();
}
}

void PasswordManagerSettingsServiceAndroidImpl::TurnOffAutoSignIn() {
if (!bridge_ || !IsPasswordSyncEnabled(sync_service_)) {
if (!bridge_ || !IsPasswordSyncEnabled(sync_service_) ||
CannotUseUPMDueToPersistentSyncError(sync_service_)) {
pref_service_->SetBoolean(
password_manager::prefs::kCredentialsEnableAutosignin, false);
return;
Expand Down Expand Up @@ -205,6 +213,9 @@ void PasswordManagerSettingsServiceAndroidImpl::OnSettingValueAbsent(
password_manager::PasswordManagerSetting setting) {
DCHECK(bridge_);
UpdateSettingFetchState(setting);
if (CannotUseUPMDueToPersistentSyncError(sync_service_))
return;

if (!IsPasswordSyncEnabled(sync_service_))
return;

Expand All @@ -226,6 +237,9 @@ void PasswordManagerSettingsServiceAndroidImpl::OnSettingValueAbsent(
}

void PasswordManagerSettingsServiceAndroidImpl::MigratePrefsIfNeeded() {
if (CannotUseUPMDueToPersistentSyncError(sync_service_))
return;

if (pref_service_->GetBoolean(
password_manager::prefs::kSettingsMigratedToUPM))
return;
Expand All @@ -243,21 +257,38 @@ void PasswordManagerSettingsServiceAndroidImpl::MigratePrefsIfNeeded() {

void PasswordManagerSettingsServiceAndroidImpl::OnStateChanged(
syncer::SyncService* sync) {
// Return early if the setting didn't change.
if (IsPasswordSyncEnabled(sync) == is_password_sync_enabled_) {
// Settings cannot be fetched from GMS due to a persistent auth error.
// Return early.
if (CannotUseUPMDueToPersistentSyncError(sync_service_)) {
sync_has_persistent_error_ = true;
return;
}

if (IsPasswordSyncEnabled(sync))
DumpChromePrefsIntoGMSPrefs();
bool sync_setting_changed =
IsPasswordSyncEnabled(sync) != is_password_sync_enabled_;
bool sync_error_resolved =
sync_has_persistent_error_ &&
!CannotUseUPMDueToPersistentSyncError(sync_service_);

// Return early if the setting didn't change and no sync errors were resolved.
if (!sync_setting_changed && !sync_error_resolved)
return;

// Fetch settings from the backend to align values stored in GMS Core and
// Chrome.
is_password_sync_enabled_ = IsPasswordSyncEnabled(sync);
fetch_after_sync_status_change_in_progress_ = true;
for (PasswordManagerSetting setting : kAllPasswordSettings)
awaited_settings_.insert(setting);
FetchSettings();
sync_has_persistent_error_ = false;

if (is_password_sync_enabled_)
DumpChromePrefsIntoGMSPrefs();

if (sync_setting_changed ||
(sync_error_resolved && IsPasswordSyncEnabled(sync))) {
// Fetch settings from the backend to align values stored in GMS Core and
// Chrome.
fetch_after_sync_status_change_in_progress_ = true;
for (PasswordManagerSetting setting : kAllPasswordSettings)
awaited_settings_.insert(setting);
FetchSettings();
}
}

void PasswordManagerSettingsServiceAndroidImpl::UpdateSettingFetchState(
Expand Down
3 changes: 3 additions & 0 deletions chrome/browser/password_manager/android/password_manager_settings_service_android_impl.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -104,6 +104,9 @@ class PasswordManagerSettingsServiceAndroidImpl
// Cached value of the password sync setting.
bool is_password_sync_enabled_ = false;

// Cached value of the persistent sync error state.
bool sync_has_persistent_error_ = false;

// True if settings were requested from the backend after password sync
// setting was changed, and the fetch is still in progress.
bool fetch_after_sync_status_change_in_progress_ = false;
Expand Down
179 changes: 179 additions & 0 deletions ...owser/password_manager/android/password_manager_settings_service_android_impl_unittest.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -64,6 +64,8 @@ class PasswordManagerSettingsServiceAndroidImplTest : public testing::Test {

void SetPasswordsSync(bool enabled);
void SetSettingsSync(bool enabled);
void EnablePermanentAuthError();
void ResolvePermanentAuthError();

void AssertInitialMigrationDidntChangePrefs();
void ExpectSettingsRetrievalFromBackend(size_t times);
Expand Down Expand Up @@ -172,6 +174,19 @@ void PasswordManagerSettingsServiceAndroidImplTest::SetSettingsSync(
selected_sync_types);
}

void PasswordManagerSettingsServiceAndroidImplTest::EnablePermanentAuthError() {
GoogleServiceAuthError persistent_error(
GoogleServiceAuthError::INVALID_GAIA_CREDENTIALS);
DCHECK(persistent_error.IsPersistentError());
test_sync_service_.SetAuthError(persistent_error);
}

void PasswordManagerSettingsServiceAndroidImplTest::
ResolvePermanentAuthError() {
GoogleServiceAuthError resolved_error(GoogleServiceAuthError::NONE);
test_sync_service_.SetAuthError(resolved_error);
}

// TODO(crbug.com/1324648): Get rid of this method by not instantiating the
// service when it's not needed from the beginning of the test.
void PasswordManagerSettingsServiceAndroidImplTest::
Expand Down Expand Up @@ -372,6 +387,31 @@ TEST_F(PasswordManagerSettingsServiceAndroidImplTest,
nullptr);
}

TEST_F(PasswordManagerSettingsServiceAndroidImplTest,
TestNewMigrationSyncBroken) {
EnablePermanentAuthError();
ASSERT_FALSE(pref_service()->GetBoolean(
password_manager::prefs::kSettingsMigratedToUPM));
// Set an explicit value on the "Offer to save passwords" pref.
pref_service()->SetBoolean(password_manager::prefs::kCredentialsEnableService,
false);

// No migration should happen if passwords sync is broken, no prefs should
// change and no metrcis should be recorded.
InitializeSettingsService(/*password_sync_enabled=*/true,
/*setting_sync_enabled=*/true);
histogram_tester()->ExpectTotalCount(
"PasswordManager.MigratedSettingsUPMAndroid", 0);
EXPECT_EQ(pref_service()->GetUserPrefValue(
password_manager::prefs::kOfferToSavePasswordsEnabledGMS),
nullptr);
EXPECT_EQ(pref_service()->GetUserPrefValue(
password_manager::prefs::kAutoSignInEnabledGMS),
nullptr);
EXPECT_FALSE(pref_service()->GetBoolean(
password_manager::prefs::kSettingsMigratedToUPM));
}

TEST_F(PasswordManagerSettingsServiceAndroidImplTest,
OnSaveSettingFetchSyncingBoth) {
InitializeSettingsService(/*password_sync_enabled=*/true,
Expand Down Expand Up @@ -517,6 +557,19 @@ TEST_F(PasswordManagerSettingsServiceAndroidImplTest,
PasswordManagerSetting::kOfferToSavePasswords);
}

TEST_F(PasswordManagerSettingsServiceAndroidImplTest,
OnSaveSettingAbsentSyncingBroken) {
InitializeSettingsService(/*password_sync_enabled=*/true,
/*setting_sync_enabled=*/true);
EnablePermanentAuthError();
pref_service()->SetUserPref(
password_manager::prefs::kOfferToSavePasswordsEnabledGMS,
base::Value(false));
EXPECT_CALL(*bridge(), SetPasswordSettingValue(_, _, _)).Times(0);
updater_bridge_consumer()->OnSettingValueAbsent(
PasswordManagerSetting::kOfferToSavePasswords);
}

TEST_F(PasswordManagerSettingsServiceAndroidImplTest,
OnAutoSignInAbsentDefaultSyncing) {
InitializeSettingsService(/*password_sync_enabled=*/true,
Expand Down Expand Up @@ -552,6 +605,18 @@ TEST_F(PasswordManagerSettingsServiceAndroidImplTest,
PasswordManagerSetting::kAutoSignIn);
}

TEST_F(PasswordManagerSettingsServiceAndroidImplTest,
OnAutoSignInAbsentSetValueSyncingBroken) {
InitializeSettingsService(/*password_sync_enabled=*/true,
/*setting_sync_enabled=*/true);
EnablePermanentAuthError();
pref_service()->SetUserPref(password_manager::prefs::kAutoSignInEnabledGMS,
base::Value(false));
EXPECT_CALL(*bridge(), SetPasswordSettingValue(_, _, _)).Times(0);
updater_bridge_consumer()->OnSettingValueAbsent(
PasswordManagerSetting::kAutoSignIn);
}

// Checks that general syncable prefs are dumped into the android-only GMS
// prefs before settings are requested when sync is enabled.
TEST_F(PasswordManagerSettingsServiceAndroidImplTest,
Expand All @@ -577,6 +642,62 @@ TEST_F(PasswordManagerSettingsServiceAndroidImplTest,
password_manager::prefs::kAutoSignInEnabledGMS));
}

// Checks that general syncable prefs are dumped into the android-only GMS
// prefs before settings are requested when sync is enabled.
TEST_F(PasswordManagerSettingsServiceAndroidImplTest,
PasswordSyncEnablingPrefsNotMovingWhenSyncIsbroken) {
InitializeSettingsService(/*password_sync_enabled=*/false,
/*setting_sync_enabled=*/false);
pref_service()->SetUserPref(
password_manager::prefs::kCredentialsEnableService, base::Value(false));
pref_service()->SetUserPref(
password_manager::prefs::kCredentialsEnableAutosignin,
base::Value(false));
ASSERT_TRUE(pref_service()->GetBoolean(
password_manager::prefs::kOfferToSavePasswordsEnabledGMS));
ASSERT_TRUE(pref_service()->GetBoolean(
password_manager::prefs::kAutoSignInEnabledGMS));

SetPasswordsSync(/*enabled=*/true);
EnablePermanentAuthError();
sync_service()->FireStateChanged();

EXPECT_TRUE(pref_service()->GetBoolean(
password_manager::prefs::kOfferToSavePasswordsEnabledGMS));
EXPECT_TRUE(pref_service()->GetBoolean(
password_manager::prefs::kAutoSignInEnabledGMS));
}

// Checks that general syncable prefs are dumped into the android-only GMS
// prefs and that settings are requested from GMS Core when persistent sync
// error is resolved.
TEST_F(PasswordManagerSettingsServiceAndroidImplTest,
PasswordSyncEnablingPrefsMovingOnSyncErrorResolution) {
InitializeSettingsService(/*password_sync_enabled=*/true,
/*setting_sync_enabled=*/false);
pref_service()->SetUserPref(
password_manager::prefs::kCredentialsEnableService, base::Value(false));
pref_service()->SetUserPref(
password_manager::prefs::kCredentialsEnableAutosignin,
base::Value(false));
ASSERT_TRUE(pref_service()->GetBoolean(
password_manager::prefs::kOfferToSavePasswordsEnabledGMS));
ASSERT_TRUE(pref_service()->GetBoolean(
password_manager::prefs::kAutoSignInEnabledGMS));

EnablePermanentAuthError();
sync_service()->FireStateChanged();

ResolvePermanentAuthError();
ExpectSettingsRetrievalFromBackend(/*times=*/1);
sync_service()->FireStateChanged();

EXPECT_FALSE(pref_service()->GetBoolean(
password_manager::prefs::kOfferToSavePasswordsEnabledGMS));
EXPECT_FALSE(pref_service()->GetBoolean(
password_manager::prefs::kAutoSignInEnabledGMS));
}

TEST_F(PasswordManagerSettingsServiceAndroidImplTest,
PasswordSyncEnablingGMSSettingAbsentChromeSettingDefault) {
InitializeSettingsService(/*password_sync_enabled=*/false,
Expand Down Expand Up @@ -773,6 +894,21 @@ TEST_F(PasswordManagerSettingsServiceAndroidImplTest,
PasswordManagerSetting::kOfferToSavePasswords));
}

TEST_F(PasswordManagerSettingsServiceAndroidImplTest,
SavePasswordsSettingSyncIsBroken) {
InitializeSettingsService(/*password_sync_enabled=*/true,
/*setting_sync_enabled=*/true);
EnablePermanentAuthError();

pref_service()->SetUserPref(
password_manager::prefs::kCredentialsEnableService, base::Value(true));
pref_service()->SetUserPref(
password_manager::prefs::kOfferToSavePasswordsEnabledGMS,
base::Value(false));
EXPECT_TRUE(settings_service()->IsSettingEnabled(
PasswordManagerSetting::kOfferToSavePasswords));
}

TEST_F(PasswordManagerSettingsServiceAndroidImplTest,
AutoSignInSettingNotSyncing) {
InitializeSettingsService(/*password_sync_enabled=*/false,
Expand Down Expand Up @@ -825,6 +961,20 @@ TEST_F(PasswordManagerSettingsServiceAndroidImplTest,
PasswordManagerSetting::kAutoSignIn));
}

TEST_F(PasswordManagerSettingsServiceAndroidImplTest,
AutoSignInSettingSyncIsBroken) {
InitializeSettingsService(/*password_sync_enabled=*/true,
/*setting_sync_enabled=*/true);
EnablePermanentAuthError();

pref_service()->SetUserPref(
password_manager::prefs::kCredentialsEnableAutosignin, base::Value(true));
pref_service()->SetUserPref(password_manager::prefs::kAutoSignInEnabledGMS,
base::Value(false));
EXPECT_TRUE(settings_service()->IsSettingEnabled(
PasswordManagerSetting::kAutoSignIn));
}

TEST_F(PasswordManagerSettingsServiceAndroidImplTest,
SettingsAreRequestedFromBackendWhenPasswordSyncEnabled) {
InitializeSettingsService(/*password_sync_enabled=*/true,
Expand All @@ -841,6 +991,15 @@ TEST_F(PasswordManagerSettingsServiceAndroidImplTest,
settings_service()->RequestSettingsFromBackend();
}

TEST_F(PasswordManagerSettingsServiceAndroidImplTest,
SettingsAreNotRequestedFromBackendWhenPasswordSyncBroken) {
InitializeSettingsService(/*password_sync_enabled=*/true,
/*setting_sync_enabled=*/true);
EnablePermanentAuthError();
ExpectSettingsRetrievalFromBackend(/*times=*/0);
settings_service()->RequestSettingsFromBackend();
}

TEST_F(PasswordManagerSettingsServiceAndroidImplTest,
TurnOffAutoSignInNoBackend) {
std::unique_ptr<PasswordManagerSettingsServiceAndroidImpl>
Expand Down Expand Up @@ -919,3 +1078,23 @@ TEST_F(PasswordManagerSettingsServiceAndroidImplTest,
EXPECT_FALSE(pref_service()->GetBoolean(
password_manager::prefs::kAutoSignInEnabledGMS));
}

TEST_F(PasswordManagerSettingsServiceAndroidImplTest,
TurnOffAutoSignInSyncingPasswordsBroken) {
InitializeSettingsService(/*password_sync_enabled=*/true,
/*setting_sync_enabled=*/true);
EnablePermanentAuthError();
ASSERT_TRUE(pref_service()->GetBoolean(
password_manager::prefs::kCredentialsEnableAutosignin));
ASSERT_TRUE(pref_service()->GetBoolean(
password_manager::prefs::kAutoSignInEnabledGMS));

EXPECT_CALL(*bridge(), SetPasswordSettingValue(
_, Eq(PasswordManagerSetting::kAutoSignIn), _))
.Times(0);
settings_service()->TurnOffAutoSignIn();
EXPECT_FALSE(pref_service()->GetBoolean(
password_manager::prefs::kCredentialsEnableAutosignin));
EXPECT_TRUE(pref_service()->GetBoolean(
password_manager::prefs::kAutoSignInEnabledGMS));
}
3 changes: 2 additions & 1 deletion components/password_manager/core/browser/password_store_proxy_backend.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,7 @@
#include "base/ranges/algorithm.h"
#include "base/strings/strcat.h"
#include "components/password_manager/core/browser/field_info_table.h"
#include "components/password_manager/core/browser/password_sync_util.h"
#include "components/password_manager/core/common/password_manager_features.h"
#include "components/password_manager/core/common/password_manager_pref_names.h"
#include "components/prefs/pref_service.h"
Expand Down Expand Up @@ -653,7 +654,7 @@ bool PasswordStoreProxyBackend::UsesAndroidBackendAsMainBackend() {
return false;

// Check for sync service errors if sync service is already initialized.
if (sync_service_ && sync_service_->GetAuthError().IsPersistentError())
if (sync_util::CannotUseUPMDueToPersistentSyncError(sync_service_))
return false;

if (!base::FeatureList::IsEnabled(features::kUnifiedPasswordManagerAndroid))
Expand Down

0 comments on commit 922bc51

Please sign in to comment.