From d91baba240636bac09a199dcaf18e6af31fd8d2c Mon Sep 17 00:00:00 2001 From: Aram Price Date: Mon, 7 Dec 2020 17:22:34 -0800 Subject: [PATCH] authorize and callback endpoints now handle the offline_access scope - This is in preparation for the token endpoint to support the refresh grant Signed-off-by: Ryan Richard --- internal/oidc/auth/auth_handler.go | 14 ++--- internal/oidc/auth/auth_handler_test.go | 22 +++++++- internal/oidc/callback/callback_handler.go | 14 ++--- .../oidc/callback/callback_handler_test.go | 56 ++++++++++++++----- internal/oidc/nullstorage_test.go | 2 +- internal/oidc/oidc.go | 11 +++- 6 files changed, 83 insertions(+), 36 deletions(-) diff --git a/internal/oidc/auth/auth_handler.go b/internal/oidc/auth/auth_handler.go index 966bfb54..1998b72a 100644 --- a/internal/oidc/auth/auth_handler.go +++ b/internal/oidc/auth/auth_handler.go @@ -9,6 +9,7 @@ import ( "net/http" "time" + coreosoidc "github.com/coreos/go-oidc" "github.com/ory/fosite" "github.com/ory/fosite/handler/openid" "github.com/ory/fosite/token/jwt" @@ -57,7 +58,10 @@ func NewHandler( } // Grant the openid scope (for now) if they asked for it so that `NewAuthorizeResponse` will perform its OIDC validations. - grantOpenIDScopeIfRequested(authorizeRequester) + oidc.GrantScopeIfRequested(authorizeRequester, coreosoidc.ScopeOpenID) + // There don't seem to be any validations inside `NewAuthorizeResponse` related to the offline_access scope + // at this time, however we will temporarily grant the scope just in case that changes in a future release of fosite. + oidc.GrantScopeIfRequested(authorizeRequester, coreosoidc.ScopeOfflineAccess) now := time.Now() _, err = oauthHelper.NewAuthorizeResponse(r.Context(), authorizeRequester, &openid.DefaultSession{ @@ -148,14 +152,6 @@ func readCSRFCookie(r *http.Request, codec oidc.Codec) csrftoken.CSRFToken { return csrfFromCookie } -func grantOpenIDScopeIfRequested(authorizeRequester fosite.AuthorizeRequester) { - for _, scope := range authorizeRequester.GetRequestedScopes() { - if scope == "openid" { - authorizeRequester.GrantScope(scope) - } - } -} - func chooseUpstreamIDP(idpListGetter oidc.IDPListGetter) (provider.UpstreamOIDCIdentityProviderI, error) { allUpstreamIDPs := idpListGetter.GetIDPList() if len(allUpstreamIDPs) == 0 { diff --git a/internal/oidc/auth/auth_handler_test.go b/internal/oidc/auth/auth_handler_test.go index 30a8dc5d..0175c64c 100644 --- a/internal/oidc/auth/auth_handler_test.go +++ b/internal/oidc/auth/auth_handler_test.go @@ -119,7 +119,7 @@ func TestAuthorizationEndpoint(t *testing.T) { Name: "some-idp", ClientID: "some-client-id", AuthorizationURL: *upstreamAuthURL, - Scopes: []string{"scope1", "scope2"}, + Scopes: []string{"scope1", "scope2"}, // the scopes to request when starting the upstream authorization flow } // Configure fosite the same way that the production code would, using NullStorage to turn off storage. @@ -372,6 +372,26 @@ func TestAuthorizationEndpoint(t *testing.T) { wantUpstreamStateParamInLocationHeader: true, wantBodyStringWithLocationInHref: true, }, + { + name: "happy path when downstream requested scopes include offline_access", + issuer: downstreamIssuer, + idpListGetter: oidctestutil.NewIDPListGetter(&upstreamOIDCIdentityProvider), + generateCSRF: happyCSRFGenerator, + generatePKCE: happyPKCEGenerator, + generateNonce: happyNonceGenerator, + stateEncoder: happyStateEncoder, + cookieEncoder: happyCookieEncoder, + method: http.MethodGet, + path: modifiedHappyGetRequestPath(map[string]string{"scope": "openid offline_access"}), + wantStatus: http.StatusFound, + wantContentType: "text/html; charset=utf-8", + wantCSRFValueInCookieHeader: happyCSRF, + wantLocationHeader: expectedRedirectLocation(expectedUpstreamStateParam(map[string]string{ + "scope": "openid offline_access", + }, "", "")), + wantUpstreamStateParamInLocationHeader: true, + wantBodyStringWithLocationInHref: true, + }, { name: "downstream redirect uri does not match what is configured for client", issuer: downstreamIssuer, diff --git a/internal/oidc/callback/callback_handler.go b/internal/oidc/callback/callback_handler.go index e102dc62..3c85ee4c 100644 --- a/internal/oidc/callback/callback_handler.go +++ b/internal/oidc/callback/callback_handler.go @@ -11,6 +11,7 @@ import ( "net/url" "time" + coreosoidc "github.com/coreos/go-oidc" "github.com/ory/fosite" "github.com/ory/fosite/handler/openid" "github.com/ory/fosite/token/jwt" @@ -70,8 +71,9 @@ func NewHandler( return httperr.New(http.StatusBadRequest, "error using state downstream auth params") } - // Grant the openid scope only if it was requested. - grantOpenIDScopeIfRequested(authorizeRequester) + // Automatically grant the openid and offline_access scopes, but only if they were requested. + oidc.GrantScopeIfRequested(authorizeRequester, coreosoidc.ScopeOpenID) + oidc.GrantScopeIfRequested(authorizeRequester, coreosoidc.ScopeOfflineAccess) token, err := upstreamIDPConfig.ExchangeAuthcodeAndValidateTokens( r.Context(), @@ -189,14 +191,6 @@ func readState(r *http.Request, stateDecoder oidc.Decoder) (*oidc.UpstreamStateP return &state, nil } -func grantOpenIDScopeIfRequested(authorizeRequester fosite.AuthorizeRequester) { - for _, scope := range authorizeRequester.GetRequestedScopes() { - if scope == "openid" { - authorizeRequester.GrantScope(scope) - } - } -} - func getUsernameFromUpstreamIDToken( upstreamIDPConfig provider.UpstreamOIDCIdentityProviderI, idTokenClaims map[string]interface{}, diff --git a/internal/oidc/callback/callback_handler_test.go b/internal/oidc/callback/callback_handler_test.go index ed4c6b01..67857f6a 100644 --- a/internal/oidc/callback/callback_handler_test.go +++ b/internal/oidc/callback/callback_handler_test.go @@ -66,7 +66,8 @@ const ( var ( upstreamGroupMembership = []string{"test-pinniped-group-0", "test-pinniped-group-1"} - happyDownstreamScopesRequested = []string{"openid", "profile", "email"} + happyDownstreamScopesRequested = []string{"openid"} + happyDownstreamScopesGranted = []string{"openid"} happyDownstreamRequestParamsQuery = url.Values{ "response_type": []string{"code"}, @@ -127,7 +128,7 @@ func TestCallbackEndpoint(t *testing.T) { wantStatus int wantBody string wantRedirectLocationRegexp string - wantGrantedOpenidScope bool + wantDownstreamGrantedScopes []string wantDownstreamIDTokenSubject string wantDownstreamIDTokenGroups []string wantDownstreamRequestedScopes []string @@ -145,11 +146,11 @@ func TestCallbackEndpoint(t *testing.T) { csrfCookie: happyCSRFCookie, wantStatus: http.StatusFound, wantRedirectLocationRegexp: happyDownstreamRedirectLocationRegexp, - wantGrantedOpenidScope: true, wantBody: "", wantDownstreamIDTokenSubject: upstreamUsername, wantDownstreamIDTokenGroups: upstreamGroupMembership, wantDownstreamRequestedScopes: happyDownstreamScopesRequested, + wantDownstreamGrantedScopes: happyDownstreamScopesGranted, wantDownstreamNonce: downstreamNonce, wantDownstreamPKCEChallenge: downstreamPKCEChallenge, wantDownstreamPKCEChallengeMethod: downstreamPKCEChallengeMethod, @@ -163,11 +164,11 @@ func TestCallbackEndpoint(t *testing.T) { csrfCookie: happyCSRFCookie, wantStatus: http.StatusFound, wantRedirectLocationRegexp: happyDownstreamRedirectLocationRegexp, - wantGrantedOpenidScope: true, wantBody: "", wantDownstreamIDTokenSubject: upstreamIssuer + "?sub=" + upstreamSubject, wantDownstreamIDTokenGroups: nil, wantDownstreamRequestedScopes: happyDownstreamScopesRequested, + wantDownstreamGrantedScopes: happyDownstreamScopesGranted, wantDownstreamNonce: downstreamNonce, wantDownstreamPKCEChallenge: downstreamPKCEChallenge, wantDownstreamPKCEChallengeMethod: downstreamPKCEChallengeMethod, @@ -181,11 +182,11 @@ func TestCallbackEndpoint(t *testing.T) { csrfCookie: happyCSRFCookie, wantStatus: http.StatusFound, wantRedirectLocationRegexp: happyDownstreamRedirectLocationRegexp, - wantGrantedOpenidScope: true, wantBody: "", wantDownstreamIDTokenSubject: upstreamSubject, wantDownstreamIDTokenGroups: upstreamGroupMembership, wantDownstreamRequestedScopes: happyDownstreamScopesRequested, + wantDownstreamGrantedScopes: happyDownstreamScopesGranted, wantDownstreamNonce: downstreamNonce, wantDownstreamPKCEChallenge: downstreamPKCEChallenge, wantDownstreamPKCEChallengeMethod: downstreamPKCEChallengeMethod, @@ -316,6 +317,28 @@ func TestCallbackEndpoint(t *testing.T) { wantDownstreamPKCEChallengeMethod: downstreamPKCEChallengeMethod, wantExchangeAndValidateTokensCall: happyExchangeAndValidateTokensArgs, }, + { + name: "state's downstream auth params also included offline_access scope", + idp: happyUpstream().Build(), + method: http.MethodGet, + path: newRequestPath(). + WithState( + happyUpstreamStateParam(). + WithAuthorizeRequestParams(shallowCopyAndModifyQuery(happyDownstreamRequestParamsQuery, map[string]string{"scope": "openid offline_access"}).Encode()). + Build(t, happyStateCodec), + ).String(), + csrfCookie: happyCSRFCookie, + wantStatus: http.StatusFound, + wantRedirectLocationRegexp: downstreamRedirectURI + `\?code=([^&]+)&scope=openid%20offline_access&state=` + happyDownstreamState, + wantDownstreamIDTokenSubject: upstreamUsername, + wantDownstreamRequestedScopes: []string{"openid", "offline_access"}, + wantDownstreamGrantedScopes: []string{"openid", "offline_access"}, + wantDownstreamIDTokenGroups: upstreamGroupMembership, + wantDownstreamNonce: downstreamNonce, + wantDownstreamPKCEChallenge: downstreamPKCEChallenge, + wantDownstreamPKCEChallengeMethod: downstreamPKCEChallengeMethod, + wantExchangeAndValidateTokensCall: happyExchangeAndValidateTokensArgs, + }, { name: "the UpstreamOIDCProvider CRD has been deleted", idp: otherUpstreamOIDCIdentityProvider, @@ -481,7 +504,7 @@ func TestCallbackEndpoint(t *testing.T) { // Several Secrets should have been created expectedNumberOfCreatedSecrets := 2 - if test.wantGrantedOpenidScope { + if includesOpenIDScope(test.wantDownstreamGrantedScopes) { expectedNumberOfCreatedSecrets++ } require.Len(t, client.Actions(), expectedNumberOfCreatedSecrets) @@ -493,7 +516,7 @@ func TestCallbackEndpoint(t *testing.T) { t, oauthStore, authcodeDataAndSignature[1], // Authcode store key is authcode signature - test.wantGrantedOpenidScope, + test.wantDownstreamGrantedScopes, test.wantDownstreamIDTokenSubject, test.wantDownstreamIDTokenGroups, test.wantDownstreamRequestedScopes, @@ -513,7 +536,7 @@ func TestCallbackEndpoint(t *testing.T) { ) // One IDSession should have been stored, if the downstream actually requested the "openid" scope - if test.wantGrantedOpenidScope { + if includesOpenIDScope(test.wantDownstreamGrantedScopes) { testutil.RequireNumberOfSecretsMatchingLabelSelector(t, secrets, labels.Set{crud.SecretLabelKey: openidconnect.TypeLabelValue}, 1) validateIDSessionStorage( @@ -530,6 +553,15 @@ func TestCallbackEndpoint(t *testing.T) { } } +func includesOpenIDScope(scopes []string) bool { + for _, scope := range scopes { + if scope == "openid" { + return true + } + } + return false +} + type requestPath struct { code, state *string } @@ -704,7 +736,7 @@ func validateAuthcodeStorage( t *testing.T, oauthStore *oidc.KubeStorage, storeKey string, - wantGrantedOpenidScope bool, + wantDownstreamGrantedScopes []string, wantDownstreamIDTokenSubject string, wantDownstreamIDTokenGroups []string, wantDownstreamRequestedScopes []string, @@ -719,11 +751,7 @@ func validateAuthcodeStorage( storedRequestFromAuthcode, storedSessionFromAuthcode := castStoredAuthorizeRequest(t, storedAuthorizeRequestFromAuthcode) // Check which scopes were granted. - if wantGrantedOpenidScope { - require.Contains(t, storedRequestFromAuthcode.GetGrantedScopes(), "openid") - } else { - require.NotContains(t, storedRequestFromAuthcode.GetGrantedScopes(), "openid") - } + require.ElementsMatch(t, wantDownstreamGrantedScopes, storedRequestFromAuthcode.GetGrantedScopes()) // Check all the other fields of the stored request. require.NotEmpty(t, storedRequestFromAuthcode.ID) diff --git a/internal/oidc/nullstorage_test.go b/internal/oidc/nullstorage_test.go index 2983a800..d0c49b1e 100644 --- a/internal/oidc/nullstorage_test.go +++ b/internal/oidc/nullstorage_test.go @@ -28,7 +28,7 @@ func TestNullStorage_GetClient(t *testing.T) { RedirectURIs: []string{"http://127.0.0.1/callback"}, ResponseTypes: []string{"code"}, GrantTypes: []string{"authorization_code"}, - Scopes: []string{"openid", "profile", "email"}, + Scopes: []string{"openid", "offline_access", "profile", "email"}, }, TokenEndpointAuthMethod: "none", }, diff --git a/internal/oidc/oidc.go b/internal/oidc/oidc.go index 7c550751..10d633aa 100644 --- a/internal/oidc/oidc.go +++ b/internal/oidc/oidc.go @@ -7,6 +7,7 @@ package oidc import ( "time" + coreosoidc "github.com/coreos/go-oidc" "github.com/ory/fosite" "github.com/ory/fosite/compose" @@ -84,7 +85,7 @@ func PinnipedCLIOIDCClient() *fosite.DefaultOpenIDConnectClient { RedirectURIs: []string{"http://127.0.0.1/callback"}, ResponseTypes: []string{"code"}, GrantTypes: []string{"authorization_code"}, - Scopes: []string{"openid", "profile", "email"}, + Scopes: []string{coreosoidc.ScopeOpenID, coreosoidc.ScopeOfflineAccess, "profile", "email"}, }, TokenEndpointAuthMethod: "none", } @@ -156,3 +157,11 @@ func FositeErrorForLog(err error) []interface{} { type IDPListGetter interface { GetIDPList() []provider.UpstreamOIDCIdentityProviderI } + +func GrantScopeIfRequested(authorizeRequester fosite.AuthorizeRequester, scopeName string) { + for _, scope := range authorizeRequester.GetRequestedScopes() { + if scope == scopeName { + authorizeRequester.GrantScope(scope) + } + } +}