fix(09): WR-11 reject ambiguous admin logins and cross-field login or email collisions

This commit is contained in:
Jakub Zych
2026-10-01 21:17:50 +02:00
parent eb8c727790
commit 331351a73c
5 changed files with 70 additions and 8 deletions

View File

@@ -77,7 +77,7 @@ The application binary has two commands for operators:
./bin/acme admin:reset-password admin@example.com --password '<secret>'
```
`admin:create` creates an activated administrator; `--login` defaults to the lower-cased email and `--role <code>` 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 <code>` 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.

View File

@@ -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) {

View File

@@ -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())
}
}

View File

@@ -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 {

View File

@@ -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)
}
}