Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

✨ Enable changing user email address #1493

Merged
merged 8 commits into from
Jan 13, 2025
Merged

✨ Enable changing user email address #1493

merged 8 commits into from
Jan 13, 2025

Conversation

lukevella
Copy link
Owner

@lukevella lukevella commented Jan 13, 2025

Close #626

Summary by CodeRabbit

Release Notes

  • New Features

    • Added email change verification functionality.
    • Users can now request and verify email address changes.
    • Implemented a new component for email change requests.
    • Introduced a new component for updating user email addresses in profile settings.
    • Added a new section in profile settings for managing email addresses.
  • Localization Updates

    • Added new translation entries for the email change process.
    • Enhanced internationalization support for email verification messages.
  • User Experience Improvements

    • Added email change request tracking.
    • Implemented user feedback for email change status.
    • Improved profile settings for email management.
  • Security Enhancements

    • Added email verification token mechanism.
    • Implemented email change verification process with expiration.

Copy link

vercel bot commented Jan 13, 2025

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated (UTC)
app ✅ Ready (Inspect) Visit Preview 💬 Add feedback Jan 13, 2025 7:23pm
1 Skipped Deployment
Name Status Preview Comments Updated (UTC)
landing ⬜️ Skipped (Inspect) Jan 13, 2025 7:23pm

Copy link
Contributor

coderabbitai bot commented Jan 13, 2025

Walkthrough

This pull request introduces a comprehensive email change functionality for users. The implementation spans multiple files across the application, including configuration updates, localization files, API routes, TRPC routers, and email templates. The changes enable users to request an email change, receive a verification email, and complete the process by clicking a verification link, with robust error handling and user feedback mechanisms.

Changes

File Change Summary
apps/web/i18next-scanner.config.js Updated input configuration and removed keySeparator option
apps/web/public/locales/en/app.json Added localization keys for email change process
apps/web/src/app/[locale]/(admin)/settings/profile/profile-settings.tsx Removed email input field and updated name change logic
apps/web/src/app/api/trpc/[trpc]/route.ts Added email property to user context
apps/web/src/app/api/user/verify-email-change/route.ts New route handler for email change verification
apps/web/src/trpc/context.ts Added optional email property to TRPCContext
apps/web/src/trpc/routers/user.ts Replaced updatePreferences with requestEmailChange method
apps/web/src/trpc/trpc.ts Modified authentication logic to check for email
packages/emails/locales/en/emails.json Added localization keys for email change request email
packages/emails/src/previews/change-email-request.tsx Added email change request preview component
packages/emails/src/templates.ts Imported and added ChangeEmailRequest template
packages/emails/src/templates/change-email-request.tsx Added ChangeEmailRequest email template
apps/web/src/app/[locale]/(admin)/settings/profile/profile-email-address.tsx New component for updating user email address
apps/web/src/app/[locale]/(admin)/settings/profile/profile-page.tsx Added section for managing user email address

Assessment against linked issues

Objective Addressed Explanation
Allow users to change email
Verify new email address via link
e2e tests No e2e tests visible in this PR

Possibly related PRs

Poem

🐰 A rabbit's tale of email's new trail,
Changing addresses without travail,
Verification link, a digital seal,
Hopping through changes with zeal!
Email's journey, smooth and bright 🌟

Finishing Touches

  • 📝 Generate Docstrings (Beta)

Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media?

❤️ Share
🪧 Tips

Chat

There are 3 ways to chat with CodeRabbit:

  • Review comments: Directly reply to a review comment made by CodeRabbit. Example:
    • I pushed a fix in commit <commit_id>, please review it.
    • Generate unit testing code for this file.
    • Open a follow-up GitHub issue for this discussion.
  • Files and specific lines of code (under the "Files changed" tab): Tag @coderabbitai in a new review comment at the desired location with your query. Examples:
    • @coderabbitai generate unit testing code for this file.
    • @coderabbitai modularize this function.
  • PR comments: Tag @coderabbitai in a new PR comment to ask questions about the PR branch. For the best results, please provide a very specific query, as very limited context is provided in this mode. Examples:
    • @coderabbitai gather interesting stats about this repository and render them as a table. Additionally, render a pie chart showing the language distribution in the codebase.
    • @coderabbitai read src/utils.ts and generate unit testing code.
    • @coderabbitai read the files in the src/scheduler package and generate a class diagram using mermaid and a README in the markdown format.
    • @coderabbitai help me debug CodeRabbit configuration file.

Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments.

CodeRabbit Commands (Invoked using PR comments)

  • @coderabbitai pause to pause the reviews on a PR.
  • @coderabbitai resume to resume the paused reviews.
  • @coderabbitai review to trigger an incremental review. This is useful when automatic reviews are disabled for the repository.
  • @coderabbitai full review to do a full review from scratch and review all the files again.
  • @coderabbitai summary to regenerate the summary of the PR.
  • @coderabbitai generate docstrings to generate docstrings for this PR. (Beta)
  • @coderabbitai resolve resolve all the CodeRabbit review comments.
  • @coderabbitai configuration to show the current CodeRabbit configuration for the repository.
  • @coderabbitai help to get help.

Other keywords and placeholders

  • Add @coderabbitai ignore anywhere in the PR description to prevent this PR from being reviewed.
  • Add @coderabbitai summary to generate the high-level summary at a specific location in the PR description.
  • Add @coderabbitai anywhere in the PR title to generate the title automatically.

CodeRabbit Configuration File (.coderabbit.yaml)

  • You can programmatically configure CodeRabbit by adding a .coderabbit.yaml file to the root of your repository.
  • Please see the configuration documentation for more information.
  • If your editor has YAML language server enabled, you can add the path at the top of this file to enable auto-completion and validation: # yaml-language-server: $schema=https://coderabbit.ai/integrations/schema.v2.json

Documentation and Community

  • Visit our Documentation for detailed information on how to use CodeRabbit.
  • Join our Discord Community to get help, request features, and share feedback.
  • Follow us on X/Twitter for updates and announcements.

Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 4

🧹 Nitpick comments (11)
packages/emails/src/previews/change-email-request.tsx (1)

7-10: Consider using more realistic example data.

While the current example data is functional, using more realistic examples like [email protected] and a proper UUID for the token would make the preview more representative of production data.

-      fromEmail="[email protected]"
-      toEmail="[email protected]"
-      verificationUrl="https://rallly.co/verify-email-change?token=1234567890"
+      fromEmail="[email protected]"
+      toEmail="[email protected]"
+      verificationUrl="https://rallly.co/verify-email-change?token=123e4567-e89b-12d3-a456-426614174000"
packages/emails/src/templates.ts (1)

1-1: LGTM! Consider alphabetizing template entries.

The ChangeEmailRequest template is correctly imported and registered. For better maintainability, consider alphabetizing the entries in the templates object.

 const templates = {
+  ChangeEmailRequest,
   FinalizeHostEmail,
   FinalizeParticipantEmail,
   LoginEmail,
   NewCommentEmail,
   NewParticipantEmail,
   NewParticipantConfirmationEmail,
   NewPollEmail,
   RegisterEmail,
-  ChangeEmailRequest,
 };

Also applies to: 21-21

apps/web/src/app/api/trpc/[trpc]/route.ts (1)

32-32: Consider explicit null handling for email.

While setting undefined as a fallback works, consider being more explicit about null handling since session.user.email can be null. This would make the type system more accurate and the code's intent clearer.

-          email: session.user.email ?? undefined,
+          email: session.user.email === null ? undefined : session.user.email,
packages/emails/src/templates/change-email-request.tsx (2)

61-67: Make expiration time configurable and consistent.

The hardcoded "10 minutes" expiration message should be configurable and consistent with the actual token expiration time.

+ interface ChangeEmailRequestProps {
+  ctx: EmailContext;
+  verificationUrl: string;
+  fromEmail: string;
+  toEmail: string;
+  expirationMinutes: number;
+ }

  <Text light>
    {ctx.t("changeEmailRequest_text3", {
      ns: "emails",
+     expirationMinutes,
      defaultValue:
-       "This link will expire in 10 minutes. If you did not request this change, please contact support.",
+       "This link will expire in {{expirationMinutes}} minutes. If you did not request this change, please contact support.",
    })}
  </Text>

15-70: Add plain text fallback for email clients.

Consider adding a plain text version of the email for better compatibility with email clients that don't support HTML.

apps/web/src/trpc/trpc.ts (1)

67-82: Enhance type safety and error messaging.

