diff --git a/internal/controller/supervisorconfig/githubupstreamwatcher/github_upstream_watcher.go b/internal/controller/supervisorconfig/githubupstreamwatcher/github_upstream_watcher.go index fc0c98d83..b1de18d0c 100644 --- a/internal/controller/supervisorconfig/githubupstreamwatcher/github_upstream_watcher.go +++ b/internal/controller/supervisorconfig/githubupstreamwatcher/github_upstream_watcher.go @@ -51,14 +51,12 @@ const ( countExpectedConditions = 6 - HostValid string = "HostValid" - TLSConfigurationValid string = "TLSConfigurationValid" - OrganizationsPolicyValid string = "OrganizationsPolicyValid" - // ClientCredentialsObtained is different from other status conditions because it only checks that the client credentials - // have been obtained. The controller has no way to verify whether they are valid. - ClientCredentialsObtained string = "ClientCredentialsObtained" //nolint:gosec // this is not a credential - GitHubConnectionValid string = "GitHubConnectionValid" - ClaimsValid string = "ClaimsValid" + HostValid string = "HostValid" + TLSConfigurationValid string = "TLSConfigurationValid" + OrganizationsPolicyValid string = "OrganizationsPolicyValid" + ClientCredentialsSecretValid string = "ClientCredentialsSecretValid" //nolint:gosec // this is not a credential + GitHubConnectionValid string = "GitHubConnectionValid" + ClaimsValid string = "ClaimsValid" defaultHost = "github.com" defaultApiBaseURL = "https://api.github.com" @@ -166,7 +164,7 @@ func (c *gitHubWatcherController) validateClientSecret(secretName string) (*meta buildFalseCondition := func(prefix string) (*metav1.Condition, string, string, error) { return &metav1.Condition{ - Type: ClientCredentialsObtained, + Type: ClientCredentialsSecretValid, Status: metav1.ConditionFalse, Reason: upstreamwatchers.ReasonNotFound, Message: fmt.Sprintf("%s: secret from spec.client.SecretName (%q) must be found in namespace %q with type %q and keys %q and %q", @@ -202,7 +200,7 @@ func (c *gitHubWatcherController) validateClientSecret(secretName string) (*meta } return &metav1.Condition{ - Type: ClientCredentialsObtained, + Type: ClientCredentialsSecretValid, Status: metav1.ConditionTrue, Reason: upstreamwatchers.ReasonSuccess, Message: fmt.Sprintf("clientID and clientSecret have been read from spec.client.SecretName (%q)", secretName), diff --git a/internal/controller/supervisorconfig/githubupstreamwatcher/github_upstream_watcher_test.go b/internal/controller/supervisorconfig/githubupstreamwatcher/github_upstream_watcher_test.go index 7be242074..0da18dff8 100644 --- a/internal/controller/supervisorconfig/githubupstreamwatcher/github_upstream_watcher_test.go +++ b/internal/controller/supervisorconfig/githubupstreamwatcher/github_upstream_watcher_test.go @@ -79,12 +79,12 @@ func TestController(t *testing.T) { ) require.NoError(t, err) - generation := int64(1234) - lastTransitionTime := metav1.Time{Time: time.Now().Add(-1 * time.Hour)} + wantObservedGeneration := int64(1234) namespace := "some-namespace" - nowDoesntMatter := time.Date(1122, time.September, 33, 4, 55, 56, 778899, time.Local) - frozenClock := clocktesting.NewFakeClock(nowDoesntMatter) + wantFrozenTime := time.Date(1122, time.September, 33, 4, 55, 56, 778899, time.Local) + frozenClockForLastTransitionTime := clocktesting.NewFakeClock(wantFrozenTime) + wantLastTransitionTime := metav1.Time{Time: wantFrozenTime} goodSecret := &corev1.Secret{ ObjectMeta: metav1.ObjectMeta{ @@ -103,7 +103,7 @@ func TestController(t *testing.T) { Name: "minimal-idp-name", Namespace: namespace, UID: types.UID("minimal-uid"), - Generation: generation, + Generation: wantObservedGeneration, }, Spec: v1alpha1.GitHubIdentityProviderSpec{ GitHubAPI: v1alpha1.GitHubAPIConfig{ @@ -134,7 +134,7 @@ func TestController(t *testing.T) { Name: "some-idp-name", Namespace: namespace, UID: types.UID("some-resource-uid"), - Generation: generation, + Generation: wantObservedGeneration, }, Spec: v1alpha1.GitHubIdentityProviderSpec{ GitHubAPI: v1alpha1.GitHubAPIConfig{ @@ -165,8 +165,8 @@ func TestController(t *testing.T) { return metav1.Condition{ Type: HostValid, Status: metav1.ConditionTrue, - ObservedGeneration: generation, - LastTransitionTime: lastTransitionTime, + ObservedGeneration: wantObservedGeneration, + LastTransitionTime: wantLastTransitionTime, Reason: upstreamwatchers.ReasonSuccess, Message: fmt.Sprintf("spec.githubAPI.host (%q) is valid", host), } @@ -178,8 +178,8 @@ func TestController(t *testing.T) { return metav1.Condition{ Type: HostValid, Status: metav1.ConditionFalse, - ObservedGeneration: generation, - LastTransitionTime: lastTransitionTime, + ObservedGeneration: wantObservedGeneration, + LastTransitionTime: wantLastTransitionTime, Reason: "InvalidHost", Message: fmt.Sprintf(`spec.githubAPI.host (%q) is not valid: %s`, host, message), } @@ -191,8 +191,8 @@ func TestController(t *testing.T) { return metav1.Condition{ Type: TLSConfigurationValid, Status: metav1.ConditionTrue, - ObservedGeneration: generation, - LastTransitionTime: lastTransitionTime, + ObservedGeneration: wantObservedGeneration, + LastTransitionTime: wantLastTransitionTime, Reason: upstreamwatchers.ReasonSuccess, Message: "spec.githubAPI.tls.certificateAuthorityData is valid", } @@ -204,8 +204,8 @@ func TestController(t *testing.T) { return metav1.Condition{ Type: TLSConfigurationValid, Status: metav1.ConditionFalse, - ObservedGeneration: generation, - LastTransitionTime: lastTransitionTime, + ObservedGeneration: wantObservedGeneration, + LastTransitionTime: wantLastTransitionTime, Reason: "InvalidTLSConfig", Message: message, } @@ -217,8 +217,8 @@ func TestController(t *testing.T) { return metav1.Condition{ Type: OrganizationsPolicyValid, Status: metav1.ConditionTrue, - ObservedGeneration: generation, - LastTransitionTime: lastTransitionTime, + ObservedGeneration: wantObservedGeneration, + LastTransitionTime: wantLastTransitionTime, Reason: upstreamwatchers.ReasonSuccess, Message: fmt.Sprintf("spec.allowAuthentication.organizations.policy (%q) is valid", policy), } @@ -230,33 +230,34 @@ func TestController(t *testing.T) { return metav1.Condition{ Type: OrganizationsPolicyValid, Status: metav1.ConditionFalse, - ObservedGeneration: generation, - LastTransitionTime: lastTransitionTime, + ObservedGeneration: wantObservedGeneration, + LastTransitionTime: wantLastTransitionTime, Reason: "Invalid", Message: message, } } - buildClientCredentialsObtainedTrue := func(t *testing.T, secretName string) metav1.Condition { + buildClientCredentialsSecretValidTrue := func(t *testing.T, secretName string) metav1.Condition { t.Helper() + return metav1.Condition{ - Type: ClientCredentialsObtained, + Type: ClientCredentialsSecretValid, Status: metav1.ConditionTrue, - ObservedGeneration: generation, - LastTransitionTime: lastTransitionTime, + ObservedGeneration: wantObservedGeneration, + LastTransitionTime: wantLastTransitionTime, Reason: upstreamwatchers.ReasonSuccess, Message: fmt.Sprintf("clientID and clientSecret have been read from spec.client.SecretName (%q)", secretName), } } - buildClientCredentialsObtainedFalse := func(t *testing.T, prefix, secretName, namespace, reason string) metav1.Condition { + buildClientCredentialsSecretValidFalse := func(t *testing.T, prefix, secretName, namespace, reason string) metav1.Condition { t.Helper() return metav1.Condition{ - Type: ClientCredentialsObtained, + Type: ClientCredentialsSecretValid, Status: metav1.ConditionFalse, - ObservedGeneration: generation, - LastTransitionTime: lastTransitionTime, + ObservedGeneration: wantObservedGeneration, + LastTransitionTime: wantLastTransitionTime, Reason: reason, Message: fmt.Sprintf( `%s: secret from spec.client.SecretName (%q) must be found in namespace %q with type "secrets.pinniped.dev/github-client" and keys "clientID" and "clientSecret"`, @@ -269,11 +270,12 @@ func TestController(t *testing.T) { buildClaimsValidatedTrue := func(t *testing.T) metav1.Condition { t.Helper() + return metav1.Condition{ Type: ClaimsValid, Status: metav1.ConditionTrue, - ObservedGeneration: generation, - LastTransitionTime: lastTransitionTime, + ObservedGeneration: wantObservedGeneration, + LastTransitionTime: wantLastTransitionTime, Reason: upstreamwatchers.ReasonSuccess, Message: "spec.claims are valid", } @@ -281,11 +283,12 @@ func TestController(t *testing.T) { buildClaimsValidatedFalse := func(t *testing.T, message string) metav1.Condition { t.Helper() + return metav1.Condition{ Type: ClaimsValid, Status: metav1.ConditionFalse, - ObservedGeneration: generation, - LastTransitionTime: lastTransitionTime, + ObservedGeneration: wantObservedGeneration, + LastTransitionTime: wantLastTransitionTime, Reason: "Invalid", Message: message, } @@ -297,8 +300,8 @@ func TestController(t *testing.T) { return metav1.Condition{ Type: GitHubConnectionValid, Status: metav1.ConditionTrue, - ObservedGeneration: generation, - LastTransitionTime: lastTransitionTime, + ObservedGeneration: wantObservedGeneration, + LastTransitionTime: wantLastTransitionTime, Reason: upstreamwatchers.ReasonSuccess, Message: fmt.Sprintf("spec.githubAPI.host (%q) is reachable and TLS verification succeeds", host), } @@ -310,8 +313,8 @@ func TestController(t *testing.T) { return metav1.Condition{ Type: GitHubConnectionValid, Status: metav1.ConditionFalse, - ObservedGeneration: generation, - LastTransitionTime: lastTransitionTime, + ObservedGeneration: wantObservedGeneration, + LastTransitionTime: wantLastTransitionTime, Reason: "UnableToDialServer", Message: message, } @@ -323,8 +326,8 @@ func TestController(t *testing.T) { return metav1.Condition{ Type: GitHubConnectionValid, Status: metav1.ConditionUnknown, - ObservedGeneration: generation, - LastTransitionTime: lastTransitionTime, + ObservedGeneration: wantObservedGeneration, + LastTransitionTime: wantLastTransitionTime, Reason: "UnableToValidate", Message: "unable to validate; see other conditions for details", } @@ -338,8 +341,8 @@ func TestController(t *testing.T) { return fmt.Sprintf(`{"level":"info","timestamp":"2099-08-08T13:57:36.123456Z","logger":"github-upstream-observer","caller":"conditionsutil/conditions_util.go:$conditionsutil.MergeConditions","message":"updated condition","namespace":"some-namespace","name":"%s","type":"ClaimsValid","status":"False","reason":"Invalid","message":"%s"}`, name, message) } - buildLogForUpdatingClientCredentialsObtained := func(name, status, reason, message string) string { - return fmt.Sprintf(`{"level":"info","timestamp":"2099-08-08T13:57:36.123456Z","logger":"github-upstream-observer","caller":"conditionsutil/conditions_util.go:$conditionsutil.MergeConditions","message":"updated condition","namespace":"some-namespace","name":"%s","type":"ClientCredentialsObtained","status":"%s","reason":"%s","message":"%s"}`, name, status, reason, message) + buildLogForUpdatingClientCredentialsSecretValid := func(name, status, reason, message string) string { + return fmt.Sprintf(`{"level":"info","timestamp":"2099-08-08T13:57:36.123456Z","logger":"github-upstream-observer","caller":"conditionsutil/conditions_util.go:$conditionsutil.MergeConditions","message":"updated condition","namespace":"some-namespace","name":"%s","type":"ClientCredentialsSecretValid","status":"%s","reason":"%s","message":"%s"}`, name, status, reason, message) } buildLogForUpdatingOrganizationPolicyValid := func(name, status, reason, message string) string { @@ -419,7 +422,7 @@ func TestController(t *testing.T) { Phase: v1alpha1.GitHubPhaseReady, Conditions: []metav1.Condition{ buildClaimsValidatedTrue(t), - buildClientCredentialsObtainedTrue(t, validFilledOutIDP.Spec.Client.SecretName), + buildClientCredentialsSecretValidTrue(t, validFilledOutIDP.Spec.Client.SecretName), buildGitHubConnectionValidTrue(t, *validFilledOutIDP.Spec.GitHubAPI.Host), buildHostValidTrue(t, *validFilledOutIDP.Spec.GitHubAPI.Host), buildOrganizationsPolicyValidTrue(t, *validFilledOutIDP.Spec.AllowAuthentication.Organizations.Policy), @@ -429,7 +432,7 @@ func TestController(t *testing.T) { }, }, wantLogs: []string{ - buildLogForUpdatingClientCredentialsObtained("some-idp-name", "True", "Success", fmt.Sprintf(`clientID and clientSecret have been read from spec.client.SecretName (\"%s\")`, validFilledOutIDP.Spec.Client.SecretName)), + buildLogForUpdatingClientCredentialsSecretValid("some-idp-name", "True", "Success", fmt.Sprintf(`clientID and clientSecret have been read from spec.client.SecretName (\"%s\")`, validFilledOutIDP.Spec.Client.SecretName)), buildLogForUpdatingClaimsValidTrue("some-idp-name"), buildLogForUpdatingOrganizationPolicyValid("some-idp-name", "True", "Success", fmt.Sprintf(`spec.allowAuthentication.organizations.policy (\"%s\") is valid`, string(*validFilledOutIDP.Spec.AllowAuthentication.Organizations.Policy))), buildLogForUpdatingHostValid("some-idp-name", "True", "Success", `spec.githubAPI.host (\"%s\") is valid`, *validFilledOutIDP.Spec.GitHubAPI.Host), @@ -475,7 +478,7 @@ func TestController(t *testing.T) { Phase: v1alpha1.GitHubPhaseReady, Conditions: []metav1.Condition{ buildClaimsValidatedTrue(t), - buildClientCredentialsObtainedTrue(t, validMinimalIDP.Spec.Client.SecretName), + buildClientCredentialsSecretValidTrue(t, validMinimalIDP.Spec.Client.SecretName), buildGitHubConnectionValidTrue(t, *validMinimalIDP.Spec.GitHubAPI.Host), buildHostValidTrue(t, *validMinimalIDP.Spec.GitHubAPI.Host), buildOrganizationsPolicyValidTrue(t, *validMinimalIDP.Spec.AllowAuthentication.Organizations.Policy), @@ -485,7 +488,7 @@ func TestController(t *testing.T) { }, }, wantLogs: []string{ - buildLogForUpdatingClientCredentialsObtained("minimal-idp-name", "True", "Success", fmt.Sprintf(`clientID and clientSecret have been read from spec.client.SecretName (\"%s\")`, validMinimalIDP.Spec.Client.SecretName)), + buildLogForUpdatingClientCredentialsSecretValid("minimal-idp-name", "True", "Success", fmt.Sprintf(`clientID and clientSecret have been read from spec.client.SecretName (\"%s\")`, validMinimalIDP.Spec.Client.SecretName)), buildLogForUpdatingClaimsValidTrue("minimal-idp-name"), buildLogForUpdatingOrganizationPolicyValid("minimal-idp-name", "True", "Success", fmt.Sprintf(`spec.allowAuthentication.organizations.policy (\"%s\") is valid`, string(*validMinimalIDP.Spec.AllowAuthentication.Organizations.Policy))), buildLogForUpdatingHostValid("minimal-idp-name", "True", "Success", `spec.githubAPI.host (\"%s\") is valid`, *validMinimalIDP.Spec.GitHubAPI.Host), @@ -550,7 +553,7 @@ func TestController(t *testing.T) { Phase: v1alpha1.GitHubPhaseReady, Conditions: []metav1.Condition{ buildClaimsValidatedTrue(t), - buildClientCredentialsObtainedTrue(t, validMinimalIDP.Spec.Client.SecretName), + buildClientCredentialsSecretValidTrue(t, validMinimalIDP.Spec.Client.SecretName), buildGitHubConnectionValidTrue(t, "github.com:443"), buildHostValidTrue(t, "github.com"), buildOrganizationsPolicyValidTrue(t, *validMinimalIDP.Spec.AllowAuthentication.Organizations.Policy), @@ -560,7 +563,7 @@ func TestController(t *testing.T) { }, }, wantLogs: []string{ - buildLogForUpdatingClientCredentialsObtained("minimal-idp-name", "True", "Success", fmt.Sprintf(`clientID and clientSecret have been read from spec.client.SecretName (\"%s\")`, validMinimalIDP.Spec.Client.SecretName)), + buildLogForUpdatingClientCredentialsSecretValid("minimal-idp-name", "True", "Success", fmt.Sprintf(`clientID and clientSecret have been read from spec.client.SecretName (\"%s\")`, validMinimalIDP.Spec.Client.SecretName)), buildLogForUpdatingClaimsValidTrue("minimal-idp-name"), buildLogForUpdatingOrganizationPolicyValid("minimal-idp-name", "True", "Success", fmt.Sprintf(`spec.allowAuthentication.organizations.policy (\"%s\") is valid`, string(*validMinimalIDP.Spec.AllowAuthentication.Organizations.Policy))), buildLogForUpdatingHostValid("minimal-idp-name", "True", "Success", `spec.githubAPI.host (\"%s\") is valid`, "github.com"), @@ -621,7 +624,7 @@ func TestController(t *testing.T) { Phase: v1alpha1.GitHubPhaseReady, Conditions: []metav1.Condition{ buildClaimsValidatedTrue(t), - buildClientCredentialsObtainedTrue(t, validMinimalIDP.Spec.Client.SecretName), + buildClientCredentialsSecretValidTrue(t, validMinimalIDP.Spec.Client.SecretName), buildGitHubConnectionValidTrue(t, goodServerIPv6Domain), buildHostValidTrue(t, goodServerIPv6Domain), buildOrganizationsPolicyValidTrue(t, *validMinimalIDP.Spec.AllowAuthentication.Organizations.Policy), @@ -631,7 +634,7 @@ func TestController(t *testing.T) { }, }, wantLogs: []string{ - buildLogForUpdatingClientCredentialsObtained("minimal-idp-name", "True", "Success", fmt.Sprintf(`clientID and clientSecret have been read from spec.client.SecretName (\"%s\")`, validMinimalIDP.Spec.Client.SecretName)), + buildLogForUpdatingClientCredentialsSecretValid("minimal-idp-name", "True", "Success", fmt.Sprintf(`clientID and clientSecret have been read from spec.client.SecretName (\"%s\")`, validMinimalIDP.Spec.Client.SecretName)), buildLogForUpdatingClaimsValidTrue("minimal-idp-name"), buildLogForUpdatingOrganizationPolicyValid("minimal-idp-name", "True", "Success", fmt.Sprintf(`spec.allowAuthentication.organizations.policy (\"%s\") is valid`, string(*validMinimalIDP.Spec.AllowAuthentication.Organizations.Policy))), buildLogForUpdatingHostValid("minimal-idp-name", "True", "Success", `spec.githubAPI.host (\"%s\") is valid`, goodServerIPv6Domain), @@ -730,7 +733,7 @@ func TestController(t *testing.T) { Phase: v1alpha1.GitHubPhaseError, Conditions: []metav1.Condition{ buildClaimsValidatedTrue(t), - buildClientCredentialsObtainedFalse( + buildClientCredentialsSecretValidFalse( t, `secret "no-secret-with-this-name" not found`, "no-secret-with-this-name", @@ -760,7 +763,7 @@ func TestController(t *testing.T) { Phase: v1alpha1.GitHubPhaseReady, Conditions: []metav1.Condition{ buildClaimsValidatedTrue(t), - buildClientCredentialsObtainedTrue(t, "other-secret-name"), + buildClientCredentialsSecretValidTrue(t, "other-secret-name"), buildGitHubConnectionValidTrue(t, *validFilledOutIDP.Spec.GitHubAPI.Host), buildHostValidTrue(t, *validFilledOutIDP.Spec.GitHubAPI.Host), buildOrganizationsPolicyValidTrue(t, *validFilledOutIDP.Spec.AllowAuthentication.Organizations.Policy), @@ -775,7 +778,7 @@ func TestController(t *testing.T) { Phase: v1alpha1.GitHubPhaseReady, Conditions: []metav1.Condition{ buildClaimsValidatedTrue(t), - buildClientCredentialsObtainedTrue(t, validFilledOutIDP.Spec.Client.SecretName), + buildClientCredentialsSecretValidTrue(t, validFilledOutIDP.Spec.Client.SecretName), buildGitHubConnectionValidTrue(t, *validFilledOutIDP.Spec.GitHubAPI.Host), buildHostValidTrue(t, *validFilledOutIDP.Spec.GitHubAPI.Host), buildOrganizationsPolicyValidTrue(t, *validFilledOutIDP.Spec.AllowAuthentication.Organizations.Policy), @@ -785,7 +788,7 @@ func TestController(t *testing.T) { }, }, wantLogs: []string{ - buildLogForUpdatingClientCredentialsObtained("invalid-idp-name", "False", "SecretNotFound", `secret \"no-secret-with-this-name\" not found: secret from spec.client.SecretName (\"no-secret-with-this-name\") must be found in namespace \"some-namespace\" with type \"secrets.pinniped.dev/github-client\" and keys \"clientID\" and \"clientSecret\"`), + buildLogForUpdatingClientCredentialsSecretValid("invalid-idp-name", "False", "SecretNotFound", `secret \"no-secret-with-this-name\" not found: secret from spec.client.SecretName (\"no-secret-with-this-name\") must be found in namespace \"some-namespace\" with type \"secrets.pinniped.dev/github-client\" and keys \"clientID\" and \"clientSecret\"`), buildLogForUpdatingClaimsValidTrue("invalid-idp-name"), buildLogForUpdatingOrganizationPolicyValid("invalid-idp-name", "True", "Success", fmt.Sprintf(`spec.allowAuthentication.organizations.policy (\"%s\") is valid`, string(*validFilledOutIDP.Spec.AllowAuthentication.Organizations.Policy))), buildLogForUpdatingHostValid("invalid-idp-name", "True", "Success", `spec.githubAPI.host (\"%s\") is valid`, *validFilledOutIDP.Spec.GitHubAPI.Host), @@ -793,7 +796,7 @@ func TestController(t *testing.T) { buildLogForUpdatingGitHubConnectionValid("invalid-idp-name", "True", "Success", `spec.githubAPI.host (\"%s\") is reachable and TLS verification succeeds`, *validFilledOutIDP.Spec.GitHubAPI.Host), buildLogForUpdatingPhase("invalid-idp-name", "Error"), - buildLogForUpdatingClientCredentialsObtained("other-idp-name", "True", "Success", `clientID and clientSecret have been read from spec.client.SecretName (\"other-secret-name\")`), + buildLogForUpdatingClientCredentialsSecretValid("other-idp-name", "True", "Success", `clientID and clientSecret have been read from spec.client.SecretName (\"other-secret-name\")`), buildLogForUpdatingClaimsValidTrue("other-idp-name"), buildLogForUpdatingOrganizationPolicyValid("other-idp-name", "True", "Success", fmt.Sprintf(`spec.allowAuthentication.organizations.policy (\"%s\") is valid`, string(*validFilledOutIDP.Spec.AllowAuthentication.Organizations.Policy))), buildLogForUpdatingHostValid("other-idp-name", "True", "Success", `spec.githubAPI.host (\"%s\") is valid`, *validFilledOutIDP.Spec.GitHubAPI.Host), @@ -801,7 +804,7 @@ func TestController(t *testing.T) { buildLogForUpdatingGitHubConnectionValid("other-idp-name", "True", "Success", `spec.githubAPI.host (\"%s\") is reachable and TLS verification succeeds`, *validFilledOutIDP.Spec.GitHubAPI.Host), buildLogForUpdatingPhase("other-idp-name", "Ready"), - buildLogForUpdatingClientCredentialsObtained("some-idp-name", "True", "Success", fmt.Sprintf(`clientID and clientSecret have been read from spec.client.SecretName (\"%s\")`, validFilledOutIDP.Spec.Client.SecretName)), + buildLogForUpdatingClientCredentialsSecretValid("some-idp-name", "True", "Success", fmt.Sprintf(`clientID and clientSecret have been read from spec.client.SecretName (\"%s\")`, validFilledOutIDP.Spec.Client.SecretName)), buildLogForUpdatingClaimsValidTrue("some-idp-name"), buildLogForUpdatingOrganizationPolicyValid("some-idp-name", "True", "Success", fmt.Sprintf(`spec.allowAuthentication.organizations.policy (\"%s\") is valid`, string(*validFilledOutIDP.Spec.AllowAuthentication.Organizations.Policy))), buildLogForUpdatingHostValid("some-idp-name", "True", "Success", `spec.githubAPI.host (\"%s\") is valid`, *validFilledOutIDP.Spec.GitHubAPI.Host), @@ -832,7 +835,7 @@ func TestController(t *testing.T) { Phase: v1alpha1.GitHubPhaseError, Conditions: []metav1.Condition{ buildClaimsValidatedTrue(t), - buildClientCredentialsObtainedTrue(t, validFilledOutIDP.Spec.Client.SecretName), + buildClientCredentialsSecretValidTrue(t, validFilledOutIDP.Spec.Client.SecretName), buildGitHubConnectionValidUnknown(t), buildHostValidFalse(t, "", "must not be empty"), buildOrganizationsPolicyValidTrue(t, *validFilledOutIDP.Spec.AllowAuthentication.Organizations.Policy), @@ -842,7 +845,7 @@ func TestController(t *testing.T) { }, }, wantLogs: []string{ - buildLogForUpdatingClientCredentialsObtained("some-idp-name", "True", "Success", fmt.Sprintf(`clientID and clientSecret have been read from spec.client.SecretName (\"%s\")`, validFilledOutIDP.Spec.Client.SecretName)), + buildLogForUpdatingClientCredentialsSecretValid("some-idp-name", "True", "Success", fmt.Sprintf(`clientID and clientSecret have been read from spec.client.SecretName (\"%s\")`, validFilledOutIDP.Spec.Client.SecretName)), buildLogForUpdatingClaimsValidTrue("some-idp-name"), buildLogForUpdatingOrganizationPolicyValid("some-idp-name", "True", "Success", fmt.Sprintf(`spec.allowAuthentication.organizations.policy (\"%s\") is valid`, string(*validFilledOutIDP.Spec.AllowAuthentication.Organizations.Policy))), buildLogForUpdatingHostValid("some-idp-name", "False", "InvalidHost", `spec.githubAPI.host (\"%s\") is not valid: must not be empty`, ""), @@ -873,7 +876,7 @@ func TestController(t *testing.T) { Phase: v1alpha1.GitHubPhaseError, Conditions: []metav1.Condition{ buildClaimsValidatedTrue(t), - buildClientCredentialsObtainedTrue(t, validFilledOutIDP.Spec.Client.SecretName), + buildClientCredentialsSecretValidTrue(t, validFilledOutIDP.Spec.Client.SecretName), buildGitHubConnectionValidUnknown(t), buildHostValidFalse(t, "https://example.com", `invalid port "//example.com"`), buildOrganizationsPolicyValidTrue(t, *validMinimalIDP.Spec.AllowAuthentication.Organizations.Policy), @@ -883,7 +886,7 @@ func TestController(t *testing.T) { }, }, wantLogs: []string{ - buildLogForUpdatingClientCredentialsObtained("minimal-idp-name", "True", "Success", fmt.Sprintf(`clientID and clientSecret have been read from spec.client.SecretName (\"%s\")`, validMinimalIDP.Spec.Client.SecretName)), + buildLogForUpdatingClientCredentialsSecretValid("minimal-idp-name", "True", "Success", fmt.Sprintf(`clientID and clientSecret have been read from spec.client.SecretName (\"%s\")`, validMinimalIDP.Spec.Client.SecretName)), buildLogForUpdatingClaimsValidTrue("minimal-idp-name"), buildLogForUpdatingOrganizationPolicyValid("minimal-idp-name", "True", "Success", fmt.Sprintf(`spec.allowAuthentication.organizations.policy (\"%s\") is valid`, string(*validMinimalIDP.Spec.AllowAuthentication.Organizations.Policy))), buildLogForUpdatingHostValid("minimal-idp-name", "False", "InvalidHost", `spec.githubAPI.host (\"%s\") is not valid: invalid port \"//example.com\"`, "https://example.com"), @@ -914,7 +917,7 @@ func TestController(t *testing.T) { Phase: v1alpha1.GitHubPhaseError, Conditions: []metav1.Condition{ buildClaimsValidatedTrue(t), - buildClientCredentialsObtainedTrue(t, validMinimalIDP.Spec.Client.SecretName), + buildClientCredentialsSecretValidTrue(t, validMinimalIDP.Spec.Client.SecretName), buildGitHubConnectionValidUnknown(t), buildHostValidFalse(t, "example.com/foo", `host "example.com/foo" is not a valid hostname or IP address`), buildOrganizationsPolicyValidTrue(t, *validMinimalIDP.Spec.AllowAuthentication.Organizations.Policy), @@ -924,7 +927,7 @@ func TestController(t *testing.T) { }, }, wantLogs: []string{ - buildLogForUpdatingClientCredentialsObtained("minimal-idp-name", "True", "Success", fmt.Sprintf(`clientID and clientSecret have been read from spec.client.SecretName (\"%s\")`, validMinimalIDP.Spec.Client.SecretName)), + buildLogForUpdatingClientCredentialsSecretValid("minimal-idp-name", "True", "Success", fmt.Sprintf(`clientID and clientSecret have been read from spec.client.SecretName (\"%s\")`, validMinimalIDP.Spec.Client.SecretName)), buildLogForUpdatingClaimsValidTrue("minimal-idp-name"), buildLogForUpdatingOrganizationPolicyValid("minimal-idp-name", "True", "Success", fmt.Sprintf(`spec.allowAuthentication.organizations.policy (\"%s\") is valid`, string(*validMinimalIDP.Spec.AllowAuthentication.Organizations.Policy))), buildLogForUpdatingHostValid("minimal-idp-name", "False", "InvalidHost", `spec.githubAPI.host (\"%s\") is not valid: host \"example.com/foo\" is not a valid hostname or IP address`, "example.com/foo"), @@ -955,7 +958,7 @@ func TestController(t *testing.T) { Phase: v1alpha1.GitHubPhaseError, Conditions: []metav1.Condition{ buildClaimsValidatedTrue(t), - buildClientCredentialsObtainedTrue(t, validMinimalIDP.Spec.Client.SecretName), + buildClientCredentialsSecretValidTrue(t, validMinimalIDP.Spec.Client.SecretName), buildGitHubConnectionValidUnknown(t), buildHostValidFalse(t, "u:p@example.com", `invalid port "p@example.com"`), buildOrganizationsPolicyValidTrue(t, *validMinimalIDP.Spec.AllowAuthentication.Organizations.Policy), @@ -965,7 +968,7 @@ func TestController(t *testing.T) { }, }, wantLogs: []string{ - buildLogForUpdatingClientCredentialsObtained("minimal-idp-name", "True", "Success", fmt.Sprintf(`clientID and clientSecret have been read from spec.client.SecretName (\"%s\")`, validMinimalIDP.Spec.Client.SecretName)), + buildLogForUpdatingClientCredentialsSecretValid("minimal-idp-name", "True", "Success", fmt.Sprintf(`clientID and clientSecret have been read from spec.client.SecretName (\"%s\")`, validMinimalIDP.Spec.Client.SecretName)), buildLogForUpdatingClaimsValidTrue("minimal-idp-name"), buildLogForUpdatingOrganizationPolicyValid("minimal-idp-name", "True", "Success", fmt.Sprintf(`spec.allowAuthentication.organizations.policy (\"%s\") is valid`, string(*validMinimalIDP.Spec.AllowAuthentication.Organizations.Policy))), buildLogForUpdatingHostValid("minimal-idp-name", "False", "InvalidHost", `spec.githubAPI.host (\"%s\") is not valid: invalid port \"p@example.com\"`, "u:p@example.com"), @@ -996,7 +999,7 @@ func TestController(t *testing.T) { Phase: v1alpha1.GitHubPhaseError, Conditions: []metav1.Condition{ buildClaimsValidatedTrue(t), - buildClientCredentialsObtainedTrue(t, validMinimalIDP.Spec.Client.SecretName), + buildClientCredentialsSecretValidTrue(t, validMinimalIDP.Spec.Client.SecretName), buildGitHubConnectionValidUnknown(t), buildHostValidFalse(t, "example.com?a=b", `host "example.com?a=b" is not a valid hostname or IP address`), buildOrganizationsPolicyValidTrue(t, *validMinimalIDP.Spec.AllowAuthentication.Organizations.Policy), @@ -1006,7 +1009,7 @@ func TestController(t *testing.T) { }, }, wantLogs: []string{ - buildLogForUpdatingClientCredentialsObtained("minimal-idp-name", "True", "Success", fmt.Sprintf(`clientID and clientSecret have been read from spec.client.SecretName (\"%s\")`, validMinimalIDP.Spec.Client.SecretName)), + buildLogForUpdatingClientCredentialsSecretValid("minimal-idp-name", "True", "Success", fmt.Sprintf(`clientID and clientSecret have been read from spec.client.SecretName (\"%s\")`, validMinimalIDP.Spec.Client.SecretName)), buildLogForUpdatingClaimsValidTrue("minimal-idp-name"), buildLogForUpdatingOrganizationPolicyValid("minimal-idp-name", "True", "Success", fmt.Sprintf(`spec.allowAuthentication.organizations.policy (\"%s\") is valid`, string(*validMinimalIDP.Spec.AllowAuthentication.Organizations.Policy))), buildLogForUpdatingHostValid("minimal-idp-name", "False", "InvalidHost", `spec.githubAPI.host (\"%s\") is not valid: host \"example.com?a=b\" is not a valid hostname or IP address`, "example.com?a=b"), @@ -1037,7 +1040,7 @@ func TestController(t *testing.T) { Phase: v1alpha1.GitHubPhaseError, Conditions: []metav1.Condition{ buildClaimsValidatedTrue(t), - buildClientCredentialsObtainedTrue(t, validMinimalIDP.Spec.Client.SecretName), + buildClientCredentialsSecretValidTrue(t, validMinimalIDP.Spec.Client.SecretName), buildGitHubConnectionValidUnknown(t), buildHostValidFalse(t, "example.com#a", `host "example.com#a" is not a valid hostname or IP address`), buildOrganizationsPolicyValidTrue(t, *validMinimalIDP.Spec.AllowAuthentication.Organizations.Policy), @@ -1047,7 +1050,7 @@ func TestController(t *testing.T) { }, }, wantLogs: []string{ - buildLogForUpdatingClientCredentialsObtained("minimal-idp-name", "True", "Success", fmt.Sprintf(`clientID and clientSecret have been read from spec.client.SecretName (\"%s\")`, validMinimalIDP.Spec.Client.SecretName)), + buildLogForUpdatingClientCredentialsSecretValid("minimal-idp-name", "True", "Success", fmt.Sprintf(`clientID and clientSecret have been read from spec.client.SecretName (\"%s\")`, validMinimalIDP.Spec.Client.SecretName)), buildLogForUpdatingClaimsValidTrue("minimal-idp-name"), buildLogForUpdatingOrganizationPolicyValid("minimal-idp-name", "True", "Success", fmt.Sprintf(`spec.allowAuthentication.organizations.policy (\"%s\") is valid`, string(*validMinimalIDP.Spec.AllowAuthentication.Organizations.Policy))), buildLogForUpdatingHostValid("minimal-idp-name", "False", "InvalidHost", `spec.githubAPI.host (\"%s\") is not valid: host \"example.com#a\" is not a valid hostname or IP address`, "example.com#a"), @@ -1082,7 +1085,7 @@ func TestController(t *testing.T) { Phase: v1alpha1.GitHubPhaseError, Conditions: []metav1.Condition{ buildClaimsValidatedTrue(t), - buildClientCredentialsObtainedTrue(t, validFilledOutIDP.Spec.Client.SecretName), + buildClientCredentialsSecretValidTrue(t, validFilledOutIDP.Spec.Client.SecretName), buildGitHubConnectionValidUnknown(t), buildHostValidTrue(t, *validFilledOutIDP.Spec.GitHubAPI.Host), buildOrganizationsPolicyValidTrue(t, *validFilledOutIDP.Spec.AllowAuthentication.Organizations.Policy), @@ -1092,7 +1095,7 @@ func TestController(t *testing.T) { }, }, wantLogs: []string{ - buildLogForUpdatingClientCredentialsObtained("some-idp-name", "True", "Success", fmt.Sprintf(`clientID and clientSecret have been read from spec.client.SecretName (\"%s\")`, validFilledOutIDP.Spec.Client.SecretName)), + buildLogForUpdatingClientCredentialsSecretValid("some-idp-name", "True", "Success", fmt.Sprintf(`clientID and clientSecret have been read from spec.client.SecretName (\"%s\")`, validFilledOutIDP.Spec.Client.SecretName)), buildLogForUpdatingClaimsValidTrue("some-idp-name"), buildLogForUpdatingOrganizationPolicyValid("some-idp-name", "True", "Success", fmt.Sprintf(`spec.allowAuthentication.organizations.policy (\"%s\") is valid`, string(*validFilledOutIDP.Spec.AllowAuthentication.Organizations.Policy))), buildLogForUpdatingHostValid("some-idp-name", "True", "Success", `spec.githubAPI.host (\"%s\") is valid`, *validFilledOutIDP.Spec.GitHubAPI.Host), @@ -1124,7 +1127,7 @@ func TestController(t *testing.T) { Phase: v1alpha1.GitHubPhaseError, Conditions: []metav1.Condition{ buildClaimsValidatedTrue(t), - buildClientCredentialsObtainedTrue(t, validMinimalIDP.Spec.Client.SecretName), + buildClientCredentialsSecretValidTrue(t, validMinimalIDP.Spec.Client.SecretName), buildGitHubConnectionValidFalse(t, fmt.Sprintf(`cannot dial server spec.githubAPI.host (%q): dial tcp: lookup nowhere.bad-tld: no such host`, "nowhere.bad-tld:443")), buildHostValidTrue(t, "nowhere.bad-tld"), buildOrganizationsPolicyValidTrue(t, *validMinimalIDP.Spec.AllowAuthentication.Organizations.Policy), @@ -1134,7 +1137,7 @@ func TestController(t *testing.T) { }, }, wantLogs: []string{ - buildLogForUpdatingClientCredentialsObtained("minimal-idp-name", "True", "Success", fmt.Sprintf(`clientID and clientSecret have been read from spec.client.SecretName (\"%s\")`, validMinimalIDP.Spec.Client.SecretName)), + buildLogForUpdatingClientCredentialsSecretValid("minimal-idp-name", "True", "Success", fmt.Sprintf(`clientID and clientSecret have been read from spec.client.SecretName (\"%s\")`, validMinimalIDP.Spec.Client.SecretName)), buildLogForUpdatingClaimsValidTrue("minimal-idp-name"), buildLogForUpdatingOrganizationPolicyValid("minimal-idp-name", "True", "Success", fmt.Sprintf(`spec.allowAuthentication.organizations.policy (\"%s\") is valid`, string(*validMinimalIDP.Spec.AllowAuthentication.Organizations.Policy))), buildLogForUpdatingHostValid("minimal-idp-name", "True", "Success", `spec.githubAPI.host (\"%s\") is valid`, "nowhere.bad-tld"), @@ -1165,7 +1168,7 @@ func TestController(t *testing.T) { Phase: v1alpha1.GitHubPhaseError, Conditions: []metav1.Condition{ buildClaimsValidatedTrue(t), - buildClientCredentialsObtainedTrue(t, validMinimalIDP.Spec.Client.SecretName), + buildClientCredentialsSecretValidTrue(t, validMinimalIDP.Spec.Client.SecretName), buildGitHubConnectionValidUnknown(t), buildHostValidFalse(t, "0:0:0:0:0:0:0:1:9876", `host "0:0:0:0:0:0:0:1:9876" is not a valid hostname or IP address`), buildOrganizationsPolicyValidTrue(t, *validMinimalIDP.Spec.AllowAuthentication.Organizations.Policy), @@ -1175,7 +1178,7 @@ func TestController(t *testing.T) { }, }, wantLogs: []string{ - buildLogForUpdatingClientCredentialsObtained("minimal-idp-name", "True", "Success", fmt.Sprintf(`clientID and clientSecret have been read from spec.client.SecretName (\"%s\")`, validMinimalIDP.Spec.Client.SecretName)), + buildLogForUpdatingClientCredentialsSecretValid("minimal-idp-name", "True", "Success", fmt.Sprintf(`clientID and clientSecret have been read from spec.client.SecretName (\"%s\")`, validMinimalIDP.Spec.Client.SecretName)), buildLogForUpdatingClaimsValidTrue("minimal-idp-name"), buildLogForUpdatingOrganizationPolicyValid("minimal-idp-name", "True", "Success", fmt.Sprintf(`spec.allowAuthentication.organizations.policy (\"%s\") is valid`, string(*validMinimalIDP.Spec.AllowAuthentication.Organizations.Policy))), buildLogForUpdatingHostValid("minimal-idp-name", "False", "InvalidHost", `spec.githubAPI.host (\"0:0:0:0:0:0:0:1:9876\") is not valid: host \"%s\" is not a valid hostname or IP address`, "0:0:0:0:0:0:0:1:9876"), @@ -1207,7 +1210,7 @@ func TestController(t *testing.T) { Phase: v1alpha1.GitHubPhaseError, Conditions: []metav1.Condition{ buildClaimsValidatedTrue(t), - buildClientCredentialsObtainedTrue(t, validFilledOutIDP.Spec.Client.SecretName), + buildClientCredentialsSecretValidTrue(t, validFilledOutIDP.Spec.Client.SecretName), buildGitHubConnectionValidFalse(t, fmt.Sprintf(`cannot dial server spec.githubAPI.host (%q): tls: failed to verify certificate: x509: certificate signed by unknown authority`, *validFilledOutIDP.Spec.GitHubAPI.Host)), buildHostValidTrue(t, *validFilledOutIDP.Spec.GitHubAPI.Host), buildOrganizationsPolicyValidTrue(t, *validFilledOutIDP.Spec.AllowAuthentication.Organizations.Policy), @@ -1217,7 +1220,7 @@ func TestController(t *testing.T) { }, }, wantLogs: []string{ - buildLogForUpdatingClientCredentialsObtained("some-idp-name", "True", "Success", fmt.Sprintf(`clientID and clientSecret have been read from spec.client.SecretName (\"%s\")`, validFilledOutIDP.Spec.Client.SecretName)), + buildLogForUpdatingClientCredentialsSecretValid("some-idp-name", "True", "Success", fmt.Sprintf(`clientID and clientSecret have been read from spec.client.SecretName (\"%s\")`, validFilledOutIDP.Spec.Client.SecretName)), buildLogForUpdatingClaimsValidTrue("some-idp-name"), buildLogForUpdatingOrganizationPolicyValid("some-idp-name", "True", "Success", fmt.Sprintf(`spec.allowAuthentication.organizations.policy (\"%s\") is valid`, string(*validFilledOutIDP.Spec.AllowAuthentication.Organizations.Policy))), buildLogForUpdatingHostValid("some-idp-name", "True", "Success", `spec.githubAPI.host (\"%s\") is valid`, *validFilledOutIDP.Spec.GitHubAPI.Host), @@ -1253,7 +1256,7 @@ func TestController(t *testing.T) { Phase: v1alpha1.GitHubPhaseError, Conditions: []metav1.Condition{ buildClaimsValidatedTrue(t), - buildClientCredentialsObtainedTrue(t, validFilledOutIDP.Spec.Client.SecretName), + buildClientCredentialsSecretValidTrue(t, validFilledOutIDP.Spec.Client.SecretName), buildGitHubConnectionValidFalse(t, fmt.Sprintf(`cannot dial server spec.githubAPI.host (%q): tls: failed to verify certificate: x509: certificate signed by unknown authority`, *validFilledOutIDP.Spec.GitHubAPI.Host)), buildHostValidTrue(t, *validFilledOutIDP.Spec.GitHubAPI.Host), buildOrganizationsPolicyValidTrue(t, *validFilledOutIDP.Spec.AllowAuthentication.Organizations.Policy), @@ -1263,7 +1266,7 @@ func TestController(t *testing.T) { }, }, wantLogs: []string{ - buildLogForUpdatingClientCredentialsObtained("some-idp-name", "True", "Success", fmt.Sprintf(`clientID and clientSecret have been read from spec.client.SecretName (\"%s\")`, validFilledOutIDP.Spec.Client.SecretName)), + buildLogForUpdatingClientCredentialsSecretValid("some-idp-name", "True", "Success", fmt.Sprintf(`clientID and clientSecret have been read from spec.client.SecretName (\"%s\")`, validFilledOutIDP.Spec.Client.SecretName)), buildLogForUpdatingClaimsValidTrue("some-idp-name"), buildLogForUpdatingOrganizationPolicyValid("some-idp-name", "True", "Success", fmt.Sprintf(`spec.allowAuthentication.organizations.policy (\"%s\") is valid`, string(*validFilledOutIDP.Spec.AllowAuthentication.Organizations.Policy))), buildLogForUpdatingHostValid("some-idp-name", "True", "Success", `spec.githubAPI.host (\"%s\") is valid`, *validFilledOutIDP.Spec.GitHubAPI.Host), @@ -1294,7 +1297,7 @@ func TestController(t *testing.T) { Phase: v1alpha1.GitHubPhaseError, Conditions: []metav1.Condition{ buildClaimsValidatedTrue(t), - buildClientCredentialsObtainedTrue(t, validFilledOutIDP.Spec.Client.SecretName), + buildClientCredentialsSecretValidTrue(t, validFilledOutIDP.Spec.Client.SecretName), buildGitHubConnectionValidTrue(t, *validFilledOutIDP.Spec.GitHubAPI.Host), buildHostValidTrue(t, *validFilledOutIDP.Spec.GitHubAPI.Host), buildOrganizationsPolicyValidFalse(t, "spec.allowAuthentication.organizations.policy must be 'OnlyUsersFromAllowedOrganizations' when spec.allowAuthentication.organizations.allowed has organizations listed"), @@ -1304,7 +1307,7 @@ func TestController(t *testing.T) { }, }, wantLogs: []string{ - buildLogForUpdatingClientCredentialsObtained("some-idp-name", "True", "Success", fmt.Sprintf(`clientID and clientSecret have been read from spec.client.SecretName (\"%s\")`, validFilledOutIDP.Spec.Client.SecretName)), + buildLogForUpdatingClientCredentialsSecretValid("some-idp-name", "True", "Success", fmt.Sprintf(`clientID and clientSecret have been read from spec.client.SecretName (\"%s\")`, validFilledOutIDP.Spec.Client.SecretName)), buildLogForUpdatingClaimsValidTrue("some-idp-name"), buildLogForUpdatingOrganizationPolicyValid("some-idp-name", "False", "Invalid", "spec.allowAuthentication.organizations.policy must be 'OnlyUsersFromAllowedOrganizations' when spec.allowAuthentication.organizations.allowed has organizations listed"), buildLogForUpdatingHostValid("some-idp-name", "True", "Success", `spec.githubAPI.host (\"%s\") is valid`, *validFilledOutIDP.Spec.GitHubAPI.Host), @@ -1335,7 +1338,7 @@ func TestController(t *testing.T) { Phase: v1alpha1.GitHubPhaseError, Conditions: []metav1.Condition{ buildClaimsValidatedTrue(t), - buildClientCredentialsObtainedTrue(t, validFilledOutIDP.Spec.Client.SecretName), + buildClientCredentialsSecretValidTrue(t, validFilledOutIDP.Spec.Client.SecretName), buildGitHubConnectionValidTrue(t, *validFilledOutIDP.Spec.GitHubAPI.Host), buildHostValidTrue(t, *validFilledOutIDP.Spec.GitHubAPI.Host), buildOrganizationsPolicyValidFalse(t, "spec.allowAuthentication.organizations.policy must be 'OnlyUsersFromAllowedOrganizations' when spec.allowAuthentication.organizations.allowed has organizations listed"), @@ -1345,7 +1348,7 @@ func TestController(t *testing.T) { }, }, wantLogs: []string{ - buildLogForUpdatingClientCredentialsObtained("some-idp-name", "True", "Success", fmt.Sprintf(`clientID and clientSecret have been read from spec.client.SecretName (\"%s\")`, validFilledOutIDP.Spec.Client.SecretName)), + buildLogForUpdatingClientCredentialsSecretValid("some-idp-name", "True", "Success", fmt.Sprintf(`clientID and clientSecret have been read from spec.client.SecretName (\"%s\")`, validFilledOutIDP.Spec.Client.SecretName)), buildLogForUpdatingClaimsValidTrue("some-idp-name"), buildLogForUpdatingOrganizationPolicyValid("some-idp-name", "False", "Invalid", "spec.allowAuthentication.organizations.policy must be 'OnlyUsersFromAllowedOrganizations' when spec.allowAuthentication.organizations.allowed has organizations listed"), buildLogForUpdatingHostValid("some-idp-name", "True", "Success", `spec.githubAPI.host (\"%s\") is valid`, *validFilledOutIDP.Spec.GitHubAPI.Host), @@ -1376,7 +1379,7 @@ func TestController(t *testing.T) { Phase: v1alpha1.GitHubPhaseError, Conditions: []metav1.Condition{ buildClaimsValidatedTrue(t), - buildClientCredentialsObtainedTrue(t, validFilledOutIDP.Spec.Client.SecretName), + buildClientCredentialsSecretValidTrue(t, validFilledOutIDP.Spec.Client.SecretName), buildGitHubConnectionValidTrue(t, *validFilledOutIDP.Spec.GitHubAPI.Host), buildHostValidTrue(t, *validFilledOutIDP.Spec.GitHubAPI.Host), buildOrganizationsPolicyValidFalse(t, "spec.allowAuthentication.organizations.policy must be 'OnlyUsersFromAllowedOrganizations' when spec.allowAuthentication.organizations.allowed has organizations listed"), @@ -1386,7 +1389,7 @@ func TestController(t *testing.T) { }, }, wantLogs: []string{ - buildLogForUpdatingClientCredentialsObtained("some-idp-name", "True", "Success", fmt.Sprintf(`clientID and clientSecret have been read from spec.client.SecretName (\"%s\")`, validFilledOutIDP.Spec.Client.SecretName)), + buildLogForUpdatingClientCredentialsSecretValid("some-idp-name", "True", "Success", fmt.Sprintf(`clientID and clientSecret have been read from spec.client.SecretName (\"%s\")`, validFilledOutIDP.Spec.Client.SecretName)), buildLogForUpdatingClaimsValidTrue("some-idp-name"), buildLogForUpdatingOrganizationPolicyValid("some-idp-name", "False", "Invalid", "spec.allowAuthentication.organizations.policy must be 'OnlyUsersFromAllowedOrganizations' when spec.allowAuthentication.organizations.allowed has organizations listed"), buildLogForUpdatingHostValid("some-idp-name", "True", "Success", `spec.githubAPI.host (\"%s\") is valid`, *validFilledOutIDP.Spec.GitHubAPI.Host), @@ -1417,7 +1420,7 @@ func TestController(t *testing.T) { Phase: v1alpha1.GitHubPhaseError, Conditions: []metav1.Condition{ buildClaimsValidatedTrue(t), - buildClientCredentialsObtainedTrue(t, validFilledOutIDP.Spec.Client.SecretName), + buildClientCredentialsSecretValidTrue(t, validFilledOutIDP.Spec.Client.SecretName), buildGitHubConnectionValidTrue(t, *validFilledOutIDP.Spec.GitHubAPI.Host), buildHostValidTrue(t, *validFilledOutIDP.Spec.GitHubAPI.Host), buildOrganizationsPolicyValidFalse(t, "spec.allowAuthentication.organizations.policy must be 'AllGitHubUsers' when spec.allowAuthentication.organizations.allowed is empty"), @@ -1427,7 +1430,7 @@ func TestController(t *testing.T) { }, }, wantLogs: []string{ - buildLogForUpdatingClientCredentialsObtained("some-idp-name", "True", "Success", fmt.Sprintf(`clientID and clientSecret have been read from spec.client.SecretName (\"%s\")`, validFilledOutIDP.Spec.Client.SecretName)), + buildLogForUpdatingClientCredentialsSecretValid("some-idp-name", "True", "Success", fmt.Sprintf(`clientID and clientSecret have been read from spec.client.SecretName (\"%s\")`, validFilledOutIDP.Spec.Client.SecretName)), buildLogForUpdatingClaimsValidTrue("some-idp-name"), buildLogForUpdatingOrganizationPolicyValid("some-idp-name", "False", "Invalid", "spec.allowAuthentication.organizations.policy must be 'AllGitHubUsers' when spec.allowAuthentication.organizations.allowed is empty"), buildLogForUpdatingHostValid("some-idp-name", "True", "Success", `spec.githubAPI.host (\"%s\") is valid`, *validFilledOutIDP.Spec.GitHubAPI.Host), @@ -1458,7 +1461,7 @@ func TestController(t *testing.T) { Phase: v1alpha1.GitHubPhaseError, Conditions: []metav1.Condition{ buildClaimsValidatedFalse(t, "spec.claims.username is required"), - buildClientCredentialsObtainedTrue(t, validFilledOutIDP.Spec.Client.SecretName), + buildClientCredentialsSecretValidTrue(t, validFilledOutIDP.Spec.Client.SecretName), buildGitHubConnectionValidTrue(t, *validFilledOutIDP.Spec.GitHubAPI.Host), buildHostValidTrue(t, *validFilledOutIDP.Spec.GitHubAPI.Host), buildOrganizationsPolicyValidTrue(t, *validFilledOutIDP.Spec.AllowAuthentication.Organizations.Policy), @@ -1468,7 +1471,7 @@ func TestController(t *testing.T) { }, }, wantLogs: []string{ - buildLogForUpdatingClientCredentialsObtained("some-idp-name", "True", "Success", fmt.Sprintf(`clientID and clientSecret have been read from spec.client.SecretName (\"%s\")`, validFilledOutIDP.Spec.Client.SecretName)), + buildLogForUpdatingClientCredentialsSecretValid("some-idp-name", "True", "Success", fmt.Sprintf(`clientID and clientSecret have been read from spec.client.SecretName (\"%s\")`, validFilledOutIDP.Spec.Client.SecretName)), buildLogForUpdatingClaimsValidFalse("some-idp-name", "spec.claims.username is required"), buildLogForUpdatingOrganizationPolicyValid("some-idp-name", "True", "Success", fmt.Sprintf(`spec.allowAuthentication.organizations.policy (\"%s\") is valid`, string(*validFilledOutIDP.Spec.AllowAuthentication.Organizations.Policy))), buildLogForUpdatingHostValid("some-idp-name", "True", "Success", `spec.githubAPI.host (\"%s\") is valid`, *validFilledOutIDP.Spec.GitHubAPI.Host), @@ -1499,7 +1502,7 @@ func TestController(t *testing.T) { Phase: v1alpha1.GitHubPhaseError, Conditions: []metav1.Condition{ buildClaimsValidatedFalse(t, `spec.claims.username ("a") is not valid`), - buildClientCredentialsObtainedTrue(t, validFilledOutIDP.Spec.Client.SecretName), + buildClientCredentialsSecretValidTrue(t, validFilledOutIDP.Spec.Client.SecretName), buildGitHubConnectionValidTrue(t, *validFilledOutIDP.Spec.GitHubAPI.Host), buildHostValidTrue(t, *validFilledOutIDP.Spec.GitHubAPI.Host), buildOrganizationsPolicyValidTrue(t, *validFilledOutIDP.Spec.AllowAuthentication.Organizations.Policy), @@ -1509,7 +1512,7 @@ func TestController(t *testing.T) { }, }, wantLogs: []string{ - buildLogForUpdatingClientCredentialsObtained("some-idp-name", "True", "Success", fmt.Sprintf(`clientID and clientSecret have been read from spec.client.SecretName (\"%s\")`, validFilledOutIDP.Spec.Client.SecretName)), + buildLogForUpdatingClientCredentialsSecretValid("some-idp-name", "True", "Success", fmt.Sprintf(`clientID and clientSecret have been read from spec.client.SecretName (\"%s\")`, validFilledOutIDP.Spec.Client.SecretName)), buildLogForUpdatingClaimsValidFalse("some-idp-name", `spec.claims.username (\"a\") is not valid`), buildLogForUpdatingOrganizationPolicyValid("some-idp-name", "True", "Success", fmt.Sprintf(`spec.allowAuthentication.organizations.policy (\"%s\") is valid`, string(*validFilledOutIDP.Spec.AllowAuthentication.Organizations.Policy))), buildLogForUpdatingHostValid("some-idp-name", "True", "Success", `spec.githubAPI.host (\"%s\") is valid`, *validFilledOutIDP.Spec.GitHubAPI.Host), @@ -1540,7 +1543,7 @@ func TestController(t *testing.T) { Phase: v1alpha1.GitHubPhaseError, Conditions: []metav1.Condition{ buildClaimsValidatedFalse(t, "spec.claims.groups is required"), - buildClientCredentialsObtainedTrue(t, validFilledOutIDP.Spec.Client.SecretName), + buildClientCredentialsSecretValidTrue(t, validFilledOutIDP.Spec.Client.SecretName), buildGitHubConnectionValidTrue(t, *validFilledOutIDP.Spec.GitHubAPI.Host), buildHostValidTrue(t, *validFilledOutIDP.Spec.GitHubAPI.Host), buildOrganizationsPolicyValidTrue(t, *validFilledOutIDP.Spec.AllowAuthentication.Organizations.Policy), @@ -1550,7 +1553,7 @@ func TestController(t *testing.T) { }, }, wantLogs: []string{ - buildLogForUpdatingClientCredentialsObtained("some-idp-name", "True", "Success", fmt.Sprintf(`clientID and clientSecret have been read from spec.client.SecretName (\"%s\")`, validFilledOutIDP.Spec.Client.SecretName)), + buildLogForUpdatingClientCredentialsSecretValid("some-idp-name", "True", "Success", fmt.Sprintf(`clientID and clientSecret have been read from spec.client.SecretName (\"%s\")`, validFilledOutIDP.Spec.Client.SecretName)), buildLogForUpdatingClaimsValidFalse("some-idp-name", "spec.claims.groups is required"), buildLogForUpdatingOrganizationPolicyValid("some-idp-name", "True", "Success", fmt.Sprintf(`spec.allowAuthentication.organizations.policy (\"%s\") is valid`, string(*validFilledOutIDP.Spec.AllowAuthentication.Organizations.Policy))), buildLogForUpdatingHostValid("some-idp-name", "True", "Success", `spec.githubAPI.host (\"%s\") is valid`, *validFilledOutIDP.Spec.GitHubAPI.Host), @@ -1581,7 +1584,7 @@ func TestController(t *testing.T) { Phase: v1alpha1.GitHubPhaseError, Conditions: []metav1.Condition{ buildClaimsValidatedFalse(t, `spec.claims.groups ("b") is not valid`), - buildClientCredentialsObtainedTrue(t, validFilledOutIDP.Spec.Client.SecretName), + buildClientCredentialsSecretValidTrue(t, validFilledOutIDP.Spec.Client.SecretName), buildGitHubConnectionValidTrue(t, *validFilledOutIDP.Spec.GitHubAPI.Host), buildHostValidTrue(t, *validFilledOutIDP.Spec.GitHubAPI.Host), buildOrganizationsPolicyValidTrue(t, *validFilledOutIDP.Spec.AllowAuthentication.Organizations.Policy), @@ -1591,7 +1594,7 @@ func TestController(t *testing.T) { }, }, wantLogs: []string{ - buildLogForUpdatingClientCredentialsObtained("some-idp-name", "True", "Success", fmt.Sprintf(`clientID and clientSecret have been read from spec.client.SecretName (\"%s\")`, validFilledOutIDP.Spec.Client.SecretName)), + buildLogForUpdatingClientCredentialsSecretValid("some-idp-name", "True", "Success", fmt.Sprintf(`clientID and clientSecret have been read from spec.client.SecretName (\"%s\")`, validFilledOutIDP.Spec.Client.SecretName)), buildLogForUpdatingClaimsValidFalse("some-idp-name", `spec.claims.groups (\"b\") is not valid`), buildLogForUpdatingOrganizationPolicyValid("some-idp-name", "True", "Success", fmt.Sprintf(`spec.allowAuthentication.organizations.policy (\"%s\") is valid`, string(*validFilledOutIDP.Spec.AllowAuthentication.Organizations.Policy))), buildLogForUpdatingHostValid("some-idp-name", "True", "Success", `spec.githubAPI.host (\"%s\") is valid`, *validFilledOutIDP.Spec.GitHubAPI.Host), @@ -1618,7 +1621,7 @@ func TestController(t *testing.T) { Phase: v1alpha1.GitHubPhaseError, Conditions: []metav1.Condition{ buildClaimsValidatedTrue(t), - buildClientCredentialsObtainedFalse( + buildClientCredentialsSecretValidFalse( t, fmt.Sprintf("secret %q not found", validMinimalIDP.Spec.Client.SecretName), validMinimalIDP.Spec.Client.SecretName, @@ -1634,7 +1637,7 @@ func TestController(t *testing.T) { }, }, wantLogs: []string{ - buildLogForUpdatingClientCredentialsObtained("minimal-idp-name", "False", "SecretNotFound", `secret \"some-secret-name\" not found: secret from spec.client.SecretName (\"some-secret-name\") must be found in namespace \"some-namespace\" with type \"secrets.pinniped.dev/github-client\" and keys \"clientID\" and \"clientSecret\"`), + buildLogForUpdatingClientCredentialsSecretValid("minimal-idp-name", "False", "SecretNotFound", `secret \"some-secret-name\" not found: secret from spec.client.SecretName (\"some-secret-name\") must be found in namespace \"some-namespace\" with type \"secrets.pinniped.dev/github-client\" and keys \"clientID\" and \"clientSecret\"`), buildLogForUpdatingClaimsValidTrue("minimal-idp-name"), buildLogForUpdatingOrganizationPolicyValid("minimal-idp-name", "True", "Success", fmt.Sprintf(`spec.allowAuthentication.organizations.policy (\"%s\") is valid`, string(*validMinimalIDP.Spec.AllowAuthentication.Organizations.Policy))), buildLogForUpdatingHostValid("minimal-idp-name", "True", "Success", `spec.githubAPI.host (\"%s\") is valid`, *validFilledOutIDP.Spec.GitHubAPI.Host), @@ -1661,7 +1664,7 @@ func TestController(t *testing.T) { Phase: v1alpha1.GitHubPhaseError, Conditions: []metav1.Condition{ buildClaimsValidatedTrue(t), - buildClientCredentialsObtainedFalse( + buildClientCredentialsSecretValidFalse( t, `wrong secret type "other-type"`, validMinimalIDP.Spec.Client.SecretName, @@ -1677,7 +1680,7 @@ func TestController(t *testing.T) { }, }, wantLogs: []string{ - buildLogForUpdatingClientCredentialsObtained("minimal-idp-name", "False", "SecretNotFound", `wrong secret type \"other-type\": secret from spec.client.SecretName (\"some-secret-name\") must be found in namespace \"some-namespace\" with type \"secrets.pinniped.dev/github-client\" and keys \"clientID\" and \"clientSecret\"`), + buildLogForUpdatingClientCredentialsSecretValid("minimal-idp-name", "False", "SecretNotFound", `wrong secret type \"other-type\": secret from spec.client.SecretName (\"some-secret-name\") must be found in namespace \"some-namespace\" with type \"secrets.pinniped.dev/github-client\" and keys \"clientID\" and \"clientSecret\"`), buildLogForUpdatingClaimsValidTrue("minimal-idp-name"), buildLogForUpdatingOrganizationPolicyValid("minimal-idp-name", "True", "Success", fmt.Sprintf(`spec.allowAuthentication.organizations.policy (\"%s\") is valid`, string(*validMinimalIDP.Spec.AllowAuthentication.Organizations.Policy))), buildLogForUpdatingHostValid("minimal-idp-name", "True", "Success", `spec.githubAPI.host (\"%s\") is valid`, *validMinimalIDP.Spec.GitHubAPI.Host), @@ -1704,7 +1707,7 @@ func TestController(t *testing.T) { Phase: v1alpha1.GitHubPhaseError, Conditions: []metav1.Condition{ buildClaimsValidatedTrue(t), - buildClientCredentialsObtainedFalse( + buildClientCredentialsSecretValidFalse( t, `missing key "clientID"`, validMinimalIDP.Spec.Client.SecretName, @@ -1720,7 +1723,7 @@ func TestController(t *testing.T) { }, }, wantLogs: []string{ - buildLogForUpdatingClientCredentialsObtained("minimal-idp-name", "False", "SecretNotFound", `missing key \"clientID\": secret from spec.client.SecretName (\"some-secret-name\") must be found in namespace \"some-namespace\" with type \"secrets.pinniped.dev/github-client\" and keys \"clientID\" and \"clientSecret\"`), + buildLogForUpdatingClientCredentialsSecretValid("minimal-idp-name", "False", "SecretNotFound", `missing key \"clientID\": secret from spec.client.SecretName (\"some-secret-name\") must be found in namespace \"some-namespace\" with type \"secrets.pinniped.dev/github-client\" and keys \"clientID\" and \"clientSecret\"`), buildLogForUpdatingClaimsValidTrue("minimal-idp-name"), buildLogForUpdatingOrganizationPolicyValid("minimal-idp-name", "True", "Success", fmt.Sprintf(`spec.allowAuthentication.organizations.policy (\"%s\") is valid`, string(*validMinimalIDP.Spec.AllowAuthentication.Organizations.Policy))), buildLogForUpdatingHostValid("minimal-idp-name", "True", "Success", `spec.githubAPI.host (\"%s\") is valid`, *validFilledOutIDP.Spec.GitHubAPI.Host), @@ -1747,7 +1750,7 @@ func TestController(t *testing.T) { Phase: v1alpha1.GitHubPhaseError, Conditions: []metav1.Condition{ buildClaimsValidatedTrue(t), - buildClientCredentialsObtainedFalse( + buildClientCredentialsSecretValidFalse( t, `missing key "clientSecret"`, validMinimalIDP.Spec.Client.SecretName, @@ -1763,7 +1766,7 @@ func TestController(t *testing.T) { }, }, wantLogs: []string{ - buildLogForUpdatingClientCredentialsObtained("minimal-idp-name", "False", "SecretNotFound", `missing key \"clientSecret\": secret from spec.client.SecretName (\"some-secret-name\") must be found in namespace \"some-namespace\" with type \"secrets.pinniped.dev/github-client\" and keys \"clientID\" and \"clientSecret\"`), + buildLogForUpdatingClientCredentialsSecretValid("minimal-idp-name", "False", "SecretNotFound", `missing key \"clientSecret\": secret from spec.client.SecretName (\"some-secret-name\") must be found in namespace \"some-namespace\" with type \"secrets.pinniped.dev/github-client\" and keys \"clientID\" and \"clientSecret\"`), buildLogForUpdatingClaimsValidTrue("minimal-idp-name"), buildLogForUpdatingOrganizationPolicyValid("minimal-idp-name", "True", "Success", fmt.Sprintf(`spec.allowAuthentication.organizations.policy (\"%s\") is valid`, string(*validMinimalIDP.Spec.AllowAuthentication.Organizations.Policy))), buildLogForUpdatingHostValid("minimal-idp-name", "True", "Success", `spec.githubAPI.host (\"%s\") is valid`, *validMinimalIDP.Spec.GitHubAPI.Host), @@ -1790,7 +1793,7 @@ func TestController(t *testing.T) { Phase: v1alpha1.GitHubPhaseError, Conditions: []metav1.Condition{ buildClaimsValidatedTrue(t), - buildClientCredentialsObtainedFalse( + buildClientCredentialsSecretValidFalse( t, "extra keys found", validMinimalIDP.Spec.Client.SecretName, @@ -1806,7 +1809,7 @@ func TestController(t *testing.T) { }, }, wantLogs: []string{ - buildLogForUpdatingClientCredentialsObtained("minimal-idp-name", "False", "SecretNotFound", `extra keys found: secret from spec.client.SecretName (\"some-secret-name\") must be found in namespace \"some-namespace\" with type \"secrets.pinniped.dev/github-client\" and keys \"clientID\" and \"clientSecret\"`), + buildLogForUpdatingClientCredentialsSecretValid("minimal-idp-name", "False", "SecretNotFound", `extra keys found: secret from spec.client.SecretName (\"some-secret-name\") must be found in namespace \"some-namespace\" with type \"secrets.pinniped.dev/github-client\" and keys \"clientID\" and \"clientSecret\"`), buildLogForUpdatingClaimsValidTrue("minimal-idp-name"), buildLogForUpdatingOrganizationPolicyValid("minimal-idp-name", "True", "Success", fmt.Sprintf(`spec.allowAuthentication.organizations.policy (\"%s\") is valid`, string(*validMinimalIDP.Spec.AllowAuthentication.Organizations.Policy))), buildLogForUpdatingHostValid("minimal-idp-name", "True", "Success", `spec.githubAPI.host (\"%s\") is valid`, *validFilledOutIDP.Spec.GitHubAPI.Host), @@ -1852,7 +1855,7 @@ func TestController(t *testing.T) { kubeInformers.Core().V1().Secrets(), logger, controllerlib.WithInformer, - frozenClock, + frozenClockForLastTransitionTime, dialer, ) @@ -1879,7 +1882,7 @@ func TestController(t *testing.T) { // Do not expect any particular order in the cache var actualProvider *upstreamgithub.Provider for _, possibleIDP := range actualIDPList { - if possibleIDP.GetName() == tt.wantResultingCache[i].Name { + if possibleIDP.GetResourceName() == tt.wantResultingCache[i].Name { var ok bool actualProvider, ok = possibleIDP.(*upstreamgithub.Provider) require.True(t, ok) @@ -1887,7 +1890,7 @@ func TestController(t *testing.T) { } } - require.Equal(t, tt.wantResultingCache[i].Name, actualProvider.GetName()) + require.Equal(t, tt.wantResultingCache[i].Name, actualProvider.GetResourceName()) require.Equal(t, tt.wantResultingCache[i].ResourceUID, actualProvider.GetResourceUID()) require.Equal(t, tt.wantResultingCache[i].OAuth2Config.ClientID, actualProvider.GetClientID()) require.Equal(t, tt.wantResultingCache[i].GroupNameAttribute, actualProvider.GetGroupNameAttribute()) @@ -1924,15 +1927,6 @@ func TestController(t *testing.T) { require.NotNil(t, actualIDP, "must find IDP with name %s", tt.wantResultingUpstreams[i].Name) require.Len(t, actualIDP.Status.Conditions, countExpectedConditions) - - // Update all expected conditions to the frozenTime. - // TODO: Push this out to the test table - for j := range countExpectedConditions { - // Get this as a pointer so that we can update the value within the array - condition := &tt.wantResultingUpstreams[i].Status.Conditions[j] - condition.LastTransitionTime = metav1.Time{Time: frozenClock.Now()} - } - require.Equal(t, tt.wantResultingUpstreams[i], *actualIDP) } @@ -1967,8 +1961,9 @@ func TestController_OnlyWantActions(t *testing.T) { oneHourAgo := metav1.Time{Time: time.Now().Add(-1 * time.Hour)} namespace := "existing-conditions-namespace" - nowDoesntMatter := time.Date(1122, time.September, 33, 4, 55, 56, 778899, time.Local) - frozenClock := clocktesting.NewFakeClock(nowDoesntMatter) + wantFrozenTime := time.Date(1122, time.September, 33, 4, 55, 56, 778899, time.Local) + frozenClockForLastTransitionTime := clocktesting.NewFakeClock(wantFrozenTime) + wantLastTransitionTime := metav1.Time{Time: wantFrozenTime} goodSecret := &corev1.Secret{ ObjectMeta: metav1.ObjectMeta{ @@ -2051,7 +2046,7 @@ func TestController_OnlyWantActions(t *testing.T) { Message: "spec.claims.username is required", }, { - Type: ClientCredentialsObtained, + Type: ClientCredentialsSecretValid, Status: metav1.ConditionFalse, ObservedGeneration: 333, LastTransitionTime: oneHourAgo, @@ -2132,7 +2127,7 @@ func TestController_OnlyWantActions(t *testing.T) { for i := range idp.Status.Conditions { idp.Status.Conditions[i].ObservedGeneration = 400 } - idp.Status.Conditions[0].LastTransitionTime = metav1.Time{Time: nowDoesntMatter} + idp.Status.Conditions[0].LastTransitionTime = wantLastTransitionTime wantAction := coretesting.NewUpdateSubresourceAction(githubIDPGVR, "status", namespace, idp) return wantAction }(), @@ -2158,15 +2153,15 @@ func TestController_OnlyWantActions(t *testing.T) { Type: ClaimsValid, Status: metav1.ConditionTrue, ObservedGeneration: 1234, - LastTransitionTime: metav1.NewTime(frozenClock.Now()), + LastTransitionTime: wantLastTransitionTime, Reason: upstreamwatchers.ReasonSuccess, Message: "spec.claims are valid", }, { - Type: ClientCredentialsObtained, + Type: ClientCredentialsSecretValid, Status: metav1.ConditionTrue, ObservedGeneration: 1234, - LastTransitionTime: metav1.NewTime(frozenClock.Now()), + LastTransitionTime: wantLastTransitionTime, Reason: upstreamwatchers.ReasonSuccess, Message: `clientID and clientSecret have been read from spec.client.SecretName ("some-secret-name")`, }, @@ -2174,7 +2169,7 @@ func TestController_OnlyWantActions(t *testing.T) { Type: GitHubConnectionValid, Status: metav1.ConditionTrue, ObservedGeneration: 1234, - LastTransitionTime: metav1.NewTime(frozenClock.Now()), + LastTransitionTime: wantLastTransitionTime, Reason: upstreamwatchers.ReasonSuccess, Message: fmt.Sprintf("spec.githubAPI.host (%q) is reachable and TLS verification succeeds", goodServerDomain), }, @@ -2182,7 +2177,7 @@ func TestController_OnlyWantActions(t *testing.T) { Type: HostValid, Status: metav1.ConditionTrue, ObservedGeneration: 1234, - LastTransitionTime: metav1.NewTime(frozenClock.Now()), + LastTransitionTime: wantLastTransitionTime, Reason: upstreamwatchers.ReasonSuccess, Message: fmt.Sprintf("spec.githubAPI.host (%q) is valid", goodServerDomain), }, @@ -2190,7 +2185,7 @@ func TestController_OnlyWantActions(t *testing.T) { Type: OrganizationsPolicyValid, Status: metav1.ConditionTrue, ObservedGeneration: 1234, - LastTransitionTime: metav1.NewTime(frozenClock.Now()), + LastTransitionTime: wantLastTransitionTime, Reason: upstreamwatchers.ReasonSuccess, Message: `spec.allowAuthentication.organizations.policy ("AllGitHubUsers") is valid`, }, @@ -2198,7 +2193,7 @@ func TestController_OnlyWantActions(t *testing.T) { Type: TLSConfigurationValid, Status: metav1.ConditionTrue, ObservedGeneration: 1234, - LastTransitionTime: metav1.NewTime(frozenClock.Now()), + LastTransitionTime: wantLastTransitionTime, Reason: upstreamwatchers.ReasonSuccess, Message: "spec.githubAPI.tls.certificateAuthorityData is valid", }, @@ -2234,7 +2229,7 @@ func TestController_OnlyWantActions(t *testing.T) { kubeInformers.Core().V1().Secrets(), logger, controllerlib.WithInformer, - frozenClock, + frozenClockForLastTransitionTime, tls.Dial, ) diff --git a/internal/controller/supervisorconfig/oidcupstreamwatcher/oidc_upstream_watcher.go b/internal/controller/supervisorconfig/oidcupstreamwatcher/oidc_upstream_watcher.go index 913371f98..69bd173a5 100644 --- a/internal/controller/supervisorconfig/oidcupstreamwatcher/oidc_upstream_watcher.go +++ b/internal/controller/supervisorconfig/oidcupstreamwatcher/oidc_upstream_watcher.go @@ -54,7 +54,7 @@ const ( oidcValidatorCacheTTL = 15 * time.Minute // Constants related to conditions. - typeClientCredentialsValid = "ClientCredentialsValid" //nolint:gosec // this is not a credential + typeClientCredentialsSecretValid = "ClientCredentialsSecretValid" //nolint:gosec // this is not a credential typeAdditionalAuthorizeParametersValid = "AdditionalAuthorizeParametersValid" typeOIDCDiscoverySucceeded = "OIDCDiscoverySucceeded" @@ -260,7 +260,7 @@ func (c *oidcWatcherController) validateUpstream(ctx controllerlib.Context, upst return nil } -// validateSecret validates the .spec.client.secretName field and returns the appropriate ClientCredentialsValid condition. +// validateSecret validates the .spec.client.secretName field and returns the appropriate ClientCredentialsSecretValid condition. func (c *oidcWatcherController) validateSecret(upstream *v1alpha1.OIDCIdentityProvider, result *upstreamoidc.ProviderConfig) *metav1.Condition { secretName := upstream.Spec.Client.SecretName @@ -268,7 +268,7 @@ func (c *oidcWatcherController) validateSecret(upstream *v1alpha1.OIDCIdentityPr secret, err := c.secretInformer.Lister().Secrets(upstream.Namespace).Get(secretName) if err != nil { return &metav1.Condition{ - Type: typeClientCredentialsValid, + Type: typeClientCredentialsSecretValid, Status: metav1.ConditionFalse, Reason: upstreamwatchers.ReasonNotFound, Message: err.Error(), @@ -278,7 +278,7 @@ func (c *oidcWatcherController) validateSecret(upstream *v1alpha1.OIDCIdentityPr // Validate the secret .type field. if secret.Type != oidcClientSecretType { return &metav1.Condition{ - Type: typeClientCredentialsValid, + Type: typeClientCredentialsSecretValid, Status: metav1.ConditionFalse, Reason: upstreamwatchers.ReasonWrongType, Message: fmt.Sprintf("referenced Secret %q has wrong type %q (should be %q)", secretName, secret.Type, oidcClientSecretType), @@ -290,7 +290,7 @@ func (c *oidcWatcherController) validateSecret(upstream *v1alpha1.OIDCIdentityPr clientSecret := secret.Data[clientSecretDataKey] if len(clientID) == 0 || len(clientSecret) == 0 { return &metav1.Condition{ - Type: typeClientCredentialsValid, + Type: typeClientCredentialsSecretValid, Status: metav1.ConditionFalse, Reason: upstreamwatchers.ReasonMissingKeys, Message: fmt.Sprintf("referenced Secret %q is missing required keys %q", secretName, []string{clientIDDataKey, clientSecretDataKey}), @@ -301,7 +301,7 @@ func (c *oidcWatcherController) validateSecret(upstream *v1alpha1.OIDCIdentityPr result.Config.ClientID = string(clientID) result.Config.ClientSecret = string(clientSecret) return &metav1.Condition{ - Type: typeClientCredentialsValid, + Type: typeClientCredentialsSecretValid, Status: metav1.ConditionTrue, Reason: upstreamwatchers.ReasonSuccess, Message: "loaded client credentials", diff --git a/internal/controller/supervisorconfig/oidcupstreamwatcher/oidc_upstream_watcher_test.go b/internal/controller/supervisorconfig/oidcupstreamwatcher/oidc_upstream_watcher_test.go index 5810e272d..b8f82a966 100644 --- a/internal/controller/supervisorconfig/oidcupstreamwatcher/oidc_upstream_watcher_test.go +++ b/internal/controller/supervisorconfig/oidcupstreamwatcher/oidc_upstream_watcher_test.go @@ -174,10 +174,10 @@ func TestOIDCUpstreamWatcherControllerSync(t *testing.T) { inputSecrets: []runtime.Object{}, wantErr: controllerlib.ErrSyntheticRequeue.Error(), wantLogs: []string{ - `oidc-upstream-observer "level"=0 "msg"="updated condition" "name"="test-name" "namespace"="test-namespace" "message"="secret \"test-client-secret\" not found" "reason"="SecretNotFound" "status"="False" "type"="ClientCredentialsValid"`, + `oidc-upstream-observer "level"=0 "msg"="updated condition" "name"="test-name" "namespace"="test-namespace" "message"="secret \"test-client-secret\" not found" "reason"="SecretNotFound" "status"="False" "type"="ClientCredentialsSecretValid"`, `oidc-upstream-observer "level"=0 "msg"="updated condition" "name"="test-name" "namespace"="test-namespace" "message"="discovered issuer configuration" "reason"="Success" "status"="True" "type"="OIDCDiscoverySucceeded"`, `oidc-upstream-observer "level"=0 "msg"="updated condition" "name"="test-name" "namespace"="test-namespace" "message"="additionalAuthorizeParameters parameter names are allowed" "reason"="Success" "status"="True" "type"="AdditionalAuthorizeParametersValid"`, - `oidc-upstream-observer "msg"="found failing condition" "error"="OIDCIdentityProvider has a failing condition" "message"="secret \"test-client-secret\" not found" "name"="test-name" "namespace"="test-namespace" "reason"="SecretNotFound" "type"="ClientCredentialsValid"`, + `oidc-upstream-observer "msg"="found failing condition" "error"="OIDCIdentityProvider has a failing condition" "message"="secret \"test-client-secret\" not found" "name"="test-name" "namespace"="test-namespace" "reason"="SecretNotFound" "type"="ClientCredentialsSecretValid"`, }, wantResultingCache: []*oidctestutil.TestUpstreamOIDCIdentityProvider{}, wantResultingUpstreams: []v1alpha1.OIDCIdentityProvider{{ @@ -187,7 +187,7 @@ func TestOIDCUpstreamWatcherControllerSync(t *testing.T) { Conditions: []metav1.Condition{ happyAdditionalAuthorizeParametersValidCondition, { - Type: "ClientCredentialsValid", + Type: "ClientCredentialsSecretValid", Status: "False", LastTransitionTime: now, Reason: "SecretNotFound", @@ -221,10 +221,10 @@ func TestOIDCUpstreamWatcherControllerSync(t *testing.T) { }}, wantErr: controllerlib.ErrSyntheticRequeue.Error(), wantLogs: []string{ - `oidc-upstream-observer "level"=0 "msg"="updated condition" "name"="test-name" "namespace"="test-namespace" "message"="referenced Secret \"test-client-secret\" has wrong type \"some-other-type\" (should be \"secrets.pinniped.dev/oidc-client\")" "reason"="SecretWrongType" "status"="False" "type"="ClientCredentialsValid"`, + `oidc-upstream-observer "level"=0 "msg"="updated condition" "name"="test-name" "namespace"="test-namespace" "message"="referenced Secret \"test-client-secret\" has wrong type \"some-other-type\" (should be \"secrets.pinniped.dev/oidc-client\")" "reason"="SecretWrongType" "status"="False" "type"="ClientCredentialsSecretValid"`, `oidc-upstream-observer "level"=0 "msg"="updated condition" "name"="test-name" "namespace"="test-namespace" "message"="discovered issuer configuration" "reason"="Success" "status"="True" "type"="OIDCDiscoverySucceeded"`, `oidc-upstream-observer "level"=0 "msg"="updated condition" "name"="test-name" "namespace"="test-namespace" "message"="additionalAuthorizeParameters parameter names are allowed" "reason"="Success" "status"="True" "type"="AdditionalAuthorizeParametersValid"`, - `oidc-upstream-observer "msg"="found failing condition" "error"="OIDCIdentityProvider has a failing condition" "message"="referenced Secret \"test-client-secret\" has wrong type \"some-other-type\" (should be \"secrets.pinniped.dev/oidc-client\")" "name"="test-name" "namespace"="test-namespace" "reason"="SecretWrongType" "type"="ClientCredentialsValid"`, + `oidc-upstream-observer "msg"="found failing condition" "error"="OIDCIdentityProvider has a failing condition" "message"="referenced Secret \"test-client-secret\" has wrong type \"some-other-type\" (should be \"secrets.pinniped.dev/oidc-client\")" "name"="test-name" "namespace"="test-namespace" "reason"="SecretWrongType" "type"="ClientCredentialsSecretValid"`, }, wantResultingCache: []*oidctestutil.TestUpstreamOIDCIdentityProvider{}, wantResultingUpstreams: []v1alpha1.OIDCIdentityProvider{{ @@ -234,7 +234,7 @@ func TestOIDCUpstreamWatcherControllerSync(t *testing.T) { Conditions: []metav1.Condition{ happyAdditionalAuthorizeParametersValidCondition, { - Type: "ClientCredentialsValid", + Type: "ClientCredentialsSecretValid", Status: "False", LastTransitionTime: now, Reason: "SecretWrongType", @@ -267,10 +267,10 @@ func TestOIDCUpstreamWatcherControllerSync(t *testing.T) { }}, wantErr: controllerlib.ErrSyntheticRequeue.Error(), wantLogs: []string{ - `oidc-upstream-observer "level"=0 "msg"="updated condition" "name"="test-name" "namespace"="test-namespace" "message"="referenced Secret \"test-client-secret\" is missing required keys [\"clientID\" \"clientSecret\"]" "reason"="SecretMissingKeys" "status"="False" "type"="ClientCredentialsValid"`, + `oidc-upstream-observer "level"=0 "msg"="updated condition" "name"="test-name" "namespace"="test-namespace" "message"="referenced Secret \"test-client-secret\" is missing required keys [\"clientID\" \"clientSecret\"]" "reason"="SecretMissingKeys" "status"="False" "type"="ClientCredentialsSecretValid"`, `oidc-upstream-observer "level"=0 "msg"="updated condition" "name"="test-name" "namespace"="test-namespace" "message"="discovered issuer configuration" "reason"="Success" "status"="True" "type"="OIDCDiscoverySucceeded"`, `oidc-upstream-observer "level"=0 "msg"="updated condition" "name"="test-name" "namespace"="test-namespace" "message"="additionalAuthorizeParameters parameter names are allowed" "reason"="Success" "status"="True" "type"="AdditionalAuthorizeParametersValid"`, - `oidc-upstream-observer "msg"="found failing condition" "error"="OIDCIdentityProvider has a failing condition" "message"="referenced Secret \"test-client-secret\" is missing required keys [\"clientID\" \"clientSecret\"]" "name"="test-name" "namespace"="test-namespace" "reason"="SecretMissingKeys" "type"="ClientCredentialsValid"`, + `oidc-upstream-observer "msg"="found failing condition" "error"="OIDCIdentityProvider has a failing condition" "message"="referenced Secret \"test-client-secret\" is missing required keys [\"clientID\" \"clientSecret\"]" "name"="test-name" "namespace"="test-namespace" "reason"="SecretMissingKeys" "type"="ClientCredentialsSecretValid"`, }, wantResultingCache: []*oidctestutil.TestUpstreamOIDCIdentityProvider{}, wantResultingUpstreams: []v1alpha1.OIDCIdentityProvider{{ @@ -280,7 +280,7 @@ func TestOIDCUpstreamWatcherControllerSync(t *testing.T) { Conditions: []metav1.Condition{ happyAdditionalAuthorizeParametersValidCondition, { - Type: "ClientCredentialsValid", + Type: "ClientCredentialsSecretValid", Status: "False", LastTransitionTime: now, Reason: "SecretMissingKeys", @@ -316,7 +316,7 @@ func TestOIDCUpstreamWatcherControllerSync(t *testing.T) { }}, wantErr: controllerlib.ErrSyntheticRequeue.Error(), wantLogs: []string{ - `oidc-upstream-observer "level"=0 "msg"="updated condition" "name"="test-name" "namespace"="test-namespace" "message"="loaded client credentials" "reason"="Success" "status"="True" "type"="ClientCredentialsValid"`, + `oidc-upstream-observer "level"=0 "msg"="updated condition" "name"="test-name" "namespace"="test-namespace" "message"="loaded client credentials" "reason"="Success" "status"="True" "type"="ClientCredentialsSecretValid"`, `oidc-upstream-observer "level"=0 "msg"="updated condition" "name"="test-name" "namespace"="test-namespace" "message"="spec.certificateAuthorityData is invalid: illegal base64 data at input byte 7" "reason"="InvalidTLSConfig" "status"="False" "type"="OIDCDiscoverySucceeded"`, `oidc-upstream-observer "level"=0 "msg"="updated condition" "name"="test-name" "namespace"="test-namespace" "message"="additionalAuthorizeParameters parameter names are allowed" "reason"="Success" "status"="True" "type"="AdditionalAuthorizeParametersValid"`, `oidc-upstream-observer "msg"="found failing condition" "error"="OIDCIdentityProvider has a failing condition" "message"="spec.certificateAuthorityData is invalid: illegal base64 data at input byte 7" "name"="test-name" "namespace"="test-namespace" "reason"="InvalidTLSConfig" "type"="OIDCDiscoverySucceeded"`, @@ -329,7 +329,7 @@ func TestOIDCUpstreamWatcherControllerSync(t *testing.T) { Conditions: []metav1.Condition{ happyAdditionalAuthorizeParametersValidCondition, { - Type: "ClientCredentialsValid", + Type: "ClientCredentialsSecretValid", Status: "True", LastTransitionTime: now, Reason: "Success", @@ -365,7 +365,7 @@ func TestOIDCUpstreamWatcherControllerSync(t *testing.T) { }}, wantErr: controllerlib.ErrSyntheticRequeue.Error(), wantLogs: []string{ - `oidc-upstream-observer "level"=0 "msg"="updated condition" "name"="test-name" "namespace"="test-namespace" "message"="loaded client credentials" "reason"="Success" "status"="True" "type"="ClientCredentialsValid"`, + `oidc-upstream-observer "level"=0 "msg"="updated condition" "name"="test-name" "namespace"="test-namespace" "message"="loaded client credentials" "reason"="Success" "status"="True" "type"="ClientCredentialsSecretValid"`, `oidc-upstream-observer "level"=0 "msg"="updated condition" "name"="test-name" "namespace"="test-namespace" "message"="spec.certificateAuthorityData is invalid: no certificates found" "reason"="InvalidTLSConfig" "status"="False" "type"="OIDCDiscoverySucceeded"`, `oidc-upstream-observer "level"=0 "msg"="updated condition" "name"="test-name" "namespace"="test-namespace" "message"="additionalAuthorizeParameters parameter names are allowed" "reason"="Success" "status"="True" "type"="AdditionalAuthorizeParametersValid"`, `oidc-upstream-observer "msg"="found failing condition" "error"="OIDCIdentityProvider has a failing condition" "message"="spec.certificateAuthorityData is invalid: no certificates found" "name"="test-name" "namespace"="test-namespace" "reason"="InvalidTLSConfig" "type"="OIDCDiscoverySucceeded"`, @@ -378,7 +378,7 @@ func TestOIDCUpstreamWatcherControllerSync(t *testing.T) { Conditions: []metav1.Condition{ happyAdditionalAuthorizeParametersValidCondition, { - Type: "ClientCredentialsValid", + Type: "ClientCredentialsSecretValid", Status: "True", LastTransitionTime: now, Reason: "Success", @@ -411,7 +411,7 @@ func TestOIDCUpstreamWatcherControllerSync(t *testing.T) { }}, wantErr: controllerlib.ErrSyntheticRequeue.Error(), wantLogs: []string{ - `oidc-upstream-observer "level"=0 "msg"="updated condition" "name"="test-name" "namespace"="test-namespace" "message"="loaded client credentials" "reason"="Success" "status"="True" "type"="ClientCredentialsValid"`, + `oidc-upstream-observer "level"=0 "msg"="updated condition" "name"="test-name" "namespace"="test-namespace" "message"="loaded client credentials" "reason"="Success" "status"="True" "type"="ClientCredentialsSecretValid"`, `oidc-upstream-observer "level"=0 "msg"="updated condition" "name"="test-name" "namespace"="test-namespace" "message"="failed to parse issuer URL: parse \"%invalid-url-that-is-really-really-long-nanananananananannanananan-batman-nanananananananananananananana-batman-lalalalalalalalalal-batman-weeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeee\": invalid URL escape \"%in\"" "reason"="Unreachable" "status"="False" "type"="OIDCDiscoverySucceeded"`, `oidc-upstream-observer "level"=0 "msg"="updated condition" "name"="test-name" "namespace"="test-namespace" "message"="additionalAuthorizeParameters parameter names are allowed" "reason"="Success" "status"="True" "type"="AdditionalAuthorizeParametersValid"`, `oidc-upstream-observer "msg"="found failing condition" "error"="OIDCIdentityProvider has a failing condition" "message"="failed to parse issuer URL: parse \"%invalid-url-that-is-really-really-long-nanananananananannanananan-batman-nanananananananananananananana-batman-lalalalalalalalalal-batman-weeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeee\": invalid URL escape \"%in\"" "name"="test-name" "namespace"="test-namespace" "reason"="Unreachable" "type"="OIDCDiscoverySucceeded"`, @@ -424,7 +424,7 @@ func TestOIDCUpstreamWatcherControllerSync(t *testing.T) { Conditions: []metav1.Condition{ happyAdditionalAuthorizeParametersValidCondition, { - Type: "ClientCredentialsValid", + Type: "ClientCredentialsSecretValid", Status: "True", LastTransitionTime: now, Reason: "Success", @@ -457,7 +457,7 @@ func TestOIDCUpstreamWatcherControllerSync(t *testing.T) { }}, wantErr: controllerlib.ErrSyntheticRequeue.Error(), wantLogs: []string{ - `oidc-upstream-observer "level"=0 "msg"="updated condition" "name"="test-name" "namespace"="test-namespace" "message"="loaded client credentials" "reason"="Success" "status"="True" "type"="ClientCredentialsValid"`, + `oidc-upstream-observer "level"=0 "msg"="updated condition" "name"="test-name" "namespace"="test-namespace" "message"="loaded client credentials" "reason"="Success" "status"="True" "type"="ClientCredentialsSecretValid"`, `oidc-upstream-observer "level"=0 "msg"="updated condition" "name"="test-name" "namespace"="test-namespace" "message"="issuer URL '` + strings.Replace(testIssuerURL, "https", "http", 1) + `' must have \"https\" scheme, not \"http\"" "reason"="Unreachable" "status"="False" "type"="OIDCDiscoverySucceeded"`, `oidc-upstream-observer "level"=0 "msg"="updated condition" "name"="test-name" "namespace"="test-namespace" "message"="additionalAuthorizeParameters parameter names are allowed" "reason"="Success" "status"="True" "type"="AdditionalAuthorizeParametersValid"`, `oidc-upstream-observer "msg"="found failing condition" "error"="OIDCIdentityProvider has a failing condition" "message"="issuer URL '` + strings.Replace(testIssuerURL, "https", "http", 1) + `' must have \"https\" scheme, not \"http\"" "name"="test-name" "namespace"="test-namespace" "reason"="Unreachable" "type"="OIDCDiscoverySucceeded"`, @@ -470,7 +470,7 @@ func TestOIDCUpstreamWatcherControllerSync(t *testing.T) { Conditions: []metav1.Condition{ happyAdditionalAuthorizeParametersValidCondition, { - Type: "ClientCredentialsValid", + Type: "ClientCredentialsSecretValid", Status: "True", LastTransitionTime: now, Reason: "Success", @@ -503,7 +503,7 @@ func TestOIDCUpstreamWatcherControllerSync(t *testing.T) { }}, wantErr: controllerlib.ErrSyntheticRequeue.Error(), wantLogs: []string{ - `oidc-upstream-observer "level"=0 "msg"="updated condition" "name"="test-name" "namespace"="test-namespace" "message"="loaded client credentials" "reason"="Success" "status"="True" "type"="ClientCredentialsValid"`, + `oidc-upstream-observer "level"=0 "msg"="updated condition" "name"="test-name" "namespace"="test-namespace" "message"="loaded client credentials" "reason"="Success" "status"="True" "type"="ClientCredentialsSecretValid"`, `oidc-upstream-observer "level"=0 "msg"="updated condition" "name"="test-name" "namespace"="test-namespace" "message"="issuer URL '` + testIssuerURL + "?sub=foo" + `' cannot contain query or fragment component" "reason"="Unreachable" "status"="False" "type"="OIDCDiscoverySucceeded"`, `oidc-upstream-observer "level"=0 "msg"="updated condition" "name"="test-name" "namespace"="test-namespace" "message"="additionalAuthorizeParameters parameter names are allowed" "reason"="Success" "status"="True" "type"="AdditionalAuthorizeParametersValid"`, `oidc-upstream-observer "msg"="found failing condition" "error"="OIDCIdentityProvider has a failing condition" "message"="issuer URL '` + testIssuerURL + "?sub=foo" + `' cannot contain query or fragment component" "name"="test-name" "namespace"="test-namespace" "reason"="Unreachable" "type"="OIDCDiscoverySucceeded"`, @@ -516,7 +516,7 @@ func TestOIDCUpstreamWatcherControllerSync(t *testing.T) { Conditions: []metav1.Condition{ happyAdditionalAuthorizeParametersValidCondition, { - Type: "ClientCredentialsValid", + Type: "ClientCredentialsSecretValid", Status: "True", LastTransitionTime: now, Reason: "Success", @@ -549,7 +549,7 @@ func TestOIDCUpstreamWatcherControllerSync(t *testing.T) { }}, wantErr: controllerlib.ErrSyntheticRequeue.Error(), wantLogs: []string{ - `oidc-upstream-observer "level"=0 "msg"="updated condition" "name"="test-name" "namespace"="test-namespace" "message"="loaded client credentials" "reason"="Success" "status"="True" "type"="ClientCredentialsValid"`, + `oidc-upstream-observer "level"=0 "msg"="updated condition" "name"="test-name" "namespace"="test-namespace" "message"="loaded client credentials" "reason"="Success" "status"="True" "type"="ClientCredentialsSecretValid"`, `oidc-upstream-observer "level"=0 "msg"="updated condition" "name"="test-name" "namespace"="test-namespace" "message"="issuer URL '` + testIssuerURL + "#fragment" + `' cannot contain query or fragment component" "reason"="Unreachable" "status"="False" "type"="OIDCDiscoverySucceeded"`, `oidc-upstream-observer "level"=0 "msg"="updated condition" "name"="test-name" "namespace"="test-namespace" "message"="additionalAuthorizeParameters parameter names are allowed" "reason"="Success" "status"="True" "type"="AdditionalAuthorizeParametersValid"`, `oidc-upstream-observer "msg"="found failing condition" "error"="OIDCIdentityProvider has a failing condition" "message"="issuer URL '` + testIssuerURL + "#fragment" + `' cannot contain query or fragment component" "name"="test-name" "namespace"="test-namespace" "reason"="Unreachable" "type"="OIDCDiscoverySucceeded"`, @@ -562,7 +562,7 @@ func TestOIDCUpstreamWatcherControllerSync(t *testing.T) { Conditions: []metav1.Condition{ happyAdditionalAuthorizeParametersValidCondition, { - Type: "ClientCredentialsValid", + Type: "ClientCredentialsSecretValid", Status: "True", LastTransitionTime: now, Reason: "Success", @@ -597,7 +597,7 @@ func TestOIDCUpstreamWatcherControllerSync(t *testing.T) { wantErr: controllerlib.ErrSyntheticRequeue.Error(), wantLogs: []string{ `oidc-upstream-observer "msg"="failed to perform OIDC discovery" "error"="Get \"` + testIssuerURL + `/valid-url-that-is-really-really-long-nanananananananannanananan-batman-nanananananananananananananana-batman-lalalalalalalalalal-batman-weeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeee/.well-known/openid-configuration\": tls: failed to verify certificate: x509: certificate signed by unknown authority" "issuer"="` + testIssuerURL + `/valid-url-that-is-really-really-long-nanananananananannanananan-batman-nanananananananananananananana-batman-lalalalalalalalalal-batman-weeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeee" "name"="test-name" "namespace"="test-namespace"`, - `oidc-upstream-observer "level"=0 "msg"="updated condition" "name"="test-name" "namespace"="test-namespace" "message"="loaded client credentials" "reason"="Success" "status"="True" "type"="ClientCredentialsValid"`, + `oidc-upstream-observer "level"=0 "msg"="updated condition" "name"="test-name" "namespace"="test-namespace" "message"="loaded client credentials" "reason"="Success" "status"="True" "type"="ClientCredentialsSecretValid"`, `oidc-upstream-observer "level"=0 "msg"="updated condition" "name"="test-name" "namespace"="test-namespace" "message"="failed to perform OIDC discovery against \"` + testIssuerURL + `/valid-url-that-is-really-really-long-nanananananananannanananan-batman-nanananananananananananananana-batman-lalalalalalalalalal-batman-weeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeee\":\nGet \"` + testIssuerURL + `/valid-url-that-is-really-really-long-nanananananananannanananan-batman-nanananananananananananananana-batman-lalalalalalalalalal-batman-weeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeee/.well-known/openid-configuration\": tls: failed to verify certificate: x509: certificate signed by unknown authority" "reason"="Unreachable" "status"="False" "type"="OIDCDiscoverySucceeded"`, `oidc-upstream-observer "level"=0 "msg"="updated condition" "name"="test-name" "namespace"="test-namespace" "message"="additionalAuthorizeParameters parameter names are allowed" "reason"="Success" "status"="True" "type"="AdditionalAuthorizeParametersValid"`, `oidc-upstream-observer "msg"="found failing condition" "error"="OIDCIdentityProvider has a failing condition" "message"="failed to perform OIDC discovery against \"` + testIssuerURL + `/valid-url-that-is-really-really-long-nanananananananannanananan-batman-nanananananananananananananana-batman-lalalalalalalalalal-batman-weeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeee\":\nGet \"` + testIssuerURL + `/valid-url-that-is-really-really-long-nanananananananannanananan-batman-nanananananananananananananana-batman-lalalalalalalalalal-batman-weeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeee/.well-known/openid-configuration\": tls: failed to verify certificate: x509: certificate signed by unknown authority" "name"="test-name" "namespace"="test-namespace" "reason"="Unreachable" "type"="OIDCDiscoverySucceeded"`, @@ -610,7 +610,7 @@ func TestOIDCUpstreamWatcherControllerSync(t *testing.T) { Conditions: []metav1.Condition{ happyAdditionalAuthorizeParametersValidCondition, { - Type: "ClientCredentialsValid", + Type: "ClientCredentialsSecretValid", Status: "True", LastTransitionTime: now, Reason: "Success", @@ -645,7 +645,7 @@ Get "` + testIssuerURL + `/valid-url-that-is-really-really-long-nananananananana }}, wantErr: controllerlib.ErrSyntheticRequeue.Error(), wantLogs: []string{ - `oidc-upstream-observer "level"=0 "msg"="updated condition" "name"="test-name" "namespace"="test-namespace" "message"="loaded client credentials" "reason"="Success" "status"="True" "type"="ClientCredentialsValid"`, + `oidc-upstream-observer "level"=0 "msg"="updated condition" "name"="test-name" "namespace"="test-namespace" "message"="loaded client credentials" "reason"="Success" "status"="True" "type"="ClientCredentialsSecretValid"`, `oidc-upstream-observer "level"=0 "msg"="updated condition" "name"="test-name" "namespace"="test-namespace" "message"="failed to parse authorization endpoint URL: parse \"%\": invalid URL escape \"%\"" "reason"="InvalidResponse" "status"="False" "type"="OIDCDiscoverySucceeded"`, `oidc-upstream-observer "level"=0 "msg"="updated condition" "name"="test-name" "namespace"="test-namespace" "message"="additionalAuthorizeParameters parameter names are allowed" "reason"="Success" "status"="True" "type"="AdditionalAuthorizeParametersValid"`, `oidc-upstream-observer "msg"="found failing condition" "error"="OIDCIdentityProvider has a failing condition" "message"="failed to parse authorization endpoint URL: parse \"%\": invalid URL escape \"%\"" "name"="test-name" "namespace"="test-namespace" "reason"="InvalidResponse" "type"="OIDCDiscoverySucceeded"`, @@ -658,7 +658,7 @@ Get "` + testIssuerURL + `/valid-url-that-is-really-really-long-nananananananana Conditions: []metav1.Condition{ happyAdditionalAuthorizeParametersValidCondition, { - Type: "ClientCredentialsValid", + Type: "ClientCredentialsSecretValid", Status: "True", LastTransitionTime: now, Reason: "Success", @@ -692,7 +692,7 @@ Get "` + testIssuerURL + `/valid-url-that-is-really-really-long-nananananananana }}, wantErr: controllerlib.ErrSyntheticRequeue.Error(), wantLogs: []string{ - `oidc-upstream-observer "level"=0 "msg"="updated condition" "name"="test-name" "namespace"="test-namespace" "message"="loaded client credentials" "reason"="Success" "status"="True" "type"="ClientCredentialsValid"`, + `oidc-upstream-observer "level"=0 "msg"="updated condition" "name"="test-name" "namespace"="test-namespace" "message"="loaded client credentials" "reason"="Success" "status"="True" "type"="ClientCredentialsSecretValid"`, `oidc-upstream-observer "level"=0 "msg"="updated condition" "name"="test-name" "namespace"="test-namespace" "message"="failed to parse revocation endpoint URL: parse \"%\": invalid URL escape \"%\"" "reason"="InvalidResponse" "status"="False" "type"="OIDCDiscoverySucceeded"`, `oidc-upstream-observer "level"=0 "msg"="updated condition" "name"="test-name" "namespace"="test-namespace" "message"="additionalAuthorizeParameters parameter names are allowed" "reason"="Success" "status"="True" "type"="AdditionalAuthorizeParametersValid"`, `oidc-upstream-observer "msg"="found failing condition" "error"="OIDCIdentityProvider has a failing condition" "message"="failed to parse revocation endpoint URL: parse \"%\": invalid URL escape \"%\"" "name"="test-name" "namespace"="test-namespace" "reason"="InvalidResponse" "type"="OIDCDiscoverySucceeded"`, @@ -705,7 +705,7 @@ Get "` + testIssuerURL + `/valid-url-that-is-really-really-long-nananananananana Conditions: []metav1.Condition{ happyAdditionalAuthorizeParametersValidCondition, { - Type: "ClientCredentialsValid", + Type: "ClientCredentialsSecretValid", Status: "True", LastTransitionTime: now, Reason: "Success", @@ -739,7 +739,7 @@ Get "` + testIssuerURL + `/valid-url-that-is-really-really-long-nananananananana }}, wantErr: controllerlib.ErrSyntheticRequeue.Error(), wantLogs: []string{ - `oidc-upstream-observer "level"=0 "msg"="updated condition" "name"="test-name" "namespace"="test-namespace" "message"="loaded client credentials" "reason"="Success" "status"="True" "type"="ClientCredentialsValid"`, + `oidc-upstream-observer "level"=0 "msg"="updated condition" "name"="test-name" "namespace"="test-namespace" "message"="loaded client credentials" "reason"="Success" "status"="True" "type"="ClientCredentialsSecretValid"`, `oidc-upstream-observer "level"=0 "msg"="updated condition" "name"="test-name" "namespace"="test-namespace" "message"="authorization endpoint URL 'http://example.com/authorize' must have \"https\" scheme, not \"http\"" "reason"="InvalidResponse" "status"="False" "type"="OIDCDiscoverySucceeded"`, `oidc-upstream-observer "level"=0 "msg"="updated condition" "name"="test-name" "namespace"="test-namespace" "message"="additionalAuthorizeParameters parameter names are allowed" "reason"="Success" "status"="True" "type"="AdditionalAuthorizeParametersValid"`, `oidc-upstream-observer "msg"="found failing condition" "error"="OIDCIdentityProvider has a failing condition" "message"="authorization endpoint URL 'http://example.com/authorize' must have \"https\" scheme, not \"http\"" "name"="test-name" "namespace"="test-namespace" "reason"="InvalidResponse" "type"="OIDCDiscoverySucceeded"`, @@ -752,7 +752,7 @@ Get "` + testIssuerURL + `/valid-url-that-is-really-really-long-nananananananana Conditions: []metav1.Condition{ happyAdditionalAuthorizeParametersValidCondition, { - Type: "ClientCredentialsValid", + Type: "ClientCredentialsSecretValid", Status: "True", LastTransitionTime: now, Reason: "Success", @@ -786,7 +786,7 @@ Get "` + testIssuerURL + `/valid-url-that-is-really-really-long-nananananananana }}, wantErr: controllerlib.ErrSyntheticRequeue.Error(), wantLogs: []string{ - `oidc-upstream-observer "level"=0 "msg"="updated condition" "name"="test-name" "namespace"="test-namespace" "message"="loaded client credentials" "reason"="Success" "status"="True" "type"="ClientCredentialsValid"`, + `oidc-upstream-observer "level"=0 "msg"="updated condition" "name"="test-name" "namespace"="test-namespace" "message"="loaded client credentials" "reason"="Success" "status"="True" "type"="ClientCredentialsSecretValid"`, `oidc-upstream-observer "level"=0 "msg"="updated condition" "name"="test-name" "namespace"="test-namespace" "message"="revocation endpoint URL 'http://example.com/revoke' must have \"https\" scheme, not \"http\"" "reason"="InvalidResponse" "status"="False" "type"="OIDCDiscoverySucceeded"`, `oidc-upstream-observer "level"=0 "msg"="updated condition" "name"="test-name" "namespace"="test-namespace" "message"="additionalAuthorizeParameters parameter names are allowed" "reason"="Success" "status"="True" "type"="AdditionalAuthorizeParametersValid"`, `oidc-upstream-observer "msg"="found failing condition" "error"="OIDCIdentityProvider has a failing condition" "message"="revocation endpoint URL 'http://example.com/revoke' must have \"https\" scheme, not \"http\"" "name"="test-name" "namespace"="test-namespace" "reason"="InvalidResponse" "type"="OIDCDiscoverySucceeded"`, @@ -799,7 +799,7 @@ Get "` + testIssuerURL + `/valid-url-that-is-really-really-long-nananananananana Conditions: []metav1.Condition{ happyAdditionalAuthorizeParametersValidCondition, { - Type: "ClientCredentialsValid", + Type: "ClientCredentialsSecretValid", Status: "True", LastTransitionTime: now, Reason: "Success", @@ -833,7 +833,7 @@ Get "` + testIssuerURL + `/valid-url-that-is-really-really-long-nananananananana }}, wantErr: controllerlib.ErrSyntheticRequeue.Error(), wantLogs: []string{ - `oidc-upstream-observer "level"=0 "msg"="updated condition" "name"="test-name" "namespace"="test-namespace" "message"="loaded client credentials" "reason"="Success" "status"="True" "type"="ClientCredentialsValid"`, + `oidc-upstream-observer "level"=0 "msg"="updated condition" "name"="test-name" "namespace"="test-namespace" "message"="loaded client credentials" "reason"="Success" "status"="True" "type"="ClientCredentialsSecretValid"`, `oidc-upstream-observer "level"=0 "msg"="updated condition" "name"="test-name" "namespace"="test-namespace" "message"="token endpoint URL 'http://example.com/token' must have \"https\" scheme, not \"http\"" "reason"="InvalidResponse" "status"="False" "type"="OIDCDiscoverySucceeded"`, `oidc-upstream-observer "level"=0 "msg"="updated condition" "name"="test-name" "namespace"="test-namespace" "message"="additionalAuthorizeParameters parameter names are allowed" "reason"="Success" "status"="True" "type"="AdditionalAuthorizeParametersValid"`, `oidc-upstream-observer "msg"="found failing condition" "error"="OIDCIdentityProvider has a failing condition" "message"="token endpoint URL 'http://example.com/token' must have \"https\" scheme, not \"http\"" "name"="test-name" "namespace"="test-namespace" "reason"="InvalidResponse" "type"="OIDCDiscoverySucceeded"`, @@ -846,7 +846,7 @@ Get "` + testIssuerURL + `/valid-url-that-is-really-really-long-nananananananana Conditions: []metav1.Condition{ happyAdditionalAuthorizeParametersValidCondition, { - Type: "ClientCredentialsValid", + Type: "ClientCredentialsSecretValid", Status: "True", LastTransitionTime: now, Reason: "Success", @@ -880,7 +880,7 @@ Get "` + testIssuerURL + `/valid-url-that-is-really-really-long-nananananananana }}, wantErr: controllerlib.ErrSyntheticRequeue.Error(), wantLogs: []string{ - `oidc-upstream-observer "level"=0 "msg"="updated condition" "name"="test-name" "namespace"="test-namespace" "message"="loaded client credentials" "reason"="Success" "status"="True" "type"="ClientCredentialsValid"`, + `oidc-upstream-observer "level"=0 "msg"="updated condition" "name"="test-name" "namespace"="test-namespace" "message"="loaded client credentials" "reason"="Success" "status"="True" "type"="ClientCredentialsSecretValid"`, `oidc-upstream-observer "level"=0 "msg"="updated condition" "name"="test-name" "namespace"="test-namespace" "message"="token endpoint URL '' must have \"https\" scheme, not \"\"" "reason"="InvalidResponse" "status"="False" "type"="OIDCDiscoverySucceeded"`, `oidc-upstream-observer "level"=0 "msg"="updated condition" "name"="test-name" "namespace"="test-namespace" "message"="additionalAuthorizeParameters parameter names are allowed" "reason"="Success" "status"="True" "type"="AdditionalAuthorizeParametersValid"`, `oidc-upstream-observer "msg"="found failing condition" "error"="OIDCIdentityProvider has a failing condition" "message"="token endpoint URL '' must have \"https\" scheme, not \"\"" "name"="test-name" "namespace"="test-namespace" "reason"="InvalidResponse" "type"="OIDCDiscoverySucceeded"`, @@ -893,7 +893,7 @@ Get "` + testIssuerURL + `/valid-url-that-is-really-really-long-nananananananana Conditions: []metav1.Condition{ happyAdditionalAuthorizeParametersValidCondition, { - Type: "ClientCredentialsValid", + Type: "ClientCredentialsSecretValid", Status: "True", LastTransitionTime: now, Reason: "Success", @@ -927,7 +927,7 @@ Get "` + testIssuerURL + `/valid-url-that-is-really-really-long-nananananananana }}, wantErr: controllerlib.ErrSyntheticRequeue.Error(), wantLogs: []string{ - `oidc-upstream-observer "level"=0 "msg"="updated condition" "name"="test-name" "namespace"="test-namespace" "message"="loaded client credentials" "reason"="Success" "status"="True" "type"="ClientCredentialsValid"`, + `oidc-upstream-observer "level"=0 "msg"="updated condition" "name"="test-name" "namespace"="test-namespace" "message"="loaded client credentials" "reason"="Success" "status"="True" "type"="ClientCredentialsSecretValid"`, `oidc-upstream-observer "level"=0 "msg"="updated condition" "name"="test-name" "namespace"="test-namespace" "message"="authorization endpoint URL '' must have \"https\" scheme, not \"\"" "reason"="InvalidResponse" "status"="False" "type"="OIDCDiscoverySucceeded"`, `oidc-upstream-observer "level"=0 "msg"="updated condition" "name"="test-name" "namespace"="test-namespace" "message"="additionalAuthorizeParameters parameter names are allowed" "reason"="Success" "status"="True" "type"="AdditionalAuthorizeParametersValid"`, `oidc-upstream-observer "msg"="found failing condition" "error"="OIDCIdentityProvider has a failing condition" "message"="authorization endpoint URL '' must have \"https\" scheme, not \"\"" "name"="test-name" "namespace"="test-namespace" "reason"="InvalidResponse" "type"="OIDCDiscoverySucceeded"`, @@ -940,7 +940,7 @@ Get "` + testIssuerURL + `/valid-url-that-is-really-really-long-nananananananana Conditions: []metav1.Condition{ happyAdditionalAuthorizeParametersValidCondition, { - Type: "ClientCredentialsValid", + Type: "ClientCredentialsSecretValid", Status: "True", LastTransitionTime: now, Reason: "Success", @@ -974,7 +974,7 @@ Get "` + testIssuerURL + `/valid-url-that-is-really-really-long-nananananananana Status: v1alpha1.OIDCIdentityProviderStatus{ Phase: "Error", Conditions: []metav1.Condition{ - {Type: "ClientCredentialsValid", Status: "False", LastTransitionTime: earlier, Reason: "SomeError1", Message: "some previous error 1"}, + {Type: "ClientCredentialsSecretValid", Status: "False", LastTransitionTime: earlier, Reason: "SomeError1", Message: "some previous error 1"}, {Type: "OIDCDiscoverySucceeded", Status: "False", LastTransitionTime: earlier, Reason: "SomeError2", Message: "some previous error 2"}, }, }, @@ -985,7 +985,7 @@ Get "` + testIssuerURL + `/valid-url-that-is-really-really-long-nananananananana Data: testValidSecretData, }}, wantLogs: []string{ - `oidc-upstream-observer "level"=0 "msg"="updated condition" "name"="test-name" "namespace"="test-namespace" "message"="loaded client credentials" "reason"="Success" "status"="True" "type"="ClientCredentialsValid"`, + `oidc-upstream-observer "level"=0 "msg"="updated condition" "name"="test-name" "namespace"="test-namespace" "message"="loaded client credentials" "reason"="Success" "status"="True" "type"="ClientCredentialsSecretValid"`, `oidc-upstream-observer "level"=0 "msg"="updated condition" "name"="test-name" "namespace"="test-namespace" "message"="discovered issuer configuration" "reason"="Success" "status"="True" "type"="OIDCDiscoverySucceeded"`, `oidc-upstream-observer "level"=0 "msg"="updated condition" "name"="test-name" "namespace"="test-namespace" "message"="additionalAuthorizeParameters parameter names are allowed" "reason"="Success" "status"="True" "type"="AdditionalAuthorizeParametersValid"`, }, @@ -1010,7 +1010,7 @@ Get "` + testIssuerURL + `/valid-url-that-is-really-really-long-nananananananana Phase: "Ready", Conditions: []metav1.Condition{ happyAdditionalAuthorizeParametersValidCondition, - {Type: "ClientCredentialsValid", Status: "True", LastTransitionTime: now, Reason: "Success", Message: "loaded client credentials"}, + {Type: "ClientCredentialsSecretValid", Status: "True", LastTransitionTime: now, Reason: "Success", Message: "loaded client credentials"}, {Type: "OIDCDiscoverySucceeded", Status: "True", LastTransitionTime: now, Reason: "Success", Message: "discovered issuer configuration"}, }, }, @@ -1030,7 +1030,7 @@ Get "` + testIssuerURL + `/valid-url-that-is-really-really-long-nananananananana Phase: "Ready", Conditions: []metav1.Condition{ happyAdditionalAuthorizeParametersValidConditionEarlier, - {Type: "ClientCredentialsValid", Status: "True", LastTransitionTime: earlier, Reason: "Success", Message: "loaded client credentials"}, + {Type: "ClientCredentialsSecretValid", Status: "True", LastTransitionTime: earlier, Reason: "Success", Message: "loaded client credentials"}, {Type: "OIDCDiscoverySucceeded", Status: "True", LastTransitionTime: earlier, Reason: "Success", Message: "discovered issuer configuration"}, }, }, @@ -1041,7 +1041,7 @@ Get "` + testIssuerURL + `/valid-url-that-is-really-really-long-nananananananana Data: testValidSecretData, }}, wantLogs: []string{ - `oidc-upstream-observer "level"=0 "msg"="updated condition" "name"="test-name" "namespace"="test-namespace" "message"="loaded client credentials" "reason"="Success" "status"="True" "type"="ClientCredentialsValid"`, + `oidc-upstream-observer "level"=0 "msg"="updated condition" "name"="test-name" "namespace"="test-namespace" "message"="loaded client credentials" "reason"="Success" "status"="True" "type"="ClientCredentialsSecretValid"`, `oidc-upstream-observer "level"=0 "msg"="updated condition" "name"="test-name" "namespace"="test-namespace" "message"="discovered issuer configuration" "reason"="Success" "status"="True" "type"="OIDCDiscoverySucceeded"`, `oidc-upstream-observer "level"=0 "msg"="updated condition" "name"="test-name" "namespace"="test-namespace" "message"="additionalAuthorizeParameters parameter names are allowed" "reason"="Success" "status"="True" "type"="AdditionalAuthorizeParametersValid"`, }, @@ -1066,7 +1066,7 @@ Get "` + testIssuerURL + `/valid-url-that-is-really-really-long-nananananananana Phase: "Ready", Conditions: []metav1.Condition{ {Type: "AdditionalAuthorizeParametersValid", Status: "True", LastTransitionTime: earlier, Reason: "Success", Message: "additionalAuthorizeParameters parameter names are allowed", ObservedGeneration: 1234}, - {Type: "ClientCredentialsValid", Status: "True", LastTransitionTime: earlier, Reason: "Success", Message: "loaded client credentials", ObservedGeneration: 1234}, + {Type: "ClientCredentialsSecretValid", Status: "True", LastTransitionTime: earlier, Reason: "Success", Message: "loaded client credentials", ObservedGeneration: 1234}, {Type: "OIDCDiscoverySucceeded", Status: "True", LastTransitionTime: earlier, Reason: "Success", Message: "discovered issuer configuration", ObservedGeneration: 1234}, }, }, @@ -1086,7 +1086,7 @@ Get "` + testIssuerURL + `/valid-url-that-is-really-really-long-nananananananana Phase: "Ready", Conditions: []metav1.Condition{ happyAdditionalAuthorizeParametersValidConditionEarlier, - {Type: "ClientCredentialsValid", Status: "True", LastTransitionTime: earlier, Reason: "Success", Message: "loaded client credentials"}, + {Type: "ClientCredentialsSecretValid", Status: "True", LastTransitionTime: earlier, Reason: "Success", Message: "loaded client credentials"}, {Type: "OIDCDiscoverySucceeded", Status: "True", LastTransitionTime: earlier, Reason: "Success", Message: "discovered issuer configuration"}, }, }, @@ -1097,7 +1097,7 @@ Get "` + testIssuerURL + `/valid-url-that-is-really-really-long-nananananananana Data: testValidSecretData, }}, wantLogs: []string{ - `oidc-upstream-observer "level"=0 "msg"="updated condition" "name"="test-name" "namespace"="test-namespace" "message"="loaded client credentials" "reason"="Success" "status"="True" "type"="ClientCredentialsValid"`, + `oidc-upstream-observer "level"=0 "msg"="updated condition" "name"="test-name" "namespace"="test-namespace" "message"="loaded client credentials" "reason"="Success" "status"="True" "type"="ClientCredentialsSecretValid"`, `oidc-upstream-observer "level"=0 "msg"="updated condition" "name"="test-name" "namespace"="test-namespace" "message"="discovered issuer configuration" "reason"="Success" "status"="True" "type"="OIDCDiscoverySucceeded"`, `oidc-upstream-observer "level"=0 "msg"="updated condition" "name"="test-name" "namespace"="test-namespace" "message"="additionalAuthorizeParameters parameter names are allowed" "reason"="Success" "status"="True" "type"="AdditionalAuthorizeParametersValid"`, }, @@ -1122,7 +1122,7 @@ Get "` + testIssuerURL + `/valid-url-that-is-really-really-long-nananananananana Phase: "Ready", Conditions: []metav1.Condition{ {Type: "AdditionalAuthorizeParametersValid", Status: "True", LastTransitionTime: earlier, Reason: "Success", Message: "additionalAuthorizeParameters parameter names are allowed", ObservedGeneration: 1234}, - {Type: "ClientCredentialsValid", Status: "True", LastTransitionTime: earlier, Reason: "Success", Message: "loaded client credentials", ObservedGeneration: 1234}, + {Type: "ClientCredentialsSecretValid", Status: "True", LastTransitionTime: earlier, Reason: "Success", Message: "loaded client credentials", ObservedGeneration: 1234}, {Type: "OIDCDiscoverySucceeded", Status: "True", LastTransitionTime: earlier, Reason: "Success", Message: "discovered issuer configuration", ObservedGeneration: 1234}, }, }, @@ -1145,7 +1145,7 @@ Get "` + testIssuerURL + `/valid-url-that-is-really-really-long-nananananananana Phase: "Ready", Conditions: []metav1.Condition{ happyAdditionalAuthorizeParametersValidConditionEarlier, - {Type: "ClientCredentialsValid", Status: "True", LastTransitionTime: earlier, Reason: "Success", Message: "loaded client credentials"}, + {Type: "ClientCredentialsSecretValid", Status: "True", LastTransitionTime: earlier, Reason: "Success", Message: "loaded client credentials"}, {Type: "OIDCDiscoverySucceeded", Status: "True", LastTransitionTime: earlier, Reason: "Success", Message: "discovered issuer configuration"}, }, }, @@ -1156,7 +1156,7 @@ Get "` + testIssuerURL + `/valid-url-that-is-really-really-long-nananananananana Data: testValidSecretData, }}, wantLogs: []string{ - `oidc-upstream-observer "level"=0 "msg"="updated condition" "name"="test-name" "namespace"="test-namespace" "message"="loaded client credentials" "reason"="Success" "status"="True" "type"="ClientCredentialsValid"`, + `oidc-upstream-observer "level"=0 "msg"="updated condition" "name"="test-name" "namespace"="test-namespace" "message"="loaded client credentials" "reason"="Success" "status"="True" "type"="ClientCredentialsSecretValid"`, `oidc-upstream-observer "level"=0 "msg"="updated condition" "name"="test-name" "namespace"="test-namespace" "message"="discovered issuer configuration" "reason"="Success" "status"="True" "type"="OIDCDiscoverySucceeded"`, `oidc-upstream-observer "level"=0 "msg"="updated condition" "name"="test-name" "namespace"="test-namespace" "message"="additionalAuthorizeParameters parameter names are allowed" "reason"="Success" "status"="True" "type"="AdditionalAuthorizeParametersValid"`, }, @@ -1181,7 +1181,7 @@ Get "` + testIssuerURL + `/valid-url-that-is-really-really-long-nananananananana Phase: "Ready", Conditions: []metav1.Condition{ {Type: "AdditionalAuthorizeParametersValid", Status: "True", LastTransitionTime: earlier, Reason: "Success", Message: "additionalAuthorizeParameters parameter names are allowed", ObservedGeneration: 1234}, - {Type: "ClientCredentialsValid", Status: "True", LastTransitionTime: earlier, Reason: "Success", Message: "loaded client credentials", ObservedGeneration: 1234}, + {Type: "ClientCredentialsSecretValid", Status: "True", LastTransitionTime: earlier, Reason: "Success", Message: "loaded client credentials", ObservedGeneration: 1234}, {Type: "OIDCDiscoverySucceeded", Status: "True", LastTransitionTime: earlier, Reason: "Success", Message: "discovered issuer configuration", ObservedGeneration: 1234}, }, }, @@ -1212,7 +1212,7 @@ Get "` + testIssuerURL + `/valid-url-that-is-really-really-long-nananananananana Phase: "Ready", Conditions: []metav1.Condition{ happyAdditionalAuthorizeParametersValidConditionEarlier, - {Type: "ClientCredentialsValid", Status: "True", LastTransitionTime: earlier, Reason: "Success", Message: "loaded client credentials"}, + {Type: "ClientCredentialsSecretValid", Status: "True", LastTransitionTime: earlier, Reason: "Success", Message: "loaded client credentials"}, {Type: "OIDCDiscoverySucceeded", Status: "True", LastTransitionTime: earlier, Reason: "Success", Message: "discovered issuer configuration"}, }, }, @@ -1223,7 +1223,7 @@ Get "` + testIssuerURL + `/valid-url-that-is-really-really-long-nananananananana Data: testValidSecretData, }}, wantLogs: []string{ - `oidc-upstream-observer "level"=0 "msg"="updated condition" "name"="test-name" "namespace"="test-namespace" "message"="loaded client credentials" "reason"="Success" "status"="True" "type"="ClientCredentialsValid"`, + `oidc-upstream-observer "level"=0 "msg"="updated condition" "name"="test-name" "namespace"="test-namespace" "message"="loaded client credentials" "reason"="Success" "status"="True" "type"="ClientCredentialsSecretValid"`, `oidc-upstream-observer "level"=0 "msg"="updated condition" "name"="test-name" "namespace"="test-namespace" "message"="discovered issuer configuration" "reason"="Success" "status"="True" "type"="OIDCDiscoverySucceeded"`, `oidc-upstream-observer "level"=0 "msg"="updated condition" "name"="test-name" "namespace"="test-namespace" "message"="additionalAuthorizeParameters parameter names are allowed" "reason"="Success" "status"="True" "type"="AdditionalAuthorizeParametersValid"`, }, @@ -1250,7 +1250,7 @@ Get "` + testIssuerURL + `/valid-url-that-is-really-really-long-nananananananana Phase: "Ready", Conditions: []metav1.Condition{ {Type: "AdditionalAuthorizeParametersValid", Status: "True", LastTransitionTime: earlier, Reason: "Success", Message: "additionalAuthorizeParameters parameter names are allowed", ObservedGeneration: 1234}, - {Type: "ClientCredentialsValid", Status: "True", LastTransitionTime: earlier, Reason: "Success", Message: "loaded client credentials", ObservedGeneration: 1234}, + {Type: "ClientCredentialsSecretValid", Status: "True", LastTransitionTime: earlier, Reason: "Success", Message: "loaded client credentials", ObservedGeneration: 1234}, {Type: "OIDCDiscoverySucceeded", Status: "True", LastTransitionTime: earlier, Reason: "Success", Message: "discovered issuer configuration", ObservedGeneration: 1234}, }, }, @@ -1287,7 +1287,7 @@ Get "` + testIssuerURL + `/valid-url-that-is-really-really-long-nananananananana }}, wantErr: controllerlib.ErrSyntheticRequeue.Error(), wantLogs: []string{ - `oidc-upstream-observer "level"=0 "msg"="updated condition" "name"="test-name" "namespace"="test-namespace" "message"="loaded client credentials" "reason"="Success" "status"="True" "type"="ClientCredentialsValid"`, + `oidc-upstream-observer "level"=0 "msg"="updated condition" "name"="test-name" "namespace"="test-namespace" "message"="loaded client credentials" "reason"="Success" "status"="True" "type"="ClientCredentialsSecretValid"`, `oidc-upstream-observer "level"=0 "msg"="updated condition" "name"="test-name" "namespace"="test-namespace" "message"="discovered issuer configuration" "reason"="Success" "status"="True" "type"="OIDCDiscoverySucceeded"`, `oidc-upstream-observer "level"=0 "msg"="updated condition" "name"="test-name" "namespace"="test-namespace" "message"="the following additionalAuthorizeParameters are not allowed: response_type,scope,client_id,state,nonce,code_challenge,code_challenge_method,redirect_uri,hd" "reason"="DisallowedParameterName" "status"="False" "type"="AdditionalAuthorizeParametersValid"`, `oidc-upstream-observer "msg"="found failing condition" "error"="OIDCIdentityProvider has a failing condition" "message"="the following additionalAuthorizeParameters are not allowed: response_type,scope,client_id,state,nonce,code_challenge,code_challenge_method,redirect_uri,hd" "name"="test-name" "namespace"="test-namespace" "reason"="DisallowedParameterName" "type"="AdditionalAuthorizeParametersValid"`, @@ -1301,7 +1301,7 @@ Get "` + testIssuerURL + `/valid-url-that-is-really-really-long-nananananananana {Type: "AdditionalAuthorizeParametersValid", Status: "False", LastTransitionTime: now, Reason: "DisallowedParameterName", Message: "the following additionalAuthorizeParameters are not allowed: " + "response_type,scope,client_id,state,nonce,code_challenge,code_challenge_method,redirect_uri,hd", ObservedGeneration: 1234}, - {Type: "ClientCredentialsValid", Status: "True", LastTransitionTime: now, Reason: "Success", Message: "loaded client credentials", ObservedGeneration: 1234}, + {Type: "ClientCredentialsSecretValid", Status: "True", LastTransitionTime: now, Reason: "Success", Message: "loaded client credentials", ObservedGeneration: 1234}, {Type: "OIDCDiscoverySucceeded", Status: "True", LastTransitionTime: now, Reason: "Success", Message: "discovered issuer configuration", ObservedGeneration: 1234}, }, }, @@ -1325,7 +1325,7 @@ Get "` + testIssuerURL + `/valid-url-that-is-really-really-long-nananananananana wantErr: controllerlib.ErrSyntheticRequeue.Error(), wantLogs: []string{ `oidc-upstream-observer "msg"="failed to perform OIDC discovery" "error"="oidc: issuer did not match the issuer returned by provider, expected \"` + testIssuerURL + `/ends-with-slash\" got \"` + testIssuerURL + `/ends-with-slash/\"" "issuer"="` + testIssuerURL + `/ends-with-slash" "name"="test-name" "namespace"="test-namespace"`, - `oidc-upstream-observer "level"=0 "msg"="updated condition" "name"="test-name" "namespace"="test-namespace" "message"="loaded client credentials" "reason"="Success" "status"="True" "type"="ClientCredentialsValid"`, + `oidc-upstream-observer "level"=0 "msg"="updated condition" "name"="test-name" "namespace"="test-namespace" "message"="loaded client credentials" "reason"="Success" "status"="True" "type"="ClientCredentialsSecretValid"`, `oidc-upstream-observer "level"=0 "msg"="updated condition" "name"="test-name" "namespace"="test-namespace" "message"="failed to perform OIDC discovery against \"` + testIssuerURL + `/ends-with-slash\":\noidc: issuer did not match the issuer returned by provider, expected \"` + testIssuerURL + `/ends-with-slash\" got \"` + testIssuerURL + `/ends-with-slash/\"" "reason"="Unreachable" "status"="False" "type"="OIDCDiscoverySucceeded"`, `oidc-upstream-observer "level"=0 "msg"="updated condition" "name"="test-name" "namespace"="test-namespace" "message"="additionalAuthorizeParameters parameter names are allowed" "reason"="Success" "status"="True" "type"="AdditionalAuthorizeParametersValid"`, `oidc-upstream-observer "msg"="found failing condition" "error"="OIDCIdentityProvider has a failing condition" "message"="failed to perform OIDC discovery against \"` + testIssuerURL + `/ends-with-slash\":\noidc: issuer did not match the issuer returned by provider, expected \"` + testIssuerURL + `/ends-with-slash\" got \"` + testIssuerURL + `/ends-with-slash/\"" "name"="test-name" "namespace"="test-namespace" "reason"="Unreachable" "type"="OIDCDiscoverySucceeded"`, @@ -1338,7 +1338,7 @@ Get "` + testIssuerURL + `/valid-url-that-is-really-really-long-nananananananana Conditions: []metav1.Condition{ happyAdditionalAuthorizeParametersValidCondition, { - Type: "ClientCredentialsValid", + Type: "ClientCredentialsSecretValid", Status: "True", LastTransitionTime: now, Reason: "Success", @@ -1374,7 +1374,7 @@ oidc: issuer did not match the issuer returned by provider, expected "` + testIs wantErr: controllerlib.ErrSyntheticRequeue.Error(), wantLogs: []string{ `oidc-upstream-observer "msg"="failed to perform OIDC discovery" "error"="oidc: issuer did not match the issuer returned by provider, expected \"` + testIssuerURL + `/\" got \"` + testIssuerURL + `\"" "issuer"="` + testIssuerURL + `/" "name"="test-name" "namespace"="test-namespace"`, - `oidc-upstream-observer "level"=0 "msg"="updated condition" "name"="test-name" "namespace"="test-namespace" "message"="loaded client credentials" "reason"="Success" "status"="True" "type"="ClientCredentialsValid"`, + `oidc-upstream-observer "level"=0 "msg"="updated condition" "name"="test-name" "namespace"="test-namespace" "message"="loaded client credentials" "reason"="Success" "status"="True" "type"="ClientCredentialsSecretValid"`, `oidc-upstream-observer "level"=0 "msg"="updated condition" "name"="test-name" "namespace"="test-namespace" "message"="failed to perform OIDC discovery against \"` + testIssuerURL + `/\":\noidc: issuer did not match the issuer returned by provider, expected \"` + testIssuerURL + `/\" got \"` + testIssuerURL + `\"" "reason"="Unreachable" "status"="False" "type"="OIDCDiscoverySucceeded"`, `oidc-upstream-observer "level"=0 "msg"="updated condition" "name"="test-name" "namespace"="test-namespace" "message"="additionalAuthorizeParameters parameter names are allowed" "reason"="Success" "status"="True" "type"="AdditionalAuthorizeParametersValid"`, `oidc-upstream-observer "msg"="found failing condition" "error"="OIDCIdentityProvider has a failing condition" "message"="failed to perform OIDC discovery against \"` + testIssuerURL + `/\":\noidc: issuer did not match the issuer returned by provider, expected \"` + testIssuerURL + `/\" got \"` + testIssuerURL + `\"" "name"="test-name" "namespace"="test-namespace" "reason"="Unreachable" "type"="OIDCDiscoverySucceeded"`, @@ -1387,7 +1387,7 @@ oidc: issuer did not match the issuer returned by provider, expected "` + testIs Conditions: []metav1.Condition{ happyAdditionalAuthorizeParametersValidCondition, { - Type: "ClientCredentialsValid", + Type: "ClientCredentialsSecretValid", Status: "True", LastTransitionTime: now, Reason: "Success", @@ -1448,7 +1448,7 @@ oidc: issuer did not match the issuer returned by provider, expected "` + testIs require.Equal(t, len(tt.wantResultingCache), len(actualIDPList)) for i := range actualIDPList { actualIDP := actualIDPList[i].(*upstreamoidc.ProviderConfig) - require.Equal(t, tt.wantResultingCache[i].GetName(), actualIDP.GetName()) + require.Equal(t, tt.wantResultingCache[i].GetResourceName(), actualIDP.GetResourceName()) require.Equal(t, tt.wantResultingCache[i].GetClientID(), actualIDP.GetClientID()) require.Equal(t, tt.wantResultingCache[i].GetAuthorizationURL().String(), actualIDP.GetAuthorizationURL().String()) require.Equal(t, tt.wantResultingCache[i].GetUsernameClaim(), actualIDP.GetUsernameClaim()) diff --git a/internal/controller/supervisorstorage/garbage_collector.go b/internal/controller/supervisorstorage/garbage_collector.go index bd791f8ba..7cf881eea 100644 --- a/internal/controller/supervisorstorage/garbage_collector.go +++ b/internal/controller/supervisorstorage/garbage_collector.go @@ -246,7 +246,7 @@ func (c *garbageCollectorController) tryRevokeUpstreamOIDCToken(ctx context.Cont // Try to find the provider that was originally used to create the stored session. var foundOIDCIdentityProviderI upstreamprovider.UpstreamOIDCIdentityProviderI for _, p := range c.idpCache.GetOIDCIdentityProviders() { - if p.GetName() == customSessionData.ProviderName && p.GetResourceUID() == customSessionData.ProviderUID { + if p.GetResourceName() == customSessionData.ProviderName && p.GetResourceUID() == customSessionData.ProviderUID { foundOIDCIdentityProviderI = p break } diff --git a/internal/federationdomain/downstreamsession/downstream_session.go b/internal/federationdomain/downstreamsession/downstream_session.go index 6c4c9ec35..54a13672c 100644 --- a/internal/federationdomain/downstreamsession/downstream_session.go +++ b/internal/federationdomain/downstreamsession/downstream_session.go @@ -56,7 +56,7 @@ func NewPinnipedSession( UpstreamUsername: c.UpstreamIdentity.UpstreamUsername, UpstreamGroups: c.UpstreamIdentity.UpstreamGroups, ProviderUID: idp.GetProvider().GetResourceUID(), - ProviderName: idp.GetProvider().GetName(), + ProviderName: idp.GetProvider().GetResourceName(), ProviderType: idp.GetSessionProviderType(), Warnings: c.UpstreamLoginExtras.Warnings, } diff --git a/internal/federationdomain/endpoints/callback/callback_handler.go b/internal/federationdomain/endpoints/callback/callback_handler.go index c812d3143..a7c24dfe7 100644 --- a/internal/federationdomain/endpoints/callback/callback_handler.go +++ b/internal/federationdomain/endpoints/callback/callback_handler.go @@ -48,6 +48,9 @@ func NewHandler( authorizeRequester, err := oauthHelper.NewAuthorizeRequest(r.Context(), reconstitutedAuthRequest) if err != nil { plog.Error("error using state downstream auth params", err, + "identityProviderDisplayName", idp.GetDisplayName(), + "identityProviderResourceName", idp.GetProvider().GetResourceName(), + "supervisorCallbackURL", redirectURI, "fositeErr", oidc.FositeErrorForLog(err)) return httperr.New(http.StatusBadRequest, "error using state downstream auth params") } @@ -59,6 +62,10 @@ func NewHandler( identity, loginExtras, err := idp.LoginFromCallback(r.Context(), authcode(r), state.PKCECode, state.Nonce, redirectURI) if err != nil { + plog.InfoErr("unable to complete login from callback", err, + "identityProviderDisplayName", idp.GetDisplayName(), + "identityProviderResourceName", idp.GetProvider().GetResourceName(), + "supervisorCallbackURL", redirectURI) return err } @@ -69,13 +76,20 @@ func NewHandler( GrantedScopes: authorizeRequester.GetGrantedScopes(), }) if err != nil { + plog.InfoErr("unable to create a Pinniped session", err, + "identityProviderDisplayName", idp.GetDisplayName(), + "identityProviderResourceName", idp.GetProvider().GetResourceName(), + "supervisorCallbackURL", redirectURI) return httperr.Wrap(http.StatusUnprocessableEntity, err.Error(), err) } authorizeResponder, err := oauthHelper.NewAuthorizeResponse(r.Context(), authorizeRequester, session) if err != nil { plog.WarningErr("error while generating and saving authcode", err, - "identityProviderDisplayName", idp.GetDisplayName(), "fositeErr", oidc.FositeErrorForLog(err)) + "identityProviderDisplayName", idp.GetDisplayName(), + "identityProviderResourceName", idp.GetProvider().GetResourceName(), + "supervisorCallbackURL", redirectURI, + "fositeErr", oidc.FositeErrorForLog(err)) return httperr.Wrap(http.StatusInternalServerError, "error while generating and saving authcode", err) } diff --git a/internal/federationdomain/endpoints/token/token_handler.go b/internal/federationdomain/endpoints/token/token_handler.go index f9d31a7f2..0790f323a 100644 --- a/internal/federationdomain/endpoints/token/token_handler.go +++ b/internal/federationdomain/endpoints/token/token_handler.go @@ -233,7 +233,7 @@ func findProviderByNameAndType( idpLister federationdomainproviders.FederationDomainIdentityProvidersListerI, ) (resolvedprovider.FederationDomainResolvedIdentityProvider, error) { for _, p := range idpLister.GetIdentityProviders() { - if p.GetSessionProviderType() == providerType && p.GetProvider().GetName() == providerResourceName { + if p.GetSessionProviderType() == providerType && p.GetProvider().GetResourceName() == providerResourceName { if p.GetProvider().GetResourceUID() != mustHaveResourceUID { return nil, errorsx.WithStack(errUpstreamRefreshError().WithHint( "Provider from upstream session data has changed its resource UID since authentication.")) diff --git a/internal/federationdomain/resolvedprovider/resolvedgithub/resolved_github_provider.go b/internal/federationdomain/resolvedprovider/resolvedgithub/resolved_github_provider.go index f363d5efe..eb846b3e4 100644 --- a/internal/federationdomain/resolvedprovider/resolvedgithub/resolved_github_provider.go +++ b/internal/federationdomain/resolvedprovider/resolvedgithub/resolved_github_provider.go @@ -17,7 +17,6 @@ import ( "go.pinniped.dev/internal/federationdomain/upstreamprovider" "go.pinniped.dev/internal/httputil/httperr" "go.pinniped.dev/internal/idtransform" - "go.pinniped.dev/internal/plog" "go.pinniped.dev/internal/psession" "go.pinniped.dev/pkg/oidcclient/nonce" "go.pinniped.dev/pkg/oidcclient/pkce" @@ -103,7 +102,6 @@ func (p *FederationDomainResolvedGitHubIdentityProvider) LoginFromCallback( ) (*resolvedprovider.Identity, *resolvedprovider.IdentityLoginExtras, error) { accessToken, err := p.Provider.ExchangeAuthcode(ctx, authCode, redirectURI) if err != nil { - plog.WarningErr("failed to exchange authcode using GitHub API", err, "upstreamName", p.Provider.GetName()) return nil, nil, httperr.Wrap(http.StatusBadGateway, "failed to exchange authcode using GitHub API", err, @@ -111,8 +109,14 @@ func (p *FederationDomainResolvedGitHubIdentityProvider) LoginFromCallback( } user, err := p.Provider.GetUser(ctx, accessToken, p.GetDisplayName()) - if err != nil { - plog.WarningErr("failed to get user info from GitHub API", err, "upstreamName", p.Provider.GetName()) + + if errors.As(err, &upstreamprovider.GitHubLoginDeniedError{}) { + // We specifically want errors of type GitHubLoginDeniedError to have a user-displayed message. + // Don't wrap the error since we include it in the sprintf here. + return nil, nil, httperr.Newf(http.StatusForbidden, + "login denied due to configuration on GitHubIdentityProvider with display name %q: %s", + p.GetDisplayName(), err) + } else if err != nil { return nil, nil, httperr.Wrap(http.StatusUnprocessableEntity, "failed to get user info from GitHub API", err, @@ -151,8 +155,7 @@ func (p *FederationDomainResolvedGitHubIdentityProvider) UpstreamRefresh( // Get the user's GitHub identity and groups again using the cached access token. refreshedUserInfo, err := p.Provider.GetUser(ctx, githubSessionData.UpstreamAccessToken, p.GetDisplayName()) if err != nil { - plog.WarningErr("failed to refresh user info from GitHub API", err, "upstreamName", p.Provider.GetName()) - return nil, p.refreshErr(errors.New("failed to refresh user info from GitHub API")) + return nil, p.refreshErr(err) } if refreshedUserInfo.DownstreamSubject != identity.DownstreamSubject { @@ -172,5 +175,5 @@ func (p *FederationDomainResolvedGitHubIdentityProvider) refreshErr(err error) * return resolvedprovider.ErrUpstreamRefreshError(). WithHint("Upstream refresh failed."). WithTrace(err). - WithDebugf("provider name: %q, provider type: %q", p.Provider.GetName(), p.GetSessionProviderType()) + WithDebugf("provider name: %q, provider type: %q", p.Provider.GetResourceName(), p.GetSessionProviderType()) } diff --git a/internal/federationdomain/resolvedprovider/resolvedgithub/resolved_github_provider_test.go b/internal/federationdomain/resolvedprovider/resolvedgithub/resolved_github_provider_test.go index 35f657538..409a22cf1 100644 --- a/internal/federationdomain/resolvedprovider/resolvedgithub/resolved_github_provider_test.go +++ b/internal/federationdomain/resolvedprovider/resolvedgithub/resolved_github_provider_test.go @@ -7,6 +7,7 @@ import ( "context" "errors" "net/http" + "net/http/httptest" "testing" "github.com/ory/fosite" @@ -125,7 +126,9 @@ func TestLoginFromCallback(t *testing.T) { wantGetUserArgs *oidctestutil.GetUserArgs wantIdentity *resolvedprovider.Identity wantExtras *resolvedprovider.IdentityLoginExtras - wantErr string + wantErrMsg string + wantErrResponseMsg string + wantErrStatusCode int }{ { name: "happy path", @@ -176,13 +179,15 @@ func TestLoginFromCallback(t *testing.T) { Authcode: "fake-authcode", RedirectURI: "https://fake-redirect-uri", }, - wantGetUserCall: false, - wantIdentity: nil, - wantExtras: nil, - wantErr: "failed to exchange authcode using GitHub API: fake authcode exchange error", + wantGetUserCall: false, + wantIdentity: nil, + wantExtras: nil, + wantErrMsg: "failed to exchange authcode using GitHub API: fake authcode exchange error", + wantErrResponseMsg: "Bad Gateway: failed to exchange authcode using GitHub API", + wantErrStatusCode: http.StatusBadGateway, }, { - name: "error while getting user info", + name: "generic error while getting user info", provider: oidctestutil.NewTestUpstreamGitHubIdentityProviderBuilder(). WithAccessToken("fake-access-token"). WithGetUserError(errors.New("fake user info error")). @@ -202,9 +207,38 @@ func TestLoginFromCallback(t *testing.T) { AccessToken: "fake-access-token", IDPDisplayName: "fake-display-name", }, - wantIdentity: nil, - wantExtras: nil, - wantErr: "failed to get user info from GitHub API: fake user info error", + wantIdentity: nil, + wantExtras: nil, + wantErrMsg: "failed to get user info from GitHub API: fake user info error", + wantErrResponseMsg: "Unprocessable Entity: failed to get user info from GitHub API", + wantErrStatusCode: http.StatusUnprocessableEntity, + }, + { + name: "loginDenied error while getting user info", + provider: oidctestutil.NewTestUpstreamGitHubIdentityProviderBuilder(). + WithAccessToken("fake-access-token"). + WithGetUserError(upstreamprovider.NewGitHubLoginDeniedError("some login denied error")). + Build(), + idpDisplayName: "fake-display-name", + authcode: "fake-authcode", + redirectURI: "https://fake-redirect-uri", + wantExchangeAuthcodeCall: true, + wantExchangeAuthcodeArgs: &oidctestutil.ExchangeAuthcodeArgs{ + Ctx: uniqueCtx, + Authcode: "fake-authcode", + RedirectURI: "https://fake-redirect-uri", + }, + wantGetUserCall: true, + wantGetUserArgs: &oidctestutil.GetUserArgs{ + Ctx: uniqueCtx, + AccessToken: "fake-access-token", + IDPDisplayName: "fake-display-name", + }, + wantIdentity: nil, + wantExtras: nil, + wantErrMsg: `login denied due to configuration on GitHubIdentityProvider with display name "fake-display-name": some login denied error`, + wantErrResponseMsg: `Forbidden: login denied due to configuration on GitHubIdentityProvider with display name "fake-display-name": some login denied error`, + wantErrStatusCode: http.StatusForbidden, }, } @@ -238,12 +272,17 @@ func TestLoginFromCallback(t *testing.T) { require.Zero(t, test.provider.GetUserCallCount()) } - if test.wantErr == "" { + if test.wantErrResponseMsg == "" { require.NoError(t, err) } else { - errAsResponder, ok := err.(httperr.Responder) - require.True(t, ok) - require.EqualError(t, errAsResponder, test.wantErr) + require.Implements(t, (*httperr.Responder)(nil), err) + errAsResponder := err.(httperr.Responder) + rec := httptest.NewRecorder() + errAsResponder.Respond(rec) + require.Equal(t, test.wantErrStatusCode, rec.Code) + require.Equal(t, test.wantErrResponseMsg+"\n", rec.Body.String()) + + require.EqualError(t, errAsResponder, test.wantErrMsg) } require.Equal(t, test.wantExtras, loginExtras) require.Equal(t, test.wantIdentity, identity) @@ -297,7 +336,7 @@ func TestUpstreamRefresh(t *testing.T) { name: "error while getting user info", provider: oidctestutil.NewTestUpstreamGitHubIdentityProviderBuilder(). WithName("fake-provider-name"). - WithGetUserError(errors.New("any error message")). + WithGetUserError(errors.New("fake github GetUser error message")). Build(), identity: &resolvedprovider.Identity{ UpstreamUsername: "initial-username", @@ -313,7 +352,7 @@ func TestUpstreamRefresh(t *testing.T) { IDPDisplayName: "fake-display-name", }, wantRefreshedIdentity: nil, - wantWrappedErr: "failed to refresh user info from GitHub API", + wantWrappedErr: "fake github GetUser error message", }, { name: "wrong session data type, which should not really happen", diff --git a/internal/federationdomain/resolvedprovider/resolvedldap/resolved_ldap_provider.go b/internal/federationdomain/resolvedprovider/resolvedldap/resolved_ldap_provider.go index 69d7096b8..7d44630e9 100644 --- a/internal/federationdomain/resolvedprovider/resolvedldap/resolved_ldap_provider.go +++ b/internal/federationdomain/resolvedprovider/resolvedldap/resolved_ldap_provider.go @@ -128,7 +128,7 @@ func (p *FederationDomainResolvedLDAPIdentityProvider) Login( ) (*resolvedprovider.Identity, *resolvedprovider.IdentityLoginExtras, error) { authenticateResponse, authenticated, err := p.Provider.AuthenticateUser(ctx, submittedUsername, submittedPassword) if err != nil { - plog.WarningErr("unexpected error during upstream LDAP authentication", err, "upstreamName", p.Provider.GetName()) + plog.WarningErr("unexpected error during upstream LDAP authentication", err, "upstreamName", p.Provider.GetResourceName()) return nil, nil, ErrUnexpectedUpstreamLDAPError.WithWrap(err) } if !authenticated { @@ -211,7 +211,7 @@ func (p *FederationDomainResolvedLDAPIdentityProvider) UpstreamRefresh( // This shouldn't really happen. return nil, resolvedprovider.ErrUpstreamRefreshError().WithHintf( "Unexpected provider type during refresh %q", p.GetSessionProviderType()).WithTrace(err). - WithDebugf("provider name: %q, provider type: %q", p.Provider.GetName(), p.GetSessionProviderType()) + WithDebugf("provider name: %q, provider type: %q", p.Provider.GetResourceName(), p.GetSessionProviderType()) } if dn == "" { @@ -219,7 +219,9 @@ func (p *FederationDomainResolvedLDAPIdentityProvider) UpstreamRefresh( } plog.Debug("attempting upstream refresh request", - "providerName", p.Provider.GetName(), "providerType", p.GetSessionProviderType(), "providerUID", p.Provider.GetResourceUID()) + "identityProviderResourceName", p.Provider.GetResourceName(), + "identityProviderType", p.GetSessionProviderType(), + "identityProviderUID", p.Provider.GetResourceUID()) refreshedUntransformedGroups, err := p.Provider.PerformRefresh(ctx, upstreamprovider.LDAPRefreshAttributes{ Username: identity.UpstreamUsername, @@ -231,7 +233,7 @@ func (p *FederationDomainResolvedLDAPIdentityProvider) UpstreamRefresh( if err != nil { return nil, resolvedprovider.ErrUpstreamRefreshError().WithHint( "Upstream refresh failed.").WithTrace(err). - WithDebugf("provider name: %q, provider type: %q", p.Provider.GetName(), p.GetSessionProviderType()) + WithDebugf("provider name: %q, provider type: %q", p.Provider.GetResourceName(), p.GetSessionProviderType()) } return &resolvedprovider.RefreshedIdentity{ diff --git a/internal/federationdomain/resolvedprovider/resolvedoidc/resolved_oidc_provider.go b/internal/federationdomain/resolvedprovider/resolvedoidc/resolved_oidc_provider.go index 1258e292c..37a35c7f1 100644 --- a/internal/federationdomain/resolvedprovider/resolvedoidc/resolved_oidc_provider.go +++ b/internal/federationdomain/resolvedprovider/resolvedoidc/resolved_oidc_provider.go @@ -191,8 +191,7 @@ func (p *FederationDomainResolvedOIDCIdentityProvider) LoginFromCallback( redirectURI, ) if err != nil { - plog.WarningErr("error exchanging and validating upstream tokens", err, "upstreamName", p.Provider.GetName()) - return nil, nil, httperr.New(http.StatusBadGateway, "error exchanging and validating upstream tokens") + return nil, nil, httperr.Wrap(http.StatusBadGateway, "error exchanging and validating upstream tokens", err) } subject, upstreamUsername, upstreamGroups, err := getIdentityFromUpstreamIDToken( @@ -241,7 +240,9 @@ func (p *FederationDomainResolvedOIDCIdentityProvider) UpstreamRefresh( } plog.Debug("attempting upstream refresh request", - "providerName", p.Provider.GetName(), "providerType", p.GetSessionProviderType(), "providerUID", p.Provider.GetResourceUID()) + "identityProviderResourceName", p.Provider.GetResourceName(), + "identityProviderType", p.GetSessionProviderType(), + "identityProviderUID", p.Provider.GetResourceUID()) var tokens *oauth2.Token if refreshTokenStored { @@ -249,7 +250,7 @@ func (p *FederationDomainResolvedOIDCIdentityProvider) UpstreamRefresh( if err != nil { return nil, resolvedprovider.ErrUpstreamRefreshError().WithHint( "Upstream refresh failed.", - ).WithTrace(err).WithDebugf("provider name: %q, provider type: %q", p.Provider.GetName(), p.GetSessionProviderType()) + ).WithTrace(err).WithDebugf("provider name: %q, provider type: %q", p.Provider.GetResourceName(), p.GetSessionProviderType()) } } else { tokens = &oauth2.Token{AccessToken: sessionData.UpstreamAccessToken} @@ -270,13 +271,13 @@ func (p *FederationDomainResolvedOIDCIdentityProvider) UpstreamRefresh( if err != nil { return nil, resolvedprovider.ErrUpstreamRefreshError().WithHintf( "Upstream refresh returned an invalid ID token or UserInfo response.").WithTrace(err). - WithDebugf("provider name: %q, provider type: %q", p.Provider.GetName(), p.GetSessionProviderType()) + WithDebugf("provider name: %q, provider type: %q", p.Provider.GetResourceName(), p.GetSessionProviderType()) } mergedClaims := validatedTokens.IDToken.Claims // To the extent possible, check that the user's basic identity hasn't changed. We check that their downstream // username has not changed separately below, as part of reapplying the transformations. - err = validateUpstreamSubjectAndIssuerUnchangedSinceInitialLogin(mergedClaims, sessionData, p.Provider.GetName(), p.GetSessionProviderType()) + err = validateUpstreamSubjectAndIssuerUnchangedSinceInitialLogin(mergedClaims, sessionData, p.Provider.GetResourceName(), p.GetSessionProviderType()) if err != nil { return nil, err } @@ -292,7 +293,7 @@ func (p *FederationDomainResolvedOIDCIdentityProvider) UpstreamRefresh( if err != nil { return nil, resolvedprovider.ErrUpstreamRefreshError().WithHintf( "Upstream refresh error while extracting groups claim.").WithTrace(err). - WithDebugf("provider name: %q, provider type: %q", p.Provider.GetName(), p.GetSessionProviderType()) + WithDebugf("provider name: %q, provider type: %q", p.Provider.GetResourceName(), p.GetSessionProviderType()) } // It's possible that a username wasn't returned by the upstream provider during refresh, @@ -312,7 +313,9 @@ func (p *FederationDomainResolvedOIDCIdentityProvider) UpstreamRefresh( // overwriting the old one. if tokens.RefreshToken != "" { plog.Debug("upstream refresh request returned a new refresh token", - "providerName", p.Provider.GetName(), "providerType", p.GetSessionProviderType(), "providerUID", p.Provider.GetResourceUID()) + "identityProviderResourceName", p.Provider.GetResourceName(), + "identityProviderType", p.GetSessionProviderType(), + "identityProviderUID", p.Provider.GetResourceUID()) updatedSessionData.UpstreamRefreshToken = tokens.RefreshToken } @@ -370,11 +373,11 @@ func makeDownstreamOIDCSessionData( oidcUpstream upstreamprovider.UpstreamOIDCIdentityProviderI, token *oidctypes.Token, ) (*psession.OIDCSessionData, []string, error) { - upstreamSubject, err := extractStringClaimValue(oidc.IDTokenClaimSubject, oidcUpstream.GetName(), token.IDToken.Claims) + upstreamSubject, err := extractStringClaimValue(oidc.IDTokenClaimSubject, oidcUpstream.GetResourceName(), token.IDToken.Claims) if err != nil { return nil, nil, err } - upstreamIssuer, err := extractStringClaimValue(oidc.IDTokenClaimIssuer, oidcUpstream.GetName(), token.IDToken.Claims) + upstreamIssuer, err := extractStringClaimValue(oidc.IDTokenClaimIssuer, oidcUpstream.GetResourceName(), token.IDToken.Claims) if err != nil { return nil, nil, err } @@ -387,7 +390,7 @@ func makeDownstreamOIDCSessionData( const pleaseCheck = "please check configuration of OIDCIdentityProvider and the client in the " + "upstream provider's API/UI and try to get a refresh token if possible" logKV := []interface{}{ - "upstreamName", oidcUpstream.GetName(), + "identityProviderResourceName", oidcUpstream.GetResourceName(), "scopes", oidcUpstream.GetScopes(), "additionalParams", oidcUpstream.GetAdditionalAuthcodeParams(), } @@ -452,7 +455,7 @@ func mapAdditionalClaimsFromUpstreamIDToken( if !ok { plog.Warning( "additionalClaims mapping claim in upstream ID token missing", - "upstreamName", upstreamIDPConfig.GetName(), + "identityProviderResourceName", upstreamIDPConfig.GetResourceName(), "claimName", upstreamClaimName, ) } else { @@ -469,11 +472,11 @@ func getDownstreamSubjectAndUpstreamUsernameFromUpstreamIDToken( ) (string, string, error) { // The spec says the "sub" claim is only unique per issuer, // so we will prepend the issuer string to make it globally unique. - upstreamIssuer, err := extractStringClaimValue(oidc.IDTokenClaimIssuer, upstreamIDPConfig.GetName(), idTokenClaims) + upstreamIssuer, err := extractStringClaimValue(oidc.IDTokenClaimIssuer, upstreamIDPConfig.GetResourceName(), idTokenClaims) if err != nil { return "", "", err } - upstreamSubject, err := extractStringClaimValue(oidc.IDTokenClaimSubject, upstreamIDPConfig.GetName(), idTokenClaims) + upstreamSubject, err := extractStringClaimValue(oidc.IDTokenClaimSubject, upstreamIDPConfig.GetResourceName(), idTokenClaims) if err != nil { return "", "", err } @@ -492,7 +495,7 @@ func getDownstreamSubjectAndUpstreamUsernameFromUpstreamIDToken( if !ok { plog.Warning( "username claim configured as \"email\" and upstream email_verified claim is not a boolean", - "upstreamName", upstreamIDPConfig.GetName(), + "identityProviderResourceName", upstreamIDPConfig.GetResourceName(), "configuredUsernameClaim", usernameClaimName, "emailVerifiedClaim", emailVerifiedAsInterface, ) @@ -501,14 +504,14 @@ func getDownstreamSubjectAndUpstreamUsernameFromUpstreamIDToken( if !emailVerified { plog.Warning( "username claim configured as \"email\" and upstream email_verified claim has false value", - "upstreamName", upstreamIDPConfig.GetName(), + "identityProviderResourceName", upstreamIDPConfig.GetResourceName(), "configuredUsernameClaim", usernameClaimName, ) return "", "", emailVerifiedClaimFalseErr } } - username, err := extractStringClaimValue(usernameClaimName, upstreamIDPConfig.GetName(), idTokenClaims) + username, err := extractStringClaimValue(usernameClaimName, upstreamIDPConfig.GetResourceName(), idTokenClaims) if err != nil { return "", "", err } @@ -571,7 +574,7 @@ func getGroupsFromUpstreamIDToken( if !ok { plog.Warning( "no groups claim in upstream ID token", - "upstreamName", upstreamIDPConfig.GetName(), + "identityProviderResourceName", upstreamIDPConfig.GetResourceName(), "configuredGroupsClaim", groupsClaimName, ) return nil, nil // the upstream IDP may have omitted the claim if the user has no groups @@ -581,7 +584,7 @@ func getGroupsFromUpstreamIDToken( if !okAsArray { plog.Warning( "groups claim in upstream ID token has invalid format", - "upstreamName", upstreamIDPConfig.GetName(), + "identityProviderResourceName", upstreamIDPConfig.GetResourceName(), "configuredGroupsClaim", groupsClaimName, ) return nil, requiredClaimInvalidFormatErr diff --git a/internal/federationdomain/upstreamprovider/upsteam_provider.go b/internal/federationdomain/upstreamprovider/upstream_provider.go similarity index 93% rename from internal/federationdomain/upstreamprovider/upsteam_provider.go rename to internal/federationdomain/upstreamprovider/upstream_provider.go index 40da3f922..5ca624892 100644 --- a/internal/federationdomain/upstreamprovider/upsteam_provider.go +++ b/internal/federationdomain/upstreamprovider/upstream_provider.go @@ -39,11 +39,11 @@ type LDAPRefreshAttributes struct { // UpstreamIdentityProviderI includes the interface functions that are common to all upstream identity provider types. // These represent the identity provider resources, i.e. OIDCIdentityProvider, etc. type UpstreamIdentityProviderI interface { - // GetName returns a name for this upstream provider. The controller watching the identity provider resources will + // GetResourceName returns a name for this upstream provider. The controller watching the identity provider resources will // set this to be the Name of the CR from its metadata. Note that this is different from the DisplayName configured // in each FederationDomain that uses this provider, so this name is for internal use only, not for interacting // with clients. Clients should not expect to see this name or send this name. - GetName() string + GetResourceName() string // GetResourceUID returns the Kubernetes resource ID GetResourceUID() types.UID @@ -134,6 +134,22 @@ type GitHubUser struct { DownstreamSubject string // the whole downstream subject URI } +// GitHubLoginDeniedError can be returned by UpstreamGithubIdentityProviderI GetUser() when a policy +// configured on GitHubIdentityProvider should prevent this user from completing authentication. +type GitHubLoginDeniedError struct { + message string +} + +func NewGitHubLoginDeniedError(message string) GitHubLoginDeniedError { + return GitHubLoginDeniedError{message: message} +} + +func (g GitHubLoginDeniedError) Error() string { + return g.message +} + +var _ error = &GitHubLoginDeniedError{} + type UpstreamGithubIdentityProviderI interface { UpstreamIdentityProviderI diff --git a/internal/mocks/mockupstreamoidcidentityprovider/mockupstreamoidcidentityprovider.go b/internal/mocks/mockupstreamoidcidentityprovider/mockupstreamoidcidentityprovider.go index 1c00c427e..c17bd85dd 100644 --- a/internal/mocks/mockupstreamoidcidentityprovider/mockupstreamoidcidentityprovider.go +++ b/internal/mocks/mockupstreamoidcidentityprovider/mockupstreamoidcidentityprovider.go @@ -150,9 +150,9 @@ func (mr *MockUpstreamOIDCIdentityProviderIMockRecorder) GetGroupsClaim() *gomoc } // GetName mocks base method. -func (m *MockUpstreamOIDCIdentityProviderI) GetName() string { +func (m *MockUpstreamOIDCIdentityProviderI) GetResourceName() string { m.ctrl.T.Helper() - ret := m.ctrl.Call(m, "GetName") + ret := m.ctrl.Call(m, "GetResourceName") ret0, _ := ret[0].(string) return ret0 } @@ -160,7 +160,7 @@ func (m *MockUpstreamOIDCIdentityProviderI) GetName() string { // GetName indicates an expected call of GetName. func (mr *MockUpstreamOIDCIdentityProviderIMockRecorder) GetName() *gomock.Call { mr.mock.ctrl.T.Helper() - return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "GetName", reflect.TypeOf((*MockUpstreamOIDCIdentityProviderI)(nil).GetName)) + return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "GetResourceName", reflect.TypeOf((*MockUpstreamOIDCIdentityProviderI)(nil).GetResourceName)) } // GetResourceUID mocks base method. diff --git a/internal/plog/plog.go b/internal/plog/plog.go index 9b722fe52..f5e1abe33 100644 --- a/internal/plog/plog.go +++ b/internal/plog/plog.go @@ -12,6 +12,7 @@ // // info should be reserved for "nice to know" information. It should be possible to run a production // pinniped server at the info log level with no performance degradation due to high log volume. +// // debug should be used for information targeted at developers and to aid in support cases. Care must // be taken at this level to not leak any secrets into the log stream. That is, even though debug may // cause performance issues in production, it must not cause security issues in production. diff --git a/internal/testutil/oidctestutil/testgithubprovider.go b/internal/testutil/oidctestutil/testgithubprovider.go index 88f183e15..aa8d1bc76 100644 --- a/internal/testutil/oidctestutil/testgithubprovider.go +++ b/internal/testutil/oidctestutil/testgithubprovider.go @@ -183,7 +183,7 @@ func (u *TestUpstreamGitHubIdentityProvider) GetResourceUID() types.UID { return u.ResourceUID } -func (u *TestUpstreamGitHubIdentityProvider) GetName() string { +func (u *TestUpstreamGitHubIdentityProvider) GetResourceName() string { return u.Name } diff --git a/internal/testutil/oidctestutil/testldapprovider.go b/internal/testutil/oidctestutil/testldapprovider.go index b9f3ea153..2808e9932 100644 --- a/internal/testutil/oidctestutil/testldapprovider.go +++ b/internal/testutil/oidctestutil/testldapprovider.go @@ -117,7 +117,7 @@ func (u *TestUpstreamLDAPIdentityProvider) GetResourceUID() types.UID { return u.ResourceUID } -func (u *TestUpstreamLDAPIdentityProvider) GetName() string { +func (u *TestUpstreamLDAPIdentityProvider) GetResourceName() string { return u.Name } diff --git a/internal/testutil/oidctestutil/testoidcprovider.go b/internal/testutil/oidctestutil/testoidcprovider.go index 285988917..33b1fb42b 100644 --- a/internal/testutil/oidctestutil/testoidcprovider.go +++ b/internal/testutil/oidctestutil/testoidcprovider.go @@ -123,7 +123,7 @@ func (u *TestUpstreamOIDCIdentityProvider) GetAdditionalClaimMappings() map[stri return u.AdditionalClaimMappings } -func (u *TestUpstreamOIDCIdentityProvider) GetName() string { +func (u *TestUpstreamOIDCIdentityProvider) GetResourceName() string { return u.Name } diff --git a/internal/upstreamgithub/upstreamgithub.go b/internal/upstreamgithub/upstreamgithub.go index d1085e466..6660787a1 100644 --- a/internal/upstreamgithub/upstreamgithub.go +++ b/internal/upstreamgithub/upstreamgithub.go @@ -6,7 +6,6 @@ package upstreamgithub import ( "context" - "errors" "fmt" "net/http" @@ -18,6 +17,7 @@ import ( "go.pinniped.dev/internal/federationdomain/downstreamsubject" "go.pinniped.dev/internal/federationdomain/upstreamprovider" "go.pinniped.dev/internal/githubclient" + "go.pinniped.dev/internal/plog" "go.pinniped.dev/internal/setutil" ) @@ -66,7 +66,7 @@ func New(config ProviderConfig) *Provider { } } -func (p *Provider) GetName() string { +func (p *Provider) GetResourceName() string { return p.c.Name } @@ -147,7 +147,20 @@ func (p *Provider) GetUser(ctx context.Context, accessToken string, idpDisplayNa } if !p.c.AllowedOrganizations.Empty() && !p.c.AllowedOrganizations.HasAnyIgnoringCase(orgMembership) { - return nil, errors.New("user is not allowed to log in due to organization membership policy") + plog.Warning("user is not allowed to log in due to organization membership policy", // do not log username to avoid PII + "userBelongsToOrganizations", orgMembership, + "configuredAllowedOrganizations", p.c.AllowedOrganizations, + "identityProviderDisplayName", idpDisplayName, + "identityProviderResourceName", p.GetResourceName()) + plog.Trace("user is not allowed to log in due to organization membership policy", // okay to log PII at trace level + "githubLogin", userInfo.Login, + "githubID", userInfo.ID, + "calculatedUsername", githubUser.Username, + "userBelongsToOrganizations", orgMembership, + "configuredAllowedOrganizations", p.c.AllowedOrganizations, + "identityProviderDisplayName", idpDisplayName, + "identityProviderResourceName", p.GetResourceName()) + return nil, upstreamprovider.NewGitHubLoginDeniedError("user is not allowed to log in due to organization membership policy") } teamMembership, err := githubClient.GetTeamMembership(ctx, p.c.AllowedOrganizations) diff --git a/internal/upstreamgithub/upstreamgithub_test.go b/internal/upstreamgithub/upstreamgithub_test.go index f9de917d3..f2dd1263d 100644 --- a/internal/upstreamgithub/upstreamgithub_test.go +++ b/internal/upstreamgithub/upstreamgithub_test.go @@ -74,7 +74,7 @@ func TestGitHubProvider(t *testing.T) { }, }, subject.GetConfig()) - require.Equal(t, "foo", subject.GetName()) + require.Equal(t, "foo", subject.GetResourceName()) require.Equal(t, types.UID("resource-uid-12345"), subject.GetResourceUID()) require.Equal(t, "fake-client-id", subject.GetClientID()) require.Equal(t, "fake-client-id", subject.GetClientID()) @@ -205,7 +205,8 @@ func TestGetUser(t *testing.T) { buildGitHubClientError error buildMockResponses func(hubInterface *mockgithubclient.MockGitHubInterface) wantUser *upstreamprovider.GitHubUser - wantErr string + wantErrMsg string + wantErr error }{ { name: "happy path with username=login:id", @@ -303,7 +304,7 @@ func TestGetUser(t *testing.T) { }, nil) mockGitHubInterface.EXPECT().GetOrgMembership(someContext).Return([]string{"disallowed-org"}, nil) }, - wantErr: "user is not allowed to log in due to organization membership policy", + wantErr: upstreamprovider.NewGitHubLoginDeniedError("user is not allowed to log in due to organization membership policy"), }, { name: "happy path with groups=name", @@ -390,7 +391,7 @@ func TestGetUser(t *testing.T) { HttpClient: someHttpClient, }, buildGitHubClientError: errors.New("error from building a github client"), - wantErr: "error from building a github client", + wantErrMsg: "error from building a github client", }, { name: "returns errors from githubClient.GetUserInfo()", @@ -401,7 +402,7 @@ func TestGetUser(t *testing.T) { buildMockResponses: func(mockGitHubInterface *mockgithubclient.MockGitHubInterface) { mockGitHubInterface.EXPECT().GetUserInfo(someContext).Return(nil, errors.New("error from githubClient.GetUserInfo")) }, - wantErr: "error from githubClient.GetUserInfo", + wantErrMsg: "error from githubClient.GetUserInfo", }, { name: "returns errors from githubClient.GetOrgMembership()", @@ -414,7 +415,7 @@ func TestGetUser(t *testing.T) { mockGitHubInterface.EXPECT().GetUserInfo(someContext).Return(&githubclient.UserInfo{}, nil) mockGitHubInterface.EXPECT().GetOrgMembership(someContext).Return(nil, errors.New("error from githubClient.GetOrgMembership")) }, - wantErr: "error from githubClient.GetOrgMembership", + wantErrMsg: "error from githubClient.GetOrgMembership", }, { name: "returns errors from githubClient.GetTeamMembership()", @@ -428,7 +429,7 @@ func TestGetUser(t *testing.T) { mockGitHubInterface.EXPECT().GetOrgMembership(someContext).Return(nil, nil) mockGitHubInterface.EXPECT().GetTeamMembership(someContext, gomock.Any()).Return(nil, errors.New("error from githubClient.GetTeamMembership")) }, - wantErr: "error from githubClient.GetTeamMembership", + wantErrMsg: "error from githubClient.GetTeamMembership", }, { name: "bad configuration: UsernameAttribute", @@ -443,7 +444,7 @@ func TestGetUser(t *testing.T) { ID: "some-github-id", }, nil) }, - wantErr: "bad configuration: unknown GitHub username attribute: this-is-not-legal-value-from-the-enum", + wantErrMsg: "bad configuration: unknown GitHub username attribute: this-is-not-legal-value-from-the-enum", }, { name: "bad configuration: GroupNameAttribute", @@ -467,7 +468,7 @@ func TestGetUser(t *testing.T) { }, }, nil) }, - wantErr: "bad configuration: unknown GitHub group name attribute: this-is-not-legal-value-from-the-enum", + wantErrMsg: "bad configuration: unknown GitHub group name attribute: this-is-not-legal-value-from-the-enum", }, } for _, test := range tests { @@ -493,13 +494,18 @@ func TestGetUser(t *testing.T) { } actualUser, actualErr := p.GetUser(context.Background(), accessToken, idpDisplayName) - if test.wantErr != "" { - require.EqualError(t, actualErr, test.wantErr) + + switch { + case test.wantErrMsg != "": + require.EqualError(t, actualErr, test.wantErrMsg) require.Nil(t, actualUser) - return + case test.wantErr != nil: + require.Equal(t, test.wantErr, actualErr) + require.Nil(t, actualUser) + default: + require.NoError(t, actualErr) + require.Equal(t, test.wantUser, actualUser) } - require.NoError(t, actualErr) - require.Equal(t, test.wantUser, actualUser) }) } } diff --git a/internal/upstreamldap/upstreamldap.go b/internal/upstreamldap/upstreamldap.go index 2509c2921..2821a4483 100644 --- a/internal/upstreamldap/upstreamldap.go +++ b/internal/upstreamldap/upstreamldap.go @@ -187,7 +187,7 @@ func closeAndLogError(conn Conn, doingWhat string) { } func (p *Provider) PerformRefresh(ctx context.Context, storedRefreshAttributes upstreamprovider.LDAPRefreshAttributes, idpDisplayName string) ([]string, error) { - t := trace.FromContext(ctx).Nest("slow ldap refresh attempt", trace.Field{Key: "providerName", Value: p.GetName()}) + t := trace.FromContext(ctx).Nest("slow ldap refresh attempt", trace.Field{Key: "providerName", Value: p.GetResourceName()}) defer t.LogIfLong(500 * time.Millisecond) // to help users debug slow LDAP searches userDN := storedRefreshAttributes.DN @@ -373,7 +373,7 @@ func (p *Provider) tlsConfig() (*tls.Config, error) { } // GetName returns a name for this upstream provider. -func (p *Provider) GetName() string { +func (p *Provider) GetResourceName() string { return p.c.Name } @@ -435,7 +435,7 @@ func (p *Provider) AuthenticateUser(ctx context.Context, username, password stri } func (p *Provider) authenticateUserImpl(ctx context.Context, username string, bindFunc func(conn Conn, foundUserDN string) error) (*authenticators.Response, bool, error) { - t := trace.FromContext(ctx).Nest("slow ldap authenticate user attempt", trace.Field{Key: "providerName", Value: p.GetName()}) + t := trace.FromContext(ctx).Nest("slow ldap authenticate user attempt", trace.Field{Key: "providerName", Value: p.GetResourceName()}) defer t.LogIfLong(500 * time.Millisecond) // to help users debug slow LDAP searches err := p.validateConfig() @@ -528,7 +528,7 @@ func (p *Provider) validateConfig() error { } func (p *Provider) SearchForDefaultNamingContext(ctx context.Context) (string, error) { - t := trace.FromContext(ctx).Nest("slow ldap attempt when searching for default naming context", trace.Field{Key: "providerName", Value: p.GetName()}) + t := trace.FromContext(ctx).Nest("slow ldap attempt when searching for default naming context", trace.Field{Key: "providerName", Value: p.GetResourceName()}) defer t.LogIfLong(500 * time.Millisecond) // to help users debug slow LDAP searches conn, err := p.dial(ctx) @@ -564,7 +564,7 @@ func (p *Provider) searchAndBindUser(conn Conn, username string, bindFunc func(c searchResult, err := conn.Search(p.userSearchRequest(username)) if err != nil { plog.All(`error searching for user`, - "upstreamName", p.GetName(), + "upstreamName", p.GetResourceName(), "username", username, "err", err, ) @@ -573,11 +573,11 @@ func (p *Provider) searchAndBindUser(conn Conn, username string, bindFunc func(c if len(searchResult.Entries) == 0 { if plog.Enabled(plog.LevelAll) { plog.All("error finding user: user not found (if this username is valid, please check the user search configuration)", - "upstreamName", p.GetName(), + "upstreamName", p.GetResourceName(), "username", username, ) } else { - plog.Debug("error finding user: user not found (cowardly avoiding printing username because log level is not 'all')", "upstreamName", p.GetName()) + plog.Debug("error finding user: user not found (cowardly avoiding printing username because log level is not 'all')", "upstreamName", p.GetResourceName()) } return nil, nil } @@ -632,7 +632,7 @@ func (p *Provider) searchAndBindUser(conn Conn, username string, bindFunc func(c err = bindFunc(conn, userEntry.DN) if err != nil { plog.DebugErr("error binding for user (if this is not the expected dn for this username, please check the user search configuration)", - err, "upstreamName", p.GetName(), "username", username, "dn", userEntry.DN) + err, "upstreamName", p.GetResourceName(), "username", username, "dn", userEntry.DN) ldapErr := &ldap.Error{} if errors.As(err, &ldapErr) && ldapErr.ResultCode == ldap.LDAPResultInvalidCredentials { return nil, nil diff --git a/internal/upstreamoidc/upstreamoidc.go b/internal/upstreamoidc/upstreamoidc.go index 8cd569a18..7c020f0e7 100644 --- a/internal/upstreamoidc/upstreamoidc.go +++ b/internal/upstreamoidc/upstreamoidc.go @@ -1,4 +1,4 @@ -// Copyright 2020-2023 the Pinniped contributors. All Rights Reserved. +// Copyright 2020-2024 the Pinniped contributors. All Rights Reserved. // SPDX-License-Identifier: Apache-2.0 // Package upstreamoidc implements an abstraction of upstream OIDC provider interactions. @@ -84,7 +84,7 @@ func (p *ProviderConfig) GetAdditionalClaimMappings() map[string]string { return p.AdditionalClaimMappings } -func (p *ProviderConfig) GetName() string { +func (p *ProviderConfig) GetResourceName() string { return p.Name } diff --git a/internal/upstreamoidc/upstreamoidc_test.go b/internal/upstreamoidc/upstreamoidc_test.go index 904bf6b21..c67110890 100644 --- a/internal/upstreamoidc/upstreamoidc_test.go +++ b/internal/upstreamoidc/upstreamoidc_test.go @@ -47,7 +47,7 @@ func TestProviderConfig(t *testing.T) { rawClaims: []byte(`{"userinfo_endpoint": "https://example.com/userinfo"}`), }, } - require.Equal(t, "test-name", p.GetName()) + require.Equal(t, "test-name", p.GetResourceName()) require.Equal(t, "test-client-id", p.GetClientID()) require.Equal(t, "https://example.com", p.GetAuthorizationURL().String()) require.ElementsMatch(t, []string{"scope1", "scope2"}, p.GetScopes()) diff --git a/proposals/1859_github-auth/README.md b/proposals/1859_github-auth/README.md index c63a7052e..a0bacdbde 100644 --- a/proposals/1859_github-auth/README.md +++ b/proposals/1859_github-auth/README.md @@ -1,7 +1,7 @@ --- title: "Authenticating Users via GitHub" authors: [ "@cfryanr" ] -status: "accepted" +status: "partially-implemented" sponsor: [ ] approval_date: "March 27, 2024" --- @@ -77,6 +77,7 @@ GitHub offers a REST API which can be used to find out about, among other things [team memberships of the currently authenticated user](https://docs.github.com/en/rest/teams/teams?apiVersion=2022-11-28#list-teams-for-the-authenticated-user) (`/user/teams`). These are the only three GitHub APIs that would be used by Pinniped. +Note that the path to these APIs is slightly different for GitHub Enterprise. Access tokens from both types of OAuth 2.0 clients and both types PATs can be used to call the GitHub APIs on behalf of a GitHub user. @@ -280,7 +281,7 @@ The status conditions will be populated by a controller. The conditions will inc - `HostValid`. This is not a URL, so it must not contain `://`. It must be parsable as defined by our `endpointaddr.Parse()` helper function. - `TLSConfigurationValid`. The CA bundle can be parsed as a CA bundle. - `OrganizationsPolicyValid`. Use the same logic and same error text in CEL validations for this field, and repeat it in the controller in case an old version of Kubernetes is being used. -- `ClientCredentialsValid`. This is what we call the same check in the OIDCIdentityProvider's status. +- `ClientCredentialsSecretValid`. The specified Secret must be found and must have the expected type and key/value pairs. - `GitHubConnectionValid`. Report if the host can be dialed with TLS verification. (In LDAPIdentityProvider we called this `LDAPConnectionValid`.) #### Web Browser-based Authentication using GitHubIdentityProvider @@ -556,18 +557,16 @@ None. or just one specific GitHubIdentityProvider in that FederationDomain? - Would it be helpful to offer a `GET` endpoint to list current PAT consents? What would a user do with this information? -- Should we also reduce the lifetime of the Supervisor-issued refresh tokens? This would be a signal to the client +- For PAT-base auth, should we also reduce the lifetime of the Supervisor-issued refresh tokens? This would be a signal to the client that the token is not going to work. Would this help the Pinniped CLI remove stale entries from the session cache file more quickly? - For the consent CLI commands, what if the kubeconfig's exec plugin is a path to a different CLI? It could be a different path to a different version of the Pinniped CLI, or it could be a different CLI entirely (like the `tanzu` CLI). Does this matter? -- Are the three GitHub API endpoints that we intend to use different for GitHub Enterprise Server (on-prem)? - Do they have different paths or different inputs and outputs? Need to check the GitHub docs. ## Answered Questions -- Does the GitHub Identity Provider need `additionalClaimMappings`? No. This feature was only added for +- Does the GitHubIdentityProvider need `additionalClaimMappings`? No. This feature was only added for OIDCIdentityProvider, and not yet added for other identity provider types. Please raise an issue in this repo if you need this feature. @@ -579,3 +578,9 @@ Community contributions to the effort would be welcomed. Contact the maintainers Implementing browser-based authentication will happen first. It is simpler and is a pre-requisite for the PAT consent feature for CLI-based authentication. + +### Implementation Progress + +As of June 2024, browser-based authentication using GitHub has been implemented. +This includes the GitHubIdentityProvider CRD except for the `spec.allowAuthentication.personalAccessTokens` section. +Authentication without a web browser using personal access tokens is not yet implemented. diff --git a/test/integration/supervisor_discovery_test.go b/test/integration/supervisor_discovery_test.go index a27980602..1c106ac7c 100644 --- a/test/integration/supervisor_discovery_test.go +++ b/test/integration/supervisor_discovery_test.go @@ -115,7 +115,7 @@ func TestSupervisorOIDCDiscovery_Disruptive(t *testing.T) { // When multiple FederationDomains exist at the same time they each serve a unique discovery endpoint. config3, jwks3 := requireCreatingFederationDomainCausesDiscoveryEndpointsToAppear(ctx, t, scheme, addr, caBundle, issuer3, client) config4, jwks4 := requireCreatingFederationDomainCausesDiscoveryEndpointsToAppear(ctx, t, scheme, addr, caBundle, issuer4, client) - requireDiscoveryEndpointsAreWorking(t, scheme, addr, caBundle, issuer3, nil) // discovery for issuer3 is still working after issuer4 started working + requireStandardDiscoveryEndpointsAreWorking(t, scheme, addr, caBundle, issuer3, nil) // discovery for issuer3 is still working after issuer4 started working // The auto-created JWK's were different from each other. require.NotEqual(t, jwks3.Keys[0]["x"], jwks4.Keys[0]["x"]) require.NotEqual(t, jwks3.Keys[0]["y"], jwks4.Keys[0]["y"]) @@ -123,7 +123,7 @@ func TestSupervisorOIDCDiscovery_Disruptive(t *testing.T) { // Editing a FederationDomain to change the issuer URL updates the endpoints that are being served. updatedConfig4 := editFederationDomainIssuerName(t, config4, client, ns, issuer5) requireDiscoveryEndpointsAreNotFound(t, scheme, addr, caBundle, issuer4) - jwks5 := requireDiscoveryEndpointsAreWorking(t, scheme, addr, caBundle, issuer5, nil) + jwks5 := requireStandardDiscoveryEndpointsAreWorking(t, scheme, addr, caBundle, issuer5, nil) // The JWK did not change when the issuer name was updated. require.Equal(t, jwks4.Keys[0], jwks5.Keys[0]) @@ -206,7 +206,7 @@ func TestSupervisorTLSTerminationWithSNI_Disruptive(t *testing.T) { ca1 := createTLSCertificateSecret(ctx, t, ns, hostname1, nil, certSecretName1, kubeClient) // Now that the Secret exists, we should be able to access the endpoints by hostname using the CA. - _ = requireDiscoveryEndpointsAreWorking(t, scheme, address, string(ca1.Bundle()), issuer1, nil) + _ = requireStandardDiscoveryEndpointsAreWorking(t, scheme, address, string(ca1.Bundle()), issuer1, nil) // Update the config to with a new .spec.tls.secretName. certSecretName1update := "integration-test-cert-1-update" @@ -227,7 +227,7 @@ func TestSupervisorTLSTerminationWithSNI_Disruptive(t *testing.T) { ca1update := createTLSCertificateSecret(ctx, t, ns, hostname1, nil, certSecretName1update, kubeClient) // Now that the Secret exists at the new name, we should be able to access the endpoints by hostname using the CA. - _ = requireDiscoveryEndpointsAreWorking(t, scheme, address, string(ca1update.Bundle()), issuer1, nil) + _ = requireStandardDiscoveryEndpointsAreWorking(t, scheme, address, string(ca1update.Bundle()), issuer1, nil) // To test SNI virtual hosting, send requests to discovery endpoints when the public address is different from the issuer name. hostname2 := "some-issuer-host-and-port-that-doesnt-match-public-supervisor-address.com" @@ -247,7 +247,7 @@ func TestSupervisorTLSTerminationWithSNI_Disruptive(t *testing.T) { ca2 := createTLSCertificateSecret(ctx, t, ns, hostname2, nil, certSecretName2, kubeClient) // Now that the Secret exists, we should be able to access the endpoints by hostname using the CA. - _ = requireDiscoveryEndpointsAreWorking(t, scheme, hostname2+":"+hostnamePort2, string(ca2.Bundle()), issuer2, map[string]string{ + _ = requireStandardDiscoveryEndpointsAreWorking(t, scheme, hostname2+":"+hostnamePort2, string(ca2.Bundle()), issuer2, map[string]string{ hostname2 + ":" + hostnamePort2: address, }) } @@ -300,7 +300,7 @@ func TestSupervisorTLSTerminationWithDefaultCerts_Disruptive(t *testing.T) { defaultCA := createTLSCertificateSecret(ctx, t, ns, "cert-hostname-doesnt-matter", []net.IP{ips[0]}, defaultTLSCertSecretName(env), kubeClient) // Now that the Secret exists, we should be able to access the endpoints by IP address using the CA. - _ = requireDiscoveryEndpointsAreWorking(t, scheme, ipWithPort, string(defaultCA.Bundle()), issuerUsingIPAddress, nil) + _ = requireStandardDiscoveryEndpointsAreWorking(t, scheme, ipWithPort, string(defaultCA.Bundle()), issuerUsingIPAddress, nil) // Create an FederationDomain with a spec.tls.secretName. certSecretName := "integration-test-cert-1" @@ -317,10 +317,10 @@ func TestSupervisorTLSTerminationWithDefaultCerts_Disruptive(t *testing.T) { // Now that the Secret exists, we should be able to access the endpoints by hostname using the CA from the SNI cert. // Hostnames are case-insensitive, so the request should still work even if the case of the hostname is different // from the case of the issuer URL's hostname. - _ = requireDiscoveryEndpointsAreWorking(t, scheme, strings.ToUpper(hostname)+":"+port, string(certCA.Bundle()), issuerUsingHostname, nil) + _ = requireStandardDiscoveryEndpointsAreWorking(t, scheme, strings.ToUpper(hostname)+":"+port, string(certCA.Bundle()), issuerUsingHostname, nil) // And we can still access the other issuer using the default cert. - _ = requireDiscoveryEndpointsAreWorking(t, scheme, ipWithPort, string(defaultCA.Bundle()), issuerUsingIPAddress, nil) + _ = requireStandardDiscoveryEndpointsAreWorking(t, scheme, ipWithPort, string(defaultCA.Bundle()), issuerUsingIPAddress, nil) } func defaultTLSCertSecretName(env *testlib.TestEnv) string { @@ -496,13 +496,12 @@ func requireCreatingFederationDomainCausesDiscoveryEndpointsToAppear( ) (*v1alpha1.FederationDomain, *ExpectedJWKSResponseFormat) { t.Helper() newFederationDomain := testlib.CreateTestFederationDomain(ctx, t, v1alpha1.FederationDomainSpec{Issuer: issuerName}, v1alpha1.FederationDomainPhaseReady) - jwksResult := requireDiscoveryEndpointsAreWorking(t, supervisorScheme, supervisorAddress, supervisorCABundle, issuerName, nil) + jwksResult := requireStandardDiscoveryEndpointsAreWorking(t, supervisorScheme, supervisorAddress, supervisorCABundle, issuerName, nil) requireStatus(t, client, newFederationDomain.Namespace, newFederationDomain.Name, v1alpha1.FederationDomainPhaseReady, withAllSuccessfulConditions()) return newFederationDomain, jwksResult } -// TODO: consider renaming this since it does not actually check all discovery endpoints (example: idp discovery is not tested). -func requireDiscoveryEndpointsAreWorking(t *testing.T, supervisorScheme, supervisorAddress, supervisorCABundle, issuerName string, dnsOverrides map[string]string) *ExpectedJWKSResponseFormat { +func requireStandardDiscoveryEndpointsAreWorking(t *testing.T, supervisorScheme, supervisorAddress, supervisorCABundle, issuerName string, dnsOverrides map[string]string) *ExpectedJWKSResponseFormat { requireWellKnownEndpointIsWorking(t, supervisorScheme, supervisorAddress, supervisorCABundle, issuerName, dnsOverrides) jwksResult := requireJWKSEndpointIsWorking(t, supervisorScheme, supervisorAddress, supervisorCABundle, issuerName, dnsOverrides) return jwksResult @@ -887,7 +886,7 @@ func requireIDPsListedByIDPDiscoveryEndpoint( IdentityProviders: idpsForFD, }, v1alpha1.FederationDomainPhaseReady) - requireDiscoveryEndpointsAreWorking(t, supervisorScheme, supervisorAddress, supervisorCABundle, issuerName, nil) + requireStandardDiscoveryEndpointsAreWorking(t, supervisorScheme, supervisorAddress, supervisorCABundle, issuerName, nil) issuer8URL, err := url.Parse(issuerName) require.NoError(t, err) wellKnownURL := wellKnownURLForIssuer(supervisorScheme, supervisorAddress, issuer8URL.Path) diff --git a/test/integration/supervisor_github_idp_test.go b/test/integration/supervisor_github_idp_test.go index 201d2a527..b250cdf3f 100644 --- a/test/integration/supervisor_github_idp_test.go +++ b/test/integration/supervisor_github_idp_test.go @@ -383,7 +383,7 @@ func TestGitHubIDPPhaseAndConditions_Parallel(t *testing.T) { Message: "spec.claims are valid", }, { - Type: "ClientCredentialsObtained", + Type: "ClientCredentialsSecretValid", Status: metav1.ConditionTrue, Reason: "Success", Message: fmt.Sprintf("clientID and clientSecret have been read from spec.client.SecretName (%q)", happySecretName), @@ -450,7 +450,7 @@ func TestGitHubIDPPhaseAndConditions_Parallel(t *testing.T) { Message: "spec.claims are valid", }, { - Type: "ClientCredentialsObtained", + Type: "ClientCredentialsSecretValid", Status: metav1.ConditionFalse, Reason: "SecretNotFound", Message: fmt.Sprintf(`missing key "clientID": secret from spec.client.SecretName (%q) must be found in namespace %q with type "secrets.pinniped.dev/github-client" and keys "clientID" and "clientSecret"`, @@ -656,7 +656,7 @@ func TestGitHubIDPSecretInOtherNamespace_Parallel(t *testing.T) { Message: "spec.claims are valid", }, { - Type: "ClientCredentialsObtained", + Type: "ClientCredentialsSecretValid", Status: metav1.ConditionFalse, Reason: "SecretNotFound", Message: fmt.Sprintf(`secret %q not found: secret from spec.client.SecretName (%q) must be found in namespace %q with type "secrets.pinniped.dev/github-client" and keys "clientID" and "clientSecret"`, diff --git a/test/integration/supervisor_login_test.go b/test/integration/supervisor_login_test.go index 42af416c7..011252b8c 100644 --- a/test/integration/supervisor_login_test.go +++ b/test/integration/supervisor_login_test.go @@ -2358,6 +2358,20 @@ func supervisorLoginGithubTestcases( } return testlib.CreateTestGitHubIdentityProvider(t, spec, idpv1alpha1.GitHubPhaseReady).Name }, + federationDomainIDPs: func(t *testing.T, idpName string) ([]configv1alpha1.FederationDomainIdentityProvider, string) { + displayName := "some-github-identity-provider-name" + return []configv1alpha1.FederationDomainIdentityProvider{ + { + DisplayName: displayName, + ObjectRef: corev1.TypedLocalObjectReference{ + APIGroup: ptr.To("idp.supervisor." + env.APIGroupSuffix), + Kind: "GitHubIdentityProvider", + Name: idpName, + }, + }, + }, + displayName + }, requestAuthorization: func(t *testing.T, _, downstreamAuthorizeURL, downstreamCallbackURL, _, _ string, httpClient *http.Client) { t.Helper() browser := openBrowserAndNavigateToAuthorizeURL(t, downstreamAuthorizeURL, httpClient) @@ -2370,7 +2384,7 @@ func supervisorLoginGithubTestcases( // Get the text of the preformatted error message showing on the page. textOfPreTag := browser.TextOfFirstMatch(t, "pre") require.Equal(t, - "Unprocessable Entity: failed to get user info from GitHub API\n", + `Forbidden: login denied due to configuration on GitHubIdentityProvider with display name "some-github-identity-provider-name": user is not allowed to log in due to organization membership policy`+"\n", textOfPreTag) }, wantLocalhostCallbackToNeverHappen: true, @@ -2532,7 +2546,7 @@ func testSupervisorLogin( ) { env := testlib.IntegrationEnv(t) - ctx, cancel := context.WithTimeout(context.Background(), 5*time.Minute) + ctx, cancel := context.WithTimeout(context.Background(), 7*time.Minute) defer cancel() // Infer the downstream issuer URL from the callback associated with the upstream test client registration. diff --git a/test/integration/supervisor_upstream_test.go b/test/integration/supervisor_upstream_test.go index 70c805ee1..fd91ed918 100644 --- a/test/integration/supervisor_upstream_test.go +++ b/test/integration/supervisor_upstream_test.go @@ -28,7 +28,7 @@ func TestSupervisorUpstreamOIDCDiscovery(t *testing.T) { upstream := testlib.CreateTestOIDCIdentityProvider(t, spec, v1alpha1.PhaseError) expectUpstreamConditions(t, upstream, []metav1.Condition{ { - Type: "ClientCredentialsValid", + Type: "ClientCredentialsSecretValid", Status: metav1.ConditionFalse, Reason: "SecretNotFound", Message: `secret "does-not-exist" not found`, @@ -66,7 +66,7 @@ Get "https://127.0.0.1:444444/invalid-url-that-is-really-really-long-nananananan upstream := testlib.CreateTestOIDCIdentityProvider(t, spec, v1alpha1.PhaseError) expectUpstreamConditions(t, upstream, []metav1.Condition{ { - Type: "ClientCredentialsValid", + Type: "ClientCredentialsSecretValid", Status: metav1.ConditionTrue, Reason: "Success", Message: "loaded client credentials", @@ -104,7 +104,7 @@ oidc: issuer did not match the issuer returned by provider, expected "` + env.Su upstream := testlib.CreateTestOIDCIdentityProvider(t, spec, v1alpha1.PhaseReady) expectUpstreamConditions(t, upstream, []metav1.Condition{ { - Type: "ClientCredentialsValid", + Type: "ClientCredentialsSecretValid", Status: metav1.ConditionTrue, Reason: "Success", Message: "loaded client credentials", diff --git a/test/testlib/browsertest/browsertest.go b/test/testlib/browsertest/browsertest.go index 9ace9fc4b..90a521075 100644 --- a/test/testlib/browsertest/browsertest.go +++ b/test/testlib/browsertest/browsertest.go @@ -5,7 +5,10 @@ package browsertest import ( + "bytes" + "compress/gzip" "context" + "encoding/base64" "fmt" "log" "regexp" @@ -16,6 +19,7 @@ import ( "time" chromedpbrowser "github.com/chromedp/cdproto/browser" + "github.com/chromedp/cdproto/dom" chromedpruntime "github.com/chromedp/cdproto/runtime" "github.com/chromedp/chromedp" "github.com/stretchr/testify/require" @@ -163,12 +167,52 @@ func OpenBrowser(t *testing.T) *Browser { for _, e := range b.exceptionEvents { t.Logf("exception: %s", e) } + + // If the test failed, dump helpful debugging info from the browser's final page. + if t.Failed() { + b.dumpPage(t) + } }) // Done. The browser is ready to be driven by the test. return b } +func (b *Browser) dumpPage(t *testing.T) { + // Log the URL of the current page. + var url string + b.runWithTimeout(t, b.timeout(), chromedp.Location(&url)) + t.Logf("Browser URL from end of test %q: %s", t.Name(), url) + + // Log the title of the current page. + t.Logf("Browser page title from end of test %q: %q", t.Name(), b.Title(t)) + + // Log a screenshot of the current page. + var screenBuf []byte + b.runWithTimeout(t, b.timeout(), chromedp.FullScreenshot(&screenBuf, 10)) // low quality to make it smaller + t.Logf("Browser screenshot (base64 encoded jpeg format) from end of test %q:\n%s\n", + t.Name(), base64.StdEncoding.EncodeToString(screenBuf)) + + // Log the HTML of the current page. + var html string + b.runWithTimeout(t, b.timeout(), chromedp.ActionFunc(func(ctx context.Context) error { + node, err := dom.GetDocument().Do(ctx) + if err != nil { + return err + } + html, err = dom.GetOuterHTML().WithNodeID(node.NodeID).Do(ctx) + return err + })) + var htmlBuf bytes.Buffer + gz := gzip.NewWriter(&htmlBuf) + _, err := gz.Write([]byte(html)) + require.NoError(t, err) + err = gz.Close() + require.NoError(t, err) + t.Logf("Browser html (gzip and base64 encoded) from end of test %q:\n%s\n", + t.Name(), base64.StdEncoding.EncodeToString(htmlBuf.Bytes())) +} + func (b *Browser) timeout() time.Duration { return 30 * time.Second } @@ -398,7 +442,10 @@ func handleGithubOTPLoginPage(t *testing.T, b *Browser, upstream testlib.TestGit // Sleep for a bit to make it less likely that we use the same OTP code twice when multiple tests are run in serial. // GitHub gets upset when the same OTP code gets reused. - otpSleepSeconds := 25 + // GitHub seems to also get upset when any OTP codes are used often, like when all our GitHub tests run sequentially, + // because sometimes auth will go to a GitHub page that says: "We were unable to authenticate your request because too + // many codes have been submitted. Please wait a few minutes and contact support if you continue to have problems." + otpSleepSeconds := 60 t.Logf("sleeping %d seconds before generating a GitHub OTP code", otpSleepSeconds) time.Sleep(time.Duration(otpSleepSeconds) * time.Second) @@ -427,7 +474,7 @@ func handleOccasionalGithubLoginPage(t *testing.T, b *Browser, upstream testlib. switch { case strings.HasPrefix(lowercaseTitle, "authorize "): // the title is "Authorize " - // Next Github might go to another page asking if you authorize the GitHub App to act on your behalf, + // Next GitHub might go to another page asking if you authorize the GitHub App to act on your behalf, // if this user has never authorized this app. // Wait for the authorize app page to be rendered. t.Logf("waiting for GitHub authorize button") @@ -441,7 +488,7 @@ func handleOccasionalGithubLoginPage(t *testing.T, b *Browser, upstream testlib. return true case strings.HasPrefix(lowercaseTitle, "confirm your account recovery settings"): - // Next Github might occasionally as you to confirm your recovery settings. + // Next GitHub might occasionally as you to confirm your recovery settings. // Wait for the page to be rendered. t.Logf("waiting for GitHub confirm button") // There are several buttons and links. We want to click this confirm button to confirm our settings: