LDAP: Enhance debug logging and add cmd line tool (ASG-5028) by bosvos · Pull Request #83 · IMQS/authaus

bosvos · 2026-02-01T23:09:53Z

Add command line tool to inspect ldap to auth mapping
Add Hierarchy printing

Swop out ldap library with github.com/go-ldap/ldap/v3
Update to go 1.24

Add command line tool to inspect ldap to auth mapping Add Hierarchy printing Swop out ldap library with github.com/go-ldap/ldap/v3 Update to go 1.24

Copilot

Pull request overview

This pull request enhances LDAP functionality by migrating from the legacy github.com/mavricknz/ldap library to the more modern github.com/go-ldap/ldap/v3, adds enhanced debug logging with LDAP hierarchy visualization, introduces a command-line tool for LDAP diagnostics, and updates the Go version to 1.24.

Changes:

Migrated LDAP library to github.com/go-ldap/ldap/v3 with updated connection handling for PlainText and TLS modes
Added hierarchy extraction and printing functionality to visualize LDAP directory structure during debug mode
Created ldapfetch command-line tool for inspecting LDAP-to-auth user mapping
Enhanced debug logging to show raw LDAP entries, all attributes, and detailed mapping information

Reviewed changes

Copilot reviewed 7 out of 8 changed files in this pull request and generated 13 comments.

Show a summary per file

File	Description
go.mod	Updated Go version to 1.24.0 with toolchain 1.24.10, replaced old LDAP library with go-ldap/ldap/v3, updated dependencies
go.sum	Updated checksums for all new and updated dependencies
msaad.go	Fixed unnecessary fmt.Sprintf wrapper in fmt.Errorf call
ldap.go	Major refactor: exported Config field, migrated to new LDAP library, added hierarchy extraction/printing, enhanced debug logging, deprecated SSL mode
ldap_test.go	Added unit test for printTrunc string truncation function
dummyLDAP.go	Updated GetLdapUsers signature to accept logger parameter
db.go	Updated LDAP interface GetLdapUsers signature to accept logger parameter
cmd/ldapfetch/main.go	New command-line tool for fetching and displaying LDAP users with configuration from JSON file

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Copilot · 2026-02-01T23:16:58Z

ldap.go

 	switch ldapMode {
 	case LdapConnectionModePlainText:
-	case LdapConnectionModeSSL:
-		con.IsSSL = true
+		c, e := ldap.DialURL(addr, ldap.DialWithDialer(dialer))
+		if e != nil {
+			return nil, e
+		}
+		c.SetTimeout(10 * time.Second)
+		return c, e
+	//// DEPRECATED
+	//case LdapConnectionModeSSL:
+	//	con.IsSSL = true
 	case LdapConnectionModeTLS:
-		con.IsTLS = true
-	}
-	if config.InsecureSkipVerify {
-		con.TlsConfig = &tls.Config{}
-		con.TlsConfig.InsecureSkipVerify = config.InsecureSkipVerify
-	}
-	if err := con.Connect(); err != nil {
-		con.Close()
-		return nil, err
+		tlsConfig := &tls.Config{}
+		if config.InsecureSkipVerify {
+			tlsConfig.InsecureSkipVerify = config.InsecureSkipVerify
+		}
+		c, e := ldap.DialURL(addr, ldap.DialWithDialer(dialer), ldap.DialWithTLSConfig(tlsConfig))
+		if e != nil {
+			return nil, e
+		}
+		c.SetTimeout(10 * time.Second)
+		return c, e
+	default:
+		return nil, errors.New("unimplemented LDAP connection mode")
 	}


The SSL connection mode is commented out as deprecated, but it's still defined in the LdapConnectionMode enum and is still referenced in the configLdapNameToMode map in config.go. This creates an inconsistency - users can still configure "SSL" as the encryption mode in their configuration, but the connection will fail at runtime with "unimplemented LDAP connection mode". Either implement SSL mode support with the new library (using ldaps:// protocol as noted in the TODO), or properly handle this case by returning a clear error message explaining that SSL mode is deprecated and users should migrate to TLS.

Copilot · 2026-02-01T23:16:58Z

