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

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

This CL depends on 28713002

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

[Will Harris, 2013-10-22 19:31:13]

This is the companion CL to 28713002, and depends on it.  cpu@ for win stuff,
gcastro@ for password_manager stuff.

I investigated how to determine a blank password on Windows.  Turns out the only
way I found is to call undocumented SAM interfaces, and for that you need to be
Admin.  The other way suggested by the Internet was to attempt to change
password from blank to blank, but this seems really hacky.  I am tempted to try
and work out the way Control Panel users widget does it, but this will probably
require significant effort.

In the end, I decided to test blank password if the machine is a workstation
(not on a domain) - the only downside to this is that the failed login count
will increase, so in the unlikely event that account lockout as been set, it
might be possible to lock an account out by repeatedly attempting to unlock
passwords and then failing.  I don't think this is a significant risk.

The other issue is that I haven't tested smart card authentication, for this I'd
like to get a reader and test we can at least fail gracefully in the case the
user is using smart cards.

Comments appreciated on my approach.

[jschuh, 2013-10-22 19:48:59]

If Windows let you check for a blank password without logging some sort of event
it would clearly be a security bug. So, I'm not surprised it doesn't.

As for checking for a blank password, I'll stick with what I said originally:
just don't do it. You're in the corner case of a corner case where the user has
no OS account password, has saved passwords, and actually knows how to find the
interface for viewing them. It's a waste of time to optimize for that case,
particularly when the result is negative (log events of failed auth attempts).

[Will Harris, 2013-10-22 20:08:24]

On 2013/10/22 19:48:59, Justin Schuh wrote:
> If Windows let you check for a blank password without logging some sort of
event
> it would clearly be a security bug. So, I'm not surprised it doesn't.
> 
> As for checking for a blank password, I'll stick with what I said originally:
> just don't do it. You're in the corner case of a corner case where the user
has
> no OS account password, has saved passwords, and actually knows how to find
the
> interface for viewing them. It's a waste of time to optimize for that case,
> particularly when the result is negative (log events of failed auth attempts).

I really don't like the idea of a user who has never had to enter a password
to login to Windows suddenly to have to realise that they have to push enter
on a dialog box, or work out that their password is, in fact, "".  I think not
prompting for password when there is none to prompt for is quite a key
requirement for the implementation here.

I spoke to Nasko about this, and he suggested that I attempt blank password
authentication once, then remember the lastChanged attribute from the SAM
and do not check again until this is updated.   This will mean that the
failed login attempt for a non-domain connected machine with a non-blank
password will only increment once per password change.

Another option would be to check the lockout policy before attempting blank
password, but I would have to investigate whether this requires elevation or
not in the first place.

There is an API to test blank password, because Control Panel does it, but I
haven't managed to find out what it is - the only other API I found was an
undocumented SAM RPC call that I could only get to work as an Administrator.

[jschuh, 2013-10-22 20:20:26]

On 2013/10/22 20:08:24, Will Harris wrote:
> On 2013/10/22 19:48:59, Justin Schuh wrote:
> > If Windows let you check for a blank password without logging some sort of
> event
> > it would clearly be a security bug. So, I'm not surprised it doesn't.
> > 
> > As for checking for a blank password, I'll stick with what I said
originally:
> > just don't do it. You're in the corner case of a corner case where the user
> has
> > no OS account password, has saved passwords, and actually knows how to find
> the
> > interface for viewing them. It's a waste of time to optimize for that case,
> > particularly when the result is negative (log events of failed auth
attempts).
> 
> I really don't like the idea of a user who has never had to enter a password
> to login to Windows suddenly to have to realise that they have to push enter
> on a dialog box, or work out that their password is, in fact, "".  I think not
> prompting for password when there is none to prompt for is quite a key
> requirement for the implementation here.

This really strikes me as a waste of time at this stage. If you're concerned
about it, implement the simple version with an UMA stat to track the blank
password case. That way you can get some actual data to see if there's any point
in burning further cycles on it.

[cpu_(ooo_6.6-7.5), 2013-10-22 21:10:06]

https://codereview.chromium.org/34393007/diff/40001/chrome/browser/password_m...
File chrome/browser/password_manager/password_manager_util_win.cc (right):

https://codereview.chromium.org/34393007/diff/40001/chrome/browser/password_m...
chrome/browser/password_manager/password_manager_util_win.cc:5: #include
<Ntsecapi.h>
it seems we don't capitalize the files that we #include, so to remain
consistent, don't.

https://codereview.chromium.org/34393007/diff/40001/chrome/browser/password_m...
chrome/browser/password_manager/password_manager_util_win.cc:16: 
possibly undefine SECURITY_WIN32 here?

