Injecting an NSHTTPCookieStorage dependency into CookieStoreIOS

ios/net/cookies/cookie_store_ios.mm

 // Copyright 2012 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 "ios/net/cookies/cookie_store_ios.h"
 
 #import <Foundation/Foundation.h>
 
 #include "base/bind.h"
 #include "base/files/file_path.h"
@@ skipping 182 matching lines @@
   // to cookies if one doesn't already exist.
   DLOG_IF(ERROR, created_a.is_null() || created_b.is_null())
       << "Cookie without creation date";
 #endif
   if (created_a < created_b)
     return NSOrderedAscending;
   return (created_a > created_b) ? NSOrderedDescending : NSOrderedSame;
 }
 
 // Gets the cookies for |url| from the system cookie store.
-NSArray* GetCookiesForURL(const GURL& url, CookieCreationTimeManager* manager) {
-  NSArray* cookies = [[NSHTTPCookieStorage sharedHTTPCookieStorage]
-      cookiesForURL:net::NSURLWithGURL(url)];
+NSArray* GetCookiesForURL(NSHTTPCookieStorage* system_store,
+                          const GURL& url, CookieCreationTimeManager* manager) {
+  NSArray* cookies = [system_store cookiesForURL:net::NSURLWithGURL(url)];
 
   // Sort cookies by decreasing path length, then creation time, as per RFC6265.
   return [cookies sortedArrayUsingFunction:CompareCookies context:manager];
 }
 
 // Builds a cookie line (such as "key1=value1; key2=value2") from an array of
 // cookies.
 std::string BuildCookieLine(NSArray* cookies,
                             const net::CookieOptions& options) {
   // The exclude_httponly() option would only be used by a javascript engine.
@@ skipping 51 matching lines @@
   return cookie.HasDomain();
 }
 
 }  // namespace
 
 #pragma mark -
 #pragma mark CookieStoreIOS
 
 CookieStoreIOS::CookieStoreIOS(
     net::CookieMonster::PersistentCookieStore* persistent_store)
-    : creation_time_manager_(new CookieCreationTimeManager),
+    : CookieStoreIOS(persistent_store,
+                     [NSHTTPCookieStorage sharedHTTPCookieStorage]) {
+}
+
+CookieStoreIOS::CookieStoreIOS(
+    net::CookieMonster::PersistentCookieStore* persistent_store,
+    NSHTTPCookieStorage* system_store)
+    : system_store_(system_store),
+      creation_time_manager_(new CookieCreationTimeManager),
       metrics_enabled_(false),
       flush_delay_(base::TimeDelta::FromSeconds(10)),
       synchronization_state_(NOT_SYNCHRONIZED),
       cookie_cache_(new CookieCache()) {
+  DCHECK(system_store);
+
   NotificationTrampoline::GetInstance()->AddObserver(this);
   cookie_monster_ = new net::CookieMonster(persistent_store, nullptr);
   cookie_monster_->SetPersistSessionCookies(true);
   cookie_monster_->SetForceKeepSessionState();
 }
 
 // static
 void CookieStoreIOS::SetCookiePolicy(CookiePolicy setting) {
   NSHTTPCookieAcceptPolicy policy = (setting == ALLOW)
                                         ? NSHTTPCookieAcceptPolicyAlways
                                         : NSHTTPCookieAcceptPolicyNever;
   NSHTTPCookieStorage* store = [NSHTTPCookieStorage sharedHTTPCookieStorage];

[droger, 2015/10/30 17:02:51]

Using sharedHTTPCookieStorage here is a bit scary.

I would suggest either
- passing the cookie storage as a parameter, or
- passing the shared NSHTTPCookieStorage to the NotifyCookiePolicyChanged()
function, and in  OnSystemCookiePolicyChanged(), do a early return if the cookie
storages don't match.

In the latter case, please also add a comment in the .h saying that the cookie
policy only affects the CookieStoreIOS instances that are backed by the
[NSHTTPCookieStorage sharedHTTPCookieStorage].

[droger, 2015/10/30 17:02:51]

Also, not for this CL, but you should make sure that whatever custom cookie
store you inject honors the cookie policy set by the user.

[shreyasv1, 2015/11/02 22:21:44]

I don't really understand the need to pass a NSHTTPCookieStorage* param into
NotifyCookiePolicyChanged because this is an internal method.

What I think is a tad cleaner is that I have added a comment on the
CookieNotificationObserver saying that these
methods are called only for NSHTTPCookieStorge sharedHTTPCookieStorage. My
concern with adding a param on  OnSystemCookiePolicyChanged this leads to
incongruity. e.g. OnSystemCookiePolicyChanged takes an NSHTTPCookieStorage param
but OnSystemCookiesChanged does not.

I have added comments and early returns  in the relevant places.

[shreyasv1, 2015/11/02 22:21:44]

Acknowledged.

[droger, 2015/11/03 09:49:00]

Ok, what you did works too. The important bit was to do early returns if the
cookie stores don't match.

   NSHTTPCookieAcceptPolicy current_policy = [store cookieAcceptPolicy];
   if (current_policy == policy)
     return;
   [store setCookieAcceptPolicy:policy];
   NotificationTrampoline::GetInstance()->NotifyCookiePolicyChanged();
 }
 
