Commit 924bccc5 authored by Xing Liu's avatar Xing Liu Committed by Commit Bot

Revert "[webauthn] Don't adjust timeout when testing"

This reverts commit 60d219b6.

Reason for revert: breaks android downstream.
https://logs.chromium.org/logs/chrome/buildbucket/cr-buildbucket.appspot.com/8886827242049965872/+/steps/compile/0/stdout


Original change's description:
> [webauthn] Don't adjust timeout when testing
> 
> Move the timeout adjustment from blink to the browser and avoid
> adjusting it if we detect a discovery factory override is in place.
> 
> This change makes authenticator browser tests and WPTs 3X faster.
> 
> Fixed: 976428
> Change-Id: Iaea88f356a6d090c14474151b29c04e912c471b8
> Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2079043
> Commit-Queue: Nina Satragno <nsatragno@chromium.org>
> Reviewed-by: Ken Buchanan <kenrb@chromium.org>
> Reviewed-by: Jared Saul <jsaul@google.com>
> Cr-Commit-Position: refs/heads/master@{#746432}

TBR=kenrb@chromium.org,nsatragno@chromium.org,jsaul@google.com

Change-Id: Iab40100ead5acdbc6dc631461470314f9eafc815
No-Presubmit: true
No-Tree-Checks: true
No-Try: true
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2083564Reviewed-by: default avatarXing Liu <xingliu@chromium.org>
Commit-Queue: Xing Liu <xingliu@chromium.org>
Cr-Commit-Position: refs/heads/master@{#746445}
parent dc7e5427
...@@ -528,7 +528,7 @@ CreditCardFIDOAuthenticator::ParseRequestOptions( ...@@ -528,7 +528,7 @@ CreditCardFIDOAuthenticator::ParseRequestOptions(
const auto* timeout = request_options.FindKeyOfType( const auto* timeout = request_options.FindKeyOfType(
"timeout_millis", base::Value::Type::INTEGER); "timeout_millis", base::Value::Type::INTEGER);
options->timeout = base::TimeDelta::FromMilliseconds( options->adjusted_timeout = base::TimeDelta::FromMilliseconds(
timeout ? timeout->GetInt() : kWebAuthnTimeoutMs); timeout ? timeout->GetInt() : kWebAuthnTimeoutMs);
options->user_verification = UserVerificationRequirement::kRequired; options->user_verification = UserVerificationRequirement::kRequired;
...@@ -595,7 +595,7 @@ CreditCardFIDOAuthenticator::ParseCreationOptions( ...@@ -595,7 +595,7 @@ CreditCardFIDOAuthenticator::ParseCreationOptions(
const auto* timeout = creation_options.FindKeyOfType( const auto* timeout = creation_options.FindKeyOfType(
"timeout_millis", base::Value::Type::INTEGER); "timeout_millis", base::Value::Type::INTEGER);
options->timeout = base::TimeDelta::FromMilliseconds( options->adjusted_timeout = base::TimeDelta::FromMilliseconds(
timeout ? timeout->GetInt() : kWebAuthnTimeoutMs); timeout ? timeout->GetInt() : kWebAuthnTimeoutMs);
const auto* attestation = const auto* attestation =
......
...@@ -276,30 +276,6 @@ enum class AttestationErasureOption { ...@@ -276,30 +276,6 @@ enum class AttestationErasureOption {
kEraseAttestationAndAaguid, kEraseAttestationAndAaguid,
}; };
base::TimeDelta AdjustTimeout(base::Optional<base::TimeDelta> timeout,
RenderFrameHost* render_frame_host) {
// Time to wait for an authenticator to successfully complete an operation.
static constexpr base::TimeDelta kAdjustedTimeoutLower =
base::TimeDelta::FromSeconds(10);
static constexpr base::TimeDelta kAdjustedTimeoutUpper =
base::TimeDelta::FromMinutes(10);
if (!timeout)
return kAdjustedTimeoutUpper;
bool testing_api_enabled =
AuthenticatorEnvironmentImpl::GetInstance()->GetDiscoveryFactoryOverride(
static_cast<RenderFrameHostImpl*>(render_frame_host)
->frame_tree_node());
if (testing_api_enabled) {
return *timeout;
}
return std::max(kAdjustedTimeoutLower,
std::min(kAdjustedTimeoutUpper, *timeout));
}
blink::mojom::MakeCredentialAuthenticatorResponsePtr blink::mojom::MakeCredentialAuthenticatorResponsePtr
CreateMakeCredentialResponse( CreateMakeCredentialResponse(
const std::string& client_data_json, const std::string& client_data_json,
...@@ -826,7 +802,7 @@ void AuthenticatorCommon::MakeCredential( ...@@ -826,7 +802,7 @@ void AuthenticatorCommon::MakeCredential(
make_credential_response_callback_ = std::move(callback); make_credential_response_callback_ = std::move(callback);
timer_->Start( timer_->Start(
FROM_HERE, AdjustTimeout(options->timeout, render_frame_host_), FROM_HERE, options->adjusted_timeout,
base::BindOnce(&AuthenticatorCommon::OnTimeout, base::Unretained(this))); base::BindOnce(&AuthenticatorCommon::OnTimeout, base::Unretained(this)));
const bool origin_is_crypto_token_extension = const bool origin_is_crypto_token_extension =
...@@ -1010,7 +986,7 @@ void AuthenticatorCommon::GetAssertion( ...@@ -1010,7 +986,7 @@ void AuthenticatorCommon::GetAssertion(
get_assertion_response_callback_ = std::move(callback); get_assertion_response_callback_ = std::move(callback);
timer_->Start( timer_->Start(
FROM_HERE, AdjustTimeout(options->timeout, render_frame_host_), FROM_HERE, options->adjusted_timeout,
base::BindOnce(&AuthenticatorCommon::OnTimeout, base::Unretained(this))); base::BindOnce(&AuthenticatorCommon::OnTimeout, base::Unretained(this)));
ctap_get_assertion_request_ = CreateCtapGetAssertionRequest( ctap_get_assertion_request_ = CreateCtapGetAssertionRequest(
......
...@@ -346,7 +346,7 @@ GetTestPublicKeyCredentialCreationOptions() { ...@@ -346,7 +346,7 @@ GetTestPublicKeyCredentialCreationOptions() {
options->public_key_parameters = options->public_key_parameters =
GetTestPublicKeyCredentialParameters(kCoseEs256); GetTestPublicKeyCredentialParameters(kCoseEs256);
options->challenge.assign(32, 0x0A); options->challenge.assign(32, 0x0A);
options->timeout = base::TimeDelta::FromMinutes(1); options->adjusted_timeout = base::TimeDelta::FromMinutes(1);
options->authenticator_selection = GetTestAuthenticatorSelectionCriteria(); options->authenticator_selection = GetTestAuthenticatorSelectionCriteria();
return options; return options;
} }
...@@ -356,7 +356,7 @@ GetTestPublicKeyCredentialRequestOptions() { ...@@ -356,7 +356,7 @@ GetTestPublicKeyCredentialRequestOptions() {
auto options = PublicKeyCredentialRequestOptions::New(); auto options = PublicKeyCredentialRequestOptions::New();
options->relying_party_id = std::string(kTestRelyingPartyId); options->relying_party_id = std::string(kTestRelyingPartyId);
options->challenge.assign(32, 0x0A); options->challenge.assign(32, 0x0A);
options->timeout = base::TimeDelta::FromMinutes(1); options->adjusted_timeout = base::TimeDelta::FromMinutes(1);
options->user_verification = device::UserVerificationRequirement::kPreferred; options->user_verification = device::UserVerificationRequirement::kPreferred;
options->allow_credentials = GetTestCredentials(); options->allow_credentials = GetTestCredentials();
return options; return options;
...@@ -2027,7 +2027,7 @@ class AuthenticatorContentBrowserClientTest : public AuthenticatorImplTest { ...@@ -2027,7 +2027,7 @@ class AuthenticatorContentBrowserClientTest : public AuthenticatorImplTest {
PublicKeyCredentialCreationOptionsPtr options = PublicKeyCredentialCreationOptionsPtr options =
GetTestPublicKeyCredentialCreationOptions(); GetTestPublicKeyCredentialCreationOptions();
options->relying_party.id = "example.com"; options->relying_party.id = "example.com";
options->timeout = base::TimeDelta::FromSeconds(1); options->adjusted_timeout = base::TimeDelta::FromSeconds(1);
options->attestation = options->attestation =
ConvertAttestationConveyancePreference(test.attestation_requested); ConvertAttestationConveyancePreference(test.attestation_requested);
TestMakeCredentialCallback callback_receiver; TestMakeCredentialCallback callback_receiver;
...@@ -3771,7 +3771,7 @@ TEST_F(InternalUVAuthenticatorImplTest, MakeCredential) { ...@@ -3771,7 +3771,7 @@ TEST_F(InternalUVAuthenticatorImplTest, MakeCredential) {
!fingerprints_enrolled && !fingerprints_enrolled &&
uv == device::UserVerificationRequirement::kRequired; uv == device::UserVerificationRequirement::kRequired;
if (should_timeout) { if (should_timeout) {
options->timeout = base::TimeDelta::FromMilliseconds(100); options->adjusted_timeout = base::TimeDelta::FromMilliseconds(100);
} }
TestMakeCredentialCallback callback_receiver; TestMakeCredentialCallback callback_receiver;
...@@ -4093,7 +4093,7 @@ TEST_F(UVTokenAuthenticatorImplTest, MakeCredentialUVToken) { ...@@ -4093,7 +4093,7 @@ TEST_F(UVTokenAuthenticatorImplTest, MakeCredentialUVToken) {
!fingerprints_enrolled && !fingerprints_enrolled &&
uv == device::UserVerificationRequirement::kRequired; uv == device::UserVerificationRequirement::kRequired;
if (should_timeout) { if (should_timeout) {
options->timeout = base::TimeDelta::FromMilliseconds(100); options->adjusted_timeout = base::TimeDelta::FromMilliseconds(100);
} }
TestMakeCredentialCallback callback_receiver; TestMakeCredentialCallback callback_receiver;
......
...@@ -128,7 +128,7 @@ constexpr char kCreatePublicKeyTemplate[] = ...@@ -128,7 +128,7 @@ constexpr char kCreatePublicKeyTemplate[] =
" displayName: 'Avery A. Jones', " " displayName: 'Avery A. Jones', "
" icon: '$8'}," " icon: '$8'},"
" pubKeyCredParams: [{ type: 'public-key', alg: '$4'}]," " pubKeyCredParams: [{ type: 'public-key', alg: '$4'}],"
" timeout: _timeout_," " timeout: 1000,"
" excludeCredentials: []," " excludeCredentials: [],"
" authenticatorSelection: {" " authenticatorSelection: {"
" requireResidentKey: $1," " requireResidentKey: $1,"
...@@ -150,7 +150,7 @@ constexpr char kCreatePublicKeyWithAbortSignalTemplate[] = ...@@ -150,7 +150,7 @@ constexpr char kCreatePublicKeyWithAbortSignalTemplate[] =
" displayName: 'Avery A. Jones', " " displayName: 'Avery A. Jones', "
" icon: '$8'}," " icon: '$8'},"
" pubKeyCredParams: [{ type: 'public-key', alg: '$4'}]," " pubKeyCredParams: [{ type: 'public-key', alg: '$4'}],"
" timeout: _timeout_," " timeout: 1000,"
" excludeCredentials: []," " excludeCredentials: [],"
" authenticatorSelection: {" " authenticatorSelection: {"
" requireResidentKey: $1," " requireResidentKey: $1,"
...@@ -167,7 +167,6 @@ constexpr char kPlatform[] = "platform"; ...@@ -167,7 +167,6 @@ constexpr char kPlatform[] = "platform";
constexpr char kCrossPlatform[] = "cross-platform"; constexpr char kCrossPlatform[] = "cross-platform";
constexpr char kPreferredVerification[] = "preferred"; constexpr char kPreferredVerification[] = "preferred";
constexpr char kRequiredVerification[] = "required"; constexpr char kRequiredVerification[] = "required";
constexpr char kShortTimeout[] = "100";
// Default values for kCreatePublicKeyTemplate. // Default values for kCreatePublicKeyTemplate.
struct CreateParameters { struct CreateParameters {
...@@ -184,7 +183,6 @@ struct CreateParameters { ...@@ -184,7 +183,6 @@ struct CreateParameters {
// It can use the |PublicKeyCredential| object named |c| to extract useful // It can use the |PublicKeyCredential| object named |c| to extract useful
// fields. // fields.
const char* extra_ok_output = "''"; const char* extra_ok_output = "''";
const char* timeout = "1000";
}; };
std::string BuildCreateCallWithParameters(const CreateParameters& parameters) { std::string BuildCreateCallWithParameters(const CreateParameters& parameters) {
...@@ -198,26 +196,20 @@ std::string BuildCreateCallWithParameters(const CreateParameters& parameters) { ...@@ -198,26 +196,20 @@ std::string BuildCreateCallWithParameters(const CreateParameters& parameters) {
substitutions.push_back(parameters.rp_icon); substitutions.push_back(parameters.rp_icon);
substitutions.push_back(parameters.user_icon); substitutions.push_back(parameters.user_icon);
std::string result;
if (strlen(parameters.signal) == 0) { if (strlen(parameters.signal) == 0) {
substitutions.push_back(parameters.extra_ok_output); substitutions.push_back(parameters.extra_ok_output);
result = base::ReplaceStringPlaceholders(kCreatePublicKeyTemplate, return base::ReplaceStringPlaceholders(kCreatePublicKeyTemplate,
substitutions, nullptr); substitutions, nullptr);
} else {
substitutions.push_back(parameters.signal);
result = base::ReplaceStringPlaceholders(
kCreatePublicKeyWithAbortSignalTemplate, substitutions, nullptr);
} }
substitutions.push_back(parameters.signal);
base::ReplaceFirstSubstringAfterOffset(&result, 0, "_timeout_", return base::ReplaceStringPlaceholders(
parameters.timeout); kCreatePublicKeyWithAbortSignalTemplate, substitutions, nullptr);
return result;
} }
constexpr char kGetPublicKeyTemplate[] = constexpr char kGetPublicKeyTemplate[] =
"navigator.credentials.get({ publicKey: {" "navigator.credentials.get({ publicKey: {"
" challenge: new TextEncoder().encode('climb a mountain')," " challenge: new TextEncoder().encode('climb a mountain'),"
" timeout: $4," " timeout: 1000,"
" userVerification: '$1'," " userVerification: '$1',"
" $2}" " $2}"
"}).then(c => window.domAutomationController.send('webauth: OK' + $3)," "}).then(c => window.domAutomationController.send('webauth: OK' + $3),"
...@@ -227,10 +219,10 @@ constexpr char kGetPublicKeyTemplate[] = ...@@ -227,10 +219,10 @@ constexpr char kGetPublicKeyTemplate[] =
constexpr char kGetPublicKeyWithAbortSignalTemplate[] = constexpr char kGetPublicKeyWithAbortSignalTemplate[] =
"navigator.credentials.get({ publicKey: {" "navigator.credentials.get({ publicKey: {"
" challenge: new TextEncoder().encode('climb a mountain')," " challenge: new TextEncoder().encode('climb a mountain'),"
" timeout: $4," " timeout: 1000,"
" userVerification: '$1'," " userVerification: '$1',"
" $2}," " $2},"
" signal: $5" " signal: $4"
"}).catch(c => window.domAutomationController.send(" "}).catch(c => window.domAutomationController.send("
" 'webauth: ' + c.toString()));"; " 'webauth: ' + c.toString()));";
...@@ -242,7 +234,6 @@ struct GetParameters { ...@@ -242,7 +234,6 @@ struct GetParameters {
" id: new TextEncoder().encode('allowedCredential')," " id: new TextEncoder().encode('allowedCredential'),"
" transports: ['usb', 'nfc', 'ble']}]"; " transports: ['usb', 'nfc', 'ble']}]";
const char* signal = ""; const char* signal = "";
const char* timeout = "1000";
// extra_ok_output is a Javascript expression which must evaluate to a string. // extra_ok_output is a Javascript expression which must evaluate to a string.
// It can use the |PublicKeyCredential| object named |c| to extract useful // It can use the |PublicKeyCredential| object named |c| to extract useful
// fields. // fields.
...@@ -254,7 +245,6 @@ std::string BuildGetCallWithParameters(const GetParameters& parameters) { ...@@ -254,7 +245,6 @@ std::string BuildGetCallWithParameters(const GetParameters& parameters) {
substitutions.push_back(parameters.user_verification); substitutions.push_back(parameters.user_verification);
substitutions.push_back(parameters.allow_credentials); substitutions.push_back(parameters.allow_credentials);
substitutions.push_back(parameters.extra_ok_output); substitutions.push_back(parameters.extra_ok_output);
substitutions.push_back(parameters.timeout);
if (strlen(parameters.signal) == 0) { if (strlen(parameters.signal) == 0) {
return base::ReplaceStringPlaceholders(kGetPublicKeyTemplate, substitutions, return base::ReplaceStringPlaceholders(kGetPublicKeyTemplate, substitutions,
nullptr); nullptr);
...@@ -868,7 +858,6 @@ IN_PROC_BROWSER_TEST_F(WebAuthJavascriptClientBrowserTest, ...@@ -868,7 +858,6 @@ IN_PROC_BROWSER_TEST_F(WebAuthJavascriptClientBrowserTest,
CreateParameters parameters; CreateParameters parameters;
parameters.user_verification = kRequiredVerification; parameters.user_verification = kRequiredVerification;
parameters.timeout = kShortTimeout;
std::string result; std::string result;
ASSERT_TRUE(content::ExecuteScriptAndExtractString( ASSERT_TRUE(content::ExecuteScriptAndExtractString(
shell()->web_contents()->GetMainFrame(), shell()->web_contents()->GetMainFrame(),
...@@ -906,7 +895,6 @@ IN_PROC_BROWSER_TEST_F(WebAuthJavascriptClientBrowserTest, ...@@ -906,7 +895,6 @@ IN_PROC_BROWSER_TEST_F(WebAuthJavascriptClientBrowserTest,
CreateParameters parameters; CreateParameters parameters;
parameters.algorithm_identifier = "123"; parameters.algorithm_identifier = "123";
parameters.timeout = kShortTimeout;
std::string result; std::string result;
ASSERT_TRUE(content::ExecuteScriptAndExtractString( ASSERT_TRUE(content::ExecuteScriptAndExtractString(
shell()->web_contents()->GetMainFrame(), shell()->web_contents()->GetMainFrame(),
...@@ -926,7 +914,6 @@ IN_PROC_BROWSER_TEST_F(WebAuthJavascriptClientBrowserTest, ...@@ -926,7 +914,6 @@ IN_PROC_BROWSER_TEST_F(WebAuthJavascriptClientBrowserTest,
CreateParameters parameters; CreateParameters parameters;
parameters.authenticator_attachment = kPlatform; parameters.authenticator_attachment = kPlatform;
parameters.timeout = kShortTimeout;
std::string result; std::string result;
ASSERT_TRUE(content::ExecuteScriptAndExtractString( ASSERT_TRUE(content::ExecuteScriptAndExtractString(
shell()->web_contents()->GetMainFrame(), shell()->web_contents()->GetMainFrame(),
...@@ -1028,7 +1015,6 @@ IN_PROC_BROWSER_TEST_F(WebAuthJavascriptClientBrowserTest, ...@@ -1028,7 +1015,6 @@ IN_PROC_BROWSER_TEST_F(WebAuthJavascriptClientBrowserTest,
" id: new TextEncoder().encode('allowedCredential')," " id: new TextEncoder().encode('allowedCredential'),"
" transports: ['carrierpigeon']," " transports: ['carrierpigeon'],"
"}]"; "}]";
parameters.timeout = kShortTimeout;
std::string result; std::string result;
ASSERT_TRUE(content::ExecuteScriptAndExtractString( ASSERT_TRUE(content::ExecuteScriptAndExtractString(
shell()->web_contents()->GetMainFrame(), shell()->web_contents()->GetMainFrame(),
......
...@@ -210,9 +210,9 @@ struct PublicKeyCredentialRequestOptions { ...@@ -210,9 +210,9 @@ struct PublicKeyCredentialRequestOptions {
// to be sent to an authenticator for signing. // to be sent to an authenticator for signing.
array<uint8> challenge; array<uint8> challenge;
// Time to wait for an authenticator to complete an operation provided by the // Time to wait for an authenticator to complete an operation.
// relying party. // Adjusted to fall within a client-defined range.
mojo_base.mojom.TimeDelta? timeout; mojo_base.mojom.TimeDelta adjusted_timeout;
// An ASCII serialization of the origin claimed by the relying party. // An ASCII serialization of the origin claimed by the relying party.
string relying_party_id; string relying_party_id;
...@@ -299,9 +299,9 @@ struct PublicKeyCredentialCreationOptions { ...@@ -299,9 +299,9 @@ struct PublicKeyCredentialCreationOptions {
// party would accept. // party would accept.
array<PublicKeyCredentialParameters> public_key_parameters; array<PublicKeyCredentialParameters> public_key_parameters;
// Time to wait for an authenticator to complete an operation provided by the // Time to wait for an authenticator to complete an operation.
// relying party. // Adjusted to fall within a client-defined range.
mojo_base.mojom.TimeDelta? timeout; mojo_base.mojom.TimeDelta adjusted_timeout;
// A list of credentials the relying party knows about. If an // A list of credentials the relying party knows about. If an
// authenticator has one of these credentials, it should not // authenticator has one of these credentials, it should not
......
...@@ -25,6 +25,21 @@ ...@@ -25,6 +25,21 @@
#include "third_party/blink/renderer/modules/credentialmanager/password_credential.h" #include "third_party/blink/renderer/modules/credentialmanager/password_credential.h"
#include "third_party/blink/renderer/modules/credentialmanager/public_key_credential.h" #include "third_party/blink/renderer/modules/credentialmanager/public_key_credential.h"
namespace {
// Time to wait for an authenticator to successfully complete an operation.
constexpr base::TimeDelta kAdjustedTimeoutLower =
base::TimeDelta::FromSeconds(10);
constexpr base::TimeDelta kAdjustedTimeoutUpper =
base::TimeDelta::FromMinutes(10);
base::TimeDelta AdjustTimeout(uint32_t timeout) {
base::TimeDelta adjusted_timeout;
adjusted_timeout = base::TimeDelta::FromMilliseconds(timeout);
return std::max(kAdjustedTimeoutLower,
std::min(kAdjustedTimeoutUpper, adjusted_timeout));
}
} // namespace
namespace mojo { namespace mojo {
using blink::mojom::blink::AttestationConveyancePreference; using blink::mojom::blink::AttestationConveyancePreference;
...@@ -393,8 +408,9 @@ TypeConverter<PublicKeyCredentialCreationOptionsPtr, ...@@ -393,8 +408,9 @@ TypeConverter<PublicKeyCredentialCreationOptionsPtr,
// Step 4 of https://w3c.github.io/webauthn/#createCredential // Step 4 of https://w3c.github.io/webauthn/#createCredential
if (options->hasTimeout()) { if (options->hasTimeout()) {
mojo_options->timeout = mojo_options->adjusted_timeout = AdjustTimeout(options->timeout());
base::TimeDelta::FromMilliseconds(options->timeout()); } else {
mojo_options->adjusted_timeout = kAdjustedTimeoutUpper;
} }
// Steps 8 and 9 of // Steps 8 and 9 of
...@@ -536,8 +552,9 @@ TypeConverter<PublicKeyCredentialRequestOptionsPtr, ...@@ -536,8 +552,9 @@ TypeConverter<PublicKeyCredentialRequestOptionsPtr,
mojo_options->challenge = ConvertTo<Vector<uint8_t>>(options->challenge()); mojo_options->challenge = ConvertTo<Vector<uint8_t>>(options->challenge());
if (options->hasTimeout()) { if (options->hasTimeout()) {
mojo_options->timeout = mojo_options->adjusted_timeout = AdjustTimeout(options->timeout());
base::TimeDelta::FromMilliseconds(options->timeout()); } else {
mojo_options->adjusted_timeout = kAdjustedTimeoutUpper;
} }
mojo_options->relying_party_id = options->rpId(); mojo_options->relying_party_id = options->rpId();
......
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