Simplified the login throttler, corrected races on throttler access, recreate the auth cookie with the remaining duration

src/auth/AuthService.cpp

@@ -26,46 +26,66 @@
 #include <Wt/WRandom.h>
 
 #include "database/Session.hpp"
+#include "utils/Exception.hpp"
 #include "utils/Utils.hpp"
 #include "utils/Logger.hpp"
 
 namespace Auth {
 
 AuthService::AuthService(std::size_t maxThrottlerEntries)
-: _loginThrottler {maxThrottlerEntries}
+: _passwordLoginThrottler {maxThrottlerEntries}
+, _tokenLoginThrottler {maxThrottlerEntries}
 {
 }
 
-AuthService::PasswordCheckResult
-AuthService::checkUserPassword(Database::Session& session, const boost::asio::ip::address& clientAddress, const std::string& loginName, const std::string& password)
+static
+bool
+checkUserPassword(Database::Session& session, const std::string& loginName, const std::string& password)
 {
-	if (_loginThrottler.isClientThrottled(clientAddress))
-		return PasswordCheckResult::Throttled;
-
 	Database::User::PasswordHash passwordHash;
 	{
 		auto transaction {session.createSharedTransaction()};
 
 		const Database::User::pointer user {Database::User::getByLoginName(session, loginName)};
 		if (!user)
-		{
-			_loginThrottler.onBadClientAttempt(clientAddress);
-			return PasswordCheckResult::Mismatch;
-		}
+			return false;
 
 		passwordHash = user->getPasswordHash();
 	}
 
 	const Wt::Auth::BCryptHashFunction hashFunc {6};
-	if (hashFunc.verify(password, passwordHash.salt, passwordHash.hash))
+	return hashFunc.verify(password, passwordHash.salt, passwordHash.hash);
+}
+
+
+AuthService::PasswordCheckResult
+AuthService::checkUserPassword(Database::Session& session, const boost::asio::ip::address& clientAddress, const std::string& loginName, const std::string& password)
+{
+	// Do not waste too much resource on brute force attacks (optim)
 	{
-		_loginThrottler.onGoodClientAttempt(clientAddress);
-		return PasswordCheckResult::Match;
+		std::shared_lock<std::shared_timed_mutex> lock {_passwordCheckMutex};
+
+		if (_passwordLoginThrottler.isClientThrottled(clientAddress))
+			return PasswordCheckResult::Throttled;
 	}
-	else
+
+	const bool match {Auth::checkUserPassword(session, loginName, password)};
 	{
-		_loginThrottler.onBadClientAttempt(clientAddress);
-		return PasswordCheckResult::Mismatch;
+		std::unique_lock<std::shared_timed_mutex> lock {_passwordCheckMutex};
+
+		if (_passwordLoginThrottler.isClientThrottled(clientAddress))
+			return PasswordCheckResult::Throttled;
+
+		if (match)
+		{
+			_passwordLoginThrottler.onGoodClientAttempt(clientAddress);
+			return PasswordCheckResult::Match;
+		}
+		else
+		{
+			_passwordLoginThrottler.onBadClientAttempt(clientAddress);
+			return PasswordCheckResult::Mismatch;
+		}
 	}
 }
 
@@ -93,5 +113,82 @@ AuthService::evaluatePasswordStrength(const std::string& loginName, const std::s
 	return validator.evaluateStrength(password, loginName, "").isValid();
 }
 
+
+std::string
+AuthService::createAuthToken(Database::Session& session, Database::IdType userId, const Wt::WDateTime& expiry)
+{
+	const std::string secret {Wt::WRandom::generateId(64)};
+
+	auto transaction {session.createUniqueTransaction()};
+
+	Database::User::pointer user {Database::User::getById(session, userId)};
+	if (!user)
+		throw LmsException {"User deleted"};
+
+	Database::AuthToken::pointer authToken {Database::AuthToken::create(session, secret, expiry, user)};
+
+	LMS_LOG(UI, DEBUG) << "Created auth token for user '" << user->getLoginName() << "', expiry = " << expiry.toString();
+
+	if (user->getAuthTokensCount() >= 50)
+		Database::AuthToken::removeExpiredTokens(session, Wt::WDateTime::currentDateTime());
+
+	return secret;
+}
+
+static
+boost::optional<AuthService::AuthTokenProcessResult::AuthTokenInfo>
+processAuthToken(Database::Session& session, const std::string& tokenValue)
+{
+	auto transaction {session.createUniqueTransaction()};
+
+	Database::AuthToken::pointer authToken {Database::AuthToken::getByValue(session, tokenValue)};
+	if (!authToken)
+		return boost::none;
+
+	if (authToken->getExpiry() < Wt::WDateTime::currentDateTime())
+	{
+		authToken.remove();
+		return boost::none;
+	}
+
+	LMS_LOG(UI, DEBUG) << "Found auth token for user '" << authToken->getUser()->getLoginName() << "'!";
+
+	AuthService::AuthTokenProcessResult::AuthTokenInfo res {authToken->getUser().id(), authToken->getExpiry()};
+	authToken.remove();
+
+	return res;
+}
+
+
+AuthService::AuthTokenProcessResult
+AuthService::processAuthToken(Database::Session& session, const boost::asio::ip::address& clientAddress, const std::string& tokenValue)
+{
+	// Do not waste too much resource on brute force attacks (optim)
+	{
+		std::shared_lock<std::shared_timed_mutex> lock {_tokenCheckMutex};
+
+		if (_tokenLoginThrottler.isClientThrottled(clientAddress))
+			return AuthTokenProcessResult {AuthTokenProcessResult::State::Throttled};
+	}
+
+	auto res {Auth::processAuthToken(session, tokenValue)};
+	{
+		std::unique_lock<std::shared_timed_mutex> lock {_tokenCheckMutex};
+
+		if (_tokenLoginThrottler.isClientThrottled(clientAddress))
+			return AuthTokenProcessResult {AuthTokenProcessResult::State::Throttled};
+
+		if (!res)
+		{
+			_tokenLoginThrottler.onBadClientAttempt(clientAddress);
+			return AuthTokenProcessResult {AuthTokenProcessResult::State::NotFound};
+		}
+
+		_tokenLoginThrottler.onGoodClientAttempt(clientAddress);
+		return AuthTokenProcessResult {AuthTokenProcessResult::State::Found, std::move(*res)};
+	}
+}
+
+
 } // namespace Auth
 

