LDAP: Disable user in case it has been removed from LDAP directory (#60231)

* Fix login flow

* Align test

* Fix comments

* Improve test

pkg/login/ldap_login.go

@@ -47,6 +47,7 @@ var loginUsingLDAP = func(ctx context.Context, query *models.LoginUserQuery, log
 				ldapLogger.Debug("Failed to disable external user", "err", err)
 			}
 
+			// Return invalid credentials if we couldn't find the user anywhere
 			return true, ldap.ErrInvalidCredentials
 		}
 

pkg/services/multildap/multildap.go

@@ -106,6 +106,8 @@ func (multiples *MultiLDAP) Login(query *models.LoginUserQuery) (
 		return nil, ErrNoLDAPServers
 	}
 
+	ldapSilentErrors := []error{}
+
 	for index, config := range multiples.configs {
 		server := newLDAP(config)
 
@@ -122,12 +124,9 @@ func (multiples *MultiLDAP) Login(query *models.LoginUserQuery) (
 		defer server.Close()
 
 		user, err := server.Login(query)
-		// FIXME
-		if user != nil {
-			return user, nil
-		}
 		if err != nil {
 			if isSilentError(err) {
+				ldapSilentErrors = append(ldapSilentErrors, err)
 				logger.Debug(
 					"unable to login with LDAP - skipping server",
 					"host", config.Host,
@@ -139,10 +138,21 @@ func (multiples *MultiLDAP) Login(query *models.LoginUserQuery) (
 
 			return nil, err
 		}
+
+		if user != nil {
+			return user, nil
+		}
 	}
 
-	// Return invalid credentials if we couldn't find the user anywhere
+	// Return ErrInvalidCredentials in case any of the errors was ErrInvalidCredentials (means that the authentication has failed at least once)
-	return nil, ErrInvalidCredentials
+	for _, ldapErr := range ldapSilentErrors {
+		if errors.Is(ldapErr, ErrInvalidCredentials) {
+			return nil, ErrInvalidCredentials
+		}
+	}
+
+	// Return ErrCouldNotFindUser if all of the configured LDAP servers returned with ErrCouldNotFindUser
+	return nil, ErrCouldNotFindUser
 }
 
 // User attempts to find an user by login/username by searching into all of the configured LDAP servers. Then, if the user is found it returns the user alongisde the server it was found.

pkg/services/multildap/multildap_test.go

@@ -99,6 +99,7 @@ func TestMultiLDAP(t *testing.T) {
 
 		t.Run("Should call underlying LDAP methods", func(t *testing.T) {
 			mock := setup()
+			mock.loginErrReturn = ErrInvalidCredentials
 
 			multi := New([]*ldap.ServerConfig{
 				{}, {},
@@ -109,7 +110,7 @@ func TestMultiLDAP(t *testing.T) {
 			require.Equal(t, 2, mock.loginCalledTimes)
 			require.Equal(t, 2, mock.closeCalledTimes)
 
-			require.Equal(t, ErrInvalidCredentials, err)
+			require.Equal(t, ldap.ErrInvalidCredentials, err)
 
 			teardown()
 		})
@@ -150,7 +151,7 @@ func TestMultiLDAP(t *testing.T) {
 			require.Equal(t, 2, mock.loginCalledTimes)
 			require.Equal(t, 2, mock.closeCalledTimes)
 
-			require.Equal(t, ErrInvalidCredentials, err)
+			require.Equal(t, ErrCouldNotFindUser, err)
 
 			teardown()
 		})