Improve display password feature security and restrict to Gmail accounts

Bug: 993653
Change-Id: Icf3cdb322223487a6780a5f20a3c22f9ed22ca5e
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2133972
Commit-Queue: Thomas Tellier <tellier@google.com>
Reviewed-by: Roman Sorokin [CET] <rsorokin@chromium.org>
Reviewed-by: Denis Kuznetsov [CET] <antrim@chromium.org>
Cr-Commit-Position: refs/heads/master@{#757518}

ash/login/ui/login_auth_user_view.cc

@@ -119,6 +119,19 @@ constexpr int kDisabledAuthMessageRoundedCornerRadiusDp = 8;
 
 constexpr int kNonEmptyWidthDp = 1;
 
+// TODO(tellier): This should be removed in M84. See crbug.com/1062524
+const char kGmailDomain[] = "gmail.com";
+const char kGooglemailDomain[] = "googlemail.com";
+
+bool IsNotEnterpriseManagedEmail(const std::string& email) {
+  size_t separator_pos = email.find('@');
+  if (separator_pos != email.npos && separator_pos < email.length() - 1) {
+    std::string domain = base::ToLowerASCII(email.substr(separator_pos + 1));
+    return domain == kGmailDomain || domain == kGooglemailDomain;
+  }
+  return false;
+}
+
 // Returns an observer that will hide |view| when it fires. The observer will
 // delete itself after firing (by returning true). Make sure to call
 // |observer->SetActive()| after attaching it.
@@ -181,7 +194,7 @@ class ClearPasswordAndHideAnimationObserver
 
   // ui::ImplicitAnimationObserver:
   void OnImplicitAnimationsCompleted() override {
-    password_view_->Clear();
+    password_view_->Reset();
     password_view_->SetVisible(false);
     delete this;
   }
@@ -799,6 +812,8 @@ LoginAuthUserView::LoginAuthUserView(const LoginUserInfo& user,
   password_view_ = password_view.get();
   password_view->SetPaintToLayer();  // Needed for opacity animation.
   password_view->layer()->SetFillsBoundsOpaquely(false);
+  password_view_->SetDisplayPasswordButtonVisible(
+      IsNotEnterpriseManagedEmail(user.basic_user_info.display_email));
 
   auto pin_view = std::make_unique<LoginPinView>(
       LoginPinView::Style::kAlphanumeric,
@@ -1206,8 +1221,11 @@ void LoginAuthUserView::UpdateForUser(const LoginUserInfo& user) {
   const bool user_changed = current_user().basic_user_info.account_id !=
                             user.basic_user_info.account_id;
   user_view_->UpdateForUser(user, true /*animate*/);
-  if (user_changed)
-    password_view_->Clear();
+  if (user_changed) {
+    password_view_->Reset();
+    password_view_->SetDisplayPasswordButtonVisible(
+        IsNotEnterpriseManagedEmail(user.basic_user_info.display_email));
+  }
   online_sign_in_message_->SetText(
       l10n_util::GetStringUTF16(IDS_ASH_LOGIN_SIGN_IN_REQUIRED_MESSAGE));
 }
@@ -1291,7 +1309,7 @@ void LoginAuthUserView::OnAuthComplete(base::Optional<bool> auth_success) {
   // animating the next lock screen will not work as expected. See
   // https://crbug.com/808486.
   if (!auth_success.has_value() || !auth_success.value()) {
-    password_view_->Clear();
+    password_view_->Reset();
     password_view_->SetReadOnly(false);
     external_binary_auth_button_->SetEnabled(true);
     external_binary_enrollment_button_->SetEnabled(true);
@@ -1303,7 +1321,7 @@ void LoginAuthUserView::OnAuthComplete(base::Optional<bool> auth_success) {
 void LoginAuthUserView::OnChallengeResponseAuthComplete(
     base::Optional<bool> auth_success) {
   if (!auth_success.has_value() || !auth_success.value()) {
-    password_view_->Clear();
+    password_view_->Reset();
     password_view_->SetReadOnly(false);
     // If the user canceled the PIN request during ChallengeResponse,
     // ChallengeResponse will fail with an unknown error. Since this is

ash/login/ui/login_auth_user_view_unittest.cc

@@ -228,6 +228,14 @@ TEST_F(LoginAuthUserViewUnittest, PasswordFieldChangeOnUpdateUser) {
   auto another_user = CreateUser("user2@domain.com");
   view_->UpdateForUser(another_user);
   EXPECT_TRUE(password_test.textfield()->GetText().empty());
+  password_test.textfield()->SetTextInputType(ui::TEXT_INPUT_TYPE_TEXT);
+  EXPECT_EQ(password_test.textfield()->GetTextInputType(),
+            ui::TEXT_INPUT_TYPE_TEXT);
+
+  // Updating user should make the textfield as a password again.
+  view_->UpdateForUser(user_);
+  EXPECT_EQ(password_test.textfield()->GetTextInputType(),
+            ui::TEXT_INPUT_TYPE_PASSWORD);
 }
 
 }  // namespace ash

ash/login/ui/login_password_view.cc

@@ -4,6 +4,7 @@
 
 #include "ash/login/ui/login_password_view.h"
 
+#include "ash/accessibility/accessibility_controller_impl.h"
 #include "ash/login/ui/horizontal_image_sequence_animation_decoder.h"
 #include "ash/login/ui/hover_notifier.h"
 #include "ash/login/ui/lock_screen.h"
@@ -79,6 +80,15 @@ constexpr const int kHorizontalDistanceBetweenEasyUnlockAndPasswordDp = 12;
 // Non-empty height, useful for debugging/visualization.
 constexpr const int kNonEmptyHeight = 1;
 
+// Clears the password after some time if no action has been done and the
+// display password feature is enabled, for security reasons.
+constexpr base::TimeDelta kClearPasswordAfterDelay =
+    base::TimeDelta::FromSeconds(30);
+
+// Hides the password after a short delay for security reasons.
+constexpr base::TimeDelta kHidePasswordAfterDelay =
+    base::TimeDelta::FromSeconds(3);
+
 constexpr const char kLoginPasswordViewName[] = "LoginPasswordView";
 
 class NonAccessibleSeparator : public views::Separator {
@@ -201,6 +211,12 @@ class LoginPasswordView::LoginTextfield : public views::Textfield {
       SetTextInputType(ui::TEXT_INPUT_TYPE_TEXT);
   }
 
+  // This is useful when the display password button is not shown. In such a
+  // case, the login text field needs to define its size.
+  gfx::Size CalculatePreferredSize() const override {
+    return gfx::Size(kPasswordTotalWidthDp, kDisplayPasswordButtonSizeDp);
+  }
+
  private:
   // Closures that will be called when the element receives and loses focus.
   base::RepeatingClosure on_focus_closure_;
@@ -412,7 +428,18 @@ void LoginPasswordView::TestApi::set_immediately_hover_easy_unlock_icon() {
   view_->easy_unlock_icon_->set_immediately_hover_for_test();
 }
 
-LoginPasswordView::LoginPasswordView() {
+void LoginPasswordView::TestApi::SetTimers(
+    std::unique_ptr<base::RetainingOneShotTimer> clear_timer,
+    std::unique_ptr<base::RetainingOneShotTimer> hide_timer) {
+  view_->clear_password_timer_ = std::move(clear_timer);
+  view_->hide_password_timer_ = std::move(hide_timer);
+  // Starts the clearing timer.
+  view_->SetDisplayPasswordButtonVisible(true);
+}
+
+LoginPasswordView::LoginPasswordView()
+    : clear_password_timer_(std::make_unique<base::RetainingOneShotTimer>()),
+      hide_password_timer_(std::make_unique<base::RetainingOneShotTimer>()) {
   Shell::Get()->ime_controller()->AddObserver(this);
 
   auto* root_layout = SetLayoutManager(std::make_unique<views::BoxLayout>(
@@ -527,8 +554,28 @@ void LoginPasswordView::SetFocusEnabledOnTextfield(bool enable) {
   textfield_->SetFocusBehavior(behavior);
 }
 
+void LoginPasswordView::SetDisplayPasswordButtonVisible(bool visible) {
+  display_password_button_->SetVisible(visible);
+  // Only start the timer if the display password feature is enabled.
+  if (visible) {
+    clear_password_timer_->Start(
+        FROM_HERE, kClearPasswordAfterDelay,
+        base::BindRepeating(&LoginPasswordView::Clear, base::Unretained(this)));
+  }
+}
+
+void LoginPasswordView::Reset() {
+  Clear();
+  // A user could hit the display button, then quickly switch account and
+  // type; we want the password to be hidden in such a case.
+  HidePassword(false /*chromevox_exception*/);
+}
+
 void LoginPasswordView::Clear() {
   textfield_->SetText(base::string16());
+  // For security reasons, we also want to clear the edit history if the Clear
+  // function is invoked by the clear password timer.
+  textfield_->ClearEditHistory();
   // |ContentsChanged| won't be called by |Textfield| if the text is changed
   // by |Textfield::SetText()|.
   ContentsChanged(textfield_, textfield_->GetText());
@@ -585,17 +632,38 @@ bool LoginPasswordView::OnKeyPressed(const ui::KeyEvent& event) {
   return false;
 }
 
+void LoginPasswordView::InvertPasswordDisplayingState() {
+  display_password_button_->InvertToggled();
+  textfield_->InvertTextInputType();
+  hide_password_timer_->Start(
+      FROM_HERE, kHidePasswordAfterDelay,
+      base::BindRepeating(&LoginPasswordView::HidePassword,
+                          base::Unretained(this),
+                          true /*chromevox_exception*/));
+}
+
 void LoginPasswordView::ButtonPressed(views::Button* sender,
                                       const ui::Event& event) {
   DCHECK_EQ(sender, display_password_button_);
-  display_password_button_->InvertToggled();
-  textfield_->InvertTextInputType();
+  InvertPasswordDisplayingState();
+}
+
+void LoginPasswordView::HidePassword(bool chromevox_exception) {
+  if (chromevox_exception &&
+      Shell::Get()->accessibility_controller()->spoken_feedback_enabled()) {
+    return;
+  }
+  if (textfield_->GetTextInputType() == ui::TEXT_INPUT_TYPE_TEXT)
+    InvertPasswordDisplayingState();
 }
 
 void LoginPasswordView::ContentsChanged(views::Textfield* sender,
                                         const base::string16& new_contents) {
   DCHECK_EQ(sender, textfield_);
   on_password_text_changed_.Run(new_contents.empty() /*is_empty*/);
+  // Only reset the timer if the display password feature is enabled.
+  if (display_password_button_->GetVisible())
+    clear_password_timer_->Reset();
 }
 
 // Implements swapping active user with arrow keys

ash/login/ui/login_password_view.h

@@ -59,6 +59,9 @@ class ASH_EXPORT LoginPasswordView : public views::View,
     views::ToggleImageButton* display_password_button() const;
     views::View* easy_unlock_icon() const;
     void set_immediately_hover_easy_unlock_icon();
+    // Sets the timers that are used to clear and hide the password.
+    void SetTimers(std::unique_ptr<base::RetainingOneShotTimer> clear_timer,
+                   std::unique_ptr<base::RetainingOneShotTimer> hide_timer);
 
    private:
     LoginPasswordView* view_;
@@ -95,6 +98,12 @@ class ASH_EXPORT LoginPasswordView : public views::View,
   // Enable or disable focus on the password field.
   void SetFocusEnabledOnTextfield(bool enable);
 
+  // Sets whether the display password button is visible.
+  void SetDisplayPasswordButtonVisible(bool visible);
+
+  // Clear the text and put the password into hide mode.
+  void Reset();
+
   // Clear all currently entered text.
   void Clear();
 
@@ -118,12 +127,19 @@ class ASH_EXPORT LoginPasswordView : public views::View,
   void RequestFocus() override;
   bool OnKeyPressed(const ui::KeyEvent& event) override;
 
+  // Invert the textfield type and toggle the display password button.
+  void InvertPasswordDisplayingState();
+
   // views::ButtonListener:
   // Handles click on the display password button. Therefore, it inverts the
   // display password button icon's (show/hide) and shows/hides the content of
   // the password field.
   void ButtonPressed(views::Button* sender, const ui::Event& event) override;
 
+  // Hides the password. When |chromevox_exception| is true, the password is not
+  // hidden if ChromeVox is enabled.
+  void HidePassword(bool chromevox_exception);
+
   // views::TextfieldController:
   void ContentsChanged(views::Textfield* sender,
                        const base::string16& new_contents) override;
@@ -157,6 +173,15 @@ class ASH_EXPORT LoginPasswordView : public views::View,
   // Is the password field enabled when there is no text?
   bool enabled_on_empty_password_ = false;
 
+  // Clears the password field after a time without action if the display
+  // password feature is enabled.
+  std::unique_ptr<base::RetainingOneShotTimer> clear_password_timer_;
+
+  // Hides the password after a short delay if the password is shown, except if
+  // ChromeVox is enabled (otherwise, the user would not have time to navigate
+  // through the password and make the characters read out loud one by one).
+  std::unique_ptr<base::RetainingOneShotTimer> hide_password_timer_;
+
   views::View* password_row_ = nullptr;
 
   LoginTextfield* textfield_ = nullptr;

ash/login/ui/login_password_view_test.cc

@@ -9,6 +9,7 @@
 #include "ash/shell.h"
 #include "base/bind.h"
 #include "base/strings/utf_string_conversions.h"
+#include "base/timer/mock_timer.h"
 #include "ui/base/ime/text_input_type.h"
 #include "ui/events/test/event_generator.h"
 #include "ui/views/controls/button/image_button.h"
@@ -184,4 +185,61 @@ TEST_F(LoginPasswordViewTest, EasyUnlockMouseHover) {
   EXPECT_FALSE(easy_unlock_icon_tapped_called_);
 }
 
+// Checks that the user can't hit Ctrl+Z to revert the password when it has been
+// cleared.
+TEST_F(LoginPasswordViewTest, CtrlZDisabled) {
+  LoginPasswordView::TestApi test_api(view_);
+  ui::test::EventGenerator* generator = GetEventGenerator();
+
+  generator->PressKey(ui::KeyboardCode::VKEY_A, ui::EF_NONE);
+  EXPECT_FALSE(is_password_field_empty_);
+  view_->Clear();
+  EXPECT_TRUE(is_password_field_empty_);
+  generator->PressKey(ui::KeyboardCode::VKEY_Z, ui::EF_CONTROL_DOWN);
+  EXPECT_TRUE(is_password_field_empty_);
+}
+
+// Verifies that the password textfield  clear after a delay when the display
+// password button is shown.
+TEST_F(LoginPasswordViewTest, PasswordAutoClearsAndHides) {
+  LoginPasswordView::TestApi test_api(view_);
+  ui::test::EventGenerator* generator = GetEventGenerator();
+
+  // Install mock timers into the password view.
+  auto clear_timer0 = std::make_unique<base::MockRetainingOneShotTimer>();
+  auto hide_timer0 = std::make_unique<base::MockRetainingOneShotTimer>();
+  base::MockRetainingOneShotTimer* clear_timer = clear_timer0.get();
+  base::MockRetainingOneShotTimer* hide_timer = hide_timer0.get();
+  test_api.SetTimers(std::move(clear_timer0), std::move(hide_timer0));
+
+  // Verify clearing timer works.
+  generator->PressKey(ui::KeyboardCode::VKEY_A, ui::EF_NONE);
+  EXPECT_FALSE(is_password_field_empty_);
+
+  clear_timer->Fire();
+  EXPECT_TRUE(is_password_field_empty_);
+
+  // Check a second time.
+  generator->PressKey(ui::KeyboardCode::VKEY_A, ui::EF_NONE);
+  EXPECT_FALSE(is_password_field_empty_);
+  clear_timer->Fire();
+  EXPECT_TRUE(is_password_field_empty_);
+
+  // Verify hiding timer works; set the password visible first then fire the
+  // hiding timer and check it is hidden.
+  EXPECT_EQ(test_api.textfield()->GetTextInputType(),
+            ui::TEXT_INPUT_TYPE_PASSWORD);
+  generator->MoveMouseTo(
+      test_api.display_password_button()->GetBoundsInScreen().CenterPoint());
+  generator->ClickLeftButton();
+  EXPECT_EQ(test_api.textfield()->GetTextInputType(), ui::TEXT_INPUT_TYPE_TEXT);
+  hide_timer->Fire();
+  EXPECT_EQ(test_api.textfield()->GetTextInputType(),
+            ui::TEXT_INPUT_TYPE_PASSWORD);
+  // Hide an empty password already hidden and make sure a second fire works.
+  hide_timer->Fire();
+  EXPECT_EQ(test_api.textfield()->GetTextInputType(),
+            ui::TEXT_INPUT_TYPE_PASSWORD);
+}
+
 }  // namespace ash

ash/login/ui/login_test_base.cc

@@ -103,8 +103,10 @@ void LoginTestBase::SetUserCount(size_t count) {
 
 void LoginTestBase::AddUsers(size_t num_users) {
   for (size_t i = 0; i < num_users; i++) {
+    // TODO(tellier): Use gmail.com instead of domain.com temporarily  so the
+    // display password button can be accessible. See crbug.com/1062524
     std::string email =
-        base::StrCat({"user", std::to_string(users_.size()), "@domain.com"});
+        base::StrCat({"user", std::to_string(users_.size()), "@gmail.com"});
     users_.push_back(CreateUser(email));
   }