Remove password-manager-reauthentication flag

Remove password-manager-reauthentication flag

The flag allowed to switch off reauthentication before viewing passwords on
Mac and Win, a feature which has been launched two years ago.

Together with removing the flag, also the preference
password_manager_allow_show_passwords is removed -- it had no effect unless
the password-manager-reauthentication flag was set.

After this CL, Chrome on Mac or Win will not put passwords inside
about:settings/passwords unless the user reauthenticated.

BUG=598698
Committed: https://crrev.com/f639489fe56fd214f2584a71a90d36b30bdfb33c
Cr-Commit-Position: refs/heads/master@{#384601}

[vabr (Chromium), 2016-03-31 13:57:56]

vabr@chromium.org changed reviewers:
+ bartfab@chromium.org, bauerb@chromium.org, finnur@chromium.org,
vasilii@chromium.org

[vabr (Chromium), 2016-03-31 13:57:57]

Hello reviewers!
Please review the following:

@bartfab
  components/policy/resources/policy_templates.json
  chrome/test/data/policy/policy_test_cases.json
  chrome/browser/policy/configuration_policy_handler_list_factory.cc

@vasilii
  chrome/browser/ui/passwords/*
  components/password_manager/core/*
  chrome/browser/about_flags.cc
  chrome/common/chrome_switches.cc

@bauerb
  chrome/browser/android/password_ui_view_android.*
  chrome/browser/resources/*
  chrome/browser/ui/webui/options/password_manager_handler.*

@finnur
  chrome/browser/extensions/api/*

Thanks!
Vaclav

[Bernhard Bauer, 2016-03-31 14:34:27]

LGTM w/ nits:

https://codereview.chromium.org/1846623002/diff/40001/chrome/browser/android/...
File chrome/browser/android/password_ui_view_android.h (right):

https://codereview.chromium.org/1846623002/diff/40001/chrome/browser/android/...
chrome/browser/android/password_ui_view_android.h:37: password_list) override;
Ick. I would probably format this like this: 

  void SetPasswordList(
      const std::vector<scoped_ptr<autofill::PasswordForm>>& password_list)
          override;

or like SetPasswordExceptionList() below.

https://codereview.chromium.org/1846623002/diff/40001/chrome/browser/ui/webui...
File chrome/browser/ui/webui/options/password_manager_handler.h (right):

https://codereview.chromium.org/1846623002/diff/40001/chrome/browser/ui/webui...
chrome/browser/ui/webui/options/password_manager_handler.h:42: password_list)
override;
Here as well.

[Finnur, 2016-03-31 15:08:10]

chrome/browser/extensions/api/* LGTM

[vasilii, 2016-03-31 15:34:38]

https://codereview.chromium.org/1846623002/diff/40001/chrome/browser/about_fl...
File chrome/browser/about_flags.cc (left):

https://codereview.chromium.org/1846623002/diff/40001/chrome/browser/about_fl...
chrome/browser/about_flags.cc:926:
IDS_FLAGS_PASSWORD_MANAGER_REAUTHENTICATION_DESCRIPTION, kOsMac | kOsWin,
The strings are to be dropped too.

https://codereview.chromium.org/1846623002/diff/40001/chrome/browser/ui/passw...
File chrome/browser/ui/passwords/password_manager_presenter.cc (left):

https://codereview.chromium.org/1846623002/diff/40001/chrome/browser/ui/passw...
chrome/browser/ui/passwords/password_manager_presenter.cc:64: // TODO(mdm):
remove this hack once it is no longer necessary.
Not true anymore?

[vabr (Chromium), 2016-03-31 15:47:20]

Thanks for the reviews so far!

Vasilii (and Bernhard), please have a look at how I addressed your comments.

Thanks,
Vaclav

https://codereview.chromium.org/1846623002/diff/40001/chrome/browser/about_fl...
File chrome/browser/about_flags.cc (left):

https://codereview.chromium.org/1846623002/diff/40001/chrome/browser/about_fl...
chrome/browser/about_flags.cc:926:
IDS_FLAGS_PASSWORD_MANAGER_REAUTHENTICATION_DESCRIPTION, kOsMac | kOsWin,
On 2016/03/31 15:34:38, vasilii wrote:
> The strings are to be dropped too.

Done, good catch!

https://codereview.chromium.org/1846623002/diff/40001/chrome/browser/android/...
File chrome/browser/android/password_ui_view_android.h (right):

https://codereview.chromium.org/1846623002/diff/40001/chrome/browser/android/...
chrome/browser/android/password_ui_view_android.h:37: password_list) override;
On 2016/03/31 14:34:27, Bernhard Bauer wrote:
> Ick. I would probably format this like this: 
> 
>   void SetPasswordList(
>       const std::vector<scoped_ptr<autofill::PasswordForm>>& password_list)
>           override;
> 
> or like SetPasswordExceptionList() below.

Done.

https://codereview.chromium.org/1846623002/diff/40001/chrome/browser/ui/passw...
File chrome/browser/ui/passwords/password_manager_presenter.cc (left):

https://codereview.chromium.org/1846623002/diff/40001/chrome/browser/ui/passw...
chrome/browser/ui/passwords/password_manager_presenter.cc:64: // TODO(mdm):
remove this hack once it is no longer necessary.
On 2016/03/31 15:34:38, vasilii wrote:
> Not true anymore?

Not relevant any more -- we no longer care about the order of calls to
Initialize() and SetPasswordList(), because the latter no longer depends on the
former. And because SetPasswordList does not call Initialize() any more, there
is no danger of calling Initialize() multiple times.

https://codereview.chromium.org/1846623002/diff/40001/chrome/browser/ui/webui...
File chrome/browser/ui/webui/options/password_manager_handler.h (right):

https://codereview.chromium.org/1846623002/diff/40001/chrome/browser/ui/webui...
chrome/browser/ui/webui/options/password_manager_handler.h:42: password_list)
override;
On 2016/03/31 14:34:27, Bernhard Bauer wrote:
> Here as well.

Done.

[vasilii, 2016-03-31 15:55:59]

lgtm

[vabr (Chromium), 2016-03-31 15:59:57]

The CQ bit was checked by vabr@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-03-31 16:00:30]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1846623002/60001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1846623002/60001

[commit-bot: I haz the power, 2016-03-31 16:58:12]

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

[commit-bot: I haz the power, 2016-03-31 16:58:13]

Dry run: This issue passed the CQ dry run.

[bartfab (slow), 2016-04-01 14:54:16]

Policy lgtm

https://codereview.chromium.org/1846623002/diff/60001/components/policy/resou...
File components/policy/resources/policy_templates.json (right):

https://codereview.chromium.org/1846623002/diff/60001/components/policy/resou...
components/policy/resources/policy_templates.json:1195: 'example_value': False,
Nit: Please add:

'deprecated': True,

[vabr (Chromium), 2016-04-01 14:59:01]

Thanks!

https://codereview.chromium.org/1846623002/diff/60001/components/policy/resou...
File components/policy/resources/policy_templates.json (right):

https://codereview.chromium.org/1846623002/diff/60001/components/policy/resou...
components/policy/resources/policy_templates.json:1195: 'example_value': False,
On 2016/04/01 14:54:16, bartfab (slow) wrote:
> Nit: Please add:
> 
> 'deprecated': True,

Done.

[vabr (Chromium), 2016-04-01 14:59:07]

The CQ bit was checked by vabr@chromium.org

[vabr (Chromium), 2016-04-01 14:59:08]

The patchset sent to the CQ was uploaded after l-g-t-m from bauerb@chromium.org,
finnur@chromium.org, bartfab@chromium.org, vasilii@chromium.org
Link to the patchset: https://codereview.chromium.org/1846623002/#ps100001
(title: "Just rebased")

[commit-bot: I haz the power, 2016-04-01 14:59:29]

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

[commit-bot: I haz the power, 2016-04-01 14:59:30]

COMMIT=false detected. CQ is abandoning the patch.

[vabr (Chromium), 2016-04-01 14:59:52]

Description was changed from

==========
Remove password-manager-reauthentication flag

COMMIT=false
Presubmit warns me that the policy JSON is syntactically incorrect, need to
figure out how to fix that.

The flag allowed to switch off reauthentication before viewing passwords on
Mac and Win, a feature which has been launched two years ago.

Together with removing the flag, also the preference
password_manager_allow_show_passwords is removed -- it had no effect unless
the password-manager-reauthentication flag was set.

After this CL, Chrome on Mac or Win will not put passwords inside
about:settings/passwords unless the user reauthenticated.

BUG=598698
==========

to

==========
Remove password-manager-reauthentication flag

The flag allowed to switch off reauthentication before viewing passwords on
Mac and Win, a feature which has been launched two years ago.

Together with removing the flag, also the preference
password_manager_allow_show_passwords is removed -- it had no effect unless
the password-manager-reauthentication flag was set.

After this CL, Chrome on Mac or Win will not put passwords inside
about:settings/passwords unless the user reauthenticated.

BUG=598698
==========

[vabr (Chromium), 2016-04-01 14:59:58]

The CQ bit was checked by vabr@chromium.org

[commit-bot: I haz the power, 2016-04-01 15:00:11]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1846623002/100001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1846623002/100001

[commit-bot: I haz the power, 2016-04-01 16:21:45]

Description was changed from

==========
Remove password-manager-reauthentication flag

The flag allowed to switch off reauthentication before viewing passwords on
Mac and Win, a feature which has been launched two years ago.

Together with removing the flag, also the preference
password_manager_allow_show_passwords is removed -- it had no effect unless
the password-manager-reauthentication flag was set.

After this CL, Chrome on Mac or Win will not put passwords inside
about:settings/passwords unless the user reauthenticated.

BUG=598698
==========

to

==========
Remove password-manager-reauthentication flag

The flag allowed to switch off reauthentication before viewing passwords on
Mac and Win, a feature which has been launched two years ago.

Together with removing the flag, also the preference
password_manager_allow_show_passwords is removed -- it had no effect unless
the password-manager-reauthentication flag was set.

After this CL, Chrome on Mac or Win will not put passwords inside
about:settings/passwords unless the user reauthenticated.

BUG=598698
==========

[commit-bot: I haz the power, 2016-04-01 16:21:46]

Committed patchset #6 (id:100001)

[commit-bot: I haz the power, 2016-04-01 16:23:33]

Description was changed from

==========
Remove password-manager-reauthentication flag

The flag allowed to switch off reauthentication before viewing passwords on
Mac and Win, a feature which has been launched two years ago.

Together with removing the flag, also the preference
password_manager_allow_show_passwords is removed -- it had no effect unless
the password-manager-reauthentication flag was set.

After this CL, Chrome on Mac or Win will not put passwords inside
about:settings/passwords unless the user reauthenticated.

BUG=598698
==========

to

==========
Remove password-manager-reauthentication flag

The flag allowed to switch off reauthentication before viewing passwords on
Mac and Win, a feature which has been launched two years ago.

Together with removing the flag, also the preference
password_manager_allow_show_passwords is removed -- it had no effect unless
the password-manager-reauthentication flag was set.

After this CL, Chrome on Mac or Win will not put passwords inside
about:settings/passwords unless the user reauthenticated.

BUG=598698

Committed: https://crrev.com/f639489fe56fd214f2584a71a90d36b30bdfb33c
Cr-Commit-Position: refs/heads/master@{#384601}
==========

[commit-bot: I haz the power, 2016-04-01 16:23:34]

Patchset 6 (id:??) landed as
https://crrev.com/f639489fe56fd214f2584a71a90d36b30bdfb33c
Cr-Commit-Position: refs/heads/master@{#384601}

[Will Harris, 2017-01-24 17:20:36]

hmm sorry I did not see this until now. Removing this switch removes a failsafe
case for those users who are currently unable to use reauthentication due to
outstanding bugs e.g. 347825.

but I suppose it's a bit late now to put this back in?

[vabr (Chromium), 2017-01-27 20:45:23]

On 2017/01/24 17:20:36, Will Harris wrote:
> hmm sorry I did not see this until now. Removing this switch removes a
failsafe
> case for those users who are currently unable to use reauthentication due to
> outstanding bugs e.g. 347825.
> 
> but I suppose it's a bit late now to put this back in?

Removing the escape hatch is an unfortunate effect. But at the same time, if the
feature is aimed at protecting the privacy against casual snoopers, having a
flag to disable it renders it useless enough to discard completely, IMO.

Reintroducing the flag will not prevent disruption given its absence for several
months. It might make the disruption shorter, but it also can just decrease the
urgency and put this issue in an indefinite limbo. I don't want the flag to
become a de facto user interface for password manager.

We have a couple of options:

* Ideally if we could figure out what the problem is in cases when the users are
unable to unlock the settings, we can just fix it.

* Or if we can at least detect a class of problematic Windows instances, we
might consider disabling this feature for those. Especially if these are likely
enterprise instances. We would need to consult the privacy team once we know
which class of instances this could be.

* Or we can actually allow the user to switch the guard off, but with a time
delay: "Check this check-box to allow viewing passwords without prompt, starting
in 24 hours" and possibly a running notification: "Passwords viewable without a
prompt in 17 hours. Click this button to cancel." This crude version is likely
impossible to get a UI approval for, but perhaps we could tweak it to be more
palatable.

(* My favourite would be to ditch the reauthentication completely, given its
dubious value with false expectations of protection. But this has been decided
in the past, and the public was apparently furious about it missing on Mac and
Win.)

I'm happy to talk more, with the caveat that I'm working only one day a week
until mid February. During my absence sabineb@, our PM, is a good person to talk
to about this.

Cheers,
Vaclav