Merge pull request #231 from writeas/oauth-wrapper

Pass OAuth requests through OAuth handler

go.mod

@@ -39,7 +39,7 @@ require (
 	github.com/writeas/go-strip-markdown v2.0.1+incompatible
 	github.com/writeas/go-webfinger v0.0.0-20190106002315-85cf805c86d2
 	github.com/writeas/httpsig v1.0.0
-	github.com/writeas/impart v1.1.0
+	github.com/writeas/impart v1.1.1-0.20191230230525-d3c45ced010d
 	github.com/writeas/monday v0.0.0-20181024183321-54a7dd579219
 	github.com/writeas/nerds v1.0.0
 	github.com/writeas/saturday v1.7.1

go.sum

@@ -123,6 +123,8 @@ github.com/writeas/httpsig v1.0.0 h1:peIAoIA3DmlP8IG8tMNZqI4YD1uEnWBmkcC9OFPjt3A
 github.com/writeas/httpsig v1.0.0/go.mod h1:7ClMGSrSVXJbmiLa17bZ1LrG1oibGZmUMlh3402flPY=
 github.com/writeas/impart v1.1.0 h1:nPnoO211VscNkp/gnzir5UwCDEvdHThL5uELU60NFSE=
 github.com/writeas/impart v1.1.0/go.mod h1:g0MpxdnTOHHrl+Ca/2oMXUHJ0PcRAEWtkCzYCJUXC9Y=
+github.com/writeas/impart v1.1.1-0.20191230230525-d3c45ced010d h1:PK7DOj3JE6MGf647esPrKzXEHFjGWX2hl22uX79ixaE=
+github.com/writeas/impart v1.1.1-0.20191230230525-d3c45ced010d/go.mod h1:g0MpxdnTOHHrl+Ca/2oMXUHJ0PcRAEWtkCzYCJUXC9Y=
 github.com/writeas/monday v0.0.0-20181024183321-54a7dd579219 h1:baEp0631C8sT2r/hqwypIw2snCFZa6h7U6TojoLHu/c=
 github.com/writeas/monday v0.0.0-20181024183321-54a7dd579219/go.mod h1:NyM35ayknT7lzO6O/1JpfgGyv+0W9Z9q7aE0J8bXxfQ=
 github.com/writeas/nerds v1.0.0 h1:ZzRcCN+Sr3MWID7o/x1cr1ZbLvdpej9Y1/Ho+JKlqxo=

handle.go

@@ -549,6 +549,37 @@ func (h *Handler) All(f handlerFunc) http.HandlerFunc {
 	}
 }
 