src/auth/AuthService.hpp

@@ -23,10 +23,12 @@
 
 #include <string>
 
+#include <boost/optional.hpp>
 #include <boost/asio/ip/address.hpp>
 
 #include "LoginThrottler.hpp"
 #include "database/User.hpp"
+#include "database/Types.hpp"
 
 namespace Database
 {
@@ -62,9 +64,37 @@ namespace Auth {
 			Database::User::PasswordHash	hashPassword(const std::string& password) const;
 			bool				evaluatePasswordStrength(const std::string& loginName, const std::string& password) const;
 
+			// Auth Token services
+			struct AuthTokenProcessResult
+			{
+				enum class State
+				{
+					Found,
+					Throttled,
+					NotFound,
+				};
+
+				struct AuthTokenInfo
+				{
+					Database::IdType userId;
+					Wt::WDateTime expiry;
+				};
+
+				State state;
+				boost::optional<AuthTokenInfo>	authTokenInfo;
+			};
+
+			// Removed if found
+			AuthTokenProcessResult	processAuthToken(Database::Session& session, const boost::asio::ip::address& clientAddress, const std::string& tokenValue);
+			std::string		createAuthToken(Database::Session& session, Database::IdType userid, const Wt::WDateTime& expiry);
+
 		private:
 
-			LoginThrottler _loginThrottler;
+			std::shared_timed_mutex	_passwordCheckMutex;
+			std::shared_timed_mutex	_tokenCheckMutex;
+
+			LoginThrottler	_passwordLoginThrottler;
+			LoginThrottler	_tokenLoginThrottler;
 	};
 
 }

