Skip to content

finish auth application#248

Open
sashkatmshchk-pixel wants to merge 3 commits intomate-academy:masterfrom
sashkatmshchk-pixel:develop
Open

finish auth application#248
sashkatmshchk-pixel wants to merge 3 commits intomate-academy:masterfrom
sashkatmshchk-pixel:develop

Conversation

@sashkatmshchk-pixel
Copy link

No description provided.

Copy link

@mateacademy-ai-mentor mateacademy-ai-mentor left a comment

Choose a reason for hiding this comment

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

Review Summary

You've built a solid foundation for the authentication system, and the registration and activation flows are a great start. However, this submission is missing several core features required by the task, so I am requesting a few changes before approval.

Specifically, the following functionalities need to be implemented:

  • The complete logout and password reset flows.
  • Endpoints for users to update their profile (name, email, and password).
  • Password validation during registration to enforce complexity rules.
  • Redirects to the profile page after a successful login and account activation. Instead of sending a token or text response in src/controllers/auth.controller.js, you should use res.redirect('/profile') as required.

You can add the new logic in src/controllers/auth.controller.js and the corresponding routes in src/routes/auth.routes.js. Once these features are in place, the application will be complete. Keep up the great work!


✨ 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

  1. Rate AI review example

  2. Re-request AI review example

  3. Add AI review comments example

Comment on lines +13 to +15
if (!name || !email || !password) {
return res.status(400).send('Missing fields');
}

Choose a reason for hiding this comment

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

According to the requirements, you need to check the password against defined rules. This validation is missing. It's also a good practice to check if a user with the provided email already exists to prevent duplicate accounts.

user.isActive = true;
delete user.activationToken;

res.send('Account activated');

Choose a reason for hiding this comment

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

The task requires redirecting the user to the Profile page after account activation. Instead of sending a text response, you should perform a redirect, for example, by using res.redirect('/profile').

expiresIn: '1d',
});

res.send({ token });

Choose a reason for hiding this comment

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

The requirements state that the user should be redirected to the profile page after a successful login. Returning a token is a common practice for APIs, but for this task, a server-side redirect is expected.

Comment on lines +84 to +88
module.exports = {
register,
activate,
login,
profile,

Choose a reason for hiding this comment

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

This controller is missing several required functionalities:

  • A logout function.
  • A complete password reset flow (requesting and confirming).
  • Endpoints for updating the user's profile (name, password, and email).

return res.status(401).send('Unauthorized');
}

const token = header.split(' ')[1];

Choose a reason for hiding this comment

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

The Authorization header is expected to be in the format Bearer <token>. This line assumes there will always be a space and the token will be the second element. It would be more robust to check for the Bearer scheme and ensure a token exists before attempting to use it.

Comment on lines +14 to +20
router.post('/register', register);

router.get('/activate/:token', activate);

router.post('/login', login);

router.get('/profile', authMiddleware, profile);

Choose a reason for hiding this comment

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

This router is missing endpoints for several required features. Based on the task description, you need to add routes for:

  • Logout
  • Password reset
  • Profile updates (changing name, password, and email)

Copy link

@mateacademy-ai-mentor mateacademy-ai-mentor left a comment

Choose a reason for hiding this comment

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

Review Summary

Excellent work on implementing the core features! The application is very close to completion. I'm requesting a few changes to fully align with the project requirements and improve consistency.

Here are the key points to address in src/controllers/auth.controller.js:

  1. Registration Response: The registration endpoint sends back the entire user object, which exposes sensitive data like the password hash. Please change this to send a simple confirmation message, for example: res.status(201).send('Registration successful. Please check your email for the activation link.');
  2. Password Reset Flow: The password reset endpoints currently return plain text. The requirements specify showing an "email sent" page and a "success page." Please adjust these to send a response that helps the frontend render the correct pages, which is a more user-friendly approach.
  3. User Enumeration Vulnerability: The requestPasswordReset function returns a 'User not found' error if the email doesn't exist. To prevent attackers from guessing registered emails, it's a security best practice to always return a generic success message, regardless of whether the user exists.

You've made great progress. Once these adjustments are made, the project will be complete!


✨ 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

  1. Rate AI review example

  2. Re-request AI review example

  3. Add AI review comments example


await sendActivationEmail(email, activationToken);

res.status(201).send(user);

Choose a reason for hiding this comment

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

The response for a successful registration should guide the user on the next step. Sending back the entire user object is inconsistent with other flows (like login or activation which redirect) and also exposes sensitive information like the password hash.

A better approach would be to send a clear message, for example:
res.status(201).send('Registration successful. Please check your email for the activation link.');

Comment on lines +114 to +116
if (!user) {
return res.status(404).send('User not found');
}

Choose a reason for hiding this comment

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

Returning a 'User not found' error here can lead to a security issue known as user enumeration, where an attacker could guess which email addresses are registered. It's a best practice to always return the same generic success message, regardless of whether the user exists or not. The email-sending logic should, of course, only run if the user is found.

user.password = await bcrypt.hash(password, 10);
delete user.resetToken;

res.send('Password updated');

Choose a reason for hiding this comment

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

While this response is correct, the requirement mentions showing a "Success page with a link to login". To better support the frontend in creating this page, consider sending a more descriptive response. For instance, a JSON object could provide both a message and the login path:

res.json({ 
  message: 'Password has been successfully reset.', 
  loginLink: '/login' 
});

Comment on lines +1 to +25
'use strict';

const jwt = require('jsonwebtoken');

const authMiddleware = (req, res, next) => {
const header = req.headers.authorization;

if (!header) {
return res.status(401).send('Unauthorized');
}

const token = header.split(' ')[1];

try {
const user = jwt.verify(token, 'secret');

req.user = user;

next();
} catch {
res.status(401).send('Invalid token');
}
};

module.exports = authMiddleware;

Choose a reason for hiding this comment

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

This file seems to be an unused duplicate. The application's routes use src/middleware/auth_middleware.js instead. To avoid confusion and keep the project clean, it's best to remove this file.

Copy link

@mateacademy-ai-mentor mateacademy-ai-mentor left a comment

Choose a reason for hiding this comment

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

Review Summary

Great job on this submission! I am approving it as an educational override to help you move forward, but there are a few important improvements to consider for your next project.

You've done excellent work implementing the core features, especially with the secure authentication middleware and clean routing. The progress you've made is fantastic!

To make it perfect, please remember to:

  • Validate password strength everywhere: In src/controllers/auth.controller.js, reuse your isPasswordValid function when users reset their password (resetPassword) or update it (updateProfilePassword) to ensure all new passwords are secure.
  • Confirm the new email: The updateProfileEmail function should check for a matching confirmNewEmail field in the request, as per the requirements.
  • Clean up unused files: The file src/middleware/auth.middleware.js isn't being used and can be safely removed to keep the project tidy.

Keep up the great work!


✨ Please rate this review in the Platform's notification center1. I will be grateful for your feedback! ✨

Footnotes

  1. Rate AI review example

return res.status(401).send('Unauthorized');
}

const token = header.split(' ')[1];

Choose a reason for hiding this comment

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

This line assumes the header format is correct but doesn't validate it. For instance, a header like 'Basic dXNlcjpwYXNz' would pass this split, but jwt.verify would fail. The check in your other middleware file, auth_middleware.js, which verifies the 'Bearer' scheme and token existence, is a much more robust approach.


if (password !== confirmPassword) {
return res.status(400).send('Passwords do not match');
}

Choose a reason for hiding this comment

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

After this check, you should also validate the new password's strength using the isPasswordValid function. This ensures consistency with the registration requirements and keeps all user passwords secure.


if (newPassword !== confirmPassword) {
return res.status(400).send('Passwords do not match');
}

Choose a reason for hiding this comment

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

The newPassword should be validated against the strength requirements using isPasswordValid. This is to ensure consistency with the password rules set during registration.

};

const updateProfileEmail = async (req, res) => {
const { password, newEmail } = req.body;

Choose a reason for hiding this comment

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

The requirement says to 'confirm the new email'. This usually means the request should include a confirmation field (e.g., confirmNewEmail) that must match the newEmail field. Please add this validation check.

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.

2 participants