Skip to content

Commit

Permalink
[Android][UPM] Forward Init callbacks from relevant backend
Browse files Browse the repository at this point in the history 
Before this change, only the built-in backend triggered the callbacks
passed to Init. Instead, the active backend should handle the callbacks.

Since the active backend changes based on the sync status, the proxy
will receive the signal of both backends.
For suspected remote changes, the proxy only invoke the callback if the
signal originated from the current main backend.
For sync changes, all signals currently originate from the built-in
backend. While there are no local passwords, this signal therefore can
always be forwarded.

Bug: 1279341
Change-Id: Iea362aa50977587d782d97d6e48d6c6f70b7729a
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/3560442
Reviewed-by: Mohamed Amir Yosef <mamir@chromium.org>
Commit-Queue: Friedrich Horschig <fhorschig@chromium.org>
Cr-Commit-Position: refs/heads/main@{#988906}
  • Loading branch information
@FHorschig
FHorschig authored and Chromium LUCI CQ committed Apr 5, 2022
1 parent 8c7940b commit 27d2ac6
Show file tree
Hide file tree
Showing 3 changed files with 173 additions and 9 deletions.
76 changes: 67 additions & 9 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 @@ -107,6 +107,22 @@ bool UsesAndroidBackendAsMainBackend(bool is_syncing) {
return false;
}

bool IsBuiltInBackendSyncEnabled() {
DCHECK(
base::FeatureList::IsEnabled(features::kUnifiedPasswordManagerAndroid));

features::UpmExperimentVariation variation =
features::kUpmExperimentVariationParam.Get();
switch (variation) {
case features::UpmExperimentVariation::kEnableForSyncingUsers:
case features::UpmExperimentVariation::kShadowSyncingUsers:
case features::UpmExperimentVariation::kEnableOnlyBackendForSyncingUsers:
return true;
}
NOTREACHED() << "Define which backend handles sync change callbacks!";
return false;
}

using MethodName = base::StrongAlias<struct MethodNameTag, std::string>;

struct LoginsResultImpl {
Expand Down Expand Up @@ -342,18 +358,32 @@ void PasswordStoreProxyBackend::InitBackend(
/*num_callbacks=*/2, base::BindOnce(&InvokeCallbackWithCombinedStatus,
std::move(completion)));

// TODO(crbug.com/1279341): Ensure that `remote_form_changes_received is
// always invoked by the current main backend. Same for
// `sync_enabled_or_disabled`.

// Both backends need to be initialized, so using the helpers for main/shadow
// backend is unnecessary and won't work since the sync status may not be
// available yet.
built_in_backend_->InitBackend(std::move(remote_form_changes_received),
std::move(sync_enabled_or_disabled_cb),
base::BindOnce(pending_initialization_calls));
android_backend_->InitBackend(base::DoNothing(), base::DoNothing(),
base::BindOnce(pending_initialization_calls));
built_in_backend_->InitBackend(
base::BindRepeating(
&PasswordStoreProxyBackend::OnRemoteFormChangesReceived,
weak_ptr_factory_.GetWeakPtr(),
CallbackOriginatesFromAndroidBackend(false),
remote_form_changes_received),
base::BindRepeating(&PasswordStoreProxyBackend::OnSyncEnabledOrDisabled,
weak_ptr_factory_.GetWeakPtr(),
CallbackOriginatesFromAndroidBackend(false),
sync_enabled_or_disabled_cb),
base::BindOnce(pending_initialization_calls));

android_backend_->InitBackend(
base::BindRepeating(
&PasswordStoreProxyBackend::OnRemoteFormChangesReceived,
weak_ptr_factory_.GetWeakPtr(),
CallbackOriginatesFromAndroidBackend(true),
std::move(remote_form_changes_received)),
base::BindRepeating(&PasswordStoreProxyBackend::OnSyncEnabledOrDisabled,
weak_ptr_factory_.GetWeakPtr(),
CallbackOriginatesFromAndroidBackend(true),
std::move(sync_enabled_or_disabled_cb)),
base::BindOnce(pending_initialization_calls));
}

void PasswordStoreProxyBackend::Shutdown(base::OnceClosure shutdown_completed) {
Expand Down Expand Up @@ -599,4 +629,32 @@ PasswordStoreBackend* PasswordStoreProxyBackend::shadow_backend() {
: android_backend_;
}

void PasswordStoreProxyBackend::OnRemoteFormChangesReceived(
CallbackOriginatesFromAndroidBackend originatesFromAndroid,
RemoteChangesReceived remote_form_changes_received,
absl::optional<PasswordStoreChangeList> changes) {
// `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 (originatesFromAndroid.value() ==
UsesAndroidBackendAsMainBackend(
sync_delegate_->IsSyncingPasswordsEnabled())) {
remote_form_changes_received.Run(std::move(changes));
}
}

void PasswordStoreProxyBackend::OnSyncEnabledOrDisabled(
CallbackOriginatesFromAndroidBackend originatesFromAndroid,
base::RepeatingClosure sync_enabled_or_disabled_cb) {
if (IsBuiltInBackendSyncEnabled()) {
if (!originatesFromAndroid) {
sync_enabled_or_disabled_cb.Run();
}
return;
}
DCHECK(UsesAndroidBackendAsMainBackend(
sync_delegate_->IsSyncingPasswordsEnabled()));
sync_enabled_or_disabled_cb.Run();
}

} // namespace password_manager
20 changes: 20 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 @@ -10,7 +10,10 @@
#include "base/callback_forward.h"
#include "base/memory/raw_ptr.h"
#include "base/memory/weak_ptr.h"
#include "base/types/strong_alias.h"
#include "components/password_manager/core/browser/password_store_backend.h"
#include "components/password_manager/core/browser/password_store_change.h"
#include "third_party/abseil-cpp/absl/types/optional.h"

