[SmartLock] Re-show password field when leaving kAuthenticated state.
When Smart Lock is the active auth factor, do not associate just
state==kClickRequired to "hide password field"; also associate hiding
the password field to state==kAuthenticated.
Now, unconditionally set a "hide password" field on
LoginAuthFactorsView each time the state changes to kClickRequired or
kAuthenticated (or neither), and expose it via a method for
LoginAuthUserView to consume (removing the previous unnecessary
callback mechanism).
Explanation:
Bug b:214051215 occurs when Sign-in with Smart Lock seems to be
working up to the last second (state==kAuthenticated and the green
check mark is shown), but Cryptohome returns a failure shortly
afterward (state becomes kErrorForeground).
In this unique state change, the password field is not reshown,
because hiding the password field is (supposedly) only supposed to
happen upon entering or leaving state==kClickRequired. In reality,
the existing code is *actually* hiding the LoginAuthUserView password
field based on whether the new state was kClickRequired
*OR kAuthenticated*. But because this association isn't explicit,
the existing code does not consider leaving the kAuthenticated state
to be a reason to re-show the password -- the password field remains
stuck/hidden.
Fixed: b:214051215
Change-Id: Ia0423b71e582d3642e51bbaa2f2b55fa53496917
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/3405569
Reviewed-by: Thomas Tellier <tellier@google.com>
Reviewed-by: Curt Clemens <cclem@google.com>
Commit-Queue: Ryan Hansberry <hansberry@chromium.org>
Cr-Commit-Position: refs/heads/main@{#963639}
diff --git a/ash/login/ui/auth_factor_model.h b/ash/login/ui/auth_factor_model.h
index 463e212..65f62d8 100644
--- a/ash/login/ui/auth_factor_model.h
+++ b/ash/login/ui/auth_factor_model.h
@@ -40,24 +40,38 @@
// hardware that isn’t present.
kUnavailable,
// The auth factor cannot be used because of an unrecoverable
- // error, e.g. Fingerprint’s “Too many attempts”. GetLabel()
- // and UpdateIcon() show the relevant messages.
+ // error, for example:
+ // - Fingerprint’s “Too many attempts”,
+ // - Smart Lock's "Failed to sign-in",
+ // - etc.
+ // GetLabel() and UpdateIcon() show the relevant messages.
kErrorPermanent,
// The auth factor can be used but requires additional steps
- // before use, e.g. turn on Bluetooth.
+ // before use. For example, Smart Lock will be in this state if Bluetooth
+ // needs to be enabled.
kAvailable,
// The auth factor is ready to authenticate. This state should
// only be returned if authentication can be completed in one
- // step (two if a click is required).
+ // step (two if a click is required). For each auth factor, this looks like:
+ // - Fingerprint: the sensor is ready to read the user's finger(s).
+ // - Smart Lock: the phone has been found/connected, but its screen is
+ // locked and/or it's too far away.
kReady,
- // The auth factor has a non-blocking error to show the
- // user, e.g. Fingerprint’s “Not recognized”, which clears
- // after a few seconds. GetLabel() and UpdateIcon() show the
- // relevant messages.
+ // The auth factor has a non-blocking error to show the user, e.g.,
+ // Fingerprint’s “Not recognized”, which clears after a few seconds.
+ // GetLabel() and UpdateIcon() show the relevant messages.
kErrorTemporary,
- // The auth factor requires the user to tap/click to enter.
+ // The auth factor requires the user to tap/click to enter. At the moment,
+ // only Smart Lock enters this state, and it occurs when the phone is
+ // unlocked and nearby.
kClickRequired,
- // Authentication is complete.
+ // Authentication is complete. Notes for each auth factor:
+ // - Fingerprint: this is the last state; Fingerprint will not transition
+ // to any other state after this.
+ // - Smart Lock: in the lock screen, this is also Smart Lock's last state.
+ // However, at the login screen, there is a chance that Cryptohome can
+ // fail to decrypt the user directory via the phone-provided decryption
+ // key -- in which case Smart Lock can transition to kErrorPermanent.
kAuthenticated,
};
diff --git a/ash/login/ui/login_auth_factors_view.cc b/ash/login/ui/login_auth_factors_view.cc
index a7306e99..71ea148 100644
--- a/ash/login/ui/login_auth_factors_view.cc
+++ b/ash/login/ui/login_auth_factors_view.cc
@@ -166,11 +166,9 @@
}
LoginAuthFactorsView::LoginAuthFactorsView(
- base::RepeatingClosure on_click_to_enter,
- base::RepeatingCallback<void(bool)> on_click_required_changed)
- : on_click_to_enter_callback_(on_click_to_enter),
- on_click_required_changed_callback_(on_click_required_changed) {
- DCHECK(on_click_required_changed);
+ base::RepeatingClosure on_click_to_enter_callback)
+ : on_click_to_enter_callback_(on_click_to_enter_callback) {
+ DCHECK(on_click_to_enter_callback);
SetPaintToLayer();
layer()->SetFillsBoundsOpaquely(false);
@@ -258,6 +256,10 @@
UpdateState();
}
+bool LoginAuthFactorsView::ShouldHidePasswordField() {
+ return should_hide_password_field_;
+}
+
void LoginAuthFactorsView::UpdateState() {
AuthFactorModel* active_auth_factor =
GetHighestPriorityAuthFactor(auth_factors_);
@@ -278,17 +280,16 @@
error_timer_.Stop();
}
- // Fire a callback whenever entering/leaving the kClickRequired state. Avoid
- // firing the callback for the kAuthenticated state since we do not want to
- // change the visibility of the password/PIN fields when transitioning from
- // kClickRequired to kAuthenticated.
- bool should_show_arrow_button =
- state == PrioritizedAuthFactorViewState::kClickRequired;
- if (on_click_required_changed_callback_ &&
- should_show_arrow_button != arrow_button_->GetVisible() &&
- state != PrioritizedAuthFactorViewState::kAuthenticated) {
- on_click_required_changed_callback_.Run(should_show_arrow_button);
- }
+ // Update |should_hide_password_field_| when entering/leaving a state that
+ // requires hiding/showing the password/PIN fields.
+ //
+ // At the moment, Smart Lock is the only auth factor which needs to hide
+ // the password field, and it does so only during states kClickRequired and
+ // kAuthenticated.
+ should_hide_password_field_ =
+ active_auth_factor->GetType() == AuthFactorType::kSmartLock &&
+ (state == PrioritizedAuthFactorViewState::kClickRequired ||
+ state == PrioritizedAuthFactorViewState::kAuthenticated);
int ready_label_id;
size_t num_factors_in_error_background_state;
diff --git a/ash/login/ui/login_auth_factors_view.h b/ash/login/ui/login_auth_factors_view.h
index b6feafb0..acb8f44 100644
--- a/ash/login/ui/login_auth_factors_view.h
+++ b/ash/login/ui/login_auth_factors_view.h
@@ -51,9 +51,7 @@
LoginAuthFactorsView* const view_;
};
- LoginAuthFactorsView(
- base::RepeatingClosure on_click_to_enter,
- base::RepeatingCallback<void(bool)> on_click_required_changed);
+ LoginAuthFactorsView(base::RepeatingClosure on_click_to_enter_callback);
LoginAuthFactorsView(LoginAuthFactorsView&) = delete;
LoginAuthFactorsView& operator=(LoginAuthFactorsView&) = delete;
~LoginAuthFactorsView() override;
@@ -71,6 +69,10 @@
// authentication mechanism.
void SetCanUsePin(bool can_use_pin);
+ // Returns true if the active auth factor is requesting to take visual
+ // precedence, by hiding the password field.
+ bool ShouldHidePasswordField();
+
private:
// Recomputes the state and updates the label and icons. Should be called
// whenever any auth factor's state changes so that those changes can be
@@ -145,8 +147,11 @@
// multiple are visible.
std::vector<std::unique_ptr<AuthFactorModel>> auth_factors_;
+ // True if an active auth factor is requesting to hide the password field.
+ // Changes value based on the current state of the active auth factor.
+ bool should_hide_password_field_ = false;
+
base::RepeatingClosure on_click_to_enter_callback_;
- base::RepeatingCallback<void(bool)> on_click_required_changed_callback_;
base::OneShotTimer error_timer_;
};
diff --git a/ash/login/ui/login_auth_factors_view_unittest.cc b/ash/login/ui/login_auth_factors_view_unittest.cc
index 1fd88e39..9250bd3 100644
--- a/ash/login/ui/login_auth_factors_view_unittest.cc
+++ b/ash/login/ui/login_auth_factors_view_unittest.cc
@@ -136,12 +136,10 @@
container_->SetLayoutManager(std::make_unique<views::BoxLayout>(
views::BoxLayout::Orientation::kVertical));
- view_ = container_->AddChildView(std::make_unique<LoginAuthFactorsView>(
- base::BindRepeating(
+ view_ = container_->AddChildView(
+ std::make_unique<LoginAuthFactorsView>(base::BindRepeating(
&LoginAuthFactorsViewUnittest::set_click_to_enter_called,
- base::Unretained(this), true),
- base::BindRepeating(&LoginAuthFactorsViewUnittest::set_click_required,
- base::Unretained(this))));
+ base::Unretained(this), true)));
}
void TearDown() override {
@@ -169,12 +167,26 @@
return count;
}
+ bool ShouldHidePasswordField() { return view_->ShouldHidePasswordField(); }
+
void set_click_to_enter_called(bool called) {
click_to_enter_called_ = called;
}
- void set_click_required(bool click_required) {
- click_required_ = click_required;
+ void VerifyAuthenticatedUiState(
+ bool is_lock_screen,
+ LoginAuthFactorsView::TestApi& test_api,
+ bool should_hide_password_field_when_authenticated) {
+ EXPECT_TRUE(test_api.checkmark_icon()->GetVisible());
+ EXPECT_FALSE(test_api.arrow_button()->GetVisible());
+ EXPECT_FALSE(test_api.arrow_nudge_animation()->GetVisible());
+ EXPECT_FALSE(test_api.auth_factor_icon_row()->GetVisible());
+ EXPECT_EQ(l10n_util::GetStringUTF16(is_lock_screen
+ ? IDS_AUTH_FACTOR_LABEL_UNLOCKED
+ : IDS_AUTH_FACTOR_LABEL_SIGNED_IN),
+ test_api.label()->GetText());
+ EXPECT_EQ(should_hide_password_field_when_authenticated,
+ ShouldHidePasswordField());
}
base::test::ScopedFeatureList feature_list_;
@@ -182,7 +194,6 @@
LoginAuthFactorsView* view_ = nullptr; // Owned by container.
std::vector<FakeAuthFactorModel*> auth_factors_;
bool click_to_enter_called_ = false;
- bool click_required_ = false;
};
TEST_F(LoginAuthFactorsViewUnittest, NotVisibleIfNoAuthFactors) {
@@ -299,12 +310,14 @@
test_api.label()->GetText());
}
-TEST_F(LoginAuthFactorsViewUnittest, ClickRequired) {
+// Note: At the moment, Smart Lock is the only auth factor that uses state
+// kClickRequired (hence no similar test for Fingerprint).
+TEST_F(LoginAuthFactorsViewUnittest, ClickRequired_SmartLock) {
ui::ScopedAnimationDurationScaleMode non_zero_duration_mode(
ui::ScopedAnimationDurationScaleMode::NORMAL_DURATION);
AddAuthFactors({AuthFactorType::kFingerprint, AuthFactorType::kSmartLock});
- ASSERT_FALSE(click_required_);
+ ASSERT_FALSE(ShouldHidePasswordField());
LoginAuthFactorsView::TestApi test_api(view_);
auth_factors_[0]->state_ = AuthFactorState::kReady;
@@ -319,12 +332,12 @@
EXPECT_FALSE(test_api.auth_factor_icon_row()->GetVisible());
EXPECT_EQ(l10n_util::GetStringUTF16(IDS_AUTH_FACTOR_LABEL_CLICK_TO_ENTER),
test_api.label()->GetText());
- EXPECT_TRUE(click_required_);
+ EXPECT_TRUE(ShouldHidePasswordField());
auth_factors_[1]->state_ = AuthFactorState::kReady;
test_api.UpdateState();
- EXPECT_FALSE(click_required_);
+ EXPECT_FALSE(ShouldHidePasswordField());
}
TEST_F(LoginAuthFactorsViewUnittest, ClickingArrowButton) {
@@ -353,44 +366,100 @@
EXPECT_FALSE(test_api.arrow_nudge_animation()->GetVisible());
}
-TEST_F(LoginAuthFactorsViewUnittest, Authenticated_LockScreen) {
+// Present all possible auth factors on the lock screen, and verify behavior
+// when only Fingerprint is in state kAuthenticated. When Fingerprint is in
+// kAuthenticated state, it should not request to hide the password field, and
+// that is its final state as the screen becomes unlocked (it doesn't
+// transition to any further states).
+TEST_F(LoginAuthFactorsViewUnittest, Authenticated_LockScreen_Fingerprint) {
GetSessionControllerClient()->SetSessionState(
session_manager::SessionState::LOCKED);
Shell::Get()->login_screen_controller()->ShowLockScreen();
- AddAuthFactors({AuthFactorType::kFingerprint, AuthFactorType::kSmartLock});
+
+ AddAuthFactors({AuthFactorType::kSmartLock, AuthFactorType::kFingerprint});
+ ASSERT_FALSE(ShouldHidePasswordField());
+
LoginAuthFactorsView::TestApi test_api(view_);
- auth_factors_[0]->state_ = AuthFactorState::kAuthenticated;
- auth_factors_[1]->state_ = AuthFactorState::kClickRequired;
+ auth_factors_[0]->state_ = AuthFactorState::kReady;
+ auth_factors_[1]->state_ = AuthFactorState::kAuthenticated;
test_api.UpdateState();
- // Check that only the arrow button is shown and that the label has been
- // updated.
- EXPECT_TRUE(test_api.checkmark_icon()->GetVisible());
- EXPECT_FALSE(test_api.arrow_button()->GetVisible());
- EXPECT_FALSE(test_api.arrow_nudge_animation()->GetVisible());
- EXPECT_FALSE(test_api.auth_factor_icon_row()->GetVisible());
- EXPECT_EQ(l10n_util::GetStringUTF16(IDS_AUTH_FACTOR_LABEL_UNLOCKED),
- test_api.label()->GetText());
+ // Fingerprint should not request to hide the password field when
+ // authenticated.
+ VerifyAuthenticatedUiState(
+ /*is_lock_screen=*/true, test_api,
+ /*should_hide_password_field_when_authenticated=*/false);
+
+ // Fingerprint does not leave the kAuthenticated state once entering it.
}
-TEST_F(LoginAuthFactorsViewUnittest, Authenticated_LoginScreen) {
+// Present all possible auth factors on the lock screen, and verify behavior
+// when only Smart Lock is in state kAuthenticated.
+//
+// When Smart Lock is in kAuthenticated state, it should request to hide the
+// password field.
+//
+// On the lock screen, kAuthenticated is its final state as the screen becomes
+// unlocked (it doesn't transition to any further states).
+TEST_F(LoginAuthFactorsViewUnittest, Authenticated_LockScreen_SmartLock) {
+ GetSessionControllerClient()->SetSessionState(
+ session_manager::SessionState::LOCKED);
+ Shell::Get()->login_screen_controller()->ShowLockScreen();
+
+ AddAuthFactors({AuthFactorType::kFingerprint, AuthFactorType::kSmartLock});
+ ASSERT_FALSE(ShouldHidePasswordField());
+
+ LoginAuthFactorsView::TestApi test_api(view_);
+ auth_factors_[0]->state_ = AuthFactorState::kReady;
+ auth_factors_[1]->state_ = AuthFactorState::kAuthenticated;
+ test_api.UpdateState();
+
+ // Smart Lock is authenticated -- it should request to hide the password
+ // field.
+ VerifyAuthenticatedUiState(
+ /*is_lock_screen=*/true, test_api,
+ /*should_hide_password_field_when_authenticated=*/true);
+
+ // On the lock screen, Smart Lock does not leave the kAuthenticated state
+ // once entering it.
+}
+
+// Present all possible auth factors on the login screen, and verify behavior
+// when Smart Lock is in state kAuthenticated.
+//
+// At the moment, Smart Lock is the only auth factor which can be present on the
+// login screen.
+//
+// When Smart Lock is in kAuthenticated state, it should request to hide the
+// password field.
+//
+// On the login screen, Smart Lock may transition the kErrorPermanent
+// state (if Cryptohome fails to decrypt the user directory with the
+// phone-provided decryption key). When Smart Lock transitions out of
+// kAuthenticated, it should no longer request to hide the password field.
+TEST_F(LoginAuthFactorsViewUnittest, Authenticated_LoginScreen_SmartLock) {
GetSessionControllerClient()->SetSessionState(
session_manager::SessionState::LOGIN_PRIMARY);
Shell::Get()->login_screen_controller()->ShowLoginScreen();
- AddAuthFactors({AuthFactorType::kFingerprint, AuthFactorType::kSmartLock});
+
+ AddAuthFactors({AuthFactorType::kSmartLock});
+ ASSERT_FALSE(ShouldHidePasswordField());
+
LoginAuthFactorsView::TestApi test_api(view_);
auth_factors_[0]->state_ = AuthFactorState::kAuthenticated;
- auth_factors_[1]->state_ = AuthFactorState::kClickRequired;
test_api.UpdateState();
- // Check that only the arrow button is shown and that the label has been
- // updated.
- EXPECT_TRUE(test_api.checkmark_icon()->GetVisible());
- EXPECT_FALSE(test_api.arrow_button()->GetVisible());
- EXPECT_FALSE(test_api.arrow_nudge_animation()->GetVisible());
- EXPECT_FALSE(test_api.auth_factor_icon_row()->GetVisible());
- EXPECT_EQ(l10n_util::GetStringUTF16(IDS_AUTH_FACTOR_LABEL_SIGNED_IN),
- test_api.label()->GetText());
+ // Smart Lock is authenticated -- it should request to hide the password
+ // field.
+ VerifyAuthenticatedUiState(
+ /*is_lock_screen=*/false, test_api,
+ /*should_hide_password_field_when_authenticated=*/true);
+
+ // Simulate Cryptohome failure. Smart Lock should no longer request to hide
+ // the password field.
+ auth_factors_[0]->state_ = AuthFactorState::kErrorPermanent;
+ test_api.UpdateState();
+ EXPECT_FALSE(ShouldHidePasswordField());
}
TEST_F(LoginAuthFactorsViewUnittest, ErrorTemporary) {
diff --git a/ash/login/ui/login_auth_user_view.cc b/ash/login/ui/login_auth_user_view.cc
index 668e3ba1..0edc25b3 100644
--- a/ash/login/ui/login_auth_user_view.cc
+++ b/ash/login/ui/login_auth_user_view.cc
@@ -140,8 +140,8 @@
constexpr int kLockedTpmMessageDeltaDp = 0;
constexpr int kLockedTpmMessageRoundedCornerRadiusDp = 8;
-constexpr int kAuthFactorClickRequiredSlideUpDistanceDp = 42;
-constexpr base::TimeDelta kAuthFactorClickRequiredSlideUpDuration =
+constexpr int kAuthFactorHidingPasswordFieldSlideUpDistanceDp = 42;
+constexpr base::TimeDelta kAuthFactorHidingPasswordFieldSlideUpDuration =
base::Milliseconds(600);
constexpr int kNonEmptyWidthDp = 1;
@@ -1139,13 +1139,10 @@
std::make_unique<SmartLockAuthFactorModel>(base::BindRepeating(
&LoginAuthUserView::OnUserViewTap, base::Unretained(this)));
smart_lock_auth_factor_model_ = smart_lock_auth_factor_model.get();
- auth_factors_view = std::make_unique<LoginAuthFactorsView>(
- base::BindRepeating(
+ auth_factors_view =
+ std::make_unique<LoginAuthFactorsView>(base::BindRepeating(
&SmartLockAuthFactorModel::OnArrowButtonTapOrClickEvent,
- base::Unretained(smart_lock_auth_factor_model_)),
- base::BindRepeating(
- &LoginAuthUserView::OnAuthFactorClickRequiredChanged,
- base::Unretained(this)));
+ base::Unretained(smart_lock_auth_factor_model_)));
auth_factors_view_ = auth_factors_view.get();
auth_factors_view_->AddAuthFactor(std::move(fingerprint_auth_factor_model));
auth_factors_view_->AddAuthFactor(std::move(smart_lock_auth_factor_model));
@@ -1569,28 +1566,32 @@
}
////////
- // Slide the auth factors view up/down when entering/leaving the click
- // required state.
+ // Slide the auth factors view up/down when entering/leaving a state of
+ // |auth_factors_view_| that requests the password field to be hidden.
if (smart_lock_ui_revamp_enabled_) {
+ bool should_hide_password_field =
+ auth_factors_view_->ShouldHidePasswordField();
+
{
CHECK(auth_factors_view_);
ui::ScopedLayerAnimationSettings settings(
auth_factors_view_->layer()->GetAnimator());
- settings.SetTransitionDuration(kAuthFactorClickRequiredSlideUpDuration);
+ settings.SetTransitionDuration(
+ kAuthFactorHidingPasswordFieldSlideUpDuration);
settings.SetTweenType(gfx::Tween::Type::ACCEL_20_DECEL_100);
gfx::Transform transform;
transform.Translate(/*x=*/0,
- /*y=*/auth_factor_has_click_required_
- ? -kAuthFactorClickRequiredSlideUpDistanceDp
+ /*y=*/should_hide_password_field
+ ? -kAuthFactorHidingPasswordFieldSlideUpDistanceDp
: 0);
auth_factors_view_->layer()->GetAnimator()->SetTransform(transform);
}
- // Translate the user view to its previous position when in the click
- // required state. This prevents the user view from moving when the password
- // view collapses.
- if (auth_factor_has_click_required_) {
+ // Translate the user view to its previous position when in the auth factor
+ // view requests to hide the password field. This prevents the user view
+ // from moving when the password view collapses.
+ if (should_hide_password_field) {
layer()->GetAnimator()->StopAnimating();
int non_pin_y_end_in_screen = GetBoundsInScreen().y();
gfx::Transform transform;
@@ -1823,13 +1824,6 @@
pin_view_->OnPasswordTextChanged(is_empty);
}
-void LoginAuthUserView::OnAuthFactorClickRequiredChanged(bool click_required) {
- if (click_required == auth_factor_has_click_required_) {
- return;
- }
- auth_factor_has_click_required_ = click_required;
-}
-
bool LoginAuthUserView::HasAuthMethod(AuthMethods auth_method) const {
return (auth_methods_ & auth_method) != 0;
}
@@ -1906,16 +1900,18 @@
ApplyAnimationPostLayout(/*animate*/ true);
}
+// TODO(b/213926876): Write a unit test for ShouldHidePasswordField() usage.
void LoginAuthUserView::UpdateInputFieldMode() {
// There isn't an input field when any of the following is true:
// - Challenge response is active (Smart Card)
// - Online sign in message shown
// - Disabled message shown
// - No password auth available
- // - Auth factors view is showing "click to enter"
+ // - Auth factors view is requesting to hide the password/PIN field
if (HasAuthMethod(AUTH_CHALLENGE_RESPONSE) ||
HasAuthMethod(AUTH_ONLINE_SIGN_IN) || HasAuthMethod(AUTH_DISABLED) ||
- !HasAuthMethod(AUTH_PASSWORD) || auth_factor_has_click_required_) {
+ !HasAuthMethod(AUTH_PASSWORD) ||
+ (auth_factors_view_ && auth_factors_view_->ShouldHidePasswordField())) {
input_field_mode_ = InputFieldMode::NONE;
return;
}
diff --git a/ash/login/ui/login_auth_user_view.h b/ash/login/ui/login_auth_user_view.h
index 2e491247..52e428e 100644
--- a/ash/login/ui/login_auth_user_view.h
+++ b/ash/login/ui/login_auth_user_view.h
@@ -231,11 +231,6 @@
void OnPasswordTextChanged(bool is_empty);
void OnPinTextChanged(bool is_empty);
- // Called when the |auth_factors_view_| enters or leaves the "click to enter"
- // state, which indicates that the user has authenticated with an auth factor
- // but is required to click a button to complete login/unlock.
- void OnAuthFactorClickRequiredChanged(bool click_required);
-
// Helper method to check if an auth method is enable. Use it like this:
// bool has_tap = HasAuthMethod(AUTH_TAP).
bool HasAuthMethod(AuthMethods auth_method) const;
@@ -322,8 +317,6 @@
// `ApplyAnimationPostLayout`.
std::unique_ptr<UiState> previous_state_;
- bool auth_factor_has_click_required_ = false;
-
base::WeakPtrFactory<LoginAuthUserView> weak_factory_{this};
};