src/auth/LoginThrottler.cpp

@@ -54,7 +54,7 @@ LoginThrottler::removeOutdatedEntries()
 
 	for (auto it {std::begin(_attemptsInfo)}; it != std::end(_attemptsInfo); )
 	{
-		if (it->second.nextValidAttempt <= now)
+		if (it->second <= now)
 			it = _attemptsInfo.erase(it);
 		else
 			++it;
@@ -68,32 +68,14 @@ LoginThrottler::onBadClientAttempt(const boost::asio::ip::address& address)
 
 	const Wt::WDateTime now {Wt::WDateTime::currentDateTime()};
 
-	std::unique_lock<std::shared_timed_mutex> lock {_mutex};
-
 	if (_attemptsInfo.size() >= _maxEntries)
 		removeOutdatedEntries();
-	// If still full, kill one random entry
 	if (_attemptsInfo.size() >= _maxEntries)
 		_attemptsInfo.erase(pickRandom(_attemptsInfo));
 
-	AttemptsInfo& attemptsInfo {_attemptsInfo[address]};
+	_attemptsInfo[address] = now.addSecs(3);
 
-	attemptsInfo.nbSuccessiveBadAttempts++;
-
-	if (attemptsInfo.nbSuccessiveBadAttempts >= 50)
-		attemptsInfo.nextValidAttempt = now.addSecs(60);
-	if (attemptsInfo.nbSuccessiveBadAttempts >= 20)
-		attemptsInfo.nextValidAttempt = now.addSecs(10);
-	else if (attemptsInfo.nbSuccessiveBadAttempts >= 10)
-		attemptsInfo.nextValidAttempt = now.addSecs(5);
-	else if (attemptsInfo.nbSuccessiveBadAttempts >= 5)
-		attemptsInfo.nextValidAttempt = now.addSecs(2);
-	else if (attemptsInfo.nbSuccessiveBadAttempts >= 2)
-		attemptsInfo.nextValidAttempt = now.addSecs(1);
-	else
-		attemptsInfo.nextValidAttempt = now;
-
-	LMS_LOG(AUTH, INFO) << "Registering bad attempt for '" << clientAddress.to_string() << "' (" << attemptsInfo.nbSuccessiveBadAttempts << " successive bad attempts)";
+	LMS_LOG(AUTH, INFO) << "Registering bad attempt for '" << clientAddress.to_string() << "'";
 }
 
 void
@@ -101,8 +83,6 @@ LoginThrottler::onGoodClientAttempt(const boost::asio::ip::address& address)
 {
 	const boost::asio::ip::address clientAddress {getAddressToThrottle(address)};
 
-	std::unique_lock<std::shared_timed_mutex> lock {_mutex};
-
 	_attemptsInfo.erase(address);
 }
 
@@ -111,13 +91,11 @@ LoginThrottler::isClientThrottled(const boost::asio::ip::address& address) const
 {
 	const boost::asio::ip::address clientAddress {getAddressToThrottle(address)};
 
-	std::shared_lock<std::shared_timed_mutex> lock {_mutex};
-
 	auto it {_attemptsInfo.find(address)};
 	if (it == _attemptsInfo.end())
 		return false;
 
-	return it->second.nextValidAttempt > Wt::WDateTime::currentDateTime();
+	return it->second > Wt::WDateTime::currentDateTime();
 }
 
 } // Auth

src/auth/LoginThrottler.hpp

@@ -37,6 +37,7 @@ class LoginThrottler
 	public:
 		LoginThrottler(std::size_t maxEntries) : _maxEntries {maxEntries} {}
 
+		// user must lock these calls to avoid races
 		bool isClientThrottled(const boost::asio::ip::address& address) const;
 		void onBadClientAttempt(const boost::asio::ip::address& address);
 		void onGoodClientAttempt(const boost::asio::ip::address& address);
@@ -45,16 +46,9 @@ class LoginThrottler
 
 		void removeOutdatedEntries();
 
