Prevent ONC-pushed certificates from being used with multiprofiles.

chrome/browser/chromeos/policy/policy_cert_service_factory.cc

Index: chrome/browser/chromeos/policy/policy_cert_service_factory.cc
diff --git a/chrome/browser/chromeos/policy/policy_cert_service_factory.cc b/chrome/browser/chromeos/policy/policy_cert_service_factory.cc
index f0e3d8eda24247260125d3881f3c2560af11083f..b888f5edad647d839c99ef55865dae531869f007 100644
--- a/chrome/browser/chromeos/policy/policy_cert_service_factory.cc
+++ b/chrome/browser/chromeos/policy/policy_cert_service_factory.cc
@@ -5,6 +5,11 @@
 #include "chrome/browser/chromeos/policy/policy_cert_service_factory.h"
 
 #include "base/memory/singleton.h"
+#include "base/prefs/pref_registry_simple.h"
+#include "base/prefs/pref_service.h"
+#include "base/prefs/scoped_user_pref_update.h"
+#include "chrome/browser/browser_process.h"
+#include "chrome/browser/chromeos/login/user_manager.h"
 #include "chrome/browser/chromeos/policy/policy_cert_service.h"
 #include "chrome/browser/chromeos/policy/policy_cert_verifier.h"
 #include "chrome/browser/chromeos/policy/user_network_configuration_updater_factory.h"
@@ -38,6 +43,38 @@ PolicyCertServiceFactory* PolicyCertServiceFactory::GetInstance() {
   return Singleton<PolicyCertServiceFactory>::get();
 }
 
+// static
+void PolicyCertServiceFactory::SetUsedPolicyCertificates(
+    const std::string& user_id) {
+  if (!UsedPolicyCertificates(user_id)) {

[pneubeck (no reviews), 2013/12/17 15:25:31]

if (Used...)
  return;

to remove indentation, if you like

[Joao da Silva, 2013/12/17 16:36:41]

Done.

+    ListPrefUpdate update(g_browser_process->local_state(),
+                          prefs::kUsedPolicyCertificates);

[pneubeck (no reviews), 2013/12/17 15:25:31]

is it safe to use local_state implicitly in static functions, or should we
better pass it as an argument?

[Joao da Silva, 2013/12/17 16:36:41]

That's a good point. This call is made from the service which only lives as long
as the Profile does, so this is safe.

[pneubeck (no reviews), 2013/12/17 17:57:00]

Yeah, in case of this function. The other to functions have the same "issue".

+    update->AppendString(user_id);
+  }
+}
+
+// static
+void PolicyCertServiceFactory::ClearUsedPolicyCertificates(
+    const std::string& user_id) {

[pneubeck (no reviews), 2013/12/17 15:25:31]

not sure whether we consistently use "user_id" as a synonym for "user_email".
If not, better use user_email consistently.

[Joao da Silva, 2013/12/17 16:36:41]

This comes from UserManager, which uses user_id.

+  ListPrefUpdate update(g_browser_process->local_state(),
+                        prefs::kUsedPolicyCertificates);
+  update->Remove(base::StringValue(user_id), NULL);
+}
+
+// static
+bool PolicyCertServiceFactory::UsedPolicyCertificates(
+    const std::string& user_id) {
+  base::StringValue value(user_id);
+  const base::ListValue* list =
+      g_browser_process->local_state()->GetList(prefs::kUsedPolicyCertificates);
+  return list && list->Find(value) != list->end();

[pneubeck (no reviews), 2013/12/17 15:25:31]

!list is an error so logging or DCHECK should be more correct.

[Joao da Silva, 2013/12/17 16:36:41]

Done.

+}
+
+// static
+void PolicyCertServiceFactory::RegisterPrefs(PrefRegistrySimple* local_state) {
+  local_state->RegisterListPref(prefs::kUsedPolicyCertificates);
+}
+
 PolicyCertServiceFactory::PolicyCertServiceFactory()
     : BrowserContextKeyedServiceFactory(
           "PolicyCertService",
@@ -50,15 +87,30 @@ PolicyCertServiceFactory::~PolicyCertServiceFactory() {}
 BrowserContextKeyedService* PolicyCertServiceFactory::BuildServiceInstanceFor(
     content::BrowserContext* context) const {
   Profile* profile = static_cast<Profile*>(context);
+
+  chromeos::UserManager* user_manager = chromeos::UserManager::Get();
+  chromeos::User* user =
+      user_manager->GetUserByProfile(profile->GetOriginalProfile());
+  if (!user)
+    return NULL;
+
+  // Backwards compatibility: profiles that used policy-pushed certificates used
+  // to have this condition marked in their prefs. This signal has moved to
+  // local_state though, to support checking it before the profile is loaded.
+  // Check the profile here and update the local_state, if appropriate.
+  // TODO(joaodasilva): remove this, eventually.
+  PrefService* prefs = profile->GetOriginalProfile()->GetPrefs();
+  if (prefs->GetBoolean(prefs::kUsedPolicyCertificatesOnce)) {

[pneubeck (no reviews), 2013/12/17 15:25:31]

is a UMA metric necessary, to decide when to remove this?

[Joao da Silva, 2013/12/17 16:36:41]

This is mostly used by schools, and IIRC they disable metrics reporting. We'll
have to sync with Vidya about this.

+    prefs->ClearPref(prefs::kUsedPolicyCertificatesOnce);
+    SetUsedPolicyCertificates(user->email());
+  }
+
   UserNetworkConfigurationUpdater* net_conf_updater =
       UserNetworkConfigurationUpdaterFactory::GetForProfile(profile);
   if (!net_conf_updater)
     return NULL;
 
-  // In case of usage of additional trust anchors from an incognito profile, the
-  // prefs of the original profile have to be marked.
-  return new PolicyCertService(net_conf_updater,
-                               profile->GetOriginalProfile()->GetPrefs());
+  return new PolicyCertService(net_conf_updater, user->email(), user_manager);
 }
 
 content::BrowserContext* PolicyCertServiceFactory::GetBrowserContextToUse(