Skip to content

GitLab

T
tangled
Commit cdd46b0b authored by Vadym Doroshenko's avatar Vadym Doroshenko Committed by Commit Bot
Browse files
Options

[Bling Password Manager] Unifying treating of password form action 

Normalizing of password form action is done in 2 different cases with different code:
 1.On Load, in Obj-C code in password_controller.mm
 2.On fill in JS in password_controller.js

That's bad, as any duplicating code, because in some cases it generates different canonical actions (for example when the action is empty). This CL unifying canonical action calculation. As a model for canonical action calculation GetCanonicalActionForForm from Desktop/Android implementation is taken.

Bug: 710438, 708602
Change-Id: I1ea879c60d01cf02bed6ea9715089221971ff4a0
Reviewed-on: https://chromium-review.googlesource.com/596031
Commit-Queue: Vadym Doroshenko <dvadym@chromium.org>
Reviewed-by: default avatarVasilii Sukhanov <vasilii@chromium.org>
Cr-Commit-Position: refs/heads/master@{#491434}
parent 2491c29c
Hide whitespace changes
Inline Side-by-side
Showing with 56 additions and 18 deletions
ios/chrome/browser/passwords/password_controller.mm
View file @ cdd46b0b
...@@ -686,12 +686,8 @@ bool GetPageURLAndCheckTrustLevel(web::WebState* web_state, GURL* page_url) { ...@@ -686,12 +686,8 @@ bool GetPageURLAndCheckTrustLevel(web::WebState* web_state, GURL* page_url) {
form->signon_realm = form->origin.ReplaceComponents(remove_path).spec(); form->signon_realm = form->origin.ReplaceComponents(remove_path).spec();
std::string action; std::string action;
// Not checking the return value, as empty action is valid.
dictionary->GetString("action", &action); dictionary->GetString("action", &action);
GURL actionUrl = action.empty() ? originUrl : originUrl.Resolve(action); form->action = GURL(action);
if (!actionUrl.is_valid())
return NO;
form->action = stripURL(actionUrl);
if (!dictionary->GetString("usernameElement", &form->username_element) || if (!dictionary->GetString("usernameElement", &form->username_element) ||
!dictionary->GetString("usernameValue", &form->username_value) || !dictionary->GetString("usernameValue", &form->username_value) ||
......
ios/chrome/browser/passwords/password_controller_js_unittest.mm
View file @ cdd46b0b
...@@ -209,7 +209,7 @@ TEST_F(PasswordControllerJsTest, ...@@ -209,7 +209,7 @@ TEST_F(PasswordControllerJsTest,
const std::string base_url = BaseUrl(); const std::string base_url = BaseUrl();
NSString* result = [NSString NSString* result = [NSString
stringWithFormat: stringWithFormat:
@"[{\"action\":\"/generic_submit\"," @"[{\"action\":\"https://chromium.test/generic_submit\","
"\"method\":\"post\"," "\"method\":\"post\","
"\"name\":\"login_form\"," "\"name\":\"login_form\","
"\"origin\":\"%s\"," "\"origin\":\"%s\","
...@@ -244,7 +244,7 @@ TEST_F(PasswordControllerJsTest, ...@@ -244,7 +244,7 @@ TEST_F(PasswordControllerJsTest,
const std::string base_url = BaseUrl(); const std::string base_url = BaseUrl();
NSString* result = [NSString NSString* result = [NSString
stringWithFormat: stringWithFormat:
@"[{\"action\":\"/generic_submit\"," @"[{\"action\":\"https://chromium.test/generic_submit\","
"\"method\":\"post\"," "\"method\":\"post\","
"\"name\":\"login_form1\"," "\"name\":\"login_form1\","
"\"origin\":\"%s\"," "\"origin\":\"%s\","
...@@ -254,7 +254,7 @@ TEST_F(PasswordControllerJsTest, ...@@ -254,7 +254,7 @@ TEST_F(PasswordControllerJsTest,
"\"usernameElement\":\"name\"," "\"usernameElement\":\"name\","
"\"usernameValue\":\"\"," "\"usernameValue\":\"\","
"\"passwords\":[{\"element\":\"password\",\"value\":\"\"}]}," "\"passwords\":[{\"element\":\"password\",\"value\":\"\"}]},"
"{\"action\":\"/generic_s2\"," "{\"action\":\"https://chromium.test/generic_s2\","
"\"method\":\"post\"," "\"method\":\"post\","
"\"name\":\"login_form2\"," "\"name\":\"login_form2\","
"\"origin\":\"%s\"," "\"origin\":\"%s\","
...@@ -284,7 +284,7 @@ TEST_F(PasswordControllerJsTest, GetPasswordFormData) { ...@@ -284,7 +284,7 @@ TEST_F(PasswordControllerJsTest, GetPasswordFormData) {
NSString* parameter = @"window.document.getElementsByTagName('form')[0]"; NSString* parameter = @"window.document.getElementsByTagName('form')[0]";
NSString* result = [NSString NSString* result = [NSString
stringWithFormat: stringWithFormat:
@"{\"action\":\"/generic_submit\"," @"{\"action\":\"https://chromium.test/generic_submit\","
"\"method\":\"post\"," "\"method\":\"post\","
"\"name\":\"np\"," "\"name\":\"np\","
"\"origin\":\"%s\"," "\"origin\":\"%s\","
...@@ -301,4 +301,34 @@ TEST_F(PasswordControllerJsTest, GetPasswordFormData) { ...@@ -301,4 +301,34 @@ TEST_F(PasswordControllerJsTest, GetPasswordFormData) {
@"__gCrWeb.stringify(__gCrWeb.getPasswordFormData(%@))", parameter)); @"__gCrWeb.stringify(__gCrWeb.getPasswordFormData(%@))", parameter));
}; };
// Check that if a form action is not set then the action is parsed to the
// current url.
TEST_F(PasswordControllerJsTest, FormActionIsNotSet) {
LoadHtmlAndInject(
@"<html><body>"
"<form name='login_form'>"
" Name: <input type='text' name='name'>"
" Password: <input type='password' name='password'>"
" <input type='submit' value='Submit'>"
"</form>"
"</body></html>");
const std::string base_url = BaseUrl();
NSString* result = [NSString
stringWithFormat:
@"[{\"action\":\"https://chromium.test/\","
"\"method\":null,"
"\"name\":\"login_form\","
"\"origin\":\"%s\","
"\"fields\":[{\"element\":\"name\",\"type\":\"text\"},"
"{\"element\":\"password\",\"type\":\"password\"},"
"{\"element\":\"\",\"type\":\"submit\"}],"
"\"usernameElement\":\"name\","
"\"usernameValue\":\"\","
"\"passwords\":[{\"element\":\"password\",\"value\":\"\"}]}]",
base_url.c_str()];
EXPECT_NSEQ(result,
ExecuteJavaScriptWithFormat(@"__gCrWeb.findPasswordForms()"));
};
} // namespace } // namespace
ios/chrome/browser/passwords/password_controller_unittest.mm
View file @ cdd46b0b
...@@ -268,7 +268,7 @@ TEST_F(PasswordControllerTest, PopulatePasswordFormWithDictionary) { ...@@ -268,7 +268,7 @@ TEST_F(PasswordControllerTest, PopulatePasswordFormWithDictionary) {
// to be stripped off. The password is recognized as an old password. // to be stripped off. The password is recognized as an old password.
{ {
"http://john:doe@fakedomain.com/foo/bar?baz=quz#foobar", "http://john:doe@fakedomain.com/foo/bar?baz=quz#foobar",
"{ \"action\": \"some/action?to=be&or=not#tobe\"," "{ \"action\": \"http://fakedomain.com/foo/some/action\","
"\"usernameElement\": \"account\"," "\"usernameElement\": \"account\","
"\"usernameValue\": \"fakeaccount\"," "\"usernameValue\": \"fakeaccount\","
"\"name\": \"signup\"," "\"name\": \"signup\","
...@@ -289,7 +289,7 @@ TEST_F(PasswordControllerTest, PopulatePasswordFormWithDictionary) { ...@@ -289,7 +289,7 @@ TEST_F(PasswordControllerTest, PopulatePasswordFormWithDictionary) {
// due to an origin mismatch. // due to an origin mismatch.
{ {
"http://john:doe@fakedomain.com/foo/bar?baz=quz#foobar", "http://john:doe@fakedomain.com/foo/bar?baz=quz#foobar",
"{ \"action\": \"some/action?to=be&or=not#tobe\"," "{ \"action\": \"\","
"\"usernameElement\": \"account\"," "\"usernameElement\": \"account\","
"\"usernameValue\": \"fakeaccount\"," "\"usernameValue\": \"fakeaccount\","
"\"name\": \"signup\"," "\"name\": \"signup\","
...@@ -334,7 +334,7 @@ TEST_F(PasswordControllerTest, PopulatePasswordFormWithDictionary) { ...@@ -334,7 +334,7 @@ TEST_F(PasswordControllerTest, PopulatePasswordFormWithDictionary) {
// to enter the old password and new password. // to enter the old password and new password.
{ {
"http://fakedomain.com/foo", "http://fakedomain.com/foo",
"{ \"action\": \"\"," "{ \"action\": \"http://fakedomain.com/foo\","
"\"usernameElement\": \"account\"," "\"usernameElement\": \"account\","
"\"usernameValue\": \"fakeaccount\"," "\"usernameValue\": \"fakeaccount\","
"\"name\": \"signup\"," "\"name\": \"signup\","
...@@ -357,7 +357,7 @@ TEST_F(PasswordControllerTest, PopulatePasswordFormWithDictionary) { ...@@ -357,7 +357,7 @@ TEST_F(PasswordControllerTest, PopulatePasswordFormWithDictionary) {
// does not make sense. // does not make sense.
{ {
"http://fakedomain.com", "http://fakedomain.com",
"{ \"action\": \"\"," "{ \"action\": \"http://fakedomain.com/\","
"\"usernameElement\": \"account\"," "\"usernameElement\": \"account\","
"\"usernameValue\": \"fakeaccount\"," "\"usernameValue\": \"fakeaccount\","
"\"name\": \"signup\"," "\"name\": \"signup\","
...@@ -381,7 +381,7 @@ TEST_F(PasswordControllerTest, PopulatePasswordFormWithDictionary) { ...@@ -381,7 +381,7 @@ TEST_F(PasswordControllerTest, PopulatePasswordFormWithDictionary) {
// password is the old one. // password is the old one.
{ {
"http://fakedomain.com", "http://fakedomain.com",
"{ \"action\": \"\"," "{ \"action\": \"http://fakedomain.com/\","
"\"usernameElement\": \"account\"," "\"usernameElement\": \"account\","
"\"usernameValue\": \"fakeaccount\"," "\"usernameValue\": \"fakeaccount\","
"\"name\": \"signup\"," "\"name\": \"signup\","
...@@ -405,7 +405,7 @@ TEST_F(PasswordControllerTest, PopulatePasswordFormWithDictionary) { ...@@ -405,7 +405,7 @@ TEST_F(PasswordControllerTest, PopulatePasswordFormWithDictionary) {
// password is the new one. // password is the new one.
{ {
"http://fakedomain.com", "http://fakedomain.com",
"{ \"action\": \"\"," "{ \"action\": \"http://fakedomain.com/\","
"\"usernameElement\": \"account\"," "\"usernameElement\": \"account\","
"\"usernameValue\": \"fakeaccount\"," "\"usernameValue\": \"fakeaccount\","
"\"name\": \"signup\"," "\"name\": \"signup\","
......
ios/chrome/browser/passwords/resources/password_controller.js
View file @ cdd46b0b
...@@ -79,6 +79,19 @@ if (__gCrWeb && !__gCrWeb['fillPasswordForm']) { ...@@ -79,6 +79,19 @@ if (__gCrWeb && !__gCrWeb['fillPasswordForm']) {
return result; return result;
}; };
/**
* Returns a canonical action for |formElement|. It works the same as upstream
* function GetCanonicalActionForForm.
* @param {Form} formElement.
* @return {string} Canonical action.
*/
var getCanonicalActionForForm_ = function(formElement) {
var raw_action = formElement.getAttribute('action') || "";
var absolute_url = __gCrWeb.common.absoluteURL(
formElement.ownerDocument, raw_action);
return __gCrWeb.common.removeQueryAndReferenceFromURL(absolute_url);
};
/** /**
* Returns the password form with the given |name| as a JSON string. * Returns the password form with the given |name| as a JSON string.
* @param {string} name The name of the form to extract. * @param {string} name The name of the form to extract.
...@@ -111,8 +124,7 @@ if (__gCrWeb && !__gCrWeb['fillPasswordForm']) { ...@@ -111,8 +124,7 @@ if (__gCrWeb && !__gCrWeb['fillPasswordForm']) {
for (var i = 0; i < forms.length; i++) { for (var i = 0; i < forms.length; i++) {
var form = forms[i]; var form = forms[i];
var normalizedFormAction = opt_normalizedAction || var normalizedFormAction = opt_normalizedAction ||
__gCrWeb.common.removeQueryAndReferenceFromURL( getCanonicalActionForForm_(form);
__gCrWeb.common.absoluteURL(doc, form.action));
if (formData.action != normalizedFormAction) { if (formData.action != normalizedFormAction) {
continue; continue;
} }
...@@ -455,7 +467,7 @@ if (__gCrWeb && !__gCrWeb['fillPasswordForm']) { ...@@ -455,7 +467,7 @@ if (__gCrWeb && !__gCrWeb['fillPasswordForm']) {
formElement.ownerDocument.location.href); formElement.ownerDocument.location.href);
return { return {
'action': formElement.getAttribute('action'), 'action': getCanonicalActionForForm_(formElement),
'method': formElement.getAttribute('method'), 'method': formElement.getAttribute('method'),
'name': __gCrWeb.common.getFormIdentifier(formElement), 'name': __gCrWeb.common.getFormIdentifier(formElement),
'origin': origin, 'origin': origin,
......
Markdown is supported
0% or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment