PasswordStoreMac::RemoveLoginsCreatedBetween() shouldn't affect other profiles.

PasswordStoreMac::RemoveLoginsCreatedBetween() shouldn't affect other profiles.

Previous strategy: remove all the Chrome passwords not referenced by the local database.
New strategy: remove exactly those passwords which were removed from the database.

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

[vasilii, 2014-07-01 17:46:39]

Hi guys,

please review the CL.

[stuartmorgan, 2014-07-01 23:37:41]

> The removed comment suggests that they may include the passwords referenced by
other DB entries.
> But this is an improvement from the privacy perspective.

I don't follow this argument. If I save two passwords, one a month ago and one
today, and both are on the same domain and have the same username, etc., such
that they share the same keychain entry, and then I say "delete all passwords
from the last 24 hours", how is also deleting the password that I stored a month
ago a privacy improvement?

[vabr (Chromium), 2014-07-02 07:41:50]

Vasilii, I'm also confused. First, you write:
"we'll remove exactly those passwords, which we removed from the database"
but then
"[the deleted keychain passwords] may include the passwords referenced by other
DB entries".
So do we end up in a state when we have metadata in the Login DB without the
actual passwords in the Keychain?

I'm happy to talk to you in the office to clarify.

Cheers,
Vaclav

https://codereview.chromium.org/360343002/diff/1/chrome/browser/password_mana...
File chrome/browser/password_manager/password_store_mac_unittest.cc (right):

