From 000bf27c877953e897012fe0aabccdebd81f06d6 Mon Sep 17 00:00:00 2001 From: binwiederhier Date: Sat, 28 Jan 2023 09:03:14 -0500 Subject: [PATCH] Speed up tests, hopefully fix races --- Makefile | 4 ++-- cmd/user.go | 2 +- server/config.go | 2 ++ server/message_cache.go | 4 ++++ server/server.go | 13 +++++++++++-- server/server_account_test.go | 33 ++++++++++++++++++++++++++++----- server/server_test.go | 6 +++++- user/manager.go | 26 +++++++++++++++----------- user/manager_test.go | 23 ++++++++++++----------- 9 files changed, 80 insertions(+), 33 deletions(-) diff --git a/Makefile b/Makefile index 32c6502c..39f4e988 100644 --- a/Makefile +++ b/Makefile @@ -232,11 +232,11 @@ test: .PHONY go test -v $(shell go list ./... | grep -vE 'ntfy/(test|examples|tools)') race: .PHONY - go test -race $(shell go list ./... | grep -vE 'ntfy/(test|examples|tools)') + go test -v -race $(shell go list ./... | grep -vE 'ntfy/(test|examples|tools)') coverage: mkdir -p build/coverage - go test -race -coverprofile=build/coverage/coverage.txt -covermode=atomic $(shell go list ./... | grep -vE 'ntfy/(test|examples|tools)') + go test -v -race -coverprofile=build/coverage/coverage.txt -covermode=atomic $(shell go list ./... | grep -vE 'ntfy/(test|examples|tools)') go tool cover -func build/coverage/coverage.txt coverage-html: diff --git a/cmd/user.go b/cmd/user.go index 9ca9f401..e7c601c2 100644 --- a/cmd/user.go +++ b/cmd/user.go @@ -330,7 +330,7 @@ func createUserManager(c *cli.Context) (*user.Manager, error) { if err != nil { return nil, errors.New("if set, auth-default-access must start set to 'read-write', 'read-only', 'write-only' or 'deny-all'") } - return user.NewManager(authFile, authStartupQueries, authDefault) + return user.NewManager(authFile, authStartupQueries, authDefault, user.DefaultUserPasswordBcryptCost, user.DefaultUserStatsQueueWriterInterval) } func readPasswordAndConfirm(c *cli.Context) (string, error) { diff --git a/server/config.go b/server/config.go index fb85670b..9ab2de91 100644 --- a/server/config.go +++ b/server/config.go @@ -76,6 +76,7 @@ type Config struct { AuthFile string AuthStartupQueries string AuthDefault user.Permission + AuthBcryptCost int AttachmentCacheDir string AttachmentTotalSizeLimit int64 AttachmentFileSizeLimit int64 @@ -143,6 +144,7 @@ func NewConfig() *Config { AuthFile: "", AuthStartupQueries: "", AuthDefault: user.NewPermission(true, true), + AuthBcryptCost: user.DefaultUserPasswordBcryptCost, AttachmentCacheDir: "", AttachmentTotalSizeLimit: DefaultAttachmentTotalSizeLimit, AttachmentFileSizeLimit: DefaultAttachmentFileSizeLimit, diff --git a/server/message_cache.go b/server/message_cache.go index b9723bde..f2b977c4 100644 --- a/server/message_cache.go +++ b/server/message_cache.go @@ -676,6 +676,10 @@ func readMessages(rows *sql.Rows) ([]*message, error) { return messages, nil } +func (c *messageCache) Close() error { + return c.db.Close() +} + func setupDB(db *sql.DB, startupQueries string, cacheDuration time.Duration) error { // Run startup queries if startupQueries != "" { diff --git a/server/server.go b/server/server.go index 5a129fc6..538109ac 100644 --- a/server/server.go +++ b/server/server.go @@ -171,7 +171,7 @@ func New(conf *Config) (*Server, error) { } var userManager *user.Manager if conf.AuthFile != "" { - userManager, err = user.NewManager(conf.AuthFile, conf.AuthStartupQueries, conf.AuthDefault) + userManager, err = user.NewManager(conf.AuthFile, conf.AuthStartupQueries, conf.AuthDefault, conf.AuthBcryptCost, user.DefaultUserStatsQueueWriterInterval) if err != nil { return nil, err } @@ -296,9 +296,17 @@ func (s *Server) Stop() { if s.smtpServer != nil { s.smtpServer.Close() } + s.closeDatabases() close(s.closeChan) } +func (s *Server) closeDatabases() { + if s.userManager != nil { + s.userManager.Close() + } + s.messageCache.Close() +} + func (s *Server) handle(w http.ResponseWriter, r *http.Request) { v, err := s.maybeAuthenticate(r) // Note: Always returns v, even when error is returned if err == nil { @@ -1567,8 +1575,9 @@ func (s *Server) autorizeTopic(next handleFunc, perm user.Permission) handleFunc if err != nil { return err } + u := v.User() for _, t := range topics { - if err := s.userManager.Authorize(v.user, t.ID, perm); err != nil { + if err := s.userManager.Authorize(u, t.ID, perm); err != nil { log.Info("unauthorized: %s", err.Error()) return errHTTPForbidden } diff --git a/server/server_account_test.go b/server/server_account_test.go index 77519e51..bd5bc2c6 100644 --- a/server/server_account_test.go +++ b/server/server_account_test.go @@ -16,6 +16,7 @@ func TestAccount_Signup_Success(t *testing.T) { conf := newTestConfigWithAuthFile(t) conf.EnableSignup = true s := newTestServer(t, conf) + defer s.closeDatabases() rr := request(t, s, "POST", "/v1/account", `{"username":"phil", "password":"mypass"}`, nil) require.Equal(t, 200, rr.Code) @@ -41,6 +42,7 @@ func TestAccount_Signup_UserExists(t *testing.T) { conf := newTestConfigWithAuthFile(t) conf.EnableSignup = true s := newTestServer(t, conf) + defer s.closeDatabases() rr := request(t, s, "POST", "/v1/account", `{"username":"phil", "password":"mypass"}`, nil) require.Equal(t, 200, rr.Code) @@ -54,6 +56,7 @@ func TestAccount_Signup_LimitReached(t *testing.T) { conf := newTestConfigWithAuthFile(t) conf.EnableSignup = true s := newTestServer(t, conf) + defer s.closeDatabases() for i := 0; i < 3; i++ { rr := request(t, s, "POST", "/v1/account", fmt.Sprintf(`{"username":"phil%d", "password":"mypass"}`, i), nil) @@ -68,15 +71,18 @@ func TestAccount_Signup_AsUser(t *testing.T) { conf := newTestConfigWithAuthFile(t) conf.EnableSignup = true s := newTestServer(t, conf) + defer s.closeDatabases() + log.Info("1") require.Nil(t, s.userManager.AddUser("phil", "phil", user.RoleAdmin)) + log.Info("2") require.Nil(t, s.userManager.AddUser("ben", "ben", user.RoleUser)) - + log.Info("3") rr := request(t, s, "POST", "/v1/account", `{"username":"emma", "password":"emma"}`, map[string]string{ "Authorization": util.BasicAuth("phil", "phil"), }) require.Equal(t, 200, rr.Code) - + log.Info("4") rr = request(t, s, "POST", "/v1/account", `{"username":"marian", "password":"marian"}`, map[string]string{ "Authorization": util.BasicAuth("ben", "ben"), }) @@ -87,6 +93,7 @@ func TestAccount_Signup_Disabled(t *testing.T) { conf := newTestConfigWithAuthFile(t) conf.EnableSignup = false s := newTestServer(t, conf) + defer s.closeDatabases() rr := request(t, s, "POST", "/v1/account", `{"username":"phil", "password":"mypass"}`, nil) require.Equal(t, 400, rr.Code) @@ -115,6 +122,7 @@ func TestAccount_Get_Anonymous(t *testing.T) { conf.AttachmentFileSizeLimit = 512 s := newTestServer(t, conf) s.smtpSender = &testMailer{} + defer s.closeDatabases() rr := request(t, s, "GET", "/v1/account", "", nil) require.Equal(t, 200, rr.Code) @@ -149,6 +157,8 @@ func TestAccount_Get_Anonymous(t *testing.T) { func TestAccount_ChangeSettings(t *testing.T) { s := newTestServer(t, newTestConfigWithAuthFile(t)) + defer s.closeDatabases() + require.Nil(t, s.userManager.AddUser("phil", "phil", user.RoleUser)) u, _ := s.userManager.User("phil") token, _ := s.userManager.CreateToken(u.ID, "", time.Unix(0, 0)) @@ -176,6 +186,8 @@ func TestAccount_ChangeSettings(t *testing.T) { func TestAccount_Subscription_AddUpdateDelete(t *testing.T) { s := newTestServer(t, newTestConfigWithAuthFile(t)) + defer s.closeDatabases() + require.Nil(t, s.userManager.AddUser("phil", "phil", user.RoleUser)) rr := request(t, s, "POST", "/v1/account/subscription", `{"base_url": "http://abc.com", "topic": "def"}`, map[string]string{ @@ -226,6 +238,8 @@ func TestAccount_Subscription_AddUpdateDelete(t *testing.T) { func TestAccount_ChangePassword(t *testing.T) { s := newTestServer(t, newTestConfigWithAuthFile(t)) + defer s.closeDatabases() + require.Nil(t, s.userManager.AddUser("phil", "phil", user.RoleUser)) rr := request(t, s, "POST", "/v1/account/password", `{"password": "phil", "new_password": "new password"}`, map[string]string{ @@ -246,6 +260,7 @@ func TestAccount_ChangePassword(t *testing.T) { func TestAccount_ChangePassword_NoAccount(t *testing.T) { s := newTestServer(t, newTestConfigWithAuthFile(t)) + defer s.closeDatabases() rr := request(t, s, "POST", "/v1/account/password", `{"password": "new password"}`, nil) require.Equal(t, 401, rr.Code) @@ -253,6 +268,8 @@ func TestAccount_ChangePassword_NoAccount(t *testing.T) { func TestAccount_ExtendToken(t *testing.T) { s := newTestServer(t, newTestConfigWithAuthFile(t)) + defer s.closeDatabases() + require.Nil(t, s.userManager.AddUser("phil", "phil", user.RoleUser)) rr := request(t, s, "POST", "/v1/account/token", "", map[string]string{ @@ -276,6 +293,8 @@ func TestAccount_ExtendToken(t *testing.T) { func TestAccount_ExtendToken_NoTokenProvided(t *testing.T) { s := newTestServer(t, newTestConfigWithAuthFile(t)) + defer s.closeDatabases() + require.Nil(t, s.userManager.AddUser("phil", "phil", user.RoleUser)) rr := request(t, s, "PATCH", "/v1/account/token", "", map[string]string{ @@ -287,6 +306,8 @@ func TestAccount_ExtendToken_NoTokenProvided(t *testing.T) { func TestAccount_DeleteToken(t *testing.T) { s := newTestServer(t, newTestConfigWithAuthFile(t)) + defer s.closeDatabases() + require.Nil(t, s.userManager.AddUser("phil", "phil", user.RoleUser)) rr := request(t, s, "POST", "/v1/account/token", "", map[string]string{ @@ -295,7 +316,6 @@ func TestAccount_DeleteToken(t *testing.T) { require.Equal(t, 200, rr.Code) token, err := util.UnmarshalJSON[apiAccountTokenResponse](io.NopCloser(rr.Body)) require.Nil(t, err) - log.Info("token = %#v", token) require.True(t, token.Expires > time.Now().Add(71*time.Hour).Unix()) // Delete token failure (using basic auth) @@ -522,6 +542,7 @@ func TestAccount_Reservation_Add_Kills_Other_Subscribers(t *testing.T) { conf.AuthDefault = user.PermissionReadWrite conf.EnableSignup = true s := newTestServer(t, conf) + defer s.closeDatabases() // Create user with tier rr := request(t, s, "POST", "/v1/account", `{"username":"phil", "password":"mypass"}`, nil) @@ -544,6 +565,7 @@ func TestAccount_Reservation_Add_Kills_Other_Subscribers(t *testing.T) { require.Equal(t, "open", messages[0].Event) require.Equal(t, "message before reservation", messages[1].Message) anonCh <- true + log.Info("Anonymous subscription ended") }() // Subscribe with user @@ -558,13 +580,14 @@ func TestAccount_Reservation_Add_Kills_Other_Subscribers(t *testing.T) { require.Equal(t, "message before reservation", messages[1].Message) require.Equal(t, "message after reservation", messages[2].Message) userCh <- true + log.Info("User subscription ended") }() // Publish message (before reservation) - time.Sleep(700 * time.Millisecond) // Wait for subscribers + time.Sleep(time.Second) // Wait for subscribers rr = request(t, s, "POST", "/mytopic", "message before reservation", nil) require.Equal(t, 200, rr.Code) - time.Sleep(700 * time.Millisecond) // Wait for subscribers to receive message + time.Sleep(time.Second) // Wait for subscribers to receive message // Reserve a topic rr = request(t, s, "POST", "/v1/account/reservation", `{"topic": "mytopic", "everyone":"deny-all"}`, map[string]string{ diff --git a/server/server_test.go b/server/server_test.go index 410473fb..3ad42ceb 100644 --- a/server/server_test.go +++ b/server/server_test.go @@ -6,6 +6,7 @@ import ( "encoding/base64" "encoding/json" "fmt" + "golang.org/x/crypto/bcrypt" "heckel.io/ntfy/user" "io" "math/rand" @@ -893,7 +894,7 @@ func TestServer_DailyMessageQuotaFromDatabase(t *testing.T) { c := newTestConfigWithAuthFile(t) s := newTestServer(t, c) var err error - s.userManager, err = user.NewManagerWithStatsInterval(c.AuthFile, c.AuthStartupQueries, c.AuthDefault, 100*time.Millisecond) + s.userManager, err = user.NewManager(c.AuthFile, c.AuthStartupQueries, c.AuthDefault, c.AuthBcryptCost, 100*time.Millisecond) require.Nil(t, err) // Create user, and update it with some message and email stats @@ -1794,6 +1795,7 @@ func newTestConfig(t *testing.T) *Config { conf := NewConfig() conf.BaseURL = "http://127.0.0.1:12345" conf.CacheFile = filepath.Join(t.TempDir(), "cache.db") + conf.CacheStartupQueries = "pragma journal_mode = WAL; pragma synchronous = normal; pragma temp_store = memory;" conf.AttachmentCacheDir = t.TempDir() return conf } @@ -1801,6 +1803,8 @@ func newTestConfig(t *testing.T) *Config { func newTestConfigWithAuthFile(t *testing.T) *Config { conf := newTestConfig(t) conf.AuthFile = filepath.Join(t.TempDir(), "user.db") + conf.AuthStartupQueries = "pragma journal_mode = WAL; pragma synchronous = normal; pragma temp_store = memory;" + conf.AuthBcryptCost = bcrypt.MinCost // This speeds up tests a lot return conf } diff --git a/user/manager.go b/user/manager.go index 4114c794..79db16d0 100644 --- a/user/manager.go +++ b/user/manager.go @@ -22,15 +22,18 @@ const ( syncTopicLength = 16 userIDPrefix = "u_" userIDLength = 12 - userPasswordBcryptCost = 10 - userAuthIntentionalSlowDownHash = "$2a$10$YFCQvqQDwIIwnJM1xkAYOeih0dg17UVGanaTStnrSzC8NCWxcLDwy" // Cost should match userPasswordBcryptCost - userStatsQueueWriterInterval = 33 * time.Second + userAuthIntentionalSlowDownHash = "$2a$10$YFCQvqQDwIIwnJM1xkAYOeih0dg17UVGanaTStnrSzC8NCWxcLDwy" // Cost should match DefaultUserPasswordBcryptCost userHardDeleteAfterDuration = 7 * 24 * time.Hour tokenPrefix = "tk_" tokenLength = 32 tokenMaxCount = 10 // Only keep this many tokens in the table per user ) +const ( + DefaultUserStatsQueueWriterInterval = 33 * time.Second + DefaultUserPasswordBcryptCost = 10 +) + var ( errNoTokenProvided = errors.New("no token provided") errTopicOwnedByOthers = errors.New("topic owned by others") @@ -296,18 +299,14 @@ type Manager struct { db *sql.DB defaultAccess Permission // Default permission if no ACL matches statsQueue map[string]*Stats // "Queue" to asynchronously write user stats to the database (UserID -> Stats) + bcryptCost int // Makes testing easier mu sync.Mutex } var _ Auther = (*Manager)(nil) // NewManager creates a new Manager instance -func NewManager(filename, startupQueries string, defaultAccess Permission) (*Manager, error) { - return NewManagerWithStatsInterval(filename, startupQueries, defaultAccess, userStatsQueueWriterInterval) -} - -// NewManagerWithStatsInterval creates a new Manager instance -func NewManagerWithStatsInterval(filename, startupQueries string, defaultAccess Permission, statsWriterInterval time.Duration) (*Manager, error) { +func NewManager(filename, startupQueries string, defaultAccess Permission, bcryptCost int, statsWriterInterval time.Duration) (*Manager, error) { db, err := sql.Open("sqlite3", filename) if err != nil { return nil, err @@ -322,6 +321,7 @@ func NewManagerWithStatsInterval(filename, startupQueries string, defaultAccess db: db, defaultAccess: defaultAccess, statsQueue: make(map[string]*Stats), + bcryptCost: bcryptCost, } go manager.userStatsQueueWriter(statsWriterInterval) return manager, nil @@ -615,7 +615,7 @@ func (a *Manager) AddUser(username, password string, role Role) error { if !AllowedUsername(username) || !AllowedRole(role) { return ErrInvalidArgument } - hash, err := bcrypt.GenerateFromPassword([]byte(password), userPasswordBcryptCost) + hash, err := bcrypt.GenerateFromPassword([]byte(password), a.bcryptCost) if err != nil { return err } @@ -871,7 +871,7 @@ func (a *Manager) ReservationsCount(username string) (int64, error) { // ChangePassword changes a user's password func (a *Manager) ChangePassword(username, password string) error { - hash, err := bcrypt.GenerateFromPassword([]byte(password), userPasswordBcryptCost) + hash, err := bcrypt.GenerateFromPassword([]byte(password), a.bcryptCost) if err != nil { return err } @@ -1144,6 +1144,10 @@ func (a *Manager) readTier(rows *sql.Rows) (*Tier, error) { }, nil } +func (a *Manager) Close() error { + return a.db.Close() +} + func toSQLWildcard(s string) string { return strings.ReplaceAll(s, "*", "%") } diff --git a/user/manager_test.go b/user/manager_test.go index e4b742f9..4c5e368b 100644 --- a/user/manager_test.go +++ b/user/manager_test.go @@ -3,6 +3,7 @@ package user import ( "database/sql" "github.com/stretchr/testify/require" + "golang.org/x/crypto/bcrypt" "heckel.io/ntfy/util" "path/filepath" "strings" @@ -13,7 +14,7 @@ import ( const minBcryptTimingMillis = int64(50) // Ideally should be >100ms, but this should also run on a Raspberry Pi without massive resources func TestManager_FullScenario_Default_DenyAll(t *testing.T) { - a := newTestManager(t, PermissionDenyAll) + a := newTestManagerFromFile(t, filepath.Join(t.TempDir(), "user.db"), "", PermissionDenyAll, DefaultUserPasswordBcryptCost, DefaultUserStatsQueueWriterInterval) require.Nil(t, a.AddUser("phil", "phil", RoleAdmin)) require.Nil(t, a.AddUser("ben", "ben", RoleUser)) require.Nil(t, a.AllowAccess("ben", "mytopic", PermissionReadWrite)) @@ -98,14 +99,14 @@ func TestManager_AddUser_Invalid(t *testing.T) { } func TestManager_AddUser_Timing(t *testing.T) { - a := newTestManager(t, PermissionDenyAll) + a := newTestManagerFromFile(t, filepath.Join(t.TempDir(), "user.db"), "", PermissionDenyAll, DefaultUserPasswordBcryptCost, DefaultUserStatsQueueWriterInterval) start := time.Now().UnixMilli() require.Nil(t, a.AddUser("user", "pass", RoleAdmin)) require.GreaterOrEqual(t, time.Now().UnixMilli()-start, minBcryptTimingMillis) } func TestManager_Authenticate_Timing(t *testing.T) { - a := newTestManager(t, PermissionDenyAll) + a := newTestManagerFromFile(t, filepath.Join(t.TempDir(), "user.db"), "", PermissionDenyAll, DefaultUserPasswordBcryptCost, DefaultUserStatsQueueWriterInterval) require.Nil(t, a.AddUser("user", "pass", RoleAdmin)) // Timing a correct attempt @@ -192,7 +193,7 @@ func TestManager_UserManagement(t *testing.T) { phil, err := a.User("phil") require.Nil(t, err) require.Equal(t, "phil", phil.Name) - require.True(t, strings.HasPrefix(phil.Hash, "$2a$10$")) + require.True(t, strings.HasPrefix(phil.Hash, "$2a$04$")) // Min cost for testing require.Equal(t, RoleAdmin, phil.Role) philGrants, err := a.Grants("phil") @@ -202,7 +203,7 @@ func TestManager_UserManagement(t *testing.T) { ben, err := a.User("ben") require.Nil(t, err) require.Equal(t, "ben", ben.Name) - require.True(t, strings.HasPrefix(ben.Hash, "$2a$10$")) + require.True(t, strings.HasPrefix(ben.Hash, "$2a$04$")) // Min cost for testing require.Equal(t, RoleUser, ben.Role) benGrants, err := a.Grants("ben") @@ -551,7 +552,7 @@ func TestManager_Token_MaxCount_AutoDelete(t *testing.T) { } func TestManager_EnqueueStats(t *testing.T) { - a, err := NewManagerWithStatsInterval(filepath.Join(t.TempDir(), "db"), "", PermissionReadWrite, 1500*time.Millisecond) + a, err := NewManager(filepath.Join(t.TempDir(), "db"), "", PermissionReadWrite, bcrypt.MinCost, 1500*time.Millisecond) require.Nil(t, err) require.Nil(t, a.AddUser("ben", "ben", RoleUser)) @@ -581,7 +582,7 @@ func TestManager_EnqueueStats(t *testing.T) { } func TestManager_ChangeSettings(t *testing.T) { - a, err := NewManagerWithStatsInterval(filepath.Join(t.TempDir(), "db"), "", PermissionReadWrite, 1500*time.Millisecond) + a, err := NewManager(filepath.Join(t.TempDir(), "db"), "", PermissionReadWrite, bcrypt.MinCost, 1500*time.Millisecond) require.Nil(t, err) require.Nil(t, a.AddUser("ben", "ben", RoleUser)) @@ -665,7 +666,7 @@ func TestSqliteCache_Migration_From1(t *testing.T) { require.Nil(t, err) // Create manager to trigger migration - a := newTestManagerFromFile(t, filename, "", PermissionDenyAll, userStatsQueueWriterInterval) + a := newTestManagerFromFile(t, filename, "", PermissionDenyAll, bcrypt.MinCost, DefaultUserStatsQueueWriterInterval) checkSchemaVersion(t, a.db) users, err := a.Users() @@ -720,11 +721,11 @@ func checkSchemaVersion(t *testing.T, db *sql.DB) { } func newTestManager(t *testing.T, defaultAccess Permission) *Manager { - return newTestManagerFromFile(t, filepath.Join(t.TempDir(), "user.db"), "", defaultAccess, userStatsQueueWriterInterval) + return newTestManagerFromFile(t, filepath.Join(t.TempDir(), "user.db"), "", defaultAccess, bcrypt.MinCost, DefaultUserStatsQueueWriterInterval) } -func newTestManagerFromFile(t *testing.T, filename, startupQueries string, defaultAccess Permission, statsWriterInterval time.Duration) *Manager { - a, err := NewManagerWithStatsInterval(filename, startupQueries, defaultAccess, statsWriterInterval) +func newTestManagerFromFile(t *testing.T, filename, startupQueries string, defaultAccess Permission, bcryptCost int, statsWriterInterval time.Duration) *Manager { + a, err := NewManager(filename, startupQueries, defaultAccess, bcryptCost, statsWriterInterval) require.Nil(t, err) return a }