feat: implement auth application#296
Conversation
mateacademy-ai-mentor
left a comment
mateacademy-ai-mentor
left a comment
There was a problem hiding this comment.
Review Summary
I am requesting changes because a few key requirements are not yet fully met, and they affect the core functionality of the auth flow.
Critical issues that must be fixed:
- Post-activation behavior (blocker) – In
src/controllers/authController.js,activateAccountcurrently just rendersauth/activationand never setsreq.session.userIdor redirects. The requirements say that after confirming via email, the user should be considered logged in and redirected to/profile. You should, after successfully activating (user.isActive = true), set the session (e.g.req.session.userId = user._id) and thenres.redirect('/profile')instead of rendering the activation view. - Email change confirmation (blocker) – In
updateEmail(same controller file), you only acceptnewEmailand do not require a confirmation field. The spec explicitly says the user must “confirm the new email”, so update both the form and controller to accept something likenewEmailandnewEmailConfirmationand verify they match before updating the email. - Password reset must be guest-only (blocker) – The token-based reset routes (
GET /reset-password/:tokenandPOST /reset-password/:tokeninsrc/routes/authRoutes.js) are currently not protected byisGuest, so authenticated users can access them, which contradicts the “Password reset (only non authenticated)” requirement. Wrap both of these routes withisGuestjust like the initial/reset-passwordroutes.
Aside from these points, the rest of the flows (registration with validation, login/logout, profile updates, and reset mechanics) are solid and close to the specification. Once you address the activation redirect/login, add new-email confirmation, and tighten access control on reset-by-token routes, your solution will align well with the task requirements.
✨ Please rate this review in the Platform's notification center1. I will be grateful for your feedback! ✨
Important
Fix the issues, commit the changes, and then re-request my review using the Re-request review button2 in the Reviewers tab.
Tip
If you have any comments about this review, please leave a message in the Add a comment section below and mention me with the @mateacademy-ai-mentor tag3.
I will read your message during my next review of your pull request. Please do not reply directly to my comments, as that feature is not supported yet.
Caution
Do not close the pull request until the review is complete. Closing it early can lead to unexpected behavior and disrupt the review process.
Footnotes
| const activateAccount = async (req, res) => { | ||
| try { | ||
| const user = await User.findOne({ activationToken: req.params.token }); | ||
|
|
||
| if (!user) { | ||
| return res.render('auth/activation', { success: false }); | ||
| } | ||
|
|
||
| user.isActive = true; | ||
| user.activationToken = null; | ||
| await user.save(); | ||
|
|
||
| return res.render('auth/activation', { success: true }); |
There was a problem hiding this comment.
According to the requirements, after activation the user should be activated only after email confirmation and then redirected to the Profile page; here you render an auth/activation view and never set req.session.userId, so the user is not logged in and not redirected to /profile as specified.
| const updateEmail = async (req, res) => { | ||
| const { password, newEmail } = req.body; |
There was a problem hiding this comment.
The email-change requirement says the user should type the password and confirm the new email; this handler only accepts a single newEmail value and never checks that a confirmation value matches, so you’re missing the "confirm new email" step.
| isActive: { | ||
| type: Boolean, | ||
| default: false, |
There was a problem hiding this comment.
The specification says "Password reset (only non authenticated)", but the token-based reset routes are not protected by isGuest, so authenticated users can access them. Consider adding the isGuest middleware here (and on the POST route) to keep the entire reset flow guest-only.
mateacademy-ai-mentor
left a comment
mateacademy-ai-mentor
left a comment
There was a problem hiding this comment.
Review Summary
Your solution is approved because it meets the core functional requirements of the auth assignment without any remaining critical blockers. Registration validates inputs and enforces explicit password rules, sends an activation email with a token, and activation correctly marks the account as active, logs the user in via the session, and redirects to /profile. Login and logout behave as required, including rejecting inactive users and redirecting appropriately, and access control is correctly implemented with isGuest and isAuth on all relevant routes (registration, login, activation, reset flows, profile, and logout). The password reset flow fully matches the spec: request by email, “email sent” state, token-based reset with expiry, password/confirmation matching, and a success page with a login path. The profile page supports updating name, password (old/new/confirmation), and email (with password check, new email confirmation, and notification to the old email), which covers the specified profile features.
As minor, non-blocking improvements, you could enhance UX by providing more descriptive error messages on activation/reset failures and by standardizing user-facing text, and you might later consider adding rate limiting or more generic error responses on login/reset endpoints for better security. Overall, this is a solid implementation that aligns well with the requirements—nice work bringing all the flows together correctly.
✨ Please rate this review in the Platform's notification center1. I will be grateful for your feedback! ✨
Footnotes
2 participants
No description provided.