https://codereview.chromium.org/360343002/diff/1/chrome/browser/password_mana...
chrome/browser/password_manager/password_store_mac_unittest.cc:1083: void
WaitForStoreUpdate(scoped_refptr<TestPasswordStoreMac> store) {
It looks like you always call this with |store_|, so maybe revert back to a
single WaitForStoreUpdate()?

[vasilii, 2014-07-02 12:34:03]

The situation where we have 2 database entries referring to the same keychain
entry isn't typical. According to the comment in
PasswordStoreMac::RemoveLoginImpl it's possible if two forms differ only in
element names. The database uses the tuple [origin_url, username_element,
username_value, password_element, submit_element, signon_realm] as a primary
key. The keychain uses only [scheme, origin_url, username_value, signon_realm].
Anyway, if you save a password and then "delete all passwords
from the last 24 hours", you expect that this password is gone. As a user you
don't care if the corresponding Keychain item is referenced by some older
database entry.

https://codereview.chromium.org/360343002/diff/1/chrome/browser/password_mana...
File chrome/browser/password_manager/password_store_mac_unittest.cc (right):

https://codereview.chromium.org/360343002/diff/1/chrome/browser/password_mana...
chrome/browser/password_manager/password_store_mac_unittest.cc:1083: void
WaitForStoreUpdate(scoped_refptr<TestPasswordStoreMac> store) {
On 2014/07/02 07:41:49, vabr (Chromium) wrote:
> It looks like you always call this with |store_|, so maybe revert back to a
> single WaitForStoreUpdate()?

Done.

[vabr (Chromium), 2014-07-02 13:15:55]

Thanks, Vasilii. LGTM.

I agree with you that if the user saved a password in the last, say 24 hours,
and then requires deleting passwords from the last 24 hours, that password must
be deleted. If there are multiple login DB entries for that, then:
(1) either the password corresponding to the older DB entry was different and
thus got overwritten by the new one anyway, or
(2) the password corresponding to the older DB entry was the same, which the
user requested to be deleted, and thus should be gone

I also agree with what you told me offline: it looks like a bug, that Chrome
would actually create two DB entries which match the same keychain entry --
those entries differ only in element names, which is probably due to website
updates, and the new entry should actually replace the old one. Maybe you could
file a bug for that.
(There is always the hypothetical case of the same website actually offering the
multiple sign-up scopes, which use the same signon realm, and the user uses the
same username but a different password. But that already does not work -- the
older password is always removed -- and seems crazy enough not to be supported.)

Finally, I checked that dangling database entries without corresponding keychain
items are purged in PasswordStoreMac::GetLoginsImpl, so we are not gathering
invisible trash.

Thanks for the educational experience in password storing on Mac! :)
Vaclav

https://codereview.chromium.org/360343002/diff/20001/chrome/browser/password_...
File chrome/browser/password_manager/password_store_mac.cc (right):

https://codereview.chromium.org/360343002/diff/20001/chrome/browser/password_...
chrome/browser/password_manager/password_store_mac.cc:963:
ScopedVector<PasswordForm> forms;
It's great that you caught the leak.
Looking at the code of LoginDatabase::GetLogins* and similar methods in
LoginDatabase which return a vector of PasswordForm*, they all pass the
ownership to the caller.
Could you please add comment to the declarations of those methods to make that
clear, so that people are less likely to reintroduce the same error again?

https://codereview.chromium.org/360343002/diff/20001/chrome/browser/password_...
File chrome/browser/password_manager/password_store_mac_unittest.cc (right):

https://codereview.chromium.org/360343002/diff/20001/chrome/browser/password_...
chrome/browser/password_manager/password_store_mac_unittest.cc:1303:
matching_items.get() = owned_keychain_adapter.PasswordsFillingForm(
optional nit: Consider doing these 2 checks (or at least the one for facebook)
also before you call RemoveLogins*, so that if AddLogin fails, the test does not
accidentally pass. (Naturally, the expected size would be 1u in both cases.)

https://codereview.chromium.org/360343002/diff/20001/chrome/browser/password_...
chrome/browser/password_manager/password_store_mac_unittest.cc:1340: // Add a
password from another profile.
optional nit: Indicate that "from another profile" means just that instead of
calling PasswordStore::AddLogin on the credentials, you only add them through
the keychain adapter. Ideally also how that's different from the 3rd party case
above.

[vasilii, 2014-07-02 14:02:58]

It's possible in theory to create 2 database items sharing one password using
PasswordStoreMac public interface. However, I didn't reproduce it in practice.
Thus, I don't think there is a bug.
The only practical workflow I'm aware of is crbug/344337. We can import
passwords on Windows and all the element's fields will be empty. On the first
successful login we update the form with actual element's values. In
https://codereview.chromium.org/299443002 I made sure that the previous form is
removed.

https://codereview.chromium.org/360343002/diff/20001/chrome/browser/password_...
File chrome/browser/password_manager/password_store_mac.cc (right):

https://codereview.chromium.org/360343002/diff/20001/chrome/browser/password_...
chrome/browser/password_manager/password_store_mac.cc:963:
ScopedVector<PasswordForm> forms;
On 2014/07/02 13:15:55, vabr (Chromium) wrote:
> It's great that you caught the leak.
> Looking at the code of LoginDatabase::GetLogins* and similar methods in
> LoginDatabase which return a vector of PasswordForm*, they all pass the
> ownership to the caller.
> Could you please add comment to the declarations of those methods to make that
> clear, so that people are less likely to reintroduce the same error again?

Done.

https://codereview.chromium.org/360343002/diff/20001/chrome/browser/password_...
File chrome/browser/password_manager/password_store_mac_unittest.cc (right):

https://codereview.chromium.org/360343002/diff/20001/chrome/browser/password_...
chrome/browser/password_manager/password_store_mac_unittest.cc:1303:
matching_items.get() = owned_keychain_adapter.PasswordsFillingForm(
On 2014/07/02 13:15:55, vabr (Chromium) wrote:
> optional nit: Consider doing these 2 checks (or at least the one for facebook)
> also before you call RemoveLogins*, so that if AddLogin fails, the test does
not
> accidentally pass. (Naturally, the expected size would be 1u in both cases.)

Done.

https://codereview.chromium.org/360343002/diff/20001/chrome/browser/password_...
chrome/browser/password_manager/password_store_mac_unittest.cc:1340: // Add a
password from another profile.
On 2014/07/02 13:15:55, vabr (Chromium) wrote:
> optional nit: Indicate that "from another profile" means just that instead of
> calling PasswordStore::AddLogin on the credentials, you only add them through
> the keychain adapter. Ideally also how that's different from the 3rd party
case
> above.

Done.

[vabr (Chromium), 2014-07-02 14:10:35]

Thanks for further explanation, Vasilii!
LGTM.
Vaclav

https://codereview.chromium.org/360343002/diff/40001/components/password_mana...
File components/password_manager/core/browser/login_database.h (right):

https://codereview.chromium.org/360343002/diff/40001/components/password_mana...
components/password_manager/core/browser/login_database.h:65: // including
blacklisted matches. The caller ownes |forms| after the call.
typo: ownes->owns (also below)

[vasilii, 2014-07-02 15:28:32]

https://codereview.chromium.org/360343002/diff/40001/components/password_mana...
File components/password_manager/core/browser/login_database.h (right):

https://codereview.chromium.org/360343002/diff/40001/components/password_mana...
components/password_manager/core/browser/login_database.h:65: // including
blacklisted matches. The caller ownes |forms| after the call.
On 2014/07/02 14:10:35, vabr (Chromium) wrote:
> typo: ownes->owns (also below)

Done.

[stuartmorgan, 2014-07-02 18:43:43]

On 2014/07/02 12:34:03, vasilii wrote:
> Anyway, if you save a password and then "delete all passwords
> from the last 24 hours", you expect that this password is gone. As a user you
> don't care if the corresponding Keychain item is referenced by some older
> database entry.

Of course you do; it's data loss that you didn't ask for.

Users don't know or care about the details of our database schemes and how we
map back and forth between the two different storage system. Chrome's password
storage system distinguishes between those two stored login entires; one was
stored 24 hours ago, and one was stored a month ago. They are totally separate,
and deleting one *does not* delete the other. The fact that on Mac we happen to
use a shared secure storage entry for the actual password data doesn't change
anything from the user's perspective. Deleting both (which is effectively what
you do when you leave a dangling DB entry for the one that wasn't supposed to be
deleted) is inconsistent with the other platforms.

If this never happens, why was the password system designed to store them
separately? If users expect both to be deleted, why would they expect that on
Mac, but not on every other Chrome platform?

[vasilii, 2014-07-03 15:13:21]

On 2014/07/02 18:43:43, stuartmorgan wrote:
> On 2014/07/02 12:34:03, vasilii wrote:
> > Anyway, if you save a password and then "delete all passwords
> > from the last 24 hours", you expect that this password is gone. As a user
you
> > don't care if the corresponding Keychain item is referenced by some older
> > database entry.
> 
> Of course you do; it's data loss that you didn't ask for.
> 
> Users don't know or care about the details of our database schemes and how we
> map back and forth between the two different storage system. Chrome's password
> storage system distinguishes between those two stored login entires; one was
> stored 24 hours ago, and one was stored a month ago. They are totally
separate,
> and deleting one *does not* delete the other. The fact that on Mac we happen
to
> use a shared secure storage entry for the actual password data doesn't change
> anything from the user's perspective. Deleting both (which is effectively what
> you do when you leave a dangling DB entry for the one that wasn't supposed to
be
> deleted) is inconsistent with the other platforms.
> 
> If this never happens, why was the password system designed to store them
> separately? If users expect both to be deleted, why would they expect that on
> Mac, but not on every other Chrome platform?

1) I'm not aware of any valid usecase when two database entries'd differ only in
their element's names. The fact that the Mac keychain doesn't care about
elements at all proves that this usecase doesn't exist.
2) Let's even suppose that the usecase exists. It cannot be not supported on
Mac. Eventually we are talking about one password regardless of how many DB
entries use it.
3) I'm not sure why PasswordStore allows this redundancy. But this design
decision doesn't prove anything. For example, in Sync we identify passwords by
[origin_url, username_element,
username_value, password_element, signon_realm]. |submit_element| is absent from
this list. Everything still works OK.
4) Users expect both to be deleted on Mac because it's the same password value.
What happened on other platforms - Chrome created an entry with nonempty
elements. Then user imported a password from somewhere with empty elements.
Chrome uses only one entry. Another entry can hold user's password from 2010.
That's impossible on Mac.

