Skip to content

Commit

Permalink
[Password Manager] Disable saving and filling on sync signup
Browse files Browse the repository at this point in the history
BUG=386390, 357233

Review URL: https://codereview.chromium.org/344033008

git-svn-id: svn://svn.chromium.org/chrome/trunk/src@281896 0039d316-1c4b-4281-b951-d872f2087c98
  • Loading branch information
gcasto@chromium.org committed Jul 9, 2014
1 parent afb0e08 commit f67ac43
Show file tree
Hide file tree
Showing 9 changed files with 94 additions and 3 deletions.
36 changes: 36 additions & 0 deletions chrome/browser/password_manager/chrome_password_manager_client.cc
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,7 @@
#include "chrome/browser/ui/passwords/manage_passwords_ui_controller.h"
#include "chrome/common/chrome_switches.h"
#include "chrome/common/chrome_version_info.h"
#include "chrome/common/url_constants.h"
#include "components/autofill/content/common/autofill_messages.h"
#include "components/autofill/core/browser/password_generator.h"
#include "components/autofill/core/common/password_form.h"
Expand All @@ -28,6 +29,7 @@
#include "components/password_manager/core/browser/password_manager_internals_service.h"
#include "components/password_manager/core/browser/password_manager_metrics_util.h"
#include "components/password_manager/core/common/password_manager_switches.h"
#include "content/public/browser/navigation_entry.h"
#include "content/public/browser/render_view_host.h"
#include "content/public/browser/web_contents.h"

Expand Down Expand Up @@ -81,6 +83,24 @@ bool ChromePasswordManagerClient::IsAutomaticPasswordSavingEnabled() const {
chrome::VersionInfo::CHANNEL_UNKNOWN;
}

bool ChromePasswordManagerClient::IsPasswordManagerEnabledForCurrentPage()
const {
if (EnabledForSyncSignin())
return true;

DCHECK(web_contents());
content::NavigationEntry* entry =
web_contents()->GetController().GetLastCommittedEntry();
if (!entry) {
// TODO(gcasto): Determine if fix for crbug.com/388246 is relevant here.
return true;
}
// 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.
return entry->GetURL().host() != chrome::kChromeUIChromeSigninHost;
}

