Skip to content

Commit

Permalink
[3CSC] Show 3CSC view from CardUnmaskPromptController
Browse files Browse the repository at this point in the history
This CL passes the CardUnmaskChallengeOption used for 3CSC challenge authentication to the card_unmask_prompt_controller. The controller will render unique text in the dialog if the CardUnmaskChallengeOption is present (mock: https://screenshot.googleplex.com/9xMNkkrobcXqLzV.png). Specifically, the dialog window text, instruction message, and text field placeholder are different from the original unmask prompt.

All code in this CL is feature protected in backend components, meaning this view will not be shown until #autofill-enable-cvc-for-vcn-yellow-path is enabled and the server begins returning a selected_challege_option which holds value.

Local implementation screenshot (10/17/22): https://screenshot.googleplex.com/7hkhSFi2rrWPfDa.png

Before screenshot (when there is no selected_challege_option passed):
https://screenshot.googleplex.com/BisaMoDqxTnxawP.png

go/vcn-3csc-chrome-design

Bug: 1370329
Change-Id: I760f35de96d43821dac72eac6386272d26463079
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/3958205
Reviewed-by: Florian Leimgruber <fleimgruber@google.com>
Commit-Queue: Alexander Tekle <alexandertekle@google.com>
Reviewed-by: Mohamed Amir Yosef <mamir@chromium.org>
Reviewed-by: Clemens Arbesser <arbesser@google.com>
Reviewed-by: Richard Coles <torne@chromium.org>
Reviewed-by: Vinny Persky <vinnypersky@google.com>
Reviewed-by: Rohit Rao <rohitrao@chromium.org>
Reviewed-by: Siyu An <siyua@chromium.org>
Reviewed-by: Rayan Kanso <rayankans@chromium.org>
Cr-Commit-Position: refs/heads/main@{#1068968}
  • Loading branch information
Alexander Tekle authored and Chromium LUCI CQ committed Nov 9, 2022
1 parent e862c87 commit ea91f9e
Show file tree
Hide file tree
Showing 46 changed files with 337 additions and 100 deletions.
2 changes: 1 addition & 1 deletion android_webview/browser/aw_autofill_client.cc
Original file line number Diff line number Diff line change
Expand Up @@ -131,7 +131,7 @@ void AwAutofillClient::ShowAutofillSettings(bool show_credit_card_settings) {

void AwAutofillClient::ShowUnmaskPrompt(
const autofill::CreditCard& card,
UnmaskCardReason reason,
const autofill::CardUnmaskPromptOptions& card_unmask_prompt_options,
base::WeakPtr<autofill::CardUnmaskDelegate> delegate) {
NOTIMPLEMENTED();
}
Expand Down
4 changes: 2 additions & 2 deletions android_webview/browser/aw_autofill_client.h
Original file line number Diff line number Diff line change
Expand Up @@ -21,11 +21,11 @@ namespace autofill {
class AutocompleteHistoryManager;
class AutofillDriver;
class AutofillPopupDelegate;
class CardUnmaskDelegate;
class CreditCard;
class FormStructure;
class PersonalDataManager;
class StrikeDatabase;
struct CardUnmaskPromptOptions;
} // namespace autofill

namespace content {
Expand Down Expand Up @@ -85,7 +85,7 @@ class AwAutofillClient : public autofill::AutofillClient,
void ShowAutofillSettings(bool show_credit_card_settings) override;
void ShowUnmaskPrompt(
const autofill::CreditCard& card,
UnmaskCardReason reason,
const autofill::CardUnmaskPromptOptions& card_unmask_prompt_options,
base::WeakPtr<autofill::CardUnmaskDelegate> delegate) override;
void OnUnmaskVerificationResult(PaymentsRpcResult result) override;
void ConfirmAccountNameFixFlow(
Expand Down
6 changes: 3 additions & 3 deletions chrome/browser/ui/autofill/chrome_autofill_client.cc
Original file line number Diff line number Diff line change
Expand Up @@ -373,13 +373,13 @@ void ChromeAutofillClient::OnUnmaskOtpVerificationResult(

void ChromeAutofillClient::ShowUnmaskPrompt(
const CreditCard& card,
UnmaskCardReason reason,
base::WeakPtr<CardUnmaskDelegate> delegate) {
const CardUnmaskPromptOptions& card_unmask_prompt_options,
base::WeakPtr<CardUnmaskDelegate> delegate) {
unmask_controller_.ShowPrompt(
base::BindOnce(&CreateCardUnmaskPromptView,
base::Unretained(&unmask_controller_),
base::Unretained(web_contents())),
card, reason, delegate);
card, card_unmask_prompt_options, delegate);
}

// TODO(crbug.com/1220990): Refactor this for both CVC and Biometrics flows.
Expand Down
8 changes: 5 additions & 3 deletions chrome/browser/ui/autofill/chrome_autofill_client.h
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,7 @@
#include "components/autofill/core/browser/logging/log_manager.h"
#include "components/autofill/core/browser/payments/legal_message_line.h"
#include "components/autofill/core/browser/ui/payments/card_unmask_prompt_controller_impl.h"
#include "components/autofill/core/browser/ui/payments/card_unmask_prompt_options.h"
#include "components/signin/public/identity_manager/account_info.h"
#include "content/public/browser/visibility.h"
#include "content/public/browser/web_contents_observer.h"
Expand Down Expand Up @@ -103,9 +104,10 @@ class ChromeAutofillClient
const size_t& otp_length,
base::WeakPtr<OtpUnmaskDelegate> delegate) override;
void OnUnmaskOtpVerificationResult(OtpUnmaskResult unmask_result) override;
void ShowUnmaskPrompt(const CreditCard& card,
UnmaskCardReason reason,
base::WeakPtr<CardUnmaskDelegate> delegate) override;
void ShowUnmaskPrompt(
const CreditCard& card,
const CardUnmaskPromptOptions& card_unmask_prompt_options,
base::WeakPtr<CardUnmaskDelegate> delegate) override;
void OnUnmaskVerificationResult(PaymentsRpcResult result) override;
void ShowUnmaskAuthenticatorSelectionDialog(
const std::vector<CardUnmaskChallengeOption>& challenge_options,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,7 @@
#include "components/autofill/core/browser/autofill_test_utils.h"
#include "components/autofill/core/browser/payments/card_unmask_delegate.h"
#include "components/autofill/core/browser/ui/payments/card_unmask_prompt_controller_impl.h"
#include "components/autofill/core/browser/ui/payments/card_unmask_prompt_options.h"
#include "components/autofill/core/browser/ui/payments/card_unmask_prompt_view.h"
#include "components/user_prefs/user_prefs.h"
#include "content/public/browser/browser_context.h"
Expand Down Expand Up @@ -169,10 +170,14 @@ class CardUnmaskPromptViewBrowserTest : public DialogBrowserTest {
if (name == kExpiryExpired)
card.SetExpirationYear(2016);

CardUnmaskPromptOptions card_unmask_prompt_options =
CardUnmaskPromptOptions(
/*challenge_option=*/
absl::nullopt, AutofillClient::UnmaskCardReason::kAutofill);
controller()->ShowPrompt(base::BindOnce(&CreateCardUnmaskPromptView,
base::Unretained(controller()),
base::Unretained(contents())),
card, AutofillClient::UnmaskCardReason::kAutofill,
card, card_unmask_prompt_options,
delegate()->GetWeakPtr());
// Setting error expectations and confirming the dialogs for some test
// cases.
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -352,7 +352,19 @@ void CardUnmaskPromptViews::InitIfNecessary() {
year_input_->SetVisible(false);
}

std::unique_ptr<views::Textfield> cvc_input = CreateCvcTextfield();
std::unique_ptr<views::Textfield> cvc_input =
std::make_unique<views::Textfield>();
// Only put a placeholder text if there is no challenge option present. A
// challenge option being present indicates we are unmasking a virtual card
// CVC.
if (!controller_->IsChallengeOptionPresent()) {
cvc_input->SetPlaceholderText(
l10n_util::GetStringUTF16(IDS_AUTOFILL_DIALOG_PLACEHOLDER_CVC));
}
cvc_input->SetAccessibleName(l10n_util::GetStringUTF16(
IDS_AUTOFILL_DIALOG_ACCESSIBLE_NAME_SECURITY_CODE));
cvc_input->SetDefaultWidthInChars(8);
cvc_input->SetTextInputType(ui::TextInputType::TEXT_INPUT_TYPE_NUMBER);
cvc_input->set_controller(this);
cvc_input_ = input_row->AddChildView(std::move(cvc_input));

Expand Down
10 changes: 0 additions & 10 deletions chrome/browser/ui/views/autofill/payments/payments_view_util.cc
Original file line number Diff line number Diff line change
Expand Up @@ -33,7 +33,6 @@
#include "ui/views/controls/label.h"
#include "ui/views/controls/separator.h"
#include "ui/views/controls/styled_label.h"
#include "ui/views/controls/textfield/textfield.h"
#include "ui/views/controls/throbber.h"
#include "ui/views/layout/box_layout.h"
#include "ui/views/layout/layout_provider.h"
Expand Down Expand Up @@ -155,15 +154,6 @@ gfx::Size TitleWithIconAndSeparatorView::GetMinimumSize() const {
BEGIN_METADATA(TitleWithIconAndSeparatorView, views::View)
END_METADATA

std::unique_ptr<views::Textfield> CreateCvcTextfield() {
auto textfield = std::make_unique<views::Textfield>();
textfield->SetPlaceholderText(
l10n_util::GetStringUTF16(IDS_AUTOFILL_DIALOG_PLACEHOLDER_CVC));
textfield->SetDefaultWidthInChars(8);
textfield->SetTextInputType(ui::TextInputType::TEXT_INPUT_TYPE_NUMBER);
return textfield;
}

LegalMessageView::LegalMessageView(
const LegalMessageLines& legal_message_lines,
absl::optional<std::u16string> optional_user_email,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -23,7 +23,6 @@

namespace views {
class Label;
class Textfield;
class Throbber;
} // namespace views

Expand Down Expand Up @@ -52,9 +51,6 @@ class TitleWithIconAndSeparatorView : public views::TableLayoutView {
gfx::Size GetMinimumSize() const override;
};

// Creates and returns a small Textfield intended to be used for CVC entry.
std::unique_ptr<views::Textfield> CreateCvcTextfield();

// Defines a view with legal message. This class handles the legal message
// parsing and the links clicking events.
class LegalMessageView : public views::BoxLayoutView {
Expand Down
2 changes: 2 additions & 0 deletions components/autofill/core/browser/BUILD.gn
Original file line number Diff line number Diff line change
Expand Up @@ -405,6 +405,8 @@ static_library("browser") {
"ui/payments/card_unmask_prompt_controller.h",
"ui/payments/card_unmask_prompt_controller_impl.cc",
"ui/payments/card_unmask_prompt_controller_impl.h",
"ui/payments/card_unmask_prompt_options.cc",
"ui/payments/card_unmask_prompt_options.h",
"ui/payments/card_unmask_prompt_view.h",
"ui/payments/payments_bubble_closed_reasons.h",
"ui/payments/virtual_card_enroll_bubble_controller.h",
Expand Down
8 changes: 5 additions & 3 deletions components/autofill/core/browser/autofill_client.h
Original file line number Diff line number Diff line change
Expand Up @@ -69,6 +69,7 @@ class AutofillProfile;
enum class AutofillProgressDialogType;
struct CardUnmaskChallengeOption;
class CardUnmaskDelegate;
struct CardUnmaskPromptOptions;
class CreditCard;
class CreditCardCVCAuthenticator;
enum class CreditCardFetchResult;
Expand Down Expand Up @@ -429,9 +430,10 @@ class AutofillClient : public RiskDataLoader {

// A user has attempted to use a masked card. Prompt them for further
// information to proceed.
virtual void ShowUnmaskPrompt(const CreditCard& card,
UnmaskCardReason reason,
base::WeakPtr<CardUnmaskDelegate> delegate) = 0;
virtual void ShowUnmaskPrompt(
const CreditCard& card,
const CardUnmaskPromptOptions& card_unmask_prompt_options,
base::WeakPtr<CardUnmaskDelegate> delegate) = 0;
virtual void OnUnmaskVerificationResult(PaymentsRpcResult result) = 0;

// Shows a dialog for the user to choose/confirm the authentication
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -103,9 +103,9 @@ void CreditCardCVCAuthenticator::OnFullCardRequestFailed(

void CreditCardCVCAuthenticator::ShowUnmaskPrompt(
const CreditCard& card,
AutofillClient::UnmaskCardReason reason,
const CardUnmaskPromptOptions& card_unmask_prompt_options,
base::WeakPtr<CardUnmaskDelegate> delegate) {
client_->ShowUnmaskPrompt(card, reason, delegate);
client_->ShowUnmaskPrompt(card, card_unmask_prompt_options, delegate);
}

void CreditCardCVCAuthenticator::OnUnmaskVerificationResult(
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,7 @@
#include "components/autofill/core/browser/data_model/credit_card.h"
#include "components/autofill/core/browser/payments/card_unmask_delegate.h"
#include "components/autofill/core/browser/payments/full_card_request.h"
#include "components/autofill/core/browser/ui/payments/card_unmask_prompt_options.h"

namespace autofill {

Expand Down Expand Up @@ -106,9 +107,10 @@ class CreditCardCVCAuthenticator
payments::FullCardRequest::FailureType failure_type) override;

// payments::FullCardRequest::UIDelegate
void ShowUnmaskPrompt(const CreditCard& card,
AutofillClient::UnmaskCardReason reason,
base::WeakPtr<CardUnmaskDelegate> delegate) override;
void ShowUnmaskPrompt(
const CreditCard& card,
const CardUnmaskPromptOptions& card_unmask_prompt_options,
base::WeakPtr<CardUnmaskDelegate> delegate) override;
void OnUnmaskVerificationResult(
AutofillClient::PaymentsRpcResult result) override;
#if BUILDFLAG(IS_ANDROID)
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -152,8 +152,10 @@ void FullCardRequest::GetFullCardImpl(
// If there is a UI delegate, then perform a CVC check.
// Otherwise, continue and use |fido_assertion_info| to unmask.
if (ui_delegate_) {
ui_delegate_->ShowUnmaskPrompt(request_->card, reason,
weak_ptr_factory_.GetWeakPtr());
ui_delegate_->ShowUnmaskPrompt(
request_->card,
CardUnmaskPromptOptions(selected_challenge_option, reason),
weak_ptr_factory_.GetWeakPtr());
}

if (should_unmask_card_) {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,7 @@
#include "components/autofill/core/browser/autofill_client.h"
#include "components/autofill/core/browser/payments/card_unmask_delegate.h"
#include "components/autofill/core/browser/payments/payments_client.h"
#include "components/autofill/core/browser/ui/payments/card_unmask_prompt_options.h"

namespace autofill {

Expand Down Expand Up @@ -80,7 +81,7 @@ class FullCardRequest final : public CardUnmaskDelegate {
virtual ~UIDelegate() = default;
virtual void ShowUnmaskPrompt(
const CreditCard& card,
AutofillClient::UnmaskCardReason reason,
const CardUnmaskPromptOptions& card_unmask_prompt_options,
base::WeakPtr<CardUnmaskDelegate> delegate) = 0;
virtual void OnUnmaskVerificationResult(
AutofillClient::PaymentsRpcResult result) = 0;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,7 @@
#include "components/autofill/core/browser/test_autofill_client.h"
#include "components/autofill/core/browser/test_autofill_driver.h"
#include "components/autofill/core/browser/test_personal_data_manager.h"
#include "components/autofill/core/browser/ui/payments/card_unmask_prompt_options.h"
#include "components/autofill/core/common/autofill_clock.h"
#include "components/autofill/core/common/autofill_payments_features.h"
#include "components/variations/scoped_variations_ids_provider.h"
Expand Down Expand Up @@ -58,7 +59,7 @@ class MockUIDelegate : public FullCardRequest::UIDelegate,
MOCK_METHOD(void,
ShowUnmaskPrompt,
(const CreditCard&,
AutofillClient::UnmaskCardReason,
const CardUnmaskPromptOptions&,
base::WeakPtr<CardUnmaskDelegate>),
(override));
MOCK_METHOD(void,
Expand Down Expand Up @@ -402,7 +403,7 @@ TEST_F(FullCardRequestTest,
request()->GetFullVirtualCardViaCVC(
card, AutofillClient::UnmaskCardReason::kAutofill,
result_delegate()->AsWeakPtr(), ui_delegate()->AsWeakPtr(),
GURL("https://example.com/"), "test_vcn_context_token",
GURL("https://example.com/"), "test_context_token",
CardUnmaskChallengeOption{.id = "test_challenge_option_id",
.type = CardUnmaskChallengeOptionType::kCvc});
ASSERT_TRUE(request()->GetShouldUnmaskCardForTesting());
Expand All @@ -412,7 +413,7 @@ TEST_F(FullCardRequestTest,
CardUnmaskChallengeOptionType::kCvc);
EXPECT_EQ(request_details->selected_challenge_option->id,
"test_challenge_option_id");
EXPECT_EQ(request_details->context_token, "test_vcn_context_token");
EXPECT_EQ(request_details->context_token, "test_context_token");
EXPECT_EQ(request_details->last_committed_primary_main_frame_origin->spec(),
GURL("https://example.com/").spec());

Expand Down Expand Up @@ -539,7 +540,7 @@ TEST_F(FullCardRequestTest, VcnRetrievalTemporaryFailure) {
request()->GetFullVirtualCardViaCVC(
card, AutofillClient::UnmaskCardReason::kAutofill,
result_delegate()->AsWeakPtr(), ui_delegate()->AsWeakPtr(),
GURL("https://example.com/"), "test_vcn_context_token",
GURL("https://example.com/"), "test_context_token",
CardUnmaskChallengeOption{.type = CardUnmaskChallengeOptionType::kCvc});
CardUnmaskDelegate::UserProvidedUnmaskDetails details;
details.cvc = u"123";
Expand Down Expand Up @@ -569,7 +570,7 @@ TEST_F(FullCardRequestTest, VcnRetrievalPermanentFailure) {
request()->GetFullVirtualCardViaCVC(
card, AutofillClient::UnmaskCardReason::kAutofill,
result_delegate()->AsWeakPtr(), ui_delegate()->AsWeakPtr(),
GURL("https://example.com/"), "test_vcn_context_token",
GURL("https://example.com/"), "test_context_token",
CardUnmaskChallengeOption{.type = CardUnmaskChallengeOptionType::kCvc});
CardUnmaskDelegate::UserProvidedUnmaskDetails details;
details.cvc = u"123";
Expand Down Expand Up @@ -626,7 +627,7 @@ TEST_F(FullCardRequestTest, TryAgainFailureGiveUp) {

// If the server provides an empty PAN with TRY_AGAIN_FAILURE, the user can
// correct their mistake and resubmit.
TEST_F(FullCardRequestTest, TryAgainFailureRetry) {
TEST_F(FullCardRequestTest, ServerCardTryAgainFailure) {
EXPECT_CALL(*result_delegate(), OnFullCardRequestFailed(_)).Times(0);
EXPECT_CALL(*result_delegate(),
OnFullCardRequestSucceeded(
Expand Down Expand Up @@ -656,7 +657,7 @@ TEST_F(FullCardRequestTest, TryAgainFailureRetry) {

// If the server provides an empty PAN with TRY_AGAIN_FAILURE for virtual card,
// ensure it is handled the same way as a regular try again case.
TEST_F(FullCardRequestTest, VcnTryAgainFailure) {
TEST_F(FullCardRequestTest, VirtualCardTryAgainFailure) {
EXPECT_CALL(*ui_delegate(), ShowUnmaskPrompt(_, _, _)).Times(1);
EXPECT_CALL(*ui_delegate(),
OnUnmaskVerificationResult(
Expand All @@ -669,7 +670,7 @@ TEST_F(FullCardRequestTest, VcnTryAgainFailure) {
request()->GetFullVirtualCardViaCVC(
virtual_card, AutofillClient::UnmaskCardReason::kAutofill,
result_delegate()->AsWeakPtr(), ui_delegate()->AsWeakPtr(),
GURL("https://example.com/"), "test_vcn_context_token",
GURL("https://example.com/"), "test_context_token",
CardUnmaskChallengeOption{.id = "test_challenge_option_id",
.type = CardUnmaskChallengeOptionType::kCvc});
CardUnmaskDelegate::UserProvidedUnmaskDetails user_provided_details;
Expand All @@ -678,11 +679,11 @@ TEST_F(FullCardRequestTest, VcnTryAgainFailure) {
PaymentsClient::UnmaskResponseDetails response_details;
response_details.card_type =
AutofillClient::PaymentsRpcCardType::kVirtualCard;
response_details.context_token = "test_vcn_context_token";
response_details.context_token = "test_context_token";
request()->OnDidGetRealPan(
AutofillClient::PaymentsRpcResult::kTryAgainFailure, response_details);
EXPECT_EQ(request()->GetUnmaskRequestDetailsForTesting()->context_token,
"test_vcn_context_token");
"test_context_token");
EXPECT_EQ(request()
->GetUnmaskRequestDetailsForTesting()
->selected_challenge_option->id,
Expand Down
6 changes: 3 additions & 3 deletions components/autofill/core/browser/test_autofill_client.cc
Original file line number Diff line number Diff line change
Expand Up @@ -154,9 +154,9 @@ TestAutofillClient::CreateCreditCardInternalAuthenticator(
void TestAutofillClient::ShowAutofillSettings(bool show_credit_card_settings) {}

void TestAutofillClient::ShowUnmaskPrompt(
const CreditCard& card,
UnmaskCardReason reason,
base::WeakPtr<CardUnmaskDelegate> delegate) {}
const autofill::CreditCard& card,
const autofill::CardUnmaskPromptOptions& card_unmask_prompt_options,
base::WeakPtr<autofill::CardUnmaskDelegate> delegate) {}

void TestAutofillClient::OnUnmaskVerificationResult(PaymentsRpcResult result) {}

Expand Down
8 changes: 5 additions & 3 deletions components/autofill/core/browser/test_autofill_client.h
Original file line number Diff line number Diff line change
Expand Up @@ -31,6 +31,7 @@
#include "components/autofill/core/browser/test_address_normalizer.h"
#include "components/autofill/core/browser/test_form_data_importer.h"
#include "components/autofill/core/browser/test_personal_data_manager.h"
#include "components/autofill/core/browser/ui/payments/card_unmask_prompt_options.h"
#include "components/prefs/pref_service.h"
#include "components/signin/public/identity_manager/identity_test_environment.h"
#include "components/translate/core/browser/language_state.h"
Expand Down Expand Up @@ -90,9 +91,10 @@ class TestAutofillClient : public AutofillClient {
#endif

void ShowAutofillSettings(bool show_credit_card_settings) override;
void ShowUnmaskPrompt(const CreditCard& card,
UnmaskCardReason reason,
base::WeakPtr<CardUnmaskDelegate> delegate) override;
void ShowUnmaskPrompt(
const autofill::CreditCard& card,
const autofill::CardUnmaskPromptOptions& card_unmask_prompt_options,
base::WeakPtr<autofill::CardUnmaskDelegate> delegate) override;
void OnUnmaskVerificationResult(PaymentsRpcResult result) override;
VirtualCardEnrollmentManager* GetVirtualCardEnrollmentManager() override;
void ShowVirtualCardEnrollDialog(
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -47,6 +47,7 @@ class CardUnmaskPromptController {
virtual bool InputExpirationIsValid(const std::u16string& month,
const std::u16string& year) const = 0;
virtual int GetExpectedCvcLength() const = 0;
virtual bool IsChallengeOptionPresent() const = 0;
};

} // namespace autofill
Expand Down

0 comments on commit ea91f9e

Please sign in to comment.