test(auth): add unit tests for auth.service and fix passport local strategy done() call#3175
Conversation
…rategy done() call
- Add auth.service.unit.tests.js: 17 unit tests covering all six service functions
(removeSensitive, comparePassword, hashPassword, authenticate, checkPassword,
generateRandomPassphrase) using jest.unstable_mockModule for ESM compatibility
- Fix local strategy error handler: done() → done(null, false, { message }) so
passport correctly signals authentication failure instead of internal error
Closes #3174
PierreBrisorgueil
added
Fix
PierreBrisorgueil
added
Fix
|
Warning Rate limit exceeded
⌛ 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. 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughFixes passport local strategy error handling to return authentication-failure via Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #3175 +/- ##
==========================================
+ Coverage 89.86% 89.87% +0.01%
==========================================
Files 52 52
Lines 1164 1166 +2
Branches 234 235 +1
==========================================
+ Hits 1046 1048 +2
Misses 107 107
Partials 11 11 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Pull request overview
Adds unit-level test coverage for modules/auth/services/auth.service.js and adjusts the Passport local strategy failure path so invalid credentials are reported as an authentication failure rather than an internal error.
Changes:
- Added a new unit test suite covering
removeSensitive,comparePassword,hashPassword,authenticate,checkPassword, andgenerateRandomPassphrase. - Updated the Passport local strategy to call
done(null, false, { message })on errors instead of callingdone()with no args.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| modules/auth/tests/auth.service.unit.tests.js | New unit tests for all exported AuthService functions (with module mocks for repo, bcrypt, zxcvbn, and config). |
| modules/auth/config/strategies/local.js | Adjusts error handling to return an auth failure via done(null, false, info) instead of an empty done() call. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
modules/auth/config/strategies/local.js (1)
16-27:⚠️ Potential issue | 🟡 MinorAdd JSDoc to the modified strategy verifier function.
The async verifier callback was modified but has no JSDoc header (
@param/@returns).As per coding guidelines,
**/*.js: Every new or modified function must have a JSDoc header: one-line description,@paramfor each argument,@returnsfor any non-void return value (always include@returnsfor async functions to document the resolved value).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@modules/auth/config/strategies/local.js` around lines 16 - 27, Add a JSDoc header above the async verifier callback (the async (email, password, done) => { ... } function) describing its purpose in one line, add `@param` tags for email (string), password (string) and done (Passport-style callback) and add an `@returns` tag documenting the Promise resolution (e.g., Promise<void> or the resolved value from calling done). Keep the JSDoc concise and placed immediately above the anonymous verifier function inside the strategy so it satisfies the /** ... */ rule for modified JS functions.
🧹 Nitpick comments (1)
modules/auth/config/strategies/local.js (1)
24-26: Distinguish authentication failures from infrastructure errors in the catch block.The current implementation converts all exceptions—including database and bcrypt errors—into auth failures. Since
authenticate()throws bothAppError('invalid user or password.')for expected auth failures and can propagate infrastructure errors (fromUserRepository.get()andbcrypt.compare()), usedone(err)for non-authentication exceptions to surface real operational issues instead of masking them as invalid credentials.Suggested improvement
- } catch (_err) { - return done(null, false, { message: 'Invalid email or password' }); + } catch (err) { + if (err?.message === 'invalid user or password.') { + return done(null, false, { message: 'Invalid email or password' }); + } + return done(err); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@modules/auth/config/strategies/local.js` around lines 24 - 26, The catch block currently converts every exception into an authentication failure; update it to rethrow/invoke done(err) for infrastructure errors while preserving the auth-failure path for expected auth errors: detect whether the caught error is the expected authentication sentinel (e.g., an instance of AppError or has the message 'invalid user or password.' produced by authenticate()/UserRepository.get()/bcrypt.compare()), and if so call done(null, false, { message: 'Invalid email or password' }); otherwise call done(err) to surface real operational errors from UserRepository.get, bcrypt.compare, or other unexpected exceptions.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@modules/auth/tests/auth.service.unit.tests.js`:
- Around line 10-180: Add JSDoc headers to every new/modified function in this
test module (including the callbacks passed to describe, test, and beforeEach
and any standalone helper functions) so the repository JSDoc rule is satisfied;
for each function (e.g., the beforeEach callback, describe callbacks like 'Auth
service unit tests:' and nested describe blocks, and each test callback such as
the async tests in comparePassword, hashPassword, authenticate), add a one-line
description, `@param` entries for any parameters, and `@returns` for any non-void or
async functions (document the resolved value for async tests). Ensure each JSDoc
immediately precedes the function expression/arrow function so linters pick it
up.
---
Outside diff comments:
In `@modules/auth/config/strategies/local.js`:
- Around line 16-27: Add a JSDoc header above the async verifier callback (the
async (email, password, done) => { ... } function) describing its purpose in one
line, add `@param` tags for email (string), password (string) and done
(Passport-style callback) and add an `@returns` tag documenting the Promise
resolution (e.g., Promise<void> or the resolved value from calling done). Keep
the JSDoc concise and placed immediately above the anonymous verifier function
inside the strategy so it satisfies the /** ... */ rule for modified JS
functions.
---
Nitpick comments:
In `@modules/auth/config/strategies/local.js`:
- Around line 24-26: The catch block currently converts every exception into an
authentication failure; update it to rethrow/invoke done(err) for infrastructure
errors while preserving the auth-failure path for expected auth errors: detect
whether the caught error is the expected authentication sentinel (e.g., an
instance of AppError or has the message 'invalid user or password.' produced by
authenticate()/UserRepository.get()/bcrypt.compare()), and if so call done(null,
false, { message: 'Invalid email or password' }); otherwise call done(err) to
surface real operational errors from UserRepository.get, bcrypt.compare, or
other unexpected exceptions.
- Rename auth.service.unit.tests.js → auth.unit.tests.js to match project naming convention - Fix local strategy catch block: propagate unexpected errors via done(err) instead of silently converting all exceptions (including DB/bcrypt failures) to authentication failures
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
modules/auth/config/strategies/local.js (1)
10-17:⚠️ Potential issue | 🟡 MinorAdd JSDoc headers for modified functions.
The modified strategy initializer/verify function does not have JSDoc headers. This repo rule applies to modified JS functions too.
As per coding guidelines,
**/*.{js,ts}: Every new or modified function must have a JSDoc header: one-line description,@paramfor each argument, and@returnsfor non-void/async functions.Also applies to: 25-31
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@modules/auth/config/strategies/local.js` around lines 10 - 17, Add JSDoc headers to the exported strategy initializer (export default () => { ... }) and the async verify callback passed to new Strategy (async (email, password, done) => { ... }); for each include a one-line description and `@param` entries for each argument (e.g., email, password, done) and for the async verify include an `@returns` {Promise<void>} (or appropriate return type) tag. Ensure the JSDoc appears immediately above the export default function and immediately above the verify function definition (the anonymous async verify passed into new Strategy) and also apply the same JSDoc pattern to the other modified function blocks referenced in the review.
♻️ Duplicate comments (1)
modules/auth/tests/auth.unit.tests.js (1)
10-180:⚠️ Potential issue | 🟡 MinorJSDoc rule is still unmet across new test callbacks.
Callbacks added in this file (mock module factories,
beforeEach,describe, andtesthandlers) are new/modified functions and still missing JSDoc blocks.As per coding guidelines,
**/*.js: Every new or modified function must have a JSDoc header: one-line description,@paramfor each argument, and@returnsfor non-void/async functions.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@modules/auth/tests/auth.unit.tests.js` around lines 10 - 180, The file is missing JSDoc headers for newly added/modified callbacks: the mock factory functions passed to jest.unstable_mockModule, the beforeEach callback, and each describe/test handler (all arrow functions used in describe(...) and test(...)). Add a JSDoc block above each callback with a one-line description, `@param` entries for each argument (e.g., evt or done if used) and `@returns` for any non-void or async handlers (e.g., tests/blocks returning Promises), covering the mock factories, beforeEach, and all test callbacks like those around removeSensitive, comparePassword, hashPassword, authenticate, checkPassword, and generateRandomPassphrase.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@modules/auth/config/strategies/local.js`:
- Line 7: The import for AppError in modules/auth/config/strategies/local.js is
pointing to the wrong relative path; locate the real AppError module in the repo
and update the import statement that currently references
'../../../lib/helpers/AppError.js' to the correct relative path (or use the
project alias/import root if configured) so the AppError symbol resolves; ensure
the import still uses the exact exported name AppError and run the tests/CI to
confirm resolution.
---
Outside diff comments:
In `@modules/auth/config/strategies/local.js`:
- Around line 10-17: Add JSDoc headers to the exported strategy initializer
(export default () => { ... }) and the async verify callback passed to new
Strategy (async (email, password, done) => { ... }); for each include a one-line
description and `@param` entries for each argument (e.g., email, password, done)
and for the async verify include an `@returns` {Promise<void>} (or appropriate
return type) tag. Ensure the JSDoc appears immediately above the export default
function and immediately above the verify function definition (the anonymous
async verify passed into new Strategy) and also apply the same JSDoc pattern to
the other modified function blocks referenced in the review.
---
Duplicate comments:
In `@modules/auth/tests/auth.unit.tests.js`:
- Around line 10-180: The file is missing JSDoc headers for newly added/modified
callbacks: the mock factory functions passed to jest.unstable_mockModule, the
beforeEach callback, and each describe/test handler (all arrow functions used in
describe(...) and test(...)). Add a JSDoc block above each callback with a
one-line description, `@param` entries for each argument (e.g., evt or done if
used) and `@returns` for any non-void or async handlers (e.g., tests/blocks
returning Promises), covering the mock factories, beforeEach, and all test
callbacks like those around removeSensitive, comparePassword, hashPassword,
authenticate, checkPassword, and generateRandomPassphrase.
Adds an integration test that mocks AuthService.authenticate to throw a plain Error (non-AppError) and verifies the server responds with 500, covering the done(err) branch in the local passport strategy.
Summary
auth.service.jscovering all six exported functions; fixed a bug in the passport local strategy error handlerauth.service.jshad zero unit tests; the local strategy was callingdone()with no arguments on authentication failure, which causes passport to treat the failure as an internal error rather than anunauthorizedresponseScope
modules/auth(service unit tests, local strategy)nonelowValidation
npm run lintnpm test(unit tests — 101 passing)Guardrails check
.env*,secrets/**, keys, tokens)Notes for reviewers
done()→done(null, false, { message })fix ensures passport correctly returns a 401 (Unauthorized) rather than a 500 (Internal Server Error) when credentials are invalid — a security-relevant correctionlocal.jsfix is self-containedSummary by CodeRabbit
Bug Fixes
Tests