class PrefService;

Expand All @@ -35,6 +38,9 @@ class PasswordStoreProxyBackend : public PasswordStoreBackend {
~PasswordStoreProxyBackend() override;

private:
using CallbackOriginatesFromAndroidBackend =
base::StrongAlias<struct CallbackOriginatesFromAndroidBackendTag, bool>;

// Implements PasswordStoreBackend interface.
void InitBackend(RemoteChangesReceived remote_form_changes_received,
base::RepeatingClosure sync_enabled_or_disabled_cb,
Expand Down Expand Up @@ -72,13 +78,27 @@ class PasswordStoreProxyBackend : public PasswordStoreBackend {
void ClearAllLocalPasswords() override;
void OnSyncServiceInitialized(syncer::SyncService* sync_service) override;

// Forwards the (possible) forms changes caused by a remote event to the
// main backend.
void OnRemoteFormChangesReceived(
CallbackOriginatesFromAndroidBackend originatesFromAndroid,
RemoteChangesReceived remote_form_changes_received,
absl::optional<PasswordStoreChangeList> changes);

// Forwards sync status changes by the backend facilitating them.
void OnSyncEnabledOrDisabled(
CallbackOriginatesFromAndroidBackend originatesFromAndroid,
base::RepeatingClosure sync_enabled_or_disabled_cb);

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_;

base::WeakPtrFactory<PasswordStoreProxyBackend> weak_ptr_factory_{this};
};

} // namespace password_manager
Expand Down
86 changes: 86 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 @@ -37,9 +37,11 @@ using ::testing::Eq;
using ::testing::Invoke;
using ::testing::Pointer;
using ::testing::Return;
using ::testing::SaveArg;
using ::testing::StrictMock;
using ::testing::WithArg;
using Type = PasswordStoreChange::Type;
using RemoveChangesReceived = PasswordStoreBackend::RemoteChangesReceived;

PasswordForm CreateTestForm() {
PasswordForm form;
Expand Down Expand Up @@ -145,6 +147,90 @@ TEST_F(PasswordStoreProxyBackendTest, CallCompletionWithFailureForAnyError) {
completion_callback.Get());
}

TEST_F(PasswordStoreProxyBackendTest, CallRemoteChangesOnlyForMainBackend) {
// Use the rollout stage which changes the main backend with the sync status.
base::test::ScopedFeatureList feature_list;
feature_list.InitAndEnableFeatureWithParameters(
features::kUnifiedPasswordManagerAndroid, {{"stage", "0"}});
base::MockCallback<RemoveChangesReceived> original_callback;

// Both backends receive a callback that they trigger for new remote changes.
RemoveChangesReceived built_in_remote_changes_callback;
EXPECT_CALL(built_in_backend(), InitBackend)
.WillOnce(SaveArg<0>(&built_in_remote_changes_callback));
RemoveChangesReceived android_remote_changes_callback;
EXPECT_CALL(android_backend(), InitBackend)
.WillOnce(SaveArg<0>(&android_remote_changes_callback));
proxy_backend().InitBackend(original_callback.Get(), base::DoNothing(),
base::DoNothing());

// With sync enabled, only the android backend calls the original callback.
EXPECT_CALL(sync_delegate(), IsSyncingPasswordsEnabled)
.WillRepeatedly(Return(true));
EXPECT_CALL(original_callback, Run);
android_remote_changes_callback.Run(absl::nullopt);
testing::Mock::VerifyAndClearExpectations(&original_callback);

EXPECT_CALL(original_callback, Run).Times(0);
built_in_remote_changes_callback.Run(absl::nullopt);
testing::Mock::VerifyAndClearExpectations(&original_callback);

// As soon as sync is disabled, only the built-in backend calls the original
// callback. The callbacks are stable. No new Init call is necessary.
testing::Mock::VerifyAndClearExpectations(&sync_delegate());
EXPECT_CALL(sync_delegate(), IsSyncingPasswordsEnabled)
.WillRepeatedly(Return(false));

EXPECT_CALL(original_callback, Run).Times(0);
android_remote_changes_callback.Run(absl::nullopt);
testing::Mock::VerifyAndClearExpectations(&original_callback);

EXPECT_CALL(original_callback, Run);
built_in_remote_changes_callback.Run(absl::nullopt);
}

TEST_F(PasswordStoreProxyBackendTest, CallSyncCallbackOnlyForBuiltInBackend) {
// Use the rollout stage which changes the main backend with the sync status.
base::test::ScopedFeatureList feature_list;
feature_list.InitAndEnableFeatureWithParameters(
features::kUnifiedPasswordManagerAndroid, {{"stage", "0"}});
base::MockCallback<base::RepeatingClosure> original_callback;

// Both backends receive a callback that they trigger for new remote changes.
base::RepeatingClosure built_in_sync_callback;
EXPECT_CALL(built_in_backend(), InitBackend)
.WillOnce(SaveArg<1>(&built_in_sync_callback));
base::RepeatingClosure android_sync_callback;
EXPECT_CALL(android_backend(), InitBackend)
.WillOnce(SaveArg<1>(&android_sync_callback));
proxy_backend().InitBackend(base::DoNothing(), original_callback.Get(),
base::DoNothing());

// With sync enabled, only the built-in backend calls the original callback.
EXPECT_CALL(sync_delegate(), IsSyncingPasswordsEnabled)
.WillRepeatedly(Return(true));
EXPECT_CALL(original_callback, Run).Times(0);
android_sync_callback.Run();
testing::Mock::VerifyAndClearExpectations(&original_callback);

EXPECT_CALL(original_callback, Run);
built_in_sync_callback.Run();
testing::Mock::VerifyAndClearExpectations(&original_callback);

// With sync is disabled, the built-in backend remains the only to call the
// original callback.
testing::Mock::VerifyAndClearExpectations(&sync_delegate());
EXPECT_CALL(sync_delegate(), IsSyncingPasswordsEnabled)
.WillRepeatedly(Return(false));

EXPECT_CALL(original_callback, Run).Times(0);
android_sync_callback.Run();
testing::Mock::VerifyAndClearExpectations(&original_callback);

EXPECT_CALL(original_callback, Run);
built_in_sync_callback.Run();
}

TEST_F(PasswordStoreProxyBackendTest, UseMainBackendToGetAllLoginsAsync) {
base::MockCallback<LoginsOrErrorReply> mock_reply;
std::vector<std::unique_ptr<PasswordForm>> expected_logins =
Expand Down

0 comments on commit 27d2ac6

Please sign in to comment.