Skip to content
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

Jump to bottom

MOD-8036 Add tests for 393 and 394 #395

Merged
merged 9 commits into from
Nov 4, 2024
Merged

MOD-8036 Add tests for 393 and 394 #395

Show file tree
Hide file tree
Changes from 6 commits
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
a85c1c4
add test
ephraimfeldblum Nov 3, 2024
e234658
add comment
ephraimfeldblum Nov 3, 2024
cdb4965
comments from Meir
ephraimfeldblum Nov 3, 2024
a2451d1
add else
ephraimfeldblum Nov 3, 2024
3388925
tests
ephraimfeldblum Nov 3, 2024
a6e4203
formatting
ephraimfeldblum Nov 4, 2024
0f33019
implement `enum AclCategory`
ephraimfeldblum Nov 4, 2024
56cf136
Merge branch 'master' into ephraim_add-test-to-ACL
ephraimfeldblum Nov 4, 2024
fc51c7e
using `TestConnection` API
ephraimfeldblum Nov 4, 2024
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
5 changes: 3 additions & 2 deletions examples/acl.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,8 +25,9 @@ redis_module! {
version: 1,
allocator: (redis_module::alloc::RedisAlloc, redis_module::alloc::RedisAlloc),
data_types: [],
acl_category: "acl",
commands: [
["verify_key_access_for_user", verify_key_access_for_user, "", 0, 0, 0, ""],
["get_current_user", get_current_user, "", 0, 0, 0, ""],
["verify_key_access_for_user", verify_key_access_for_user, "", 0, 0, 0, "read", "acl"],
["get_current_user", get_current_user, "", 0, 0, 0, "read", "acl"],
],
}
1 change: 1 addition & 0 deletions src/include/redismodule.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1326,6 +1326,7 @@ static void RedisModule_InitAPI(RedisModuleCtx *ctx) {
REDISMODULE_GET_API(CreateSubcommand);
REDISMODULE_GET_API(SetCommandInfo);
REDISMODULE_GET_API(SetCommandACLCategories);
REDISMODULE_GET_API(AddACLCategory);
REDISMODULE_GET_API(SetModuleAttribs);
REDISMODULE_GET_API(IsModuleNameBusy);
REDISMODULE_GET_API(WrongArity);
Expand Down
76 changes: 57 additions & 19 deletions src/macros.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,9 @@ macro_rules! redis_command {
$firstkey:expr,
$lastkey:expr,
$keystep:expr,
$acl_categories:expr) => {{
$mandatory_acl_categories:expr
$(, $optional_acl_categories:expr)?
) => {{
Comment on lines +10 to +12
Copy link
Collaborator

Choose a reason for hiding this comment

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

For the simplicity, I'd do two things:

  1. Add a separate enum: CommandAclCategory:
enum CommandAclCategory {
    Write,
    Read,
    Fast,
    Other(String),
}

impl std::fmt::Display for CommandAclCategory {
    // ...
}

Where we would add all the already existing, commonly used acl categories which have already existed for a long time, and would always specifying a new one by having the Other variant.

  1. If the mandatory ACL category is mandatory, why not simply omit it and add it automatically? We are not in C, so we can have two macros, with it and without. In the "without" macro branch, I'd invoke the "with" branch with the mandatory ACL category name as the module name (this is the case for all the modules we are currently adding the ACLs anyway).
  2. Rename the acl_categories arguments, as we specify them separately, not all within one argument: mandatory_acl_category and optional_acl_category.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

  1. doable. will take under advisement.
  2. there is no one singular mandatory ACL category. the mandatory ACL categories are those that are mandatory for the module to work at all, and loading the module should fail if they are not added. optional ACL categories are those that we warn on failure to add, but do not fail.
  3. we do specify multiple categories within one argument. as a space delimited string.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Thanks for the response!

How we want to specify them for redis - this is the implementation detail. The interface here is in Rust, and we should allow the safest, most idiomatic way possible, which is an explicit set of type-safe values. Do you agree? For the user of this macro it shouldn't matter how exactly those end up being sent to the server, the user shouldn't care about this, nor should he know about this, if this is hidden behind the interface. This is not why this crate is simply forwarding the rust types to the C-api, right?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

it's the user that decides which categories are mandatory and which are optional, not us.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Yes, okay, that I am not discussing anymore. I am talking now about the type safety and the explicit lists.

let name = CString::new($command_name).unwrap();
let flags = CString::new($command_flags).unwrap();

Expand Down Expand Up @@ -37,34 +39,65 @@ macro_rules! redis_command {
)
} == $crate::raw::Status::Err as c_int
{
$crate::raw::redis_log(
$ctx,
&format!("Error: failed to create command {}", $command_name),
);
return $crate::raw::Status::Err as c_int;
}

if $acl_categories != "" {
let acl_categories = CString::new($acl_categories).unwrap();
let command =
unsafe { $crate::raw::RedisModule_GetCommand.unwrap()($ctx, name.as_ptr()) };
if command.is_null() {
$crate::raw::redis_log(
$ctx,
&format!("Error: failed to get command {}", $command_name),
);
return $crate::raw::Status::Err as c_int;
}

let command =
unsafe { $crate::raw::RedisModule_GetCommand.unwrap()($ctx, name.as_ptr()) };
if command.is_null() {
return $crate::raw::Status::Err as c_int;
}
if let Some(RM_SetCommandACLCategories) = $crate::raw::RedisModule_SetCommandACLCategories {
let mut acl_categories = CString::new("").unwrap();
ephraimfeldblum marked this conversation as resolved.
Show resolved Hide resolved
$(
if $mandatory_acl_categories != "" && $optional_acl_categories != "" {
acl_categories = CString::new(format!("{} {}", $mandatory_acl_categories, $optional_acl_categories)).unwrap();
} else if $optional_acl_categories != "" {
acl_categories = CString::new($optional_acl_categories).unwrap();
}
ephraimfeldblum marked this conversation as resolved.
Show resolved Hide resolved
// Warn if optional ACL categories are not set, but don't fail.
if RM_SetCommandACLCategories(command, acl_categories.as_ptr()) == $crate::raw::Status::Err as c_int {
$crate::raw::redis_log(
$ctx,
&format!(
"Warning: failed to set command `{}` ACL categories `{}`",
$command_name, acl_categories.to_str().unwrap()
),
);
} else
)?
if $mandatory_acl_categories != "" {
acl_categories = CString::new($mandatory_acl_categories).unwrap();

if let Some(RM_SetCommandACLCategories) =
$crate::raw::RedisModule_SetCommandACLCategories
{
// Fail if mandatory ACL categories are not set.
if RM_SetCommandACLCategories(command, acl_categories.as_ptr())
== $crate::raw::Status::Err as c_int
{
$crate::raw::redis_log(
$ctx,
&format!(
"Error: failed to set command {} ACL categories {}",
$command_name, $acl_categories
"Error: failed to set command `{}` mandatory ACL categories `{}`",
iddm marked this conversation as resolved.
Show resolved Hide resolved
$command_name, $mandatory_acl_categories
),
);
return $crate::raw::Status::Err as c_int;
}
}
} else if $mandatory_acl_categories != "" {
$crate::raw::redis_log(
$ctx,
"Error: Redis version does not support ACL categories",
);
return $crate::raw::Status::Err as c_int;
}
}};
}
Expand Down Expand Up @@ -134,7 +167,9 @@ macro_rules! redis_module {
data_types: [
$($data_type:ident),* $(,)*
],
$(acl_category: $acl_category:expr,)* $(,)*
// eg: `acl_category: "name_of_module_acl_category",`
// This will add the specified (optional) ACL categories.
$(acl_category: $module_acl_categories:expr,)* $(,)*
$(init: $init_func:ident,)* $(,)*
$(deinit: $deinit_func:ident,)* $(,)*
$(info: $info_func:ident,)?
Expand All @@ -146,7 +181,8 @@ macro_rules! redis_module {
$firstkey:expr,
$lastkey:expr,
$keystep:expr,
$acl_categories:expr
$mandatory_command_acl_categories:expr
Copy link
Collaborator

Choose a reason for hiding this comment

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

Again, this is plural but represents just one category; IIRC, there is just one mandatory category now, or am I wrong? Either way, I understand the precaution here of actually passing more than one as a string separated with spaces, but I'd really like to have it more verbose and explicit by having the enum and a [,] expression instead then.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

there may be many mandatory categories. mandatory just means that if adding any of these categories fails, the module loading should fail.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Alright, then I'd like to have those not as string literals but as a comma-separated values list, which implements From<T> for CommandAclCategory enum either way, so either a string, or a CommandAclCategory enum variant itself.

$(, $optional_command_acl_categories:expr)?
]),* $(,)*
] $(,)*
$(event_handlers: [
Expand Down Expand Up @@ -271,17 +307,19 @@ macro_rules! redis_module {
)*

$(
let category = CString::new($acl_category).unwrap();
if let Some(RM_AddACLCategory) = raw::RedisModule_AddACLCategory {
if RM_AddACLCategory(ctx, category.as_ptr()) == raw::Status::Err as c_int {
raw::redis_log(ctx, &format!("Error: failed to add ACL category {}", $acl_category));
let categories = CString::new($module_acl_categories).unwrap();
if RM_AddACLCategory(ctx, categories.as_ptr()) == raw::Status::Err as c_int {
raw::redis_log(ctx, &format!("Error: failed to add ACL categories `{}`", $module_acl_categories));
return raw::Status::Err as c_int;
}
} else {
raw::redis_log(ctx, "Warning: Redis version does not support adding new ACL categories");
}
)*

$(
$crate::redis_command!(ctx, $name, $command, $flags, $firstkey, $lastkey, $keystep, $acl_categories);
$crate::redis_command!(ctx, $name, $command, $flags, $firstkey, $lastkey, $keystep, $mandatory_command_acl_categories $(, $optional_command_acl_categories)?);
)*

if $crate::commands::register_commands(&context) == raw::Status::Err {
Expand Down
2 changes: 1 addition & 1 deletion test.sh
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,2 +1,2 @@
#!/usr/bin/env sh
cargo test --all --all-targets --no-default-features
cargo test --all --all-targets --no-default-features --features min-redis-compatibility-version-7-4
33 changes: 33 additions & 0 deletions tests/integration.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -329,6 +329,39 @@ fn test_get_current_user() -> Result<()> {
Ok(())
}

#[test]
#[cfg(feature = "min-redis-compatibility-version-7-4")]
fn test_set_acl_categories() -> Result<()> {
let port: u16 = 6490;
let _guards = vec![start_redis_server_with_module("acl", port)
.with_context(|| "failed to start redis server")?];
let mut con =
get_redis_connection(port).with_context(|| "failed to connect to redis server")?;

let res: Vec<String> = redis::cmd("ACL").arg("CAT").query(&mut con)?;
assert!(res.contains(&"acl".to_owned()));

Ok(())
}

#[test]
#[cfg(feature = "min-redis-compatibility-version-8-0")]
fn test_set_acl_categories_commands() -> Result<()> {
let port: u16 = 6490;
let _guards = vec![start_redis_server_with_module("acl", port)
.with_context(|| "failed to start redis server")?];
let mut con =
get_redis_connection(port).with_context(|| "failed to connect to redis server")?;
ephraimfeldblum marked this conversation as resolved.
Show resolved Hide resolved

let res: Vec<String> = redis::cmd("ACL").arg("CAT").arg("acl").query(&mut con)?;
assert!(
res.contains(&"verify_key_access_for_user".to_owned())
&& res.contains(&"get_current_user".to_owned())
);

Ok(())
}

#[test]
fn test_verify_acl_on_user() -> Result<()> {
let port: u16 = 6491;
Expand Down
Loading