make processCommand check publish channel permissions. (#8534)

Add publish channel permissions check in processCommand.

processCommand didn't check publish channel permissions, so we can
queue a publish command in a transaction. But when exec the transaction,
it will fail with -NOPERM.

We also union keys/commands/channels permissions check togegher in
ACLCheckAllPerm. Remove pubsubCheckACLPermissionsOrReply in 
publishCommand/subscribeCommand/psubscribeCommand. Always 
check permissions in processCommand/execCommand/
luaRedisGenericCommand.

7 files changed, 55 insertions, 33 deletions

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 <channel> <message> */
 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