Restrict Event Booking to Verified Users #427
Conversation
|
@haseebzaki-07 is attempting to deploy a commit to the bunty's projects Team on Vercel. A member of the Team first needs to authorize it. |
|
Warning Rate limit exceeded@haseebzaki-07 has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 6 minutes and 58 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. WalkthroughThe pull request introduces several modifications across multiple files, primarily focusing on updates to the Changes
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: 5
🧹 Outside diff range and nitpick comments (5)
backend/routes/eventRouter.js (2)
28-30: Fix spacing in route definitions
The middleware function calls lack proper spacing, making the code less readable.
Apply this formatting fix:
-router.post("/create",authenticateCustomer, createEvent);
-router.get("/all",authenticateCustomer, getEvents);
-router.get("/delete",authenticateCustomer, deleteEvent);
+router.post("/create", authenticateCustomer, createEvent);
+router.get("/all", authenticateCustomer, getEvents);
+router.get("/delete", authenticateCustomer, deleteEvent);28-30: Consider splitting event viewing and booking into separate routes
To better align with the PR objectives of restricting only the booking functionality to verified users, consider restructuring the routes:
- Keep GET /all public for event browsing
- Add a new protected POST /book route for actual booking
- Maintain protection on create/delete for admin operations
This would provide a better user experience while maintaining security requirements.
Example structure:
// Public routes
router.get("/all", getEvents);
// Protected routes
router.post("/book", authenticateCustomer, bookEvent);
router.post("/create", authenticateCustomer, createEvent);
router.delete("/:id", authenticateCustomer, deleteEvent);frontend/src/components/Pages/Event.jsx (1)
Line range hint 1-324: Consider architectural improvements for security and UX
The current implementation could benefit from several architectural improvements:
-
Event data fetching:
- Consider implementing pagination for better performance
- Add proper error boundaries
- Show loading states during data fetching
-
Authentication state:
- Use React Context or Redux for global auth state management
- Implement proper loading states during auth checks
- Add proper error handling for authentication failures
-
Security:
- Move sensitive operations to protected routes
- Implement rate limiting for event registration
- Add CSRF protection for form submissions
Would you like me to provide example implementations for any of these improvements?
README.md (1)
Line range hint 283-416: Replace hard tabs with spaces in the contributors table
The contributors table contains hard tabs that should be replaced with spaces for consistent formatting across different editors and platforms.
🧰 Tools
🪛 Markdownlint
371-371: Column: 1
Hard tabs
(MD010, no-hard-tabs)
372-372: Column: 1
Hard tabs
(MD010, no-hard-tabs)
backend/middlewares/authCustomer.js (1)
5-5: Remove unnecessary comment.
The comment "// Assuming the Customer model is located here" is unnecessary and can be removed to keep the code clean and maintainable.
Apply this diff to remove the comment:
- // Assuming the Customer model is located here📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (5)
- README.md (6 hunks)
- backend/controller/customer.controller.js (0 hunks)
- backend/middlewares/authCustomer.js (1 hunks)
- backend/routes/eventRouter.js (2 hunks)
- frontend/src/components/Pages/Event.jsx (3 hunks)
💤 Files with no reviewable changes (1)
- backend/controller/customer.controller.js
🧰 Additional context used
🪛 Markdownlint
README.md
283-283: Column: 1
Hard tabs
(MD010, no-hard-tabs)
284-284: Column: 1
Hard tabs
(MD010, no-hard-tabs)
327-327: Column: 1
Hard tabs
(MD010, no-hard-tabs)
328-328: Column: 1
Hard tabs
(MD010, no-hard-tabs)
371-371: Column: 1
Hard tabs
(MD010, no-hard-tabs)
372-372: Column: 1
Hard tabs
(MD010, no-hard-tabs)
415-415: Column: 1
Hard tabs
(MD010, no-hard-tabs)
416-416: Column: 1
Hard tabs
(MD010, no-hard-tabs)
🔇 Additional comments (6)
backend/routes/eventRouter.js (1)
8-8: LGTM: Clean middleware import
The addition of the authentication middleware aligns well with the PR objective of restricting event access to verified users.
frontend/src/components/Pages/Event.jsx (3)
17-18: LGTM!
The new imports are appropriate for implementing the authentication check and navigation functionality.
36-36: LGTM!
The navigation hook is correctly initialized.
Line range hint 1-324: Verify backend validation implementation
The frontend changes implement the redirect functionality, but we should verify that the backend properly validates both authentication and verification status for event registration endpoints.
README.md (1)
330-341: LGTM! Contributor profile updates look good
The following changes have been implemented correctly:
- Updated profile link for Suhas Koheda
- Updated profile link for Jay shah
- Updated profile link for Sajal Batra
- Added new contributor Vinay Anand Lodhi
Also applies to: 351-356, 380-386
backend/middlewares/authCustomer.js (1)
19-20: Verify that 'decoded.sub' contains the customer ID.
Ensure that the sub property in the decoded token payload contains the customer's ID. This is crucial for retrieving the correct user from the database.
Run the following script to verify the structure of the JWT payload in your codebase:
✅ Verification successful
Let me search for the payload construction in the customer controller to verify if it includes the customer ID in the sub claim.
decoded.sub correctly contains the customer ID
The verification confirms that the JWT payload is properly structured with the customer's ID in the sub claim. The customer controller sets customer._id as the sub claim when signing the token, which matches the usage in authCustomer.js where it's used to find the customer by ID.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify that the JWT tokens are signed with 'sub' containing the customer ID.
# Search for JWT signing instances to confirm 'sub' is set correctly.
rg -A 5 'jwt\.sign'
Length of output: 1233
Script:
#!/bin/bash
# Search for payload construction in customer controller
rg -B 5 -A 2 'payload' backend/controller/customer.controller.js
Length of output: 554
| router.get("/delete", deleteEvent); | ||
| router.post("/create",authenticateCustomer, createEvent); | ||
| router.get("/all",authenticateCustomer, getEvents); | ||
| router.get("/delete",authenticateCustomer, deleteEvent); |
There was a problem hiding this comment.
Replace GET /delete with DELETE method
Using GET for delete operations violates RESTful principles and can lead to unintended consequences (e.g., search engine crawlers accidentally deleting resources).
Apply this change:
-router.get("/delete", authenticateCustomer, deleteEvent);
+router.delete("/:id", authenticateCustomer, deleteEvent);📝 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.
| router.get("/delete",authenticateCustomer, deleteEvent); | |
| router.delete("/:id", authenticateCustomer, deleteEvent); |
| router.get("/all", getEvents); | ||
| router.get("/delete", deleteEvent); | ||
| router.post("/create",authenticateCustomer, createEvent); | ||
| router.get("/all",authenticateCustomer, getEvents); |
There was a problem hiding this comment.
💡 Codebase verification
Based on the gathered information, I can now provide a final response:
Authentication requirement for GET /all should be removed
The frontend Event.jsx component makes an unauthenticated request to fetch events (/api/event/all), while the backend requires authentication. This mismatch causes a functionality gap where:
- The events page is publicly accessible and attempts to display events
- The backend blocks the event data due to authentication
- Only the "Register" action appropriately checks for authentication
Recommended changes:
- Remove
authenticateCustomermiddleware from the GET/allroute - Keep authentication for POST
/createand GET/deleteroutes - Keep the frontend's authentication check for event registration
🔗 Analysis chain
Reconsider authentication requirement for GET /all
The current implementation requires authentication for viewing all events, which might be too restrictive. Consider whether events should be publicly viewable while keeping the booking functionality restricted to verified users (as per PR objectives).
Let's verify the requirements:
Let me gather more specific information about the authentication and event access patterns.
Let me try one more search to understand the event-related components and authentication flow better.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check if there are any public-facing components that might need event data
echo "Checking for frontend components that might display events..."
rg -l "events" --type js --type jsx --type ts --type tsx
# Check if there are any documentation or requirements specifying public access
echo "Checking for documentation about event visibility..."
rg -l "event.*public|public.*event" --type md
Length of output: 399
Script:
#!/bin/bash
# Check the event-related routes and their authentication requirements
echo "Checking event routes and authentication patterns..."
rg -A 5 -B 5 "router\.(get|post|put|delete).*event" --type js
# Check for any frontend API calls or services related to events
echo "Checking frontend API calls..."
rg -l "api.*event|event.*api" --type js
# Check for any documentation about authentication requirements
echo "Checking authentication requirements..."
rg "auth.*require|require.*auth" --type md README.md
Length of output: 515
Script:
#!/bin/bash
# Check the content of event-related frontend components
echo "Checking Event.jsx content..."
cat frontend/src/components/Pages/Event.jsx
# Check the event router implementation
echo "Checking eventRouter.js content..."
cat backend/routes/eventRouter.js
# Look for any authentication middleware implementation
echo "Checking authentication middleware..."
fd authenticateCustomer --type f --exec cat {}
Length of output: 12050
| @@ -183,7 +197,7 @@ function Event() { | |||
| ))} | |||
| </div> | |||
| <div className="text-center mt-4"> | |||
| <button className="bg-[#FEF3C7] dark:bg-black text-gray-700 dark:text-white px-4 py-2 rounded-md mt-4"> | |||
| <button onClick={handleRegisterClick} className="bg-[#FEF3C7] dark:bg-black text-gray-700 dark:text-white px-4 py-2 rounded-md mt-4"> | |||
There was a problem hiding this comment.
Inconsistent registration button behavior
There are two different registration buttons in the component:
- "Register for Event" button with authentication check
- "Book your slot" button without authentication check
This creates inconsistent user experience and potential security gaps.
Consolidate the registration functionality:
- <button onClick={handleRegisterClick} className="bg-[#FEF3C7] dark:bg-black text-gray-700 dark:text-white px-4 py-2 rounded-md mt-4">
- Register for Event
- </button>
- <button className="btn btn-primary bg-[#C3E0DC] p-4 rounded-xl text-md font-medium mt-4 hover:bg-[#FFF9B1]">
- Book your slot
- </button>
+ <button
+ onClick={() => handleRegisterClick(event._id)}
+ className="btn btn-primary bg-[#C3E0DC] p-4 rounded-xl text-md font-medium mt-4 hover:bg-[#FFF9B1]"
+ >
+ Register for Event
+ </button>Also applies to: 308-308
| const handleRegisterClick = () => { | ||
| const isAuthenticated = Boolean(Cookies.get('authToken')); | ||
|
|
||
| if (!isAuthenticated) { | ||
| alert("Please sign in to register for the event."); | ||
| navigate('/login'); | ||
| } | ||
| }; |
There was a problem hiding this comment.
Authentication check needs enhancement
The current implementation has several areas for improvement:
- It only checks for token presence but not user verification status (required by PR objectives)
- The alert is not user-friendly and may be blocked by browsers
- After login, users lose context of which event they were trying to register for
Consider implementing these improvements:
const handleRegisterClick = () => {
- const isAuthenticated = Boolean(Cookies.get('authToken'));
+ try {
+ const authToken = Cookies.get('authToken');
+ const isVerified = Cookies.get('isVerified'); // Add verification check
- if (!isAuthenticated) {
- alert("Please sign in to register for the event.");
- navigate('/login');
+ if (!authToken || !isVerified) {
+ // Store current path for redirect after login
+ sessionStorage.setItem('redirectAfterLogin', window.location.pathname);
+ // Use a more user-friendly notification system
+ setError(isVerified ?
+ "Please sign in to register for the event" :
+ "Please verify your email before registering");
+ navigate('/login');
+ return;
}
+ } catch (error) {
+ console.error('Error checking authentication:', error);
+ setError('Something went wrong. Please try again.');
+ }
};Also, consider adding a state management solution (like Redux or Context) to handle authentication state more robustly.
Committable suggestion was skipped due to low confidence.
| } catch (err) { | ||
| if (err.name === "TokenExpiredError") { | ||
| return res.status(401).json({ message: "Token expired" }); | ||
| } | ||
| logger.error("Token verification failed:", err); | ||
| return res.status(403).json({ message: "Invalid token" }); |
There was a problem hiding this comment.
Change status code to 401 Unauthorized for invalid token.
When token verification fails due to an invalid token, it's more appropriate to return a 401 Unauthorized response instead of 403 Forbidden. The 401 status code indicates that authentication has failed, which aligns with the nature of an invalid token.
Apply this diff to change the status code:
logger.error("Token verification failed:", err);
- return res.status(403).json({ message: "Invalid token" });
+ return res.status(401).json({ message: "Invalid token" });📝 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.
| } catch (err) { | |
| if (err.name === "TokenExpiredError") { | |
| return res.status(401).json({ message: "Token expired" }); | |
| } | |
| logger.error("Token verification failed:", err); | |
| return res.status(403).json({ message: "Invalid token" }); | |
| } catch (err) { | |
| if (err.name === "TokenExpiredError") { | |
| return res.status(401).json({ message: "Token expired" }); | |
| } | |
| logger.error("Token verification failed:", err); | |
| return res.status(401).json({ message: "Invalid token" }); |
RamakrushnaBiswal
added
level1
|
@haseebzaki-07 resolve the conflicts |
|
@RamakrushnaBiswal done. |
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Outside diff range and nitpick comments (2)
README.md (2)
Line range hint 263-267: Remove duplicate contributor entry for Haseeb Zaki.
The contributor "Haseeb Zaki" appears twice in the contributors table. Please remove one of the duplicate entries to maintain a clean and accurate contributor list.
Also applies to: 330-334
🧰 Tools
🪛 Markdownlint
264-264: Expected: fenced; Actual: indented
Code block style
(MD046, code-block-style)
Line range hint 1-650: Consider automating contributor list updates.
To prevent issues like duplicate entries and inconsistent formatting, consider using automated tools like All-Contributors bot to manage the contributors list.
This would help:
- Prevent duplicate entries
- Maintain consistent formatting
- Automate contributor recognition
- Keep the list up-to-date
🧰 Tools
🪛 Markdownlint
343-343: Column: 1
Hard tabs
(MD010, no-hard-tabs)
344-344: Column: 1
Hard tabs
(MD010, no-hard-tabs)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (2)
- README.md (14 hunks)
- backend/controller/customer.controller.js (2 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- backend/controller/customer.controller.js
🧰 Additional context used
🪛 Markdownlint
README.md
299-299: Column: 1
Hard tabs
(MD010, no-hard-tabs)
300-300: Column: 1
Hard tabs
(MD010, no-hard-tabs)
343-343: Column: 1
Hard tabs
(MD010, no-hard-tabs)
344-344: Column: 1
Hard tabs
(MD010, no-hard-tabs)
413-413: Column: 1
Hard tabs
(MD010, no-hard-tabs)
414-414: Column: 1
Hard tabs
(MD010, no-hard-tabs)
459-459: Column: 1
Hard tabs
(MD010, no-hard-tabs)
460-460: Column: 1
Hard tabs
(MD010, no-hard-tabs)
RamakrushnaBiswal
merged commit c5c48fa
into
RamakrushnaBiswal:main
|
hi @haseebzaki-07 can you repush the code |
|
okay @RamakrushnaBiswal I will do it |
fixes #298
This PR updates the authCustomer middleware to verify for the token and the isVerified field in the customer model.
Updated the frontend component to check for the cookie present in the application.
2024-10-29.13-22-25.mp4
Summary by CodeRabbit
Release Notes
New Features
Bug Fixes
Documentation
Chores