Commit 21249b1a authored by A Olsen's avatar A Olsen Committed by Commit Bot

Modify confirm-password-change dialog to prompt less

Currently the dialog always prompts for both passwords
(the old one once, and the new one twice).

This change modifies it to only ask for passwords that
were not scraped.

Details:
If no passwords are scraped, both prompts are show immediately.
If the wrong old password is then entered, the same prompts are
shown again with "Incorrect password" next to the old password prompt.

If both passwords are scraped, a spinner is shown immediately.
If the old password scraped then fails to unlock cryptohome, then
the old-password prompt is shown as below.

If only the new password is scraped, then only the old password
prompt is shown. If the wrong old password is then entered, the
same prompt is shown again with "Incorrect password" next to it.

If only the old password is scraped, then only the new password
prompt is shown. Once the new password has been entered consistently,
if the old password scraped then fails to unlock cryptohome, then
the dialog is reshown but now with an old password prompt added.
If the old password is then entered incorrectly, then as usual,
the "Incorrect password" text will be shown next to that prompt.

The dialog is sized appropriately when first shown to make room for
the prompts that it contains. In the case that a new prompt is
added, it resizes itself.

Bug: 930109
Change-Id: I89f326ce50f4b0543c5053af25cae618b0c8ee92
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1724080
Commit-Queue: A Olsen <olsen@chromium.org>
Reviewed-by: default avatarRoman Sorokin [CET] <rsorokin@chromium.org>
Cr-Commit-Position: refs/heads/master@{#685184}
parent e77c238f
......@@ -347,6 +347,8 @@ void InSessionPasswordChangeManager::ChangePassword(
const std::string& old_password,
const std::string& new_password,
PasswordSource password_source) {
CHECK(!old_password.empty() && !new_password.empty());
password_source_ = password_source;
NotifyObservers(Event::START_CRYPTOHOME_PASSWORD_CHANGE);
UserContext user_context(*primary_user_);
......
......@@ -78,23 +78,23 @@
<div slot="title">[[i18n('title')]]</div>
<div slot="body" spellcheck="false">
<div id="prompt">[[i18n('bothPasswordsPrompt')]]</div>
<div id="prompt">[[promptString_]]</div>
<div>
<cr-input type="password" value="{{old_password_}}"
<div hidden="[[!showOldPasswordPrompt_]]">
<cr-input type="password" value="{{oldPassword_}}"
label="[[i18n('oldPassword')]]"
invalid="[[invalidOldPassword_(currentValidationError_)]]"
error-message="[[errorString_]]">
</cr-input>
</div>
<div>
<cr-input type="password" value="{{new_password_}}"
<div hidden="[[!showNewPasswordPrompt_]]">
<cr-input type="password" value="{{newPassword_}}"
label="[[i18n('newPassword')]]"
invalid="[[invalidNewPassword_(currentValidationError_)]]"
error-message="[[errorString_]]">
</cr-input>
<cr-input type="password" value="{{confirm_new_password_}}"
<cr-input type="password" value="{{confirmNewPassword_}}"
label="[[i18n('confirmNewPassword')]]"
invalid=
"[[invalidConfirmNewPassword_(currentValidationError_)]]"
......
......@@ -25,93 +25,121 @@ const ValidationErrorType = {
INCORRECT_OLD_PASSWORD: 5,
};
/** Default arguments that are used when not embedded in a dialog. */
const DefaultArgs = {
showSpinnerInitially: false,
};
Polymer({
is: 'confirm-password-change',
behaviors: [I18nBehavior, WebUIListenerBehavior],
properties: {
/** @private {boolean} */
showSpinner_:
{type: Boolean, value: true, observer: 'onShowSpinnerChanged_'},
/** @private {boolean} */
showOldPasswordPrompt_: {type: Boolean, value: true},
/** @private {string} */
old_password_: {
type: String,
value: '',
},
oldPassword_: {type: String, value: ''},
/** @private {boolean} */
showNewPasswordPrompt_: {type: Boolean, value: true},
/** @private {string} */
new_password_: {
type: String,
value: '',
},
newPassword_: {type: String, value: ''},
/** @private {string} */
confirm_new_password_: {
type: String,
value: '',
},
confirmNewPassword_: {type: String, value: ''},
/** @private {!ValidationErrorType} */
currentValidationError_: {
type: Number,
value: ValidationErrorType.NO_ERROR,
observer: 'onErrorChanged_',
},
/** @private {string} */
promptString_: {
type: String,
computed:
'getPromptString_(showOldPasswordPrompt_, showNewPasswordPrompt_)',
},
/** @private {string} */
errorString_:
{type: String, computed: 'getErrorString_(currentValidationError_)'}
{type: String, computed: 'getErrorString_(currentValidationError_)'},
},
observers: [
'onShowPromptChanged_(showOldPasswordPrompt_, showNewPasswordPrompt_)',
],
/** @override */
attached: function() {
// 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_();
});
this.getInitialState_();
},
/**
* @private
*/
showSpinner_: function(showSpinner) {
/** @private */
getInitialState_() {
cr.sendWithPromise('getInitialState').then((result) => {
this.showOldPasswordPrompt_ = result.showOldPasswordPrompt;
this.showNewPasswordPrompt_ = result.showNewPasswordPrompt;
this.showSpinner_ = result.showSpinner;
});
},
/** @private */
onShowSpinnerChanged_: function() {
// Dialog is on top, spinner is underneath, so showing dialog hides spinner.
if (showSpinner)
if (this.showSpinner_)
this.$.dialog.close();
else
this.$.dialog.showModal();
},
/**
* @private
*/
/** @private */
onShowPromptChanged_: function() {
const suffix = (this.showOldPasswordPrompt_ ? 'Old' : '') +
(this.showNewPasswordPrompt_ ? 'New' : '');
const width = loadTimeData.getInteger('width' + suffix);
const height = loadTimeData.getInteger('height' + suffix);
window.resizeTo(width, height);
},
/** @private */
onErrorChanged_: function() {
if (this.currentValidationError_ != ValidationErrorType.NO_ERROR) {
this.showSpinner_ = false;
}
},
/** @private */
onSaveTap_: function() {
this.currentValidationError_ = this.findFirstError_();
if (this.currentValidationError_ == ValidationErrorType.NO_ERROR) {
chrome.send('changePassword', [this.old_password_, this.new_password_]);
this.showSpinner_(true);
chrome.send('changePassword', [this.oldPassword_, this.newPassword_]);
this.showSpinner_ = true;
}
},
/**
* @private
*/
/** @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_) {
if (this.showOldPasswordPrompt_) {
// User manually typed in the incorrect old password. Show the user an
// incorrect password error and hide the spinner so they can try again.
this.currentValidationError_ = ValidationErrorType.INCORRECT_OLD_PASSWORD;
this.old_password_ = '';
} else {
// Until now we weren't showing the old password prompt, since we had
// scraped the old password. But the password we scraped seems to be the
// wrong one. So, start again, but this time ask for the old password too.
this.showOldPasswordPrompt_ = true;
this.currentValidationError_ = ValidationErrorType.MISSING_OLD_PASSWORD;
}
this.showSpinner_(false);
},
/**
......@@ -119,18 +147,22 @@ Polymer({
* @private
*/
findFirstError_: function() {
if (!this.old_password_) {
if (this.showOldPasswordPrompt_) {
if (!this.oldPassword_) {
return ValidationErrorType.MISSING_OLD_PASSWORD;
}
if (!this.new_password_) {
}
if (this.showNewPasswordPrompt_) {
if (!this.newPassword_) {
return ValidationErrorType.MISSING_NEW_PASSWORD;
}
if (!this.confirm_new_password_) {
if (!this.confirmNewPassword_) {
return ValidationErrorType.MISSING_CONFIRM_NEW_PASSWORD;
}
if (this.new_password_ != this.confirm_new_password_) {
if (this.newPassword_ != this.confirmNewPassword_) {
return ValidationErrorType.PASSWORDS_DO_NOT_MATCH;
}
}
return ValidationErrorType.NO_ERROR;
},
......@@ -163,6 +195,23 @@ Polymer({
err == ValidationErrorType.PASSWORDS_DO_NOT_MATCH;
},
/**
* @return {string}
* @private
*/
getPromptString_: function() {
if (this.showOldPasswordPrompt_ && this.showNewPasswordPrompt_) {
return this.i18n('bothPasswordsPrompt');
}
if (this.showOldPasswordPrompt_) {
return this.i18n('oldPasswordPrompt');
}
if (this.showNewPasswordPrompt_) {
return this.i18n('newPasswordPrompt');
}
return '';
},
/**
* @return {string}
* @private
......
......@@ -27,9 +27,21 @@ const InSessionPasswordChangeManager::Event kIncorrectPasswordEvent =
const InSessionPasswordChangeManager::PasswordSource kPasswordSource =
InSessionPasswordChangeManager::PasswordSource::PASSWORDS_RETYPED;
// Returns one if it is non-empty, otherwise returns two.
const std::string& FirstNonEmpty(const std::string& one,
const std::string& two) {
return !one.empty() ? one : two;
}
} // namespace
ConfirmPasswordChangeHandler::ConfirmPasswordChangeHandler() {
ConfirmPasswordChangeHandler::ConfirmPasswordChangeHandler(
const std::string& scraped_old_password,
const std::string& scraped_new_password,
const bool show_spinner_initially)
: scraped_old_password_(scraped_old_password),
scraped_new_password_(scraped_new_password),
show_spinner_initially_(show_spinner_initially) {
if (InSessionPasswordChangeManager::IsInitialized()) {
InSessionPasswordChangeManager::Get()->AddObserver(this);
}
......@@ -41,23 +53,51 @@ ConfirmPasswordChangeHandler::~ConfirmPasswordChangeHandler() {
}
}
void ConfirmPasswordChangeHandler::OnEvent(
InSessionPasswordChangeManager::Event event) {
if (event == kIncorrectPasswordEvent) {
void ConfirmPasswordChangeHandler::HandleGetInitialState(
const base::ListValue* params) {
const std::string callback_id = params->GetList()[0].GetString();
base::Value state(base::Value::Type::DICTIONARY);
state.SetBoolKey("showOldPasswordPrompt", scraped_old_password_.empty());
state.SetBoolKey("showNewPasswordPrompt", scraped_new_password_.empty());
state.SetBoolKey("showSpinner", show_spinner_initially_);
AllowJavascript();
FireWebUIListener("incorrect-old-password");
}
ResolveJavascriptCallback(base::Value(callback_id), state);
}
void ConfirmPasswordChangeHandler::HandleChangePassword(
const base::ListValue* params) {
const std::string old_password = params->GetList()[0].GetString();
const std::string new_password = params->GetList()[1].GetString();
const std::string old_password =
FirstNonEmpty(params->GetList()[0].GetString(), scraped_old_password_);
const std::string new_password =
FirstNonEmpty(params->GetList()[1].GetString(), scraped_new_password_);
DCHECK(!old_password.empty() && !new_password.empty());
InSessionPasswordChangeManager::Get()->ChangePassword(
old_password, new_password, kPasswordSource);
}
void ConfirmPasswordChangeHandler::OnEvent(
InSessionPasswordChangeManager::Event event) {
if (event == kIncorrectPasswordEvent) {
// If this event comes before getInitialState, then don't show the spinner
// initially after all - the initial password change attempt using scraped
// passwords already failed before the dialog finished loading.
show_spinner_initially_ = false;
// Discard the scraped old password and ask the user what it really is.
scraped_old_password_.clear();
if (IsJavascriptAllowed()) {
FireWebUIListener("incorrect-old-password");
}
}
}
void ConfirmPasswordChangeHandler::RegisterMessages() {
web_ui()->RegisterMessageCallback(
"getInitialState",
base::BindRepeating(&ConfirmPasswordChangeHandler::HandleGetInitialState,
weak_factory_.GetWeakPtr()));
web_ui()->RegisterMessageCallback(
"changePassword",
base::BindRepeating(&ConfirmPasswordChangeHandler::HandleChangePassword,
......
......@@ -16,9 +16,14 @@ class ConfirmPasswordChangeHandler
: public content::WebUIMessageHandler,
public InSessionPasswordChangeManager::Observer {
public:
ConfirmPasswordChangeHandler();
ConfirmPasswordChangeHandler(const std::string& scraped_old_password,
const std::string& scraped_new_password,
const bool show_spinner_initially);
~ConfirmPasswordChangeHandler() override;
// Called by the JS UI to find out what to show and what size to be.
void HandleGetInitialState(const base::ListValue* params);
// 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);
......@@ -30,6 +35,10 @@ class ConfirmPasswordChangeHandler
void RegisterMessages() override;
private:
std::string scraped_old_password_;
std::string scraped_new_password_;
bool show_spinner_initially_ = false;
base::WeakPtrFactory<ConfirmPasswordChangeHandler> weak_factory_{this};
DISALLOW_COPY_AND_ASSIGN(ConfirmPasswordChangeHandler);
};
......
......@@ -7,29 +7,13 @@
#include <memory>
#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/login/saml/password_expiry_notification.h"
#include "chrome/browser/chromeos/policy/user_cloud_policy_manager_chromeos.h"
#include "chrome/browser/profiles/profile.h"
#include "chrome/browser/ui/webui/chromeos/in_session_password_change/confirm_password_change_handler.h"
#include "chrome/browser/ui/webui/chromeos/in_session_password_change/password_change_handler.h"
#include "chrome/browser/ui/webui/chromeos/in_session_password_change/urgent_password_expiry_notification_handler.h"
#include "chrome/browser/ui/webui/localized_string.h"
#include "chrome/common/pref_names.h"
#include "chrome/common/webui_url_constants.h"
#include "chrome/grit/browser_resources.h"
#include "chrome/grit/generated_resources.h"
#include "chromeos/constants/chromeos_switches.h"
#include "chromeos/login/auth/saml_password_attributes.h"
#include "chromeos/strings/grit/chromeos_strings.h"
#include "components/prefs/pref_service.h"
#include "components/strings/grit/components_strings.h"
#include "content/public/browser/browser_thread.h"
#include "content/public/browser/web_ui_data_source.h"
#include "net/base/url_util.h"
#include "ui/base/l10n/l10n_util.h"
#include "ui/display/display.h"
#include "ui/display/screen.h"
#include "ui/strings/grit/ui_strings.h"
......@@ -44,13 +28,16 @@ ConfirmPasswordChangeDialog* g_confirm_dialog = nullptr;
UrgentPasswordExpiryNotificationDialog* g_notification_dialog = nullptr;
constexpr gfx::Size kPasswordChangeDialogDesiredSize(768, 640);
constexpr gfx::Size kPasswordChangeSize(768, 640);
// TODO(https://crbug.com/930109): Change these numbers depending on what is
// shown in the dialog.
constexpr gfx::Size kConfirmPasswordChangeDialogDesiredSize(520, 380);
constexpr gfx::Size kUrgentPasswordExpiryNotificationSize = kPasswordChangeSize;
constexpr gfx::Size kNotificationDesiredSize = kPasswordChangeDialogDesiredSize;
// The size of the confirm password change UI depends on which passwords were
// scraped and which ones we need to prompt for:
constexpr int kConfirmPasswordsWidth = 520;
constexpr int kConfirmOldPasswordHeight = 230;
constexpr int kConfirmNewPasswordHeight = 310;
constexpr int kConfirmBothPasswordsHeight = 380;
// Given a desired size, returns the same size if it fits on screen,
// or the closest possible size that will fit on the screen.
......@@ -110,7 +97,7 @@ void PasswordChangeDialog::Dismiss() {
PasswordChangeDialog::PasswordChangeDialog()
: BasePasswordDialog(GURL(chrome::kChromeUIPasswordChangeUrl),
kPasswordChangeDialogDesiredSize) {}
kPasswordChangeSize) {}
PasswordChangeDialog::~PasswordChangeDialog() {
DCHECK_EQ(this, g_dialog);
......@@ -142,8 +129,9 @@ ConfirmPasswordChangeDialog::ConfirmPasswordChangeDialog(
const std::string& scraped_old_password,
const std::string& scraped_new_password,
bool show_spinner_initially)
: BasePasswordDialog(GURL(chrome::kChromeUIConfirmPasswordChangeUrl),
kConfirmPasswordChangeDialogDesiredSize),
: BasePasswordDialog(
GURL(chrome::kChromeUIConfirmPasswordChangeUrl),
GetSize(scraped_old_password.empty(), scraped_new_password.empty())),
scraped_old_password_(scraped_old_password),
scraped_new_password_(scraped_new_password),
show_spinner_initially_(show_spinner_initially) {}
......@@ -153,14 +141,32 @@ ConfirmPasswordChangeDialog::~ConfirmPasswordChangeDialog() {
g_confirm_dialog = nullptr;
}
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::Value dialog_args(base::Value::Type::DICTIONARY);
dialog_args.SetBoolKey("showSpinnerInitially", show_spinner_initially_);
base::JSONWriter::Write(dialog_args, &data);
return data;
// static
gfx::Size ConfirmPasswordChangeDialog::GetSize(
const bool show_old_password_prompt,
const bool show_new_password_prompt) {
const int desired_width = kConfirmPasswordsWidth;
if (show_old_password_prompt && show_new_password_prompt) {
return gfx::Size(desired_width, kConfirmBothPasswordsHeight);
}
if (show_new_password_prompt) {
return gfx::Size(desired_width, kConfirmNewPasswordHeight);
}
// Use the same size for these two cases:
// 1) We scraped new password, but not old, so we need to prompt for that.
// 2) We scraped both passwords, so we don't need to prompt for anything.
// In case 2, we need to show a spinner. That spinner could be any size, so
// we size it the same as in case 1, because there is a chance that the
// scraped password will be wrong and so we will need to show the old password
// prompt. So it looks best if the dialog is already the right size.
return gfx::Size(desired_width, kConfirmOldPasswordHeight);
}
void ConfirmPasswordChangeDialog::GetWebUIMessageHandlers(
std::vector<content::WebUIMessageHandler*>* handlers) const {
handlers->push_back(new ConfirmPasswordChangeHandler(
scraped_old_password_, scraped_new_password_, show_spinner_initially_));
}
// static
......@@ -184,7 +190,7 @@ void UrgentPasswordExpiryNotificationDialog::Dismiss() {
UrgentPasswordExpiryNotificationDialog::UrgentPasswordExpiryNotificationDialog()
: BasePasswordDialog(
GURL(chrome::kChromeUIUrgentPasswordExpiryNotificationUrl),
kNotificationDesiredSize) {}
kUrgentPasswordExpiryNotificationSize) {}
UrgentPasswordExpiryNotificationDialog::
~UrgentPasswordExpiryNotificationDialog() {
......
......@@ -51,6 +51,10 @@ class ConfirmPasswordChangeDialog : public BasePasswordDialog {
bool show_spinner_initially);
static void Dismiss();
// How big does this dialog need to be to show these prompts:
static gfx::Size GetSize(bool show_old_password_prompt,
bool show_new_password_prompt);
protected:
ConfirmPasswordChangeDialog(const std::string& scraped_old_password,
const std::string& scraped_new_password,
......@@ -58,7 +62,8 @@ class ConfirmPasswordChangeDialog : public BasePasswordDialog {
~ConfirmPasswordChangeDialog() override;
// ui::WebDialogDelegate:
std::string GetDialogArgs() const override;
void GetWebUIMessageHandlers(
std::vector<content::WebUIMessageHandler*>* handlers) const override;
private:
std::string scraped_old_password_;
......
......@@ -13,7 +13,7 @@
#include "chrome/browser/chromeos/login/saml/password_expiry_notification.h"
#include "chrome/browser/chromeos/policy/user_cloud_policy_manager_chromeos.h"
#include "chrome/browser/profiles/profile.h"
#include "chrome/browser/ui/webui/chromeos/in_session_password_change/confirm_password_change_handler.h"
#include "chrome/browser/ui/webui/chromeos/in_session_password_change/password_change_dialogs.h"
#include "chrome/browser/ui/webui/chromeos/in_session_password_change/password_change_handler.h"
#include "chrome/browser/ui/webui/chromeos/in_session_password_change/urgent_password_expiry_notification_handler.h"
#include "chrome/browser/ui/webui/localized_string.h"
......@@ -67,6 +67,13 @@ base::string16 GetHostedHeaderText(const std::string& password_change_url) {
host);
}
void AddSize(content::WebUIDataSource* source,
const std::string& suffix,
const gfx::Size& size) {
source->AddInteger("width" + suffix, size.width());
source->AddInteger("height" + suffix, size.height());
}
} // namespace
PasswordChangeUI::PasswordChangeUI(content::WebUI* web_ui)
......@@ -115,18 +122,25 @@ ConfirmPasswordChangeUI::ConfirmPasswordChangeUI(content::WebUI* web_ui)
{"oldPassword", IDS_PASSWORD_CHANGE_OLD_PASSWORD_LABEL},
{"newPassword", IDS_PASSWORD_CHANGE_NEW_PASSWORD_LABEL},
{"confirmNewPassword", IDS_PASSWORD_CHANGE_CONFIRM_NEW_PASSWORD_LABEL},
{"incorrectPassword", IDS_LOGIN_CONFIRM_PASSWORD_INCORRECT_PASSWORD},
{"matchError", IDS_PASSWORD_CHANGE_PASSWORDS_DONT_MATCH},
{"save", IDS_PASSWORD_CHANGE_CONFIRM_SAVE_BUTTON}};
AddLocalizedStringsBulk(source, kLocalizedStrings,
base::size(kLocalizedStrings));
AddSize(source, "", ConfirmPasswordChangeDialog::GetSize(false, false));
AddSize(source, "Old", ConfirmPasswordChangeDialog::GetSize(true, false));
AddSize(source, "New", ConfirmPasswordChangeDialog::GetSize(false, true));
AddSize(source, "OldNew", ConfirmPasswordChangeDialog::GetSize(true, true));
source->SetJsonPath("strings.js");
source->SetDefaultResource(IDR_CONFIRM_PASSWORD_CHANGE_HTML);
source->AddResourcePath("confirm_password_change.js",
IDR_CONFIRM_PASSWORD_CHANGE_JS);
web_ui->AddMessageHandler(std::make_unique<ConfirmPasswordChangeHandler>());
// The ConfirmPasswordChangeHandler is added by the dialog, so no need to add
// it here.
content::WebUIDataSource::Add(profile, source);
}
......
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