fido: make AuthenticatorTestBase set the MOCK_TIME TaskEnvironment trait

This allows us to get rid of base::Timer injection into AuthenticatorCommon, which tests used to wait for requests to time out. Also get remove a number of superfluous RunUntilIdle() calls, and of calls to OverrideLastCommittedOrigin() where SimulateNavigation() would suffice. Change-Id: I8cb4980d51efe43f59d828cde5f34ce67d38d928 Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2299630 Auto-Submit: Martin Kreichgauer <martinkr@google.com> Reviewed-by: Jared Saul <jsaul@google.com> Reviewed-by: Adam Langley <agl@chromium.org> Commit-Queue: Martin Kreichgauer <martinkr@google.com> Cr-Commit-Position: refs/heads/master@{#788885}
chromium · Jul 16, 2020 · f55cd37 · f55cd37
1 parent a025272
commit f55cd37
Show file tree

Hide file tree

Showing 7 changed files with 107 additions and 287 deletions.
diff --git a/components/autofill/content/browser/webauthn/internal_authenticator_impl.cc b/components/autofill/content/browser/webauthn/internal_authenticator_impl.cc
@@ -7,7 +7,6 @@
 #include <string>
 #include <utility>
 
-#include "base/timer/timer.h"
 #include "content/browser/webauth/authenticator_common.h"
 #include "content/public/browser/navigation_handle.h"
 #include "content/public/browser/render_frame_host.h"
@@ -18,20 +17,12 @@ namespace content {
 
 InternalAuthenticatorImpl::InternalAuthenticatorImpl(
     RenderFrameHost* render_frame_host)
-    : InternalAuthenticatorImpl(render_frame_host,
-                                std::make_unique<AuthenticatorCommon>(
-                                    render_frame_host,
-                                    std::make_unique<base::OneShotTimer>())) {}
-
-InternalAuthenticatorImpl::InternalAuthenticatorImpl(
-    RenderFrameHost* render_frame_host,
-    std::unique_ptr<AuthenticatorCommon> authenticator_common)
     : WebContentsObserver(WebContents::FromRenderFrameHost(render_frame_host)),
       render_frame_host_(render_frame_host),
       effective_origin_(render_frame_host->GetLastCommittedOrigin()),
-      authenticator_common_(std::move(authenticator_common)) {
+      authenticator_common_(
+          std::make_unique<AuthenticatorCommon>(render_frame_host)) {
   DCHECK(render_frame_host_);
-  DCHECK(authenticator_common_);
   // Disabling WebAuthn modal dialogs to avoid conflict with Autofill's own
   // modal dialogs. Since WebAuthn is designed for websites, rather than browser
   // components, the UI can be confusing for users in the case for Autofill.

diff --git a/components/autofill/content/browser/webauthn/internal_authenticator_impl.h b/components/autofill/content/browser/webauthn/internal_authenticator_impl.h
@@ -57,14 +57,6 @@ class InternalAuthenticatorImpl : public autofill::InternalAuthenticator,
  private:
   friend class InternalAuthenticatorImplTest;
 
-  // By being able to set AuthenticatorCommon, this constructor permits setting
-  // the connector and timer for testing. Using this constructor will also empty
-  // out the protocol set, since no device discovery will take place during
-  // tests.
-  InternalAuthenticatorImpl(
-      RenderFrameHost* render_frame_host,
-      std::unique_ptr<AuthenticatorCommon> authenticator_common);
-
   AuthenticatorCommon* get_authenticator_common_for_testing() {
     return authenticator_common_.get();
   }

diff --git a/content/browser/webauth/authenticator_common.cc b/content/browser/webauth/authenticator_common.cc
@@ -510,15 +510,11 @@ base::flat_set<device::FidoTransportProtocol> GetAvailableTransports(
 
 }  // namespace
 
-AuthenticatorCommon::AuthenticatorCommon(
-    RenderFrameHost* render_frame_host,
-    std::unique_ptr<base::OneShotTimer> timer)
+AuthenticatorCommon::AuthenticatorCommon(RenderFrameHost* render_frame_host)
     : render_frame_host_(render_frame_host),
       security_checker_(static_cast<RenderFrameHostImpl*>(render_frame_host)
-                            ->GetWebAuthRequestSecurityChecker()),
-      timer_(std::move(timer)) {
+                            ->GetWebAuthRequestSecurityChecker()) {
   DCHECK(render_frame_host_);
-  DCHECK(timer_);
   // Disable the back-forward cache for any document that makes WebAuthn
   // requests. Pages using privacy-sensitive APIs are generally exempt from
   // back-forward cache for now as a precaution.

diff --git a/content/browser/webauth/authenticator_common.h b/content/browser/webauth/authenticator_common.h
@@ -16,6 +16,7 @@
 #include "base/containers/span.h"
 #include "base/macros.h"
 #include "base/optional.h"
+#include "base/timer/timer.h"
 #include "content/common/content_export.h"
 #include "content/public/browser/authenticator_request_client_delegate.h"
 #include "content/public/browser/web_contents_observer.h"
@@ -67,9 +68,7 @@ CONTENT_EXPORT extern const char kGetType[];
 // Common code for any WebAuthn Authenticator interfaces.
 class CONTENT_EXPORT AuthenticatorCommon {
  public:
-  // Permits setting timer for testing.
-  AuthenticatorCommon(RenderFrameHost* render_frame_host,
-                      std::unique_ptr<base::OneShotTimer>);
+  explicit AuthenticatorCommon(RenderFrameHost* render_frame_host);
   virtual ~AuthenticatorCommon();
 
   // This is not-quite an implementation of blink::mojom::Authenticator. The
@@ -183,7 +182,8 @@ class CONTENT_EXPORT AuthenticatorCommon {
   url::Origin caller_origin_;
   std::string relying_party_id_;
   scoped_refptr<WebAuthRequestSecurityChecker> security_checker_;
-  std::unique_ptr<base::OneShotTimer> timer_;
+  std::unique_ptr<base::OneShotTimer> timer_ =
+      std::make_unique<base::OneShotTimer>();
   base::Optional<device::AuthenticatorSelectionCriteria>
       authenticator_selection_criteria_;
   base::Optional<std::string> app_id_;

diff --git a/content/browser/webauth/authenticator_impl.cc b/content/browser/webauth/authenticator_impl.cc
@@ -17,10 +17,9 @@
 namespace content {
 
 AuthenticatorImpl::AuthenticatorImpl(RenderFrameHost* render_frame_host)
-    : AuthenticatorImpl(render_frame_host,
-                        std::make_unique<AuthenticatorCommon>(
-                            render_frame_host,
-                            std::make_unique<base::OneShotTimer>())) {}
+    : AuthenticatorImpl(
+          render_frame_host,
+          std::make_unique<AuthenticatorCommon>(render_frame_host)) {}
 
 AuthenticatorImpl::AuthenticatorImpl(
     RenderFrameHost* render_frame_host,

diff --git a/content/browser/webauth/authenticator_impl.h b/content/browser/webauth/authenticator_impl.h
@@ -46,9 +46,8 @@ class CONTENT_EXPORT AuthenticatorImpl : public blink::mojom::Authenticator,
  public:
   explicit AuthenticatorImpl(RenderFrameHost* render_frame_host);
 
-  // By being able to set AuthenticatorCommon, this constructor permits setting
-  // the timer for testing. Using this constructor will also empty out the
-  // protocol set, since no device discovery will take place during tests.
+  // Constructs an AuthenticatorImpl with an injected AuthenticatorCommon for
+  // testing.
   AuthenticatorImpl(RenderFrameHost* render_frame_host,
                     std::unique_ptr<AuthenticatorCommon> authenticator_common);
   ~AuthenticatorImpl() override;