Commit 6def1de7 authored by Ioana Pandele's avatar Ioana Pandele Committed by Commit Bot

[PwdCheckAndroid] Ensure known compromised credentials are loaded before running a check

This CL makes sure that we don't start a check before we can be sure
that all preconditions are fulfilled:
* saved passwords are loaded
* first set of compromised credentials is displayed
* scripts are fetched (if necessary)

To prevent a third boolean value, this CL stores all applicable
preconditions as flags.

Bug: 1119366, 1092444
Change-Id: I81bf4577185709bd14b99cc0e878c5d38fbc4ebb
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2366759
Commit-Queue: Ioana Pandele <ioanap@chromium.org>
Reviewed-by: default avatarIoana Pandele <ioanap@chromium.org>
Cr-Commit-Position: refs/heads/master@{#800244}
parent 7d2d8cc1
......@@ -82,12 +82,16 @@ PasswordCheckManager::PasswordCheckManager(Profile* profile, Observer* observer)
// empty list.
saved_passwords_presenter_.Init();
compromised_credentials_manager_.Init();
if (!ShouldOfferAutomaticPasswordChange()) {
// Ensure that scripts are treated as initialized if they are unnecessary.
FulfillPrecondition(kScriptsCachePrewarmed);
}
}
PasswordCheckManager::~PasswordCheckManager() = default;
void PasswordCheckManager::StartCheck() {
if (!is_initialized_ || !AreScriptsRefreshed()) {
if (!IsPreconditionFulfilled(kAll)) {
was_start_requested_ = true;
return;
}
......@@ -164,9 +168,9 @@ void PasswordCheckManager::PasswordCheckProgress::OnProcessed(
void PasswordCheckManager::OnSavedPasswordsChanged(
password_manager::SavedPasswordsPresenter::SavedPasswordsView passwords) {
if (!is_initialized_) {
if (!IsPreconditionFulfilled(kSavedPasswordsAvailable)) {
observer_->OnSavedPasswordsFetched(passwords.size());
is_initialized_ = true;
FulfillPrecondition(kSavedPasswordsAvailable);
}
if (passwords.empty()) {
......@@ -184,7 +188,9 @@ void PasswordCheckManager::OnSavedPasswordsChanged(
void PasswordCheckManager::OnCompromisedCredentialsChanged(
password_manager::CompromisedCredentialsManager::CredentialsView
credentials) {
if (!AreScriptsRefreshed()) {
if (AreScriptsRefreshed()) {
FulfillPrecondition(kKnownCredentialsFetched);
} else {
credentials_count_to_notify_ = credentials.size();
}
observer_->OnCompromisedCredentialsChanged(credentials.size());
......@@ -301,22 +307,22 @@ bool PasswordCheckManager::CanUseAccountCheck() const {
}
bool PasswordCheckManager::AreScriptsRefreshed() const {
return are_scripts_refreshed_ || !ShouldOfferAutomaticPasswordChange();
return IsPreconditionFulfilled(kScriptsCachePrewarmed);
}
void PasswordCheckManager::RefreshScripts() {
if (!ShouldOfferAutomaticPasswordChange()) {
FulfillPrecondition(kScriptsCachePrewarmed);
return;
}
are_scripts_refreshed_ = false;
ResetPrecondition(kScriptsCachePrewarmed);
password_script_fetcher_->RefreshScriptsIfNecessary(base::BindOnce(
&PasswordCheckManager::OnScriptsFetched, base::Unretained(this)));
}
void PasswordCheckManager::OnScriptsFetched() {
are_scripts_refreshed_ = true;
FulfillPrecondition(kScriptsCachePrewarmed);
if (credentials_count_to_notify_.has_value()) {
// Inform the UI about compromised credentials another time because it was
// not allowed to generate UI before the availability of password scripts is
......@@ -343,3 +349,18 @@ bool PasswordCheckManager::ShouldOfferAutomaticPasswordChange() const {
return base::FeatureList::IsEnabled(
password_manager::features::kPasswordChangeInSettings);
}
bool PasswordCheckManager::IsPreconditionFulfilled(
CheckPreconditions condition) const {
return (fulfilled_preconditions_ & condition) == condition;
}
void PasswordCheckManager::FulfillPrecondition(CheckPreconditions condition) {
fulfilled_preconditions_ |= condition;
if (was_start_requested_)
StartCheck();
}
void PasswordCheckManager::ResetPrecondition(CheckPreconditions condition) {
fulfilled_preconditions_ &= !condition;
}
......@@ -100,6 +100,21 @@ class PasswordCheckManager
PasswordCheckManager& operator=(PasswordCheckManager&&) = delete;
private:
// Helps to track which preconditions are fulfilled.
enum CheckPreconditions {
// No preconditions have been fulfilled.
kNone = 0,
// Saved passwords can be accessed.
kSavedPasswordsAvailable = 1 << 0,
// Sites with available Scripts are fetched.
kScriptsCachePrewarmed = 1 << 1,
// Already known compromised credentials were loaded.
kKnownCredentialsFetched = 1 << 2,
// All preconditions have been fulfilled.
kAll = kSavedPasswordsAvailable | kScriptsCachePrewarmed |
kKnownCredentialsFetched,
};
// Class remembering the state required to update the progress of an ongoing
// Password Check.
class PasswordCheckProgress {
......@@ -175,6 +190,15 @@ class PasswordCheckManager
// Callback when PasswordScriptsFetcher's cache has been warmed up.
void OnScriptsFetched();
// Returns true if the passed |condition| was already met.
bool IsPreconditionFulfilled(CheckPreconditions condition) const;
// Marks the passed |condition| as fulfilled and runs a check if applicable.
void FulfillPrecondition(CheckPreconditions condition);
// Resets the passed |condition| so that it's expected to happen again.
void ResetPrecondition(CheckPreconditions condition);
// Obsever being notified of UI-relevant events.
// It must outlive `this`.
Observer* observer_ = nullptr;
......@@ -214,8 +238,8 @@ class PasswordCheckManager
BulkLeakCheckServiceFactory::GetForProfile(profile_),
profile_->GetPrefs()};
// This is true when the saved passwords have been fetched from the store.
bool is_initialized_ = false;
// The check can be run only of this is CheckPreconditions::kAll;
int fulfilled_preconditions_ = CheckPreconditions::kNone;
// Whether the check start was requested.
bool was_start_requested_ = false;
......@@ -223,9 +247,6 @@ class PasswordCheckManager
// Whether a check is currently running.
bool is_check_running_ = false;
// Whether scripts refreshement is finished.
bool are_scripts_refreshed_ = false;
// Latest number of changed compromised credentials while script fetching
// was running. If `credentials_count_to_notify_` has value, after scripts are
// fetched `onCompromisedCredentials` should be called.
......
......@@ -44,6 +44,7 @@ using password_manager::PasswordCheckUIStatus;
using password_manager::TestPasswordStore;
using password_manager::prefs::kLastTimePasswordCheckCompleted;
using testing::_;
using testing::AtLeast;
using testing::ElementsAre;
using testing::Field;
using testing::Invoke;
......@@ -276,6 +277,28 @@ TEST_F(PasswordCheckManagerTest, OnCompromisedCredentialsChanged) {
RunUntilIdle();
}
TEST_F(PasswordCheckManagerTest, RunCheckAfterLastInitialization) {
EXPECT_CALL(mock_observer(), OnPasswordCheckStatusChanged(_))
.Times(AtLeast(1));
EXPECT_CALL(mock_observer(), OnSavedPasswordsFetched(1));
store().AddLogin(MakeSavedPassword(kExampleCom, kUsername1));
InitializeManager();
// Initialization is incomplete, so check shouldn't run.
manager().StartCheck(); // Try to start a check — has no immediate effect.
service()->set_state_and_notify(State::kIdle);
// Since check hasn't started, the last completion time should remain 0.
EXPECT_EQ(0.0, manager().GetLastCheckTimestamp().ToDoubleT());
// Complete pending initialization. The check should run now.
EXPECT_CALL(mock_observer(), OnCompromisedCredentialsChanged(0))
.Times(AtLeast(1));
RunUntilIdle();
service()->set_state_and_notify(State::kIdle); // Complete check, if any.
// Check should have started and the last completion time be non-zero.
EXPECT_NE(0.0, manager().GetLastCheckTimestamp().ToDoubleT());
}
TEST_F(PasswordCheckManagerTest, CorrectlyCreatesUIStructForSiteCredential) {
InitializeManager();
store().AddLogin(MakeSavedPassword(kExampleCom, kUsername1));
......
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