feat(acl): add validation for acl keys

src/facade/acl_commands_def.h

@@ -4,8 +4,24 @@
 
 #pragma once
 
+#include <cstdint>
+#include <limits>
+#include <string>
+#include <utility>
+#include <vector>
+
 namespace dfly::acl {
 // Special flag/mask for all
 constexpr uint32_t NONE = 0;
 constexpr uint32_t ALL = std::numeric_limits<uint32_t>::max();
+
+enum class KeyOp : int8_t { READ, WRITE, READ_WRITE };
+
+using GlobType = std::pair<std::string, KeyOp>;
+
+struct AclKeys {
+  std::vector<GlobType> key_globs;
+  bool all_keys = false;
+};
+
 }  // namespace dfly::acl

src/facade/conn_context.h

@@ -85,11 +85,14 @@ class ConnectionContext {
   // How many async subscription sources are active: monitor and/or pubsub - at most 2.
   uint8_t subscriptions;
 
+  // TODO fix inherit actual values from default
   std::string authed_username{"default"};
   uint32_t acl_categories{dfly::acl::ALL};
   std::vector<uint64_t> acl_commands;
   // Skip ACL validation, used by internal commands and commands run on admin port
   bool skip_acl_validation = false;
+  // keys
+  dfly::acl::AclKeys keys{{}, true};
 
  private:
   Connection* owner_;

src/facade/dragonfly_connection.cc

@@ -8,6 +8,7 @@
 #include <absl/strings/match.h>
 #include <mimalloc.h>
 
+#include <numeric>
 #include <variant>
 
 #include "absl/strings/str_cat.h"
@@ -205,9 +206,12 @@ size_t Connection::MessageHandle::UsedMemory() const {
       return msg.capacity();
     }
     size_t operator()(const AclUpdateMessagePtr& msg) {
-      return sizeof(AclUpdateMessage) + msg->username.capacity() * sizeof(string) +
-             msg->commands.capacity() * sizeof(vector<int>) +
-             msg->categories.capacity() * sizeof(uint32_t);
+      size_t key_cap = std::accumulate(
+          msg->keys.key_globs.begin(), msg->keys.key_globs.end(), 0, [](auto acc, auto& str) {
+            return acc + (str.first.capacity() * sizeof(char)) + sizeof(str.second);
+          });
+      return sizeof(AclUpdateMessage) + msg->username.capacity() * sizeof(char) +
+             msg->commands.capacity() * sizeof(uint64_t) + key_cap;
     }
     size_t operator()(const MigrationRequestMessage& msg) {
       return 0;
@@ -240,11 +244,10 @@ void Connection::DispatchOperations::operator()(const MonitorMessage& msg) {
 
 void Connection::DispatchOperations::operator()(const AclUpdateMessage& msg) {
   if (self->cntx()) {
-    for (size_t id = 0; id < msg.username.size(); ++id) {
-      if (msg.username[id] == self->cntx()->authed_username) {
-        self->cntx()->acl_categories = msg.categories[id];
-        self->cntx()->acl_commands = msg.commands[id];
-      }
+    if (msg.username == self->cntx()->authed_username) {
+      self->cntx()->acl_categories = msg.categories;
+      self->cntx()->acl_commands = msg.commands;
+      self->cntx()->keys = msg.keys;
     }
   }
 }

src/facade/dragonfly_connection.h

@@ -16,6 +16,7 @@
 
 #include "base/io_buf.h"
 #include "core/fibers.h"
+#include "facade/acl_commands_def.h"
 #include "facade/facade_types.h"
 #include "facade/resp_expr.h"
 #include "util/connection.h"
@@ -99,9 +100,10 @@ class Connection : public util::Connection {
 
   // ACL Update message, contains ACL updates to be applied to the connection.
   struct AclUpdateMessage {
-    std::vector<std::string> username;
-    std::vector<uint32_t> categories;
-    std::vector<std::vector<uint64_t>> commands;
+    std::string username;
+    uint32_t categories;
+    std::vector<uint64_t> commands;
+    dfly::acl::AclKeys keys;
   };
 
   // Migration request message, the dispatch fiber stops to give way for thread migration.

src/server/acl/acl_family.cc

@@ -64,27 +64,27 @@ void AclFamily::List(CmdArgList args, ConnectionContext* cntx) {
     const std::string password = pass == "nopass" ? "nopass" : PrettyPrintSha(pass);
     const std::string acl_cat = AclCatToString(user.AclCategory());
     const std::string acl_commands = AclCommandToString(user.AclCommandsRef());
-    const std::string maybe_space = acl_commands.empty() ? "" : " ";
+    const std::string maybe_space_com = acl_commands.empty() ? "" : " ";
+    const std::string acl_keys = AclKeysToString(user.Keys());
+    const std::string maybe_space = acl_keys.empty() ? "" : " ";
 
     using namespace std::string_view_literals;
 
     absl::StrAppend(&buffer, username, " ", user.IsActive() ? "on "sv : "off "sv, password, " ",
-                    acl_cat, maybe_space, acl_commands);
+                    acl_cat, maybe_space_com, acl_commands, maybe_space, acl_keys);
 
     (*cntx)->SendSimpleString(buffer);
   }
 }
 
-void AclFamily::StreamUpdatesToAllProactorConnections(const std::vector<std::string>& user,
-                                                      const std::vector<uint32_t>& update_cat,
-                                                      const NestedVector& update_commands) {
-  auto update_cb = [&user, &update_cat, &update_commands]([[maybe_unused]] size_t id,
-                                                          util::Connection* conn) {
+void AclFamily::StreamUpdatesToAllProactorConnections(const std::string& user, uint32_t update_cat,
+                                                      const Commands& update_commands,
+                                                      const AclKeys& update_keys) {
+  auto update_cb = [&]([[maybe_unused]] size_t id, util::Connection* conn) {
     DCHECK(conn);
     auto connection = static_cast<facade::Connection*>(conn);
-    DCHECK(user.size() == update_cat.size());
     connection->SendAclUpdateAsync(
-        facade::Connection::AclUpdateMessage{user, update_cat, update_commands});
+        facade::Connection::AclUpdateMessage{user, update_cat, update_commands, update_keys});
   };
 
   if (main_listener_) {
@@ -96,14 +96,20 @@ using facade::ErrorReply;
 
 void AclFamily::SetUser(CmdArgList args, ConnectionContext* cntx) {
   std::string_view username = facade::ToSV(args[0]);
-  auto req = ParseAclSetUser(args.subspan(1), *cmd_registry_);
+  auto reg = registry_->GetRegistryWithWriteLock();
+  const bool exists = reg.registry.contains(username);
+  const bool has_all_keys = exists ? reg.registry.find(username)->second.Keys().all_keys : false;
+
+  auto req = ParseAclSetUser(args.subspan(1), *cmd_registry_, false, has_all_keys);
+
   auto error_case = [cntx](ErrorReply&& error) { (*cntx)->SendError(error); };
-  auto update_case = [username, cntx, this](User::UpdateRequest&& req) {
-    auto user_with_lock = registry_->MaybeAddAndUpdateWithLock(username, std::move(req));
-    if (user_with_lock.exists) {
-      StreamUpdatesToAllProactorConnections({std::string(username)},
-                                            {user_with_lock.user.AclCategory()},
-                                            {user_with_lock.user.AclCommands()});
+
+  auto update_case = [username, &reg, cntx, this, exists](User::UpdateRequest&& req) {
+    auto& user = reg.registry[username];
+    user.Update(std::move(req));
+    if (exists) {
+      StreamUpdatesToAllProactorConnections(std::string(username), user.AclCategory(),
+                                            user.AclCommands(), user.Keys());
     }
     cntx->SendOk();
   };
@@ -272,13 +278,10 @@ std::optional<facade::ErrorReply> AclFamily::LoadToRegistryFromFile(std::string_
     EvictOpenConnectionsOnAllProactorsWithRegistry(registry);
     registry.clear();
   }
-  std::vector<uint32_t> categories;
-  NestedVector commands;
+
   for (size_t i = 0; i < usernames.size(); ++i) {
     auto& user = registry[usernames[i]];
     user.Update(std::move(requests[i]));
-    categories.push_back(user.AclCategory());
-    commands.push_back(user.AclCommands());
   }
 
   if (!registry.contains("default")) {
@@ -460,7 +463,7 @@ void AclFamily::GetUser(CmdArgList args, ConnectionContext* cntx) {
   std::string status = user.IsActive() ? "on" : "off";
   auto pass = user.Password();
 
-  (*cntx)->StartArray(6);
+  (*cntx)->StartArray(8);
 
   (*cntx)->SendSimpleString("flags");
   const size_t total_elements = (pass != "nopass") ? 1 : 2;
@@ -481,6 +484,14 @@ void AclFamily::GetUser(CmdArgList args, ConnectionContext* cntx) {
   std::string acl = absl::StrCat(AclCatToString(user.AclCategory()), " ",
                                  AclCommandToString(user.AclCommandsRef()));
   (*cntx)->SendSimpleString(acl);
+
+  (*cntx)->SendSimpleString("keys");
+  std::string keys = AclKeysToString(user.Keys());
+  if (!keys.empty()) {
+    (*cntx)->SendSimpleString(keys);
+  } else {
+    (*cntx)->SendEmptyArray();
+  }
 }
 
 void AclFamily::GenPass(CmdArgList args, ConnectionContext* cntx) {
@@ -531,7 +542,8 @@ void AclFamily::DryRun(CmdArgList args, ConnectionContext* cntx) {
   }
 
   const auto& user = registry.find(username)->second;
-  if (IsUserAllowedToInvokeCommandGeneric(user.AclCategory(), user.AclCommandsRef(), *cid)) {
+  if (IsUserAllowedToInvokeCommandGeneric(user.AclCategory(), user.AclCommandsRef(), {{}, true}, {},
+                                          *cid)) {
     (*cntx)->SendOk();
     return;
   }

src/server/acl/acl_family.h

@@ -47,10 +47,10 @@ class AclFamily final {
 
   // Helper function that updates all open connections and their
   // respective ACL fields on all the available proactor threads
-  using NestedVector = std::vector<std::vector<uint64_t>>;
-  void StreamUpdatesToAllProactorConnections(const std::vector<std::string>& user,
-                                             const std::vector<uint32_t>& update_cat,
-                                             const NestedVector& update_commands);
+  using Commands = std::vector<uint64_t>;
+  void StreamUpdatesToAllProactorConnections(const std::string& user, uint32_t update_cat,
+                                             const Commands& update_commands,
+                                             const AclKeys& update_keys);
 
   // Helper function that closes all open connection from the deleted user
   void EvictOpenConnectionsOnAllProactors(std::string_view user);

src/server/acl/acl_family_test.cc

@@ -46,15 +46,15 @@ TEST_F(AclFamilyTest, AclSetUser) {
   EXPECT_THAT(resp, "OK");
   resp = Run({"ACL", "LIST"});
   auto vec = resp.GetVec();
-  EXPECT_THAT(vec, UnorderedElementsAre("user default on nopass +@ALL +ALL",
+  EXPECT_THAT(vec, UnorderedElementsAre("user default on nopass +@ALL +ALL ~*",
                                         "user vlad off nopass +@NONE"));
 
   resp = Run({"ACL", "SETUSER", "vlad", "+ACL"});
   EXPECT_THAT(resp, "OK");
 
   resp = Run({"ACL", "LIST"});
   vec = resp.GetVec();
-  EXPECT_THAT(vec, UnorderedElementsAre("user default on nopass +@ALL +ALL",
+  EXPECT_THAT(vec, UnorderedElementsAre("user default on nopass +@ALL +ALL ~*",
                                         "user vlad off nopass +@NONE +ACL"));
 }
 
@@ -79,7 +79,7 @@ TEST_F(AclFamilyTest, AclDelUser) {
   EXPECT_THAT(resp, "OK");
 
   resp = Run({"ACL", "LIST"});
-  EXPECT_THAT(resp.GetString(), "user default on nopass +@ALL +ALL");
+  EXPECT_THAT(resp.GetString(), "user default on nopass +@ALL +ALL ~*");
 }
 
 TEST_F(AclFamilyTest, AclList) {
@@ -95,7 +95,7 @@ TEST_F(AclFamilyTest, AclList) {
 
   resp = Run({"ACL", "LIST"});
   auto vec = resp.GetVec();
-  EXPECT_THAT(vec, UnorderedElementsAre("user default on nopass +@ALL +ALL",
+  EXPECT_THAT(vec, UnorderedElementsAre("user default on nopass +@ALL +ALL ~*",
                                         "user kostas off d74ff0ee8da3b98 +@ADMIN",
                                         "user adi off d74ff0ee8da3b98 +@FAST"));
 }
@@ -146,15 +146,15 @@ TEST_F(AclFamilyTest, TestAllCategories) {
 
       resp = Run({"ACL", "LIST"});
       EXPECT_THAT(resp.GetVec(),
-                  UnorderedElementsAre("user default on nopass +@ALL +ALL",
+                  UnorderedElementsAre("user default on nopass +@ALL +ALL ~*",
                                        absl::StrCat("user kostas off nopass ", "+@", cat)));
 
       resp = Run({"ACL", "SETUSER", "kostas", absl::StrCat("-@", cat)});
       EXPECT_THAT(resp, "OK");
 
       resp = Run({"ACL", "LIST"});
       EXPECT_THAT(resp.GetVec(),
-                  UnorderedElementsAre("user default on nopass +@ALL +ALL",
+                  UnorderedElementsAre("user default on nopass +@ALL +ALL ~*",
                                        absl::StrCat("user kostas off nopass ", "+@NONE")));
 
       resp = Run({"ACL", "DELUSER", "kostas"});
@@ -193,15 +193,15 @@ TEST_F(AclFamilyTest, TestAllCommands) {
       EXPECT_THAT(resp, "OK");
 
       resp = Run({"ACL", "LIST"});
-      EXPECT_THAT(resp.GetVec(), UnorderedElementsAre("user default on nopass +@ALL +ALL",
+      EXPECT_THAT(resp.GetVec(), UnorderedElementsAre("user default on nopass +@ALL +ALL ~*",
                                                       absl::StrCat("user kostas off nopass +@NONE ",
                                                                    "+", command_name)));
 
       resp = Run({"ACL", "SETUSER", "kostas", absl::StrCat("-", command_name)});
 
       resp = Run({"ACL", "LIST"});
       EXPECT_THAT(resp.GetVec(),
-                  UnorderedElementsAre("user default on nopass +@ALL +ALL",
+                  UnorderedElementsAre("user default on nopass +@ALL +ALL ~*",
                                        absl::StrCat("user kostas off nopass ", "+@NONE")));
 
       resp = Run({"ACL", "DELUSER", "kostas"});
@@ -252,6 +252,8 @@ TEST_F(AclFamilyTest, TestGetUser) {
   EXPECT_TRUE(vec[3].GetVec().empty());
   EXPECT_THAT(vec[4], "commands");
   EXPECT_THAT(vec[5], "+@ALL +ALL");
+  EXPECT_THAT(vec[6], "keys");
+  EXPECT_THAT(vec[7], "~*");
 
   resp = Run({"ACL", "SETUSER", "kostas", "+@STRING", "+HSET"});
   resp = Run({"ACL", "GETUSER", "kostas"});
@@ -351,4 +353,58 @@ TEST_F(AclFamilyTestRename, AclRename) {
   EXPECT_THAT(resp.GetString(), "OK");
 }
 
+TEST_F(AclFamilyTest, TestKeys) {
+  TestInitAclFam();
+  auto resp = Run({"ACL", "SETUSER", "temp", "~foo", "~bar*"});
+  EXPECT_THAT(resp, "OK");
+
+  resp = Run({"ACL", "GETUSER", "temp"});
+  auto& vec = resp.GetVec();
+  EXPECT_THAT(vec[6], "keys");
+  EXPECT_THAT(vec[7], "~foo ~bar*");
+
+  resp = Run({"ACL", "SETUSER", "temp", "~*", "~foo"});
+  EXPECT_THAT(resp, ErrArg("ERR Error in ACL SETUSER modifier '~tmp': Adding a pattern after the * "
+                           "pattern (or the 'allkeys' flag) is not valid and does not have any "
+                           "effect. Try 'resetkeys' to start with an empty list of patterns"));
+
+  resp = Run({"ACL", "SETUSER", "temp", "~*"});
+  EXPECT_THAT(resp, "OK");
+
+  resp = Run({"ACL", "SETUSER", "temp", "~foo"});
+  EXPECT_THAT(resp, ErrArg("ERR Error in ACL SETUSER modifier '~tmp': Adding a pattern after the * "
+                           "pattern (or the 'allkeys' flag) is not valid and does not have any "
+                           "effect. Try 'resetkeys' to start with an empty list of patterns"));
+
+  resp = Run({"ACL", "SETUSER", "temp", "resetkeys"});
+  EXPECT_THAT(resp, "OK");
+
+  resp = Run({"ACL", "GETUSER", "temp"});
+  EXPECT_TRUE(resp.GetVec()[7].GetVec().empty());
+
+  resp = Run({"ACL", "SETUSER", "temp", "%R~foo"});
+  EXPECT_THAT(resp, "OK");
+
+  resp = Run({"ACL", "GETUSER", "temp"});
+  EXPECT_THAT(resp.GetVec()[7], "%R~foo");
+
+  resp = Run({"ACL", "SETUSER", "temp", "resetkeys", "%W~foo"});
+  EXPECT_THAT(resp, "OK");
+
+  resp = Run({"ACL", "GETUSER", "temp"});
+  EXPECT_THAT(resp.GetVec()[7], "%W~foo");
+
+  resp = Run({"ACL", "SETUSER", "temp", "resetkeys", "%RW~foo"});
+  EXPECT_THAT(resp, "OK");
+
+  resp = Run({"ACL", "GETUSER", "temp"});
+  EXPECT_THAT(resp.GetVec()[7], "~foo");
+
+  resp = Run({"ACL", "SETUSER", "temp", "resetkeys", "%K~foo"});
+  EXPECT_THAT(resp, ErrArg("ERR Unrecognized parameter %K~FOO"));
+
+  resp = Run({"ACL", "SETUSER", "temp", "resetkeys", "%Rfoo"});
+  EXPECT_THAT(resp, ErrArg("ERR Unrecognized parameter %RFOO"));
+}
+
 }  // namespace dfly