[Mac] Add option to reauthenticate the OS user before revealing passwords.

[Mac] Add option to reauthenticate the OS user before revealing passwords.

When the flag is enabled and the user attempts to reveal a plaintext password
in chrome://settings/passwords, they will be prompted to reauthenticate with
their OS password. This matches Safari's behaviour on OS X.

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

[Patrick Dubroy, 2013-10-18 09:32:49]

Please take a look.
thakis: Mac stuff
gcasto: password_manager

[Patrick Dubroy, 2013-10-18 09:52:15]

cc: wfh

[Will Harris, 2013-10-18 11:56:51]

https://codereview.chromium.org/28713002/diff/80001/chrome/browser/ui/webui/o...
File chrome/browser/ui/webui/options/password_manager_handler.cc (right):

https://codereview.chromium.org/28713002/diff/80001/chrome/browser/ui/webui/o...
chrome/browser/ui/webui/options/password_manager_handler.cc:167: if
(require_reauthentication_ && !is_user_authenticated_) {
for users who leave their browser open for several days, e.g. googlers, or
people who use 'suspend' on their laptop - perhaps we need a timeout rather than
remembering that an authentication succeeded forever?

[Patrick Dubroy, 2013-10-18 13:15:53]

https://codereview.chromium.org/28713002/diff/80001/chrome/browser/ui/webui/o...
File chrome/browser/ui/webui/options/password_manager_handler.cc (right):

https://codereview.chromium.org/28713002/diff/80001/chrome/browser/ui/webui/o...
chrome/browser/ui/webui/options/password_manager_handler.cc:167: if
(require_reauthentication_ && !is_user_authenticated_) {
On 2013/10/18 11:56:52, Will Harris wrote:
> for users who leave their browser open for several days, e.g. googlers, or
> people who use 'suspend' on their laptop - perhaps we need a timeout rather
than
> remembering that an authentication succeeded forever?

Yeah. For some reason I was thinking I'd leave that for another CL, but it's
pretty easy, so I may as well do it here.

[Nico, 2013-10-18 14:14:24]

Since this is a somewhat contentious issue, I'd like to have buy-in from
chrome-ux-leads to experiment with this ahead of time. Do you have that?

[Patrick Dubroy, 2013-10-18 14:30:58]

On 2013/10/18 14:14:24, Nico wrote:
> Since this is a somewhat contentious issue, I'd like to have buy-in from
> chrome-ux-leads to experiment with this ahead of time. Do you have that?

This was approved by Linus and Avni, as well as jschuh. You can get more
background from the design doc linked in the bug.

[Nico, 2013-10-18 14:47:57]

https://codereview.chromium.org/28713002/diff/140001/chrome/app/generated_res...
File chrome/app/generated_resources.grd (right):

https://codereview.chromium.org/28713002/diff/140001/chrome/app/generated_res...
chrome/app/generated_resources.grd:7538: +          <ph
name="PRODUCT_NAME">$1<ex>Google Chrome</ex></ph> is trying to show passwords.
Please read "[chromium-dev] PRODUCT_NAME placeholders in strings considered
harmful"

https://codereview.chromium.org/28713002/diff/140001/chrome/browser/about_fla...
File chrome/browser/about_flags.cc (right):

https://codereview.chromium.org/28713002/diff/140001/chrome/browser/about_fla...
chrome/browser/about_flags.cc:1268: ENABLE_DISABLE_VALUE_TYPE(
Isn't a SINGLE_VALUE_TYPE for the enable flag enough? Why do you need a disable
flag?

https://codereview.chromium.org/28713002/diff/140001/chrome/browser/password_...
File chrome/browser/password_manager/password_manager_util.h (right):

https://codereview.chromium.org/28713002/diff/140001/chrome/browser/password_...
chrome/browser/password_manager/password_manager_util.h:14: bool
AuthenticateUser();
As is, this api forces the implementation to run a nested message loop. Should
this get a "done" callback instead?

[Patrick Dubroy, 2013-10-18 15:24:00]

https://codereview.chromium.org/28713002/diff/140001/chrome/app/generated_res...
File chrome/app/generated_resources.grd (right):

https://codereview.chromium.org/28713002/diff/140001/chrome/app/generated_res...
chrome/app/generated_resources.grd:7538: +          <ph
name="PRODUCT_NAME">$1<ex>Google Chrome</ex></ph> is trying to show passwords.
On 2013/10/18 14:47:57, Nico wrote:
> Please read "[chromium-dev] PRODUCT_NAME placeholders in strings considered
> harmful"

Ok, I will fix this up.

https://codereview.chromium.org/28713002/diff/140001/chrome/browser/about_fla...
File chrome/browser/about_flags.cc (right):

https://codereview.chromium.org/28713002/diff/140001/chrome/browser/about_fla...
chrome/browser/about_flags.cc:1268: ENABLE_DISABLE_VALUE_TYPE(
On 2013/10/18 14:47:57, Nico wrote:
> Isn't a SINGLE_VALUE_TYPE for the enable flag enough? Why do you need a
disable
> flag?

I've done this for features before with the intention of minimizing the code
churn to turn a feature on or off by default. This way turning it on by default
is a 1-line CL, and it's simpler and safer to revert if we change our minds
later.

If you think it's not a good idea, I can just add the enable flag.

https://codereview.chromium.org/28713002/diff/140001/chrome/browser/password_...
File chrome/browser/password_manager/password_manager_util.h (right):

https://codereview.chromium.org/28713002/diff/140001/chrome/browser/password_...
chrome/browser/password_manager/password_manager_util.h:14: bool
AuthenticateUser();
On 2013/10/18 14:47:57, Nico wrote:
> As is, this api forces the implementation to run a nested message loop. Should
> this get a "done" callback instead?

I think on most platforms it will simply call into a native API to show a modal
dialog, so this seems like the simplest approach for now.

[Nico, 2013-10-18 15:28:45]

On Fri, Oct 18, 2013 at 8:24 AM, <dubroy@chromium.org> wrote:

>
> https://codereview.chromium.**org/28713002/diff/140001/**
>
chrome/app/generated_**resources.grd<https://codereview.chromium.org/28713002/diff/140001/chrome/app/generated_resources.grd>
> File chrome/app/generated_**resources.grd (right):
>
> https://codereview.chromium.**org/28713002/diff/140001/**
>
chrome/app/generated_**resources.grd#newcode7538<https://codereview.chromium.org/28713002/diff/140001/chrome/app/generated_resources.grd#newcode7538>
> chrome/app/generated_**resources.grd:7538: +          <ph
> name="PRODUCT_NAME">$1<ex>**Google Chrome</ex></ph> is trying to show
> passwords.
> On 2013/10/18 14:47:57, Nico wrote:
>
>> Please read "[chromium-dev] PRODUCT_NAME placeholders in strings
>>
> considered
>
>> harmful"
>>
>
> Ok, I will fix this up.
>
>
> https://codereview.chromium.**org/28713002/diff/140001/**
>
chrome/browser/about_flags.cc<https://codereview.chromium.org/28713002/diff/140001/chrome/browser/about_flags.cc>
> File chrome/browser/about_flags.cc (right):
>
> https://codereview.chromium.**org/28713002/diff/140001/**
>
chrome/browser/about_flags.cc#**newcode1268<https://codereview.chromium.org/28713002/diff/140001/chrome/browser/about_flags.cc#newcode1268>
> chrome/browser/about_flags.cc:**1268: ENABLE_DISABLE_VALUE_TYPE(
> On 2013/10/18 14:47:57, Nico wrote:
>
>> Isn't a SINGLE_VALUE_TYPE for the enable flag enough? Why do you need
>>
> a disable
>
>> flag?
>>
>
> I've done this for features before with the intention of minimizing the
> code churn to turn a feature on or off by default. This way turning it
> on by default is a 1-line CL, and it's simpler and safer to revert if we
> change our minds later.
>

In the normal case, when you turn on a feature you should remove the
command line parameters and the about:flags entry and just turn it on.
--disable flag are for things that might need debugging on user machines
(gpu-related features for example). Do you think this feature will need
that?


>
> If you think it's not a good idea, I can just add the enable flag.
>
>
> https://codereview.chromium.**org/28713002/diff/140001/**
>
chrome/browser/password_**manager/password_manager_util.**h<https://codereview.chromium.org/28713002/diff/140001/chrome/browser/password_manager/password_manager_util.h>
> File chrome/browser/password_**manager/password_manager_util.**h (right):
>
> https://codereview.chromium.**org/28713002/diff/140001/**
>
chrome/browser/password_**manager/password_manager_util.**h#newcode14<https://codereview.chromium.org/28713002/diff/140001/chrome/browser/password_manager/password_manager_util.h#newcode14>
> chrome/browser/password_**manager/password_manager_util.**h:14: bool
> AuthenticateUser();
> On 2013/10/18 14:47:57, Nico wrote:
>
>> As is, this api forces the implementation to run a nested message
>>
> loop. Should
>
>> this get a "done" callback instead?
>>
>
> I think on most platforms it will simply call into a native API to show
> a modal dialog, so this seems like the simplest approach for now.
>

Ok, as long as you thought about it :-)


>
>
https://codereview.chromium.**org/28713002/<https://codereview.chromium.org/2...
>

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

[Nico, 2013-10-18 15:30:45]

(also, it looks like gcasto is away until monday, maybe pick another
password_manager/OWNER


On Fri, Oct 18, 2013 at 8:28 AM, Nico Weber <thakis@chromium.org> wrote:

> On Fri, Oct 18, 2013 at 8:24 AM, <dubroy@chromium.org> wrote:
>
>>
>> https://codereview.chromium.**org/28713002/diff/140001/**
>>
chrome/app/generated_**resources.grd<https://codereview.chromium.org/28713002/diff/140001/chrome/app/generated_resources.grd>
>> File chrome/app/generated_**resources.grd (right):
>>
>> https://codereview.chromium.**org/28713002/diff/140001/**
>>
chrome/app/generated_**resources.grd#newcode7538<https://codereview.chromium.org/28713002/diff/140001/chrome/app/generated_resources.grd#newcode7538>
>> chrome/app/generated_**resources.grd:7538: +          <ph
>> name="PRODUCT_NAME">$1<ex>**Google Chrome</ex></ph> is trying to show
>> passwords.
>> On 2013/10/18 14:47:57, Nico wrote:
>>
>>> Please read "[chromium-dev] PRODUCT_NAME placeholders in strings
>>>
>> considered
>>
>>> harmful"
>>>
>>
>> Ok, I will fix this up.
>>
>>
>> https://codereview.chromium.**org/28713002/diff/140001/**
>>
chrome/browser/about_flags.cc<https://codereview.chromium.org/28713002/diff/140001/chrome/browser/about_flags.cc>
>> File chrome/browser/about_flags.cc (right):
>>
>> https://codereview.chromium.**org/28713002/diff/140001/**
>>
chrome/browser/about_flags.cc#**newcode1268<https://codereview.chromium.org/28713002/diff/140001/chrome/browser/about_flags.cc#newcode1268>
>> chrome/browser/about_flags.cc:**1268: ENABLE_DISABLE_VALUE_TYPE(
>> On 2013/10/18 14:47:57, Nico wrote:
>>
>>> Isn't a SINGLE_VALUE_TYPE for the enable flag enough? Why do you need
>>>
>> a disable
>>
>>> flag?
>>>
>>
>> I've done this for features before with the intention of minimizing the
>> code churn to turn a feature on or off by default. This way turning it
>> on by default is a 1-line CL, and it's simpler and safer to revert if we
>> change our minds later.
>>
>
> In the normal case, when you turn on a feature you should remove the
> command line parameters and the about:flags entry and just turn it on.
> --disable flag are for things that might need debugging on user machines
> (gpu-related features for example). Do you think this feature will need
> that?
>
>
>>
>> If you think it's not a good idea, I can just add the enable flag.
>>
>>
>> https://codereview.chromium.**org/28713002/diff/140001/**
>>
chrome/browser/password_**manager/password_manager_util.**h<https://codereview.chromium.org/28713002/diff/140001/chrome/browser/password_manager/password_manager_util.h>
>> File chrome/browser/password_**manager/password_manager_util.**h (right):
>>
>> https://codereview.chromium.**org/28713002/diff/140001/**
>>
chrome/browser/password_**manager/password_manager_util.**h#newcode14<https://codereview.chromium.org/28713002/diff/140001/chrome/browser/password_manager/password_manager_util.h#newcode14>
>> chrome/browser/password_**manager/password_manager_util.**h:14: bool
>> AuthenticateUser();
>> On 2013/10/18 14:47:57, Nico wrote:
>>
>>> As is, this api forces the implementation to run a nested message
>>>
>> loop. Should
>>
>>> this get a "done" callback instead?
>>>
>>
>> I think on most platforms it will simply call into a native API to show
>> a modal dialog, so this seems like the simplest approach for now.
>>
>
> Ok, as long as you thought about it :-)
>
>
>>
>>
https://codereview.chromium.**org/28713002/<https://codereview.chromium.org/2...
>>
>
>

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

[Patrick Dubroy, 2013-10-21 12:53:32]

https://codereview.chromium.org/28713002/diff/140001/chrome/app/generated_res...
File chrome/app/generated_resources.grd (right):

https://codereview.chromium.org/28713002/diff/140001/chrome/app/generated_res...
chrome/app/generated_resources.grd:7538: +          <ph
name="PRODUCT_NAME">$1<ex>Google Chrome</ex></ph> is trying to show passwords.
On 2013/10/18 15:24:01, Patrick Dubroy wrote:
> On 2013/10/18 14:47:57, Nico wrote:
> > Please read "[chromium-dev] PRODUCT_NAME placeholders in strings considered
> > harmful"
> 
> Ok, I will fix this up.
> 

Done.

https://codereview.chromium.org/28713002/diff/140001/chrome/browser/about_fla...
File chrome/browser/about_flags.cc (right):

https://codereview.chromium.org/28713002/diff/140001/chrome/browser/about_fla...
chrome/browser/about_flags.cc:1268: ENABLE_DISABLE_VALUE_TYPE(
On 2013/10/18 15:24:01, Patrick Dubroy wrote:
> On 2013/10/18 14:47:57, Nico wrote:
> > Isn't a SINGLE_VALUE_TYPE for the enable flag enough? Why do you need a
> disable
> > flag?
> 
> I've done this for features before with the intention of minimizing the code
> churn to turn a feature on or off by default. This way turning it on by
default
> is a 1-line CL, and it's simpler and safer to revert if we change our minds
> later.
> 
> If you think it's not a good idea, I can just add the enable flag.

Done.

[Patrick Dubroy, 2013-10-22 15:36:04]

Ping

[Nico, 2013-10-22 15:39:25]

On 2013/10/22 15:36:04, Patrick Dubroy wrote:
> Ping

gcasto: ^

[Garrett Casto, 2013-10-22 18:34:20]

lgtm

[Nico, 2013-10-22 19:06:33]

lgtm

[Will Harris, 2013-10-22 19:24:38]

lgtm

[commit-bot: I haz the power, 2013-10-23 15:43:26]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/dubroy@chromium.org/28713002/680001

[commit-bot: I haz the power, 2013-10-23 18:40:53]

Change committed as 230479