Refactoring defaulting logic

hack/prepare-for-integration-tests.sh

@@ -392,6 +392,7 @@ export PINNIPED_TEST_AD_USER_PASSWORD="$(gcloud secrets versions access latest -
 export PINNIPED_TEST_AD_LDAPS_CA_BUNDLE="$(gcloud secrets versions access latest --secret="concourse-secrets" --project tanzu-user-authentication | yq e '.aws-ad-ca-data' -)"
 export PINNIPED_TEST_AD_USER_EXPECTED_GROUPS_DN="$(gcloud secrets versions access latest --secret="concourse-secrets" --project tanzu-user-authentication | yq e '.aws-ad-expected-direct-groups-dn' -)"
 export PINNIPED_TEST_AD_USER_EXPECTED_GROUPS_CN="$(gcloud secrets versions access latest --secret="concourse-secrets" --project tanzu-user-authentication | yq e '.aws-ad-expected-direct-groups-cn' -)"
+export PINNIPED_TEST_AD_USER_EXPECTED_GROUPS_SAMACCOUNTNAME="$(gcloud secrets versions access latest --secret="concourse-secrets" --project tanzu-user-authentication | yq e '.aws-ad-expected-direct-and-nested-groups-samaccountnames' -)"
 fi
 
 read -r -d '' PINNIPED_TEST_CLUSTER_CAPABILITY_YAML << PINNIPED_TEST_CLUSTER_CAPABILITY_YAML_EOF || true

internal/controller/supervisorconfig/activedirectoryupstreamwatcher/active_directory_upstream_watcher.go

@@ -43,7 +43,7 @@ const (
 	// - is a group.
 	// - has a member that matches the DN of the user we successfully logged in as.
 	// - perform nested group search by default.
-	defaultActiveDirectoryGroupSearchFilter = "(&(objectClass=group)(member:1.2.840.113556.1.4.1941:={})"
+	defaultActiveDirectoryGroupSearchFilter = "(&(objectClass=group)(member:1.2.840.113556.1.4.1941:={}))"
 )
 
 type activeDirectoryUpstreamGenericLDAPImpl struct {
@@ -142,14 +142,23 @@ func (u *activeDirectoryUpstreamGenericLDAPUserSearch) Base() string {
 }
 
 func (u *activeDirectoryUpstreamGenericLDAPUserSearch) Filter() string {
+	if len(u.userSearch.Filter) == 0 {
+		return defaultActiveDirectoryUserSearchFilter
+	}
 	return u.userSearch.Filter
 }
 
 func (u *activeDirectoryUpstreamGenericLDAPUserSearch) UsernameAttribute() string {
+	if len(u.userSearch.Attributes.Username) == 0 {
+		return defaultActiveDirectoryUsernameAttributeName
+	}
 	return u.userSearch.Attributes.Username
 }
 
 func (u *activeDirectoryUpstreamGenericLDAPUserSearch) UIDAttribute() string {
+	if len(u.userSearch.Attributes.UID) == 0 {
+		return defaultActiveDirectoryUIDAttributeName
+	}
 	return u.userSearch.Attributes.UID
 }
 
@@ -162,10 +171,16 @@ func (g *activeDirectoryUpstreamGenericLDAPGroupSearch) Base() string {
 }
 
 func (g *activeDirectoryUpstreamGenericLDAPGroupSearch) Filter() string {
+	if len(g.groupSearch.Filter) == 0 {
+		return defaultActiveDirectoryGroupSearchFilter
+	}
 	return g.groupSearch.Filter
 }
 
 func (g *activeDirectoryUpstreamGenericLDAPGroupSearch) GroupNameAttribute() string {
+	if len(g.groupSearch.Attributes.GroupName) == 0 {
+		return defaultActiveDirectoryGroupNameAttributeName
+	}
 	return g.groupSearch.Attributes.GroupName
 }
 
@@ -275,48 +290,26 @@ 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
+	adUpstreamImpl := activeDirectoryUpstreamGenericLDAPImpl{*upstream}
-	if len(usernameAttribute) == 0 {
-		usernameAttribute = defaultActiveDirectoryUsernameAttributeName
-	}
-	uidAttribute := spec.UserSearch.Attributes.UID
-	if len(uidAttribute) == 0 {
-		uidAttribute = defaultActiveDirectoryUIDAttributeName
-	}
-
-	groupNameAttribute := spec.GroupSearch.Attributes.GroupName
-	if len(groupNameAttribute) == 0 {
-		groupNameAttribute = defaultActiveDirectoryGroupNameAttributeName
-	}
-
-	userSearchFilter := spec.UserSearch.Filter
-	if len(userSearchFilter) == 0 {
-		userSearchFilter = defaultActiveDirectoryUserSearchFilter
-	}
-
-	groupSearchFilter := spec.GroupSearch.Filter
-	if len(groupSearchFilter) == 0 {
-		groupSearchFilter = defaultActiveDirectoryGroupSearchFilter
-	}
 
 	config := &upstreamldap.ProviderConfig{
 		Name: upstream.Name,
 		Host: spec.Host,
 		UserSearch: upstreamldap.UserSearchConfig{
 			Base:              spec.UserSearch.Base,
-			Filter:            userSearchFilter,
+			Filter:            adUpstreamImpl.Spec().UserSearch().Filter(),
-			UsernameAttribute: usernameAttribute,
+			UsernameAttribute: adUpstreamImpl.Spec().UserSearch().UsernameAttribute(),
-			UIDAttribute:      uidAttribute,
+			UIDAttribute:      adUpstreamImpl.Spec().UserSearch().UIDAttribute(),
 		},
 		GroupSearch: upstreamldap.GroupSearchConfig{
 			Base:               spec.GroupSearch.Base,
-			Filter:             groupSearchFilter,
+			Filter:             adUpstreamImpl.Spec().GroupSearch().Filter(),
-			GroupNameAttribute: groupNameAttribute,
+			GroupNameAttribute: adUpstreamImpl.Spec().GroupSearch().GroupNameAttribute(),
 		},
 		Dialer: c.ldapDialer,
 	}
 
-	conditions := upstreamwatchers.ValidateGenericLDAP(ctx, &activeDirectoryUpstreamGenericLDAPImpl{*upstream}, c.secretInformer, c.validatedSecretVersionsCache, config)
+	conditions := upstreamwatchers.ValidateGenericLDAP(ctx, &adUpstreamImpl, c.secretInformer, c.validatedSecretVersionsCache, config)
 
 	c.updateStatus(ctx, upstream, conditions.Conditions())
 

internal/controller/supervisorconfig/activedirectoryupstreamwatcher/active_directory_upstream_watcher_test.go

@@ -1183,7 +1183,7 @@ func TestActiveDirectoryUpstreamWatcherControllerSync(t *testing.T) {
 					},
 					GroupSearch: upstreamldap.GroupSearchConfig{
 						Base:               testGroupSearchBase,
-						Filter:             "(&(objectClass=group)(member:1.2.840.113556.1.4.1941:={})",
+						Filter:             "(&(objectClass=group)(member:1.2.840.113556.1.4.1941:={}))",
 						GroupNameAttribute: "sAMAccountName",
 					},
 				},

