Ignore autocomplete=off when password manager is saving the generated passwords during account crea…

chrome/browser/password_manager/password_manager_unittest.cc

 // Copyright (c) 2012 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include <vector>
 
 #include "base/message_loop.h"
 #include "base/string_util.h"
 #include "base/utf_string_conversions.h"
 #include "chrome/browser/password_manager/mock_password_store.h"
@@ skipping 57 matching lines @@
         web_contents(), &delegate_);
     EXPECT_CALL(delegate_, DidLastPageLoadEncounterSSLErrors())
         .WillRepeatedly(Return(false));
   }
 
   virtual void TearDown() {
     store_ = NULL;
     ChromeRenderViewHostTestHarness::TearDown();
   }
 
-  PasswordForm MakeSimpleForm() {
+  // Make default value of |autocomplete| to be true, such that we only need to
+  // add tests in autocomplete=off cases.
+  PasswordForm MakeSimpleForm(bool autocomplete = true) {

[Ilya Sherman, 2012/12/06 22:55:59]

nit: Default function parameters are disallowed by the style guide:
http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml#Default_Arguments

The way I'd recommend doing this is not passing a parameter to this function,
and instead explicitly setting form.password_should_autocomplete = false on the
value returned from this function, for the tests that are testing that case.

[zysxqn, 2012/12/06 23:54:49]

Done.

     PasswordForm form;
     form.origin = GURL("http://www.google.com/a/LoginAuth");
     form.action = GURL("http://www.google.com/a/Login");
     form.username_element = ASCIIToUTF16("Email");
     form.password_element = ASCIIToUTF16("Passwd");
     form.username_value = ASCIIToUTF16("google");
     form.password_value = ASCIIToUTF16("password");
+    form.password_should_autocomplete = autocomplete;
     form.submit_element = ASCIIToUTF16("signIn");
     form.signon_realm = "http://www.google.com";
     return form;
   }
 
   PasswordManager* manager() {
     return PasswordManager::FromWebContents(web_contents());
   }
 
   // We create a UI thread to satisfy PasswordStore.
   content::TestBrowserThread ui_thread_;
 
   scoped_refptr<MockPasswordStore> store_;
   MockPasswordManagerDelegate delegate_;  // Owned by manager_.
 
   TestingProfile* testing_profile_;
 };
 
 MATCHER_P(FormMatches, form, "") {
   return form.signon_realm == arg.signon_realm &&
          form.origin == arg.origin &&
          form.action == arg.action &&
          form.username_element == arg.username_element &&
          form.password_element == arg.password_element &&
+         form.password_should_autocomplete ==
+             arg.password_should_autocomplete &&
          form.submit_element == arg.submit_element;
 }
 
 TEST_F(PasswordManagerTest, FormSubmitEmptyStore) {
   // Test that observing a newly submitted form shows the save password bar.
   std::vector<PasswordForm*> result;  // Empty password store.
   EXPECT_CALL(delegate_, FillPasswordForm(_)).Times(Exactly(0));
   EXPECT_CALL(*store_, GetLogins(_,_))
       .WillOnce(DoAll(WithArg<1>(InvokeConsumer(0, result)), Return(0)));
   std::vector<PasswordForm> observed;
@@ skipping 243 matching lines @@
                            Value::CreateBooleanValue(false));
   EXPECT_CALL(delegate_, FillPasswordForm(_));
   EXPECT_CALL(*store_, GetLogins(_, _))
     .WillRepeatedly(DoAll(WithArg<1>(InvokeConsumer(0, result)), Return(0)));
   std::vector<PasswordForm> observed;
   PasswordForm form(MakeSimpleForm());
   observed.push_back(form);
   manager()->OnPasswordFormsParsed(observed);
 }
 
