Skip to content

Commit

Permalink
[UPM] Do not use AndroidBackend when persistent auth errors exist.
Browse files Browse the repository at this point in the history 
This CL ensures that the android password store backend is not used as
active backend when password sync is broken because of auth errors.
This is needed to avoid silent failures on saving passwords in GMS Core.

(cherry picked from commit a45e933)

Bug: 1327307
Change-Id: I08ea4d57820c2a14dd1f419aecb5e5236720f98c
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/3654170
Reviewed-by: Vasilii Sukhanov <vasilii@chromium.org>
Commit-Queue: Maria Kazinova <kazinova@google.com>
Cr-Original-Commit-Position: refs/heads/main@{#1005695}
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/3657496
Auto-Submit: Maria Kazinova <kazinova@google.com>
Commit-Queue: Vasilii Sukhanov <vasilii@chromium.org>
Cr-Commit-Position: refs/branch-heads/5060@{#171}
Cr-Branched-From: b83393d-refs/heads/main@{#1002911}
  • Loading branch information
Maria Kazinova authored and Chromium LUCI CQ committed May 23, 2022
1 parent 6e52b8e commit dc2c86c
Show file tree
Hide file tree
Showing 3 changed files with 113 additions and 35 deletions.
68 changes: 33 additions & 35 deletions components/password_manager/core/browser/password_store_proxy_backend.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -20,7 +20,9 @@
#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"
#include "components/sync/driver/sync_service.h"
#include "components/sync/model/proxy_model_type_controller_delegate.h"
#include "google_apis/gaia/google_service_auth_error.h"

namespace password_manager {

Expand Down Expand Up @@ -89,30 +91,6 @@ bool ShouldExecuteDeletionsOnShadowBackend(PrefService* prefs,
return false;
}

// This helper is used to determine main *and* shadow backends. Technically,
// some "Enable" groups don't require shadow traffic but they use it for safe
// deletions.
bool UsesAndroidBackendAsMainBackend(bool is_syncing) {
if (!is_syncing)
return false;

if (!base::FeatureList::IsEnabled(features::kUnifiedPasswordManagerAndroid))
return false;

features::UpmExperimentVariation variation =
features::kUpmExperimentVariationParam.Get();
switch (variation) {
case features::UpmExperimentVariation::kEnableForSyncingUsers:
case features::UpmExperimentVariation::kEnableOnlyBackendForSyncingUsers:
case features::UpmExperimentVariation::kEnableForAllUsers:
return true;
case features::UpmExperimentVariation::kShadowSyncingUsers:
return false;
}
NOTREACHED() << "Define explicitly whether Android is the main backend!";
return false;
}

bool IsBuiltInBackendSyncEnabled() {
DCHECK(
base::FeatureList::IsEnabled(features::kUnifiedPasswordManagerAndroid));
Expand Down Expand Up @@ -644,21 +622,18 @@ void PasswordStoreProxyBackend::ClearAllLocalPasswords() {

void PasswordStoreProxyBackend::OnSyncServiceInitialized(
syncer::SyncService* sync_service) {
sync_service_ = sync_service;
android_backend_->OnSyncServiceInitialized(sync_service);
}

PasswordStoreBackend* PasswordStoreProxyBackend::main_backend() {
return UsesAndroidBackendAsMainBackend(
sync_delegate_->IsSyncingPasswordsEnabled())
? android_backend_
: built_in_backend_;
return UsesAndroidBackendAsMainBackend() ? android_backend_
: built_in_backend_;
}

PasswordStoreBackend* PasswordStoreProxyBackend::shadow_backend() {
return UsesAndroidBackendAsMainBackend(
sync_delegate_->IsSyncingPasswordsEnabled())
? built_in_backend_
: android_backend_;
return UsesAndroidBackendAsMainBackend() ? built_in_backend_
: android_backend_;
}

void PasswordStoreProxyBackend::OnRemoteFormChangesReceived(
Expand All @@ -668,11 +643,34 @@ void PasswordStoreProxyBackend::OnRemoteFormChangesReceived(
// `remote_form_changes_received` is used to inform observers about changes in
// the backend. This check guarantees observers are informed only about
// changes in the main backend.
if (originates_from_android.value() ==
UsesAndroidBackendAsMainBackend(
sync_delegate_->IsSyncingPasswordsEnabled())) {
if (originates_from_android.value() == UsesAndroidBackendAsMainBackend()) {
remote_form_changes_received.Run(std::move(changes));
}
}

bool PasswordStoreProxyBackend::UsesAndroidBackendAsMainBackend() {
if (!sync_delegate_->IsSyncingPasswordsEnabled())
return false;

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

if (!base::FeatureList::IsEnabled(features::kUnifiedPasswordManagerAndroid))
return false;

features::UpmExperimentVariation variation =
features::kUpmExperimentVariationParam.Get();
switch (variation) {
case features::UpmExperimentVariation::kEnableForSyncingUsers:
case features::UpmExperimentVariation::kEnableOnlyBackendForSyncingUsers:
case features::UpmExperimentVariation::kEnableForAllUsers:
return true;
case features::UpmExperimentVariation::kShadowSyncingUsers:
return false;
}
NOTREACHED() << "Define explicitly whether Android is the main backend!";
return false;
}

} // namespace password_manager
7 changes: 7 additions & 0 deletions components/password_manager/core/browser/password_store_proxy_backend.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -92,13 +92,20 @@ class PasswordStoreProxyBackend : public PasswordStoreBackend {
CallbackOriginatesFromAndroidBackend originatesFromAndroid,
base::RepeatingClosure sync_enabled_or_disabled_cb);

// Helper used to determine main *and* shadow backends. Some UPM experiment
// groups use shadow traffic to compare the two backends, other may need it
// to execute login deletions on both backends, to avoid recovery of deleted
// data.
bool UsesAndroidBackendAsMainBackend();

PasswordStoreBackend* main_backend();
PasswordStoreBackend* shadow_backend();

const raw_ptr<PasswordStoreBackend> built_in_backend_;
const raw_ptr<PasswordStoreBackend> android_backend_;
raw_ptr<PrefService> const prefs_ = nullptr;
const raw_ptr<SyncDelegate> sync_delegate_;
raw_ptr<syncer::SyncService> sync_service_ = nullptr;

base::WeakPtrFactory<PasswordStoreProxyBackend> weak_ptr_factory_{this};
};
Expand Down
73 changes: 73 additions & 0 deletions components/password_manager/core/browser/password_store_proxy_backend_unittest.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -706,6 +706,79 @@ TEST_F(PasswordStoreProxyBackendTest,
proxy_backend().OnSyncServiceInitialized(&sync_service);
}

TEST_F(PasswordStoreProxyBackendTest,
UsesAndroidBackendAsMainBackendPasswordSyncDisabledInSettings) {
base::test::ScopedFeatureList feature_list;
// Enable UPM for syncing users only.
feature_list.InitAndEnableFeatureWithParameters(
features::kUnifiedPasswordManagerAndroid, {{"stage", "2"}});

// Imitate password sync being disabled in settings.
EXPECT_CALL(sync_delegate(), IsSyncingPasswordsEnabled)
.WillRepeatedly(Return(false));

// Initialize sync service.
syncer::TestSyncService sync_service;
EXPECT_CALL(android_backend(), OnSyncServiceInitialized(&sync_service));
proxy_backend().OnSyncServiceInitialized(&sync_service);

// Verify that android backend is not used.
EXPECT_CALL(android_backend(), GetAllLoginsAsync).Times(0);
EXPECT_CALL(built_in_backend(), GetAllLoginsAsync);
proxy_backend().GetAllLoginsAsync(base::DoNothing());
}

TEST_F(PasswordStoreProxyBackendTest,
UsesAndroidBackendAsMainBackendSyncPersistentAuthError) {
base::test::ScopedFeatureList feature_list;
// Enable UPM for syncing users only.
feature_list.InitAndEnableFeatureWithParameters(
features::kUnifiedPasswordManagerAndroid, {{"stage", "2"}});

EXPECT_CALL(sync_delegate(), IsSyncingPasswordsEnabled)
.WillRepeatedly(Return(true));

// Initialize sync service.
syncer::TestSyncService sync_service;
GoogleServiceAuthError persistent_error(
GoogleServiceAuthError::INVALID_GAIA_CREDENTIALS);
ASSERT_TRUE(persistent_error.IsPersistentError());
sync_service.SetAuthError(persistent_error);
EXPECT_CALL(android_backend(), OnSyncServiceInitialized(&sync_service));
proxy_backend().OnSyncServiceInitialized(&sync_service);

// Verify that android backend is not used.
EXPECT_CALL(android_backend(), GetAllLoginsAsync).Times(0);
EXPECT_CALL(built_in_backend(), GetAllLoginsAsync);
proxy_backend().GetAllLoginsAsync(base::DoNothing());
}

TEST_F(PasswordStoreProxyBackendTest,
UsesAndroidBackendAsMainBackendSyncTransientAuthError) {
base::test::ScopedFeatureList feature_list;
// Enable UPM for syncing users only.
feature_list.InitAndEnableFeatureWithParameters(
features::kUnifiedPasswordManagerAndroid, {{"stage", "2"}});

EXPECT_CALL(sync_delegate(), IsSyncingPasswordsEnabled)
.WillRepeatedly(Return(true));

// Initialize sync service.
syncer::TestSyncService sync_service;
GoogleServiceAuthError transient_error(
GoogleServiceAuthError::CONNECTION_FAILED);
ASSERT_TRUE(transient_error.IsTransientError());
sync_service.SetAuthError(transient_error);
EXPECT_CALL(android_backend(), OnSyncServiceInitialized(&sync_service));
proxy_backend().OnSyncServiceInitialized(&sync_service);

// Transient errors should not stop users from accessing android backend, as
// they are likely to succeed on retries.
EXPECT_CALL(android_backend(), GetAllLoginsAsync);
EXPECT_CALL(built_in_backend(), GetAllLoginsAsync).Times(0);
proxy_backend().GetAllLoginsAsync(base::DoNothing());
}

// Holds the main and shadow backend's logins and the expected number of common
// and different logins.
struct LoginsMetricsParam {
Expand Down

0 comments on commit dc2c86c

Please sign in to comment.