Merge pull request #766 from ericchiang/implicit-flow

server: fixes for the implicit and hybrid flow

server/handlers.go

@@ -144,12 +144,23 @@ func (s *Server) discoveryHandler() (http.Handler, error) {
 
 // handleAuthorization handles the OAuth2 auth endpoint.
 func (s *Server) handleAuthorization(w http.ResponseWriter, r *http.Request) {
-	authReq, err := s.parseAuthorizationRequest(s.supportedResponseTypes, r)
+	authReq, err := s.parseAuthorizationRequest(r)
 	if err != nil {
 		s.logger.Errorf("Failed to parse authorization request: %v", err)
-		s.renderError(w, http.StatusInternalServerError, "Failed to connect to the database.")
+		if handler, ok := err.Handle(); ok {
+			// client_id and redirect_uri checked out and we can redirect back to
+			// the client with the error.
+			handler.ServeHTTP(w, r)
+			return
+		}
+
+		// Otherwise render the error to the user.
+		//
+		// TODO(ericchiang): Should we just always render the error?
+		s.renderError(w, err.Status(), err.Error())
 		return
 	}
+
 	authReq.Expiry = s.now().Add(time.Minute * 30)
 	if err := s.storage.CreateAuthRequest(authReq); err != nil {
 		s.logger.Errorf("Failed to create authorization request: %v", err)
@@ -441,12 +452,25 @@ func (s *Server) sendCodeResponse(w http.ResponseWriter, r *http.Request, authRe
 		s.renderError(w, http.StatusInternalServerError, "Invalid redirect URI.")
 		return
 	}
-	q := u.Query()
+
+	var (
+		// Was the initial request using the implicit or hybrid flow instead of
+		// the "normal" code flow?
+		implicitOrHybrid = false
+
+		// Only present in hybrid or code flow. code.ID == "" if this is not set.
+		code storage.AuthCode
+
+		// ID token returned immediately if the response_type includes "id_token".
+		// Only valid for implicit and hybrid flows.
+		idToken       string
+		idTokenExpiry time.Time
+	)
 
 	for _, responseType := range authReq.ResponseTypes {
 		switch responseType {
 		case responseTypeCode:
-			code := storage.AuthCode{
+			code = storage.AuthCode{
 				ID:            storage.NewID(),
 				ClientID:      authReq.ClientID,
 				ConnectorID:   authReq.ConnectorID,
@@ -463,32 +487,73 @@ func (s *Server) sendCodeResponse(w http.ResponseWriter, r *http.Request, authRe
 				return
 			}
 
+			// Implicit and hybrid flows that try to use the OOB redirect URI are
+			// rejected earlier. If we got here we're using the code flow.
 			if authReq.RedirectURI == redirectURIOOB {
 				if err := s.templates.oob(w, code.ID); err != nil {
 					s.logger.Errorf("Server template error: %v", err)
 				}
 				return
 			}
-			q.Set("code", code.ID)
 		case responseTypeToken:
-			idToken, expiry, err := s.newIDToken(authReq.ClientID, authReq.Claims, authReq.Scopes, authReq.Nonce)
+			implicitOrHybrid = true
+		case responseTypeIDToken:
+			implicitOrHybrid = true
+			var err error
+			idToken, idTokenExpiry, err = s.newIDToken(authReq.ClientID, authReq.Claims, authReq.Scopes, authReq.Nonce)
 			if err != nil {
 				s.logger.Errorf("failed to create ID token: %v", err)
 				s.tokenErrHelper(w, errServerError, "", http.StatusInternalServerError)
 				return
 			}
-			v := url.Values{}
-			v.Set("access_token", storage.NewID())
-			v.Set("token_type", "bearer")
-			v.Set("id_token", idToken)
-			v.Set("state", authReq.State)
-			v.Set("expires_in", strconv.Itoa(int(expiry.Sub(s.now()).Seconds())))
-			u.Fragment = v.Encode()
 		}
 	}
 
-	q.Set("state", authReq.State)
-	u.RawQuery = q.Encode()
+	if implicitOrHybrid {
+		v := url.Values{}
+		v.Set("access_token", storage.NewID())
+		v.Set("token_type", "bearer")
+		v.Set("state", authReq.State)
+		if idToken != "" {
+			v.Set("id_token", idToken)
+			// The hybrid flow with only "code token" or "code id_token" doesn't return an
+			// "expires_in" value. If "code" wasn't provided, indicating the implicit flow,
+			// don't add it.
+			//
+			// https://openid.net/specs/openid-connect-core-1_0.html#HybridAuthResponse
+			if code.ID == "" {
+				v.Set("expires_in", strconv.Itoa(int(idTokenExpiry.Sub(s.now()).Seconds())))
+			}
+		}
+		if code.ID != "" {
+			v.Set("code", code.ID)
+		}
+
+		// Implicit and hybrid flows return their values as part of the fragment.
+		//
+		//   HTTP/1.1 303 See Other
+		//   Location: https://client.example.org/cb#
+		//     access_token=SlAV32hkKG
+		//     &token_type=bearer
+		//     &id_token=eyJ0 ... NiJ9.eyJ1c ... I6IjIifX0.DeWt4Qu ... ZXso
+		//     &expires_in=3600
+		//     &state=af0ifjsldkj
+		//
+		u.Fragment = v.Encode()
+	} else {
+		// The code flow add values to the URL query.
+		//
+		//   HTTP/1.1 303 See Other
+		//   Location: https://client.example.org/cb?
+		//     code=SplxlOBeZQQYbYS6WxSbIA
+		//     &state=af0ifjsldkj
+		//
+		q := u.Query()
+		q.Set("code", code.ID)
+		q.Set("state", authReq.State)
+		u.RawQuery = q.Encode()
+	}
+
 	http.Redirect(w, r, u.String(), http.StatusSeeOther)
 }
 

server/oauth2.go

@@ -24,20 +24,39 @@ type authErr struct {
 	Description string
 }
 
-func (err *authErr) ServeHTTP(w http.ResponseWriter, r *http.Request) {
-	v := url.Values{}
-	v.Add("state", err.State)
-	v.Add("error", err.Type)
-	if err.Description != "" {
-		v.Add("error_description", err.Description)
+func (err *authErr) Status() int {
+	if err.State == errServerError {
+		return http.StatusInternalServerError
 	}
-	var redirectURI string
-	if strings.Contains(err.RedirectURI, "?") {
-		redirectURI = err.RedirectURI + "&" + v.Encode()
-	} else {
-		redirectURI = err.RedirectURI + "?" + v.Encode()
+	return http.StatusBadRequest
+}
+
+func (err *authErr) Error() string {
+	return err.Description
+}
+
+func (err *authErr) Handle() (http.Handler, bool) {
+	// Didn't get a valid redirect URI.
+	if err.RedirectURI == "" {
+		return nil, false
 	}
-	http.Redirect(w, r, redirectURI, http.StatusSeeOther)
+
+	hf := func(w http.ResponseWriter, r *http.Request) {
+		v := url.Values{}
+		v.Add("state", err.State)
+		v.Add("error", err.Type)
+		if err.Description != "" {
+			v.Add("error_description", err.Description)
+		}
+		var redirectURI string
+		if strings.Contains(err.RedirectURI, "?") {
+			redirectURI = err.RedirectURI + "&" + v.Encode()
+		} else {
+			redirectURI = err.RedirectURI + "?" + v.Encode()
+		}
+		http.Redirect(w, r, redirectURI, http.StatusSeeOther)
+	}
+	return http.HandlerFunc(hf), true
 }
 
 func tokenErr(w http.ResponseWriter, typ, description string, statusCode int) error {
@@ -192,20 +211,19 @@ func (s *Server) newIDToken(clientID string, claims storage.Claims, scopes []str
 }
 
 // parse the initial request from the OAuth2 client.
-//
-// For correctness the logic is largely copied from https://github.com/RangelReale/osin.
-func (s *Server) parseAuthorizationRequest(supportedResponseTypes map[string]bool, r *http.Request) (req storage.AuthRequest, oauth2Err *authErr) {
-	if err := r.ParseForm(); err != nil {
-		return req, &authErr{"", "", errInvalidRequest, "Failed to parse request."}
-	}
-
-	redirectURI, err := url.QueryUnescape(r.Form.Get("redirect_uri"))
+func (s *Server) parseAuthorizationRequest(r *http.Request) (req storage.AuthRequest, oauth2Err *authErr) {
+	q := r.URL.Query()
+	redirectURI, err := url.QueryUnescape(q.Get("redirect_uri"))
 	if err != nil {
 		return req, &authErr{"", "", errInvalidRequest, "No redirect_uri provided."}
 	}
-	state := r.FormValue("state")
 
-	clientID := r.Form.Get("client_id")
+	clientID := q.Get("client_id")
+	state := q.Get("state")
+	nonce := q.Get("nonce")
+	// Some clients, like the old go-oidc, provide extra whitespace. Tolerate this.
+	scopes := strings.Fields(q.Get("scope"))
+	responseTypes := strings.Fields(q.Get("response_type"))
 
 	client, err := s.storage.GetClient(clientID)
 	if err != nil {
@@ -222,12 +240,11 @@ func (s *Server) parseAuthorizationRequest(supportedResponseTypes map[string]boo
 		return req, &authErr{"", "", errInvalidRequest, description}
 	}
 
+	// From here on out, we want to redirect back to the client with an error.
 	newErr := func(typ, format string, a ...interface{}) *authErr {
 		return &authErr{state, redirectURI, typ, fmt.Sprintf(format, a...)}
 	}
 
-	scopes := strings.Fields(r.Form.Get("scope"))
-
 	var (
 		unrecognized  []string
 		invalidScopes []string
@@ -247,7 +264,7 @@ func (s *Server) parseAuthorizationRequest(supportedResponseTypes map[string]boo
 
 			isTrusted, err := s.validateCrossClientTrust(clientID, peerID)
 			if err != nil {
-				return req, newErr(errServerError, "")
+				return req, newErr(errServerError, "Internal server error.")
 			}
 			if !isTrusted {
 				invalidScopes = append(invalidScopes, scope)
@@ -264,37 +281,61 @@ func (s *Server) parseAuthorizationRequest(supportedResponseTypes map[string]boo
 		return req, newErr("invalid_scope", "Client can't request scope(s) %q", invalidScopes)
 	}
 
-	nonce := r.Form.Get("nonce")
-	responseTypes := strings.Split(r.Form.Get("response_type"), " ")
+	var rt struct {
+		code    bool
+		idToken bool
+		token   bool
+	}
+
 	for _, responseType := range responseTypes {
-		if !supportedResponseTypes[responseType] {
+		switch responseType {
+		case responseTypeCode:
+			rt.code = true
+		case responseTypeIDToken:
+			rt.idToken = true
+		case responseTypeToken:
+			rt.token = true
+		default:
 			return req, newErr("invalid_request", "Invalid response type %q", responseType)
 		}
 
-		switch responseType {
-		case responseTypeCode:
-		case responseTypeToken:
-			// Implicit flow requires a nonce value.
-			// https://openid.net/specs/openid-connect-core-1_0.html#ImplicitAuthRequest
-			if nonce == "" {
-				return req, newErr("invalid_request", "Response type 'token' requires a 'nonce' value.")
-			}
+		if !s.supportedResponseTypes[responseType] {
+			return req, newErr(errUnsupportedResponseType, "Unsupported response type %q", responseType)
+		}
+	}
 
-			if redirectURI == redirectURIOOB {
-				err := fmt.Sprintf("Cannot use response type 'token' with redirect_uri '%s'.", redirectURIOOB)
-				return req, newErr("invalid_request", err)
-			}
-		default:
-			return req, newErr("invalid_request", "Invalid response type %q", responseType)
+	if len(responseTypes) == 0 {
+		return req, newErr("invalid_requests", "No response_type provided")
+	}
+
+	if rt.token && !rt.code && !rt.idToken {
+		// "token" can't be provided by its own.
+		//
+		// https://openid.net/specs/openid-connect-core-1_0.html#Authentication
+		return req, newErr("invalid_request", "Response type 'token' must be provided with type 'id_token' and/or 'code'")
+	}
+	if !rt.code {
+		// Either "id_token code" or "id_token" has been provided which implies the
+		// implicit flow. Implicit flow requires a nonce value.
+		//
+		// https://openid.net/specs/openid-connect-core-1_0.html#ImplicitAuthRequest
+		if nonce == "" {
+			return req, newErr("invalid_request", "Response type 'token' requires a 'nonce' value.")
+		}
+	}
+	if rt.token {
+		if redirectURI == redirectURIOOB {
+			err := fmt.Sprintf("Cannot use response type 'token' with redirect_uri '%s'.", redirectURIOOB)
+			return req, newErr("invalid_request", err)
 		}
 	}
 
 	return storage.AuthRequest{
 		ID:                  storage.NewID(),
 		ClientID:            client.ID,
-		State:               r.Form.Get("state"),
+		State:               state,
 		Nonce:               nonce,
-		ForceApprovalPrompt: r.Form.Get("approval_prompt") == "force",
+		ForceApprovalPrompt: q.Get("approval_prompt") == "force",
 		Scopes:              scopes,
 		RedirectURI:         redirectURI,
 		ResponseTypes:       responseTypes,

server/oauth2_test.go

@@ -1 +1,150 @@
 package server
+
+import (
+	"context"
+	"net/http/httptest"
+	"net/url"
+	"testing"
+
+	"github.com/coreos/dex/storage"
+)
+
+func TestParseAuthorizationRequest(t *testing.T) {
+	tests := []struct {
+		name                   string
+		clients                []storage.Client
+		supportedResponseTypes []string
+
+		queryParams map[string]string
+
+		wantErr bool
+	}{
+		{
+			name: "normal request",
+			clients: []storage.Client{
+				{
+					ID:           "foo",
+					RedirectURIs: []string{"https://example.com/foo"},
+				},
+			},
+			supportedResponseTypes: []string{"code"},
+			queryParams: map[string]string{
+				"client_id":     "foo",
+				"redirect_uri":  "https://example.com/foo",
+				"response_type": "code",
+				"scope":         "openid email profile",
+			},
+		},
+		{
+			name: "invalid client id",
+			clients: []storage.Client{
+				{
+					ID:           "foo",
+					RedirectURIs: []string{"https://example.com/foo"},
+				},
+			},
+			supportedResponseTypes: []string{"code"},
+			queryParams: map[string]string{
+				"client_id":     "bar",
+				"redirect_uri":  "https://example.com/foo",
+				"response_type": "code",
+				"scope":         "openid email profile",
+			},
+			wantErr: true,
+		},
+		{
+			name: "invalid redirect uri",
+			clients: []storage.Client{
+				{
+					ID:           "bar",
+					RedirectURIs: []string{"https://example.com/bar"},
+				},
+			},
+			supportedResponseTypes: []string{"code"},
+			queryParams: map[string]string{
+				"client_id":     "bar",
+				"redirect_uri":  "https://example.com/foo",
+				"response_type": "code",
+				"scope":         "openid email profile",
+			},
+			wantErr: true,
+		},
+		{
+			name: "implicit flow",
+			clients: []storage.Client{
+				{
+					ID:           "bar",
+					RedirectURIs: []string{"https://example.com/bar"},
+				},
+			},
+			supportedResponseTypes: []string{"code", "id_token", "token"},
+			queryParams: map[string]string{
+				"client_id":     "bar",
+				"redirect_uri":  "https://example.com/bar",
+				"response_type": "code id_token",
+				"scope":         "openid email profile",
+			},
+		},
+		{
+			name: "unsupported response type",
+			clients: []storage.Client{
+				{
+					ID:           "bar",
+					RedirectURIs: []string{"https://example.com/bar"},
+				},
+			},
+			supportedResponseTypes: []string{"code"},
+			queryParams: map[string]string{
+				"client_id":     "bar",
+				"redirect_uri":  "https://example.com/bar",
+				"response_type": "code id_token",
+				"scope":         "openid email profile",
+			},
+			wantErr: true,
+		},
+		{
+			name: "only token response type",
+			clients: []storage.Client{
+				{
+					ID:           "bar",
+					RedirectURIs: []string{"https://example.com/bar"},
+				},
+			},
+			supportedResponseTypes: []string{"code", "id_token", "token"},
+			queryParams: map[string]string{
+				"client_id":     "bar",
+				"redirect_uri":  "https://example.com/bar",
+				"response_type": "token",
+				"scope":         "openid email profile",
+			},
+			wantErr: true,
+		},
+	}
+
+	for _, tc := range tests {
+		func() {
+			ctx, cancel := context.WithCancel(context.Background())
+			defer cancel()
+
+			httpServer, server := newTestServer(ctx, t, func(c *Config) {
+				c.SupportedResponseTypes = tc.supportedResponseTypes
+				c.Storage = storage.WithStaticClients(c.Storage, tc.clients)
+			})
+			defer httpServer.Close()
+
+			params := url.Values{}
+			for k, v := range tc.queryParams {
+				params.Set(k, v)
+			}
+
+			req := httptest.NewRequest("GET", httpServer.URL+"/auth?"+params.Encode(), nil)
+			_, err := server.parseAuthorizationRequest(req)
+			if err != nil && !tc.wantErr {
+				t.Errorf("%s: %v", tc.name, err)
+			}
+			if err == nil && tc.wantErr {
+				t.Errorf("%s: expected error", tc.name)
+			}
+		}()
+	}
+}

server/rotation.go

@@ -130,6 +130,7 @@ func (k keyRotater) rotate() error {
 
 		// Remove expired verification keys.
 		i := 0
+
 		for _, key := range keys.VerificationKeys {
 			if !key.Expiry.After(tNow) {
 				keys.VerificationKeys[i] = key

server/server.go

@@ -159,7 +159,7 @@ func newServer(ctx context.Context, c Config, rotationStrategy rotationStrategy)
 	supported := make(map[string]bool)
 	for _, respType := range c.SupportedResponseTypes {
 		switch respType {
-		case responseTypeCode, responseTypeToken:
+		case responseTypeCode, responseTypeIDToken, responseTypeToken:
 		default:
 			return nil, fmt.Errorf("unsupported response_type %q", respType)
 		}

server/server_test.go

@@ -510,7 +510,7 @@ func TestOAuth2ImplicitFlow(t *testing.T) {
 
 	httpServer, s := newTestServer(ctx, t, func(c *Config) {
 		// Enable support for the implicit flow.
-		c.SupportedResponseTypes = []string{"code", "token"}
+		c.SupportedResponseTypes = []string{"code", "token", "id_token"}
 	})
 	defer httpServer.Close()
 
@@ -553,7 +553,7 @@ func TestOAuth2ImplicitFlow(t *testing.T) {
 			w.WriteHeader(http.StatusOK)
 			return
 		}
-		u := oauth2Config.AuthCodeURL(state, oauth2.SetAuthURLParam("response_type", "token"), oidc.Nonce(nonce))
+		u := oauth2Config.AuthCodeURL(state, oauth2.SetAuthURLParam("response_type", "id_token token"), oidc.Nonce(nonce))
 		http.Redirect(w, r, u, http.StatusSeeOther)
 	}))