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

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

By default there is no change in behavior, but autofilling can now be disabled
for the sync credential entirely or just disabled for reauth pages that support
transactional reauth.

Note that this also changes GetSyncUsername() to not return the username if
password sync is disabled if it is possible to determine. This makes GetSyncUsername() a little inconsistent depending on the state of sync setup, but it's important to be as specific as possible when disabling autofilling, since it's a
usability hit.

BUG=386692
R=isherman@chromium.org

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=290030

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=290057

[Garrett Casto, 2014-08-11 09:14:29]

isherman: Please review
jww: FYI

[Ilya Sherman, 2014-08-12 02:17:34]

https://codereview.chromium.org/451853003/diff/100001/chrome/browser/password...
File chrome/browser/password_manager/chrome_password_manager_client.cc (right):

https://codereview.chromium.org/451853003/diff/100001/chrome/browser/password...
chrome/browser/password_manager/chrome_password_manager_client.cc:390: "rart",
What is "rart"?  Probably worth a comment, IMO.

https://codereview.chromium.org/451853003/diff/100001/chrome/browser/password...
chrome/browser/password_manager/chrome_password_manager_client.cc:437: }
Hmm, what about the other switch values?

https://codereview.chromium.org/451853003/diff/100001/chrome/browser/password...
chrome/browser/password_manager/chrome_password_manager_client.cc:446: }
I recall seeing a message on the finch-team mailing list indicating that using
finch params might be preferable to using group names.  However, I'm not
completely up to speed on the latest thinking here.  You might want to check
with one of the Finch crew.

https://codereview.chromium.org/451853003/diff/100001/chrome/browser/password...
File chrome/browser/password_manager/chrome_password_manager_client.h (right):

https://codereview.chromium.org/451853003/diff/100001/chrome/browser/password...
chrome/browser/password_manager/chrome_password_manager_client.h:107:
LastLoadWasTransactionalReauthPage);
Can you test the public API, rather than testing an internal method?

https://codereview.chromium.org/451853003/diff/100001/chrome/browser/password...
chrome/browser/password_manager/chrome_password_manager_client.h:141: void
SetupAutofillSyncState();
nit: "Setup" -> "SetUp".  "setup" is a noun; "set up" is a verb.

https://codereview.chromium.org/451853003/diff/100001/chrome/browser/password...
chrome/browser/password_manager/chrome_password_manager_client.h:160:
AutofillForSyncCredentialsState autofill_state_;
nit: Please document this variable.  Also, "autofill_state_" is a very generic
name -- I'd definitely mention "sync" somewhere in the variable name.

https://codereview.chromium.org/451853003/diff/100001/chrome/browser/password...
File chrome/browser/password_manager/chrome_password_manager_client_unittest.cc
(right):

https://codereview.chromium.org/451853003/diff/100001/chrome/browser/password...
chrome/browser/password_manager/chrome_password_manager_client_unittest.cc:205:
NavigateAndCommit(GURL("https://site.com/login?rart=678"));
What about rart with no value, e.g. "https://site.com/login?rart"?

https://codereview.chromium.org/451853003/diff/100001/chrome/browser/password...
File chrome/browser/password_manager/sync_metrics.cc (right):

https://codereview.chromium.org/451853003/diff/100001/chrome/browser/password...
chrome/browser/password_manager/sync_metrics.cc:17: // If sync is setup, return
early if we aren't syncing passwords.
nit: "setup" -> "set up"

https://codereview.chromium.org/451853003/diff/100001/chrome/browser/password...
chrome/browser/password_manager/sync_metrics.cc:21: if (!sync_service ||
Why might the sync_service be null, if HasProfileSyncService() returns true?

https://codereview.chromium.org/451853003/diff/100001/chrome/browser/password...
chrome/browser/password_manager/sync_metrics.cc:22:
!sync_service->HasSyncSetupCompleted() ||
Are we not syncing the sync password if sync setup has not yet completed?

https://codereview.chromium.org/451853003/diff/100001/chrome/browser/password...
chrome/browser/password_manager/sync_metrics.cc:24: return "";
nit: Prefer std::string() to "".

https://codereview.chromium.org/451853003/diff/100001/components/password_man...
File components/password_manager/core/browser/password_form_manager.h (right):

https://codereview.chromium.org/451853003/diff/100001/components/password_man...
components/password_manager/core/browser/password_form_manager.h:222: // to true
iff a result is dropped because the the |client_| requested that
nit: "the the" -> "the"

https://codereview.chromium.org/451853003/diff/100001/components/password_man...
components/password_manager/core/browser/password_form_manager.h:223: // it be
filtered (e.g. the form matches the users sync credentials).
nit: "users" -> "user's"

https://codereview.chromium.org/451853003/diff/100001/components/password_man...
components/password_manager/core/browser/password_form_manager.h:224: bool
IgnoreResult(const autofill::PasswordForm& form,
nit: As long as you're here: Might it be clearer to rename this method to
something like "ShouldIgnoreResult"?

https://codereview.chromium.org/451853003/diff/100001/tools/metrics/histogram...
File tools/metrics/histograms/histograms.xml (right):

https://codereview.chromium.org/451853003/diff/100001/tools/metrics/histogram...
tools/metrics/histograms/histograms.xml:20099: +    If autofill results were not
shown, e.g. the sync credential was removed.
There are other reasons that IgnoreResult() might return |true|, resulting in
autofill results not being shown -- right?  Perhaps it makes sense to have a
more focused histogram, to measure specifically the case that the sync
credential was removed?

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

https://codereview.chromium.org/451853003/diff/100001/chrome/browser/password...
File chrome/browser/password_manager/chrome_password_manager_client.cc (right):

https://codereview.chromium.org/451853003/diff/100001/chrome/browser/password...
chrome/browser/password_manager/chrome_password_manager_client.cc:390: "rart",
On 2014/08/12 02:17:32, Ilya Sherman wrote:
> What is "rart"?  Probably worth a comment, IMO.

Done.

https://codereview.chromium.org/451853003/diff/100001/chrome/browser/password...
chrome/browser/password_manager/chrome_password_manager_client.cc:437: }
On 2014/08/12 02:17:32, Ilya Sherman wrote:
> Hmm, what about the other switch values?

Fixed, with tests to verify.

https://codereview.chromium.org/451853003/diff/100001/chrome/browser/password...
chrome/browser/password_manager/chrome_password_manager_client.cc:446: }
On 2014/08/12 02:17:32, Ilya Sherman wrote:
> I recall seeing a message on the finch-team mailing list indicating that using
> finch params might be preferable to using group names.  However, I'm not
> completely up to speed on the latest thinking here.  You might want to check
> with one of the Finch crew.

Sent an e-mail out asking. It doesn't seem like it's useful to me, but I could
be wrong.

https://codereview.chromium.org/451853003/diff/100001/chrome/browser/password...
File chrome/browser/password_manager/chrome_password_manager_client.h (right):

https://codereview.chromium.org/451853003/diff/100001/chrome/browser/password...
chrome/browser/password_manager/chrome_password_manager_client.h:107:
LastLoadWasTransactionalReauthPage);
On 2014/08/12 02:17:32, Ilya Sherman wrote:
> Can you test the public API, rather than testing an internal method?

Done.

https://codereview.chromium.org/451853003/diff/100001/chrome/browser/password...
chrome/browser/password_manager/chrome_password_manager_client.h:141: void
SetupAutofillSyncState();
On 2014/08/12 02:17:32, Ilya Sherman wrote:
> nit: "Setup" -> "SetUp".  "setup" is a noun; "set up" is a verb.

Done.

https://codereview.chromium.org/451853003/diff/100001/chrome/browser/password...
chrome/browser/password_manager/chrome_password_manager_client.h:160:
AutofillForSyncCredentialsState autofill_state_;
On 2014/08/12 02:17:33, Ilya Sherman wrote:
> nit: Please document this variable.  Also, "autofill_state_" is a very generic
> name -- I'd definitely mention "sync" somewhere in the variable name.

Done.

https://codereview.chromium.org/451853003/diff/100001/chrome/browser/password...
File chrome/browser/password_manager/chrome_password_manager_client_unittest.cc
(right):

https://codereview.chromium.org/451853003/diff/100001/chrome/browser/password...
chrome/browser/password_manager/chrome_password_manager_client_unittest.cc:205:
NavigateAndCommit(GURL("https://site.com/login?rart=678"));
On 2014/08/12 02:17:33, Ilya Sherman wrote:
> What about rart with no value, e.g. "https://site.com/login?rart"?

It works right now. I don't think that you will get a valid URL that looks like
that so I'm not sure it matters either way. Added it to the test suite with a
comment.

