From a4ca44ca14a230614e4a4611e1c81200ae635e81 Mon Sep 17 00:00:00 2001 From: Ryan Richard Date: Wed, 19 Jan 2022 12:57:47 -0800 Subject: [PATCH 1/3] Improve error handling when upstream groups is invalid during refresh --- internal/oidc/token/token_handler.go | 4 +- internal/oidc/token/token_handler_test.go | 63 ++++++++++++++++++++++- 2 files changed, 65 insertions(+), 2 deletions(-) diff --git a/internal/oidc/token/token_handler.go b/internal/oidc/token/token_handler.go index 93e1ac8f..4820a6d6 100644 --- a/internal/oidc/token/token_handler.go +++ b/internal/oidc/token/token_handler.go @@ -169,7 +169,9 @@ func upstreamOIDCRefresh(ctx context.Context, session *psession.PinnipedSession, // and let any old groups memberships in the session remain. refreshedGroups, err := downstreamsession.GetGroupsFromUpstreamIDToken(p, mergedClaims) if err != nil { - return err + return errorsx.WithStack(errUpstreamRefreshError.WithHintf( + "Upstream refresh error while extracting groups claim.").WithWrap(err). + WithDebugf("provider name: %q, provider type: %q", s.ProviderName, s.ProviderType)) } if refreshedGroups != nil { session.Fosite.Claims.Extra[oidc.DownstreamGroupsClaim] = refreshedGroups diff --git a/internal/oidc/token/token_handler_test.go b/internal/oidc/token/token_handler_test.go index d9b4cb68..8ef3c742 100644 --- a/internal/oidc/token/token_handler_test.go +++ b/internal/oidc/token/token_handler_test.go @@ -190,6 +190,13 @@ var ( } `) + fositeUpstreamGroupClaimErrorBody = here.Doc(` + { + "error": "error", + "error_description": "Error during upstream refresh. Upstream refresh error while extracting groups claim." + } + `) + happyAuthRequest = &http.Request{ Form: url.Values{ "response_type": {"code"}, @@ -1275,7 +1282,36 @@ func TestRefreshGrant(t *testing.T) { }, }, { - name: "happy path refresh grant when the upstream refresh does not return new group memberships from the merged ID token and userinfo results, it keeps groups from initial login", + name: "happy path refresh grant when the upstream refresh returns new group memberships as an empty list from the merged ID token and userinfo results, it updates groups to be empty", + idps: oidctestutil.NewUpstreamIDPListerBuilder().WithOIDC( + upstreamOIDCIdentityProviderBuilder().WithGroupsClaim("my-groups-claim").WithValidatedAndMergedWithUserInfoTokens(&oidctypes.Token{ + IDToken: &oidctypes.IDToken{ + Claims: map[string]interface{}{ + "sub": goodUpstreamSubject, + "my-groups-claim": []string{}, // refreshed groups claims is updated to be an empty list + }, + }, + }).WithRefreshedTokens(refreshedUpstreamTokensWithIDAndRefreshTokens()).Build()), + authcodeExchange: authcodeExchangeInputs{ + customSessionData: initialUpstreamOIDCRefreshTokenCustomSessionData(), + modifyAuthRequest: func(r *http.Request) { r.Form.Set("scope", "openid offline_access") }, + want: happyAuthcodeExchangeTokenResponseForOpenIDAndOfflineAccess(initialUpstreamOIDCRefreshTokenCustomSessionData()), + }, + refreshRequest: refreshRequestInputs{ + want: tokenEndpointResponseExpectedValues{ + wantStatus: http.StatusOK, + wantSuccessBodyFields: []string{"refresh_token", "access_token", "id_token", "token_type", "expires_in", "scope"}, + wantRequestedScopes: []string{"openid", "offline_access"}, + wantGrantedScopes: []string{"openid", "offline_access"}, + wantGroups: []string{}, // the user no longer belongs to any groups + wantUpstreamRefreshCall: happyOIDCUpstreamRefreshCall(), + wantUpstreamOIDCValidateTokenCall: happyUpstreamValidateTokenCall(refreshedUpstreamTokensWithIDAndRefreshTokens(), true), + wantCustomSessionDataStored: upstreamOIDCCustomSessionDataWithNewRefreshToken(oidcUpstreamRefreshedRefreshToken), + }, + }, + }, + { + name: "happy path refresh grant when the upstream refresh does not return new group memberships from the merged ID token and userinfo results by omitting claim, it keeps groups from initial login", idps: oidctestutil.NewUpstreamIDPListerBuilder().WithOIDC( upstreamOIDCIdentityProviderBuilder().WithGroupsClaim("my-groups-claim").WithValidatedAndMergedWithUserInfoTokens(&oidctypes.Token{ IDToken: &oidctypes.IDToken{ @@ -1303,6 +1339,31 @@ func TestRefreshGrant(t *testing.T) { }, }, }, + { + name: "error from refresh grant when the upstream refresh does not return new group memberships from the merged ID token and userinfo results by returning group claim with illegal nil value", + idps: oidctestutil.NewUpstreamIDPListerBuilder().WithOIDC( + upstreamOIDCIdentityProviderBuilder().WithGroupsClaim("my-groups-claim").WithValidatedAndMergedWithUserInfoTokens(&oidctypes.Token{ + IDToken: &oidctypes.IDToken{ + Claims: map[string]interface{}{ + "sub": goodUpstreamSubject, + "my-groups-claim": nil, + }, + }, + }).WithRefreshedTokens(refreshedUpstreamTokensWithIDAndRefreshTokens()).Build()), + authcodeExchange: authcodeExchangeInputs{ + customSessionData: initialUpstreamOIDCRefreshTokenCustomSessionData(), + modifyAuthRequest: func(r *http.Request) { r.Form.Set("scope", "openid offline_access") }, + want: happyAuthcodeExchangeTokenResponseForOpenIDAndOfflineAccess(initialUpstreamOIDCRefreshTokenCustomSessionData()), + }, + refreshRequest: refreshRequestInputs{ + want: tokenEndpointResponseExpectedValues{ + wantStatus: http.StatusUnauthorized, + wantErrorResponseBody: fositeUpstreamGroupClaimErrorBody, + wantUpstreamRefreshCall: happyOIDCUpstreamRefreshCall(), + wantUpstreamOIDCValidateTokenCall: happyUpstreamValidateTokenCall(refreshedUpstreamTokensWithIDAndRefreshTokens(), true), + }, + }, + }, { name: "happy path refresh grant when the upstream refresh does not return a new refresh token", idps: oidctestutil.NewUpstreamIDPListerBuilder().WithOIDC( From 513c943e87721f42918b30a61294a295e16862b4 Mon Sep 17 00:00:00 2001 From: Margo Crawford Date: Wed, 19 Jan 2022 13:29:26 -0800 Subject: [PATCH 2/3] Keep all scopes except offline_access in integration test --- test/integration/supervisor_login_test.go | 13 ++++++++++++- 1 file changed, 12 insertions(+), 1 deletion(-) diff --git a/test/integration/supervisor_login_test.go b/test/integration/supervisor_login_test.go index 5fdd6060..1fec0989 100644 --- a/test/integration/supervisor_login_test.go +++ b/test/integration/supervisor_login_test.go @@ -136,6 +136,17 @@ func TestSupervisorLogin(t *testing.T) { }, createIDP: func(t *testing.T) string { t.Helper() + var additionalScopes []string + // keep all the scopes except for offline access so we can test the access token based refresh flow. + if len(env.ToolsNamespace) == 0 { + additionalScopes = env.SupervisorUpstreamOIDC.AdditionalScopes + } else { + for _, additionalScope := range env.SupervisorUpstreamOIDC.AdditionalScopes { + if additionalScope != "offline_access" { + additionalScopes = append(additionalScopes, additionalScope) + } + } + } oidcIDP := testlib.CreateTestOIDCIdentityProvider(t, idpv1alpha1.OIDCIdentityProviderSpec{ Issuer: env.SupervisorUpstreamOIDC.Issuer, TLS: &idpv1alpha1.TLSSpec{ @@ -149,7 +160,7 @@ func TestSupervisorLogin(t *testing.T) { Groups: env.SupervisorUpstreamOIDC.GroupsClaim, }, AuthorizationConfig: idpv1alpha1.OIDCAuthorizationConfig{ - AdditionalScopes: []string{"email"}, // does not ask for offline_access. + AdditionalScopes: additionalScopes, }, }, idpv1alpha1.PhaseReady) return oidcIDP.Name From cd3d1333deefe8d389398af826c8bfbd5f80b886 Mon Sep 17 00:00:00 2001 From: "dependabot[bot]" <49699333+dependabot[bot]@users.noreply.github.com> Date: Wed, 19 Jan 2022 22:07:18 +0000 Subject: [PATCH 3/3] Bump github.com/ory/x from 0.0.331 to 0.0.334 Bumps [github.com/ory/x](https://github.com/ory/x) from 0.0.331 to 0.0.334. - [Release notes](https://github.com/ory/x/releases) - [Commits](https://github.com/ory/x/compare/v0.0.331...v0.0.334) --- updated-dependencies: - dependency-name: github.com/ory/x dependency-type: direct:production update-type: version-update:semver-patch ... Signed-off-by: dependabot[bot] --- go.mod | 2 +- go.sum | 4 ++-- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/go.mod b/go.mod index a911db20..2785e2a3 100644 --- a/go.mod +++ b/go.mod @@ -55,7 +55,7 @@ require ( github.com/joshlf/go-acl v0.0.0-20200411065538-eae00ae38531 github.com/mohae/deepcopy v0.0.0-20170929034955-c48cc78d4826 github.com/ory/fosite v0.41.0 - github.com/ory/x v0.0.331 + github.com/ory/x v0.0.334 github.com/pkg/browser v0.0.0-20210911075715-681adbf594b8 github.com/pkg/errors v0.9.1 github.com/sclevine/agouti v3.0.0+incompatible diff --git a/go.sum b/go.sum index 50303658..c8231f8a 100644 --- a/go.sum +++ b/go.sum @@ -1533,8 +1533,8 @@ github.com/ory/x v0.0.214/go.mod h1:aRl57gzyD4GF0HQCekovXhv0xTZgAgiht3o8eVhsm9Q= github.com/ory/x v0.0.250/go.mod h1:jUJaVptu+geeqlb9SyQCogTKj5ztSDIF6APkhbKtwLc= github.com/ory/x v0.0.272/go.mod h1:1TTPgJGQutrhI2OnwdrTIHE9ITSf4MpzXFzA/ncTGRc= github.com/ory/x v0.0.288/go.mod h1:APpShLyJcVzKw1kTgrHI+j/L9YM+8BRjHlcYObc7C1U= -github.com/ory/x v0.0.331 h1:Elw9xiTXqRDkO+7b4NKTIXTPC2nyk70HosbF2jQzEOI= -github.com/ory/x v0.0.331/go.mod h1:l9sL63RvxMWjhMIe9epl8SiqXoPh2TrwMXE4rNA2dOY= +github.com/ory/x v0.0.334 h1:ZtxDKRjrRYadZGYIg7kFI4wuEpRX7n5eMBQnxRU07lw= +github.com/ory/x v0.0.334/go.mod h1:vRr+//Cmpcu4HwkYwstv4mzie65ss+r76+iXU9fqQiA= github.com/pact-foundation/pact-go v1.0.4/go.mod h1:uExwJY4kCzNPcHRj+hCR/HBbOOIwwtUjcrb0b5/5kLM= github.com/parnurzeal/gorequest v0.2.15/go.mod h1:3Kh2QUMJoqw3icWAecsyzkpY7UzRfDhbRdTjtNwNiUE= github.com/pascaldekloe/goe v0.0.0-20180627143212-57f6aae5913c/go.mod h1:lzWF7FIEvWOWxwDKqyGYQf6ZUaNfKdP144TG7ZOy1lc=