Commit 70fc0cf0 authored by Martin Kreichgauer's avatar Martin Kreichgauer Committed by Commit Bot

Reland "fido: make AuthenticatorTestBase set the MOCK_TIME TaskEnvironment trait"

This reverts commit 02303ee2.

Reason for revert: Original CL did not cause the test failure

Original change's description:
> Revert "fido: make AuthenticatorTestBase set the MOCK_TIME TaskEnvironment trait"
> 
> This reverts commit f55cd37e.
> 
> Reason for revert: The tests testAuthenticatorImplGetAssertionBridge_resultCanceled and testAuthenticatorImplGetAssertionBridgeWithUvmRequestedWithUvmResponded_success in org.chromium.chrome.browser.webauth.Fido2CredentialRequestTest consistently fail on android-pie-x86-rel since https://ci.chromium.org/p/chromium/builders/ci/android-pie-x86-rel/1498.
> This CL is the only one that touches authenticator_impl around that time. Revert it to see if it can fix the failure
> 
> Original change's description:
> > 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}
> 
> TBR=agl@chromium.org,jsaul@google.com,martinkr@google.com
> 
> Change-Id: I47c8796582f40f1946811b5eca4ae3cdfe5aac0f
> No-Presubmit: true
> No-Tree-Checks: true
> No-Try: true
> Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2303030
> Reviewed-by: Haiyang Pan <hypan@google.com>
> Commit-Queue: Haiyang Pan <hypan@google.com>
> Cr-Commit-Position: refs/heads/master@{#789079}

TBR=agl@chromium.org,jsaul@google.com,martinkr@google.com,hypan@google.com

# Not skipping CQ checks because this is a reland.

Change-Id: I0a07662127df43624d7fa69c165d2503107514aa
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2303567Reviewed-by: default avatarHaiyang Pan <hypan@google.com>
Commit-Queue: Martin Kreichgauer <martinkr@google.com>
Cr-Commit-Position: refs/heads/master@{#789304}
parent 5e05c54e
...@@ -7,7 +7,6 @@ ...@@ -7,7 +7,6 @@
#include <string> #include <string>
#include <utility> #include <utility>
#include "base/timer/timer.h"
#include "content/browser/webauth/authenticator_common.h" #include "content/browser/webauth/authenticator_common.h"
#include "content/public/browser/navigation_handle.h" #include "content/public/browser/navigation_handle.h"
#include "content/public/browser/render_frame_host.h" #include "content/public/browser/render_frame_host.h"
...@@ -18,20 +17,12 @@ namespace content { ...@@ -18,20 +17,12 @@ namespace content {
InternalAuthenticatorImpl::InternalAuthenticatorImpl( InternalAuthenticatorImpl::InternalAuthenticatorImpl(
RenderFrameHost* render_frame_host) 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)), : WebContentsObserver(WebContents::FromRenderFrameHost(render_frame_host)),
render_frame_host_(render_frame_host), render_frame_host_(render_frame_host),
effective_origin_(render_frame_host->GetLastCommittedOrigin()), 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(render_frame_host_);
DCHECK(authenticator_common_);
// Disabling WebAuthn modal dialogs to avoid conflict with Autofill's own // Disabling WebAuthn modal dialogs to avoid conflict with Autofill's own
// modal dialogs. Since WebAuthn is designed for websites, rather than browser // modal dialogs. Since WebAuthn is designed for websites, rather than browser
// components, the UI can be confusing for users in the case for Autofill. // components, the UI can be confusing for users in the case for Autofill.
......
...@@ -57,14 +57,6 @@ class InternalAuthenticatorImpl : public autofill::InternalAuthenticator, ...@@ -57,14 +57,6 @@ class InternalAuthenticatorImpl : public autofill::InternalAuthenticator,
private: private:
friend class InternalAuthenticatorImplTest; 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() { AuthenticatorCommon* get_authenticator_common_for_testing() {
return authenticator_common_.get(); return authenticator_common_.get();
} }
......
...@@ -534,15 +534,11 @@ base::flat_set<device::FidoTransportProtocol> GetAvailableTransports( ...@@ -534,15 +534,11 @@ base::flat_set<device::FidoTransportProtocol> GetAvailableTransports(
} // namespace } // namespace
AuthenticatorCommon::AuthenticatorCommon( AuthenticatorCommon::AuthenticatorCommon(RenderFrameHost* render_frame_host)
RenderFrameHost* render_frame_host,
std::unique_ptr<base::OneShotTimer> timer)
: render_frame_host_(render_frame_host), : render_frame_host_(render_frame_host),
security_checker_(static_cast<RenderFrameHostImpl*>(render_frame_host) security_checker_(static_cast<RenderFrameHostImpl*>(render_frame_host)
->GetWebAuthRequestSecurityChecker()), ->GetWebAuthRequestSecurityChecker()) {
timer_(std::move(timer)) {
DCHECK(render_frame_host_); DCHECK(render_frame_host_);
DCHECK(timer_);
// Disable the back-forward cache for any document that makes WebAuthn // Disable the back-forward cache for any document that makes WebAuthn
// requests. Pages using privacy-sensitive APIs are generally exempt from // requests. Pages using privacy-sensitive APIs are generally exempt from
// back-forward cache for now as a precaution. // back-forward cache for now as a precaution.
......
...@@ -16,6 +16,7 @@ ...@@ -16,6 +16,7 @@
#include "base/containers/span.h" #include "base/containers/span.h"
#include "base/macros.h" #include "base/macros.h"
#include "base/optional.h" #include "base/optional.h"
#include "base/timer/timer.h"
#include "content/common/content_export.h" #include "content/common/content_export.h"
#include "content/public/browser/authenticator_request_client_delegate.h" #include "content/public/browser/authenticator_request_client_delegate.h"
#include "content/public/browser/web_contents_observer.h" #include "content/public/browser/web_contents_observer.h"
...@@ -67,9 +68,7 @@ CONTENT_EXPORT extern const char kGetType[]; ...@@ -67,9 +68,7 @@ CONTENT_EXPORT extern const char kGetType[];
// Common code for any WebAuthn Authenticator interfaces. // Common code for any WebAuthn Authenticator interfaces.
class CONTENT_EXPORT AuthenticatorCommon { class CONTENT_EXPORT AuthenticatorCommon {
public: public:
// Permits setting timer for testing. explicit AuthenticatorCommon(RenderFrameHost* render_frame_host);
AuthenticatorCommon(RenderFrameHost* render_frame_host,
std::unique_ptr<base::OneShotTimer>);
virtual ~AuthenticatorCommon(); virtual ~AuthenticatorCommon();
// This is not-quite an implementation of blink::mojom::Authenticator. The // This is not-quite an implementation of blink::mojom::Authenticator. The
...@@ -183,7 +182,8 @@ class CONTENT_EXPORT AuthenticatorCommon { ...@@ -183,7 +182,8 @@ class CONTENT_EXPORT AuthenticatorCommon {
url::Origin caller_origin_; url::Origin caller_origin_;
std::string relying_party_id_; std::string relying_party_id_;
scoped_refptr<WebAuthRequestSecurityChecker> security_checker_; 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> base::Optional<device::AuthenticatorSelectionCriteria>
authenticator_selection_criteria_; authenticator_selection_criteria_;
base::Optional<std::string> app_id_; base::Optional<std::string> app_id_;
......
...@@ -17,10 +17,9 @@ ...@@ -17,10 +17,9 @@
namespace content { namespace content {
AuthenticatorImpl::AuthenticatorImpl(RenderFrameHost* render_frame_host) AuthenticatorImpl::AuthenticatorImpl(RenderFrameHost* render_frame_host)
: AuthenticatorImpl(render_frame_host, : AuthenticatorImpl(
std::make_unique<AuthenticatorCommon>( render_frame_host,
render_frame_host, std::make_unique<AuthenticatorCommon>(render_frame_host)) {}
std::make_unique<base::OneShotTimer>())) {}
AuthenticatorImpl::AuthenticatorImpl( AuthenticatorImpl::AuthenticatorImpl(
RenderFrameHost* render_frame_host, RenderFrameHost* render_frame_host,
......
...@@ -46,9 +46,8 @@ class CONTENT_EXPORT AuthenticatorImpl : public blink::mojom::Authenticator, ...@@ -46,9 +46,8 @@ class CONTENT_EXPORT AuthenticatorImpl : public blink::mojom::Authenticator,
public: public:
explicit AuthenticatorImpl(RenderFrameHost* render_frame_host); explicit AuthenticatorImpl(RenderFrameHost* render_frame_host);
// By being able to set AuthenticatorCommon, this constructor permits setting // Constructs an AuthenticatorImpl with an injected AuthenticatorCommon for
// the timer for testing. Using this constructor will also empty out the // testing.
// protocol set, since no device discovery will take place during tests.
AuthenticatorImpl(RenderFrameHost* render_frame_host, AuthenticatorImpl(RenderFrameHost* render_frame_host,
std::unique_ptr<AuthenticatorCommon> authenticator_common); std::unique_ptr<AuthenticatorCommon> authenticator_common);
~AuthenticatorImpl() override; ~AuthenticatorImpl() override;
......
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