https://codereview.chromium.org/451853003/diff/100001/chrome/browser/password...
File chrome/browser/password_manager/sync_metrics.cc (right):

https://codereview.chromium.org/451853003/diff/100001/chrome/browser/password...
chrome/browser/password_manager/sync_metrics.cc:17: // If sync is setup, return
early if we aren't syncing passwords.
On 2014/08/12 02:17:33, Ilya Sherman wrote:
> nit: "setup" -> "set up"

Done.

https://codereview.chromium.org/451853003/diff/100001/chrome/browser/password...
chrome/browser/password_manager/sync_metrics.cc:21: if (!sync_service ||
On 2014/08/12 02:17:33, Ilya Sherman wrote:
> Why might the sync_service be null, if HasProfileSyncService() returns true?

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

https://codereview.chromium.org/451853003/diff/100001/chrome/browser/password...
chrome/browser/password_manager/sync_metrics.cc:22:
!sync_service->HasSyncSetupCompleted() ||
On 2014/08/12 02:17:33, Ilya Sherman wrote:
> Are we not syncing the sync password if sync setup has not yet completed?

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.

https://codereview.chromium.org/451853003/diff/100001/chrome/browser/password...
chrome/browser/password_manager/sync_metrics.cc:24: return "";
On 2014/08/12 02:17:33, Ilya Sherman wrote:
> nit: Prefer std::string() to "".

Done.

https://codereview.chromium.org/451853003/diff/100001/components/password_man...
File components/password_manager/core/browser/password_form_manager.h (right):

https://codereview.chromium.org/451853003/diff/100001/components/password_man...
components/password_manager/core/browser/password_form_manager.h:222: // to true
iff a result is dropped because the the |client_| requested that
On 2014/08/12 02:17:33, Ilya Sherman wrote:
> nit: "the the" -> "the"

Done.

https://codereview.chromium.org/451853003/diff/100001/components/password_man...
components/password_manager/core/browser/password_form_manager.h:223: // it be
filtered (e.g. the form matches the users sync credentials).
On 2014/08/12 02:17:33, Ilya Sherman wrote:
> nit: "users" -> "user's"

Done.

https://codereview.chromium.org/451853003/diff/100001/components/password_man...
components/password_manager/core/browser/password_form_manager.h:224: bool
IgnoreResult(const autofill::PasswordForm& form,
On 2014/08/12 02:17:33, Ilya Sherman wrote:
> nit: As long as you're here: Might it be clearer to rename this method to
> something like "ShouldIgnoreResult"?

Done.

https://codereview.chromium.org/451853003/diff/100001/tools/metrics/histogram...
File tools/metrics/histograms/histograms.xml (right):

https://codereview.chromium.org/451853003/diff/100001/tools/metrics/histogram...
tools/metrics/histograms/histograms.xml:20099: +    If autofill results were not
shown, e.g. the sync credential was removed.
On 2014/08/12 02:17:34, Ilya Sherman wrote:
> There are other reasons that IgnoreResult() might return |true|, resulting in
> autofill results not being shown -- right?  Perhaps it makes sense to have a
> more focused histogram, to measure specifically the case that the sync
> credential was removed?

There are other reasons that IgnoreResult might return true (e.g. bad SSL cert),
but this histogram will only be get a yes if it's because of the client
requesting it be blocked. At the moment this is only due to sync credential
issues. 

I was a little torn about this. I first wanted to put the histogram in the
client, but if you do that then you will get one report per filtered credential
instead of one per Autofill experience (which is really what matters). I could
have also made the API more specific about why it's filtering, but I wanted to
leave open the option of changing what exactly we filter without changing the
function defintion. Does this seem reasonable to you?

[Ilya Sherman, 2014-08-13 20:48:15]

https://codereview.chromium.org/451853003/diff/100001/chrome/browser/password...
File chrome/browser/password_manager/sync_metrics.cc (right):

https://codereview.chromium.org/451853003/diff/100001/chrome/browser/password...
chrome/browser/password_manager/sync_metrics.cc:22:
!sync_service->HasSyncSetupCompleted() ||
On 2014/08/13 20:34:40, Garrett Casto wrote:
> On 2014/08/12 02:17:33, Ilya Sherman wrote:
> > Are we not syncing the sync password if sync setup has not yet completed?
> 
> 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.

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.

https://codereview.chromium.org/451853003/diff/100001/tools/metrics/histogram...
File tools/metrics/histograms/histograms.xml (right):

https://codereview.chromium.org/451853003/diff/100001/tools/metrics/histogram...
tools/metrics/histograms/histograms.xml:20099: +    If autofill results were not
shown, e.g. the sync credential was removed.
On 2014/08/13 20:34:41, Garrett Casto wrote:
> On 2014/08/12 02:17:34, Ilya Sherman wrote:
> > There are other reasons that IgnoreResult() might return |true|, resulting
in
> > autofill results not being shown -- right?  Perhaps it makes sense to have a
> > more focused histogram, to measure specifically the case that the sync
> > credential was removed?
> 
> There are other reasons that IgnoreResult might return true (e.g. bad SSL
cert),
> but this histogram will only be get a yes if it's because of the client
> requesting it be blocked. At the moment this is only due to sync credential
> issues. 
> 
> I was a little torn about this. I first wanted to put the histogram in the
> client, but if you do that then you will get one report per filtered
credential
> instead of one per Autofill experience (which is really what matters). I could
> have also made the API more specific about why it's filtering, but I wanted to
> leave open the option of changing what exactly we filter without changing the
> function defintion. Does this seem reasonable to you?

We could have an additional method on the client for reporting metrics.  I'm
worried that if we leave the code as is, it can easily evolve to start
conflating multiple different events into this one histogram, without anyone
ever noticing the problem.

https://codereview.chromium.org/451853003/diff/140001/chrome/browser/password...
File chrome/browser/password_manager/chrome_password_manager_client.cc (right):

https://codereview.chromium.org/451853003/diff/140001/chrome/browser/password...
chrome/browser/password_manager/chrome_password_manager_client.cc:47: namespace
pm_switches = password_manager::switches;
nit: Please don't use acronyms or abbreviations when naming code.  Either use
the full namespace name, or just alias it 'switches' or something.

https://codereview.chromium.org/451853003/diff/140001/chrome/browser/password...
File chrome/browser/password_manager/chrome_password_manager_client_unittest.cc
(right):

https://codereview.chromium.org/451853003/diff/140001/chrome/browser/password...
chrome/browser/password_manager/chrome_password_manager_client_unittest.cc:43:
const std::string& username, const std::string& origin) const OVERRIDE {
nit: One param per line, please.

https://codereview.chromium.org/451853003/diff/140001/chrome/browser/password...
chrome/browser/password_manager/chrome_password_manager_client_unittest.cc:47:
void SetIsSyncAccountCredential(bool is_sync_account_credential) {
nit: hacker_case, please.

https://codereview.chromium.org/451853003/diff/140001/chrome/browser/password...
chrome/browser/password_manager/chrome_password_manager_client_unittest.cc:250:
// Normally the client should allow any credentials thourgh, even if they
nit: "thourgh" -> "through"

https://codereview.chromium.org/451853003/diff/140001/chrome/browser/password...
chrome/browser/password_manager/chrome_password_manager_client_unittest.cc:265:
EXPECT_TRUE(client->ShouldFilterAutofillResult(form));
Optional nit: WDYT of "ShouldDrop" rather than "ShouldFilter"?

https://codereview.chromium.org/451853003/diff/140001/components/password_man...
File components/password_manager/core/browser/password_form_manager.cc (right):

https://codereview.chromium.org/451853003/diff/140001/components/password_man...
components/password_manager/core/browser/password_form_manager.cc:515: const
PasswordForm& form, bool* autofill_result_filtered) const {
nit: One arg per line, please.

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

https://codereview.chromium.org/451853003/diff/100001/chrome/browser/password...
File chrome/browser/password_manager/sync_metrics.cc (right):

https://codereview.chromium.org/451853003/diff/100001/chrome/browser/password...
chrome/browser/password_manager/sync_metrics.cc:22:
!sync_service->HasSyncSetupCompleted() ||
On 2014/08/13 20:48:14, Ilya Sherman wrote:
> On 2014/08/13 20:34:40, Garrett Casto wrote:
> > On 2014/08/12 02:17:33, Ilya Sherman wrote:
> > > Are we not syncing the sync password if sync setup has not yet completed?
> > 
> > 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.
> 
> 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.

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?

https://codereview.chromium.org/451853003/diff/100001/tools/metrics/histogram...
File tools/metrics/histograms/histograms.xml (right):

https://codereview.chromium.org/451853003/diff/100001/tools/metrics/histogram...
tools/metrics/histograms/histograms.xml:20099: +    If autofill results were not
shown, e.g. the sync credential was removed.
On 2014/08/13 20:48:14, Ilya Sherman wrote:
> On 2014/08/13 20:34:41, Garrett Casto wrote:
> > On 2014/08/12 02:17:34, Ilya Sherman wrote:
> > > There are other reasons that IgnoreResult() might return |true|, resulting
> in
> > > autofill results not being shown -- right?  Perhaps it makes sense to have
a
> > > more focused histogram, to measure specifically the case that the sync
> > > credential was removed?
> > 
> > There are other reasons that IgnoreResult might return true (e.g. bad SSL
> cert),
> > but this histogram will only be get a yes if it's because of the client
> > requesting it be blocked. At the moment this is only due to sync credential
> > issues. 
> > 
> > I was a little torn about this. I first wanted to put the histogram in the
> > client, but if you do that then you will get one report per filtered
> credential
> > instead of one per Autofill experience (which is really what matters). I
could
> > have also made the API more specific about why it's filtering, but I wanted
to
> > leave open the option of changing what exactly we filter without changing
the
> > function defintion. Does this seem reasonable to you?
> 
> We could have an additional method on the client for reporting metrics.  I'm
> worried that if we leave the code as is, it can easily evolve to start
> conflating multiple different events into this one histogram, without anyone
> ever noticing the problem.

Added a reporting mechanism.

https://codereview.chromium.org/451853003/diff/140001/chrome/browser/password...
File chrome/browser/password_manager/chrome_password_manager_client.cc (right):

https://codereview.chromium.org/451853003/diff/140001/chrome/browser/password...
chrome/browser/password_manager/chrome_password_manager_client.cc:47: namespace
pm_switches = password_manager::switches;
On 2014/08/13 20:48:14, Ilya Sherman wrote:
> nit: Please don't use acronyms or abbreviations when naming code.  Either use
> the full namespace name, or just alias it 'switches' or something.

Changed to switches. I don't normally do this, but in this case it made fitting
under 80 chars quite difficult.

https://codereview.chromium.org/451853003/diff/140001/chrome/browser/password...
File chrome/browser/password_manager/chrome_password_manager_client_unittest.cc
(right):

https://codereview.chromium.org/451853003/diff/140001/chrome/browser/password...
chrome/browser/password_manager/chrome_password_manager_client_unittest.cc:43:
const std::string& username, const std::string& origin) const OVERRIDE {
On 2014/08/13 20:48:14, Ilya Sherman wrote:
> nit: One param per line, please.

Done.

https://codereview.chromium.org/451853003/diff/140001/chrome/browser/password...
chrome/browser/password_manager/chrome_password_manager_client_unittest.cc:47:
void SetIsSyncAccountCredential(bool is_sync_account_credential) {
On 2014/08/13 20:48:14, Ilya Sherman wrote:
> nit: hacker_case, please.

Done.

https://codereview.chromium.org/451853003/diff/140001/chrome/browser/password...
chrome/browser/password_manager/chrome_password_manager_client_unittest.cc:250:
// Normally the client should allow any credentials thourgh, even if they
On 2014/08/13 20:48:14, Ilya Sherman wrote:
> nit: "thourgh" -> "through"

Done.

https://codereview.chromium.org/451853003/diff/140001/chrome/browser/password...
chrome/browser/password_manager/chrome_password_manager_client_unittest.cc:265:
EXPECT_TRUE(client->ShouldFilterAutofillResult(form));
On 2014/08/13 20:48:14, Ilya Sherman wrote:
> Optional nit: WDYT of "ShouldDrop" rather than "ShouldFilter"?

I'm indifferent. If you prefer I can change it, but I don't know if either is
particularly clearer.

https://codereview.chromium.org/451853003/diff/140001/components/password_man...
File components/password_manager/core/browser/password_form_manager.cc (right):

https://codereview.chromium.org/451853003/diff/140001/components/password_man...
components/password_manager/core/browser/password_form_manager.cc:515: const
PasswordForm& form, bool* autofill_result_filtered) const {
On 2014/08/13 20:48:15, Ilya Sherman wrote:
> nit: One arg per line, please.

Done.

[jww, 2014-08-14 01:16:25]

https://codereview.chromium.org/451853003/diff/100001/chrome/browser/password...
File chrome/browser/password_manager/chrome_password_manager_client.cc (right):

https://codereview.chromium.org/451853003/diff/100001/chrome/browser/password...
chrome/browser/password_manager/chrome_password_manager_client.cc:446: }
On 2014/08/13 20:34:40, Garrett Casto wrote:
> On 2014/08/12 02:17:32, Ilya Sherman wrote:
> > I recall seeing a message on the finch-team mailing list indicating that
using
> > finch params might be preferable to using group names.  However, I'm not
> > completely up to speed on the latest thinking here.  You might want to check
> > with one of the Finch crew.
> 
> Sent an e-mail out asking. It doesn't seem like it's useful to me, but I could
> be wrong.

The explanation I've gotten in the past is that if you have continuous values,
rather than a clearly defined set of binary choices, parameters give you the
advantage of changing the experiment on the fly. Instead of checking the group
name, you'd check your parameter and use its value and completely ignore the
group name. Then, if you want to change the experiment later, you can just add a
new group with a new parameter. See a recent experiment that I committed for an
example: https://codereview.chromium.org/369703002/

In this case, it seems like there's only a small set of binary choices with no
chance of wanting/needing to change that later, so I suspect this is the right
choice.

[Ilya Sherman, 2014-08-14 07:38:15]

https://codereview.chromium.org/451853003/diff/100001/chrome/browser/password...
File chrome/browser/password_manager/sync_metrics.cc (right):

https://codereview.chromium.org/451853003/diff/100001/chrome/browser/password...
chrome/browser/password_manager/sync_metrics.cc:22:
!sync_service->HasSyncSetupCompleted() ||
On 2014/08/13 23:12:54, Garrett Casto wrote:
> On 2014/08/13 20:48:14, Ilya Sherman wrote:
> > On 2014/08/13 20:34:40, Garrett Casto wrote:
> > > On 2014/08/12 02:17:33, Ilya Sherman wrote:
> > > > Are we not syncing the sync password if sync setup has not yet
completed?
> > > 
> > > 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.
> > 
> > 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.
> 
> 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?

Joel, ^^^

https://codereview.chromium.org/451853003/diff/180001/chrome/browser/password...
File chrome/browser/password_manager/chrome_password_manager_client.cc (right):

https://codereview.chromium.org/451853003/diff/180001/chrome/browser/password...
chrome/browser/password_manager/chrome_password_manager_client.cc:117:
sync_credential_filtered_ = true;
You should initialize this variable in the constructor.

https://codereview.chromium.org/451853003/diff/180001/chrome/browser/password...
File chrome/browser/password_manager/chrome_password_manager_client.h (right):

https://codereview.chromium.org/451853003/diff/180001/chrome/browser/password...
chrome/browser/password_manager/chrome_password_manager_client.h:166: bool
sync_credential_filtered_;
Optional nit: Perhaps "filtered_" -> "was_filtered_"?

https://codereview.chromium.org/451853003/diff/180001/chrome/browser/password...
File chrome/browser/password_manager/chrome_password_manager_client_unittest.cc
(right):

https://codereview.chromium.org/451853003/diff/180001/chrome/browser/password...
chrome/browser/password_manager/chrome_password_manager_client_unittest.cc:260:
//
nit: ^^^

https://codereview.chromium.org/451853003/diff/180001/components/password_man...
File components/password_manager/core/browser/password_form_manager.cc (right):

https://codereview.chromium.org/451853003/diff/180001/components/password_man...
components/password_manager/core/browser/password_form_manager.cc:378: bool
autofill_result_filtered = false;
I was thinking that you'd pass this in to the new method on the client_... but
if you prefer to cache state within the client_, then please remove this
variable and remove the additional param from ShouldIgnoreResult.

https://codereview.chromium.org/451853003/diff/180001/components/password_man...
File components/password_manager/core/browser/password_manager_client.h (right):

https://codereview.chromium.org/451853003/diff/180001/components/password_man...
components/password_manager/core/browser/password_manager_client.h:49: virtual
void AutofillResultsComputed() {};
nit: No need for the semicolon.

[Garrett Casto, 2014-08-14 19:48:43]

https://codereview.chromium.org/451853003/diff/180001/chrome/browser/password...
File chrome/browser/password_manager/chrome_password_manager_client.cc (right):

https://codereview.chromium.org/451853003/diff/180001/chrome/browser/password...
chrome/browser/password_manager/chrome_password_manager_client.cc:117:
sync_credential_filtered_ = true;
On 2014/08/14 07:38:14, Ilya Sherman wrote:
> You should initialize this variable in the constructor.

Done.

https://codereview.chromium.org/451853003/diff/180001/chrome/browser/password...
File chrome/browser/password_manager/chrome_password_manager_client.h (right):

https://codereview.chromium.org/451853003/diff/180001/chrome/browser/password...
chrome/browser/password_manager/chrome_password_manager_client.h:166: bool
sync_credential_filtered_;
On 2014/08/14 07:38:14, Ilya Sherman wrote:
> Optional nit: Perhaps "filtered_" -> "was_filtered_"?

Done.

https://codereview.chromium.org/451853003/diff/180001/chrome/browser/password...
File chrome/browser/password_manager/chrome_password_manager_client_unittest.cc
(right):

https://codereview.chromium.org/451853003/diff/180001/chrome/browser/password...
chrome/browser/password_manager/chrome_password_manager_client_unittest.cc:260:
//
On 2014/08/14 07:38:14, Ilya Sherman wrote:
> nit: ^^^

Filled out the comment I meant to leave.

https://codereview.chromium.org/451853003/diff/180001/components/password_man...
File components/password_manager/core/browser/password_form_manager.cc (right):

https://codereview.chromium.org/451853003/diff/180001/components/password_man...
components/password_manager/core/browser/password_form_manager.cc:378: bool
autofill_result_filtered = false;
On 2014/08/14 07:38:14, Ilya Sherman wrote:
> I was thinking that you'd pass this in to the new method on the client_... but
> if you prefer to cache state within the client_, then please remove this
> variable and remove the additional param from ShouldIgnoreResult.

No, just missed. Removed.

https://codereview.chromium.org/451853003/diff/180001/components/password_man...
File components/password_manager/core/browser/password_manager_client.h (right):

https://codereview.chromium.org/451853003/diff/180001/components/password_man...
components/password_manager/core/browser/password_manager_client.h:49: virtual
void AutofillResultsComputed() {};
On 2014/08/14 07:38:14, Ilya Sherman wrote:
> nit: No need for the semicolon.

Done.

[Ilya Sherman, 2014-08-14 21:13:06]

LGTM % the question for Joel and a final nit.  Thanks!

https://codereview.chromium.org/451853003/diff/200001/components/password_man...
File components/password_manager/core/browser/password_form_manager.cc (right):

https://codereview.chromium.org/451853003/diff/200001/components/password_man...
components/password_manager/core/browser/password_form_manager.cc:513: const
PasswordForm& form) const {
nit: Please unwrap.

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

https://codereview.chromium.org/451853003/diff/100001/chrome/browser/password...
File chrome/browser/password_manager/sync_metrics.cc (right):

https://codereview.chromium.org/451853003/diff/100001/chrome/browser/password...
chrome/browser/password_manager/sync_metrics.cc:22:
!sync_service->HasSyncSetupCompleted() ||
On 2014/08/14 07:38:14, Ilya Sherman wrote:
> On 2014/08/13 23:12:54, Garrett Casto wrote:
> > On 2014/08/13 20:48:14, Ilya Sherman wrote:
> > > On 2014/08/13 20:34:40, Garrett Casto wrote:
> > > > On 2014/08/12 02:17:33, Ilya Sherman wrote:
> > > > > Are we not syncing the sync password if sync setup has not yet
> completed?
> > > > 
> > > > 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.
> > > 
> > > 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.
> > 
> > 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?
> 
> Joel, ^^^

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).

[Garrett Casto, 2014-08-15 15:32:57]

https://codereview.chromium.org/451853003/diff/200001/components/password_man...
File components/password_manager/core/browser/password_form_manager.cc (right):

https://codereview.chromium.org/451853003/diff/200001/components/password_man...
components/password_manager/core/browser/password_form_manager.cc:513: const
PasswordForm& form) const {
On 2014/08/14 21:13:06, Ilya Sherman wrote:
> nit: Please unwrap.

Done.

[Garrett Casto, 2014-08-15 15:34:15]

The CQ bit was checked by gcasto@chromium.org

[commit-bot: I haz the power, 2014-08-15 15:36:02]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/gcasto@chromium.org/451853003/220001

[commit-bot: I haz the power, 2014-08-15 15:49:48]

FYI, CQ is re-trying this CL (attempt #1).
The failing builders are:
  linux_gpu on tryserver.chromium.gpu
(http://build.chromium.org/p/tryserver.chromium.gpu/builders/linux_gpu/builds/...)
  mac_gpu on tryserver.chromium.gpu
(http://build.chromium.org/p/tryserver.chromium.gpu/builders/mac_gpu/builds/42317)
  win_gpu on tryserver.chromium.gpu
(http://build.chromium.org/p/tryserver.chromium.gpu/builders/win_gpu/builds/47519)
  android_aosp on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/android_aosp/bu...)
  android_chromium_gn_compile_rel on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/android_chromiu...)
  android_clang_dbg on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/android_clang_d...)
  android_dbg on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/android_dbg/bui...)
  chromium_presubmit on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
  linux_chromium_chromeos_rel_swarming on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_gn_rel on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_rel_swarming on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  ios_dbg_simulator on tryserver.chromium.mac
(http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_dbg_simulator...)
  ios_rel_device on tryserver.chromium.mac
(http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_rel_device/bu...)
  ios_rel_device_ninja on tryserver.chromium.mac
(http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_rel_device_ni...)
  mac_chromium_compile_dbg on tryserver.chromium.mac
(http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
  mac_chromium_rel_swarming on tryserver.chromium.mac
(http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
  win8_chromium_rel on tryserver.chromium.win
(http://build.chromium.org/p/tryserver.chromium.win/builders/win8_chromium_rel...)
  win_chromium_compile_dbg on tryserver.chromium.win
(http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_comp...)
  win_chromium_rel_swarming on tryserver.chromium.win
(http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)
  win_chromium_x64_rel on tryserver.chromium.win
(http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)

[commit-bot: I haz the power, 2014-08-15 15:54:23]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-08-15 15:54:24]

Try jobs failed on following builders:
  mac_gpu on tryserver.chromium.gpu
(http://build.chromium.org/p/tryserver.chromium.gpu/builders/mac_gpu/builds/42321)
  win_gpu on tryserver.chromium.gpu
(http://build.chromium.org/p/tryserver.chromium.gpu/builders/win_gpu/builds/47524)
  ios_dbg_simulator on tryserver.chromium.mac
(http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_dbg_simulator...)
  ios_rel_device on tryserver.chromium.mac
(http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_rel_device/bu...)
  ios_rel_device_ninja on tryserver.chromium.mac
(http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_rel_device_ni...)
  mac_chromium_compile_dbg on tryserver.chromium.mac
(http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
  mac_chromium_rel_swarming on tryserver.chromium.mac
(http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
  win8_chromium_rel on tryserver.chromium.win
(http://build.chromium.org/p/tryserver.chromium.win/builders/win8_chromium_rel...)
  win_chromium_x64_rel on tryserver.chromium.win
(http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)

[Garrett Casto, 2014-08-15 15:59:45]

The CQ bit was checked by gcasto@chromium.org

[commit-bot: I haz the power, 2014-08-15 16:03:35]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/gcasto@chromium.org/451853003/240001

[commit-bot: I haz the power, 2014-08-15 19:00:33]

FYI, CQ is re-trying this CL (attempt #1).
The failing builders are:
  mac_gpu on tryserver.chromium.gpu
(http://build.chromium.org/p/tryserver.chromium.gpu/builders/mac_gpu/builds/42401)

[commit-bot: I haz the power, 2014-08-15 19:06:51]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-08-15 19:06:52]

Try jobs failed on following builders:
  mac_gpu on tryserver.chromium.gpu
(http://build.chromium.org/p/tryserver.chromium.gpu/builders/mac_gpu/builds/42456)

[Garrett Casto, 2014-08-15 22:11:19]

On 2014/08/15 19:06:52, I haz the power (commit-bot) wrote:
> Try jobs failed on following builders:
>   mac_gpu on tryserver.chromium.gpu
>
(http://build.chromium.org/p/tryserver.chromium.gpu/builders/mac_gpu/builds/42456)

Per our out of band discussion, updated to not block on IsSyncSetupComplete().
Submitting.

[Garrett Casto, 2014-08-15 22:26:12]

Committed patchset #12 manually as 290030 (presubmit successful).

[Garrett Casto, 2014-08-15 23:54:40]

Relanding. First time a rebase added a new histograms test which I was failing.

[Garrett Casto, 2014-08-16 00:00:50]

Committed patchset #13 manually as 290057 (presubmit successful).