Don't show "Show all passwords" context menu item in the guest mode and on

Chrome internal pages.

Basically it should not appear if password filling is disabled. In addition the
guest mode should also suppress the item as it leads to nowhere.

Bug: 865917
Change-Id: Id260341e381ab99d364daf8819f7f07cd350b141
Reviewed-on: https://chromium-review.googlesource.com/1205030
Commit-Queue: Vasilii Sukhanov <vasilii@chromium.org>
Reviewed-by: Vadym Doroshenko <dvadym@chromium.org>
Reviewed-by: Istiaque Ahmed <lazyboy@chromium.org>
Cr-Commit-Position: refs/heads/master@{#589899}

chrome/browser/password_manager/chrome_password_manager_client.cc

@@ -242,19 +242,7 @@ bool ChromePasswordManagerClient::IsPasswordManagementEnabledForCurrentPage()
     // TODO(gcasto): Determine if fix for crbug.com/388246 is relevant here.
     is_enabled = true;
   } else {
-    // Do not fill nor save password when a user is signing in for sync. This
-    // is because users need to remember their password if they are syncing as
-    // this is effectively their master password.
-    is_enabled =
-        entry->GetURL().host_piece() != chrome::kChromeUIChromeSigninHost;
-
-    // Per https://crbug.com/756587, exclude existing saved passwords for
-    // about: documents.  Note that this only checks main frames, but this is
-    // sufficient for credentials manager API which is only enabled for main
-    // frames.  Autofill for about: subframes is already disabled by
-    // restricting OnPasswordFormsParsed/OnPasswordFormsRendered.
-    if (entry->GetURL().SchemeIs(url::kAboutScheme))
-      is_enabled = false;
+    is_enabled = CanShowBubbleOnURL(entry->GetURL());
   }
 
   // The password manager is disabled while VR (virtual reality) is being used,
@@ -303,7 +291,8 @@ bool ChromePasswordManagerClient::IsFillingEnabledForCurrentPage() const {
 
 bool ChromePasswordManagerClient::IsFillingFallbackEnabledForCurrentPage()
     const {
-  return !Profile::FromBrowserContext(web_contents()->GetBrowserContext())
+  return IsFillingEnabledForCurrentPage() &&
+         !Profile::FromBrowserContext(web_contents()->GetBrowserContext())
               ->IsGuestSession();
 }
 

chrome/browser/password_manager/chrome_password_manager_client_unittest.cc

@@ -355,6 +355,7 @@ TEST_F(ChromePasswordManagerClientTest, SavingAndFillingEnabledConditionsTest) {
       .WillRepeatedly(Return(net::CERT_STATUS_AUTHORITY_INVALID));
   EXPECT_FALSE(client->IsSavingAndFillingEnabledForCurrentPage());
   EXPECT_FALSE(client->IsFillingEnabledForCurrentPage());
+  EXPECT_FALSE(client->IsFillingFallbackEnabledForCurrentPage());
 
   // Functionality disabled if there are SSL errors and the manager itself is
   // disabled.
@@ -362,14 +363,16 @@ TEST_F(ChromePasswordManagerClientTest, SavingAndFillingEnabledConditionsTest) {
                        std::make_unique<base::Value>(false));
   EXPECT_FALSE(client->IsSavingAndFillingEnabledForCurrentPage());
   EXPECT_FALSE(client->IsFillingEnabledForCurrentPage());
+  EXPECT_FALSE(client->IsFillingFallbackEnabledForCurrentPage());
 
-  // Functionality disabled if there are no SSL errors, but the manager itself
-  // is disabled.
+  // Saving disabled if there are no SSL errors, but the manager itself is
+  // disabled.
   EXPECT_CALL(*client, GetMainFrameCertStatus()).WillRepeatedly(Return(0));
   prefs()->SetUserPref(password_manager::prefs::kCredentialsEnableService,
                        std::make_unique<base::Value>(false));
   EXPECT_FALSE(client->IsSavingAndFillingEnabledForCurrentPage());
   EXPECT_TRUE(client->IsFillingEnabledForCurrentPage());
+  EXPECT_TRUE(client->IsFillingFallbackEnabledForCurrentPage());
 
   // Functionality enabled if there are no SSL errors and the manager is
   // enabled.
@@ -378,19 +381,28 @@ TEST_F(ChromePasswordManagerClientTest, SavingAndFillingEnabledConditionsTest) {
                        std::make_unique<base::Value>(true));
   EXPECT_TRUE(client->IsSavingAndFillingEnabledForCurrentPage());
   EXPECT_TRUE(client->IsFillingEnabledForCurrentPage());
+  EXPECT_TRUE(client->IsFillingFallbackEnabledForCurrentPage());
 
-  // Functionality disabled in Incognito mode.
+  // Saving disabled in Incognito mode.
   profile()->ForceIncognito(true);
   EXPECT_FALSE(client->IsSavingAndFillingEnabledForCurrentPage());
   EXPECT_TRUE(client->IsFillingEnabledForCurrentPage());
+  EXPECT_TRUE(client->IsFillingFallbackEnabledForCurrentPage());
 
-  // Functionality disabled in Incognito mode also when manager itself is
+  // Saving disabled in Incognito mode also when manager itself is
   // enabled.
   prefs()->SetUserPref(password_manager::prefs::kCredentialsEnableService,
                        std::make_unique<base::Value>(true));
   EXPECT_FALSE(client->IsSavingAndFillingEnabledForCurrentPage());
   EXPECT_TRUE(client->IsFillingEnabledForCurrentPage());
-  profile()->ForceIncognito(false);
+  EXPECT_TRUE(client->IsFillingFallbackEnabledForCurrentPage());
+
+  // In guest mode saving is disabled, filling is enabled but there is in fact
+  // nothing to fill, manual filling is disabled.
+  profile()->SetGuestSession(true);
+  EXPECT_FALSE(client->IsSavingAndFillingEnabledForCurrentPage());
+  EXPECT_TRUE(client->IsFillingEnabledForCurrentPage());
+  EXPECT_FALSE(client->IsFillingFallbackEnabledForCurrentPage());
 }
 
 TEST_F(ChromePasswordManagerClientTest, SavingDependsOnAutomation) {
@@ -404,14 +416,76 @@ TEST_F(ChromePasswordManagerClientTest, SavingDependsOnAutomation) {
 
 // Check that password manager is disabled on about:blank pages.
 // See https://crbug.com/756587.
-TEST_F(ChromePasswordManagerClientTest, SavingAndFillingDisbledForAboutBlank) {
+TEST_F(ChromePasswordManagerClientTest, SavingAndFillingDisabledForAboutBlank) {
   const GURL kUrl(url::kAboutBlankURL);
   NavigateAndCommit(kUrl);
   EXPECT_EQ(kUrl, GetClient()->GetLastCommittedEntryURL());
   EXPECT_FALSE(GetClient()->IsSavingAndFillingEnabledForCurrentPage());
   EXPECT_FALSE(GetClient()->IsFillingEnabledForCurrentPage());
+  EXPECT_FALSE(GetClient()->IsFillingFallbackEnabledForCurrentPage());
 }
 
+namespace {
+
+struct SchemeTestCase {
+  const char* scheme;
+  bool password_manager_works;
+};
+const SchemeTestCase kTestCases[] = {
+    {url::kHttpScheme, true},
+    {url::kHttpsScheme, true},
+    {url::kFtpScheme, true},
+    {url::kDataScheme, true},
+    {"feed", true},
+
+    {"invalid-scheme-i-just-made-up", false},
+    {content::kChromeDevToolsScheme, false},
+    {content::kChromeUIScheme, false},
+    {url::kMailToScheme, false},
+};
+
+// Parameterized test that takes a URL scheme as a parameter. Every scheme
+// requires a separate test because NavigateAndCommit can be called only once.
+class ChromePasswordManagerClientSchemeTest
+    : public ChromePasswordManagerClientTest,
+      public ::testing::WithParamInterface<const char*> {
+ public:
+  static std::vector<const char*> GetSchemes() {
+    std::vector<const char*> result;
+    for (const SchemeTestCase& test_case : kTestCases) {
+      result.push_back(test_case.scheme);
+    }
+    return result;
+  }
+};
+
+TEST_P(ChromePasswordManagerClientSchemeTest,
+       SavingAndFillingOnDifferentSchemes) {
+  const GURL url(base::StringPrintf("%s://example.org", GetParam()));
+  VLOG(0) << url.possibly_invalid_spec();
+  NavigateAndCommit(url);
+  EXPECT_EQ(url, GetClient()->GetLastCommittedEntryURL());
+
+  auto* it = std::find_if(std::begin(kTestCases), std::end(kTestCases),
+                          [this](auto test_case) {
+                            return strcmp(test_case.scheme, GetParam()) == 0;
+                          });
+  ASSERT_FALSE(it == std::end(kTestCases));
+  EXPECT_EQ(it->password_manager_works,
+            GetClient()->IsSavingAndFillingEnabledForCurrentPage());
+  EXPECT_EQ(it->password_manager_works,
+            GetClient()->IsFillingEnabledForCurrentPage());
+  EXPECT_EQ(it->password_manager_works,
+            GetClient()->IsFillingFallbackEnabledForCurrentPage());
+}
+
+INSTANTIATE_TEST_CASE_P(
+    ,
+    ChromePasswordManagerClientSchemeTest,
+    ::testing::ValuesIn(ChromePasswordManagerClientSchemeTest::GetSchemes()));
+
+}  // namespace
+
 // Verify the filling check behaves accordingly to the passed type of navigation
 // entry to check.
 TEST_F(ChromePasswordManagerClientTest,

chrome/browser/renderer_context_menu/render_view_context_menu.cc

@@ -1574,18 +1574,17 @@ void RenderViewContextMenu::AppendProtocolHandlerSubMenu() {
 void RenderViewContextMenu::AppendPasswordItems() {
   bool add_separator = false;
 
-  // Don't offer saving or generating passwords in incognito profiles.
-  if (!browser_context_->IsOffTheRecord()) {
-    password_manager::ContentPasswordManagerDriver* driver =
-        password_manager::ContentPasswordManagerDriver::GetForRenderFrameHost(
-            GetRenderFrameHost());
-    if (password_manager_util::ManualPasswordGenerationEnabled(driver)) {
-      menu_model_.AddItemWithStringId(IDC_CONTENT_CONTEXT_GENERATEPASSWORD,
-                                      IDS_CONTENT_CONTEXT_GENERATEPASSWORD);
-      add_separator = true;
-    }
+  password_manager::ContentPasswordManagerDriver* driver =
+      password_manager::ContentPasswordManagerDriver::GetForRenderFrameHost(
+          GetRenderFrameHost());
+  // Don't show the item for guest or incognito profiles and also when the
+  // automatic generation feature is disabled.
+  if (password_manager_util::ManualPasswordGenerationEnabled(driver)) {
+    menu_model_.AddItemWithStringId(IDC_CONTENT_CONTEXT_GENERATEPASSWORD,
+                                    IDS_CONTENT_CONTEXT_GENERATEPASSWORD);
+    add_separator = true;
   }
-  if (password_manager_util::ShowAllSavedPasswordsContextMenuEnabled()) {
+  if (password_manager_util::ShowAllSavedPasswordsContextMenuEnabled(driver)) {
     menu_model_.AddItemWithStringId(IDC_CONTENT_CONTEXT_SHOWALLSAVEDPASSWORDS,
                                     IDS_AUTOFILL_SHOW_ALL_SAVED_FALLBACK);
     add_separator = true;

chrome/browser/renderer_context_menu/render_view_context_menu_unittest.cc

@@ -16,6 +16,7 @@
 #include "chrome/browser/extensions/test_extension_prefs.h"
 #include "chrome/browser/net/spdyproxy/data_reduction_proxy_chrome_settings.h"
 #include "chrome/browser/net/spdyproxy/data_reduction_proxy_chrome_settings_factory.h"
+#include "chrome/browser/password_manager/chrome_password_manager_client.h"
 #include "chrome/browser/prefs/incognito_mode_prefs.h"
 #include "chrome/browser/renderer_context_menu/render_view_context_menu_test_util.h"
 #include "chrome/common/chrome_features.h"
@@ -26,6 +27,7 @@
 #include "components/data_reduction_proxy/core/browser/data_store.h"
 #include "components/data_reduction_proxy/core/common/data_reduction_proxy_headers.h"
 #include "components/data_reduction_proxy/core/common/data_reduction_proxy_params.h"
+#include "components/password_manager/content/browser/content_password_manager_driver_factory.h"
 #include "components/prefs/pref_registry_simple.h"
 #include "components/prefs/pref_service.h"
 #include "components/proxy_config/proxy_config_pref_names.h"
@@ -593,3 +595,41 @@ TEST_F(RenderViewContextMenuPrefsTest, SaveMediaSuggestedFileName) {
       content::WebContentsTester::For(web_contents())->GetSuggestedFileName();
   EXPECT_EQ(kTestSuggestedFileName, suggested_filename);
 }
+
+// Verify that "Show all passwords" is displayed on a password field.
+TEST_F(RenderViewContextMenuPrefsTest, ShowAllPasswords) {
+  // Set up password manager stuff.
+  ChromePasswordManagerClient::CreateForWebContentsWithAutofillClient(
+      web_contents(), nullptr);
+  password_manager::ContentPasswordManagerDriverFactory::FromWebContents(
+      web_contents())
+      ->RenderFrameCreated(web_contents()->GetMainFrame());
+
+  content::ContextMenuParams params = CreateParams(MenuItem::EDITABLE);
+  params.input_field_type = blink::WebContextMenuData::kInputFieldTypePassword;
+  auto menu = std::make_unique<TestRenderViewContextMenu>(
+      web_contents()->GetMainFrame(), params);
+  menu->Init();
+
+  EXPECT_TRUE(menu->IsItemPresent(IDC_CONTENT_CONTEXT_SHOWALLSAVEDPASSWORDS));
+}
+
+// Verify that "Show all passwords" is displayed on a password field in
+// Incognito.
+TEST_F(RenderViewContextMenuPrefsTest, ShowAllPasswordsIncognito) {
+  profile()->ForceIncognito(true);
+  // Set up password manager stuff.
+  ChromePasswordManagerClient::CreateForWebContentsWithAutofillClient(
+      web_contents(), nullptr);
+  password_manager::ContentPasswordManagerDriverFactory::FromWebContents(
+      web_contents())
+      ->RenderFrameCreated(web_contents()->GetMainFrame());
+
+  content::ContextMenuParams params = CreateParams(MenuItem::EDITABLE);
+  params.input_field_type = blink::WebContextMenuData::kInputFieldTypePassword;
+  auto menu = std::make_unique<TestRenderViewContextMenu>(
+      web_contents()->GetMainFrame(), params);
+  menu->Init();
+
+  EXPECT_TRUE(menu->IsItemPresent(IDC_CONTENT_CONTEXT_SHOWALLSAVEDPASSWORDS));
+}

components/password_manager/core/browser/password_autofill_manager.cc

@@ -282,8 +282,6 @@ void PasswordAutofillManager::OnShowPasswordSuggestions(
 
   if (ShouldShowManualFallbackForPreLollipop(
           autofill_client_->GetSyncService())) {
-    if (password_client_ &&
-        password_client_->IsFillingFallbackEnabledForCurrentPage()) {
       autofill::Suggestion suggestion(
           l10n_util::GetStringUTF8(IDS_PASSWORD_MANAGER_MANAGE_PASSWORDS),
           std::string(), std::string(),
@@ -292,7 +290,6 @@ void PasswordAutofillManager::OnShowPasswordSuggestions(
 
       metrics_util::LogContextOfShowAllSavedPasswordsShown(
           metrics_util::SHOW_ALL_SAVED_PASSWORDS_CONTEXT_PASSWORD);
-    }
   }
 
   autofill_client_->ShowAutofillPopup(bounds, text_direction, suggestions,

components/password_manager/core/browser/password_manager_util.cc

@@ -19,6 +19,7 @@
 #include "components/password_manager/core/browser/hsts_query.h"
 #include "components/password_manager/core/browser/log_manager.h"
 #include "components/password_manager/core/browser/password_generation_manager.h"
+#include "components/password_manager/core/browser/password_manager.h"
 #include "components/password_manager/core/browser/password_manager_client.h"
 #include "components/password_manager/core/browser/password_manager_driver.h"
 #include "components/password_manager/core/browser/password_manager_metrics_util.h"
@@ -304,14 +305,21 @@ bool ManualPasswordGenerationEnabled(
   return true;
 }
 
-bool ShowAllSavedPasswordsContextMenuEnabled() {
-  if (!base::FeatureList::IsEnabled(
-          password_manager::features::kShowAllSavedPasswordsContextMenu)) {
+bool ShowAllSavedPasswordsContextMenuEnabled(
+    password_manager::PasswordManagerDriver* driver) {
+  password_manager::PasswordManager* password_manager =
+      driver ? driver->GetPasswordManager() : nullptr;
+  if (!password_manager)
     return false;
-  }
+
+  password_manager::PasswordManagerClient* client = password_manager->client();
+  if (!client || !client->IsFillingFallbackEnabledForCurrentPage())
+    return false;
+
   LogContextOfShowAllSavedPasswordsShown(
       password_manager::metrics_util::
           SHOW_ALL_SAVED_PASSWORDS_CONTEXT_CONTEXT_MENU);
+
   return true;
 }
 

components/password_manager/core/browser/password_manager_util.h

@@ -81,7 +81,8 @@ bool ManualPasswordGenerationEnabled(
 // Returns true iff the "Show all saved passwords" option should be shown in
 // Context Menu. Also records metric, that the Context Menu will have "Show all
 // saved passwords" option.
-bool ShowAllSavedPasswordsContextMenuEnabled();
+bool ShowAllSavedPasswordsContextMenuEnabled(
+    password_manager::PasswordManagerDriver* driver);
 
 // Opens Password Manager setting page and records the metrics.
 void UserTriggeredShowAllSavedPasswordsFromContextMenu(

components/password_manager/core/common/password_manager_features.cc

@@ -38,10 +38,6 @@ const base::Feature kPasswordGenerationRequirementsDomainOverrides = {
     "PasswordGenerationRequirementsDomainOverrides",
     base::FEATURE_ENABLED_BY_DEFAULT};
 
-// Enables the "Show all saved passwords" option in Context Menu.
-const base::Feature kShowAllSavedPasswordsContextMenu{
-    "ShowAllSavedPasswordsContextMenu", base::FEATURE_ENABLED_BY_DEFAULT};
-
 // Disallow autofilling of the sync credential.
 const base::Feature kProtectSyncCredential = {
     "protect-sync-credential", base::FEATURE_DISABLED_BY_DEFAULT};

components/password_manager/core/common/password_manager_features.h

@@ -23,7 +23,6 @@ extern const base::Feature kDeleteUndecryptableLogins;
 extern const base::Feature kHtmlBasedUsernameDetector;
 extern const base::Feature kPasswordGenerationRequirements;
 extern const base::Feature kPasswordGenerationRequirementsDomainOverrides;
-extern const base::Feature kShowAllSavedPasswordsContextMenu;
 extern const base::Feature kFillOnAccountSelect;
 extern const base::Feature kMigrateLinuxToLoginDB;
 extern const base::Feature kNewPasswordFormParsing;