Refactor exception handling in login() et al.

Simplify LDAPClient and LDAPAccessStorage

src/Access/IAccessStorage.cpp

@@ -14,6 +14,8 @@ namespace ErrorCodes
     extern const int ACCESS_ENTITY_ALREADY_EXISTS;
     extern const int ACCESS_ENTITY_NOT_FOUND;
     extern const int ACCESS_STORAGE_READONLY;
+    extern const int WRONG_PASSWORD;
+    extern const int IP_ADDRESS_NOT_ALLOWED;
     extern const int AUTHENTICATION_FAILED;
     extern const int LOGICAL_ERROR;
 }
@@ -420,7 +422,14 @@ UUID IAccessStorage::login(
     const Poco::Net::IPAddress & address,
     const ExternalAuthenticators & external_authenticators) const
 {
-    return loginImpl(user_name, password, address, external_authenticators);
+    try {
+        return loginImpl(user_name, password, address, external_authenticators);
+    }
+    catch (...)
+    {
+        tryLogCurrentException(getLogger(), user_name + ": Authentication failed");
+        throwCannotAuthenticate(user_name);
+    }
 }
 
 
@@ -434,11 +443,16 @@ UUID IAccessStorage::loginImpl(
     {
         if (auto user = tryRead<User>(*id))
         {
-            if (isPasswordCorrectImpl(*user, password, external_authenticators) && isAddressAllowedImpl(*user, address))
+            if (!isPasswordCorrectImpl(*user, password, external_authenticators))
-                return *id;
+                throwInvalidPassword();
+
+            if (!isAddressAllowedImpl(*user, address))
+                throwAddressNotAllowed(address);
+
+            return *id;
         }
     }
-    throwCannotAuthenticate(user_name);
+    throwNotFound(EntityType::USER, user_name);
 }
 
 
@@ -554,6 +568,15 @@ void IAccessStorage::throwReadonlyCannotRemove(EntityType type, const String & n
         ErrorCodes::ACCESS_STORAGE_READONLY);
 }
 
+void IAccessStorage::throwAddressNotAllowed(const Poco::Net::IPAddress & address)
+{
+    throw Exception("Connections from " + address.toString() + " are not allowed", ErrorCodes::IP_ADDRESS_NOT_ALLOWED);
+}
+
+void IAccessStorage::throwInvalidPassword()
+{
+    throw Exception("Invalid password", ErrorCodes::WRONG_PASSWORD);
+}
 
 void IAccessStorage::throwCannotAuthenticate(const String & user_name)
 {

src/Access/IAccessStorage.h

@@ -182,6 +182,8 @@ protected:
     [[noreturn]] void throwReadonlyCannotInsert(EntityType type, const String & name) const;
     [[noreturn]] void throwReadonlyCannotUpdate(EntityType type, const String & name) const;
     [[noreturn]] void throwReadonlyCannotRemove(EntityType type, const String & name) const;
+    [[noreturn]] static void throwAddressNotAllowed(const Poco::Net::IPAddress & address);
+    [[noreturn]] static void throwInvalidPassword();
     [[noreturn]] static void throwCannotAuthenticate(const String & user_name);
 
     using Notification = std::tuple<OnChangedHandler, UUID, AccessEntityPtr>;

src/Access/LDAPAccessStorage.cpp

@@ -230,42 +230,50 @@ bool LDAPAccessStorage::hasSubscriptionImpl(EntityType type) const
 UUID LDAPAccessStorage::loginImpl(const String & user_name, const String & password, const Poco::Net::IPAddress & address, const ExternalAuthenticators & external_authenticators) const
 {
     std::scoped_lock lock(mutex);
-    try
+    auto id = memory_storage.find<User>(user_name);
+    if (id)
     {
-        auto id = memory_storage.find<User>(user_name);
+        auto user = memory_storage.read<User>(*id);
-        if (id)
-        {
-            auto user = memory_storage.tryRead<User>(*id);
-            if (user && isAddressAllowedImpl(*user, address) && isPasswordCorrectImpl(*user, password, external_authenticators))
-                return *id;
-        }
-        else
-        {
-            // User does not exist, so we create one, and will add it if authentication is successful.
-            auto user = std::make_shared<User>();
-            user->setName(user_name);
-            user->authentication = Authentication(Authentication::Type::LDAP_SERVER);
-            user->authentication.setServerName(ldap_server);
 
-            if (isAddressAllowedImpl(*user, address) && isPasswordCorrectImpl(*user, password, external_authenticators))
+        if (!isPasswordCorrectImpl(*user, password, external_authenticators))
-            {
+            throwInvalidPassword();
-                for (const auto& role_name : default_role_names)
+
-                {
+        if (!isAddressAllowedImpl(*user, address))
-                    std::optional<UUID> role_id = access_control_manager->find<Role>(role_name);
+            throwAddressNotAllowed(address);
-                    if (!role_id)
+
-                        throw Exception("One of the default roles, the role '" + role_name + "', is not found", IAccessEntity::TypeInfo::get(IAccessEntity::Type::ROLE).not_found_error_code);
+        return *id;
-                    roles_of_interest.insert(role_id.value());
-                    user->granted_roles.grant(role_id.value());
-                }
-                return memory_storage.insert(user);
-            }
-        }
     }
-    catch (...)
+    else
     {
-        tryLogCurrentException(getLogger(), "Authentication failed for user '" + user_name + "' from access storage '" + getStorageName() + "'");
+        // User does not exist, so we create one, and will add it if authentication is successful.
+        auto user = std::make_shared<User>();
+        user->setName(user_name);
+        user->authentication = Authentication(Authentication::Type::LDAP_SERVER);
+        user->authentication.setServerName(ldap_server);
+
+        if (!isPasswordCorrectImpl(*user, password, external_authenticators))
+            throwInvalidPassword();
+
+        if (!isAddressAllowedImpl(*user, address))
+            throwAddressNotAllowed(address);
+
+        for (const auto& role_name : default_role_names)
+        {
+            std::optional<UUID> role_id = access_control_manager->find<Role>(role_name);
+            if (!role_id)
+                throwDefaultRoleNotFound(role_name);
+
+            roles_of_interest.insert(role_id.value());
+            user->granted_roles.grant(role_id.value());
+        }
+
+        return memory_storage.insert(user);
     }
-    throwCannotAuthenticate(user_name);
+}
+
+void LDAPAccessStorage::throwDefaultRoleNotFound(const String & role_name)
+{
+    throw Exception("One of the default roles, the role '" + role_name + "', is not found", IAccessEntity::TypeInfo::get(IAccessEntity::Type::ROLE).not_found_error_code);
 }
 
 }

