Cleanup new group attribute behavior and add test coverage

internal/controller/supervisorconfig/activedirectoryupstreamwatcher/active_directory_upstream_watcher.go

@@ -32,13 +32,12 @@ const (
 	defaultActiveDirectoryUsernameAttributeName = "userPrincipalName"
 	defaultActiveDirectoryUIDAttributeName      = "objectGUID"
 
-	// This is not a real attribute in active directory.
+	// By default this group name attribute is the sAMAccountName with special mapping.
-	// It represents a combined attribute name, sAMAccountName + "@" + domain.
+	// Each group will look like sAMAccountName + "@" + domain.
 	// For example if your group sAMAccountName is "mammals" and your domain is
 	// "activedirectory.example.com", it would be mammals@activedirectory.example.com.
 	// This is because sAMAccountName is only unique within a domain, not a forest.
-	defaultActiveDirectoryGroupNameAttributeName         = "sAMAccountName"
+	defaultActiveDirectoryGroupNameAttributeName = "sAMAccountName"
-	defaultActiveDirectoryGroupNameOverrideAttributeName = "pinniped:sAMAccountName@domain"
 
 	// - is a person.
 	// - is not a computer.
@@ -188,10 +187,6 @@ func (g *activeDirectoryUpstreamGenericLDAPGroupSearch) GroupNameAttribute() str
 	if len(g.groupSearch.Attributes.GroupName) == 0 {
 		return defaultActiveDirectoryGroupNameAttributeName
 	}
-	// you explicitly told us to use the override value
-	if g.groupSearch.Attributes.GroupName == defaultActiveDirectoryGroupNameOverrideAttributeName {
-		return defaultActiveDirectoryGroupNameAttributeName
-	}
 	return g.groupSearch.Attributes.GroupName
 }
 
@@ -321,7 +316,7 @@ func (c *activeDirectoryWatcherController) validateUpstream(ctx context.Context,
 		UIDAttributeParsingOverrides: []upstreamldap.AttributeParsingOverride{{AttributeName: "objectGUID", OverrideFunc: upstreamldap.MicrosoftUUIDFromBinary("objectGUID")}},
 	}
 
-	if spec.GroupSearch.Attributes.GroupName == defaultActiveDirectoryGroupNameOverrideAttributeName || spec.GroupSearch.Attributes.GroupName == "" {
+	if spec.GroupSearch.Attributes.GroupName == "" {
 		config.GroupAttributeParsingOverrides = []upstreamldap.AttributeParsingOverride{{AttributeName: defaultActiveDirectoryGroupNameAttributeName, OverrideFunc: upstreamldap.GroupSAMAccountNameWithDomainSuffix}}
 	}
 

internal/controller/supervisorconfig/activedirectoryupstreamwatcher/active_directory_upstream_watcher_test.go

@@ -218,7 +218,7 @@ func TestActiveDirectoryUpstreamWatcherControllerSync(t *testing.T) {
 			Filter:             testGroupSearchFilter,
 			GroupNameAttribute: testGroupNameAttrName,
 		},
-		UIDAttributeParsingOverrides: []upstreamldap.AttributeParsingOverride{{AttributeName: "objectGUID", OverrideFunc: upstreamldap.MicrosoftUUIDFromBinary}},
+		UIDAttributeParsingOverrides: []upstreamldap.AttributeParsingOverride{{AttributeName: "objectGUID", OverrideFunc: upstreamldap.MicrosoftUUIDFromBinary("objectGUID")}},
 	}
 
 	// Make a copy with targeted changes.
@@ -533,7 +533,7 @@ func TestActiveDirectoryUpstreamWatcherControllerSync(t *testing.T) {
 						Filter:             testGroupSearchFilter,
 						GroupNameAttribute: testGroupNameAttrName,
 					},
-					UIDAttributeParsingOverrides: []upstreamldap.AttributeParsingOverride{{AttributeName: "objectGUID", OverrideFunc: upstreamldap.MicrosoftUUIDFromBinary}},
+					UIDAttributeParsingOverrides: []upstreamldap.AttributeParsingOverride{{AttributeName: "objectGUID", OverrideFunc: upstreamldap.MicrosoftUUIDFromBinary("objectGUID")}},
 				},
 			},
 			wantResultingUpstreams: []v1alpha1.ActiveDirectoryIdentityProvider{{
@@ -591,7 +591,7 @@ func TestActiveDirectoryUpstreamWatcherControllerSync(t *testing.T) {
 						Filter:             testGroupSearchFilter,
 						GroupNameAttribute: testGroupNameAttrName,
 					},
-					UIDAttributeParsingOverrides: []upstreamldap.AttributeParsingOverride{{AttributeName: "objectGUID", OverrideFunc: upstreamldap.MicrosoftUUIDFromBinary}},
+					UIDAttributeParsingOverrides: []upstreamldap.AttributeParsingOverride{{AttributeName: "objectGUID", OverrideFunc: upstreamldap.MicrosoftUUIDFromBinary("objectGUID")}},
 				},
 			},
 			wantResultingUpstreams: []v1alpha1.ActiveDirectoryIdentityProvider{{
@@ -649,7 +649,7 @@ func TestActiveDirectoryUpstreamWatcherControllerSync(t *testing.T) {
 						Filter:             testGroupSearchFilter,
 						GroupNameAttribute: testGroupNameAttrName,
 					},
-					UIDAttributeParsingOverrides: []upstreamldap.AttributeParsingOverride{{AttributeName: "objectGUID", OverrideFunc: upstreamldap.MicrosoftUUIDFromBinary}},
+					UIDAttributeParsingOverrides: []upstreamldap.AttributeParsingOverride{{AttributeName: "objectGUID", OverrideFunc: upstreamldap.MicrosoftUUIDFromBinary("objectGUID")}},
 				},
 			},
 			wantErr: controllerlib.ErrSyntheticRequeue.Error(),
@@ -706,7 +706,7 @@ func TestActiveDirectoryUpstreamWatcherControllerSync(t *testing.T) {
 						Filter:             testGroupSearchFilter,
 						GroupNameAttribute: testGroupNameAttrName,
 					},
-					UIDAttributeParsingOverrides: []upstreamldap.AttributeParsingOverride{{AttributeName: "objectGUID", OverrideFunc: upstreamldap.MicrosoftUUIDFromBinary}},
+					UIDAttributeParsingOverrides: []upstreamldap.AttributeParsingOverride{{AttributeName: "objectGUID", OverrideFunc: upstreamldap.MicrosoftUUIDFromBinary("objectGUID")}},
 				},
 			},
 			wantResultingUpstreams: []v1alpha1.ActiveDirectoryIdentityProvider{{
@@ -830,7 +830,7 @@ func TestActiveDirectoryUpstreamWatcherControllerSync(t *testing.T) {
 						Filter:             testGroupSearchFilter,
 						GroupNameAttribute: testGroupNameAttrName,
 					},
-					UIDAttributeParsingOverrides: []upstreamldap.AttributeParsingOverride{{AttributeName: "objectGUID", OverrideFunc: upstreamldap.MicrosoftUUIDFromBinary}},
+					UIDAttributeParsingOverrides: []upstreamldap.AttributeParsingOverride{{AttributeName: "objectGUID", OverrideFunc: upstreamldap.MicrosoftUUIDFromBinary("objectGUID")}},
 				},
 			},
 			wantResultingUpstreams: []v1alpha1.ActiveDirectoryIdentityProvider{{
@@ -950,7 +950,7 @@ func TestActiveDirectoryUpstreamWatcherControllerSync(t *testing.T) {
 						Filter:             testGroupSearchFilter,
 						GroupNameAttribute: testGroupNameAttrName,
 					},
-					UIDAttributeParsingOverrides: []upstreamldap.AttributeParsingOverride{{AttributeName: "objectGUID", OverrideFunc: upstreamldap.MicrosoftUUIDFromBinary}},
+					UIDAttributeParsingOverrides: []upstreamldap.AttributeParsingOverride{{AttributeName: "objectGUID", OverrideFunc: upstreamldap.MicrosoftUUIDFromBinary("objectGUID")}},
 				},
 			},
 			wantResultingUpstreams: []v1alpha1.ActiveDirectoryIdentityProvider{{
@@ -1000,7 +1000,7 @@ func TestActiveDirectoryUpstreamWatcherControllerSync(t *testing.T) {
 						Filter:             testGroupSearchFilter,
 						GroupNameAttribute: testGroupNameAttrName,
 					},
-					UIDAttributeParsingOverrides: []upstreamldap.AttributeParsingOverride{{AttributeName: "objectGUID", OverrideFunc: upstreamldap.MicrosoftUUIDFromBinary}},
+					UIDAttributeParsingOverrides: []upstreamldap.AttributeParsingOverride{{AttributeName: "objectGUID", OverrideFunc: upstreamldap.MicrosoftUUIDFromBinary("objectGUID")}},
 				},
 			},
 			wantResultingUpstreams: []v1alpha1.ActiveDirectoryIdentityProvider{{
@@ -1190,58 +1190,7 @@ func TestActiveDirectoryUpstreamWatcherControllerSync(t *testing.T) {
 						Filter:             "(&(objectClass=group)(member:1.2.840.113556.1.4.1941:={}))",
 						GroupNameAttribute: "sAMAccountName",
 					},
-					UIDAttributeParsingOverrides:   []upstreamldap.AttributeParsingOverride{{AttributeName: "objectGUID", OverrideFunc: upstreamldap.MicrosoftUUIDFromBinary}},
+					UIDAttributeParsingOverrides:   []upstreamldap.AttributeParsingOverride{{AttributeName: "objectGUID", OverrideFunc: upstreamldap.MicrosoftUUIDFromBinary("objectGUID")}},
-					GroupAttributeParsingOverrides: []upstreamldap.AttributeParsingOverride{{AttributeName: "sAMAccountName", OverrideFunc: upstreamldap.GroupSAMAccountNameWithDomainSuffix}},
-				},
-			},
-			wantResultingUpstreams: []v1alpha1.ActiveDirectoryIdentityProvider{{
-				ObjectMeta: metav1.ObjectMeta{Namespace: testNamespace, Name: testName, Generation: 1234},
-				Status: v1alpha1.ActiveDirectoryIdentityProviderStatus{
-					Phase:      "Ready",
-					Conditions: allConditionsTrue(1234, "4242"),
-				},
-			}},
-			wantValidatedSettings: map[string]upstreamwatchers.ValidatedSettings{testName: {
-				BindSecretResourceVersion: "4242",
-				LDAPConnectionProtocol:    upstreamldap.TLS,
-				UserSearchBase:            testUserSearchBase,
-				GroupSearchBase:           testGroupSearchBase,
-			}},
-		},
-		{
-			name: "when the input activedirectoryidentityprovider group search attributes is the special cased pinniped:sAMAccountName@domain value",
-			inputUpstreams: []runtime.Object{editedValidUpstream(func(upstream *v1alpha1.ActiveDirectoryIdentityProvider) {
-				upstream.Spec.UserSearch.Attributes = v1alpha1.ActiveDirectoryIdentityProviderUserSearchAttributes{}
-				upstream.Spec.UserSearch.Filter = ""
-				upstream.Spec.GroupSearch.Filter = ""
-				upstream.Spec.GroupSearch.Attributes = v1alpha1.ActiveDirectoryIdentityProviderGroupSearchAttributes{GroupName: "pinniped:sAMAccountName@domain"}
-			})},
-			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:            "(&(objectClass=person)(!(objectClass=computer))(!(showInAdvancedViewOnly=TRUE))(|(sAMAccountName={})(mail={})(userPrincipalName={}))(sAMAccountType=805306368))",
-						UsernameAttribute: "userPrincipalName",
-						UIDAttribute:      "objectGUID",
-					},
-					GroupSearch: upstreamldap.GroupSearchConfig{
-						Base:               testGroupSearchBase,
-						Filter:             "(&(objectClass=group)(member:1.2.840.113556.1.4.1941:={}))",
-						GroupNameAttribute: "sAMAccountName",
-					},
-					UIDAttributeParsingOverrides:   []upstreamldap.AttributeParsingOverride{{AttributeName: "objectGUID", OverrideFunc: upstreamldap.MicrosoftUUIDFromBinary}},
 					GroupAttributeParsingOverrides: []upstreamldap.AttributeParsingOverride{{AttributeName: "sAMAccountName", OverrideFunc: upstreamldap.GroupSAMAccountNameWithDomainSuffix}},
 				},
 			},
