Fix/be/refactor auth controller #1054
Conversation
WalkthroughThe changes in this pull request primarily involve modifications to the Changes
Possibly related PRs
Suggested reviewers
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
Documentation and Community
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (3)
Server/controllers/authController.js (2)
349-368: Mom's spaghetti moment: Let's make this recovery flow bulletproof! 🍝The removal of redundant user existence check is on point since
getUserByEmailalready throws if user isn't found. However, we should consider rate limiting these requests to prevent email flooding.Consider adding rate limiting middleware for the recovery endpoint:
import rateLimit from 'express-rate-limit' const recoveryLimiter = rateLimit({ windowMs: 60 * 60 * 1000, // 1 hour max: 3 // limit each IP to 3 requests per windowMs }); // Apply to your route app.post('/api/recovery', recoveryLimiter, requestRecovery);
Line range hint
282-368: Knees weak, arms heavy: Let's level up our error game! 💪The error handling is consistent, but we could make it even better by adding error codes to help client-side error handling.
Consider extending the error messages with codes:
// utils/messages.js export const errorMessages = { - AUTH_INCORRECT_PASSWORD: "Incorrect password", + AUTH_INCORRECT_PASSWORD: { + code: "AUTH001", + message: "Incorrect password" + }, // ... other messages } // In your controller const error = new Error(errorMessages.AUTH_INCORRECT_PASSWORD.message); error.code = errorMessages.AUTH_INCORRECT_PASSWORD.code;This would help frontend developers handle specific error cases more reliably.
Server/tests/controllers/authController.test.js (1)
815-817: Test case needs to be more explicit about error handling.The test case could be improved to better validate the error handling flow:
Consider this enhancement:
- it("should throw an error if user is not found", async () => { + it("should propagate DB_USER_NOT_FOUND error when user doesn't exist", async () => { jwt.decode.returns({ email: "[email protected]" }); - req.db.getUserByEmail.throws(new Error(errorMessages.DB_USER_NOT_FOUND)); + const notFoundError = new Error(errorMessages.DB_USER_NOT_FOUND); + notFoundError.status = 404; // Explicitly set status + req.db.getUserByEmail.throws(notFoundError);
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (3)
- Server/controllers/authController.js (2 hunks)
- Server/db/mongo/modules/userModule.js (1 hunks)
- Server/tests/controllers/authController.test.js (2 hunks)
✅ Files skipped from review due to trivial changes (1)
- Server/db/mongo/modules/userModule.js
🔇 Additional comments (2)
Server/controllers/authController.js (1)
282-285: Yo dawg, that status code change is straight fire! 🔥Changing from 403 to 401 for incorrect password is the right move. 401 Unauthorized is the proper status code when authentication fails (like wrong password), while 403 Forbidden is for when you're authenticated but don't have permission.
Server/tests/controllers/authController.test.js (1)
470-470: LGTM! Status code change aligns with HTTP standards.The change from 403 to 401 for incorrect password is more appropriate as:
- 401 indicates authentication failure (wrong credentials)
- 403 would indicate authorization failure (valid credentials but insufficient permissions)
This PR refactors
authControllerto remove some redundant checks.getUserByEmailthrows anErrorat the db level if a user is not found, so a check to see if a user is found or not at the controller level is redundant.