The current implementation could be improved for better type safety and user experience.

 const email = ctx.user.email;
 if (!email) {
   throw new TRPCError({
     code: "UNAUTHORIZED",
-    message: "Login is required",
+    message: "Email verification is required to perform this action",
   });
 }

+// Narrow the type to non-null email
+const user: typeof ctx.user & { email: string } = {
+  ...ctx.user,
+  email,
+};

 return next({
   ctx: {
-    user: {
-      ...ctx.user,
-      email,
-    },
+    user,
   },
 });
apps/web/src/app/[locale]/(admin)/settings/profile/profile-settings.tsx (3)

44-80: Add type validation for error messages.

The error handling is comprehensive, but it would be safer to validate the error type against known values.

Consider this improvement:

   if (error) {
+    const isKnownError = error === "invalidToken";
     posthog.capture("email change failed", { error });
     toast({
       variant: "destructive",
       title: t("emailChangeFailed", {
         defaultValue: "Email change failed",
       }),
       description:
-        error === "invalidToken"
+        isKnownError
           ? t("emailChangeInvalidToken", {
               defaultValue:
                 "The verification link is invalid or has expired. Please try again.",
             })
           : t("emailChangeError", {
               defaultValue: "An error occurred while changing your email",
             }),
     });
     Cookies.remove("email-change-error");
   }

88-92: Add client-side email validation.

While the server validates the email, adding client-side validation would provide immediate feedback to users.

Consider adding email validation:

   if (data.email !== user.email) {
+    const emailRegex = /^[^\s@]+@[^\s@]+\.[^\s@]+$/;
+    if (!emailRegex.test(data.email)) {
+      toast({
+        variant: "destructive",
+        title: t("invalidEmail", {
+          defaultValue: "Invalid email address",
+        }),
+      });
+      return;
+    }
     posthog.capture("email change requested");
     await requestEmailChange.mutateAsync({ email: data.email });
     setDidRequestEmailChange(true);
   }

128-143: Add loading state during email verification.

The alert provides good feedback, but users should also see a loading state while the verification is being processed.

Consider adding a loading state:

   {didRequestEmailChange ? (
-    <Alert icon={InfoIcon}>
+    <Alert icon={requestEmailChange.isLoading ? Loader : InfoIcon}>
       <AlertTitle>
         <Trans
           i18nKey="emailChangeRequestSent"
           defaults="Verify your new email address"
         />
       </AlertTitle>
       <AlertDescription>
         <Trans
           i18nKey="emailChangeRequestSentDescription"
           defaults="To complete the change, please check your email for a verification link."
         />
+        {requestEmailChange.isLoading && (
+          <p className="mt-2 text-sm text-muted-foreground">
+            {t("sendingVerification", "Sending verification email...")}
+          </p>
+        )}
       </AlertDescription>
     </Alert>
   ) : null}
packages/emails/locales/en/emails.json (1)

50-56: Add security-related information to email templates.

Consider adding translations for additional security notices in the verification email.

Add these translations:

   "changeEmailRequest_preview": "Please verify your email address",
   "changeEmailRequest_heading": "Verify Your New Email Address",
   "changeEmailRequest_text1": "We've received a request to change the email address for your account.",
   "changeEmailRequest_text2": "To complete this change, please click the button below:",
   "changeEmailRequest_button": "Verify Email Address",
   "changeEmailRequest_subject": "Verify your new email address",
-  "changeEmailRequest_text3": "This link will expire in 10 minutes. If you did not request this change, please contact support."
+  "changeEmailRequest_text3": "This link will expire in 10 minutes. If you did not request this change, please contact support immediately as your account may be compromised.",
+  "changeEmailRequest_security": "For security, this request was made from: IP: {ipAddress}, Browser: {userAgent}",
+  "changeEmailRequest_warning": "Never share this verification link with anyone"
apps/web/public/locales/en/app.json (1)

284-290: Add more specific error message translations.

Consider adding translations for specific error cases to provide clearer feedback to users.

Add these translations:

   "emailChangeSuccess": "Email changed successfully",
   "emailChangeSuccessDescription": "Your email has been updated",
   "emailChangeFailed": "Email change failed",
   "emailChangeInvalidToken": "The verification link is invalid or has expired. Please try again.",
   "emailChangeError": "An error occurred while changing your email",
+  "emailChangeAlreadyInUse": "This email address is already in use",
+  "emailChangeTooManyRequests": "Too many requests. Please wait before trying again",
+  "emailChangeNoMatch": "New email must be different from current email",
   "emailChangeRequestSent": "Verify your new email address",
   "emailChangeRequestSentDescription": "To complete the change, please check your email for a verification link."
📜 Review details

Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 31dc85b and 44b7161.

📒 Files selected for processing (12)
  • apps/web/i18next-scanner.config.js (1 hunks)
  • apps/web/public/locales/en/app.json (1 hunks)
  • apps/web/src/app/[locale]/(admin)/settings/profile/profile-settings.tsx (5 hunks)
  • apps/web/src/app/api/trpc/[trpc]/route.ts (1 hunks)
  • apps/web/src/app/api/user/verify-email-change/route.ts (1 hunks)
  • apps/web/src/trpc/context.ts (1 hunks)
  • apps/web/src/trpc/routers/user.ts (2 hunks)
  • apps/web/src/trpc/trpc.ts (1 hunks)
  • packages/emails/locales/en/emails.json (1 hunks)
  • packages/emails/src/previews/change-email-request.tsx (1 hunks)
  • packages/emails/src/templates.ts (2 hunks)
  • packages/emails/src/templates/change-email-request.tsx (1 hunks)
🔇 Additional comments (3)
apps/web/src/trpc/context.ts (1)

10-10: LGTM! Verify email property usage in authentication flow.

The optional email property is correctly typed and aligns with the authentication requirements.

Let's verify the email property usage in the authentication flow:

✅ Verification successful

✓ Email property usage verified and correctly implemented

The optional email property in the context is properly used throughout the authentication flow:

  • Used for guest vs authenticated user determination
  • Validated in protected TRPC routes
  • Properly handled in auth state management
🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Search for places where the email property is used in authentication
ast-grep --pattern 'user.$email' 

Length of output: 34


Script:

#!/bin/bash
# Search for email-related patterns in authentication context
echo "=== Direct email property access ==="
rg -i "\.email" --type ts --type js -A 2

echo -e "\n=== Destructured email usage ==="
ast-grep --pattern 'const { email } = $user'

echo -e "\n=== Email in type definitions ==="
ast-grep --pattern 'email: string'

Length of output: 19732

apps/web/i18next-scanner.config.js (1)

4-4: Verify the auth file path change and translation key impacts.

The exclusion path has been updated, and the keySeparator option has been removed. These changes could affect existing translations.

Let's verify the changes:

✅ Verification successful

Auth file move and translation changes are safe

  • The auth.ts file has been cleanly moved from utils/ to src/
  • Translation configuration changes are safe as all translations use a flat structure in app.json files
🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Check if auth.ts exists in both old and new locations
echo "Checking auth file locations:"
fd auth.ts src/

# Check for any translation keys using dots that might be affected by keySeparator removal
echo "Checking for potentially affected translation keys:"
fd -e json . public/locales/ -x jq 'paths | select(.[]) | join(".")' {} \; | grep "\."

Length of output: 486


Script:

#!/bin/bash
# First, let's understand the repository structure
echo "Repository structure:"
ls -la

# Find all auth-related files
echo -e "\nAuth-related files:"
fd -t f auth

# Find translation config and locale files
echo -e "\nTranslation-related files:"
fd -t f -e json locale
fd -t f i18n

Length of output: 2511


Script:

#!/bin/bash
# Check if old auth location still exists
echo "Checking old auth location:"
fd -t f . apps/web/src/utils/

# Find and check translation files in web app
echo -e "\nChecking translation files and patterns:"
fd -t f -e json . apps/web/public/locales/ || true
fd -t f -e json . apps/web/src/locales/ || true

# Check i18next scanner config content
echo -e "\nChecking i18next scanner config:"
cat apps/web/i18next-scanner.config.js

Length of output: 2919

apps/web/src/app/[locale]/(admin)/settings/profile/profile-settings.tsx (1)

1-1: LGTM! Appropriate hooks and imports for email change functionality.

The added imports and hooks provide the necessary functionality for:

  • Analytics tracking with PostHog
  • Toast notifications for user feedback
  • Cookie management for handling verification states
  • Email change mutation via TRPC

