diff --git a/internal/oidc/auth/auth_handler_test.go b/internal/oidc/auth/auth_handler_test.go index 8d744868..1ac7cb87 100644 --- a/internal/oidc/auth/auth_handler_test.go +++ b/internal/oidc/auth/auth_handler_test.go @@ -711,6 +711,8 @@ func TestAuthorizationEndpoint(t *testing.T) { } rsp := httptest.NewRecorder() subject.ServeHTTP(rsp, req) + t.Logf("response: %#v", rsp) + t.Logf("response body: %q", rsp.Body.String()) require.Equal(t, test.wantStatus, rsp.Code) requireEqualContentType(t, rsp.Header().Get("Content-Type"), test.wantContentType) diff --git a/internal/oidc/callback/callback_handler.go b/internal/oidc/callback/callback_handler.go index 7d1f555c..4208a746 100644 --- a/internal/oidc/callback/callback_handler.go +++ b/internal/oidc/callback/callback_handler.go @@ -46,8 +46,8 @@ func NewHandler(idpListGetter oidc.IDPListGetter, oauthHelper fosite.OAuth2Provi return httperr.New(http.StatusBadRequest, "error using state downstream auth params") } - // TODO: grant the openid scope only if it was requested, similar to what we did in auth_handler.go - authorizeRequester.GrantScope("openid") + // Grant the openid scope only if it was requested. + grantOpenIDScopeIfRequested(authorizeRequester) _, idTokenClaims, err := upstreamIDPConfig.ExchangeAuthcodeAndValidateTokens( r.Context(), @@ -168,3 +168,11 @@ 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) + } + } +} diff --git a/internal/oidc/callback/callback_handler_test.go b/internal/oidc/callback/callback_handler_test.go index afa88c4b..4b47e96b 100644 --- a/internal/oidc/callback/callback_handler_test.go +++ b/internal/oidc/callback/callback_handler_test.go @@ -37,17 +37,6 @@ func TestCallbackEndpoint(t *testing.T) { downstreamRedirectURI = "http://127.0.0.1/callback" ) - // Configure fosite the same way that the production code would, except use in-memory storage. - oauthStore := &storage.MemoryStore{ - Clients: map[string]fosite.Client{oidc.PinnipedCLIOIDCClient().ID: oidc.PinnipedCLIOIDCClient()}, - AuthorizeCodes: map[string]storage.StoreAuthorizeCode{}, - PKCES: map[string]fosite.Requester{}, - IDSessions: map[string]fosite.Requester{}, - } - hmacSecret := []byte("some secret - must have at least 32 bytes") - require.GreaterOrEqual(t, len(hmacSecret), 32, "fosite requires that hmac secrets have at least 32 bytes") - oauthHelper := oidc.FositeOauth2Helper(oauthStore, hmacSecret) - upstreamOIDCIdentityProvider := testutil.TestUpstreamOIDCIdentityProvider{ Name: happyUpstreamIDPName, ClientID: "some-client-id", @@ -157,7 +146,18 @@ func TestCallbackEndpoint(t *testing.T) { missingClientIDState, err := happyStateCodec.Encode("s", testutil.ExpectedUpstreamStateParamFormat{ - P: shallowCopyQueryExceptFor(happyOriginalRequestParamsQuery, "client_id").Encode(), + P: shallowCopyAndModifyQuery(happyOriginalRequestParamsQuery, map[string]string{"client_id": ""}).Encode(), + N: happyNonce, + C: happyCSRF, + K: happyPKCE, + V: happyStateVersion, + }, + ) + require.NoError(t, err) + + noOpenidScopeState, err := happyStateCodec.Encode("s", + testutil.ExpectedUpstreamStateParamFormat{ + P: shallowCopyAndModifyQuery(happyOriginalRequestParamsQuery, map[string]string{"scope": "profile email"}).Encode(), N: happyNonce, C: happyCSRF, K: happyPKCE, @@ -181,7 +181,9 @@ func TestCallbackEndpoint(t *testing.T) { wantStatus int wantBody string wantRedirectLocationRegexp string - wantAuthcodeStored bool + // TODO: I am unused... + wantAuthcodeStored bool + wantGrantedOpenidScope bool }{ { name: "GET with good state and cookie and successful upstream token exchange returns 302 to downstream client callback with its state and code", @@ -193,6 +195,7 @@ func TestCallbackEndpoint(t *testing.T) { // Note that fosite puts the granted scopes as a param in the redirect URI even though the spec doesn't seem to require it wantRedirectLocationRegexp: downstreamRedirectURI + `\?code=([^&]+)&scope=openid&state=` + happyDownstreamState, wantAuthcodeStored: true, + wantGrantedOpenidScope: true, wantBody: "", }, // TODO: when we call the callback twice in a row, we get two different auth codes (to prove we are using an RNG for auth codes) @@ -278,6 +281,15 @@ func TestCallbackEndpoint(t *testing.T) { wantStatus: http.StatusBadRequest, wantBody: "Bad Request: error using state downstream auth params\n", }, + { + name: "state's downstream auth params does not contain openid scope", + idpListGetter: testutil.NewIDPListGetter(upstreamOIDCIdentityProvider), + method: http.MethodGet, + path: newRequestPath().WithState(noOpenidScopeState).String(), + csrfCookie: happyCSRFCookie, + wantStatus: http.StatusFound, + wantRedirectLocationRegexp: downstreamRedirectURI + `\?code=([^&]+)&scope=&state=` + happyDownstreamState, + }, { name: "the UpstreamOIDCProvider CRD has been deleted", idpListGetter: testutil.NewIDPListGetter(otherUpstreamOIDCIdentityProvider), @@ -328,6 +340,18 @@ func TestCallbackEndpoint(t *testing.T) { for _, test := range tests { test := test t.Run(test.name, func(t *testing.T) { + // Configure fosite the same way that the production code would, except use in-memory storage. + // Inject this into our test subject at the last second so we get a fresh storage for every test. + oauthStore := &storage.MemoryStore{ + Clients: map[string]fosite.Client{oidc.PinnipedCLIOIDCClient().ID: oidc.PinnipedCLIOIDCClient()}, + AuthorizeCodes: map[string]storage.StoreAuthorizeCode{}, + PKCES: map[string]fosite.Requester{}, + IDSessions: map[string]fosite.Requester{}, + } + hmacSecret := []byte("some secret - must have at least 32 bytes") + require.GreaterOrEqual(t, len(hmacSecret), 32, "fosite requires that hmac secrets have at least 32 bytes") + oauthHelper := oidc.FositeOauth2Helper(oauthStore, hmacSecret) + subject := NewHandler(test.idpListGetter, oauthHelper, happyStateCodec, happyCookieCodec) req := httptest.NewRequest(test.method, test.path, nil) if test.csrfCookie != "" { @@ -335,6 +359,8 @@ func TestCallbackEndpoint(t *testing.T) { } rsp := httptest.NewRecorder() subject.ServeHTTP(rsp, req) + t.Logf("response: %#v", rsp) + t.Logf("response body: %q", rsp.Body.String()) require.Equal(t, test.wantStatus, rsp.Code) @@ -367,12 +393,17 @@ func TestCallbackEndpoint(t *testing.T) { require.NoError(t, err) // Check that storage returned the expected concrete data types. - _, ok := storedAuthorizeRequest.(*fosite.Request) + storedRequest, ok := storedAuthorizeRequest.(*fosite.Request) require.True(t, ok) storedSession, ok := storedAuthorizeRequest.GetSession().(*openid.DefaultSession) require.True(t, ok) // Check various fields of the stored data. + if test.wantGrantedOpenidScope { + require.Contains(t, storedRequest.GetGrantedScopes(), "openid") + } else { + require.NotContains(t, storedRequest.GetGrantedScopes(), "openid") + } require.Equal(t, "test-pinniped-username", storedSession.Claims.Subject) } else { require.Empty(t, rsp.Header().Values("Location")) @@ -433,21 +464,16 @@ func (r *requestPath) String() string { return path + params.Encode() } -func shallowCopyQueryExceptFor(query url.Values, keys ...string) url.Values { +func shallowCopyAndModifyQuery(query url.Values, modifications map[string]string) url.Values { copied := url.Values{} for key, value := range query { - if !contains(keys, key) { + if modification, ok := modifications[key]; ok { + if modification != "" { + copied[key] = []string{modification} + } + } else { copied[key] = value } } return copied } - -func contains(haystack []string, needle string) bool { - for _, hay := range haystack { - if hay == needle { - return true - } - } - return false -}