LDAP: nitpicks (#18309)

* LDAP: nitpicks

* Add more tests

* Correct and clarify comment for Login() method

* Rename methods (hail consistency!)

* Uppercases first letter of the logs everywhere

* Moves method definitions around to more appropriate places

Fixes #18295

pkg/services/ldap/ldap.go

@@ -48,8 +48,8 @@ type Server struct {
 // - with the username and password setup in the config
 // - or, anonymously
 func (server *Server) Bind() error {
-	if server.shouldAuthAdmin() {
-		if err := server.AuthAdmin(); err != nil {
+	if server.shouldAdminBind() {
+		if err := server.AdminBind(); err != nil {
 			return err
 		}
 	} else {
@@ -75,7 +75,7 @@ var (
 	ErrCouldNotFindUser = errors.New("Can't find user in LDAP")
 )
 
-// New creates the new LDAP auth
+// New creates the new LDAP connection
 func New(config *ServerConfig) IServer {
 	return &Server{
 		Config: config,
@@ -84,6 +84,7 @@ func New(config *ServerConfig) IServer {
 }
 
 // Dial dials in the LDAP
+// TODO: decrease cyclomatic complexity
 func (server *Server) Dial() error {
 	var err error
 	var certPool *x509.CertPool
@@ -144,16 +145,20 @@ func (server *Server) Close() {
 }
 
 // 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
+// There are several cases -
+// 1. "admin" user
+// Bind the "admin" user (defined in Grafana config file) which has the search privileges
+// in LDAP server, then we search the targeted user through that bind, then the second
+// perform the bind via passed login/password.
+// 2. Single bind
+// // If all the users meant to be used with Grafana have the ability to search in LDAP server
+// then we bind with LDAP server with targeted login/password
+// and then search for the said user in order to retrive all the information about them
+// 3. Unauthenticated bind
+// For some LDAP configurations it is allowed to search the
+// user without login/password binding with LDAP server, in such case
+// we will perform "unauthenticated bind", then search for the
+// targeted user and then perform the bind with passed login/password.
 func (server *Server) Login(query *models.LoginUserQuery) (
 	*models.ExternalUserInfo, error,
 ) {
@@ -161,13 +166,16 @@ func (server *Server) Login(query *models.LoginUserQuery) (
 	var authAndBind bool
 
 	// Check if we can use a search user
-	if server.shouldAuthAdmin() {
-		if err := server.AuthAdmin(); err != nil {
+	if server.shouldAdminBind() {
+		if err := server.AdminBind(); err != nil {
 			return nil, err
 		}
 	} else if server.shouldSingleBind() {
 		authAndBind = true
-		err = server.UserBind(server.singleBindDN(query.Username), query.Password)
+		err = server.UserBind(
+			server.singleBindDN(query.Username),
+			query.Password,
+		)
 		if err != nil {
 			return nil, err
 		}
@@ -206,38 +214,24 @@ func (server *Server) Login(query *models.LoginUserQuery) (
 	return user, nil
 }
 
+// shouldAdminBind checks if we should use
+// admin username & password for LDAP bind
+func (server *Server) shouldAdminBind() bool {
+	return server.Config.BindPassword != ""
+}
+
+// singleBindDN combines the bind with the username
+// in order to get the proper path
 func (server *Server) singleBindDN(username string) string {
 	return fmt.Sprintf(server.Config.BindDN, username)
 }
 
+// shouldSingleBind checks if we can use "single bind" approach
 func (server *Server) shouldSingleBind() bool {
 	return strings.Contains(server.Config.BindDN, "%s")
 }
 
-// getUsersIteration is a helper function for Users() method.
-// It divides the users by equal parts for the anticipated requests
-func getUsersIteration(logins []string, fn func(int, int) error) error {
-	lenLogins := len(logins)
-	iterations := int(
-		math.Ceil(
-			float64(lenLogins) / float64(UsersMaxRequest),
-		),
-	)
-
-	for i := 1; i < iterations+1; i++ {
-		previous := float64(UsersMaxRequest * (i - 1))
-		current := math.Min(float64(i*UsersMaxRequest), float64(lenLogins))
-
-		err := fn(int(previous), int(current))
-		if err != nil {
-			return err
-		}
-	}
-
-	return nil
-}
-
-// Users gets LDAP users
+// Users gets LDAP users by logins
 func (server *Server) Users(logins []string) (
 	[]*models.ExternalUserInfo,
 	error,
@@ -273,6 +267,29 @@ func (server *Server) Users(logins []string) (
 	return serializedUsers, nil
 }
 
+// getUsersIteration is a helper function for Users() method.
+// It divides the users by equal parts for the anticipated requests
+func getUsersIteration(logins []string, fn func(int, int) error) error {
+	lenLogins := len(logins)
+	iterations := int(
+		math.Ceil(
+			float64(lenLogins) / float64(UsersMaxRequest),
+		),
+	)
+
+	for i := 1; i < iterations+1; i++ {
+		previous := float64(UsersMaxRequest * (i - 1))
+		current := math.Min(float64(i*UsersMaxRequest), float64(lenLogins))
+
+		err := fn(int(previous), int(current))
+		if err != nil {
+			return err
+		}
+	}
+
+	return nil
+}
+
 // users is helper method for the Users()
 func (server *Server) users(logins []string) (
 	[]*ldap.Entry,
@@ -304,7 +321,7 @@ func (server *Server) users(logins []string) (
 func (server *Server) validateGrafanaUser(user *models.ExternalUserInfo) error {
 	if len(server.Config.Groups) > 0 && len(user.OrgRoles) < 1 {
 		server.log.Error(
-			"user does not belong in any of the specified LDAP groups",
+			"User does not belong in any of the specified LDAP groups",
 			"username", user.Login,
 			"groups", user.Groups,
 		)
@@ -397,18 +414,12 @@ func (server *Server) buildGrafanaUser(user *ldap.Entry) (*models.ExternalUserIn
 	return extUser, nil
 }
 
-// shouldAuthAdmin checks if we should use
-// admin username & password for LDAP bind
-func (server *Server) shouldAuthAdmin() bool {
-	return server.Config.BindPassword != ""
-}
-
-// UserBind authenticates the connection with the LDAP server
+// UserBind binds the user with the LDAP server
 func (server *Server) UserBind(username, password string) error {
 	err := server.userBind(username, password)
 	if err != nil {
 		server.log.Error(
-			fmt.Sprintf("Cannot authentificate user %s in LDAP", username),
+			fmt.Sprintf("Cannot bind user %s with LDAP", username),
 			"error",
 			err,
 		)
@@ -418,8 +429,8 @@ func (server *Server) UserBind(username, password string) error {
 	return nil
 }
 
-// AuthAdmin authentificates LDAP admin user
-func (server *Server) AuthAdmin() error {
+// AdminBind binds "admin" user with LDAP
+func (server *Server) AdminBind() error {
 	err := server.userBind(server.Config.BindDN, server.Config.BindPassword)
 	if err != nil {
 		server.log.Error(
@@ -433,7 +444,7 @@ func (server *Server) AuthAdmin() error {
 	return nil
 }
 
-// userBind authenticates the connection with the LDAP server
+// userBind binds the user with the LDAP server
 func (server *Server) userBind(path, password string) error {
 	err := server.Connection.Bind(path, password)
 	if err != nil {
@@ -448,7 +459,8 @@ func (server *Server) userBind(path, password string) error {
 	return nil
 }
 
-// requestMemberOf use this function when POSIX LDAP schema does not support memberOf, so it manually search the groups
+// requestMemberOf use this function when POSIX LDAP
+// schema does not support memberOf, so it manually search the groups
 func (server *Server) requestMemberOf(entry *ldap.Entry) ([]string, error) {
 	var memberOf []string
 	var config = server.Config

pkg/services/ldap/ldap_private_test.go

@@ -182,7 +182,7 @@ func TestLDAPPrivateMethods(t *testing.T) {
 		})
 	})
 
-	Convey("shouldAuthAdmin()", t, func() {
+	Convey("shouldAdminBind()", t, func() {
 		Convey("it should require admin userBind", func() {
 			server := &Server{
 				Config: &ServerConfig{
@@ -190,7 +190,7 @@ func TestLDAPPrivateMethods(t *testing.T) {
 				},
 			}
 
-			result := server.shouldAuthAdmin()
+			result := server.shouldAdminBind()
 			So(result, ShouldBeTrue)
 		})
 
@@ -201,9 +201,46 @@ func TestLDAPPrivateMethods(t *testing.T) {
 				},
 			}
 
-			result := server.shouldAuthAdmin()
+			result := server.shouldAdminBind()
 			So(result, ShouldBeFalse)
 		})
 	})
 
+	Convey("shouldSingleBind()", t, func() {
+		Convey("it should allow single bind", func() {
+			server := &Server{
+				Config: &ServerConfig{
+					BindDN: "cn=%s,dc=grafana,dc=org",
+				},
+			}
+
+			result := server.shouldSingleBind()
+			So(result, ShouldBeTrue)
+		})
+
+		Convey("it should not allow single bind", func() {
+			server := &Server{
+				Config: &ServerConfig{
+					BindDN: "cn=admin,dc=grafana,dc=org",
+				},
+			}
+
+			result := server.shouldSingleBind()
+			So(result, ShouldBeFalse)
+		})
+	})
+
+	Convey("singleBindDN()", t, func() {
+		Convey("it should allow single bind", func() {
+			server := &Server{
+				Config: &ServerConfig{
+					BindDN: "cn=%s,dc=grafana,dc=org",
+				},
+			}
+
+			result := server.singleBindDN("test")
+			So(result, ShouldEqual, "cn=test,dc=grafana,dc=org")
+		})
+	})
+
 }

pkg/services/ldap/ldap_test.go

@@ -146,7 +146,7 @@ func TestPublicAPI(t *testing.T) {
 		})
 	})
 
-	Convey("AuthAdmin()", t, func() {
+	Convey("AdminBind()", t, func() {
 		Convey("Should use admin DN and password", func() {
 			connection := &MockConnection{}
 			var actualUsername, actualPassword string
@@ -166,7 +166,7 @@ func TestPublicAPI(t *testing.T) {
 				},
 			}
 
-			err := server.AuthAdmin()
+			err := server.AdminBind()
 
 			So(err, ShouldBeNil)
 			So(actualUsername, ShouldEqual, dn)
@@ -193,7 +193,7 @@ func TestPublicAPI(t *testing.T) {
 				log: log.New("test-logger"),
 			}
 
-			err := server.AuthAdmin()
+			err := server.AdminBind()
 			So(err, ShouldEqual, expected)
 		})
 	})