Adding asyn auth api, and moving Windows auth call to thread pool.

When user tries to export password or view saved password by visiting
chrome://settings/passwords url and clicking on "Export passwords..."
or eye icon on password list respectively, OS Reauthentication is
triggered, and on successful authentication plain text password is
revealed. Currently OS authentication call is on UI Thread.
And if OS authentication takes time, it freezes/hangs
browser. This change has made authentication api async, and moved
windows authentication call to thread pool from UI thread. Since the
UI thread will be freed, browser will not freeze or hang.

Bug: 1007950
Change-Id: I2f35707fe458687a4ec22de1381d21ec9603733f
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1828730
Commit-Queue: Ankit Jain <ankjain@microsoft.com>
Reviewed-by: Vasilii Sukhanov <vasilii@chromium.org>
Cr-Commit-Position: refs/heads/master@{#707029}

chrome/browser/extensions/api/passwords_private/passwords_private_delegate_impl.cc

@@ -194,21 +194,12 @@ void PasswordsPrivateDelegateImpl::RequestShowPassword(
   // TODO(crbug.com/495290): Pass the native window directly to the
   // reauth-handling code.
   web_contents_ = web_contents;
-  if (!password_access_authenticator_.EnsureUserIsAuthenticated(
-          password_manager::ReauthPurpose::VIEW_PASSWORD)) {
-    std::move(callback).Run(base::nullopt);
-    return;
-  }
-
-  // Request the password. When it is retrieved, ShowPassword() will be called.
-  const std::string* sort_key = password_id_generator_.TryGetSortKey(id);
-  if (!sort_key) {
-    std::move(callback).Run(base::nullopt);
-    return;
-  }
-
-  password_manager_presenter_->RequestShowPassword(*sort_key,
-                                                   std::move(callback));
+  base::OnceCallback<void(bool)> request_show_password_reply =
+      base::BindOnce(&PasswordsPrivateDelegateImpl::RequestShowPasswordReply,
+                     weak_ptr_factory_.GetWeakPtr(), std::move(callback), id);
+  password_access_authenticator_.EnsureUserIsAuthenticatedAsync(
+      password_manager::ReauthPurpose::VIEW_PASSWORD,
+      std::move(request_show_password_reply));
 }
 
 bool PasswordsPrivateDelegateImpl::OsReauthCall(
@@ -327,15 +318,12 @@ void PasswordsPrivateDelegateImpl::ExportPasswords(
   // TODO(crbug.com/495290): Pass the native window directly to the
   // reauth-handling code.
   web_contents_ = web_contents;
-  if (!password_access_authenticator_.ForceUserReauthentication(
-          password_manager::ReauthPurpose::EXPORT)) {
-    std::move(callback).Run(kReauthenticationFailed);
-    return;
-  }
-
-  password_manager_porter_->set_web_contents(web_contents);
-  bool accepted = password_manager_porter_->Store();
-  std::move(callback).Run(accepted ? std::string() : kExportInProgress);
+  base::OnceCallback<void(bool)> export_password_reply = base::BindOnce(
+      &PasswordsPrivateDelegateImpl::ExportPasswordReply,
+      weak_ptr_factory_.GetWeakPtr(), std::move(callback), web_contents);
+  password_access_authenticator_.ForceUserReauthenticationAsync(
+      password_manager::ReauthPurpose::EXPORT,
+      std::move(export_password_reply));
 }
 
 void PasswordsPrivateDelegateImpl::CancelExportPasswords() {
@@ -383,6 +371,40 @@ void PasswordsPrivateDelegateImpl::ExecuteFunction(
   pre_initialization_callbacks_.push_back(callback);
 }
 
+void PasswordsPrivateDelegateImpl::ExportPasswordReply(
+    base::OnceCallback<void(const std::string&)> callback,
+    content::WebContents* web_contents,
+    bool authenticated) {
+  if (!authenticated) {
+    std::move(callback).Run(kReauthenticationFailed);
+    return;
+  }
+
+  password_manager_porter_->set_web_contents(web_contents);
+  bool accepted = password_manager_porter_->Store();
+  std::move(callback).Run(accepted ? std::string() : kExportInProgress);
+}
+
+void PasswordsPrivateDelegateImpl::RequestShowPasswordReply(
+    PlaintextPasswordCallback callback,
+    int id,
+    bool authenticated) {
+  if (!authenticated) {
+    std::move(callback).Run(base::nullopt);
+    return;
+  }
+
+  // Request the password. When it is retrieved, ShowPassword() will be called.
+  const std::string* sort_key = password_id_generator_.TryGetSortKey(id);
+  if (!sort_key) {
+    std::move(callback).Run(base::nullopt);
+    return;
+  }
+
+  password_manager_presenter_->RequestShowPassword(*sort_key,
+                                                   std::move(callback));
+}
+
 void PasswordsPrivateDelegateImpl::InitializeIfNecessary() {
   if (is_initialized_ || !current_entries_initialized_ ||
       !current_exceptions_initialized_)

chrome/browser/extensions/api/passwords_private/passwords_private_delegate_impl.h

@@ -97,6 +97,17 @@ class PasswordsPrivateDelegateImpl : public PasswordsPrivateDelegate,
   // has been initialized or by deferring it until initialization has completed.
   void ExecuteFunction(const base::Closure& callback);
 
+  // Called after OS authentication call for export password request.
+  void ExportPasswordReply(
+      base::OnceCallback<void(const std::string&)> accepted,
+      content::WebContents* web_contents,
+      bool authenticated);
+
+  // Called after OS authentication call for show password request.
+  void RequestShowPasswordReply(PlaintextPasswordCallback callback,
+                                int id,
+                                bool authenticated);
+
   void SendSavedPasswordsList();
   void SendPasswordExceptionsList();
 
@@ -148,6 +159,9 @@ class PasswordsPrivateDelegateImpl : public PasswordsPrivateDelegate,
   // NativeWindow for the window where the API was called.
   content::WebContents* web_contents_;
 
+  // Weak pointers for different callbacks.
+  base::WeakPtrFactory<PasswordsPrivateDelegateImpl> weak_ptr_factory_{this};
+
   DISALLOW_COPY_AND_ASSIGN(PasswordsPrivateDelegateImpl);
 };
 

chrome/browser/extensions/api/passwords_private/passwords_private_delegate_impl_unittest.cc

@@ -281,6 +281,7 @@ TEST_F(PasswordsPrivateDelegateImplTest, TestPassedReauthOnView) {
   base::Optional<base::string16> plaintext_password;
   delegate.RequestShowPassword(0, GetCallbackArgument(&plaintext_password),
                                nullptr);
+  task_environment_.RunUntilIdle();
   EXPECT_TRUE(reauth_called);
   EXPECT_TRUE(plaintext_password.has_value());
   EXPECT_EQ(base::ASCIIToUTF16("test"), *plaintext_password);
@@ -301,6 +302,7 @@ TEST_F(PasswordsPrivateDelegateImplTest, TestFailedReauthOnView) {
   base::Optional<base::string16> plaintext_password;
   delegate.RequestShowPassword(0, GetCallbackArgument(&plaintext_password),
                                nullptr);
+  task_environment_.RunUntilIdle();
   EXPECT_TRUE(reauth_called);
   EXPECT_FALSE(plaintext_password.has_value());
 }
@@ -322,11 +324,13 @@ TEST_F(PasswordsPrivateDelegateImplTest, TestReauthOnExport) {
   EXPECT_CALL(mock_accepted, Run(std::string())).Times(2);
 
   delegate.ExportPasswords(mock_accepted.Get(), nullptr);
+  task_environment_.RunUntilIdle();
   EXPECT_TRUE(reauth_called);
 
   // Export should ignore previous reauthentication results.
   reauth_called = false;
   delegate.ExportPasswords(mock_accepted.Get(), nullptr);
+  task_environment_.RunUntilIdle();
   EXPECT_TRUE(reauth_called);
 }
 
@@ -347,6 +351,7 @@ TEST_F(PasswordsPrivateDelegateImplTest, TestReauthFailedOnExport) {
       &FakeOsReauthCall, &reauth_called, ReauthResult::FAIL));
 
   delegate.ExportPasswords(mock_accepted.Get(), nullptr);
+  task_environment_.RunUntilIdle();
   EXPECT_TRUE(reauth_called);
 }
 

chrome/browser/ui/passwords/settings/password_access_authenticator.cc

@@ -6,8 +6,12 @@
 
 #include <utility>
 
+#include "base/callback.h"
 #include "base/metrics/histogram_macros.h"
+#include "base/task/post_task.h"
+#include "base/task/task_traits.h"
 #include "base/time/default_clock.h"
+#include "build/build_config.h"
 #include "components/password_manager/core/browser/password_manager_metrics_util.h"
 
 PasswordAccessAuthenticator::PasswordAccessAuthenticator(
@@ -19,34 +23,44 @@ PasswordAccessAuthenticator::~PasswordAccessAuthenticator() = default;
 
 // TODO(crbug.com/327331): Trigger Re-Auth after closing and opening the
 // settings tab.
-bool PasswordAccessAuthenticator::EnsureUserIsAuthenticated(
-    password_manager::ReauthPurpose purpose) {
-  const bool can_skip_reauth =
-      last_authentication_time_.has_value() &&
-      clock_->Now() - *last_authentication_time_ <=
-          base::TimeDelta::FromSeconds(kAuthValidityPeriodSeconds);
+void PasswordAccessAuthenticator::EnsureUserIsAuthenticatedAsync(
+    password_manager::ReauthPurpose purpose,
+    base::OnceCallback<void(bool)> postAuthCallback) {
+  const bool can_skip_reauth = CanSkipReauth();
   if (can_skip_reauth) {
     UMA_HISTOGRAM_ENUMERATION(
         "PasswordManager.ReauthToAccessPasswordInSettings",
         password_manager::metrics_util::REAUTH_SKIPPED,
         password_manager::metrics_util::REAUTH_COUNT);
-    return true;
+    std::move(postAuthCallback).Run(can_skip_reauth);
+    return;
   }
 
-  return ForceUserReauthentication(purpose);
+  ForceUserReauthenticationAsync(purpose, std::move(postAuthCallback));
 }
 
-bool PasswordAccessAuthenticator::ForceUserReauthentication(
-    password_manager::ReauthPurpose purpose) {
+void PasswordAccessAuthenticator::ForceUserReauthenticationAsync(
+    password_manager::ReauthPurpose purpose,
+    base::OnceCallback<void(bool)> postAuthCallback) {
+#if defined(OS_WIN)
+  // In Windows it is possible to move OS authentication to separate thread.
+  ReauthCallback os_reauth_call(os_reauth_call_);
+  base::PostTaskAndReplyWithResult(
+      FROM_HERE,
+      {base::ThreadPool(), base::TaskPriority::USER_VISIBLE, base::MayBlock(),
+       base::TaskShutdownBehavior::SKIP_ON_SHUTDOWN},
+      base::BindOnce(std::move(os_reauth_call), purpose),
+      base::BindOnce(
+          &PasswordAccessAuthenticator::ProcessReauthenticationResult,
+          weak_ptr_factory_.GetWeakPtr(), purpose,
+          std::move(postAuthCallback)));
+#else
+  // For MAC OS, user input to a process is pulled in by OS prompt,
+  // if OS prompt is in open state.
   bool authenticated = os_reauth_call_.Run(purpose);
-  if (authenticated)
-    last_authentication_time_ = clock_->Now();
-  UMA_HISTOGRAM_ENUMERATION(
-      "PasswordManager.ReauthToAccessPasswordInSettings",
-      authenticated ? password_manager::metrics_util::REAUTH_SUCCESS
-                    : password_manager::metrics_util::REAUTH_FAILURE,
-      password_manager::metrics_util::REAUTH_COUNT);
-  return authenticated;
+  ProcessReauthenticationResult(purpose, std::move(postAuthCallback),
+                                authenticated);
+#endif  // OS_WIN
 }
 
 void PasswordAccessAuthenticator::SetOsReauthCallForTesting(
@@ -57,3 +71,24 @@ void PasswordAccessAuthenticator::SetOsReauthCallForTesting(
 void PasswordAccessAuthenticator::SetClockForTesting(base::Clock* clock) {
   clock_ = clock;
 }
+
+bool PasswordAccessAuthenticator::CanSkipReauth() const {
+  return last_authentication_time_.has_value() &&
+         clock_->Now() - *last_authentication_time_ <=
+             base::TimeDelta::FromSeconds(kAuthValidityPeriodSeconds);
+}
+
+void PasswordAccessAuthenticator::ProcessReauthenticationResult(
+    password_manager::ReauthPurpose purpose,
+    base::OnceCallback<void(bool)> postAuthCallback,
+    bool authenticated) {
+  if (authenticated)
+    last_authentication_time_ = clock_->Now();
+  UMA_HISTOGRAM_ENUMERATION(
+      "PasswordManager.ReauthToAccessPasswordInSettings",
+      authenticated ? password_manager::metrics_util::REAUTH_SUCCESS
+                    : password_manager::metrics_util::REAUTH_FAILURE,
+      password_manager::metrics_util::REAUTH_COUNT);
+
+  std::move(postAuthCallback).Run(authenticated);
+}

chrome/browser/ui/passwords/settings/password_access_authenticator.h

@@ -9,6 +9,7 @@
 
 #include "base/callback.h"
 #include "base/macros.h"
+#include "base/memory/weak_ptr.h"
 #include "base/optional.h"
 #include "base/time/clock.h"
 #include "base/time/time.h"
@@ -35,16 +36,23 @@ class PasswordAccessAuthenticator {
 
   ~PasswordAccessAuthenticator();
 
-  // Returns whether the user is able to pass the authentication challenge,
-  // which is represented by |os_reauth_call_| returning true. A successful
-  // result of |os_reauth_call_| is cached for |kAuthValidityPeriodSeconds|
-  // seconds.
-  bool EnsureUserIsAuthenticated(password_manager::ReauthPurpose purpose);
-
-  // Presents the reauthentication challenge to the user and returns whether
-  // the user passed the challenge. This call is guaranteed to present the
-  // challenge to the user.
-  bool ForceUserReauthentication(password_manager::ReauthPurpose purpose);
+  // Checks whether user is already authenticated or not, if yes,
+  // execute |postAuthCallback|. Otherwise call
+  // |ForceUserReauthenticationAsync| function for presenting OS
+  // authentication challenge.
+  // This function and |postAuthCallback| callback should be executed
+  // on main thread only.
+  void EnsureUserIsAuthenticatedAsync(
+      password_manager::ReauthPurpose purpose,
+      base::OnceCallback<void(bool)> postAuthCallback);
+
+  // Presents the authentication challenge to the user on the
+  // background thread for Windows, and on UI thread for other platforms.
+  // This call is guaranteed to present the challenge to the user.
+  // This function should be executed on main thread only.
+  void ForceUserReauthenticationAsync(
+      password_manager::ReauthPurpose purpose,
+      base::OnceCallback<void(bool)> postAuthCallback);
 
   // Use this in tests to mock the OS-level reauthentication.
   void SetOsReauthCallForTesting(ReauthCallback os_reauth_call);
@@ -53,6 +61,19 @@ class PasswordAccessAuthenticator {
   void SetClockForTesting(base::Clock* clock);
 
  private:
+  // Returns whether the user can skip Reauth, based on
+  // |last_authentication_time_| and |kAuthValidityPeriodSeconds|
+  bool CanSkipReauth() const;
+
+  // This function will be called on main thread and it will run
+  // |postAuthCallback| on main thread.
+  // A successful result of |os_reauth_call_| is cached for
+  // |kAuthValidityPeriodSeconds| seconds.
+  void ProcessReauthenticationResult(
+      password_manager::ReauthPurpose purpose,
+      base::OnceCallback<void(bool)> postAuthCallback,
+      bool authenticated);
+
   // The last time the user was successfully authenticated.
   base::Optional<base::Time> last_authentication_time_;
 
@@ -63,6 +84,9 @@ class PasswordAccessAuthenticator {
   // prompt) to the user.
   ReauthCallback os_reauth_call_;
 
+  // Weak pointers for different callbacks.
+  base::WeakPtrFactory<PasswordAccessAuthenticator> weak_ptr_factory_{this};
+
   DISALLOW_COPY_AND_ASSIGN(PasswordAccessAuthenticator);
 };
 

chrome/browser/ui/passwords/settings/password_access_authenticator_unittest.cc

@@ -7,7 +7,10 @@
 #include <utility>
 
 #include "base/bind.h"
+#include "base/test/mock_callback.h"
 #include "base/test/simple_test_clock.h"
+#include "base/test/task_environment.h"
+#include "testing/gmock/include/gmock/gmock.h"
 #include "testing/gtest/include/gtest/gtest.h"
 
 using ::testing::TestWithParam;
@@ -15,6 +18,9 @@ using ::testing::Values;
 
 namespace {
 
+// Callback to be executed post os authentication.
+using PostAuthCallback = base::OnceCallback<void(bool)>;
+
 enum class ReauthResult { PASS, FAIL };
 bool FakeOsReauthCall(bool* reauth_called,
                       ReauthResult result,
@@ -32,7 +38,11 @@ class PasswordAccessAuthenticatorTest
   ~PasswordAccessAuthenticatorTest() = default;
 
  protected:
+  base::test::TaskEnvironment& task_env() { return task_env_; }
   password_manager::ReauthPurpose purpose_;
+
+ private:
+  base::test::TaskEnvironment task_env_;
 };
 
 // Check that a passed authentication does not expire before
@@ -48,20 +58,27 @@ TEST_P(PasswordAccessAuthenticatorTest, Expiration) {
       &FakeOsReauthCall, &reauth_called, ReauthResult::PASS));
   authenticator.SetClockForTesting(&clock);
 
-  EXPECT_TRUE(authenticator.EnsureUserIsAuthenticated(purpose_));
+  base::MockCallback<PostAuthCallback> callback;
+  EXPECT_CALL(callback, Run(true));
+  authenticator.EnsureUserIsAuthenticatedAsync(purpose_, callback.Get());
+  task_env().RunUntilIdle();
   EXPECT_TRUE(reauth_called);
 
   clock.Advance(base::TimeDelta::FromSeconds(
       PasswordAccessAuthenticator::kAuthValidityPeriodSeconds - 1));
   reauth_called = false;
 
-  EXPECT_TRUE(authenticator.EnsureUserIsAuthenticated(purpose_));
+  EXPECT_CALL(callback, Run(true));
+  authenticator.EnsureUserIsAuthenticatedAsync(purpose_, callback.Get());
+  task_env().RunUntilIdle();
   EXPECT_FALSE(reauth_called);
 
   clock.Advance(base::TimeDelta::FromSeconds(2));
   reauth_called = false;
 
-  EXPECT_TRUE(authenticator.EnsureUserIsAuthenticated(purpose_));
+  EXPECT_CALL(callback, Run(true));
+  authenticator.EnsureUserIsAuthenticatedAsync(purpose_, callback.Get());
+  task_env().RunUntilIdle();
   EXPECT_TRUE(reauth_called);
 }
 
@@ -76,14 +93,19 @@ TEST_P(PasswordAccessAuthenticatorTest, ForceReauth) {
       &FakeOsReauthCall, &reauth_called, ReauthResult::PASS));
   authenticator.SetClockForTesting(&clock);
 
-  EXPECT_TRUE(authenticator.EnsureUserIsAuthenticated(purpose_));
+  base::MockCallback<PostAuthCallback> callback;
+  EXPECT_CALL(callback, Run(true));
+  authenticator.EnsureUserIsAuthenticatedAsync(purpose_, callback.Get());
+  task_env().RunUntilIdle();
   EXPECT_TRUE(reauth_called);
 
   clock.Advance(base::TimeDelta::FromSeconds(
-      PasswordAccessAuthenticator::kAuthValidityPeriodSeconds - 1));
+      PasswordAccessAuthenticator::kAuthValidityPeriodSeconds + 1));
   reauth_called = false;
 
-  EXPECT_TRUE(authenticator.ForceUserReauthentication(purpose_));
+  EXPECT_CALL(callback, Run(true));
+  authenticator.EnsureUserIsAuthenticatedAsync(purpose_, callback.Get());
+  task_env().RunUntilIdle();
   EXPECT_TRUE(reauth_called);
 }
 
@@ -99,7 +121,10 @@ TEST_P(PasswordAccessAuthenticatorTest, Failed) {
       &FakeOsReauthCall, &reauth_called, ReauthResult::FAIL));
   authenticator.SetClockForTesting(&clock);
 
-  EXPECT_FALSE(authenticator.EnsureUserIsAuthenticated(purpose_));
+  base::MockCallback<PostAuthCallback> callback;
+  EXPECT_CALL(callback, Run(false));
+  authenticator.EnsureUserIsAuthenticatedAsync(purpose_, callback.Get());
+  task_env().RunUntilIdle();
   EXPECT_TRUE(reauth_called);
 
   // Advance just a little bit, so that if |authenticator| starts the grace
@@ -107,7 +132,9 @@ TEST_P(PasswordAccessAuthenticatorTest, Failed) {
   clock.Advance(base::TimeDelta::FromSeconds(1));
   reauth_called = false;
 
-  EXPECT_FALSE(authenticator.EnsureUserIsAuthenticated(purpose_));
+  EXPECT_CALL(callback, Run(false));
+  authenticator.EnsureUserIsAuthenticatedAsync(purpose_, callback.Get());
+  task_env().RunUntilIdle();
   EXPECT_TRUE(reauth_called);
 }
 
@@ -124,7 +151,10 @@ TEST_P(PasswordAccessAuthenticatorTest, TimeZero) {
       &FakeOsReauthCall, &reauth_called, ReauthResult::PASS));
   authenticator.SetClockForTesting(&clock);
 
-  EXPECT_TRUE(authenticator.EnsureUserIsAuthenticated(purpose_));
+  base::MockCallback<PostAuthCallback> callback;
+  EXPECT_CALL(callback, Run(true));
+  authenticator.EnsureUserIsAuthenticatedAsync(purpose_, callback.Get());
+  task_env().RunUntilIdle();
   EXPECT_TRUE(reauth_called);
 }