diff --git a/pkg/oidcclient/login.go b/pkg/oidcclient/login.go index 641aac81..f79b192f 100644 --- a/pkg/oidcclient/login.go +++ b/pkg/oidcclient/login.go @@ -423,9 +423,15 @@ func (h *handlerState) cliBasedAuth(authorizeOptions *[]oauth2.AuthCodeOption) ( return nil, fmt.Errorf("error getting authorization: redirected to the wrong location: %s", rawLocation) } + // Validate OAuth2 state and fail if it's incorrect (to block CSRF). + if err := h.state.Validate(location.Query().Get("state")); err != nil { + return nil, fmt.Errorf("missing or invalid state parameter in authorization response: %s", rawLocation) + } + // Get the auth code or return the error from the server. authCode := location.Query().Get("code") if authCode == "" { + // Check for error response parameters. See https://openid.net/specs/openid-connect-core-1_0.html#AuthError. requiredErrorCode := location.Query().Get("error") optionalErrorDescription := location.Query().Get("error_description") if optionalErrorDescription == "" { @@ -434,11 +440,6 @@ func (h *handlerState) cliBasedAuth(authorizeOptions *[]oauth2.AuthCodeOption) ( return nil, fmt.Errorf("login failed with code %q: %s", requiredErrorCode, optionalErrorDescription) } - // Validate OAuth2 state and fail if it's incorrect (to block CSRF). - if err := h.state.Validate(location.Query().Get("state")); err != nil { - return nil, fmt.Errorf("missing or invalid state parameter in authorization response: %s", rawLocation) - } - // Exchange the authorization code for access, ID, and refresh tokens and perform required // validations on the returned ID token. tokenCtx, tokenCtxCancelFunc := context.WithTimeout(context.Background(), httpRequestTimeout) @@ -655,10 +656,11 @@ func (h *handlerState) handleAuthCodeCallback(w http.ResponseWriter, r *http.Req return httperr.New(http.StatusForbidden, "missing or invalid state parameter") } - // Check for error response parameters. + // Check for error response parameters. See https://openid.net/specs/openid-connect-core-1_0.html#AuthError. if errorParam := params.Get("error"); errorParam != "" { - // TODO This should also show the value of the optional "error_description" param if it exists. - // See https://openid.net/specs/openid-connect-core-1_0.html#AuthError + if errorDescParam := params.Get("error_description"); errorDescParam != "" { + return httperr.Newf(http.StatusBadRequest, "login failed with code %q: %s", errorParam, errorDescParam) + } return httperr.Newf(http.StatusBadRequest, "login failed with code %q", errorParam) } diff --git a/pkg/oidcclient/login_test.go b/pkg/oidcclient/login_test.go index e32251c4..eb9b5147 100644 --- a/pkg/oidcclient/login_test.go +++ b/pkg/oidcclient/login_test.go @@ -724,7 +724,7 @@ func TestLogin(t *testing.T) { // nolint:gocyclo return defaultLDAPTestOpts(t, h, &http.Response{ StatusCode: http.StatusFound, Header: http.Header{"Location": []string{ - "http://127.0.0.1:0/callback?error=access_denied&error_description=optional-error-description", + "http://127.0.0.1:0/callback?error=access_denied&error_description=optional-error-description&state=test-state", }}, }, nil) } @@ -756,7 +756,7 @@ func TestLogin(t *testing.T) { // nolint:gocyclo return defaultLDAPTestOpts(t, h, &http.Response{ StatusCode: http.StatusFound, Header: http.Header{"Location": []string{ - "http://127.0.0.1:0/callback?error=access_denied", + "http://127.0.0.1:0/callback?error=access_denied&state=test-state", }}, }, nil) } @@ -1279,6 +1279,12 @@ func TestHandleAuthCodeCallback(t *testing.T) { wantErr: `login failed with code "some_error"`, wantHTTPStatus: http.StatusBadRequest, }, + { + name: "error code with a description from provider", + query: "state=test-state&error=some_error&error_description=optional%20error%20description", + wantErr: `login failed with code "some_error": optional error description`, + wantHTTPStatus: http.StatusBadRequest, + }, { name: "invalid code", query: "state=test-state&code=invalid",