From e7ced7566bd86183b4673846cbd071d61fe8c0db Mon Sep 17 00:00:00 2001 From: Harkrishn Patro <30795839+hpatro@users.noreply.github.com> Date: Mon, 22 Feb 2021 14:22:25 +0100 Subject: [PATCH] Remove acl subcommand validation if fully added command exists. (#8483) This validation was only done for sub-commands and not for commands. These would have been valid (not produce any error) ACL SETUSER bob +@all +client ACL SETUSER bob +client +client so no reason for this one to fail: ACL SETUSER bob +client +client|id One example why this is needed is that pfdebug wasn't part of the @hyperloglog group and now it is. so something like: acl setuser user1 +@hyperloglog +pfdebug|test would have succeeded in early 6.0.x, and fail in 6.2 RC3 Co-authored-by: Harkrishn Patro Co-authored-by: Madelyn Olson Co-authored-by: Oran Agra --- src/acl.c | 22 +++------------------- tests/unit/acl.tcl | 19 +++++++++++++++++++ 2 files changed, 22 insertions(+), 19 deletions(-) diff --git a/src/acl.c b/src/acl.c index 74210ebce..e120b36fc 100644 --- a/src/acl.c +++ b/src/acl.c @@ -814,8 +814,6 @@ void ACLAddAllowedSubcommand(user *u, unsigned long id, const char *sub) { * invalid (contains non allowed characters). * ENOENT: The command name or command category provided with + or - is not * known. - * EBUSY: The subcommand you want to add is about a command that is currently - * fully added. * EEXIST: You are adding a key pattern after "*" was already added. This is * almost surely an error on the user side. * EISDIR: You are adding a channel pattern after "*" was already added. This is @@ -976,22 +974,12 @@ int ACLSetUser(user *u, const char *op, ssize_t oplen) { return C_ERR; } - /* The command should not be set right now in the command - * bitmap, because adding a subcommand of a fully added - * command is probably an error on the user side. */ unsigned long id = ACLGetCommandID(copy); - if (ACLGetUserCommandBit(u,id) == 1) { - zfree(copy); - errno = EBUSY; - return C_ERR; + /* Add the subcommand to the list of valid ones, if the command is not set. */ + if (ACLGetUserCommandBit(u,id) == 0) { + ACLAddAllowedSubcommand(u,id,sub); } - /* Add the subcommand to the list of valid ones. */ - ACLAddAllowedSubcommand(u,id,sub); - - /* We have to clear the command bit so that we force the - * subcommand check. */ - ACLSetUserCommandBit(u,id,0); zfree(copy); } } else if (op[0] == '-' && op[1] != '@') { @@ -1030,10 +1018,6 @@ const char *ACLSetUserStringError(void) { errmsg = "Unknown command or category name in ACL"; else if (errno == EINVAL) errmsg = "Syntax error"; - else if (errno == EBUSY) - errmsg = "Adding a subcommand of a command already fully " - "added is not allowed. Remove the command to start. " - "Example: -DEBUG +DEBUG|DIGEST"; else if (errno == EEXIST) errmsg = "Adding a pattern after the * pattern (or the " "'allkeys' flag) is not valid and does not have any " diff --git a/tests/unit/acl.tcl b/tests/unit/acl.tcl index f5e3259fe..7c09195a1 100644 --- a/tests/unit/acl.tcl +++ b/tests/unit/acl.tcl @@ -217,6 +217,25 @@ start_server {tags {"acl"}} { set e } {*NOPERM*} + test {ACLs set can include subcommands, if already full command exists} { + r ACL setuser bob +memory|doctor + set cmdstr [dict get [r ACL getuser bob] commands] + assert_equal {-@all +memory|doctor} $cmdstr + + # Validate the commands have got engulfed to +memory. + r ACL setuser bob +memory + set cmdstr [dict get [r ACL getuser bob] commands] + assert_equal {-@all +memory} $cmdstr + + # Appending to the existing access string of bob. + r ACL setuser bob +@all +client|id + # Validate the new commands has got engulfed to +@all. + set cmdstr [dict get [r ACL getuser bob] commands] + assert_equal {+@all} $cmdstr + r CLIENT ID; # Should not fail + r MEMORY DOCTOR; # Should not fail + } + # Note that the order of the generated ACL rules is not stable in Redis # so we need to match the different parts and not as a whole string. test {ACL GETUSER is able to translate back command permissions} {