LDAP: finishing touches (#17945)

* LDAP:Docs: `active_sync_enabled` setting

Mention `active_sync_enabled` setting and enable it by default

* LDAP: move "disableExternalUser" method

Idea behind new design of the LDAP module is to minimise conflation
between other parts of the system, so it would decoupled as much as
possible from stuff like database, HTTP transport and etc.

Following "Do One Thing and Do It Well" Unix philosophy principal, other things
could be better fitted on the consumer side of things.

Which what this commit trying to archive

* LDAP: correct user/admin binding

The second binding was not happening, so if the admin login/password
in LDAP configuration was correct, anyone could had login as anyone using
incorrect password

conf/defaults.ini

@@ -413,7 +413,7 @@ allow_sign_up = true
 # LDAP backround sync (Enterprise only)
 # At 1 am every day
 sync_cron = "0 0 1 * * *"
-active_sync_enabled = false
+active_sync_enabled = true
 
 #################################### SMTP / Emailing #####################
 [smtp]

conf/sample.ini

@@ -378,7 +378,7 @@
 # LDAP backround sync (Enterprise only)
 # At 1 am every day
 ;sync_cron = "0 0 1 * * *"
-;active_sync_enabled = false
+;active_sync_enabled = true
 
 #################################### SMTP / Emailing ##########################
 [smtp]

docs/sources/auth/enhanced_ldap.md

@@ -44,7 +44,7 @@ a user as member of a team and it will not be removed when the user signs in. Th
 
 ## Active LDAP Synchronization
 
->  Only available in Grafana Enterprise v6.3+
+> Only available in Grafana Enterprise v6.3+
 
 In the open source version of Grafana, user data from LDAP will be synchronized only during the login process when authenticating using LDAP.
 
@@ -60,8 +60,11 @@ With this feature you can configure Grafana to actively sync users with LDAP ser
 # @weekly                | Run once a week, midnight between Sat/Sun  | 0 0 0 * * 0
 # @daily (or @midnight)  | Run once a day, midnight                   | 0 0 0 * * *
 # @hourly                | Run once an hour, beginning of hour        | 0 0 * * * *
-sync_cron = "@hourly"
+sync_cron = "0 0 1 * * *" # This is default value (At 1 am every day)
 # This cron expression format uses 6 space-separated fields (including seconds), for example
 # sync_cron = "* */10 * * * *"
 # This will run the LDAP Synchronization every 10th minute, which is also the minimal interval between the grafana sync times i.e. you cannot set it for every 9th minute
+
+# You can also disable active LDAP synchronization
+active_sync_enabled = true # enabled by default
 ```

pkg/login/ldap_login.go

@@ -2,7 +2,9 @@ package login
 
 import (
 	"github.com/grafana/grafana/pkg/bus"
+	"github.com/grafana/grafana/pkg/infra/log"
 	"github.com/grafana/grafana/pkg/models"
+	"github.com/grafana/grafana/pkg/services/ldap"
 	"github.com/grafana/grafana/pkg/services/multildap"
 	"github.com/grafana/grafana/pkg/setting"
 	"github.com/grafana/grafana/pkg/util/errutil"
@@ -17,6 +19,9 @@ var isLDAPEnabled = multildap.IsEnabled
 // newLDAP creates multiple LDAP instance
 var newLDAP = multildap.New
 
+// logger for the LDAP auth
+var logger = log.New("login.ldap")
+
 // loginUsingLDAP logs in user using LDAP. It returns whether LDAP is enabled and optional error and query arg will be
 // populated with the logged in user if successful.
 var loginUsingLDAP = func(query *models.LoginUserQuery) (bool, error) {
@@ -33,6 +38,13 @@ var loginUsingLDAP = func(query *models.LoginUserQuery) (bool, error) {
 
 	externalUser, err := newLDAP(config.Servers).Login(query)
 	if err != nil {
+		if err == ldap.ErrCouldNotFindUser {
+			// Ignore the error since user might not be present anyway
+			disableExternalUser(query.Username)
+
+			return true, ldap.ErrInvalidCredentials
+		}
+
 		return true, err
 	}
 
@@ -48,3 +60,43 @@ var loginUsingLDAP = func(query *models.LoginUserQuery) (bool, error) {
 
 	return true, nil
 }
+
+// disableExternalUser marks external user as disabled in Grafana db
+func disableExternalUser(username string) error {
+	// Check if external user exist in Grafana
+	userQuery := &models.GetExternalUserInfoByLoginQuery{
+		LoginOrEmail: username,
+	}
+
+	if err := bus.Dispatch(userQuery); err != nil {
+		return err
+	}
+
+	userInfo := userQuery.Result
+	if !userInfo.IsDisabled {
+
+		logger.Debug(
+			"Disabling external user",
+			"user",
+			userQuery.Result.Login,
+		)
+
+		// Mark user as disabled in grafana db
+		disableUserCmd := &models.DisableUserCommand{
+			UserId:     userQuery.Result.UserId,
+			IsDisabled: true,
+		}
+
+		if err := bus.Dispatch(disableUserCmd); err != nil {
+			logger.Debug(
+				"Error disabling external user",
+				"user",
+				userQuery.Result.Login,
+				"message",
+				err.Error(),
+			)
+			return err
+		}
+	}
+	return nil
+}

pkg/services/ldap/ldap.go

@@ -12,7 +12,6 @@ import (
 	"github.com/davecgh/go-spew/spew"
 	"gopkg.in/ldap.v3"
 
-	"github.com/grafana/grafana/pkg/bus"
 	"github.com/grafana/grafana/pkg/infra/log"
 	"github.com/grafana/grafana/pkg/models"
 )
@@ -53,6 +52,9 @@ var (
 
 	// ErrInvalidCredentials is returned if username and password do not match
 	ErrInvalidCredentials = errors.New("Invalid Username or Password")
+
+	// ErrCouldNotFindUser is returned when username hasn't been found (not username+password)
+	ErrCouldNotFindUser = errors.New("Can't find user in LDAP")
 )
 
 // New creates the new LDAP auth
@@ -123,14 +125,35 @@ func (server *Server) Close() {
 	server.Connection.Close()
 }
 
-// Login user by searching and serializing it
+// Login the user.
+// There is several cases -
+// 1. First we check if we need to authenticate the admin user.
+// That user should have search privileges.
+// 2. For some configurations it is allowed to search the
+// user without any authenfication, in such case we
+// perform "unauthenticated bind".
+// --
+// After either first or second step is done we find the user DN
+// by its username, after that, we then combine it with user password and
+// then try to authentificate that user
 func (server *Server) Login(query *models.LoginUserQuery) (
 	*models.ExternalUserInfo, error,
 ) {
-	// Authentication
-	err := server.Auth(query.Username, query.Password)
-	if err != nil {
-		return nil, err
+	var err error
+
+	// Do we need to authenticate the "admin" user first?
+	// Admin user should have access for the user search in LDAP server
+	if server.shouldAuthAdmin() {
+		if err := server.AuthAdmin(); err != nil {
+			return nil, err
+		}
+
+		// Or if anyone can perform the search in LDAP?
+	} else {
+		err := server.Connection.UnauthenticatedBind(server.Config.BindDN)
+		if err != nil {
+			return nil, err
+		}
 	}
 
 	// Find user entry & attributes
@@ -142,8 +165,7 @@ func (server *Server) Login(query *models.LoginUserQuery) (
 	// If we couldn't find the user -
 	// we should show incorrect credentials err
 	if len(users) == 0 {
-		server.disableExternalUser(query.Username)
-		return nil, ErrInvalidCredentials
+		return nil, ErrCouldNotFindUser
 	}
 
 	user := users[0]
@@ -151,6 +173,12 @@ func (server *Server) Login(query *models.LoginUserQuery) (
 		return nil, err
 	}
 
+	// Authenticate user
+	err = server.Auth(user.AuthId, query.Password)
+	if err != nil {
+		return nil, err
+	}
+
 	return user, nil
 }
 
@@ -252,34 +280,6 @@ func (server *Server) validateGrafanaUser(user *models.ExternalUserInfo) error {
 	return nil
 }
 
-// disableExternalUser marks external user as disabled in Grafana db
-func (server *Server) disableExternalUser(username string) error {
-	// Check if external user exist in Grafana
-	userQuery := &models.GetExternalUserInfoByLoginQuery{
-		LoginOrEmail: username,
-	}
-
-	if err := bus.Dispatch(userQuery); err != nil {
-		return err
-	}
-
-	userInfo := userQuery.Result
-	if !userInfo.IsDisabled {
-		server.log.Debug("Disabling external user", "user", userQuery.Result.Login)
-		// Mark user as disabled in grafana db
-		disableUserCmd := &models.DisableUserCommand{
-			UserId:     userQuery.Result.UserId,
-			IsDisabled: true,
-		}
-
-		if err := bus.Dispatch(disableUserCmd); err != nil {
-			server.log.Debug("Error disabling external user", "user", userQuery.Result.Login, "message", err.Error())
-			return err
-		}
-	}
-	return nil
-}
-
 // getSearchRequest returns LDAP search request for users
 func (server *Server) getSearchRequest(
 	base string,
@@ -360,32 +360,46 @@ func (server *Server) buildGrafanaUser(user *ldap.Entry) (*models.ExternalUserIn
 	return extUser, nil
 }
 
-// shouldBindAdmin checks if we should use
+// shouldAuthAdmin checks if we should use
 // admin username & password for LDAP bind
-func (server *Server) shouldBindAdmin() bool {
+func (server *Server) shouldAuthAdmin() bool {
 	return server.Config.BindPassword != ""
 }
 
-// Auth authentificates user in LDAP.
-// It might not use passed password and username,
-// since they can be overwritten with admin config values -
-// see "bind_dn" and "bind_password" options in LDAP config
+// Auth authentificates user in LDAP
 func (server *Server) Auth(username, password string) error {
-	path := server.Config.BindDN
-
-	if server.shouldBindAdmin() {
-		password = server.Config.BindPassword
-	} else {
-		path = fmt.Sprintf(path, username)
+	err := server.auth(username, password)
+	if err != nil {
+		server.log.Error(
+			fmt.Sprintf("Cannot authentificate user %s in LDAP", username),
+			"error",
+			err,
+		)
+		return err
 	}
 
-	bindFn := func() error {
-		return server.Connection.Bind(path, password)
+	return nil
+}
+
+// AuthAdmin authentificates LDAP admin user
+func (server *Server) AuthAdmin() error {
+	err := server.auth(server.Config.BindDN, server.Config.BindPassword)
+	if err != nil {
+		server.log.Error(
+			"Cannot authentificate admin user in LDAP",
+			"error",
+			err,
+		)
+		return err
 	}
 
-	if err := bindFn(); err != nil {
-		server.log.Error("Cannot authentificate in LDAP", "err", err)
+	return nil
+}
 
+// auth is helper for several types of LDAP authentification
+func (server *Server) auth(path, password string) error {
+	err := server.Connection.Bind(path, password)
+	if err != nil {
 		if ldapErr, ok := err.(*ldap.Error); ok {
 			if ldapErr.ResultCode == 49 {
 				return ErrInvalidCredentials

pkg/services/ldap/ldap_login_test.go

@@ -4,10 +4,11 @@ import (
 	"errors"
 	"testing"
 
-	"github.com/grafana/grafana/pkg/infra/log"
-	"github.com/grafana/grafana/pkg/models"
 	. "github.com/smartystreets/goconvey/convey"
 	"gopkg.in/ldap.v3"
+
+	"github.com/grafana/grafana/pkg/infra/log"
+	"github.com/grafana/grafana/pkg/models"
 )
 
 func TestLDAPLogin(t *testing.T) {
@@ -24,7 +25,7 @@ func TestLDAPLogin(t *testing.T) {
 			result := ldap.SearchResult{Entries: []*ldap.Entry{&entry}}
 			connection.setSearchResult(&result)
 
-			connection.bindProvider = func(username, password string) error {
+			connection.BindProvider = func(username, password string) error {
 				return &ldap.Error{
 					ResultCode: 49,
 				}
@@ -42,12 +43,12 @@ func TestLDAPLogin(t *testing.T) {
 			So(err, ShouldEqual, ErrInvalidCredentials)
 		})
 
-		Convey("Returns an error when search hasn't find anything", func() {
+		Convey("Returns an error when search didn't find anything", func() {
 			connection := &MockConnection{}
 			result := ldap.SearchResult{Entries: []*ldap.Entry{}}
 			connection.setSearchResult(&result)
 
-			connection.bindProvider = func(username, password string) error {
+			connection.BindProvider = func(username, password string) error {
 				return nil
 			}
 			server := &Server{
@@ -60,7 +61,7 @@ func TestLDAPLogin(t *testing.T) {
 
 			_, err := server.Login(defaultLogin)
 
-			So(err, ShouldEqual, ErrInvalidCredentials)
+			So(err, ShouldEqual, ErrCouldNotFindUser)
 		})
 
 		Convey("When search returns an error", func() {
@@ -68,7 +69,7 @@ func TestLDAPLogin(t *testing.T) {
 			expected := errors.New("Killa-gorilla")
 			connection.setSearchError(expected)
 
-			connection.bindProvider = func(username, password string) error {
+			connection.BindProvider = func(username, password string) error {
 				return nil
 			}
 			server := &Server{
@@ -98,7 +99,7 @@ func TestLDAPLogin(t *testing.T) {
 			result := ldap.SearchResult{Entries: []*ldap.Entry{&entry}}
 			connection.setSearchResult(&result)
 
-			connection.bindProvider = func(username, password string) error {
+			connection.BindProvider = func(username, password string) error {
 				return nil
 			}
 			server := &Server{
@@ -119,5 +120,83 @@ func TestLDAPLogin(t *testing.T) {
 			So(err, ShouldBeNil)
 			So(resp.Login, ShouldEqual, "markelog")
 		})
+
+		Convey("Should perform unauthentificate bind without admin", func() {
+			connection := &MockConnection{}
+			entry := ldap.Entry{
+				DN: "test",
+			}
+			result := ldap.SearchResult{Entries: []*ldap.Entry{&entry}}
+			connection.setSearchResult(&result)
+
+			connection.UnauthenticatedBindProvider = func() error {
+				return nil
+			}
+			server := &Server{
+				Config: &ServerConfig{
+					SearchBaseDNs: []string{"BaseDNHere"},
+				},
+				Connection: connection,
+				log:        log.New("test-logger"),
+			}
+
+			user, err := server.Login(defaultLogin)
+
+			So(err, ShouldBeNil)
+			So(user.AuthId, ShouldEqual, "test")
+			So(connection.UnauthenticatedBindCalled, ShouldBeTrue)
+		})
+
+		Convey("Should perform authentificate binds", func() {
+			connection := &MockConnection{}
+			entry := ldap.Entry{
+				DN: "test",
+			}
+			result := ldap.SearchResult{Entries: []*ldap.Entry{&entry}}
+			connection.setSearchResult(&result)
+
+			adminUsername := ""
+			adminPassword := ""
+			username := ""
+			password := ""
+
+			i := 0
+			connection.BindProvider = func(name, pass string) error {
+				i++
+				if i == 1 {
+					adminUsername = name
+					adminPassword = pass
+				}
+
+				if i == 2 {
+					username = name
+					password = pass
+				}
+
+				return nil
+			}
+			server := &Server{
+				Config: &ServerConfig{
+					BindDN:        "killa",
+					BindPassword:  "gorilla",
+					SearchBaseDNs: []string{"BaseDNHere"},
+				},
+				Connection: connection,
+				log:        log.New("test-logger"),
+			}
+
+			user, err := server.Login(defaultLogin)
+
+			So(err, ShouldBeNil)
+
+			So(user.AuthId, ShouldEqual, "test")
+			So(connection.BindCalled, ShouldBeTrue)
+
+			So(adminUsername, ShouldEqual, "killa")
+			So(adminPassword, ShouldEqual, "gorilla")
+
+			So(username, ShouldEqual, "test")
+			So(password, ShouldEqual, "pwd")
+		})
 	})
 }

pkg/services/ldap/ldap_private_test.go

@@ -143,4 +143,29 @@ func TestLDAPPrivateMethods(t *testing.T) {
 			So(result, ShouldBeNil)
 		})
 	})
+
+	Convey("shouldAuthAdmin()", t, func() {
+		Convey("it should require admin auth", func() {
+			server := &Server{
+				Config: &ServerConfig{
+					BindPassword: "test",
+				},
+			}
+
+			result := server.shouldAuthAdmin()
+			So(result, ShouldBeTrue)
+		})
+
+		Convey("it should not require admin auth", func() {
+			server := &Server{
+				Config: &ServerConfig{
+					BindPassword: "",
+				},
+			}
+
+			result := server.shouldAuthAdmin()
+			So(result, ShouldBeFalse)
+		})
+	})
+
 }

pkg/services/ldap/ldap_test.go

@@ -103,10 +103,10 @@ func TestPublicAPI(t *testing.T) {
 	})
 
 	Convey("Auth()", t, func() {
-		Convey("Should ignore passsed username and password", func() {
+		Convey("Should use provided DN and password", func() {
 			connection := &MockConnection{}
 			var actualUsername, actualPassword string
-			connection.bindProvider = func(username, password string) error {
+			connection.BindProvider = func(username, password string) error {
 				actualUsername = username
 				actualPassword = password
 				return nil
@@ -114,33 +114,62 @@ func TestPublicAPI(t *testing.T) {
 			server := &Server{
 				Connection: connection,
 				Config: &ServerConfig{
-					BindDN:       "cn=admin,dc=grafana,dc=org",
-					BindPassword: "bindpwd",
+					BindDN: "cn=admin,dc=grafana,dc=org",
 				},
 			}
-			err := server.Auth("user", "pwd")
+
+			dn := "cn=user,ou=users,dc=grafana,dc=org"
+			err := server.Auth(dn, "pwd")
+
 			So(err, ShouldBeNil)
-			So(actualUsername, ShouldEqual, "cn=admin,dc=grafana,dc=org")
-			So(actualPassword, ShouldEqual, "bindpwd")
+			So(actualUsername, ShouldEqual, dn)
+			So(actualPassword, ShouldEqual, "pwd")
 		})
 
-		Convey("Given bind dn configured", func() {
+		Convey("Should handle an error", func() {
+			connection := &MockConnection{}
+			expected := &ldap.Error{
+				ResultCode: uint16(25),
+			}
+			connection.BindProvider = func(username, password string) error {
+				return expected
+			}
+			server := &Server{
+				Connection: connection,
+				Config: &ServerConfig{
+					BindDN: "cn=%s,ou=users,dc=grafana,dc=org",
+				},
+				log: log.New("test-logger"),
+			}
+			err := server.Auth("user", "pwd")
+			So(err, ShouldEqual, expected)
+		})
+	})
+
+	Convey("AuthAdmin()", t, func() {
+		Convey("Should use admin DN and password", func() {
 			connection := &MockConnection{}
 			var actualUsername, actualPassword string
-			connection.bindProvider = func(username, password string) error {
+			connection.BindProvider = func(username, password string) error {
 				actualUsername = username
 				actualPassword = password
 				return nil
 			}
+
+			dn := "cn=admin,dc=grafana,dc=org"
+
 			server := &Server{
 				Connection: connection,
 				Config: &ServerConfig{
-					BindDN: "cn=%s,o=users,dc=grafana,dc=org",
+					BindPassword: "pwd",
+					BindDN:       dn,
 				},
 			}
-			err := server.Auth("user", "pwd")
+
+			err := server.AuthAdmin()
+
 			So(err, ShouldBeNil)
-			So(actualUsername, ShouldEqual, "cn=user,o=users,dc=grafana,dc=org")
+			So(actualUsername, ShouldEqual, dn)
 			So(actualPassword, ShouldEqual, "pwd")
 		})
 
@@ -149,17 +178,22 @@ func TestPublicAPI(t *testing.T) {
 			expected := &ldap.Error{
 				ResultCode: uint16(25),
 			}
-			connection.bindProvider = func(username, password string) error {
+			connection.BindProvider = func(username, password string) error {
 				return expected
 			}
+
+			dn := "cn=admin,dc=grafana,dc=org"
+
 			server := &Server{
 				Connection: connection,
 				Config: &ServerConfig{
-					BindDN: "cn=%s,o=users,dc=grafana,dc=org",
+					BindPassword: "pwd",
+					BindDN:       dn,
 				},
 				log: log.New("test-logger"),
 			}
-			err := server.Auth("user", "pwd")
+
+			err := server.AuthAdmin()
 			So(err, ShouldEqual, expected)
 		})
 	})

pkg/services/ldap/testing.go

@@ -19,14 +19,19 @@ type MockConnection struct {
 	DelParams *ldap.DelRequest
 	DelCalled bool
 
-	bindProvider                func(username, password string) error
-	unauthenticatedBindProvider func(username string) error
+	UnauthenticatedBindCalled bool
+	BindCalled                bool
+
+	BindProvider                func(username, password string) error
+	UnauthenticatedBindProvider func() error
 }
 
 // Bind mocks Bind connection function
 func (c *MockConnection) Bind(username, password string) error {
-	if c.bindProvider != nil {
-		return c.bindProvider(username, password)
+	c.BindCalled = true
+
+	if c.BindProvider != nil {
+		return c.BindProvider(username, password)
 	}
 
 	return nil
@@ -34,8 +39,10 @@ func (c *MockConnection) Bind(username, password string) error {
 
 // UnauthenticatedBind mocks UnauthenticatedBind connection function
 func (c *MockConnection) UnauthenticatedBind(username string) error {
-	if c.unauthenticatedBindProvider != nil {
-		return c.unauthenticatedBindProvider(username)
+	c.UnauthenticatedBindCalled = true
+
+	if c.UnauthenticatedBindProvider != nil {
+		return c.UnauthenticatedBindProvider()
 	}
 
 	return nil