Add unit tests for RemoveAccount() on KerberosCredentialsManager

Third batch of KerberosCredentialsManager unit tests. This time, adding
tests for the RemoveAccount() method.

Bug: 952251
Change-Id: Ifaed8c6d290e604cc63677e5697528523b9b4074
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1919212
Commit-Queue: Felipe Andrade <fsandrade@chromium.org>
Reviewed-by: Lutz Justen <ljusten@chromium.org>
Cr-Commit-Position: refs/heads/master@{#718106}

chrome/browser/chromeos/authpolicy/kerberos_files_handler.h

@@ -38,14 +38,14 @@ extern const char kKrb5ConfFile[];
 class KerberosFilesHandler {
  public:
   explicit KerberosFilesHandler(base::RepeatingClosure get_kerberos_files);
-  ~KerberosFilesHandler();
+  virtual ~KerberosFilesHandler();
 
   // Writes the Kerberos credentials to disk asynchronously.
   void SetFiles(base::Optional<std::string> krb5cc,
                 base::Optional<std::string> krb5conf);
 
   // Deletes the Kerberos credentials from disk asynchronously.
-  void DeleteFiles();
+  virtual void DeleteFiles();
 
   // Sets a callback for when disk IO task posted by SetFiles has finished.
   void SetFilesChangedForTesting(base::OnceClosure callback);

chrome/browser/chromeos/kerberos/kerberos_credentials_manager.cc

@@ -278,9 +278,9 @@ KerberosCredentialsManager::KerberosCredentialsManager(PrefService* local_state,
                                                        Profile* primary_profile)
     : local_state_(local_state),
       primary_profile_(primary_profile),
-      kerberos_files_handler_(
+      kerberos_files_handler_(std::make_unique<KerberosFilesHandler>(
           base::BindRepeating(&KerberosCredentialsManager::GetKerberosFiles,
-                              base::Unretained(this))) {
+                              base::Unretained(this)))) {
   DCHECK(!g_instance);
   g_instance = this;
 
@@ -672,10 +672,10 @@ void KerberosCredentialsManager::OnGetKerberosFiles(
   // ticket. In that case, the files must go.
   if (response.files().has_krb5cc()) {
     DCHECK(response.files().has_krb5conf());
-    kerberos_files_handler_.SetFiles(response.files().krb5cc(),
-                                     response.files().krb5conf());
+    kerberos_files_handler_->SetFiles(response.files().krb5cc(),
+                                      response.files().krb5conf());
   } else {
-    kerberos_files_handler_.DeleteFiles();
+    kerberos_files_handler_->DeleteFiles();
   }
 }
 
@@ -724,7 +724,7 @@ void KerberosCredentialsManager::SetActivePrincipalName(
 
 void KerberosCredentialsManager::ClearActivePrincipalName() {
   primary_profile_->GetPrefs()->ClearPref(prefs::kKerberosActivePrincipalName);
-  kerberos_files_handler_.DeleteFiles();
+  kerberos_files_handler_->DeleteFiles();
 }
 
 void KerberosCredentialsManager::ValidateActivePrincipal(
@@ -899,4 +899,15 @@ void KerberosCredentialsManager::OnTicketExpiryNotificationClick(
   kerberos_ticket_expiry_notification::Close(primary_profile_);
 }
 
+base::RepeatingClosure
+KerberosCredentialsManager::GetGetKerberosFilesCallbackForTesting() {
+  return base::BindRepeating(&KerberosCredentialsManager::GetKerberosFiles,
+                             base::Unretained(this));
+}
+
+void KerberosCredentialsManager::SetKerberosFilesHandlerForTesting(
+    std::unique_ptr<KerberosFilesHandler> kerberos_files_handler) {
+  kerberos_files_handler_ = std::move(kerberos_files_handler);
+}
+
 }  // namespace chromeos

chrome/browser/chromeos/kerberos/kerberos_credentials_manager.h

@@ -147,6 +147,15 @@ class KerberosCredentialsManager : public policy::PolicyService::Observer {
     return GetActivePrincipalName();
   }
 
+  // Getter for the GetKerberosFiles() callback, used on tests to build a mock
+  // KerberosFilesHandler.
+  base::RepeatingClosure GetGetKerberosFilesCallbackForTesting();
+
+  // Used on tests to replace the KerberosFilesHandler created on the
+  // constructor with a mock KerberosFilesHandler.
+  void SetKerberosFilesHandlerForTesting(
+      std::unique_ptr<KerberosFilesHandler> kerberos_files_handler);
+
  private:
   friend class KerberosAddAccountRunner;
   using RepeatedAccountField =
@@ -247,7 +256,7 @@ class KerberosCredentialsManager : public policy::PolicyService::Observer {
   policy::PolicyService* policy_service_ = nullptr;
 
   // Called by OnSignalConnected(), puts Kerberos files where GSSAPI finds them.
-  KerberosFilesHandler kerberos_files_handler_;
+  std::unique_ptr<KerberosFilesHandler> kerberos_files_handler_;
 
   // Observer for Kerberos-related prefs.
   std::unique_ptr<PrefChangeRegistrar> pref_change_registrar_;

chrome/browser/chromeos/kerberos/kerberos_credentials_manager_test.cc

@@ -15,6 +15,7 @@
 #include "base/run_loop.h"
 #include "base/stl_util.h"
 #include "base/test/bind_test_util.h"
+#include "chrome/browser/chromeos/authpolicy/kerberos_files_handler.h"
 #include "chrome/browser/chromeos/login/users/mock_user_manager.h"
 #include "chrome/browser/notifications/notification_display_service_tester.h"
 #include "chrome/common/pref_names.h"
@@ -26,6 +27,7 @@
 #include "components/user_manager/scoped_user_manager.h"
 #include "components/user_manager/user_manager.h"
 #include "content/public/test/browser_task_environment.h"
+#include "testing/gmock/include/gmock/gmock.h"
 #include "testing/gtest/include/gtest/gtest.h"
 
 namespace chromeos {
@@ -98,6 +100,17 @@ class FakeKerberosCredentialsManagerObserver
   // OnAccountsChanged() call.
   int accounts_count_at_last_notification_ = 0;
 };
+
+class MockKerberosFilesHandler : public KerberosFilesHandler {
+ public:
+  explicit MockKerberosFilesHandler(base::RepeatingClosure get_kerberos_files)
+      : KerberosFilesHandler(get_kerberos_files) {}
+
+  ~MockKerberosFilesHandler() = default;
+
+  MOCK_METHOD(void, DeleteFiles, ());
+};
+
 }  // namespace
 
 class KerberosCredentialsManagerTest : public testing::Test {
@@ -206,6 +219,25 @@ class KerberosCredentialsManagerTest : public testing::Test {
                         expected_accounts_count);
   }
 
+  void RemoveAccount(const char* principal_name,
+                     kerberos::ErrorType expected_error,
+                     int expected_notifications_count,
+                     int expected_accounts_count) {
+    base::RunLoop run_loop;
+    mgr_->RemoveAccount(
+        principal_name,
+        base::BindLambdaForTesting([&](const kerberos::ErrorType error) {
+          EXPECT_EQ(expected_error, error);
+          run_loop.Quit();
+        }));
+    run_loop.Run();
+
+    EXPECT_EQ(expected_notifications_count, observer_.notifications_count());
+    EXPECT_EQ(expected_accounts_count,
+              observer_.accounts_count_at_last_notification());
+    observer_.Reset();
+  }
+
   // Calls |mgr_->ListAccounts()|, waits for the result and expects success.
   // Returns the list of accounts.
   Accounts ListAccounts() {
@@ -529,13 +561,109 @@ TEST_F(KerberosCredentialsManagerTest,
   EXPECT_EQ(kNormalizedOtherPrincipal, mgr_->GetActiveAccount());
 }
 
+// RemoveAccount fails with ERROR_PARSE_PRINCIPAL_FAILED when a bad principal
+// name is passed in.
+TEST_F(KerberosCredentialsManagerTest, RemoveAccountFailsForBadPrincipal) {
+  const kerberos::ErrorType expected_error =
+      kerberos::ERROR_PARSE_PRINCIPAL_FAILED;
+  RemoveAccount(kBadPrincipal1, expected_error, kNoNotification, kNoAccount);
+  RemoveAccount(kBadPrincipal2, expected_error, kNoNotification, kNoAccount);
+  RemoveAccount(kBadPrincipal3, expected_error, kNoNotification, kNoAccount);
+  RemoveAccount(kBadPrincipal4, expected_error, kNoNotification, kNoAccount);
+  RemoveAccount(kBadPrincipal5, expected_error, kNoNotification, kNoAccount);
+}
+
+// RemoveAccount normalizes |principal_name| before trying to find account.
+TEST_F(KerberosCredentialsManagerTest, RemoveAccountNormalizesPrincipalName) {
+  AddAccountAndAuthenticate(kPrincipal, kerberos::ERROR_NONE, kOneNotification,
+                            kOneAccount);
+  RemoveAccount(kPrincipal, kerberos::ERROR_NONE, kOneNotification, kNoAccount);
+}
+
+// RemoveAccount removes last account, should clear the active principal.
+TEST_F(KerberosCredentialsManagerTest,
+       RemoveAccountRemoveLastAccountClearsActivePrincipal) {
+  AddAccountAndAuthenticate(kPrincipal, kerberos::ERROR_NONE, kOneNotification,
+                            kOneAccount);
+  EXPECT_EQ(kNormalizedPrincipal, mgr_->GetActiveAccount());
+  client_test_interface()->StartRecordingFunctionCalls();
+
+  RemoveAccount(kPrincipal, kerberos::ERROR_NONE, kOneNotification, kNoAccount);
+
+  std::string calls =
+      client_test_interface()->StopRecordingAndGetRecordedFunctionCalls();
+  EXPECT_EQ(calls, "RemoveAccount");
+  EXPECT_TRUE(mgr_->GetActiveAccount().empty());
+}
+
+// RemoveAccount removes last account, should delete kerberos files.
+TEST_F(KerberosCredentialsManagerTest,
+       RemoveAccountRemoveLastAccountDeletesKerberosFiles) {
+  auto files_handler = std::make_unique<MockKerberosFilesHandler>(
+      mgr_->GetGetKerberosFilesCallbackForTesting());
+  EXPECT_CALL(*files_handler, DeleteFiles());
+  mgr_->SetKerberosFilesHandlerForTesting(std::move(files_handler));
+  AddAccountAndAuthenticate(kPrincipal, kerberos::ERROR_NONE, kOneNotification,
+                            kOneAccount);
+
+  RemoveAccount(kPrincipal, kerberos::ERROR_NONE, kOneNotification, kNoAccount);
+}
+
+// RemoveAccount removes active account while another account is available,
+// should set a new active principal.
+TEST_F(KerberosCredentialsManagerTest,
+       RemoveAccountRemoveActiveAccountAnotherAccountAvailable) {
+  AddAccountAndAuthenticate(kPrincipal, kerberos::ERROR_NONE, kOneNotification,
+                            kOneAccount);
+  AddAccountAndAuthenticate(kOtherPrincipal, kerberos::ERROR_NONE,
+                            kOneNotification, kTwoAccounts);
+  EXPECT_EQ(kNormalizedOtherPrincipal, mgr_->GetActiveAccount());
+  client_test_interface()->StartRecordingFunctionCalls();
+
+  RemoveAccount(kOtherPrincipal, kerberos::ERROR_NONE, kOneNotification,
+                kOneAccount);
+
+  std::string calls =
+      client_test_interface()->StopRecordingAndGetRecordedFunctionCalls();
+  EXPECT_EQ(calls, "RemoveAccount,GetKerberosFiles");
+  EXPECT_EQ(kNormalizedPrincipal, mgr_->GetActiveAccount());
+}
+
+// RemoveAccount removes non-active account, no change on active account.
+TEST_F(KerberosCredentialsManagerTest, RemoveAccountNonActiveAccount) {
+  AddAccountAndAuthenticate(kPrincipal, kerberos::ERROR_NONE, kOneNotification,
+                            kOneAccount);
+  AddAccountAndAuthenticate(kOtherPrincipal, kerberos::ERROR_NONE,
+                            kOneNotification, kTwoAccounts);
+  EXPECT_EQ(kNormalizedOtherPrincipal, mgr_->GetActiveAccount());
+
+  client_test_interface()->StartRecordingFunctionCalls();
+  RemoveAccount(kPrincipal, kerberos::ERROR_NONE, kOneNotification,
+                kOneAccount);
+
+  std::string calls =
+      client_test_interface()->StopRecordingAndGetRecordedFunctionCalls();
+  EXPECT_EQ(calls, "RemoveAccount");
+  EXPECT_EQ(kNormalizedOtherPrincipal, mgr_->GetActiveAccount());
+}
+
+// RemoveAccount fails to remove unknown account.
+TEST_F(KerberosCredentialsManagerTest, RemoveAccountFailsUnknownAccount) {
+  AddAccountAndAuthenticate(kPrincipal, kerberos::ERROR_NONE, kOneNotification,
+                            kOneAccount);
+  EXPECT_EQ(kNormalizedPrincipal, mgr_->GetActiveAccount());
+  client_test_interface()->StartRecordingFunctionCalls();
+
+  RemoveAccount(kOtherPrincipal, kerberos::ERROR_UNKNOWN_PRINCIPAL_NAME,
+                kNoNotification, kNoAccount);
+
+  std::string calls =
+      client_test_interface()->StopRecordingAndGetRecordedFunctionCalls();
+  EXPECT_EQ(calls, "RemoveAccount");
+  EXPECT_EQ(kNormalizedPrincipal, mgr_->GetActiveAccount());
+}
+
 // TODO(https://crbug.com/952251): Add more tests
-// - RemoveAccount
-//     + Normalization like in AddAccountAndAuthenticate
-//     + Calls the RemoveAccount KerberosClient method
-//     + On success and if active principal was removed, sets a new active
-//       principal (empty if no accounts left)
-//     + On success, calls OnAccountsChanged on observers
 // - ClearAccounts
 //     + Normalization like in AddAccountAndAuthenticate
 //     + Calls the ClearAccounts KerberosClient method