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

Change password from settings #3245

Closed
Bonapara opened this issue Jan 5, 2024 · 17 comments · Fixed by #3538 or #3637
Closed

Change password from settings #3245

Bonapara opened this issue Jan 5, 2024 · 17 comments · Fixed by #3538 or #3637

Comments

@Bonapara
Copy link
Member

Bonapara commented Jan 5, 2024

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

@i-am-chitti
Copy link
Contributor

i-am-chitti commented Jan 6, 2024

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.

@FelixMalfait
Copy link
Member

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

@i-am-chitti
Copy link
Contributor

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

@FelixMalfait
Copy link
Member

@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 :)

@martmull
Copy link
Contributor

martmull commented Jan 9, 2024

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.

@i-am-chitti
Copy link
Contributor

i-am-chitti commented Jan 9, 2024 via email

@martmull
Copy link
Contributor

martmull commented Jan 9, 2024

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

@i-am-chitti
Copy link
Contributor

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?

@FelixMalfait
Copy link
Member

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

@i-am-chitti
Copy link
Contributor

i-am-chitti commented Jan 11, 2024

.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.

@charlesBochet
Copy link
Member

@martmull FYI

@martmull
Copy link
Contributor

martmull commented Jan 16, 2024

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

i-am-chitti added a commit to i-am-chitti/twenty that referenced this issue Jan 16, 2024
i-am-chitti added a commit to i-am-chitti/twenty that referenced this issue Jan 16, 2024
i-am-chitti added a commit to i-am-chitti/twenty that referenced this issue Jan 16, 2024
i-am-chitti added a commit to i-am-chitti/twenty that referenced this issue Jan 16, 2024
@i-am-chitti
Copy link
Contributor

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?

@martmull
Copy link
Contributor

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

i-am-chitti added a commit to i-am-chitti/twenty that referenced this issue Jan 17, 2024
@i-am-chitti
Copy link
Contributor

@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
Copy link
Contributor

@FelixMalfait @charlesBochet, I've opened a PR here - #3538

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

@charlesBochet
Copy link
Member

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

i-am-chitti added a commit to i-am-chitti/twenty that referenced this issue Jan 23, 2024
i-am-chitti added a commit to i-am-chitti/twenty that referenced this issue Jan 23, 2024
i-am-chitti added a commit to i-am-chitti/twenty that referenced this issue Jan 24, 2024
martmull added a commit that referenced this issue Jan 25, 2024
* GH-3245 add passwordResetToken and passwordResetTokenExpiresAt column on user entity

* Add password reset token expiry delay env variable

* Add generatePasswordResetToken mutation resolver

* Update .env.sample file on server

* Add password reset token and expiry migration script

* Add validate password reset token query and a dummy password update (WIP) resolver

* Fix bug in password reset token generate

* add update password mutation

* Update name and add email password reset link

* Add change password UI on settings page

* Add reset password route on frontend

* Add reset password form UI

* sign in user on password reset

* format code

* make PASSWORD_RESET_TOKEN_EXPIRES_IN optional

* add email template for password reset

* Improve error message

* Rename methods and DTO to improve naming

* fix formatting of backend code

* Update change password component

* Update password reset via token component

* update graphql files

* spelling fix

* Make password-reset route authless on frontend

* show token generation wait time

* remove constant from .env.example

* Add PASSWORD_RESET_TOKEN_EXPIRES_IN in docs

* refactor emails module in reset password

* update Graphql generated file

* update email template of password reset

* add space between date and text

* update method name

* fix lint issues

* remove unused code, fix indentation, and email link color

* update test file for auth and token service

* Fix ci: build twenty-emails when running tests

---------

Co-authored-by: martmull <martmull@hotmail.fr>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
5 participants