Update error code to ITEM_FAILURE for MAC decrypted the password.

Update error code to ITEM_FAILURE for MAC decrypted the password.

When the encryption key for the login database is lost (deleted from the
Keychain, Keychain itself deleted, or locked), and the user has some
passwords stored (encrypted) in the login database, Chrome cannot "read
all" passwords. Now Chrome returns all passwords that it was able to
decrypt.
Tested manually by removing the key from the Keychain.

R=vabr@chromium.org
BUG=479725

[vabr (Chromium), 2017-06-28 14:20:43]

vabr@chromium.org changed reviewers:
+ vasilii@chromium.org

[vabr (Chromium), 2017-06-28 14:20:44]

I think Vasilii is the best person to review this topic.

Cheers,
Vaclav

[vasilii, 2017-06-28 15:31:38]

I'm against landing this change.

It basically says 'we detected a problem, we can't recover from it but let's
hide it from the user'. Currently when the key is lost, the sync becomes broken
and chrome://settings/passwords will stay empty forever.
After the patch Sync will stay green. The passwords may be either recreated or
not in case the new encryption key wasn't created. chrome://settings/passwords
may contain only the new passwords. The password database still contains the
trash values. While the data loss is the same in both scenarios, the second one
is impossible to debug.

[chuguev, 2017-06-29 07:15:04]

Vasilii,
I understand that the problem is more complex and it should be fixed.
But now I propose this change to make the behaviour on MAC is similar to other
platforms in similar situations (e.g. win).

[vabr (Chromium), 2017-06-29 07:23:09]

On 2017/06/29 07:15:04, chuguev wrote:
> Vasilii,
> I understand that the problem is more complex and it should be fixed.
> But now I propose this change to make the behaviour on MAC is similar to other
> platforms in similar situations (e.g. win).

Win is not similar to Mac. On Mac, the encryption will break any time the key in
Keychain gets deleted or inaccessible. On Win, the encryption is provided by the
OS and tied to the user account, so does not break so easily.

I fully second Vasilii's opinion that the proposed change will cause chaos in
the password storage which will be much harder to detect than today's issues.
Therefore, not lgtm.

Cheers,
Vaclav

[chuguev, 2017-07-03 08:10:16]

On 2017/06/29 07:23:09, vabr (Chromium) wrote:
> On 2017/06/29 07:15:04, chuguev wrote:
> > Vasilii,
> > I understand that the problem is more complex and it should be fixed.
> > But now I propose this change to make the behaviour on MAC is similar to
other
> > platforms in similar situations (e.g. win).
> 
> Win is not similar to Mac. On Mac, the encryption will break any time the key
in
> Keychain gets deleted or inaccessible. On Win, the encryption is provided by
the
> OS and tied to the user account, so does not break so easily.
> 
> I fully second Vasilii's opinion that the proposed change will cause chaos in
> the password storage which will be much harder to detect than today's issues.
> Therefore, not lgtm.
> 
> Cheers,
> Vaclav

Vaclav, vasilii, I understand you.
But let’s look at the following case:
- There are a set of passwords. All passwords become invalid because the master
key has changed.
- When I request a list of passwords - the list is empty.
- I update some of the passwords. Now they are substituted to forms, but the
password list is empty.
- I add some new passwords. The behaviour remain similar to the previous one.
- Only when I update all old (invalid) passwords, the list will display all
saved passwords.
I think we meet not encrypted passwords, we should follow one of the following
ways:
- Ignore not decrypted passwords.
- Clean database from non-decrypted passwords.

As vasilii says, in any case, we will lose the data. 
But now we say nothing to the user about this problem and continue to work. We
update passwords to new and substitute new passwords to forms but show empy
passwords list.

What is your opinion?

[vabr (Chromium), 2017-07-03 08:15:17]

On 2017/07/03 08:10:16, chuguev wrote:
> On 2017/06/29 07:23:09, vabr (Chromium) wrote:
> > On 2017/06/29 07:15:04, chuguev wrote:
> > > Vasilii,
> > > I understand that the problem is more complex and it should be fixed.
> > > But now I propose this change to make the behaviour on MAC is similar to
> other
> > > platforms in similar situations (e.g. win).
> > 
> > Win is not similar to Mac. On Mac, the encryption will break any time the
key
> in
> > Keychain gets deleted or inaccessible. On Win, the encryption is provided by
> the
> > OS and tied to the user account, so does not break so easily.
> > 
> > I fully second Vasilii's opinion that the proposed change will cause chaos
in
> > the password storage which will be much harder to detect than today's
issues.
> > Therefore, not lgtm.
> > 
> > Cheers,
> > Vaclav
> 
> Vaclav, vasilii, I understand you.
> But let’s look at the following case:
> - There are a set of passwords. All passwords become invalid because the
master
> key has changed.
> - When I request a list of passwords - the list is empty.
> - I update some of the passwords. Now they are substituted to forms, but the
> password list is empty.
> - I add some new passwords. The behaviour remain similar to the previous one.
> - Only when I update all old (invalid) passwords, the list will display all
> saved passwords.
> I think we meet not encrypted passwords, we should follow one of the following
> ways:
> - Ignore not decrypted passwords.
> - Clean database from non-decrypted passwords.
> 
> As vasilii says, in any case, we will lose the data. 
> But now we say nothing to the user about this problem and continue to work. We
> update passwords to new and substitute new passwords to forms but show empy
> passwords list.
> 
> What is your opinion?

I agree that we could help the user more.

In case the encryption key in the Keychain is lost forever, the best thing
Chrome can do is to delete the passwords and attempt to re-sync in case the user
syncs their passwords. Chrome does not know, whether the key in the Keychain is
lost forever (the user might be able to restore access to it), so the safest
option would be to ask the user.

Vasilii, I recently talked about this with battre@ and he pointed out you had
some plans towards a more user-friendly solution for these situations?

Cheers,
Vaclav

[vasilii, 2017-07-03 15:54:44]

The plan is that for Sync users we should delete the file and resync the
passwords https://crbug.com/730625
For non sync users we definitely shouldn't allow any new passwords with another
encryption key (we allow now). Rather we should show an error to the user
somehow. In case the user has a backup of the Keychain, it's possible to get the
encryption key back. Otherwise, we may point the user to a help article or
delete the file ourselves.

That's a medium project that requires a design doc. So far it's not assigned to
anyone.

[vabr (Chromium), 2017-07-03 18:21:33]

On 2017/07/03 15:54:44, vasilii wrote:
> The plan is that for Sync users we should delete the file and resync the
> passwords https://crbug.com/730625
> For non sync users we definitely shouldn't allow any new passwords with
another
> encryption key (we allow now). Rather we should show an error to the user
> somehow. In case the user has a backup of the Keychain, it's possible to get
the
> encryption key back. Otherwise, we may point the user to a help article or
> delete the file ourselves.
> 
> That's a medium project that requires a design doc. So far it's not assigned
to
> anyone.

Thanks, Vasilii, for the useful context.

I think the non-syncing, error-showing part might fall under
https://crbug.com/479725.

I agree with the conclusion that this is not a matter of a single CL, but a
properly done project, including a design doc before code changes.

Cheers,
Vaclav