Chromium Code Reviews
Help | Chromium Project | Gerrit Changes | Sign in
(590)

Issue 34393007: [Win] Add option to reauthenticate the OS user before revealing passwords. (Closed)

Created:
5 years, 1 month ago by Will Harris
Modified:
5 years ago
CC:
chromium-reviews, DO NOT USE (jschuh), nasko
Base URL:
https://chromium.googlesource.com/chromium/src.git@password
Visibility:
Public.

Description

[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

Patch Set 1 #

Patch Set 2 : lint nits #

Total comments: 24

Patch Set 3 : code review changes #

Patch Set 4 : shuffle headers #

Patch Set 5 : nits #

Total comments: 1

Patch Set 6 : enable reauthentication flag on Windows #

Total comments: 15

Patch Set 7 : rebase since 28713002 got committed #

Patch Set 8 : determine if password is blank using SAM API #

Total comments: 2

Patch Set 9 : Pass the WebContents to AuthenticateUser so OS dialog can be correctly modal #

Patch Set 10 : remove SAM code. Now checks for blank once and remembers. #

Patch Set 11 : Remove spurious logging code. Add clock skew factor. #

Total comments: 12

Patch Set 12 : rebase. remove one unneeded commit. add support for changes in 39253002 #

Total comments: 13

Patch Set 13 : fix nits. fix error checking on NetUserGetInfo #

Total comments: 2

Patch Set 14 : rebase, and fix issue with last_changed return value #

Total comments: 2

Patch Set 15 : pref optimization. change webcontents to nativewindow #

Patch Set 16 : remove GetNativeWindow from OS_ANDROID #

Unified diffs Side-by-side diffs Delta from patch set Stats (+293 lines, -19 lines) Patch
M build/common.gypi View 1 2 3 4 5 6 7 8 9 10 11 12 13 1 chunk +2 lines, -0 lines 0 comments Download
M chrome/app/chromium_strings.grd View 1 2 3 4 5 6 7 8 9 10 11 1 chunk +5 lines, -0 lines 0 comments Download
M chrome/app/google_chrome_strings.grd View 1 2 3 4 5 6 7 8 9 10 11 12 13 1 chunk +5 lines, -0 lines 0 comments Download
M chrome/browser/about_flags.cc View 1 2 3 4 5 6 7 8 9 10 11 12 13 1 chunk +1 line, -1 line 0 comments Download
M chrome/browser/password_manager/password_manager.h View 1 2 3 4 5 6 7 8 9 2 chunks +4 lines, -1 line 0 comments Download
M chrome/browser/password_manager/password_manager_util.h View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 1 chunk +4 lines, -1 line 0 comments Download
A + chrome/browser/password_manager/password_manager_util_linux.cc View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 1 chunk +3 lines, -1 line 0 comments Download
M chrome/browser/password_manager/password_manager_util_mac.mm View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 1 chunk +1 line, -1 line 0 comments Download
D chrome/browser/password_manager/password_manager_util_stub.cc View 1 2 3 4 5 6 1 chunk +0 lines, -11 lines 0 comments Download
A chrome/browser/password_manager/password_manager_util_win.cc View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 1 chunk +224 lines, -0 lines 0 comments Download
M chrome/browser/prefs/browser_prefs.cc View 1 2 3 4 5 6 7 8 9 10 11 12 13 1 chunk +1 line, -0 lines 0 comments Download
M chrome/browser/ui/passwords/password_manager_presenter.cc View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 2 chunks +5 lines, -1 line 0 comments Download
M chrome/browser/ui/passwords/password_manager_presenter_unittest.cc View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 2 chunks +6 lines, -0 lines 0 comments Download
chrome/browser/ui/passwords/password_ui_view.h View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 2 chunks +5 lines, -0 lines 0 comments Download
chrome/browser/ui/webui/options/password_manager_handler.h View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 1 chunk +3 lines, -1 line 0 comments Download
M chrome/browser/ui/webui/options/password_manager_handler.cc View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 2 chunks +8 lines, -0 lines 0 comments Download
M chrome/chrome_browser.gypi View 1 2 3 4 5 6 7 8 9 10 11 12 13 1 chunk +2 lines, -1 line 0 comments Download
M chrome/common/pref_names.h View 1 2 3 4 5 6 7 8 9 10 11 12 13 1 chunk +5 lines, -0 lines 0 comments Download
M chrome/common/pref_names.cc View 1 2 3 4 5 6 7 8 9 10 11 12 13 1 chunk +9 lines, -0 lines 0 comments Download

Messages

Total messages: 35 (0 generated)
Will Harris
This is the companion CL to 28713002, and depends on it. cpu@ for win stuff, ...
5 years, 1 month ago (2013-10-22 19:31:13 UTC) #1
jschuh
If Windows let you check for a blank password without logging some sort of event ...
5 years, 1 month ago (2013-10-22 19:48:59 UTC) #2
Will Harris
On 2013/10/22 19:48:59, Justin Schuh wrote: > If Windows let you check for a blank ...
5 years, 1 month ago (2013-10-22 20:08:24 UTC) #3
jschuh
On 2013/10/22 20:08:24, Will Harris wrote: > On 2013/10/22 19:48:59, Justin Schuh wrote: > > ...
5 years, 1 month ago (2013-10-22 20:20:26 UTC) #4
cpu_(ooo_6.6-7.5)
https://codereview.chromium.org/34393007/diff/40001/chrome/browser/password_manager/password_manager_util_win.cc File chrome/browser/password_manager/password_manager_util_win.cc (right): https://codereview.chromium.org/34393007/diff/40001/chrome/browser/password_manager/password_manager_util_win.cc#newcode5 chrome/browser/password_manager/password_manager_util_win.cc:5: #include <Ntsecapi.h> it seems we don't capitalize the files ...
5 years, 1 month ago (2013-10-22 21:10:06 UTC) #5
Will Harris
https://codereview.chromium.org/34393007/diff/40001/chrome/browser/password_manager/password_manager_util_win.cc File chrome/browser/password_manager/password_manager_util_win.cc (right): https://codereview.chromium.org/34393007/diff/40001/chrome/browser/password_manager/password_manager_util_win.cc#newcode5 chrome/browser/password_manager/password_manager_util_win.cc:5: #include <Ntsecapi.h> On 2013/10/22 21:10:06, cpu wrote: > it ...
5 years, 1 month ago (2013-10-23 14:57:18 UTC) #6
cpu_(ooo_6.6-7.5)
looking good, the win32 piece at least. https://codereview.chromium.org/34393007/diff/70001/chrome/browser/about_flags.cc File chrome/browser/about_flags.cc (right): https://codereview.chromium.org/34393007/diff/70001/chrome/browser/about_flags.cc#newcode1276 chrome/browser/about_flags.cc:1276: kOsMac | ...
5 years, 1 month ago (2013-10-23 19:54:42 UTC) #7
Garrett Casto
https://codereview.chromium.org/34393007/diff/70001/chrome/browser/password_manager/password_manager_util_win.cc File chrome/browser/password_manager/password_manager_util_win.cc (right): https://codereview.chromium.org/34393007/diff/70001/chrome/browser/password_manager/password_manager_util_win.cc#newcode21 chrome/browser/password_manager/password_manager_util_win.cc:21: #define PASSWORD_MANAGER_MAX_PASSWORD_TRIES 3 Use a static constant here instead ...
5 years, 1 month ago (2013-10-23 20:39:33 UTC) #8
cpu_(ooo_6.6-7.5)
5 years, 1 month ago (2013-10-23 23:46:40 UTC) #9
Will Harris
PTAL I've added some code to query the SAM for whether the local user has ...
5 years, 1 month ago (2013-10-25 16:01:41 UTC) #10
cpu_(ooo_6.6-7.5)
https://codereview.chromium.org/34393007/diff/380001/chrome/browser/password_manager/password_manager_util_win.cc File chrome/browser/password_manager/password_manager_util_win.cc (right): https://codereview.chromium.org/34393007/diff/380001/chrome/browser/password_manager/password_manager_util_win.cc#newcode76 chrome/browser/password_manager/password_manager_util_win.cc:76: HMODULE samlibDll = LoadLibrary(L"samlib.dll"); you are loading the library ...
5 years, 1 month ago (2013-10-25 17:04:43 UTC) #11
Will Harris
On 2013/10/25 17:04:43, cpu wrote: > https://codereview.chromium.org/34393007/diff/380001/chrome/browser/password_manager/password_manager_util_win.cc > File chrome/browser/password_manager/password_manager_util_win.cc (right): > > https://codereview.chromium.org/34393007/diff/380001/chrome/browser/password_manager/password_manager_util_win.cc#newcode76 > ...
5 years, 1 month ago (2013-10-25 17:15:05 UTC) #12
cpu_(ooo_6.6-7.5)
we basically try to avoid doing file IO on the UI thread, among the issues ...
5 years, 1 month ago (2013-10-26 00:10:50 UTC) #13
Will Harris
On 2013/10/26 00:10:50, cpu wrote: > we basically try to avoid doing file IO on ...
5 years, 1 month ago (2013-10-26 18:27:20 UTC) #14
Patrick Dubroy
On 2013/10/26 18:27:20, Will Harris wrote: > On 2013/10/26 00:10:50, cpu wrote: > > we ...
5 years, 1 month ago (2013-10-28 11:56:26 UTC) #15
nasko
On Sat, Oct 26, 2013 at 11:27 AM, <wfh@chromium.org> wrote: > On 2013/10/26 00:10:50, cpu ...
5 years, 1 month ago (2013-10-28 15:51:44 UTC) #16
cpu_(ooo_6.6-7.5)
My point is not that they can change at any moment, these API is disclosed ...
5 years, 1 month ago (2013-10-28 21:43:42 UTC) #17
Patrick Dubroy
https://codereview.chromium.org/34393007/diff/380001/chrome/browser/password_manager/password_manager_util_stub.cc File chrome/browser/password_manager/password_manager_util_stub.cc (left): https://codereview.chromium.org/34393007/diff/380001/chrome/browser/password_manager/password_manager_util_stub.cc#oldcode1 chrome/browser/password_manager/password_manager_util_stub.cc:1: // Copyright 2013 The Chromium Authors. All rights reserved. ...
5 years, 1 month ago (2013-11-06 14:37:55 UTC) #18
Patrick Dubroy
https://codereview.chromium.org/34393007/diff/600001/chrome/browser/password_manager/password_manager_util_win.cc File chrome/browser/password_manager/password_manager_util_win.cc (right): https://codereview.chromium.org/34393007/diff/600001/chrome/browser/password_manager/password_manager_util_win.cc#newcode54 chrome/browser/password_manager/password_manager_util_win.cc:54: static int64 getPasswordLastChanged(WCHAR* username) { Nit: Function names should ...
5 years, 1 month ago (2013-11-12 15:41:38 UTC) #19
Will Harris
(apologies for slow response, I've been on leave in the Philippines) cpu: PTAL, I removed ...
5 years ago (2013-11-26 22:07:11 UTC) #20
cpu_(ooo_6.6-7.5)
just two final comments https://codereview.chromium.org/34393007/diff/690001/chrome/browser/password_manager/password_manager_util_win.cc File chrome/browser/password_manager/password_manager_util_win.cc (right): https://codereview.chromium.org/34393007/diff/690001/chrome/browser/password_manager/password_manager_util_win.cc#newcode105 chrome/browser/password_manager/password_manager_util_win.cc:105: GetLastError() == ERROR_ACCOUNT_RESTRICTION); not sure ...
5 years ago (2013-11-27 01:06:38 UTC) #21
Will Harris
https://codereview.chromium.org/34393007/diff/690001/chrome/browser/password_manager/password_manager_util_win.cc File chrome/browser/password_manager/password_manager_util_win.cc (right): https://codereview.chromium.org/34393007/diff/690001/chrome/browser/password_manager/password_manager_util_win.cc#newcode105 chrome/browser/password_manager/password_manager_util_win.cc:105: GetLastError() == ERROR_ACCOUNT_RESTRICTION); On 2013/11/27 01:06:39, cpu wrote: > ...
5 years ago (2013-11-27 01:21:02 UTC) #22
Will Harris
add OWNERS for the additional files I had to touch to pass the WebContent through ...
5 years ago (2013-11-27 16:39:01 UTC) #23
markusheintz_
LGTM chrome/browser/ui/passwords changes
5 years ago (2013-11-27 16:44:15 UTC) #24
Patrick Dubroy
https://codereview.chromium.org/34393007/diff/690001/chrome/browser/ui/passwords/password_ui_view.h File chrome/browser/ui/passwords/password_ui_view.h (right): https://codereview.chromium.org/34393007/diff/690001/chrome/browser/ui/passwords/password_ui_view.h#newcode48 chrome/browser/ui/passwords/password_ui_view.h:48: virtual content::WebContents* GetWebContents() = 0; I don't think this ...
5 years ago (2013-11-27 16:59:17 UTC) #25
Bernhard Bauer
https://codereview.chromium.org/34393007/diff/690001/chrome/browser/password_manager/password_manager_util_win.cc File chrome/browser/password_manager/password_manager_util_win.cc (right): https://codereview.chromium.org/34393007/diff/690001/chrome/browser/password_manager/password_manager_util_win.cc#newcode23 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 ...
5 years ago (2013-11-27 17:04:52 UTC) #26
cpu_(ooo_6.6-7.5)
https://codereview.chromium.org/34393007/diff/690001/chrome/browser/password_manager/password_manager_util_win.cc File chrome/browser/password_manager/password_manager_util_win.cc (right): https://codereview.chromium.org/34393007/diff/690001/chrome/browser/password_manager/password_manager_util_win.cc#newcode105 chrome/browser/password_manager/password_manager_util_win.cc:105: GetLastError() == ERROR_ACCOUNT_RESTRICTION); On 2013/11/27 01:21:03, Will Harris wrote: ...
5 years ago (2013-11-29 18:51:11 UTC) #27
cpu_(ooo_6.6-7.5)
https://codereview.chromium.org/34393007/diff/690001/chrome/browser/password_manager/password_manager_util_win.cc File chrome/browser/password_manager/password_manager_util_win.cc (right): https://codereview.chromium.org/34393007/diff/690001/chrome/browser/password_manager/password_manager_util_win.cc#newcode65 chrome/browser/password_manager/password_manager_util_win.cc:65: apparently my second comment got lost, here it is ...
5 years ago (2013-11-29 18:55:35 UTC) #28
Will Harris
fixed cpu@ and bauerb@ issues so PTAL. dubroy@ can we chat about the changes to ...
5 years ago (2013-12-02 23:19:26 UTC) #29
cpu_(ooo_6.6-7.5)
lgtm modulo the bug below. https://codereview.chromium.org/34393007/diff/710001/chrome/browser/password_manager/password_manager_util_win.cc File chrome/browser/password_manager/password_manager_util_win.cc (right): https://codereview.chromium.org/34393007/diff/710001/chrome/browser/password_manager/password_manager_util_win.cc#newcode115 chrome/browser/password_manager/password_manager_util_win.cc:115: Beware that last_changed can ...
5 years ago (2013-12-03 00:36:43 UTC) #30
Bernhard Bauer
LGTM with some simplifications in using PrefService: https://codereview.chromium.org/34393007/diff/730001/chrome/browser/password_manager/password_manager_util_win.cc File chrome/browser/password_manager/password_manager_util_win.cc (right): https://codereview.chromium.org/34393007/diff/730001/chrome/browser/password_manager/password_manager_util_win.cc#newcode86 chrome/browser/password_manager/password_manager_util_win.cc:86: blank_password = ...
5 years ago (2013-12-03 09:34:03 UTC) #31
Will Harris
https://codereview.chromium.org/34393007/diff/710001/chrome/browser/password_manager/password_manager_util_win.cc File chrome/browser/password_manager/password_manager_util_win.cc (right): https://codereview.chromium.org/34393007/diff/710001/chrome/browser/password_manager/password_manager_util_win.cc#newcode115 chrome/browser/password_manager/password_manager_util_win.cc:115: On 2013/12/03 00:36:44, cpu wrote: > Beware that last_changed ...
5 years ago (2013-12-03 21:16:56 UTC) #32
Garrett Casto
LGTM for chrome/password_manager/...
5 years ago (2013-12-03 22:17:53 UTC) #33
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/wfh@chromium.org/34393007/770001
5 years ago (2013-12-03 23:32:17 UTC) #34
commit-bot: I haz the power
5 years ago (2013-12-04 04:02:05 UTC) #35
Message was sent while issue was closed.
Change committed as 238552

Powered by Google App Engine
This is Rietveld 408576698