Fix copying of SAML IdP cookies on subsequent logins

The code that copies cookies set by a SAML IdP on subsequent logins is
wrong as it uses a method which modifies the cookie being copied by
prepending a dot to the domain name. This CL fixes the problem by
switching to the method that is used during initial login. The method
is also renamed to make it clearer that it may be used on subsequent
logins too.

BUG=410416
TEST=unit_tests updated to detect this problem

TBR=dbeam (wallet_signin_helper_unittest.cc)

Review URL: https://codereview.chromium.org/538543003

Cr-Commit-Position: refs/heads/master@{#293524}

chrome/browser/chromeos/login/profile_auth_data.cc

@@ -286,31 +286,19 @@ void ProfileAuthDataTransferer::MaybeTransferCookiesAndChannelIDs() {
       to_context_->GetURLRequestContext()->cookie_store();
   net::CookieMonster* to_monster = to_store->GetCookieMonster();
   if (first_login_) {
-    to_monster->InitializeFrom(cookies_to_transfer_);
+    to_monster->ImportCookies(cookies_to_transfer_);
     net::ChannelIDService* to_cert_service =
         to_context_->GetURLRequestContext()->channel_id_service();
     to_cert_service->GetChannelIDStore()->InitializeFrom(
         channel_ids_to_transfer_);
   } else {
+    net::CookieList non_gaia_cookies;
     for (net::CookieList::const_iterator it = cookies_to_transfer_.begin();
          it != cookies_to_transfer_.end(); ++it) {
-      if (IsGAIACookie(*it))
-        continue;
-      // Although this method can be asynchronous, it will run synchronously in
-      // this case as the target cookie jar is guaranteed to be loaded and
-      // ready.
-      to_monster->SetCookieWithDetailsAsync(
-          GURL(it->Source()),
-          it->Name(),
-          it->Value(),
-          it->Domain(),
-          it->Path(),
-          it->ExpiryDate(),
-          it->IsSecure(),
-          it->IsHttpOnly(),
-          it->Priority(),
-          net::CookieStore::SetCookiesCallback());
+      if (!IsGAIACookie(*it))
+        non_gaia_cookies.push_back(*it);
     }
+    to_monster->ImportCookies(non_gaia_cookies);
   }
 
   Finish();

chrome/browser/chromeos/login/profile_auth_data_unittest.cc

@@ -47,8 +47,8 @@ const char kSAMLIdPCookieURL[] = "http://example.com/";
 const char kCookieName[] = "cookie";
 const char kCookieValue1[] = "value 1";
 const char kCookieValue2[] = "value 2";
-const char kGAIACookieDomain[] = ".google.com";
-const char kSAMLIdPCookieDomain[] = ".example.com";
+const char kGAIACookieDomain[] = "google.com";
+const char kSAMLIdPCookieDomain[] = "example.com";
 
 const char kChannelIDServerIdentifier[] = "server";
 const char kChannelIDPrivateKey1[] = "private key 1";
@@ -91,7 +91,7 @@ class ProfileAuthDataTest : public testing::Test {
   net::CookieMonster* GetCookies(content::BrowserContext* browser_context);
   net::ChannelIDStore* GetChannelIDs(content::BrowserContext* browser_context);
 
-  void QuitLoop(bool ignored);
+  void QuitLoop(const net::CookieList& ignored);
   void StoreCookieListAndQuitLoop(const net::CookieList& cookie_list);
   void StoreChannelIDListAndQuitLoop(
       const net::ChannelIDStore::ChannelIDList& channel_id_list);
@@ -178,13 +178,15 @@ void ProfileAuthDataTest::VerifyUserCookies(
   net::CookieList user_cookies = GetUserCookies();
   ASSERT_EQ(2u, user_cookies.size());
   net::CanonicalCookie* cookie = &user_cookies[0];
-  EXPECT_EQ(kGAIACookieURL, cookie->Source());
-  EXPECT_EQ(kCookieName, cookie->Name());
-  EXPECT_EQ(expected_gaia_cookie_value, cookie->Value());
-  cookie = &user_cookies[1];
   EXPECT_EQ(kSAMLIdPCookieURL, cookie->Source());
   EXPECT_EQ(kCookieName, cookie->Name());
   EXPECT_EQ(expected_saml_idp_cookie_value, cookie->Value());
+  EXPECT_EQ(kSAMLIdPCookieDomain, cookie->Domain());
+  cookie = &user_cookies[1];
+  EXPECT_EQ(kGAIACookieURL, cookie->Source());
+  EXPECT_EQ(kCookieName, cookie->Name());
+  EXPECT_EQ(expected_gaia_cookie_value, cookie->Value());
+  EXPECT_EQ(kGAIACookieDomain, cookie->Domain());
 }
 
 void ProfileAuthDataTest::VerifyUserChannelID(
@@ -214,33 +216,37 @@ void ProfileAuthDataTest::PopulateBrowserContext(
       std::string());
 
   net::CookieMonster* cookies = GetCookies(browser_context);
+  // Ensure |cookies| is fully initialized.
   run_loop_.reset(new base::RunLoop);
-  cookies->SetCookieWithDetailsAsync(
-      GURL(kGAIACookieURL),
-      kCookieName,
-      cookie_value,
-      kGAIACookieDomain,
-      std::string(),
-      base::Time(),
-      true,
-      false,
-      net::COOKIE_PRIORITY_DEFAULT,
-      base::Bind(&ProfileAuthDataTest::QuitLoop, base::Unretained(this)));
-  run_loop_->Run();
-  run_loop_.reset(new base::RunLoop);
-  cookies->SetCookieWithDetailsAsync(
-      GURL(kSAMLIdPCookieURL),
-      kCookieName,
-      cookie_value,
-      kSAMLIdPCookieDomain,
-      std::string(),
-      base::Time(),
-      true,
-      false,
-      net::COOKIE_PRIORITY_DEFAULT,
-      base::Bind(&ProfileAuthDataTest::QuitLoop, base::Unretained(this)));
+  cookies->GetAllCookiesAsync(base::Bind(&ProfileAuthDataTest::QuitLoop,
+                                         base::Unretained(this)));
   run_loop_->Run();
 
+  net::CookieList cookie_list;
+  cookie_list.push_back(net::CanonicalCookie(GURL(kGAIACookieURL),
+                                             kCookieName,
+                                             cookie_value,
+                                             kGAIACookieDomain,
+                                             std::string(),
+                                             base::Time(),
+                                             base::Time(),
+                                             base::Time(),
+                                             true,
+                                             false,
+                                             net::COOKIE_PRIORITY_DEFAULT));
+  cookie_list.push_back(net::CanonicalCookie(GURL(kSAMLIdPCookieURL),
+                                             kCookieName,
+                                             cookie_value,
+                                             kSAMLIdPCookieDomain,
+                                             std::string(),
+                                             base::Time(),
+                                             base::Time(),
+                                             base::Time(),
+                                             true,
+                                             false,
+                                             net::COOKIE_PRIORITY_DEFAULT));
+  cookies->ImportCookies(cookie_list);
+
   GetChannelIDs(browser_context)->SetChannelID(kChannelIDServerIdentifier,
                                                base::Time(),
                                                base::Time(),
@@ -270,7 +276,7 @@ net::ChannelIDStore* ProfileAuthDataTest::GetChannelIDs(
       GetChannelIDStore();
 }
 
-void ProfileAuthDataTest::QuitLoop(bool ignored) {
+void ProfileAuthDataTest::QuitLoop(const net::CookieList& ignored) {
   run_loop_->Quit();
 }
 

components/autofill/content/browser/wallet/wallet_signin_helper_unittest.cc

@@ -144,7 +144,7 @@ TEST_F(WalletSigninHelperTest, GetWalletCookieValueWhenPresent) {
 
   net::CookieList cookie_list;
   cookie_list.push_back(*cookie);
-  cookie_monster->InitializeFrom(cookie_list);
+  cookie_monster->ImportCookies(cookie_list);
   request_context_->GetURLRequestContext()
       ->set_cookie_store(cookie_monster);
   signin_helper_->StartWalletCookieValueFetch();
@@ -166,7 +166,7 @@ TEST_F(WalletSigninHelperTest, GetWalletCookieValueWhenMissing) {
 
   net::CookieList cookie_list;
   cookie_list.push_back(*cookie);
-  cookie_monster->InitializeFrom(cookie_list);
+  cookie_monster->ImportCookies(cookie_list);
   request_context_->GetURLRequestContext()
       ->set_cookie_store(cookie_monster);
   signin_helper_->StartWalletCookieValueFetch();

net/cookies/cookie_monster.cc

@@ -1101,7 +1101,7 @@ bool CookieMonster::SetCookieWithDetails(const GURL& url,
   return SetCanonicalCookie(&cc, creation_time, options);
 }
 
-bool CookieMonster::InitializeFrom(const CookieList& list) {
+bool CookieMonster::ImportCookies(const CookieList& list) {
   base::AutoLock autolock(lock_);
   InitIfNecessary();
   for (net::CookieList::const_iterator iter = list.begin();

net/cookies/cookie_monster.h

@@ -146,8 +146,9 @@ class NET_EXPORT CookieMonster : public CookieStore {
                 CookieMonsterDelegate* delegate,
                 int last_access_threshold_milliseconds);
 
-  // Helper function that adds all cookies from |list| into this instance.
-  bool InitializeFrom(const CookieList& list);
+  // Helper function that adds all cookies from |list| into this instance,
+  // overwriting any equivalent cookies.
+  bool ImportCookies(const CookieList& list);
 
   typedef base::Callback<void(const CookieList& cookies)> GetCookieListCallback;
   typedef base::Callback<void(bool success)> DeleteCookieCallback;

net/cookies/cookie_monster_unittest.cc

@@ -1391,7 +1391,7 @@ TEST_F(CookieMonsterTest, DeleteCookieByName) {
   }
 }
 
-TEST_F(CookieMonsterTest, InitializeFromCookieMonster) {
+TEST_F(CookieMonsterTest, ImportCookiesFromCookieMonster) {
   scoped_refptr<CookieMonster> cm_1(new CookieMonster(NULL, NULL));
   CookieOptions options;
 
@@ -1407,7 +1407,7 @@ TEST_F(CookieMonsterTest, InitializeFromCookieMonster) {
 
   CookieList cookies_1 = GetAllCookies(cm_1.get());
   scoped_refptr<CookieMonster> cm_2(new CookieMonster(NULL, NULL));
-  ASSERT_TRUE(cm_2->InitializeFrom(cookies_1));
+  ASSERT_TRUE(cm_2->ImportCookies(cookies_1));
   CookieList cookies_2 = GetAllCookies(cm_2.get());
 
   size_t expected_size = 3;