From a0546942b830fee657be8d8e5e35f2ebe918b3f5 Mon Sep 17 00:00:00 2001 From: Andrew Keesler Date: Thu, 14 Jan 2021 10:17:17 -0500 Subject: [PATCH 1/6] test/integration: skip part of test to avoid Kube 1.20 GC bug See comment. Signed-off-by: Andrew Keesler Co-authored-by: Margo Crawford Co-authored-by: Monis Khan --- test/integration/kubeclient_test.go | 60 ++++++++++++++++++----------- 1 file changed, 37 insertions(+), 23 deletions(-) diff --git a/test/integration/kubeclient_test.go b/test/integration/kubeclient_test.go index 639b476c..4837604c 100644 --- a/test/integration/kubeclient_test.go +++ b/test/integration/kubeclient_test.go @@ -136,30 +136,44 @@ func TestKubeClientOwnerRef(t *testing.T) { return err }) - // sanity check API service client - apiService, err := ownerRefClient.Aggregation.ApiregistrationV1().APIServices().Create( - ctx, - &apiregistrationv1.APIService{ - ObjectMeta: metav1.ObjectMeta{ - Name: "v1.pandas.dev", - OwnerReferences: nil, // no owner refs set + // TODO: update middleware code to not set owner references on cluster-scoped objects. + // + // The Kube 1.20 garbage collector asserts some new behavior in regards to invalid owner + // references (i.e., when you have a namespace-scoped owner references for a cluster-scoped + // dependent, the cluster-scoped dependent is not removed). We also found a bug in the 1.20 + // garbage collector where namespace-scoped dependents are not garbage collected if their owner + // had been used as an invalid owner reference before - this bug causes our test to fallover + // because we are setting a namespace-scoped owner ref on this APIService. + // + // We believe that the best way to get around this problem is to update our kubeclient code to + // never set owner references on cluster-scoped objects. After we do that, we will uncomment this + // part of the test. + if false { + // sanity check API service client + apiService, err := ownerRefClient.Aggregation.ApiregistrationV1().APIServices().Create( + ctx, + &apiregistrationv1.APIService{ + ObjectMeta: metav1.ObjectMeta{ + Name: "v1.pandas.dev", + OwnerReferences: nil, // no owner refs set + }, + Spec: apiregistrationv1.APIServiceSpec{ + Version: "v1", + Group: "pandas.dev", + GroupPriorityMinimum: 10_000, + VersionPriority: 500, + }, }, - Spec: apiregistrationv1.APIServiceSpec{ - Version: "v1", - Group: "pandas.dev", - GroupPriorityMinimum: 10_000, - VersionPriority: 500, - }, - }, - metav1.CreateOptions{}, - ) - require.NoError(t, err) - hasOwnerRef(t, apiService, ref) - // this owner ref is invalid for an API service so it should be immediately deleted - isEventuallyDeleted(t, func() error { - _, err := ownerRefClient.Aggregation.ApiregistrationV1().APIServices().Get(ctx, apiService.Name, metav1.GetOptions{}) - return err - }) + metav1.CreateOptions{}, + ) + require.NoError(t, err) + hasOwnerRef(t, apiService, ref) + // this owner ref is invalid for an API service so it should be immediately deleted + isEventuallyDeleted(t, func() error { + _, err := ownerRefClient.Aggregation.ApiregistrationV1().APIServices().Get(ctx, apiService.Name, metav1.GetOptions{}) + return err + }) + } // sanity check concierge client credentialIssuer, err := ownerRefClient.PinnipedConcierge.ConfigV1alpha1().CredentialIssuers(namespace.Name).Create( From 8a916ce8ae87ac4cdbde2301d72841045fe1753d Mon Sep 17 00:00:00 2001 From: Andrew Keesler Date: Thu, 14 Jan 2021 10:17:46 -0500 Subject: [PATCH 2/6] test/integration: add test helper to avoid race conditions We were seeing a race in this test code since the require.NoError() and require.Eventually() would write to the same testing.T state on separate goroutines. Hopefully this helper function should cover the cases when we want to require.NoError() inside a require.Eventually() without causing a race. Signed-off-by: Andrew Keesler Co-authored-by: Margo Crawford Co-authored-by: Monis Khan --- test/integration/kubeclient_test.go | 9 ++++----- test/library/assertions.go | 26 ++++++++++++++++++++++++++ 2 files changed, 30 insertions(+), 5 deletions(-) create mode 100644 test/library/assertions.go diff --git a/test/integration/kubeclient_test.go b/test/integration/kubeclient_test.go index 4837604c..f65026ba 100644 --- a/test/integration/kubeclient_test.go +++ b/test/integration/kubeclient_test.go @@ -258,16 +258,15 @@ func hasOwnerRef(t *testing.T, obj metav1.Object, ref metav1.OwnerReference) { func isEventuallyDeleted(t *testing.T, f func() error) { t.Helper() - require.Eventually(t, func() bool { + library.RequireEventuallyWithoutError(t, func() (bool, error) { err := f() switch { case err == nil: - return false + return false, nil case errors.IsNotFound(err): - return true + return true, nil default: - require.NoError(t, err) - return false + return false, err } }, time.Minute, time.Second) } diff --git a/test/library/assertions.go b/test/library/assertions.go new file mode 100644 index 00000000..476e1ff3 --- /dev/null +++ b/test/library/assertions.go @@ -0,0 +1,26 @@ +// Copyright 2021 the Pinniped contributors. All Rights Reserved. +// SPDX-License-Identifier: Apache-2.0 + +package library + +import ( + "testing" + "time" + + "github.com/stretchr/testify/require" + "k8s.io/apimachinery/pkg/util/wait" +) + +// RequireEventuallyWithoutError is a wrapper around require.Eventually() that allows the caller to +// return an error from the condition function. If the condition function returns an error at any +// point, the assertion will immediately fail. +func RequireEventuallyWithoutError( + t *testing.T, + f func() (bool, error), + waitFor time.Duration, + tick time.Duration, + msgAndArgs ...interface{}, +) { + t.Helper() + require.NoError(t, wait.PollImmediate(tick, waitFor, f), msgAndArgs...) +} From 5e60c14ce77cb9edf60b6c78cd58220781d82e87 Mon Sep 17 00:00:00 2001 From: Margo Crawford Date: Thu, 14 Jan 2021 16:46:01 -0500 Subject: [PATCH 3/6] internal/upstreamoidc: log claims from ID token and userinfo Signed-off-by: Andrew Keesler --- internal/upstreamoidc/upstreamoidc.go | 15 ++++++++++++++- 1 file changed, 14 insertions(+), 1 deletion(-) diff --git a/internal/upstreamoidc/upstreamoidc.go b/internal/upstreamoidc/upstreamoidc.go index 9e825a3d..335ca5ae 100644 --- a/internal/upstreamoidc/upstreamoidc.go +++ b/internal/upstreamoidc/upstreamoidc.go @@ -1,4 +1,4 @@ -// Copyright 2020 the Pinniped contributors. All Rights Reserved. +// Copyright 2020-2021 the Pinniped contributors. All Rights Reserved. // SPDX-License-Identifier: Apache-2.0 // Package upstreamoidc implements an abstraction of upstream OIDC provider interactions. @@ -16,6 +16,7 @@ import ( "go.pinniped.dev/internal/httputil/httperr" "go.pinniped.dev/internal/oidc" "go.pinniped.dev/internal/oidc/provider" + "go.pinniped.dev/internal/plog" "go.pinniped.dev/pkg/oidcclient/nonce" "go.pinniped.dev/pkg/oidcclient/oidctypes" "go.pinniped.dev/pkg/oidcclient/pkce" @@ -101,10 +102,12 @@ func (p *ProviderConfig) ValidateToken(ctx context.Context, tok *oauth2.Token, e if err := validated.Claims(&validatedClaims); err != nil { return nil, httperr.Wrap(http.StatusInternalServerError, "could not unmarshal id token claims", err) } + plog.All("claims from ID token", "providerName", p.Name, "claims", listClaims(validatedClaims)) if err := p.fetchUserInfo(ctx, tok, validatedClaims); err != nil { return nil, httperr.Wrap(http.StatusInternalServerError, "could not fetch user info claims", err) } + plog.All("claims from ID token and userinfo", "providerName", p.Name, "claims", listClaims(validatedClaims)) return &oidctypes.Token{ AccessToken: &oidctypes.AccessToken{ @@ -159,3 +162,13 @@ func (p *ProviderConfig) fetchUserInfo(ctx context.Context, tok *oauth2.Token, c return nil } + +func listClaims(claims map[string]interface{}) []string { + list := make([]string, len(claims)) + i := 0 + for claim := range claims { + list[i] = claim + i++ + } + return list +} From 6fce1bd6bb64dd4e93098a2d3c5636f4b239b515 Mon Sep 17 00:00:00 2001 From: Andrew Keesler Date: Thu, 14 Jan 2021 17:21:41 -0500 Subject: [PATCH 4/6] Allow arrays of type interface and always set the groups claim to an array in the downstream token Signed-off-by: Margo Crawford --- internal/oidc/callback/callback_handler.go | 52 ++++++++++---- .../oidc/callback/callback_handler_test.go | 71 +++++++++++++------ test/integration/supervisor_login_test.go | 2 +- 3 files changed, 87 insertions(+), 38 deletions(-) diff --git a/internal/oidc/callback/callback_handler.go b/internal/oidc/callback/callback_handler.go index 417f1d2c..3598c35c 100644 --- a/internal/oidc/callback/callback_handler.go +++ b/internal/oidc/callback/callback_handler.go @@ -1,4 +1,4 @@ -// Copyright 2020 the Pinniped contributors. All Rights Reserved. +// Copyright 2020-2021 the Pinniped contributors. All Rights Reserved. // SPDX-License-Identifier: Apache-2.0 // Package callback provides a handler for the OIDC callback endpoint. @@ -255,10 +255,10 @@ func getSubjectAndUsernameFromUpstreamIDToken( func getGroupsFromUpstreamIDToken( upstreamIDPConfig provider.UpstreamOIDCIdentityProviderI, idTokenClaims map[string]interface{}, -) (interface{}, error) { +) ([]string, error) { groupsClaim := upstreamIDPConfig.GetGroupsClaim() if groupsClaim == "" { - return nil, nil + return []string{}, nil } groupsAsInterface, ok := idTokenClaims[groupsClaim] @@ -269,12 +269,11 @@ func getGroupsFromUpstreamIDToken( "configuredGroupsClaim", upstreamIDPConfig.GetGroupsClaim(), "groupsClaim", groupsClaim, ) - return nil, nil // the upstream IDP may have omitted the claim if the user has no groups + return []string{}, nil // the upstream IDP may have omitted the claim if the user has no groups } - groupsAsArray, okAsArray := groupsAsInterface.([]string) - groupsAsString, okAsString := groupsAsInterface.(string) - if !okAsArray && !okAsString { + groupsAsArray, okAsArray := extractGroups(groupsAsInterface) + if !okAsArray { plog.Warning( "groups claim in upstream ID token has invalid format", "upstreamName", upstreamIDPConfig.GetName(), @@ -284,13 +283,38 @@ func getGroupsFromUpstreamIDToken( return nil, httperr.New(http.StatusUnprocessableEntity, "groups claim in upstream ID token has invalid format") } - if okAsArray { - return groupsAsArray, nil - } - return groupsAsString, nil + return groupsAsArray, nil } -func makeDownstreamSession(subject string, username string, groups interface{}) *openid.DefaultSession { +func extractGroups(groupsAsInterface interface{}) ([]string, bool) { + groupsAsString, okAsString := groupsAsInterface.(string) + if okAsString { + return []string{groupsAsString}, true + } + + groupsAsStringArray, okAsStringArray := groupsAsInterface.([]string) + if okAsStringArray { + return groupsAsStringArray, true + } + + groupsAsInterfaceArray, okAsArray := groupsAsInterface.([]interface{}) + if !okAsArray { + return nil, false + } + + groupsAsStrings := make([]string, len(groupsAsInterfaceArray)) + for i, groupAsInterface := range groupsAsInterfaceArray { + groupAsString, okAsString := groupAsInterface.(string) + if !okAsString { + return nil, false + } + groupsAsStrings[i] = groupAsString + } + + return groupsAsStrings, true +} + +func makeDownstreamSession(subject string, username string, groups []string) *openid.DefaultSession { now := time.Now().UTC() openIDSession := &openid.DefaultSession{ Claims: &jwt.IDTokenClaims{ @@ -301,9 +325,7 @@ func makeDownstreamSession(subject string, username string, groups interface{}) } openIDSession.Claims.Extra = map[string]interface{}{ oidc.DownstreamUsernameClaim: username, - } - if groups != nil { - openIDSession.Claims.Extra[oidc.DownstreamGroupsClaim] = groups + oidc.DownstreamGroupsClaim: groups, } return openIDSession } diff --git a/internal/oidc/callback/callback_handler_test.go b/internal/oidc/callback/callback_handler_test.go index 3b5659f4..3e7db1af 100644 --- a/internal/oidc/callback/callback_handler_test.go +++ b/internal/oidc/callback/callback_handler_test.go @@ -1,4 +1,4 @@ -// Copyright 2020 the Pinniped contributors. All Rights Reserved. +// Copyright 2020-2021 the Pinniped contributors. All Rights Reserved. // SPDX-License-Identifier: Apache-2.0 package callback @@ -134,7 +134,7 @@ func TestCallbackEndpoint(t *testing.T) { wantDownstreamGrantedScopes []string wantDownstreamIDTokenSubject string wantDownstreamIDTokenUsername string - wantDownstreamIDTokenGroups interface{} + wantDownstreamIDTokenGroups []string wantDownstreamRequestedScopes []string wantDownstreamNonce string wantDownstreamPKCEChallenge string @@ -172,7 +172,7 @@ func TestCallbackEndpoint(t *testing.T) { wantBody: "", wantDownstreamIDTokenSubject: upstreamIssuer + "?sub=" + upstreamSubject, wantDownstreamIDTokenUsername: upstreamIssuer + "?sub=" + upstreamSubject, - wantDownstreamIDTokenGroups: nil, + wantDownstreamIDTokenGroups: []string{}, wantDownstreamRequestedScopes: happyDownstreamScopesRequested, wantDownstreamGrantedScopes: happyDownstreamScopesGranted, wantDownstreamNonce: downstreamNonce, @@ -210,7 +210,26 @@ func TestCallbackEndpoint(t *testing.T) { wantBody: "", wantDownstreamIDTokenSubject: upstreamIssuer + "?sub=" + upstreamSubject, wantDownstreamIDTokenUsername: upstreamUsername, - wantDownstreamIDTokenGroups: "notAnArrayGroup1 notAnArrayGroup2", + wantDownstreamIDTokenGroups: []string{"notAnArrayGroup1 notAnArrayGroup2"}, + wantDownstreamRequestedScopes: happyDownstreamScopesRequested, + wantDownstreamGrantedScopes: happyDownstreamScopesGranted, + wantDownstreamNonce: downstreamNonce, + wantDownstreamPKCEChallenge: downstreamPKCEChallenge, + wantDownstreamPKCEChallengeMethod: downstreamPKCEChallengeMethod, + wantExchangeAndValidateTokensCall: happyExchangeAndValidateTokensArgs, + }, + { + name: "upstream IDP's configured groups claim in the ID token is a slice of interfaces", + idp: happyUpstream().WithIDTokenClaim(upstreamGroupsClaim, []interface{}{"group1", "group2"}).Build(), + method: http.MethodGet, + path: newRequestPath().WithState(happyState).String(), + csrfCookie: happyCSRFCookie, + wantStatus: http.StatusFound, + wantRedirectLocationRegexp: happyDownstreamRedirectLocationRegexp, + wantBody: "", + wantDownstreamIDTokenSubject: upstreamIssuer + "?sub=" + upstreamSubject, + wantDownstreamIDTokenUsername: upstreamUsername, + wantDownstreamIDTokenGroups: []string{"group1", "group2"}, wantDownstreamRequestedScopes: happyDownstreamScopesRequested, wantDownstreamGrantedScopes: happyDownstreamScopesGranted, wantDownstreamNonce: downstreamNonce, @@ -437,6 +456,7 @@ func TestCallbackEndpoint(t *testing.T) { wantDownstreamIDTokenUsername: upstreamUsername, wantDownstreamRequestedScopes: happyDownstreamScopesRequested, wantDownstreamGrantedScopes: happyDownstreamScopesGranted, + wantDownstreamIDTokenGroups: []string{}, wantDownstreamNonce: downstreamNonce, wantDownstreamPKCEChallenge: downstreamPKCEChallenge, wantDownstreamPKCEChallengeMethod: downstreamPKCEChallengeMethod, @@ -482,6 +502,26 @@ func TestCallbackEndpoint(t *testing.T) { wantBody: "Unprocessable Entity: groups claim in upstream ID token has invalid format\n", wantExchangeAndValidateTokensCall: happyExchangeAndValidateTokensArgs, }, + { + name: "upstream ID token contains groups claim where one element is invalid", + idp: happyUpstream().WithIDTokenClaim(upstreamGroupsClaim, []interface{}{"foo", 7}).Build(), + method: http.MethodGet, + path: newRequestPath().WithState(happyState).String(), + csrfCookie: happyCSRFCookie, + wantStatus: http.StatusUnprocessableEntity, + wantBody: "Unprocessable Entity: groups claim in upstream ID token has invalid format\n", + wantExchangeAndValidateTokensCall: happyExchangeAndValidateTokensArgs, + }, + { + name: "upstream ID token contains groups claim with invalid null type", + idp: happyUpstream().WithIDTokenClaim(upstreamGroupsClaim, nil).Build(), + method: http.MethodGet, + path: newRequestPath().WithState(happyState).String(), + csrfCookie: happyCSRFCookie, + wantStatus: http.StatusUnprocessableEntity, + wantBody: "Unprocessable Entity: groups claim in upstream ID token has invalid format\n", + wantExchangeAndValidateTokensCall: happyExchangeAndValidateTokensArgs, + }, } for _, test := range tests { test := test @@ -779,7 +819,7 @@ func validateAuthcodeStorage( wantDownstreamGrantedScopes []string, wantDownstreamIDTokenSubject string, wantDownstreamIDTokenUsername string, - wantDownstreamIDTokenGroups interface{}, + wantDownstreamIDTokenGroups []string, wantDownstreamRequestedScopes []string, ) (*fosite.Request, *openid.DefaultSession) { t.Helper() @@ -818,23 +858,10 @@ func validateAuthcodeStorage( // Check the user's identity, which are put into the downstream ID token's subject, username and groups claims. require.Equal(t, wantDownstreamIDTokenSubject, actualClaims.Subject) require.Equal(t, wantDownstreamIDTokenUsername, actualClaims.Extra["username"]) - if wantDownstreamIDTokenGroups != nil { //nolint:nestif // there are some nested if's here but its probably fine for a test - require.Len(t, actualClaims.Extra, 2) - wantArray, ok := wantDownstreamIDTokenGroups.([]string) - if ok { - require.ElementsMatch(t, wantArray, actualClaims.Extra["groups"]) - } else { - wantString, ok := wantDownstreamIDTokenGroups.(string) - if ok { - require.Equal(t, wantString, actualClaims.Extra["groups"]) - } else { - require.Fail(t, "wantDownstreamIDTokenGroups should be of type: either []string or string") - } - } - } else { - require.Len(t, actualClaims.Extra, 1) - require.NotContains(t, actualClaims.Extra, "groups") - } + require.Len(t, actualClaims.Extra, 2) + actualDownstreamIDTokenGroups := actualClaims.Extra["groups"] + require.NotNil(t, actualDownstreamIDTokenGroups) + require.ElementsMatch(t, wantDownstreamIDTokenGroups, actualDownstreamIDTokenGroups) // Check the rest of the downstream ID token's claims. Fosite wants us to set these (in UTC time). testutil.RequireTimeInDelta(t, time.Now().UTC(), actualClaims.RequestedAt, timeComparisonFudgeFactor) diff --git a/test/integration/supervisor_login_test.go b/test/integration/supervisor_login_test.go index 60ecbb67..3c3b966e 100644 --- a/test/integration/supervisor_login_test.go +++ b/test/integration/supervisor_login_test.go @@ -207,7 +207,7 @@ func TestSupervisorLogin(t *testing.T) { tokenResponse, err := downstreamOAuth2Config.Exchange(oidcHTTPClientContext, authcode, pkceParam.Verifier()) require.NoError(t, err) - expectedIDTokenClaims := []string{"iss", "exp", "sub", "aud", "auth_time", "iat", "jti", "nonce", "rat", "username"} + expectedIDTokenClaims := []string{"iss", "exp", "sub", "aud", "auth_time", "iat", "jti", "nonce", "rat", "username", "groups"} verifyTokenResponse(t, tokenResponse, discovery, downstreamOAuth2Config, env.SupervisorTestUpstream.Issuer, nonceParam, expectedIDTokenClaims) // token exchange on the original token From d11a73c519961ed582e01a25b46cbeda338f8be4 Mon Sep 17 00:00:00 2001 From: Margo Crawford Date: Thu, 14 Jan 2021 15:11:00 -0800 Subject: [PATCH 5/6] PR feedback-- omit empty groups, keep groups as nil until last minute Also log keys and values for claims --- internal/oidc/callback/callback_handler.go | 15 ++++++++++----- internal/upstreamoidc/upstreamoidc.go | 14 ++------------ 2 files changed, 12 insertions(+), 17 deletions(-) diff --git a/internal/oidc/callback/callback_handler.go b/internal/oidc/callback/callback_handler.go index 3598c35c..6a6e6431 100644 --- a/internal/oidc/callback/callback_handler.go +++ b/internal/oidc/callback/callback_handler.go @@ -258,7 +258,7 @@ func getGroupsFromUpstreamIDToken( ) ([]string, error) { groupsClaim := upstreamIDPConfig.GetGroupsClaim() if groupsClaim == "" { - return []string{}, nil + return nil, nil } groupsAsInterface, ok := idTokenClaims[groupsClaim] @@ -269,7 +269,7 @@ func getGroupsFromUpstreamIDToken( "configuredGroupsClaim", upstreamIDPConfig.GetGroupsClaim(), "groupsClaim", groupsClaim, ) - return []string{}, nil // the upstream IDP may have omitted the claim if the user has no groups + return nil, nil // the upstream IDP may have omitted the claim if the user has no groups } groupsAsArray, okAsArray := extractGroups(groupsAsInterface) @@ -302,13 +302,15 @@ func extractGroups(groupsAsInterface interface{}) ([]string, bool) { return nil, false } - groupsAsStrings := make([]string, len(groupsAsInterfaceArray)) - for i, groupAsInterface := range groupsAsInterfaceArray { + var groupsAsStrings []string + for _, groupAsInterface := range groupsAsInterfaceArray { groupAsString, okAsString := groupAsInterface.(string) if !okAsString { return nil, false } - groupsAsStrings[i] = groupAsString + if groupAsString != "" { + groupsAsStrings = append(groupsAsStrings, groupAsString) + } } return groupsAsStrings, true @@ -323,6 +325,9 @@ func makeDownstreamSession(subject string, username string, groups []string) *op AuthTime: now, }, } + if groups == nil { + groups = []string{} + } openIDSession.Claims.Extra = map[string]interface{}{ oidc.DownstreamUsernameClaim: username, oidc.DownstreamGroupsClaim: groups, diff --git a/internal/upstreamoidc/upstreamoidc.go b/internal/upstreamoidc/upstreamoidc.go index 335ca5ae..43eccb15 100644 --- a/internal/upstreamoidc/upstreamoidc.go +++ b/internal/upstreamoidc/upstreamoidc.go @@ -102,12 +102,12 @@ func (p *ProviderConfig) ValidateToken(ctx context.Context, tok *oauth2.Token, e if err := validated.Claims(&validatedClaims); err != nil { return nil, httperr.Wrap(http.StatusInternalServerError, "could not unmarshal id token claims", err) } - plog.All("claims from ID token", "providerName", p.Name, "claims", listClaims(validatedClaims)) + plog.All("claims from ID token", "providerName", p.Name, "claims", validatedClaims) if err := p.fetchUserInfo(ctx, tok, validatedClaims); err != nil { return nil, httperr.Wrap(http.StatusInternalServerError, "could not fetch user info claims", err) } - plog.All("claims from ID token and userinfo", "providerName", p.Name, "claims", listClaims(validatedClaims)) + plog.All("claims from ID token and userinfo", "providerName", p.Name, "claims", validatedClaims) return &oidctypes.Token{ AccessToken: &oidctypes.AccessToken{ @@ -162,13 +162,3 @@ func (p *ProviderConfig) fetchUserInfo(ctx context.Context, tok *oauth2.Token, c return nil } - -func listClaims(claims map[string]interface{}) []string { - list := make([]string, len(claims)) - i := 0 - for claim := range claims { - list[i] = claim - i++ - } - return list -} From 6a0dc1e2bb756fcbf92ac3f66d3748bdc29a04c4 Mon Sep 17 00:00:00 2001 From: Matt Moyer Date: Thu, 14 Jan 2021 20:46:56 -0600 Subject: [PATCH 6/6] Fix an issue in TestE2EFullIntegration groups assertions. The group claims read from the session cache file are loaded as `[]interface{}` (slice of empty interfaces) so when we previously did a `groups, _ := idTokenClaims[oidc.DownstreamGroupsClaim].([]string)`, then `groups` would always end up nil. The solution I tried here was to convert the expected value to also be `[]interface{}` so that `require.Equal(t, ...)` does the right thing. This bug only showed up in our acceptance environnment against Okta, since we don't have any other integration test coverage with IDPs that pass a groups claim. Signed-off-by: Matt Moyer --- test/integration/e2e_test.go | 14 ++++++-------- 1 file changed, 6 insertions(+), 8 deletions(-) diff --git a/test/integration/e2e_test.go b/test/integration/e2e_test.go index 844e3e35..5afa4c9d 100644 --- a/test/integration/e2e_test.go +++ b/test/integration/e2e_test.go @@ -292,14 +292,12 @@ func TestE2EFullIntegration(t *testing.T) { require.NotNil(t, token) idTokenClaims := token.IDToken.Claims - username := idTokenClaims[oidc.DownstreamUsernameClaim].(string) - groups, _ := idTokenClaims[oidc.DownstreamGroupsClaim].([]string) + require.Equal(t, env.SupervisorTestUpstream.Username, idTokenClaims[oidc.DownstreamUsernameClaim]) - require.Equal(t, env.SupervisorTestUpstream.Username, username) - if len(env.SupervisorTestUpstream.ExpectedGroups) == 0 { - // We only put a groups claim in our downstream ID token if we got groups from the upstream. - require.Nil(t, groups) - } else { - require.Equal(t, env.SupervisorTestUpstream.ExpectedGroups, groups) + // The groups claim in the file ends up as an []interface{}, so adjust our expectation to match. + expectedGroups := make([]interface{}, 0, len(env.SupervisorTestUpstream.ExpectedGroups)) + for _, g := range env.SupervisorTestUpstream.ExpectedGroups { + expectedGroups = append(expectedGroups, g) } + require.Equal(t, expectedGroups, idTokenClaims[oidc.DownstreamGroupsClaim]) }