diff --git a/go.mod b/go.mod index 29c08db..5c5e936 100644 --- a/go.mod +++ b/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 diff --git a/go.sum b/go.sum index 035538e..0c9d05d 100644 --- a/go.sum +++ b/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= diff --git a/handle.go b/handle.go index 7346f79..0fcc483 100644 --- a/handle.go +++ b/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 == "" { diff --git a/oauth.go b/oauth.go index dc15d53..7dfc4c7 100644 --- a/oauth.go +++ b/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 + return impart.HTTPError{http.StatusInternalServerError, "could not prepare oauth redirect url"} } - 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/callback", handler.viewOauthCallback).Methods("GET") + r.HandleFunc("/oauth/"+oauthClient.GetProvider(), parentHandler.OAuth(handler.viewOauthInit)).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 + return impart.HTTPError{http.StatusInternalServerError, err.Error()} } tokenResponse, err := h.oauthClient.exchangeOauthCode(ctx, code) if err != nil { log.Error("Unable to exchangeOauthCode: %s", err) - failOAuthRequest(w, http.StatusInternalServerError, err.Error()) - return + return impart.HTTPError{http.StatusInternalServerError, err.Error()} } // 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 + return impart.HTTPError{http.StatusInternalServerError, err.Error()} } 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 + return impart.HTTPError{http.StatusInternalServerError, err.Error()} } 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 + return impart.HTTPError{http.StatusInternalServerError, "unable to create password hash"} } 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 + return impart.HTTPError{http.StatusInternalServerError, err.Error()} } err = h.DB.RecordRemoteUserID(ctx, newUser.ID, tokenInfo.UserID, provider, clientID, tokenResponse.AccessToken) if err != nil { - failOAuthRequest(w, http.StatusInternalServerError, err.Error()) - return + return impart.HTTPError{http.StatusInternalServerError, err.Error()} } 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 + return impart.HTTPError{http.StatusInternalServerError, err.Error()} } 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) - } -} diff --git a/oauth_test.go b/oauth_test.go index 2efc46d..1daabd5 100644 --- a/oauth_test.go +++ b/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" @@ -139,7 +140,7 @@ func TestViewOauthInit(t *testing.T) { Config: app.Config(), DB: app.DB(), Store: app.SessionStore(), - oauthClient:writeAsOauthClient{ + oauthClient: writeAsOauthClient{ ClientID: app.Config().WriteAsOauth.ClientID, ClientSecret: app.Config().WriteAsOauth.ClientSecret, ExchangeLocation: app.Config().WriteAsOauth.TokenLocation, @@ -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) - assert.Equal(t, http.StatusTemporaryRedirect, rr.Code) - locURI, err := url.Parse(rr.Header().Get("Location")) + err = h.viewOauthInit(nil, rr, req) + assert.NotNil(t, err) + 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")) @@ -177,7 +182,7 @@ func TestViewOauthInit(t *testing.T) { Config: app.Config(), DB: app.DB(), Store: app.SessionStore(), - oauthClient:writeAsOauthClient{ + oauthClient: writeAsOauthClient{ ClientID: app.Config().WriteAsOauth.ClientID, ClientSecret: app.Config().WriteAsOauth.ClientSecret, ExchangeLocation: app.Config().WriteAsOauth.TokenLocation, @@ -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) - assert.Equal(t, http.StatusInternalServerError, rr.Code) - expected := `{"error":"could not prepare oauth redirect url"}` + "\n" - assert.Equal(t, expected, rr.Body.String()) + err = h.viewOauthInit(nil, rr, req) + httpErr, ok := err.(impart.HTTPError) + assert.True(t, ok) + 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) }) diff --git a/routes.go b/routes.go index 40db096..a3d4a1c 100644 --- a/routes.go +++ b/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()) - configureWriteAsOauth(write, apper.App()) + configureSlackOauth(handler, write, apper.App()) + configureWriteAsOauth(handler, write, apper.App()) // Set up dyamic page handlers // Handle auth