Remove acl subcommand validation if fully added command exists. #8483
Conversation
|
@redis/core-team I think this is a pretty low risk, but please take a look for Redis 6.2. The risk here is that a user might have an ACL file that loads on 6.0 but fails to load on 6.2 if they whitelist a subcommand that was implicitly added in it's entirety by a category. I think the likelihood of this is low, but given that we're just removing some validation, this seems okay.= |
oranagra
left a comment
There was a problem hiding this comment.
I agree with this "fix" (to remove a validation).
I was initially hesitant to remove some validation that was purposely coded.
But I agree this validation is kinda silly, considering we did that only for sub-commands and not for commands.
i.e. 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
tests/unit/acl.tcl
Outdated
| r ACL setuser bob +pfcount|hll | ||
| set cmdstr [dict get [r ACL getuser bob] commands] | ||
| assert_equal {-@all +pfcount|hll} $cmdstr |
There was a problem hiding this comment.
I don't like the fact that this test uses hll as if it's a sub-command of pfcount
I would prefer to convert this test to check some command that really does have a sub-command.
how about OBJECT REFCOUNT or MEMORY USAGE?
…s#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 <harkrisp@amazon.com> Co-authored-by: Madelyn Olson <madelyneolson@gmail.com> Co-authored-by: Oran Agra <oran@redislabs.com>
Currently there is a validation to disallow subcommand addition to a fully added command. This is to avoid if a customer has wrongly added it. Due to this there is a breaking change across versions, due to the addition of PFDEBUG into hyperloglog category.
However, we can ignore the subcommand instead of throwing the error.
Current behaviour:
Proposed changes: