Commit 70589ab6 authored by Nina Satragno's avatar Nina Satragno Committed by Commit Bot

[webauthn] Do not always prompt for second tap if a PIN is needed

When resolving a webauthn request, skip the first touch when able if the only
authenticator detected will require PIN entry. The touch is skipped when only
one authenticator is detected.

If the user clicks the back arrow and starts the request over, the touch will
not be skipped.

The patch changes FidoRequestHandlerBase to wait until all discoveries are ready
before signalling transport availability to avoid race conditions, and the
StartOver logic to keep the state on the FidoRequestHandler.

Bug: 996958
Change-Id: I8c639320b5052675ae3aad7c389bff175b529bbc
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1800725
Commit-Queue: Nina Satragno <nsatragno@chromium.org>
Reviewed-by: default avatarMartin Kreichgauer <martinkr@google.com>
Cr-Commit-Position: refs/heads/master@{#702067}
parent a5871bf8
......@@ -548,7 +548,8 @@ AuthenticatorCommon::CreateRequestDelegate(std::string relying_party_id) {
render_frame_host_, relying_party_id_);
}
void AuthenticatorCommon::StartMakeCredentialRequest() {
void AuthenticatorCommon::StartMakeCredentialRequest(
bool allow_skipping_pin_touch) {
discovery_factory_ =
AuthenticatorEnvironmentImpl::GetInstance()->GetDiscoveryFactoryOverride(
static_cast<RenderFrameHostImpl*>(render_frame_host_)
......@@ -576,14 +577,17 @@ void AuthenticatorCommon::StartMakeCredentialRequest() {
connector_, discovery_factory_,
GetTransports(caller_origin_, transports_),
*ctap_make_credential_request_, *authenticator_selection_criteria_,
allow_skipping_pin_touch,
base::BindOnce(&AuthenticatorCommon::OnRegisterResponse,
weak_factory_.GetWeakPtr()));
request_delegate_->RegisterActionCallbacks(
base::BindOnce(&AuthenticatorCommon::OnCancelFromUI,
weak_factory_.GetWeakPtr()) /* cancel_callback */,
base::BindRepeating(&AuthenticatorCommon::StartMakeCredentialRequest,
weak_factory_.GetWeakPtr()) /* start_over_callback */,
base::BindRepeating(
&AuthenticatorCommon::StartMakeCredentialRequest,
weak_factory_.GetWeakPtr(),
/*allow_skipping_pin_touch=*/false) /* start_over_callback */,
base::BindRepeating(
&device::FidoRequestHandlerBase::StartAuthenticatorRequest,
request_->GetWeakPtr()) /* request_callback */,
......@@ -599,7 +603,8 @@ void AuthenticatorCommon::StartMakeCredentialRequest() {
request_->set_observer(request_delegate_.get());
}
void AuthenticatorCommon::StartGetAssertionRequest() {
void AuthenticatorCommon::StartGetAssertionRequest(
bool allow_skipping_pin_touch) {
discovery_factory_ =
AuthenticatorEnvironmentImpl::GetInstance()->GetDiscoveryFactoryOverride(
static_cast<RenderFrameHostImpl*>(render_frame_host_)
......@@ -637,14 +642,17 @@ void AuthenticatorCommon::StartGetAssertionRequest() {
request_ = std::make_unique<device::GetAssertionRequestHandler>(
connector_, discovery_factory_,
GetTransports(caller_origin_, transports_), *ctap_get_assertion_request_,
allow_skipping_pin_touch,
base::BindOnce(&AuthenticatorCommon::OnSignResponse,
weak_factory_.GetWeakPtr()));
request_delegate_->RegisterActionCallbacks(
base::BindOnce(&AuthenticatorCommon::OnCancelFromUI,
weak_factory_.GetWeakPtr()) /* cancel_callback */,
base::BindRepeating(&AuthenticatorCommon::StartGetAssertionRequest,
weak_factory_.GetWeakPtr()) /* start_over_callback */,
base::BindRepeating(
&AuthenticatorCommon::StartGetAssertionRequest,
weak_factory_.GetWeakPtr(),
/*allow_skipping_pin_touch=*/false) /* start_over_callback */,
base::BindRepeating(
&device::FidoRequestHandlerBase::StartAuthenticatorRequest,
request_->GetWeakPtr()) /* request_callback */,
......@@ -895,7 +903,7 @@ void AuthenticatorCommon::MakeCredential(
break;
}
StartMakeCredentialRequest();
StartMakeCredentialRequest(/*allow_skipping_pin_touch=*/true);
}
// mojom:Authenticator
......@@ -1008,7 +1016,7 @@ void AuthenticatorCommon::GetAssertion(
ctap_get_assertion_request_->is_u2f_only =
OriginIsCryptoTokenExtension(caller_origin_);
StartGetAssertionRequest();
StartGetAssertionRequest(/*allow_skipping_pin_touch=*/true);
}
void AuthenticatorCommon::IsUserVerifyingPlatformAuthenticatorAvailable(
......
......@@ -117,11 +117,11 @@ class CONTENT_EXPORT AuthenticatorCommon {
// Replaces the current |request_| with a |MakeCredentialRequestHandler|,
// effectively restarting the request.
void StartMakeCredentialRequest();
void StartMakeCredentialRequest(bool allow_skipping_pin_touch);
// Replaces the current |request_| with a |GetAssertionRequestHandler|,
// effectively restarting the request.
void StartGetAssertionRequest();
void StartGetAssertionRequest(bool allow_skipping_pin_touch);
bool IsFocused() const;
......
......@@ -114,6 +114,18 @@ class FidoBleDiscoveryTest : public ::testing::Test {
BluetoothAdapterFactory::SetAdapterForTesting(adapter_);
}
void ExpectSuccessfulStartScan() {
EXPECT_CALL(*adapter(), StartScanWithFilter_)
.WillOnce(testing::Invoke(
[](const device::BluetoothDiscoveryFilter* discovery_filter,
device::BluetoothAdapter::DiscoverySessionResultCallback&
callback) {
std::move(callback).Run(
/*is_error=*/false,
device::UMABluetoothDiscoverySessionOutcome::SUCCESS);
}));
}
FidoBleDiscovery* discovery() { return &discovery_; }
MockFidoDiscoveryObserver* observer() { return &observer_; }
MockBluetoothAdapter* adapter() {
......@@ -136,7 +148,8 @@ TEST_F(FidoBleDiscoveryTest,
SetMockBluetoothAdapter();
EXPECT_CALL(*adapter(), IsPresent()).WillOnce(Return(false));
EXPECT_CALL(*adapter(), SetPowered).Times(0);
EXPECT_CALL(*observer(), DiscoveryStarted(discovery(), false));
EXPECT_CALL(*observer(), DiscoveryStarted(discovery(), false,
std::vector<FidoAuthenticator*>()));
discovery()->Start();
task_environment_.FastForwardUntilNoTasksRemain();
}
......@@ -149,15 +162,9 @@ TEST_F(FidoBleDiscoveryTest, FidoBleDiscoveryResumeScanningAfterPoweredOn) {
// After BluetoothAdapter is powered on, we expect that discovery session
// starts again. Immediately calling the callback so that it does not hold a
// reference to the adapter.
EXPECT_CALL(*adapter(), StartScanWithFilter_)
.WillOnce(testing::Invoke(
[](const device::BluetoothDiscoveryFilter* discovery_filter,
device::BluetoothAdapter::DiscoverySessionResultCallback&
callback) {
std::move(callback).Run(
/*is_error=*/false,
device::UMABluetoothDiscoverySessionOutcome::SUCCESS);
}));
ExpectSuccessfulStartScan();
EXPECT_CALL(*observer(), DiscoveryStarted(discovery(), true,
std::vector<FidoAuthenticator*>()));
discovery()->Start();
task_environment_.FastForwardUntilNoTasksRemain();
adapter()->NotifyAdapterPoweredChanged(true);
......@@ -168,7 +175,7 @@ TEST_F(FidoBleDiscoveryTest, FidoBleDiscoveryNoAdapter) {
// simulating cases where the discovery is destroyed before obtaining a handle
// to an adapter. This should be handled gracefully and not result in a crash.
// We don't expect any calls to the notification methods.
EXPECT_CALL(*observer(), DiscoveryStarted(discovery(), _)).Times(0);
EXPECT_CALL(*observer(), DiscoveryStarted(discovery(), _, _)).Times(0);
EXPECT_CALL(*observer(), AuthenticatorAdded(discovery(), _)).Times(0);
EXPECT_CALL(*observer(), AuthenticatorRemoved(discovery(), _)).Times(0);
}
......@@ -191,11 +198,10 @@ TEST_F(BluetoothTest, FidoBleDiscoveryFindsKnownDevice) {
{
base::RunLoop run_loop;
auto quit = run_loop.QuitClosure();
EXPECT_CALL(
observer,
AuthenticatorAdded(&discovery,
IdMatches(BluetoothTestBase::kTestDeviceAddress1)));
EXPECT_CALL(observer, DiscoveryStarted(&discovery, true))
EXPECT_CALL(observer,
DiscoveryStarted(&discovery, true,
testing::ElementsAre(IdMatches(
BluetoothTestBase::kTestDeviceAddress1))))
.WillOnce(ReturnFromAsyncCall(quit));
discovery.Start();
......@@ -217,7 +223,8 @@ TEST_F(BluetoothTest, FidoBleDiscoveryFindsNewDevice) {
{
base::RunLoop run_loop;
auto quit = run_loop.QuitClosure();
EXPECT_CALL(observer, DiscoveryStarted(&discovery, true))
EXPECT_CALL(observer, DiscoveryStarted(&discovery, true,
std::vector<FidoAuthenticator*>()))
.WillOnce(ReturnFromAsyncCall(quit));
discovery.Start();
......@@ -261,7 +268,8 @@ TEST_F(BluetoothTest, FidoBleDiscoveryFindsUpdatedDevice) {
{
base::RunLoop run_loop;
auto quit = run_loop.QuitClosure();
EXPECT_CALL(observer, DiscoveryStarted(&discovery, true))
EXPECT_CALL(observer, DiscoveryStarted(&discovery, true,
std::vector<FidoAuthenticator*>()))
.WillOnce(ReturnFromAsyncCall(quit));
discovery.Start();
......@@ -306,7 +314,8 @@ TEST_F(BluetoothTest, FidoBleDiscoveryRejectsCableDevice) {
{
base::RunLoop run_loop;
auto quit = run_loop.QuitClosure();
EXPECT_CALL(observer, DiscoveryStarted(&discovery, true))
EXPECT_CALL(observer, DiscoveryStarted(&discovery, true,
std::vector<FidoAuthenticator*>()))
.WillOnce(ReturnFromAsyncCall(quit));
discovery.Start();
......@@ -355,13 +364,16 @@ TEST_F(FidoBleDiscoveryTest,
TEST_F(FidoBleDiscoveryTest, DiscoveryNotifiesObserverWhenDeviceInPairingMode) {
SetMockBluetoothAdapter();
EXPECT_CALL(*adapter(), IsPresent()).WillOnce(Return(true));
EXPECT_CALL(*adapter(), IsPowered()).WillOnce(Return(true));
auto mock_device = CreateMockFidoDevice();
::testing::InSequence sequence;
ExpectSuccessfulStartScan();
EXPECT_CALL(*observer(), DiscoveryStarted(discovery(), true, _));
const auto device_id = FidoBleDevice::GetIdForAddress(kDeviceAddress);
discovery()->Start();
task_environment_.FastForwardUntilNoTasksRemain();
::testing::InSequence sequence;
EXPECT_CALL(*observer(),
AuthenticatorAdded(discovery(), IdMatches(kDeviceAddress)));
adapter()->NotifyDeviceChanged(mock_device.get());
......@@ -378,13 +390,16 @@ TEST_F(FidoBleDiscoveryTest,
DiscoveryNotifiesObserverWhenDeviceInNonPairingMode) {
SetMockBluetoothAdapter();
EXPECT_CALL(*adapter(), IsPresent()).WillOnce(Return(true));
EXPECT_CALL(*adapter(), IsPowered()).WillOnce(Return(true));
auto mock_device = CreateMockFidoDevice();
::testing::InSequence sequence;
ExpectSuccessfulStartScan();
EXPECT_CALL(*observer(), DiscoveryStarted(discovery(), true, _));
const auto device_id = FidoBleDevice::GetIdForAddress(kDeviceAddress);
discovery()->Start();
task_environment_.FastForwardUntilNoTasksRemain();
::testing::InSequence sequence;
EXPECT_CALL(*observer(),
AuthenticatorAdded(discovery(), IdMatches(kDeviceAddress)));
adapter()->NotifyDeviceChanged(mock_device.get());
......@@ -429,9 +444,13 @@ TEST_F(FidoBleDiscoveryTest,
TEST_F(FidoBleDiscoveryTest, DiscoveryDoesNotDeleteDeviceOnAddressCollision) {
SetMockBluetoothAdapter();
EXPECT_CALL(*adapter(), IsPresent()).WillOnce(Return(true));
EXPECT_CALL(*adapter(), IsPowered()).WillOnce(Return(true));
auto mock_device = CreateMockFidoDevice();
auto changed_mock_device = CreateChangedMockFidoDevice();
ExpectSuccessfulStartScan();
EXPECT_CALL(*observer(), DiscoveryStarted(discovery(), true, _));
EXPECT_CALL(*observer(),
AuthenticatorAdded(discovery(), IdMatches(kDeviceAddress)));
......
......@@ -14,8 +14,8 @@
#include "device/bluetooth/bluetooth_adapter_factory.h"
#include "device/bluetooth/test/mock_bluetooth_adapter.h"
#include "device/bluetooth/test/mock_bluetooth_device.h"
#include "device/fido/fake_fido_discovery.h"
#include "device/fido/fido_authenticator.h"
#include "device/fido/fido_discovery_factory.h"
#include "device/fido/fido_request_handler_base.h"
#include "device/fido/fido_transport_protocol.h"
#include "device/fido/test_callback_receiver.h"
......@@ -70,6 +70,7 @@ class FakeFidoRequestHandlerBase : public FidoRequestHandlerBase {
fido_discovery_factory,
{FidoTransportProtocol::kBluetoothLowEnergy}) {
set_observer(observer);
Start();
}
void SimulateFidoRequestHandlerHasAuthenticator(bool simulate_authenticator) {
......@@ -95,6 +96,11 @@ class FidoBleAdapterManagerTest : public ::testing::Test {
public:
FidoBleAdapterManagerTest() {
BluetoothAdapterFactory::SetAdapterForTesting(adapter_);
fido_discovery_factory_->ForgeNextBleDiscovery(
test::FakeFidoDiscovery::StartMode::kAutomatic);
fake_request_handler_ = std::make_unique<FakeFidoRequestHandlerBase>(
mock_observer_.get(), fido_discovery_factory_.get());
}
MockBluetoothDevice* AddMockBluetoothDeviceToAdapter() {
......@@ -134,13 +140,10 @@ class FidoBleAdapterManagerTest : public ::testing::Test {
base::MakeRefCounted<::testing::NiceMock<MockBluetoothAdapter>>();
std::unique_ptr<MockObserver> mock_observer_ =
std::make_unique<MockObserver>();
std::unique_ptr<FidoDiscoveryFactory> fido_discovery_factory_ =
std::make_unique<FidoDiscoveryFactory>();
std::unique_ptr<test::FakeFidoDiscoveryFactory> fido_discovery_factory_ =
std::make_unique<test::FakeFidoDiscoveryFactory>();
std::unique_ptr<FakeFidoRequestHandlerBase> fake_request_handler_ =
std::make_unique<FakeFidoRequestHandlerBase>(
mock_observer_.get(),
fido_discovery_factory_.get());
std::unique_ptr<FakeFidoRequestHandlerBase> fake_request_handler_;
};
TEST_F(FidoBleAdapterManagerTest, AdapterNotPresent) {
......
......@@ -335,7 +335,7 @@ class FidoCableDiscoveryTest : public ::testing::Test {
TEST_F(FidoCableDiscoveryTest, TestDiscoveryFindsNewDevice) {
auto cable_discovery = CreateDiscovery();
NiceMock<MockFidoDiscoveryObserver> mock_observer;
EXPECT_CALL(mock_observer, AuthenticatorAdded(_, _));
EXPECT_CALL(mock_observer, DiscoveryStarted(_, _, testing::SizeIs(1)));
cable_discovery->set_observer(&mock_observer);
auto mock_adapter =
......@@ -354,7 +354,7 @@ TEST_F(FidoCableDiscoveryTest, TestDiscoveryFindsNewDevice) {
TEST_F(FidoCableDiscoveryTest, TestDiscoveryFindsNewAppleDevice) {
auto cable_discovery = CreateDiscovery();
NiceMock<MockFidoDiscoveryObserver> mock_observer;
EXPECT_CALL(mock_observer, AuthenticatorAdded(_, _));
EXPECT_CALL(mock_observer, DiscoveryStarted(_, _, testing::SizeIs(1)));
cable_discovery->set_observer(&mock_observer);
auto mock_adapter =
......@@ -376,6 +376,7 @@ TEST_F(FidoCableDiscoveryTest, TestDiscoveryFindsIncorrectDevice) {
auto cable_discovery = CreateDiscovery();
NiceMock<MockFidoDiscoveryObserver> mock_observer;
EXPECT_CALL(mock_observer, AuthenticatorAdded(_, _)).Times(0);
EXPECT_CALL(mock_observer, DiscoveryStarted(_, _, testing::IsEmpty()));
cable_discovery->set_observer(&mock_observer);
auto mock_adapter =
......@@ -413,7 +414,7 @@ TEST_F(FidoCableDiscoveryTest, TestDiscoveryWithMultipleEids) {
mock_adapter->ExpectDiscoveryWithScanCallback(kAuthenticatorEid);
NiceMock<MockFidoDiscoveryObserver> mock_observer;
EXPECT_CALL(mock_observer, AuthenticatorAdded(_, _));
EXPECT_CALL(mock_observer, DiscoveryStarted(_, _, testing::SizeIs(1)));
cable_discovery->set_observer(&mock_observer);
Sequence sequence;
......@@ -442,7 +443,7 @@ TEST_F(FidoCableDiscoveryTest, TestDiscoveryWithPartialAdvertisementSuccess) {
auto cable_discovery =
std::make_unique<FakeFidoCableDiscovery>(std::move(discovery_data));
NiceMock<MockFidoDiscoveryObserver> mock_observer;
EXPECT_CALL(mock_observer, AuthenticatorAdded(_, _));
EXPECT_CALL(mock_observer, DiscoveryStarted(_, _, testing::SizeIs(1)));
cable_discovery->set_observer(&mock_observer);
auto mock_adapter =
......@@ -475,6 +476,7 @@ TEST_F(FidoCableDiscoveryTest, TestDiscoveryWithAdvertisementFailures) {
NiceMock<MockFidoDiscoveryObserver> mock_observer;
EXPECT_CALL(mock_observer, AuthenticatorAdded(_, _)).Times(0);
EXPECT_CALL(mock_observer, DiscoveryStarted(_, _, testing::IsEmpty()));
cable_discovery->set_observer(&mock_observer);
auto mock_adapter =
......@@ -522,7 +524,7 @@ TEST_F(FidoCableDiscoveryTest, TestUnregisterAdvertisementUponDestruction) {
TEST_F(FidoCableDiscoveryTest, TestResumeDiscoveryAfterPoweredOn) {
auto cable_discovery = CreateDiscovery();
NiceMock<MockFidoDiscoveryObserver> mock_observer;
EXPECT_CALL(mock_observer, AuthenticatorAdded);
EXPECT_CALL(mock_observer, DiscoveryStarted(_, _, testing::SizeIs(1)));
cable_discovery->set_observer(&mock_observer);
auto mock_adapter =
......
......@@ -67,7 +67,8 @@ TEST_F(FakeFidoDiscoveryTest, StartDiscovery) {
ASSERT_TRUE(discovery.is_start_requested());
ASSERT_FALSE(discovery.is_running());
EXPECT_CALL(observer, DiscoveryStarted(&discovery, true));
EXPECT_CALL(observer, DiscoveryStarted(&discovery, true,
std::vector<FidoAuthenticator*>()));
discovery.WaitForCallToStartAndSimulateSuccess();
ASSERT_TRUE(discovery.is_running());
ASSERT_TRUE(discovery.is_start_requested());
......@@ -87,7 +88,8 @@ TEST_F(FakeFidoDiscoveryTest, WaitThenStartStopDiscovery) {
ASSERT_FALSE(discovery.is_running());
ASSERT_TRUE(discovery.is_start_requested());
EXPECT_CALL(observer, DiscoveryStarted(&discovery, true));
EXPECT_CALL(observer, DiscoveryStarted(&discovery, true,
std::vector<FidoAuthenticator*>()));
discovery.SimulateStarted(true);
ASSERT_TRUE(discovery.is_running());
ASSERT_TRUE(discovery.is_start_requested());
......@@ -105,7 +107,8 @@ TEST_F(FakeFidoDiscoveryTest, StartFail) {
ASSERT_FALSE(discovery.is_running());
ASSERT_TRUE(discovery.is_start_requested());
EXPECT_CALL(observer, DiscoveryStarted(&discovery, false));
EXPECT_CALL(observer, DiscoveryStarted(&discovery, false,
std::vector<FidoAuthenticator*>()));
discovery.SimulateStarted(false);
ASSERT_FALSE(discovery.is_running());
ASSERT_TRUE(discovery.is_start_requested());
......@@ -119,19 +122,16 @@ TEST_F(FakeFidoDiscoveryTest, AddDevice) {
discovery.set_observer(&observer);
discovery.Start();
auto device0 = std::make_unique<MockFidoDevice>();
EXPECT_CALL(*device0, GetId()).WillOnce(::testing::Return("device0"));
base::RunLoop device0_done;
EXPECT_CALL(observer, AuthenticatorAdded(&discovery, _))
.WillOnce(testing::InvokeWithoutArgs(
[&device0_done]() { device0_done.Quit(); }));
discovery.AddDevice(std::move(device0));
device0_done.Run();
::testing::Mock::VerifyAndClearExpectations(&observer);
EXPECT_CALL(observer, DiscoveryStarted(&discovery, true));
EXPECT_CALL(observer, DiscoveryStarted(&discovery, true, testing::SizeIs(1)))
.WillOnce(testing::InvokeWithoutArgs(
[&device0_done]() { device0_done.Quit(); }));
discovery.SimulateStarted(true);
device0_done.Run();
::testing::Mock::VerifyAndClearExpectations(&observer);
auto device1 = std::make_unique<MockFidoDevice>();
......
......@@ -38,13 +38,18 @@ void FidoDeviceDiscovery::NotifyDiscoveryStarted(bool success) {
state_ = State::kRunning;
if (!observer())
return;
observer()->DiscoveryStarted(this, success);
std::vector<FidoAuthenticator*> authenticators;
authenticators.reserve(authenticators_.size());
for (const auto& authenticator : authenticators_)
authenticators.push_back(authenticator.second.get());
observer()->DiscoveryStarted(this, success, std::move(authenticators));
}
void FidoDeviceDiscovery::NotifyAuthenticatorAdded(
FidoAuthenticator* authenticator) {
DCHECK_NE(state_, State::kIdle);
if (!observer())
if (!observer() || state_ != State::kRunning)
return;
observer()->AuthenticatorAdded(this, authenticator);
}
......@@ -52,7 +57,7 @@ void FidoDeviceDiscovery::NotifyAuthenticatorAdded(
void FidoDeviceDiscovery::NotifyAuthenticatorRemoved(
FidoAuthenticator* authenticator) {
DCHECK_NE(state_, State::kIdle);
if (!observer())
if (!observer() || state_ != State::kRunning)
return;
observer()->AuthenticatorRemoved(this, authenticator);
}
......
......@@ -76,7 +76,8 @@ TEST(FidoDiscoveryTest, TestNotificationsOnSuccessfulStart) {
EXPECT_FALSE(discovery.is_running());
::testing::Mock::VerifyAndClearExpectations(&discovery);
EXPECT_CALL(observer, DiscoveryStarted(&discovery, true));
EXPECT_CALL(observer, DiscoveryStarted(&discovery, true,
std::vector<FidoAuthenticator*>()));
discovery.NotifyDiscoveryStarted(true);
EXPECT_TRUE(discovery.is_start_requested());
EXPECT_TRUE(discovery.is_running());
......@@ -93,7 +94,8 @@ TEST(FidoDiscoveryTest, TestNotificationsOnFailedStart) {
discovery.Start();
task_environment_.RunUntilIdle();
EXPECT_CALL(observer, DiscoveryStarted(&discovery, false));
EXPECT_CALL(observer, DiscoveryStarted(&discovery, false,
std::vector<FidoAuthenticator*>()));
discovery.NotifyDiscoveryStarted(false);
EXPECT_TRUE(discovery.is_start_requested());
EXPECT_FALSE(discovery.is_running());
......@@ -112,19 +114,23 @@ TEST(FidoDiscoveryTest, TestAddRemoveDevices) {
auto* device0_raw = device0.get();
FidoAuthenticator* authenticator0 = nullptr;
base::RunLoop device0_done;
EXPECT_CALL(observer, AuthenticatorAdded(&discovery, _))
.WillOnce(DoAll(SaveArg<1>(&authenticator0),
testing::InvokeWithoutArgs(
[&device0_done]() { device0_done.Quit(); })));
EXPECT_CALL(observer, DiscoveryStarted(&discovery, true, _))
.WillOnce(testing::Invoke(
[&](auto* discovery, bool success, auto authenticators) {
EXPECT_EQ(1u, authenticators.size());
authenticator0 = authenticators[0];
device0_done.Quit();
}));
EXPECT_CALL(*device0, GetId()).WillRepeatedly(Return("device0"));
EXPECT_TRUE(discovery.AddDevice(std::move(device0)));
discovery.NotifyDiscoveryStarted(true);
EXPECT_EQ("device0", authenticator0->GetId());
EXPECT_EQ(device0_raw,
static_cast<FidoDeviceAuthenticator*>(authenticator0)->device());
device0_done.Run();
::testing::Mock::VerifyAndClearExpectations(&observer);
// Expect successful insertion.
// Expect successful insertion after starting.
base::RunLoop device1_done;
auto device1 = std::make_unique<MockFidoDevice>();
auto* device1_raw = device1.get();
......
......@@ -5,6 +5,8 @@
#ifndef DEVICE_FIDO_FIDO_DISCOVERY_BASE_H_
#define DEVICE_FIDO_FIDO_DISCOVERY_BASE_H_
#include <vector>
#include "base/component_export.h"
#include "base/logging.h"
#include "base/macros.h"
......@@ -23,13 +25,14 @@ class COMPONENT_EXPORT(DEVICE_FIDO) FidoDiscoveryBase {
virtual ~Observer();
// It is guaranteed that this is never invoked synchronously from Start().
virtual void DiscoveryStarted(FidoDiscoveryBase* discovery, bool success) {}
// It is guaranteed that AuthenticatorAdded/AuthenticatorRemoved() will not
// be invoked before the client of FidoDiscoveryBase calls
// FidoDiscoveryBase::Start(). However, for authenticators already known to
// the system at that point, AuthenticatorAdded() might already be called to
// reported already known devices.
// |authenticators| is the list of authenticators discovered upon start.
virtual void DiscoveryStarted(
FidoDiscoveryBase* discovery,
bool success,
std::vector<FidoAuthenticator*> authenticators = {}) {}
// Called after DiscoveryStarted for any devices discovered after
// initialization.
virtual void AuthenticatorAdded(FidoDiscoveryBase* discovery,
FidoAuthenticator* authenticator) = 0;
virtual void AuthenticatorRemoved(FidoDiscoveryBase* discovery,
......
......@@ -59,26 +59,6 @@ FidoRequestHandlerBase::FidoRequestHandlerBase(
void FidoRequestHandlerBase::InitDiscoveries(
const base::flat_set<FidoTransportProtocol>& available_transports) {
// The number of times |notify_observer_callback_| needs to be invoked before
// Observer::OnTransportAvailabilityEnumerated is dispatched. Essentially this
// is used to wait until the parts |transport_availability_info_| are
// filled out; the |notify_observer_callback_| is invoked once for each part
// once that part is ready, namely:
//
// 1) [If the platform authenticator is enabled] once the platform
// authenticator is ready.
// 2) [If BLE or caBLE are enabled] if Bluetooth adapter is present.
//
// On top of that, we wait for (3) an invocation that happens when the
// |observer_| is set, so that OnTransportAvailabilityEnumerated is never
// called before the observer is set.
size_t transport_info_callback_count = 1u;
#if defined(OS_WIN)
if (transport_availability_info_.has_win_native_api_authenticator)
++transport_info_callback_count;
#endif // defined(OS_WIN)
transport_availability_info_.available_transports = available_transports;
for (const auto transport : available_transports) {
std::unique_ptr<FidoDiscoveryBase> discovery =
......@@ -91,27 +71,35 @@ void FidoRequestHandlerBase::InitDiscoveries(
continue;
}
if (transport == FidoTransportProtocol::kInternal) {
++transport_info_callback_count;
}
discovery->set_observer(this);
discoveries_.push_back(std::move(discovery));
}
bool has_ble = false;
if (base::Contains(transport_availability_info_.available_transports,
FidoTransportProtocol::kCloudAssistedBluetoothLowEnergy) ||
base::Contains(transport_availability_info_.available_transports,
FidoTransportProtocol::kBluetoothLowEnergy)) {
++transport_info_callback_count;
has_ble = true;
base::SequencedTaskRunnerHandle::Get()->PostTask(
FROM_HERE,
base::BindOnce(&FidoRequestHandlerBase::ConstructBleAdapterPowerManager,
weak_factory_.GetWeakPtr()));
}
// Initialize |notify_observer_callback_| with the number of times it has to
// be invoked before Observer::OnTransportAvailabilityEnumerated is
// dispatched.
// Essentially this is used to wait until the parts
// |transport_availability_info_| are filled out; the
// |notify_observer_callback_| is invoked once for each discovery once it is
// ready, and additionally:
//
// 1) [If BLE or caBLE are enabled] if Bluetooth adapter is present.
// 2) When |observer_| is set, so that OnTransportAvailabilityEnumerated is
// never called before it is set.
notify_observer_callback_ = base::BarrierClosure(
transport_info_callback_count,
discoveries_.size() + has_ble + 1,
base::BindOnce(
&FidoRequestHandlerBase::NotifyObserverTransportAvailability,
weak_factory_.GetWeakPtr()));
......@@ -296,6 +284,17 @@ void FidoRequestHandlerBase::AuthenticatorPairingModeChanged(
}
}
void FidoRequestHandlerBase::DiscoveryStarted(
FidoDiscoveryBase* discovery,
bool success,
std::vector<FidoAuthenticator*> authenticators) {
for (auto* authenticator : authenticators)
AddAuthenticator(authenticator);
DCHECK(notify_observer_callback_);
notify_observer_callback_.Run();
}
void FidoRequestHandlerBase::AddAuthenticator(
FidoAuthenticator* authenticator) {
DCHECK(authenticator &&
......@@ -336,16 +335,8 @@ void FidoRequestHandlerBase::AddAuthenticator(
.win_native_ui_shows_resident_credential_notice =
static_cast<WinWebAuthnApiAuthenticator*>(authenticator)
->ShowsPrivacyNotice();
DCHECK(notify_observer_callback_);
notify_observer_callback_.Run();
}
#endif // defined(OS_WIN)
if (authenticator->AuthenticatorTransport() ==
FidoTransportProtocol::kInternal) {
DCHECK(notify_observer_callback_);
notify_observer_callback_.Run();
}
}
bool FidoRequestHandlerBase::HasAuthenticator(
......
......@@ -235,6 +235,10 @@ class COMPONENT_EXPORT(DEVICE_FIDO) FidoRequestHandlerBase
Observer* observer() const { return observer_; }
// FidoDiscoveryBase::Observer
void DiscoveryStarted(
FidoDiscoveryBase* discovery,
bool success,
std::vector<FidoAuthenticator*> authenticators) override;
void AuthenticatorAdded(FidoDiscoveryBase* discovery,
FidoAuthenticator* authenticator) override;
void AuthenticatorRemoved(FidoDiscoveryBase* discovery,
......
......@@ -576,6 +576,8 @@ TEST_F(FidoRequestHandlerTest,
discovery()->AddDevice(std::move(device0));
platform_discovery->AddDevice(std::move(device1));
discovery()->WaitForCallToStartAndSimulateSuccess();
platform_discovery->WaitForCallToStartAndSimulateSuccess();
task_environment_.FastForwardUntilNoTasksRemain();
callback().WaitForCallback();
......@@ -621,7 +623,8 @@ TEST_F(FidoRequestHandlerTest, TestWithPlatformAuthenticator) {
device->ExpectRequestAndRespondWith(std::vector<uint8_t>(),
CreateFakeSuccessDeviceResponse());
device->SetDeviceTransport(FidoTransportProtocol::kInternal);
auto* fake_discovery = fake_discovery_factory_.ForgeNextPlatformDiscovery();
auto* fake_discovery = fake_discovery_factory_.ForgeNextPlatformDiscovery(
test::FakeFidoDiscovery::StartMode::kAutomatic);
TestObserver observer;
auto request_handler = std::make_unique<FakeFidoRequestHandler>(
......@@ -655,6 +658,8 @@ TEST_F(FidoRequestHandlerTest, BleTransportAllowedIfBluetoothAdapterPresent) {
TestObserver observer;
auto request_handler = CreateFakeHandler();
ble_discovery()->WaitForCallToStartAndSimulateSuccess();
discovery()->WaitForCallToStartAndSimulateSuccess();
request_handler->set_observer(&observer);
observer.WaitForAndExpectAvailableTransportsAre(
......@@ -668,6 +673,8 @@ TEST_F(FidoRequestHandlerTest,
TestObserver observer;
auto request_handler = CreateFakeHandler();
ble_discovery()->WaitForCallToStartAndSimulateSuccess();
discovery()->WaitForCallToStartAndSimulateSuccess();
request_handler->set_observer(&observer);
observer.WaitForAndExpectAvailableTransportsAre(
......@@ -680,6 +687,8 @@ TEST_F(FidoRequestHandlerTest,
TestObserver observer;
auto request_handler = CreateFakeHandler();
ble_discovery()->WaitForCallToStartAndSimulateSuccess();
discovery()->WaitForCallToStartAndSimulateSuccess();
task_environment_.FastForwardUntilNoTasksRemain();
request_handler->set_observer(&observer);
......@@ -693,6 +702,7 @@ TEST_F(FidoRequestHandlerTest, EmbedderNotifiedWhenAuthenticatorIdChanges) {
TestObserver observer;
auto request_handler = CreateFakeHandler();
request_handler->set_observer(&observer);
discovery()->WaitForCallToStartAndSimulateSuccess();
ble_discovery()->WaitForCallToStartAndSimulateSuccess();
auto device = std::make_unique<MockFidoDevice>();
......@@ -718,6 +728,12 @@ TEST_F(FidoRequestHandlerTest, TransportAvailabilityOfWindowsAuthenticator) {
{FidoTransportProtocol::kUsbHumanInterfaceDevice},
&fake_discovery_factory_);
request_handler.set_observer(&observer);
// If the windows API is not enabled, the request is dispatched to the USB
// discovery. Simulate a success to fill the transport availability info.
if (!api_available)
discovery()->WaitForCallToStartAndSimulateSuccess();
task_environment_.FastForwardUntilNoTasksRemain();
auto transport_availability_info =
......
......@@ -101,7 +101,7 @@ class FidoGetAssertionHandlerTest : public ::testing::Test {
auto handler = std::make_unique<GetAssertionRequestHandler>(
nullptr /* connector */, fake_discovery_factory_.get(),
supported_transports_, std::move(request),
get_assertion_cb_.callback());
/*allow_skipping_pin_touch=*/true, get_assertion_cb_.callback());
return handler;
}
......@@ -759,7 +759,7 @@ TEST(GetAssertionRequestHandlerTest, IncorrectTransportType) {
nullptr /* connector */, &virtual_device_factory,
base::flat_set<FidoTransportProtocol>(
{FidoTransportProtocol::kUsbHumanInterfaceDevice}),
std::move(request), cb.callback());
std::move(request), /*allow_skipping_pin_touch=*/true, cb.callback());
task_environment.FastForwardUntilNoTasksRemain();
EXPECT_FALSE(cb.was_called());
......@@ -829,7 +829,7 @@ TEST(GetAssertionRequestHandlerWinTest, TestWinUsbDiscovery) {
{FidoTransportProtocol::kUsbHumanInterfaceDevice}),
CtapGetAssertionRequest(test_data::kRelyingPartyId,
test_data::kClientDataJson),
cb.callback());
/*allow_skipping_pin_touch=*/true, cb.callback());
// Register an observer that disables automatic dispatch. Dispatch to the
// (unimplemented) fake Windows API would immediately result in an invalid
// response.
......
......@@ -224,6 +224,7 @@ GetAssertionRequestHandler::GetAssertionRequestHandler(
FidoDiscoveryFactory* fido_discovery_factory,
const base::flat_set<FidoTransportProtocol>& supported_transports,
CtapGetAssertionRequest request,
bool allow_skipping_pin_touch,
CompletionCallback completion_callback)
: FidoRequestHandlerBase(
connector,
......@@ -232,7 +233,8 @@ GetAssertionRequestHandler::GetAssertionRequestHandler(
supported_transports,
GetTransportsAllowedByRP(request))),
completion_callback_(std::move(completion_callback)),
request_(std::move(request)) {
request_(std::move(request)),
allow_skipping_pin_touch_(allow_skipping_pin_touch) {
transport_availability_info().request_type =
FidoRequestHandlerBase::RequestType::kGetAssertion;
transport_availability_info().has_empty_allow_list =
......@@ -262,6 +264,11 @@ void GetAssertionRequestHandler::DispatchRequest(
switch (authenticator->WillNeedPINToGetAssertion(request_, observer())) {
case FidoAuthenticator::GetAssertionPINDisposition::kUsePIN:
// Skip asking for touch if this is the only available authenticator.
if (active_authenticators().size() == 1 && allow_skipping_pin_touch_) {
HandleTouch(authenticator);
return;
}
// A PIN will be needed. Just request a touch to let the user select
// this authenticator if they wish.
FIDO_LOG(DEBUG) << "Asking for touch from "
......@@ -548,7 +555,6 @@ void GetAssertionRequestHandler::OnHavePIN(std::string pin) {
if (authenticator_ == nullptr) {
// Authenticator was detached. The request will already have been canceled
// but this callback may have been waiting in a queue.
DCHECK(!completion_callback_);
return;
}
......
......@@ -64,6 +64,7 @@ class COMPONENT_EXPORT(DEVICE_FIDO) GetAssertionRequestHandler
FidoDiscoveryFactory* fido_discovery_factory,
const base::flat_set<FidoTransportProtocol>& supported_transports,
CtapGetAssertionRequest request_parameter,
bool allow_skipping_pin_touch,
CompletionCallback completion_callback);
~GetAssertionRequestHandler() override;
......@@ -108,6 +109,11 @@ class COMPONENT_EXPORT(DEVICE_FIDO) GetAssertionRequestHandler
CompletionCallback completion_callback_;
State state_ = State::kWaitingForTouch;
CtapGetAssertionRequest request_;
// If true, and if at the time the request is dispatched to the first
// authenticator no other authenticators are available, the request handler
// will skip the initial touch that is usually required to select a PIN
// protected authenticator.
bool allow_skipping_pin_touch_;
// authenticator_ points to the authenticator that will be used for this
// operation. It's only set after the user touches an authenticator to select
// it, after which point that authenticator will be used exclusively through
......
......@@ -4,8 +4,10 @@
#include "device/fido/hid/fido_hid_discovery.h"
#include <algorithm>
#include <string>
#include <utility>
#include <vector>
#include "base/test/task_environment.h"
#include "device/fido/fido_authenticator.h"
......@@ -52,13 +54,13 @@ TEST_F(FidoHidDiscoveryTest, TestAddRemoveDevice) {
fake_hid_manager_.AddFidoHidDevice("known");
EXPECT_CALL(observer, DiscoveryStarted(&discovery, true));
discovery.set_observer(&observer);
discovery.Start();
// Devices initially known to the service before discovery started should be
// reported as KNOWN.
EXPECT_CALL(observer, AuthenticatorAdded(&discovery, IdMatches("known")));
EXPECT_CALL(observer,
DiscoveryStarted(&discovery, true,
testing::ElementsAre(IdMatches("known"))));
discovery.set_observer(&observer);
discovery.Start();
task_environment_.RunUntilIdle();
// Devices added during the discovery should be reported as ADDED.
......
......@@ -26,13 +26,6 @@ void FidoTouchIdDiscovery::Start() {
return;
}
if (!TouchIdAuthenticator::IsAvailable(authenticator_config_)) {
observer()->DiscoveryStarted(this, /*success=*/false);
return;
}
observer()->DiscoveryStarted(this, /*success=*/true);
// Start() is currently invoked synchronously in the
// FidoRequestHandler ctor. Invoke AddAuthenticator() asynchronously
// to avoid hairpinning FidoRequestHandler::AuthenticatorAdded()
......@@ -43,9 +36,12 @@ void FidoTouchIdDiscovery::Start() {
}
void FidoTouchIdDiscovery::AddAuthenticator() {
DCHECK(TouchIdAuthenticator::IsAvailable(authenticator_config_));
if (!TouchIdAuthenticator::IsAvailable(authenticator_config_)) {
observer()->DiscoveryStarted(this, /*success=*/false);
return;
}
authenticator_ = TouchIdAuthenticator::Create(authenticator_config_);
observer()->AuthenticatorAdded(this, authenticator_.get());
observer()->DiscoveryStarted(this, /*success=*/true, {authenticator_.get()});
}
} // namespace mac
......
......@@ -86,9 +86,12 @@ class FidoMakeCredentialHandlerTest : public ::testing::Test {
auto handler = std::make_unique<MakeCredentialRequestHandler>(
nullptr, fake_discovery_factory_.get(), supported_transports_,
std::move(request_parameter),
std::move(authenticator_selection_criteria), cb_.callback());
if (pending_mock_platform_device_)
std::move(authenticator_selection_criteria),
/*allow_skipping_pin_touch=*/true, cb_.callback());
if (pending_mock_platform_device_) {
platform_discovery_->AddDevice(std::move(pending_mock_platform_device_));
platform_discovery_->WaitForCallToStartAndSimulateSuccess();
}
return handler;
}
......
......@@ -178,6 +178,7 @@ MakeCredentialRequestHandler::MakeCredentialRequestHandler(
const base::flat_set<FidoTransportProtocol>& supported_transports,
CtapMakeCredentialRequest request,
AuthenticatorSelectionCriteria authenticator_selection_criteria,
bool allow_skipping_pin_touch,
CompletionCallback completion_callback)
: FidoRequestHandlerBase(
connector,
......@@ -188,7 +189,8 @@ MakeCredentialRequestHandler::MakeCredentialRequestHandler(
completion_callback_(std::move(completion_callback)),
request_(std::move(request)),
authenticator_selection_criteria_(
std::move(authenticator_selection_criteria)) {
std::move(authenticator_selection_criteria)),
allow_skipping_pin_touch_(allow_skipping_pin_touch) {
transport_availability_info().request_type =
FidoRequestHandlerBase::RequestType::kMakeCredential;
......@@ -253,6 +255,11 @@ void MakeCredentialRequestHandler::DispatchRequest(
switch (authenticator->WillNeedPINToMakeCredential(request_, observer())) {
case MakeCredentialPINDisposition::kUsePIN:
case MakeCredentialPINDisposition::kSetPIN:
// Skip asking for touch if this is the only available authenticator.
if (active_authenticators().size() == 1 && allow_skipping_pin_touch_) {
HandleTouch(authenticator);
return;
}
// A PIN will be needed. Just request a touch to let the user select
// this authenticator if they wish.
authenticator->GetTouch(
......
......@@ -70,6 +70,7 @@ class COMPONENT_EXPORT(DEVICE_FIDO) MakeCredentialRequestHandler
const base::flat_set<FidoTransportProtocol>& supported_transports,
CtapMakeCredentialRequest request_parameter,
AuthenticatorSelectionCriteria authenticator_criteria,
bool allow_skipping_pin_touch,
CompletionCallback completion_callback);
~MakeCredentialRequestHandler() override;
......@@ -115,6 +116,9 @@ class COMPONENT_EXPORT(DEVICE_FIDO) MakeCredentialRequestHandler
State state_ = State::kWaitingForTouch;
CtapMakeCredentialRequest request_;
AuthenticatorSelectionCriteria authenticator_selection_criteria_;
// If true, the request handler may skip the first touch to select a device
// that will require a PIN.
bool allow_skipping_pin_touch_;
// authenticator_ points to the authenticator that will be used for this
// operation. It's only set after the user touches an authenticator to select
// it, after which point that authenticator will be used exclusively through
......
......@@ -6,6 +6,7 @@
#define DEVICE_FIDO_MOCK_FIDO_DISCOVERY_OBSERVER_H_
#include <string>
#include <vector>
#include "base/component_export.h"
#include "base/macros.h"
......@@ -21,7 +22,8 @@ class MockFidoDiscoveryObserver : public FidoDiscoveryBase::Observer {
MockFidoDiscoveryObserver();
~MockFidoDiscoveryObserver() override;
MOCK_METHOD2(DiscoveryStarted, void(FidoDiscoveryBase*, bool));
MOCK_METHOD3(DiscoveryStarted,
void(FidoDiscoveryBase*, bool, std::vector<FidoAuthenticator*>));
MOCK_METHOD2(DiscoveryStopped, void(FidoDiscoveryBase*, bool));
MOCK_METHOD2(AuthenticatorAdded,
void(FidoDiscoveryBase*, FidoAuthenticator*));
......
......@@ -27,13 +27,6 @@ void WinWebAuthnApiAuthenticatorDiscovery::Start() {
return;
}
if (!api_->IsAvailable()) {
observer()->DiscoveryStarted(this, /*success=*/false);
return;
}
observer()->DiscoveryStarted(this, /*success=*/true);
// Start() is currently invoked synchronously in the
// FidoRequestHandler ctor. Invoke AddAuthenticator() asynchronously
// to avoid hairpinning FidoRequestHandler::AuthenticatorAdded()
......@@ -46,12 +39,12 @@ void WinWebAuthnApiAuthenticatorDiscovery::Start() {
void WinWebAuthnApiAuthenticatorDiscovery::AddAuthenticator() {
if (!api_->IsAvailable()) {
NOTREACHED();
observer()->DiscoveryStarted(this, /*success=*/false);
return;
}
authenticator_ =
std::make_unique<WinWebAuthnApiAuthenticator>(parent_window_, api_);
observer()->AuthenticatorAdded(this, authenticator_.get());
observer()->DiscoveryStarted(this, /*success=*/true, {authenticator_.get()});
}
} // namespace device
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