void ChromePasswordManagerClient::PromptUserToSavePassword(
password_manager::PasswordFormManager* form_to_save) {
if (IsTheHotNewBubbleUIEnabled()) {
Expand Down Expand Up @@ -333,3 +353,19 @@ bool ChromePasswordManagerClient::IsTheHotNewBubbleUIEnabled() {
// The bubble should be the default case that runs on the bots.
return group_name != "Infobar";
}

bool ChromePasswordManagerClient::EnabledForSyncSignin() {
CommandLine* command_line = CommandLine::ForCurrentProcess();
if (command_line->HasSwitch(
password_manager::switches::kDisableManagerForSyncSignin))
return false;

if (command_line->HasSwitch(
password_manager::switches::kEnableManagerForSyncSignin))
return true;

// Default is enabled.
std::string group_name =
base::FieldTrialList::FindFullName("PasswordManagerStateForSyncSignin");
return group_name != "Disabled";
}
Original file line number Diff line number Diff line change
Expand Up @@ -39,6 +39,7 @@ class ChromePasswordManagerClient

// PasswordManagerClient implementation.
virtual bool IsAutomaticPasswordSavingEnabled() const OVERRIDE;
virtual bool IsPasswordManagerEnabledForCurrentPage() const OVERRIDE;
virtual void PromptUserToSavePassword(
password_manager::PasswordFormManager* form_to_save) OVERRIDE;
virtual void PasswordWasAutofilled(
Expand Down Expand Up @@ -81,6 +82,9 @@ class ChromePasswordManagerClient
// the sad old Infobar UI.
static bool IsTheHotNewBubbleUIEnabled();

// Returns true if the password manager should be enabled during sync signin.
static bool EnabledForSyncSignin();

private:
ChromePasswordManagerClient(content::WebContents* web_contents,
autofill::AutofillClient* autofill_client);
Expand Down
10 changes: 7 additions & 3 deletions components/password_manager/core/browser/password_manager.cc
Original file line number Diff line number Diff line change
Expand Up @@ -111,9 +111,14 @@ void PasswordManager::SetFormHasGeneratedPassword(const PasswordForm& form) {
// TODO(gcasto): Add UMA stats to track this.
}

bool PasswordManager::IsEnabledForCurrentPage() const {
return !driver_->DidLastPageLoadEncounterSSLErrors() &&
client_->IsPasswordManagerEnabledForCurrentPage();
}

bool PasswordManager::IsSavingEnabledForCurrentPage() const {
return *password_manager_enabled_ && !driver_->IsOffTheRecord() &&
!driver_->DidLastPageLoadEncounterSSLErrors();
IsEnabledForCurrentPage();
}

void PasswordManager::ProvisionallySavePassword(const PasswordForm& form) {
Expand Down Expand Up @@ -318,8 +323,7 @@ void PasswordManager::OnPasswordFormsParsed(

void PasswordManager::CreatePendingLoginManagers(
const std::vector<PasswordForm>& forms) {
// Don't try to autofill or save passwords in the presence of SSL errors.
if (driver_->DidLastPageLoadEncounterSSLErrors())
if (!IsEnabledForCurrentPage())
return;

// Copy the weak pointers to the currently known login managers for comparison
Expand Down
5 changes: 5 additions & 0 deletions components/password_manager/core/browser/password_manager.h
Original file line number Diff line number Diff line change
Expand Up @@ -116,6 +116,11 @@ class PasswordManager : public LoginModel {
MAX_FAILURE_VALUE
};

// Returns if the password manager is enabled for this page. There are certain
// situations (e.g. bad SSL cert) where we disable the password manager
// temporarily.
bool IsEnabledForCurrentPage() const;

// Log failure for UMA. Logs additional metrics if the |form_origin|
// corresponds to one of the top, explicitly monitored websites. For some
// values of |failure| also sends logs to the internals page through |logger|,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,10 @@ bool PasswordManagerClient::IsAutomaticPasswordSavingEnabled() const {
return false;
}

bool PasswordManagerClient::IsPasswordManagerEnabledForCurrentPage() const {
return true;
}

base::FieldTrial::Probability
PasswordManagerClient::GetProbabilityForExperiment(
const std::string& experiment_name) {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -30,6 +30,10 @@ class PasswordManagerClient {
// The default return value is false.
virtual bool IsAutomaticPasswordSavingEnabled() const;

// If the password manager should work for the current page. Default
// always returns true.
virtual bool IsPasswordManagerEnabledForCurrentPage() const;

// Informs the embedder of a password form that can be saved if the user
// allows it. The embedder is not required to prompt the user if it decides
// that this form doesn't need to be saved.
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -42,6 +42,7 @@ namespace {

class MockPasswordManagerClient : public StubPasswordManagerClient {
public:
MOCK_CONST_METHOD0(IsPasswordManagerEnabledForCurrentPage, bool());
MOCK_METHOD1(PromptUserToSavePassword, void(PasswordFormManager*));
MOCK_METHOD0(GetPasswordStore, PasswordStore*());
MOCK_METHOD0(GetPrefs, PrefService*());
Expand Down Expand Up @@ -82,6 +83,8 @@ class PasswordManagerTest : public testing::Test {
EXPECT_CALL(*store_, ReportMetrics()).Times(AnyNumber());
CHECK(store_->Init(syncer::SyncableService::StartSyncFlare()));

EXPECT_CALL(client_, IsPasswordManagerEnabledForCurrentPage())
.WillRepeatedly(Return(true));
EXPECT_CALL(client_, GetPasswordStore()).WillRepeatedly(Return(store_));
EXPECT_CALL(client_, GetPrefs()).WillRepeatedly(Return(&prefs_));
EXPECT_CALL(client_, GetDriver()).WillRepeatedly(Return(&driver_));
Expand Down Expand Up @@ -625,4 +628,24 @@ TEST_F(PasswordManagerTest, AutofillingNotEnabledOnSSLErrors) {
manager()->OnPasswordFormsParsed(forms);
}

TEST_F(PasswordManagerTest, SavingDisabledIfManagerDisabled) {
EXPECT_CALL(client_, IsPasswordManagerEnabledForCurrentPage())
.WillRepeatedly(Return(false));
EXPECT_FALSE(manager()->IsSavingEnabledForCurrentPage());
}

TEST_F(PasswordManagerTest, AutofillingDisabledIfManagerDisabled) {
EXPECT_CALL(client_, IsPasswordManagerEnabledForCurrentPage())
.WillRepeatedly(Return(false));

// Let us pretend some forms were found on a website.
std::vector<PasswordForm> forms;
forms.push_back(MakeSimpleForm());

// Feed those forms to |manager()| and check that it does not try to find
// matching saved credentials for the forms.
EXPECT_CALL(*store_.get(), GetLogins(_, _, _)).Times(Exactly(0));
manager()->OnPasswordFormsParsed(forms);
}

} // namespace password_manager
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,15 @@ namespace password_manager {

namespace switches {

// Disable both saving and filling for the sync signin form.
const char kDisableManagerForSyncSignin[] =
"disable-manager-for-sync-signin";

// Enable saving and filling for the sync signin form. Currently the default
// behavior.
const char kEnableManagerForSyncSignin[] =
"enable-manager-for-sync-signin";

// Disables the save-password prompt. Passwords are then saved automatically,
// without asking the user.
const char kEnableAutomaticPasswordSaving[] =
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,8 @@ namespace switches {
// All switches in alphabetical order. The switches should be documented
// alongside the definition of their values in the .cc file.

extern const char kDisableManagerForSyncSignin[];
extern const char kEnableManagerForSyncSignin[];
extern const char kEnableAutomaticPasswordSaving[];

} // namespace switches
Expand Down

0 comments on commit f67ac43

Please sign in to comment.