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

Add Email Verification for non-Microsoft/Google Emails #9288

Merged
merged 63 commits into from
Jan 15, 2025

Conversation

samyakpiya
Copy link
Contributor

@samyakpiya samyakpiya commented Dec 31, 2024

Closes #8240

This PR introduces email verification for non-Microsoft/Google Emails:

Email Verification SignInUp Flow:

Sign.in.or.Create.an.account.1.mp4

The email verification link is sent as part of the SignInUpStep.EmailVerification. The email verification token validation is handled on a separate page (AppPath.VerifyEmail). A verification email resend can be triggered from both pages.

Email Verification Flow Screenshots (In Order):

image
image
image

Sent Email Details (Subject & Template):

Screenshot 2025-01-05 at 11 56 56 PM
image

Successful Email Verification Redirect:

image

Unsuccessful Email Verification (invalid token, invalid email, token expired, user does not exist, etc.):

image

Force Sign In When Email Not Verified:

image

TODOs:

Sign Up Process

  • Introduce server-level environment variable IS_EMAIL_VERIFICATION_REQUIRED (defaults to false)
  • Ensure users joining an existing workspace through an invite are not required to validate their email
  • Generate an email verification token
  • Store the token in appToken
  • Send email containing the verification link
    • Create new email template for email verification
  • Create a frontend page to handle verification requests

Sign In Process

  • After verifying user credentials, check if user's email is verified and prompt to to verify
  • Show an option to resend the verification email

Database

  • Rename the emailVerified colum on user to to isEmailVerified for consistency

During Deployment

  • Run a script/sql query to set isEmailVerified to true for all users with a Google/Microsoft email and all users that show an indication of a valid subscription (e.g. linked credit card)
  • I have created a draft migration file below that shows one possible approach to implementing this change:
import { MigrationInterface, QueryRunner } from 'typeorm';

export class UpdateEmailVerifiedForActiveUsers1733318043628
  implements MigrationInterface
{
  name = 'UpdateEmailVerifiedForActiveUsers1733318043628';

  public async up(queryRunner: QueryRunner): Promise<void> {
    await queryRunner.query(`
      CREATE TABLE core."user_email_verified_backup" AS
      SELECT id, email, "isEmailVerified"
      FROM core."user"
      WHERE "deletedAt" IS NULL;
    `);

    await queryRunner.query(`
      -- Update isEmailVerified for users who have been part of workspaces with active subscriptions
      UPDATE core."user" u
      SET "isEmailVerified" = true
      WHERE EXISTS (
        -- Check if user has been part of a workspace through userWorkspace table
        SELECT 1 
        FROM core."userWorkspace" uw
        JOIN core."workspace" w ON uw."workspaceId" = w.id
        WHERE uw."userId" = u.id
        -- Check for valid subscription indicators
        AND (
          w."activationStatus" = 'ACTIVE'
          -- Add any other subscription-related conditions here
        )
      )
      AND u."deletedAt" IS NULL;
  `);
  }

  public async down(queryRunner: QueryRunner): Promise<void> {
    await queryRunner.query(`
      UPDATE core."user" u
      SET "isEmailVerified" = b."isEmailVerified"
      FROM core."user_email_verified_backup" b
      WHERE u.id = b.id;
    `);

    await queryRunner.query(`DROP TABLE core."user_email_verified_backup";`);
  }
}

Copy link

github-actions bot commented Dec 31, 2024

Fails
🚫

node failed.

Log

