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

chrome/browser/password_manager/sync_metrics.cc

 // Copyright 2014 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 "chrome/browser/password_manager/sync_metrics.h"
 
 #include "chrome/browser/signin/signin_manager_factory.h"
 #include "chrome/browser/sync/profile_sync_service.h"
 #include "chrome/browser/sync/profile_sync_service_factory.h"
 #include "components/signin/core/browser/signin_manager.h"
 #include "google_apis/gaia/gaia_auth_util.h"
 #include "google_apis/gaia/gaia_urls.h"
 
 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 "";
 
   return signin_manager->GetAuthenticatedUsername();
 }
 
 bool IsSyncAccountCredential(Profile* profile,
                              const std::string& username,
                              const std::string& origin) {
   if (origin != GaiaUrls::GetInstance()->gaia_url().GetOrigin().spec())
     return false;
 
   return gaia::AreEmailsSame(username, GetSyncUsername(profile));
 }
 
 }  // namespace password_manager_sync_metrics