From 6f3977de9da0dce4ba730383069f45f9032d1c5d Mon Sep 17 00:00:00 2001 From: Margo Crawford Date: Tue, 11 Jan 2022 11:00:54 -0800 Subject: [PATCH] Store access token when refresh not available for authcode flow. Also refactor oidc downstreamsessiondata code to be shared between callback handler and auth handler. Signed-off-by: Ryan Richard --- internal/oidc/auth/auth_handler.go | 43 +---------- internal/oidc/auth/auth_handler_test.go | 2 +- internal/oidc/callback/callback_handler.go | 27 +------ .../oidc/callback/callback_handler_test.go | 74 +++++++++++++++++-- .../downstreamsession/downstream_session.go | 46 ++++++++++++ 5 files changed, 119 insertions(+), 73 deletions(-) diff --git a/internal/oidc/auth/auth_handler.go b/internal/oidc/auth/auth_handler.go index 737567a9..4f281ad8 100644 --- a/internal/oidc/auth/auth_handler.go +++ b/internal/oidc/auth/auth_handler.go @@ -181,52 +181,13 @@ func handleAuthRequestForOIDCUpstreamPasswordGrant( fosite.ErrAccessDenied.WithHintf("Reason: %s.", err.Error()), true, ) } - upstreamSubject, err := downstreamsession.ExtractStringClaimValue(oidc.IDTokenSubjectClaim, oidcUpstream.GetName(), token.IDToken.Claims) + + customSessionData, err := downstreamsession.MakeDownstreamOIDCCustomSessionData(oidcUpstream, token) if err != nil { - // Return a user-friendly error for this case which is entirely within our control. return writeAuthorizeError(w, oauthHelper, authorizeRequester, fosite.ErrAccessDenied.WithHintf("Reason: %s.", err.Error()), true, ) } - upstreamIssuer, err := downstreamsession.ExtractStringClaimValue(oidc.IDTokenIssuerClaim, oidcUpstream.GetName(), token.IDToken.Claims) - if err != nil { - // Return a user-friendly error for this case which is entirely within our control. - return writeAuthorizeError(w, oauthHelper, authorizeRequester, - fosite.ErrAccessDenied.WithHintf("Reason: %s.", err.Error()), true, - ) - } - - customSessionData := &psession.CustomSessionData{ - ProviderUID: oidcUpstream.GetResourceUID(), - ProviderName: oidcUpstream.GetName(), - ProviderType: psession.ProviderTypeOIDC, - OIDC: &psession.OIDCSessionData{ - UpstreamIssuer: upstreamIssuer, - UpstreamSubject: upstreamSubject, - }, - } - - hasRefreshToken := token.RefreshToken != nil && token.RefreshToken.Token != "" - hasAccessToken := token.AccessToken != nil && token.AccessToken.Token != "" - switch { - case hasRefreshToken: // we prefer refresh tokens, so check for this first - customSessionData.OIDC.UpstreamRefreshToken = token.RefreshToken.Token - case hasAccessToken: - plog.Info("refresh token not returned by upstream provider during password grant, using access token instead. "+ - "please check configuration of OIDCIdentityProvider and the client in the upstream provider's API/UI "+ - "and try to get a refresh token if possible", - "upstreamName", oidcUpstream.GetName(), - "scopes", oidcUpstream.GetScopes()) - customSessionData.OIDC.UpstreamAccessToken = token.AccessToken.Token - default: - plog.Warning("refresh token and access token not returned by upstream provider during password grant, "+ - "please check configuration of OIDCIdentityProvider and the client in the upstream provider's API/UI", - "upstreamName", oidcUpstream.GetName(), - "scopes", oidcUpstream.GetScopes()) - return writeAuthorizeError(w, oauthHelper, authorizeRequester, - fosite.ErrAccessDenied.WithHint( - "Neither access token nor refresh token returned by upstream provider during password grant."), true) - } return makeDownstreamSessionAndReturnAuthcodeRedirect(r, w, oauthHelper, authorizeRequester, subject, username, groups, customSessionData) } diff --git a/internal/oidc/auth/auth_handler_test.go b/internal/oidc/auth/auth_handler_test.go index 1cb4a778..65f21c76 100644 --- a/internal/oidc/auth/auth_handler_test.go +++ b/internal/oidc/auth/auth_handler_test.go @@ -157,7 +157,7 @@ func TestAuthorizationEndpoint(t *testing.T) { fositeAccessDeniedWithMissingAccessTokenErrorQuery = map[string]string{ "error": "access_denied", - "error_description": "The resource owner or authorization server denied the request. Neither access token nor refresh token returned by upstream provider during password grant.", + "error_description": "The resource owner or authorization server denied the request. Reason: neither access token nor refresh token returned by upstream provider.", "state": happyState, } diff --git a/internal/oidc/callback/callback_handler.go b/internal/oidc/callback/callback_handler.go index 1a5a3aee..fbf13728 100644 --- a/internal/oidc/callback/callback_handler.go +++ b/internal/oidc/callback/callback_handler.go @@ -19,7 +19,6 @@ import ( "go.pinniped.dev/internal/oidc/provider" "go.pinniped.dev/internal/oidc/provider/formposthtml" "go.pinniped.dev/internal/plog" - "go.pinniped.dev/internal/psession" ) func NewHandler( @@ -69,39 +68,17 @@ func NewHandler( return httperr.New(http.StatusBadGateway, "error exchanging and validating upstream tokens") } - if token.RefreshToken == nil || token.RefreshToken.Token == "" { - plog.Warning("refresh token not returned by upstream provider during authcode exchange, "+ - "please check configuration of OIDCIdentityProvider and the client in the upstream provider's API/UI", - "upstreamName", upstreamIDPConfig.GetName(), - "scopes", upstreamIDPConfig.GetScopes(), - "additionalParams", upstreamIDPConfig.GetAdditionalAuthcodeParams()) - return httperr.New(http.StatusUnprocessableEntity, "refresh token not returned by upstream provider during authcode exchange") - } - subject, username, groups, err := downstreamsession.GetDownstreamIdentityFromUpstreamIDToken(upstreamIDPConfig, token.IDToken.Claims) if err != nil { return httperr.Wrap(http.StatusUnprocessableEntity, err.Error(), err) } - upstreamSubject, err := downstreamsession.ExtractStringClaimValue(oidc.IDTokenSubjectClaim, upstreamIDPConfig.GetName(), token.IDToken.Claims) - if err != nil { - return httperr.Wrap(http.StatusUnprocessableEntity, err.Error(), err) - } - upstreamIssuer, err := downstreamsession.ExtractStringClaimValue(oidc.IDTokenIssuerClaim, upstreamIDPConfig.GetName(), token.IDToken.Claims) + customSessionData, err := downstreamsession.MakeDownstreamOIDCCustomSessionData(upstreamIDPConfig, token) if err != nil { return httperr.Wrap(http.StatusUnprocessableEntity, err.Error(), err) } - openIDSession := downstreamsession.MakeDownstreamSession(subject, username, groups, &psession.CustomSessionData{ - ProviderUID: upstreamIDPConfig.GetResourceUID(), - ProviderName: upstreamIDPConfig.GetName(), - ProviderType: psession.ProviderTypeOIDC, - OIDC: &psession.OIDCSessionData{ - UpstreamRefreshToken: token.RefreshToken.Token, - UpstreamSubject: upstreamSubject, - UpstreamIssuer: upstreamIssuer, - }, - }) + openIDSession := downstreamsession.MakeDownstreamSession(subject, username, groups, customSessionData) authorizeResponder, err := oauthHelper.NewAuthorizeResponse(r.Context(), authorizeRequester, openIDSession) if err != nil { diff --git a/internal/oidc/callback/callback_handler_test.go b/internal/oidc/callback/callback_handler_test.go index d14c159f..784bde99 100644 --- a/internal/oidc/callback/callback_handler_test.go +++ b/internal/oidc/callback/callback_handler_test.go @@ -31,6 +31,7 @@ const ( oidcUpstreamIssuer = "https://my-upstream-issuer.com" oidcUpstreamRefreshToken = "test-refresh-token" + oidcUpstreamAccessToken = "test-access-token" oidcUpstreamSubject = "abc123-some guid" // has a space character which should get escaped in URL oidcUpstreamSubjectQueryEscaped = "abc123-some+guid" oidcUpstreamUsername = "test-pinniped-username" @@ -83,6 +84,16 @@ var ( UpstreamSubject: oidcUpstreamSubject, }, } + happyDownstreamAccessTokenCustomSessionData = &psession.CustomSessionData{ + ProviderUID: happyUpstreamIDPResourceUID, + ProviderName: happyUpstreamIDPName, + ProviderType: psession.ProviderTypeOIDC, + OIDC: &psession.OIDCSessionData{ + UpstreamAccessToken: oidcUpstreamAccessToken, + UpstreamIssuer: oidcUpstreamIssuer, + UpstreamSubject: oidcUpstreamSubject, + }, + } ) func TestCallbackEndpoint(t *testing.T) { @@ -200,6 +211,29 @@ func TestCallbackEndpoint(t *testing.T) { args: happyExchangeAndValidateTokensArgs, }, }, + { + name: "GET with authcode exchange that returns an access token but no refresh token returns 303 to downstream client callback with its state and code", + idps: oidctestutil.NewUpstreamIDPListerBuilder().WithOIDC(happyUpstream().WithEmptyRefreshToken().WithAccessToken(oidcUpstreamAccessToken).Build()), + method: http.MethodGet, + path: newRequestPath().WithState(happyState).String(), + csrfCookie: happyCSRFCookie, + wantStatus: http.StatusSeeOther, + wantRedirectLocationRegexp: happyDownstreamRedirectLocationRegexp, + wantBody: "", + wantDownstreamIDTokenSubject: oidcUpstreamIssuer + "?sub=" + oidcUpstreamSubjectQueryEscaped, + wantDownstreamIDTokenUsername: oidcUpstreamUsername, + wantDownstreamIDTokenGroups: oidcUpstreamGroupMembership, + wantDownstreamRequestedScopes: happyDownstreamScopesRequested, + wantDownstreamGrantedScopes: happyDownstreamScopesGranted, + wantDownstreamNonce: downstreamNonce, + wantDownstreamPKCEChallenge: downstreamPKCEChallenge, + wantDownstreamPKCEChallengeMethod: downstreamPKCEChallengeMethod, + wantDownstreamCustomSessionData: happyDownstreamAccessTokenCustomSessionData, + wantAuthcodeExchangeCall: &expectedAuthcodeExchange{ + performedByUpstreamName: happyUpstreamIDPName, + args: happyExchangeAndValidateTokensArgs, + }, + }, { name: "upstream IDP provides no username or group claim configuration, so we use default username claim and skip groups", idps: oidctestutil.NewUpstreamIDPListerBuilder().WithOIDC( @@ -323,28 +357,56 @@ func TestCallbackEndpoint(t *testing.T) { }, }, { - name: "return an error when upstream IDP did not return a refresh token", - idps: oidctestutil.NewUpstreamIDPListerBuilder().WithOIDC(happyUpstream().WithoutRefreshToken().Build()), + name: "return an error when upstream IDP returned no refresh token and no access token", + idps: oidctestutil.NewUpstreamIDPListerBuilder().WithOIDC(happyUpstream().WithoutRefreshToken().WithoutAccessToken().Build()), method: http.MethodGet, path: newRequestPath().WithState(happyState).String(), csrfCookie: happyCSRFCookie, wantStatus: http.StatusUnprocessableEntity, wantContentType: htmlContentType, - wantBody: "Unprocessable Entity: refresh token not returned by upstream provider during authcode exchange\n", + wantBody: "Unprocessable Entity: neither access token nor refresh token returned by upstream provider\n", wantAuthcodeExchangeCall: &expectedAuthcodeExchange{ performedByUpstreamName: happyUpstreamIDPName, args: happyExchangeAndValidateTokensArgs, }, }, { - name: "return an error when upstream IDP returned an empty refresh token", - idps: oidctestutil.NewUpstreamIDPListerBuilder().WithOIDC(happyUpstream().WithEmptyRefreshToken().Build()), + name: "return an error when upstream IDP returned an empty refresh token and empty access token", + idps: oidctestutil.NewUpstreamIDPListerBuilder().WithOIDC(happyUpstream().WithEmptyRefreshToken().WithEmptyAccessToken().Build()), method: http.MethodGet, path: newRequestPath().WithState(happyState).String(), csrfCookie: happyCSRFCookie, wantStatus: http.StatusUnprocessableEntity, wantContentType: htmlContentType, - wantBody: "Unprocessable Entity: refresh token not returned by upstream provider during authcode exchange\n", + wantBody: "Unprocessable Entity: neither access token nor refresh token returned by upstream provider\n", + wantAuthcodeExchangeCall: &expectedAuthcodeExchange{ + performedByUpstreamName: happyUpstreamIDPName, + args: happyExchangeAndValidateTokensArgs, + }, + }, + { + name: "return an error when upstream IDP returned no refresh token and empty access token", + idps: oidctestutil.NewUpstreamIDPListerBuilder().WithOIDC(happyUpstream().WithoutRefreshToken().WithEmptyAccessToken().Build()), + method: http.MethodGet, + path: newRequestPath().WithState(happyState).String(), + csrfCookie: happyCSRFCookie, + wantStatus: http.StatusUnprocessableEntity, + wantContentType: htmlContentType, + wantBody: "Unprocessable Entity: neither access token nor refresh token returned by upstream provider\n", + wantAuthcodeExchangeCall: &expectedAuthcodeExchange{ + performedByUpstreamName: happyUpstreamIDPName, + args: happyExchangeAndValidateTokensArgs, + }, + }, + { + name: "return an error when upstream IDP returned an empty refresh token and no access token", + idps: oidctestutil.NewUpstreamIDPListerBuilder().WithOIDC(happyUpstream().WithEmptyRefreshToken().WithoutAccessToken().Build()), + method: http.MethodGet, + path: newRequestPath().WithState(happyState).String(), + csrfCookie: happyCSRFCookie, + wantStatus: http.StatusUnprocessableEntity, + wantContentType: htmlContentType, + wantBody: "Unprocessable Entity: neither access token nor refresh token returned by upstream provider\n", wantAuthcodeExchangeCall: &expectedAuthcodeExchange{ performedByUpstreamName: happyUpstreamIDPName, args: happyExchangeAndValidateTokensArgs, diff --git a/internal/oidc/downstreamsession/downstream_session.go b/internal/oidc/downstreamsession/downstream_session.go index 265ed5c1..4fa90e2d 100644 --- a/internal/oidc/downstreamsession/downstream_session.go +++ b/internal/oidc/downstreamsession/downstream_session.go @@ -5,6 +5,7 @@ package downstreamsession import ( + "errors" "fmt" "net/url" "time" @@ -19,6 +20,7 @@ import ( "go.pinniped.dev/internal/oidc/provider" "go.pinniped.dev/internal/plog" "go.pinniped.dev/internal/psession" + "go.pinniped.dev/pkg/oidcclient/oidctypes" ) const ( @@ -58,6 +60,50 @@ func MakeDownstreamSession(subject string, username string, groups []string, cus return openIDSession } +func MakeDownstreamOIDCCustomSessionData(oidcUpstream provider.UpstreamOIDCIdentityProviderI, token *oidctypes.Token) (*psession.CustomSessionData, error) { + upstreamSubject, err := ExtractStringClaimValue(oidc.IDTokenSubjectClaim, oidcUpstream.GetName(), token.IDToken.Claims) + if err != nil { + return nil, err + } + upstreamIssuer, err := ExtractStringClaimValue(oidc.IDTokenIssuerClaim, oidcUpstream.GetName(), token.IDToken.Claims) + if err != nil { + return nil, err + } + + customSessionData := &psession.CustomSessionData{ + ProviderUID: oidcUpstream.GetResourceUID(), + ProviderName: oidcUpstream.GetName(), + ProviderType: psession.ProviderTypeOIDC, + OIDC: &psession.OIDCSessionData{ + UpstreamIssuer: upstreamIssuer, + UpstreamSubject: upstreamSubject, + }, + } + + hasRefreshToken := token.RefreshToken != nil && token.RefreshToken.Token != "" + hasAccessToken := token.AccessToken != nil && token.AccessToken.Token != "" + switch { + case hasRefreshToken: // we prefer refresh tokens, so check for this first + customSessionData.OIDC.UpstreamRefreshToken = token.RefreshToken.Token + case hasAccessToken: + plog.Info("refresh token not returned by upstream provider during password grant, using access token instead. "+ + "please check configuration of OIDCIdentityProvider and the client in the upstream provider's API/UI "+ + "and try to get a refresh token if possible", + "upstreamName", oidcUpstream.GetName(), + "scopes", oidcUpstream.GetScopes(), + "additionalParams", oidcUpstream.GetAdditionalAuthcodeParams()) + customSessionData.OIDC.UpstreamAccessToken = token.AccessToken.Token + default: + plog.Warning("refresh token and access token not returned by upstream provider during password grant, "+ + "please check configuration of OIDCIdentityProvider and the client in the upstream provider's API/UI", + "upstreamName", oidcUpstream.GetName(), + "scopes", oidcUpstream.GetScopes(), + "additionalParams", oidcUpstream.GetAdditionalAuthcodeParams()) + return nil, errors.New("neither access token nor refresh token returned by upstream provider") + } + return customSessionData, nil +} + // GrantScopesIfRequested auto-grants the scopes for which we do not require end-user approval, if they were requested. func GrantScopesIfRequested(authorizeRequester fosite.AuthorizeRequester) { oidc.GrantScopeIfRequested(authorizeRequester, coreosoidc.ScopeOpenID)