From 279249ef56dcf599f5c61445b3f0358e5db70f01 Mon Sep 17 00:00:00 2001
From: gotjosh <josue@grafana.com>
Date: Mon, 23 Sep 2019 13:34:05 +0100
Subject: [PATCH] Multi-LDAP: Do not fail-fast on invalid credentials (#19261)

* Multi-LDAP: Do not fail-fast on invalid credentials

When configuring LDAP authentication, it is very common to have multiple
servers configured. When using user bind (authenticating with LDAP using
the same credentials as the user authenticating to Grafana) we don't
expect all the users to be on all LDAP servers.

Because of this use-case, we should not fail-fast when authenticating on
multiple LDAP server configurations. Instead, we should continue to try
the credentials with the next LDAP server configured.

Fixes #19066
---
 pkg/services/multildap/multildap.go      | 34 ++++++++++++++++++++----
 pkg/services/multildap/multildap_test.go | 19 +++++++++++++
 2 files changed, 48 insertions(+), 5 deletions(-)

diff --git a/pkg/services/multildap/multildap.go b/pkg/services/multildap/multildap.go
index b79a21e0417..fa84c852344 100644
--- a/pkg/services/multildap/multildap.go
+++ b/pkg/services/multildap/multildap.go
@@ -3,10 +3,14 @@ package multildap
 import (
 	"errors"
 
+	"github.com/grafana/grafana/pkg/infra/log"
 	"github.com/grafana/grafana/pkg/models"
 	"github.com/grafana/grafana/pkg/services/ldap"
 )
 
+// logger to log
+var logger = log.New("ldap")
+
 // GetConfig gets LDAP config
 var GetConfig = ldap.GetConfig
 
@@ -119,12 +123,18 @@ func (multiples *MultiLDAP) Login(query *models.LoginUserQuery) (
 			return user, nil
 		}
 
-		// Continue if we couldn't find the user
-		if err == ErrCouldNotFindUser {
-			continue
-		}
-
 		if err != nil {
+
+			if isSilentError(err) {
+				logger.Debug(
+					"unable to login with LDAP - skipping server",
+					"host", config.Host,
+					"port", config.Port,
+					"error", err,
+				)
+				continue
+			}
+
 			return nil, err
 		}
 	}
@@ -204,3 +214,17 @@ func (multiples *MultiLDAP) Users(logins []string) (
 
 	return result, nil
 }
+
+// isSilentError evaluates an error and tells whenever we should fail the LDAP request
+// immediately or if we should continue into other LDAP servers
+func isSilentError(err error) bool {
+	continueErrs := []error{ErrInvalidCredentials, ErrCouldNotFindUser}
+
+	for _, cerr := range continueErrs {
+		if err == cerr {
+			return true
+		}
+	}
+
+	return false
+}
diff --git a/pkg/services/multildap/multildap_test.go b/pkg/services/multildap/multildap_test.go
index 6554bdbaabf..81fcdca732a 100644
--- a/pkg/services/multildap/multildap_test.go
+++ b/pkg/services/multildap/multildap_test.go
@@ -152,6 +152,25 @@ func TestMultiLDAP(t *testing.T) {
 				teardown()
 			})
 
+			Convey("Should still try to auth with the second server after receiving an invalid credentials error from the first", func() {
+				mock := setup()
+
+				mock.loginErrReturn = ErrInvalidCredentials
+
+				multi := New([]*ldap.ServerConfig{
+					{}, {},
+				})
+				_, err := multi.Login(&models.LoginUserQuery{})
+
+				So(mock.dialCalledTimes, ShouldEqual, 2)
+				So(mock.loginCalledTimes, ShouldEqual, 2)
+				So(mock.closeCalledTimes, ShouldEqual, 2)
+
+				So(err, ShouldEqual, ErrInvalidCredentials)
+
+				teardown()
+			})
+
 			Convey("Should return unknown error", func() {
 				mock := setup()