diff --git a/src/acl.c b/src/acl.c index e4d7744a7..0054402b3 100644 --- a/src/acl.c +++ b/src/acl.c @@ -2472,6 +2472,22 @@ 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. */ + switch (acl_res) { + case ACL_DENIED_CMD: + return "can't run this command or subcommand"; + case ACL_DENIED_KEY: + return "can't access at least one of the keys mentioned in the command arguments"; + case ACL_DENIED_CHANNEL: + return "can't publish to the channel mentioned in the command"; + default: + return "lacking the permissions for the command"; + } + serverPanic("Reached deadcode on getAclErrorMessage"); +} + /* ============================================================================= * ACL related commands * ==========================================================================*/ @@ -2863,6 +2879,8 @@ setuser_cleanup: 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) { diff --git a/src/call_reply.c b/src/call_reply.c index 3694db55e..759cd792a 100644 --- a/src/call_reply.c +++ b/src/call_reply.c @@ -525,3 +525,18 @@ CallReply *callReplyCreate(sds reply, list *deferred_error_list, void *private_d res->deferred_error_list = deferred_error_list; return res; } + +/* Create a new CallReply struct from the reply blob representing an error message. + * Automatically creating deferred_error_list and set a copy of the reply in it. + * Refer to callReplyCreate for detailed explanation. */ +CallReply *callReplyCreateError(sds reply, void *private_data) { + sds err_buff = reply; + if (err_buff[0] != '-') { + err_buff = sdscatfmt(sdsempty(), "-ERR %S\r\n", reply); + sdsfree(reply); + } + list *deferred_error_list = listCreate(); + listSetFreeMethod(deferred_error_list, (void (*)(void*))sdsfree); + listAddNodeTail(deferred_error_list, sdsnew(err_buff)); + return callReplyCreate(err_buff, deferred_error_list, private_data); +} diff --git a/src/call_reply.h b/src/call_reply.h index ff98f7f5a..ff1c4ba3f 100644 --- a/src/call_reply.h +++ b/src/call_reply.h @@ -35,6 +35,7 @@ typedef struct CallReply CallReply; CallReply *callReplyCreate(sds reply, list *deferred_error_list, void *private_data); +CallReply *callReplyCreateError(sds reply, void *private_data); int callReplyType(CallReply *rep); const char *callReplyGetString(CallReply *rep, size_t *len); long long callReplyGetLongLong(CallReply *rep); diff --git a/src/module.c b/src/module.c index ff928b950..727da4783 100644 --- a/src/module.c +++ b/src/module.c @@ -352,6 +352,9 @@ typedef struct RedisModuleServerInfoData { #define REDISMODULE_ARGV_RESP_3 (1<<3) #define REDISMODULE_ARGV_RESP_AUTO (1<<4) #define REDISMODULE_ARGV_CHECK_ACL (1<<5) +#define REDISMODULE_ARGV_SCRIPT_MODE (1<<6) +#define REDISMODULE_ARGV_NO_WRITES (1<<7) +#define REDISMODULE_ARGV_CALL_REPLIES_AS_ERRORS (1<<8) /* Determine whether Redis should signalModifiedKey implicitly. * In case 'ctx' has no 'module' member (and therefore no module->options), @@ -5548,6 +5551,12 @@ robj **moduleCreateArgvFromUserFormat(const char *cmdname, const char *fmt, int if (flags) (*flags) |= REDISMODULE_ARGV_RESP_AUTO; } else if (*p == 'C') { if (flags) (*flags) |= REDISMODULE_ARGV_CHECK_ACL; + } else if (*p == 'S') { + if (flags) (*flags) |= REDISMODULE_ARGV_SCRIPT_MODE; + } else if (*p == 'W') { + if (flags) (*flags) |= REDISMODULE_ARGV_NO_WRITES; + } else if (*p == 'E') { + if (flags) (*flags) |= REDISMODULE_ARGV_CALL_REPLIES_AS_ERRORS; } else { goto fmterr; } @@ -5587,6 +5596,17 @@ fmterr: * same as the client attached to the given RedisModuleCtx. This will * probably used when you want to pass the reply directly to the client. * * `C` -- Check if command can be executed according to ACL rules. + * * 'S' -- Run the command in a script mode, this means that it will raise + * an error if a command which are not allowed inside a script + * (flagged with the `deny-script` flag) is invoked (like SHUTDOWN). + * In addition, on script mode, write commands are not allowed if there are + * not enough good replicas (as configured with `min-replicas-to-write`) + * or when the server is unable to persist to the disk. + * * 'W' -- Do not allow to run any write command (flagged with the `write` flag). + * * 'E' -- Return error as RedisModuleCallReply. If there is an error before + * invoking the command, the error is returned using errno mechanism. + * This flag allows to get the error also as an error CallReply with + * relevant error message. * * **...**: The actual arguments to the Redis command. * * On success a RedisModuleCallReply object is returned, otherwise @@ -5601,6 +5621,8 @@ fmterr: * * ENETDOWN: operation in Cluster instance when cluster is down. * * ENOTSUP: No ACL user for the specified module context * * EACCES: Command cannot be executed, according to ACL rules + * * ENOSPC: Write command is not allowed + * * ESPIPE: Command not allowed on script mode * * Example code fragment: * @@ -5620,11 +5642,13 @@ RedisModuleCallReply *RM_Call(RedisModuleCtx *ctx, const char *cmdname, const ch va_list ap; RedisModuleCallReply *reply = NULL; int replicate = 0; /* Replicate this command? */ + int error_as_call_replies = 0; /* return errors as RedisModuleCallReply object */ /* Handle arguments. */ va_start(ap, fmt); argv = moduleCreateArgvFromUserFormat(cmdname,fmt,&argc,&argv_len,&flags,ap); replicate = flags & REDISMODULE_ARGV_REPLICATE; + error_as_call_replies = flags & REDISMODULE_ARGV_CALL_REPLIES_AS_ERRORS; va_end(ap); c = moduleAllocTempClient(); @@ -5647,6 +5671,10 @@ RedisModuleCallReply *RM_Call(RedisModuleCtx *ctx, const char *cmdname, const ch /* We handle the above format error only when the client is setup so that * we can free it normally. */ if (argv == NULL) { + /* We do not return a call reply here this is an error that should only + * be catch by the module indicating wrong fmt was given, the module should + * handle this error and decide how to continue. It is not an error that + * should be propagated to the user. */ errno = EBADF; goto cleanup; } @@ -5660,6 +5688,11 @@ RedisModuleCallReply *RM_Call(RedisModuleCtx *ctx, const char *cmdname, const ch cmd = lookupCommand(c->argv,c->argc); if (!cmd) { errno = ENOENT; + if (error_as_call_replies) { + sds msg = sdscatfmt(sdsempty(),"Unknown Redis " + "command '%S'.",c->argv[0]->ptr); + reply = callReplyCreateError(msg, ctx); + } goto cleanup; } c->cmd = c->lastcmd = c->realcmd = cmd; @@ -5667,9 +5700,66 @@ RedisModuleCallReply *RM_Call(RedisModuleCtx *ctx, const char *cmdname, const ch /* Basic arity checks. */ if ((cmd->arity > 0 && cmd->arity != argc) || (argc < -cmd->arity)) { errno = EINVAL; + if (error_as_call_replies) { + sds msg = sdscatfmt(sdsempty(), "Wrong number of " + "args calling Redis command '%S'.", c->cmd->fullname); + reply = callReplyCreateError(msg, ctx); + } goto cleanup; } + if (flags & REDISMODULE_ARGV_SCRIPT_MODE) { + /* Basically on script mode we want to only allow commands that can + * be executed on scripts (CMD_NOSCRIPT is not set on the command flags) */ + if (cmd->flags & CMD_NOSCRIPT) { + errno = ESPIPE; + if (error_as_call_replies) { + sds msg = sdscatfmt(sdsempty(), "command '%S' is not allowed on script mode", c->cmd->fullname); + reply = callReplyCreateError(msg, ctx); + } + goto cleanup; + } + } + + if (cmd->flags & CMD_WRITE) { + if (flags & REDISMODULE_ARGV_NO_WRITES) { + errno = ENOSPC; + if (error_as_call_replies) { + sds msg = sdscatfmt(sdsempty(), "Write command '%S' was " + "called while write is not allowed.", c->cmd->fullname); + reply = callReplyCreateError(msg, ctx); + } + goto cleanup; + } + + if (flags & REDISMODULE_ARGV_SCRIPT_MODE) { + /* on script mode, if a command is a write command, + * We will not run it if we encounter disk error + * or we do not have enough replicas */ + + if (!checkGoodReplicasStatus()) { + errno = ENOSPC; + if (error_as_call_replies) { + sds msg = sdsdup(shared.noreplicaserr->ptr); + reply = callReplyCreateError(msg, ctx); + } + goto cleanup; + } + + int deny_write_type = writeCommandsDeniedByDiskError(); + + if (deny_write_type != DISK_ERROR_TYPE_NONE) { + errno = ENOSPC; + if (error_as_call_replies) { + sds msg = writeCommandsGetDiskErrorMessage(deny_write_type); + reply = callReplyCreateError(msg, ctx); + } + goto cleanup; + } + + } + } + /* Check if the user can run this command according to the current * ACLs. */ if (flags & REDISMODULE_ARGV_CHECK_ACL) { @@ -5678,12 +5768,20 @@ RedisModuleCallReply *RM_Call(RedisModuleCtx *ctx, const char *cmdname, const ch if (ctx->client->user == NULL) { errno = ENOTSUP; + if (error_as_call_replies) { + sds msg = sdsnew("acl verification failed, context is not attached to a client."); + reply = callReplyCreateError(msg, ctx); + } goto cleanup; } acl_retval = ACLCheckAllUserCommandPerm(ctx->client->user,c->cmd,c->argv,c->argc,&acl_errpos); if (acl_retval != ACL_OK) { sds object = (acl_retval == ACL_DENIED_CMD) ? sdsdup(c->cmd->fullname) : sdsdup(c->argv[acl_errpos]->ptr); addACLLogEntry(ctx->client, acl_retval, ACL_LOG_CTX_MODULE, -1, ctx->client->user->name, object); + if (error_as_call_replies) { + sds msg = sdscatfmt(sdsempty(), "acl verification failed, %s.", getAclErrorMessage(acl_retval)); + reply = callReplyCreateError(msg, ctx); + } errno = EACCES; goto cleanup; } @@ -5700,13 +5798,26 @@ RedisModuleCallReply *RM_Call(RedisModuleCtx *ctx, const char *cmdname, const ch if (getNodeByQuery(c,c->cmd,c->argv,c->argc,NULL,&error_code) != server.cluster->myself) { + sds msg = NULL; if (error_code == CLUSTER_REDIR_DOWN_RO_STATE) { + if (error_as_call_replies) { + msg = sdscatfmt(sdsempty(), "Can not execute a write command '%S' while the cluster is down and readonly", c->cmd->fullname); + } errno = EROFS; } else if (error_code == CLUSTER_REDIR_DOWN_STATE) { + if (error_as_call_replies) { + msg = sdscatfmt(sdsempty(), "Can not execute a command '%S' while the cluster is down", c->cmd->fullname); + } errno = ENETDOWN; } else { + if (error_as_call_replies) { + msg = sdsnew("Attempted to access a non local key in a cluster node"); + } errno = EPERM; } + if (msg) { + reply = callReplyCreateError(msg, ctx); + } goto cleanup; } } @@ -5748,9 +5859,9 @@ RedisModuleCallReply *RM_Call(RedisModuleCtx *ctx, const char *cmdname, const ch } reply = callReplyCreate(proto, c->deferred_reply_errors, ctx); c->deferred_reply_errors = NULL; /* now the responsibility of the reply object. */ - autoMemoryAdd(ctx,REDISMODULE_AM_REPLY,reply); cleanup: + if (reply) autoMemoryAdd(ctx,REDISMODULE_AM_REPLY,reply); if (ctx->module) ctx->module->in_call--; moduleReleaseTempClient(c); return reply; diff --git a/src/replication.c b/src/replication.c index c1671dab2..b93c512fc 100644 --- a/src/replication.c +++ b/src/replication.c @@ -3335,6 +3335,14 @@ void refreshGoodSlavesCount(void) { server.repl_good_slaves_count = good; } +/* return true if status of good replicas is OK. otherwise false */ +int checkGoodReplicasStatus(void) { + return server.masterhost || /* not a primary status should be OK */ + !server.repl_min_slaves_max_lag || /* Min slave max lag not configured */ + !server.repl_min_slaves_to_write || /* Min slave to write not configured */ + server.repl_good_slaves_count >= server.repl_min_slaves_to_write; /* check if we have enough slaves */ +} + /* ----------------------- SYNCHRONOUS REPLICATION -------------------------- * Redis synchronous replication design can be summarized in points: * diff --git a/src/script.c b/src/script.c index a34f53e66..990248c45 100644 --- a/src/script.c +++ b/src/script.c @@ -312,25 +312,7 @@ static int scriptVerifyACL(client *c, sds *err) { int acl_retval = ACLCheckAllPerm(c, &acl_errpos); if (acl_retval != ACL_OK) { addACLLogEntry(c,acl_retval,ACL_LOG_CTX_LUA,acl_errpos,NULL,NULL); - switch (acl_retval) { - case ACL_DENIED_CMD: - *err = sdsnew("The user executing the script can't run this " - "command or subcommand"); - break; - case ACL_DENIED_KEY: - *err = sdsnew("The user executing the script can't access " - "at least one of the keys mentioned in the " - "command arguments"); - break; - case ACL_DENIED_CHANNEL: - *err = sdsnew("The user executing the script can't publish " - "to the channel mentioned in the command"); - break; - default: - *err = sdsnew("The user executing the script is lacking the " - "permissions for the command"); - break; - } + *err = sdscatfmt(sdsempty(), "The user executing the script %s", getAclErrorMessage(acl_retval)); return C_ERR; } return C_OK; @@ -360,14 +342,7 @@ static int scriptVerifyWriteCommandAllow(scriptRunCtx *run_ctx, char **err) { } if (deny_write_type != DISK_ERROR_TYPE_NONE) { - if (deny_write_type == DISK_ERROR_TYPE_RDB) { - *err = sdsdup(shared.bgsaveerr->ptr); - } else { - *err = sdsempty(); - *err = sdscatfmt(*err, - "-MISCONF Errors writing to the AOF file: %s\r\n", - strerror(server.aof_last_write_errno)); - } + *err = writeCommandsGetDiskErrorMessage(deny_write_type); return C_ERR; } @@ -375,11 +350,7 @@ static int scriptVerifyWriteCommandAllow(scriptRunCtx *run_ctx, char **err) { * user configured the min-slaves-to-write option. Note this only reachable * for Eval scripts that didn't declare flags, see the other check in * scriptPrepareForRun */ - if (server.masterhost == NULL && - server.repl_min_slaves_max_lag && - server.repl_min_slaves_to_write && - server.repl_good_slaves_count < server.repl_min_slaves_to_write) - { + if (!checkGoodReplicasStatus()) { *err = sdsdup(shared.noreplicaserr->ptr); return C_ERR; } diff --git a/src/server.c b/src/server.c index 3bf04a770..531709d8a 100644 --- a/src/server.c +++ b/src/server.c @@ -3426,6 +3426,16 @@ void rejectCommand(client *c, robj *reply) { } } +void rejectCommandSds(client *c, sds s) { + if (c->cmd && c->cmd->proc == execCommand) { + execCommandAbort(c, s); + sdsfree(s); + } else { + /* The following frees 's'. */ + addReplyErrorSds(c, s); + } +} + void rejectCommandFormat(client *c, const char *fmt, ...) { if (c->cmd) c->cmd->rejected_calls++; flagTransaction(c); @@ -3436,13 +3446,7 @@ void rejectCommandFormat(client *c, const char *fmt, ...) { /* Make sure there are no newlines in the string, otherwise invalid protocol * is emitted (The args come from the user, they may contain any character). */ sdsmapchars(s, "\r\n", " ", 2); - if (c->cmd && c->cmd->proc == execCommand) { - execCommandAbort(c, s); - sdsfree(s); - } else { - /* The following frees 's'. */ - addReplyErrorSds(c, s); - } + rejectCommandSds(c, s); } /* This is called after a command in call, we can do some maintenance job in it. */ @@ -3725,23 +3729,14 @@ int processCommand(client *c) { server.masterhost == NULL && (is_write_command ||c->cmd->proc == pingCommand)) { - if (deny_write_type == DISK_ERROR_TYPE_RDB) - rejectCommand(c, shared.bgsaveerr); - else - rejectCommandFormat(c, - "-MISCONF Errors writing to the AOF file: %s", - strerror(server.aof_last_write_errno)); + sds err = writeCommandsGetDiskErrorMessage(deny_write_type); + rejectCommandSds(c, err); return C_OK; } /* Don't accept write commands if there are not enough good slaves and * user configured the min-slaves-to-write option. */ - if (server.masterhost == NULL && - server.repl_min_slaves_to_write && - server.repl_min_slaves_max_lag && - is_write_command && - server.repl_good_slaves_count < server.repl_min_slaves_to_write) - { + if (is_write_command && !checkGoodReplicasStatus()) { rejectCommand(c, shared.noreplicaserr); return C_OK; } @@ -4166,6 +4161,18 @@ int writeCommandsDeniedByDiskError(void) { return DISK_ERROR_TYPE_NONE; } +sds writeCommandsGetDiskErrorMessage(int error_code) { + sds ret = NULL; + if (error_code == DISK_ERROR_TYPE_RDB) { + ret = sdsdup(shared.bgsaveerr->ptr); + } else { + ret = sdscatfmt(sdsempty(), + "-MISCONF Errors writing to the AOF file: %s", + strerror(server.aof_last_write_errno)); + } + return ret; +} + /* The PING command. It works in a different way if the client is in * in Pub/Sub mode. */ void pingCommand(client *c) { diff --git a/src/server.h b/src/server.h index 4f261e91a..5f5417de1 100644 --- a/src/server.h +++ b/src/server.h @@ -2649,6 +2649,7 @@ void resizeReplicationBacklog(); void replicationSetMaster(char *ip, int port); void replicationUnsetMaster(void); void refreshGoodSlavesCount(void); +int checkGoodReplicasStatus(void); void processClientsWaitingReplicas(void); void unblockClientWaitingReplicas(client *c); int replicationCountAcksByOffset(long long offset); @@ -2689,6 +2690,7 @@ int allPersistenceDisabled(void); #define DISK_ERROR_TYPE_RDB 2 /* Don't accept writes: RDB errors. */ #define DISK_ERROR_TYPE_NONE 0 /* No problems, we can accept writes. */ int writeCommandsDeniedByDiskError(void); +sds writeCommandsGetDiskErrorMessage(int); /* RDB persistence */ #include "rdb.h" @@ -2770,6 +2772,7 @@ void addReplyCommandCategories(client *c, struct redisCommand *cmd); user *ACLCreateUnlinkedUser(); void ACLFreeUserAndKillClients(user *u); void addACLLogEntry(client *c, int reason, int context, int argpos, sds username, sds object); +const char* getAclErrorMessage(int acl_res); void ACLUpdateDefaultUserPassword(sds password); /* Sorted sets data type */ diff --git a/tests/modules/aclcheck.c b/tests/modules/aclcheck.c index 0e9c9af29..8a9d468a6 100644 --- a/tests/modules/aclcheck.c +++ b/tests/modules/aclcheck.c @@ -136,6 +136,22 @@ int rm_call_aclcheck_cmd_module_user(RedisModuleCtx *ctx, RedisModuleString **ar return res; } +int rm_call_aclcheck_with_errors(RedisModuleCtx *ctx, RedisModuleString **argv, int argc){ + REDISMODULE_NOT_USED(argv); + REDISMODULE_NOT_USED(argc); + + if(argc < 2){ + return RedisModule_WrongArity(ctx); + } + + const char* cmd = RedisModule_StringPtrLen(argv[1], NULL); + + RedisModuleCallReply* rep = RedisModule_Call(ctx, cmd, "vEC", argv + 2, argc - 2); + RedisModule_ReplyWithCallReply(ctx, rep); + RedisModule_FreeCallReply(rep); + return REDISMODULE_OK; +} + /* A wrap for RM_Call that pass the 'C' flag to do ACL check on the command. */ int rm_call_aclcheck(RedisModuleCtx *ctx, RedisModuleString **argv, int argc){ REDISMODULE_NOT_USED(argv); @@ -190,5 +206,9 @@ int RedisModule_OnLoad(RedisModuleCtx *ctx, RedisModuleString **argv, int argc) "write",0,0,0) == REDISMODULE_ERR) return REDISMODULE_ERR; + if (RedisModule_CreateCommand(ctx,"aclcheck.rm_call_with_errors", rm_call_aclcheck_with_errors, + "write",0,0,0) == REDISMODULE_ERR) + return REDISMODULE_ERR; + return REDISMODULE_OK; } diff --git a/tests/modules/basics.c b/tests/modules/basics.c index 4d639d682..ecd1b8852 100644 --- a/tests/modules/basics.c +++ b/tests/modules/basics.c @@ -718,6 +718,25 @@ end: /* Return 1 if the reply matches the specified string, otherwise log errors * in the server log and return 0. */ +int TestAssertErrorReply(RedisModuleCtx *ctx, RedisModuleCallReply *reply, char *str, size_t len) { + RedisModuleString *mystr, *expected; + if (RedisModule_CallReplyType(reply) != REDISMODULE_REPLY_ERROR) { + return 0; + } + + mystr = RedisModule_CreateStringFromCallReply(reply); + expected = RedisModule_CreateString(ctx,str,len); + if (RedisModule_StringCompare(mystr,expected) != 0) { + const char *mystr_ptr = RedisModule_StringPtrLen(mystr,NULL); + const char *expected_ptr = RedisModule_StringPtrLen(expected,NULL); + RedisModule_Log(ctx,"warning", + "Unexpected Error reply reply '%s' (instead of '%s')", + mystr_ptr, expected_ptr); + return 0; + } + return 1; +} + int TestAssertStringReply(RedisModuleCtx *ctx, RedisModuleCallReply *reply, char *str, size_t len) { RedisModuleString *mystr, *expected; @@ -846,6 +865,18 @@ int TestBasics(RedisModuleCtx *ctx, RedisModuleString **argv, int argc) { if (!TestAssertStringReply(ctx,RedisModule_CallReplyArrayElement(reply, 0),"test",4)) goto fail; if (!TestAssertStringReply(ctx,RedisModule_CallReplyArrayElement(reply, 1),"1234",4)) goto fail; + T("foo", "E"); + if (!TestAssertErrorReply(ctx,reply,"ERR Unknown Redis command 'foo'.",32)) goto fail; + + T("set", "Ec", "x"); + if (!TestAssertErrorReply(ctx,reply,"ERR Wrong number of args calling Redis command 'set'.",53)) goto fail; + + T("shutdown", "SE"); + if (!TestAssertErrorReply(ctx,reply,"ERR command 'shutdown' is not allowed on script mode",52)) goto fail; + + T("set", "WEcc", "x", "1"); + if (!TestAssertErrorReply(ctx,reply,"ERR Write command 'set' was called while write is not allowed.",62)) goto fail; + RedisModule_ReplyWithSimpleString(ctx,"ALL TESTS PASSED"); return REDISMODULE_OK; diff --git a/tests/modules/blockedclient.c b/tests/modules/blockedclient.c index a2d7c6d00..9c2598a34 100644 --- a/tests/modules/blockedclient.c +++ b/tests/modules/blockedclient.c @@ -195,7 +195,7 @@ int do_rm_call(RedisModuleCtx *ctx, RedisModuleString **argv, int argc){ const char* cmd = RedisModule_StringPtrLen(argv[1], NULL); - RedisModuleCallReply* rep = RedisModule_Call(ctx, cmd, "v", argv + 2, argc - 2); + RedisModuleCallReply* rep = RedisModule_Call(ctx, cmd, "Ev", argv + 2, argc - 2); if(!rep){ RedisModule_ReplyWithError(ctx, "NULL reply returned"); }else{ diff --git a/tests/unit/moduleapi/aclcheck.tcl b/tests/unit/moduleapi/aclcheck.tcl index 953f4bf05..5adf65371 100644 --- a/tests/unit/moduleapi/aclcheck.tcl +++ b/tests/unit/moduleapi/aclcheck.tcl @@ -62,10 +62,13 @@ start_server {tags {"modules acl"}} { # rm call check for key permission (y: only WRITE) assert_equal [r aclcheck.rm_call set y 5] OK assert_error {*NOPERM*} {r aclcheck.rm_call set y 5 get} + assert_error {ERR acl verification failed, can't access at least one of the keys mentioned in the command arguments.} {r aclcheck.rm_call_with_errors set y 5 get} # rm call check for key permission (z: only READ) assert_error {*NOPERM*} {r aclcheck.rm_call set z 5} + assert_error {ERR acl verification failed, can't access at least one of the keys mentioned in the command arguments.} {r aclcheck.rm_call_with_errors set z 5} assert_error {*NOPERM*} {r aclcheck.rm_call set z 6 get} + assert_error {ERR acl verification failed, can't access at least one of the keys mentioned in the command arguments.} {r aclcheck.rm_call_with_errors set z 6 get} # verify that new log entry added set entry [lindex [r ACL LOG] 0] @@ -77,6 +80,8 @@ start_server {tags {"modules acl"}} { r acl setuser default -set catch {r aclcheck.rm_call set x 5} e assert_match {*NOPERM*} $e + catch {r aclcheck.rm_call_with_errors set x 5} e + assert_match {ERR acl verification failed, can't run this command or subcommand.} $e # verify that new log entry added set entry [lindex [r ACL LOG] 0] diff --git a/tests/unit/moduleapi/blockedclient.tcl b/tests/unit/moduleapi/blockedclient.tcl index ea2d6f5a4..de3cf5946 100644 --- a/tests/unit/moduleapi/blockedclient.tcl +++ b/tests/unit/moduleapi/blockedclient.tcl @@ -184,17 +184,17 @@ start_server {tags {"modules"}} { r config resetstat # simple module command that replies with string error - assert_error "NULL reply returned" {r do_rm_call hgetalllll} - assert_equal [errorrstat NULL r] {count=1} + assert_error "ERR Unknown Redis command 'hgetalllll'." {r do_rm_call hgetalllll} + assert_equal [errorrstat ERR r] {count=1} # module command that replies with string error from bg thread assert_error "NULL reply returned" {r do_bg_rm_call hgetalllll} - assert_equal [errorrstat NULL r] {count=2} + assert_equal [errorrstat NULL r] {count=1} # module command that returns an arity error r do_rm_call set x x assert_error "ERR wrong number of arguments for 'do_rm_call' command" {r do_rm_call} - assert_equal [errorrstat ERR r] {count=1} + assert_equal [errorrstat ERR r] {count=2} # RM_Call that propagates an error assert_error "WRONGTYPE*" {r do_rm_call hgetall x} diff --git a/tests/unit/moduleapi/cluster.tcl b/tests/unit/moduleapi/cluster.tcl index 3bd4977fd..f1238992d 100644 --- a/tests/unit/moduleapi/cluster.tcl +++ b/tests/unit/moduleapi/cluster.tcl @@ -20,6 +20,7 @@ proc csi {args} { set testmodule [file normalize tests/modules/blockonkeys.so] set testmodule_nokey [file normalize tests/modules/blockonbackground.so] +set testmodule_blockedclient [file normalize tests/modules/blockedclient.so] # make sure the test infra won't use SELECT set old_singledb $::singledb @@ -44,6 +45,10 @@ start_server [list overrides $base_conf] { $node2 module load $testmodule_nokey $node3 module load $testmodule_nokey + $node1 module load $testmodule_blockedclient + $node2 module load $testmodule_blockedclient + $node3 module load $testmodule_blockedclient + test {Create 3 node cluster} { exec src/redis-cli --cluster-yes --cluster create \ 127.0.0.1:[srv 0 port] \ @@ -194,6 +199,10 @@ start_server [list overrides $base_conf] { assert_equal [s -1 blocked_clients] {0} } + 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} + } + exec kill -SIGCONT $node3_pid $node1_rd close $node2_rd close