fix: Base invite email related issues #9907
Conversation
📝 Walkthrough📝 WalkthroughWalkthroughThe changes in this pull request involve modifications to the Changes
Assessment against linked issues
Possibly related PRs
Suggested labels
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
CodeRabbit Configuration File (
|
535a9c8 to
504d852
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (2)
packages/nocodb/src/services/base-users/base-users.service.ts (2)
438-448: Add JSDoc documentation for the sendInviteEmail methodWhile the new method signature with object parameters improves maintainability, adding JSDoc documentation would enhance code clarity and help other developers understand the purpose and usage of each parameter.
Example documentation:
/** * Sends an invitation email to a user * @param {Object} params - The parameters for sending the invite email * @param {string} params.email - The recipient's email address * @param {string} params.token - The invitation token * @param {NcRequest} params.req - The request object * @param {string} params.baseName - The name of the base * @returns {Promise<boolean>} Returns true if email was sent successfully * @throws {Error} If email configuration is invalid or sending fails */
464-464: Consider removing unnecessary fallbackSince
baseNameis now a required parameter in the method signature, the fallback toreq.body?.baseNameappears unnecessary and could be removed.- baseName: sanitiseEmailContent(baseName || req.body?.baseName), + baseName: sanitiseEmailContent(baseName),
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
packages/nocodb/src/services/base-users/base-users.service.ts(7 hunks)
🔇 Additional comments (5)
packages/nocodb/src/services/base-users/base-users.service.ts (5)
29-29: LGTM: Good addition of structured logging
The addition of a Logger instance improves error tracking and debugging capabilities.
176-195: LGTM: Fixed duplicate email issue
The modified logic properly handles email sending status and differentiates between single and multiple user invites, addressing the duplicate email issue mentioned in #8460.
Line range hint 198-203: LGTM: Enhanced error handling
The error handling is improved with proper logging and differentiated handling for single vs multiple user scenarios.
477-480: LGTM: Comprehensive error logging
The enhanced error logging provides better visibility into email configuration issues and includes stack traces for debugging.
176-195: Verify the complete invite flow
While the changes look good, let's verify that there are no remaining scenarios where duplicate emails could be sent.
Also applies to: 438-480
✅ Verification successful
Based on the shell script results, I can now provide a final verification response:
Invite flow verification completed - No duplicate email concerns
The code changes have properly addressed the duplicate email scenario. Here's the verification summary:
- The invite token generation is properly synchronized with email sending in both
base-users.service.tsandorg-users.service.ts - The email sending logic in
base-users.service.tshas proper conditional handling:- For single email: Sends email and returns token only if sending fails
- For multiple emails: Sends emails without returning tokens
- No other parts of the codebase attempt to send duplicate invite emails for the same token
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Search for all email sending calls in the codebase to ensure no duplicate sends
# Look for email sending patterns
echo "Checking for email sending patterns..."
rg "mailSend|sendInviteEmail" -A 5
# Look for invite token generation to trace the invite flow
echo "Checking invite token generation..."
rg "invite_token.*=.*uuidv4\(\)" -A 10
Length of output: 16136
|
Uffizzi Preview |
…late Signed-off-by: Pranav C <pranavxc@gmail.com>
504d852 to
efd1f7a
Compare
Signed-off-by: Pranav C <pranavxc@gmail.com>
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (4)
packages/nocodb/src/services/org-users.service.ts (1)
164-182: LGTM! The email sending logic effectively prevents duplicate invites.The implementation correctly handles both single and batch invites with appropriate error handling. The email is sent exactly once per user, addressing the duplicate email issue mentioned in #8460.
Consider enhancing the error logging for the batch invite case:
} catch (e) { - console.log(e); + console.error('Failed to invite user:', email, e); if (emails.length === 1) { throw e; } else {packages/nocodb/src/services/base-users/base-users.service.ts (3)
179-185: Add email configuration check before attempting to sendConsider checking for email configuration before attempting to send the invite email to fail fast and provide better error messages.
+ const emailAdapter = await NcPluginMgrv2.emailAdapter(); + if (!emailAdapter) { + return { invite_token, email }; + } const mailSendStatus = await this.sendInviteEmail({ email, token: invite_token, req: param.req, baseName: base.title, roles: param.baseUser.roles || 'editor', });
Line range hint
476-481: Update email subject to reflect invitation contextThe current subject "Verify email" doesn't accurately reflect that this is an invitation email. Consider updating it to be more specific.
- subject: 'Verify email', + subject: `Invitation to ${sanitiseEmailContent(baseName || req.body?.baseName)}`,
494-497: Improve error handling specificityThe current error handling logs all errors the same way. Consider categorizing errors:
- Configuration errors (e.g., missing SMTP settings)
- Network errors
- Template rendering errors
} catch (e) { + if (e.code === 'ECONNREFUSED') { + this.logger.error('Email server connection failed:', e.message); + throw new Error('Failed to connect to email server. Please check your SMTP settings.'); + } else if (e instanceof Error && e.message.includes('Template')) { + this.logger.error('Template rendering failed:', e.message); + throw new Error('Failed to render email template. Please check template syntax.'); + } else { this.logger.warn( 'Warning : `mailSend` failed, Please re-configure emailClient configuration.', ); this.logger.error(e.message, e.stack); throw e; + } }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (4)
packages/nocodb/src/services/base-users/base-users.service.ts(8 hunks)packages/nocodb/src/services/base-users/ui/emailTemplates/invite.ts(2 hunks)packages/nocodb/src/services/base-users/ui/emailTemplates/org-invite.ts(1 hunks)packages/nocodb/src/services/org-users.service.ts(2 hunks)
✅ Files skipped from review due to trivial changes (1)
- packages/nocodb/src/services/base-users/ui/emailTemplates/org-invite.ts
🔇 Additional comments (7)
packages/nocodb/src/services/org-users.service.ts (1)
241-247: LGTM! The resend invitation logic is correct and consistent.
The implementation ensures a single email is sent with the updated token, and all necessary parameters (including roles) are properly passed.
packages/nocodb/src/services/base-users/ui/emailTemplates/invite.ts (2)
6-6: LGTM! Enhanced email personalization.
The changes improve the email's clarity by:
- Using a personalized title with the base name
- Adding a descriptive preheader that includes both role and base name
Also applies to: 104-104
6-6:
Ensure proper HTML escaping for user-provided content.
The template uses user-provided content (baseName and roles) directly in HTML. Ensure these values are properly escaped to prevent XSS attacks.
Let's verify the escaping implementation:
Consider using EJS's built-in escape syntax <%- %> instead of <%= %> for these variables:
- <title><%= baseName %> - Invite</title>
+ <title><%- baseName %> - Invite</title>- style="color: transparent; display: none; height: 0; max-height: 0; max-width: 0; opacity: 0; overflow: hidden; mso-hide: all; visibility: hidden; width: 0;">You have been invited to become "<%= roles -%>" of the NocoDB base "<%= baseName %>".</span>
+ style="color: transparent; display: none; height: 0; max-height: 0; max-width: 0; opacity: 0; overflow: hidden; mso-hide: all; visibility: hidden; width: 0;">You have been invited to become "<%- roles %>" of the NocoDB base "<%- baseName %>".</span>Also applies to: 104-104
✅ Verification successful
Let me gather more information about the email template rendering and sanitization.
Let me check how EJS handles HTML escaping by default and if there are any configuration settings for it.
Let me check for the sanitiseEmailContent function since it appears to be used for sanitizing the content.
Let me check for the sanitiseEmailContent function implementation.
HTML content is properly escaped and sanitized.
The template variables are properly sanitized before being used in the HTML template:
-
The
baseNameandrolesvalues are passed throughsanitiseEmailContent()which:- HTML encodes special characters using numeric character references (&#xxx;)
- Wraps dots and URL separators in
<span>tags to prevent URL injection - Handles
<>&;?#,'"$and other special characters
-
The EJS template uses
<%=which HTML escapes content by default
The implementation provides adequate protection against XSS attacks through proper sanitization and encoding of user-provided content.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check if HTML escaping is implemented for EJS templates
# Look for EJS configuration or escaping utilities
rg -A 3 "configureEjs|escape|sanitize|xss"
# Check if the BaseUsersService sanitizes these values
ast-grep --pattern 'class BaseUsersService {
$$$
sendInviteEmail($$$) {
$$$
}
$$$
}'
Length of output: 155876
Script:
#!/bin/bash
# Check if there's any HTML escaping/sanitization in the email template rendering
rg -A 3 "renderEmailTemplate|compileTemplate|ejs\.render"
# Look for any HTML escaping utilities
rg -A 3 "escape|sanitize|xss" packages/nocodb/src/services/base-users/ui/emailTemplates/
Length of output: 2760
Script:
#!/bin/bash
# Look for EJS configuration or initialization
rg -A 3 "ejs\.(configure|renderFile|compile)"
# Check if there's any global EJS setup or middleware
rg -A 3 "app\.set.*ejs|app\.engine.*ejs"
# Look for any HTML escaping configuration in the base users service
ast-grep --pattern 'ejs.render($_, {
$$$
})'
Length of output: 3418
Script:
#!/bin/bash
# Look for the sanitiseEmailContent function definition
rg -A 5 "sanitiseEmailContent|sanitizeEmailContent"
# Also check for any email content sanitization utilities
ast-grep --pattern 'function sanitiseEmailContent($_) {
$$$
}'
Length of output: 132838
Script:
#!/bin/bash
# Look for the sanitiseEmailContent function implementation
rg -A 5 "export const sanitiseEmailContent" packages/nocodb/src/utils/emailUtils.ts
# Also check for any encode function used in sanitiseEmailContent
rg -A 5 "encode" packages/nocodb/src/utils/emailUtils.ts
Length of output: 812
packages/nocodb/src/services/base-users/base-users.service.ts (4)
1-1: LGTM: Good addition of structured logging
The addition of NestJS Logger will help with better error tracking and debugging.
Also applies to: 29-29
176-189: LGTM: Fixed duplicate email issue
The logic now properly handles email sending status and returns the invite token only when email sending fails, addressing the duplicate email issue mentioned in #8460.
Line range hint 405-429: LGTM: Improved error handling in invite resend
Good improvements:
- Added proper retrieval of user roles
- Added email plugin configuration check before attempting to send
443-457: LGTM: Improved type safety with proper parameter typing
The refactoring improves code maintainability by using a properly typed parameter object instead of individual parameters.
Change Summary
Change type
Test/ Verification
Provide summary of changes.
Additional information / screenshots (optional)
Anything for maintainers to be made aware of