wifi_sync: allow WifiCredentialSyncableServiceFactory to ignore LoginState

components/wifi_sync/wifi_credential_syncable_service_factory.cc

Index: components/wifi_sync/wifi_credential_syncable_service_factory.cc
diff --git a/components/wifi_sync/wifi_credential_syncable_service_factory.cc b/components/wifi_sync/wifi_credential_syncable_service_factory.cc
index cf3b46020cc7d1c61b50313e0da097190e90d282..708336e16ea0a63a5e9b6da884bc3b3d0781c9d3 100644
--- a/components/wifi_sync/wifi_credential_syncable_service_factory.cc
+++ b/components/wifi_sync/wifi_credential_syncable_service_factory.cc
@@ -4,12 +4,17 @@
 
 #include "components/wifi_sync/wifi_credential_syncable_service_factory.h"
 
+#include <string>
+
 #include "base/logging.h"
 #include "base/memory/scoped_ptr.h"
 #include "components/keyed_service/content/browser_context_dependency_manager.h"
+#include "components/wifi_sync/wifi_config_delegate.h"
 #include "components/wifi_sync/wifi_credential_syncable_service.h"
+#include "content/public/browser/browser_context.h"
 
 #if defined(OS_CHROMEOS)
+#include "base/files/file_path.h"
 #include "chromeos/login/login_state.h"
 #include "chromeos/network/network_handler.h"
 #include "components/wifi_sync/wifi_config_delegate_chromeos.h"
@@ -19,26 +24,28 @@ namespace wifi_sync {
 
 namespace {
 
-scoped_ptr<WifiConfigDelegate> BuildConfigDelegate(
-    content::BrowserContext* context) {
 #if defined(OS_CHROMEOS)
-  const chromeos::LoginState* login_state = chromeos::LoginState::Get();
-  DCHECK(login_state->IsUserLoggedIn());
-  DCHECK(!login_state->primary_user_hash().empty());
-  // TODO(quiche): Verify that |context| is the primary user's context.
-
-  // Note: NetworkHandler is a singleton that is managed by
-  // ChromeBrowserMainPartsChromeos, and destroyed after all
-  // KeyedService instances are destroyed.
-  chromeos::NetworkHandler* network_handler = chromeos::NetworkHandler::Get();
-  return make_scoped_ptr(new WifiConfigDelegateChromeOs(
-      login_state->primary_user_hash(),
-      network_handler->managed_network_configuration_handler()));
-#else
-  NOTREACHED();
-  return nullptr;
-#endif
+// Returns a string identifying a ChromeOS network settings profile,
+// by that profile's UserHash property. This value may be communicated
+// to the ChromeOS connection manager ("Shill"), but must not be
+// exposed to any untrusted code (e.g., via web APIs).

[mukesh agrawal, 2015/02/06 02:40:55]

I believe this (newly added) comment captures how the hash will be used, and
also circumscribes inappropriate uses in the future.

+std::string GetUserHash(content::BrowserContext* context,
+                        bool use_login_state) {
+  if (use_login_state) {
+    const chromeos::LoginState* login_state = chromeos::LoginState::Get();
+    DCHECK(login_state->IsUserLoggedIn());
+    DCHECK(!login_state->primary_user_hash().empty());
+    // TODO(quiche): Verify that |context| is the primary user's context.
+    return login_state->primary_user_hash();
+  } else {
+    // In WiFi credential sync tests, LoginState is not
+    // available. Instead, those tests set their Shill profiles'
+    // UserHashes based on the corresponding BrowserContexts' storage
+    // paths.

[mukesh agrawal, 2015/02/06 02:40:55]

This (newly added) comment should explain why this a "good" hash. (It's good in
the sense that it's a value that the sync tests and this factory can share.)

WRT why it's okay, I assume you mean okay from a privacy perspective. For that,
see the comment at line 28.

+    return context->GetPath().BaseName().value();
+  }
 }
+#endif
 
 }  // namespace
 
@@ -71,7 +78,27 @@ KeyedService* WifiCredentialSyncableServiceFactory::BuildServiceInstanceFor(
     content::BrowserContext* context) const {
   // TODO(quiche): Figure out if this behaves properly for multi-profile.
   // crbug.com/430681.
-  return new WifiCredentialSyncableService(BuildConfigDelegate(context));
+#if defined(OS_CHROMEOS)
+  return new WifiCredentialSyncableService(
+      BuildWifiConfigDelegateChromeOs(context));
+#else
+  NOTREACHED();
+  return nullptr;
+#endif
+}
+
+#if defined(OS_CHROMEOS)
+scoped_ptr<WifiConfigDelegate>
+WifiCredentialSyncableServiceFactory::BuildWifiConfigDelegateChromeOs(
+    content::BrowserContext* context) const {
+  // Note: NetworkHandler is a singleton that is managed by
+  // ChromeBrowserMainPartsChromeos, and destroyed after all
+  // KeyedService instances are destroyed.
+  chromeos::NetworkHandler* network_handler = chromeos::NetworkHandler::Get();
+  return make_scoped_ptr(new WifiConfigDelegateChromeOs(
+      GetUserHash(context, !ignore_login_state_for_test_),
+      network_handler->managed_network_configuration_handler()));
 }
+#endif
 
 }  // namespace wifi_sync