-
Notifications
You must be signed in to change notification settings - Fork 1.1k
feat(AclFamily): add AUTH for acl members #1732
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
Merged
Merged
Changes from all commits
Commits
Show all changes
16 commits
Select commit
Hold shift + click to select a range
f6d8d1e
feat(AclFamily): add acl setuser command
kostasrim f4dbe93
chore: add tests, add overloaded header, improve implementation
kostasrim d37290a
fix: clang bug on Overloaded
kostasrim 0cf215e
Merge branch 'main' into acl_part_4_add_acl_set_user
kostasrim 20412b9
fix: revert accidental removal of a line in acl_family_test
kostasrim 9d3b767
chore: address gh comments
kostasrim cc2432d
Merge branch 'main' into acl_part_4_add_acl_set_user
kostasrim c0356d8
chore: use StartsWith instead of string comparison
kostasrim 2251f8e
chore: replace `starts_with` member function with `absl::StartsWith`
kostasrim ff6e3f0
feat(AclFamily): add AUTH for acl members
kostasrim 16aa85f
Merge branch 'main' into acl_part_5_add_acl_auth
kostasrim d98d270
chore: address gh comments
kostasrim d84ff8f
Merge branch 'main' into acl_part_5_add_acl_auth
kostasrim b4a9163
chore: apply gh comments
kostasrim cdfdb29
Merge branch 'main' into acl_part_5_add_acl_auth
kostasrim dba320c
fix broken test
kostasrim File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
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
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
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
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
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
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
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
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.
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.
This is a bit silly, but do we want to check no category appears both as a minus and as a plus?
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.
This is from the previous PR. What do you mean
no
caregory? You meanNONE
?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.
like what happens if I do
ACL SETUSER +@JSON -@JSON
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.
So I thought about this before, and for this specific case there won't be an issue because the ADD set is applied Before the REMOVE set and I thought that this redundancy is fine. However, now I am thinking about it, for the reverse case this is not true, that is
-@JSON +@JSON
won't do what is expected and therefore I think we should only allow a group once perSET USER
command (and probably this will require some special handling for some categories, example is-@JSON +@ALL
).Another solution is to apply them one by one in order which is slightly slower (since I can apply them all at once) but I don't expect it will affect the performance and IMO I think this is a better solution since it covers the ordering and we can allow
+@JSON -@JSON
and vice versa.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 will address this in a separate PR since this is about the
AUTH
commandUh oh!
There was an error while loading. Please reload this page.
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.
Wait... What operations are permitted? If the string is SSO, then any operations on the map (rehashing, shrinking, etc) will invalidate the string_view 🤨
PS: I wanted to put this comment below the
std::optional<std::string_view> authed_username;
discussion 😬