Conversation
Contributor
WalkthroughThis PR introduces an email verification system for user registration by adding a service layer that orchestrates user creation and verification token generation within a transaction, refactoring database queries to use generic executors, and adding email utility support. Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant Handler as register_user Handler
participant Service as Service Layer
participant Database as Database
participant Email as Email Utility
Client->>Handler: POST /auth/register (email, username, password)
Handler->>Service: new_user(pool, email, username, password)
Service->>Database: BEGIN TRANSACTION
Service->>Database: create_user (insert into users, RETURNING id)
Database-->>Service: user_id (Uuid)
Service->>Service: Generate token (UUID string)
Service->>Service: Calculate expiry (now + 24h)
Service->>Database: create_verification_token (insert into tokens)
Database-->>Service: confirmation
Service->>Database: COMMIT
Database-->>Service: success
Service-->>Handler: (user_id, token)
Handler->>Email: send_verification_email(email, token)
Email-->>Handler: Ok(())
Handler-->>Client: 201 Created
Estimated Code Review Effort🎯 3 (Moderate) | ⏱️ ~20 minutes The changes span multiple coordinated layers with new logic around transaction handling, generic executor patterns, and token generation—but they follow a clear, cohesive architectural pattern that's easy to trace. Possibly Related PRs
🎓 Professional Dev InsightYou're organizing your auth flow beautifully here, mentor! Notice how you've separated concerns nicely:
This generic 🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
rust-server/src/features/auth/types.rs (1)
28-34: 🧹 Nitpick | 🔵 TrivialConsider: The
Tokenstruct isn't used in the current flow.I notice you've added
SerializetoToken, which is good forward-thinking. However, looking at yourservice.rs, you're returning(Uuid, String)tuple instead of using this struct.A pro tip for cleaner code: when you have a well-defined struct like
Token, consider using it as your return type instead of anonymous tuples. It makes your code self-documenting:// Instead of: Result<(Uuid, String), AppError> // Consider: Result<Token, AppError> or a dedicated NewUserResult structThis isn't blocking—the
#[allow(dead_code)]shows you're aware—but keep it in mind as you flesh out the feature.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@rust-server/src/features/auth/types.rs` around lines 28 - 34, The Token struct is defined and serialized but not used: update the service function currently returning Result<(Uuid, String), AppError> to return Result<Token, AppError> (or a NewUserResult wrapper) — construct a Token (populate token, expires_at, user_id) where you currently build the tuple, update the function signature and any callers in service.rs to accept Token, and remove #[allow(dead_code)] from Token once it's used; ensure the serialization remains by keeping #[derive(Serialize, Debug)] on Token.rust-server/src/features/auth/handlers.rs (1)
35-52:⚠️ Potential issue | 🟠 MajorRace condition: uniqueness checks happen outside the transaction.
Here's a subtle but important bug pattern to learn. Your flow is:
- Check if email exists (line 35) — outside transaction
- Check if username exists (line 39) — outside transaction
- Call
new_user()which starts transaction and inserts (line 46)The problem: Between steps 1-2 and step 3, another concurrent request could insert the same email/username. Your database constraints will catch it, but the error bubbles up as a raw SQL constraint violation instead of your friendly "Email already in use" message.
Professional fix: Either move the checks inside the transaction in
service.rs, or handle the unique constraint violation in your error mapping to return the appropriate user-friendly message.🛠️ Suggested approach: Handle constraint violations gracefully
In your error handling (or in the handler), catch the specific unique constraint violation:
// In the match/error handling for new_user result: match new_user(db, &cleaned_data.email, &cleaned_data.username, &hashed_password).await { Ok((user_id, token)) => { /* success path */ }, Err(AppError::Sql(sqlx::Error::Database(db_err))) if db_err.is_unique_violation() => { // Parse which constraint failed and return appropriate message return Err(AppError::conflict("Email or username already in use")); }, Err(e) => return Err(e), }Alternatively, move the existence checks into
new_user()so they run within the transaction.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@rust-server/src/features/auth/handlers.rs` around lines 35 - 52, The current uniqueness checks (find_user_by_email and find_user_by_username) run outside the transaction and can race with concurrent inserts; update the handler to either move those checks inside the transaction in new_user (so do existence checks in new_user in service.rs) or catch and map SQL unique-constraint errors from new_user: match the Err(AppError::Sql(sqlx::Error::Database(db_err))) case, use db_err.is_unique_violation() and inspect db_err.constraint() (or message) to return AppError::conflict("Email already in use") or AppError::conflict("Username already in use") as appropriate, otherwise rethrow the error.rust-server/src/features/auth/queries.rs (1)
18-23:⚠️ Potential issue | 🔴 CriticalUse the correct DB column name for stored passwords (
password_hash).Line [18] inserts into
password, but theuserstable schema definespassword_hash. This will fail during registration with a SQL column error.Proposed fix
pub async fn create_user<'e, E>( db: E, email: &str, username: &str, - password: &str, + password_hash: &str, ) -> Result<Uuid, AppError> where E: Executor<'e, Database = Postgres>, { let row = sqlx::query_scalar::<_, Uuid>( - "INSERT INTO users (email, username, password) VALUES ($1, $2, $3) RETURNING id", + "INSERT INTO users (email, username, password_hash) VALUES ($1, $2, $3) RETURNING id", ) .bind(email) .bind(username) - .bind(password) + .bind(password_hash) .fetch_one(db)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@rust-server/src/features/auth/queries.rs` around lines 18 - 23, The INSERT uses the wrong column name for stored passwords; update the SQL in the user creation query to use password_hash instead of password and ensure the bound value matches (replace "INSERT INTO users (email, username, password) ..." with "INSERT INTO users (email, username, password_hash) ..." in the query used in this file and keep the .bind(...) that supplies the hashed password (e.g., .bind(password) or rename that variable to password_hash if you have one) so the bound parameter matches the password_hash column; locate the query in rust-server/src/features/auth/queries.rs where .bind(email).bind(username).bind(password).fetch_one(db) is used (the user creation function) and make the column name change there.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@rust-server/Cargo.toml`:
- Around line 23-24: Add a short TODO comment above the resend-rs dependency in
Cargo.toml to indicate it's intentionally added for future email integration;
update the file so the line near "resend-rs = \"0.21.1\"" is preceded by a
comment like "# TODO: Integrate resend-rs email functionality" to make the
intent explicit for future maintainers (references: dependency names "resend-rs"
and "uuid").
In `@rust-server/documentation.md`:
- Around line 324-326: The eprintln in send_verification_email is logging
sensitive PII and secrets (the full to_email and token); remove that direct
logging and instead log only non-sensitive context such as the email domain. In
the send_verification_email function replace the eprintln that prints
"{to_email} with token: {token}" with logic that extracts the domain (e.g.,
split to_email on '@' and fallback to "unknown") and log only the domain and a
safe message; do not log the token or full address anywhere. Also audit any
similar eprintln/println in authentication/account recovery helpers and update
them to the same domain-only pattern.
In `@rust-server/src/features/auth/handlers.rs`:
- Around line 54-55: Replace the two eprintln! debug statements that print
sensitive data in handlers.rs with non-production-safe logging: remove
eprintln!("USER ID: {:#?}", user_id) and eprintln!("VERIFICATION TOKEN: {:#?}",
token) and instead use tracing::debug! with a redacted token; e.g., emit
tracing::debug! for the user_id only if needed and log the token as a
masked/hashed value (e.g., show first 4 chars and length or a hash) so the full
verification token is never printed; update the same pattern as in email.rs to
ensure no raw tokens are logged.
In `@rust-server/src/features/auth/service.rs`:
- Around line 18-19: The expiry for generated tokens is hardcoded when creating
expires_at in service.rs; make it configurable by adding a token_expiry_hours
(u32) field to your config (e.g., in config.rs) and replace the literal 24 with
config.token_expiry_hours cast to i64 when computing chrono::Duration::hours;
update any callers/constructors that build the service to pass the config and
add a default of 24 for existing behavior (so tests can override to shorter
values).
In `@rust-server/src/utils/email.rs`:
- Around line 3-7: Remove the eprintln! in send_verification_email and stop
printing PII or tokens; instead use tracing::debug! (to match handlers.rs) for
non-sensitive diagnostics and log only non-secret metadata (e.g., that an email
was queued/sent or the recipient domain or a truncated/hashed identifier, never
the full address or token). Update the function
send_verification_email(to_email: &str, token: &str) -> Result<(), AppError> to
replace direct stderr output with a tracing::debug! call that omits the raw
token and full email, and ensure any error paths return or log AppError via
tracing without exposing secrets.
---
Outside diff comments:
In `@rust-server/src/features/auth/handlers.rs`:
- Around line 35-52: The current uniqueness checks (find_user_by_email and
find_user_by_username) run outside the transaction and can race with concurrent
inserts; update the handler to either move those checks inside the transaction
in new_user (so do existence checks in new_user in service.rs) or catch and map
SQL unique-constraint errors from new_user: match the
Err(AppError::Sql(sqlx::Error::Database(db_err))) case, use
db_err.is_unique_violation() and inspect db_err.constraint() (or message) to
return AppError::conflict("Email already in use") or
AppError::conflict("Username already in use") as appropriate, otherwise rethrow
the error.
In `@rust-server/src/features/auth/queries.rs`:
- Around line 18-23: The INSERT uses the wrong column name for stored passwords;
update the SQL in the user creation query to use password_hash instead of
password and ensure the bound value matches (replace "INSERT INTO users (email,
username, password) ..." with "INSERT INTO users (email, username,
password_hash) ..." in the query used in this file and keep the .bind(...) that
supplies the hashed password (e.g., .bind(password) or rename that variable to
password_hash if you have one) so the bound parameter matches the password_hash
column; locate the query in rust-server/src/features/auth/queries.rs where
.bind(email).bind(username).bind(password).fetch_one(db) is used (the user
creation function) and make the column name change there.
In `@rust-server/src/features/auth/types.rs`:
- Around line 28-34: The Token struct is defined and serialized but not used:
update the service function currently returning Result<(Uuid, String), AppError>
to return Result<Token, AppError> (or a NewUserResult wrapper) — construct a
Token (populate token, expires_at, user_id) where you currently build the tuple,
update the function signature and any callers in service.rs to accept Token, and
remove #[allow(dead_code)] from Token once it's used; ensure the serialization
remains by keeping #[derive(Serialize, Debug)] on Token.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 3c20d76e-c3a0-4d50-9dd6-87fd6868e192
⛔ Files ignored due to path filters (1)
rust-server/Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (10)
rust-server/Cargo.tomlrust-server/documentation.mdrust-server/src/features/auth/handlers.rsrust-server/src/features/auth/mod.rsrust-server/src/features/auth/queries.rsrust-server/src/features/auth/routes.rsrust-server/src/features/auth/service.rsrust-server/src/features/auth/types.rsrust-server/src/utils/email.rsrust-server/src/utils/mod.rs
This was referenced Mar 31, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary by CodeRabbit
New Features
Chores