Do not involve PasswordManagerDriver in filling HTTP-auth forms; also check realm

Do not involve PasswordManagerDriver in filling HTTP-auth forms; also check realm

The HTTP authentication is not tied to a renderer and happens completely browser-side.

This CL simplifies the password manager API for filling these forms to avoid referencing the renderer. It also enhances LoginModelObserver to filter irrelevant credentials by realm.

BUG=537823
Committed: https://crrev.com/e0e1e99f39349e912612e58249393f2281de4d00
Cr-Commit-Position: refs/heads/master@{#352816}

[vabr (Chromium), 2015-10-05 15:18:01]

Patchset #1 (id:1) has been deleted

[vabr (Chromium), 2015-10-06 14:06:12]

vabr@chromium.org changed reviewers:
+ davidben@chromium.org, pkasting@chromium.org

[vabr (Chromium), 2015-10-06 14:06:13]

Hi all, please have a look as follows:

pkasting: //chrome/browser/ui/*

davidben: Please review the whole CL.

Thanks!
Vaclav

[vabr (Chromium), 2015-10-06 14:07:12]

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

[commit-bot: I haz the power, 2015-10-06 14:07:47]

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

[vabr (Chromium), 2015-10-06 14:23:21]

vabr@chromium.org changed reviewers:
+ melandory@chromium.org

[vabr (Chromium), 2015-10-06 14:23:22]

Tanja, could you please review the password_manager part?

Cheers,
Vaclav

[melandory, 2015-10-06 14:46:22]

On 2015/10/06 14:23:22, vabr (Chromium) wrote:
> Tanja, could you please review the password_manager part?
> 
> Cheers,
> Vaclav

password_manager piece lgtm.

[commit-bot: I haz the power, 2015-10-06 15:04:16]

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

[commit-bot: I haz the power, 2015-10-06 15:04:17]

Dry run: This issue passed the CQ dry run.

[Peter Kasting, 2015-10-06 22:14:49]

LGTM

https://codereview.chromium.org/1384283003/diff/60001/chrome/browser/ui/login...
File chrome/browser/ui/login/login_prompt.cc (right):

https://codereview.chromium.org/1384283003/diff/60001/chrome/browser/ui/login...
chrome/browser/ui/login/login_prompt.cc:152: return
client->GetPasswordManager();
Nit: Shorter:

  return client ? client->GetPasswordManager() : nullptr;

https://codereview.chromium.org/1384283003/diff/60001/chrome/browser/ui/login...
File chrome/browser/ui/login/login_prompt.h (right):

https://codereview.chromium.org/1384283003/diff/60001/chrome/browser/ui/login...
chrome/browser/ui/login/login_prompt.h:58: const autofill::PasswordForm*
observed_form) = 0;
I can't say I love the pervasive

  DCHECK_EQ(manager == nullptr, observed_form == nullptr);

that this generates.  I wonder if it would make sense to split this into two
functions, one of which takes an explanation, and one of which takes a non-null
manager and observed_form as well.  Maybe not, since most implementations seem
to want to act similarly on both.

Maybe we should just move |manager| to be the second arg (to keep it next to
|observed_form| since they both seem to be optional)?  The DCHECK could also be
shortened:

  DCHECK_EQ(!manager, !observed_form);

...but I'm not sure that's more readable.

[vabr (Chromium), 2015-10-07 09:02:39]

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

[commit-bot: I haz the power, 2015-10-07 09:04:03]

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

[vabr (Chromium), 2015-10-07 10:01:53]

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

[commit-bot: I haz the power, 2015-10-07 10:02:25]

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

[commit-bot: I haz the power, 2015-10-07 10:21:31]

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

[commit-bot: I haz the power, 2015-10-07 10:21:34]

Dry run: This issue passed the CQ dry run.

[vabr (Chromium), 2015-10-07 10:29:23]

Thanks, Peter, for the speedy review!

I addressed your comments. The second one was a bit tricky, so I did my best,
and will be landing this asap (once I check that this change works on iOS as
well). I'm happy to do any follow-ups you request.

