Fix to allow the proxy authentication dialog to appear when hosting gaia in webview.

Fix to allow the proxy authentication dialog to appear when hosting gaia in webview.

BUG=452452
Committed: https://crrev.com/830f9093200af7528fb0930ab823f247e539ac40
Cr-Commit-Position: refs/heads/master@{#326936}

[paulmeyer, 2015-04-20 19:00:33]

paulmeyer@chromium.org changed reviewers:
+ fsamuel@chromium.org

[paulmeyer, 2015-04-20 19:00:34]

+fsamuel@

[engedy, 2015-04-21 14:34:21]

engedy@chromium.org changed reviewers:
+ engedy@chromium.org, vabr@chromium.org

[engedy, 2015-04-21 14:35:45]

On 2015/04/20 19:00:34, Paul Meyer wrote:
> +fsamuel@

Adding vabr@ and myself as reviewers. As discussed, vabr@ will be back by
Thursday, and then we can take a look to make sure we are not running into the
same crashers as with https://codereview.chromium.org/866523002/.

[vabr (Chromium), 2015-04-22 09:53:56]

Thanks for looping me in, Balazs.

Paul, could you please check by running, that this code does not introduce
crashers as those referenced by Balazs? You seem to be adding all the components
need, but just to be sure.

A separate issue -- while I understand the motivation for adding this into
webview for the sake of proxy auth, the code does not enforce this restriction.
Could we make sure that this won't fire inside forms unexpectedly in WebUI?
(There might be currently no forms like that in WebUI, but I still would prefer
whitelisting case-by-case over blacklisting after an issue appears.)

Cheers,
Vaclav

https://codereview.chromium.org/1093153002/diff/20001/chrome/browser/guest_vi...
File chrome/browser/guest_view/web_view/chrome_web_view_guest_delegate.cc
(right):

https://codereview.chromium.org/1093153002/diff/20001/chrome/browser/guest_vi...
chrome/browser/guest_view/web_view/chrome_web_view_guest_delegate.cc:111: //
Autofill is currently only be enabled for webview in WebUI.
typo: Remove "be".

[paulmeyer, 2015-04-22 17:21:06]

https://codereview.chromium.org/1093153002/diff/20001/chrome/browser/guest_vi...
File chrome/browser/guest_view/web_view/chrome_web_view_guest_delegate.cc
(right):

https://codereview.chromium.org/1093153002/diff/20001/chrome/browser/guest_vi...
chrome/browser/guest_view/web_view/chrome_web_view_guest_delegate.cc:111: //
Autofill is currently only be enabled for webview in WebUI.
On 2015/04/22 09:53:56, vabr (back by 23 April) wrote:
> typo: Remove "be".

Whoops, I changed "will" to "is currently" and broke the sentence. Done.

[paulmeyer, 2015-04-22 17:27:18]

On 2015/04/22 09:53:56, vabr (back by 23 April) wrote:
> Thanks for looping me in, Balazs.
> 
> Paul, could you please check by running, that this code does not introduce
> crashers as those referenced by Balazs? You seem to be adding all the
components
> need, but just to be sure.
> 
> A separate issue -- while I understand the motivation for adding this into
> webview for the sake of proxy auth, the code does not enforce this
restriction.
> Could we make sure that this won't fire inside forms unexpectedly in WebUI?
> (There might be currently no forms like that in WebUI, but I still would
prefer
> whitelisting case-by-case over blacklisting after an issue appears.)
> 
> Cheers,
> Vaclav
> 
>
https://codereview.chromium.org/1093153002/diff/20001/chrome/browser/guest_vi...
> File chrome/browser/guest_view/web_view/chrome_web_view_guest_delegate.cc
> (right):
> 
>
https://codereview.chromium.org/1093153002/diff/20001/chrome/browser/guest_vi...
> chrome/browser/guest_view/web_view/chrome_web_view_guest_delegate.cc:111: //
> Autofill is currently only be enabled for webview in WebUI.
> typo: Remove "be".

I have manually tested for the crashes that were happening with the patch by
noms@, and they are fixed.

Regarding your suggestion to restrict these changes even further, I don't think
that that is a good idea. The scope is already restricted very tightly to only
WebUI, but we do actually want this for all WebUI (and in the future, eventually
all of webview). If you are just worried about potential issues popping up, I
can assure you that since I am a full-time member of the <webview> team, I will
continue to personally oversee these changes and any bugs related to them. If
any issues do appear, I will available to handle them right away.

[Fady Samuel, 2015-04-23 03:03:29]

https://codereview.chromium.org/1093153002/diff/40001/chrome/browser/guest_vi...
File chrome/browser/guest_view/web_view/chrome_web_view_guest_delegate.cc
(right):

https://codereview.chromium.org/1093153002/diff/40001/chrome/browser/guest_vi...
chrome/browser/guest_view/web_view/chrome_web_view_guest_delegate.cc:112: if
(!web_view_guest_->owner_web_contents()->GetWebUI())
I just realized this isn't a good way to determine you're in WebUI. This is not
nullptr in Chrome Apps too. See IsOwnedByExtension in this file:

https://codereview.chromium.org/1066563006/patch/120001/130016

[vabr (Chromium), 2015-04-23 09:45:05]

Thanks, Paul,
For your answers. And also thanks for your testing, much appreciated!

I understand that the future plans to use password manager in webview go beyond
the current CL, but I still prefer making sure that the CL corresponds to the
current plan. In each step we should make it precise, what is being supported,
and document why. It does not mean that that cannot change later.

The discrepancy I see now is that the CL description says this should allow the
proxy authentication dialogue to appear, while the CL just initialises the
password manager for certain types of webviews. I must admit that I still not
understand enough:
(1) How is actually displaying the proxy auth dialogue tied to password manager?
I would expect the proxy auth would be shown, only not filled, if the password
manager is missing. Is that not true?
(2) Depending on (1), it needs to be clarified, what is the expected use of the
password manager, and we should be adjusting the restriction accordingly. There
are two concerns connected to allowing the password manager in webview: for
WebUI, the forms might be rather sensitive (e.g., re-auth for sync, etc.), and
it is not sure that we want the password manager to be active on those. That is
why we need to do whitelisting instead of blacklisting -- only making sure that
the manager works where it is certain that it should.
(The other concern is for non-WebUI, where the whole use-case needs to be first
defined, but that is not the purpose of this CL.)

To be more concrete about (2), if it were, e.g., the case, that the password
manager would be used for proxies with HTTPS schemes, we would need to make sure
that PasswordManagerClient::IsPasswordManagementEnabledForCurrentPage checks
that the manager, even if present, does not trigger on protected schemes like
chrome://.

Could you please help me to understand more about (1)?

Thanks,
Vaclav

[paulmeyer, 2015-04-23 14:09:15]

On 2015/04/23 09:45:05, vabr (Chromium) wrote:
> Thanks, Paul,
> For your answers. And also thanks for your testing, much appreciated!
> 
> I understand that the future plans to use password manager in webview go
beyond
> the current CL, but I still prefer making sure that the CL corresponds to the
> current plan. In each step we should make it precise, what is being supported,
> and document why. It does not mean that that cannot change later.
> 
> The discrepancy I see now is that the CL description says this should allow
the
> proxy authentication dialogue to appear, while the CL just initialises the
> password manager for certain types of webviews. I must admit that I still not
> understand enough:
> (1) How is actually displaying the proxy auth dialogue tied to password
manager?
> I would expect the proxy auth would be shown, only not filled, if the password
> manager is missing. Is that not true?
> (2) Depending on (1), it needs to be clarified, what is the expected use of
the
> password manager, and we should be adjusting the restriction accordingly.
There
> are two concerns connected to allowing the password manager in webview: for
> WebUI, the forms might be rather sensitive (e.g., re-auth for sync, etc.), and
> it is not sure that we want the password manager to be active on those. That
is
> why we need to do whitelisting instead of blacklisting -- only making sure
that
> the manager works where it is certain that it should.
> (The other concern is for non-WebUI, where the whole use-case needs to be
first
> defined, but that is not the purpose of this CL.)
> 
> To be more concrete about (2), if it were, e.g., the case, that the password
> manager would be used for proxies with HTTPS schemes, we would need to make
sure
> that PasswordManagerClient::IsPasswordManagementEnabledForCurrentPage checks
> that the manager, even if present, does not trigger on protected schemes like
> chrome://.
> 
> Could you please help me to understand more about (1)?
> 
> Thanks,
> Vaclav

Regarding (1), the code for the proxy auth dialog is deeply intertwined with the
autofill code, and requires it to work (it will not appear without it). That is
why autofill is needed, and all of the other modules, including the password
manager, are only added because autofill depends on them and there are cases
that result in crashes when autofill is included without the others.

To answer (2), the use of the password manager is just to allow autofill to
work. The main blocker I am trying to address now is just the proxy auth dialog,
but I do also want to get autofill enabled for all webview in WebUI (which right
now is just the Chrome signin page and CromeOS login). Could you please explain
a bit futher what the danger would be to have the password manager active for a
WebUI form with potentially sensitive data?

[vabr (Chromium), 2015-04-23 15:22:36]

Thanks for explaining more, Paul!
My answers inline.

On 2015/04/23 14:09:15, Paul Meyer wrote:
> On 2015/04/23 09:45:05, vabr (Chromium) wrote:
> > Thanks, Paul,
> > For your answers. And also thanks for your testing, much appreciated!
> > 
> > I understand that the future plans to use password manager in webview go
> beyond
> > the current CL, but I still prefer making sure that the CL corresponds to
the
> > current plan. In each step we should make it precise, what is being
supported,
> > and document why. It does not mean that that cannot change later.
> > 
> > The discrepancy I see now is that the CL description says this should allow
> the
> > proxy authentication dialogue to appear, while the CL just initialises the
> > password manager for certain types of webviews. I must admit that I still
not
> > understand enough:
> > (1) How is actually displaying the proxy auth dialogue tied to password
> manager?
> > I would expect the proxy auth would be shown, only not filled, if the
password
> > manager is missing. Is that not true?
> > (2) Depending on (1), it needs to be clarified, what is the expected use of
> the
> > password manager, and we should be adjusting the restriction accordingly.
> There
> > are two concerns connected to allowing the password manager in webview: for
> > WebUI, the forms might be rather sensitive (e.g., re-auth for sync, etc.),
and
> > it is not sure that we want the password manager to be active on those. That
> is
> > why we need to do whitelisting instead of blacklisting -- only making sure
> that
> > the manager works where it is certain that it should.
> > (The other concern is for non-WebUI, where the whole use-case needs to be
> first
> > defined, but that is not the purpose of this CL.)
> > 
> > To be more concrete about (2), if it were, e.g., the case, that the password
> > manager would be used for proxies with HTTPS schemes, we would need to make
> sure
> > that PasswordManagerClient::IsPasswordManagementEnabledForCurrentPage checks
> > that the manager, even if present, does not trigger on protected schemes
like
> > chrome://.
> > 
> > Could you please help me to understand more about (1)?
> > 
> > Thanks,
> > Vaclav
> 
> Regarding (1), the code for the proxy auth dialog is deeply intertwined with
the
> autofill code, and requires it to work (it will not appear without it). That
is
> why autofill is needed, and all of the other modules, including the password
> manager, are only added because autofill depends on them and there are cases
> that result in crashes when autofill is included without the others.
This sounds like a bad situation. When you say "code for the proxy auth dialog",
do you mean //chrome/browser/ui/login and related files?
Ideally, if the functions are dependent, neither should be the code. I
understand that this might be too complex a fix to block your current change,
but before accepting a temporary workaround, we should at least have a good and
concrete description of the problem captured as a bug, and add TODOs explaining
the workarounds you are planning to add, with references to the bug.

> 
> To answer (2), the use of the password manager is just to allow autofill to
> work. The main blocker I am trying to address now is just the proxy auth
dialog,
> but I do also want to get autofill enabled for all webview in WebUI (which
right
> now is just the Chrome signin page and CromeOS login). Could you please
explain
> a bit futher what the danger would be to have the password manager active for
a
> WebUI form with potentially sensitive data?
One example of where the password manager must not work is
chrome://chrome-signin. For security reasons, because the sync password protects
all the other passwords, it must not be a part of the password list (that would
turn short-time access to the list of passwords into a permanent one).
I do not have more examples, but neither are there examples where a working
password manager would be useful in WebUI. There are no password forms there
now, and if more are added, they need to be checked first, in case they meet
similar criteria to that in chrome://chrome-signin.

Stepping back -- you are interested in having the password manager enabled for
WebUI, but not in offering any user-visible functionality (saving or filling).
Is that correct? Could we just make sure that
ChromePasswordManagerClient::IsPasswordManagementEnabledForCurrentPage returns
false on the WebUI pages? That would be an acceptable workaround for the issue
with the code dependencies (given that the bug to fixing them is filed and
worked on eventually).

Cheers,
Vaclav

[Nikita (slow), 2015-04-23 16:48:22]

nkostylev@chromium.org changed reviewers:
+ nkostylev@chromium.org

[Nikita (slow), 2015-04-23 16:48:23]

chrome/browser/chromeos/* lgtm

[paulmeyer, 2015-04-23 23:35:27]

https://codereview.chromium.org/1093153002/diff/40001/chrome/browser/guest_vi...
File chrome/browser/guest_view/web_view/chrome_web_view_guest_delegate.cc
(right):

https://codereview.chromium.org/1093153002/diff/40001/chrome/browser/guest_vi...
chrome/browser/guest_view/web_view/chrome_web_view_guest_delegate.cc:112: if
(!web_view_guest_->owner_web_contents()->GetWebUI())
On 2015/04/23 03:03:29, Fady Samuel wrote:
> I just realized this isn't a good way to determine you're in WebUI. This is
not
> nullptr in Chrome Apps too. See IsOwnedByExtension in this file:
> 
> https://codereview.chromium.org/1066563006/patch/120001/130016

Okay, after talking to vabr@, I'm going to restrict these changes to just the
ChromeOS OOBE login case.

[vabr (Chromium), 2015-04-24 15:49:36]

Hi Paul,

After reading the design doc you shared (thanks!), I believe that instead of
adding the password manager, the ShowLoginPrompt method of login_prompt.cc needs
to be changed:

Instead of cancelling the auth in the case of missing password manager, it
should simply call handler->BuildViewForPasswordManager(nullptr, explanation)
and return early.

This way, the dialogue gets shown, but the password manager won't be trying to
fill or save credentials.

Given your design doc so far, I understand that displaying the dialogue is the
goal, not saving or filling the credentials. If you also want to do the latter,
I need a use-case to understand what is the best way out. If I understand
correctly, during OOBE, the user profile is not set up yet, so filling
credentials is not possible (and storing them questionable -- won't the final
profile use a different storage?). Also, in general, webviews should be
separated from the normal browser context, so we would need to define what
actually storing inside a webview means for the enclosing profile. This is
probably way out of scope of this CL, but I'm happy to be your point of contact
if you need to discuss this further.

Cheers,
Vaclav

[paulmeyer, 2015-04-24 16:59:10]

On 2015/04/24 15:49:36, vabr (in transit---no reviews) wrote:
> Hi Paul,
> 
> After reading the design doc you shared (thanks!), I believe that instead of
> adding the password manager, the ShowLoginPrompt method of login_prompt.cc
needs
> to be changed:
> 
> Instead of cancelling the auth in the case of missing password manager, it
> should simply call handler->BuildViewForPasswordManager(nullptr, explanation)
> and return early.
> 
> This way, the dialogue gets shown, but the password manager won't be trying to
> fill or save credentials.
> 
> Given your design doc so far, I understand that displaying the dialogue is the
> goal, not saving or filling the credentials. If you also want to do the
latter,
> I need a use-case to understand what is the best way out. If I understand
> correctly, during OOBE, the user profile is not set up yet, so filling
> credentials is not possible (and storing them questionable -- won't the final
> profile use a different storage?). Also, in general, webviews should be
> separated from the normal browser context, so we would need to define what
> actually storing inside a webview means for the enclosing profile. This is
> probably way out of scope of this CL, but I'm happy to be your point of
contact
> if you need to discuss this further.
> 
> Cheers,
> Vaclav

Hi Vaclav,

I've implemented the new solution as you suggested. I do believe that in the
future we will still want autofill for webview, but that can wait for now. Thank
you for offering to be my point of contact for that initiative.

As for your comment about storing credentials, the behavior that regular login
has right now (I believe), is that the credentials do get stored in the signin
profile (which you're right about being in a different storage location), and
then copied into the user profile after logging in. I'm not sure exactly how
this would work from within a webview, but that can also be handled later.

Let me know if the current patch set (#5) looks good to you.

Thanks,
Paul

[vabr (Chromium), 2015-04-24 17:40:37]

Thanks, Paul.

Patch set 5 LGTM, although I'm unfortunately not an OWNER of login_prompt.cc.

Thanks also for the clarification about the sign-up profile, and for the
heads-up about future plans. I'll be happy to work with you on that.

Cheers,
Vaclav

[paulmeyer, 2015-04-24 17:52:38]

paulmeyer@chromium.org changed reviewers:
+ msw@chromium.org

[paulmeyer, 2015-04-24 17:52:39]

+msw@ for OWNER review of login_prompt.cc

[msw, 2015-04-24 18:03:57]

lgtm

[paulmeyer, 2015-04-24 18:04:36]

The CQ bit was checked by paulmeyer@chromium.org

[paulmeyer, 2015-04-24 18:04:37]

The patchset sent to the CQ was uploaded after l-g-t-m from
nkostylev@chromium.org
Link to the patchset: https://codereview.chromium.org/1093153002/#ps80001
(title: "Totally different solution, as suggested by vabr@.")

[commit-bot: I haz the power, 2015-04-24 18:05:43]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1093153002/80001

[commit-bot: I haz the power, 2015-04-24 19:23:39]

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

[commit-bot: I haz the power, 2015-04-24 19:23:40]

Try jobs failed on following builders:
  linux_chromium_chromeos_rel_ng on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[paulmeyer, 2015-04-24 20:06:01]

On 2015/04/24 19:23:40, I haz the power (commit-bot) wrote:
> Try jobs failed on following builders:
>   linux_chromium_chromeos_rel_ng on tryserver.chromium.linux (JOB_FAILED,
>
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

Patchset #6 adds a check in ShowLoginPrompt() that only shows the prompt without
a password manager for guestview types. This fixes a failing test.

[paulmeyer, 2015-04-24 20:07:50]

The CQ bit was checked by paulmeyer@chromium.org

[paulmeyer, 2015-04-24 20:07:50]

The patchset sent to the CQ was uploaded after l-g-t-m from vabr@chromium.org,
nkostylev@chromium.org, msw@chromium.org
Link to the patchset: https://codereview.chromium.org/1093153002/#ps100001
(title: "Fix for test.")

[commit-bot: I haz the power, 2015-04-24 20:08:03]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1093153002/100001

[commit-bot: I haz the power, 2015-04-24 20:31:09]

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

[commit-bot: I haz the power, 2015-04-24 20:31:10]

Try jobs failed on following builders:
  android_compile_dbg on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/android_compile...)
  linux_android_rel_ng on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_android_r...)

[msw, 2015-04-24 21:45:50]

lgtm with a nit.

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

https://codereview.chromium.org/1093153002/diff/120001/chrome/browser/ui/logi...
chrome/browser/ui/login/login_prompt.cc:484: 
nit: remove extra blank line

[paulmeyer, 2015-04-24 21:53:39]

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

https://codereview.chromium.org/1093153002/diff/120001/chrome/browser/ui/logi...
chrome/browser/ui/login/login_prompt.cc:484: 
On 2015/04/24 21:45:50, msw wrote:
> nit: remove extra blank line

Done.

[paulmeyer, 2015-04-24 21:53:42]

The CQ bit was checked by paulmeyer@chromium.org

[paulmeyer, 2015-04-24 21:53:43]

The patchset sent to the CQ was uploaded after l-g-t-m from vabr@chromium.org,
nkostylev@chromium.org, msw@chromium.org
Link to the patchset: https://codereview.chromium.org/1093153002/#ps140001
(title: "Addressed nit.")

[commit-bot: I haz the power, 2015-04-24 21:55:26]

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

[commit-bot: I haz the power, 2015-04-24 23:52:29]

Committed patchset #8 (id:140001)

[commit-bot: I haz the power, 2015-04-24 23:53:21]

Patchset 8 (id:??) landed as
https://crrev.com/830f9093200af7528fb0930ab823f247e539ac40
Cr-Commit-Position: refs/heads/master@{#326936}