Address review feedback

This commit is contained in:
Joshua Casey 2023-01-09 09:18:17 -06:00
parent 02ed2d9c95
commit 87fe42e18d
4 changed files with 20 additions and 10 deletions

View File

@ -156,14 +156,6 @@ spec:
claims: claims:
username: "$PINNIPED_TEST_SUPERVISOR_UPSTREAM_OIDC_USERNAME_CLAIM" username: "$PINNIPED_TEST_SUPERVISOR_UPSTREAM_OIDC_USERNAME_CLAIM"
groups: "$PINNIPED_TEST_SUPERVISOR_UPSTREAM_OIDC_GROUPS_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: client:
secretName: my-oidc-provider-client-secret secretName: my-oidc-provider-client-secret
EOF EOF

View File

@ -999,6 +999,7 @@ Get "` + testIssuerURL + `/valid-url-that-is-really-really-long-nananananananana
GroupsClaim: testGroupsClaim, GroupsClaim: testGroupsClaim,
AllowPasswordGrant: true, AllowPasswordGrant: true,
AdditionalAuthcodeParams: map[string]string{}, AdditionalAuthcodeParams: map[string]string{},
AdditionalClaimMappings: nil, // Does not default to empty map
ResourceUID: testUID, ResourceUID: testUID,
}, },
}, },
@ -1054,6 +1055,7 @@ Get "` + testIssuerURL + `/valid-url-that-is-really-really-long-nananananananana
GroupsClaim: testGroupsClaim, GroupsClaim: testGroupsClaim,
AllowPasswordGrant: false, AllowPasswordGrant: false,
AdditionalAuthcodeParams: map[string]string{}, AdditionalAuthcodeParams: map[string]string{},
AdditionalClaimMappings: nil, // Does not default to empty map
ResourceUID: testUID, ResourceUID: testUID,
}, },
}, },
@ -1109,6 +1111,7 @@ Get "` + testIssuerURL + `/valid-url-that-is-really-really-long-nananananananana
GroupsClaim: testGroupsClaim, GroupsClaim: testGroupsClaim,
AllowPasswordGrant: false, AllowPasswordGrant: false,
AdditionalAuthcodeParams: map[string]string{}, AdditionalAuthcodeParams: map[string]string{},
AdditionalClaimMappings: nil, // Does not default to empty map
ResourceUID: testUID, ResourceUID: testUID,
}, },
}, },
@ -1167,6 +1170,7 @@ Get "` + testIssuerURL + `/valid-url-that-is-really-really-long-nananananananana
GroupsClaim: testGroupsClaim, GroupsClaim: testGroupsClaim,
AllowPasswordGrant: false, AllowPasswordGrant: false,
AdditionalAuthcodeParams: map[string]string{}, AdditionalAuthcodeParams: map[string]string{},
AdditionalClaimMappings: nil, // Does not default to empty map
ResourceUID: testUID, ResourceUID: testUID,
}, },
}, },
@ -1195,7 +1199,13 @@ Get "` + testIssuerURL + `/valid-url-that-is-really-really-long-nananananananana
AdditionalAuthorizeParameters: testAdditionalParams, AdditionalAuthorizeParameters: testAdditionalParams,
AllowPasswordGrant: true, 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{ Status: v1alpha1.OIDCIdentityProviderStatus{
Phase: "Ready", Phase: "Ready",
@ -1227,7 +1237,10 @@ Get "` + testIssuerURL + `/valid-url-that-is-really-really-long-nananananananana
GroupsClaim: testGroupsClaim, GroupsClaim: testGroupsClaim,
AllowPasswordGrant: true, AllowPasswordGrant: true,
AdditionalAuthcodeParams: testExpectedAdditionalParams, AdditionalAuthcodeParams: testExpectedAdditionalParams,
ResourceUID: testUID, AdditionalClaimMappings: map[string]string{
"downstream": "upstream",
},
ResourceUID: testUID,
}, },
}, },
wantResultingUpstreams: []v1alpha1.OIDCIdentityProvider{{ 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].GetGroupsClaim(), actualIDP.GetGroupsClaim())
require.Equal(t, tt.wantResultingCache[i].AllowsPasswordGrant(), actualIDP.AllowsPasswordGrant()) require.Equal(t, tt.wantResultingCache[i].AllowsPasswordGrant(), actualIDP.AllowsPasswordGrant())
require.Equal(t, tt.wantResultingCache[i].GetAdditionalAuthcodeParams(), actualIDP.GetAdditionalAuthcodeParams()) 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].GetResourceUID(), actualIDP.GetResourceUID())
require.Equal(t, tt.wantResultingCache[i].GetRevocationURL(), actualIDP.GetRevocationURL()) require.Equal(t, tt.wantResultingCache[i].GetRevocationURL(), actualIDP.GetRevocationURL())
require.ElementsMatch(t, tt.wantResultingCache[i].GetScopes(), actualIDP.GetScopes()) require.ElementsMatch(t, tt.wantResultingCache[i].GetScopes(), actualIDP.GetScopes())

View File

@ -55,6 +55,8 @@ func TestMapAdditionalClaimsFromUpstreamIDToken(t *testing.T) {
for _, test := range tests { for _, test := range tests {
test := test test := test
t.Run(test.name, func(t *testing.T) { t.Run(test.name, func(t *testing.T) {
t.Parallel()
idp := oidctestutil.NewTestUpstreamOIDCIdentityProviderBuilder(). idp := oidctestutil.NewTestUpstreamOIDCIdentityProviderBuilder().
WithAdditionalClaimMappings(test.additionalClaimMappings). WithAdditionalClaimMappings(test.additionalClaimMappings).
Build() Build()

View File

@ -1108,6 +1108,8 @@ func validateAuthcodeStorage(
actualAdditionalClaims, ok := actualClaims.Get("additionalClaims").(map[string]interface{}) actualAdditionalClaims, ok := actualClaims.Get("additionalClaims").(map[string]interface{})
require.True(t, ok, "expected additionalClaims to be a map[string]interface{}") require.True(t, ok, "expected additionalClaims to be a map[string]interface{}")
require.Equal(t, wantAdditionalClaims, actualAdditionalClaims) 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. // Make sure that we asserted on every extra claim.