Reduce some code duplication in LDAP related drivers

This moves the LDAP Bind operation into utils/ldap to reduce some code duplication accross the auth, user- and group-providers.
cs3org · Sep 29, 2021 · d9361ed · d9361ed
1 parent ceb92c3
commit d9361ed
Show file tree

Hide file tree

Showing 5 changed files with 22 additions and 66 deletions.
diff --git a/changelog/unreleased/ldap-util.md b/changelog/unreleased/ldap-util.md
@@ -0,0 +1,3 @@
+Enhancement: Reduce code duplication in LDAP related drivers
+
+https://github.com/cs3org/reva/pull/2115
diff --git a/pkg/auth/manager/ldap/ldap.go b/pkg/auth/manager/ldap/ldap.go
@@ -53,8 +53,6 @@ type config struct {
 	BaseDN         string     `mapstructure:"base_dn"`
 	UserFilter     string     `mapstructure:"userfilter"`
 	LoginFilter    string     `mapstructure:"loginfilter"`
-	BindUsername   string     `mapstructure:"bind_username"`
-	BindPassword   string     `mapstructure:"bind_password"`
 	Idp            string     `mapstructure:"idp"`
 	GatewaySvc     string     `mapstructure:"gatewaysvc"`
 	Schema         attributes `mapstructure:"schema"`
@@ -141,13 +139,6 @@ func (am *mgr) Authenticate(ctx context.Context, clientID, clientSecret string)
 	}
 	defer l.Close()
 
-	// First bind with a read only user
-	err = l.Bind(am.c.BindUsername, am.c.BindPassword)
-	if err != nil {
-		log.Error().Err(err).Msg("bind with system user failed")
-		return nil, nil, err
-	}
-
 	// Search for the given clientID
 	searchRequest := ldap.NewSearchRequest(
 		am.c.BaseDN,

diff --git a/pkg/group/manager/ldap/ldap.go b/pkg/group/manager/ldap/ldap.go
@@ -56,8 +56,6 @@ type config struct {
 	MemberFilter    string     `mapstructure:"memberfilter"`
 	AttributeFilter string     `mapstructure:"attributefilter"`
 	FindFilter      string     `mapstructure:"findfilter"`
-	BindUsername    string     `mapstructure:"bind_username"`
-	BindPassword    string     `mapstructure:"bind_password"`
 	Idp             string     `mapstructure:"idp"`
 	Schema          attributes `mapstructure:"schema"`
 	Nobody          int64      `mapstructure:"nobody"`
@@ -139,12 +137,6 @@ func (m *manager) GetGroup(ctx context.Context, gid *grouppb.GroupId) (*grouppb.
 	}
 	defer l.Close()
 
-	// First bind with a read only user
-	err = l.Bind(m.c.BindUsername, m.c.BindPassword)
-	if err != nil {
-		return nil, err
-	}
-
 	// Search for the given clientID
 	searchRequest := ldap.NewSearchRequest(
 		m.c.BaseDN,
@@ -216,12 +208,6 @@ func (m *manager) GetGroupByClaim(ctx context.Context, claim, value string) (*gr
 	}
 	defer l.Close()
 
-	// First bind with a read only user
-	err = l.Bind(m.c.BindUsername, m.c.BindPassword)
-	if err != nil {
-		return nil, err
-	}
-
 	// Search for the given clientID
 	searchRequest := ldap.NewSearchRequest(
 		m.c.BaseDN,
@@ -274,12 +260,6 @@ func (m *manager) FindGroups(ctx context.Context, query string) ([]*grouppb.Grou
 	}
 	defer l.Close()
 
-	// First bind with a read only user
-	err = l.Bind(m.c.BindUsername, m.c.BindPassword)
-	if err != nil {
-		return nil, err
-	}
-
 	// Search for the given clientID
 	searchRequest := ldap.NewSearchRequest(
 		m.c.BaseDN,
@@ -326,12 +306,6 @@ func (m *manager) GetMembers(ctx context.Context, gid *grouppb.GroupId) ([]*user
 	}
 	defer l.Close()
 
-	// First bind with a read only user
-	err = l.Bind(m.c.BindUsername, m.c.BindPassword)
-	if err != nil {
-		return nil, err
-	}
-
 	// Search for the given clientID
 	searchRequest := ldap.NewSearchRequest(
 		m.c.BaseDN,

diff --git a/pkg/user/manager/ldap/ldap.go b/pkg/user/manager/ldap/ldap.go
@@ -55,8 +55,6 @@ type config struct {
 	AttributeFilter string     `mapstructure:"attributefilter"`
 	FindFilter      string     `mapstructure:"findfilter"`
 	GroupFilter     string     `mapstructure:"groupfilter"`
-	BindUsername    string     `mapstructure:"bind_username"`
-	BindPassword    string     `mapstructure:"bind_password"`
 	Idp             string     `mapstructure:"idp"`
 	Schema          attributes `mapstructure:"schema"`
 	Nobody          int64      `mapstructure:"nobody"`
@@ -151,12 +149,6 @@ func (m *manager) GetUser(ctx context.Context, uid *userpb.UserId) (*userpb.User
 	}
 	defer l.Close()
 
-	// First bind with a read only user
-	err = l.Bind(m.c.BindUsername, m.c.BindPassword)
-	if err != nil {
-		return nil, err
-	}
-
 	// Search for the given clientID
 	searchRequest := ldap.NewSearchRequest(
 		m.c.BaseDN,
@@ -239,12 +231,6 @@ func (m *manager) GetUserByClaim(ctx context.Context, claim, value string) (*use
 	}
 	defer l.Close()
 
-	// First bind with a read only user
-	err = l.Bind(m.c.BindUsername, m.c.BindPassword)
-	if err != nil {
-		return nil, err
-	}
-
 	// Search for the given clientID
 	searchRequest := ldap.NewSearchRequest(
 		m.c.BaseDN,
@@ -311,12 +297,6 @@ func (m *manager) FindUsers(ctx context.Context, query string) ([]*userpb.User,
 	}
 	defer l.Close()
 
-	// First bind with a read only user
-	err = l.Bind(m.c.BindUsername, m.c.BindPassword)
-	if err != nil {
-		return nil, err
-	}
-
 	// Search for the given clientID
 	searchRequest := ldap.NewSearchRequest(
 		m.c.BaseDN,
@@ -381,12 +361,6 @@ func (m *manager) GetUserGroups(ctx context.Context, uid *userpb.UserId) ([]stri
 	}
 	defer l.Close()
 
-	// First bind with a read only user
-	err = l.Bind(m.c.BindUsername, m.c.BindPassword)
-	if err != nil {
-		return []string{}, err
-	}
-
 	// Search for the given clientID
 	searchRequest := ldap.NewSearchRequest(
 		m.c.BaseDN,

diff --git a/pkg/utils/ldap.go b/pkg/utils/ldap.go
@@ -31,10 +31,12 @@ import (
 // LDAPConn holds the basic parameter for setting up an
 // LDAP connection.
 type LDAPConn struct {
-	Hostname string `mapstructure:"hostname"`
-	Port     int    `mapstructure:"port"`
-	Insecure bool   `mapstructure:"insecure"`
-	CACert   string `mapstructure:"cacert"`
+	Hostname     string `mapstructure:"hostname"`
+	Port         int    `mapstructure:"port"`
+	Insecure     bool   `mapstructure:"insecure"`
+	CACert       string `mapstructure:"cacert"`
+	BindUsername string `mapstructure:"bind_username"`
+	BindPassword string `mapstructure:"bind_password"`
 }
 
 // GetLDAPConnection initializes an LDAPS connection and allows
@@ -52,5 +54,17 @@ func GetLDAPConnection(c *LDAPConn) (*ldap.Conn, error) {
 			return nil, errors.Wrapf(err, "Error reading LDAP CA Cert '%s.'", c.CACert)
 		}
 	}
-	return ldap.DialTLS("tcp", fmt.Sprintf("%s:%d", c.Hostname, c.Port), tlsconfig)
+	l, err := ldap.DialTLS("tcp", fmt.Sprintf("%s:%d", c.Hostname, c.Port), tlsconfig)
+	if err != nil {
+		return nil, err
+	}
+
+	if c.BindUsername != "" && c.BindPassword != "" {
+		err = l.Bind(c.BindUsername, c.BindPassword)
+		if err != nil {
+			l.Close()
+			return nil, err
+		}
+	}
+	return l, nil
 }
Original file line number	Diff line number	Diff line change
		@@ -0,0 +1,3 @@
		Enhancement: Reduce code duplication in LDAP related drivers

		https://github.com/cs3org/reva/pull/2115