chore: fix eslint config for ESM and ignore local env/artifacts #227
Conversation
mateacademy-ai-mentor
left a comment
mateacademy-ai-mentor
left a comment
There was a problem hiding this comment.
Review Summary
You've built a solid application with a great structure and well-implemented services. However, some changes are needed to the route protection logic to meet all the requirements.
Currently, the guestMiddleware in src/middlewares/guest.middleware.js is not effective because req.user is not populated on guest routes. This allows authenticated users to access pages like /login and /register, which should be blocked. Additionally, the /auth/logout route in src/routes/auth.routes.js is unprotected and should only be accessible to logged-in users.
To fix this, please ensure the guestMiddleware correctly identifies authenticated users and apply the authMiddleware to the logout route. Addressing these middleware issues will ensure your application's access control works as required.
✨ 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
| @@ -0,0 +1,9 @@ | |||
| export function guestMiddleware(req, res, next) { | |||
| if (req.user) { | |||
There was a problem hiding this comment.
This check for req.user is the correct approach for a guest middleware. However, req.user is not populated on the routes where this middleware is used (e.g., /register or /login), so an already authenticated user will not be blocked.
Consider implementing a middleware that runs earlier in the request lifecycle to check for a token and populate req.user if it's valid, but doesn't reject the request if no token is found. This would make the user's authentication status available to all subsequent middleware and handlers.
src/routes/auth.routes.js
Outdated
| authController.resetConfirm, | ||
| ); | ||
|
|
||
| router.post('/logout', authController.logout); |
There was a problem hiding this comment.
According to the task requirements, the logout feature should only be accessible to authenticated users. This route is currently unprotected. You should add the authMiddleware to ensure only logged-in users can access it.
mateacademy-ai-mentor
left a comment
mateacademy-ai-mentor
left a comment
There was a problem hiding this comment.
Review Summary
Excellent work on this project! The overall structure is solid, and the business logic is well-implemented. However, changes are needed because a key access control requirement is not met: authenticated users can still access guest-only pages like /login and /register.
This issue occurs because the guestMiddleware doesn't have the user information it needs. To fix this, you need to apply your authOptionalMiddleware globally in src/app.js. By adding app.use(authOptionalMiddleware); before your main routes are mounted, req.user will be populated on every request, allowing the guest middleware to correctly block logged-in users.
This is the final piece of the puzzle. Once you make this small but critical change, the application 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
| app.use(express.json()); | ||
| app.use(cookieParser()); | ||
|
|
||
| app.use('/', routes); |
There was a problem hiding this comment.
To ensure guest routes are properly protected, you should apply a middleware here that populates req.user for authenticated users. The authOptionalMiddleware you've created is perfect for this task. By adding app.use(authOptionalMiddleware); before this line, you'll make user information available to guestMiddleware across all routes, fixing the issue where logged-in users could access pages like /login.
mateacademy-ai-mentor
left a comment
mateacademy-ai-mentor
left a comment
There was a problem hiding this comment.
Review Summary
Excellent work on this project! Your business logic, security practices, and overall code structure are professional and meet all the requirements. I am approving this submission as an educational override so you can move forward, but there is one critical issue causing the build to fail.
You've added a file, src/index.js, which is a duplicate of src/app.js and contains an import error. It incorrectly tries to import { notFoundMiddleware } when the correct export is notFound.
To fix this, please delete the src/index.js file. Your application is correctly configured in src/app.js, so this duplicate file is not needed. After removing it, your project will be in perfect shape. Keep up the fantastic work!
✨ Please rate this review in the Platform's notification center1. I will be grateful for your feedback! ✨
Footnotes
|
|
||
| import routes from './routes/index.js'; | ||
| import { authOpt } from './middlewares/authOpt.middleware.js'; | ||
| import { notFoundMiddleware } from './middlewares/notFound.middleware.js'; |
There was a problem hiding this comment.
The named import notFoundMiddleware does not exist in ./middlewares/notFound.middleware.js. The file exports a function named notFound. This will cause the application to crash on startup.
This file also seems to be a duplicate of src/app.js, which is implemented correctly. You should remove this file to fix the error and avoid confusion.
2 participants
No description provided.