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 all 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
9 changes: 5 additions & 4 deletions examples/acl.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
use redis_module::{
redis_module, AclPermissions, Context, NextArg, RedisError, RedisResult, RedisString,
RedisValue,
redis_module, AclCategory, AclPermissions, Context, NextArg, RedisError, RedisResult,
RedisString, RedisValue,
};

fn verify_key_access_for_user(ctx: &Context, args: Vec<RedisString>) -> RedisResult {
Expand All @@ -25,8 +25,9 @@ redis_module! {
version: 1,
allocator: (redis_module::alloc::RedisAlloc, redis_module::alloc::RedisAlloc),
data_types: [],
acl_categories: [AclCategory::from("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, AclCategory::Read, AclCategory::from("acl")],
["get_current_user", get_current_user, "", 0, 0, 0, vec![AclCategory::Read, AclCategory::Fast], AclCategory::from("acl")],
],
}
107 changes: 107 additions & 0 deletions src/context/mod.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -926,6 +926,113 @@ bitflags! {
}
}

#[derive(Debug, Clone, PartialEq, Eq, Default)]
pub enum AclCategory {
#[default]
None,
Keyspace,
Read,
Write,
Set,
SortedSet,
List,
Hash,
String,
Bitmap,
HyperLogLog,
Geo,
Stream,
PubSub,
Admin,
Fast,
Slow,
Blocking,
Dangerous,
Connection,
Transaction,
Scripting,
Single(String),
Multi(Vec<AclCategory>),
}

impl From<Vec<AclCategory>> for AclCategory {
fn from(value: Vec<AclCategory>) -> Self {
AclCategory::Multi(value)
}
}

impl From<&str> for AclCategory {
fn from(value: &str) -> Self {
match value {
"" => AclCategory::None,
Copy link
Collaborator

Choose a reason for hiding this comment

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

What should the result be in the case of read or read ? Is it even possible?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

error?
passing that to Redis will be rejected

"keyspace" => AclCategory::Keyspace,
"read" => AclCategory::Read,
"write" => AclCategory::Write,
"set" => AclCategory::Set,
"sortedset" => AclCategory::SortedSet,
"list" => AclCategory::List,
"hash" => AclCategory::Hash,
"string" => AclCategory::String,
"bitmap" => AclCategory::Bitmap,
"hyperloglog" => AclCategory::HyperLogLog,
"geo" => AclCategory::Geo,
"stream" => AclCategory::Stream,
"pubsub" => AclCategory::PubSub,
"admin" => AclCategory::Admin,
"fast" => AclCategory::Fast,
"slow" => AclCategory::Slow,
"blocking" => AclCategory::Blocking,
"dangerous" => AclCategory::Dangerous,
"connection" => AclCategory::Connection,
"transaction" => AclCategory::Transaction,
"scripting" => AclCategory::Scripting,
_ if !value.contains(" ") => AclCategory::Single(value.to_string()),
_ => AclCategory::Multi(value.split_whitespace().map(AclCategory::from).collect()),
}
}
}

impl From<AclCategory> for String {
fn from(value: AclCategory) -> Self {
match value {
AclCategory::None => "".to_string(),
AclCategory::Keyspace => "keyspace".to_string(),
AclCategory::Read => "read".to_string(),
AclCategory::Write => "write".to_string(),
AclCategory::Set => "set".to_string(),
AclCategory::SortedSet => "sortedset".to_string(),
AclCategory::List => "list".to_string(),
AclCategory::Hash => "hash".to_string(),
AclCategory::String => "string".to_string(),
AclCategory::Bitmap => "bitmap".to_string(),
AclCategory::HyperLogLog => "hyperloglog".to_string(),
AclCategory::Geo => "geo".to_string(),
AclCategory::Stream => "stream".to_string(),
AclCategory::PubSub => "pubsub".to_string(),
AclCategory::Admin => "admin".to_string(),
AclCategory::Fast => "fast".to_string(),
AclCategory::Slow => "slow".to_string(),
AclCategory::Blocking => "blocking".to_string(),
AclCategory::Dangerous => "dangerous".to_string(),
AclCategory::Connection => "connection".to_string(),
AclCategory::Transaction => "transaction".to_string(),
AclCategory::Scripting => "scripting".to_string(),
AclCategory::Single(s) => s,
AclCategory::Multi(v) => v
.into_iter()
.map(String::from)
.collect::<Vec<_>>()
.join(" "),
}
}
}

impl std::fmt::Display for AclCategory {
fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result {
write!(f, "{}", String::from(self.clone()))
}
}

/// The values allowed in the "info" sections and dictionaries.
#[derive(Debug, Clone)]
pub enum InfoContextBuilderFieldBottomLevelValue {
Expand Down
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
1 change: 1 addition & 0 deletions src/lib.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -33,6 +33,7 @@ pub use crate::context::commands;
pub use crate::context::defrag;
pub use crate::context::keys_cursor::KeysCursor;
pub use crate::context::server_events;
pub use crate::context::AclCategory;
pub use crate::context::AclPermissions;
#[cfg(any(
feature = "min-redis-compatibility-version-7-4",
Expand Down
95 changes: 72 additions & 23 deletions src/macros.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,11 @@ 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.

use redis_module::AclCategory;

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

Expand Down Expand Up @@ -37,34 +41,67 @@ 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;
}
let mandatory_acl_categories = AclCategory::from($mandatory_acl_categories);
if let Some(RM_SetCommandACLCategories) = $crate::raw::RedisModule_SetCommandACLCategories {
let mut acl_categories = CString::default();
$(
let optional_acl_categories = AclCategory::from($optional_acl_categories);
if mandatory_acl_categories != AclCategory::None && optional_acl_categories != AclCategory::None {
acl_categories = CString::new(format!("{} {}", mandatory_acl_categories, optional_acl_categories)).unwrap();
} else if optional_acl_categories != AclCategory::None {
Copy link
Collaborator

Choose a reason for hiding this comment

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

I'd personally add a method for this AclCategory::is_none() and is_some(). It may clash with the interface of Option and confuse the reader, though, but there is no better alternative.

acl_categories = CString::new(format!("{}", $optional_acl_categories)).unwrap();
Copy link
Collaborator

Choose a reason for hiding this comment

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

Can we just $optional_acl_categories.to_string()?

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 needs to be a c_string to pass to Redis

Copy link
Collaborator

Choose a reason for hiding this comment

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

I know, I meant instead of using the format! macro we could have simply used the to_string() method of the fmt::Display trait, no?

}
// 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 != AclCategory::None {
acl_categories = CString::new(format!("{}", 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 != AclCategory::None {
$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 +171,11 @@ 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_categories: [
$($module_acl_category:expr,)*
],)?
$(init: $init_func:ident,)* $(,)*
$(deinit: $deinit_func:ident,)* $(,)*
$(info: $info_func:ident,)?
Expand All @@ -146,7 +187,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 +313,24 @@ 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));
return raw::Status::Err as c_int;
$(
if let Some(RM_AddACLCategory) = raw::RedisModule_AddACLCategory {
let module_acl_category = AclCategory::from($module_acl_category);
if module_acl_category != AclCategory::None {
let category = CString::new(format!("{}", $module_acl_category)).unwrap();
if RM_AddACLCategory(ctx, category.as_ptr()) == raw::Status::Err as c_int {
raw::redis_log(ctx, &format!("Error: failed to add ACL category `{}`", $module_acl_category));
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
25 changes: 25 additions & 0 deletions tests/integration.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -275,6 +275,31 @@ fn test_get_current_user() -> Result<()> {
Ok(())
}

#[test]
#[cfg(feature = "min-redis-compatibility-version-7-4")]
fn test_set_acl_categories() -> Result<()> {
let mut con = TestConnection::new("acl");

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 mut con = TestConnection::new("acl");

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 mut con = TestConnection::new("acl");
Expand Down
Loading