From c547f8172c6b7c28bc7c4fb75419f039614eabd6 Mon Sep 17 00:00:00 2001 From: "tbarzic@chromium.org" Date: Fri, 13 Sep 2013 23:36:37 +0000 Subject: Fix device policy recovery on CrOS login If the device policy file cannot be loaded on login, the device enters a state where it allows login process to proceed only if the user logging in is the owner. To be able to successfully determine if the user is owner, parallel authenticator has to wait until the certificates are loaded by CertLoader. The problem is that the CertLoader starts loading the certificates _after_ the user actually logs in. So if the authenticator actually waited for the owner status to be resolved, the login would hang. This CL adds another state to LoginState::LoggedInState enum (SAFE_MODE) in which CertLoader will be allowed to start loading the certificates. When the authenticator detects the policy file corruption, it changes login state to SAFE MODE (which triggers the certificate loading) and waits for the DeviceSettingsService to determine whether the current user is the owner. Also, removed LoginState::GetLoggedInState. Replaced it's pre-existing usages with LoginState::IsUserLoggedIn; and added LoginState::IsInSafeMode to be used here. BUG=285450 TEST= 1. manually remove device policy file 2. try logging in to a non-owner account -> should fail 3. try logging in to the owner account -> should succeed 4. try logging in to a non-owner account again -> should succeed Review URL: https://chromiumcodereview.appspot.com/23684033 git-svn-id: svn://svn.chromium.org/chrome/trunk/src@223168 0039d316-1c4b-4281-b951-d872f2087c98 --- .../chromeos/login/parallel_authenticator.cc | 16 ++++-- .../chromeos/login/parallel_authenticator.h | 2 +- .../login/parallel_authenticator_unittest.cc | 23 +++++++- .../chromeos/system/ash_system_tray_delegate.cc | 14 ++--- .../options/chromeos/internet_options_handler.cc | 3 +- .../options/chromeos/internet_options_handler.h | 3 +- chromeos/cert_loader.cc | 18 +++--- chromeos/cert_loader.h | 2 +- chromeos/login/login_state.cc | 19 ++++--- chromeos/login/login_state.h | 18 +++--- chromeos/login/login_state_unittest.cc | 66 ++++++++++++++++++---- chromeos/network/network_connection_handler.cc | 8 +-- chromeos/network/network_connection_handler.h | 2 +- 13 files changed, 131 insertions(+), 63 deletions(-) diff --git a/chrome/browser/chromeos/login/parallel_authenticator.cc b/chrome/browser/chromeos/login/parallel_authenticator.cc index 1f85f8d..54b5aaf 100644 --- a/chrome/browser/chromeos/login/parallel_authenticator.cc +++ b/chrome/browser/chromeos/login/parallel_authenticator.cc @@ -22,6 +22,7 @@ #include "chromeos/cryptohome/cryptohome_library.h" #include "chromeos/dbus/cryptohome_client.h" #include "chromeos/dbus/dbus_thread_manager.h" +#include "chromeos/login/login_state.h" #include "content/public/browser/browser_thread.h" #include "content/public/browser/notification_service.h" #include "crypto/sha2.h" @@ -517,17 +518,20 @@ bool ParallelAuthenticator::VerifyOwner() { // Now we can continue reading the private key. DeviceSettingsService::Get()->SetUsername( current_state_->user_context.username); - DeviceSettingsService::Get()->GetOwnershipStatusAsync( + // This should trigger certificate loading, which is needed in order to + // correctly determine if the current user is the owner. + if (LoginState::IsInitialized()) { + LoginState::Get()->SetLoggedInState(LoginState::LOGGED_IN_SAFE_MODE, + LoginState::LOGGED_IN_USER_NONE); + } + DeviceSettingsService::Get()->IsCurrentUserOwnerAsync( base::Bind(&ParallelAuthenticator::OnOwnershipChecked, this)); return false; } -void ParallelAuthenticator::OnOwnershipChecked( - DeviceSettingsService::OwnershipStatus status) { +void ParallelAuthenticator::OnOwnershipChecked(bool is_owner) { // Now we can check if this user is the owner. - // TODO(tbarzic): This is broken. At this point, DeviceSettingsService will - // never have private key loaded (http://crbug.com/285450). - user_can_login_ = DeviceSettingsService::Get()->HasPrivateOwnerKey(); + user_can_login_ = is_owner; owner_is_verified_ = true; Resolve(); } diff --git a/chrome/browser/chromeos/login/parallel_authenticator.h b/chrome/browser/chromeos/login/parallel_authenticator.h index 5689f81..0338c47 100644 --- a/chrome/browser/chromeos/login/parallel_authenticator.h +++ b/chrome/browser/chromeos/login/parallel_authenticator.h @@ -220,7 +220,7 @@ class ParallelAuthenticator : public Authenticator, bool VerifyOwner(); // Handles completion of the ownership check and continues login. - void OnOwnershipChecked(DeviceSettingsService::OwnershipStatus status); + void OnOwnershipChecked(bool is_owner); // Signal login completion status for cases when a new user is added via // an external authentication provider (i.e. GAIA extension). diff --git a/chrome/browser/chromeos/login/parallel_authenticator_unittest.cc b/chrome/browser/chromeos/login/parallel_authenticator_unittest.cc index 7569a9d..e1896f2 100644 --- a/chrome/browser/chromeos/login/parallel_authenticator_unittest.cc +++ b/chrome/browser/chromeos/login/parallel_authenticator_unittest.cc @@ -6,6 +6,7 @@ #include +#include "base/command_line.h" #include "base/file_util.h" #include "base/files/file_path.h" #include "base/memory/scoped_ptr.h" @@ -22,6 +23,7 @@ #include "chrome/browser/chromeos/settings/device_settings_test_helper.h" #include "chrome/browser/chromeos/settings/stub_cros_settings_provider.h" #include "chrome/test/base/testing_profile.h" +#include "chromeos/chromeos_switches.h" #include "chromeos/cryptohome/mock_async_method_caller.h" #include "chromeos/cryptohome/mock_cryptohome_library.h" #include "chromeos/dbus/fake_cryptohome_client.h" @@ -55,7 +57,8 @@ class ParallelAuthenticatorTest : public testing::Test { : username_("me@nowhere.org"), password_("fakepass"), hash_ascii_("0a010000000000a0" + std::string(16, '0')), - user_manager_enabler_(new MockUserManager) { + user_manager_enabler_(new MockUserManager), + mock_caller_(NULL) { } virtual ~ParallelAuthenticatorTest() { @@ -63,6 +66,8 @@ class ParallelAuthenticatorTest : public testing::Test { } virtual void SetUp() { + CommandLine::ForCurrentProcess()->AppendSwitch(switches::kLoginManager); + mock_caller_ = new cryptohome::MockAsyncMethodCaller; cryptohome::AsyncMethodCaller::InitializeForTesting(mock_caller_); @@ -327,9 +332,21 @@ TEST_F(ParallelAuthenticatorTest, ResolveOwnerNeededFailedMount) { CrosSettings::Get()->AddSettingsProvider(&stub_settings_provider); CrosSettings::Get()->SetBoolean(kPolicyMissingMitigationMode, true); + // Initialize login state for this test to verify the login state is changed + // to SAFE_MODE. + LoginState::Initialize(); + EXPECT_EQ(ParallelAuthenticator::CONTINUE, SetAndResolveState(auth_.get(), state_.release())); - // Let the owner verification run. + EXPECT_TRUE(LoginState::Get()->IsInSafeMode()); + + // Simulate certificates load event. The exact certificates loaded are not + // actually used by the DeviceSettingsService, so it is OK to pass an empty + // list. + DeviceSettingsService::Get()->OnCertificatesLoaded(net::CertificateList(), + true); + // Flush all the pending operations. The operations should induce an owner + // verification. device_settings_test_helper_.Flush(); // and test that the mount has succeeded. state_.reset(new TestAttemptState(UserContext(username_, @@ -344,6 +361,8 @@ TEST_F(ParallelAuthenticatorTest, ResolveOwnerNeededFailedMount) { EXPECT_EQ(ParallelAuthenticator::OWNER_REQUIRED, SetAndResolveState(auth_.get(), state_.release())); + // Unset global objects used by this test. + LoginState::Shutdown(); EXPECT_TRUE( CrosSettings::Get()->RemoveSettingsProvider(&stub_settings_provider)); CrosSettings::Get()->AddSettingsProvider(device_settings_provider); diff --git a/chrome/browser/chromeos/system/ash_system_tray_delegate.cc b/chrome/browser/chromeos/system/ash_system_tray_delegate.cc index 652c7da..86435cb 100644 --- a/chrome/browser/chromeos/system/ash_system_tray_delegate.cc +++ b/chrome/browser/chromeos/system/ash_system_tray_delegate.cc @@ -471,12 +471,11 @@ class SystemTrayDelegate : public ash::SystemTrayDelegate, } virtual ash::user::LoginStatus GetUserLoginStatus() const OVERRIDE { - // Map ChromeOS specific LOGGED_IN states to Ash LOGGED_IN states. - LoginState::LoggedInState state = LoginState::Get()->GetLoggedInState(); - if (state == LoginState::LOGGED_IN_OOBE || - state == LoginState::LOGGED_IN_NONE) { + // All non-logged in ChromeOS specific LOGGED_IN states map to the same + // Ash specific LOGGED_IN state. + if (!LoginState::Get()->IsUserLoggedIn()) return ash::user::LOGGED_IN_NONE; - } + if (screen_locked_) return ash::user::LOGGED_IN_LOCKED; @@ -506,7 +505,7 @@ class SystemTrayDelegate : public ash::SystemTrayDelegate, virtual bool IsOobeCompleted() const OVERRIDE { if (!base::chromeos::IsRunningOnChromeOS() && - LoginState::Get()->GetLoggedInState() == LoginState::LOGGED_IN_ACTIVE) + LoginState::Get()->IsUserLoggedIn()) return true; return StartupUtils::IsOobeCompleted(); } @@ -1082,8 +1081,7 @@ class SystemTrayDelegate : public ash::SystemTrayDelegate, } // LoginState::Observer overrides. - virtual void LoggedInStateChanged( - chromeos::LoginState::LoggedInState state) OVERRIDE { + virtual void LoggedInStateChanged() OVERRIDE { UpdateClockType(); } diff --git a/chrome/browser/ui/webui/options/chromeos/internet_options_handler.cc b/chrome/browser/ui/webui/options/chromeos/internet_options_handler.cc index 5f1032c..6c19b83 100644 --- a/chrome/browser/ui/webui/options/chromeos/internet_options_handler.cc +++ b/chrome/browser/ui/webui/options/chromeos/internet_options_handler.cc @@ -1346,8 +1346,7 @@ void InternetOptionsHandler::NetworkPropertiesUpdated( UpdateConnectionData(network->path()); } -void InternetOptionsHandler::LoggedInStateChanged( - LoginState::LoggedInState state) { +void InternetOptionsHandler::LoggedInStateChanged() { UpdateLoggedInUserType(); } diff --git a/chrome/browser/ui/webui/options/chromeos/internet_options_handler.h b/chrome/browser/ui/webui/options/chromeos/internet_options_handler.h index 07666e5..d1abde6 100644 --- a/chrome/browser/ui/webui/options/chromeos/internet_options_handler.h +++ b/chrome/browser/ui/webui/options/chromeos/internet_options_handler.h @@ -95,8 +95,7 @@ class InternetOptionsHandler const chromeos::NetworkState* network) OVERRIDE; // chromeos::LoginState::Observer - virtual void LoggedInStateChanged( - chromeos::LoginState::LoggedInState) OVERRIDE; + virtual void LoggedInStateChanged() OVERRIDE; // Updates the logged in user type. void UpdateLoggedInUserType(); diff --git a/chromeos/cert_loader.cc b/chromeos/cert_loader.cc index 060d464..62a4f12 100644 --- a/chromeos/cert_loader.cc +++ b/chromeos/cert_loader.cc @@ -145,10 +145,14 @@ void CertLoader::MaybeRequestCertificates() { if (certificates_requested_ || !crypto_task_runner_.get()) return; - const bool logged_in = LoginState::IsInitialized() ? - LoginState::Get()->IsUserLoggedIn() : false; - VLOG(1) << "RequestCertificates: " << logged_in; - if (!logged_in) + if (!LoginState::IsInitialized()) + return; + + bool request_certificates = LoginState::Get()->IsUserLoggedIn() || + LoginState::Get()->IsInSafeMode(); + + VLOG(1) << "RequestCertificates: " << request_certificates; + if (!request_certificates) return; certificates_requested_ = true; @@ -159,7 +163,7 @@ void CertLoader::MaybeRequestCertificates() { tpm_token_state_ = TPM_DISABLED; // Treat TPM as disabled for guest users since they do not store certs. - if (LoginState::IsInitialized() && LoginState::Get()->IsGuestUser()) + if (LoginState::Get()->IsGuestUser()) tpm_token_state_ = TPM_DISABLED; InitializeTokenAndLoadCertificates(); @@ -387,8 +391,8 @@ void CertLoader::OnCertRemoved(const net::X509Certificate* cert) { LoadCertificates(); } -void CertLoader::LoggedInStateChanged(LoginState::LoggedInState state) { - VLOG(1) << "LoggedInStateChanged: " << state; +void CertLoader::LoggedInStateChanged() { + VLOG(1) << "LoggedInStateChanged"; MaybeRequestCertificates(); } diff --git a/chromeos/cert_loader.h b/chromeos/cert_loader.h index bc1123e..9c5ace4 100644 --- a/chromeos/cert_loader.h +++ b/chromeos/cert_loader.h @@ -147,7 +147,7 @@ class CHROMEOS_EXPORT CertLoader : public net::CertDatabase::Observer, virtual void OnCertRemoved(const net::X509Certificate* cert) OVERRIDE; // LoginState::Observer - virtual void LoggedInStateChanged(LoginState::LoggedInState state) OVERRIDE; + virtual void LoggedInStateChanged() OVERRIDE; bool initialize_tpm_for_test_; diff --git a/chromeos/login/login_state.cc b/chromeos/login/login_state.cc index 8c8c329..57a889b 100644 --- a/chromeos/login/login_state.cc +++ b/chromeos/login/login_state.cc @@ -67,22 +67,23 @@ void LoginState::SetLoggedInState(LoggedInState state, NotifyObservers(); } -LoginState::LoggedInState LoginState::GetLoggedInState() const { - if (AlwaysLoggedIn()) - return LOGGED_IN_ACTIVE; - return logged_in_state_; -} - LoginState::LoggedInUserType LoginState::GetLoggedInUserType() const { return logged_in_user_type_; } bool LoginState::IsUserLoggedIn() const { - return GetLoggedInState() == LOGGED_IN_ACTIVE; + if (AlwaysLoggedIn()) + return true; + return logged_in_state_ == LOGGED_IN_ACTIVE; +} + +bool LoginState::IsInSafeMode() const { + DCHECK(!AlwaysLoggedIn() || logged_in_state_ != LOGGED_IN_SAFE_MODE); + return logged_in_state_ == LOGGED_IN_SAFE_MODE; } bool LoginState::IsGuestUser() const { - if (GetLoggedInState() != LOGGED_IN_ACTIVE) + if (!IsUserLoggedIn()) return false; switch (logged_in_user_type_) { case LOGGED_IN_USER_NONE: @@ -124,7 +125,7 @@ LoginState::~LoginState() { void LoginState::NotifyObservers() { FOR_EACH_OBSERVER(LoginState::Observer, observer_list_, - LoggedInStateChanged(GetLoggedInState())); + LoggedInStateChanged()); } } // namespace chromeos diff --git a/chromeos/login/login_state.h b/chromeos/login/login_state.h index 68966f5..0ef18a7 100644 --- a/chromeos/login/login_state.h +++ b/chromeos/login/login_state.h @@ -15,9 +15,10 @@ namespace chromeos { class CHROMEOS_EXPORT LoginState { public: enum LoggedInState { - LOGGED_IN_OOBE, // Out of box experience not completed - LOGGED_IN_NONE, // Not logged in - LOGGED_IN_ACTIVE // A user has logged in + LOGGED_IN_OOBE, // Out of box experience not completed + LOGGED_IN_NONE, // Not logged in + LOGGED_IN_SAFE_MODE, // Not logged in and login not allowed for non-owners + LOGGED_IN_ACTIVE // A user has logged in }; enum LoggedInUserType { @@ -34,7 +35,7 @@ class CHROMEOS_EXPORT LoginState { class Observer { public: // Called when either the login state or the logged in user type changes. - virtual void LoggedInStateChanged(LoggedInState state) = 0; + virtual void LoggedInStateChanged() = 0; protected: virtual ~Observer() {} @@ -53,13 +54,16 @@ class CHROMEOS_EXPORT LoginState { // Set the logged in state and user type. void SetLoggedInState(LoggedInState state, LoggedInUserType type); - // Get the logged in state / user type. - LoggedInState GetLoggedInState() const; + // Get the logged in user type. LoggedInUserType GetLoggedInUserType() const; - // Returns true if |logged_in_state_| is active. + // Returns true if a user is considered to be logged in. bool IsUserLoggedIn() const; + // Returns true if |logged_in_state_| is safe mode (i.e. the user is not yet + // logged in, and only the owner will be allowed to log in). + bool IsInSafeMode() const; + // Returns true if logged in and is a guest, retail, public, or kiosk user. bool IsGuestUser() const; diff --git a/chromeos/login/login_state_unittest.cc b/chromeos/login/login_state_unittest.cc index 0665745..d71b9d5 100644 --- a/chromeos/login/login_state_unittest.cc +++ b/chromeos/login/login_state_unittest.cc @@ -16,9 +16,8 @@ namespace chromeos { class LoginStateTest : public testing::Test, public LoginState::Observer { public: - LoginStateTest() - : logged_in_state_(LoginState::LOGGED_IN_OOBE), - logged_in_user_type_(LoginState::LOGGED_IN_USER_NONE) { + LoginStateTest() : logged_in_user_type_(LoginState::LOGGED_IN_USER_NONE), + login_state_changes_count_(0) { } virtual ~LoginStateTest() { } @@ -39,38 +38,81 @@ class LoginStateTest : public testing::Test, } // LoginState::Observer - virtual void LoggedInStateChanged(LoginState::LoggedInState state) OVERRIDE { - logged_in_state_ = state; + virtual void LoggedInStateChanged() OVERRIDE { + ++login_state_changes_count_; logged_in_user_type_ = LoginState::Get()->GetLoggedInUserType(); } protected: + // Returns number of times the login state changed since the last call to + // this method. + unsigned int GetNewLoginStateChangesCount() { + unsigned int result = login_state_changes_count_; + login_state_changes_count_ = 0; + return result; + } + base::MessageLoopForUI message_loop_; - LoginState::LoggedInState logged_in_state_; LoginState::LoggedInUserType logged_in_user_type_; private: + unsigned int login_state_changes_count_; + DISALLOW_COPY_AND_ASSIGN(LoginStateTest); }; TEST_F(LoginStateTest, TestLoginState) { - EXPECT_EQ(LoginState::LOGGED_IN_OOBE, logged_in_state_); - EXPECT_EQ(LoginState::LOGGED_IN_OOBE, LoginState::Get()->GetLoggedInState()); + EXPECT_FALSE(LoginState::Get()->IsUserLoggedIn()); + EXPECT_FALSE(LoginState::Get()->IsInSafeMode()); EXPECT_EQ(LoginState::LOGGED_IN_USER_NONE, logged_in_user_type_); EXPECT_EQ(LoginState::LOGGED_IN_USER_NONE, LoginState::Get()->GetLoggedInUserType()); + // Setting login state to ACTIVE. LoginState::Get()->SetLoggedInState(LoginState::LOGGED_IN_ACTIVE, LoginState::LOGGED_IN_USER_REGULAR); - EXPECT_EQ(LoginState::LOGGED_IN_ACTIVE, - LoginState::Get()->GetLoggedInState()); EXPECT_EQ(LoginState::LOGGED_IN_USER_REGULAR, LoginState::Get()->GetLoggedInUserType()); EXPECT_TRUE(LoginState::Get()->IsUserLoggedIn()); + EXPECT_FALSE(LoginState::Get()->IsInSafeMode()); + // Run the message loop, observer should update members. message_loop_.RunUntilIdle(); - EXPECT_EQ(LoginState::LOGGED_IN_ACTIVE, logged_in_state_); + EXPECT_EQ(1U, GetNewLoginStateChangesCount()); EXPECT_EQ(LoginState::LOGGED_IN_USER_REGULAR, logged_in_user_type_); } -} // namespace +TEST_F(LoginStateTest, TestSafeModeLoginState) { + EXPECT_FALSE(LoginState::Get()->IsUserLoggedIn()); + EXPECT_FALSE(LoginState::Get()->IsInSafeMode()); + EXPECT_EQ(LoginState::LOGGED_IN_USER_NONE, logged_in_user_type_); + EXPECT_EQ(LoginState::LOGGED_IN_USER_NONE, + LoginState::Get()->GetLoggedInUserType()); + // Setting login state to SAFE MODE. + LoginState::Get()->SetLoggedInState(LoginState::LOGGED_IN_SAFE_MODE, + LoginState::LOGGED_IN_USER_NONE); + EXPECT_EQ(LoginState::LOGGED_IN_USER_NONE, + LoginState::Get()->GetLoggedInUserType()); + EXPECT_FALSE(LoginState::Get()->IsUserLoggedIn()); + EXPECT_TRUE(LoginState::Get()->IsInSafeMode()); + + // Run the message loop, observer should update members. + message_loop_.RunUntilIdle(); + EXPECT_EQ(1U, GetNewLoginStateChangesCount()); + EXPECT_EQ(LoginState::LOGGED_IN_USER_NONE, logged_in_user_type_); + + // Setting login state to ACTIVE. + LoginState::Get()->SetLoggedInState(LoginState::LOGGED_IN_ACTIVE, + LoginState::LOGGED_IN_USER_OWNER); + EXPECT_EQ(LoginState::LOGGED_IN_USER_OWNER, + LoginState::Get()->GetLoggedInUserType()); + EXPECT_TRUE(LoginState::Get()->IsUserLoggedIn()); + EXPECT_FALSE(LoginState::Get()->IsInSafeMode()); + + // Run the message loop, observer should update members. + message_loop_.RunUntilIdle(); + EXPECT_EQ(1U, GetNewLoginStateChangesCount()); + EXPECT_EQ(LoginState::LOGGED_IN_USER_OWNER, logged_in_user_type_); +} + +} // namespace chromeos diff --git a/chromeos/network/network_connection_handler.cc b/chromeos/network/network_connection_handler.cc index dfcba54..6600e56 100644 --- a/chromeos/network/network_connection_handler.cc +++ b/chromeos/network/network_connection_handler.cc @@ -157,8 +157,7 @@ void NetworkConnectionHandler::Init( NetworkConfigurationHandler* network_configuration_handler) { if (LoginState::IsInitialized()) { LoginState::Get()->AddObserver(this); - logged_in_ = - LoginState::Get()->GetLoggedInState() == LoginState::LOGGED_IN_ACTIVE; + logged_in_ = LoginState::Get()->IsUserLoggedIn(); } if (CertLoader::IsInitialized()) { cert_loader_ = CertLoader::Get(); @@ -175,9 +174,8 @@ void NetworkConnectionHandler::Init( network_configuration_handler_ = network_configuration_handler; } -void NetworkConnectionHandler::LoggedInStateChanged( - LoginState::LoggedInState state) { - if (state == LoginState::LOGGED_IN_ACTIVE) { +void NetworkConnectionHandler::LoggedInStateChanged() { + if (LoginState::Get()->IsUserLoggedIn()) { logged_in_ = true; NET_LOG_EVENT("Logged In", ""); } diff --git a/chromeos/network/network_connection_handler.h b/chromeos/network/network_connection_handler.h index 326e579..d67a41c 100644 --- a/chromeos/network/network_connection_handler.h +++ b/chromeos/network/network_connection_handler.h @@ -126,7 +126,7 @@ class CHROMEOS_EXPORT NetworkConnectionHandler virtual void NetworkPropertiesUpdated(const NetworkState* network) OVERRIDE; // LoginState::Observer - virtual void LoggedInStateChanged(LoginState::LoggedInState state) OVERRIDE; + virtual void LoggedInStateChanged() OVERRIDE; // CertLoader::Observer virtual void OnCertificatesLoaded(const net::CertificateList& cert_list, -- cgit v1.1