Skip to content

Commit

Permalink
UMA to track the reason for re-auth
Browse files Browse the repository at this point in the history
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}
  • Loading branch information
nkostylev authored and Commit bot committed May 7, 2015
1 parent 179b32c commit eb9ce0d
Show file tree
Hide file tree
Showing 16 changed files with 319 additions and 8 deletions.
10 changes: 10 additions & 0 deletions chrome/browser/chromeos/login/existing_user_controller.cc
Original file line number Diff line number Diff line change
Expand Up @@ -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"
Expand Down Expand Up @@ -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) {
Expand Down Expand Up @@ -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);
Expand Down
36 changes: 36 additions & 0 deletions chrome/browser/chromeos/login/reauth_stats.cc
Original file line number Diff line number Diff line change
@@ -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<ReauthReason>(old_reason) == ReauthReason::NONE &&
reason != ReauthReason::NONE)) {
user_manager->UpdateReauthReason(user_id, static_cast<int>(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<ReauthReason>(reauth_reason) != ReauthReason::NONE) {
UMA_HISTOGRAM_ENUMERATION("Login.ReauthReason", reauth_reason,
NUM_REAUTH_FLOW_REASONS);
user_manager->UpdateReauthReason(user_id,
static_cast<int>(ReauthReason::NONE));
}
}

} // namespace chromeos
59 changes: 59 additions & 0 deletions chrome/browser/chromeos/login/reauth_stats.h
Original file line number Diff line number Diff line change
@@ -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 <string>

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_
Original file line number Diff line number Diff line change
Expand Up @@ -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"
Expand Down Expand Up @@ -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();
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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"
Expand All @@ -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::_;

Expand All @@ -44,6 +45,9 @@ class SAMLOfflineSigninLimiterTest : public testing::Test {
void DestroyLimiter();
void CreateLimiter();

void SetUpUserManager();
TestingPrefServiceSimple* GetTestingLocalState();

scoped_refptr<base::TestSimpleTaskRunner> runner_;
base::ThreadTaskRunnerHandle runner_handle_;

Expand All @@ -55,6 +59,8 @@ class SAMLOfflineSigninLimiterTest : public testing::Test {

SAMLOfflineSigninLimiter* limiter_; // Owned.

TestingPrefServiceSimple testing_local_state_;

DISALLOW_COPY_AND_ASSIGN(SAMLOfflineSigninLimiterTest);
};

Expand Down Expand Up @@ -88,13 +94,25 @@ 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);

SAMLOfflineSigninLimiterFactory::SetClockForTesting(&clock_);
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() {
Expand Down Expand Up @@ -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);
Expand Down Expand Up @@ -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);
Expand Down Expand Up @@ -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);
Expand Down Expand Up @@ -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);
Expand All @@ -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);
Expand All @@ -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();
Expand Down Expand Up @@ -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);
Expand All @@ -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);
Expand Down Expand Up @@ -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);
Expand Down Expand Up @@ -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();
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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"
Expand Down Expand Up @@ -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);
Expand Down Expand Up @@ -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);
Expand Down
2 changes: 2 additions & 0 deletions chrome/browser/chromeos/login/signin/auth_sync_observer.cc
Original file line number Diff line number Diff line change
Expand Up @@ -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"
Expand Down Expand Up @@ -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.
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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();
});

Expand Down Expand Up @@ -142,7 +143,7 @@ login.createScreen('PasswordChangedScreen', 'password-changed', function() {
*/
cancel: function() {
this.disabled = true;
chrome.send('cancelPasswordChangedFlow');
chrome.send('cancelPasswordChangedFlow', ['']);
},

/**
Expand Down
Loading

0 comments on commit eb9ce0d

Please sign in to comment.