[signin] Unit test for DiceResponseHandler

chrome/browser/signin/dice_response_handler_unittest.cc

Index: chrome/browser/signin/dice_response_handler_unittest.cc
diff --git a/chrome/browser/signin/dice_response_handler_unittest.cc b/chrome/browser/signin/dice_response_handler_unittest.cc
new file mode 100644
index 0000000000000000000000000000000000000000..aed6afdb35a659f74b8c417035588c379607eaa1
--- /dev/null
+++ b/chrome/browser/signin/dice_response_handler_unittest.cc
@@ -0,0 +1,123 @@
+// Copyright 2017 The Chromium Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+#include "chrome/browser/signin/dice_response_handler.h"
+
+#include "base/command_line.h"
+#include "base/memory/ref_counted.h"
+#include "base/message_loop/message_loop.h"
+#include "base/test/test_simple_task_runner.h"
+#include "chrome/test/base/testing_profile.h"
+#include "components/signin/core/browser/account_tracker_service.h"
+#include "components/signin/core/browser/profile_oauth2_token_service.h"
+#include "components/signin/core/browser/signin_header_helper.h"
+#include "components/signin/core/browser/test_signin_client.h"
+#include "components/signin/core/common/profile_management_switches.h"
+#include "components/sync_preferences/testing_pref_service_syncable.h"
+#include "content/public/test/test_browser_thread_bundle.h"
+#include "google_apis/gaia/fake_oauth2_token_service_delegate.h"
+#include "net/url_request/url_request_test_util.h"
+#include "testing/gmock/include/gmock/gmock.h"
+#include "testing/gtest/include/gtest/gtest.h"
+
+using signin::DiceAction;
+using signin::DiceResponseParams;
+
+namespace {
+
+const char kAuthorizationCode[] = "authorization_code";
+const char kEmail[] = "email";
+const char kObfuscatedGaiaID[] = "obfuscated_gaia_id";

[msarda, 2017/06/22 09:29:48]

I think we can drop the obfuscated from the Gaia ID - all Gaia IDs are
obfuscated in Chrome.

[droger, 2017/06/22 13:21:20]

SGTM.

I'm doing this in another CL:
https://codereview.chromium.org/2944383006

+const int kSessionIndex = 42;
+
+// TestSigninClient implementation that intercepts the GaiaAuthConsumer and
+// replaces it by a dummy one.
+class DiceTestSigninClient : public TestSigninClient, public GaiaAuthConsumer {
+ public:
+  explicit DiceTestSigninClient(PrefService* pref_service)
+      : TestSigninClient(pref_service), consumer_(nullptr) {}
+
+  std::unique_ptr<GaiaAuthFetcher> CreateGaiaAuthFetcher(

[msarda, 2017/06/22 09:29:48]

Should this be override?

[droger, 2017/06/22 13:21:20]

Yes, it is override. Did you miss it on line 44?

+      GaiaAuthConsumer* consumer,
+      const std::string& source,
+      net::URLRequestContextGetter* getter) override {
+    DCHECK(!consumer_ || (consumer_ == consumer));
+    consumer_ = consumer;
+
+    // Pass |this| as a dummy consumer. Drop everything on the floor.

[msarda, 2017/06/22 09:29:48]

I do not understand this comment - this method is not returning |this|.

[droger, 2017/06/22 13:21:20]

I am passing |this| to CreateGaiaAuthFetcher instead of the original consumer.
Clarified the comment.

+    return TestSigninClient::CreateGaiaAuthFetcher(this, source, getter);
+  }
+
+  GaiaAuthConsumer* consumer_;
+};
+
+class DiceResponseHandlerTest : public testing::Test {
+ protected:
+  DiceResponseHandlerTest()

[msarda, 2017/06/22 09:29:47]

I think in general we prefer to do the initialization in SetUp and the tear down
in TearDown, not in the constructor. Is there a reason for doing them in the
constructor?

[droger, 2017/06/22 13:21:19]

This is the other way around. Initialization in SetUp is discouraged.
See:
https://github.com/google/googletest/blob/master/googletest/docs/FAQ.md#shoul...

+      : task_runner_(new base::TestSimpleTaskRunner()),
+        request_context_getter_(
+            new net::TestURLRequestContextGetter(task_runner_)),
+        signin_client_(&pref_service_),
+        token_service_(base::MakeUnique<FakeOAuth2TokenServiceDelegate>(
+            request_context_getter_.get())),
+        dice_response_handler_(&signin_client_,
+                               &token_service_,
+                               &account_tracker_service_) {
+    switches::EnableAccountConsistencyDiceForTesting(
+        base::CommandLine::ForCurrentProcess());
+    signin_client_.SetURLRequestContext(request_context_getter_.get());
+    AccountTrackerService::RegisterPrefs(pref_service_.registry());
+    account_tracker_service_.Initialize(&signin_client_);
+  }
+
+  ~DiceResponseHandlerTest() override {}
+
+  DiceResponseParams MakeDiceParams(DiceAction action) {
+    DiceResponseParams dice_params;
+    dice_params.user_intention = action;
+    dice_params.obfuscated_gaia_id = kObfuscatedGaiaID;

[msarda, 2017/06/22 09:29:47]

Same remark as above - the Gaia ID in Chrome is always obfuscated - Gaia never
shares the non-obfuscated ID with clients. Let's rename this to gaia_id to avoid
any confusion in the future.

+    dice_params.email = kEmail;
+    dice_params.session_index = kSessionIndex;
+    dice_params.authorization_code = kAuthorizationCode;
+    return dice_params;
+  }
+
+  base::MessageLoop loop_;

[msarda, 2017/06/22 09:29:48]

Is the message loop needed?

[droger, 2017/06/22 13:21:20]

Yes, GaiaAuthFetcher requires it.

Removing it gives a crash:

[38249:38249:0622/151854.856368:2089634038704:FATAL:sequenced_task_runner_handle.cc(54)]
Check failed: pool. 
#0 0x7f2a570d454b base::debug::StackTrace::StackTrace()
#1 0x7f2a570d324c base::debug::StackTrace::StackTrace()
#2 0x7f2a57147703 logging::LogMessage::~LogMessage()
#3 0x7f2a572bb587 base::SequencedTaskRunnerHandle::Get()
#4 0x7f2a5a90549b net::URLFetcherCore::URLFetcherCore()
#5 0x7f2a5a91415a net::URLFetcherImpl::URLFetcherImpl()
#6 0x7f2a5a904a6a net::URLFetcher::Create()
#7 0x0000022fab5b GaiaAuthFetcher::CreateAndStartGaiaFetcher()

+  scoped_refptr<base::TestSimpleTaskRunner> task_runner_;
+  scoped_refptr<net::TestURLRequestContextGetter> request_context_getter_;
+  sync_preferences::TestingPrefServiceSyncable pref_service_;
+  DiceTestSigninClient signin_client_;
+  ProfileOAuth2TokenService token_service_;
+  AccountTrackerService account_tracker_service_;
+  DiceResponseHandler dice_response_handler_;
+};
+
+// Checks that a SIGNIN action triggers a token exchange request.
+TEST_F(DiceResponseHandlerTest, Signin) {
+  DiceResponseParams dice_params = MakeDiceParams(DiceAction::SIGNIN);
+  ASSERT_FALSE(
+      token_service_.RefreshTokenIsAvailable(dice_params.obfuscated_gaia_id));
+  dice_response_handler_.ProcessDiceHeader(dice_params);
+  // Check that a GaiaAuthFetcher has been created.
+  ASSERT_TRUE(signin_client_.consumer_);
+  // Simulate GaiaAuthFetcher success.
+  signin_client_.consumer_->OnClientOAuthSuccess(

[msarda, 2017/06/22 09:29:47]

Please also add a test for OnClientOAuthFailure.

[droger, 2017/06/22 13:21:20]

Doing this in another CL because then I can use the
GetPendingDiceTokenFetchersCountForTesting() function for that test.

+      GaiaAuthConsumer::ClientOAuthResult("refresh_token", "access_token", 10));
+  // Check that the token has been inserted in the token service.
+  EXPECT_TRUE(
+      token_service_.RefreshTokenIsAvailable(dice_params.obfuscated_gaia_id));
+}
+
+// Tests that the DiceResponseHandler is created for a normal profile but not
+// for an incognito profile.
+TEST(DiceResponseHandlerFactoryTest, NotInIncognito) {
+  content::TestBrowserThreadBundle thread_bundle;
+  TestingProfile profile;
+  EXPECT_THAT(DiceResponseHandler::GetForProfile(&profile), testing::NotNull());
+  EXPECT_THAT(
+      DiceResponseHandler::GetForProfile(profile.GetOffTheRecordProfile()),
+      testing::IsNull());
+}
+
+}  // namespace