From 331351a73cd00c2243353f60c240c6c2b57e1e59 Mon Sep 17 00:00:00 2001 From: Jakub Zych Date: Thu, 1 Oct 2026 21:17:50 +0200 Subject: [PATCH] fix(09): WR-11 reject ambiguous admin logins and cross-field login or email collisions --- docs/backend/users-and-permissions.md | 2 +- modules/cabana/auth.go | 17 +++++++++------ modules/cabana/auth_test.go | 23 ++++++++++++++++++++ modules/cabana/commands.go | 5 ++++- modules/cabana/commands_test.go | 31 +++++++++++++++++++++++++++ 5 files changed, 70 insertions(+), 8 deletions(-) diff --git a/docs/backend/users-and-permissions.md b/docs/backend/users-and-permissions.md index e653a7f..357aaca 100644 --- a/docs/backend/users-and-permissions.md +++ b/docs/backend/users-and-permissions.md @@ -77,7 +77,7 @@ The application binary has two commands for operators: ./bin/acme admin:reset-password admin@example.com --password '' ``` -`admin:create` creates an activated administrator; `--login` defaults to the lower-cased email and `--role ` assigns a role. `admin:reset-password` takes a login or an email, sets the password and revokes every token issued before the reset. Passwords are hashed with bcrypt at `admin.password.bcrypt_cost`, so hashes copied from a WinterCMS database keep working. +`admin:create` creates an activated administrator; `--login` defaults to the lower-cased email and `--role ` assigns a role. It refuses a login or email that matches another administrator's login or email in either field, because a sign-in identifier that matches two administrators is answered like a wrong password. `admin:reset-password` takes a login or an email, sets the password and revokes every token issued before the reset. Passwords are hashed with bcrypt at `admin.password.bcrypt_cost`, so hashes copied from a WinterCMS database keep working. > [!TIP] > Pass the password through an environment variable or a prompt of your shell rather than typing it on the command line, where it stays in the shell history. diff --git a/modules/cabana/auth.go b/modules/cabana/auth.go index a09bd4d..c7cef63 100644 --- a/modules/cabana/auth.go +++ b/modules/cabana/auth.go @@ -381,17 +381,22 @@ func adminBlacklist(app *backpack.App) bouncer.BlacklistStore { return bouncer.NewPostgresBlacklist(sqlDB, backendJWTBlacklistTable) } +// findBackendLogin resolves a login identifier (a login or an email) to one +// administrator. An identifier that matches two rows, such as one admin's login +// equal to another's email, resolves to nobody: it is reported as not found, so +// the caller answers it like any wrong credential instead of letting the lowest +// id win and lock the other admin out. func findBackendLogin(db *gorm.DB, identifier string) (BackendUser, bool, error) { email := strings.ToLower(identifier) - var user BackendUser - err := db.Preload("Role").Where("login = ? OR lower(email) = ?", identifier, email).First(&user).Error - if errors.Is(err, gorm.ErrRecordNotFound) { - return BackendUser{}, false, nil - } + var users []BackendUser + err := db.Preload("Role").Where("login = ? OR lower(email) = ?", identifier, email).Order("id").Limit(2).Find(&users).Error if err != nil { return BackendUser{}, false, err } - return user, true, nil + if len(users) != 1 { + return BackendUser{}, false, nil + } + return users[0], true, nil } func (s *service) db() (*gorm.DB, error) { diff --git a/modules/cabana/auth_test.go b/modules/cabana/auth_test.go index 9203a24..725645f 100644 --- a/modules/cabana/auth_test.go +++ b/modules/cabana/auth_test.go @@ -553,3 +553,26 @@ func itoa(id uint) string { func adminAPI(rel string) string { return cabana.DefaultAdminPrefix + "/api/v1" + rel } + +// TestLoginAmbiguousIdentifier pins WR-11: an identifier that is one admin's +// login and another's email resolves to nobody (a plain 401), so neither admin +// is silently locked out by the other; each can still sign in by the +// unambiguous field. +func TestLoginAmbiguousIdentifier(t *testing.T) { + gdb := adminGorm(t) + h := adminHandler(t, gdb, nil) + insertAdmin(t, gdb, "ambig@amb.test", "owner-a@amb.test", "password-of-a", true, false) + insertAdmin(t, gdb, "owner-b", "ambig@amb.test", "password-of-b", true, false) + for _, password := range []string{"password-of-a", "password-of-b"} { + rec := postJSON(t, h, adminAPI("/auth/login"), map[string]string{"login": "ambig@amb.test", "password": password}) + if rec.Code != http.StatusUnauthorized { + t.Fatalf("ambiguous identifier with %q = %d %s, want 401", password, rec.Code, rec.Body.String()) + } + } + if rec := postJSON(t, h, adminAPI("/auth/login"), map[string]string{"login": "owner-a@amb.test", "password": "password-of-a"}); rec.Code != http.StatusOK { + t.Fatalf("admin A by email = %d %s", rec.Code, rec.Body.String()) + } + if rec := postJSON(t, h, adminAPI("/auth/login"), map[string]string{"login": "owner-b", "password": "password-of-b"}); rec.Code != http.StatusOK { + t.Fatalf("admin B by login = %d %s", rec.Code, rec.Body.String()) + } +} diff --git a/modules/cabana/commands.go b/modules/cabana/commands.go index 07a2dc2..c99481f 100644 --- a/modules/cabana/commands.go +++ b/modules/cabana/commands.go @@ -69,7 +69,10 @@ func adminCreate(ctx context.Context, app *backpack.App, in bonfire.Input, out b return err } var existing int64 - if err := tx.Model(&BackendUser{}).Where("login = ? OR lower(email) = ?", login, email).Count(&existing).Error; err != nil { + // Check both fields against both values: a new login equal to an + // existing email (or the reverse) would make the login identifier + // ambiguous, so neither admin could rely on it. + if err := tx.Model(&BackendUser{}).Where("login IN (?, ?) OR lower(email) IN (?, ?)", login, email, strings.ToLower(login), email).Count(&existing).Error; err != nil { return err } if existing > 0 { diff --git a/modules/cabana/commands_test.go b/modules/cabana/commands_test.go index 3b01178..ace11a7 100644 --- a/modules/cabana/commands_test.go +++ b/modules/cabana/commands_test.go @@ -195,3 +195,34 @@ func (f flagInput) Flag(name string) (string, bool) { } func (f flagInput) Flags(string) []string { return nil } + +// TestAdminCreateRejectsCrossFieldCollision pins WR-11: admin:create refuses a +// login that equals another admin's email and an email that equals another +// admin's login, because either makes the login identifier ambiguous. +func TestAdminCreateRejectsCrossFieldCollision(t *testing.T) { + gdb := adminGorm(t) + app := commandApp(t, gdb) + create := commandByName(t, cabana.RuntimeCommands(app), "admin:create") + out := bonfire.NewOutput(nil, &bytes.Buffer{}, &bytes.Buffer{}) + const password = "correct-horse-battery" + if err := create.Run(context.Background(), flagInput{flags: map[string]string{ + "email": "a-xf@example.test", "login": "login-xf@example.test", "password": password, + }}, out); err != nil { + t.Fatal(err) + } + for name, flags := range map[string]map[string]string{ + "email equals an existing login": {"email": "login-xf@example.test", "login": "b-xf"}, + "login equals an existing email": {"email": "c-xf@example.test", "login": "a-xf@example.test"}, + "email differs only by case": {"email": "A-XF@example.test", "login": "d-xf"}, + } { + flags["password"] = password + err := create.Run(context.Background(), flagInput{flags: flags}, out) + if err == nil || !strings.Contains(err.Error(), "already exists") { + t.Fatalf("%s: err = %v, want an already-exists refusal", name, err) + } + } + var n int64 + if err := gdb.Model(&cabana.BackendUser{}).Where("login LIKE ?", "%-xf%").Count(&n).Error; err != nil || n != 1 { + t.Fatalf("admins after refused creates = %d, %v; want 1", n, err) + } +}