diff --git a/internal/db/auth.go b/internal/db/auth.go index 4927853..03e8999 100644 --- a/internal/db/auth.go +++ b/internal/db/auth.go @@ -10,13 +10,6 @@ import ( "golang.org/x/crypto/bcrypt" ) -//nolint:gochecknoglobals // var so tests can override via SetBcryptCost -var bcryptCost = bcrypt.DefaultCost - -// SetBcryptCost overrides the bcrypt cost. -// Use bcrypt.MinCost in tests to avoid slow hashing. -func SetBcryptCost(cost int) { bcryptCost = cost } - var errNoPassword = errors.New( "account has no password set", ) @@ -29,7 +22,7 @@ func (database *Database) SetPassword( password string, ) error { hash, err := bcrypt.GenerateFromPassword( - []byte(password), bcryptCost, + []byte(password), bcrypt.DefaultCost, ) if err != nil { return fmt.Errorf("hash password: %w", err) diff --git a/internal/db/db.go b/internal/db/db.go index b669061..922ee2e 100644 --- a/internal/db/db.go +++ b/internal/db/db.go @@ -85,11 +85,6 @@ func New( return database, nil } -// GetDB returns the underlying sql.DB connection. -func (database *Database) GetDB() *sql.DB { - return database.conn -} - func (database *Database) connect(ctx context.Context) error { dbURL := database.params.Config.DBURL if dbURL == "" { diff --git a/internal/db/export_test.go b/internal/db/export_test.go index 5dd9384..f24537c 100644 --- a/internal/db/export_test.go +++ b/internal/db/export_test.go @@ -6,6 +6,7 @@ import ( "fmt" "log/slog" "testing" + "time" "sneak.berlin/go/neoirc/internal/dbtest" ) @@ -67,3 +68,78 @@ func (database *Database) ExecForTest( return nil } + +// GetDB returns the underlying sql.DB connection. +func (database *Database) GetDB() *sql.DB { + return database.conn +} + +// GetOrCreateChannel returns channel id, creating if needed. +// Uses INSERT OR IGNORE to avoid TOCTOU races. +func (database *Database) GetOrCreateChannel( + ctx context.Context, + name string, +) (int64, error) { + now := time.Now() + + _, err := database.conn.ExecContext(ctx, + `INSERT OR IGNORE INTO channels + (name, created_at, updated_at) + VALUES (?, ?, ?)`, + name, now, now) + if err != nil { + return 0, fmt.Errorf("create channel: %w", err) + } + + var channelID int64 + + err = database.conn.QueryRowContext( + ctx, + "SELECT id FROM channels WHERE name = ?", + name, + ).Scan(&channelID) + if err != nil { + return 0, fmt.Errorf("get channel: %w", err) + } + + return channelID, nil +} + +// JoinChannel adds a session to a channel. +func (database *Database) JoinChannel( + ctx context.Context, + channelID, sessionID int64, +) error { + _, err := database.conn.ExecContext(ctx, + `INSERT OR IGNORE INTO channel_members + (channel_id, session_id, joined_at) + VALUES (?, ?, ?)`, + channelID, sessionID, time.Now()) + if err != nil { + return fmt.Errorf("join channel: %w", err) + } + + return nil +} + +// HasChannelInvite checks if a session has been invited +// to a channel. +func (database *Database) HasChannelInvite( + ctx context.Context, + channelID, sessionID int64, +) (bool, error) { + var count int + + err := database.conn.QueryRowContext(ctx, + `SELECT COUNT(*) FROM channel_invites + WHERE channel_id = ? AND session_id = ?`, + channelID, sessionID, + ).Scan(&count) + if err != nil { + return false, fmt.Errorf( + "check invite: %w", err, + ) + } + + return count > 0, nil +} diff --git a/internal/db/main_test.go b/internal/db/main_test.go deleted file mode 100644 index e9cd045..0000000 --- a/internal/db/main_test.go +++ /dev/null @@ -1,14 +0,0 @@ -package db_test - -import ( - "os" - "testing" - - "golang.org/x/crypto/bcrypt" - "sneak.berlin/go/neoirc/internal/db" -) - -func TestMain(m *testing.M) { - db.SetBcryptCost(bcrypt.MinCost) - os.Exit(m.Run()) -} diff --git a/internal/db/queries.go b/internal/db/queries.go index e83e2a0..f50839d 100644 --- a/internal/db/queries.go +++ b/internal/db/queries.go @@ -320,29 +320,6 @@ type ClientHostInfo struct { Hostname string } -// GetClientHostInfo returns the IP and hostname for a -// client. -func (database *Database) GetClientHostInfo( - ctx context.Context, - clientID int64, -) (*ClientHostInfo, error) { - var info ClientHostInfo - - err := database.conn.QueryRowContext( - ctx, - `SELECT ip, hostname - FROM clients WHERE id = ?`, - clientID, - ).Scan(&info.IP, &info.Hostname) - if err != nil { - return nil, fmt.Errorf( - "get client host info: %w", err, - ) - } - - return &info, nil -} - // SetSessionOper sets the is_oper flag on a session. func (database *Database) SetSessionOper( ctx context.Context, @@ -433,54 +410,6 @@ func (database *Database) GetChannelByName( return channelID, nil } -// GetOrCreateChannel returns channel id, creating if needed. -// Uses INSERT OR IGNORE to avoid TOCTOU races. -func (database *Database) GetOrCreateChannel( - ctx context.Context, - name string, -) (int64, error) { - now := time.Now() - - _, err := database.conn.ExecContext(ctx, - `INSERT OR IGNORE INTO channels - (name, created_at, updated_at) - VALUES (?, ?, ?)`, - name, now, now) - if err != nil { - return 0, fmt.Errorf("create channel: %w", err) - } - - var channelID int64 - - err = database.conn.QueryRowContext( - ctx, - "SELECT id FROM channels WHERE name = ?", - name, - ).Scan(&channelID) - if err != nil { - return 0, fmt.Errorf("get channel: %w", err) - } - - return channelID, nil -} - -// JoinChannel adds a session to a channel. -func (database *Database) JoinChannel( - ctx context.Context, - channelID, sessionID int64, -) error { - _, err := database.conn.ExecContext(ctx, - `INSERT OR IGNORE INTO channel_members - (channel_id, session_id, joined_at) - VALUES (?, ?, ?)`, - channelID, sessionID, time.Now()) - if err != nil { - return fmt.Errorf("join channel: %w", err) - } - - return nil -} - // Errors that CreateOrJoinChannel refuses a join with. var ( ErrBannedFromChannel = errors.New("banned from channel") @@ -2460,28 +2389,6 @@ func (database *Database) AddChannelInvite( return nil } -// HasChannelInvite checks if a session has been invited -// to a channel. -func (database *Database) HasChannelInvite( - ctx context.Context, - channelID, sessionID int64, -) (bool, error) { - var count int - - err := database.conn.QueryRowContext(ctx, - `SELECT COUNT(*) FROM channel_invites - WHERE channel_id = ? AND session_id = ?`, - channelID, sessionID, - ).Scan(&count) - if err != nil { - return false, fmt.Errorf( - "check invite: %w", err, - ) - } - - return count > 0, nil -} - // --- Tier 2: Secret (+s) --- // IsChannelSecret checks if a channel has +s mode. diff --git a/internal/db/queries_test.go b/internal/db/queries_test.go index a39f88a..c44ea2a 100644 --- a/internal/db/queries_test.go +++ b/internal/db/queries_test.go @@ -125,7 +125,7 @@ func TestCreateSessionStoresIP(t *testing.T) { database := setupTestDB(t) ctx := t.Context() - sessionID, clientID, _, err := database.CreateSession( + sessionID, _, _, err := database.CreateSession( ctx, "ipuser", "ident", "host.example.com", "192.168.1.42", ) @@ -147,8 +147,8 @@ func TestCreateSessionStoresIP(t *testing.T) { ) } - clientInfo, err := database.GetClientHostInfo( - ctx, clientID, + clientInfo, err := database.GetLatestClientForSession( + ctx, sessionID, ) if err != nil { t.Fatal(err) @@ -169,19 +169,6 @@ func TestCreateSessionStoresIP(t *testing.T) { } } -func TestGetClientHostInfoNotFound(t *testing.T) { - t.Parallel() - - database := setupTestDB(t) - - _, err := database.GetClientHostInfo( - t.Context(), 99999, - ) - if err == nil { - t.Fatal("expected error for nonexistent client") - } -} - func TestGetSessionHostInfoNotFound(t *testing.T) { t.Parallel() diff --git a/internal/db/testing.go b/internal/db/testing.go deleted file mode 100644 index d036611..0000000 --- a/internal/db/testing.go +++ /dev/null @@ -1,25 +0,0 @@ -package db - -import ( - "context" - "database/sql" - "log/slog" -) - -// NewTestDatabaseFromConn creates a Database wrapping an -// existing *sql.DB connection. Intended for integration -// tests in other packages. -func NewTestDatabaseFromConn(conn *sql.DB) *Database { - return &Database{ //nolint:exhaustruct - conn: conn, - log: slog.Default(), - } -} - -// RunMigrations applies all schema migrations. Exposed -// for integration tests in other packages. -func (database *Database) RunMigrations( - ctx context.Context, -) error { - return database.runMigrations(ctx) -} diff --git a/internal/dbtest/dbtest.go b/internal/dbtest/dbtest.go index add7f03..8346bf1 100644 --- a/internal/dbtest/dbtest.go +++ b/internal/dbtest/dbtest.go @@ -1,5 +1,5 @@ -// Package dbtest gives each test a database of its own. Only tests -// may import it. +// Package dbtest gives each test a database of its own and runs raw +// SQL on it. Only tests may import it. package dbtest import ( @@ -37,3 +37,23 @@ func NewURL(tb testing.TB) string { return dbURL } + +// Exec runs one raw SQL statement on the database at dbURL, over a +// connection of its own. A test uses it for what the methods of +// db.Database cannot do, such as creating a trigger that makes a +// write fail. +func Exec(tb testing.TB, dbURL, statement string) { + tb.Helper() + + pool, err := sql.Open("sqlite", dbURL) + if err != nil { + tb.Fatalf("open test database: %v", err) + } + + defer func() { _ = pool.Close() }() + + _, err = pool.ExecContext(tb.Context(), statement) + if err != nil { + tb.Fatalf("run %q: %v", statement, err) + } +} diff --git a/internal/handlers/api_test.go b/internal/handlers/api_test.go index 305be3f..0c065e7 100644 --- a/internal/handlers/api_test.go +++ b/internal/handlers/api_test.go @@ -23,7 +23,6 @@ import ( "go.uber.org/fx" "go.uber.org/fx/fxtest" - "golang.org/x/crypto/bcrypt" "modernc.org/sqlite" "sneak.berlin/go/neoirc/internal/broker" api "sneak.berlin/go/neoirc/internal/cli/api" @@ -47,22 +46,25 @@ import ( const templateDBURL = "file:template?mode=memory&cache=shared" func TestMain(m *testing.M) { - db.SetBcryptCost(bcrypt.MinCost) + template := fx.New( + fx.NopLogger, + fx.Supply( + &globals.Globals{}, + &config.Config{DBURL: templateDBURL}, + ), + fx.Provide(logger.New, newTestDB), + // fx makes the database only if something asks for it. + fx.Invoke(func(*db.Database) {}), + ) - template, err := sql.Open("sqlite", templateDBURL) - if err != nil { - panic(err) - } - - err = db.NewTestDatabaseFromConn(template). - RunMigrations(context.Background()) + err := template.Start(context.Background()) if err != nil { panic(err) } code := m.Run() - _ = template.Close() + _ = template.Stop(context.Background()) os.Exit(code) } @@ -137,6 +139,7 @@ type testServer struct { t *testing.T fxApp *fxtest.App broker *broker.Broker + dbURL string database *db.Database handlers *handlers.Handlers } @@ -231,6 +234,7 @@ func newTestServerWith( t: t, fxApp: app, broker: brk, + dbURL: dbURL, database: database, handlers: hdlr, } @@ -1887,15 +1891,12 @@ func TestFailedLogoutChangesNothing(t *testing.T) { _, lastID := tserver.pollMessages(observerToken, 0) - _, err := tserver.database.GetDB().ExecContext(t.Context(), + dbtest.Exec(t, tserver.dbURL, `CREATE TRIGGER keep_sessions BEFORE DELETE ON sessions BEGIN SELECT RAISE(ABORT, 'sessions are kept'); END`) - if err != nil { - t.Fatal(err) - } resp, err := doRequestAuth( t, http.MethodPost, tserver.url(apiLogout), token, nil, diff --git a/internal/ircserver/server_test.go b/internal/ircserver/server_test.go index bff359c..2ca7d02 100644 --- a/internal/ircserver/server_test.go +++ b/internal/ircserver/server_test.go @@ -32,14 +32,9 @@ import ( const testTimeout = 5 * time.Second -func TestMain(m *testing.M) { - db.SetBcryptCost(4) - - os.Exit(m.Run()) -} - // testEnv holds the shared test infrastructure. type testEnv struct { + dbURL string database *db.Database brk *broker.Broker cfg *config.Config @@ -110,34 +105,27 @@ func newTestEnvWithResolver( ) *testEnv { t.Helper() - conn, err := sql.Open("sqlite", dbtest.NewURL(t)) - if err != nil { - t.Fatalf("open db: %v", err) - } + dbURL := dbtest.NewURL(t) + lifecycle := fxtest.NewLifecycle(t) - t.Cleanup(func() { - err := conn.Close() - if err != nil { - t.Logf("close db: %v", err) - } + dbLog, err := logger.New(lifecycle, logger.Params{ + Globals: &globals.Globals{}, }) - - conn.SetMaxOpenConns(1) - - _, err = conn.ExecContext( - t.Context(), "PRAGMA foreign_keys = ON", - ) if err != nil { - t.Fatalf("pragma: %v", err) + t.Fatalf("new logger: %v", err) } - database := db.NewTestDatabaseFromConn(conn) - - err = database.RunMigrations(t.Context()) + database, err := db.New(lifecycle, db.Params{ + Logger: dbLog, + Config: &config.Config{DBURL: dbURL}, + }) if err != nil { - t.Fatalf("migrate: %v", err) + t.Fatalf("new database: %v", err) } + lifecycle.RequireStart() + t.Cleanup(lifecycle.RequireStop) + brk := broker.New() log := slog.New(slog.NewTextHandler( @@ -161,6 +149,7 @@ func newTestEnvWithResolver( t.Cleanup(srv.Stop) return &testEnv{ + dbURL: dbURL, database: database, brk: brk, cfg: cfg, @@ -929,12 +918,7 @@ func quitSeenByPeers(t *testing.T, quit string) (string, []string) { t.Fatalf("create peer session: %v", err) } - channelID, err := env.database.GetChannelByName(ctx, "#bye") - if err != nil { - t.Fatalf("get channel: %v", err) - } - - err = env.database.JoinChannel(ctx, channelID, apiPeerID) + _, _, err = env.database.CreateOrJoinChannel(ctx, "#bye", apiPeerID, "") if err != nil { t.Fatalf("join peer: %v", err) } @@ -1005,12 +989,9 @@ func TestStopEndsSessions(t *testing.T) { t.Fatalf("create peer session: %v", err) } - channelID, err := env.database.GetChannelByName(ctx, "#shut") - if err != nil { - t.Fatalf("get channel: %v", err) - } - - err = env.database.JoinChannel(ctx, channelID, peerID) + channelID, _, err := env.database.CreateOrJoinChannel( + ctx, "#shut", peerID, "", + ) if err != nil { t.Fatalf("join peer: %v", err) } @@ -1068,11 +1049,16 @@ func TestStopReturnsWhileCleanupIsStuck(t *testing.T) { client.register("stuck") - // The test database has one connection. While the test - // holds it, the cleanup's first query waits for it. - held, err := env.database.GetDB().Conn(t.Context()) + // While a connection of the test's own holds a write + // transaction, the cleanup's first write waits for it to end. + holder, err := sql.Open("sqlite", env.dbURL) if err != nil { - t.Fatalf("take the database connection: %v", err) + t.Fatalf("open a second database connection: %v", err) + } + + _, err = holder.ExecContext(t.Context(), "BEGIN IMMEDIATE") + if err != nil { + t.Fatalf("begin a write transaction: %v", err) } stopped := make(chan struct{}) @@ -1088,10 +1074,11 @@ func TestStopReturnsWhileCleanupIsStuck(t *testing.T) { t.Error("Stop did not return while a cleanup was stuck") } - // Lets the cleanup finish before the database is closed. - err = held.Close() + // Closing ends the transaction, which lets the cleanup finish + // before the database is closed. + err = holder.Close() if err != nil { - t.Errorf("release the database connection: %v", err) + t.Errorf("close the second database connection: %v", err) } } @@ -1370,14 +1357,11 @@ func TestPassPostRegistrationNotStored(t *testing.T) { client.register("failpass") - _, err := env.database.GetDB().ExecContext(t.Context(), + dbtest.Exec(t, env.dbURL, `CREATE TRIGGER refuse_password BEFORE UPDATE OF password_hash ON sessions BEGIN SELECT RAISE(ABORT, 'refused'); END`, ) - if err != nil { - t.Fatalf("create trigger: %v", err) - } client.send("PASS :mypassword123") diff --git a/internal/service/service_test.go b/internal/service/service_test.go index a51710a..f27eff8 100644 --- a/internal/service/service_test.go +++ b/internal/service/service_test.go @@ -9,7 +9,6 @@ import ( "encoding/json" "errors" "fmt" - "os" "slices" "strings" "sync" @@ -18,7 +17,6 @@ import ( "go.uber.org/fx" "go.uber.org/fx/fxtest" - "golang.org/x/crypto/bcrypt" "sneak.berlin/go/neoirc/internal/broker" "sneak.berlin/go/neoirc/internal/config" "sneak.berlin/go/neoirc/internal/db" @@ -30,14 +28,10 @@ import ( "sneak.berlin/go/neoirc/pkg/irc" ) -func TestMain(m *testing.M) { - db.SetBcryptCost(bcrypt.MinCost) - os.Exit(m.Run()) -} - // testEnv holds all dependencies for a service test. type testEnv struct { svc *service.Service + dbURL string db *db.Database broker *broker.Broker app *fxtest.App @@ -111,6 +105,7 @@ func newTestEnv(t *testing.T) *testEnv { return &testEnv{ svc: svc, + dbURL: dbURL, db: database, broker: brk, app: app, @@ -940,19 +935,16 @@ func TestSendDirectMessageTargetQuits(t *testing.T) { sid := createSession(ctx, t, env.db, "alice") createSession(ctx, t, env.db, "bob") - _, err := env.db.GetDB().ExecContext(ctx, + dbtest.Exec(t, env.dbURL, `CREATE TRIGGER delete_recipient BEFORE INSERT ON messages BEGIN DELETE FROM sessions WHERE id = NEW.to_session_id; END`) - if err != nil { - t.Fatalf("create trigger: %v", err) - } body, _ := json.Marshal([]string{"hi"}) //nolint:errchkjson - _, err = env.svc.SendDirectMessage( + _, err := env.svc.SendDirectMessage( ctx, sid, 0, "alice", irc.CmdPrivmsg, "bob", body, nil, ) @@ -978,15 +970,12 @@ func TestSendChannelMessageChannelDeleted(t *testing.T) { t.Fatalf("join alice: %v", err) } - _, err = env.db.GetDB().ExecContext(ctx, + dbtest.Exec(t, env.dbURL, `CREATE TRIGGER delete_recipient BEFORE INSERT ON messages BEGIN DELETE FROM channels WHERE id = NEW.to_channel_id; END`) - if err != nil { - t.Fatalf("create trigger: %v", err) - } body, _ := json.Marshal([]string{"hi"}) //nolint:errchkjson