-		struct AttemptsInfo
-		{
-			std::size_t	nbSuccessiveBadAttempts {};
-			Wt::WDateTime	nextValidAttempt;
-		};
-
 		const std::size_t _maxEntries;
 
-		mutable std::shared_timed_mutex					_mutex;
-		std::unordered_map<boost::asio::ip::address, AttemptsInfo>	_attemptsInfo;
+		std::unordered_map<boost::asio::ip::address, Wt::WDateTime>	_attemptsInfo;
 };
 
 

src/database/Session.cpp

@@ -213,6 +213,7 @@ Session::prepareTables()
 		_session.execute("CREATE INDEX IF NOT EXISTS artist_sort_name_nocase_idx ON artist(sort_name COLLATE NOCASE)");
 		_session.execute("CREATE INDEX IF NOT EXISTS artist_mbid_idx ON artist(mbid)");
 		_session.execute("CREATE INDEX IF NOT EXISTS auth_token_user_idx ON auth_token(user_id)");
+		_session.execute("CREATE INDEX IF NOT EXISTS auth_token_value_idx ON auth_token(value)");
 		_session.execute("CREATE INDEX IF NOT EXISTS cluster_name_idx ON cluster(name)");
 		_session.execute("CREATE INDEX IF NOT EXISTS cluster_cluster_type_idx ON cluster(cluster_type_id)");
 		_session.execute("CREATE INDEX IF NOT EXISTS cluster_type_name_idx ON cluster_type(name)");

src/database/User.hpp

@@ -61,10 +61,12 @@ class AuthToken
 		static pointer create(Session& session, const std::string& value, const Wt::WDateTime&expiry, Wt::Dbo::ptr<User> user);
 		static void removeExpiredTokens(Session& session, Wt::WDateTime now);
 		static pointer getByValue(Session& session, const std::string& value);
+		static pointer getById(Session& session, IdType tokenId);
 
 		// Accessors
-		Wt::Dbo::ptr<User>	getUser() const { return _user; }
 		const Wt::WDateTime&	getExpiry() const { return _expiry; }
+		Wt::Dbo::ptr<User>	getUser() const { return _user; }
+		const std::string&	getValue() const { return _value; }
 
 		template<class Action>
 		void persist(Action& a)
@@ -121,6 +123,7 @@ class User : public Wt::Dbo::Dbo<User>
 		const std::string& getLoginName() const { return _loginName; }
 		PasswordHash getPasswordHash() const { return PasswordHash {_passwordSalt, _passwordHash}; }
 		Wt::WDateTime getLastLogin() const { return _lastLogin; }
+		std::size_t getAuthTokensCount() const { return _authTokens.size(); }
 
 		// write
 		void setLastLogin(const Wt::WDateTime& dateTime)	{ _lastLogin = dateTime; }

src/ui/Auth.cpp

@@ -40,31 +40,13 @@ static const std::string authCookieName {"LmsAuth"};
 
 static
 void