test/integration/supervisor_login_test.go

@@ -281,7 +281,7 @@ func TestSupervisorLogin(t *testing.T) {
 			),
 			// the ID token Username should have been pulled from the requested UserSearch.Attributes.Username attribute
 			wantDownstreamIDTokenUsernameToMatch: regexp.QuoteMeta(env.SupervisorUpstreamActiveDirectory.TestUserSAMAccountNameValue),
-			wantDownstreamIDTokenGroups:          env.SupervisorUpstreamActiveDirectory.TestUserDirectGroupsDNs,
+			wantDownstreamIDTokenGroups:          env.SupervisorUpstreamActiveDirectory.TestUserIndirectGroupsSAMAccountNames,
 		},
 	}
 	for _, test := range tests {

test/testlib/env.go

@@ -99,6 +99,7 @@ type TestLDAPUpstream struct {
 	TestUserDirectGroupsCNs               []string `json:"testUserDirectGroupsCNs"`
 	TestUserDirectGroupsDNs               []string `json:"testUserDirectGroupsDNs"` //nolint:golint // this is "distinguished names", not "DNS"
 	TestUserSAMAccountNameValue           string   `json:"testUserSAMAccountNameValue"`
+	TestUserIndirectGroupsSAMAccountNames []string `json:"TestUserIndirectGroupsSAMAccountNames"`
 }
 
 // ProxyEnv returns a set of environment variable strings (e.g., to combine with os.Environ()) which set up the configured test HTTP proxy.
@@ -280,12 +281,14 @@ func loadEnvVars(t *testing.T, result *TestEnv) {
 		TestUserSAMAccountNameValue:           wantEnv("PINNIPED_TEST_AD_USERNAME_ATTRIBUTE_VALUE", ""),
 		TestUserDirectGroupsDNs:               filterEmpty(strings.Split(wantEnv("PINNIPED_TEST_AD_USER_EXPECTED_GROUPS_DN", ""), ";")),
 		TestUserDirectGroupsCNs:               filterEmpty(strings.Split(wantEnv("PINNIPED_TEST_AD_USER_EXPECTED_GROUPS_CN", ""), ";")),
+		TestUserIndirectGroupsSAMAccountNames: filterEmpty(strings.Split(wantEnv("PINNIPED_TEST_AD_USER_EXPECTED_GROUPS_SAMACCOUNTNAME", ""), ";")),
 	}
 
 	sort.Strings(result.SupervisorUpstreamLDAP.TestUserDirectGroupsCNs)
 	sort.Strings(result.SupervisorUpstreamLDAP.TestUserDirectGroupsDNs)
 	sort.Strings(result.SupervisorUpstreamActiveDirectory.TestUserDirectGroupsCNs)
 	sort.Strings(result.SupervisorUpstreamActiveDirectory.TestUserDirectGroupsDNs)
+	sort.Strings(result.SupervisorUpstreamActiveDirectory.TestUserIndirectGroupsSAMAccountNames)
 }
 
 func (e *TestEnv) HasCapability(cap Capability) bool {