diff --git a/src/acl.c b/src/acl.c index aecd0629b..16f8606d3 100644 --- a/src/acl.c +++ b/src/acl.c @@ -1360,6 +1360,22 @@ int ACLCheckPubsubPerm(client *c, int idx, int count, int literal, int *idxptr) } +/* Check whether the command is ready to be exceuted by ACLCheckCommandPerm. + * If check passes, then check whether pub/sub channels of the command is + * ready to be executed by ACLCheckPubsubPerm */ +int ACLCheckAllPerm(client *c, int *idxptr) { + int acl_retval = ACLCheckCommandPerm(c,idxptr); + if (acl_retval != ACL_OK) + return acl_retval; + if (c->cmd->proc == publishCommand) + acl_retval = ACLCheckPubsubPerm(c,1,1,0,idxptr); + else if (c->cmd->proc == subscribeCommand) + acl_retval = ACLCheckPubsubPerm(c,1,c->argc-1,0,idxptr); + else if (c->cmd->proc == psubscribeCommand) + acl_retval = ACLCheckPubsubPerm(c,1,c->argc-1,1,idxptr); + return acl_retval; +} + /* ============================================================================= * ACL loading / saving functions * ==========================================================================*/ diff --git a/src/multi.c b/src/multi.c index 4abdb7499..902c919c7 100644 --- a/src/multi.c +++ b/src/multi.c @@ -204,9 +204,7 @@ void execCommand(client *c) { /* ACL permissions are also checked at the time of execution in case * they were changed after the commands were queued. */ int acl_errpos; - int acl_retval = ACLCheckCommandPerm(c,&acl_errpos); - if (acl_retval == ACL_OK && c->cmd->proc == publishCommand) - acl_retval = ACLCheckPubsubPerm(c,1,1,0,&acl_errpos); + int acl_retval = ACLCheckAllPerm(c,&acl_errpos); if (acl_retval != ACL_OK) { char *reason; switch (acl_retval) { @@ -217,7 +215,8 @@ void execCommand(client *c) { reason = "no permission to touch the specified keys"; break; case ACL_DENIED_CHANNEL: - reason = "no permission to publish to the specified channel"; + reason = "no permission to access one of the channels used " + "as arguments"; break; default: reason = "no permission"; diff --git a/src/pubsub.c b/src/pubsub.c index 5f7335bbe..3409deac2 100644 --- a/src/pubsub.c +++ b/src/pubsub.c @@ -331,21 +331,6 @@ int pubsubPublishMessage(robj *channel, robj *message) { return receivers; } -/* This wraps handling ACL channel permissions for the given client. */ -int pubsubCheckACLPermissionsOrReply(client *c, int idx, int count, int literal) { - /* Check if the user can run the command according to the current - * ACLs. */ - int acl_chanpos; - int acl_retval = ACLCheckPubsubPerm(c,idx,count,literal,&acl_chanpos); - if (acl_retval == ACL_DENIED_CHANNEL) { - addACLLogEntry(c,acl_retval,acl_chanpos,NULL); - addReplyError(c, - "-NOPERM this user has no permissions to access " - "one of the channels used as arguments"); - } - return acl_retval; -} - /*----------------------------------------------------------------------------- * Pubsub commands implementation *----------------------------------------------------------------------------*/ @@ -353,7 +338,6 @@ int pubsubCheckACLPermissionsOrReply(client *c, int idx, int count, int literal) /* SUBSCRIBE channel [channel ...] */ void subscribeCommand(client *c) { int j; - if (pubsubCheckACLPermissionsOrReply(c,1,c->argc-1,0) != ACL_OK) return; if ((c->flags & CLIENT_DENY_BLOCKING) && !(c->flags & CLIENT_MULTI)) { /** * A client that has CLIENT_DENY_BLOCKING flag on @@ -387,7 +371,6 @@ void unsubscribeCommand(client *c) { /* PSUBSCRIBE pattern [pattern ...] */ void psubscribeCommand(client *c) { int j; - if (pubsubCheckACLPermissionsOrReply(c,1,c->argc-1,1) != ACL_OK) return; if ((c->flags & CLIENT_DENY_BLOCKING) && !(c->flags & CLIENT_MULTI)) { /** * A client that has CLIENT_DENY_BLOCKING flag on @@ -420,7 +403,6 @@ void punsubscribeCommand(client *c) { /* PUBLISH */ void publishCommand(client *c) { - if (pubsubCheckACLPermissionsOrReply(c,1,1,0) != ACL_OK) return; int receivers = pubsubPublishMessage(c->argv[1],c->argv[2]); if (server.cluster_enabled) clusterPropagatePublish(c->argv[1],c->argv[2]); diff --git a/src/scripting.c b/src/scripting.c index 4276190ce..299e60810 100644 --- a/src/scripting.c +++ b/src/scripting.c @@ -604,9 +604,7 @@ int luaRedisGenericCommand(lua_State *lua, int raise_error) { /* Check the ACLs. */ int acl_errpos; - int acl_retval = ACLCheckCommandPerm(c,&acl_errpos); - if (acl_retval == ACL_OK && c->cmd->proc == publishCommand) - acl_retval = ACLCheckPubsubPerm(c,1,1,0,&acl_errpos); + int acl_retval = ACLCheckAllPerm(c,&acl_errpos); if (acl_retval != ACL_OK) { addACLLogEntry(c,acl_retval,acl_errpos,NULL); switch (acl_retval) { diff --git a/src/server.c b/src/server.c index 3df7a5a72..3421b9303 100644 --- a/src/server.c +++ b/src/server.c @@ -4011,18 +4011,30 @@ int processCommand(client *c) { /* Check if the user can run this command according to the current * ACLs. */ - int acl_keypos; - int acl_retval = ACLCheckCommandPerm(c,&acl_keypos); + int acl_errpos; + int acl_retval = ACLCheckAllPerm(c,&acl_errpos); if (acl_retval != ACL_OK) { - addACLLogEntry(c,acl_retval,acl_keypos,NULL); - if (acl_retval == ACL_DENIED_CMD) + addACLLogEntry(c,acl_retval,acl_errpos,NULL); + switch (acl_retval) { + case ACL_DENIED_CMD: rejectCommandFormat(c, "-NOPERM this user has no permissions to run " "the '%s' command or its subcommand", c->cmd->name); - else + break; + case ACL_DENIED_KEY: rejectCommandFormat(c, "-NOPERM this user has no permissions to access " "one of the keys used as arguments"); + break; + case ACL_DENIED_CHANNEL: + rejectCommandFormat(c, + "-NOPERM this user has no permissions to access " + "one of the channels used as arguments"); + break; + default: + rejectCommandFormat(c, "no permission"); + break; + } return C_OK; } diff --git a/src/server.h b/src/server.h index a9624cfc6..0b9cc3341 100644 --- a/src/server.h +++ b/src/server.h @@ -2092,7 +2092,7 @@ int isMutuallyExclusiveChildType(int type); extern rax *Users; extern user *DefaultUser; void ACLInit(void); -/* Return values for ACLCheckCommandPerm() and ACLCheckPubsubPerm(). */ +/* Return values for ACLCheckAllPerm(). */ #define ACL_OK 0 #define ACL_DENIED_CMD 1 #define ACL_DENIED_KEY 2 @@ -2103,8 +2103,7 @@ int ACLAuthenticateUser(client *c, robj *username, robj *password); unsigned long ACLGetCommandID(const char *cmdname); void ACLClearCommandID(void); user *ACLGetUserByName(const char *name, size_t namelen); -int ACLCheckCommandPerm(client *c, int *keyidxptr); -int ACLCheckPubsubPerm(client *c, int idx, int count, int literal, int *idxptr); +int ACLCheckAllPerm(client *c, int *idxptr); int ACLSetUser(user *u, const char *op, ssize_t oplen); sds ACLDefaultUserFirstPassword(void); uint64_t ACLGetCommandCategoryFlagByName(const char *name); diff --git a/tests/unit/acl.tcl b/tests/unit/acl.tcl index a6afd3f9e..175984274 100644 --- a/tests/unit/acl.tcl +++ b/tests/unit/acl.tcl @@ -113,6 +113,22 @@ start_server {tags {"acl"}} { set e } {*NOPERM*channel*} + test {In transaction queue publish/subscribe/psubscribe to unauthorized channel will fail} { + r ACL setuser psuser +multi +discard + r MULTI + catch {r PUBLISH notexits helloworld} e + r DISCARD + assert_match {*NOPERM*} $e + r MULTI + catch {r SUBSCRIBE notexits foo:1} e + r DISCARD + assert_match {*NOPERM*} $e + r MULTI + catch {r PSUBSCRIBE notexits:* bar:*} e + r DISCARD + assert_match {*NOPERM*} $e + } + test {It's possible to allow subscribing to a subset of channels} { set rd [redis_deferring_client] $rd AUTH psuser pspass