Have a look at crbug/349138. It dealt with the situation I described. The fix
for it (https://codereview.chromium.org/202903002) simply merged the two
entries.

[stuartmorgan, 2014-07-07 05:54:05]

On 2014/07/03 15:13:21, vasilii wrote:
> 1) I'm not aware of any valid usecase when two database entries'd differ only
in
> their element's names.

If this is true, then we may want to revisit parts of the design of the password
store, at least on Mac.

> The fact that the Mac keychain doesn't care about
> elements at all proves that this usecase doesn't exist.

This part is not true though. The Mac keychain also doesn't provide a field for
the origin the credentials will be submitted to, and yet Chrome, Firefox, and
maybe Safari all store and check that information. Apple simply never updated
the Keychain API to reflect the reality of what browsers do now. I've had to
write a bag-on-the-side storage for that in two Mac browsers now, while Radar I
filed many years ago requesting that as a new field rots. Keychain's API is not
the be-all-end-all of internet password use cases.

[Garrett Casto, 2014-07-07 09:02:01]

I was going to say that this still has problems with the same credentials
stored in multiple profiles, but it looks like Stuart already brought this
up (https://code.google.com/p/chromium/issues/detail?id=343891#c23). It may
be rare, but the fact that this behavior is platform dependent doesn't seem
ideal. It also seems like it would be confusing and not really what the
user intends.

That being said, the only solution that I can think of is to tag the
Keychain entry with metadata about which profile is using it. This seems
like it would be a lot more work and it's not obvious that there is any
other value to be gained from doing this. The proposed solution does seem
strictly better than what we are currently doing, so it may make sense to
just keep a bug filed on this behavior and work to clean it up at some
later point. Anyone else have thoughts on this?


On Sun, Jul 6, 2014 at 10:54 PM, <stuartmorgan@chromium.org> wrote:

> On 2014/07/03 15:13:21, vasilii wrote:
>
>> 1) I'm not aware of any valid usecase when two database entries'd differ
>> only
>>
> in
>
>> their element's names.
>>
>
> If this is true, then we may want to revisit parts of the design of the
> password
> store, at least on Mac.
>
>
>  The fact that the Mac keychain doesn't care about
>> elements at all proves that this usecase doesn't exist.
>>
>
> This part is not true though. The Mac keychain also doesn't provide a
> field for
> the origin the credentials will be submitted to, and yet Chrome, Firefox,
> and
> maybe Safari all store and check that information. Apple simply never
> updated
> the Keychain API to reflect the reality of what browsers do now. I've had
> to
> write a bag-on-the-side storage for that in two Mac browsers now, while
> Radar I
> filed many years ago requesting that as a new field rots. Keychain's API
> is not
> the be-all-end-all of internet password use cases.
>
> https://codereview.chromium.org/360343002/
>

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[vasilii, 2014-07-07 12:54:08]

