Move defaulting of ad username and uid attributes to controller

Now the controller uses upstreamldap so there is less duplication, since they are very similar. Signed-off-by: Ryan Richard <richardry@vmware.com>
2026-09-18 22:14:22 +00:00 · 2021-07-23 13:01:40 -07:00
parent aaa4861373
commit 7696f4256d
4 changed files with 61 additions and 1954 deletions
@@ -27,7 +27,6 @@ import (
 	"go.pinniped.dev/internal/controllerlib"
 	"go.pinniped.dev/internal/oidc/provider"
 	"go.pinniped.dev/internal/plog"
-	"go.pinniped.dev/internal/upstreamad"
 	"go.pinniped.dev/internal/upstreamldap"
 )

@@ -43,6 +42,10 @@ const (
 	reasonActiveDirectoryConnectionError = "ActiveDirectoryConnectionError"
 	noTLSConfigurationMessage            = "no TLS configuration provided"
 	loadedTLSConfigurationMessage        = "loaded TLS configuration"
+
+	// Default values for active directory config
+	defaultActiveDirectoryUsernameAttributeName = "sAMAccountName"
+	defaultActiveDirectoryUIDAttributeName      = "objectGUID"
 )

 // UpstreamActiveDirectoryIdentityProviderICache is a thread safe cache that holds a list of validated upstream LDAP IDP configurations.
@@ -158,14 +161,23 @@ func (c *activeDirectoryWatcherController) Sync(ctx controllerlib.Context) error
 func (c *activeDirectoryWatcherController) validateUpstream(ctx context.Context, upstream *v1alpha1.ActiveDirectoryIdentityProvider) (p provider.UpstreamLDAPIdentityProviderI, requeue bool) {
 	spec := upstream.Spec

+	usernameAttribute := spec.UserSearch.Attributes.Username
+	if len(usernameAttribute) == 0 {
+		usernameAttribute = defaultActiveDirectoryUsernameAttributeName
+	}
+	uidAttribute := spec.UserSearch.Attributes.UID
+	if len(uidAttribute) == 0 {
+		uidAttribute = defaultActiveDirectoryUIDAttributeName
+	}
+
 	config := &upstreamldap.ProviderConfig{
 		Name: upstream.Name,
 		Host: spec.Host,
 		UserSearch: upstreamldap.UserSearchConfig{
 			Base:              spec.UserSearch.Base,
 			Filter:            spec.UserSearch.Filter,
-			UsernameAttribute: spec.UserSearch.Attributes.Username,
-			UIDAttribute:      spec.UserSearch.Attributes.UID,
+			UsernameAttribute: usernameAttribute,
+			UIDAttribute:      uidAttribute,
 		},
 		GroupSearch: upstreamldap.GroupSearchConfig{
 			Base:               spec.GroupSearch.Base,
@@ -198,12 +210,12 @@ func (c *activeDirectoryWatcherController) validateUpstream(ctx context.Context,
 		requeue = true
 	case finishedConfigCondition != nil && finishedConfigCondition.Status != v1alpha1.ConditionTrue:
 		// Error but load it into the cache anyway, treating this condition failure more like a warning.
-		p = upstreamad.New(*config)
+		p = upstreamldap.New(*config)
 		// Try again hoping that the condition will improve.
 		requeue = true
 	default:
 		// Fully validated provider, so load it into the cache.
-		p = upstreamad.New(*config)
+		p = upstreamldap.New(*config)
 		requeue = false
 	}

@@ -12,8 +12,6 @@ import (
 	"testing"
 	"time"

-	"go.pinniped.dev/internal/upstreamad"
-
 	"github.com/go-ldap/ldap/v3"
 	"github.com/golang/mock/gomock"
 	"github.com/stretchr/testify/require"
@@ -139,7 +137,7 @@ func TestActiveDirectoryUpstreamWatcherControllerFilterActiveDirectoryIdentityPr
 	}
 }

-// Wrap the func into a struct so the test can do deep equal assertions on instances of upstreamad.Provider.
+// Wrap the func into a struct so the test can do deep equal assertions on instances of upstreamldap.Provider.
 type comparableDialer struct {
 	upstreamldap.LDAPDialerFunc
 }
@@ -850,6 +848,47 @@ func TestActiveDirectoryUpstreamWatcherControllerSync(t *testing.T) {
 			}},
 			wantValidatedSettings: map[string]validatedSettings{testName: {BindSecretResourceVersion: "4242", LDAPConnectionProtocol: upstreamldap.TLS}},
 		},
