Skip to content

Conversation

@sachinvmurthy
Copy link
Contributor

This PR introduces a new hook to the macro.rs, allowing module devs to register callbacks that are invoked after authentication completes. The hook provides both the previous and new username, enabling post-auth logic such as auditing, logging, or user state management.

feature = "min-redis-compatibility-version-7-2",
feature = "min-valkey-compatibility-version-8-0"
)))]
compile_error!("Post-auth callbacks require Redis 7.2 or Valkey 8.0 and above");
Copy link
Collaborator

Choose a reason for hiding this comment

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

wouldn't this work on Valkey 7.2 as well?

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 would, i'll update it

feature = "min-valkey-compatibility-version-8-0"
))]
unsafe {
$crate::raw::RedisModule_RegisterAuthCallback.expect("RedisModule_RegisterAuthCallback should exist on Redis/Valkey 7.0 and above")($ctx, Some([<__do_post_auth_ $module_name _ $post_auth_callback>]))
Copy link
Collaborator

Choose a reason for hiding this comment

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

shouldn't this be Redis/Valkey 7.2?

) -> std::os::raw::c_int {
let context = $crate::Context::new(ctx);
let ctx_ptr = unsafe { std::ptr::NonNull::new_unchecked(ctx) };
let username_rs = $crate::ValkeyString::new(Some(ctx_ptr), username);
Copy link
Collaborator

Choose a reason for hiding this comment

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

what does username_rs stand for?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

username_"rust" since Im converting from the unsafe C pointer

@dmitrypol
Copy link
Collaborator

interesting idea, let's talk more next week

@@ -0,0 +1,34 @@
use std::sync::{LazyLock, Mutex};
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 add integration test for this?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I have added examples, not sure how I can do an integ test for this.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants