-
Notifications
You must be signed in to change notification settings - Fork 742
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.
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
Enhance ACL WHOAMI command to display active ACL rules for the current connection #1821
base: unstable
Are you sure you want to change the base?
Conversation
Signed-off-by: hwware <[email protected]>
a88175a
to
8b23382
Compare
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## unstable #1821 +/- ##
============================================
- Coverage 71.03% 70.98% -0.05%
============================================
Files 123 123
Lines 65665 65672 +7
============================================
- Hits 46645 46618 -27
- Misses 19020 19054 +34
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What's the use case for this ?
if (c->user != NULL) { | ||
addReplyBulkCBuffer(c, c->user->name, sdslen(c->user->name)); | ||
if (c->argc == 3) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I generally don't like variadic arguments that change the type of response. So would also somewhat prefer another command.
Also, do we want this to be a variant of ACL GETUSER
instead? Somewhat related to this issue, #1807.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Another command? you mean a new command? I am afraid the user need to remember one more.
I guess this command is not same as ACL GETUSER, because this command only apply to current connection, even user do not know the username either.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I guess this command is not same as ACL GETUSER, because this command only apply to current connection, even user do not know the username either.
A new command, like ACL GETMYSELF
, is what I'm suggesting. I agree it's one more command.
As the top comment describe, I thought |
I mean it's not an expensive operation and don't see what's the pain behind doing it one after another. |
Current ACL WHOAMI can only display the username for the current connection.
It need to call ACL LIST to get the active ACL rules, thus there are 2 disadvantages:
This PR allows the current connection to call once "ACL WHOAMI FULL" to get all information.