From 87fe42e18d76788fd71ab0da8ae664cbc871acfb Mon Sep 17 00:00:00 2001 From: Joshua Casey Date: Mon, 9 Jan 2023 09:18:17 -0600 Subject: [PATCH] Address review feedback --- hack/prepare-supervisor-on-kind.sh | 8 -------- .../oidc_upstream_watcher_test.go | 18 ++++++++++++++++-- .../downstream_session_test.go | 2 ++ internal/testutil/oidctestutil/oidctestutil.go | 2 ++ 4 files changed, 20 insertions(+), 10 deletions(-) diff --git a/hack/prepare-supervisor-on-kind.sh b/hack/prepare-supervisor-on-kind.sh index 17dcf33b..a56e5970 100755 --- a/hack/prepare-supervisor-on-kind.sh +++ b/hack/prepare-supervisor-on-kind.sh @@ -156,14 +156,6 @@ spec: claims: username: "$PINNIPED_TEST_SUPERVISOR_UPSTREAM_OIDC_USERNAME_CLAIM" groups: "$PINNIPED_TEST_SUPERVISOR_UPSTREAM_OIDC_GROUPS_CLAIM" - # TODO: This is just an experiment. Don't really want to commit additionalClaimMappings in this file. - additionalClaimMappings: - upstream_sub: sub - upstream_email: email - upstream_email_verified: email_verified - upstream_name: name - upstream_exp: exp - upstream_does_not_exist: foobaz client: secretName: my-oidc-provider-client-secret EOF diff --git a/internal/controller/supervisorconfig/oidcupstreamwatcher/oidc_upstream_watcher_test.go b/internal/controller/supervisorconfig/oidcupstreamwatcher/oidc_upstream_watcher_test.go index 6a17908c..e30fc824 100644 --- a/internal/controller/supervisorconfig/oidcupstreamwatcher/oidc_upstream_watcher_test.go +++ b/internal/controller/supervisorconfig/oidcupstreamwatcher/oidc_upstream_watcher_test.go @@ -999,6 +999,7 @@ Get "` + testIssuerURL + `/valid-url-that-is-really-really-long-nananananananana GroupsClaim: testGroupsClaim, AllowPasswordGrant: true, AdditionalAuthcodeParams: map[string]string{}, + AdditionalClaimMappings: nil, // Does not default to empty map ResourceUID: testUID, }, }, @@ -1054,6 +1055,7 @@ Get "` + testIssuerURL + `/valid-url-that-is-really-really-long-nananananananana GroupsClaim: testGroupsClaim, AllowPasswordGrant: false, AdditionalAuthcodeParams: map[string]string{}, + AdditionalClaimMappings: nil, // Does not default to empty map ResourceUID: testUID, }, }, @@ -1109,6 +1111,7 @@ Get "` + testIssuerURL + `/valid-url-that-is-really-really-long-nananananananana GroupsClaim: testGroupsClaim, AllowPasswordGrant: false, AdditionalAuthcodeParams: map[string]string{}, + AdditionalClaimMappings: nil, // Does not default to empty map ResourceUID: testUID, }, }, @@ -1167,6 +1170,7 @@ Get "` + testIssuerURL + `/valid-url-that-is-really-really-long-nananananananana GroupsClaim: testGroupsClaim, AllowPasswordGrant: false, AdditionalAuthcodeParams: map[string]string{}, + AdditionalClaimMappings: nil, // Does not default to empty map ResourceUID: testUID, }, }, @@ -1195,7 +1199,13 @@ Get "` + testIssuerURL + `/valid-url-that-is-really-really-long-nananananananana AdditionalAuthorizeParameters: testAdditionalParams, AllowPasswordGrant: true, }, - Claims: v1alpha1.OIDCClaims{Groups: testGroupsClaim, Username: testUsernameClaim}, + Claims: v1alpha1.OIDCClaims{ + Groups: testGroupsClaim, + Username: testUsernameClaim, + AdditionalClaimMappings: map[string]string{ + "downstream": "upstream", + }, + }, }, Status: v1alpha1.OIDCIdentityProviderStatus{ Phase: "Ready", @@ -1227,7 +1237,10 @@ Get "` + testIssuerURL + `/valid-url-that-is-really-really-long-nananananananana GroupsClaim: testGroupsClaim, AllowPasswordGrant: true, AdditionalAuthcodeParams: testExpectedAdditionalParams, - ResourceUID: testUID, + AdditionalClaimMappings: map[string]string{ + "downstream": "upstream", + }, + ResourceUID: testUID, }, }, wantResultingUpstreams: []v1alpha1.OIDCIdentityProvider{{ @@ -1442,6 +1455,7 @@ oidc: issuer did not match the issuer returned by provider, expected "` + testIs require.Equal(t, tt.wantResultingCache[i].GetGroupsClaim(), actualIDP.GetGroupsClaim()) require.Equal(t, tt.wantResultingCache[i].AllowsPasswordGrant(), actualIDP.AllowsPasswordGrant()) require.Equal(t, tt.wantResultingCache[i].GetAdditionalAuthcodeParams(), actualIDP.GetAdditionalAuthcodeParams()) + require.Equal(t, tt.wantResultingCache[i].GetAdditionalClaimMappings(), actualIDP.GetAdditionalClaimMappings()) require.Equal(t, tt.wantResultingCache[i].GetResourceUID(), actualIDP.GetResourceUID()) require.Equal(t, tt.wantResultingCache[i].GetRevocationURL(), actualIDP.GetRevocationURL()) require.ElementsMatch(t, tt.wantResultingCache[i].GetScopes(), actualIDP.GetScopes()) diff --git a/internal/oidc/downstreamsession/downstream_session_test.go b/internal/oidc/downstreamsession/downstream_session_test.go index e4e8af3d..58b87a56 100644 --- a/internal/oidc/downstreamsession/downstream_session_test.go +++ b/internal/oidc/downstreamsession/downstream_session_test.go @@ -55,6 +55,8 @@ func TestMapAdditionalClaimsFromUpstreamIDToken(t *testing.T) { for _, test := range tests { test := test t.Run(test.name, func(t *testing.T) { + t.Parallel() + idp := oidctestutil.NewTestUpstreamOIDCIdentityProviderBuilder(). WithAdditionalClaimMappings(test.additionalClaimMappings). Build() diff --git a/internal/testutil/oidctestutil/oidctestutil.go b/internal/testutil/oidctestutil/oidctestutil.go index 6e8ff494..a730b410 100644 --- a/internal/testutil/oidctestutil/oidctestutil.go +++ b/internal/testutil/oidctestutil/oidctestutil.go @@ -1108,6 +1108,8 @@ func validateAuthcodeStorage( actualAdditionalClaims, ok := actualClaims.Get("additionalClaims").(map[string]interface{}) require.True(t, ok, "expected additionalClaims to be a map[string]interface{}") require.Equal(t, wantAdditionalClaims, actualAdditionalClaims) + } else { + require.Nil(t, actualClaims.Get("additionalClaims"), "additionalClaims must be nil when there are no wanted additional claims") } // Make sure that we asserted on every extra claim.