Also applies to: 11-11, 13-14, 27-28, 38-42

apps/web/src/app/api/user/verify-email-change/route.ts Outdated Show resolved Hide resolved
apps/web/src/app/api/user/verify-email-change/route.ts Outdated Show resolved Hide resolved
apps/web/src/app/api/user/verify-email-change/route.ts Outdated Show resolved Hide resolved
apps/web/src/trpc/routers/user.ts Outdated Show resolved Hide resolved
Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 2

🧹 Nitpick comments (2)
apps/web/src/app/[locale]/(admin)/settings/profile/profile-settings.tsx (2)

Line range hint 1-42: LGTM! Consider adding type safety for form data.

The component setup and imports look good. For better type safety, consider defining an interface for the form data.

interface ProfileFormData {
  name: string;
  email: string;
}

const form = useForm<ProfileFormData>({
  defaultValues: {
    name: user.isGuest ? "" : user.name,
    email: user.email ?? "",
  },
});

128-143: Enhance alert accessibility.

The alert implementation looks good, but consider adding ARIA attributes for better screen reader support.

-  <Alert icon={InfoIcon}>
+  <Alert 
+    icon={InfoIcon}
+    role="alert"
+    aria-live="polite"
+  >
📜 Review details

Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 44b7161 and 0e24119.