src/Access/LDAPAccessStorage.h

@@ -55,6 +55,8 @@ private:
     void setConfiguration(AccessControlManager * access_control_manager_, const Poco::Util::AbstractConfiguration & config, const String & prefix);
     void processRoleChange(const UUID & id, const AccessEntityPtr & entity);
 
+    [[noreturn]] static void throwDefaultRoleNotFound(const String & role_name);
+
     mutable std::recursive_mutex mutex;
     AccessControlManager * access_control_manager = nullptr;
     String ldap_server;

src/Access/LDAPClient.cpp

@@ -106,14 +106,6 @@ void LDAPClient::openConnection()
 {
     std::scoped_lock lock(ldap_global_mutex);
 
-    const bool graceful_bind_failure = false;
-    diag(openConnection(graceful_bind_failure));
-}
-
-int LDAPClient::openConnection(const bool graceful_bind_failure)
-{
-    std::scoped_lock lock(ldap_global_mutex);
-
     closeConnection();
 
     {
@@ -244,8 +236,6 @@ int LDAPClient::openConnection(const bool graceful_bind_failure)
     if (params.enable_tls == LDAPServerParams::TLSEnable::YES_STARTTLS)
         diag(ldap_start_tls_s(handle, nullptr, nullptr));
 
-    int rc = LDAP_OTHER;
-
     switch (params.sasl_mechanism)
     {
         case LDAPServerParams::SASLMechanism::SIMPLE:
@@ -256,16 +246,15 @@ int LDAPClient::openConnection(const bool graceful_bind_failure)
             cred.bv_val = const_cast<char *>(params.password.c_str());
             cred.bv_len = params.password.size();
 
-            rc = ldap_sasl_bind_s(handle, dn.c_str(), LDAP_SASL_SIMPLE, &cred, nullptr, nullptr, nullptr);
+            diag(ldap_sasl_bind_s(handle, dn.c_str(), LDAP_SASL_SIMPLE, &cred, nullptr, nullptr, nullptr));
-
-            if (!graceful_bind_failure)
-                diag(rc);
 
             break;
         }
+        default:
+        {
+            throw Exception("Unknown SASL mechanism", ErrorCodes::LDAP_ERROR);
+        }
     }
-
-    return rc;
 }
 
 void LDAPClient::closeConnection() noexcept
@@ -286,39 +275,16 @@ bool LDAPSimpleAuthClient::check()
     if (params.user.empty())
         throw Exception("LDAP authentication of a user with empty name is not allowed", ErrorCodes::BAD_ARGUMENTS);
 
+    // Silently reject authentication attempt if the password is empty as if it didn't match.
     if (params.password.empty())
-        return false; // Silently reject authentication attempt if the password is empty as if it didn't match.
+        return false;
 
     SCOPE_EXIT({ closeConnection(); });
 
-    const bool graceful_bind_failure = true;
+    // Will throw on any error, including invalid credentials.
-    const auto rc = openConnection(graceful_bind_failure);
+    openConnection();
 
-    bool result = false;
+    return true;
-
-    switch (rc)
-    {
-        case LDAP_SUCCESS:
-        {
-            result = true;
-            break;
-        }
-
-        case LDAP_INVALID_CREDENTIALS:
-        {
-            result = false;
-            break;
-        }
-
-        default:
-        {
-            result = false;
-            diag(rc);
-            break;
-        }
-    }
-
-    return result;
 }
 
 #else // USE_LDAP
@@ -333,11 +299,6 @@ void LDAPClient::openConnection()
     throw Exception("ClickHouse was built without LDAP support", ErrorCodes::FEATURE_IS_NOT_ENABLED_AT_BUILD_TIME);
 }
 
-int LDAPClient::openConnection(const bool)
-{
-    throw Exception("ClickHouse was built without LDAP support", ErrorCodes::FEATURE_IS_NOT_ENABLED_AT_BUILD_TIME);
-}
-
 void LDAPClient::closeConnection() noexcept
 {
 }

src/Access/LDAPClient.h

@@ -32,7 +32,6 @@ public:
 protected:
     MAYBE_NORETURN void diag(const int rc);
     MAYBE_NORETURN void openConnection();
-    int openConnection(const bool graceful_bind_failure = false);
     void closeConnection() noexcept;
 
 protected:

src/Access/LDAPParams.h

@@ -42,6 +42,7 @@ struct LDAPServerParams
 
     enum class SASLMechanism
     {
+        UNKNOWN,
         SIMPLE
     };
 

src/Access/MultipleAccessStorage.cpp

@@ -425,7 +425,7 @@ UUID MultipleAccessStorage::loginImpl(const String & user_name, const String & p
             throw;
         }
     }
-    throwCannotAuthenticate(user_name);
+    throwNotFound(EntityType::USER, user_name);
 }