From eb9ce0de55f3746e59ea805ca954a1be849b3815 Mon Sep 17 00:00:00 2001 From: nkostylev Date: Thu, 7 May 2015 15:23:24 -0700 Subject: [PATCH] UMA to track the reason for re-auth Landing https://codereview.chromium.org/1114543002/ BUG=469992 TBR=isherman@chromium.org Review URL: https://codereview.chromium.org/1132523002 Cr-Commit-Position: refs/heads/master@{#328858} --- .../login/existing_user_controller.cc | 10 +++ chrome/browser/chromeos/login/reauth_stats.cc | 36 ++++++++++ chrome/browser/chromeos/login/reauth_stats.h | 59 ++++++++++++++++ .../login/saml/saml_offline_signin_limiter.cc | 2 + .../saml_offline_signin_limiter_unittest.cc | 30 ++++++++- .../login/screens/user_selection_screen.cc | 7 ++ .../login/signin/auth_sync_observer.cc | 2 + .../chromeos/login/screen_password_changed.js | 5 +- .../chromeos/login/signin_screen_handler.cc | 26 ++++++- .../chromeos/login/signin_screen_handler.h | 4 +- chrome/chrome_browser_chromeos.gypi | 2 + components/user_manager/user_manager.h | 20 ++++++ components/user_manager/user_manager_base.cc | 67 ++++++++++++++++++- components/user_manager/user_manager_base.h | 9 +++ tools/metrics/histograms/histograms.xml | 42 ++++++++++++ .../account_picker/screen_account_picker.js | 6 ++ 16 files changed, 319 insertions(+), 8 deletions(-) create mode 100644 chrome/browser/chromeos/login/reauth_stats.cc create mode 100644 chrome/browser/chromeos/login/reauth_stats.h diff --git a/chrome/browser/chromeos/login/existing_user_controller.cc b/chrome/browser/chromeos/login/existing_user_controller.cc index b951c2da740955..c7f5a1af0efcb6 100644 --- a/chrome/browser/chromeos/login/existing_user_controller.cc +++ b/chrome/browser/chromeos/login/existing_user_controller.cc @@ -28,6 +28,7 @@ #include "chrome/browser/chromeos/login/easy_unlock/bootstrap_user_context_initializer.h" #include "chrome/browser/chromeos/login/easy_unlock/bootstrap_user_flow.h" #include "chrome/browser/chromeos/login/helper.h" +#include "chrome/browser/chromeos/login/reauth_stats.h" #include "chrome/browser/chromeos/login/session/user_session_manager.h" #include "chrome/browser/chromeos/login/signin/oauth2_token_initializer.h" #include "chrome/browser/chromeos/login/signin_specifics.h" @@ -572,6 +573,14 @@ void ExistingUserController::OnAuthFailure(const AuthFailure& failure) { // Clear the recorded displayed email so it won't affect any future attempts. display_email_.clear(); + + // TODO(ginkage): Fix this case once crbug.com/469990 is ready. + /* + if (failure.reason() == AuthFailure::COULD_NOT_MOUNT_CRYPTOHOME) { + RecordReauthReason(last_login_attempt_username_, + ReauthReason::MISSING_CRYPTOHOME); + } + */ } void ExistingUserController::OnAuthSuccess(const UserContext& user_context) { @@ -982,6 +991,7 @@ void ExistingUserController::ShowGaiaPasswordChanged( // changed. user_manager::UserManager::Get()->SaveUserOAuthStatus( username, user_manager::User::OAUTH2_TOKEN_STATUS_INVALID); + RecordReauthReason(username, ReauthReason::OTHER); login_display_->SetUIEnabled(true); login_display_->ShowGaiaPasswordChanged(username); diff --git a/chrome/browser/chromeos/login/reauth_stats.cc b/chrome/browser/chromeos/login/reauth_stats.cc new file mode 100644 index 00000000000000..f442c974f012ef --- /dev/null +++ b/chrome/browser/chromeos/login/reauth_stats.cc @@ -0,0 +1,36 @@ +// Copyright (c) 2015 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 "chrome/browser/chromeos/login/reauth_stats.h" + +#include "base/metrics/histogram_macros.h" +#include "components/user_manager/user_manager.h" + +namespace chromeos { + +void RecordReauthReason(const std::string& user_id, ReauthReason reason) { + user_manager::UserManager* user_manager = user_manager::UserManager::Get(); + int old_reason; + // We record only the first value, skipping everything else, except "none" + // value, which is used to reset the current state. + if (!user_manager->FindReauthReason(user_id, &old_reason) || + (static_cast(old_reason) == ReauthReason::NONE && + reason != ReauthReason::NONE)) { + user_manager->UpdateReauthReason(user_id, static_cast(reason)); + } +} + +void SendReauthReason(const std::string& user_id) { + user_manager::UserManager* user_manager = user_manager::UserManager::Get(); + int reauth_reason; + if (user_manager->FindReauthReason(user_id, &reauth_reason) && + static_cast(reauth_reason) != ReauthReason::NONE) { + UMA_HISTOGRAM_ENUMERATION("Login.ReauthReason", reauth_reason, + NUM_REAUTH_FLOW_REASONS); + user_manager->UpdateReauthReason(user_id, + static_cast(ReauthReason::NONE)); + } +} + +} // namespace chromeos diff --git a/chrome/browser/chromeos/login/reauth_stats.h b/chrome/browser/chromeos/login/reauth_stats.h new file mode 100644 index 00000000000000..99bd159e92c6b5 --- /dev/null +++ b/chrome/browser/chromeos/login/reauth_stats.h @@ -0,0 +1,59 @@ +// Copyright (c) 2015 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. + +#ifndef CHROME_BROWSER_CHROMEOS_LOGIN_REAUTH_STATS_H_ +#define CHROME_BROWSER_CHROMEOS_LOGIN_REAUTH_STATS_H_ + +#include + +namespace chromeos { + +// Track all the ways a user may be sent through the re-auth flow. +// This enum is used to define the buckets for an enumerated UMA histogram. +// Hence, existing enumerated constants should never be reordered, and all new +// constants should only be appended at the end of the enumeration. +enum ReauthReason { + // Default value: no reauth reasons were detected so far, or the reason was + // already reported. + NONE = 0, + + // Legacy profile holders. + OTHER = 1, + + // Password changed, revoked credentials, account deleted. + INVALID_TOKEN_HANDLE = 2, + + // Incorrect password entered 3 times at the user pod. + INCORRECT_PASSWORD_ENTERED = 3, + + // Incorrect password entered by a SAML user once. + // OS would show a tooltip offering user to complete the online sign-in. + INCORRECT_SAML_PASSWORD_ENTERED = 4, + + // Device policy is set not to show user pods, which requires re-auth on every + // login. + SAML_REAUTH_POLICY = 5, + + // Cryptohome is missing, most likely due to deletion during garbage + // collection. + MISSING_CRYPTOHOME = 6, + + // During last login OS failed to connect to the sync with the existing RT. + // This could be due to account deleted, password changed, account revoked, + // etc. + SYNC_FAILED = 7, + + // User cancelled the password change prompt when prompted by Chrome OS. + PASSWORD_UPDATE_SKIPPED = 8, + + // Must be the last value in this list. + NUM_REAUTH_FLOW_REASONS, +}; + +void RecordReauthReason(const std::string& user_id, ReauthReason reason); +void SendReauthReason(const std::string& user_id); + +} // namespace chromeos + +#endif // CHROME_BROWSER_CHROMEOS_LOGIN_REAUTH_STATS_H_ diff --git a/chrome/browser/chromeos/login/saml/saml_offline_signin_limiter.cc b/chrome/browser/chromeos/login/saml/saml_offline_signin_limiter.cc index ba92b40f059bfc..42ee55c28901e3 100644 --- a/chrome/browser/chromeos/login/saml/saml_offline_signin_limiter.cc +++ b/chrome/browser/chromeos/login/saml/saml_offline_signin_limiter.cc @@ -13,6 +13,7 @@ #include "base/prefs/pref_service.h" #include "base/time/clock.h" #include "base/time/time.h" +#include "chrome/browser/chromeos/login/reauth_stats.h" #include "chrome/browser/chromeos/profiles/profile_helper.h" #include "chrome/browser/profiles/profile.h" #include "chrome/common/pref_names.h" @@ -144,6 +145,7 @@ void SAMLOfflineSigninLimiter::ForceOnlineLogin() { } user_manager::UserManager::Get()->SaveForceOnlineSignin(user->email(), true); + RecordReauthReason(user->email(), ReauthReason::SAML_REAUTH_POLICY); offline_signin_limit_timer_.reset(); } diff --git a/chrome/browser/chromeos/login/saml/saml_offline_signin_limiter_unittest.cc b/chrome/browser/chromeos/login/saml/saml_offline_signin_limiter_unittest.cc index beaf843eb41a1b..c82a398527e186 100644 --- a/chrome/browser/chromeos/login/saml/saml_offline_signin_limiter_unittest.cc +++ b/chrome/browser/chromeos/login/saml/saml_offline_signin_limiter_unittest.cc @@ -6,6 +6,7 @@ #include "base/memory/ref_counted.h" #include "base/prefs/pref_service.h" +#include "base/prefs/testing_pref_service.h" #include "base/test/simple_test_clock.h" #include "base/test/test_simple_task_runner.h" #include "base/thread_task_runner_handle.h" @@ -22,7 +23,7 @@ #include "testing/gtest/include/gtest/gtest.h" using testing::Mock; -using testing::ReturnRef; +using testing::Return; using testing::Sequence; using testing::_; @@ -44,6 +45,9 @@ class SAMLOfflineSigninLimiterTest : public testing::Test { void DestroyLimiter(); void CreateLimiter(); + void SetUpUserManager(); + TestingPrefServiceSimple* GetTestingLocalState(); + scoped_refptr runner_; base::ThreadTaskRunnerHandle runner_handle_; @@ -55,6 +59,8 @@ class SAMLOfflineSigninLimiterTest : public testing::Test { SAMLOfflineSigninLimiter* limiter_; // Owned. + TestingPrefServiceSimple testing_local_state_; + DISALLOW_COPY_AND_ASSIGN(SAMLOfflineSigninLimiterTest); }; @@ -88,6 +94,11 @@ void SAMLOfflineSigninLimiterTest::CreateLimiter() { limiter_ = new SAMLOfflineSigninLimiter(profile_.get(), &clock_); } +void SAMLOfflineSigninLimiterTest::SetUpUserManager() { + EXPECT_CALL(*user_manager_, GetLocalState()) + .WillRepeatedly(Return(GetTestingLocalState())); +} + void SAMLOfflineSigninLimiterTest::SetUp() { profile_.reset(new TestingProfile); @@ -95,6 +106,13 @@ void SAMLOfflineSigninLimiterTest::SetUp() { user_manager_->AddUser(kTestUser); profile_->set_profile_name(kTestUser); clock_.Advance(base::TimeDelta::FromHours(1)); + + user_manager_->RegisterPrefs(GetTestingLocalState()->registry()); + SetUpUserManager(); +} + +TestingPrefServiceSimple* SAMLOfflineSigninLimiterTest::GetTestingLocalState() { + return &testing_local_state_; } void SAMLOfflineSigninLimiterTest::TearDown() { @@ -130,6 +148,7 @@ TEST_F(SAMLOfflineSigninLimiterTest, NoSAMLDefaultLimit) { // changed and the time of last login with SAML is not set. CreateLimiter(); Mock::VerifyAndClearExpectations(user_manager_); + SetUpUserManager(); EXPECT_CALL(*user_manager_, SaveForceOnlineSignin(kTestUser, false)).Times(0); EXPECT_CALL(*user_manager_, SaveForceOnlineSignin(kTestUser, true)).Times(0); limiter_->SignedIn(UserContext::AUTH_FLOW_OFFLINE); @@ -174,6 +193,7 @@ TEST_F(SAMLOfflineSigninLimiterTest, NoSAMLNoLimit) { // changed and the time of last login with SAML is not set. CreateLimiter(); Mock::VerifyAndClearExpectations(user_manager_); + SetUpUserManager(); EXPECT_CALL(*user_manager_, SaveForceOnlineSignin(kTestUser, false)).Times(0); EXPECT_CALL(*user_manager_, SaveForceOnlineSignin(kTestUser, true)).Times(0); limiter_->SignedIn(UserContext::AUTH_FLOW_OFFLINE); @@ -218,6 +238,7 @@ TEST_F(SAMLOfflineSigninLimiterTest, NoSAMLZeroLimit) { // changed and the time of last login with SAML is not set. CreateLimiter(); Mock::VerifyAndClearExpectations(user_manager_); + SetUpUserManager(); EXPECT_CALL(*user_manager_, SaveForceOnlineSignin(kTestUser, false)).Times(0); EXPECT_CALL(*user_manager_, SaveForceOnlineSignin(kTestUser, true)).Times(0); limiter_->SignedIn(UserContext::AUTH_FLOW_OFFLINE); @@ -344,6 +365,7 @@ TEST_F(SAMLOfflineSigninLimiterTest, SAMLDefaultLimit) { // login is cleared and the time of last login with SAML is updated. CreateLimiter(); Mock::VerifyAndClearExpectations(user_manager_); + SetUpUserManager(); EXPECT_CALL(*user_manager_, SaveForceOnlineSignin(kTestUser, false)).Times(1); EXPECT_CALL(*user_manager_, SaveForceOnlineSignin(kTestUser, true)).Times(0); limiter_->SignedIn(UserContext::AUTH_FLOW_GAIA_WITH_SAML); @@ -366,6 +388,7 @@ TEST_F(SAMLOfflineSigninLimiterTest, SAMLDefaultLimit) { // time of last login with SAML are not changed. CreateLimiter(); Mock::VerifyAndClearExpectations(user_manager_); + SetUpUserManager(); EXPECT_CALL(*user_manager_, SaveForceOnlineSignin(kTestUser, false)).Times(0); EXPECT_CALL(*user_manager_, SaveForceOnlineSignin(kTestUser, true)).Times(0); limiter_->SignedIn(UserContext::AUTH_FLOW_OFFLINE); @@ -383,6 +406,7 @@ TEST_F(SAMLOfflineSigninLimiterTest, SAMLDefaultLimit) { // Allow the timer to fire. Verify that the flag enforcing online login is // set. Mock::VerifyAndClearExpectations(user_manager_); + SetUpUserManager(); EXPECT_CALL(*user_manager_, SaveForceOnlineSignin(kTestUser, false)).Times(0); EXPECT_CALL(*user_manager_, SaveForceOnlineSignin(kTestUser, true)).Times(1); runner_->RunPendingTasks(); @@ -418,6 +442,7 @@ TEST_F(SAMLOfflineSigninLimiterTest, SAMLNoLimit) { // login is cleared and the time of last login with SAML is updated. CreateLimiter(); Mock::VerifyAndClearExpectations(user_manager_); + SetUpUserManager(); EXPECT_CALL(*user_manager_, SaveForceOnlineSignin(kTestUser, false)).Times(1); EXPECT_CALL(*user_manager_, SaveForceOnlineSignin(kTestUser, true)).Times(0); limiter_->SignedIn(UserContext::AUTH_FLOW_GAIA_WITH_SAML); @@ -440,6 +465,7 @@ TEST_F(SAMLOfflineSigninLimiterTest, SAMLNoLimit) { // time of last login with SAML are not changed. CreateLimiter(); Mock::VerifyAndClearExpectations(user_manager_); + SetUpUserManager(); EXPECT_CALL(*user_manager_, SaveForceOnlineSignin(kTestUser, false)).Times(0); EXPECT_CALL(*user_manager_, SaveForceOnlineSignin(kTestUser, true)).Times(0); limiter_->SignedIn(UserContext::AUTH_FLOW_OFFLINE); @@ -498,6 +524,7 @@ TEST_F(SAMLOfflineSigninLimiterTest, SAMLSetLimitWhileLoggedIn) { // Set a zero time limit. Verify that the flag enforcing online login is set. Mock::VerifyAndClearExpectations(user_manager_); + SetUpUserManager(); EXPECT_CALL(*user_manager_, SaveForceOnlineSignin(kTestUser, false)).Times(0); EXPECT_CALL(*user_manager_, SaveForceOnlineSignin(kTestUser, true)).Times(1); prefs->SetInteger(prefs::kSAMLOfflineSigninTimeLimit, 0); @@ -526,6 +553,7 @@ TEST_F(SAMLOfflineSigninLimiterTest, SAMLRemoveLimit) { // Allow the timer to fire. Verify that the flag enforcing online login is not // changed. Mock::VerifyAndClearExpectations(user_manager_); + SetUpUserManager(); EXPECT_CALL(*user_manager_, SaveForceOnlineSignin(kTestUser, false)).Times(0); EXPECT_CALL(*user_manager_, SaveForceOnlineSignin(kTestUser, true)).Times(0); runner_->RunUntilIdle(); diff --git a/chrome/browser/chromeos/login/screens/user_selection_screen.cc b/chrome/browser/chromeos/login/screens/user_selection_screen.cc index 9bcb4048b14c8d..4b633308ce49b9 100644 --- a/chrome/browser/chromeos/login/screens/user_selection_screen.cc +++ b/chrome/browser/chromeos/login/screens/user_selection_screen.cc @@ -11,6 +11,7 @@ #include "chrome/browser/browser_process.h" #include "chrome/browser/browser_process_platform_part.h" #include "chrome/browser/chromeos/login/lock/screen_locker.h" +#include "chrome/browser/chromeos/login/reauth_stats.h" #include "chrome/browser/chromeos/login/ui/login_display_host_impl.h" #include "chrome/browser/chromeos/login/ui/views/user_board_view.h" #include "chrome/browser/chromeos/login/users/chrome_user_manager.h" @@ -236,6 +237,11 @@ bool UserSelectionScreen::ShouldForceOnlineSignIn( if (is_public_session) return false; + // At this point the reason for invalid token should be already set. If not, + // this might be a leftover from an old version. + if (token_status == user_manager::User::OAUTH2_TOKEN_STATUS_INVALID) + RecordReauthReason(user->email(), ReauthReason::OTHER); + return user->force_online_signin() || (token_status == user_manager::User::OAUTH2_TOKEN_STATUS_INVALID) || (token_status == user_manager::User::OAUTH_TOKEN_STATUS_UNKNOWN); @@ -426,6 +432,7 @@ void UserSelectionScreen::OnUserStatusChecked( const user_manager::UserID& user_id, TokenHandleUtil::TokenHandleStatus status) { if (status == TokenHandleUtil::INVALID) { + RecordReauthReason(user_id, ReauthReason::INVALID_TOKEN_HANDLE); user_manager::UserManager::Get()->SaveUserOAuthStatus( user_id, user_manager::User::OAUTH2_TOKEN_STATUS_INVALID); token_handle_util_->DeleteToken(user_id); diff --git a/chrome/browser/chromeos/login/signin/auth_sync_observer.cc b/chrome/browser/chromeos/login/signin/auth_sync_observer.cc index 6cd4e76f60fedb..8fa574959b7f0d 100644 --- a/chrome/browser/chromeos/login/signin/auth_sync_observer.cc +++ b/chrome/browser/chromeos/login/signin/auth_sync_observer.cc @@ -7,6 +7,7 @@ #include "base/metrics/user_metrics.h" #include "base/metrics/user_metrics_action.h" #include "base/prefs/pref_service.h" +#include "chrome/browser/chromeos/login/reauth_stats.h" #include "chrome/browser/chromeos/login/users/chrome_user_manager.h" #include "chrome/browser/chromeos/login/users/supervised_user_manager.h" #include "chrome/browser/chromeos/profiles/profile_helper.h" @@ -68,6 +69,7 @@ void AuthSyncObserver::OnStateChanged() { user->oauth_token_status(); user_manager::UserManager::Get()->SaveUserOAuthStatus( email, user_manager::User::OAUTH2_TOKEN_STATUS_INVALID); + RecordReauthReason(email, ReauthReason::SYNC_FAILED); if (user->GetType() == user_manager::USER_TYPE_SUPERVISED && old_status != user_manager::User::OAUTH2_TOKEN_STATUS_INVALID) { // Attempt to restore token from file. diff --git a/chrome/browser/resources/chromeos/login/screen_password_changed.js b/chrome/browser/resources/chromeos/login/screen_password_changed.js index 4f22fb833858dc..0bb4f3ab440f56 100644 --- a/chrome/browser/resources/chromeos/login/screen_password_changed.js +++ b/chrome/browser/resources/chromeos/login/screen_password_changed.js @@ -45,7 +45,8 @@ login.createScreen('PasswordChangedScreen', 'password-changed', function() { var gaiaPasswordChanged = $('gaia-password-changed'); gaiaPasswordChanged.addEventListener('cancel', function(e) { - chrome.send('cancelPasswordChangedFlow'); + chrome.send('cancelPasswordChangedFlow', + [$('gaia-password-changed').email]); gaiaPasswordChanged.reset(); }); @@ -142,7 +143,7 @@ login.createScreen('PasswordChangedScreen', 'password-changed', function() { */ cancel: function() { this.disabled = true; - chrome.send('cancelPasswordChangedFlow'); + chrome.send('cancelPasswordChangedFlow', ['']); }, /** diff --git a/chrome/browser/ui/webui/chromeos/login/signin_screen_handler.cc b/chrome/browser/ui/webui/chromeos/login/signin_screen_handler.cc index c631d08da1f776..1b24e33aa92692 100644 --- a/chrome/browser/ui/webui/chromeos/login/signin_screen_handler.cc +++ b/chrome/browser/ui/webui/chromeos/login/signin_screen_handler.cc @@ -32,6 +32,7 @@ #include "chrome/browser/chromeos/login/error_screens_histogram_helper.h" #include "chrome/browser/chromeos/login/hwid_checker.h" #include "chrome/browser/chromeos/login/lock/screen_locker.h" +#include "chrome/browser/chromeos/login/reauth_stats.h" #include "chrome/browser/chromeos/login/screens/core_oobe_actor.h" #include "chrome/browser/chromeos/login/screens/network_error.h" #include "chrome/browser/chromeos/login/startup_utils.h" @@ -511,6 +512,10 @@ void SigninScreenHandler::RegisterMessages() { &SigninScreenHandler::HandleGetTouchViewState); AddCallback("logRemoveUserWarningShown", &SigninScreenHandler::HandleLogRemoveUserWarningShown); + AddCallback("firstIncorrectPasswordAttempt", + &SigninScreenHandler::HandleFirstIncorrectPasswordAttempt); + AddCallback("maxIncorrectPasswordAttempts", + &SigninScreenHandler::HandleMaxIncorrectPasswordAttempts); // This message is sent by the kiosk app menu, but is handled here // so we can tell the delegate to launch the app. @@ -1088,6 +1093,8 @@ void SigninScreenHandler::HandleShowAddUser(const base::ListValue* args) { if (args) args->GetString(0, &email); gaia_screen_handler_->PopulateEmail(email); + if (!email.empty()) + SendReauthReason(email); OnShowAddUser(); } @@ -1192,7 +1199,10 @@ void SigninScreenHandler::HandleLoginVisible(const std::string& source) { OnPreferencesChanged(); } -void SigninScreenHandler::HandleCancelPasswordChangedFlow() { +void SigninScreenHandler::HandleCancelPasswordChangedFlow( + const std::string& user_id) { + if (!user_id.empty()) + RecordReauthReason(user_id, ReauthReason::PASSWORD_UPDATE_SKIPPED); gaia_screen_handler_->StartClearingCookies( base::Bind(&SigninScreenHandler::CancelPasswordChangedFlowInternal, weak_factory_.GetWeakPtr())); @@ -1317,6 +1327,20 @@ void SigninScreenHandler::HandleLogRemoveUserWarningShown() { ProfileMetrics::DELETE_PROFILE_USER_MANAGER_SHOW_WARNING); } +void SigninScreenHandler::HandleFirstIncorrectPasswordAttempt( + const std::string& email) { + // TODO(ginkage): Fix this case once crbug.com/469987 is ready. + /* + if (user_manager::UserManager::Get()->FindUsingSAML(email)) + RecordReauthReason(email, ReauthReason::INCORRECT_SAML_PASSWORD_ENTERED); + */ +} + +void SigninScreenHandler::HandleMaxIncorrectPasswordAttempts( + const std::string& email) { + RecordReauthReason(email, ReauthReason::INCORRECT_PASSWORD_ENTERED); +} + bool SigninScreenHandler::AllWhitelistedUsersPresent() { CrosSettings* cros_settings = CrosSettings::Get(); bool allow_new_user = false; diff --git a/chrome/browser/ui/webui/chromeos/login/signin_screen_handler.h b/chrome/browser/ui/webui/chromeos/login/signin_screen_handler.h index 951600b56888b8..31df3b670526c7 100644 --- a/chrome/browser/ui/webui/chromeos/login/signin_screen_handler.h +++ b/chrome/browser/ui/webui/chromeos/login/signin_screen_handler.h @@ -349,7 +349,7 @@ class SigninScreenHandler void HandleSignOutUser(); void HandleOpenProxySettings(); void HandleLoginVisible(const std::string& source); - void HandleCancelPasswordChangedFlow(); + void HandleCancelPasswordChangedFlow(const std::string& user_id); void HandleCancelUserAdding(); void HandleMigrateUserData(const std::string& password); void HandleResyncUserData(); @@ -367,6 +367,8 @@ class SigninScreenHandler void HandleCancelConsumerManagementEnrollment(); void HandleGetTouchViewState(); void HandleLogRemoveUserWarningShown(); + void HandleFirstIncorrectPasswordAttempt(const std::string& email); + void HandleMaxIncorrectPasswordAttempts(const std::string& email); // Sends the list of |keyboard_layouts| available for the |locale| that is // currently selected for the public session identified by |user_id|. diff --git a/chrome/chrome_browser_chromeos.gypi b/chrome/chrome_browser_chromeos.gypi index 5fc14dc53eda95..d66c9f08aed32f 100644 --- a/chrome/chrome_browser_chromeos.gypi +++ b/chrome/chrome_browser_chromeos.gypi @@ -502,6 +502,8 @@ 'browser/chromeos/login/saml/saml_offline_signin_limiter.h', 'browser/chromeos/login/saml/saml_offline_signin_limiter_factory.cc', 'browser/chromeos/login/saml/saml_offline_signin_limiter_factory.h', + 'browser/chromeos/login/reauth_stats.cc', + 'browser/chromeos/login/reauth_stats.h', 'browser/chromeos/login/screen_manager.cc', 'browser/chromeos/login/screen_manager.h', 'browser/chromeos/login/screens/base_screen.cc', diff --git a/components/user_manager/user_manager.h b/components/user_manager/user_manager.h index 9038c94a88b02b..5d9268ea1f1c6c 100644 --- a/components/user_manager/user_manager.h +++ b/components/user_manager/user_manager.h @@ -348,6 +348,17 @@ class USER_MANAGER_EXPORT UserManager { const std::string& path, const bool in_value) = 0; + // Returns true if |user_id| preference by |path| does exist, + // fills in |out_value|. Otherwise returns false. + virtual bool GetKnownUserIntegerPref(const UserID& user_id, + const std::string& path, + int* out_value) = 0; + + // Updates user's identified by |user_id| integer preference |path|. + virtual void SetKnownUserIntegerPref(const UserID& user_id, + const std::string& path, + const int in_value) = 0; + // Updates |gaia_id| for user with |user_id|. // TODO(antrim): Update this once UserID contains GAIA ID. virtual void UpdateGaiaID(const UserID& user_id, @@ -372,6 +383,15 @@ class USER_MANAGER_EXPORT UserManager { const std::string& device_id) = 0; virtual std::string GetKnownUserDeviceId(const UserID& user_id) = 0; + // Saves why the user has to go through re-auth flow. + virtual void UpdateReauthReason(const UserID& user_id, + const int reauth_reason) = 0; + + // Returns the reason why the user with |user_id| has to go through the + // re-auth flow. Returns true if such a reason was recorded or false + // otherwise. + virtual bool FindReauthReason(const UserID& user_id, int* out_value) = 0; + protected: // Sets UserManager instance. static void SetInstance(UserManager* user_manager); diff --git a/components/user_manager/user_manager_base.cc b/components/user_manager/user_manager_base.cc index 2cee60b6d7984a..8b1feae462c471 100644 --- a/components/user_manager/user_manager_base.cc +++ b/components/user_manager/user_manager_base.cc @@ -88,6 +88,9 @@ const char kUsingSAMLKey[] = "using_saml"; // Key of Device Id. const char kDeviceId[] = "device_id"; +// Key of the reason for re-auth. +const char kReauthReasonKey[] = "reauth_reason"; + // Upper bound for a histogram metric reporting the amount of time between // one regular user logging out and a different regular user logging in. const int kLogoutToLoginDelayMaxSec = 1800; @@ -549,6 +552,16 @@ bool UserManagerBase::FindUsingSAML(const std::string& user_id) { return false; } +void UserManagerBase::UpdateReauthReason(const std::string& user_id, + const int reauth_reason) { + SetKnownUserIntegerPref(user_id, kReauthReasonKey, reauth_reason); +} + +bool UserManagerBase::FindReauthReason(const std::string& user_id, + int* out_value) { + return GetKnownUserIntegerPref(user_id, kReauthReasonKey, out_value); +} + void UserManagerBase::UpdateUserAccountData( const std::string& user_id, const UserAccountData& account_data) { @@ -837,6 +850,7 @@ void UserManagerBase::EnsureUsersLoaded() { } user->set_oauth_token_status(LoadUserOAuthStatus(*it)); user->set_force_online_signin(LoadForceOnlineSignin(*it)); + user->set_using_saml(FindUsingSAML(*it)); users_.push_back(user); base::string16 display_name; @@ -1002,6 +1016,11 @@ bool UserManagerBase::FindKnownUserPrefs( const UserID& user_id, const base::DictionaryValue** out_value) { PrefService* local_state = GetLocalState(); + + // Local State may not be initialized in tests. + if (!local_state) + return false; + const base::ListValue* known_users = local_state->GetList(kKnownUsers); for (size_t i = 0; i < known_users->GetSize(); ++i) { const base::DictionaryValue* element = nullptr; @@ -1018,7 +1037,13 @@ bool UserManagerBase::FindKnownUserPrefs( void UserManagerBase::UpdateKnownUserPrefs(const UserID& user_id, const base::DictionaryValue& values, bool clear) { - ListPrefUpdate update(GetLocalState(), kKnownUsers); + PrefService* local_state = GetLocalState(); + + // Local State may not be initialized in tests. + if (!local_state) + return; + + ListPrefUpdate update(local_state, kKnownUsers); for (size_t i = 0; i < update->GetSize(); ++i) { base::DictionaryValue* element = nullptr; if (update->GetDictionary(i, &element)) { @@ -1050,7 +1075,13 @@ bool UserManagerBase::GetKnownUserStringPref(const UserID& user_id, void UserManagerBase::SetKnownUserStringPref(const UserID& user_id, const std::string& path, const std::string& in_value) { - ListPrefUpdate update(GetLocalState(), kKnownUsers); + PrefService* local_state = GetLocalState(); + + // Local State may not be initialized in tests. + if (!local_state) + return; + + ListPrefUpdate update(local_state, kKnownUsers); base::DictionaryValue dict; dict.SetString(path, in_value); UpdateKnownUserPrefs(user_id, dict, false); @@ -1069,12 +1100,42 @@ bool UserManagerBase::GetKnownUserBooleanPref(const UserID& user_id, void UserManagerBase::SetKnownUserBooleanPref(const UserID& user_id, const std::string& path, const bool in_value) { - ListPrefUpdate update(GetLocalState(), kKnownUsers); + PrefService* local_state = GetLocalState(); + + // Local State may not be initialized in tests. + if (!local_state) + return; + + ListPrefUpdate update(local_state, kKnownUsers); base::DictionaryValue dict; dict.SetBoolean(path, in_value); UpdateKnownUserPrefs(user_id, dict, false); } +bool UserManagerBase::GetKnownUserIntegerPref(const UserID& user_id, + const std::string& path, + int* out_value) { + const base::DictionaryValue* user_pref_dict = nullptr; + if (!FindKnownUserPrefs(user_id, &user_pref_dict)) + return false; + return user_pref_dict->GetInteger(path, out_value); +} + +void UserManagerBase::SetKnownUserIntegerPref(const UserID& user_id, + const std::string& path, + const int in_value) { + PrefService* local_state = GetLocalState(); + + // Local State may not be initialized in tests. + if (!local_state) + return; + + ListPrefUpdate update(local_state, kKnownUsers); + base::DictionaryValue dict; + dict.SetInteger(path, in_value); + UpdateKnownUserPrefs(user_id, dict, false); +} + void UserManagerBase::UpdateGaiaID(const UserID& user_id, const std::string& gaia_id) { SetKnownUserStringPref(user_id, kGAIAIdKey, gaia_id); diff --git a/components/user_manager/user_manager_base.h b/components/user_manager/user_manager_base.h index d61f8fbd370eda..d8454efa620387 100644 --- a/components/user_manager/user_manager_base.h +++ b/components/user_manager/user_manager_base.h @@ -123,6 +123,12 @@ class USER_MANAGER_EXPORT UserManagerBase : public UserManager { void SetKnownUserBooleanPref(const UserID& user_id, const std::string& path, const bool in_value) override; + bool GetKnownUserIntegerPref(const UserID& user_id, + const std::string& path, + int* out_value) override; + void SetKnownUserIntegerPref(const UserID& user_id, + const std::string& path, + const int in_value) override; void UpdateGaiaID(const UserID& user_id, const std::string& gaia_id) override; bool FindGaiaID(const UserID& user_id, std::string* out_value) override; void UpdateUsingSAML(const std::string& user_id, @@ -131,6 +137,9 @@ class USER_MANAGER_EXPORT UserManagerBase : public UserManager { void SetKnownUserDeviceId(const UserID& user_id, const std::string& device_id) override; std::string GetKnownUserDeviceId(const UserID& user_id) override; + void UpdateReauthReason(const std::string& user_id, + const int reauth_reason) override; + bool FindReauthReason(const std::string& user_id, int* out_value) override; virtual void SetIsCurrentUserNew(bool is_new); diff --git a/tools/metrics/histograms/histograms.xml b/tools/metrics/histograms/histograms.xml index 9ad3aa1344e79f..5a59757455e351 100644 --- a/tools/metrics/histograms/histograms.xml +++ b/tools/metrics/histograms/histograms.xml @@ -13886,6 +13886,13 @@ Therefore, the affected-histogram name has to have at least one dot in it. + + ginkage@chromium.org + + Tracks the reason why a user was sent through the GAIA re-auth flow. + + + mnissler@chromium.org @@ -56033,6 +56040,41 @@ To add a new entry, add it with any value and run test to compute valid value. Reserved + + + No reason recorded so far, nothing to report. This value should never be + reported in histogram. + + Legacy profile holders. + + Password changed, revoked credentials, account deleted. + + + Incorrect password entered 3 times at the pod. + + + Incorrect password entered by a SAML user once. OS would show a tooltip + offering user to complete the online sign-in. + + + Company policy required re-auth for SAML users. + + + Cryptohome is missing, most likely due to deletion during garbage + collection. + + + After prior login, OS failed to connect to the sync with the existing RT. + This could be due to a deleted account, password changed, account revoked, + etc. + + + User cancelled the password change prompt. Chrome OS has to continue to send + the user through the online flow until user updates their cryptohome + password or agrees to start the new cryptohome. + + + The result of a state key generation operation. diff --git a/ui/login/account_picker/screen_account_picker.js b/ui/login/account_picker/screen_account_picker.js index 61e6f54ea02eb4..7f7635d7c96ed3 100644 --- a/ui/login/account_picker/screen_account_picker.js +++ b/ui/login/account_picker/screen_account_picker.js @@ -167,8 +167,14 @@ login.createScreen('AccountPickerScreen', 'account-picker', function() { // Show web authentication if this is not a supervised user. if (loginAttempts > MAX_LOGIN_ATTEMPTS_IN_POD && !activatedPod.user.supervisedUser) { + chrome.send('maxIncorrectPasswordAttempts', + [activatedPod.user.emailAddress]); activatedPod.showSigninUI(); } else { + if (loginAttempts == 1) { + chrome.send('firstIncorrectPasswordAttempt', + [activatedPod.user.emailAddress]); + } // We want bubble's arrow to point to the first letter of input. /** @const */ var BUBBLE_OFFSET = 7; /** @const */ var BUBBLE_PADDING = 4;