ldap.go

 type LdapImpl struct {
-	config *ConfigLDAP
+	Config *ConfigLDAP
+}


The field Config is now exported (changed from lowercase 'config' to uppercase 'Config'), which changes the public API of LdapImpl. This is a breaking change for any external code that might be using reflection or JSON serialization/deserialization of this struct. While the command-line tool needs access to this field, consider whether this API change is intentional and document it if it is, or use a getter method instead to maintain backward compatibility.

@copilot Please remove the export and add a getter method - also update the command line tool to reflect this change.

Copilot · 2026-02-01T23:16:59Z

ldap.go

+	if x.Config.DebugUserPull {
+		log.Infof("%d records retrieved from LDAP server...\n", len(sr.Entries))
+	}
+	allAtrributes := make(map[string]struct{})


The variable 'allAtrributes' has a typo - it should be 'allAttributes' (with two 't's). This affects readability and should be corrected throughout its usage in the function.

Copilot · 2026-02-01T23:16:59Z

ldap.go

+		for _, attr := range value.Attributes {
+			allAtrributes[attr.Name] = struct{}{}


The same typo 'allAtrributes' appears here and should be corrected to 'allAttributes'.

Copilot · 2026-02-01T23:16:59Z

ldap.go

+		attributeNames := make([]string, 0, len(allAtrributes))
+		for attrName := range allAtrributes {
+			attributeNames = append(attributeNames, attrName)
+		}
+		log.Infof("%v\n", strings.Join(attributeNames, ", "))


The same typo 'allAtrributes' appears here and should be corrected to 'allAttributes'.

Copilot · 2026-02-01T23:17:00Z

ldap.go

+	if x.Config.DebugUserPull {
+		// print hierarchy by iterating over the tree, depth first
+		log.Infof("LDAP hierarchy:\n")
+		printHierarchy(extractHierarchy(sr), "", true, log)
+	}


The hierarchy is printed before checking if the search was successful. If the search fails (err != nil on line 99), the hierarchy will still be printed based on potentially incomplete or empty results. The hierarchy printing should be moved after the error check on line 99-101 to ensure it only prints valid, complete results.

Copilot · 2026-02-01T23:17:00Z

ldap.go

+func extractHierarchy(sr *ldap.SearchResult) *hierarchyNode {
+	tree := &hierarchyNode{
+		name:     "ROOT",
+		nodeType: "ROOT",
+		children: make(map[string]*hierarchyNode),
+	}
+
+	for _, a := range sr.Entries {
+		pathElements := extractPath(a.DN)
+		for p := range pathElements {
+			currentNode := tree
+			for i := 0; i <= p; i++ {
+				part := pathElements[i]
+				childNode, found := currentNode.children[part.name]
+				if !found {
+					childNode = &hierarchyNode{
+						name:     part.name,
+						nodeType: part.nodeType,
+						children: make(map[string]*hierarchyNode),
+					}
+					currentNode.children[part.name] = childNode
+				}
+				currentNode = childNode
+			}
+		}
+	}
+	return tree
+}
+
+// extractPath extracts the top-down path elements from a DN string
+func extractPath(dn string) []pathElement {
+	parts := strings.Split(dn, ",")
+	result := make([]pathElement, 0, len(parts))
+	for i := len(parts) - 1; i >= 0; i-- {
+		part := strings.TrimSpace(parts[i])
+		equalIndex := strings.Index(part, "=")
+		if equalIndex != -1 && equalIndex < len(part)-1 {
+			result = append(result, pathElement{part[equalIndex+1:], part[:equalIndex]})
+		}
+	}
+	return result
+}


The new functions extractHierarchy, extractPath, and printHierarchy lack test coverage. While printTrunc has a test added, the hierarchy extraction and printing logic is more complex and should have unit tests to verify correct behavior, especially for edge cases like empty DNs, malformed DNs, or DNs with special characters. The repository has comprehensive test coverage for LDAP functionality (see all_test.go), and these new functions should follow that pattern.

@copilot open a new pull request to apply changes based on this feedback

Copilot · 2026-02-01T23:17:01Z

ldap.go

+	if config.LdapPort == 0 {
+		config.LdapPort = 389
+	}


The default port is set to 389 inside NewLDAPConnect by modifying the config parameter. This is a side effect that modifies the caller's config struct. While this works when the config is passed by pointer, it's unexpected behavior - the function should either document this side effect clearly in its function comment, or use a local copy/variable for the port instead of modifying the config. The existing comment on lines 413-417 doesn't mention this mutation behavior.

Copilot · 2026-02-01T23:17:01Z

cmd/ldapfetch/main.go

+// Usage: ldapfetch <path to config>
+package main
+
+import (
+	"encoding/json"
+	"fmt"
+	"github.com/IMQS/authaus"
+	"github.com/IMQS/log"
+	"os"
+)
+
+func main() {
+	if len(os.Args) < 2 {
+		fmt.Println("Usage: ldapfetch <path to ldapauth.json>")


The usage message in line 17 says "ldapfetch <path to ldapauth.json>" but the actual command-line comment in line 4 says "ldapfetch <path to config>". These should be consistent. Consider using the same terminology in both places - either "config file" or "ldapauth.json" - for clarity.

Copilot · 2026-02-01T23:17:01Z

ldap.go

+	} else {
+		return str
+	}
+}
+


The else clause on line 218 is unnecessary. When the condition on line 216 is true, the function returns early, so the else can be removed and the return statement can be dedented. This is a minor style improvement that makes the code cleaner.

Suggested change

} else {

return str

}

}

}

