mirrors/dragonfly
1
0
Fork 0
mirror of https://github.com/dragonflydb/dragonfly.git synced 2025-05-11 10:25:47 +02:00
Code Issues Projects Releases Packages Wiki Activity Actions 5

chore(acl): adjust some ACL command responses (#2943)

Browse source 
* 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).
This commit is contained in:
Joe Zhou 2024-04-22 11:14:47 -04:00 • committed by GitHub
parent e78b909b96
commit 84aa237ba7
No known key found for this signature in database
GPG key ID: B5690EEEBB952194
3 changed files with 26 additions and 22 deletions
Download patch file Download diff file Expand all files Collapse all files

21
src/server/acl/acl_family.cc
View file

@ -155,22 +155,22 @@ void AclFamily::EvictOpenConnectionsOnAllProactorsWithRegistry(
void AclFamily::DelUser(CmdArgList args, ConnectionContext* cntx) { void AclFamily::DelUser(CmdArgList args, ConnectionContext* cntx) {
std::string_view username = facade::ToSV(args[0]); std::string_view username = facade::ToSV(args[0]);
if (username == "default") { if (username == "default") {
cntx->SendError("The'default' user cannot be removed"); cntx->SendError("The 'default' user cannot be removed");
return; return;
} }
auto& registry = *registry_; auto& registry = *registry_;
if (!registry.RemoveUser(username)) { if (!registry.RemoveUser(username)) {
cntx->SendError(absl::StrCat("User ", username, " does not exist")); cntx->SendLong(0);
return; return;
} }
EvictOpenConnectionsOnAllProactors(username); EvictOpenConnectionsOnAllProactors(username);
cntx->SendOk(); cntx->SendLong(1);
} }
void AclFamily::WhoAmI(CmdArgList args, ConnectionContext* cntx) { 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 { 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_with_lock = registry_->GetRegistryWithLock();
const auto& registry = registry_with_lock.registry; const auto& registry = registry_with_lock.registry;
if (!registry.contains(username)) { if (!registry.contains(username)) {
auto error = absl::StrCat("User: ", username, " does not exists!"); auto* rb = static_cast<facade::RedisReplyBuilder*>(cntx->reply_builder());
cntx->SendError(error); rb->SendNull();
return; return;
} }
auto& user = registry.find(username)->second; 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_with_lock = registry_->GetRegistryWithLock();
const auto& registry = registry_with_lock.registry; const auto& registry = registry_with_lock.registry;
if (!registry.contains(username)) { if (!registry.contains(username)) {
auto error = absl::StrCat("User: ", username, " does not exists!"); auto error = absl::StrCat("User '", username, "' not found");
cntx->SendError(error); cntx->SendError(error);
return; return;
} }
@ -554,7 +554,7 @@ void AclFamily::DryRun(CmdArgList args, ConnectionContext* cntx) {
auto command = facade::ArgS(args, 1); auto command = facade::ArgS(args, 1);
auto* cid = cmd_registry_->Find(command); auto* cid = cmd_registry_->Find(command);
if (!cid) { if (!cid) {
auto error = absl::StrCat("Command: ", command, " does not exists!"); auto error = absl::StrCat("Command '", command, "' not found");
cntx->SendError(error); cntx->SendError(error);
return; return;
} }
@ -568,8 +568,9 @@ void AclFamily::DryRun(CmdArgList args, ConnectionContext* cntx) {
return; return;
} }
auto error = absl::StrCat("User: ", username, " is not allowed to execute command: ", command); auto msg = absl::StrCat("This user has no permissions to run the '", command, "' command");
cntx->SendError(error); auto* rb = static_cast<facade::RedisReplyBuilder*>(cntx->reply_builder());
rb->SendBulkString(msg);
} }
using MemberFunc = void (AclFamily::*)(CmdArgList args, ConnectionContext* cntx); using MemberFunc = void (AclFamily::*)(CmdArgList args, ConnectionContext* cntx);

25
src/server/acl/acl_family_test.cc
View file

@ -64,10 +64,10 @@ TEST_F(AclFamilyTest, AclDelUser) {
EXPECT_THAT(resp, ErrArg("ERR wrong number of arguments for 'acl deluser' command")); EXPECT_THAT(resp, ErrArg("ERR wrong number of arguments for 'acl deluser' command"));
resp = Run({"ACL", "DELUSER", "default"}); 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"}); 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"}); resp = Run({"ACL", "SETUSER", "kostas", "ON"});
EXPECT_THAT(resp, "OK"); 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")); EXPECT_THAT(resp, ErrArg("ERR wrong number of arguments for 'acl deluser' command"));
resp = Run({"ACL", "DELUSER", "kostas"}); 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"}); resp = Run({"ACL", "LIST"});
EXPECT_THAT(resp.GetString(), "user default on nopass +@ALL +ALL ~*"); EXPECT_THAT(resp.GetString(), "user default on nopass +@ALL +ALL ~*");
@ -134,7 +137,7 @@ TEST_F(AclFamilyTest, AclWhoAmI) {
EXPECT_THAT(resp, "OK"); EXPECT_THAT(resp, "OK");
resp = Run({"ACL", "WHOAMI"}); resp = Run({"ACL", "WHOAMI"});
EXPECT_THAT(resp, "User is kostas"); EXPECT_THAT(resp, "kostas");
} }
TEST_F(AclFamilyTest, TestAllCategories) { TEST_F(AclFamilyTest, TestAllCategories) {
@ -158,7 +161,7 @@ TEST_F(AclFamilyTest, TestAllCategories) {
absl::StrCat("user kostas off nopass ", "+@NONE"))); absl::StrCat("user kostas off nopass ", "+@NONE")));
resp = Run({"ACL", "DELUSER", "kostas"}); 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"))); absl::StrCat("user kostas off nopass ", "+@NONE")));
resp = Run({"ACL", "DELUSER", "kostas"}); 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) { TEST_F(AclFamilyTest, TestGetUser) {
TestInitAclFam(); TestInitAclFam();
auto resp = Run({"ACL", "GETUSER", "kostas"}); 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"}); resp = Run({"ACL", "GETUSER", "default"});
const auto& vec = resp.GetVec(); 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")); EXPECT_THAT(resp, ErrArg("ERR wrong number of arguments for 'acl dryrun' command"));
resp = Run({"ACL", "DRYRUN", "kostas", "more"}); 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"}); 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"}); resp = Run({"ACL", "DRYRUN", "default", "SET"});
EXPECT_THAT(resp, "OK"); EXPECT_THAT(resp, "OK");
@ -293,7 +296,7 @@ TEST_F(AclFamilyTest, TestDryRun) {
EXPECT_THAT(resp, "OK"); EXPECT_THAT(resp, "OK");
resp = Run({"ACL", "DRYRUN", "kostas", "SET"}); 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) { TEST_F(AclFamilyTest, AclGenPassTooManyArguments) {
@ -350,7 +353,7 @@ TEST_F(AclFamilyTestRename, AclRename) {
EXPECT_THAT(resp.GetString(), "OK"); EXPECT_THAT(resp.GetString(), "OK");
resp = Run({"ROCKS", "DELUSER", "billy"}); resp = Run({"ROCKS", "DELUSER", "billy"});
EXPECT_THAT(resp.GetString(), "OK"); EXPECT_THAT(resp.GetInt(), 1);
} }
TEST_F(AclFamilyTest, TestKeys) { TEST_F(AclFamilyTest, TestKeys) {

2
tests/dragonfly/acl_family_test.py
View file

@ -341,7 +341,7 @@ async def test_good_acl_file(df_local_factory, tmp_dir):
assert "user default on nopass +@ALL +ALL ~*" in result assert "user default on nopass +@ALL +ALL ~*" in result
result = await client.execute_command("ACL DELUSER shahar") result = await client.execute_command("ACL DELUSER shahar")
assert result == b"OK" assert result == 1
result = await client.execute_command("ACL SAVE") result = await client.execute_command("ACL SAVE")