Validate display names are unique in federation_domain_watcher.go

internal/controller/supervisorconfig/federation_domain_watcher.go

@@ -7,6 +7,7 @@ import (
 	"context"
 	"fmt"
 	"net/url"
+	"sort"
 	"strings"
 	"time"
 
@@ -17,6 +18,7 @@ import (
 	"k8s.io/apimachinery/pkg/labels"
 	"k8s.io/apimachinery/pkg/types"
 	errorsutil "k8s.io/apimachinery/pkg/util/errors"
+	"k8s.io/apimachinery/pkg/util/sets"
 	"k8s.io/utils/clock"
 
 	configv1alpha1 "go.pinniped.dev/generated/latest/apis/supervisor/config/v1alpha1"
@@ -38,6 +40,7 @@ const (
 	typeOneTLSSecretPerIssuerHostname = "OneTLSSecretPerIssuerHostname"
 	typeIssuerIsUnique                = "IssuerIsUnique"
 	typeIdentityProvidersFound        = "IdentityProvidersFound"
+	typeDisplayNamesUnique            = "DisplayNamesUnique"
 
 	reasonSuccess                                     = "Success"
 	reasonNotReady                                    = "NotReady"
@@ -49,6 +52,7 @@ const (
 	reasonLegacyConfigurationIdentityProviderNotFound = "LegacyConfigurationIdentityProviderNotFound"
 	reasonIdentityProvidersObjectRefsNotFound         = "IdentityProvidersObjectRefsNotFound"
 	reasonIdentityProviderNotSpecified                = "IdentityProviderNotSpecified"
+	reasonDuplicateDisplayNames                       = "DuplicateDisplayNames"
 
 	celTransformerMaxExpressionRuntime = 5 * time.Second
 )
@@ -293,6 +297,8 @@ func (c *federationDomainWatcherController) makeLegacyFederationDomainIssuer(
 		})
 	}
 
+	conditions = c.addDuplicateDisplayNamesCondition(sets.Set[string]{}, conditions)
+
 	// This is the constructor for the backwards compatibility mode.
 	federationDomainIssuer, err := federationdomainproviders.NewFederationDomainIssuerWithDefaultIDP(federationDomain.Spec.Issuer, defaultFederationDomainIdentityProvider)
 	conditions = appendIssuerURLValidCondition(err, conditions)
@@ -306,10 +312,17 @@ func (c *federationDomainWatcherController) makeFederationDomainIssuerWithExplic
 ) (*federationdomainproviders.FederationDomainIssuer, []*configv1alpha1.Condition, error) {
 	federationDomainIdentityProviders := []*federationdomainproviders.FederationDomainIdentityProvider{}
 	idpNotFoundIndices := []int{}
+	displayNames := sets.Set[string]{}
+	duplicateDisplayNames := sets.Set[string]{}
 
 	for index, idp := range federationDomain.Spec.IdentityProviders {
-		// TODO: Validate that all displayNames are unique within this FederationDomain's spec's list of identity providers.
+		if displayNames.Has(idp.DisplayName) {
-		// TODO: Validate that idp.ObjectRef.APIGroup is the expected APIGroup for IDP CRs "idp.supervisor.pinniped.dev"
+			duplicateDisplayNames.Insert(idp.DisplayName)
+		}
+		displayNames.Insert(idp.DisplayName)
+
+		// TODO: Validate that idp.ObjectRef.APIGroup is the expected APIGroup for IDP CRs "idp.supervisor.pinniped.dev" where .pinniped.dev is the configurable suffix
+
 		// Validate that each objectRef resolves to an existing IDP. It does not matter if the IDP itself
 		// is phase=Ready, because it will not be loaded into the cache if not ready. For each objectRef
 		// that does not resolve, put an error on the FederationDomain status.
@@ -361,12 +374,36 @@ func (c *federationDomainWatcherController) makeFederationDomainIssuerWithExplic
 		})
 	}
 
+	conditions = c.addDuplicateDisplayNamesCondition(duplicateDisplayNames, conditions)
+
 	// This is the constructor for any case other than the legacy case, including when there is an empty list of IDPs.
 	federationDomainIssuer, err := federationdomainproviders.NewFederationDomainIssuer(federationDomain.Spec.Issuer, federationDomainIdentityProviders)
 	conditions = appendIssuerURLValidCondition(err, conditions)
 	return federationDomainIssuer, conditions, nil
 }
 
