device/fido/mac: fix a threading bug in TouchIdContext
TouchIdContext implements the Touch ID authorization prompt. After
receiving the OS reply callback from the Security framework it forwards
the result to the user-supplied callback, but doesn't do so on the
original TaskRunner that it was invoked on. As a result, Touch ID
authenticator requests fail some sequence checker tests. This fixes that
by posting the user-supplied callback back on the original runner.
Bug: 678128
Change-Id: I0e7a8730ec898f288cd25323134aa95ef50ddbdf
Reviewed-on: https://chromium-review.googlesource.com/1136496
Commit-Queue: Martin Kreichgauer <martinkr@google.com>
Reviewed-by: Robert Sesek <rsesek@chromium.org>
Reviewed-by: Kim Paulhamus <kpaulhamus@chromium.org>
Cr-Commit-Position: refs/heads/master@{#575361}diff --git a/device/fido/mac/get_assertion_operation.h b/device/fido/mac/get_assertion_operation.h
index 3b4efd5..3146c8f 100644
--- a/device/fido/mac/get_assertion_operation.h
+++ b/device/fido/mac/get_assertion_operation.h
@@ -40,7 +40,7 @@
private:
const std::string& RpId() const override;
- void PromptTouchIdDone(bool success, NSError* err) override;
+ void PromptTouchIdDone(bool success) override;
DISALLOW_COPY_AND_ASSIGN(GetAssertionOperation);
};
diff --git a/device/fido/mac/get_assertion_operation.mm b/device/fido/mac/get_assertion_operation.mm
index e778cd1..4f1bec1 100644
--- a/device/fido/mac/get_assertion_operation.mm
+++ b/device/fido/mac/get_assertion_operation.mm
@@ -52,11 +52,8 @@
PromptTouchId("sign in to " + RpId());
}
-void GetAssertionOperation::PromptTouchIdDone(bool success, NSError* err) {
+void GetAssertionOperation::PromptTouchIdDone(bool success) {
if (!success) {
- // err is autoreleased.
- CHECK(err != nil);
- DVLOG(1) << "Touch ID prompt failed: " << base::mac::NSToCFCast(err);
std::move(callback())
.Run(CtapDeviceResponseCode::kCtap2ErrOperationDenied, base::nullopt);
return;
diff --git a/device/fido/mac/make_credential_operation.h b/device/fido/mac/make_credential_operation.h
index f30ce14..4f9f78c 100644
--- a/device/fido/mac/make_credential_operation.h
+++ b/device/fido/mac/make_credential_operation.h
@@ -58,7 +58,7 @@
private:
// OperationBase:
const std::string& RpId() const override;
- void PromptTouchIdDone(bool success, NSError* err) override;
+ void PromptTouchIdDone(bool success) override;
base::Optional<std::vector<uint8_t>> GenerateCredentialIdForRequest() const;
};
diff --git a/device/fido/mac/make_credential_operation.mm b/device/fido/mac/make_credential_operation.mm
index 0081e96..51f598dd 100644
--- a/device/fido/mac/make_credential_operation.mm
+++ b/device/fido/mac/make_credential_operation.mm
@@ -70,11 +70,8 @@
PromptTouchId("register with " + RpId());
}
-void MakeCredentialOperation::PromptTouchIdDone(bool success, NSError* err) {
+void MakeCredentialOperation::PromptTouchIdDone(bool success) {
if (!success) {
- // err is autoreleased.
- CHECK(err != nil);
- DVLOG(1) << "Touch ID prompt failed: " << base::mac::NSToCFCast(err);
std::move(callback())
.Run(CtapDeviceResponseCode::kCtap2ErrOperationDenied, base::nullopt);
return;
diff --git a/device/fido/mac/operation_base.h b/device/fido/mac/operation_base.h
index 6f030be..5452520 100644
--- a/device/fido/mac/operation_base.h
+++ b/device/fido/mac/operation_base.h
@@ -65,8 +65,8 @@
base::Unretained(this)));
}
- // Callback for |PromptTouchId|. Any NSError that gets passed is autoreleased.
- virtual void PromptTouchIdDone(bool success, NSError* err) = 0;
+ // Callback for |PromptTouchId|.
+ virtual void PromptTouchIdDone(bool success) = 0;
// Subclasses override RpId to return the RP ID from the type-specific
// request.
diff --git a/device/fido/mac/touch_id_context.h b/device/fido/mac/touch_id_context.h
index 29133b4..e54f64d 100644
--- a/device/fido/mac/touch_id_context.h
+++ b/device/fido/mac/touch_id_context.h
@@ -23,9 +23,8 @@
class API_AVAILABLE(macosx(10.12.2)) TouchIdContext {
public:
// The callback is invoked when the Touch ID prompt completes. It receives a
- // boolean indicating success and an autoreleased NSError if the prompt was
- // denied or failed.
- using Callback = base::OnceCallback<void(bool, NSError*)>;
+ // boolean indicating whether obtaining the fingerprint was successful.
+ using Callback = base::OnceCallback<void(bool)>;
TouchIdContext();
~TouchIdContext();
@@ -44,6 +43,8 @@
SecAccessControlRef access_control() const { return access_control_; }
private:
+ void RunCallback(bool success);
+
base::scoped_nsobject<LAContext> context_;
base::ScopedCFTypeRef<SecAccessControlRef> access_control_;
Callback callback_;
diff --git a/device/fido/mac/touch_id_context.mm b/device/fido/mac/touch_id_context.mm
index 8a31f2bc..f372a44f 100644
--- a/device/fido/mac/touch_id_context.mm
+++ b/device/fido/mac/touch_id_context.mm
@@ -6,7 +6,12 @@
#import <Foundation/Foundation.h>
+#include "base/bind.h"
+#include "base/logging.h"
+#include "base/mac/foundation_util.h"
+#include "base/sequenced_task_runner.h"
#include "base/strings/sys_string_conversions.h"
+#include "base/threading/sequenced_task_runner_handle.h"
namespace device {
namespace fido {
@@ -33,6 +38,8 @@
void TouchIdContext::PromptTouchId(std::string reason, Callback callback) {
callback_ = std::move(callback);
+ scoped_refptr<base::SequencedTaskRunner> runner =
+ base::SequencedTaskRunnerHandle::Get();
auto weak_self = weak_ptr_factory_.GetWeakPtr();
// If evaluation succeeds (i.e. user provides a fingerprint), |context_| can
// be used for one signing operation. N.B. even in |MakeCredentialOperation|,
@@ -42,13 +49,35 @@
operation:LAAccessControlOperationUseKeySign
localizedReason:base::SysUTF8ToNSString(reason)
reply:^(BOOL success, NSError* error) {
- if (!weak_self) {
- return;
+ // The reply block is invoked in a separate
+ // thread. We want to invoke the callback in the
+ // original thread, so we post it onto the
+ // originating runner. The weak_self and runner
+ // variables inside the block are const-copies of
+ // the ones in the enclosing scope (c.f.
+ // http://clang.llvm.org/docs/Block-ABI-Apple.html#imported-variables).
+ if (!success) {
+ // |error| is autoreleased in this block. It
+ // is not currently passed onto the other
+ // thread running the callback; but if it
+ // were, it would have to be retained first
+ // (and probably captured in a
+ // scoped_nsobject<NSError>).
+ DCHECK(error != nil);
+ DVLOG(1) << "Touch ID prompt failed: "
+ << base::mac::NSToCFCast(error);
}
- std::move(callback_).Run(success, error);
+ runner->PostTask(
+ FROM_HERE,
+ base::BindOnce(&TouchIdContext::RunCallback,
+ weak_self, success));
}];
}
+void TouchIdContext::RunCallback(bool success) {
+ std::move(callback_).Run(success);
+}
+
} // namespace mac
} // namespace fido
} // namespace device
