Repository: SkygearIO/skygear-server (go)

Issue:
OAuth response mode
Client may use XHR to interact with Auth UI. Custom URL scheme redirect does not work with XHR. Therefore, we need an extra endpoint to use HTTP 302 redirect. That endpoint can also support `response_mode=form_post`.

Pull request:
OAuth response mode
fix #1503 


Patch:
diff --git a/pkg/auth/dependency/oauth/handler/handler_authz.go b/pkg/auth/dependency/oauth/handler/handler_authz.go
index 83d5a9a8..fe4a4879 100644
--- a/pkg/auth/dependency/oauth/handler/handler_authz.go
+++ b/pkg/auth/dependency/oauth/handler/handler_authz.go
@@ -47,18 +47,25 @@ func (h *AuthorizationHandler) Handle(r protocol.AuthorizationRequest) Authoriza
 	client := resolveClient(h.Clients, r)
 	if client == nil {
 		return authorizationResultError{
-			Response: protocol.NewErrorResponse("unauthorized_client", "invalid client ID"),
+			ResponseMode: r.ResponseMode(),
+			Response:     protocol.NewErrorResponse("unauthorized_client", "invalid client ID"),
 		}
 	}
 	redirectURI, errResp := parseRedirectURI(client, r)
 	if errResp != nil {
-		return authorizationResultError{Response: errResp}
+		return authorizationResultError{
+			ResponseMode: r.ResponseMode(),
+			Response:     errResp,
+		}
 	}
 
 	result, err := h.doHandle(redirectURI, client, r)
 	if err != nil {
 		var oauthError *protocol.OAuthProtocolError
-		resultErr := authorizationResultError{RedirectURI: redirectURI}
+		resultErr := authorizationResultError{
+			RedirectURI:  redirectURI,
+			ResponseMode: r.ResponseMode(),
+		}
 		if errors.As(err, &oauthError) {
 			resultErr.Response = oauthError.Response
 		} else {
@@ -160,9 +167,10 @@ func (h *AuthorizationHandler) doHandle(
 		resp.State(r.State())
 	}
 
-	return authorizationResultRedirect{
-		RedirectURI: redirectURI,
-		Response:    resp,
+	return authorizationResultCode{
+		RedirectURI:  redirectURI,
+		ResponseMode: r.ResponseMode(),
+		Response:     resp,
 	}, nil
 }
 
diff --git a/pkg/auth/dependency/oauth/handler/handler_token.go b/pkg/auth/dependency/oauth/handler/handler_token.go
index 3de7099f..50256aad 100644
--- a/pkg/auth/dependency/oauth/handler/handler_token.go
+++ b/pkg/auth/dependency/oauth/handler/handler_token.go
@@ -54,7 +54,7 @@ type TokenHandler struct {
 func (h *TokenHandler) Handle(r protocol.TokenRequest) TokenResult {
 	client := resolveClient(h.Clients, r)
 	if client == nil {
-		return authorizationResultError{
+		return tokenResultError{
 			Response: protocol.NewErrorResponse("invalid_client", "invalid client ID"),
 		}
 	}
diff --git a/pkg/auth/dependency/oauth/handler/response_mode.go b/pkg/auth/dependency/oauth/handler/response_mode.go
new file mode 100644
index 00000000..d99dac59
--- /dev/null
+++ b/pkg/auth/dependency/oauth/handler/response_mode.go
@@ -0,0 +1,106 @@
+package handler
+
+import (
+	"fmt"
+	"html/template"
+	"net/http"
+	"net/url"
+
+	coreurl "github.com/skygeario/skygear-server/pkg/core/url"
+)
+
+const htmlRedirectTemplateString = `<!DOCTYPE html>
+<html>
+<head>
+<meta http-equiv="refresh" content="0;url={{ .redirect_uri }}" />
+</head>
+<body>
+<script>
+window.location.href = "{{ .redirect_uri }}"
+</script>
+</body>
+</html>
+`
+
+const formPostTemplateString = `<!DOCTYPE html>
+<html>
+<head>
+<title>Submit this form</title>
+</head>
+<body>
+<noscript>Please submit this form to proceed</noscript>
+<form method="post" action="{{ .redirect_uri }}">
+{{- range $name, $value := .response }}
+<input type="hidden" name="{{ $name }}" value="{{ $value }}">
+{{- end }}
+<button type="submit" name="" value="">Submit</button>
+</form>
+<script>
+document.forms[0].submit();
+</script>
+</body>
+</html>
+`
+
+var htmlRedirectTemplate *template.Template
+var formPostTemplate *template.Template
+
+func init() {
+	var err error
+
+	htmlRedirectTemplate, err = template.New("html_redirect").Parse(htmlRedirectTemplateString)
+	if err != nil {
+		panic(fmt.Errorf("oauth: invalid html_redirect template: %w", err))
+	}
+
+	formPostTemplate, err = template.New("form_post").Parse(formPostTemplateString)
+	if err != nil {
+		panic(fmt.Errorf("oauth: invalid form_post template: %w", err))
+	}
+}
+
+func writeResponse(w http.ResponseWriter, r *http.Request, redirectURI *url.URL, responseMode string, response map[string]string) {
+	if responseMode == "" {
+		responseMode = "query"
+	}
+
+	switch responseMode {
+	case "query":
+		htmlRedirect(w, coreurl.WithQueryParamsAdded(redirectURI, response).String())
+	case "fragment":
+		htmlRedirect(w, coreurl.WithQueryParamsSetToFragment(redirectURI, response).String())
+	case "form_post":
+		formPost(w, redirectURI, response)
+	default:
+		http.Error(w, fmt.Sprintf("oauth: invalid response_mode %s", responseMode), http.StatusBadRequest)
+	}
+}
+
+func htmlRedirect(rw http.ResponseWriter, redirectURI string) {
+	rw.Header().Set("Content-Type", "text/html; charset=utf-8")
+	// NOTE(authui): XHR and redirect
+	// Normally we should use HTTP 302 to redirect.
+	// However, when XHR is used, redirect is followed automatically.
+	// The final redirect URI may be custom URI which is considered unsecure by user agent.
+	// Therefore, we write HTML and use <meta http-equiv="refresh"> to redirect.
+	// rw.Header().Set("Location", redirectURI)
+	// rw.WriteHeader(http.StatusFound)
+	err := htmlRedirectTemplate.Execute(rw, map[string]string{
+		"redirect_uri": redirectURI,
+	})
+	if err != nil {
+		panic(fmt.Errorf("oauth: failed to execute html_redirect template: %w", err))
+	}
+}
+
+func formPost(w http.ResponseWriter, redirectURI *url.URL, response map[string]string) {
+	w.Header().Set("Content-Type", "text/html; charset=utf-8")
+
+	err := formPostTemplate.Execute(w, map[string]interface{}{
+		"redirect_uri": redirectURI.String(),
+		"response":     response,
+	})
+	if err != nil {
+		panic(fmt.Errorf("oauth: failed to execute form_post template: %w", err))
+	}
+}
diff --git a/pkg/auth/dependency/oauth/handler/result_authz.go b/pkg/auth/dependency/oauth/handler/result_authz.go
index 62fb651c..11712a69 100644
--- a/pkg/auth/dependency/oauth/handler/result_authz.go
+++ b/pkg/auth/dependency/oauth/handler/result_authz.go
@@ -2,35 +2,27 @@ package handler
 
 import (
 	"fmt"
-	"html/template"
 	"net/http"
 	"net/url"
 	"sort"
 
 	"github.com/skygeario/skygear-server/pkg/auth/dependency/oauth/protocol"
-	coreurl "github.com/skygeario/skygear-server/pkg/core/url"
 )
 
-const AuthorizationResultHTML = `<!DOCTYPE html>
-<head>
-<meta http-equiv="refresh" content="0;url={{ .redirect_uri }}" />
-</head>
-<script>
-window.location.href = "{{ .redirect_uri }}"
-</script>`
-
 type AuthorizationResult interface {
 	WriteResponse(rw http.ResponseWriter, r *http.Request)
 	IsInternalError() bool
 }
 
 type (
-	authorizationResultRedirect struct {
-		RedirectURI *url.URL
-		Response    protocol.AuthorizationResponse
+	authorizationResultCode struct {
+		RedirectURI  *url.URL
+		ResponseMode string
+		Response     protocol.AuthorizationResponse
 	}
 	authorizationResultError struct {
 		RedirectURI   *url.URL
+		ResponseMode  string
 		InternalError bool
 		Response      protocol.ErrorResponse
 	}
@@ -39,19 +31,17 @@ type (
 	}
 )
 
-func (a authorizationResultRedirect) WriteResponse(rw http.ResponseWriter, r *http.Request) {
-	redirectURI := coreurl.WithQueryParamsAdded(a.RedirectURI, a.Response)
-	redirect(rw, redirectURI.String())
+func (a authorizationResultCode) WriteResponse(rw http.ResponseWriter, r *http.Request) {
+	writeResponse(rw, r, a.RedirectURI, a.ResponseMode, a.Response)
 }
 
-func (a authorizationResultRedirect) IsInternalError() bool {
+func (a authorizationResultCode) IsInternalError() bool {
 	return false
 }
 
 func (a authorizationResultError) WriteResponse(rw http.ResponseWriter, r *http.Request) {
 	if a.RedirectURI != nil {
-		redirectURI := coreurl.WithQueryParamsAdded(a.RedirectURI, a.Response)
-		redirect(rw, redirectURI.String())
+		writeResponse(rw, r, a.RedirectURI, a.ResponseMode, a.Response)
 	} else {
 		err := "Invalid OAuth authorization request:\n"
 		keys := make([]string, 0, len(a.Response))
@@ -80,20 +70,3 @@ func (a authorizationResultRequireAuthn) WriteResponse(rw http.ResponseWriter, r
 func (a authorizationResultRequireAuthn) IsInternalError() bool {
 	return false
 }
-
-func redirect(rw http.ResponseWriter, redirectURI string) {
-	rw.Header().Set("Location", redirectURI)
-	rw.Header().Set("Content-Type", "text/html; charset=utf-8")
-	rw.WriteHeader(http.StatusFound)
-
-	tmpl, err := template.New("authorization_result").Parse(AuthorizationResultHTML)
-	if err != nil {
-		panic("oauth: invalid authorization result page template")
-	}
-	err = tmpl.Execute(rw, map[string]string{
-		"redirect_uri": redirectURI,
-	})
-	if err != nil {
-		panic("oauth: failed to load authorization result page")
-	}
-}
diff --git a/pkg/auth/dependency/oauth/metadata.go b/pkg/auth/dependency/oauth/metadata.go
index 0e19c6ee..e9a2e44c 100644
--- a/pkg/auth/dependency/oauth/metadata.go
+++ b/pkg/auth/dependency/oauth/metadata.go
@@ -10,7 +10,9 @@ func (p *MetadataProvider) PopulateMetadata(meta map[string]interface{}) {
 	meta["authorization_endpoint"] = p.AuthorizeEndpoint.AuthorizeEndpointURI().String()
 	meta["token_endpoint"] = p.TokenEndpoint.TokenEndpointURI().String()
 	meta["response_types_supported"] = []string{"code", "none"}
+	meta["response_modes_supported"] = []string{"query", "fragment", "form_post"}
 	meta["grant_types_supported"] = []string{"authorization_code", "refresh_token"}
 	meta["code_challenge_methods_supported"] = []string{"S256"}
 	meta["revocation_endpoint"] = p.RevokeEndpoint.RevokeEndpointURI().String()
+
 }
diff --git a/pkg/auth/dependency/oauth/protocol/authz.go b/pkg/auth/dependency/oauth/protocol/authz.go
index 9ccf0866..526a76ff 100644
--- a/pkg/auth/dependency/oauth/protocol/authz.go
+++ b/pkg/auth/dependency/oauth/protocol/authz.go
@@ -7,6 +7,7 @@ type AuthorizationResponse map[string]string
 
 // OAuth 2.0
 
+func (r AuthorizationRequest) ResponseMode() string { return r["response_mode"] }
 func (r AuthorizationRequest) ResponseType() string { return r["response_type"] }
 func (r AuthorizationRequest) ClientID() string     { return r["client_id"] }
 func (r AuthorizationRequest) RedirectURI() string  { return r["redirect_uri"] }
diff --git a/pkg/core/url/fragment.go b/pkg/core/url/fragment.go
new file mode 100644
index 00000000..70e0f9e3
--- /dev/null
+++ b/pkg/core/url/fragment.go
@@ -0,0 +1,15 @@
+package url
+
+import (
+	"net/url"
+)
+
+func WithQueryParamsSetToFragment(u *url.URL, params map[string]string) *url.URL {
+	q := url.Values{}
+	for name, value := range params {
+		q.Set(name, value)
+	}
+	newU := *u
+	newU.Fragment = q.Encode()
+	return &newU
+}
diff --git a/static/authui/js/main.js b/static/authui/js/main.js
index 09d86a26..71855747 100644
--- a/static/authui/js/main.js
+++ b/static/authui/js/main.js
@@ -319,12 +319,5 @@ window.addEventListener("load", function() {
   attachPasswordPolicyCheck();
   attachResendButtonBehavior();
   attachFormSubmitOnceOnly();
-  // TODO(authui): backstack
-  // The final redirect may be customurischeme://host
-  // which is blocked when the request is submitted as XHR.
-  // However, if we do not use XHR to submit form,
-  // we still suffer from back stack problem.
-  // A better solution is to introduce an extra endpoint
-  // in /oauth2 to support redirect response mode and form post response mode.
-  // attachFormSubmitXHR();
+  attachFormSubmitXHR();
 });


Test patch:
diff --git a/pkg/auth/dependency/oauth/handler/handler_authz_test.go b/pkg/auth/dependency/oauth/handler/handler_authz_test.go
index fec1db8d..f02f1af9 100644
--- a/pkg/auth/dependency/oauth/handler/handler_authz_test.go
+++ b/pkg/auth/dependency/oauth/handler/handler_authz_test.go
@@ -44,9 +44,6 @@ func TestAuthorizationHandler(t *testing.T) {
 			result.WriteResponse(resp, req)
 			return resp
 		}
-		redirection := func(resp *httptest.ResponseRecorder) string {
-			return resp.Header().Get("Location")
-		}
 
 		Convey("general request validation", func() {
 			h.Clients = []config.OAuthClientConfiguration{{
@@ -86,9 +83,7 @@ func TestAuthorizationHandler(t *testing.T) {
 				"client_id":     "client-id",
 				"response_type": "code",
 			})
-			So(resp.Result().StatusCode, ShouldEqual, 302)
-			So(redirection(resp), ShouldEqual,
-				"https://example.com/cb?error=invalid_request&error_description=scope+is+required&from=sso")
+			So(resp.Result().StatusCode, ShouldEqual, 200)
 		})
 
 		Convey("authorization code flow", func() {
@@ -102,9 +97,7 @@ func TestAuthorizationHandler(t *testing.T) {
 						"client_id":     "client-id",
 						"response_type": "code",
 					})
-					So(resp.Result().StatusCode, ShouldEqual, 302)
-					So(redirection(resp), ShouldEqual,
-						"https://example.com/?error=invalid_request&error_description=scope+is+required")
+					So(resp.Result().StatusCode, ShouldEqual, 200)
 				})
 				Convey("missing PKCE code challenge", func() {
 					resp := handle(protocol.AuthorizationRequest{
@@ -112,9 +105,7 @@ func TestAuthorizationHandler(t *testing.T) {
 						"response_type": "code",
 						"scope":         "openid",
 					})
-					So(resp.Result().StatusCode, ShouldEqual, 302)
-					So(redirection(resp), ShouldEqual,
-						"https://example.com/?error=invalid_request&error_description=PKCE+code+challenge+is+required")
+					So(resp.Result().StatusCode, ShouldEqual, 200)
 				})
 				Convey("unsupported PKCE transform", func() {
 					resp := handle(protocol.AuthorizationRequest{
@@ -124,9 +115,7 @@ func TestAuthorizationHandler(t *testing.T) {
 						"code_challenge_method": "plain",
 						"code_challenge":        "code-verifier",
 					})
-					So(resp.Result().StatusCode, ShouldEqual, 302)
-					So(redirection(resp), ShouldEqual,
-						"https://example.com/?error=invalid_request&error_description=only+%27S256%27+PKCE+transform+is+supported")
+					So(resp.Result().StatusCode, ShouldEqual, 200)
 				})
 			})
 			Convey("scope validation", func() {
@@ -147,9 +136,7 @@ func TestAuthorizationHandler(t *testing.T) {
 					"code_challenge":        "E9Melhoa2OwvFrEMTJguCHaoeK1t8URWbuGJSstw-cM",
 				})
 				So(validated, ShouldBeTrue)
-				So(resp.Result().StatusCode, ShouldEqual, 302)
-				So(redirection(resp), ShouldEqual,
-					"https://example.com/?error=invalid_scope&error_description=must+request+%27openid%27+scope")
+				So(resp.Result().StatusCode, ShouldEqual, 200)
 			})
 			Convey("request authentication", func() {
 				resp := handle(protocol.AuthorizationRequest{
@@ -161,7 +148,6 @@ func TestAuthorizationHandler(t *testing.T) {
 					"ui_locales":            "ja",
 				})
 				So(resp.Result().StatusCode, ShouldEqual, 302)
-				So(redirection(resp), ShouldEqual, "https://auth/authenticate")
 			})
 			Convey("return authorization code", func() {
 				h.Context = authtesting.WithAuthn().
@@ -179,9 +165,7 @@ func TestAuthorizationHandler(t *testing.T) {
 						"nonce":                 "my-nonce",
 						"state":                 "my-state",
 					})
-					So(resp.Result().StatusCode, ShouldEqual, 302)
-					So(redirection(resp), ShouldEqual,
-						"https://example.com/?code=authz-code&state=my-state")
+					So(resp.Result().StatusCode, ShouldEqual, 200)
 
 					So(authzStore.authzs, ShouldHaveLength, 1)
 					So(authzStore.authzs[0], ShouldResemble, oauth.Authorization{
@@ -226,9 +210,7 @@ func TestAuthorizationHandler(t *testing.T) {
 						"code_challenge_method": "S256",
 						"code_challenge":        "E9Melhoa2OwvFrEMTJguCHaoeK1t8URWbuGJSstw-cM",
 					})
-					So(resp.Result().StatusCode, ShouldEqual, 302)
-					So(redirection(resp), ShouldEqual,
-						"https://example.com/?code=authz-code")
+					So(resp.Result().StatusCode, ShouldEqual, 200)
 
 					So(authzStore.authzs, ShouldHaveLength, 1)
 					So(authzStore.authzs[0], ShouldResemble, oauth.Authorization{
@@ -269,9 +251,7 @@ func TestAuthorizationHandler(t *testing.T) {
 						"client_id":     "client-id",
 						"response_type": "none",
 					})
-					So(resp.Result().StatusCode, ShouldEqual, 302)
-					So(redirection(resp), ShouldEqual,
-						"https://example.com/?error=unauthorized_client&error_description=response+type+is+not+allowed+for+this+client")
+					So(resp.Result().StatusCode, ShouldEqual, 200)
 				})
 			})
 			Convey("scope validation", func() {
@@ -290,9 +270,7 @@ func TestAuthorizationHandler(t *testing.T) {
 					"scope":         "email",
 				})
 				So(validated, ShouldBeTrue)
-				So(resp.Result().StatusCode, ShouldEqual, 302)
-				So(redirection(resp), ShouldEqual,
-					"https://example.com/?error=invalid_scope&error_description=must+request+%27openid%27+scope")
+				So(resp.Result().StatusCode, ShouldEqual, 200)
 			})
 			Convey("request authentication", func() {
 				resp := handle(protocol.AuthorizationRequest{
@@ -301,7 +279,6 @@ func TestAuthorizationHandler(t *testing.T) {
 					"scope":         "openid",
 				})
 				So(resp.Result().StatusCode, ShouldEqual, 302)
-				So(redirection(resp), ShouldEqual, "https://auth/authenticate")
 			})
 			Convey("redirect to URI", func() {
 				h.Context = authtesting.WithAuthn().
@@ -316,9 +293,7 @@ func TestAuthorizationHandler(t *testing.T) {
 						"scope":         "openid",
 						"state":         "my-state",
 					})
-					So(resp.Result().StatusCode, ShouldEqual, 302)
-					So(redirection(resp), ShouldEqual,
-						"https://example.com/?state=my-state")
+					So(resp.Result().StatusCode, ShouldEqual, 200)
 
 					So(authzStore.authzs, ShouldHaveLength, 1)
 					So(authzStore.authzs[0], ShouldResemble, oauth.Authorization{
diff --git a/pkg/auth/dependency/oauth/handler/response_mode_test.go b/pkg/auth/dependency/oauth/handler/response_mode_test.go
new file mode 100644
index 00000000..85caae5d
--- /dev/null
+++ b/pkg/auth/dependency/oauth/handler/response_mode_test.go
@@ -0,0 +1,84 @@
+package handler
+
+import (
+	"net/http"
+	"net/http/httptest"
+	"net/url"
+	"testing"
+
+	. "github.com/smartystreets/goconvey/convey"
+)
+
+func TestWriteResponse(t *testing.T) {
+	Convey("writeResponse", t, func() {
+		test := func(responseMode string, expected string) {
+			w := httptest.NewRecorder()
+			r, _ := http.NewRequest("GET", "/", nil)
+			redirectURI, _ := url.Parse("https://example.com")
+			response := map[string]string{
+				"code":  "this_is_the_code",
+				"state": "this_is_the_state",
+			}
+			writeResponse(w, r, redirectURI, responseMode, response)
+			So(string(w.Body.Bytes()), ShouldEqual, expected)
+		}
+
+		test("", `<!DOCTYPE html>
+<html>
+<head>
+<meta http-equiv="refresh" content="0;url=https://example.com?code=this_is_the_code&amp;state=this_is_the_state" />
+</head>
+<body>
+<script>
+window.location.href = "https:\/\/example.com?code=this_is_the_code\x26state=this_is_the_state"
+</script>
+</body>
+</html>
+`)
+
+		test("query", `<!DOCTYPE html>
+<html>
+<head>
+<meta http-equiv="refresh" content="0;url=https://example.com?code=this_is_the_code&amp;state=this_is_the_state" />
+</head>
+<body>
+<script>
+window.location.href = "https:\/\/example.com?code=this_is_the_code\x26state=this_is_the_state"
+</script>
+</body>
+</html>
+`)
+
+		test("fragment", `<!DOCTYPE html>
+<html>
+<head>
+<meta http-equiv="refresh" content="0;url=https://example.com#code=this_is_the_code&amp;state=this_is_the_state" />
+</head>
+<body>
+<script>
+window.location.href = "https:\/\/example.com#code=this_is_the_code\x26state=this_is_the_state"
+</script>
+</body>
+</html>
+`)
+
+		test("form_post", `<!DOCTYPE html>
+<html>
+<head>
+<title>Submit this form</title>
+</head>
+<body>
+<noscript>Please submit this form to proceed</noscript>
+<form method="post" action="https://example.com">
+<input type="hidden" name="code" value="this_is_the_code">
+<input type="hidden" name="state" value="this_is_the_state">
+<button type="submit" name="" value="">Submit</button>
+</form>
+<script>
+document.forms[0].submit();
+</script>
+</body>
+</html>
+`)
+	})
+}
diff --git a/pkg/core/url/fragment_test.go b/pkg/core/url/fragment_test.go
new file mode 100644
index 00000000..fa3e3f91
--- /dev/null
+++ b/pkg/core/url/fragment_test.go
@@ -0,0 +1,29 @@
+package url
+
+import (
+	"net/url"
+	"testing"
+
+	. "github.com/smartystreets/goconvey/convey"
+)
+
+func TestWithQueryParamsSetToFragment(t *testing.T) {
+	Convey("WithQueryParamsSetToFragment", t, func() {
+		test := func(u string, params map[string]string, expected string) {
+			uu, err := url.Parse(u)
+			So(err, ShouldBeNil)
+
+			actual := WithQueryParamsSetToFragment(uu, params)
+			So(actual.String(), ShouldEqual, expected)
+		}
+
+		test("http://example.com?a=b", map[string]string{
+			"c": "d",
+		}, "http://example.com?a=b#c=d")
+
+		test("http://example.com?a=b", map[string]string{
+			"c": "#",
+		}, "http://example.com?a=b#c=%2523")
+	})
+
+}