From 84aa237ba7d71a8c1c54181f58864ca8a094c61b Mon Sep 17 00:00:00 2001 From: Joe Zhou Date: Mon, 22 Apr 2024 11:14:47 -0400 Subject: [PATCH] chore(acl): adjust some ACL command responses (#2943) * change ACL DELUSER, ACL WHOAMI, and some ACL DRYRUN string/integer responses. * change ACL GETUSER response, when the user does not exist, it should reply (nil). --- src/server/acl/acl_family.cc | 21 +++++++++++---------- src/server/acl/acl_family_test.cc | 25 ++++++++++++++----------- tests/dragonfly/acl_family_test.py | 2 +- 3 files changed, 26 insertions(+), 22 deletions(-) diff --git a/src/server/acl/acl_family.cc b/src/server/acl/acl_family.cc index f29a1a457..ace25b5e4 100644 --- a/src/server/acl/acl_family.cc +++ b/src/server/acl/acl_family.cc @@ -155,22 +155,22 @@ void AclFamily::EvictOpenConnectionsOnAllProactorsWithRegistry( 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"); + 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")); + cntx->SendLong(0); return; } EvictOpenConnectionsOnAllProactors(username); - cntx->SendOk(); + cntx->SendLong(1); } void AclFamily::WhoAmI(CmdArgList args, ConnectionContext* cntx) { - cntx->SendSimpleString(absl::StrCat("User is ", cntx->authed_username)); + cntx->SendSimpleString(cntx->authed_username); } std::string AclFamily::RegistryToString() const { @@ -471,8 +471,8 @@ void AclFamily::GetUser(CmdArgList args, ConnectionContext* cntx) { const auto registry_with_lock = registry_->GetRegistryWithLock(); const auto& registry = registry_with_lock.registry; if (!registry.contains(username)) { - auto error = absl::StrCat("User: ", username, " does not exists!"); - cntx->SendError(error); + auto* rb = static_cast(cntx->reply_builder()); + rb->SendNull(); return; } auto& user = registry.find(username)->second; @@ -545,7 +545,7 @@ void AclFamily::DryRun(CmdArgList args, ConnectionContext* cntx) { const auto registry_with_lock = registry_->GetRegistryWithLock(); const auto& registry = registry_with_lock.registry; if (!registry.contains(username)) { - auto error = absl::StrCat("User: ", username, " does not exists!"); + auto error = absl::StrCat("User '", username, "' not found"); cntx->SendError(error); return; } @@ -554,7 +554,7 @@ void AclFamily::DryRun(CmdArgList args, ConnectionContext* cntx) { auto command = facade::ArgS(args, 1); auto* cid = cmd_registry_->Find(command); if (!cid) { - auto error = absl::StrCat("Command: ", command, " does not exists!"); + auto error = absl::StrCat("Command '", command, "' not found"); cntx->SendError(error); return; } @@ -568,8 +568,9 @@ void AclFamily::DryRun(CmdArgList args, ConnectionContext* cntx) { return; } - auto error = absl::StrCat("User: ", username, " is not allowed to execute command: ", command); - cntx->SendError(error); + auto msg = absl::StrCat("This user has no permissions to run the '", command, "' command"); + auto* rb = static_cast(cntx->reply_builder()); + rb->SendBulkString(msg); } using MemberFunc = void (AclFamily::*)(CmdArgList args, ConnectionContext* cntx); diff --git a/src/server/acl/acl_family_test.cc b/src/server/acl/acl_family_test.cc index b6a3bf391..96b32b86d 100644 --- a/src/server/acl/acl_family_test.cc +++ b/src/server/acl/acl_family_test.cc @@ -64,10 +64,10 @@ TEST_F(AclFamilyTest, AclDelUser) { 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")); + 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")); + EXPECT_THAT(resp.GetInt(), 0); resp = Run({"ACL", "SETUSER", "kostas", "ON"}); EXPECT_THAT(resp, "OK"); @@ -76,7 +76,10 @@ TEST_F(AclFamilyTest, AclDelUser) { EXPECT_THAT(resp, ErrArg("ERR wrong number of arguments for 'acl deluser' command")); resp = Run({"ACL", "DELUSER", "kostas"}); - EXPECT_THAT(resp, "OK"); + EXPECT_THAT(resp.GetInt(), 1); + + resp = Run({"ACL", "DELUSER", "kostas"}); + EXPECT_THAT(resp.GetInt(), 0); resp = Run({"ACL", "LIST"}); EXPECT_THAT(resp.GetString(), "user default on nopass +@ALL +ALL ~*"); @@ -134,7 +137,7 @@ TEST_F(AclFamilyTest, AclWhoAmI) { EXPECT_THAT(resp, "OK"); resp = Run({"ACL", "WHOAMI"}); - EXPECT_THAT(resp, "User is kostas"); + EXPECT_THAT(resp, "kostas"); } TEST_F(AclFamilyTest, TestAllCategories) { @@ -158,7 +161,7 @@ TEST_F(AclFamilyTest, TestAllCategories) { absl::StrCat("user kostas off nopass ", "+@NONE"))); resp = Run({"ACL", "DELUSER", "kostas"}); - EXPECT_THAT(resp, "OK"); + EXPECT_THAT(resp.GetInt(), 1); } } @@ -205,7 +208,7 @@ TEST_F(AclFamilyTest, TestAllCommands) { absl::StrCat("user kostas off nopass ", "+@NONE"))); resp = Run({"ACL", "DELUSER", "kostas"}); - EXPECT_THAT(resp, "OK"); + EXPECT_THAT(resp.GetInt(), 1); } } } @@ -242,7 +245,7 @@ TEST_F(AclFamilyTest, TestCat) { TEST_F(AclFamilyTest, TestGetUser) { TestInitAclFam(); auto resp = Run({"ACL", "GETUSER", "kostas"}); - EXPECT_THAT(resp, ErrArg("ERR User: kostas does not exists!")); + EXPECT_THAT(resp, ArgType(RespExpr::NIL)); resp = Run({"ACL", "GETUSER", "default"}); const auto& vec = resp.GetVec(); @@ -278,10 +281,10 @@ TEST_F(AclFamilyTest, TestDryRun) { EXPECT_THAT(resp, ErrArg("ERR wrong number of arguments for 'acl dryrun' command")); resp = Run({"ACL", "DRYRUN", "kostas", "more"}); - EXPECT_THAT(resp, ErrArg("ERR User: kostas does not exists!")); + EXPECT_THAT(resp, ErrArg("ERR User 'kostas' not found")); resp = Run({"ACL", "DRYRUN", "default", "nope"}); - EXPECT_THAT(resp, ErrArg("ERR Command: NOPE does not exists!")); + EXPECT_THAT(resp, ErrArg("ERR Command 'NOPE' not found")); resp = Run({"ACL", "DRYRUN", "default", "SET"}); EXPECT_THAT(resp, "OK"); @@ -293,7 +296,7 @@ TEST_F(AclFamilyTest, TestDryRun) { EXPECT_THAT(resp, "OK"); resp = Run({"ACL", "DRYRUN", "kostas", "SET"}); - EXPECT_THAT(resp, ErrArg("ERR User: kostas is not allowed to execute command: SET")); + EXPECT_THAT(resp, "This user has no permissions to run the 'SET' command"); } TEST_F(AclFamilyTest, AclGenPassTooManyArguments) { @@ -350,7 +353,7 @@ TEST_F(AclFamilyTestRename, AclRename) { EXPECT_THAT(resp.GetString(), "OK"); resp = Run({"ROCKS", "DELUSER", "billy"}); - EXPECT_THAT(resp.GetString(), "OK"); + EXPECT_THAT(resp.GetInt(), 1); } TEST_F(AclFamilyTest, TestKeys) { diff --git a/tests/dragonfly/acl_family_test.py b/tests/dragonfly/acl_family_test.py index 620d12fe3..2b6c803d4 100644 --- a/tests/dragonfly/acl_family_test.py +++ b/tests/dragonfly/acl_family_test.py @@ -341,7 +341,7 @@ async def test_good_acl_file(df_local_factory, tmp_dir): assert "user default on nopass +@ALL +ALL ~*" in result result = await client.execute_command("ACL DELUSER shahar") - assert result == b"OK" + assert result == 1 result = await client.execute_command("ACL SAVE")