Repository navigation
Conversation
Implement JWT-based authentication with role-based access control: - Add auth middleware for token verification and role authorization - Create /auth/login and /auth/register endpoints - Protect all routes except /health and /auth - Add role-based permissions (admin/user) to routes - Add jsonwebtoken and bcryptjs dependencies BREAKING CHANGE: API endpoints now require Bearer token authentication
WalkthroughThis pull request introduces JWT-based authentication and role-based authorization to the API. It adds a new auth middleware module, login/register endpoints with bcrypt hashing, and applies authentication/authorization checks across existing product, order, and visitor routes with role enforcement (user/admin). Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant App as App / Router
participant Auth as Auth Middleware
participant AuthRoute as Auth Route<br/>(Login/Register)
participant JWT as JWT / Crypto
participant Storage as In-Memory<br/>User Storage
Note over Client,Storage: Authentication Flow - Login
Client->>App: POST /auth/login<br/>{username, password}
App->>Auth: Validate request body
Auth->>AuthRoute: Pass to login handler
AuthRoute->>Storage: Lookup user by username
Storage-->>AuthRoute: User data
AuthRoute->>JWT: Verify password<br/>(bcrypt.compare)
JWT-->>AuthRoute: Password match: true/false
alt Password match
AuthRoute->>JWT: Sign JWT<br/>(id, username, role)
JWT-->>AuthRoute: Token (24h expiry)
AuthRoute-->>Client: 200 {token, user}
else Password mismatch
AuthRoute-->>Client: 401 Unauthorized
end
Note over Client,Storage: Protected Route - Authenticated Request
Client->>App: GET /products<br/>Authorization: Bearer {token}
App->>Auth: Extract & verify JWT
Auth->>JWT: Verify token<br/>with JWT_SECRET
JWT-->>Auth: Decoded user {id, role}
Auth->>Auth: Check role in allowedRoles<br/>('user' or 'admin')
alt Valid token & authorized role
Auth->>App: Attach req.user
App-->>Client: 200 {products, user}
else Invalid/expired token
Auth-->>Client: 401 Unauthorized
else Valid token but insufficient role
Auth-->>Client: 403 Forbidden
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~30 minutes 🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
✅ Snyk checks have passed. No issues have been found so far.
💻 Catch issues earlier using the plugins for VS Code, JetBrains IDEs, Visual Studio, and Eclipse. |
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Fix all issues with AI agents
In `@api/middleware/auth.js`:
- Around line 3-4: The JWT_SECRET constant currently falls back to a hardcoded
insecure default which allows token forgery if the env var is missing; update
the initialization of JWT_SECRET to require process.env.JWT_SECRET and fail fast
in non-development environments (throw an Error or exit the process) when it is
unset, while optionally allowing a configurable dev-only default when NODE_ENV
=== 'development' for local testing; reference the JWT_SECRET constant in
auth.js (and any middleware functions that read it) and add a startup check that
validates the secret and stops the app if it’s not provided in production.
In `@api/routes/auth.js`:
- Around line 99-109: The registration handler (router.post('/register'))
currently reads role from req.body allowing privilege escalation; remove the
role value from the request path and always set the new user's role to a safe
default (e.g., 'user') within the handler (do not destructure or use
req.body.role), or alternatively enforce that only an authenticated admin
endpoint can assign non-default roles; update any user-creation call (the
function that persists the user) to use this enforced role instead of the
incoming role value and ensure validation/logging reflects the enforced default.
- Around line 24-34: The async initializer initializeUsers is invoked without
awaiting, causing a race where login may occur before passwords are hashed; fix
by either (A) making module initialization explicit: replace the direct
initializeUsers() call with an awaited top-level IIFE (e.g., (async () => {
await initializeUsers(); })()) so hashing completes before exports/requests are
served, or (B) avoid runtime hashing entirely by precomputing bcrypt hashes for
'admin123' and 'user123', updating the users array to store those hashed strings
and removing initializeUsers and its call; locate initializeUsers and the users
array in auth.js to apply one of these changes.
🧹 Nitpick comments (3)
app.js (1)
5-5: Consider using Express 5.x built-in body parsing.Express 5.x (^5.2.1 per package.json) includes built-in body parsing via
express.json()andexpress.urlencoded(). The separatebody-parserdependency is redundant.♻️ Proposed fix
-const bodyParser = require('body-parser'); const authRoute = require('./api/routes/auth');-app.use(bodyParser.json()); -app.use(bodyParser.urlencoded({ extended: true })); +app.use(express.json()); +app.use(express.urlencoded({ extended: true }));Also applies to: 14-15
api/routes/auth.js (2)
123-129: Minor: ID generation and redundant role fallback.
users.length + 1for ID generation can produce duplicates if users are ever deleted. Consider using a separate counter or UUID.- Line 128
role: role || 'user'is redundant sincerolealready defaults to'user'in the destructuring on line 101.
89-89: Consider logging caught errors for debugging.The
errorvariables are caught but unused. While the generic error messages are appropriate for clients, logging the actual error server-side aids debugging.♻️ Proposed fix
} catch (error) { + console.error('Login failed:', error); res.status(500).json({} catch (error) { + console.error('Registration failed:', error); res.status(500).json({Also applies to: 153-153
| const err = new Error('This page does not exist, Navigate to /products or /orders or /visitors or /health or /auth'); | ||
| err.status = 404; |
There was a problem hiding this comment.
Please update this expected test results in app.test.js so that test can pass
| if (user.username === 'admin' && user.password.startsWith('$2a$10$rOzJqZqZqZqZqZqZqZqZqO')) { | ||
| user.password = await bcrypt.hash('admin123', 10); | ||
| } else if (user.username === 'user' && user.password.startsWith('$2a$10$rOzJqZqZqZqZqZqZqZqZqO')) { | ||
| user.password = await bcrypt.hash('user123', 10); | ||
| } | ||
| } | ||
| } | ||
| initializeUsers(); | ||
|
|
There was a problem hiding this comment.
initializeUsers() function is asynchronous but is called without a .catch() block. If hashing fails, this will cause an UnhandledPromiseRejection, which may crash the server in production.
| const authenticate = (req, res, next) => { | ||
| try { | ||
| // Get token from Authorization header | ||
| const authHeader = req.headers.authorization; | ||
|
|
||
| if (!authHeader || !authHeader.startsWith('Bearer ')) { | ||
| return res.status(401).json({ | ||
| error: { | ||
| message: 'No token provided. Please include a Bearer token in Authorization header.' |
There was a problem hiding this comment.
Please add a default test case for /order where we will just verify the error:
message: 'No token provided. Please include a Bearer token in Authorization header.'
|
Also please update the readme.md (you can auto generate via any AI) |
Implement JWT-based authentication with role-based access control:
BREAKING CHANGE: API endpoints now require Bearer token authentication
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.