Skip to content

Commit

Permalink
webauthn: unwind prefer_native_api.
Browse files Browse the repository at this point in the history
This value is unused.

Change-Id: Icb71621254e1ef85b607d209f59310653a2d1d94
Bug: 1426683
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/4359192
Commit-Queue: Adam Langley <agl@chromium.org>
Reviewed-by: Avi Drissman <avi@chromium.org>
Reviewed-by: Gabriel Charette <gab@chromium.org>
Reviewed-by: Nina Satragno <nsatragno@chromium.org>
Cr-Commit-Position: refs/heads/main@{#1124523}
  • Loading branch information
Adam Langley authored and Chromium LUCI CQ committed Mar 31, 2023
1 parent 2ec46be commit 3906703
Show file tree
Hide file tree
Showing 16 changed files with 58 additions and 157 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -120,7 +120,7 @@ class ChromeWebAuthnCredentialsDelegateTest
#if !BUILDFLAG(IS_ANDROID)
dialog_model()->StartFlow(
AuthenticatorRequestDialogModel::TransportAvailabilityInfo(),
/*is_conditional_mediation=*/true, /*prefer_native_api=*/false);
/*is_conditional_mediation=*/true);
dialog_model()->ReplaceCredListForTesting(std::move(creds));
#else
delegate_->OnWebAuthnRequestPending(
Expand Down
16 changes: 16 additions & 0 deletions chrome/browser/prefs/browser_prefs.cc
Original file line number Diff line number Diff line change
Expand Up @@ -833,6 +833,12 @@ const char kDarkLightModeNudgeLeftToShowCount[] =
"ash.dark_light_mode.educational_nudge";
#endif // BUILDFLAG(IS_CHROMEOS_ASH)

// Deprecated 03/2023.
#if BUILDFLAG(IS_WIN)
const char kWebAuthnLastOperationWasNativeAPI[] =
"webauthn.last_op_used_native_api";
#endif

// Register local state used only for migration (clearing or moving to a new
// key).
void RegisterLocalStatePrefsForMigration(PrefRegistrySimple* registry) {
Expand Down Expand Up @@ -1132,6 +1138,11 @@ void RegisterProfilePrefsForMigration(
registry->RegisterIntegerPref(kDarkLightModeNudgeLeftToShowCount,
ash::kDarkLightModeNudgeMaxShownCount);
#endif // BUILDFLAG(IS_CHROMEOS_ASH)

// Deprecated 03/2023.
#if BUILDFLAG(IS_WIN)
registry->RegisterBooleanPref(kWebAuthnLastOperationWasNativeAPI, false);
#endif
}

} // namespace
Expand Down Expand Up @@ -2192,6 +2203,11 @@ void MigrateObsoleteProfilePrefs(Profile* profile) {
profile_prefs->ClearPref(kDarkLightModeNudgeLeftToShowCount);
#endif // BUILDFLAG(IS_CHROMEOS_ASH)

// Added 03/2023.
#if BUILDFLAG(IS_WIN)
profile_prefs->ClearPref(kWebAuthnLastOperationWasNativeAPI);
#endif

// Please don't delete the following line. It is used by PRESUBMIT.py.
// END_MIGRATE_OBSOLETE_PROFILE_PREFS

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -118,8 +118,7 @@ class AuthenticatorDialogViewTest : public DialogBrowserTest {
if (name == "default") {
dialog_model_->StartFlow(
device::FidoRequestHandlerBase::TransportAvailabilityInfo(),
/*use_location_bar_bubble=*/false,
/*prefer_native_api=*/false);
/*is_conditional_mediation=*/false);
dialog_model_->SetCurrentStepForTesting(
AuthenticatorRequestDialogModel::Step::kTimedOut);
content::WebContents* const web_contents =
Expand Down Expand Up @@ -147,8 +146,7 @@ class AuthenticatorDialogViewTest : public DialogBrowserTest {
/*paired_phones=*/{phone},
/*contact_phone_callback=*/base::DoNothing(), "fido://qrcode");
dialog_model_->StartFlow(std::move(transport_availability),
/*use_location_bar_bubble=*/false,
/*prefer_native_api=*/false);
/*is_conditional_mediation=*/false);

// The dialog is owned by the Views hierarchy so this is a non-owning
// pointer.
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -320,8 +320,7 @@ class AuthenticatorDialogTest : public DialogBrowserTest {
#endif

model_->StartFlow(std::move(transport_availability),
/*is_conditional_mediation=*/false,
/*prefer_native_api=*/false);
/*is_conditional_mediation=*/false);
}

private:
Expand Down
8 changes: 3 additions & 5 deletions chrome/browser/webauthn/authenticator_request_dialog_model.cc
Original file line number Diff line number Diff line change
Expand Up @@ -196,16 +196,15 @@ void AuthenticatorRequestDialogModel::HideDialog() {

void AuthenticatorRequestDialogModel::StartFlow(
TransportAvailabilityInfo transport_availability,
bool use_conditional_mediation,
bool prefer_native_api) {
bool use_conditional_mediation) {
DCHECK(!started_);
DCHECK_EQ(current_step(), Step::kNotStarted);

started_ = true;
transport_availability_ = std::move(transport_availability);
use_conditional_mediation_ = use_conditional_mediation;

PopulateMechanisms(prefer_native_api);
PopulateMechanisms();
if (base::FeatureList::IsEnabled(device::kWebAuthnNewPrioritiesImpl)) {
priority_mechanism_index_ = IndexOfPriorityMechanism();
} else {
Expand Down Expand Up @@ -1093,8 +1092,7 @@ void AuthenticatorRequestDialogModel::ContactNextPhoneByName(
DCHECK(found_name);
}

void AuthenticatorRequestDialogModel::PopulateMechanisms(
bool prefer_native_api) {
void AuthenticatorRequestDialogModel::PopulateMechanisms() {
const bool is_get_assertion = transport_availability_.request_type ==
device::FidoRequestType::kGetAssertion;
const bool is_passkey_request =
Expand Down
17 changes: 3 additions & 14 deletions chrome/browser/webauthn/authenticator_request_dialog_model.h
Original file line number Diff line number Diff line change
Expand Up @@ -297,15 +297,9 @@ class AuthenticatorRequestDialogModel {
// If |is_conditional_mediation| is true, credentials will be shown on the
// password autofill instead of the full-blown page-modal UI.
//
// |prefer_native_api| indicates that the UI should jump directly to the
// system WebAuthn UI if there's no better option. This is currently only
// meaningful on Windows, but the parameter exists on all platforms to avoid
// too much #ifdef soup.
//
// Valid action when at step: kNotStarted.
void StartFlow(TransportAvailabilityInfo transport_availability,
bool is_conditional_mediation,
bool prefer_native_api);
void StartFlow(TransportAvailabilityInfo trasport_availability,
bool is_conditional_mediation);

// Restarts the UX flow.
void StartOver();
Expand Down Expand Up @@ -687,12 +681,7 @@ class AuthenticatorRequestDialogModel {
void ContactNextPhoneByName(const std::string& name);

// PopulateMechanisms fills in |mechanisms_|.
//
// |prefer_native_api| indicates that the UI should jump directly to the
// system WebAuthn UI if there's no better option. This is currently only
// meaningful on Windows, but the parameter exists on all platforms to avoid
// too much #ifdef soup.
void PopulateMechanisms(bool prefer_native_api);
void PopulateMechanisms();

// IndexOfPriorityMechanism returns the index, in |mechanisms_|, of the
// Mechanism that should be triggered immediately, if any.
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -109,7 +109,6 @@ enum class TransportAvailabilityParam {
kHasWinNativeAuthenticator,
kHasCableV1Extension,
kHasCableV2Extension,
kPreferNativeAPI,
kRequireResidentKey,
kIsConditionalUI,
kAttachmentAny,
Expand Down Expand Up @@ -137,8 +136,6 @@ base::StringPiece TransportAvailabilityParamToString(
return "kHasCableV1Extension";
case TransportAvailabilityParam::kHasCableV2Extension:
return "kHasCableV2Extension";
case TransportAvailabilityParam::kPreferNativeAPI:
return "kPreferNativeAPI";
case TransportAvailabilityParam::kRequireResidentKey:
return "kRequireResidentKey";
case TransportAvailabilityParam::kIsConditionalUI:
Expand Down Expand Up @@ -663,11 +660,7 @@ TEST_F(AuthenticatorRequestDialogModelTest, Mechanisms) {

bool is_conditional_ui = base::Contains(
test.params, TransportAvailabilityParam::kIsConditionalUI);
model.StartFlow(
std::move(transports_info), is_conditional_ui,
/*prefer_native_api=*/
base::Contains(test.params,
TransportAvailabilityParam::kPreferNativeAPI));
model.StartFlow(std::move(transports_info), is_conditional_ui);
if (is_conditional_ui) {
EXPECT_EQ(model.current_step(), Step::kConditionalMediation);
model.TransitionToModalWebAuthnRequest();
Expand Down Expand Up @@ -713,8 +706,7 @@ TEST_F(AuthenticatorRequestDialogModelTest, WinCancel) {
"fido:/1234");

model.StartFlow(std::move(tai),
/*is_conditional_mediation=*/false,
/*prefer_native_api=*/false);
/*is_conditional_mediation=*/false);

if (!is_passkey_request) {
// The Windows native UI should have been triggered.
Expand All @@ -740,8 +732,7 @@ TEST_F(AuthenticatorRequestDialogModelTest, WinNoPlatformAuthenticator) {
tai.win_is_uvpaa = false;
tai.has_win_native_api_authenticator = true;
AuthenticatorRequestDialogModel model(/*render_frame_host=*/nullptr);
model.StartFlow(std::move(tai), /*is_conditional_mediation=*/false,
/*prefer_native_api=*/false);
model.StartFlow(std::move(tai), /*is_conditional_mediation=*/false);
EXPECT_EQ(
model.current_step(),
AuthenticatorRequestDialogModel::Step::kErrorWindowsHelloNotEnabled);
Expand All @@ -756,8 +747,7 @@ TEST_F(AuthenticatorRequestDialogModelTest, NoAvailableTransports) {

EXPECT_CALL(mock_observer, OnStepTransition());
model.StartFlow(TransportAvailabilityInfo(),
/*is_conditional_mediation=*/false,
/*prefer_native_api=*/false);
/*is_conditional_mediation=*/false);
EXPECT_EQ(Step::kErrorNoAvailableTransports, model.current_step());
testing::Mock::VerifyAndClearExpectations(&mock_observer);

Expand Down Expand Up @@ -833,8 +823,7 @@ TEST_F(AuthenticatorRequestDialogModelTest, Cable2ndFactorFlows) {
absl::nullopt);

model.StartFlow(std::move(transports_info),
/*is_conditional_mediation=*/false,
/*prefer_native_api=*/false);
/*is_conditional_mediation=*/false);
ASSERT_EQ(model.mechanisms().size(), 2u);

for (const auto step : test.steps) {
Expand Down Expand Up @@ -896,8 +885,7 @@ TEST_F(AuthenticatorRequestDialogModelTest, AwaitingAcknowledgement) {

EXPECT_CALL(mock_observer, OnStepTransition());
model.StartFlow(std::move(transports_info),
/*is_conditional_mediation=*/false,
/*prefer_native_api=*/false);
/*is_conditional_mediation=*/false);
EXPECT_EQ(Step::kMechanismSelection, model.current_step());
testing::Mock::VerifyAndClearExpectations(&mock_observer);

Expand Down Expand Up @@ -936,8 +924,7 @@ TEST_F(AuthenticatorRequestDialogModelTest, BleAdapterAlreadyPowered) {
model.SetBluetoothAdapterPowerOnCallback(power_receiver.GetCallback());
model.set_cable_transport_info(true, {}, base::DoNothing(), absl::nullopt);
model.StartFlow(std::move(transports_info),
/*is_conditional_mediation=*/false,
/*prefer_native_api=*/false);
/*is_conditional_mediation=*/false);
EXPECT_EQ(test_case.expected_final_step, model.current_step());
EXPECT_TRUE(model.ble_adapter_is_powered());
EXPECT_FALSE(power_receiver.was_called());
Expand Down Expand Up @@ -966,8 +953,7 @@ TEST_F(AuthenticatorRequestDialogModelTest, BleAdapterNeedToBeManuallyPowered) {
model.SetBluetoothAdapterPowerOnCallback(power_receiver.GetCallback());
model.set_cable_transport_info(true, {}, base::DoNothing(), absl::nullopt);
model.StartFlow(std::move(transports_info),
/*is_conditional_mediation=*/false,
/*prefer_native_api=*/false);
/*is_conditional_mediation=*/false);

EXPECT_EQ(Step::kBlePowerOnManual, model.current_step());
EXPECT_FALSE(model.ble_adapter_is_powered());
Expand Down Expand Up @@ -1007,8 +993,7 @@ TEST_F(AuthenticatorRequestDialogModelTest,
model.SetBluetoothAdapterPowerOnCallback(power_receiver.GetCallback());
model.set_cable_transport_info(true, {}, base::DoNothing(), absl::nullopt);
model.StartFlow(std::move(transports_info),
/*is_conditional_mediation=*/false,
/*prefer_native_api=*/false);
/*is_conditional_mediation=*/false);

EXPECT_EQ(Step::kBlePowerOnAutomatic, model.current_step());

Expand Down Expand Up @@ -1044,8 +1029,7 @@ TEST_F(AuthenticatorRequestDialogModelTest,
&dispatched_authenticator_ids));

model.StartFlow(std::move(transports_info),
/*is_conditional_mediation=*/false,
/*prefer_native_api=*/false);
/*is_conditional_mediation=*/false);

EXPECT_TRUE(model.should_dialog_be_closed());
task_environment()->RunUntilIdle();
Expand Down Expand Up @@ -1078,8 +1062,7 @@ TEST_F(AuthenticatorRequestDialogModelTest,
transports_info.has_platform_authenticator_credential = device::
FidoRequestHandlerBase::RecognizedCredential::kHasRecognizedCredential;
model.StartFlow(std::move(transports_info),
/*is_conditional_mediation=*/true,
/*prefer_native_api=*/false);
/*is_conditional_mediation=*/true);
task_environment()->RunUntilIdle();
EXPECT_EQ(model.current_step(), Step::kConditionalMediation);
EXPECT_TRUE(model.should_dialog_be_closed());
Expand Down Expand Up @@ -1115,8 +1098,7 @@ TEST_F(AuthenticatorRequestDialogModelTest, ConditionalUIRecognizedCredential) {
transports_info.recognized_platform_authenticator_credentials = {kCred1,
kCred2};
model.StartFlow(std::move(transports_info),
/*is_conditional_mediation=*/true,
/*prefer_native_api=*/false);
/*is_conditional_mediation=*/true);
EXPECT_EQ(model.current_step(), Step::kConditionalMediation);
EXPECT_TRUE(model.should_dialog_be_closed());
EXPECT_EQ(request_num_called, 0);
Expand All @@ -1140,8 +1122,7 @@ TEST_F(AuthenticatorRequestDialogModelTest, ConditionalUICancelRequest) {

EXPECT_CALL(mock_observer, OnStepTransition());
model.StartFlow(std::move(TransportAvailabilityInfo()),
/*is_conditional_mediation=*/true,
/*prefer_native_api=*/false);
/*is_conditional_mediation=*/true);
EXPECT_EQ(model.current_step(), Step::kConditionalMediation);
testing::Mock::VerifyAndClearExpectations(&mock_observer);

Expand All @@ -1168,8 +1149,7 @@ TEST_F(AuthenticatorRequestDialogModelTest, ConditionalUIWindowsCancel) {

EXPECT_CALL(mock_observer, OnStepTransition());
model.StartFlow(std::move(TransportAvailabilityInfo()),
/*is_conditional_mediation=*/true,
/*prefer_native_api=*/false);
/*is_conditional_mediation=*/true);
EXPECT_EQ(model.current_step(), Step::kConditionalMediation);
testing::Mock::VerifyAndClearExpectations(&mock_observer);

Expand Down Expand Up @@ -1214,8 +1194,7 @@ TEST_F(AuthenticatorRequestDialogModelTest, PreSelectWithEmptyAllowList) {
transports_info.recognized_platform_authenticator_credentials = {kCred1,
kCred2};
model.StartFlow(std::move(transports_info),
/*is_conditional_mediation=*/false,
/*prefer_native_api=*/false);
/*is_conditional_mediation=*/false);
EXPECT_EQ(model.current_step(), Step::kPreSelectAccount);
EXPECT_EQ(request_num_called, 0);

Expand All @@ -1239,8 +1218,7 @@ TEST_F(AuthenticatorRequestDialogModelTest, ContactPriorityPhone) {
transports_info.request_type = device::FidoRequestType::kGetAssertion;
transports_info.available_transports = {AuthenticatorTransport::kHybrid};
model.StartFlow(std::move(transports_info),
/*is_conditional_mediation=*/false,
/*prefer_native_api=*/false);
/*is_conditional_mediation=*/false);
model.ContactPriorityPhone();
EXPECT_EQ(model.current_step(), Step::kCableActivate);
EXPECT_EQ(model.selected_phone_name(), "phone");
Expand Down
40 changes: 1 addition & 39 deletions chrome/browser/webauthn/chrome_authenticator_request_delegate.cc
Original file line number Diff line number Diff line change
Expand Up @@ -111,14 +111,6 @@ bool IsOriginListedInEnterpriseAttestationSwitch(
});
}

#if BUILDFLAG(IS_WIN)
// kWebAuthnLastOperationWasNativeAPI is a boolean preference that records
// whether the last successful operation used the Windows native API. If so
// then we'll try and jump directly to it next time.
const char kWebAuthnLastOperationWasNativeAPI[] =
"webauthn.last_op_used_native_api";
#endif

#if BUILDFLAG(IS_MAC)
const char kWebAuthnTouchIdMetadataSecretPrefName[] =
"webauthn.touchid.metadata_secret";
Expand Down Expand Up @@ -310,23 +302,6 @@ bool ChromeWebAuthenticationDelegate::IsFocused(
return web_contents->GetVisibility() == content::Visibility::VISIBLE;
}

#if BUILDFLAG(IS_WIN)
void ChromeWebAuthenticationDelegate::OperationSucceeded(
content::BrowserContext* browser_context,
bool used_win_api) {
// If a registration or assertion operation was successful, record whether the
// Windows native API was used for it. If so we'll jump directly to the native
// UI for the next operation.
Profile* const profile = Profile::FromBrowserContext(browser_context);
if (profile->IsOffTheRecord()) {
return;
}

profile->GetPrefs()->SetBoolean(kWebAuthnLastOperationWasNativeAPI,
used_win_api);
}
#endif

absl::optional<bool> ChromeWebAuthenticationDelegate::
IsUserVerifyingPlatformAuthenticatorAvailableOverride(
content::RenderFrameHost* render_frame_host) {
Expand Down Expand Up @@ -413,7 +388,6 @@ void ChromeAuthenticatorRequestDelegate::RegisterProfilePrefs(
user_prefs::PrefRegistrySyncable* registry) {
registry->RegisterListPref(prefs::kSecurityKeyPermitAttestation);
#if BUILDFLAG(IS_WIN)
registry->RegisterBooleanPref(kWebAuthnLastOperationWasNativeAPI, false);
LocalCredentialManagementWin::RegisterProfilePrefs(registry);
#endif
#if BUILDFLAG(IS_MAC)
Expand Down Expand Up @@ -789,19 +763,7 @@ void ChromeAuthenticatorRequestDelegate::OnTransportAvailabilityEnumerated(
return;
}

bool jump_to_native_ui = false;
#if BUILDFLAG(IS_WIN)
// Conditional requests always show the Chrome UI first because the UI is
// triggered from "Use passkey from another device" in autofill, and it would
// be confusing if the caBLE option wasn't presented after that.
if (!is_conditional_) {
PrefService* const prefs =
user_prefs::UserPrefs::Get(GetRenderFrameHost()->GetBrowserContext());
jump_to_native_ui = prefs->GetBoolean(kWebAuthnLastOperationWasNativeAPI);
}
#endif

dialog_model_->StartFlow(std::move(data), is_conditional_, jump_to_native_ui);
dialog_model_->StartFlow(std::move(data), is_conditional_);

if (g_observer) {
g_observer->UIShown(this);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -86,10 +86,6 @@ class ChromeWebAuthenticationDelegate
content::BrowserContext* browser_context,
const url::Origin& caller_origin) override;
#endif
#if BUILDFLAG(IS_WIN)
void OperationSucceeded(content::BrowserContext* browser_context,
bool used_win_api) override;
#endif
#if BUILDFLAG(IS_MAC)
absl::optional<TouchIdAuthenticatorConfig> GetTouchIdAuthenticatorConfig(
content::BrowserContext* browser_context) override;
Expand Down
Loading

0 comments on commit 3906703

Please sign in to comment.