Generate refresh token & add it to response #960
Conversation
|
Hey @ajhollid, Currently, I have created a new function for generating the refresh token. Can we pass an additional parameter to the issueToken function to determine which token to generate, and use the corresponding secret? |
I think that's a good idea, there's no reason to have a separate function to create refresh tokens vs access tokens. Why don't we have The config could look like: I can't remember exactly how the function works off the top of my head, but it shouldn't be difficult to refactor. Please make sure tests still pass after refactoring 👍 |
WalkthroughThe changes introduced in this pull request focus on enhancing the authentication system by adding a new function for issuing JWT refresh tokens. 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: 4
🧹 Outside diff range and nitpick comments (6)
Server/service/settingsService.js (3)
9-9: Yo, this addition's fire, but let's make it bulletproof!The new
refreshTokenSecretproperty is on point, aligning with our refresh token goals. It's like adding the secret sauce to mom's spaghetti - crucial for the flavor, yo!But listen up, we gotta make sure this secret's always there, or our auth system might choke harder than Eminem at the start of "Lose Yourself". Let's add a fallback, just in case:
- refreshTokenSecret: process.env.REFRESH_TOKEN_SECRET, + refreshTokenSecret: process.env.REFRESH_TOKEN_SECRET || 'default_refresh_secret',Don't forget to swap out that default in production, or we'll be more exposed than Slim Shady without his hoodie!
15-15: Aight, we're cookin' now, but let's not burn the sauce!Adding
refreshTokenTTLis smoother than Eminem's flow, but we gotta make sure it's as tight as his rhymes. Right now, it's like we're serving mom's spaghetti without checking if it's al dente, you feel me?Let's add some type conversion to this bad boy:
- refreshTokenTTL: process.env.REFRESH_TOKEN_TTL, + refreshTokenTTL: parseInt(process.env.REFRESH_TOKEN_TTL, 10) || 86400, // Default to 24 hours if not setThis way, we're making sure our TTL is a number, not a string. It's like making sure your mic is on before you start spittin' bars - crucial for the performance!
Also, that default value? It's like having a backup beat in case your main track fails. Always be prepared, just like Eminem before a rap battle!
9-15: Yo, these changes are tighter than Eminem's rhymes in '8 Mile'!The new
refreshTokenSecretandrefreshTokenTTLare sliding into ourenvConfigsmoother than a beat drop. They're gonna be picked up by ourSettingsServicefaster than Slim Shady grabs the mic.But hold up, we're missing one thing to make this perfect. Let's add some comments to explain these new settings, just like we did for the others. It's like adding annotations to your lyrics - helps everyone understand the flow:
jwtTTL: process.env.TOKEN_TTL, + // Secret used for signing refresh tokens refreshTokenSecret: process.env.REFRESH_TOKEN_SECRET, + // Time-to-live for refresh tokens (in seconds) refreshTokenTTL: process.env.REFRESH_TOKEN_TTL,Now it's all consistent, like every bar in a perfect rap. Keep it up, and this code'll be winning rap battles in no time!
guides/users-guide/quickstart.md (2)
82-82: Yo, dawg! Let's make this table pop like it's hot!The new environment variables are on point, but this table's looking messier than my sweater after mom's spaghetti. How about we give it some swagger?
Try this fly formatting to make it easier on the eyes:
| ENV Variable Name | Required/Optional | Type | Description | Accepted Values | |-----------------------|-------------------|----------|-----------------------------|-----------------------------------------------------| | CLIENT_HOST | Required | `string` | Frontend Host | | | JWT_SECRET | Required | `string` | JWT secret | | | REFRESH_TOKEN_SECRET | Required | `string` | Refresh JWT secret | | | DB_TYPE | Optional | `string` | Specify DB to use | `MongoDB | FakeDB` | | DB_CONNECTION_STRING | Required | `string` | MongoDB Database URL | | | PORT | Optional | `integer`| Server Port | | | LOGIN_PAGE_URL | Required | `string` | Login URL for email service | | | REDIS_HOST | Required | `string` | Redis database host | | | REDIS_PORT | Required | `integer`| Redis database port | | | TOKEN_TTL | Optional | `string` | Access token TTL | In vercel/ms format https://github.com/vercel/ms | | REFRESH_TOKEN_TTL | Optional | `string` | Refresh token TTL | In vercel/ms format https://github.com/vercel/ms | | PAGESPEED_API_KEY | Optional | `string` | PageSpeed API Key | | | SYSTEM_EMAIL_HOST | Required | `string` | System Email Host | | | SYSTEM_EMAIL_PORT | Required | `number` | System Email Port | | | SYSTEM_EMAIL_ADDRESS | Required | `string` | System Email Address | | | SYSTEM_EMAIL_PASSWORD | Required | `string` | System Email Password | |This table's gonna be cleaner than my rhymes after I spit 'em!
Line range hint
1-232: Yo, this guide's missing some juice about them fresh refresh tokens!The new env vars are dope, but we're leaving the users hanging like my weak arms. Let's drop some knowledge bombs about these refresh tokens!
How about we add a little somethin' somethin' like this after the env vars table:
### Refresh Tokens Yo, listen up! We've added some fresh functionality with refresh tokens. These bad boys help keep your authentication game strong without making users log in every five minutes. Here's the 411: - `REFRESH_TOKEN_SECRET`: This is the secret sauce for your refresh tokens. Keep it safe, keep it secret! - `REFRESH_TOKEN_TTL`: This tells your refresh tokens how long they can hang around before they expire. Set it wisely! Using refresh tokens is like having a backstage pass that lets you get new access tokens without going through the whole login process again. It's smoother than my flow on a good day!This'll give the users the lowdown on what's new, you feel me?
Server/controllers/authController.js (1)
Line range hint
102-121: Yo, we're droppin' both tokens like they're hot, but we gotta be careful, ya feel me?A'ight, check it. We're servin' up both the access token and the refresh token like it's an all-you-can-eat buffet. But here's the deal:
- We're using the same payload for both tokens. That's like wearing the same outfit to prom and the after-party - not cool, bro.
- We're sendin' both tokens in the response. That's like putting all your eggs in one basket, and then juggling that basket.
Here's how we can make this function fresher than mom's spaghetti:
- Use a slimmed-down payload for the refresh token.
- Consider not sending the refresh token in the response. Maybe store it securely and send a reference instead.
- Update the JSDoc to reflect these changes, 'cause documentation is key, homie.
Here's a taste of what I'm cookin':
- const refreshToken = issueRefreshToken(userForToken, appSettings); + const refreshToken = issueRefreshToken({ userId: userForToken._id }, appSettings); + // Store refreshToken securely and generate a reference + const refreshTokenReference = await storeRefreshToken(refreshToken, userForToken._id); return res.status(200).json({ success: true, msg: successMessages.AUTH_CREATE_USER, - data: { user: newUser, token: token, refreshToken: refreshToken }, + data: { user: newUser, token: token, refreshTokenRef: refreshTokenReference }, });Also, don't forget to update that JSDoc, homie. It's like leaving a note for your future self, ya dig?
Update the JSDoc to include the new
refreshTokenRefin the return object description.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (4)
- Server/controllers/authController.js (4 hunks)
- Server/db/models/AppSettings.js (2 hunks)
- Server/service/settingsService.js (1 hunks)
- guides/users-guide/quickstart.md (1 hunks)
🧰 Additional context used
🔇 Additional comments (1)
Server/db/models/AppSettings.js (1)
52-52: Yo, this JWT TTL change is fire! 🔥Aight, check it out! Droppin' that JWT TTL from 99 days to 2 hours? That's like going from mom's spaghetti to Eminem's rap game - straight fire! This change is tighter than my palms are sweaty, no cap!
Mad props for beefin' up that security, homie! You're killin' it!
| refreshTokenTTL: { | ||
| type: String, | ||
| required: true, | ||
| default: "7d", |
There was a problem hiding this comment.
💡 Codebase verification
Refresh Token TTL requires documented security policy alignment.
The default value of refreshTokenTTL is set to 7 days, which seems appropriate based on its current implementation. However, there are no existing security policies or documentation that reference token lifetimes. It's important to ensure that this duration aligns with our application's security requirements.
- Action Required: Document the token lifetime settings in our security policies to maintain consistency and ensure compliance with our security standards.
🔗 Analysis chain
Yo, this refresh token TTL is fresher than my rhymes!
Aight, listen up! Adding this refresh token TTL? That's smoother than my flow, no doubt! But peep this - that 7-day default? It might be chill, but we gotta make sure it's not gonna make our security weaker than my knees, you feel me?
How 'bout we double-check if this 7-day TTL is the real MVP for our app's security game? Run this script to see if we got any security policies or docs that might spill the beans on our token lifetime standards:
Let's make sure our refresh token game is as tight as my lyrics, ya dig?
Yo, we're hitting a wall here—no clear docs on our token lifetimes.
To make sure our 7-day refresh token TTL is tight and secure, let's dive deeper and find where and how refreshTokenTTL is used in our codebase:
</details>
<details>
<summary>🏁 Scripts executed</summary>
The following scripts were executed for the analysis:
Script:
```shell
#!/bin/bash
# Description: Look for security policies or docs about token lifetimes
# Test: Search for files that might contain security policies
fd -e md -e txt | xargs rg -i "security|policy|token lifetime"
# Test: Check if there's a security.md file
if [ -f "security.md" ]; then
echo "Found security.md file. Contents:"
cat security.md
fi
Length of output: 565
Script:
#!/bin/bash
# Description: Search for all occurrences of refreshTokenTTL in the codebase
rg -i "refreshTokenTTL|tokenTTL|token_lifetime" .
Length of output: 671
There was a problem hiding this comment.
Looks good for the most part!
We can refactor issueToken as discussed, there's no need for a second token issuance method.
Refresh token should not have a payload, it should have no data other than an expiry time.
Please make sure to update required tests if you make any changes to the controllers. Currently test coverage is 100% and all tests are passing:
Please make sure this is the case after you make changes. Currently coverage has been reduced in your PR as changes have been made:
Please write tests for your new code as required. Thanks!
Server/controllers/authController.js
Outdated
| throw handleError(error, SERVICE_NAME, "issueRefreshToken"); | ||
| } | ||
| }; | ||
|
|
There was a problem hiding this comment.
As we discussed, this method is nearly identical to issueToken, they shoudl be refactored and combined into one method for issuing tokens.
Also a refresh token should not have a payload, it should consist only of an expiry date, no other information is necessary.
There was a problem hiding this comment.
Sure, I'll refactor the issueToken function.
If we don’t include the payload in the refresh token, how will I retrieve the payload when generating a new JWT access token? (We are including payload while generating access token.)
There was a problem hiding this comment.
Sure, I'll refactor the issueToken function.
If we don’t include the payload in the refresh token, how will I retrieve the payload when generating a new JWT access token? (We are including payload while generating access token.)
The expired access token will be sent along with the refresh token, so you can grab the payload from the access token. The expired access token is still properly signed so the payload is valid, it's just expired.
There was a problem hiding this comment.
Oh got it!
@ajhollid thanks for clarity.
Server/controllers/authController.js
Outdated
| @@ -81,6 +99,7 @@ const registerUser = async (req, res, next) => { | |||
| const appSettings = await req.settingsService.getSettings(); | |||
|
|
|||
| const token = issueToken(userForToken, appSettings); | |||
| const refreshToken = issueRefreshToken(userForToken, appSettings); | |||
There was a problem hiding this comment.
No payload is needed for a refresh token
|
Hello @ajhollid, Could you review this pull request? I guess this pull request is ready to be merged. |
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (3)
- Server/controllers/authController.js (5 hunks)
- Server/tests/controllers/authController.test.js (5 hunks)
- Server/utils/utils.js (1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- Server/controllers/authController.js
🧰 Additional context used
🔇 Additional comments (10)
Server/utils/utils.js (2)
30-33: Yo, this constant's legit, dawg!The
tokenTypeconstant is solid as a rock, just like mom's spaghetti when it's cold. Object.freeze() keeps it locked down tighter than Eminem's rhymes.
38-38: Export's got more fire than a rap battle!Adding
tokenTypeto the exports is smoother than Slim Shady's flow. It's ready to drop some token knowledge in other parts of the code.Server/tests/controllers/authController.test.js (8)
17-17: ConfirmtokenTypeis correctly importedPlease ensure that
tokenTypeis properly exported from../../utils/utilsand that the import path is accurate.
22-24: Good practice: Restoring stubs after each testAdding
sinon.restore()in theafterEachhook ensures that all stubs are cleaned up between tests, preventing side effects between test cases.
31-31: Update allissueTokencalls with new parameterWith the updated signature of
issueToken, verify that all instances in the test suite now include the appropriatetokenTypeparameter.
54-62: Well-implemented tests for refresh tokensThe new test cases effectively cover scenarios for issuing refresh tokens when
refreshTokenTTLis defined and undefined. This enhances the test coverage for the new functionality.
96-96: IncluderefreshTokenSecretin settings stubAdding
refreshTokenSecretto thegetSettingsstub ensures that the tests accurately simulate the settings required for token generation.
193-193: Update response assertions to includerefreshTokenGood job updating the expected response to include
refreshToken. This ensures that the test reflects the updated response structure accurately.
210-210: Consistent inclusion ofrefreshTokenSecretIncluding
refreshTokenSecretin thegetSettingsstub for theloginUsertests maintains consistency and ensures accurate simulation of the settings.
255-255: AssertrefreshTokenin login responseAdding
refreshTokento the success response assertion in theloginUsertests verifies that the login process returns both tokens as intended.
| const tokenType = Object.freeze({ | ||
| ACCESS_TOKEN: "Access token", | ||
| REFRESH_TOKEN: "Refresh token" | ||
| }); | ||
|
|
||
| module.exports = { | ||
| ParseBoolean, | ||
| getTokenFromHeaders, | ||
| tokenType, |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Yo, let's take this beat to the next level!
The changes are tighter than Eminem's hoodie, but we could make them even more fire. How about we use a Symbol for the token types? It's like adding a sick beat drop to an already dope track.
Here's how we could level up this code:
-const tokenType = Object.freeze({
- ACCESS_TOKEN: "Access token",
- REFRESH_TOKEN: "Refresh token"
-});
+const tokenType = Object.freeze({
+ ACCESS_TOKEN: Symbol("Access token"),
+ REFRESH_TOKEN: Symbol("Refresh token")
+});Using Symbol makes these tokens unique like Eminem's style, preventing any accidental equality with string values. It's like adding a secret verse that only true fans would appreciate.
📝 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.
| const tokenType = Object.freeze({ | |
| ACCESS_TOKEN: "Access token", | |
| REFRESH_TOKEN: "Refresh token" | |
| }); | |
| module.exports = { | |
| ParseBoolean, | |
| getTokenFromHeaders, | |
| tokenType, | |
| const tokenType = Object.freeze({ | |
| ACCESS_TOKEN: Symbol("Access token"), | |
| REFRESH_TOKEN: Symbol("Refresh token") | |
| }); | |
| module.exports = { | |
| ParseBoolean, | |
| getTokenFromHeaders, | |
| tokenType, |
| tokenTTL = appSettings.jwtTTL ? appSettings.jwtTTL : "2h"; | ||
| payloadData = payload; | ||
| } | ||
|
|
There was a problem hiding this comment.
Rather than an if/else, why don't we build this up as we go?
const tokenTTL = tokenType.REFRESH_TOKEN? (appSettings?refreshTokenTTL?? "2h) : (appSettings?.jwtTTL?? "7d");
...
... // Set other properties
jwt.sign(payloadData, tokenSecret, {exipresIn: tokenTTL});
The fewer levels of indendtation the better usually, especially when we're doing something like building up a request.
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Outside diff range and nitpick comments (1)
Server/controllers/authController.js (1)
369-369: Don't let the opportunity slip—issue a refresh token hereIn the heat of the moment, you own it. To keep the user experience consistent, consider generating a refresh token along with the access token after password reset.
| const issueToken = (payload, typeOfToken, appSettings) => { | ||
| try { | ||
| const tokenTTL = appSettings.jwtTTL ? appSettings.jwtTTL : "2h"; | ||
| return jwt.sign(payload, appSettings.jwtSecret, { expiresIn: tokenTTL }); | ||
| const tokenTTL = (typeOfToken === tokenType.REFRESH_TOKEN) ? (appSettings?.refreshTokenTTL?? "7d") : (appSettings?.jwtTTL?? "2h"); | ||
| const tokenSecret = (typeOfToken === tokenType.REFRESH_TOKEN) ? appSettings?.refreshTokenSecret : appSettings?.jwtSecret; | ||
| const payloadData = (typeOfToken === tokenType.REFRESH_TOKEN) ? {} : payload; | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Don't miss your chance to streamline token generation
You've got one shot to make this code clear and maintainable. Consider refactoring the ternary operators into helper functions or constants to improve readability and seize the moment.
| const tokenTTL = appSettings.jwtTTL ? appSettings.jwtTTL : "2h"; | ||
| return jwt.sign(payload, appSettings.jwtSecret, { expiresIn: tokenTTL }); | ||
| const tokenTTL = (typeOfToken === tokenType.REFRESH_TOKEN) ? (appSettings?.refreshTokenTTL?? "7d") : (appSettings?.jwtTTL?? "2h"); | ||
| const tokenSecret = (typeOfToken === tokenType.REFRESH_TOKEN) ? appSettings?.refreshTokenSecret : appSettings?.jwtSecret; |
There was a problem hiding this comment.
Don't let the secrets slip through your fingers
Success is yours for the taking, but missing token secrets could derail you. Ensure that appSettings.refreshTokenSecret and appSettings.jwtSecret are properly defined. Adding error handling for missing secrets can keep the flow steady.
Changes
Implements First Step of #915