twenty icon indicating copy to clipboard operation
twenty copied to clipboard

Change password from settings

Open Bonapara opened this issue 1 year ago • 7 comments

Expected behavior

1. Go to profile settings

In the Profile settings, we would like to add a new section that allows users to reset their password:

image https://www.figma.com/file/xt8O9mFeLl46C5InWwoMrN/Twenty?type=design&node-id=13273-96299&mode=design&t=tY8CUvxiUNT2VRnR-11

2. Receive an email

Selecting Change password will trigger an email to the user with a reset link:

Email body style & content:

image https://www.figma.com/file/xt8O9mFeLl46C5InWwoMrN/Twenty?type=design&node-id=16594-92960&mode=design&t=tY8CUvxiUNT2VRnR-11

3. Change password

When the user clicks on the email link, they should be redirected to a page at app.twenty.com/reset-password. This page will prompt them to enter a new password:

image https://www.figma.com/file/xt8O9mFeLl46C5InWwoMrN/Twenty?type=design&node-id=19100-70339&mode=design&t=tY8CUvxiUNT2VRnR-11

Bonapara avatar Jan 05 '24 08:01 Bonapara

Hi @Bonapara, I'm interested in working on this.

Please have a review of the below engineering notes in order to confirm the development pieces, just to be on same page.

  1. Add passwordResetToken and passwordResetTokenExpires column on user entity. The first is a string while second is date.
  2. Add two mutations in auth resolver - first for generating password link and second for updating the password - https://github.com/twentyhq/twenty/blob/main/packages/twenty-server/src/core/auth/auth.resolver.ts
  3. Add a query here as well to validate the reset password link.
  4. In first mutation, prepare a reset password token using crypto. Store the token by hashing it in DB at passwordResetToken. Also, update passwordResetTokenExpires with current date time plus five minutes. Prefix the token with reset-password and frontend URL as well - app.twenty.com/reset-password/{xxxx} to make it reset link. Prepare the HTML markup and send an email to the user using "EmailService" 's send. Display an alert on UI in return of successful mutation.
  5. Register a route with reset-password on frontend here - https://github.com/twentyhq/twenty/blob/main/packages/twenty-front/src/App.tsx
  6. On hitting this URL, logout the user if already logged in.
  7. Now, make a GQL request to validate query. In the query, search the user table with provided token against passwordResetToken and time should be smaller than current time against passwordResetTokenExpires. Reject it if not found.
  8. On failure, redirect to sign-in page.
  9. On success, show the password reset dialog box.
  10. On submission of new password, call the update Password mutation and update it. Finally, redirect user to sign-in page.

For sending HTML mail, are we using any templating engine? If not, I would like to propose the use of any template like nodemailer handlebars or MJML template or any other so that we are consistent throughout.

i-am-chitti avatar Jan 06 '24 19:01 i-am-chitti

Thanks a lot @i-am-chitti - that's a lot of work something labeled as a "good first issue" 😅 I agree with the steps you've given. A few comments:

  • You can add an env variable for the password expiration delay
  • Name column passwordResetTokenExpiresAt to be consistent with other date columns
  • For sending email @martmull recently introduced EmailService which relies on node mailder but I don't think it's used anywhere yet
  • I agree we should use a templating engine. "nodemailer handlebar" seems a bit risky to go with because it isn't very popular. MJML looks like a good option. Although I think my preference goes to React Email because it's more aligned with what's already done with the code base, it's simple / modern and React-like. Check example template here or here without Tailwind. Do you see any issue with it compared to MJML?
  • "On hitting this URL, logout the user if already logged in." => I wonder if this is absolutely necessary (but no big deal if it is)
  • "Finally, redirect user to sign-in page." Would be nice to sign-in the user automatically at the ended of the reset token return also return an auth token and redirect to the app
  • Add button to settings page

cc @charlesBochet fyi

FelixMalfait avatar Jan 06 '24 21:01 FelixMalfait

Thanks @FelixMalfait. I've started working on this. Will raise PR once ready.

i-am-chitti avatar Jan 08 '24 15:01 i-am-chitti

@i-am-chitti thanks! FYI this was a required step to publish our Zapier app in their store so your work will also unlock this :)

FelixMalfait avatar Jan 09 '24 07:01 FelixMalfait

Hello @i-am-chitti, have you had the chance to work on email templating yet? I'll be needing email templates for a different issue soon. If you haven't started on this, I'm planning to open a pull request to implement email templating, making it available for both of us to use.

martmull avatar Jan 09 '24 09:01 martmull

No, I haven't yet started on email templating. I planned it at last as it's a styling part. Currently only testing with bare minimum. You can start on this. Once I develop the requirements, will integrate templates which you would develop.

On Tue, 9 Jan 2024, 15:25 martmull, @.***> wrote:

Hello @i-am-chitti https://github.com/i-am-chitti, have you had the chance to work on email templating yet? I'll be needing email templates for a different issue soon. If you haven't started on this, I'm planning to open a pull request to implement email templating, making it available for both of us to use.