return str

}

Copilot · 2026-03-07T07:48:47Z

@bosvos I've opened a new pull request, #84, to work on those changes. Once the pull request is ready, I'll request review from you.

Copilot · 2026-03-07T08:27:29Z

@bosvos I've opened a new pull request, #85, to work on those changes. Once the pull request is ready, I'll request review from you.

… tool Co-authored-by: bosvos <2437699+bosvos@users.noreply.github.com>

Co-authored-by: bosvos <2437699+bosvos@users.noreply.github.com> PR #83 Changes

LDAP: Enhance debug logging and add cmd line tool (ASG-5028)

ae0a4fe

Add command line tool to inspect ldap to auth mapping Add Hierarchy printing Swop out ldap library with github.com/go-ldap/ldap/v3 Update to go 1.24

bosvos requested review from FritzOnFire and Copilot February 1, 2026 23:09

Copilot started reviewing on behalf of bosvos February 1, 2026 23:10 View session

Copilot AI reviewed Feb 1, 2026

View reviewed changes

Copilot AI mentioned this pull request Mar 7, 2026

Unexport LdapImpl.Config and expose via getter method #84

Merged

Copilot AI mentioned this pull request Mar 7, 2026

Add unit tests for extractPath, extractHierarchy, and printHierarchy #85

Merged

bosvos and others added 2 commits March 7, 2026 10:32

PR #83 Changes

044af97

Unexport Config field in LdapImpl, add GetConfig() getter, update cmd…

d9d51ca

… tool Co-authored-by: bosvos <2437699+bosvos@users.noreply.github.com>

bosvos pushed a commit that referenced this pull request Mar 7, 2026

Add unit tests for extractPath, extractHierarchy, and printHierarchy

1b70c1c

Co-authored-by: bosvos <2437699+bosvos@users.noreply.github.com> PR #83 Changes

Add unit tests for extractPath, extractHierarchy, and printHierarchy

1c7f725

Co-authored-by: bosvos <2437699+bosvos@users.noreply.github.com> PR #83 Changes

		for _, attr := range value.Attributes {
		allAtrributes[attr.Name] = struct{}{}

Conversation

bosvos commented Feb 1, 2026

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Copilot AI Feb 1, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Feb 1, 2026

Choose a reason for hiding this comment

Uh oh!

bosvos Mar 7, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Feb 1, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Feb 1, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Feb 1, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Feb 1, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Feb 1, 2026

Choose a reason for hiding this comment

Uh oh!

bosvos Mar 7, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Feb 1, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Feb 1, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Feb 1, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI commented Mar 7, 2026

Uh oh!

Copilot AI commented Mar 7, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants