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

[Finishes #187354251] Update user password #46

Closed
wants to merge 2 commits into from

Conversation

P-Rwirangira
Copy link
Collaborator

@P-Rwirangira P-Rwirangira commented Apr 30, 2024

Purpose

The purpose of the following PR is to help the user to change his/her password but not when forgotten the first password

Changes Made

-Added a controller in userController.ts for updating/changing user password
-Added API endpoint for updating/changing password in userRouter.js which is also with a middleware that checks if the user is authenticated.

Testing Instructions

Check on localhost:{PORT}/users/update-password (On Postman use POST method)

Related Issues

Reference any related issues or pull requests

Checklist

Please review the following checklist and make sure all tasks are complete before submitting:

  • Code follows the project's coding standards
  • Changes are covered by tests
  • Documentation is updated (if applicable)
  • All tests pass

Comment on lines 101 to 115
const token = req.headers.authorization?.split(' ')[1];
if (!token) {
res.status(401).json({
ok: false,
message: 'Unauthorized',
});
return;
}
const decode = jwt.verify(token, process.env.JWT_SECRET as string) as { id: string };
if (!decode) {
res.status(400).json({
ok: false,
message: 'Invalid token',
});
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

I guess you aren't supposed to write this particular code cuz there's a reusable function in middleware folder for that.

@@ -18,4 +17,6 @@ router.get('/google/callback', authenticateViaGoogle);
// Route to login a user
router.post('/login', login);

router.put('/update-password', updatePassword, isAuthenticated);
Copy link
Collaborator

Choose a reason for hiding this comment

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

Hey Patrick,
I believe the authenticated middleware should be placed at the beginning of the updatePassword controller.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Exactly, please the middleware should be between the route string and the controller function

@@ -93,5 +95,61 @@ const login = async (req: Request, res: Response): Promise<void> => {
sendInternalErrorResponse(res, err);
}
};
const updatePassword = async (req: Request, res: Response): Promise<void> => {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Let's shift this controller to the userController instead of keeping it in the authController.

Copy link
Collaborator

Choose a reason for hiding this comment

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

No Yves anything related to auth should stay in auth. Normal these are the controllers that goes in auth

  1. Login
  2. Logout
  3. Forgot Password
  4. Change Password
  5. Reset password

@@ -93,5 +95,61 @@ const login = async (req: Request, res: Response): Promise<void> => {
sendInternalErrorResponse(res, err);
}
};
const updatePassword = async (req: Request, res: Response): Promise<void> => {
Copy link
Collaborator

Choose a reason for hiding this comment

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

No Yves anything related to auth should stay in auth. Normal these are the controllers that goes in auth

  1. Login
  2. Logout
  3. Forgot Password
  4. Change Password
  5. Reset password

@@ -18,4 +17,6 @@ router.get('/google/callback', authenticateViaGoogle);
// Route to login a user
router.post('/login', login);

router.put('/update-password', updatePassword, isAuthenticated);
Copy link
Collaborator

Choose a reason for hiding this comment

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

Exactly, please the middleware should be between the route string and the controller function

@P-Rwirangira P-Rwirangira force-pushed the 187354251-ft-update-password branch 3 times, most recently from 3c6f7b5 to 8d4ac51 Compare May 1, 2024 21:24
@P-Rwirangira P-Rwirangira changed the title Update user password [Finishes #187354251] Update user password May 2, 2024
@P-Rwirangira P-Rwirangira force-pushed the 187354251-ft-update-password branch 3 times, most recently from eaba3b6 to ff623dc Compare May 2, 2024 15:47
const updatePassword = async (req: Request, res: Response): Promise<void> => {
try {
const { oldPassword, newPassword } = req.body;
const token = req.headers.authorization?.split(' ')[1];
Copy link
Collaborator

Choose a reason for hiding this comment

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

Good morning! Since there's middleware handling user authorization, there's no need for you to redundantly check the user's authorization status in your code

}

// Generate salt and hash new password
const hashedNewPassword = await passwordEncrypt(newPassword);
Copy link
Collaborator

Choose a reason for hiding this comment

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

Dont we have to validate newPassword?

Copy link
Collaborator

Choose a reason for hiding this comment

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

You are right. @A-gent64 validate passwords

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Done

@niyontwali niyontwali requested review from patrickhag and amin-leon May 6, 2024 08:52
Copy link
Collaborator

@amin-leon amin-leon left a comment

Choose a reason for hiding this comment

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

Don't we need to validate newPassword if iam not mistaken?

}

// Generate salt and hash new password
const hashedNewPassword = await passwordEncrypt(newPassword);
Copy link
Collaborator

Choose a reason for hiding this comment

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

You are right. @A-gent64 validate passwords

@P-Rwirangira P-Rwirangira force-pushed the 187354251-ft-update-password branch from ff623dc to be643e2 Compare May 6, 2024 15:26
…r-the-Project-Repository

[finishes 187511688]-implement-badges-status(CI/CD)-show-whether-pass…
@P-Rwirangira P-Rwirangira force-pushed the 187354251-ft-update-password branch from be643e2 to ae26381 Compare May 6, 2024 15:27
@hozayves hozayves self-requested a review May 6, 2024 20:50
@P-Rwirangira P-Rwirangira deleted the 187354251-ft-update-password branch May 6, 2024 22:28
@P-Rwirangira P-Rwirangira restored the 187354251-ft-update-password branch May 6, 2024 22:29
@P-Rwirangira P-Rwirangira deleted the 187354251-ft-update-password branch May 6, 2024 22:32
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants