Automated Token Refreshes (Human Version)#23
Conversation
There was a problem hiding this comment.
Pull request overview
This PR refactors the token refresh mechanism from an on-startup check to an Express middleware that automatically refreshes expired access tokens before each authenticated request. The implementation shifts from a fire-and-forget function to a proper async middleware pattern integrated into the request lifecycle.
- Converts
updateExpiredToken()function tocheckAccessTokenExpress middleware that checks and refreshes tokens before each authenticated route - Integrates token refresh middleware into all RERUM API routes (create, update, delete, overwrite)
- Updates package dependencies to newer versions and improves CORS configuration
Reviewed changes
Copilot reviewed 8 out of 9 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| tokens.js | Refactored token refresh from startup function to Express middleware with improved error handling and documentation |
| routes/create.js | Added checkAccessToken middleware to POST route |
| routes/update.js | Added checkAccessToken middleware to PUT route |
| routes/overwrite.js | Added checkAccessToken middleware to PUT route |
| routes/delete.js | Added checkAccessToken middleware to DELETE route |
| app.js | Removed startup token check, added express.text() parser, expanded CORS allowed methods and headers |
| package.json | Updated dependency versions for jest, express, dotenv, debug, http-errors, and supertest |
| tests/mount.test.js | Updated mock function name from updateExpiredToken to checkAccessToken |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| .catch(err => { | ||
| throw err | ||
| }) |
There was a problem hiding this comment.
The catch block re-throws the error without adding context, which doesn't provide additional value. Since the error will propagate anyway, consider either:
- Removing the
.catch()entirely and letting errors propagate naturally, or - Adding meaningful error context before re-throwing:
.catch(err => {
throw new Error(`Failed to fetch access token: ${err.message}`)
})| .catch(err => { | |
| throw err | |
| }) |
| .then(res => res.json()) | ||
| .catch(err => { | ||
| throw err | ||
| }) |
There was a problem hiding this comment.
Missing validation: The code doesn't check if tokenObject.access_token exists before accessing it on line 33. If the token refresh request fails but doesn't throw (e.g., returns a 4xx status with an error message), this will attempt to assign undefined to process.env.ACCESS_TOKEN and the .env file, which could break subsequent operations. Consider adding:
if (!tokenObject?.access_token) {
throw new Error('Failed to retrieve access token from response')
}before line 33.
| }) | |
| }) | |
| if (!tokenObject?.access_token) { | |
| throw new Error('Failed to retrieve access token from response') | |
| } |
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
No description provided.