Allow autofill in iframe inside page of same origin

Allow autofill in iframe inside page of same origin

Previously password manager will not autofill in iframe regardless of its
origin. This change will allow autofill in iframes that have the same origin as
the enclosing page.

BUG=345371
Committed: https://crrev.com/4bb54b508f665bfaaa3b0c64ee4cc1d28f138bec
Cr-Commit-Position: refs/heads/master@{#332299}

[xunlu, 2015-05-22 23:22:15]

xunlu@chromium.org changed reviewers:
+ gcasto@chromium.org

[Garrett Casto, 2015-05-26 20:31:38]

Looks really good for a first CL :). Mostly small changes and style nits.

https://codereview.chromium.org/1159513002/diff/1/chrome/browser/password_man...
File chrome/browser/password_manager/password_manager_browsertest.cc (right):

https://codereview.chromium.org/1159513002/diff/1/chrome/browser/password_man...
chrome/browser/password_manager/password_manager_browsertest.cc:295:
scoped_ptr<net::test_server::HttpResponse> HandleTestRequest(
So after thinking about it some more, I don't think that this is necessary. If
we have the content in a file and have the host resolver and the path set
correctly it looks like it should load fine. We shouldn't add this additional
way of loading content to this test unless it's necessary.

https://codereview.chromium.org/1159513002/diff/1/chrome/browser/password_man...
chrome/browser/password_manager/password_manager_browsertest.cc:301: if
(request.relative_url == "/done.html") {
Looks like this could be solved by having the path be completely specified in
the iframe ("/passwords/done.html"), though if we move this content to
"password/cross_domain_iframed_content.html", this should work as is.

https://codereview.chromium.org/1159513002/diff/1/chrome/browser/password_man...
chrome/browser/password_manager/password_manager_browsertest.cc:418: "  var
element;"
This is a counter intuitive, but this line isn't necessary in JavaScript. See
http://stackoverflow.com/questions/500431/what-is-the-scope-of-variables-in-j...

https://codereview.chromium.org/1159513002/diff/1/chrome/browser/password_man...
chrome/browser/password_manager/password_manager_browsertest.cc:438: "  var
element;"
Same here.

https://codereview.chromium.org/1159513002/diff/1/chrome/browser/password_man...
chrome/browser/password_manager/password_manager_browsertest.cc:484: "var
element;"
Same here.

https://codereview.chromium.org/1159513002/diff/1/chrome/browser/password_man...
chrome/browser/password_manager/password_manager_browsertest.cc:2004: "  if
(event.data == 'init_fill') { "
Maybe this message should be "fill_and_submit" since it's not just filling.

https://codereview.chromium.org/1159513002/diff/1/chrome/browser/password_man...
chrome/browser/password_manager/password_manager_browsertest.cc:2018: "  } else
if (event.data == 'check_for_username') {"
These two "check" messages don't look like they are being used.

https://codereview.chromium.org/1159513002/diff/1/chrome/browser/password_man...
chrome/browser/password_manager/password_manager_browsertest.cc:2027: "function
valueCheck(id, expect_value) {"
Looks like these two functions aren't necessary.

https://codereview.chromium.org/1159513002/diff/1/chrome/browser/password_man...
chrome/browser/password_manager/password_manager_browsertest.cc:2061: // Visit
the sign-up form to store a password for autofill later
Nit: The code right after this comment actually sets up the iframe and then
actually navigates to it. It might help to be more specific here that you setup
the iframe first. It might also be nice to explain why you need to do this, as I
don't think it's particularly intuitive.

https://codereview.chromium.org/1159513002/diff/1/chrome/browser/password_man...
chrome/browser/password_manager/password_manager_browsertest.cc:2066:
std::string submit = base::StringPrintf(
Nit: I'd name this string something more like "create_iframe"

https://codereview.chromium.org/1159513002/diff/1/chrome/browser/password_man...
chrome/browser/password_manager/password_manager_browsertest.cc:2072:
ifrm_observer.SetPathToWaitFor("");
This line looks superfluous as you change the string to wait for 3 lines down.

https://codereview.chromium.org/1159513002/diff/1/chrome/browser/password_man...
chrome/browser/password_manager/password_manager_browsertest.cc:2113: // filled
in until a user interact with the form.
This comment looks like it's from the other test.

https://codereview.chromium.org/1159513002/diff/1/chrome/test/data/password/p...
File chrome/test/data/password/password_form_in_crosssite_iframe.html (right):

https://codereview.chromium.org/1159513002/diff/1/chrome/test/data/password/p...
chrome/test/data/password/password_form_in_crosssite_iframe.html:28: <iframe
src="http://randomsite.net/index.html"  id="iframe" name="iframe">
Did you keep this around for a reason or is it just leftover from before
createCrossSiteIframe()? I would probably just remove it.

https://codereview.chromium.org/1159513002/diff/1/components/autofill/content...
File components/autofill/content/renderer/password_autofill_agent.cc (right):

https://codereview.chromium.org/1159513002/diff/1/components/autofill/content...
components/autofill/content/renderer/password_autofill_agent.cc:461: while
(cur_frame->parent() &&
I think that it's slightly simpler to construct this as

while (cur_frame->parent()) {
  cur_frame = cur_frame->parent()
  if (!<origin check>)
    return false
}

https://codereview.chromium.org/1159513002/diff/1/components/autofill/content...
components/autofill/content/renderer/password_autofill_agent.cc:465: if
(!bottom_frame_origin.equals(cur_frame->securityOrigin().toString())) {
Nit: We don't put one braces on one line if statements.

[xunlu, 2015-05-27 22:54:21]

Patchset #2 (id:20001) has been deleted

[xunlu, 2015-05-27 23:01:11]

Patchset #2 (id:2) has been deleted

[xunlu, 2015-05-27 23:03:17]

Comments addressed.

[Garrett Casto, 2015-05-28 18:08:19]

Almost there, just a few minor changes. Note that generally when you respond to
code reviews you respond to the comments so that it's clear how you addressed
them. If you simply accept the comment you can click "Done", or you can respond
if you want clarification or you disagree with the suggestion.

https://codereview.chromium.org/1159513002/diff/50001/chrome/browser/password...
File chrome/browser/password_manager/password_manager_browsertest.cc (right):

https://codereview.chromium.org/1159513002/diff/50001/chrome/browser/password...
chrome/browser/password_manager/password_manager_browsertest.cc:1978:
std::string submit = base::StringPrintf(
Sorry, if this was clear, but the JS function name was descriptive, but the
std::string named "submit" didn't really describe what the JS would do. That is
what I would rename.

https://codereview.chromium.org/1159513002/diff/50001/chrome/browser/password...
chrome/browser/password_manager/password_manager_browsertest.cc:1990: // Store a
password for autofill later
Nit: This isn't a rule, but we generally value consistency and in this file the
comments generally come at the beginning of a block of code, not in the middle.

https://codereview.chromium.org/1159513002/diff/50001/chrome/browser/password...
chrome/browser/password_manager/password_manager_browsertest.cc:2006: 
So we talked about how it's probably okay here not to actually interact with the
page to verify that you can get the data if you actually select the dropdown,
but it would be nice to interact with the frame in the same way that you do
below so that the password would be visible to script if it was filled.

https://codereview.chromium.org/1159513002/diff/50001/chrome/browser/password...
chrome/browser/password_manager/password_manager_browsertest.cc:2007: // Verfiy
username is not autofilled
"Verify"

https://codereview.chromium.org/1159513002/diff/50001/chrome/browser/password...
chrome/browser/password_manager/password_manager_browsertest.cc:2016: // Verfiy
password is not autofilled
"Verify"

https://codereview.chromium.org/1159513002/diff/50001/chrome/browser/password...
chrome/browser/password_manager/password_manager_browsertest.cc:2051: // Verfiy
username is autofilled
"Verify"

https://codereview.chromium.org/1159513002/diff/50001/chrome/browser/password...
chrome/browser/password_manager/password_manager_browsertest.cc:2054: // Verfiy
password is not autofilled
"Verify"

https://codereview.chromium.org/1159513002/diff/50001/chrome/browser/password...
chrome/browser/password_manager/password_manager_browsertest.cc:2079: // Verfiy
username has been autofilled
"Verify"

[xunlu, 2015-05-28 22:25:47]

Patchset #4 (id:90001) has been deleted

[xunlu, 2015-05-28 22:25:54]

Patchset #3 (id:70001) has been deleted

[xunlu, 2015-05-28 22:28:16]

Address comments

https://codereview.chromium.org/1159513002/diff/50001/chrome/browser/password...
File chrome/browser/password_manager/password_manager_browsertest.cc (right):

https://codereview.chromium.org/1159513002/diff/50001/chrome/browser/password...
chrome/browser/password_manager/password_manager_browsertest.cc:1978:
std::string submit = base::StringPrintf(
On 2015/05/28 18:08:18, Garrett Casto wrote:
> Sorry, if this was clear, but the JS function name was descriptive, but the
> std::string named "submit" didn't really describe what the JS would do. That
is
> what I would rename.

Done.

https://codereview.chromium.org/1159513002/diff/50001/chrome/browser/password...
chrome/browser/password_manager/password_manager_browsertest.cc:1990: // Store a
password for autofill later
On 2015/05/28 18:08:18, Garrett Casto wrote:
> Nit: This isn't a rule, but we generally value consistency and in this file
the
> comments generally come at the beginning of a block of code, not in the
middle. 

Done.

https://codereview.chromium.org/1159513002/diff/50001/chrome/browser/password...
chrome/browser/password_manager/password_manager_browsertest.cc:2006: 
On 2015/05/28 18:08:18, Garrett Casto wrote:
> So we talked about how it's probably okay here not to actually interact with
the
> page to verify that you can get the data if you actually select the dropdown,
> but it would be nice to interact with the frame in the same way that you do
> below so that the password would be visible to script if it was filled.

Done.

https://codereview.chromium.org/1159513002/diff/50001/chrome/browser/password...
chrome/browser/password_manager/password_manager_browsertest.cc:2007: // Verfiy
username is not autofilled
On 2015/05/28 18:08:18, Garrett Casto wrote:
> "Verify"

Done.

https://codereview.chromium.org/1159513002/diff/50001/chrome/browser/password...
chrome/browser/password_manager/password_manager_browsertest.cc:2016: // Verfiy
password is not autofilled
On 2015/05/28 18:08:18, Garrett Casto wrote:
> "Verify"

Done.

https://codereview.chromium.org/1159513002/diff/50001/chrome/browser/password...
chrome/browser/password_manager/password_manager_browsertest.cc:2051: // Verfiy
username is autofilled
On 2015/05/28 18:08:18, Garrett Casto wrote:
> "Verify"

Done.

https://codereview.chromium.org/1159513002/diff/50001/chrome/browser/password...
chrome/browser/password_manager/password_manager_browsertest.cc:2054: // Verfiy
password is not autofilled
On 2015/05/28 18:08:18, Garrett Casto wrote:
> "Verify"

Done.

https://codereview.chromium.org/1159513002/diff/50001/chrome/browser/password...
chrome/browser/password_manager/password_manager_browsertest.cc:2079: // Verfiy
username has been autofilled
On 2015/05/28 18:08:18, Garrett Casto wrote:
> "Verify"

Done.

[Garrett Casto, 2015-05-29 20:47:42]

LGTM

[xunlu, 2015-06-01 18:50:29]

The CQ bit was checked by xunlu@chromium.org

[commit-bot: I haz the power, 2015-06-01 18:54:53]

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

[commit-bot: I haz the power, 2015-06-01 20:15:34]

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

[commit-bot: I haz the power, 2015-06-01 20:15:35]

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_...)

[xunlu, 2015-06-01 20:50:18]

Update PasswordAutofillAgentTest.IframeNoFillTest

[Garrett Casto, 2015-06-01 22:48:52]

lgtm

https://codereview.chromium.org/1159513002/diff/130001/chrome/renderer/autofi...
File chrome/renderer/autofill/password_autofill_agent_browsertest.cc (right):

https://codereview.chromium.org/1159513002/diff/130001/chrome/renderer/autofi...
chrome/renderer/autofill/password_autofill_agent_browsertest.cc:821: //
interaction with the form.
So I would just remove this test. We test this same functionality in
password_manager_browsertest now instead.

[xunlu, 2015-06-01 23:41:38]

Remove PasswordAutofillAgentTest.IframeNoFillTest

https://codereview.chromium.org/1159513002/diff/130001/chrome/renderer/autofi...
File chrome/renderer/autofill/password_autofill_agent_browsertest.cc (right):

https://codereview.chromium.org/1159513002/diff/130001/chrome/renderer/autofi...
chrome/renderer/autofill/password_autofill_agent_browsertest.cc:821: //
interaction with the form.
On 2015/06/01 22:48:51, Garrett Casto wrote:
> So I would just remove this test. We test this same functionality in
> password_manager_browsertest now instead.

Done.

[xunlu, 2015-06-01 23:42:11]

The CQ bit was checked by xunlu@chromium.org

[xunlu, 2015-06-01 23:42:11]

The patchset sent to the CQ was uploaded after l-g-t-m from gcasto@chromium.org
Link to the patchset: https://codereview.chromium.org/1159513002/#ps150001
(title: "Remove PasswordAutofillAgentTest.IframeNoFillTest")

[commit-bot: I haz the power, 2015-06-01 23:45:28]

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

[commit-bot: I haz the power, 2015-06-02 01:18:09]

Committed patchset #5 (id:150001)

[commit-bot: I haz the power, 2015-06-02 01:18:52]

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