Skip to content

Commit

Permalink
[FedCM] Add error API error dialog type metrics
Browse files Browse the repository at this point in the history 
Records the type of error dialog being shown.

UKM collection review
https://docs.google.com/document/d/1mXcanchRk6BUDIopsz7AMyLtQV4gNTPEg00ZRz7n7xk/edit?usp=sharing

Bug: 1478837
Change-Id: Idbd6eef63b382f7828981361b215cd4e1ca02537
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/4983385
Reviewed-by: Jesse Doherty <jwd@chromium.org>
Commit-Queue: Zachary Tan <tanzachary@chromium.org>
Reviewed-by: Yi Gu <yigu@chromium.org>
Cr-Commit-Position: refs/heads/main@{#1217163}
  • Loading branch information
@tttzach
tttzach authored and Chromium LUCI CQ committed Oct 30, 2023
1 parent f6b3eff commit 1b38e66
Show file tree
Hide file tree
Showing 15 changed files with 324 additions and 37 deletions.
19 changes: 19 additions & 0 deletions content/browser/webid/fedcm_metrics.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -425,6 +425,25 @@ void RecordPreventSilentAccess(RenderFrameHost& rfh,
ukm_builder.Record(ukm::UkmRecorder::Get());
}

void FedCmMetrics::RecordErrorDialogType(
IdpNetworkRequestManager::FedCmErrorDialogType type) {
if (is_disabled_) {
return;
}
auto RecordUkm = [&](auto& ukm_builder) {
ukm_builder.SetError_ErrorDialogType(static_cast<int>(type));
ukm_builder.SetFedCmSessionID(session_id_);
ukm_builder.Record(ukm::UkmRecorder::Get());
};
ukm::builders::Blink_FedCm fedcm_builder(page_source_id_);
RecordUkm(fedcm_builder);

ukm::builders::Blink_FedCmIdp fedcm_idp_builder(provider_source_id_);
RecordUkm(fedcm_idp_builder);

base::UmaHistogramEnumeration("Blink.FedCm.Error.ErrorDialogType", type);
}

void RecordApprovedClientsExistence(bool has_approved_clients) {
if (IsFedCmMultipleIdentityProvidersEnabled())
return;
Expand Down
4 changes: 4 additions & 0 deletions content/browser/webid/fedcm_metrics.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -247,6 +247,10 @@ class CONTENT_EXPORT FedCmMetrics {
// Records the status of the |Revoke| call.
void RecordRevokeStatus(FedCmRevokeStatus status);

// Records the type of error dialog shown.
void RecordErrorDialogType(
IdpNetworkRequestManager::FedCmErrorDialogType type);

private:
// The page's SourceId. Used to log the UKM event Blink.FedCm.
ukm::SourceId page_source_id_;
Expand Down
28 changes: 28 additions & 0 deletions content/browser/webid/federated_auth_request_impl.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -61,6 +61,7 @@ using FederatedApiPermissionStatus =
using RevokeStatusForMetrics = content::FedCmRevokeStatus;
using TokenStatus = content::FedCmRequestIdTokenStatus;
using SignInStateMatchStatus = content::FedCmSignInStateMatchStatus;
using ErrorDialogType = content::IdpNetworkRequestManager::FedCmErrorDialogType;
using LoginState = content::IdentityRequestAccount::LoginState;
using SignInMode = content::IdentityRequestAccount::SignInMode;
using CompleteRequestWithErrorCallback =
Expand Down Expand Up @@ -1805,6 +1806,9 @@ void FederatedAuthRequestImpl::OnAccountSelected(const GURL& idp_config_url,
weak_ptr_factory_.GetWeakPtr(),
idp_info.provider->Clone()),
base::BindOnce(&FederatedAuthRequestImpl::OnContinueOnResponseReceived,
weak_ptr_factory_.GetWeakPtr(),
idp_info.provider->Clone()),
base::BindOnce(&FederatedAuthRequestImpl::RecordErrorMetrics,
weak_ptr_factory_.GetWeakPtr(),
idp_info.provider->Clone()));
}
Expand Down Expand Up @@ -2704,4 +2708,28 @@ void FederatedAuthRequestImpl::Revoke(
api_permission_delegate_.get());
}

