Add security feature to ProvisionalSavePassword

components/password_manager/core/browser/password_manager_unittest.cc

Index: components/password_manager/core/browser/password_manager_unittest.cc
diff --git a/components/password_manager/core/browser/password_manager_unittest.cc b/components/password_manager/core/browser/password_manager_unittest.cc
index f45f191dc78c45ac5e8a20f6a9ae5010fd4f3621..a409d5a632e1fb35beee73943f8b99860692c060 100644
--- a/components/password_manager/core/browser/password_manager_unittest.cc
+++ b/components/password_manager/core/browser/password_manager_unittest.cc
@@ -33,6 +33,7 @@ using base::ASCIIToUTF16;
 using testing::_;
 using testing::AnyNumber;
 using testing::Return;
+using testing::ReturnRef;
 using testing::SaveArg;
 using testing::WithArg;
 
@@ -67,6 +68,7 @@ class MockPasswordManagerClient : public StubPasswordManagerClient {
                void(const autofill::PasswordForm&));
   MOCK_METHOD0(AutomaticPasswordSaveIndicator, void());
   MOCK_METHOD0(GetPrefs, PrefService*());
+  MOCK_CONST_METHOD0(GetMainFrameURL, const GURL&());
   MOCK_METHOD0(GetDriver, PasswordManagerDriver*());
   MOCK_CONST_METHOD0(GetStoreResultFilter, const MockStoreResultFilter*());
 
