feat(auth): add email verification code backend services#3051
Open
feat(auth): add email verification code backend services#3051
Conversation
Implement server-side email verification using 6-digit codes with cooldown enforcement, expiry, and attempt limits. Includes DB migration, repository with advisory locks, Auth0 signup integration, notification template, and HTTP SDK methods.
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughAdds email verification: DB migration and Drizzle schema, repository and service for issuing/validating 6-digit codes, AuthController signup and verification endpoints, notification template and sender adjustments, Auth0 helpers, client SDK methods, routes/OpenAPI, and accompanying tests and snapshots. Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant AuthController
participant Auth0Service
participant EmailCodeService
participant Repo
participant NotificationService
participant Database
Client->>AuthController: POST /v1/auth/signup (email,password)
AuthController->>Auth0Service: createUser(email,password,connection)
Auth0Service->>Auth0Service: call Auth0 Management API
AuthController-->>Client: 204
Client->>AuthController: POST /v1/send-verification-code (auth)
AuthController->>EmailCodeService: sendCode(userId)
EmailCodeService->>Repo: countRecentByUserId(userId,since)
Repo->>Database: SELECT count(...)
EmailCodeService->>Repo: create(hashedCode,expiresAt,createdAt)
Repo->>Database: INSERT verification code
EmailCodeService->>NotificationService: create/send email (plaintext code)
EmailCodeService-->>AuthController: { codeSentAt }
AuthController-->>Client: 200 { data: { codeSentAt } }
Client->>AuthController: POST /v1/verify-email-code (code)
AuthController->>EmailCodeService: verifyCode(userId,code)
EmailCodeService->>Repo: findActiveByUserIdForUpdate(userId)
Repo->>Database: SELECT ... FOR UPDATE
alt code matches
EmailCodeService->>Repo: update user (emailVerified=true)
Repo->>Database: UPDATE users
EmailCodeService->>Auth0Service: markEmailVerified(auth0Id)
Auth0Service->>Auth0Service: call Auth0 Management API
else mismatch
EmailCodeService->>Repo: incrementAttempts(codeId)
Repo->>Database: UPDATE attempts
end
EmailCodeService-->>AuthController: success or error
AuthController-->>Client: 204 or error
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Poem
🚥 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)
Comment |
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #3051 +/- ##
==========================================
+ Coverage 59.71% 59.83% +0.12%
==========================================
Files 1034 1038 +4
Lines 24273 24367 +94
Branches 6003 6011 +8
==========================================
+ Hits 14494 14580 +86
- Misses 8528 8534 +6
- Partials 1251 1253 +2
*This pull request uses carry forward flags. Click here to find out more.
🚀 New features to boost your workflow:
|
![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: 2
🧹 Nitpick comments (10)
apps/api/src/notifications/services/notification-templates/email-verification-code-notification.ts (1)
10-12: Consider extracting the expiry duration to a shared constant.The "expires in 10 minutes" text is hardcoded here, while the actual expiry (
CODE_EXPIRY_MS = 10 * 60 * 1000) is defined inemail-verification-code.service.ts. If the expiry changes, these could drift out of sync.♻️ Optional: Share expiry constant
You could export
CODE_EXPIRY_MINUTESfrom the service or a shared constants file and interpolate it here:`This code expires in ${CODE_EXPIRY_MINUTES} minutes.`🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/api/src/notifications/services/notification-templates/email-verification-code-notification.ts` around lines 10 - 12, The template in email-verification-code-notification.ts hardcodes "expires in 10 minutes" while the real expiry is defined as CODE_EXPIRY_MS in email-verification-code.service.ts; to prevent drift, export a shared expiry constant (e.g., CODE_EXPIRY_MINUTES) from the service or a common constants module, import that constant into the notification template, and replace the hardcoded "10" with an interpolated ${CODE_EXPIRY_MINUTES} in the description string used by the EmailVerificationCodeNotification template.apps/api/src/auth/routes/signup/signup.router.ts (1)
9-12: Consider enhancing password validation or deferring to Auth0.The password schema only enforces
min(8). Auth0 typically has its own password policy (complexity, special characters, etc.) that will reject weak passwords anyway. However, for better UX, you might want to validate against Auth0's policy client-side or document that Auth0 rules apply.If you want to match common Auth0 default password policies:
password: z.string() .min(8) .regex(/[a-z]/, "Must contain lowercase") .regex(/[A-Z]/, "Must contain uppercase") .regex(/[0-9]/, "Must contain a number")Alternatively, keep it minimal and let Auth0 return validation errors (current approach is acceptable).
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/api/src/auth/routes/signup/signup.router.ts` around lines 9 - 12, The signup password validation currently only enforces min(8) on the signupInputSchema; either strengthen it to mirror Auth0's common policy by adding regex checks for lowercase, uppercase, and digit (e.g., add .regex(/[a-z]/), .regex(/[A-Z]/), .regex(/[0-9]/) to the password z.string()) or explicitly document in the file that Auth0 enforces stronger password rules and defer full validation to Auth0; update signupInputSchema accordingly (or add the explanatory comment next to signupInputSchema) so the intent is clear to future readers.apps/api/src/auth/services/auth0/auth0.service.spec.ts (1)
113-127: Consider usingvitest-mock-extendedfor mocking consistency.The setup function constructs mocks using
jest.fn()and casts tounknown as ManagementClient. Per coding guidelines, unit tests should usevitest-mock-extended(mock(),MockProxy<T>) for mocking.♻️ Suggested refactor using vitest-mock-extended
+import { mock, type MockProxy } from "vitest-mock-extended"; +import type { ManagementClient } from "auth0"; - function setup( - input: { - jobs?: { verifyEmail: jest.Mock }; - users?: { update?: jest.Mock; create?: jest.Mock }; - usersByEmail?: { getByEmail: jest.Mock }; - } = {} - ) { - const mockManagementClient = { - ...input - } as unknown as ManagementClient; + function setup() { + const mockManagementClient = mock<ManagementClient>(); const auth0Service = new Auth0Service(mockManagementClient); - return { auth0Service, mockManagementClient }; + return { auth0Service, managementClient: mockManagementClient }; }Then configure mock behavior in individual tests using
managementClient.users.create.mockResolvedValue(...).As per coding guidelines: "Use
vitest-mock-extended(mock(),MockProxy<T>) for mocking in unit tests."🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/api/src/auth/services/auth0/auth0.service.spec.ts` around lines 113 - 127, The setup helper currently builds ad-hoc jest mocks and casts to ManagementClient; replace this with vitest-mock-extended to create a properly typed mock: import and use mock<ManagementClient>() (typed as MockProxy<ManagementClient>) to create mockManagementClient inside setup, remove the manual jest.Mock shapes, and return { auth0Service, mockManagementClient } where tests can call typed methods like mockManagementClient.users.create.mockResolvedValue(...) or mockManagementClient.jobs.verifyEmail.mockResolvedValue(...); ensure Auth0Service is instantiated with the MockProxy so TypeScript-aware mocks are used throughout.apps/api/src/auth/routes/verify-email-code/verify-email-code.router.ts (1)
11-18: Minor redundancy in validation schema.The
.length(6)check is redundant since the regex/^\d{6}$/already enforces exactly 6 digits. However, this redundancy is harmless and provides defense-in-depth.♻️ Optional simplification
const VerifyEmailCodeRequestSchema = z.object({ data: z.object({ code: z .string() - .length(6) .regex(/^\d{6}$/, "Code must be exactly 6 digits") }) });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/api/src/auth/routes/verify-email-code/verify-email-code.router.ts` around lines 11 - 18, The schema VerifyEmailCodeRequestSchema redundantly applies .length(6) on the code field while the regex /^\d{6}$/ already enforces exactly six digits; remove the .length(6) call from the code validator so the field becomes z.string().regex(/^\d{6}$/, "Code must be exactly 6 digits") to simplify the validation while preserving the same constraint.apps/api/src/auth/services/email-verification-code/email-verification-code.service.spec.ts (2)
43-51: Consider asserting the specific error message or status code.The test only asserts that an error is thrown but doesn't verify it's specifically a 429 rate limit error. This could mask regressions if the error type changes.
💚 More specific assertion
it("throws 429 when rate limit exceeded", async () => { const user = UserSeeder.create({ email: "test@example.com" }); const { service, emailVerificationCodeRepository, userRepository } = setup(); userRepository.findById.mockResolvedValue(user); emailVerificationCodeRepository.countRecentByUserId.mockResolvedValue(5); - await expect(service.sendCode(user.id)).rejects.toThrow(); + await expect(service.sendCode(user.id)).rejects.toMatchObject({ + status: 429, + message: "Too many verification code requests. Please try again later." + }); });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/api/src/auth/services/email-verification-code/email-verification-code.service.spec.ts` around lines 43 - 51, The test currently just checks that service.sendCode(user.id) throws, which is too broad; update the assertion to verify it's specifically a 429 rate-limit error by asserting the thrown error's type/message/status. Locate the test and replace the generic rejects.toThrow() with an assertion that inspects the rejected error from service.sendCode (e.g., expect(...).rejects.toMatchObject({ status: 429 }) or expect(...).rejects.toThrowError(/429|rate limit/i) or assert instance of your RateLimitError class), keeping use of emailVerificationCodeRepository.countRecentByUserId and the same arrange/act setup.
53-68: Consider asserting specific error status codes for error cases.The 404 and 400 error tests only verify that an error is thrown, not the specific status. Verifying status codes ensures the correct HTTP responses reach clients.
💚 More specific assertions
it("throws 404 when user not found", async () => { const { service, userRepository } = setup(); userRepository.findById.mockResolvedValue(undefined); - await expect(service.sendCode("nonexistent")).rejects.toThrow(); + await expect(service.sendCode("nonexistent")).rejects.toMatchObject({ status: 404 }); }); it("throws 400 when user has no email", async () => { const user = UserSeeder.create({ email: null }); const { service, userRepository } = setup(); userRepository.findById.mockResolvedValue(user); - await expect(service.sendCode(user.id)).rejects.toThrow(); + await expect(service.sendCode(user.id)).rejects.toMatchObject({ status: 400 }); });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/api/src/auth/services/email-verification-code/email-verification-code.service.spec.ts` around lines 53 - 68, Update the two tests to assert the specific HTTP error status instead of a generic throw: when userRepository.findById returns undefined, call service.sendCode("nonexistent") and assert the rejected error has status 404 (e.g., check error.status or error.code); when UserSeeder.create({ email: null }) is returned, call service.sendCode(user.id) and assert the rejected error has status 400. Use the same service.sendCode and userRepository.findById symbols to locate the tests and replace await expect(...).rejects.toThrow() with assertions that inspect the thrown error object for the expected status.apps/api/src/auth/services/auth0/auth0.service.ts (2)
10-17: Consider returning the created user for downstream use.The
createUsermethod discards the Auth0 response. If the caller needs the created user's Auth0 ID (e.g., for immediate verification), returning it avoids an extra lookup.♻️ Optional: return created user data
- async createUser(input: { email: string; password: string; connection: string }): Promise<void> { - await this.managementClient.users.create({ + async createUser(input: { email: string; password: string; connection: string }): Promise<{ user_id: string }> { + const { data } = await this.managementClient.users.create({ email: input.email, password: input.password, connection: input.connection, verify_email: false }); + return { user_id: data.user_id! }; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/api/src/auth/services/auth0/auth0.service.ts` around lines 10 - 17, The createUser method currently discards the Auth0 response; change it to capture and return the created user so callers can access the Auth0 ID or other fields. Update the signature of createUser to return Promise<any> (or a proper Auth0 user type), assign the result of await this.managementClient.users.create({ ... }) to a const (e.g., createdUser) and return it; ensure callers are updated if they expect the previous void return. This references createUser and managementClient.users.create in the existing code.
23-25: Add explicit return type annotation for consistency.Other methods in this class have explicit return types. Adding
Promise<void>maintains consistency.♻️ Add return type
- async markEmailVerified(userId: string) { + async markEmailVerified(userId: string): Promise<void> { await this.managementClient.users.update({ id: userId }, { email_verified: true }); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/api/src/auth/services/auth0/auth0.service.ts` around lines 23 - 25, The method markEmailVerified lacks an explicit return type; update its signature to include Promise<void> (i.e., async markEmailVerified(userId: string): Promise<void>) to match other methods in the class and maintain consistency, leaving the body that calls this.managementClient.users.update({ id: userId }, { email_verified: true }) unchanged.apps/api/src/auth/controllers/auth/auth.controller.spec.ts (1)
30-38: Test doesn't verify the HTTP status code is preserved.The test only checks the message but doesn't verify that the 400 status code from Auth0 is preserved in the thrown error.
💚 Enhanced assertion
it("throws http error when auth0 returns a non-409 error", async () => { const { controller, auth0Service } = setup(); auth0Service.createUser.mockRejectedValue( new ResponseError(400, JSON.stringify({ message: "PasswordStrengthError: Password is too weak" }), new Headers()) ); - await expect(controller.signup({ email: "user@example.com", password: "weak" })).rejects.toThrow("PasswordStrengthError: Password is too weak"); + await expect(controller.signup({ email: "user@example.com", password: "weak" })).rejects.toMatchObject({ + status: 400, + message: "PasswordStrengthError: Password is too weak" + }); });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/api/src/auth/controllers/auth/auth.controller.spec.ts` around lines 30 - 38, The test for controller.signup currently only checks the error message but not that the original HTTP status code from auth0Service.createUser (a ResponseError) is preserved; update the test to await the rejected promise and capture the thrown error from controller.signup, then assert the error is the expected ResponseError (or has the same shape) and that its status (e.g., error.status or error.statusCode) equals 400 while still asserting the message contains "PasswordStrengthError: Password is too weak"; reference the mocked auth0Service.createUser, the controller.signup call, and the ResponseError type when making these assertions.apps/api/src/auth/repositories/email-verification-code/email-verification-code.repository.ts (1)
9-10: Narrow the repository input type.
Partial<Table["$inferInsert"]>exposes a much wider write surface than this repository actually needs, including DB-managed fields. An explicit input DTO orPick/Omit-based type will keep inserts harder to misuse and the contract clearer.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/api/src/auth/repositories/email-verification-code/email-verification-code.repository.ts` around lines 9 - 10, The current EmailVerificationCodeInput type uses Partial<Table["$inferInsert"]>, which exposes DB-managed fields; replace it with a narrowed DTO using Pick/Omit or an explicit interface that only includes the fields required for repository inserts (e.g., omit id, created_at, updated_at, and other DB-managed columns). Update the EmailVerificationCodeInput type declaration (and any insert/create methods that accept it) so it only accepts the writable properties the repository actually sets, while keeping EmailVerificationCodeDbOutput as the selected DB output type.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@apps/api/src/auth/controllers/auth/auth.controller.ts`:
- Around line 30-40: In the catch block that handles ResponseError in
auth.controller.ts (the code that inspects error instanceof ResponseError and
calls assert), guard the JSON.parse(error.body) call so a non-JSON body cannot
throw: attempt to parse error.body in a try/catch and if parsing fails fall back
to a safe message (e.g., use the raw error.body or a generic message like
"Unknown error from Auth0"); preserve the existing special-case mapping that
turns statusCode 409 into a 422 assertion message, and then call assert(false,
statusCode, message) using the safe message variable so the original parse
failure cannot crash the handler.
In `@apps/api/src/user/services/user/user.service.integration.ts`:
- Around line 347-350: In the test setup where EmailVerificationCodeService is
mocked (the mock<EmailVerificationCodeService> with sendCode), replace the Jest
compatibility global by using Vitest's native API: change
jest.fn().mockResolvedValue(...) to vi.fn().mockResolvedValue(...); ensure any
other occurrences in this test file use vi.fn() instead of jest.fn() so the mock
for sendCode and similar stubs use Vitest directly.
---
Nitpick comments:
In `@apps/api/src/auth/controllers/auth/auth.controller.spec.ts`:
- Around line 30-38: The test for controller.signup currently only checks the
error message but not that the original HTTP status code from
auth0Service.createUser (a ResponseError) is preserved; update the test to await
the rejected promise and capture the thrown error from controller.signup, then
assert the error is the expected ResponseError (or has the same shape) and that
its status (e.g., error.status or error.statusCode) equals 400 while still
asserting the message contains "PasswordStrengthError: Password is too weak";
reference the mocked auth0Service.createUser, the controller.signup call, and
the ResponseError type when making these assertions.
In
`@apps/api/src/auth/repositories/email-verification-code/email-verification-code.repository.ts`:
- Around line 9-10: The current EmailVerificationCodeInput type uses
Partial<Table["$inferInsert"]>, which exposes DB-managed fields; replace it with
a narrowed DTO using Pick/Omit or an explicit interface that only includes the
fields required for repository inserts (e.g., omit id, created_at, updated_at,
and other DB-managed columns). Update the EmailVerificationCodeInput type
declaration (and any insert/create methods that accept it) so it only accepts
the writable properties the repository actually sets, while keeping
EmailVerificationCodeDbOutput as the selected DB output type.
In `@apps/api/src/auth/routes/signup/signup.router.ts`:
- Around line 9-12: The signup password validation currently only enforces
min(8) on the signupInputSchema; either strengthen it to mirror Auth0's common
policy by adding regex checks for lowercase, uppercase, and digit (e.g., add
.regex(/[a-z]/), .regex(/[A-Z]/), .regex(/[0-9]/) to the password z.string()) or
explicitly document in the file that Auth0 enforces stronger password rules and
defer full validation to Auth0; update signupInputSchema accordingly (or add the
explanatory comment next to signupInputSchema) so the intent is clear to future
readers.
In `@apps/api/src/auth/routes/verify-email-code/verify-email-code.router.ts`:
- Around line 11-18: The schema VerifyEmailCodeRequestSchema redundantly applies
.length(6) on the code field while the regex /^\d{6}$/ already enforces exactly
six digits; remove the .length(6) call from the code validator so the field
becomes z.string().regex(/^\d{6}$/, "Code must be exactly 6 digits") to simplify
the validation while preserving the same constraint.
In `@apps/api/src/auth/services/auth0/auth0.service.spec.ts`:
- Around line 113-127: The setup helper currently builds ad-hoc jest mocks and
casts to ManagementClient; replace this with vitest-mock-extended to create a
properly typed mock: import and use mock<ManagementClient>() (typed as
MockProxy<ManagementClient>) to create mockManagementClient inside setup, remove
the manual jest.Mock shapes, and return { auth0Service, mockManagementClient }
where tests can call typed methods like
mockManagementClient.users.create.mockResolvedValue(...) or
mockManagementClient.jobs.verifyEmail.mockResolvedValue(...); ensure
Auth0Service is instantiated with the MockProxy so TypeScript-aware mocks are
used throughout.
In `@apps/api/src/auth/services/auth0/auth0.service.ts`:
- Around line 10-17: The createUser method currently discards the Auth0
response; change it to capture and return the created user so callers can access
the Auth0 ID or other fields. Update the signature of createUser to return
Promise<any> (or a proper Auth0 user type), assign the result of await
this.managementClient.users.create({ ... }) to a const (e.g., createdUser) and
return it; ensure callers are updated if they expect the previous void return.
This references createUser and managementClient.users.create in the existing
code.
- Around line 23-25: The method markEmailVerified lacks an explicit return type;
update its signature to include Promise<void> (i.e., async
markEmailVerified(userId: string): Promise<void>) to match other methods in the
class and maintain consistency, leaving the body that calls
this.managementClient.users.update({ id: userId }, { email_verified: true })
unchanged.
In
`@apps/api/src/auth/services/email-verification-code/email-verification-code.service.spec.ts`:
- Around line 43-51: The test currently just checks that
service.sendCode(user.id) throws, which is too broad; update the assertion to
verify it's specifically a 429 rate-limit error by asserting the thrown error's
type/message/status. Locate the test and replace the generic rejects.toThrow()
with an assertion that inspects the rejected error from service.sendCode (e.g.,
expect(...).rejects.toMatchObject({ status: 429 }) or
expect(...).rejects.toThrowError(/429|rate limit/i) or assert instance of your
RateLimitError class), keeping use of
emailVerificationCodeRepository.countRecentByUserId and the same arrange/act
setup.
- Around line 53-68: Update the two tests to assert the specific HTTP error
status instead of a generic throw: when userRepository.findById returns
undefined, call service.sendCode("nonexistent") and assert the rejected error
has status 404 (e.g., check error.status or error.code); when
UserSeeder.create({ email: null }) is returned, call service.sendCode(user.id)
and assert the rejected error has status 400. Use the same service.sendCode and
userRepository.findById symbols to locate the tests and replace await
expect(...).rejects.toThrow() with assertions that inspect the thrown error
object for the expected status.
In
`@apps/api/src/notifications/services/notification-templates/email-verification-code-notification.ts`:
- Around line 10-12: The template in email-verification-code-notification.ts
hardcodes "expires in 10 minutes" while the real expiry is defined as
CODE_EXPIRY_MS in email-verification-code.service.ts; to prevent drift, export a
shared expiry constant (e.g., CODE_EXPIRY_MINUTES) from the service or a common
constants module, import that constant into the notification template, and
replace the hardcoded "10" with an interpolated ${CODE_EXPIRY_MINUTES} in the
description string used by the EmailVerificationCodeNotification template.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: 102172ac-ceac-4fbf-a247-c6f8aa6c9946
⛔ Files ignored due to path filters (1)
apps/api/test/functional/__snapshots__/docs.spec.ts.snapis excluded by!**/*.snap
📒 Files selected for processing (25)
apps/api/drizzle/0029_fluffy_drax.sqlapps/api/drizzle/meta/0029_snapshot.jsonapps/api/drizzle/meta/_journal.jsonapps/api/src/auth/controllers/auth/auth.controller.spec.tsapps/api/src/auth/controllers/auth/auth.controller.tsapps/api/src/auth/model-schemas/email-verification-code/email-verification-code.schema.tsapps/api/src/auth/model-schemas/index.tsapps/api/src/auth/repositories/email-verification-code/email-verification-code.repository.tsapps/api/src/auth/routes/index.tsapps/api/src/auth/routes/send-verification-code/send-verification-code.router.tsapps/api/src/auth/routes/signup/signup.router.tsapps/api/src/auth/routes/verify-email-code/verify-email-code.router.tsapps/api/src/auth/services/auth0/auth0.service.spec.tsapps/api/src/auth/services/auth0/auth0.service.tsapps/api/src/auth/services/email-verification-code/email-verification-code.service.spec.tsapps/api/src/auth/services/email-verification-code/email-verification-code.service.tsapps/api/src/notifications/services/notification-templates/email-verification-code-notification.spec.tsapps/api/src/notifications/services/notification-templates/email-verification-code-notification.tsapps/api/src/rest-app.tsapps/api/src/user/services/user/user.service.integration.tsapps/api/src/user/services/user/user.service.spec.tsapps/api/src/user/services/user/user.service.tsapps/notifications/src/modules/notifications/services/email-sender/email-sender.service.tspackages/http-sdk/src/auth/auth-http.service.tspackages/http-sdk/src/auth/auth-http.types.ts
Replace http-assert with http-errors for error handling consistency. Add safe JSON parsing for Auth0 error bodies. Fix UserSeeder import to use existing createUser function and replace jest.fn with vi.fn.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
apps/api/src/auth/services/email-verification-code/email-verification-code.service.spec.ts (1)
43-68: Tighten negative-path assertions to check expected error shape.A few tests only use
rejects.toThrow(), which can pass on unrelated failures. Since the test names specify 429/404/400 behavior, assert status/message explicitly to prevent false positives.Example pattern
- await expect(service.sendCode(user.id)).rejects.toThrow(); + await expect(service.sendCode(user.id)).rejects.toMatchObject({ + status: 429 + });Also applies to: 99-109, 138-146
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/api/src/auth/services/email-verification-code/email-verification-code.service.spec.ts` around lines 43 - 68, The tests for email-verification-code.service are using generic rejects.toThrow() and should assert the specific error shape and status to avoid false positives; update the negative tests around service.sendCode to capture the rejected error from sendCode (when userRepository.findById returns undefined, when user.email is null, and when emailVerificationCodeRepository.countRecentByUserId returns 5) and assert the thrown error contains the expected HTTP status codes (429, 404, 400) and/or message fields rather than a generic toThrow; locate these assertions near service.sendCode, userRepository.findById and emailVerificationCodeRepository.countRecentByUserId in the spec and replace the loose rejects.toThrow() with explicit checks on error.status or error.message (or toMatchObject) matching the test names.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@apps/api/src/auth/services/email-verification-code/email-verification-code.service.spec.ts`:
- Line 15: The test file imports EmailVerificationCodeService via a relative
path; update the import to use the TypeScript path alias for backend sources
(use the `@src/`* alias instead of "./email-verification-code.service") so the
spec imports EmailVerificationCodeService from the aliased module location
matching the project's backend alias rules; ensure the import string uses
`@src/`.../email-verification-code.service and keep the imported symbol
EmailVerificationCodeService unchanged.
---
Nitpick comments:
In
`@apps/api/src/auth/services/email-verification-code/email-verification-code.service.spec.ts`:
- Around line 43-68: The tests for email-verification-code.service are using
generic rejects.toThrow() and should assert the specific error shape and status
to avoid false positives; update the negative tests around service.sendCode to
capture the rejected error from sendCode (when userRepository.findById returns
undefined, when user.email is null, and when
emailVerificationCodeRepository.countRecentByUserId returns 5) and assert the
thrown error contains the expected HTTP status codes (429, 404, 400) and/or
message fields rather than a generic toThrow; locate these assertions near
service.sendCode, userRepository.findById and
emailVerificationCodeRepository.countRecentByUserId in the spec and replace the
loose rejects.toThrow() with explicit checks on error.status or error.message
(or toMatchObject) matching the test names.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: 6455759f-cda3-47af-979b-c7d41a25085f
📒 Files selected for processing (5)
apps/api/src/auth/controllers/auth/auth.controller.spec.tsapps/api/src/auth/controllers/auth/auth.controller.tsapps/api/src/auth/services/email-verification-code/email-verification-code.service.spec.tsapps/api/src/user/services/user/user.service.integration.tsapps/api/src/user/services/user/user.service.spec.ts
✅ Files skipped from review due to trivial changes (1)
- apps/api/src/auth/controllers/auth/auth.controller.spec.ts
🚧 Files skipped from review as they are similar to previous changes (3)
- apps/api/src/user/services/user/user.service.integration.ts
- apps/api/src/user/services/user/user.service.spec.ts
- apps/api/src/auth/controllers/auth/auth.controller.ts
apps/api/src/auth/services/email-verification-code/email-verification-code.service.spec.ts
Outdated
Show resolved
Hide resolved
- Replace jest.fn/jest.Mock with vi.fn/Mock from vitest in auth0.service.spec - Remove redundant .length(6) validation (regex already enforces 6 digits) - Parallelize independent DB reads in sendCode via Promise.all
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
apps/api/src/auth/services/email-verification-code/email-verification-code.service.ts (1)
67-90: Consider invalidating the verification code after successful verification.After the code is successfully verified (line 87), the code record remains active in the database until it expires. While not a security issue (the user is already verified), this means:
- Unnecessary DB lookups on subsequent calls until expiration.
- The code could theoretically be "verified" multiple times (idempotent but wasteful).
Consider deleting or marking the code as used after successful verification.
♻️ Suggested fix
await this.userRepository.updateById(userInternalId, { emailVerified: true }); + await this.emailVerificationCodeRepository.deleteById(record.id); return user.userId;This would require adding a
deleteByIdmethod to the repository if it doesn't exist.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/api/src/auth/services/email-verification-code/email-verification-code.service.ts` around lines 67 - 90, The verifyCodeInTransaction method leaves the verification record active after successful verification; update it to invalidate the record (e.g., call emailVerificationCodeRepository.deleteById(record.id) or an equivalent mark-as-used method) immediately after setting user.emailVerified (or before returning) so the code cannot be reused and DB lookups stop; add or use a deleteById/markUsed method on emailVerificationCodeRepository if missing and ensure this delete runs inside the same transaction/WithTransaction context.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@apps/api/src/auth/services/email-verification-code/email-verification-code.service.ts`:
- Around line 59-65: The verifyCode flow can leave local DB and Auth0
inconsistent because auth0Service.markEmailVerified(auth0UserId) is called
outside the transaction; wrap the call in a try-catch inside verifyCode
(referencing verifyCode and verifyCodeInTransaction), on failure log a distinct
event (e.g., "EMAIL_VERIFIED_MARK_AUTH0_FAILED") with auth0UserId,
userInternalId and the error via this.logger.error, and implement retry
semantics (e.g., limited immediate retries with backoff) or enqueue a background
reconciliation job/message for later retry; ensure the controller still surfaces
a clear error when retries/external reconciliation are scheduled and document
that downstream flows rely on local emailVerified state.
---
Nitpick comments:
In
`@apps/api/src/auth/services/email-verification-code/email-verification-code.service.ts`:
- Around line 67-90: The verifyCodeInTransaction method leaves the verification
record active after successful verification; update it to invalidate the record
(e.g., call emailVerificationCodeRepository.deleteById(record.id) or an
equivalent mark-as-used method) immediately after setting user.emailVerified (or
before returning) so the code cannot be reused and DB lookups stop; add or use a
deleteById/markUsed method on emailVerificationCodeRepository if missing and
ensure this delete runs inside the same transaction/WithTransaction context.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: 9a529fde-fcf6-4214-9b76-f11dc46ea15c
⛔ Files ignored due to path filters (1)
apps/api/test/functional/__snapshots__/docs.spec.ts.snapis excluded by!**/*.snap
📒 Files selected for processing (3)
apps/api/src/auth/routes/verify-email-code/verify-email-code.router.tsapps/api/src/auth/services/auth0/auth0.service.spec.tsapps/api/src/auth/services/email-verification-code/email-verification-code.service.ts
🚧 Files skipped from review as they are similar to previous changes (2)
- apps/api/src/auth/services/auth0/auth0.service.spec.ts
- apps/api/src/auth/routes/verify-email-code/verify-email-code.router.ts
apps/api/src/auth/services/email-verification-code/email-verification-code.service.ts
Show resolved
Hide resolved
- Use @src/* path alias instead of relative import in spec file - Add try-catch with structured error logging around Auth0 markEmailVerified
stalniy
reviewed
Apr 13, 2026
| userIdIdx: index("email_verification_codes_user_id_idx").on(table.userId), | ||
| expiresAtIdx: index("email_verification_codes_expires_at_idx").on(table.expiresAt) | ||
| }) | ||
| ); |
Contributor
There was a problem hiding this comment.
Consider using a unique index on userId instead of a regular index. This would:
- Structurally enforce one code per user at the DB level — no need for
SELECT ... FOR UPDATEwithORDER BY+LIMIT 1in the repository - Simplify
sendCode— use upsert (INSERT ... ON CONFLICT DO UPDATE) which is atomic, eliminating race conditions between concurrentsendCodecalls - Simplify
verifyCode—FOR UPDATEquery becomes justWHERE userId = ?with no ordering
Rate limiting can be derived from the existing expiresAt column: if expiresAt > now() + (EXPIRY - COOLDOWN), the code was created within the cooldown window. No need for countRecentByUserId or multiple rows.
|
|
||
| const codeBuffer = Buffer.from(hashCode(code)); | ||
| const recordBuffer = Buffer.from(record.code); | ||
| const isCodeValid = codeBuffer.length === recordBuffer.length && timingSafeEqual(recordBuffer, codeBuffer); |
Contributor
There was a problem hiding this comment.
Since the code is already hashed with SHA-256 before storage, timingSafeEqual is unnecessary here. Timing attacks exploit character-by-character comparison of secrets — but the attacker would be trying to guess a SHA-256 hash, which is meaningless for timing side-channels.
You could instead query the DB directly:
WHERE userId = ? AND code = hash(input) AND expiresAt > now()This eliminates the need to fetch the row and compare in application code.
| ]); | ||
| assert(user, 404, "User not found"); | ||
| assert(user.email, 400, "User has no email address"); | ||
| assert(recentCount < MAX_CODES_PER_WINDOW, 429, "Too many verification code requests. Please try again later."); |
Contributor
There was a problem hiding this comment.
Race condition: Two concurrent sendCode requests can both pass this rate-limit check simultaneously (both read the same countRecentByUserId before either inserts), allowing both to insert — effectively bypassing the rate limit.
With a unique index on userId + upsert, this goes away structurally since the upsert is atomic.
| private async verifyCodeInTransaction(userInternalId: string, code: string): Promise<string> { | ||
| const [user, record] = await Promise.all([ | ||
| this.userRepository.findById(userInternalId), | ||
| this.emailVerificationCodeRepository.findActiveByUserIdForUpdate(userInternalId) |
Contributor
There was a problem hiding this comment.
Race condition between verify and send: verifyCodeInTransaction locks the row with FOR UPDATE, but sendCode does a plain INSERT (different row, no lock). A new code can be inserted while verification of the old one is in progress — the transaction isolation doesn't help because it's a different row.
With a unique index, there's only ever one row per user, so FOR UPDATE during verify also blocks concurrent upserts on the same row.
Contributor
|
@baktun14 could you please explain why you decided to use I also don’t understand why there are multiple rows per user in the table. How do you plan to use it? |
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
2 participants
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.
Why
Part of CON-197 — Replace Auth0's default email verification link with a 6-digit code flow.
This is PR 1 of 2 — backend only. Split from #2824 to make review manageable.
PR 2 (frontend): #3052 (targets this branch).
What
email_verification_codestable with expiry and attempt limitsEmailVerificationCodeRepositorywith Drizzle ORM andSELECT ... FOR UPDATEEmailVerificationCodeServicewith cooldown enforcement, code generation, and verificationPOST /v1/send-verification-code,POST /v1/verify-email-code,POST /v1/signupcreateUsermethod for signup flow (409 sanitized to generic 422)UserService: sends verification code on registrationmarkEmailVerifiedcall moved outside transactionsendVerificationCode()andverifyEmailCode()methods26 files changed (~2,350 additions)
Summary by CodeRabbit
New Features
Tests