From 6e9e10803355fbd1f8be00d660cbd578359ecc85 Mon Sep 17 00:00:00 2001 From: Florian Sylvain Date: Thu, 24 Sep 2026 01:59:24 +0200 Subject: [PATCH] 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 --- ...nverified_users_and_reregistration.feature | 70 ++++++ .../unverified_users_and_reregistration.qa.md | 50 ++++ .../persistence/mappers/userMapper.go | 22 +- .../persistence/models/userModel.go | 17 +- .../repositories/userRepository.go | 5 +- ...nverified_users_and_reregistration_test.go | 229 ++++++++++++++++++ 6 files changed, 377 insertions(+), 16 deletions(-) create mode 100644 features/unverified_users_and_reregistration.feature create mode 100644 features/unverified_users_and_reregistration.qa.md create mode 100644 test/unverified_users_and_reregistration_test.go diff --git a/features/unverified_users_and_reregistration.feature b/features/unverified_users_and_reregistration.feature new file mode 100644 index 0000000..6c8878d --- /dev/null +++ b/features/unverified_users_and_reregistration.feature @@ -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" diff --git a/features/unverified_users_and_reregistration.qa.md b/features/unverified_users_and_reregistration.qa.md new file mode 100644 index 0000000..883815f --- /dev/null +++ b/features/unverified_users_and_reregistration.qa.md @@ -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. diff --git a/internal/infrastructure/persistence/mappers/userMapper.go b/internal/infrastructure/persistence/mappers/userMapper.go index d08b675..66c7564 100644 --- a/internal/infrastructure/persistence/mappers/userMapper.go +++ b/internal/infrastructure/persistence/mappers/userMapper.go @@ -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, diff --git a/internal/infrastructure/persistence/models/userModel.go b/internal/infrastructure/persistence/models/userModel.go index e58d852..01674aa 100644 --- a/internal/infrastructure/persistence/models/userModel.go +++ b/internal/infrastructure/persistence/models/userModel.go @@ -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"` diff --git a/internal/infrastructure/persistence/repositories/userRepository.go b/internal/infrastructure/persistence/repositories/userRepository.go index 9009760..5a192e2 100644 --- a/internal/infrastructure/persistence/repositories/userRepository.go +++ b/internal/infrastructure/persistence/repositories/userRepository.go @@ -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 diff --git a/test/unverified_users_and_reregistration_test.go b/test/unverified_users_and_reregistration_test.go new file mode 100644 index 0000000..bc9b731 --- /dev/null +++ b/test/unverified_users_and_reregistration_test.go @@ -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) + } + } + }) +}