@@ -1292,7 +1241,7 @@ func TestActiveDirectoryUpstreamWatcherControllerSync(t *testing.T) {
 						Filter:             testGroupSearchFilter,
 						GroupNameAttribute: testGroupNameAttrName,
 					},
-					UIDAttributeParsingOverrides: []upstreamldap.AttributeParsingOverride{{AttributeName: "objectGUID", OverrideFunc: upstreamldap.MicrosoftUUIDFromBinary}},
+					UIDAttributeParsingOverrides: []upstreamldap.AttributeParsingOverride{{AttributeName: "objectGUID", OverrideFunc: upstreamldap.MicrosoftUUIDFromBinary("objectGUID")}},
 				},
 			},
 			wantResultingUpstreams: []v1alpha1.ActiveDirectoryIdentityProvider{{
@@ -1341,7 +1290,7 @@ func TestActiveDirectoryUpstreamWatcherControllerSync(t *testing.T) {
 						Filter:             testGroupSearchFilter,
 						GroupNameAttribute: testGroupNameAttrName,
 					},
-					UIDAttributeParsingOverrides: []upstreamldap.AttributeParsingOverride{{AttributeName: "objectGUID", OverrideFunc: upstreamldap.MicrosoftUUIDFromBinary}},
+					UIDAttributeParsingOverrides: []upstreamldap.AttributeParsingOverride{{AttributeName: "objectGUID", OverrideFunc: upstreamldap.MicrosoftUUIDFromBinary("objectGUID")}},
 				},
 			},
 			wantResultingUpstreams: []v1alpha1.ActiveDirectoryIdentityProvider{{
@@ -1390,7 +1339,7 @@ func TestActiveDirectoryUpstreamWatcherControllerSync(t *testing.T) {
 						Filter:             testGroupSearchFilter,
 						GroupNameAttribute: testGroupNameAttrName,
 					},
-					UIDAttributeParsingOverrides: []upstreamldap.AttributeParsingOverride{{AttributeName: "objectGUID", OverrideFunc: upstreamldap.MicrosoftUUIDFromBinary}},
+					UIDAttributeParsingOverrides: []upstreamldap.AttributeParsingOverride{{AttributeName: "objectGUID", OverrideFunc: upstreamldap.MicrosoftUUIDFromBinary("objectGUID")}},
 				},
 			},
 			wantResultingUpstreams: []v1alpha1.ActiveDirectoryIdentityProvider{{

internal/upstreamldap/upstreamldap.go

@@ -386,23 +386,26 @@ func (p *Provider) searchGroupsForUserDN(conn Conn, userDN string) ([]string, er
 	}
 
 	groups := []string{}
+entries:
 	for _, groupEntry := range searchResult.Entries {
 		if len(groupEntry.DN) == 0 {
 			return nil, fmt.Errorf(`searching for group memberships for user with DN %q resulted in search result without DN`, userDN)
 		}
-		mappedGroupName, err := p.getSearchResultAttributeValue(groupAttributeName, groupEntry, userDN)
-		if err != nil {
-			return nil, fmt.Errorf(`error searching for group memberships for user with DN %q: %w`, userDN, err)
-		}
 		for _, override := range p.c.GroupAttributeParsingOverrides {
 			if groupAttributeName == override.AttributeName {
 				overrideGroupName, err := override.OverrideFunc(groupEntry)
 				if err != nil {
-					return nil, fmt.Errorf("error finding groups: %w", err)
+					return nil, fmt.Errorf("error finding groups for user %s: %w", userDN, err)
 				}
-				mappedGroupName = overrideGroupName
+				groups = append(groups, overrideGroupName)
+				continue entries
 			}
 		}
+		// if none of the overrides matched, use the default behavior (no mapping)
+		mappedGroupName, err := p.getSearchResultAttributeValue(groupAttributeName, groupEntry, userDN)
+		if err != nil {
+			return nil, fmt.Errorf(`error searching for group memberships for user with DN %q: %w`, userDN, err)
+		}
 		groups = append(groups, mappedGroupName)
 	}
 
@@ -708,7 +711,21 @@ func microsoftUUIDFromBinary(binaryUUID []byte) (string, error) {
 
 func GroupSAMAccountNameWithDomainSuffix(entry *ldap.Entry) (string, error) {
 	sAMAccountNameAttribute := "sAMAccountName"
-	sAMAccountName := entry.GetAttributeValue(sAMAccountNameAttribute)
+	sAMAccountNameAttributeValues := entry.GetAttributeValues(sAMAccountNameAttribute)
+
+	if len(sAMAccountNameAttributeValues) != 1 {
+		return "", fmt.Errorf(`found %d values for attribute "%s", but expected 1 result`,
+			len(sAMAccountNameAttributeValues), sAMAccountNameAttribute,
+		)
+	}
+
+	sAMAccountName := sAMAccountNameAttributeValues[0]
+	if len(sAMAccountName) == 0 {
+		return "", fmt.Errorf(`found empty value for attribute "%s", but expected value to be non-empty`,
+			sAMAccountNameAttribute,
+		)
+	}
+
 	distinguishedName := entry.DN
 	domain, err := getDomainFromDistinguishedName(distinguishedName)
 	if err != nil {

internal/upstreamldap/upstreamldap_test.go

@@ -608,6 +608,62 @@ func TestEndUserAuthentication(t *testing.T) {
 				r.Groups = []string{"Animals@activedirectory.mycompany.example.com", "Mammals@activedirectory.mycompany.example.com"}
 			}),
 		},
+		{
+			name: "only the first group override for a given attribute name is applied",
+			// the choice to only run the first is somewhat arbitrary and likely irrelevant since we only
+			// have one group override at the moment...
+			// And as soon as we have starlark attribute mapping it will be obsolete. But this test
+			// ensures that we
+			username: testUpstreamUsername,
+			password: testUpstreamPassword,
+			providerConfig: providerConfig(func(p *ProviderConfig) {
+				p.GroupSearch.GroupNameAttribute = "sAMAccountName"
+				p.GroupAttributeParsingOverrides = []AttributeParsingOverride{
+					{
+						AttributeName: "sAMAccountName",
+						OverrideFunc:  GroupSAMAccountNameWithDomainSuffix,
+					},
+					{
+						AttributeName: "sAMAccountName",
+						OverrideFunc: func(entry *ldap.Entry) (string, error) {
+							return "override-group-name", nil
+						},
+					},
+				}
+			}),
+			searchMocks: func(conn *mockldapconn.MockConn) {
+				conn.EXPECT().Bind(testBindUsername, testBindPassword).Times(1)
+				conn.EXPECT().Search(expectedUserSearch(nil)).Return(exampleUserSearchResult, nil).Times(1)
+				conn.EXPECT().SearchWithPaging(expectedGroupSearch(func(r *ldap.SearchRequest) {
+					r.Attributes = []string{"sAMAccountName"}
+				}), expectedGroupSearchPageSize).
+					Return(&ldap.SearchResult{
+						Entries: []*ldap.Entry{
+							{
+								DN: "CN=Mammals,OU=Users,OU=pinniped-ad,DC=activedirectory,DC=mycompany,DC=example,DC=com",
+								Attributes: []*ldap.EntryAttribute{
+									ldap.NewEntryAttribute("sAMAccountName", []string{"Mammals"}),
+								},
+							},
+							{
+								DN: "CN=Animals,OU=Users,OU=pinniped-ad,DC=activedirectory,DC=mycompany,DC=example,DC=com",
+								Attributes: []*ldap.EntryAttribute{
+									ldap.NewEntryAttribute("sAMAccountName", []string{"Animals"}),
+								},
+							},
+						},
+						Referrals: []string{}, // note that we are not following referrals at this time
+						Controls:  []ldap.Control{},
+					}, nil).Times(1)
+				conn.EXPECT().Close().Times(1)
+			},
+			bindEndUserMocks: func(conn *mockldapconn.MockConn) {
+				conn.EXPECT().Bind(testUserSearchResultDNValue, testUpstreamPassword).Times(1)
+			},
+			wantAuthResponse: expectedAuthResponse(func(r *user.DefaultInfo) {
+				r.Groups = []string{"Animals@activedirectory.mycompany.example.com", "Mammals@activedirectory.mycompany.example.com"}
+			}),
+		},
 		{
 			name:     "override group parsing when domain can't be determined from dn",
 			username: testUpstreamUsername,
@@ -645,7 +701,70 @@ func TestEndUserAuthentication(t *testing.T) {
 					}, nil).Times(1)
 				conn.EXPECT().Close().Times(1)
 			},
-			wantError: "error finding groups: did not find domain components in group dn: no-domain-components",
+			wantError: "error finding groups for user some-upstream-user-dn: did not find domain components in group dn: no-domain-components",
+		},
+		{
+			name:     "override group parsing when entry has multiple values for attribute",
+			username: testUpstreamUsername,
+			password: testUpstreamPassword,
+			providerConfig: providerConfig(func(p *ProviderConfig) {
+				p.GroupSearch.GroupNameAttribute = "sAMAccountName"
+				p.GroupAttributeParsingOverrides = []AttributeParsingOverride{{
+					AttributeName: "sAMAccountName",
+					OverrideFunc:  GroupSAMAccountNameWithDomainSuffix,
+				}}
+			}),
+			searchMocks: func(conn *mockldapconn.MockConn) {
+				conn.EXPECT().Bind(testBindUsername, testBindPassword).Times(1)
+				conn.EXPECT().Search(expectedUserSearch(nil)).Return(exampleUserSearchResult, nil).Times(1)
+				conn.EXPECT().SearchWithPaging(expectedGroupSearch(func(r *ldap.SearchRequest) {
+					r.Attributes = []string{"sAMAccountName"}
+				}), expectedGroupSearchPageSize).
+					Return(&ldap.SearchResult{
+						Entries: []*ldap.Entry{
+							{
+								DN: "no-domain-components",
+								Attributes: []*ldap.EntryAttribute{
+									ldap.NewEntryAttribute("sAMAccountName", []string{"Mammals", "Eukaryotes"}),
+								},
+							},
+						},
+						Referrals: []string{}, // note that we are not following referrals at this time
+						Controls:  []ldap.Control{},
+					}, nil).Times(1)
+				conn.EXPECT().Close().Times(1)
+			},
+			wantError: "error finding groups for user some-upstream-user-dn: found 2 values for attribute \"sAMAccountName\", but expected 1 result",
+		}, {
+			name:     "override group parsing when entry has no values for attribute",
+			username: testUpstreamUsername,
+			password: testUpstreamPassword,
+			providerConfig: providerConfig(func(p *ProviderConfig) {
+				p.GroupSearch.GroupNameAttribute = "sAMAccountName"
+				p.GroupAttributeParsingOverrides = []AttributeParsingOverride{{
+					AttributeName: "sAMAccountName",
+					OverrideFunc:  GroupSAMAccountNameWithDomainSuffix,
+				}}
+			}),
+			searchMocks: func(conn *mockldapconn.MockConn) {
+				conn.EXPECT().Bind(testBindUsername, testBindPassword).Times(1)
+				conn.EXPECT().Search(expectedUserSearch(nil)).Return(exampleUserSearchResult, nil).Times(1)
+				conn.EXPECT().SearchWithPaging(expectedGroupSearch(func(r *ldap.SearchRequest) {
+					r.Attributes = []string{"sAMAccountName"}
+				}), expectedGroupSearchPageSize).
+					Return(&ldap.SearchResult{
+						Entries: []*ldap.Entry{
+							{
+								DN:         "no-domain-components",
+								Attributes: []*ldap.EntryAttribute{},
+							},
+						},
+						Referrals: []string{}, // note that we are not following referrals at this time
+						Controls:  []ldap.Control{},
+					}, nil).Times(1)
+				conn.EXPECT().Close().Times(1)
+			},
+			wantError: "error finding groups for user some-upstream-user-dn: found 0 values for attribute \"sAMAccountName\", but expected 1 result",
 		},
 		{
 			name:           "when dial fails",