Save password without an associated username.

chrome/browser/password_manager/password_form_manager_unittest.cc

Index: chrome/browser/password_manager/password_form_manager_unittest.cc
===================================================================
--- chrome/browser/password_manager/password_form_manager_unittest.cc	(revision 129489)
+++ chrome/browser/password_manager/password_form_manager_unittest.cc	(working copy)
@@ -33,10 +33,25 @@
     saved_match_.username_value = ASCIIToUTF16("test@gmail.com");
     saved_match_.password_value = ASCIIToUTF16("test1");
     profile_ = new TestingProfile();
+
+    // A form for testing a case there's no username (there's only password).
+    observed_form2_.origin = GURL("http://www.google.com/b/LoginAuth");
+    observed_form2_.action = GURL("http://www.google.com/b/Login");
+    observed_form2_.password_element = ASCIIToUTF16("Passwd");
+    observed_form2_.submit_element = ASCIIToUTF16("signIn");
+    observed_form2_.signon_realm = "http://www.google.com";
+
+    saved_match2_ = observed_form_;
+    saved_match2_.origin = GURL("http://www.google.com/a/ServiceLoginAuth");
+    saved_match2_.action = GURL("http://www.google.com/a/ServiceLogin");
+    saved_match2_.preferred = true;
+    saved_match2_.password_value = ASCIIToUTF16("test2");
+    profile2_ = new TestingProfile();
   }
 
   virtual void TearDown() {
     delete profile_;
+    delete profile2_;
   }
 
   PasswordForm* GetPendingCredentials(PasswordFormManager* p) {
@@ -60,14 +75,21 @@
   }
 
   Profile* profile() { return profile_; }
+  Profile* profile2() { return profile2_; }
 
   PasswordForm* observed_form() { return &observed_form_; }
   PasswordForm* saved_match() { return &saved_match_; }
+  PasswordForm* observed_form2() { return &observed_form2_; }
+  PasswordForm* saved_match2() { return &saved_match2_; }
 
+
  private:
   PasswordForm observed_form_;
   PasswordForm saved_match_;
   Profile* profile_;
+  PasswordForm observed_form2_;
+  PasswordForm saved_match2_;
+  Profile* profile2_;
 };
 
 TEST_F(PasswordFormManagerTest, TestNewLogin) {
@@ -123,6 +145,60 @@
   delete manager;
 }
 
+TEST_F(PasswordFormManagerTest, TestNewLoginAndUpdateWithoutUsername) {
+  PasswordFormManager* manager = new PasswordFormManager(
+      profile(), NULL, *observed_form2(), false);

[tim (not reviewing), 2012/04/03 16:40:27]

Why do we need observed_form2 It seems tests use either version, so it should be
a way to set this up so that we don't need the extra members or Profile in the
test class, and instead do an extra configuration step in the tests that want
'2'.

Also, this appears to be mostly copy/paste from the test above.  Can you move
the similar code into a function (perhaps parameterized by your form1/2
variables) that is called once with a non-empty username form and once with an
empty username?  This will prevent the tests from diverging in the future.

[Yumikiyo Osanai, 2012/04/05 00:19:50]

I basically agree with your idea.

I've removed the '2' members and added SetUpForNoUsernameTests() function.
And, I've took refactoring the code to delete copy/paste tests as possible.

On 2012/04/03 16:40:27, timsteele wrote:

+  SimulateMatchingPhase(manager, false);
+
+  // User submits credentials for the observed form.
+  PasswordForm credentials = *observed_form2();
+  credentials.username_value = saved_match2()->username_value;
+  credentials.password_value = saved_match2()->password_value;
+  credentials.preferred = true;
+  manager->ProvisionallySave(credentials);
+
+  // Successful login. The PasswordManager would instruct PasswordFormManager
+  // to save, which should know this is a new login.
+  EXPECT_TRUE(manager->IsNewLogin());
+  // Make sure the credentials that would be submitted on successful login
+  // are going to match the stored entry in the db.
+  EXPECT_EQ(observed_form2()->origin.spec(),
+            GetPendingCredentials(manager)->origin.spec());
+  EXPECT_EQ(observed_form2()->signon_realm,
+            GetPendingCredentials(manager)->signon_realm);
+  EXPECT_TRUE(GetPendingCredentials(manager)->preferred);
+  EXPECT_EQ(saved_match2()->password_value,
+            GetPendingCredentials(manager)->password_value);
+  EXPECT_EQ(saved_match2()->username_value,
+            GetPendingCredentials(manager)->username_value);
+
+  // Now, suppose the user re-visits the site and wants to save an additional
+  // login for the site with a new password. In this case, the matching phase
+  // will yield the previously saved login.
+  SimulateMatchingPhase(manager, true);
+
+  // Set up the new login.
+  string16 new_pass = ASCIIToUTF16("newpass2");
+  credentials.password_value = new_pass;
+  manager->ProvisionallySave(credentials);
+
+  // Again, the PasswordFormManager should know this is still a new login.
+  EXPECT_TRUE(manager->IsNewLogin());
+
+  // And make sure everything squares up again.
+  EXPECT_EQ(observed_form2()->origin.spec(),
+            GetPendingCredentials(manager)->origin.spec());
+  EXPECT_EQ(observed_form2()->signon_realm,
+            GetPendingCredentials(manager)->signon_realm);
+  EXPECT_TRUE(GetPendingCredentials(manager)->preferred);
+  EXPECT_EQ(new_pass,
+            GetPendingCredentials(manager)->password_value);
+
+  // Done.
+  delete manager;
+}
+
+
 TEST_F(PasswordFormManagerTest, TestUpdatePassword) {
   // Create a PasswordFormManager with observed_form, as if we just
   // saw this form and need to find matching logins.
@@ -197,6 +273,25 @@
             GetPendingCredentials(manager.get())->action);
 }
 