https://codereview.chromium.org/34393007/diff/40001/chrome/browser/password_m...
chrome/browser/password_manager/password_manager_util_win.cc:23: bool
AuthenticateUser() {
can you point me to the caller of this function?

https://codereview.chromium.org/34393007/diff/40001/chrome/browser/password_m...
chrome/browser/password_manager/password_manager_util_win.cc:31:
UTF16ToWide(l10n_util::GetStringUTF16(IDS_PRODUCT_NAME));
indenting is off.

https://codereview.chromium.org/34393007/diff/40001/chrome/browser/password_m...
chrome/browser/password_manager/password_manager_util_win.cc:35: HANDLE hToken =
INVALID_HANDLE_VALUE;
here and elsewhere, do not name variables Microsoft style. Check our style
guide.

https://codereview.chromium.org/34393007/diff/40001/chrome/browser/password_m...
chrome/browser/password_manager/password_manager_util_win.cc:41: // On a domain,
we obtain the User Principle Name
/s/Principal

https://codereview.chromium.org/34393007/diff/40001/chrome/browser/password_m...
chrome/browser/password_manager/password_manager_util_win.cc:57:
LOGON32_LOGON_INTERACTIVE,
see my comment of line 114

https://codereview.chromium.org/34393007/diff/40001/chrome/browser/password_m...
chrome/browser/password_manager/password_manager_util_win.cc:65:
CloseHandle(hToken);
My reading indicates that you are closing INVALID_HANDLE_VALUE here.

https://codereview.chromium.org/34393007/diff/40001/chrome/browser/password_m...
chrome/browser/password_manager/password_manager_util_win.cc:90: // TODO(wfh)
make sure we support smart cards here
here and elsewhere, make sure comments end with a period.

https://codereview.chromium.org/34393007/diff/40001/chrome/browser/password_m...
chrome/browser/password_manager/password_manager_util_win.cc:114:
LOGON32_LOGON_INTERACTIVE,
this is the expensive one, try using the cheap ones which I don't remember which
one is, maybe LOGON32_LOGON_BATCH or or LOGON32_LOGON_NETWORK

https://codereview.chromium.org/34393007/diff/40001/chrome/browser/password_m...
chrome/browser/password_manager/password_manager_util_win.cc:121: if (
GetLastError() == ERROR_ACCOUNT_RESTRICTION &&
No spaces between ( and the following expression.

https://codereview.chromium.org/34393007/diff/40001/chrome/browser/password_m...
chrome/browser/password_manager/password_manager_util_win.cc:131: (retval ==
false && tries < PASSWORD_MANAGER_MAX_PASSWORD_TRIES));
I don't agree with this loop of tries, I think it is very cheap to press the
'show my passwords' button again. Unless is several clicks/gestures away.

It also makes it expensive for people that click the button by accident.

[Will Harris, 2013-10-23 14:57:18]

https://codereview.chromium.org/34393007/diff/40001/chrome/browser/password_m...
File chrome/browser/password_manager/password_manager_util_win.cc (right):

https://codereview.chromium.org/34393007/diff/40001/chrome/browser/password_m...
chrome/browser/password_manager/password_manager_util_win.cc:5: #include
<Ntsecapi.h>
On 2013/10/22 21:10:06, cpu wrote:
> it seems we don't capitalize the files that we #include, so to remain
> consistent, don't.

Done.

https://codereview.chromium.org/34393007/diff/40001/chrome/browser/password_m...
chrome/browser/password_manager/password_manager_util_win.cc:16: 
On 2013/10/22 21:10:06, cpu wrote:
> possibly undefine SECURITY_WIN32 here?

Done.

https://codereview.chromium.org/34393007/diff/40001/chrome/browser/password_m...
chrome/browser/password_manager/password_manager_util_win.cc:23: bool
AuthenticateUser() {
On 2013/10/22 21:10:06, cpu wrote:
> can you point me to the caller of this function?

in CL 28713002 filename password_manager_handler.cc:

https://codereview.chromium.org/28713002/diff/590001/chrome/browser/ui/webui/...

https://codereview.chromium.org/34393007/diff/40001/chrome/browser/password_m...
chrome/browser/password_manager/password_manager_util_win.cc:31:
UTF16ToWide(l10n_util::GetStringUTF16(IDS_PRODUCT_NAME));
On 2013/10/22 21:10:06, cpu wrote:
> indenting is off.

Done.

https://codereview.chromium.org/34393007/diff/40001/chrome/browser/password_m...
chrome/browser/password_manager/password_manager_util_win.cc:35: HANDLE hToken =
INVALID_HANDLE_VALUE;
On 2013/10/22 21:10:06, cpu wrote:
> here and elsewhere, do not name variables Microsoft style. Check our style
> guide.

Done.

https://codereview.chromium.org/34393007/diff/40001/chrome/browser/password_m...
chrome/browser/password_manager/password_manager_util_win.cc:41: // On a domain,
we obtain the User Principle Name
On 2013/10/22 21:10:06, cpu wrote:
> /s/Principal

Done.

https://codereview.chromium.org/34393007/diff/40001/chrome/browser/password_m...
chrome/browser/password_manager/password_manager_util_win.cc:57:
LOGON32_LOGON_INTERACTIVE,
On 2013/10/22 21:10:06, cpu wrote:
> see my comment of line 114

Done.

https://codereview.chromium.org/34393007/diff/40001/chrome/browser/password_m...
chrome/browser/password_manager/password_manager_util_win.cc:65:
CloseHandle(hToken);
On 2013/10/22 21:10:06, cpu wrote:
> My reading indicates that you are closing INVALID_HANDLE_VALUE here.

Done.

https://codereview.chromium.org/34393007/diff/40001/chrome/browser/password_m...
chrome/browser/password_manager/password_manager_util_win.cc:90: // TODO(wfh)
make sure we support smart cards here
On 2013/10/22 21:10:06, cpu wrote:
> here and elsewhere, make sure comments end with a period.

Done.

https://codereview.chromium.org/34393007/diff/40001/chrome/browser/password_m...
chrome/browser/password_manager/password_manager_util_win.cc:114:
LOGON32_LOGON_INTERACTIVE,
On 2013/10/22 21:10:06, cpu wrote:
> this is the expensive one, try using the cheap ones which I don't remember
which
> one is, maybe LOGON32_LOGON_BATCH or or LOGON32_LOGON_NETWORK

changed to LOGON32_LOGON_NETWORK

https://codereview.chromium.org/34393007/diff/40001/chrome/browser/password_m...
chrome/browser/password_manager/password_manager_util_win.cc:121: if (
GetLastError() == ERROR_ACCOUNT_RESTRICTION &&
On 2013/10/22 21:10:06, cpu wrote:
> No spaces between ( and the following expression.

Done.

https://codereview.chromium.org/34393007/diff/40001/chrome/browser/password_m...
chrome/browser/password_manager/password_manager_util_win.cc:131: (retval ==
false && tries < PASSWORD_MANAGER_MAX_PASSWORD_TRIES));
On 2013/10/22 21:10:06, cpu wrote:
> I don't agree with this loop of tries, I think it is very cheap to press the
> 'show my passwords' button again. Unless is several clicks/gestures away.
> 
> It also makes it expensive for people that click the button by accident.

as discussed, this is okay because the user can break out of the loop at any
time
by hitting the cancel button on the authentication dialog

https://codereview.chromium.org/34393007/diff/220001/chrome/app/chromium_stri...
File chrome/app/chromium_strings.grd (left):

https://codereview.chromium.org/34393007/diff/220001/chrome/app/chromium_stri...
chrome/app/chromium_strings.grd:501: <message
name="IDS_PASSWORDS_PAGE_AUTHENTICATION_PROMPT" desc="Text for the dialog box
that prompts the user for their OS account password before revealing plaintext
passwords on the password page.">
Should I make this more explicit that we are looking for Windows logon
credentials (and not, for example, Google credentials)

[cpu_(ooo_6.6-7.5), 2013-10-23 19:54:42]

looking good, the win32 piece at least.

https://codereview.chromium.org/34393007/diff/70001/chrome/browser/about_flag...
File chrome/browser/about_flags.cc (right):

https://codereview.chromium.org/34393007/diff/70001/chrome/browser/about_flag...
chrome/browser/about_flags.cc:1276: kOsMac | kOsWin,
Is this feature only available via about:flags ? 

I don't see why, the windows version is imho non-controversial, I don't see the
downside to just doing it.

https://codereview.chromium.org/34393007/diff/70001/chrome/browser/password_m...
File chrome/browser/password_manager/password_manager_util_win.cc (right):

https://codereview.chromium.org/34393007/diff/70001/chrome/browser/password_m...
chrome/browser/password_manager/password_manager_util_win.cc:45: if
(GetUserNameEx(NameUserPrincipal, username, &username_length)) {
I don't think we need to do SecureZeroMemory for username or displayname. I
understand it does not hurt but it keeps me on my toes when reading the code.

https://codereview.chromium.org/34393007/diff/70001/chrome/browser/password_m...
chrome/browser/password_manager/password_manager_util_win.cc:136:
SecureZeroMemory(password, sizeof(password));
lets move 136 to right after LogonUser  line 117.

[Garrett Casto, 2013-10-23 20:39:33]

https://codereview.chromium.org/34393007/diff/70001/chrome/browser/password_m...
File chrome/browser/password_manager/password_manager_util_win.cc (right):

https://codereview.chromium.org/34393007/diff/70001/chrome/browser/password_m...
chrome/browser/password_manager/password_manager_util_win.cc:21: #define
PASSWORD_MANAGER_MAX_PASSWORD_TRIES 3
Use a static constant here instead
(http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml#Preprocessor_M...)

https://codereview.chromium.org/34393007/diff/70001/chrome/browser/password_m...
chrome/browser/password_manager/password_manager_util_win.cc:48: username_length
= CREDUI_MAX_USERNAME_LENGTH;
Do you actually need this? The API for GetUserNameEx() seems to say that
username_length will only be modified if it returns true
(http://msdn.microsoft.com/en-us/library/windows/desktop/ms724435(v=vs.85).aspx).

https://codereview.chromium.org/34393007/diff/70001/chrome/browser/password_m...
chrome/browser/password_manager/password_manager_util_win.cc:98: // TODO(wfh)
Make sure we support smart cards here.
indentation

https://codereview.chromium.org/34393007/diff/70001/chrome/browser/password_m...
chrome/browser/password_manager/password_manager_util_win.cc:119: retval = true;
indentation

https://codereview.chromium.org/34393007/diff/70001/chrome/browser/password_m...
chrome/browser/password_manager/password_manager_util_win.cc:124: // Password is
blank, so permit.
You have already checked local users for blank passwords so this must only be to
check for users with domain authentication. Is there any reason that this can't
also be checked ahead of time? Something like

if (GetUserNameEx(...)) {
  ...
} else if (!GetUserName(...)) {
  ...
}

// Check for empty password
logon_result = LogonUser(...)

[Will Harris, 2013-10-25 16:01:41]

PTAL

I've added some code to query the SAM for whether the local user has a blank
password or not.  This avoids having to increment the failed login count, and
also solves a few of the other nits.

https://codereview.chromium.org/34393007/diff/70001/chrome/browser/password_m...
File chrome/browser/password_manager/password_manager_util_win.cc (right):

https://codereview.chromium.org/34393007/diff/70001/chrome/browser/password_m...
chrome/browser/password_manager/password_manager_util_win.cc:21: #define
PASSWORD_MANAGER_MAX_PASSWORD_TRIES 3
On 2013/10/23 20:39:33, Garrett Casto wrote:
> Use a static constant here instead
>
(http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml#Preprocessor_M...)

Done.

https://codereview.chromium.org/34393007/diff/70001/chrome/browser/password_m...
chrome/browser/password_manager/password_manager_util_win.cc:45: if
(GetUserNameEx(NameUserPrincipal, username, &username_length)) {
On 2013/10/23 19:54:43, cpu wrote:
> I don't think we need to do SecureZeroMemory for username or displayname. I
> understand it does not hurt but it keeps me on my toes when reading the code.

Done.

https://codereview.chromium.org/34393007/diff/70001/chrome/browser/password_m...
chrome/browser/password_manager/password_manager_util_win.cc:48: username_length
= CREDUI_MAX_USERNAME_LENGTH;
On 2013/10/23 20:39:33, Garrett Casto wrote:
> Do you actually need this? The API for GetUserNameEx() seems to say that
> username_length will only be modified if it returns true
>
(http://msdn.microsoft.com/en-us/library/windows/desktop/ms724435%28v=vs.85%29...).

both APIs have lpnSize as an in/out parameter so we need to reset the parameter
to the length otherwise the call will fail if the length supplied is longer,
unless I'm misunderstanding what you're saying here?

https://codereview.chromium.org/34393007/diff/70001/chrome/browser/password_m...
chrome/browser/password_manager/password_manager_util_win.cc:98: // TODO(wfh)
Make sure we support smart cards here.
On 2013/10/23 20:39:33, Garrett Casto wrote:
> indentation

Done.

https://codereview.chromium.org/34393007/diff/70001/chrome/browser/password_m...
chrome/browser/password_manager/password_manager_util_win.cc:119: retval = true;
On 2013/10/23 20:39:33, Garrett Casto wrote:
> indentation

Done.

https://codereview.chromium.org/34393007/diff/70001/chrome/browser/password_m...
chrome/browser/password_manager/password_manager_util_win.cc:124: // Password is
blank, so permit.
On 2013/10/23 20:39:33, Garrett Casto wrote:
> You have already checked local users for blank passwords so this must only be
to
> check for users with domain authentication. Is there any reason that this
can't
> also be checked ahead of time? Something like
> 
> if (GetUserNameEx(...)) {
>   ...
> } else if (!GetUserName(...)) {
>   ...
> }
> 
> // Check for empty password
> logon_result = LogonUser(...)
> 

I only wanted to perform the blank password check against local SAM credentials
because it's far more less likely in the standalone workstation case that
account lockout restrictions are enabled.   It's also very unlikely that a blank
password is set on a domain account - in fact I'm not sure that's even possible
since a blank domain account password would by default prevent network logins
and all sorts of havoc would ensue.

I now have some code that checks for blank password from the SAM in a superior
way, I'll upload this for review.  Once that code is in, this code will only
remain in case the blank password check reports a false negative for some reason
and to prevent locking users out of their passwords.

https://codereview.chromium.org/34393007/diff/70001/chrome/browser/password_m...
chrome/browser/password_manager/password_manager_util_win.cc:136:
SecureZeroMemory(password, sizeof(password));
On 2013/10/23 19:54:43, cpu wrote:
> lets move 136 to right after LogonUser  line 117.

can't because need wcslen(password)

[cpu_(ooo_6.6-7.5), 2013-10-25 17:04:43]

https://codereview.chromium.org/34393007/diff/380001/chrome/browser/password_...
File chrome/browser/password_manager/password_manager_util_win.cc (right):

https://codereview.chromium.org/34393007/diff/380001/chrome/browser/password_...
chrome/browser/password_manager/password_manager_util_win.cc:76: HMODULE
samlibDll = LoadLibrary(L"samlib.dll");
you are loading the library in the UI thread?

[Will Harris, 2013-10-25 17:15:05]

On 2013/10/25 17:04:43, cpu wrote:
>
https://codereview.chromium.org/34393007/diff/380001/chrome/browser/password_...
> File chrome/browser/password_manager/password_manager_util_win.cc (right):
> 
>
https://codereview.chromium.org/34393007/diff/380001/chrome/browser/password_...
> chrome/browser/password_manager/password_manager_util_win.cc:76: HMODULE
> samlibDll = LoadLibrary(L"samlib.dll");
> you are loading the library in the UI thread?

I'd appreciate some guidance on the best best way to do this - should I call
LoadLibrary in an initialization function in a singleton at browser start up and
then call these functions via that?

[cpu_(ooo_6.6-7.5), 2013-10-26 00:10:50]

we basically try to avoid doing file IO on the UI thread, among the issues
beyond yank, we have a watchdog thread that will kill the process if the UI
thread is stuck for several seconds.

I have looked into the sam code and that is really not documented interfaces. I
mean they are in msdn but they are part of the court mandated forced disclosure
so I really fear that there would be cases where this thing does not behave as
we think.

Are you planning on getting this into m32?  We have just one week.

[Will Harris, 2013-10-26 18:27:20]

On 2013/10/26 00:10:50, cpu wrote:
> we basically try to avoid doing file IO on the UI thread, among the issues
> beyond yank, we have a watchdog thread that will kill the process if the UI
> thread is stuck for several seconds.
> 
> I have looked into the sam code and that is really not documented interfaces.
I
> mean they are in msdn but they are part of the court mandated forced
disclosure
> so I really fear that there would be cases where this thing does not behave as
> we think.
> 
> Are you planning on getting this into m32?  We have just one week.

no, it's been punted to M33.

Both the Mac and the Windows reauthentication dialog are running in the UI
thread, meaning that if the user takes too long to authenticate, I think the
watchdog will detect this as unresponsive.   Rather than trying to do a separate
implementation for win and mac, maybe it's worth pulling the call to
AuthenticateUser into an async call with a callback, to avoid this happening. 
Patrick, what do you think?

Nasko - what do you think about the SAM calls, do you think it's worth the risk
of these APIs changing under us to avoid the call to LoginUser to determine
blank password?  For reference, I have tested this code works successfully on
Vista, Win7, Win8 and Win8.1.  I was going to test XP next week.  I personally
consider it highly unlikely these will change, and if they do, then all that
happens is that the user has to fallback to authenticate with a blank password,
and the code specifically still supports this (by still handling
ERROR_ACCOUNT_RESTRICTION)

[Patrick Dubroy, 2013-10-28 11:56:26]

On 2013/10/26 18:27:20, Will Harris wrote:
> On 2013/10/26 00:10:50, cpu wrote:
> > we basically try to avoid doing file IO on the UI thread, among the issues
> > beyond yank, we have a watchdog thread that will kill the process if the UI
> > thread is stuck for several seconds.
> > 
> > I have looked into the sam code and that is really not documented
interfaces.
> I
> > mean they are in msdn but they are part of the court mandated forced
> disclosure
> > so I really fear that there would be cases where this thing does not behave
as
> > we think.
> > 
> > Are you planning on getting this into m32?  We have just one week.
> 
> no, it's been punted to M33.
> 
> Both the Mac and the Windows reauthentication dialog are running in the UI
> thread, meaning that if the user takes too long to authenticate, I think the
> watchdog will detect this as unresponsive.   Rather than trying to do a
separate
> implementation for win and mac, maybe it's worth pulling the call to
> AuthenticateUser into an async call with a callback, to avoid this happening. 
> Patrick, what do you think?

thakis@ raised a similar question in my code review. At the time, I didn't see
what the issue would be, but after taking a look at some other dialogs, I
realize that maybe we should implement it this way. Would it make sense to
subclass BaseShellDialog?

FWIW, I haven't seen any problems with the watchdog process, even if I leave the
dialog open for a minute or so.

> Nasko - what do you think about the SAM calls, do you think it's worth the
risk
> of these APIs changing under us to avoid the call to LoginUser to determine
> blank password?  For reference, I have tested this code works successfully on
> Vista, Win7, Win8 and Win8.1.  I was going to test XP next week.  I personally
> consider it highly unlikely these will change, and if they do, then all that
> happens is that the user has to fallback to authenticate with a blank
password,
> and the code specifically still supports this (by still handling
> ERROR_ACCOUNT_RESTRICTION)

[nasko, 2013-10-28 15:51:44]

On Sat, Oct 26, 2013 at 11:27 AM, <wfh@chromium.org> wrote:

> On 2013/10/26 00:10:50, cpu wrote:
>
>> we basically try to avoid doing file IO on the UI thread, among the issues
>> beyond yank, we have a watchdog thread that will kill the process if the
>> UI
>> thread is stuck for several seconds.
>>
>
>  I have looked into the sam code and that is really not documented
>> interfaces.
>>
> I
>
>> mean they are in msdn but they are part of the court mandated forced
>>
> disclosure
>
>> so I really fear that there would be cases where this thing does not
>> behave as
>> we think.
>>
>
>  Are you planning on getting this into m32?  We have just one week.
>>
>
> no, it's been punted to M33.
>
> Both the Mac and the Windows reauthentication dialog are running in the UI
> thread, meaning that if the user takes too long to authenticate, I think
> the
> watchdog will detect this as unresponsive.   Rather than trying to do a
> separate
> implementation for win and mac, maybe it's worth pulling the call to
> AuthenticateUser into an async call with a callback, to avoid this
> happening.
> Patrick, what do you think?
>
> Nasko - what do you think about the SAM calls, do you think it's worth the
> risk
> of these APIs changing under us to avoid the call to LoginUser to determine
> blank password?  For reference, I have tested this code works successfully
> on
> Vista, Win7, Win8 and Win8.1.  I was going to test XP next week.  I
> personally
> consider it highly unlikely these will change, and if they do, then all
> that
> happens is that the user has to fallback to authenticate with a blank
> password,
> and the code specifically still supports this (by still handling
> ERROR_ACCOUNT_RESTRICTION)
>

I doubt these APIs will change, but that can be easily monitored with a
tests.
While the documentation is indeed the court mandated one, it is
documentation of what goes on the wire. If this format was to change, there
will be an interoperability problem, so those changes will have to be
documented prior to releasing the new behavior.
If the alternative is to use LogonUser to test for empty password, I'd
prefer going through SAM.


>
>
https://codereview.chromium.**org/34393007/<https://codereview.chromium.org/3...
>

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

[cpu_(ooo_6.6-7.5), 2013-10-28 21:43:42]

My point is not that they can change at any moment, these API is disclosed but
is not something that they need to keep stable or that XP SP2 has the exact
behavior. 

Basically we are choosing to enter in eternal vigilance mode. Which we have in
places but they are high value. This is very much a corner case.

I am not sure how the 'bits on the wire' inter op argument applies here we are
not constructing raw buffers afaik.

What I rather do is to just remember that the user has a blank password and
don't show auth dialog again. We can do so via what comes from the credUI dialog
or via the LogonUser.

[Patrick Dubroy, 2013-11-06 14:37:55]

https://codereview.chromium.org/34393007/diff/380001/chrome/browser/password_...
File chrome/browser/password_manager/password_manager_util_stub.cc (left):

https://codereview.chromium.org/34393007/diff/380001/chrome/browser/password_...
chrome/browser/password_manager/password_manager_util_stub.cc:1: // Copyright
2013 The Chromium Authors. All rights reserved.
Why are you deleting this file? Isn't it required on platforms that aren't
win/mac/linux?

[Patrick Dubroy, 2013-11-12 15:41:38]

https://codereview.chromium.org/34393007/diff/600001/chrome/browser/password_...
File chrome/browser/password_manager/password_manager_util_win.cc (right):

https://codereview.chromium.org/34393007/diff/600001/chrome/browser/password_...
chrome/browser/password_manager/password_manager_util_win.cc:54: static int64
getPasswordLastChanged(WCHAR* username) {
Nit: Function names should start with a capital letter.

https://codereview.chromium.org/34393007/diff/600001/chrome/browser/password_...
chrome/browser/password_manager/password_manager_util_win.cc:71: static bool
checkBlankPassword(WCHAR* username) {
Nit: Function names should start with a capital letter.

https://codereview.chromium.org/34393007/diff/600001/chrome/browser/password_...
chrome/browser/password_manager/password_manager_util_win.cc:101: if
(logon_result) {
Nit: No braces necessary here.

https://codereview.chromium.org/34393007/diff/600001/chrome/browser/password_...
chrome/browser/password_manager/password_manager_util_win.cc:151: if
(checkBlankPassword(username)) {
Nit: No braces necessary here.

https://codereview.chromium.org/34393007/diff/600001/chrome/browser/password_...
chrome/browser/password_manager/password_manager_util_win.cc:159: if
(GetUserNameEx(NameDisplay, displayname, &username_length)) {
Nit: No braces necessary here.

https://codereview.chromium.org/34393007/diff/600001/chrome/browser/password_...
chrome/browser/password_manager/password_manager_util_win.cc:186: &cui,
This column of args should be indented 4 spaces past "credErr".

[Will Harris, 2013-11-26 22:07:11]

(apologies for slow response, I've been on leave in the Philippines)

cpu: PTAL, I removed all the SAM code, replaced it with a persistent local_state
that determines if the password is blank once per OS password change event.

dubroy: fixed your nits, thanks for the review.

I had to add a new method to allow access to the WebContents to get the HWND
required to correctly modal the OS dialog. Welcome comments on the way I did
this.

https://codereview.chromium.org/34393007/diff/600001/chrome/browser/password_...
File chrome/browser/password_manager/password_manager_util_win.cc (right):

https://codereview.chromium.org/34393007/diff/600001/chrome/browser/password_...
chrome/browser/password_manager/password_manager_util_win.cc:54: static int64
getPasswordLastChanged(WCHAR* username) {
On 2013/11/12 15:41:39, Patrick Dubroy wrote:
> Nit: Function names should start with a capital letter.

Done.

https://codereview.chromium.org/34393007/diff/600001/chrome/browser/password_...
chrome/browser/password_manager/password_manager_util_win.cc:71: static bool
checkBlankPassword(WCHAR* username) {
On 2013/11/12 15:41:39, Patrick Dubroy wrote:
> Nit: Function names should start with a capital letter.

Done.

https://codereview.chromium.org/34393007/diff/600001/chrome/browser/password_...
chrome/browser/password_manager/password_manager_util_win.cc:101: if
(logon_result) {
On 2013/11/12 15:41:39, Patrick Dubroy wrote:
> Nit: No braces necessary here.

Done.

https://codereview.chromium.org/34393007/diff/600001/chrome/browser/password_...
chrome/browser/password_manager/password_manager_util_win.cc:151: if
(checkBlankPassword(username)) {
On 2013/11/12 15:41:39, Patrick Dubroy wrote:
> Nit: No braces necessary here.

Done.

https://codereview.chromium.org/34393007/diff/600001/chrome/browser/password_...
chrome/browser/password_manager/password_manager_util_win.cc:159: if
(GetUserNameEx(NameDisplay, displayname, &username_length)) {
On 2013/11/12 15:41:39, Patrick Dubroy wrote:
> Nit: No braces necessary here.

Done.

https://codereview.chromium.org/34393007/diff/600001/chrome/browser/password_...
chrome/browser/password_manager/password_manager_util_win.cc:186: &cui,
On 2013/11/12 15:41:39, Patrick Dubroy wrote:
> This column of args should be indented 4 spaces past "credErr".

Done.

[cpu_(ooo_6.6-7.5), 2013-11-27 01:06:38]

just two final comments

https://codereview.chromium.org/34393007/diff/690001/chrome/browser/password_...
File chrome/browser/password_manager/password_manager_util_win.cc (right):

https://codereview.chromium.org/34393007/diff/690001/chrome/browser/password_...
chrome/browser/password_manager/password_manager_util_win.cc:105: GetLastError()
== ERROR_ACCOUNT_RESTRICTION);
not sure I agree with the logical statement that ERROR_ACCOUNT_RESTRICTION means
blank password. Why not blank_password == valid handle?

[Will Harris, 2013-11-27 01:21:02]

https://codereview.chromium.org/34393007/diff/690001/chrome/browser/password_...
File chrome/browser/password_manager/password_manager_util_win.cc (right):

https://codereview.chromium.org/34393007/diff/690001/chrome/browser/password_...
chrome/browser/password_manager/password_manager_util_win.cc:105: GetLastError()
== ERROR_ACCOUNT_RESTRICTION);
On 2013/11/27 01:06:39, cpu wrote:
> not sure I agree with the logical statement that ERROR_ACCOUNT_RESTRICTION
means
> blank password. Why not blank_password == valid handle?

in the case the password is blank, then LogonUser returns a failure, handle is
INVALID_HANDLE_VALUE, and GetLastError() is ERROR_ACCOUNT_RESTRICTION.  I'm not
sure of another way to test for this other than the way I use here...   If the
user really did have an account restriction (e.g. logon hours) and a non blank
password then the error would be different.

[Will Harris, 2013-11-27 16:39:01]

add OWNERS for the additional files I had to touch to pass the WebContent
through

Would like to get this pushed this week if possible so we can make the cut for
the dev release next week and on track for M33.

Cheers

[markusheintz_, 2013-11-27 16:44:15]

LGTM chrome/browser/ui/passwords changes

[Patrick Dubroy, 2013-11-27 16:59:17]

https://codereview.chromium.org/34393007/diff/690001/chrome/browser/ui/passwo...
File chrome/browser/ui/passwords/password_ui_view.h (right):

https://codereview.chromium.org/34393007/diff/690001/chrome/browser/ui/passwo...
chrome/browser/ui/passwords/password_ui_view.h:48: virtual content::WebContents*
GetWebContents() = 0;
I don't think this makes sense. This interface is intended to be
platform-independent, and some implementations (e.g. Android) won't have a web
view. Can you ask for the Browser or the native window instead?

[Bernhard Bauer, 2013-11-27 17:04:52]

https://codereview.chromium.org/34393007/diff/690001/chrome/browser/password_...
File chrome/browser/password_manager/password_manager_util_win.cc (right):

https://codereview.chromium.org/34393007/diff/690001/chrome/browser/password_...
chrome/browser/password_manager/password_manager_util_win.cc:23: #include
"chrome/browser/password_manager/password_manager_util.h"
Nit: The header file corresponding to the current implementation file should be
the first include (in this case after the windows headers).

https://codereview.chromium.org/34393007/diff/690001/chrome/browser/ui/webui/...
File chrome/browser/ui/webui/options/password_manager_handler.cc (right):

https://codereview.chromium.org/34393007/diff/690001/chrome/browser/ui/webui/...
chrome/browser/ui/webui/options/password_manager_handler.cc:20: #include
"content/public/browser/web_contents.h"
Also here. I think nothing in this file needs to know the definition of
WebContents.

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

https://codereview.chromium.org/34393007/diff/690001/chrome/browser/ui/webui/...
chrome/browser/ui/webui/options/password_manager_handler.h:15: #include
"content/public/browser/web_contents.h"
Is this include really necessary? password_ui_view.h already forward-declares
WebContents (and a forward-declaration is all you need here).

[cpu_(ooo_6.6-7.5), 2013-11-29 18:51:11]

https://codereview.chromium.org/34393007/diff/690001/chrome/browser/password_...
File chrome/browser/password_manager/password_manager_util_win.cc (right):

https://codereview.chromium.org/34393007/diff/690001/chrome/browser/password_...
chrome/browser/password_manager/password_manager_util_win.cc:105: GetLastError()
== ERROR_ACCOUNT_RESTRICTION);
On 2013/11/27 01:21:03, Will Harris wrote:
> On 2013/11/27 01:06:39, cpu wrote:
> > not sure I agree with the logical statement that ERROR_ACCOUNT_RESTRICTION
> means
> > blank password. Why not blank_password == valid handle?
> 
> in the case the password is blank, then LogonUser returns a failure, handle is
> INVALID_HANDLE_VALUE, and GetLastError() is ERROR_ACCOUNT_RESTRICTION.  I'm
not
> sure of another way to test for this other than the way I use here...   If the
> user really did have an account restriction (e.g. logon hours) and a non blank
> password then the error would be different.

Ok, please add a comment about that, from my reading of msdn that behavior is
not obvious.

[cpu_(ooo_6.6-7.5), 2013-11-29 18:55:35]

https://codereview.chromium.org/34393007/diff/690001/chrome/browser/password_...
File chrome/browser/password_manager/password_manager_util_win.cc (right):

https://codereview.chromium.org/34393007/diff/690001/chrome/browser/password_...
chrome/browser/password_manager/password_manager_util_win.cc:65: 
apparently my second comment got lost, here it is again.

When NetUserGetInfo fails, age is 0 so changed = Now() which seems to me a bug.
Maybe return 0 or -1 for error here, as in "I don't know when the user changed
the password".

[Will Harris, 2013-12-02 23:19:26]

fixed cpu@ and bauerb@ issues so PTAL.

dubroy@ can we chat about the changes to password_ui_view when you're in
tomorrow.  jschuh is eager to get this CL in and then iterate so we can still
hit the M33 timescales.

https://codereview.chromium.org/34393007/diff/690001/chrome/browser/password_...
File chrome/browser/password_manager/password_manager_util_win.cc (right):

https://codereview.chromium.org/34393007/diff/690001/chrome/browser/password_...
chrome/browser/password_manager/password_manager_util_win.cc:23: #include
"chrome/browser/password_manager/password_manager_util.h"
On 2013/11/27 17:04:52, Bernhard Bauer wrote:
> Nit: The header file corresponding to the current implementation file should
be
> the first include (in this case after the windows headers).

Done.

https://codereview.chromium.org/34393007/diff/690001/chrome/browser/password_...
chrome/browser/password_manager/password_manager_util_win.cc:65: 
On 2013/11/29 18:55:36, cpu wrote:
> apparently my second comment got lost, here it is again.
> 
> When NetUserGetInfo fails, age is 0 so changed = Now() which seems to me a
bug.
> Maybe return 0 or -1 for error here, as in "I don't know when the user changed
> the password".
> 

Done.

https://codereview.chromium.org/34393007/diff/690001/chrome/browser/password_...
chrome/browser/password_manager/password_manager_util_win.cc:105: GetLastError()
== ERROR_ACCOUNT_RESTRICTION);
On 2013/11/29 18:51:12, cpu wrote:
> On 2013/11/27 01:21:03, Will Harris wrote:
> > On 2013/11/27 01:06:39, cpu wrote:
> > > not sure I agree with the logical statement that ERROR_ACCOUNT_RESTRICTION
> > means
> > > blank password. Why not blank_password == valid handle?
> > 
> > in the case the password is blank, then LogonUser returns a failure, handle
is
> > INVALID_HANDLE_VALUE, and GetLastError() is ERROR_ACCOUNT_RESTRICTION.  I'm
> not
> > sure of another way to test for this other than the way I use here...   If
the
> > user really did have an account restriction (e.g. logon hours) and a non
blank
> > password then the error would be different.
> 
> Ok, please add a comment about that, from my reading of msdn that behavior is
> not obvious.

Done.

https://codereview.chromium.org/34393007/diff/690001/chrome/browser/ui/webui/...
File chrome/browser/ui/webui/options/password_manager_handler.cc (right):

https://codereview.chromium.org/34393007/diff/690001/chrome/browser/ui/webui/...
chrome/browser/ui/webui/options/password_manager_handler.cc:20: #include
"content/public/browser/web_contents.h"
On 2013/11/27 17:04:52, Bernhard Bauer wrote:
> Also here. I think nothing in this file needs to know the definition of
> WebContents.

Done.

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

https://codereview.chromium.org/34393007/diff/690001/chrome/browser/ui/webui/...
chrome/browser/ui/webui/options/password_manager_handler.h:15: #include
"content/public/browser/web_contents.h"
On 2013/11/27 17:04:52, Bernhard Bauer wrote:
> Is this include really necessary? password_ui_view.h already forward-declares
> WebContents (and a forward-declaration is all you need here).

Done.

[cpu_(ooo_6.6-7.5), 2013-12-03 00:36:43]

lgtm modulo the bug below.

https://codereview.chromium.org/34393007/diff/710001/chrome/browser/password_...
File chrome/browser/password_manager/password_manager_util_win.cc (right):

https://codereview.chromium.org/34393007/diff/710001/chrome/browser/password_...
chrome/browser/password_manager/password_manager_util_win.cc:115: 
Beware that last_changed can be -1 when you reach 118, so you can't += that

[Bernhard Bauer, 2013-12-03 09:34:03]

LGTM with some simplifications in using PrefService:

https://codereview.chromium.org/34393007/diff/730001/chrome/browser/password_...
File chrome/browser/password_manager/password_manager_util_win.cc (right):

https://codereview.chromium.org/34393007/diff/730001/chrome/browser/password_...
chrome/browser/password_manager/password_manager_util_win.cc:86: blank_password
= local_state->GetBoolean(prefs::kOsPasswordBlank);
It's fine to call this method even if the preference is not set. In that case it
will return the registered default, which is false. So you can simplify this to
`bool blank_password = local_state->GetBoolean(...)`. Same for
kOsPasswordLastChanged below.

[Will Harris, 2013-12-03 21:16:56]

https://codereview.chromium.org/34393007/diff/710001/chrome/browser/password_...
File chrome/browser/password_manager/password_manager_util_win.cc (right):

https://codereview.chromium.org/34393007/diff/710001/chrome/browser/password_...
chrome/browser/password_manager/password_manager_util_win.cc:115: 
On 2013/12/03 00:36:44, cpu wrote:
> Beware that last_changed can be -1 when you reach 118, so you can't += that

Done.

https://codereview.chromium.org/34393007/diff/730001/chrome/browser/password_...
File chrome/browser/password_manager/password_manager_util_win.cc (right):

https://codereview.chromium.org/34393007/diff/730001/chrome/browser/password_...
chrome/browser/password_manager/password_manager_util_win.cc:86: blank_password
= local_state->GetBoolean(prefs::kOsPasswordBlank);
On 2013/12/03 09:34:04, Bernhard Bauer wrote:
> It's fine to call this method even if the preference is not set. In that case
it
> will return the registered default, which is false. So you can simplify this
to
> `bool blank_password = local_state->GetBoolean(...)`. Same for
> kOsPasswordLastChanged below.

Done.

[Garrett Casto, 2013-12-03 22:17:53]

LGTM for chrome/password_manager/...

[commit-bot: I haz the power, 2013-12-03 23:32:17]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/wfh@chromium.org/34393007/770001

[commit-bot: I haz the power, 2013-12-04 04:02:05]

Change committed as 238552