+func (h *Handler) OAuth(f handlerFunc) http.HandlerFunc {
+	return func(w http.ResponseWriter, r *http.Request) {
+		h.handleOAuthError(w, r, func() error {
+			// TODO: return correct "success" status
+			status := 200
+			start := time.Now()
+
+			defer func() {
+				if e := recover(); e != nil {
+					log.Error("%s:\n%s", e, debug.Stack())
+					impart.WriteError(w, impart.HTTPError{http.StatusInternalServerError, "Something didn't work quite right."})
+					status = 500
+				}
+
+				log.Info(h.app.ReqLog(r, status, time.Since(start)))
+			}()
+
+			err := f(h.app.App(), w, r)
+			if err != nil {
+				if err, ok := err.(impart.HTTPError); ok {
+					status = err.Status
+				} else {
+					status = 500
+				}
+			}
+
+			return err
+		}())
+	}
+}
+
 func (h *Handler) AllReader(f handlerFunc) http.HandlerFunc {
 	return func(w http.ResponseWriter, r *http.Request) {
 		h.handleError(w, r, func() error {
@@ -779,6 +810,25 @@ func (h *Handler) handleError(w http.ResponseWriter, r *http.Request, err error)
 	h.errors.InternalServerError.ExecuteTemplate(w, "base", pageForReq(h.app.App(), r))
 }
 
+func (h *Handler) handleOAuthError(w http.ResponseWriter, r *http.Request, err error) {
+	if err == nil {
+		return
+	}
+
+	if err, ok := err.(impart.HTTPError); ok {
+		if err.Status >= 300 && err.Status < 400 {
+			sendRedirect(w, err.Status, err.Message)
+			return
+		}
+
+		impart.WriteOAuthError(w, err)
+		return
+	}
+
+	impart.WriteOAuthError(w, impart.HTTPError{http.StatusInternalServerError, "This is an unhelpful error message for a miscellaneous internal error."})
+	return
+}
+
 func correctPageFromLoginAttempt(r *http.Request) string {
 	to := r.FormValue("to")
 	if to == "" {

oauth.go

@@ -7,6 +7,7 @@ import (
 	"github.com/gorilla/mux"
 	"github.com/gorilla/sessions"
 	"github.com/guregu/null/zero"
+	"github.com/writeas/impart"
 	"github.com/writeas/nerds/store"
 	"github.com/writeas/web-core/auth"
 	"github.com/writeas/web-core/log"
@@ -85,21 +86,20 @@ type oauthHandler struct {
 	oauthClient oauthClient
 }
 
-func (h oauthHandler) viewOauthInit(w http.ResponseWriter, r *http.Request) {
+func (h oauthHandler) viewOauthInit(app *App, w http.ResponseWriter, r *http.Request) error {
 	ctx := r.Context()
 	state, err := h.DB.GenerateOAuthState(ctx, h.oauthClient.GetProvider(), h.oauthClient.GetClientID())
 	if err != nil {
-		failOAuthRequest(w, http.StatusInternalServerError, "could not prepare oauth redirect url")
+		return impart.HTTPError{http.StatusInternalServerError, "could not prepare oauth redirect url"}
 	}
 	location, err := h.oauthClient.buildLoginURL(state)
 	if err != nil {
-		failOAuthRequest(w, http.StatusInternalServerError, "could not prepare oauth redirect url")
+		return impart.HTTPError{http.StatusInternalServerError, "could not prepare oauth redirect url"}
-		return
 	}
-	http.Redirect(w, r, location, http.StatusTemporaryRedirect)
+	return impart.HTTPError{http.StatusTemporaryRedirect, location}
 }
 
-func configureSlackOauth(r *mux.Router, app *App) {
+func configureSlackOauth(parentHandler *Handler, r *mux.Router, app *App) {
 	if app.Config().SlackOauth.ClientID != "" {
 		oauthClient := slackOauthClient{
 			ClientID:         app.Config().SlackOauth.ClientID,
@@ -108,11 +108,11 @@ func configureSlackOauth(r *mux.Router, app *App) {
 			CallbackLocation: app.Config().App.Host + "/oauth/callback",
 			HttpClient:       config.DefaultHTTPClient(),
 		}
-		configureOauthRoutes(r, app, oauthClient)
+		configureOauthRoutes(parentHandler, r, app, oauthClient)
 	}
 }
 
-func configureWriteAsOauth(r *mux.Router, app *App) {
+func configureWriteAsOauth(parentHandler *Handler, r *mux.Router, app *App) {
 	if app.Config().WriteAsOauth.ClientID != "" {
 		oauthClient := writeAsOauthClient{
 			ClientID:         app.Config().WriteAsOauth.ClientID,
@@ -126,22 +126,22 @@ func configureWriteAsOauth(r *mux.Router, app *App) {
 		if oauthClient.ExchangeLocation == "" {
 
 		}
-		configureOauthRoutes(r, app, oauthClient)
+		configureOauthRoutes(parentHandler, r, app, oauthClient)
 	}
 }
 
-func configureOauthRoutes(r *mux.Router, app *App, oauthClient oauthClient) {
+func configureOauthRoutes(parentHandler *Handler, r *mux.Router, app *App, oauthClient oauthClient) {
 	handler := &oauthHandler{
 		Config:      app.Config(),
 		DB:          app.DB(),
 		Store:       app.SessionStore(),
 		oauthClient: oauthClient,
 	}
-	r.HandleFunc("/oauth/"+oauthClient.GetProvider(), handler.viewOauthInit).Methods("GET")
+	r.HandleFunc("/oauth/"+oauthClient.GetProvider(), parentHandler.OAuth(handler.viewOauthInit)).Methods("GET")
-	r.HandleFunc("/oauth/callback", handler.viewOauthCallback).Methods("GET")
+	r.HandleFunc("/oauth/callback", parentHandler.OAuth(handler.viewOauthCallback)).Methods("GET")
 }
 
-func (h oauthHandler) viewOauthCallback(w http.ResponseWriter, r *http.Request) {
+func (h oauthHandler) viewOauthCallback(app *App, w http.ResponseWriter, r *http.Request) error {
 	ctx := r.Context()
 
 	code := r.FormValue("code")
@@ -150,15 +150,13 @@ func (h oauthHandler) viewOauthCallback(w http.ResponseWriter, r *http.Request)
 	provider, clientID, err := h.DB.ValidateOAuthState(ctx, state)
 	if err != nil {
 		log.Error("Unable to ValidateOAuthState: %s", err)
-		failOAuthRequest(w, http.StatusInternalServerError, err.Error())
+		return impart.HTTPError{http.StatusInternalServerError, err.Error()}
-		return
 	}
 
 	tokenResponse, err := h.oauthClient.exchangeOauthCode(ctx, code)
 	if err != nil {
 		log.Error("Unable to exchangeOauthCode: %s", err)
-		failOAuthRequest(w, http.StatusInternalServerError, err.Error())
+		return impart.HTTPError{http.StatusInternalServerError, err.Error()}
-		return
 	}
 
 	// Now that we have the access token, let's use it real quick to make sur
@@ -166,15 +164,13 @@ func (h oauthHandler) viewOauthCallback(w http.ResponseWriter, r *http.Request)
 	tokenInfo, err := h.oauthClient.inspectOauthAccessToken(ctx, tokenResponse.AccessToken)
 	if err != nil {
 		log.Error("Unable to inspectOauthAccessToken: %s", err)
-		failOAuthRequest(w, http.StatusInternalServerError, err.Error())
+		return impart.HTTPError{http.StatusInternalServerError, err.Error()}
-		return
 	}
 
 	localUserID, err := h.DB.GetIDForRemoteUser(ctx, tokenInfo.UserID, provider, clientID)
 	if err != nil {
 		log.Error("Unable to GetIDForRemoteUser: %s", err)
-		failOAuthRequest(w, http.StatusInternalServerError, err.Error())
+		return impart.HTTPError{http.StatusInternalServerError, err.Error()}
-		return
 	}
 
 	if localUserID == -1 {
@@ -185,8 +181,7 @@ func (h oauthHandler) viewOauthCallback(w http.ResponseWriter, r *http.Request)
 		randPass := store.Generate62RandomString(14)
 		hashedPass, err := auth.HashPass([]byte(randPass))
 		if err != nil {
-			failOAuthRequest(w, http.StatusInternalServerError, "unable to create password hash")
+			return impart.HTTPError{http.StatusInternalServerError, "unable to create password hash"}
-			return
 		}
 		newUser := &User{
 			Username:   tokenInfo.Username,
@@ -202,30 +197,28 @@ func (h oauthHandler) viewOauthCallback(w http.ResponseWriter, r *http.Request)
 
 		err = h.DB.CreateUser(h.Config, newUser, displayName)
 		if err != nil {
-			failOAuthRequest(w, http.StatusInternalServerError, err.Error())
+			return impart.HTTPError{http.StatusInternalServerError, err.Error()}
-			return
 		}
 
 		err = h.DB.RecordRemoteUserID(ctx, newUser.ID, tokenInfo.UserID, provider, clientID, tokenResponse.AccessToken)
 		if err != nil {
-			failOAuthRequest(w, http.StatusInternalServerError, err.Error())
+			return impart.HTTPError{http.StatusInternalServerError, err.Error()}
-			return
 		}
 
 		if err := loginOrFail(h.Store, w, r, newUser); err != nil {
-			failOAuthRequest(w, http.StatusInternalServerError, err.Error())
+			return impart.HTTPError{http.StatusInternalServerError, err.Error()}
 		}
-		return
+		return nil
 	}
 
 	user, err := h.DB.GetUserForAuthByID(localUserID)
 	if err != nil {
-		failOAuthRequest(w, http.StatusInternalServerError, err.Error())
+		return impart.HTTPError{http.StatusInternalServerError, err.Error()}
-		return
 	}
 	if err = loginOrFail(h.Store, w, r, user); err != nil {
-		failOAuthRequest(w, http.StatusInternalServerError, err.Error())
+		return impart.HTTPError{http.StatusInternalServerError, err.Error()}
 	}
+	return nil
 }
 
 func limitedJsonUnmarshal(body io.ReadCloser, n int, thing interface{}) error {
@@ -251,16 +244,3 @@ func loginOrFail(store sessions.Store, w http.ResponseWriter, r *http.Request, u
 	http.Redirect(w, r, "/", http.StatusTemporaryRedirect)
 	return nil
 }
-
-// failOAuthRequest is an HTTP handler helper that formats returned error
-// messages.
-func failOAuthRequest(w http.ResponseWriter, statusCode int, message string) {
-	w.Header().Set("Content-Type", "application/json")
-	w.WriteHeader(statusCode)
-	err := json.NewEncoder(w).Encode(map[string]interface{}{
-		"error": message,
-	})
-	if err != nil {
-		panic(err)
-	}
-}

oauth_test.go

@@ -5,6 +5,7 @@ import (
 	"fmt"
 	"github.com/gorilla/sessions"
 	"github.com/stretchr/testify/assert"
+	"github.com/writeas/impart"
 	"github.com/writeas/nerds/store"
 	"github.com/writeas/writefreely/config"
 	"net/http"
@@ -152,9 +153,13 @@ func TestViewOauthInit(t *testing.T) {
 		req, err := http.NewRequest("GET", "/oauth/client", nil)
 		assert.NoError(t, err)
 		rr := httptest.NewRecorder()
-		h.viewOauthInit(rr, req)
+		err = h.viewOauthInit(nil, rr, req)
-		assert.Equal(t, http.StatusTemporaryRedirect, rr.Code)
+		assert.NotNil(t, err)
-		locURI, err := url.Parse(rr.Header().Get("Location"))
+		httpErr, ok := err.(impart.HTTPError)
+		assert.True(t, ok)
+		assert.Equal(t, http.StatusTemporaryRedirect, httpErr.Status)
+		assert.NotEmpty(t, httpErr.Message)
+		locURI, err := url.Parse(httpErr.Message)
 		assert.NoError(t, err)
 		assert.Equal(t, "/oauth/login", locURI.Path)
 		assert.Equal(t, "development", locURI.Query().Get("client_id"))
@@ -190,10 +195,12 @@ func TestViewOauthInit(t *testing.T) {
 		req, err := http.NewRequest("GET", "/oauth/client", nil)
 		assert.NoError(t, err)
 		rr := httptest.NewRecorder()
-		h.viewOauthInit(rr, req)
+		err = h.viewOauthInit(nil, rr, req)
-		assert.Equal(t, http.StatusInternalServerError, rr.Code)
+		httpErr, ok := err.(impart.HTTPError)
-		expected := `{"error":"could not prepare oauth redirect url"}` + "\n"
+		assert.True(t, ok)
-		assert.Equal(t, expected, rr.Body.String())
+		assert.NotEmpty(t, httpErr.Message)
+		assert.Equal(t, http.StatusInternalServerError, httpErr.Status)
+		assert.Equal(t, "could not prepare oauth redirect url", httpErr.Message)
 	})
 }
 
@@ -236,7 +243,7 @@ func TestViewOauthCallback(t *testing.T) {
 		req, err := http.NewRequest("GET", "/oauth/callback", nil)
 		assert.NoError(t, err)
 		rr := httptest.NewRecorder()
-		h.viewOauthCallback(rr, req)
+		h.viewOauthCallback(nil, rr, req)
 		assert.NoError(t, err)
 		assert.Equal(t, http.StatusTemporaryRedirect, rr.Code)
 	})

routes.go

@@ -70,8 +70,8 @@ func InitRoutes(apper Apper, r *mux.Router) *mux.Router {
 	write.HandleFunc(nodeinfo.NodeInfoPath, handler.LogHandlerFunc(http.HandlerFunc(ni.NodeInfoDiscover)))
 	write.HandleFunc(niCfg.InfoURL, handler.LogHandlerFunc(http.HandlerFunc(ni.NodeInfo)))
 
-	configureSlackOauth(write, apper.App())
+	configureSlackOauth(handler, write, apper.App())
-	configureWriteAsOauth(write, apper.App())
+	configureWriteAsOauth(handler, write, apper.App())
 
 	// Set up dyamic page handlers
 	// Handle auth