From ac171ade75da485f67b0f7c92c478451c9e29463 Mon Sep 17 00:00:00 2001 From: Ricardo Dias Date: Mon, 6 Oct 2025 13:12:09 +0100 Subject: [PATCH 1/8] Add ValkeyModule_ACLCheckCommandPermissionsForContextUser API Introduces a new module API function that allows modules to check if a command can be executed by the current module context user according to ACL rules. This function provides a convenient wrapper around the existing `ValkeyModule_ACLCheckCommandPermissions` API by automatically using the user associated with the module context, eliminating the need for modules to manually retrieve the user. This addition simplifies ACL permission checking for modules that need to validate commands against the current context user. Signed-off-by: Ricardo Dias --- src/module.c | 27 +++++++++++++++++++++++++++ src/valkeymodule.h | 4 ++++ 2 files changed, 31 insertions(+) diff --git a/src/module.c b/src/module.c index e5afa952fa2..49838e448a8 100644 --- a/src/module.c +++ b/src/module.c @@ -9993,6 +9993,32 @@ int VM_ACLCheckCommandPermissions(ValkeyModuleUser *user, ValkeyModuleString **a return VALKEYMODULE_OK; } +/* Checks if the command can be executed by the module context user, according to the ACLs + * associated with it. + * + * On success a VALKEYMODULE_OK is returned, otherwise + * VALKEYMODULE_ERR is returned and errno is set to the following values: + * + * * ENOENT: Specified command does not exist. + * * EACCES: Command cannot be executed, according to ACL rules; or the user is not authenticated. + */ +int VM_ACLCheckCommandPermissionsForContextUser(ValkeyModuleCtx *ctx, ValkeyModuleString **argv, int argc) { + serverAssert(ctx != NULL && ctx->client != NULL && ctx->client->user != NULL); + + /* If the user is not authenticated, we cannot check permissions. */ + if (ctx->client->user == DefaultUser) { + errno = EACCES; + return VALKEYMODULE_ERR; + } + + ValkeyModuleUser user = { + .user = ctx->client->user, + .free_user = 0, + }; + + return VM_ACLCheckCommandPermissions(&user, argv, argc); +} + /* Check if the key can be accessed by the user according to the ACLs attached to the user * and the flags representing the key access. The flags are the same that are used in the * keyspec for logical operations. These flags are documented in ValkeyModule_SetCommandInfo as @@ -14330,6 +14356,7 @@ void moduleRegisterCoreAPI(void) { REGISTER_API(GetCurrentUserName); REGISTER_API(GetModuleUserFromUserName); REGISTER_API(ACLCheckCommandPermissions); + REGISTER_API(ACLCheckCommandPermissionsForContextUser); REGISTER_API(ACLCheckKeyPermissions); REGISTER_API(ACLCheckChannelPermissions); REGISTER_API(ACLAddLogEntry); diff --git a/src/valkeymodule.h b/src/valkeymodule.h index 2d5dbca7b95..471cefef130 100644 --- a/src/valkeymodule.h +++ b/src/valkeymodule.h @@ -1844,6 +1844,9 @@ VALKEYMODULE_API ValkeyModuleUser *(*ValkeyModule_GetModuleUserFromUserName)(Val VALKEYMODULE_API int (*ValkeyModule_ACLCheckCommandPermissions)(ValkeyModuleUser *user, ValkeyModuleString **argv, int argc) VALKEYMODULE_ATTR; +VALKEYMODULE_API int (*ValkeyModule_ACLCheckCommandPermissionsForContextUser)(ValkeyModuleCtx *ctx, + ValkeyModuleString **argv, + int argc) VALKEYMODULE_ATTR; VALKEYMODULE_API int (*ValkeyModule_ACLCheckKeyPermissions)(ValkeyModuleUser *user, ValkeyModuleString *key, int flags) VALKEYMODULE_ATTR; @@ -2295,6 +2298,7 @@ static int ValkeyModule_Init(ValkeyModuleCtx *ctx, const char *name, int ver, in VALKEYMODULE_GET_API(GetCurrentUserName); VALKEYMODULE_GET_API(GetModuleUserFromUserName); VALKEYMODULE_GET_API(ACLCheckCommandPermissions); + VALKEYMODULE_GET_API(ACLCheckCommandPermissionsForContextUser); VALKEYMODULE_GET_API(ACLCheckKeyPermissions); VALKEYMODULE_GET_API(ACLCheckChannelPermissions); VALKEYMODULE_GET_API(ACLAddLogEntry); From 71ce27c1f455f16df919f6a98bf49fa3f9f5dd93 Mon Sep 17 00:00:00 2001 From: Ricardo Dias Date: Mon, 6 Oct 2025 14:31:23 +0100 Subject: [PATCH 2/8] Use new API in existing module test Signed-off-by: Ricardo Dias --- tests/modules/aclcheck.c | 54 ++++++++++++++++++++++++++-------------- 1 file changed, 35 insertions(+), 19 deletions(-) diff --git a/tests/modules/aclcheck.c b/tests/modules/aclcheck.c index f7474c2e00f..668e9539c13 100644 --- a/tests/modules/aclcheck.c +++ b/tests/modules/aclcheck.c @@ -82,13 +82,13 @@ int publish_aclcheck_channel(ValkeyModuleCtx *ctx, ValkeyModuleString **argv, in } /* A wrap for RM_Call that check first that the command can be executed */ -int rm_call_aclcheck_cmd(ValkeyModuleCtx *ctx, ValkeyModuleUser *user, ValkeyModuleString **argv, int argc) { +int rm_call_aclcheck_context_user_cmd(ValkeyModuleCtx *ctx, ValkeyModuleString **argv, int argc) { if (argc < 2) { return ValkeyModule_WrongArity(ctx); } /* Check that the command can be executed */ - int ret = ValkeyModule_ACLCheckCommandPermissions(user, argv + 1, argc - 1); + int ret = ValkeyModule_ACLCheckCommandPermissionsForContextUser(ctx, argv + 1, argc - 1); if (ret != 0) { ValkeyModule_ReplyWithError(ctx, "DENIED CMD"); /* Add entry to ACL log */ @@ -109,15 +109,31 @@ int rm_call_aclcheck_cmd(ValkeyModuleCtx *ctx, ValkeyModuleUser *user, ValkeyMod return VALKEYMODULE_OK; } -int rm_call_aclcheck_cmd_default_user(ValkeyModuleCtx *ctx, ValkeyModuleString **argv, int argc) { - ValkeyModuleString *user_name = ValkeyModule_GetCurrentUserName(ctx); - ValkeyModuleUser *user = ValkeyModule_GetModuleUserFromUserName(user_name); +int rm_call_aclcheck_cmd(ValkeyModuleCtx *ctx, ValkeyModuleUser *user, ValkeyModuleString **argv, int argc) { + if (argc < 2) { + return ValkeyModule_WrongArity(ctx); + } - int res = rm_call_aclcheck_cmd(ctx, user, argv, argc); + /* Check that the command can be executed */ + int ret = ValkeyModule_ACLCheckCommandPermissions(user, argv + 1, argc - 1); + if (ret != 0) { + ValkeyModule_ReplyWithError(ctx, "DENIED CMD"); + /* Add entry to ACL log */ + ValkeyModule_ACLAddLogEntry(ctx, user, argv[1], VALKEYMODULE_ACL_LOG_CMD); + return VALKEYMODULE_OK; + } - ValkeyModule_FreeModuleUser(user); - ValkeyModule_FreeString(ctx, user_name); - return res; + const char* cmd = ValkeyModule_StringPtrLen(argv[1], NULL); + + ValkeyModuleCallReply* rep = ValkeyModule_Call(ctx, cmd, "v", argv + 2, (size_t)argc - 2); + if(!rep){ + ValkeyModule_ReplyWithError(ctx, "NULL reply returned"); + }else{ + ValkeyModule_ReplyWithCallReply(ctx, rep); + ValkeyModule_FreeCallReply(rep); + } + + return VALKEYMODULE_OK; } int rm_call_aclcheck_cmd_module_user(ValkeyModuleCtx *ctx, ValkeyModuleString **argv, int argc) { @@ -199,14 +215,14 @@ int commandBlockCheck(ValkeyModuleCtx *ctx, ValkeyModuleString **argv, int argc) result = ValkeyModule_AddACLCategory(ctx,"blockedcategory"); response_ok |= (result == VALKEYMODULE_OK); - + ValkeyModuleCommand *parent = ValkeyModule_GetCommand(ctx,"block.commands.outside.onload"); result = ValkeyModule_SetCommandACLCategories(parent, "write"); response_ok |= (result == VALKEYMODULE_OK); result = ValkeyModule_CreateSubcommand(parent,"subcommand.that.should.fail",module_test_acl_category,"",0,0,0); response_ok |= (result == VALKEYMODULE_OK); - + /* This validates that it's not possible to create commands or add * a new ACL Category outside OnLoad function. * thus returns an error if they succeed. */ @@ -224,7 +240,7 @@ int ValkeyModule_OnLoad(ValkeyModuleCtx *ctx, ValkeyModuleString **argv, int arg return VALKEYMODULE_ERR; if (argc > 1) return ValkeyModule_WrongArity(ctx); - + /* When that flag is passed, we try to create too many categories, * and the test expects this to fail. In this case the server returns VALKEYMODULE_ERR * and set errno to ENOMEM*/ @@ -274,7 +290,7 @@ int ValkeyModule_OnLoad(ValkeyModuleCtx *ctx, ValkeyModuleString **argv, int arg if (ValkeyModule_CreateCommand(ctx,"aclcheck.publish.check.channel", publish_aclcheck_channel,"",0,0,0) == VALKEYMODULE_ERR) return VALKEYMODULE_ERR; - if (ValkeyModule_CreateCommand(ctx,"aclcheck.rm_call.check.cmd", rm_call_aclcheck_cmd_default_user,"",0,0,0) == VALKEYMODULE_ERR) + if (ValkeyModule_CreateCommand(ctx,"aclcheck.rm_call.check.cmd", rm_call_aclcheck_context_user_cmd,"",0,0,0) == VALKEYMODULE_ERR) return VALKEYMODULE_ERR; if (ValkeyModule_CreateCommand(ctx,"aclcheck.rm_call.check.cmd.module.user", rm_call_aclcheck_cmd_module_user,"",0,0,0) == VALKEYMODULE_ERR) @@ -292,25 +308,25 @@ int ValkeyModule_OnLoad(ValkeyModuleCtx *ctx, ValkeyModuleString **argv, int arg * the server returns VALKEYMODULE_ERR and set errno to `EINVAL` */ if (ValkeyModule_AddACLCategory(ctx,"!nval!dch@r@cter$") == VALKEYMODULE_ERR) ValkeyModule_Assert(errno == EINVAL); - else + else return VALKEYMODULE_ERR; - + /* This validates that, when module tries to add a category that already exists, * the server returns VALKEYMODULE_ERR and set errno to `EBUSY` */ if (ValkeyModule_AddACLCategory(ctx,"write") == VALKEYMODULE_ERR) ValkeyModule_Assert(errno == EBUSY); - else + else return VALKEYMODULE_ERR; - + if (ValkeyModule_AddACLCategory(ctx,"foocategory") == VALKEYMODULE_ERR) return VALKEYMODULE_ERR; - + if (ValkeyModule_CreateCommand(ctx,"aclcheck.module.command.test.add.new.aclcategories", module_test_acl_category,"",0,0,0) == VALKEYMODULE_ERR) return VALKEYMODULE_ERR; ValkeyModuleCommand *test_add_new_aclcategories = ValkeyModule_GetCommand(ctx,"aclcheck.module.command.test.add.new.aclcategories"); if (ValkeyModule_SetCommandACLCategories(test_add_new_aclcategories, "foocategory") == VALKEYMODULE_ERR) return VALKEYMODULE_ERR; - + return VALKEYMODULE_OK; } From 5111bd44608f6c46febf8eec8ab2d7c19ed9a4ec Mon Sep 17 00:00:00 2001 From: Ricardo Dias Date: Tue, 7 Oct 2025 14:28:03 +0100 Subject: [PATCH 3/8] remove nonsensical condition Signed-off-by: Ricardo Dias --- src/module.c | 6 ------ tests/modules/aclcheck.c | 4 ++++ 2 files changed, 4 insertions(+), 6 deletions(-) diff --git a/src/module.c b/src/module.c index 49838e448a8..518139fb89b 100644 --- a/src/module.c +++ b/src/module.c @@ -10005,12 +10005,6 @@ int VM_ACLCheckCommandPermissions(ValkeyModuleUser *user, ValkeyModuleString **a int VM_ACLCheckCommandPermissionsForContextUser(ValkeyModuleCtx *ctx, ValkeyModuleString **argv, int argc) { serverAssert(ctx != NULL && ctx->client != NULL && ctx->client->user != NULL); - /* If the user is not authenticated, we cannot check permissions. */ - if (ctx->client->user == DefaultUser) { - errno = EACCES; - return VALKEYMODULE_ERR; - } - ValkeyModuleUser user = { .user = ctx->client->user, .free_user = 0, diff --git a/tests/modules/aclcheck.c b/tests/modules/aclcheck.c index 668e9539c13..9bff520204f 100644 --- a/tests/modules/aclcheck.c +++ b/tests/modules/aclcheck.c @@ -92,7 +92,11 @@ int rm_call_aclcheck_context_user_cmd(ValkeyModuleCtx *ctx, ValkeyModuleString * if (ret != 0) { ValkeyModule_ReplyWithError(ctx, "DENIED CMD"); /* Add entry to ACL log */ + ValkeyModuleString *user_name = ValkeyModule_GetCurrentUserName(ctx); + ValkeyModuleUser *user = ValkeyModule_GetModuleUserFromUserName(user_name); ValkeyModule_ACLAddLogEntry(ctx, user, argv[1], VALKEYMODULE_ACL_LOG_CMD); + ValkeyModule_FreeModuleUser(user); + ValkeyModule_FreeString(ctx, user_name); return VALKEYMODULE_OK; } From 8238124cfb6c81672bdaee5d50186665c657c3e3 Mon Sep 17 00:00:00 2001 From: Ricardo Dias Date: Tue, 7 Oct 2025 15:20:32 +0100 Subject: [PATCH 4/8] Rename to VM_ACLCheckCommandPermissionsForCurrentUser Signed-off-by: Ricardo Dias --- src/module.c | 4 ++-- src/valkeymodule.h | 4 ++-- tests/modules/aclcheck.c | 2 +- 3 files changed, 5 insertions(+), 5 deletions(-) diff --git a/src/module.c b/src/module.c index 518139fb89b..30e613eedc8 100644 --- a/src/module.c +++ b/src/module.c @@ -10002,7 +10002,7 @@ int VM_ACLCheckCommandPermissions(ValkeyModuleUser *user, ValkeyModuleString **a * * ENOENT: Specified command does not exist. * * EACCES: Command cannot be executed, according to ACL rules; or the user is not authenticated. */ -int VM_ACLCheckCommandPermissionsForContextUser(ValkeyModuleCtx *ctx, ValkeyModuleString **argv, int argc) { +int VM_ACLCheckCommandPermissionsForCurrentUser(ValkeyModuleCtx *ctx, ValkeyModuleString **argv, int argc) { serverAssert(ctx != NULL && ctx->client != NULL && ctx->client->user != NULL); ValkeyModuleUser user = { @@ -14350,7 +14350,7 @@ void moduleRegisterCoreAPI(void) { REGISTER_API(GetCurrentUserName); REGISTER_API(GetModuleUserFromUserName); REGISTER_API(ACLCheckCommandPermissions); - REGISTER_API(ACLCheckCommandPermissionsForContextUser); + REGISTER_API(ACLCheckCommandPermissionsForCurrentUser); REGISTER_API(ACLCheckKeyPermissions); REGISTER_API(ACLCheckChannelPermissions); REGISTER_API(ACLAddLogEntry); diff --git a/src/valkeymodule.h b/src/valkeymodule.h index 471cefef130..d27fc168232 100644 --- a/src/valkeymodule.h +++ b/src/valkeymodule.h @@ -1844,7 +1844,7 @@ VALKEYMODULE_API ValkeyModuleUser *(*ValkeyModule_GetModuleUserFromUserName)(Val VALKEYMODULE_API int (*ValkeyModule_ACLCheckCommandPermissions)(ValkeyModuleUser *user, ValkeyModuleString **argv, int argc) VALKEYMODULE_ATTR; -VALKEYMODULE_API int (*ValkeyModule_ACLCheckCommandPermissionsForContextUser)(ValkeyModuleCtx *ctx, +VALKEYMODULE_API int (*ValkeyModule_ACLCheckCommandPermissionsForCurrentUser)(ValkeyModuleCtx *ctx, ValkeyModuleString **argv, int argc) VALKEYMODULE_ATTR; VALKEYMODULE_API int (*ValkeyModule_ACLCheckKeyPermissions)(ValkeyModuleUser *user, @@ -2298,7 +2298,7 @@ static int ValkeyModule_Init(ValkeyModuleCtx *ctx, const char *name, int ver, in VALKEYMODULE_GET_API(GetCurrentUserName); VALKEYMODULE_GET_API(GetModuleUserFromUserName); VALKEYMODULE_GET_API(ACLCheckCommandPermissions); - VALKEYMODULE_GET_API(ACLCheckCommandPermissionsForContextUser); + VALKEYMODULE_GET_API(ACLCheckCommandPermissionsForCurrentUser); VALKEYMODULE_GET_API(ACLCheckKeyPermissions); VALKEYMODULE_GET_API(ACLCheckChannelPermissions); VALKEYMODULE_GET_API(ACLAddLogEntry); diff --git a/tests/modules/aclcheck.c b/tests/modules/aclcheck.c index 9bff520204f..9833cc49b95 100644 --- a/tests/modules/aclcheck.c +++ b/tests/modules/aclcheck.c @@ -88,7 +88,7 @@ int rm_call_aclcheck_context_user_cmd(ValkeyModuleCtx *ctx, ValkeyModuleString * } /* Check that the command can be executed */ - int ret = ValkeyModule_ACLCheckCommandPermissionsForContextUser(ctx, argv + 1, argc - 1); + int ret = ValkeyModule_ACLCheckCommandPermissionsForCurrentUser(ctx, argv + 1, argc - 1); if (ret != 0) { ValkeyModule_ReplyWithError(ctx, "DENIED CMD"); /* Add entry to ACL log */ From cfc47a6e996f0644543224766ff5ac28d94c1b0e Mon Sep 17 00:00:00 2001 From: Ricardo Dias Date: Wed, 8 Oct 2025 09:29:26 +0100 Subject: [PATCH 5/8] fix formatting Signed-off-by: Ricardo Dias --- tests/modules/aclcheck.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/tests/modules/aclcheck.c b/tests/modules/aclcheck.c index 9833cc49b95..f178b7b0694 100644 --- a/tests/modules/aclcheck.c +++ b/tests/modules/aclcheck.c @@ -130,9 +130,9 @@ int rm_call_aclcheck_cmd(ValkeyModuleCtx *ctx, ValkeyModuleUser *user, ValkeyMod const char* cmd = ValkeyModule_StringPtrLen(argv[1], NULL); ValkeyModuleCallReply* rep = ValkeyModule_Call(ctx, cmd, "v", argv + 2, (size_t)argc - 2); - if(!rep){ + if (!rep) { ValkeyModule_ReplyWithError(ctx, "NULL reply returned"); - }else{ + } else { ValkeyModule_ReplyWithCallReply(ctx, rep); ValkeyModule_FreeCallReply(rep); } From 0273be07cd8a2d56e991f238593ff9fc323f7892 Mon Sep 17 00:00:00 2001 From: Ricardo Dias Date: Wed, 8 Oct 2025 15:07:30 +0100 Subject: [PATCH 6/8] change logic to error out when no user is in context Signed-off-by: Ricardo Dias --- src/module.c | 12 +++++++++++- 1 file changed, 11 insertions(+), 1 deletion(-) diff --git a/src/module.c b/src/module.c index 30e613eedc8..43854af03d2 100644 --- a/src/module.c +++ b/src/module.c @@ -10001,9 +10001,19 @@ int VM_ACLCheckCommandPermissions(ValkeyModuleUser *user, ValkeyModuleString **a * * * ENOENT: Specified command does not exist. * * EACCES: Command cannot be executed, according to ACL rules; or the user is not authenticated. + * * EINVAL: There is no user associated with this module context. */ int VM_ACLCheckCommandPermissionsForCurrentUser(ValkeyModuleCtx *ctx, ValkeyModuleString **argv, int argc) { - serverAssert(ctx != NULL && ctx->client != NULL && ctx->client->user != NULL); + if (ctx == NULL || (ctx->user == NULL && (ctx->client == NULL || ctx->client->user == NULL))) { + errno = EINVAL; + return VALKEYMODULE_ERR; + } + + /* If the ctx->user was set by VM_SetContextUser then use that user instead + * of the ctx->client->user. */ + if (ctx->user) { + return VM_ACLCheckCommandPermissions((ValkeyModuleUser *)ctx->user, argv, argc); + } ValkeyModuleUser user = { .user = ctx->client->user, From ac071e63522285aef5e781b7533ce693b8ddace5 Mon Sep 17 00:00:00 2001 From: Ricardo Dias Date: Wed, 8 Oct 2025 17:04:30 +0100 Subject: [PATCH 7/8] error improvement Signed-off-by: Ricardo Dias --- src/module.c | 9 +++++++-- 1 file changed, 7 insertions(+), 2 deletions(-) diff --git a/src/module.c b/src/module.c index 43854af03d2..8809ce3e3e4 100644 --- a/src/module.c +++ b/src/module.c @@ -70,6 +70,7 @@ #include "io_threads.h" #include "scripting_engine.h" #include "cluster_migrateslots.h" +#include #include #include #include @@ -10001,12 +10002,16 @@ int VM_ACLCheckCommandPermissions(ValkeyModuleUser *user, ValkeyModuleString **a * * * ENOENT: Specified command does not exist. * * EACCES: Command cannot be executed, according to ACL rules; or the user is not authenticated. - * * EINVAL: There is no user associated with this module context. + * * EINVAL: Invalid module context. + * * ENOTSUP: There is no user associated with this module context. */ int VM_ACLCheckCommandPermissionsForCurrentUser(ValkeyModuleCtx *ctx, ValkeyModuleString **argv, int argc) { - if (ctx == NULL || (ctx->user == NULL && (ctx->client == NULL || ctx->client->user == NULL))) { + if (ctx == NULL) { errno = EINVAL; return VALKEYMODULE_ERR; + } else if (ctx->user == NULL && (ctx->client == NULL || ctx->client->user == NULL)) { + errno = ENOTSUP; + return VALKEYMODULE_ERR; } /* If the ctx->user was set by VM_SetContextUser then use that user instead From f65208b4347e6afec3b4b1581ca3907c3b2c44cc Mon Sep 17 00:00:00 2001 From: Ricardo Dias Date: Wed, 8 Oct 2025 17:42:25 +0100 Subject: [PATCH 8/8] fix wrong include Signed-off-by: Ricardo Dias --- src/module.c | 1 - 1 file changed, 1 deletion(-) diff --git a/src/module.c b/src/module.c index 8809ce3e3e4..ad3c5ee8408 100644 --- a/src/module.c +++ b/src/module.c @@ -70,7 +70,6 @@ #include "io_threads.h" #include "scripting_engine.h" #include "cluster_migrateslots.h" -#include #include #include #include