do not use hash to determine if roles changed (similar to ClickHouse#88412)

Author: zvonand

src/Access/TokenAccessStorage.cpp

@@ -43,7 +43,7 @@ TokenAccessStorage::TokenAccessStorage(const String & storage_name_, AccessContr
     }
     common_role_names.swap(common_roles_cfg);
 
-    external_role_hashes.clear();
+    user_external_roles.clear();
     users_per_roles.clear();
     roles_per_users.clear();
     granted_role_names.clear();
@@ -223,7 +223,7 @@ std::optional<std::pair<String, AccessEntityType>> TokenAccessStorage::readNameW
     return memory_storage.readNameWithType(id, throw_if_not_exists);
 }
 
-void TokenAccessStorage::assignRolesNoLock(User & user, const std::set<String> & external_roles, std::size_t external_roles_hash) const
+void TokenAccessStorage::assignRolesNoLock(User & user, const std::set<String> & external_roles) const
 {
     const auto & user_name = user.getName();
     auto & granted_roles = user.granted_roles;
@@ -251,7 +251,7 @@ void TokenAccessStorage::assignRolesNoLock(User & user, const std::set<String> &
         }
     };
 
-    external_role_hashes.erase(user_name);
+    user_external_roles.erase(user_name);
     granted_roles = {};
     const auto old_role_names = std::move(roles_per_users[user_name]);
 
@@ -299,31 +299,28 @@ void TokenAccessStorage::assignRolesNoLock(User & user, const std::set<String> &
         granted_role_ids.erase(iit);
     }
 
-    // Actualize roles_per_users mapping and external_role_hashes cache.
+    // Actualize roles_per_users mapping and user_external_roles cache.
     if (external_roles.empty())
         roles_per_users.erase(user_name);
     else
         roles_per_users[user_name] = external_roles;
 
-    external_role_hashes[user_name] = external_roles_hash;
+    user_external_roles[user_name] = external_roles;
 }
 
 void TokenAccessStorage::updateAssignedRolesNoLock(const UUID & id, const String & user_name, const std::set<String> & external_roles) const
 {
-    // No need to include common_role_names in this hash each time, since they don't change.
-    const auto external_roles_hash = boost::hash<std::set<String>>{}(external_roles);
-
     // Map and grant the roles from scratch only if the list of external role has changed.
-    const auto it = external_role_hashes.find(user_name);
-    if (it != external_role_hashes.end() && it->second == external_roles_hash)
+    const auto it = user_external_roles.find(user_name);
+    if (it != user_external_roles.end() && it->second == external_roles)
         return;
 
-    auto update_func = [this, &external_roles, external_roles_hash] (const AccessEntityPtr & entity_, const UUID &) -> AccessEntityPtr
+    auto update_func = [this, &external_roles] (const AccessEntityPtr & entity_, const UUID &) -> AccessEntityPtr
     {
         if (auto user = typeid_cast<std::shared_ptr<const User>>(entity_))
         {
             auto changed_user = typeid_cast<std::shared_ptr<User>>(user->clone());
-            assignRolesNoLock(*changed_user, external_roles, external_roles_hash);
+            assignRolesNoLock(*changed_user, external_roles);
             return changed_user;
         }
         return entity_;
@@ -390,7 +387,7 @@ std::optional<AuthResult> TokenAccessStorage::authenticateImpl(
 
     if (new_user)
     {
-        assignRolesNoLock(*new_user, external_roles, boost::hash<std::set<String>>{}(external_roles));
+        assignRolesNoLock(*new_user, external_roles);
         id = memory_storage.insert(new_user);
     }
     else

src/Access/TokenAccessStorage.h

@@ -51,7 +51,7 @@ class TokenAccessStorage : public IAccessStorage
     std::optional<re2::RE2> roles_filter = std::nullopt;
 
     std::set<String> common_role_names;                         // role name that should be granted to all users at all times
-    mutable std::map<String, std::size_t> external_role_hashes;
+    mutable std::map<String, std::set<String>> user_external_roles;
     mutable std::map<String, std::set<String>> users_per_roles; // role name -> user names (...it should be granted to; may but don't have to exist for common roles)
     mutable std::map<String, std::set<String>> roles_per_users; // user name -> role names (...that should be granted to it; may but don't have to include common roles)
     mutable std::map<UUID, String> granted_role_names;          // (currently granted) role id -> its name
@@ -65,7 +65,7 @@ class TokenAccessStorage : public IAccessStorage
     bool areTokenCredentialsValidNoLock(const User & user, const Credentials & credentials, const ExternalAuthenticators & external_authenticators) const;
 
     void applyRoleChangeNoLock(bool grant, const UUID & role_id, const String & role_name);
-    void assignRolesNoLock(User & user, const std::set<String> & external_roles, std::size_t external_roles_hash) const;
+    void assignRolesNoLock(User & user, const std::set<String> & external_roles) const;
     void updateAssignedRolesNoLock(const UUID & id, const String & user_name, const std::set<String> & external_roles) const;
 
 protected: