Skip to content

Add ValkeyModule_ACLCheckCommandPermissionsForCurrentUser API #2690

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Draft
wants to merge 8 commits into
base: unstable
Choose a base branch
from
+78 −19
Draft

Add ValkeyModule_ACLCheckCommandPermissionsForCurrentUser API #2690

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
ac171ad
Add ValkeyModule_ACLCheckCommandPermissionsForContextUser API
rjd15372 Oct 6, 2025
71ce27c
Use new API in existing module test
rjd15372 Oct 6, 2025
5111bd4
remove nonsensical condition
rjd15372 Oct 7, 2025
8238124
Rename to VM_ACLCheckCommandPermissionsForCurrentUser
rjd15372 Oct 7, 2025
cfc47a6
fix formatting
rjd15372 Oct 8, 2025
0273be0
change logic to error out when no user is in context
rjd15372 Oct 8, 2025
ac071e6
error improvement
rjd15372 Oct 8, 2025
f65208b
fix wrong include
rjd15372 Oct 8, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
35 changes: 35 additions & 0 deletions src/module.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9993,6 +9993,40 @@ 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.
* * 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) {
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
* of the ctx->client->user. */
if (ctx->user) {
return VM_ACLCheckCommandPermissions((ValkeyModuleUser *)ctx->user, argv, argc);
}

ValkeyModuleUser user = {
.user = ctx->client->user,
.free_user = 0,
};
Comment on lines +10022 to +10025
Copy link
Member

@madolson madolson Oct 8, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This entire API is because we're trying to bypass the allocation and do it on the stack here. When the API was written, we cared more about abstraction then performance. If we think performance is so critical, I would rather just ditch the abstraction and directly return the user from the module APIs.

Glancing through the code I don't see any incompatibility with that.

Copy link
Member Author

@rjd15372 rjd15372 Oct 9, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Are you proposing to make ValkeyModuleUser a type alias to user?

In that case we would probably need to add a field in the user struct to distinguish between users created by modules, and users created in the core.

Copy link
Member

@madolson madolson Oct 10, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In that case we would probably need to add a field in the user struct to distinguish between users created by modules, and users created in the core.

Yes. We don't expect a large number of users, so adding an extra field should be safe. I think we even have a bit field we could use if we wanted to avoid adding any memory at all.


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
Expand Down Expand Up @@ -14330,6 +14364,7 @@ void moduleRegisterCoreAPI(void) {
REGISTER_API(GetCurrentUserName);
REGISTER_API(GetModuleUserFromUserName);
REGISTER_API(ACLCheckCommandPermissions);
REGISTER_API(ACLCheckCommandPermissionsForCurrentUser);
REGISTER_API(ACLCheckKeyPermissions);
REGISTER_API(ACLCheckChannelPermissions);
REGISTER_API(ACLAddLogEntry);
Expand Down
4 changes: 4 additions & 0 deletions src/valkeymodule.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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_ACLCheckCommandPermissionsForCurrentUser)(ValkeyModuleCtx *ctx,
ValkeyModuleString **argv,
int argc) VALKEYMODULE_ATTR;
VALKEYMODULE_API int (*ValkeyModule_ACLCheckKeyPermissions)(ValkeyModuleUser *user,
ValkeyModuleString *key,
int flags) VALKEYMODULE_ATTR;
Expand Down Expand Up @@ -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(ACLCheckCommandPermissionsForCurrentUser);
VALKEYMODULE_GET_API(ACLCheckKeyPermissions);
VALKEYMODULE_GET_API(ACLCheckChannelPermissions);
VALKEYMODULE_GET_API(ACLAddLogEntry);
Expand Down
58 changes: 39 additions & 19 deletions tests/modules/aclcheck.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -82,17 +82,21 @@ 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_ACLCheckCommandPermissionsForCurrentUser(ctx, argv + 1, argc - 1);
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;
}

Expand All @@ -109,15 +113,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) {
Expand Down Expand Up @@ -199,14 +219,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. */
Expand All @@ -224,7 +244,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*/
Expand Down Expand Up @@ -274,7 +294,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)
Expand All @@ -292,25 +312,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;
}
Loading