Add magiclogin token to email links #104
Conversation
WalkthroughThe changes in this pull request focus on enhancing the Changes
Assessment against linked issues
Possibly related PRs
Poem
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
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 8
🧹 Outside diff range and nitpick comments (4)
backend/authStrategies/magiclogin.ts (1)
2-2: Simplify the import statement for 'MagicLoginStrategy'You can simplify the import by directly importing the default export without using the
{ default as ... }syntax.Apply this diff:
-import { default as MagicLoginStrategy } from 'passport-magic-login' +import MagicLoginStrategy from 'passport-magic-login'backend/controller/authController.ts (2)
2-2: Group the 'jsonwebtoken' import with other third-party importsConsider grouping the
jsonwebtokenimport with other third-party libraries for better organization and readability.
43-43: Specify a more precise type for theredirectvariableThe variable
redirectis currently typed asany. For better type safety, consider specifying it asstring | undefined.Apply this diff:
- let redirect: any + let redirect: string | undefinedbackend/mail/mail.ts (1)
84-84: Update comment to reflect single recipientThe
tofield now contains a single recipient's email address, but the comment says 'list of receivers'. Update the comment for clarity.Apply this diff to update the comment:
to: recipient.email, // list of receivers + to: recipient.email, // recipient email address
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
⛔ Files ignored due to path filters (1)
backend/package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (4)
- backend/authStrategies/magiclogin.ts (2 hunks)
- backend/controller/authController.ts (2 hunks)
- backend/mail/mail.ts (2 hunks)
- backend/package.json (2 hunks)
🔇 Additional comments (4)
backend/package.json (3)
16-16: LGTM: Addition of jsonwebtoken dependencyThe addition of the
jsonwebtokenlibrary (version ^9.0.2) is appropriate for implementing the magiclogin token functionality as described in the PR objectives. This is the latest stable version, and the use of the caret (^) for version matching allows for compatible updates, which is a good practice.
34-34: LGTM: Addition of @types/jsonwebtoken dev dependencyThe addition of
@types/jsonwebtoken(version ^9.0.7) as a dev dependency is appropriate. This package provides TypeScript type definitions for the jsonwebtoken library, which will improve development experience and type safety. The version is compatible with the main jsonwebtoken library and is correctly placed in the devDependencies section.
Line range hint
1-62: Overall changes look good and align with PR objectivesThe changes to
package.jsonare minimal and focused, adding only the necessary dependencies for implementing the magiclogin token functionality. The additions ofjsonwebtokenand its TypeScript types are appropriate and consistent with the project's structure. No other modifications were made, which keeps the changes scoped to the task at hand.To ensure the new dependencies are properly integrated:
This script will help verify that the newly added dependencies are actually being used in the codebase, ensuring that the changes are not just in the
package.jsonfile but are also reflected in the implementation.✅ Verification successful
Dependencies Verified Successfully
The new dependencies, including
jsonwebtoken, have been properly integrated and are actively used within the codebase. Specifically,jsonwebtokenis imported inbackend/controller/authController.tsandbackend/authStrategies/magiclogin.ts, and its functionsjwt.signare utilized in the latter. No further issues detected.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Verify that the new dependencies are being used in the codebase # Check for imports of jsonwebtoken echo "Checking for imports of jsonwebtoken:" rg --type typescript 'import.*jsonwebtoken' # Check for uses of jwt.sign or jwt.verify functions echo "Checking for uses of jwt.sign or jwt.verify:" rg --type typescript 'jwt\.(sign|verify)'Length of output: 343
Script:
#!/bin/bash # Verify that the new dependencies are being used in the codebase # Check for imports of jsonwebtoken echo "Checking for imports of jsonwebtoken:" rg --type ts 'import.*jsonwebtoken' # Check for uses of jwt.sign or jwt.verify functions echo "Checking for uses of jwt.sign or jwt.verify:" rg --type ts 'jwt\.(sign|verify)'Length of output: 503
backend/authStrategies/magiclogin.ts (1)
34-35: Approve addition of user activation checkThe additional check for
await user.isActive()ensures that only active users can log in, enhancing security measures.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (3)
backend/authStrategies/magiclogin.ts (1)
44-55: LGTM with suggestion: New genAuthenticatedLink functionThe new
genAuthenticatedLinkfunction successfully implements the PR objective of adding a magiclogin token to email links. However, as mentioned in a previous review, this function can be simplified using the synchronous version ofjwt.sign. Consider refactoring as follows:export function genAuthenticatedLink(payload: { destination: string; redirect: string }) { const code = Math.floor(Math.random() * 90000) + 10000 + '' const token = jwt.sign({ ...payload, code }, secret, jwtOptions) return `${callbackUrl}?token=${token}` }This refactoring will improve code readability and reduce complexity while maintaining the same functionality.
backend/mail/mail.ts (2)
26-37: LGTM: Implementation of magiclogin token additionThe changes effectively implement the PR objective of adding magiclogin tokens to email links:
- Each recipient is processed individually, resolving the issue of shared button object mutation.
- Link authentication is correctly implemented and conditional based on the
authenticateLinkparameter and the presence of a magiclogin foreign key.- The new
_sendMailfunction is called for each recipient, allowing for individual email customization.Minor optimization suggestion: Consider using
Array.prototype.forEachinstead of a traditionalforloop for better readability:recipients.forEach(async (recipient) => { const language = recipient.settings.language; const recipientButton = { ...button }; if (authenticateLink && recipient.fk.magiclogin && recipientButton.link.startsWith(process.env.VITE_FRONTEND_URL)) { recipientButton.link = await genAuthenticatedLink({ destination: recipient.fk.magiclogin!, redirect: recipientButton.link.substring(process.env.VITE_FRONTEND_URL.length) }); } _sendMail(recipient, subject, paragraph, recipientButton, lastParagraph, language); });This change is optional and doesn't affect functionality.
Line range hint
39-90: LGTM: New_sendMailfunctionThe new
_sendMailfunction effectively encapsulates the email sending logic for a single recipient:
- It improves code modularity by separating the email sending logic from the recipient processing logic.
- The early return when
mailClientis undefined addresses a previous review comment about inconsistent return statements.- The function correctly uses the provided parameters to generate and send the email.
Suggestion for improved error handling: Consider logging an error or throwing an exception when
mailClientis undefined, to make debugging easier in case of configuration issues:if (mailClient == undefined) { console.error('Mail client is not initialized'); return; }This change is optional and doesn't affect the core functionality.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (4)
- backend/authStrategies/magiclogin.ts (2 hunks)
- backend/controller/authController.ts (2 hunks)
- backend/mail/mail.ts (4 hunks)
- backend/templates/mail.ejs (1 hunks)
🔇 Additional comments (9)
backend/authStrategies/magiclogin.ts (4)
1-2: LGTM: Necessary imports addedThe imports for
jsonwebtokenandMagicLoginStrategyare correctly added and will be used in the implementation of the magic login functionality.
9-13: LGTM: Configuration constants properly definedThe
secret,callbackUrl, andjwtOptionsare correctly defined. TheexpiresInvalue injwtOptionsis now properly set to 120 minutes in seconds, addressing the previous issue mentioned in the past review comments.
Line range hint
18-41: LGTM: Updated sendMagicLink and verify functionsThe changes to
sendMagicLinkandverifyfunctions look good. Theverifyfunction now correctly checks if the user is both found and active.However, could you please clarify the purpose of the new
falseparameter added to thesendMailfunction call on line 27? It would be helpful to understand its significance in the context of the PR objectives.
Line range hint
1-55: Overall implementation looks good, with minor improvements neededThe changes in this file successfully implement the magiclogin token functionality as per the PR objectives. The new
genAuthenticatedLinkfunction and updates to theMagicLoginStrategyconfiguration work together to add magiclogin tokens to email links.To ensure the changes are fully integrated and don't cause unintended side effects, please:
- Address the remaining issues mentioned in the previous comments.
- Clarify the purpose of the new
falseparameter in thesendMailfunction call.- Consider the suggested refactoring for the
genAuthenticatedLinkfunction.To verify the impact of these changes on the wider codebase, please run the following script:
This script will help identify any other parts of the codebase that might need to be updated to align with the changes made in this file.
✅ Verification successful
Verification Successful: No Issues Found
The changes in
backend/authStrategies/magiclogin.tshave been successfully verified. There are no unintended usages ofMagicLoginStrategy, and other functions likesendMailandgenAuthenticatedLinkin different parts of the codebase remain unaffected by these updates.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check for any other uses of MagicLoginStrategy or sendMail that might be affected by these changes # Search for other uses of MagicLoginStrategy echo "Checking for other uses of MagicLoginStrategy:" rg --type typescript "new MagicLoginStrategy" -g '!backend/authStrategies/magiclogin.ts' # Search for other uses of sendMail with 5 parameters echo "Checking for other uses of sendMail with 5 parameters:" rg --type typescript "sendMail\([^)]+,[^)]+,[^)]+,[^)]+,[^)]+\)" -g '!backend/authStrategies/magiclogin.ts' # Search for uses of genAuthenticatedLink echo "Checking for uses of genAuthenticatedLink:" rg --type typescript "genAuthenticatedLink"Length of output: 666
Script:
#!/bin/bash # Description: Check for any other uses of MagicLoginStrategy or sendMail that might be affected by these changes # Search for other uses of MagicLoginStrategy in .ts and .tsx files echo "Checking for other uses of MagicLoginStrategy:" rg "new MagicLoginStrategy" --glob "!backend/authStrategies/magiclogin.ts" -g "*.ts" -g "*.tsx" # Search for other uses of sendMail with 5 parameters in .ts and .tsx files echo "Checking for other uses of sendMail with 5 parameters:" rg "sendMail\([^)]+,[^)]+,[^)]+,[^)]+,[^)]+\)" --glob "!backend/authStrategies/magiclogin.ts" -g "*.ts" -g "*.tsx" # Search for uses of genAuthenticatedLink in .ts and .tsx files echo "Checking for uses of genAuthenticatedLink:" rg "genAuthenticatedLink" -g "*.ts" -g "*.tsx"Length of output: 1253
backend/mail/mail.ts (3)
13-13: LGTM: Import statement forgenAuthenticatedLinkThe import of
genAuthenticatedLinkfrom the magiclogin module is correctly added and aligns with the PR objective of incorporating magiclogin tokens into email links.
18-24: LGTM: UpdatedsendMailfunction signatureThe changes to the
sendMailfunction signature are appropriate:
- The function is now correctly marked as
async.- The new
authenticateLinkparameter allows flexible control over link authentication.- The typo in the
paragraphparameter has been fixed, addressing a previous review comment.These changes align well with the PR objectives and improve the function's flexibility.
Line range hint
153-153: LGTM: UpdatedsendMailfunction callThe
sendMailfunction call insendNotificationMailhas been correctly updated to match the new function signature. This change ensures compatibility with the modifiedsendMailfunction while maintaining the existing notification mail functionality.backend/controller/authController.ts (1)
42-56:⚠️ Potential issueSecurity improvements needed in magiclogin callback handler
While the changes align with the PR objectives, there are two important security issues that need to be addressed:
Use
jwt.verifyinstead ofjwt.decode:
The current implementation usesjwt.decode, which doesn't validate the token's signature. This could allow tampered tokens to be accepted.Validate the
redirectparameter:
TheredirectURL from the token is not properly validated, which could lead to open redirect vulnerabilities.To address these issues, please apply the following changes:
const magicloginCallbackHandler = useMagicLogin ? (req: ExRequest, res: ExResponse, next: NextFunction) => { let redirect: any if (req.query.token) { - const token = jwt.decode(req.query.token as string) as jwt.JwtPayload - const redirectPath = token.redirect - if (redirectPath && typeof redirectPath === 'string' && redirectPath.startsWith('/')) { - redirect = redirectPath + try { + const token = jwt.verify(req.query.token as string, process.env.JWT_SECRET) as jwt.JwtPayload + const redirectPath = token.redirect + if (redirectPath && typeof redirectPath === 'string' && redirectPath.startsWith('/')) { + redirect = redirectPath + } + } catch (error) { + console.error('Invalid token:', error) } } passport.authenticate('magiclogin', { failureRedirect: process.env.VITE_FRONTEND_URL + '/login' + (redirect ? '?redirect=' + redirect : '') })(req, res, next) } : NotImplementedMiddlewareThese changes will:
- Use
jwt.verifyto validate the token's signature.- Wrap the token verification in a try-catch block to handle invalid tokens gracefully.
- Maintain the existing validation for the
redirectpath.Make sure to define
process.env.JWT_SECRETin your environment variables and keep it secure.To ensure that
JWT_SECRETis properly set, run the following command:This will search for occurrences of
JWT_SECRETin your project files, helping to verify its presence in configuration files or documentation.backend/templates/mail.ejs (1)
352-352: Typo correction approved.The change from
<%= paragaph %>to<%= paragraph %>corrects a typo in the variable name. This improvement ensures that the correct variable is referenced when rendering the email template.
Fixes #101
Summary by CodeRabbit
New Features
Bug Fixes
Documentation
jsonwebtokenfor enhanced token handling.Style