void FederatedAuthRequestImpl::RecordErrorMetrics(
IdentityProviderRequestOptionsPtr idp,
absl::optional<ErrorDialogType> error_dialog_type) {
if (!IsFedCmErrorEnabled()) {
return;
}

if (!fedcm_metrics_) {
// Ensure the lifecycle state as GetPageUkmSourceId doesn't support the
// prerendering page. As FederatedAuthRequest runs behind the
// BrowserInterfaceBinders, the service doesn't receive any request while
// prerendering, and the CHECK should always meet the condition.
CHECK(!render_frame_host().IsInLifecycleState(
RenderFrameHost::LifecycleState::kPrerendering));
fedcm_metrics_ = CreateFedCmMetrics(
idp->config->config_url, render_frame_host().GetPageUkmSourceId(),
/*is_disabled=*/false);
}

if (error_dialog_type) {
fedcm_metrics_->RecordErrorDialogType(*error_dialog_type);
}
}

} // namespace content
5 changes: 5 additions & 0 deletions content/browser/webid/federated_auth_request_impl.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -353,6 +353,11 @@ class CONTENT_EXPORT FederatedAuthRequestImpl
void CompleteRevokeRequest(RevokeCallback callback,
blink::mojom::RevokeStatus status);

void RecordErrorMetrics(
blink::mojom::IdentityProviderRequestOptionsPtr idp,
absl::optional<IdpNetworkRequestManager::FedCmErrorDialogType>
error_dialog_type);

std::unique_ptr<IdpNetworkRequestManager> network_manager_;
std::unique_ptr<IdentityRequestDialogController> request_dialog_controller_;

Expand Down
12 changes: 7 additions & 5 deletions content/browser/webid/federated_auth_request_impl_multiple_frames_unittest.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -118,11 +118,13 @@ class TestIdpNetworkRequestManager : public MockIdpNetworkRequestManager {
base::BindOnce(std::move(callback), kFetchStatusSuccess, kAccounts));
}

