[Password Manager] Setup experiment to restrict autofilling of sync credential

chrome/browser/password_manager/sync_metrics.cc

Index: chrome/browser/password_manager/sync_metrics.cc
diff --git a/chrome/browser/password_manager/sync_metrics.cc b/chrome/browser/password_manager/sync_metrics.cc
index 4eddf427ccd22db327b2fa421aaee66bd509056d..bf4bf95d14f00c9eb071a31422024f51b7206f32 100644
--- a/chrome/browser/password_manager/sync_metrics.cc
+++ b/chrome/browser/password_manager/sync_metrics.cc
@@ -14,8 +14,19 @@
 namespace password_manager_sync_metrics {
 
 std::string GetSyncUsername(Profile* profile) {
+  // If sync is setup, return early if we aren't syncing passwords.

[Ilya Sherman, 2014/08/12 02:17:33]

nit: "setup" -> "set up"

[Garrett Casto, 2014/08/13 20:34:41]

Done.

+  if (ProfileSyncServiceFactory::HasProfileSyncService(profile)) {
+    ProfileSyncService* sync_service =
+        ProfileSyncServiceFactory::GetForProfile(profile);
+    if (!sync_service ||

[Ilya Sherman, 2014/08/12 02:17:33]

Why might the sync_service be null, if HasProfileSyncService() returns true?

[Garrett Casto, 2014/08/13 20:34:40]

That's true. I just reverted the previous change to this file, I guess this
check is no longer necessary.

+        !sync_service->HasSyncSetupCompleted() ||

[Ilya Sherman, 2014/08/12 02:17:33]

Are we not syncing the sync password if sync setup has not yet completed?

[Garrett Casto, 2014/08/13 20:34:40]

The issue is that we can't tell if we are syncing passwords until sync setup is
complete (AFAIK), and we only really care about these users. The security and
privacy issues that people are worried about don't exist if you aren't syncing
passwords.

[Ilya Sherman, 2014/08/13 20:48:14]

I guess it just seems like it would be better to be conservative here, rather
than permissive, if we're worried about this as a security/privacy issue.  I bet
it's not too hard for an attacker to get sync into a state where it's in the
process of setting up.

[Garrett Casto, 2014/08/13 23:12:54]

From a privacy standpoint I don't think that it really matters as that's a
pretty high bar. I also was under the impression that the browser process was
safe, so if an attack could affect sync in such a way then all bets are off.
Changing this to disallow autofilling when you aren't syncing passwords is also
harder to message, as there isn't really a good reason for it, other than
possible technical constraints on our part.

Joel, do you have any thoughts on this?

[Ilya Sherman, 2014/08/14 07:38:14]

Joel, ^^^

[jww, 2014/08/14 21:50:20]

Whoops, sorry I missed it. I agree with Garrett. If the browser process is
compromised, we've got bigger fish to fry, so I'm not worried about that attack.
I *am* worried about people turning on sync later and having the sync credential
already there, but this is not the place to solve that. I think we're going to
work on that in the bigger picture of what to do about 'legacy' sync users (i.e.
users who've been syncing their sync credential for a while).

+        !sync_service->GetActiveDataTypes().Has(syncer::PASSWORDS))
+      return "";

[Ilya Sherman, 2014/08/12 02:17:33]

nit: Prefer std::string() to "".

[Garrett Casto, 2014/08/13 20:34:41]

Done.

+  }
+
   SigninManagerBase* signin_manager =
       SigninManagerFactory::GetForProfile(profile);
+
   if (!signin_manager)
     return "";