From 31d67a1af33549c6a9856b17f88ccd33eb6af255 Mon Sep 17 00:00:00 2001 From: Ryan Richard Date: Wed, 12 Jul 2023 09:43:33 -0700 Subject: [PATCH] Validate display names are unique in federation_domain_watcher.go --- .../federation_domain_watcher.go | 41 ++++++- .../federation_domain_watcher_test.go | 107 ++++++++++++++++++ 2 files changed, 146 insertions(+), 2 deletions(-) diff --git a/internal/controller/supervisorconfig/federation_domain_watcher.go b/internal/controller/supervisorconfig/federation_domain_watcher.go index c7f478b1..44d3f9fc 100644 --- a/internal/controller/supervisorconfig/federation_domain_watcher.go +++ b/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. - // TODO: Validate that idp.ObjectRef.APIGroup is the expected APIGroup for IDP CRs "idp.supervisor.pinniped.dev" + if displayNames.Has(idp.DisplayName) { + 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 diff --git a/internal/controller/supervisorconfig/federation_domain_watcher_test.go b/internal/controller/supervisorconfig/federation_domain_watcher_test.go index 6d7f75b8..c460f58d 100644 --- a/internal/controller/supervisorconfig/federation_domain_watcher_test.go +++ b/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{