David, I won't wait for your review this time, because I have OWNERS coverage,
and you seemed happy in general with my proposal on the bug. Again, I'm happy to
do follow-ups.

Cheers,
Vaclav

https://codereview.chromium.org/1384283003/diff/60001/chrome/browser/ui/login...
File chrome/browser/ui/login/login_prompt.cc (right):

https://codereview.chromium.org/1384283003/diff/60001/chrome/browser/ui/login...
chrome/browser/ui/login/login_prompt.cc:152: return
client->GetPasswordManager();
On 2015/10/06 22:14:49, Peter Kasting wrote:
> Nit: Shorter:
> 
>   return client ? client->GetPasswordManager() : nullptr;

Done.

https://codereview.chromium.org/1384283003/diff/60001/chrome/browser/ui/login...
File chrome/browser/ui/login/login_prompt.h (right):

https://codereview.chromium.org/1384283003/diff/60001/chrome/browser/ui/login...
chrome/browser/ui/login/login_prompt.h:58: const autofill::PasswordForm*
observed_form) = 0;
On 2015/10/06 22:14:49, Peter Kasting wrote:
> I can't say I love the pervasive
> 
>   DCHECK_EQ(manager == nullptr, observed_form == nullptr);
> 
> that this generates.  I wonder if it would make sense to split this into two
> functions, one of which takes an explanation, and one of which takes a
non-null
> manager and observed_form as well.  Maybe not, since most implementations seem
> to want to act similarly on both.
> 
> Maybe we should just move |manager| to be the second arg (to keep it next to
> |observed_form| since they both seem to be optional)?  The DCHECK could also
be
> shortened:
> 
>   DCHECK_EQ(!manager, !observed_form);
> 
> ...but I'm not sure that's more readable.

You raise a good point. I attempted to solve this by bundling the login model
and the form into a struct, to enforce passing both or none.

[vabr (Chromium), 2015-10-07 10:59:31]

The CQ bit was checked by vabr@chromium.org

[vabr (Chromium), 2015-10-07 10:59:32]

The patchset sent to the CQ was uploaded after l-g-t-m from
melandory@chromium.org, pkasting@chromium.org
Link to the patchset: https://codereview.chromium.org/1384283003/#ps140001
(title: "Fix Mac")

[commit-bot: I haz the power, 2015-10-07 11:00:26]

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

[commit-bot: I haz the power, 2015-10-07 11:06:20]

Committed patchset #7 (id:140001)

[commit-bot: I haz the power, 2015-10-07 11:07:17]

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

[Peter Kasting, 2015-10-07 22:27:07]

LGTM, this solution is better than mine were.

https://codereview.chromium.org/1384283003/diff/140001/chrome/browser/ui/logi...
File chrome/browser/ui/login/login_prompt.cc (right):

https://codereview.chromium.org/1384283003/diff/140001/chrome/browser/ui/logi...
chrome/browser/ui/login/login_prompt.cc:286: login_model_->RemoveObserver(this);
Nit: I wonder if this should be a call to ResetModel() instead?

It doesn't matter much for this implementation; the question would be, let's say
someday we want resetting the model to include some more complicated logic or
series of actions.  In that world, would we need to add that code to both these
functions?  If yes, then this should probably just call that.

https://codereview.chromium.org/1384283003/diff/140001/chrome/browser/ui/logi...
File chrome/browser/ui/login/login_prompt.h (right):

https://codereview.chromium.org/1384283003/diff/140001/chrome/browser/ui/logi...
chrome/browser/ui/login/login_prompt.h:112: // Clear |login_model_| and remove
|this| as an observer.
Nit: "Clears", "removes"

[davidben, 2015-10-07 23:13:54]

lgtm and sorry about the delay. Feel free to tackle or not tackle the "Existing
{un,tangentially-}related issue"s as you like. I'll doing a pass over this code
myself now that you've resolved the password manager bits.

(I'm moving the chrome/content boundary to be after the thread hops and surfaced
out of WebContentsDelegate so that this thing needn't mess with threads and IDs
and ref-counts and fun things like that.)

