Allow FT._debug to use when client is admin#914
Allow FT._debug to use when client is admin#914bandalgomsu wants to merge 1 commit intovalkey-io:mainfrom
Conversation
bandalgomsu
force-pushed
the
gh-779
branch
2 times, most recently
from
0d96946 to
e3289c8
Compare
Signed-off-by: Su Ko <rhtn1128@gmail.com>
| @@ -347,7 +348,7 @@ absl::Status HelpCmd(ValkeyModuleCtx *ctx, vmsdk::ArgsIterator &itr) { | |||
| absl::Status FTDebugCmd(ValkeyModuleCtx *ctx, ValkeyModuleString **argv, | |||
There was a problem hiding this comment.
Can we add a comment here explaining when this command is allowed to be used?
| auto reply = vmsdk::UniquePtrValkeyCallReply( | ||
| ValkeyModule_Call(ctx, "ACL", "cs3", "GETUSER", username.get())); | ||
| VMSDK_ASSIGN_OR_RETURN(auto acl_views, GetAclViewFromCallReply(reply.get())); | ||
| for (const auto &acl_view : acl_views) { |
There was a problem hiding this comment.
Can we instead make use of this API?
ValkeyModule_GetModuleUserACLString
https://valkey.io/topics/modules-api-ref/#ValkeyModule_GetModuleUserACLString
We should not need to execute ACL commands since we have these Module APIs
There was a problem hiding this comment.
Also, all of this can be skipped if we follow: #914 (comment)
| int argc) { | ||
| std::string msg; | ||
| if (!vmsdk::config::IsDebugModeEnabled()) { | ||
| if (!vmsdk::config::IsDebugModeEnabled() && !AclAdminCheck(ctx).ok()) { |
There was a problem hiding this comment.
I can agree that this is a way to allow a non-breaking change. ie, we continue to allow the command to be used by all clients in DebugModeEnabled whether they are admin or not.
But I don't understand why the FT.DEBUG command was not flagged as "ADMIN" to begin with. @murphyjacob4 , do you have context on this?
The DEBUG command in the valkey core is flagged as admin-only. See: https://valkey.io/commands/debug/
Non-admin clients cannot use it.
If we flagged the FT.DEBUG command as "admin" in the command flags we use to register it, we would not need to add these custom checks.
If we don't want this to be a breaking change, we can continue with what you have in this PR
There was a problem hiding this comment.
Update: Checked with Jacob, the admin command flag was likely just missed when FT.DEBUG was created.
@bandalgomsu , so let's just add the admin and dangerous command flags to the FT.DEBUG command registration. Then, we can skip the custom code for ACL checks because the valkey core will handle it
There was a problem hiding this comment.
It looks more consistent and simple. thus I agree with that 👍
could you please confirm if my understanding is correct?
- set the ACL categories of
FT._DEBUGtoADMIN,DANGEROUS, andSLOW(ShouldSearchbe included as well ??) - delete the debug mode check logic.
2 participants
Implement an admin check function and, through it, allow the use of FT._debug if the user is an admin.
issue : #779