Add a spinner to confirm-password-change

The spinner is shown if it showSpinnerInitially is true, or
when the save button is clicked.
ShowSpinnerInitially is set to true if passwords are scraped
successfully and so we have the ability to try and change
the password without prompting the user for more input.
The confirm-password-page now listens to the password change manager
and if the auth fails, it prompts again, showing the error message
"incorrect password".
It doesn't listen for success - in this case, the dialog is closed.

Bug: 930109
Change-Id: Ib497aceec64ad6981168689425bacf2eb634ebda
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1687539
Commit-Queue: A Olsen <olsen@chromium.org>
Reviewed-by: Roman Sorokin [CET] <rsorokin@chromium.org>
Cr-Commit-Position: refs/heads/master@{#675646}

chrome/browser/chromeos/login/saml/in_session_password_change_manager.cc

@@ -22,8 +22,11 @@ InSessionPasswordChangeManager::CreateIfEnabled(Profile* primary_profile) {
   if (primary_profile->GetPrefs()->GetBoolean(
           prefs::kSamlInSessionPasswordChangeEnabled)) {
     return std::make_unique<InSessionPasswordChangeManager>(primary_profile);
+  } else {
+    // If the policy is disabled, clear the SAML password attributes.
+    SamlPasswordAttributes::DeleteFromPrefs(primary_profile->GetPrefs());
+    return nullptr;
   }
-  return nullptr;
 }
 
 InSessionPasswordChangeManager::InSessionPasswordChangeManager(
@@ -41,6 +44,32 @@ void InSessionPasswordChangeManager::StartInSessionPasswordChange() {
   PasswordChangeDialog::Show(primary_profile_);
 }
 
+void InSessionPasswordChangeManager::OnSamlPasswordChanged(
+    const std::string& scraped_old_password,
+    const std::string& scraped_new_password) {
+  user_manager::UserManager::Get()->SaveForceOnlineSignin(
+      primary_user_->GetAccountId(), true);
+  PasswordChangeDialog::Dismiss();
+
+  const bool both_passwords_scraped =
+      !scraped_old_password.empty() && !scraped_new_password.empty();
+  if (both_passwords_scraped) {
+    // Both passwords scraped so we try to change cryptohome password now.
+    // Show the confirm dialog but initially showing a spinner. If the change
+    // fails, then the dialog will hide the spinner and show a prompt.
+    // If the change succeeds, the dialog and spinner will just disappear.
+    ConfirmPasswordChangeDialog::Show(scraped_old_password,
+                                      scraped_new_password,
+                                      /*show_spinner_initially=*/true);
+    ChangePassword(scraped_old_password, scraped_new_password);
+  } else {
+    // Failed to scrape passwords - prompt for passwords immediately.
+    ConfirmPasswordChangeDialog::Show(scraped_old_password,
+                                      scraped_new_password,
+                                      /*show_spinner_initially=*/false);
+  }
+}
+
 void InSessionPasswordChangeManager::ChangePassword(
     const std::string& old_password,
     const std::string& new_password) {
@@ -49,11 +78,12 @@ void InSessionPasswordChangeManager::ChangePassword(
   authenticator_->MigrateKey(user_context, old_password);
 }
 
-void InSessionPasswordChangeManager::HandlePasswordScrapeFailure() {
-  // TODO(https://crbug.com/930109): Show different versions of the dialog
-  // depending on whether any usable passwords were scraped.
-  PasswordChangeDialog::Dismiss();
-  ConfirmPasswordChangeDialog::Show();
+void InSessionPasswordChangeManager::AddObserver(Observer* observer) {
+  observer_list_.AddObserver(observer);
+}
+
+void InSessionPasswordChangeManager::RemoveObserver(Observer* observer) {
+  observer_list_.RemoveObserver(observer);
 }
 
 void InSessionPasswordChangeManager::OnAuthSuccess(
@@ -77,8 +107,14 @@ void InSessionPasswordChangeManager::OnAuthSuccess(
 }
 
 void InSessionPasswordChangeManager::OnAuthFailure(const AuthFailure& error) {
-  // TODO(https://crbug.com/930109): Ask user for the old password.
   VLOG(1) << "Failed to change cryptohome password: " << error.GetErrorString();
+  NotifyObservers(CHANGE_PASSWORD_AUTH_FAILURE);
+}
+
+void InSessionPasswordChangeManager::NotifyObservers(Event event) {
+  for (auto& observer : observer_list_) {
+    observer.OnEvent(event);
+  }
 }
 
 }  // namespace chromeos

chrome/browser/chromeos/login/saml/in_session_password_change_manager.h

@@ -9,6 +9,7 @@
 #include <string>
 
 #include "base/memory/scoped_refptr.h"
+#include "base/observer_list.h"
 #include "chromeos/login/auth/auth_status_consumer.h"
 
 class Profile;
@@ -25,6 +26,18 @@ class UserContext;
 // response from dialogs, and callbacks from subsystems.
 class InSessionPasswordChangeManager : public AuthStatusConsumer {
  public:
+  // Events in the in-session SAML password change flow.
+  enum Event {
+    CHANGE_PASSWORD_AUTH_FAILURE,
+    // TODO(https://crbug.com/930109): Add more useful events.
+  };
+
+  // Observers of InSessionPasswordChangeManager are notified of certain events.
+  class Observer : public base::CheckedObserver {
+   public:
+    virtual void OnEvent(Event event) = 0;
+  };
+
   // Returns null if in-session password change is disabled.
   static std::unique_ptr<InSessionPasswordChangeManager> CreateIfEnabled(
       Profile* primary_profile);
@@ -36,6 +49,12 @@ class InSessionPasswordChangeManager : public AuthStatusConsumer {
   // the user's SAML IdP change-password page:
   void StartInSessionPasswordChange();
 
+  // Handle a SAML password change. |old_password| and |new_password| can be
+  // empty if scraping failed, in which case the user will be prompted to enter
+  // them again. If they are scraped, this calls ChangePassword immediately,
+  void OnSamlPasswordChanged(const std::string& scraped_old_password,
+                             const std::string& scraped_new_password);
+
   // Change cryptohome password for primary user.
   void ChangePassword(const std::string& old_password,
                       const std::string& new_password);
@@ -44,13 +63,19 @@ class InSessionPasswordChangeManager : public AuthStatusConsumer {
   // by showing a dialog for the user to confirm their old + new password.
   void HandlePasswordScrapeFailure();
 
+  void AddObserver(Observer* observer);
+  void RemoveObserver(Observer* observer);
+
   // AuthStatusConsumer:
   void OnAuthFailure(const AuthFailure& error) override;
   void OnAuthSuccess(const UserContext& user_context) override;
 
  private:
+  void NotifyObservers(Event event);
+
   Profile* primary_profile_;
   const user_manager::User* primary_user_;
+  base::ObserverList<Observer> observer_list_;
 
   scoped_refptr<CryptohomeAuthenticator> authenticator_;
 

chrome/browser/resources/chromeos/password_change/confirm_password_change.html

@@ -7,14 +7,14 @@
   <link rel="import" href="chrome://resources/cr_elements/cr_button/cr_button.html">
   <link rel="import" href="chrome://resources/cr_elements/cr_dialog/cr_dialog.html">
   <link rel="import" href="chrome://resources/cr_elements/cr_input/cr_input.html">
-  <link rel="import" href="chrome://resources/cr_elements/icons.html">
   <link rel="import" href="chrome://resources/cr_elements/shared_vars_css.html">
   <link rel="import" href="chrome://resources/html/load_time_data.html">
   <link rel="import" href="chrome://resources/html/i18n_behavior.html">
   <link rel="import" href="chrome://resources/html/web_ui_listener_behavior.html">
-  <link rel="import" href="chrome://resources/polymer/v1_0/iron-icon/iron-icon.html">
+  <link rel="import" href="chrome://resources/polymer/v1_0/paper-spinner/paper-spinner-lite.html">
   <link rel="stylesheet" href="chrome://resources/css/text_defaults_md.css">
 
+
   <script src="confirm_password_change.js"></script>
   <script src="strings.js"></script>
 
@@ -41,20 +41,18 @@
           font-size: 20px;
         }
 
-        #password-match-error-container {
-          margin-top: -16px;
-        }
-
-        #error-icon {
-        --iron-icon-fill-color: var(--google-red-600);
-        }
-
-        #password-match-error {
-          color: var(--google-red-600);
-          display: inline-block;
+        paper-spinner-lite {
+          height: 44px;
+          left: 50%;
+          margin: -22px;
+          position: fixed;
+          top: 50%;
+          width: 44px;
         }
       </style>
 
+     <paper-spinner-lite active></paper-spinner-lite>
+
       <cr-dialog id="dialog" exportparts="dialog">
         <div slot="title">[[i18n('title')]]</div>
 
@@ -64,27 +62,24 @@
           <div>
             <cr-input type="password" value="{{old_password_}}"
                 label="[[i18n('oldPassword')]]"
-                invalid="[[invalidOldPassword_(currentValidationError_)]]">
+                invalid="[[invalidOldPassword_(currentValidationError_)]]"
+                error-message="[[errorString_]]">
             </cr-input>
           </div>
 
           <div>
             <cr-input type="password" value="{{new_password_}}"
                 label="[[i18n('newPassword')]]"
-                invalid="[[invalidNewPassword_(currentValidationError_)]]">
+                invalid="[[invalidNewPassword_(currentValidationError_)]]"
+                error-message="[[errorString_]]">
             </cr-input>
             <cr-input type="password" value="{{confirm_new_password_}}"
                 label="[[i18n('confirmNewPassword')]]"
                 invalid=
-                    "[[invalidConfirmNewPassword_(currentValidationError_)]]">
+                    "[[invalidConfirmNewPassword_(currentValidationError_)]]"
+                error-message="[[errorString_]]">
             </cr-input>
           </div>
-
-          <div id="password-match-error-container"
-              hidden="[[!passwordsDoNotMatch_(currentValidationError_)]]">
-            <iron-icon id="error-icon" icon="cr:warning"></iron-icon>
-            <div id="password-match-error">[[i18n('matchError')]]</div>
-          </div>
         </div>
 
         <div slot="button-container">

chrome/browser/resources/chromeos/password_change/confirm_password_change.js

@@ -6,11 +6,14 @@
  * @fileoverview 'confirm-password-change' is a dialog so that the user can
  * either confirm their old password, or confirm their new password (twice),
  * or both, as part of an in-session password change.
+ * The dialog shows a spinner while it tries to change the password. This
+ * spinner is also shown immediately in the case we are trying to change the
+ * password using scraped data, and if this fails the spinner is hidden and
+ * the main confirm dialog is shown.
  */
 
-// TODO(https://crbug.com/930109): Logic is not done. Need to add logic to
-// show a spinner, to show only some of the password fields,
-// and to handle clicks on the save button.
+// TODO(https://crbug.com/930109): Add logic to show only some of the passwords
+// fields if some of the passwords were successfully scraped.
 
 /** @enum{number} */
 const ValidationErrorType = {
@@ -19,6 +22,12 @@ const ValidationErrorType = {
   MISSING_NEW_PASSWORD: 2,
   MISSING_CONFIRM_NEW_PASSWORD: 3,
   PASSWORDS_DO_NOT_MATCH: 4,
+  INCORRECT_OLD_PASSWORD: 5,
+};
+
+/** Default arguments that are used when not embedded in a dialog. */
+const DefaultArgs = {
+  showSpinnerInitially: false,
 };
 
 Polymer({
@@ -50,11 +59,34 @@ Polymer({
       type: Number,
       value: ValidationErrorType.NO_ERROR,
     },
+
+    errorString_:
+        {type: String, computed: 'getErrorString(currentValidationError_)'}
   },
 
   /** @override */
   attached: function() {
-    this.$.dialog.showModal();
+    // WebDialogUI class stores extra parameters in JSON in 'dialogArguments'
+    // variable, if this webui is being rendered in a dialog.
+    const argsJson = chrome.getVariableValue('dialogArguments');
+    const args = argsJson ? JSON.parse(argsJson) : DefaultArgs;
+
+    this.showSpinner_(args.showSpinnerInitially);
+
+    this.addWebUIListener('incorrect-old-password', () => {
+      this.onIncorrectOldPassword_();
+    });
+  },
+
+  /**
+   * @private
+   */
+  showSpinner_: function(showSpinner) {
+    // Dialog is on top, spinner is underneath, so showing dialog hides spinner.
+    if (showSpinner)
+      this.$.dialog.close();
+    else
+      this.$.dialog.showModal();
   },
 
   /**
@@ -64,9 +96,24 @@ Polymer({
     this.currentValidationError_ = this.findFirstError_();
     if (this.currentValidationError_ == ValidationErrorType.NO_ERROR) {
       chrome.send('changePassword', [this.old_password_, this.new_password_]);
+      this.showSpinner_(true);
     }
   },
 
+  /**
+   * @private
+   */
+  onIncorrectOldPassword_: function() {
+    // Only show the error if the user had previously typed an old password.
+    // This is also called when an incorrect password was scraped. In that case
+    // we hide the spinner and show the dialog so they can confirm.
+    if (this.old_password_) {
+      this.currentValidationError_ = ValidationErrorType.INCORRECT_OLD_PASSWORD;
+      this.old_password_ = '';
+    }
+    this.showSpinner_(false);
+  },
+
   /**
    * @return {!ValidationErrorType}
    * @private
@@ -92,8 +139,9 @@ Polymer({
    * @private
    */
   invalidOldPassword_: function() {
-    return this.currentValidationError_ ==
-        ValidationErrorType.MISSING_OLD_PASSWORD;
+    const err = this.currentValidationError_;
+    return err == ValidationErrorType.MISSING_OLD_PASSWORD ||
+        err == ValidationErrorType.INCORRECT_OLD_PASSWORD;
   },
 
   /**
@@ -110,17 +158,23 @@ Polymer({
    * @private
    */
   invalidConfirmNewPassword_: function() {
-    return this.currentValidationError_ ==
-        ValidationErrorType.MISSING_CONFIRM_NEW_PASSWORD ||
-        this.passwordsDoNotMatch_();
+    const err = this.currentValidationError_;
+    return err == ValidationErrorType.MISSING_CONFIRM_NEW_PASSWORD ||
+        err == ValidationErrorType.PASSWORDS_DO_NOT_MATCH;
   },
 
   /**
-   * @return {boolean}
+   * @return {string}
    * @private
    */
-  passwordsDoNotMatch_: function() {
-    return this.currentValidationError_ ==
-        ValidationErrorType.PASSWORDS_DO_NOT_MATCH;
+  getErrorString_: function() {
+    switch (this.currentValidationError_) {
+      case ValidationErrorType.INCORRECT_OLD_PASSWORD:
+        return this.i18n('incorrectPassword');
+      case ValidationErrorType.PASSWORDS_DO_NOT_MATCH:
+        return this.i18n('matchError');
+      default:
+        return '';
+    }
   },
 });

chrome/browser/ui/webui/chromeos/in_session_password_change/confirm_password_change_handler.cc

@@ -21,9 +21,27 @@
 
 namespace chromeos {
 
-ConfirmPasswordChangeHandler::ConfirmPasswordChangeHandler() = default;
+ConfirmPasswordChangeHandler::ConfirmPasswordChangeHandler() {
+  auto* in_session_password_change_manager =
+      g_browser_process->platform_part()->in_session_password_change_manager();
+  CHECK(in_session_password_change_manager);
+  in_session_password_change_manager->AddObserver(this);
+}
+
+ConfirmPasswordChangeHandler::~ConfirmPasswordChangeHandler() {
+  auto* in_session_password_change_manager =
+      g_browser_process->platform_part()->in_session_password_change_manager();
+  CHECK(in_session_password_change_manager);
+  in_session_password_change_manager->RemoveObserver(this);
+}
 
-ConfirmPasswordChangeHandler::~ConfirmPasswordChangeHandler() = default;
+void ConfirmPasswordChangeHandler::OnEvent(
+    InSessionPasswordChangeManager::Event event) {
+  if (event == InSessionPasswordChangeManager::CHANGE_PASSWORD_AUTH_FAILURE) {
+    AllowJavascript();
+    FireWebUIListener("incorrect-old-password");
+  }
+}
 
 void ConfirmPasswordChangeHandler::HandleChangePassword(
     const base::ListValue* params) {
@@ -34,7 +52,6 @@ void ConfirmPasswordChangeHandler::HandleChangePassword(
   CHECK(in_session_password_change_manager);
   in_session_password_change_manager->ChangePassword(old_password,
                                                      new_password);
-  // TODO(olsen): Show a spinner until password change is complete.
 }
 
 void ConfirmPasswordChangeHandler::RegisterMessages() {

chrome/browser/ui/webui/chromeos/in_session_password_change/confirm_password_change_handler.h

@@ -7,22 +7,28 @@
 
 #include "base/memory/weak_ptr.h"
 #include "base/values.h"
+#include "chrome/browser/chromeos/login/saml/in_session_password_change_manager.h"
 #include "content/public/browser/web_ui_message_handler.h"
 
 namespace chromeos {
 
-class ConfirmPasswordChangeHandler : public content::WebUIMessageHandler {
+class ConfirmPasswordChangeHandler
+    : public content::WebUIMessageHandler,
+      public InSessionPasswordChangeManager::Observer {
  public:
   ConfirmPasswordChangeHandler();
   ~ConfirmPasswordChangeHandler() override;
 
-  // content::WebUIMessageHandler:
-  void RegisterMessages() override;
-
   // Tries to change the cryptohome password once the confirm-password-change
   // dialog is filled in and the password change is confirmed.
   void HandleChangePassword(const base::ListValue* passwords);
 
+  // InSessionPasswordChangeManager::Observer:
+  void OnEvent(InSessionPasswordChangeManager::Event event) override;
+
+  // content::WebUIMessageHandler:
+  void RegisterMessages() override;
+
  private:
   base::WeakPtrFactory<ConfirmPasswordChangeHandler> weak_factory_{this};
   DISALLOW_COPY_AND_ASSIGN(ConfirmPasswordChangeHandler);

chrome/browser/ui/webui/chromeos/in_session_password_change/password_change_handler.cc

@@ -52,22 +52,19 @@ void PasswordChangeHandler::HandleChangePassword(
   const base::Value& new_passwords = params->GetList()[1];
   VLOG(4) << "Scraped " << old_passwords.GetList().size() << " old passwords";
   VLOG(4) << "Scraped " << new_passwords.GetList().size() << " new passwords";
-  user_manager::User* user =
-      ProfileHelper::Get()->GetUserByProfile(Profile::FromWebUI(web_ui()));
-  user_manager::UserManager::Get()->SaveForceOnlineSignin(user->GetAccountId(),
-                                                          true);
+
+  const std::string old_password = (old_passwords.GetList().size() > 0)
+                                       ? old_passwords.GetList()[0].GetString()
+                                       : "";
+  const std::string new_password = (new_passwords.GetList().size() == 1)
+                                       ? new_passwords.GetList()[0].GetString()
+                                       : "";
+
   auto* in_session_password_change_manager =
       g_browser_process->platform_part()->in_session_password_change_manager();
   CHECK(in_session_password_change_manager);
-
-  if (new_passwords.GetList().size() == 1 &&
-      old_passwords.GetList().size() > 0) {
-    in_session_password_change_manager->ChangePassword(
-        old_passwords.GetList()[0].GetString(),
-        new_passwords.GetList()[0].GetString());
-  } else {
-    in_session_password_change_manager->HandlePasswordScrapeFailure();
-  }
+  in_session_password_change_manager->OnSamlPasswordChanged(old_password,
+                                                            new_password);
 }
 
 void PasswordChangeHandler::RegisterMessages() {

chrome/browser/ui/webui/chromeos/in_session_password_change/password_change_ui.cc

@@ -8,6 +8,7 @@
 
 #include "base/bind.h"
 #include "base/command_line.h"
+#include "base/json/json_writer.h"
 #include "base/strings/utf_string_conversions.h"
 #include "chrome/browser/chromeos/policy/user_cloud_policy_manager_chromeos.h"
 #include "chrome/browser/profiles/profile.h"
@@ -159,28 +160,17 @@ PasswordChangeUI::PasswordChangeUI(content::WebUI* web_ui)
 
 PasswordChangeUI::~PasswordChangeUI() = default;
 
-ConfirmPasswordChangeDialog::ConfirmPasswordChangeDialog()
-    : SystemWebDialogDelegate(GURL(chrome::kChromeUIConfirmPasswordChangeUrl),
-                              /*title=*/base::string16()) {}
-
-ConfirmPasswordChangeDialog::~ConfirmPasswordChangeDialog() {
-  DCHECK_EQ(this, g_confirm_dialog);
-  g_confirm_dialog = nullptr;
-}
-
-void ConfirmPasswordChangeDialog::GetDialogSize(gfx::Size* size) const {
-  *size = FitSizeToDisplay(kMaxConfirmPasswordChangeDialogWidth,
-                           kMaxConfirmPasswordChangeDialogHeight);
-}
-
 // static
-void ConfirmPasswordChangeDialog::Show() {
+void ConfirmPasswordChangeDialog::Show(const std::string& scraped_old_password,
+                                       const std::string& scraped_new_password,
+                                       bool show_spinner_initially) {
   DCHECK_CURRENTLY_ON(content::BrowserThread::UI);
   if (g_confirm_dialog) {
     g_confirm_dialog->Focus();
     return;
   }
-  g_confirm_dialog = new ConfirmPasswordChangeDialog();
+  g_confirm_dialog = new ConfirmPasswordChangeDialog(
+      scraped_old_password, scraped_new_password, show_spinner_initially);
   g_confirm_dialog->ShowSystemDialog();
 }
 
@@ -191,6 +181,36 @@ void ConfirmPasswordChangeDialog::Dismiss() {
     g_confirm_dialog->Close();
 }
 
+ConfirmPasswordChangeDialog::ConfirmPasswordChangeDialog(
+    const std::string& scraped_old_password,
+    const std::string& scraped_new_password,
+    bool show_spinner_initially)
+    : SystemWebDialogDelegate(GURL(chrome::kChromeUIConfirmPasswordChangeUrl),
+                              /*title=*/base::string16()),
+      scraped_old_password_(scraped_old_password),
+      scraped_new_password_(scraped_new_password),
+      show_spinner_initially_(show_spinner_initially) {}
+
+ConfirmPasswordChangeDialog::~ConfirmPasswordChangeDialog() {
+  DCHECK_EQ(this, g_confirm_dialog);
+  g_confirm_dialog = nullptr;
+}
+
+void ConfirmPasswordChangeDialog::GetDialogSize(gfx::Size* size) const {
+  *size = FitSizeToDisplay(kMaxConfirmPasswordChangeDialogWidth,
+                           kMaxConfirmPasswordChangeDialogHeight);
+}
+
+std::string ConfirmPasswordChangeDialog::GetDialogArgs() const {
+  // TODO(https://crbug.com/930109): Configure the embedded UI to only display
+  // prompts for the passwords that were not scraped.
+  std::string data;
+  base::DictionaryValue dialog_args;
+  dialog_args.SetBoolean("showSpinnerInitially", show_spinner_initially_);
+  base::JSONWriter::Write(dialog_args, &data);
+  return data;
+}
+
 ConfirmPasswordChangeUI::ConfirmPasswordChangeUI(content::WebUI* web_ui)
     : ui::WebDialogUI(web_ui) {
   Profile* profile = Profile::FromWebUI(web_ui);

chrome/browser/ui/webui/chromeos/in_session_password_change/password_change_ui.h

@@ -44,17 +44,26 @@ class PasswordChangeUI : public ui::WebDialogUI {
 // System dialog wrapping chrome://confirm-password-change
 class ConfirmPasswordChangeDialog : public SystemWebDialogDelegate {
  public:
-  static void Show();
+  static void Show(const std::string& scraped_old_password,
+                   const std::string& scraped_new_password,
+                   bool show_spinner_initially);
   static void Dismiss();
 
  protected:
-  ConfirmPasswordChangeDialog();
+  ConfirmPasswordChangeDialog(const std::string& scraped_old_password,
+                              const std::string& scraped_new_password,
+                              bool show_spinner_initially);
   ~ConfirmPasswordChangeDialog() override;
 
   // ui::WebDialogDelegate:
   void GetDialogSize(gfx::Size* size) const override;
+  std::string GetDialogArgs() const override;
 
  private:
+  std::string scraped_old_password_;
+  std::string scraped_new_password_;
+  bool show_spinner_initially_ = false;
+
   DISALLOW_COPY_AND_ASSIGN(ConfirmPasswordChangeDialog);
 };