From c627347e0752599c220dd63d963f71589088ddbc Mon Sep 17 00:00:00 2001
From: KarthikSubbarao <karthikrs2021@gmail.com>
Date: Mon, 1 Jul 2024 13:59:06 -0700
Subject: [PATCH] Allow Module authentication to succeed when cluster is down
 (#693)

Module Authentication using a blocking implementation currently gets
rejected when the "cluster is down" from the client timeout cron job
(`clientsCronHandleTimeout`).

This PR exempts clients blocked on Module Authentication from being
rejected here.

---------

Signed-off-by: KarthikSubbarao <karthikrs2021@gmail.com>
---
 src/cluster.c                    |  9 ++++-----
 tests/unit/moduleapi/cluster.tcl | 24 +++++++++++++++++-------
 2 files changed, 21 insertions(+), 12 deletions(-)

diff --git a/src/cluster.c b/src/cluster.c
index a9b1b9a64e..1570d8e089 100644
--- a/src/cluster.c
+++ b/src/cluster.c
@@ -7539,6 +7539,10 @@ int clusterRedirectBlockedClientIfNeeded(client *c) {
         dictEntry *de;
         dictIterator *di;
 
+        /* If the client is blocked on module, but not on a specific key,
+         * don't unblock it. */
+        if (c->bstate.btype == BLOCKED_MODULE && !moduleClientIsBlockedOnKeys(c)) return 0;
+
         /* If the cluster is down, unblock the client with the right error.
          * If the cluster is configured to allow reads on cluster down, we
          * still want to emit this error since a write will be required
@@ -7548,11 +7552,6 @@ int clusterRedirectBlockedClientIfNeeded(client *c) {
             return 1;
         }
 
-        /* If the client is blocked on module, but not on a specific key,
-         * don't unblock it (except for the CLUSTER_FAIL case above). */
-        if (c->bstate.btype == BLOCKED_MODULE && !moduleClientIsBlockedOnKeys(c))
-            return 0;
-
         /* All keys must belong to the same slot, so check first key only. */
         di = dictGetIterator(c->bstate.keys);
         if ((de = dictNext(di)) != NULL) {
diff --git a/tests/unit/moduleapi/cluster.tcl b/tests/unit/moduleapi/cluster.tcl
index b4fb021250..d7ab97a9c3 100644
--- a/tests/unit/moduleapi/cluster.tcl
+++ b/tests/unit/moduleapi/cluster.tcl
@@ -8,8 +8,9 @@ tags {tls:skip external:skip cluster modules} {
 set testmodule_nokey [file normalize tests/modules/blockonbackground.so]
 set testmodule_blockedclient [file normalize tests/modules/blockedclient.so]
 set testmodule [file normalize tests/modules/blockonkeys.so]
+set testmodule_auth [file normalize tests/modules/auth.so]
 
-set modules [list loadmodule $testmodule loadmodule $testmodule_nokey loadmodule $testmodule_blockedclient]
+set modules [list loadmodule $testmodule loadmodule $testmodule_nokey loadmodule $testmodule_blockedclient loadmodule $testmodule_auth]
 start_cluster 3 0 [list config_lines $modules] {
 
     set node1 [srv 0 client]
@@ -146,18 +147,27 @@ start_cluster 3 0 [list config_lines $modules] {
         assert_error {*CLUSTERDOWN*} {$node1_rd read}
     }
 
-    test "Verify command (no keys) got unblocked after cluster failure" {
-        assert_error {*CLUSTERDOWN*} {$node2_rd read}
-
-        # verify there are no blocked clients
-        assert_equal [s 0 blocked_clients]  {0}
-        assert_equal [s -1 blocked_clients]  {0}
+    test "Verify command (with no keys) is not unblocked after cluster failure" {
+        assert_no_match {*CLUSTERDOWN*} {$node2_rd read}
+        # verify there are blocked clients
+        assert_equal [s -1 blocked_clients]  {1}
     }
 
     test "Verify command RM_Call is rejected when cluster is down" {
         assert_error "ERR Can not execute a command 'set' while the cluster is down" {$node1 do_rm_call set x 1}
     }
 
+    test "Verify Module Auth Succeeds when cluster is down" {
+        r acl setuser foo >pwd on ~* &* +@all
+        assert_error "*CLUSTERDOWN*" {r set x 1}
+        # Non Blocking Module Auth
+        assert_equal {OK} [r testmoduleone.rm_register_auth_cb]
+        assert_equal {OK} [r AUTH foo allow]
+        # Blocking Module Auth
+        assert_equal {OK} [r testmoduleone.rm_register_blocking_auth_cb]
+        assert_equal {OK} [r AUTH foo block_allow]
+    }
+
     resume_process $node3_pid
     $node1_rd close
     $node2_rd close