Commit c4c3bb07 authored by siyua's avatar siyua Committed by Commit Bot

[Autofill Auth UI] Disable the cancel button when OS dialog is visible.

Disable the cancel button in the Webauthn verify pending dialog when
the system OS authentication dialog is shown. At this moment, if user
cancel the OS dialog, the verify pending dialog will also go away.

After user interacts with the system OS dialog, the OS dialog will
go away and the cancel button will be enabled again for users to
cancel the flow if they want.

Bug: 991037
Change-Id: I8760690e377e3d294832dc5278d177eb3017dc65
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1935493
Commit-Queue: Siyu An <siyua@chromium.org>
Reviewed-by: default avatarFabio Tirelo <ftirelo@chromium.org>
Reviewed-by: default avatarVasilii Sukhanov <vasilii@chromium.org>
Reviewed-by: default avatarManas Verma <manasverma@google.com>
Cr-Commit-Position: refs/heads/master@{#720832}
parent 65fc95ff
...@@ -293,6 +293,17 @@ void ChromeAutofillClient::UpdateWebauthnOfferDialogWithError() { ...@@ -293,6 +293,17 @@ void ChromeAutofillClient::UpdateWebauthnOfferDialogWithError() {
controller->UpdateDialog(WebauthnDialogState::kOfferError); controller->UpdateDialog(WebauthnDialogState::kOfferError);
} }
void ChromeAutofillClient::UpdateWebauthnVerifyPendingCancelButton(
bool should_be_enabled) {
WebauthnDialogControllerImpl* controller =
autofill::WebauthnDialogControllerImpl::FromWebContents(web_contents());
if (controller) {
controller->UpdateDialog(
should_be_enabled ? WebauthnDialogState::kVerifyPending
: WebauthnDialogState::kVerifyPendingButtonDisabled);
}
}
bool ChromeAutofillClient::CloseWebauthnDialog() { bool ChromeAutofillClient::CloseWebauthnDialog() {
WebauthnDialogControllerImpl* controller = WebauthnDialogControllerImpl* controller =
autofill::WebauthnDialogControllerImpl::FromWebContents(web_contents()); autofill::WebauthnDialogControllerImpl::FromWebContents(web_contents());
......
...@@ -92,6 +92,7 @@ class ChromeAutofillClient ...@@ -92,6 +92,7 @@ class ChromeAutofillClient
void ShowWebauthnVerifyPendingDialog( void ShowWebauthnVerifyPendingDialog(
WebauthnDialogCallback verify_pending_dialog_callback) override; WebauthnDialogCallback verify_pending_dialog_callback) override;
void UpdateWebauthnOfferDialogWithError() override; void UpdateWebauthnOfferDialogWithError() override;
void UpdateWebauthnVerifyPendingCancelButton(bool should_be_enabled) override;
bool CloseWebauthnDialog() override; bool CloseWebauthnDialog() override;
#endif // !defined(OS_ANDROID) #endif // !defined(OS_ANDROID)
void ConfirmSaveAutofillProfile(const AutofillProfile& profile, void ConfirmSaveAutofillProfile(const AutofillProfile& profile,
......
...@@ -55,12 +55,26 @@ bool WebauthnDialogControllerImpl::CloseDialog() { ...@@ -55,12 +55,26 @@ bool WebauthnDialogControllerImpl::CloseDialog() {
void WebauthnDialogControllerImpl::UpdateDialog( void WebauthnDialogControllerImpl::UpdateDialog(
WebauthnDialogState dialog_state) { WebauthnDialogState dialog_state) {
if (!dialog_model_)
return;
switch (dialog_state) {
case WebauthnDialogState::kOfferError:
callback_.Reset();
break;
case WebauthnDialogState::kVerifyPending:
case WebauthnDialogState::kVerifyPendingButtonDisabled:
// Do not reset the |callback_| here since after the platform
// authentication is done, the cancel button will be enabled again.
break;
case WebauthnDialogState::kUnknown:
case WebauthnDialogState::kInactive:
case WebauthnDialogState::kOffer:
case WebauthnDialogState::kOfferPending:
NOTREACHED();
break;
}
dialog_model_->SetDialogState(dialog_state); dialog_model_->SetDialogState(dialog_state);
// TODO(crbug.com/991037): Handle callback resetting for verify pending
// dialog. Right now this function should only be passed in
// WebauthnDialogState::kOfferError.
DCHECK_EQ(dialog_state, WebauthnDialogState::kOfferError);
callback_.Reset();
} }
void WebauthnDialogControllerImpl::OnDialogClosed() { void WebauthnDialogControllerImpl::OnDialogClosed() {
...@@ -95,6 +109,7 @@ void WebauthnDialogControllerImpl::OnCancelButtonClicked() { ...@@ -95,6 +109,7 @@ void WebauthnDialogControllerImpl::OnCancelButtonClicked() {
case WebauthnDialogState::kUnknown: case WebauthnDialogState::kUnknown:
case WebauthnDialogState::kInactive: case WebauthnDialogState::kInactive:
case WebauthnDialogState::kOfferError: case WebauthnDialogState::kOfferError:
case WebauthnDialogState::kVerifyPendingButtonDisabled:
NOTREACHED(); NOTREACHED();
return; return;
} }
......
...@@ -35,7 +35,8 @@ void WebauthnDialogModel::RemoveObserver( ...@@ -35,7 +35,8 @@ void WebauthnDialogModel::RemoveObserver(
bool WebauthnDialogModel::IsActivityIndicatorVisible() const { bool WebauthnDialogModel::IsActivityIndicatorVisible() const {
return state_ == WebauthnDialogState::kOfferPending || return state_ == WebauthnDialogState::kOfferPending ||
state_ == WebauthnDialogState::kVerifyPending; state_ == WebauthnDialogState::kVerifyPending ||
state_ == WebauthnDialogState::kVerifyPendingButtonDisabled;
} }
bool WebauthnDialogModel::IsBackButtonVisible() const { bool WebauthnDialogModel::IsBackButtonVisible() const {
...@@ -46,6 +47,10 @@ bool WebauthnDialogModel::IsCancelButtonVisible() const { ...@@ -46,6 +47,10 @@ bool WebauthnDialogModel::IsCancelButtonVisible() const {
return true; return true;
} }
bool WebauthnDialogModel::IsCancelButtonEnabled() const {
return state_ != WebauthnDialogState::kVerifyPendingButtonDisabled;
}
base::string16 WebauthnDialogModel::GetCancelButtonLabel() const { base::string16 WebauthnDialogModel::GetCancelButtonLabel() const {
switch (state_) { switch (state_) {
case WebauthnDialogState::kOffer: case WebauthnDialogState::kOffer:
...@@ -56,6 +61,7 @@ base::string16 WebauthnDialogModel::GetCancelButtonLabel() const { ...@@ -56,6 +61,7 @@ base::string16 WebauthnDialogModel::GetCancelButtonLabel() const {
return l10n_util::GetStringUTF16( return l10n_util::GetStringUTF16(
IDS_AUTOFILL_WEBAUTHN_OPT_IN_DIALOG_CANCEL_BUTTON_LABEL_ERROR); IDS_AUTOFILL_WEBAUTHN_OPT_IN_DIALOG_CANCEL_BUTTON_LABEL_ERROR);
case WebauthnDialogState::kVerifyPending: case WebauthnDialogState::kVerifyPending:
case WebauthnDialogState::kVerifyPendingButtonDisabled:
return l10n_util::GetStringUTF16( return l10n_util::GetStringUTF16(
IDS_AUTOFILL_WEBAUTHN_VERIFY_PENDING_DIALOG_CANCEL_BUTTON_LABEL); IDS_AUTOFILL_WEBAUTHN_VERIFY_PENDING_DIALOG_CANCEL_BUTTON_LABEL);
case WebauthnDialogState::kInactive: case WebauthnDialogState::kInactive:
...@@ -85,6 +91,7 @@ const gfx::VectorIcon& WebauthnDialogModel::GetStepIllustration( ...@@ -85,6 +91,7 @@ const gfx::VectorIcon& WebauthnDialogModel::GetStepIllustration(
case WebauthnDialogState::kOffer: case WebauthnDialogState::kOffer:
case WebauthnDialogState::kOfferPending: case WebauthnDialogState::kOfferPending:
case WebauthnDialogState::kVerifyPending: case WebauthnDialogState::kVerifyPending:
case WebauthnDialogState::kVerifyPendingButtonDisabled:
return color_scheme == ImageColorScheme::kDark return color_scheme == ImageColorScheme::kDark
? kWebauthnDialogHeaderDarkIcon ? kWebauthnDialogHeaderDarkIcon
: kWebauthnDialogHeaderIcon; : kWebauthnDialogHeaderIcon;
...@@ -109,6 +116,7 @@ base::string16 WebauthnDialogModel::GetStepTitle() const { ...@@ -109,6 +116,7 @@ base::string16 WebauthnDialogModel::GetStepTitle() const {
return l10n_util::GetStringUTF16( return l10n_util::GetStringUTF16(
IDS_AUTOFILL_WEBAUTHN_OPT_IN_DIALOG_TITLE_ERROR); IDS_AUTOFILL_WEBAUTHN_OPT_IN_DIALOG_TITLE_ERROR);
case WebauthnDialogState::kVerifyPending: case WebauthnDialogState::kVerifyPending:
case WebauthnDialogState::kVerifyPendingButtonDisabled:
return l10n_util::GetStringUTF16( return l10n_util::GetStringUTF16(
IDS_AUTOFILL_WEBAUTHN_VERIFY_PENDING_DIALOG_TITLE); IDS_AUTOFILL_WEBAUTHN_VERIFY_PENDING_DIALOG_TITLE);
case WebauthnDialogState::kInactive: case WebauthnDialogState::kInactive:
...@@ -129,6 +137,7 @@ base::string16 WebauthnDialogModel::GetStepDescription() const { ...@@ -129,6 +137,7 @@ base::string16 WebauthnDialogModel::GetStepDescription() const {
return l10n_util::GetStringUTF16( return l10n_util::GetStringUTF16(
IDS_AUTOFILL_WEBAUTHN_OPT_IN_DIALOG_INSTRUCTION_ERROR); IDS_AUTOFILL_WEBAUTHN_OPT_IN_DIALOG_INSTRUCTION_ERROR);
case WebauthnDialogState::kVerifyPending: case WebauthnDialogState::kVerifyPending:
case WebauthnDialogState::kVerifyPendingButtonDisabled:
return base::string16(); return base::string16();
case WebauthnDialogState::kInactive: case WebauthnDialogState::kInactive:
case WebauthnDialogState::kUnknown: case WebauthnDialogState::kUnknown:
......
...@@ -35,6 +35,7 @@ class WebauthnDialogModel : public AuthenticatorRequestSheetModel { ...@@ -35,6 +35,7 @@ class WebauthnDialogModel : public AuthenticatorRequestSheetModel {
bool IsActivityIndicatorVisible() const override; bool IsActivityIndicatorVisible() const override;
bool IsBackButtonVisible() const override; bool IsBackButtonVisible() const override;
bool IsCancelButtonVisible() const override; bool IsCancelButtonVisible() const override;
bool IsCancelButtonEnabled() const override;
base::string16 GetCancelButtonLabel() const override; base::string16 GetCancelButtonLabel() const override;
bool IsAcceptButtonVisible() const override; bool IsAcceptButtonVisible() const override;
bool IsAcceptButtonEnabled() const override; bool IsAcceptButtonEnabled() const override;
......
...@@ -22,8 +22,8 @@ enum class WebauthnDialogState { ...@@ -22,8 +22,8 @@ enum class WebauthnDialogState {
// Indicating the card verification is in progress. Shown only for opted-in // Indicating the card verification is in progress. Shown only for opted-in
// users. // users.
kVerifyPending, kVerifyPending,
// TODO(crbug.com/991037): Add an extra state for case when the cancel button // Same as above but the cancel button should be disabled.
// in the verify pending dialog should be disabled. kVerifyPendingButtonDisabled,
}; };
} // namespace autofill } // namespace autofill
......
...@@ -4,6 +4,7 @@ ...@@ -4,6 +4,7 @@
#include "base/run_loop.h" #include "base/run_loop.h"
#include "chrome/browser/ui/autofill/payments/webauthn_dialog_controller_impl.h" #include "chrome/browser/ui/autofill/payments/webauthn_dialog_controller_impl.h"
#include "chrome/browser/ui/autofill/payments/webauthn_dialog_state.h"
#include "chrome/browser/ui/autofill/payments/webauthn_dialog_view.h" #include "chrome/browser/ui/autofill/payments/webauthn_dialog_view.h"
#include "chrome/browser/ui/browser.h" #include "chrome/browser/ui/browser.h"
#include "chrome/browser/ui/browser_window.h" #include "chrome/browser/ui/browser_window.h"
...@@ -133,4 +134,26 @@ IN_PROC_BROWSER_TEST_F(WebauthnDialogBrowserTest, ...@@ -133,4 +134,26 @@ IN_PROC_BROWSER_TEST_F(WebauthnDialogBrowserTest,
base::RunLoop().RunUntilIdle(); base::RunLoop().RunUntilIdle();
} }
// Ensures dialog cancel button is disabled when dialog state changed to
// |kVerifyPendingButtonDisabled|.
IN_PROC_BROWSER_TEST_F(WebauthnDialogBrowserTest,
VerifyPendingDialog_CancelButtonDisabled) {
ShowUi(kVerifyDialogName);
VerifyUi();
controller()->UpdateDialog(WebauthnDialogState::kVerifyPendingButtonDisabled);
EXPECT_FALSE(
GetWebauthnDialog()->IsDialogButtonEnabled(ui::DIALOG_BUTTON_CANCEL));
}
// Ensures dialog cancel button is enabled when dialog state changed to
// |kVerifyPending|.
IN_PROC_BROWSER_TEST_F(WebauthnDialogBrowserTest,
VerifyPendingDialog_CancelButtonEnabled) {
ShowUi(kVerifyDialogName);
VerifyUi();
controller()->UpdateDialog(WebauthnDialogState::kVerifyPending);
EXPECT_TRUE(
GetWebauthnDialog()->IsDialogButtonEnabled(ui::DIALOG_BUTTON_CANCEL));
}
} // namespace autofill } // namespace autofill
...@@ -69,6 +69,7 @@ void WebauthnDialogViewImpl::OnDialogStateChanged() { ...@@ -69,6 +69,7 @@ void WebauthnDialogViewImpl::OnDialogStateChanged() {
case WebauthnDialogState::kOfferPending: case WebauthnDialogState::kOfferPending:
case WebauthnDialogState::kOfferError: case WebauthnDialogState::kOfferError:
case WebauthnDialogState::kVerifyPending: case WebauthnDialogState::kVerifyPending:
case WebauthnDialogState::kVerifyPendingButtonDisabled:
RefreshContent(); RefreshContent();
break; break;
case WebauthnDialogState::kUnknown: case WebauthnDialogState::kUnknown:
...@@ -110,7 +111,7 @@ int WebauthnDialogViewImpl::GetDialogButtons() const { ...@@ -110,7 +111,7 @@ int WebauthnDialogViewImpl::GetDialogButtons() const {
bool WebauthnDialogViewImpl::IsDialogButtonEnabled( bool WebauthnDialogViewImpl::IsDialogButtonEnabled(
ui::DialogButton button) const { ui::DialogButton button) const {
return button == ui::DIALOG_BUTTON_OK ? model_->IsAcceptButtonEnabled() return button == ui::DIALOG_BUTTON_OK ? model_->IsAcceptButtonEnabled()
: true; : model_->IsCancelButtonEnabled();
} }
ui::ModalType WebauthnDialogViewImpl::GetModalType() const { ui::ModalType WebauthnDialogViewImpl::GetModalType() const {
......
...@@ -38,6 +38,7 @@ class TestSheetModel : public AuthenticatorRequestSheetModel { ...@@ -38,6 +38,7 @@ class TestSheetModel : public AuthenticatorRequestSheetModel {
bool IsActivityIndicatorVisible() const override { return true; } bool IsActivityIndicatorVisible() const override { return true; }
bool IsBackButtonVisible() const override { return true; } bool IsBackButtonVisible() const override { return true; }
bool IsCancelButtonVisible() const override { return true; } bool IsCancelButtonVisible() const override { return true; }
bool IsCancelButtonEnabled() const override { return true; }
base::string16 GetCancelButtonLabel() const override { base::string16 GetCancelButtonLabel() const override {
return base::ASCIIToUTF16("Test Cancel"); return base::ASCIIToUTF16("Test Cancel");
} }
......
...@@ -162,7 +162,7 @@ bool AuthenticatorRequestDialogView::IsDialogButtonEnabled( ...@@ -162,7 +162,7 @@ bool AuthenticatorRequestDialogView::IsDialogButtonEnabled(
case ui::DIALOG_BUTTON_OK: case ui::DIALOG_BUTTON_OK:
return sheet()->model()->IsAcceptButtonEnabled(); return sheet()->model()->IsAcceptButtonEnabled();
case ui::DIALOG_BUTTON_CANCEL: case ui::DIALOG_BUTTON_CANCEL:
return true; // Cancel is always enabled if visible. return sheet()->model()->IsCancelButtonEnabled();
} }
NOTREACHED(); NOTREACHED();
return false; return false;
...@@ -189,8 +189,10 @@ views::View* AuthenticatorRequestDialogView::GetInitiallyFocusedView() { ...@@ -189,8 +189,10 @@ views::View* AuthenticatorRequestDialogView::GetInitiallyFocusedView() {
if (ShouldOtherTransportsButtonBeVisible()) if (ShouldOtherTransportsButtonBeVisible())
return other_transports_button_; return other_transports_button_;
if (sheet()->model()->IsCancelButtonVisible()) if (sheet()->model()->IsCancelButtonVisible() &&
sheet()->model()->IsCancelButtonEnabled()) {
return GetCancelButton(); return GetCancelButton();
}
return nullptr; return nullptr;
} }
......
...@@ -48,6 +48,7 @@ class AuthenticatorRequestSheetModel { ...@@ -48,6 +48,7 @@ class AuthenticatorRequestSheetModel {
virtual bool IsBackButtonVisible() const = 0; virtual bool IsBackButtonVisible() const = 0;
virtual bool IsCancelButtonVisible() const = 0; virtual bool IsCancelButtonVisible() const = 0;
virtual bool IsCancelButtonEnabled() const = 0;
virtual base::string16 GetCancelButtonLabel() const = 0; virtual base::string16 GetCancelButtonLabel() const = 0;
virtual bool IsAcceptButtonVisible() const = 0; virtual bool IsAcceptButtonVisible() const = 0;
......
...@@ -77,6 +77,10 @@ bool AuthenticatorSheetModelBase::IsCancelButtonVisible() const { ...@@ -77,6 +77,10 @@ bool AuthenticatorSheetModelBase::IsCancelButtonVisible() const {
return true; return true;
} }
bool AuthenticatorSheetModelBase::IsCancelButtonEnabled() const {
return true;
}
base::string16 AuthenticatorSheetModelBase::GetCancelButtonLabel() const { base::string16 AuthenticatorSheetModelBase::GetCancelButtonLabel() const {
return l10n_util::GetStringUTF16(IDS_CANCEL); return l10n_util::GetStringUTF16(IDS_CANCEL);
} }
......
...@@ -42,6 +42,7 @@ class AuthenticatorSheetModelBase ...@@ -42,6 +42,7 @@ class AuthenticatorSheetModelBase
bool IsActivityIndicatorVisible() const override; bool IsActivityIndicatorVisible() const override;
bool IsBackButtonVisible() const override; bool IsBackButtonVisible() const override;
bool IsCancelButtonVisible() const override; bool IsCancelButtonVisible() const override;
bool IsCancelButtonEnabled() const override;
base::string16 GetCancelButtonLabel() const override; base::string16 GetCancelButtonLabel() const override;
bool IsAcceptButtonVisible() const override; bool IsAcceptButtonVisible() const override;
bool IsAcceptButtonEnabled() const override; bool IsAcceptButtonEnabled() const override;
......
...@@ -330,6 +330,10 @@ class AutofillClient : public RiskDataLoader { ...@@ -330,6 +330,10 @@ class AutofillClient : public RiskDataLoader {
// challenge. // challenge.
virtual void UpdateWebauthnOfferDialogWithError() = 0; virtual void UpdateWebauthnOfferDialogWithError() = 0;
// Will update the cancel button in the WebAuthn verify pending dialog.
virtual void UpdateWebauthnVerifyPendingCancelButton(
bool should_be_enabled) = 0;
// Will close the current visible WebAuthn dialog. Returns true if dialog was // Will close the current visible WebAuthn dialog. Returns true if dialog was
// visible and has been closed. // visible and has been closed.
virtual bool CloseWebauthnDialog() = 0; virtual bool CloseWebauthnDialog() = 0;
......
...@@ -235,6 +235,12 @@ void CreditCardFIDOAuthenticator::GetAssertion( ...@@ -235,6 +235,12 @@ void CreditCardFIDOAuthenticator::GetAssertion(
return; return;
} }
} }
if (current_flow_ == AUTHENTICATION_FLOW) {
// The cancel button in the verify pending dialog should be disabled when
// the OS authentication dialog is visible.
autofill_client_->UpdateWebauthnVerifyPendingCancelButton(
/*should_be_enabled=*/false);
}
#endif #endif
authenticator_->GetAssertion( authenticator_->GetAssertion(
std::move(request_options), std::move(request_options),
...@@ -358,6 +364,12 @@ void CreditCardFIDOAuthenticator::OnDidGetAssertion( ...@@ -358,6 +364,12 @@ void CreditCardFIDOAuthenticator::OnDidGetAssertion(
} }
if (current_flow_ == AUTHENTICATION_FLOW) { if (current_flow_ == AUTHENTICATION_FLOW) {
#if !defined(OS_ANDROID)
// On desktop, enables the cancel button in the verify pending dialog again.
autofill_client_->UpdateWebauthnVerifyPendingCancelButton(
/*should_be_enabled=*/true);
#endif
base::Value response = base::Value response =
ParseAssertionResponse(std::move(assertion_response)); ParseAssertionResponse(std::move(assertion_response));
full_card_request_.reset(new payments::FullCardRequest( full_card_request_.reset(new payments::FullCardRequest(
......
...@@ -116,6 +116,9 @@ void TestAutofillClient::ShowWebauthnVerifyPendingDialog( ...@@ -116,6 +116,9 @@ void TestAutofillClient::ShowWebauthnVerifyPendingDialog(
void TestAutofillClient::UpdateWebauthnOfferDialogWithError() {} void TestAutofillClient::UpdateWebauthnOfferDialogWithError() {}
void TestAutofillClient::UpdateWebauthnVerifyPendingCancelButton(
bool should_be_enabled) {}
bool TestAutofillClient::CloseWebauthnDialog() { bool TestAutofillClient::CloseWebauthnDialog() {
return true; return true;
} }
......
...@@ -71,6 +71,7 @@ class TestAutofillClient : public AutofillClient { ...@@ -71,6 +71,7 @@ class TestAutofillClient : public AutofillClient {
void ShowWebauthnVerifyPendingDialog( void ShowWebauthnVerifyPendingDialog(
WebauthnDialogCallback verify_pending_dialog_callback) override; WebauthnDialogCallback verify_pending_dialog_callback) override;
void UpdateWebauthnOfferDialogWithError() override; void UpdateWebauthnOfferDialogWithError() override;
void UpdateWebauthnVerifyPendingCancelButton(bool should_be_enabled) override;
bool CloseWebauthnDialog() override; bool CloseWebauthnDialog() override;
#endif #endif
void ConfirmSaveAutofillProfile(const AutofillProfile& profile, void ConfirmSaveAutofillProfile(const AutofillProfile& profile,
......
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