[FedCM] Degrade gracefully when client metadata is not provided

Major changes in this patch:
  1. No longer require client metadata endpoint in the manifest
  2. No longer require privacy policy URL in client metadata

We show different UI when they are not provided by IDP.
For 1: do not show "See this site's privacy policy and terms of service"
For 2: if "terms of service" is provided, we show "See this site's terms
of service.". If not, fall back to #1.

Bug: 1324798
Change-Id: Id4f41eb77ee94eade1b2ea58ac6265504462020f
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/3640231
Reviewed-by: Christian Biesinger <cbiesinger@chromium.org>
Reviewed-by: Sam Goto <goto@chromium.org>
Commit-Queue: Yi Gu <yigu@chromium.org>
Cr-Commit-Position: refs/heads/main@{#1002428}

chrome/app/generated_resources.grd

@@ -12931,7 +12931,13 @@ Please help our engineers fix this problem. Tell us what happened right before y
     <message name="IDS_ACCOUNT_SELECTION_CONTINUE" desc="Title of the button that continues filling with the only available set of credentials.">
       Continue as <ph name="NAME">$1<ex>Albus (or Albus Dumbledore)</ex></ph>
     </message>
-    <message name="IDS_ACCOUNT_SELECTION_DATA_SHARING_CONSENT_NO_TOS" desc="The consent text shown to the user before sign up when there are no terms of service.">
+    <message name="IDS_ACCOUNT_SELECTION_DATA_SHARING_CONSENT_NO_PP_OR_TOS" desc="The consent text shown to the user before sign up when there is no privacy policy or terms of service.">
+      To continue, <ph name="IDENTITY_PROVIDER_ETLD_PLUS_ONE">$1<ex>idp.com</ex></ph> will share your name, email address, and profile picture with this site.
+    </message>
+    <message name="IDS_ACCOUNT_SELECTION_DATA_SHARING_CONSENT_NO_PP" desc="The consent text shown to the user before sign up when there is no privacy policy.">
+      To continue, <ph name="IDENTITY_PROVIDER_ETLD_PLUS_ONE">$1<ex>idp.com</ex></ph> will share your name, email address, and profile picture with this site. See this site's <ph name="BEGIN_LINK">$2</ph>terms of service<ph name="END_LINK">$3</ph>.
+    </message>
+    <message name="IDS_ACCOUNT_SELECTION_DATA_SHARING_CONSENT_NO_TOS" desc="The consent text shown to the user before sign up when there is no terms of service.">
       To continue, <ph name="IDENTITY_PROVIDER_ETLD_PLUS_ONE">$1<ex>idp.com</ex></ph> will share your name, email address, and profile picture with this site. See this site's <ph name="BEGIN_LINK">$2</ph>privacy policy<ph name="END_LINK">$3</ph>.
     </message>
     <message name="IDS_ACCOUNT_SELECTION_DATA_SHARING_CONSENT" desc="The consent text shown to the user before sign up.">

chrome/app/generated_resources_grd/IDS_ACCOUNT_SELECTION_DATA_SHARING_CONSENT_NO_PP.png.sha1

@@ -0,0 +1 @@
+a6f400a241e6a4873ae9f5219cf085a37ecf0020

chrome/app/generated_resources_grd/IDS_ACCOUNT_SELECTION_DATA_SHARING_CONSENT_NO_PP_OR_TOS.png.sha1

@@ -0,0 +1 @@
+126b2ce7eae56c480f30de2728ad400e06f3e67a

chrome/browser/ui/android/strings/android_chrome_strings.grd

@@ -5456,7 +5456,13 @@ To change this setting, <ph name="BEGIN_LINK">&lt;resetlink&gt;</ph>reset sync<p
       <message name="IDS_ACCOUNT_SELECTION_SHEET_TITLE_EXPLICIT" desc="Header for sign in sheet. Sheet is shown to prompt user for sign in consent.">
         Sign in to <ph name="SITE_ETLD_PLUS_ONE">%1$s<ex>rp.example</ex></ph> with <ph name="IDENTITY_PROVIDER_ETLD_PLUS_ONE">%2$s<ex>idp.com</ex></ph>
       </message>
-      <message name="IDS_ACCOUNT_SELECTION_DATA_SHARING_CONSENT_NO_TOS" desc="The consent text shown to the user before sign up when there are no terms of service.">
+      <message name="IDS_ACCOUNT_SELECTION_DATA_SHARING_CONSENT_NO_PP_OR_TOS" desc="The consent text shown to the user before sign up when there is no privacy policy or terms of service.">
+      To continue, <ph name="IDENTITY_PROVIDER_ETLD_PLUS_ONE">%1$s<ex>idp.com</ex></ph> will share your name, email address, and profile picture with this site.
+      </message>
+      <message name="IDS_ACCOUNT_SELECTION_DATA_SHARING_CONSENT_NO_PP" desc="The consent text shown to the user before sign up when there is no privacy policy.">
+      To continue, <ph name="IDENTITY_PROVIDER_ETLD_PLUS_ONE">%1$s<ex>idp.com</ex></ph> will share your name, email address, and profile picture with this site. See this site's <ph name="BEGIN_LINK1">&lt;link_terms_of_service&gt;</ph>terms of service<ph name="END_LINK1">&lt;/link_terms_of_service&gt;</ph>.
+      </message>
+      <message name="IDS_ACCOUNT_SELECTION_DATA_SHARING_CONSENT_NO_TOS" desc="The consent text shown to the user before sign up when there is no terms of service.">
       To continue, <ph name="IDENTITY_PROVIDER_ETLD_PLUS_ONE">%1$s<ex>idp.com</ex></ph> will share your name, email address, and profile picture with this site. See this site's <ph name="BEGIN_LINK1">&lt;link_privacy_policy&gt;</ph>privacy policy<ph name="END_LINK1">&lt;/link_privacy_policy&gt;</ph>.
       </message>
       <message name="IDS_ACCOUNT_SELECTION_DATA_SHARING_CONSENT" desc="The consent text shown to the user before sign up.">

chrome/browser/ui/android/strings/android_chrome_strings_grd/IDS_ACCOUNT_SELECTION_DATA_SHARING_CONSENT_NO_PP.png.sha1

@@ -0,0 +1 @@
+27524f3da48a11477746a8b01e1342cf510bae89

chrome/browser/ui/android/strings/android_chrome_strings_grd/IDS_ACCOUNT_SELECTION_DATA_SHARING_CONSENT_NO_PP_OR_TOS.png.sha1

@@ -0,0 +1 @@
+1b1a2782f4033fde537fc88d4e3b0aeabdae2103

chrome/browser/ui/android/webid/internal/java/src/org/chromium/chrome/browser/ui/android/webid/AccountSelectionViewBinder.java

@@ -178,19 +178,23 @@ static void bindDataSharingConsentView(PropertyModel model, View view, PropertyK
             SpanApplier.SpanInfo termsOfServiceSpan =
                     createLink(context, properties.mTermsOfServiceUrl, "link_terms_of_service");
 
-            int consentTextId = termsOfServiceSpan == null
-                    ? R.string.account_selection_data_sharing_consent_no_tos
-                    : R.string.account_selection_data_sharing_consent;
+            int consentTextId;
+            if (privacyPolicySpan == null && termsOfServiceSpan == null) {
+                consentTextId = R.string.account_selection_data_sharing_consent_no_pp_or_tos;
+            } else if (privacyPolicySpan == null) {
+                consentTextId = R.string.account_selection_data_sharing_consent_no_pp;
+            } else if (termsOfServiceSpan == null) {
+                consentTextId = R.string.account_selection_data_sharing_consent_no_tos;
+            } else {
+                consentTextId = R.string.account_selection_data_sharing_consent;
+            }
             String consentText = String.format(
                     context.getString(consentTextId), properties.mFormattedIdpEtldPlusOne);
 
-            // |privacyPolicySpan| cannot be null due to the following:
-            // 1. We check that the privacy URL is valid in
-            // FederatedAuthRequestImpl::OnClientMetadataResponseReceived(), and an empty URL is
-            // invalid.
-            // 2. createLink() only returns null if the provided URL is empty.
             List<SpanApplier.SpanInfo> spans = new ArrayList<>();
-            spans.add(privacyPolicySpan);
+            if (privacyPolicySpan != null) {
+                spans.add(privacyPolicySpan);
+            }
             if (termsOfServiceSpan != null) {
                 spans.add(termsOfServiceSpan);
             }

chrome/browser/ui/views/webid/account_selection_bubble_view.cc

@@ -262,6 +262,35 @@ AccountSelectionBubbleView::CreateSingleAccountChooser(
   disclosure_label->SetDefaultTextStyle(views::style::STYLE_SECONDARY);
 
   std::vector<size_t> offsets;
+
+  if (client_data_.privacy_policy_url.is_empty() &&
+      client_data_.terms_of_service_url.is_empty()) {
+    // Case for both the privacy policy and terms of service URLs are missing.
+    std::u16string disclosure_text = l10n_util::GetStringFUTF16(
+        IDS_ACCOUNT_SELECTION_DATA_SHARING_CONSENT_NO_PP_OR_TOS,
+        {idp_etld_plus_one_});
+    disclosure_label->SetText(disclosure_text);
+    return row;
+  }
+
+  if (client_data_.privacy_policy_url.is_empty()) {
+    // Case for when we only need to add a link for terms of service URL, but
+    // not privacy policy. We use two placeholders for the start and end of
+    // 'terms of service' in order to style that text as a link.
+    std::u16string disclosure_text = l10n_util::GetStringFUTF16(
+        IDS_ACCOUNT_SELECTION_DATA_SHARING_CONSENT_NO_PP,
+        {idp_etld_plus_one_, std::u16string(), std::u16string()}, &offsets);
+    disclosure_label->SetText(disclosure_text);
+    // Add link styling for terms of service url.
+    disclosure_label->AddStyleRange(
+        gfx::Range(offsets[1], offsets[2]),
+        views::StyledLabel::RangeStyleInfo::CreateForLink(
+            base::BindRepeating(&AccountSelectionBubbleView::OnLinkClicked,
+                                weak_ptr_factory_.GetWeakPtr(),
+                                client_data_.terms_of_service_url)));
+    return row;
+  }
+
   if (client_data_.terms_of_service_url.is_empty()) {
     // Case for when we only need to add a link for privacy policy URL, but not
     // terms of service. We use two placeholders for the start and end of

content/browser/webid/federated_auth_request_impl.cc

@@ -757,9 +757,7 @@ void FederatedAuthRequestImpl::OnManifestReady(
     IdentityProviderMetadata idp_metadata) {
   bool is_token_valid = IsEndpointUrlValid(endpoints_.token);
   bool is_accounts_valid = IsEndpointUrlValid(endpoints_.accounts);
-  bool is_client_metadata_valid =
-      IsEndpointUrlValid(endpoints_.client_metadata);
-  if (!is_token_valid || !is_accounts_valid || !is_client_metadata_valid) {
+  if (!is_token_valid || !is_accounts_valid) {
     std::string message =
         "Manifest is missing or has an invalid URL for the following "
         "endpoints:\n";
@@ -769,9 +767,6 @@ void FederatedAuthRequestImpl::OnManifestReady(
     if (!is_accounts_valid) {
       message += "\"accounts_endpoint\"\n";
     }
-    if (!is_client_metadata_valid) {
-      message += "\"client_metadata_endpoint\"\n";
-    }
     render_frame_host_->AddMessageToConsole(
         blink::mojom::ConsoleMessageLevel::kError, message);
     RecordRequestIdTokenStatus(IdTokenStatus::kManifestInvalidResponse,
@@ -910,71 +905,31 @@ void FederatedAuthRequestImpl::OnBrandIconDownloaded(
     idp_metadata.brand_icon = bitmaps[0];
   }
 
-  network_manager_->FetchClientMetadata(
-      endpoints_.client_metadata, client_id_,
-      base::BindOnce(
-          &FederatedAuthRequestImpl::OnClientMetadataResponseReceived,
-          weak_ptr_factory_.GetWeakPtr(), std::move(idp_metadata)));
+  if (IsEndpointUrlValid(endpoints_.client_metadata)) {
+    network_manager_->FetchClientMetadata(
+        endpoints_.client_metadata, client_id_,
+        base::BindOnce(
+            &FederatedAuthRequestImpl::OnClientMetadataResponseReceived,
+            weak_ptr_factory_.GetWeakPtr(), std::move(idp_metadata)));
+  } else {
+    network_manager_->SendAccountsRequest(
+        endpoints_.accounts, client_id_,
+        base::BindOnce(&FederatedAuthRequestImpl::OnAccountsResponseReceived,
+                       weak_ptr_factory_.GetWeakPtr(), idp_metadata));
+  }
 }
 
 void FederatedAuthRequestImpl::OnClientMetadataResponseReceived(
     IdentityProviderMetadata idp_metadata,
     IdpNetworkRequestManager::FetchStatus status,
     IdpNetworkRequestManager::ClientMetadata data) {
-  switch (status) {
-    case IdpNetworkRequestManager::FetchStatus::kHttpNotFoundError: {
-      RecordRequestIdTokenStatus(IdTokenStatus::kClientMetadataHttpNotFound,
-                                 render_frame_host_->GetPageUkmSourceId());
-      CompleteRequest(
-          FederatedAuthRequestResult::kErrorFetchingClientMetadataHttpNotFound,
-          "",
-          /*should_call_callback=*/false);
-      return;
-    }
-    case IdpNetworkRequestManager::FetchStatus::kNoResponseError: {
-      RecordRequestIdTokenStatus(IdTokenStatus::kClientMetadataNoResponse,
-                                 render_frame_host_->GetPageUkmSourceId());
-      CompleteRequest(
-          FederatedAuthRequestResult::kErrorFetchingClientMetadataNoResponse,
-          "",
-          /*should_call_callback=*/false);
-      return;
-    }
-    case IdpNetworkRequestManager::FetchStatus::kInvalidResponseError: {
-      RecordRequestIdTokenStatus(IdTokenStatus::kClientMetadataInvalidResponse,
-                                 render_frame_host_->GetPageUkmSourceId());
-      CompleteRequest(FederatedAuthRequestResult::
-                          kErrorFetchingClientMetadataInvalidResponse,
-                      "",
-                      /*should_call_callback=*/false);
-      return;
-    }
-    case IdpNetworkRequestManager::FetchStatus::kSuccess: {
-      // Since the |privacy_policy_url| is required, consider the result an
-      // invalid response in the case where the parser returns an empty value
-      // for it or an invalid url.
-      GURL pp_url(data.privacy_policy_url);
-      if (!pp_url.is_valid()) {
-        RecordRequestIdTokenStatus(
-            IdTokenStatus::kClientMetadataMissingPrivacyPolicyUrl,
-            render_frame_host_->GetPageUkmSourceId());
-        CompleteRequest(FederatedAuthRequestResult::
-                            kErrorClientMetadataMissingPrivacyPolicyUrl,
-                        "", /*should_call_callback=*/false);
-        return;
-      }
-      client_metadata_ = data;
-
-      network_manager_->SendAccountsRequest(
-          endpoints_.accounts, client_id_,
-          base::BindOnce(&FederatedAuthRequestImpl::OnAccountsResponseReceived,
-                         weak_ptr_factory_.GetWeakPtr(), idp_metadata));
-      return;
-    }
-    case IdpNetworkRequestManager::FetchStatus::kInvalidRequestError: {
-      NOTREACHED();
-    }
-  }
+  // TODO(yigu): Clean up the client metadata related errors for metrics and
+  // console logs.
+  client_metadata_ = data;
+  network_manager_->SendAccountsRequest(
+      endpoints_.accounts, client_id_,
+      base::BindOnce(&FederatedAuthRequestImpl::OnAccountsResponseReceived,
+                     weak_ptr_factory_.GetWeakPtr(), idp_metadata));
 }
 
 void FederatedAuthRequestImpl::DownloadBitmap(

content/browser/webid/federated_auth_request_impl_unittest.cc

@@ -191,11 +191,6 @@ static const RequestExpectations kExpectationSuccess{
     RequestIdTokenStatus::kSuccess, FederatedAuthRequestResult::kSuccess,
     FETCH_ENDPOINT_ALL_REQUEST_ID_TOKEN};
 
-MockClientIdConfiguration BuildClientMetadataErrorResponse(
-    FetchStatus fetch_status) {
-  return {fetch_status, "", ""};
-}
-
 // Helper class for receiving the mojo method callback.
 class AuthRequestCallbackHelper {
  public:
@@ -1045,25 +1040,14 @@ TEST_F(BasicFederatedAuthRequestImplTest, MissingAccountsEndpoint) {
   EXPECT_EQ("Provider's FedCM manifest configuration is invalid.", messages[1]);
 }
 
-// Test that request fails if manifest is missing client metadata endpoint.
+// Test that client metadata endpoint is not required in manifest.
 TEST_F(BasicFederatedAuthRequestImplTest, MissingClientMetadataEndpoint) {
   MockConfiguration configuration = kConfigurationValid;
   configuration.manifest.client_metadata_endpoint = "";
   RequestExpectations expectations = {
-      RequestIdTokenStatus::kError,
-      FederatedAuthRequestResult::kErrorFetchingManifestInvalidResponse,
-      FetchedEndpoint::MANIFEST | FetchedEndpoint::MANIFEST_LIST};
+      RequestIdTokenStatus::kSuccess, FederatedAuthRequestResult::kSuccess,
+      FETCH_ENDPOINT_ALL_REQUEST_ID_TOKEN & ~FetchedEndpoint::CLIENT_METADATA};
   RunAuthTest(kDefaultRequestParameters, expectations, configuration);
-
-  std::vector<std::string> messages =
-      RenderFrameHostTester::For(main_rfh())->GetConsoleMessages();
-  ASSERT_EQ(2U, messages.size());
-  EXPECT_EQ(
-      "Manifest is missing or has an invalid URL for the following "
-      "endpoints:\n"
-      "\"client_metadata_endpoint\"\n",
-      messages[0]);
-  EXPECT_EQ("Provider's FedCM manifest configuration is invalid.", messages[1]);
 }
 
 // Test that request fails if the accounts endpoint is in a different origin
@@ -1102,58 +1086,31 @@ TEST_F(BasicFederatedAuthRequestImplTest, AccountsCannotBeParsed) {
   RunAuthTest(kDefaultRequestParameters, expectations, configuration);
 }
 
-// Test that request fails if client metadata cannot be found.
-TEST_F(BasicFederatedAuthRequestImplTest, ClientMetadataNotFound) {
-  MockConfiguration configuration = kConfigurationValid;
-  configuration.client_metadata =
-      BuildClientMetadataErrorResponse(FetchStatus::kHttpNotFoundError);
-  RequestExpectations expectations = {
-      RequestIdTokenStatus::kError,
-      FederatedAuthRequestResult::kErrorFetchingClientMetadataHttpNotFound,
-      FetchedEndpoint::MANIFEST | FetchedEndpoint::CLIENT_METADATA |
-          FetchedEndpoint::MANIFEST_LIST};
-  RunAuthTest(kDefaultRequestParameters, expectations, configuration);
-}
-
-// Test that request fails if client metadata endpoint returns empty response.
-TEST_F(BasicFederatedAuthRequestImplTest, ClientMetadataEmptyResponse) {
+// Test that privacy policy URL or terms of service is not required in client
+// metadata.
+TEST_F(BasicFederatedAuthRequestImplTest,
+       ClientMetadataNoPrivacyPolicyOrTermsOfServiceUrl) {
   MockConfiguration configuration = kConfigurationValid;
-  configuration.client_metadata =
-      BuildClientMetadataErrorResponse(FetchStatus::kNoResponseError);
-  RequestExpectations expectations = {
-      RequestIdTokenStatus::kError,
-      FederatedAuthRequestResult::kErrorFetchingClientMetadataNoResponse,
-      FetchedEndpoint::MANIFEST | FetchedEndpoint::CLIENT_METADATA |
-          FetchedEndpoint::MANIFEST_LIST};
-  RunAuthTest(kDefaultRequestParameters, expectations, configuration);
+  configuration.client_metadata = kDefaultClientMetadata;
+  configuration.client_metadata.privacy_policy_url = "";
+  configuration.client_metadata.terms_of_service_url = "";
+  RunAuthTest(kDefaultRequestParameters, kExpectationSuccess, configuration);
 }
 
-// Test that request fails if client metadata returns invalid response.
-TEST_F(BasicFederatedAuthRequestImplTest, ClientMetadataInvalidResponse) {
+// Test that privacy policy URL is not required in client metadata.
+TEST_F(BasicFederatedAuthRequestImplTest, ClientMetadataNoPrivacyPolicyUrl) {
   MockConfiguration configuration = kConfigurationValid;
-  configuration.client_metadata =
-      BuildClientMetadataErrorResponse(FetchStatus::kInvalidResponseError);
-  RequestExpectations expectations = {
-      RequestIdTokenStatus::kError,
-      FederatedAuthRequestResult::kErrorFetchingClientMetadataInvalidResponse,
-      FetchedEndpoint::MANIFEST | FetchedEndpoint::CLIENT_METADATA |
-          FetchedEndpoint::MANIFEST_LIST};
-  RunAuthTest(kDefaultRequestParameters, expectations, configuration);
+  configuration.client_metadata = kDefaultClientMetadata;
+  configuration.client_metadata.privacy_policy_url = "";
+  RunAuthTest(kDefaultRequestParameters, kExpectationSuccess, configuration);
 }
 
-// Test that request fails if client metadata does not contain a privacy policy
-// URL.
-TEST_F(BasicFederatedAuthRequestImplTest, ClientMetadataNoPrivacyUrl) {
+// Test that terms of service URL is not required in client metadata.
+TEST_F(BasicFederatedAuthRequestImplTest, ClientMetadataNoTermsOfServiceUrl) {
   MockConfiguration configuration = kConfigurationValid;
   configuration.client_metadata = kDefaultClientMetadata;
-  configuration.client_metadata.privacy_policy_url = "";
   configuration.client_metadata.terms_of_service_url = "";
-  RequestExpectations expectations = {
-      RequestIdTokenStatus::kError,
-      FederatedAuthRequestResult::kErrorClientMetadataMissingPrivacyPolicyUrl,
-      FetchedEndpoint::MANIFEST | FetchedEndpoint::CLIENT_METADATA |
-          FetchedEndpoint::MANIFEST_LIST};
-  RunAuthTest(kDefaultRequestParameters, expectations, configuration);
+  RunAuthTest(kDefaultRequestParameters, kExpectationSuccess, configuration);
 }
 
 // Test that request fails if all of the endpoints in the manifest are invalid.
@@ -1162,8 +1119,6 @@ TEST_F(BasicFederatedAuthRequestImplTest, AllInvalidEndpoints) {
   MockConfiguration configuration = kConfigurationValid;
   configuration.manifest.accounts_endpoint = "https://cross-origin-1.com";
   configuration.manifest.token_endpoint = "";
-  configuration.manifest.client_metadata_endpoint =
-      "https://cross-origin-2.com";
   RequestExpectations expectations = {
       RequestIdTokenStatus::kError,
       FederatedAuthRequestResult::kErrorFetchingManifestInvalidResponse,
@@ -1176,8 +1131,7 @@ TEST_F(BasicFederatedAuthRequestImplTest, AllInvalidEndpoints) {
       "Manifest is missing or has an invalid URL for the following "
       "endpoints:\n"
       "\"id_token_endpoint\"\n"
-      "\"accounts_endpoint\"\n"
-      "\"client_metadata_endpoint\"\n",
+      "\"accounts_endpoint\"\n",
       messages[0]);
   EXPECT_EQ("Provider's FedCM manifest configuration is invalid.", messages[1]);
 }