From 63b8c6e4b2779a4f7e078de2399776d6ff52b512 Mon Sep 17 00:00:00 2001
From: Andrew Keesler <akeesler@vmware.com>
Date: Thu, 19 Nov 2020 08:51:23 -0500
Subject: [PATCH] callback_handler.go: test when state missing a needed param

Signed-off-by: Andrew Keesler <akeesler@vmware.com>
---
 internal/oidc/callback/callback_handler.go    |  4 +-
 .../oidc/callback/callback_handler_test.go    | 46 +++++++++++++++++--
 2 files changed, 45 insertions(+), 5 deletions(-)

diff --git a/internal/oidc/callback/callback_handler.go b/internal/oidc/callback/callback_handler.go
index 2491f20cb..a1cc49b4e 100644
--- a/internal/oidc/callback/callback_handler.go
+++ b/internal/oidc/callback/callback_handler.go
@@ -36,14 +36,14 @@ func NewHandler(idpListGetter oidc.IDPListGetter, oauthHelper fosite.OAuth2Provi
 
 		downstreamAuthParams, err := url.ParseQuery(state.AuthParams)
 		if err != nil {
-			return httperr.New(http.StatusBadRequest, "error reading state's downstream auth params")
+			return httperr.New(http.StatusBadRequest, "error reading state downstream auth params")
 		}
 
 		// Recreate enough of the original authorize request so we can pass it to NewAuthorizeRequest().
 		reconstitutedAuthRequest := &http.Request{Form: downstreamAuthParams}
 		authorizeRequester, err := oauthHelper.NewAuthorizeRequest(r.Context(), reconstitutedAuthRequest)
 		if err != nil {
-			panic(err) // TODO
+			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
diff --git a/internal/oidc/callback/callback_handler_test.go b/internal/oidc/callback/callback_handler_test.go
index 652b87854..eb6de5751 100644
--- a/internal/oidc/callback/callback_handler_test.go
+++ b/internal/oidc/callback/callback_handler_test.go
@@ -83,7 +83,7 @@ func TestCallbackEndpoint(t *testing.T) {
 
 	happyDownstreamState := "some-downstream-state"
 
-	happyOriginalRequestParams := url.Values{
+	happyOriginalRequestParamsQuery := url.Values{
 		"response_type":         []string{"code"},
 		"scope":                 []string{"openid profile email"},
 		"client_id":             []string{"pinniped-cli"},
@@ -92,7 +92,8 @@ func TestCallbackEndpoint(t *testing.T) {
 		"code_challenge":        []string{"some-challenge"},
 		"code_challenge_method": []string{"S256"},
 		"redirect_uri":          []string{downstreamRedirectURI},
-	}.Encode()
+	}
+	happyOriginalRequestParams := happyOriginalRequestParamsQuery.Encode()
 	happyCSRF := "test-csrf"
 	happyPKCE := "test-pkce"
 	happyNonce := "test-nonce"
@@ -142,6 +143,17 @@ func TestCallbackEndpoint(t *testing.T) {
 	)
 	require.NoError(t, err)
 
+	missingClientIDState, err := happyStateCodec.Encode("s",
+		testutil.ExpectedUpstreamStateParamFormat{
+			P: shallowCopyQueryExceptFor(happyOriginalRequestParamsQuery, "client_id").Encode(),
+			N: happyNonce,
+			C: happyCSRF,
+			K: happyPKCE,
+			V: happyStateVersion,
+		},
+	)
+	require.NoError(t, err)
+
 	encodedIncomingCookieCSRFValue, err := happyCookieCodec.Encode("csrf", happyCSRF)
 	require.NoError(t, err)
 	happyCSRFCookie := "__Host-pinniped-csrf=" + encodedIncomingCookieCSRFValue
@@ -243,7 +255,16 @@ func TestCallbackEndpoint(t *testing.T) {
 			path:          newRequestPath().WithState(wrongDownstreamAuthParamsState).String(),
 			csrfCookie:    happyCSRFCookie,
 			wantStatus:    http.StatusBadRequest,
-			wantBody:      "Bad Request: error reading state's downstream auth params\n",
+			wantBody:      "Bad Request: error reading state downstream auth params\n",
+		},
+		{
+			name:          "state's downstream auth params are missing required value (e.g., client_id)",
+			idpListGetter: testutil.NewIDPListGetter(upstreamOIDCIdentityProvider),
+			method:        http.MethodGet,
+			path:          newRequestPath().WithState(missingClientIDState).String(),
+			csrfCookie:    happyCSRFCookie,
+			wantStatus:    http.StatusBadRequest,
+			wantBody:      "Bad Request: error using state downstream auth params\n",
 		},
 		{
 			name:          "the UpstreamOIDCProvider CRD has been deleted",
@@ -388,3 +409,22 @@ func (r *requestPath) String() string {
 	}
 	return path + params.Encode()
 }
+
+func shallowCopyQueryExceptFor(query url.Values, keys ...string) url.Values {
+	copied := url.Values{}
+	for key, value := range query {
+		if !contains(keys, key) {
+			copied[key] = value
+		}
+	}
+	return copied
+}
+
+func contains(haystack []string, needle string) bool {
+	for _, hay := range haystack {
+		if hay == needle {
+			return true
+		}
+	}
+	return false
+}