feat: add user-specific email verification setting#24298
feat: add user-specific email verification setting#24298keithwillcode merged 15 commits intomainfrom
Conversation
🤖 Devin AI EngineerI'll be helping with this pull request! Here's what you should know: ✅ I will automatically:
Note: I can only respond to comments from users who have write access to this repository. ⚙️ Control Options:
|
WalkthroughAdds per-user Booker email verification across the stack: UI toggle and i18n keys; Prisma schema + migration adding users.requiresBookerEmailVerification; UserRepository selection and new lookup methods exposing the flag; session and viewer handlers return the flag; profile update schema accepts the field; booking flows (checkIfBookerEmailIsBlocked, handleNewBooking, addGuests, and a public viewer check) consult the flag, accept/validate an optional verificationCode, filter or block guests/bookings accordingly, and migrate error handling to ErrorWithCode; tests and test fixtures updated to cover verification scenarios. Possibly related PRs
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Disabled knowledge base sources:
📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
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 |
packages/features/bookings/lib/handleNewBooking/checkIfBookerEmailIsBlocked.ts
Outdated
Show resolved
Hide resolved
packages/prisma/migrations/20251006111422_add_requires_booker_email_verification/migration.sql
Outdated
Show resolved
Hide resolved
- Remove unrelated Watchlist index drops from migration - Add missing Watchlist indexes to schema.prisma to fix drift - Refactor checkIfBookerEmailIsBlocked to throw ErrorWithCode - Move HttpError handling to handleNewBooking caller layer Addresses review comments on PR #24298 Co-Authored-By: keith@cal.com <keithwillcode@gmail.com>
|
The latest updates on your projects. Learn more about Vercel for GitHub. |
packages/trpc/server/routers/publicViewer/checkIfUserEmailVerificationRequired.handler.ts
Outdated
Show resolved
Hide resolved
packages/trpc/server/routers/viewer/bookings/addGuests.handler.ts
Outdated
Show resolved
Hide resolved
…Watchlist changes - Add findByEmailWithEmailVerificationSetting method to UserRepository - Add findManyByEmailsWithEmailVerificationSettings method to UserRepository - Refactor checkIfUserEmailVerificationRequired handler to use UserRepository - Refactor addGuests handler to use UserRepository - Remove unrelated Watchlist schema indices (organizationId/isGlobal, source) - Remove unrelated WatchlistAudit unique constraint on id Addresses review comments on PR #24298 Co-Authored-By: keith@cal.com <keithwillcode@gmail.com>
- Remove unrelated Watchlist index drops from migration - Add missing Watchlist indexes to schema.prisma to fix drift - Refactor checkIfBookerEmailIsBlocked to throw ErrorWithCode - Move HttpError handling to handleNewBooking caller layer Addresses review comments on PR #24298 Co-Authored-By: keith@cal.com <keithwillcode@gmail.com>
…Watchlist changes - Add findByEmailWithEmailVerificationSetting method to UserRepository - Add findManyByEmailsWithEmailVerificationSettings method to UserRepository - Refactor checkIfUserEmailVerificationRequired handler to use UserRepository - Refactor addGuests handler to use UserRepository - Remove unrelated Watchlist schema indices (organizationId/isGlobal, source) - Remove unrelated WatchlistAudit unique constraint on id Addresses review comments on PR #24298 Co-Authored-By: keith@cal.com <keithwillcode@gmail.com>
rodrigoehlers
force-pushed
the
devin/booker-email-verification-setting-1759751444
branch
from
645d3a5 to
2580c48
Compare
- Remove unrelated Watchlist index drops from migration - Add missing Watchlist indexes to schema.prisma to fix drift - Refactor checkIfBookerEmailIsBlocked to throw ErrorWithCode - Move HttpError handling to handleNewBooking caller layer Addresses review comments on PR #24298 Co-Authored-By: keith@cal.com <keithwillcode@gmail.com>
…Watchlist changes - Add findByEmailWithEmailVerificationSetting method to UserRepository - Add findManyByEmailsWithEmailVerificationSettings method to UserRepository - Refactor checkIfUserEmailVerificationRequired handler to use UserRepository - Refactor addGuests handler to use UserRepository - Remove unrelated Watchlist schema indices (organizationId/isGlobal, source) - Remove unrelated WatchlistAudit unique constraint on id Addresses review comments on PR #24298 Co-Authored-By: keith@cal.com <keithwillcode@gmail.com>
rodrigoehlers
force-pushed
the
devin/booker-email-verification-setting-1759751444
branch
from
2580c48 to
af2e5c6
Compare
- Remove unrelated Watchlist index drops from migration - Add missing Watchlist indexes to schema.prisma to fix drift - Refactor checkIfBookerEmailIsBlocked to throw ErrorWithCode - Move HttpError handling to handleNewBooking caller layer Addresses review comments on PR #24298 Co-Authored-By: keith@cal.com <keithwillcode@gmail.com>
…Watchlist changes - Add findByEmailWithEmailVerificationSetting method to UserRepository - Add findManyByEmailsWithEmailVerificationSettings method to UserRepository - Refactor checkIfUserEmailVerificationRequired handler to use UserRepository - Refactor addGuests handler to use UserRepository - Remove unrelated Watchlist schema indices (organizationId/isGlobal, source) - Remove unrelated WatchlistAudit unique constraint on id Addresses review comments on PR #24298 Co-Authored-By: keith@cal.com <keithwillcode@gmail.com>
rodrigoehlers
force-pushed
the
devin/booker-email-verification-setting-1759751444
branch
from
1abe87e to
342c638
Compare
rodrigoehlers
force-pushed
the
devin/booker-email-verification-setting-1759751444
branch
from
6745766 to
f6e8f45
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
♻️ Duplicate comments (2)
packages/trpc/server/routers/viewer/bookings/addGuests.handler.ts (1)
30-46: Refactor DB access: use repositories and select (no include).
This handler still performs direct Prisma calls and uses include (and pulls user.credentials). Prior guidance asked to move DB access to repositories and avoid include; select only needed fields. Also avoid fetching credentials here.As per coding guidelines
Also applies to: 129-136
packages/features/bookings/lib/handleNewBooking.ts (1)
510-521: Don’t coerce all verification errors to 403; map to proper HTTP status and preserve dataMap codes to 400/401/403 and pass error.data through.
try { await checkIfBookerEmailIsBlocked({ loggedInUserId: userId, bookerEmail, verificationCode: reqBody.verificationCode, }); } catch (error) { if (error instanceof ErrorWithCode) { - throw new HttpError({ statusCode: 403, message: error.message }); + const status = + error.code === ErrorCode.InvalidVerificationCode || + error.code === ErrorCode.UnableToValidateVerificationCode + ? 400 + : error.code === ErrorCode.BookerEmailRequiresLogin + ? 401 + : 403; + throw new HttpError({ + statusCode: status, + message: error.message, + // keep any extra payload (e.g., { email }) + // eslint-disable-next-line @typescript-eslint/no-explicit-any + data: (error as any).data, + }); } throw error; }
🧹 Nitpick comments (1)
packages/trpc/server/routers/viewer/bookings/addGuests.handler.ts (1)
104-113: Precompute attendee/blacklist sets for O(1) lookups and fewer recomputations.
Replace repeated some()/includes() + extract calls with Sets for clarity and performance.Apply this diff within the current block:
- const uniqueGuests = deduplicatedGuests.filter((guest) => { - const baseGuestEmail = extractBaseEmail(guest).toLowerCase(); - return ( - !booking.attendees.some( - (attendee) => extractBaseEmail(attendee.email).toLowerCase() === baseGuestEmail - ) && - !blacklistedGuestEmails.includes(baseGuestEmail) && - !emailToRequiresVerification.get(baseGuestEmail) - ); - }); + const uniqueGuests = deduplicatedGuests.filter((guest) => { + const baseGuestEmail = extractBaseEmail(guest).toLowerCase(); + return ( + !baseAttendeeEmails.has(baseGuestEmail) && + !blacklistedGuestEmailsSet.has(baseGuestEmail) && + !emailToRequiresVerification.get(baseGuestEmail) + ); + });Add these declarations just above the filter:
const baseAttendeeEmails = new Set( booking.attendees.map((a) => extractBaseEmail(a.email).toLowerCase()) ); const blacklistedGuestEmailsSet = new Set(blacklistedGuestEmails);
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (13)
apps/web/modules/settings/my-account/general-view.tsx(2 hunks)apps/web/public/static/locales/en/common.json(1 hunks)packages/features/bookings/lib/handleNewBooking.ts(8 hunks)packages/features/bookings/lib/handleNewBooking/checkIfBookerEmailIsBlocked.ts(2 hunks)packages/features/users/repositories/UserRepository.ts(5 hunks)packages/lib/errorCodes.ts(1 hunks)packages/prisma/migrations/20251006111422_add_requires_booker_email_verification/migration.sql(1 hunks)packages/prisma/schema.prisma(1 hunks)packages/trpc/server/middlewares/sessionMiddleware.ts(3 hunks)packages/trpc/server/routers/publicViewer/checkIfUserEmailVerificationRequired.handler.ts(2 hunks)packages/trpc/server/routers/viewer/bookings/addGuests.handler.ts(3 hunks)packages/trpc/server/routers/viewer/me/get.handler.ts(1 hunks)packages/trpc/server/routers/viewer/me/updateProfile.schema.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (6)
- packages/trpc/server/routers/publicViewer/checkIfUserEmailVerificationRequired.handler.ts
- packages/prisma/schema.prisma
- apps/web/public/static/locales/en/common.json
- packages/prisma/migrations/20251006111422_add_requires_booker_email_verification/migration.sql
- apps/web/modules/settings/my-account/general-view.tsx
- packages/trpc/server/middlewares/sessionMiddleware.ts
🧰 Additional context used
📓 Path-based instructions (4)
**/*.ts
📄 CodeRabbit inference engine (.cursor/rules/review.mdc)
**/*.ts: For Prisma queries, only select data you need; never useinclude, always useselect
Ensure thecredential.keyfield is never returned from tRPC endpoints or APIs
Files:
packages/trpc/server/routers/viewer/me/get.handler.tspackages/trpc/server/routers/viewer/me/updateProfile.schema.tspackages/features/bookings/lib/handleNewBooking/checkIfBookerEmailIsBlocked.tspackages/trpc/server/routers/viewer/bookings/addGuests.handler.tspackages/features/users/repositories/UserRepository.tspackages/features/bookings/lib/handleNewBooking.tspackages/lib/errorCodes.ts
**/*.{ts,tsx}
📄 CodeRabbit inference engine (.cursor/rules/review.mdc)
Flag excessive Day.js use in performance-critical code; prefer native Date or Day.js
.utc()in hot paths like loops
Files:
packages/trpc/server/routers/viewer/me/get.handler.tspackages/trpc/server/routers/viewer/me/updateProfile.schema.tspackages/features/bookings/lib/handleNewBooking/checkIfBookerEmailIsBlocked.tspackages/trpc/server/routers/viewer/bookings/addGuests.handler.tspackages/features/users/repositories/UserRepository.tspackages/features/bookings/lib/handleNewBooking.tspackages/lib/errorCodes.ts
**/*.{ts,tsx,js,jsx}
⚙️ CodeRabbit configuration file
Flag default exports and encourage named exports. Named exports provide better tree-shaking, easier refactoring, and clearer imports. Exempt main components like pages, layouts, and components that serve as the primary export of a module.
Files:
packages/trpc/server/routers/viewer/me/get.handler.tspackages/trpc/server/routers/viewer/me/updateProfile.schema.tspackages/features/bookings/lib/handleNewBooking/checkIfBookerEmailIsBlocked.tspackages/trpc/server/routers/viewer/bookings/addGuests.handler.tspackages/features/users/repositories/UserRepository.tspackages/features/bookings/lib/handleNewBooking.tspackages/lib/errorCodes.ts
**/*Repository.ts
📄 CodeRabbit inference engine (.cursor/rules/review.mdc)
Repository files must include
Repositorysuffix, prefix with technology if applicable (e.g.,PrismaAppRepository.ts), and use PascalCase matching the exported class
Files:
packages/features/users/repositories/UserRepository.ts
🧠 Learnings (1)
📚 Learning: 2025-09-03T09:52:51.182Z
Learnt from: hariombalhara
PR: calcom/cal.com#23541
File: packages/features/bookings/lib/di/modules/RegularBookingServiceModule.ts:22-28
Timestamp: 2025-09-03T09:52:51.182Z
Learning: The IBookingServiceDependencies interface in packages/features/bookings/lib/handleNewBooking.ts contains 6 properties: cacheService, checkBookingAndDurationLimitsService, prismaClient, bookingRepository, featuresRepository, and checkBookingLimitsService. This interface is used by RegularBookingService and matches the depsMap structure in RegularBookingServiceModule.
Applied to files:
packages/features/bookings/lib/handleNewBooking.ts
🧬 Code graph analysis (3)
packages/trpc/server/routers/viewer/bookings/addGuests.handler.ts (2)
packages/features/users/repositories/UserRepository.ts (1)
UserRepository(124-1181)packages/emails/email-manager.ts (1)
sendAddGuestsEmails(672-697)
packages/features/users/repositories/UserRepository.ts (2)
packages/prisma/zod-utils.ts (1)
userMetadata(327-349)packages/types/Calendar.d.ts (1)
SelectedCalendar(321-324)
packages/features/bookings/lib/handleNewBooking.ts (2)
packages/features/bookings/lib/handleNewBooking/checkIfBookerEmailIsBlocked.ts (1)
checkIfBookerEmailIsBlocked(7-92)packages/lib/http-error.ts (1)
HttpError(1-45)
🔇 Additional comments (16)
packages/trpc/server/routers/viewer/bookings/addGuests.handler.ts (4)
6-7: Good adoption of UserRepository and base-email util.
Aligns handler with repository pattern and shared normalization.
84-93: Base-email dedupe looks good.
Prevents alias/plus-tag duplicates within the same request.
94-103: Keying by matchedEmail is correct.
Prevents misses when repository matched a secondary email.
212-214: Sending only to filtered guests and logging real error: LGTM.
Correctly avoids emailing blocked/verification-required addresses and preserves error context.packages/trpc/server/routers/viewer/me/get.handler.ts (1)
143-143: Expose requiresBookerEmailVerification — looks goodField correctly surfaced from user to API consumer.
packages/trpc/server/routers/viewer/me/updateProfile.schema.ts (1)
24-24: Schema extension is appropriateOptional boolean is fine and consistent with other flags.
packages/lib/errorCodes.ts (1)
29-32: New error codes aligned with featureEnum additions are clear and non-breaking.
packages/features/bookings/lib/handleNewBooking.ts (3)
1191-1195: Guest removal by per-user setting — logic reads wellCase-insensitive base matching via map is correct.
Please confirm findManyByEmailsWithEmailVerificationSettings treats input emails case-insensitively and accounts for secondary emails to avoid false negatives.
26-26: Import/type surface updates look consistentNew types and repos are correctly imported as types; tree-shaking unaffected.
Also applies to: 39-39, 41-41, 47-47, 61-61, 98-98
451-459: No DI update needed:PrismaUserRepositoryis already provided inapps/api/v2/src/lib/modules/regular-booking.module.ts, souserRepositoryinjection is satisfied.Likely an incorrect or invalid review comment.
packages/features/bookings/lib/handleNewBooking/checkIfBookerEmailIsBlocked.ts (1)
22-24: Blocking logic and Prisma select look correct
- Case-insensitive env match and minimal select are good.
- ErrorWithCode usage (no HttpError here) aligns with layering guidance.
Also applies to: 48-51, 54-59, 86-90
packages/features/users/repositories/UserRepository.ts (5)
14-15: LGTM!The new imports for
DestinationCalendar,SelectedCalendar, andPrismaare properly added to support the new type definitions and raw SQL query method.
52-64: LGTM!The SessionUser type updates improve type safety by using proper Prisma types for
destinationCalendar,metadata,profiles,allSelectedCalendars, anduserLevelSelectedCalendars. The newrequiresBookerEmailVerificationfield correctly supports the per-user email verification feature.
112-112: LGTM!Adding
requiresBookerEmailVerificationto the select object ensures the field is retrieved when needed.
306-318: LGTM!The method properly normalizes emails, handles empty input, and delegates to the raw SQL method. The inclusion of
matchedEmailin the return type is useful for distinguishing whether the primary or secondary email matched.
1005-1005: LGTM!Adding
requiresBookerEmailVerificationto the session user select ensures the field is available in session data for booking flows to enforce email verification checks.
| const guestEmails = (reqGuests || []).map((email) => extractBaseEmail(email).toLowerCase()); | ||
| const guestUsers = await userRepository.findManyByEmailsWithEmailVerificationSettings({ | ||
| emails: guestEmails, | ||
| }); | ||
|
|
||
| const emailToRequiresVerification = new Map<string, boolean>(); | ||
| for (const user of guestUsers) { | ||
| const matchedBase = extractBaseEmail(user.matchedEmail ?? user.email).toLowerCase(); | ||
| emailToRequiresVerification.set(matchedBase, user.requiresBookerEmailVerification === true); | ||
| } | ||
|
|
There was a problem hiding this comment.
Deduplicate guest emails before lookup; avoid logging raw PII
- Use a Set to dedupe base emails to reduce query load.
- guestsRemoved is later logged (Lines 1211-1213); avoid logging raw emails. Log count and masked values instead.
- const guestEmails = (reqGuests || []).map((email) => extractBaseEmail(email).toLowerCase());
+ const guestEmails = Array.from(
+ new Set((reqGuests || []).map((email) => extractBaseEmail(email).toLowerCase()))
+ );Example masking for later log (near Lines 1211-1213):
- log.info("Removed guests from the booking", guestsRemoved);
+ const masked = guestsRemoved.map((e) => {
+ const [local, domain] = e.split("@");
+ return (local?.slice(0, 2) || "") + "***@" + (domain || "");
+ });
+ log.info("Removed guests from the booking", { count: guestsRemoved.length, masked });📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const guestEmails = (reqGuests || []).map((email) => extractBaseEmail(email).toLowerCase()); | |
| const guestUsers = await userRepository.findManyByEmailsWithEmailVerificationSettings({ | |
| emails: guestEmails, | |
| }); | |
| const emailToRequiresVerification = new Map<string, boolean>(); | |
| for (const user of guestUsers) { | |
| const matchedBase = extractBaseEmail(user.matchedEmail ?? user.email).toLowerCase(); | |
| emailToRequiresVerification.set(matchedBase, user.requiresBookerEmailVerification === true); | |
| } | |
| const guestEmails = Array.from( | |
| new Set((reqGuests || []).map((email) => extractBaseEmail(email).toLowerCase())) | |
| ); | |
| const guestUsers = await userRepository.findManyByEmailsWithEmailVerificationSettings({ | |
| emails: guestEmails, | |
| }); | |
| const emailToRequiresVerification = new Map<string, boolean>(); | |
| for (const user of guestUsers) { | |
| const matchedBase = extractBaseEmail(user.matchedEmail ?? user.email).toLowerCase(); | |
| emailToRequiresVerification.set(matchedBase, user.requiresBookerEmailVerification === true); | |
| } |
🤖 Prompt for AI Agents
In packages/features/bookings/lib/handleNewBooking.ts around lines 1171 to 1181,
dedupe guest base-emails before querying and avoid handling raw PII in logs:
compute a Set of extractBaseEmail(email).toLowerCase() from reqGuests, convert
that Set to an array and pass it to
userRepository.findManyByEmailsWithEmailVerificationSettings to reduce query
load, build the emailToRequiresVerification map from the returned users using
matchedBase as before, and replace any future logging of removed/filtered guest
emails (near lines 1211-1213) to log only the count and masked forms (e.g.,
partial redaction) rather than raw email strings.
| try { | ||
| isValid = await verifyCodeUnAuthenticated(baseEmail, verificationCode); | ||
| } catch { |
There was a problem hiding this comment.
🧩 Analysis chain
Bug: verification code validated against baseEmail instead of the exact email
Codes are issued to the original address; validating with a stripped base can cause false negatives for plus-addressing/aliases.
- isValid = await verifyCodeUnAuthenticated(baseEmail, verificationCode);
+ isValid = await verifyCodeUnAuthenticated(bookerEmail, verificationCode);🏁 Script executed:
#!/bin/bash
# Audit all usages to ensure we validate against the exact email elsewhere
rg -n -C2 'verifyCodeUnAuthenticated\s*\('Length of output: 2120
🏁 Script executed:
#!/bin/bash
# Inspect definition and usage of baseEmail and bookerEmail in the file
rg -n -C3 'baseEmail' packages/features/bookings/lib/handleNewBooking/checkIfBookerEmailIsBlocked.ts
rg -n -C3 'bookerEmail' packages/features/bookings/lib/handleNewBooking/checkIfBookerEmailIsBlocked.tsLength of output: 1831
Replace baseEmail with bookerEmail in verification call
- isValid = await verifyCodeUnAuthenticated(baseEmail, verificationCode);
+ isValid = await verifyCodeUnAuthenticated(bookerEmail, verificationCode);Ensures codes issued to alias addresses validate correctly.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| try { | |
| isValid = await verifyCodeUnAuthenticated(baseEmail, verificationCode); | |
| } catch { | |
| try { | |
| isValid = await verifyCodeUnAuthenticated(bookerEmail, verificationCode); | |
| } catch { |
🤖 Prompt for AI Agents
In
packages/features/bookings/lib/handleNewBooking/checkIfBookerEmailIsBlocked.ts
around lines 70 to 72, the call to verifyCodeUnAuthenticated currently passes
baseEmail but should pass bookerEmail so verification uses the actual booker
address (ensuring codes issued to alias addresses validate correctly); update
the function call to use bookerEmail and keep existing try/catch logic intact.
| async findByEmailWithEmailVerificationSetting({ email }: { email: string }) { | ||
| const user = await this.prismaClient.user.findFirst({ | ||
| where: { | ||
| OR: [ | ||
| { | ||
| email: email.toLowerCase(), | ||
| emailVerified: { not: null }, | ||
| }, | ||
| { | ||
| secondaryEmails: { | ||
| some: { | ||
| email: email.toLowerCase(), | ||
| emailVerified: { not: null }, | ||
| }, | ||
| }, | ||
| }, | ||
| ], | ||
| }, | ||
| select: { | ||
| id: true, | ||
| email: true, | ||
| requiresBookerEmailVerification: true, | ||
| }, | ||
| }); | ||
| return user; | ||
| } |
There was a problem hiding this comment.
Add locked user filtering for consistency and security.
The method doesn't filter out locked users, but findVerifiedUsersByEmailsRaw at lines 340 and 354 explicitly filters locked = FALSE. This inconsistency could allow locked users to bypass email verification checks in booking flows.
Apply this diff to add the locked user filter:
async findByEmailWithEmailVerificationSetting({ email }: { email: string }) {
const user = await this.prismaClient.user.findFirst({
where: {
+ locked: false,
OR: [
{
email: email.toLowerCase(),
emailVerified: { not: null },
},📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| async findByEmailWithEmailVerificationSetting({ email }: { email: string }) { | |
| const user = await this.prismaClient.user.findFirst({ | |
| where: { | |
| OR: [ | |
| { | |
| email: email.toLowerCase(), | |
| emailVerified: { not: null }, | |
| }, | |
| { | |
| secondaryEmails: { | |
| some: { | |
| email: email.toLowerCase(), | |
| emailVerified: { not: null }, | |
| }, | |
| }, | |
| }, | |
| ], | |
| }, | |
| select: { | |
| id: true, | |
| email: true, | |
| requiresBookerEmailVerification: true, | |
| }, | |
| }); | |
| return user; | |
| } | |
| async findByEmailWithEmailVerificationSetting({ email }: { email: string }) { | |
| const user = await this.prismaClient.user.findFirst({ | |
| where: { | |
| locked: false, | |
| OR: [ | |
| { | |
| email: email.toLowerCase(), | |
| emailVerified: { not: null }, | |
| }, | |
| { | |
| secondaryEmails: { | |
| some: { | |
| email: email.toLowerCase(), | |
| emailVerified: { not: null }, | |
| }, | |
| }, | |
| }, | |
| ], | |
| }, | |
| select: { | |
| id: true, | |
| email: true, | |
| requiresBookerEmailVerification: true, | |
| }, | |
| }); | |
| return user; | |
| } |
🤖 Prompt for AI Agents
In packages/features/users/repositories/UserRepository.ts around lines 279 to
304, the query in findByEmailWithEmailVerificationSetting doesn't exclude locked
users unlike findVerifiedUsersByEmailsRaw; update the Prisma where clause to
include locked: false (e.g., add locked: false at the top-level of the where so
both the primary email and secondaryEmails branches respect the locked=false
filter) so locked accounts are consistently excluded from verified-email
lookups.
| private async findVerifiedUsersByEmailsRaw(emails: string[]) { | ||
| const emailListSql = Prisma.join(emails.map((e) => Prisma.sql`${e}`)); | ||
| return this.prismaClient.$queryRaw< | ||
| Array<{ | ||
| id: number; | ||
| email: string; | ||
| matchedEmail: string; | ||
| requiresBookerEmailVerification: boolean; | ||
| }> | ||
| >(Prisma.sql` | ||
| SELECT | ||
| u."id", | ||
| u."email", | ||
| u."email" AS "matchedEmail", | ||
| u."requiresBookerEmailVerification" | ||
| FROM | ||
| "public"."users" AS u | ||
| WHERE | ||
| u."email" IN (${emailListSql}) | ||
| AND u."emailVerified" IS NOT NULL | ||
| AND u."locked" = FALSE | ||
| UNION | ||
| SELECT | ||
| u."id", | ||
| u."email", | ||
| t0."email" AS "matchedEmail", | ||
| u."requiresBookerEmailVerification" | ||
| FROM | ||
| "public"."users" AS u | ||
| INNER JOIN "public"."SecondaryEmail" AS t0 | ||
| ON t0."userId" = u."id" | ||
| WHERE | ||
| t0."email" IN (${emailListSql}) | ||
| AND t0."emailVerified" IS NOT NULL | ||
| AND u."locked" = FALSE | ||
| `); | ||
| } |
There was a problem hiding this comment.
Prevent duplicate results when a user has multiple matching emails.
The UNION query doesn't deduplicate results. If a user has both their primary email and one of their secondary emails in the emails array, they'll appear twice in the results (once from each SELECT). This could lead to duplicate processing in calling code.
Apply this diff to deduplicate results using UNION instead of keeping it as-is, or use a CTE with DISTINCT:
- >(Prisma.sql`
+ >(Prisma.sql`
+ WITH matched_users AS (
SELECT
u."id",
u."email",
u."email" AS "matchedEmail",
u."requiresBookerEmailVerification"
FROM
"public"."users" AS u
WHERE
u."email" IN (${emailListSql})
AND u."emailVerified" IS NOT NULL
AND u."locked" = FALSE
UNION
SELECT
u."id",
u."email",
t0."email" AS "matchedEmail",
u."requiresBookerEmailVerification"
FROM
"public"."users" AS u
INNER JOIN "public"."SecondaryEmail" AS t0
ON t0."userId" = u."id"
WHERE
t0."email" IN (${emailListSql})
AND t0."emailVerified" IS NOT NULL
AND u."locked" = FALSE
+ )
+ SELECT DISTINCT ON ("id", "matchedEmail")
+ "id",
+ "email",
+ "matchedEmail",
+ "requiresBookerEmailVerification"
+ FROM matched_users
`);Alternatively, if you need all matched emails per user, consider returning a structure that groups them by user ID in the calling code.
Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In packages/features/users/repositories/UserRepository.ts around lines 320-356,
the current UNION returns duplicate rows when a user matches both primary and
secondary emails because the matchedEmail column differs; change the query to
first UNION all matches in a CTE or subquery and then SELECT from that CTE
grouping by user id (and other user fields) to deduplicate — e.g. aggregate
matched emails into an array (array_agg) or use DISTINCT ON (u.id) /
MIN(matchedEmail) to ensure each user appears only once while preserving matched
email(s) as a list.
- Add 9 test scenarios covering user email verification setting - Test main booker verification (logged in/out, with/without code) - Test secondary email verification as main booker and guest - Test guest filtering when verification is required - Test plus-addressed email handling - Test multiple guests with mixed verification requirements - Test invalid verification code error handling - Update bookingScenario helper to support requiresBookerEmailVerification and secondaryEmails Co-Authored-By: keith@cal.com <keithwillcode@gmail.com>
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (10)
apps/web/test/utils/bookingScenario/bookingScenario.ts (4)
12-12: Decouple tests from repositories when possibleImporting ProfileRepository solely for UID generation couples test utils to domain code. Consider generating a UID locally (e.g., using uuidv4 already imported) unless the exact format from ProfileRepository is required by assertions.
839-853: Batch-create secondary emails to speed up testsCreate entries in bulk to reduce per-item awaits.
Apply this diff:
- for (const user of users) { - if (user.secondaryEmails) { - log.debug("Creating SecondaryEmail entries for user", user.id); - for (const secondaryEmail of user.secondaryEmails) { - await prismock.secondaryEmail.create({ - data: { - email: secondaryEmail.email, - emailVerified: secondaryEmail.emailVerified, - userId: user.id, - }, - }); - } - } - } + for (const user of users) { + if (user.secondaryEmails?.length) { + log.debug("Creating SecondaryEmail entries for user", user.id); + await prismock.secondaryEmail.createMany({ + data: user.secondaryEmails.map((se) => ({ + email: se.email, + emailVerified: se.emailVerified, + userId: user.id, + })), + }); + } + }
865-865: Prefer select over include for Prisma queriesSwitch to select with the minimal fields/relations required to align with the project guidelines. This avoids over-fetching and matches production patterns. As per coding guidelines
1575-1577: getOrganizer: plumbed-through fields; consider defaults and shared types
- Optionally default requiresBookerEmailVerification to false to mirror DB default and avoid undefined in tests.
- Extract a shared SecondaryEmailInput type and reuse it here and in InputUser to avoid duplication.
Also applies to: 1595-1597, 1619-1621
packages/features/bookings/lib/handleNewBooking/test/booking-validations.test.ts (6)
752-755: Use Prisma select instead of include (tests too)Select only fields you need. Avoid include per guidelines.
As per coding guidelines
- const booking = await prismaMock.booking.findFirst({ - where: { id: createdBooking.id }, - include: { attendees: true }, - }); + const booking = await prismaMock.booking.findFirst({ + where: { id: createdBooking.id }, + select: { attendees: { select: { email: true } } }, + });
836-839: Use select over include here as wellKeep Prisma usage consistent; select only attendee email.
As per coding guidelines
- const booking = await prismaMock.booking.findFirst({ - where: { id: createdBooking.id }, - include: { attendees: true }, - }); + const booking = await prismaMock.booking.findFirst({ + where: { id: createdBooking.id }, + select: { attendees: { select: { email: true } } }, + });
1000-1003: Prefer select here tooAvoid include; select attendee email only.
As per coding guidelines
- const booking = await prismaMock.booking.findFirst({ - where: { id: createdBooking.id }, - include: { attendees: true }, - }); + const booking = await prismaMock.booking.findFirst({ + where: { id: createdBooking.id }, + select: { attendees: { select: { email: true } } }, + });
516-527: Also assert verification isn’t called when logged inStrengthen signal: when owner is logged in, code verification should not run.
const createdBooking = await handleNewBooking({ bookingData: mockBookingData, userId: 201, }); + const { verifyCodeUnAuthenticated } = await import("@calcom/trpc/server/routers/viewer/auth/util"); + expect(verifyCodeUnAuthenticated).not.toHaveBeenCalled();
752-760: Add explicit null-check on booking fetch (clearer failures)If booking isn’t found, tests fail with undefined errors. Assert non-null for clarity.
- const guestEmails = booking?.attendees.map((a) => a.email).filter((e) => e !== booker.email); + expect(booking).toBeTruthy(); + const guestEmails = booking!.attendees.map((a) => a.email).filter((e) => e !== booker.email);Also applies to: 841-844, 1005-1012
846-912: Add case-insensitive email test (optional)Recommend a test using User@Example.com to ensure CI-matching across primary/secondary emails and plus-address normalization.
I can add a parametric test covering case-insensitive main/secondary emails and plus-address combos. Want me to draft it?
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (2)
apps/web/test/utils/bookingScenario/bookingScenario.ts(7 hunks)packages/features/bookings/lib/handleNewBooking/test/booking-validations.test.ts(10 hunks)
🧰 Additional context used
📓 Path-based instructions (3)
**/*.ts
📄 CodeRabbit inference engine (.cursor/rules/review.mdc)
**/*.ts: For Prisma queries, only select data you need; never useinclude, always useselect
Ensure thecredential.keyfield is never returned from tRPC endpoints or APIs
Files:
packages/features/bookings/lib/handleNewBooking/test/booking-validations.test.tsapps/web/test/utils/bookingScenario/bookingScenario.ts
**/*.{ts,tsx}
📄 CodeRabbit inference engine (.cursor/rules/review.mdc)
Flag excessive Day.js use in performance-critical code; prefer native Date or Day.js
.utc()in hot paths like loops
Files:
packages/features/bookings/lib/handleNewBooking/test/booking-validations.test.tsapps/web/test/utils/bookingScenario/bookingScenario.ts
**/*.{ts,tsx,js,jsx}
⚙️ CodeRabbit configuration file
Flag default exports and encourage named exports. Named exports provide better tree-shaking, easier refactoring, and clearer imports. Exempt main components like pages, layouts, and components that serve as the primary export of a module.
Files:
packages/features/bookings/lib/handleNewBooking/test/booking-validations.test.tsapps/web/test/utils/bookingScenario/bookingScenario.ts
🧬 Code graph analysis (1)
packages/features/bookings/lib/handleNewBooking/test/booking-validations.test.ts (1)
apps/web/test/utils/bookingScenario/bookingScenario.ts (7)
getBooker(2274-2288)getOrganizer(1557-1622)TestData(1276-1548)getGoogleCalendarCredential(1229-1237)createBookingScenario(1015-1046)getScenarioData(1624-1712)mockCalendarToHaveNoBusySlots(2013-2026)
⏰ Context from checks skipped due to timeout of 180000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: Linters / lint
- GitHub Check: Tests / Unit
- GitHub Check: Type check / check-types
🔇 Additional comments (3)
apps/web/test/utils/bookingScenario/bookingScenario.ts (1)
243-245: InputUser extensions look goodrequiresBookerEmailVerification and secondaryEmails additions align with the feature and test use cases.
packages/features/bookings/lib/handleNewBooking/test/booking-validations.test.ts (2)
27-29: Mock wiring looks goodModule-level vi.mock + dynamic import pattern is correct and isolates verifyCodeUnAuthenticated for each test.
39-41: Good cleanup of env stateafterEach reset of BLACKLISTED_GUEST_EMAILS prevents test bleed.
| await expect( | ||
| handleNewBooking({ | ||
| bookingData: mockBookingData, | ||
| }) | ||
| ).rejects.toThrow( | ||
| "Attendee email has been blocked. Make sure to login as organizer@example.com to use this email for creating a booking." | ||
| ); |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
🧩 Analysis chain
Assert error codes, not messages
Message strings are brittle. PR introduces ErrorWithCode and new codes; assert on error.code instead.
Example change for one test (apply similarly to others):
- await expect(
- handleNewBooking({
- bookingData: mockBookingData,
- })
- ).rejects.toThrow(
- "Attendee email has been blocked. Make sure to login as user@example.com to use this email for creating a booking."
- );
+ await expect(
+ handleNewBooking({
+ bookingData: mockBookingData,
+ })
+ ).rejects.toHaveProperty("code", "BookerEmailRequiresLogin");Run this to find the exact code identifiers to use and where they’re thrown:
Also applies to: 179-184, 447-454, 670-676, 906-910, 1078-1083
🏁 Script executed:
#!/bin/bash
# Locate new error codes and their throw sites
rg -nP -C2 '(BookerEmail[A-Za-z]+|InvalidVerificationCode|VerificationCode)' packages | sed -n '1,200p'
rg -nPU -C3 '(?s)throw new\s+ErrorWithCode\('
# Find tests asserting the specific strings to update
rg -nP -C2 'toThrow\([^)]*Attendee email has been blocked|Invalid verification code|booker_limit_exceeded_error' --type=tsLength of output: 29705
Assert on error codes, not messages
Replace all .toThrow(<message>) and expect(error.message).toEqual(...) in booking-validations.test.ts with assertions on the code property via rejects.toHaveProperty('code', ErrorCode.XYZ) (or expect(error).toHaveProperty('code', ErrorCode.XYZ)).
Import ErrorCode from @calcom/lib/errorCodes.
Apply to all occurrences at lines 179–184, 447–454, 670–676, 906–910, and 1078–1083.
🤖 Prompt for AI Agents
In
packages/features/bookings/lib/handleNewBooking/test/booking-validations.test.ts
around lines 101–107 and additionally at 179–184, 447–454, 670–676, 906–910,
1078–1083, replace assertions that check error messages (e.g.,
.toThrow(<message>) or expect(error.message).toEqual(...)) with assertions on
the error.code property: import ErrorCode from '@calcom/lib/errorCodes' at the
top of the test file, then for promise rejections use await
expect(...).rejects.toHaveProperty('code', ErrorCode.<APPROPRIATE_CODE>) or for
caught errors use expect(error).toHaveProperty('code',
ErrorCode.<APPROPRIATE_CODE>); ensure you choose the matching ErrorCode constant
for each assertion and remove the string message checks.
Move guests array from top-level booking data into responses object to match expected structure in getBookingData.ts which looks for responses.guests (line 74). Fixes three failing tests: - should filter out guest that requires verification - should filter out secondary email with verification when added as guest - should filter only guests requiring verification from multiple guests Co-Authored-By: keith@cal.com <keithwillcode@gmail.com>
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
packages/features/bookings/lib/handleNewBooking/test/booking-validations.test.ts (1)
129-136: Asserterror.code, not brittle messagesPrevious feedback still applies: these tests need to check the new
ErrorWithCodevalues instead of string messages. ImportErrorCodeand switch each.rejects.toThrow("…")/expect(error.message)torejects.toHaveProperty("code", ErrorCode.BookerEmailRequiresLogin)(and the appropriate constant per case). Please update all occurrences in this file.Also applies to: 208-213, 303-307, 396-407, 476-483, 699-705, 951-956, 1136-1141
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (2)
apps/web/test/utils/bookingScenario/getMockRequestDataForBooking.ts(1 hunks)packages/features/bookings/lib/handleNewBooking/test/booking-validations.test.ts(10 hunks)
🧰 Additional context used
📓 Path-based instructions (3)
**/*.ts
📄 CodeRabbit inference engine (.cursor/rules/review.mdc)
**/*.ts: For Prisma queries, only select data you need; never useinclude, always useselect
Ensure thecredential.keyfield is never returned from tRPC endpoints or APIs
Files:
apps/web/test/utils/bookingScenario/getMockRequestDataForBooking.tspackages/features/bookings/lib/handleNewBooking/test/booking-validations.test.ts
**/*.{ts,tsx}
📄 CodeRabbit inference engine (.cursor/rules/review.mdc)
Flag excessive Day.js use in performance-critical code; prefer native Date or Day.js
.utc()in hot paths like loops
Files:
apps/web/test/utils/bookingScenario/getMockRequestDataForBooking.tspackages/features/bookings/lib/handleNewBooking/test/booking-validations.test.ts
**/*.{ts,tsx,js,jsx}
⚙️ CodeRabbit configuration file
Flag default exports and encourage named exports. Named exports provide better tree-shaking, easier refactoring, and clearer imports. Exempt main components like pages, layouts, and components that serve as the primary export of a module.
Files:
apps/web/test/utils/bookingScenario/getMockRequestDataForBooking.tspackages/features/bookings/lib/handleNewBooking/test/booking-validations.test.ts
🔇 Additional comments (1)
apps/web/test/utils/bookingScenario/getMockRequestDataForBooking.ts (1)
30-30: LGTM! Clean extension for guest verification testing.The optional
guestsfield appropriately extends the mock data type to support testing scenarios with multiple guest emails, aligning with the PR's Booker email verification feature.
| beforeEach(() => { | ||
| vi.clearAllMocks(); | ||
| }); | ||
|
|
There was a problem hiding this comment.
Do not clear repository mocks without restoring defaults
This inner beforeEach runs after the outer one, so calling vi.clearAllMocks() here wipes out the default mockResolvedValue([]/null) stubs set above. Every test in this block now gets undefined back from the repository methods, which breaks the booking code that expects arrays. Drop this clear or reapply the default stubs immediately after it; otherwise the verification tests will fail nondeterministically.
🤖 Prompt for AI Agents
In
packages/features/bookings/lib/handleNewBooking/test/booking-validations.test.ts
around lines 412 to 415, the inner beforeEach calling vi.clearAllMocks() wipes
the default repository mockResolvedValue stubs established in the outer
beforeEach and causes repository calls to return undefined; remove this
vi.clearAllMocks() or replace it with a non-destructive call (e.g.,
vi.resetHistory()) OR if you must clear mocks, immediately reapply the default
mockResolvedValue([]/null) stubs after clearing so repository methods continue
returning the expected arrays/nulls for the tests.
CarinaWolli
left a comment
CarinaWolli
left a comment
There was a problem hiding this comment.
Looks good and works as expected 👏🏻
E2E results are ready! |
emrysal
left a comment
emrysal
left a comment
There was a problem hiding this comment.
Looks good to me, minor issue is that emailVerified doesn't have an index on the secondary email table but I don't think that's a problem.
keithwillcode
deleted the
devin/booker-email-verification-setting-1759751444
branch
## What does this PR do? <!-- Please include a summary of the change and which issue is fixed. Please also include relevant motivation and context. List any dependencies that are required for this change. --> Removes and replaces the singular query that was still using the slow OR query from prisma. It was mainly used on the public booking page and triggered after someone entered their email (600ms debounce delay). ## Visual Demo (For contributors especially) Internal change only nothing changes on the UI or business logic. #### Video Demo (if applicable): N/A #### Image Demo (if applicable): N/A ## Mandatory Tasks (DO NOT REMOVE) - [x] I have self-reviewed the code (A decent size PR without self-review might be rejected). - [x] I have updated the developer docs in /docs if this PR makes changes that would require a [documentation change](https://cal.com/docs). If N/A, write N/A here and check the checkbox. - [x] I confirm automated tests are in place that prove my fix is effective or that my feature works. ## How should this be tested? <!-- Please describe the tests that you ran to verify your changes. Provide instructions so we can reproduce. Please also list any relevant details for your test configuration. Write details that help to start the tests --> Basically the same as #24298 since it touches relevant code. 1. Have a user that has the setting to prevent impersonation on. 2. Go to another users public booking page of any meeting. 3. Try to enter the email of the first user into the booking for. 4. Wait 1 second and make sure the button shows "Verify email". ## Checklist <!-- Remove bullet points below that don't apply to you --> - I haven't read the [contributing guide](https://github.com/calcom/cal.com/blob/main/CONTRIBUTING.md) - My code doesn't follow the style guidelines of this project - I haven't commented my code, particularly in hard-to-understand areas - I haven't checked if my changes generate no new warnings
7 participants
What does this PR do?
This PR converts the global
BLACKLISTED_GUEST_EMAILSenvironment variable into a user-specific setting that allows users to enable email verification for their event bookings. When enabled, anyone trying to book events using the user's email address must verify email ownership and be logged in to prevent impersonation.Key Changes:
requiresBookerEmailVerificationboolean field to User model with database migration/settings/my-account/generalBookerEmailBlocked,BookerEmailRequiresLogin) with proper error handlingRequested by: keith@cal.com
Taken over by: rodrigo@cal.com
Devin Session: https://app.devin.ai/sessions/5b3b084ccf034d35a488edc707adb59c
Visual Demo
Settings UI
The new toggle appears in General Settings:
Booking Flow
When enabled, users trying to book with the protected email will see email verification prompts (reuses existing verification system).
Mandatory Tasks
How should this be tested?
Environment Setup:
Test Steps:
Settings UI Test:
/settings/my-account/generalBooking Protection Test:
Guest Filtering Test:
Expected Behavior:
Human Reviewer Checklist
🚨 High Risk Areas - Please Review Carefully:
Booking Logic Changes (
packages/features/bookings/lib/handleNewBooking.ts):emailToRequiresVerificationMapHttpErrortoErrorWithCodeRepository Pattern Refactoring:
findByEmailWithEmailVerificationSettingandfindManyByEmailsWithEmailVerificationSettingsError Handling Pattern (
checkIfBookerEmailIsBlocked.ts):HttpErrortoErrorWithCodeDatabase Migration Safety:
requiresBookerEmailVerificationfield with proper default (false)Session & Middleware Updates: