LDAP: Search all DNs for users

pkg/services/ldap/ldap.go

@@ -12,9 +12,10 @@ import (
 	"strings"
 
 	"github.com/davecgh/go-spew/spew"
+	"gopkg.in/ldap.v3"
+
 	"github.com/grafana/grafana/pkg/infra/log"
 	"github.com/grafana/grafana/pkg/models"
-	"gopkg.in/ldap.v3"
 )
 
 // IConnection is interface for LDAP connection manipulation
@@ -252,16 +253,11 @@ func (server *Server) Users(logins []string) (
 	[]*models.ExternalUserInfo,
 	error,
 ) {
-	var users []*ldap.Entry
+	var users [][]*ldap.Entry
 	err := getUsersIteration(logins, func(previous, current int) error {
-		entries, err := server.users(logins[previous:current])
-		if err != nil {
-			return err
-		}
-
-		users = append(users, entries...)
-
-		return nil
+		var err error
+		users, err = server.users(logins[previous:current])
+		return err
 	})
 	if err != nil {
 		return nil, err
@@ -308,13 +304,15 @@ func getUsersIteration(logins []string, fn func(int, int) error) error {
 
 // users is helper method for the Users()
 func (server *Server) users(logins []string) (
-	[]*ldap.Entry,
+	[][]*ldap.Entry,
 	error,
 ) {
 	var result *ldap.SearchResult
 	var Config = server.Config
 	var err error
 
+	var entries = make([][]*ldap.Entry, 0, len(Config.SearchBaseDNs))
+
 	for _, base := range Config.SearchBaseDNs {
 		result, err = server.Connection.Search(
 			server.getSearchRequest(base, logins),
@@ -324,11 +322,11 @@ func (server *Server) users(logins []string) (
 		}
 
 		if len(result.Entries) > 0 {
-			break
+			entries = append(entries, result.Entries)
 		}
 	}
 
-	return result.Entries, nil
+	return entries, nil
 }
 
 // validateGrafanaUser validates user access.
@@ -557,17 +555,26 @@ func (server *Server) requestMemberOf(entry *ldap.Entry) ([]string, error) {
 // serializeUsers serializes the users
 // from LDAP result to ExternalInfo struct
 func (server *Server) serializeUsers(
-	entries []*ldap.Entry,
+	entries [][]*ldap.Entry,
 ) ([]*models.ExternalUserInfo, error) {
 	var serialized []*models.ExternalUserInfo
+	var users = map[string]struct{}{}
 
-	for _, user := range entries {
-		extUser, err := server.buildGrafanaUser(user)
-		if err != nil {
-			return nil, err
-		}
+	for _, dn := range entries {
+		for _, user := range dn {
+			extUser, err := server.buildGrafanaUser(user)
+			if err != nil {
+				return nil, err
+			}
+
+			if _, exists := users[extUser.Login]; exists {
+				// ignore duplicates
+				continue
+			}
+			users[extUser.Login] = struct{}{}
 
-		serialized = append(serialized, extUser)
+			serialized = append(serialized, extUser)
+		}
 	}
 
 	return serialized, nil

pkg/services/ldap/ldap_helpers_test.go

@@ -1,191 +1,141 @@
 package ldap
 
 import (
+	"fmt"
 	"testing"
 
-	. "github.com/smartystreets/goconvey/convey"
+	"github.com/stretchr/testify/assert"
 	"gopkg.in/ldap.v3"
 )
 
-func TestLDAPHelpers(t *testing.T) {
-	Convey("isMemberOf()", t, func() {
-		Convey("Wildcard", func() {
-			result := isMemberOf([]string{}, "*")
-			So(result, ShouldBeTrue)
+func TestIsMemberOf(t *testing.T) {
+	tests := []struct {
+		memberOf []string
+		group    string
+		expected bool
+	}{
+		{memberOf: []string{}, group: "*", expected: true},
+		{memberOf: []string{"one", "Two", "three"}, group: "two", expected: true},
+		{memberOf: []string{"one", "Two", "three"}, group: "twos", expected: false},
+	}
+
+	for _, tc := range tests {
+		t.Run(fmt.Sprintf("isMemberOf(%v, \"%s\") = %v", tc.memberOf, tc.group, tc.expected), func(t *testing.T) {
+			assert.Equal(t, tc.expected, isMemberOf(tc.memberOf, tc.group))
 		})
+	}
+}
 
-		Convey("Should find one", func() {
-			result := isMemberOf([]string{"one", "Two", "three"}, "two")
-			So(result, ShouldBeTrue)
-		})
+func TestGetUsersIteration(t *testing.T) {
+	const pageSize = UsersMaxRequest
+	iterations := map[int]int{
+		0:    0,
+		400:  1,
+		600:  2,
+		1500: 3,
+	}
 
-		Convey("Should not find one", func() {
-			result := isMemberOf([]string{"one", "Two", "three"}, "twos")
-			So(result, ShouldBeFalse)
-		})
-	})
+	for userCount, expectedIterations := range iterations {
+		t.Run(fmt.Sprintf("getUserIteration iterates %d times for %d users", expectedIterations, userCount), func(t *testing.T) {
+			logins := make([]string, userCount)
 
-	Convey("getUsersIteration()", t, func() {
-		Convey("it should execute twice for 600 users", func() {
-			logins := make([]string, 600)
 			i := 0
+			_ = getUsersIteration(logins, func(first int, last int) error {
+				assert.Equal(t, pageSize*i, first)
 
-			result := getUsersIteration(logins, func(previous, current int) error {
-				i++
-
-				if i == 1 {
-					So(previous, ShouldEqual, 0)
-					So(current, ShouldEqual, 500)
-				} else {
-					So(previous, ShouldEqual, 500)
-					So(current, ShouldEqual, 600)
+				expectedLast := pageSize*i + pageSize
+				if expectedLast > userCount {
+					expectedLast = userCount
 				}
 
-				return nil
-			})
-
-			So(i, ShouldEqual, 2)
-			So(result, ShouldBeNil)
-		})
-
-		Convey("it should execute three times for 1500 users", func() {
-			logins := make([]string, 1500)
-			i := 0
+				assert.Equal(t, expectedLast, last)
 
-			result := getUsersIteration(logins, func(previous, current int) error {
 				i++
-				switch i {
-				case 1:
-					So(previous, ShouldEqual, 0)
-					So(current, ShouldEqual, 500)
-				case 2:
-					So(previous, ShouldEqual, 500)
-					So(current, ShouldEqual, 1000)
-				default:
-					So(previous, ShouldEqual, 1000)
-					So(current, ShouldEqual, 1500)
-				}
-
-				return nil
-			})
-
-			So(i, ShouldEqual, 3)
-			So(result, ShouldBeNil)
-		})
-
-		Convey("it should execute once for 400 users", func() {
-			logins := make([]string, 400)
-			i := 0
-
-			result := getUsersIteration(logins, func(previous, current int) error {
-				i++
-				if i == 1 {
-					So(previous, ShouldEqual, 0)
-					So(current, ShouldEqual, 400)
-				}
-
 				return nil
 			})
 
-			So(i, ShouldEqual, 1)
-			So(result, ShouldBeNil)
+			assert.Equal(t, expectedIterations, i)
 		})
+	}
+}
 
-		Convey("it should not execute for 0 users", func() {
-			logins := make([]string, 0)
-			i := 0
-
-			result := getUsersIteration(logins, func(previous, current int) error {
-				i++
-				return nil
-			})
+func TestGetAttribute(t *testing.T) {
+	t.Run("DN", func(t *testing.T) {
+		entry := &ldap.Entry{
+			DN: "test",
+		}
 
-			So(i, ShouldEqual, 0)
-			So(result, ShouldBeNil)
-		})
+		result := getAttribute("dn", entry)
+		assert.Equal(t, "test", result)
 	})
 
-	Convey("getAttribute()", t, func() {
-		Convey("Should get DN", func() {
-			entry := &ldap.Entry{
-				DN: "test",
-			}
-
-			result := getAttribute("dn", entry)
-
-			So(result, ShouldEqual, "test")
-		})
-
-		Convey("Should get username", func() {
-			value := []string{"roelgerrits"}
-			entry := &ldap.Entry{
-				Attributes: []*ldap.EntryAttribute{
-					{
-						Name: "username", Values: value,
-					},
+	t.Run("username", func(t *testing.T) {
+		value := "roelgerrits"
+		entry := &ldap.Entry{
+			Attributes: []*ldap.EntryAttribute{
+				{
+					Name: "username", Values: []string{value},
 				},
-			}
+			},
+		}
 
-			result := getAttribute("username", entry)
-
-			So(result, ShouldEqual, value[0])
-		})
+		result := getAttribute("username", entry)
+		assert.Equal(t, value, result)
+	})
 
-		Convey("Should not get anything", func() {
-			value := []string{"roelgerrits"}
-			entry := &ldap.Entry{
-				Attributes: []*ldap.EntryAttribute{
-					{
-						Name: "killa", Values: value,
-					},
+	t.Run("no result", func(t *testing.T) {
+		value := []string{"roelgerrits"}
+		entry := &ldap.Entry{
+			Attributes: []*ldap.EntryAttribute{
+				{
+					Name: "killa", Values: value,
 				},
-			}
+			},
+		}
 
-			result := getAttribute("username", entry)
-
-			So(result, ShouldEqual, "")
-		})
+		result := getAttribute("username", entry)
+		assert.Empty(t, result)
 	})
+}
 
-	Convey("getArrayAttribute()", t, func() {
-		Convey("Should get DN", func() {
-			entry := &ldap.Entry{
-				DN: "test",
-			}
+func TestGetArrayAttribute(t *testing.T) {
+	t.Run("DN", func(t *testing.T) {
+		entry := &ldap.Entry{
+			DN: "test",
+		}
 
-			result := getArrayAttribute("dn", entry)
+		result := getArrayAttribute("dn", entry)
 
-			So(result, ShouldResemble, []string{"test"})
-		})
+		assert.EqualValues(t, []string{"test"}, result)
+	})
 
-		Convey("Should get username", func() {
-			value := []string{"roelgerrits"}
-			entry := &ldap.Entry{
-				Attributes: []*ldap.EntryAttribute{
-					{
-						Name: "username", Values: value,
-					},
+	t.Run("username", func(t *testing.T) {
+		value := []string{"roelgerrits"}
+		entry := &ldap.Entry{
+			Attributes: []*ldap.EntryAttribute{
+				{
+					Name: "username", Values: value,
 				},
-			}
+			},
+		}
 
-			result := getArrayAttribute("username", entry)
+		result := getArrayAttribute("username", entry)
 
-			So(result, ShouldResemble, value)
-		})
+		assert.EqualValues(t, value, result)
+	})
 
-		Convey("Should not get anything", func() {
-			value := []string{"roelgerrits"}
-			entry := &ldap.Entry{
-				Attributes: []*ldap.EntryAttribute{
-					{
-						Name: "username", Values: value,
-					},
+	t.Run("no result", func(t *testing.T) {
+		value := []string{"roelgerrits"}
+		entry := &ldap.Entry{
+			Attributes: []*ldap.EntryAttribute{
+				{
+					Name: "username", Values: value,
 				},
-			}
+			},
+		}
 
-			result := getArrayAttribute("something", entry)
+		result := getArrayAttribute("something", entry)
 
-			So(result, ShouldResemble, []string{})
-		})
+		assert.Empty(t, result)
 	})
 }