feat(acl): add support of multiple passwords #3189

kostasrim · 2024-06-18T17:03:54Z

resolves #2817

add support for multiple passwords
add support for deleting passwords
add support for resetpass
add tests
always prefix passwords with hashtag when printed

kostasrim · 2024-06-18T17:11:51Z

romange · 2024-06-18T18:13:10Z

That was quick! 👏🏼

romange · 2024-06-18T18:19:56Z

src/server/acl/user_registry.cc

@@ -86,11 +87,11 @@ void UserRegistry::Init() {
  auto default_user = DefaultUserUpdateRequest();
  auto maybe_password = absl::GetFlag(FLAGS_requirepass);
  if (!maybe_password.empty()) {
-    default_user.password = std::move(maybe_password);
+    default_user.passwords.front().nopass = false;


should not you set maybe_password as well?

this is a bug! it also got caught by the tests ;)

src/server/acl/user.h

dranikpg · 2024-06-20T08:00:35Z

src/server/acl/helpers.cc

+std::string PasswordsToString(const absl::flat_hash_set<std::string>& passwords, bool nopass,
+                              bool full_sha) {
+  if (nopass) {
+    return "nopass ";
+  }
+  std::string result;
+  for (const auto& pass : passwords) {
+    absl::StrAppend(&result, "#", PrettyPrintSha(pass, full_sha), " ");
+  }
+
+  return result;
+}


so it can very well be that a user doesn't have nopass set, but it's password set is empty. Maybe we should print some kind of warning in that case?

I am following redis/valkey here :) If there is no nopass or anything that starts with # it means that there is no password set and the field is left blank

dranikpg · 2024-06-20T08:01:28Z

src/server/acl/acl_family.cc

+  auto pass = PasswordsToString(user.Passwords(), user.HasNopass(), false);
+  if (!pass.empty()) {
+    pass.pop_back();
+  }


A little bit unusual interface with the leftover space 🙂 But yes, it's only used twice, so not important

Soooo, it's used in 3 places and two of them needed the extra space, so I thought this was the minimal set of changes required 🤣 I promise I will clean this, just like the other two places that almost have identical printing (acl list and when we serialize the registry to write it via acl save)

* add support for multiple passwords * add support for deleting passwords * add support for resetpass * add tests * always prefix passwords with hashtag when printed

feat(acl): add support of multiple passwords

d364a73

kostasrim self-assigned this Jun 18, 2024

kostasrim requested a review from dranikpg June 18, 2024 17:03

fix test

3d284e7

romange reviewed Jun 18, 2024

View reviewed changes

src/server/acl/user.h Show resolved Hide resolved

dranikpg reviewed Jun 20, 2024

View reviewed changes

fix bugs

1264468

kostasrim requested review from romange and dranikpg June 21, 2024 10:36

dranikpg approved these changes Jun 21, 2024

View reviewed changes

romange approved these changes Jun 21, 2024

View reviewed changes

kostasrim merged commit f01aa2d into main Jun 21, 2024
10 checks passed

kostasrim deleted the acl_multiple_passwords branch June 21, 2024 17:39

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(acl): add support of multiple passwords #3189

feat(acl): add support of multiple passwords #3189

kostasrim commented Jun 18, 2024 •

edited

Loading

kostasrim commented Jun 18, 2024

romange commented Jun 18, 2024

romange Jun 18, 2024

kostasrim Jun 20, 2024

dranikpg Jun 20, 2024

kostasrim Jun 20, 2024

dranikpg Jun 20, 2024

kostasrim Jun 20, 2024

feat(acl): add support of multiple passwords #3189

feat(acl): add support of multiple passwords #3189

Conversation

kostasrim commented Jun 18, 2024 • edited Loading

kostasrim commented Jun 18, 2024

romange commented Jun 18, 2024

romange Jun 18, 2024

Choose a reason for hiding this comment

kostasrim Jun 20, 2024

Choose a reason for hiding this comment

dranikpg Jun 20, 2024

Choose a reason for hiding this comment

kostasrim Jun 20, 2024

Choose a reason for hiding this comment

dranikpg Jun 20, 2024

Choose a reason for hiding this comment

kostasrim Jun 20, 2024

Choose a reason for hiding this comment

kostasrim commented Jun 18, 2024 •

edited

Loading