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 <richardry@vmware.com>

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)
 }

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,
 		}
 

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)
+		customSessionData, err := downstreamsession.MakeDownstreamOIDCCustomSessionData(upstreamIDPConfig, token)
-		if err != nil {
-			return httperr.Wrap(http.StatusUnprocessableEntity, err.Error(), err)
-		}
-		upstreamIssuer, err := downstreamsession.ExtractStringClaimValue(oidc.IDTokenIssuerClaim, upstreamIDPConfig.GetName(), token.IDToken.Claims)
 		if err != nil {
 			return httperr.Wrap(http.StatusUnprocessableEntity, err.Error(), err)
 		}
 
-		openIDSession := downstreamsession.MakeDownstreamSession(subject, username, groups, &psession.CustomSessionData{
+		openIDSession := downstreamsession.MakeDownstreamSession(subject, username, groups, customSessionData)
-			ProviderUID:  upstreamIDPConfig.GetResourceUID(),
-			ProviderName: upstreamIDPConfig.GetName(),
-			ProviderType: psession.ProviderTypeOIDC,
-			OIDC: &psession.OIDCSessionData{
-				UpstreamRefreshToken: token.RefreshToken.Token,
-				UpstreamSubject:      upstreamSubject,
-				UpstreamIssuer:       upstreamIssuer,
-			},
-		})
 
 		authorizeResponder, err := oauthHelper.NewAuthorizeResponse(r.Context(), authorizeRequester, openIDSession)
 		if err != nil {

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",
+			name:            "return an error when upstream IDP returned no refresh token and no access token",
-			idps:            oidctestutil.NewUpstreamIDPListerBuilder().WithOIDC(happyUpstream().WithoutRefreshToken().Build()),
+			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",
+			name:            "return an error when upstream IDP returned an empty refresh token and empty access token",
-			idps:            oidctestutil.NewUpstreamIDPListerBuilder().WithOIDC(happyUpstream().WithEmptyRefreshToken().Build()),
+			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,

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)