[Dice] Parse the Dice response header

components/signin/core/browser/signin_header_helper_unittest.cc

Index: components/signin/core/browser/signin_header_helper_unittest.cc
diff --git a/components/signin/core/browser/signin_header_helper_unittest.cc b/components/signin/core/browser/signin_header_helper_unittest.cc
index a0433c6fc6680c9ef3e5a48efcaa1ba07c543295..157ba76b3c207895ef5676aaa4ac80bb20e6a85b 100644
--- a/components/signin/core/browser/signin_header_helper_unittest.cc
+++ b/components/signin/core/browser/signin_header_helper_unittest.cc
@@ -9,6 +9,7 @@
 
 #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"
@@ -24,6 +25,8 @@
 #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 {
@@ -42,10 +45,10 @@ class SigninHeaderHelperTest : public testing::Test {
   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(
@@ -54,9 +57,9 @@ class SigninHeaderHelperTest : public testing::Test {
     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;
   }
 
@@ -80,7 +83,7 @@ class SigninHeaderHelperTest : public testing::Test {
     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)
@@ -90,11 +93,10 @@ class SigninHeaderHelperTest : public testing::Test {
                               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
 
@@ -222,6 +224,30 @@ TEST_F(SigninHeaderHelperTest, TestMirrorRequestDrive) {
       "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
@@ -235,11 +261,11 @@ TEST_F(SigninHeaderHelperTest, TestMirrorHeaderEligibleRedirectURL) {
   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
@@ -253,11 +279,11 @@ TEST_F(SigninHeaderHelperTest, TestMirrorHeaderNonEligibleRedirectURL) {
   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
@@ -272,13 +298,15 @@ TEST_F(SigninHeaderHelperTest, TestIgnoreMirrorHeaderNonEligibleURLs) {
   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