Allow FT._debug to use when client is admin by bandalgomsu · Pull Request #914 · valkey-io/valkey-search

bandalgomsu · 2026-03-17T13:52:58Z

Implement an admin check function and, through it, allow the use of FT._debug if the user is an admin.

issue : #779

KarthikSubbarao · 2026-03-19T18:38:58Z

src/commands/ft_debug.cc

@@ -347,7 +348,7 @@ absl::Status HelpCmd(ValkeyModuleCtx *ctx, vmsdk::ArgsIterator &itr) {
 absl::Status FTDebugCmd(ValkeyModuleCtx *ctx, ValkeyModuleString **argv,


Can we add a comment here explaining when this command is allowed to be used?

KarthikSubbarao · 2026-03-19T18:42:53Z

src/acl.cc

+  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) {


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

Also, all of this can be skipped if we follow: #914 (comment)

KarthikSubbarao · 2026-03-19T18:48:48Z

src/commands/ft_debug.cc

                        int argc) {
  std::string msg;
-  if (!vmsdk::config::IsDebugModeEnabled()) {
+  if (!vmsdk::config::IsDebugModeEnabled() && !AclAdminCheck(ctx).ok()) {


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

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

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._DEBUG to ADMIN, DANGEROUS, and SLOW (Should Search be included as well ??)

delete the debug mode check logic.

set the ACL categories of FT._DEBUG to ADMIN, DANGEROUS, and SLOW

Yes

(Should Search be included as well ??)

Just FT._DEBUG

delete the debug mode check logic.

Yes

Oh sorry for confusion

'Search' mean that is acl category

https://github.com/valkey-io/valkey-search/blob/main/src/commands/ft._debug.json

It should be included in FT._DEBUG's acl categories ?

Oh Yes, the SEARCH category too

Signed-off-by: Su Ko <rhtn1128@gmail.com>

KarthikSubbarao · 2026-03-25T22:15:40Z

integration/test_debug.py

        assert bysize[-12] == {'Count': 10, 'Bytes': 120, 'AvgSize': b'12', 'Allocated': 280, 'AvgAllocated': b'28', 'Utilization': 42}
        assert bysize[14] == {'Count': 10, 'Bytes': 140, 'AvgSize': b'14', 'Allocated': 320, 'AvgAllocated': b'32', 'Utilization': 43}
+
+    def test_cannot_run_ft_debug_when_debug_mode_enabled(self):


We can rename the test now.

Also, do we have a good way to test admin vs non admin client usage from the integration tests (non admin clients should not be able to access this command)

How about switching the inheritance from ValkeySearchTestCaseDebugMode to ValkeySearchTestCaseBase and adding a test similar to test_acl_category_permissions ? ?

KarthikSubbarao · 2026-03-25T22:15:45Z

integration/test_debug.py

+        with pytest.raises(ResponseError):
+            restricted_client.execute_command("FT._DEBUG", "HELP")
+
+        self.client.execute_command("ACL", "DELUSER", "debug_user")


Missing new line

KarthikSubbarao · 2026-03-25T22:21:10Z

src/commands/commands.h

@@ -55,7 +56,7 @@ const absl::flat_hash_set<absl::string_view> kInfoCmdPermissions{
 const absl::flat_hash_set<absl::string_view> kListCmdPermissions{
    kSearchCategory, kReadCategory, kSlowCategory, kAdminCategory};
 const absl::flat_hash_set<absl::string_view> kDebugCmdPermissions{


Valkey commands have both ACl flags and Command flags.

You updated the ACL flags.... This looks good.

The command flags are here: https://github.com/valkey-io/valkey-search/blob/main/src/module_loader.cc#L88. We need to update this next

Refer to the core Valkey DEBUG cmd flags here: https://github.com/valkey-io/valkey/blob/unstable/src/commands/debug.json#L12

PROTECTED command flag is only using a core command (only DEBUG, MODULE).
and It is not supported as a module command flag in ValkeyModule_CreateCommand, so we cannot set it for FT._DEBUG and did not add it.

https://github.com/valkey-io/valkey/blob/unstable/src/server.c#L4292
https://github.com/valkey-io/valkey/blob/unstable/src/module.c#L1243C1-L1243C42

Signed-off-by: Su Ko <rhtn1128@gmail.com>

bandalgomsu force-pushed the gh-779 branch 3 times, most recently from e3289c8 to 9de16d5 Compare March 17, 2026 14:10

KarthikSubbarao requested changes Mar 19, 2026

View reviewed changes

bandalgomsu force-pushed the gh-779 branch from 9de16d5 to b7488b8 Compare March 25, 2026 10:39

Allow FT._debug to use when client is admin

7a9c4bf

Signed-off-by: Su Ko <rhtn1128@gmail.com>

bandalgomsu force-pushed the gh-779 branch from b7488b8 to 7a9c4bf Compare March 25, 2026 10:41

KarthikSubbarao reviewed Mar 25, 2026

View reviewed changes

bandalgomsu added 2 commits March 26, 2026 19:18

Add new line in test_dubug.py

e10eb11

Signed-off-by: Su Ko <rhtn1128@gmail.com>

Add FT._DEBUG's command_flags

01654cd

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,

Conversation

bandalgomsu commented Mar 17, 2026

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

KarthikSubbarao Mar 19, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

KarthikSubbarao Mar 25, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

KarthikSubbarao Mar 19, 2026 •

edited

Loading

KarthikSubbarao Mar 25, 2026 •

edited

Loading