From 949cedf66caf68d8a942ff5efab6c20f6f1438ab Mon Sep 17 00:00:00 2001
From: Kostas Kyrimis <kostas@dragonflydb.io>
Date: Thu, 28 Sep 2023 09:22:45 +0300
Subject: [PATCH] fix(AclFamily): do not allow to delete default user (#1954)

* do not allow to delete default user
* upon loading an acl file, if default does not exist create them
* add test
---
 src/server/acl/acl_family.cc       | 10 ++++++++++
 src/server/acl/acl_family_test.cc  |  3 +++
 src/server/acl/user_registry.cc    | 12 ++++++++----
 src/server/acl/user_registry.h     |  2 ++
 tests/dragonfly/acl_family_test.py |  3 ++-
 5 files changed, 25 insertions(+), 5 deletions(-)

diff --git a/src/server/acl/acl_family.cc b/src/server/acl/acl_family.cc
index f6cc6f096..3210cd500 100644
--- a/src/server/acl/acl_family.cc
+++ b/src/server/acl/acl_family.cc
@@ -125,6 +125,11 @@ void AclFamily::EvictOpenConnectionsOnAllProactors(std::string_view user) {
 
 void AclFamily::DelUser(CmdArgList args, ConnectionContext* cntx) {
   std::string_view username = facade::ToSV(args[0]);
+  if (username == "default") {
+    cntx->SendError("The'default' user cannot be removed");
+    return;
+  }
+
   auto& registry = *registry_;
   if (!registry.RemoveUser(username)) {
     cntx->SendError(absl::StrCat("User ", username, " does not exist"));
@@ -255,6 +260,11 @@ std::optional<facade::ErrorReply> AclFamily::LoadToRegistryFromFile(std::string_
     commands.push_back(user.AclCommands());
   }
 
+  if (!registry.contains("default")) {
+    auto& user = registry["default"];
+    user.Update(registry_->DefaultUserUpdateRequest());
+  }
+
   if (!init) {
     StreamUpdatesToAllProactorConnections(usernames, categories, commands);
   }
diff --git a/src/server/acl/acl_family_test.cc b/src/server/acl/acl_family_test.cc
index 31ef66040..2b1c3e484 100644
--- a/src/server/acl/acl_family_test.cc
+++ b/src/server/acl/acl_family_test.cc
@@ -44,6 +44,9 @@ TEST_F(AclFamilyTest, AclDelUser) {
   auto resp = Run({"ACL", "DELUSER"});
   EXPECT_THAT(resp, ErrArg("ERR wrong number of arguments for 'acl deluser' command"));
 
+  resp = Run({"ACL", "DELUSER", "default"});
+  EXPECT_THAT(resp, ErrArg("ERR The'default' user cannot be removed"));
+
   resp = Run({"ACL", "DELUSER", "NOTEXISTS"});
   EXPECT_THAT(resp, ErrArg("ERR User NOTEXISTS does not exist"));
 
diff --git a/src/server/acl/user_registry.cc b/src/server/acl/user_registry.cc
index 9288735ce..7639f8c06 100644
--- a/src/server/acl/user_registry.cc
+++ b/src/server/acl/user_registry.cc
@@ -76,16 +76,20 @@ UserRegistry::UserWithWriteLock UserRegistry::MaybeAddAndUpdateWithLock(std::str
   return {std::move(lock), user, exists};
 }
 
-void UserRegistry::Init() {
-  // Add default user
+User::UpdateRequest UserRegistry::DefaultUserUpdateRequest() const {
   User::UpdateRequest::CommandsUpdateType tmp(NumberOfFamilies());
   size_t id = 0;
   for (auto& elem : tmp) {
     elem = {User::Sign::PLUS, id++, acl::ALL_COMMANDS};
   }
   std::pair<User::Sign, uint32_t> acl{User::Sign::PLUS, acl::ALL};
-  User::UpdateRequest req{{}, {acl}, true, false, std::move(tmp)};
-  MaybeAddAndUpdate("default", std::move(req));
+  return {{}, {acl}, true, false, std::move(tmp)};
+}
+
+void UserRegistry::Init() {
+  // Add default user
+  User::UpdateRequest::CommandsUpdateType tmp(NumberOfFamilies());
+  MaybeAddAndUpdate("default", DefaultUserUpdateRequest());
 }
 
 }  // namespace dfly::acl
diff --git a/src/server/acl/user_registry.h b/src/server/acl/user_registry.h
index 43169dd50..11c8eae65 100644
--- a/src/server/acl/user_registry.h
+++ b/src/server/acl/user_registry.h
@@ -79,6 +79,8 @@ class UserRegistry {
 
   UserWithWriteLock MaybeAddAndUpdateWithLock(std::string_view username, User::UpdateRequest req);
 
+  User::UpdateRequest DefaultUserUpdateRequest() const;
+
  private:
   RegistryType registry_;
   mutable util::SharedMutex mu_;
diff --git a/tests/dragonfly/acl_family_test.py b/tests/dragonfly/acl_family_test.py
index ff4f76b0f..ebd0b9818 100644
--- a/tests/dragonfly/acl_family_test.py
+++ b/tests/dragonfly/acl_family_test.py
@@ -344,9 +344,10 @@ async def test_good_acl_file(df_local_factory, tmp_dir):
     result = await client.execute_command("ACL LOAD")
 
     result = await client.execute_command("ACL LIST")
-    assert 2 == len(result)
+    assert 3 == len(result)
     assert "user roy on ea71c25a7a60224 +@STRING +HSET" in result
     assert "user vlad off nopass +@STRING" in result
+    assert "user default on nopass +@ALL +ALL" in result
 
     await client.close()