From 7a0863f71b2504583e0537aa18664a5073d7f642 Mon Sep 17 00:00:00 2001 From: Nick Gerakines Date: Thu, 19 Dec 2019 11:48:04 -0500 Subject: [PATCH 1/6] Added oauth handlers and tests with mocks. Part of T705. --- admin.go | 2 +- app.go | 10 +- config/config.go | 9 ++ database.go | 80 +++++++++++++++ go.mod | 9 +- go.sum | 2 + handle.go | 4 +- main_test.go | 18 ++++ oauth.go | 252 +++++++++++++++++++++++++++++++++++++++++++++++ oauth/state.go | 10 ++ oauth_test.go | 198 +++++++++++++++++++++++++++++++++++++ 11 files changed, 585 insertions(+), 9 deletions(-) create mode 100644 main_test.go create mode 100644 oauth.go create mode 100644 oauth/state.go create mode 100644 oauth_test.go diff --git a/admin.go b/admin.go index ebb4225..0a73a11 100644 --- a/admin.go +++ b/admin.go @@ -260,7 +260,7 @@ func handleAdminToggleUserStatus(app *App, u *User, w http.ResponseWriter, r *ht } if err != nil { log.Error("toggle user suspended: %v", err) - return impart.HTTPError{http.StatusInternalServerError, fmt.Sprintf("Could not toggle user status: %v")} + return impart.HTTPError{http.StatusInternalServerError, fmt.Sprintf("Could not toggle user status: %v", err)} } return impart.HTTPError{http.StatusFound, fmt.Sprintf("/admin/user/%s#status", username)} } diff --git a/app.go b/app.go index d71fb1e..d465a3e 100644 --- a/app.go +++ b/app.go @@ -70,7 +70,7 @@ type App struct { cfg *config.Config cfgFile string keys *key.Keychain - sessionStore *sessions.CookieStore + sessionStore sessions.Store formDecoder *schema.Decoder timeline *localTimeline @@ -101,6 +101,14 @@ func (app *App) SetKeys(k *key.Keychain) { app.keys = k } +func (app *App) SessionStore() sessions.Store { + return app.sessionStore +} + +func (app *App) SetSessionStore(s sessions.Store) { + app.sessionStore = s +} + // Apper is the interface for getting data into and out of a WriteFreely // instance (or "App"). // diff --git a/config/config.go b/config/config.go index 84bae86..4b9586e 100644 --- a/config/config.go +++ b/config/config.go @@ -92,6 +92,15 @@ type ( LocalTimeline bool `ini:"local_timeline"` UserInvites string `ini:"user_invites"` + // OAuth + EnableOAuth bool `ini:"enable_oauth"` + OAuthProviderAuthLocation string `ini:"oauth_auth_location"` + OAuthProviderTokenLocation string `ini:"oauth_token_location"` + OAuthProviderInspectLocation string `ini:"oauth_inspect_location"` + OAuthClientCallbackLocation string `ini:"oauth_callback_location"` + OAuthClientID string `ini:"oauth_client_id"` + OAuthClientSecret string `ini:"oauth_client_secret"` + // Defaults DefaultVisibility string `ini:"default_visibility"` } diff --git a/database.go b/database.go index d78d888..735d3f7 100644 --- a/database.go +++ b/database.go @@ -11,8 +11,12 @@ package writefreely import ( + "context" + "crypto/rand" "database/sql" "fmt" + "github.com/pkg/errors" + "math/big" "net/http" "strings" "time" @@ -2453,6 +2457,59 @@ func (db *datastore) GetCollectionLastPostTime(id int64) (*time.Time, error) { return &t, nil } +func (db *datastore) GenerateOAuthState(ctx context.Context) (string, error) { + state, err := randString(24) + if err != nil { + return "", err + } + _, err = db.ExecContext(ctx, "INSERT INTO oauth_client_state (state, used, created_at) VALUES (?, FALSE, NOW())", state) + if err != nil { + return "", fmt.Errorf("unable to record oauth client state: %w", err) + } + return state, nil +} + +func (db *datastore) ValidateOAuthState(ctx context.Context, state string) error { + res, err := db.ExecContext(ctx, "UPDATE oauth_client_state SET used = TRUE WHERE state = ?", state) + if err != nil { + return err + } + rowsAffected, err := res.RowsAffected() + if err != nil { + return err + } + if rowsAffected != 1 { + return fmt.Errorf("state not found") + } + return nil +} + +func (db *datastore) RecordRemoteUserID(ctx context.Context, localUserID, remoteUserID int64) error { + var err error + if db.driverName == driverSQLite { + _, err = db.ExecContext(ctx, "INSERT OR REPLACE INTO users_oauth (user_id, remote_user_id) VALUES (?, ?)", localUserID, remoteUserID) + } else { + _, err = db.ExecContext(ctx, "INSERT INTO users_oauth (user_id, remote_user_id) VALUES (?, ?) "+db.upsert("user_id"), localUserID, remoteUserID) + } + if err != nil { + log.Error("Unable to INSERT users_oauth for '%d': %v", localUserID, err) + } + return err +} + +// GetIDForRemoteUser returns a user ID associated with a remote user ID. +func (db *datastore) GetIDForRemoteUser(ctx context.Context, remoteUserID int64) (int64, error) { + var userID int64 = -1 + err := db. + QueryRowContext(ctx, "SELECT user_id FROM users_oauth WHERE remote_user_id = ?", remoteUserID). + Scan(&userID) + // Not finding a record is OK. + if err != nil && err != sql.ErrNoRows { + return -1, err + } + return userID, nil +} + // DatabaseInitialized returns whether or not the current datastore has been // initialized with the correct schema. // Currently, it checks to see if the `users` table exists. @@ -2483,3 +2540,26 @@ func handleFailedPostInsert(err error) error { log.Error("Couldn't insert into posts: %v", err) return err } + +func randString(length int) (string, error) { + // every printable character on a US keyboard + charset := []rune("ABCDEFGHIJKLMNOPQRSTUVWXYZabcdefghijklmnopqrstuvwxyz0123456789") + out := make([]rune, length) + + setLen := big.NewInt(int64(len(charset))) + for idx := 0; idx < length; idx++ { + offset, err := rand.Int(rand.Reader, setLen) + if err != nil { + return "", err + } + + if !offset.IsUint64() { + // this should (in theory) never happen + return "", errors.Errorf("Non-Uint64 offset returned from rand.Int") + } + + out[idx] = charset[offset.Uint64()] + } + + return string(out), nil +} diff --git a/go.mod b/go.mod index 88af8c9..d34c4c9 100644 --- a/go.mod +++ b/go.mod @@ -29,12 +29,11 @@ require ( github.com/nicksnyder/go-i18n v1.10.0 // indirect github.com/nu7hatch/gouuid v0.0.0-20131221200532-179d4d0c4d8d github.com/pelletier/go-toml v1.2.0 // indirect - github.com/pkg/errors v0.8.1 // indirect + github.com/pkg/errors v0.8.1 github.com/rainycape/unidecode v0.0.0-20150907023854-cb7f23ec59be // indirect - github.com/shurcooL/sanitized_anchor_name v1.0.0 // indirect github.com/smartystreets/assertions v0.0.0-20190116191733-b6c0e53d7304 // indirect github.com/smartystreets/goconvey v0.0.0-20181108003508-044398e4856c // indirect - github.com/stretchr/testify v1.3.0 // indirect + github.com/stretchr/testify v1.3.0 github.com/writeas/activity v0.1.2 github.com/writeas/go-strip-markdown v2.0.1+incompatible github.com/writeas/go-webfinger v0.0.0-20190106002315-85cf805c86d2 @@ -42,7 +41,6 @@ require ( github.com/writeas/impart v1.1.0 github.com/writeas/monday v0.0.0-20181024183321-54a7dd579219 github.com/writeas/nerds v1.0.0 - github.com/writeas/openssl-go v1.0.0 // indirect github.com/writeas/saturday v1.7.1 github.com/writeas/slug v1.2.0 github.com/writeas/web-core v1.2.0 @@ -55,6 +53,7 @@ require ( google.golang.org/appengine v1.4.0 // indirect gopkg.in/alecthomas/kingpin.v3-unstable v3.0.0-20180810215634-df19058c872c // indirect gopkg.in/ini.v1 v1.41.0 - gopkg.in/yaml.v1 v1.0.0-20140924161607-9f9df34309c0 // indirect gopkg.in/yaml.v2 v2.2.2 // indirect ) + +go 1.13 diff --git a/go.sum b/go.sum index b256223..035538e 100644 --- a/go.sum +++ b/go.sum @@ -129,6 +129,7 @@ github.com/writeas/nerds v1.0.0 h1:ZzRcCN+Sr3MWID7o/x1cr1ZbLvdpej9Y1/Ho+JKlqxo= github.com/writeas/nerds v1.0.0/go.mod h1:Gn2bHy1EwRcpXeB7ZhVmuUwiweK0e+JllNf66gvNLdU= github.com/writeas/openssl-go v1.0.0 h1:YXM1tDXeYOlTyJjoMlYLQH1xOloUimSR1WMF8kjFc5o= github.com/writeas/openssl-go v1.0.0/go.mod h1:WsKeK5jYl0B5y8ggOmtVjbmb+3rEGqSD25TppjJnETA= +github.com/writeas/saturday v1.6.0/go.mod h1:ETE1EK6ogxptJpAgUbcJD0prAtX48bSloie80+tvnzQ= github.com/writeas/saturday v1.7.1 h1:lYo1EH6CYyrFObQoA9RNWHVlpZA5iYL5Opxo7PYAnZE= github.com/writeas/saturday v1.7.1/go.mod h1:ETE1EK6ogxptJpAgUbcJD0prAtX48bSloie80+tvnzQ= github.com/writeas/slug v1.2.0 h1:EMQ+cwLiOcA6EtFwUgyw3Ge18x9uflUnOnR6bp/J+/g= @@ -141,6 +142,7 @@ github.com/writefreely/go-nodeinfo v1.2.0 h1:La+YbTCvmpTwFhBSlebWDDL81N88Qf/SCAv github.com/writefreely/go-nodeinfo v1.2.0/go.mod h1:UTvE78KpcjYOlRHupZIiSEFcXHioTXuacCbHU+CAcPg= golang.org/x/crypto v0.0.0-20180527072434-ab813273cd59 h1:hk3yo72LXLapY9EXVttc3Z1rLOxT9IuAPPX3GpY2+jo= golang.org/x/crypto v0.0.0-20180527072434-ab813273cd59/go.mod h1:6SG95UA2DQfeDnfUPMdvaQW0Q7yPrPDi9nlGo2tz2b4= +golang.org/x/crypto v0.0.0-20190131182504-b8fe1690c613/go.mod h1:6SG95UA2DQfeDnfUPMdvaQW0Q7yPrPDi9nlGo2tz2b4= golang.org/x/crypto v0.0.0-20190208162236-193df9c0f06f h1:ETU2VEl7TnT5bl7IvuKEzTDpplg5wzGYsOCAPhdoEIg= golang.org/x/crypto v0.0.0-20190208162236-193df9c0f06f/go.mod h1:6SG95UA2DQfeDnfUPMdvaQW0Q7yPrPDi9nlGo2tz2b4= golang.org/x/lint v0.0.0-20181026193005-c67002cb31c3/go.mod h1:UVdnD1Gm6xHRNCYTkRU2/jEulfH38KcIWyp/GAMgvoE= diff --git a/handle.go b/handle.go index 7e410f5..7346f79 100644 --- a/handle.go +++ b/handle.go @@ -73,7 +73,7 @@ type ( type Handler struct { errors *ErrorPages - sessionStore *sessions.CookieStore + sessionStore sessions.Store app Apper } @@ -96,7 +96,7 @@ func NewHandler(apper Apper) *Handler { InternalServerError: template.Must(template.New("").Parse("{{define \"base\"}}500

Internal server error.

{{end}}")), Blank: template.Must(template.New("").Parse("{{define \"base\"}}{{.Title}}

{{.Content}}

{{end}}")), }, - sessionStore: apper.App().sessionStore, + sessionStore: apper.App().SessionStore(), app: apper, } diff --git a/main_test.go b/main_test.go new file mode 100644 index 0000000..3d16ece --- /dev/null +++ b/main_test.go @@ -0,0 +1,18 @@ +package writefreely + +import ( + "encoding/gob" + "math/rand" + "os" + "testing" + "time" +) + +// TestMain provides testing infrastructure within this package. +func TestMain(m *testing.M) { + rand.Seed(time.Now().UTC().UnixNano()) + + gob.Register(&User{}) + + os.Exit(m.Run()) +} \ No newline at end of file diff --git a/oauth.go b/oauth.go new file mode 100644 index 0000000..4b37277 --- /dev/null +++ b/oauth.go @@ -0,0 +1,252 @@ +package writefreely + +import ( + "context" + "encoding/json" + "github.com/gorilla/sessions" + "github.com/guregu/null/zero" + "github.com/writeas/impart" + "github.com/writeas/web-core/auth" + "github.com/writeas/web-core/log" + "github.com/writeas/writefreely/config" + "io" + "io/ioutil" + "net/http" + "net/url" + "strings" + "time" +) + +// TokenResponse contains data returned when a token is created either +// through a code exchange or using a refresh token. +type TokenResponse struct { + AccessToken string `json:"access_token"` + ExpiresIn int `json:"expires_in"` + RefreshToken string `json:"refresh_token"` + TokenType string `json:"token_type"` +} + +// InspectResponse contains data returned when an access token is inspected. +type InspectResponse struct { + ClientID string `json:"client_id"` + UserID int64 `json:"user_id"` + ExpiresAt time.Time `json:"expires_at"` + Username string `json:"username"` + Email string `json:"email"` +} + +// tokenRequestMaxLen is the most bytes that we'll read from the /oauth/token +// endpoint. One megabyte is plenty. +const tokenRequestMaxLen = 1000000 + +// infoRequestMaxLen is the most bytes that we'll read from the +// /oauth/inspect endpoint. +const infoRequestMaxLen = 1000000 + +// OAuthDatastoreProvider provides a minimal interface of data store, config, +// and session store for use with the oauth handlers. +type OAuthDatastoreProvider interface { + DB() OAuthDatastore + Config() *config.Config + SessionStore() sessions.Store +} + +// OAuthDatastore provides a minimal interface of data store methods used in +// oauth functionality. +type OAuthDatastore interface { + GenerateOAuthState(context.Context) (string, error) + ValidateOAuthState(context.Context, string) error + GetIDForRemoteUser(context.Context, int64) (int64, error) + CreateUser(*config.Config, *User, string) error + RecordRemoteUserID(context.Context, int64, int64) error + GetUserForAuthByID(int64) (*User, error) +} + +type HttpClient interface { + Do(req *http.Request) (*http.Response, error) +} + +type oauthHandler struct { + HttpClient HttpClient +} + +// buildAuthURL returns a URL used to initiate authentication. +func buildAuthURL(app OAuthDatastoreProvider, ctx context.Context, clientID, authLocation, callbackURL string) (string, error) { + state, err := app.DB().GenerateOAuthState(ctx) + if err != nil { + return "", err + } + + u, err := url.Parse(authLocation) + if err != nil { + return "", err + } + q := u.Query() + q.Set("client_id", clientID) + q.Set("redirect_uri", callbackURL) + q.Set("response_type", "code") + q.Set("state", state) + u.RawQuery = q.Encode() + + return u.String(), nil +} + +func (h oauthHandler) viewOauthInit(app OAuthDatastoreProvider, w http.ResponseWriter, r *http.Request) error { + location, err := buildAuthURL(app, r.Context(), app.Config().App.OAuthClientID, app.Config().App.OAuthProviderAuthLocation, app.Config().App.OAuthClientCallbackLocation) + if err != nil { + log.ErrorLog.Println(err) + return impart.HTTPError{Status: http.StatusInternalServerError, Message: "Could not prepare OAuth redirect URL."} + } + http.Redirect(w, r, location, http.StatusTemporaryRedirect) + return nil +} + +func (h oauthHandler) viewOauthCallback(app OAuthDatastoreProvider, w http.ResponseWriter, r *http.Request) error { + ctx := r.Context() + + code := r.FormValue("code") + state := r.FormValue("state") + + err := app.DB().ValidateOAuthState(ctx, state) + if err != nil { + return err + } + + tokenResponse, err := h.exchangeOauthCode(app, ctx, code) + if err != nil { + return err + } + + // Now that we have the access token, let's use it real quick to make sur + // it really really works. + tokenInfo, err := h.inspectOauthAccessToken(app, ctx, tokenResponse.AccessToken) + if err != nil { + return err + } + + localUserID, err := app.DB().GetIDForRemoteUser(ctx, tokenInfo.UserID) + if err != nil { + return err + } + + if localUserID == -1 { + // We don't have, nor do we want, the password from the origin, so we + //create a random string. If the user needs to set a password, they + //can do so through the settings page or through the password reset + //flow. + randPass, err := randString(14) + if err != nil { + return err + } + hashedPass, err := auth.HashPass([]byte(randPass)) + if err != nil { + log.ErrorLog.Println(err) + return impart.HTTPError{http.StatusInternalServerError, "Could not create password hash."} + } + newUser := &User{ + Username: tokenInfo.Username, + HashedPass: hashedPass, + HasPass: true, + Email: zero.NewString("", tokenInfo.Email != ""), + Created: time.Now().Truncate(time.Second).UTC(), + } + + err = app.DB().CreateUser(app.Config(), newUser, newUser.Username) + if err != nil { + return err + } + + err = app.DB().RecordRemoteUserID(ctx, newUser.ID, tokenInfo.UserID) + if err != nil { + return err + } + + return loginOrFail(app, w, r, newUser) + } + + user, err := app.DB().GetUserForAuthByID(localUserID) + if err != nil { + return err + } + return loginOrFail(app, w, r, user) +} + +func (h oauthHandler) exchangeOauthCode(app OAuthDatastoreProvider, ctx context.Context, code string) (*TokenResponse, error) { + form := url.Values{} + form.Add("grant_type", "authorization_code") + form.Add("redirect_uri", app.Config().App.OAuthClientCallbackLocation) + form.Add("code", code) + req, err := http.NewRequest("POST", app.Config().App.OAuthProviderTokenLocation, strings.NewReader(form.Encode())) + if err != nil { + return nil, err + } + req.WithContext(ctx) + req.Header.Set("User-Agent", "writefreely") + req.Header.Set("Accept", "application/json") + req.Header.Set("Content-Type", "application/x-www-form-urlencoded") + req.SetBasicAuth(app.Config().App.OAuthClientID, app.Config().App.OAuthClientSecret) + + resp, err := h.HttpClient.Do(req) + if err != nil { + return nil, err + } + + // Nick: I like using limited readers to reduce the risk of an endpoint + // being broken or compromised. + lr := io.LimitReader(resp.Body, tokenRequestMaxLen) + body, err := ioutil.ReadAll(lr) + if err != nil { + return nil, err + } + + var tokenResponse TokenResponse + err = json.Unmarshal(body, &tokenResponse) + if err != nil { + return nil, err + } + return &tokenResponse, nil +} + +func (h oauthHandler) inspectOauthAccessToken(app OAuthDatastoreProvider, ctx context.Context, accessToken string) (*InspectResponse, error) { + req, err := http.NewRequest("GET", app.Config().App.OAuthProviderInspectLocation, nil) + if err != nil { + return nil, err + } + req.WithContext(ctx) + req.Header.Set("User-Agent", "writefreely") + req.Header.Set("Accept", "application/json") + req.Header.Set("Authorization", "Bearer "+accessToken) + + resp, err := h.HttpClient.Do(req) + if err != nil { + return nil, err + } + + // Nick: I like using limited readers to reduce the risk of an endpoint + // being broken or compromised. + lr := io.LimitReader(resp.Body, infoRequestMaxLen) + body, err := ioutil.ReadAll(lr) + if err != nil { + return nil, err + } + + var inspectResponse InspectResponse + err = json.Unmarshal(body, &inspectResponse) + if err != nil { + return nil, err + } + return &inspectResponse, nil +} + +func loginOrFail(app OAuthDatastoreProvider, w http.ResponseWriter, r *http.Request, user *User) error { + session, err := app.SessionStore().Get(r, cookieName) + if err != nil { + return err + } + session.Values[cookieUserVal] = user.Cookie() + if err = session.Save(r, w); err != nil { + return err + } + http.Redirect(w, r, "/", http.StatusTemporaryRedirect) + return nil +} diff --git a/oauth/state.go b/oauth/state.go new file mode 100644 index 0000000..e8dd154 --- /dev/null +++ b/oauth/state.go @@ -0,0 +1,10 @@ +package oauth + +import "context" + +// ClientStateStore provides state management used by the OAuth client. +type ClientStateStore interface { + Generate(ctx context.Context) (string, error) + Validate(ctx context.Context, state string) error +} + diff --git a/oauth_test.go b/oauth_test.go new file mode 100644 index 0000000..02e357d --- /dev/null +++ b/oauth_test.go @@ -0,0 +1,198 @@ +package writefreely + +import ( + "context" + "fmt" + "github.com/gorilla/sessions" + "github.com/stretchr/testify/assert" + "github.com/writeas/impart" + "github.com/writeas/writefreely/config" + "net/http" + "net/http/httptest" + "net/url" + "strings" + "testing" +) + +type MockOAuthDatastoreProvider struct { + DoDB func() OAuthDatastore + DoConfig func() *config.Config + DoSessionStore func() sessions.Store +} + +type MockOAuthDatastore struct { + DoGenerateOAuthState func(ctx context.Context) (string, error) + DoValidateOAuthState func(context.Context, string) error + DoGetIDForRemoteUser func(context.Context, int64) (int64, error) + DoCreateUser func(*config.Config, *User, string) error + DoRecordRemoteUserID func(context.Context, int64, int64) error + DoGetUserForAuthByID func(int64) (*User, error) +} + +type StringReadCloser struct { + *strings.Reader +} + +func (src *StringReadCloser) Close() error { + return nil +} + +type MockHTTPClient struct { + DoDo func(req *http.Request) (*http.Response, error) +} + +func (m *MockHTTPClient) Do(req *http.Request) (*http.Response, error) { + if m.DoDo != nil { + return m.DoDo(req) + } + return &http.Response{}, nil +} + +func (m *MockOAuthDatastoreProvider) SessionStore() sessions.Store { + if m.DoSessionStore != nil { + return m.DoSessionStore() + } + return sessions.NewCookieStore([]byte("secret-key")) +} + +func (m *MockOAuthDatastoreProvider) DB() OAuthDatastore { + if m.DoDB != nil { + return m.DoDB() + } + return &MockOAuthDatastore{} +} + +func (m *MockOAuthDatastoreProvider) Config() *config.Config { + if m.DoConfig != nil { + return m.DoConfig() + } + cfg := config.New() + cfg.UseSQLite(true) + cfg.App.EnableOAuth = true + cfg.App.OAuthProviderAuthLocation = "https://write.as/oauth/login" + cfg.App.OAuthProviderTokenLocation = "https://write.as/oauth/token" + cfg.App.OAuthProviderInspectLocation = "https://write.as/oauth/inspect" + cfg.App.OAuthClientCallbackLocation = "http://localhost/oauth/callback" + cfg.App.OAuthClientID = "development" + cfg.App.OAuthClientSecret = "development" + return cfg +} + +func (m *MockOAuthDatastore) ValidateOAuthState(ctx context.Context, state string) error { + if m.DoValidateOAuthState != nil { + return m.DoValidateOAuthState(ctx, state) + } + return nil +} + +func (m *MockOAuthDatastore) GetIDForRemoteUser(ctx context.Context, remoteUserID int64) (int64, error) { + if m.DoGetIDForRemoteUser != nil { + return m.DoGetIDForRemoteUser(ctx, remoteUserID) + } + return -1, nil +} + +func (m *MockOAuthDatastore) CreateUser(cfg *config.Config, u *User, username string) error { + if m.DoCreateUser != nil { + return m.DoCreateUser(cfg, u, username) + } + u.ID = 1 + return nil +} + +func (m *MockOAuthDatastore) RecordRemoteUserID(ctx context.Context, localUserID int64, remoteUserID int64) error { + if m.DoRecordRemoteUserID != nil { + return m.DoRecordRemoteUserID(ctx, localUserID, remoteUserID) + } + return nil +} + +func (m *MockOAuthDatastore) GetUserForAuthByID(userID int64) (*User, error) { + if m.DoGetUserForAuthByID != nil { + return m.DoGetUserForAuthByID(userID) + } + user := &User{ + + } + return user, nil +} + +func (m *MockOAuthDatastore) GenerateOAuthState(ctx context.Context) (string, error) { + if m.DoGenerateOAuthState != nil { + return m.DoGenerateOAuthState(ctx) + } + return randString(14) +} + +func TestViewOauthInit(t *testing.T) { + h := oauthHandler{} + t.Run("success", func(t *testing.T) { + app := &MockOAuthDatastoreProvider{} + req, err := http.NewRequest("GET", "/oauth/client", nil) + assert.NoError(t, err) + rr := httptest.NewRecorder() + err = h.viewOauthInit(app, rr, req) + assert.NoError(t, err) + assert.Equal(t, http.StatusTemporaryRedirect, rr.Code) + locURI, err := url.Parse(rr.Header().Get("Location")) + assert.NoError(t, err) + assert.Equal(t, "/oauth/login", locURI.Path) + assert.Equal(t, "development", locURI.Query().Get("client_id")) + assert.Equal(t, "http://localhost/oauth/callback", locURI.Query().Get("redirect_uri")) + assert.Equal(t, "code", locURI.Query().Get("response_type")) + assert.NotEmpty(t, locURI.Query().Get("state")) + }) + + t.Run("state failure", func(t *testing.T) { + app := &MockOAuthDatastoreProvider{ + DoDB: func() OAuthDatastore { + return &MockOAuthDatastore{ + DoGenerateOAuthState: func(ctx context.Context) (string, error) { + return "", fmt.Errorf("pretend unable to write state error") + }, + } + }, + } + req, err := http.NewRequest("GET", "/oauth/client", nil) + assert.NoError(t, err) + rr := httptest.NewRecorder() + err = h.viewOauthInit(app, rr, req) + assert.Error(t, err) + assert.Equal(t, impart.HTTPError{Status: http.StatusInternalServerError, Message: "Could not prepare OAuth redirect URL."}, err) + }) +} + +func TestViewOauthCallback(t *testing.T) { + t.Run("success", func(t *testing.T) { + app := &MockOAuthDatastoreProvider{} + h := oauthHandler{ + HttpClient: &MockHTTPClient{ + DoDo: func(req *http.Request) (*http.Response, error) { + switch req.URL.String() { + case "https://write.as/oauth/token": + return &http.Response{ + StatusCode: 200, + Body: &StringReadCloser{strings.NewReader(`{"access_token": "access_token", "expires_in": 1000, "refresh_token": "refresh_token", "token_type": "access"}`)}, + }, nil + case "https://write.as/oauth/inspect": + return &http.Response{ + StatusCode: 200, + Body: &StringReadCloser{strings.NewReader(`{"client_id": "development", "user_id": 1, "expires_at": "2019-12-19T11:42:01Z", "username": "nick", "email": "nick@testing.write.as"}`)}, + }, nil + } + + return &http.Response{ + StatusCode: http.StatusNotFound, + }, nil + }, + }, + } + req, err := http.NewRequest("GET", "/oauth/callback", nil) + assert.NoError(t, err) + rr := httptest.NewRecorder() + err = h.viewOauthCallback(app, rr, req) + assert.NoError(t, err) + assert.Equal(t, http.StatusTemporaryRedirect, rr.Code) + + }) +} From bf3b6a5ba01fbe1c3b726676da326b66e2ad4a4e Mon Sep 17 00:00:00 2001 From: Nick Gerakines Date: Mon, 23 Dec 2019 14:30:32 -0500 Subject: [PATCH 2/6] Unit tests, integration testing, and code cleanup for oauth support. Part of T705. --- database.go | 7 +- database_test.go | 43 +++++++ db/create.go | 271 +++++++++++++++++++++++++++++++++++++++ db/create_test.go | 146 +++++++++++++++++++++ db/tx.go | 26 ++++ go.mod | 2 +- go.sum | 1 - main_test.go | 139 +++++++++++++++++++- migrations/migrations.go | 1 + migrations/v4.go | 46 +++++++ oauth.go | 109 ++++++++++------ oauth_test.go | 28 ++-- routes.go | 10 ++ 13 files changed, 777 insertions(+), 52 deletions(-) create mode 100644 database_test.go create mode 100644 db/create.go create mode 100644 db/create_test.go create mode 100644 db/tx.go create mode 100644 migrations/v4.go diff --git a/database.go b/database.go index 735d3f7..5b3aaa8 100644 --- a/database.go +++ b/database.go @@ -128,6 +128,11 @@ type writestore interface { GetUserLastPostTime(id int64) (*time.Time, error) GetCollectionLastPostTime(id int64) (*time.Time, error) + GetIDForRemoteUser(ctx context.Context, remoteUserID int64) (int64, error) + RecordRemoteUserID(ctx context.Context, localUserID, remoteUserID int64) error + ValidateOAuthState(ctx context.Context, state string) error + GenerateOAuthState(ctx context.Context) (string, error) + DatabaseInitialized() bool } @@ -2489,7 +2494,7 @@ func (db *datastore) RecordRemoteUserID(ctx context.Context, localUserID, remote if db.driverName == driverSQLite { _, err = db.ExecContext(ctx, "INSERT OR REPLACE INTO users_oauth (user_id, remote_user_id) VALUES (?, ?)", localUserID, remoteUserID) } else { - _, err = db.ExecContext(ctx, "INSERT INTO users_oauth (user_id, remote_user_id) VALUES (?, ?) "+db.upsert("user_id"), localUserID, remoteUserID) + _, err = db.ExecContext(ctx, "INSERT INTO users_oauth (user_id, remote_user_id) VALUES (?, ?) "+db.upsert("user_id") + " user_id = ?", localUserID, remoteUserID, localUserID) } if err != nil { log.Error("Unable to INSERT users_oauth for '%d': %v", localUserID, err) diff --git a/database_test.go b/database_test.go new file mode 100644 index 0000000..4a45dd0 --- /dev/null +++ b/database_test.go @@ -0,0 +1,43 @@ +package writefreely + +import ( + "context" + "database/sql" + "github.com/stretchr/testify/assert" + "testing" +) + +func TestOAuthDatastore(t *testing.T) { + if !runMySQLTests() { + t.Skip("skipping mysql tests") + } + withTestDB(t, func(db *sql.DB) { + ctx := context.Background() + ds := &datastore{ + DB: db, + driverName: "", + } + + state, err := ds.GenerateOAuthState(ctx) + assert.NoError(t, err) + assert.Len(t, state, 24) + + countRows(t, ctx, db, 1, "SELECT COUNT(*) FROM `oauth_client_state` WHERE `state` = ? AND `used` = false", state) + + err = ds.ValidateOAuthState(ctx, state) + assert.NoError(t, err) + + countRows(t, ctx, db, 1, "SELECT COUNT(*) FROM `oauth_client_state` WHERE `state` = ? AND `used` = true", state) + + var localUserID int64 = 99 + var remoteUserID int64 = 100 + err = ds.RecordRemoteUserID(ctx, localUserID, remoteUserID) + assert.NoError(t, err) + + countRows(t, ctx, db, 1, "SELECT COUNT(*) FROM `users_oauth` WHERE `user_id` = ? AND `remote_user_id` = ?", localUserID, remoteUserID) + + foundUserID, err := ds.GetIDForRemoteUser(ctx, remoteUserID) + assert.NoError(t, err) + assert.Equal(t, localUserID, foundUserID) + }) +} diff --git a/db/create.go b/db/create.go new file mode 100644 index 0000000..5e4f34e --- /dev/null +++ b/db/create.go @@ -0,0 +1,271 @@ +package db + +import ( + "fmt" + "strings" +) + +type DialectType int +type ColumnType int + +type OptionalInt struct { + Set bool + Value int +} + +type OptionalString struct { + Set bool + Value string +} + +type SQLBuilder interface { + ToSQL() (string, error) +} + +type Column struct { + Dialect DialectType + Name string + Nullable bool + Default OptionalString + Type ColumnType + Size OptionalInt + PrimaryKey bool +} + +type CreateTableSqlBuilder struct { + Dialect DialectType + Name string + IfNotExists bool + ColumnOrder []string + Columns map[string]*Column + Constraints []string +} + +const ( + DialectSQLite DialectType = iota + DialectMySQL DialectType = iota +) + +const ( + ColumnTypeBool ColumnType = iota + ColumnTypeSmallInt ColumnType = iota + ColumnTypeInteger ColumnType = iota + ColumnTypeChar ColumnType = iota + ColumnTypeVarChar ColumnType = iota + ColumnTypeText ColumnType = iota + ColumnTypeDateTime ColumnType = iota +) + +var _ SQLBuilder = &CreateTableSqlBuilder{} + +var UnsetSize OptionalInt = OptionalInt{Set: false, Value: 0} +var UnsetDefault OptionalString = OptionalString{Set: false, Value: ""} + +func (d DialectType) Column(name string, t ColumnType, size OptionalInt) *Column { + switch d { + case DialectSQLite: + return &Column{Dialect: DialectSQLite, Name: name, Type: t, Size: size} + case DialectMySQL: + return &Column{Dialect: DialectMySQL, Name: name, Type: t, Size: size} + default: + panic(fmt.Sprintf("unexpected dialect: %d", d)) + } +} + +func (d DialectType) Table(name string) *CreateTableSqlBuilder { + switch d { + case DialectSQLite: + return &CreateTableSqlBuilder{Dialect: DialectSQLite, Name: name} + case DialectMySQL: + return &CreateTableSqlBuilder{Dialect: DialectMySQL, Name: name} + default: + panic(fmt.Sprintf("unexpected dialect: %d", d)) + } +} + +func (d ColumnType) Format(dialect DialectType, size OptionalInt) (string, error) { + if dialect != DialectMySQL && dialect != DialectSQLite { + return "", fmt.Errorf("unsupported column type %d for dialect %d and size %v", d, dialect, size) + } + switch d { + case ColumnTypeSmallInt: + { + if dialect == DialectSQLite { + return "INTEGER", nil + } + mod := "" + if size.Set { + mod = fmt.Sprintf("(%d)", size.Value) + } + return "SMALLINT" + mod, nil + } + case ColumnTypeInteger: + { + if dialect == DialectSQLite { + return "INTEGER", nil + } + mod := "" + if size.Set { + mod = fmt.Sprintf("(%d)", size.Value) + } + return "INT" + mod, nil + } + case ColumnTypeChar: + { + if dialect == DialectSQLite { + return "TEXT", nil + } + mod := "" + if size.Set { + mod = fmt.Sprintf("(%d)", size.Value) + } + return "CHAR" + mod, nil + } + case ColumnTypeVarChar: + { + if dialect == DialectSQLite { + return "TEXT", nil + } + mod := "" + if size.Set { + mod = fmt.Sprintf("(%d)", size.Value) + } + return "VARCHAR" + mod, nil + } + case ColumnTypeBool: + { + if dialect == DialectSQLite { + return "INTEGER", nil + } + return "TINYINT(1)", nil + } + case ColumnTypeDateTime: + return "DATETIME", nil + case ColumnTypeText: + return "TEXT", nil + } + return "", fmt.Errorf("unsupported column type %d for dialect %d and size %v", d, dialect, size) +} + +func (c *Column) SetName(name string) *Column { + c.Name = name + return c +} + +func (c *Column) SetNullable(nullable bool) *Column { + c.Nullable = nullable + return c +} + +func (c *Column) SetPrimaryKey(pk bool) *Column { + c.PrimaryKey = pk + return c +} + +func (c *Column) SetDefault(value string) *Column { + c.Default = OptionalString{Set: true, Value: value} + return c +} + +func (c *Column) SetType(t ColumnType) *Column { + c.Type = t + return c +} + +func (c *Column) SetSize(size int) *Column { + c.Size = OptionalInt{Set: true, Value: size} + return c +} + +func (c *Column) String() (string, error) { + var str strings.Builder + + str.WriteString(c.Name) + + str.WriteString(" ") + typeStr, err := c.Type.Format(c.Dialect, c.Size) + if err != nil { + return "", err + } + + str.WriteString(typeStr) + + if !c.Nullable { + str.WriteString(" NOT NULL") + } + + if c.Default.Set { + str.WriteString(" DEFAULT ") + str.WriteString(c.Default.Value) + } + + if c.PrimaryKey { + str.WriteString(" PRIMARY KEY") + } + + return str.String(), nil +} + +func (b *CreateTableSqlBuilder) Column(column *Column) *CreateTableSqlBuilder { + if b.Columns == nil { + b.Columns = make(map[string]*Column) + } + b.Columns[column.Name] = column + b.ColumnOrder = append(b.ColumnOrder, column.Name) + return b +} + +func (b *CreateTableSqlBuilder) UniqueConstraint(columns ...string) *CreateTableSqlBuilder { + for _, column := range columns { + if _, ok := b.Columns[column]; !ok { + // This fails silently. + return b + } + } + b.Constraints = append(b.Constraints, fmt.Sprintf("UNIQUE(%s)", strings.Join(columns, ","))) + return b +} + +func (b *CreateTableSqlBuilder) SetIfNotExists(ine bool) *CreateTableSqlBuilder { + b.IfNotExists = ine + return b +} + +func (b *CreateTableSqlBuilder) ToSQL() (string, error) { + var str strings.Builder + + str.WriteString("CREATE TABLE ") + if b.IfNotExists { + str.WriteString("IF NOT EXISTS ") + } + str.WriteString(b.Name) + + var things []string + for _, columnName := range b.ColumnOrder { + column, ok := b.Columns[columnName] + if !ok { + return "", fmt.Errorf("column not found: %s", columnName) + } + columnStr, err := column.String() + if err != nil { + return "", err + } + things = append(things, columnStr) + } + for _, constraint := range b.Constraints { + things = append(things, constraint) + } + + if thingLen := len(things); thingLen > 0 { + str.WriteString(" ( ") + for i, thing := range things { + str.WriteString(thing) + if i < thingLen-1 { + str.WriteString(", ") + } + } + str.WriteString(" )") + } + + return str.String(), nil +} diff --git a/db/create_test.go b/db/create_test.go new file mode 100644 index 0000000..369d5c1 --- /dev/null +++ b/db/create_test.go @@ -0,0 +1,146 @@ +package db + +import ( + "github.com/stretchr/testify/assert" + "testing" +) + +func TestDialect_Column(t *testing.T) { + c1 := DialectSQLite.Column("foo", ColumnTypeBool, UnsetSize) + assert.Equal(t, DialectSQLite, c1.Dialect) + c2 := DialectMySQL.Column("foo", ColumnTypeBool, UnsetSize) + assert.Equal(t, DialectMySQL, c2.Dialect) +} + +func TestColumnType_Format(t *testing.T) { + type args struct { + dialect DialectType + size OptionalInt + } + tests := []struct { + name string + d ColumnType + args args + want string + wantErr bool + }{ + {"Sqlite bool", ColumnTypeBool, args{dialect: DialectSQLite}, "INTEGER", false}, + {"Sqlite small int", ColumnTypeSmallInt, args{dialect: DialectSQLite}, "INTEGER", false}, + {"Sqlite int", ColumnTypeInteger, args{dialect: DialectSQLite}, "INTEGER", false}, + {"Sqlite char", ColumnTypeChar, args{dialect: DialectSQLite}, "TEXT", false}, + {"Sqlite varchar", ColumnTypeVarChar, args{dialect: DialectSQLite}, "TEXT", false}, + {"Sqlite text", ColumnTypeText, args{dialect: DialectSQLite}, "TEXT", false}, + {"Sqlite datetime", ColumnTypeDateTime, args{dialect: DialectSQLite}, "DATETIME", false}, + + {"MySQL bool", ColumnTypeBool, args{dialect: DialectMySQL}, "TINYINT(1)", false}, + {"MySQL small int", ColumnTypeSmallInt, args{dialect: DialectMySQL}, "SMALLINT", false}, + {"MySQL small int with param", ColumnTypeSmallInt, args{dialect: DialectMySQL, size: OptionalInt{true, 3}}, "SMALLINT(3)", false}, + {"MySQL int", ColumnTypeInteger, args{dialect: DialectMySQL}, "INT", false}, + {"MySQL int with param", ColumnTypeInteger, args{dialect: DialectMySQL, size: OptionalInt{true, 11}}, "INT(11)", false}, + {"MySQL char", ColumnTypeChar, args{dialect: DialectMySQL}, "CHAR", false}, + {"MySQL char with param", ColumnTypeChar, args{dialect: DialectMySQL, size: OptionalInt{true, 4}}, "CHAR(4)", false}, + {"MySQL varchar", ColumnTypeVarChar, args{dialect: DialectMySQL}, "VARCHAR", false}, + {"MySQL varchar with param", ColumnTypeVarChar, args{dialect: DialectMySQL, size: OptionalInt{true, 25}}, "VARCHAR(25)", false}, + {"MySQL text", ColumnTypeText, args{dialect: DialectMySQL}, "TEXT", false}, + {"MySQL datetime", ColumnTypeDateTime, args{dialect: DialectMySQL}, "DATETIME", false}, + + {"invalid column type", 10000, args{dialect: DialectMySQL}, "", true}, + {"invalid dialect", ColumnTypeBool, args{dialect: 10000}, "", true}, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + got, err := tt.d.Format(tt.args.dialect, tt.args.size) + if (err != nil) != tt.wantErr { + t.Errorf("Format() error = %v, wantErr %v", err, tt.wantErr) + return + } + if got != tt.want { + t.Errorf("Format() got = %v, want %v", got, tt.want) + } + }) + } +} + +func TestColumn_Build(t *testing.T) { + type fields struct { + Dialect DialectType + Name string + Nullable bool + Default OptionalString + Type ColumnType + Size OptionalInt + PrimaryKey bool + } + tests := []struct { + name string + fields fields + want string + wantErr bool + }{ + {"Sqlite bool", fields{DialectSQLite, "foo", false, UnsetDefault, ColumnTypeBool, UnsetSize, false}, "foo INTEGER NOT NULL", false}, + {"Sqlite bool nullable", fields{DialectSQLite, "foo", true, UnsetDefault, ColumnTypeBool, UnsetSize, false}, "foo INTEGER", false}, + {"Sqlite small int", fields{DialectSQLite, "foo", false, UnsetDefault, ColumnTypeSmallInt, UnsetSize, true}, "foo INTEGER NOT NULL PRIMARY KEY", false}, + {"Sqlite small int nullable", fields{DialectSQLite, "foo", true, UnsetDefault, ColumnTypeSmallInt, UnsetSize, false}, "foo INTEGER", false}, + {"Sqlite int", fields{DialectSQLite, "foo", false, UnsetDefault, ColumnTypeInteger, UnsetSize, false}, "foo INTEGER NOT NULL", false}, + {"Sqlite int nullable", fields{DialectSQLite, "foo", true, UnsetDefault, ColumnTypeInteger, UnsetSize, false}, "foo INTEGER", false}, + {"Sqlite char", fields{DialectSQLite, "foo", false, UnsetDefault, ColumnTypeChar, UnsetSize, false}, "foo TEXT NOT NULL", false}, + {"Sqlite char nullable", fields{DialectSQLite, "foo", true, UnsetDefault, ColumnTypeChar, UnsetSize, false}, "foo TEXT", false}, + {"Sqlite varchar", fields{DialectSQLite, "foo", false, UnsetDefault, ColumnTypeVarChar, UnsetSize, false}, "foo TEXT NOT NULL", false}, + {"Sqlite varchar nullable", fields{DialectSQLite, "foo", true, UnsetDefault, ColumnTypeVarChar, UnsetSize, false}, "foo TEXT", false}, + {"Sqlite text", fields{DialectSQLite, "foo", false, UnsetDefault, ColumnTypeText, UnsetSize, false}, "foo TEXT NOT NULL", false}, + {"Sqlite text nullable", fields{DialectSQLite, "foo", true, UnsetDefault, ColumnTypeText, UnsetSize, false}, "foo TEXT", false}, + {"Sqlite datetime", fields{DialectSQLite, "foo", false, UnsetDefault, ColumnTypeDateTime, UnsetSize, false}, "foo DATETIME NOT NULL", false}, + {"Sqlite datetime nullable", fields{DialectSQLite, "foo", true, UnsetDefault, ColumnTypeDateTime, UnsetSize, false}, "foo DATETIME", false}, + + {"MySQL bool", fields{DialectMySQL, "foo", false, UnsetDefault, ColumnTypeBool, UnsetSize, false}, "foo TINYINT(1) NOT NULL", false}, + {"MySQL bool nullable", fields{DialectMySQL, "foo", true, UnsetDefault, ColumnTypeBool, UnsetSize, false}, "foo TINYINT(1)", false}, + {"MySQL small int", fields{DialectMySQL, "foo", false, UnsetDefault, ColumnTypeSmallInt, UnsetSize, true}, "foo SMALLINT NOT NULL PRIMARY KEY", false}, + {"MySQL small int nullable", fields{DialectMySQL, "foo", true, UnsetDefault, ColumnTypeSmallInt, UnsetSize, false}, "foo SMALLINT", false}, + {"MySQL int", fields{DialectMySQL, "foo", false, UnsetDefault, ColumnTypeInteger, UnsetSize, false}, "foo INT NOT NULL", false}, + {"MySQL int nullable", fields{DialectMySQL, "foo", true, UnsetDefault, ColumnTypeInteger, UnsetSize, false}, "foo INT", false}, + {"MySQL char", fields{DialectMySQL, "foo", false, UnsetDefault, ColumnTypeChar, UnsetSize, false}, "foo CHAR NOT NULL", false}, + {"MySQL char nullable", fields{DialectMySQL, "foo", true, UnsetDefault, ColumnTypeChar, UnsetSize, false}, "foo CHAR", false}, + {"MySQL varchar", fields{DialectMySQL, "foo", false, UnsetDefault, ColumnTypeVarChar, UnsetSize, false}, "foo VARCHAR NOT NULL", false}, + {"MySQL varchar nullable", fields{DialectMySQL, "foo", true, UnsetDefault, ColumnTypeVarChar, UnsetSize, false}, "foo VARCHAR", false}, + {"MySQL text", fields{DialectMySQL, "foo", false, UnsetDefault, ColumnTypeText, UnsetSize, false}, "foo TEXT NOT NULL", false}, + {"MySQL text nullable", fields{DialectMySQL, "foo", true, UnsetDefault, ColumnTypeText, UnsetSize, false}, "foo TEXT", false}, + {"MySQL datetime", fields{DialectMySQL, "foo", false, UnsetDefault, ColumnTypeDateTime, UnsetSize, false}, "foo DATETIME NOT NULL", false}, + {"MySQL datetime nullable", fields{DialectMySQL, "foo", true, UnsetDefault, ColumnTypeDateTime, UnsetSize, false}, "foo DATETIME", false}, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + c := &Column{ + Dialect: tt.fields.Dialect, + Name: tt.fields.Name, + Nullable: tt.fields.Nullable, + Default: tt.fields.Default, + Type: tt.fields.Type, + Size: tt.fields.Size, + PrimaryKey: tt.fields.PrimaryKey, + } + if got, err := c.String(); got != tt.want { + if (err != nil) != tt.wantErr { + t.Errorf("String() error = %v, wantErr %v", err, tt.wantErr) + return + } + if got != tt.want { + t.Errorf("String() got = %v, want %v", got, tt.want) + } + } + }) + } +} + +func TestCreateTableSqlBuilder_ToSQL(t *testing.T) { + sql, err := DialectMySQL. + Table("foo"). + SetIfNotExists(true). + Column(DialectMySQL.Column("bar", ColumnTypeInteger, UnsetSize).SetPrimaryKey(true)). + Column(DialectMySQL.Column("baz", ColumnTypeText, UnsetSize)). + Column(DialectMySQL.Column("qux", ColumnTypeDateTime, UnsetSize).SetDefault("NOW()")). + UniqueConstraint("bar"). + UniqueConstraint("bar", "baz"). + ToSQL() + assert.NoError(t, err) + assert.Equal(t, "CREATE TABLE IF NOT EXISTS foo ( bar INT NOT NULL PRIMARY KEY, baz TEXT NOT NULL, qux DATETIME NOT NULL DEFAULT NOW(), UNIQUE(bar), UNIQUE(bar,baz) )", sql) +} diff --git a/db/tx.go b/db/tx.go new file mode 100644 index 0000000..5c321af --- /dev/null +++ b/db/tx.go @@ -0,0 +1,26 @@ +package db + +import ( + "context" + "database/sql" +) + +// TransactionScopedWork describes code executed within a database transaction. +type TransactionScopedWork func(ctx context.Context, db *sql.Tx) error + +// RunTransactionWithOptions executes a block of code within a database transaction. +func RunTransactionWithOptions(ctx context.Context, db *sql.DB, txOpts *sql.TxOptions, txWork TransactionScopedWork) error { + tx, err := db.BeginTx(ctx, txOpts) + if err != nil { + return err + } + + if err = txWork(ctx, tx); err != nil { + if txErr := tx.Rollback(); txErr != nil { + return txErr + } + return err + } + return tx.Commit() +} + diff --git a/go.mod b/go.mod index d34c4c9..372b7ba 100644 --- a/go.mod +++ b/go.mod @@ -49,7 +49,7 @@ require ( golang.org/x/lint v0.0.0-20181217174547-8f45f776aaf1 // indirect golang.org/x/net v0.0.0-20190206173232-65e2d4e15006 // indirect golang.org/x/sys v0.0.0-20190209173611-3b5209105503 // indirect - golang.org/x/tools v0.0.0-20190208222737-3744606dbb67 // indirect + golang.org/x/tools v0.0.0-20190208222737-3744606dbb67 google.golang.org/appengine v1.4.0 // indirect gopkg.in/alecthomas/kingpin.v3-unstable v3.0.0-20180810215634-df19058c872c // indirect gopkg.in/ini.v1 v1.41.0 diff --git a/go.sum b/go.sum index 035538e..ee3a418 100644 --- a/go.sum +++ b/go.sum @@ -64,7 +64,6 @@ github.com/jtolds/gls v4.2.1+incompatible h1:fSuqC+Gmlu6l/ZYAoZzx2pyucC8Xza35fpR github.com/jtolds/gls v4.2.1+incompatible/go.mod h1:QJZ7F/aHp+rZTRtaJ1ow/lLfFfVYBRgL+9YlvaHOwJU= github.com/juju/ansiterm v0.0.0-20180109212912-720a0952cc2a h1:FaWFmfWdAUKbSCtOU2QjDaorUexogfaMgbipgYATUMU= github.com/juju/ansiterm v0.0.0-20180109212912-720a0952cc2a/go.mod h1:UJSiEoRfvx3hP73CvoARgeLjaIOjybY9vj8PUPPFGeU= -github.com/kr/pretty v0.1.0 h1:L/CwN0zerZDmRFUapSPitk6f+Q3+0za1rQkzVuMiMFI= github.com/kr/pretty v0.1.0/go.mod h1:dAy3ld7l9f0ibDNOQOHHMYYIIbhfbHSm3C4ZsoJORNo= github.com/kr/pty v1.1.1/go.mod h1:pFQYn66WHrOpPYNljwOMqo10TkYh1fy3cYio2l3bCsQ= github.com/kr/text v0.1.0 h1:45sCR5RtlFHMR4UwH9sdQ5TC8v0qDQCHnXt+kaKSTVE= diff --git a/main_test.go b/main_test.go index 3d16ece..d600d83 100644 --- a/main_test.go +++ b/main_test.go @@ -1,18 +1,153 @@ package writefreely import ( + "context" + "database/sql" "encoding/gob" + "errors" + "fmt" + uuid "github.com/nu7hatch/gouuid" + "github.com/stretchr/testify/assert" "math/rand" "os" + "strings" "testing" "time" ) +var testDB *sql.DB + +type ScopedTestBody func(*sql.DB) + // TestMain provides testing infrastructure within this package. func TestMain(m *testing.M) { rand.Seed(time.Now().UTC().UnixNano()) - gob.Register(&User{}) - os.Exit(m.Run()) + if runMySQLTests() { + var err error + + testDB, err = initMySQL(os.Getenv("WF_USER"), os.Getenv("WF_PASSWORD"), os.Getenv("WF_DB"), os.Getenv("WF_HOST")) + if err != nil { + fmt.Println(err) + return + } + } + + code := m.Run() + if runMySQLTests() { + if closeErr := testDB.Close(); closeErr != nil { + fmt.Println(closeErr) + } + } + os.Exit(code) +} + +func runMySQLTests() bool { + return len(os.Getenv("TEST_MYSQL")) > 0 +} + +func initMySQL(dbUser, dbPassword, dbName, dbHost string) (*sql.DB, error) { + if dbUser == "" || dbPassword == "" { + return nil, errors.New("database user or password not set") + } + if dbHost == "" { + dbHost = "localhost" + } + if dbName == "" { + dbName = "writefreely" + } + + dsn := fmt.Sprintf("%s:%s@tcp(%s:3306)/%s?charset=utf8mb4&parseTime=true", dbUser, dbPassword, dbHost, dbName) + db, err := sql.Open("mysql", dsn) + if err != nil { + return nil, err + } + if err := ensureMySQL(db); err != nil { + return nil, err + } + return db, nil +} + +func ensureMySQL(db *sql.DB) error { + if err := db.Ping(); err != nil { + return err + } + db.SetMaxOpenConns(250) + return nil +} + +// withTestDB provides a scoped database connection. +func withTestDB(t *testing.T, testBody ScopedTestBody) { + db, cleanup, err := newTestDatabase(testDB, + os.Getenv("WF_USER"), + os.Getenv("WF_PASSWORD"), + os.Getenv("WF_DB"), + os.Getenv("WF_HOST"), + ) + assert.NoError(t, err) + defer func() { + assert.NoError(t, cleanup()) + }() + + testBody(db) +} + +// newTestDatabase creates a new temporary test database. When a test +// database connection is returned, it will have created a new database and +// initialized it with tables from a reference database. +func newTestDatabase(base *sql.DB, dbUser, dbPassword, dbName, dbHost string) (*sql.DB, func() error, error) { + var err error + var baseName = dbName + + if baseName == "" { + row := base.QueryRow("SELECT DATABASE()") + err := row.Scan(&baseName) + if err != nil { + return nil, nil, err + } + } + tUUID, _ := uuid.NewV4() + suffix := strings.Replace(tUUID.String(), "-", "_", -1) + newDBName := baseName + suffix + _, err = base.Exec("CREATE DATABASE " + newDBName) + if err != nil { + return nil, nil, err + } + newDB, err := initMySQL(dbUser, dbPassword, newDBName, dbHost) + if err != nil { + return nil, nil, err + } + + rows, err := base.Query("SHOW TABLES IN " + baseName) + if err != nil { + return nil, nil, err + } + for rows.Next() { + var tableName string + if err := rows.Scan(&tableName); err != nil { + return nil, nil, err + } + query := fmt.Sprintf("CREATE TABLE %s LIKE %s.%s", tableName, baseName, tableName) + if _, err := newDB.Exec(query); err != nil { + return nil, nil, err + } + } + + cleanup := func() error { + if closeErr := newDB.Close(); closeErr != nil { + fmt.Println(closeErr) + } + + _, err = base.Exec("DROP DATABASE " + newDBName) + return err + } + return newDB, cleanup, nil +} + +func countRows(t *testing.T, ctx context.Context, db *sql.DB, count int, query string, args ...interface{}) { + var returned int + err := db.QueryRowContext(ctx, query, args...).Scan(&returned) + assert.NoError(t, err, "error executing query %s and args %s", query, args) + assert.Equal(t, count, returned, "unexpected return count %d, expected %d from %s and args %s", returned, count, query, args) } \ No newline at end of file diff --git a/migrations/migrations.go b/migrations/migrations.go index 0799f8e..b9ead23 100644 --- a/migrations/migrations.go +++ b/migrations/migrations.go @@ -59,6 +59,7 @@ var migrations = []Migration{ New("support user invites", supportUserInvites), // -> V1 (v0.8.0) New("support dynamic instance pages", supportInstancePages), // V1 -> V2 (v0.9.0) New("support users suspension", supportUserStatus), // V2 -> V3 (v0.11.0) + New("support oauth", oauth), // V3 -> V4 } // CurrentVer returns the current migration version the application is on diff --git a/migrations/v4.go b/migrations/v4.go new file mode 100644 index 0000000..c123f54 --- /dev/null +++ b/migrations/v4.go @@ -0,0 +1,46 @@ +package migrations + +import ( + "context" + "database/sql" + + wf_db "github.com/writeas/writefreely/db" +) + +func oauth(db *datastore) error { + dialect := wf_db.DialectMySQL + if db.driverName == driverSQLite { + dialect = wf_db.DialectSQLite + } + return wf_db.RunTransactionWithOptions(context.Background(), db.DB, &sql.TxOptions{}, func(ctx context.Context, tx *sql.Tx) error { + createTableUsersOauth, err := dialect. + Table("users_oauth"). + SetIfNotExists(true). + Column(dialect.Column("user_id", wf_db.ColumnTypeInteger, wf_db.UnsetSize)). + Column(dialect.Column("remote_user_id", wf_db.ColumnTypeInteger, wf_db.UnsetSize)). + UniqueConstraint("user_id"). + UniqueConstraint("remote_user_id"). + ToSQL() + if err != nil { + return err + } + createTableOauthClientState, err := dialect. + Table("oauth_client_state"). + SetIfNotExists(true). + Column(dialect.Column("state", wf_db.ColumnTypeVarChar, wf_db.OptionalInt{Set: true, Value: 255})). + Column(dialect.Column("used", wf_db.ColumnTypeBool, wf_db.UnsetSize)). + Column(dialect.Column("created_at", wf_db.ColumnTypeDateTime, wf_db.UnsetSize).SetDefault("NOW()")). + UniqueConstraint("state"). + ToSQL() + if err != nil { + return err + } + + for _, table := range []string{createTableUsersOauth, createTableOauthClientState} { + if _, err := tx.ExecContext(ctx, table); err != nil { + return err + } + } + return nil + }) +} diff --git a/oauth.go b/oauth.go index 4b37277..aafdb51 100644 --- a/oauth.go +++ b/oauth.go @@ -3,9 +3,9 @@ package writefreely import ( "context" "encoding/json" + "fmt" "github.com/gorilla/sessions" "github.com/guregu/null/zero" - "github.com/writeas/impart" "github.com/writeas/web-core/auth" "github.com/writeas/web-core/log" "github.com/writeas/writefreely/config" @@ -67,12 +67,15 @@ type HttpClient interface { } type oauthHandler struct { + Config *config.Config + DB OAuthDatastore + Store sessions.Store HttpClient HttpClient } // buildAuthURL returns a URL used to initiate authentication. -func buildAuthURL(app OAuthDatastoreProvider, ctx context.Context, clientID, authLocation, callbackURL string) (string, error) { - state, err := app.DB().GenerateOAuthState(ctx) +func buildAuthURL(db OAuthDatastore, ctx context.Context, clientID, authLocation, callbackURL string) (string, error) { + state, err := db.GenerateOAuthState(ctx) if err != nil { return "", err } @@ -91,44 +94,50 @@ func buildAuthURL(app OAuthDatastoreProvider, ctx context.Context, clientID, aut return u.String(), nil } -func (h oauthHandler) viewOauthInit(app OAuthDatastoreProvider, w http.ResponseWriter, r *http.Request) error { - location, err := buildAuthURL(app, r.Context(), app.Config().App.OAuthClientID, app.Config().App.OAuthProviderAuthLocation, app.Config().App.OAuthClientCallbackLocation) +// app *App, w http.ResponseWriter, r *http.Request +func (h oauthHandler) viewOauthInit(w http.ResponseWriter, r *http.Request) { + location, err := buildAuthURL(h.DB, r.Context(), h.Config.App.OAuthClientID, h.Config.App.OAuthProviderAuthLocation, h.Config.App.OAuthClientCallbackLocation) if err != nil { - log.ErrorLog.Println(err) - return impart.HTTPError{Status: http.StatusInternalServerError, Message: "Could not prepare OAuth redirect URL."} + failOAuthRequest(w, http.StatusInternalServerError, "could not prepare oauth redirect url") + return } http.Redirect(w, r, location, http.StatusTemporaryRedirect) - return nil } -func (h oauthHandler) viewOauthCallback(app OAuthDatastoreProvider, w http.ResponseWriter, r *http.Request) error { +func (h oauthHandler) viewOauthCallback(w http.ResponseWriter, r *http.Request) { ctx := r.Context() code := r.FormValue("code") state := r.FormValue("state") - err := app.DB().ValidateOAuthState(ctx, state) + err := h.DB.ValidateOAuthState(ctx, state) if err != nil { - return err + failOAuthRequest(w, http.StatusInternalServerError, err.Error()) + return } - tokenResponse, err := h.exchangeOauthCode(app, ctx, code) + tokenResponse, err := h.exchangeOauthCode(ctx, code) if err != nil { - return err + failOAuthRequest(w, http.StatusInternalServerError, err.Error()) + return } // Now that we have the access token, let's use it real quick to make sur // it really really works. - tokenInfo, err := h.inspectOauthAccessToken(app, ctx, tokenResponse.AccessToken) + tokenInfo, err := h.inspectOauthAccessToken(ctx, tokenResponse.AccessToken) if err != nil { - return err + failOAuthRequest(w, http.StatusInternalServerError, err.Error()) + return } - localUserID, err := app.DB().GetIDForRemoteUser(ctx, tokenInfo.UserID) + localUserID, err := h.DB.GetIDForRemoteUser(ctx, tokenInfo.UserID) if err != nil { - return err + failOAuthRequest(w, http.StatusInternalServerError, err.Error()) + return } + fmt.Println("local user id", localUserID) + if localUserID == -1 { // We don't have, nor do we want, the password from the origin, so we //create a random string. If the user needs to set a password, they @@ -136,12 +145,14 @@ func (h oauthHandler) viewOauthCallback(app OAuthDatastoreProvider, w http.Respo //flow. randPass, err := randString(14) if err != nil { - return err + failOAuthRequest(w, http.StatusInternalServerError, err.Error()) + return } hashedPass, err := auth.HashPass([]byte(randPass)) if err != nil { log.ErrorLog.Println(err) - return impart.HTTPError{http.StatusInternalServerError, "Could not create password hash."} + failOAuthRequest(w, http.StatusInternalServerError, "unable to create password hash") + return } newUser := &User{ Username: tokenInfo.Username, @@ -151,32 +162,40 @@ func (h oauthHandler) viewOauthCallback(app OAuthDatastoreProvider, w http.Respo Created: time.Now().Truncate(time.Second).UTC(), } - err = app.DB().CreateUser(app.Config(), newUser, newUser.Username) + err = h.DB.CreateUser(h.Config, newUser, newUser.Username) if err != nil { - return err + failOAuthRequest(w, http.StatusInternalServerError, err.Error()) + return } - err = app.DB().RecordRemoteUserID(ctx, newUser.ID, tokenInfo.UserID) + err = h.DB.RecordRemoteUserID(ctx, newUser.ID, tokenInfo.UserID) if err != nil { - return err + failOAuthRequest(w, http.StatusInternalServerError, err.Error()) + return } - return loginOrFail(app, w, r, newUser) + if err := loginOrFail(h.Store, w, r, newUser); err != nil { + failOAuthRequest(w, http.StatusInternalServerError, err.Error()) + } + return } - user, err := app.DB().GetUserForAuthByID(localUserID) + user, err := h.DB.GetUserForAuthByID(localUserID) if err != nil { - return err + failOAuthRequest(w, http.StatusInternalServerError, err.Error()) + return + } + if err = loginOrFail(h.Store, w, r, user); err != nil { + failOAuthRequest(w, http.StatusInternalServerError, err.Error()) } - return loginOrFail(app, w, r, user) } -func (h oauthHandler) exchangeOauthCode(app OAuthDatastoreProvider, ctx context.Context, code string) (*TokenResponse, error) { +func (h oauthHandler) exchangeOauthCode(ctx context.Context, code string) (*TokenResponse, error) { form := url.Values{} form.Add("grant_type", "authorization_code") - form.Add("redirect_uri", app.Config().App.OAuthClientCallbackLocation) + form.Add("redirect_uri", h.Config.App.OAuthClientCallbackLocation) form.Add("code", code) - req, err := http.NewRequest("POST", app.Config().App.OAuthProviderTokenLocation, strings.NewReader(form.Encode())) + req, err := http.NewRequest("POST", h.Config.App.OAuthProviderTokenLocation, strings.NewReader(form.Encode())) if err != nil { return nil, err } @@ -184,7 +203,7 @@ func (h oauthHandler) exchangeOauthCode(app OAuthDatastoreProvider, ctx context. req.Header.Set("User-Agent", "writefreely") req.Header.Set("Accept", "application/json") req.Header.Set("Content-Type", "application/x-www-form-urlencoded") - req.SetBasicAuth(app.Config().App.OAuthClientID, app.Config().App.OAuthClientSecret) + req.SetBasicAuth(h.Config.App.OAuthClientID, h.Config.App.OAuthClientSecret) resp, err := h.HttpClient.Do(req) if err != nil { @@ -207,8 +226,8 @@ func (h oauthHandler) exchangeOauthCode(app OAuthDatastoreProvider, ctx context. return &tokenResponse, nil } -func (h oauthHandler) inspectOauthAccessToken(app OAuthDatastoreProvider, ctx context.Context, accessToken string) (*InspectResponse, error) { - req, err := http.NewRequest("GET", app.Config().App.OAuthProviderInspectLocation, nil) +func (h oauthHandler) inspectOauthAccessToken(ctx context.Context, accessToken string) (*InspectResponse, error) { + req, err := http.NewRequest("GET", h.Config.App.OAuthProviderInspectLocation, nil) if err != nil { return nil, err } @@ -238,15 +257,27 @@ func (h oauthHandler) inspectOauthAccessToken(app OAuthDatastoreProvider, ctx co return &inspectResponse, nil } -func loginOrFail(app OAuthDatastoreProvider, w http.ResponseWriter, r *http.Request, user *User) error { - session, err := app.SessionStore().Get(r, cookieName) - if err != nil { - return err - } +func loginOrFail(store sessions.Store, w http.ResponseWriter, r *http.Request, user *User) error { + // An error may be returned, but a valid session should always be returned. + session, _ := store.Get(r, cookieName) session.Values[cookieUserVal] = user.Cookie() - if err = session.Save(r, w); err != nil { + if err := session.Save(r, w); err != nil { + fmt.Println("error saving session", err) return err } 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 02e357d..9418721 100644 --- a/oauth_test.go +++ b/oauth_test.go @@ -5,7 +5,6 @@ import ( "fmt" "github.com/gorilla/sessions" "github.com/stretchr/testify/assert" - "github.com/writeas/impart" "github.com/writeas/writefreely/config" "net/http" "net/http/httptest" @@ -125,14 +124,18 @@ func (m *MockOAuthDatastore) GenerateOAuthState(ctx context.Context) (string, er } func TestViewOauthInit(t *testing.T) { - h := oauthHandler{} + t.Run("success", func(t *testing.T) { app := &MockOAuthDatastoreProvider{} + h := oauthHandler{ + Config: app.Config(), + DB: app.DB(), + Store: app.SessionStore(), + } req, err := http.NewRequest("GET", "/oauth/client", nil) assert.NoError(t, err) rr := httptest.NewRecorder() - err = h.viewOauthInit(app, rr, req) - assert.NoError(t, err) + h.viewOauthInit(rr, req) assert.Equal(t, http.StatusTemporaryRedirect, rr.Code) locURI, err := url.Parse(rr.Header().Get("Location")) assert.NoError(t, err) @@ -153,12 +156,18 @@ func TestViewOauthInit(t *testing.T) { } }, } + h := oauthHandler{ + Config: app.Config(), + DB: app.DB(), + Store: app.SessionStore(), + } req, err := http.NewRequest("GET", "/oauth/client", nil) assert.NoError(t, err) rr := httptest.NewRecorder() - err = h.viewOauthInit(app, rr, req) - assert.Error(t, err) - assert.Equal(t, impart.HTTPError{Status: http.StatusInternalServerError, Message: "Could not prepare OAuth redirect URL."}, err) + 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()) }) } @@ -166,6 +175,9 @@ func TestViewOauthCallback(t *testing.T) { t.Run("success", func(t *testing.T) { app := &MockOAuthDatastoreProvider{} h := oauthHandler{ + Config: app.Config(), + DB: app.DB(), + Store: app.SessionStore(), HttpClient: &MockHTTPClient{ DoDo: func(req *http.Request) (*http.Response, error) { switch req.URL.String() { @@ -190,7 +202,7 @@ func TestViewOauthCallback(t *testing.T) { req, err := http.NewRequest("GET", "/oauth/callback", nil) assert.NoError(t, err) rr := httptest.NewRecorder() - err = h.viewOauthCallback(app, rr, req) + h.viewOauthCallback(rr, req) assert.NoError(t, err) assert.Equal(t, http.StatusTemporaryRedirect, rr.Code) diff --git a/routes.go b/routes.go index 64c6c0f..e286c3e 100644 --- a/routes.go +++ b/routes.go @@ -80,6 +80,16 @@ func InitRoutes(apper Apper, r *mux.Router) *mux.Router { auth.HandleFunc("/read", handler.WebErrors(handleWebCollectionUnlock, UserLevelNone)).Methods("POST") auth.HandleFunc("/me", handler.All(handleAPILogout)).Methods("DELETE") + oauthHandler := oauthHandler{ + HttpClient: &http.Client{}, + Config: apper.App().Config(), + DB: apper.App().DB(), + Store: apper.App().SessionStore(), + } + + write.HandleFunc("/oauth/write.as", oauthHandler.viewOauthInit).Methods("GET") + write.HandleFunc("/oauth/callback", oauthHandler.viewOauthCallback).Methods("GET") + // Handle logged in user sections me := write.PathPrefix("/me").Subrouter() me.HandleFunc("/", handler.Redirect("/me", UserLevelUser)) From 426615474920db1e481d24ad6b5822deed1d34aa Mon Sep 17 00:00:00 2001 From: Nick Gerakines Date: Fri, 27 Dec 2019 13:35:48 -0500 Subject: [PATCH 3/6] Code cleanup from PR 255 feedback. T705 --- database.go | 35 +++-------------------------------- oauth.go | 7 ++----- oauth_test.go | 3 ++- 3 files changed, 7 insertions(+), 38 deletions(-) diff --git a/database.go b/database.go index 5b3aaa8..56035dd 100644 --- a/database.go +++ b/database.go @@ -12,11 +12,8 @@ package writefreely import ( "context" - "crypto/rand" "database/sql" "fmt" - "github.com/pkg/errors" - "math/big" "net/http" "strings" "time" @@ -2463,11 +2460,8 @@ func (db *datastore) GetCollectionLastPostTime(id int64) (*time.Time, error) { } func (db *datastore) GenerateOAuthState(ctx context.Context) (string, error) { - state, err := randString(24) - if err != nil { - return "", err - } - _, err = db.ExecContext(ctx, "INSERT INTO oauth_client_state (state, used, created_at) VALUES (?, FALSE, NOW())", state) + state := store.Generate62RandomString(24) + _, err := db.ExecContext(ctx, "INSERT INTO oauth_client_state (state, used, created_at) VALUES (?, FALSE, NOW())", state) if err != nil { return "", fmt.Errorf("unable to record oauth client state: %w", err) } @@ -2494,7 +2488,7 @@ func (db *datastore) RecordRemoteUserID(ctx context.Context, localUserID, remote if db.driverName == driverSQLite { _, err = db.ExecContext(ctx, "INSERT OR REPLACE INTO users_oauth (user_id, remote_user_id) VALUES (?, ?)", localUserID, remoteUserID) } else { - _, err = db.ExecContext(ctx, "INSERT INTO users_oauth (user_id, remote_user_id) VALUES (?, ?) "+db.upsert("user_id") + " user_id = ?", localUserID, remoteUserID, localUserID) + _, err = db.ExecContext(ctx, "INSERT INTO users_oauth (user_id, remote_user_id) VALUES (?, ?) "+db.upsert("user_id")+" user_id = ?", localUserID, remoteUserID, localUserID) } if err != nil { log.Error("Unable to INSERT users_oauth for '%d': %v", localUserID, err) @@ -2545,26 +2539,3 @@ func handleFailedPostInsert(err error) error { log.Error("Couldn't insert into posts: %v", err) return err } - -func randString(length int) (string, error) { - // every printable character on a US keyboard - charset := []rune("ABCDEFGHIJKLMNOPQRSTUVWXYZabcdefghijklmnopqrstuvwxyz0123456789") - out := make([]rune, length) - - setLen := big.NewInt(int64(len(charset))) - for idx := 0; idx < length; idx++ { - offset, err := rand.Int(rand.Reader, setLen) - if err != nil { - return "", err - } - - if !offset.IsUint64() { - // this should (in theory) never happen - return "", errors.Errorf("Non-Uint64 offset returned from rand.Int") - } - - out[idx] = charset[offset.Uint64()] - } - - return string(out), nil -} diff --git a/oauth.go b/oauth.go index aafdb51..d918f7f 100644 --- a/oauth.go +++ b/oauth.go @@ -6,6 +6,7 @@ import ( "fmt" "github.com/gorilla/sessions" "github.com/guregu/null/zero" + "github.com/writeas/nerds/store" "github.com/writeas/web-core/auth" "github.com/writeas/web-core/log" "github.com/writeas/writefreely/config" @@ -143,11 +144,7 @@ func (h oauthHandler) viewOauthCallback(w http.ResponseWriter, r *http.Request) //create a random string. If the user needs to set a password, they //can do so through the settings page or through the password reset //flow. - randPass, err := randString(14) - if err != nil { - failOAuthRequest(w, http.StatusInternalServerError, err.Error()) - return - } + randPass := store.Generate62RandomString(14) hashedPass, err := auth.HashPass([]byte(randPass)) if err != nil { log.ErrorLog.Println(err) diff --git a/oauth_test.go b/oauth_test.go index 9418721..482a846 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/nerds/store" "github.com/writeas/writefreely/config" "net/http" "net/http/httptest" @@ -120,7 +121,7 @@ func (m *MockOAuthDatastore) GenerateOAuthState(ctx context.Context) (string, er if m.DoGenerateOAuthState != nil { return m.DoGenerateOAuthState(ctx) } - return randString(14) + return store.Generate62RandomString(14), nil } func TestViewOauthInit(t *testing.T) { From 39d0f1de98310fb351641b0347610cf4dc036b33 Mon Sep 17 00:00:00 2001 From: Matt Baer Date: Mon, 30 Dec 2019 18:23:45 -0500 Subject: [PATCH 4/6] Add logging in viewOauthCallback() Ref T705 --- oauth.go | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/oauth.go b/oauth.go index d918f7f..98e0d43 100644 --- a/oauth.go +++ b/oauth.go @@ -113,12 +113,14 @@ func (h oauthHandler) viewOauthCallback(w http.ResponseWriter, r *http.Request) err := h.DB.ValidateOAuthState(ctx, state) if err != nil { + log.Error("Unable to ValidateOAuthState: %s", err) failOAuthRequest(w, http.StatusInternalServerError, err.Error()) return } tokenResponse, err := h.exchangeOauthCode(ctx, code) if err != nil { + log.Error("Unable to exchangeOauthCode: %s", err) failOAuthRequest(w, http.StatusInternalServerError, err.Error()) return } @@ -127,12 +129,14 @@ func (h oauthHandler) viewOauthCallback(w http.ResponseWriter, r *http.Request) // it really really works. tokenInfo, err := h.inspectOauthAccessToken(ctx, tokenResponse.AccessToken) if err != nil { + log.Error("Unable to inspectOauthAccessToken: %s", err) failOAuthRequest(w, http.StatusInternalServerError, err.Error()) return } localUserID, err := h.DB.GetIDForRemoteUser(ctx, tokenInfo.UserID) if err != nil { + log.Error("Unable to GetIDForRemoteUser: %s", err) failOAuthRequest(w, http.StatusInternalServerError, err.Error()) return } From 6bcc4cfa46b681f3d1341e4166ac38ae91d6068b Mon Sep 17 00:00:00 2001 From: Matt Baer Date: Mon, 30 Dec 2019 18:25:24 -0500 Subject: [PATCH 5/6] Check for error response in code exchange This checks to see if we get a response with a populated `error` field in exchangeOauthCode(). If so, we return that error message as an error, to ensure the callback logic doesn't continue with a bad response. Ref T705 --- oauth.go | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/oauth.go b/oauth.go index 98e0d43..0042433 100644 --- a/oauth.go +++ b/oauth.go @@ -25,6 +25,7 @@ type TokenResponse struct { ExpiresIn int `json:"expires_in"` RefreshToken string `json:"refresh_token"` TokenType string `json:"token_type"` + Error string `json:"error"` } // InspectResponse contains data returned when an access token is inspected. @@ -224,6 +225,11 @@ func (h oauthHandler) exchangeOauthCode(ctx context.Context, code string) (*Toke if err != nil { return nil, err } + + // Check the response for an error message, and return it if there is one. + if tokenResponse.Error != "" { + return nil, fmt.Errorf(tokenResponse.Error) + } return &tokenResponse, nil } From b5f716135b9b28cdaff706bae670571ed010b9ac Mon Sep 17 00:00:00 2001 From: Nick Gerakines Date: Tue, 31 Dec 2019 11:28:05 -0500 Subject: [PATCH 6/6] Changed oauth table names per PR feedback. T705 --- database.go | 12 ++++++------ database_test.go | 6 +++--- migrations/v4.go | 4 ++-- 3 files changed, 11 insertions(+), 11 deletions(-) diff --git a/database.go b/database.go index 56035dd..ca62d3c 100644 --- a/database.go +++ b/database.go @@ -2461,7 +2461,7 @@ func (db *datastore) GetCollectionLastPostTime(id int64) (*time.Time, error) { func (db *datastore) GenerateOAuthState(ctx context.Context) (string, error) { state := store.Generate62RandomString(24) - _, err := db.ExecContext(ctx, "INSERT INTO oauth_client_state (state, used, created_at) VALUES (?, FALSE, NOW())", state) + _, err := db.ExecContext(ctx, "INSERT INTO oauth_client_states (state, used, created_at) VALUES (?, FALSE, NOW())", state) if err != nil { return "", fmt.Errorf("unable to record oauth client state: %w", err) } @@ -2469,7 +2469,7 @@ func (db *datastore) GenerateOAuthState(ctx context.Context) (string, error) { } func (db *datastore) ValidateOAuthState(ctx context.Context, state string) error { - res, err := db.ExecContext(ctx, "UPDATE oauth_client_state SET used = TRUE WHERE state = ?", state) + res, err := db.ExecContext(ctx, "UPDATE oauth_client_states SET used = TRUE WHERE state = ?", state) if err != nil { return err } @@ -2486,12 +2486,12 @@ func (db *datastore) ValidateOAuthState(ctx context.Context, state string) error func (db *datastore) RecordRemoteUserID(ctx context.Context, localUserID, remoteUserID int64) error { var err error if db.driverName == driverSQLite { - _, err = db.ExecContext(ctx, "INSERT OR REPLACE INTO users_oauth (user_id, remote_user_id) VALUES (?, ?)", localUserID, remoteUserID) + _, err = db.ExecContext(ctx, "INSERT OR REPLACE INTO oauth_users (user_id, remote_user_id) VALUES (?, ?)", localUserID, remoteUserID) } else { - _, err = db.ExecContext(ctx, "INSERT INTO users_oauth (user_id, remote_user_id) VALUES (?, ?) "+db.upsert("user_id")+" user_id = ?", localUserID, remoteUserID, localUserID) + _, err = db.ExecContext(ctx, "INSERT INTO oauth_users (user_id, remote_user_id) VALUES (?, ?) "+db.upsert("user_id")+" user_id = ?", localUserID, remoteUserID, localUserID) } if err != nil { - log.Error("Unable to INSERT users_oauth for '%d': %v", localUserID, err) + log.Error("Unable to INSERT oauth_users for '%d': %v", localUserID, err) } return err } @@ -2500,7 +2500,7 @@ func (db *datastore) RecordRemoteUserID(ctx context.Context, localUserID, remote func (db *datastore) GetIDForRemoteUser(ctx context.Context, remoteUserID int64) (int64, error) { var userID int64 = -1 err := db. - QueryRowContext(ctx, "SELECT user_id FROM users_oauth WHERE remote_user_id = ?", remoteUserID). + QueryRowContext(ctx, "SELECT user_id FROM oauth_users WHERE remote_user_id = ?", remoteUserID). Scan(&userID) // Not finding a record is OK. if err != nil && err != sql.ErrNoRows { diff --git a/database_test.go b/database_test.go index 4a45dd0..879840e 100644 --- a/database_test.go +++ b/database_test.go @@ -22,19 +22,19 @@ func TestOAuthDatastore(t *testing.T) { assert.NoError(t, err) assert.Len(t, state, 24) - countRows(t, ctx, db, 1, "SELECT COUNT(*) FROM `oauth_client_state` WHERE `state` = ? AND `used` = false", state) + countRows(t, ctx, db, 1, "SELECT COUNT(*) FROM `oauth_client_states` WHERE `state` = ? AND `used` = false", state) err = ds.ValidateOAuthState(ctx, state) assert.NoError(t, err) - countRows(t, ctx, db, 1, "SELECT COUNT(*) FROM `oauth_client_state` WHERE `state` = ? AND `used` = true", state) + countRows(t, ctx, db, 1, "SELECT COUNT(*) FROM `oauth_client_states` WHERE `state` = ? AND `used` = true", state) var localUserID int64 = 99 var remoteUserID int64 = 100 err = ds.RecordRemoteUserID(ctx, localUserID, remoteUserID) assert.NoError(t, err) - countRows(t, ctx, db, 1, "SELECT COUNT(*) FROM `users_oauth` WHERE `user_id` = ? AND `remote_user_id` = ?", localUserID, remoteUserID) + countRows(t, ctx, db, 1, "SELECT COUNT(*) FROM `oauth_users` WHERE `user_id` = ? AND `remote_user_id` = ?", localUserID, remoteUserID) foundUserID, err := ds.GetIDForRemoteUser(ctx, remoteUserID) assert.NoError(t, err) diff --git a/migrations/v4.go b/migrations/v4.go index c123f54..c075dd8 100644 --- a/migrations/v4.go +++ b/migrations/v4.go @@ -14,7 +14,7 @@ func oauth(db *datastore) error { } return wf_db.RunTransactionWithOptions(context.Background(), db.DB, &sql.TxOptions{}, func(ctx context.Context, tx *sql.Tx) error { createTableUsersOauth, err := dialect. - Table("users_oauth"). + Table("oauth_users"). SetIfNotExists(true). Column(dialect.Column("user_id", wf_db.ColumnTypeInteger, wf_db.UnsetSize)). Column(dialect.Column("remote_user_id", wf_db.ColumnTypeInteger, wf_db.UnsetSize)). @@ -25,7 +25,7 @@ func oauth(db *datastore) error { return err } createTableOauthClientState, err := dialect. - Table("oauth_client_state"). + Table("oauth_client_states"). SetIfNotExists(true). Column(dialect.Column("state", wf_db.ColumnTypeVarChar, wf_db.OptionalInt{Set: true, Value: 255})). Column(dialect.Column("used", wf_db.ColumnTypeBool, wf_db.UnsetSize)).