�[31mError: �[39m SyntaxError: Unexpected token C in JSON at position 0
    at JSON.parse (<anonymous>)
�[90m    at parseJSONFromBytes (node:internal/deps/undici/undici:5584:19)�[39m
�[90m    at successSteps (node:internal/deps/undici/undici:5555:27)�[39m
�[90m    at fullyReadBody (node:internal/deps/undici/undici:1665:9)�[39m
�[90m    at process.processTicksAndRejections (node:internal/process/task_queues:95:5)�[39m
�[90m    at async specConsumeBody (node:internal/deps/undici/undici:5564:7)�[39m
danger-results://tmp/danger-results-cc3a5dfd.json

Generated by 🚫 dangerJS against 5f8d2b5

- Add token generation for email verification
- Store verification tokens in appToken
- Implement verification email sending
- Add verification status check post-login
- Display verification prompt for unverified users
- Handle email verification flow in auth process
…Service to prevent circular dependency issue
@samyakpiya samyakpiya marked this pull request as ready for review January 4, 2025 15:17
@samyakpiya
Copy link
Contributor Author

@FelixMalfait I spent way too long trying to figure out why the frontend couldn't make a mutation request without an auth token. Turns out there was middleware in place even though no auth guards were on the resolver. Probably should’ve just asked someone, lol.

Still need to rename the column to isEmailVerified, but other than that, it’s ready for review!

Copy link
Contributor

@greptile-apps greptile-apps bot left a comment

Choose a reason for hiding this comment

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

PR Summary

This PR implements a comprehensive email verification system for non-Microsoft/Google email signups, with configurable settings and secure token handling.

  • Added new SendEmailVerificationLinkEmail template with verification link and standardized footer component
  • Implemented secure token generation and verification flow in EmailVerificationService with proper expiration handling
  • Added frontend verification page and UI components for handling verification states and resending tokens
  • Created GraphQL mutations/queries for email verification with proper DTOs and exception handling
  • Added environment variables IS_EMAIL_VERIFICATION_REQUIRED and EMAIL_VERIFICATION_TOKEN_EXPIRES_IN for configuration

44 file(s) reviewed, 34 comment(s)
Edit PR Review Bot Settings | Greptile

- Addresses PR review feedback for conflict resolution
- Removes unnecessary null check since handleVerify already handles errors
- Simplifies code by directly destructuring verify response
Replace manual workspace existence check with workspaceValidator.assertIsDefinedOrThrow() for consistent validation handling in the auth resolver.
… email verification

This change allows users to validate their email for the specific workspace they're currently accessing, rather than defaulting to the first available workspace.
@samyakpiya
Copy link
Contributor Author

@FelixMalfait @AMoreaux I've made all the necessary changes based on feedback. Could you go through the unresolved conversations I've responded to and resolve them or let me know if further changes are needed? Thanks!

…ication

# Conflicts:
#	packages/twenty-front/src/generated/graphql.tsx
#	packages/twenty-front/src/modules/auth/hooks/useAuth.ts
#	packages/twenty-front/src/modules/client-config/components/ClientConfigProviderEffect.tsx
#	packages/twenty-front/src/modules/client-config/graphql/queries/getClientConfig.ts
#	packages/twenty-front/src/testing/mock-data/config.ts
#	packages/twenty-server/src/engine/core-modules/client-config/client-config.entity.ts
Introduced the `dedupeKey` property to prevent duplicate snack bar notifications. Modified related components and hooks to handle deduplication logic efficiently. This ensures a cleaner and more user-friendly notification experience.
Define `SnackBarOptions` type for the `newValue` parameter in `setSnackBarQueue`. This ensures better type safety and consistency when managing snack bar state.
@@ -29,6 +30,15 @@ export const useSnackBar = () => {
({ set }) =>
(newValue) =>
set(snackBarInternalScopedState({ scopeId }), (prev) => {
if (
isDefined(newValue.dedupeKey) &&
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah, this makes sense now! I didn't realize the issue was rooted at the snackbar level. I've been banging my head trying to implement workarounds: avoiding useEffect, steering clear of useRef for state, and adding guards. Thanks for catching this!

Copy link
Member

@FelixMalfait FelixMalfait Jan 15, 2025

Choose a reason for hiding this comment

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

Yes sorry I think I led you in the wrong direction. Initial loading is probably the only case where we'll keep useEffects ; in that case we want this to be done at a high level (page level), not in sub-components.
Solving the double notification at the snackbar level was probably the cleanest way to make it more idempotent in strict mode, even though it's not perfect either

AMoreaux and others added 7 commits January 15, 2025 15:37
…ication

# Conflicts:
#	packages/twenty-front/src/modules/auth/hooks/useAuth.ts
#	packages/twenty-server/src/engine/core-modules/auth/auth.resolver.ts
#	packages/twenty-server/src/engine/core-modules/auth/services/auth.service.spec.ts
#	packages/twenty-server/src/engine/core-modules/auth/services/auth.service.ts
#	packages/twenty-server/src/engine/core-modules/auth/services/sign-in-up.service.spec.ts
#	packages/twenty-server/src/engine/core-modules/auth/services/sign-in-up.service.ts
Removed unused `EmailVerificationService` and redundant `findAvailableWorkspacesByEmail` method from `auth.service`. Integrated `UserService` where necessary to optimize functionality and improve maintainability.
@AMoreaux AMoreaux merged commit f722a2d into twentyhq:main Jan 15, 2025
32 checks passed
@AMoreaux
Copy link
Contributor

Thanks @samyakpiya 🙏

It's merged 🎉

@samyakpiya
Copy link
Contributor Author

@AMoreaux Sweet, thank you! 😄

pacyL2K19 pushed a commit to pacyL2K19/twenty that referenced this pull request Jan 16, 2025
Closes twentyhq#8240

This PR introduces email verification for non-Microsoft/Google Emails:

https://github.com/user-attachments/assets/740e9714-5413-4fd8-b02e-ace728ea47ef

The email verification link is sent as part of the
`SignInUpStep.EmailVerification`. The email verification token
validation is handled on a separate page (`AppPath.VerifyEmail`). A
verification email resend can be triggered from both pages.

![image](https://github.com/user-attachments/assets/d52237dc-fcc6-4754-a40f-b7d6294eebad)

![image](https://github.com/user-attachments/assets/263a4b6b-db49-406b-9e43-6c0f90488bb8)

![image](https://github.com/user-attachments/assets/0343ae51-32ef-48b8-8167-a96deb7db99e)

![Screenshot 2025-01-05 at 11 56
56 PM](https://github.com/user-attachments/assets/475840d1-7d47-4792-b8c6-5c9ef5e02229)

![image](https://github.com/user-attachments/assets/a41b3b36-a36f-4a8e-b1f9-beeec7fe23e4)

![image](https://github.com/user-attachments/assets/e2fad9e2-f4b1-485e-8f4a-32163c2718e7)

expired, user does not exist, etc.):

![image](https://github.com/user-attachments/assets/92f4b65e-2971-4f26-a9fa-7aafadd2b305)

![image](https://github.com/user-attachments/assets/86d0f188-cded-49a6-bde9-9630fd18d71e)

- [x] Introduce server-level environment variable
IS_EMAIL_VERIFICATION_REQUIRED (defaults to false)
- [x] Ensure users joining an existing workspace through an invite are
not required to validate their email
- [x] Generate an email verification token
- [x] Store the token in appToken
- [x] Send email containing the verification link
  - [x] Create new email template for email verification
- [x] Create a frontend page to handle verification requests

- [x] After verifying user credentials, check if user's email is
verified and prompt to to verify
- [x] Show an option to resend the verification email

- [x] Rename the `emailVerified` colum on `user` to to `isEmailVerified`
for consistency

- [x] Run a script/sql query to set `isEmailVerified` to `true` for all
users with a Google/Microsoft email and all users that show an
indication of a valid subscription (e.g. linked credit card)
- I have created a draft migration file below that shows one possible
approach to implementing this change:

```typescript
import { MigrationInterface, QueryRunner } from 'typeorm';

export class UpdateEmailVerifiedForActiveUsers1733318043628
  implements MigrationInterface
{
  name = 'UpdateEmailVerifiedForActiveUsers1733318043628';

  public async up(queryRunner: QueryRunner): Promise<void> {
    await queryRunner.query(`
      CREATE TABLE core."user_email_verified_backup" AS
      SELECT id, email, "isEmailVerified"
      FROM core."user"
      WHERE "deletedAt" IS NULL;
    `);

    await queryRunner.query(`
      -- Update isEmailVerified for users who have been part of workspaces with active subscriptions
      UPDATE core."user" u
      SET "isEmailVerified" = true
      WHERE EXISTS (
        -- Check if user has been part of a workspace through userWorkspace table
        SELECT 1
        FROM core."userWorkspace" uw
        JOIN core."workspace" w ON uw."workspaceId" = w.id
        WHERE uw."userId" = u.id
        -- Check for valid subscription indicators
        AND (
          w."activationStatus" = 'ACTIVE'
          -- Add any other subscription-related conditions here
        )
      )
      AND u."deletedAt" IS NULL;
  `);
  }

  public async down(queryRunner: QueryRunner): Promise<void> {
    await queryRunner.query(`
      UPDATE core."user" u
      SET "isEmailVerified" = b."isEmailVerified"
      FROM core."user_email_verified_backup" b
      WHERE u.id = b.id;
    `);

    await queryRunner.query(`DROP TABLE core."user_email_verified_backup";`);
  }
}

```

---------

Co-authored-by: Antoine Moreaux <[email protected]>
Co-authored-by: Félix Malfait <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Email verification
5 participants