-CookieStoreIOS* CookieStoreIOS::CreateCookieStoreFromNSHTTPCookieStorage() {
+// static
+CookieStoreIOS* CookieStoreIOS::CreateCookieStore(
+    NSHTTPCookieStorage* cookie_storage) {
   // TODO(huey): Update this when CrNet supports multiple cookie jars.
-  [[NSHTTPCookieStorage sharedHTTPCookieStorage]
-      setCookieAcceptPolicy:NSHTTPCookieAcceptPolicyAlways];
+  [cookie_storage setCookieAcceptPolicy:NSHTTPCookieAcceptPolicyAlways];
 
   // Create a cookie store with no persistent store backing. Then, populate
   // it from the system's cookie jar.
   CookieStoreIOS* cookie_store = new CookieStoreIOS(nullptr);
   cookie_store->synchronization_state_ = SYNCHRONIZED;
   cookie_store->Flush(base::Closure());
   return cookie_store;
 }
 
 // static
@@ skipping 10 matching lines @@
 void CookieStoreIOS::NotifySystemCookiesChanged() {
   NotificationTrampoline::GetInstance()->NotifyCookiesChanged();
 }
 
 void CookieStoreIOS::Flush(const base::Closure& closure) {
   DCHECK(thread_checker_.CalledOnValidThread());
 
   if (SystemCookiesAllowed()) {
     // If cookies are disabled, the system store is empty, and the cookies are
     // stashed on disk. Do not delete the cookies on the disk in this case.
-    WriteToCookieMonster(
-        [[NSHTTPCookieStorage sharedHTTPCookieStorage] cookies]);
+    WriteToCookieMonster([system_store_ cookies]);
   }
   cookie_monster_->FlushStore(closure);
   flush_closure_.Cancel();
 }
 
 void CookieStoreIOS::UnSynchronize() {
   SetSynchronizedWithSystemStore(false);
 }
 
 void CookieStoreIOS::SetMetricsEnabled() {
@@ skipping 54 matching lines @@
       //   b) The Domain attribute, if present, was not malformed
       //   c) At least one of:
       //       1) The cookie had no explicit Domain, so the Domain was inferred
       //          from the URL, or
       //       2) The cookie had an explicit Domain for which the URL is allowed
       //          to set cookies.
       bool success = (cookie != nil) && !domain_string.empty() &&
                      (!has_explicit_domain || has_valid_domain);
 
       if (success) {
-        [[NSHTTPCookieStorage sharedHTTPCookieStorage] setCookie:cookie];
+        [system_store_ setCookie:cookie];
         creation_time_manager_->SetCreationTime(
             cookie,
             creation_time_manager_->MakeUniqueCreationTime(base::Time::Now()));
       }
 
       if (!callback.is_null())
         callback.Run(success);
       break;
   }
 }
@@ skipping 14 matching lines @@
                      options, callback));
       break;
     case SYNCHRONIZED:
       // If cookies are not allowed, they are stashed in the CookieMonster, and
       // should be read from there instead.
       DCHECK(SystemCookiesAllowed());
       // The exclude_httponly() option would only be used by a javascript
       // engine.
       DCHECK(!options.exclude_httponly());
 
-      NSArray* cookies = GetCookiesForURL(url, creation_time_manager_.get());
+      NSArray* cookies = GetCookiesForURL(system_store_,
+                                          url, creation_time_manager_.get());
       if (!callback.is_null())
         callback.Run(BuildCookieLine(cookies, options));
       break;
   }
 }
 
 void CookieStoreIOS::GetAllCookiesForURLAsync(
     const GURL& url,
     const GetCookieListCallback& callback) {
   DCHECK(thread_checker_.CalledOnValidThread());
 
   switch (synchronization_state_) {
     case NOT_SYNCHRONIZED:
       cookie_monster_->GetAllCookiesForURLAsync(url, callback);
       break;
     case SYNCHRONIZING:
       tasks_pending_synchronization_.push_back(base::Bind(
           &CookieStoreIOS::GetAllCookiesForURLAsync, this, url, callback));
       break;
     case SYNCHRONIZED:
       if (!SystemCookiesAllowed()) {
         // If cookies are not allowed, the cookies are stashed in the
         // CookieMonster, so get them from there.
         cookie_monster_->GetAllCookiesForURLAsync(url, callback);
         return;
       }
 
-      NSArray* cookies = GetCookiesForURL(url, creation_time_manager_.get());
+      NSArray* cookies = GetCookiesForURL(system_store_,
+                                          url, creation_time_manager_.get());
       net::CookieList cookie_list;
       cookie_list.reserve([cookies count]);
       for (NSHTTPCookie* cookie in cookies) {
         base::Time created = creation_time_manager_->GetCreationTime(cookie);
         cookie_list.push_back(CanonicalCookieFromSystemCookie(cookie, created));
       }
       if (!callback.is_null())
         callback.Run(cookie_list);
       break;
   }
 }
 
 void CookieStoreIOS::DeleteCookieAsync(const GURL& url,
                                        const std::string& cookie_name,
                                        const base::Closure& callback) {
   DCHECK(thread_checker_.CalledOnValidThread());
 
   switch (synchronization_state_) {
     case NOT_SYNCHRONIZED:
       cookie_monster_->DeleteCookieAsync(url, cookie_name,
                                          WrapClosure(callback));
       break;
     case SYNCHRONIZING:
       tasks_pending_synchronization_.push_back(
           base::Bind(&CookieStoreIOS::DeleteCookieAsync, this, url, cookie_name,
                      WrapClosure(callback)));
       break;
     case SYNCHRONIZED:
-      NSArray* cookies = GetCookiesForURL(url, creation_time_manager_.get());
+      NSArray* cookies = GetCookiesForURL(system_store_,
+                                          url, creation_time_manager_.get());
       for (NSHTTPCookie* cookie in cookies) {
         if ([[cookie name]
                 isEqualToString:base::SysUTF8ToNSString(cookie_name)]) {
-          [[NSHTTPCookieStorage sharedHTTPCookieStorage] deleteCookie:cookie];
+          [system_store_ deleteCookie:cookie];
           creation_time_manager_->DeleteCreationTime(cookie);
         }
       }
       if (!callback.is_null())
         callback.Run();
       break;
   }
 }
 
 // CookieStoreIOS is an implementation of CookieStore which is not a
@@ skipping 88 matching lines @@
 CookieStoreIOS::~CookieStoreIOS() {
   NotificationTrampoline::GetInstance()->RemoveObserver(this);
   STLDeleteContainerPairSecondPointers(hook_map_.begin(), hook_map_.end());
 }
 
 #pragma mark -
 #pragma mark Private methods
 
 void CookieStoreIOS::ClearSystemStore() {
   DCHECK(thread_checker_.CalledOnValidThread());
-  NSHTTPCookieStorage* cookie_storage =
-      [NSHTTPCookieStorage sharedHTTPCookieStorage];
   base::scoped_nsobject<NSArray> copy(
-      [[NSArray alloc] initWithArray:[cookie_storage cookies]]);
+      [[NSArray alloc] initWithArray:[system_store_ cookies]]);
   for (NSHTTPCookie* cookie in copy.get())
-    [cookie_storage deleteCookie:cookie];
-  DCHECK_EQ(0u, [[cookie_storage cookies] count]);
+    [system_store_ deleteCookie:cookie];
+  DCHECK_EQ(0u, [[system_store_ cookies] count]);
   creation_time_manager_->Clear();
 }
 
 void CookieStoreIOS::OnSystemCookiePolicyChanged() {
   DCHECK(thread_checker_.CalledOnValidThread());
 
   if (synchronization_state_ == NOT_SYNCHRONIZED)
     return;
 
   NSHTTPCookieAcceptPolicy policy =
-      [[NSHTTPCookieStorage sharedHTTPCookieStorage] cookieAcceptPolicy];
+      [system_store_ cookieAcceptPolicy];
   if (policy == NSHTTPCookieAcceptPolicyAlways) {
     // If cookies are disabled, the system cookie store should be empty.
-    DCHECK(![[[NSHTTPCookieStorage sharedHTTPCookieStorage] cookies] count]);
+    DCHECK(![[system_store_ cookies] count]);
     DCHECK(synchronization_state_ != SYNCHRONIZING);
     synchronization_state_ = SYNCHRONIZING;
     cookie_monster_->GetAllCookiesAsync(
         base::Bind(&CookieStoreIOS::AddCookiesToSystemStore, this));
   } else {
     DCHECK_EQ(NSHTTPCookieAcceptPolicyNever, policy);
     // Flush() does not write the cookies to disk when they are disabled.
     // Explicitly copy them.
-    WriteToCookieMonster(
-        [[NSHTTPCookieStorage sharedHTTPCookieStorage] cookies]);
+    WriteToCookieMonster([system_store_ cookies]);
     Flush(base::Closure());
     ClearSystemStore();
     if (synchronization_state_ == SYNCHRONIZING) {
       // If synchronization was in progress, abort it and leave the cookie store
       // empty.
       // Temporarily toggle the synchronization state so that pending tasks are
       // redirected to cookie_monster_ and can complete normally.
       synchronization_state_ = NOT_SYNCHRONIZED;
       RunAllPendingTasks();
       synchronization_state_ = SYNCHRONIZED;
@@ skipping 13 matching lines @@
         << "This cookie store was not synchronized";
     g_current_synchronized_store = nullptr;
   } else {
     DCHECK_EQ((CookieStoreIOS*)nullptr, g_current_synchronized_store)
         << "Un-synchronize the current cookie store first.";
     g_current_synchronized_store = this;
   }
 #endif
 
   NSHTTPCookieAcceptPolicy policy =
-      [[NSHTTPCookieStorage sharedHTTPCookieStorage] cookieAcceptPolicy];
+      [system_store_ cookieAcceptPolicy];
   DCHECK(policy == NSHTTPCookieAcceptPolicyAlways ||
          policy == NSHTTPCookieAcceptPolicyNever);
 
   // If cookies are disabled, the system cookie store should be empty.
   DCHECK(policy == NSHTTPCookieAcceptPolicyAlways ||
-         ![[[NSHTTPCookieStorage sharedHTTPCookieStorage] cookies] count]);
+         ![[system_store_ cookies] count]);
 
   // If cookies are disabled, nothing is done now, the work will be done when
   // cookies are re-enabled.
   if (policy == NSHTTPCookieAcceptPolicyAlways) {
     if (synchronized) {
       synchronization_state_ = SYNCHRONIZING;
       ClearSystemStore();
       cookie_monster_->GetAllCookiesAsync(
           base::Bind(&CookieStoreIOS::AddCookiesToSystemStore, this));
       return;
     } else {
       // Copy the cookies from the global store to |cookie_monster_|.
       Flush(base::Closure());
     }
   }
   synchronization_state_ = synchronized ? SYNCHRONIZED : NOT_SYNCHRONIZED;
 
   if (synchronization_state_ == NOT_SYNCHRONIZED) {
     // If there are pending tasks, then it means that the synchronization is
     // being canceled. All pending tasks can be sent to cookie_monster_.
     RunAllPendingTasks();
   }
 }
 
 bool CookieStoreIOS::SystemCookiesAllowed() {
   DCHECK(thread_checker_.CalledOnValidThread());
-  return [[NSHTTPCookieStorage sharedHTTPCookieStorage] cookieAcceptPolicy] ==
+  return [system_store_ cookieAcceptPolicy] ==
          NSHTTPCookieAcceptPolicyAlways;
 }
 
 void CookieStoreIOS::AddCookiesToSystemStore(const net::CookieList& cookies) {
   DCHECK(thread_checker_.CalledOnValidThread());
   if (!SystemCookiesAllowed() || synchronization_state_ != SYNCHRONIZING) {
     // If synchronization was aborted, the pending tasks have been processed at
     // that time. Now is too late.
     DCHECK(tasks_pending_synchronization_.empty());
     return;
   }
 
   // Report metrics.
   if (metrics_enabled_) {
     size_t cookie_count = cookies.size();
     UMA_HISTOGRAM_COUNTS_10000("CookieIOS.CookieReadCount", cookie_count);
     CheckForCookieLoss(cookie_count, COOKIES_READ);
   }
 
   net::CookieList::const_iterator it;
   for (it = cookies.begin(); it != cookies.end(); ++it) {
     const net::CanonicalCookie& net_cookie = *it;
     NSHTTPCookie* system_cookie = SystemCookieFromCanonicalCookie(net_cookie);
     // Canonical cookie may not be convertable into system cookie if it contains
     // invalid characters.
     if (!system_cookie)
       continue;
-    [[NSHTTPCookieStorage sharedHTTPCookieStorage] setCookie:system_cookie];
+    [system_store_ setCookie:system_cookie];
     creation_time_manager_->SetCreationTime(system_cookie,
                                             net_cookie.CreationDate());
   }
 
   synchronization_state_ = SYNCHRONIZED;
   RunAllPendingTasks();
 }
 
 void CookieStoreIOS::WriteToCookieMonster(NSArray* system_cookies) {
   DCHECK(thread_checker_.CalledOnValidThread());
@@ skipping 24 matching lines @@
   for (const auto& task : tasks_pending_synchronization_) {
     task.Run();
   }
   tasks_pending_synchronization_.clear();
 }
 
 void CookieStoreIOS::DeleteCookiesWithFilter(const CookieFilterFunction& filter,
                                              const DeleteCallback& callback) {
   DCHECK(thread_checker_.CalledOnValidThread());
   DCHECK_EQ(SYNCHRONIZED, synchronization_state_);
-  NSArray* cookies = [[NSHTTPCookieStorage sharedHTTPCookieStorage] cookies];
+  NSArray* cookies = [system_store_ cookies];
 
   // Collect the cookies to delete.
   base::scoped_nsobject<NSMutableArray> to_delete(
       [[NSMutableArray alloc] init]);
   for (NSHTTPCookie* cookie in cookies) {
     base::Time creation_time = creation_time_manager_->GetCreationTime(cookie);
     if (filter.Run(cookie, creation_time))
       [to_delete addObject:cookie];
   }
 
   // Delete them.
   for (NSHTTPCookie* cookie in to_delete.get()) {
-    [[NSHTTPCookieStorage sharedHTTPCookieStorage] deleteCookie:cookie];
+    [system_store_ deleteCookie:cookie];
     creation_time_manager_->DeleteCreationTime(cookie);
   }
 
   if (!callback.is_null())
     callback.Run([to_delete count]);
 }
 
 void CookieStoreIOS::OnSystemCookiesChanged() {
   DCHECK(thread_checker_.CalledOnValidThread());
 
@@ skipping 68 matching lines @@
     callbacks->Notify(cookie, removed);
   }
 }
 
 bool CookieStoreIOS::GetSystemCookies(
     const GURL& gurl,
     const std::string& name,
     std::vector<net::CanonicalCookie>* cookies) {
   DCHECK(thread_checker_.CalledOnValidThread());
   NSURL* url = net::NSURLWithGURL(gurl);
-  NSHTTPCookieStorage* storage = [NSHTTPCookieStorage sharedHTTPCookieStorage];
-  NSArray* nscookies = [storage cookiesForURL:url];
+  NSArray* nscookies = [system_store_ cookiesForURL:url];
   bool found_cookies = false;
   for (NSHTTPCookie* nscookie in nscookies) {
     if (nscookie.name.UTF8String == name) {
       net::CanonicalCookie canonical_cookie = CanonicalCookieFromSystemCookie(
           nscookie, creation_time_manager_->GetCreationTime(nscookie));
       cookies->push_back(canonical_cookie);
       found_cookies = true;
     }
   }
   return found_cookies;
@@ skipping 82 matching lines @@
   DCHECK(thread_checker_.CalledOnValidThread());
   return base::Bind(&CookieStoreIOS::UpdateCachesAfterDelete, this, callback);
 }
 
 base::Closure CookieStoreIOS::WrapClosure(const base::Closure& callback) {
   DCHECK(thread_checker_.CalledOnValidThread());
   return base::Bind(&CookieStoreIOS::UpdateCachesAfterClosure, this, callback);
 }
 
 }  // namespace net