— Reply to this email directly, view it on GitHub https://github.com/twentyhq/twenty/issues/3245#issuecomment-1882750413, or unsubscribe https://github.com/notifications/unsubscribe-auth/AOK2TGWRJZMOXBQSZJWADTTYNUHZVAVCNFSM6AAAAABBOB3I22VHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMYTQOBSG42TANBRGM . You are receiving this because you were mentioned.Message ID: @.***>

i-am-chitti avatar Jan 09 '24 10:01 i-am-chitti

No, I haven't yet started on email templating. I planned it at last as it's a styling part. Currently only testing with bare minimum. You can start on this. Once I develop the requirements, will integrate templates which you would develop. On Tue, 9 Jan 2024, 15:25 martmull, @.> wrote: Hello @i-am-chitti https://github.com/i-am-chitti, have you had the chance to work on email templating yet? I'll be needing email templates for a different issue soon. If you haven't started on this, I'm planning to open a pull request to implement email templating, making it available for both of us to use. — Reply to this email directly, view it on GitHub <#3245 (comment)>, or unsubscribe https://github.com/notifications/unsubscribe-auth/AOK2TGWRJZMOXBQSZJWADTTYNUHZVAVCNFSM6AAAAABBOB3I22VHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMYTQOBSG42TANBRGM . You are receiving this because you were mentioned.Message ID: @.>

Perfect

martmull avatar Jan 09 '24 10:01 martmull

Hey @FelixMalfait,

Backend dev has completed. Now setting up frontend. I've hit one blocker though -

I've added a mutation file under twenty-front/src/modules/auth/graphql/mutations/generatePasswordResetToken.ts with content

import { gql } from '@apollo/client';

export const GENERATE_PASSWORD_RESET_TOKEN = gql`
  mutation GeneratePasswordResetToken {
    generatePasswordResetToken() {
      passwordResetToken
      passwordResetTokenExpiresAt
    }
  }
`;

Now, executing command to generate the Graphql data with yarn nx graphql:data:generate twenty-front doesn't add the useGeneratePasswordResetTokenMutation in the file src/generate/graphql.tsx file. I've already added generatePasswordResetToken mutation on backend side. It's showing up correctly and working in the Graphql Explorer docs - image

So, mutation resolver has been added. Any idea why it may not be present in generated Graphql on frontend?

i-am-chitti avatar Jan 11 '24 17:01 i-am-chitti

Sorry I'm not sure. You seem to be doing it properly 🤔 .env is pointing to the right backend? The command works for us

FelixMalfait avatar Jan 11 '24 17:01 FelixMalfait

.env is pointing to the right backend?

Yep, frontend application works at localhost:3001 and opens correctly.

I debugged it, actually, the GQL query was incorrect instead of generatePasswordResetToken() it should be generatePasswordResetToken 😑 in the file content . Thanks @FelixMalfait for quick response. Blocker resolved. I can take it further.

i-am-chitti avatar Jan 11 '24 18:01 i-am-chitti

@martmull FYI

charlesBochet avatar Jan 11 '24 19:01 charlesBochet

Hello @i-am-chitti, FYI email templating is available packages/twenty-server/src/emails, you can have an example of an email using the templating here packages/twenty-server/src/workspace/cron/clean-inactive-workspaces/clean-inactive-workspaces.email.tsx and its usage in the command packages/twenty-server/src/workspace/cron/clean-inactive-workspaces/clean-inactive-workspace.job.ts (look for render( @react-email library function call in that file)

It is not perfect at all and we will move that templating in a dedicated package soon in the future, but you can use the current templating for your change password email

Feel free to ping me if you have any question, and thanks again for your contribution

martmull avatar Jan 16 '24 09:01 martmull

Thanks @martmull. How do I get the mails? Currently, they are logged in terminal but would want to see the actual email. Do we have any mailhog setup where these emails are being intercepted and can be viewed?

i-am-chitti avatar Jan 16 '24 19:01 i-am-chitti

Hey @i-am-chitti, you need to setup some env variables listed here: https://docs.twenty.com/start/self-hosting/environment-variables#email Set EMAIL_DRIVER=smtp, then set the other SMTP env variables using one of the smtp configuration example. I suggest that you use Smtp4dev for prototyping and local development, but it works well with gmail if you prefer

martmull avatar Jan 16 '24 21:01 martmull

@FelixMalfait @Bonapara How do I get to show the proper error message on frontend? For example, one request gives this error - image

But on frontend, on extracting the error from the hook of mutation, the error.message only gives Internal Server Error. Instead of this generic message, I would like to show the Token has been already generated. Please wait for 4 minutes to generate again. Any idea?

      const { data } = await emailPasswordResetLink({
        onError: (error) => {
          console.log('error message', error.message); //Internal Server error
        },
        onCompleted: (data) => {
          console.log('completed', data);
        },
      });

i-am-chitti avatar Jan 17 '24 17:01 i-am-chitti

@FelixMalfait @charlesBochet, I've opened a PR here - https://github.com/twentyhq/twenty/pull/3538

@martmull LMK if there is any change in email module. Will update this PR too.

i-am-chitti avatar Jan 18 '24 17:01 i-am-chitti

Thank you @i-am-chitti, @martmull, @lucasbordeau or @thaisguigon should take a look at your PR today :)

charlesBochet avatar Jan 22 '24 10:01 charlesBochet