[Dice] Parse the Dice response header

components/signin/core/browser/signin_header_helper_unittest.cc

 // Copyright 2016 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 "components/signin/core/browser/signin_header_helper.h"
 
 #include <memory>
 #include <string>
 
 #include "base/command_line.h"
 #include "base/message_loop/message_loop.h"
+#include "base/strings/stringprintf.h"
 #include "build/build_config.h"
 #include "components/content_settings/core/browser/cookie_settings.h"
 #include "components/signin/core/browser/chrome_connected_header_helper.h"
 #include "components/signin/core/common/profile_management_switches.h"
 #include "components/sync_preferences/testing_pref_service_syncable.h"
 #include "google_apis/gaia/gaia_urls.h"
 #include "net/traffic_annotation/network_traffic_annotation_test_helper.h"
 #include "net/url_request/url_request_test_util.h"
 #include "testing/gtest/include/gtest/gtest.h"
 #include "url/gurl.h"
 
 #if !defined(OS_IOS) && !defined(OS_ANDROID)
 #include "components/signin/core/browser/dice_header_helper.h"
 #endif
 
+namespace signin {

[msarda, 2017/06/12 11:52:21]

I think that tests can be in an anonymous namespace. What is the reason for
having them in the |signin| namespace (is it just for convenience)?

[droger, 2017/06/12 12:39:16]

Yes, this is for convenience (removing the signin:: prefix everywhere).

[msarda, 2017/06/12 14:03:42]

Acknowledged.

+
 class SigninHeaderHelperTest : public testing::Test {
  protected:
   void SetUp() override {
     content_settings::CookieSettings::RegisterProfilePrefs(prefs_.registry());
     HostContentSettingsMap::RegisterProfilePrefs(prefs_.registry());
 
     settings_map_ = new HostContentSettingsMap(
         &prefs_, false /* incognito_profile */, false /* guest_profile */,
         false /* store_last_modified */);
     cookie_settings_ =
         new content_settings::CookieSettings(settings_map_.get(), &prefs_, "");
   }
 
   void TearDown() override { settings_map_->ShutdownOnUIThread(); }
 
   void CheckMirrorCookieRequest(const GURL& url,
                                 const std::string& account_id,
                                 const std::string& expected_request) {
-    EXPECT_EQ(signin::BuildMirrorRequestCookieIfPossible(
-                  url, account_id, cookie_settings_.get(),
-                  signin::PROFILE_MODE_DEFAULT),
-              expected_request);
+    EXPECT_EQ(
+        BuildMirrorRequestCookieIfPossible(
+            url, account_id, cookie_settings_.get(), PROFILE_MODE_DEFAULT),
+        expected_request);
   }
 
   std::unique_ptr<net::URLRequest> CreateRequest(
       const GURL& url,
       const std::string& account_id) {
     std::unique_ptr<net::URLRequest> url_request =
         url_request_context_.CreateRequest(url, net::DEFAULT_PRIORITY, nullptr,
                                            TRAFFIC_ANNOTATION_FOR_TESTS);
-    signin::AppendOrRemoveAccountConsistentyRequestHeader(
+    AppendOrRemoveAccountConsistentyRequestHeader(
         url_request.get(), GURL(), account_id, cookie_settings_.get(),
-        signin::PROFILE_MODE_DEFAULT);
+        PROFILE_MODE_DEFAULT);
     return url_request;
   }
 
   void CheckAccountConsistencyHeaderRequest(
       net::URLRequest* url_request,
       const char* header_name,
       const std::string& expected_request) {
     bool expected_result = !expected_request.empty();
     std::string request;
     EXPECT_EQ(
         url_request->extra_request_headers().GetHeader(header_name, &request),
         expected_result);
     if (expected_result) {
       EXPECT_EQ(expected_request, request);
     }
   }
 
   void CheckMirrorHeaderRequest(const GURL& url,
                                 const std::string& account_id,
                                 const std::string& expected_request) {
     std::unique_ptr<net::URLRequest> url_request =
         CreateRequest(url, account_id);
     CheckAccountConsistencyHeaderRequest(
-        url_request.get(), signin::kChromeConnectedHeader, expected_request);
+        url_request.get(), kChromeConnectedHeader, expected_request);
   }
 
 #if !defined(OS_IOS) && !defined(OS_ANDROID)
   void CheckDiceHeaderRequest(const GURL& url,
                               const std::string& account_id,
                               const std::string& expected_mirror_request,
                               const std::string& expected_dice_request) {
     std::unique_ptr<net::URLRequest> url_request =
         CreateRequest(url, account_id);
-    CheckAccountConsistencyHeaderRequest(url_request.get(),
-                                         signin::kChromeConnectedHeader,
-                                         expected_mirror_request);
     CheckAccountConsistencyHeaderRequest(
-        url_request.get(), signin::kDiceRequestHeader, expected_dice_request);
+        url_request.get(), kChromeConnectedHeader, expected_mirror_request);
+    CheckAccountConsistencyHeaderRequest(url_request.get(), kDiceRequestHeader,
+                                         expected_dice_request);
   }
 #endif
 
   base::MessageLoop loop_;
 
   sync_preferences::TestingPrefServiceSyncable prefs_;
   net::TestURLRequestContext url_request_context_;
 
   scoped_refptr<HostContentSettingsMap> settings_map_;
   scoped_refptr<content_settings::CookieSettings> cookie_settings_;