+func (c *federationDomainWatcherController) addDuplicateDisplayNamesCondition(duplicateDisplayNames sets.Set[string], conditions []*configv1alpha1.Condition) []*configv1alpha1.Condition {
+	if duplicateDisplayNames.Len() > 0 {
+		duplicates := duplicateDisplayNames.UnsortedList()
+		sort.Strings(duplicates)
+		conditions = append(conditions, &configv1alpha1.Condition{
+			Type:   typeDisplayNamesUnique,
+			Status: configv1alpha1.ConditionFalse,
+			Reason: reasonDuplicateDisplayNames,
+			Message: fmt.Sprintf("the names specified by .spec.identityProviders[].displayName contain duplicates: %s",
+				strings.Join(duplicates, ", ")),
+		})
+	} else {
+		conditions = append(conditions, &configv1alpha1.Condition{
+			Type:    typeDisplayNamesUnique,
+			Status:  configv1alpha1.ConditionTrue,
+			Reason:  reasonSuccess,
+			Message: "the names specified by .spec.identityProviders[].displayName are unique",
+		})
+	}
+	return conditions
+}
+
 func (c *federationDomainWatcherController) findIDPsUIDByObjectRef(objectRef corev1.TypedLocalObjectReference, namespace string) (types.UID, bool, error) {
 	var idpResourceUID types.UID
 	var foundIDP metav1.Object

internal/controller/supervisorconfig/federation_domain_watcher_test.go

@@ -373,9 +373,32 @@ func TestTestFederationDomainWatcherControllerSync(t *testing.T) {
 		}
 	}
 
+	happyDisplayNamesUniqueCondition := func(time metav1.Time, observedGeneration int64) configv1alpha1.Condition {
+		return configv1alpha1.Condition{
+			Type:               "DisplayNamesUnique",
+			Status:             "True",
+			ObservedGeneration: observedGeneration,
+			LastTransitionTime: time,
+			Reason:             "Success",
+			Message:            "the names specified by .spec.identityProviders[].displayName are unique",
+		}
+	}
+
+	sadDisplayNamesUniqueCondition := func(duplicateNames string, time metav1.Time, observedGeneration int64) configv1alpha1.Condition {
+		return configv1alpha1.Condition{
+			Type:               "DisplayNamesUnique",
+			Status:             "False",
+			ObservedGeneration: observedGeneration,
+			LastTransitionTime: time,
+			Reason:             "DuplicateDisplayNames",
+			Message:            fmt.Sprintf("the names specified by .spec.identityProviders[].displayName contain duplicates: %s", duplicateNames),
+		}
+	}
+
 	allHappyConditionsLegacyConfigurationSuccess := func(issuer string, idpName string, time metav1.Time, observedGeneration int64) []configv1alpha1.Condition {
 		return []configv1alpha1.Condition{
 			// expect them to be sorted alphabetically by type
+			happyDisplayNamesUniqueCondition(frozenMetav1Now, 123),
 			happyIdentityProvidersFoundConditionLegacyConfigurationSuccess(idpName, time, observedGeneration),
 			happyIssuerIsUniqueCondition(time, observedGeneration),
 			happyIssuerURLValidCondition(time, observedGeneration),
@@ -387,6 +410,7 @@ func TestTestFederationDomainWatcherControllerSync(t *testing.T) {
 	allHappyConditionsSuccess := func(issuer string, time metav1.Time, observedGeneration int64) []configv1alpha1.Condition {
 		return []configv1alpha1.Condition{
 			// expect them to be sorted alphabetically by type
+			happyDisplayNamesUniqueCondition(frozenMetav1Now, 123),
 			happyIdentityProvidersFoundConditionSuccess(frozenMetav1Now, 123),
 			happyIssuerIsUniqueCondition(frozenMetav1Now, 123),
 			happyIssuerURLValidCondition(frozenMetav1Now, 123),
@@ -515,6 +539,7 @@ func TestTestFederationDomainWatcherControllerSync(t *testing.T) {
 				expectedFederationDomainStatusUpdate(invalidIssuerURLFederationDomain,
 					configv1alpha1.FederationDomainPhaseError,
 					[]configv1alpha1.Condition{
+						happyDisplayNamesUniqueCondition(frozenMetav1Now, 123),
 						happyIdentityProvidersFoundConditionLegacyConfigurationSuccess(oidcIdentityProvider.Name, frozenMetav1Now, 123),
 						happyIssuerIsUniqueCondition(frozenMetav1Now, 123),
 						sadIssuerURLValidConditionCannotHaveQuery(frozenMetav1Now, 123),
@@ -558,6 +583,7 @@ func TestTestFederationDomainWatcherControllerSync(t *testing.T) {
 				expectedFederationDomainStatusUpdate(invalidIssuerURLFederationDomain,
 					configv1alpha1.FederationDomainPhaseError,
 					[]configv1alpha1.Condition{
+						happyDisplayNamesUniqueCondition(frozenMetav1Now, 123),
 						happyIdentityProvidersFoundConditionLegacyConfigurationSuccess(oidcIdentityProvider.Name, frozenMetav1Now, 123),
 						happyIssuerIsUniqueCondition(frozenMetav1Now, 123),
 						sadIssuerURLValidConditionCannotHaveQuery(frozenMetav1Now, 123),
@@ -599,6 +625,7 @@ func TestTestFederationDomainWatcherControllerSync(t *testing.T) {
 					},
 					configv1alpha1.FederationDomainPhaseError,
 					[]configv1alpha1.Condition{
+						happyDisplayNamesUniqueCondition(frozenMetav1Now, 123),
 						happyIdentityProvidersFoundConditionLegacyConfigurationSuccess(oidcIdentityProvider.Name, frozenMetav1Now, 123),
 						sadIssuerIsUniqueCondition(frozenMetav1Now, 123),
 						happyIssuerURLValidCondition(frozenMetav1Now, 123),
@@ -612,6 +639,7 @@ func TestTestFederationDomainWatcherControllerSync(t *testing.T) {
 					},
 					configv1alpha1.FederationDomainPhaseError,
 					[]configv1alpha1.Condition{
+						happyDisplayNamesUniqueCondition(frozenMetav1Now, 123),
 						happyIdentityProvidersFoundConditionLegacyConfigurationSuccess(oidcIdentityProvider.Name, frozenMetav1Now, 123),
 						sadIssuerIsUniqueCondition(frozenMetav1Now, 123),
 						happyIssuerURLValidCondition(frozenMetav1Now, 123),
@@ -674,6 +702,7 @@ func TestTestFederationDomainWatcherControllerSync(t *testing.T) {
 					},
 					configv1alpha1.FederationDomainPhaseError,
 					[]configv1alpha1.Condition{
+						happyDisplayNamesUniqueCondition(frozenMetav1Now, 123),
 						happyIdentityProvidersFoundConditionLegacyConfigurationSuccess(oidcIdentityProvider.Name, frozenMetav1Now, 123),
 						happyIssuerIsUniqueCondition(frozenMetav1Now, 123),
 						happyIssuerURLValidCondition(frozenMetav1Now, 123),
@@ -687,6 +716,7 @@ func TestTestFederationDomainWatcherControllerSync(t *testing.T) {
 					},
 					configv1alpha1.FederationDomainPhaseError,
 					[]configv1alpha1.Condition{
+						happyDisplayNamesUniqueCondition(frozenMetav1Now, 123),
 						happyIdentityProvidersFoundConditionLegacyConfigurationSuccess(oidcIdentityProvider.Name, frozenMetav1Now, 123),
 						happyIssuerIsUniqueCondition(frozenMetav1Now, 123),
 						happyIssuerURLValidCondition(frozenMetav1Now, 123),
@@ -700,6 +730,7 @@ func TestTestFederationDomainWatcherControllerSync(t *testing.T) {
 					},
 					configv1alpha1.FederationDomainPhaseError,
 					[]configv1alpha1.Condition{
+						happyDisplayNamesUniqueCondition(frozenMetav1Now, 123),
 						happyIdentityProvidersFoundConditionLegacyConfigurationSuccess(oidcIdentityProvider.Name, frozenMetav1Now, 123),
 						unknownIssuerIsUniqueCondition(frozenMetav1Now, 123),
 						sadIssuerURLValidConditionCannotParse(frozenMetav1Now, 123),
@@ -727,6 +758,7 @@ func TestTestFederationDomainWatcherControllerSync(t *testing.T) {
 				expectedFederationDomainStatusUpdate(federationDomain1,
 					configv1alpha1.FederationDomainPhaseError,
 					[]configv1alpha1.Condition{
+						happyDisplayNamesUniqueCondition(frozenMetav1Now, 123),
 						sadIdentityProvidersFoundConditionLegacyConfigurationIdentityProviderNotFound(frozenMetav1Now, 123),
 						happyIssuerIsUniqueCondition(frozenMetav1Now, 123),
 						happyIssuerURLValidCondition(frozenMetav1Now, 123),
@@ -737,6 +769,7 @@ func TestTestFederationDomainWatcherControllerSync(t *testing.T) {
 				expectedFederationDomainStatusUpdate(federationDomain2,
 					configv1alpha1.FederationDomainPhaseError,
 					[]configv1alpha1.Condition{
+						happyDisplayNamesUniqueCondition(frozenMetav1Now, 123),
 						sadIdentityProvidersFoundConditionLegacyConfigurationIdentityProviderNotFound(frozenMetav1Now, 123),
 						happyIssuerIsUniqueCondition(frozenMetav1Now, 123),
 						happyIssuerURLValidCondition(frozenMetav1Now, 123),
@@ -759,6 +792,7 @@ func TestTestFederationDomainWatcherControllerSync(t *testing.T) {
 				expectedFederationDomainStatusUpdate(federationDomain1,
 					configv1alpha1.FederationDomainPhaseError,
 					[]configv1alpha1.Condition{
+						happyDisplayNamesUniqueCondition(frozenMetav1Now, 123),
 						sadIdentityProvidersFoundConditionIdentityProviderNotSpecified(3, frozenMetav1Now, 123),
 						happyIssuerIsUniqueCondition(frozenMetav1Now, 123),
 						happyIssuerURLValidCondition(frozenMetav1Now, 123),
@@ -812,6 +846,7 @@ func TestTestFederationDomainWatcherControllerSync(t *testing.T) {
 					},
 					configv1alpha1.FederationDomainPhaseError,
 					[]configv1alpha1.Condition{
+						happyDisplayNamesUniqueCondition(frozenMetav1Now, 123),
 						sadIdentityProvidersFoundConditionIdentityProvidersObjectRefsNotFound(
 							`.spec.identityProviders[].objectRef identifies resource(s) that cannot be found: `+
 								`.spec.identityProviders[0] with displayName "cant-find-me", `+
@@ -895,6 +930,78 @@ func TestTestFederationDomainWatcherControllerSync(t *testing.T) {
 				),
 			},
 		},
+		{
+			name: "the federation domain has duplicate display names for IDPs",
+			inputObjects: []runtime.Object{
+				oidcIdentityProvider,
+				ldapIdentityProvider,
+				adIdentityProvider,
+				&configv1alpha1.FederationDomain{
+					ObjectMeta: metav1.ObjectMeta{Name: "config1", Namespace: namespace, Generation: 123},
+					Spec: configv1alpha1.FederationDomainSpec{
+						Issuer: "https://issuer1.com",
+						IdentityProviders: []configv1alpha1.FederationDomainIdentityProvider{
+							{
+								DisplayName: "duplicate1",
+								ObjectRef: corev1.TypedLocalObjectReference{
+									APIGroup: pointer.String(apiGroupSupervisor),
+									Kind:     "OIDCIdentityProvider",
+									Name:     oidcIdentityProvider.Name,
+								},
+							},
+							{
+								DisplayName: "duplicate1",
+								ObjectRef: corev1.TypedLocalObjectReference{
+									APIGroup: pointer.String(apiGroupSupervisor),
+									Kind:     "LDAPIdentityProvider",
+									Name:     ldapIdentityProvider.Name,
+								},
+							},
+							{
+								DisplayName: "unique",
+								ObjectRef: corev1.TypedLocalObjectReference{
+									APIGroup: pointer.String(apiGroupSupervisor),
+									Kind:     "ActiveDirectoryIdentityProvider",
+									Name:     adIdentityProvider.Name,
+								},
+							},
+							{
+								DisplayName: "duplicate2",
+								ObjectRef: corev1.TypedLocalObjectReference{
+									APIGroup: pointer.String(apiGroupSupervisor),
+									Kind:     "LDAPIdentityProvider",
+									Name:     ldapIdentityProvider.Name,
+								},
+							},
+							{
+								DisplayName: "duplicate2",
+								ObjectRef: corev1.TypedLocalObjectReference{
+									APIGroup: pointer.String(apiGroupSupervisor),
+									Kind:     "ActiveDirectoryIdentityProvider",
+									Name:     adIdentityProvider.Name,
+								},
+							},
+						},
+					},
+				},
+			},
+			wantFDIssuers: []*federationdomainproviders.FederationDomainIssuer{},
+			wantStatusUpdates: []*configv1alpha1.FederationDomain{
+				expectedFederationDomainStatusUpdate(
+					&configv1alpha1.FederationDomain{
+						ObjectMeta: metav1.ObjectMeta{Name: "config1", Namespace: namespace, Generation: 123},
+					},
+					configv1alpha1.FederationDomainPhaseError,
+					[]configv1alpha1.Condition{
+						sadDisplayNamesUniqueCondition("duplicate1, duplicate2", frozenMetav1Now, 123),
+						happyIdentityProvidersFoundConditionSuccess(frozenMetav1Now, 123),
+						happyIssuerIsUniqueCondition(frozenMetav1Now, 123),
+						happyIssuerURLValidCondition(frozenMetav1Now, 123),
+						happyOneTLSSecretPerIssuerHostnameCondition(frozenMetav1Now, 123),
+						sadReadyCondition(frozenMetav1Now, 123),
+					}),
+			},
+		},
 		{
 			name: "the federation domain specifies illegal const type, which shouldn't really happen since the CRD validates it",
 			inputObjects: []runtime.Object{