[Password Manager] Disable password manager for password website reauth

[Password Manager] Disable password manager for password website reauth

Viewing passwords locally requires reauth and users should not be able to
go to a different location to circumvent this restriction.

BUG=406579
Committed: https://crrev.com/af1f8817e683ff672313ce25cb758d0f9c284077
Cr-Commit-Position: refs/heads/master@{#291971}

[Garrett Casto, 2014-08-22 21:42:07]

Mike: Please review
jww: FYI

[jww, 2014-08-22 22:42:10]

lgtm modulo my one drive-by suggestion.

https://codereview.chromium.org/497233003/diff/20001/chrome/browser/password_...
File chrome/browser/password_manager/chrome_password_manager_client.cc (right):

https://codereview.chromium.org/497233003/diff/20001/chrome/browser/password_...
chrome/browser/password_manager/chrome_password_manager_client.cc:415: // "rart"
param signals this page is for transactional reauth.
Seems like this might be worth factoring out as a generic "are we on a
transactional auth" page? Is there a chance we could use it elsewhere in the
future?

[Mike West, 2014-08-24 05:37:57]

One thing I'd suggest changing, and some tests I'd like to see.

https://codereview.chromium.org/497233003/diff/20001/chrome/browser/password_...
File chrome/browser/password_manager/chrome_password_manager_client.cc (right):

https://codereview.chromium.org/497233003/diff/20001/chrome/browser/password_...
chrome/browser/password_manager/chrome_password_manager_client.cc:426: return
GURL(param_value).GetOrigin() ==
If a user changes the value of the continue param to 'http://whatever', we
wouldn't block autofill, but the magic of HSTS pinning would probably log the
user in anyway.

Perhaps we should check against the hostname instead?

https://codereview.chromium.org/497233003/diff/20001/chrome/browser/password_...
File chrome/browser/password_manager/chrome_password_manager_client_unittest.cc
(right):

https://codereview.chromium.org/497233003/diff/20001/chrome/browser/password_...
chrome/browser/password_manager/chrome_password_manager_client_unittest.cc:271:
IsPasswordManagerEnabledForCurrentPage) {
Can you add some clever bypass checks? Things that come to mind are HTTP, as
mentioned in a previous comment, ports, trailing '.', and URL encoding. Most of
this is probably handled out of the box by GURL, but double-checking seems
reasonable.

[Garrett Casto, 2014-08-25 08:16:49]

https://codereview.chromium.org/497233003/diff/20001/chrome/browser/password_...
File chrome/browser/password_manager/chrome_password_manager_client.cc (right):

https://codereview.chromium.org/497233003/diff/20001/chrome/browser/password_...
chrome/browser/password_manager/chrome_password_manager_client.cc:415: // "rart"
param signals this page is for transactional reauth.
On 2014/08/22 22:42:10, jww wrote:
> Seems like this might be worth factoring out as a generic "are we on a
> transactional auth" page? Is there a chance we could use it elsewhere in the
> future?

I'm not sure. I could imagine it possibly going in GaiaUrls() or some nearby
utility function, but I don't know if this sort of inspection is something that
they want to generally allow. I'll ping Roger about it tomorrow.

https://codereview.chromium.org/497233003/diff/20001/chrome/browser/password_...
chrome/browser/password_manager/chrome_password_manager_client.cc:426: return
GURL(param_value).GetOrigin() ==
On 2014/08/24 05:37:57, Mike West (OOO until Sept 1) wrote:
> If a user changes the value of the continue param to 'http://whatever', we
> wouldn't block autofill, but the magic of HSTS pinning would probably log the
> user in anyway.
> 
> Perhaps we should check against the hostname instead?

Ah, interesting. I assumed that the server side checks would also verify that
https was used, but I guess it doesn't hurt to be permissive about that here.
Done.

https://codereview.chromium.org/497233003/diff/20001/chrome/browser/password_...
File chrome/browser/password_manager/chrome_password_manager_client_unittest.cc
(right):

https://codereview.chromium.org/497233003/diff/20001/chrome/browser/password_...
chrome/browser/password_manager/chrome_password_manager_client_unittest.cc:271:
IsPasswordManagerEnabledForCurrentPage) {
On 2014/08/24 05:37:57, Mike West (OOO until Sept 1) wrote:
> Can you add some clever bypass checks? Things that come to mind are HTTP, as
> mentioned in a previous comment, ports, trailing '.', and URL encoding. Most
of
> this is probably handled out of the box by GURL, but double-checking seems
> reasonable.

Added some. Interestingly a trailing dot is considered a different hostname, I
guess because it could conceivably resolve differently. Added a comment about
this in the test, and I'll follow up with the GAIA folks about to see if it's
particularly difficult to verify that these types of URLs are not valid server
side. If it is, we can always add additional validation here.

[Garrett Casto, 2014-08-25 20:26:24]

Hi Roger,

Do you think that there is any desire/reason to pull code that identifies
transactional reauth pages in src/google_apis/gaia? I think that it's unlikely
that anyone else will need this ability, but just wanted to see if you had any
thoughts.

[Garrett Casto, 2014-08-25 20:30:23]

Since Mike went out of town, Ilya can you take a look at the latest version?
Thanks!

[Ilya Sherman, 2014-08-26 01:46:08]

LGTM, thanks.

https://codereview.chromium.org/497233003/diff/40001/chrome/browser/password_...
File chrome/browser/password_manager/chrome_password_manager_client.h (right):

https://codereview.chromium.org/497233003/diff/40001/chrome/browser/password_...
chrome/browser/password_manager/chrome_password_manager_client.h:143: bool
IsURLPasswordWebsiteReauth(GURL url) const;
nit: Pass by const-ref.

https://codereview.chromium.org/497233003/diff/40001/chrome/browser/password_...
File chrome/browser/password_manager/chrome_password_manager_client_unittest.cc
(right):

https://codereview.chromium.org/497233003/diff/40001/chrome/browser/password_...
chrome/browser/password_manager/chrome_password_manager_client_unittest.cc:304:
EXPECT_TRUE(client->IsPasswordManagerEnabledForCurrentPage());
Wat.  Can you check with the GURL maintainers whether this is WAI?

[Garrett Casto, 2014-08-26 18:27:58]

For expediency, I'm going to submit this now. I want this merged and I don't
think that the refactoring issue is particularly pressing.

https://codereview.chromium.org/497233003/diff/40001/chrome/browser/password_...
File chrome/browser/password_manager/chrome_password_manager_client.h (right):

https://codereview.chromium.org/497233003/diff/40001/chrome/browser/password_...
chrome/browser/password_manager/chrome_password_manager_client.h:143: bool
IsURLPasswordWebsiteReauth(GURL url) const;
On 2014/08/26 01:46:08, Ilya Sherman wrote:
> nit: Pass by const-ref.

Done.

https://codereview.chromium.org/497233003/diff/40001/chrome/browser/password_...
File chrome/browser/password_manager/chrome_password_manager_client_unittest.cc
(right):

https://codereview.chromium.org/497233003/diff/40001/chrome/browser/password_...
chrome/browser/password_manager/chrome_password_manager_client_unittest.cc:304:
EXPECT_TRUE(client->IsPasswordManagerEnabledForCurrentPage());
On 2014/08/26 01:46:08, Ilya Sherman wrote:
> Wat.  Can you check with the GURL maintainers whether this is WAI?

Sure, I'll follow up with Brett. My guess is that the issue is that this
particular hostname is functionally identical because "com" is a TLD, but int
the general case these could be different. I also don't know enough about DNS to
know for certain that even these hostnames are always equivalent (could your
local DNS override/intercept certain hostnames?).

[Garrett Casto, 2014-08-26 18:28:09]

The CQ bit was checked by gcasto@chromium.org

[commit-bot: I haz the power, 2014-08-26 18:29:44]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/gcasto@chromium.org/497233003/60001

[Garrett Casto, 2014-08-26 18:41:15]

The CQ bit was checked by gcasto@chromium.org

[commit-bot: I haz the power, 2014-08-26 18:42:29]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/gcasto@chromium.org/497233003/80001

[commit-bot: I haz the power, 2014-08-26 19:50:07]

Committed patchset #5 (80001) as 13d4a561010711d5a0b061a0f1c4fb0e3ede12f9

[commit-bot: I haz the power, 2014-09-10 02:45:16]

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