@@ skipping 107 matching lines @@
   switches::EnableAccountConsistencyMirrorForTesting(
       base::CommandLine::ForCurrentProcess());
   CheckMirrorHeaderRequest(
       GURL("https://docs.google.com/document"), "0123456789",
       "id=0123456789,mode=0,enable_account_consistency=true");
   CheckMirrorCookieRequest(
       GURL("https://drive.google.com/drive"), "0123456789",
       "id=0123456789:mode=0:enable_account_consistency=true");
 }
 
+TEST_F(SigninHeaderHelperTest, TestDiceInvalidResponseParams) {
+  DiceResponseParams params = BuildDiceResponseParams("blah");
+  EXPECT_EQ(DiceAction::NONE, params.user_intention);
+}
+
+TEST_F(SigninHeaderHelperTest, TestBuildDiceResponseParams) {
+  const char kAuthorizationCode[] = "authorization_code";
+  const char kEmail[] = "foo@example.com";
+  const char kLogoutURL[] = "https://example.com/logout";
+  const char kObfuscatedGaiaID[] = "obfuscated_gaia_id";
+  const char kSessionIndex[] = "session_index";
+
+  DiceResponseParams params = BuildDiceResponseParams(base::StringPrintf(
+      "action=%s,id=%s,email=%s,authuser=%s,authorization_code=%s,logout=%s",
+      "SIGNIN", kObfuscatedGaiaID, kEmail, kSessionIndex, kAuthorizationCode,
+      kLogoutURL));
+  EXPECT_EQ(DiceAction::SIGNIN, params.user_intention);

[msarda, 2017/06/12 11:52:21]

I think we should test the following cases:
* valid sign-in: "action=SIGNIN,id=..,authuser=..,authorization_code=.."
* invalid sign-in (missing authorization code, which would match a bug on Gaia):
"action=..,id=..,authuser=.."
* valid logout:
"action=SIGNOUT,id=..,authuser=..,session_index=..,logout_url=.."

[droger, 2017/06/12 12:39:16]

SIGNIN and SIGNOUT done.

However: what would be the expected behavior in the "invalid signin"? Were you
thinking that we should also do some validation in this function and for example
return DiceAction::NONE if some fields are missing?

[msarda, 2017/06/12 14:03:42]

I wanted a test that shows how the parse is done in case where an argument is
missing. I think we just need a test (it is up to you what the right behavior
would be in this case).

[droger, 2017/06/12 16:12:58]

Done. I'm now doing validation and return NONE if headers are missing. Added
test cases for this.

+  EXPECT_EQ(kObfuscatedGaiaID, params.obfuscated_gaia_id);
+  EXPECT_EQ(kEmail, params.email);
+  EXPECT_EQ(kSessionIndex, params.session_index);
+  EXPECT_EQ(kAuthorizationCode, params.authorization_code);
+  EXPECT_EQ(GURL(kLogoutURL), params.logout_url);
+}
+
 #endif  // !defined(OS_ANDROID) && !defined(OS_IOS)
 
 // Tests that the Mirror header request is returned normally when the redirect
 // URL is eligible.
 TEST_F(SigninHeaderHelperTest, TestMirrorHeaderEligibleRedirectURL) {
   switches::EnableAccountConsistencyMirrorForTesting(
       base::CommandLine::ForCurrentProcess());
   const GURL url("https://docs.google.com/document");
   const GURL redirect_url("https://www.google.com");
   const std::string account_id = "0123456789";
   std::unique_ptr<net::URLRequest> url_request =
       url_request_context_.CreateRequest(url, net::DEFAULT_PRIORITY, nullptr,
                                          TRAFFIC_ANNOTATION_FOR_TESTS);
-  signin::AppendOrRemoveAccountConsistentyRequestHeader(
+  AppendOrRemoveAccountConsistentyRequestHeader(
       url_request.get(), redirect_url, account_id, cookie_settings_.get(),
-      signin::PROFILE_MODE_DEFAULT);
-  EXPECT_TRUE(url_request->extra_request_headers().HasHeader(
-      signin::kChromeConnectedHeader));
+      PROFILE_MODE_DEFAULT);
+  EXPECT_TRUE(
+      url_request->extra_request_headers().HasHeader(kChromeConnectedHeader));
 }
 
 // Tests that the Mirror header request is stripped when the redirect URL is not
 // eligible.
 TEST_F(SigninHeaderHelperTest, TestMirrorHeaderNonEligibleRedirectURL) {
   switches::EnableAccountConsistencyMirrorForTesting(
       base::CommandLine::ForCurrentProcess());
   const GURL url("https://docs.google.com/document");
   const GURL redirect_url("http://www.foo.com");
   const std::string account_id = "0123456789";
   std::unique_ptr<net::URLRequest> url_request =
       url_request_context_.CreateRequest(url, net::DEFAULT_PRIORITY, nullptr,
                                          TRAFFIC_ANNOTATION_FOR_TESTS);
-  signin::AppendOrRemoveAccountConsistentyRequestHeader(
+  AppendOrRemoveAccountConsistentyRequestHeader(
       url_request.get(), redirect_url, account_id, cookie_settings_.get(),
-      signin::PROFILE_MODE_DEFAULT);
-  EXPECT_FALSE(url_request->extra_request_headers().HasHeader(
-      signin::kChromeConnectedHeader));
+      PROFILE_MODE_DEFAULT);
+  EXPECT_FALSE(
+      url_request->extra_request_headers().HasHeader(kChromeConnectedHeader));
 }
 
 // Tests that the Mirror header, whatever its value is, is untouched when both
 // the current and the redirect URL are non-eligible.
 TEST_F(SigninHeaderHelperTest, TestIgnoreMirrorHeaderNonEligibleURLs) {
   switches::EnableAccountConsistencyMirrorForTesting(
       base::CommandLine::ForCurrentProcess());
   const GURL url("https://www.bar.com");
   const GURL redirect_url("http://www.foo.com");
   const std::string account_id = "0123456789";
   const std::string fake_header = "foo,bar";
   std::unique_ptr<net::URLRequest> url_request =
       url_request_context_.CreateRequest(url, net::DEFAULT_PRIORITY, nullptr,
                                          TRAFFIC_ANNOTATION_FOR_TESTS);
-  url_request->SetExtraRequestHeaderByName(signin::kChromeConnectedHeader,
-                                           fake_header, false);
-  signin::AppendOrRemoveAccountConsistentyRequestHeader(
+  url_request->SetExtraRequestHeaderByName(kChromeConnectedHeader, fake_header,
+                                           false);
+  AppendOrRemoveAccountConsistentyRequestHeader(
       url_request.get(), redirect_url, account_id, cookie_settings_.get(),
-      signin::PROFILE_MODE_DEFAULT);
+      PROFILE_MODE_DEFAULT);
   std::string header;
   EXPECT_TRUE(url_request->extra_request_headers().GetHeader(
-      signin::kChromeConnectedHeader, &header));
+      kChromeConnectedHeader, &header));
   EXPECT_EQ(fake_header, header);
 }
+
+}  // namespace signin