From 8275ab917e685313d0c5619935a5fb77be283611 Mon Sep 17 00:00:00 2001
From: Chris Lu <chris.lu@gmail.com>
Date: Sun, 11 Jan 2026 19:53:26 -0800
Subject: [PATCH] fix: address PR feedback (Round 2) - config validation &
 provider improvements

- Implement `validateLDAPConfig` in `ProviderFactory`
- Improve `LDAPProvider.Initialize`:
  - Support `connectionTimeout` parsing (string/int/float) from config map
  - Warn if `BindDN` is present but `BindPassword` is empty
- Improve `LDAPProvider.GetUserInfo`:
  - Add fallback to `searchUserGroups` if `memberOf` returns no groups (consistent with Authenticate)
---
 weed/iam/ldap/ldap_provider.go   | 34 ++++++++++++++++++++++++++++++--
 weed/iam/sts/provider_factory.go |  7 ++++++-
 2 files changed, 38 insertions(+), 3 deletions(-)

diff --git a/weed/iam/ldap/ldap_provider.go b/weed/iam/ldap/ldap_provider.go
index 0e8e61c4a..623ad80b7 100644
--- a/weed/iam/ldap/ldap_provider.go
+++ b/weed/iam/ldap/ldap_provider.go
@@ -116,6 +116,19 @@ func (p *LDAPProvider) Initialize(config interface{}) error {
 			if v, ok := cfgMap["insecureSkipVerify"].(bool); ok {
 				cfg.InsecureSkipVerify = v
 			}
+			// Parse connection timeout
+			if v, ok := cfgMap["connectionTimeout"]; ok {
+				switch val := v.(type) {
+				case float64:
+					cfg.ConnectionTimeout = time.Duration(val) * time.Second
+				case int:
+					cfg.ConnectionTimeout = time.Duration(val) * time.Second
+				case string:
+					if d, err := time.ParseDuration(val); err == nil {
+						cfg.ConnectionTimeout = d
+					}
+				}
+			}
 			// Parse attributes
 			if attrs, ok := cfgMap["attributes"].(map[string]interface{}); ok {
 				if v, ok := attrs["email"].(string); ok {
@@ -147,6 +160,11 @@ func (p *LDAPProvider) Initialize(config interface{}) error {
 		cfg.UserFilter = "(cn=%s)" // Default filter
 	}
 
+	// Warn if BindDN is configured but BindPassword is empty
+	if cfg.BindDN != "" && cfg.BindPassword == "" {
+		glog.Warningf("LDAP provider '%s' configured with BindDN but no BindPassword", p.name)
+	}
+
 	// Set default attributes
 	if cfg.Attributes.Email == "" {
 		cfg.Attributes.Email = "mail"
@@ -388,7 +406,7 @@ func (p *LDAPProvider) GetUserInfo(ctx context.Context, userID string) (*provide
 	}
 
 	userEntry := result.Entries[0]
-	return &providers.ExternalIdentity{
+	identity := &providers.ExternalIdentity{
 		UserID:      userID,
 		Email:       userEntry.GetAttributeValue(config.Attributes.Email),
 		DisplayName: userEntry.GetAttributeValue(config.Attributes.DisplayName),
@@ -398,7 +416,19 @@ func (p *LDAPProvider) GetUserInfo(ctx context.Context, userID string) (*provide
 			"dn":  userEntry.DN,
 			"uid": userEntry.GetAttributeValue(config.Attributes.UID),
 		},
-	}, nil
+	}
+
+	// If no groups from memberOf, try group search
+	if len(identity.Groups) == 0 && config.GroupFilter != "" {
+		groups, err := p.searchUserGroups(conn, userEntry.DN, config)
+		if err != nil {
+			glog.V(2).Infof("Group search failed for %s: %v", userID, err)
+		} else {
+			identity.Groups = groups
+		}
+	}
+
+	return identity, nil
 }
 
 // ValidateToken validates credentials (username:password format) and returns claims
diff --git a/weed/iam/sts/provider_factory.go b/weed/iam/sts/provider_factory.go
index c9317a1e1..53635c8f2 100644
--- a/weed/iam/sts/provider_factory.go
+++ b/weed/iam/sts/provider_factory.go
@@ -321,7 +321,12 @@ func (f *ProviderFactory) validateOIDCConfig(config map[string]interface{}) erro
 
 // validateLDAPConfig validates LDAP provider configuration
 func (f *ProviderFactory) validateLDAPConfig(config map[string]interface{}) error {
-	// TODO: Implement when LDAP provider is available
+	if _, ok := config["server"]; !ok {
+		return fmt.Errorf("LDAP provider requires 'server' field")
+	}
+	if _, ok := config["baseDN"]; !ok {
+		return fmt.Errorf("LDAP provider requires 'baseDN' field")
+	}
 	return nil
 }