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

Forget password restructured #140

Merged
merged 7 commits into from
Feb 25, 2022
Merged

Forget password restructured #140

merged 7 commits into from
Feb 25, 2022

Conversation

kevhines
Copy link
Contributor

@kevhines kevhines commented Feb 7, 2022

Dev Summary

This replaces PR 138 - forget password
Because all the client files had moved it felt easier (?) to just make a new branch and bring my changes in since it hit far fewer files.
I addressed issues that were on PR 138 (except where noted - see below)

This is the first part fo the Password Reset Flow issue (I didn't link it because this PR shouldn't close that issue as it will only be half completed)

client - front end files
src/features/users/Auth/ForgetPassword.tsx - created file, based on Login.tsx - prompts for email, once it gets an email should display a snackbar saying message is sent and remove field to enter password. This response should not let the user know this the email was found in the database. it then routes the form data (email) to api/users/reset_password on backend
src/routes.tsx - modified Routes to go to ForgetPassword.tsx when user clicks Forgot Password link from login.tsx page
src/features/action/assests/SimpleSnackbar.tsx - modified this as it had a hardcoded message. Now it takes in a message to be displayed (but defaults to "You Claimed This!" so as not to effect the other page that uses this function

Additionally now I have changed:
src/features/Main.tsx - to handle the more dynamic route set up we are using for ForgetPassword.
src/features/users/Auth/PasswordInput.tsx - changed the Forget Password link to use the dynamic ability to grab it from routes.tsx
server - backend files

src/users/users.controller.ts - importer Request and created a route for Post('reset_password'). It takes the email and attempts to find the users using already created function from user.serve findByEmail.
If it finds the user. Right now it doesn't do anything (though I tested that it knows if it finds the user via console.log's - but I commented those out.

PREVIOUS PR CHANGES
use kabab case for routes - this didn't need to be fixed as they got fixed in ynda's changes and I matched that.
README.md mistake - cleaned up since I rebranched and didn't keep my mistake change to this file
correct props typing in SimpleSnackbar - fixed
change the handling of my resetPassword method in the UserController to use a cleaner Response method in error handling and slot the eventually email sending in the try piece of code. - fixed and removed all old comments too.
clear commented out imports and code in ForgotPassword.tsx (leftover from what I copied from Login.tsx) - fixed
clear the email field (form data) after form is submitted - fixed
Modify button to change it's text while loading (instead of a seperate piece of text below the button) - fixed

Don't resolve error handling of email form - not fixed. The PasswordInput.tsx file is looking for that to sent, and so I left it until such time as I (or someone) can fix it in other places that the component is called.

NEXT STEPS

Need to send an email to the user if the user is found. This email will need to have a token so it can send that along to the change password page (not created yet).
The frontend change password page needs to be created as well as backend route to do that process.

Some of my research shows that I think we might need to create a table to hold these tokens temporarily. So when they are created they are stored and can be compared when the password update request comes in. These tokens should be removed after a time as well as removed if the password is updated.
James seems to think this is not needed and he's most likely right! But I link to one example (though it's node.js) that does create a table with the temporary toknes below in resources.

Test Plan

On the front end you can click Forgot Password (from the login screen) and be taken to a new page to enter your email. Regardless of what email you entered you should get the same result.
(backend should only find the user if you enter an email that exists in your database - but right now that does not change functionality)

Resources

This is for node.js but I think a similar methodology will be needed for nest.js
https://blog.logrocket.com/implementing-a-secure-password-reset-in-node-js/

Copy link
Contributor

@jd2rogers2 jd2rogers2 left a comment

Choose a reason for hiding this comment

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

2 small things but otherwise ready to go 🚀

@@ -17,6 +29,16 @@ export class UsersController {
return user;
}

@Post('reset_password')
async resetPassword(@Request() req, @Response({ passthrough: true }) response: ResponseT) {
Copy link
Contributor

Choose a reason for hiding this comment

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

oh one more thing here, return typing
i think should be ): Promise<void> { since we're not returning anything
nbd if we don't have it, just a nice to have

Copy link
Contributor Author

Choose a reason for hiding this comment

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

great. done.

value={formData.email}
placeholder="[email protected]"
onChange={handleChange}
error={error?.type === 'email' ? error.message : null}
Copy link
Contributor

Choose a reason for hiding this comment

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

right now your error state is always null, because you don't have a setError ever being called (which would come from the useState line)
i say just hardcode pass in null for now, remove that error state from here, and write up an issue to move the error state management to live inside the EmailInput, not ideal to have to rewrite that everywhere

Copy link
Contributor Author

Choose a reason for hiding this comment

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

makes sense. Thanks. Changed.

…lin ForgetPassword.tsx set error as null in EmailInput to ease changes when error handeling is set inside EmailInput entirely
@kevhines
Copy link
Contributor Author

kevhines commented Feb 8, 2022

Made these two fixes. I'll archive the old PR and branch once we/I merge this into main.

@kevhines kevhines merged commit 342b3ed into main Feb 25, 2022
@kevhines kevhines deleted the forget_password_restructured branch February 25, 2022 01:27
@jd2rogers2 jd2rogers2 linked an issue Mar 2, 2022 that may be closed by this pull request
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.

password reset flow
2 participants