Skip to content

GitLab

T
tangled
Commit 7cbada43 authored by Martin Kreichgauer's avatar Martin Kreichgauer Committed by Commit Bot
Browse files
Options

fido: fix lifetime issue with MaybeAddPlatformAuthenticatorCallback 

Currently, |AuthenticatorImpl| is binding
|CreatePlatformAuthenticatorIfAvailable| with |base::Unretained| for the
|MaybeAddPlatformAuthenticatorCallback| argument to the
|FidoRequestHandlerBase| ctor. The callback was originally invoked
synchronously from the ctor, but then recently changed to async
invocation to account for observer registration (crrev.com/c/1175418).

This is a potential callback lifetime issue because the
AuthenticatorImpl and its request handler can now theoretically get
destroyed before the callback gets invoked.

To work around this issue, replace the callback with a method on the
request handler invoked by the AuthenticatorImpl.

Bug: 875338
Change-Id: I089f0783a46791ae8d9d6bac5f0e663e52075b86
Reviewed-on: https://chromium-review.googlesource.com/1179081
Commit-Queue: Martin Kreichgauer <martinkr@google.com>
Reviewed-by: default avatarBalazs Engedy <engedy@chromium.org>
Reviewed-by: default avatarJun Choi <hongjunchoi@chromium.org>
Cr-Commit-Position: refs/heads/master@{#584288}
parent ff3c1bc9
Show whitespace changes
Inline Side-by-side
Showing with 67 additions and 170 deletions
content/browser/webauth/authenticator_impl.cc
View file @ 7cbada43
...@@ -490,9 +490,7 @@ void AuthenticatorImpl::MakeCredential( ...@@ -490,9 +490,7 @@ void AuthenticatorImpl::MakeCredential(
individual_attestation), individual_attestation),
std::move(authenticator_selection_criteria), std::move(authenticator_selection_criteria),
base::BindOnce(&AuthenticatorImpl::OnRegisterResponse, base::BindOnce(&AuthenticatorImpl::OnRegisterResponse,
weak_factory_.GetWeakPtr()), weak_factory_.GetWeakPtr()));
base::BindOnce(&AuthenticatorImpl::CreatePlatformAuthenticatorIfAvailable,
base::Unretained(this)));
request_delegate_->RegisterActionCallbacks( request_delegate_->RegisterActionCallbacks(
base::BindOnce(&AuthenticatorImpl::Cancel, base::BindOnce(&AuthenticatorImpl::Cancel,
...@@ -501,6 +499,9 @@ void AuthenticatorImpl::MakeCredential( ...@@ -501,6 +499,9 @@ void AuthenticatorImpl::MakeCredential(
&device::FidoRequestHandlerBase::StartAuthenticatorRequest, &device::FidoRequestHandlerBase::StartAuthenticatorRequest,
request_->GetWeakPtr()) /* request_callback */); request_->GetWeakPtr()) /* request_callback */);
request_->set_observer(request_delegate_.get()); request_->set_observer(request_delegate_.get());
request_->SetPlatformAuthenticatorOrMarkUnavailable(
CreatePlatformAuthenticatorIfAvailable());
} }
// mojom:Authenticator // mojom:Authenticator
...@@ -573,9 +574,7 @@ void AuthenticatorImpl::GetAssertion( ...@@ -573,9 +574,7 @@ void AuthenticatorImpl::GetAssertion(
std::move(options), std::move(options),
alternative_application_parameter_), alternative_application_parameter_),
base::BindOnce(&AuthenticatorImpl::OnSignResponse, base::BindOnce(&AuthenticatorImpl::OnSignResponse,
weak_factory_.GetWeakPtr()), weak_factory_.GetWeakPtr()));
base::BindOnce(&AuthenticatorImpl::CreatePlatformAuthenticatorIfAvailable,
base::Unretained(this)));
request_delegate_->RegisterActionCallbacks( request_delegate_->RegisterActionCallbacks(
base::BindOnce(&AuthenticatorImpl::Cancel, base::BindOnce(&AuthenticatorImpl::Cancel,
...@@ -584,6 +583,9 @@ void AuthenticatorImpl::GetAssertion( ...@@ -584,6 +583,9 @@ void AuthenticatorImpl::GetAssertion(
&device::FidoRequestHandlerBase::StartAuthenticatorRequest, &device::FidoRequestHandlerBase::StartAuthenticatorRequest,
request_->GetWeakPtr()) /* request_callback */); request_->GetWeakPtr()) /* request_callback */);
request_->set_observer(request_delegate_.get()); request_->set_observer(request_delegate_.get());
request_->SetPlatformAuthenticatorOrMarkUnavailable(
CreatePlatformAuthenticatorIfAvailable());
} }
void AuthenticatorImpl::IsUserVerifyingPlatformAuthenticatorAvailable( void AuthenticatorImpl::IsUserVerifyingPlatformAuthenticatorAvailable(
......
device/fido/fido_request_handler.h
View file @ 7cbada43
...@@ -36,19 +36,9 @@ class FidoRequestHandler : public FidoRequestHandlerBase { ...@@ -36,19 +36,9 @@ class FidoRequestHandler : public FidoRequestHandlerBase {
service_manager::Connector* connector, service_manager::Connector* connector,
const base::flat_set<FidoTransportProtocol>& available_transports, const base::flat_set<FidoTransportProtocol>& available_transports,
CompletionCallback completion_callback) CompletionCallback completion_callback)
: FidoRequestHandler(connector, : FidoRequestHandlerBase(connector, available_transports),
available_transports,
std::move(completion_callback),
AddPlatformAuthenticatorCallback()) {}
FidoRequestHandler(
service_manager::Connector* connector,
const base::flat_set<FidoTransportProtocol>& available_transports,
CompletionCallback completion_callback,
AddPlatformAuthenticatorCallback add_platform_authenticator)
: FidoRequestHandlerBase(connector,
available_transports,
std::move(add_platform_authenticator)),
completion_callback_(std::move(completion_callback)) {} completion_callback_(std::move(completion_callback)) {}
~FidoRequestHandler() override { ~FidoRequestHandler() override {
if (!is_complete()) if (!is_complete())
CancelOngoingTasks(); CancelOngoingTasks();
......
device/fido/fido_request_handler_base.cc
View file @ 7cbada43
...@@ -44,16 +44,7 @@ FidoRequestHandlerBase::TransportAvailabilityObserver:: ...@@ -44,16 +44,7 @@ FidoRequestHandlerBase::TransportAvailabilityObserver::
FidoRequestHandlerBase::FidoRequestHandlerBase( FidoRequestHandlerBase::FidoRequestHandlerBase(
service_manager::Connector* connector, service_manager::Connector* connector,
const base::flat_set<FidoTransportProtocol>& available_transports) const base::flat_set<FidoTransportProtocol>& available_transports)
: FidoRequestHandlerBase(connector, : weak_factory_(this) {
available_transports,
AddPlatformAuthenticatorCallback()) {}
FidoRequestHandlerBase::FidoRequestHandlerBase(
service_manager::Connector* connector,
const base::flat_set<FidoTransportProtocol>& available_transports,
AddPlatformAuthenticatorCallback add_platform_authenticator)
: add_platform_authenticator_(std::move(add_platform_authenticator)),
weak_factory_(this) {
// The number of times |notify_observer_callback_| needs to be invoked before // The number of times |notify_observer_callback_| needs to be invoked before
// Observer::OnTransportAvailabilityEnumerated is dispatched. Essentially this // Observer::OnTransportAvailabilityEnumerated is dispatched. Essentially this
// is used to wait until all the parts of |transport_availability_info_| are // is used to wait until all the parts of |transport_availability_info_| are
...@@ -79,8 +70,8 @@ FidoRequestHandlerBase::FidoRequestHandlerBase( ...@@ -79,8 +70,8 @@ FidoRequestHandlerBase::FidoRequestHandlerBase(
} }
if (transport == FidoTransportProtocol::kInternal) { if (transport == FidoTransportProtocol::kInternal) {
// Internal authenticators are injected through // Platform authenticator availability is always indicated by
// AddPlatformAuthenticatorCallback. // |AuthenticatorImpl|.
continue; continue;
} }
...@@ -139,15 +130,6 @@ base::WeakPtr<FidoRequestHandlerBase> FidoRequestHandlerBase::GetWeakPtr() { ...@@ -139,15 +130,6 @@ base::WeakPtr<FidoRequestHandlerBase> FidoRequestHandlerBase::GetWeakPtr() {
void FidoRequestHandlerBase::Start() { void FidoRequestHandlerBase::Start() {
for (const auto& discovery : discoveries_) for (const auto& discovery : discoveries_)
discovery->Start(); discovery->Start();
// Post |MaybeAddPlatformAuthenticator| into its own task. This avoids
// FidoAuthenticatorAdded() to be called synchronously from the constructor of
// FidoRequestHandlerBase prior to any TransportAvailabilityObserver being
// set.
base::SequencedTaskRunnerHandle::Get()->PostTask(
FROM_HERE,
base::BindOnce(&FidoRequestHandlerBase::MaybeAddPlatformAuthenticator,
weak_factory_.GetWeakPtr()));
} }
void FidoRequestHandlerBase::DiscoveryStarted(FidoDiscovery* discovery, void FidoRequestHandlerBase::DiscoveryStarted(FidoDiscovery* discovery,
...@@ -225,15 +207,13 @@ void FidoRequestHandlerBase::AddAuthenticator( ...@@ -225,15 +207,13 @@ void FidoRequestHandlerBase::AddAuthenticator(
GetWeakPtr(), authenticator_ptr)); GetWeakPtr(), authenticator_ptr));
} }
void FidoRequestHandlerBase::MaybeAddPlatformAuthenticator() { void FidoRequestHandlerBase::SetPlatformAuthenticatorOrMarkUnavailable(
std::unique_ptr<FidoAuthenticator> authenticator; std::unique_ptr<FidoAuthenticator> authenticator) {
if (add_platform_authenticator_ && if (authenticator &&
base::ContainsKey(transport_availability_info_.available_transports, base::ContainsKey(transport_availability_info_.available_transports,
FidoTransportProtocol::kInternal)) { FidoTransportProtocol::kInternal)) {
authenticator = std::move(add_platform_authenticator_).Run(); DCHECK(authenticator->AuthenticatorTransport() ==
} FidoTransportProtocol::kInternal);
if (authenticator) {
AddAuthenticator(std::move(authenticator)); AddAuthenticator(std::move(authenticator));
} else { } else {
transport_availability_info_.available_transports.erase( transport_availability_info_.available_transports.erase(
......
device/fido/fido_request_handler_base.h
View file @ 7cbada43
...@@ -43,8 +43,6 @@ class COMPONENT_EXPORT(DEVICE_FIDO) FidoRequestHandlerBase ...@@ -43,8 +43,6 @@ class COMPONENT_EXPORT(DEVICE_FIDO) FidoRequestHandlerBase
public: public:
using AuthenticatorMap = using AuthenticatorMap =
std::map<std::string, std::unique_ptr<FidoAuthenticator>, std::less<>>; std::map<std::string, std::unique_ptr<FidoAuthenticator>, std::less<>>;
using AddPlatformAuthenticatorCallback =
base::OnceCallback<std::unique_ptr<FidoAuthenticator>()>;
using RequestCallback = base::RepeatingCallback<void(const std::string&)>; using RequestCallback = base::RepeatingCallback<void(const std::string&)>;
enum class RequestType { kMakeCredential, kGetAssertion }; enum class RequestType { kMakeCredential, kGetAssertion };
...@@ -96,10 +94,6 @@ class COMPONENT_EXPORT(DEVICE_FIDO) FidoRequestHandlerBase ...@@ -96,10 +94,6 @@ class COMPONENT_EXPORT(DEVICE_FIDO) FidoRequestHandlerBase
FidoRequestHandlerBase( FidoRequestHandlerBase(
service_manager::Connector* connector, service_manager::Connector* connector,
const base::flat_set<FidoTransportProtocol>& available_transports); const base::flat_set<FidoTransportProtocol>& available_transports);
FidoRequestHandlerBase(
service_manager::Connector* connector,
const base::flat_set<FidoTransportProtocol>& available_transports,
AddPlatformAuthenticatorCallback add_platform_authenticator);
~FidoRequestHandlerBase() override; ~FidoRequestHandlerBase() override;
// Triggers DispatchRequest() if |active_authenticators_| hold // Triggers DispatchRequest() if |active_authenticators_| hold
...@@ -128,6 +122,12 @@ class COMPONENT_EXPORT(DEVICE_FIDO) FidoRequestHandlerBase ...@@ -128,6 +122,12 @@ class COMPONENT_EXPORT(DEVICE_FIDO) FidoRequestHandlerBase
notify_observer_callback_.Run(); notify_observer_callback_.Run();
} }
// Set the platform authenticator for this request, if one is available.
// |AuthenticatorImpl| must call this method after invoking |set_oberver| even
// if no platform authenticator is available, in which case it passes nullptr.
void SetPlatformAuthenticatorOrMarkUnavailable(
std::unique_ptr<FidoAuthenticator> authenticator);
TransportAvailabilityInfo& transport_availability_info() { TransportAvailabilityInfo& transport_availability_info() {
return transport_availability_info_; return transport_availability_info_;
} }
...@@ -158,7 +158,6 @@ class COMPONENT_EXPORT(DEVICE_FIDO) FidoRequestHandlerBase ...@@ -158,7 +158,6 @@ class COMPONENT_EXPORT(DEVICE_FIDO) FidoRequestHandlerBase
void BluetoothAdapterPowerChanged(bool is_powered_on) final; void BluetoothAdapterPowerChanged(bool is_powered_on) final;
void AddAuthenticator(std::unique_ptr<FidoAuthenticator> authenticator); void AddAuthenticator(std::unique_ptr<FidoAuthenticator> authenticator);
void MaybeAddPlatformAuthenticator();
void NotifyObserverTransportAvailability(); void NotifyObserverTransportAvailability();
AuthenticatorMap active_authenticators_; AuthenticatorMap active_authenticators_;
...@@ -166,7 +165,6 @@ class COMPONENT_EXPORT(DEVICE_FIDO) FidoRequestHandlerBase ...@@ -166,7 +165,6 @@ class COMPONENT_EXPORT(DEVICE_FIDO) FidoRequestHandlerBase
TransportAvailabilityObserver* observer_ = nullptr; TransportAvailabilityObserver* observer_ = nullptr;
TransportAvailabilityInfo transport_availability_info_; TransportAvailabilityInfo transport_availability_info_;
base::RepeatingClosure notify_observer_callback_; base::RepeatingClosure notify_observer_callback_;
AddPlatformAuthenticatorCallback add_platform_authenticator_;
base::WeakPtrFactory<FidoRequestHandlerBase> weak_factory_; base::WeakPtrFactory<FidoRequestHandlerBase> weak_factory_;
DISALLOW_COPY_AND_ASSIGN(FidoRequestHandlerBase); DISALLOW_COPY_AND_ASSIGN(FidoRequestHandlerBase);
......
device/fido/fido_request_handler_unittest.cc
View file @ 7cbada43
...@@ -137,15 +137,11 @@ class FakeFidoAuthenticator : public FidoDeviceAuthenticator { ...@@ -137,15 +137,11 @@ class FakeFidoAuthenticator : public FidoDeviceAuthenticator {
class FakeFidoRequestHandler : public FidoRequestHandler<std::vector<uint8_t>> { class FakeFidoRequestHandler : public FidoRequestHandler<std::vector<uint8_t>> {
public: public:
FakeFidoRequestHandler( FakeFidoRequestHandler(const base::flat_set<FidoTransportProtocol>& protocols,
const base::flat_set<FidoTransportProtocol>& protocols, FakeHandlerCallback callback)
FakeHandlerCallback callback,
AddPlatformAuthenticatorCallback add_platform_authenticator =
AddPlatformAuthenticatorCallback())
: FidoRequestHandler(nullptr /* connector */, : FidoRequestHandler(nullptr /* connector */,
protocols, protocols,
std::move(callback), std::move(callback)),
std::move(add_platform_authenticator)),
weak_factory_(this) { weak_factory_(this) {
Start(); Start();
} }
...@@ -191,21 +187,13 @@ class FidoRequestHandlerTest : public ::testing::Test { ...@@ -191,21 +187,13 @@ class FidoRequestHandlerTest : public ::testing::Test {
std::unique_ptr<FakeFidoRequestHandler> CreateFakeHandler() { std::unique_ptr<FakeFidoRequestHandler> CreateFakeHandler() {
ForgeNextHidDiscovery(); ForgeNextHidDiscovery();
return std::make_unique<FakeFidoRequestHandler>( auto handler = std::make_unique<FakeFidoRequestHandler>(
base::flat_set<FidoTransportProtocol>( base::flat_set<FidoTransportProtocol>(
{FidoTransportProtocol::kUsbHumanInterfaceDevice, {FidoTransportProtocol::kUsbHumanInterfaceDevice,
FidoTransportProtocol::kBluetoothLowEnergy}), FidoTransportProtocol::kBluetoothLowEnergy}),
cb_.callback()); cb_.callback());
} handler->SetPlatformAuthenticatorOrMarkUnavailable(nullptr);
return handler;
std::unique_ptr<FakeFidoRequestHandler>
CreateFakeHandlerWithPlatformAuthenticatorCallback(
FidoRequestHandlerBase::AddPlatformAuthenticatorCallback
add_platform_authenticator) {
return std::make_unique<FakeFidoRequestHandler>(
base::flat_set<FidoTransportProtocol>(
{FidoTransportProtocol::kInternal}),
cb_.callback(), std::move(add_platform_authenticator));
} }
test::FakeFidoDiscovery* discovery() const { return discovery_; } test::FakeFidoDiscovery* discovery() const { return discovery_; }
...@@ -268,8 +256,8 @@ TEST_F(FidoRequestHandlerTest, TestAuthenticatorHandlerReset) { ...@@ -268,8 +256,8 @@ TEST_F(FidoRequestHandlerTest, TestAuthenticatorHandlerReset) {
request_handler.reset(); request_handler.reset();
} }
// Test a scenario where 2 devices are connected and a response is received from // Test a scenario where 2 devices are connected and a response is received
// only a single device(device1) and the remaining device hangs. // from only a single device(device1) and the remaining device hangs.
TEST_F(FidoRequestHandlerTest, TestRequestWithMultipleDevices) { TEST_F(FidoRequestHandlerTest, TestRequestWithMultipleDevices) {
auto request_handler = CreateFakeHandler(); auto request_handler = CreateFakeHandler();
discovery()->WaitForCallToStartAndSimulateSuccess(); discovery()->WaitForCallToStartAndSimulateSuccess();
...@@ -342,11 +330,11 @@ TEST_F(FidoRequestHandlerTest, TestRequestWithMultipleSuccessResponses) { ...@@ -342,11 +330,11 @@ TEST_F(FidoRequestHandlerTest, TestRequestWithMultipleSuccessResponses) {
} }
// Test a scenario where 3 devices respond with a processing error, an UP(user // Test a scenario where 3 devices respond with a processing error, an UP(user
// presence) verified failure response with small time delay, and an UP verified // presence) verified failure response with small time delay, and an UP
// failure response with big time delay, respectively. Request for device with // verified failure response with big time delay, respectively. Request for
// processing error should be immediately dropped. Also, for UP verified // device with processing error should be immediately dropped. Also, for UP
// failures, the first received response should be passed on to the relying // verified failures, the first received response should be passed on to the
// party and cancel command should be sent to the remaining device. // relying party and cancel command should be sent to the remaining device.
TEST_F(FidoRequestHandlerTest, TestRequestWithMultipleFailureResponses) { TEST_F(FidoRequestHandlerTest, TestRequestWithMultipleFailureResponses) {
auto request_handler = CreateFakeHandler(); auto request_handler = CreateFakeHandler();
discovery()->WaitForCallToStartAndSimulateSuccess(); discovery()->WaitForCallToStartAndSimulateSuccess();
...@@ -395,9 +383,9 @@ TEST_F(FidoRequestHandlerTest, TestRequestWithMultipleFailureResponses) { ...@@ -395,9 +383,9 @@ TEST_F(FidoRequestHandlerTest, TestRequestWithMultipleFailureResponses) {
callback().status()); callback().status());
} }
// Requests should be dispatched to the authenticator returned from the // Requests should be dispatched to the authenticator passed to
// AddPlatformAuthenticatorCallback if one is passed. // SetPlatformAuthenticatorOrMarkUnavailable.
TEST_F(FidoRequestHandlerTest, TestPlatformAuthenticatorCallback) { TEST_F(FidoRequestHandlerTest, TestSetPlatformAuthenticator) {
// A platform authenticator usually wouldn't usually use a FidoDevice, but // A platform authenticator usually wouldn't usually use a FidoDevice, but
// that's not the point of the test here. The test is only trying to ensure // that's not the point of the test here. The test is only trying to ensure
// the authenticator gets injected and used. // the authenticator gets injected and used.
...@@ -406,18 +394,16 @@ TEST_F(FidoRequestHandlerTest, TestPlatformAuthenticatorCallback) { ...@@ -406,18 +394,16 @@ TEST_F(FidoRequestHandlerTest, TestPlatformAuthenticatorCallback) {
// Device returns success response. // Device returns success response.
device->ExpectRequestAndRespondWith(std::vector<uint8_t>(), device->ExpectRequestAndRespondWith(std::vector<uint8_t>(),
CreateFakeSuccessDeviceResponse()); CreateFakeSuccessDeviceResponse());
device->SetDeviceTransport(FidoTransportProtocol::kInternal);
FidoRequestHandlerBase::AddPlatformAuthenticatorCallback auto authenticator = std::make_unique<FakeFidoAuthenticator>(device.get());
make_platform_authenticator = base::BindOnce(
[](FidoDevice* device) -> std::unique_ptr<FidoAuthenticator> {
return std::make_unique<FakeFidoAuthenticator>(device);
},
device.get());
TestTransportAvailabilityObserver observer; TestTransportAvailabilityObserver observer;
auto request_handler = CreateFakeHandlerWithPlatformAuthenticatorCallback( auto request_handler = std::make_unique<FakeFidoRequestHandler>(
std::move(make_platform_authenticator)); base::flat_set<FidoTransportProtocol>({FidoTransportProtocol::kInternal}),
callback().callback());
request_handler->set_observer(&observer); request_handler->set_observer(&observer);
request_handler->SetPlatformAuthenticatorOrMarkUnavailable(
std::move(authenticator));
observer.WaitForAndExpectAvailableTransportsAre( observer.WaitForAndExpectAvailableTransportsAre(
{FidoTransportProtocol::kInternal}); {FidoTransportProtocol::kInternal});
...@@ -427,13 +413,14 @@ TEST_F(FidoRequestHandlerTest, TestPlatformAuthenticatorCallback) { ...@@ -427,13 +413,14 @@ TEST_F(FidoRequestHandlerTest, TestPlatformAuthenticatorCallback) {
EXPECT_EQ(FidoReturnCode::kSuccess, callback().status()); EXPECT_EQ(FidoReturnCode::kSuccess, callback().status());
} }
TEST_F(FidoRequestHandlerTest, TEST_F(FidoRequestHandlerTest, InternalTransportDisallowedIfMarkedUnavailable) {
InternalTransportDisallowedIfFactoryYieldsNoAuthenticator) {
TestTransportAvailabilityObserver observer; TestTransportAvailabilityObserver observer;
auto request_handler = auto request_handler = std::make_unique<FakeFidoRequestHandler>(
CreateFakeHandlerWithPlatformAuthenticatorCallback(base::BindOnce( base::flat_set<FidoTransportProtocol>({FidoTransportProtocol::kInternal}),
[]() -> std::unique_ptr<FidoAuthenticator> { return nullptr; })); callback().callback());
request_handler->set_observer(&observer); request_handler->set_observer(&observer);
request_handler->SetPlatformAuthenticatorOrMarkUnavailable(nullptr);
observer.WaitForAndExpectAvailableTransportsAre({}); observer.WaitForAndExpectAvailableTransportsAre({});
} }
......
device/fido/get_assertion_handler_unittest.cc
View file @ 7cbada43
...@@ -76,12 +76,12 @@ class FidoGetAssertionHandlerTest : public ::testing::Test { ...@@ -76,12 +76,12 @@ class FidoGetAssertionHandlerTest : public ::testing::Test {
CreateGetAssertionHandlerWithRequest(CtapGetAssertionRequest request) { CreateGetAssertionHandlerWithRequest(CtapGetAssertionRequest request) {
ForgeDiscoveries(); ForgeDiscoveries();
return std::make_unique<GetAssertionRequestHandler>( auto handler = std::make_unique<GetAssertionRequestHandler>(
nullptr /* connector */, supported_transports_, std::move(request), nullptr /* connector */, supported_transports_, std::move(request),
get_assertion_cb_.callback(), get_assertion_cb_.callback());
base::BindOnce( handler->SetPlatformAuthenticatorOrMarkUnavailable(
&FidoGetAssertionHandlerTest::CreatePlatformAuthenticator, CreatePlatformAuthenticator());
base::Unretained(this))); return handler;
} }
void ExpectAllowedTransportsForRequestAre( void ExpectAllowedTransportsForRequestAre(
...@@ -94,8 +94,6 @@ class FidoGetAssertionHandlerTest : public ::testing::Test { ...@@ -94,8 +94,6 @@ class FidoGetAssertionHandlerTest : public ::testing::Test {
ble_discovery()->WaitForCallToStartAndSimulateSuccess(); ble_discovery()->WaitForCallToStartAndSimulateSuccess();
if (base::ContainsKey(transports, Transport::kNearFieldCommunication)) if (base::ContainsKey(transports, Transport::kNearFieldCommunication))
nfc_discovery()->WaitForCallToStartAndSimulateSuccess(); nfc_discovery()->WaitForCallToStartAndSimulateSuccess();
if (base::ContainsKey(transports, Transport::kInternal))
platform_authenticator_factory().WaitForCallback();
scoped_task_environment_.FastForwardUntilNoTasksRemain(); scoped_task_environment_.FastForwardUntilNoTasksRemain();
EXPECT_FALSE(get_assertion_callback().was_called()); EXPECT_FALSE(get_assertion_callback().was_called());
...@@ -106,8 +104,6 @@ class FidoGetAssertionHandlerTest : public ::testing::Test { ...@@ -106,8 +104,6 @@ class FidoGetAssertionHandlerTest : public ::testing::Test {
EXPECT_FALSE(ble_discovery()->is_start_requested()); EXPECT_FALSE(ble_discovery()->is_start_requested());
if (!base::ContainsKey(transports, Transport::kNearFieldCommunication)) if (!base::ContainsKey(transports, Transport::kNearFieldCommunication))
EXPECT_FALSE(nfc_discovery()->is_start_requested()); EXPECT_FALSE(nfc_discovery()->is_start_requested());
if (!base::ContainsKey(transports, Transport::kInternal))
EXPECT_FALSE(platform_authenticator_factory().was_called());
// Even with FidoTransportProtocol::kInternal allowed, unless the platform // Even with FidoTransportProtocol::kInternal allowed, unless the platform
// authenticator factory returns a FidoAuthenticator instance (which it will // authenticator factory returns a FidoAuthenticator instance (which it will
...@@ -140,10 +136,6 @@ class FidoGetAssertionHandlerTest : public ::testing::Test { ...@@ -140,10 +136,6 @@ class FidoGetAssertionHandlerTest : public ::testing::Test {
mock_platform_device_ = std::move(device); mock_platform_device_ = std::move(device);
} }
test::TestCallbackReceiver<>& platform_authenticator_factory() {
return create_platform_authenticator_receiver_;
}
void set_supported_transports( void set_supported_transports(
base::flat_set<FidoTransportProtocol> transports) { base::flat_set<FidoTransportProtocol> transports) {
supported_transports_ = std::move(transports); supported_transports_ = std::move(transports);
...@@ -151,7 +143,6 @@ class FidoGetAssertionHandlerTest : public ::testing::Test { ...@@ -151,7 +143,6 @@ class FidoGetAssertionHandlerTest : public ::testing::Test {
protected: protected:
std::unique_ptr<FidoAuthenticator> CreatePlatformAuthenticator() { std::unique_ptr<FidoAuthenticator> CreatePlatformAuthenticator() {
create_platform_authenticator_receiver_.callback().Run();
if (!mock_platform_device_) if (!mock_platform_device_)
return nullptr; return nullptr;
return std::make_unique<FidoDeviceAuthenticator>( return std::make_unique<FidoDeviceAuthenticator>(
...@@ -166,7 +157,6 @@ class FidoGetAssertionHandlerTest : public ::testing::Test { ...@@ -166,7 +157,6 @@ class FidoGetAssertionHandlerTest : public ::testing::Test {
test::FakeFidoDiscovery* ble_discovery_; test::FakeFidoDiscovery* ble_discovery_;
test::FakeFidoDiscovery* nfc_discovery_; test::FakeFidoDiscovery* nfc_discovery_;
std::unique_ptr<MockFidoDevice> mock_platform_device_; std::unique_ptr<MockFidoDevice> mock_platform_device_;
test::TestCallbackReceiver<> create_platform_authenticator_receiver_;
TestGetAssertionRequestCallback get_assertion_cb_; TestGetAssertionRequestCallback get_assertion_cb_;
base::flat_set<FidoTransportProtocol> supported_transports_ = base::flat_set<FidoTransportProtocol> supported_transports_ =
GetTestableTransportProtocols(); GetTestableTransportProtocols();
......
device/fido/get_assertion_request_handler.cc
View file @ 7cbada43
...@@ -151,25 +151,12 @@ GetAssertionRequestHandler::GetAssertionRequestHandler( ...@@ -151,25 +151,12 @@ GetAssertionRequestHandler::GetAssertionRequestHandler(
const base::flat_set<FidoTransportProtocol>& supported_transports, const base::flat_set<FidoTransportProtocol>& supported_transports,
CtapGetAssertionRequest request, CtapGetAssertionRequest request,
SignResponseCallback completion_callback) SignResponseCallback completion_callback)
: GetAssertionRequestHandler(connector,
supported_transports,
std::move(request),
std::move(completion_callback),
AddPlatformAuthenticatorCallback()) {}
GetAssertionRequestHandler::GetAssertionRequestHandler(
service_manager::Connector* connector,
const base::flat_set<FidoTransportProtocol>& supported_transports,
CtapGetAssertionRequest request,
SignResponseCallback completion_callback,
AddPlatformAuthenticatorCallback add_platform_authenticator)
: FidoRequestHandler( : FidoRequestHandler(
connector, connector,
base::STLSetIntersection<base::flat_set<FidoTransportProtocol>>( base::STLSetIntersection<base::flat_set<FidoTransportProtocol>>(
supported_transports, supported_transports,
GetTransportsAllowedByRP(request)), GetTransportsAllowedByRP(request)),
std::move(completion_callback), std::move(completion_callback)),
std::move(add_platform_authenticator)),
request_(std::move(request)), request_(std::move(request)),
weak_factory_(this) { weak_factory_(this) {
transport_availability_info().rp_id = request.rp_id(); transport_availability_info().rp_id = request.rp_id();
......
device/fido/get_assertion_request_handler.h
View file @ 7cbada43
...@@ -38,12 +38,6 @@ class COMPONENT_EXPORT(DEVICE_FIDO) GetAssertionRequestHandler ...@@ -38,12 +38,6 @@ class COMPONENT_EXPORT(DEVICE_FIDO) GetAssertionRequestHandler
const base::flat_set<FidoTransportProtocol>& supported_transports, const base::flat_set<FidoTransportProtocol>& supported_transports,
CtapGetAssertionRequest request_parameter, CtapGetAssertionRequest request_parameter,
SignResponseCallback completion_callback); SignResponseCallback completion_callback);
GetAssertionRequestHandler(
service_manager::Connector* connector,
const base::flat_set<FidoTransportProtocol>& supported_transports,
CtapGetAssertionRequest request_parameter,
SignResponseCallback completion_callback,
AddPlatformAuthenticatorCallback add_platform_authenticator);
~GetAssertionRequestHandler() override; ~GetAssertionRequestHandler() override;
private: private:
......
device/fido/make_credential_handler_unittest.cc
View file @ 7cbada43
...@@ -77,12 +77,12 @@ class FidoMakeCredentialHandlerTest : public ::testing::Test { ...@@ -77,12 +77,12 @@ class FidoMakeCredentialHandlerTest : public ::testing::Test {
test_data::kClientDataHash, std::move(rp), std::move(user), test_data::kClientDataHash, std::move(rp), std::move(user),
std::move(credential_params)); std::move(credential_params));
return std::make_unique<MakeCredentialRequestHandler>( auto handler = std::make_unique<MakeCredentialRequestHandler>(
nullptr, supported_transports_, std::move(request_parameter), nullptr, supported_transports_, std::move(request_parameter),
std::move(authenticator_selection_criteria), cb_.callback(), std::move(authenticator_selection_criteria), cb_.callback());
base::BindOnce( handler->SetPlatformAuthenticatorOrMarkUnavailable(
&FidoMakeCredentialHandlerTest::CreatePlatformAuthenticator, CreatePlatformAuthenticator());
base::Unretained(this))); return handler;
} }
void ExpectAllowedTransportsForRequestAre( void ExpectAllowedTransportsForRequestAre(
...@@ -95,8 +95,6 @@ class FidoMakeCredentialHandlerTest : public ::testing::Test { ...@@ -95,8 +95,6 @@ class FidoMakeCredentialHandlerTest : public ::testing::Test {
ble_discovery()->WaitForCallToStartAndSimulateSuccess(); ble_discovery()->WaitForCallToStartAndSimulateSuccess();
if (base::ContainsKey(transports, Transport::kNearFieldCommunication)) if (base::ContainsKey(transports, Transport::kNearFieldCommunication))
nfc_discovery()->WaitForCallToStartAndSimulateSuccess(); nfc_discovery()->WaitForCallToStartAndSimulateSuccess();
if (base::ContainsKey(transports, Transport::kInternal))
platform_authenticator_factory().WaitForCallback();
scoped_task_environment_.FastForwardUntilNoTasksRemain(); scoped_task_environment_.FastForwardUntilNoTasksRemain();
EXPECT_FALSE(callback().was_called()); EXPECT_FALSE(callback().was_called());
...@@ -107,8 +105,6 @@ class FidoMakeCredentialHandlerTest : public ::testing::Test { ...@@ -107,8 +105,6 @@ class FidoMakeCredentialHandlerTest : public ::testing::Test {
EXPECT_FALSE(ble_discovery()->is_start_requested()); EXPECT_FALSE(ble_discovery()->is_start_requested());
if (!base::ContainsKey(transports, Transport::kNearFieldCommunication)) if (!base::ContainsKey(transports, Transport::kNearFieldCommunication))
EXPECT_FALSE(nfc_discovery()->is_start_requested()); EXPECT_FALSE(nfc_discovery()->is_start_requested());
if (!base::ContainsKey(transports, Transport::kInternal))
EXPECT_FALSE(platform_authenticator_factory().was_called());
// Even with FidoTransportProtocol::kInternal allowed, unless the platform // Even with FidoTransportProtocol::kInternal allowed, unless the platform
// authenticator factory returns a FidoAuthenticator instance (which it will // authenticator factory returns a FidoAuthenticator instance (which it will
...@@ -139,10 +135,6 @@ class FidoMakeCredentialHandlerTest : public ::testing::Test { ...@@ -139,10 +135,6 @@ class FidoMakeCredentialHandlerTest : public ::testing::Test {
mock_platform_device_ = std::move(device); mock_platform_device_ = std::move(device);
} }
test::TestCallbackReceiver<>& platform_authenticator_factory() {
return create_platform_authenticator_receiver_;
}
void set_supported_transports( void set_supported_transports(
base::flat_set<FidoTransportProtocol> transports) { base::flat_set<FidoTransportProtocol> transports) {
supported_transports_ = std::move(transports); supported_transports_ = std::move(transports);
...@@ -150,7 +142,6 @@ class FidoMakeCredentialHandlerTest : public ::testing::Test { ...@@ -150,7 +142,6 @@ class FidoMakeCredentialHandlerTest : public ::testing::Test {
protected: protected:
std::unique_ptr<FidoAuthenticator> CreatePlatformAuthenticator() { std::unique_ptr<FidoAuthenticator> CreatePlatformAuthenticator() {
create_platform_authenticator_receiver_.callback().Run();
if (!mock_platform_device_) if (!mock_platform_device_)
return nullptr; return nullptr;
return std::make_unique<FidoDeviceAuthenticator>( return std::make_unique<FidoDeviceAuthenticator>(
...@@ -165,7 +156,6 @@ class FidoMakeCredentialHandlerTest : public ::testing::Test { ...@@ -165,7 +156,6 @@ class FidoMakeCredentialHandlerTest : public ::testing::Test {
test::FakeFidoDiscovery* ble_discovery_; test::FakeFidoDiscovery* ble_discovery_;
test::FakeFidoDiscovery* nfc_discovery_; test::FakeFidoDiscovery* nfc_discovery_;
std::unique_ptr<MockFidoDevice> mock_platform_device_; std::unique_ptr<MockFidoDevice> mock_platform_device_;
test::TestCallbackReceiver<> create_platform_authenticator_receiver_;
TestMakeCredentialRequestCallback cb_; TestMakeCredentialRequestCallback cb_;
base::flat_set<FidoTransportProtocol> supported_transports_ = base::flat_set<FidoTransportProtocol> supported_transports_ =
GetTestableTransportProtocols(); GetTestableTransportProtocols();
...@@ -363,6 +353,7 @@ TEST_F(FidoMakeCredentialHandlerTest, ...@@ -363,6 +353,7 @@ TEST_F(FidoMakeCredentialHandlerTest,
set_supported_transports({FidoTransportProtocol::kInternal}); set_supported_transports({FidoTransportProtocol::kInternal});
auto platform_device = MockFidoDevice::MakeCtap( auto platform_device = MockFidoDevice::MakeCtap(
ReadCTAPGetInfoResponse(test_data::kTestGetInfoResponsePlatformDevice)); ReadCTAPGetInfoResponse(test_data::kTestGetInfoResponsePlatformDevice));
platform_device->SetDeviceTransport(FidoTransportProtocol::kInternal);
EXPECT_CALL(*platform_device, GetId()) EXPECT_CALL(*platform_device, GetId())
.WillRepeatedly(testing::Return("device0")); .WillRepeatedly(testing::Return("device0"));
platform_device->ExpectCtap2CommandAndRespondWith( platform_device->ExpectCtap2CommandAndRespondWith(
...@@ -413,6 +404,7 @@ TEST_F(FidoMakeCredentialHandlerTest, ...@@ -413,6 +404,7 @@ TEST_F(FidoMakeCredentialHandlerTest,
PlatformAuthenticatorPretendingToBeCrossPlatform) { PlatformAuthenticatorPretendingToBeCrossPlatform) {
auto platform_device = MockFidoDevice::MakeCtap( auto platform_device = MockFidoDevice::MakeCtap(
ReadCTAPGetInfoResponse(test_data::kTestAuthenticatorGetInfoResponse)); ReadCTAPGetInfoResponse(test_data::kTestAuthenticatorGetInfoResponse));
platform_device->SetDeviceTransport(FidoTransportProtocol::kInternal);
EXPECT_CALL(*platform_device, GetId()) EXPECT_CALL(*platform_device, GetId())
.WillRepeatedly(testing::Return("device0")); .WillRepeatedly(testing::Return("device0"));
set_mock_platform_device(std::move(platform_device)); set_mock_platform_device(std::move(platform_device));
...@@ -425,7 +417,6 @@ TEST_F(FidoMakeCredentialHandlerTest, ...@@ -425,7 +417,6 @@ TEST_F(FidoMakeCredentialHandlerTest,
true /* require_resident_key */, true /* require_resident_key */,
UserVerificationRequirement::kRequired)); UserVerificationRequirement::kRequired));
platform_authenticator_factory().WaitForCallback();
scoped_task_environment_.FastForwardUntilNoTasksRemain(); scoped_task_environment_.FastForwardUntilNoTasksRemain();
EXPECT_FALSE(callback().was_called()); EXPECT_FALSE(callback().was_called());
} }
......
device/fido/make_credential_request_handler.cc
View file @ 7cbada43
...@@ -98,27 +98,12 @@ MakeCredentialRequestHandler::MakeCredentialRequestHandler( ...@@ -98,27 +98,12 @@ MakeCredentialRequestHandler::MakeCredentialRequestHandler(
CtapMakeCredentialRequest request, CtapMakeCredentialRequest request,
AuthenticatorSelectionCriteria authenticator_selection_criteria, AuthenticatorSelectionCriteria authenticator_selection_criteria,
RegisterResponseCallback completion_callback) RegisterResponseCallback completion_callback)
: MakeCredentialRequestHandler(connector,
supported_transports,
std::move(request),
authenticator_selection_criteria,
std::move(completion_callback),
AddPlatformAuthenticatorCallback()) {}
MakeCredentialRequestHandler::MakeCredentialRequestHandler(
service_manager::Connector* connector,
const base::flat_set<FidoTransportProtocol>& supported_transports,
CtapMakeCredentialRequest request,
AuthenticatorSelectionCriteria authenticator_selection_criteria,
RegisterResponseCallback completion_callback,
AddPlatformAuthenticatorCallback add_platform_authenticator)
: FidoRequestHandler( : FidoRequestHandler(
connector, connector,
base::STLSetIntersection<base::flat_set<FidoTransportProtocol>>( base::STLSetIntersection<base::flat_set<FidoTransportProtocol>>(
supported_transports, supported_transports,
GetTransportsAllowedByRP(authenticator_selection_criteria)), GetTransportsAllowedByRP(authenticator_selection_criteria)),
std::move(completion_callback), std::move(completion_callback)),
std::move(add_platform_authenticator)),
request_parameter_(std::move(request)), request_parameter_(std::move(request)),
authenticator_selection_criteria_( authenticator_selection_criteria_(
std::move(authenticator_selection_criteria)), std::move(authenticator_selection_criteria)),
......
device/fido/make_credential_request_handler.h
View file @ 7cbada43
...@@ -41,13 +41,6 @@ class COMPONENT_EXPORT(DEVICE_FIDO) MakeCredentialRequestHandler ...@@ -41,13 +41,6 @@ class COMPONENT_EXPORT(DEVICE_FIDO) MakeCredentialRequestHandler
CtapMakeCredentialRequest request_parameter, CtapMakeCredentialRequest request_parameter,
AuthenticatorSelectionCriteria authenticator_criteria, AuthenticatorSelectionCriteria authenticator_criteria,
RegisterResponseCallback completion_callback); RegisterResponseCallback completion_callback);
MakeCredentialRequestHandler(
service_manager::Connector* connector,
const base::flat_set<FidoTransportProtocol>& supported_transports,
CtapMakeCredentialRequest request_parameter,
AuthenticatorSelectionCriteria authenticator_criteria,
RegisterResponseCallback completion_callback,
AddPlatformAuthenticatorCallback add_platform_authenticator);
~MakeCredentialRequestHandler() override; ~MakeCredentialRequestHandler() override;
private: private:
......
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