KerberosCredentialsManager: Listen to pref changes

Adds code so KerberosCredentialsManager can listen to the 4 Kerberos
prefs and handles kerberos.enabled (removes all existing accounts).

Moves the registration of 3 prefs from profile to local_state since
those prefs were not per-profile, anyway, and it simplifies lifetime of
KerberosCredentialsManager as it does not have to wait for the primary
user to be available.

Also changes lifetime from a static singleton to explicit management
since KerberosCredentialsManager owns a pointer to local_state now and
it would otherwise be hard to justify that the pointer is never
dangling.

TBR=battre@chromium.org

BUG=chromium:952239
TEST=Turn KerberosEnabled policy on, navigate to
     chrome://settings/kerberosAccounts, add an account,
     Turn KerberosEnabled policy off, verify that the account vanishes.

Change-Id: I3f4ed443014726d8b2c5f2cc536703badf37d2ae
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1617455
Commit-Queue: Lutz Justen <ljusten@chromium.org>
Reviewed-by: Dominic Battré <battre@chromium.org>
Reviewed-by: Xiyuan Xia <xiyuan@chromium.org>
Reviewed-by: Pavol Marko <pmarko@chromium.org>
Cr-Commit-Position: refs/heads/master@{#664432}

chrome/browser/chromeos/chrome_browser_main_chromeos.cc

@@ -64,6 +64,7 @@
 #include "chrome/browser/chromeos/extensions/login_screen_ui/login_screen_extension_ui_handler.h"
 #include "chrome/browser/chromeos/external_metrics.h"
 #include "chrome/browser/chromeos/input_method/input_method_configuration.h"
+#include "chrome/browser/chromeos/kerberos/kerberos_credentials_manager.h"
 #include "chrome/browser/chromeos/language_preferences.h"
 #include "chrome/browser/chromeos/lock_screen_apps/state_controller.h"
 #include "chrome/browser/chromeos/logging.h"
@@ -962,6 +963,9 @@ void ChromeBrowserMainPartsChromeos::PostProfileInit() {
   g_browser_process->platform_part()->InitializeAutomaticRebootManager();
   user_removal_manager::RemoveUsersIfNeeded();
 
+  kerberos_credentials_manager_ = std::make_unique<KerberosCredentialsManager>(
+      g_browser_process->local_state());
+
   // This observer cannot be created earlier because it requires the shell to be
   // available.
   idle_action_warning_observer_ = std::make_unique<IdleActionWarningObserver>();
@@ -1121,6 +1125,7 @@ void ChromeBrowserMainPartsChromeos::PostMainMessageLoopRun() {
   lock_to_single_user_manager_.reset();
   wilco_dtc_supportd_manager_.reset();
   gnubby_notification_.reset();
+  kerberos_credentials_manager_.reset();
 
   // Detach D-Bus clients before DBusThreadManager is shut down.
   idle_action_warning_observer_.reset();

chrome/browser/chromeos/chrome_browser_main_chromeos.h

@@ -44,6 +44,7 @@ class DiscoverManager;
 class EventRewriterDelegateImpl;
 class FastTransitionObserver;
 class IdleActionWarningObserver;
+class KerberosCredentialsManager;
 class LowDiskNotification;
 class NetworkChangeManagerClient;
 class NetworkPrefStateObserver;
@@ -173,6 +174,7 @@ class ChromeBrowserMainPartsChromeos : public ChromeBrowserMainPartsLinux {
   std::unique_ptr<SessionTerminationManager> session_termination_manager_;
   std::unique_ptr<policy::LockToSingleUserManager> lock_to_single_user_manager_;
   std::unique_ptr<WilcoDtcSupportdManager> wilco_dtc_supportd_manager_;
+  std::unique_ptr<KerberosCredentialsManager> kerberos_credentials_manager_;
 
   std::unique_ptr<GnubbyNotification> gnubby_notification_;
 

chrome/browser/chromeos/kerberos/kerberos_credentials_manager.cc

@@ -6,16 +6,16 @@
 
 #include "base/bind.h"
 #include "base/strings/string_split.h"
+#include "base/strings/string_util.h"
 #include "base/strings/stringprintf.h"
-#include "chrome/browser/browser_process.h"
-#include "chrome/browser/browser_process_platform_part_chromeos.h"
+#include "base/threading/thread_task_runner_handle.h"
 #include "chrome/browser/chromeos/authpolicy/data_pipe_utils.h"
-#include "chrome/browser/chromeos/policy/browser_policy_connector_chromeos.h"
 #include "chrome/common/pref_names.h"
 #include "chromeos/dbus/kerberos/kerberos_client.h"
 #include "chromeos/dbus/kerberos/kerberos_service.pb.h"
-#include "chromeos/dbus/upstart/upstart_client.h"
+#include "components/prefs/pref_change_registrar.h"
 #include "components/prefs/pref_registry_simple.h"
+#include "components/prefs/pref_service.h"
 #include "dbus/message.h"
 #include "third_party/cros_system_api/dbus/kerberos/dbus-constants.h"
 
@@ -23,6 +23,8 @@ namespace chromeos {
 
 namespace {
 
+KerberosCredentialsManager* g_instance = nullptr;
+
 // Default encryption with strong encryption.
 constexpr char kDefaultKerberosConfigFmt[] = R"(
 [libdefaults]
@@ -228,40 +230,64 @@ KerberosCredentialsManager::Observer::Observer() = default;
 
 KerberosCredentialsManager::Observer::~Observer() = default;
 
-KerberosCredentialsManager::KerberosCredentialsManager()
-    : kerberos_files_handler_(
+KerberosCredentialsManager::KerberosCredentialsManager(PrefService* local_state)
+    : local_state_(local_state),
+      kerberos_files_handler_(
           base::BindRepeating(&KerberosCredentialsManager::GetKerberosFiles,
                               base::Unretained(this))) {
+  DCHECK(!g_instance);
+  g_instance = this;
+
   // Connect to a signal that indicates when Kerberos files change.
+  // TODO(https://crbug.com/963824): Make sure no code inside this constructor
+  // causes the daemon to start.
   KerberosClient::Get()->ConnectToKerberosFileChangedSignal(
       base::BindRepeating(&KerberosCredentialsManager::OnKerberosFilesChanged,
                           weak_factory_.GetWeakPtr()));
+
+  // Listen to pref changes.
+  pref_change_registrar_ = std::make_unique<PrefChangeRegistrar>();
+  pref_change_registrar_->Init(local_state);
+  pref_change_registrar_->Add(
+      prefs::kKerberosEnabled,
+      base::BindRepeating(&KerberosCredentialsManager::UpdateEnabledFromPref,
+                          weak_factory_.GetWeakPtr()));
+  pref_change_registrar_->Add(
+      prefs::kKerberosRememberPasswordEnabled,
+      base::BindRepeating(
+          &KerberosCredentialsManager::UpdateRememberPasswordEnabledFromPref,
+          weak_factory_.GetWeakPtr()));
+  pref_change_registrar_->Add(
+      prefs::kKerberosAddAccountsAllowed,
+      base::BindRepeating(
+          &KerberosCredentialsManager::UpdateAddAccountsAllowedFromPref,
+          weak_factory_.GetWeakPtr()));
+  pref_change_registrar_->Add(
+      prefs::kKerberosAccounts,
+      base::BindRepeating(&KerberosCredentialsManager::UpdateAccountsFromPref,
+                          weak_factory_.GetWeakPtr()));
 }
 
-KerberosCredentialsManager::~KerberosCredentialsManager() = default;
+KerberosCredentialsManager::~KerberosCredentialsManager() {
+  DCHECK(g_instance);
+  g_instance = nullptr;
+}
 
 // static
 KerberosCredentialsManager& KerberosCredentialsManager::Get() {
-  static base::NoDestructor<KerberosCredentialsManager> instance;
-  return *instance;
+  DCHECK(g_instance);
+  return *g_instance;
 }
 
 // static
-void KerberosCredentialsManager::RegisterProfilePrefs(
+void KerberosCredentialsManager::RegisterLocalStatePrefs(
     PrefRegistrySimple* registry) {
+  registry->RegisterBooleanPref(prefs::kKerberosEnabled, false);
   registry->RegisterBooleanPref(prefs::kKerberosRememberPasswordEnabled, true);
   registry->RegisterBooleanPref(prefs::kKerberosAddAccountsAllowed, true);
   registry->RegisterDictionaryPref(prefs::kKerberosAccounts);
 }
 
-// static
-void KerberosCredentialsManager::RegisterLocalStatePrefs(
-    PrefRegistrySimple* registry) {
-  // Kerberos enabled is used by SystemNetworkContextManager, which reads prefs
-  // off of local state.
-  registry->RegisterBooleanPref(prefs::kKerberosEnabled, false);
-}
-
 void KerberosCredentialsManager::AddObserver(Observer* observer) {
   observers_.AddObserver(observer);
 }
@@ -348,6 +374,29 @@ void KerberosCredentialsManager::OnRemoveAccount(
   std::move(callback).Run(response.error());
 }
 
+void KerberosCredentialsManager::ClearAccounts(ResultCallback callback) {
+  kerberos::ClearAccountsRequest request;
+  KerberosClient::Get()->ClearAccounts(
+      request, base::BindOnce(&KerberosCredentialsManager::OnClearAccounts,
+                              weak_factory_.GetWeakPtr(), std::move(callback)));
+}
+
+void KerberosCredentialsManager::OnClearAccounts(
+    ResultCallback callback,
+    const kerberos::ClearAccountsResponse& response) {
+  LogError("ClearAccounts", response.error());
+  if (Succeeded(response.error())) {
+    // Clear out active credentials.
+    kerberos_files_handler_.DeleteFiles();
+    active_principal_name_.clear();
+
+    // Tattle on the lost accounts to the observers.
+    NotifyAccountsChanged();
+  }
+
+  std::move(callback).Run(response.error());
+}
+
 void KerberosCredentialsManager::ListAccounts(ListAccountsCallback callback) {
   kerberos::ListAccountsRequest request;
   KerberosClient::Get()->ListAccounts(
@@ -460,4 +509,24 @@ void KerberosCredentialsManager::NotifyAccountsChanged() {
     observer.OnAccountsChanged();
 }
 
+void KerberosCredentialsManager::UpdateEnabledFromPref() {
+  const bool enabled = local_state_->GetBoolean(prefs::kKerberosEnabled);
+  if (!enabled) {
+    // Note that ClearAccounts logs an error if the operation fails.
+    ClearAccounts(base::BindOnce([](kerberos::ErrorType) {}));
+  }
+}
+
+void KerberosCredentialsManager::UpdateRememberPasswordEnabledFromPref() {
+  // TODO(https://crbug.com/952239): Implement
+}
+
+void KerberosCredentialsManager::UpdateAddAccountsAllowedFromPref() {
+  // TODO(https://crbug.com/952239): Implement
+}
+
+void KerberosCredentialsManager::UpdateAccountsFromPref() {
+  // TODO(https://crbug.com/952239): Implement
+}
+
 }  // namespace chromeos

chrome/browser/chromeos/kerberos/kerberos_credentials_manager.h

@@ -15,12 +15,14 @@
 #include "chromeos/dbus/kerberos/kerberos_service.pb.h"
 
 class PrefRegistrySimple;
+class PrefService;
+class PrefChangeRegistrar;
 
 namespace chromeos {
 
 class KerberosAddAccountRunner;
 
-class KerberosCredentialsManager {
+class KerberosCredentialsManager final {
  public:
   using ResultCallback = base::OnceCallback<void(kerberos::ErrorType)>;
   using ListAccountsCallback =
@@ -39,15 +41,12 @@ class KerberosCredentialsManager {
     DISALLOW_COPY_AND_ASSIGN(Observer);
   };
 
-  KerberosCredentialsManager();
+  explicit KerberosCredentialsManager(PrefService* local_state);
   ~KerberosCredentialsManager();
 
-  // Singleton accessor.
+  // Singleton accessor. DCHECKs if the instance has not been created yet.
   static KerberosCredentialsManager& Get();
 
-  // Registers prefs stored in the user profile.
-  static void RegisterProfilePrefs(PrefRegistrySimple* registry);
-
   // Registers prefs stored in local state.
   static void RegisterLocalStatePrefs(PrefRegistrySimple* registry);
 
@@ -68,6 +67,10 @@ class KerberosCredentialsManager {
   // On success, calls OnAccountsChanged() for observers.
   void RemoveAccount(std::string principal_name, ResultCallback callback);
 
+  // Removes all Kerberos accounts.
+  // On success, calls OnAccountsChanged() for observers.
+  void ClearAccounts(ResultCallback callback);
+
   // Returns a list of all existing accounts, including current status like
   // remaining Kerberos ticket lifetime.
   void ListAccounts(ListAccountsCallback callback);
@@ -101,6 +104,10 @@ class KerberosCredentialsManager {
                        ResultCallback callback,
                        const kerberos::RemoveAccountResponse& response);
 
+  // Callback for ClearAccounts().
+  void OnClearAccounts(ResultCallback callback,
+                       const kerberos::ClearAccountsResponse& response);
+
   // Callback for RemoveAccount().
   void OnListAccounts(ListAccountsCallback callback,
                       const kerberos::ListAccountsResponse& response);
@@ -127,9 +134,21 @@ class KerberosCredentialsManager {
   // Calls OnAccountsChanged() on all observers.
   void NotifyAccountsChanged();
 
+  // Pref change handlers.
+  void UpdateEnabledFromPref();
+  void UpdateRememberPasswordEnabledFromPref();
+  void UpdateAddAccountsAllowedFromPref();
+  void UpdateAccountsFromPref();
+
+  // Local state prefs, not owned.
+  PrefService* local_state_ = nullptr;
+
   // Called by OnSignalConnected(), puts Kerberos files where GSSAPI finds them.
   KerberosFilesHandler kerberos_files_handler_;
 
+  // Observer for Kerberos-related prefs.
+  std::unique_ptr<PrefChangeRegistrar> pref_change_registrar_;
+
   // Handles the steps to add a Kerberos account.
   std::unique_ptr<KerberosAddAccountRunner> add_account_runner_;
 

chrome/browser/prefs/browser_prefs.cc

@@ -821,7 +821,6 @@ void RegisterProfilePrefs(user_prefs::PrefRegistrySyncable* registry,
   chromeos::CupsPrintersManager::RegisterProfilePrefs(registry);
   chromeos::first_run::RegisterProfilePrefs(registry);
   chromeos::file_system_provider::RegisterProfilePrefs(registry);
-  chromeos::KerberosCredentialsManager::RegisterProfilePrefs(registry);
   chromeos::KeyPermissions::RegisterProfilePrefs(registry);
   chromeos::multidevice_setup::MultiDeviceSetupService::RegisterProfilePrefs(
       registry);

chrome/test/data/policy/policy_test_cases.json

@@ -993,7 +993,8 @@
     "os": ["chromeos"],
     "test_policy": { "KerberosRememberPasswordEnabled": false },
     "pref_mappings": [
-      { "pref": "kerberos.remember_password_enabled" }
+      { "pref": "kerberos.remember_password_enabled",
+        "local_state": true }
     ]
   },
 
@@ -1001,7 +1002,8 @@
     "os": ["chromeos"],
     "test_policy": { "KerberosAddAccountsAllowed": false },
     "pref_mappings": [
-      { "pref": "kerberos.add_accounts_allowed" }
+      { "pref": "kerberos.add_accounts_allowed",
+        "local_state": true }
     ]
   },
 
@@ -1024,7 +1026,8 @@
       ]
     },
     "pref_mappings": [
-      { "pref": "kerberos.accounts" }
+      { "pref": "kerberos.accounts",
+        "local_state": true }
     ]
   },