Handle case that network status returned asynchronously in ForceSigninVerifier.

When the GetConnectionType returns asynchronously, verification request is never
sent because the default network type is CONNECTION_NONE and there is no network
change notification later either.

Resolves the issue by handling the callback from the API.

Also
1) Separate network check from others so that we don't fetch network type again
   when we know it.
2) Rewrite the unittest to use more fake services instead of stun value. It
   increase the code coverage. Including SigninManager, ProfileOAuth2TokenService
   and TestNetworkConnectionTracker.

Bug: 891817
Change-Id: Iebdc5bbc30e699b347d6a343bb6f4c979fbe1596
Reviewed-on: https://chromium-review.googlesource.com/c/1259542Reviewed-by: Mihai Sardarescu <msarda@chromium.org>
Commit-Queue: Owen Min <zmin@chromium.org>
Cr-Commit-Position: refs/heads/master@{#597560}

chrome/browser/signin/force_signin_verifier.cc

@@ -92,8 +92,7 @@ void ForceSigninVerifier::OnConnectionChanged(
   if (backoff_request_timer_.IsRunning())
     backoff_request_timer_.Stop();
 
-  if (type != network::mojom::ConnectionType::CONNECTION_NONE)
-    SendRequest();
+  SendRequestIfNetworkAvailable(type);
 }
 
 void ForceSigninVerifier::Cancel() {
@@ -108,8 +107,21 @@ bool ForceSigninVerifier::HasTokenBeenVerified() {
 }
 
 void ForceSigninVerifier::SendRequest() {
-  if (!ShouldSendRequest())
+  auto type = network::mojom::ConnectionType::CONNECTION_NONE;
+  if (content::GetNetworkConnectionTracker()->GetConnectionType(
+          &type,
+          base::BindOnce(&ForceSigninVerifier::SendRequestIfNetworkAvailable,
+                         base::Unretained(this)))) {
+    SendRequestIfNetworkAvailable(type);
+  }
+}
+
+void ForceSigninVerifier::SendRequestIfNetworkAvailable(
+    network::mojom::ConnectionType network_type) {
+  if (network_type == network::mojom::ConnectionType::CONNECTION_NONE ||
+      !ShouldSendRequest()) {
     return;
+  }
 
   std::string account_id = signin_manager_->GetAuthenticatedAccountId();
   OAuth2TokenService::ScopeSet oauth2_scopes;
@@ -119,11 +131,7 @@ void ForceSigninVerifier::SendRequest() {
 }
 
 bool ForceSigninVerifier::ShouldSendRequest() {
-  auto type = network::mojom::ConnectionType::CONNECTION_NONE;
-  content::GetNetworkConnectionTracker()->GetConnectionType(&type,
-                                                            base::DoNothing());
   return !has_token_verified_ && access_token_request_.get() == nullptr &&
-         type != network::mojom::ConnectionType::CONNECTION_NONE &&
          signin_manager_->IsAuthenticated();
 }
 

chrome/browser/signin/force_signin_verifier.h

@@ -57,7 +57,12 @@ class ForceSigninVerifier
   //
   void SendRequest();
 
-  virtual bool ShouldSendRequest();
+  // Send the request if |network_type| is not CONNECTION_NONE and
+  // ShouldSendRequest returns true.
+  void SendRequestIfNetworkAvailable(
+      network::mojom::ConnectionType network_type);
+
+  bool ShouldSendRequest();
 
   virtual void CloseAllBrowserWindows();
 

chrome/browser/signin/force_signin_verifier_unittest.cc

@@ -4,9 +4,18 @@
 
 #include "chrome/browser/signin/force_signin_verifier.h"
 
+#include "base/run_loop.h"
 #include "base/test/metrics/histogram_tester.h"
+#include "base/threading/thread_task_runner_handle.h"
+#include "chrome/browser/signin/fake_profile_oauth2_token_service_builder.h"
+#include "chrome/browser/signin/fake_signin_manager_builder.h"
+#include "chrome/browser/signin/profile_oauth2_token_service_factory.h"
+#include "chrome/browser/signin/signin_manager_factory.h"
 #include "chrome/test/base/testing_profile.h"
+#include "components/signin/core/browser/fake_profile_oauth2_token_service.h"
+#include "content/public/browser/network_service_instance.h"
 #include "content/public/test/test_browser_thread_bundle.h"
+#include "services/network/test/test_network_connection_tracker.h"
 #include "testing/gmock/include/gmock/gmock.h"
 #include "testing/gtest/include/gtest/gtest.h"
 
@@ -15,10 +24,6 @@ class MockForceSigninVerifier : public ForceSigninVerifier {
   explicit MockForceSigninVerifier(Profile* profile)
       : ForceSigninVerifier(profile) {}
 
-  bool ShouldSendRequest() override { return true; }
-
-  void SendTestRequest() { SendRequest(); }
-
   bool IsDelayTaskPosted() { return GetOneShotTimerForTesting()->IsRunning(); }
 
   int FailureCount() { return GetBackoffEntryForTesting()->failure_count(); }
@@ -28,17 +33,42 @@ class MockForceSigninVerifier : public ForceSigninVerifier {
   MOCK_METHOD0(CloseAllBrowserWindows, void(void));
 };
 
-class ForceSigninVerifierTest : public ::testing::Test {
+class ForceSigninVerifierTest
+    : public ::testing::Test,
+      public network::NetworkConnectionTracker::NetworkConnectionObserver {
  public:
   void SetUp() override {
-    verifier_ = std::make_unique<MockForceSigninVerifier>(&profile_);
+    TestingProfile::Builder builder;
+    builder.AddTestingFactory(SigninManagerFactory::GetInstance(),
+                              BuildFakeSigninManagerBase);
+    builder.AddTestingFactory(ProfileOAuth2TokenServiceFactory::GetInstance(),
+                              BuildFakeProfileOAuth2TokenService);
+    profile_ = builder.Build();
+    FakeSigninManager* signin_manager_ = static_cast<FakeSigninManager*>(
+        SigninManagerFactory::GetForProfile(profile_.get()));
+    signin_manager_->SignIn("fake_id", "fake_username", "fake_password");
+
+    oauth2_token_service()->GetDelegate()->UpdateCredentials("fake_id",
+                                                             "fake_token");
   }
 
   void TearDown() override { verifier_.reset(); }
 
+  void OnConnectionChanged(network::mojom::ConnectionType type) override {
+    wait_for_network_type_change_.QuitWhenIdle();
+  }
+
+  FakeProfileOAuth2TokenService* oauth2_token_service() {
+    return static_cast<FakeProfileOAuth2TokenService*>(
+        ProfileOAuth2TokenServiceFactory::GetForProfile(profile_.get()));
+  }
+
   std::unique_ptr<MockForceSigninVerifier> verifier_;
   content::TestBrowserThreadBundle bundle_;
-  TestingProfile profile_;
+  std::unique_ptr<TestingProfile> profile_;
+
+  base::RunLoop wait_for_network_type_change_;
+  base::RunLoop wait_for_network_type_async_return_;
 
   GoogleServiceAuthError persistent_error_ = GoogleServiceAuthError(
       GoogleServiceAuthError::State::INVALID_GAIA_CREDENTIALS);
@@ -49,22 +79,23 @@ class ForceSigninVerifierTest : public ::testing::Test {
 };
 
 TEST_F(ForceSigninVerifierTest, OnGetTokenSuccess) {
-  ASSERT_EQ(nullptr, verifier_->request());
-
-  verifier_->SendTestRequest();
+  verifier_ = std::make_unique<MockForceSigninVerifier>(profile_.get());
 
   ASSERT_NE(nullptr, verifier_->request());
   ASSERT_FALSE(verifier_->HasTokenBeenVerified());
   ASSERT_FALSE(verifier_->IsDelayTaskPosted());
   EXPECT_CALL(*verifier_.get(), CloseAllBrowserWindows()).Times(0);
 
-  verifier_->OnGetTokenSuccess(verifier_->request(),
-                               OAuth2AccessTokenConsumer::TokenResponse());
+  FakeProfileOAuth2TokenService* service = oauth2_token_service();
+  ASSERT_EQ(1u, service->GetPendingRequests().size());
+  service->IssueAllTokensForAccount("fake_id",
+                                    OAuth2AccessTokenConsumer::TokenResponse());
+
   ASSERT_EQ(nullptr, verifier_->request());
   ASSERT_TRUE(verifier_->HasTokenBeenVerified());
   ASSERT_FALSE(verifier_->IsDelayTaskPosted());
   ASSERT_EQ(0, verifier_->FailureCount());
-  histogram_tester_.ExpectBucketCount(kForceSigninVerificationMetricsName, 0,
+  histogram_tester_.ExpectBucketCount(kForceSigninVerificationMetricsName, 1,
                                       1);
   histogram_tester_.ExpectTotalCount(
       kForceSigninVerificationSuccessTimeMetricsName, 1);
@@ -73,21 +104,22 @@ TEST_F(ForceSigninVerifierTest, OnGetTokenSuccess) {
 }
 
 TEST_F(ForceSigninVerifierTest, OnGetTokenPersistentFailure) {
-  ASSERT_EQ(nullptr, verifier_->request());
-
-  verifier_->SendTestRequest();
+  verifier_ = std::make_unique<MockForceSigninVerifier>(profile_.get());
 
   ASSERT_NE(nullptr, verifier_->request());
   ASSERT_FALSE(verifier_->HasTokenBeenVerified());
   ASSERT_FALSE(verifier_->IsDelayTaskPosted());
   EXPECT_CALL(*verifier_.get(), CloseAllBrowserWindows()).Times(1);
 
-  verifier_->OnGetTokenFailure(verifier_->request(), persistent_error_);
+  FakeProfileOAuth2TokenService* service = oauth2_token_service();
+  ASSERT_EQ(1u, service->GetPendingRequests().size());
+  service->IssueErrorForAllPendingRequests(persistent_error_);
+
   ASSERT_EQ(nullptr, verifier_->request());
   ASSERT_TRUE(verifier_->HasTokenBeenVerified());
   ASSERT_FALSE(verifier_->IsDelayTaskPosted());
   ASSERT_EQ(0, verifier_->FailureCount());
-  histogram_tester_.ExpectBucketCount(kForceSigninVerificationMetricsName, 0,
+  histogram_tester_.ExpectBucketCount(kForceSigninVerificationMetricsName, 1,
                                       1);
   histogram_tester_.ExpectTotalCount(
       kForceSigninVerificationSuccessTimeMetricsName, 0);
@@ -96,20 +128,22 @@ TEST_F(ForceSigninVerifierTest, OnGetTokenPersistentFailure) {
 }
 
 TEST_F(ForceSigninVerifierTest, OnGetTokenTransientFailure) {
-  ASSERT_EQ(nullptr, verifier_->request());
+  verifier_ = std::make_unique<MockForceSigninVerifier>(profile_.get());
 
-  verifier_->SendTestRequest();
   ASSERT_NE(nullptr, verifier_->request());
   ASSERT_FALSE(verifier_->HasTokenBeenVerified());
   ASSERT_FALSE(verifier_->IsDelayTaskPosted());
   EXPECT_CALL(*verifier_.get(), CloseAllBrowserWindows()).Times(0);
 
-  verifier_->OnGetTokenFailure(verifier_->request(), transient_error_);
+  FakeProfileOAuth2TokenService* service = oauth2_token_service();
+  ASSERT_EQ(1u, service->GetPendingRequests().size());
+  service->IssueErrorForAllPendingRequests(transient_error_);
+
   ASSERT_EQ(nullptr, verifier_->request());
   ASSERT_FALSE(verifier_->HasTokenBeenVerified());
   ASSERT_TRUE(verifier_->IsDelayTaskPosted());
   ASSERT_EQ(1, verifier_->FailureCount());
-  histogram_tester_.ExpectBucketCount(kForceSigninVerificationMetricsName, 0,
+  histogram_tester_.ExpectBucketCount(kForceSigninVerificationMetricsName, 1,
                                       1);
   histogram_tester_.ExpectTotalCount(
       kForceSigninVerificationSuccessTimeMetricsName, 0);
@@ -118,14 +152,22 @@ TEST_F(ForceSigninVerifierTest, OnGetTokenTransientFailure) {
 }
 
 TEST_F(ForceSigninVerifierTest, OnLostConnection) {
-  verifier_->SendTestRequest();
-  verifier_->OnGetTokenFailure(verifier_->request(), transient_error_);
+  verifier_ = std::make_unique<MockForceSigninVerifier>(profile_.get());
+
+  content::GetNetworkConnectionTracker()->AddNetworkConnectionObserver(this);
+
+  FakeProfileOAuth2TokenService* service = oauth2_token_service();
+  ASSERT_EQ(1u, service->GetPendingRequests().size());
+  service->IssueErrorForAllPendingRequests(transient_error_);
+
   ASSERT_EQ(1, verifier_->FailureCount());
   ASSERT_EQ(nullptr, verifier_->request());
   ASSERT_TRUE(verifier_->IsDelayTaskPosted());
 
-  verifier_->OnConnectionChanged(
+  network::TestNetworkConnectionTracker::GetInstance()->SetConnectionType(
       network::mojom::ConnectionType::CONNECTION_NONE);
+  wait_for_network_type_change_.Run();
+  content::GetNetworkConnectionTracker()->RemoveNetworkConnectionObserver(this);
 
   ASSERT_EQ(0, verifier_->FailureCount());
   ASSERT_EQ(nullptr, verifier_->request());
@@ -133,16 +175,140 @@ TEST_F(ForceSigninVerifierTest, OnLostConnection) {
 }
 
 TEST_F(ForceSigninVerifierTest, OnReconnected) {
-  verifier_->SendTestRequest();
-  verifier_->OnGetTokenFailure(verifier_->request(), transient_error_);
+  verifier_ = std::make_unique<MockForceSigninVerifier>(profile_.get());
+
+  content::GetNetworkConnectionTracker()->AddNetworkConnectionObserver(this);
+
+  FakeProfileOAuth2TokenService* service = oauth2_token_service();
+  ASSERT_EQ(1u, service->GetPendingRequests().size());
+  service->IssueErrorForAllPendingRequests(transient_error_);
+
   ASSERT_EQ(1, verifier_->FailureCount());
   ASSERT_EQ(nullptr, verifier_->request());
   ASSERT_TRUE(verifier_->IsDelayTaskPosted());
 
-  verifier_->OnConnectionChanged(
+  network::TestNetworkConnectionTracker::GetInstance()->SetConnectionType(
       network::mojom::ConnectionType::CONNECTION_WIFI);
+  wait_for_network_type_change_.Run();
+  content::GetNetworkConnectionTracker()->RemoveNetworkConnectionObserver(this);
 
   ASSERT_EQ(0, verifier_->FailureCount());
   ASSERT_NE(nullptr, verifier_->request());
   ASSERT_FALSE(verifier_->IsDelayTaskPosted());
 }
+
+TEST_F(ForceSigninVerifierTest, GetNetworkStatusAsync) {
+  network::TestNetworkConnectionTracker::GetInstance()->SetRespondSynchronously(
+      false);
+
+  verifier_ = std::make_unique<MockForceSigninVerifier>(profile_.get());
+
+  // There is no network type at first.
+  ASSERT_EQ(nullptr, verifier_->request());
+
+  // Waiting for the network type returns.
+  base::ThreadTaskRunnerHandle::Get()->PostTask(
+      FROM_HERE, wait_for_network_type_async_return_.QuitClosure());
+  wait_for_network_type_async_return_.Run();
+
+  // Get the type and send the request.
+  ASSERT_NE(nullptr, verifier_->request());
+}
+
+TEST_F(ForceSigninVerifierTest, LaunchVerifierWithoutNetwork) {
+  network::TestNetworkConnectionTracker::GetInstance()->SetConnectionType(
+      network::mojom::ConnectionType::CONNECTION_NONE);
+  network::TestNetworkConnectionTracker::GetInstance()->SetRespondSynchronously(
+      false);
+
+  verifier_ = std::make_unique<MockForceSigninVerifier>(profile_.get());
+
+  // There is no network type.
+  ASSERT_EQ(nullptr, verifier_->request());
+
+  // Waiting for the network type returns.
+  base::ThreadTaskRunnerHandle::Get()->PostTask(
+      FROM_HERE, wait_for_network_type_async_return_.QuitClosure());
+  wait_for_network_type_async_return_.Run();
+
+  // Get the type, there is no network connection, don't send the request.
+  ASSERT_EQ(nullptr, verifier_->request());
+
+  // Network is resumed.
+  content::GetNetworkConnectionTracker()->AddNetworkConnectionObserver(this);
+  network::TestNetworkConnectionTracker::GetInstance()->SetConnectionType(
+      network::mojom::ConnectionType::CONNECTION_WIFI);
+  wait_for_network_type_change_.Run();
+  content::GetNetworkConnectionTracker()->RemoveNetworkConnectionObserver(this);
+
+  // Send the request.
+  ASSERT_NE(nullptr, verifier_->request());
+}
+
+TEST_F(ForceSigninVerifierTest, ChangeNetworkFromWIFITo4GWithOnGoingRequest) {
+  network::TestNetworkConnectionTracker::GetInstance()->SetConnectionType(
+      network::mojom::ConnectionType::CONNECTION_WIFI);
+  network::TestNetworkConnectionTracker::GetInstance()->SetRespondSynchronously(
+      false);
+
+  verifier_ = std::make_unique<MockForceSigninVerifier>(profile_.get());
+  FakeProfileOAuth2TokenService* service = oauth2_token_service();
+
+  EXPECT_EQ(nullptr, verifier_->request());
+
+  // Waiting for the network type returns.
+  base::ThreadTaskRunnerHandle::Get()->PostTask(
+      FROM_HERE, wait_for_network_type_async_return_.QuitClosure());
+  wait_for_network_type_async_return_.Run();
+
+  // The network type if wifi, send the request.
+  auto* first_request = verifier_->request();
+  EXPECT_NE(nullptr, first_request);
+  EXPECT_EQ(1u, service->GetPendingRequests().size());
+
+  // Network is changed to 4G.
+  content::GetNetworkConnectionTracker()->AddNetworkConnectionObserver(this);
+  network::TestNetworkConnectionTracker::GetInstance()->SetConnectionType(
+      network::mojom::ConnectionType::CONNECTION_4G);
+  wait_for_network_type_change_.Run();
+  content::GetNetworkConnectionTracker()->RemoveNetworkConnectionObserver(this);
+
+  // There is still one on-going request.
+  EXPECT_EQ(first_request, verifier_->request());
+  EXPECT_EQ(1u, service->GetPendingRequests().size());
+}
+
+TEST_F(ForceSigninVerifierTest, ChangeNetworkFromWIFITo4GWithFinishedRequest) {
+  network::TestNetworkConnectionTracker::GetInstance()->SetConnectionType(
+      network::mojom::ConnectionType::CONNECTION_WIFI);
+  network::TestNetworkConnectionTracker::GetInstance()->SetRespondSynchronously(
+      false);
+
+  verifier_ = std::make_unique<MockForceSigninVerifier>(profile_.get());
+  FakeProfileOAuth2TokenService* service = oauth2_token_service();
+
+  EXPECT_EQ(nullptr, verifier_->request());
+
+  // Waiting for the network type returns.
+  base::ThreadTaskRunnerHandle::Get()->PostTask(
+      FROM_HERE, wait_for_network_type_async_return_.QuitClosure());
+  wait_for_network_type_async_return_.Run();
+
+  // The network type if wifi, send the request.
+  EXPECT_NE(nullptr, verifier_->request());
+
+  // Finishes the request.
+  service->IssueAllTokensForAccount("fake_id",
+                                    OAuth2AccessTokenConsumer::TokenResponse());
+  EXPECT_EQ(nullptr, verifier_->request());
+
+  // Network is changed to 4G.
+  content::GetNetworkConnectionTracker()->AddNetworkConnectionObserver(this);
+  network::TestNetworkConnectionTracker::GetInstance()->SetConnectionType(
+      network::mojom::ConnectionType::CONNECTION_4G);
+  wait_for_network_type_change_.Run();
+  content::GetNetworkConnectionTracker()->RemoveNetworkConnectionObserver(this);
+
+  // No more request because it's verfied already.
+  EXPECT_EQ(nullptr, verifier_->request());
+}