implemented admin login/signup #442
Conversation
|
@Sourabh782 is attempting to deploy a commit to the bunty's projects Team on Vercel. A member of the Team first needs to authorize it. |
WalkthroughThe pull request introduces enhancements to the admin and customer login functionalities. Key changes include the addition of Changes
Sequence Diagram(s)sequenceDiagram
participant Admin
participant Server
participant CookieManager
participant UserContext
Admin->>Server: Submit login credentials
Server->>CookieManager: Set authToken cookie
Server->>Admin: Return response with role
Admin->>UserContext: Update user state with role
Assessment against linked issues
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
|
Thank you for submitting your pull request! 🙌 We'll review it as soon as possible. In the meantime, please ensure that your changes align with our CONTRIBUTING.md. If there are any specific instructions or feedback regarding your PR, we'll provide them here. Thanks again for your contribution! 😊 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 28
🧹 Outside diff range and nitpick comments (24)
frontend/src/router/ProtectedRoute.jsx (3)
5-9: Add prop types validationAdd PropTypes or TypeScript types to validate the children prop and improve code maintainability.
+import PropTypes from 'prop-types'; + const ProtectedRoute = ({ children }) => { // ... component logic }; + +ProtectedRoute.propTypes = { + children: PropTypes.node.isRequired, +};
5-5: Add JSDoc documentationAdd documentation to explain the component's purpose, props, and usage.
+/** + * A route wrapper that protects admin-only routes. + * Redirects to admin login if the user is not authenticated as an admin. + * + * @param {Object} props + * @param {React.ReactNode} props.children - The components to render if access is granted + * @returns {React.ReactElement} The protected route + */ const ProtectedRoute = ({ children }) => {
1-11: Consider implementing error boundariesThe component should be wrapped in an error boundary to gracefully handle runtime errors.
Would you like me to provide an example implementation of an error boundary component?
frontend/src/context/userContext.jsx (1)
1-20: Consider enhancing user context for role-based authentication.Given that this PR implements admin authentication, the context should explicitly handle user roles and permissions.
Consider these enhancements:
- Add role-based access control (RBAC)
- Include admin-specific methods in the context
- Add role validation utilities
Example structure:
export const UserContext = createContext({ user: null, setUser: () => {}, isAdmin: () => false, hasPermission: (permission) => false, });frontend/src/router/index.jsx (1)
42-51: Consider restructuring admin routes for better scalability.As you add more admin features, consider organizing admin-related routes in a separate router configuration file (e.g.,
adminRoutes.jsx). This would improve maintainability and make it easier to manage admin-specific route protection.Example structure:
// adminRoutes.jsx const adminRoutes = [ { path: "/admin", element: <ProtectedRoute><Admin /></ProtectedRoute> }, { path: "/admin-login", element: <AdminLogin /> }, { path: "/admin-signup", element: <AdminSignup /> } ]; // index.jsx import { adminRoutes } from './adminRoutes'; // ... then spread adminRoutes in your route configurationbackend/controller/admin.controller.js (1)
Line range hint
1-93: Consider implementing additional security measuresThe current implementation could benefit from several security enhancements:
- Add rate limiting to prevent brute force attacks:
const rateLimit = require('express-rate-limit'); const loginLimiter = rateLimit({ windowMs: 15 * 60 * 1000, // 15 minutes max: 5 // limit each IP to 5 login attempts per windowMs }); // Apply to login route app.use('/api/admin/login', loginLimiter);
- Validate JWT_SECRET at startup:
if (!process.env.JWT_SECRET || process.env.JWT_SECRET.length < 32) { throw new Error('JWT_SECRET must be at least 32 characters long'); }
- Consider using a more generic error message for invalid credentials:
- return res.status(401).json({ error: "Invalid email or password" }); + return res.status(401).json({ error: "Invalid credentials" });
- Add password complexity validation to the schema:
- password: z.string().min(6, "Password must be at least 6 characters long"), + password: z.string() + .min(8, "Password must be at least 8 characters long") + .regex(/[A-Z]/, "Password must contain at least one uppercase letter") + .regex(/[a-z]/, "Password must contain at least one lowercase letter") + .regex(/[0-9]/, "Password must contain at least one number") + .regex(/[^A-Za-z0-9]/, "Password must contain at least one special character"),frontend/src/components/Pages/Login.jsx (1)
Line range hint
1-156: Enhance admin login functionality.Given that this PR implements admin login/signup, consider these improvements:
- Add a toggle or separate section for admin login
- Implement role-based navigation after successful login
- Add visual indicators for admin login mode
Example implementation for role-based navigation:
if (!response.ok) { throw new Error(result.message || 'Login failed'); } + // Role-based navigation + if (result.user.role === 'admin') { + navigate('/admin/dashboard'); + } else { + navigate('/'); + } - navigate('/');Would you like me to provide a complete implementation for the admin-specific UI enhancements?
backend/controller/customer.controller.js (6)
Line range hint
89-94: Security: Reduce JWT payload information.The JWT payload contains sensitive user information that isn't necessary for authentication purposes.
Consider minimizing the JWT payload to only essential claims:
const payload = { sub: customer._id, - name: customer.name, // Optional - role: "customer", // Optional - email: customer.email, // Optional };
Line range hint
127-131: Security: Strengthen cookie configuration.While HttpOnly and Secure flags are correctly set, the cookie configuration could be enhanced.
Add SameSite attribute and domain restriction:
res.cookie("authToken", token, { maxAge: 1000 * 60 * 60, httpOnly: true, secure: true, + sameSite: 'strict', + domain: process.env.COOKIE_DOMAIN });
Line range hint
102-104: Security: Use generic error messages.Current error messages could enable user enumeration attacks by differentiating between non-existent accounts and wrong passwords.
Standardize error messages:
if (!customer) { - return res.status(401).json({ error: "Invalid email or password" }); + return res.status(401).json({ error: "Invalid credentials" }); } if (!customer.isVerified) { - return res.status(403).json({ error: "Account not verified. Please verify your email." }); + return res.status(401).json({ error: "Invalid credentials" }); }Also applies to: 108-110
Line range hint
95-146: Architecture: Consider removing session-based authentication.The code currently maintains both JWT and session-based authentication, which could lead to inconsistent auth states and increased complexity.
Consider:
- Choosing either JWT or session-based auth, not both
- If keeping both, ensure proper synchronization between them
- Document the reason for maintaining dual authentication mechanisms
Would you like me to provide an example implementation using only JWT-based authentication?
Line range hint
1-146: Security: Implement rate limiting.The authentication endpoints should implement rate limiting to prevent brute force attacks.
Consider adding rate limiting middleware using
express-rate-limit:const rateLimit = require('express-rate-limit'); const loginLimiter = rateLimit({ windowMs: 15 * 60 * 1000, // 15 minutes max: 5, // 5 attempts message: { error: 'Too many login attempts. Please try again later.' } }); // Apply to routes router.post('/login', loginLimiter, loginCustomer);
Line range hint
8-12: Security: Strengthen password requirements.Current password validation only checks for minimum length of 6 characters.
Enhance the password schema to require stronger passwords:
const customerSchema = z.object({ name: z.string().min(1, "Name is required"), email: z.string().email("Invalid email address"), - password: z.string().min(6, "Password must be at least 6 characters long"), + password: z.string() + .min(8, "Password must be at least 8 characters long") + .regex(/[A-Z]/, "Password must contain at least one uppercase letter") + .regex(/[a-z]/, "Password must contain at least one lowercase letter") + .regex(/[0-9]/, "Password must contain at least one number") + .regex(/[^A-Za-z0-9]/, "Password must contain at least one special character"), });frontend/src/components/Pages/Admin/AdminLogin.jsx (1)
1-150: Consider implementing rate limiting and monitoring.While the basic admin login functionality is implemented, consider these architectural improvements:
- Implement rate limiting to prevent brute force attacks
- Add logging for security monitoring
- Consider implementing 2FA for admin accounts
Would you like assistance in implementing any of these security enhancements?
frontend/src/components/Pages/Admin/AdminSignup.jsx (1)
1-182: Consider implementing form state management library.For a complex form with validation, consider using a form management library like Formik or React Hook Form. This would provide:
- Better validation handling
- Reduced boilerplate
- Built-in accessibility features
- More robust error handling
Would you like examples of implementing this component using a form management library?
frontend/src/components/Shared/Navbar.jsx (4)
Line range hint
19-21: Add role-based UI elements for admin users.The navbar should reflect different navigation options for admin users to align with the PR's admin functionality implementation.
Consider adding admin-specific menu items:
const menuItems = [ { name: 'HOME', path: '/' }, { name: 'EVENTS', path: '/events' }, { name: 'MENU', path: '/menu' }, { name: 'RESERVATION', path: '/reservation' }, { name: 'BOARDGAMES', path: '/boardgame' }, { name: 'MEMBERSHIP', path: '/membership' }, + // Conditionally render admin menu items + ...(Cookies.get('authenticatedUser') === 'admin' ? [ + { name: 'ADMIN DASHBOARD', path: '/admin' }, + { name: 'MANAGE USERS', path: '/admin/users' } + ] : []) ];
Line range hint
11-15: Implement proper state management for user roles.The current implementation doesn't track user roles effectively. Consider using the global context mentioned in the PR objectives.
Add role-based state management:
const [token, setToken] = useState(Cookies.get('authToken')); +const [userRole, setUserRole] = useState(Cookies.get('authenticatedUser')); const location = useLocation(); const navigate = useNavigate();Then update the useEffect:
useEffect(() => { setToken(Cookies.get('authToken')); + setUserRole(Cookies.get('authenticatedUser')); });
Line range hint
54-61: Enhance logout error handling.The logout process should handle API errors and network failures gracefully.
Implement proper error handling:
const handleLogout = async () => { + try { const response = await fetch(`${API_URL}/api/user/logout`, { method: 'POST', headers: { 'Content-Type': 'application/json', }, }) + if (!response.ok) { + throw new Error('Logout failed'); + } Cookies.remove('authToken'); Cookies.remove("authenticatedUser"); setToken(null); setIsModalOpen(false); setIsMenuOpen(false); navigate('/login'); + } catch (error) { + console.error('Logout failed:', error); + // Show error notification to user + } };
Token validation is properly implemented in the backend but needs improvement in the frontend
While the backend properly validates tokens using JWT verification (including expiration checks), the frontend Navbar component only checks for token presence without validating its state. To improve security:
- Add a check in the
useEffecthook to validate the token's existence and set the login state accordingly- Consider implementing an interceptor or authentication context to handle token validation consistently
- Add error handling for expired or invalid tokens
Example improvement for the useEffect:
useEffect(() => { const token = Cookies.get('authToken'); setToken(token); setisloggedIn(!!token); }, []);🔗 Analysis chain
Line range hint
42-52: Verify token validation on component mount.The current implementation only checks for token presence but doesn't validate its authenticity.
Let's verify the token validation implementation:
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for token validation logic in the codebase echo "Searching for token validation implementations..." rg -l "validateToken|verifyToken|checkToken" --type js --type jsx # Search for JWT verification utilities echo "Searching for JWT verification utilities..." rg -l "jwt.verify|verify\(token" --type js --type jsxLength of output: 385
Script:
#!/bin/bash # Search for token validation logic in the codebase with correct file type echo "Searching for token validation implementations..." rg -l "validateToken|verifyToken|checkToken|jwt\.verify" . # Look for useEffect hooks that might contain token validation echo "Searching for useEffect implementations in Navbar..." rg -A 10 "useEffect\(" frontend/src/components/Shared/Navbar.jsx # Search for authentication-related utilities echo "Searching for auth utilities..." fd -e js -e jsx authLength of output: 1017
Script:
#!/bin/bash # Check the token validation implementation in auth middleware echo "Checking auth middleware implementation..." cat backend/middlewares/authCustomer.js # Check if there's any token validation in the Navbar component echo "Checking token handling in Navbar..." rg -C 5 "setToken|authToken" frontend/src/components/Shared/Navbar.jsxLength of output: 2300
frontend/src/components/Pages/Admin.jsx (5)
Line range hint
38-40: Remove unused Admin object definition.The
Adminobject with hardcoded name is now redundant since we're using theuseUserhook to get the user's name dynamically. This should be removed to prevent confusion and potential bugs.- const Admin = { - name: 'AdminName', - };
Line range hint
13-31: Enhance error handling and add loading state management.The current implementation could benefit from the following improvements:
- Add loading state management for better UX
- Include authentication token in API requests
- Use antd's message system for user-friendly error notifications
const Admin = () => { + const [loading, setLoading] = useState(false); // ... const fetchData = async () => { + setLoading(true); try { const response = await fetch( `${import.meta.env.VITE_BACKEND_URL}/api/event/all`, { method: 'GET', headers: { 'Content-Type': 'application/json', + 'Authorization': `Bearer ${localStorage.getItem('authToken')}`, }, } ); + if (!response.ok) { + throw new Error('Failed to fetch events'); + } const data = await response.json(); setEvents(data); } catch (error) { setError(error.message); + message.error('Failed to fetch events: ' + error.message); console.error('Error fetching events:', error); + } finally { + setLoading(false); } };
Line range hint
94-116: Fix critical issues in event deletion logic.There are several issues that need to be addressed:
- The success message is shown when
!response.ok(incorrect logic)- Using GET method for deletion violates REST principles
- Missing user confirmation before deletion
const handleDelete = async (eventId) => { + if (!confirm('Are you sure you want to delete this event?')) { + return; + } try { const response = await fetch( `${import.meta.env.VITE_BACKEND_URL}/api/event/delete?id=${eventId}`, { - method: 'get', + method: 'DELETE', headers: { 'Content-Type': 'application/json', + 'Authorization': `Bearer ${localStorage.getItem('authToken')}`, }, } ); const data = await response.json(); - if (!response.ok) { + if (response.ok) { message.success('Event deleted successfully'); - navigate('/admin'); + // Filter out the deleted event from the list + setEvents((prevEvents) => + prevEvents.filter((event) => event._id !== eventId) + ); + } else { + throw new Error(data.message || 'Failed to delete event'); } - - // Filter out the deleted event from the list - setEvents((prevEvents) => - prevEvents.filter((event) => event._id !== eventId) - ); } catch (error) { console.error('Error deleting event:', error); + message.error('Failed to delete event: ' + error.message); setError(error.message); } };
Line range hint
65-89: Enhance event creation validation and error handling.The event creation logic needs additional validation and improved error handling:
- Validate that start time is before end time
- Add date validation (no past dates)
- Improve success flow
const handleSubmit = async (e) => { e.preventDefault(); + // Validate times + const [startHour, startMin] = formData.startTime.split(':').map(Number); + const [endHour, endMin] = formData.endTime.split(':').map(Number); + if (startHour > endHour || (startHour === endHour && startMin >= endMin)) { + message.error('Start time must be before end time'); + return; + } + + // Validate date + const eventDate = new Date(formData.date); + if (eventDate < new Date().setHours(0, 0, 0, 0)) { + message.error('Cannot create events in the past'); + return; + } + const formattedTime = `${formatTime(formData.startTime)} - ${formatTime(formData.endTime)}`; const { title, description, ageRange, date } = formData; const event = { title, description, ageRange, date, time: formattedTime }; - console.log(event); + try { const response = await fetch( `${import.meta.env.VITE_BACKEND_URL}/api/event/create`, { method: 'POST', headers: { 'Content-Type': 'application/json', + 'Authorization': `Bearer ${localStorage.getItem('authToken')}`, }, body: JSON.stringify(event), } ); const data = await response.json(); + if (!response.ok) { + throw new Error(data.message || 'Failed to create event'); + } - if (response.ok) { - message.success('Event added successfully'); - navigate('/admin'); - fetchData(); - } - console.log(data); + message.success('Event added successfully'); + setFormData({ + title: '', + description: '', + ageRange: '', + startTime: '', + endTime: '', + date: '', + }); + fetchData(); } catch (error) { console.error('Error adding event:', error); - message.error('Error adding event'); + message.error('Failed to create event: ' + error.message); } };
Line range hint
245-268: Improve UI accessibility and performance.The event listing section needs several improvements:
- Add loading states
- Optimize images
- Enhance accessibility
<div className="event-list"> + {loading && <div className="text-center"><Spin size="large" /></div>} {error && <p className="text-red-500">{error}</p>} - {events.map((event) => ( + {!loading && events.map((event) => ( <div key={event._id} + role="article" + aria-label={`Event: ${event.title}`} className="grid grid-cols-1 md:grid-cols-2 gap-10 lg:grid-cols-1 xl:grid-cols-2 md:px-6 lg:px-4 xl:px-0" > <div className="w-full m-10 mx-auto lg:mx-0 md:mx-0"> <img src={event.image} alt={event.title} loading='lazy' + width="800" + height="400" className="h-[400px] w-full" /> </div> <div className="w-full lg:m-10 md:m-10"> <h1 className="text-4xl font-semibold">{event.title}</h1> <button onClick={() => handleDelete(event._id)} + aria-label={`Delete event: ${event.title}`} className="btn btn-primary bg-[#f81b1b] p-4 rounded-xl text-md font-medium mt-4 hover:scale-110 transition text-white" > Remove Event </button>
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (13)
backend/controller/admin.controller.js(1 hunks)backend/controller/customer.controller.js(1 hunks)frontend/src/components/Pages/Admin.jsx(2 hunks)frontend/src/components/Pages/Admin/AdminLogin.jsx(1 hunks)frontend/src/components/Pages/Admin/AdminSignup.jsx(1 hunks)frontend/src/components/Pages/Login.jsx(1 hunks)frontend/src/components/Pages/Signup.jsx(1 hunks)frontend/src/components/Pages/VerifyOtp.jsx(1 hunks)frontend/src/components/Shared/Navbar.jsx(1 hunks)frontend/src/context/userContext.jsx(1 hunks)frontend/src/main.jsx(1 hunks)frontend/src/router/ProtectedRoute.jsx(1 hunks)frontend/src/router/index.jsx(2 hunks)
✅ Files skipped from review due to trivial changes (1)
- frontend/src/components/Pages/Signup.jsx
🔇 Additional comments (8)
frontend/src/router/ProtectedRoute.jsx (1)
8-8: Verify route protection implementation
Let's verify that this protected route is properly integrated with the router configuration.
✅ Verification successful
Let me gather more information about how ProtectedRoute is being used in the router configuration.
Route protection is correctly implemented
The ProtectedRoute component is properly integrated in the router configuration:
- It's correctly imported and used to protect the
/adminroute - The component redirects to
/admin-loginfor non-admin users, which exists as a valid route - The protection is specifically applied to the admin dashboard while keeping other routes accessible
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for usage of ProtectedRoute in router configuration
rg -l "ProtectedRoute" "frontend/src/router/"
# Check if admin routes are properly protected
ast-grep --pattern 'Route $_ element={<ProtectedRoute>$$$</ProtectedRoute>}'
Length of output: 191
Script:
#!/bin/bash
# Check the router configuration in index.jsx
cat frontend/src/router/index.jsx
# Also check for any admin-related routes
rg -A 5 "admin" frontend/src/router/index.jsx
Length of output: 3089
frontend/src/main.jsx (1)
6-6: LGTM! Clean implementation of global user context.
The UserProvider is correctly positioned at the root level, ensuring user context is available throughout the application. This implementation aligns well with the PR objectives for admin authentication.
Also applies to: 10-12
frontend/src/router/index.jsx (2)
25-27: LGTM! Imports are properly organized.
The new imports for admin authentication components follow the existing naming conventions and organization pattern.
42-49: Protected route implementation looks good, but verify the protection logic.
The admin route is correctly wrapped with the ProtectedRoute component, implementing the required access control.
Let's verify the ProtectedRoute implementation:
#!/bin/bash
# Description: Verify ProtectedRoute implementation for proper role checking
# Check ProtectedRoute implementation
echo "Checking ProtectedRoute implementation..."
ast-grep --pattern 'export $_ function ProtectedRoute' -A 10
# Look for role checking logic
echo "Checking for role verification..."
rg -l "role.*admin" --type jsxfrontend/src/components/Pages/VerifyOtp.jsx (1)
Line range hint 22-45: Verify integration with admin authentication flow.
The OTP verification logic appears to be generic and not specifically integrated with the admin authentication flow mentioned in the PR objectives. Please verify:
- Is this component meant to be shared between admin and regular user flows?
- Should there be admin-specific handling in the verification process?
Let's check how this component is used in the admin flow:
backend/controller/customer.controller.js (1)
146-146: LGTM: Role addition aligns with PR objectives.
The addition of the role field in the response object is consistent with the PR's goal of implementing role-based authentication.
frontend/src/components/Pages/Admin/AdminLogin.jsx (1)
1-9: LGTM! Dependencies are well organized.
The imports are appropriate for the component's functionality, showing good separation of concerns with context usage.
frontend/src/components/Pages/Admin/AdminSignup.jsx (1)
66-66: Clarify OTP implementation status.
The commented-out OTP alert suggests incomplete implementation of a security feature.
Let's check if OTP verification is implemented elsewhere:
Would you like help implementing the OTP verification flow?
✅ Verification successful
OTP implementation is fully functional - commented alert can be removed
The OTP verification system is completely implemented with:
- Frontend components for OTP verification (
VerifyOtp.jsxandverifyRegisterOtp.jsx) - Backend routes and controllers handling OTP generation, verification, and email delivery
- Proper email verification flow with 6-digit OTP and 5-minute expiry
The commented-out alert in AdminSignup.jsx can be safely removed as it's redundant with the existing implementation.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for OTP-related code
rg -i "otp" --type js --type jsx
Length of output: 60
Script:
#!/bin/bash
# Search for OTP-related code with ripgrep, using all text files
rg -i "otp" -t txt -t js -t ts -t tsx
# Also search for email verification related code since OTP is often used for email verification
rg -i "email.*verif|verif.*email" -t txt -t js -t ts -t tsx
# Look for authentication or verification related files
fd -e js -e jsx -e ts -e tsx . | grep -i "auth\|verif"
Length of output: 499
Script:
#!/bin/bash
# Check the content of OTP verification related files
cat frontend/src/components/Pages/VerifyOtp.jsx frontend/src/components/Pages/verifyRegisterOtp.jsx
# Check the content of email verification file
cat frontend/src/components/Pages/EmailVerify.jsx
# Look for OTP related routes or API endpoints
rg -i "otp" --type js backend/
Length of output: 14007
| const ProtectedRoute = ({ children }) => { | ||
| const {user} = useUser(); | ||
|
|
||
| return user.role == "admin" ? children : <Navigate to="/admin-login" />; |
There was a problem hiding this comment.
Fix security vulnerability in role comparison
The loose equality operator (==) can lead to unexpected type coercion. Use strict equality (===) for role comparison.
- return user.role == "admin" ? children : <Navigate to="/admin-login" />;
+ return user.role === "admin" ? children : <Navigate to="/admin-login" />;📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| return user.role == "admin" ? children : <Navigate to="/admin-login" />; | |
| return user.role === "admin" ? children : <Navigate to="/admin-login" />; |
| const ProtectedRoute = ({ children }) => { | ||
| const {user} = useUser(); | ||
|
|
||
| return user.role == "admin" ? children : <Navigate to="/admin-login" />; | ||
| }; |
There was a problem hiding this comment.
Add null checks and loading state handling
The component doesn't handle cases where the user object might be null/undefined or when authentication is still loading. This could lead to runtime errors.
-const ProtectedRoute = ({ children }) => {
- const {user} = useUser();
+const ProtectedRoute = ({ children }) => {
+ const { user, isLoading } = useUser();
+
+ if (isLoading) {
+ return <div>Loading...</div>; // Consider using a proper loading component
+ }
+
+ if (!user) {
+ return <Navigate to="/admin-login" />;
+ }
- return user.role === "admin" ? children : <Navigate to="/admin-login" />;
+ return user.role === "admin" ? children : <Navigate to="/admin-login" />;📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const ProtectedRoute = ({ children }) => { | |
| const {user} = useUser(); | |
| return user.role == "admin" ? children : <Navigate to="/admin-login" />; | |
| }; | |
| const ProtectedRoute = ({ children }) => { | |
| const { user, isLoading } = useUser(); | |
| if (isLoading) { | |
| return <div>Loading...</div>; // Consider using a proper loading component | |
| } | |
| if (!user) { | |
| return <Navigate to="/admin-login" />; | |
| } | |
| return user.role === "admin" ? children : <Navigate to="/admin-login" />; | |
| }; |
| ReactDOM.createRoot(document.getElementById('root')).render( | ||
| <React.StrictMode> | ||
| <RouterProvider router={router} /> | ||
| <UserProvider> | ||
| <RouterProvider router={router} /> | ||
| </UserProvider> | ||
| </React.StrictMode> | ||
| ); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider adding error boundaries for improved reliability.
To prevent authentication-related errors from crashing the entire application, consider wrapping the UserProvider with an error boundary component.
Here's a suggested implementation:
+ import { ErrorBoundary } from 'react-error-boundary';
ReactDOM.createRoot(document.getElementById('root')).render(
<React.StrictMode>
+ <ErrorBoundary fallback={<div>Something went wrong with authentication. Please refresh.</div>}>
<UserProvider>
<RouterProvider router={router} />
</UserProvider>
+ </ErrorBoundary>
</React.StrictMode>
);Committable suggestion was skipped due to low confidence.
frontend/src/context/userContext.jsx
Outdated
| import React, { createContext, useState, useContext } from 'react'; | ||
| import Cookies from 'js-cookie'; | ||
|
|
||
| const UserContext = createContext(); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider adding a default value and exporting the context.
The context creation could be improved for better maintainability and type safety.
import React, { createContext, useState, useContext } from 'react';
import Cookies from 'js-cookie';
-const UserContext = createContext();
+export const UserContext = createContext({
+ user: null,
+ setUser: () => {},
+});📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| import React, { createContext, useState, useContext } from 'react'; | |
| import Cookies from 'js-cookie'; | |
| const UserContext = createContext(); | |
| import React, { createContext, useState, useContext } from 'react'; | |
| import Cookies from 'js-cookie'; | |
| export const UserContext = createContext({ | |
| user: null, | |
| setUser: () => {}, | |
| }); |
frontend/src/context/userContext.jsx
Outdated
| ); | ||
| }; | ||
|
|
||
| export const useUser = () => useContext(UserContext); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add context validation in useUser hook.
The custom hook should validate context usage and provide helpful error messages.
-export const useUser = () => useContext(UserContext);
+export const useUser = () => {
+ const context = useContext(UserContext);
+ if (context === undefined) {
+ throw new Error('useUser must be used within a UserProvider');
+ }
+ return context;
+};📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| export const useUser = () => useContext(UserContext); | |
| export const useUser = () => { | |
| const context = useContext(UserContext); | |
| if (context === undefined) { | |
| throw new Error('useUser must be used within a UserProvider'); | |
| } | |
| return context; | |
| }; |
| const getPasswordStrengthText = (score) => { | ||
| const strengthLevels = ['Very Weak', 'Weak', 'Okay', 'Good', 'Strong']; | ||
| return strengthLevels[score]; | ||
| }; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Enhance password strength feedback.
The current strength indicators could be more informative to help users create better passwords.
Consider enhancing the feedback:
const getPasswordStrengthText = (score) => {
- const strengthLevels = ['Very Weak', 'Weak', 'Okay', 'Good', 'Strong'];
+ const strengthLevels = [
+ 'Very Weak - Add more characters',
+ 'Weak - Try adding numbers',
+ 'Okay - Consider special characters',
+ 'Good - Length is important too',
+ 'Strong - Excellent password!'
+ ];
return strengthLevels[score];
};📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const getPasswordStrengthText = (score) => { | |
| const strengthLevels = ['Very Weak', 'Weak', 'Okay', 'Good', 'Strong']; | |
| return strengthLevels[score]; | |
| }; | |
| const getPasswordStrengthText = (score) => { | |
| const strengthLevels = [ | |
| 'Very Weak - Add more characters', | |
| 'Weak - Try adding numbers', | |
| 'Okay - Consider special characters', | |
| 'Good - Length is important too', | |
| 'Strong - Excellent password!' | |
| ]; | |
| return strengthLevels[score]; | |
| }; |
| try { | ||
| const response = await fetch(`${API_URL}/api/admin/register`, { | ||
| method: 'POST', | ||
| headers: { 'Content-Type': 'application/json' }, | ||
| body: JSON.stringify(data), | ||
| }); | ||
| const result = await response.json(); | ||
|
|
||
| if (!response.ok) { | ||
| setIsLoading(false); | ||
| setError(result.error); | ||
| return; | ||
| } | ||
|
|
There was a problem hiding this comment.
Enhance API request security.
The API request lacks CSRF protection and rate limiting considerations.
Consider these security enhancements:
try {
const response = await fetch(`${API_URL}/api/admin/register`, {
method: 'POST',
headers: {
'Content-Type': 'application/json',
+ 'X-CSRF-Token': document.querySelector('meta[name="csrf-token"]').content
},
credentials: 'include',
body: JSON.stringify(data),
});Committable suggestion was skipped due to low confidence.
| <a href={`${API_URL}/api/user/auth/google`} className="w-full"> | ||
| <button className="button-confirm w-full h-10 rounded-md border-2 border-black bg-beige text-[17px] font-semibold shadow-[4px_4px_0px_0px_black] hover:text-green-300"> | ||
| Sign up with Google | ||
| </button> | ||
| </a> |
There was a problem hiding this comment.
Enhance Google authentication security and UX.
The Google authentication link lacks CSRF protection and loading state handling.
Consider these improvements:
-<a href={`${API_URL}/api/user/auth/google`} className="w-full">
+<a
+ href={`${API_URL}/api/user/auth/google?csrf=${document.querySelector('meta[name="csrf-token"]').content}`}
+ className="w-full"
+ onClick={(e) => {
+ e.preventDefault();
+ setIsLoading(true);
+ window.location.href = e.target.href;
+ }}
+>
<button className="button-confirm w-full h-10 rounded-md border-2 border-black bg-beige text-[17px] font-semibold shadow-[4px_4px_0px_0px_black] hover:text-green-300">
- Sign up with Google
+ {isLoading ? 'Redirecting...' : 'Sign up with Google'}
</button>
</a>Committable suggestion was skipped due to low confidence.
| if (!data.email.includes('@')) { | ||
| setError('Please enter a valid email address'); | ||
| setIsLoading(false); | ||
| return; | ||
| } |
There was a problem hiding this comment.
Enhance email validation security.
The current email validation is too basic and could allow invalid email formats.
Consider using a proper email validation regex or a validation library:
-if (!data.email.includes('@')) {
+const emailRegex = /^[^\s@]+@[^\s@]+\.[^\s@]+$/;
+if (!emailRegex.test(data.email)) {
setError('Please enter a valid email address');
setIsLoading(false);
return;
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if (!data.email.includes('@')) { | |
| setError('Please enter a valid email address'); | |
| setIsLoading(false); | |
| return; | |
| } | |
| const emailRegex = /^[^\s@]+@[^\s@]+\.[^\s@]+$/; | |
| if (!emailRegex.test(data.email)) { | |
| setError('Please enter a valid email address'); | |
| setIsLoading(false); | |
| return; | |
| } |
| @@ -55,6 +55,7 @@ const Navbar = () => { | |||
| }) | |||
|
|
|||
| Cookies.remove('authToken'); | |||
| Cookies.remove("authenticatedUser"); | |||
There was a problem hiding this comment.
Enhance cookie removal security during logout.
While adding authenticatedUser cookie removal is good, consider these security improvements:
- Set secure attributes when removing cookies
- Clear all auth-related cookies in one operation
Apply this enhancement:
- Cookies.remove("authenticatedUser");
+ Cookies.remove("authToken", { secure: true, sameSite: 'strict' });
+ Cookies.remove("authenticatedUser", { secure: true, sameSite: 'strict' });📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| Cookies.remove("authenticatedUser"); | |
| Cookies.remove("authToken", { secure: true, sameSite: 'strict' }); | |
| Cookies.remove("authenticatedUser", { secure: true, sameSite: 'strict' }); |
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Outside diff range and nitpick comments (1)
frontend/src/router/index.jsx (1)
43-52: Consider implementing additional security measuresWhile the current implementation provides basic protection, consider these additional security measures:
- Implement rate limiting for admin login attempts
- Add CSRF protection for admin routes
- Consider implementing session management for admin sessions
- Add audit logging for admin actions
- Consider implementing 2FA for admin accounts
These measures would enhance the security of the admin functionality.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (4)
backend/controller/customer.controller.js(1 hunks)frontend/src/components/Pages/Login.jsx(2 hunks)frontend/src/components/Shared/Navbar.jsx(1 hunks)frontend/src/router/index.jsx(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- backend/controller/customer.controller.js
- frontend/src/components/Pages/Login.jsx
- frontend/src/components/Shared/Navbar.jsx
🔇 Additional comments (1)
frontend/src/router/index.jsx (1)
25-27: LGTM: Required imports added correctly
The new imports are properly structured and align with the PR objectives for implementing admin authentication.
| <Route | ||
| path="/admin" | ||
| element={ | ||
| <ProtectedRoute> | ||
| <Admin /> | ||
| </ProtectedRoute> | ||
| } | ||
| /> |
There was a problem hiding this comment.
💡 Codebase verification
Security concern: Loose equality check in admin role validation
The ProtectedRoute implementation has a potential security issue using loose equality (==) instead of strict equality (===) when checking the admin role. While it works, it's not following security best practices and could lead to unexpected behavior.
frontend/src/router/ProtectedRoute.jsx: Changeuser.role == "admin"touser.role === "admin"
🔗 Analysis chain
Verify ProtectedRoute implementation for admin role check
The admin route is now protected, which is good. Let's verify the implementation of the ProtectedRoute component.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify ProtectedRoute implementation for admin authentication
# Check ProtectedRoute component implementation
echo "Checking ProtectedRoute implementation..."
rg -A 10 "function ProtectedRoute|const ProtectedRoute" frontend/src/router/
# Check for admin role validation in ProtectedRoute
echo "Checking admin role validation..."
rg -A 5 "role.*admin|isAdmin" frontend/src/router/ProtectedRoute
# Check for proper redirection logic
echo "Checking redirect implementation..."
rg -A 3 "navigate|redirect" frontend/src/router/ProtectedRoute
Length of output: 1066
Script:
#!/bin/bash
# Let's verify the actual path of ProtectedRoute component and its implementation
echo "Finding ProtectedRoute component location..."
fd -e jsx -e js ProtectedRoute
# Once found, let's check its content
echo "Checking component implementation..."
rg -A 10 "function ProtectedRoute|const ProtectedRoute"
# Let's also check for any admin role checks in the entire frontend
echo "Checking admin role validations..."
rg -A 5 "role.*admin|isAdmin" frontend/src/
Length of output: 1157
| <Route path="/admin-login" element={<AdminLogin />} /> | ||
| <Route path="/admin-signup" element={<AdminSignup />} /> |
There was a problem hiding this comment.
💡 Codebase verification
Admin authentication implementation needs role-based security enhancements
The current implementation has several security concerns:
- No explicit admin role validation in the authentication flow
- Common user context is used for both admin and regular users without role distinction
- Admin authentication endpoints (
/api/admin/loginand/api/admin/register) exist but no role-based guards - Google OAuth integration lacks admin-specific handling
Required changes:
- Add admin role validation in the authentication response
- Implement separate admin context or extend user context with role information
- Add route guards to prevent regular users from accessing admin routes
- Remove Google OAuth from admin authentication as it bypasses admin validation
🔗 Analysis chain
Verify admin-specific authentication handling
The routes are correctly added, but we need to ensure proper admin-specific authentication handling in the components.
Let me gather more information about the admin authentication implementation.
This will help us verify if there's proper admin-specific authentication logic and role validation in place.
Let me check the actual implementation of the admin components and user context to verify the authentication handling.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify admin-specific authentication implementation
# Check admin role handling in login/signup
echo "Checking admin role handling in authentication..."
rg -A 5 "role.*admin|isAdmin" frontend/src/components/Pages/Admin/Admin{Login,Signup}.jsx
# Check for proper error handling
echo "Checking error handling..."
rg -A 5 "catch|error" frontend/src/components/Pages/Admin/Admin{Login,Signup}.jsx
# Check for proper state management
echo "Checking authentication state management..."
rg -A 5 "useState|useContext|useAuth" frontend/src/components/Pages/Admin/Admin{Login,Signup}.jsx
Length of output: 7706
Script:
#!/bin/bash
# Check admin-specific API endpoints and authentication logic
echo "Checking admin API endpoints and authentication..."
rg -A 5 "admin.*login|admin.*signup|admin.*auth" frontend/src/api/
# Check admin role validation in components
echo "Checking admin role validation..."
rg -A 5 "admin.*role|role.*admin|isAdmin" frontend/src/api/
# Check admin authentication state handling
echo "Checking admin auth state..."
rg -A 5 "useUser|AuthContext|admin.*context" frontend/src/context/
Length of output: 661
Script:
#!/bin/bash
# Check admin component implementations
echo "Checking AdminLogin implementation..."
cat frontend/src/components/Pages/Admin/AdminLogin.jsx
echo "Checking AdminSignup implementation..."
cat frontend/src/components/Pages/Admin/AdminSignup.jsx
# Check user context implementation
echo "Checking UserContext implementation..."
cat frontend/src/context/userContext.jsx
Length of output: 12474
|
@Sourabh782 resolve the code rabbit suggetions |
okk implementing |
There was a problem hiding this comment.
Actionable comments posted: 13
🧹 Outside diff range and nitpick comments (7)
frontend/src/router/ProtectedRoute.jsx (1)
1-3: Remove unused Route importThe
Routecomponent is imported but never used in this file.import React from 'react'; -import { Route, Navigate } from 'react-router-dom'; +import { Navigate } from 'react-router-dom'; import { useUser } from '../context/userContext';frontend/src/context/userContext.jsx (1)
1-35: Consider enhancing the authentication architecture.While the current implementation provides basic authentication, consider adding these architectural improvements:
- Implement a refresh token mechanism to handle token expiration
- Add session management utilities (e.g., idle timeout, force logout)
- Create role-based access control (RBAC) utilities to centralize permission checks
Would you like assistance in implementing any of these architectural improvements?
frontend/src/components/Pages/Admin/AdminLogin.jsx (1)
65-67: Consider enhancing scroll behavior.The current scroll implementation could be improved for a smoother user experience.
useEffect(() => { - window.scrollTo(0, 0); + window.scrollTo({ + top: 0, + behavior: 'smooth' + }); }, []);frontend/src/components/Pages/Admin.jsx (4)
Line range hint
37-39: Remove redundant Admin object.The
Adminobject is no longer needed since we're now using theuseUserhook to get the user's name dynamically.- const Admin = { - name: 'AdminName', - };
Line range hint
95-116: Fix critical bug in handleDelete function.There are several issues in the delete event handler:
- Success message is shown when the response is NOT ok (incorrect condition)
- Navigation occurs on failure
- Error handling could be improved with user feedback
Apply this fix:
const handleDelete = async (eventId) => { try { const response = await fetch( `${import.meta.env.VITE_BACKEND_URL}/api/event/delete?id=${eventId}`, { - method: 'get', + method: 'DELETE', headers: { 'Content-Type': 'application/json', }, } ); const data = await response.json(); - if (!response.ok) { + if (response.ok) { message.success('Event deleted successfully'); - navigate('/admin'); + // Filter out the deleted event from the list + setEvents((prevEvents) => + prevEvents.filter((event) => event._id !== eventId) + ); + } else { + message.error(data.message || 'Failed to delete event'); } - - // Filter out the deleted event from the list - setEvents((prevEvents) => - prevEvents.filter((event) => event._id !== eventId) - ); } catch (error) { console.error('Error deleting event:', error); - setError(error.message); + message.error('Error deleting event: ' + error.message); } };
Line range hint
66-89: Enhance form submission handling and user feedback.The form submission could benefit from better user feedback and validation:
- Add loading state during submission
- Improve input validation
- Consistent error handling
Consider implementing these improvements:
const handleSubmit = async (e) => { e.preventDefault(); + // Add form validation + if (new Date(formData.date) < new Date()) { + message.error('Event date cannot be in the past'); + return; + } + + // Add loading state + const [isSubmitting, setIsSubmitting] = useState(false); + setIsSubmitting(true); + const formattedTime = `${formatTime(formData.startTime)} - ${formatTime(formData.endTime)}`; const { title, description, ageRange, date } = formData; const event = { title, description, ageRange, date, time: formattedTime }; - console.log(event); try { const response = await fetch( `${import.meta.env.VITE_BACKEND_URL}/api/event/create`, { method: 'POST', headers: { 'Content-Type': 'application/json', }, body: JSON.stringify(event), } ); const data = await response.json(); if (response.ok) { message.success('Event added successfully'); - navigate('/admin'); fetchData(); + setFormData({ + title: '', + description: '', + ageRange: '', + startTime: '', + endTime: '', + date: '', + }); + } else { + message.error(data.message || 'Failed to add event'); } - console.log(data); } catch (error) { console.error('Error adding event:', error); message.error('Error adding event: ' + error.message); + } finally { + setIsSubmitting(false); } };Also update the submit button to show loading state:
<button type="submit" + disabled={isSubmitting} className="w-full bg-blue-500 text-white font-bold py-2 px-4 rounded-md hover:bg-blue-600 disabled:bg-gray-400" > - Submit + {isSubmitting ? 'Submitting...' : 'Submit'} </button>
Security improvements needed for Admin component API interactions
The verification reveals several security gaps that need to be addressed:
- Missing Authorization headers in Admin component API calls while other components like Dashboard implement them properly
- No CSRF protection implemented across the application
- No rate limiting for form submissions
- Admin route protection relies on client-side role check that could be bypassed
Required changes:
- Add
Authorization: Bearer ${token}header to all API calls in Admin.jsx- Implement CSRF token validation
- Add request throttling for form submissions
- Move role-based access control to the backend
const fetchData = async () => { try { const response = await fetch( `${import.meta.env.VITE_BACKEND_URL}/api/event/all`, { method: 'GET', headers: { 'Content-Type': 'application/json', + 'Authorization': `Bearer ${user?.token}`, + 'X-CSRF-Token': document.querySelector('meta[name="csrf-token"]').content }, } );🔗 Analysis chain
Line range hint
14-31: Address security concerns in API interactions.Several security improvements are recommended for the admin panel:
- Add CSRF protection to API calls
- Implement rate limiting for form submissions
- Add authorization token to API requests
- Validate file uploads (if implemented later)
Consider these security enhancements:
+ const authToken = localStorage.getItem('adminToken'); const fetchData = async () => { try { const response = await fetch( `${import.meta.env.VITE_BACKEND_URL}/api/event/all`, { method: 'GET', headers: { 'Content-Type': 'application/json', + 'Authorization': `Bearer ${authToken}`, + 'X-CSRF-Token': document.querySelector('meta[name="csrf-token"]').content }, } );Apply similar changes to
handleSubmitandhandleDeletefunctions. Also consider implementing request throttling on the frontend to prevent rapid-fire submissions.Let's verify the current security measures:
Also applies to: 66-89, 95-116
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Check for proper authorization headers in API calls rg -A 5 "fetch\(" frontend/src/ # Check for CSRF protection rg -l "csrf" frontend/src/Length of output: 8723
Script:
#!/bin/bash # Check for authentication token usage in API calls rg -A 5 "Authorization.*Bearer" frontend/src/ # Check for any authentication-related code in Admin component rg -A 10 "localStorage.getItem.*Token|useAuth" frontend/src/components/Pages/Admin.jsx # Check if there's any rate limiting implementation rg -A 5 "rate|throttle|limit" frontend/src/components/Pages/Admin.jsx # Check for any authentication hooks or context fd -e jsx -e js -e ts -e tsx -p "auth|protect" frontend/src/Length of output: 732
Script:
#!/bin/bash # Check the implementation of ProtectedRoute cat frontend/src/router/ProtectedRoute.jsx # Check if there's any authentication context provider rg -l "AuthContext|AuthProvider" frontend/src/ # Check how admin routes are protected in router configuration rg -A 10 "path.*admin" frontend/src/Length of output: 1685
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (7)
backend/controller/admin.controller.js(1 hunks)frontend/src/components/Pages/Admin.jsx(3 hunks)frontend/src/components/Pages/Admin/AdminLogin.jsx(1 hunks)frontend/src/components/Pages/Admin/AdminSignup.jsx(1 hunks)frontend/src/components/Pages/Login.jsx(2 hunks)frontend/src/context/userContext.jsx(1 hunks)frontend/src/router/ProtectedRoute.jsx(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- backend/controller/admin.controller.js
- frontend/src/components/Pages/Login.jsx
🔇 Additional comments (2)
frontend/src/router/ProtectedRoute.jsx (1)
5-13: Verify authentication token validity
The current implementation doesn't verify if the user's authentication token is still valid. An expired token could lead to unauthorized access.
Consider implementing token validation in the useUser hook:
- Check token expiration
- Implement token refresh mechanism
- Clear user data and redirect on token expiration
frontend/src/components/Pages/Admin.jsx (1)
4-4: LGTM: User context integration.
The integration of the user context is well implemented with proper import and hook usage.
Also applies to: 10-11
| const ProtectedRoute = ({ children }) => { | ||
| const {user} = useUser(); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add prop validation
Consider adding PropTypes or TypeScript to validate the children prop and improve type safety.
+import PropTypes from 'prop-types';
import React from 'react';
const ProtectedRoute = ({ children }) => {
const {user} = useUser();
// ... rest of the component
};
+ProtectedRoute.propTypes = {
+ children: PropTypes.node.isRequired,
+};
export default ProtectedRoute;Committable suggestion skipped: line range outside the PR's diff.
| return <Navigate to="/admin-login" />; | ||
| } | ||
|
|
||
| return user.role === "admin" ? children : <Navigate to="/admin-login" />; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Enhance role checking and configuration
Consider the following improvements for better maintainability and flexibility:
- Extract magic strings to constants
- Make redirect path configurable via props
- Add case-insensitive role comparison
+const ADMIN_ROLE = 'admin';
+const DEFAULT_REDIRECT = '/admin-login';
-const ProtectedRoute = ({ children }) => {
+const ProtectedRoute = ({ children, redirectPath = DEFAULT_REDIRECT }) => {
const {user} = useUser();
if (!user) {
- return <Navigate to="/admin-login" />;
+ return <Navigate to={redirectPath} />;
}
- return user.role === "admin" ? children : <Navigate to="/admin-login" />;
+ return user.role?.toLowerCase() === ADMIN_ROLE ?
+ children :
+ <Navigate to={redirectPath} />;
};Committable suggestion skipped: line range outside the PR's diff.
| export const UserProvider = ({ children }) => { | ||
| const data = Cookies.get("authenticatedUser"); | ||
|
|
||
| let userData = null; | ||
| try { | ||
| userData = data ? JSON.parse(data) : null; | ||
| } catch (error) { | ||
| console.error('Invalid user data in cookie:', error); | ||
| Cookies.remove("authenticatedUser"); | ||
| } | ||
|
|
||
| const [user, setUser] = useState(userData); | ||
|
|
||
| return ( | ||
| <UserContext.Provider value={{ user, setUser }}> | ||
| {children} | ||
| </UserContext.Provider> | ||
| ); | ||
| }; |
There was a problem hiding this comment.
Enhance security measures in UserProvider implementation.
The current implementation needs additional security measures to protect against common web vulnerabilities:
- Add cookie security options when setting cookies
- Validate user data structure before setting state
- Implement CSRF protection
Here's how to enhance the implementation:
export const UserProvider = ({ children }) => {
const data = Cookies.get("authenticatedUser");
let userData = null;
try {
userData = data ? JSON.parse(data) : null;
+ // Validate user data structure
+ if (userData && !isValidUserData(userData)) {
+ throw new Error('Invalid user data structure');
+ }
} catch (error) {
console.error('Invalid user data in cookie:', error);
Cookies.remove("authenticatedUser");
}
const [user, setUser] = useState(userData);
+ // Wrap setUser to ensure cookie options
+ const setUserWithCookie = (newUser) => {
+ setUser(newUser);
+ if (newUser) {
+ Cookies.set("authenticatedUser", JSON.stringify(newUser), {
+ secure: true,
+ sameSite: 'strict',
+ expires: 1, // 1 day
+ });
+ } else {
+ Cookies.remove("authenticatedUser");
+ }
+ }
return (
- <UserContext.Provider value={{ user, setUser }}>
+ <UserContext.Provider value={{ user, setUser: setUserWithCookie }}>
{children}
</UserContext.Provider>
);
};
+// Add this helper function
+function isValidUserData(data) {
+ return (
+ data &&
+ typeof data === 'object' &&
+ typeof data.id === 'string' &&
+ typeof data.role === 'string' &&
+ ['admin', 'user'].includes(data.role)
+ );
+}Committable suggestion skipped: line range outside the PR's diff.
| </button> | ||
| </Link> | ||
|
|
||
| {error && <p className="text-red-500 mt-2">{error}</p>} |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Improve error message accessibility and visibility.
The error message display could be enhanced for better accessibility and user experience.
-{error && <p className="text-red-500 mt-2">{error}</p>}
+{error && (
+ <div
+ role="alert"
+ aria-live="polite"
+ className="p-3 bg-red-100 border border-red-400 text-red-700 rounded relative"
+ >
+ <span className="block sm:inline">{error}</span>
+ </div>
+)}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| {error && <p className="text-red-500 mt-2">{error}</p>} | |
| {error && ( | |
| <div | |
| role="alert" | |
| aria-live="polite" | |
| className="p-3 bg-red-100 border border-red-400 text-red-700 rounded relative" | |
| > | |
| <span className="block sm:inline">{error}</span> | |
| </div> | |
| )} |
| if (!response.ok) { | ||
| throw new Error(result.message || 'Login failed'); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Enhance error handling with more specific error messages.
The current error handling could be more informative by providing specific error messages based on different error scenarios.
if (!response.ok) {
- throw new Error(result.message || 'Login failed');
+ const errorMessage = {
+ 401: 'Invalid email or password',
+ 403: 'Account not verified',
+ 429: 'Too many login attempts. Please try again later',
+ }[response.status] || result.message || 'Login failed';
+ throw new Error(errorMessage);
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if (!response.ok) { | |
| throw new Error(result.message || 'Login failed'); | |
| } | |
| if (!response.ok) { | |
| const errorMessage = { | |
| 401: 'Invalid email or password', | |
| 403: 'Account not verified', | |
| 429: 'Too many login attempts. Please try again later', | |
| }[response.status] || result.message || 'Login failed'; | |
| throw new Error(errorMessage); | |
| } |
| const handleSubmit = async (e) => { | ||
| e.preventDefault(); | ||
| setIsLoading(true); | ||
|
|
||
| // Input validation | ||
| if (!data.email || !data.password || !data.name) { | ||
| setError('Please fill in all fields'); | ||
| setIsLoading(false); | ||
| return; | ||
| } | ||
| if (data.password.length < 8) { | ||
| setError('Password must be at least 8 characters long'); | ||
| setIsLoading(false); | ||
| return; | ||
| } | ||
| if (data.name.length < 3) { | ||
| setError('Name must be at least 3 characters long'); | ||
| setIsLoading(false); | ||
| return; | ||
| } | ||
| const emailRegex = /^[^\s@]+@[^\s@]+\.[^\s@]+$/; | ||
| if (!emailRegex.test(data.email)) { | ||
| setError('Please enter a valid email address'); | ||
| setIsLoading(false); | ||
| return; | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Centralize validation logic for better maintainability.
The validation logic is scattered throughout the handleSubmit function. Consider extracting it into a separate utility function.
const validateFormData = (data) => {
const errors = [];
if (!data.email || !data.password || !data.name) {
errors.push('Please fill in all fields');
}
if (data.password.length < 8) {
errors.push('Password must be at least 8 characters long');
}
if (data.name.length < 3) {
errors.push('Name must be at least 3 characters long');
}
const emailRegex = /^[^\s@]+@[^\s@]+\.[^\s@]+$/;
if (!emailRegex.test(data.email)) {
errors.push('Please enter a valid email address');
}
return errors;
};| try { | ||
| const response = await fetch(`${API_URL}/api/admin/register`, { | ||
| method: 'POST', | ||
| headers: { 'Content-Type': 'application/json' }, | ||
| body: JSON.stringify(data), | ||
| }); | ||
| const result = await response.json(); | ||
|
|
||
| if (!response.ok) { | ||
| setIsLoading(false); | ||
| setError(result.error); | ||
| return; | ||
| } | ||
|
|
||
|
|
||
| // alert('OTP sent to your email. Verify to complete registration.'); | ||
| navigate('/admin-login'); | ||
|
|
||
| } catch (error) { | ||
| setError(error.message); | ||
| console.error('Error:', error); | ||
| } finally { | ||
| setIsLoading(false); // Ensure loading state is reset after request | ||
| } | ||
| }; |
There was a problem hiding this comment.
Implement rate limiting for failed signup attempts.
As this is an admin signup endpoint, it should be protected against brute force attempts.
Consider implementing a client-side attempt counter:
const MAX_ATTEMPTS = 3;
const LOCKOUT_TIME = 15 * 60 * 1000; // 15 minutes
const [attempts, setAttempts] = useState(0);
const [lockoutUntil, setLockoutUntil] = useState(null);
// In handleSubmit, before API call:
if (lockoutUntil && Date.now() < lockoutUntil) {
setError(`Too many attempts. Please try again in ${Math.ceil((lockoutUntil - Date.now()) / 60000)} minutes`);
return;
}
// After failed attempt:
if (!response.ok) {
setAttempts(prev => {
if (prev + 1 >= MAX_ATTEMPTS) {
setLockoutUntil(Date.now() + LOCKOUT_TIME);
return 0;
}
return prev + 1;
});
}| useEffect(() => { | ||
| window.scrollTo(0, 0); | ||
| }, []); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Enhance scroll behavior accessibility.
The current scroll implementation might interfere with assistive technologies.
Consider using a more accessible approach:
useEffect(() => {
// Announce page load to screen readers
const pageTitle = document.title;
document.title = "Admin Signup - " + pageTitle;
// Smooth scroll to top
window.scrollTo({
top: 0,
behavior: 'smooth'
});
return () => {
document.title = pageTitle;
};
}, []);| <button | ||
| className="button-confirm w-full h-10 rounded-md border-2 border-black bg-beige text-[17px] font-semibold shadow-[4px_4px_0px_0px_black] mb-2 hover:text-green-300" | ||
| onClick={handleSubmit} | ||
| > | ||
| {isLoading ? 'Loading...' : "Let's go →"} | ||
| </button> |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Enhance loading state and form submission feedback.
The current loading state only updates the button text. Consider providing more comprehensive feedback.
<button
className="button-confirm w-full h-10 rounded-md border-2 border-black bg-beige text-[17px] font-semibold shadow-[4px_4px_0px_0px_black] mb-2 hover:text-green-300"
onClick={handleSubmit}
+ disabled={isLoading}
>
- {isLoading ? 'Loading...' : "Let's go →"}
+ {isLoading ? (
+ <span className="flex items-center justify-center">
+ <svg className="animate-spin h-5 w-5 mr-3" viewBox="0 0 24 24">
+ <circle className="opacity-25" cx="12" cy="12" r="10" stroke="currentColor" strokeWidth="4" fill="none" />
+ <path className="opacity-75" fill="currentColor" d="M4 12a8 8 0 018-8V0C5.373 0 0 5.373 0 12h4zm2 5.291A7.962 7.962 0 014 12H0c0 3.042 1.135 5.824 3 7.938l3-2.647z" />
+ </svg>
+ Creating account...
+ </span>
+ ) : (
+ "Let's go →"
+ )}
</button>📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| <button | |
| className="button-confirm w-full h-10 rounded-md border-2 border-black bg-beige text-[17px] font-semibold shadow-[4px_4px_0px_0px_black] mb-2 hover:text-green-300" | |
| onClick={handleSubmit} | |
| > | |
| {isLoading ? 'Loading...' : "Let's go →"} | |
| </button> | |
| <button | |
| className="button-confirm w-full h-10 rounded-md border-2 border-black bg-beige text-[17px] font-semibold shadow-[4px_4px_0px_0px_black] mb-2 hover:text-green-300" | |
| onClick={handleSubmit} | |
| disabled={isLoading} | |
| > | |
| {isLoading ? ( | |
| <span className="flex items-center justify-center"> | |
| <svg className="animate-spin h-5 w-5 mr-3" viewBox="0 0 24 24"> | |
| <circle className="opacity-25" cx="12" cy="12" r="10" stroke="currentColor" strokeWidth="4" fill="none" /> | |
| <path className="opacity-75" fill="currentColor" d="M4 12a8 8 0 018-8V0C5.373 0 0 5.373 0 12h4zm2 5.291A7.962 7.962 0 014 12H0c0 3.042 1.135 5.824 3 7.938l3-2.647z" /> | |
| </svg> | |
| Creating account... | |
| </span> | |
| ) : ( | |
| "Let's go →" | |
| )} | |
| </button> |
| {events.length > 0 && events.map((event) => ( | ||
| <div |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Enhance UI with loading and empty states.
The events rendering could be improved with better handling of different states.
Consider implementing these improvements:
+ const [isLoading, setIsLoading] = useState(true);
const fetchData = async () => {
+ setIsLoading(true);
try {
const response = await fetch(...);
const data = await response.json();
setEvents(data);
} catch (error) {
setError(error.message);
} finally {
+ setIsLoading(false);
}
};
- {events.length > 0 && events.map((event) => (
+ {isLoading ? (
+ <div className="flex justify-center items-center h-40">
+ <span className="loading loading-spinner loading-lg"></span>
+ </div>
+ ) : error ? (
+ <div className="text-red-500 text-center">
+ <p>Error loading events: {error}</p>
+ <button
+ onClick={fetchData}
+ className="mt-4 bg-blue-500 text-white px-4 py-2 rounded"
+ >
+ Retry
+ </button>
+ </div>
+ ) : events.length === 0 ? (
+ <div className="text-center text-gray-500">
+ <p>No events found</p>
+ </div>
+ ) : (
+ events.map((event) => (Committable suggestion skipped: line range outside the PR's diff.
|
@RamakrushnaBiswal can you now check? |
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
RamakrushnaBiswal
added
enhancement
Fixes #212
hello @RamakrushnaBiswal i have implemented the changes:
BoardGame.Cafe.-.Google.Chrome.2024-10-31.17-19-24.mp4
Summary by CodeRabbit
Release Notes
New Features
ProtectedRoutecomponent to restrict access to admin routes based on user roles.Bug Fixes
Improvements
Admincomponent to dynamically display the logged-in user's name.Styling