+TEST_F(PasswordManagerTest, FormNotSubmitAutocompleteOff) {

[Ilya Sherman, 2012/12/06 22:55:59]

nit: Perhaps "Saved" rather than "Submit", or else omit "Not"?  (This also
applies on line 403 below.)

[zysxqn, 2012/12/06 23:54:49]

Done.

+  // Test password form with non-generated password will not be saved if
+  // autocomplete=off.
+  //
+  // This is mostly copied from FormSubmitEmptyStore.

[Ilya Sherman, 2012/12/06 22:55:59]

No need to include this line in the comment, though it suggests that extracting
shared functionality into a helper function might be useful for these tests.

[zysxqn, 2012/12/06 23:54:49]

Done.

+  std::vector<PasswordForm*> result;  // Empty password store.
+  EXPECT_CALL(delegate_, FillPasswordForm(_)).Times(Exactly(0));
+  EXPECT_CALL(*store_, GetLogins(_,_))
+      .WillOnce(DoAll(WithArg<1>(InvokeConsumer(0, result)), Return(0)));
+  std::vector<PasswordForm> observed;
+  PasswordForm form(MakeSimpleForm(false));
+  observed.push_back(form);
+  manager()->OnPasswordFormsParsed(observed);  // The initial load.
+  manager()->OnPasswordFormsRendered(observed);  // The initial layout.
+
+  // And the form submit contract is to call ProvisionallySavePassword.
+  manager()->ProvisionallySavePassword(form);
+
+  // Now the password manager waits for the navigation to complete. No call
+  // should be expected since the form will not be save.
+  observed.clear();
+  manager()->OnPasswordFormsParsed(observed);  // The post-navigation load.
+  manager()->OnPasswordFormsRendered(observed);  // The post-navigation layout.
+}

[Ilya Sherman, 2012/12/06 22:55:59]

This test isn't actually testing that the form isn't saved.  You probably want
to include the following lines:

EXPECT_CALL(delegate_,
            AddSavePasswordInfoBarIfPermitted(_)).Times(Exactly(0));
EXPECT_CALL(*store_, AddLogin(FormMatches(form))).Times(Exactly(0));

[zysxqn, 2012/12/06 23:54:49]

Done.

+
+TEST_F(PasswordManagerTest, GeneratedPasswordFormSubmitAutocompleteOff) {
+  // Test password form with generated password will still be saved if
+  // autocomplete=off.
+  //
+  // This is mostly copied from GenerartedPasswordFormSubmitEmptyStore.
+  std::vector<PasswordForm*> result;  // Empty password store.
+  EXPECT_CALL(delegate_, FillPasswordForm(_)).Times(Exactly(0));
+  EXPECT_CALL(*store_, GetLogins(_,_))
+      .WillOnce(DoAll(WithArg<1>(InvokeConsumer(0, result)), Return(0)));
+  std::vector<PasswordForm> observed;
+  PasswordForm form(MakeSimpleForm(false));
+  observed.push_back(form);
+  manager()->OnPasswordFormsParsed(observed);  // The initial load.
+  manager()->OnPasswordFormsRendered(observed);  // The initial layout.
+
+  // Simulate the user generating the password and submitting the form.
+  manager()->SetFormHasGeneratedPassword(form);
+  manager()->ProvisionallySavePassword(form);
+
+  // The user should not be presented with an infobar as they have already given
+  // consent. The form should be saved once navigation occurs.

[Ilya Sherman, 2012/12/06 22:55:59]

Where has the user given consent?  Does this test pass when run in isolation?

[Ilya Sherman, 2012/12/06 23:40:37]

Never mind, I think I understand what this means: The user has given consent by
asking Chrome to generate the password.  I think the comment could probably be
improved to make that a little clearer -- I read this to mean that the user had
already given consent by previously saving a password for this website/username
combo, or something like that.

[zysxqn, 2012/12/06 23:54:49]

This is copied from GeneratedPasswordFormSubmitEmptyStore. I think this
statement means user has given consent to save the password by using the
generated password. Yeah, for this test this is just "simulated" by calling
manager()->SetFormHasGeneratedpassword(form) above.

[zysxqn, 2012/12/06 23:54:49]

Done.

+  EXPECT_CALL(delegate_,
+              AddSavePasswordInfoBarIfPermitted(_)).Times(Exactly(0));
+  EXPECT_CALL(*store_, AddLogin(FormMatches(form)));
+
+  // Now the password manager waits for the navigation to complete.
+  observed.clear();
+  manager()->OnPasswordFormsParsed(observed);  // The post-navigation load.
+  manager()->OnPasswordFormsRendered(observed);  // The post-navigation layout.
+}