Add ACL role support#3967
Open
yang-z-o wants to merge 4 commits into
Open
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughThis PR implements first-class ACL roles in Valkey by introducing named, reusable permission sets that users can be assigned to. The implementation adds role data structures, role-aware user lifecycle management, role-to-user binding, authorization updates, config/ACL file persistence, new role administration commands, and comprehensive test coverage. ChangesACL Roles Feature
Sequence Diagram(s)sequenceDiagram
participant User as User (with roles)
participant ACLCheck as ACLCheckAllUserCommandPerm
participant RoleSelector as Role Selectors
participant UserSelector as User Selectors
participant Result as Allow/Deny
User->>ACLCheck: Check command permission
ACLCheck->>UserSelector: Evaluate user's own selectors
UserSelector-->>ACLCheck: Allowed/Denied
ACLCheck->>RoleSelector: For each assigned role, evaluate selectors
RoleSelector-->>ACLCheck: Allowed/Denied
ACLCheck->>Result: OR all results (any allow = allowed)
Result-->>User: Permission decision
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Suggested reviewers
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/commands.def (1)
7146-7152:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winAdd a
10.0.0history entry for the newACL SETUSERrole syntax.
ACL_SETUSER_Historystill stops at9.1.0, but this PR adds+@role:<name>/-@role:<name>handling. That leaves generated metadata andCOMMAND DOCSwithout any record of the new public syntax.Based on PR objectives: role assignment/removal is exposed through
ACL SETUSERusing+@role:<name>/-@role:<name>.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/commands.def` around lines 7146 - 7152, ACL_SETUSER_History is missing the new 10.0.0 entry for the role syntax change; add a new history entry to the ACL_SETUSER_History array describing the addition of "+@role:<name> / -@role:<name>" role assignment/removal syntax (use the exact symbol ACL_SETUSER_History and append a {"10.0.0","Added role assignment/removal via +@role:<name> and -@role:<name>."} entry so generated metadata and COMMAND DOCS include the new public syntax).
🧹 Nitpick comments (2)
src/acl.c (2)
2863-2892: 💤 Low valueConsider adding rule validation for consistency with user loading.
Unlike
ACLAppendUserForLoadingwhich validates rules against a fake user (tolerating unknown commands/roles), this function only validates selector parenthesis matching viaACLMergeSelectorArgumentsbut doesn't validate the actual rules.While invalid rules will still be caught during
ACLLoadConfiguredRolesat startup, validating here would:
- Provide earlier, more precise error reporting (line number from config parsing)
- Be consistent with user loading behavior
This is a minor inconsistency since both approaches ultimately prevent startup with invalid ACLs.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/acl.c` around lines 2863 - 2892, ACLAppendRoleForLoading should validate the merged rule arguments the same way ACLAppendUserForLoading does (i.e., by applying the rules to a temporary/fake ACL user) instead of only relying on ACLMergeSelectorArguments; after acl_args is produced, create a temporary user object and run the same rule-validation path used by ACLAppendUserForLoading/ACLLoadConfiguredRoles to detect invalid commands/roles and parenthesis errors, set argc_err to invalid_idx+2 (or the appropriate argument index returned by the validator) and return C_ERR on failure, freeing acl_args before returning; on success continue to copy/store the validated acl_args as currently done.
729-735: 💤 Low valueMinor: Unreachable code path with confusing error message.
The check at line 731 can only fail if a role with the same name was created between line 703 (where we checked
!r) and line 730. In single-threaded command processing, this race condition cannot occur. The error message "Role already exists" is also misleading since the function already determined the role doesn't exist.This is defensive code and harmless, but consider removing it or updating the message if kept for robustness against future changes.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/acl.c` around lines 729 - 735, The defensive check after ACLCreateRole is confusing and unreachable in current single-threaded processing; either remove the entire if (!r) { r = ACLCreateRole(rolename, sdslen(rolename)); if (!r) { error = sdsnew("Role already exists"); goto cleanup; } } block, or if you want to keep a defensive fallback, change the sdsnew message to a generic failure like "Failed to create role" or "ACLCreateRole returned NULL" and ensure the code still jumps to cleanup; reference ACLCreateRole, r, rolename, sdsnew, error and cleanup when making the change.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/commands.def`:
- Around line 7215-7216: Update the summary strings for the getrole and getuser
command entries so they mention role-related fields: modify the MAKE_CMD
invocation for "getrole" (the entry using ACL_GETROLE_History, ACL_GETROLE_Tips,
ACL_GETROLE_Args) to say it returns role selectors and members in addition to
ACL rules, and modify the MAKE_CMD invocation for "getuser" (the entry using
ACL_GETUSER_History, ACL_GETUSER_Tips, ACL_GETUSER_Args) to indicate the
response includes a roles field (in addition to listing
rules/passwords/patterns); keep the rest of each command entry unchanged.
In `@src/config.c`:
- Line 580: The error message for duplicate roles is misleading because
ACLSetStringError() returns a user-specific string when errno==EALREADY; update
handling so duplicate-role errors produce a generic or role-aware message.
Either modify ACLSetStringError() (used by ACLAppendRoleForLoading and others)
to return a neutral wording for EALREADY like "Duplicate ACL definition found.
Each user or role can only be defined once in config files", or refactor callers
(e.g., where ACLAppendRoleForLoading invokes ACLSetStringError() in config.c) to
pass contextual information so the error text distinguishes roles from users;
implement the simpler fix by changing the EALREADY branch in ACLSetStringError()
to the generic message.
- Around line 577-584: Add "role" to the allowlist used by
rewriteConfigReadOldFile/lookupConfig so lines starting with "role" are not
commented out during CONFIG REWRITE (preventing data loss), and fix the
duplicate-role error text by ensuring role-specific errors are produced: either
have ACLAppendRoleForLoading set a role-specific errno/message or update
ACLSetStringError to return "Duplicate role found..." when called after
ACLAppendRoleForLoading returns EALREADY; reference the functions
rewriteConfigReadOldFile, lookupConfig (allowlist), ACLAppendRoleForLoading and
ACLSetStringError to locate and apply these changes.
---
Outside diff comments:
In `@src/commands.def`:
- Around line 7146-7152: ACL_SETUSER_History is missing the new 10.0.0 entry for
the role syntax change; add a new history entry to the ACL_SETUSER_History array
describing the addition of "+@role:<name> / -@role:<name>" role
assignment/removal syntax (use the exact symbol ACL_SETUSER_History and append a
{"10.0.0","Added role assignment/removal via +@role:<name> and -@role:<name>."}
entry so generated metadata and COMMAND DOCS include the new public syntax).
---
Nitpick comments:
In `@src/acl.c`:
- Around line 2863-2892: ACLAppendRoleForLoading should validate the merged rule
arguments the same way ACLAppendUserForLoading does (i.e., by applying the rules
to a temporary/fake ACL user) instead of only relying on
ACLMergeSelectorArguments; after acl_args is produced, create a temporary user
object and run the same rule-validation path used by
ACLAppendUserForLoading/ACLLoadConfiguredRoles to detect invalid commands/roles
and parenthesis errors, set argc_err to invalid_idx+2 (or the appropriate
argument index returned by the validator) and return C_ERR on failure, freeing
acl_args before returning; on success continue to copy/store the validated
acl_args as currently done.
- Around line 729-735: The defensive check after ACLCreateRole is confusing and
unreachable in current single-threaded processing; either remove the entire if
(!r) { r = ACLCreateRole(rolename, sdslen(rolename)); if (!r) { error =
sdsnew("Role already exists"); goto cleanup; } } block, or if you want to keep a
defensive fallback, change the sdsnew message to a generic failure like "Failed
to create role" or "ACLCreateRole returned NULL" and ensure the code still jumps
to cleanup; reference ACLCreateRole, r, rolename, sdsnew, error and cleanup when
making the change.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 0a1a1111-8d14-43ee-9833-a72088926703
📒 Files selected for processing (11)
src/acl.csrc/commands.defsrc/commands/acl-delrole.jsonsrc/commands/acl-getrole.jsonsrc/commands/acl-getuser.jsonsrc/commands/acl-roles.jsonsrc/commands/acl-setrole.jsonsrc/config.csrc/server.htests/assets/role.acltests/unit/acl-role.tcl
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## unstable #3967 +/- ##
============================================
+ Coverage 76.68% 76.76% +0.08%
============================================
Files 162 162
Lines 80753 81230 +477
============================================
+ Hits 61929 62360 +431
- Misses 18824 18870 +46
🚀 New features to boost your workflow:
|
roshkhatri
reviewed
Jun 11, 2026
Signed-off-by: Yang Zhao <zymy701@gmail.com>
Signed-off-by: Yang Zhao <zymy701@gmail.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Closes #3726
Implements ACL roles as described in the issue. A role is a named, reusable set of ACL selectors that can be assigned to multiple users. This allows operators to define permission policies once and apply them to many users, avoiding per-user rule duplication.
Core design
New commands
ACL SETROLE <name> <rules...>ACL DELROLE <name> [name ...]ACL GETROLE <name>ACL ROLESACL SETUSER <user> +@role:<name>ACL SETUSER <user> -@role:<name>ACL file and config support
rolekeyword or inline invalkey.conf.ACL SAVEwrites roles before users.ACL LISToutputs roles before users.Tests
1. ACL commands (runtime)
SETROLE,DELROLE,GETROLE,ROLES)+@role:<name>,-@role:<name>)ACL DRYRUNrespect role selectorsACL LISTincludes roles2. ACL file (
aclfileoption)ACL SAVEandACL LOADpreserve roles3. Inline directives in
valkey.confroleanduserdirectives loaded from main configBackwards compatibility
valkey.conffiles withoutroledirectives work as before.ACL GETUSERoutput adds a newrolesfield but all other fields remain unchanged.ACL LISTprepends role entries before user entries; existing user entry format is unchanged.