rosa/vernier

Fork 0

domain: in-memory mock repo diverges from Postgres (reset-token action_type; missing EmailTaken) #63

New issue

Closed

opened 2026-07-03 01:42:23 +00:00 by rosa · 1 comment

rosa commented

2026-07-03 01:42:23 +00:00

Owner

Found during a code review of the domain crate.

Location: crates/domain/src/mocks.rs:250 and crates/domain/src/mocks.rs:167/208
Severity: Correctness — test/prod divergence (mock is feature-gated)

Problem

Two divergences let tests pass on behavior the real Postgres repo rejects:

delete_password_reset_tokens_for_user retains on user_id only, with no action_type filter, while Postgres filters action_type = PasswordReset. On the mock, a successful login also deletes a pending email-confirmation token, orphaning email confirmation.
create_user checks only username uniqueness (duplicate email silently overwrites email_index, never returns RegistrationError::EmailTaken), and update_user never returns EmailTaken, while Postgres enforces the unique email constraint.

Failure scenario

Dev/test flows against the mock confirm behavior (log in before confirming email; register two accounts with the same email) that would error against Postgres, hiding real bugs.

Suggested fix

Bring the mock in line with the Postgres WHERE clauses/constraints: filter by action_type, and enforce email uniqueness in create_user/update_user.

Found during a code review of the `domain` crate. **Location:** `crates/domain/src/mocks.rs:250` and `crates/domain/src/mocks.rs:167`/`208` **Severity:** Correctness — test/prod divergence (mock is feature-gated) ## Problem Two divergences let tests pass on behavior the real Postgres repo rejects: 1. `delete_password_reset_tokens_for_user` retains on `user_id` only, with no `action_type` filter, while Postgres filters `action_type = PasswordReset`. On the mock, a successful login also deletes a pending email-confirmation token, orphaning email confirmation. 2. `create_user` checks only username uniqueness (duplicate email silently overwrites `email_index`, never returns `RegistrationError::EmailTaken`), and `update_user` never returns `EmailTaken`, while Postgres enforces the unique email constraint. ## Failure scenario Dev/test flows against the mock confirm behavior (log in before confirming email; register two accounts with the same email) that would error against Postgres, hiding real bugs. ## Suggested fix Bring the mock in line with the Postgres WHERE clauses/constraints: filter by `action_type`, and enforce email uniqueness in `create_user`/`update_user`.

rosa added the

needs-triage

label

2026-07-03 01:43:57 +00:00

rosa referenced this issue

2026-07-03 20:55:18 +00:00

domain: authorization code redemption is non-atomic, concurrent redeems bypass single-use #49

rosa referenced this issue

2026-07-03 20:55:18 +00:00

domain: password-reset job retries mint additional valid reset tokens #59

rosa commented

2026-07-04 18:25:32 +00:00

Author

Owner

This was generated by AI during triage.

Agent Brief

Category: bug
Summary: Bring the in-memory MemoryAppRepo mock in line with the Postgres adapter on confirmation-deletion scope and email-uniqueness enforcement, so tests can't pass on behavior the real repo rejects.

Current behavior:
The MemoryAppRepo mock (feature-gated, used by tests) diverges from the Postgres AppRepository implementation in four places, letting tests confirm behavior Postgres would reject:

delete_password_reset_tokens_for_user deletes all confirmations for the user, ignoring action_type. Postgres deletes only PasswordReset confirmations. On the mock, deleting reset tokens also destroys a pending email-confirmation token.
update_user_password has the same over-broad deletion: it removes all confirmations for the user, where Postgres deletes only PasswordReset ones. (Not called out in the original report — same class of bug as #1 in a sibling method.)
create_user checks only username uniqueness; a duplicate email silently overwrites the email index and the call succeeds, where Postgres returns RegistrationError::EmailTaken on the unique email constraint.
update_user never returns UpdateUserError::EmailTaken; it checks only username uniqueness, where Postgres returns EmailTaken on the unique email constraint.

Desired behavior:
The mock matches the Postgres adapter's observable contract:

Confirmation deletions in delete_password_reset_tokens_for_user and update_user_password are scoped to PasswordReset confirmations only, leaving EmailConfirmation (and any other action type) confirmations intact. The mock already demonstrates the correct pattern in mint_password_reset_confirmation, which retains confirmations unless both the user matches and the action type is PasswordReset.
create_user returns RegistrationError::EmailTaken when the email is already registered to another user, and does not mutate any state in that case.
update_user returns UpdateUserError::EmailTaken when the requested email already belongs to a different user. It must still allow a user to keep their own unchanged email — mirror the existing username guard, which only rejects when the value actually changed and collides with another record.

Key interfaces:

AppRepository::delete_password_reset_tokens_for_user and AppRepository::update_user_password on MemoryAppRepo — narrow the confirmation deletion to the PasswordReset action type.
AppRepository::create_user on MemoryAppRepo — enforce email uniqueness before inserting, returning RegistrationError::EmailTaken.
AppRepository::update_user on MemoryAppRepo — enforce email uniqueness on change, returning UpdateUserError::EmailTaken, without falsely rejecting an unchanged email.
The ConfirmationAction::PasswordReset variant and the confirmation's action_type() accessor already exist and are used elsewhere in the mock.

Acceptance criteria:

After delete_password_reset_tokens_for_user, a pending EmailConfirmation confirmation for the same user still exists; reset confirmations are gone.
After update_user_password, a pending EmailConfirmation confirmation for the same user still exists; reset confirmations are gone.
create_user with an email already registered to another user returns RegistrationError::EmailTaken and leaves existing state unchanged.
update_user targeting an email owned by a different user returns UpdateUserError::EmailTaken.
update_user that keeps the user's own email unchanged succeeds (no false EmailTaken).
The mock and Postgres adapters agree on all five behaviors above; where practical, shared behavioral tests exercise both.
mise run format, mise run clippy, and mise run ci pass.

Out of scope:

Any change to the Postgres adapter — it is the source of truth here; the mock moves to match it.
Broader schema/constraint changes or new confirmation action types.
Session or lockout behavior beyond what these four methods already touch.

> *This was generated by AI during triage.* ## Agent Brief **Category:** bug **Summary:** Bring the in-memory `MemoryAppRepo` mock in line with the Postgres adapter on confirmation-deletion scope and email-uniqueness enforcement, so tests can't pass on behavior the real repo rejects. **Current behavior:** The `MemoryAppRepo` mock (feature-gated, used by tests) diverges from the Postgres `AppRepository` implementation in four places, letting tests confirm behavior Postgres would reject: 1. `delete_password_reset_tokens_for_user` deletes **all** confirmations for the user, ignoring `action_type`. Postgres deletes only `PasswordReset` confirmations. On the mock, deleting reset tokens also destroys a pending email-confirmation token. 2. `update_user_password` has the **same** over-broad deletion: it removes all confirmations for the user, where Postgres deletes only `PasswordReset` ones. (Not called out in the original report — same class of bug as #1 in a sibling method.) 3. `create_user` checks only username uniqueness; a duplicate email silently overwrites the email index and the call succeeds, where Postgres returns `RegistrationError::EmailTaken` on the unique email constraint. 4. `update_user` never returns `UpdateUserError::EmailTaken`; it checks only username uniqueness, where Postgres returns `EmailTaken` on the unique email constraint. **Desired behavior:** The mock matches the Postgres adapter's observable contract: - Confirmation deletions in `delete_password_reset_tokens_for_user` and `update_user_password` are scoped to `PasswordReset` confirmations only, leaving `EmailConfirmation` (and any other action type) confirmations intact. The mock already demonstrates the correct pattern in `mint_password_reset_confirmation`, which retains confirmations unless both the user matches *and* the action type is `PasswordReset`. - `create_user` returns `RegistrationError::EmailTaken` when the email is already registered to another user, and does not mutate any state in that case. - `update_user` returns `UpdateUserError::EmailTaken` when the requested email already belongs to a *different* user. It must still allow a user to keep their own unchanged email — mirror the existing username guard, which only rejects when the value actually changed and collides with another record. **Key interfaces:** - `AppRepository::delete_password_reset_tokens_for_user` and `AppRepository::update_user_password` on `MemoryAppRepo` — narrow the confirmation deletion to the `PasswordReset` action type. - `AppRepository::create_user` on `MemoryAppRepo` — enforce email uniqueness before inserting, returning `RegistrationError::EmailTaken`. - `AppRepository::update_user` on `MemoryAppRepo` — enforce email uniqueness on change, returning `UpdateUserError::EmailTaken`, without falsely rejecting an unchanged email. - The `ConfirmationAction::PasswordReset` variant and the confirmation's `action_type()` accessor already exist and are used elsewhere in the mock. **Acceptance criteria:** - [ ] After `delete_password_reset_tokens_for_user`, a pending `EmailConfirmation` confirmation for the same user still exists; reset confirmations are gone. - [ ] After `update_user_password`, a pending `EmailConfirmation` confirmation for the same user still exists; reset confirmations are gone. - [ ] `create_user` with an email already registered to another user returns `RegistrationError::EmailTaken` and leaves existing state unchanged. - [ ] `update_user` targeting an email owned by a different user returns `UpdateUserError::EmailTaken`. - [ ] `update_user` that keeps the user's own email unchanged succeeds (no false `EmailTaken`). - [ ] The mock and Postgres adapters agree on all five behaviors above; where practical, shared behavioral tests exercise both. - [ ] `mise run format`, `mise run clippy`, and `mise run ci` pass. **Out of scope:** - Any change to the Postgres adapter — it is the source of truth here; the mock moves to match it. - Broader schema/constraint changes or new confirmation action types. - Session or lockout behavior beyond what these four methods already touch.

rosa added

bug

ready-for-agent

and removed

needs-triage

labels

2026-07-04 18:25:32 +00:00

rosa referenced this issue from a commit

2026-07-04 18:40:29 +00:00

domain: Align in-memory mock repo with Postgres constraints

rosa closed this issue

2026-07-04 18:40:29 +00:00