📒 Files selected for processing (3)
  • apps/web/src/app/[locale]/(admin)/settings/profile/profile-settings.tsx (5 hunks)
  • apps/web/src/app/api/user/verify-email-change/route.ts (1 hunks)
  • packages/emails/locales/en/emails.json (1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
  • apps/web/src/app/api/user/verify-email-change/route.ts
  • packages/emails/locales/en/emails.json
🔇 Additional comments (1)
apps/web/src/app/[locale]/(admin)/settings/profile/profile-settings.tsx (1)

123-123: Verify email validation implementation.

Ensure proper email validation is implemented to prevent invalid email submissions.

✅ Verification successful

Email validation is properly implemented

The implementation includes:

  • Client-side validation using Zod schema
  • Server-side validation in the tRPC router
  • Proper error handling and user feedback
🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Search for email validation in the codebase
echo "Searching for email validation..."
rg -A 5 'requestEmailChange' --type typescript

# Check for validation schema definitions
echo "Checking validation schemas..."
rg -A 10 'z.string\(\).email\(\)' --type typescript

Length of output: 318


Script:

#!/bin/bash
# Search for email validation patterns
echo "Searching for email validation..."
rg -A 5 'email.*validation|validation.*email|z\.string\(\)\.email|validateEmail'

# Search for form handling related to email
echo "Searching for email-related form handling..."
rg -A 10 'handleSubmit.*email|onSubmit.*email|requestEmailChange'

# Search for email input validation
echo "Looking for email input validation..."
rg -A 5 'type=["'\'']email["'\'']|<Input.*email'

Length of output: 8521

Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1

♻️ Duplicate comments (1)
apps/web/src/app/api/user/verify-email-change/route.ts (1)

60-70: ⚠️ Potential issue

Add rate limiting to prevent abuse.

The email verification endpoint should be rate limited to prevent abuse.

Consider implementing rate limiting middleware similar to the TRPC endpoints.

🧹 Nitpick comments (1)
apps/web/src/app/[locale]/(admin)/settings/profile/profile-email-address.tsx (1)

84-105: Optimize form reset timing.

The form is reset before the async operation completes, which could lead to a confusing UX if the operation fails.

Move the reset after successful completion:

 onSubmit={handleSubmit(async (data) => {
-  reset(data);
   if (data.email !== user.email) {
     posthog.capture("email change requested");
     const res = await requestEmailChange.mutateAsync({
       email: data.email,
     });
     if (res.success === false) {
       if (res.reason === "emailAlreadyInUse") {
         form.setError("email", {
           message: t("emailAlreadyInUse"),
         });
       }
     } else {
+      reset(data);
       setDidRequestEmailChange(true);
     }
   }
   await refresh();
 })}
📜 Review details

Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 0e24119 and 3b2edc5.

📒 Files selected for processing (6)
  • apps/web/public/locales/en/app.json (1 hunks)
  • apps/web/src/app/[locale]/(admin)/settings/profile/profile-email-address.tsx (1 hunks)
  • apps/web/src/app/[locale]/(admin)/settings/profile/profile-page.tsx (2 hunks)
  • apps/web/src/app/[locale]/(admin)/settings/profile/profile-settings.tsx (3 hunks)
  • apps/web/src/app/api/user/verify-email-change/route.ts (1 hunks)
  • apps/web/src/trpc/routers/user.ts (2 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
  • apps/web/src/app/[locale]/(admin)/settings/profile/profile-settings.tsx
  • apps/web/public/locales/en/app.json
🔇 Additional comments (4)
apps/web/src/app/[locale]/(admin)/settings/profile/profile-page.tsx (1)

83-95: LGTM! Well-structured settings section for email management.

The new email settings section is properly integrated with clear i18n support and logical placement in the settings hierarchy.

apps/web/src/app/api/user/verify-email-change/route.ts (2)

13-18: ⚠️ Potential issue

Strengthen cookie security configuration.

The current cookie configuration has security vulnerabilities:

  • httpOnly: false allows JavaScript access
  • secure: false allows transmission over HTTP
  • Missing SameSite attribute

Apply this security enhancement:

 const COOKIE_CONFIG = {
   path: "/",
-  httpOnly: false,
-  secure: false,
+  httpOnly: true,
+  secure: process.env.NODE_ENV === "production",
+  sameSite: "lax",
   expires: new Date(Date.now() + 5 * 1000), // 5 seconds
 } as const;

Likely invalid or redundant comment.


27-58: ⚠️ Potential issue

Add token expiration check.

The function doesn't verify if the token has expired before processing it.

Add expiration check:

 const handleEmailChange = async (token: string) => {
   const verificationToken = await prisma.verificationToken.findUnique({
     where: { token },
   });

   if (!verificationToken) {
     setEmailChangeCookie("error", "invalidToken");
     return false;
   }

+  if (new Date() > verificationToken.expires) {
+    setEmailChangeCookie("error", "tokenExpired");
+    return false;
+  }

   const payload = await decryptToken<EmailChangePayload>(token);

Likely invalid or redundant comment.

apps/web/src/trpc/routers/user.ts (1)

99-140: LGTM! Robust implementation with proper security measures.

The implementation includes essential security features:

  • Rate limiting middleware
  • Email existence check
  • Proper token generation and expiration
  • Secure email delivery

@vercel vercel bot temporarily deployed to Preview – landing January 13, 2025 18:56 Inactive
@vercel vercel bot temporarily deployed to Preview – landing January 13, 2025 19:20 Inactive
Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 0

♻️ Duplicate comments (2)
apps/web/src/app/[locale]/(admin)/settings/profile/profile-email-address.tsx (1)

43-77: 🛠️ Refactor suggestion

Add cookie cleanup after processing.

The cookies should be removed after processing to prevent stale states:

 React.useEffect(() => {
   const success = Cookies.get("email-change-success");
   const error = Cookies.get("email-change-error");

   if (success) {
     posthog.capture("email change completed");
     toast({
       title: t("emailChangeSuccess"),
       description: t("emailChangeSuccessDescription"),
     });
+    Cookies.remove("email-change-success", { path: "/" });
   }

   if (error) {
     posthog.capture("email change failed", { error });
     toast({
       variant: "destructive",
       title: t("emailChangeFailed"),
       description: error === "invalidToken"
         ? t("emailChangeInvalidToken")
         : t("emailChangeError"),
     });
+    Cookies.remove("email-change-error", { path: "/" });
   }
 }, [posthog, refresh, t, toast]);
apps/web/src/trpc/routers/user.ts (1)

125-164: 🛠️ Refactor suggestion

Enhance security measures for email change.

While the implementation is solid with rate limiting and email uniqueness check, consider these additional security measures:

   requestEmailChange: privateProcedure
     .use(rateLimitMiddleware)
     .input(z.object({ email: z.string().email() }))
     .mutation(async ({ input, ctx }) => {
+      // Check for recent email change requests
+      const recentToken = await prisma.verificationToken.findFirst({
+        where: {
+          identifier: ctx.user.email,
+          createdAt: { gt: new Date(Date.now() - 5 * 60 * 1000) },
+        },
+      });
+      
+      if (recentToken) {
+        throw new TRPCError({
+          code: "TOO_MANY_REQUESTS",
+          message: "Please wait 5 minutes before requesting another email change",
+        });
+      }

       // check if the email is already in use
       const existingUser = await prisma.user.count({
         where: { email: input.email },
       });

       if (existingUser) {
         return {
           success: false as const,
           reason: "emailAlreadyInUse" as const,
         };
       }

       // create a verification token
       const token = await createToken(
         {
           fromEmail: ctx.user.email,
           toEmail: input.email,
         },
         {
           ttl: 60 * 10,
         },
       );

+      // Store token in database for validation
+      await prisma.verificationToken.create({
+        data: {
+          identifier: ctx.user.email,
+          token,
+          expires: new Date(Date.now() + 1000 * 60 * 10),
+          createdAt: new Date(),
+        },
+      });

       ctx.user.getEmailClient().sendTemplate("ChangeEmailRequest", {
         to: input.email,
         props: {
           verificationUrl: absoluteUrl(
             `/api/user/verify-email-change?token=${token}`,
           ),
           fromEmail: ctx.user.email,
           toEmail: input.email,
         },
       });

       return { success: true as const };
     }),
🧹 Nitpick comments (2)
apps/web/src/trpc/trpc.ts (1)

67-82: Consider standardizing authentication checks across procedures.

The authentication logic now differs between procedures:

  • privateProcedure: Uses email presence
  • proProcedure and possiblyPublicProcedure: Use isGuest

This inconsistency might lead to confusion. Consider standardizing the authentication approach across all procedures or documenting the rationale for the different checks.

apps/web/src/app/[locale]/(admin)/settings/profile/profile-email-address.tsx (1)

84-105: Add debouncing to form submission.

Consider adding debouncing to prevent rapid-fire form submissions:

+import { useDebouncedCallback } from "use-debounce";
+
 // ... in component
+const debouncedSubmit = useDebouncedCallback(
+  async (data: { email: string }) => {
     reset(data);
     if (data.email !== user.email) {
       posthog.capture("email change requested");
       const res = await requestEmailChange.mutateAsync({
         email: data.email,
       });
       // ... rest of the logic
     }
     await refresh();
+  },
+  1000
+);
📜 Review details

Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 3b2edc5 and a2d12a0.

📒 Files selected for processing (14)
  • apps/web/i18next-scanner.config.js (1 hunks)
  • apps/web/public/locales/en/app.json (1 hunks)
  • apps/web/src/app/[locale]/(admin)/settings/profile/profile-email-address.tsx (1 hunks)
  • apps/web/src/app/[locale]/(admin)/settings/profile/profile-page.tsx (2 hunks)
  • apps/web/src/app/[locale]/(admin)/settings/profile/profile-settings.tsx (3 hunks)
  • apps/web/src/app/api/trpc/[trpc]/route.ts (1 hunks)
  • apps/web/src/app/api/user/verify-email-change/route.ts (1 hunks)
  • apps/web/src/trpc/context.ts (1 hunks)
  • apps/web/src/trpc/routers/user.ts (2 hunks)
  • apps/web/src/trpc/trpc.ts (1 hunks)
  • packages/emails/locales/en/emails.json (1 hunks)
  • packages/emails/src/previews/change-email-request.tsx (1 hunks)
  • packages/emails/src/templates.ts (2 hunks)
  • packages/emails/src/templates/change-email-request.tsx (1 hunks)
🚧 Files skipped from review as they are similar to previous changes (9)
  • apps/web/src/trpc/context.ts
  • packages/emails/src/templates.ts
  • apps/web/i18next-scanner.config.js
  • apps/web/src/app/api/trpc/[trpc]/route.ts
  • apps/web/src/app/[locale]/(admin)/settings/profile/profile-page.tsx
  • apps/web/src/app/[locale]/(admin)/settings/profile/profile-settings.tsx
  • packages/emails/src/previews/change-email-request.tsx
  • apps/web/public/locales/en/app.json
  • packages/emails/locales/en/emails.json
⏰ Context from checks skipped due to timeout of 90000ms (4)
  • GitHub Check: Type check
  • GitHub Check: Integration tests
  • GitHub Check: Linting
  • GitHub Check: Unit tests
🔇 Additional comments (6)
apps/web/src/trpc/trpc.ts (2)

75-82: Ensure type safety for the enriched context.

The middleware adds the email property to the context. Verify that:

  1. The TRPCContext type properly includes this email property
  2. Downstream handlers expecting this property are type-safe

Let's verify the type definitions and usage:

✅ Verification successful

Type safety verification successful

The email property is properly typed in TRPCContext, and its usage is protected by both compile-time type checking and runtime validation in the middleware.

🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Search for TRPCContext type definition and its usage
ast-grep --pattern 'type TRPCContext = {
  $$$
}'

# Search for downstream handlers using the email property
rg -A 5 "ctx\.user\.email" --type ts

Length of output: 1634


67-74: Verify authentication requirements for email-based checks.

The change from isGuest to email presence validation might allow guest users with an email set. Consider if this aligns with the authentication requirements.

Let's verify the authentication flow:

✅ Verification successful

The email-based check is consistent with the authentication model.

The codebase consistently defines guest status based on email presence (isGuest = !email). The change from isGuest to email validation in privateProcedure maintains this invariant and is safe.

🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Search for guest user handling and email validation patterns
rg -A 5 "isGuest|email.*validation" --type ts

Length of output: 17567

apps/web/src/app/api/user/verify-email-change/route.ts (3)

46-64: ⚠️ Potential issue

Add rate limiting and improve error handling.

The endpoint needs protection against abuse and should provide less verbose error messages:

+import { rateLimit } from "@/utils/rate-limit";
+
+const limiter = rateLimit({
+  interval: 60 * 1000, // 1 minute
+  uniqueTokenPerInterval: 500,
+});
+
 export const GET = async (request: NextRequest) => {
+  try {
+    await limiter.check(request, 3); // 3 requests per minute
+  } catch {
+    return NextResponse.json(
+      { error: "Too many requests" },
+      { status: 429 }
+    );
+  }
+
   const token = request.nextUrl.searchParams.get("token");

   if (!token) {
-    return NextResponse.json({ error: "No token provided" }, { status: 400 });
+    return NextResponse.json({ error: "Invalid request" }, { status: 400 });
   }

Likely invalid or redundant comment.


14-19: ⚠️ Potential issue

Strengthen cookie security configuration.

The current cookie configuration needs security improvements:

 const COOKIE_CONFIG = {
   path: "/",
-  httpOnly: false,
-  secure: false,
+  httpOnly: true,
+  secure: process.env.NODE_ENV === "production",
   expires: new Date(Date.now() + 5 * 1000), // 5 seconds
+  sameSite: "lax",
 } as const;

Likely invalid or redundant comment.


28-44: ⚠️ Potential issue

Enhance token verification and database operations.

The current implementation needs improvements in token handling and database operations:

 const handleEmailChange = async (token: string) => {
+  const verificationToken = await prisma.verificationToken.findUnique({
+    where: { token },
+  });
+
+  if (!verificationToken) {
+    setEmailChangeCookie("error", "invalidToken");
+    return false;
+  }
+
+  if (new Date() > verificationToken.expires) {
+    setEmailChangeCookie("error", "tokenExpired");
+    return false;
+  }
+
   const payload = await decryptToken<EmailChangePayload>(token);

   if (!payload) {
     setEmailChangeCookie("error", "invalidToken");
     return false;
   }

-  await prisma.user.update({
-    where: { email: payload.fromEmail },
-    data: { email: payload.toEmail },
-  });
+  await prisma.$transaction(async (tx) => {
+    await tx.user.update({
+      where: { 
+        email: payload.fromEmail,
+        AND: { NOT: { email: payload.toEmail } }
+      },
+      data: { email: payload.toEmail },
+    });
+
+    await tx.verificationToken.delete({
+      where: { token },
+    });
+  });

   setEmailChangeCookie("success");
   return true;
 };

Likely invalid or redundant comment.

packages/emails/src/templates/change-email-request.tsx (1)

1-82: LGTM! Well-structured email template with proper i18n support.

The implementation demonstrates good practices:

  • Clear separation of concerns
  • Proper internationalization
  • Informative user communication

@lukevella lukevella merged commit 8c77047 into main Jan 13, 2025
9 checks passed
@lukevella lukevella deleted the change-email branch January 13, 2025 19:32
@coderabbitai coderabbitai bot mentioned this pull request Jan 15, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Allow users to change email
1 participant