https://codereview.chromium.org/1384283003/diff/140001/chrome/browser/ui/logi...
File chrome/browser/ui/login/login_prompt.cc (right):

https://codereview.chromium.org/1384283003/diff/140001/chrome/browser/ui/logi...
chrome/browser/ui/login/login_prompt.cc:445: LoginHandler* handler) {
(Existing unrelated issue, but this and the two functions below probably want to
be in some anonymous namespace since they're not exported outside this header.)

https://codereview.chromium.org/1384283003/diff/140001/chrome/browser/ui/logi...
chrome/browser/ui/login/login_prompt.cc:470:
handler->SetPasswordForm(dialog_form);
(Existing unrelated issue, but it's pretty weird to have this function bother
return a PasswordForm and also stash it somewhere. Could remove the LoginHandler
parameter so this purely creates a PasswordForm and then move the
SetPasswordForm call to the caller.)

https://codereview.chromium.org/1384283003/diff/140001/chrome/browser/ui/logi...
chrome/browser/ui/login/login_prompt.cc:539: handler->BuildView(explanation,
&model_data);
(Existing tangentially-related issue, but it's pretty weird to have LoginHandler
receive PasswordManager and PasswordForm in two ways, both in
SetPassword{Form,Manager} and as a parameter to BuildView.)

https://codereview.chromium.org/1384283003/diff/140001/chrome/browser/ui/logi...
File chrome/browser/ui/login/login_prompt.h (right):

https://codereview.chromium.org/1384283003/diff/140001/chrome/browser/ui/logi...
chrome/browser/ui/login/login_prompt.h:31: class ContentPasswordManagerDriver;
No longer needed.

https://codereview.chromium.org/1384283003/diff/140001/components/password_ma...
File components/password_manager/core/browser/password_manager.cc (right):

https://codereview.chromium.org/1384283003/diff/140001/components/password_ma...
components/password_manager/core/browser/password_manager.cc:402: const
PasswordForm& observed_form) {
In a vacuum, I think an API like perhaps

  void PasswordManager::GetPreferredCredentials(
      const PasswordForm& observed_form,
      const base::Callback<std::string, std::string>& callback) {
    // Call |callback| if credentials are found.
  }

would be cleaner. (If you get two copies of the same prompt, I think this
observer mechanism will still double-signal. It just won't matter because the
realms will match.) But that seems a much more involved job and I'm not familiar
with this code, so I'll leave that judgement to you.

https://codereview.chromium.org/1384283003/diff/140001/components/password_ma...
components/password_manager/core/browser/password_manager.cc:751:
logger->LogMessage(Logger::STRING_PASSWORDMANAGER_AUTOFILLHTTPAUTH);
[I have no idea what this logger thing is for and will defer to you on whether
havnig a separate one for HTTP is what you want. :-) ]

[vabr (Chromium), 2015-10-08 10:24:12]

Thanks for the additional comments!

I answered here, and uploaded https://codereview.chromium.org/1395473003/ with
the actual code changes.

Cheers,
Vaclav

https://codereview.chromium.org/1384283003/diff/140001/chrome/browser/ui/logi...
File chrome/browser/ui/login/login_prompt.cc (right):

https://codereview.chromium.org/1384283003/diff/140001/chrome/browser/ui/logi...
chrome/browser/ui/login/login_prompt.cc:286: login_model_->RemoveObserver(this);
On 2015/10/07 22:27:07, Peter Kasting wrote:
> Nit: I wonder if this should be a call to ResetModel() instead?
> 
> It doesn't matter much for this implementation; the question would be, let's
say
> someday we want resetting the model to include some more complicated logic or
> series of actions.  In that world, would we need to add that code to both
these
> functions?  If yes, then this should probably just call that.

I was considering this, and did not find compelling reasons to do so. But you
found one, and I agree, so will switch to ResetModel here.
Done.

https://codereview.chromium.org/1384283003/diff/140001/chrome/browser/ui/logi...
chrome/browser/ui/login/login_prompt.cc:445: LoginHandler* handler) {
On 2015/10/07 23:13:54, David Benjamin wrote:
> (Existing unrelated issue, but this and the two functions below probably want
to
> be in some anonymous namespace since they're not exported outside this
header.)

Done.

https://codereview.chromium.org/1384283003/diff/140001/chrome/browser/ui/logi...
chrome/browser/ui/login/login_prompt.cc:470:
handler->SetPasswordForm(dialog_form);
On 2015/10/07 23:13:54, David Benjamin wrote:
> (Existing unrelated issue, but it's pretty weird to have this function bother
> return a PasswordForm and also stash it somewhere. Could remove the
LoginHandler
> parameter so this purely creates a PasswordForm and then move the
> SetPasswordForm call to the caller.)

Done.

https://codereview.chromium.org/1384283003/diff/140001/chrome/browser/ui/logi...
chrome/browser/ui/login/login_prompt.cc:539: handler->BuildView(explanation,
&model_data);
On 2015/10/07 23:13:54, David Benjamin wrote:
> (Existing tangentially-related issue, but it's pretty weird to have
LoginHandler
> receive PasswordManager and PasswordForm in two ways, both in
> SetPassword{Form,Manager} and as a parameter to BuildView.)

Done.

https://codereview.chromium.org/1384283003/diff/140001/chrome/browser/ui/logi...
File chrome/browser/ui/login/login_prompt.h (right):

https://codereview.chromium.org/1384283003/diff/140001/chrome/browser/ui/logi...
chrome/browser/ui/login/login_prompt.h:31: class ContentPasswordManagerDriver;
On 2015/10/07 23:13:54, David Benjamin wrote:
> No longer needed.

Done.

https://codereview.chromium.org/1384283003/diff/140001/chrome/browser/ui/logi...
chrome/browser/ui/login/login_prompt.h:112: // Clear |login_model_| and remove
|this| as an observer.
On 2015/10/07 22:27:07, Peter Kasting wrote:
> Nit: "Clears", "removes"

Done.

https://codereview.chromium.org/1384283003/diff/140001/components/password_ma...
File components/password_manager/core/browser/password_manager.cc (right):

https://codereview.chromium.org/1384283003/diff/140001/components/password_ma...
components/password_manager/core/browser/password_manager.cc:402: const
PasswordForm& observed_form) {
On 2015/10/07 23:13:54, David Benjamin wrote:
> In a vacuum, I think an API like perhaps
> 
>   void PasswordManager::GetPreferredCredentials(
>       const PasswordForm& observed_form,
>       const base::Callback<std::string, std::string>& callback) {
>     // Call |callback| if credentials are found.
>   }
> 
> would be cleaner. (If you get two copies of the same prompt, I think this
> observer mechanism will still double-signal. It just won't matter because the
> realms will match.) But that seems a much more involved job and I'm not
familiar
> with this code, so I'll leave that judgement to you.

I agree with you that what you suggest would be a cleaner approach. On the other
hand, the observer list currently is there to guarantee that observers can get
removed and added any time in a safe way, so to use the callback, we would need
a weak pointer to the observer or something analogous.

The double-signal is not a big issue (not a security one, and performance-wise
we are not too concerned, given the expected number of the observers). We might
have a major redesign of the code during the coming year or so, so at this point
I'd prefer not to invest too much effort in clearing this particular issue. I'm
noting a TODO, nevertheless, so that your observation is kept once we get to the
redesign.

https://codereview.chromium.org/1384283003/diff/140001/components/password_ma...
components/password_manager/core/browser/password_manager.cc:751:
logger->LogMessage(Logger::STRING_PASSWORDMANAGER_AUTOFILLHTTPAUTH);
On 2015/10/07 23:13:54, David Benjamin wrote:
> [I have no idea what this logger thing is for and will defer to you on whether
> havnig a separate one for HTTP is what you want. :-) ]

This is for the chrome://password-manager-internals page. It has been useful to
have the logs indicate the code path taken by signatures of notable methods.
While it would probably be possible to deduce this from the other messages
currently, I don't see a harm in having it explicit.