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

Added Forget Password #8

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

JainPriya1234
Copy link

@varun-singhh @abhijeetnishal Please review my PR for issue #5

Copy link
Contributor

@abhijeetnishal abhijeetnishal left a comment

Choose a reason for hiding this comment

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

@JainPriya1234 Without email verification, how can you directly updating the password. Can you please explain.

@JainPriya1234
Copy link
Author

@abhijeetnishal I am thinking about updating the password after OTP verification.
Is it okay..if I make another API for OTP verification and then update the password?

@JainPriya1234
Copy link
Author

@abhijeetnishal please review my approch.So that I can work on it.

@abhijeetnishal
Copy link
Contributor

@JainPriya1234 ok I got your approach.
Hey @varun-singhh please review this PR to proceed further

@JainPriya1234
Copy link
Author

@abhijeetnishal @varun-singhh please update

@JainPriya1234
Copy link
Author

@abhijeetnishal please review my PR

@abhijeetnishal
Copy link
Contributor

abhijeetnishal commented Jun 3, 2023

Hey @JainPriya1234 atleast 2 approves are required. All contributer need to approve, wait for @varun-singhh once he approves then proceed. There are lot of projects to contribute.

const { email, password } = req.body;
if (!email || !password) throw Error("All fields are mandatory");
const { rows } = await pool.query(
"SELECT * FROM admin WHERE email = $1",

Choose a reason for hiding this comment

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

Please add a condition to check password as well, this way you are not authenticating the user

);
if (rows.length === 0) throw Error("Email not registered");
await pool.query(
"UPDATE admin SET password = $1 WHERE email = $2 ",[password,email]

Choose a reason for hiding this comment

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

cant set a password like this directly, will need encryption, check the register-user logic

@kaustubhai
Copy link

@abhijeetnishal I am thinking about updating the password after OTP verification. Is it okay..if I make another API for OTP verification and then update the password?

Are you planning to implement OTP API as well? without that it is incomplete and cant be merged. LMK if you want help with the approach. Can consider this article for it

@JainPriya1234
Copy link
Author

@abhijeetnishal @kaustubhai @varun-singhh I think the flow should be this
forgot password ->Otp send ( if the email is valid) -> Submit OTP and Password -> save password.
Yes, OTP API is needed. I can implement the OTP API also.
Please suggest any changes

@kaustubhai
Copy link

forgot password > Otp send ( if the email is valid) -> Submit OTP and Password -> save password.

I'm satisfied with this approach. But... where are you planning to store the OTP to validate it?

@JainPriya1234
Copy link
Author

forgot password > Otp send ( if the email is valid) -> Submit OTP and Password -> save password.

I'm satisfied with this approach. But... where are you planning to store the OTP to validate it?

@kaustubhai yes i will store OTP in database but temporarily for 5-10 min

@kaustubhai
Copy link

@kaustubhai yes i will store OTP in database but temporarily for 5-10 min

Not sure how are you planning to do this but please go ahead.

I would again suggest you to go through the article I shared before as storing data temporary will need in-memory db implementation like redis

@JainPriya1234
Copy link
Author

JainPriya1234 commented Jun 7, 2023

@kaustubhai yes i will store OTP in database but temporarily for 5-10 min

Not sure how are you planning to do this but please go ahead.

I would again suggest you to go through the article I shared before as storing data temporary will need in-memory db implementation like redis

okay, @kaustubhai I will follow the article you suggested earlier
Will follow this approach
image

@kaustubhai
Copy link

Still working on this @JainPriya1234 ?

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.

3 participants