Merge pull request #1862 from tkleczek/fix-rfc-errors

Improve auth flow error handling
This commit is contained in:
Maksim Nabokikh 2021-08-03 00:34:54 +04:00 committed by GitHub
commit 3fac2ab6bc
No known key found for this signature in database
GPG Key ID: 4AEE18F83AFDEB23
4 changed files with 106 additions and 85 deletions

View File

@ -158,7 +158,7 @@ func (s *Server) handleAuthorization(w http.ResponseWriter, r *http.Request) {
return return
} }
} }
s.tokenErrHelper(w, errInvalidConnectorID, "Connector ID does not match a valid Connector", http.StatusNotFound) s.renderError(r, w, http.StatusBadRequest, "Connector ID does not match a valid Connector")
return return
} }
@ -187,21 +187,16 @@ func (s *Server) handleConnectorLogin(w http.ResponseWriter, r *http.Request) {
authReq, err := s.parseAuthorizationRequest(r) authReq, err := s.parseAuthorizationRequest(r)
if err != nil { if err != nil {
s.logger.Errorf("Failed to parse authorization request: %v", err) s.logger.Errorf("Failed to parse authorization request: %v", err)
status := http.StatusInternalServerError
// If this is an authErr, let's let it handle the error, or update the HTTP switch authErr := err.(type) {
// status code case *redirectedAuthErr:
if err, ok := err.(*authErr); ok { authErr.Handler().ServeHTTP(w, r)
if handler, ok := err.Handle(); ok { case *displayedAuthErr:
// client_id and redirect_uri checked out and we can redirect back to s.renderError(r, w, authErr.Status, err.Error())
// the client with the error. default:
handler.ServeHTTP(w, r) panic("unsupported error type")
return
}
status = err.Status()
} }
s.renderError(r, w, status, err.Error())
return return
} }
@ -770,7 +765,7 @@ func (s *Server) handleToken(w http.ResponseWriter, r *http.Request) {
case grantTypePassword: case grantTypePassword:
s.withClientFromStorage(w, r, s.handlePasswordGrant) s.withClientFromStorage(w, r, s.handlePasswordGrant)
default: default:
s.tokenErrHelper(w, errInvalidGrant, "", http.StatusBadRequest) s.tokenErrHelper(w, errUnsupportedGrantType, "", http.StatusBadRequest)
} }
} }

View File

@ -29,32 +29,35 @@ import (
// TODO(ericchiang): clean this file up and figure out more idiomatic error handling. // TODO(ericchiang): clean this file up and figure out more idiomatic error handling.
// authErr is an error response to an authorization request.
// See: https://tools.ietf.org/html/rfc6749#section-4.1.2.1 // See: https://tools.ietf.org/html/rfc6749#section-4.1.2.1
type authErr struct {
// displayedAuthErr is an error that should be displayed to the user as a web page
type displayedAuthErr struct {
Status int
Description string
}
func (err *displayedAuthErr) Error() string {
return err.Description
}
func newDisplayedErr(status int, format string, a ...interface{}) *displayedAuthErr {
return &displayedAuthErr{status, fmt.Sprintf(format, a...)}
}
// redirectedAuthErr is an error that should be reported back to the client by 302 redirect
type redirectedAuthErr struct {
State string State string
RedirectURI string RedirectURI string
Type string Type string
Description string Description string
} }
func (err *authErr) Status() int { func (err *redirectedAuthErr) Error() string {
if err.State == errServerError {
return http.StatusInternalServerError
}
return http.StatusBadRequest
}
func (err *authErr) Error() string {
return err.Description return err.Description
} }
func (err *authErr) Handle() (http.Handler, bool) { func (err *redirectedAuthErr) Handler() http.Handler {
// Didn't get a valid redirect URI.
if err.RedirectURI == "" {
return nil, false
}
hf := func(w http.ResponseWriter, r *http.Request) { hf := func(w http.ResponseWriter, r *http.Request) {
v := url.Values{} v := url.Values{}
v.Add("state", err.State) v.Add("state", err.State)
@ -70,7 +73,7 @@ func (err *authErr) Handle() (http.Handler, bool) {
} }
http.Redirect(w, r, redirectURI, http.StatusSeeOther) http.Redirect(w, r, redirectURI, http.StatusSeeOther)
} }
return http.HandlerFunc(hf), true return http.HandlerFunc(hf)
} }
func tokenErr(w http.ResponseWriter, typ, description string, statusCode int) error { func tokenErr(w http.ResponseWriter, typ, description string, statusCode int) error {
@ -102,7 +105,6 @@ const (
errUnsupportedGrantType = "unsupported_grant_type" errUnsupportedGrantType = "unsupported_grant_type"
errInvalidGrant = "invalid_grant" errInvalidGrant = "invalid_grant"
errInvalidClient = "invalid_client" errInvalidClient = "invalid_client"
errInvalidConnectorID = "invalid_connector_id"
) )
const ( const (
@ -408,12 +410,12 @@ func (s *Server) newIDToken(clientID string, claims storage.Claims, scopes []str
// parse the initial request from the OAuth2 client. // parse the initial request from the OAuth2 client.
func (s *Server) parseAuthorizationRequest(r *http.Request) (*storage.AuthRequest, error) { func (s *Server) parseAuthorizationRequest(r *http.Request) (*storage.AuthRequest, error) {
if err := r.ParseForm(); err != nil { if err := r.ParseForm(); err != nil {
return nil, &authErr{"", "", errInvalidRequest, "Failed to parse request body."} return nil, newDisplayedErr(http.StatusBadRequest, "Failed to parse request.")
} }
q := r.Form q := r.Form
redirectURI, err := url.QueryUnescape(q.Get("redirect_uri")) redirectURI, err := url.QueryUnescape(q.Get("redirect_uri"))
if err != nil { if err != nil {
return nil, &authErr{"", "", errInvalidRequest, "No redirect_uri provided."} return nil, newDisplayedErr(http.StatusBadRequest, "No redirect_uri provided.")
} }
clientID := q.Get("client_id") clientID := q.Get("client_id")
@ -434,45 +436,44 @@ func (s *Server) parseAuthorizationRequest(r *http.Request) (*storage.AuthReques
client, err := s.storage.GetClient(clientID) client, err := s.storage.GetClient(clientID)
if err != nil { if err != nil {
if err == storage.ErrNotFound { if err == storage.ErrNotFound {
description := fmt.Sprintf("Invalid client_id (%q).", clientID) return nil, newDisplayedErr(http.StatusNotFound, "Invalid client_id (%q).", clientID)
return nil, &authErr{"", "", errUnauthorizedClient, description}
} }
s.logger.Errorf("Failed to get client: %v", err) s.logger.Errorf("Failed to get client: %v", err)
return nil, &authErr{"", "", errServerError, ""} return nil, newDisplayedErr(http.StatusInternalServerError, "Database error.")
}
if connectorID != "" {
connectors, err := s.storage.ListConnectors()
if err != nil {
return nil, &authErr{"", "", errServerError, "Unable to retrieve connectors"}
}
if !validateConnectorID(connectors, connectorID) {
return nil, &authErr{"", "", errInvalidRequest, "Invalid ConnectorID"}
}
} }
if !validateRedirectURI(client, redirectURI) { if !validateRedirectURI(client, redirectURI) {
description := fmt.Sprintf("Unregistered redirect_uri (%q).", redirectURI) return nil, newDisplayedErr(http.StatusBadRequest, "Unregistered redirect_uri (%q).", redirectURI)
return nil, &authErr{"", "", errInvalidRequest, description}
} }
if redirectURI == deviceCallbackURI && client.Public { if redirectURI == deviceCallbackURI && client.Public {
redirectURI = s.issuerURL.Path + deviceCallbackURI redirectURI = s.issuerURL.Path + deviceCallbackURI
} }
// From here on out, we want to redirect back to the client with an error. // From here on out, we want to redirect back to the client with an error.
newErr := func(typ, format string, a ...interface{}) *authErr { newRedirectedErr := func(typ, format string, a ...interface{}) *redirectedAuthErr {
return &authErr{state, redirectURI, typ, fmt.Sprintf(format, a...)} return &redirectedAuthErr{state, redirectURI, typ, fmt.Sprintf(format, a...)}
}
if connectorID != "" {
connectors, err := s.storage.ListConnectors()
if err != nil {
s.logger.Errorf("Failed to list connectors: %v", err)
return nil, newRedirectedErr(errServerError, "Unable to retrieve connectors")
}
if !validateConnectorID(connectors, connectorID) {
return nil, newRedirectedErr(errInvalidRequest, "Invalid ConnectorID")
}
} }
// dex doesn't support request parameter and must return request_not_supported error // dex doesn't support request parameter and must return request_not_supported error
// https://openid.net/specs/openid-connect-core-1_0.html#6.1 // https://openid.net/specs/openid-connect-core-1_0.html#6.1
if q.Get("request") != "" { if q.Get("request") != "" {
return nil, newErr(errRequestNotSupported, "Server does not support request parameter.") return nil, newRedirectedErr(errRequestNotSupported, "Server does not support request parameter.")
} }
if codeChallengeMethod != codeChallengeMethodS256 && codeChallengeMethod != codeChallengeMethodPlain { if codeChallengeMethod != codeChallengeMethodS256 && codeChallengeMethod != codeChallengeMethodPlain {
description := fmt.Sprintf("Unsupported PKCE challenge method (%q).", codeChallengeMethod) description := fmt.Sprintf("Unsupported PKCE challenge method (%q).", codeChallengeMethod)
return nil, newErr(errInvalidRequest, description) return nil, newRedirectedErr(errInvalidRequest, description)
} }
var ( var (
@ -494,7 +495,7 @@ func (s *Server) parseAuthorizationRequest(r *http.Request) (*storage.AuthReques
isTrusted, err := s.validateCrossClientTrust(clientID, peerID) isTrusted, err := s.validateCrossClientTrust(clientID, peerID)
if err != nil { if err != nil {
return nil, newErr(errServerError, "Internal server error.") return nil, newRedirectedErr(errServerError, "Internal server error.")
} }
if !isTrusted { if !isTrusted {
invalidScopes = append(invalidScopes, scope) invalidScopes = append(invalidScopes, scope)
@ -502,13 +503,13 @@ func (s *Server) parseAuthorizationRequest(r *http.Request) (*storage.AuthReques
} }
} }
if !hasOpenIDScope { if !hasOpenIDScope {
return nil, newErr(errInvalidScope, `Missing required scope(s) ["openid"].`) return nil, newRedirectedErr(errInvalidScope, `Missing required scope(s) ["openid"].`)
} }
if len(unrecognized) > 0 { if len(unrecognized) > 0 {
return nil, newErr(errInvalidScope, "Unrecognized scope(s) %q", unrecognized) return nil, newRedirectedErr(errInvalidScope, "Unrecognized scope(s) %q", unrecognized)
} }
if len(invalidScopes) > 0 { if len(invalidScopes) > 0 {
return nil, newErr(errInvalidScope, "Client can't request scope(s) %q", invalidScopes) return nil, newRedirectedErr(errInvalidScope, "Client can't request scope(s) %q", invalidScopes)
} }
var rt struct { var rt struct {
@ -526,23 +527,23 @@ func (s *Server) parseAuthorizationRequest(r *http.Request) (*storage.AuthReques
case responseTypeToken: case responseTypeToken:
rt.token = true rt.token = true
default: default:
return nil, newErr(errInvalidRequest, "Invalid response type %q", responseType) return nil, newRedirectedErr(errInvalidRequest, "Invalid response type %q", responseType)
} }
if !s.supportedResponseTypes[responseType] { if !s.supportedResponseTypes[responseType] {
return nil, newErr(errUnsupportedResponseType, "Unsupported response type %q", responseType) return nil, newRedirectedErr(errUnsupportedResponseType, "Unsupported response type %q", responseType)
} }
} }
if len(responseTypes) == 0 { if len(responseTypes) == 0 {
return nil, newErr(errInvalidRequest, "No response_type provided") return nil, newRedirectedErr(errInvalidRequest, "No response_type provided")
} }
if rt.token && !rt.code && !rt.idToken { if rt.token && !rt.code && !rt.idToken {
// "token" can't be provided by its own. // "token" can't be provided by its own.
// //
// https://openid.net/specs/openid-connect-core-1_0.html#Authentication // https://openid.net/specs/openid-connect-core-1_0.html#Authentication
return nil, newErr(errInvalidRequest, "Response type 'token' must be provided with type 'id_token' and/or 'code'") return nil, newRedirectedErr(errInvalidRequest, "Response type 'token' must be provided with type 'id_token' and/or 'code'")
} }
if !rt.code { if !rt.code {
// Either "id_token token" or "id_token" has been provided which implies the // Either "id_token token" or "id_token" has been provided which implies the
@ -550,13 +551,13 @@ func (s *Server) parseAuthorizationRequest(r *http.Request) (*storage.AuthReques
// //
// https://openid.net/specs/openid-connect-core-1_0.html#ImplicitAuthRequest // https://openid.net/specs/openid-connect-core-1_0.html#ImplicitAuthRequest
if nonce == "" { if nonce == "" {
return nil, newErr(errInvalidRequest, "Response type 'token' requires a 'nonce' value.") return nil, newRedirectedErr(errInvalidRequest, "Response type 'token' requires a 'nonce' value.")
} }
} }
if rt.token { if rt.token {
if redirectURI == redirectURIOOB { if redirectURI == redirectURIOOB {
err := fmt.Sprintf("Cannot use response type 'token' with redirect_uri '%s'.", redirectURIOOB) err := fmt.Sprintf("Cannot use response type 'token' with redirect_uri '%s'.", redirectURIOOB)
return nil, newErr(errInvalidRequest, err) return nil, newRedirectedErr(errInvalidRequest, err)
} }
} }

View File

@ -10,7 +10,6 @@ import (
"strings" "strings"
"testing" "testing"
"github.com/stretchr/testify/require"
"gopkg.in/square/go-jose.v2" "gopkg.in/square/go-jose.v2"
"github.com/dexidp/dex/storage" "github.com/dexidp/dex/storage"
@ -27,8 +26,7 @@ func TestParseAuthorizationRequest(t *testing.T) {
queryParams map[string]string queryParams map[string]string
wantErr bool expectedError error
exactError *authErr
}{ }{
{ {
name: "normal request", name: "normal request",
@ -78,7 +76,7 @@ func TestParseAuthorizationRequest(t *testing.T) {
"response_type": "code", "response_type": "code",
"scope": "openid email profile", "scope": "openid email profile",
}, },
wantErr: true, expectedError: &displayedAuthErr{Status: http.StatusNotFound},
}, },
{ {
name: "invalid redirect uri", name: "invalid redirect uri",
@ -95,7 +93,7 @@ func TestParseAuthorizationRequest(t *testing.T) {
"response_type": "code", "response_type": "code",
"scope": "openid email profile", "scope": "openid email profile",
}, },
wantErr: true, expectedError: &displayedAuthErr{Status: http.StatusBadRequest},
}, },
{ {
name: "implicit flow", name: "implicit flow",
@ -128,7 +126,7 @@ func TestParseAuthorizationRequest(t *testing.T) {
"response_type": "code id_token", "response_type": "code id_token",
"scope": "openid email profile", "scope": "openid email profile",
}, },
wantErr: true, expectedError: &redirectedAuthErr{Type: errUnsupportedResponseType},
}, },
{ {
name: "only token response type", name: "only token response type",
@ -145,7 +143,7 @@ func TestParseAuthorizationRequest(t *testing.T) {
"response_type": "token", "response_type": "token",
"scope": "openid email profile", "scope": "openid email profile",
}, },
wantErr: true, expectedError: &redirectedAuthErr{Type: errInvalidRequest},
}, },
{ {
name: "choose connector_id", name: "choose connector_id",
@ -197,7 +195,7 @@ func TestParseAuthorizationRequest(t *testing.T) {
"response_type": "code id_token", "response_type": "code id_token",
"scope": "openid email profile", "scope": "openid email profile",
}, },
wantErr: true, expectedError: &redirectedAuthErr{Type: errInvalidRequest},
}, },
{ {
name: "PKCE code_challenge_method plain", name: "PKCE code_challenge_method plain",
@ -269,7 +267,7 @@ func TestParseAuthorizationRequest(t *testing.T) {
"code_challenge_method": "invalid_method", "code_challenge_method": "invalid_method",
"scope": "openid email profile", "scope": "openid email profile",
}, },
wantErr: true, expectedError: &redirectedAuthErr{Type: errInvalidRequest},
}, },
{ {
name: "No response type", name: "No response type",
@ -287,12 +285,7 @@ func TestParseAuthorizationRequest(t *testing.T) {
"code_challenge_method": "plain", "code_challenge_method": "plain",
"scope": "openid email profile", "scope": "openid email profile",
}, },
wantErr: true, expectedError: &redirectedAuthErr{Type: errInvalidRequest},
exactError: &authErr{
RedirectURI: "https://example.com/bar",
Type: "invalid_request",
Description: "No response_type provided",
},
}, },
} }
@ -321,13 +314,34 @@ func TestParseAuthorizationRequest(t *testing.T) {
} }
_, err := server.parseAuthorizationRequest(req) _, err := server.parseAuthorizationRequest(req)
if tc.wantErr { if tc.expectedError == nil {
require.Error(t, err) if err != nil {
if tc.exactError != nil { t.Errorf("%s: expected no error", tc.name)
require.Equal(t, tc.exactError, err)
} }
} else { } else {
require.NoError(t, err) switch expectedErr := tc.expectedError.(type) {
case *redirectedAuthErr:
e, ok := err.(*redirectedAuthErr)
if !ok {
t.Fatalf("%s: expected redirectedAuthErr error", tc.name)
}
if e.Type != expectedErr.Type {
t.Errorf("%s: expected error type %v, got %v", tc.name, expectedErr.Type, e.Type)
}
if e.RedirectURI != tc.queryParams["redirect_uri"] {
t.Errorf("%s: expected error to be returned in redirect to %v", tc.name, tc.queryParams["redirect_uri"])
}
case *displayedAuthErr:
e, ok := err.(*displayedAuthErr)
if !ok {
t.Fatalf("%s: expected displayedAuthErr error", tc.name)
}
if e.Status != expectedErr.Status {
t.Errorf("%s: expected http status %v, got %v", tc.name, expectedErr.Status, e.Status)
}
default:
t.Fatalf("%s: unsupported error type", tc.name)
}
} }
}() }()
} }

