Skip to content

Commit

Permalink
[Payments Autofill] Refactoring OnDidGetRealPan() to take UnmaskRespo…
Browse files Browse the repository at this point in the history 
…nseDetails struct.

Instead of passing a std::string for the real pan response, this CL is replacing it with a struct. The reason for this is that future projects will have additional data in the response (e.g. tokenized cvc and fido enrollment challenge). The struct will make it easier to append to.

Bug: 949269
Change-Id: I3724ca885871e8f4dbeb9d85b3c01995136b9103
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1719969
Reviewed-by: Tommy Martino <tmartino@chromium.org>
Reviewed-by: Jared Saul <jsaul@google.com>
Commit-Queue: Manas Verma <manasverma@google.com>
Cr-Commit-Position: refs/heads/master@{#684987}
  • Loading branch information
Manas Verma authored and Commit Bot committed Aug 7, 2019
1 parent a85e5cb commit f032fb5
Show file tree
Hide file tree
Showing 11 changed files with 78 additions and 42 deletions.
4 changes: 3 additions & 1 deletion components/autofill/core/browser/autofill_manager_unittest.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -539,7 +539,9 @@ class AutofillManagerTest : public testing::Test {
autofill_manager_->credit_card_access_manager_->cvc_authenticator_
->full_card_request_.get();
DCHECK(full_card_request);
full_card_request->OnDidGetRealPan(result, real_pan);
payments::PaymentsClient::UnmaskResponseDetails response;
full_card_request->OnDidGetRealPan(result,
response.with_real_pan(real_pan));
}

// Convenience method to cast the FullCardRequest into a CardUnmaskDelegate.
Expand Down
3 changes: 2 additions & 1 deletion components/autofill/core/browser/autofill_metrics_unittest.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -400,7 +400,8 @@ void AutofillMetricsTest::OnDidGetRealPan(
->GetOrCreateCVCAuthenticator()
->full_card_request_.get();
DCHECK(full_card_request);
full_card_request->OnDidGetRealPan(result, real_pan);
payments::PaymentsClient::UnmaskResponseDetails response;
full_card_request->OnDidGetRealPan(result, response.with_real_pan(real_pan));
}

void AutofillMetricsTest::RecreateCreditCards(
Expand Down
8 changes: 6 additions & 2 deletions components/autofill/core/browser/payments/credit_card_access_manager_unittest.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -223,7 +223,9 @@ class CreditCardAccessManagerTest : public testing::Test {
if (!full_card_request)
return false;

full_card_request->OnDidGetRealPan(result, real_pan);
payments::PaymentsClient::UnmaskResponseDetails response;
full_card_request->OnDidGetRealPan(result,
response.with_real_pan(real_pan));
return true;
}

Expand All @@ -237,7 +239,9 @@ class CreditCardAccessManagerTest : public testing::Test {
if (!full_card_request)
return false;

full_card_request->OnDidGetRealPan(result, real_pan);
payments::PaymentsClient::UnmaskResponseDetails response;
full_card_request->OnDidGetRealPan(result,
response.with_real_pan(real_pan));
return true;
}

Expand Down
4 changes: 3 additions & 1 deletion components/autofill/core/browser/payments/credit_card_cvc_authenticator_unittest.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -144,7 +144,9 @@ class CreditCardCVCAuthenticatorTest : public testing::Test {
payments::FullCardRequest* full_card_request =
cvc_authenticator_->full_card_request_.get();
DCHECK(full_card_request);
full_card_request->OnDidGetRealPan(result, real_pan);
payments::PaymentsClient::UnmaskResponseDetails response;
full_card_request->OnDidGetRealPan(result,
response.with_real_pan(real_pan));
}

protected:
Expand Down
4 changes: 3 additions & 1 deletion components/autofill/core/browser/payments/credit_card_fido_authenticator_unittest.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -210,7 +210,9 @@ class CreditCardFIDOAuthenticatorTest : public testing::Test {
void GetRealPan(AutofillClient::PaymentsRpcResult result,
const std::string& real_pan) {
DCHECK(fido_authenticator_->full_card_request_);
fido_authenticator_->full_card_request_->OnDidGetRealPan(result, real_pan);
payments::PaymentsClient::UnmaskResponseDetails response;
fido_authenticator_->full_card_request_->OnDidGetRealPan(
result, response.with_real_pan(real_pan));
}

// Mocks an OptChange response from Payments Client.
Expand Down
9 changes: 5 additions & 4 deletions components/autofill/core/browser/payments/full_card_request.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -166,8 +166,9 @@ void FullCardRequest::SendUnmaskCardRequest() {
weak_ptr_factory_.GetWeakPtr()));
}

void FullCardRequest::OnDidGetRealPan(AutofillClient::PaymentsRpcResult result,
const std::string& real_pan) {
void FullCardRequest::OnDidGetRealPan(
AutofillClient::PaymentsRpcResult result,
payments::PaymentsClient::UnmaskResponseDetails& response_details) {
AutofillMetrics::LogRealPanDuration(
AutofillClock::Now() - real_pan_request_timestamp_, result);

Expand All @@ -190,9 +191,9 @@ void FullCardRequest::OnDidGetRealPan(AutofillClient::PaymentsRpcResult result,
}

case AutofillClient::SUCCESS: {
DCHECK(!real_pan.empty());
DCHECK(!response_details.real_pan.empty());
request_->card.set_record_type(CreditCard::FULL_SERVER_CARD);
request_->card.SetNumber(base::UTF8ToUTF16(real_pan));
request_->card.SetNumber(base::UTF8ToUTF16(response_details.real_pan));
request_->card.SetServerStatus(CreditCard::OK);

if (request_->user_response.should_store_pan)
Expand Down
5 changes: 3 additions & 2 deletions components/autofill/core/browser/payments/full_card_request.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -89,8 +89,9 @@ class FullCardRequest final : public CardUnmaskDelegate {
bool IsGettingFullCard() const;

// Called by the payments client when a card has been unmasked.
void OnDidGetRealPan(AutofillClient::PaymentsRpcResult result,
const std::string& real_pan);
void OnDidGetRealPan(
AutofillClient::PaymentsRpcResult result,
payments::PaymentsClient::UnmaskResponseDetails& response_details);

base::TimeTicks form_parsed_timestamp() const {
return form_parsed_timestamp_;
Expand Down
3 changes: 2 additions & 1 deletion components/autofill/core/browser/payments/full_card_request_unittest.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -102,7 +102,8 @@ class FullCardRequestTest : public testing::Test {

void OnDidGetRealPan(AutofillClient::PaymentsRpcResult result,
const std::string& real_pan) {
request_->OnDidGetRealPan(result, real_pan);
payments::PaymentsClient::UnmaskResponseDetails response;
request_->OnDidGetRealPan(result, response.with_real_pan(real_pan));
}

private:
Expand Down
28 changes: 18 additions & 10 deletions components/autofill/core/browser/payments/payments_client.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -325,10 +325,11 @@ class GetUnmaskDetailsRequest : public PaymentsRequest {

class UnmaskCardRequest : public PaymentsRequest {
public:
UnmaskCardRequest(const PaymentsClient::UnmaskRequestDetails& request_details,
const bool full_sync_enabled,
base::OnceCallback<void(AutofillClient::PaymentsRpcResult,
const std::string&)> callback)
UnmaskCardRequest(
const PaymentsClient::UnmaskRequestDetails& request_details,
const bool full_sync_enabled,
base::OnceCallback<void(AutofillClient::PaymentsRpcResult,
PaymentsClient::UnmaskResponseDetails&)> callback)
: request_details_(request_details),
full_sync_enabled_(full_sync_enabled),
callback_(std::move(callback)) {
Expand Down Expand Up @@ -407,22 +408,24 @@ class UnmaskCardRequest : public PaymentsRequest {

void ParseResponse(const base::Value& response) override {
const auto* pan = response.FindStringKey("pan");
real_pan_ = pan ? *pan : std::string();
response_details_.real_pan = pan ? *pan : std::string();
}

bool IsResponseComplete() override { return !real_pan_.empty(); }
bool IsResponseComplete() override {
return !response_details_.real_pan.empty();
}

void RespondToDelegate(AutofillClient::PaymentsRpcResult result) override {
std::move(callback_).Run(result, real_pan_);
std::move(callback_).Run(result, response_details_);
}

private:
PaymentsClient::UnmaskRequestDetails request_details_;
const bool full_sync_enabled_;
base::OnceCallback<void(AutofillClient::PaymentsRpcResult,
const std::string&)>
PaymentsClient::UnmaskResponseDetails&)>
callback_;
std::string real_pan_;
PaymentsClient::UnmaskResponseDetails response_details_;

DISALLOW_COPY_AND_ASSIGN(UnmaskCardRequest);
};
Expand Down Expand Up @@ -942,6 +945,11 @@ PaymentsClient::UnmaskRequestDetails::UnmaskRequestDetails(
}
PaymentsClient::UnmaskRequestDetails::~UnmaskRequestDetails() {}

PaymentsClient::UnmaskResponseDetails::UnmaskResponseDetails() {}
PaymentsClient::UnmaskResponseDetails::UnmaskResponseDetails(
const UnmaskResponseDetails& other) = default;
PaymentsClient::UnmaskResponseDetails::~UnmaskResponseDetails() {}

PaymentsClient::OptChangeRequestDetails::OptChangeRequestDetails() {}
PaymentsClient::OptChangeRequestDetails::OptChangeRequestDetails(
const OptChangeRequestDetails& other) {
Expand Down Expand Up @@ -990,7 +998,7 @@ void PaymentsClient::GetUnmaskDetails(GetUnmaskDetailsCallback callback,
void PaymentsClient::UnmaskCard(
const PaymentsClient::UnmaskRequestDetails& request_details,
base::OnceCallback<void(AutofillClient::PaymentsRpcResult,
const std::string&)> callback) {
PaymentsClient::UnmaskResponseDetails&)> callback) {
IssueRequest(
std::make_unique<UnmaskCardRequest>(
request_details, account_info_getter_->IsSyncFeatureEnabled(),
Expand Down
16 changes: 15 additions & 1 deletion components/autofill/core/browser/payments/payments_client.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -94,6 +94,20 @@ class PaymentsClient {
base::Value fido_assertion_info;
};

// Information retrieved from an UnmaskRequest.
struct UnmaskResponseDetails {
UnmaskResponseDetails();
UnmaskResponseDetails(const UnmaskResponseDetails& other);
~UnmaskResponseDetails();

UnmaskResponseDetails& with_real_pan(std::string r) {
real_pan = r;
return *this;
}

std::string real_pan;
};

// Information required to either opt-in or opt-out a user for FIDO
// Authentication.
struct OptChangeRequestDetails {
Expand Down Expand Up @@ -186,7 +200,7 @@ class PaymentsClient {
// The user has attempted to unmask a card with the given cvc.
void UnmaskCard(const UnmaskRequestDetails& request_details,
base::OnceCallback<void(AutofillClient::PaymentsRpcResult,
const std::string&)> callback);
UnmaskResponseDetails&)> callback);

// Opts-in or opts-out the user to use FIDO authentication for card unmasking
// on this device.
Expand Down
36 changes: 18 additions & 18 deletions components/autofill/core/browser/payments/payments_client_unittest.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -90,7 +90,7 @@ class PaymentsClientTest : public testing::Test {

result_ = AutofillClient::NONE;
server_id_.clear();
real_pan_.clear();
unmask_response_details_ = nullptr;
legal_message_.reset();
has_variations_header_ = false;

Expand Down Expand Up @@ -145,9 +145,9 @@ class PaymentsClientTest : public testing::Test {
}

void OnDidGetRealPan(AutofillClient::PaymentsRpcResult result,
const std::string& real_pan) {
PaymentsClient::UnmaskResponseDetails& response) {
result_ = result;
real_pan_ = real_pan;
unmask_response_details_ = &response;
}

void OnDidGetOptChangeResult(AutofillClient::PaymentsRpcResult result,
Expand Down Expand Up @@ -307,9 +307,9 @@ class PaymentsClientTest : public testing::Test {
AutofillClient::UnmaskDetails* unmask_details_;

std::string server_id_;
std::string real_pan_;
base::Optional<bool> user_is_opted_in_;
base::Value fido_creation_options_;
PaymentsClient::UnmaskResponseDetails* unmask_response_details_ = nullptr;
std::unique_ptr<base::Value> legal_message_;
std::vector<std::pair<int, int>> supported_card_bin_ranges_;
std::vector<MigratableCreditCard> migratable_credit_cards_;
Expand Down Expand Up @@ -393,7 +393,7 @@ TEST_F(PaymentsClientTest, OAuthError) {
identity_test_env_.WaitForAccessTokenRequestIfNecessaryAndRespondWithError(
GoogleServiceAuthError(GoogleServiceAuthError::SERVICE_UNAVAILABLE));
EXPECT_EQ(AutofillClient::PERMANENT_FAILURE, result_);
EXPECT_TRUE(real_pan_.empty());
EXPECT_TRUE(unmask_response_details_->real_pan.empty());
}

TEST_F(PaymentsClientTest,
Expand All @@ -412,15 +412,15 @@ TEST_F(PaymentsClientTest, UnmaskSuccessViaCVC) {
IssueOAuthToken();
ReturnResponse(net::HTTP_OK, "{ \"pan\": \"1234\" }");
EXPECT_EQ(AutofillClient::SUCCESS, result_);
EXPECT_EQ("1234", real_pan_);
EXPECT_EQ("1234", unmask_response_details_->real_pan);
}

TEST_F(PaymentsClientTest, UnmaskSuccessViaFIDO) {
StartUnmasking(CardUnmaskOptions().with_use_fido(true));
IssueOAuthToken();
ReturnResponse(net::HTTP_OK, "{ \"pan\": \"1234\" }");
EXPECT_EQ(AutofillClient::SUCCESS, result_);
EXPECT_EQ("1234", real_pan_);
EXPECT_EQ("1234", unmask_response_details_->real_pan);
}

TEST_F(PaymentsClientTest, UnmaskSuccessAccountFromSyncTest) {
Expand All @@ -429,7 +429,7 @@ TEST_F(PaymentsClientTest, UnmaskSuccessAccountFromSyncTest) {
IssueOAuthToken();
ReturnResponse(net::HTTP_OK, "{ \"pan\": \"1234\" }");
EXPECT_EQ(AutofillClient::SUCCESS, result_);
EXPECT_EQ("1234", real_pan_);
EXPECT_EQ("1234", unmask_response_details_->real_pan);
}

TEST_F(PaymentsClientTest, UnmaskIncludesChromeUserContext) {
Expand Down Expand Up @@ -1169,7 +1169,7 @@ TEST_F(PaymentsClientTest, RetryFailure) {
IssueOAuthToken();
ReturnResponse(net::HTTP_OK, "{ \"error\": { \"code\": \"INTERNAL\" } }");
EXPECT_EQ(AutofillClient::TRY_AGAIN_FAILURE, result_);
EXPECT_EQ("", real_pan_);
EXPECT_EQ("", unmask_response_details_->real_pan);
}

TEST_F(PaymentsClientTest, PermanentFailure) {
Expand All @@ -1178,15 +1178,15 @@ TEST_F(PaymentsClientTest, PermanentFailure) {
ReturnResponse(net::HTTP_OK,
"{ \"error\": { \"code\": \"ANYTHING_ELSE\" } }");
EXPECT_EQ(AutofillClient::PERMANENT_FAILURE, result_);
EXPECT_EQ("", real_pan_);
EXPECT_EQ("", unmask_response_details_->real_pan);
}

TEST_F(PaymentsClientTest, MalformedResponse) {
StartUnmasking(CardUnmaskOptions());
IssueOAuthToken();
ReturnResponse(net::HTTP_OK, "{ \"error_code\": \"WRONG_JSON_FORMAT\" }");
EXPECT_EQ(AutofillClient::PERMANENT_FAILURE, result_);
EXPECT_EQ("", real_pan_);
EXPECT_EQ("", unmask_response_details_->real_pan);
}

TEST_F(PaymentsClientTest, ReauthNeeded) {
Expand All @@ -1196,17 +1196,17 @@ TEST_F(PaymentsClientTest, ReauthNeeded) {
ReturnResponse(net::HTTP_UNAUTHORIZED, "");
// No response yet.
EXPECT_EQ(AutofillClient::NONE, result_);
EXPECT_EQ("", real_pan_);
EXPECT_EQ(nullptr, unmask_response_details_);

// Second HTTP_UNAUTHORIZED causes permanent failure.
IssueOAuthToken();
ReturnResponse(net::HTTP_UNAUTHORIZED, "");
EXPECT_EQ(AutofillClient::PERMANENT_FAILURE, result_);
EXPECT_EQ("", real_pan_);
EXPECT_EQ("", unmask_response_details_->real_pan);
}

result_ = AutofillClient::NONE;
real_pan_.clear();
unmask_response_details_ = nullptr;

{
StartUnmasking(CardUnmaskOptions());
Expand All @@ -1217,13 +1217,13 @@ TEST_F(PaymentsClientTest, ReauthNeeded) {
ReturnResponse(net::HTTP_UNAUTHORIZED, "");
// No response yet.
EXPECT_EQ(AutofillClient::NONE, result_);
EXPECT_EQ("", real_pan_);
EXPECT_EQ(nullptr, unmask_response_details_);

// HTTP_OK after first HTTP_UNAUTHORIZED results in success.
IssueOAuthToken();
ReturnResponse(net::HTTP_OK, "{ \"pan\": \"1234\" }");
EXPECT_EQ(AutofillClient::SUCCESS, result_);
EXPECT_EQ("1234", real_pan_);
EXPECT_EQ("1234", unmask_response_details_->real_pan);
}
}

Expand All @@ -1232,15 +1232,15 @@ TEST_F(PaymentsClientTest, NetworkError) {
IssueOAuthToken();
ReturnResponse(net::HTTP_REQUEST_TIMEOUT, std::string());
EXPECT_EQ(AutofillClient::NETWORK_ERROR, result_);
EXPECT_EQ("", real_pan_);
EXPECT_EQ("", unmask_response_details_->real_pan);
}

TEST_F(PaymentsClientTest, OtherError) {
StartUnmasking(CardUnmaskOptions());
IssueOAuthToken();
ReturnResponse(net::HTTP_FORBIDDEN, std::string());
EXPECT_EQ(AutofillClient::PERMANENT_FAILURE, result_);
EXPECT_EQ("", real_pan_);
EXPECT_EQ("", unmask_response_details_->real_pan);
}

} // namespace payments
Expand Down

0 comments on commit f032fb5

Please sign in to comment.