From e4923e2d107fad54899e5233fc609c9ce002fd88 Mon Sep 17 00:00:00 2001 From: Aiee <18348405+Aiee@users.noreply.github.com> Date: Tue, 4 Jan 2022 17:58:48 +0800 Subject: [PATCH 1/3] Refector logic in loadUsersAndRoles() --- src/clients/meta/MetaClient.cpp | 41 +++++++++++++++------------------ src/clients/meta/MetaClient.h | 5 ++-- 2 files changed, 21 insertions(+), 25 deletions(-) diff --git a/src/clients/meta/MetaClient.cpp b/src/clients/meta/MetaClient.cpp index 48413c7fd21..250878291d8 100644 --- a/src/clients/meta/MetaClient.cpp +++ b/src/clients/meta/MetaClient.cpp @@ -175,8 +175,6 @@ bool MetaClient::loadUsersAndRoles() { } decltype(userRolesMap_) userRolesMap; decltype(userPasswordMap_) userPasswordMap; - decltype(userPasswordAttemptsRemain_) userPasswordAttemptsRemain; - decltype(userLoginLockTime_) userLoginLockTime; // List of username std::unordered_set userNameList; @@ -188,8 +186,6 @@ bool MetaClient::loadUsersAndRoles() { } userRolesMap[user.first] = rolesRet.value(); userPasswordMap[user.first] = user.second; - userPasswordAttemptsRemain[user.first] = FLAGS_failed_login_attempts; - userLoginLockTime[user.first] = 0; userNameList.emplace(user.first); } { @@ -197,31 +193,30 @@ bool MetaClient::loadUsersAndRoles() { userRolesMap_ = std::move(userRolesMap); userPasswordMap_ = std::move(userPasswordMap); - // This method is called periodically by the heartbeat thread, but we don't want to reset the - // failed login attempts every time. Remove expired users from cache - for (auto& ele : userPasswordAttemptsRemain) { - if (userNameList.count(ele.first) == 0) { - userPasswordAttemptsRemain.erase(ele.first); - } - } - for (auto& ele : userLoginLockTime) { - if (userNameList.count(ele.first) == 0) { - userLoginLockTime.erase(ele.first); + // Remove expired users from cache + auto removeExpiredUser = [&](folly::ConcurrentHashMap& userMap, + const std::unordered_set userList) { + for (auto& ele : userMap) { + if (userList.count(ele.first) == 0) { + userMap.erase(ele.first); + } } - } + }; + removeExpiredUser(userPasswordAttemptsRemain_, userNameList); + removeExpiredUser(userLoginLockTime_, userNameList); - // If the user is not in the map, insert value with the default value + // This method is called periodically by the heartbeat thread, but we don't want to reset the + // failed login attempts every time. for (const auto& user : userNameList) { - if (userPasswordAttemptsRemain.count(user) == 0) { - userPasswordAttemptsRemain[user] = FLAGS_failed_login_attempts; + // If the user is not in the map, insert value with the default value + // Do nothing if the account is already in the map + if (userPasswordAttemptsRemain_.find(user) == userPasswordAttemptsRemain_.end()) { + userPasswordAttemptsRemain_.insert(user, FLAGS_failed_login_attempts); } - if (userLoginLockTime.count(user) == 0) { - userLoginLockTime[user] = 0; + if (userLoginLockTime_.find(user) == userLoginLockTime_.end()) { + userLoginLockTime_.insert(user, 0); } } - - userPasswordAttemptsRemain_ = std::move(userPasswordAttemptsRemain); - userLoginLockTime_ = std::move(userLoginLockTime); } return true; } diff --git a/src/clients/meta/MetaClient.h b/src/clients/meta/MetaClient.h index bb97c1dad12..22814b05fb0 100644 --- a/src/clients/meta/MetaClient.h +++ b/src/clients/meta/MetaClient.h @@ -7,6 +7,7 @@ #define CLIENTS_META_METACLIENT_H_ #include +#include #include #include #include @@ -145,9 +146,9 @@ using UserRolesMap = std::unordered_map // get user password by account using UserPasswordMap = std::unordered_map; // Mapping of user name and remaining wrong password attempts -using UserPasswordAttemptsRemain = std::unordered_map; +using UserPasswordAttemptsRemain = folly::ConcurrentHashMap; // Mapping of user name and the timestamp when the account is locked -using UserLoginLockTime = std::unordered_map; +using UserLoginLockTime = folly::ConcurrentHashMap; // config cache, get config via module and name using MetaConfigMap = From 4b0efefc4d3f99d082206a29691eb3584bd253ab Mon Sep 17 00:00:00 2001 From: Aiee <18348405+Aiee@users.noreply.github.com> Date: Tue, 4 Jan 2022 22:42:37 +0800 Subject: [PATCH 2/3] Substitude std::map with folly::ConcurrentHashMap --- src/clients/meta/MetaClient.cpp | 32 ++++++++++++++++---------------- 1 file changed, 16 insertions(+), 16 deletions(-) diff --git a/src/clients/meta/MetaClient.cpp b/src/clients/meta/MetaClient.cpp index 250878291d8..b1fc57e9e2f 100644 --- a/src/clients/meta/MetaClient.cpp +++ b/src/clients/meta/MetaClient.cpp @@ -2421,13 +2421,10 @@ Status MetaClient::authCheckFromCache(const std::string& account, const std::str return Status::Error("User not exist"); } - folly::RWSpinLock::WriteHolder holder(localCacheLock_); + auto lockedSince = userLoginLockTime_[account]; + auto passwordAttemtRemain = userPasswordAttemptsRemain_[account]; - auto& lockedSince = userLoginLockTime_[account]; - auto& passwordAttemtRemain = userPasswordAttemptsRemain_[account]; - LOG(INFO) << "Thread id: " << std::this_thread::get_id() - << " ,passwordAttemtRemain: " << passwordAttemtRemain; - // lockedSince is non-zero means the account has been locked + // If lockedSince is non-zero, it means the account has been locked if (lockedSince != 0) { auto remainingLockTime = (lockedSince + FLAGS_password_lock_time_in_secs) - time::WallClock::fastNowInSec(); @@ -2441,8 +2438,9 @@ Status MetaClient::authCheckFromCache(const std::string& account, const std::str remainingLockTime); } // Clear lock state and reset attempts - lockedSince = 0; - passwordAttemtRemain = FLAGS_failed_login_attempts; + userLoginLockTime_.assign_if_equal(account, lockedSince, 0); + userPasswordAttemptsRemain_.assign_if_equal( + account, passwordAttemtRemain, FLAGS_failed_login_attempts); } if (iter->second != password) { @@ -2453,12 +2451,14 @@ Status MetaClient::authCheckFromCache(const std::string& account, const std::str // If the password is not correct and passwordAttemtRemain > 0, // Allow another attemp + passwordAttemtRemain = userPasswordAttemptsRemain_[account]; if (passwordAttemtRemain > 0) { - --passwordAttemtRemain; - if (passwordAttemtRemain == 0) { + auto newAttemtRemain = passwordAttemtRemain - 1; + userPasswordAttemptsRemain_.assign_if_equal(account, passwordAttemtRemain, newAttemtRemain); + if (newAttemtRemain == 0) { // If the remaining attemps is 0, failed to authenticate // Block user login - lockedSince = time::WallClock::fastNowInSec(); + userLoginLockTime_.assign_if_equal(account, 0, time::WallClock::fastNowInSec()); return Status::Error( "%d times consecutive incorrect passwords has been input, user name: %s has been " "locked, try again in %d seconds", @@ -2466,14 +2466,14 @@ Status MetaClient::authCheckFromCache(const std::string& account, const std::str account.c_str(), FLAGS_password_lock_time_in_secs); } - LOG(ERROR) << "Invalid password, remaining attempts: " << passwordAttemtRemain; - return Status::Error("Invalid password, remaining attempts: %d", passwordAttemtRemain); + LOG(ERROR) << "Invalid password, remaining attempts: " << newAttemtRemain; + return Status::Error("Invalid password, remaining attempts: %d", newAttemtRemain); } } - // Reset password attempts - passwordAttemtRemain = FLAGS_failed_login_attempts; - lockedSince = 0; + // Authentication succeed, reset password attempts + userPasswordAttemptsRemain_.assign(account, FLAGS_failed_login_attempts); + userLoginLockTime_.assign(account, 0); return Status::OK(); } From fc95ed6b6125c103d45005d9865a5d835c2b76b4 Mon Sep 17 00:00:00 2001 From: Aiee <18348405+Aiee@users.noreply.github.com> Date: Wed, 5 Jan 2022 17:12:01 +0800 Subject: [PATCH 3/3] Address comments --- src/clients/meta/MetaClient.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/clients/meta/MetaClient.cpp b/src/clients/meta/MetaClient.cpp index b1fc57e9e2f..3d08c9cfe18 100644 --- a/src/clients/meta/MetaClient.cpp +++ b/src/clients/meta/MetaClient.cpp @@ -195,7 +195,7 @@ bool MetaClient::loadUsersAndRoles() { // Remove expired users from cache auto removeExpiredUser = [&](folly::ConcurrentHashMap& userMap, - const std::unordered_set userList) { + const std::unordered_set& userList) { for (auto& ele : userMap) { if (userList.count(ele.first) == 0) { userMap.erase(ele.first);