refac: several unverified users can exist at once: an empty reset or verification code is stored as no value so it doesn't break the unique constraint, and deleting a user removes the row so the same username and email can register again
This commit is contained in:
1 parent
0b4e614ec9
commit
6e9e108033
6 files changed
+377
-16
No files matched your search
@@ -0,0 +1,70 @@
|
||||
Feature: Several unverified users can coexist, and deleting a user frees its username and email
|
||||
|
||||
As the maintainer of RenewCMS
|
||||
I want an absent password reset code or verification code stored as no value instead of an empty
|
||||
string, and a deleted user's row actually removed
|
||||
So that several unverified users can register one after another without colliding on the unique
|
||||
reset-code column, and so that deleting a user lets the same username and email register again
|
||||
|
||||
Background:
|
||||
Given internal/infrastructure/persistence/models.User stores PasswordResetCode and VerificationCode
|
||||
as nullable, unique columns, so a domain value of "" is persisted as no value (SQL NULL), and SQL
|
||||
NULL is exempt from the unique constraint
|
||||
And internal/infrastructure/persistence/mappers.UserToModel maps a domain "" for PasswordResetCode or
|
||||
VerificationCode to no value, and UserToDomain maps no value back to ""
|
||||
And internal/infrastructure/persistence/models.User no longer embeds gorm.Model, so
|
||||
UserRepository.Delete removes the row instead of leaving a soft-deleted row that still occupies the
|
||||
unique username and email
|
||||
And POST /v1/auth/register is public, and CreateUser stores a new user with is_verified false and
|
||||
PasswordResetCode "" as soon as the request is valid
|
||||
And Register only answers 403 once at least one existing user is verified (SomeUsersVerified), never
|
||||
because the user table already has rows
|
||||
And DeleteUser has no handler mounted in api/router.go today, so deletion in these scenarios is
|
||||
exercised through UserRepository.Delete / DeleteUserUseCase.DeleteUser directly, not through an
|
||||
HTTP route
|
||||
|
||||
Scenario: Two unverified users can register one after another without a reset-code collision
|
||||
Given the user repository is empty
|
||||
When the client sends POST /v1/auth/register with username "alice", password "hunter2example12",
|
||||
email "alice@example.com"
|
||||
Then the response status is 200
|
||||
And a jwt cookie is set
|
||||
When the client sends POST /v1/auth/register with username "bob", password "hunter2example12",
|
||||
email "bob@example.com"
|
||||
Then the response status is 200
|
||||
And a jwt cookie is set
|
||||
And the user repository contains both "alice" and "bob", each with is_verified false and an empty
|
||||
password reset code
|
||||
|
||||
Scenario: Creating two users with no active reset code does not violate the unique constraint
|
||||
Given a repository backed by an in-memory SQLite database with models.User migrated
|
||||
When Create is called with username "alice", email "alice@example.com", password_reset_code "" and
|
||||
verification_code "code-alice"
|
||||
And Create is called with username "bob", email "bob@example.com", password_reset_code "" and
|
||||
verification_code "code-bob"
|
||||
Then both calls succeed
|
||||
And GetByUsername "alice" and GetByUsername "bob" each return password_reset_code ""
|
||||
|
||||
Scenario: The unique constraint still rejects two users sharing the same active reset code
|
||||
Given a repository backed by an in-memory SQLite database containing a user "alice" with
|
||||
password_reset_code "shared-code"
|
||||
When Create is called with username "bob", email "bob@example.com" and password_reset_code
|
||||
"shared-code"
|
||||
Then the Create call returns an error
|
||||
And GetByUsername "bob" still returns user.ErrUserNotFound
|
||||
|
||||
Scenario: Deleting a user removes the row so the same username and email can register again
|
||||
Given a repository backed by an in-memory SQLite database containing a user with id 1, username
|
||||
"alice" and email "alice@example.com"
|
||||
When Delete is called with id 1
|
||||
And Create is called with username "alice", email "alice@example.com" and a new password
|
||||
Then the Create call succeeds
|
||||
And the newly created user's id is different from 1
|
||||
And Get with id 1 returns user.ErrUserNotFound
|
||||
|
||||
Scenario: Registering the same username while the first account still exists is still rejected
|
||||
Given a repository backed by an in-memory SQLite database containing a user with username "alice"
|
||||
and email "alice@example.com"
|
||||
When Create is called with username "alice", email "someone-else@example.com" and a new password
|
||||
Then the Create call returns an error
|
||||
And the user repository still contains exactly one user with username "alice"
|
||||
@@ -0,0 +1,50 @@
|
||||
# QA procedure: unverified_users_and_reregistration
|
||||
|
||||
Package: `test` (black-box), extending `test/user_test.go` (mockUserRepo) and reusing the
|
||||
`openInMemoryDb(t)` helper from `test/not_found_errors_test.go`.
|
||||
|
||||
## Setup shared by every case
|
||||
1. Call `validator.InitValidator()` once.
|
||||
2. Repository-level checks use `openInMemoryDb(t)`, `db.AutoMigrate(&models.User{})`, and a real
|
||||
`persistence.NewUserRepository(db)`.
|
||||
3. HTTP-level checks use `mockUserRepo` (from `test/user_test.go`) wired into
|
||||
`useCases.NewCreateUserUseCase(repo, &hashing.BcryptHasher{})`,
|
||||
`useCases.NewListUsersUseCase(repo)`, `useCases.NewGetUserUseCase(repo)`,
|
||||
`useCases.NewAuthenticateUserUseCase(repo, ...)`, and `auth.NewHandler(...)` with a test
|
||||
`jwtauth.New("HS256", []byte("test-secret"), nil)`. Mount `POST /v1/auth/register` and
|
||||
`POST /v1/auth/login` unprotected on a fresh `chi.NewRouter()`, mirroring `api/router.go`. No live
|
||||
server.
|
||||
|
||||
## Repository-level checks (real persistence repository, in-memory SQLite, no HTTP)
|
||||
4. On an empty repository, `Create` a user "alice"/"alice@example.com" with `PasswordResetCode: ""` and
|
||||
`VerificationCode: "code-alice"` → assert `err == nil`. `Create` a second user "bob"/"bob@example.com"
|
||||
with `PasswordResetCode: ""` and `VerificationCode: "code-bob"` → assert `err == nil` (regression: before
|
||||
the fix this fails with a SQLite `UNIQUE constraint failed: users.password_reset_code` error). Then
|
||||
`GetByUsername("alice")` and `GetByUsername("bob")` → assert `PasswordResetCode == ""` for both.
|
||||
5. `Create` a user "alice" with `PasswordResetCode: "shared-code"`. `Create` a user "bob" with the same
|
||||
`PasswordResetCode: "shared-code"` → assert `err != nil` (the unique constraint still applies to an
|
||||
actual shared value). Assert `GetByUsername("bob")` returns `user.ErrUserNotFound` (the row was never
|
||||
created).
|
||||
6. `Create` a user "alice"/"alice@example.com", capture its id. `Delete(id)` → assert `err == nil`.
|
||||
`Create` a new user with the same username "alice" and email "alice@example.com" (a different
|
||||
password and verification code) → assert `err == nil` (regression: before the fix this fails with a
|
||||
SQLite `UNIQUE constraint failed: users.username` error because the row was only soft-deleted). Assert
|
||||
the new user's id differs from the deleted one. Assert `Get(deletedId)` returns `user.ErrUserNotFound`.
|
||||
7. `Create` a user "alice"/"alice@example.com" and do **not** delete it. `Create` a second user with
|
||||
username "alice" and a different email → assert `err != nil` (unchanged: still rejected while the row
|
||||
is live).
|
||||
|
||||
## HTTP checks (fakes, per the shared setup above)
|
||||
8. `POST /v1/auth/register` with `{"username":"alice","password":"hunter2example12","email":"alice@example.com"}`
|
||||
against an empty `mockUserRepo` → assert status `200`, a `Set-Cookie: jwt=...` header present, body
|
||||
decodes to `{"message":"User registered! HTTPonly jwt cookie created"}`.
|
||||
9. `POST /v1/auth/register` again with `{"username":"bob","password":"hunter2example12","email":"bob@example.com"}`
|
||||
(repo now holds unverified "alice") → assert status `200` and a `Set-Cookie: jwt=...` header present
|
||||
(register stays allowed because no user is verified yet).
|
||||
10. Assert `mockUserRepo.users` holds two entries, both `IsVerified == false` and both
|
||||
`PasswordResetCode == ""`.
|
||||
|
||||
## Manual verification (not exercised by the Go test)
|
||||
- None. `DeleteUser` has no HTTP route mounted today, so there is no SPA-visible delete-and-re-register
|
||||
flow to check by hand, and `CreateUser` does not call the mailer, so there is no SMTP involvement in
|
||||
this slice.
|
||||
@@ -5,15 +5,29 @@ import (
|
||||
"RenewCMS/internal/infrastructure/persistence/models"
|
||||
)
|
||||
|
||||
func nullableStringToDomain(value *string) string {
|
||||
if value == nil {
|
||||
return ""
|
||||
}
|
||||
return *value
|
||||
}
|
||||
|
||||
func DomainToNullableString(value string) *string {
|
||||
if value == "" {
|
||||
return nil
|
||||
}
|
||||
return &value
|
||||
}
|
||||
|
||||
func UserToDomain(m models.User) user.User {
|
||||
return user.User{
|
||||
ID: m.ID,
|
||||
Username: m.Username,
|
||||
Password: m.Password,
|
||||
PasswordResetCode: m.PasswordResetCode,
|
||||
PasswordResetCode: nullableStringToDomain(m.PasswordResetCode),
|
||||
Email: m.Email,
|
||||
IsVerified: m.IsVerified,
|
||||
VerificationCode: m.VerificationCode,
|
||||
VerificationCode: nullableStringToDomain(m.VerificationCode),
|
||||
VerificationExpiration: m.VerificationExpiration,
|
||||
CreatedAt: m.CreatedAt,
|
||||
UpdatedAt: m.UpdatedAt,
|
||||
@@ -24,10 +38,10 @@ func UserToModel(a user.User) models.User {
|
||||
return models.User{
|
||||
Username: a.Username,
|
||||
Password: a.Password,
|
||||
PasswordResetCode: a.PasswordResetCode,
|
||||
PasswordResetCode: DomainToNullableString(a.PasswordResetCode),
|
||||
Email: a.Email,
|
||||
IsVerified: a.IsVerified,
|
||||
VerificationCode: a.VerificationCode,
|
||||
VerificationCode: DomainToNullableString(a.VerificationCode),
|
||||
VerificationExpiration: a.VerificationExpiration,
|
||||
CreatedAt: a.CreatedAt,
|
||||
UpdatedAt: a.UpdatedAt,
|
||||
|
||||
@@ -2,19 +2,16 @@ package models
|
||||
|
||||
import (
|
||||
"time"
|
||||
|
||||
"gorm.io/gorm"
|
||||
)
|
||||
|
||||
type User struct {
|
||||
gorm.Model
|
||||
ID uint32 `gorm:"primaryKey;autoIncrement"`
|
||||
Username string `gorm:"unique;not null"`
|
||||
Password string `gorm:"not null"`
|
||||
PasswordResetCode string `gorm:"unique"`
|
||||
Email string `gorm:"unique;not null"`
|
||||
IsVerified bool `gorm:"default=false;not null"`
|
||||
VerificationCode string `gorm:"unique"`
|
||||
ID uint32 `gorm:"primaryKey;autoIncrement"`
|
||||
Username string `gorm:"unique;not null"`
|
||||
Password string `gorm:"not null"`
|
||||
PasswordResetCode *string `gorm:"unique"`
|
||||
Email string `gorm:"unique;not null"`
|
||||
IsVerified bool `gorm:"default=false;not null"`
|
||||
VerificationCode *string `gorm:"unique"`
|
||||
VerificationExpiration time.Time
|
||||
CreatedAt time.Time `gorm:"autoCreateTime"`
|
||||
UpdatedAt time.Time `gorm:"autoUpdateTime"`
|
||||
|
||||
@@ -30,8 +30,9 @@ func (u *UserRepository) Create(user domainUser.User) (domainUser.User, error) {
|
||||
creationResult := u.db.Create(&entity.User{
|
||||
Username: user.Username,
|
||||
Password: user.Password,
|
||||
PasswordResetCode: mappers.DomainToNullableString(user.PasswordResetCode),
|
||||
Email: user.Email,
|
||||
VerificationCode: user.VerificationCode,
|
||||
VerificationCode: mappers.DomainToNullableString(user.VerificationCode),
|
||||
VerificationExpiration: user.VerificationExpiration,
|
||||
})
|
||||
if creationResult.Error != nil {
|
||||
@@ -121,7 +122,7 @@ func (u *UserRepository) UpdatePasswordResetCode(userId uint32, code string) (do
|
||||
return domainUser.User{}, mapNotFoundErr(err, domainUser.ErrUserNotFound)
|
||||
}
|
||||
|
||||
localUser.PasswordResetCode = code
|
||||
localUser.PasswordResetCode = mappers.DomainToNullableString(code)
|
||||
err = u.db.Save(&localUser).Error
|
||||
if err != nil {
|
||||
return domainUser.User{}, err
|
||||
|
||||
@@ -0,0 +1,229 @@
|
||||
package test
|
||||
|
||||
import (
|
||||
"RenewCMS/api/handlers/auth"
|
||||
validator "RenewCMS/api/pkg/validator"
|
||||
domainUser "RenewCMS/internal/domain/user"
|
||||
"RenewCMS/internal/infrastructure/hashing"
|
||||
"RenewCMS/internal/infrastructure/persistence/models"
|
||||
persistence "RenewCMS/internal/infrastructure/persistence/repositories"
|
||||
"RenewCMS/internal/infrastructure/useCases"
|
||||
"bytes"
|
||||
"encoding/json"
|
||||
"errors"
|
||||
"net/http"
|
||||
"net/http/httptest"
|
||||
"testing"
|
||||
|
||||
"github.com/go-chi/chi/v5"
|
||||
"github.com/go-chi/jwtauth/v5"
|
||||
)
|
||||
|
||||
func newMigratedUserRepository(t *testing.T) *persistence.UserRepository {
|
||||
t.Helper()
|
||||
db := openInMemoryDb(t)
|
||||
if err := db.AutoMigrate(&models.User{}); err != nil {
|
||||
t.Fatalf("failed to migrate: %v", err)
|
||||
}
|
||||
return persistence.NewUserRepository(db)
|
||||
}
|
||||
|
||||
func TestUnverifiedUsersAndReregistrationRepository(t *testing.T) {
|
||||
t.Run("Creating two users with no active reset code does not violate the unique constraint", func(t *testing.T) {
|
||||
repo := newMigratedUserRepository(t)
|
||||
|
||||
if _, err := repo.Create(domainUser.User{Username: "alice", Email: "alice@example.com", Password: "hunter2example12", PasswordResetCode: "", VerificationCode: "code-alice"}); err != nil {
|
||||
t.Fatalf("expected alice creation to succeed, got %v", err)
|
||||
}
|
||||
if _, err := repo.Create(domainUser.User{Username: "bob", Email: "bob@example.com", Password: "hunter2example12", PasswordResetCode: "", VerificationCode: "code-bob"}); err != nil {
|
||||
t.Fatalf("expected bob creation to succeed, got %v", err)
|
||||
}
|
||||
|
||||
alice, err := repo.GetByUsername("alice")
|
||||
if err != nil {
|
||||
t.Fatalf("failed to get alice: %v", err)
|
||||
}
|
||||
if alice.PasswordResetCode != "" {
|
||||
t.Errorf("expected empty password reset code for alice, got %q", alice.PasswordResetCode)
|
||||
}
|
||||
|
||||
bob, err := repo.GetByUsername("bob")
|
||||
if err != nil {
|
||||
t.Fatalf("failed to get bob: %v", err)
|
||||
}
|
||||
if bob.PasswordResetCode != "" {
|
||||
t.Errorf("expected empty password reset code for bob, got %q", bob.PasswordResetCode)
|
||||
}
|
||||
})
|
||||
|
||||
t.Run("The unique constraint still rejects two users sharing the same active reset code", func(t *testing.T) {
|
||||
repo := newMigratedUserRepository(t)
|
||||
|
||||
if _, err := repo.Create(domainUser.User{Username: "alice", Email: "alice@example.com", Password: "hunter2example12", PasswordResetCode: "shared-code", VerificationCode: "code-alice"}); err != nil {
|
||||
t.Fatalf("expected alice creation to succeed, got %v", err)
|
||||
}
|
||||
|
||||
_, err := repo.Create(domainUser.User{Username: "bob", Email: "bob@example.com", Password: "hunter2example12", PasswordResetCode: "shared-code", VerificationCode: "code-bob"})
|
||||
if err == nil {
|
||||
t.Fatal("expected Create for bob to fail because of the shared reset code")
|
||||
}
|
||||
|
||||
_, err = repo.GetByUsername("bob")
|
||||
if !errors.Is(err, domainUser.ErrUserNotFound) {
|
||||
t.Errorf("expected user.ErrUserNotFound, got %v", err)
|
||||
}
|
||||
})
|
||||
|
||||
t.Run("Deleting a user removes the row so the same username and email can register again", func(t *testing.T) {
|
||||
repo := newMigratedUserRepository(t)
|
||||
|
||||
created, err := repo.Create(domainUser.User{Username: "alice", Email: "alice@example.com", Password: "hunter2example12", VerificationCode: "code-alice"})
|
||||
if err != nil {
|
||||
t.Fatalf("failed to seed alice: %v", err)
|
||||
}
|
||||
|
||||
if err := repo.Delete(created.ID); err != nil {
|
||||
t.Fatalf("expected delete to succeed, got %v", err)
|
||||
}
|
||||
|
||||
recreated, err := repo.Create(domainUser.User{Username: "alice", Email: "alice@example.com", Password: "differentpassword12", VerificationCode: "code-alice-2"})
|
||||
if err != nil {
|
||||
t.Fatalf("expected alice to be re-registrable after deletion, got %v", err)
|
||||
}
|
||||
if recreated.ID == created.ID {
|
||||
t.Errorf("expected the recreated user to have a different id than %d", created.ID)
|
||||
}
|
||||
|
||||
_, err = repo.Get(created.ID)
|
||||
if !errors.Is(err, domainUser.ErrUserNotFound) {
|
||||
t.Errorf("expected user.ErrUserNotFound for the deleted id, got %v", err)
|
||||
}
|
||||
})
|
||||
|
||||
t.Run("Registering the same username while the first account still exists is still rejected", func(t *testing.T) {
|
||||
repo := newMigratedUserRepository(t)
|
||||
|
||||
if _, err := repo.Create(domainUser.User{Username: "alice", Email: "alice@example.com", Password: "hunter2example12", VerificationCode: "code-alice"}); err != nil {
|
||||
t.Fatalf("failed to seed alice: %v", err)
|
||||
}
|
||||
|
||||
_, err := repo.Create(domainUser.User{Username: "alice", Email: "someone-else@example.com", Password: "anotherpassword12", VerificationCode: "code-alice-2"})
|
||||
if err == nil {
|
||||
t.Fatal("expected Create to fail because the username is still in use")
|
||||
}
|
||||
|
||||
users := repo.GetAll()
|
||||
count := 0
|
||||
for _, u := range users {
|
||||
if u.Username == "alice" {
|
||||
count++
|
||||
}
|
||||
}
|
||||
if count != 1 {
|
||||
t.Errorf("expected exactly one user with username alice, got %d", count)
|
||||
}
|
||||
})
|
||||
|
||||
t.Run("Updating a user's reset code to one already active for another user is rejected", func(t *testing.T) {
|
||||
repo := newMigratedUserRepository(t)
|
||||
|
||||
if _, err := repo.Create(domainUser.User{Username: "alice", Email: "alice@example.com", Password: "hunter2example12", PasswordResetCode: "alice-code", VerificationCode: "code-alice"}); err != nil {
|
||||
t.Fatalf("expected alice creation to succeed, got %v", err)
|
||||
}
|
||||
bob, err := repo.Create(domainUser.User{Username: "bob", Email: "bob@example.com", Password: "hunter2example12", VerificationCode: "code-bob"})
|
||||
if err != nil {
|
||||
t.Fatalf("expected bob creation to succeed, got %v", err)
|
||||
}
|
||||
|
||||
_, err = repo.UpdatePasswordResetCode(bob.ID, "alice-code")
|
||||
if err == nil {
|
||||
t.Fatal("expected UpdatePasswordResetCode for bob to fail because the code is already active for alice")
|
||||
}
|
||||
|
||||
reloadedBob, err := repo.Get(bob.ID)
|
||||
if err != nil {
|
||||
t.Fatalf("failed to reload bob: %v", err)
|
||||
}
|
||||
if reloadedBob.PasswordResetCode != "" {
|
||||
t.Errorf("expected bob's reset code to remain unset, got %q", reloadedBob.PasswordResetCode)
|
||||
}
|
||||
})
|
||||
}
|
||||
|
||||
func newUnverifiedRegistrationRouter(userRepo *mockUserRepo, token *jwtauth.JWTAuth) *chi.Mux {
|
||||
hasher := hashing.NewBcryptHasher()
|
||||
authHandler := auth.NewHandler(
|
||||
token,
|
||||
useCases.NewGetUserUseCase(userRepo),
|
||||
useCases.NewListUsersUseCase(userRepo),
|
||||
useCases.NewCreateUserUseCase(userRepo, hasher),
|
||||
useCases.NewAuthenticateUserUseCase(userRepo, hasher),
|
||||
true,
|
||||
)
|
||||
|
||||
r := chi.NewRouter()
|
||||
r.Route("/v1", func(r chi.Router) {
|
||||
r.Route("/auth", func(r chi.Router) {
|
||||
r.Post("/register", authHandler.Register)
|
||||
})
|
||||
})
|
||||
return r
|
||||
}
|
||||
|
||||
func registerViaHttp(t *testing.T, router *chi.Mux, username, password, email string) *httptest.ResponseRecorder {
|
||||
t.Helper()
|
||||
body, err := json.Marshal(map[string]string{"username": username, "password": password, "email": email})
|
||||
if err != nil {
|
||||
t.Fatalf("failed to marshal body: %v", err)
|
||||
}
|
||||
req := httptest.NewRequest(http.MethodPost, "/v1/auth/register", bytes.NewReader(body))
|
||||
req.Header.Set("Content-Type", "application/json")
|
||||
rec := httptest.NewRecorder()
|
||||
router.ServeHTTP(rec, req)
|
||||
return rec
|
||||
}
|
||||
|
||||
func TestUnverifiedUsersAndReregistrationHttp(t *testing.T) {
|
||||
validator.InitValidator()
|
||||
token := jwtauth.New("HS256", []byte("test-secret"), nil)
|
||||
|
||||
t.Run("Two unverified users can register one after another without a reset-code collision", func(t *testing.T) {
|
||||
repo := newMockUserRepo()
|
||||
router := newUnverifiedRegistrationRouter(repo, token)
|
||||
|
||||
aliceRec := registerViaHttp(t, router, "alice", "hunter2example12", "alice@example.com")
|
||||
if aliceRec.Code != http.StatusOK {
|
||||
t.Fatalf("expected status 200 for alice, got %d: %s", aliceRec.Code, aliceRec.Body.String())
|
||||
}
|
||||
if aliceRec.Header().Get("Set-Cookie") == "" {
|
||||
t.Errorf("expected a jwt cookie to be set for alice")
|
||||
}
|
||||
var aliceBody map[string]string
|
||||
if err := json.Unmarshal(aliceRec.Body.Bytes(), &aliceBody); err != nil {
|
||||
t.Fatalf("failed to decode alice response body: %v", err)
|
||||
}
|
||||
if aliceBody["message"] != "User registered! HTTPonly jwt cookie created" {
|
||||
t.Errorf("unexpected response body for alice: %v", aliceBody)
|
||||
}
|
||||
|
||||
bobRec := registerViaHttp(t, router, "bob", "hunter2example12", "bob@example.com")
|
||||
if bobRec.Code != http.StatusOK {
|
||||
t.Fatalf("expected status 200 for bob, got %d: %s", bobRec.Code, bobRec.Body.String())
|
||||
}
|
||||
if bobRec.Header().Get("Set-Cookie") == "" {
|
||||
t.Errorf("expected a jwt cookie to be set for bob")
|
||||
}
|
||||
|
||||
if len(repo.users) != 2 {
|
||||
t.Fatalf("expected 2 users in the repository, got %d", len(repo.users))
|
||||
}
|
||||
for _, u := range repo.users {
|
||||
if u.IsVerified {
|
||||
t.Errorf("expected user %q to be unverified", u.Username)
|
||||
}
|
||||
if u.PasswordResetCode != "" {
|
||||
t.Errorf("expected user %q to have an empty password reset code, got %q", u.Username, u.PasswordResetCode)
|
||||
}
|
||||
}
|
||||
})
|
||||
}
|
||||
Reference in new issue
Block a user