-createAuthToken(Database::IdType userId)
+createAuthToken(Database::IdType userId, const Wt::WDateTime& expiry)
 {
-
-	const std::string secret {Wt::WRandom::generateId(48)};
-	const Wt::WDateTime now {Wt::WDateTime::currentDateTime()};
-	Wt::WDateTime expiry;
-
-	{
-		auto transaction {LmsApp->getDbSession().createUniqueTransaction()};
-
-		Database::User::pointer user {Database::User::getById(LmsApp->getDbSession(), userId)};
-		if (!user)
-			return;
-
-		expiry = user->isDemo() ? now.addDays(7) : now.addYears(1);
-		Database::AuthToken::create(LmsApp->getDbSession(), secret, expiry, user);
-
-		LMS_LOG(UI, DEBUG) << "Created auth token for user '" << user->getLoginName() << "', expiry = " << expiry.toString();
-
-		Database::AuthToken::removeExpiredTokens(LmsApp->getDbSession(), now);
-	}
+	const std::string secret {getService<::Auth::AuthService>()->createAuthToken(LmsApp->getDbSession(), userId, expiry)};
 
 	LmsApp->setCookie(authCookieName,
 			secret,
-			expiry.toTime_t() - now.toTime_t(),
+			expiry.toTime_t() - Wt::WDateTime::currentDateTime().toTime_t(),
 			"",
 			"",
 			LmsApp->environment().urlScheme() == "https");
@@ -78,33 +60,20 @@ processAuthToken(const Wt::WEnvironment& env)
 	if (!authCookie)
 		return boost::none;
 
-	Database::IdType userId {};
+	const auto res {getService<::Auth::AuthService>()->processAuthToken(LmsApp->getDbSession(), boost::asio::ip::address::from_string(env.clientAddress()), *authCookie)};
+	switch (res.state)
 	{
-		auto transaction {LmsApp->getDbSession().createUniqueTransaction()};
-
-		Database::AuthToken::pointer authToken {Database::AuthToken::getByValue(LmsApp->getDbSession(), *authCookie)};
-		if (!authToken)
-		{
-			LMS_LOG(UI, INFO) << "Client '" << env.clientAddress() << "' presented a token that has not been found";
+		case ::Auth::AuthService::AuthTokenProcessResult::State::NotFound:
+		case ::Auth::AuthService::AuthTokenProcessResult::State::Throttled:
+			LmsApp->setCookie(authCookieName, std::string {}, 0, "", "", env.urlScheme() == "https");
 			return boost::none;
-		}
 
-		if (authToken->getExpiry() < Wt::WDateTime::currentDateTime())
-		{
-			LMS_LOG(UI, INFO) << "Expired auth token for user '" << authToken->getUser()->getLoginName() << "'!";
-			authToken.remove();
-			return boost::none;
-		}
-
-		LMS_LOG(UI, DEBUG) << "Found auth token for user '" << authToken->getUser()->getLoginName() << "'!";
-		userId = authToken->getUser().id();
-
-		authToken.remove();
+		case ::Auth::AuthService::AuthTokenProcessResult::State::Found:
+			createAuthToken(res.authTokenInfo->userId, res.authTokenInfo->expiry);
+			break;
 	}
 
-	createAuthToken(userId);
-
-	return userId;
+	return res.authTokenInfo->userId;
 }
 
 class AuthModel : public Wt::WFormModel
@@ -129,16 +98,24 @@ class AuthModel : public Wt::WFormModel
 
 		void saveData()
 		{
+			bool isDemo;
 			{
 				auto transaction {LmsApp->getDbSession().createUniqueTransaction()};
 
 				Database::User::pointer user {Database::User::getByLoginName(LmsApp->getDbSession(), valueText(LoginNameField).toUTF8())};
 				user.modify()->setLastLogin(Wt::WDateTime::currentDateTime());
 				_userId = user.id();
+
+				const Wt::WDateTime now {Wt::WDateTime::currentDateTime()};
+				isDemo = user->isDemo();
 			}
 
 			if (Wt::asNumber(value(RememberMeField)))
-				createAuthToken(*_userId);
+			{
+				const Wt::WDateTime now {Wt::WDateTime::currentDateTime()};
+
+				createAuthToken(*_userId, isDemo ? now.addDays(3) : now.addYears(1));
+			}
 		}
 
 		bool validateField(Field field)

src/ui/Auth.hpp

@@ -27,9 +27,8 @@
 
 namespace UserInterface {
 
-
-// If success, returns the authenticated user id
-boost::optional<Database::IdType> processAuthToken(const Wt::WEnvironment& env);
+boost::optional<Database::IdType>
+processAuthToken(const Wt::WEnvironment& env);
 
 class Auth : public Wt::WTemplateFormView
 {

src/ui/LmsApplication.cpp

@@ -168,7 +168,7 @@ LmsApplication::LmsApplication(const Wt::WEnvironment& env,
 		return;
 	}
 
-	auto userId {processAuthToken(env)};
+	const auto userId {processAuthToken(env)};
 	if (userId)
 	{
 		handleUserLoggedIn(*userId);