summaryrefslogtreecommitdiff
path: root/src/acl.c
diff options
context:
space:
mode:
authorShaya Potter <shaya@redislabs.com>2022-10-16 09:01:37 +0300
committerGitHub <noreply@github.com>2022-10-16 09:01:37 +0300
commit3193f086ca0e167e89b6c5cf14133c03213b8378 (patch)
tree4f7243a90017fdb890625cd3bbd91f4c0ce88b57 /src/acl.c
parent56f97bfa5fb082f812c711676d5a77d29af56940 (diff)
downloadredis-3193f086ca0e167e89b6c5cf14133c03213b8378.tar.gz
Unify ACL failure error messaging. (#11160)
Motivation: for applications that use RM ACL verification functions, they would want to return errors back to the user, in ways that are consistent with Redis. While investigating how we should return ACL errors to the user, we realized that Redis isn't consistent, and currently returns ACL error strings in 3 primary ways. [For the actual implications of this change, see the "Impact" section at the bottom] 1. how it returns an error when calling a command normally ACL_DENIED_CMD -> "this user has no permissions to run the '%s' command" ACL_DENIED_KEY -> "this user has no permissions to access one of the keys used as arguments" ACL_DENIED_CHANNEL -> "this user has no permissions to access one of the channels used as arguments" 2. how it returns an error when calling via 'acl dryrun' command ACL_DENIED_CMD -> "This user has no permissions to run the '%s' command" ACL_DENIED_KEY -> "This user has no permissions to access the '%s' key" ACL_DENIED_CHANNEL -> "This user has no permissions to access the '%s' channel" 3. how it returns an error via RM_Call (and scripting is similar). ACL_DENIED_CMD -> "can't run this command or subcommand"; ACL_DENIED_KEY -> "can't access at least one of the keys mentioned in the command arguments"; ACL_DENIED_CHANNEL -> "can't publish to the channel mentioned in the command"; In addition, if one wants to use RM_Call's "dry run" capability instead of the RM ACL functions directly, one also sees a different problem than it returns ACL errors with a -ERR, not a -PERM, so it can't be returned directly to the caller. This PR modifies the code to generate a base message in a common manner with the ability to set verbose flag for acl dry run errors, and keep it unset for normal/rm_call/script cases ```c sds getAclErrorMessage(int acl_res, user *user, struct redisCommand *cmd, sds errored_val, int verbose) { switch (acl_res) { case ACL_DENIED_CMD: return sdscatfmt(sdsempty(), "User %S has no permissions to run " "the '%S' command", user->name, cmd->fullname); case ACL_DENIED_KEY: if (verbose) { return sdscatfmt(sdsempty(), "User %S has no permissions to access " "the '%S' key", user->name, errored_val); } else { return sdsnew("No permissions to access a key"); } case ACL_DENIED_CHANNEL: if (verbose) { return sdscatfmt(sdsempty(), "User %S has no permissions to access " "the '%S' channel", user->name, errored_val); } else { return sdsnew("No permissions to access a channel"); } } ``` The caller can append/prepend the message (adding NOPERM for normal/RM_Call or indicating it's within a script). Impact: - Plain commands, as well as scripts and RM_Call now include the user name. - ACL DRYRUN remains the only one that's verbose (mentions the offending channel or key name) - Changes RM_Call ACL errors from being a `-ERR` to being `-NOPERM` (besides for textual changes) **This somewhat a breaking change, but it only affects the RM_Call with both `C` and `E`, or `D`** - Changes ACL errors in scripts textually from being `The user executing the script <old non unified text>` to `ACL failure in script: <new unified text>`
Diffstat (limited to 'src/acl.c')
-rw-r--r--src/acl.c45
1 files changed, 21 insertions, 24 deletions
diff --git a/src/acl.c b/src/acl.c
index 8257b59b1..782254eb9 100644
--- a/src/acl.c
+++ b/src/acl.c
@@ -1766,7 +1766,11 @@ int ACLUserCheckChannelPerm(user *u, sds channel, int is_pattern) {
return ACL_DENIED_CHANNEL;
}
-/* Lower level API that checks if a specified user is able to execute a given command. */
+/* Lower level API that checks if a specified user is able to execute a given command.
+ *
+ * If the command fails an ACL check, idxptr will be to set to the first argv entry that
+ * causes the failure, either 0 if the command itself fails or the idx of the key/channel
+ * that causes the failure */
int ACLCheckAllUserCommandPerm(user *u, struct redisCommand *cmd, robj **argv, int argc, int *idxptr) {
listIter li;
listNode *ln;
@@ -2581,18 +2585,25 @@ void addACLLogEntry(client *c, int reason, int context, int argpos, sds username
}
}
-const char* getAclErrorMessage(int acl_res) {
- /* Notice that a variant of this code also exists on aclCommand so
- * it also need to be updated on changed. */
+sds getAclErrorMessage(int acl_res, user *user, struct redisCommand *cmd, sds errored_val, int verbose) {
switch (acl_res) {
case ACL_DENIED_CMD:
- return "can't run this command or subcommand";
+ return sdscatfmt(sdsempty(), "User %S has no permissions to run "
+ "the '%S' command", user->name, cmd->fullname);
case ACL_DENIED_KEY:
- return "can't access at least one of the keys mentioned in the command arguments";
+ if (verbose) {
+ return sdscatfmt(sdsempty(), "User %S has no permissions to access "
+ "the '%S' key", user->name, errored_val);
+ } else {
+ return sdsnew("No permissions to access a key");
+ }
case ACL_DENIED_CHANNEL:
- return "can't publish to the channel mentioned in the command";
- default:
- return "lacking the permissions for the command";
+ if (verbose) {
+ return sdscatfmt(sdsempty(), "User %S has no permissions to access "
+ "the '%S' channel", user->name, errored_val);
+ } else {
+ return sdsnew("No permissions to access a channel");
+ }
}
serverPanic("Reached deadcode on getAclErrorMessage");
}
@@ -2956,22 +2967,8 @@ void aclCommand(client *c) {
int idx;
int result = ACLCheckAllUserCommandPerm(u, cmd, c->argv + 3, c->argc - 3, &idx);
- /* Notice that a variant of this code also exists on getAclErrorMessage so
- * it also need to be updated on changed. */
if (result != ACL_OK) {
- sds err = sdsempty();
- if (result == ACL_DENIED_CMD) {
- err = sdscatfmt(err, "This user has no permissions to run "
- "the '%s' command", cmd->fullname);
- } else if (result == ACL_DENIED_KEY) {
- err = sdscatfmt(err, "This user has no permissions to access "
- "the '%s' key", c->argv[idx + 3]->ptr);
- } else if (result == ACL_DENIED_CHANNEL) {
- err = sdscatfmt(err, "This user has no permissions to access "
- "the '%s' channel", c->argv[idx + 3]->ptr);
- } else {
- serverPanic("Invalid permission result");
- }
+ sds err = getAclErrorMessage(result, u, cmd, c->argv[idx+3]->ptr, 1);
addReplyBulkSds(c, err);
return;
}