feat(AclFamily): add acl setuser command #1725

kostasrim · 2023-08-22T17:45:08Z

Add ACL SETUSER command
Add tests

kostasrim · 2023-08-23T05:55:44Z

src/core/overloaded.h

+// See LICENSE for licensing terms.
+//
+//
+


We have this but the implementation is slightly different because it adds an extra overloads.
Also I am not sure if we can have the licence here ?

Maybe you can also replace the overloaded in other files wit this include?

It's only used in dragonfly_connection.cc

template <class... Ts> struct Overloaded : Ts... { using Ts::operator()...; template <typename T, typename D> size_t operator()(const unique_ptr<T, D>& ptr) { return operator()(*ptr.get()); } };

So I can't really replace it because:

template in lambdas is C++20 and it wouldn't be wise to use auto instead of std::unique_ptr<T, D>

I could inherit from my Overload but that would be exactly of the same verbosity + we would need a new deduction rule for this new derived class (therefore literally exactly the same code)

There are some things in C++20 I really love and would be useful in certain cases -- who knows maybe we consider it at some point in the future :D

dranikpg · 2023-08-23T06:07:32Z

tests/dragonfly/acl_family_test.py

+def assert_any_of(assertion, result):
+    assert assertion == result[0] or assertion == result[1]


small nit: better have the function return bool and assert its condition so you can see the failed line

This is not a small nit but essential. I was very dissatisfied that it would not print the line and I wanted to comment on that. Thankfully you did first + I think your other comment using in is far more better so I opted in for that

dranikpg · 2023-08-23T06:08:29Z

tests/dragonfly/acl_family_test.py

+
+    await async_client.execute_command("ACL SETUSER kostas -@list -@admin")
+    result = await async_client.execute_command("ACL LIST")
+    assert_any_of("user kostas on nopass +@STRING", result)


actually, instead of the function you can do assert "user kostas..." in result

dranikpg · 2023-08-23T06:14:06Z

src/core/overloaded.h

+// See LICENSE for licensing terms.
+//
+//
+


Maybe you can also replace the overloaded in other files wit this include?

dranikpg · 2023-08-23T06:16:17Z

src/server/acl/acl_family.cc

+  auto error_case = [cntx](std::string&& error) { (*cntx)->SendError(error); };
+  auto update_case = [username, cntx](User::UpdateRequest&& req) {
+    ServerState::tlocal()->user_registry->MaybeAddAndUpdate(username, std::move(req));
+    (*cntx)->SendOk();
+  };
+
+  std::visit(Overloaded{error_case, update_case}, std::move(req));


Its unusual to see it being done this way though I like this direction more than how we generally do it

Often we just use a function like optional<Result> ParseXXXOrReply(). It seems ok for me because those functions are unlikely to be ever re-used in a modular way where replies are not desirable.

Small nit: We now have a ErrorReply type that can be used instead of a string

It seems ok for me because those functions are unlikely to be ever re-used in a modular way where replies are not desirable.

100% they are local to the scope

Small nit: We now have a ErrorReply type that can be used instead of a string

Lit lit lit 🔥

dranikpg · 2023-08-23T06:19:53Z

src/server/acl/acl_family.cc

+
+using OptCat = std::optional<uint32_t>;
+
+std::pair<OptCat, OptCat> MaybeParseAclCategory(std::string_view command) {


I would note somewhere that the first are added cats and the second removed cats (though can be inferred from the +/- signs below)

Actually, don't you think that returning a Cat + bool (add) would be much shorter and easier than working with the pair?

That's a very good suggestion -- I like this!

dranikpg · 2023-08-23T06:28:53Z

src/server/acl/acl_family.cc

+    maybe_update_acl(req.plus_acl_categories, acl_plus);
+    maybe_update_acl(req.minus_acl_categories, acl_minus);


🤯 🔫

So if you choose to use the optional<[cat, bool remove]> you can just

if (result) { auto* acl_field = result.second ? &req.minus_acl_categories : req.plus_acl_categories; *acl_field |= result.first; } else { return ErrorReply{absl::StrCat("Unrecognized paramter", command)}; }

If acl_field is an optional you can do *acl_field = acl_field -> value_or(0) | result.first instead of adding if's 🤓

This is too good 💎 💎

dranikpg

👍🏻

dranikpg · 2023-08-23T19:35:33Z

src/server/acl/acl_family.cc

+    return {res->second, true};
+  }
+
+  if (command[0] == '-' && command[1] == '@') {


You can really squash it into a single if statement 😅

StartsWith! Fixed.

kostasrim added 2 commits August 22, 2023 20:44

feat(AclFamily): add acl setuser command

f6d8d1e

chore: add tests, add overloaded header, improve implementation

f4dbe93

kostasrim marked this pull request as ready for review August 23, 2023 05:55

kostasrim commented Aug 23, 2023

View reviewed changes

kostasrim self-assigned this Aug 23, 2023

kostasrim requested review from dranikpg and romange August 23, 2023 05:55

kostasrim added 3 commits August 23, 2023 09:13

fix: clang bug on Overloaded

d37290a

Merge branch 'main' into acl_part_4_add_acl_set_user

0cf215e

fix: revert accidental removal of a line in acl_family_test

20412b9

dranikpg reviewed Aug 23, 2023

View reviewed changes

kostasrim added 2 commits August 23, 2023 17:48

chore: address gh comments

9d3b767

Merge branch 'main' into acl_part_4_add_acl_set_user

cc2432d

kostasrim requested a review from dranikpg August 23, 2023 15:28

dranikpg previously approved these changes Aug 23, 2023

View reviewed changes

chore: use StartsWith instead of string comparison

c0356d8

kostasrim dismissed dranikpg’s stale review via c0356d8 August 24, 2023 06:54

kostasrim requested a review from dranikpg August 24, 2023 06:55

chore: replace starts_with member function with absl::StartsWith

2251f8e

kostasrim enabled auto-merge (squash) August 24, 2023 08:40

dranikpg approved these changes Aug 24, 2023

View reviewed changes

kostasrim merged commit bd87fb7 into main Aug 24, 2023

kostasrim deleted the acl_part_4_add_acl_set_user branch August 24, 2023 10:24

arukiidou mentioned this pull request Aug 31, 2023

Support ACL[API-6][API-7] #447

Closed

		def assert_any_of(assertion, result):
		assert assertion == result[0] or assertion == result[1]


		using OptCat = std::optional<uint32_t>;

		std::pair<OptCat, OptCat> MaybeParseAclCategory(std::string_view command) {

		maybe_update_acl(req.plus_acl_categories, acl_plus);
		maybe_update_acl(req.minus_acl_categories, acl_minus);

feat(AclFamily): add acl setuser command #1725

feat(AclFamily): add acl setuser command #1725

Uh oh!

Conversation

kostasrim commented Aug 22, 2023

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kostasrim Aug 23, 2023 • 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!

kostasrim Aug 23, 2023 • 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!

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!

dranikpg left a comment

Choose a reason for hiding this comment

Uh oh!

dranikpg Aug 23, 2023 • 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!

Uh oh!

kostasrim Aug 23, 2023 •

edited

Loading

kostasrim Aug 23, 2023 •

edited

Loading

dranikpg Aug 23, 2023 •

edited

Loading