+TEST_F(PasswordFormManagerTest, TestEmptyActionWithoutUsername) {
+  scoped_ptr<PasswordFormManager> manager(new PasswordFormManager(
+      profile(), NULL, *observed_form2(), false));
+
+  saved_match2()->action = GURL();
+  SimulateMatchingPhase(manager.get(), true);
+  // User logs in with the autofilled username / password from saved_match.
+  PasswordForm login = *observed_form2();
+  login.password_value = saved_match2()->password_value;
+  manager->ProvisionallySave(login);
+
+  // If there's no username, we supporse it's the new login.
+  EXPECT_TRUE(manager->IsNewLogin());
+  // We bless our saved PasswordForm entry with the action URL of the
+  // observed form.
+  EXPECT_EQ(observed_form2()->action,
+            GetPendingCredentials(manager.get())->action);
+}
+
 TEST_F(PasswordFormManagerTest, TestValidForms) {
   // User submits credentials for the observed form.
   PasswordForm credentials = *observed_form();
@@ -210,10 +305,12 @@
   EXPECT_TRUE(manager1.HasValidPasswordForm());
 
   // Form without a username_element but with a password_element.
+  // It should be true, because we should save and autofill a password
+  // if there isn't any username.
   credentials.username_element.clear();
   PasswordFormManager manager2(profile(), NULL, credentials, false);
   SimulateMatchingPhase(&manager2, false);
-  EXPECT_FALSE(manager2.HasValidPasswordForm());
+  EXPECT_TRUE(manager2.HasValidPasswordForm());
 
   // Form without a password_element but with a username_element.
   credentials.username_element = saved_match()->username_element;
@@ -230,6 +327,24 @@
   EXPECT_FALSE(manager4.HasValidPasswordForm());
 }
 
+TEST_F(PasswordFormManagerTest, TestValidFormsWithNoUsername) {
+  // User submits credentials for the observed form.
+  PasswordForm credentials = *observed_form2();
+  credentials.scheme = PasswordForm::SCHEME_HTML;
+  credentials.password_value = saved_match2()->password_value;
+
+  // Form with a password_element.
+  PasswordFormManager manager1(profile2(), NULL, credentials, false);
+  SimulateMatchingPhase(&manager1, false);
+  EXPECT_TRUE(manager1.HasValidPasswordForm());
+
+  // Form with no a password_element
+  credentials.password_element.clear();
+  PasswordFormManager manager2(profile2(), NULL, credentials, false);
+  SimulateMatchingPhase(&manager2, false);
+  EXPECT_FALSE(manager2.HasValidPasswordForm());
+}
+
 TEST_F(PasswordFormManagerTest, TestValidFormsBasic) {
   // User submits credentials for the observed form.
   PasswordForm credentials = *observed_form();
@@ -262,3 +377,22 @@
   SimulateMatchingPhase(&manager4, false);
   EXPECT_TRUE(manager4.HasValidPasswordForm());
 }
+
+TEST_F(PasswordFormManagerTest, TestValidFormsBasicWithoutUsername) {
+  // User submits credentials for the observed form.
+  PasswordForm credentials = *observed_form2();
+  credentials.scheme = PasswordForm::SCHEME_BASIC;
+  credentials.password_value = saved_match2()->password_value;
+
+  // Form with a password_element.
+  PasswordFormManager manager1(profile2(), NULL, credentials, false);
+  SimulateMatchingPhase(&manager1, false);
+  EXPECT_TRUE(manager1.HasValidPasswordForm());
+
+  // Form with no a password_element
+  credentials.password_element.clear();
+  PasswordFormManager manager2(profile2(), NULL, credentials, false);
+  SimulateMatchingPhase(&manager2, false);
+  EXPECT_TRUE(manager2.HasValidPasswordForm());
+}

[tim (not reviewing), 2012/04/03 16:40:27]

What happens if
- a user saves a password for a form with no username element on foo.com
- the user visits foo.com/a, which has a password form *with* [and next,
without] a username element. 

[Yumikiyo Osanai, 2012/04/05 00:19:50]

Umm... I don't understand what you want to test...
Please write the detail.

And I've tried to create the test as PasswordFormManagerTest(), but I haven't
found the way to express "visit foo.com/a".
I know manager->ProvisionallySave() can save the password, but I haven't found
the funciton that offers "visit a URL".
Please see the  TEST_F(PasswordFormManagerTest, TestVisitOtherForm), and if you
have any idea, please let me know.

On 2012/04/03 16:40:27, timsteele wrote:

+