diff --git a/internal/celtransformer/celformer.go b/internal/celtransformer/celformer.go
index 2c1bf409d..bdf351b48 100644
--- a/internal/celtransformer/celformer.go
+++ b/internal/celtransformer/celformer.go
@@ -29,7 +29,7 @@ const (
constStringVariableName = "strConst"
constStringListVariableName = "strListConst"
- defaultPolicyRejectedAuthMessage = "Authentication was rejected by a configured policy"
+ DefaultPolicyRejectedAuthMessage = "Authentication was rejected by a configured policy"
)
// CELTransformer can compile any number of transformation expression pipelines.
@@ -96,6 +96,10 @@ type CELTransformation interface {
compile(transformer *CELTransformer, consts *TransformationConstants) (idtransform.IdentityTransformation, error)
}
+var _ CELTransformation = (*UsernameTransformation)(nil)
+var _ CELTransformation = (*GroupsTransformation)(nil)
+var _ CELTransformation = (*AllowAuthenticationPolicy)(nil)
+
// UsernameTransformation is a CEL expression that can transform a username (or leave it unchanged).
// It implements CELTransformation.
type UsernameTransformation struct {
@@ -290,7 +294,7 @@ func (c *compiledAllowAuthenticationPolicy) Evaluate(ctx context.Context, userna
}
if !boolValue {
if len(c.rejectedAuthenticationMessage) == 0 {
- result.RejectedAuthenticationMessage = defaultPolicyRejectedAuthMessage
+ result.RejectedAuthenticationMessage = DefaultPolicyRejectedAuthMessage
} else {
result.RejectedAuthenticationMessage = c.rejectedAuthenticationMessage
}
diff --git a/internal/celtransformer/celformer_test.go b/internal/celtransformer/celformer_test.go
index d781dae05..227ed3928 100644
--- a/internal/celtransformer/celformer_test.go
+++ b/internal/celtransformer/celformer_test.go
@@ -7,6 +7,7 @@ import (
"context"
"fmt"
"runtime"
+ "sort"
"sync"
"testing"
"time"
@@ -113,7 +114,7 @@ func TestTransformer(t *testing.T) {
&GroupsTransformation{Expression: `groups + [username + "2"]`}, // by the time this expression runs, the username was already changed to "other"
},
wantUsername: "other",
- wantGroups: []string{"admins", "developers", "other", "ryan", "other2"},
+ wantGroups: []string{"admins", "developers", "other", "other2", "ryan"},
},
{
name: "any transformation can use the provided constants as variables",
@@ -135,7 +136,7 @@ func TestTransformer(t *testing.T) {
&AllowAuthenticationPolicy{Expression: `strConst.x == "abc"`},
},
wantUsername: "abcuvw",
- wantGroups: []string{"abc", "def", "xyz", "123"},
+ wantGroups: []string{"123", "abc", "def", "xyz"},
},
{
name: "the CEL string extensions are enabled for use in the expressions",
@@ -297,7 +298,7 @@ func TestTransformer(t *testing.T) {
&GroupsTransformation{Expression: `groups + ["new-group"]`},
},
wantUsername: "ryan",
- wantGroups: []string{"admins", "developers", "other", "new-group"},
+ wantGroups: []string{"admins", "developers", "new-group", "other"},
},
{
name: "a nil passed as groups will be converted to an empty list",
@@ -340,7 +341,7 @@ func TestTransformer(t *testing.T) {
&GroupsTransformation{Expression: `groups + [strConst.groupToAlwaysAdd]`},
},
wantUsername: "ryan",
- wantGroups: []string{"admins", "developers", "other", "new-group"},
+ wantGroups: []string{"admins", "developers", "new-group", "other"},
},
{
name: "can add a group but only if they already belong to another group - when the user does belong to that other group",
@@ -350,7 +351,7 @@ func TestTransformer(t *testing.T) {
&GroupsTransformation{Expression: `"other" in groups ? groups + ["new-group"] : groups`},
},
wantUsername: "ryan",
- wantGroups: []string{"admins", "developers", "other", "new-group"},
+ wantGroups: []string{"admins", "developers", "new-group", "other"},
},
{
name: "can add a group but only if they already belong to another group - when the user does NOT belong to that other group",
@@ -424,7 +425,7 @@ func TestTransformer(t *testing.T) {
&AllowAuthenticationPolicy{Expression: `["foobar", "foobaz", "foobat"].all(g, g in groups)`, RejectedAuthenticationMessage: `Only users who belong to all groups in a list are allowed`},
},
wantUsername: "ryan",
- wantGroups: []string{"admins", "developers", "other", "foobar", "foobaz", "foobat"},
+ wantGroups: []string{"admins", "developers", "foobar", "foobat", "foobaz", "other"},
},
{
name: "can reject auth unless the user belongs to all of the groups in a list - when the user does NOT meet the criteria",
@@ -820,6 +821,7 @@ func TestTypicalPerformanceAndThreadSafety(t *testing.T) {
groups = append(groups, fmt.Sprintf("g%d", i))
wantGroups = append(wantGroups, fmt.Sprintf("group_prefix:g%d", i))
}
+ sort.Strings(wantGroups)
// Before looking at performance, check that the behavior of the function is correct.
result, err := pipeline.Evaluate(context.Background(), "ryan", groups)
diff --git a/internal/controller/supervisorconfig/federation_domain_watcher.go b/internal/controller/supervisorconfig/federation_domain_watcher.go
index 1546f9128..4d10953d3 100644
--- a/internal/controller/supervisorconfig/federation_domain_watcher.go
+++ b/internal/controller/supervisorconfig/federation_domain_watcher.go
@@ -45,6 +45,7 @@ const (
typeIdentityProvidersObjectRefKindValid = "IdentityProvidersObjectRefKindValid"
typeTransformsConstantsNamesUnique = "TransformsConstantsNamesUnique"
typeTransformsExpressionsValid = "TransformsExpressionsValid"
+ typeTransformsExamplesPassed = "TransformsExamplesPassed"
reasonSuccess = "Success"
reasonNotReady = "NotReady"
@@ -61,6 +62,7 @@ const (
reasonKindUnrecognized = "KindUnrecognized"
reasonDuplicateConstantsNames = "DuplicateConstantsNames"
reasonInvalidTransformsExpressions = "InvalidTransformsExpressions"
+ reasonTransformsExamplesFailed = "TransformsExamplesFailed"
kindLDAPIdentityProvider = "LDAPIdentityProvider"
kindOIDCIdentityProvider = "OIDCIdentityProvider"
@@ -328,10 +330,12 @@ func (c *federationDomainWatcherController) makeLegacyFederationDomainIssuer(
conditions = appendIdentityProviderObjectRefKindCondition(c.sortedAllowedKinds(), []string{}, conditions)
conditions = appendTransformsConstantsNamesUniqueCondition(sets.Set[string]{}, conditions)
conditions = appendTransformsExpressionsValidCondition([]string{}, conditions)
+ conditions = appendTransformsExamplesPassedCondition([]string{}, conditions)
return federationDomainIssuer, conditions, nil
}
+//nolint:funlen
func (c *federationDomainWatcherController) makeFederationDomainIssuerWithExplicitIDPs(
ctx context.Context,
federationDomain *configv1alpha1.FederationDomain,
@@ -490,7 +494,7 @@ func (c *federationDomainWatcherController) makeTransformationPipelineAndEvaluat
return nil, false, nil, err
}
- allExamplesPassed, conditions := c.evaluateExamples(ctx, idp, pipeline, conditions)
+ allExamplesPassed, conditions := c.evaluateExamples(ctx, idp, idpIndex, pipeline, conditions)
return pipeline, allExamplesPassed, conditions, nil
}
@@ -580,83 +584,76 @@ func (c *federationDomainWatcherController) makeTransformationPipeline(
func (c *federationDomainWatcherController) evaluateExamples(
ctx context.Context,
idp configv1alpha1.FederationDomainIdentityProvider,
+ idpIndex int,
pipeline *idtransform.TransformationPipeline,
conditions []*configv1alpha1.Condition,
) (bool, []*configv1alpha1.Condition) {
+ const errorFmt = ".spec.identityProviders[%d].transforms.examples[%d] example failed:\nexpected: %s\nactual: %s"
+ examplesErrors := []string{}
+
if pipeline == nil {
- // TODO cannot evaluate examples, but still need to write a condition for it
+ // Unable to evaluate the conditions where the pipeline of expressions was invalid.
+ conditions = appendTransformsExamplesPassedCondition(nil, conditions)
return false, conditions
}
// Run all the provided transform examples. If any fail, put errors on the FederationDomain status.
- examplesErrors := []string{}
- for examplesIndex, e := range idp.Transforms.Examples {
- result, _ := pipeline.Evaluate(ctx, e.Username, e.Groups)
- // TODO: handle err
+ for exIndex, e := range idp.Transforms.Examples {
+ result, err := pipeline.Evaluate(ctx, e.Username, e.Groups)
+ if err != nil {
+ examplesErrors = append(examplesErrors, fmt.Sprintf(errorFmt, idpIndex, exIndex,
+ "no transformation errors",
+ fmt.Sprintf("transformations resulted in an unexpected error %q", err.Error())))
+ continue
+ }
resultWasAuthRejected := !result.AuthenticationAllowed
- if e.Expects.Rejected && !resultWasAuthRejected { //nolint:gocritic,nestif
- // TODO: handle this failed example
- examplesErrors = append(examplesErrors, "TODO")
- plog.Warning("FederationDomain identity provider transformations example failed: expected authentication to be rejected but it was not",
- "idpDisplayName", idp.DisplayName,
- "exampleIndex", examplesIndex,
- "expectedRejected", e.Expects.Rejected,
- "actualRejectedResult", resultWasAuthRejected,
- "expectedMessage", e.Expects.Message,
- "actualMessageResult", result.RejectedAuthenticationMessage,
- )
- } else if !e.Expects.Rejected && resultWasAuthRejected {
- // TODO: handle this failed example
- examplesErrors = append(examplesErrors, "TODO")
- plog.Warning("FederationDomain identity provider transformations example failed: expected authentication not to be rejected but it was rejected",
- "idpDisplayName", idp.DisplayName,
- "exampleIndex", examplesIndex,
- "expectedRejected", e.Expects.Rejected,
- "actualRejectedResult", resultWasAuthRejected,
- "expectedMessage", e.Expects.Message,
- "actualMessageResult", result.RejectedAuthenticationMessage,
- )
- } else if e.Expects.Rejected && resultWasAuthRejected && e.Expects.Message != result.RejectedAuthenticationMessage {
- // TODO: when expected message is blank, then treat it like it expects the default message
- // TODO: handle this failed example
- examplesErrors = append(examplesErrors, "TODO")
- plog.Warning("FederationDomain identity provider transformations example failed: expected a different authentication rejection message",
- "idpDisplayName", idp.DisplayName,
- "exampleIndex", examplesIndex,
- "expectedRejected", e.Expects.Rejected,
- "actualRejectedResult", resultWasAuthRejected,
- "expectedMessage", e.Expects.Message,
- "actualMessageResult", result.RejectedAuthenticationMessage,
- )
- } else if result.AuthenticationAllowed {
+
+ if e.Expects.Rejected && !resultWasAuthRejected {
+ examplesErrors = append(examplesErrors,
+ fmt.Sprintf(errorFmt, idpIndex, exIndex, "authentication to be rejected", "authentication was not rejected"))
+ continue
+ }
+
+ if !e.Expects.Rejected && resultWasAuthRejected {
+ examplesErrors = append(examplesErrors, fmt.Sprintf(errorFmt, idpIndex, exIndex,
+ "authentication not to be rejected",
+ fmt.Sprintf("authentication was rejected with message %q", result.RejectedAuthenticationMessage)))
+ continue
+ }
+
+ expectedRejectionMessage := e.Expects.Message
+ if len(expectedRejectionMessage) == 0 {
+ expectedRejectionMessage = celtransformer.DefaultPolicyRejectedAuthMessage
+ }
+ if e.Expects.Rejected && resultWasAuthRejected && expectedRejectionMessage != result.RejectedAuthenticationMessage {
+ examplesErrors = append(examplesErrors, fmt.Sprintf(errorFmt, idpIndex, exIndex,
+ fmt.Sprintf("authentication rejection message %q", expectedRejectionMessage),
+ fmt.Sprintf("authentication rejection message %q", result.RejectedAuthenticationMessage)))
+ continue
+ }
+
+ if result.AuthenticationAllowed {
// In the case where the user expected the auth to be allowed and it was allowed, then compare
// the expected username and group names to the actual username and group names.
- // TODO: when both of these fail, put both errors onto the status (not just the first one)
if e.Expects.Username != result.Username {
- // TODO: handle this failed example
- examplesErrors = append(examplesErrors, "TODO")
- plog.Warning("FederationDomain identity provider transformations example failed: expected a different transformed username",
- "idpDisplayName", idp.DisplayName,
- "exampleIndex", examplesIndex,
- "expectedUsername", e.Expects.Username,
- "actualUsernameResult", result.Username,
- )
+ examplesErrors = append(examplesErrors, fmt.Sprintf(errorFmt, idpIndex, exIndex,
+ fmt.Sprintf("username %q", e.Expects.Username),
+ fmt.Sprintf("username %q", result.Username)))
}
- if !stringSlicesEqual(e.Expects.Groups, result.Groups) {
- // TODO: Do we need to make this insensitive to ordering, or should the transformations evaluator be changed to always return sorted group names at the end of the pipeline?
- // TODO: What happens if the user did not write any group expectation? Treat it like expecting an empty list of groups?
- // TODO: handle this failed example
- examplesErrors = append(examplesErrors, "TODO")
- plog.Warning("FederationDomain identity provider transformations example failed: expected a different transformed groups list",
- "idpDisplayName", idp.DisplayName,
- "exampleIndex", examplesIndex,
- "expectedGroups", e.Expects.Groups,
- "actualGroupsResult", result.Groups,
- )
+ expectedGroups := e.Expects.Groups
+ if expectedGroups == nil {
+ expectedGroups = []string{}
+ }
+ if !stringSetsEqual(expectedGroups, result.Groups) {
+ examplesErrors = append(examplesErrors, fmt.Sprintf(errorFmt, idpIndex, exIndex,
+ fmt.Sprintf("groups [%s]", strings.Join(sortAndQuote(expectedGroups), ", ")),
+ fmt.Sprintf("groups [%s]", strings.Join(sortAndQuote(result.Groups), ", "))))
}
}
}
+ conditions = appendTransformsExamplesPassedCondition(examplesErrors, conditions)
+
return len(examplesErrors) == 0, conditions
}
@@ -724,6 +721,34 @@ func appendTransformsExpressionsValidCondition(errors []string, conditions []*co
return conditions
}
+func appendTransformsExamplesPassedCondition(errors []string, conditions []*configv1alpha1.Condition) []*configv1alpha1.Condition {
+ switch {
+ case errors == nil:
+ conditions = append(conditions, &configv1alpha1.Condition{
+ Type: typeTransformsExamplesPassed,
+ Status: configv1alpha1.ConditionUnknown,
+ Reason: reasonUnableToValidate,
+ Message: "unable to check if the examples specified by .spec.identityProviders[].transforms.examples[] had errors because an expression was invalid",
+ })
+ case len(errors) > 0:
+ conditions = append(conditions, &configv1alpha1.Condition{
+ Type: typeTransformsExamplesPassed,
+ Status: configv1alpha1.ConditionFalse,
+ Reason: reasonTransformsExamplesFailed,
+ Message: fmt.Sprintf("the examples specified by .spec.identityProviders[].transforms.examples[] had errors:\n\n%s",
+ strings.Join(errors, "\n\n")),
+ })
+ default:
+ conditions = append(conditions, &configv1alpha1.Condition{
+ Type: typeTransformsExamplesPassed,
+ Status: configv1alpha1.ConditionTrue,
+ Reason: reasonSuccess,
+ Message: "the examples specified by .spec.identityProviders[].transforms.examples[] had no errors",
+ })
+ }
+ return conditions
+}
+
func appendIdentityProviderDuplicateDisplayNamesCondition(duplicateDisplayNames sets.Set[string], conditions []*configv1alpha1.Condition) []*configv1alpha1.Condition {
if duplicateDisplayNames.Len() > 0 {
conditions = append(conditions, &configv1alpha1.Condition{
@@ -948,14 +973,8 @@ func hadErrorCondition(conditions []*configv1alpha1.Condition) bool {
return false
}
-func stringSlicesEqual(a []string, b []string) bool {
- if len(a) != len(b) {
- return false
- }
- for i, itemFromA := range a {
- if b[i] != itemFromA {
- return false
- }
- }
- return true
+func stringSetsEqual(a []string, b []string) bool {
+ aSet := sets.New(a...)
+ bSet := sets.New(b...)
+ return aSet.Equal(bSet)
}
diff --git a/internal/controller/supervisorconfig/federation_domain_watcher_test.go b/internal/controller/supervisorconfig/federation_domain_watcher_test.go
index 15031ba2c..2ff07cd39 100644
--- a/internal/controller/supervisorconfig/federation_domain_watcher_test.go
+++ b/internal/controller/supervisorconfig/federation_domain_watcher_test.go
@@ -442,6 +442,39 @@ func TestTestFederationDomainWatcherControllerSync(t *testing.T) {
}
}
+ happyTransformationExamplesCondition := func(time metav1.Time, observedGeneration int64) configv1alpha1.Condition {
+ return configv1alpha1.Condition{
+ Type: "TransformsExamplesPassed",
+ Status: "True",
+ ObservedGeneration: observedGeneration,
+ LastTransitionTime: time,
+ Reason: "Success",
+ Message: "the examples specified by .spec.identityProviders[].transforms.examples[] had no errors",
+ }
+ }
+
+ sadTransformationExamplesCondition := func(errorMessages string, time metav1.Time, observedGeneration int64) configv1alpha1.Condition {
+ return configv1alpha1.Condition{
+ Type: "TransformsExamplesPassed",
+ Status: "False",
+ ObservedGeneration: observedGeneration,
+ LastTransitionTime: time,
+ Reason: "TransformsExamplesFailed",
+ Message: fmt.Sprintf("the examples specified by .spec.identityProviders[].transforms.examples[] had errors:\n\n%s", errorMessages),
+ }
+ }
+
+ unknownTransformationExamplesCondition := func(time metav1.Time, observedGeneration int64) configv1alpha1.Condition {
+ return configv1alpha1.Condition{
+ Type: "TransformsExamplesPassed",
+ Status: "Unknown",
+ ObservedGeneration: observedGeneration,
+ LastTransitionTime: time,
+ Reason: "UnableToValidate",
+ Message: "unable to check if the examples specified by .spec.identityProviders[].transforms.examples[] had errors because an expression was invalid",
+ }
+ }
+
happyAPIGroupSuffixCondition := func(time metav1.Time, observedGeneration int64) configv1alpha1.Condition {
return configv1alpha1.Condition{
Type: "IdentityProvidersObjectRefAPIGroupSuffixValid",
@@ -511,6 +544,7 @@ func TestTestFederationDomainWatcherControllerSync(t *testing.T) {
allHappyConditionsSuccess := func(issuer string, time metav1.Time, observedGeneration int64) []configv1alpha1.Condition {
return sortConditionsByType([]configv1alpha1.Condition{
+ happyTransformationExamplesCondition(frozenMetav1Now, 123),
happyTransformationExpressionsCondition(frozenMetav1Now, 123),
happyConstNamesUniqueCondition(frozenMetav1Now, 123),
happyKindCondition(frozenMetav1Now, 123),
@@ -1330,7 +1364,248 @@ func TestTestFederationDomainWatcherControllerSync(t *testing.T) {
spec.identityProvider[0].transforms.expressions[3].expression was invalid:
CEL expression compile error: ERROR: :1:7: Syntax error: mismatched input 'not' expecting
| still not a valid cel expression
- | ......^`),
+ | ......^`,
+ ),
+ frozenMetav1Now, 123),
+ unknownTransformationExamplesCondition(frozenMetav1Now, 123),
+ sadReadyCondition(frozenMetav1Now, 123),
+ }),
+ ),
+ },
+ },
+ {
+ name: "the federation domain has transformation examples which don't pass",
+ inputObjects: []runtime.Object{
+ oidcIdentityProvider,
+ &configv1alpha1.FederationDomain{
+ ObjectMeta: metav1.ObjectMeta{Name: "config1", Namespace: namespace, Generation: 123},
+ Spec: configv1alpha1.FederationDomainSpec{
+ Issuer: "https://issuer1.com",
+ IdentityProviders: []configv1alpha1.FederationDomainIdentityProvider{
+ {
+ DisplayName: "name1",
+ ObjectRef: corev1.TypedLocalObjectReference{
+ APIGroup: pointer.String(apiGroupSupervisor),
+ Kind: "OIDCIdentityProvider",
+ Name: oidcIdentityProvider.Name,
+ },
+ Transforms: configv1alpha1.FederationDomainTransforms{
+ Expressions: []configv1alpha1.FederationDomainTransformsExpression{
+ {Type: "policy/v1", Expression: `username == "ryan" || username == "rejectMeWithDefaultMessage"`, Message: "only ryan allowed"},
+ {Type: "policy/v1", Expression: `username != "rejectMeWithDefaultMessage"`}, // no message specified
+ {Type: "username/v1", Expression: `"pre:" + username`},
+ {Type: "groups/v1", Expression: `groups.map(g, "pre:" + g)`},
+ },
+ Examples: []configv1alpha1.FederationDomainTransformsExample{
+ { // this example should pass
+ Username: "ryan",
+ Groups: []string{"a", "b"},
+ Expects: configv1alpha1.FederationDomainTransformsExampleExpects{
+ Username: "pre:ryan",
+ Groups: []string{"pre:b", "pre:a", "pre:b", "pre:a"}, // order and repeats don't matter, treated like a set
+ Rejected: false,
+ },
+ },
+ { // this example should pass
+ Username: "other",
+ Expects: configv1alpha1.FederationDomainTransformsExampleExpects{
+ Rejected: true,
+ Message: "only ryan allowed",
+ },
+ },
+ { // this example should fail because it expects the user to be rejected but the user was actually not rejected
+ Username: "ryan",
+ Groups: []string{"a", "b"},
+ Expects: configv1alpha1.FederationDomainTransformsExampleExpects{
+ Rejected: true,
+ Message: "this input is ignored in this case",
+ },
+ },
+ { // this example should fail because it expects the user not to be rejected but they were actually rejected
+ Username: "other",
+ Groups: []string{"a", "b"},
+ Expects: configv1alpha1.FederationDomainTransformsExampleExpects{
+ Username: "pre:other",
+ Groups: []string{"pre:a", "pre:b"},
+ Rejected: false,
+ },
+ },
+ { // this example should fail because it expects the wrong rejection message
+ Username: "other",
+ Groups: []string{"a", "b"},
+ Expects: configv1alpha1.FederationDomainTransformsExampleExpects{
+ Rejected: true,
+ Message: "wrong message",
+ },
+ },
+ { // this example should pass even though it does not make any assertion about the rejection message
+ // because the message assertions defaults to asserting the default rejection message
+ Username: "rejectMeWithDefaultMessage",
+ Groups: []string{"a", "b"},
+ Expects: configv1alpha1.FederationDomainTransformsExampleExpects{
+ Rejected: true,
+ },
+ },
+ { // this example should fail because it expects both the wrong username and groups
+ Username: "ryan",
+ Groups: []string{"b", "a"},
+ Expects: configv1alpha1.FederationDomainTransformsExampleExpects{
+ Username: "wrong",
+ Groups: []string{},
+ Rejected: false,
+ },
+ },
+ { // this example should fail because it expects the wrong username only
+ Username: "ryan",
+ Groups: []string{"a", "b"},
+ Expects: configv1alpha1.FederationDomainTransformsExampleExpects{
+ Username: "wrong",
+ Groups: []string{"pre:b", "pre:a"},
+ Rejected: false,
+ },
+ },
+ { // this example should fail because it expects the wrong groups only
+ Username: "ryan",
+ Groups: []string{"b", "a"},
+ Expects: configv1alpha1.FederationDomainTransformsExampleExpects{
+ Username: "pre:ryan",
+ Groups: []string{"wrong2", "wrong1"},
+ Rejected: false,
+ },
+ },
+ { // this example should fail because it does not expect anything but the auth actually was successful
+ Username: "ryan",
+ Groups: []string{"b", "a"},
+ Expects: configv1alpha1.FederationDomainTransformsExampleExpects{},
+ },
+ },
+ },
+ },
+ },
+ },
+ },
+ },
+ wantFDIssuers: []*federationdomainproviders.FederationDomainIssuer{},
+ wantStatusUpdates: []*configv1alpha1.FederationDomain{
+ expectedFederationDomainStatusUpdate(
+ &configv1alpha1.FederationDomain{
+ ObjectMeta: metav1.ObjectMeta{Name: "config1", Namespace: namespace, Generation: 123},
+ },
+ configv1alpha1.FederationDomainPhaseError,
+ replaceConditions(
+ allHappyConditionsSuccess("https://issuer1.com", frozenMetav1Now, 123),
+ []configv1alpha1.Condition{
+ sadTransformationExamplesCondition(
+ here.Doc(
+ `.spec.identityProviders[0].transforms.examples[2] example failed:
+ expected: authentication to be rejected
+ actual: authentication was not rejected
+
+ .spec.identityProviders[0].transforms.examples[3] example failed:
+ expected: authentication not to be rejected
+ actual: authentication was rejected with message "only ryan allowed"
+
+ .spec.identityProviders[0].transforms.examples[4] example failed:
+ expected: authentication rejection message "wrong message"
+ actual: authentication rejection message "only ryan allowed"
+
+ .spec.identityProviders[0].transforms.examples[6] example failed:
+ expected: username "wrong"
+ actual: username "pre:ryan"
+
+ .spec.identityProviders[0].transforms.examples[6] example failed:
+ expected: groups []
+ actual: groups ["pre:a", "pre:b"]
+
+ .spec.identityProviders[0].transforms.examples[7] example failed:
+ expected: username "wrong"
+ actual: username "pre:ryan"
+
+ .spec.identityProviders[0].transforms.examples[8] example failed:
+ expected: groups ["wrong1", "wrong2"]
+ actual: groups ["pre:a", "pre:b"]
+
+ .spec.identityProviders[0].transforms.examples[9] example failed:
+ expected: username ""
+ actual: username "pre:ryan"
+
+ .spec.identityProviders[0].transforms.examples[9] example failed:
+ expected: groups []
+ actual: groups ["pre:a", "pre:b"]`,
+ ),
+ frozenMetav1Now, 123),
+ sadReadyCondition(frozenMetav1Now, 123),
+ }),
+ ),
+ },
+ },
+ {
+ name: "the federation domain has transformation expressions that return illegal values with examples which exercise them",
+ inputObjects: []runtime.Object{
+ oidcIdentityProvider,
+ &configv1alpha1.FederationDomain{
+ ObjectMeta: metav1.ObjectMeta{Name: "config1", Namespace: namespace, Generation: 123},
+ Spec: configv1alpha1.FederationDomainSpec{
+ Issuer: "https://issuer1.com",
+ IdentityProviders: []configv1alpha1.FederationDomainIdentityProvider{
+ {
+ DisplayName: "name1",
+ ObjectRef: corev1.TypedLocalObjectReference{
+ APIGroup: pointer.String(apiGroupSupervisor),
+ Kind: "OIDCIdentityProvider",
+ Name: oidcIdentityProvider.Name,
+ },
+ Transforms: configv1alpha1.FederationDomainTransforms{
+ Expressions: []configv1alpha1.FederationDomainTransformsExpression{
+ {Type: "username/v1", Expression: `username == "ryan" ? "" : username`}, // not allowed to return an empty string as the transformed username
+ },
+ Examples: []configv1alpha1.FederationDomainTransformsExample{
+ { // every example which encounters an unexpected error should fail because the transformation pipeline returned an error
+ Username: "ryan",
+ Groups: []string{"a", "b"},
+ Expects: configv1alpha1.FederationDomainTransformsExampleExpects{},
+ },
+ { // every example which encounters an unexpected error should fail because the transformation pipeline returned an error
+ Username: "ryan",
+ Groups: []string{"a", "b"},
+ Expects: configv1alpha1.FederationDomainTransformsExampleExpects{},
+ },
+ { // this should pass
+ Username: "other",
+ Groups: []string{"a", "b"},
+ Expects: configv1alpha1.FederationDomainTransformsExampleExpects{
+ Username: "other",
+ Groups: []string{"a", "b"},
+ Rejected: false,
+ },
+ },
+ },
+ },
+ },
+ },
+ },
+ },
+ },
+ wantFDIssuers: []*federationdomainproviders.FederationDomainIssuer{},
+ wantStatusUpdates: []*configv1alpha1.FederationDomain{
+ expectedFederationDomainStatusUpdate(
+ &configv1alpha1.FederationDomain{
+ ObjectMeta: metav1.ObjectMeta{Name: "config1", Namespace: namespace, Generation: 123},
+ },
+ configv1alpha1.FederationDomainPhaseError,
+ replaceConditions(
+ allHappyConditionsSuccess("https://issuer1.com", frozenMetav1Now, 123),
+ []configv1alpha1.Condition{
+ sadTransformationExamplesCondition(
+ here.Doc(
+ `.spec.identityProviders[0].transforms.examples[0] example failed:
+ expected: no transformation errors
+ actual: transformations resulted in an unexpected error "identity transformation returned an empty username, which is not allowed"
+
+ .spec.identityProviders[0].transforms.examples[1] example failed:
+ expected: no transformation errors
+ actual: transformations resulted in an unexpected error "identity transformation returned an empty username, which is not allowed"`,
+ ),
frozenMetav1Now, 123),
sadReadyCondition(frozenMetav1Now, 123),
}),
diff --git a/internal/idtransform/identity_transformations.go b/internal/idtransform/identity_transformations.go
index 095b98f81..133d2601b 100644
--- a/internal/idtransform/identity_transformations.go
+++ b/internal/idtransform/identity_transformations.go
@@ -8,7 +8,10 @@ package idtransform
import (
"context"
"fmt"
+ "sort"
"strings"
+
+ "k8s.io/apimachinery/pkg/util/sets"
)
// TransformationResult is the result of evaluating a transformation against some inputs.
@@ -50,11 +53,13 @@ func (p *TransformationPipeline) Evaluate(ctx context.Context, username string,
if groups == nil {
groups = []string{}
}
+
accumulatedResult := &TransformationResult{
Username: username,
Groups: groups,
AuthenticationAllowed: true,
}
+
for i, transform := range p.transforms {
var err error
accumulatedResult, err = transform.Evaluate(ctx, accumulatedResult.Username, accumulatedResult.Groups)
@@ -73,6 +78,15 @@ func (p *TransformationPipeline) Evaluate(ctx context.Context, username string,
return nil, fmt.Errorf("identity transformation returned a null list of groups, which is not allowed")
}
}
+
+ accumulatedResult.Groups = sortAndUniq(accumulatedResult.Groups)
+
// There were no unexpected errors and no policy which rejected auth.
return accumulatedResult, nil
}
+
+func sortAndUniq(s []string) []string {
+ unique := sets.New(s...).UnsortedList()
+ sort.Strings(unique)
+ return unique
+}
diff --git a/internal/idtransform/identity_transformations_test.go b/internal/idtransform/identity_transformations_test.go
index a19b6febb..f3e8707fa 100644
--- a/internal/idtransform/identity_transformations_test.go
+++ b/internal/idtransform/identity_transformations_test.go
@@ -110,6 +110,29 @@ func TestTransformationPipeline(t *testing.T) {
wantAuthenticationAllowed: true,
wantRejectionAuthenticationMessage: "none",
},
+ {
+ name: "group results are sorted and made unique",
+ transforms: []IdentityTransformation{
+ FakeAppendStringTransformer{},
+ },
+ username: "foo",
+ groups: []string{
+ "b",
+ "a",
+ "b",
+ "a",
+ "c",
+ "b",
+ },
+ wantUsername: "foo:transformed",
+ wantGroups: []string{
+ "a:transformed",
+ "b:transformed",
+ "c:transformed",
+ },
+ wantAuthenticationAllowed: true,
+ wantRejectionAuthenticationMessage: "none",
+ },
{
name: "multiple transformations applied successfully",
username: "foo",
@@ -163,7 +186,9 @@ func TestTransformationPipeline(t *testing.T) {
{
name: "unexpected error at index",
username: "foo",
- groups: []string{"foobar"},
+ groups: []string{
+ "foobar",
+ },
transforms: []IdentityTransformation{
FakeAppendStringTransformer{},
FakeErrorTransformer{},
@@ -214,7 +239,9 @@ func TestTransformationPipeline(t *testing.T) {
{
name: "any transformation returning nil for the list of groups will cause an error",
username: "foo",
- groups: []string{"these.will.be.converted.to.nil"},
+ groups: []string{
+ "these.will.be.converted.to.nil",
+ },
transforms: []IdentityTransformation{
FakeNilGroupTransformer{},
},
diff --git a/test/integration/supervisor_discovery_test.go b/test/integration/supervisor_discovery_test.go
index ed2e2e204..4e8d061ca 100644
--- a/test/integration/supervisor_discovery_test.go
+++ b/test/integration/supervisor_discovery_test.go
@@ -143,6 +143,7 @@ func TestSupervisorOIDCDiscovery_Disruptive(t *testing.T) {
"IdentityProvidersDisplayNamesUnique": v1alpha1.ConditionTrue,
"TransformsConstantsNamesUnique": v1alpha1.ConditionTrue,
"TransformsExpressionsValid": v1alpha1.ConditionTrue,
+ "TransformsExamplesPassed": v1alpha1.ConditionTrue,
})
requireStatus(t, client, ns, config6Duplicate2.Name, v1alpha1.FederationDomainPhaseError, map[string]v1alpha1.ConditionStatus{
"Ready": v1alpha1.ConditionFalse,
@@ -155,6 +156,7 @@ func TestSupervisorOIDCDiscovery_Disruptive(t *testing.T) {
"IdentityProvidersDisplayNamesUnique": v1alpha1.ConditionTrue,
"TransformsConstantsNamesUnique": v1alpha1.ConditionTrue,
"TransformsExpressionsValid": v1alpha1.ConditionTrue,
+ "TransformsExamplesPassed": v1alpha1.ConditionTrue,
})
requireDiscoveryEndpointsAreNotFound(t, scheme, addr, caBundle, issuer6)
@@ -184,6 +186,7 @@ func TestSupervisorOIDCDiscovery_Disruptive(t *testing.T) {
"IdentityProvidersDisplayNamesUnique": v1alpha1.ConditionTrue,
"TransformsConstantsNamesUnique": v1alpha1.ConditionTrue,
"TransformsExpressionsValid": v1alpha1.ConditionTrue,
+ "TransformsExamplesPassed": v1alpha1.ConditionTrue,
})
requireDiscoveryEndpointsAreNotFound(t, scheme, addr, caBundle, badIssuer)
requireDeletingFederationDomainCausesDiscoveryEndpointsToDisappear(t, badConfig, client, ns, scheme, addr, caBundle, badIssuer)
@@ -702,6 +705,7 @@ func requireFullySuccessfulStatus(t *testing.T, client pinnipedclientset.Interfa
"IdentityProvidersDisplayNamesUnique": v1alpha1.ConditionTrue,
"TransformsConstantsNamesUnique": v1alpha1.ConditionTrue,
"TransformsExpressionsValid": v1alpha1.ConditionTrue,
+ "TransformsExamplesPassed": v1alpha1.ConditionTrue,
})
}