+		{
+			name: "when the input activedirectoryidentityprovider leaves user attributes blank, provide default values",
+			inputUpstreams: []runtime.Object{editedValidUpstream(func(upstream *v1alpha1.ActiveDirectoryIdentityProvider) {
+				upstream.Spec.UserSearch.Attributes = v1alpha1.ActiveDirectoryIdentityProviderUserSearchAttributes{}
+			})},
+			inputSecrets: []runtime.Object{validBindUserSecret("4242")},
+			setupMocks: func(conn *mockldapconn.MockConn) {
+				// Should perform a test dial and bind.
+				conn.EXPECT().Bind(testBindUsername, testBindPassword).Times(1)
+				conn.EXPECT().Close().Times(1)
+			},
+			wantResultingCache: []*upstreamldap.ProviderConfig{
+				{
+					Name:               testName,
+					Host:               testHost,
+					ConnectionProtocol: upstreamldap.TLS,
+					CABundle:           testCABundle,
+					BindUsername:       testBindUsername,
+					BindPassword:       testBindPassword,
+					UserSearch: upstreamldap.UserSearchConfig{
+						Base:              testUserSearchBase,
+						Filter:            testUserSearchFilter,
+						UsernameAttribute: "sAMAccountName",
+						UIDAttribute:      "objectGUID",
+					},
+					GroupSearch: upstreamldap.GroupSearchConfig{
+						Base:               testGroupSearchBase,
+						Filter:             testGroupSearchFilter,
+						GroupNameAttribute: testGroupNameAttrName,
+					},
+				},
+			},
+			wantResultingUpstreams: []v1alpha1.ActiveDirectoryIdentityProvider{{
+				ObjectMeta: metav1.ObjectMeta{Namespace: testNamespace, Name: testName, Generation: 1234},
+				Status: v1alpha1.ActiveDirectoryIdentityProviderStatus{
+					Phase:      "Ready",
+					Conditions: allConditionsTrue(1234, "4242"),
+				},
+			}},
+			wantValidatedSettings: map[string]validatedSettings{testName: {BindSecretResourceVersion: "4242", LDAPConnectionProtocol: upstreamldap.TLS}},
+		},
 	}

 	for _, tt := range tests {
@@ -863,7 +902,7 @@ func TestActiveDirectoryUpstreamWatcherControllerSync(t *testing.T) {
 			kubeInformers := informers.NewSharedInformerFactory(fakeKubeClient, 0)
 			cache := provider.NewDynamicUpstreamIDPProvider()
 			cache.SetActiveDirectoryIdentityProviders([]provider.UpstreamLDAPIdentityProviderI{
-				upstreamad.New(upstreamldap.ProviderConfig{Name: "initial-entry"}),
+				upstreamldap.New(upstreamldap.ProviderConfig{Name: "initial-entry"}),
 			})

 			ctrl := gomock.NewController(t)
@@ -917,7 +956,7 @@ func TestActiveDirectoryUpstreamWatcherControllerSync(t *testing.T) {
 			actualIDPList := cache.GetActiveDirectoryIdentityProviders()
 			require.Equal(t, len(tt.wantResultingCache), len(actualIDPList))
 			for i := range actualIDPList {
-				actualIDP := actualIDPList[i].(*upstreamad.Provider)
+				actualIDP := actualIDPList[i].(*upstreamldap.Provider)
 				copyOfExpectedValueForResultingCache := *tt.wantResultingCache[i] // copy before edit to avoid race because these tests are run in parallel
 				// The dialer that was passed in to the controller's constructor should always have been
 				// passed through to the provider.