View File

@ -532,6 +532,17 @@ func makeOAuth2Tests(clientID string, clientSecret string, now func() time.Time)
return nil return nil
}, },
}, },
{
name: "unsupported grant type",
retrieveTokenOptions: []oauth2.AuthCodeOption{
oauth2.SetAuthURLParam("grant_type", "unsupported"),
},
handleToken: basicIDTokenVerify,
tokenError: ErrorResponse{
Error: errUnsupportedGrantType,
StatusCode: http.StatusBadRequest,
},
},
{ {
// This test ensures that PKCE work in "plain" mode (no code_challenge_method specified) // This test ensures that PKCE work in "plain" mode (no code_challenge_method specified)
name: "PKCE with plain", name: "PKCE with plain",
@ -678,7 +689,7 @@ func TestOAuth2CodeFlow(t *testing.T) {
tests := makeOAuth2Tests(clientID, clientSecret, now) tests := makeOAuth2Tests(clientID, clientSecret, now)
for _, tc := range tests.tests { for _, tc := range tests.tests {
func() { t.Run(tc.name, func(t *testing.T) {
ctx, cancel := context.WithCancel(context.Background()) ctx, cancel := context.WithCancel(context.Background())
defer cancel() defer cancel()
@ -812,7 +823,7 @@ func TestOAuth2CodeFlow(t *testing.T) {
if respDump, err = httputil.DumpResponse(resp, true); err != nil { if respDump, err = httputil.DumpResponse(resp, true); err != nil {
t.Fatal(err) t.Fatal(err)
} }
}() })
} }
} }