Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
14 changes: 14 additions & 0 deletions internal/webserver/authentication_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -110,6 +110,20 @@ func TestAuthentication(t *testing.T) {
t.Errorf("Expected redirect to /, received %s", location.Path)
}
})

t.Run("Log in using a different email case than the one stored", func(t *testing.T) {
mixedCaseData := url.Values{
"email": {"Admin@Example.COM"},
"password": {"admin"},
}
response, err := postRequest(mixedCaseData, &http.Cookie{}, app, "/sessions", t)
if response == nil {
t.Fatalf("Unexpected error: %v", err.Error())
}
if response.StatusCode != http.StatusFound && response.StatusCode != http.StatusSeeOther {
t.Errorf("Expected status 302 or 303, received %d", response.StatusCode)
}
})
}

func TestRecoverNoEmailService(t *testing.T) {
Expand Down
4 changes: 2 additions & 2 deletions internal/webserver/controller/user/create.go
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,7 @@ func (u *Controller) Create(c fiber.Ctx) error {
user := model.User{
Name: strings.TrimSpace(c.FormValue("name")),
Username: strings.ToLower(c.FormValue("username")),
Email: c.FormValue("email"),
Email: strings.ToLower(strings.TrimSpace(c.FormValue("email"))),
Password: c.FormValue("password"),
Role: role,
Uuid: uuid.NewString(),
Expand All @@ -26,7 +26,7 @@ func (u *Controller) Create(c fiber.Ctx) error {
}

errs := user.Validate(u.config.MinPasswordLength)
if exist, _ := u.usersRepository.FindByEmail(c.FormValue("email")); exist != nil {
if exist, _ := u.usersRepository.FindByEmail(user.Email); exist != nil {
errs["email"] = "A user with this email address already exists"
}

Expand Down
7 changes: 3 additions & 4 deletions internal/webserver/controller/user/invite.go
Original file line number Diff line number Diff line change
Expand Up @@ -232,15 +232,14 @@ func parseCommaSeparatedInviteEmails(raw string) []string {
seen := make(map[string]struct{})
var out []string
for _, p := range parts {
e := strings.TrimSpace(p)
e := strings.ToLower(strings.TrimSpace(p))
if e == "" {
continue
}
key := strings.ToLower(e)
if _, ok := seen[key]; ok {
if _, ok := seen[e]; ok {
continue
}
seen[key] = struct{}{}
seen[e] = struct{}{}
out = append(out, e)
}
return out
Expand Down
2 changes: 1 addition & 1 deletion internal/webserver/controller/user/update.go
Original file line number Diff line number Diff line change
Expand Up @@ -140,7 +140,7 @@ func (u *Controller) refreshSession(session model.Session, user *model.User, c f
func (u *Controller) updateUserData(c fiber.Ctx, user *model.User, session model.Session) (map[string]string, error) {
user.Name = strings.TrimSpace(c.FormValue("name"))
user.Username = strings.ToLower(c.FormValue("username"))
user.Email = c.FormValue("email")
user.Email = strings.ToLower(strings.TrimSpace(c.FormValue("email")))

validationErrs, err := u.validate(c, user, session)
if err != nil || len(validationErrs) > 0 {
Expand Down
85 changes: 83 additions & 2 deletions internal/webserver/infrastructure/sqlite.go
Original file line number Diff line number Diff line change
Expand Up @@ -32,14 +32,95 @@ func Connect(path string, wordsPerMinute float64) *gorm.DB {
log.Fatal(err)
}

if err := db.AutoMigrate(&model.User{}, &model.Highlight{}, &model.Reading{}, &model.Invitation{}); err != nil {
log.Fatal(err)
// AutoMigrate can decide a table needs a full rebuild (e.g. to add a missing/renamed constraint), which
// briefly drops and recreates it. If another table holds an enforced foreign key into it (highlights/
// readings -> users here) and has rows actually referencing it, SQLite correctly refuses that DROP with
// foreign keys on. gorm's own AlterColumn already guards itself against this (see fixEmailCollation
// below); AutoMigrate's own constraint-adding path does not, so it's wrapped the same way here.
migrateErr := runWithoutForeignKeys(db, func() error {
return db.AutoMigrate(&model.User{}, &model.Highlight{}, &model.Reading{}, &model.Invitation{})
})
if migrateErr != nil {
log.Fatal(migrateErr)
}
fixEmailCollation(db, "users", &model.User{})
fixEmailCollation(db, "invitations", &model.Invitation{})
applySQLiteIndexes(db)
addDefaultAdmin(db, wordsPerMinute)
return db
}

// fixEmailCollation brings table's email column in line with today's schema (declared "collate nocase", see
// model.User/model.Invitation) for databases created before that change, since SQLite can't alter a column's
// collation via a plain ALTER TABLE, and gorm's own AutoMigrate doesn't revisit a column's type/collation when
// it only needs to add a missing constraint elsewhere.
//
// Uses gorm's own Migrator.AlterColumn, which rebuilds the table internally (rename/copy/drop, with foreign
// keys disabled around it) while preserving the table's existing constraint names, e.g. fk_highlights_shared_by
// - unlike a hand-rolled rebuild under a temporary table name, which bakes in new, table-specific names
// instead. AlterColumn's rebuild also drops the table's standalone indexes as a side effect (it only
// reconstructs what is declared directly on the CREATE TABLE statement), so a follow-up AutoMigrate restores
// them, including the ones enforcing email/UUID uniqueness.
//
// Skipped (with a warning) if two existing rows' emails already only differ by case, since the restored
// unique index would then reject one of them and this won't guess which one to keep.
func fixEmailCollation(db *gorm.DB, table string, value any) {
var schemaSQL string
if err := db.Raw("SELECT sql FROM sqlite_master WHERE type = 'table' AND name = ?", table).Scan(&schemaSQL).Error; err != nil {
log.Printf("warning: could not read %s schema: %v\n", table, err)
return
}
if schemaSQL == "" {
return // table doesn't exist yet; AutoMigrate already created it with today's (correct) schema.
}
if strings.Contains(schemaSQL, "`email` text collate nocase") {
return // already migrated.
}

var collisions int64
if err := db.Raw(fmt.Sprintf(`
SELECT COUNT(*) FROM (
SELECT 1 FROM %s GROUP BY LOWER(email) HAVING COUNT(*) > 1
)
`, table)).Scan(&collisions).Error; err != nil {
log.Printf("warning: could not check %s for case-duplicate emails: %v\n", table, err)
return
}
if collisions > 0 {
log.Printf("warning: %s has rows whose email only differs by case; leaving email case-sensitive for this table until resolved manually\n", table)
return
}

// Lowercase data before fixing the column's collation below: once it's "collate nocase", a plain
// "email != LOWER(email)" always evaluates false (both sides compare equal under nocase), silently
// turning this into a no-op.
if err := db.Exec(fmt.Sprintf("UPDATE %s SET email = LOWER(email) WHERE email != LOWER(email)", table)).Error; err != nil {
log.Printf("warning: could not lowercase existing emails in %s: %v\n", table, err)
return
}
if err := db.Migrator().AlterColumn(value, "Email"); err != nil {
log.Printf("warning: could not fix %s.email collation: %v\n", table, err)
return
}
if err := db.AutoMigrate(value); err != nil {
log.Printf("warning: could not restore indexes on %s after collation fix: %v\n", table, err)
}
}

// runWithoutForeignKeys disables foreign key enforcement for the duration of fc, restoring it afterwards -
// same technique gorm's own Migrator.AlterColumn uses internally (see fixEmailCollation above), needed here
// because Migrator.AutoMigrate's own constraint-adding path doesn't guard itself the same way, and a table
// rebuild it triggers can otherwise be rejected by a real foreign key reference from another table.
func runWithoutForeignKeys(db *gorm.DB, fc func() error) error {
var enabled int
db.Raw("PRAGMA foreign_keys").Scan(&enabled)
if enabled == 1 {
db.Exec("PRAGMA foreign_keys = OFF")
defer db.Exec("PRAGMA foreign_keys = ON")
}
return fc()
}

func applySQLiteIndexes(db *gorm.DB) {
indexes := []string{
`CREATE INDEX IF NOT EXISTS idx_readings_user_in_progress_partial ON readings(user_id, updated_at) WHERE completed_on IS NULL`,
Expand Down
203 changes: 203 additions & 0 deletions internal/webserver/infrastructure/sqlite_test.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,203 @@
package infrastructure_test

import (
"path/filepath"
"strings"
"testing"
"time"

"github.com/glebarez/sqlite"
"github.com/svera/coreander/v5/internal/webserver/infrastructure"
"github.com/svera/coreander/v5/internal/webserver/model"
"gorm.io/gorm"
)

// legacyUser and legacyInvitation mirror model.User/model.Invitation as they were before email columns
// were declared case-insensitive (see model.User.Email, model.Invitation.Email). Migrating a database
// through these shapes first, then through infrastructure.Connect, simulates a real database created by
// an older version of the app being opened by the current one.
type legacyUser struct {
ID uint `gorm:"primarykey"`
CreatedAt time.Time
UpdatedAt time.Time
Uuid string `gorm:"uniqueIndex; not null"`
Name string `gorm:"not null"`
Username string `gorm:"type:text collate nocase; not null; unique"`
Email string `gorm:"uniqueIndex; not null"`
SendToEmail string
Password string
Role int `gorm:"not null"`
WordsPerMinute float64
RecoveryUUID string
RecoveryValidUntil time.Time
LastRequest time.Time
ShowFileName bool `gorm:"default:false; not null"`
PrivateProfile int `gorm:"default:0; not null"`
PreferredEpubType string `gorm:"default:'epub'; not null"`
DefaultAction string `gorm:"default:'download'; not null"`
Language string
}

func (legacyUser) TableName() string { return "users" }

type legacyInvitation struct {
ID uint `gorm:"primarykey"`
Email string `gorm:"uniqueIndex; not null"`
UUID string `gorm:"uniqueIndex; not null"`
ValidUntil time.Time
}

func (legacyInvitation) TableName() string { return "invitations" }

// seedLegacySchema creates users/invitations tables shaped like they were before email columns were made
// case-insensitive, with an email stored in mixed case in each, plus a highlight and a reading that hold
// real foreign key references into users - so the users table rebuild triggered by the collation fix has
// something real to potentially break.
func seedLegacySchema(t *testing.T, path string) {
t.Helper()

db, err := gorm.Open(sqlite.Open(path), &gorm.Config{})
if err != nil {
t.Fatalf("open legacy db: %v", err)
}
sqlDB, err := db.DB()
if err != nil {
t.Fatalf("get sql.DB: %v", err)
}
defer sqlDB.Close()

if err := db.AutoMigrate(&legacyUser{}, &model.Highlight{}, &model.Reading{}, &legacyInvitation{}); err != nil {
t.Fatalf("migrate legacy schema: %v", err)
}

user := legacyUser{
Uuid: "u1",
Name: "Existing",
Username: "existing",
Email: "Mixed@Case.com",
Password: "x",
Role: model.RoleRegular,
}
if err := db.Create(&user).Error; err != nil {
t.Fatalf("seed legacy user: %v", err)
}

if err := db.Create(&model.Highlight{UserID: int(user.ID), Slug: "some-book"}).Error; err != nil {
t.Fatalf("seed highlight referencing user: %v", err)
}
if err := db.Create(&model.Reading{UserID: int(user.ID), Slug: "some-book"}).Error; err != nil {
t.Fatalf("seed reading referencing user: %v", err)
}

if err := db.Create(&legacyInvitation{
Email: "Invited@Example.COM",
UUID: "inv1",
ValidUntil: time.Now().Add(24 * time.Hour),
}).Error; err != nil {
t.Fatalf("seed legacy invitation: %v", err)
}
}

func TestConnect_NormalizesExistingEmailCase(t *testing.T) {
path := filepath.Join(t.TempDir(), "legacy.db")
seedLegacySchema(t, path)

db := infrastructure.Connect(path, 250)

var storedEmail string
if err := db.Raw("SELECT email FROM users WHERE uuid = ?", "u1").Scan(&storedEmail).Error; err != nil {
t.Fatalf("query stored email: %v", err)
}
if storedEmail != "mixed@case.com" {
t.Errorf("expected existing email to be lowercased to %q, got %q", "mixed@case.com", storedEmail)
}

var invEmail string
if err := db.Raw("SELECT email FROM invitations WHERE uuid = ?", "inv1").Scan(&invEmail).Error; err != nil {
t.Fatalf("query stored invitation email: %v", err)
}
if invEmail != "invited@example.com" {
t.Errorf("expected existing invitation email to be lowercased to %q, got %q", "invited@example.com", invEmail)
}

repo := &model.UserRepository{DB: db}
user, err := repo.FindByEmail("MIXED@CASE.COM")
if err != nil {
t.Fatalf("FindByEmail: %v", err)
}
if user == nil || user.Uuid != "u1" {
t.Errorf("expected to find existing user u1 by a differently-cased email, got %+v", user)
}

// The rebuild must preserve the table's other constraints (gorm bakes them in at CREATE TABLE time; a
// naive rebuild under a temporary table name would instead produce differently-named ones).
var usersSchema string
if err := db.Raw("SELECT sql FROM sqlite_master WHERE type = 'table' AND name = 'users'").Scan(&usersSchema).Error; err != nil {
t.Fatalf("query users schema: %v", err)
}
for _, want := range []string{"`email` text collate nocase", "CONSTRAINT `uni_users_username`"} {
if !strings.Contains(usersSchema, want) {
t.Errorf("expected users schema to contain %q, got %q", want, usersSchema)
}
}

// Standalone indexes (email/uuid uniqueness included) must survive the rebuild too.
var indexCount int64
if err := db.Raw("SELECT COUNT(*) FROM sqlite_master WHERE type = 'index' AND tbl_name = 'users' AND name IN ('idx_users_email','idx_users_uuid')").Scan(&indexCount).Error; err != nil {
t.Fatalf("query users indexes: %v", err)
}
if indexCount != 2 {
t.Errorf("expected both original indexes to be restored on users, found %d", indexCount)
}

// highlights/readings hold real foreign key references into users; the rebuild must not have broken them.
rows, err := db.Raw("PRAGMA foreign_key_check").Rows()
if err != nil {
t.Fatalf("foreign_key_check: %v", err)
}
defer rows.Close()
if rows.Next() {
t.Error("expected no foreign key violations after rebuild")
}

// Uniqueness must actually be enforced case-insensitively now, not just declared.
if err := db.Create(&legacyUser{
Uuid: "u2", Name: "Dup", Username: "dup", Email: "mixed@CASE.com", Password: "x", Role: model.RoleRegular,
}).Error; err == nil {
t.Error("expected inserting a case-variant duplicate email to be rejected")
}
}

func TestConnect_SkipsNormalizationOnExistingCaseCollision(t *testing.T) {
path := filepath.Join(t.TempDir(), "collision.db")
seedLegacySchema(t, path)

// Add a second row whose email only differs in case from the first, directly at the DB level -
// simulating data that could exist precisely because of the bug being fixed here.
db, err := gorm.Open(sqlite.Open(path), &gorm.Config{})
if err != nil {
t.Fatalf("reopen legacy db: %v", err)
}
if err := db.Create(&legacyUser{
Uuid: "u2",
Name: "Existing2",
Username: "existing2",
Email: "mixed@CASE.com",
Password: "x",
Role: model.RoleRegular,
}).Error; err != nil {
t.Fatalf("seed colliding row: %v", err)
}
sqlDB, _ := db.DB()
sqlDB.Close()

gotDB := infrastructure.Connect(path, 250)

var emails []string
if err := gotDB.Raw("SELECT email FROM users WHERE uuid IN ('u1','u2') ORDER BY uuid").Scan(&emails).Error; err != nil {
t.Fatalf("query stored emails: %v", err)
}
if len(emails) != 2 || emails[0] != "Mixed@Case.com" || emails[1] != "mixed@CASE.com" {
t.Errorf("expected colliding emails to be left untouched, got %v", emails)
}
}
2 changes: 1 addition & 1 deletion internal/webserver/model/invitation.go
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,7 @@ import "time"
// Invitation represents a user invitation
type Invitation struct {
ID uint `gorm:"primarykey"`
Email string `gorm:"uniqueIndex; not null"`
Email string `gorm:"type:text collate nocase; not null; uniqueIndex"`
UUID string `gorm:"uniqueIndex; not null"`
ValidUntil time.Time
}
2 changes: 1 addition & 1 deletion internal/webserver/model/user.go
Original file line number Diff line number Diff line change
Expand Up @@ -24,7 +24,7 @@ type User struct {
Uuid string `gorm:"uniqueIndex; not null"`
Name string `gorm:"not null"`
Username string `gorm:"type:text collate nocase; not null; unique"`
Email string `gorm:"uniqueIndex; not null"`
Email string `gorm:"type:text collate nocase; not null; uniqueIndex"`
SendToEmail string
Password string
Role int `gorm:"not null"`
Expand Down
Loading