void SendTokenRequest(const GURL& token_url,
const std::string& account,
const std::string& url_encoded_post_data,
TokenRequestCallback callback,
ContinueOnCallback continue_on) override {
void SendTokenRequest(
const GURL& token_url,
const std::string& account,
const std::string& url_encoded_post_data,
TokenRequestCallback callback,
ContinueOnCallback continue_on,
RecordErrorMetricsCallback record_error_metrics_callback) override {
TokenResult result;
result.token = kToken;
base::SequencedTaskRunner::GetCurrentDefault()->PostTask(
Expand Down
97 changes: 85 additions & 12 deletions content/browser/webid/federated_auth_request_impl_unittest.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -61,6 +61,7 @@ using TokenStatus = content::FedCmRequestIdTokenStatus;
using LoginState = content::IdentityRequestAccount::LoginState;
using SignInMode = content::IdentityRequestAccount::SignInMode;
using SignInStateMatchStatus = content::FedCmSignInStateMatchStatus;
using ErrorDialogType = content::IdpNetworkRequestManager::FedCmErrorDialogType;
using ::testing::_;
using ::testing::ElementsAre;
using ::testing::Eq;
Expand Down Expand Up @@ -294,6 +295,7 @@ struct MockConfiguration {
absl::optional<GURL> continue_on;
MediationRequirement mediation_requirement = MediationRequirement::kOptional;
absl::optional<TokenError> token_error;
absl::optional<ErrorDialogType> error_dialog_type;
};

static const MockClientIdConfiguration kDefaultClientMetadata{
Expand Down Expand Up @@ -487,13 +489,22 @@ class TestIdpNetworkRequestManager : public MockIdpNetworkRequestManager {
info.accounts));
}

void SendTokenRequest(const GURL& token_url,
const std::string& account,
const std::string& url_encoded_post_data,
TokenRequestCallback callback,
ContinueOnCallback on_continue) override {
void SendTokenRequest(
const GURL& token_url,
const std::string& account,
const std::string& url_encoded_post_data,
TokenRequestCallback callback,
ContinueOnCallback on_continue,
RecordErrorMetricsCallback record_error_metrics_callback) override {
++num_fetched_[FetchedEndpoint::TOKEN];

if (config_.error_dialog_type) {
base::OnceCallback bound_record_error_metrics_callback = base::BindOnce(
std::move(record_error_metrics_callback), config_.error_dialog_type);
base::SequencedTaskRunner::GetCurrentDefault()->PostTask(
FROM_HERE, std::move(bound_record_error_metrics_callback));
}

if (config_.token_error) {
TokenResult result;
result.error = config_.token_error;
Expand Down Expand Up @@ -575,18 +586,20 @@ class IdpNetworkRequestManagerParamChecker
std::move(callback));
}

void SendTokenRequest(const GURL& token_url,
const std::string& account,
const std::string& url_encoded_post_data,
TokenRequestCallback callback,
ContinueOnCallback on_continue) override {
void SendTokenRequest(
const GURL& token_url,
const std::string& account,
const std::string& url_encoded_post_data,
TokenRequestCallback callback,
ContinueOnCallback on_continue,
RecordErrorMetricsCallback record_error_metrics_callback) override {
if (expected_selected_account_id_)
EXPECT_EQ(expected_selected_account_id_, account);
if (expected_url_encoded_post_data_)
EXPECT_EQ(expected_url_encoded_post_data_, url_encoded_post_data);
TestIdpNetworkRequestManager::SendTokenRequest(
token_url, account, url_encoded_post_data, std::move(callback),
std::move(on_continue));
std::move(on_continue), std::move(record_error_metrics_callback));
}

private:
Expand Down Expand Up @@ -5090,8 +5103,11 @@ TEST_F(FederatedAuthRequestImplTest, InvalidResponseErrorDialogShown) {
list.InitAndEnableFeature(features::kFedCmError);

MockConfiguration configuration = kConfigurationValid;
ErrorDialogType error_dialog_type = ErrorDialogType::kGenericEmptyWithoutUrl;
configuration.token_response.parse_status =
ParseStatus::kInvalidResponseError;
configuration.error_dialog_type = error_dialog_type;

RequestExpectations expectations = {
RequestTokenStatus::kError,
FederatedAuthRequestResult::kErrorFetchingIdTokenInvalidResponse,
Expand All @@ -5101,6 +5117,12 @@ TEST_F(FederatedAuthRequestImplTest, InvalidResponseErrorDialogShown) {

EXPECT_TRUE(DidFetch(FetchedEndpoint::TOKEN));
EXPECT_TRUE(dialog_controller_state_.did_show_error_dialog);

histogram_tester_.ExpectUniqueSample("Blink.FedCm.Error.ErrorDialogType",
error_dialog_type, 1);

ExpectUKMPresence("Error.ErrorDialogType");
CheckAllFedCmSessionIDs();
}

// Test that an error dialog is not shown when the token response is invalid but
Expand All @@ -5112,6 +5134,7 @@ TEST_F(FederatedAuthRequestImplTest, InvalidResponseErrorDialogDisabled) {
MockConfiguration configuration = kConfigurationValid;
configuration.token_response.parse_status =
ParseStatus::kInvalidResponseError;

RequestExpectations expectations = {
RequestTokenStatus::kError,
FederatedAuthRequestResult::kErrorFetchingIdTokenInvalidResponse,
Expand All @@ -5121,6 +5144,11 @@ TEST_F(FederatedAuthRequestImplTest, InvalidResponseErrorDialogDisabled) {

EXPECT_TRUE(DidFetch(FetchedEndpoint::TOKEN));
EXPECT_FALSE(dialog_controller_state_.did_show_error_dialog);

histogram_tester_.ExpectTotalCount("Blink.FedCm.Error.ErrorDialogType", 0);

ExpectNoUKMPresence("Error.ErrorDialogType");
CheckAllFedCmSessionIDs();
}

// Test that an error dialog is shown when the token response is missing.
Expand All @@ -5129,7 +5157,10 @@ TEST_F(FederatedAuthRequestImplTest, NoResponseErrorDialogShown) {
list.InitAndEnableFeature(features::kFedCmError);

MockConfiguration configuration = kConfigurationValid;
ErrorDialogType error_dialog_type = ErrorDialogType::kGenericEmptyWithoutUrl;
configuration.token_response.parse_status = ParseStatus::kNoResponseError;
configuration.error_dialog_type = error_dialog_type;

RequestExpectations expectations = {
RequestTokenStatus::kError,
FederatedAuthRequestResult::kErrorFetchingIdTokenNoResponse,
Expand All @@ -5139,6 +5170,12 @@ TEST_F(FederatedAuthRequestImplTest, NoResponseErrorDialogShown) {

EXPECT_TRUE(DidFetch(FetchedEndpoint::TOKEN));
EXPECT_TRUE(dialog_controller_state_.did_show_error_dialog);

histogram_tester_.ExpectUniqueSample("Blink.FedCm.Error.ErrorDialogType",
error_dialog_type, 1);

ExpectUKMPresence("Error.ErrorDialogType");
CheckAllFedCmSessionIDs();
}

// Test that the error UI has proper url set.
Expand All @@ -5147,8 +5184,10 @@ TEST_F(FederatedAuthRequestImplTest, ErrorUrlDisplayedWithProperUrl) {
list.InitAndEnableFeature(features::kFedCmError);

MockConfiguration configuration = kConfigurationValid;
ErrorDialogType error_dialog_type = ErrorDialogType::kGenericEmptyWithUrl;
configuration.token_error =
TokenError(/*token=*/"", GURL("https://foo.idp.example/error"));
TokenError(/*code=*/"", GURL("https://foo.idp.example/error"));
configuration.error_dialog_type = error_dialog_type;

RequestExpectations expectations = {
RequestTokenStatus::kError,
Expand All @@ -5161,6 +5200,12 @@ TEST_F(FederatedAuthRequestImplTest, ErrorUrlDisplayedWithProperUrl) {
EXPECT_TRUE(dialog_controller_state_.did_show_error_dialog);
EXPECT_EQ(dialog_controller_state_.token_error->url,
GURL("https://foo.idp.example/error"));

histogram_tester_.ExpectUniqueSample("Blink.FedCm.Error.ErrorDialogType",
error_dialog_type, 1);

ExpectUKMPresence("Error.ErrorDialogType");
CheckAllFedCmSessionIDs();
}

// Test that permission is embargoed upon closing a mismatch dialog.
Expand Down Expand Up @@ -5211,4 +5256,32 @@ TEST_F(FederatedAuthRequestImplTest, IdpSigninStatusClosePopupEmbargo) {
EXPECT_TRUE(test_api_permission_delegate_->embargoed_origins_.empty());
}

// Test that error dialog type metrics are recorded.
TEST_F(FederatedAuthRequestImplTest, ErrorDialogTypeMetrics) {
base::test::ScopedFeatureList list;
list.InitAndEnableFeature(features::kFedCmError);

MockConfiguration configuration = kConfigurationValid;
ErrorDialogType error_dialog_type = ErrorDialogType::kInvalidRequestWithUrl;
configuration.token_error = TokenError(/*code=*/"invalid_request",
GURL("https://foo.idp.example/error"));
configuration.error_dialog_type = error_dialog_type;

RequestExpectations expectations = {
RequestTokenStatus::kError,
FederatedAuthRequestResult::kErrorFetchingIdTokenInvalidResponse,
/*standalone_console_message=*/absl::nullopt,
/*selected_idp_config_url=*/absl::nullopt};
RunAuthTest(kDefaultRequestParameters, expectations, configuration);

EXPECT_TRUE(DidFetch(FetchedEndpoint::TOKEN));
EXPECT_TRUE(dialog_controller_state_.did_show_error_dialog);

histogram_tester_.ExpectUniqueSample("Blink.FedCm.Error.ErrorDialogType",
error_dialog_type, 1);

ExpectUKMPresence("Error.ErrorDialogType");
CheckAllFedCmSessionIDs();
}

} // namespace content

0 comments on commit 1b38e66

Please sign in to comment.