@@ -135,6 +137,9 @@ class PasswordManagerTest : public testing::Test {
         .WillRepeatedly(Return(password_autofill_manager_.get()));
     EXPECT_CALL(client_, DidLastPageLoadEncounterSSLErrors())
         .WillRepeatedly(Return(false));
+
+    ON_CALL(client_, GetMainFrameURL())
+        .WillByDefault(ReturnRef(GURL::EmptyGURL()));
   }
 
   void TearDown() override {
@@ -748,6 +753,113 @@ TEST_F(PasswordManagerTest,
   manager()->OnPasswordFormsRendered(&driver_, observed, true);
 }
 
+TEST_F(PasswordManagerTest,
+       ShouldBlockPasswordForSameOriginButDifferentSchemeTest) {
+  PasswordForm form = MakeSimpleForm();
+
+  // Same origin and same scheme.
+  manager()->main_frame_url_ = GURL("https://example.com/login");
+  form.origin = GURL("https://example.com/login");

[vasilii, 2017/01/12 14:59:21]

Optionally: introduce an array with input data and expected result. Iterate over
the array. See password_bubble_experiment_unittest.cc

It's just less code in total.

[jdoerrie, 2017/01/12 17:40:58]

Done.

+  EXPECT_FALSE(
+      manager()->ShouldBlockPasswordForSameOriginButDifferentScheme(form));
+
+  // Same host and same scheme, different port.
+  manager()->main_frame_url_ = GURL("https://example.com:443/login");
+  form.origin = GURL("https://example.com:444/login");
+  EXPECT_FALSE(
+      manager()->ShouldBlockPasswordForSameOriginButDifferentScheme(form));
+
+  // Same host but different scheme (https to http).
+  manager()->main_frame_url_ = GURL("https://example.com/login");
+  form.origin = GURL("http://example.com/login");
+  EXPECT_TRUE(
+      manager()->ShouldBlockPasswordForSameOriginButDifferentScheme(form));
+
+  // Same host but different scheme (http to https).
+  manager()->main_frame_url_ = GURL("https://example.com/login");
+  form.origin = GURL("http://example.com/login");

[vasilii, 2017/01/12 14:59:21]

Looks suspiciously similar to the one above ;-)

[jdoerrie, 2017/01/12 17:40:58]

Done.

+  EXPECT_TRUE(
+      manager()->ShouldBlockPasswordForSameOriginButDifferentScheme(form));
+
+  // Different TLD, same schemes.
+  manager()->main_frame_url_ = GURL("https://example.com/login");
+  form.origin = GURL("https://example.org/login");
+  EXPECT_FALSE(
+      manager()->ShouldBlockPasswordForSameOriginButDifferentScheme(form));
+
+  // Different TLD, different schemes.
+  manager()->main_frame_url_ = GURL("https://example.com/login");
+  form.origin = GURL("http://example.org/login");
+  EXPECT_FALSE(
+      manager()->ShouldBlockPasswordForSameOriginButDifferentScheme(form));
+
+  // Different subdomains, same schemes.
+  manager()->main_frame_url_ = GURL("https://sub1.example.com/login");
+  form.origin = GURL("https://sub2.example.org/login");
+  EXPECT_FALSE(
+      manager()->ShouldBlockPasswordForSameOriginButDifferentScheme(form));
+}
+
+// Tests whether two submissions to the same origin but different schemes
+// result in only saving the first submission, which has a secure scheme.
+TEST_F(PasswordManagerTest, AttemptedSavePasswordSameOriginInsecureScheme) {
+  PasswordForm secure_form(MakeSimpleForm());
+  secure_form.origin = GURL("https://example.com/login");
+  secure_form.action = GURL("https://example.com/login");
+  secure_form.signon_realm = secure_form.origin.spec();
+
+  PasswordForm insecure_form(MakeSimpleForm());
+  insecure_form.username_value = ASCIIToUTF16("compromised_user");
+  insecure_form.password_value = ASCIIToUTF16("C0mpr0m1s3d_P4ss");
+  insecure_form.origin = GURL("http://example.com/home");
+  insecure_form.action = GURL("http://example.com/home");
+  insecure_form.signon_realm = insecure_form.origin.spec();
+
+  EXPECT_CALL(*store_, GetLogins(_, _))
+      .WillRepeatedly(WithArg<1>(InvokeEmptyConsumerWithForms()));
+
+  EXPECT_CALL(client_, IsSavingAndFillingEnabledForCurrentPage())
+      .WillRepeatedly(Return(true));
+
+  EXPECT_CALL(client_, GetMainFrameURL())
+      .WillRepeatedly(ReturnRef(secure_form.origin));

[vasilii, 2017/01/12 14:59:21]

I want to be more careful about this one. It's used in ProvisionallySavePassword
and may influence the test result.
Please write the expectation before the places they are needed. Obviously,
before OnPasswordFormSubmitted(insecure_form) you'll need another (unsecure)
URL.

[jdoerrie, 2017/01/12 17:40:58]

Done.

+
+  // Parse, render and submit the secure form.
+  std::vector<PasswordForm> observed = {secure_form};
+  manager()->OnPasswordFormsParsed(&driver_, observed);
+  manager()->OnPasswordFormsRendered(&driver_, observed, true);
+  OnPasswordFormSubmitted(secure_form);
+
+  // Make sure |PromptUserToSaveOrUpdatePassword| gets called, and the resulting
+  // form manager is saved.
+  std::unique_ptr<PasswordFormManager> form_manager_to_save;
+  EXPECT_CALL(client_,
+              PromptUserToSaveOrUpdatePasswordPtr(
+                  _, CredentialSourceType::CREDENTIAL_SOURCE_PASSWORD_MANAGER))
+      .WillOnce(WithArg<0>(SaveToScopedPtr(&form_manager_to_save)));
+
+  // Parse, render and submit the insecure form.
+  observed = {insecure_form};
+  manager()->OnPasswordFormsParsed(&driver_, observed);
+  manager()->OnPasswordFormsRendered(&driver_, observed, true);
+  OnPasswordFormSubmitted(insecure_form);
+
+  // Expect no further calls to |ProptUserToSaveOrUpdatePassword| due to
+  // insecure origin.
+  EXPECT_CALL(client_, PromptUserToSaveOrUpdatePasswordPtr(_, _)).Times(0);
+
+  // Trigger call to |ProvisionalSavePassword| by rendering a page without
+  // forms.
+  observed.clear();
+  manager()->OnPasswordFormsParsed(&driver_, observed);
+  manager()->OnPasswordFormsRendered(&driver_, observed, true);
+
+  // Make sure that the form saved by the user is indeed the secure form.
+  EXPECT_CALL(*store_, AddLogin(FormMatches(secure_form)));
+  ASSERT_TRUE(form_manager_to_save);
+  form_manager_to_save->Save();

[vasilii, 2017/01/12 14:59:21]

Optionally: EXPECT_THAT(form_manager_to_save->pending_credentials(),
FormMatches(secure_form));

[jdoerrie, 2017/01/12 17:40:58]

Done.

+}
+
 // Create a form with both a new and current password element. Let the current
 // password value be non-empty and the new password value be empty and submit
 // the form. While normally saving the new password is preferred (on change