From ecd23e86cea1d7dbcd69f8626767689e94eb9afa Mon Sep 17 00:00:00 2001 From: Ryan Richard Date: Thu, 21 Nov 2024 13:01:32 -0800 Subject: [PATCH] callback endpoint renders more useful user-facing error messages Co-authored-by: Joshua Casey --- .../endpoints/callback/callback_handler.go | 40 ++++++++++++- .../callback/callback_handler_test.go | 56 ++++++++++++++++--- 2 files changed, 86 insertions(+), 10 deletions(-) diff --git a/internal/federationdomain/endpoints/callback/callback_handler.go b/internal/federationdomain/endpoints/callback/callback_handler.go index 4cd330505..17984ee86 100644 --- a/internal/federationdomain/endpoints/callback/callback_handler.go +++ b/internal/federationdomain/endpoints/callback/callback_handler.go @@ -7,9 +7,11 @@ package callback import ( "net/http" "net/url" + "strings" "github.com/ory/fosite" "k8s.io/apimachinery/pkg/util/sets" + "k8s.io/apiserver/pkg/audit" "go.pinniped.dev/internal/auditevent" "go.pinniped.dev/internal/federationdomain/downstreamsession" @@ -152,9 +154,43 @@ func validateRequest(r *http.Request, stateDecoder, cookieDecoder oidc.Decoder) if authcode(r) == "" { plog.Info("code param not found") - return "", nil, httperr.New(http.StatusBadRequest, - "code param not found: check URL in browser's address bar for error parameters from upstream identity provider") + return "", nil, httperr.New(http.StatusBadRequest, errorMsgForNoCodeParam(r)) } return encodedState, decodedState, nil } + +func errorMsgForNoCodeParam(r *http.Request) string { + msg := strings.Builder{} + + msg.WriteString("code param not found\n\n") + + errorParam, hasError := r.Form["error"] + errorDescParam, hasErrorDesc := r.Form["error_description"] + errorURIParam, hasErrorURI := r.Form["error_uri"] + + if hasError { + msg.WriteString("error from external identity provider: ") + msg.WriteString(errorParam[0]) + msg.WriteByte('\n') + } + if hasErrorDesc { + msg.WriteString("error_description from external identity provider: ") + msg.WriteString(errorDescParam[0]) + msg.WriteByte('\n') + } + if hasErrorURI { + msg.WriteString("error_uri from external identity provider: ") + msg.WriteString(errorURIParam[0]) + msg.WriteByte('\n') + } + if !hasError && !hasErrorDesc && !hasErrorURI { + msg.WriteString("Something went wrong with your authentication attempt at your external identity provider.\n") + } + + msg.WriteByte('\n') + msg.WriteString("Pinniped AuditID: ") + msg.WriteString(audit.GetAuditIDTruncated(r.Context())) + + return msg.String() +} diff --git a/internal/federationdomain/endpoints/callback/callback_handler_test.go b/internal/federationdomain/endpoints/callback/callback_handler_test.go index 6dd419553..c05770731 100644 --- a/internal/federationdomain/endpoints/callback/callback_handler_test.go +++ b/internal/federationdomain/endpoints/callback/callback_handler_test.go @@ -29,6 +29,7 @@ import ( "go.pinniped.dev/internal/federationdomain/stateparam" "go.pinniped.dev/internal/federationdomain/storage" "go.pinniped.dev/internal/federationdomain/upstreamprovider" + "go.pinniped.dev/internal/here" "go.pinniped.dev/internal/plog" "go.pinniped.dev/internal/psession" "go.pinniped.dev/internal/testutil" @@ -1180,36 +1181,75 @@ func TestCallbackEndpoint(t *testing.T) { }, }, { - name: "error redirect from upstream IDP audit logs the error params from the OAuth2 spec", + name: "error redirect from upstream IDP audit logs all the error params from the OAuth2 spec", idps: testidplister.NewUpstreamIDPListerBuilder().WithOIDC(happyOIDCUpstream().Build()), method: http.MethodGet, - path: newRequestPath().WithState(happyOIDCState).WithoutCode().String() + "&error=some_error&error_description=some_description&error_uri=some_uri", + path: newRequestPath().WithState(happyOIDCState).WithoutCode().String() + "&error=some%20error&error_description=some%20description&error_uri=some%20uri", csrfCookie: happyCSRFCookie, wantStatus: http.StatusBadRequest, wantContentType: htmlContentType, - wantBody: "Bad Request: code param not found: check URL in browser's address bar for error parameters from upstream identity provider\n", + wantBody: here.Doc(`Bad Request: code param not found + + error from external identity provider: some error + error_description from external identity provider: some description + error_uri from external identity provider: some uri + + Pinniped AuditID: fake-audit-id + `), wantAuditLogs: func(encodedStateParam stateparam.Encoded, sessionID string) []testutil.WantedAuditLog { return []testutil.WantedAuditLog{ testutil.WantAuditLog("HTTP Request Parameters", map[string]any{ "params": map[string]any{ "state": "redacted", - "error": "some_error", - "error_description": "some_description", - "error_uri": "some_uri", + "error": "some error", + "error_description": "some description", + "error_uri": "some uri", }, }), } }, }, { - name: "code param was not included on request", + name: "error redirect from upstream IDP when only some of the error params from the OAuth2 spec are included on the URL", + idps: testidplister.NewUpstreamIDPListerBuilder().WithOIDC(happyOIDCUpstream().Build()), + method: http.MethodGet, + path: newRequestPath().WithState(happyOIDCState).WithoutCode().String() + "&error=some%20error&error_description=some%20description", + csrfCookie: happyCSRFCookie, + wantStatus: http.StatusBadRequest, + wantContentType: htmlContentType, + wantBody: here.Doc(`Bad Request: code param not found + + error from external identity provider: some error + error_description from external identity provider: some description + + Pinniped AuditID: fake-audit-id + `), + wantAuditLogs: func(encodedStateParam stateparam.Encoded, sessionID string) []testutil.WantedAuditLog { + return []testutil.WantedAuditLog{ + testutil.WantAuditLog("HTTP Request Parameters", map[string]any{ + "params": map[string]any{ + "state": "redacted", + "error": "some error", + "error_description": "some description", + }, + }), + } + }, + }, + { + name: "code param was not included on request and there is no error param", idps: testidplister.NewUpstreamIDPListerBuilder().WithOIDC(happyOIDCUpstream().Build()), method: http.MethodGet, path: newRequestPath().WithState(happyOIDCState).WithoutCode().String(), csrfCookie: happyCSRFCookie, wantStatus: http.StatusBadRequest, wantContentType: htmlContentType, - wantBody: "Bad Request: code param not found: check URL in browser's address bar for error parameters from upstream identity provider\n", + wantBody: here.Doc(`Bad Request: code param not found + + Something went wrong with your authentication attempt at your external identity provider. + + Pinniped AuditID: fake-audit-id + `), wantAuditLogs: func(encodedStateParam stateparam.Encoded, sessionID string) []testutil.WantedAuditLog { return []testutil.WantedAuditLog{ testutil.WantAuditLog("HTTP Request Parameters", map[string]any{