Develop#291
Conversation
mateacademy-ai-mentor
left a comment
mateacademy-ai-mentor
left a comment
There was a problem hiding this comment.
Review Summary
The implementation has several critical issues that block requirements from the task specification. The login function does not check if users are activated before authenticating, violating the explicit requirement to prompt inactive users to activate their email. Multiple redirects go to the wrong pages: logout redirects to '/' instead of '/login', login redirects to '/' instead of '/profile', and AccountActivationPage doesn't redirect to '/profile' after activation. Additionally, there's a critical security issue where OAuth users with null passwords will cause runtime errors when calling bcrypt.compare. These issues must be fixed before approval.
✨ 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 handleLogout = () => { | ||
| logout() | ||
| .then(() => { |
There was a problem hiding this comment.
The task requirement states: 'Must redirect to login page after logging out'. Currently, the logout redirects to '/' but should redirect to '/login' instead.
| <Route path="/" element={<RequireAuth />}> | ||
| <Route path="users" element={<UsersPage />} /> | ||
| </Route> | ||
|
|
||
| <Route path="forgot-password" element={<ForgotPasswordPage />} /> | ||
| <Route path="forgot-password/sent" element={<PasswordResetEmailSentPage />} /> | ||
| <Route path="reset-password/:resetToken" element={<ResetPasswordPage />} /> | ||
| <Route path="reset-password/success" element={<ResetPasswordSuccessPage />} /> | ||
|
|
||
| <Route path="/" element={<RequireAuth />}> | ||
| <Route path="users" element={<UsersPage />} /> |
There was a problem hiding this comment.
Duplicate route definition for 'users' - this route is already defined at lines 115-117 inside RequireAuth, and redefined at lines 124-127. The second definition will override the first, making the first one unreachable.
|
|
||
| const login: RequestHandler = async (req, res) => { | ||
| const { email, password } = req.body; | ||
|
|
||
| const user = await usersRepository.getByEmail(email); | ||
|
|
||
| if (!user) { | ||
| return res.status(401).json({ message: 'Invalid credentials' }); | ||
| } | ||
|
|
||
| const isPasswordValid = await bcrypt.compare(password, user?.password || ''); | ||
|
|
||
| if (!isPasswordValid) { | ||
| return res.status(401).json({ message: 'Invalid credentials' }); | ||
| } | ||
|
|
||
| sendAuthentication(res, user); |
There was a problem hiding this comment.
The login function does not check if the user is activated. According to the task requirement: 'If user is not active ask them to activate their email'. The function should verify that user.isActivated === true before allowing login, and return an appropriate error message if not.
| } | ||
|
|
||
| const emailError = userService.validateEmail(email); | ||
|
|
||
| if (emailError) { | ||
| return res.status(400).json({ message: emailError }); | ||
| } | ||
|
|
||
| const user = await usersRepository.getById(userId); | ||
|
|
||
| if (!user) { | ||
| return res.status(404).json({ message: 'User not found' }); | ||
| } | ||
|
|
||
| const isPasswordValid = await bcrypt.compare(password, user.password); | ||
|
|
There was a problem hiding this comment.
Critical: The login handler doesn't check if the user is activated. The task requirement states: 'If user is not active, must ask them to activate their email'. Add a check like: if (!user.isActivated) { return res.status(403).json({ message: 'Please activate your account' }); }
| } | ||
|
|
||
| const user = await usersRepository.getById(userId); | ||
|
|
There was a problem hiding this comment.
Critical Security Issue: bcrypt.compare(password, user?.password || '') uses empty string fallback, but user.password can be null for OAuth users (see migration: ALTER COLUMN "password" DROP NOT NULL). This allows login with invalid credentials for OAuth users.
|
|
||
| return ( | ||
| <> | ||
| <h1 className="title">Account activation</h1> | ||
|
|
||
| {error ? ( | ||
| <p className="notification is-danger is-light">{error}</p> | ||
| ) : ( | ||
| <p className="notification is-success is-light"> | ||
| Your account is now active | ||
| </p> | ||
| )} | ||
| </> | ||
| ); |
There was a problem hiding this comment.
The AccountActivationPage shows 'Your account is now active' but doesn't redirect to the Profile page. The task requirement states: 'redirect to Profile after the activation'. After successful activation, consider using useNavigate to redirect to /profile.
| <> | ||
| <Formik | ||
| initialValues={{ | ||
| email: '', | ||
| password: '', | ||
| }} | ||
| validateOnMount={true} | ||
| onSubmit={({ email, password }) => { | ||
| return login(email, password) | ||
| .then(() => { | ||
| const state = location.state as { from?: Location }; | ||
| navigate(state.from?.pathname ?? '/'); | ||
| }) | ||
| .catch((error: AxiosError<{ message?: string }>) => { |
There was a problem hiding this comment.
The task requirement states: 'redirect to Profile after the activation'. Currently, after successful activation, the page only shows a message without redirecting. Consider using useNavigate to redirect to /profile after successful activation (when error is empty and done is true).
| return login(email, password) | ||
| .then(() => { | ||
| const state = location.state as { from?: Location }; |
There was a problem hiding this comment.
The task requirement states: 'Redirect to profile after login'. Currently, on successful login, the page navigates to state.from?.pathname ?? '/' (home page). Change this to navigate to /profile instead.
| const validationError = validateEmail(email); | ||
|
|
||
| if (validationError) { | ||
| setEmailError(validationError); | ||
| return; | ||
| } | ||
|
|
||
| try { | ||
| await authService.requestPasswordReset(email); | ||
| navigate('/forgot-password/sent'); | ||
| } catch (error) { | ||
| const err = error as AxiosError<{ message?: string }>; | ||
|
|
||
| setError(err.response?.data?.message ?? 'Something went wrong'); |
There was a problem hiding this comment.
The task requirement states: 'redirect to Profile after the activation'. After successful account activation, the page should use useNavigate to redirect to /profile instead of just showing a success message.
| await authService.requestPasswordReset(email); | ||
| navigate('/forgot-password/sent'); | ||
| } catch (error) { |
There was a problem hiding this comment.
The task requirement states: 'Redirect to profile after login'. Currently, the code navigates to state.from?.pathname ?? '/' which could send users to any page they came from, not specifically to profile. Consider changing to navigate('/profile') instead.
mateacademy-ai-mentor
left a comment
mateacademy-ai-mentor
left a comment
There was a problem hiding this comment.
Review Summary
The implementation correctly addresses all authentication requirements from the task specification. All core features are properly implemented: registration with activation email, login with activation status check, logout with redirect to login, password reset flow with confirmation validation, profile management for name/password/email changes, 404 page handling, and Google OAuth using Passport.js. The CI test failure is due to a missing test script in package.json, which is an environment/setup issue rather than a code implementation problem. Add a test script to package.json (e.g., "test": "jest") to resolve the CI workflow. The codebase demonstrates solid architecture with proper separation of concerns and follows React best practices throughout.
✨ 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
mateacademy-ai-mentor
left a comment
mateacademy-ai-mentor
left a comment
There was a problem hiding this comment.
Review Summary
All core authentication requirements have been successfully implemented, including registration with activation emails, login with activation checks, logout redirects, password reset flows, profile management (name, password, email changes), and OAuth support with null password safety. All previously identified critical issues have been resolved. The GitHub Actions test failure is due to a missing test script in package.json (a CI configuration issue, not a code implementation problem) and should be addressed separately by adding an appropriate test script. No critical blockers remain.
✨ 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
mateacademy-ai-mentor
left a comment
mateacademy-ai-mentor
left a comment
There was a problem hiding this comment.
Review Summary
Your implementation demonstrates a solid understanding of authentication flows, including registration with email activation, login/logout with proper redirects, password reset with token validation, and profile management with security-conscious patterns. However, three issues should be addressed: (1) In src/api/auth.controller.ts, using bcrypt.genSaltSync(1) for activation token generation is cryptographically weak; use crypto.randomBytes() instead for random tokens. (2) The RequireNonAuth component exists but isn't used in App.tsx; wrap login/register/password-reset routes with it for consistency with RequireAuth. (3) In src/index.js, importing .ts files directly will fail at runtime unless compiled or using ts-node/register; use .js extensions matching your compiled output. These are straightforward fixes, and your overall implementation is well-structured and functional.
✨ Please rate this review in the Platform's notification center1. I will be grateful for your feedback! ✨
Footnotes
|
|
||
| const register: RequestHandler = async (req, res) => { | ||
| const { name, email, password } = req.body; | ||
|
|
There was a problem hiding this comment.
Using bcrypt.genSaltSync(1) for activation token generation is cryptographically weak. Bcrypt is designed for passwords with many iterations; for random tokens, crypto.randomBytes() should be used instead.
| import passport from 'passport'; | ||
| import { Strategy as GoogleStrategy } from 'passport-google-oauth20'; | ||
|
|
||
| import { usersRepository } from '../entity/users.repository'; | ||
|
|
||
| passport.use( | ||
| new GoogleStrategy( | ||
| { | ||
| clientID: process.env.GOOGLE_CLIENT_ID!, | ||
| clientSecret: process.env.GOOGLE_CLIENT_SECRET!, | ||
| callbackURL: process.env.GOOGLE_CALLBACK_URL!, | ||
| }, | ||
| async (_accessToken, _refreshToken, profile, done) => { | ||
| try { | ||
| const email = profile.emails?.[0]?.value; | ||
|
|
||
| if (!email) { |
There was a problem hiding this comment.
RequireNonAuth component exists but isn't used in App.tsx. Registration and login pages have internal Navigate redirects instead. For consistency with how RequireAuth is used for protected routes, consider wrapping these routes with RequireNonAuth in the router configuration.
| export function authMiddleware( | ||
| req: Request, | ||
| res: Response, |
There was a problem hiding this comment.
The Express server (index.js) imports .ts files directly (auth.route.ts, users.route.ts, profile.route.ts). This will fail at runtime unless ts-node/register is configured or the files are compiled first. Consider using .js extensions that match the compiled output or adding ts-node/register in the startup script.
2 participants
No description provided.