On 2014/07/07 05:54:05, stuartmorgan wrote:
> On 2014/07/03 15:13:21, vasilii wrote:
> > 1) I'm not aware of any valid usecase when two database entries'd differ
only
> in
> > their element's names.
> 
> If this is true, then we may want to revisit parts of the design of the
password
> store, at least on Mac.

Probably. "Only Mac" isn't good for Syncing passwords.

> > The fact that the Mac keychain doesn't care about
> > elements at all proves that this usecase doesn't exist.
> 
> This part is not true though. The Mac keychain also doesn't provide a field
for
> the origin the credentials will be submitted to, and yet Chrome, Firefox, and
> maybe Safari all store and check that information. Apple simply never updated
> the Keychain API to reflect the reality of what browsers do now. I've had to
> write a bag-on-the-side storage for that in two Mac browsers now, while Radar
I
> filed many years ago requesting that as a new field rots. Keychain's API is
not
> the be-all-end-all of internet password use cases.

OK. The fact suggests that this usecase doesn't exist. Anyway, we have three
owners reviewing this CL and no one clarified why having two passwords with
different elements can be useful.

Garrett, I am not sure how to solve the problem of sharing the same password
across the profiles. Tagging passwords has another drawback (see
https://code.google.com/p/chromium/issues/detail?id=357233#c7) that is worse
than the proposed behavior. Wiping out other profiles (currently) is definitely
a bug. Cleaning the identical password from another profile (this CL) is between
bug and feature (in the context of RemoveLoginsCreatedBetween). Though cleaning
the identical password from chrome://settings/passwords is a bug again.

[chromium-reviews, 2014-07-07 18:17:14]

On Mon, Jul 7, 2014 at 5:54 AM, <vasilii@chromium.org> wrote:

> On 2014/07/07 05:54:05, stuartmorgan wrote:
>
>> On 2014/07/03 15:13:21, vasilii wrote:
>> > 1) I'm not aware of any valid usecase when two database entries'd differ
>>
> only
>
>> in
>> > their element's names.
>>
>
>  If this is true, then we may want to revisit parts of the design of the
>>
> password
>
>> store, at least on Mac.
>>
>
> Probably. "Only Mac" isn't good for Syncing passwords.
>
>
>  > The fact that the Mac keychain doesn't care about
>> > elements at all proves that this usecase doesn't exist.
>>
>
>  This part is not true though. The Mac keychain also doesn't provide a
>> field
>>
> for
>
>> the origin the credentials will be submitted to, and yet Chrome, Firefox,
>> and
>> maybe Safari all store and check that information. Apple simply never
>> updated
>> the Keychain API to reflect the reality of what browsers do now. I've had
>> to
>> write a bag-on-the-side storage for that in two Mac browsers now, while
>> Radar
>>
> I
>
>> filed many years ago requesting that as a new field rots. Keychain's API
>> is
>>
> not
>
>> the be-all-end-all of internet password use cases.
>>
>
> OK. The fact suggests that this usecase doesn't exist. Anyway, we have
> three
> owners reviewing this CL and no one clarified why having two passwords with
> different elements can be useful.
>
>
The only use case I can think of is a page that has both signup and login
forms together. Given that we are actively working on not filling passwords
for signup forms, it doesn't seem like that case matters very much. I was
actually curious a little while ago about how often these attributes were
used for selecting which passwords to show, so I added an UMA stat to track
this (
https://uma.googleplex.com/p/chrome/histograms/?dayCount=1&endDate=07-07-2014...).
Unfortunately this doesn't take into account how often credentials are
culled because of e.g. different path vs. different username_element. It's
also possible that a lot of the credentials here would be collapsed if we
didn't pay attention to html elements.

In any case, it's not clear to my why these are particularly useful. Stuart
may have some more insight since he was around earlier. I also agree that
it's not clear that it's possible to end up with credentials differing only
by html element attributes since even if these attributes change for a
particular form, we would continue offering the old credentials and
wouldn't save a new version. So I think that this bug may be just a
theoretical concern, though I'd think that we'd want to leave a comment
describing the possibility (and why we think it's not a problem).

Garrett, I am not sure how to solve the problem of sharing the same password
> across the profiles. Tagging passwords has another drawback (see
> https://code.google.com/p/chromium/issues/detail?id=357233#c7) that is
> worse
> than the proposed behavior. Wiping out other profiles (currently) is
> definitely
> a bug. Cleaning the identical password from another profile (this CL) is
> between
> bug and feature (in the context of RemoveLoginsCreatedBetween). Though
> cleaning
> the identical password from chrome://settings/passwords is a bug again.
>
>
I'll follow up on this in the bug to try to keep the conversation in one
place.


> https://codereview.chromium.org/360343002/
>

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[stuartmorgan, 2014-07-07 18:27:45]

Seems like there's general feeling that having different form elements shouldn't
be possible; based on that, LGTM.

I'd feel better about this longer term if it were made impossible all the way
through the stack across platforms (by changing the LoginDB schema) rather than
having Mac silently rely on the belief that it's not supposed to happen.

[Garrett Casto, 2014-07-07 18:36:36]

On 2014/07/07 18:27:45, stuartmorgan wrote:
> Seems like there's general feeling that having different form elements
shouldn't
> be possible; based on that, LGTM.
> 
> I'd feel better about this longer term if it were made impossible all the way
> through the stack across platforms (by changing the LoginDB schema) rather
than
> having Mac silently rely on the belief that it's not supposed to happen.

LGTM

I agree that if we don't think that these have use we should just change the
schema. I'm going to try and get ahold of Tim to make sure that there wasn't a
use case here that I just can't think of.

[vasilii, 2014-07-08 09:13:50]

The CQ bit was checked by vasilii@chromium.org

[commit-bot: I haz the power, 2014-07-08 09:14:58]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/vasilii@chromium.org/360343002/80001

[commit-bot: I haz the power, 2014-07-08 12:18:48]

Change committed as 281728